fix(ci): run the test gate on stacked child PRs, and label PRs whose title carries no type - #979
Merged
Conversation
Two defects share one symptom report, and only one of them is about stacked PRs. `ci.yml` filters `pull_request` on `branches: [main, dev]`, which GitHub matches against the BASE ref. A stacked child's base is another open PR's head, so the workflow is never queued: the #951-#955 stack carried 24 changed files under `src/` and 748 added lines with `enforce-target`, `label`, and `react-doctor` as its only check-runs. AGENTS.md calls stacked children an intentional review workflow and `enforce-target` already exempts them from the wrong-base gate, so the test gate is the one place that never got the memo. The labeler is a separate bug the stack happened to expose. `stack 1/5:` fails the conventional-commit regex — the `1/5` sits between the word and the colon — and the sentence-case fallback then extracts `stack`, which has no entry in PREFIX_TO_LABEL. The sync skips, a skip is not a failure, and the `label` check stays green while all four PRs carry no type label. The audit killed the first design. It chose `branches: [main, dev, "codex/**"]` on the theory that stacked bases live in the `codex/` namespace; listing real open PR head refs falsified that (codex/ 14, but also fix/ 4, feat/ 3, agent/ 3, split/, ingw/). Any of those can become a stacked base, and a contributor stack is precisely the case that most needs CI — the allowlist would have fixed the maintainer half and left the contributor half silently unverified. So the filter goes and the untouched `paths:` stays the scope gate, which is already how issue-quality-tests.yml is written. The audit also caught 020 asserting that the conventional regex matches `stack 1/5:`. It does not; both docs now name the sentence-case path. Docs only — no workflow or script changes in this commit.
`ci.yml` filtered `pull_request` on `branches: [main, dev]`, and GitHub matches that against the BASE ref. A stacked child's base is another open PR's head branch, so the workflow was never queued: the #951-#955 stack merged with `enforce-target`, `label`, and `react-doctor` as its only check-runs, and no test job at all, for 24 changed files under `src/`. That contradicts the repository's own design. AGENTS.md calls stacked children an intentional review workflow, and `enforce-target` implements the exemption by listing open PRs and matching `other.head.ref === pr.base.ref`. The gate accepts them; the check workflow never saw them. The filter is removed rather than extended. An allowlist cannot express "base is another PR's head": open PR head refs today are `codex/` (14) but also `fix/` (4), `feat/` (3), `agent/` (3), `split/`, and `ingw/`, and any of them can become a stacked base. A `codex/**` glob — the first draft of this change — would have fixed the maintainer's own stacks and left contributor stacks silently unverified, which is the worse half of the bug. `paths:` is untouched and is the real scope gate, so docs-only and devlog-only PRs still queue nothing. The precedent is already in this repository: `issue-quality-tests.yml` runs `pull_request` with `paths:` and no `branches:`. Widening is safe here specifically because this workflow is `pull_request` (not `pull_request_target`), declares `contents: read`, and reads no secrets — the same change would not be safe on `enforce-pr-target.yml` or `pr-labeler.yml`, which are untouched. `push:` stays pinned to the integration lines. The existing "PR checks reach every branch the target gate accepts" test asserted the opposite contract, which is how this survived: it pinned both workflows to `["dev", "main"]` and asserted the trigger key set as `["branches", "paths"]`. Its INTENT is exactly this fix — commit 5229717 wrote it so "an accepted PR is also a checked one" — so it is extended to the gate's stacked exemption rather than deleted: `ci.yml` is now asserted to carry no base filter, `service-lifecycle.yml` keeps its list (it gates the release service path, not review), and the gate itself is pinned as base-unfiltered. The block's type annotation did not even model `branches` on the PR trigger, so the second assertion adds it and pins its absence. Driven red: restoring `branches: [main, dev]` fails exactly that one test. Verified: bun test tests/ci-workflows.test.ts 83 pass; typecheck and privacy:scan pass.
All four PRs of the #951-#955 stack carried no type label while the `label` check reported success. `stack 1/5:` fails the conventional-commit regex — the `1/5` sits between the word and the colon — and then reaches the sentence-case fallback, which extracts `stack`. That has no entry in PREFIX_TO_LABEL, so `planTypeLabelSync` returns `{skip: true, reason: "no-prefix"}`, and a skip is not a failure. The check stays green and nothing is labeled. This is not a stacked-PR bug; the labeler has no branch filter and ran fine on all four. It is a title-vocabulary bug that the stack happened to expose: any title with an unrecognised prefix word is silently unlabeled. The commits underneath are conventional even when the title is not, so they answer what the title cannot. Adding `stack` to PREFIX_TO_LABEL was rejected — a stack PR can carry fixes, features, or docs, so any fixed mapping would be a lie. The unanimity rule this started with was falsified by running it on the real data. #952 gives `{bug: 1}` and labels, but #955 gives `{bug: 4, chore: 1}` and would abstain — four `fix(codex):` commits plus one `test(codex):`, which is a bug fix by any honest reading. A rule that abstains there abstains on most real PRs, since nearly every substantial change carries a test or chore commit. So `chore` is supporting, not competing: `test:`, `ci:`, `chore:`, `style:`, `refactor:`, and `build:` all map to it, and none of them says what a PR is FOR. It drops out of the tally when a non-chore type is present. An all-chore PR still gets `chore`, and a genuine `fix:`-plus-`feat:` mix is still left unlabeled rather than guessed. The title stays authoritative when it classifies, so a well-formed title is never overridden by what happens to be committed under it, and the existing human-override gate still runs first. No permission change: `pulls.listCommits` is covered by the existing `contents: read`. Both rules were driven red: removing the fallback fails the stack-PR test and nothing else; removing the chore-demotion fails the #955-shape test and nothing else. The docs now also state the promotion model, which was only a code comment before: `enforce-target` and `label` run on `pull_request_target` and are loaded from the default branch, so merging either to `dev` does not change live behavior until promotion to `main`. Verified: node --test .github/scripts/pr-labeler.test.cjs 24 pass; bun test tests/ci-workflows.test.ts 83 pass; typecheck and privacy:scan pass.
📝 WalkthroughWalkthroughCI pull-request triggers no longer restrict base branches to ChangesStacked PR CI and labeling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260804_stacked_pr_ci/000_scope.md`:
- Around line 80-83: Add the text language identifier to each fenced
literal-output block: update the opening fences at
devlog/_plan/260804_stacked_pr_ci/000_scope.md lines 80-83,
devlog/_plan/260804_stacked_pr_ci/010_ci_trigger.md lines 31-33, and
devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md lines 15-18, 24-27,
and 56-59. No other content changes are needed.
In `@devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md`:
- Around line 61-64: Update the reference at the start of the paragraph in the
PR classification discussion to use “PR `#955`” instead of “#955”, preserving the
rest of the text unchanged so the Markdown heading rule is satisfied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a53ee9c9-3c4b-4324-b8a7-97ed55605225
📒 Files selected for processing (9)
.github/scripts/pr-labeler.cjs.github/scripts/pr-labeler.test.cjs.github/workflows/ci.yml.github/workflows/pr-labeler.ymldevlog/_plan/260804_stacked_pr_ci/000_scope.mddevlog/_plan/260804_stacked_pr_ci/010_ci_trigger.mddevlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.mddocs-site/src/content/docs/contributing/pr-quality.mdtests/ci-workflows.test.ts
Comment on lines
+80
to
+83
| ``` | ||
| planTypeLabelSync({title: "stack 1/5: triage the open issue surface..."}) | ||
| -> { skip: true, reason: "no-prefix" } | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add languages to all fenced literal-output blocks.
markdownlint-cli2 reports MD040 at each site. Add text after each opening fence.
devlog/_plan/260804_stacked_pr_ci/000_scope.md#L80-L83: change the opening fence to ````text``.devlog/_plan/260804_stacked_pr_ci/010_ci_trigger.md#L31-L33: change the opening fence to ````text``.devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md#L15-L18: change the opening fence to ````text``.devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md#L24-L27: change the opening fence to ````text``.devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md#L56-L59: change the opening fence to ````text``.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 3 files
devlog/_plan/260804_stacked_pr_ci/000_scope.md#L80-L83(this comment)devlog/_plan/260804_stacked_pr_ci/010_ci_trigger.md#L31-L33devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md#L15-L18devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md#L24-L27devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md#L56-L59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260804_stacked_pr_ci/000_scope.md` around lines 80 - 83, Add the
text language identifier to each fenced literal-output block: update the opening
fences at devlog/_plan/260804_stacked_pr_ci/000_scope.md lines 80-83,
devlog/_plan/260804_stacked_pr_ci/010_ci_trigger.md lines 31-33, and
devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md lines 15-18, 24-27,
and 56-59. No other content changes are needed.
Source: Linters/SAST tools
Comment on lines
+61
to
+64
| #955 is four `fix(codex):` commits plus one `test(codex):`. It is a bug-fix PR | ||
| by any honest reading, and a rule that abstains there is a rule that abstains on | ||
| most real PRs — almost every substantial change carries a test or chore commit | ||
| alongside its feature or fix. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Prevent #955 from becoming malformed heading syntax.
Line 61 starts with #955. markdownlint-cli2 reports MD018 for this form. Prefix the reference with PR .
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 61-61: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md` around lines 61 -
64, Update the reference at the start of the paragraph in the PR classification
discussion to use “PR `#955`” instead of “#955”, preserving the rest of the text
unchanged so the Markdown heading rule is satisfied.
Source: Linters/SAST tools
![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: 243197ae6a
ℹ️ 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".
Comment on lines
+5
to
+8
| # No base-branch filter on purpose. GitHub matches `branches:` against the | ||
| # BASE ref, so `[main, dev]` silently excluded stacked child PRs — whose | ||
| # base is another open PR's head branch, an intentional review workflow per | ||
| # AGENTS.md that `enforce-target` already exempts from the wrong-base gate. |
There was a problem hiding this comment.
Removing the base-branch filter makes this workflow run for qualifying PRs against any base, but structure/06_docs-and-release.md:43 still says Cross-platform CI runs only for pull requests to main or dev. This leaves the repository's maintainer workflow map contradicting the actual trigger and the newly updated public guide; update that entry to describe the unfiltered pull_request trigger and its path filtering.
Useful? React with 👍 / 👎.
| for (const [path, expectedKeys] of [ | ||
| // No `branches`: the stacked-base exemption has no enumerable branch list. | ||
| [".github/workflows/ci.yml", ["paths"]], | ||
| [".github/workflows/service-lifecycle.yml", ["branches", "paths"]], |
There was a problem hiding this comment.
For a stacked child submitted from a fork, retaining branches: [main, dev] on service-lifecycle.yml means changes to src/service.ts, src/cli/index.ts, or the other service paths receive none of the Linux, macOS, and Windows lifecycle smoke tests that the same change would receive when targeting dev; unlike an in-repository stack, the fork's push cannot produce an upstream push-triggered run. Since enforce-target accepts this stacked shape, remove the service workflow's base filter or otherwise ensure its three-platform PR coverage reaches every accepted stacked base.
AGENTS.md reference: .github/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
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.
Summary
Two defects that shared one symptom report on the #951-#955 stack, and only one of them was about stacked PRs.
ci.ymlnever ran for stacked children. Thepull_requesttrigger filtered onbranches: [main, dev], which GitHub matches against the base ref. A stacked child's base is another open PR's head branch, so the workflow was never queued: the stack merged withenforce-target,label, andreact-doctoras its only check-runs, and no test job at all, for 24 changed files undersrc/.stack 1/5:fails the conventional-commit regex (the1/5sits between the word and the colon), reaches the sentence-case fallback, which extractsstack— a word with noPREFIX_TO_LABELentry. The sync returns{skip: true}, a skip is not a failure, so thelabelcheck stayed green while all four PRs carried no type label.This contradicted the repository's own design:
AGENTS.mdcalls stacked children an intentional review workflow, andenforce-targetalready implements the exemption. The gate accepted them; the test gate never saw them.What changed
CI trigger. The
branches:filter is removed rather than extended. An allowlist cannot express "base is another PR's head": open PR head refs today arecodex/(14) but alsofix/(4),feat/(3),agent/(3),split/,ingw/, and any of them can become a stacked base. Acodex/**glob — the first draft — would have fixed the maintainer's own stacks and left contributor stacks silently unverified, which is the worse half of the bug.paths:is untouched and remains the real scope gate, so docs-only anddevlog/-only PRs still queue nothing. The precedent is already in-repo:issue-quality-tests.ymlrunspull_requestwithpaths:and nobranches:. Widening is safe here specifically because this workflow ispull_request(notpull_request_target), declarescontents: read, and reads no secrets — the same change would not be safe onenforce-pr-target.yml.push:stays pinned to the integration lines.Labeler. A title that carries no type falls back to the PR's commits, which stay conventional even when the title does not. Adding
stacktoPREFIX_TO_LABELwas rejected: a stack PR can carry fixes, features, or docs, so any fixed mapping would be a lie.The unanimity rule this started with was falsified by running it on real data — #952 gives
{bug: 1}and labels, but #955 gives{bug: 4, chore: 1}and would abstain, despite being fourfix(codex):commits plus onetest(codex):. Sochoreis treated as supporting, not competing:test:/ci:/chore:/style:/refactor:/build:all map to it and none says what a PR is for, so it drops out when a non-chore type is present. An all-chore PR still getschore; a genuinefix:-plus-feat:mix stays unlabeled rather than guessed.The title stays authoritative when it classifies, and the existing human-override gate still runs first. No permission change —
pulls.listCommitsis covered by the existingcontents: read.The test that was asserting the bug
"PR checks reach every branch the target gate accepts"pinned both workflows to["dev", "main"]and asserted the trigger key set as["branches", "paths"]— which is how this survived. Its intent is exactly this fix (commit5229717b1: "an accepted PR is also a checked one"), so it is extended to the gate's stacked exemption rather than deleted.service-lifecycle.ymlkeeps its branch list; it gates the release service path, not review.Test plan
bun run test— 8004 pass / 0 fail / 8 skip across 525 files.bun test tests/ci-workflows.test.ts— 83 pass.node --test .github/scripts/pr-labeler.test.cjs— 24 pass (8 new).bun run typecheck,bun run privacy:scan— pass.ci.ymlat verification:pull_requestkeys["paths"],branchesundefined,push.branchesunchanged[main, preview, dev], both path lists still 17 entries,permissions: {contents: read}.github-scriptactually runs it.Driven red, all three:
branches: [main, dev]fails exactly"PR checks reach every branch the target gate accepts"and nothing else;Note on live effect
enforce-targetandlabelrun onpull_request_target, which GitHub loads from the default branch. The labeler half of this does not change live behavior until promotion tomain; the CI trigger half takes effect as soon as it is on the targeted base. The docs now say so — it was previously only a code comment.No
src/changes. Planning notes indevlog/_plan/260804_stacked_pr_ci/.Summary by CodeRabbit
New Features
Documentation
Tests