diff --git a/.claude/settings.json b/.claude/settings.json index 8b9e35e9cb..448a471de0 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -7,6 +7,7 @@ "Skill(create-package)", "Skill(glossary)", "Skill(pr)", + "Skill(prose-pass)", "Skill(push)" ], "deny": ["Read(**/.env)", "Read(**/.env.*)"] diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md index 83ad493799..f877bc98e8 100644 --- a/.claude/skills/code-review/SKILL.md +++ b/.claude/skills/code-review/SKILL.md @@ -12,6 +12,10 @@ When asked to review code, whether a PR on GitHub or some local changes, provide - Test coverage Use the repository's CLAUDE.md for guidance on style and conventions. +For comments, changelog entries, and PR bodies, review against the standard in +[`.claude/skills/prose-pass/SKILL.md`](../prose-pass/SKILL.md) — read it as +criteria; do not invoke it, since it edits files and a review may have nothing +checked out. If you encounter terms that should be added to the glossary, invoke the glossary skill using: Skill tool with skill="glossary" Be constructive and helpful in your feedback. diff --git a/.claude/skills/commit/SKILL.md b/.claude/skills/commit/SKILL.md index 002c090e3d..de95657acd 100644 --- a/.claude/skills/commit/SKILL.md +++ b/.claude/skills/commit/SKILL.md @@ -7,7 +7,7 @@ When asked to commit code, follow these steps: ## Arguments -- `check` (default): Run checks first to lint, build, and test the code. Stop if any checks fail. +- `check` (default): Lint, build, and test the code at step 3, after the prose pass. Stop if any checks fail. - `force`: Skip the check step and commit directly. ## Steps @@ -18,20 +18,24 @@ When asked to commit code, follow these steps: - `git diff HEAD` to see both staged and unstaged changes - `git log --oneline -10` to see recent commit messages for style consistency -2. If you are on the `main` branch, create a new feature branch using `git branch` and switch to it. +2. Run the prose pass over the diff: Skill tool with skill="prose-pass". Apply its cuts before staging anything. **Do not skip this**, however small the diff. -3. Analyze all changes and draft a commit message: +3. Unless invoked with `force`, lint, build, and test the code (`lint-build-test`). This runs after the prose pass because that pass may rename an identifier or drop a comment lint requires. Stop if any checks fail. + +4. If you are on the `main` branch, create a new feature branch using `git branch` and switch to it. + +5. Analyze all changes and draft a commit message: - Summarize the nature of the changes (new feature, enhancement, bug fix, refactoring, test, docs, etc.) - Use the conventional commit format: `type(scope): description` - Keep the first line under 72 characters - Do not commit files that likely contain secrets (.env, credentials.json, etc.) -4. Stage and commit the changes: +6. Stage and commit the changes: - Add relevant files using `git add` - Use a plain string for the commit message (do not use HEREDOCs). -5. Report the results including: +7. Report the results including: - The commit hash - The commit message diff --git a/.claude/skills/pr/SKILL.md b/.claude/skills/pr/SKILL.md index 2f532a296f..1c424bab5e 100644 --- a/.claude/skills/pr/SKILL.md +++ b/.claude/skills/pr/SKILL.md @@ -25,9 +25,12 @@ When asked to create a pull request, follow these steps: 3. Run `git log main..HEAD --oneline` to see the commit history. 4. Get the diff for the review: + - If this is a **stacked PR**, run `git diff ...HEAD` to scope the diff to only this branch's changes. - Otherwise, run `git diff main...HEAD`. +5. Run the prose pass over the comments in that diff: Skill tool with skill="prose-pass". It runs here, before review, so that anything it changes is reviewed in Phase 2. If it changes files, run the checks (`lint-build-test`), commit with the message "docs: Trim comments", push, and re-run step 4 so the review sees the trimmed diff. + ## Phase 2: Automated PR review (parallel subagents) **MANDATORY — DO NOT SKIP.** This phase must run before any PR is created, regardless of how "simple", "mechanical", or "well-tested" the changes appear. Subtle bugs (e.g., semantic mismatches in API migrations) hide in exactly the changes that seem safe to skip. The only exception is docs-only changes as described below. @@ -109,7 +112,9 @@ Otherwise, after all launched subagents complete: ## Phase 4: Create the PR -1. Run `gh pr create` to create a pull request. The PR body should include: +1. Draft the PR body, then run the prose pass over the body: Skill tool with skill="prose-pass". The comments were already swept in Phase 1; this pass is for the body only. + +2. Run `gh pr create` to create a pull request. The PR body should include: - A brief narrative description of the PR - A summary of the changes (bullet points) @@ -117,7 +122,7 @@ Otherwise, after all launched subagents complete: **If this is a stacked PR**, add `--draft` to create it as a draft PR. -2. Note the PR number from the created PR URL — it is needed for changelog entries. Proceed to Phase 5 before presenting results to the user. +3. Note the PR number from the created PR URL — it is needed for changelog entries. Proceed to Phase 5 before presenting results to the user. ## Phase 5: Update changelogs @@ -132,6 +137,7 @@ Read the instructions in [`docs/contributing/updating-changelogs.md`](../../../d - **Combine like changes.** If multiple commits contribute to a single logical change within one package, write one changelog entry — not one per commit. - **Split disparate changes.** If one commit touches unrelated concerns in a single package, write separate entries. - **Link the PR.** Use the PR number from Phase 4 in each entry (e.g. `([#123](https://github.com/.../pull/123))`). +- **Keep entries short.** Run the prose pass over the drafted entries, for changelogs only: Skill tool with skill="prose-pass", args="changelogs". Commit the changelog updates to the current branch with the message "docs: Update changelogs" and push. diff --git a/.claude/skills/prose-pass/SKILL.md b/.claude/skills/prose-pass/SKILL.md new file mode 100644 index 0000000000..61f1aae857 --- /dev/null +++ b/.claude/skills/prose-pass/SKILL.md @@ -0,0 +1,178 @@ +--- +name: prose-pass +description: Judge added comments, JSDoc, changelog entries, and PR bodies on information content and cut what a reader can derive. Use before presenting a diff, committing, or writing a PR body. +--- + +A pass over the prose in a diff — comments, JSDoc, changelog entries, PR body — +deleting text a reader does not need. Scope is the diff: prose the change added +or touched, not every comment in the files it happens to open. + +Run it **before** presenting work, not after someone asks for it. When done, +return control to the calling workflow rather than stopping to present. + +Invoked with `args="changelogs"` (from `update-changelogs`, or the `pr` changelog +phase), run the Changelogs section only and skip the rest. + +## The bar + +**The desired default is no comment.** A comment must justify its existence; +accuracy is not a justification. + +The bar is **"obvious and logical"**, not "true and useful". A comment stating a +real hazard still goes if a competent reader derives it from the adjacent lines. +What survives is what the code cannot carry: a constraint, an invariant, a +non-obvious failure mode, or why a test is shaped the way it is — in the shortest +form that carries it. + +Every judgment here is about **information content, never about style**. No +phrasing is banned; nothing survives on phrasing either. + +Deleting a comment is cheap to undo — one line restored on request. A trivial +rename nearly so. Anything larger is not, and is out of scope for this pass; a +dropped JSDoc block can also break lint, so verify the result still lints. + +## Prefer a trivial rename to a comment + +If a comment exists only because an identifier is vague, and a **trivial rename** +removes the need for it, rename and delete the comment. Trivial means a local, a +parameter, or a private field that this diff already touches, renamed within the +one file — nothing exported, nothing that ripples across call sites. + +Stop there. **Do not introduce structural refactors to eliminate prose**, even +when the result would clearly read better: + +```ts +// Check whether the remote peer restarted. +if (currentId !== previousId) { +``` + +A `remotePeerRestarted()` helper would carry that sentence in its name, but +extracting it is a behavior-adjacent change with its own review surface. Keep the +comment, leave the structure alone, and propose the refactor separately if it is +worth doing. + +This pass runs before every commit and PR, so it has to be predictable: its diff +is comments removed, occasionally one identifier renamed — never a refactor +nobody asked for. + +## AI smell openers + +Comments beginning with phrases such as `Ensure`, `Handle`, `Now`, `First`, +`Then`, `We need to`, `This allows`, `This ensures`, `Note that`, or `Important` +often introduce a restatement of the next line. **Treat the opener as a signal to +inspect the comment, not as a deletion rule.** + +Delete it when the remaining sentence merely narrates the code. Keep it when the +sentence carries information the code cannot express: a constraint, an invariant, +a compatibility requirement, or a non-obvious failure mode. For example: + +```ts +// Note that N must stay a power of two. +``` + +stays if nothing in the type system or the surrounding code makes that constraint +apparent. + +When the verdict is delete, delete — rewriting a restatement into a shorter +restatement is not the fix. + +## Delete on sight + +- **Restatements of the next line.** If the identifier, the following statement, + or the `it(...)` title already says it, cut it. This includes a comment + describing a newly introduced variable immediately above its declaration. + (Function JSDoc is lint-required — see below.) +- **TDD scaffolding.** `// FAILING REPRO.` and the paragraph under it describe a + bug that no longer exists once the fix lands. +- **Per-test preambles** that restate the title or re-argue the fix. The test + title is the claim; the source comment is the reasoning. +- **Narration of test steps** when the mock's name already says it + (`// The second enqueue is the write that fails.`). + +## Comments above a log call + +A comment above a `logger.error`/`logger.warn` is usually the log message +restated, or a defense of logging rather than throwing. Both go. Keep it when it +carries what the log line cannot: + +```ts +// Expected during shutdown; warn so we don't page. +``` + +Same test as the openers — narration out, information in. Separately, an empty +`catch {}` may need an annotation because lint requires one; keep that minimal. + +## Tests + +Keep mock and setup mechanics that would otherwise baffle: which prepared +statement the failure targets, why a shared mock makes the expected call count 2, +why a cache is primed before the assertion. Shape rationale stays too — why the +test needs two kernels, why the assertion runs inside the crank. + +## Say it once + +State a rationale once, at the code it justifies — not in both a source comment +and its test, and not duplicated across sibling files. + +At the second site, **delete it and add nothing.** No cross-file pointer, no +"same reasoning as ``" — a reader who needs the argument finds it, and one +who doesn't pays for the line. A pointer is warranted only when the second site +is genuinely hard to understand without it; that is rare, and the burden is on +the pointer. + +## JSDoc + +This repo lints JSDoc: `jsdoc/require-jsdoc` is an error on functions, methods, +and classes, and `jsdoc/require-description` an error everywhere. Tests and some +JS and config globs are exempt — check `eslint.config.mjs` rather than assuming. +**Do not delete a required block or its description** — make the prose minimal +instead. Keep the `- ` before each +`@param` description, and the sentence casing and terminal period the `jsdoc/*` +rules require: + +```ts +/** + * Roll back the crank. + * + * @param err - The error the rollback threw, if it threw. + */ +``` + +Trim `@param` text to a bare noun phrase, not a clause about how the argument +gets used. + +## Changelogs + +State the change and its why once, then stop. See +[`docs/contributing/updating-changelogs.md`](../../../docs/contributing/updating-changelogs.md) +for format and categories; this section is only about length. + +Default to **one line per user-visible change**. + +Ask of every line: **Would a consumer notice this change, care about it, or need to act differently because of it?** +If not, cut it. That test is what rules out describing how the fix works +internally, which mechanism was chosen over which alternative, what was +verified, or a "known gap" / follow-up bullet. + +## PR bodies + +A PR body has a different reader — a reviewer, who does want the narrative and +how the change was tested. The `pr` skill requires both, so **the changelog test +above does not apply here.** Apply only "say it once": the description, the +change summary, and the testing note each make their point once, and none +repeats a rationale that is already in a code comment. + +## Checklist + +1. Judge every comment the diff adds or touches on information content. The + openers above are a hint about where to look, not the filter. +2. Delete any whose information is in the identifier, the next line, or the test + title. If it lives in another file, delete it here too — unless this site is + genuinely hard to follow without it. +3. For each survivor, ask whether a trivial rename removes the need for it. If + so, rename and delete the comment, then re-run lint. Do not restructure code + to remove a comment. +4. Re-read what is left asking **"is this obvious?"** — this is the pass that + gets skipped. +5. Apply the Changelogs and PR bodies sections to those artifacts. +6. Return control to the calling workflow. diff --git a/.claude/skills/update-changelogs/SKILL.md b/.claude/skills/update-changelogs/SKILL.md index 31ae5c7dce..56e8513ca3 100644 --- a/.claude/skills/update-changelogs/SKILL.md +++ b/.claude/skills/update-changelogs/SKILL.md @@ -6,5 +6,6 @@ description: Updates changelogs for all packages with consumer-facing changes. When asked to update changelogs, follow these steps: 1. Read the instructions in [`docs/contributing/updating-changelogs.md`](../../../docs/contributing/updating-changelogs.md) and follow them to the letter. -2. Commit the changes to the current branch with the commit message "docs: Update changelogs" before creating the PR. -3. Return the commit hash and the commit message. +2. Run the prose pass over the entries you drafted: Skill tool with skill="prose-pass", args="changelogs". Apply its cuts before committing. +3. Commit the changes to the current branch with the commit message "docs: Update changelogs" before creating the PR. +4. Return the commit hash and the commit message. diff --git a/AGENTS.md b/AGENTS.md index 22174d8908..3435b0b0f4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,6 +20,9 @@ Development workflows: General conventions: +- Comments, changelog entries, and PR bodies follow + [`.claude/skills/prose-pass/SKILL.md`](./.claude/skills/prose-pass/SKILL.md); apply it to + a diff before presenting it (in Claude Code, via the `prose-pass` skill) - Use `@metamask/superstruct` for runtime type checking and to define object types - kebab-case for package and file names (`@ocap/test-utils`, `kernel-worker.js`, `vat.js`) - If a function has more than two arguments or could be expected to grow thereto,