[Review] Comment coverage — every module should explain itself to a stranger #444

New issue

Open

opened 2026-08-29 18:18:57 +00:00 by claude-bot · 0 comments

claude-bot commented

2026-08-29 18:18:57 +00:00

Contributor

Part of the v4.6.0 release-readiness gate.

What

Bring the whole codebase up to the standard the best parts already set: a reader arriving cold should be able to work out why the code is the way it is, not merely what it does.

The standard already exists here

The strong examples are the ones that explain a decision and its cost:

transcribe_session argues from first principles that any echo-based attribution join is unverifiable, and pre-empts the obvious "join on ids instead" objection.
PerUserPCMSink explains why placement is absolute rather than incremental, and bounds the error that choice leaves.
_MAX_FOLD_DEPTH records that unbounded folding climbs until the OOM killer takes the worker and the session sits in "processing" forever with nothing logged.
The settings_service module docstring enumerates four policies for "no LLM configured" and says what is not acceptable, because eight behaviours had grown for one condition.

Each answers a question the next reader would otherwise have to rediscover. That is the bar.

What to add

Why, not what. # increment the counter earns nothing. # non-cumulative, so it cannot drift over a four-hour session earns its line.
The rejected alternative, where one was seriously considered. Knowing what was tried and why it failed prevents it being retried.
The cost of the choice. _COMPACT_MAX_RUN_SECONDS states the worst-case timestamp error its merging introduces — that is what makes it reviewable.
Invariants a future change could break silently, especially cross-file ones. The transcript line format is parsed back by beat_service; both ends say so.
Module docstrings that orient: what this file is responsible for, what it deliberately is not.

What to avoid

Comments restating the line beneath them
Docstrings that will rot on the next change — prefer explaining the constraint over the current values
Ceremony for its own sake: a genuinely obvious three-line helper needs nothing

A hard rule from experience

A wrong comment is worse than no comment. VAD's docstring claimed the opposite of the truth for months, and #427's docstring confidently justified behaviour that destroyed a real session. This pass must correct stale prose wherever it is found — that counts as in-scope work here, not a distraction from it.

Acceptance criteria

Every module has a docstring stating its responsibility and its boundaries
Every non-obvious constant records why that value, or what constrains it
Every cross-file invariant is documented at both ends
Stale or contradicted comments found during the pass are corrected
Deliberate omissions and deliberately-tolerated failure modes are labelled as deliberate, so a later reader does not "fix" them
Spot-check: a reader unfamiliar with a module can state its purpose and one non-obvious constraint after five minutes

Part of the v4.6.0 release-readiness gate. ## What Bring the whole codebase up to the standard the best parts already set: a reader arriving cold should be able to work out **why** the code is the way it is, not merely what it does. ## The standard already exists here The strong examples are the ones that explain a decision and its cost: - `transcribe_session` argues from first principles that any echo-based attribution join is unverifiable, and pre-empts the obvious "join on ids instead" objection. - `PerUserPCMSink` explains why placement is absolute rather than incremental, and bounds the error that choice leaves. - `_MAX_FOLD_DEPTH` records that unbounded folding climbs until the OOM killer takes the worker and the session sits in "processing" forever with nothing logged. - The `settings_service` module docstring enumerates four policies for "no LLM configured" and says what is *not* acceptable, because eight behaviours had grown for one condition. Each answers a question the next reader would otherwise have to rediscover. That is the bar. ## What to add - **Why, not what.** `# increment the counter` earns nothing. `# non-cumulative, so it cannot drift over a four-hour session` earns its line. - **The rejected alternative**, where one was seriously considered. Knowing what was tried and why it failed prevents it being retried. - **The cost of the choice.** `_COMPACT_MAX_RUN_SECONDS` states the worst-case timestamp error its merging introduces — that is what makes it reviewable. - **Invariants a future change could break silently**, especially cross-file ones. The transcript line format is parsed back by `beat_service`; both ends say so. - **Module docstrings** that orient: what this file is responsible for, what it deliberately is not. ## What to avoid - Comments restating the line beneath them - Docstrings that will rot on the next change — prefer explaining the constraint over the current values - Ceremony for its own sake: a genuinely obvious three-line helper needs nothing ## A hard rule from experience **A wrong comment is worse than no comment.** VAD's docstring claimed the opposite of the truth for months, and #427's docstring confidently justified behaviour that destroyed a real session. This pass must correct stale prose wherever it is found — that counts as in-scope work here, not a distraction from it. ## Acceptance criteria - [ ] Every module has a docstring stating its responsibility and its boundaries - [ ] Every non-obvious constant records why that value, or what constrains it - [ ] Every cross-file invariant is documented at both ends - [ ] Stale or contradicted comments found during the pass are corrected - [ ] Deliberate omissions and deliberately-tolerated failure modes are labelled as deliberate, so a later reader does not "fix" them - [ ] Spot-check: a reader unfamiliar with a module can state its purpose and one non-obvious constraint after five minutes

claude-bot added this to the v4.6.0 - Release Readiness: Review, Simplification & Documentation milestone

2026-08-29 18:19:28 +00:00