fix(repo-fleet-hygiene): report merged evidence on a protected branch - #2689
Conversation
|
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 1m 4s —— View job Security review — no issues foundScope:
The change adds a single Checked for the things this lane cares about:
This is an informational-only reporting fix with no security-relevant behavior change. No findings to report. |
|
Last security-reviewed head: |
![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: 9313504fcf
ℹ️ 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 4m 30s —— View job Code review completeScope: What the fix does: Adds an Finding posted inline (1, Important): The Verified independently:
Not independently verified (author-claimed only): Everything else in the diff (variable declaration, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 5m 2s —— View job Code review completeScope: files changed since the last review pass (commit
New finding posted inline (1, Important) on Caveat: this sandbox's Prior findings from earlier review passes remain unaddressed (no code in
Neither is a correctness bug (dead code, no wrong output), so I'm not re-posting duplicate inline comments on unchanged lines — just noting they're still open. |
|
Claude finished @kyle-sexton's task in 43s —— View job Security review — no issues foundScope: commits since the last-reviewed head
Skip gate: triggeredThe only change since the last security review is test-fixture data and assertions in
All values are static, author-authored test literals — no new user input, no injection sink, no execution of tainted data, no workflow/permissions/dependency changes. This has no security-relevant surface, so per the skip gate this lane stops here with nothing further to report. (For reference, the |
A branch carrying an exact-OID merged PR that is ALSO protected fell through both arms of the pr_match block and emitted nothing. The merged-worktree arm requires a non-main worktree; the merged-local-branch arm requires protected=false. A branch attached to the main worktree, or the canonical checkout's current branch, satisfies neither, so the strongest merge evidence the collector holds was computed and discarded. The tell that this was an oversight rather than a protection rule: the weaker merged-pr-tip-drift below carries no protection guard at all and does emit. Silence on the strong path therefore read as "nothing merged" rather than "merged, but protected". Adds a LOW merged-protected-branch finding naming which protection applies. The protection rule itself is unchanged -- the kind is deliberately absent from branch_action_kind(), so it is reported and never becomes a cleanup candidate or an action-plan entry. Note the default branch cannot reach this arm: it is excluded from merge evidence collection upstream. The reachable cases are a main-worktree attached branch and the current branch.
The fleet-finding-test-coverage gate requires every emitted finding kind to carry a Finding: assertion in the collector's test file, and correctly red-lined the new kind. Fixture is a branch attached to the MAIN worktree that is not the default branch: merged-worktree needs a non-main worktree and merged-local-branch needs protected=false, so it reaches the new arm and nothing else. The default branch cannot stand in — merge evidence collection excludes it upstream. Asserts the finding is emitted, that it names the protection reason, and that it never routes to branch cleanup.
7da876d to
eb8271e
Compare
|
Claude finished @kyle-sexton's task in 2m 51s —— View job Code review completeScope: all three changed files at head
Finding posted inline (1, Important) on Not re-flagged (already open from 3 prior review rounds, unchanged in this commit, non-blocking): the unreachable Everything else in the diff — the |
|
Claude finished @kyle-sexton's task in 1m 42s —— View job Security review — no issues foundScope:
Skip gate: triggeredThis Per the skip gate, a head whose reviewable surface is unchanged from an already-reviewed head gets no further security pass. The prior review of this same content (comment, run (Unrelated to security: the non-security review threads on this PR have open findings — dead |
CI ran the suite on Linux and showed the fixture never reached the new arm. is_main is `wt_index -eq 0` — the FIRST worktree record — so putting feature/main-attached at index 1 gave attached=true with is_main=false, which routes to merged-worktree instead. The branch is now the main worktree's own record. Three shapes fail to reach the arm and only the fourth works: the default branch (excluded from merge-evidence collection upstream), a non-first worktree record (routes to merged-worktree), an unattached non-current branch (routes to merged-local-branch), and the main worktree checked out on a non-default branch. Recorded in the fixture comment. Adds the version bump and CHANGELOG entry that check-changelog-parity requires whenever shipped plugin files change. The earlier no-bump approach was correct only while several PRs were staged in parallel and would have collided on --check-order; this is the sole open PR touching the plugin, so the bump is both required and safe.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
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, or workflow_dispatch this workflow with the PR number, to retry the review. A new push re-triggers this lane only if the caller's |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
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, or workflow_dispatch this workflow with the PR number, to retry the review (ci-workflows#227). A new push also retries it only if the caller's Re-running does NOT help for every class:
|
Two P2/Important review findings, both correct. Confidence: the evidence here is the same successful MERGED PR with an exact headRefOid match that gives merged-local-branch and merged-worktree HIGH. The confidence model separates evidence strength from disposition, so downgrading to LOW conflated the two. Raised to HIGH; the protection stays in the disposition, and the kind remains absent from branch_action_kind() so it is still never a cleanup candidate. Dead code: the default_branch protection-reason arm can never execute. pr_match is populated only under [[ "$branch" != "$default_branch" ]] and is reset each iteration, so a default branch never enters the enclosing block at all. I noted that reachability in the PR body and then left the unreachable branch in the code. Removed, with a comment recording why the case is impossible rather than merely absent. Reference table and CHANGELOG updated to match both.
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
1 participant
Closes #2687
Summary
A branch carrying exact-OID merged-PR evidence that is also protected fell through both arms of the
pr_matchblock and emitted nothing. Themerged-worktreearm requires a non-main worktree; themerged-local-brancharm requiresprotected=false. A branch attached to the main worktree, or the canonical checkout's current branch, satisfies neither — so the strongest merge evidence the collector holds was computed and then discarded.The tell that this was an oversight rather than a protection rule: the weaker
merged-pr-tip-driftimmediately below carries no protection guard at all and does emit. Silence on the strong path read as "nothing merged" rather than "merged, but protected".Fix
Adds an
elsearm emittingLOW merged-protected-branch, naming the PR number, the matchingheadRefOid, and which protection applies.The protection rule is unchanged. The new kind is deliberately absent from
branch_action_kind()— which matches onlymerged-local-branch— so it is reported and can never become a cleanup candidate, inflate the rollup candidate count, or enter the action plan.Documented in
reference/confidence-model.md; the repo already gates on every emitted kind appearing in that tier table, and that gate caught the omission during development.Verification
Replaying the exact conditions the collector evaluates, across all five reachable input shapes:
Two cases go from silent to reported; the other three are byte-identical, so no existing classification changes.
Also run:
bash -nclean,shellcheck -xclean,markdownlint-cli20 errors on the reference doc.What I could not verify, stated plainly
The full
audit-fleet.test.shsuite does not complete on Windows under Git Bash. It exceeded 500s here with no buffered output; two independent earlier attempts hit the same wall at a 9-minute budget. So the decision-table evidence above comes from isolating the arm, not from a suite run.I drafted a fixture assertion and then reverted it, because I could not run the suite to confirm it, and an unvalidated fixture edit to a shared harness risks breaking CI for everyone else. The shape a follow-up needs: a branch attached to the main worktree (index 0 of the worktree porcelain list) that is not the default branch, plus a matching row in the PR fixture table. Note the default branch cannot be used — it is excluded from merge-evidence collection upstream, which I discovered by trying it first.
That suite's Windows runtime is worth its own issue; it is now the second finding this session that could not be closed by running the tests that exist.
No version bump or CHANGELOG entry — a consolidating bump is landing separately, and parallel PRs staging the same version collide on
check-changelog-parity.sh --check-order.Related
Supersedes the mechanism described in #2600, which was closed by #2629 against a moved-identity stop that does not exist. That fix is real but addresses a different path; this defect survived it, which is why the symptom recurred after the issue was closed.
N/A