From d066e4bb211aa6178b3e26205c0b84d288b3b3d6 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 27 Aug 2026 13:54:03 +0200 Subject: [PATCH 1/4] docs(skills): add prose-pass skill for comment and changelog brevity Codifies the standard for comments, changelog entries, and PR bodies as a shared skill, and wires it into the commit, pr, and update-changelogs flows. --- .claude/settings.json | 1 + .claude/skills/commit/SKILL.md | 11 +- .claude/skills/pr/SKILL.md | 9 +- .claude/skills/prose-pass/SKILL.md | 125 ++++++++++++++++++++++ .claude/skills/update-changelogs/SKILL.md | 5 +- AGENTS.md | 2 + docs/contributing/updating-changelogs.md | 3 + 7 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 .claude/skills/prose-pass/SKILL.md 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/commit/SKILL.md b/.claude/skills/commit/SKILL.md index 002c090e3d..9881d0ad50 100644 --- a/.claude/skills/commit/SKILL.md +++ b/.claude/skills/commit/SKILL.md @@ -18,20 +18,23 @@ 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. If you are on the `main` branch, create a new feature branch using `git branch` and switch to it. + +4. 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: +5. 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: +6. 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..1f44b34092 100644 --- a/.claude/skills/pr/SKILL.md +++ b/.claude/skills/pr/SKILL.md @@ -109,7 +109,11 @@ 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 it and over the comments in + the diff: Skill tool with skill="prose-pass". Apply its cuts before creating + the PR; if it cuts comments, commit and push those changes first. + +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 +121,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 +136,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: Skill tool with skill="prose-pass". 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..61a4410359 --- /dev/null +++ b/.claude/skills/prose-pass/SKILL.md @@ -0,0 +1,125 @@ +--- +name: prose-pass +description: Trim comments, changelog entries, and PR bodies down to non-obvious "why". Run before presenting code, committing, or opening a PR. +--- + +A pass over the prose in a diff — code comments, JSDoc, changelog entries, PR +body — deleting text a reader does not need. + +Run it **before** presenting work, not after someone asks for it. + +## The bar + +**The desired default is no comment.** A comment must justify its existence; +accuracy is not a justification. Do not preserve prose merely because it is +true, and do not preserve it merely because you wrote it. + +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, a non-obvious failure +mode, or why a test is shaped oddly — in the shortest form that carries it. + +Every judgement here is about **information content, never about style**. No +phrasing is banned; nothing survives on phrasing either. + +The cost of cutting too much is one line restored on request. The cost of +cutting too little is paid by every future reader. + +## Prefer a code change to a comment + +If a comment is necessary only because the surrounding identifier or structure +is unclear, **fix the identifier or the structure and delete the comment.** A +comment explaining what a name means is a rename waiting to happen; a comment +labelling a stretch of a long function is an extracted function waiting to +happen. + +## 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: + +```js +// 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 covers a comment + describing a newly introduced variable immediately above its declaration, and + a comment restating the signature above a function. +- **Any comment above a `logger.error`/`logger.warn` call.** The log message + states the failure; a preamble explaining why it is logged rather than thrown + is padding. (Exception: an empty `catch {}` that lint forces you to annotate.) +- **TDD scaffolding.** `// FAILING REPRO.` and the paragraph under it describe a + bug that no longer exists once the fix lands. +- **Per-test preambles.** The test title is the claim; the source comment is the + reasoning. Do not re-argue the fix in the test file. +- **Narration of test steps** when the mock's name already says it + (`// The second enqueue is the write that fails.`). + +Keep in tests **only** 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. + +## 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 + +Keep the tags the repo's `jsdoc/*` lint rules require (`@param`, `@returns`) — +make the prose minimal rather than dropping the tag. Trim `@param` text to a +bare noun phrase (`The error the rollback threw, if it threw.`), not a clause +about how the argument is used. + +## Changelogs and PR bodies + +Same rule: state the change and its why once, then stop. See +[`docs/contributing/updating-changelogs.md`](../../../docs/contributing/updating-changelogs.md) +for the format; this section is about length. + +- Default to **one line per user-visible change**, naming renamed or added API, + and `**BREAKING:**` where it applies. +- A sub-bullet is for a consumer-facing detail that doesn't fit the line, not + for the story behind the change. +- Several entries describing one user-visible change collapse into one. + +Never put in a changelog: how the fix works internally, which mechanism was +chosen over which alternative, what was verified, or a "known gap" / follow-up +bullet. + +Ask of every line: **does a consumer do something differently because of it?** +If not, cut it. + +## Checklist + +1. Find the diff's added comments matching the openers above and judge each one + on information content — delete the narration, keep the constraints. +2. Re-read every remaining comment. Delete any whose information is in the + identifier, the next line, the test title, or another file. +3. For each survivor, ask whether a rename or an extracted function removes the + need for it. If so, make that change instead. +4. Re-read what is left asking **"is this obvious?"** — this pass is the one that + gets skipped, and it is where most of the cuts are. +5. Apply the changelog / PR-body test above to each entry. +6. Report what was cut only if asked; otherwise just present the trimmed work. diff --git a/.claude/skills/update-changelogs/SKILL.md b/.claude/skills/update-changelogs/SKILL.md index 31ae5c7dce..56998b78a8 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". 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..e307d09f7a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,6 +20,8 @@ Development workflows: General conventions: +- Comments, changelog entries, and PR bodies must state only the non-obvious "why"; + run the `prose-pass` skill over them before presenting, committing, or opening a PR - 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, diff --git a/docs/contributing/updating-changelogs.md b/docs/contributing/updating-changelogs.md index af85321e5d..b740aed7f5 100644 --- a/docs/contributing/updating-changelogs.md +++ b/docs/contributing/updating-changelogs.md @@ -27,3 +27,6 @@ be updated. This is enforced by CI. When updating changelogs, keep the following If your PR does not contain any consumer-facing changes, add the label `no-changelog`, and the changelog validation CI job will be skipped. + +For how long an entry should be, and the same standard applied to code comments and PR +bodies, see [`.claude/skills/prose-pass/SKILL.md`](../../.claude/skills/prose-pass/SKILL.md). From 2dbba7bedbe261369fb610855fe69c51507cca0a Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 27 Aug 2026 14:04:43 +0200 Subject: [PATCH 2/4] docs(skills): resolve prose-pass conflicts with lint and the pr skill Review found the skill instructing edits that break jsdoc/require-description, banning the test summary the pr skill requires in a PR body, and sweeping comments after the review subagents had already run. --- .claude/skills/code-review/SKILL.md | 2 + .claude/skills/commit/SKILL.md | 15 +-- .claude/skills/pr/SKILL.md | 9 +- .claude/skills/prose-pass/SKILL.md | 138 ++++++++++++++-------- .claude/skills/update-changelogs/SKILL.md | 2 +- AGENTS.md | 5 +- docs/contributing/updating-changelogs.md | 3 - 7 files changed, 107 insertions(+), 67 deletions(-) diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md index 83ad493799..e3a8ab7f13 100644 --- a/.claude/skills/code-review/SKILL.md +++ b/.claude/skills/code-review/SKILL.md @@ -12,6 +12,8 @@ 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. +Judge added comments against the prose standard by invoking the prose-pass +skill using: Skill tool with skill="prose-pass" 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 9881d0ad50..c458fa3d0f 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,23 +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. 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. +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. If you are on the `main` branch, create a new feature branch using `git branch` and switch to it. +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 identifiers or extract functions. Stop if any checks fail. -4. Analyze all changes and draft a commit message: +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.) -5. 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). -6. 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 1f44b34092..6cb48ab931 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, re-run the checks (`lint-build-test`), commit with the message "docs: Trim comments", and push before continuing. + ## 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,9 +112,7 @@ Otherwise, after all launched subagents complete: ## Phase 4: Create the PR -1. Draft the PR body, then run the prose pass over it and over the comments in - the diff: Skill tool with skill="prose-pass". Apply its cuts before creating - the PR; if it cuts comments, commit and push those changes first. +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: @@ -136,7 +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: Skill tool with skill="prose-pass". +- **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 index 61a4410359..e3a02e6bfd 100644 --- a/.claude/skills/prose-pass/SKILL.md +++ b/.claude/skills/prose-pass/SKILL.md @@ -1,36 +1,42 @@ --- name: prose-pass -description: Trim comments, changelog entries, and PR bodies down to non-obvious "why". Run before presenting code, committing, or opening a PR. +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 — code comments, JSDoc, changelog entries, PR -body — deleting text a reader does not need. +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. +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 for changelog entries alone (from `update-changelogs`, or the `pr` +changelog phase), run the Changelogs section only. ## The bar **The desired default is no comment.** A comment must justify its existence; -accuracy is not a justification. Do not preserve prose merely because it is -true, and do not preserve it merely because you wrote it. +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, a non-obvious failure -mode, or why a test is shaped oddly — in the shortest form that carries it. +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 judgement here is about **information content, never about style**. No +Every judgment here is about **information content, never about style**. No phrasing is banned; nothing survives on phrasing either. -The cost of cutting too much is one line restored on request. The cost of -cutting too little is paid by every future reader. +Deleting a comment is cheap to undo — one line restored on request. A rename, an +extracted function, or a dropped JSDoc block is not: hold those to the same bar, +but verify the result still lints and passes its tests. ## Prefer a code change to a comment If a comment is necessary only because the surrounding identifier or structure is unclear, **fix the identifier or the structure and delete the comment.** A comment explaining what a name means is a rename waiting to happen; a comment -labelling a stretch of a long function is an extracted function waiting to +labeling a stretch of a long function is an extracted function waiting to happen. ## AI smell openers @@ -44,7 +50,7 @@ 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: -```js +```ts // Note that N must stay a power of two. ``` @@ -57,22 +63,35 @@ 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 covers a comment - describing a newly introduced variable immediately above its declaration, and - a comment restating the signature above a function. -- **Any comment above a `logger.error`/`logger.warn` call.** The log message - states the failure; a preamble explaining why it is logged rather than thrown - is padding. (Exception: an empty `catch {}` that lint forces you to annotate.) + 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.** The test title is the claim; the source comment is the - reasoning. Do not re-argue the fix in the test file. +- **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.`). -Keep in tests **only** 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. +## 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 @@ -87,39 +106,58 @@ the pointer. ## JSDoc -Keep the tags the repo's `jsdoc/*` lint rules require (`@param`, `@returns`) — -make the prose minimal rather than dropping the tag. Trim `@param` text to a -bare noun phrase (`The error the rollback threw, if it threw.`), not a clause -about how the argument is used. +This repo lints JSDoc: `jsdoc/require-jsdoc` is an error on functions, methods, +and classes in source (off under `**/test/**` and `*.test.ts`), and +`jsdoc/require-description` an error everywhere. **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. + */ +``` -## Changelogs and PR bodies +Trim `@param` text to a bare noun phrase, not a clause about how the argument +gets used. -Same rule: state the change and its why once, then stop. See -[`docs/contributing/updating-changelogs.md`](../../../docs/contributing/updating-changelogs.md) -for the format; this section is about length. +## Changelogs -- Default to **one line per user-visible change**, naming renamed or added API, - and `**BREAKING:**` where it applies. -- A sub-bullet is for a consumer-facing detail that doesn't fit the line, not - for the story behind the change. -- Several entries describing one user-visible change collapse into one. +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. -Never put in a changelog: how the fix works internally, which mechanism was -chosen over which alternative, what was verified, or a "known gap" / follow-up -bullet. +Default to **one line per user-visible change**. A sub-bullet is for a +consumer-facing detail that doesn't fit the line, not for the story behind the +change. Ask of every line: **does a consumer do something differently because of it?** -If not, cut 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. Find the diff's added comments matching the openers above and judge each one - on information content — delete the narration, keep the constraints. -2. Re-read every remaining comment. Delete any whose information is in the - identifier, the next line, the test title, or another file. +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 rename or an extracted function removes the - need for it. If so, make that change instead. -4. Re-read what is left asking **"is this obvious?"** — this pass is the one that - gets skipped, and it is where most of the cuts are. -5. Apply the changelog / PR-body test above to each entry. -6. Report what was cut only if asked; otherwise just present the trimmed work. + need for it. If so, make that change instead, then re-run lint and the + affected tests. +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 56998b78a8..56e8513ca3 100644 --- a/.claude/skills/update-changelogs/SKILL.md +++ b/.claude/skills/update-changelogs/SKILL.md @@ -6,6 +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. Run the prose pass over the entries you drafted: Skill tool with skill="prose-pass". Apply its cuts before committing. +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 e307d09f7a..3435b0b0f4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,8 +20,9 @@ Development workflows: General conventions: -- Comments, changelog entries, and PR bodies must state only the non-obvious "why"; - run the `prose-pass` skill over them before presenting, committing, or opening a PR +- 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, diff --git a/docs/contributing/updating-changelogs.md b/docs/contributing/updating-changelogs.md index b740aed7f5..af85321e5d 100644 --- a/docs/contributing/updating-changelogs.md +++ b/docs/contributing/updating-changelogs.md @@ -27,6 +27,3 @@ be updated. This is enforced by CI. When updating changelogs, keep the following If your PR does not contain any consumer-facing changes, add the label `no-changelog`, and the changelog validation CI job will be skipped. - -For how long an entry should be, and the same standard applied to code comments and PR -bodies, see [`.claude/skills/prose-pass/SKILL.md`](../../.claude/skills/prose-pass/SKILL.md). From 0500aa28510ccf2e0ee13b3e1e9b35cf039bd294 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 27 Aug 2026 14:09:05 +0200 Subject: [PATCH 3/4] docs(skills): make code-review read the prose standard instead of running it prose-pass edits files and returns no findings, so a review of a remote PR had nothing to act on. --- .claude/skills/code-review/SKILL.md | 6 ++++-- .claude/skills/pr/SKILL.md | 2 +- .claude/skills/prose-pass/SKILL.md | 17 ++++++++--------- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md index e3a8ab7f13..f877bc98e8 100644 --- a/.claude/skills/code-review/SKILL.md +++ b/.claude/skills/code-review/SKILL.md @@ -12,8 +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. -Judge added comments against the prose standard by invoking the prose-pass -skill using: Skill tool with skill="prose-pass" +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/pr/SKILL.md b/.claude/skills/pr/SKILL.md index 6cb48ab931..1c424bab5e 100644 --- a/.claude/skills/pr/SKILL.md +++ b/.claude/skills/pr/SKILL.md @@ -29,7 +29,7 @@ When asked to create a pull request, follow these steps: - 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, re-run the checks (`lint-build-test`), commit with the message "docs: Trim comments", and push before continuing. +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) diff --git a/.claude/skills/prose-pass/SKILL.md b/.claude/skills/prose-pass/SKILL.md index e3a02e6bfd..53a83baaaa 100644 --- a/.claude/skills/prose-pass/SKILL.md +++ b/.claude/skills/prose-pass/SKILL.md @@ -10,8 +10,8 @@ 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 for changelog entries alone (from `update-changelogs`, or the `pr` -changelog phase), run the Changelogs section only. +Invoked with `args="changelogs"` (from `update-changelogs`, or the `pr` changelog +phase), run the Changelogs section only and skip the rest. ## The bar @@ -107,9 +107,10 @@ the pointer. ## JSDoc This repo lints JSDoc: `jsdoc/require-jsdoc` is an error on functions, methods, -and classes in source (off under `**/test/**` and `*.test.ts`), and -`jsdoc/require-description` an error everywhere. **Do not delete a required block -or its description** — make the prose minimal instead. Keep the `- ` before each +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: @@ -130,11 +131,9 @@ 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**. A sub-bullet is for a -consumer-facing detail that doesn't fit the line, not for the story behind the -change. +Default to **one line per user-visible change**. -Ask of every line: **does a consumer do something differently because of it?** +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. From 51ff46dd7543f6cf445838aa7827444cbdbf17b7 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Thu, 27 Aug 2026 14:11:25 +0200 Subject: [PATCH 4/4] docs(skills): limit prose-pass to trivial renames, no structural refactors The pass runs before every commit, so its diff must stay predictable: comments out, at most one identifier renamed. --- .claude/skills/commit/SKILL.md | 2 +- .claude/skills/prose-pass/SKILL.md | 40 +++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/.claude/skills/commit/SKILL.md b/.claude/skills/commit/SKILL.md index c458fa3d0f..de95657acd 100644 --- a/.claude/skills/commit/SKILL.md +++ b/.claude/skills/commit/SKILL.md @@ -20,7 +20,7 @@ When asked to commit code, follow these steps: 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. Unless invoked with `force`, lint, build, and test the code (`lint-build-test`). This runs after the prose pass because that pass may rename identifiers or extract functions. Stop if any checks fail. +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. diff --git a/.claude/skills/prose-pass/SKILL.md b/.claude/skills/prose-pass/SKILL.md index 53a83baaaa..61f1aae857 100644 --- a/.claude/skills/prose-pass/SKILL.md +++ b/.claude/skills/prose-pass/SKILL.md @@ -27,17 +27,33 @@ 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 rename, an -extracted function, or a dropped JSDoc block is not: hold those to the same bar, -but verify the result still lints and passes its tests. +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 code change to a comment +## Prefer a trivial rename to a comment -If a comment is necessary only because the surrounding identifier or structure -is unclear, **fix the identifier or the structure and delete the comment.** A -comment explaining what a name means is a rename waiting to happen; a comment -labeling a stretch of a long function is an extracted function waiting to -happen. +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 @@ -153,9 +169,9 @@ repeats a rationale that is already in a code comment. 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 rename or an extracted function removes the - need for it. If so, make that change instead, then re-run lint and the - affected tests. +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.