feat(runner-policy): admit review-tier variable as a governed selector input

[kyle-sexton]

Summary

• canonicalSelectorInputs.self-hosted-label may now be either ${{ vars.CI_SELF_HOSTED_LABEL }} (default fleet tier) or ${{ vars.CI_REVIEW_SELF_HOSTED_LABEL }} (dedicated capped review tier). A claude-review caller passes the review variable from a separate select-review job so its review workload routes to that tier while the repository's other self-hosted jobs keep the default.
• exactCanonicalMap becomes set-membership: a canonical value is one exact governed expression or an exact set of them. Fail-closed is preserved — any expression outside the reviewed set is rejected. A single-string canonical value keeps identical exact-match behavior, so the string-valued selector-secret, reusable-secret, and reusable caller-permission maps are unchanged.
• Schema gains canonicalSelectorInputMap (each value is a nonWhitespaceString or a nonEmptyUniqueStringArray); canonicalSelectorSecrets stays string-only.
• Pins the ci-workflows#123 merge commit select-runner.yml@cdc5917c15aade1995bd810b60d818cadc635b52 in approvedSelectorReferencesByRepositoryOwner.melodic-software (the revision that admits melodic-review-ubuntu-24.04-x64 in the selector's strict allowlist), registered in the allowlist drift-guard test.
• Tests: the review variable is admitted; an ungoverned self-hosted-label expression is rejected (selector-pin, "must be one of"); and an end-to-end claude-review caller (dedicated select-review job + runner from its output under the feat(runner-policy): review privileged reusable callers #140 caller-perms waiver) audits to zero findings. That last test doubles as the flip agent's executable spec and proves the review label threads through claude-review's optional-default path without tripping the hardcoded default-label routing-condition guards.
• README documents the two-expression allowance and notes that the required-no-default local runner-input form still routes only to the default tier.

Scope / retained fallback

The consumer runner fallback || 'ubuntu-24.04' is intentionally retained for now — it is the selector-failure recovery path. The floor-conversion wave (owner directive: zero hosted minutes on private repos) will revisit inert hosted fallbacks separately; this PR keeps them so it stays a focused input-contract change.

Merge order

1. ci-workflows#123 — merged (admits the label in the selector's strict allowlist).
2. This PR — SHA pin included; mergeable on green.
3. github-iac#141 (provisions CI_REVIEW_SELF_HOSTED_LABEL) can merge in parallel.
4. Campaign A flip consumes all three.

This PR and the wave-12 reusable-contract PR both touch components/runner-policy/policy.json, but in disjoint objects — this one edits canonicalSelectorInputs and approvedSelectorReferencesByRepositoryOwner; wave-12 edits approvedReusableWorkflowContracts. Whichever merges second rebases onto the first; no semantic conflict.

Validation

• node --test components/runner-policy: 146 passed (3 new + the drift-guard updated for the new SHA).
• Biome, markdownlint, typos, editorconfig, gitleaks (lefthook): clean.
• Schema compiles under Ajv 2020 at module load; shipped policy.json validates.

Related

• Refs melodic-software/github-iac#78 (epic), melodic-software/provisioning#135 (review-tier governance decision, step 3), melodic-software/provisioning#141 (tier config).
• Companion PRs: ci-workflows#123 (selector admission, merged), melodic-software/github-iac#141 (org variable).

No linked issue.

🤖 Generated with Claude Code

https://claude.ai/code/session_01K3QehVwmWzkBLpKokNCkkt

[claude]

Claude finished @kyle-sexton's task in 3m 53s —— View job

---

Review: feat(runner-policy): admit review-tier variable as a governed selector input

• Load tools and set up todo list
• Read changed files and diff
• Check REVIEW.md for review criteria
• Analyze schema changes
• Analyze policy logic changes
• Analyze test coverage
• Analyze README documentation
• Post review findings

---

The change is architecturally sound and fail-closed. No blocking correctness or security issues found. Notes below, ordered by significance.

---

Pre-merge hard dependency (informational, not a code defect)

The PR body correctly flags this, but worth re-stating explicitly in the review record: the approvedSelectorReferencesByRepositoryOwner.melodic-software array in policy.json does not yet contain the select-runner.yml SHA that admits melodic-review-ubuntu-24.04-x64. Without that entry, a consumer's select-review job references an un-approved revision and the audit rejects it at selector-pin. The PR body requires ci-workflows#123 to merge first and its merge-commit SHA to be appended here before this PR merges. This is an intentional, documented hold — just making sure a reviewer doesn't accidentally approve a merge before that commit lands.

---

🟡 Nit — rawManagedLabel misclassifies a mis-placed review-tier expression

runner-policy.mjs lines 1395–1404

function rawManagedLabel(value, policy) {
  const trimmed = value.trim();
  if (trimmed.toLowerCase().includes("self-hosted")) { return true; }
  if (/CI_(?:SELF_HOSTED_LABEL|MANAGED_RUNNER)/i.test(trimmed)) { return true; }
  return policy.managedLabelRegexes.some((pattern) => pattern.test(trimmed));
}

CI_REVIEW_SELF_HOSTED_LABEL contains neither the hyphenated self-hosted substring nor the alternation CI_(SELF_HOSTED_LABEL|MANAGED_RUNNER) after CI_. If a consumer mistakenly placed ${{ vars.CI_REVIEW_SELF_HOSTED_LABEL }} directly in runs-on, the raw-self-hosted-label rule would not fire. The workflow is still rejected — the general expression check at line 1370 produces a runner-target-contract finding instead — so enforcement is intact. The only consequence is rule misclassification in the finding output.

If diagnostic fidelity matters for downstream consumers parsing finding rules, extending the alternation to CI_(?:(?:REVIEW_)?SELF_HOSTED_LABEL|MANAGED_RUNNER) would give the right rule name. Fix this →

---

🟡 Nit — optionalCanonicalSelectorInputs schema type not upgraded

policy.schema.json line 53

"optionalCanonicalSelectorInputs": { "$ref": "#/$defs/stringMap" },

canonicalSelectorInputs now references canonicalSelectorInputMap (which allows string | string[]), but optionalCanonicalSelectorInputs remains stringMap (string-only). This is internally consistent today — no optional input currently needs two canonical expressions, and the review-tier label was added as a required input. If a future optional input ever needs the same two-expression treatment, that would require another schema change. A brief schema comment (or doc note in the README alongside the existing paragraph) explaining the deliberate asymmetry would prevent a future contributor from wondering why only one of the two maps supports arrays.

---

Correctness: set-membership logic is correct

runner-policy.mjs lines 748–774

for (const [name, expected] of Object.entries(required)) {
  const allowed = Array.isArray(expected) ? expected : [expected];
  if (!allowed.includes(actual[name])) {
    return `${location}.${name} must be ${canonicalExpectation(expected)}`;
  }
}

The Array.isArray branch is transparent to single-string canonicals (they're wrapped in a one-element array and includes behaves identically). The new code path is only triggered when policy.json contains an array-valued canonical. The fail-closed invariant holds: any expression absent from the reviewed set is rejected.

---

Test coverage is sufficient

Three new tests cover the three meaningful cases introduced:

• line 1418 — review variable admitted
• line 1430 — ungoverned variable rejected with must be one of (correct given the change from string to array canonical)
• line 1445 — end-to-end claude-review caller with a dedicated select-review job audits to zero findings, proving the label threads through without tripping the hardcoded CI_SELF_HOSTED_LABEL routing-condition guards in selfHostedSelectorConditionStatus and unroutableFailureStatus (which are only reachable via required-no-default mode, not the optional-default path this caller uses)

The rejection-message assertion at line 1442 (/selector inputs\.self-hosted-label must be one of/) correctly reflects the new array-valued canonical producing canonicalExpectation(["...", "..."]) → "one of [...]".

---

Summary: Two nits (rule misclassification edge case, undocumented schema asymmetry), one documented hard merge-order dependency. No blocking issues. Enforcement is fail-closed throughout.

[claude]

Claude finished @kyle-sexton's task in 1m 22s —— View job

---

• Read the new commit and understand what changed
• Review the selector SHA pin changes
• Update comment with findings

---

Review of d0cdc03 — selector SHA pin

This commit resolves the one hard merge-order dependency flagged in the prior review: the select-runner.yml@cdc5917c SHA (ci-workflows#123 merge commit) is now in approvedSelectorReferencesByRepositoryOwner.melodic-software.

Placement is correct. The new SHA lands only in the owner-scoped list, not in approvedSelectorReferences (the global base list). That's the right call — cdc5917c admits melodic-review-ubuntu-24.04-x64, which is melodic-software-specific and should not be globally available to arbitrary owners.

Test coverage is complete. The drift-guard test at runner-policy.test.mjs:1045 now asserts the exact allowlist shape including REVIEW_TIER_SELECTOR_SHA, and the per-SHA loop at line 1071 verifies that a workflow pinned to this revision audits to zero findings under a melodic-software owner identity. Both checks are required and both are present.

SHA format: 40-character full SHA (cdc5917c15aade1995bd810b60d818cadc635b52) — consistent with every other entry in the list.

No new issues. The two nits from the prior review (rawManagedLabel regex misclassification, optionalCanonicalSelectorInputs schema asymmetry) are unchanged — they were already assessed as non-blocking.

---

This PR is now complete per its stated merge pre-conditions. The blocking hold is resolved; the enforcement chain is closed; tests confirm zero findings end-to-end.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0cdc03017

ℹ️ 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]

Claude finished @kyle-sexton's task in 2m 0s —— View job

---

• Load tools and set up todo list
• Read the new commit (db209f7) and understand what changed
• Analyze Codex P2 findings against current state
• Post review findings

---

Review of db209f7 — selector trust note

This commit adds the cdc5917c review note to the selector revision lockstep paragraph in README.md and updates the counts (seven → eight total, four → five owner-scoped).

Codex P2 finding #2 (policy.json:15) is resolved by this commit. The trust note now documents that cdc5917c15aade1995bd810b60d818cadc635b52 adds melodic-review-ubuntu-24.04-x64 to the strict self-hosted-only allowlist, that the control-plane job still runs on the default fleet label, and critically: "A review lane must pin this revision: older approved revisions do not admit the review-tier label, and a self-hosted-only selector at an older pin fails closed on it (unapproved-label)." The Codex reviewer evaluated a pre-note state that no longer exists.

---

Codex P2 finding #1 — canonical inputs are not SHA-scoped

policy.json line 174–177

The Codex finding is technically correct in its observation: selectorStatus() approves CI_REVIEW_SELF_HOSTED_LABEL for any approved selector SHA, not only for cdc5917c. A consumer that pins to an older globally-approved SHA (e.g. 99ac2f8c) and passes CI_REVIEW_SELF_HOSTED_LABEL will pass the policy audit. At runtime, however, the older selector rejects the label with unapproved-label — fail-closed semantics hold.

The claim that this "defeats the selector lockstep" overstates the risk. No runner routing bypass is possible: the selector's internal allowlist is the enforcement layer, and the README now explicitly documents the fail-closed behavior for this exact scenario. The current layered design (canonical inputs are SHA-agnostic; the selector enforces its own allowlist) is intentional — making canonical inputs SHA-aware would require a significant schema redesign (a SHA → allowed inputs map), and the fail-closed runtime outcome removes the security rationale for doing so.

The finding does identify a real developer-experience gap: a consumer could pass audit and then receive a confusing unapproved-label failure at runtime with no audit-time signal. The README's guidance ("A review lane must pin this revision") is the mitigation, and it's now present. This is non-blocking.

---

This PR is complete. All three commits reviewed; no new blocking issues. Codex finding #2 is resolved; finding #1 is addressed by documentation and is non-blocking given fail-closed runtime enforcement. The two earlier nits (rawManagedLabel regex, optionalCanonicalSelectorInputs schema asymmetry) remain non-blocking and unchanged.