test(source-control): pin the dies-with-the-directory half of the in-progress reason - #2352
Conversation
|
Claude finished @kyle-sexton's task in 43s —— View job Security Review Complete
No security issues found. This PR is a documentation/test-only change with no security-relevant surface:
No workflow files ( |
…progress reason; correct the fixture's cherry-pick rationale Two follow-ups to #2315 (#2257): - The paused-merge case asserted only "recomputable" -- the clause carried over from the old wording -- so the #2257 half of the reason (the operation's transient state is LOST with the directory, close to the opposite claim) could regress silently. A second assertion pins it. - The landed+in-progress fixture's comment said a cherry-pick "would reuse the same object". That is not the mechanism: a cherry-pick creates a new commit, but with parent == HEAD it reproduces tree, parent, author, and message, and -- within the same second -- the committer timestamp, minting the identical SHA, so the branch commit is literally on the base and the fixture collapses (unpushed 0, landed n/a, nothing to diverge). Reproduced on CI while local runs straddled second boundaries and passed, which is exactly why the comment must warn the local-green reader off simplifying the twin back into a cherry-pick. CHANGELOG 0.52.2; plugin.json bumped to match. Refs #2257 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4db89ae to
8c3722d
Compare
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 31s —— View job Security Review Complete
No security issues found. This PR touches only test assertions, code comments, a changelog entry, and a version bump — no security-relevant surface:
No workflow files ( |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging. Re-run the job to retry the review; a new push also retries it only if the caller's Re-running does NOT help for every class:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c3722de2c
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 1m 28s —— View job Code Review Complete
No correctness, coverage, or maintainability issues found. (Security is out of scope for this lane per What I verified
One non-blocking note: the PR description's "Test evidence" section says "CHANGELOG 0.52.2 + plugin.json bump," but the actual diff bumps to |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ress fixture ordering Address review: unrelated on main precedes the twin, so cherry-pick would not mint an identical SHA here; document ordering constraint instead of the parent==HEAD collision case. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Test evidence (author-side, promised in the PR body)Full 58 = the merged suite's 57 cases plus the new |
Holding this one — the evidence above is pinned to a superseded headApplying Two things, and the second is the substantive one. 1. This PR left draft before the verdict it promisedThe comment above says: "The independent verifier's verdict … will be posted here before this PR leaves draft." The PR is now I am not questioning the work. I am pointing out that the stated gate did not hold, again, which is the whole subject of #2330. 2. The head moved — the evidence and the verifier pin are both staleSo the This matters more here than it usually would, because this PR's entire purpose is a discriminating check — proving that removing Note this is exactly the head-SHA staleness tradeoff raised in #2330: requiring a verdict to name the current head makes verdicts expire on every push, which is correct in principle and costly in practice. This PR is a concrete instance of the cost of not requiring it. To clear the holdEither is fine:
Then remove the label. Independent of the hold: the change is still warrantedConfirmed on current |
Substantiating the hold: the test file itself changed between the two headsI offered "show the two trees are equivalent" as a way to clear this without re-running. I checked that myself rather than leaving you to, and it does not hold — comparing blob SHAs for the files this PR is about:
So the The engine ( What clears it, concretelyRe-run at
Item 2 is the one that matters. A discrimination proof against If it turns out the delta between the two test files is purely comment text, showing that diff is enough and the existing run carries over — but show it rather than assert it. One thing I got right by accident, worth statingMy first instinct was to treat "head moved" as sufficient grounds on its own. It is not — a rebase that touches nothing under test would leave the evidence perfectly valid, and holding on that alone would have been process for its own sake. The hold is justified because I checked which files moved, not because the SHA changed. Same distinction this batch has been enforcing everywhere else: the question is what actually changed, not what a coarser signal implies. |
Hold cleared — the delta is comment-only, so the existing evidence carriesI offered two ways to clear this and said to try the cheap one first. I ran it myself rather than leaving you to, since I placed the hold: Every non-comment line is identical between the two heads. The only change is the fixture comment. Combined with The hold was right to place and right to drop — the test file genuinely was a different blob, and "different blob" is not the same question as "different behaviour". Checking which one it was took one command. The revised comment is a correction, and it revises something I championedWorth flagging, because I argued for the earlier wording. Old (the version I rescued to
New:
These say materially different things. The old one describes the SHA collision as a live hazard the fixture must dodge. The new one says the fixture's own ordering — If the new description is accurate, it is the better comment: it explains why the current sequence is safe rather than warning about a hazard that sequence has already eliminated, and it keeps the operative directive (do not replace the twin with a cherry-pick) while fixing the reason. A comment whose stated mechanism does not match the code is the kind that gets "corrected" into a real bug later. I have not verified which description matches the fixture as written — that is squarely within your verifier's remit and I would rather it check than take my word. Flagging only because I pushed for the old wording, so my endorsement of it should not be treated as evidence for it. Still true
|
Evidence at the live head
|
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-run the job to retry the review. A new push re-triggers this lane only if the caller's |
Independent verifier verdict — post-hoc, covering #2315 (
|
Follow-up to #2315 (issue #2257, closed there). No linked issue — the gap is documented in the batch coordination on #2330/#2342-adjacent threads and below.
What
…(staged result recomputable from base, sequencer position) dies with the directory, but the suite asserted onlyrecomputable— the clause carried over from the old wording — so the source-control:worktree cleanup classifies by cleanliness, not liveness — a clean worktree with a rebase in flight is removed without --force, and the 'locked' flag it honors is never armed #2257 half (the transient state is LOST with the directory, close to the opposite claim) could regress silently. A secondassert_containspinsdies with the directory.mainimmediately before merge).Test evidence
Full
landed-work.test.shrun on this tree posted below when complete; the new assertion is discriminating (removing thedies with the directoryclause from the engine's reason makes it fail — verifier will demonstrate).Draft until an independent verifier verdict is posted here, per the batch rule.
Related
9273751acarries the behaviour but not the reason-string assertion🤖 Generated with Claude Code