Skip to content

feat(workflows): paths-file input for the security lane + Phase 2 close-out - #282

Merged
kyle-sexton merged 3 commits into
mainfrom
feat/claude-security-paths-file
Jul 27, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
feat/claude-security-paths-file

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Reusable-side prerequisite for the Phase 3 caller components (docs/topics/claude-review-lanes/PLAN.md, item 3a): the security lane gains a paths-file input so each consumer keeps its security-sensitive-surface list in its own tree at the conventional .github/claude-security-paths, instead of inlining it in the synced caller.

  • Precedence: non-empty inline paths wins (unchanged semantics, file never fetched); else paths-file is fetched from the PR's base repository at the base branch via the contents API inside the existing github-script step — no checkout added; both empty → every call relevant (byte-compatible with today).
  • Fail-open discipline throughout: absent file, fetch error, non-string response, or vacuous content → relevant=true with a warning. A matcher fault can never silently skip a required security review.
  • The changes job gains only contents: read (single-blob read; the job stays checkout-free). Base workflow's security-review job already required contents: read, so no existing caller breaks.
  • PLAN.md: Phase 2 marked [DONE] with close-out evidence (PR feat(workflows): Claude lane feature set — config currency, cadence, retry, kill-switches, copy (PR-B, 2b-2i) #280 / cf666f67, kill-switch dogfood, v0.9.0); Phase 3 [DOING].

Verification: fresh-context verifier ran a 13-case base/head differential (byte-identical semantics on all pre-existing paths) plus a 15-case adversarial fail-open matrix — verdict SHIP; its two findings (trust-boundary wording, vacuous-file regression pin) are folded in. 286 script tests green; actionlint clean; zizmor identical to base.

Related

No linked issue — part of the claude-review-lanes effort (Phase 3a prerequisite).

🤖 Generated with Claude Code

kyle-sexton and others added 3 commits July 27, 2026 08:51
Let a consumer keep its security-sensitive-surface list in its own tree
(conventionally `.github/claude-security-paths`) instead of inlining it
in the caller — the reusable-side prerequisite for the Phase 3a fleet
caller components. The `changes` job resolves the effective pattern
list: a non-empty explicit `paths` input wins (unchanged semantics);
else a non-empty `paths-file` is fetched via the contents API from the
PR's BASE repository at the base branch — never the head, so a PR
cannot edit the file to skip its own review; else every call stays
relevant. An absent or unreadable file fails OPEN to relevant=true with
a warning, matching the job's existing fail-open discipline. The
checkout-free `changes` job gains `contents: read` for the single-blob
getContent read.

Tests pin the base-ref read, the fail-open fault path, and — executing
the filter's expression-free run block directly — the input-precedence
contract.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Phase 2 [DOING] -> [DONE] with its close-out evidence: PR-B merged as
cf666f6 (#280), verifier verdict SHIP (findings fixed in 069a2c3),
dogfood count-comment and kill-switch observations, release v0.9.0;
the forced-failure sandbox probe and second-push cadence observation
are deferred into the Phase 3c smoke. Phase 3 [TODO] -> [DOING].

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ssion pin

Verifier findings: the base-branch read protects the FILE'S CONTENT
from PR tampering, not the caller input naming it — repointing the
input is a visible caller diff, the same trust boundary the inline
paths list already has; the description now says exactly that. A new
test pins the vacuous-file outcome (comments/blanks only → fail open)
so a refactor of the shared no-usable-patterns guard cannot silently
turn an empty matcher into skip-every-PR.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton enabled auto-merge (squash) July 27, 2026 13:13
@claude

claude Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

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


Code Review Todo

  • Gather context (read diff, changed files, PLAN.md)
  • Review .github/workflows/claude-security-review.yml changes
  • Review .github/scripts/claude-security-review-fail-closed.test.cjs changes
  • Review docs/topics/claude-review-lanes/PLAN.md changes
  • Post final review feedback

Review (code-review lane — security findings out of scope per REVIEW.md, owned by the security lane)

Solid, well-documented change. The precedence (paths > paths-file > none), the fail-open discipline (absent/unreadable file, vacuous pattern list, listing failure), and the header/consumer-contract docs are all internally consistent — I traced every branch of the new logic in claude-security-review.yml (both the github-script fetch step and the Determine security relevance shell step) and didn't find a correctness gap. Job-level contents: read addition on changes is correctly scoped and matches the "own the minimal grant at the job level" convention already used elsewhere in this file.

One non-blocking observation on test maintainability:

  • claude-security-review-fail-closed.test.cjs#L279-L289 — the assert.match against a literal multi-line regex (hardcoded to 14-space indentation) duplicates coverage already provided by the executable runFilter-based test a few lines down (a failed paths-file fetch fails open to relevant, L380), which verifies the same fail-open behavior by actually running the script rather than pattern-matching its source. The regex assertion is brittle to reformatting (e.g. a future indentation or wording tweak in the echo lines would break the test without any behavior change). Not blocking — this style is already used elsewhere in the file for intent-level checks (e.g. "the fetch pins the base ref, never the head") — but the fail-open portion of that assertion could be dropped in favor of the functional test below it, since it verifies nothing the executable test doesn't already cover. Fix this →

Also confirmed: PLAN.md's Phase 2 close-out and Phase 3 [DOING] transition line up with the referenced PR #280 / cf666f67, and the new paths-file shape matches what Phase 3a of the plan already describes as the prerequisite.

@claude

claude Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 45s —— View job


Security review complete

  • Gather context (diff, changed files, CLAUDE.md rules)
  • Review claude-security-review.yml workflow changes
  • Review test file changes
  • Review PLAN.md changes
  • Post findings

Findings

None. No security issues found in this PR's changes.

What I checked

.github/workflows/claude-security-review.yml — the new paths-file fetch logic in the changes job:

  • Trust boundary is correctly enforced. The paths-file is fetched via github.rest.repos.getContent with ref: context.payload.pull_request.base.ref, and context.repo resolves to the base repository even for fork-originated PRs on pull_request events. A PR (including a fork PR) cannot alter the file's content to skip its own review — matches the stated contract at claude-security-review.yml#L175-L183 and is pinned by a dedicated test (the paths file is read from the base branch and its faults fail open).
  • No script injection surface. All untrusted/config-controlled values (PATHS, PATHS_FILE, file paths) are threaded through env: and read from process.env / shell env vars — never interpolated directly as ${{ }} into a run: script body. The paths-file repo-relative path itself is caller/workflow-input-controlled, not PR-controlled, so it isn't an attacker-influenced value regardless.
  • Fail-open discipline is consistent and matches the stated design: 404/unreadable file, non-string content (directory/submodule), and vacuous (comments/blank-only) content all resolve to relevant=true with a warning — verified against the new shell logic and its test coverage (precedence, vacuous-file, fetch-failure, both-empty cases).
  • Permission grant is minimal and correctly scoped. The changes job gains only contents: read for a single-blob read via the Contents API; still no checkout. Consistent with the CLAUDE.md least-privilege rule for this lane.
  • No trigger/permission-widening changes. security-review job permissions, the pull_request_target/workflow_run tripwire, kill-switches, and fork-skip logic are all unchanged by this diff.

Test file (claude-security-review-fail-closed.test.cjs) — only test-scaffolding changes (a step-boundary fix for the "no ${{ }} in run blocks" assertion helper, plus new precedence/fail-open test cases). Not part of the executed workflow surface; no security implications.

PLAN.md — documentation-only status update (Phase 2 → [DONE], Phase 3 → [DOING]). No code or config changes.

Per instructions, I deferred to zizmor's advisory lane for supply-chain/pin, trigger, and permission-class findings (the PR description notes zizmor output is unchanged from base).

@kyle-sexton
kyle-sexton merged commit c136b27 into main Jul 27, 2026
1 check passed
@kyle-sexton
kyle-sexton deleted the feat/claude-security-paths-file branch July 27, 2026 13:14
@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.

kyle-sexton added a commit that referenced this pull request Aug 12, 2026
…change

Review caught the timeline explaining two user-actor versions while
leaving a third of the same shape unexplained. Version 45582580 carries
the kyle-sexton actor seventeen minutes before the bot-applied 45584301
that the prose credits with the PR #282 disable, so it was out of band
too — a hand disable the Pulumi apply then codified, which is why nothing
looked wrong afterwards.

That makes three out-of-band changes across three weeks, not two. The
only thing separating 45582580 from the 2026-08-10 flip is that the
declared state happened to agree with it, which is luck rather than a
mechanism — and it strengthens rather than softens the recorded lesson.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 12, 2026
…abled, not active (#450)

## Summary

`docs/topics/claude-review-lanes/PLAN.md` asserted that org ruleset
`19388547`
(`security-review-gate`) carries `enforcement: active`. It does not, and
has not
since 2026-08-11. Live state and the Pulumi declaration in
`melodic-software/github-iac` both say `disabled` — so **live matches
IaC and
there is no infrastructure drift**. The documentation was the half that
was
wrong, and this PR corrects only the documentation.

Direction of the fix follows the org's standing convention: GitHub org
settings
are Pulumi-managed in `github-iac` and changed there. The IaC
declaration
carries an explicit written rationale ("Agentic review is advisory.
GitHub Team
does not offer evaluation mode, so disabled is the only declarative
state that
cannot make a model-backed action a merge dependency."). The PLAN ledger
is a
record OF decisions, not the source of truth FOR them. **No ruleset was
touched, no `github-iac` change is proposed, and no workflow was
modified.**
Re-arming the gate is a policy change only the operator can authorize.

### What I determined about the 2026-08-10 flip

Determined conclusively, not inferred. The ruleset version-history API
(`gh api orgs/melodic-software/rulesets/19388547/history`, plus each
`.../history/<version_id>`) works on the Team plan — unlike the org
audit log,
which is Enterprise-only and returns 404. It gives a fully attributed
timeline:

| version | when (-0400) | actor | enforcement |
| --- | --- | --- | --- |
| `45584301` | 2026-08-05 23:38:29 | `melodic-software-github-iac[bot]`
| disabled |
| `46092646` | 2026-08-10 10:13:39 | `kyle-sexton` | **active** |
| `46262852` | 2026-08-11 19:29:45 | `kyle-sexton` | **disabled** |

The 2026-08-10 flip **was real** — made by the operator's user account
eleven
minutes before the ledger bullet recording it was committed (`53bba06`,
10:24:37 -0400). It was reverted 33 hours later by the same user
account.

The hypothesis that a Pulumi apply reasserted the declared `disabled` is
**false**. Both the flip and the revert carry a User actor, not the
`melodic-software-github-iac[bot]` actor every Pulumi-applied version
carries,
and no production deploy ran between 2026-08-06T03:38Z and
2026-08-11T23:26Z.

The sharper finding, recorded as a lesson: **Pulumi could not have
caught this
drift.** The production deploy's `pulumi refresh` is targeted at nine
`ci-runner-*` org variables (its log reads `9 unchanged`), so no ruleset
is ever
refreshed; `pulumi up` then diffs desired against *recorded* state
rather than
live, and reported this ruleset `unchanged` while live disagreed. UI or
ad-hoc
changes to any ruleset are invisible to the pipeline until a human
notices.
Recorded as an operator-facing observation only — no fix proposed here.

## Fix

Corrected by **appended, dated AMENDMENT rather than rewrite**. The file
is an
append-only ledger in places, and it already has its own convention for
this,
which I reused rather than inventing a second one:

> **PHASE 4 CLOSE-OUT AMENDMENT (2026-08-08; ...).** The list above is
kept
> verbatim as the 2026-08-06 record; every item is now dispositioned.

Both stale entries recorded what was believed *and true* at the time, so
rewriting them would falsify the historical record. Every original line
is
untouched — the diff is 91 insertions and 0 deletions.

Four stale assertions, all corrected:

1. **`3c INVARIANT RE-READ (2026-07-30)` — `enforcement: active`.** True
when
written; version history confirms `44922452` and `44942752` both carried
`active` on 2026-07-30. **Appended** correction noting it no longer
holds.
2. **The same block's `EXPECTED DOCUMENTED DELTA` instruction.** This
one is
forward-looking and now *inverted*: it tells a future reader that after
github-iac #247 applies, `bypass_actors: []` "would itself be drift".
#282
later removed that bypass, so empty is now the CORRECT state — following
the
line as written would diagnose correct state as a disarm. **Appended**
supersession, flagged explicitly because this one can actively mislead.
3. **`SECURITY-REVIEW-GATE ENFORCEMENT ACTIVE (2026-08-10,
operator-directed)`.**
Records a flip that happened but did not last. **Appended** correction
with
   the current state and the attributed timeline above.
4. **"the program has no open items beyond its recorded date/event
triggers"**
(end of the `DESIGN A PROBE DROPPED` bullet). Now false — #448 is open.
   **Appended** correction citing it.

The amendments also make explicit a distinction the original text blurs:
the
`security-review / security-review` check being present in the ruleset's
`rules`
is not the same as it being *enforced*. With `enforcement: disabled` the
rule is
declared but binds nothing.

## Verification

Everything below was run in this session, on this branch.

Live ruleset read — quoted verbatim:

```console
$ gh api orgs/melodic-software/rulesets/19388547 --jq '{name,enforcement,updated_at}'
{"enforcement":"disabled","name":"security-review-gate","updated_at":"2026-08-11T19:29:45.184-04:00"}
```

The consequence — `security-review / security-review` is a required
check
nowhere, including on the only repo carrying `requires-security-review
== "true"`:

```console
$ gh api repos/melodic-software/claude-code-plugins/rules/branches/main \
    --jq '[.[]|select(.type=="required_status_checks")|.parameters.required_status_checks[].context]'
["pr-title / pr-title","pr-issue-linkage / pr-issue-linkage","do-not-merge / do-not-merge","ci-status"]

$ gh api "orgs/melodic-software/properties/values?repository_query=props.requires-security-review:true" \
    --jq '.[].repository_name'
claude-code-plugins
```

Markdown gate (the repo uses `.markdownlint-cli2.jsonc`; there is no
root
`package.json`, so the equivalent is markdownlint-cli2 at the pinned
version):

```console
$ npx markdownlint-cli2@0.23.2 "docs/topics/claude-review-lanes/PLAN.md"
markdownlint-cli2 v0.23.2 (markdownlint v0.41.1)
Linting: 1 file
Summary: 0 issues in 0 files
```

Spell gate, matching CI's `typos` lane:

```console
$ typos docs/topics/claude-review-lanes/PLAN.md   # exit 0, no output
```

Change is docs-only: `git diff --stat` reports one file, `91
insertions(+)`, no
deletions. `reference-integrity` is scoped to `.github/**` and
`fixtures/reference-integrity/**`, so it does not apply.

Not claimed: I did not re-run the historical github-iac deploy jobs; the
actor
and timing evidence above comes from the ruleset history API and the
existing
run listings, read read-only.

## Related

- Closes #447 — the doc-correction tracking issue for this change.
- Related: #448 — **left deliberately OPEN and unassigned, addressed to
the
operator.** With the gate disabled, the two-tier availability ruling in
`.github/workflows/claude-security-review.yml` is premised on a required
check that currently binds nothing. That issue lays out the evidence
both
ways and proposes nothing; it is explicitly *not* resolved by this PR,
and no
  workflow was changed.
- Context: `github-iac` PR #282 (`fix(governance): make agentic reviews
advisory`) is what declared `disabled`; it also removed the #247
break-glass
  bypass and superseded ADR 0010.

---------

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