Skip to content

feat(ci): default-on security review — path-filtered caller + posture ADR - #722

Merged
kyle-sexton merged 8 commits into
mainfrom
feat/696-security-review-caller
Jul 20, 2026
Merged

feat(ci): default-on security review — path-filtered caller + posture ADR#722
kyle-sexton merged 8 commits into
mainfrom
feat/696-security-review-caller

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Contributor

Summary

Consumer wiring for the dedicated security-review lane plus the recorded decision, completing the two remaining default-on review items:

Related

Closes #696
Refs #509 (advisory increment only — the enforcement/evidencing design stays with the operator-deferred #509 design session)

🤖 Generated with Claude Code

@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review

  • Gather context and understand the PR
  • Read changed files
  • Review .github/workflows/claude-security-review.yml
  • Review .github/workflows/ci.yml change
  • Review ADR docs/adr/0002-...md
  • Post comprehensive review feedback

Overall this is a clean, well-structured PR. The security posture of the new caller is strong and the ADR is coherent. A few observations below, ordered from most to least significant.


.github/workflows/claude-security-review.yml

Permissions model — correct (L25–37)

Top-level permissions: contents: read correctly restricts the workflow's default grant. Job-level permissions are scoped to exactly what's needed: contents: read, pull-requests: write, id-token: write. The id-token: write appearing only at the job level (not the top-level) is the right call — OIDC token minting is scoped to where it's consumed.

Concurrency — correct and intentionally asymmetric (L28–31)

cancel-in-progress: false is the right choice for a security reviewer: an in-progress scan is never cancelled mid-flight. GitHub's queuing semantics (one pending slot per group) mean rapid pushes collapse to at-most two runs — the running one and the latest-enqueued one — which is a reasonable trade. The sibling claude-review.yml has no concurrency block at all; the new caller is more conservative, not less. No change needed, but if the sibling is ever updated, it should adopt the same pattern.

Secret handling — correct (L42–44)

Named secret, never secrets: inherit. Consistent with claude-review.yml.

SHA pin — correct (L38)

Full 40-character SHA with inline short-SHA + date comment. Format matches claude-review.yml and the action pins in ci.yml.

Path filter — well-calibrated, one gap worth noting (L14–23)

The filter covers the primary attack surfaces for this repo (CI config, plugin execution surfaces, shell scripts, ESM scripts). One gap: **/*.py, **/*.js, **/*.ts, **/*.bash aren't covered. For the initial rollout of a doc-heavy repo this is a reasonable tradeoff — those extensions would catch many plugin asset files and generate noise — but documenting the intentional exclusion in the ADR or a comment would prevent future confusion about whether they were forgotten. Not a blocker.


docs/adr/0002-...md

Decision rationale is sound

