Skip to content

feat: govern local runner routing policy - #85

Merged
kyle-sexton merged 9 commits into
mainfrom
codex/local-runner-policy
Jul 11, 2026
Merged

feat: govern local runner routing policy#85
kyle-sexton merged 9 commits into
mainfrom
codex/local-runner-policy

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a centralized GitHub Actions runner policy that enforces approved selector routing, public/hosted boundaries, explicit read-only permissions, cancellation-safe literal hosted fallback, and machine-readable hosted exceptions
  • derive the required literal fallback from the governed policy default, so changing an approved hosted image is a policy/configuration change rather than parser-code surgery
  • route Standards' 28 eligible private Linux workloads through the governed selector while retaining exact hosted exceptions for policy and control-plane boundaries
  • recursively validate repository-local reusable workflows and caller/callee permission narrowing without allowing arbitrary secrets, tokens, inputs, labels, or runner expressions
  • distribute the locked policy runtime and Node version to six enrolled private consumers from one deterministic manifest
  • harden staged .NET formatting and PSScriptAnalyzer adapters with deterministic cross-platform path semantics and per-target no-profile PowerShell isolation
  • keep the complete .NET-format named job managed by Standards while each consumer owns only strict data in .lefthook/dotnet-format.json
  • preserve executable source and consumer index modes through distribution
  • pin every production selector/reusable and Actionlint parity reference to merged ci-workflows/main commit 99ac2f8c5b09dbb785d4eaf18465cbd96c30290c

Dependencies

The final routing contract is the immutable squash merge from melodic-software/ci-workflows#74 (including stacked #76/#77):

  • 99ac2f8c5b09dbb785d4eaf18465cbd96c30290c

Reviewed head

0795d22c89cb8fae11642ede9757e7b43fd5d546

Validation

Independent author, reviewer, recheck, and integration-review gates all PASS with no findings.

  • runner-policy adversarial suite: 83/83, including alternate configured hosted-default proof
  • Standards private self-audit: PASS
  • .NET/Lefthook adapter: 12/12
  • pinned Lefthook 2.1.9 validate, dump, and actual job execution: PASS
  • independent argv probe: spaces, semicolons, and $() remain inert data with shell:false
  • production distribution suite: 114/114 under checksum-pinned yq 4.53.3 in author native Linux and hosted Linux
  • independent reviewer inspected the exact-head hosted log and confirmed assertions 1 through 114
  • exact executable-bit gate: PASS; both source CLIs are index mode 100755
  • routing graph: 28 selectors, 28 workloads, 31 actual ci-status gates, zero selector gates
  • final pin proof: exactly 46 merged-main references, zero stale full/short feature-stack references, and 25 preserved transitional compatibility references
  • all eight changed files reconstruct byte-for-byte from only the two intended SHA/comment substitutions
  • Actionlint 1.7.12 plus hosted checksum-verified ShellCheck 0.11.0: PASS
  • all 10 uniquely referenced workflow/action paths exist at the immutable ci-workflows commit
  • six workflow schemas, Zizmor medium/high, Biome, Markdown, ShellCheck, Gitleaks, full Lefthook, and diff checks: PASS
  • signed final pin commit: 0795d22c89cb8fae11642ede9757e7b43fd5d546

All 63 hosted checks pass on this exact head.

Authoritative basis

Rollout safety

This PR does not change GitHub variables, secrets, runners, repository settings, or live infrastructure. Production routing remains hosted until the IaC and physical canary gates are applied later.


Note

High Risk
Large CI workflow refactor with secrets/vars for runner selection and a new security gate; misconfiguration could break merges or route jobs incorrectly until fleet is live.

Overview
Introduces a YAML-aware runner policy (components/runner-policy, .github/runner-policy.json, policy.json) and a hosted Runner policy CI lane that tests and enforces it against workflow inventory and repository visibility.

CI routing shifts eligible lint/contract jobs from fixed ubuntu-latest to paired select-runner + workload jobs using needs.select-*.outputs.runner || 'ubuntu-24.04', if: ${{ !cancelled() }}, and merge_group support. Control-plane jobs (runner-policy gate, ci-status, zizmor, osv-scanner) stay on explicit hosted runners with documented exceptions. ci-status now requires runner-policy, treats only success as pass (not skipped), and pins several workflows to ci-workflows@99ac2f8.

Local hooks: Lefthook .NET formatting moves to a consumer-owned .lefthook/dotnet-format.json and dotnet-format-staged.mjs (shell-less dotnet format whitespace). PSScriptAnalyzer staged checks run one target per fresh pwsh worker; PSUseCorrectCasing is removed from settings. Dependabot gains an npm root for components/runner-policy.

Reviewed by Cursor Bugbot for commit 0795d22. Bugbot is set up for automated code reviews on this repo. Configure here.

