Skip to content

feat(runner-policy): approve the pr-automerge-dependabot reusable contract - #687

Merged
kyle-sexton merged 4 commits into
mainfrom
feat/contract-pr-automerge-dependabot
Oct 4, 2026
Merged

kyle-sexton merged 4 commits into
mainfrom
feat/contract-pr-automerge-dependabot

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

No related issue: follow-up registration for the ci-workflows v0.35.0 reusable reviewed on melodic-software/ci-workflows#676

Summary

Approves the new ci-workflows pr-automerge-dependabot.yml reusable at v0.35.0 in runner-policy and adds a lockstep check so its PUBLISHER_ALLOWLIST constant cannot drift from dependabot-policy autoMerge.publisherAllowlist.

Contract for hand review (components/runner-policy/policy.json, approvedReusableWorkflowContracts):

"melodic-software/ci-workflows/.github/workflows/pr-automerge-dependabot.yml@69e506b7c119517ef1dfc421479232b386b93a52": {
  "routing": "runner-input",
  "runnerInput": "runner",
  "allowedInputs": ["runner"],
  "allowedSecrets": {},
  "allowedCallerPermissions": {
    "contents": "write",
    "pull-requests": "write"
  }
}

allowedCallerPermissions rather than minimumCallerPermissions: runner-policy.mjs rejects a write scope in minimumCallerPermissions (GitHub downgrades caller write grants on Dependabot PRs), and the job declares exactly these two write scopes.

