Menu ▾ ▴

#138 fix(automations,git,mcp): dedupe re-reviews, fix base ref and hints

closed
nobody
bug (36)
2026-08-03
2026-08-03
Anonymous
No

Originally created by: theBGuy
Originally owned by: theBGuy

Three unrelated defects fixed together: automated PR re-reviews fired twice after a push, branch-name suggestions compared against a stale local branch in non-origin repos, and a set of MCP tools that can silently discard state carried no destructive annotation. Each ships its own changelog fragment.

Duplicate pr-sync re-reviews

The old dedup only remembered the last head fired for, and the runner only compared against the latest persisted review — so an eventually-consistent forge poll re-serving the previous head after a push (an A→B→A flap) was read as new work and paid for a second review.

  • src/lib/automations/sync.ts: replaces the lastFiredHead single-value map with firedHeads, keeping every head fired for per (kind, repo, ref); membership is tested with sameSha rather than set equality so a short SHA from one provider and a full SHA from another still match.
  • src/lib/automations/runner.ts: swaps getLatestReview for listReviews, filtered to the mode, and skips the run when any retained record covers the head — the history store's MAX_PER_GROUP window is what absorbs the flap.
  • Comment sweep replacing the "watermark" wording across src/lib/automations/runner.ts (rule-outcome docs, cancel/dismiss notes, rerunAutomation toast comment), sync.ts and src/features/repository/RepositoryView.tsx, since the mechanism is now "heads this mode already covered".

Branch-name base ref on non-origin remotes

committed_base_ref only ever tried origin/<default> before falling back to the local branch — the exact copy that goes stale — so a clone -o upstream repo diffed against local main.

  • src-tauri/src/mcp_server/generate.rs: after the origin probe, lists remotes and checks refs/remotes/<remote>/<default> in git remote order, mirroring git_default_branch's own precedence; doc comment updated to state the new order.
  • Adds committed_base_ref_resolves_a_non_origin_remote, a temp-dir repo with no origin, a hand-added upstream remote, and a same-named local branch, asserting upstream/main wins.
  • src-tauri/src/git/remote.rs: promotes remote_head_branch here as pub(crate) and narrows read_symbolic_ref to private; git_remote_default_branch now calls the shared helper on both its pre- and post-fetch paths.
  • src-tauri/src/git/branches.rs: drops its local duplicate of remote_head_branch in favor of the git::remote one.

MCP destructive_hint annotations

Flips the hint on the tools that can drop state the caller never named, or whose outcome isn't trivially recoverable, so a connected agent can prompt first.

  • src-tauri/src/mcp_server/write_forge.rs: request_reviewers, set_issue_assignees, set_pull_request_assignees, set_issue_milestone — all replace-a-collection / clear-by-omission tools; module doc now explains the two classes (payload-replaces-collection vs. not-trivially-recoverable, the latter covering merge_pull_request and update_release).
  • src-tauri/src/mcp_server/write_jira.rs: assign_jira_issue (omitted account_id unassigns) and update_jira_issue (labels replaces the whole set).
  • src-tauri/src/mcp_server/write_git.rs: merge_branch, whose ours/theirs strategy drops one side of every conflicting hunk; module doc records that destructive_hint and the ensure_destructive capability ladder answer different questions and don't track 1:1.
  • src-tauri/src/mcp_server/write_local.rs: set_review_notes, since an empty body clears hand-written text with no history or forge copy behind it.

remotes tool: one git remote -v

  • src-tauri/src/mcp_server/read_git.rs: the remotes tool now shells out once and parses the listing via a new parse_remote_v helper instead of a listing plus a get-url per remote; names come from every row (a URL-less remote emits only a bare name\t row) and URLs from the (fetch) row, with the old git_remote_url path kept as the fallback arm so git's "answers with the name itself" behavior is preserved.
  • Adds three unit tests covering the URL-less remote, a multi-push-URL remote (fetch row only, matching what get-url reports), and the ordinary fetch+push pair — measured against git 2.51.1.windows.1.

Doc touch-up

  • src-tauri/src/github/release.rs: softens the gh_release_sync_updater_notes doc comment to say a failed upload parks the patched copy on disk when it can.

