Repository navigation
feat(pr-convention-policy): check the pr-contract composite and describe the advisory linkage posture - #525
Merged
Merged
Conversation
…ibe the advisory linkage posture Phase 3 of the ci-perf program moves every consumer from the `pr-issue-linkage.yml` reusable to the `pr-contract` composite called as a step inside `ci-status`, one repository at a time. The lockstep lane read the reusable and only the reusable, so it would have gone red the moment the first consumer migrated and blind the moment the last one did. The live gate source is now `.github/actions/pr-contract/run.sh` on ci-workflows `main`, plus `action.yml` for the allowed title types the composite hardcodes as its `types` default. Per consumer the check detects which artifact the repository runs, resolves that pin, and validates the contract at that pin with the matching extractor, so a mixed fleet passes throughout the transition and a drifted artifact of either kind still fails. The distributed rule and the component README stop calling `pr-issue-linkage` the required check: `ci-status` is the single required check, the title and the `do-not-merge` label fail it, and a non-conforming body gets an advisory comment and the `needs-issue-linkage` label instead of a red check. 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. |
|
Claude finished @kyle-sexton's task in 5m 47s —— View job Code review complete
Note: FindingsNo correctness or maintainability issues found in the diff ( I traced through the new extraction/comparison logic in
Unable to verify: the PR's stated verification results ( One environment note unrelated to the PR content itself: the local working tree in this session has an uncommitted, unstaged reversion of |
![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: a40ebb054c
ℹ️ 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 has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ries The comment-hygiene lane rejected a test comment quoting a closing keyword beside an issue number; reworded without changing what the test asserts. Artifact detection now requires a `uses:` entry with no `#` earlier on the line. The composite path also appears in ci-workflows' own `ci.yml` as a lint `paths:` list and as a `run.test.sh` command, and a migration leaves commented-out call sites behind; either would otherwise select an artifact the repository does not run and suppress detection of the one it does, so a repository that had lost its last live caller would validate an unused artifact instead of reporting that it runs neither. Raised by the Codex reviewer on #525. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ies are exempt The comment claimed the org `ci-gate` ruleset does not cover agent-plugins, claude-code-proxy and cursor-plugins. `distribution/sync-manifest.yml` records that github-iac#367 already extended the ruleset to all three and that each carries the four required contexts, so that claim is stale. What is still true is that they sit outside the `pr-body-contract-rule` sync roster, which has nine targets. The exemption itself is unchanged and should shrink to nothing at Phase 3.3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ck require-scope Two decisions from the review on #525. The three-repository exemption is gone. agent-plugins, claude-code-proxy and cursor-plugins were reported rather than failed while their default branches sat outside the org ci-gate ruleset; `distribution/sync-manifest.yml` records that github-iac#367 extended the ruleset to all three and that each carries the required contexts, so a repository running neither contract artifact is now a drift finding wherever it happens. `checkFleet` returns a plain error list and the note channel is gone with the exemption it served. `inputs.require-scope.default` joins the composite comparison, at the source and at every pin. A composite release that flipped it would reject every unscoped title fleet-wide while the section and type checks still reported agreement. Either YAML spelling of the boolean is accepted; anything else is drift rather than a coerced guess. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Sep 5, 2026
…view callers to it (#527) No related issue: melodic-software/github-iac#403 tracks Phase 3b of the ci-perf program ## Summary Phase 4 and Phase 5 of the ci-perf program move the private four and medley off the governed runner selector and onto the managed fleet label written literally. Two of the files that would have to change are sync-managed by this repository: `claude-review.yml` in dotfiles, provisioning, github-iac and medley, and `claude-security-review.yml` in github-iac and provisioning. The `managed-files-guard` lane fails a hand edit and the next sync reverts it, so the change is made once here and distributed. The gate those repositories run refuses a hardcoded managed label outright today (`raw-self-hosted-label`, "consume the approved selector output"), and the four `ci.yml#ci-status` local-routing grants added in Phase 3.2 stop applying the moment the literal replaces the selector expression, at which point each one fires as an unconsumed grant while the job it was written for fires `privileged-hosted-only`. Both halves have to land before any Phase 4 pull request opens. ## Fix `components/runner-policy/runner-policy.mjs` - New routing target kind `managed-literal`: a plain string that is an exact member of the new required policy key `approvedManagedRunnerLabels`, classified after the `${{ … }}` catch-all so an unrecognised expression that merely mentions the label is still refused. Admission is deliberately not a `managedLabelPatterns` match: those patterns are an unanchored, unbounded DETECTION surface whose job is to catch every label-shaped string and refuse it, so using them to admit would invert the safe bias and hand an unreviewed near-miss such as `melodic-anything-ubuntu-24.04-x64` the same trust as the real fleet label. Entries are validated at load: surrounding whitespace, a known GitHub-hosted label, or a string that does not itself read as a managed label is a configuration error. Approval is unconditional in the classifier and the existing routing gate refuses it when routing is disabled, which is exactly how `selector-output` is handled, so a public repository keeps reporting `public-self-hosted-routing` and a private repository with `selfHostedCi: false` keeps reporting `self-hosted-routing-disabled`. - One predicate, `locallyRoutedTarget`, now stands for the two ways a job reaches the fleet, and every gate reads it: grant applicability and grant consumption (which have to stay exact complements, or a grant admits a job and then fires inventory drift on the same run); the reviewed caller-permission waiver and the reviewed secret boundary (without which a fleet-literal `claude-review` caller reports `privileged-hosted-only`); the explicit-read-only floor; and the local-execution test inside `auditLocalPermissionFlow`. - That last one closes a hole. A job reached through a repository-local `workflow_call` whose own `runs-on` named the fleet label matched neither the local nor the hosted execution test, so the flow pass fell through and emitted nothing: a called job inheriting a write-capable caller token ran on a persistent host with no finding at all. medley's gateway plus its eight `workflow_call` workers are that shape until Phase 5 collapses them. - `raw-self-hosted-label` keeps every case it should still catch: a literal containing `self-hosted`, a `vars.CI_SELF_HOSTED_LABEL` or `CI_MANAGED_RUNNER` reference, and a managed label on any repository that is not an enrolled private consumer. Only the exact label a routing-enabled job resolves to is skipped, in `runs-on` and in an approved reusable call's `runner` input alike. - Two messages change and no rule id does. `privileged-hosted-only` now says the job requires a reviewed `localRoutingGrants` entry or a hosted exception; `hosted-exception-required` now says an eligible private job must name the governed fleet label or declare its key in `.github/runner-policy.json`. The id stays because it appears in `THREAT-MODEL.md`, throughout the test suite (25 occurrences) and in merged pull-request bodies. `components/runner-policy/policy.json` and `policy.schema.json`: two additions. The code-review lane contract at `0f8176e87e0be518f382664779655011bf95784a` widens its reviewed input list to `runner, pr-number, timeout-minutes`, all three of which the reusable declares at that revision. `approvedManagedRunnerLabels` is a new required key carrying the two live fleet labels (`melodic-ubuntu-24.04-x64`, `melodic-review-ubuntu-24.04-x64`), so adding a label to the fleet is a reviewed, data-only policy change exactly like adding an approved hosted label. Both labels every planned wave writes are already in the set, so no downstream repository change needs a policy edit; the `kyle-*` personal-repository namespace stays detection-only and would need its own reviewed entry. Making the key `required` is safe to ship because `policy.json`, `policy.schema.json` and `runner-policy.mjs` are one component in `distribution/sync-manifest.yml`, so a sync moves them atomically and no consumer can run a new checker against an old policy. `approvedSelectorReferences`, `approvedSelectorReferencesByRepositoryOwner`, `approvedSelectorInputContracts` and `canonicalSelectorSecrets` are byte-unchanged, and the `selector-pin` and `selector-contract` rules are untouched: three of the five sync targets are still selector-routed when this reaches them, so a grammar that refused selector routing would turn every sync pull request red. `components/claude-lanes/claude-review.yml`: the `select-review` job is deleted and `runner` is the literal `melodic-review-ubuntu-24.04-x64`. Triggers keep `[opened, ready_for_review, reopened]` and gain a `workflow_dispatch` with a required `pr-number`. The draft filter is event-guarded (`github.event_name == 'workflow_dispatch' || github.event.pull_request.draft == false`), because a dispatched run has no `github.event.pull_request` and a bare draft comparison would refuse the review the operator just asked for. `timeout-minutes` is 15, below the reusable's 30-minute default, since the lane is advisory. Both concurrency blocks stay, including the repository-wide `queue: max` seat group. `components/claude-lanes/claude-security-review.yml`: same selector deletion, same literal, a plain draft filter, and `synchronize` dropped. The filter is plain rather than event-guarded because this lane has no `workflow_dispatch` trigger, so `pull_request` is the only event that can start it and `github.event.pull_request` is always present; the comment says that adding a dispatch trigger later means adding the guard with it. The rationale for keeping `synchronize` was that a required execution-evidence check must report on the latest head, and that constraint is not live: github-iac ADR 0011 makes agentic reviews advisory and forbids any organization ruleset from making their status a merge or deployment prerequisite, and `security-review-gate` is disabled. No `timeout-minutes` and no `pr-number`: the reusable exposes neither at this pin nor at `main`. Its per-pull-request group with `cancel-in-progress: false` stays. Both callers keep the pin they already carried, `0f8176e87e0be518f382664779655011bf95784a`. `components/claude-lanes/repin-policy-lockstep.mjs`: `updateTestMjs` is gated on a selector copy-forward having actually happened. The selector repin target already read zero pins from a selector-less caller and skipped, so `copySelectorContract` could not run and `policy.json` never gained the entry, but the test file would have gained a constant for it anyway on the next ci-workflows tag. README and THREAT-MODEL record the new target kind, the widened grant applicability, the new threat row for a fleet label named where routing is not enabled, and the accepted residual: the literal has no hosted recovery fallback and no failure sentinel, so a job queues when no runner is online. That is a different control's job (fleet queue-depth alerting, and a second host), and it is written down rather than implied. ## Verification - `npm run test:runner-policy` exits 0: 287 tests, 287 pass, up from 274. Thirteen new cases plus one rewritten. New coverage: a fleet literal admitted on a private `selfHostedCi: true` repository; refused on public with `public-self-hosted-routing` and on `selfHostedCi: false` with `self-hosted-routing-disabled`; a config claiming `private` under public visibility evidence raising the configuration error the CLI reports as exit 2; a hosted literal still requiring a named exception on an enrolled private repository and still clean on public and on `selfHostedCi: false`; a write-token job on the fleet label refused without a grant, admitted with a matching one, and refused with the "outside the reviewed local-routing grant" message when the map differs; a fleet-literal job with omitted permissions still refused; a grant keyed to a fleet-literal reusable-call job still surfacing as `local-routing-grant-drift`; a reusable call whose `runner` input is the fleet review label admitted under `allowedCallerPermissions`; a repository-local call whose jobs run on the fleet label reported as `local-reusable-permissions`; the Phase 3.2 `ci-status` grant shape auditing clean both selector-routed and on the literal; five unreviewed near-miss and adjacent-text label shapes refused; and three `approvedManagedRunnerLabels` entries that must fail configuration. - Every one of those new cases was run against the unmodified grammar first and failed there: thirteen failures on `origin/main`'s `runner-policy.mjs`, zero after. - The two lane-caller tests that were keyed to `/select-runner.yml@` now recognise a managed label as well, and both assert that at least one caller was actually checked, so they cannot shrink to nothing when a caller drops its selector job. - `bash components/claude-lanes/claude-lanes.test.sh`: 32 assertions, all pass. Both rewritten callers materialize through `sync-manifest.sh apply` and lint clean under `actionlint` 1.7.12 at every managed target. Harness note for the next person running this locally: `sync-manifest.sh apply` compares the source worktree bytes against the indexed object, so an uncommitted, unstaged edit to a component file fails the suite with "source worktree bytes differ from the indexed object" rather than with anything about the edit. Stage first. - `node --test components/claude-lanes/repin-policy-lockstep.test.mjs`: 7 tests, 7 pass. - `bash components/claude-lanes/repin-callers.test.sh`: 116 assertions, exit 0. - `GITHUB_REPOSITORY=melodic-software/standards CI_REPOSITORY_VISIBILITY=public npm run lint:runner-policy` exits 0. - `npm run lint:md` clean; `npx @biomejs/biome check components/runner-policy components/claude-lanes` reports only the pre-existing `noTemplateCurlyInString` warning on `repin-policy-lockstep.mjs:300`. lefthook pre-commit ran on both commits and was never bypassed. - Acceptance run required by the transition window: this branch's checker and this branch's `policy.json` against a scratch clone of each sync target's current `origin/main` `.github/` tree, with `GITHUB_REPOSITORY` and `CI_REPOSITORY_VISIBILITY` set per target. All five exit 0: dotfiles 0, github-iac 0, provisioning 0, medley 0, claude-code-plugins 0 (public). The same five run against `origin/main`'s checker and policy also exit 0, so this change neither breaks nor newly silences any of them. ## Related - Refs melodic-software/github-iac#403 - Refs melodic-software/github-iac#378 - Phase 3.5 landed in #525, which is still open; that pull request and this one touch disjoint components. - After merge this reaches five targets through `distribution/sync-manifest.yml`, one sync pull request each: dotfiles and medley take `runner-policy` and `claude-review-caller`, github-iac and provisioning take those plus `claude-security-review-caller`, and claude-code-plugins takes `runner-policy` only. Each sync pull request must be green on its own `runner-policy` lane and must merge before that repository's Phase 4 or Phase 5 pull request opens. `distribution/sync-manifest.yml` is not edited here; no target list changes. - A separate github-iac pull request amends ADR 0011 to record that `security-review-gate` stays disabled and that the security lane therefore drops `synchronize`. It is not in this repository and is not opened here. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
Author
|
Lane claim: babysit-loop
Advancing this PR through the repo's tracked merge configuration. This claim is released when the lane finishes or the expiry passes, whichever comes first. Generated by Claude Code |
Contributor
Author
|
Lane release: babysit-loop — not merged, held on the Phase 3.3 apply Lease released; the claim above is withdrawn. No merge, no push, no thread resolution was performed. Head is unchanged at Why this did not merge, despite The scout read this PR as independent and closest to done. That is wrong, and the evidence is on this PR and its tracker:
So Remaining owner-only steps (from github-iac#396): dispatch Second, independent hold. The deterministic merge gate could not run in this session, so merge-readiness is unproven, never merge-ready. It exited 2 with a Also noted, not acted on: the branch is Instance Generated by Claude Code |
kyle-sexton
added a commit
to melodic-software/cursor-plugins
that referenced
this pull request
Sep 5, 2026
…27) No related issue: melodic-software/github-iac#396 tracks Phase 3 of the ci-perf program ## Summary Deletes the three pull-request contract workflow callers that `ci-status` has replaced: `.github/workflows/pr-title.yml`, `.github/workflows/do-not-merge.yml` and `.github/workflows/pr-issue-linkage.yml`. Nothing else changes; `ci.yml` is untouched. Since cursor-plugins#26 (merged as `855cc8994109fc1a32066780b8ebb29c787cc887`) the `ci-status` job runs the `pr-contract` composite, which performs the same three checks in one job: the Conventional Commits title check, the `do-not-merge` label check, and the issue-linkage check. Phase 3.3 then reduced the org `ci-gate` ruleset to require only `ci-status` on every enrolled default branch (github-iac run 33954697296), and the standards lockstep re-point (melodic-software/standards#525) merged, so the three callers no longer back a required check and nothing reads them. ## Fix - `git rm .github/workflows/pr-title.yml` - `git rm .github/workflows/do-not-merge.yml` - `git rm .github/workflows/pr-issue-linkage.yml` Three files, 77 deletions, no insertions. No `exempt-authors` value carries over. This repository's `pr-issue-linkage.yml` caller passed only `runner: ubuntu-24.04`, so the `pr-contract` step in `ci.yml` needs no new input and `ci.yml` is not edited. The reusable workflows in `melodic-software/ci-workflows` (`semantic-pr.yml`, `do-not-merge-gate.yml`, `pr-issue-linkage.yml`) are not touched; they are retired separately in Phase 7. The `do-not-merge` and `needs-issue-linkage` labels are still real and stay: `pr-contract` reads the first and writes the second. ## Verification Run locally in the worktree at `5351c25ef8687438054ee47ed1265a4664a9b512`: - `grep -rl "pull_request_target" .github/workflows/` returns nothing. The three deleted callers held the last `pull_request_target` triggers in this repository. - `ls .github/workflows/ | grep -cE "^(pr-title|do-not-merge|pr-issue-linkage(-self)?)\.yml$"` prints `0`. - `actionlint .github/workflows/*.yml`: one finding, the pre-existing `SC2016:info` note inside the `links` lane of `ci.yml` at `:181:9`, present byte-identical on `origin/main`. Nothing new. - `zizmor --persona regular .github/workflows/`: `8 findings (3 suppressed)`, `1 low, 4 medium`, versus `17 findings (3 ignored, 9 suppressed)`, `1 low, 4 medium` on `origin/main`. The five reported findings are the same four `warning[artipacked]` and one `help[adhoc-packages]` in `ci.yml`. The drop is the deleted files' own findings and suppressions. Nothing new. - `yq '.permissions' .github/workflows/ci.yml` prints `contents: read`, not null. - `git diff origin/main --stat` shows only the three deletions. The repository has no `package.json`, no lefthook and no test that enumerates workflow files. A repository-wide grep for `pr-title`, `do-not-merge` and `pr-issue-linkage` across Markdown, YAML, JSON, JavaScript, PowerShell, Python and shell files found no documentation or test that describes the deleted workflows as checks; the only surviving mentions are two comment lines in `ci.yml` that name the three contract answers the `ci-status` predicate covers, which stay correct. ## Related Refs melodic-software/github-iac#396 Refs melodic-software/github-iac#378 Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No related issue: melodic-software/github-iac#396 tracks Phase 3 of the ci-perf program
Summary
Phase 3 of the ci-perf program moves every consumer from the
pr-issue-linkage.ymlreusable to thepr-contractcomposite, called as a stepinside the
ci-statusjob, one pull request per repository over the comingdays. The
pr-convention-locksteplane read the reusable and only thereusable: it would have gone red the moment the first consumer migrated, and
blind the moment the last one did. It has to stay green at every point of that
transition, and it has to keep catching drift on both sides of it.
Two other standards-owned surfaces still described
pr-issue-linkageas arequired check that fails on a non-conforming body. After Phase 3 that is
wrong in a way that matters to anyone drafting a pull request:
ci-statusisthe one required check, and the body contract is advisory.
Fix
components/pr-convention-policy/lockstep-drift.mjs.github/actions/pr-contract/run.shonci-workflows
main, plusaction.ymlfor the allowed title types. Extractorsand their anchors: required sections from the
section_report("<name>")calls; closing keywords from the
match(chunk, /.../)expression; no-issuemarkers from the
tolower(body) ~ /.../expression; allowed title types frominputs.types.defaultinaction.yml, parsed as YAML so the anchor is thedata key rather than the shape of the description block above it; the scope
requirement from
inputs.require-scope.defaultthe same way. Both awkpatterns are probed functionally, with every policy keyword and marker, and
the probes lowercase because the analyzer does; the extractor asserts that
lower = tolower(line)is still there rather than assuming it, which is thecomposite's equivalent of the reusable's
iflag.its
typesdefault, sopolicy.jsonand the gate can now be checked againsteach other where
semantic-pr's eleven-type action default never was, andrequire-scopeis checked alongside it: a composite release that flipped itwould reject every unscoped title fleet-wide while the section and type checks
still reported agreement.
uses:ofthe composite in any workflow, else the
pr-issue-linkage.ymlcaller),resolves that pin, and checks the contract at that pin with the matching
extractor. Detection requires a
uses:entry with no#earlier on the line,so a commented-out call site left behind by a migration, or the same path in a
lint
paths:list, never selects an artifact the repository does not run. Thecomposite wins where a repository carries both mid-transition; ci-workflows'
local
./.github/actions/pr-contractreference resolves to its own tree atmain. A repository running neither artifact fails the lane, with noexemption: github-iac#367 put every repository in the roster under the org
ci-gateruleset, so a lost caller is a gate removal wherever it happens.and the distributed rule are unchanged.
.claude/rules/pr-body-contract.md(the always-loaded distributed rule): statesthat
ci-statusis the single required check, that it fails on anon-conforming title or a
do-not-mergelabel, and that a body missing theclosing keyword or a section draws an advisory comment and the
needs-issue-linkagelabel rather than a red check. The contract itself(sections, keywords, no-issue markers) is unchanged.
components/pr-convention-policy/README.md: names the composite as the gateconsumers run from v0.20.0 (
449157aaa8e30f7b1457305d8048ebe6168e174a) withpr-issue-linkage.ymlas the predecessor each repository leaves during Phase 3,records that the composite's twelve-type default includes
securityand endsthe drift against the eleven-type
semantic-prdefault, and describes theadvisory linkage posture.
ADRs are untouched (Phase 9 owns them), as are
distribution/sync-manifest.yml,the runner-policy component and the claude-lanes component (Phase 3b owns
those).
Verification
node --test components/pr-convention-policy/lockstep-drift.test.mjs components/pr-convention-policy/pr-convention-policy.test.mjs— 47 tests,47 pass. New fixtures cover a composite consumer, a reusable consumer, a mixed
fleet, a types drift, a
require-scopedrift, a sections drift, everyrepository running neither artifact, artifact detection in all six shapes
(pinned composite, local composite, reusable, both, neither, commented-out),
and a composite that stops lowercasing.
LOCKSTEP_GITHUB_TOKEN=$(gh auth token) node components/pr-convention-policy/lockstep-drift.mjsagainst the real fleet —exit 0. Twelve repositories resolved to the reusable at their pins,
ci-workflows to the composite it dogfoods locally; every one of the thirteen
resolved an artifact.
npm run lint:md,npm run lint:hooks, andnpx @biomejs/biome check components/pr-convention-policyclean; lefthookpre-commit ran on every commit (typos, editorconfig, gitleaks, markdownlint,
biome all green).
git diff origin/main --stattouches four files and nothing else.pr-convention-locksteplane on this pull request is the live proof thatthe mixed fleet passes today.
Related
(
449157aaa8e30f7b1457305d8048ebe6168e174a), which ships thepr-contractcomposite this lane now reads.
docs/adr/0008-distribute-pr-body-contract-rule.md) states thelockstep decision this change re-points; it is not edited here.
distribution/sync-manifest.ymlafter merge, one sync pull request each.