The advisory-with-earned-promotion posture is well-grounded in the cited evidence (#618 advisory noise, #697 friction data, WP5 discipline). Revisit triggers are concrete and actionable.

Minor format inconsistency with ADR 0001

ADR 0001 includes ## Consequences and ## Sources sections; ADR 0002 has neither. The "Revisit triggers" section partially covers consequences, but the format divergence could matter if a template or automation ever normalizes ADR structure. If a template lands later (the ADR 0001 consequences mention a sync PR for the convention README), ADR 0002 would need a retrofit. Not a blocker. Fix this →


.github/workflows/ci.yml

Single-line addition (L143) is exactly right — the new workflow file is added to the check-jsonschema step's file list in alphabetical order between claude-review.yml and do-not-merge.yml.


Summary

File Finding Severity
claude-security-review.yml Concurrency, permissions, pin, secrets all correct
claude-security-review.yml Path filter omits *.py/*.js/*.ts/*.bash — likely intentional but undocumented Minor
docs/adr/0002-...md Missing ## Consequences and ## Sources vs. ADR 0001's format Minor
ci.yml One-line addition is correct

No blocking issues. The two minor items are worth a follow-up but don't need to hold the merge.

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

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

Copy link
Copy Markdown
Contributor Author

Tower scope check (checkpoint 2026-07-20 ~17:5xZ) — trim before merge:

  1. Drop Closes #509. autonomy pipeline: no dedicated security-review pass — automated code review only #509 is operator-DEFERRED to a scoped /planning:design session (2026-07-19 decision batch, reaffirmed in the tower brief on autonomy pipeline: no dedicated security-review pass — automated code review only #509 today) — that's a specific operator instruction the momentum delegation does not override. This PR's advisory path-filtered caller is a legitimate increment, but it does not answer autonomy pipeline: no dedicated security-review pass — automated code review only #509's actual question (how the pipeline ENFORCES and EVIDENCES a security pass on every PR — the brief's RECOMMENDED direction is a required check, which an advisory lane is definitionally not). Keep Closes #696; change autonomy pipeline: no dedicated security-review pass — automated code review only #509 to Refs #509.
  2. Reframe ADR 0002 as interim: advisory posture is the current state pending autonomy pipeline: no dedicated security-review pass — automated code review only #509's design ruling, not the recorded end-state — otherwise the ADR forecloses the design session's decision space. One sentence fix.

With those two edits this merges as a clean #696 increment. Authoring lane or babysit can apply; tower will re-digest on the next cycle.

@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — post-sync (2f1fc78)

  • Gather context and understand the PR
  • Read changed files in latest HEAD
  • Summarize MCP-path fix and check pending requests
  • Post comprehensive review

MCP-path filter fix (2f1fc78) — ✅ Correct

The latest commit directly addresses the Codex P2 finding. Two patterns were added:

Directory patterns (L21–22):

- "plugins/*/src/**"
- "plugins/*/dist/**"

These cover the specific MCP plugin structure and will catch any plugin's source tree (e.g., plugins/miro/src/miro-client.ts) and compiled output.

Extension patterns (L26–28):

- "**/*.ts"
- "**/*.js"
- "**/*.cjs"

These provide blanket coverage for any TS/JS/CJS file anywhere in the repo, which is the right call for a security lane — a narrower pattern risks new plugin structures slipping through. The overlap with the directory patterns is harmless.

Scope note (informational, not a blocker): Combined with the existing **/*.sh, **/*.ps1, **/*.mjs, the path filter now fires on any PR touching code files. This is broader than the original "security-sensitive surfaces" framing, but appropriate — it matches what a security review should cover. The filter still correctly excludes pure-prose PRs (markdown, YAML config not under .github/, JSON schemas).


Pending items from kyle-sexton's 17:41 comment — ⚠️ Not yet applied

The latest commit addressed the Codex P2 concern but did not apply the two changes kyle-sexton explicitly requested. Both should be resolved before merging:

1. Closes #509Refs #509 in the PR body and commit b14b320

The commit message for b14b320 reads Closes #696 / Closes #509. Merging as-is will auto-close #509. Per kyle-sexton's comment, #509 (how the pipeline enforces a security pass — the "required check" design question) is explicitly deferred to a scoped design session; this PR's advisory lane is a legitimate increment toward #509 but does not answer it. Closing #509 prematurely would lose that design gate.

The PR body needs Closes #509 changed to Refs #509. Note: if the commit message also says Closes #509 and the PR squash-merges (as per repo convention), GitHub will close #509 from the commit message regardless of the PR body — so the commit message text matters too.

2. ADR 0002: missing interim-framing sentence

The ADR currently reads Status: accepted with no qualifier. Kyle-sexton's intent: "advisory posture is the current state pending #509's design ruling, not the recorded end-state — otherwise the ADR forecloses the design session's decision space." A single sentence in the Decision section (e.g., "This is the interim posture pending the #509 design ruling on enforcement; that session may elect to promote the security lane to a required gate with no further precision window.") would preserve the design space. Fix this →


Previously reviewed items — status unchanged

Item Status
Permissions model (top-level + job-level) ✅ Correct
Concurrency cancel-in-progress: false ✅ Correct
SHA pin with inline comment ✅ Correct
Secret handling (named, not inherit) ✅ Correct
ci.yml single-line alphabetical addition ✅ Correct
ADR ## Consequences + ## Sources sections missing vs ADR 0001 Minor — still open
Closes #509 should be Refs #509 ⚠️ Pending
ADR interim framing ⚠️ Pending
MCP-server paths missing from filter ✅ Fixed in 2f1fc78

Recommendation: The two ⚠️ Pending items from kyle-sexton's comment should be applied before merge — particularly Closes #509Refs #509, since merging as-is will auto-close a deferred design issue.

@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: 2f1fc78b8c

ℹ️ 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 to melodic-software/standards that referenced this pull request Jul 20, 2026
… reusable workflow (#211)

## Summary

Adds the reviewed runner-input contract entry for
`melodic-software/ci-workflows/.github/workflows/claude-security-review.yml@0a5fd4c…`
(merged in ci-workflows#160), mirroring the claude-review sibling's
shape exactly: `routing: runner-input` with `runner`, `allowedInputs`
runner + skip-actors, single named `CLAUDE_CODE_OAUTH_TOKEN`, caller
permissions contents:read + pull-requests:write + id-token:write.

Unblocks claude-code-plugins#722's Runner-policy gate (the new
security-review caller currently fails `runner-target-contract: no
reviewed runner-input contract`). Component tests: 224/224 pass.

## Related

- melodic-software/ci-workflows#160 (the lane)
- melodic-software/claude-code-plugins#722 (the blocked consumer caller)

No linked issue in this repository.

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

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — post-merge sync (2d00174)

  • Gather context and understand the PR
  • Read current state of changed files
  • Check pending items from kyle-sexton's 17:41 comment
  • Review merge commit for any new concerns
  • Post comprehensive updated review

The latest commit (2d00174) is a clean merge from main — no substantive changes to the three PR files. The diff stat is identical to the prior review: 3 files, 96 additions, 0 deletions.


Status of pending items from kyle-sexton's 17:41 comment

Both items are still open:

1. Closes #509Refs #509 in the PR body ⚠️ Still pending

The PR body still reads Closes #509. Per repo convention, this PR squash-merges (PR title becomes the commit subject), so the auto-close would fire from the PR body, not the commit message. The b14b320 commit title uses (#696, #509) parenthetical notation, which does not auto-close — so only the PR body is the risk. This requires a manual edit to the PR body before merge. (I cannot modify PR bodies.)

2. ADR 0002: interim-status framing ⚠️ Still pending

The ADR still reads Status: accepted with no interim qualifier. Kyle-sexton's intent was: "advisory posture is the current state pending #509's design ruling, not the recorded end-state — otherwise the ADR forecloses the design session's decision space." Suggested fix:

Fix ADR interim framing →


Previously resolved items — status unchanged

Item Status
Permissions model (top-level + job-level) ✅ Correct
Concurrency cancel-in-progress: false ✅ Correct
SHA pin with inline comment ✅ Correct
Secret handling (named, not inherit) ✅ Correct
ci.yml alphabetical addition ✅ Correct
MCP-server paths in path filter ✅ Fixed in 2f1fc78
Runner-policy contract (Codex P1) ✅ Acknowledged — upstream standards#211 brings the contract via the next sync PR
Closes #509Refs #509 ⚠️ Still pending (manual PR body edit needed)
ADR interim-status framing ⚠️ Still pending
ADR missing ## Consequences + ## Sources vs ADR 0001 Minor — still open

Recommendation: The ⚠️ Pending items should be resolved before merge — particularly Closes #509Refs #509, since merging as-is will auto-close a deferred design issue that per kyle-sexton's 17:41 comment is explicitly out-of-scope for this PR. The ADR format gap (## Consequences + ## Sources) remains a minor follow-up, not a blocker.

@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: 2d001744a4

ℹ️ 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 and others added 3 commits July 20, 2026 14:12
… ADR (#696, #509)

Wires the ci-workflows claude-security-review reusable workflow (pinned
at its merge SHA) behind a path filter over this repo's
security-sensitive surfaces, and records the default-on decision as ADR
0002: both AI review lanes default-on and ADVISORY, promotion to
blocking earned by demonstrated precision (the WP5
verification-promotion discipline), security severity vocabulary
CRITICAL/IMPORTANT/SUGGESTION with a confidence axis. Registers the new
caller in the workflow schema-check list.

Closes #696
Closes #509

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ew path filter

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

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton force-pushed the feat/696-security-review-caller branch from 2d00174 to a845663 Compare July 20, 2026 18:13
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — post-sync (a845663)

  • Gather context and read changed files
  • Review latest commit (a845663) — MCP launch config path fix
  • Check pending items from kyle-sexton's 17:41 comment
  • Post comprehensive review

Latest commit (a845663) — ✅ Correct

Two patterns added to the path filter in claude-security-review.yml:

- "**/.mcp.json"
- "**/mcp.json"

This addresses the Codex P2 finding from the 18:11 round. MCP launch configurations (plugins/miro/.mcp.json and similar) define the process command, arguments, and environment injection — they are executable surfaces that can redirect token handling without touching src or dist. Covering both the dotfile and the bare-name form is correct; both patterns are needed because the dotfile form is common (hidden config) and the bare form covers named variants. The overlap with the existing directory patterns is harmless.

The path filter as a whole is now comprehensive:

  • .github/**, .claude/** — CI config and agent rules
  • scripts/**, hooks/tools/skills scripts — execution surfaces
  • plugins/*/src/**, plugins/*/dist/** — MCP server source and compiled output
  • **/.mcp.json, **/mcp.json — MCP launch configs (new)
  • **/*.sh, **/*.ps1, **/*.mjs, **/*.ts, **/*.js, **/*.cjs — all code extensions

Pending items from kyle-sexton's 17:41 comment — ⚠️ Both still open

1. Closes #509Refs #509

The original commit 3feef14 body reads Closes #696 / Closes #509. Since the repo squash-merges (PR title becomes the commit subject, PR body becomes the body), GitHub will auto-close #509 from the PR body on merge. Per kyle-sexton's explicit instruction, #509 (how the pipeline enforces a security pass — the required-check design question) is deferred to a separate design session and must not be closed by this PR. The PR body needs Closes #509 changed to Refs #509 before merge. [I cannot modify PR bodies — this requires a manual edit by the PR author.]

2. ADR 0002: missing interim-status framing

docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md L3 still reads:

- Status: accepted

Kyle-sexton's intent: advisory posture is the current interim state pending #509's design ruling, not the recorded end-state. Without this qualifier, ADR 0002 closes the design session's option space. Suggested fix:

Fix ADR interim framing →


Previously reviewed items — final state

Item Status
Permissions model (top-level + job-level) ✅ Correct
Concurrency cancel-in-progress: false ✅ Correct
SHA pin with inline comment ✅ Correct
Secret handling (named, not inherit) ✅ Correct
ci.yml alphabetical addition ✅ Correct
MCP-server source + compiled JS paths ✅ Fixed in a14450a
MCP launch config paths (**/.mcp.json, **/mcp.json) ✅ Fixed in a845663
Runner-policy contract (Codex P1) ✅ Acknowledged — upstream standards#211 brings the contract via next sync PR
Closes #509Refs #509 in PR body ⚠️ Still pending — manual edit required before merge
ADR interim-status framing ⚠️ Still pending
ADR missing ## Consequences + ## Sources vs ADR 0001 format Minor — still open

Recommendation: Resolve both ⚠️ Pending items before merge. The Closes #509 item is the higher-priority one — auto-closing a deferred design gate on squash-merge would lose the enforcement design question permanently.

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

ℹ️ 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 Outdated
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — post-sync (b2277e5)

  • Gather context and understand the PR
  • Read current state of changed files
  • Review latest commit (b2277e5) — bin wrapper path fix
  • Check pending items from kyle-sexton's 17:41 comment
  • Post comprehensive review

Latest commit (b2277e5) — ✅ Correct

One-line addition to claude-security-review.yml L19:

- "plugins/*/bin/**"

This directly addresses the Codex P2 finding. The pattern is correctly scoped (plugins/*/) — consistent with the sibling hook/tool/skill patterns — and placement between hooks/** and tools/** is alphabetically ordered within that group. Extensionless scripts such as source-control-babysit-merge now schedule the security pass.


Path filter — final state

The filter as a whole is now well-calibrated. Every layer of the plugin execution surface is covered:

Layer Patterns
CI / agent config .github/**, .claude/**
Repo-level scripts scripts/**
Plugin hooks, bins, tools, skill scripts plugins/*/hooks/**, plugins/*/bin/**, plugins/*/tools/**, plugins/*/skills/*/scripts/**
Plugin MCP source + compiled output plugins/*/src/**, plugins/*/dist/**
MCP launch configs **/.mcp.json, **/mcp.json
Code extensions (blanket) **/*.sh, **/*.ps1, **/*.mjs, **/*.ts, **/*.js, **/*.cjs

No remaining gaps for the current plugin inventory.


Pending items from kyle-sexton's 17:41 comment — ⚠️ Both still open

1. Closes #509Refs #509

The initial commit (3feef14) body reads Closes #696 / Closes #509. Per this repo's squash-merge convention (PR title becomes the commit subject, PR body becomes the commit body), merging as-is will auto-close #509 from the PR body. Per kyle-sexton's explicit instruction, #509 (the pipeline-enforcement / required-gate design question) is deferred to a scoped design session — this PR's advisory lane is a legitimate increment toward it but does not answer it. The PR body needs a manual edit to change Closes #509 to Refs #509 before merge.

2. ADR 0002: missing interim-status framing

docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md L3 still reads:

- Status: accepted

Kyle-sexton's intent: the advisory posture is the current interim state pending #509's design ruling, not the recorded end-state; without a qualifier the ADR closes the design session's option space. Suggested two-part fix:

Fix ADR interim framing →


Full status table

Item Status
Permissions model (top-level + job-level) ✅ Correct
Concurrency cancel-in-progress: false ✅ Correct
SHA pin with inline comment ✅ Correct
Secret handling (named, not inherit) ✅ Correct
ci.yml alphabetical addition ✅ Correct
MCP-server source + compiled JS paths ✅ Fixed in a14450a
MCP launch config paths (**/.mcp.json, **/mcp.json) ✅ Fixed in a845663
Plugin bin wrapper paths (plugins/*/bin/**) ✅ Fixed in b2277e5
Runner-policy contract (Codex P1) ✅ Acknowledged — upstream standards#211 contract via next sync PR
ADR missing ## Consequences + ## Sources vs ADR 0001 format Minor — still open
Closes #509Refs #509 in PR body ⚠️ Pending — manual PR body edit required before merge
ADR interim-status framing ⚠️ Pending

Recommendation: Resolve both ⚠️ Pending items before merge. The Closes #509Refs #509 change is the higher-priority one — auto-closing a deferred design gate on squash-merge would lose the enforcement design question. The ADR format gap (## Consequences + ## Sources) remains a minor follow-up, not a blocker.

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

ℹ️ 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
Comment thread .github/workflows/claude-security-review.yml
… path filter

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — post-sync (1066c43)

  • Gather context and read changed files
  • Review latest commit (1066c43) — dependency and plugin-manifest path fix
  • Check pending items from kyle-sexton's 17:41 comment
  • Post comprehensive review

Latest commit (1066c43) — ✅ Correct

Three patterns added to the path filter in claude-security-review.yml L32–34:

- "**/package.json"
- "**/package-lock.json"
- "plugins/*/.claude-plugin/plugin.json"

**/package.json — correct. Addresses the Codex P2 finding: Dependabot can bump plugins/miro/package.json (the MCP server's @mirohq/miro-api + MCP SDK dependencies) without touching src/dist, and the security pass previously wouldn't fire. The broad glob is appropriate here — any package.json in the repo could introduce a vulnerable or malicious dependency.

**/package-lock.json — correct and necessary as a separate entry. Lockfiles are the authoritative resolved dependency tree; a crafted lockfile can point a semver range to a different package without changing the manifest. Including it independently (not relying on **/package.json to imply the lockfile) is the right call.

plugins/*/.claude-plugin/plugin.json — correct and appropriately scoped. Plugin manifests carry security-relevant runtime config: sensitive flags on user-config keys (e.g., API tokens), default-on/off states for safety controls. The plugins/*/ prefix correctly prevents this from matching unrelated plugin.json files at other paths.

Scope note (informational only, not a blocker): **/package.json is broad enough to match fixture or vendored package.json files if any are added later (test data, example projects). For the current repo inventory this is a non-issue; if it becomes noisy in the future, a scoped alternative like plugins/**/package.json can be evaluated then.


Path filter — final state

The filter is now comprehensive across all execution layers:

Layer Patterns
CI / agent config .github/**, .claude/**
Repo-level scripts scripts/**
Plugin hooks, bins, tools, skill scripts plugins/*/hooks/**, plugins/*/bin/**, plugins/*/tools/**, plugins/*/skills/*/scripts/**
Plugin MCP source + compiled output plugins/*/src/**, plugins/*/dist/**
MCP launch configs **/.mcp.json, **/mcp.json
Code extensions (blanket) **/*.sh, **/*.ps1, **/*.mjs, **/*.ts, **/*.js, **/*.cjs
Dependency manifests + lockfiles **/package.json, **/package-lock.json
Plugin runtime manifests plugins/*/.claude-plugin/plugin.json

No remaining gaps for the current plugin inventory.


Pending items from kyle-sexton's 17:41 comment — ⚠️ Both still open

1. Closes #509Refs #509 in the PR body

The PR body still reads Closes #509. Per kyle-sexton's explicit instruction, #509 (the pipeline-enforcement / required-gate design question) is deferred to a scoped design session — this PR's advisory lane is a legitimate increment toward it but does not answer it. Merging as-is will auto-close #509 from the PR body. This requires a manual edit to the PR body before merge. I cannot modify PR bodies.

2. ADR 0002: missing interim-status framing

docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md L3 still reads:

- Status: accepted

Without an interim qualifier, this ADR records the advisory posture as the settled end-state and closes the #509 design session's option space. Suggested two-part fix:

Fix ADR interim framing →


Full status table

Item Status
Permissions model (top-level + job-level) ✅ Correct
Concurrency cancel-in-progress: false ✅ Correct
SHA pin with inline comment ✅ Correct
Secret handling (named, not inherit) ✅ Correct
ci.yml alphabetical addition ✅ Correct
MCP-server source + compiled JS paths ✅ Fixed in a14450a
MCP launch config paths (**/.mcp.json, **/mcp.json) ✅ Fixed in a845663
Plugin bin wrapper paths (plugins/*/bin/**) ✅ Fixed in b2277e5
Dependency manifests + lockfiles (**/package.json, **/package-lock.json) ✅ Fixed in 1066c43
Plugin runtime manifests (plugins/*/.claude-plugin/plugin.json) ✅ Fixed in 1066c43
Runner-policy contract (Codex P1) ✅ Acknowledged — upstream standards#211 contract via next sync PR
ADR missing ## Consequences + ## Sources vs ADR 0001 format Minor — still open
Closes #509Refs #509 in PR body ⚠️ Pending — manual PR body edit required before merge
ADR interim-status framing ⚠️ Pending

Recommendation: The path filter is now comprehensive and the workflow itself is correct. The two ⚠️ Pending items should be resolved before merge — particularly Closes #509Refs #509, since auto-closing a deferred design gate on squash-merge would permanently lose the enforcement design question.

@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: 1066c43c0a

ℹ️ 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 Outdated
Comment thread .github/workflows/claude-security-review.yml
…urity surfaces

In a plugin marketplace, skill/agent/command markdown is executable
behavior and the marketplace manifest supplies each plugin's source —
both now schedule the dedicated pass. Repository prose (docs, READMEs,
changelogs) deliberately stays with the general review lane.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — post-sync (28cffd3)

  • Gather context and understand the PR
  • Read current state of changed files
  • Review latest commit (28cffd3) — instruction assets + marketplace manifest fix
  • Check remaining pending items from kyle-sexton's 17:41 comment
  • Post comprehensive review

Latest commit (28cffd3) — ✅ Correct

Four changes to the path filter in claude-security-review.yml L21–24:

- "plugins/*/skills/**"      # was: plugins/*/skills/*/scripts/**
- "plugins/*/agents/**"
- "plugins/*/commands/**"
- ".claude-plugin/**"

plugins/*/skills/** — correct and appropriately broadened. The previous plugins/*/skills/*/scripts/** covered only executable shell wrappers under each skill; SKILL.md (the instruction that directs the agent's behavior, including token handling in the Miro skill) was excluded. The broader glob catches instruction assets. The expansion is well-motivated — in a plugin marketplace, skill markdown is behavior.

plugins/*/agents/** — correct. Agent definition files (plugins/review/agents/*.md) define the review agents' behavior and tool grants; an instruction-only change can introduce credential disclosure or unsafe tool-use without touching src/dist.

plugins/*/commands/** — correct. Analogous to skills and agents: slash-command markdown is executable behavior.

.claude-plugin/** — correct and appropriately top-level (not scoped to plugins/*/). The marketplace manifest at .claude-plugin/marketplace.json supplies each plugin's source — a config-only change can redirect an installed marketplace entry to a different plugin without touching any plugin's own manifest or source. Top-level glob is the right scope.

Commit message rationale — well-stated: "Repository prose (docs, READMEs, changelogs) deliberately stays with the general review lane". This is the critical scope line that keeps the dedicated pass affordable per ADR 0002. No documentation exclusion needed beyond what the commit message already captures.


Path filter — final state

The filter is now comprehensive across all execution layers:

Layer Patterns
CI / agent config .github/**, .claude/**
Repo-level scripts scripts/**
Plugin hooks, bins, tools plugins/*/hooks/**, plugins/*/bin/**, plugins/*/tools/**
Plugin instruction assets (skills, agents, commands) plugins/*/skills/**, plugins/*/agents/**, plugins/*/commands/**
Plugin MCP source + compiled output plugins/*/src/**, plugins/*/dist/**
MCP launch configs **/.mcp.json, **/mcp.json
Code extensions (blanket) **/*.sh, **/*.ps1, **/*.mjs, **/*.ts, **/*.js, **/*.cjs
Dependency manifests + lockfiles **/package.json, **/package-lock.json
Plugin runtime manifests plugins/*/.claude-plugin/plugin.json
Marketplace manifest .claude-plugin/**

No remaining gaps for the current plugin inventory. All Codex P2 findings are resolved.


Pending items from kyle-sexton's 17:41 comment — ⚠️ Both still open

1. Closes #509Refs #509 in the PR body

The PR body still reads Closes #509. Per this repo's squash-merge convention the PR body becomes the commit body, so GitHub will auto-close #509 on merge. Per kyle-sexton's explicit instruction, #509 (the pipeline-enforcement / required-gate design question) is deferred to a scoped design session — this PR's advisory lane is a legitimate increment toward it but does not answer it. This requires a manual edit to the PR body before merge. I cannot modify PR bodies.

2. ADR 0002: missing interim-status framing

docs/adr/0002-...md L3 still reads - Status: accepted. I attempted to apply this fix locally but cannot push to the branch (GitHub App write permission denied for this repo). The exact diff needed:

-  - Status: accepted
+  - Status: accepted (interim — pending #509 design ruling)

And at the end of the Decision section (after the severity-vocabulary item):

+  This is the interim posture pending the #509 design session on enforcement;
+  that session may elect to promote the security lane to a required gate
+  without requiring a further precision window.

Fix ADR interim framing →


Full status table

Item Status
Permissions model (top-level + job-level) ✅ Correct
Concurrency cancel-in-progress: false ✅ Correct
SHA pin with inline comment ✅ Correct
Secret handling (named, not inherit) ✅ Correct
ci.yml alphabetical addition ✅ Correct
MCP-server source + compiled JS paths ✅ Fixed in a14450a
MCP launch config paths (**/.mcp.json, **/mcp.json) ✅ Fixed in a845663
Plugin bin wrapper paths (plugins/*/bin/**) ✅ Fixed in b2277e5
Dependency manifests + lockfiles ✅ Fixed in 1066c43
Plugin runtime manifests (plugins/*/.claude-plugin/plugin.json) ✅ Fixed in 1066c43
Plugin instruction assets (skills, agents, commands) ✅ Fixed in 28cffd3
Marketplace manifest (.claude-plugin/**) ✅ Fixed in 28cffd3
Runner-policy contract (Codex P1) ✅ Acknowledged — standards#211 brings the contract via next sync PR
ADR missing ## Consequences + ## Sources vs ADR 0001 format Minor — still open
Closes #509Refs #509 in PR body ⚠️ Pending — manual edit required before merge
ADR interim-status framing ⚠️ Pending — push permission denied; exact diff above

Recommendation: The path filter is now comprehensive and the workflow itself is correct. The two ⚠️ Pending items should be resolved before merge. Closes #509Refs #509 is the higher-priority one — auto-closing a deferred design gate on squash-merge would permanently lose the enforcement design question.

…orcement design ruling

The operator deferred the enforcement/evidencing question to a design
session (2026-07-19, on-record on the issue); this ADR records the
interim advisory posture that session builds on, not its answer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — post-sync (6e4066c)

  • Gather context and understand the PR
  • Read current state of ADR 0002 (interim framing check)
  • Check remaining pending items from kyle-sexton's 17:41 comment
  • Post comprehensive review

Latest commit (6e4066c) — ✅ Correct

ADR 0002 now fully reflects the interim posture requested in kyle-sexton's 17:41 comment. Two changes applied:

Status line (L3):

- Status: accepted (interim — pending the #509 enforcement design ruling)

✅ Correct — qualifies the decision as interim without downgrading it from accepted.

Decision section close (L39–42):

"This is the interim posture pending #509's enforcement design session (operator-deferred, 2026-07-19): how the pipeline enforces and evidences that a security pass ran on every PR is that session's question, and it may promote the security lane to a required gate on its own terms without a further precision window."

✅ Correct — preserves the design session's option space (including the "without a further precision window" path). The operator-deferred date is explicit, which helps future readers understand the provenance.


Remaining pending item — ⚠️ Closes #509 in the PR body

The PR body still reads Closes #509. Per this repo's squash-merge convention the PR body becomes the commit body, so GitHub will auto-close #509 on merge. Per kyle-sexton's explicit 17:41 instruction, #509 (the pipeline-enforcement / required-gate design question) is deferred to a scoped design session and must not be closed here. This requires a manual edit to the PR body before merge — I cannot modify PR bodies. Change Closes #509 to Refs #509.


Full status table

Item Status
Permissions model (top-level + job-level) ✅ Correct
Concurrency cancel-in-progress: false ✅ Correct
SHA pin with inline comment ✅ Correct
Secret handling (named, not inherit) ✅ Correct
ci.yml alphabetical addition ✅ Correct
MCP-server source + compiled JS paths ✅ Fixed in a14450a
MCP launch config paths (**/.mcp.json, **/mcp.json) ✅ Fixed in a845663
Plugin bin wrapper paths (plugins/*/bin/**) ✅ Fixed in b2277e5
Dependency manifests + lockfiles ✅ Fixed in 1066c43
Plugin runtime manifests (plugins/*/.claude-plugin/plugin.json) ✅ Fixed in 1066c43
Plugin instruction assets (skills, agents, commands) ✅ Fixed in 28cffd3
Marketplace manifest (.claude-plugin/**) ✅ Fixed in 28cffd3
Runner-policy contract (Codex P1) ✅ Acknowledged — standards#211 brings the contract via next sync PR
ADR interim-status framing ✅ Fixed in 6e4066c
ADR missing ## Consequences + ## Sources vs ADR 0001 format Minor — still open
Closes #509Refs #509 in PR body ⚠️ Pending — manual PR body edit required before merge

One item left before merge: edit the PR body to change Closes #509 to Refs #509. Everything else is resolved — the path filter is comprehensive, the workflow is correct, and the ADR is properly framed as interim.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Both pending review items resolved: ADR 0002 now carries the interim-status framing (6e4066c) and the PR body closes only #696#509 stays open for the operator-deferred enforcement design session, with this lane recorded as the advisory increment it builds on.

@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: 6e4066c578

ℹ️ 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
…path filter

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

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


PR Review — post-sync (0465acc)

  • Gather context and understand the PR
  • Read current state of changed files
  • Review latest commit (0465acc) — root agent-instruction files fix
  • Verify all previously pending items are resolved
  • Post comprehensive review

Latest commit (0465acc) — ✅ Correct

Three entries added to the path filter in claude-security-review.yml L25–27:

- "REVIEW.md"
- "CLAUDE.md"
- "AGENTS.md"

This directly addresses the final Codex P2 finding. These root-level files are the highest-priority instruction blocks injected into every review and agent context — a change that weakens a security criterion in REVIEW.md, adjusts tool permissions in CLAUDE.md, or rewrites behavior in AGENTS.md is a security-relevant change that previously slipped through the filter. All three belong here.

Placement note (informational only): The three entries land between .claude-plugin/** and plugins/*/src/**. Since GitHub's path filter is a logical OR (any match fires the workflow), ordering has zero functional impact. Thematically they're kin to .github/** and .claude/** at the top of the list, but a follow-up reorder is cosmetic, not a correctness issue.


Path filter — final state

Every execution layer is now covered:

Layer Patterns
CI / agent config .github/**, .claude/**
Root agent-instruction files REVIEW.md, CLAUDE.md, AGENTS.md
Repo-level scripts scripts/**
Plugin hooks, bins, tools plugins/*/hooks/**, plugins/*/bin/**, plugins/*/tools/**
Plugin instruction assets plugins/*/skills/**, plugins/*/agents/**, plugins/*/commands/**
Plugin MCP source + compiled output plugins/*/src/**, plugins/*/dist/**
MCP launch configs **/.mcp.json, **/mcp.json
Code extensions (blanket) **/*.sh, **/*.ps1, **/*.mjs, **/*.ts, **/*.js, **/*.cjs
Dependency manifests + lockfiles **/package.json, **/package-lock.json
Plugin runtime manifests plugins/*/.claude-plugin/plugin.json
Marketplace manifest .claude-plugin/**

No remaining gaps for the current plugin inventory. All Codex P2 findings resolved.


All previously pending items — resolved ✅

Closes #509Refs #509 — ✅ Confirmed resolved per kyle-sexton's 17:41 comment ("the PR body closes only #696#509 stays open for the operator-deferred enforcement design session").

ADR 0002 interim-status framing — ✅ Fixed in 6e4066c. Status line reads accepted (interim — pending the #509 enforcement design ruling) and the Decision section closes with the enforcement design session note.


Full status table

Item Status
Permissions model (top-level + job-level) ✅ Correct
Concurrency cancel-in-progress: false ✅ Correct
SHA pin with inline comment ✅ Correct
Secret handling (named, not inherit) ✅ Correct
ci.yml alphabetical addition ✅ Correct
MCP-server source + compiled JS paths ✅ Fixed in a14450a
MCP launch config paths ✅ Fixed in a845663
Plugin bin wrapper paths ✅ Fixed in b2277e5
Dependency manifests + lockfiles ✅ Fixed in 1066c43
Plugin runtime manifests ✅ Fixed in 1066c43
Plugin instruction assets (skills, agents, commands) ✅ Fixed in 28cffd3
Marketplace manifest ✅ Fixed in 28cffd3
Root agent-instruction files (REVIEW.md, CLAUDE.md, AGENTS.md) ✅ Fixed in 0465acc
ADR interim-status framing ✅ Fixed in 6e4066c
Closes #509Refs #509 ✅ Resolved (PR body)
Runner-policy contract (Codex P1) ✅ Acknowledged — standards#211 brings the contract via next sync PR
ADR missing ## Consequences + ## Sources vs ADR 0001 format Minor — still open

No blocking issues. The ADR format gap (## Consequences + ## Sources) remains a minor cosmetic follow-up — it doesn't affect the decision record's correctness or the security lane's behavior. Everything else is resolved. This PR is ready to merge.

@kyle-sexton
kyle-sexton merged commit 3a87f5f into main Jul 20, 2026
21 checks passed
@kyle-sexton
kyle-sexton deleted the feat/696-security-review-caller branch July 20, 2026 19:04
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.

reviews: default-on automated review — promote LLM lane + auto-invoke fanout (1→2 residue)

1 participant