feat(runner-policy): auto-approve Dependabot SHA bumps with an unchanged security surface - #119
Conversation
…ged security surface Dependabot bumps the SHA of an already-reviewed reusable workflow with no change to its own commit history in policy.json, so every bump fails closed with "no reviewed runner-input contract" until a maintainer manually adds a new contract entry -- confirmed recurring 5x on one PR in melodic-software/provisioning#72, plus hits in claude-code-plugins, standards, and github-iac. Before per-job checks run, resolveAutoApprovedContracts() now looks for path@SHA candidates that share a workflow path with an already-approved contract but have no contract of their own. For each, it fetches the previously reviewed revision and the candidate revision from the source repository (raw.githubusercontent.com, no token needed since ci-workflows is public) and structurally diffs permissions and on.workflow_call.inputs/secrets. A byte-identical surface auto-approves the candidate under the reviewed contract, stamped with autoApproved: { basisSha, approvedAt } provenance. Any fetch failure, parse failure, or surface change still fails closed, with the declined reason folded into the existing diagnostic instead of a bare "no reviewed runner-input contract". This is a deterministic structural comparison, not an LLM judgment call, and grants no blanket trust to a source repository: every other file in the called workflow, and any change to permissions/inputs/secrets themselves, still requires a human to add a new contract entry. Set disableAutoApproval: true (or CI_RUNNER_POLICY_DISABLE_AUTO_APPROVAL=true in CI) to restore today's behavior and require an explicit contract for every SHA. Also hardened the test suite's shared audit() helper to default to a hermetic fetch stub -- without it, the existing "obsolete reusable workflow SHA" test (whose fixture shares a workflow path with an approved contract) silently made a live network call to raw.githubusercontent.com on every run. Updates the threat model: the analyzer's "does not fetch external bytes" claim no longer holds, so the data flow, trust boundaries, threats table, and residual risk sections now describe the new fetch-and-diff step and its bounded blast radius. Verification: 118/118 components/runner-policy tests pass (112 existing + 6 new covering identical-surface auto-approval, changed-permissions and changed-inputs decline paths, a fetch-failure decline, the disableAutoApproval option, and the CI_RUNNER_POLICY_DISABLE_AUTO_APPROVAL env var). Confirmed via instrumented fetch that this repository's own "Enforce runner policy on standards" CI self-audit makes zero network calls today (no candidates exist until the next Dependabot bump). lefthook pre-commit (typos, editorconfig, gitleaks, biome, markdownlint) all pass. Propagation: runner-policy is a managed component per distribution/sync-manifest.yml. Consumer repositories receive this fix through the existing sync mechanism once this merges; this PR does not touch consumer repositories directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qmz53fqQcp7BqStweHH41H
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1c93e8355
ℹ️ 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".
Resolves conflicts in runner-policy.mjs/test.mjs: main rewrote routeStatus (required repository-local runner inputs, unroutable failure sentinel) after this branch's tip; this branch's routeStatus was untouched, so main's version is kept in full alongside this branch's new Dependabot auto-approval functions (resolveAutoApprovedContracts, reusableWorkflowSecuritySurface, etc).
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8450e02804
ℹ️ 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".
…in the auto-approval security surface
The Dependabot auto-approval diff (resolveAutoApprovedContracts) only
compared workflow-level permissions plus workflow_call inputs/secrets
between the previously reviewed basis SHA and the candidate SHA. Two
P1 review findings (chatgpt-codex-connector) identified gaps that let
a security-relevant change through as auto-approved:
- A job-level `permissions:` block can grant more than the
workflow-level default; only the workflow-level block was compared.
jobPermissionsSurface now captures each job's effective permissions
(job.permissions ?? workflow.permissions, via the existing
effectivePermissions helper) and includes it in the diffed surface.
- The reviewed reusable-workflow contract this feature auto-approves
is specifically a routing contract (runner-input or hosted-only),
but the diff ignored jobs.*.runs-on entirely. A candidate SHA could
keep the same inputs/secrets/permissions while flipping runs-on
(e.g. `${{ inputs.runner }}` -> `self-hosted`) and still be
auto-approved. jobRoutingSurface now captures each job's runs-on and
strategy (matrix values can change a matrix-driven runs-on's
resolved target without changing the runs-on literal) and includes
it in the diffed surface.
Both additions extend securitySurfaceDiffField's compared keys, so any
difference in either surface declines auto-approval with a specific
diagnostic naming the changed field, same as the existing
permissions/inputs/secrets diagnostics.
|
Claude finished @kyle-sexton's task in 0s —— View job I'll analyze this and get back to you. |
Preserve GitHub's distinction between omitted permissions and an explicit empty mapping at both workflow and effective job scope. Require the fetched candidate to retain a valid workflow_call trigger, and extend the compared runner boundary to nested reusable calls, containers, services, and deployment environments. Add adversarial regression coverage and align the component documentation and threat model with the enforced structural contract.
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
|
@codex review |
…pendabot-autoapprove
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b1f10493d
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b1f10493d
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd30e28cc1
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 898087b2d0
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bff65f2b2a
ℹ️ 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".
…oval surface A Dependabot SHA bump of a reviewed reusable workflow was auto-approved by diffing only permissions, workflow_call, and job routing/boundaries between the previously reviewed and candidate revisions. The fetched candidate's job steps/env were never checked, so a bump could add a localCredentialActions entry (e.g. actions/create-github-app-token) or an unapproved credential expression to a called job while leaving that compared surface unchanged, and the job would inherit the old self-hosted contract without ever passing the privileged-hosted credential check already enforced against every directly declared or repository-local job. Extend the auto-approval security surface with a credentials dimension that runs the existing privilegedHostedRequirement() against each job in the fetched reusable workflow, the same detection already trusted for direct/local jobs. Update THREAT-MODEL.md and README.md so the documented compared surface matches the code. Verified the regression test fails without the fix (auto-approval silently accepts the credential-action bump) and passes with it.
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31d1349809
ℹ️ 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".
jobPermissionsSurface, jobRoutingSurface, and jobCredentialSurface each filtered out a job whose value was not a mapping (e.g. jobs.extra: [] or a scalar), the same shape auditRepository rejects locally as job-shape, before comparing surfaces. Filtering kept those surfaces from ever calling privilegedHostedRequirement with a malformed job, but it also made the malformed job invisible to the diff: a bumped SHA could add one without changing anything the compared surface inspected, so the candidate could still match the reviewed basis and inherit its contract, only to fail later when GitHub actually validated the called workflow. Add malformedJobIds() and check both the candidate and every reviewed basis for a malformed job before their per-job surfaces are computed or diffed. A malformed candidate job declines auto-approval with a diagnostic identifying the job; a malformed basis job is treated the same as an unparseable basis, folding into the existing "could not be fetched, parsed, or validated" failure path so it fails closed instead of silently matching. Verified both regression tests fail without the fix (a malformed candidate or basis job is silently auto-approved) and pass with it. Validation passed: runner-policy 156/156, repository self-audit, Biome, and Lefthook validation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5107550f76
ℹ️ 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".
…ir category jobCredentialSurface recorded only privilegedHostedRequirement()'s category for each job in a fetched reusable workflow (e.g. "an unapproved or transformed credential expression"), never the credential-bearing value itself. A candidate SHA that swapped an already-declared/allowed secret for a different secret in the identical env/with position tripped the same category and produced an identical requirement object, so the coarse comparison alone let the candidate silently inherit the previously reviewed contract even though the actual credential referenced had changed. Add credentialReferences to the auto-approval security surface: jobCredentialReferenceSurface() mirrors localCredentialRequirement's own traversal (workflow-level env, job condition, job fields outside steps, then each step's condition, non-credential fields, env, and with), but instead of stopping at the first credential-bearing value and returning a category, it records every credential-bearing value's own normalized text per job. Include the new field in securitySurfaceDiffField so a same-category, different-secret change is a visible diff. Update THREAT-MODEL.md and README.md so the documented compared surface matches the code. Verified the regression test fails without the fix (auto-approval silently accepts the swapped-secret bump, producing no finding at all) and passes with it. Full suite: runner-policy 158/158, lint:runner-policy against the repository itself, and lint:md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52e0b6c30b
ℹ️ 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".
…direction and selector-result reporters Two Codex P1 findings identified auto-approval surface gaps the structural diff cannot close: a job's runs-on (or other routing field) can stay byte-identical while resolving through needs.<job>.outputs.<name> to a producing job whose value changes the real runner boundary, and a selectorResultInput fail-closed reporter's required-check guarantee depends on its own steps honoring the forwarded selector result, which sits outside the compared surface entirely. Both are undetectable by structural diffing, so both now decline auto-approval unconditionally and require a human to add a new contract entry, consistent with this feature's existing fail-closed posture.
|
Claude finished @kyle-sexton's task in 11m 44s —— View job PR Review
SummaryThis PR is well-designed and the iterative hardening through the Codex review thread is thorough. All P1 Codex findings are addressed in the final commits. The architecture is fail-closed at every stage: incomplete basis evidence, surface diffs, contract ambiguity, malformed jobs, and dynamic routing indirection all decline. Below are the findings from my independent read. Findings1.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f30f6f99b9
ℹ️ 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".
…pendabot-autoapprove # Conflicts: # components/runner-policy/policy.schema.json
…ract ambiguity checks reviewedContractSurface() feeds differingReviewedContractFields(), which exists specifically so a Dependabot SHA bump cannot silently inherit one of several structurally-matching reviewed revisions' contract terms when those revisions disagree (see THREAT-MODEL.md's "inherits a broader contract from one of several matching reviewed revisions by insertion order" threat). It already compares every existing human-reviewed contract term (routing, runnerInput, selectorResultInput, allowedInputs, allowedSecrets, fixedRunsOn) but was written before allowedCallerPermissions existed, so a future contract carrying that field would bypass the ambiguity check entirely. allowedCallerPermissions is the same kind of term: an exact caller-side permission grant a human approved for one reviewed SHA, not something the diffed callee workflow surface encodes. Add it to reviewedContractSurface so disagreement between matching bases fails closed instead of silently picking whichever basis sorts first. This is a no-op today: no contract in this component's schema carries allowedCallerPermissions yet. It becomes load-bearing once a contract with that field exists, so it must land before or alongside such a contract rather than be added retroactively. runner-policy tests: 160/160 passed. Biome clean.
…s grants in auto-approval A future reviewed reusable-workflow contract can carry an allowedCallerPermissions grant: an exact caller-side permission and secret boundary that lets a job reach a self-hosted runner despite an elevated GITHUB_TOKEN grant (e.g. pull-requests:write, id-token:write), bypassing the local-routing permission and credential checks that otherwise force such jobs hosted. This is a narrow, deliberately human-reviewed exception. resolveAutoApprovedContracts' structural surface diff (workflow_call declaration, permissions, job routing, and credential *references*) proves the called workflow's caller-facing contract and execution boundary are unchanged between a reviewed SHA and a Dependabot-bumped SHA, but it never inspects step bodies (run: scripts, non-credential uses:) for content. A bumped SHA could keep every compared field identical while its steps do something different with an already privileged grant -- for example exfiltrating an id-token-derived credential or misusing a pull-requests:write grant -- and auto-approval would silently extend the already-reviewed grant onto that unreviewed content. This is the same class of gap selectorResultInput's existing unconditional decline closes for fail-closed selector reporting, with a larger blast radius (privileged permissions and self-hosted reachability rather than a required-check result). This corrects the previous commit on this branch tip, which only added allowedCallerPermissions to reviewedContractSurface's ambiguity check. That alone is inert in the common single-reviewed-SHA case (e.g. one approved claude-review@SHA): with only one basis there is nothing to disagree with, so the ambiguity check never fires and a Dependabot bump would still silently inherit the privileged grant. The unconditional decline below is the fix that actually closes that case; the ambiguity comparison remains as defense in depth for when multiple reviewed revisions exist and disagree. Two changes: - resolveAutoApprovedContracts declines auto-approval unconditionally whenever the matched reviewed basis carries allowedCallerPermissions, mirroring the existing selectorResultInput decline. - reviewedContractSurface also compares allowedCallerPermissions, so if multiple already-reviewed revisions of the same workflow path structurally match a candidate but disagree on their caller-permission grant, that disagreement is caught as ambiguity instead of silently picking whichever matching basis sorts first. THREAT-MODEL.md documents the new decline alongside the existing selectorResultInput one. Neither change has any effect today: no contract in this component's schema carries allowedCallerPermissions yet (that field does not exist on this branch). Both become load-bearing the moment a contract with that field exists, so they must land before or alongside such a contract rather than be added retroactively. Behavioral test coverage for the new decline path lands together with the schema field that introduces allowedCallerPermissions, since a contract carrying it is not currently schema-valid on this branch. runner-policy tests: 160/160 passed (unchanged). Biome and markdownlint clean.
…ssion test The two preceding commits added the allowedCallerPermissions fail-closed decline before allowedCallerPermissions existed as a schema-valid contract field, so their behavioral test coverage was deferred to land together with the schema field that introduces it (see #140). #140 has now merged main into this branch and added allowedCallerPermissions to policy.schema.json, runnerInputContract validation, and reusableWorkflowStatus's caller-permission check (scoped to target.kind === "selector-output" for the runtime waiver, a scoping decision that is orthogonal to and does not affect this auto-approval decline, which fires unconditionally whenever the matched reviewed basis carries the field regardless of how the candidate job routes). Add the deferred regression test: a Dependabot SHA bump of a reviewed allowedCallerPermissions contract is declined even when the fetched candidate source is byte-identical to the reviewed basis, proving the decline is unconditional rather than incidentally triggered by a structural diff. Also remove two now-stale comments/citations that claimed allowedCallerPermissions had no live effect or was not yet schema-valid: that was true when those commits were originally authored, against #119's pre-merge base, but is no longer true now that #140's schema field and components/runner-policy/policy.json's claude-review.yml contract both exist on main. runner-policy tests: 169/169 passed. Biome and markdownlint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBb8hPQQsw3GZV4QL6Zpvs
|
Claude finished @kyle-sexton's task in 6m 25s —— View job PR Review
SummaryThe Finding 1 — Codex P1 still open: bracket-notation needs-output references not caughtThe const NEEDS_OUTPUT_REFERENCE = /needs\.[A-Za-z0-9_-]+\.outputs\.[A-Za-z0-9_-]+/;GitHub's expression syntax supports bracket notation as a full semantic equivalent of dot notation, so The THREAT-MODEL.md describes the control as applying to Recommended fix: Extend the regex to match bracket notation as well: const NEEDS_OUTPUT_REFERENCE =
/needs(?:\.[A-Za-z0-9_-]+|\[['"][A-Za-z0-9_-]+['"]\])\.outputs(?:\.[A-Za-z0-9_-]+|\[['"][A-Za-z0-9_-]+['"]\])/;Alternatively, fail closed on any routing field that contains Finding 2 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eb61c4d3f
ℹ️ 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".
…dential-action ref bumps Two bot findings on PR #119 auto-approval hardening: - dynamicRoutingReferenceJobIds() matched only property-dereference needs-output syntax (needs.<job>.outputs.<name>), missing GitHub's equivalent index syntax (needs.<job>.outputs['<name>']). A fetched reusable workflow using index syntax could keep its runs-on byte-identical across a SHA bump while the producing job's output value changed underneath it, silently inheriting the reviewed contract. The detector now matches outputs followed by either `.` or `[`, covering any bracket-index spelling. - jobCredentialSurface recorded only privilegedHostedRequirement's category (e.g. "credential-minting action actions/create-github-app-token"), never the step's pinned @ref. A Dependabot bump repointing an already-reviewed credential-minting action at a different, unreviewed ref left the compared surface identical. A new credentialActionUses() helper backs both credentialAction() and a new per-step credentialAction field in jobCredentialReferenceSurface, so the full normalized uses: value (including ref) is part of the diffed credentialReferences surface. Adds regression tests proving both dot- and index-syntax needs-output routing decline auto-approval, and that a ref-only change to an existing credential-minting action declines. Updates README.md and THREAT-MODEL.md to document both surfaces precisely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBb8hPQQsw3GZV4QL6Zpvs
|
Claude finished @kyle-sexton's task in 3m 30s —— View job PR Review
SummaryBoth P1s from the previous review round are correctly addressed in Fix 1 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9a83ce92b
ℹ️ 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".
The previous commit's needs-output detector only matched bracket indexing on the output-name segment (needs.pick.outputs['runner']), missing that GitHub's index syntax is a generic property accessor equally valid for the job-id segment (needs['pick'].outputs.runner) and the literal outputs segment (needs.pick['outputs'].runner), per GitHub's expression docs, which list `.` (property de-reference) and `[ ]` (index) as interchangeable operators, not one reserved for a specific segment. NEEDS_OUTPUT_REFERENCE now accepts dot-or-bracket form (with optional whitespace) for both the job-id and outputs segments, so any GitHub-valid spelling of a needs-output reference is caught. Adds a regression test with every segment bracketed (needs['pick']['outputs']['runner']), confirmed to fail against the prior narrower fix before this change. Corrects README.md and THREAT-MODEL.md prose that described only the single-segment case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBb8hPQQsw3GZV4QL6Zpvs
|
Claude finished @kyle-sexton's task in 3m 43s —— View job PR Review
SummaryThe final commit Finding 1 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9b5d65670
ℹ️ 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".
…outing fields Seven rounds of P1 findings each closed one precise needs-indirection syntax (dot, then index/bracket forms) while leaving the next one open; an object-filter route (needs.*.outputs.runner, typically wrapped in join(...)) has no named job-id segment for a job-id-shaped pattern to match at all. Replace the enumerated blocklist with a coarse allowlist-style catch-all: decline whenever a routing-relevant field mentions the needs context at all (needs. or needs[, matched case-insensitively since GitHub's expression evaluator treats context and property names case-insensitively), regardless of what follows. False positives just mean a human reviews instead of auto-approving, which is the safe direction to err for this gate.
|
Claude finished @kyle-sexton's task in 2m 40s —— View job PR Review
SummaryThe single commit Codex P1 resolved: object-filter output routeThe precise, enumeration-based regex that previously matched specific spellings of const NEEDS_REFERENCE = /\bneeds\b\s*[.[]/i;This is the correct architectural response to a detector that had been patched three times (dot-only → bracket on outputs segment → bracket on both segments) and still missed Regex correctness: False positive direction: The comment explicitly states that false positives (routing fields mentioning Tests: Three new test fixtures and a new parameterized test loop (168 total test invocations, up from 165) cover exactly the three cases the old detector would have missed:
The prior dot/bracket/fully-bracketed tests are kept and re-verified (rather than deleted) with an explanatory comment confirming the coarse catch-all subsumes all specific prior cases. Documentation: THREAT-MODEL.md and README.md are updated to describe the allowlist rationale, name the object-filter form explicitly, and explain why the enumeration approach was abandoned. The diagnostic message update ( Carry-over non-blocking observations (unchanged from prior rounds)None of these are new; all were noted as non-blocking in prior Claude review rounds and none have been addressed (nor need to be for correctness).
What remains correct across the full PRThe full review of all prior commits (through
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16096b8cc9
ℹ️ 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".
The coarse needs-reference catch-all still required a property or index accessor (`.`/`[`) immediately after the `needs` token, which is itself an enumerated dereference shape. GitHub's expression functions can take `needs` as a bare argument and return a dereferenceable object, e.g. `fromJSON(toJSON(needs)).pick.outputs.runner`, where the token right after `needs` is a function's closing `)`, not `.` or `[`; that byte-identical routing field could keep passing the diff while the producer job's output changed the actual runner underneath it. Match the bare `needs` word itself instead of requiring an immediate accessor, matching the catch-all's own documented intent to decline any routing field that mentions `needs` at all. Adds a function-wrapped-needs regression case to the existing needs-reference catch-all test loop.
|
Claude finished @kyle-sexton's task in 2m 47s —— View job PR Review
SummaryThe latest commit ( Fix —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bc63c2144
ℹ️ 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".
jobCredentialReferenceSurface recorded only the fields of a credential-bearing step that themselves contained a credential expression (condition/env/with, filtered through credentialBearingEntries), never the rest of the step. A bumped SHA could keep an already-reviewed step's credential expression byte-identical while rewriting the step's `run:` body, or swapping a non-localCredentialActions `uses:` action, to consume that same credential differently; the filtered surface stayed unchanged and the diff never saw it, so auto-approval copied the reviewed runner contract onto unreviewed executable code. Once a step is identified as credential-bearing, record its full normalized content (`other`, `env`, `with`) instead of only the filtered subset, so any body change becomes a visible diff. The credential-bearing gate itself now checks every field family (condition, env, with, the remaining step body, and credentialAction) instead of omitting the remaining-step-body check, which would have dropped a step whose only credential expression lives directly in `run:` (no env/with block) out of the surface entirely. Adds a step-body-change regression case (env-based credential, run: rewritten) and a second case proving the gate itself fires when the credential expression is inline in `run:` with no env/with block.
|
Claude finished @kyle-sexton's task in 2m 41s —— View job PR Review
SummaryThe latest commit ( Fix — Credential-bearing step body changesThe fix restructures Gate expansion: Surface widening: Once a step is confirmed credential-bearing, the recorded The combination is correct: non-credential-bearing steps are still excluded entirely (no false positives on cosmetic Test coverage: Two new regression cases:
Finding 1 — THREAT-MODEL.md threat and evidence columns not updated (non-blocking)The threat description in the row at line 120 lists the attack surfaces the auto-approval diff covers, but does not describe the new gap: keeping a credential expression byte-identical while rewriting the step body that consumes it. The evidence column lists test case categories ("changed-credential-reference", "changed-credential-action-ref", etc.) but does not name the new cases added in Recommendation: Add "rewrites a credential-bearing step's body to consume the same credential differently" to the threat column description. Add "changed-credential-step-body and inline-run-credential cases" to the evidence column. Add "function-wrapped needs reference" to the evidence column at line 121. Not blocking. Fix this → Finding 2 —
|
1 participant
Problem
Runner policy correctly requires an explicitly reviewed contract for each reusable-workflow path and immutable SHA, but that made every Dependabot SHA update fail even when the already-reviewed workflow source retained the same security and execution surface.
Implementation
The analyzer now considers a new SHA only when the same workflow path already has a reviewed contract. It fetches both immutable revisions and compares a canonical, fail-closed surface before inheriting the reviewed contract with provenance.
The comparison covers:
Any incomplete basis fetch/parse/validation evidence, schema or declaration change, permission or routing change, or ambiguity among matching reviewed contracts declines automatic approval. There is no same-organization trust exception. disableAutoApproval and CI_RUNNER_POLICY_DISABLE_AUTO_APPROVAL preserve the explicit-contract-only mode and make no network request.
Distribution
Runner policy remains Standards-managed and will reach enrolled consumers only through the existing generated sync process. This PR does not hand-copy managed policy into downstream repositories.
Verification
Authoritative basis