diff --git a/.github/scripts/pr-labeler.cjs b/.github/scripts/pr-labeler.cjs index c57dce0d2..11880be21 100644 --- a/.github/scripts/pr-labeler.cjs +++ b/.github/scripts/pr-labeler.cjs @@ -55,6 +55,35 @@ function detectTypeLabelFromTitle(title) { return null; } +/** + * Type from the PR's own commits, for titles the title matcher cannot classify. + * + * A PR titled `stack 3/5: carry six contributor bug fixes` fails the + * conventional regex (the `3/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 the sync skips — and a skip is not a failure, so the + * `label` check stays green while the PR carries no type label at all. The + * commits underneath are conventional (`fix(codex): ...`), so they can answer + * the question the title cannot. + * + * `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. Requiring unanimity would abstain on almost every real PR: #955 is four + * `fix(codex):` commits plus one `test(codex):`, and it is a bug fix. + * + * Anything still ambiguous after that (`fix:` alongside `feat:`) stays + * unlabeled rather than guessed. + */ +function detectTypeLabelFromCommits(messages) { + const types = new Set(); + for (const message of Array.isArray(messages) ? messages : []) { + const detected = detectTypeLabelFromTitle(String(message || "").split("\n")[0]); + if (detected) types.add(detected); + } + if (types.size > 1) types.delete("chore"); + return types.size === 1 ? [...types][0] : null; +} + /** * True when a human (any non-bot actor) has ever labeled or unlabeled a managed * type label on this PR. Mirrors issue-quality's sticky maintainerOverride: @@ -105,7 +134,11 @@ function planTypeLabelSync(input) { return { skip: true, reason: "human-override" }; } - const detected = detectTypeLabelFromTitle(title); + // The title is authoritative when it classifies. The commits only answer for + // titles it cannot (`stack 3/5: ...`), so a well-formed title is never + // overridden by what happens to be committed under it. + const detected = + detectTypeLabelFromTitle(title) ?? detectTypeLabelFromCommits(input?.commitMessages); if (!detected) { return { skip: true, reason: "no-prefix" }; } @@ -121,6 +154,7 @@ module.exports = { TYPE_LABELS, BOT_ACTORS, detectTypeLabelFromTitle, + detectTypeLabelFromCommits, hasHumanTypeLabelOverride, planTypeLabelSync, }; diff --git a/.github/scripts/pr-labeler.test.cjs b/.github/scripts/pr-labeler.test.cjs index 1c6193754..103a396a2 100644 --- a/.github/scripts/pr-labeler.test.cjs +++ b/.github/scripts/pr-labeler.test.cjs @@ -6,6 +6,7 @@ const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); const { detectTypeLabelFromTitle, + detectTypeLabelFromCommits, hasHumanTypeLabelOverride, planTypeLabelSync, TYPE_LABELS, @@ -145,6 +146,115 @@ describe("planTypeLabelSync", () => { }); }); +describe("detectTypeLabelFromCommits", () => { + it("reads the type from unanimous commits", () => { + assert.equal( + detectTypeLabelFromCommits([ + "fix(usage): price long-context requests at the published long rate", + ]), + "bug", + ); + }); + + it("treats chore as supporting, not competing (PR #955 shape)", () => { + // Four `fix(codex):` commits plus one `test(codex):`. Requiring unanimity + // would abstain here, and on almost every real PR — nearly every + // substantial change carries a test or chore commit alongside its fix. + assert.equal( + detectTypeLabelFromCommits([ + "fix(codex): probe reset-derived cooldowns without waiting to be selected", + "fix(codex): fail closed on an unrecognized plan", + "fix(codex): classify prolite as a weekly plan", + "fix(codex): share one window rule instead of a plan allowlist", + "test(codex): assert the window rule in literals", + ]), + "bug", + ); + }); + + it("keeps chore when nothing else competes", () => { + assert.equal( + detectTypeLabelFromCommits(["ci: pin an action", "test: add a case"]), + "chore", + ); + }); + + it("abstains on a genuine mix of fix and feat", () => { + assert.equal( + detectTypeLabelFromCommits(["fix(a): repair x", "feat(b): add y"]), + null, + ); + }); + + it("reads only the first line of a multi-line commit message", () => { + // A body line starting with `feat:` must not vote. + assert.equal( + detectTypeLabelFromCommits([ + "fix(a): repair x\n\nfeat: this is prose in the body, not a type", + ]), + "bug", + ); + }); + + it("returns null for absent or unusable input", () => { + assert.equal(detectTypeLabelFromCommits([]), null); + assert.equal(detectTypeLabelFromCommits(undefined), null); + assert.equal(detectTypeLabelFromCommits(["", null]), null); + }); +}); + +describe("planTypeLabelSync commit fallback", () => { + it("labels a stack PR whose title carries no type", () => { + // `stack 3/5:` fails the conventional regex (the `3/5` sits between the + // word and the colon), reaches the sentence-case fallback, which extracts + // `stack` — a word with no PREFIX_TO_LABEL entry. The sync used to skip + // here, and a skip is not a failure, so the `label` check stayed green + // while all four stack PRs carried no type label. + const plan = planTypeLabelSync({ + title: "stack 3/5: carry six contributor bug fixes with authorship intact", + currentLabels: [], + events: [], + commitMessages: [ + "fix(kiro): round-trip the redactedContent reasoning blob", + "fix(responses): close passthrough streams at terminal events", + ], + }); + assert.deepEqual(plan, { skip: false, detected: "bug", add: "bug", remove: [] }); + }); + + it("does not let commits override a title that already classifies", () => { + const plan = planTypeLabelSync({ + title: "feat(providers): add a preset", + currentLabels: [], + events: [], + commitMessages: ["fix(a): repair x", "fix(b): repair y"], + }); + assert.equal(plan.detected, "enhancement"); + }); + + it("still skips when neither the title nor the commits classify", () => { + const plan = planTypeLabelSync({ + title: "stack 1/5: triage the open issue surface", + currentLabels: [], + events: [], + commitMessages: ["wip", "more wip"], + }); + assert.deepEqual(plan, { skip: true, reason: "no-prefix" }); + }); + + it("still honours a human override before consulting commits", () => { + const plan = planTypeLabelSync({ + title: "stack 2/5: price long-context requests", + currentLabels: ["enhancement"], + events: [ + { event: "labeled", label: { name: "enhancement" }, actor: { login: "a-human" } }, + ], + commitMessages: ["fix(usage): price long-context requests"], + }); + assert.deepEqual(plan, { skip: true, reason: "human-override" }); + }); +}); + describe("pr-labeler workflow", () => { const workflowPath = path.join(__dirname, "../workflows/pr-labeler.yml"); const workflow = fs.readFileSync(workflowPath, "utf8"); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 420c22944..79b880267 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,7 +2,23 @@ name: Cross-platform CI on: pull_request: - branches: [main, dev] + # 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. + # The #951-#955 stack merged with `enforce-target`, `label`, and + # `react-doctor` as its only check-runs: no test job ever queued for 24 + # changed files under `src/`. + # + # An allowlist cannot express "base is another PR's head" — stacked bases + # carry contributor prefixes (`fix/`, `feat/`, `agent/`) as readily as + # `codex/`, and contributor stacks need CI most. `paths:` below is the real + # scope gate, same shape as issue-quality-tests.yml. Safe to widen here + # because this workflow is `pull_request` (not `pull_request_target`), + # declares `contents: read`, and reads no secrets. + # + # `push:` stays pinned to the integration lines: it gates the release path, + # and this trigger already covers review. paths: - "src/**" - "bin/**" diff --git a/.github/workflows/pr-labeler.yml b/.github/workflows/pr-labeler.yml index 348620fca..309fd40b8 100644 --- a/.github/workflows/pr-labeler.yml +++ b/.github/workflows/pr-labeler.yml @@ -68,10 +68,18 @@ jobs: owner, repo, issue_number: pr, per_page: 100, }); + // Titles that carry no recognisable type (e.g. `stack 3/5: ...`) + // fall back to the PR's commits, which stay conventional even when + // the title does not. Covered by the existing `contents: read`. + const commits = await github.paginate(github.rest.pulls.listCommits, { + owner, repo, pull_number: pr, per_page: 100, + }); + const plan = planTypeLabelSync({ title: liveTitle, currentLabels: currentLabels.map((label) => label.name), events, + commitMessages: commits.map((commit) => commit.commit?.message || ''), }); if (plan.skip) { diff --git a/devlog/_plan/260804_stacked_pr_ci/000_scope.md b/devlog/_plan/260804_stacked_pr_ci/000_scope.md new file mode 100644 index 000000000..1230dbe2e --- /dev/null +++ b/devlog/_plan/260804_stacked_pr_ci/000_scope.md @@ -0,0 +1,119 @@ +# 000 — Stacked PR CI: scope and evidence + +Unit: `260804_stacked_pr_ci` +Class: C3 (two workflow-surface defects, cross-cutting, needs durable audit) + +## The defect, in one sentence + +A stacked child pull request — one whose base is another **open** PR's head +branch — runs no test CI at all, and gets no type label. + +## Evidence + +`AGENTS.md` calls stacked child PRs an intentional review workflow: + +> Stacked child pull requests that target another **open** PR's head branch are +> an intentional review workflow, not an alternate integration line. + +`enforce-pr-target.yml` implements that intent: it detects a stacked base by +listing open PRs and matching `other.head.ref === pr.base.ref`, then skips the +wrong-base gate. So the repository deliberately supports this shape. + +Observed on the #951–#955 stack (checked 2026-08-04, `gh api +repos/lidge-jun/opencodex/commits/
/check-runs`): + +| PR | base | check-runs present | +| --- | --- | --- | +| #952 | `codex/bug-stack-plan` | `enforce-target`, `label`, `react-doctor` | +| #953 | `codex/908-long-context-pricing` | `enforce-target`, `label`, `react-doctor` | +| #954 | `codex/carry-contributor-bugfixes` | `enforce-target`, `label`, `react-doctor` | +| #955 | `codex/545-classifier-thinking-disabled` | `enforce-target`, `label`, `react-doctor` | + +No `ci`, no `gates`, no `test 1/4`–`test 4/4`. The stack carried 24 changed +files under `src/` and 748 added lines with **zero** CI verification history. + +Labels on all four: empty. + +## Root cause 1 — the `ci.yml` branch filter + +`.github/workflows/ci.yml`: + +```yaml +on: + pull_request: + branches: [main, dev] + paths: [...] +``` + +GitHub evaluates `branches:` against the PR's **base** ref. A stacked child's +base is `codex/bug-stack-plan`, which is neither `main` nor `dev`, so the +workflow is never queued. The other PR workflows have no `branches:` filter +(`enforce-pr-target.yml`, `pr-labeler.yml`, `react-doctor.yml`), which is +exactly why those three checks appear and the test jobs do not. + +The filter is not wrong on its own — it exists to keep CI off unrelated base +branches. It is wrong that it has no exception for the one alternate base shape +the repository explicitly supports. + +### The fix, after audit + +The first draft narrowed the filter to `[main, dev, "codex/**"]`. The audit +killed it: open PR head refs are `codex/` (14) **and** `fix/` (4), `feat/` (3), +`agent/` (3), `split/`, `ingw/`. Any of those can become a stacked base, and a +contributor stack is the case that most needs CI. An allowlist cannot express +"base is another open PR's head". + +So the filter goes, and `paths:` — untouched — remains the scope gate. That is +already this repository's other pattern: `issue-quality-tests.yml` runs +`pull_request` with `paths:` and no `branches:`. Details in `010`. + +## Root cause 2 — the labeler's title contract + +`.github/scripts/pr-labeler.cjs` → `detectTypeLabelFromTitle()` recognises two +forms: + +1. conventional commit — `^([a-zA-Z]+)(\([^)]*\))?!?\s*:` +2. sentence-case fallback — `^([A-Za-z]+)\s+\S` + +Verified locally against the real stack titles: + +``` +planTypeLabelSync({title: "stack 1/5: triage the open issue surface..."}) + -> { skip: true, reason: "no-prefix" } +``` + +`stack 1/5:` fails the conventional regex (the `1/5` sits between the word and +the colon) and is then caught by the sentence-case fallback, which extracts +`stack`. `PREFIX_TO_LABEL` has no `stack` key, so the lookup returns `null` and +the sync skips. The `label` check still reports success — a skip is not a +failure — which is why this stayed invisible. + +This is **not** a stacked-PR bug. It is a title-vocabulary bug that the stack +happened to expose: any PR titled with an unrecognised prefix word is silently +unlabeled. The stack shape and the label gap are independent defects that share +one symptom report. + +## Non-goals + +- Merging #952–#955. The user owns that; this unit never touches those PRs. +- Changing `src/` runtime code. +- Promotion to `main`/`preview`, releases, tags. + +## Promotion caveat that must reach the docs + +`pr-labeler.yml` and `enforce-pr-target.yml` run on `pull_request_target`, which +GitHub always loads from the repository **default branch** (`main`). Landing a +labeler change on `dev` does not change live behavior until it is promoted. The +labeler file already carries this comment; the contributor docs do not say it. +`ci.yml` runs on `pull_request` and is read from the PR's merge ref, so the CI +trigger fix takes effect as soon as it is on the base branch being targeted. + +## Work-phase map (dependency-ordered) + +| Phase | Doc | Depends on | +| --- | --- | --- | +| 1 | `010_ci_trigger.md` | — | +| 2 | `020_labeler_and_docs.md` | 010 (shares the test file) | + +Phase 2 touches `tests/ci-workflows.test.ts` after phase 1 has added its block, +so it must run second to avoid re-resolving the same region twice. diff --git a/devlog/_plan/260804_stacked_pr_ci/010_ci_trigger.md b/devlog/_plan/260804_stacked_pr_ci/010_ci_trigger.md new file mode 100644 index 000000000..11c7d80e8 --- /dev/null +++ b/devlog/_plan/260804_stacked_pr_ci/010_ci_trigger.md @@ -0,0 +1,159 @@ +# 010 — Phase 1: make `ci.yml` reach stacked child PRs + +Depends on: nothing. Blocks: `020` (shares `tests/ci-workflows.test.ts`). + +## Objective + +A PR whose base is another open PR's head branch must queue the same test jobs +it would queue against `dev`, without making the workflow fire on every +conceivable base branch and without loosening the `paths:` filter. + +## Design decision + +GitHub has no "base is another PR's head" trigger condition. The available +lever is `branches` / `branches-ignore` on the `pull_request` event, matched +against the base ref. + +Options considered: + +| Option | Verdict | +| --- | --- | +| Add each stack base by name | Rejected — base names are per-stack and unknowable in advance. | +| `branches: [main, dev, "codex/**"]` | **Rejected on audit.** See below. | +| `branches-ignore` | Rejected — an exclusion list has to enumerate what to exclude, which is the same unknowable set. | +| Drop `branches:`, keep `paths:` | **Chosen.** | + +### Why `codex/**` was rejected + +The first draft of this doc chose `codex/**`, reasoning that stacked bases live +in the `codex/` namespace. The audit falsified that. Open PR head refs today: + +``` +14 codex/ 4 fix/ 3 feat/ 3 agent/ 1 split/ 1 ingw/ +``` + +Any of those 12 non-`codex/` branches can become a stacked base the moment +someone opens a child against it — contributor stacks are exactly the case that +most needs CI, since contributor code is the least trusted. `codex/**` would fix +the maintainer's own stacks and leave the contributor's silently unverified, +which is the worse half of the bug. + +### Why dropping `branches:` is safe here + +`branches:` and `paths:` are AND-ed, and `paths:` is the filter that actually +decides whether work runs: a PR that touches nothing under `src/`, `tests/`, +`gui/`, `bin/`, `scripts/`, or the pinned root files queues nothing regardless +of base. + +This is already the established pattern in this repository — +`.github/workflows/issue-quality-tests.yml` runs `pull_request` with `paths:` +and no `branches:` at all. So "scope by paths, not by base branch" is not a new +idea being introduced here; it is the convention this workflow diverges from. + +The cost delta is bounded and small: of the 26 open PRs with a non-`dev`/`main` +base, only those touching a `paths:` surface would newly queue, and each of them +is a PR that *should* have been running tests all along. + +The `push:` trigger is deliberately **left alone**. It stays pinned to +`[main, preview, dev]`: push CI gates the release lines, and widening it would +run the full matrix on every feature-branch push — a cost change nobody asked +for, and one the PR trigger already covers for the review path. + +## Change 1 — MODIFY `.github/workflows/ci.yml` + +Before (lines 4-6): + +```yaml +on: + pull_request: + branches: [main, dev] +``` + +After: + +```yaml +on: + pull_request: + # No `branches:` filter on purpose. GitHub matches it 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. + # The #951-#955 stack merged with `enforce-target`, `label`, and + # `react-doctor` as its only check-runs: no test job ever queued for 24 + # changed files under `src/`. + # + # An allowlist cannot express "base is another PR's head": stacked bases + # carry contributor prefixes (`fix/`, `feat/`, `agent/`, ...) as readily as + # `codex/`, and contributor stacks are the ones that most need CI. `paths:` + # below is the real scope gate — same shape as issue-quality-tests.yml, + # which has always run `pull_request` with `paths:` and no `branches:`. + # + # `push:` stays pinned to the integration lines: it gates the release path, + # and this trigger already covers review. +``` + +`paths:` is untouched, so a docs-only stacked PR still runs nothing. + +## Change 2 — MODIFY `tests/ci-workflows.test.ts` + +**Stale check (wp2 P, 2026-08-04).** The unit was drafted against a stale +checkout. On `origin/dev` the block sits at lines 243-278 (not ~89), and the +pinned `ciPaths` list has since grown `assets/**`, `README.md`, and `LICENSE`. +Neither affects this change: `paths:` stays untouched and its assertions are +left exactly as they are. + +The block parses `ci.yml` and pins `on.push.branches` plus both `paths` lists. +Its type annotation declares `pull_request?: { paths?: string[] }` — it does not +even model `branches` on the PR trigger, let alone assert it. That is precisely +how the gap survived. + +Extend the parsed shape: + +```ts + const ci = Bun.YAML.parse(await readText(".github/workflows/ci.yml")) as { + on?: { + push?: { branches?: string[]; paths?: string[] }; + pull_request?: { branches?: string[]; paths?: string[] }; + }; + }; +``` + +And add, after the `push.branches` assertion: + +```ts + // The PR trigger must carry NO base-branch filter. GitHub matches + // `branches:` against the BASE ref, so `[main, dev]` silently excluded + // stacked child PRs, whose base is another open PR's head branch — 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/`. + // + // Re-adding an allowlist is the regression this pins. It cannot be written + // correctly: stacked bases carry contributor prefixes (`fix/`, `feat/`, + // `agent/`) as readily as `codex/`, so any list leaves some stack silently + // unverified. `paths:` above is the scope gate. + expect(ci.on?.pull_request?.branches).toBeUndefined(); +``` + +Note `push.branches` keeps its exact-set assertion. The two triggers now differ +on purpose, and the test says so. + +## Verification + +1. `bun test tests/ci-workflows.test.ts` — green. +2. Restore `branches: [main, dev]` in `ci.yml`, re-run: the new assertion must + FAIL and nothing else should. Restore afterwards. (Not-vacuous proof.) +3. `bun run typecheck`, `bun run privacy:scan` — green. +4. Live: the PR carrying this change itself targets `dev`, so it proves the + unchanged path. Stacked-base proof comes from the parsed trigger plus the + GitHub docs semantics for `branches:` on `pull_request`. + +## Risk + +The change can only widen when the workflow runs, never narrow it, so no +currently-verified PR loses coverage. The cost is bounded by the untouched +`paths:` filter: a PR queues jobs only if it touches a real code surface. + +The one thing to watch is CI minutes on long-lived non-integration bases. That +is the intended trade — those PRs were merging unverified — and `paths:` keeps +documentation and devlog-only PRs free. diff --git a/devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md b/devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md new file mode 100644 index 000000000..8784cecc7 --- /dev/null +++ b/devlog/_plan/260804_stacked_pr_ci/020_labeler_and_docs.md @@ -0,0 +1,189 @@ +# 020 — Phase 2: label coverage for unrecognised title prefixes, and the docs + +Depends on: `010` (both edit `tests/ci-workflows.test.ts`). + +## What is actually broken + +Not the stacked shape. `pr-labeler.yml` has no `branches:` filter, so it runs on +stacked children exactly as it does anywhere else — the `label` check-run is +present on all four stack PRs and reports success. + +The gap is the title vocabulary. `detectTypeLabelFromTitle()` matches a prefix +word, then looks it up in `PREFIX_TO_LABEL`; an unknown word returns `null` and +`planTypeLabelSync` returns `{ skip: true, reason: "no-prefix" }`. Verified: + +``` +"stack 1/5: triage the open issue surface and lock the bug plan" + -> { skip: true, reason: "no-prefix" } +``` + +Note the shape of the failure, because it is not the obvious one. The +conventional-commit regex `^([a-zA-Z]+)(\([^)]*\))?!?\s*:` does **not** match +`stack 1/5:` — the `1/5` sits between the word and the colon. Verified: + +``` +"stack 1/5: triage...".match(conventional) -> null +"stack 1/5: triage...".match(sentence) -> ["stack 1", "stack"] +``` + +So it is the *sentence-case fallback* that captures `stack`, which is then +dropped at the `PREFIX_TO_LABEL` lookup. Either way the result is a skip, and a +skip is not an error — the check stays green and the PR silently carries no type +label. All four stack PRs have empty label sets. + +The underlying commits are correctly prefixed (`fix(usage):`, `fix(codex):`, +`test(codex):`). Only the PR titles use the stack vocabulary. + +## Design decision + +Rejected: adding `stack` to `PREFIX_TO_LABEL`. There is no correct type for it — +a stack PR can carry fixes, features, or docs — so any mapping would be a lie, +and it treats one team's naming habit as a repository-wide vocabulary. + +Rejected: falling back to the branch name. `codex/915-cooldown-recovery-probe` +has no type information either. + +**Chosen:** when the title yields no type, fall back to the PR's own commit +messages. The commits are conventional even when the title is not, and they are +the most faithful available statement of what the PR contains. + +### The unanimity rule, and why it had to change (wp3 audit) + +The first draft required unanimity: apply the type only if every typed commit +agrees, else skip. The audit ran it against the real stack and it failed the +very PRs it was written for: + +``` +#952 -> { bug: 1 } unanimous -> "bug" +#955 -> { bug: 4, chore: 1 } NOT unanimous -> skip +``` + +#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. + +So `chore` is treated as **supporting**, not competing. `test:`, `ci:`, +`chore:`, `style:`, `refactor:`, and `build:` all map to `chore`, and none of +them describes what a PR is *for*; they describe work that accompanies it. + +Final rule: + +1. Drop `chore` from the tally when any non-`chore` type is present. +2. If exactly one type remains, apply it. +3. Otherwise skip — a PR genuinely mixing `fix:` and `feat:` has no single + honest type, and inventing one is worse than leaving it unlabeled. + +An all-`chore` PR still gets `chore`, since step 1 only fires when something +else is present. + +## Change 1 — MODIFY `.github/scripts/pr-labeler.cjs` + +Add an exported helper next to `detectTypeLabelFromTitle`: + +```js +/** + * Type from the PR's commits, for titles the title matcher cannot classify. + * + * A PR titled `stack 3/5: carry six contributor bug fixes` reaches the + * sentence-case fallback, which extracts `stack`; that has no entry in + * PREFIX_TO_LABEL, so the sync skips and the PR carries no type label while the + * `label` check stays green. Its commits are conventional (`fix(codex): ...`), + * so they can answer the question the title cannot. + * + * `chore` is supporting, not competing. `test:`/`ci:`/`chore:`/`style:`/ + * `refactor:`/`build:` all map to it, and none of them says what a PR is FOR — + * requiring unanimity would abstain on almost every real PR. #955 is four + * `fix(codex):` commits plus one `test(codex):`; it is a bug fix. + * + * Anything still ambiguous after that (`fix:` plus `feat:`) is left unlabeled + * rather than guessed. + */ +function detectTypeLabelFromCommits(messages) { + const types = new Set(); + for (const message of Array.isArray(messages) ? messages : []) { + const detected = detectTypeLabelFromTitle(String(message || "").split("\n")[0]); + if (detected) types.add(detected); + } + if (types.size > 1) types.delete("chore"); + return types.size === 1 ? [...types][0] : null; +} +``` + +And extend `planTypeLabelSync` to accept `commitMessages` and consult the +fallback before giving up: + +```js + const detected = + detectTypeLabelFromTitle(title) ?? detectTypeLabelFromCommits(input?.commitMessages); + if (!detected) { + return { skip: true, reason: "no-prefix" }; + } +``` + +Export `detectTypeLabelFromCommits` alongside the existing names. + +## Change 2 — MODIFY `.github/workflows/pr-labeler.yml` + +Fetch the commits and pass their headlines. Inserted after the live-title +refetch, before `planTypeLabelSync`: + +```js + // Titles that carry no recognisable type (e.g. `stack 3/5: ...`) + // fall back to the PR's commits, which are conventional even when + // the title is not. + const commits = await github.paginate(github.rest.pulls.listCommits, { + owner, repo, pull_number: pr, per_page: 100, + }); + + const plan = planTypeLabelSync({ + title: liveTitle, + currentLabels: currentLabels.map((label) => label.name), + events, + commitMessages: commits.map((commit) => commit.commit?.message || ""), + }); +``` + +No permission change: `pulls.listCommits` is covered by the existing +`contents: read`. + +## Change 3 — MODIFY `.github/scripts/pr-labeler.test.cjs` + +Add a `detectTypeLabelFromCommits` describe block plus `planTypeLabelSync` +cases: + +- real stack titles + conventional commits → the commits' type is applied; +- the real #955 shape (four `fix:` + one `test:`) → `bug`, not a skip; +- an all-`chore` PR (`ci:` + `test:`) → `chore`; +- genuinely disagreeing commits (`fix:` + `feat:`) → still skipped; +- no commits and no title prefix → still skipped; +- a recognisable title is NOT overridden by its commits. + +## Change 4 — MODIFY `docs-site/src/content/docs/contributing/pr-quality.md` + +Document two things contributors currently cannot know: + +1. stacked child PRs run the same test CI as `dev`-targeted PRs (post-`010`); +2. the type label comes from the title, falling back to the commits. + +And state the promotion caveat: `pr-labeler.yml` and `enforce-pr-target.yml` run +on `pull_request_target`, which GitHub loads from the repository **default +branch**, so a change to either takes effect only after promotion to `main` — +not when it lands on `dev`. `ci.yml` runs on `pull_request` and takes effect as +soon as it is on the targeted base branch. + +## Verification + +1. `node --test .github/scripts/pr-labeler.test.cjs` — green. +2. Drive red: remove the `?? detectTypeLabelFromCommits(...)` fallback; the new + cases must fail. Restore. +3. `bun test tests/ci-workflows.test.ts`, `bun run typecheck`, + `bun run privacy:scan` — green. +4. Live label behavior cannot be proven from `dev`: the labeler is loaded from + the default branch. Report that honestly rather than claiming live effect. + +## Risk + +The fallback can only *add* a label where there was none, or leave the skip in +place. It never overrides a title that already classifies, and never overrides a +human's label (the existing `hasHumanTypeLabelOverride` gate runs first). diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md index ae6a27cd4..326431a1d 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -59,10 +59,29 @@ tells you exactly what to change: every pull request. Windows runs at the shipping boundary — on promotion to `main` or `preview` — so a slow or flaky Windows runner cannot decide when your pull request turns green. + This runs for **every** pull request, whatever its base branch — including a + stacked child whose base is another open PR's head. The `paths:` filter, not + the base branch, decides whether the jobs run at all: a PR touching only docs + or `devlog/` queues nothing. + +- **Type label.** The `label` check derives `bug` / `enhancement` / + `documentation` / `chore` from your PR title. A title without a recognisable + prefix (`stack 3/5: …`) falls back to the PR's commits, which usually stay + conventional; `chore`-family commits (`test:`, `ci:`, `refactor:`) do not + outvote a `fix:` or `feat:`. A PR that genuinely mixes types is left + unlabeled rather than guessed, and a label a human sets is never overwritten. CodeRabbit reviews every PR and its findings are advisory. Address what it gets right; say why when it is wrong. It does not block a merge. +### When a workflow change takes effect + +`enforce-target` and `label` run on `pull_request_target`, which GitHub always +loads from the repository **default branch**. A change to either takes effect +only after it is promoted to `main` — merging it to `dev` does not change live +behavior. The cross-platform CI workflow runs on `pull_request` and takes effect +as soon as it is on the branch being targeted. + ## Sponsored surfaces Authentication, credential handling, GitHub Actions workflows, release diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index e35788cb1..abe220531 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -205,23 +205,39 @@ describe("GitHub Actions hardening", () => { }); test("PR checks reach every branch the target gate accepts", async () => { - // These two lists have to move together with enforce-pr-target.yml. A PR - // that passes the gate but triggers no checks is worse than one that is - // blocked: it looks reviewable and has nothing behind it. Pin the - // pull_request branch lists to the gate's allow-list plus main. + // These lists have to move together with enforce-pr-target.yml. A PR that + // passes the gate but triggers no checks is worse than one that is blocked: + // it looks reviewable and has nothing behind it (commit 5229717b1). + // + // The gate accepts more than `ALLOWED_BASES`. It also exempts a STACKED + // child — a PR whose base is another open PR's head branch — from the + // wrong-base failure. That exemption has no fixed branch list, so a + // `branches:` allow-list on the check workflow can never cover it, and + // `ci.yml` therefore carries no base filter at all. `service-lifecycle.yml` + // keeps its list: it gates the release service path, not review. const gate = await readText(".github/workflows/enforce-pr-target.yml"); const allowed = gate.match(/const ALLOWED_BASES = \[([^\]]*)\];/); expect(allowed).not.toBeNull(); const bases = [...(allowed?.[1] ?? "").matchAll(/"([^"]+)"/g)].map(m => m[1]); expect(bases).toEqual(["dev"]); - for (const path of [".github/workflows/ci.yml", ".github/workflows/service-lifecycle.yml"]) { + // The gate itself must stay unfiltered by base, or the stacked exemption it + // implements would never be evaluated for the branches it exempts. + expect(gate).not.toMatch(/pull_request_target:[\s\S]{0,200}?branches:/); + + 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"]], + ] as const) { const workflow = Bun.YAML.parse(await readText(path)) as { on?: { pull_request?: Record