Discussion

  • Anonymous

    Anonymous - 2026-08-03
     

  • Anonymous

    Anonymous - 2026-08-03

    Originally posted by: theBGuy

    Context for reviewers — deliberate calls and disclosures on the record before the ready-flip. The PR description covers what/why; these are the calls a review would otherwise re-derive.

    Deliberate calls

    1. The pr-sync dedup is now a covered-SET, and that deliberately removes one capability: force-pushing a PR back to an already-reviewed head no longer auto-fires a review (an eventually-consistent poll re-serving an old head is indistinguishable from that, and the old-head re-fire is the bug — observed live on [#137], store evidence on record). The manual review panel and a stopped row's Re-run remain the deliberate re-review paths.
    2. Package A ships without tests because this repo has no frontend test runner (standing testing posture) — verification is typecheck plus two adversarial review passes.
    3. committed_base_ref mirrors git_default_branch's probe order, and claims only that. In a corner (origin's tracking ref exists but its HEAD symref is unset while another remote resolved the default), the two functions can pick different remotes — the doc deliberately states order-mirroring, not same-remote; a same-remote claim was caught false in review and removed.
    4. destructive_hint now states the tool's semantic, not its permission tier. The write_git doc's 1:1 hint↔ladder claim is gone — they answer different questions. merge_branch is the proof case: hint-destructive (its ours/theirs strategies drop one side of every conflicting hunk, across files the caller never named) without being ladder-destructive.
    5. Kept-false, on record: close_pull_request (a static annotation can't express "irreversible on Bitbucket only"; its description carries the caveat) · the in-progress-conflict family (rebase_branch, cherry_pick, revert_commit, rebase-mode pull) — the real gap is that the MCP surface has no abort/continue tools, filed in the backlog as its own decision · the remaining write_git ladder tools, each with a recorded reason.
    6. set_review_notes flipped because it whole-value-replaces hand-written, unversioned notes with no history and no forge copy — strictly worse recoverability than the milestone/label flips it matches.
    7. The remotes tool is one git remote -v spawn, with the parse extracted and pinned by measured fixtures (git 2.51.1.windows.1): a URL-less remote emits a bare name<TAB> row (and get-url answers the name, exit 0 — the fallback preserves that degenerate answer exactly); a multi-URL remote emits exactly ONE (fetch) row, which equals get-url's answer. Side effect on record: this path no longer warms git_remote_url's TTL cache.
    8. The pr-sync gate comment names MAX_PER_GROUP rather than duplicating the literal 3 — a copied literal goes stale; the symbol doesn't.

    Disclosures

    1. firedHeads is per-process; a second app instance keeps its own map. Cross-instance duplicate protection rests on the runner's existing claim system, unchanged here.
    2. The A→B→A flap premise is grounded in [#137]'s live incident (review-store evidence), not re-reproduced against a live forge in this batch.
    3. Doc surfaces: changelog fragments only, deliberately — the affected behaviors have no README/site/help presence (verified independently by both review passes); the MCP module docs were updated in-code alongside the annotation changes.
    4. Local CRLF noise on runner.ts is the standing Windows checkout artifact (HEAD blob is LF; content verified clean via LF-normalized format checks; CI checks out LF).

    Verification at open: full cargo test 934/934 (6 new tests) · CI-form clippy clean · pnpm build clean · tsc -b clean · changelog:check 14 fragments valid · live MCP stdio probe: 119 tools, 18 destructive, remotes output byte-identical pre/post refactor.

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

     

    Related

    Tickets: #137

  • Anonymous

    Anonymous - 2026-08-03

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    No blockers — the pr-sync covered-set rewrite, the remote_head_branch consolidation, the committed_base_ref non-origin sweep, and the destructive_hint flips all hold up under tracing. I verified the module-doc claims against the actual annotation set (update_release is already destructive_hint = true at write_forge.rs:1114; edit_labels is add/remove so correctly left false; update_issue/update_pull_request take only title/body so correctly left false; merge_branch at write_git.rs:598 is gated on ensure_git_write, so the new module doc's hint-vs-ladder claim is accurate), and I independently confirmed note [#11]'s doc-surface call — README.md:577-579, site/src/data/capabilities.ts, and content.ts:1683-1685 carry no claim this change contradicts. Recorded decisions #1/#2/#3/#5/#7/#9 stand as noted; none is contradicted by code I can see. Findings below are one should-fix and a handful of nits.

    Tests

    • should-fix — write_forge.rs:758,885,909,1306, write_jira.rs:256,289, write_git.rs:596, write_local.rs:252: the eight destructive_hint flips are the entire payload of changelog.d/fixed-mcp-destructive-hints.md, and nothing in-repo pins them — verification was a manual stdio probe only. A future edit that flips merge_branch or set_pull_request_assignees back to false compiles, passes cargo test, and silently removes the confirm prompt a connected agent shows before auto-resolving a merge or clearing an assignee set. mcp_server/mod.rs:904-916 already pins the tool count via ToolRouter::list_all(), so the sibling test is a few lines: in the same mod tests, build handler(false, false, false, false), collect handler.tool_router.list_all() into the sorted set of names whose annotations report destructive_hint == Some(true), and assert_eq! it against an explicit literal list of the 18 (merge_pull_request, request_reviewers, set_issue_assignees, set_pull_request_assignees, update_release, set_issue_milestone, assign_jira_issue, update_jira_issue, set_review_notes, merge_branch, delete_branch, discard_changes, discard_all_changes, reset_to_commit, force_push, delete_remote_branch, drop_stash, delete_tag). Give it the same doc-comment framing the count test uses — that the list is updated only when a change intends to move the destructive set — so adding a destructive tool carries the obligation explicitly rather than tripping a mystery failure.

    • nit — generate.rs, committed_base_ref_resolves_a_non_origin_remote: the comment calls it "A clone -o upstream repo", but the fixture never writes refs/remotes/upstream/HEAD, so git_default_branch (branches.rs:396) resolves main through its local refs/heads/main fallback, not through upstream's HEAD — the opposite of a real clone -o upstream. Either add git symbolic-ref refs/remotes/upstream/HEAD refs/remotes/upstream/main after the update-ref (the assertion is unchanged), or reword the comment to describe the local-fallback path it actually builds.

    Readability

    • nit — the renamed "watermark" vocabulary survives in two spots the sweep missed: src/features/activity/ActivityDock.tsx:372 ("a claim/watermark still held") is the direct mirror of the rerunAutomation comment reworded at runner.ts:624, and src-tauri/src/automation_claims.rs:7 describes the frontend dedup as "an in-memory debounce map plus a tauri-store watermark" — the map now holds a head list per PR and the store side is a covered set; reword both to "an already-covered head" / "a per-PR fired-head list plus a tauri-store covered-set".

    • nit — src/lib/pulls/reviews-history.ts:159-160: listReviews's doc scopes it to "the 'Previous reviews' disclosure", but runner.ts:282-284 now makes it the load-bearing pr-sync covered-set read; name both consumers so a future change to its filtering knows it isn't UI-only.

    • nit — src/features/repository/RepositoryView.tsx:222: the line above the one you edited still claims useWatchPrHeads covers "(local + remote)", but useWatchPrHeads.ts:8-11 watches open local PRs only (remote heads come from usePrNotifications/useBackgroundPrSync) — drop "+ remote" while you're in the comment.

    • nit — read_git.rs, parse_remote_v: names are collected first-wins while fetch_urls.insert is last-wins, and git remote get-url reports the first url; fetch_urls.entry(name).or_insert(url); makes both halves agree with git's answer for free.

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

     

    Related

    Tickets: #11

  • Anonymous

    Anonymous - 2026-08-03

    Originally posted by: theBGuy

    🤖 GitDesktop AI security audit · opus · automated

    No genuinely exploitable security issues in these changes.

    What I checked and why each changed sink is safe:

    • remotes (read_git.rs) — the new git remote -v parse replaces per-remote get-url, but both arms still pass the URL through the existing redact_url_credentials before it reaches the agent, so the credential-redaction guard is unchanged. The remote names taken from git's stdout are only emitted as JSON values; the one place a name is fed back into a spawn is the fallback git_remote_url, which still runs validate_remote_arg.
    • destructive_hint flips (write_forge/write_git/write_jira/write_local) — advisory MCP annotations only. Authorization is ensure_write/ensure_remote_write/ensure_git_write/ensure_destructive in mcp_server/mod.rs, all untouched; every flipped tool still calls its same gate (e.g. merge_branch → ensure_git_write). Flipping false → true cannot widen capability, and the module doc's update_release claim matches the code (destructive_hint = true at write_forge.rs:1114).
    • committed_base_ref (generate.rs) — the new {remote}/{default} base is built from local .git/config remote names, and its diff consumer git_branch_diff runs the project's validate_ref on base before interpolating the base...compare range, so the option/refspec-injection shape is covered by the established validator.
    • remote_head_branch / read_symbolic_ref (branches.rs, remote.rs) — pure move plus visibility narrowing (read_symbolic_ref became private); no behavior or argument construction changed.
    • pr-sync dedup (runner.ts, sync.ts) — the firedHeads/covered-set change gates whether a paid AI re-review fires; it is not an auth, trust, or output-sanitization boundary, and the cross-instance claim_automation_run path is unchanged. Unbounded firedHeads growth is resource-use, which is out of scope.

    Copilot flagged the parse_remote_v loop for missing trim_end() on CRLF; that's not a defect — Rust's str::lines() already strips a trailing \r, and even if a row did miss the " (fetch)" match the fallback arm redacts identically, so there's no credential-exposure path either way. Its other two comments are style/perf, not security.

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

     

  • Anonymous

    Anonymous - 2026-08-03

    Originally posted by: theBGuy

    Round-1 dispositions — the should-fix and all five nits verified and accepted; the batch is applied and rides the next push.

    1. Destructive-set pin test (should-fix) — fixed as proposed: destructive_tools_are_exactly_this_set sits beside the tool-count test, filters list_all() on destructive_hint == Some(true), and pins the sorted 18-name literal with the intends-to-move doc framing. Negative-controlled: temporarily flipping an unrelated tool's annotation made the test fail with the 19-name diff, then reverted — the pin is not vacuous. A fresh stdio probe independently measured the same 18.
    2. Fixture truthfulness (nit) — fixed the stronger way: the fixture now writes refs/remotes/upstream/HEAD, so it exercises the remote-HEAD resolution path end-to-end and the "clone -o upstream" description is true (the symref command itself was probe-verified in a scratch repo, not assumed from the still-passing assertion).
    3. Watermark stragglers (nit) — both reworded (ActivityDock.tsx, automation_claims.rs) to the covered-set vocabulary.
    4. listReviews doc (nit) — fixed: names both consumers (the "Previous reviews" disclosure and the runner's pr-sync covered-set read).
    5. useWatchPrHeads scope claim (nit) — fixed: "(local + remote)" corrected to local-only, with the remote-head sources named.
    6. parse_remote_v first-wins (nit) — fixed via entry(...).or_insert(...); the existing fixtures pass unchanged (measured: git emits a single (fetch) row, so this is agreement-by-construction with get-url, not a behavior change).

    Copilot's two comment-tightening nits are also in the batch (both threads replied + resolved); its parse_remote_v CRLF/O(n²) finding is declined with evidence on the thread.

    Verification on the batch: full cargo test 935/935 (the pin test included) · CI-form clippy clean · tsc -b clean · scoped biome clean (the standing runner.ts CRLF artifact aside) · changelog:check 14 valid.

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

     

  • Anonymous

    Anonymous - 2026-08-03

    Originally posted by: theBGuy

    🤖 GitDesktop AI review · opus · automated

    The three fixes and their doc/test collateral hold up: I re-traced the destructive-hint pin against the actual annotations (grep finds exactly 18 destructive_hint = true sites across write_forge.rs, write_git.rs, write_jira.rs, write_local.rs — matching the test's literal list name-for-name), the committed_base_ref upstream fixture now really exercises the remote-HEAD path, and the reworded comments all check out against the code they describe (useWatchPrHeads.ts:7-13 is local-only; reviews-history.ts:41,129-144 prunes MAX_PER_GROUP per (kind, ref, mode), so the runner's flap-window claim is exact; release.rs:528-537 really does have a "could not be saved" arm behind the new "when it can"). Nothing blocking.

    Readability

    • nit — src/features/activity/ActivityDock.tsx:372-373: "a claim or already-covered head still held by the canceled run unwinding" reads as a double or and attaches "held by the canceled run unwinding" to the covered head, which isn't something a run holds; …, or a claim still held by the canceled run unwinding, or a head this mode already covered) keeps both causes distinct.

    Copilot's parse_remote_v comment (CRLF-unsafe suffix match / O(n²) name scan) is not a real issue on the current code: str::lines() strips a trailing \r before strip_suffix(" (fetch)") sees it, and names holds one entry per remote (single digits), so no trim_end() or set is warranted.

    Resolved since last review

    • The destructive-set pin test is in place (mcp_server/mod.rs:918-961), filters list_all() on destructive_hint == Some(true), and its 18-name literal matches the annotations in the tree exactly; the doc comment carries the "moves only when a change INTENDS to move it" framing.
    • generate.rs:3029-3037 now writes refs/remotes/upstream/HEAD, so the fixture matches its "what a clone -o upstream writes" comment and resolves through the remote-HEAD path rather than the local fallback.
    • The "watermark" vocabulary is gone from ActivityDock.tsx:372 and automation_claims.rs:6-8 (now "per-PR fired-head list plus a tauri-store covered set").
    • reviews-history.ts:159-162 names both consumers of listReviews.
    • RepositoryView.tsx:222-224 corrects the scope to open local PRs and names where remote heads come from.
    • read_git.rs:325 uses fetch_urls.entry(name).or_insert(url), so the url map is first-wins like names and agrees with git remote get-url.

    Recorded decisions #1/#2/#3/#5/#7/#9/#11 remain as noted; nothing I can see in the current code contradicts them.

    Verdict: no blocking issues — remaining items are non-blocking; merge when ready

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

     

  • Anonymous

    Anonymous - 2026-08-03

    Originally posted by: theBGuy

    Round-2 disposition — the ActivityDock double-or nit is accepted and deferred with a home: the suggested phrasing is right (a covered head isn't something a run holds), but a comment-only push would spend a full review round on two lines. It rides the next automations-area PR (the backlog's MCP abort/continue item is the likely vehicle) — recorded there.

    With that on record: two rounds, all findings closed against on-record dispositions or deferred with homes, every inline thread resolved, CI green on this head (all seven checks, full matrix). Merge-ready.

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

     

  • Anonymous

    Anonymous - 2026-08-03

    Ticket changed by: theBGuy

    • status: open --> closed
     

Log in to post a comment.

MongoDB Logo MongoDB