@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 7m 24s —— View job


PR Review: feat: govern local runner routing policy

  • Gather context and read review criteria
  • Review runner policy core logic (components/runner-policy/runner-policy.mjs)
  • Review policy configuration (components/runner-policy/policy.json, .github/runner-policy.json)
  • Review CI workflow changes (.github/workflows/ci.yml)
  • Review PSScriptAnalyzer adapter changes
  • Review .NET format adapter changes
  • Review distribution manifest changes
  • Review test coverage
  • Synthesize findings

Overall

This is a well-engineered, high-quality PR. The policy logic is correct, security boundaries are sound, and the test coverage is comprehensive. All issues below are suggestions or minor observations — none are merge-blockers.


Security (positive observations)

  • YAML parsing uses maxAliasCount: 0, merge: false, strict: true, uniqueKeys: true — closes YAML anchor/alias amplification and duplicate-key confusion vectors. runner-policy.mjs:381
  • Symlink detection in repositoryWorkflowIndex prevents symlink-based workflow file substitution. runner-policy.mjs:1457
  • secrets: inherit prohibition on both selector and all reusable workflow contracts prevents credential forwarding to governed callees.
  • Literal fallback enforcement in RUNNER_OUTPUT — the regex requires exactly 'ubuntu-24.04' as the literal fallback, preventing variable-controlled fallbacks that would silently route work to self-hosted on selector failure. Tests at lines 232–258 of the test file confirm this specifically.
  • containsCredentialExpression correctly carves out step-level env/with values while rejecting transformed or indirect credential forwarding via run: blocks or job-level env. runner-policy.mjs:1307
  • localCredentialActions allowlist correctly enforces that credential-minting actions require a privileged-control-plane hosted exception, even when the job otherwise has read-only permissions.

Suggestion — RUNNER_OUTPUT regex and governedReusableRunnerInput.default are silently coupled

The regex at runner-policy.mjs:14 hardcodes ubuntu-24.04 as the accepted literal fallback. The same label appears again in policy.json as governedReusableRunnerInput.default. If the approved default label ever changes (e.g., to ubuntu-26.04), both need updating in sync. A future maintainer touching only the policy field would produce a policy that accepts a fallback label the regex no longer matches. A brief inline comment explaining the intentional coupling would prevent silent drift:

// Literal fallback must be the governed default label — changing it here requires
// updating policy.governedReusableRunnerInput.default in policy.json in tandem.
const RUNNER_OUTPUT = /^\s*\$\{\{\s*needs\.([A-Za-z0-9_-]+)\.outputs\.runner\s*\|\|\s*'ubuntu-24\.04'\s*}}\s*$/;

Fix this →


Suggestion — cancellationSafeConditionStatus wrapper regex is greedy

At runner-policy.mjs:995:

const wrapper = /^\s*\$\{\{([\s\S]*)}}\s*$/.exec(value);

The greedy [\s\S]* backtracks to the last }} in the string. For every legitimate ${{ expr }} condition this is correct. The pathological case — a single-quoted string literal inside the expression that contains a literal }} — isn't reachable via GitHub's expression language (single-quoted strings escape with '', not with }}), so this is extremely low-risk. Nonetheless, a comment noting the assumption, or switching to non-greedy [\s\S]*?, would make the intent explicit:

// Greedy match: GitHub expression strings use '' escaping and cannot contain }}
// so the last }} in value is always the expression closer.
const wrapper = /^\s*\$\{\{([\s\S]*)}}\s*$/.exec(value);

Suggestion — psscriptanalyzer-target.ps1 loads whichever PSSA version is newest

At psscriptanalyzer-target.ps1:25:

Import-Module PSScriptAnalyzer -ErrorAction Stop

On developer machines with multiple PSSA versions installed, this loads the newest available rather than the 1.25.0 that CI enforces and that the per-target isolation specifically works around. The workaround was introduced because 1.25.0 can throw a NullReferenceException in sequential in-process runs — other versions may behave differently. A comment pointing to the version constraint used in CI (and PSScriptAnalyzerSettings.psd1) would help local reproducibility:

# CI pins PSScriptAnalyzer 1.25.0; local developer installs may differ.
# See .github/workflows/ci.yml (powershell-fixtures job) for the authoritative version.
Import-Module PSScriptAnalyzer -ErrorAction Stop

Architecture observations

Two-tier policy design (operator policy.json + per-repo runner-policy.json) is clean. The schemaVersion field on both tiers future-proofs independent evolution.

localPermissionVisits dedup across all seedings — the set at runner-policy.mjs:1512 uses ${file}\0${inherited} keys, so the same callee visited with "may-write" from two different callers is only checked once. This is a correct optimization: if a callee has write-permission concerns under a given inherited level, reporting them once is sufficient regardless of which caller reached it. The multi-level chain test at line 1361 of the test file confirms the flow is preserved.

