Menu ▾ ▴

#107 feat(terminal,automation): add custom terminal command and reclaim stale review claims

closed
nobody
bug (36) enhancement (85)
2026-07-23
2026-07-23
Anonymous
No

Originally created by: theBGuy
Originally owned by: theBGuy

Bundles two independent improvements: a new Custom command… terminal mode that lets you launch any shell-free command with a {path} placeholder (for multiplexers, wrappers, and terminals auto-detection doesn't know), and a fix for automation review claims that were starving PRs for up to 30 days when an app instance died before releasing them. The claim fix also makes missed-review catch-up work per review mode, so a failed general review is retried even after the security audit ran.

Custom terminal command mode

  • Adds launch_custom_command in src-tauri/src/fsops.rs, plus helpers tokenize_command, substitute_path, is_batch_file, and first_token_is_pathlike: it tokenizes the template (double-quote grouping, no shell), substitutes {path} per token so spaced paths stay a single argv entry, resolves the first token to an absolute executable via agent::resolve_named, and spawns it rooted at the repo. Batch files (.cmd/.bat) are rejected to avoid re-introducing a shell through cmd.exe.
  • Extends open_in_terminal in src-tauri/src/fsops.rs with a command parameter and dispatches the new custom-command kind ahead of the per-OS matchers.
  • Fixes macOS "Custom…" in launch_terminal_unix so a plain (non-.app) executable is spawned directly rooted at the repo instead of being mis-launched through open -a.
  • Adds pure unit tests in src-tauri/src/fsops.rs for tokenizing, path substitution, batch-file detection, and path-like first-token detection.
  • Adds the Custom command… UI in src/features/settings/TerminalSection.tsx: a new mode with a monospace command Input, per-platform placeholder, a non-blocking warning when {path} is missing, and mode-switching that preserves the other mode's stored value.
  • Adds the terminalCommand field (and default) to AppSettings in src/lib/settings/api.ts, and threads command through openInTerminal in src/lib/git/api.ts.
  • Passes the new terminalCommand at every "Open in terminal" call site: ReconnectDialog.tsx, ChangesEmptyState.tsx, ForgeNotReady.tsx, RepoList.tsx, RepositoryMenu.tsx, and SessionOpenMenu.tsx.

Automation claim reclaim and per-mode catch-up

  • Adds STALE_CLAIM_AGE (30 minutes) in src-tauri/src/automation_claims.rs and splits the exclusive-create into create_new_claim, so claim_in_dir now reclaims a claim whose mtime is older than the threshold (best-effort delete + one retry, yielding on a lost race) instead of waiting for the 30-day sweep. Adds tests covering reclaim, fresh-claim denial, and that the reclaimed file holds the new key.
  • Makes pr-open first-review per mode in src/lib/automations/runner.ts: it skips any mode that already has a review record or a matching dismissed head, so synthesized pr-open events only fire the mode(s) still missing a review.
  • Reworks prOpenEligible in src/lib/automations/sync.ts to return eligible when at least one mode still needs a review (previously required both modes missing), so a single failed or stolen mode can be caught up after the other mode ran.
  • Updates the guard comment in src/features/pulls/RemotePrView.tsx to reflect the per-mode eligibility.

Documentation

  • Updates the Integrations highlight in README.md and the External editor / Terminal section in src/features/help/content.ts to describe the custom-command mode and {path} placeholder.
  • Adds changelog fragments changelog.d/added-custom-terminal-command.md and changelog.d/fixed-automation-claim-starvation.md.

Discussion

  • Anonymous

    Anonymous - 2026-07-23
     

  • Anonymous

    Anonymous - 2026-07-23

    Originally posted by: theBGuy

    Context for reviewers — deliberate calls and evidence, one claim per item:

    1. The claim fix's 30-minute stale reclaim is safe by construction, and the rationale lives in the STALE_CLAIM_AGE doc comment. A DELIVERED review's dedupe does not depend on its claim: the pr-reviews record is written at delivery, and BOTH re-review gates (the new per-mode pr-open gate and pr-sync's same-sha skip) consult that record BEFORE the claim is ever taken. Reclaiming an old delivered claim therefore cannot cause a re-review. The accepted residual: a legitimately still-running review that outlasts 30 minutes can be double-claimed by a concurrently polling second instance — bounded to one duplicate review, strictly better than the 30-day starvation it replaces.
    2. Release-on-failure is NOT part of this PR — it already exists. The runner has released claims on cancel/no-changes/error since the stopped-rows work. This PR adds only the arm that machinery can't cover: a claimant that dies WITHOUT running its failure path (crash, kill, version-skewed old instance). A successfully delivered review still deliberately keeps its claim.
    3. The reclaim state machine is single-retry and fail-open on purpose: stale (mtime ≥ 30 min) → best-effort delete → retry exclusive-create EXACTLY ONCE; a second AlreadyExists, a failed delete, an unstattable file, or a future mtime (clock skew) all resolve to "someone else owns it". Fresh claims deny exactly as before; the 30-day sweep is unchanged.
    4. prOpenEligible flipped from "no prior in either mode" to "some mode missing" deliberately, paired with the runner's new per-mode pr-open gate — a synthesized catch-up pr-open runs ONLY the missing mode(s), never re-running a mode with a prior. The other caller (fireReadyReview in RemotePrView) was verified under the new semantics and its comment updated; the gate, not claim dedupe, is what prevents double reviews there (the PR [#91] invariant, preserved).
    5. resolve_named(names, Some(bin_path)) is exists-check-ONLY — it never falls through to the PATH search. That contract was live-confirmed the hard way (a bare-name template failed with "terminal command not found: cmd" in the dev app) and is why launch_custom_command routes by token shape: path-like first tokens (containing a separator) get the exists-check, bare names pass None so the resolver runs its real PATH/PATHEXT + Unix login-shell lookup. Please don't propose collapsing this back to an unconditional Some(first).
    6. The custom command is argv-only, end to end, by security design: quote-aware tokenization, {path} substituted per-token (a repo path with spaces/;/$() stays ONE argv token), the first token resolved to an ABSOLUTE path BEFORE current_dir is set (Windows resolves bare names against the child's cwd ahead of PATH — a repo committing wt.exe must never execute on a trusted template), resolved .cmd/.bat rejected (Rust ≥1.77 routes batch files through cmd.exe — BatBadBut — silently reintroducing a shell), CREATE_NO_WINDOW on Windows, and no open -a anywhere in command mode (it does not propagate cwd into a .app).
    7. Live E2E evidence, post-fix: in the dev app, a saved template cmd /c echo {path} > marker & cd >> marker produced a marker whose two lines both equal the repository path — proving bare-name PATH resolution, tokenization, per-token substitution, argv passthrough, and the child's working directory in one shot. The Settings UI affordances (the new dropdown mode, the shell-free helper text, the non-blocking missing-{path} warning) were verified live as well.
    8. terminalCommand is a SEPARATE field from terminalPath, and the dropdown value IS the mode — deliberately. Reusing one field would carry a stale value into the wrong launch arm when flipping modes; mode round-trips preserve BOTH stored values.
    9. The macOS "Custom…" change is a deliberate absorption of a [#99] leftover: a stored program that exists but is not a .app bundle now spawns directly with current_dir instead of mislaunching via open -a; the .app and missing-path fallbacks are unchanged.
    10. Docs: README and the in-app help wording extended IN PLACE (the "point at any executable" phrasing); the marketing site is deliberately untouched — it has no standalone terminal-integration capability line to extend. Two changelog fragments.

    Verification on this head: full cargo suite 828 passed / 0 failed (3 new claims tests, 8 new fsops tests), clippy -D warnings clean, pnpm build green, scoped biome clean. The batch was adversarially spec-reviewed pre-PR; the one blocker found (item 5) was fixed and re-verified live.

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

    Related

    Tickets: #91
    Tickets: #99

  • Anonymous

    Anonymous - 2026-07-23

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    Summary

    Two independent changes: a shell-free "Custom command…" terminal mode (with {path} substitution) threaded through every call site, and a stale-claim reclaim (30 min) for automation review claims plus a per-mode pr-open gate so a single missing review mode can be caught up. Overall the design is sound — the claim-reclaim safety argument holds up against the diff (both re-review gates consult the pr-reviews record before the claim is taken, so reclaiming a delivered claim can't re-fire a review), and the widened prOpenEligible is correctly bounded by the per-tick catchUpAttempted guard. One should-fix on the terminal-launch security invariant; the rest is solid.

    Correctness / Security

    • should-fix — src-tauri/src/fsops.rs, launch_custom_command (via first_token_is_pathlike → resolve_named). The comment asserts "Either branch yields an absolute path," but that invariant does not hold for a relative path-like first token. Trace: a token like ./bin/wt or bin\wt makes first_token_is_pathlike true → bin_path = Some(first) → crate::agent::resolve_named(&[first], Some(first)), and resolve_named (agent.rs:414-416) returns PathBuf::from(p) unchanged when pb.is_file() — it never canonicalizes. is_file() is evaluated against the app process's cwd, but Command::new(resolved).current_dir(path) then resolves/executes that relative path against the repo dir. So the two disagree: on Unix (chdir-then-exec) the existence check validates the app-cwd copy while exec runs the repo's copy — a confused-deputy corner that the "resolve to ABSOLUTE before current_dir(repo)" comment is specifically claiming to prevent. Real-world likelihood is low (users are steered to bare names / absolute paths by the placeholders, and the check is against app cwd not the repo), but the stated security invariant is provably false. Fix: canonicalize the resolved path (std::fs::canonicalize) — or reject a non-absolute resolved — before building the Command, so the path that's checked is the path that's executed.

    Tests

    • Solid coverage for the pure helpers (tokenize_command, substitute_path, is_batch_file, first_token_is_pathlike) and for the reclaim state machine (stale reclaim, fresh-denies, rewrite-with-new-key). No test exercises a relative path-like first token — worth adding once the canonicalize fix above lands, since that's the branch where the invariant currently breaks.

    Accessibility (nit)

    • nit — src/features/settings/TerminalSection.tsx, missingPathToken warning. The hint appears/disappears as the user edits the command but is a plain <p>, so a screen-reader user editing the field won't be notified. Consider role="status"/aria-live="polite" on the warning paragraph so it's announced when it toggles. Optional — it's a soft, non-blocking hint, and it correctly pairs an icon with text (not color alone).

    Docs-sync looks handled deliberately: README Integrations, the in-app help section, and both changelog fragments were updated; the marketing site has no dedicated "open in terminal" capability line to extend, so leaving it untouched for this sub-mode is reasonable.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-23

    Originally posted by: theBGuy

    🤖 GitDesktop AI security audit · opus · automated

    No security issues found. The terminalCommand input is sourced exclusively from the local user's settings store (never from a repo-shared config or any remote/attacker channel), so it's a trusted local setting; and launch_custom_command spawns shell-free with single-token {path} substitution (no re-tokenization, no shell — a malicious repo path can't inject), resolves the program to an absolute path before setting current_dir(repo) (defeating the Windows CWD-precedence hijack), and rejects resolved .cmd/.bat files (BatBadBut). The automation_claims stale-reclaim and the runner.ts/sync.ts per-mode gating change dedup behavior only, with no untrusted-data-to-sink or privilege-boundary crossing.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-23

    Originally posted by: theBGuy

    Round-1 dispositions (all four findings accepted and fixed; ready for the next push):

    • AI should-fix (relative path-like token breaks the absoluteness invariant) — accepted, fixed. Verified the mechanism: resolve_named returns a relative path-like token verbatim after exists-checking it against the APP's cwd, while Unix chdir-then-exec would resolve it against the REPO — checked file ≠ executed file. Fix is a pure ensure_absolute(p, base) helper joining a relative resolution onto the same app cwd the exists-check used, applied before the Command is built. Chose join over canonicalize deliberately: the executed bytes stay identical to the checked bytes, and it avoids Windows \\?\ verbatim-path quirks. Security comment rewritten to state the invariant honestly; pure unit test added (relative+base → joined absolute; absolute unchanged, platform-branched for the 3-OS matrix).
    • AI nit (warning not announced) — accepted, fixed: the missing-{path} hint now carries role="status" (implicit polite live region); icon+text unchanged.
    • Copilot (unbalanced quote) — accepted, fixed — see the thread reply.
    • Copilot suppressed low-confidence note (quoted-empty first token) — expanded, verified real, fixed: empty program token now rejected with a clear error before resolution, plus a deterministic test. Disclosure: this last guard (+test) was applied orchestrator-side rather than via another implementer round — slightly over the trivial-fix size letter (~9 lines) but mechanical, contract-free, and reviewer-identified; called out here for transparency.

    Also: the 3-OS Rust matrix came back green on the reviewed head (8247448); the fix delta re-ran fsops (21 passed) + clippy clean. Delta since review: fsops.rs + TerminalSection.tsx only.

    Posted by GitDesktop — automated agent comment, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-23

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    Summary

    Two independent changes: a shell-free "Custom command…" terminal mode ({path} substitution, threaded through every openInTerminal call site) and a 30-minute stale-claim reclaim for automation review claims plus a per-mode pr-open catch-up gate. The change is sound and I found nothing blocking. The previous review's one should-fix and its nits are all fixed in the current diff, and the widened catch-up eligibility — which I initially suspected of introducing a starvation regression — checks out once you follow the surrounding code.

    Resolved since last review

    • Relative path-like first token breaking the absoluteness invariant — fixed. launch_custom_command now applies ensure_absolute(resolved, &std::env::current_dir()?) right after resolve_named, pinning a relative resolution (./bin/wt) to the same app cwd the exists-check used, before current_dir(repo) is applied to the child. The security comment was also rewritten to state the invariant honestly, and ensure_absolute_joins_relative_and_passes_absolute covers both branches. Join-over-canonicalize is a reasonable call (byte-identical to the checked path, avoids Windows \\?\ quirks).
    • Missing-{path} warning not announced — fixed. role="status" added to the warning <p> (implicit polite live region); icon+text pairing preserved.
    • Unbalanced-quote tokenization (Copilot) — fixed. tokenize_command now returns InvalidArgument when the scan ends inside a quote, with tokenize_command_unbalanced_quote_is_error pinning both unterminated cases and the balanced happy path.
    • Quoted-empty first token — fixed. An empty first token is rejected with "terminal command must start with a program name" before it reaches the resolver, with a deterministic test.
    • spellcheck on the command input (Copilot) — fixed (spellCheck={false}).

    Correctness (verified sound)

    The widened prOpenEligible (now returns eligible when any mode still needs a first review, previously required both) is safe in the surrounding machinery, so it is not a finding:

    • The runner's per-mode pr-open gate (runner.ts:275-292) skips any mode with a prior review record, so an already-reviewed mode can't double-fire; a manual panel review is also visible here via getLatestReview, which the comment credits as the real double-review guard.
    • The apparent risk — a PR where one mode is reviewed and the other is permanently disabled staying eligible forever and hogging the "one PR per tick" catch-up slot — is closed by catchUpAttempted (sync.ts:96,160,171): a picked PR is marked synchronously before the await and excluded from later ticks regardless of outcome, so such a PR is attempted at most once per head per session (worst case one harmless no-op pr-open dispatch, since effectiveActions in types.ts:127-128 only yields enabled modes). Newer PRs are not starved.

    The claim-reclaim state machine (automation_claims.rs) is also sound: reclaim fires only on mtime ≥ 30 min, a failed delete or lost retry race both resolve to Ok(false), and a future mtime (duration_since → Err) is treated as not-stale. All eight openInTerminal call sites pass the new terminalCommand, and there is no Rust-side settings struct that would drop the new terminalCommand field on round-trip (the frontend store is the sole writer).

    Nothing else worth flagging.

    Posted by GitDesktop — AI output, verify before acting on it.

     

  • Anonymous

    Anonymous - 2026-07-23

    Ticket changed by: theBGuy

    • status: open --> closed
     

Log in to post a comment.

MongoDB Logo MongoDB