docs(adr): repair ADR 0002's skip-actor exception record and mechanism by kyle-sexton · Pull Request #1896 · melodic-software/claude-code-plugins

kyle-sexton · 2026-08-03T22:38:02Z

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

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.
Compensating controls, stated accurately — see the correction note below; the first draft of this PR got them wrong.
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 ci: re-pin claude lane callers to v0.9.1, adopt paths-file, serialize the review lane #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 ci: re-pin claude lane callers to v0.9.1, adopt paths-file, serialize the review lane #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

ADR 0002 docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md — the record being repaired
ci: re-pin claude lane callers to v0.9.1, adopt paths-file, serialize the review lane #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.

#1766 dropped the caller's explicit skip-actors line while re-pinning to v0.9.1, whose default had widened from one actor to four three days earlier. The exception ADR 0002 records as caller-side has since been an inherited reusable default, and claude[bot] and melodic-ai[bot] joined it without the deliberation the revisit trigger demands. Restore the explicit caller-side list so the exception is readable where it applies and cannot be rewritten by an upstream default change. The restored list encodes the four actors currently in force, so this is behavior- preserving; the actor-set question is left as a marked operator decision. Also name the compensating controls a skipped PR still clears, and record that narrowing skip-actors alone fails closed — reviewing agent PRs needs allowed_bots widened upstream. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

…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>

…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>

…'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>

The 2026-08-03 amendment deliberately left the actor set open and named silence as the ratifying default. The operator made an explicit pick on 2026-08-04: Branch A — the widened four-actor exception is ratified, so the silence default was never exercised. claude[bot] and melodic-ai[bot] are now covered by the step-3 skip-actor exception through the deliberation the revisit trigger demands, rather than through the silent inheritance that put them there. No workflow edit accompanies the decision: the amendment already restored those four to stay behavior-preserving, so the effective skip set is unchanged on both lanes. The new addendum states what the ratification rests on — dormancy of the two added actors, and the lane's low value on agent-authored chore PRs relative to its spend — so a later reader can test the premises rather than take them. Branch B is recorded as outweighed while those premises hold, not refuted, with its allowed_bots cross-repo dependency named as the cost that decided the margin, and the existing trigger for agent actors authoring substantive changes named as the condition that reopens the question. Four becomes the ratified baseline the skip-actors trigger measures additions against. Both branches' presented cases and the effort paragraphs are byte-untouched: they are the record of what was weighed. Per the doc's supersession style the scaffolding paragraph keeps its text and gains a superseded marker pointing at the new addendum, so nothing is rewritten in place. Append-only holds: 187 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>

cursor · 2026-08-04T02:18:30Z

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 · 2026-08-04T02:18:44Z

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

Claude's review

Todo list

Gather context: read changed files, diff, and repo review guidelines
Review .github/workflows/claude-security-review.yml change
Review ADR 0002 amendment for correctness, consistency, and alignment with CLAUDE.md
Post findings

Scope note

Per REVIEW.md's lane split, this repo has a claude-security-review.yml workflow, so security-scoped findings (the caller-tamper gap, the workflow-validation-skip-to-pass mapping) belong to the security lane, not here — I did not re-review or re-litigate those; the ADR already tracks them with a revisit trigger against ci-workflows#345.

Verification performed

I independently checked the factual claims this PR's diff makes, rather than taking the test-plan checklist at face value:

.github/workflows/claude-review.yml is byte-identical to the merge base (git diff against it is empty) — confirms the "restored to pre-PR state" claim.
.github/standards/runner-policy/policy.json: claude-review.yml@c136b27f... → allowedInputs: ["runner"], claude-security-review.yml@c136b27f... → allowedInputs: ["runner", "paths-file", "skip-actors"] — matches the contract table in the PR body exactly.
Of the 8 pinned claude-review.yml entries in that file, 6 list skip-actors, one (1d3762c2) permits no inputs, and c136b27f is the only one permitting runner while omitting skip-actors — matches the "second, independent memorialization" claim precisely.
Diff is purely additive (194 insertions, 0 deletions across both files) — consistent with the append-only claim.

No factual discrepancies found in any of the above.

Findings

🟡 Nit — the workflow comment fully re-narrates ADR rationale that now lives in two places. .github/workflows/claude-security-review.yml#L38-L43 restates the full #1766 incident narrative that the ADR's 2026-08-03 addendum already records in detail. It's consistent with this file's existing heavy-comment style (the top-of-file block, L3-16), so it's not a convention violation, but it does mean the incident story now has two independent copies that can drift if the ADR is amended again (e.g. if a Branch-B-style revert happens later). A shorter comment pointing at the ADR section (mirroring the terse # One named secret... comment on L45) would reduce that surface without losing the "why" for a reader who won't open the ADR.

No other correctness, design, or consistency issues found. The ADR's addendum structure is internally consistent — the superseded markers correctly bracket only the falsified claims, the revisit-trigger list already accounts for the 2026-08-04 decision, and the actor list is identical between the workflow (L44) and the ADR (L269).

Not independently verifiable here

markdownlint-cli2 — network access needed to fetch the package was blocked in this sandbox, so I could not run the linter myself. Manual read against .markdownlint-cli2.jsonc's ruleset (ATX headings, dash bullets, no line-length limit, inline HTML allowed) found nothing that looked like a violation.
The standards repo's sync-manifest.yml ownership claim (claude-security-review-caller → locally-owned) — that file lives in an external repo not checked out here, so I couldn't check it directly; nothing in this repo contradicts it.

Summary

The change is small, well-scoped, and unusually thoroughly self-verified in the PR body — everything I checked held up exactly as claimed. One non-blocking maintainability suggestion above; nothing blocking.

github-actions · 2026-08-04T02:22:34Z

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

kyle-sexton and others added 2 commits August 3, 2026 18:37

kyle-sexton mentioned this pull request Aug 4, 2026

ADR 0002 skip-actors drift: caller-side list removed by #1766 while the reusable default widened to four actors #1897

Closed

kyle-sexton and others added 3 commits August 3, 2026 20:51

kyle-sexton marked this pull request as ready for review August 4, 2026 02:18

kyle-sexton merged commit 07d176c into main Aug 4, 2026
37 checks passed

kyle-sexton deleted the chore/adr-0002-skip-actors-repair branch August 4, 2026 02:23

Navigation Menu

Uh oh!

docs(adr): repair ADR 0002's skip-actor exception record and mechanism - #1896

docs(adr): repair ADR 0002's skip-actor exception record and mechanism#1896
kyle-sexton merged 5 commits into
mainfrom
chore/adr-0002-skip-actors-repair

kyle-sexton commented Aug 3, 2026 •

edited

Loading

Uh oh!

cursor Bot commented Aug 4, 2026

Uh oh!

claude Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

kyle-sexton commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

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

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

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

Correction note (second commit)

Test plan

Related

Uh oh!

cursor Bot commented Aug 4, 2026

Uh oh!

claude Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude's review

Scope note

Verification performed

Findings

Not independently verifiable here

Summary

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kyle-sexton commented Aug 3, 2026 •

edited

Loading

claude Bot commented Aug 4, 2026 •

edited

Loading