Skip to content

ci: re-pin claude lane callers to v0.9.1, adopt paths-file, serialize the review lane - #1766

Merged
kyle-sexton merged 3 commits into
mainfrom
chore/repin-claude-lanes-v0.9.1
Jul 30, 2026
Merged

ci: re-pin claude lane callers to v0.9.1, adopt paths-file, serialize the review lane#1766
kyle-sexton merged 3 commits into
mainfrom
chore/repin-claude-lanes-v0.9.1

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Re-pins both locally-owned Claude lane callers from two divergent stale pins (review e295107, security 66073e5) to ci-workflows v0.9.1 (c136b27f404dd32ce3873f39a6f3443891d1c16e # v0.9.1), which makes both lanes reachable by the org kill-switch variables (CLAUDE_LANES_DISABLED etc.) for the first time. Three commits:

  • Re-pin both callers; delete the explicit skip-actors lists so the reusable's upstream-reviewed 4-actor default applies (widens the skip set by claude[bot] + melodic-ai[bot] — ADR 0002 re-deliberation filed as a follow-up issue).
  • Migrate the security caller's inline paths to paths-file: .github/claude-security-paths — required by the managed runner-policy contract at v0.9.1 (allowedInputs: ["runner","paths-file"]); the file pre-existed on main (ci: commit security-review path scope to .github/claude-security-paths #1701) and is order- and set-identical (26 patterns), so review scope is unchanged.
  • Add the managed component's job-level serializer to the review caller (group: claude-review-${{ github.repository }}, queue: max) — this repo generates 52% of fleet lane volume and was the only consumer without it. Security caller concurrency untouched (per-PR group, correct for a required check).

Known: the security-review / security-review required check reports green on THIS PR without running a review (workflow-touching PRs cannot be reviewed by their own new caller; the action exits 0 on refusal). The diff was independently verified instead — fresh-context verifier, 9/9 criteria PASS, including control tests proving the runner-policy checker reads both files, byte-identical pin lines, and a no-deadlock analysis of the concurrency groups.

Test plan

  • Runner-policy checker: exit 0 on the branch (base also 0; interim paths-passing state correctly failed it — the contract works).
  • actionlint clean under the repo's managed suppression (which is load-bearing for queue: maxconcurrency: Add support for queue key rhysd/actionlint#654).
  • paths-file equivalence: 26/26 patterns, order-identical, set-identical vs the removed inline list.
  • Post-merge: next code-touching PR should produce an actual security run at the new pin — tracked as Phase 3 closure evidence in ci-workflows PLAN.md.

Related

No linked issue: this PR closes no GitHub issue. Related: #1767 (ADR 0002 re-deliberation follow-up, opened by this change), #1701 (committed the paths file this PR activates), ci-workflows docs/topics/claude-review-lanes/PLAN.md Phase 3 (kill-switch reachability + closure evidence).

kyle-sexton and others added 3 commits July 29, 2026 20:43
Both Claude lane callers sat on separate stale pins (review on e295107,
security on 66073e5) with no sync path to refresh them — these callers are
locally-owned, so no standards sync PR ever reaches them.

Drop the explicit skip-actors from both callers so the reusable's v0.9.1
default applies. That default now carries the self-trigger ban list
(dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot]),
a superset of what was passed here, so restating it downstream only risks
drifting from upstream.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reviewed runner-policy contract for claude-security-review.yml at
ci-workflows v0.9.1 declares allowedInputs ["runner", "paths-file"] — the
inline `paths` input the caller still passed is absent from it, so the
managed runner-policy lane failed the branch and would have reddened the
required ci-status aggregate.

`.github/claude-security-paths` already carries this exact pattern set
(#1701 committed it ahead of the pin bump that enables `paths-file`) and
nothing referenced it until now. Point the caller at it.

Job-level relevance gating is unchanged: `paths-file` is an input to the
reusable, not a workflow-level path filter, so an out-of-scope PR still
yields a name-stable skipped check rather than a Pending required one.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This caller was the only consumer one missing the job-level queue group the
managed standards component carries, so parallel PRs contended for the shared
Claude seat here instead of queueing for it — and this repo generates the bulk
of fleet lane volume.

Copy the component's job-level block verbatim. The group is repo-wide and
job-scoped, deliberately distinct from the reusable's inner group (keyed per PR
and head SHA at v0.9.1), so the caller cannot deadlock against its own call.

Only the review lane gets this. The security caller's per-PR group with
cancel-in-progress: false is correct for a required check and is untouched.

