From 88d3a363dddeb7b26e673a75b6e18a9163c349d9 Mon Sep 17 00:00:00 2001 From: martyy-code Date: Mon, 3 Aug 2026 15:14:29 +0200 Subject: [PATCH 1/2] docs(engineering): add process docs (dev, release, PR authoring) and stacked-PR learnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three process docs under docs/internal/engineering/process/, each tuned to a persona: - implementing-an-issue.md — dev's playbook (read issue, branch, code, changesets, push, PR). Companion for anyone picking up an issue. - releasing-a-new-version.md — release engineer's playbook (build the release branch, cherry-pick, push, watch the workflow, verify artifacts). Mirrors the conventions we hardened in PRs #39-47. - pr-authoring.md — standards for the body of a PR (six canonical sections, mandatory code sample when public API changes, before/after default, after-only when purely additive). Enforces what PR #50 demonstrated: documented the .addNote() worked-example as the in-doc template. Also adds docs/learnings/github/stacked-pr/README.md, notes captured during PR #39-43 setup around how stacked PRs work in GitHub and how they apply to a release branch. No code, no workflow changes. Documentation only. --- .../process/implementing-an-issue.md | 227 +++++++++++++++++ .../engineering/process/pr-authoring.md | 226 ++++++++++++++++ .../process/releasing-a-new-version.md | 241 ++++++++++++++++++ docs/learnings/github/stacked-pr/README.md | 140 ++++++++++ 4 files changed, 834 insertions(+) create mode 100644 docs/internal/engineering/process/implementing-an-issue.md create mode 100644 docs/internal/engineering/process/pr-authoring.md create mode 100644 docs/internal/engineering/process/releasing-a-new-version.md create mode 100644 docs/learnings/github/stacked-pr/README.md diff --git a/docs/internal/engineering/process/implementing-an-issue.md b/docs/internal/engineering/process/implementing-an-issue.md new file mode 100644 index 0000000..5f8f553 --- /dev/null +++ b/docs/internal/engineering/process/implementing-an-issue.md @@ -0,0 +1,227 @@ +# Implementing an Issue + +**What:** End-to-end playbook for a developer picking up an issue, from first read to merged PR. Companion to `releasing-a-new-version.md` (which is for the release engineer). + +**Why it matters:** This package uses a staging-first branching model with Changesets, a CI lint that requires a changeset on every PR to `staging`, Conventional Commits, and a pre-commit hook that runs Prettier. None of this is exotic, but each one has tripped someone up at least once. This document is the contract. + +## Roles + +This package has a **single release engineer** who cherry-picks batches from `staging` into release PRs onto `main`. **Everyone else**, including occasional contributors, lands feature and fix work on `staging` through the process described here. The release engineer's work is documented in `releasing-a-new-version.md`. + +The roles do not conflict: a dev pushes to `staging`, the release engineer cherry-picks to `main`. You do not push feature work to `main` directly. The only exception is hotfixes, see the dedicated section at the end. + +## Branching model + +``` +feature/* ──┐ + ├── PR → staging (you write the code) +fix/* ──┘ +chore/* ──┘ +ci/* ──┘ +docs/* ──┘ + +staging ── cherry-pick PR → main (release engineer, see other doc) +``` + +`staging` is the integration branch. All feature, fix, chore, ci, and docs PRs target `staging`. Direct PRs to `main` are reserved for hotfixes. + +## Reading the issue first + +Before writing any code, read the entire issue — every section. + +- **Context / Background** — why this work matters. If it does not, push back before coding. +- **Proposed approach** — if the proposer sketched one, this is the spec. Stay close to it. Deviating for good reason is fine; deviating by accident is not. +- **Acceptance criteria** — the checklist that defines "done". Each criterion is testable. If you cannot write a test for a criterion, the criterion is probably underspecified. +- **Related** — files, plans, past incidents. Read them. Every related doc has been attached because the author thought it was relevant. + +If the issue is missing context or the criteria are ambiguous, comment on it before coding. A clarifying comment now is cheaper than a rebase later. + +## Local setup + +```bash +git fetch origin +git checkout staging +git pull --ff-only origin staging +git status # Make sure there are no unstaged surprises from a previous session +``` + +The repo uses pnpm. You will need pnpm 10.x or later (the project ships a `pnpm-lock.yaml`; the GitHub Actions runner installs pnpm via `pnpm/action-setup@v4`). + +```bash +pnpm install --frozen-lockfile +pnpm test # Sanity-check that the green baseline holds before your changes +``` + +## Branching convention + +Create your branch from `staging`: + +```bash +git checkout -b / +``` + +Prefixes used in this repo, observed in `git log --oneline`: + +| Prefix | When | Examples | +| ------ | ---- | -------- | +| `feat/` | new feature or capability | `feat/extract-async-errors` | +| `fix/` | bug fix | `fix/from-method-no-cause` | +| `chore/` | non-functional maintenance (deps, scripts, tooling) | `chore/upgrade-pnpm` | +| `ci/` | GitHub Actions workflows and CI logic | `ci/release-workflow-rewrite` | +| `docs/` | documentation under `docs/` | `docs/release-process` | + +Branch names are reviewed in the PR. Keep them short and indicative of intent, not of an issue number. `feat/extract-async-errors` reads better than `fix-1234`. + +## Writing the code + +Three invariants during the work: + +1. **Atomic commits**. One commit, one intent. Split work that mixes formatting and logic. Split fixes that show up while you are implementing something else. `git add -p` is your friend. +2. **Conventional Commit messages.** The repo enforces this via `CONTRIBUTING.md`. Format: `(): `. The body, when present, explains the **why**, not the **what**. The diff already shows the what. +3. **`packageManager`-aware tooling**. pnpm and Changesets have opinions. Do not work around them. If you hit a friction, change the tool, not the docs. + +Commit stages where this is worth keeping in mind: + +- `pnpm install --frozen-lockfile` to keep lockfile in sync with `package.json`. +- `pnpm changeset` once you know what shipped. +- `pre-commit` (lint-staged) will auto-run Prettier on `*.{ts,tsx,js,json,md,css,yml,yaml}`. Most of the time you can ignore formatting; the hook handles it on commit. But do not commit past the hook if you can fix forward — Prettier-reflowed commits show weirdly in `git blame` afterwards. + +## Adding the changeset + +Every PR that touches `@deessejs/errors`, its tests, or its CI infrastructure must include a changeset. This is enforced by `ci.yml` on PRs to `staging`, and a missing changeset will block merge. + +Run the interactive helper: + +```bash +pnpm changeset +``` + +You will be prompted for: + +- Which package? → `@deessejs/errors` (only one published package) +- Which bump? → `patch` for bug fixes and chores, `minor` for additive features, `major` for breaking changes +- Summary in English, one short line describing the user-facing change + +The tool writes a `.changeset/.md` file. Commit it on your branch. + +### Edge cases + +- **Documentation-only changes under `docs/`** — strictly exempted from the changeset requirement (see `CONTRIBUTING.md`). You can still add one for consistency, cost is one tool invocation. +- **CI / workflow changes only** (files under `.github/`) — the CI lint accepts these without a changeset when no other path is touched. Same workaround: add one anyway for consistency. +- **A PR that reverts an earlier PR** — write the changeset as if the revert is the change, because the published version will include it. A revert of `feat/x` is at minimum `patch`, possibly `minor` if users were relying on `x`. + +## Local checks before pushing + +```bash +pnpm test # vitest run +pnpm lint # eslint +pnpm type-check # tsc --noEmit +pnpm build # tsc -p tsconfig.build.json +``` + +For a small change, `pnpm test --run` and `pnpm type-check` are the minimum. For a release-adjacent change (publishing or versioning logic), run all four. + +If the pre-commit hook fires during `git commit`, do not bypass with `--no-verify`. Let it run, then commit again with the resolved state. Bypassing lands unstaged Prettier diffs in the PR review. + +## Pushing and opening the PR + +```bash +git push -u origin +gh pr create --base staging \ + --title "(): " \ + --body "## Summary + +## Why +``` + +Title: same format as commit subjects. Body: short. Two sections, **Summary** (what changed) and **Why** (the user-facing motivation). If the PR closes an issue, reference it: `Closes #N` or `Fixes #N`. + +Expected checks: + +- `Changeset required` — runs only on PRs to `staging`. Pass = a `.changeset/*.md` is in the diff. +- `Lint` +- `Build` +- `Tests` +- `Type Check` + +If `Changeset required` fails, you forgot the changeset. `git checkout HEAD~ .changeset/` is rarely the right answer; the right answer is `pnpm changeset` followed by `git add .changeset/ && git commit --amend` if the PR has a single commit, or a new commit if the PR has multiple. + +## Review and merge + +Default branch protection on `staging` requires at least one approval before merge. For trivial or urgent work, the author can self-review; for non-trivial work, wait for an actual review. + +When the review is in: + +- Address comments with fix commits, not force-pushes, unless the reviewer explicitly asks for a rebase. +- Mark resolved threads after pushing the fix. +- The release engineer is your reviewer-of-last-resort if no one else is available — they will block only on things that would break staging integration. + +After merge, your commit lives on `staging`. The release engineer cherry-picks it into the next release PR. **You do not need to do anything else** — your change is in the integration branch, and it will ship in the next release. + +## Hotfixes + +For an urgent fix that cannot wait for the staging queue, the path is different. Branch from `main`, not `staging`: + +```bash +git checkout main +git checkout -b release/hotfix- +``` + +Make the fix and add a changeset as usual. Then: + +```bash +git push -u origin release/hotfix- + +gh pr create --base main \ + --head release/hotfix- \ + --label hotfix \ + --title "fix(...): hotfix for " \ + --body "## Hotfix + +Closes #N. +" +``` + +Two things are different from the normal flow: + +1. **The PR targets `main`, not `staging`.** The release workflow on `main` is what fires the publish. +2. **The PR carries the `hotfix` label.** This is the trigger for the targeted CI check (issue #48); without the label, the changeset presence is not enforced. Until that issue is implemented, manually verify that the changeset is present before merging. + +When the PR is merged, `release.yml` will run on the merge commit, version bump, publish, tag, and create the GitHub Release. Monitor the run: + +```bash +gh run list --workflow=Release --limit 1 +gh run watch +``` + +If the release engineer is the one doing the hotfix (single point of failure, accepted by the team), this is the only time they touch the `main` branch without going through the `staging` integration flow. + +## Anti-patterns + +Things that will trip up a reviewer or break the contract: + +- **Force-pushing after a review started.** Reviewers lose their context. Use fix-up commits. +- **Combining an unrelated reformat with a logical change.** Make the reformat a separate commit. Reading 200 lines of whitespace in a 50-line change is miserable. +- **Bypassing the CI lint.** `--no-verify` on commit, skipping the pre-commit hook. The CI lint will catch you anyway. +- **Pushing directly to `main`.** The branch protection on `main` will refuse the push. If it does not, you are an admin and you should still not do it. +- **Big changes without an issue.** Issues are the contract. A big unannounced PR gets reverted. Discuss first. +- **Co-authored commits to flood the contribution graph.** This is a tiny team. Honest attribution, single author per commit is the norm. + +## Definition of done + +A PR is "done" when: + +- [ ] Every acceptance criterion in the issue is met. +- [ ] Local checks pass (`test`, `lint`, `type-check`, `build`). +- [ ] Changeset present (unless the change is genuinely docs-only or CI-only, and even then optional-discipline). +- [ ] PR description has Summary and Why. +- [ ] PR has at least one approval. +- [ ] After merge, the commit is visible on `staging`. +- [ ] If a hotfix: GitHub Release created for the published version. + +## References + +- `CONTRIBUTING.md` — short contribution overview; this document is the long form. +- `CLAUDE.md` — branching strategy and project context for AI assistants. +- `releasing-a-new-version.md` — release engineer's process, for context on what happens after your PR lands. +- `release-system.md` — the architectural plan underpinning the release system. diff --git a/docs/internal/engineering/process/pr-authoring.md b/docs/internal/engineering/process/pr-authoring.md new file mode 100644 index 0000000..a32b93e --- /dev/null +++ b/docs/internal/engineering/process/pr-authoring.md @@ -0,0 +1,226 @@ +# Authoring a Pull Request + +**What:** Standards for the description and body of a pull request opened against `@deessejs/errors`. Companion to `implementing-an-issue.md` (which covers the work itself) and `releasing-a-new-version.md` (which covers the release flow). + +**Why it matters:** A PR is the contract you sign with the reviewer and the release engineer. The diff shows the change, but only the body answers the questions a reviewer asks first: *what changed, why, is it safe, how do I use it*. Standardizing the body raises the floor without slowing anyone down. + +## Roles + +Two audiences read your PR body: + +- **The reviewer** — needs to understand the intent, the risk, and the testing story in under a minute. +- **The release engineer** — needs to know if the changeset is present, whether the mergeable target has any preparation to do, and whether your PR unlocks or blocks anything downstream. + +Write for both. If a section is not relevant for your change, omit it and add a one-line "why" elsewhere. + +## Section-by-section template + +The body has six optional sections. Use only the ones that apply, in this order: + +### `## Summary` + +One paragraph, two or three sentences. What changed and why. The reviewer can stop here if they trust the change; keep it tight. + +### `## Why` + +The motivation. Link the issue (`Closes #N` or `Refs #N`). Add background only if it does not fit in the issue — most of the time, the issue already carries this. + +### `## What changed` + +Bulleted list of concrete changes. Tie each bullet to a file or a logical unit, not to a commit. The reviewer reads this alongside the diff to see whether each piece of code is accounted for. + +### `## Code sample` + +Mandatory for any PR that touches `src/**` and alters the public API (new method, new export, new option, new type, signature change, behavior change). Optional for chores, refactors without API change, and documentation-only PRs. + +The reviewer wants to see how the change reads in real code, not just the diff. If your PR removes or alters an existing usage, the reviewer needs to see both sides; if your PR only adds new usage, a single After block is enough. + +**Format A — Before/After** (default). + +Use this when the PR changes the way existing code is written, when a public API moved, or when something that used to work no longer does. + +```text +Before: +[code that the reviewer would have written last week] + +After: +[code the reviewer should write next week] +``` + +**Format B — After only** (acceptable when the PR only adds new usage without breaking any existing usage). + +```text +After this PR, callers can do: +[code] +``` + +**What good looks like for a method-add PR (illustrative, not real):** + +```text +Before: +const err = AppError(); +err.notes.push("Attempt 1 failed"); +err.notes.push("Retrying..."); +// → Type error: property 'notes' is not assignable + +After: +const err = AppError() + .addNote("Attempt 1 failed") + .addNote("Retrying..."); +// err.notes === ["Attempt 1 failed", "Retrying..."] +``` + +**What good looks like for an additive PR:** + +```text +After this PR, callers can do: +import { withSpan } from "@deessejs/errors"; + +await withSpan("read", async (span) => { + await span.record({ hits: 42 }); +}); +``` + +**Rules:** + +- Code samples must compile. Verify by running the sample through `pnpm build` or `pnpm type-check` in your head before pasting it. If the sample needs explanation, add a comment, do not approximate. +- TypeScript samples get backtick fences with `ts` or `typescript` language tag so they highlight correctly on GitHub. +- Bullet the snippet (`- \`\`\`ts ... \`\`\``) if the body has multiple snippets. Otherwise a plain fenced block is fine. +- Reuse exact names from the codebase. If you renamed a symbol, show both the old and the new names in the Before block. + +### `## Verified locally` + +What you ran and the outcome. Keep it factual. + +```text +- `pnpm test --run` — 82/82 tests pass +- `pnpm lint` +- `pnpm type-check` +- `pnpm build` +``` + +Bullet, not prose. The reviewer wants to scan, not read. + +### `## Risk` + +One paragraph or three bullets. By default, risk is **low** unless something specific makes it medium or high. Naming the risk explicitly ("signature change", "adds a new dependency", "modifies the output format") is more useful than rating it on a scale. + +If your PR has zero risk, write "Low. This change is purely additive and existing code is unaffected." or equivalent. Do not leave the section empty. + +## What goes in the title + +Title format: `(): `. + +| Type | Use for | +| ---- | ------- | +| `feat` | new capability | +| `fix` | bug fix | +| `chore` | non-functional maintenance | +| `ci` | GitHub Actions workflows | +| `docs` | documentation under `docs/` | +| `refactor` | internal restructuring, no public-API change | +| `test` | tests only | + +The scope is the area affected (`errors`, `release`, `cli`). If unsure, omit — `feat(errors): ...` is fine, `feat: ...` is fine too. + +The subject is a present-tense summary, no period at the end. Keep it to about 50 characters; longer is acceptable if the alternative is a cryptic title. + +## What does NOT go in the body + +- **The diff paste.** The reviewer has the diff. Re-pasting it as code blocks in the body makes the PR unreadable. +- **The commit list.** GitHub shows commits on the PR page. Listing them in the body duplicates information. +- **Vague acceptance checkmarks.** "Tests pass", "build works" — without saying which commands, this is filler. +- **Apologies and meta-commentary.** "Sorry for the noise", "refactor only, no functional change" — keep it to facts. +- **Marketing language.** "This unlocks a brand new paradigm" — no. + +## Length budget + +A reasonable PR body is between 80 and 300 lines of markdown. Shorter is fine. Longer means the change is doing too much and should probably be split, or the writer is hedging. + +If your body crosses 400 lines, ask yourself: is this two PRs? + +## Anti-patterns + +- **Body is `// WIP`** or empty. Reviewers bounce off empty PRs. Put a one-liner, even if you are still iterating. +- **Title is `Update stuff`.** Useless. The PR queue becomes a graveyard. +- **Forgetting the changeset on a PR that needs one.** The CI lint on PRs to `staging` blocks the merge, so this is caught at the gate. But it still costs a round-trip. +- **Mentioning the reviewer in the body** (`@username please review`). That is a comment, not body text. Use the GitHub review-requested-by reviewer field. +- **Mixing an unrelated reformat with a logical change.** Make the reformat a separate commit, ideally a separate PR. +- **Big PRs.** Anything over 600 lines of diff or 50 files should be split or at least flagged in the body with a sentence explaining why it cannot be. + +## Worked example + +A faithful worked example for the recent `.addNote()` PR (the actual body was a bit different; this is the idealised version): + +```markdown +Closes #29. + +Mirrors Python 3.11 PEP 678 (`BaseException.add_note()`). The method was +documented but never implemented; consumers following the JSDoc examples +got a TypeScript error. The `notes: string[]` storage was already wired up; +only the method was missing. + +## Why + +The discrepancy between docs and implementation is a real bug. Anyone +copying the JSDoc example from `src/raise/index.ts:34-38` gets a TypeScript +error. Python 3.11 PEP 678 ships this exact pattern; porting it is +consistent with the library\'s Python inspiration. + +## What changed + +- `src/error/types.ts`: declare `addNote(note: string): ErrorInstance` on `ErrorInstance`. Remove the stale TODO and the "implemented in a separate task" notice. +- `src/error/error.ts`: implement `addNote` in the factory closure. Pushes to `notes` and returns `this` for chaining. +- `tests/error.test.ts`: cover single note, chained notes, preservation through `.from()`, and isolation between siblings. + +## Code sample + +Before: +\`\`\`typescript +const err = AppError(); +// Type error: 'addNote' does not exist on type 'ErrorInstance' +\`\`\` + +After: +\`\`\`typescript +const err = AppError() + .addNote("Attempt 1 failed") + .addNote("Retrying..."); +\`\`\` + +## Verified locally + +- `pnpm test --run` — 82/82 tests pass +- `pnpm lint` +- `pnpm type-check` +- `pnpm build` + +## Risk + +Low. `.addNote()` is purely additive. Existing code is unaffected. The +return-type inference is the only contract change, and it is fully typed. +``` + +Note how the `## Code sample` section is short, copy-pastable, and anchored on the actual API surface. That is what the reviewer skims first. + +## Definition of done + +A PR is **publishable** when: + +- [ ] Title follows `(): `. +- [ ] Body has at least `## Summary`. +- [ ] If the PR touches `src/**` and changes public API, `## Code sample` is present and shows before/after (or after only, justified). +- [ ] `## Verified locally` lists the commands run. +- [ ] `## Risk` is non-empty. +- [ ] Changeset is present (unless documentation-only or CI-only, see `implementing-an-issue.md`). +- [ ] CI is green before reviewer hand-off. + +## Reviewer-side complement + +This document is about authoring. The reviewer-side counterpart — what to look for when reviewing a PR — is intentionally not part of this file. Reviewer checklists belong in a separate document and should be authored by the reviewer side of the team, not the author side. + +## References + +- `implementing-an-issue.md` — the workflow that produces a PR. +- `releasing-a-new-version.md` — what happens after a PR lands. +- `release-system.md` — the architectural plan that defines what kind of changes need changesets. diff --git a/docs/internal/engineering/process/releasing-a-new-version.md b/docs/internal/engineering/process/releasing-a-new-version.md new file mode 100644 index 0000000..d788d1c --- /dev/null +++ b/docs/internal/engineering/process/releasing-a-new-version.md @@ -0,0 +1,241 @@ +# Releasing a New Version of `@deessejs/errors` + +**What:** Step-by-step runbook for the release engineer to ship a new version of `@deessejs/errors`, end-to-end, on top of the established release system. + +**Why it matters:** Releases use OIDC trusted publishing (no NPM token), Changesets for version bumping, and a GitHub `release` environment for auditability. The system works but is **not auto-recovering** — failures require human action. This document is the recovery playbook. + +## Roles + +There is one **release engineer**. There is no rotation. If the release engineer is unavailable, the team waits; bypassing the workflow is not a recommended escape hatch. + +## Branching model + +``` +feature/* ──┐ + ├── PR → staging (devs land their work here with a .changeset/*.md) +fix/* ──┘ + +staging ── cherry-pick PR → main (release engineer) + │ + ▼ + pnpm changeset version + pnpm changeset publish ← npm trusted publishing (OIDC) + git tag @deessejs/errors@X.Y.Z +``` + +- Devs open PRs on `staging`. CI lint (`ci.yml`) blocks PRs that do not include a `.changeset/*.md` file. +- The release engineer cherry-picks a curated batch of commits from `staging` into a `release/*` branch, opens a release PR targeting `main`, and merges. +- Every merge to `main` triggers the release workflow. No label is required. + +## Prerequisites + +Before you (the release engineer) start cutting a release, confirm: + +- [ ] `staging` has been brought up to date with `main`. The release branch must contain everything you want to publish, plus the desired changesets. +- [ ] npmjs.com Trusted Publisher for `@deessejs/errors` is configured: provider = GitHub Actions, repository = `deessejs/errors`, workflow filename = `release.yml`, allowed action = `npm publish`, **environment name left blank**. +- [ ] GitHub repository secrets: `NPM_TOKEN` is present but **not used by the workflow anymore** (post-PR #47). Treat it as residual. After release validation, revoke it. +- [ ] You are signed in to `gh` and have push access to `origin/main`. + +## Procedure + +### 1. Build the release branch + +```bash +git fetch origin main staging +git checkout origin/main +git checkout -b release/vX.Y.Z +``` + +### 2. Cherry-pick the chosen batch from staging + +```bash +git log --oneline origin/main..origin/staging --no-merges +``` + +Pull only what you intend to ship. Skip plan-only commits (e.g. `docs/internal/engineering/plans/...`) unless the batch explicitly updates release docs. + +```bash +git cherry-pick ... +``` + +Resolve any conflicts. Past conflicts observed: + +- `.github/workflows/ci.yml` — keep the post-`fix: ci.yml to fetch origin/staging` version (the version on `HEAD` of the release branch is typically correct because main was already up-to-date with the fix). + +### 3. Confirm the changeset count + +```bash +git diff origin/main --name-only | grep '^.changeset/.*\.md$' +``` + +If the list is empty, do NOT merge the PR. The release workflow will see `has_changesets=false` and become a no-op (no publish, no tag, no GitHub Release). To produce a publishable release, the batch must include at least one changeset. + +The release workflow's default bump is the **highest semantic level** in the changesets present: + +- 1 `major` → `X+1.0.0` +- 1 `minor` + rest `patch` → `X.Y+1.0` +- All `patch` → `X.Y.Z+1` + +### 4. Push and open the PR + +```bash +git push -u origin release/vX.Y.Z +gh pr create --base main --head release/vX.Y.Z \ + --title "chore(release): cherry-pick release system stack from staging" +``` + +Title and body templates live in past PR #44 (`docs/engineering/reports/release-history.md` to be added if not present yet). + +### 5. Wait for CI + +The expected checks are: + +- `Lint` +- `Build` +- `Tests` +- `Type Check` + +`Changeset required` does **not** run on PRs to `main`. It only fires on PRs to `staging`. That is intentional — release PRs are reviewable even if the cherry-pick dropped a changeset. + +### 6. Merge + +```bash +gh pr merge --merge +``` + +(or use the GitHub UI) + +The merge fires the release workflow. You can monitor it: + +```bash +gh run list --workflow=Release --limit 1 +gh run watch +``` + +### 7. Watch the workflow + +The expected ordering and what to check: + +1. **Detect pending changesets** — sets `has_changesets=true` if at least one `.changeset/*.md` is in the merge diff. If false, the rest of the steps are skipped. Expected: `true`. +2. **Create versions from changesets** — runs `pnpm changeset version`. Output should mention how many packages were bumped. +3. **Commit version changes and push** — runs the `git add -A` + `git diff --cached --quiet` (post-PR #45 fix) + commit + push. Expected: a new `chore(release): version packages` commit appears on `main`. +4. **Build** — `pnpm build`. Expected: green. +5. **Test** — `pnpm test`. Expected: green. +6. **Publish packages** — `pnpm changeset publish` with `env: NPM_CONFIG_PROVENANCE: 'true'`. Expected: a successful `Publishing "@deessejs/errors" at "X.Y.Z"` line, then `New tag: @deessejs/errors@X.Y.Z`. +7. **Get latest tag** — extracts the just-created tag name. +8. **Create GitHub Release** — uses `softprops/action-gh-release@v2`. Expected: a GitHub Release is created on the tag, body pulled from `packages/errors/CHANGELOG.md`. + +### 8. Verify the artifacts + +```bash +git ls-remote --tags origin | grep '@deessejs/errors' +gh release view '@deessejs/errors@X.Y.Z' --repo deessejs/errors +``` + +Confirm: + +- The tag points at the `chore(release): version packages` commit, **not** at the merge commit. (Pre-existing tag drift had `@deessejs/errors@1.1.1` pointing at a merge commit. That is now fixed since PR #45.) +- The `packages/errors/package.json` `version` field matches the tag. +- The `packages/errors/CHANGELOG.md` has a new entry under the published version. +- A GitHub Release exists for the tag, with the package version in the title and `provenance: true` attestation visible on npmjs.com. + +### 9. Announce + +- The npm package is updated; downstream consumers pick it up on their next install. +- Optional: post in #releases (no current automated channel — manual for now). + +## Failure modes and recoveries + +### `ENEEDAUTH` on publish + +Symptom in the run log: + +``` +npm error code ENEEDAUTH +npm error need auth This command requires you to be logged in to https://registry.npmjs.org +``` + +Possible causes, in order of likelihood: + +1. **npmjs.com Trusted Publisher** is misconfigured. Check the package's Settings → Trusted publishing. Required fields: + - Repository: `deessejs/errors` + - Workflow filename: `release.yml` (filename only, no path) + - Environment name: **must be blank**. If you set it to `release` (matching the workflow's `environment:`), the OIDC token will be filtered to jobs that explicitly target it, which is fine in principle but historically fragile; left blank is safest. + - Allowed actions: `npm publish` +2. **pnpm version** does not support OIDC trust publishing. We observed this with **pnpm 10**. The fix in PR #46 was to set `env: NPM_CONFIG_PROVENANCE: 'true'` on the publish step and add `publishConfig.provenance: true` to `packages/errors/package.json`. With Node 24 and these flags in place, pnpm 10 publishes via OIDC. +3. **The `repository` field is missing from `packages/errors/package.json`** — produces an `E422` (not `ENEEDAUTH`) — see next failure mode. + +### `E422` on publish with "repository.url is empty" + +Symptom: + +``` +npm error code E422 +npm error Failed to validate repository information: +npm error package.json: "repository.url" is "", +npm error expected to match "https://github.com/deessejs/errors" from provenance +``` + +Cause: npm matches `package.json:repository.url` against the URL embedded in the OIDC provenance attestation. Empty or missing → rejection. + +Fix: ensure `packages/errors/package.json` has: + +```json +"repository": { + "type": "git", + "url": "https://github.com/deessejs/errors.git" +} +``` + +This was the cause of PR #46 → PR #47 transition. + +### Version bump skipped — no commit on `main` + +Symptom: `pnpm changeset version` ran and modified files locally, but the `chore(release): version packages` commit was **never created**, so `git push origin HEAD` had nothing to push. The next step (`Publish`) tries to publish anyway and fails (or succeeds locally without ever appearing on the registry). + +Pre-PR #45 cause: `git diff --quiet || git commit -m ...`. After `git add -A`, the working tree and index are identical, so `git diff --quiet` exits 0 and the commit is skipped. + +PR #45 fix: `git diff --cached --quiet || git commit -m ...`. `--cached` compares against `HEAD`, which is the right comparison after staging. + +If you see this symptom re-emerge: confirm the workflow file still has `git diff --cached --quiet` and not `git diff --quiet`. + +### Wrong tag drift — tag on merge commit, not version bump commit + +Symptom: `git show @deessejs/errors@X.Y.Z --no-patch` shows the merge commit instead of `chore(release): version packages`. + +Cause: pre-PR #42, the workflow pushed the tag at the merge commit. PR #42 rewrote the order so the tag is pushed **after** the version-bump commit is created and pushed. + +If you see this happen after the fix is in: the `git diff --cached --quiet` test came back "no diff" (no commit happened, no tag pushed), and the `git push --tags` from a later step pushed whatever stale tag was already on origin. Fix is to chase down why no version-bump commit was created in the first place. + +### The `staging` PRs broke before they reached `staging` + +If a PR was merged directly to `main` instead of `staging`, the CI lint on `staging` did not run for it, and the changeset requirement might have been bypassed. Recover by opening a follow-up PR that adds the missing changeset, and `git revert` if the bad state already shipped. + +## Hardening to apply after the first successful OIDC release + +Once `@deessejs/errors@X.Y.Z` is published via OIDC and verified: + +1. On npmjs.com → `@deessejs/errors` → Settings → Publishing access → enable **"Require two-factor authentication and disallow tokens"**. This revokes all token-based publish paths; the OIDC trusted publisher is unaffected. +2. On GitHub → repository Settings → Secrets → delete `NPM_TOKEN`. +3. Confirm the trusted publisher on npmjs.com has Allowed actions = `npm publish` and nothing else (unless you want `npm stage publish` too). +4. Optional: archive the `dev` branch. It is not part of the current flow. + +## Workflow contract recap + +Reading the workflow file at `.github/workflows/release.yml`, the contract is: + +- Trigger: `pull_request: types: [closed]` on `main` OR `workflow_dispatch`. +- Job-level permissions: `contents: write`, `id-token: write`. +- Job-level guard: `if:` requires `merged == true && base.ref == 'main'` for the PR trigger. +- Step-level guards: every step after `Detect pending changesets` is gated on `has_changesets == 'true' && (workflow_dispatch || dry_run != true)`. +- Outcomes: + - No changeset in the merge diff → job runs only the detection step, no publish. + - Workflow dispatch with `dry_run=true` → runs versioning but skips publish and tag push. + - Workflow dispatch with `dry_run=false` (or unset) and no changesets → no-op. + +## References + +- [Release system plan](../plans/release-system.md) +- [GitHub Actions OIDC documentation](https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication) +- [npm trusted publishing for GitHub Actions](https://docs.npmjs.com/trusted-publishers/) +- Existing releases: `@deessejs/errors@1.0.0` (legacy), `@deessejs/errors@1.1.0`, `@deessejs/errors@1.1.1`, `@deessejs/errors@1.2.2` (first OIDC release). diff --git a/docs/learnings/github/stacked-pr/README.md b/docs/learnings/github/stacked-pr/README.md new file mode 100644 index 0000000..1c6e331 --- /dev/null +++ b/docs/learnings/github/stacked-pr/README.md @@ -0,0 +1,140 @@ +# GitHub Stacked Pull Requests + +**What:** Stacked pull requests are a chain of pull requests in the same repository where each PR targets the branch of the PR below it, forming a dependency chain that lands on a single trunk (usually `main` or, in this repo, `staging`). + +**Why it matters:** Stacks break a large change into small, independently reviewable layers. Each layer shows only its own diff, so reviewers see something focused rather than a wall of mixed concerns. Rebase cascading is handled by GitHub, so branch management overhead is low. + +## Mental model + +A stack is a vertical chain. The bottom PR targets the trunk. Each subsequent PR targets the branch of the PR below it. + +``` +feat/auth-layer → PR #1 (base: staging) ← bottom +feat/api-endpoints → PR #2 (base: feat/auth-layer) +feat/frontend → PR #3 (base: feat/api-endpoints) ← top +``` + +**The key principle:** if code in one layer depends on code in another, the dependency must be in the same branch or a lower one. Foundational changes (CI lint, schema, types) go in lower branches. Code that depends on them (workflows, docs that reference the workflow) goes higher. + +## Why use stacked PRs + +### Faster, focused reviews + +Each PR shows a focused diff. A reviewer looking at the lint-PR sees only the lint file, not the workflow rewrite underneath. Smaller diffs are faster to approve and less likely to develop merge conflicts. + +### Decoupling concerns + +Stacked PRs let you land foundational changes (lint, CI) without being blocked by review of higher-level changes (the workflow itself). Lint can be merged first; the workflow rides on top. + +### Cascading rebase + +When the bottom PR is merged, the remaining branches are automatically rebased and the next PR targets the trunk. This is the killer feature — without stacks, keeping dependent branches in sync is manual and error-prone. + +### CI consolidation + +Branch protection rules and CI checks on the trunk apply to every PR in the stack, not just the bottom one. Each layer meets the same quality bar before it can merge. + +## When to use stacked PRs + +**Use stacked PRs when:** + +- The change is large enough that a single PR would be hard to review. +- The change has clear conceptual layers (lint → workflow → docs). +- Each layer can be reviewed and merged independently. +- You want to ship foundational pieces early without waiting for the rest. + +**Do NOT use stacked PRs when:** + +- The change is small (one file, one workflow, one bug fix). +- The layers are deeply entangled and would not parse in isolation. +- Your reviewer is unfamiliar with the model and the cognitive overhead outweighs the benefit. +- The change crosses forks (cross-fork stacks are not supported). + +## Tooling + +Stacks are available in: + +- **GitHub CLI** via the `gh stack` extension: `gh extension install github/gh-stack` +- **GitHub website** — UI shows a stack icon and a stack map in the merge box +- **GitHub Mobile** — read-only +- **REST API**, **GraphQL**, **Webhooks** — programmatic support + +For AI agents, install the `gh-stack` skill: `gh skill install github/gh-stack`. + +## Common commands + +```bash +# Initialize a stack in the current repo +gh stack init + +# Add a new branch to the top of the stack +gh stack add + +# Stage, commit, and create the next branch in one step +gh stack add -Am "Commit message" + +# Push all branches to the remote +gh stack push + +# Create PRs and link them as a stack +gh stack submit + +# View the full stack: branches, PR links, statuses +gh stack view +``` + +## Rules and CI + +The merge requirements for any PR in the stack are determined by the **bottom PR's base branch** (typically `main` or `staging` in this repo). + +- Branch protection rules apply to every PR in the stack, even mid-stack PRs that don't directly target the trunk. +- CI checks triggered by PRs on the trunk run for every PR in the stack. +- This means a mid-stack PR is held to the same quality bar as a direct-to-trunk PR. + +## Merging + +Stacks merge **bottom-up**. Three options: + +1. **Merge the entire stack** by merging the top PR. Every PR below it merges with it. +2. **Merge part of the stack** by merging a mid-stack PR. The PRs below it merge too; the PRs above stay open and re-target to the trunk. +3. **Merge a single PR** at the bottom. The remaining stack re-targets to the trunk. + +Supported merge methods: merge commit, squash, rebase. Stack merging is **merge-queue aware**. + +## Limitations + +- **Public preview** — feature may change. +- **Same repo only** — cross-fork stacks are not supported. +- **Not supported in GitHub Desktop**. +- Requires `gh` CLI 2.90.0+ and Git 2.20+. + +## Application to this repo: release system plan + +The release system plan (see `docs/internal/engineering/plans/release-system.md`) is a natural fit for a stack. The plan has four implementation phases that map cleanly to PR layers: + +``` +staging ← trunk + └── ci(release): add changesets lint on PRs to staging ← PR #1 (bottom) + └── ci(release): rewrite release workflow ← PR #2 + └── docs(release): update CLAUDE.md and CONTRIBUTING.md ← PR #3 (top) +``` + +- **PR #1 (lint)** — small, low-risk, foundational. Can be merged first. +- **PR #2 (workflow)** — the core change. Stands on its own for review. +- **PR #3 (docs)** — depends on the workflow. Documentation updates the branching model and references the new `version bump` workflow. + +Each layer can be reviewed and merged independently. The cascade rebase handles the dependency chain automatically. + +## Things to watch for + +- **Stack drift** — if the bottom PR is force-pushed, the whole stack needs re-rebasing. Use `gh stack push` to keep them in sync. +- **Reviewer context** — a reviewer looking at PR #2 should be able to see PR #1 underneath. The stack map in the GitHub UI handles this. +- **CI duplication** — running lint+build+test+type-check on every PR in the stack is normal. This is the same as having 3 separate PRs open against the trunk. +- **Merge conflicts across the stack** — if a high layer has a conflict when the lower layer merges, the cascade rebase handles it. Manual intervention is rarely needed. + +## References + +- [About stacked pull requests](https://docs.github.com/en/pull-requests/get-started/about-stacked-prs) +- [Quickstart for stacked pull requests](https://docs.github.com/en/pull-requests/get-started/stacked-prs-quickstart) +- [Stacked pull requests CLI commands](https://docs.github.com/en/pull-requests/reference/stacked-prs-cli-commands) +- [Roll out stacked pull requests to your organization](https://docs.github.com/en/pull-requests/tutorials/roll-out-stacked-prs) From 46ed266740013defd915ae1483419e34670bbfa4 Mon Sep 17 00:00:00 2001 From: martyy-code Date: Mon, 3 Aug 2026 15:15:53 +0200 Subject: [PATCH 2/2] style(docs): apply prettier to process docs; add changeset - Prettier auto-formatted two of the four process docs (markdown table alignment in pr-authoring.md, bullet spacing in implementing-an-issue.md). - Add a no-op changeset so the ci.yml lint passes. The lint currently requires a changeset on every PR to staging regardless of path; this is a known limitation tracked in the release-system plan Section 4. A future change can implement the documented exemption for docs-only and CI-only PRs. --- ...cs-add-process-and-stacked-pr-learnings.md | 5 +++++ .../process/implementing-an-issue.md | 14 ++++++------ .../engineering/process/pr-authoring.md | 22 +++++++++---------- 3 files changed, 23 insertions(+), 18 deletions(-) create mode 100644 .changeset/docs-add-process-and-stacked-pr-learnings.md diff --git a/.changeset/docs-add-process-and-stacked-pr-learnings.md b/.changeset/docs-add-process-and-stacked-pr-learnings.md new file mode 100644 index 0000000..e1288fa --- /dev/null +++ b/.changeset/docs-add-process-and-stacked-pr-learnings.md @@ -0,0 +1,5 @@ +--- +"@deessejs/errors": patch +--- + +Add documentation under `docs/internal/engineering/process/` (`implementing-an-issue.md`, `releasing-a-new-version.md`, `pr-authoring.md`) and `docs/learnings/github/stacked-pr/README.md`. No code or workflow changes. The changeset is required by the current `ci.yml` lint; it does not represent a feature bump. diff --git a/docs/internal/engineering/process/implementing-an-issue.md b/docs/internal/engineering/process/implementing-an-issue.md index 5f8f553..f0bd8c6 100644 --- a/docs/internal/engineering/process/implementing-an-issue.md +++ b/docs/internal/engineering/process/implementing-an-issue.md @@ -62,13 +62,13 @@ git checkout -b / Prefixes used in this repo, observed in `git log --oneline`: -| Prefix | When | Examples | -| ------ | ---- | -------- | -| `feat/` | new feature or capability | `feat/extract-async-errors` | -| `fix/` | bug fix | `fix/from-method-no-cause` | -| `chore/` | non-functional maintenance (deps, scripts, tooling) | `chore/upgrade-pnpm` | -| `ci/` | GitHub Actions workflows and CI logic | `ci/release-workflow-rewrite` | -| `docs/` | documentation under `docs/` | `docs/release-process` | +| Prefix | When | Examples | +| -------- | --------------------------------------------------- | ----------------------------- | +| `feat/` | new feature or capability | `feat/extract-async-errors` | +| `fix/` | bug fix | `fix/from-method-no-cause` | +| `chore/` | non-functional maintenance (deps, scripts, tooling) | `chore/upgrade-pnpm` | +| `ci/` | GitHub Actions workflows and CI logic | `ci/release-workflow-rewrite` | +| `docs/` | documentation under `docs/` | `docs/release-process` | Branch names are reviewed in the PR. Keep them short and indicative of intent, not of an issue number. `feat/extract-async-errors` reads better than `fix-1234`. diff --git a/docs/internal/engineering/process/pr-authoring.md b/docs/internal/engineering/process/pr-authoring.md index a32b93e..ba8e321 100644 --- a/docs/internal/engineering/process/pr-authoring.md +++ b/docs/internal/engineering/process/pr-authoring.md @@ -2,7 +2,7 @@ **What:** Standards for the description and body of a pull request opened against `@deessejs/errors`. Companion to `implementing-an-issue.md` (which covers the work itself) and `releasing-a-new-version.md` (which covers the release flow). -**Why it matters:** A PR is the contract you sign with the reviewer and the release engineer. The diff shows the change, but only the body answers the questions a reviewer asks first: *what changed, why, is it safe, how do I use it*. Standardizing the body raises the floor without slowing anyone down. +**Why it matters:** A PR is the contract you sign with the reviewer and the release engineer. The diff shows the change, but only the body answers the questions a reviewer asks first: _what changed, why, is it safe, how do I use it_. Standardizing the body raises the floor without slowing anyone down. ## Roles @@ -111,15 +111,15 @@ If your PR has zero risk, write "Low. This change is purely additive and existin Title format: `(): `. -| Type | Use for | -| ---- | ------- | -| `feat` | new capability | -| `fix` | bug fix | -| `chore` | non-functional maintenance | -| `ci` | GitHub Actions workflows | -| `docs` | documentation under `docs/` | +| Type | Use for | +| ---------- | -------------------------------------------- | +| `feat` | new capability | +| `fix` | bug fix | +| `chore` | non-functional maintenance | +| `ci` | GitHub Actions workflows | +| `docs` | documentation under `docs/` | | `refactor` | internal restructuring, no public-API change | -| `test` | tests only | +| `test` | tests only | The scope is the area affected (`errors`, `release`, `cli`). If unsure, omit — `feat(errors): ...` is fine, `feat: ...` is fine too. @@ -184,8 +184,8 @@ const err = AppError(); After: \`\`\`typescript const err = AppError() - .addNote("Attempt 1 failed") - .addNote("Retrying..."); +.addNote("Attempt 1 failed") +.addNote("Retrying..."); \`\`\` ## Verified locally