GitDesktop / Tickets / #137 fix(git,pulls,mcp,stash): correct base branch, merge SHA, stash toast

Anonymous - 2026-08-02

Originally posted by: cloudflare-workers-and-pages[bot]

Deploying gitdesktop with Cloudflare Pages

Latest commit: 73b288e

Status: ✅ Deploy successful!

Preview URL: https://a016717e.gitdesktop.pages.dev

Branch Preview URL: https://fix-patch-batch-default-bran.gitdesktop.pages.dev

View logs

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-02

assigned_to: theBGuy
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

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 the what/why; these are the calls a review would otherwise re-derive.

Deliberate calls

The updater-sync gate mirrors BOTH halves of the in-app editor's gate: asset presence (TagDetailView.tsx:154) and non-empty trimmed notes (:392). The second half is load-bearing: gh_release_edit trims and skips --notes when empty (github/release.rs:260-281), so an empty-notes sync would blank the live manifest while the release body stayed unchanged. notes_to_sync is captured from args.notes only — never the preserve-current fallback — so the manifest can only ever receive the caller's own text.

detect_non_github pre-gate + a direct gh_release_sync_updater_notes call (rather than routing through forge_release_sync_updater_notes) is deliberate: the forge wrapper errors on GitLab/Bitbucket, and this path requires a silent skip — a successful edit must never be turned into an error by the sync. If wrapper routing is preferred, its error arms would need to become skips first.

