feat(actions): add pr-contract composite and carry-forward mode to ci-status - #550
Conversation
…-status Phase 3.1 of the CI/CD performance program: collapse the four required status checks every enrolled repository carries today into one, `ci-status`, so a title edit, a label change, or a body edit costs one cheap job instead of three `pull_request_target` workflows plus a full lane run. `pr-contract` is new. One step carries the Conventional Commits title check and the `do-not-merge` label check (both gate) plus the issue linkage check (advisory by default: a warning, one upserted comment carrying the marker `<!-- pr-contract:linkage -->`, and the `needs-issue-linkage` label, exit code unchanged). Semantics are ported from the `semantic-pr`, `do-not-merge-gate` and `pr-issue-linkage` reusables, including the negated-closer rule and the `Refs:` / `Relates to:` non-closing marker from #544. Those reusables are untouched; this is additive until Phase 3.4 retires their callers. `ci-status` gains an extracted `run.sh` with behaviour unchanged, its first test harness, and a carry-forward branch. Full mode aggregates as before and then records the verdict as a commit status under `ci-lanes` on the head SHA; on `edited`, `labeled` and `unlabeled` it skips aggregation and passes only when that status is `success` on the same SHA. A commit status, not the check-run list, is the carried signal because a check run cannot say which event produced it, so a chain of contract-only runs could otherwise self-certify. The status write is load-bearing: a write still refused after three retries fails the run naming the missing `statuses: write` permission. This repository dogfoods both in its own `ci.yml`: the three contract-only trigger types are added, `cancel-in-progress` excludes them so a contract-only run queues behind the full run it reads from, every job except `ci-status` carries the gate expression, and the `ci-status` job gains `pull-requests: write`, `statuses: write` and the two composite steps in order with the second under `!cancelled()`. 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 6m 3s —— View job Review complete
One inline finding posted, high confidence: Fork-originated PRs can never pass the new required Everything else held up well under review:
I was not able to execute the test suites directly in this sandbox (shell execution required approval that wasn't available), so the above is based on static reading of |
|
Claude finished @kyle-sexton's task in 6m 57s —— View job Claude finished the security review
Findings1. (High) The consolidated gate can be neutralized by editing the PR's own branch —
For This is exactly the tampering vector this repo's own predecessor reusable already documents and defends against: Failure scenario: a contributor opens a PR with a non-conventional title and a Today this is partly mitigated because 2. (Medium) Fork PRs get a forced-read-only GitHub always forces For a genuine fork-originated PR, that write will be refused every time, so every full-mode run fails the (soon-to-be sole) required check even when every real lane passed — and every later 3. (Low confidence — worth confirming) Fenced/inline-code masking in the issue-linkage analyzer may create a blind spot vs. GitHub's real closing-keyword scan
The negated-closer rule in the same file is built on the premise that "GitHub's own parser is negation-blind and closes the issue on merge regardless of the disclaimer" (run.sh#L153-L165) — i.e., GitHub's real scan is a dumb text match, indifferent to surrounding prose. If it's similarly indifferent to Markdown code fencing (unconfirmed here), a body with No issues found with the injection-safety of the PR content handling itself: both composites correctly read PR title/body/labels via |
![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: db7f749a5d
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Last security-reviewed head: |
Three lane failures from the first run of this branch, all in the new files: - shfmt: the root EditorConfig sets no `switch_case_indent`, so the CI lane formats `case` arms unindented. Reformat both runners and both harnesses with EditorConfig-driven `shfmt -w` rather than the `-ci` flag used locally. - comment-hygiene: a comment in the pr-contract harness carried a closing keyword next to a tracker reference, which the scanner reads as a deferred-work marker. Reword it. - ci-fanout-consolidation: the contract test pins ci.yml's `cancel-in-progress` expression verbatim, and the contract-only gate extends it. Update the assertion, keeping the #122 burst-collapse guard it exists to protect, and add three assertions that lock the rest of the dogfood shape: the six trigger types, the gate on every job except `ci-status`, and the `ci-status` job's permissions plus its two steps in order with the second under `!cancelled()`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 30s —— View job Security review complete
I re-ran a full security review against the current head, 1. (High) ci.yml#L3-L12 triggers on plain 2. (Medium) Load-bearing For Neither of these is new — both were raised by the Claude and Codex reviews on the first commit ( No new issues found in the formatting-only delta ( |
The repo's Biome lane covers `.github/scripts` against the fixture config; the new assertions were hand-formatted. No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kyle-sexton
changed the title
kyle-sexton
changed the title
kyle-sexton
added
do-not-merge
Review findings on #550 (amendments 3 and 4 to the Phase 3 design). The contract-only decision is now a single expression, repeated verbatim in `cancel-in-progress` and in every job `if:`: github.event.pull_request.head.repo.full_name == github.repository && (contains(fromJSON('["labeled","unlabeled"]'), github.event.action) || (github.event.action == 'edited' && !github.event.changes.base)) Two exclusions it adds are load-bearing. An `edited` event carrying `changes.base` changed the base branch, so the merge commit the lanes test changed with it and the run must be full. A fork pull request is never contract-only: its token is read-only on `pull_request` whatever `permissions:` requests, so it cannot record the lane state a later carry-forward would read, and it runs the full workflow on every event exactly as before. `cancel-in-progress` keeps its `github.event_name == 'pull_request'` guard ANDed with the negated predicate rather than replacing it. On a push there is no `github.event.pull_request`, so the predicate is false and `!(predicate)` alone would be true, re-arming the main-push burst collapse #122 disarmed. The contract test now pins that AND explicitly. In the ci-status composite, `event-action` and `carry-forward-actions` are replaced by `contract-only` (defaulting to the predicate itself) and `same-repo`. Carry-forward runs iff `contract-only` is true, branched on before `same-repo` so a caller that overrides it owns the claim that the lanes did not run. Full mode records the ci-lanes status only when `same-repo` is true; a fork prints a `::notice::` and exits on the lanes verdict alone, and the load-bearing failure on a refused write stays for same-repository runs. Five new harness cases: contract-only false aggregates and records (the base-change shape); both boolean inputs reject an unrecognised value; same-repo false skips the write and passes, and still fails on a failing lane; contract-only true with same-repo false is still carry-forward. The last is unreachable from the shipped defaults and is documented as caller responsibility in the pr-contract README. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 23s —— View job Security review complete
Re-ran a full security review against the current head, Finding #2 from prior reviews is now fixedThe fork/ Finding #1 is still open and unaddressed at this headThe For This is the same tampering vector Finding #3 (low confidence) is unchanged
No new issues foundThe predicate refactor itself is internally consistent: |
kyle-sexton
changed the title
kyle-sexton
changed the title
kyle-sexton
added
do-not-merge
Security review of #550: one blocking finding and four hardening items. BLOCKING. The carry-forward read the combined status endpoint, which collapses to one entry per context and exposes no author. Any collaborator with write can POST a commit status, so a forged `ci-lanes=success` followed by a label flip turned the sole required check green over failing lanes — the label event gates every lane off, and the carry-forward then trusted the forgery. It now reads the per-context LIST endpoint, which carries `.creator` and is newest-first, and accepts only the newest entry whose context matches AND whose creator is `github-actions[bot]` with `type == "Bot"`. A later bot failure therefore overrides an earlier bot success, and a later forged success by a user is skipped rather than shadowing the bot's verdict. Seven fixture cases cover it, including a user account whose login merely impersonates the bot. Also: - A predicate drift guard. `ci-fanout-consolidation.test.cjs` now compares the `contract-only` default text in the ci-status action metadata against the negated expression in every non-`ci-status` job gate, normalised for the folded block scalar. Drift there is silently catastrophic: lanes gated off while `contract-only` resolves false makes the composite aggregate all-`skipped` results and record `ci-lanes=success` for a run in which nothing executed. Ablation- tested — narrowing the action default to one action turned the test red, and it went green again on restore. The composite deliberately does not refuse an all-skipped aggregation; a docs-only pull request reaches that legitimately. - `find_marker_comment` selects only bot-authored comments carrying the marker and takes the newest, so a stranger's planted marker on a public repository cannot capture the advisory upsert. - One `escape_annotation` helper applies GitHub's documented workflow-command escaping (`%`, CR, LF) to every attacker-controlled string quoted in an annotation: the title, the author login, and the linkage messages that quote body text. - Both composites validate every value that reaches a `gh api` path before the first call — `sha`, `repository`, `pr-number`, `status-context` and both label inputs — and the label is percent-encoded in the label-removal path. Harness counts: ci-status 22 to 29 cases, pr-contract 41 to 48. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 26s —— View job Security review complete
The one new commit since the last-reviewed head, 1. Forged commit-status vector in 2. Workflow-command injection into annotations — now closed. 3. Marker-comment hijack and API-path injection — now closed. Standing finding — unchanged at this headThe trust-boundary issue raised by prior Claude and Codex reviews on this PR ( No new issues foundI was not able to execute |
kyle-sexton
changed the title
kyle-sexton
changed the title
kyle-sexton
added
do-not-merge
Code review of #550: one blocking finding and five cheap corrections. BLOCKING. The previous commit's input validation rejected legal values. GitHub labels may contain spaces and colons (`do not merge`, `status: blocked`) and a commit-status context such as `CI Lanes` is legal, so validating `do-not-merge-label`, `linkage-label` and `status-context` against a no-whitespace pattern would have failed real consumers. None of the three ever reaches an API path unencoded: the blocking label is compared with `grep -qxF`, the linkage label travels in a JSON body built by `jq --arg` and reaches only a percent-encoded DELETE path, and the context goes through `jq --arg` into a comparison or a payload. Those three checks are gone. The `sha`, `repository` and `pr-number` checks stay, because those two do land in a path. Also: - `url_encode` emitted code points, so an emoji or accented label was mis-encoded and the label was never removed. It is now `jq -rn --arg text "$1" '$text|@uri'`, which percent-encodes the UTF-8 bytes the API expects. - The carry-forward selected `.[0]` of the matching entries, depending on the API's documented ordering. It now takes `max_by(.id)`, stating the intent directly, as `find_marker_comment` already did. Both new cases feed an oldest-first list. - `expect_no_gh_call 'gh api'` could never fire: the shim logs `$*`, which begins `api -X GET`. Those seven assertions are now an empty-log check. Proven: with `require_pattern`'s `exit 1` removed, the two ci-status validation cases go red on it. - The title regex required exactly one space after the colon; the conventional-commits parser behind semantic-pr.yml accepts more, so `feat: two spaces` would have started failing. Now `: +`. - The `token` input description and the README record that the token must be `GITHUB_TOKEN` or a GitHub App token: the comment upsert only matches a `Bot` author, so a classic PAT would post a new comment on every failing run. The two ci-status `require_pattern` messages now route through `escape_annotation` like every other annotation. Harness counts: ci-status 29 to 31 cases, pr-contract 48 to 50. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 5s —— View job Security review complete
The one new commit since the last-reviewed head, No new issues foundI verified the three loosened validations against every sink each value reaches, and none regresses a previously-fixed issue:
The new Low-severity, non-blocking observation: three Standing finding — unchanged at this headThe I was not able to execute |
kyle-sexton
changed the title
kyle-sexton
changed the title
kyle-sexton
added
do-not-merge
…eltas (#570) No related issue: fleet fan-out of melodic-software/claude-code-plugins#3688 (closed); this repo's part of that remediation. ## Summary The fleet plugin list (standards `components/cloud-environment/fleet-plugins.json`, standards#542) is what every cloud snapshot installs, and the canonical bootstrap reads it overlaid with each repo's `.claude/settings.json`. A committed block that mirrors the whole catalog is therefore redundant in the cloud and, locally, writes one project-scope install record per entry into the user's `installed_plugins.json` on every session start in every checkout (claude-code-plugins#3813). This repo declares no per-repo deltas, so its block is reduced to `{}`. ## Fix - `.claude/settings.json`: `enabledPlugins` reduced to `{}`. `extraKnownMarketplaces` and everything else in the file is unchanged. A repo that later needs a plugin disabled adds a single `"<plugin>@melodic-software": false` entry; a repo that needs one outside the fleet list adds a `true` entry. Template and rationale: claude-code-plugins `docs/CLOUD-FLEET-SETUP.md`. ## Verification - `jq -e . .claude/settings.json` parses; `git diff` touches only the `enabledPlugins` block. - The cloud install path is unaffected: the snapshot installs the fleet list at build time (verified on stamp `2026-09-06.2` for claude-code-plugins and medley), and the canonical bootstrap's drift repair reads the same list from `/opt/melodic-fleet-plugins.json`. ## Related - melodic-software/claude-code-plugins#3688, #3917, #3813 - melodic-software/standards#542, #550 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016e6sBmnTGcidwvSrM86NYp Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
No related issue: melodic-software/github-iac#396 tracks Phase 3 of the ci-perf program
Summary
Phase 3.1 of the CI/CD performance program: collapse the four required status
checks every enrolled repository carries today (
ci-status,pr-title,do-not-merge,pr-issue-linkage) into one,ci-status, so a title edit, alabel change, or a body edit costs a single cheap job instead of three
pull_request_targetworkflows plus a full lane run.Two composite actions change here.
pr-contractis new and carries the wholepull-request contract in one step.
ci-statusgains an extracted runner, itsfirst test harness, and a carry-forward branch that lets a contract-only event
reuse the lane verdict a full run already recorded.
Fix
.github/actions/pr-contract/(action.yml,run.sh,run.test.sh,README.md). The Conventional Commits title check and thedo-not-mergelabel check fail the step; the issue-linkage check is advisory by default —
a warning, one upserted comment carrying the marker
<!-- pr-contract:linkage -->, and theneeds-issue-linkagelabel, with theexit code unchanged.
linkage-mode: enforceturns it into a gate. Semanticsare ported from the
semantic-pr,do-not-merge-gateandpr-issue-linkagereusables, including the negated-closer rule and the
Refs:/Relates to:non-closing marker from ci-workflows#544. Those reusables stay in place; this
is additive.
.github/actions/ci-status/: the inline aggregation moves intorun.shunchanged, gains
run.test.sh, and gains a carry-forward branch. In full modethe action records the lane verdict as a commit status under
ci-laneson thehead SHA; on
edited,labeledandunlabeledit skips aggregation andpasses only when that status is
successon the same SHA. The status write isload-bearing: a write still refused after three retries fails the run naming
the missing
statuses: writepermission, because the carry-forward branchreads nothing else.
ci.yml:edited,labeledandunlabeledjoin
on.pull_request.types;cancel-in-progressnow excludes those threeactions so a contract-only run queues behind the full run it reads from
instead of cancelling it; every job except
ci-statuscarries the gateexpression; and the
ci-statusjob gainspull-requests: write,statuses: write, and the two composite steps in order, the second underif: ${{ !cancelled() }}so a failing contract step never skips the statuswrite.
selector-contractlane runs both new harnesses; theshfmtlane coversboth new action directories.
pr-title.yml,do-not-merge.ymlandpr-issue-linkage-self.ymlareuntouched; Phase 3.4 retires them after the org ruleset changes.
Verification
bash .github/actions/pr-contract/run.test.sh—All pr-contract tests passed.(43 named cases: every allowed type includingsecurity, unknowntype, missing colon, empty subject, the breaking-change marker, scope under
both
require-scopevalues, the label gate, all nine closing-keyword forms,cross-repository closers, both no-issue markers,
Refs:accepted and bareRefsrejected, negated and contracted-negated closers, thenot onlyexemption, keywords masked inside HTML comments / fenced blocks / inline code
spans, each missing section, an empty section, a nested subsection, exempt
authors, comment create-then-edit upsert, 403 on both writes,
enforcemode,and the metadata contract).
bash .github/actions/ci-status/run.test.sh—All ci-status tests passed.(17 named cases covering both modes, the retry ladder, and the preserved
treat-skipped-asmessages).go red; with the carry-forward branch removed, six ci-status cases go red.
shellcheck --rcfile .shellcheckrcandshfmt -d -i 2 -ciclean on all fournew scripts;
actionlint .github/workflows/ci.ymlclean;zizmor --persona regular .github/adds no finding class this repository doesnot already carry (the new local
uses:produces the sameself-repositoryhelp the existing
ci-statusstep does);markdownlint-cli2clean on bothchanged markdown files.
yqover.jobsconfirms all 26 non-ci-statusjobs carry thecontract-only gate expression and
ci-statusalone does not.pr-contractinsideci-status, a title edit re-runs onlyci-status, and thedo-not-mergelabel flips it red and its removal flips it green.
Related
Refs melodic-software/github-iac#396
Refs melodic-software/github-iac#378