fix(ce-debug): stop asking at the handoff, stop shipping unoffered work - #1385
Conversation
Phase 4 asked permission to open a PR whenever the user was already on a branch, and Phase 1.4 could propose a tracker ticket for a bug the user had already handed over as a Sentry issue. Neither ask had a real choice behind it. Route the handoff from branch state instead. A branch holding only the fix previews and opens the PR with no question. A branch also carrying unrelated work commits just the fix-owned files locally and pushes nothing, because ce-commit-push-pr pushes the whole branch and opens a PR spanning every commit on it. One blocking question survives: a fix-owned file that already held the user's edits, where no file-level commit separates them and every option loses something. Record whatever tracker or error-monitor item the user supplied as the issue of record, and demote Phase 1.4 to reading prior work. Linking an existing ticket stays allowed; creating a second record for the same bug does not. An input with no reference has no issue of record and needs none. Treat a dirty tree as a suspect during investigation rather than noise: stash with -u, rerun the reproduction, pop in the same step. Both outcomes are evidence, and the most common reason to be debugging with uncommitted work is that the uncommitted work caused the bug. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
PR SummaryMedium Risk Overview Phase 4 handoff drops the old “PR / commit / stop” menu. After a fix, routing uses two steps: commit only fix-owned files (plus optional residual artifact), then ship only when the branch holds nothing the user has not already offered—otherwise Issue of record: Phase 0 treats whatever the user supplied (including Sentry) as the single record—no duplicate tickets and no “open a Linear issue?” when input was only a stack trace or test. Investigation: Phase 1.2 treats a dirty tree as a hypothesis with a bounded Tests expand Reviewed by Cursor Bugbot for commit 8fc94ea. Bugbot is set up for automated code reviews on this repo. Configure here. |
![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: 5c51d257e3
ℹ️ 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".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… open Address PR review feedback (#1385). The fix-only route accepted a branch this skill created as proof the branch held nothing else. `git checkout -b` carries uncommitted work forward, so auto-branching off a dirty default branch produced a skill-created branch that still held the user's WIP — and then published it, which is the exact path this change set out to close. A clean pre-fix tree is now required on both arms. Restore the index when popping the investigation stash: a plain pop returns staged work unstaged and silently destroys the user's staging. Drop the clause that let Phase 3's edit confirmation stand in for the entangled commit question. Phase 3 asks about editing a dirty file, never about committing the user's edits with the fix, so reusing it collapsed the one decision this change deliberately kept. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
… list The routing stated three branch states and let the reader match one. That enumeration is where the WIP-publish hole lived: an `or` between "skill created the branch" and "the tree was clean" opened a gap that only existed because the cases were written as a switch. A reader reasoning from the goal gets the dirty skill-created branch right without it being listed. State the goal first — ship the fix without publishing anything the user did not offer up — and demote the cases to the shapes that judgment usually takes, with an explicit instruction to fall back to the goal when a branch state fits none of them. Keep the freshness discipline the recorded git-workflow decision actually asks for: establish what the branch holds by checking it now, never by assuming it from how the branch came to exist. Pin the goal and the refusal-to-push in the contract test instead of the three case labels, so the enumeration does not become definitional by test. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25b464ff3c
ℹ️ 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".
The shipping route stopped trusting branch creation as proof a branch held only the fix, but the quality tail still did, in two places: simplify took the branch diff when the branch was "skill-owned", and review ran default `ce-code-review` when "the branch was created by this skill". Automatic branch creation makes a dirty skill-created branch reachable without the user ever assenting, so both could hand a user's unrelated WIP to `ce-simplify-code` — which modifies what it is given. State the scope rule once for both passes, in the same shape as the shipping route: a pass goes branch-wide only when the branch holds nothing but this fix, and how the branch came to exist never establishes that. Both passes now defer to that rule instead of carrying their own condition. Codex flagged the simplify site; the review site had the identical defect and is fixed with it rather than left for another round. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
The quality tail chose between a branch-wide diff and the fix-owned files. That choice was never worth making: when the branch holds only the fix the branch diff equals the fix scope, so branch-wide buys nothing, and when it does not the branch diff is exactly the WIP-swallowing case the condition was added to guard. A conditional whose safe arm is redundant and whose other arm is the hazard should not exist. Simplify now always receives the fix-owned files that were clean before Phase 3 as an explicit scope. `ce-simplify-code` treats a named scope as authoritative and will not widen it, so passing one is the guardrail rather than a preference, and it matters most for that pass because it rewrites what it is handed. Review keeps a single narrower distinction on its own merits: it reads rather than writes, and `base:<pre-fix-HEAD>` is a better input than a file list when the pre-fix tree was provably clean. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc5f80a838
ℹ️ 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".
Three separate holes have now shipped in this block, each the same shape: the routes were a flat list of cases that looked mutually exclusive, so a state could match one entry and skip another entry's handling. "Repo with no remote" was the latest — it matched, called `ce-commit` with no scope, and lost both the fix-owned-files scoping and the entanglement question that the dirty-tree entries above it carry. The goal line did not catch it either, because it said "publishing", and committing a user's WIP locally is not publishing. The block asks two orthogonal questions instead. What goes into the commit is always the fix-owned files and nothing else, on every route, remote or not, with the entanglement question attached to it. Whether it ships is a separate test needing both a remote and a branch that holds nothing but the fix; failing either commits and pushes nothing. No state can now answer one question by matching the other. Widen the goal to cover commits, not just pushes and PRs, and pin the independence of the two questions rather than the case labels. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bf96e9de5
ℹ️ 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".
…tion The previous commit split the handoff into two questions but wrote the first one as an instruction to invoke `ce-commit`. That produced two regressions it was supposed to prevent: on the clean shipping path the agent committed there and then invoked `ce-commit-push-pr`, which commits again — putting the commit before the interrupt preview — and in a non-git repo it attempted a commit before reaching the stop that says there is nothing to hand off. Question 1 now only says what may go into the commit and explicitly does not commit. Question 2 owns the single commit, states that exactly one of its routes runs, and names who commits on each: `ce-commit-push-pr` under question 1's scope when it ships, `ce-commit` under the same scope when it stays local, and nothing at all outside a repo. Both regressions were caught by review on the restructure itself, so pin the constraint-not-action property and the exactly-one-route property. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
…is used
The ship gate rejected any branch with commits since the default branch, which
includes the already-pushed commits of a branch that has an open PR. Debugging
on such a branch — the ordinary case — therefore refused the shipping route and
left the fix local, the exact outcome this change set out to remove. The guard
was aimed at the wrong property: pushed commits are published work, not the
user's unoffered WIP. Test for unpublished commits instead (`@{u}..HEAD` with an
upstream, `<default-branch>..HEAD` without), and note that `ce-commit-push-pr`
updates an existing PR rather than opening a second one.
The gate also accepted any configured remote, while `ce-commit-push-pr` pushes
to `origin` by name, so a checkout whose only remote is `upstream` passed the
gate and then failed the push. Require `origin`.
Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0abed55bee
ℹ️ 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".
The previous commit swapped "commits since the default branch" for "unpublished commits" so a branch with an open PR could still ship. That overshot: pushing is not offering. WIP pushed for backup or to trigger CI, with no PR open, counted as published, so the gate passed and `ce-commit-push-pr` would open a first PR spanning work nobody put up for review — the publish path this change exists to close. Neither push state nor commit count is the right property. What matters is whether the branch's other commits are already under review: an open PR means they are offered and the run updates that PR, and no PR means the branch must hold nothing but this fix before a first PR spans it. The two prior revisions of this gate each satisfied one reviewer and broke the other's case, so pin both directions. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
The commit-scope constraint said fix-owned files and nothing else, but Phase 3 defines fix-owned files as the tests and implementation changed for the bug. The residual fallback file the quality tail writes when no tracker is reachable is neither, so the constraint excluded the one artifact `post-fix-handoff.md` explicitly stages with the fix — leaving an accepted finding uncommitted and breaking the durable-sink contract on the local-only route. Name that artifact as the single admitted addition rather than loosening the constraint. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de048c1b3f
ℹ️ 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".
…gn stash Three review findings, all on paths this change introduced. The ship gate's open-PR arm dropped the unpushed check, so a branch with an open PR plus local unpushed commits passed and `ce-commit-push-pr` pushed those into the PR. Each revision of this gate kept one property and lost the other; both are needed together. No unpushed commits other than this run's work, and, when no PR is open, no commits beyond this run's work at all — because a first PR spans every commit on the branch. The stash experiment popped unconditionally. `git stash push` creates nothing when the dirty state is one it cannot stash (a modified submodule is the common case), so the pop took the user's pre-existing stash instead and destroyed unrelated work. Record the stash tip first and pop only when the push actually created one. Answering the entangled question with "leave the fix uncommitted" or "stop" fell through into question 2, where the dirty tree selected the local commit route and committed the very file the user chose to leave alone. Those answers now end the handoff. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f11eeb147b
ℹ️ 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".
…ble remote
Applying `git log @{u}..HEAD` on every arm broke the most common path outright:
Phase 3 creates branches with `git checkout -b`, which leaves no upstream, so
the check failed and a branch holding only the fix fell through to the local
route instead of opening a PR. The repo's own git-workflow guidance separates
"upstream exists" from "unpushed commits"; this gate had conflated them.
The two arms need different tests and an open PR guarantees an upstream, so
`@{u}` belongs only there. With a PR open, require no unpushed commits other
than this run's work. With no PR, require no commits beyond this run's work at
all, and do not consult `@{u}` — there may be no upstream, and pushed alone
would not mean offered.
Also gate on a PR-capable `origin` rather than its mere existence. Push happens
before PR creation, so a non-GitHub remote, or `gh` missing or unauthenticated,
publishes the branch and only then fails to open the PR it promised.
Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f013f0aaf
ℹ️ 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".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5f013f0. Configure here.
Both ranges compared local refs, which hides the case that matters. When the
user's own default branch holds unpushed commits and Phase 3 branches off it,
`git log <default-branch>..HEAD` is empty, the gate fires, and the run publishes
those previously unoffered commits in a first PR. Compare against
`origin/<default-branch>` after fetching instead — the range
`ce-commit-push-pr/references/branch-creation.md` already uses for this reason.
The open-PR arm also asserted that an open PR guarantees an upstream. It does
not: `git switch --no-track` and `git branch --unset-upstream` both leave a
branch with a PR and no tracking config, where `git log @{u}..HEAD` exits 128
rather than answering, so the fix stayed local for no reason. That arm now
measures against the PR's remote head and does not consult `@{u}` at all.
Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa014a0ac8
ℹ️ 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".
…mands
This gate went through six revisions and five of them were wrong for some git
configuration: a wrong proxy for a clean tree, two wrong ranges, a range dropped
from one arm, `@{u}` where no upstream exists, and local refs where the local
branch was itself ahead of the remote. No reviewer ever found the intent
unclear. Every finding was the prescribed mechanism failing somewhere, which
means prescribing the mechanism was what generated them — and `ce-commit-push-pr`
already owns git mechanics, so re-deriving them here duplicated another skill's
job badly.
The gate now states what must be true — clean pre-fix tree, nothing on the
branch the user has not already offered, a remote `gh` can open a PR against —
and leaves how to establish it to the agent and the repo in front of it. What
stays is the knowledge that cannot be derived: the push covers the whole branch
and precedes PR creation, an open PR means offered while a bare push does not,
a local ref can be ahead of the remote, and branch creation carries WIP forward.
Failing to establish it falls to the local route.
Pins move from the commands to those facts, since the commands were the part
that kept being wrong.
Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
The prior fix covered the case where `git stash push` creates nothing, but left the pop unqualified. A bare `git stash pop` takes the top of the stack, so an entry created while the reproduction ran — by test tooling, or by the user in another terminal — gets applied and dropped instead of this run's stash. State both failure modes and require restoring the exact entry the push created, rather than prescribing a command shape that the next git configuration breaks. Claude-Session: https://claude.ai/code/session_015eproCfjCz2WZdynWFwGiT
tmchow
changed the title
1 participant
Summary
ce-debugasked for two things it could work out itself. After a verified fix on a branch you were already on, Phase 4 opened a three-option menu — PR, commit, or stop — even though a reviewed fix belongs in a PR. And Phase 1.4 discovers the repo's tracker from remotes and commit-key patterns, so a bug handed over as a Sentry issue could still produce a "should I open a Linear ticket?" prompt. Both were performative: invoking the skill had already settled them.Removing them is the easy half. The hard half is that "don't ask" must not become "publish anything" — and that is where most of this PR went.
The handoff decides from state, and never publishes unoffered work
ce-commit-push-prcommits the changed files it finds, pushes the whole branch, opens a PR spanning every commit on it, and hands that PR toce-babysit-pr. So on a branch holding your in-progress work, "just open the PR" publishes what you never offered up — and a preview cannot catch it, because the preview deliberately does not wait.Phase 4 now asks two orthogonal questions instead of matching one flat case list:
ce-commit-push-pr, stay local viace-commit, or nothing at all outside a repo.Shipping requires a clean pre-fix tree, nothing on the branch you have not already offered, and an
originthatghcan actually open a PR against. One blocking question survives, deliberately: when a fix-owned file already held your edits, no commit separates them and every option loses something.The ship condition is stated, not specified. It went through six revisions here, and five were wrong for some git configuration — a wrong proxy for a clean tree, two wrong commit ranges,
@{u}where no upstream exists, local refs where the local branch was itself ahead of the remote. No reviewer ever found the intent unclear; every finding was a prescribed command failing somewhere. So the prescriptions are gone. What stays is the knowledge that cannot be derived: the push covers the whole branch and precedes PR creation, an open PR means offered while a bare push does not, a local ref can be ahead of the remote, andgit checkout -bcarries WIP forward. Establishing it is left to the agent and the repo in front of it, and failing to establish it falls to the local route.The same scoping now applies to the quality tail:
ce-simplify-codeis always handed an explicit fix scope rather than a branch diff. That pass rewrites what it is given, so an unscoped branch could have modified unrelated work in progress, not merely published it.The bug's record is whatever you handed over
Phase 0 records the supplied tracker or error-monitor item as the issue of record — a Sentry issue counts exactly as much as a Linear ticket. Phase 1.4 is demoted to reading prior work and cannot establish a new home for the bug. Two edges a narrower fix would have missed: an input with no reference (a pasted stack trace) has no issue of record and needs none, and linking an existing ticket stays allowed — only creating a second record is forbidden.
A dirty tree is a suspect, not noise
Phase 1.2 listed uncommitted changes among seven environment-sanity items, framing your WIP as noise to rule out. It is usually the opposite: the most common reason to be debugging with a dirty tree is that the uncommitted edit caused the bug. It is now a named hypothesis with a bounded experiment — stash, rerun the reproduction, restore — where both outcomes are evidence.
The restore is the delicate part and took two rounds to get right.
git stash pushcreates nothing when the dirty state is one it cannot stash (a modified submodule), and a baregit stash poptakes whatever is on top, which may be an entry that appeared while the reproduction ran. Either way it applies and drops work that is not yours. So: only the entry this run created, only if it created one,--indexso staged work returns staged,-uso untracked files do not survive the stash and read as "not the WIP."Validation
bun test3089 pass / 0 fail;release:validateandplugin:validate --strictclean.New contract pins cover what could regress silently: the two routing questions staying independent, the commit-scope question staying a constraint, the issue-of-record rule across Phase 0 / Phase 1.4 / the handoff reference, and — after six revisions — the ship condition's facts rather than its commands. The prose behavior itself is not mechanically testable; the pins guard the greppable contract only.
Ten rounds of automated review ran against this branch and every finding was valid. Several were defects introduced while fixing the previous round's, which is the honest reason the ship condition ended up stated rather than specified.
Security Disclosure
Three security-relevant surfaces, all net-tightening:
ghcan open a PR against — because push happens before PR creation, so a weaker gate publishes the branch and then fails.git stashexperiment. It is bounded: announce first, restore in the same step, restore only the entry this run created, never auto-resolve a pop conflict, and never stash for routing convenience.git log origin/<branch>..HEAD; a maliciously named branch interpolated into that is an injection vector (flagged in review). Dropping prescribed commands removed the placeholder entirely.No credential, permission, path-handling, converter-output, or dependency changes.
Agent Disclosure
Claude Code · claude-opus-5[1m]