Skip to content

perf(claude-lanes)!: review only what changed, fold the status job, cap timeouts - #653

Merged
kyle-sexton merged 6 commits into
mainfrom
perf/review-lanes-trim
Oct 3, 2026
Merged

kyle-sexton merged 6 commits into
mainfrom
perf/review-lanes-trim

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

No related issue: CI wave 2, review-lane trim approved by the owner on 2026-10-02.

Merge order

  1. This PR (perf(claude-lanes)!: review only what changed, fold the status job, cap timeouts #653), then cut its release tag.
  2. fix(source-control): accept the folded security-review check in the merge gate claude-code-plugins#5995 (babysit merge gate accepts security-review as the security lane's check). It accepts both names, so it can merge any time before step 3.
  3. perf(claude-lanes): pin the review lanes that review only what changed standards#662, re-pinned from this PR's 4d4b9a5 to the release tag's SHA, with the runner-policy contracts for that SHA. Labeled do-not-merge until then.
  4. docs(adr): amend ADR 0038 with the narrowed review cadence claude-code-plugins#5898 (ADR 0038 amendment), after the standards sync lands in claude-code-plugins. Labeled do-not-merge until then.

Summary

The two hosted Claude review lanes run on every push to a ready pull request and held 24.4% (2026-09-28 19:15-19:30 UTC) and 14.6% (2026-09-30 05:15-05:30 UTC) of Linux runner time on claude-code-plugins in the two measured peaks. This change makes each review cover only what changed, removes the second job per lane, and caps the timeouts.

  • Incremental re-review. opened, reopened and ready_for_review review the whole pull request. A later push reviews only the pull request's files that changed since the lane's last completed review. The prompt names those files and the job writes their diff to .claude-lane/incremental.diff.
  • Where the last reviewed head lives. One marker comment per lane on the pull request, written and then edited by the job as github-actions[bot]; the scope step reads only that author's marker. It never evicts (the Actions cache did), and it needs no permission beyond the caller's pull-requests: write, so callers change nothing. Reading the lane's own check runs instead would need checks: read on every caller, and editing this repo's self-callers trips claude-code-action's workflow validation (skipped-validation).
  • Whole-review rule. A push reviews the whole pull request when no earlier review is recorded, the recorded head is not an ancestor of the new head (force push or rebase), 300 or more files changed since, a base-branch merge since then changed a file the pull request also changes (or 300 or more files, too many to check), or the API returns no patch for a changed text file (too large to show). A push that changes no file of the pull request is not reviewed again.
  • Docs-only security skip. The security lane skips a scope whose files all match docs-only-paths (default docs/**/*.md, **/README.md, **/CHANGELOG.md). Agent-instruction files (CLAUDE.md, CLAUDE.local.md, AGENTS.md, GEMINI.md, SKILL.md, copilot-instructions.md, anything under .claude/, skills/, agents/, commands/, rules/, hooks/, instructions/, prompts/) never count as documentation, whatever a caller's list says. The code-review default is empty.
  • One job per lane. The status job is folded into the review job. The security job is named security-review, so with the canonical caller its check is security-review / security-review, the context github-iac's security-review-gate org ruleset names (OrgRulesets.cs:141, integration 15368; the ruleset is disabled). The code-review job is named claude-review-status, keeping review / claude-review-status. Every context named by a live org ruleset (ci-status, from ci-gate) is untouched. A not-needed review stays green and says why.
  • Timeouts. Outside the Claude step, successful jobs on claude-code-plugins (2026-09-29 to 2026-10-01: 1,169 code-review, 1,366 security-review) took p95 17 s, max 82 s; the new scope and record steps took 1 s each here. Code review: step 11 minutes (unchanged), job 15 -> 13. Security review: step 18 -> 14, job 25 -> 16 (the ceiling for every CI job). Each job leaves 38 s beyond step + 82 s.

Fix

  • .github/workflows/claude-review.yml, .github/workflows/claude-security-review.yml: a Scope the review github-script step (fails open to a whole review), review steps gated on its decision, a Record the reviewed head step that creates or edits the lane's marker comment after a completed or not-needed review, and the status script as the job's last step. New inputs incremental-review (default true) and docs-only-paths.
  • .github/scripts/claude-lane-scope.test.cjs (new): wiring, timeouts, the scope and record scripts run against a mocked API for every branch, including a round trip of the marker the record step writes.
  • .github/scripts/claude-lane-status-check.test.cjs: rewritten for the folded step and the job names, plus the not-needed case.
  • README.md: inputs, cadence, status-check section.

Release note (for the tag). BREAKING: <caller job> / review and <caller job> / claude-security-review-status no longer report; <caller job> / security-review now carries the security verdict and goes red when no review happened. Callers need no edit: they pass only runner and the same three permissions.

Verification

The new reusables ran on this PR through its self-callers (./ refs):

Event Head Lane Run Scope Claude step Result
ready_for_review 34d0818 code review 37094814300 whole PR 7m16s success; marker comment created
ready_for_review 34d0818 security review 37094814231 whole PR 5m37s success; marker comment created
synchronize 4d4b9a5 code review 37095629351 "Incremental review of 4 file(s) since 34d0818" 4m03s success; marker edited in place
synchronize 4d4b9a5 security review 37095629363 "Incremental review of 4 file(s) since 34d0818" 3m12s success; marker edited in place
synchronize b994bca code review 37095960821 "Incremental review of 4 file(s) since 4d4b9a5" 1m13s success, no findings
synchronize b994bca security review 37095960822 "Incremental review of 4 file(s) since 4d4b9a5" 1m03s success, no findings

The security check reported as security-review / security-review, the code check as review / claude-review-status. Both marker comments (5965332537, 5965343816) are authored by github-actions[bot], so pull-requests: write covers create and edit.

  • node --test .github/scripts/*.test.cjs: 194 pass, 0 fail (45 in the two claude-lane files). node --test .github/actions/claude-lane-outcome/*.test.cjs: 15 pass.
  • biome ci (fixture config, .github/scripts): clean. actionlint on both reusables and both self-callers: clean. zizmor 1.30.0: the same 2 artipacked medium findings as main on the documented persisted-credentials checkout. markdownlint-cli2 README.md, typos: clean.
  • Review findings on the first run: the docs-only floor and the no-patch fallback were applied in 4d4b9a5; Codex's cancellation P1 was answered on its thread (an if: without a status function gets an implicit success()).

Related

🤖 Generated with Claude Code

…ap timeouts

Both review lanes now scope each review. opened, reopened and
ready_for_review review the whole pull request; a later push reviews only
the pull request's files changed since the lane's last completed review,
whose head is kept in the PR's Actions cache. A rewritten history, 300 or
more changed files, or a base merge that touched a file of the PR forces a
whole review; a push that changes no file of the PR is not reviewed again.
The security lane skips a scope that is all documentation
(docs-only-paths). Both behaviors sit behind inputs whose defaults turn
them on (incremental-review: true; docs-only-paths empty for code review).

Each lane is now one job named after its status check, so
`<caller job> / claude-review-status` and
`<caller job> / claude-security-review-status` keep their names while the
`review / review` and `security-review / security-review` contexts go
away. Timeouts follow measured durations: code review 15 -> 13 minutes
(step 11 -> 12), security review 25 -> 16 (step 18 -> 15).

BREAKING CHANGE: the `<caller job> / review` and
`<caller job> / security-review` check contexts no longer report; the
status-check contexts are unchanged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
kyle-sexton and others added 2 commits October 2, 2026 23:54
…ment

The security-review job is named `security-review` again, so with the
canonical caller its check is `security-review / security-review`, the
context github-iac's security-review-gate org ruleset names (OrgRulesets.cs,
integration 15368). The code-review job keeps `claude-review-status`.

The last reviewed head moves from the Actions cache, which evicts, to one
marker comment per lane that the job writes as github-actions[bot] and edits
after each completed review; the scope step reads only that author's marker.
This needs no permission beyond the caller's `pull-requests: write`.

The code-review step limit is 11 minutes again. The job limit stays 13:
outside the step, 1,169 successful jobs on claude-code-plugins
(2026-09-29 to 2026-10-01) took p95 17 s, max 82 s.

A base merge of 300 or more files now names that reason, and every
statement of the threshold reads "300 or more files".

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 3, 2026 03:55
@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 6m 53s —— View job


Claude code review

  • Gather context (PR body, diff, changed files)
  • Read CLAUDE.md / REVIEW.md for repo-specific guidelines
  • Review .github/workflows/claude-review.yml
  • Review .github/workflows/claude-security-review.yml
  • Review .github/scripts/claude-lane-scope.test.cjs
  • Review .github/scripts/claude-lane-status-check.test.cjs
  • Review README.md changes
  • Post findings

Per REVIEW.md, this repo runs a dedicated security lane (claude-security-review.yml), so this pass is scoped to correctness/maintainability only — no security findings reported here.

The scope/record logic in claude-lane-scope.test.cjs is thorough (branch-by-branch mocked-API coverage of the whole-review rule, the docs-only skip, renames, base merges, the 300-file cap) and it runs the actual embedded github-script source straight out of the YAML rather than a re-implementation, so there's no drift risk between test and production code. I didn't find a code-level correctness bug in the scope/record/status scripts themselves — nice work there.

What I did find is that the PR description doesn't match what actually shipped in a few places that matter given this PR's own "Related" section ties external follow-up work (the standards repo contract, the babysit merge gate) to these specifics:

🔴 Important — description says the head is cached; code stores it in a PR comment

The body's "Incremental re-review" bullet says: "The last reviewed head lives in the PR's Actions cache (no new token permission)." The shipped mechanism is a PR comment written with the job's own token, not the Actions cache — confirmed by the workflow's own security-model comment and by a test that explicitly asserts no actions/cache step exists:

  • # - The last reviewed head is kept in a pull request comment this job writes
    # with its own token, so its author is github-actions[bot]; the lane reads
    # only that author's marker. Any workflow in the repository holding
    # `pull-requests: write` could write one and narrow a later incremental
    # review; same-repository writers already receive this lane's secret, so
    # the lane trusts them.
  • test(`${lane.file}: the reviewed head lives in the lane's PR comment, never the Actions cache`, () => {
    const save = stepNamed(job, "Record the reviewed head");
    assert.match(save.uses, /^actions\/github-script@[0-9a-f]{40}$/u);
    assert.equal(save["continue-on-error"], true);
    assert.equal(save.env.LANE, lane.lane);
    assert.equal(save.env.MARKER_ID, `\${{ steps.scope.outputs.marker-id }}`);
    assert.equal(
    save.env.HEAD_SHA,
    `\${{ github.event.pull_request.head.sha }}`,
    );
    assert.match(save.if, /steps\.scope\.outputs\.record == 'true'/u);
    assert.match(
    save.if,
    /steps\.review-outcome\.outputs\.review-ran == 'true'/u,
    );
    assert.ok(
    job.steps.every(
    (step) => !String(step.uses ?? "").startsWith("actions/cache"),
    ),
    );

This isn't just a wording nit: a PR comment and an Actions cache entry have a different trust/visibility model (who can write one, whether it survives a cache eviction policy, whether it's reviewable in the PR thread). Worth fixing the description before this lands, since it's the kind of claim someone will cite later.

🔴 Important — description says the security context is renamed; code intentionally keeps it

The body says: "The status contexts consumers read (review / claude-review-status, security-review / claude-security-review-status, ...); review / review and security-review / security-review stop reporting." The actual claude-security-review.yml job is still named security-review (unchanged), and its own header comment, the README, and claude-lane-status-check.test.cjs all say this is deliberate, to preserve the security-review / security-review context that the disabled github-iac security-review-gate ruleset names:

  • # One job reviews and reports. It is named `security-review`, so with the
    # canonical caller its check is `security-review / security-review`, the
    # context the github-iac `security-review-gate` org ruleset names. It goes
    # red, naming the cause, when no review happened; a review that is not needed
    # stays green. That ruleset is disabled (github-iac ADR 0011 keeps agentic
    # review advisory); do not make the check required elsewhere.
  • ci-workflows/README.md

    Lines 876 to 881 in 34d0818

    **Status check, red means no review.** Each lane is one job. The code-review
    job is named `claude-review-status`, so its check is
    `<caller job> / claude-review-status`. The security-review job is named
    `security-review`, so with the canonical caller its check is
    `security-review / security-review`, the context the github-iac
    `security-review-gate` org ruleset names. The job's last step goes red, naming

Only the code-review lane's context actually changes to claude-review-status; the security lane's does not. If the linked standards PR (#662) or the babysit merge gate config is written against the body's claimed security-review / claude-security-review-status, it'll be matching a context that never exists.

🟡 Suggestion — "step 11 -> 12" doesn't match the shipped timeout

The Timeouts section says the code-review step changed "11 -> 12", but claude-review.yml's Claude review step timeout is unchanged at 11 minutes (only the job timeout dropped, 15 -> 13):

Not a functional risk — leaving it at 11 just keeps a wider 2-minute buffer than the security lane's 1-minute buffer (job 16 / step 15) — but the stated rule (ceil(max(1.5×p95, 1.1×max)/60) ≈ 12.6 → 13 for the job) implies the step was meant to move to 12 to match that same 1-minute-buffer pattern, and it didn't. Worth squaring the numbers, or explaining why code-review intentionally gets a bigger buffer than security-review.
· branch perf/review-lanes-trim

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 5m 14s —— View job


Claude security review (/review:security-review)

Reviewed only the files this PR changes: both reusable workflows' new scope/cache
logic, the new/updated test files, and the README. GitHub Actions hardening
(trigger types, permission widening, pin format) is zizmor's lane; this review
focuses on the logic/trust-boundary reasoning behind the new incremental-review
and docs-only-skip mechanisms.

IMPORTANT — docs-only-paths can silence the security lane for agent-instruction files, with no code-level floor

.github/workflows/claude-security-review.yml#L113-L124, enforced at #L313-L316

docs-only-paths is a plain workflow_call input. Its default excludes agent
instruction files by convention ("agent-instruction markdown such as SKILL.md,
CLAUDE.md and rules is not in it"), but nothing in the scope script enforces
that exclusion — it's purely a property of the shipped default string. Any
caller of this reusable workflow (this is shared infra for other repos) can set
docs-only-paths to something broader, e.g. **/*.md, and the check at
L313-L316 (scope.every(isDocs)) will then treat a PR that only touches
CLAUDE.md / AGENTS.md / a SKILL.md / a rules file as "documentation" and
skip the security review entirely — core.setOutput("review", "false") — with
the job staying green. That is exactly the protected class this org's own
review:security-review skill calls out under "Instruction-surface deletions"
(a rule a hostile context would otherwise have to fight is removed with no
other mechanism enforcing it), except here the removal is a single caller-side
config value rather than a diff to the rule file itself. claude-lane-scope.test.cjs
only exercises the default value (lines ~526-565); there's no test (and no
runtime guard) preventing a broader override from swallowing instruction files.
README.md's new input table documents the default but carries no caution
against widening it. Concretely: a caller sets
docs-only-paths: "**/*.md" org-wide for speed, then a PR that edits only
CLAUDE.md to strip a guardrail sails through with the security lane reporting
green-and-skipped. Consider hard-excluding known agent-instruction filenames
(CLAUDE.md, AGENTS.md, **/SKILL.md, a rules-directory glob) from the
docs-only match regardless of the caller's docs-only-paths value, rather than
relying on documentation discipline alone.