`concurrency.queue` is GA and honored at job level; actionlint 1.7.12 rejects
it (rhysd/actionlint#654) and the repo's .github/actionlint.yaml already
carries the scoped, approved suppression.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

@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: f3e5d6db08

ℹ️ 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-security-review.yml
kyle-sexton added a commit that referenced this pull request Jul 30, 2026
…#1773)

No linked issue

## Summary

Every first-attempt `pr-issue-linkage` CI failure in this repo is an
authoring-time knowledge gap: the agent writing the PR body has no way
to know the contract until CI rejects it one full round trip later
(#1766 showed the pattern today — red at creation, green after a body
edit). This PR makes the contract known and enforced at authoring time,
on every surface a PR body can be written through, without weakening the
required check itself.

## Fix

Three layers, repo-level and plugin-level:

- **CLAUDE.md** documents the body contract (closing keyword or literal
`No linked issue`, plus a non-empty `## Related` section, validated
after HTML-comment stripping) so agents write a passing body on the
first try.
- **Checked-in MCP gate** (`.claude/hooks/pr-linkage-mcp-gate.sh`, wired
in `.claude/settings.json`): a PreToolUse hook that blocks a GitHub-MCP
`create_pull_request`/`update_pull_request` whose body would fail the
check — the surface cloud sessions use — with the exact missing lines
named in the block message. Loads in any session that opens this repo,
no plugin required.
- **source-control plugin 0.41.0**: the same gate ships as
`pr-linkage-mcp-gate` for every marketplace consumer, sibling to the
existing `pr-body-linkage-gate` (`gh` surface); kill switch
`pr_linkage_mcp_gate_enabled` (default true). The validator core
(comment stripping, keyword/`## Related` judging, verdict wording) is
extracted to one sourced lib, `pr-linkage-validator.sh`, consumed by
both plugin hooks and the checked-in repo hook, so a drift fix against
the upstream ci-workflows validator lands on every surface atomically.

The merge with `main` also hand-reconciles `.claude/settings.json` where
git's textual merge of this branch and #1763 produced duplicate
`hooks`/`extraKnownMarketplaces`/`enabledPlugins` keys (JSON last-wins
would have silently dropped the SessionStart hook and the 9-plugin
roster): the file now carries both hooks and #1763's marketplace source
and plugin set.

## Verification

- `plugins/source-control/hooks/pr-linkage-mcp-gate.test.sh`: 17/17
(validator shapes, create-without-body blocks, update-without-body
allows, owner/repo scope guard, kill switch).
- `plugins/source-control/hooks/pr-body-linkage-gate.test.sh`: 135/135 —
sibling regression across the shared-lib extraction.
- `.claude/hooks/pr-linkage-mcp-gate.test.sh`: 16/16 against the
checked-in hook sourcing the lib.
- `shellcheck` (repo rcfile semantics) and `shfmt` clean on all four
shell files; `check-changelog-parity.sh --check-bump origin/main`,
`sync-hook-utils.sh --check`, and `check-manifest-duplicate-keys.py` all
pass.
- Hook contracts (PreToolUse MCP matchers, plugin `hooks/hooks.json`,
`userConfig` → `CLAUDE_PLUGIN_OPTION_*` mapping) verified against the
current hooks and plugins-reference docs fetched this session, per the
fresh-docs mandate.

## Related

- Refs #1763 — the cloud-session provisioning PR whose
`.claude/settings.json` this branch merges with and reconciles.
- Refs #1766 — today's observed instance of the first-attempt
`pr-issue-linkage` failure this PR prevents.

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

---
_Generated by [Claude
Code](https://claude.ai/code/session_01KJdyaSTPhgkjYwJnVnhdV4)_

---------

Co-authored-by: Claude <noreply@anthropic.com>
@kyle-sexton
kyle-sexton merged commit 9ed2956 into main Jul 30, 2026
32 of 33 checks passed
@kyle-sexton
kyle-sexton deleted the chore/repin-claude-lanes-v0.9.1 branch July 30, 2026 19:03
kyle-sexton added a commit that referenced this pull request Aug 3, 2026
…decision

Cold-read review falsified three claims the first pass asserted without
verifying against this repo: `secret-scan / gitleaks` is not a check context
(gitleaks is a step in ci.yml's hygiene job, gated transitively via
ci-status), GitGuardian runs but is in no ruleset so it cannot gate, and
human approval is not required at all (base ruleset sets
required_approving_review_count: 0 and the repo has one collaborator).

That last one was load-bearing twice: it was offered as a compensating
control AND as half of Branch A's rationale. Both are withdrawn and the
decision point now states an affirmative case for each branch, names the
silence default, and discloses that Branch A's zero-edit cost is an artifact
of this draft restoring four rather than two.

Also record a real gap the review surfaced: the action refuses to run when
the caller file differs from the default branch, its step still reports
success, so `Fail closed on an in-scope non-run` never fires and the required
check goes green with no review performed on any PR that edits the caller.
Observed here and on #1766. Filed as a revisit trigger.

Restore skip-actors on claude-review.yml too — #1766 stripped both callers,
and fixing one left the identical failure mode live in the sibling.

Keep the superseded 2026-07-21 text in place with a marker rather than
editing a ratified addendum, and stop the new sections from orphaning its
closing paragraph.

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

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 4, 2026
…e repair

Second independent verification found the PR red-lined a required check and
disclosed it nowhere. .github/workflows/claude-review.yml is restored to its
pre-PR state, byte-identical to origin/main.

Cause: the pinned runner-policy contract for claude-review.yml@c136b27f
permits exactly one input, runner. Declaring skip-actors there fails
runner-target-contract, which fails the required ci-status check. runner-policy
is upstream-managed for this repo, so the contract entry cannot be edited here.
The security caller's contract entry does permit skip-actors, and that is the
lane carrying ADR 0002's ratified exception, so it keeps the explicit list.

The review lane's half is recorded in the ADR as deferred with a trigger
(re-pin to a SHA whose contract lists skip-actors, or a standards-reviewed
amendment to the c136b27f entry) rather than dropped silently. Two findings
worth the record: c136b27f is the only pinned claude-review.yml entry that
permits runner while omitting skip-actors, so the reviewed contract
memorialized #1766's dropped line a second time, independently of the caller;
and the mechanism that blocked the repair is exactly the compensating control
the 2026-07-21 addendum cites, firing as designed.

Also corrected:

- The effort-asymmetry paragraph, re-derived now that the review lane is out of
  both branches. Branch A needs no further edit; Branch B costs a one-line
  value change, an amendment to correction 2, and the upstream allowed_bots
  widening. Only the first half of that gap is a drafting artifact, and the
  paragraph now says so rather than dismissing the whole asymmetry.
- "Both lane callers" and "the restored lists" singularized to the security
  caller; the historical claim that #1766 dropped the line from both is
  unchanged, because it is still true.
- The revisit trigger for a caller that STOPS stating the list, which path 1
  would otherwise trip. The review lane is now a declared gap with its own
  trigger, not the undeclared drift that bullet exists to catch.
- The superseded marker on the falsified tamper mitigation, an HTML comment
  that rendered nowhere, converted to a visible blockquote. Every word of the
  ratified paragraph is preserved; the marker is appended after it.
- "this amendment's own PR" resolved to #1896.
- ci-workflows#345 added to the workflow-validation revisit trigger.
- The required set enumerated as required STATUS CHECKS, with the signing
  ruleset and base's linear-history and squash-only rules named separately.

Append-only holds: zero pre-existing lines deleted against the merge base.

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

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 4, 2026
…'s opener

Third independent verification found three defects, two of them mine from the
previous commit.

The deferral said the review lane "is not the lane this ADR's ratified exception
governs". That is false. Before #1766 BOTH callers carried the ratified
dependabot[bot],melodic-standards-sync[bot] list (verified at 9ed2956^), so the
review lane's inherited default is an unrepaired half of the same ratified
declaration, not an adjacent gap. What actually differs is consequence: the
review lane is advisory with no required check, so its inherited default cannot
let a PR satisfy a required gate with no review — the harm the exception was
ratified against. That bounds the deferral without putting the lane out of
scope. The same overstatement in the revisit-trigger bullet is corrected to
match.

The effort paragraph opened "The decision touches ONE file" and was contradicted
four lines later by its own bullets, which span this ADR and a second
repository. Reworded to say one WORKFLOW file is in play while naming Branch B's
wider reach, and the artifact/real-cost split is restated per row so the
allowed_bots dependency reads as what it is: the row that makes Branch B a
cross-repo change.

Also scoped the policy.json count precisely — eight pinned claude-review.yml
entries, six listing skip-actors, one permitting no inputs at all, and c136b27f
the only claude-review.yml entry permitting runner while omitting it. The
previous "the only entry" was true in context but overstated read alone.

Append-only holds: 152 insertions, 0 deletions against the merge base.

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

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 4, 2026
#1896)

## Summary

ADR 0002 records a skip-actor exception scoped to **two** actors and living in **the caller's** `skip-actors` list. Neither is true today.

[#1766](#1766) re-pinned the lane callers to v0.9.1 and dropped the explicit `skip-actors` line from **both** callers. The effective value silently became the reusable's own default — which had widened from one actor to four three days earlier (ci-workflows `cf666f67`). So `claude[bot]` and `melodic-ai[bot]` entered a required-check exception without the deliberation ADR 0002's own revisit trigger demands.

Neither has exercised it (`claude[bot]` has authored no PRs in this org; `melodic-ai[bot]` none here since the lane went live), so this is a **record defect, not an exploited one**.

What this changes:

1. **Mechanism** — the **security-review caller** states `skip-actors` explicitly again, so the exception is readable in the repo it governs and cannot be rewritten by an upstream default change. The inherited default is what failed, so the fix is the mechanism, not just the prose. The review lane's matching half is **deferred with a trigger** — see below.
2. **Compensating controls, stated accurately** — see the correction note below; the first draft of this PR got them wrong.
3. **Lever confusion** — records that `skip-actors` and the action's `allowed_bots` are different levers. Narrowing `skip-actors` alone does NOT restore review; the reusable passes `allowed_bots: dependabot[bot]`, the action throws on any other bot actor, and this lane's fail-closed mapping turns that into a required check red for a cause no push can fix.

## 🟡 Half the repair is deferred — the runner-policy contract blocks the review lane

**This PR originally edited both callers, and that red-lined a required check.** Disclosing it, because the episode is itself evidence for the ADR's thesis.

Declaring `skip-actors` on `claude-review.yml` failed `runner-target-contract`, which fails required `ci-status`:

```
.github/workflows/claude-review.yml#review: runner-target-contract:
the reusable workflow call has inputs absent from its reviewed contract: skip-actors
```

Cause, from `.github/standards/runner-policy/policy.json`:

| pinned target | `allowedInputs` | `skip-actors` permitted |
|---|---|---|
| `claude-review.yml@c136b27f` | `["runner"]` | **no** |
| `claude-security-review.yml@c136b27f` | `["runner","paths-file","skip-actors"]` | yes |

`runner-policy` is **upstream-managed** for this repo — `distribution/sync-manifest.yml` lists it under `melodic-software/claude-code-plugins` → `managed`, and `components/runner-policy/policy.json` is owned in `melodic-software/standards` — so the contract entry cannot be edited here.

**Resolution taken:** `.github/workflows/claude-review.yml` is restored to its pre-PR state — byte-identical to `origin/main`. `ci-status` is green again. Only the security caller, whose contract permits the input, keeps the explicit list. The review lane's inherited default is recorded in the ADR as **deferred with a trigger** (re-pin to a review-lane SHA whose contract lists `skip-actors`, or a standards-reviewed amendment to the `c136b27f` entry) rather than dropped silently. The ADR's revisit trigger for a caller that *stops* stating the list is amended so this reads as a declared gap, not the undeclared drift that bullet exists to catch.

Scope stated precisely, since it would be easy to overclaim here: before #1766 **both** callers carried the ratified `dependabot[bot],melodic-standards-sync[bot]` list, so the review lane's inherited default is an *unrepaired half of the same ratified declaration*, not an unrelated gap. What differs is consequence, not provenance — the review lane is advisory with no required check, so its inherited default cannot let a PR satisfy a required gate with no review, which is the harm the 2026-07-21 exception was ratified against. That bounds the deferral; it does not put the review lane out of scope.

Two findings from the episode, now in the ADR because they strengthen the record:

- **The contract entry is a second, independent memorialization of the same #1766 drift.** Six other pinned `claude-review.yml` entries in `policy.json` list `skip-actors`; `c136b27f` is the only entry that permits `runner` while omitting it. The reviewed contract recorded the accident too, separately from the caller.
- **The control that blocked the repair is the one ADR 0002 already credits.** The 2026-07-21 addendum cites "the reviewed runner-policy contract — an input-surface change declines auto-approval and requires a standards-reviewed policy entry" as a compensating control. That is exactly what fired here, as designed.

## 🔴 Gap this surfaced — the required check does not certify a pass on caller-editing PRs

Independent review of this PR found, and I confirmed on this PR's own run:

- The action refuses to run when the caller workflow file differs from the default branch (`Workflow validation failed… must have identical content to the version on the repository's default branch`) — precisely the class of change ADR 0002 names as its tamper mitigation.
- The step nonetheless reports **success**, so `Fail closed on an in-scope non-run` never fires.
- Result: `security-review / security-review` goes **green with no review performed and no tracking comment**, despite `track_progress: true`. Same signature on #1766 (12s run; the security lane posted no tracking comment).

Recorded in the ADR as a real bound plus a revisit trigger pointing at ci-workflows#345. **The fix belongs upstream in the ci-workflows outcome mapping** (an action-side validation skip on an in-scope PR should map to a non-run, not a pass) — not in this PR. Until then, a green security check on any PR touching the caller should be treated as unproven and reviewed by hand — **including this one**.

## ✅ OPERATOR DECISION — Branch A ratified (2026-08-04)

The restored list encodes the **four** actors currently in force, so **this PR is behavior-preserving as drafted** (byte-equal to the v0.9.1 default the security caller already inherits). Whether four is right was deliberately left to the operator, and the ADR carries an affirmative case for each branch:

- **Branch A — ratify four.** Stands on dormancy and review cost. (The 2026-07-21 rationales do *not* extend to these two actors — see the correction note.)
- **Branch B — revert to the ratified two.** Stands on the required check's claim that a pass actually ran, and on repairing the accident rather than blessing it.

**The operator picked Branch A on 2026-08-04**, so the silence default was never exercised. Recorded in the ADR as a dated addendum; per the doc's append-only supersession style the scaffolding paragraph that named silence as the ratifying default keeps its text and gains a superseded marker, and both branches' presented cases stay byte-untouched as the record of what was weighed. Branch B is recorded as outweighed while its premises hold, **not refuted** — the existing revisit trigger for agent actors beginning to author substantive changes under security-sensitive paths is the condition that reopens it. Four is now the ratified baseline the `skip-actors` trigger measures additions against.

No workflow edit accompanies the decision: Branch A is the no-further-edit branch by construction, so the merged diff is unchanged from what was already verified.

Cost model as weighed (the earlier draft understated Branch B, and the review lane is out of both branches so it differentiates neither):

| | Branch A (**chosen**) | Branch B |
|---|---|---|
| security caller | no further edit | one-line value change |
| ADR | no further edit | amend correction 2 (records four as in force) |
| upstream | none | `allowed_bots` widening in ci-workflows — **hard dependency**, not sequencing |

Branch A's zero-edit position is half a drafting artifact: had this PR restored two, Branch B would be the no-edit branch. The `allowed_bots` dependency is **not** an artifact — without it, Branch B converts a dormant record defect into a live merge block the moment either actor opens a PR.

## Correction note (second commit)

The first commit asserted three compensating controls without verifying them **in this repo**. Cold-read review falsified all three; they are now corrected in the ADR:

| Claimed | Actual |
|---|---|
| `secret-scan / gitleaks` runs and must pass | No such check context. gitleaks is a STEP in `ci.yml`'s `hygiene` job, aggregated fail-closed and gated transitively as `ci-status`. |
| GitGuardian must pass | Runs on every PR, but is in **no** ruleset — cannot block merge. |
| Merge still requires human review | **False.** `base` ruleset sets `required_approving_review_count: 0`; single collaborator; merges routinely carry zero approving reviews. Only `required_review_thread_resolution: true` applies. |

The third was load-bearing twice — offered as a compensating control *and* as half of Branch A's rationale. Both uses are withdrawn.

## Test plan

- [x] `markdownlint-cli2` clean on the amended ADR (0 errors)
- [x] `.github/workflows/claude-review.yml` restored: blob hash byte-identical to `origin/main`'s (`8be2703d`), so the file is absent from this PR's diff entirely
- [x] Security caller's YAML parses; `skip-actors` resolves to `dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot]`
- [x] Behavior-preserving: the restored list is byte-equal to the v0.9.1 reusable default the security caller currently inherits, so the effective skip set is unchanged on both lanes
- [x] No other behavioral change: triggers, permissions, SHA pins, concurrency, `runner`/`paths-file`, and the named secret are untouched
- [x] `policy.json` `allowedInputs` for both pinned targets read directly from the repo, not restated from review notes
- [x] Ownership confirmed before editing — `standards/distribution/sync-manifest.yml` lists `claude-security-review-caller` under `melodic-software/claude-code-plugins` → `locally-owned`, so the caller is the sanctioned local seam. The claim now matches the diff exactly, since the security caller is the only workflow this PR touches. Cited by key rather than by line number as a drift guard only — the earlier `:325` citation was and remains correct on live `standards` main (an intermediate revision of this body wrongly said it had drifted; that came from checking a stale local clone, and is withdrawn).
- [x] Append-only convention: zero pre-existing ADR lines deleted against the merge base
- [x] `ci-status` green at head (was red while the review caller carried the input)
- [x] Operator picked Branch A on 2026-08-04; recorded as a dated ADR addendum, append-only preserved (187 insertions, 0 deletions against the merge base) and no workflow edit needed
- [ ] Manual security review of this diff — the automated pass cannot certify it (see the gap above)

## Related

- ADR 0002 `docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md` — the record being repaired
- #1766 — the PR that dropped the caller-side lists while re-pinning to v0.9.1
- ci-workflows#345 — the upstream fix for the workflow-validation-skip gap
- Diagnosis this PR implements: `ci-workflows/.work/agent-returns/skip-actors-premise.md`. It also refutes the "agent PRs are pinned red by the security lane" premise that motivated a narrowing proposal — narrowing would have *caused* that condition, not cured it.

Closes #1897.
kyle-sexton added a commit that referenced this pull request Aug 9, 2026
No linked issue — this closes nothing. #1767 stays open deliberately:
its residual gap is
structural (`runner-policy` cannot *require* an input), and a comment
guards against a careful
reviewer, not an inattentive one. Closing it on a documentation change
would record the gap as
fixed when it is not.

## What

Seventeen lines of comment above `skip-actors` in
`.github/workflows/claude-security-review.yml`. No value changes; the
actor list stays at the
ratified four.

## Why

The existing comment says the list is stated explicitly rather than
inherited, and records how
#1766 widened it. It does not say why deleting the line is unsafe **when
its value matches the
upstream default of the moment** — which is the only state in which a
reviewer is tempted to delete
it, and precisely the state #1766 was in.

Two things the re-pin reviewer needs and the comment did not carry:

- **Deleting is not a no-op at parity.** The four actors are ADR 0002's
2026-08-04 ratified
baseline. Inheriting re-delegates that ratification to whatever
`ci-workflows` ships next, and
  every actor in an inherited default satisfies this repo's REQUIRED
`security-review / security-review` check with no review run, on
security-sensitive paths.

- **The failure directions are asymmetric**, which is what makes
explicit correct here without
contradicting the fleet's inheritance-over-explicit-list default
elsewhere. A stale explicit list
fails CLOSED and LOUD — for an actor the reusable's `allowed_bots` does
not permit, the action
throws and the fail-closed mapping turns that into a red required check
no push can fix. A stale
inherited default fails OPEN and SILENT — an actor this repo never
deliberated skips review with
CI green. Loud-and-blocking is the direction to fail in;
silent-and-permissive is not.

The first draft of this comment described the closed direction as "an
actor gets a review it may
not have needed — visible, cheap." That was wrong, and review caught it:
ADR 0002's
`skip-actors`-vs-`allowed_bots` section records that removing an actor
from `skip-actors` alone
does not restore review of its PRs — it blocks the merge. Corrected in
the second commit, which
also notes that un-skipping an actor for real requires widening
`allowed_bots` upstream, so
  removing a name here is never a one-line change.

The comment also names standards#308 at the point of use:
`runner-policy` rejects inputs outside the
reviewed contract but cannot *require* one, so dropping this line
re-widens the exception with CI
green and nothing said.

## Provenance

Salvaged from PR #1845, which I closed as superseded. That branch
re-narrowed the exception to two
actors on 2026-07-31; the 2026-08-04 addendum records an explicit
operator decision ratifying four
and names four as the baseline future additions are measured against,
having weighed the two-actor
case on its merits. The narrowing is therefore correctly dead — but the
branch's comment block was
the better explanation of an invariant that survives the ruling either
way, so it is ported here
with the actor list left alone.

## Verification

- `actionlint` clean on the changed file.
- The `skip-actors` value is byte-identical before and after — confirmed
by reading the parsed line
  back, not by eye.
- Comment-only diff: 17 insertions, 0 deletions.

## Related

- #1767 — the re-deliberation issue this comment serves; left open,
since the residual
  `requiredInputs` gap is not closed by a comment.
- #1845 — the superseded branch this is salvaged from.
- #1766 — the re-pin that dropped the line and silently widened the
exception.
- `docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md`
— the 2026-08-04 addendum
  that ratified four actors.
- melodic-software/standards#308 — the `requiredInputs`-style contract
field that would make this
  structurally enforced rather than comment-enforced.

---------

Co-authored-by: Claude Opus 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