fix: preserve caller runner for failed prerequisites - #207
Conversation
|
Claude finished @kyle-sexton's task in 2m 18s —— View job Code Review
SummaryThe core change is a clean, well-reasoned simplification: three job-level A few observations follow — one worth addressing before merge, the rest informational. Findings1. Empty-runner regression on the success path (low risk, behavioral change worth documenting)Old expression — New expression — This won't affect callers using the documented
2. Comment-prose assertions couple tests to comment text (maintainability concern)
assert.match(
jobContract,
/Always honor the caller-selected runner,[\s\S]*?failed, skipped,[\s\S]*?...fails closed before title validation/u,
);This is a tight coupling that forces a test update whenever the comment is rephrased. The structural assertions in 3.
|
|
Claude finished @kyle-sexton's task in 2m 9s —— View job Security Review
No security issues found in the changed files. The six files changed are three reusable workflow definitions, two test files, and the README. Here is what was checked:
The core change replaces Callers of The previous conditional guaranteed a Context injection surfaces — unchanged by this PR, no new risks
Permissions — unchanged ( Action pins — this PR does not modify any Trigger events — no changes; all three remain Test files ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7da36e470
ℹ️ 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 documented private-caller fallback reused the public `outputs.runner || 'label'` form, but on a strict-selector failure `select-runner` publishes the non-empty unroutable sentinel `ci-runner-selection-failed`. A `||` fallback never replaces a non-empty value, so the sentinel reached `runs-on` and the required reporter job could not start — defeating fail-closed reporting. Gate the fallback on `needs.select-runner.result == 'success'` so the sentinel is ignored, and note the fallback label must itself be routable when selection fails. Update the README-coupled test assertion to track the corrected guidance. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 57s —— View job Code Review
On the Codex Review CommentThe Codex comment flags that a private This is already addressed in commit
The correct pattern is documented immediately below (README.md:346): runner: ${{ needs.select-runner.result == 'success' && needs.select-runner.outputs.runner || 'melodic-ubuntu-24.04-x64' }}
Remaining Gap: Input Description Lacks Non-Empty Guard WarningThe old
The new expression The updated description says "Direct callers that omit it use the public hosted default," but omitting the field and passing an empty expression are different. A one-sentence addition would close this:
What's Good
VerdictThe change is correct and the Codex concern is already addressed. The one remaining item worth a before-merge look is the input description — a single sentence noting that callers must supply a non-empty expression would document the implicit guard that was removed and prevent a subtle foot-gun for callers who rely on an expression that could evaluate to empty. |
|
Claude finished @kyle-sexton's task in 2m 7s —— View job Security Review
No security issues found in the changed files. The six changed files are three reusable workflow definitions, two test files, and the README. Here is what was checked:
The core change replaces the conditional Fail-closed semantics — unchanged The "Reject failed prerequisite" step ( Context injection surfaces — no new risks The All three workflows continue the same safe
Permissions — unchanged ( Trigger events — no changes; all three remain Action SHA pins — this PR does not modify any
Test files — pure static text analysis over YAML source files; no network access, no secrets, no |
|
Per-finding classification for the code-review comment above (reacted 👍 — finding #1 acted on):
|
The `runner` input description covered callers that omit the field (they get the hosted default) but not callers passing a dynamic expression. With the reusable-side empty-string fallback removed, an expression that resolves to empty now fails runner allocation rather than defaulting. Add that caveat to the description of all three prerequisite gates (semantic-pr, do-not-merge-gate, pr-issue-linkage). Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 26s —— View job Code Review
On the Three CommitsThis review covers the final three-commit state of the PR.
Finding #1 from Previous Reviews: Fully AddressedThe empty-runner concern is now documented where it matters most — on the input itself. All three workflows carry the same three-sentence description:
This precisely covers the three distinct caller scenarios: (a) field omitted →
Test Changes in
|
| Regex token | Comment text |
|---|---|
Always honor the caller-selected runner, |
✓ exact |
failed, skipped, |
✓ |
cancelled, or empty prerequisite result |
✓ |
caller owns any recovery |
✓ |
public \ubuntu-24.04` default` |
✓ |
caller's \if: always()`` |
✓ |
fails closed before title validation |
✓ |
README assertion — Both updated matchers verify against the new README prose. First assert checks the runner-unchanged-for-all-outcomes sentence; second chains through the ubuntu-24.04 fallback statement, the sentinel warning, and the result == 'success' gate expression. All confirmed present in the README diff.
runnerFor and dispatch("") — The helper is correctly simplified to (_result, selectedRunner = "ubuntu-24.04") => selectedRunner (identity on the selected runner), and dispatch("") was added to verify that an empty prerequisite result is fail-closed. Both changes accurately model the new contract.
Step-name assertions in hosted-runner-pins.test.cjs — verified against the actual workflow files:
"Reject failed prerequisite"→semantic-pr.yml:99,do-not-merge-gate.yml:50,pr-issue-linkage.yml:70✓"Validate PR title against Conventional Commits"→semantic-pr.yml:107✓"Reject a blocking label"→do-not-merge-gate.yml:58✓"Validate PR body against the closing-keyword + Related convention"→pr-issue-linkage.yml:78✓
One Minor Style Nit (non-blocking)
In hosted-runner-pins.test.cjs, the two loop-invariant assertions:
assert.equal(runnerFor("failure"), "ubuntu-24.04");
assert.equal(mustReject("success"), false);are inside the for...of prerequisiteGateContracts loop, so they execute three times rather than once. They're correct and still fast; worth hoisting out if someone edits the file later, but no action required here.
hosted-runner-pins.test.cjs:123-126
What's Good
- All prior findings resolved. Finding feat/phase1 markdown powershell #1 (empty-runner regression) is addressed in
3f7d299with precise three-sentence input descriptions. The Codex sentinel concern is addressed in68c5546with README docs locked by test assertion. - Input descriptions are the right place for the contract. Documenting the no-implicit-fallback behavior on the
runnerinput ensures it's visible to every consumer at the point of use, independent of README navigation. - Test coverage is comprehensive and cross-workflow. The
prerequisiteGateContractsloop inhosted-runner-pins.test.cjsasserts the exactruns-onexpression, absence of the old conditional, step ordering (reject before validation), and reject-step internals for all three reusables in one test. Adding a fourth contract requires only a new array entry. - CLAUDE.md compliance. All
uses:remain SHA-pinned with version comments. Triggers, permissions, and thepull_request_targettripwire are untouched. - README accuracy. The updated prose correctly explains: caller owns the fallback; both the public
||form and the omitted-input default preserveubuntu-24.04; privateself-hosted-onlycallers must gate onresult == 'success'to avoid passing the unroutable sentinel. The[job-runs-on]reference replaces the now-irrelevant[runner-pricing]reference cleanly.
Verdict
Ready to merge. All findings from previous reviews are addressed. The three-commit arc moves the contract cleanly from the old reusable-side fallback to the new caller-owned routing model, and the documentation (input descriptions + README) correctly captures every consumer-relevant nuance.
|
Claude finished @kyle-sexton's task in 1m 33s —— View job Security Review
No security issues found in the six changed files. Threat surface analyzed
The
Context injection surfaces — unchanged, no new risk
Trigger events — unchanged; all three remain Permissions — unchanged; SHA pins — unmodified; Test files and README — static text; no executable paths, no secrets, no network access. |
|
Follow-up on code-review finding #1 (empty-runner / non-empty guard), revisiting the prior classification. The prior reply addressed this via README guidance and left the Fixed in 3f7d299. All three prerequisite-gate reusables ( |
## Summary - register the exact merged `melodic-software/ci-workflows` prerequisite-gate contracts at `380612ae1d4e0cc9741efbac7b6ffb3d3da63a04` - preserve each workflow's reviewed `runner`, `prerequisite-result`, allowed-input, and empty-secret boundaries - lock the production inventory in the runner-policy regression suite and document the strict-selector sentinel caveat ## Why ci-workflows#177 moved `semantic-pr`, `do-not-merge-gate`, and `pr-issue-linkage` to the caller-selected runner for every prerequisite outcome. Private consumers cannot repin to that immutable merge until the canonical standards runner policy recognizes the exact reviewed contracts. The canonical `components/runner-policy/policy.json` is the source of truth. Its managed downstream copies will flow through the normal standards synchronization process; no generated or consumer copy is edited here. ## Validation - focused production-contract test: 1 passed - complete runner-policy suite: 228 passed - runner-policy self-audit: passed - markdownlint: 0 errors - pre-commit: Biome, EditorConfig, markdownlint, typos, and gitleaks passed No linked issue: this PR is the required standards registration follow-up to an already completed ci-workflows issue. ## Related - melodic-software/ci-workflows#177 - melodic-software/ci-workflows#207
Summary
semantic-pr,do-not-merge-gate, andpr-issue-linkagehonor theirrunnerinput for every prerequisite outcome.Why
The current
runs-onexpression replaces the caller-selected runner withubuntu-24.04wheneverprerequisite-resultis notsuccess. That defeats a private caller's allowlisted managed-runner fallback precisely on the recovery path, causing the required reporter to request hosted capacity instead of the route the caller selected.GitHub supports input-backed
runs-onvalues, while dependency continuation and failure reporting remain separate concerns handled by the caller'sif: always()and the reusable workflow's explicit rejection step.Impact
runnerretain the declaredubuntu-24.04default.Rollout
This is only the upstream reusable-workflow contract portion of #177. After merge,
standardsmust register this PR's exact merge SHA for all three workflow paths incomponents/runner-policy/policy.json, distribute the managed policy copies, and only then should private consumer callers bump their pins and managed fallback expressions.No linked issue — this upstream phase intentionally creates no closing link because the standards registration and downstream rollout remain.
Verification
actionlintnode --test .github/scripts/*.test.cjssuite — 207 passedReferences
jobs.<job_id>.runs-onRelated