Correctness check on callee-declared write permissions: I verified that write-capable callee jobs with kind: "reusable-input" ARE independently caught by the main audit loop's privilegedHosted path (via permissionHostedRequirement), even when auditLocalPermissionFlow short-circuits them because the caller was read-only. No enforcement gap.

DFS backtracking in localWorkflowRoutingMode (visited.delete at runner-policy.mjs:620) correctly handles diamond-shaped workflow dependency graphs without falsely returning "internal-routing" for the shared callee.


CI workflow observations

  • runner-policy job runs the adversarial suite then the self-audit. CI_REPOSITORY_VISIBILITY correctly sources from github.event.repository.visibility rather than hardcoding.
  • All new jobs are wired into the ci-status aggregator. skipped is correctly accepted as a pass.
  • All composite action references use pinned SHAs. All reusable workflow calls in ci.yml match entries in the approvedReusableWorkflowContracts policy allowlist.
  • The biome job scopes to components/biome/fixtures/good components/runner-policy — consistent with the existing pattern but means any future JS outside those paths won't be Biome-checked by CI. Not a new issue, just worth noting for future additions.

Distribution manifest

  • runner-policy component correctly distributes package-lock.json alongside the runtime and policy, so consumers get the pinned yaml@2.9.0 dependency.
  • requires: [node-runtime] ensures .node-version is co-distributed.
  • New psscriptanalyzer-target.ps1 is correctly added to the lefthook-powershell component alongside psscriptanalyzer-staged.ps1.

Test coverage

80 adversarial tests cover all rule types, the production SHA allowlist, local-reusable permission flow chains, matrix hosting, cancellation-safe condition parsing, exception inventory drift, symlink detection, and recursion detection. The dotnet-format-staged.test.mjs covers Windows path containment, UNC/drive-relative rejection, workspace escapes, deduplication, and empty-set skip. PSScriptAnalyzer staged adapter tests cover both the historical six-file and current seven-file hook shapes with real PSSA 1.25 invocations.

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I revalidated each suggestion against the final head 005bf15 and the relevant upstream contracts.

Finding Classification Evidence / disposition
RUNNER_OUTPUT fallback coupling VALID — fixed Signed commit dc6addadd0a7c8ae4512ae511545c7b80dacd4c6 captures the literal fallback and compares it to policy.governedReusableRunnerInput.default. The alternate-default regression test proves the parser follows configuration rather than a hard-coded Ubuntu label.
Greedy cancellation-wrapper regex INCORRECT — no change The premise that a quoted GitHub expression string cannot contain }} is false. GitHub's runner TemplateReader explicitly treats closers inside quoted strings as literals and includes a literal-}} example. Keeping the trailing-anchor match avoids encoding the suggested false assumption; the cancellation-condition suite remains green.
Local PSScriptAnalyzer version VALID — intentionally deferred Microsoft documents -RequiredVersion for exact module selection. The local hook deliberately accepts PSScriptAnalyzer >=1.25 while hosted CI imports exact 1.25.0; that contract is now explicit in the component README and the exact-version regression remains in CI. Requiring one locally installed patch would reduce developer portability without changing the CI authority.

The remaining bullets are positive/informational observations. The Biome scope note predates this change and is outside this PR.

Final evidence: runner-policy 83/83 and all 63 hosted checks pass at 005bf15.

@kyle-sexton
kyle-sexton marked this pull request as ready for review July 11, 2026 12:34
@cursor

cursor Bot commented Jul 11, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_1919cf20-e67e-42d5-adae-6e1c0e0c37c3)

@cursor cursor 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.

Risk: high. Not approving: Cursor Bugbot completed with status skipping because usage limits blocked analysis, so the required automated review signal is incomplete. Human review is needed for this infra-heavy runner-policy change; no additional reviewers were assigned because no eligible non-author reviewers are available in this repository.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@kyle-sexton
kyle-sexton merged commit ff4b473 into main Jul 11, 2026
70 checks passed
@kyle-sexton
kyle-sexton deleted the codex/local-runner-policy branch July 11, 2026 12:37

@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: 0795d22c89

ℹ️ 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".

return { approved: true, kind: "hosted-reusable" };
}
const target = local.approved
? job.with[policy.governedReusableRunnerInput.name]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard missing local reusable inputs before indexing

When a job calls a repository-local workflow whose governed runner input is optional/defaulted and omits with: entirely, validateLocalCallMapping() accepts the call because it only rejects missing required inputs. localReusableWorkflowStatus() can then approve it as runner-input, so this line reads job.with.runner while job.with is undefined and the policy CLI exits with a TypeError instead of returning a finding or applying the default, breaking the policy gate for that workflow shape.

Useful? React with 👍 / 👎.

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