IMPORTANT — incremental scope falls back to an advisory instruction, not a hard full-review, when the API omits a patch

.github/workflows/claude-security-review.yml#L258-L270 (same pattern in claude-review.yml#L263-L275)

writeDiff substitutes "(no patch from the API; read the file at the head commit)" when change.patch is absent — which the GitHub compare API does for
large or binary diffs. The code already anticipates this, but the only
mitigation is a text instruction appended to the model's prompt asking it to go
read the file itself; the scope decision (review: "true", incremental) and the
compressed-review framing ("Review only what changed... use gh pr diff only
for context") stand regardless of whether the patch was actually available. A
contributor on an already-reviewed PR can land a large or binary-looking diff
in one file on a later push specifically to trigger patch omission for that
file; nothing forces a fallback to a whole-PR review when that happens, so
coverage of that file depends entirely on the model's best-effort compliance
under the same turn budget that was just cut for being "only an incremental
diff." Consider treating a missing patch on any file in scope as a full
trigger (same bucket as the existing 300-file cap), rather than downgrading to
an instruction the model can skip under time/turn pressure.

No other security-relevant findings in this diff. The marker-comment trust
model (any same-repo workflow with pull-requests: write can write/forge the
lane's own github-actions[bot] marker) is already called out and accepted in
this PR's own SECURITY MODEL comments, and the ancestor/force-push/rebase and
base-merge-overlap checks in sinceLastReview look correctly conservative
(they fail open to a whole review).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 34d08182ce

ℹ️ 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".

Comment thread .github/workflows/claude-review.yml
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-03T04:00:04.810097Z 34d0818 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

claude-security-review has reviewed this pull request through b994bca; a later push is reviewed from there.

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

claude-review has reviewed this pull request through b994bca; a later push is reviewed from there.

…hes whole

Two review findings on the first live run of these lanes:

- docs-only-paths is a caller input, so a wider list such as `**/*.md`
  could have skipped a security review of CLAUDE.md, AGENTS.md, a SKILL.md
  or a rules file. Those names, and anything under .claude/, skills/,
  agents/, commands/, rules/, hooks/, instructions/ or prompts/, now never
  count as documentation.
- The compare API drops the patch of a large text change. An incremental
  scope holding such a file now falls back to a whole review instead of
  asking the model to read the file; a binary file or a pure rename, which
  has no patch and no changed lines, stays incremental.

The security-review step drops from 15 to 14 minutes so the 16-minute job
holds the measured 82 s maximum outside the step plus the scope and record
steps, which took 1 s each on this pull request's first run.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 49s —— View job


Claude security review (/review:security-review)

Scoped to what changed since 34d0818 in .github/scripts/claude-lane-scope.test.cjs, .github/workflows/claude-review.yml, .github/workflows/claude-security-review.yml, README.md (read from .claude-lane/incremental.diff; gh pr diff used only for whole-PR context). This commit is a direct response to the two IMPORTANT findings from the prior pass on 34d0818:

Docs-only bypass of agent-instruction files — now closed. A hard-coded INSTRUCTIONS regex excludes CLAUDE.md, CLAUDE.local.md, AGENTS.md, GEMINI.md, SKILL.md, copilot-instructions.md, and anything under .claude/, skills/, agents/, commands/, rules/, hooks/, instructions/, prompts/ from ever matching docs-only-paths, regardless of what a caller sets that input to. Applied identically in both reusables:

  • // Agent instructions are never documentation, whatever docs-only-paths says.
    const INSTRUCTIONS =
    /(?:^|\/)(?:(?:CLAUDE|CLAUDE\.local|AGENTS|GEMINI|SKILL|copilot-instructions)\.md$|(?:\.claude|skills|agents|commands|rules|hooks|instructions|prompts)\/)/u;
    const names = (file) => [file.filename, file.previous_filename].filter(Boolean);
    const isDocs = (file) =>
    names(file).every((name) => !INSTRUCTIONS.test(name) && docsOnly.some((re) => re.test(name)));
  • // Agent instructions are never documentation, whatever docs-only-paths says.
    const INSTRUCTIONS =
    /(?:^|\/)(?:(?:CLAUDE|CLAUDE\.local|AGENTS|GEMINI|SKILL|copilot-instructions)\.md$|(?:\.claude|skills|agents|commands|rules|hooks|instructions|prompts)\/)/u;
    const names = (file) => [file.filename, file.previous_filename].filter(Boolean);
    const isDocs = (file) =>
    names(file).every((name) => !INSTRUCTIONS.test(name) && docsOnly.some((re) => re.test(name)));

I checked the regex for the substring-vs-segment trap this kind of fix usually has (e.g. .claude-lane/ or myskills/ false-matching skills/): it doesn't, because the directory alternatives require a / immediately before and after the keyword, so only an actual path segment matches. The new test (a caller's wider docs-only-paths never skips agent instructions) exercises docs-only-paths: "**/*.md" against CLAUDE.md, sub/AGENTS.md, a SKILL.md under skills/, a non-SKILL file still under a skills/ directory, .claude/rules/*.md, agents/, commands/, and copilot-instructions.md, and confirms each still forces a review. This closes the finding: the protection is now a code-level floor, not default-value documentation discipline.

Missing-patch fallback — now forces a whole review for the case that mattered. sinceLastReview now treats a changed file with patch === undefined && changes > 0 (the API drops the patch for a large text diff) as a full trigger, same bucket as the existing 300-file cap:

  • // The API drops the patch of a large text change; only a whole review covers it.
    const unshown = files.find((file) => {
    const change = patches.get(file.filename) ?? patches.get(file.previous_filename);
    return change.patch === undefined && change.changes > 0;
    });
    if (unshown) return { full: `the API returned no patch for ${unshown.filename}` };
  • // The API drops the patch of a large text change; only a whole review covers it.
    const unshown = files.find((file) => {
    const change = patches.get(file.filename) ?? patches.get(file.previous_filename);
    return change.patch === undefined && change.changes > 0;
    });
    if (unshown) return { full: `the API returned no patch for ${unshown.filename}` };

This closes the main exploit path from the prior finding: a large text-file change can no longer silently stay incremental on the model's best-effort compliance. One narrower case remains by design, not by oversight: a binary (or pure-rename) file always reports changes: 0 from the API, so it still falls through to the old "no patch from the API; read the file at the head commit" instruction rather than a forced full review — covered explicitly by the new a binary or pure-rename change without a patch stays incremental test. That's a reasonable scope line (an LLM can't meaningfully text-review binary content either way, whole-review or incremental), not a reopening of the original finding, so I'm not re-raising it.

I did not find a new security issue in this incremental diff. No other standing instruction is deleted, narrowed, or softened here — the change is additive (closing a gap), so the instruction-exception-register lens doesn't apply; for the record, docs/conventions/instruction-exception-register/README.md is not present in this checkout, so the protected-class list itself was not consulted beyond the register pointer's own wording.

GitHub Actions trigger/permission/pin hardening is zizmor's lane and out of scope here; Codex's open P1 (cancellation guard on the Claude review/Claude security review step) is also unchanged by this diff and outside this incremental scope.
· branch perf/review-lanes-trim

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 31s —— View job


Claude code review

  • Gather context (PR body, diff, changed files, incremental scope)
  • Read the incremental diff (.claude-lane/incremental.diff) since 34d0818
  • Review .github/scripts/claude-lane-scope.test.cjs changes
  • Review .github/workflows/claude-review.yml changes
  • Review .github/workflows/claude-security-review.yml changes
  • Review README.md changes
  • Post findings

This push (34d0818 → 4d4b9a5) directly implements the two Important security-lane findings from the prior review round: a hard-coded INSTRUCTIONS regex that keeps agent-instruction files out of docs-only-paths regardless of caller config, and a new "no patch returned" check that forces a whole review instead of degrading to an advisory prompt instruction. Both are backed by new, well-targeted tests (a caller's wider docs-only-paths never skips agent instructions, a changed file the API returns no patch for forces a whole review, a binary or pure-rename change without a patch stays incremental). I traced the sinceLastReview/isDocs logic in both workflows against the new tests and didn't find a correctness bug — the changes > 0 guard correctly keeps binary/zero-diff renames on the incremental path while still forcing a full review for an actually-large text change with no patch, and the regex's backtracking correctly handles CLAUDE.local.md despite the CLAUDE|CLAUDE\.local alternation ordering.

Per REVIEW.md, security-shaped findings are scoped to the dedicated security lane, so this pass is correctness/maintainability only.

Two non-blocking suggestions posted inline:

  • 🟡 claude-lane-scope.test.cjs#L610-619 — the new INSTRUCTIONS regex also excludes GEMINI.md and CLAUDE.local.md, but the test loop doesn't exercise either, so a future edit narrowing the regex to match the documented set wouldn't be caught here.
  • 🟡 README.md#L857 — same gap in the prose: the doc table (and the matching comment blocks in both workflow files, claude-review.yml#L117-119 / claude-security-review.yml#L118-120) list the exclusions as CLAUDE.md/AGENTS.md/SKILL.md/directories, omitting GEMINI.md and CLAUDE.local.md that the regex also covers. Safe direction (code protects more than docs claim), but worth squaring so a later "simplify to match the docs" pass doesn't drop real coverage.
    · branch perf/review-lanes-trim

Comment thread .github/scripts/claude-lane-scope.test.cjs
Comment thread README.md Outdated
…gnores

The instruction floor also covers CLAUDE.local.md, GEMINI.md and
copilot-instructions.md; the docs-only-paths descriptions and the README
now name them, and the test covers the two it missed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 39s —— View job


Claude security review (/review:security-review)

Scoped to what changed since 4d4b9a5 in .github/scripts/claude-lane-scope.test.cjs, .github/workflows/claude-review.yml, .github/workflows/claude-security-review.yml, README.md (read from .claude-lane/incremental.diff).

This push is a documentation/test-only follow-up to the two 🟡 suggestions from the prior round (both about the INSTRUCTIONS regex's coverage not being fully exercised/documented):

  • claude-lane-scope.test.cjs adds "CLAUDE.local.md" and "GEMINI.md" to the a caller's wider docs-only-paths never skips agent instructions test loop.
  • Both workflow files' docs-only-paths input description and README.md's input table are reworded to list CLAUDE.local.md, GEMINI.md, and copilot-instructions.md alongside CLAUDE.md/AGENTS.md/SKILL.md.

No code logic changed — the INSTRUCTIONS regex itself is untouched in this diff. I confirmed it against the live file to be sure docs and tests now actually match the enforced behavior rather than drifting further apart:

/(?:^|\/)(?:(?:CLAUDE|CLAUDE\.local|AGENTS|GEMINI|SKILL|copilot-instructions)\.md$|(?:\.claude|skills|agents|commands|rules|hooks|instructions|prompts)\/)/u;

/(?:^|\/)(?:(?:CLAUDE|CLAUDE\.local|AGENTS|GEMINI|SKILL|copilot-instructions)\.md$|(?:\.claude|skills|agents|commands|rules|hooks|instructions|prompts)\/)/u

This already included CLAUDE.local, GEMINI, and copilot-instructions before this push (from 4d4b9a5, the commit that closed the original docs-only bypass finding) — this push only brings the test and the prose in line with what the regex already enforced. No security-relevant change and no new finding in this diff.
· branch perf/review-lanes-trim

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 50s —— View job


Claude code review

  • Gather context (PR body, diff, incremental scope)
  • Read .claude-lane/incremental.diff (scope: changes since 4d4b9a5)
  • Review .github/scripts/claude-lane-scope.test.cjs change
  • Review .github/workflows/claude-review.yml / claude-security-review.yml changes
  • Review README.md change
  • Post findings

This push (4d4b9a5 → b994bca) closes both 🟡 suggestions from the prior review round on this PR: the INSTRUCTIONS regex in both reusable workflows already excluded GEMINI.md and CLAUDE.local.md from docs-only-paths matching, but neither was exercised by a test or mentioned in the docs.

  • Test coverage closed. "CLAUDE.local.md" and "GEMINI.md" are now added to the loop in a caller's wider docs-only-paths never skips agent instructions. I checked both against the live INSTRUCTIONS regex in claude-review.yml (#L236-L237): GEMINI\.md$ matches GEMINI.md directly, and CLAUDE.local.md matches via the CLAUDE\.local alternative after the engine backtracks off the CLAUDE branch (same backtracking behavior confirmed correct in the prior round). Both now fail the test if a future edit narrows the regex.
  • Docs squared with code. The agent-instruction exclusion list in both workflows' docs-only-paths input description (claude-review.yml#L117-L120, claude-security-review.yml#L118-L121) and in README.md#L857 now lists CLAUDE.local.md, GEMINI.md, and copilot-instructions.md alongside the existing entries — the prose now enumerates exactly what the regex enforces, no more and no less.

No correctness or maintainability issues found in this incremental diff — it's a docs/test-completeness fix with no logic change, and it's internally consistent (regex, test, and prose now all agree). Nothing further to report.
· branch perf/review-lanes-trim

@kyle-sexton
kyle-sexton enabled auto-merge (squash) October 3, 2026 04:41
@kyle-sexton
kyle-sexton merged commit 0a99a32 into main Oct 3, 2026
69 checks passed
@kyle-sexton
kyle-sexton deleted the perf/review-lanes-trim branch October 3, 2026 04:51
kyle-sexton added a commit to melodic-software/claude-code-plugins that referenced this pull request Oct 3, 2026
…erge gate (#5995)

No related issue: CI wave 2, review-lane trim approved by the owner on
2026-10-02.

## Merge order

1. melodic-software/ci-workflows#653, then its release tag.
2. **This PR.** It accepts both the old and the new security check name,
so it can merge any time before step 3; it must land before step 3 syncs
the new reusable into any repository.
3. melodic-software/standards#662, re-pinned to the release tag's SHA
with its runner-policy contracts.
4. #5898 (ADR 0038 amendment), after
the standards sync lands here.

## Summary

ci-workflows#653 folds the security lane's status job into its review
job and names that job `security-review`, so the lane's check is
`security-review / security-review`, the context github-iac's
`security-review-gate` ruleset names (`OrgRulesets.cs:141`).
`security-review / claude-security-review-status` stops reporting. The
babysit merge gate's `--auto` waits for `claude-security-review-status`
by job segment, so on every synced repository it would hold forever.

- `AI_REVIEW_CHECKS` maps each lane to the job segments its check may
carry; the security lane accepts `claude-security-review-status` or
`security-review`.
- Every matching check must still succeed, so a caller on an older pin,
which reports a green `security-review` review job beside its
`claude-security-review-status` verdict, still holds when the verdict is
red.

## Fix

- `plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py`:
`AI_REVIEW_CHECKS` and the hold loop.
-
`plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge.py`:
the folded check arms; an old pin's red status job holds beside a green
review job.
- `plugins/source-control/skills/babysit-prs/reference/safety.md`: names
both checks.
- `plugins/source-control`: version 0.76.0 -> 0.76.1 and its CHANGELOG
entry.

## Verification

- `python -m pytest
plugins/source-control/skills/babysit-prs/scripts/tests -q`: 797 passed,
354 subtests passed.
- `ruff check` on the scripts: clean; `ruff format --check` on the two
edited Python files: formatted.
- `markdownlint-cli2` on the CHANGELOG and `safety.md`: 0 issues.

## Related

- melodic-software/ci-workflows#653, melodic-software/standards#662,
#5898.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit to melodic-software/standards that referenced this pull request Oct 3, 2026
#662)

No related issue: CI wave 2, review-lane trim approved by the owner on
2026-10-02.

## Merge order

Labeled `do-not-merge` and kept a draft: the pin is an unreleased
pull-request head.

1. melodic-software/ci-workflows#653, then cut its release tag.
2. melodic-software/claude-code-plugins#5995 (the babysit merge gate
accepts `security-review / security-review`); it must land before this
PR's sync reaches any repository.
3. **This PR**, re-pinned from the ci-workflows#653 head `4d4b9a5` to
the release tag's SHA, with the runner-policy contracts keyed to that
SHA (steps below). Remove `do-not-merge` only then.
4. melodic-software/claude-code-plugins#5898 (ADR 0038 amendment), after
this PR's sync lands in claude-code-plugins.

Re-pin steps:

1. In the four components, replace
`4d4b9a5a2247a8aa2ca34f88b8df0f9785b39305` with the tag SHA and the pin
comment with `# <tag>`. Do this by hand: after a squash merge `4d4b9a5`
is not an ancestor of the tag, so `repin-callers.sh apply` reads the pin
as ahead of the release and refuses. For the same reason this PR must
not merge with the `4d4b9a5` pin, or the daily `claude-lanes-repin` lane
would refuse every later release.
2. In `components/runner-policy/policy.json`, rename the two
`@4d4b9a5a2247a8aa2ca34f88b8df0f9785b39305` contract keys
(`claude-review.yml`, `claude-security-review.yml`) to `@<tag-sha>` and
keep their bodies. The repin lockstep will not write them, because the
reusables' `workflow_call` inputs and job set changed. The tag also
needs a `standards-sync.yml@<tag-sha>` contract if `sync.yml` is
repinned in the same pass.

## Summary

Moves the four Claude lane caller components (fleet and hosted, code and
security) onto the ci-workflows review-lane trim,
melodic-software/ci-workflows#653, and registers its runner-policy
contracts. Callers keep the same inputs (`runner` only), secret and
permissions; the new behavior lives in the reusables:

- `opened`, `reopened` and `ready_for_review` review the whole PR; a
later push reviews only the PR's files changed since the lane's last
completed review, whose head each lane keeps in one PR comment it writes
as `github-actions[bot]` (covered by the existing `pull-requests:
write`). A push that changed none of the PR's files is not reviewed
again. Drafts stay skipped.
- The security lane skips a review whose files in scope are all
documentation (`docs/**/*.md`, `**/README.md`, `**/CHANGELOG.md`), with
a green check; agent-instruction files never count as documentation.
- One job per lane. `review / claude-review-status` keeps its name. The
security check is `security-review / security-review`, the context the
github-iac `security-review-gate` org ruleset names
(`OrgRulesets.cs:141`, integration 15368, enforcement `disabled`);
`security-review / claude-security-review-status` and `review / review`
stop reporting. Every context a live org ruleset requires is `ci-status`
(`ci-gate`), which this does not touch.
- Timeouts: code review step 11 min, job 13 (from 15); security review
step 14 (from 18), job 16 (from 25).
- Contracts: both reusables' contracts at the new pin allow
`incremental-review` beside `runner`, so a caller can set
`incremental-review: false` (ADR 0038's revisit trigger) without a
contract edit. Secrets and caller permissions are the v0.30.1 values.

## Fix

- `components/claude-lanes/claude-review.yml`,
`components/claude-lanes-hosted/claude-review.yml`: pin, the trigger
comment describes the narrowed re-review, and the `pull-requests: write`
note names the marker comment.
- `components/claude-lanes/claude-security-review.yml`,
`components/claude-lanes-hosted/claude-security-review.yml`: the same,
and the path-gating comment names the babysit merge gate's
`security-review / security-review` check.
- `components/runner-policy/policy.json`: contracts for both reusables
at the new pin, `allowedInputs: ["runner", "incremental-review"]`.

## Verification

- `npm run test:runner-policy`: 266 pass, 0 fail. `npm run
lint:runner-policy`: passed.
- `node --test components/claude-lanes/repin-policy-lockstep.test.mjs`:
11 pass; `bash harness/shell/run-tests.sh
components/claude-lanes/repin-callers.test.sh`: pass.
- `actionlint` on the four components: clean. `zizmor --offline` on the
four components: no findings. `typos`: clean.
- `components/claude-lanes/claude-lanes.test.sh` materializes every
managed target under `/tmp` and cannot run on the Windows worktree; this
PR's Linux CI runs it.
- The reusables at `4d4b9a5` ran on GitHub on ci-workflows#653 (run ids
in that PR's verification section).

## Related

- melodic-software/ci-workflows#653: the reusable change this pins.
- melodic-software/claude-code-plugins#5995: babysit merge gate.
- melodic-software/claude-code-plugins#5898: ADR amendment.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit to melodic-software/claude-code-plugins that referenced this pull request Oct 3, 2026
No related issue: CI wave 2, review-lane trim approved by the owner on
2026-10-02.

## Merge order

Labeled `do-not-merge` until step 3's sync lands here.

1. melodic-software/ci-workflows#653, then cut its release tag.
2. #5995 (babysit merge gate accepts
`security-review / security-review`). It accepts both names, so it can
merge any time before step 3.
3. melodic-software/standards#662, re-pinned from the ci-workflows#653
head to the release tag's SHA, with the runner-policy contracts for that
SHA; then its sync PR into this repository.
4. **This PR**, after that sync lands.

## Summary

Amends ADR 0038 with an addendum that records the narrowed Claude review
cadence and the owner's 2026-10-02 approval, with the measured cost
behind it.

- **Peak shares, paired with their windows:** 24.4% of Linux
runner-seconds on 2026-09-28 19:15-19:30 UTC (code 19.4%, security 5.0%)
and 14.6% on 2026-09-30 05:15-05:30 UTC (code 8.0%, security 6.6%).
- **Review volume, recounted by script** over 2026-09-29 00:00 to
2026-10-02 00:00 UTC (method below): 1,271 code reviews (423.7 a day,
3.06 per PR branch) and 1,464 security reviews (488.0 a day, 3.49 per PR
branch).
- Decision 2 narrowed: drafts skipped; open, reopen and ready review the
whole PR; a later push reviews only the PR's files changed since the
last completed review, whose head each lane keeps in its own PR comment;
whole-review fallbacks include "300 or more files changed since".
- Documentation-only scopes skip the security lane; agent-instruction
markdown is never documentation.
- Decision 4 narrowed: one job per lane; `review / claude-review-status`
keeps its name, and the security check is `security-review /
security-review`, the context github-iac's `security-review-gate`
ruleset names. Decision 5 holds.
- Timeouts: code-review step 11 minutes (unchanged), job 13;
security-review step 14, job 16. Outside the Claude step a job took p95
17 s, max 82 s.
- Revisit trigger: set `incremental-review: false` on both standards
caller components; standards#662's runner-policy contracts allow that
input.

## Fix

`docs/adr/0038-restore-the-claude-review-lanes-on-every-push.md`: the
`## Addendum (2026-10-02)` section, the repository's existing amendment
form; status stays accepted.

## Verification

- `markdownlint-cli2` on the ADR: 0 issues. `typos`: clean. No em
dashes.
- `bash scripts/check-adr-numbers.sh`: every ADR number unique or
baselined.
- Review volume and overhead: `python review-rates.py
melodic-software/claude-code-plugins 2026-09-29T00:00:00Z
2026-10-02T00:00:00Z` (below). It lists runs of each lane's two workflow
files with `status=success` and `status=failure`, one hour per query (an
hour that reaches 1,000 results is split again), reads every page,
asserts each slice's count equals the API's `total_count`, and for each
successful run subtracts the Claude step's duration from its review
job's.

<details><summary>review-rates.py</summary>

```python
"""Review-lane run rates and non-review job overhead for one repository and window.

Counts workflow runs of each Claude review workflow whose conclusion is success
or failure, created inside [START, END). The runs API caps any filtered query at
1,000 results, so the window is queried one hour at a time and an hour that
reaches the cap is split further; every page of every slice is read.

Overhead: for each successful run of each lane, the review job's duration minus
its Claude step's duration ("Claude review" or "Claude security review"): every
other step of the job plus the gaps between steps.

Usage: python review-rates.py OWNER/REPO START END   (ISO-8601 UTC instants)
"""

import json
import math
import subprocess
import sys
from concurrent.futures import ThreadPoolExecutor
from datetime import datetime, timedelta

REPO, START, END = sys.argv[1], sys.argv[2], sys.argv[3]
WORKFLOWS = {
    "code": ["claude-review.yml", "claude-review-hosted.yml"],
    "security": ["claude-security-review.yml", "claude-security-review-hosted.yml"],
}
CONCLUSIONS = ["success", "failure"]


def gh(path):
    out = subprocess.run(
        ["gh", "api", "-H", "Accept: application/vnd.github+json", path],
        capture_output=True, text=True, check=True,
    ).stdout
    return json.loads(out)


def iso(t):
    return t.strftime("%Y-%m-%dT%H:%M:%SZ")


def runs_in(workflow, conclusion, lo, hi):
    """Every run in [lo, hi), splitting the slice while it reaches the cap."""
    created = f"{iso(lo)}..{iso(hi - timedelta(seconds=1))}"
    base = (f"repos/{REPO}/actions/workflows/{workflow}/runs"
            f"?status={conclusion}&created={created}&per_page=100")
    first = gh(f"{base}&page=1")
    total = first["total_count"]
    if total >= 1000:
        mid = lo + (hi - lo) / 2
        return runs_in(workflow, conclusion, lo, mid) + runs_in(workflow, conclusion, mid, hi)
    runs = list(first["workflow_runs"])
    for page in range(2, math.ceil(total / 100) + 1):
        runs += gh(f"{base}&page={page}")["workflow_runs"]
    assert len(runs) == total, (workflow, conclusion, created, len(runs), total)
    return runs


def parse(ts):
    return datetime.fromisoformat(ts.replace("Z", "+00:00"))


start, end = parse(START), parse(END)
days = (end - start) / timedelta(days=1)
hours = []
t = start
while t < end:
    hours.append((t, min(t + timedelta(hours=1), end)))
    t += timedelta(hours=1)

report = {"repo": REPO, "window": [START, END], "days": days, "lanes": {}}
successes = {"code": [], "security": []}
STEP = {"code": "Claude review", "security": "Claude security review"}
with ThreadPoolExecutor(max_workers=8) as pool:
    for lane, files in WORKFLOWS.items():
        lane_runs = []
        per_file = {}
        for workflow in files:
            for conclusion in CONCLUSIONS:
                slices = pool.map(lambda h: runs_in(workflow, conclusion, *h), hours)
                found = [run for chunk in slices for run in chunk]
                per_file[f"{workflow}:{conclusion}"] = len(found)
                lane_runs += found
                if conclusion == "success":
                    successes[lane] += found
        ids = {run["id"] for run in lane_runs}
        assert len(ids) == len(lane_runs), "a run was counted twice"
        branches = {run["head_branch"] for run in lane_runs}
        report["lanes"][lane] = {
            "runs": len(lane_runs),
            "per_day": round(len(lane_runs) / days, 1),
            "branches": len(branches),
            "runs_per_branch": round(len(lane_runs) / len(branches), 2),
            "by_file_and_conclusion": per_file,
            "by_event": {
                event: sum(1 for run in lane_runs if run["event"] == event)
                for event in sorted({run["event"] for run in lane_runs})
            },
        }

    def overhead(lane, run):
        jobs = gh(f"repos/{REPO}/actions/runs/{run['id']}/jobs?per_page=100")["jobs"]
        for job in jobs:
            step = next((s for s in job["steps"] if s["name"] == STEP[lane]), None)
            if not step or step["conclusion"] != "success" or not job["completed_at"]:
                continue
            job_s = (parse(job["completed_at"]) - parse(job["started_at"])).total_seconds()
            step_s = (parse(step["completed_at"]) - parse(step["started_at"])).total_seconds()
            return job_s - step_s
        return None

    gaps = {
        lane: sorted(g for g in pool.map(lambda r, lane=lane: overhead(lane, r), runs) if g is not None)
        for lane, runs in successes.items()
    }


def pct(values, p):
    return values[min(len(values) - 1, math.ceil(p / 100 * len(values)) - 1)]


for lane, values in gaps.items():
    report["lanes"][lane]["non_review_overhead_s"] = {
        "jobs": len(values),
        "p50": pct(values, 50),
        "p95": pct(values, 95),
        "max": values[-1],
    }
print(json.dumps(report, indent=2))
```

</details>

## Related

- melodic-software/ci-workflows#653: the reusable change.
- melodic-software/standards#662: the caller components and
runner-policy contracts.
- #5995: the babysit merge gate.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant