diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index 6d8cadd4..7ed9b354 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -102,6 +102,20 @@ Cascade-rebases a branch's descendants onto their parents to carry a fix up the uv run ${CLAUDE_SKILL_ROOT}/scripts/propagate_stack.py --root [--dry-run] [--no-push] ``` +**One failure path leaves the repo on another branch.** Normal completion, a +rebase conflict, a balloon guard trip, and a push failure all `checkout` the +branch you started on before returning. The exception is when checking out a +_child_ fails (exit 6) — most often because that branch is checked out in a +worktree — which returns immediately, leaving you on whichever branch it had +reached. + +That matters because anything you do next inherits it as a base: branching "from +main" right after one of these silently creates a branch on top of the whole +stack, and the resulting PR carries every stack commit plus an OpenSpec change +directory it never touched. After a non-zero exit, run `git checkout main` (or +`git checkout -B origin/main`), and check `git log --oneline origin/main..HEAD` +before opening the PR. + ## Workflow ### 1. Identify PR @@ -232,11 +246,11 @@ exactly once, at the END of the work — so a PR that still carries an in-progre change directory will fail this check. Archiving on an intermediate PR is wrong: it would remove the change docs before the implementation PRs above it merge. -Note the workflow's trigger is currently `pull_request.branches: [main]`, so it -only RUNS on PRs whose base is `main`. A stacked PR based on another feature -branch won't run (or fail) this check at all — so there is nothing to ignore -there. The check matters for PRs that target `main`: typically the bottom of a -stack, plus any PR later retargeted to `main` as the stack merges down. +The workflow's trigger is `pull_request.branches: [main]`, but do NOT read that +as "it only runs on PRs whose base is `main`." Under GitHub's stacked-PR support +a stacked PR targets `main` eventually and the filter matches that eventual +target, so this workflow runs on mid-stack PRs as well. Expect to see the check +on every PR in a stack and decide from stack position, not from the `on:` block. When the archive check does run and fail, decide ONE thing before treating it as actionable: **is this PR the last in the chain (the tip)?** A PR is the tip when @@ -264,6 +278,55 @@ Then: This rule applies ONLY to the OpenSpec Archive Check. Every other check is handled normally regardless of stack position. +#### Stacked PRs: two other failures that are structural, not regressions + +**`Require a changeset` on the bottom PR.** The check looks for a +`.changeset/*.md` added in that PR's own diff, so the bottom PR — the one +targeting `main` — is the only place a changeset can satisfy it. If it fails +there, move the changeset DOWN to the bottom branch rather than labelling +anything `skip-changeset`; every branch above inherits it, since a child +contains its ancestors' commits. Extend that one file as later PRs land; never +add a second changeset per PR. + +Mid-stack PRs bypass the check on their base ref. If you see one failing it, +look at that guard rather than reaching for the label, which would wrongly +record the change as shipping no release note. + +**Do not read `on: pull_request: branches: [main]` as "this only runs on the +bottom PR."** Under GitHub's stacked-PR support a stacked PR targets `main` +eventually, and the filter matches that eventual target — so these workflows run +on mid-stack PRs too. A workflow that must act only on the PR merging to `main` +has to establish that from the base ref or its stack position. When judging +whether a check "should even be running here," check the stack rather than the +`on:` block. + +### Never leave a PR on red + +**No PR merges with failing tests, including mid-stack.** "Merging down" changes +where a PR merges, not whether its tests pass — and a red branch you merge into +its parent carries that red upward. + +It is tempting on a stack to explain a failure as structural: this unit +relocates something and the unit that consumes it lands next, so of course it +fails. **Treat that explanation as a bug report about the split, not a reason to +proceed.** If unit N alone leaves the suite red, unit N is incomplete — the +change that makes it self-consistent belongs _in_ it. In practice that means the +PR that moves something also repoints every reader and updates the tests that +assert the old shape; the next PR then generalizes. + +This is not hypothetical. A stack here sat on 20 failures rationalized as +"dispatch arrives in the next PR." The real causes were a migration that +relocated files while every reader kept the old paths, and a command that +discovered rules _before_ running the migration that creates them. Both were +real defects, in the released code path, that the structural story explained +away. An empty scan reports success, so the symptom was "no findings" rather +than an error — which is exactly why it looked like an artifact of the split. + +So: never annotate a red check as expected and move on. Check the branch out, +run the suite, and fix the unit until it is green on its own. The only failures +that survive that treatment are ones you can point at a specific missing commit +for — and if that commit is in this stack, it belongs in this PR. + ### 5. Fix CI Failures For each failure in the script output: diff --git a/.github/workflows/require-changeset.yml b/.github/workflows/require-changeset.yml index 89292e47..54555af6 100644 --- a/.github/workflows/require-changeset.yml +++ b/.github/workflows/require-changeset.yml @@ -29,12 +29,39 @@ jobs: - name: Check for a changeset env: BASE_SHA: ${{ github.event.pull_request.base.sha }} + BASE_REF: ${{ github.event.pull_request.base.ref }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} HEAD_REF: ${{ github.event.pull_request.head.ref }} LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} run: | set -euo pipefail + # Mid-stack PR. On a stack, exactly one changeset describes the whole + # change and it lives on the bottom PR — the one targeting `main`, + # which is where this check has to pass. Every branch above inherits + # that file, so it is never *added* in a child's own diff, and without + # this guard the check below fails every one of them. That must not be + # papered over with a `skip-changeset` label, which would wrongly + # record "this change ships no release note." + # + # This guard is required, not belt-and-braces. `branches: [main]` reads + # like "only PRs whose base is main," and that is how it filtered when + # a PR had exactly one base. Under GitHub's stacked-PR support a PR in + # a stack is understood to target `main` *eventually*, so the filter + # matches on the eventual target and the workflow runs on mid-stack + # PRs too. Observed here: #73, #80, and #81 — bases + # `openspec/partition-engine-*`, never `main` — each produced a failing + # `Require a changeset` check run (e.g. run 30786954198, event + # `pull_request`, head `openspec/partition-engine-2-dispatch`). + # + # So `branches:` no longer scopes a workflow to the bottom of a stack. + # Any job whose correctness depends on "is this the PR that merges to + # main" has to establish that itself, as this one does. + if [ "$BASE_REF" != "main" ]; then + echo "Base is '$BASE_REF', not 'main' — mid-stack PR, so the bottom PR of the stack carries the changeset." + exit 0 + fi + # The changesets "Version Packages" PR consumes changesets (removing # them is its whole job), so it legitimately has none. Bypass it by its # well-known bot branch name so it needs no manual `skip-changeset`. diff --git a/CLAUDE.md b/CLAUDE.md index 4eb18065..a327b4fb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -86,6 +86,21 @@ The deciding question between forward and down is only this: **can each unit rea Note how this interacts with the archive gate (see the OpenSpec archive check below): a change is archived exactly once, on whichever PR is the tip. Mid-stack PRs are expected to carry an unarchived change directory and the gate skips them. +### One changeset, at the bottom of the stack, grown as the stack grows + +`require-changeset.yml` looks for a `.changeset/*.md` **added in that PR's own diff**. On a stack that means: + +- **The changeset belongs on the bottom PR**, the one that targets `main`. That is the only place it can live: a changeset added on the tip is invisible to the bottom PR's diff, so the check would fail on the PR that actually merges. +- **Mid-stack PRs bypass the check**, because the base branch is not `main`. They inherit the base's changeset rather than adding one, so there is nothing for the check to find. **Do not label them `skip-changeset`** — the label records a deliberate "this change ships no release note," which is false here, and the bypass already handles it. + +The bypass is an in-step check on the base ref, and it is load-bearing. **`branches: [main]` no longer means "only the PR whose base is `main`."** Under GitHub's stacked-PR support, a PR in a stack is understood to target `main` eventually, so the filter matches on that eventual target and the workflow runs on mid-stack PRs as well — observed here on #73, #80, and #81, all with `openspec/partition-engine-*` bases. + +The general rule that follows: **any workflow whose correctness depends on "is this the PR that merges to `main`" must determine that itself** — from the base ref, or by resolving stack position — and cannot lean on the `on:` filter to scope it. If you see a mid-stack PR failing this check, look at that guard rather than reaching for the label. + +Put the changeset at the base and every branch above inherits it, since a child contains its ancestors' commits. + +**Grow it incrementally as the stack lands.** Each PR extends the changeset with its own scope rather than the base describing the whole future change up front. A reviewer reading the changeset then sees only what has actually landed, and is not asked to evaluate a release note that promises more than the diff in front of them. When you extend it, edit the same file on the branch you are working on — never add a second changeset per PR, or one change becomes several release notes for what merges to `main` exactly once. + ### Landing a stack: merge _down_, then one merge to `main` Merge each PR **down** into its parent's branch, from the tip to the bottom: