Repository navigation
Security review lane re-reviews a restacked layer whose files did not change #7669
Description
Problem
After a native-stack restack where no file in a layer changed, the code review lane skips with skip=no-change, but the security review lane runs a full re-review (pr-pipeline-sandbox run 38965516508; probe in #7668, Closes #7664). That is one wasted security run per restacked layer.
Proposed fix
Give the security review lane the same no-change skip the code review lane uses: when the PR's diff against its base is unchanged since the last reviewed head, record the reviewed-head marker for the new sha and skip.
The security-review caller is sync-managed from melodic-software/standards (components/claude-lanes-hosted/claude-security-review.yml), so the change lands there or in the ci-workflows reusable it calls, not in this repository's synced copy.
Done when
A restack that changes no file in a layer produces a skipped security-review run with a reviewed-head marker on the new head, and a restack that does change a file still gets a full review.
Related
- docs(source-control): re-stamp stacks reference at GA and correct queue text #7668, source-control: recheck stacked PRs at GA and probe them through the merge queue #7664 (stack probe)
- Not in scope: pr-refine and pr-unblock refusing upper stack layers is by design (only the default branch is a trusted base).
🤖 Generated with Claude Code
Activity
github-actions commentedon Oct 16, 2026
Proposed work class: none
Size: Small
Complexity: Medium
Uncertainty: Knowable
Flags: none
Blocked by: none
Understood: after a restack where no file in a layer changed, the code review lane skips with
skip=no-change, but the security review lane runs a full review. You propose giving the security lane the same no-change skip and recording the reviewed-head marker on the new sha. A restack that changes a file would still get a full review.The fix belongs in
melodic-software/standardsor the ci-workflows reusable, not this repo's synced copy, so a maintainer needs to confirm where it lands.Possibly related: #9272 (skip re-review of a base-merge-only head) and #9822 (ci-status and the no-change skip).
Automated triage. A maintainer reviews it.
feat(review): retry review lanes once on a fallback token and fix lan…
kyle-sexton commentedon Oct 25, 2026
Released for re-triage under the owner's 2026-10-24 rule: an issue stops for a person only for an intent or vision question, a security call, a host or attended run, or spend outside prepaid credit. The hold label came off and intake triage runs again now; it re-applies a hold, with a numbered question, when one of those applies.
To hold this issue again, add
needs-human(orstatus: needs-decision) and say why in a comment.melodic-automation-lanes commentedon Oct 25, 2026
Not admitted to the issue lane: no trusted-human acceptance criteria: a listed human must put the confirmed
- [ ]criteria in the body or post them as their own comment, or, on an issue a listed human filed, intake's newest refinement comment must hold them unedited. Admission takes the issue once each of those is met, when intake triages it again (.github/actions/resolve-config/admission-rules.json).melodic-automation-lanes commentedon Oct 25, 2026
The latest triage verdict holds nothing: admission moved the issue to
status: needs-decision; its comment says why.melodic-automation-lanes commentedon Oct 25, 2026
Refinement. A read-only run explored this repository, searched its issues and pull requests, and read the allowed docs to draft what follows. On an issue a listed human filed, it counts as the issue's spec once posted, while unedited; it is still model text, so check it.
Intended outcome: The security review reusable skips with a reviewed-head marker on the new head when a restack leaves the PR diff unchanged, and still reviews fully when a file changes.
Related work
Open questions
RECOMMENDED: Make the change in the ci-workflows claude-security-review reusable. Leave this repo's synced caller alone unless the caller must pass new inputs. Basis: .base/.github/workflows/pr-review-security-hosted.yml:3
RECOMMENDED: Keep it separate. A restack rewrites the head sha while the diff against base stays the same, which differs from a base merge. Share the diff-equality check if both are built. Basis: 9272
RECOMMENDED: Check by a workflow-level test in the reusable's repo, with fixtures for an unchanged-diff restack and a changed-file restack. Treat the sandbox run as a person's check. Basis: judgment
Sources read
Automated refinement by the lanes App. Answer with a comment whose first line is
**Decision recorded.**.kyle-sexton commentedon Oct 25, 2026
Held at
needs-humanfor one person check, per the owner's 2026-10-25 answer: a person restacks a sandbox stack once (one layer unchanged, one changed) and closes this issue if the unchanged layer is skipped. Evidence it is already fixed: the security lane now runs the same reusable as the code lane, whose no-change skip is not lane-specific (.github/workflows/pr-review-security-hosted.yml:3-8).kyle-sexton commentedon Oct 28, 2026
Host run. Both criteria pass. In a three-layer stack in
melodic-software/pr-pipeline-sandbox, the security lane skipped the layer that was only rebased and recorded its marker on the new head. It gave the layer that was rebased with an edit a full review.What ran (2026-10-28 UTC; the lanes ran in Actions on
melodic-software/ci-workflowspr-review-cli.ymlat4fbceb4(v0.65.9). Between that pin and this repository's v0.65.17 the scope and skip steps are identical):probe/7669-l1->main), #241 (l2->l1) and #242 (l3->l2). Each layer adds one.txtfile, so the docs-only skip cannot apply. Every layer got a full security review and aclaude-lane-reviewed-head lane=claude-security-reviewmarker on its head:06ee422in run 40693036349,0d06483in run 40694666601 andd12d640in run 40696181585.l1.txt(086c96b, run 40699356843). Layer 2 was force-pushed rebased only (dff79d8;git range-diffshows0d06483 = dff79d8). Layer 3 was force-pushed rebased plus a commit that editsl3.txt(a4022be). Both force pushes used--force-with-lease=<branch>:<full sha>.Acceptance criteria:
dff79d8) the log showsSKIP: no-changeandSKIP_REASON: no file of this pull request changed since the review of 0d06483869efd706628aecc9dc39d991ff46c931. It ends withclaude-lane-review-complete lane=claude-security-review sha=dff79d8126e953b0ef17687d8d17b89963f0b21c skip=no-change through=0d06483869efd706628aecc9dc39d991ff46c931. Does security review become a default blocking gate, and in what form? #241 then carried the markerclaude-lane-reviewed-head lane=claude-security-review sha=dff79d8126e953b0ef17687d8d17b89963f0b21c, written at 13:53:21Z.a4022be)SKIPis empty. The scope step loggedReviewing the whole pull request: the last reviewed head d12d6408195def365332af1ba0fb2417bd49c92e is not an ancestor of a4022be87dffae45db1f16b23121dc7b6da5380d.The review ran (turns=6,claude-sonnet-5-5) and finished withclaude-lane-review-complete ... sha=a4022be87dffae45db1f16b23121dc7b6da5380d findings=0 inline=0 scope=whole. The new marker on Standing scheduled routines: which maintenance classes, hosted where? #242 is ona4022be.Seen along the way: after a restack, a layer that has an edit gets a whole-PR review, not an incremental one, because the earlier reviewed head is no longer an ancestor. That is correct, but it reviews more than the edit. In every run the security key leg was skipped because of a credit stop recorded in an earlier sandbox run (run 40666897084), and the review ran on the subscription credential (
fallback=credit-stop). Neither affects the criteria.Cleanup: sandbox PRs #240, #241 and #242 are closed unmerged; branches
probe/7669-l1,-l2and-l3are deleted. No PR was opened in this repository.Target labels: add none; remove host-run.