Repository navigation
feat(runner-policy): admit pr-number and timeout-minutes on the claude-review caller contract - #521
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @kyle-sexton's task in 4m 0s —— View job Code review — PR #521
SummaryThis is a clean, minimal, well-scoped change: Checked against
One non-blocking note posted inline on policy.json:878: I couldn't reach Verification claims I could not independently reproduce in this sandbox: |
![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: c7ff6446e7
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
kyle-sexton
added a commit
that referenced
this pull request
Sep 3, 2026
Review on #521: the README's contract review record said both lane contracts copied forward unchanged at v0.14.0 and again at v0.14.2. A dated amendment now states that the review lane at 62bef7ba, and only there, admits pr-number and timeout-minutes, why, and that v0.14.2 and later stay at runner only until a repin review re-approves them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…e-review caller contract The claude-review reusable at 62bef7ba (ci-workflows v0.14.0) declares pr-number and timeout-minutes inputs, but its approved caller contract admitted only runner. The CI/CD performance plan's once-per-PR review callers pass timeout-minutes and, on workflow_dispatch, pr-number (github-iac docs/topics/ci-perf/PLAN.md, Phase 2.4). The component suite passes 274 of 274 with the widened contract. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review on #521: the README's contract review record said both lane contracts copied forward unchanged at v0.14.0 and again at v0.14.2. A dated amendment now states that the review lane at 62bef7ba, and only there, admits pr-number and timeout-minutes, why, and that v0.14.2 and later stay at runner only until a repin review re-approves them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kyle-sexton
force-pushed
the
ci-perf/runner-policy-claude-review-inputs
branch
from
September 3, 2026 23:03
f79154a to
a169ea3
Compare
kyle-sexton
added a commit
to melodic-software/claude-code-plugins
that referenced
this pull request
Sep 4, 2026
…re-running on every push (#3696) No related issue: Phase 2 of melodic-software/github-iac#385 (cross-repository) Refs melodic-software/github-iac#378 Refs melodic-software/github-iac#385 ## Summary `ci.yml` ran about fifty-five jobs. Each paid for its own runner, its own checkout and its own toolchain install so it could run a few seconds of work, and every one of them ran on every pull request whatever the diff touched. Meanwhile both AI review lanes re-ran on every push to an open branch, so an active branch bought a full LLM code review and a full LLM security review per commit, most of them superseded before they finished. This collapses `ci.yml` into six jobs behind one resolver, narrows what each lane runs to what the diff can actually affect, and stops both review lanes re-running on push and running on drafts at all. Phase 2 of the CI/CD performance program. ## Fix **Six jobs.** `changes`, `lint`, `test-linux`, `test-windows`, `hook-utils`, `ci-status`. Every former lane is a step of one of the four working jobs and keeps the name it had, so a check is still findable by the name it always had. `yq '.jobs | keys | length'` prints `6`. **One resolver publishing a table of seven outputs.** `run_full` is the root, derived from the docs-only detector. The other six narrow it by the pull request's draft state and by the ci-workflows `change-detection` action's filter groups: `shell`, `node`, `python`, `powershell`, `docs` and `workflows`. Every narrowing clause compares a group against `'false'` and never `'true'`, so an unset group runs its lane, which is the action's own documented rule and the same fail-open direction the detector already takes. Every filter group repeats the toolchain and configuration paths, so a pull request touching only a lockfile, only a workflow file or only the ShellCheck configuration runs every lane rather than none. `docs` and `powershell` are computed and read by nothing today; they exist so the group set matches the program plan and a consumer can be added without touching the resolver. **One job-level gate, outside the required-lane closure.** `test-windows` carries `if: needs.changes.outputs.run_windows == 'true'` and a `# lane-coverage-ok:` annotation, and it is deliberately not in `ci-status.needs`. A lane the aggregate cannot see costs a skipped informational run rather than a false green; inside the closure the same condition would let a lane report success having run nothing, and both topology gates reject it there. Every other gate in the file is at step level. **The repin: sixteen of seventeen.** Sixteen of this file's seventeen ci-workflows references now resolve to `85e140fa2fe9ebe96c41156055e7676de208090a`, the `v0.19.0` tag cut after the ShellCheck fan-out merged. Before this, `ci.yml` alone carried three ci-workflows revisions: the composite actions at `c2654182`, the gitleaks action at `f2d5e067`, and the zizmor reusable at `31a5b76c`. One SHA per repository was never the status quo. Between `c2654182` and the wave tag the action surface moved by 272 commits, 51 action files, and +3257 and -353 lines; the `change-detection` action this rewrite needs does not exist at `c2654182` at all. **The seventeenth is held at the old SHA, deliberately.** `machine-specific-paths` stays at `c2654182`. At the wave tag it detects five path classes, and on this repository all five fire on content that is not a machine-specific path: 40 hits across 22 files in ten plugins, every one of them prose naming the form the surrounding code parses, a published CHANGELOG entry, or a fixture whose assertion subject IS the path. Three of those categories cannot be reworded. Released history is released, one flagged changelog entry is itself documenting a previous fix to this very detector and necessarily quotes the forms it began catching, and changing a fixture's literal changes what the test proves. The action's only lever is a file-level exclude, so silencing 40 phrases would take 22 files and ten plugins out of the scan entirely. melodic-software/ci-workflows#549 carries the detection-precision report upstream and #3699 tracks repinning this one line once that lands. Other workflow files in this repository still carry other ci-workflows SHAs. Converging them is Phase 6b's work, where the program plan verifies its acceptance criterion per wave, and widening the repin here would mix a dependency bump into a performance change. **Phase 6b's input set now includes this held reference too**, alongside the eight in other files. **The zizmor correction.** The program's decision 8 specified a hash-pinned PyPI wheel at `zizmor==1.29.0`, "the version the reusable pins". Both halves were wrong. The reusable downloads a native release archive (`zizmor-x86_64-unknown-linux-gnu.tar.gz`) verified against a SHA-256 input; there is no PyPI involvement. And this repository passes no `version` input, so it takes the reviewed default at its pinned SHA `31a5b76c`, which is **v1.27.0**. 1.29.0 was a worker's local binary version. The wheel pinned here is therefore `zizmor==1.27.0`, run with the reusable's own persona and with `GH_TOKEN` from the job's read-only workflow token for the online audits, so the step is a like-for-like replacement and no security-lane behaviour changes inside a performance change. One consequence, stated because it is real: like the existing `pyyaml` pin this is a platform-specific wheel, so a host outside `manylinux_2_28_x86_64` cannot install from `.github/requirements-ci.txt` under `--require-hashes`. **The draft filter and its event guard.** Both AI review callers now trigger on `opened`, `ready_for_review` and `reopened`, and both skip drafts. `claude-review.yml`'s condition is `github.event_name == 'workflow_dispatch' || github.event.pull_request.draft == false`: the event clause is load-bearing, because a dispatched run carries no `github.event.pull_request` and a bare draft comparison would never run the review an operator asked for. That caller also gains a `workflow_dispatch` trigger with a `pr-number` input and a fifteen-minute timeout, and its header now names the three re-review paths the reusable documents and no others: reopen, flip to draft and back to ready, or dispatch with the number. `@claude` is not wired here and the header says so. Dropping the push trigger cannot block a merge, and the live ruleset is the evidence: the required status-check set on `main` is `ci-status`, `do-not-merge / do-not-merge`, `pr-issue-linkage / pr-issue-linkage` and `pr-title / pr-title`, under three active organization rulesets (`base`, `ci-gate`, `signing`). ADR 0002 lines 202-205 say otherwise and are stale on three counts: they name `security-review / security-review` as required via a `security-review-gate` ruleset that is not active, they omit `pr-issue-linkage / pr-issue-linkage` which is required, and the `OrganizationAdmin` bypass they attribute to that ruleset therefore does not apply. The program plan already schedules the ADR 0002 amendment for Phase 9, so that sentence is flagged stale here rather than edited inside a performance change. If `security-review-gate` is ever re-enabled, the caller regains its push trigger in that same change. **`--jobs 3`, not 4.** The contract corpus runs three suites at a time. The runner has four vCPUs and the suites are spawn-bound, so four looked like the shape that pays, and it is faster, but at four three separate suites failed across two runs by producing empty output from an external command on a path with no clock in it. Serialising each one in turn only moves the symptom to the next suite, so the job count is the lever rather than the allowlist. Measured: 572 s serial, 312 s green at three, two red runs at four. See #3694. | Setting | Run | Step wall time | Result | |---|---|---|---| | serial | 33656901629 | 572 s | green | | `--jobs 4` | 33787030286 | 228 s | red | | `--jobs 4` | 33794273916 | 240 s | red | | `--jobs 3` | 33797359110 | 312 s | green | **Two departures from the written specification, both forced.** A step cannot carry both a resolver gate and an event gate. The docs-only gate reads a step's condition whole, so `needs.changes.outputs.X == 'true' && github.event_name == 'pull_request'` is an unsanctioned consumer form; and a step gated on the event alone is `skipped` on a push, which the fail-closed aggregator turns red. Every diff-scoped gate therefore runs on each event and returns early from its own script when there is no base ref to diff against. The stale-base overlap detector runs in a detached worktree at the pull request head. On a `pull_request` event this job checks out the synthetic merge commit, whose merge-base with the target IS the target tip, so the detector would have reported every pull request fresh and proved nothing. Its own former job carried `ref: head.sha` for exactly that reason. **Deleted with its lane:** `scripts/check-rename-sweep.sh` and its suite. The gate ledger rules them a permanent lane for a finished rename's residue, and nothing else in the tree names either file. **Also:** `dependabot-miro-bundle.yml` gains a path filter on the two miro manifests and a per-pull-request concurrency group, and `AGENTS.md` gains the instruction that pays for the draft filter: open a pull request as a draft and flip it to ready when the work is done. ## Verification Every command below was run locally on the branch. | Command | Result | |---|---| | `bash scripts/check-docs-only-gate.sh --check` | exit 0: scope resolved once in `changes`; 91 references across 4 consumer jobs, all sanctioned | | `bash scripts/check-docs-only-gate.test.sh` | 57 of 57 `ok`, 0 `not ok`, including the live-file case that was red before this rewrite | | `bash scripts/check-lane-coverage.sh --check` | exit 0: all 4 lanes reachable from `ci-status.needs` | | `bash scripts/check-lane-coverage.test.sh` | ALL PASS, including its live-file case | | `yq '.jobs \| keys \| length' .github/workflows/ci.yml` | `6` | | `yq '.on.pull_request.types' .github/workflows/ci.yml` | `[opened, synchronize, reopened, ready_for_review]` | | `actionlint -config-file .github/actionlint.yaml` on all four changed workflows | exit 0, clean | | `zizmor --persona regular` on all four changed workflows | no findings (7, 3, 3 and 2 suppressed) | | `typos` on every changed file | clean | | `markdownlint-cli2 AGENTS.md` | 0 issues | | `grep -c "synchronize"` on both review callers | `0` and `0` | | `grep -c "timeout-minutes: 15" .github/workflows/claude-review.yml` | `1` | | `grep -c "draft == false"` on both review callers | `2` and `3` | | `grep -c "affected-tests.sh --run" .github/workflows/ci.yml` | `1` | | `bash scripts/check-changelog-parity.sh` `--check`, `--check-bump`, `--check-preserved`, `--check-order` | all exit 0 | | `bash scripts/check-contract-slice-prune.sh` `--check` and `--check-diff origin/main` | both exit 0 | | `bash scripts/check-vendor-version-bump.sh --check-bump origin/main` | exit 0 | | `bash scripts/check-orphaned-fixtures.sh --check`, `check-fixture-git-isolation.sh --check` | both exit 0 | | `bash scripts/check-hook-wiring-liveness.sh`, `check-silent-skips.sh` | both exit 0 | | `bash scripts/check-plugin-manifest-presence.sh`, `check-plugin-catalog-enablement.sh` | both exit 0 | | `bash scripts/check-skill-leaf-names.sh --check`, `check-loop-lane-floor-drift.sh --check` | both exit 0 | | `bash scripts/affected-tests.sh --base origin/main` | exit 0; both deleted files reported as covered by nothing surviving | | Machine cross-check: every `continue-on-error` step in `lint` against the `CHECK_RESULTS` feed | 54 steps, 54 feed rows, no unfed step, no unpaired row, no duplicate | | The zizmor wheel hash, re-derived from the PyPI JSON API | `zizmor-1.27.0-py3-none-manylinux_2_28_x86_64.whl` `sha256:afb28123882d2b8248f1e480bf6cc6d1af102e0d3fbe22a40f7f795b1aa9d435` | Two probe pull requests exercised the filter groups against this branch and are now closed unmerged: #3697 (lockfile-only, opened as a draft and flipped to ready) and #3698 (workflow-only). Four real runs; their per-lane verdicts are in the comment below. Three properties only a run could establish: **The filter groups matched for real, not fail-open.** This is the property the two probes cannot distinguish on their own, because the action fails OPEN to relevant: a rejected pattern, a parse warning or an API hiccup produces the same all-`true` result a correct match does. Run `33808374809`'s match step settles it, printing `FILES_LIST_FAILED: false` and then six named groups all `true`, with no warning, `outcome=success` in 59 ms. So the group names the resolver looks up through `fromJSON` are the names the action actually published. **The detached worktree reads the head, not the merge commit.** The stale-base step's log reads `Preparing worktree (detached HEAD b9996c5)`, which is #3698's own head commit. Both probe heads were genuinely fresh, so the detector's verdict proves nothing either way; the checked-out SHA is what proves it is looking at the right tree. **Wall time, against the 25-minute timeout and the program's 12-minute p95 target.** | Run | `lint` | `test-linux` | `test-windows` | |---|---|---|---| | `33808259580` (this PR, draft) | 5.3 min | 0.2 min | skipped | | `33808363829` (#3697, draft) | 4.5 min | 0.2 min | skipped | | `33809595757` (#3697, ready) | — | ran the contract suite | ran | | `33808374809` (#3698, ready) | 5.6 min | 1.6 min | 1.4 min | `lint` is the critical path on every pull request including a docs-only one, and it is serial now. At 4.5 to 5.6 minutes the p95 target is not unreachable by construction. The affected-suite selector was exercised on both sides: `No suites selected` on #3698's lone workflow file, and a non-empty run on #3697's ready-flip ending `All 6 selected suites passed or were skipped.` **Both previously-red rows are resolved.** `runner-policy` was red because `.github/standards/runner-policy/policy.json` gave the claude-review reusable at `62bef7ba` an `allowedInputs` of exactly `["runner"]`, and the review-trigger change above adds `pr-number` and `timeout-minutes`. That file is standards-distributed, so under ADR-0007 the fix was a change in `melodic-software/standards` (#521) and its sync back here (#3701, merged as `557e1ab01`); this branch is rebased onto it and `runner-policy.mjs` now passes. `machine-specific-paths` was red because of the repin, and is resolved by holding that one action at its old SHA, above. Security pass on every trigger and permission change: `pull-requests: read` is granted on `changes` alone, which is the only job that needs it (the change-detection action lists the pull request's files through the API); no `pull_request_target` anywhere; no secret is reachable from a fork context; `actionlint` and `zizmor --persona regular` are clean on every changed workflow; and `claude-review.yml` keeps its repo-wide `queue: max` seat group unchanged, with no second concurrency block added. One finding from that pass, recorded rather than silently accepted: the security lane's required check certifies execution, and the reusable's own canonical caller keeps the push trigger for that lane specifically. Without it the required check can certify a head the branch has since moved past. The program plan rules the trigger cut deliberate; the recovery path is to flip the pull request to draft and back to ready, and that is written into the caller's header rather than left for a reader to discover. ## Related - Phase 2 of melodic-software/github-iac#385, itself a sub-issue of melodic-software/github-iac#378. - Sub-topic plan: melodic-software/github-iac#391, merged as `02ac028`, with the follow-up in melodic-software/github-iac#393. - PR A, the script half of Phase 2: #3693, merged as `78349aeb3`. - PR B, the ci-workflows ShellCheck fan-out: melodic-software/ci-workflows#547, merged as `85e140f` and cut as `v0.19.0`. - #3694 holds the unexplained parallel-suite failures at `--jobs 4`. - #3699 tracks repinning `machine-specific-paths` to the wave tag once ci-workflows#549 fixes its detection precision. - #3703 tracks runners for the Node suites outside the four sub-projects and for the Pester suites, which no step executes on either the pull-request or the push path. The contract-suite step names them in a warning and counts them in the run summary rather than reporting them as executed. - melodic-software/standards#521 adds `pr-number` and `timeout-minutes` to the claude-review reviewed contract; its sync into this repository has to merge before the `runner-policy` row goes green. - A cross-repository closing keyword does nothing on GitHub, so melodic-software/github-iac#385 is closed by hand after this merges. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No related issue: CI/CD performance program, Phase 2.4 (melodic-software/github-iac#385, program melodic-software/github-iac#378).
Summary
The approved caller contract for the ci-workflows
claude-review.ymlreusable at62bef7ba(v0.14.0) admits only therunnerinput, but the reusable declarespr-numberandtimeout-minutes. claude-code-plugins' once-per-PR review caller (melodic-software/claude-code-plugins#3696, Phase 2 of the CI/CD performance program) passestimeout-minutes: 15and, onworkflow_dispatch,pr-number, so the runner-policy lane rejects the call with "inputs absent from its reviewed contract: pr-number, timeout-minutes".Fix
components/runner-policy/policy.json: theapprovedReusableWorkflowContractsentry forclaude-review.yml@62bef7bab01e8532fedfa739879034a210e9e67dgainspr-numberandtimeout-minutesinallowedInputs; routing, secrets and caller permissions are unchanged. Both inputs exist on the reusable at that SHA (workflow_call.inputs.timeout-minutesandworkflow_call.inputs.pr-number).Verification
node --test components/runner-policy/runner-policy.test.mjs: 274 passed, 0 failed.npm run lint:runner-policy: "Runner policy passed."lintjob,runner-policyrow on its three probe runs.Related
docs/topics/ci-perf/PLAN.mdPhase 2.4 and Phase 3b (the wider runner-policy grammar change, which this does not pre-empt).🤖 Generated with Claude Code