feat: runner selection moves to the RUNNER_LABELS repository variable - #32
Conversation
…efaults Each default contradicted something the repository already documents. squash_merge_commit_title=PR_TITLE. AGENTS.md, CONTRIBUTING.md, pr-authoring and branch-and-commit all state the PR title becomes the commit message on main. GitHub's COMMIT_OR_PR_TITLE default makes that false whenever a PR has exactly one commit -- visible in this repo's own log, where #7 and #9 carry the (#N) suffix and #11, #13 and #15 do not. squash_merge_commit_message=PR_BODY. The COMMIT_MESSAGES default concatenates every branch commit message into the main commit body, and release-please deliberately parses that body for further Conventional Commits and BREAKING-CHANGE footers. Already live here: f98cbf9's body carries "* chore: trigger CI on release PR", harmless only because chore is release-please's hidden bucket. A branch commit reading "fix: wip" would have produced a phantom changelog entry or an unintended bump. allow_rebase_merge=false. The comment justifying rebase claimed release-please merges its own PR. It does not -- ADR-0002, the release-please workflow header and the release-management skill all require a human merge. Nor has rebase ever been used: the history has no merge commits, all eight merged PRs were squash-merged, and both release PRs cut their tags that way. allow_update_branch=true. The ruleset sets strict_required_status_checks_policy=false, so the "Update branch" button is not offered at all without it. Also corrects skills/release-management/SKILL.md, which instructed `gh pr merge --merge`. allow_merge_commit=false has been set since day one, so that command has always returned HTTP 405 -- the documented release procedure was broken. Its parenthetical was wrong too: release-please does not merge. Accepted cost: with PR_BODY an unmodified PR template lands verbatim in the main commit message. BLANK was rejected -- it drops Co-authored-by trailers and discards the RISK/rollback record. PR_BODY reduces the misparse surface but does not eliminate it; a Conventional-Commit-shaped line in a PR body is still parsed. Closes #24 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bootstrap did nothing about security settings -- grep for secret, scanning,
visibility, vulnerability or dependabot in the script returned nothing. Two
consequences. Converting a repo private to public grants ACCESS to secret
scanning but does not enable it, and push protection in particular must be
switched on explicitly, which is how a public repo ends up without it. And
.github/dependabot.yml asserts Dependabot alerts and security updates are
enabled while nothing verifies that.
The write path is bounded by cost, not by capability: the only settings this
phase ever enables are free by construction.
- Public repo: offers secret scanning and push protection, both free.
- Private or internal: never writes them, under any flag. There they need a
paid Advanced Security / Secret Protection seat, and a setup script must not
commit an adopter's account to a per-committer charge. It reports the state
and emits a MANUAL step.
- Dependabot alerts and automated security fixes: free everywhere, so offered
regardless of visibility.
- Repository visibility is never changed and never offered. Private to public
erases stars and watchers and publishes all Actions history -- a one-way
door, and the phase comment says so, because "detect whether the repo is
public" invites someone to add that prompt later.
Two states the GitHub UI blurs are kept distinct. Unreadable settings mean the
token lacks admin, not that the setting is off, so security_and_analysis
coming back null degrades to warn plus MANUAL rather than reporting "disabled".
And automated-security-fixes returns {"enabled":..,"paused":..}: enabled but
paused means no fix PR ever opens, so it is reported separately.
Verified read-only before writing the phase: gh api documents the
key[subkey]=value nested syntax, so the security_and_analysis PATCH stays a
normal run_or_dry call visible under --dry-run; /vulnerability-alerts returns
204 enabled and 404 disabled; /automated-security-fixes returns enabled and
paused.
Unlike phases 3/5/8 this phase runs several independent checks, so it
accumulates a result and calls record_phase once at the end -- the pattern
phase_issue_types uses, matching run_phase's one-record-per-exit-path
contract.
Phases 6/7/8 renumber to 7/8/9 across the script and docs. Because every read
runs for real under --dry-run, bootstrap --dry-run now doubles as a zero-risk
security audit.
Closes #26
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
De-templating rewrote README, CHANGELOG and the release-please manifest and removed docs/template/, but never touched LICENSE -- grep for "license" in scripts/ returned nothing. So every adopted repository shipped MIT, copyright the template author. For an open-source adopter that is a wrong copyright line. For commissioned work it is an irrevocable written grant to the whole world of the right to use, modify, publish, distribute, sublicense and sell, made before payment, which removes the leverage the payment clause was built on. Four private client repos shipped that way. The obvious fix is itself a defect. Substantial portions of this template ship verbatim in every adopted repo, and MIT requires its copyright and permission notice to travel with them; rewriting LICENSE's copyright line deletes the only copy of that notice from the repository. So attribution is unconditional -- every answer that writes LICENSE also writes NOTICE, MIT-keep included. That, not the prompt, is the load-bearing part of this change. Three answers, no default. A bare Enter re-asks and a closed stdin defers, because confirm() treats closed stdin as "take the default", which here would mean silently shipping the template author's MIT -- the original bug with extra steps. --yes never writes a licence: it defers and files the decision as the first manual step, rendered above the others with a ! marker via a new MANUAL_URGENT list. --license mit|proprietary|defer answers non-interactively. Phase 9 runs before de-template and is not gated by --keep-template-docs: de-template returns early for three unrelated reasons, any of which would otherwise swallow the decision. It guards only LICENSE and NOTICE; adding LICENSE to phase 10's guard would make de-template skip itself on every run once phase 9 had written. LICENSE is regenerated from a seed rather than sed-patched, because holder names legitimately contain & and /. No docs/template/LICENSE.proprietary.example: under --yes the phase writes nothing and files a manual step, and phase 10 then removes docs/template -- deleting the example in the very run that told the adopter to read it. The body lives as a script constant, mirroring CHANGELOG_SEED, and in docs/setup/licensing.md which survives de-templating. Also adds scripts/check-license-marker.sh, wired into make check. If TEMPLATE_COPYRIGHT_* drifts from LICENSE the phase stops recognising its own licence and silently does nothing -- the original defect with no symptom. The check turns that into a red build, and no-ops in adopted repos. main is now guarded by a BASH_SOURCE test so the file can be sourced for its functions without running. Nothing in make verify covers this script; that guard is the only unit-test surface it has, and all nine licence paths were exercised through it. De-template renumbers 9 to 10. ADR-0004 records the decision and seven rejected alternatives. Closes #28 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All four workflows hardcoded runs-on: ubuntu-latest, so an adopter needing a different runner had to edit workflow YAML. That contradicted the template twice: AGENTS.md says customize the Makefile never the workflows, and design principle 5 promises workflow YAML stays untouched and upgradable. runs-on is the one adopter-facing property the Makefile physically cannot own, because GitHub resolves it when scheduling the job, before any make target exists. The rule was unsatisfiable for it. It also broke upgrading.md's advice to bulk cherry-pick .github/workflows/, since a forked runs-on line makes every template update a manual merge. One variable, not the four originally proposed: design principle 1, three of the four jobs are trivial and identical in shape, and every extra name is another thing to typo. A JSON array so multi-label self-hosted selection works. Unset reproduces today's behaviour exactly. The cost is real and was measured rather than assumed. A throwaway probe workflow using a separate variable name, so the required ci check was never at risk, was pushed with the value set to the tempting wrong `ubuntu-latest` -- a bare string instead of an array. Result: fromJSON fails during scheduling, the run completes with conclusion=failure and the message "This run likely failed because of a workflow file issue", ZERO jobs are created, and ZERO check runs are attached to the commit. Applied to ci.yml that means the only required check is never reported and the pull request cannot merge, with nothing in the Actions tab naming the variable. Recovery is `gh variable delete RUNNER_LABELS`. ci.yml's header now documents exactly that, alongside the two existing routes into the same trap. AGENTS.md is amended to name the exception rather than leave it as folklore, and hygiene rule 1 is amended to match so the skill does not contradict the shipped workflows. New docs/setup/runners.md covers the variable, the malformed-value failure and its recovery, the linux x86_64 bound imposed by install-ci-tools.sh, self-hosted runner caveats that were not written down anywhere (npm and pip installs escape INSTALL_DIR and run globally on the host every job), and why downsizing a runner to save money is usually a pessimisation on a repo shaped like this. ADR-0005 records the decision, and records the plain-string alternative as the closest call: it would remove the malformed-JSON footgun entirely at the cost of multi-label support. Closes #31 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Live verification of all three variable statesRun against this PR on the real repository, not a scratch one.
State C was measured on a throwaway branch with a separate variable name ( State B required an empty commit to re-trigger CI with the variable set. I have left it in the branch rather than rewriting history —
|
![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: 1859a3ce2e
ℹ️ 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".
|
|
||
| ```bash | ||
| gh variable set RUNNER_LABELS --body '["ubuntu-latest-4-cores"]' | ||
| gh variable set RUNNER_LABELS --body '["self-hosted","linux","x64"]' |
There was a problem hiding this comment.
When this template remains public and an adopter uses this documented setting, .github/workflows/ci.yml still triggers on every pull_request, checks out the PR-controlled tree, and executes its Makefile and scripts via make ci-tools and make ci-pr. An external contributor can therefore run arbitrary code on the self-hosted machine and access its network, filesystem, or ambient credentials; ephemeral cleanup does not protect resources reachable during the job. Add a prominent public-repository prohibition or route fork PRs to GitHub-hosted runners instead.
Useful? React with 👍 / 👎.
1. An ampersand in the copyright holder corrupted LICENSE on bash >= 5.2.
${var//pat/repl} expands an unescaped & in the REPLACEMENT to the matched
text when patsub_replacement is on, which is the default from 5.2. A holder
of "Smith & Jones a/s" rendered as "Smith __HOLDER__ Jones a/s" while the
phase reported success. My earlier test missed it because macOS /bin/bash is
3.2, where the option does not exist -- and escaping as \& is itself literal
on 3.2, so no single expansion is correct on both. Replaced with subst_all,
a literal prefix/suffix-removal helper that never interprets the
replacement. Verified identical output on 3.2 and 5.3.
2. write_notice returned early whenever a NOTICE file existed, treating mere
existence as proof the upstream notice was present. An adopter with their
own NOTICE for other dependencies would have had LICENSE replaced while the
only copy of the template's MIT notice was silently dropped -- the exact
violation this phase exists to prevent. It now looks for the upstream
copyright line, appends when absent, and keeps existing content.
3. docs/setup/licensing.md's NOTICE block shipped literal __YEAR__ __HOLDER__
placeholders, so anyone following the documented manual path produced a
NOTICE with no attribution in it. It now carries the template's real
copyright line, with a sentence explaining that this one is deliberately not
the adopter's identity.
Fixing (1) introduced a regression that the doc-vs-output diff then caught:
command substitution strips trailing newlines, so LICENSE lost its final
newline and the NOTICE closing rule ran into "SOFTWARE.". Both fixed, the
constraint is documented on subst_all, and the doc block is now byte-identical
to what the script writes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pushed the previous commit before reading the markdownlint result -- my command chained on git add rather than gating on lint. CI would have caught it; that is not the same as checking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex review, and it is the sharpest finding in the stack. runners.md documented how to point RUNNER_LABELS at a self-hosted runner and listed only housekeeping caveats. But ci.yml triggers on pull_request, which on a public repository includes fork pull requests, and the job checks out the PR's own tree and runs make ci-tools and make ci-pr from it. Anyone on the internet could therefore execute their own Makefile and scripts on the adopter's machine, with its filesystem, network position and reachable credentials. Ephemeral cleanup does not help -- the damage happens during the job. The section now leads with the prohibition rather than burying it: on a public repo leave RUNNER_LABELS unset; the only safe shapes are making the repo private, or splitting the workflow so fork PRs stay on hosted runners. It also says why "require approval for first-time contributors" is not a fix -- approval is per-contributor, not per-diff. The operational caveats are demoted to a private-repository subsection, and ci.yml's header carries the warning at the point of risk. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # docs/setup/bootstrap.md
Codex P1 addressed — and it was the sharpest finding in the stackYou're right, and the doc was actively dangerous as written. The section now leads with the prohibition instead of burying it:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8504ce6068
ℹ️ 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".
| jobs: | ||
| ci: | ||
| runs-on: ubuntu-latest | ||
| runs-on: ${{ fromJSON(vars.RUNNER_LABELS || '["ubuntu-latest"]') }} |
There was a problem hiding this comment.
When a repository belongs to an organization that already exposes an organization-level RUNNER_LABELS, vars.RUNNER_LABELS resolves that value even though the repository variable is unset, bypassing both the documented ubuntu-latest fallback and the public-repository safety instruction; an inherited self-hosted value would immediately route fork PR code onto that runner. gh variable set --help confirms that organization variables are “available to GitHub Actions runs ... within an organization.” Use a collision-resistant name and/or have bootstrap create a repository-level default that shadows organization scope; deleting the repository variable alone is not reliable recovery.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
1. An unreadable secret-scanning state was treated as disabled. security_and _analysis is only populated for callers with admin on the repo, so a token without it yields "public\tunknown\tunknown" -- a non-empty facts line that fell through to the else branch and prompted to enable, or PATCHed outright under --yes. That acts on a guess and reports a state never observed, which is the opposite of the unknown-is-not-disabled rule this phase was written around. There is now an explicit unknown branch that warns, emits a manual step, and writes nothing. 2. The Actions-permission troubleshooting entry still said phase 6 after the renumber moved it to 7, so the recovery text pointed at the Security prompt instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1. LICENSE was replaced before NOTICE was known to be writable. If the NOTICE write then failed -- directory permissions, quota, I/O -- the repository was left with neither the template's original notice nor the promised attribution, a worse state than not running the phase at all. NOTICE is now written first and LICENSE is only touched once it succeeded. Verified by making NOTICE an unwritable path: the phase fails and LICENSE is still the template MIT, byte for byte. 2. The BASH_SOURCE guard covered main but not the top-level argument parser, so sourcing the file from a shell that had positional parameters consumed the caller's arguments and exit 1'd on the first one it did not recognise -- terminating the sourcing shell. Reproduced with `set -- unexpected-arg`. Both the parser and main now sit behind a bootstrap_is_main helper. Sourcing still applies set -euo pipefail to the caller; that is documented rather than silently changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5430d51f93
ℹ️ 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".
| # Runner selection: the RUNNER_LABELS repo variable, a JSON ARRAY of labels. | ||
| # Unset = ubuntu-latest. A malformed value stops this job reporting any | ||
| # check at all -- see docs/setup/runners.md and ci.yml's header. | ||
| runs-on: ${{ fromJSON(vars.RUNNER_LABELS || '["ubuntu-latest"]') }} |
There was a problem hiding this comment.
When RUNNER_LABELS selects a persistent self-hosted pool, this write-enabled job runs on the same machines that .github/workflows/ci.yml uses to execute pull-request-controlled Makefiles. In a private repository where a less-trusted contributor's PR is allowed to run, that code can persist on the runner and capture the later contents: write, pull-requests: write, and issues: write token when release-please runs; making the repository private does not provide job isolation. Keep privileged automation on hosted or separately isolated runners rather than routing all four workflows through one label set.
Useful? React with 👍 / 👎.
| - On a **public** repository, GitHub-hosted runners are free. A smaller runner | ||
| saves exactly nothing. |
There was a problem hiding this comment.
When an adopter follows the earlier ubuntu-latest-4-cores example to upsize, this claim is false: GitHub bills larger hosted runners even for public repositories; only standard hosted runners are free there. Because the page presents upsizing as an intended use of RUNNER_LABELS, the unqualified statement can cause adopters to incur charges they were explicitly told would be zero; qualify the claim and direct larger-runner users to the applicable pricing.
Useful? React with 👍 / 👎.
# Conflicts: # docs/adr/README.md # docs/template/architecture.md
1 participant
Summary
All four workflows now resolve
runs-onfrom a singleRUNNER_LABELSrepository variable, defaulting toubuntu-latest. Unset, behaviour is byte-identical to today.Related issue
Closes #31
Validation
make test(placeholder)cirun is the unset-variable path executing for realI measured the failure mode instead of guessing
This change introduces a third route into the "Expected — waiting for status" trap that
ci.yml's header already warns about, and it is the only one an adopter can trigger from repository settings rather than from the file. That deserved evidence.A throwaway probe workflow was pushed on a scratch branch using a separate variable name (
RUNNER_LABELS_PROBE), so the requiredcicheck was never at risk, with the value set to the tempting wrongubuntu-latest— a bare string instead of a JSON array:fromJSONfails while the job is being scheduled, so no job and no check run are ever created. Applied toci.yml— this repository's only required status check — the check is never reported, the PR cannot merge, and the Actions tab never mentions the variable.Recovery, which is not discoverable from the symptom, is now documented in both
ci.yml's header anddocs/setup/runners.md:The probe workflow, its variable, its PR and its branch were all removed afterwards.
Risk / rollback
Checklist
*.local.mdfiles committedWhy this resolves a contract violation rather than creating one
AGENTS.md:19says "customize the Makefile, never the workflows", anddesign-principles.md:11promises "workflow YAML stays untouched and upgradable". Butruns-onis resolved by GitHub when scheduling the job, before any make target exists to be called — it is the one adopter-facing property the Makefile cannot own. So the rule was unsatisfiable for it, and an adopter needing a different runner had no option but to break it.It also degraded upgrades:
upgrading.md:18recommends bulk cherry-picking.github/workflows/precisely because it is rarely customized locally. A forkedruns-online makes every template update a manual merge.AGENTS.mdis therefore amended to name the exception, not left to folklore — an undocumented sanctioned exception is how a contract rots — and hygiene rule 1 is amended to match, or the skill would contradict the shipped workflows.Decisions worth challenging
One variable, not four. The field report proposed
CI_RUNNER_LABELS/AUTOMATION_RUNNER_LABELS/MAINTENANCE_RUNNER_LABELS/RELEASE_RUNNER_LABELS. Rejected: design principle 1 ("one good default beats three options"), three of the four jobs are trivial and identical in shape, and every extra name is another thing to typo. Splitting one into several later is trivial; merging four after adopters have set them is not.fromJSONarray vs plain string.${{ vars.RUNNER_LABELS || 'ubuntu-latest' }}would remove the malformed-JSON footgun entirely — the single largest cost of this change — at the cost of multi-label self-hosted selection (["self-hosted","linux","x64"]), which is the main reason self-hosted users want this. This is the closest call here and ADR-0005 records it as such. If you judge the footgun worse than multi-label support is worth, it is a one-line substitution and nothing else in this PR changes.Things that were not written down anywhere before
docs/setup/runners.md(123 lines) also captures two constraints that existed but were undocumented:install-ci-tools.sh's three tarball tools ship no other assets andrequire_supported_platformhard-fails otherwise — so pointing the variable at macOS or arm64 produces aunamemessage that reads like a broken script rather than a misconfigured variable. That script's own comment claiming the pins suit "the ubuntu-latest hosted runners this repo uses" is corrected too, since it stops being true the moment the runner is a variable.install_npm_globalandinstall_python_toolescapeINSTALL_DIR,require_install_consentwaves them through wheneverCIis set (Actions always sets it), andplace_binaryusessudo. Fine on an ephemeral container, an accumulating mess on a long-lived VM.On runner sizing
The adopter's
ubuntu-slimmeasurements are folded intorunners.mdas guidance rather than into the hygiene skill as a rule, because the billing argument already lives inci.yml:16-19and a second copy is where drift starts. Short version: hosted runners are free on public repos, private billing rounds up to whole minutes so a sub-minute job already bills the floor, and a smaller runner can only push it over. Downsize only when a job materially exceeds a minute and you are past your included minutes.If you would rather have it as a durable rule 11 in
skills/github-actions-hygiene/SKILL.md— there is a fair case, sinceci.ymlalready spends four lines rebutting the same instinct, which is evidence the debate recurs — say so and I will add it with a one-directional pointer fromci.yml.