From 28efd2e6a1147afc7a3427bc73e1e29c06b16884 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Thu, 20 Aug 2026 15:54:30 -0700 Subject: [PATCH] chore(ci): consolidate the workflow directory Eight workflow files to six, dropping two OpenSpec checks that produced more false positives than signal and demoting the changeset gate that stacks made unsatisfiable. - ci.yml -> validate.yml, name: CI -> Validate. The job was already `Validate`, which is the required status check on main, so branch protection is unchanged. - openspec-rot.yml and pr-check-openspec.yml are deleted. An unarchived change directory is the normal state of a pull request, so a PR-time gate had to infer stack position, and a nightly sweep had to date directories from git history to guess at intent. Both are replaced by one step in validate.yml that runs on push to main only and fails while main carries an unarchived change. A forward-merging stack leaves main red until its final slice archives the change; that red blocks nothing, since branch protection reads each PR's own Validate. - require-changeset.yml -> changeset.yml, warning instead of failing. It now diffs origin/main..., so a changeset on the bottom branch counts for every PR above it and the stack question answers itself. The base-ref guard and the branches: filter are gone with it. `skip-changeset` survives only to silence the warning, which is what it had degenerated into anyway. claude-code-review-on-demand.yml and vale-binaries.yml are untouched. Docs follow the code: the CLAUDE.md stacking section, the iterate-pr skill's two structural-failure sections, and the infrastructure spec, which gains requirements for the main-only archive check and for the changeset check being advisory. Co-Authored-By: Claude Opus 5 (1M context) --- .agents/skills/iterate-pr/SKILL.md | 93 ++++++------------- .github/workflows/changeset.yml | 96 ++++++++++++++++++++ .github/workflows/ci.yml | 72 --------------- .github/workflows/openspec-rot.yml | 114 ----------------------- .github/workflows/pr-check-openspec.yml | 104 --------------------- .github/workflows/require-changeset.yml | 107 ---------------------- .github/workflows/validate.yml | 116 ++++++++++++++++++++++++ CLAUDE.md | 28 ++++-- openspec/specs/infrastructure/spec.md | 50 +++++++++- 9 files changed, 303 insertions(+), 477 deletions(-) create mode 100644 .github/workflows/changeset.yml delete mode 100644 .github/workflows/ci.yml delete mode 100644 .github/workflows/openspec-rot.yml delete mode 100644 .github/workflows/pr-check-openspec.yml delete mode 100644 .github/workflows/require-changeset.yml create mode 100644 .github/workflows/validate.yml diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index 19c0c08e..09d2265c 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -238,72 +238,33 @@ Run `${CLAUDE_SKILL_ROOT}/scripts/fetch_pr_checks.py` to get structured failure **Wait if pending:** If review bot checks (sentry, warden, cursor, bugbot, seer, codeql) are still running, wait before proceeding—they post actionable feedback that must be evaluated. Informational bots (codecov) are not worth waiting for. -#### Stacked PRs: ignore the OpenSpec Archive Check unless this is the last PR in the chain - -The `PR OpenSpec Archive Check` (workflow `pr-check-openspec.yml`) fails whenever -any unarchived directory exists under `openspec/changes/`. A change is archived -exactly once, at the END of the work — so a PR that still carries an in-progress -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. - -This workflow carries **no `branches:` filter** — that is why it runs on every -PR in a stack, and it is the reliable way to get that behavior. Expect to see -the check on every PR in a stack and decide from stack position, not from the -`on:` block. - -Do not generalize from workflows that DO filter on `branches: [main]`. GitHub -sometimes resolves a stacked PR's eventual target and matches on that, so such a -workflow may appear on mid-stack PRs — but it stops without warning (see -"two other failures that are structural" below). A filter-less trigger is the -only dependable way to run everywhere. - -The archive job is also skipped while a PR is a **draft**. A spec-only proposal -is its own tip until its implementation is stacked on top, so the gate would -otherwise demand it archive a change nobody has built yet, and it would sit red -for as long as the proposal is open. A draft cannot merge, and the check runs on -`ready_for_review`, so nothing unarchived can reach `main` — if a proposal PR is -red on this check, mark it ready only when its implementation is stacked -beneath it. - -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 -no other OPEN PR targets its head branch as a base: - -```bash -HEAD=$(gh pr view --json headRefName --jq '.headRefName') -gh pr list --state open --base "$HEAD" --json number -``` - -An empty list → nothing is stacked on top → this PR is the tip. - -Then: - -- **Not the tip** (some open PR is stacked on this one) → IGNORE the - `PR OpenSpec Archive Check` failure. Do NOT archive the change on this PR. - Treat the check as expected-red and do not let it block the iterate loop - (still address every other failing check and all feedback normally). -- **The tip** (nothing stacked on top — including an ordinary standalone PR) → - the change MUST be archived before merge. Archive it via the OpenSpec archive - flow, which moves `openspec/changes//` to the dated archive directory - `openspec/changes/archive/YYYY-MM-DD-/` (do not invent a different - location), then commit and push so the check goes green. - -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. +#### No PR check asks whether the OpenSpec change is archived + +A change is archived exactly once, at the END of the work, so an unarchived +directory under `openspec/changes/` is the normal state of a pull request. There +is no PR-time gate for it — the earlier one had to infer stack position to avoid +firing on in-flight work, and a check that is expected-red on most of a stack +teaches people to ignore red. + +The archive signal lives on `main` instead: a step in `validate.yml` runs on +push events only and fails while `main` carries an unarchived change directory. +If you see it red on `main`, the fix is to archive the change via the OpenSpec +archive flow — which moves `openspec/changes//` to +`openspec/changes/archive/YYYY-MM-DD-/` — or to land the stack that is +still holding it open. + +Practically, on a PR: archive when the PR is the last in the chain, and leave +the change directory alone otherwise. Nothing will fail either way. + +#### Stacked PRs: two other check behaviours worth knowing + +**`Changeset` warns, it does not fail.** It looks for a `.changeset/*.md` added +or modified anywhere between `main` and this PR's head, which covers every +branch below it in the stack. A warning means no changeset exists anywhere in +the stack. If the change ships user-visible behaviour, add one on 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. **`on: pull_request: branches: [main]` tells you nothing dependable about where a workflow runs.** GitHub sometimes resolves a stacked PR's eventual target and diff --git a/.github/workflows/changeset.yml b/.github/workflows/changeset.yml new file mode 100644 index 00000000..d9d97429 --- /dev/null +++ b/.github/workflows/changeset.yml @@ -0,0 +1,96 @@ +name: Changeset + +# Advisory, never blocking. A pull request that ships a release note should add +# a `.changeset/.md`; this job says so when one is missing and gets out of +# the way otherwise. +# +# It used to fail, and the failure did not survive contact with stacked pull +# requests. One change gets ONE changeset, which lives on the bottom branch, so +# the file is *added* in exactly one PR's own diff and inherited by every branch +# above it. A per-PR gate therefore had to reason about stack position to know +# whether an absent changeset was a real omission — via a base-ref guard that +# GitHub's stacked-PR handling made unreliable in both directions, and a +# `skip-changeset` label that got applied to silence the check rather than to +# record "this ships no release note." What was left was a red check people had +# learned to route around, which is worse than no check. +# +# So the question it asks is now the one that has a dependable answer: does a +# changeset exist ANYWHERE in this branch's stack — the full diff against +# `main`, which includes every ancestor branch's commits? If yes, the change is +# covered no matter which PR carries the file. If no, warn, and leave the call +# to the author. + +on: + # No `branches:` filter: the check is advisory, so it should reach every PR in + # a stack, and a filter is not a dependable way to scope one anyway. It runs + # everywhere and answers the stack question itself, below. `ready_for_review` + # is not in the default event set and must be named. + pull_request: + types: [opened, reopened, synchronize, ready_for_review] + +permissions: + contents: read + +jobs: + changeset: + name: Changeset + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 # need `origin/main` to diff the whole stack against + + - name: Look for a changeset in this stack + env: + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + HEAD_REF: ${{ github.head_ref }} + LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} + run: | + set -euo pipefail + + # The changesets "Version Packages" PR consumes changesets — removing + # them is its whole job — so it legitimately has none. + if [ "$HEAD_REF" = "changeset-release/main" ]; then + echo "Version Packages PR (changeset-release/main) — no changeset expected." + exit 0 + fi + + # Kept only to keep deliberate no-release-note PRs (docs, CI, chores, + # the vale-binaries bot) quiet. It suppresses a warning now, not a + # failure, so it can no longer be used to force a merge through. + case ",${LABELS}," in + *,skip-changeset,*) + echo "The 'skip-changeset' label is present — no release note expected." + exit 0 + ;; + esac + + # Three dots: merge-base of `main` and this head, so the range is + # everything the stack has added, including the ancestor branches + # below this PR. A changeset on the bottom branch counts for every PR + # above it, which is the arrangement this repo asks for. + # + # Additions AND modifications, because one change gets one changeset: + # a stack landing forward finds its release note already on `main` and + # the right move is to extend that file, not add a second entry for + # something that ships once. + FOUND=$(git diff --name-only --diff-filter=AM "origin/main...$HEAD_SHA" -- '.changeset/*.md' \ + | grep -viE '/README\.md$' || true) + + if [ -n "$FOUND" ]; then + echo "Changeset(s) present in this stack:" + echo "$FOUND" | sed 's/^/ - /' + exit 0 + fi + + echo "::warning::No changeset found anywhere in this stack. If this pull request changes published behaviour, run \`pnpm changeset\` on the bottom branch of the stack. If it ships no release note (docs / CI / chore), ignore this or apply the \`skip-changeset\` label." + { + echo "### No changeset in this stack" + echo "" + echo "Nothing under \`.changeset/\` was added or modified between \`main\` and this branch, including the branches below it." + echo "" + echo "- Ships user-visible behaviour → run \`pnpm changeset\` on the **bottom** branch of the stack, so every branch above inherits it." + echo "- Ships no release note (docs, CI, chore) → nothing to do. Apply \`skip-changeset\` to silence this." + echo "" + echo "_This is a warning. It does not block the merge._" + } >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml deleted file mode 100644 index f23939d5..00000000 --- a/.github/workflows/ci.yml +++ /dev/null @@ -1,72 +0,0 @@ -name: CI - -on: - push: - branches: [main] - # No `branches:` filter, deliberately. Lint, typecheck, and tests have no - # interest in where a PR eventually merges, and filtering on `main` silently - # skipped this workflow on stacked PRs. - # - # The filter matches the PR's base ref, but GitHub also resolves a stacked - # PR's *eventual* target and matches on that — so `branches: [main]` did run - # on PRs based on another branch, until it stopped. Measured on the - # #71→#93→#94→#95→#100→#102→#103→#106 stack: every PR up to #102 got a - # `Validate` run, while #103 and #106 got none, across 16 `pull_request` - # events that other workflows handled fine. A filter that works for six PRs - # and quietly fails on the seventh is worse than one that never worked, - # because nobody re-checks it. - # - # `ready_for_review` is NOT in the default set (opened/synchronize/reopened) - # and must be named: without it a draft marked ready gets no fresh run until - # something happens to push again, which is exactly the state #103 sat in. - pull_request: - types: [opened, synchronize, reopened, ready_for_review] - -permissions: - contents: read - -jobs: - validate: - name: Validate - runs-on: ubuntu-latest - - steps: - - name: Checkout - uses: actions/checkout@v6 - - - name: Install pnpm - uses: pnpm/action-setup@v6 - - - name: Setup Node - uses: actions/setup-node@v6 - with: - node-version: 24 - - - name: Install dependencies - run: pnpm install --frozen-lockfile - - - name: Lint - run: pnpm lint - - - name: Typecheck - run: pnpm typecheck - - - name: Build - run: pnpm build - - - name: Test - run: pnpm test - - - name: Test workflow scripts - run: node --test .github/scripts/*.test.cjs - - # Repo-wide, not changed-files-only: spec rot accumulates in the specs a - # PR does not touch, so a scoped check would never surface it. - - name: Validate specs - run: pnpm openspec validate --all --strict - - # `--strict` validates what the parser read, not that it read the whole - # file. A second `##` inside `## Requirements` ends the section and every - # requirement below it becomes invisible — valid, unread, and green. - - name: Check spec requirement visibility - run: node .github/scripts/openspec-visibility.cjs diff --git a/.github/workflows/openspec-rot.yml b/.github/workflows/openspec-rot.yml deleted file mode 100644 index 3d4a20c5..00000000 --- a/.github/workflows/openspec-rot.yml +++ /dev/null @@ -1,114 +0,0 @@ -name: OpenSpec Rot Check - -# A daily sweep of `main` for OpenSpec debris — the direction-agnostic backstop -# the merge-time archive gate structurally cannot provide. -# -# Incremental-forward stacks land the change directory on `main` UNARCHIVED and -# only archive it on the final slice (invariant I1: main ends clean, but may -# transiently carry an in-flight change while the stack drains). If that stack -# stalls or is abandoned part-way, the unarchived change sits on `main` -# indefinitely. This sweep catches two failure modes on `main`: -# -# DONE — a change whose tasks are ALL checked but which was never moved under -# archive/. Debris regardless of age → fails immediately. The tip -# archive gate catches this at merge time; this is defence in depth -# for an admin-merge or botched down-merge that slipped past it. -# STALE — any unarchived change that is NOT done (work unfinished, tasks.md -# empty/placeholder, or no tasks.md) with NO git activity in its -# directory for STALE_DAYS. A stalled/abandoned stack rotting on main. -# Fails after the window. Because every non-done change is treated as -# live, an abandoned change can't slip the net by having an empty -# tasks.md — the exact case this backstop exists for. -# -# Age is read from git history, never file mtime: a fresh `actions/checkout` -# stamps every file's mtime to checkout time, so mtime would always look fresh. -on: - schedule: - # Daily near the start of the Pacific business day. Cron is fixed-UTC and - # can't follow DST, so 15:17 UTC lands at 8:17am PDT / 7:17am PST — early - # either way. The :17 is deliberately off the top of the hour (GitHub delays - # runs scheduled on the heavily-contended :00). - - cron: "17 15 * * *" - workflow_dispatch: - -permissions: - contents: read - -env: - STALE_DAYS: "7" - -jobs: - rot: - name: "openspec: rot" - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v6 - with: - fetch-depth: 0 # full history — git log needs it to date each change dir - - - name: Fail on stale or complete-but-unarchived OpenSpec changes - run: | - set -euo pipefail - - if [ ! -d openspec/changes ]; then - echo "openspec/changes does not exist — nothing to check." - exit 0 - fi - - stale_days="${STALE_DAYS:-7}" - now=$(date +%s) - cutoff=$(( stale_days * 24 * 3600 )) - fail=0 - - while IFS= read -r dir; do - [ -z "$dir" ] && continue - name=$(basename "$dir") - tasks="$dir/tasks.md" - - if [ -f "$tasks" ]; then - incomplete=$(grep -cE '^[[:space:]]*- \[ \]' "$tasks" || true) - complete=$(grep -cE '^[[:space:]]*- \[[xX]\]' "$tasks" || true) - else - incomplete=0 - complete=0 - fi - incomplete=${incomplete:-0} - complete=${complete:-0} - total=$(( incomplete + complete )) - - # DONE: every task checked but not archived. Debris now, not later. - if [ "$total" -gt 0 ] && [ "$incomplete" -eq 0 ]; then - echo "::error::openspec/changes/$name: all $total task(s) complete but the change is not archived. Move it under openspec/changes/archive/ (e.g. /openspec-archive-change $name)." - fail=1 - continue - fi - - # Anything unarchived that is not DONE is LIVE by definition — its - # work is unfinished, its tasks.md is empty/placeholder (new changes - # start with an empty one), or it has no tasks.md at all. So there is - # no "not-live" branch that could let an abandoned change slip the - # net; staleness applies to every non-DONE change. - # - # Age is the last commit touching ANY file in the change directory - # (NOT mtime — a fresh checkout stamps mtime to checkout time). Dating - # the whole folder means real activity on proposal.md / specs counts - # as "not abandoned", not just edits to tasks.md. - last=$(git log -1 --format=%ct -- "$dir" || true) - if [ -z "$last" ]; then - echo "::warning::openspec/changes/$name: unarchived but has no git history to date; cannot assess staleness." - continue - fi - age=$(( now - last )) - if [ "$age" -gt "$cutoff" ]; then - days=$(( age / 86400 )) - echo "::error::openspec/changes/$name: live but untouched for ${days}d (> ${stale_days}d). A stalled/abandoned stack is leaving OpenSpec debris on main — land or close it, then archive the change." - fail=1 - fi - done < <(find openspec/changes -mindepth 1 -maxdepth 1 -type d ! -name archive) - - if [ "$fail" -ne 0 ]; then - echo "" - echo "OpenSpec rot detected — see the errors above." - exit 1 - fi - echo "No OpenSpec rot — OK." diff --git a/.github/workflows/pr-check-openspec.yml b/.github/workflows/pr-check-openspec.yml deleted file mode 100644 index e19a357b..00000000 --- a/.github/workflows/pr-check-openspec.yml +++ /dev/null @@ -1,104 +0,0 @@ -name: PR OpenSpec Archive Check - -# A change is archived exactly once, when ALL of its work has landed. In a -# stack of PRs, only the tip carries the archived change; the PRs below it still -# carry the in-flight change directory by design. So this check runs only on the -# tip of a stack (or a standalone PR) and is skipped on PRs that still have work -# stacked on top of them. Tip = no other OPEN PR targets this PR's head branch -# as its base. -# -# Drafts are skipped. A spec-only proposal is its own tip until its -# implementation is stacked on top, so the gate would otherwise demand it -# archive a change that has not been built yet — leaving the PR red for as long -# as the proposal is open, which is how teams learn to ignore red. A draft -# cannot merge, so the guarantee that no unarchived change reaches `main` is -# unaffected: the check runs when the PR is marked ready for review. -on: - pull_request: - # Both draft-transition events are REQUIRED, and neither is in the default - # set (opened/synchronize/reopened), because the archive job's condition - # depends on draft state: - # ready_for_review — without it a draft could be marked ready and merged - # on a stale green that was never re-evaluated. This - # one protects the guarantee. - # converted_to_draft — without it a PR that failed while ready keeps that - # failure after being converted back to draft, until - # some unrelated push happens to re-run it. - types: - [opened, synchronize, reopened, ready_for_review, converted_to_draft] - -permissions: - contents: read - pull-requests: read - -jobs: - stack-position: - name: "stack: position" - runs-on: ubuntu-latest - outputs: - is_tip: ${{ steps.detect.outputs.is_tip }} - steps: - - name: Determine whether this PR is the tip of its stack - id: detect - env: - GH_TOKEN: ${{ github.token }} - HEAD_REF: ${{ github.head_ref }} - REPO: ${{ github.repository }} - run: | - set -euo pipefail - - # Count OPEN PRs that target this PR's head branch as their base. - # Any such PR means work is still stacked on top → not the tip. - # Do NOT guess on failure: if stack position can't be read, fail loudly - # rather than silently defaulting to "tip" (which runs the gate on a - # mid-stack PR) or "not tip" (which skips archiving on a real tip). - if ! children=$(gh pr list --repo "$REPO" --state open --base "$HEAD_REF" \ - --json number --jq 'length'); then - echo "::error::Could not determine stack position (gh pr list failed). Re-run once the API is reachable." - exit 1 - fi - children=${children:-0} - - if [ "$children" -gt 0 ]; then - echo "is_tip=false" >> "$GITHUB_OUTPUT" - echo "This PR has $children open PR(s) stacked on top — changes still in flight." - echo "The OpenSpec archive check is skipped until this PR is the tip of the stack." - else - echo "is_tip=true" >> "$GITHUB_OUTPUT" - echo "No PRs are stacked on top — this PR is the tip (or standalone); the archive check will run." - fi - - check-openspec-archived: - name: "stack: openspec-archived" - needs: stack-position - # Skipped on drafts. A proposal-only PR is its own tip — nothing is stacked - # on it yet — so the gate would demand it archive a change whose - # implementation has not been written, and it would sit red for as long as - # the proposal is open. A draft cannot merge, so nothing can reach `main` - # unarchived; the check runs the moment it is marked ready for review. - if: needs.stack-position.outputs.is_tip == 'true' && github.event.pull_request.draft == false - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v6 - - - name: Fail if any unarchived OpenSpec change exists - run: | - set -euo pipefail - - if [ ! -d openspec/changes ]; then - echo "openspec/changes does not exist — nothing to check." - exit 0 - fi - - UNARCHIVED=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d ! -name archive) - - if [ -n "$UNARCHIVED" ]; then - echo "::error::Unarchived OpenSpec change directories found under openspec/changes/:" - echo "$UNARCHIVED" | sed 's|^| - |' - echo "" - echo "This PR is the tip of its stack (no open PRs stacked on top), so its OpenSpec change must be archived before merge." - echo "Move it under openspec/changes/archive/ (e.g. /openspec-archive-change ) and commit." - exit 1 - fi - - echo "No unarchived OpenSpec changes — OK." diff --git a/.github/workflows/require-changeset.yml b/.github/workflows/require-changeset.yml deleted file mode 100644 index 5f08e2f2..00000000 --- a/.github/workflows/require-changeset.yml +++ /dev/null @@ -1,107 +0,0 @@ -name: Require Changeset - -# Every PR to `main` must add a changeset (a new `.changeset/.md`) so the -# version bump is intentional and the changelog stays complete. A PR that -# legitimately needs no release note (docs, CI, chores) can carry the -# `skip-changeset` label to bypass the requirement. -# -# The job ALWAYS runs and reports a status (the label is checked inside the -# step, not via a job-level `if`), so it stays safe to mark as a required check — -# a conditionally-skipped required check would otherwise block merges. - -on: - pull_request: - branches: [main] - types: [opened, reopened, synchronize, labeled, unlabeled] - -permissions: - contents: read - -jobs: - changeset: - name: Require a changeset - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - - 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`). - # - # That resolution is undocumented and it also stops without warning: - # on the #71→#106 stack this workflow ran up to #102 and then produced - # no check run at all on #103 or #106. So `branches:` is unreliable in - # BOTH directions — it neither scopes a workflow to the bottom of a - # stack nor guarantees it reaches every PR. This guard stays because - # the first failure mode is the dangerous one here; a workflow that - # must reach every PR should instead carry no `branches:` filter, as - # `ci.yml` and `pr-check-openspec.yml` do. - # - # 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`. - if [ "$HEAD_REF" = "changeset-release/main" ]; then - echo "Version Packages PR (changeset-release/main) — bypassing the changeset requirement." - exit 0 - fi - - # Escape hatch for PRs that intentionally ship no release note. - case ",${LABELS}," in - *,skip-changeset,*) - echo "The 'skip-changeset' label is present — bypassing the changeset requirement." - exit 0 - ;; - esac - - # A changeset is any `.changeset/*.md` this PR adds OR modifies, other - # than the template README. - # - # Modifications count because one change gets ONE changeset: when a - # stack lands forward, the second PR onward finds the release note - # already on `main` from the PR below it, and the right move is to - # extend that file rather than add a second entry for something that - # ships once. Counting additions only made "grow the changeset as the - # stack lands" impossible to satisfy. - ADDED=$(git diff --name-only --diff-filter=AM "$BASE_SHA...$HEAD_SHA" -- '.changeset/*.md' \ - | grep -viE '/README\.md$' || true) - - if [ -z "$ADDED" ]; then - echo "::error::This PR adds no changeset. Run \`pnpm changeset\` to record the release impact, or apply the \`skip-changeset\` label if no release note is needed (docs / CI / chore)." - exit 1 - fi - - echo "Changeset(s) added by this PR:" - echo "$ADDED" | sed 's/^/ - /' diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml new file mode 100644 index 00000000..043ef61e --- /dev/null +++ b/.github/workflows/validate.yml @@ -0,0 +1,116 @@ +name: Validate + +on: + push: + branches: [main] + # No `branches:` filter, deliberately. Lint, typecheck, and tests have no + # interest in where a PR eventually merges, and filtering on `main` silently + # skipped this workflow on stacked PRs. + # + # The filter matches the PR's base ref, but GitHub also resolves a stacked + # PR's *eventual* target and matches on that — so `branches: [main]` did run + # on PRs based on another branch, until it stopped. Measured on the + # #71→#93→#94→#95→#100→#102→#103→#106 stack: every PR up to #102 got a + # `Validate` run, while #103 and #106 got none, across 16 `pull_request` + # events that other workflows handled fine. A filter that works for six PRs + # and quietly fails on the seventh is worse than one that never worked, + # because nobody re-checks it. + # + # `ready_for_review` is NOT in the default set (opened/synchronize/reopened) + # and must be named: without it a draft marked ready gets no fresh run until + # something happens to push again, which is exactly the state #103 sat in. + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + +permissions: + contents: read + +jobs: + validate: + name: Validate + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v6 + + - name: Install pnpm + uses: pnpm/action-setup@v6 + + - name: Setup Node + uses: actions/setup-node@v6 + with: + node-version: 24 + + - name: Install dependencies + run: pnpm install --frozen-lockfile + + - name: Lint + run: pnpm lint + + - name: Typecheck + run: pnpm typecheck + + - name: Build + run: pnpm build + + - name: Test + run: pnpm test + + - name: Test workflow scripts + run: node --test .github/scripts/*.test.cjs + + # Repo-wide, not changed-files-only: spec rot accumulates in the specs a + # PR does not touch, so a scoped check would never surface it. + - name: Validate specs + run: pnpm openspec validate --all --strict + + # `--strict` validates what the parser read, not that it read the whole + # file. A second `##` inside `## Requirements` ends the section and every + # requirement below it becomes invisible — valid, unread, and green. + - name: Check spec requirement visibility + run: node .github/scripts/openspec-visibility.cjs + + # Main only, deliberately. An unarchived change directory is the NORMAL + # state of a pull request — a change is archived exactly once, on the last + # slice of the work — so any PR-time gate has to guess at stack position to + # avoid firing on in-flight work, and it guesses wrong often enough that + # people learn to ignore it. `main` has no such ambiguity: whatever sits + # under openspec/changes/ once the work has landed is debris, and this is + # the one place that can be said without qualification. + # + # This replaces both a merge-time archive gate (false positives on every + # mid-stack PR) and a nightly staleness sweep (a second opinion about the + # same directory, dated from git history). Neither is needed: main going + # red is a standing signal that there is something to clean up, and it + # clears the moment the change is archived. + # + # Known and accepted: a stack that merges FORWARD lands its change + # directory on main and archives it only on the final slice, so main runs + # red for as long as that stack is draining. Nothing is blocked by it — + # branch protection reads the PR's own `Validate`, not main's — and the + # alternative is dating directories from git history to guess at intent, + # which is the check this one replaces. Merging the stack DOWN (see + # CLAUDE.md) keeps main clean throughout. + - name: Check for unarchived OpenSpec changes on main + if: github.event_name == 'push' && github.ref == 'refs/heads/main' + run: | + set -euo pipefail + + if [ ! -d openspec/changes ]; then + echo "openspec/changes does not exist — nothing to check." + exit 0 + fi + + UNARCHIVED=$(find openspec/changes -mindepth 1 -maxdepth 1 -type d ! -name archive) + + if [ -n "$UNARCHIVED" ]; then + echo "::error::main is carrying unarchived OpenSpec change directories:" + echo "$UNARCHIVED" | sed 's|^| - |' + echo "" + echo "The work has landed, so these are debris. Archive each one (e.g. /openspec-archive-change ), which moves it to openspec/changes/archive/YYYY-MM-DD-/, and push to main." + echo "If a change is still in flight on an open stack, land or close that stack — main should not hold an in-progress change once its branches are gone." + exit 1 + fi + + echo "No unarchived OpenSpec changes on main — OK." diff --git a/CLAUDE.md b/CLAUDE.md index 91c84a3f..7ef0ea6d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -101,35 +101,43 @@ The proposal states which of these the change is, and why. Decide it while writi The deciding question between forward and down is only this: **can each unit reach production on its own without breaking anything?** If landing unit 1 alone would leave `check` broken, tests failing, or a migration half-applied, the answer is no and the stack merges down. Do not assume forward because it is tidier — verify it, since "each unit is safe" is a claim about behavior, not intent. -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. +Note how this interacts with archiving: a change is archived exactly once, on whichever PR is the tip. No PR check asks about that — an unarchived change directory is the normal state of a pull request, so a PR-time gate can only guess at stack position, and it guessed wrong often enough to be ignored. The only check is on `main` (a step in `validate.yml`, push events only), which goes red while `main` carries an unarchived change directory. A stack that merges **down** keeps `main` clean throughout; a stack that merges **forward** leaves `main` red until its final slice archives the change. Nothing is blocked by that red — branch protection reads each PR's own `Validate` — but it is a standing reminder that the stack is unfinished. ### 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: +`changeset.yml` looks for a `.changeset/*.md` added or modified **anywhere between `main` and the PR's head** — the whole stack, since a child branch contains its ancestors' commits. It **warns and never fails**: a missing changeset is a judgement call about whether the change ships a release note, and the workflow is not in a position to make it. -- **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. +That is a deliberate retreat from a gate. A per-PR requirement had to reason about stack position to tell a real omission from a file that simply lives further down, and the `skip-changeset` label ended up being applied to silence a red check rather than to record "this ships no release note." The label survives, but it now suppresses a warning, so it can no longer be used to force a merge through. -The bypass is an in-step check on the base ref, and it is load-bearing. **`branches: [main]` does not reliably mean either "only the PR whose base is `main`" or "every PR in the stack."** The filter matches the PR's base ref, but GitHub also resolves a stacked PR's _eventual_ target and sometimes matches on that instead, so the workflow runs on mid-stack PRs — observed on #73, #80, and #81, all with `openspec/partition-engine-*` bases. +The placement rules are unchanged, because they are about review quality rather than about passing a check: + +- **The changeset belongs on the bottom PR**, the one that targets `main`. It is the first branch every other one inherits from, and the one that carries the release note to `main` if the stack lands forward. +- **Never add a second changeset per PR.** One change ships once and gets one release note; a later PR extends the existing file. + +### `branches:` filters do not tell you where a workflow runs + +**`branches: [main]` does not reliably mean either "only the PR whose base is `main`" or "every PR in the stack."** The filter matches the PR's base ref, but GitHub also resolves a stacked PR's _eventual_ target and sometimes matches on that instead, so a filtered workflow runs on mid-stack PRs — observed on #73, #80, and #81, all with `openspec/partition-engine-*` bases. **Do not depend on that resolution. It is undocumented and it stops without warning.** On the #71→#93→#94→#95→#100→#102→#103→#106 stack, every PR up to #102 got a `Validate` run and **#103 and #106 got none** — across 16 `pull_request` events that filter-less workflows handled fine. #103 was a ~93-file change that reached "ready for review" having never been linted, typechecked, or tested in CI. Depth correlates (#102 is six hops from `main`, #103 seven) but nothing confirms a cap, and it was not a date cutoff: #102 kept getting runs after #103 had already stopped. A filter that works for six PRs and quietly fails on the seventh is worse than one that never worked, because nobody re-checks it. Two rules follow, and they pull in opposite directions: -- **A workflow that must run everywhere carries no `branches:` filter at all.** Lint, typecheck, and tests have no interest in where a PR eventually merges. `ci.yml` dropped its filter for exactly this reason; `pr-check-openspec.yml` and `stack-breadcrumb.yml` never had one, which is why they kept running on #103. If you add such a workflow, also name `ready_for_review` in `types:` — it is not in the default set (`opened`/`synchronize`/`reopened`), so without it a draft marked ready gets no fresh run until someone happens to push again. -- **A 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 the changeset check, look at that guard rather than reaching for the label. +- **A workflow that must run everywhere carries no `branches:` filter at all.** Lint, typecheck, and tests have no interest in where a PR eventually merges. `validate.yml`, `changeset.yml`, and `stack-breadcrumb.yml` all carry no filter, which is why they kept running on #103. If you add such a workflow, also name `ready_for_review` in `types:` — it is not in the default set (`opened`/`synchronize`/`reopened`), so without it a draft marked ready gets no fresh run until someone happens to push again. +- **A 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. Better still, ask a question that does not depend on stack position at all: `changeset.yml` diffs against `main` rather than against its base, and the archive check moved off pull requests entirely. The shared point: the `on:` filter is not a reliable answer to "where does this PR land." Let the workflow run, and decide inside it. +### Growing the changeset + Put the changeset at the base and every branch above inherits it, since a child contains its ancestors' commits. -**Write it on the base branch before you cut the children.** Inheritance only runs forward in time: a child branched before the file existed does not carry it, and "grown as the stack grows" has nothing to grow. What makes this easy to miss is that the natural moment to write a release note is when you finish a unit — which is exactly the moment you are standing on a child branch, several branches above the only diff `require-changeset` reads. On the #71/#93/#94/#95 stack the changeset was written on the tip instead; the bottom PR looked fine only because it carried `skip-changeset`, and the moment that label came off, `Require a changeset` went red there. +**Write it on the base branch before you cut the children.** Inheritance only runs forward in time: a child branched before the file existed does not carry it, and "grown as the stack grows" has nothing to grow. What makes this easy to miss is that the natural moment to write a release note is when you finish a unit — which is exactly the moment you are standing on a child branch, several branches above the base. A changeset stranded on the tip still reaches `main` when a stack merges down, but on a forward-merging stack it means every PR below it lands with no release note. **Grow it incrementally when the stack merges forward.** 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. **When the stack merges down, that reasoning does not apply.** Nothing reaches `main` until everything does — a single protected merge carries the whole stack — so a changeset describing the complete change is accurate at the only moment it is ever read, and no reviewer is asked to approve more than what lands. Growing it per unit is still friendlier to review, but there it is a preference, not a correctness constraint. -What _is_ a correctness constraint in both shapes is where the file lives: **on the bottom branch, present in the bottom PR's own diff.** That is the only diff `require-changeset` ever evaluates. +In both shapes the file belongs **on the bottom branch**. Nothing enforces that any more, so it is on you: a forward-merging stack publishes from `main` as each slice lands, and only a changeset that is already there gets read. ### Landing a stack: merge _down_, then one merge to `main` @@ -172,7 +180,7 @@ This happens when the **parent** PR is merged with `--delete-branch`: deleting t - **Projects-classic deprecation** breaks some GraphQL-backed `gh` commands (e.g. `gh pr reopen`). Workaround: use the REST API for PR state changes (`gh api --method PATCH .../pulls/ -f state=open`). - **`gh pr update-branch` may not exist** in the installed `gh`; update locally (`git merge origin/main` on the up-to-date remote branch) and push. -- **Stack-aware OpenSpec archive check** (`pr-check-openspec.yml`) skips on non-tip PRs and runs on the tip; "tip" recomputes as branches merge, so it lands green when the archiving PR reaches `main`. +- **No PR-time OpenSpec archive check.** An unarchived change directory on a pull request is expected, not a failure. The only signal is `main`'s own `Validate` run, which goes red while `main` carries one. - **Clean up local branches** once the stack lands: `git fetch --prune`, then delete the branches that merged (`git branch --merged main`). ## OpenSpec Apply diff --git a/openspec/specs/infrastructure/spec.md b/openspec/specs/infrastructure/spec.md index 703178a9..4a30d7aa 100644 --- a/openspec/specs/infrastructure/spec.md +++ b/openspec/specs/infrastructure/spec.md @@ -140,14 +140,14 @@ The root `pnpm typecheck` command SHALL invoke `turbo run typecheck`, which runs **Continuous integration.** -### Requirement: CI workflow exists +### Requirement: Validation workflow exists -A GitHub Actions workflow file SHALL exist at `.github/workflows/ci.yml`. +A GitHub Actions workflow file SHALL exist at `.github/workflows/validate.yml`, and its job SHALL be named `Validate` to match the required status check configured on `main`. #### Scenario: Workflow file is present - **WHEN** inspecting the repository -- **THEN** `.github/workflows/ci.yml` SHALL exist and be valid YAML +- **THEN** `.github/workflows/validate.yml` SHALL exist and be valid YAML ### Requirement: Workflow triggers on every pull request and main pushes @@ -269,6 +269,27 @@ The second check verifies that every `### Requirement:` heading sits under `## R - **WHEN** a spec reaches end of file with a code fence still open, as happens when an opening fence is lost and its closer is left dangling - **THEN** the visibility step SHALL fail, because everything after that point is unreadable to the check as well as to the parser +### Requirement: Unarchived OpenSpec changes fail on main only + +The workflow SHALL fail when `main` carries a directory under `openspec/changes/` other than `archive/`, and SHALL make that check on `push` events to `main` only. No pull request check SHALL require a change to be archived. + +An unarchived change directory is the normal state of a pull request — a change is archived exactly once, on the last slice of the work — so a pull-request gate has to infer stack position to avoid firing on work still in flight, and a check that is expected-red across most of a stack trains people to ignore red. On `main` the question has an unambiguous answer: once the work has landed, whatever remains under `openspec/changes/` is debris. A stack that merges forward leaves `main` red until its final slice archives the change; that red blocks nothing, because branch protection reads each pull request's own `Validate` run. + +#### Scenario: Unarchived change on main fails the workflow + +- **WHEN** a commit is pushed to `main` and a directory other than `archive/` exists under `openspec/changes/` +- **THEN** the workflow SHALL fail, naming each unarchived directory + +#### Scenario: Unarchived change on a pull request does not fail + +- **WHEN** a pull request carries an unarchived change directory under `openspec/changes/` +- **THEN** the workflow SHALL NOT fail for it, because the check is skipped on pull request events + +#### Scenario: Archived changes pass + +- **WHEN** a commit is pushed to `main` and `openspec/changes/` holds only `archive/` +- **THEN** the check SHALL succeed + ### Requirement: Workflow uses pnpm matching packageManager field The workflow SHALL install pnpm using a version consistent with the `packageManager` field in the root `package.json`. @@ -315,4 +336,25 @@ Workspace packages whose versions are assigned by a release workflow SHALL be li #### Scenario: A platform-package change needs no changeset - **WHEN** a pull request modifies only workflow-versioned platform packages -- **THEN** the changeset requirement check does not fail for the absence of a changeset +- **THEN** the changeset check does not fail for the absence of a changeset + +### Requirement: The changeset check is advisory and stack-aware + +A GitHub Actions workflow SHALL report whether a `.changeset/*.md` other than the template README is added or modified anywhere between `main` and a pull request's head commit. It SHALL emit a warning when none is found and SHALL NOT fail the pull request in any case. + +The range is the whole stack, not the pull request's own diff: one change gets one changeset, it lives on the bottom branch, and every branch above inherits it because a child contains its ancestors' commits. Asking each pull request to add its own file made a correct arrangement look like an omission, which is what the `skip-changeset` label was being used to silence. Whether a change ships a release note is a judgement the workflow cannot make, so it reports and leaves the call to the author. + +#### Scenario: A changeset on a lower branch of the stack satisfies the check + +- **WHEN** a pull request adds no changeset but a branch below it in the stack does +- **THEN** the check SHALL report the changeset as present, because the diff is taken against `main` + +#### Scenario: No changeset anywhere in the stack warns without failing + +- **WHEN** no changeset is added or modified between `main` and the pull request's head +- **THEN** the check SHALL emit a warning and SHALL still conclude successfully + +#### Scenario: The Version Packages pull request is exempt + +- **WHEN** the pull request's head branch is `changeset-release/main` +- **THEN** the check SHALL pass without a warning, because consuming changesets is that pull request's purpose