From 9da4154b77cc5759380d989fb4a6b1d4492d2be5 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 24 Aug 2026 22:08:42 -0700 Subject: [PATCH] style: widen the house-style rules to CLAUDE.md and .conventions First slice of the broadening in #169. The three prose rules were scoped to READMEs only, because landing them repo-wide meant roughly 2300 findings at level: error. This adds the two smallest surfaces and fixes what they catch, so the slice is green on its own. 41 findings: 39 em dashes rewritten as a period, comma, colon, or parentheses per the rule's own message rather than swapped mechanically for one substitute, and two uses of "simply" where the sentence was describing a real distinction ("merely lives further down", "just out of date") rather than hedging. Fenced code blocks are untouched. These rules carry Vale's default scope, which does not read them, so the em dashes in the shell comments at CLAUDE.md:155 and STYLEGUIDE-CODE.md:222 are out of scope and stay. Remaining surfaces, in the order #169 proposes: TypeScript comments (Vale's comments-only tier), then the agent-facing recipe text, which is the largest. openspec/changes/archive/ gets a permanent exclusion rather than a slice. Refs #169 --- .conventions/STYLEGUIDE-CODE.md | 26 ++++---- .../rules/vale/no-blocklist-phrases/.vale.ini | 12 ++++ .taskless/rules/vale/no-em-dashes/.vale.ini | 12 ++++ .taskless/rules/vale/no-hedging/.vale.ini | 5 ++ CLAUDE.md | 65 ++++++++++--------- 5 files changed, 75 insertions(+), 45 deletions(-) diff --git a/.conventions/STYLEGUIDE-CODE.md b/.conventions/STYLEGUIDE-CODE.md index bb149951..477219b5 100644 --- a/.conventions/STYLEGUIDE-CODE.md +++ b/.conventions/STYLEGUIDE-CODE.md @@ -124,7 +124,7 @@ interface GitHubComment { ### Export Types Referenced by Public API Signatures -**DO NOT** remove `export` from types that are transitively referenced by exported functions, values, or other exported types — even if tools like knip report them as "unused exports." With `declaration: true` in `tsconfig`, TypeScript requires all types in exported signatures to be exported themselves. +**DO NOT** remove `export` from types that are transitively referenced by exported functions, values, or other exported types, even if tools like knip report them as "unused exports." With `declaration: true` in `tsconfig`, TypeScript requires all types in exported signatures to be exported themselves. Before removing an `export` from a type, check whether any exported function or value references it in its signature (parameters, return types, or fields of other exported types). @@ -150,7 +150,7 @@ interface LayerResult { ... } // breaks declaration emit for VerifyResult - Knip tracks direct import usage, not transitive type reachability through exported signatures - Removing these exports causes `declaration: true` to fail with "exported function has or is using private name" errors -- The fix is tedious — each type must be re-exported individually, often across multiple review cycles +- The fix is tedious: each type must be re-exported individually, often across multiple review cycles ## Cross-Worker Durable Object Access @@ -201,7 +201,7 @@ import type { UserDO, GitHubOrganizationDO } from "@taskless/storage"; ### Verify Build Output In The Build, Not By Parsing It -**A failing build is still a valid test — of the build.** When an invariant is about a build artifact, enforce it where the artifact is produced. If a bundle must not contain something, the build should refuse to emit it, rather than emitting it and leaving a test to go looking afterwards. An invariant enforced at production time cannot be violated; one enforced afterwards can only be detected. +**A failing build is still a valid test of the build.** When an invariant is about a build artifact, enforce it where the artifact is produced. If a bundle must not contain something, the build should refuse to emit it, rather than emitting it and leaving a test to go looking afterwards. An invariant enforced at production time cannot be violated; one enforced afterwards can only be detected. **DO NOT** reconstruct a fact about generated output by parsing that output. @@ -240,7 +240,7 @@ for (const specifier of specifiers) { } ``` -**Tests that _use_ a built artifact are fine.** Importing the built entry and asserting on its behavior, or spawning the built CLI and asserting on its output, are ordinary tests. The rule is not "tests must not touch build output" — it is that tests must not re-derive what the build already knew. +**Tests that _use_ a built artifact are fine.** Importing the built entry and asserting on its behavior, or spawning the built CLI and asserting on its output, are ordinary tests. The rule is not "tests must not touch build output". It is that tests must not re-derive what the build already knew. ```typescript // ✅ Fine - uses the artifact, asserts on behavior @@ -252,19 +252,19 @@ const { stdout } = await execFileAsync("node", [builtCli, "help"]); expect(stdout).toContain("Usage:"); ``` -**Do not add a dependency in order to test an assertion.** If a test needs a parser to make sense of an artifact, that is the signal the check is in the wrong place — the generator already has the structured data. Reach for a new devDependency only when several tests need it and nothing in the existing toolchain can answer the question. +**Do not add a dependency in order to test an assertion.** If a test needs a parser to make sense of an artifact, that is the signal the check is in the wrong place: the generator already has the structured data. Reach for a new devDependency only when several tests need it and nothing in the existing toolchain can answer the question. -**Worked example.** `packages/cli/test/prompts.test.ts` asserted that the built `dist/prompts.js` chunk graph never reaches the CLI entry or a host capability, by regex-scanning the built JavaScript for `from "…"` to reconstruct the import graph. A built chunk embeds every help recipe as a string literal, and the `engine-selection` recipe contains the phrase `a different axis from "which engine"` — so the scan reported `dist/prompts.js graph imports which engine`. Prose was read as an import. +**Worked example.** `packages/cli/test/prompts.test.ts` asserted that the built `dist/prompts.js` chunk graph never reaches the CLI entry or a host capability, by regex-scanning the built JavaScript for `from "…"` to reconstruct the import graph. A built chunk embeds every help recipe as a string literal, and the `engine-selection` recipe contains the phrase `a different axis from "which engine"`, so the scan reported `dist/prompts.js graph imports which engine`. Prose was read as an import. The fixes that did not work, and why: -| Attempt | Why it was rejected | -| ---------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Filter candidates by specifier shape (`/^(?:node:)?[@\w./-]+$/`) | Passed only because that phrase contains a space. Measured against the real bundle the regex yields `["which engine"]` and the filter drops it — but `differs from "static-tier"` is a bare hyphenated name with no whitespace and would have been reported. The guard held by luck of punctuation. | -| Add `es-module-lexer` as a devDependency | Parsed the graph correctly, but bought a dependency — and a second major version, since vite already pulls 1.7.0 transitively — to serve a single test. | -| Anchor the regex to line-start | Matched the lexer exactly on today's bundles, but required `from` on the same line as `import`. A future bundler that wrapped a long import would silently stop detecting real imports — trading a loud false positive for a quiet false negative in the guard whose entire job is catching a leak. | +| Attempt | Why it was rejected | +| ---------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Filter candidates by specifier shape (`/^(?:node:)?[@\w./-]+$/`) | Passed only because that phrase contains a space. Measured against the real bundle the regex yields `["which engine"]` and the filter drops it, but `differs from "static-tier"` is a bare hyphenated name with no whitespace and would have been reported. The guard held by luck of punctuation. | +| Add `es-module-lexer` as a devDependency | Parsed the graph correctly, but bought a dependency, and a second major version since vite already pulls 1.7.0 transitively, to serve a single test. | +| Anchor the regex to line-start | Matched the lexer exactly on today's bundles, but required `from` on the same line as `import`. A future bundler that wrapped a long import would silently stop detecting real imports, trading a loud false positive for a quiet false negative in the guard whose entire job is catching a leak. | -The resolution: rollup's `OutputChunk` already exposes `imports` and `dynamicImports` — the exact resolved graph. The check moved into a vite plugin that fails the build, and the test was deleted. +The resolution: rollup's `OutputChunk` already exposes `imports` and `dynamicImports`, the exact resolved graph. The check moved into a vite plugin that fails the build, and the test was deleted. The same reasoning forbids adding a YAML parser to assert on generated config, or an HTML parser to assert on rendered output. In each case the generator knows the answer and the test is guessing at it. @@ -272,7 +272,7 @@ The same reasoning forbids adding a YAML parser to assert on generated config, o - An invariant enforced at production time cannot be violated; one enforced afterwards can only be detected - Parsing generated text reconstructs information the generator already had, using a weaker tool -- A check that needs a parser is a check in the wrong place — move it to where the structured data lives +- A check that needs a parser is a check in the wrong place; move it to where the structured data lives - A build that fails is a faster, earlier signal than a test that fails, and it cannot be skipped - Regexes over generated output are brittle in the worst direction: they break on content that merely resembles code, and they quietly stop matching when the generator's formatting changes diff --git a/.taskless/rules/vale/no-blocklist-phrases/.vale.ini b/.taskless/rules/vale/no-blocklist-phrases/.vale.ini index 4e38dbcd..260f2224 100644 --- a/.taskless/rules/vale/no-blocklist-phrases/.vale.ini +++ b/.taskless/rules/vale/no-blocklist-phrases/.vale.ini @@ -5,6 +5,18 @@ tskl) rule = no-blocklist-phrases BasedOnStyles = no-blocklist-phrases.no-blocklist-phrases = YES +# Agent-facing instructions and the house conventions. Read as often as the +# READMEs are, by both people and agents, and small enough to keep conforming. +[CLAUDE.md] +tskl) rule = no-blocklist-phrases +BasedOnStyles = +no-blocklist-phrases.no-blocklist-phrases = YES + +[.conventions/*.md] +tskl) rule = no-blocklist-phrases +BasedOnStyles = +no-blocklist-phrases.no-blocklist-phrases = YES + # Test fixtures are inputs to the CLI's own suite, not documentation. [**/test/fixtures/**/README.md] tskl) rule = no-blocklist-phrases diff --git a/.taskless/rules/vale/no-em-dashes/.vale.ini b/.taskless/rules/vale/no-em-dashes/.vale.ini index a837d6cb..12e5b2b7 100644 --- a/.taskless/rules/vale/no-em-dashes/.vale.ini +++ b/.taskless/rules/vale/no-em-dashes/.vale.ini @@ -5,6 +5,18 @@ tskl) rule = no-em-dashes BasedOnStyles = no-em-dashes.no-em-dashes = YES +# Agent-facing instructions and the house conventions. Read as often as the +# READMEs are, by both people and agents, and small enough to keep conforming. +[CLAUDE.md] +tskl) rule = no-em-dashes +BasedOnStyles = +no-em-dashes.no-em-dashes = YES + +[.conventions/*.md] +tskl) rule = no-em-dashes +BasedOnStyles = +no-em-dashes.no-em-dashes = YES + # Test fixtures are inputs to the CLI's own suite, not documentation. [**/test/fixtures/**/README.md] tskl) rule = no-em-dashes diff --git a/.taskless/rules/vale/no-hedging/.vale.ini b/.taskless/rules/vale/no-hedging/.vale.ini index af02a2a0..181cd731 100644 --- a/.taskless/rules/vale/no-hedging/.vale.ini +++ b/.taskless/rules/vale/no-hedging/.vale.ini @@ -5,6 +5,11 @@ tskl) rule = no-hedging BasedOnStyles = no-hedging.no-hedging = YES +[CLAUDE.md] +tskl) rule = no-hedging +BasedOnStyles = +no-hedging.no-hedging = YES + [.conventions/*.md] tskl) rule = no-hedging BasedOnStyles = diff --git a/CLAUDE.md b/CLAUDE.md index 4240e622..f4688844 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,7 +32,7 @@ When running OpenSpec commands in this repo, use `pnpm openspec` instead of a ba git config --get-all remote.origin.fetch # must be +refs/heads/*:refs/remotes/origin/* ``` - If either is wrong, repair it once — both are local settings, nothing is committed: + If either is wrong, repair it once. Both are local settings, nothing is committed: ```bash git fetch --unshallow @@ -40,7 +40,7 @@ When running OpenSpec commands in this repo, use `pnpm openspec` instead of a ba git fetch origin ``` - Until then: `--force-with-lease` fails with `stale info` on every branch (there is no remote-tracking ref to lease against, so people fall back to a bare `--force`), `git push -u` cannot store an upstream, `gh pr create` needs an explicit `--head `, and `git branch -r` shows only `main`. The dangerous one is quieter — `git rebase main` is only correct while the merge base sits inside the shallow window, so as `main` advances a rebase can reconstruct the wrong base without saying so. + Until then: `--force-with-lease` fails with `stale info` on every branch (there is no remote-tracking ref to lease against, so people fall back to a bare `--force`), `git push -u` cannot store an upstream, `gh pr create` needs an explicit `--head `, and `git branch -r` shows only `main`. The dangerous one is quieter. `git rebase main` is only correct while the merge base sits inside the shallow window, so as `main` advances a rebase can reconstruct the wrong base without saying so. ## PR Issue References @@ -56,7 +56,7 @@ Reference issues as a **trailing line at the bottom of the PR body**, not inline - A bare `-NNN` resolves without a URL for **any** Linear team, not just `TSKL-`. `TSKL-` is Product and `OSS-` is the open-source team; verified with `OSS-23`, which the integration linked and moved to In Review on PR creation. - `Fixes` for the issue this PR resolves; `Refs` for a parent or related issue that stays open. -- Mentioning an issue in prose (`Found while investigating TSKL-5678.`) is **not** a reference — a PR can cite an issue mid-body with no trailing directive at all. +- Mentioning an issue in prose (`Found while investigating TSKL-5678.`) is **not** a reference: a PR can cite an issue mid-body with no trailing directive at all. - Only use a reference you can verify from user input, the branch name, commits, PR discussion, or tracker output. Never invent an issue number. ### Editing an existing PR @@ -80,8 +80,8 @@ Both flags can be passed in one call. See also **Stacked PRs → Other gotchas** The two rules that cause the most damage when missed: -- **A worktree gets its own empty `node_modules`.** `git worktree add` is not finished until `pnpm install` has run inside it. Without that, `git commit` fails in `lint-staged` (no `prettier`/`eslint`), and every `pnpm` script fails. A missing `prettier` here once cost an agent an hour of dead-end workarounds. There is no `pnpm worktree` command — `git worktree` is the tool. -- **NEVER point an agent at the main repo path** (e.g. `/Users//code/taskless/skills`). It will `cd` there and run git commands and edits in the **main** checkout, defeating isolation — it can create and check out a branch in your working tree, silently switching your session off its own branch. Tell the agent to work in **its assigned worktree** (`$PWD`) and pass only relative paths plus GitHub identifiers (`owner/repo`). +- **A worktree gets its own empty `node_modules`.** `git worktree add` is not finished until `pnpm install` has run inside it. Without that, `git commit` fails in `lint-staged` (no `prettier`/`eslint`), and every `pnpm` script fails. A missing `prettier` here once cost an agent an hour of dead-end workarounds. There is no `pnpm worktree` command; `git worktree` is the tool. +- **NEVER point an agent at the main repo path** (e.g. `/Users//code/taskless/skills`). It will `cd` there and run git commands and edits in the **main** checkout, defeating isolation. It can create and check out a branch in your working tree, silently switching your session off its own branch. Tell the agent to work in **its assigned worktree** (`$PWD`) and pass only relative paths plus GitHub identifiers (`owner/repo`). ## Stacked PRs @@ -91,23 +91,23 @@ When PRs stack, the **stack-breadcrumb workflow** (`.github/workflows/stack-brea The proposal states which of these the change is, and why. Decide it while writing the proposal, not when the diff has already grown too big to review. -| Shape | When | How it lands | -| ---------------------------- | --------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------- | -| **Single PR** | The whole change fits one reviewable diff. | Spec, implementation, and the archive land together. | -| **Stacked, merging forward** | Each unit is independently safe in production. | Each PR merges to `main` in turn; the last one archives the change. | -| **Stacked, merging down** | The units are only correct together — an intermediate state would ship a broken or half-migrated product. | Merge each PR **down** into its parent from the tip, then one protected merge of the bottom branch to `main`. The change reaches `main` atomically. | +| Shape | When | How it lands | +| ---------------------------- | ------------------------------------------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Single PR** | The whole change fits one reviewable diff. | Spec, implementation, and the archive land together. | +| **Stacked, merging forward** | Each unit is independently safe in production. | Each PR merges to `main` in turn; the last one archives the change. | +| **Stacked, merging down** | The units are only correct together, and an intermediate state would ship a broken or half-migrated product. | Merge each PR **down** into its parent from the tip, then one protected merge of the bottom branch to `main`. The change reaches `main` atomically. | -**Prefer stacking, and aim to keep an individual diff under ~300 lines.** A 900-line PR does not get reviewed, it gets approved. Tests count toward the total but never split from the code they cover — if a unit is oversized because of its tests, that is usually a sign the unit itself should be smaller. +**Prefer stacking, and aim to keep an individual diff under ~300 lines.** A 900-line PR does not get reviewed, it gets approved. Tests count toward the total but never split from the code they cover. If a unit is oversized because of its tests, that is usually a sign the unit itself should be smaller. -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. +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 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. +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 -`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. +`changeset.yml` looks for a `.changeset/*.md` added or modified **anywhere between `main` and the PR's head**, meaning 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. -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. +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 merely 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 placement rules are unchanged, because they are about review quality rather than about passing a check: @@ -116,14 +116,14 @@ The placement rules are unchanged, because they are about review quality rather ### `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. +**`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. +**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. `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. +- **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. @@ -131,11 +131,11 @@ The shared point: the `on:` filter is not a reliable answer to "where does this 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 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. +**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. +**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. +**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. 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. @@ -147,7 +147,7 @@ Merge each PR **down** into its parent's branch, from the tip to the bottom: - Bring the bottom branch up to date with `main`, let `Validate` pass, then do the **single** protected merge to `main`. - Result: one CI cycle instead of N, and every PR gets a real **Merged** badge (not "closed/absorbed"). -**Merge the down-merges one at a time, not in a loop.** Merging a child immediately invalidates the parent PR's mergeability until GitHub recomputes — `gh pr merge` fails with "Pull Request is not mergeable", and the API reports `rebaseable: null`. In a tight loop this makes merges land **out of order**, which strands the tip's commits part-way down the stack (e.g. `skill`/`eval` never propagate past `help`). Merge each PR, wait for the next to report a boolean `rebaseable`, then continue. +**Merge the down-merges one at a time, not in a loop.** Merging a child immediately invalidates the parent PR's mergeability until GitHub recomputes: `gh pr merge` fails with "Pull Request is not mergeable", and the API reports `rebaseable: null`. In a tight loop this makes merges land **out of order**, which strands the tip's commits part-way down the stack (e.g. `skill`/`eval` never propagate past `help`). Merge each PR, wait for the next to report a boolean `rebaseable`, then continue. **Verify by content, not by ancestry.** Rebase-and-merge replays commits under new SHAs, so the tip's original commits are never ancestors of the branch that absorbed them, and the obvious check reports a false `STRANDED`: @@ -159,7 +159,7 @@ git merge-base --is-ancestor origin/ origin/ git diff --stat origin/ origin/ # empty = fully absorbed ``` -An empty diff with differing SHAs is the *expected* healthy state after a rebase merge, not evidence of a problem. If the diff is genuinely non-empty, reconcile from the tip — a tip branch contains the whole stack — then re-check the diff and push. +An empty diff with differing SHAs is the _expected_ healthy state after a rebase merge, not evidence of a problem. If the diff is genuinely non-empty, reconcile from the tip (a tip branch contains the whole stack), then re-check the diff and push. ### Never `--delete-branch` mid-stack @@ -167,7 +167,7 @@ An empty diff with differing SHAs is the *expected* healthy state after a rebase ### Rebase is the only merge method, and a stack pays for it -`main` keeps a linear history, so the repository allows **rebase-and-merge only** — squash and merge-commit are both disabled. Confirm rather than assume, since this changed: +`main` keeps a linear history, so the repository allows **rebase-and-merge only**; squash and merge-commit are both disabled. Confirm rather than assume, since this changed: ```bash gh api repos/{owner}/{repo} --jq '"squash=\(.allow_squash_merge) merge=\(.allow_merge_commit) rebase=\(.allow_rebase_merge)"' @@ -176,7 +176,7 @@ gh api repos/{owner}/{repo} --jq '"squash=\(.allow_squash_merge) merge=\(.allow_ `gh pr merge --merge` and `--squash` both fail. Use `gh pr merge --rebase`. -**This is the expensive case for a stack, and there is no cheaper option available.** Rebase-and-merge replays the branch onto `main` as *new commits with new SHAs*. Every child then contains the pre-rebase versions of its ancestors' commits, so the child is not merely behind — its history diverged. After each merge you must rebase the next branch onto the updated `main` and force-push it. The old guidance to prefer merge-commits so children stay clean no longer applies; that door is closed. +**This is the expensive case for a stack, and there is no cheaper option available.** Rebase-and-merge replays the branch onto `main` as _new commits with new SHAs_. Every child then contains the pre-rebase versions of its ancestors' commits, so the child is not merely behind: its history diverged. After each merge you must rebase the next branch onto the updated `main` and force-push it. The old guidance to prefer merge-commits so children stay clean no longer applies; that door is closed. Practically, landing a stack now looks like: @@ -190,8 +190,8 @@ git push origin --force-with-lease=:$(git rev-parse origin/)
:` fails with `stale info` when `` is not what the remote currently holds — which includes the case where *you* rebased the branch a moment ago and reached for its old tip. `$(git rev-parse origin/)` after a `git fetch` is the value that works. The failure looks like the shallow-clone symptom in the git section above and is not: check whether the SHA is simply out of date before concluding anything about the clone. +- **Right after a merge, `rebaseable` reads `null`** while GitHub recomputes. Poll until it is a boolean rather than treating `null` as "not mergeable". Reading it as a failure is what stranded commits mid-stack before. +- **Read the lease SHA from the remote, not from memory.** `--force-with-lease=:` fails with `stale info` when `` is not what the remote currently holds, which includes the case where _you_ rebased the branch a moment ago and reached for its old tip. `$(git rev-parse origin/)` after a `git fetch` is the value that works. The failure looks like the shallow-clone symptom in the git section above and is not: check whether the SHA is just out of date before concluding anything about the clone. ### Rebase-and-merge lands unsigned commits on `main` @@ -201,11 +201,11 @@ Commits are signed locally (`git commit -S`, mandatory above), but **GitHub rewr git log --format='%G? %h %s' -5 origin/main # N, N, N, … ``` -Nothing is wrong and nothing needs fixing on `main`. Know it so that `%G?` on a merged commit is not mistaken for a signing failure, and so a fresh commit reading `N` **before** it reaches `main` is recognised as the real problem it is — that one means `-S` was missed. +Nothing is wrong and nothing needs fixing on `main`. Know it so that `%G?` on a merged commit is not mistaken for a signing failure, and so a fresh commit reading `N` **before** it reaches `main` is recognised as the real problem it is: that one means `-S` was missed. ### Recovery if a child PR gets closed by base-branch deletion -This happens when the **parent** PR is merged with `--delete-branch`: deleting the parent's head branch (which is the child's base) closes the **child** PR. Two PRs are involved — the merged parent (``) and the closed child (``); `` is the deleted base, i.e. the parent's head branch. +This happens when the **parent** PR is merged with `--delete-branch`: deleting the parent's head branch (which is the child's base) closes the **child** PR. Two PRs are involved, the merged parent (``) and the closed child (``); `` is the deleted base, i.e. the parent's head branch. 1. Restore the deleted base branch from **GitHub's own copy of the parent's head**, `refs/pull//head`. GitHub keeps that ref after the branch is deleted and after the PR is merged, and it points at the pre-merge tip: @@ -220,9 +220,10 @@ This happens when the **parent** PR is merged with `--delete-branch`: deleting t git rev-parse "$MERGE_SHA^2" # WRONG under rebase-and-merge ``` - `^2` needs the merge commit to *have* two parents, which is true only of a merge-commit merge. Rebase replays the branch as linear single-parent commits, so `^2` fails with "unknown revision" — and this repository is rebase-only, so it fails always. `refs/pull//head` is correct under every merge method, which is the better reason to prefer it. + `^2` needs the merge commit to _have_ two parents, which is true only of a merge-commit merge. Rebase replays the branch as linear single-parent commits, so `^2` fails with "unknown revision", and this repository is rebase-only, so it fails always. `refs/pull//head` is correct under every merge method, which is the better reason to prefer it. + + Take the ref from ``, the PR that actually merged, not from the closed child, whose head is a different branch. - Take the ref from `` — the PR that actually merged — not from the closed child, whose head is a different branch. 2. Reopen the child via **REST** (GraphQL `gh pr reopen` fails on the Projects-classic deprecation): `gh api --method PATCH repos///pulls/ -f state=open` 3. Retarget it: `gh pr edit --base main` (only works once it's open).