diff --git a/.claude/rules/comments-only-when-non-obvious.md b/.claude/rules/comments-only-when-non-obvious.md new file mode 100644 index 0000000..d57b9e6 --- /dev/null +++ b/.claude/rules/comments-only-when-non-obvious.md @@ -0,0 +1,87 @@ +--- +paths: + - '*.{ts,tsx,cts,mts,js,jsx,mjs,cjs,yml,yaml}' + - '**/*.{ts,tsx,cts,mts,js,jsx,mjs,cjs,yml,yaml}' +--- + +# Comment only when the code would mislead a competent reader + +Write self-documenting code: let well-named identifiers, narrow types, and small functions carry the meaning. Add a comment only when reading the code alone would make a competent reader mispredict its behaviour, and only to explain the _why_ the code itself cannot show. + +## When a comment earns its place + +Add one — kept to a single line where possible — for exactly these: + +- A non-obvious workaround: a specific bug, race, ordering hazard, or platform quirk the code is shaped around. +- A constraint imposed by an external system not visible in the code: an API contract, a wire format, a rate limit, a required call order. +- A deliberate deviation from the obvious approach, where the obvious approach is wrong for a reason a reader would not guess. + +The keep-it test: if a competent reader would predict the behaviour correctly without the comment, delete the comment. + +## What never to write + +These are the failure modes this rule exists to stop. They are the current default-model drift, and they poison codebases at scale. + +- **Prose blocks on trivial code.** A one-liner, an assignment, or a single call never gets a multi-line comment. A paragraph above one statement is the dominant failure. +- **Restatement.** A comment that re-says what the next line already says in code is noise, not signal. The code is the source of truth; a description of it rots and must then be re-verified by every reader. +- **Leaked session narration.** Never write the editing session's ephemeral reasoning into code or to disk: `Let me…`, `Now I'll…`, `This is a substantial change…`, `we need to…`, `as requested`. That belongs in the chat, never the file. +- **LLM tells.** Em-dashes and curly quotes (`—`, `“ ” ‘ ’`) in a comment are a strong sign it was generated narration rather than a deliberate note; the gate flags them. +- **Invented shorthand.** Do not coin a new term, abbreviation, or naming scheme inside a comment and leave it undefined; it snowballs and is never cleaned up. Use the codebase's existing vocabulary. +- **Duplicated explanation.** State a fact once, at its source of truth (the README, or the single function that owns it). A second site gets a one-line pointer, not a copy — the same explanation restated across many files is the same bug as restatement, spread out. + +## Examples + +Restatement and a prose block — delete it; the code already says this: + +```ts +// BAD +// Calculate the total by multiplying quantity by unit price, then store it so the +// invoice summary can use it later when it renders the line item. +const total = quantity * unitPrice + +// GOOD +const total = quantity * unitPrice +``` + +A justified comment — keep it; the constraint is external and invisible in the code: + +```ts +// GOOD: Stripe rejects sub-cent precision; round to integer cents before sending. +const amountCents = Math.round(price * 100) +``` + +Leaked session narration — strip it: + +```ts +// BAD +// Now let me add the retry logic that was requested. This is the substantial +// part of the change: wrap the call in a loop and back off exponentially. +for (let attempt = 0; attempt < maxRetries; attempt++) { ... } + +// GOOD +for (let attempt = 0; attempt < maxRetries; attempt++) { ... } +``` + +Config and infra are not exempt — the same restatement shows up in YAML: + +```yaml +# BAD: the key and its default already say this +# The log level for the service, defaulting to info. +logLevel: info + +# GOOD +logLevel: info +``` + +Duplicated explanation — say it once, point to it: + +```text +BAD: the full paragraph on how the retry budget is derived is copied into the + README, the client, and four call sites. +GOOD: the explanation lives once in the client; each call site carries + // retry budget: see resolveRetryBudget +``` + +## Enforcement + +This is gated, not merely advised. `verifyx comments` flags low-value comments — long blocks, session narration, and high comment density — over the current diff (or the whole codebase with `--scope all`). An edit-time PostToolUse hook (`verifyx comments-hook`) applies the same rules the moment a file is edited. The `prune-comments` skill strips and consolidates comments across a change on demand. diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000..3db1b90 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,15 @@ +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|Write|MultiEdit", + "hooks": [ + { + "type": "command", + "command": "npx verifyx comments-hook" + } + ] + } + ] + } +} diff --git a/.claude/skills/prune-comments/SKILL.md b/.claude/skills/prune-comments/SKILL.md new file mode 100644 index 0000000..706ee1d --- /dev/null +++ b/.claude/skills/prune-comments/SKILL.md @@ -0,0 +1,47 @@ +--- +name: prune-comments +description: Strip low-value comments (restatement, session narration, prose blocks, dense runs) from a change. Applies edits in-place, file by file. Use when asked to "prune comments", "clean up comments", "remove comment bloat", or after the verify comments check flags them. Apply-capable — it edits files, it is not a read-only review. +--- + +# prune-comments + +Remove comment bloat and keep only the comments that earn their place, following the criteria in the +`comments-only-when-non-obvious` rule (`.claude/rules/` / `.agent-skills/rules/`). Edits are applied in-place. + +## 1. Find the flagged comments + +Run the check to see what fails (the binary is `verifyx`). By default it judges the **current diff** — the +comments this change introduced: + +``` +npx verifyx comments --pushback +``` + +Scope options (`verifyx comments` handles the git/diff scoping itself — you don't need to compute it): + +- **Whole repo:** `npx verifyx comments --scope all --pushback` — audit and prune every comment in the codebase, + not just the diff. +- **Every comment, not just heuristic hits:** add `--block-all` (with either scope) to remove _every_ comment in + scope, not just the heuristic hits. + +## 2. Delete or keep + +Apply the `comments-only-when-non-obvious` rule to each flagged comment: + +- **Delete** restatement, session narration (`let me…`, `as requested`, `this function does…`), prose blocks on + trivial code, LLM tells (em-dash / curly quotes), and duplicated explanation. When in doubt, delete. +- **Keep** only a non-obvious _why_: a workaround, an external constraint, or a deliberate non-obvious deviation. + +Prefer fixing the code over keeping the comment: a better name, a smaller function, or a narrower type often +removes the need for the comment entirely. Do not try to slip a low-value comment past the gate — remove it, or +make the code self-explanatory. + +## 3. Apply and re-run + +Edit with the Edit tool — one edit per change site, not a whole-file rewrite. Then re-run the same +`verifyx comments` command and repeat until it passes. Never silence the check or widen its ignore globs. + +## Output + +One line per file: `pruned: — removed`, `clean: `, or `needs manual review: `. +No preamble, no closing remarks. diff --git a/README.md b/README.md index befd121..7a2afa9 100644 --- a/README.md +++ b/README.md @@ -114,18 +114,18 @@ Flags on the bare `verifyx` command: ## Built-in checks -| Check | Kind | What it catches | -| ------------------- | -------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `complexity` | native | Maintainability-index gate (cyclomatic complexity + Halstead volume + SLOC). Fails files below a threshold. | -| `comments` | native | Flags comment blocks taller than `--max-lines` (default 2), to push for self-documenting code. JSDoc and `context:`-prefixed blocks are always allowed. Add `--block-new-comments` to also fail any comment on a changed line (vs `HEAD` locally, the PR base in CI). | -| `hardcoded-colors` | native | Literal hex / `0x` colour values in source (cross-platform; suggests using design tokens). | -| `forbidden-strings` | native | Disallowed JSON config values, from rules in your verify config. | -| `lint` | external | Linting; auto-fixes locally, checks in CI ([oxlint](https://oxc.rs)). | -| `format` | external | Formatting; writes locally, checks in CI ([oxfmt](https://oxc.rs)). | -| `check-types` | external | TypeScript type check (`tsc --noEmit`); skips when there is no `tsconfig.json`. | -| `unused-code` | external | Unused files, exports and dependencies ([knip](https://knip.dev)). | -| `circular-deps` | external | Circular dependencies ([skott](https://github.com/antoine-coulon/skott)). | -| `duplicate-code` | external | Copy-paste detection ([jscpd](https://github.com/kucherenko/jscpd)). | +| Check | Kind | What it catches | +| ------------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `complexity` | native | Maintainability-index gate (cyclomatic complexity + Halstead volume + SLOC). Fails files below a threshold. | +| `comments` | native | Flags comment blocks taller than `--max-lines` (default 2), plus **session narration** and high **comment density** on changed lines (both on by default), to push for self-documenting code. JSDoc and `context:`-prefixed blocks are always allowed. Add `--block-new-comments` to fail _every_ comment on a changed line (vs `HEAD` locally, the PR base in CI). | +| `hardcoded-colors` | native | Literal hex / `0x` colour values in source (cross-platform; suggests using design tokens). | +| `forbidden-strings` | native | Disallowed JSON config values, from rules in your verify config. | +| `lint` | external | Linting; auto-fixes locally, checks in CI ([oxlint](https://oxc.rs)). | +| `format` | external | Formatting; writes locally, checks in CI ([oxfmt](https://oxc.rs)). | +| `check-types` | external | TypeScript type check (`tsc --noEmit`); skips when there is no `tsconfig.json`. | +| `unused-code` | external | Unused files, exports and dependencies ([knip](https://knip.dev)). | +| `circular-deps` | external | Circular dependencies ([skott](https://github.com/antoine-coulon/skott)). | +| `duplicate-code` | external | Copy-paste detection ([jscpd](https://github.com/kucherenko/jscpd)). | External checks shell out to their tool and **skip gracefully when it is not installed**; `verifyx init` installs the ones you opt into. They run the tool from your local `node_modules/.bin` regardless of how `verifyx` was invoked. `oxlint`/`oxfmt`/`tsc` are resolved if present; the rest are declared as optional `peerDependencies`. @@ -170,26 +170,70 @@ verifyx comments --max-lines 2 --pushback "src/**/*.ts" Capable coding models (Opus 4.8 very much included) love to narrate their work: multi-line comment blocks explaining _what_ the code does rather than _why_. They add little value, drift out of sync as the code changes (and a later reader, human or LLM, may trust the stale comment over the code), and quietly grow the surface area you have to maintain. By default this check pushes back on exactly that, flagging any comment block taller than `--max-lines` (default 2) so the pressure is on the code to document itself. -- `[pattern]`: glob, directory, or file to scan. +The check has two orthogonal axes. **Scope** decides which comments are candidates — `diff` (only comments on changed lines; the default) or `all` (every comment in the matched files). **Strictness** decides what fails within that scope — the heuristics below (default), or `--block-all`, which fails _every_ comment in scope. + +- `[pattern]`: glob, directory, or file to scan (used by `--scope all`). +- `--scope `: judge only changed lines (default) or the whole codebase. - `--max-lines `: fail on comment blocks longer than `n` lines (default 2). - `--pushback`: reframe the failure message as back-pressure aimed at an AI agent (see below). - `--warn`: report the long-block violations without failing the run. -- `--block-new-comments`: also fail on any comment on a changed line (vs `HEAD` locally, the PR base in CI; see below). -- `--ignore `: exclude files (repeatable; the `--block-new-comments` gate also reads `comments.ignore` from your [verify config](#configuration)). +- `--no-narration`: turn off the session-narration gate (on by default; see below). +- `--no-context-override`: stop treating `context:`-prefixed comments as exempt — a stricter stance, or to drive a cleanup of existing `context:` use. +- `--comment-density `: the comment share (0–1) that fails a file (default `0.3`; `0` disables). +- `--min-added-lines `: skip the density gate when fewer than `n` lines are in scope (default 10). +- `--block-all`: fail _every_ non-exempt comment in scope — the strictest gate; supersedes the heuristics. +- `--block-new-comments`: alias for `--scope diff --block-all` (fail every comment on a changed line). +- `--ignore `: exclude files (repeatable; also read from `comments.ignore` in your [verify config](#configuration)). + +Three heuristics run by default within the scope: + +- **Long blocks** — a run of comment lines (or a `/* … */` block) taller than `--max-lines`. +- **Narration** — comments that think out loud or restate _what_ the next line does (`// let me add the handler`, `// as requested`, `// this function returns the total`), plus LLM punctuation tells (em-dashes and curly quotes, which generated comments emit far more than hand-typed ones). +- **Density** — a file where comments make up too large a share of the lines in scope (default 30%, once at least 10 lines are in scope). A change that _removes_ at least as many comment lines as it adds is never flagged — a trim can't be adding bloat. -**`--pushback` is the clever bit.** An AI agent that hits a failing check will often take the path of least resistance and just delete or weaken the check to make the run pass. So rather than a dry error, the pushback message tells the agent that the _only_ sanctioned way to keep the comment is to prefix it with `context:`, and that doing so **pages a human to approve it**. Confronted with a real person's time on the line, the agent tends to reconsider and remove the low-value comment instead of gaming the gate. It is a small piece of prompt-engineering baked into a lint failure, and in practice it stops agents silencing the check far more reliably than a plain error does. +The default `diff` scope means the heuristics judge only what a change introduces, never legacy comments — so you can adopt the check on an existing repo without a cleanup first. Use `--scope all` for a full-codebase audit. All gates honour the same escape hatches (JSDoc, `context:`, machine directives); tune or disable them per repo via [verify config](#configuration). -If you want to go further and block **new** comments outright, override the `verify:comments` script and add `--block-new-comments`. That turns on a stricter, diff-based gate on top of the long-block check: any comment sitting on a changed line fails, whether you added or merely edited it. Machine directives (`eslint-disable`, `@ts-expect-error`, and friends) stay exempt, as does anything marked `context:`. +**`--pushback` gives stronger back-pressure.** An AI agent that hits a failing check will often take the path of least resistance and just delete or weaken the check to make the run pass. So rather than a dry error, the pushback message tells the agent that the _only_ sanctioned way to keep the comment is to prefix it with `context:`, and that doing so **pages a human to approve it**. Confronted with a real person's time on the line, the agent tends to reconsider more often and removes the low-value comment instead of gaming the gate. It is a small piece of prompt-engineering baked into a lint failure, and in practice it we found it stops agents silencing the check more reliably than a plain error does. -The "changed lines" are resolved per environment so the gate works the same locally and in CI. Locally it diffs the working tree against `HEAD` (your uncommitted changes). In CI (`CI` set) a clean checkout has nothing uncommitted, so it diffs against the **merge base with the PR base branch** instead, read from `GITHUB_BASE_REF` (GitHub Actions). Set `VERIFY_DIFF_BASE` to a ref to override the base explicitly; if no base can be resolved it falls back to `HEAD`. For CI, make sure the base branch is fetched (`actions/checkout` with `fetch-depth: 0`), or the merge base won't be found and the gate silently passes. +If you want to go further and block comments outright, add `--block-all`: every non-exempt comment in scope fails, not just the heuristic hits. Pair it with the scope you want — `--scope diff --block-all` (the `--block-new-comments` alias) fails any comment on a changed line, while `--scope all --block-all` is a zero-comment policy across the codebase. Machine directives (`eslint-disable`, `@ts-expect-error`, and friends) stay exempt, as does anything marked `context:`. -Two escape hatches keep genuinely useful comments alive. **JSDoc** (`/** … */`) is always allowed, and prefixing a comment's first line with `context:` marks it as durable context the code itself can't express: +Under the default `diff` scope, the "changed lines" are resolved per environment so the gate works the same locally and in CI. Locally it diffs the working tree against `HEAD` (your uncommitted changes). In CI (`CI` set) a clean checkout has nothing uncommitted, so it diffs against the **merge base with the PR base branch** instead, read from `GITHUB_BASE_REF` (GitHub Actions). Set `VERIFY_DIFF_BASE` to a ref to override the base explicitly; if no base can be resolved it falls back to `HEAD`. For CI, make sure the base branch is fetched (`actions/checkout` with `fetch-depth: 0`), or the merge base won't be found and the gate silently passes. + +Two escape hatches keep genuinely useful comments alive. **JSDoc** (`/** … */`) is always allowed, and prefixing a comment's first line with `context:` marks it as durable context the code itself can't express — the kind of constraint a reader would otherwise "fix" and break, where no rename or refactor would carry the _why_: ```ts -// context: the upstream API returns seconds, not milliseconds, so do not "fix" this -const timeoutMs = timeout * 1000 +// context: keep this sequential; the gateway 429s on concurrent calls, so Promise.all here breaks it. +for (const id of ids) await fetchOne(id) ``` +**Opting out of `context:`.** For a stricter stance — or to drive a cleanup of existing `context:` comments — pass `--no-context-override` (or set `comments.contextOverride: false` in your [verify config](#configuration)). `context:` comments are then judged like any other; JSDoc and machine directives stay exempt. Because `context:` no longer offers an escape, this also removes the one sanctioned way to keep a flagged comment (and the `--pushback` message drops its "prefix with `context:`" framing accordingly), so use it when you genuinely want the gate at its strictest. Combined with `--block-all`, this is a **no-comments-at-all, JSDoc-only** policy — `verifyx init` offers it directly as its strictest comment option (or `--comment-strict`), baking `--block-all --no-context-override` into the scaffolded `verify:comments` script. + +#### Edit-time hook (Claude PostToolUse) + +The `comments` check runs at verify/CI time — the end of a task. To close the loop tighter, `verifyx init` can also register an **edit-time gate** as a Claude Code [PostToolUse hook](https://code.claude.com/docs/en/hooks), so the same rules fire the moment an agent writes a file: + +```jsonc +// .claude/settings.json (added by `verifyx init`; opt out with --no-comment-hook) +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|Write|MultiEdit", + "hooks": [{ "type": "command", "command": "npx verifyx comments-hook" }], + }, + ], + }, +} +``` + +`verifyx comments-hook` reads the tool payload on stdin, scans only the text that edit introduced (no git needed), and on a long block, narration, or dense comments writes back-pressure to stderr and exits `2` — which Claude surfaces to the agent in the same turn, so it revises before the code ever reaches you. The hook is always diff-scoped (it only sees the edit), and Claude-specific (other agents have no universal edit-time equivalent); those rely on the verify/CI check and the `prune-comments` skill. + +> **Monorepos.** Claude Code loads `.claude/settings.json` only from the directory it is _launched_ from — it does not walk up or inherit. `verifyx init` writes the hook (and the skills, rules, and `CLAUDE.md` pointer) to the **nearest existing `.claude`** — your cwd if it has one, otherwise the closest ancestor within a few levels, never past the git root — and prints where it landed. Run `init` from (or point `--claude-dir` at) the directory you launch Claude from, so the hook actually fires. + +#### `prune-comments` skill + comment rule + +`verifyx init` also drops a **`prune-comments`** skill and a **`comments-only-when-non-obvious`** rule (`.claude/rules` + `.agent-skills/rules`) — the remediation companion to the gate, with explicit keep/delete criteria and worked examples. Ask an agent to "prune comments" and it runs `verifyx comments` over the diff, then applies the rule to clean up low-value comments. To sweep the **whole codebase** rather than just the current diff, it runs `verifyx comments --scope all`. + ### `hardcoded-colors` ```sh @@ -271,7 +315,15 @@ The **native** checks that take persistent settings read them from a `verify.con ```jsonc { "verify": { - "comments": { "ignore": ["**/*.generated.ts"] }, + "comments": { + "ignore": ["**/*.generated.ts"], + "scope": "diff", + "blockAll": false, + "narration": true, + "density": 0.3, + "minAddedLines": 10, + "contextOverride": true, + }, "hardcodedColors": { "root": "src", "ignore": ["**/tokens.ts"] }, "forbiddenStrings": [{ "file": "app.json", "paths": ["env.LOG_LEVEL"], "disallowed": "debug" }], }, @@ -312,7 +364,10 @@ Every export is available from the package root: import { analyzeComplexity, getCheck, runAll } from '@makerx/verify' // Run the maintainability analysis directly. -const { failing } = analyzeComplexity({ pattern: 'src/**/*.ts', threshold: 50 }) +const { failing } = analyzeComplexity({ + pattern: 'src/**/*.ts', + threshold: 50, +}) // Run any single check by name, including the ones that shell out to an external tool. const lint = await getCheck('lint')?.runDefault() diff --git a/src/checks/comment-collect.ts b/src/checks/comment-collect.ts new file mode 100644 index 0000000..c58336d --- /dev/null +++ b/src/checks/comment-collect.ts @@ -0,0 +1,83 @@ +import fs from 'node:fs' + +import { type CommentBlockViolation, findLongCommentBlocksInContent } from '../comments.ts' +import { scanComments } from '../shared/comment-scan.ts' +import { parseDiffAddedLines, parseDiffRemovedLines } from '../shared/diff.ts' +import { gitDiffAgainstBase } from '../shared/git.ts' +import { commentSpan, isDiffExempt, looksLikeCommentLine, type NewComment, shouldScan, toSingleLine } from './comment-common.ts' +import type { FileCommentCounts } from './comment-heuristics.ts' + +export type CommentCandidates = { + /** Long comment blocks in scope. */ + blocks: CommentBlockViolation[] + /** Non-exempt comments in scope (feeds narration + --block-all). */ + comments: NewComment[] + /** Per-file line counts for the density gate. */ + perFile: Map +} + +const isBlank = (line: string | undefined): boolean => (line ?? '').trim() === '' + +const blockIntersects = (block: CommentBlockViolation, changed: ReadonlySet): boolean => { + for (let l = block.line; l < block.line + block.lines; l++) if (changed.has(l)) return true + return false +} + +const sortComments = (comments: NewComment[]): NewComment[] => comments.sort((a, b) => a.file.localeCompare(b.file) || a.line - b.line) + +// scope `all` judges every comment in the matched files: the whole-codebase audit / prune surface. +export function gatherAllComments(files: readonly string[], maxLines: number, contextOverride = true): CommentCandidates { + const blocks: CommentBlockViolation[] = [] + const comments: NewComment[] = [] + const perFile = new Map() + for (const file of files) { + const content = fs.readFileSync(file, 'utf-8') + blocks.push(...findLongCommentBlocksInContent(file, content, maxLines, contextOverride)) + let commentLines = 0 + for (const c of scanComments(file, content)) { + if (isDiffExempt(c.text, contextOverride)) continue + commentLines += commentSpan(c.text) + comments.push({ file, line: c.line, text: toSingleLine(c.text) }) + } + const nonBlank = content.split('\n').filter((l) => !isBlank(l)).length + perFile.set(file, { added: nonBlank, commentLines, removedComments: 0 }) + } + return { blocks, comments: sortComments(comments), perFile } +} + +// scope `diff` judges only comments touching changed lines (vs the diff base); one pass feeds every gate. +export function gatherDiffComments(ignoreGlobs: readonly string[], maxLines: number, contextOverride = true): CommentCandidates { + const diff = gitDiffAgainstBase() + const added = parseDiffAddedLines(diff) + const removedByFile = parseDiffRemovedLines(diff) + const blocks: CommentBlockViolation[] = [] + const comments: NewComment[] = [] + const perFile = new Map() + for (const [file, changed] of added) { + if (!shouldScan(file, ignoreGlobs)) continue + const content = fs.readFileSync(file, 'utf-8') + const fileLines = content.split('\n') + for (const block of findLongCommentBlocksInContent(file, content, maxLines, contextOverride)) { + if (blockIntersects(block, changed)) blocks.push(block) + } + let commentLines = 0 + for (const c of scanComments(file, content)) { + if (isDiffExempt(c.text, contextOverride)) continue + const span = commentSpan(c.text) + // context: a comment is in scope when any of its lines changed, not only its opening line, so an edit to a + // context: block comment's body still surfaces it to the narration and block-all gates. + let touched = false + for (let l = c.line; l < c.line + span; l++) { + if (!changed.has(l)) continue + commentLines++ + touched = true + } + if (touched) comments.push({ file, line: c.line, text: toSingleLine(c.text) }) + } + let addedNonBlank = 0 + for (const l of changed) if (!isBlank(fileLines[l - 1])) addedNonBlank++ + const removedComments = (removedByFile.get(file) ?? []).filter(looksLikeCommentLine).length + perFile.set(file, { added: addedNonBlank, commentLines, removedComments }) + } + return { blocks, comments: sortComments(comments), perFile } +} diff --git a/src/checks/comment-common.ts b/src/checks/comment-common.ts new file mode 100644 index 0000000..c0f46eb --- /dev/null +++ b/src/checks/comment-common.ts @@ -0,0 +1,67 @@ +import fs from 'node:fs' + +import { minimatch } from 'minimatch' + +const SCANNED_EXTENSIONS = ['.ts', '.tsx', '.cts', '.mts', '.js', '.jsx', '.mjs', '.cjs', '.yml', '.yaml'] + +// machine directives steer tooling, not humans, so a changed line carrying one is never a "comment" +const MACHINE_DIRECTIVES = [ + 'oxlint-disable', + 'oxlint-enable', + '@ts-expect-error', + '@ts-ignore', + '@ts-nocheck', + 'eslint-disable', + 'eslint-enable', + 'prettier-ignore', + 'istanbul ignore', + 'v8 ignore', + 'c8 ignore', + '@vitest-environment', +] + +export type NewComment = { file: string; line: number; text: string } + +export function isCommentExempt(text: string, contextOverride = true): boolean { + const lower = text.toLowerCase() + if (MACHINE_DIRECTIVES.some((d) => lower.includes(d))) return true + if (!contextOverride) return false + const stripped = text.replace(/^\s*(?:\/\/+|\/\*+|\*|#)\s*/, '') + return stripped.toLowerCase().startsWith('context:') +} + +export function isScannedExtension(file: string): boolean { + return SCANNED_EXTENSIONS.some((ext) => file.endsWith(ext)) +} + +/** JSDoc (`/** … *\/`) is an always-allowed escape hatch. */ +function isJsDoc(text: string): boolean { + return text.trimStart().startsWith('/**') +} + +export function isDiffExempt(text: string, contextOverride = true): boolean { + return isCommentExempt(text, contextOverride) || isJsDoc(text) +} + +export function shouldScan(file: string, ignoreGlobs: readonly string[]): boolean { + if (!isScannedExtension(file)) return false + if (ignoreGlobs.some((glob) => minimatch(file, glob))) return false + return fs.existsSync(file) +} + +export function toSingleLine(text: string): string { + return text.replace(/\s+/g, ' ').trim() +} + +/** Physical lines a scanned comment occupies (block comments carry embedded newlines in their text). */ +export function commentSpan(text: string): number { + return text.split('\n').length +} + +// context: a line-level comment test for *removed* diff lines — we can't AST a line that no longer exists on +// disk, so the density net-increase guard falls back to prefix matching (mirrors the checker's is_comment_line). +export function looksLikeCommentLine(line: string): boolean { + const s = line.trimStart() + if (s.startsWith('#!')) return false + return s.startsWith('//') || s.startsWith('/*') || s.startsWith('*') || s.startsWith('#') || s.startsWith('--') +} diff --git a/src/checks/comment-heuristics.test.ts b/src/checks/comment-heuristics.test.ts new file mode 100644 index 0000000..cb6496f --- /dev/null +++ b/src/checks/comment-heuristics.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it } from 'vitest' + +import type { NewComment } from './comment-common.ts' +import { findCommentDensity, findNarrationComments } from './comment-heuristics.ts' + +function comment(text: string, line = 1): NewComment { + return { file: 'a.ts', line, text } +} + +describe('findNarrationComments', () => { + it('flags session-narration phrases', () => { + const flagged = [ + '// let me add the handler', + '// as requested, wire it up', + "// now I'll return the result", + '// this function returns the total', + '// First, validate the input', + '// added the retry because it was flaky', + ].map((t, i) => comment(t, i + 1)) + expect(findNarrationComments(flagged)).toHaveLength(flagged.length) + }) + + it('leaves genuine why-comments alone', () => { + const kept = [comment('// Stripe rejects amounts under 50c'), comment('// matches the wire format the gateway expects')] + expect(findNarrationComments(kept)).toEqual([]) + }) + + it('never flags exempt comments (context: / machine directives)', () => { + const exempt = [comment('// context: let me keep this durable note'), comment('// eslint-disable-next-line -- let me disable')] + expect(findNarrationComments(exempt)).toEqual([]) + }) + + it('flags LLM punctuation tells (em-dash, curly quotes)', () => { + const tells = [comment('// wraps the call — then retries'), comment('// uses the “fast” path')] + expect(findNarrationComments(tells)).toHaveLength(2) + }) + + it('with allowContext false, stops exempting context: comments (machine directives still exempt)', () => { + const comments = [comment('// context: let me keep this durable note'), comment('// eslint-disable-next-line -- let me disable')] + const flagged = findNarrationComments(comments, false) + expect(flagged).toHaveLength(1) + expect(flagged[0]?.text).toContain('context:') + }) +}) + +describe('findCommentDensity', () => { + const opts = { threshold: 0.3, minAddedLines: 10 } + const counts = ( + added: number, + commentLines: number, + removedComments = 0, + ): Map => + new Map([['a.ts', { added, commentLines, removedComments }]]) + + it('flags a file over the density threshold', () => { + const [v] = findCommentDensity(counts(10, 4), opts) + expect(v).toMatchObject({ file: 'a.ts', added: 10, commentLines: 4 }) + expect(v?.ratio).toBeCloseTo(0.4) + }) + + it('ignores tiny diffs below minAddedLines', () => { + expect(findCommentDensity(counts(9, 9), opts)).toEqual([]) + }) + + it('passes a file under the threshold', () => { + expect(findCommentDensity(counts(20, 5), opts)).toEqual([]) + }) + + it('skips a net comment trim (removed >= added comments)', () => { + expect(findCommentDensity(counts(10, 4, 4), opts)).toEqual([]) + }) +}) diff --git a/src/checks/comment-heuristics.ts b/src/checks/comment-heuristics.ts new file mode 100644 index 0000000..7d77caa --- /dev/null +++ b/src/checks/comment-heuristics.ts @@ -0,0 +1,65 @@ +import { isCommentExempt, type NewComment } from './comment-common.ts' + +// context: session narration — comments that describe the editing act or restate *what* the next line does, +// rather than durable *why*. These are the highest-signal tell that an agent is thinking out loud in comments. +const NARRATION_PATTERNS: RegExp[] = [ + /\blet me\b/i, + /\bas requested\b/i, + /\bas (?:you |we )?(?:asked|mentioned|discussed)\b/i, + /\bnow (?:i(?:'| a| wi)ll|i'm|we(?:'ll| will)|let(?:'s| us))\b/i, + /\bhere(?:'s| is| we)\b/i, + /\bfirst,|\bnext,|\bthen,|\bfinally,/i, + /\bthis (?:function|method|code|line|block|file|class|helper) (?:does|will|is|handles|returns|creates|adds|sets|gets)\b/i, + /\bthe following\b/i, + /\b(?:added|adding|updated|updating|changed|changing|removed|removing|refactored|fixed) (?:the|this|a|to|so|because|for)\b/i, + /\b(?:step \d|todo:|fixme:)/i, +] + +// context: em-dash and curly quotes are high-precision LLM tells — models emit them freely. +const LLM_PUNCT_TELL = /[—‘’“”]/ + +export type DensityViolation = { + file: string + /** Added lines in the changed file. */ + added: number + /** Added lines that fall inside a non-exempt comment. */ + commentLines: number + /** commentLines / added, 0–1. */ + ratio: number +} + +/** From comments in scope, return those whose text reads as session narration or carries an LLM punctuation tell. */ +export function findNarrationComments(comments: readonly NewComment[], contextOverride = true): NewComment[] { + return comments.filter( + (c) => !isCommentExempt(c.text, contextOverride) && (NARRATION_PATTERNS.some((re) => re.test(c.text)) || LLM_PUNCT_TELL.test(c.text)), + ) +} + +export type FileCommentCounts = { + /** Non-blank lines in scope (added non-blank lines for diff scope; non-blank file lines for `all`). */ + added: number + /** Lines in scope that fall inside a non-exempt comment. */ + commentLines: number + /** Comment lines the change removed — the density gate skips a net comment *trim* (diff scope only; 0 for `all`). */ + removedComments: number +} + +/** + * Flag files where non-exempt comments make up too large a share of the lines in scope. Files with fewer than + * `minAddedLines` lines are skipped (tiny diffs never trip the gate), as is any change that removes at least as + * many comment lines as it adds — a net trim cannot be adding bloat. + */ +export function findCommentDensity( + perFile: ReadonlyMap, + opts: { threshold: number; minAddedLines: number }, +): DensityViolation[] { + const out: DensityViolation[] = [] + for (const [file, { added, commentLines, removedComments }] of perFile) { + if (added < opts.minAddedLines) continue + if (commentLines <= removedComments) continue + const ratio = added === 0 ? 0 : commentLines / added + if (ratio >= opts.threshold) out.push({ file, added, commentLines, ratio }) + } + out.sort((a, b) => b.ratio - a.ratio || a.file.localeCompare(b.file)) + return out +} diff --git a/src/checks/comments.test.ts b/src/checks/comments.test.ts index b05ab6d..70bd041 100644 --- a/src/checks/comments.test.ts +++ b/src/checks/comments.test.ts @@ -4,12 +4,19 @@ import path from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { gitDiffAgainstBase } from '../shared/git.ts' import { runComments } from './comments.ts' +vi.mock('../shared/git.ts', () => ({ gitDiffAgainstBase: vi.fn(() => '') })) +const mockDiff = (diff: string): void => { + vi.mocked(gitDiffAgainstBase).mockReturnValue(diff) +} + let dir: string beforeEach(() => { dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'verify-comments-'))) + mockDiff('') vi.spyOn(console, 'log').mockImplementation(() => {}) vi.spyOn(console, 'warn').mockImplementation(() => {}) vi.spyOn(console, 'error').mockImplementation(() => {}) @@ -26,24 +33,84 @@ function write(name: string, content: string): string { return file.replaceAll('\\', '/') } -describe('runComments', () => { +/** A unified diff that marks every line of `content` as freshly added in `file`. */ +function diffFor(file: string, content: string): string { + const lines = content.split('\n') + const body = lines.map((l) => `+${l}`).join('\n') + return `--- /dev/null\n+++ b/${file}\n@@ -0,0 +1,${lines.length} @@\n${body}\n` +} + +describe('runComments — comment blocks (scope: all)', () => { it('passes and is named `comments` when no block exceeds max-lines', () => { const file = write('ok.ts', ['// one line', 'const a = 1'].join('\n')) - expect(runComments({ pattern: file, maxLines: 2 })).toEqual({ name: 'comments', ok: true }) + expect(runComments({ pattern: file, maxLines: 2, scope: 'all' })).toEqual({ name: 'comments', ok: true }) }) it('fails on a comment block longer than max-lines', () => { const file = write('bad.ts', ['// one', '// two', '// three', 'const a = 1'].join('\n')) - expect(runComments({ pattern: file, maxLines: 2 })).toEqual({ name: 'comments', ok: false }) + expect(runComments({ pattern: file, maxLines: 2, scope: 'all' })).toEqual({ name: 'comments', ok: false }) }) it('reports without failing under warn', () => { const file = write('warn.ts', ['// one', '// two', '// three', 'const a = 1'].join('\n')) - expect(runComments({ pattern: file, maxLines: 2, warn: true }).ok).toBe(true) + expect(runComments({ pattern: file, maxLines: 2, warn: true, scope: 'all' }).ok).toBe(true) }) it('leaves JSDoc and context: blocks alone regardless of length', () => { const file = write('exempt.ts', ['/**', ' * a', ' * b', ' * c', ' */', '// context: durable', '// more', 'const a = 1'].join('\n')) - expect(runComments({ pattern: file, maxLines: 1 }).ok).toBe(true) + expect(runComments({ pattern: file, maxLines: 1, scope: 'all' }).ok).toBe(true) + }) + + it('does not flag a whole-file block under the default diff scope with an empty diff', () => { + const file = write('diffscope.ts', ['// one', '// two', '// three', 'const a = 1'].join('\n')) + expect(runComments({ pattern: file, maxLines: 2 }).ok).toBe(true) + }) +}) + +describe('runComments — narration (default on)', () => { + it('fails on a narration comment on a changed line', () => { + const content = ['const a = 1', '// let me add the handler', 'const b = 2'].join('\n') + const file = write('narr.ts', content) + mockDiff(diffFor(file, content)) + expect(runComments({ pattern: file, maxLines: 2 }).ok).toBe(false) + }) + + it('passes when --no-narration disables it', () => { + const content = ['const a = 1', '// let me add the handler', 'const b = 2'].join('\n') + const file = write('narr2.ts', content) + mockDiff(diffFor(file, content)) + expect(runComments({ pattern: file, maxLines: 2, narration: false, density: false }).ok).toBe(true) + }) +}) + +describe('runComments — density (default on)', () => { + it('fails a comment-dense changed file', () => { + const content = Array.from({ length: 12 }, (_, i) => (i % 2 === 0 ? `// note ${i}` : `const v${i} = ${i}`)).join('\n') + const file = write('dense.ts', content) + mockDiff(diffFor(file, content)) + expect(runComments({ pattern: file, maxLines: 2, narration: false }).ok).toBe(false) + }) + + it('honours a disabled density threshold', () => { + const content = Array.from({ length: 12 }, (_, i) => (i % 2 === 0 ? `// note ${i}` : `const v${i} = ${i}`)).join('\n') + const file = write('dense2.ts', content) + mockDiff(diffFor(file, content)) + expect(runComments({ pattern: file, maxLines: 2, narration: false, density: false }).ok).toBe(true) + }) +}) + +describe('runComments — block-new-comments (opt-in)', () => { + it('fails on any non-exempt comment on a changed line', () => { + const content = ['const a = 1', '// a plain comment', 'const b = 2'].join('\n') + const file = write('bn.ts', content) + mockDiff(diffFor(file, content)) + expect(runComments({ pattern: file, maxLines: 2, blockNewComments: true }).ok).toBe(false) + }) + + it('passes when the diff has no comments', () => { + const content = ['const a = 1', 'const b = 2'].join('\n') + const file = write('bn2.ts', content) + mockDiff(diffFor(file, content)) + expect(runComments({ pattern: file, maxLines: 2, blockNewComments: true }).ok).toBe(true) }) }) diff --git a/src/checks/comments.ts b/src/checks/comments.ts index 501d6b8..c7d39a8 100644 --- a/src/checks/comments.ts +++ b/src/checks/comments.ts @@ -1,81 +1,20 @@ -import fs from 'node:fs' - -import { minimatch } from 'minimatch' - import { DEFAULT_IGNORE, DEFAULT_PATTERN, findSourceFiles, resolvePattern } from '../analyze.ts' -import { findLongCommentBlocks } from '../comments.ts' -import { printCommentBlockReport } from '../report.ts' +import { + type CommentScope, + printBlockAllReport, + printCommentBlockReport, + printCommentDensityReport, + printNarrationReport, +} from '../report.ts' import { color } from '../shared/color.ts' -import { scanFileComments } from '../shared/comment-scan.ts' import { loadVerifyConfig } from '../shared/config.ts' -import { parseDiffAddedLines } from '../shared/diff.ts' -import { gitDiffAgainstBase } from '../shared/git.ts' +import { type CommentCandidates, gatherAllComments, gatherDiffComments } from './comment-collect.ts' +import { findCommentDensity, findNarrationComments } from './comment-heuristics.ts' import type { CheckResult } from './types.ts' const DEFAULT_MAX_COMMENT_BLOCK_LINES = 2 - -const SCANNED_EXTENSIONS = ['.ts', '.tsx', '.cts', '.mts', '.js', '.jsx', '.mjs', '.cjs', '.yml', '.yaml'] - -// context: machine directives steer tooling, not humans, so a changed line carrying one is never a "comment" -// worth removing; `context:` is this project's durable-context escape hatch and is likewise exempt. -const MACHINE_DIRECTIVES = [ - 'oxlint-disable', - 'oxlint-enable', - '@ts-expect-error', - '@ts-ignore', - '@ts-nocheck', - 'eslint-disable', - 'eslint-enable', - 'prettier-ignore', - 'istanbul ignore', - 'v8 ignore', - 'c8 ignore', - '@vitest-environment', -] - -function isCommentExempt(text: string): boolean { - const lower = text.toLowerCase() - if (MACHINE_DIRECTIVES.some((d) => lower.includes(d))) return true - const stripped = text.replace(/^\s*(?:\/\/+|\/\*+|\*|#)\s*/, '') - return stripped.toLowerCase().startsWith('context:') -} - -function shouldScan(file: string, ignoreGlobs: readonly string[]): boolean { - if (!SCANNED_EXTENSIONS.some((ext) => file.endsWith(ext))) return false - if (ignoreGlobs.some((glob) => minimatch(file, glob))) return false - return fs.existsSync(file) -} - -function toSingleLine(text: string): string { - return text.replace(/\s+/g, ' ').trim() -} - -type NewComment = { file: string; line: number; text: string } - -// context: the --block-new-comments behaviour — flag any comment sitting on a changed line (vs HEAD, or the CI base). -function findCommentsOnChangedLines(ignoreGlobs: readonly string[]): NewComment[] { - const added = parseDiffAddedLines(gitDiffAgainstBase()) - const findings: NewComment[] = [] - for (const [file, lines] of added) { - if (!shouldScan(file, ignoreGlobs)) continue - for (const comment of scanFileComments(file)) { - if (!lines.has(comment.line)) continue - if (isCommentExempt(comment.text)) continue - findings.push({ file, line: comment.line, text: toSingleLine(comment.text) }) - } - } - findings.sort((a, b) => a.file.localeCompare(b.file) || a.line - b.line) - return findings -} - -function reportCommentsOnChangedLines(findings: readonly NewComment[]): void { - console.error(color.red('\nComments on changed lines:\n')) - for (const { file, line, text } of findings) console.error(` ${file}:${line} → ${text}`) - console.error(color.red(`\nTotal: ${findings.length} comment(s)`)) - console.error( - '\nWith --block-new-comments, every comment on a changed line fails this gate, whether you added or edited it. Remove them and let the code document itself.', - ) -} +const DEFAULT_DENSITY_THRESHOLD = 0.3 +const DEFAULT_MIN_ADDED_LINES = 10 export type CommentsOptions = { pattern?: string @@ -83,39 +22,117 @@ export type CommentsOptions = { maxLines?: number pushback?: boolean warn?: boolean - /** Also fail on any comment on a line changed against HEAD (machine directives / context: exempt). */ + /** Which comments the gates judge. Default `diff` (changed lines); `all` scans whole files. */ + scope?: CommentScope + /** Fail every non-exempt comment in scope, not just heuristic hits. */ + blockAll?: boolean + /** Back-compat alias for `scope: 'diff'` + `blockAll: true`. */ blockNewComments?: boolean + /** Flag session-narration comments. Default true. */ + narration?: boolean + /** Comment-density ratio (0–1) that fails a file; `false`/`0` disables. Default 0.3. */ + density?: number | false + /** Minimum lines in scope before density applies. Default 10. */ + minAddedLines?: number + /** Treat `context:`-prefixed comments as exempt. Default true; set false for a stricter gate / cleanup. */ + contextOverride?: boolean +} + +function resolveDensity(opt: number | false | undefined, cfg: number | false | undefined): number { + const value = opt ?? cfg ?? DEFAULT_DENSITY_THRESHOLD + return value === false ? 0 : value +} + +const whereText = (scope: CommentScope): string => (scope === 'all' ? 'in the codebase' : 'on changed lines') + +function gather( + opts: CommentsOptions, + scope: CommentScope, + ignoreGlobs: readonly string[], + maxLines: number, + contextOverride: boolean, +): CommentCandidates { + if (scope === 'all') { + const files = findSourceFiles(resolvePattern(opts.pattern ?? DEFAULT_PATTERN), [...DEFAULT_IGNORE, ...ignoreGlobs]) + return gatherAllComments(files, maxLines, contextOverride) + } + return gatherDiffComments(ignoreGlobs, maxLines, contextOverride) } /** - * Native check: flag comment blocks longer than `maxLines` (JSDoc and `context:`-prefixed blocks exempt). - * With `blockNewComments`, additionally fail on any comment on a line changed against HEAD. + * Native check for low-value comments across two orthogonal axes: **scope** (`diff` — only comments on changed + * lines, the default; or `all` — every comment in the matched files) and **strictness** (heuristics by default — + * long blocks, session narration, comment density; or `blockAll`, which fails every comment in scope). JSDoc, + * `context:` blocks, and machine directives are always exempt. */ export function runComments(opts: CommentsOptions = {}): CheckResult { + const cfg = loadVerifyConfig().comments ?? {} const maxLines = opts.maxLines ?? DEFAULT_MAX_COMMENT_BLOCK_LINES - const pattern = opts.pattern ?? DEFAULT_PATTERN - const files = findSourceFiles(resolvePattern(pattern), [...DEFAULT_IGNORE, ...(opts.ignore ?? [])]) + const ignoreGlobs = opts.ignore?.length ? opts.ignore : (cfg.ignore ?? []) + const scope: CommentScope = opts.scope ?? (opts.blockNewComments ? 'diff' : undefined) ?? cfg.scope ?? 'diff' + const blockAll = opts.blockAll ?? opts.blockNewComments ?? cfg.blockAll ?? false + const narrationOn = !blockAll && (opts.narration ?? cfg.narration ?? true) + const densityThreshold = blockAll ? 0 : resolveDensity(opts.density, cfg.density) + const minAddedLines = opts.minAddedLines ?? cfg.minAddedLines ?? DEFAULT_MIN_ADDED_LINES + const contextOverride = opts.contextOverride ?? cfg.contextOverride ?? true + const pushback = !!opts.pushback + + const candidates = gather(opts, scope, ignoreGlobs, maxLines, contextOverride) + + if (blockAll) { + if (candidates.comments.length === 0) { + console.log(color.green(`No comments ${whereText(scope)}.`)) + return { name: 'comments', ok: true } + } + printBlockAllReport(candidates.comments, { scope, pushback, contextOverride }) + return { name: 'comments', ok: false } + } + + let ok = true + ok = reportBlocks(candidates, maxLines, { pushback, warn: !!opts.warn, contextOverride }) && ok + if (narrationOn) ok = reportNarration(candidates, scope, pushback, contextOverride) && ok + if (densityThreshold > 0) + ok = reportDensity(candidates, { scope, pushback, contextOverride, threshold: densityThreshold, minAddedLines }) && ok + return { name: 'comments', ok } +} - const blocks = findLongCommentBlocks(files, maxLines) - let blocksOk = true - if (blocks.length === 0) { +function reportBlocks( + candidates: CommentCandidates, + maxLines: number, + opts: { pushback: boolean; warn: boolean; contextOverride: boolean }, +): boolean { + if (candidates.blocks.length === 0) { console.log(color.green(`No comment block over ${maxLines} line(s)`)) - } else { - printCommentBlockReport(blocks, maxLines, { pushback: !!opts.pushback, warn: !!opts.warn }) - blocksOk = !!opts.warn + return true } + printCommentBlockReport(candidates.blocks, maxLines, opts) + return opts.warn +} - let changedLinesOk = true - if (opts.blockNewComments) { - const ignoreGlobs = opts.ignore?.length ? opts.ignore : (loadVerifyConfig().comments?.ignore ?? []) - const findings = findCommentsOnChangedLines(ignoreGlobs) - if (findings.length === 0) { - console.log(color.green('No comments on changed lines.')) - } else { - reportCommentsOnChangedLines(findings) - changedLinesOk = false - } +function reportNarration(candidates: CommentCandidates, scope: CommentScope, pushback: boolean, contextOverride: boolean): boolean { + const narration = findNarrationComments(candidates.comments, contextOverride) + if (narration.length === 0) { + console.log(color.green(`No narration comments ${whereText(scope)}.`)) + return true } + printNarrationReport(narration, { scope, pushback, contextOverride }) + return false +} - return { name: 'comments', ok: blocksOk && changedLinesOk } +function reportDensity( + candidates: CommentCandidates, + opts: { scope: CommentScope; pushback: boolean; contextOverride: boolean; threshold: number; minAddedLines: number }, +): boolean { + const dense = findCommentDensity(candidates.perFile, { threshold: opts.threshold, minAddedLines: opts.minAddedLines }) + if (dense.length === 0) { + console.log(color.green('Comment density within threshold.')) + return true + } + printCommentDensityReport(dense, { + threshold: opts.threshold, + scope: opts.scope, + pushback: opts.pushback, + contextOverride: opts.contextOverride, + }) + return false } diff --git a/src/checks/registry.ts b/src/checks/registry.ts index b1638c0..63b9386 100644 --- a/src/checks/registry.ts +++ b/src/checks/registry.ts @@ -20,12 +20,12 @@ function nativeCheck(name: string, description: string, recommended: boolean, ru } } -// context: checks are named for their function, never the tool behind them (see each check's bin/devDeps). +// Checks are named for their function, never the tool behind them (see each check's bin/devDeps). export const CHECKS: Check[] = [ nativeCheck('complexity', 'Maintainability-index gate (cyclomatic + Halstead + SLOC)', true, () => runComplexity()), nativeCheck( 'comments', - 'Flag long comment blocks (JSDoc / context: exempt); --block-new-comments also fails comments on changed lines', + 'Flag low-value comments — long blocks + narration/density; --scope diff|all, --block-all for a zero-comment gate', true, () => runComments({ pushback: true }), 'verifyx comments --pushback', diff --git a/src/cli.ts b/src/cli.ts index 23b4a90..5a1a4c7 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -4,6 +4,7 @@ import { createRequire } from 'node:module' import { Command } from 'commander' import { registerChecks } from './commands/registerChecks.ts' +import { registerCommentsHook } from './commands/registerCommentsHook.ts' import { registerEject } from './commands/registerEject.ts' import { registerInit } from './commands/registerInit.ts' import { registerList } from './commands/registerList.ts' @@ -50,6 +51,7 @@ withRunOptions( }) registerChecks(program) +registerCommentsHook(program) registerList(program) registerInit(program) registerUpgradeDocs(program) diff --git a/src/commands/registerChecks.ts b/src/commands/registerChecks.ts index c1b1dbe..496e820 100644 --- a/src/commands/registerChecks.ts +++ b/src/commands/registerChecks.ts @@ -28,17 +28,37 @@ export function registerChecks(program: Command): void { program .command('comments') - .description('Flag long comment blocks (JSDoc / context: exempt); --block-new-comments also fails comments on changed lines') - .argument('[pattern]', 'glob, directory, or file to scan') + .description( + 'Flag low-value comments: long blocks + narration/density (JSDoc / context: exempt). --scope diff|all sets whether it judges changed lines or the whole codebase; --block-all fails every comment in scope', + ) + .argument('[pattern]', 'glob, directory, or file to scan (used by --scope all)') .option('--max-lines ', 'maximum comment-block length', Number) .option('--pushback', 'add AI back-pressure framing to the failure message') .option('--warn', 'report without failing the run') - .option('--block-new-comments', 'also fail on any comment on a line changed against HEAD') + .option('--scope ', 'which comments to judge: diff (changed lines) or all (whole codebase)') + .option('--block-all', 'fail every non-exempt comment in scope, not just heuristic hits') + .option('--block-new-comments', 'alias for --scope diff --block-all') + .option('--no-narration', 'do not flag session-narration comments') + .option('--no-context-override', 'disable the `context:` override, so those comments are no longer exempt (stricter gate / cleanup)') + .option('--comment-density ', 'comment-density ratio (0–1) that fails a file; 0 disables', Number) + .option('--min-added-lines ', 'minimum lines in scope before density applies', Number) .option('--ignore ', 'ignore glob (repeatable)', collect, []) .action( ( pattern: string | undefined, - opts: { maxLines?: number; pushback?: boolean; warn?: boolean; blockNewComments?: boolean; ignore: string[] }, + opts: { + maxLines?: number + pushback?: boolean + warn?: boolean + scope?: string + blockAll?: boolean + blockNewComments?: boolean + narration?: boolean + contextOverride?: boolean + commentDensity?: number + minAddedLines?: number + ignore: string[] + }, ) => { finish( runComments({ @@ -46,7 +66,13 @@ export function registerChecks(program: Command): void { maxLines: opts.maxLines, pushback: opts.pushback, warn: opts.warn, + scope: opts.scope === 'all' ? 'all' : opts.scope === 'diff' ? 'diff' : undefined, + blockAll: opts.blockAll, blockNewComments: opts.blockNewComments, + narration: opts.narration === false ? false : undefined, + contextOverride: opts.contextOverride === false ? false : undefined, + density: opts.commentDensity, + minAddedLines: opts.minAddedLines, ignore: opts.ignore, }).ok, ) diff --git a/src/commands/registerCommentsHook.ts b/src/commands/registerCommentsHook.ts new file mode 100644 index 0000000..44825de --- /dev/null +++ b/src/commands/registerCommentsHook.ts @@ -0,0 +1,65 @@ +import path from 'node:path' + +import type { Command } from 'commander' +import { minimatch } from 'minimatch' + +import { analyzeAddedComments, formatHookFeedback, hasFindings, type HookOptions } from '../hook/analyze.ts' +import { parsePostToolUsePayload } from '../hook/payload.ts' +import { loadVerifyConfig } from '../shared/config.ts' + +const DEFAULT_MAX_LINES = 2 +const DEFAULT_DENSITY = 0.3 +const DEFAULT_MIN_ADDED_LINES = 10 + +async function readStdin(): Promise { + const chunks: Buffer[] = [] + for await (const chunk of process.stdin) chunks.push(chunk as Buffer) + return Buffer.concat(chunks).toString('utf-8') +} + +// context: Claude's payload path is absolute and may use Windows separators; match ignore globs (e.g. +// **/*.generated.ts) against both the forward-slashed absolute path and a cwd-relative form, as the check does. +function isIgnored(file: string, globs: readonly string[]): boolean { + const abs = file.replaceAll('\\', '/') + const rel = path.relative(process.cwd(), file).replaceAll('\\', '/') + return globs.some((glob) => minimatch(abs, glob) || minimatch(rel, glob)) +} + +function resolveOptions(): { options: HookOptions; ignore: string[] } { + const cfg = loadVerifyConfig().comments ?? {} + const density = cfg.density === false ? 0 : (cfg.density ?? DEFAULT_DENSITY) + return { + options: { + maxLines: DEFAULT_MAX_LINES, + narration: cfg.narration ?? true, + density, + minAddedLines: cfg.minAddedLines ?? DEFAULT_MIN_ADDED_LINES, + blockAll: cfg.blockAll ?? false, + contextOverride: cfg.contextOverride ?? true, + }, + ignore: cfg.ignore ?? [], + } +} + +/** + * `verifyx comments-hook` — a Claude Code PostToolUse hook. Reads the tool payload on stdin, scans the text the + * Edit/Write introduced, and on low-value comments writes feedback to stderr and exits 2 so the agent revises + * in-loop. Stays silent (exit 0) when the edit is clean or the payload is not a scannable file edit. + */ +export function registerCommentsHook(program: Command): void { + program + .command('comments-hook', { hidden: true }) + .description('Claude Code PostToolUse hook: flag low-value comments an edit introduced (reads hook JSON on stdin)') + .action(async () => { + const target = parsePostToolUsePayload(await readStdin()) + if (!target) return + const { options, ignore } = resolveOptions() + if (isIgnored(target.file, ignore)) return + + const findings = analyzeAddedComments(target, options) + if (!hasFindings(findings)) return + + process.stderr.write(`${formatHookFeedback(findings, options.contextOverride)}\n`) + process.exitCode = 2 + }) +} diff --git a/src/commands/registerInit.ts b/src/commands/registerInit.ts index 32aa9a7..ad56203 100644 --- a/src/commands/registerInit.ts +++ b/src/commands/registerInit.ts @@ -34,24 +34,54 @@ type InitCliOptions = { select: string[] claude?: boolean agents?: boolean + commentHook?: boolean + claudeDir?: string + commentScope?: string + commentBlockAll?: boolean + commentStrict?: boolean } -type Selections = { checks: string[]; targets: AgentTarget[]; defaultsOnly: boolean } +type CommentScope = 'diff' | 'all' +type CommentChoices = { commentScope: CommentScope; commentBlockAll: boolean; commentContextOverride: boolean } +type Selections = { + checks: string[] + targets: AgentTarget[] + defaultsOnly: boolean + commentHook: boolean +} & CommentChoices + +const NO_COMMENT_CHOICES: CommentChoices = { commentScope: 'diff', commentBlockAll: false, commentContextOverride: true } + +async function askCommentOptions(): Promise { + const commentScope = (await ask('select', 'Comments — which comments should the check gate?', [ + { name: 'diff', message: 'Changed lines only (gate new code, skip legacy)', enabled: true }, + { name: 'all', message: 'The whole codebase' }, + ])) as CommentScope + const strictness = await ask('select', 'Comments — how strict?', [ + { name: 'heuristics', message: 'Heuristics: long blocks, narration, density', enabled: true }, + { name: 'blockAll', message: 'Block every comment in scope (JSDoc + context: still allowed)' }, + { name: 'strict', message: 'Strict: no comments at all — only JSDoc allowed' }, + ]) + return { commentScope, commentBlockAll: strictness !== 'heuristics', commentContextOverride: strictness !== 'strict' } +} -async function resolveSelections(opts: InitCliOptions): Promise { - const nonInteractive = !!opts.yes || !process.stdin.isTTY - if (nonInteractive) { - const targets: AgentTarget[] = [] - if (opts.claude !== false) targets.push('claude') - if (opts.agents) targets.push('agents') - return { - checks: opts.select.length > 0 ? opts.select : recommendedChecks().map((c) => c.name), - targets, - defaultsOnly: !!opts.defaultsOnly, - } +function nonInteractiveSelections(opts: InitCliOptions): Selections { + const targets: AgentTarget[] = [] + if (opts.claude !== false) targets.push('claude') + if (opts.agents) targets.push('agents') + return { + checks: opts.select.length > 0 ? opts.select : recommendedChecks().map((c) => c.name), + targets, + defaultsOnly: !!opts.defaultsOnly, + commentHook: opts.commentHook !== false, + commentScope: opts.commentScope === 'all' ? 'all' : 'diff', + // --comment-strict is the "no comments, JSDoc only" preset: block every comment and drop the context: override. + commentBlockAll: !!opts.commentBlockAll || !!opts.commentStrict, + commentContextOverride: !opts.commentStrict, } +} - // context: an up-front choice — run everything with defaults (verifyx all, no verify:* scripts), or hand-pick checks. +async function interactiveSelections(): Promise { const mode = await ask('select', 'How should verify run?', [ { name: 'defaults', message: 'Run all built-in checks (verifyx all) with default options', enabled: true }, { name: 'pick', message: 'Pick specific checks to wire up as verify:* scripts' }, @@ -70,7 +100,22 @@ async function resolveSelections(opts: InitCliOptions): Promise { { name: 'claude', message: 'Claude (.claude/skills + CLAUDE.md)', enabled: true }, { name: 'agents', message: 'Other agents (.agent-skills + AGENTS.md)', enabled: false }, ]) - return { checks, targets, defaultsOnly } + + // The edit-time hook is Claude-specific; only offer it when Claude is a target. + const commentHook = targets.includes('claude') + ? (await ask('select', 'Enable the edit-time comment hook (Claude PostToolUse)?', [ + { name: 'yes', message: 'Yes — flag low-value comments the moment a file is edited', enabled: true }, + { name: 'no', message: 'No — rely on the verify/CI comments check only' }, + ])) === 'yes' + : false + + const comments = checks.includes('comments') ? await askCommentOptions() : NO_COMMENT_CHOICES + return { checks, targets, defaultsOnly, commentHook, ...comments } +} + +async function resolveSelections(opts: InitCliOptions): Promise { + const nonInteractive = !!opts.yes || !process.stdin.isTTY + return nonInteractive ? nonInteractiveSelections(opts) : interactiveSelections() } function report(result: InitResult, defaultsOnly: boolean): void { @@ -82,6 +127,13 @@ function report(result: InitResult, defaultsOnly: boolean): void { if (file.action === 'unchanged') continue console.log(` ${ACTION_MARK[file.action]} ${file.path} (${file.action})`) } + if (result.rootDir !== process.cwd() && result.agentFiles.length > 0) { + console.log( + color.yellow( + `\nAgent files were written to ${result.rootDir} (nearest existing .claude). Claude Code only loads them when launched from there — pass --claude-dir to target a different directory.`, + ), + ) + } } /** Summarise the dependency install: what was skipped, installed, and — last, so it's the takeaway — what failed. */ @@ -114,11 +166,27 @@ export function registerInit(program: Command): void { .option('--select ', 'preselect a check by name (repeatable, non-interactive)', collect, []) .option('--no-claude', 'do not write .claude/ files (non-interactive)') .option('--agents', 'also write .agent-skills/ files (non-interactive)') + .option('--no-comment-hook', 'do not register the edit-time comment PostToolUse hook') + .option('--claude-dir ', 'directory to write .claude/.agent-skills into (default: nearest existing .claude, else cwd)') + .option('--comment-scope ', 'comments check scope baked into verify:comments: diff (default) or all') + .option('--comment-block-all', 'bake --block-all into verify:comments (fail every comment in scope)') + .option('--comment-strict', 'strictest: no comments, only JSDoc (bakes --block-all --no-context-override)') .action(async (opts: InitCliOptions) => { const cwd = process.cwd() - const { checks, targets, defaultsOnly } = await resolveSelections(opts) - - const result = applyInit({ cwd, checks, targets, defaultsOnly }) + const { checks, targets, defaultsOnly, commentHook, commentScope, commentBlockAll, commentContextOverride } = + await resolveSelections(opts) + + const result = applyInit({ + cwd, + checks, + targets, + defaultsOnly, + commentHook, + claudeDir: opts.claudeDir, + commentScope, + commentBlockAll, + commentContextOverride, + }) report(result, defaultsOnly) if (result.devDeps.length > 0) { diff --git a/src/comments.ts b/src/comments.ts index b905c0e..852ea5c 100644 --- a/src/comments.ts +++ b/src/comments.ts @@ -10,7 +10,8 @@ export type CommentBlockViolation = { // context: `context:` is the deliberate escape hatch — a human/AI marks a comment as durable // context (the "why") rather than implementation narration, and it is never flagged. -function isContextExempt(firstLine: string): boolean { +function isContextExempt(firstLine: string, contextOverride: boolean): boolean { + if (!contextOverride) return false const stripped = firstLine.replace(/^\s*(?:\/\/+|\/\*+|\*)\s*/, '') return stripped.toLowerCase().startsWith('context:') } @@ -20,6 +21,7 @@ function consumeBlockComment( lines: readonly string[], start: number, maxLines: number, + contextOverride: boolean, out: CommentBlockViolation[], ): number { const isJsDoc = (lines[start] as string).trim().startsWith('/**') @@ -27,7 +29,7 @@ function consumeBlockComment( while (end < lines.length && !(lines[end] as string).includes('*/')) end++ const last = Math.min(end, lines.length - 1) const count = last - start + 1 - if (!isJsDoc && !isContextExempt(lines[start] as string) && count > maxLines) { + if (!isJsDoc && !isContextExempt(lines[start] as string, contextOverride) && count > maxLines) { out.push({ file, line: start + 1, lines: count }) } return last + 1 @@ -38,32 +40,45 @@ function consumeLineComments( lines: readonly string[], start: number, maxLines: number, + contextOverride: boolean, out: CommentBlockViolation[], ): number { let end = start while (end < lines.length && (lines[end] as string).trim().startsWith('//')) end++ const count = end - start - if (!isContextExempt(lines[start] as string) && count > maxLines) { + if (!isContextExempt(lines[start] as string, contextOverride) && count > maxLines) { out.push({ file, line: start + 1, lines: count }) } return end } -function scanFile(file: string, content: string, maxLines: number, out: CommentBlockViolation[]): void { +function scanFile(file: string, content: string, maxLines: number, contextOverride: boolean, out: CommentBlockViolation[]): void { const lines = content.split('\n') let i = 0 while (i < lines.length) { const trimmed = (lines[i] as string).trim() if (trimmed.startsWith('/*')) { - i = consumeBlockComment(file, lines, i, maxLines, out) + i = consumeBlockComment(file, lines, i, maxLines, contextOverride, out) } else if (trimmed.startsWith('//')) { - i = consumeLineComments(file, lines, i, maxLines, out) + i = consumeLineComments(file, lines, i, maxLines, contextOverride, out) } else { i++ } } } +/** Flag comment blocks longer than `maxLines` in a single file's `content` (see {@link findLongCommentBlocks}). */ +export function findLongCommentBlocksInContent( + file: string, + content: string, + maxLines: number, + contextOverride = true, +): CommentBlockViolation[] { + const out: CommentBlockViolation[] = [] + scanFile(file, content, maxLines, contextOverride, out) + return out +} + /** * Flag comment blocks longer than `maxLines`. A block is a run of consecutive whole-line `//` * comments or a `/* ... *\/` block comment. JSDoc (`/**`) blocks and blocks whose first line @@ -72,7 +87,7 @@ function scanFile(file: string, content: string, maxLines: number, out: CommentB export function findLongCommentBlocks(files: readonly string[], maxLines: number): CommentBlockViolation[] { const out: CommentBlockViolation[] = [] for (const file of files) { - scanFile(file, fs.readFileSync(file, 'utf-8'), maxLines, out) + scanFile(file, fs.readFileSync(file, 'utf-8'), maxLines, true, out) } return out } diff --git a/src/hook/analyze.test.ts b/src/hook/analyze.test.ts new file mode 100644 index 0000000..a3fca71 --- /dev/null +++ b/src/hook/analyze.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from 'vitest' + +import { analyzeAddedComments, formatHookFeedback, hasFindings, type HookOptions } from './analyze.ts' + +const opts: HookOptions = { maxLines: 2, narration: true, density: 0.3, minAddedLines: 10, blockAll: false, contextOverride: true } + +describe('analyzeAddedComments', () => { + it('flags a narration comment an edit introduced', () => { + const f = analyzeAddedComments({ file: 'a.ts', addedText: '// let me wire this up\nconst x = 1' }, opts) + expect(f.narration).toHaveLength(1) + expect(hasFindings(f)).toBe(true) + }) + + it('flags a long comment block', () => { + const f = analyzeAddedComments({ file: 'a.ts', addedText: ['// one', '// two', '// three', 'const x = 1'].join('\n') }, opts) + expect(f.blocks).toHaveLength(1) + }) + + it('flags a comment-dense edit', () => { + const lines = Array.from({ length: 12 }, (_, i) => (i % 2 === 0 ? `// note ${i}` : `const v${i} = ${i}`)) + const f = analyzeAddedComments({ file: 'a.ts', addedText: lines.join('\n') }, opts) + expect(f.density).not.toBeNull() + }) + + it('stays clean for a self-documenting edit', () => { + const f = analyzeAddedComments({ file: 'a.ts', addedText: 'export const total = items.reduce((a, b) => a + b, 0)' }, opts) + expect(hasFindings(f)).toBe(false) + }) + + it('never flags JSDoc or context: comments', () => { + const text = ['/**', ' * @param x the input', ' */', '// context: durable note about why', 'const x = 1'].join('\n') + const f = analyzeAddedComments({ file: 'a.ts', addedText: text }, opts) + expect(hasFindings(f)).toBe(false) + }) + + it('under blockAll, flags every non-exempt comment (and skips heuristics)', () => { + const f = analyzeAddedComments({ file: 'a.ts', addedText: '// a plain useful why\nconst x = 1' }, { ...opts, blockAll: true }) + expect(f.all).toHaveLength(1) + expect(f.blocks).toEqual([]) + expect(hasFindings(f)).toBe(true) + }) + + it('with contextOverride false, no longer exempts a context: comment', () => { + const text = '// context: let me keep this durable note\nconst x = 1' + expect(hasFindings(analyzeAddedComments({ file: 'a.ts', addedText: text }, opts))).toBe(false) + const strict = analyzeAddedComments({ file: 'a.ts', addedText: text }, { ...opts, contextOverride: false }) + expect(strict.narration).toHaveLength(1) + }) +}) + +describe('formatHookFeedback', () => { + it('names the file and includes the context: pushback by default', () => { + const f = analyzeAddedComments({ file: 'a.ts', addedText: '// let me do it\nconst x = 1' }, opts) + const msg = formatHookFeedback(f, true) + expect(msg).toContain('a.ts') + expect(msg).toContain('context:') + }) + + it('never mentions context: when the override is disabled', () => { + const f = analyzeAddedComments({ file: 'a.ts', addedText: '// let me do it\nconst x = 1' }, { ...opts, contextOverride: false }) + const msg = formatHookFeedback(f, false) + expect(msg).not.toContain('context:') + expect(msg).toContain('strictest') + }) +}) diff --git a/src/hook/analyze.ts b/src/hook/analyze.ts new file mode 100644 index 0000000..6fff402 --- /dev/null +++ b/src/hook/analyze.ts @@ -0,0 +1,67 @@ +import { commentSpan, isDiffExempt, type NewComment, toSingleLine } from '../checks/comment-common.ts' +import { type DensityViolation, findCommentDensity, findNarrationComments } from '../checks/comment-heuristics.ts' +import { type CommentBlockViolation, findLongCommentBlocksInContent } from '../comments.ts' +import { pushbackSuffix } from '../report.ts' +import { scanComments } from '../shared/comment-scan.ts' +import type { HookTarget } from './payload.ts' + +export type HookOptions = { + maxLines: number + narration: boolean + density: number + minAddedLines: number + blockAll: boolean + contextOverride: boolean +} + +export type HookFindings = { + file: string + /** Every non-exempt comment the edit added — populated only under blockAll. */ + all: NewComment[] + blocks: CommentBlockViolation[] + narration: NewComment[] + density: DensityViolation | null +} + +export function hasFindings(f: HookFindings): boolean { + return f.all.length > 0 || f.blocks.length > 0 || f.narration.length > 0 || f.density !== null +} + +/** + * Apply the comment gates to the text a single edit introduced. Every comment in `addedText` is treated as new, + * so there is no diff — the fragment itself is the change (always diff-scoped). JSDoc and `context:` are exempt. + */ +export function analyzeAddedComments(target: HookTarget, opts: HookOptions): HookFindings { + const fresh: NewComment[] = scanComments(target.file, target.addedText) + .filter((c) => !isDiffExempt(c.text, opts.contextOverride)) + .map((c) => ({ file: target.file, line: c.line, text: toSingleLine(c.text) })) + + if (opts.blockAll) return { file: target.file, all: fresh, blocks: [], narration: [], density: null } + + const blocks = opts.maxLines > 0 ? findLongCommentBlocksInContent(target.file, target.addedText, opts.maxLines, opts.contextOverride) : [] + const narration = opts.narration ? findNarrationComments(fresh, opts.contextOverride) : [] + + let density: DensityViolation | null = null + if (opts.density > 0) { + const added = target.addedText.split('\n').filter((l) => l.trim() !== '').length + const commentLines = scanComments(target.file, target.addedText) + .filter((c) => !isDiffExempt(c.text, opts.contextOverride)) + .reduce((n, c) => n + commentSpan(c.text), 0) + const perFile = new Map([[target.file, { added, commentLines: Math.min(commentLines, added), removedComments: 0 }]]) + density = findCommentDensity(perFile, { threshold: opts.density, minAddedLines: opts.minAddedLines })[0] ?? null + } + + return { file: target.file, all: [], blocks, narration, density } +} + +/** Build the stderr feedback an edit-time hook returns to the agent when it wrote low-value comments. */ +export function formatHookFeedback(f: HookFindings, contextOverride: boolean): string { + const parts: string[] = [`Low-value comments in your edit to ${f.file}:`] + for (const c of f.all) parts.push(` • comment (--block-all): "${c.text}"`) + for (const b of f.blocks) + parts.push(` • ${b.lines}-line comment block near line ${b.line} — too long; comments should explain *why*, not narrate *what*.`) + for (const n of f.narration) parts.push(` • narration comment: "${n.text}"`) + if (f.density) + parts.push(` • ${Math.round(f.density.ratio * 100)}% of the edit is comments (${f.density.commentLines}/${f.density.added} lines).`) + return parts.join('\n') + pushbackSuffix({ pushback: true, contextOverride }) +} diff --git a/src/hook/payload.test.ts b/src/hook/payload.test.ts new file mode 100644 index 0000000..6dd5446 --- /dev/null +++ b/src/hook/payload.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from 'vitest' + +import { parsePostToolUsePayload } from './payload.ts' + +function json(value: unknown): string { + return JSON.stringify(value) +} + +describe('parsePostToolUsePayload', () => { + it('parses a Write payload (whole content is added)', () => { + const raw = json({ tool_name: 'Write', tool_input: { file_path: 'a.ts', content: '// hi\nconst x = 1' } }) + expect(parsePostToolUsePayload(raw)).toEqual({ file: 'a.ts', addedText: '// hi\nconst x = 1' }) + }) + + it('parses an Edit payload from new_string', () => { + const raw = json({ tool_name: 'Edit', tool_input: { file_path: 'a.ts', old_string: 'a', new_string: '// added\nconst y = 2' } }) + expect(parsePostToolUsePayload(raw)).toEqual({ file: 'a.ts', addedText: '// added\nconst y = 2' }) + }) + + it('concatenates every MultiEdit new_string', () => { + const raw = json({ + tool_name: 'MultiEdit', + tool_input: { file_path: 'a.ts', edits: [{ new_string: '// one' }, { new_string: '// two' }] }, + }) + expect(parsePostToolUsePayload(raw)?.addedText).toBe('// one\n// two') + }) + + it('accepts the `path` key as well as `file_path`', () => { + const raw = json({ tool_name: 'Write', tool_input: { path: 'a.ts', content: 'const x = 1' } }) + expect(parsePostToolUsePayload(raw)?.file).toBe('a.ts') + }) + + it('returns null for non-file tools, unscanned extensions, empty text, and bad JSON', () => { + expect(parsePostToolUsePayload(json({ tool_name: 'Bash', tool_input: { command: 'ls' } }))).toBeNull() + expect(parsePostToolUsePayload(json({ tool_name: 'Write', tool_input: { file_path: 'a.py', content: 'x = 1' } }))).toBeNull() + expect(parsePostToolUsePayload(json({ tool_name: 'Write', tool_input: { file_path: 'a.ts', content: ' ' } }))).toBeNull() + expect(parsePostToolUsePayload('not json')).toBeNull() + }) +}) diff --git a/src/hook/payload.ts b/src/hook/payload.ts new file mode 100644 index 0000000..330fdde --- /dev/null +++ b/src/hook/payload.ts @@ -0,0 +1,70 @@ +import { isScannedExtension } from '../checks/comment-common.ts' + +export type HookTarget = { file: string; addedText: string } + +type Json = Record + +function asRecord(value: unknown): Json | undefined { + return value && typeof value === 'object' && !Array.isArray(value) ? (value as Json) : undefined +} + +function str(value: unknown): string | undefined { + return typeof value === 'string' ? value : undefined +} + +function filePathOf(input: Json): string | undefined { + // the file path key differs across Claude Code versions/tools (file_path vs path), so we accept either. + return str(input.file_path) ?? str(input.path) +} + +/** The text an edit operation introduces — new_string / new_str (Edit) across naming variants. */ +function newTextOf(op: Json): string { + return str(op.new_string) ?? str(op.new_str) ?? '' +} + +function addedTextFrom(toolName: string, input: Json): string { + if (toolName === 'Write') return str(input.content) ?? '' + if (toolName === 'Edit') { + // Some shapes nest the replacement under operations[]; most carry new_string directly on the input. + const ops = Array.isArray(input.operations) ? input.operations : [] + const nested = ops + .map(asRecord) + .filter((o): o is Json => !!o) + .map(newTextOf) + return [newTextOf(input), ...nested].filter(Boolean).join('\n') + } + if (toolName === 'MultiEdit') { + const edits = Array.isArray(input.edits) ? input.edits : [] + return edits + .map(asRecord) + .filter((e): e is Json => !!e) + .map(newTextOf) + .filter(Boolean) + .join('\n') + } + return '' +} + +/** + * Extract the edited file and the text a PostToolUse Write/Edit/MultiEdit introduced. Returns null when the + * payload is not a file-editing tool, has no path, targets an unscanned extension, or introduced no text. + */ +export function parsePostToolUsePayload(raw: string): HookTarget | null { + let parsed: unknown + try { + parsed = JSON.parse(raw) + } catch { + return null + } + const payload = asRecord(parsed) + if (!payload) return null + const toolName = str(payload.tool_name) ?? '' + if (!['Write', 'Edit', 'MultiEdit'].includes(toolName)) return null + const input = asRecord(payload.tool_input) + if (!input) return null + const file = filePathOf(input) + if (!file || !isScannedExtension(file)) return null + const addedText = addedTextFrom(toolName, input) + if (addedText.trim() === '') return null + return { file, addedText } +} diff --git a/src/report.ts b/src/report.ts index bb48499..8714bf7 100644 --- a/src/report.ts +++ b/src/report.ts @@ -1,6 +1,8 @@ import fs from 'node:fs' import type { FileScore } from './analyze.ts' +import type { NewComment } from './checks/comment-common.ts' +import type { DensityViolation } from './checks/comment-heuristics.ts' import type { CommentBlockViolation } from './comments.ts' import { forEachFunction } from './functions.ts' import { calculateCyclomaticComplexity, calculateHalstead, calculateMaintainabilityIndex, countSloc } from './metrics.ts' @@ -39,14 +41,37 @@ export function printFailure(failing: readonly FileScore[], threshold: number): const PUSHBACK = '\n\n⚠️ THIS IS YOUR LAST CHANCE TO RECONSIDER BEFORE INVOLVING A HUMAN. Adding `context:` pages a real person to approve this comment. DO NOT WASTE THEIR TIME. You had BETTER BE RIGHT that this comment is genuinely necessary — that it explains *why*, not *what*, and that the code cannot simply be made self-explanatory. If in doubt, delete the comment.' +// with the context override disabled there is no sanctioned way to keep a flagged comment, so the standard +// pushback (which dangles the override + a human approver) would be nonsense; this variant never mentions it. +const STRICT_PUSHBACK = + '\n\n⚠️ THIS IS YOUR LAST CHANCE TO RECONSIDER. This project runs the comment gate at its strictest: there is no escape hatch. Delete the comment, or make the code express the intent itself. Do NOT weaken, silence, or remove the check.' + +/** Where the gate looked, for report wording. */ +export type CommentScope = 'diff' | 'all' +const where = (scope: CommentScope): string => (scope === 'all' ? 'in the codebase' : 'on changed lines') + +/** The `context:`/JSDoc "how to keep it" clause — collapses to JSDoc-only when the `context:` override is off. */ +export function pushbackSuffix(opts: { pushback: boolean; contextOverride: boolean }): string { + if (!opts.pushback) return '' + return opts.contextOverride ? PUSHBACK : STRICT_PUSHBACK +} + +type FindingReportOpts = { scope: CommentScope; pushback: boolean; contextOverride: boolean } +const findingList = (findings: readonly NewComment[]): string => findings.map((c) => ` ${c.file}:${c.line} → ${c.text}`).join('\n') + +const keepClause = (contextOverride: boolean): string => + contextOverride + ? 'If the comment is genuinely durable context the code cannot express, prefix its first line with `context:` to keep it. JSDoc (`/** … */`) is always allowed.' + : 'JSDoc (`/** … */`) is the only exemption here — delete the comment or make the code self-documenting.' + /** Report comment blocks longer than the configured maximum. `warn` prints without failing; `pushback` adds AI back-pressure framing. */ export function printCommentBlockReport( violations: readonly CommentBlockViolation[], maxLines: number, - opts: { pushback: boolean; warn: boolean }, + opts: { pushback: boolean; warn: boolean; contextOverride: boolean }, ): void { const list = violations.map((v) => ` ${v.file}:${v.line} → ${v.lines} lines`).join('\n') - const message = `\n${opts.warn ? 'Warn' : 'Fail'}: ${violations.length} comment block(s) longer than ${maxLines} line(s).\n${list}\n\n${color.bold('Comments should explain *why*, not narrate *what*.')} Long comment blocks that describe implementation rot as the code changes, mislead future readers (and LLMs), and inflate complexity. Prefer self-documenting code: better names, smaller functions.\n\nIf the comment is genuinely durable context the code cannot express, prefix its first line with \`context:\` to keep it. JSDoc (\`/** … */\`) is always allowed.${opts.pushback ? PUSHBACK : ''}` + const message = `\n${opts.warn ? 'Warn' : 'Fail'}: ${violations.length} comment block(s) longer than ${maxLines} line(s).\n${list}\n\n${color.bold('Comments should explain *why*, not narrate *what*.')} Long comment blocks that describe implementation rot as the code changes, mislead future readers (and LLMs), and inflate complexity. Prefer self-documenting code: better names, smaller functions.\n\n${keepClause(opts.contextOverride)}${pushbackSuffix(opts)}` if (opts.warn) { console.warn(color.yellow(message)) } else { @@ -54,6 +79,45 @@ export function printCommentBlockReport( } } +/** Report every non-exempt comment in scope (the `--block-all` gate). `pushback` adds AI back-pressure framing. */ +export function printBlockAllReport(findings: readonly NewComment[], opts: FindingReportOpts): void { + const list = findingList(findings) + const exempt = opts.contextOverride + ? 'Machine directives and `context:`-prefixed comments are exempt (use that only where absolutely necessary; prefer deletion).' + : 'Only JSDoc and machine directives are exempt — every other comment must go.' + console.error( + color.red( + `\nFail: ${findings.length} comment(s) ${where(opts.scope)}.\n${list}\n\nWith --block-all, every comment ${where(opts.scope)} fails this gate. Remove them and let the code document itself. ${exempt}${pushbackSuffix(opts)}`, + ), + ) +} + +/** Report session-narration comments found in scope. `pushback` adds AI back-pressure framing. */ +export function printNarrationReport(findings: readonly NewComment[], opts: FindingReportOpts): void { + const list = findingList(findings) + const keep = opts.contextOverride ? ' If a line is genuinely durable context the code cannot express, prefix it with `context:`.' : '' + console.error( + color.red( + `\nFail: ${findings.length} narration comment(s) ${where(opts.scope)}.\n${list}\n\n${color.bold('These read as session narration — thinking out loud or restating *what* the next line does.')} They add no durable value and drift as the code changes. Delete them; let the code speak.${keep}${pushbackSuffix(opts)}`, + ), + ) +} + +/** Report files whose comment share in scope exceeds the density threshold. */ +export function printCommentDensityReport( + violations: readonly DensityViolation[], + opts: { threshold: number; scope: CommentScope; pushback: boolean; contextOverride: boolean }, +): void { + const pct = (n: number): string => `${Math.round(n * 100)}%` + const unit = opts.scope === 'all' ? 'lines' : 'added lines' + const list = violations.map((v) => ` ${v.file} → ${pct(v.ratio)} (${v.commentLines}/${v.added} ${unit})`).join('\n') + console.error( + color.red( + `\nFail: ${violations.length} file(s) over the ${pct(opts.threshold)} comment-density threshold ${where(opts.scope)}.\n${list}\n\n${color.bold('Too much of this is comments.')} Dense comment runs usually narrate *what* the code does. Prefer self-documenting code: better names, smaller functions. ${keepClause(opts.contextOverride)}${pushbackSuffix(opts)}`, + ), + ) +} + function printSloc(file: string, content: string): void { console.log(color.heading('SLOC')) const lines = countSloc(content) diff --git a/src/scaffold/agentFiles.ts b/src/scaffold/agentFiles.ts index 15ab9e3..38cab7c 100644 --- a/src/scaffold/agentFiles.ts +++ b/src/scaffold/agentFiles.ts @@ -26,18 +26,24 @@ function readTemplate(relativePath: string): string { * Skills are CLI-owned (created/updated as a whole). The instruction files are user-owned, so the pointer is * only appended when absent and existing content is never rewritten. */ -export function writeAgentFiles(cwd: string, targets: readonly AgentTarget[]): ManagedFileResult[] { +export function writeAgentFiles(rootDir: string, targets: readonly AgentTarget[]): ManagedFileResult[] { const results: ManagedFileResult[] = [] const skill = readTemplate('skills/verify/SKILL.md') + const pruneSkill = readTemplate('skills/prune-comments/SKILL.md') + const commentRule = readTemplate('rules/comments-only-when-non-obvious.md') const guidance = readTemplate('verify-guidance.md') if (targets.includes('claude')) { - writeManaged(path.join(cwd, '.claude', 'skills', 'verify', 'SKILL.md'), skill, results) - ensurePointer(path.join(cwd, 'CLAUDE.md'), guidance, POINTER_MARKER, results) + writeManaged(path.join(rootDir, '.claude', 'skills', 'verify', 'SKILL.md'), skill, results) + writeManaged(path.join(rootDir, '.claude', 'skills', 'prune-comments', 'SKILL.md'), pruneSkill, results) + writeManaged(path.join(rootDir, '.claude', 'rules', 'comments-only-when-non-obvious.md'), commentRule, results) + ensurePointer(path.join(rootDir, 'CLAUDE.md'), guidance, POINTER_MARKER, results) } if (targets.includes('agents')) { - writeManaged(path.join(cwd, '.agent-skills', 'verify', 'SKILL.md'), skill, results) - ensurePointer(path.join(cwd, 'AGENTS.md'), guidance, POINTER_MARKER, results) + writeManaged(path.join(rootDir, '.agent-skills', 'verify', 'SKILL.md'), skill, results) + writeManaged(path.join(rootDir, '.agent-skills', 'prune-comments', 'SKILL.md'), pruneSkill, results) + writeManaged(path.join(rootDir, '.agent-skills', 'rules', 'comments-only-when-non-obvious.md'), commentRule, results) + ensurePointer(path.join(rootDir, 'AGENTS.md'), guidance, POINTER_MARKER, results) } results.sort((a, b) => a.path.localeCompare(b.path)) diff --git a/src/scaffold/claudeDir.test.ts b/src/scaffold/claudeDir.test.ts new file mode 100644 index 0000000..edbd251 --- /dev/null +++ b/src/scaffold/claudeDir.test.ts @@ -0,0 +1,53 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { resolveClaudeDir } from './claudeDir.ts' + +let dir: string +const mk = (...segs: string[]): string => { + const p = path.join(dir, ...segs) + fs.mkdirSync(p, { recursive: true }) + return p +} + +beforeEach(() => { + dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'verify-claudedir-'))) +}) +afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }) +}) + +describe('resolveClaudeDir', () => { + it('uses cwd when it already has a .claude', () => { + const cwd = mk('pkg') + mk('pkg', '.claude') + expect(resolveClaudeDir(cwd)).toBe(cwd) + }) + + it('attaches to the nearest ancestor .claude within range', () => { + const root = mk('root') + mk('root', '.claude') + const cwd = mk('root', 'a', 'b') + expect(resolveClaudeDir(cwd)).toBe(root) + }) + + it('creates at cwd when no .claude is nearby', () => { + const cwd = mk('lonely') + expect(resolveClaudeDir(cwd)).toBe(cwd) + }) + + it('does not climb past the git root to an ancestor .claude', () => { + mk('.claude') // above the repo + mk('repo', '.git') + const cwd = mk('repo', 'pkg') + expect(resolveClaudeDir(cwd)).toBe(cwd) + }) + + it('honours an explicit override (resolved against cwd)', () => { + const cwd = mk('pkg') + expect(resolveClaudeDir(cwd, '../elsewhere')).toBe(path.resolve(cwd, '../elsewhere')) + }) +}) diff --git a/src/scaffold/claudeDir.ts b/src/scaffold/claudeDir.ts new file mode 100644 index 0000000..60c6b94 --- /dev/null +++ b/src/scaffold/claudeDir.ts @@ -0,0 +1,37 @@ +import fs from 'node:fs' +import path from 'node:path' + +const MAX_PARENTS = 3 + +function findGitRoot(from: string): string | null { + let dir = from + for (;;) { + if (fs.existsSync(path.join(dir, '.git'))) return dir + const parent = path.dirname(dir) + if (parent === dir) return null + dir = parent + } +} + +/** + * Resolve which directory init should write its `.claude` / `.agent-skills` integration into. Claude Code loads + * project settings only from the directory it is launched from (no inheritance), so the nearest existing + * `.claude` is the best available signal for that launch root: use `cwd` if it has one, else the closest ancestor + * within `MAX_PARENTS` (never climbing past the git root), else fall back to creating one at `cwd`. An explicit + * override always wins. + */ +export function resolveClaudeDir(cwd: string, override?: string): string { + if (override) return path.resolve(cwd, override) + if (fs.existsSync(path.join(cwd, '.claude'))) return cwd + + const gitRoot = findGitRoot(cwd) + let dir = cwd + for (let i = 0; i < MAX_PARENTS; i++) { + const parent = path.dirname(dir) + if (parent === dir) break + dir = parent + if (fs.existsSync(path.join(dir, '.claude'))) return dir + if (gitRoot && dir === gitRoot) break + } + return cwd +} diff --git a/src/scaffold/claudeSettings.test.ts b/src/scaffold/claudeSettings.test.ts new file mode 100644 index 0000000..d037d50 --- /dev/null +++ b/src/scaffold/claudeSettings.test.ts @@ -0,0 +1,61 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +import { ensureClaudeHook, HOOK_COMMAND } from './claudeSettings.ts' +import type { ManagedFileResult } from './writeManaged.ts' + +let dir: string +const settingsPath = (): string => path.join(dir, '.claude', 'settings.json') +const read = (): Record => JSON.parse(fs.readFileSync(settingsPath(), 'utf-8')) + +beforeEach(() => { + dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'verify-settings-'))) +}) +afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }) +}) + +function run(): ManagedFileResult[] { + const results: ManagedFileResult[] = [] + ensureClaudeHook(dir, results) + return results +} + +describe('ensureClaudeHook', () => { + it('creates settings.json with the PostToolUse hook when none exists', () => { + const [result] = run() + expect(result).toEqual({ path: settingsPath(), action: 'created' }) + const command = read().hooks as { PostToolUse: { hooks: { command: string }[] }[] } + expect(command.PostToolUse[0]?.hooks[0]?.command).toBe(HOOK_COMMAND) + }) + + it('merges the hook without clobbering existing settings', () => { + fs.mkdirSync(path.join(dir, '.claude'), { recursive: true }) + fs.writeFileSync(settingsPath(), JSON.stringify({ model: 'opus', hooks: { PreToolUse: [] } }, null, 2)) + const [result] = run() + expect(result?.action).toBe('updated') + const settings = read() as { model: string; hooks: { PreToolUse: unknown[]; PostToolUse: unknown[] } } + expect(settings.model).toBe('opus') + expect(settings.hooks.PreToolUse).toEqual([]) + expect(settings.hooks.PostToolUse).toHaveLength(1) + }) + + it('is idempotent — re-running does not duplicate the hook', () => { + run() + const [result] = run() + expect(result?.action).toBe('unchanged') + const settings = read() as { hooks: { PostToolUse: unknown[] } } + expect(settings.hooks.PostToolUse).toHaveLength(1) + }) + + it('leaves unparseable settings untouched', () => { + fs.mkdirSync(path.join(dir, '.claude'), { recursive: true }) + fs.writeFileSync(settingsPath(), '{ not valid json') + const [result] = run() + expect(result?.action).toBe('unchanged') + expect(fs.readFileSync(settingsPath(), 'utf-8')).toBe('{ not valid json') + }) +}) diff --git a/src/scaffold/claudeSettings.ts b/src/scaffold/claudeSettings.ts new file mode 100644 index 0000000..70867d7 --- /dev/null +++ b/src/scaffold/claudeSettings.ts @@ -0,0 +1,58 @@ +import fs from 'node:fs' +import path from 'node:path' + +import type { ManagedFileResult } from './writeManaged.ts' + +export const HOOK_COMMAND = 'npx verifyx comments-hook' +const HOOK_MATCHER = 'Edit|Write|MultiEdit' + +type HookEntry = { type?: string; command?: string } +type HookGroup = { matcher?: string; hooks?: HookEntry[] } +type ClaudeSettings = { hooks?: { PostToolUse?: HookGroup[] } & Record } & Record + +function hasOurHook(settings: ClaudeSettings): boolean { + const groups = settings.hooks?.PostToolUse ?? [] + return groups.some((g) => (g.hooks ?? []).some((h) => (h.command ?? '').includes(HOOK_COMMAND))) +} + +function addOurHook(settings: ClaudeSettings): void { + const hooks = (settings.hooks ??= {}) + const post = (hooks.PostToolUse ??= []) + post.push({ matcher: HOOK_MATCHER, hooks: [{ type: 'command', command: HOOK_COMMAND }] }) +} + +/** + * Ensure the project's `.claude/settings.json` registers the `verifyx comments-hook` PostToolUse hook, so the + * comment gate fires the moment an agent edits a file. Merges into existing settings — never rewrites unrelated + * keys — and is idempotent (detects our entry by its command). Leaves unparseable settings untouched. + */ +export function ensureClaudeHook(cwd: string, results: ManagedFileResult[]): void { + const file = path.join(cwd, '.claude', 'settings.json') + + if (!fs.existsSync(file)) { + const settings: ClaudeSettings = {} + addOurHook(settings) + fs.mkdirSync(path.dirname(file), { recursive: true }) + fs.writeFileSync(file, `${JSON.stringify(settings, null, 2)}\n`) + results.push({ path: file, action: 'created' }) + return + } + + let settings: ClaudeSettings + try { + settings = JSON.parse(fs.readFileSync(file, 'utf-8')) as ClaudeSettings + } catch { + // never clobber settings we can't parse. + results.push({ path: file, action: 'unchanged' }) + return + } + + if (hasOurHook(settings)) { + results.push({ path: file, action: 'unchanged' }) + return + } + + addOurHook(settings) + fs.writeFileSync(file, `${JSON.stringify(settings, null, 2)}\n`) + results.push({ path: file, action: 'updated' }) +} diff --git a/src/scaffold/init.test.ts b/src/scaffold/init.test.ts index 84cea28..40d35f2 100644 --- a/src/scaffold/init.test.ts +++ b/src/scaffold/init.test.ts @@ -56,6 +56,27 @@ describe('applyInit', () => { expect(result.devDeps).toEqual(expect.arrayContaining(['knip', 'jscpd', 'skott'])) }) + it('bakes the chosen comment scope/strictness into verify:comments', () => { + applyInit({ cwd: dir, checks: ['comments'], targets: [], defaultsOnly: false, commentScope: 'all', commentBlockAll: true }) + expect(readScripts()['verify:comments']).toBe('verifyx comments --pushback --scope all --block-all') + }) + + it('defaults comment options to a plain pushback script', () => { + applyInit({ cwd: dir, checks: ['comments'], targets: [], defaultsOnly: false }) + expect(readScripts()['verify:comments']).toBe('verifyx comments --pushback') + }) + + it('bakes the strict "no comments, JSDoc only" preset into verify:comments', () => { + applyInit({ cwd: dir, checks: ['comments'], targets: [], defaultsOnly: false, commentBlockAll: true, commentContextOverride: false }) + expect(readScripts()['verify:comments']).toBe('verifyx comments --pushback --block-all --no-context-override') + }) + + it('emits a verify:comments override under defaults-only only when a non-default comment option is chosen', () => { + applyInit({ cwd: dir, checks: CHECKS.map((c) => c.name), targets: [], defaultsOnly: true, commentBlockAll: true }) + expect(readScripts()['verify:comments']).toBe('verifyx comments --pushback --block-all') + expect(readScripts().verify).toBe('verifyx all') + }) + it('does not clobber an existing verify:* script', () => { fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ scripts: { 'verify:complexity': 'custom' } })) applyInit({ cwd: dir, checks: ['complexity'], targets: [], defaultsOnly: false }) diff --git a/src/scaffold/init.ts b/src/scaffold/init.ts index 8b2c496..d2e05d8 100644 --- a/src/scaffold/init.ts +++ b/src/scaffold/init.ts @@ -3,6 +3,8 @@ import path from 'node:path' import { getCheck } from '../checks/registry.ts' import type { Check } from '../checks/types.ts' import { type AgentTarget, writeAgentFiles } from './agentFiles.ts' +import { resolveClaudeDir } from './claudeDir.ts' +import { ensureClaudeHook } from './claudeSettings.ts' import { ensureKnipIgnores } from './knipConfig.ts' import { addVerifyScripts } from './packageScripts.ts' import type { ManagedFileResult } from './writeManaged.ts' @@ -14,6 +16,16 @@ export type InitOptions = { targets: readonly AgentTarget[] /** When true, do not write `verify:*` scripts — rely on `verify`'s built-in defaults. */ defaultsOnly: boolean + /** Register the edit-time comment gate as a Claude PostToolUse hook (claude target only). Default off. */ + commentHook?: boolean + /** Explicit directory to write `.claude`/`.agent-skills` into; overrides the nearest-`.claude` resolution. */ + claudeDir?: string + /** Comments check scope baked into the scaffolded script. Default `diff`. */ + commentScope?: 'diff' | 'all' + /** Bake `--block-all` into the scaffolded comments script (fail every comment in scope). Default false. */ + commentBlockAll?: boolean + /** Honour the `context:` override in the scaffolded script. Default true; false bakes `--no-context-override`. */ + commentContextOverride?: boolean } export type InitResult = { @@ -21,6 +33,22 @@ export type InitResult = { /** devDependencies the selected external checks need (deduped). */ devDeps: string[] agentFiles: ManagedFileResult[] + /** The directory the agent integration was written to (may differ from cwd in a monorepo). */ + rootDir: string +} + +/** The scaffolded `verify:comments` command, with the chosen scope/strictness baked in as flags. */ +function commentsScript(opts: InitOptions): string { + const flags = ['--pushback'] + if (opts.commentScope === 'all') flags.push('--scope all') + if (opts.commentBlockAll) flags.push('--block-all') + if (opts.commentContextOverride === false) flags.push('--no-context-override') + return `verifyx comments ${flags.join(' ')}` +} + +/** A non-default comment choice must be recorded, even under defaults-only, as a verify:comments override. */ +function commentsCustomised(opts: InitOptions): boolean { + return opts.commentScope === 'all' || !!opts.commentBlockAll || opts.commentContextOverride === false } /** Pure scaffolding step: write package.json scripts + agent files, and report the devDeps to install. */ @@ -32,15 +60,25 @@ export function applyInit(opts: InitOptions): InitResult { const check = getCheck(name) if (!check) continue if (check.scaffold.devDeps) devDeps.push(...check.scaffold.devDeps) - if (!opts.defaultsOnly) scripts[`verify:${name}`] = check.scaffold.script + if (!opts.defaultsOnly) scripts[`verify:${name}`] = name === 'comments' ? commentsScript(opts) : check.scaffold.script + } + + // Record non-default comment choice as a verify:comments override + if (opts.defaultsOnly && opts.checks.includes('comments') && commentsCustomised(opts)) { + scripts['verify:comments'] = commentsScript(opts) } // Defaults-only wires the top `verify` script to `verifyx all` so it runs every built-in with no verify:* list. const addedScripts = addVerifyScripts(path.join(opts.cwd, 'package.json'), scripts, opts.defaultsOnly ? 'verifyx all' : 'verifyx') - const agentFiles = writeAgentFiles(opts.cwd, opts.targets) + // Claude Code loads settings only from its launch dir, so we attach to the nearest existing .claude + const rootDir = resolveClaudeDir(opts.cwd, opts.claudeDir) + const agentFiles = writeAgentFiles(rootDir, opts.targets) + + // The PostToolUse hook is Claude-specific; other agents have no universal edit-time equivalent. + if (opts.commentHook && opts.targets.includes('claude')) ensureClaudeHook(rootDir, agentFiles) - // context: with unused-code selected, teach knip to ignore the other external tools verifyx runs at runtime. + // Teach knip to ignore the other external tools verifyx runs at runtime. if (opts.checks.includes('unused-code')) { const toolDeps = opts.checks .map(getCheck) @@ -50,5 +88,5 @@ export function applyInit(opts: InitOptions): InitResult { ensureKnipIgnores(opts.cwd, [...new Set(toolDeps)], agentFiles) } - return { addedScripts, devDeps: [...new Set(devDeps)], agentFiles } + return { addedScripts, devDeps: [...new Set(devDeps)], agentFiles, rootDir } } diff --git a/src/shared/comment-scan.ts b/src/shared/comment-scan.ts index 2ffe9ed..ddc319a 100644 --- a/src/shared/comment-scan.ts +++ b/src/shared/comment-scan.ts @@ -1,4 +1,3 @@ -import fs from 'node:fs' import path from 'node:path' import ts from 'typescript' @@ -58,8 +57,7 @@ function scanYamlComments(content: string): ScannedComment[] { return out } -/** Extract every comment (with its 1-based line) from a source file, dispatching on extension. */ -export function scanFileComments(file: string): ScannedComment[] { - const content = fs.readFileSync(file, 'utf-8') +/** Extract every comment (with its 1-based line) from source text, dispatching on the file's extension. */ +export function scanComments(file: string, content: string): ScannedComment[] { return isYamlFile(file) ? scanYamlComments(content) : scanCodeComments(file, content) } diff --git a/src/shared/config.ts b/src/shared/config.ts index b277c81..ff07eb7 100644 --- a/src/shared/config.ts +++ b/src/shared/config.ts @@ -9,7 +9,21 @@ export type ForbiddenStringsRule = { } export type VerifyConfig = { - comments?: { ignore?: string[] } + comments?: { + ignore?: string[] + /** Which comments the gates judge: `diff` (changed lines only) or `all` (whole codebase). Default `diff`. */ + scope?: 'diff' | 'all' + /** Fail every comment in scope, not just heuristic hits. Default false. */ + blockAll?: boolean + /** Flag session-narration comments. Default true. */ + narration?: boolean + /** Comment-density ratio (0–1) that fails a file; `false`/`0` disables. Default 0.3. */ + density?: number | false + /** Minimum added/scanned lines before density applies. Default 10. */ + minAddedLines?: number + /** Honour the `context:` override (those comments stay exempt). Default true; set false for a stricter gate / cleanup. */ + contextOverride?: boolean + } hardcodedColors?: { ignore?: string[]; root?: string } forbiddenStrings?: ForbiddenStringsRule[] } diff --git a/src/shared/diff.ts b/src/shared/diff.ts index a565394..1fe51a2 100644 --- a/src/shared/diff.ts +++ b/src/shared/diff.ts @@ -41,3 +41,26 @@ export function parseDiffAddedLines(diff: string): AddedLines { return added } + +/** Map each file in a unified diff to the content of the lines it removed (the `-` lines, marker stripped). */ +export function parseDiffRemovedLines(diff: string): Map { + const removed = new Map() + let currentFile: string | null = null + + for (const line of diff.split('\n')) { + const fileMatch = line.match(FILE_HEADER) + if (fileMatch) { + const file = fileMatch[1] as string + currentFile = file === '/dev/null' ? null : file + continue + } + if (line.startsWith('---') || HUNK_HEADER.test(line) || currentFile === null) continue + if (line.startsWith('-')) { + const arr = removed.get(currentFile) ?? [] + arr.push(line.slice(1)) + removed.set(currentFile, arr) + } + } + + return removed +} diff --git a/src/shared/git.ts b/src/shared/git.ts index bc8914b..9f523f3 100644 --- a/src/shared/git.ts +++ b/src/shared/git.ts @@ -8,8 +8,6 @@ function run(command: string): string { } } -// context: locally we diff the working tree against HEAD (uncommitted changes). A CI checkout has nothing -// uncommitted, so there we diff against the merge base with the PR base branch, else the gate is a no-op. function resolveDiffBase(): string { if (!process.env.CI) return 'HEAD' const base = process.env.VERIFY_DIFF_BASE || (process.env.GITHUB_BASE_REF ? `origin/${process.env.GITHUB_BASE_REF}` : '') diff --git a/templates/rules/comments-only-when-non-obvious.md b/templates/rules/comments-only-when-non-obvious.md new file mode 100644 index 0000000..d57b9e6 --- /dev/null +++ b/templates/rules/comments-only-when-non-obvious.md @@ -0,0 +1,87 @@ +--- +paths: + - '*.{ts,tsx,cts,mts,js,jsx,mjs,cjs,yml,yaml}' + - '**/*.{ts,tsx,cts,mts,js,jsx,mjs,cjs,yml,yaml}' +--- + +# Comment only when the code would mislead a competent reader + +Write self-documenting code: let well-named identifiers, narrow types, and small functions carry the meaning. Add a comment only when reading the code alone would make a competent reader mispredict its behaviour, and only to explain the _why_ the code itself cannot show. + +## When a comment earns its place + +Add one — kept to a single line where possible — for exactly these: + +- A non-obvious workaround: a specific bug, race, ordering hazard, or platform quirk the code is shaped around. +- A constraint imposed by an external system not visible in the code: an API contract, a wire format, a rate limit, a required call order. +- A deliberate deviation from the obvious approach, where the obvious approach is wrong for a reason a reader would not guess. + +The keep-it test: if a competent reader would predict the behaviour correctly without the comment, delete the comment. + +## What never to write + +These are the failure modes this rule exists to stop. They are the current default-model drift, and they poison codebases at scale. + +- **Prose blocks on trivial code.** A one-liner, an assignment, or a single call never gets a multi-line comment. A paragraph above one statement is the dominant failure. +- **Restatement.** A comment that re-says what the next line already says in code is noise, not signal. The code is the source of truth; a description of it rots and must then be re-verified by every reader. +- **Leaked session narration.** Never write the editing session's ephemeral reasoning into code or to disk: `Let me…`, `Now I'll…`, `This is a substantial change…`, `we need to…`, `as requested`. That belongs in the chat, never the file. +- **LLM tells.** Em-dashes and curly quotes (`—`, `“ ” ‘ ’`) in a comment are a strong sign it was generated narration rather than a deliberate note; the gate flags them. +- **Invented shorthand.** Do not coin a new term, abbreviation, or naming scheme inside a comment and leave it undefined; it snowballs and is never cleaned up. Use the codebase's existing vocabulary. +- **Duplicated explanation.** State a fact once, at its source of truth (the README, or the single function that owns it). A second site gets a one-line pointer, not a copy — the same explanation restated across many files is the same bug as restatement, spread out. + +## Examples + +Restatement and a prose block — delete it; the code already says this: + +```ts +// BAD +// Calculate the total by multiplying quantity by unit price, then store it so the +// invoice summary can use it later when it renders the line item. +const total = quantity * unitPrice + +// GOOD +const total = quantity * unitPrice +``` + +A justified comment — keep it; the constraint is external and invisible in the code: + +```ts +// GOOD: Stripe rejects sub-cent precision; round to integer cents before sending. +const amountCents = Math.round(price * 100) +``` + +Leaked session narration — strip it: + +```ts +// BAD +// Now let me add the retry logic that was requested. This is the substantial +// part of the change: wrap the call in a loop and back off exponentially. +for (let attempt = 0; attempt < maxRetries; attempt++) { ... } + +// GOOD +for (let attempt = 0; attempt < maxRetries; attempt++) { ... } +``` + +Config and infra are not exempt — the same restatement shows up in YAML: + +```yaml +# BAD: the key and its default already say this +# The log level for the service, defaulting to info. +logLevel: info + +# GOOD +logLevel: info +``` + +Duplicated explanation — say it once, point to it: + +```text +BAD: the full paragraph on how the retry budget is derived is copied into the + README, the client, and four call sites. +GOOD: the explanation lives once in the client; each call site carries + // retry budget: see resolveRetryBudget +``` + +## Enforcement + +This is gated, not merely advised. `verifyx comments` flags low-value comments — long blocks, session narration, and high comment density — over the current diff (or the whole codebase with `--scope all`). An edit-time PostToolUse hook (`verifyx comments-hook`) applies the same rules the moment a file is edited. The `prune-comments` skill strips and consolidates comments across a change on demand. diff --git a/templates/skills/prune-comments/SKILL.md b/templates/skills/prune-comments/SKILL.md new file mode 100644 index 0000000..706ee1d --- /dev/null +++ b/templates/skills/prune-comments/SKILL.md @@ -0,0 +1,47 @@ +--- +name: prune-comments +description: Strip low-value comments (restatement, session narration, prose blocks, dense runs) from a change. Applies edits in-place, file by file. Use when asked to "prune comments", "clean up comments", "remove comment bloat", or after the verify comments check flags them. Apply-capable — it edits files, it is not a read-only review. +--- + +# prune-comments + +Remove comment bloat and keep only the comments that earn their place, following the criteria in the +`comments-only-when-non-obvious` rule (`.claude/rules/` / `.agent-skills/rules/`). Edits are applied in-place. + +## 1. Find the flagged comments + +Run the check to see what fails (the binary is `verifyx`). By default it judges the **current diff** — the +comments this change introduced: + +``` +npx verifyx comments --pushback +``` + +Scope options (`verifyx comments` handles the git/diff scoping itself — you don't need to compute it): + +- **Whole repo:** `npx verifyx comments --scope all --pushback` — audit and prune every comment in the codebase, + not just the diff. +- **Every comment, not just heuristic hits:** add `--block-all` (with either scope) to remove _every_ comment in + scope, not just the heuristic hits. + +## 2. Delete or keep + +Apply the `comments-only-when-non-obvious` rule to each flagged comment: + +- **Delete** restatement, session narration (`let me…`, `as requested`, `this function does…`), prose blocks on + trivial code, LLM tells (em-dash / curly quotes), and duplicated explanation. When in doubt, delete. +- **Keep** only a non-obvious _why_: a workaround, an external constraint, or a deliberate non-obvious deviation. + +Prefer fixing the code over keeping the comment: a better name, a smaller function, or a narrower type often +removes the need for the comment entirely. Do not try to slip a low-value comment past the gate — remove it, or +make the code self-explanatory. + +## 3. Apply and re-run + +Edit with the Edit tool — one edit per change site, not a whole-file rewrite. Then re-run the same +`verifyx comments` command and repeat until it passes. Never silence the check or widen its ignore globs. + +## Output + +One line per file: `pruned: — removed`, `clean: `, or `needs manual review: `. +No preamble, no closing remarks.