Fix

  • components/runner-policy/policy.json: the entry above, spliced as text (diff is additions only).
  • components/runner-policy/README.md: ledger note covering what the reusable does, its gating, permissions, inputs and secrets, pinned actions (dependabot/fetch-metadata v3.1.0 25dd0e34f4fe68f24cc83900b1fe3fe149efef98, actions/github-script v9.0.0 3a2844b7e9c422d3c10d287c895573f7108da1b3), and the ci-workflows#676 security review.
  • components/dependabot-policy/automerge-lockstep.mjs: reads the reusable's SHA from its single runner-policy contract key, fetches the reusable at that SHA over the contents API with three attempts (same transport and token handling as pr-convention-policy/lockstep-drift.mjs), finds the single PUBLISHER_ALLOWLIST step env, and fails on any difference from policy.json.
  • components/dependabot-policy/README.md and THREAT-MODEL.md: the reusable is live (no longer "planned"), and the lockstep is in scope with its own threat row.
  • Enforcement rollout (docs/component-lifecycle.md#enforcement-rollout): the lockstep blocks from admission with no report-only period. It compares two files at fixed revisions, so there is no fleet baseline to observe; the observation run against the live pinned reusable matched (PUBLISHER_ALLOWLIST at 69e506b matches policy.json).
  • package.json: lint:dependabot-automerge-lockstep.
  • .github/workflows/ci.yml: a step in the existing dependabot-policy job. The fetch targets an immutable SHA in a public repository, so a separate job would add a needs edge to ci-status with no isolation gain.

Verification

  • npm run lint:runner-policy: Runner policy passed.
  • node --test components/runner-policy/runner-policy.test.mjs: 296/296 pass.
  • npm run lint:dependabot-automerge-lockstep: PUBLISHER_ALLOWLIST at 69e506b matches policy.json.
  • npm run test:dependabot-policy: 36/36 pass; npm run lint:dependabot-policy passed.
  • npm run lint:md: 0 issues; biome clean; lefthook pre-commit green.

Related

🤖 Generated with Claude Code

…tract

Register ci-workflows pr-automerge-dependabot.yml at v0.35.0 (69e506b) with
runner-input routing, the runner input only, no secrets, and contents/
pull-requests write as its allowedCallerPermissions map. Add a lockstep
check that fails when the reusable's PUBLISHER_ALLOWLIST at the pinned SHA
differs from dependabot-policy autoMerge.publisherAllowlist, run in the
dependabot-policy CI job.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 4, 2026 14:53
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-04T14:57:37.700806Z 0e7e997 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

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


Security review complete

  • Gather context (PR diff, changed files)
  • Review components/dependabot-policy/automerge-lockstep.mjs (new file, fetches remote content over GitHub API)
  • Review components/runner-policy/policy.json contract entry (permissions/secrets)
  • Review .github/workflows/ci.yml change
  • Review components/runner-policy/README.md / package.json changes

No security issues found.

Scope checked (diff vs origin/main, 5 files, +94/-0):

  • components/dependabot-policy/automerge-lockstep.mjs — new script. The fetch URL (CI_WORKFLOWS_SHA, line 15) is a hardcoded, pinned 40-hex SHA with no attacker-controlled input, so there's no SSRF/URL-injection surface. The GITHUB_TOKEN/LOCKSTEP_GITHUB_TOKEN is only sent as a Bearer header to api.github.com to read a public repo's contents — the same transport and token-handling pattern already established in components/pr-convention-policy/lockstep-drift.mjs. YAML parsing uses the yaml package's safe default parser (no custom tag execution), and the subsequent JSON.parse/JSON.stringify comparison has no prototype-pollution or injection path. A malformed response fails closed (non-2xx exits 1; unexpected shape throws and the script exits non-zero) rather than silently passing.
  • .github/workflows/ci.yml — new step added to the existing dependabot-policy job, which keeps permissions: contents: read (matches the workflow-level default, no widening). Trigger is plain pull_request (not pull_request_target), so a fork PR runs this step with GitHub's automatically read-only, secret-less token — no privilege escalation path for untrusted PR code.
  • components/runner-policy/policy.json — registers allowedCallerPermissions (contents: write, pull-requests: write) and allowedSecrets: {} for the pinned ci-workflows reusable. This is a policy-registration/approval record, not a grant executed by any workflow in this repo, and the underlying reusable was already security-reviewed upstream (ci-workflows#676, cited in the PR body). The scope matches what the README documents the reusable actually declares.
  • components/runner-policy/README.md, package.json — documentation and script-alias changes only, no executable logic.

No standing instructions (CLAUDE.md/AGENTS.md/rules files) are deleted, narrowed, or softened by this diff, so the instruction-surface-deletion lens doesn't apply here. GitHub Actions permission/trigger hardening considerations are deferred to zizmor's advisory lane per this lane's scope.

@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

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


Code review complete

  • Gather context (PR diff, CLAUDE.md/REVIEW.md, changed files)
  • Understand the request (invoke /review:code-review)
  • Review changed files for correctness/maintainability
  • Verify candidate findings
  • Post findings / summary

Reviewed the diff (git diff origin/main...HEAD) against the 5 changed files: .github/workflows/ci.yml, components/dependabot-policy/automerge-lockstep.mjs, components/runner-policy/README.md, components/runner-policy/policy.json, package.json. Confirmed dependabot-policy is a direct dependency of ci-status (the repo's single required check) in .github/workflows/ci.yml, and that allowedCallerPermissions (vs. minimumCallerPermissions) is pre-existing runner-policy.mjs machinery, not new logic from this PR.

Posted 2 inline findings on components/dependabot-policy/automerge-lockstep.mjs:

  1. Line 27-32 — the live GitHub API fetch has no retry and no try/catch, unlike the repo's own precedent (pr-convention-policy/lockstep-drift.mjs's fetchWithRetry). Since this step now gates ci-status for every PR, a transient api.github.com 5xx/rate-limit or network blip fails the fleet's required check on the first try, with no hermetic test covering the gap.
  2. Line 15 — CI_WORKFLOWS_SHA is a third independent copy of the pin this PR also writes into runner-policy/policy.json and runner-policy/README.md, synced only by a code comment. A future contract bump to runner-policy/policy.json that forgets this constant leaves the lockstep check silently validating a stale, no-longer-approved SHA.

Everything else checked out: the policy.json contract entry matches the allowedCallerPermissions write-scope rules in runner-policy.mjs, the new ci.yml step correctly inherits contents: read from the existing job, and the README additions are consistent with the policy.json fields they describe. Author-claimed verification (test/lint runs in the PR body) was not independently re-run.

@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: 0e7e997ff2

ℹ️ 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/ci.yml
Comment thread components/runner-policy/README.md
Comment thread components/dependabot-policy/automerge-lockstep.mjs Outdated
Comment thread components/dependabot-policy/automerge-lockstep.mjs Outdated
…d retry the fetch

Read the reusable's SHA from its single runner-policy contract key, retry the
contents-API fetch three times with backoff, and record the lockstep in the
component README and threat model, including why it blocks from admission.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

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


Security review complete

  • Gather context (PR diff, changed files)
  • Review components/dependabot-policy/automerge-lockstep.mjs (new file, fetches remote content, handles tokens)
  • Review .github/workflows/ci.yml change (new step, token exposure, trigger surface)
  • Review components/runner-policy/policy.json contract entry (permissions/secrets)
  • Review doc changes (README.md, THREAT-MODEL.md) for instruction-surface deletions
  • Post findings / summary

No security issues found.

Scope: diff vs origin/main at HEAD 0bbddda, 7 files (+135/-4). This head's latest commit (0bbddda, "derive the lockstep pin from runner-policy and retry the fetch") is a fix-up of the prior commit (0e7e997) already reviewed here; I re-reviewed the full diff rather than just the delta.

  • components/dependabot-policy/automerge-lockstep.mjs — fetches a fixed repo/path (melodic-software/ci-workflows/.github/workflows/pr-automerge-dependabot.yml) at a SHA now read from runner-policy/policy.json's approvedReusableWorkflowContracts key (L15-L24) instead of a hardcoded constant. The SHA is interpolated into the contents-API URL unescaped, but it only ever originates from this repo's own policy.json — content already gated by the runner-policy admission process this same PR goes through — not from any PR-external or runtime-attacker-controlled input, so there's no new trust boundary here versus what a reviewer of policy.json already accepts. The owner/repo/path segment of the URL stays a hardcoded literal (CONTRACT_PREFIX), so a malformed value can't redirect the fetch to a different repository. The token (LOCKSTEP_GITHUB_TOKEN or GITHUB_TOKEN) is sent only as a Bearer header to api.github.com, matching the established pattern in pr-convention-policy/lockstep-drift.mjs. YAML parsing uses the yaml package's safe default parser (no custom tags/aliases executed), and the SHA-pinned fetch target means the compared content is immutable and content-addressed — it can't be swapped without changing the SHA itself, which is the value this check is validating in the first place.
  • .github/workflows/ci.yml — new step in the existing dependabot-policy job; trigger is plain pull_request (not pull_request_target), job permissions stay at contents: read, and GITHUB_TOKEN is the auto-scoped, secret-less token GitHub issues for fork PRs. No privilege escalation or secret-exposure path. GitHub Actions trigger/permission hardening nuances are deferred to zizmor's advisory lane per this lane's scope.
  • components/runner-policy/policy.json — unchanged from the prior review: an approval record (allowedCallerPermissions, allowedSecrets: {}) for an already upstream-security-reviewed reusable (ci-workflows#676), not a grant any workflow in this repo executes.
  • components/dependabot-policy/README.md, components/dependabot-policy/THREAT-MODEL.md, components/runner-policy/README.md, package.json — documentation/script-alias changes. The README/threat-model updates add a new control row and document enforcement that previously didn't exist (replacing "planned consumer" language); they don't delete, narrow, or soften any standing instruction, so the instruction-surface-deletion lens doesn't apply.

Note: the retry/backoff and dynamic-SHA-derivation gaps I flagged on the prior commit (0e7e997) as code-review (not security) findings are both resolved in this commit's fetchText (3 attempts, backoff) and the runnerPolicy.approvedReusableWorkflowContracts lookup — confirmed by reading the current file, not re-reported here since those were maintainability/reliability findings, not security ones.

@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

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


Code review complete

  • Gather context (diff vs origin/main, prior review threads, fix commit 0bbddda)
  • Review components/dependabot-policy/automerge-lockstep.mjs
  • Review ci.yml / README.md / THREAT-MODEL.md rollout documentation changes
  • Post findings

Reviewed the diff from origin/main to 0bbddda (7 files, +135/-4). Fix commit 0bbddda lands on top of 0e7e997, which already carried two rounds of prior review.

Both prior findings on automerge-lockstep.mjs are resolved in this commit:

  • The live fetch now retries 3× with backoff and is wrapped so a transient api.github.com failure doesn't crash the ci-status-blocking step with a raw stack trace.
  • CI_WORKFLOWS_SHA is no longer a third hardcoded copy of the pin — it's now derived at runtime from the single matching key in runner-policy/policy.json's approvedReusableWorkflowContracts, so it can't silently drift from the approved contract.

Codex's two P1 findings are also addressed:

  • ci.yml's new step comment and the dependabot-policy/README.md "Merge lanes" section now explain why this deterministic check can begin blocking with no report-only baseline period (per component-lifecycle.md#enforcement-rollout).
  • dependabot-policy/README.md and THREAT-MODEL.md no longer describe the auto-merge reusable as "planned" — both now document it as the live enforcement path for autoMerge.publisherAllowlist.

One finding remains, posted inline on automerge-lockstep.mjs:65: the script's parsing/derivation/drift logic is all flat top-level code with nothing exported, so it has zero unit-test coverage — unlike its stated precedent, pr-convention-policy/lockstep-drift.mjs, which exports its parsers for hermetic fixture+mutation testing in lockstep-drift.test.mjs. npm run test:dependabot-policy only runs dependabot-policy.test.mjs and never touches this file, so a logic regression here (e.g. the pins.length/constants.length guards, or the YAML job/step walk) would only surface the next time CI makes a live call against the required ci-status path.

Everything else (runner-policy/policy.json contract entry, runner-policy/README.md ledger note, package.json script alias) is unchanged from the already-reviewed 0e7e997 and still checks out.

Author-claimed verification (test/lint runs in the PR description) was not independently re-run.
· branch feat/contract-pr-automerge-dependabot

Comment thread components/dependabot-policy/automerge-lockstep.mjs Outdated
…drift logic

Export the SHA resolution, allowlist extraction and comparison as pure
functions, keep the fetch in a thin CLI main, and add hermetic tests run by
test:dependabot-policy: match, added/removed/reordered drift, zero or two
PUBLISHER_ALLOWLIST entries, zero or two contract keys, malformed YAML and
non-JSON constants.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

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


Code review complete

  • Gather context (diff vs origin/main, prior review threads on commits 0e7e997/0bbddda)
  • Understand the request (/review:code-review on HEAD f6a2dfd)
  • Review changed files for correctness/maintainability
  • Verify candidate findings
  • Post findings / summary

Reviewed the diff from origin/main to f6a2dfd (8 files, +266/-5). This head's only new commit over the previously-reviewed 0bbddda is f6a2dfd, which refactors automerge-lockstep.mjs's parse/derive/compare logic into exported, pure functions (resolveApprovedSha, extractPublisherAllowlist, checkLockstep) and adds automerge-lockstep.test.mjs.

This resolves the one finding still open from the prior round: the script's drift logic is no longer untested top-level code — automerge-lockstep.test.mjs hermetically covers matching/added/removed/reordered allowlists, zero/multiple PUBLISHER_ALLOWLIST constants, malformed YAML, non-JSON constant values, and the single/zero/double runner-policy contract-key cases, mirroring lockstep-drift.test.mjs's fixture pattern. package.json's test:dependabot-policy now runs both files, so this is wired into the existing CI step rather than orphaned.

One finding from the refactor itself, posted inline on automerge-lockstep.mjs:68: pulling the drift check out into a pure checkLockstep(dependabotPolicy, workflowText) dropped the fetched SHA from both the drift-mismatch message (line 68) and the "found N PUBLISHER_ALLOWLIST" message in extractPublisherAllowlist (line 52). Before this commit the messages included ${REUSABLE_PATH}@${sha}; now neither function receives sha, so a real drift failure in the ci-status-blocking CI step reports that the allowlists differ but not which ci-workflows revision was fetched — and the one line that does print the SHA (the success log) never runs on that path. Minor, but worth threading sha back through before merge since this is exactly the failure mode the check exists to catch.

Everything else (ci.yml, runner-policy/policy.json and README.md, dependabot-policy/README.md and THREAT-MODEL.md, package.json) is unchanged from the already-reviewed 0bbddda and still checks out — no new findings there.

Author-claimed verification (test/lint runs in the PR description) was not independently re-run; this sandbox's Bash tool declined to run node --test in this session.
· branch feat/contract-pr-automerge-dependabot

@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

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


Security review complete

  • Gather context (PR diff at HEAD f6a2dfd, changed files)
  • Review components/dependabot-policy/automerge-lockstep.mjs (updated: retry, SHA derivation)
  • Review components/dependabot-policy/automerge-lockstep.test.mjs (new)
  • Review .github/workflows/ci.yml, policy.json, README/THREAT-MODEL changes
  • Post findings / summary

No security issues found.

Scope: full diff vs origin/main at HEAD f6a2dfd (8 files, +266/-5). This head adds a test file and doc edits on top of the already-reviewed 0bbddda; re-reviewed the complete diff rather than just the delta.

  • components/dependabot-policy/automerge-lockstep.mjs — the SHA used to build the GitHub contents-API URL (L33-L41, resolveApprovedSha) comes from a runner-policy/policy.json contract key. Confirmed in runner-policy.mjs (line 176) that every such key is schema-validated to be "pinned to a full 40-character SHA" before it's ever accepted into that map, so the string interpolated into the fetch URL (L93-L95) can't be a path-traversal or query-injection payload — it's always 40 hex characters. The owner/repo/path segment stays a hardcoded literal (REUSABLE_PATH/CONTRACT_PREFIX). Token handling (LOCKSTEP_GITHUB_TOKEN or GITHUB_TOKEN, sent only as a Bearer header to api.github.com) and YAML parsing (the yaml package's safe default parser, no custom tags) are unchanged from the prior review round and still check out. Fetch failures and parse/shape mismatches all throw LockstepError and exit non-zero (fail-closed), never silently pass.
  • components/dependabot-policy/automerge-lockstep.test.mjs — new hermetic fixture tests for the exported pure functions, no network calls, no secrets. Addresses the test-coverage gap from the prior code-review round; nothing security-relevant here.
  • .github/workflows/ci.yml — new step in the existing dependabot-policy job. Confirmed the job keeps permissions: contents: read (no widening) and the workflow's pull_request trigger (not pull_request_target), so a fork PR runs this step with the auto-scoped, secret-less token only. Checkout uses persist-credentials: false.
  • components/runner-policy/policy.json — unchanged approval-record entry from prior rounds (allowedCallerPermissions, allowedSecrets: {}) for an already upstream-security-reviewed reusable (ci-workflows#676).
  • components/dependabot-policy/README.md, THREAT-MODEL.md, components/runner-policy/README.md, package.json — documentation and a script-alias addition only. These are additive (a new threat-model row, a "planned" → "live" status update, a new npm script); nothing is deleted, narrowed, or softened, so the instruction-surface-deletion lens doesn't apply.

Both prior code-review findings on this file (no-retry fetch, hardcoded/duplicated SHA) and the test-coverage gap are resolved in this final diff; no new security-relevant issue was introduced resolving them. GitHub Actions permission/trigger hardening nuances remain deferred to zizmor's advisory lane per this lane's scope.
· branch feat/contract-pr-automerge-dependabot

Comment thread components/dependabot-policy/automerge-lockstep.mjs
… failures

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

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


Security review complete

  • Gather context (full PR diff vs origin/main, 8 changed files)
  • Check skip gate — this head (9a956bc) is a trivial follow-up to f6a2dfd, which already had a clean security review; re-reviewed the full diff anyway
  • Review components/dependabot-policy/automerge-lockstep.mjs at HEAD, including the new commit
  • Review remaining changed files (ci.yml, runner-policy/policy.json, runner-policy/README.md, dependabot-policy/README.md and THREAT-MODEL.md, package.json)
  • Check for instruction-surface deletions (CLAUDE.md/AGENTS.md/rules/skill/agent/hook)
  • Post findings / summary

No security issues found.

Scope: full diff vs origin/main (8 files, +267/-5), via gh pr diff 687.

What changed since the last reviewed head (f6a2dfd): one commit, 9a956bc — purely cosmetic. It hoists let sha; above the try block and appends (checked at ${sha}) to the failure message so a drift/fetch error names the ci-workflows revision that was fetched. No new code path, no change to the fetch, parsing, or comparison logic. No security implication.

Re-verified across the full diff (not just the delta):

  • automerge-lockstep.mjs — the SHA used to build the GitHub contents-API URL is read from runner-policy/policy.json's approvedReusableWorkflowContracts key (resolveApprovedSha, L97-L107). Confirmed independently in components/runner-policy/runner-policy.mjs:67,176 (FULL_SHA = /^[0-9a-f]{40}$/i) that every such key is schema-validated to a full 40-hex SHA before lint:runner-policy (which gates ci-status) passes — so the string interpolated into the fetch URL can't carry path-traversal or query-injection payloads; the owner/repo/path segment stays a hardcoded literal (REUSABLE_PATH/CONTRACT_PREFIX). YAML parsing uses the yaml package's safe default parser (no custom tags/aliases). The token (LOCKSTEP_GITHUB_TOKEN or GITHUB_TOKEN) is sent only as a Bearer header to api.github.com. All fetch/parse/shape failures throw LockstepError and exit non-zero (fail-closed).
  • .github/workflows/ci.yml:349-358 — new step in the existing dependabot-policy job. Verified directly: the job keeps permissions: contents: read (no widening), trigger is plain pull_request (not pull_request_target), checkout uses persist-credentials: false, and the step's GITHUB_TOKEN is the auto-scoped, secret-less token for fork PRs — matches the job's own read-only scope, no escalation. Actions trigger/permission hardening nuances beyond this are zizmor's advisory lane.
  • components/runner-policy/policy.json:1458-1467 — an approval record (allowedCallerPermissions, allowedSecrets: {}) for a reusable already security-reviewed upstream (ci-workflows#676); not a grant any workflow in this repo executes.
  • components/dependabot-policy/README.md, THREAT-MODEL.md, components/runner-policy/README.md, package.json — documentation and a script-alias addition only, no executable logic changes beyond what's already covered above.

Instruction-surface-deletion lens: checked — this PR's own commits don't touch any CLAUDE.md/AGENTS.md/rules file/skill/agent body/hook config; the README/THREAT-MODEL edits are additive (new rows, "planned" → "live" status), so the lens doesn't apply here. (The instruction exception register at docs/conventions/instruction-exception-register/README.md isn't present in this checkout, noted per the skill's fallback instructions, but it's moot since there's no deletion/narrowing to check against it.)

Note: an unrelated, uncommitted local modification to .claude/rules/pr-body-contract.md is present in this checkout's working tree but is not part of PR #687's commits or changed-files list, so it's out of scope for this review.
· branch feat/contract-pr-automerge-dependabot

@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

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


Code review complete

  • Gather context (diff vs branch point f61cad4, prior review threads, HEAD 9a956bc)
  • Review the latest commit (9a956bc) against the prior open finding
  • Review all changed files for new issues
  • Post findings / summary

Note: this sandbox's shallow clone can't reach current origin/main (it has since advanced past this PR's branch point with unrelated commits, and git fetch requires interactive approval unavailable here). Diffed against f61cad4, the PR's actual branch point — git diff f61cad4..HEAD --stat matches the PR's reported changed-files list exactly (8 files, +267/-5), so this is the full PR diff.

Only new change since the last review round: commit 9a956bc fixes the one finding still open after f6a2dfd (drift messages lost the fetched SHA when the parse/compare logic was extracted into pure functions). The fix is minimal and correct: sha is hoisted above the try/catch in the CLI entrypoint (automerge-lockstep.mjs:100-114) and appended as (checked at ${sha}) to any error raised after it resolves. It correctly stays absent when resolveApprovedSha itself throws (no SHA to report yet), and it only touches the thin CLI wrapper — the exported pure functions (resolveApprovedSha, extractPublisherAllowlist, checkLockstep) and their test coverage in automerge-lockstep.test.mjs are untouched. No new issues introduced.

No new findings. This PR has now been through five full review rounds (security ×3, code-review ×4, Codex ×1) across its four commits, and every substantive finding raised along the way was fixed in a subsequent commit:

  • No-retry/no-error-handling fetch → retry with backoff, fail-closed (0bbddda)
  • Hardcoded/triplicated CI_WORKFLOWS_SHA → derived at runtime from the single runner-policy contract key (0bbddda)
  • Missing blocking-gate rollout justification (Codex) → documented in README "Merge lanes" + ci.yml step comment (0bbddda)
  • Stale "planned" doc language (Codex) → reconciled in dependabot-policy/README.md and THREAT-MODEL.md (0bbddda)
  • Untested drift logic → extracted to exported pure functions + hermetic fixture tests (f6a2dfd)
  • SHA dropped from drift error messages → fixed this round (9a956bc)

The remaining files (ci.yml, runner-policy/policy.json, runner-policy/README.md, package.json) are unchanged since the already-reviewed f6a2dfd and still check out: the policy.json contract entry's allowedCallerPermissions matches what the reusable's job actually declares, the new ci.yml step inherits contents: read with no permission widening, and the npm script wiring (lint:dependabot-automerge-lockstep, test:dependabot-policy running both test files) is correct.

Author-claimed verification (test/lint runs in the PR description) was not independently re-run in this session.
· branch feat/contract-pr-automerge-dependabot

@kyle-sexton
kyle-sexton merged commit 598ce36 into main Oct 4, 2026
2 checks passed
@kyle-sexton
kyle-sexton deleted the feat/contract-pr-automerge-dependabot branch October 4, 2026 15:49
kyle-sexton added a commit that referenced this pull request Oct 4, 2026
…#692)

No related issue: Wave 4 of the org GitHub Actions naming-convention
migration (tracked by #672 for the enforcing switch).

## Summary

Hand-migrates standards' own workflows to the org naming convention.
standards is the sync source, so no sync PR delivers this. The eight
`"repo":"standards"` entries of
`components/github-actions-conventions/rename-map.json` are applied, and
every repo-owned ci-workflows pin moves to its renamed path.

## Fix

- Renames (with `git mv`; each `name:` now equals its file stem):
- `ci.yml` → `pr-require-checks.yml`, with all 31 job renames applied to
job keys, the `ci-status` `needs:` list and its `needs.<id>.result`
list. The `ci-status` job id and display name are unchanged.
  - `claude-lanes-repin.yml` → `maintenance-repin-ci-workflows.yml`
  - `claude-review.yml` → `pr-review.yml` (job `pr-review`)
- `claude-security-review.yml` → `pr-review-security.yml` (job
`pr-review-security`; its job-level concurrency group is now
`pr-review-security-*`, matching the component)
- `code-review-rules-inventory.yml` →
`maintenance-audit-code-review-rules.yml` (job `audit`)
- `lychee-private-inventory.yml` →
`maintenance-audit-link-check-excludes.yml` (job `audit`)
  - `publish-packages.yml` → `release-publish-packages.yml`
- `sync.yml` → `maintenance-sync-standards.yml` (job
`maintenance-sync-standards`)
- Repins: every repo-owned `melodic-software/ci-workflows/...@<sha>`
moves to its new path at `7446b51` (`# v0.34.0`). `check-machine-paths`
and `check-comment-markers` go to `69e506b` (`# v0.35.0`). The reusables
have approved runner-policy contracts at these SHAs. Composite input
changes are additive only.
- `.cursor/install.sh`: typos 1.50.1 → 1.50.3 and editorconfig-checker
3.11.2 → 4.0.2, to match the action defaults at `7446b51`.
`.cursor/install.test.sh` enforces this lockstep.
- Repin tooling: `repin-callers.sh` `EXTRA_CALLER_FILES`,
`repin-policy-lockstep.mjs` `REPIN_TARGETS` and the repin workflow's
`add-paths` now name the new paths. The fixtures in
`repin-callers.test.sh` move with them; the upstream `uses:` paths in
those fixtures are unchanged.
- Updated for the new paths: the key in
`.github/concurrency-policy.json`, plus the tests that read the renamed
files (`managed-files-guard.test.sh`, `package-lifecycle.test.mjs`,
`.cursor/install.test.sh`) and the current-state docs.
- Dependabot: drops the `melodic-software/*` cooldown exclude and marks
the single ci-workflows ignore as temporary until the migration
close-out sweep.
- New `pr-automerge-dependabot.yml`: a thin caller at `69e506b` (`#
v0.35.0`) on `ubuntu-24.04`, using the contract from #687.

After merge, the advisory security check context changes from
`security-review / security-review` to `pr-review-security /
security-review`. `ci-status` stays the only required check.

## Verification

- `naming-lint --mode enforcing`: 0 blocking findings. One advisory
warning remains: `job-id-verb-first` on `github-actions-conventions`,
which the rename map does not rename.
- Passing npm scripts: the `lint:*` scripts other than the pr-convention
lockstep, and every `test:*` script.
- `lint:runner-policy` passes with `CI_REPOSITORY_VISIBILITY=public`.
- Passing shell suites: `repin-callers.test.sh`, `claude-lanes.test.sh`,
`managed-files-guard.test.sh`, `.cursor/install.test.sh`, pin-comment
scan, `sync-manifest.sh validate`, `repin-policy-lockstep.test.mjs`.
- `actionlint -config-file .github/actionlint.yaml`: clean.
- zizmor (regular persona, offline): 0 high findings. The lane gates on
high.
- editorconfig-checker 4.0.2 and typos 1.50.3 pass on this repository
with the CI excludes.
- github-iac origin/main `checkFile` on `pr-require-checks.yml`, using
standards' 31 `jobRenames`: 0 failures across 39 gated jobs. The same
check against main's old `ci.yml` reports 29 failures, which shows the
check catches unrenamed jobs.

## Related

- standards#671
- standards#686 (relocated the managed callers)
- standards#687 (pr-automerge-dependabot contract)
- melodic-software/github-iac#607 (per-consumer jobRenames)

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

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