feat(guards): actions/runner-isolated — no pull_request-triggered job on a self-hosted runner #60
No reviewers
Labels
No labels
attention
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-ruling
needs-triage
offsite
post-merge
ready
release
scope:docs
scope:guards
scope:labels
scope:release-flow
stale
state:addressing
state:bots-reviewing
state:building
state:needs-human
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: heavy-duty/ceremony#60
Loading…
Reference in a new issue
No description provided.
Delete branch "build/58-runner-isolated"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #58. Part of epic #56 (decision D5): a fifth guard in the family — no
pull_request-triggered workflow may name a self-hosted runner. Unreviewed fork-branch code must never execute on our own hardware; the write-token and secrets toggles protect credentials, not the runner.One deliberate strengthening over the spec's letter, flagged for review: §2's same-line rule (
runs-on∧self-hostedon one line) would pass the block-sequence form (runs-on:newline- self-hosted) — a false negative, which §3 itself declares the one defect this guard must not have ("file-level granularity has none for the modelled threat"). The script therefore also opens a window over a bareruns-on:key's list items. Every spec'd case behaves exactly as written; the change is strictly stricter, tested (three cases), and carried in the script header. If triage or the panel reads §2 as binding to the letter, I'll take a ruling.Worklog
actions/runner-isolated/runner-isolated.sh— the scan, the two-condition rule, the exit codes, header carrying the why (threat, file-level trade, known limits)actions/runner-isolated/action.yml— composite wrapper,workflows-dirinput, description naming the gapstest/runner-isolated.test.sh— the 12-case fixture matrix, the block-sequence cases, env wiring, ceremony's own tree as a pass (22 checks).github/workflows/ci.yml— the guard joinsself-guardsdocs/CONSUMERS.md— the consumer-facing entry (+ the pin-count in Version pinning: four guards, five references)CHANGELOG.md— one line under## Unreleased, inserted above## 0.1.0bash test/run.shgreen locally (13 test files, 0 failed; self-ref-check green)Acceptance criteria (from #58)
on:shapes (block, inline list, scalar) and for the quoted"on":keypull_request_target+ self-hostedpush-only + self-hosted (incubator'sdeploy.ymlshape)pull_request+ubuntu-latest(incubator'spr-checks.ymlshape)self-hostedin a comment only, and a missing workflows directoryruns-online; success prints a one-line countworkflow_callreachability, runner groups / expression indirection)ci.ymlself-guardsruns the guard against ceremony's real tree, green\xescapes, no gawk-only constructs;bash test/run.sh, shellcheck, actionlint all greenVerification
bash test/run.sh— 13 test files pass (runner-isolated adds 22 checks);shellcheck-all.shandactionlint-all.shclean;self-ref-check.shgreen.5d709d7—self-guards(now four steps) passes, run 30002671018.pull_request-triggeredruns-on: self-hostedworkflow (never-matchingpathsfilter, so it cannot execute — its text is the violation). Run 30002684920:self-guardsfails at the runner-isolated step namingscratch-selfhosted.ymland itsruns-on: self-hostedline (job log), and the unit suite's own-tree case catches the same violation independently (test job) — a workflow change that breaks the guard shows up twice, as designed.Incubator's adoption of this guard is deliberately not here — it rides on #16's follow-ups, per the issue's §7.
🔎 reviewing head 5d709d7dd21601e7a9eebafb270c5b442a135405
Verdict: Approve — I agree with this as-is.
Cold-start first review of tip
5d709d7. Convo (body only; no prior reviews/comments) + full tip diff + checks re-read.What holds
workflow_callreachability, runner groups / expression indirection).runs-on:/- self-hostedand create a false negative — exactly what a security guard must not have. Window open/close logic looks sound; tested for both PR-fail and push-pass."on":/'on':;pull_requestsubstring also catchespull_request_targetas specified. Comment skip on both halves; missing/empty workflows dir passes.pull_requestoutsideon:, env/workflows-dirwiring, ceremony's own tree).self-guardsadds./actions/runner-isolated; CONSUMERS pin list + "four CI guards" / five-ref pin-bump note updated; CHANGELOG one line under Unreleased on0.1.1-dev.self-guardsandtest; red-once scratch evidence linked.Judgment call
Agree with the stricter block-sequence detection over the letter of §2 — it is still strictly the §3 rule.
Nits (non-blocking)
No blockers. Re-request if the scan rules or action surface moves.
🔎 reviewing head 5d709d7dd21601e7a9eebafb270c5b442a135405
🔎 reviewing head 5d709d7dd21601e7a9eebafb270c5b442a135405
Verified head
5d709d7dd21601e7a9eebafb270c5b442a135405against #58 and the repository's guard conventions.The implementation satisfies the acceptance criteria: all required trigger shapes and
pull_request_targetare caught, legal push/hosted-runner and absent-directory cases pass, every offender is reported with its matching runner line, the composite action names both known gaps, ceremony dogfoods the guard, the consumer docs and changelog are updated, and the linked scratch run supplies the required red evidence. The block-sequence strengthening is consistent with the issue's security invariant and closes an otherwise real false negative without weakening any specified behavior.Verification:
bash test/run.sh— 13 test files passed, 0 failed; the new suite contributes 22 passing checks.bash .github/scripts/shellcheck-all.sh— 29 tracked scripts clean.bash .github/scripts/actionlint-all.sh— 6 workflow/action files clean.self-guards,test, andaction-exercise.Approve — head
5d709d7.Reviewed against #58's spec, ran everything runnable in a detached worktree, and probed past the fixture matrix.
Verification I ran myself (worktree at
5d709d7):bash test/runner-isolated.test.sh— 22/22 checks pass, including the block-sequence cases and ceremony's own tree as a non-vacuous pass.bash test/run.sh— the only failures are thepackage-jsoncases requiring node, which this box lacks;mainfails them identically, so they are pre-existing and environmental, not from this PR. CI'stestjob (which has node, plus the shellcheck and actionlint steps at ci.yml:18-29) is green at this head, as areself-guards(now four steps) andaction-exercise.runs-on:window (fails, as it must); single-quoted'on':key fails; trailing comment after a bareruns-on:key still opens the window (fails); apull_requestmention in a comment inside theon:block does not trigger (passes); the trigger block ends at the next top-level key (passes);runs-on: {group: …}passes — the documented runner-group gap, named inaction.yml's description so silence is not read as coverage.self-guards, with thetestjob's own-tree case catching the same violation independently — exactly the twice-caught design the PR body claims.Acceptance criteria, one pass over #58's list: all ten hold. The three
on:shapes plus the quoted key fail;pull_request_targetfails;deploy.yml's push+self-hosted andpr-checks.yml's PR+hosted shapes pass; comment-only and missing-dir pass; failure output names every offending file and itsruns-online (fixture 11 pins both-files-named); success prints a one-line count; the action description names both known gaps;self-guardsruns it against the real tree; no\xescapes — the script uses no awk at all. Changelog line matches the spec verbatim, inserted above## 0.1.0. CONSUMERS.md gains the entry and the pin-count edit (three→four guards, four→five references) — I checked the surrounding text, both counts read correctly.The flagged strengthening (block-sequence window): endorsed. #58 §3 declares false negatives the one defect this guard must not have, and the block-sequence form is ordinary, GitHub-accepted YAML — a same-line-only rule would have shipped a real false negative. The change is strictly stricter than §2's letter, every spec'd case behaves as written, it's tested (three cases), and it's carried in the script header. This is the spec's own intent applied, not a departure from it; no ruling needed from where I sit.
Conventions:
set -euo pipefailin the executable,set -uin the test, the decision lives in a script a test drives, and the header carries the why — the threat, the file-level trade, both known limits, with issue references. Matches the four guards beside it.Non-blocking nit (rides the approval):
runner-isolated.sh:144keys the runner half off any line containingruns-on, so a job named e.g.x-runs-on-ywithself-hostedelsewhere on the line would false-positive. Strictly stricter, self-inflicted, and the fix message says exactly what to do — fine as-is.Round summary — handoff
Round 1 (head
5d709d7, the only head this PR has had): codex, grok, kimi all approve. No fixes were required — the head the panel approved is the head under handoff.What each verdict verified:
self-guards, CONSUMERS, CHANGELOG), CI green, red-run evidence. Endorsed the flagged block-sequence strengthening as the spec's own §3 intent, not scope creep.test/run.sh13 files green, shellcheck 29 scripts, actionlint 6 files) and checked all ten acceptance criteria against head.'on':, trailing comment after bareruns-on:, comment insideon:block, trigger-block termination, runner-group gap), live re-verification of the red-run evidence, and a line-by-line pass over all ten acceptance criteria. Also endorsed the strengthening: "the spec's own intent applied", no ruling needed.The one flagged judgment call (block-sequence
runs-on:window, stricter than #58 §2's same-line letter) was reviewed explicitly by all three and endorsed by all three — no triage ruling required.Non-blocking nit on record (kimi, rides the approval):
runner-isolated.sh:144keys the runner half off any line containingruns-on, so a job named likex-runs-on-ywithself-hostedelsewhere on that line would false-positive. Strictly stricter (a false positive, never a false negative), and the failure message tells the author exactly what to do. Left as-is per the approval; if it ever bites, it's a one-line tightening for a future issue via discussion.Blockers: none. CI green at head (
test,self-guards×4 steps,action-exercise, labels, release-exercise), no conflicts, noblocker:*label.@danmt this could be merged right now — requesting your review and setting
state:needs-human.Rebased onto current main (
ca9a1a0) to clear the CHANGELOG.md conflict with #62; no code changes, new head44d0a79, full test suite green — re-requesting the panel since the push staled the approvals.🔎 reviewing head
44d0a79547🔎 reviewing head
44d0a795Re-review of the rebased head
44d0a795(my previous approval was of 5d709d7d).What changed since my approval: nothing in the PR itself.
git diff 5d709d7d..44d0a795over the PR's file set shows only main-side context from the rebase — the #57 changelog line and thetriage-actorsCONSUMERS.md edits that landed via #62. The guard script, action wrapper, fixture matrix, ci.yml wiring, and the PR's own changelog line are byte-identical to what I approved. The disclosed strengthening over #58 §2's letter (block-sequenceruns-on:window) still stands and is still the right call — §3's no-false-negative requirement governs.Verified at this head:
test/runner-isolated.test.sh— 22/22 pass locally, including the block-sequence cases and ceremony's own tree as a pass.test/run.sh— the only local failures arepackage-jsonbackend cases requiringnode, which is not installed in this environment; CI runs them green at this head.44d0a795: all checks green (test, self-guards, action-exercise, docs-sync-exercise).Approving the current head.
Approve — head
44d0a79547f8a1307ef06a4227ec6196dc4c797d(re-review after rebase; prior approval was on5d709d7).Reviewed against #58's acceptance criteria. Ran the full suite and probed the security edges in a detached worktree.
Verification (worktree at this head)
bash test/run.sh— 14/14 files, 0 failed (runner-isolated: 22 checks).runs-on: \n - self-hostedunderpull_requestfails and names the line; push-only + self-hosted passes; comment-only self-hosted passes.self-guards,test, labels, release-exercise).Acceptance criteria
All #58 criteria hold at this head: three
on:shapes + quoted"on":,pull_request_target, push-only legal, hosted PR legal, comment-only and missing-dir pass, failure names file+line, action description names both known gaps (workflow_call, runner groups/indirection), wired intoself-guards, red-once evidence linked in the PR body, changelog line under Unreleased above## 0.1.0, no\x/ gawk-only constructs.Deliberate strengthening (block-sequence window)
Agree with the disclosed judgment. Spec §2's same-line rule would miss:
§3 says false negatives are the one defect this guard must not have. Opening a window over a bare
runs-on:key's list items is strictly stricter, covered by three fixture cases, and documented in the script header. Not a letter-of-spec violation that should block.Ready for the rest of the panel / human when the round converges.
🔎 reviewing head
44d0a79547Re-review of head
44d0a79547f8a1307ef06a4227ec6196dc4c797dafter the rebase; my prior approval was on5d709d7dd21601e7a9eebafb270c5b442a135405.The PR still satisfies #58 in full. The rebase only brings in current-main context; the runner-isolated action, scan logic, fixture matrix, self-guard wiring, consumer documentation, and PR changelog entry retain the approved behavior. The deliberate block-sequence strengthening remains the correct interpretation of the security invariant: it prevents a real false negative while preserving every specified legal case.
Verification at this exact head:
bash test/runner-isolated.test.sh— 22 passed, 0 failed.bash test/run.sh— 14 test files passed, 0 failed.bash .github/scripts/shellcheck-all.sh— 31 tracked scripts clean.bash .github/scripts/actionlint-all.sh— 6 workflow/action files clean.self-guards,test,action-exercise, anddocs-sync-exercise.No blockers.
Closing round summary — handoff at head
44d0a79Converged: two rounds, three approvals on the current head, zero blockers.
What shipped
actions/runner-isolated— the fifth guard (#58, epic #56 D5): nopull_request/pull_request_target-triggered job may name a self-hosted runner. Concretely: the scan script with the two-condition rule and header carrying threat model and known limits; the compositeaction.ymlwithworkflows-dirinput; a 22-check fixture suite including the block-sequence cases and ceremony's own tree; the guard wired intoself-guards; the CONSUMERS.md entry (pin-count now four guards, five references); one CHANGELOG line under Unreleased. One deliberate strengthening over #58 §2's letter — a window over a bareruns-on:key's list items — disclosed in the PR body and endorsed explicitly by all three reviewers as §3's own no-false-negative intent; no ruling was needed.What each round changed
5d709d7): the submission as written. codex, grok, kimi all approved; no fixes were required, so the round produced no code changes.ca9a1a0to clear the CHANGELOG conflict from #62. No code changes — new head44d0a79, approvals staled, panel re-requested.44d0a79): all three re-approved. Each independently confirmed5d709d7..44d0a79over the PR's file set is rebase context only — guard, wrapper, fixtures, wiring, and changelog line byte-identical to the approved round-1 content.What was verified
test/run.sh(14/14 files) in a detached worktree plus manual security probes (block-sequence fail, push-only pass, comment-only pass) against all #58 acceptance criteria; codex — full suite, shellcheck over 31 scripts, actionlint over 6 files, all ten acceptance criteria.test,self-guards(four guard steps),action-exercise,docs-sync-exercise,release-exercise(fixture-chain + both step-replays), labels.self-guardsstep and the unit suite's own-tree case (runs linked in the PR body).Post-merge residue
runner-isolated.sh:144keys the runner half off any line containingruns-on, so a line likex-runs-on-y … self-hostedwould false-positive. Strictly stricter, clear failure message; a one-line tightening for a future issue if it ever bites.@danmt this could be merged right now — requesting your review and setting
state:needs-human.