A sync failure is a caveat on a successful result (updater_manifest_error field), never a tool error — the edit already landed by then. The error text ships verbatim because it carries the recovery-file path for a clobbered manifest (--clobber is delete-then-upload; that parked copy is the caller's only recovery route).

A failed release-details read (reachable only when the caller supplies all four fields, so no pre-edit read exists) is disclosed, not silently skipped — silence would read as "this release has no manifest". Its wording deliberately asserts nothing about whether a manifest exists, because that's exactly what went unverified.

UPDATER_MANIFEST is duplicated in write_forge.rs with a cross-reference comment — the canonical const is private to github/release.rs, which this batch deliberately doesn't touch. Making it pub(crate) is a recorded backlog rider.

git_default_branch stays local-refs-only, no network — it has no State param and runs in read paths; the remote set-head --auto network write remains exclusively in git_remote_default_branch. A hand-added remote whose HEAD symref was never written intentionally falls through to the local main/master fallback (documented on the function).

The stash-ALL path is deliberately untouched — git_stash_all returns void; giving it a zero-match signal is a different (and UI-unreachable) scope. Only the path-selection stash carries the boolean.

toast.info for "Nothing to stash" follows the repo's no-op-outcome idiom (BranchSwitcher.tsx:851).

Disclosures (deferred with homes)

committed_base_ref (mcp_server/generate.rs:1774) is one step behind this fix: a non-origin clone now resolves a default branch but the remote-tracking-ref preference still probes refs/remotes/origin/ only, so it lands on the local branch there. Net improvement over the previous None; the completion (exposing which remote answered) is a recorded backlog item, its own package.

The sync's live round-trip (download → patch → clobber-upload) was not fired against a real release in this batch — it's covered by patch_updater_notes unit tests and code-trace; the GUI path it mirrors was E2E'd in [#135].

api.ts appears with a one-line comment trim only; any local format noise on it is the standing Windows CRLF checkout artifact (--numstat shows 1/2; biome lint clean; CI checks out LF).

cargo clippy --all-targets (stricter than CI's form) reports 2 pre-existing await_holding_lock hits in generate.rs test code from [#135]'s settings-lock serialization — untouched here and outside CI's gate.

Doc surfaces: changelog fragments only, deliberately. No README/site/help surface describes these behaviors (verified: README:421 and help content.ts:1141 describe the in-app updater sync and remain accurate; the MCP tool surface isn't documented in the help guide; the timeline docs never claimed a merge sha).

Verification at open: full cargo test 929/929 · both CI clippy forms clean · pnpm build clean · tsc -b clean · changelog:check 11 fragments valid.

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

Related

Tickets: ~~#135~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

The PR lands four independent fixes — non-origin default-branch resolution, the rename_all_fields serde fix for the PR timeline's merge sha, an MCP updater-manifest sync mirroring the in-app editor, and a "nothing to stash" toast — plus targeted tests for the first two. I traced each: the serde fix is complete (commit_oid is the only multi-word field in PrTimelineEventOut, the TS mirror at types.ts:1614 already declares commitOid?, and the GitLab/Bitbucket arms that also build Merged are covered by the enum-level attribute), the default-branch rewrite is behaviourally equivalent for the origin case and the three new tests genuinely discriminate, and the stash boolean is typed through useRepoMutation<TArgs, TData> correctly. Nothing blocking; two things worth fixing before merge.

Correctness & tool safety

should-fix — src-tauri/src/mcp_server/write_forge.rs, update_release tool metadata (description, lines 1082–1091; annotations(read_only_hint = false, destructive_hint = false), line 1092). The sync path now reached by default calls gh_release_sync_updater_notes, which uploads with --clobber; per that function's own doc (github/release.rs:466–472) gh implements --clobber as delete-THEN-upload, so a failed upload at release.rs:513–538 leaves the release with no latest.json at all, recoverable only from the parked temp copy. Concrete trigger: an agent calls update_release { tag, notes } on a release carrying a manifest (no opt-out passed, so args.sync_updater_notes != Some(false) is true), the download+patch succeed, and the upload fails on a transient 5xx — the live updater manifest is gone. An MCP client decides whether to auto-approve from exactly these two fields, and merge_pull_request (line 617) is annotated destructive_hint = true for the same "not trivially reversible" reason. Fix: append a clause to the description string, e.g. Syncing REPLACES the latest.json asset (delete-then-upload) — if the upload fails the release is left without a manifest and the patched copy's path is returned in updater_manifest_error., and make the annotation honest one of two ways: flip line 1092 to destructive_hint = true (accepting that plain title/state edits inherit the hint), or default sync_updater_notes to opt-in (.filter(|_| args.sync_updater_notes == Some(true)) at line 1158) so the false hint stays true — the latter also needs the description's "pass sync_updater_notes: false to leave it alone" reworded to "pass sync_updater_notes: true to sync it", and the changelog fragment changelog.d/fixed-mcp-release-updater-sync.md re-worded to match ("now can keep … in sync"). I'd take the first: opt-out matches the in-app editor's default and note 1's gate reasoning.

Tests

should-fix — src-tauri/src/mcp_server/write_forge.rs:1099–1107 and :1158: the notes_to_sync gate has no test. Both halves are load-bearing and both are pure logic: the .filter(|n| !n.is_empty()) is the only thing standing between a whitespace-only notes argument and a blanked live manifest (the edit skips --notes at github/release.rs:278–281, so the release body stays put while the manifest would be overwritten with ""), and != Some(false) is the caller's only escape hatch. The only test touched is all_write_tools_gated_on_allow_remote_write, which just adds sync_updater_notes: None to the struct literal. Fix: extract the gate next to sync_release_updater_notes as fn updater_notes_to_sync(notes: Option<&str>, sync: Option<bool>) -> Option<String> (moving the current comment at lines 1099–1101 onto it, reworded for the helper), call it from update_release as let notes_to_sync = updater_notes_to_sync(args.notes.as_deref(), args.sync_updater_notes); — still before the fallbacks consume args.notes — and drop the now-redundant .filter(…) at line 1158 so the if let Some(sync_notes) = notes_to_sync reads plainly; then add a #[test] in the existing mod tests covering None → None, Some(" ") → None, Some("n") + Some(false) → None, Some(" n ") + None → Some("n"). This fits the repo's pure-logic Rust unit-test posture and pins the guard that note 1 identifies as the reason the second half of the gate exists.

Nits

src-tauri/src/mcp_server/generate.rs:1769–1771: the committed_base_ref doc comment still says git_default_branch "returns the SHORT LOCAL name even when it derived it from origin/HEAD" — after this change it can derive from any remote's HEAD; change that phrase to "from a remote's HEAD". (The behavioural half — the remote-tracking preference still probing refs/remotes/origin/ only — stays a recorded deliberate deferral per note 9.)

src-tauri/src/git/branches.rs:382–395: the overwhelmingly common single-origin repo now pays two git spawns instead of one, because git remote always runs before the origin probe, and useDefaultBranch (src/lib/git/queries.ts:3376–3381) has no staleTime so it refetches on mount/focus; probing refs/remotes/origin/HEAD first and listing remotes only on a miss keeps the fix and the one-spawn fast path — the "Origin first when present (a stable sort…)" comment then belongs on the remaining loop, not the sort.

Recorded decisions I'm not re-raising: the duplicated UPDATER_MANIFEST const with its cross-reference comment (note 5, backlog rider), the unfired live round-trip (note 10), and the changelog-fragments-only doc call (note 13) — I confirmed independently that README.md:421–423 and content.ts:1141–1143 describe the in-app editor and stay accurate, that the README's MCP section documents the write ladder only categorically ("cut releases"), that the help guide makes no origin-only default-branch or always-creates-a-stash claim, and that CHANGELOG.md's ## [Unreleased] is correctly left untouched; all four fragments match changelog.d/README.md's format.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

No exploitable security issues found in these changes.

Notes on what I checked and why each risky-looking path is safe:

git_default_branch now interpolates remote names from git remote into refs/remotes/{remote}/HEAD. Names are passed as a single argv element to git symbolic-ref --quiet via run_git (no shell), always prefixed with refs/remotes/, so a --leading name can't become a flag, and read_symbolic_ref only returns a target that starts with the same prefix — the same output shape the origin-only code already produced, so no new refspec-injection surface for downstream consumers.

The new MCP update_release → sync_release_updater_notes path is gated by ensure_remote_write(), validates the tag (validate_tag rejects empty/leading--) inside gh_release_sync_updater_notes, and patch_updater_notes only replaces notes after asserting the Tauri manifest shape — platform URLs and signatures survive verbatim, so the update-channel integrity isn't weakened. It mirrors a capability the GUI already exposes, so it isn't a privilege escalation for an LLM caller.

The serde rename_all_fields, the pub(crate) visibility bump on an internal helper, the stash toast, and the comment trim carry no security-relevant sink.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Originally posted by: theBGuy

Round-1 dispositions — all four findings verified against the code and accepted; the fix batch is applied and lands in the next push.

Tool-metadata honesty (should-fix) — fixed, option 1 as you preferred. destructive_hint = true (matching merge_pull_request's precedent) plus the delete-then-upload clause in the description; the sync stays default-on/opt-out to mirror the in-app editor. One refinement beyond the suggested sentence: the clause is worded arm-neutral ("a failed upload can leave the release without a manifest; updater_manifest_error then carries the recovery details, including the patched copy's path when one could be parked") — the unconditional form would over-claim twice: the delete lives inside gh's clobber so a pre-delete failure leaves the manifest intact, and save_updater_recovery_copy is best-effort (release.rs:450-464 degrades to None, no path in that arm). The GUI's own summary made the same arm-neutral call (TagDetailView.tsx:428-435). Related residual logged: release.rs:466-472's own doc carries the same unconditional "names its path" phrasing (pre-existing, outside this PR's scope) — backlog.

Gate test (should-fix) — fixed as proposed. updater_notes_to_sync(Option<&str>, Option<bool>) -> Option<String> extracted next to the sync helper, call-site filter dropped, and the exact four cases pinned (None→None, whitespace→None, opt-out beats real notes, trim asserted). 87/87 in mcp_server, 930/930 full suite.

committed_base_ref comment (nit) — fixed ("derived it from a remote's HEAD"). The behavioral half stays deferred per context item 9, as you noted.

Origin fast path (nit) — fixed. refs/remotes/origin/HEAD is probed first (single-origin repos back to one spawn); the remote listing runs only on a miss and sweeps the others in git remote order. Semantics byte-identical — the three new tests pass unchanged. staleTime on useDefaultBranch deliberately not touched (out of this PR's scope; noted).

Sweep result from closing this round (deferred with a home, not this PR): the annotation-honesty class has siblings — set_pull_request_assignees / set_issue_assignees are documented REPLACE-the-full-set (an empty list clears) yet annotated destructive_hint = false, and edit_labels' remove arm is the same shape. Filed in the backlog as its own audit-and-fix batch.

Also in the next push: Copilot's doc-length nit (the sync helper doc, 8 → 6 lines — thread replied + resolved).

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

All four round-1 findings are fixed and I re-verified the fixes as new code: the origin fast path in git_default_branch (branches.rs:390–406) is semantically identical to the loop it replaced — read_symbolic_ref swallows the non-zero symbolic-ref exit into Ok(None) (remote.rs:376–397), so the ? on remote_head_branch can't turn a missing origin into an error, and the three tests still discriminate (the canonical-sorts-first fixture still rules out "first listed wins"). The update_release metadata now matches behavior, and the extracted gate is exercised.

Resolved since last review

Tool-metadata honesty — write_forge.rs:1104–1109: destructive_hint = true plus the delete-then-upload clause. I checked the wording against the code it describes: gh_release_sync_updater_notes (release.rs:513–538) only reaches the recovery arm on upload failure, and save_updater_recovery_copy (release.rs:450–464) degrades to None, so "can leave the release without a manifest" / "when one could be parked" is accurate rather than over-claiming.

Gate test — updater_notes_to_sync (write_forge.rs:392–407) with the four cases pinned at :1777–1796; the call-site .filter(…) is gone and if let Some(sync_notes) = notes_to_sync at :1168 reads plainly. notes_to_sync is still computed at :1117, before args.notes is consumed by the preserve-current fallbacks at :1141–1144, so the manifest can still only ever receive the caller's own text. Doc claims on the helper ("the edit trims and skips --notes") check out against release.rs:260–281.

committed_base_ref doc — generate.rs:1770 now says "from a remote's HEAD".

Origin fast path — single-origin repos are back to one git spawn; the stale "a stable sort keeps the rest in git remote order" comment went with the sort.

Copilot's doc-length nit on sync_release_updater_notes is addressed — the doc is 6 lines at write_forge.rs:409–415.

Leftover polish (non-blocking)

write_forge.rs:230–234 — the sync_updater_notes field doc says "Defaults to true; set false to leave the manifest alone" but never states the sync only fires when notes are supplied; an agent reading only the parameter schema can pass sync_updater_notes: true with no notes and get a silent no-op. Append: "Only applies when notes are given — without them the manifest is left alone."

ChangesPanel.tsx:625–626 — "the files were already gone by the time git ran" is narrower than the boolean: git stash push also no-ops when the selected paths still exist but no longer carry changes. "…so no stash exists to report — the selection no longer had changes when git ran" covers both.

Nothing further to raise.

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

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Originally posted by: theBGuy

Round-2 dispositions — both polish items verified and fixed, held as the closing batch (comment/doc-only, so they ride one final push rather than buying their own round):

sync_updater_notes field doc now states the coupling: "Only applies when notes are given — without them the manifest is left alone." Verified against the gate: updater_notes_to_sync returns None without notes, so sync_updater_notes: true alone is indeed a silent no-op.

The ChangesPanel comment now reads "the selection no longer had changes when git ran" — checked against the source of the boolean (git_stash_paths reports false only when git says "No local changes to save" for those paths), which covers both the files-gone and present-but-unchanged cases, as you noted.

Nothing further open on our side. After this batch lands, the board is: both rounds' findings closed against on-record dispositions, Copilot approved (thread resolved), security audit clean, CI green on every head so far.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Only two comment/doc lines changed since the last round; both land as described and I re-verified them against the code they describe. The rest of the diff is unchanged and still checks out — read_symbolic_ref is now pub(crate) and its Ok(None)-on-non-zero contract (remote.rs:376–397) keeps the origin fast path in git_default_branch from turning a missing origin into an error, git_remotes (remote.rs:267–275) returns the plain Vec<String> the sweep expects, and types.ts:1614 already declares commitOid?: string, so the rename_all_fields fix aligns the wire with the mirror PrTimeline.tsx:154 already reads. Nothing blocking.

Readability

nit — write_forge.rs:230–233, sync_updater_notes doc: the appended sentence repeats the prior clause ("set false to leave the manifest alone" / "the manifest is left alone"). Fold them: "Defaults to true, and only applies when notes are given; set false to leave the manifest alone."

Resolved since last review

write_forge.rs:230–233 — the field doc now states the coupling with notes, and it matches the gate: updater_notes_to_sync (write_forge.rs:399–407) returns None for notes == None regardless of sync, so sync_updater_notes: true alone is indeed the silent no-op the doc now warns about.

ChangesPanel.tsx:625–626 — the comment now reads "the selection no longer had changes when git ran", which covers both the files-gone and present-but-unchanged cases that make matched false; the onSuccess: (matched) callback still types cleanly off useStashPaths → gitStashPaths's invoke<boolean> (queries.ts:3438–3442, api.ts:798–799).

Nothing further to raise.

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

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Originally posted by: theBGuy

Round-3 disposition — the readability nit is accepted and deferred with a home: the suggested fold ("Defaults to true, and only applies when notes are given; set false to leave the manifest alone.") is correct, but a comment-only push would spend a full review round on it. It rides the already-filed backlog batch that audits the destructive_hint annotations on set_pull_request_assignees / set_issue_assignees / edit_labels — that batch touches exactly this file's tool docs, so the fold lands with it.

With that on record, both rounds since the last substantive fix have returned merge-when-ready with no blocking or should-fix findings; all inline threads are resolved. Pending only the rust matrix completing on this head, this PR is merge-ready.

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

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Four independent fixes (non-origin default-branch resolution, MCP release→updater-manifest sync, Merged.commit_oid wire shape, stash zero-match toast), each with a changelog fragment. The logic checks out: read_symbolic_ref (remote.rs:376–397) returns Ok(None) on a non-zero symbolic-ref, so the new origin-first/sweep path in git_default_branch never turns a missing origin into an error; commit_oid is the enum's only multi-word field (pr.rs:2573–2627), so rename_all_fields changes exactly one key and matches types.ts:1614 / PrTimeline.tsx:154; the sync gate mirrors edit_release's own trim-and-skip (release.rs:260–281), so an empty-notes sync can't blank a live manifest. Nothing blocking; two nits only.

Recorded decisions I'd otherwise raise, acknowledged rather than re-flagged: committed_base_ref (generate.rs:1778) still probes refs/remotes/origin/ only, so a -o upstream clone now lands on the local branch — a net improvement over the previous None, deferred with a home; and the changelog-fragments-only doc call — I checked README:421 and help content.ts:1141 (in-app updater sync) plus the guide's Stash (475–486) and default-branch (412–451) copy, and none of them state a claim this PR invalidates.

Readability

nit — write_forge.rs:230–233 (UpdateReleaseArgs::sync_updater_notes doc): "set false to leave the manifest alone" and "without them the manifest is left alone" say the same thing twice. Fold to one sentence: /// Defaults to true, and only applies whennotesare given; set false to leave the manifest alone.

nit — branches.rs:371–380 (remote_head_branch): it rebuilds the refs/remotes/{remote}/HEAD + refs/remotes/{remote}/ pair that git_remote_default_branch already builds inline (remote.rs:345–346), and living in branches.rs is why read_symbolic_ref had to widen to pub(crate). Moving remote_head_branch into remote.rs as pub(crate) beside read_symbolic_ref, having git_remote_default_branch call it for its first probe, and reverting read_symbolic_ref to private would fold both together; branches.rs then calls crate::git::remote::remote_head_branch at both call sites (lines 393 and 403).

Copilot's only open point — the sync_release_updater_notes doc being over the comment convention — was trimmed and now sits at the "~6 lines for multi-constraint blocks" ceiling; no action needed.

Nothing from the previous round is verifiably fixed in this diff (its one open nit, the field-doc repetition, is re-listed above), so there is no resolved list this round.

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

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Originally posted by: theBGuy

Note for the record: the 04:36 review above is a duplicate run against the pre-push head (3410401) — an app bug caught by this dogfood, now filed: a stale poll snapshot right after the push re-fired the sync for the old head (its analysis cites pre-closing-batch line numbers). Dispositions on its two items so nothing reopens as new:

The sync_updater_notes doc fold — already accepted and deferred with a home in the round-3 disposition above (rides the annotation-audit batch).

Moving remote_head_branch into remote.rs (letting read_symbolic_ref go private again and sharing the first probe with git_remote_default_branch) — reasonable polish, accepted-as-deferred: it rides the already-filed committed_base_ref follow-up batch, which reworks exactly this resolver area.

Board state stands: merge-ready — CI green on 73b288e (all 7 checks), two consecutive merge-when-ready verdicts on the real heads, all threads resolved, remaining polish deferred on record.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-03

Ticket changed by: theBGuy

status: open --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

fix(git,pulls,mcp,stash): correct base branch, merge SHA, stash toast

Milestone

Searches

Help

#137 fix(git,pulls,mcp,stash): correct base branch, merge SHA, stash toast

Default-branch detection across remotes

Merged-PR timeline commit

MCP release editing ↔ updater manifest

Stash-selection toast

Related

Discussion

Deploying gitdesktop with Cloudflare Pages

Related

Correctness & tool safety

Tests

Nits

Resolved since last review

Leftover polish (non-blocking)

Readability

Resolved since last review

Readability

Latest commit:	`73b288e`
Status:	✅ Deploy successful!
Preview URL:	https://a016717e.gitdesktop.pages.dev
Branch Preview URL:	https://fix-patch-batch-default-bran.gitdesktop.pages.dev