fix: bind a confirmed action to the repository it was confirmed against - #33
Merged
Conversation
Every confirmation in gitActions is awaited with no timeout, and the work that
follows reads `appState.repo` when it RUNS, not when the dialog opened. Switch
repositories mid-prompt and the confirmed action lands on the new one.
The discard snapshot guard does not cover this: `expected_diff` pins the DIFF,
not the repository. Two clones or worktrees holding the same path with the same
diff pass the check, and the wrong copy is irreversibly discarded.
Codex flagged discardHunk and discardLines. The same gap is in every awaited
dialog in the file, so all nine are bound rather than the two reported:
runDestructive every caller (reset, rebase, amend, reword, ...)
runDestructiveRebase ditto
discardHunk reported
discardLines reported
discard (files) destructive
clean (untracked) destructive
force push --force-with-lease
create pull request
runRemote credential retry — `fn` re-reads the repo, so a switch during the
sign-in prompt would push to a remote the user never chose
Refusing rather than retargeting is deliberate: the user authorised an action
against what they were looking at, and silently applying it elsewhere is a
different action. This is the third appearance of the shape — guard, unbounded
await, act — after the diff snapshot and the sidebar ref actions, so the audit
covered the whole file rather than the reported sites.
Reported by Codex review on the v0.4.0 promotion (#30).
npm run check 530 files 0 errors; npm test 393 passed; cargo test 232 + 7.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2709753fbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…alog Two P1s on the previous commit, both the same mistake: the capture has to sit at the point the user's intent is fixed, and I put it immediately before the prompt instead. Where an earlier await exists in the same flow, a switch during THAT await is recorded as the "original" repo and the guard waves the action through. - `runRemote` awaits the first push/pull attempt before any credentials prompt. Captured before that attempt, so an auth failure arriving after a switch can no longer authorise a retry against the new repository — which would have pushed to a remote the user never chose, with the credentials just entered. - `createPullRequest` awaits `branchSubjects` before opening the dialog, while `branch` and `bases` were read from the repo BEFORE it. Captured before that lookup, so the guard compares against the repo those selections came from. The audit script had the same blind spot as the code: it checked for a capture near the dialog rather than ahead of every await in the flow. Rewritten to walk back to the enclosing function and reject a capture with any earlier await — and to strip comments and string bodies first, because its own first version flagged the word "await" inside the comments added here. Both invariants now hold: 9 captures, none preceded by a real earlier await; 9 awaited dialogs, all with a guarded use. npm run check 530 files 0 errors; npm test 393 passed; cargo test 232 + 7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ashproto
added
the
skip-build
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported by Codex review on the v0.4.0 promotion (#30) as P1.
nextis protected by a ruleset requiring a PR, so the fix lands here and #30 picks it up.The bug
Every confirmation in
gitActions.tsis awaited with no timeout, and the work that follows readsappState.repowhen it runs — not when the dialog opened. Switch repositories mid-prompt and the confirmed action lands on the new one.The discard snapshot guard does not cover this, which is the part worth being precise about:
expected_diffpins the DIFF, not the repository. Two clones or worktrees holding the same path with the same diff pass that check, and the wrong copy is irreversibly discarded.Scope: nine sites, not the two reported
This is the third appearance of the shape — guard, unbounded await, act — after the diff snapshot (#26) and the sidebar ref actions (#29/#32). So I audited every awaited dialog in the file rather than patching the two named:
runDestructiverunDestructiveRebasediscardHunkdiscardLinesdiscard(files)clean(untracked)--force-with-leaserunRemotecredential retryfnre-reads the repo, so a switch during sign-in would push to a remote the user never choseThe last one was not on my list either until the audit script flagged it — the retry re-invokes the caller's closure, so it is the same hazard with credentials attached.
The fix
sameRepoAfterPrompt()capturesappState.repobefore the await and returns a predicate checked after it. Both destructive wrappers hold their dialog internally, so guarding them covers every caller at once.Refusing rather than retargeting is deliberate. The user authorised an action against what they were looking at; silently applying it somewhere else is a different action, not a recovered one.
Verification
A script audits every
await dialogs.in the file and reports whether a capture precedes it and a guarded use follows: 9 sites, 0 unguarded. That check is what caught the credential retry, and it is the reason I trust the sweep is complete rather than merely thorough-looking.npm run check530 files / 0 errors ·npm test393 passed ·cargo test232 + 7.Honest limit: these paths need a running app and a mid-dialog repo switch to exercise, so they are verified by reading and the type gate, not by a test.
🤖 Generated with Claude Code