Skip to content

chore: comments carry their reason, and docs/architecture is walkable - #476

Merged
JArmandoAnaya merged 8 commits into
mainfrom
chore/comment-cleanup-and-arch-docs
Aug 10, 2026
Merged

chore: comments carry their reason, and docs/architecture is walkable#476
JArmandoAnaya merged 8 commits into
mainfrom
chore/comment-cleanup-and-arch-docs

Conversation

@JArmandoAnaya

Copy link
Copy Markdown
Contributor

Closes #474

A documentation-only change in two parts: every comment and docstring that explained
itself with an issue number now explains itself, and docs/architecture/ is a walkable
tree of the system's shape. No runtime behaviour, no test assertion, no type, and no
hand-edited artifact.

Part A — comments carry their reason

#160, see #212, fixed in #323 are pointers, not explanations: a reader in an
editor, a vendored copy or a fork cannot follow one. Each reference was resolved by
reading the issue it named and applying exactly one rule:

  1. The prose already explained it → the number goes, the prose gets tighter.
  2. The meaning lived in the issue → read it, extract the reason, rewrite as one or
    two present-tense sentences about the code — what breaks without this, not which
    ticket found it
    — then drop the number.
  3. Pure noise → the line goes.

A comment carrying an invariant, a non-obvious constraint, or a "why" the code cannot
express was never deleted, only shortened. Docstrings lost their development history
and kept what the thing does and why it exists.

Scope was comments and docstrings only. String literals are code: a test name, an
assertion message, a JSX description= prop and the text scripts/export_mcp_tools.py
generates into docs/mcp-tools.md all reach an output, so none were touched.

Counts

Reference lines under src/ frontend/ tests/ scripts/ examples/ 1814 → 54
Files carrying one 382 → 23
Files edited in Part A 381
Comment lines 2939 added, 3196 removed (net −257)
Comments rewritten 1778 hunks
Comments deleted outright 2 hunks, 5 lines

The last two rows are the point: almost nothing was deleted. The two exceptions are both
development narrative whose rule survives in the sentences around it —
batchState.ts ("this file used to argue there was no green…") and a paragraph in
_tables.py describing the column that was not added.

The 54 surviving references are each inside a string literal, verified line by line:
topBar.test.tsx (11), dataset.test.tsx (9), gallery.test.tsx (4), and 20 other
files with one to three apiece.

How "only comments changed" was proven

Two verifiers, run over every file in the diff:

  • Python — parses both revisions with ast, strips every bare string-literal
    statement at every level (docstrings and the #: attribute docs), and compares
    ast.dump. Byte-identical trees on all 162 Python files.
  • TypeScript / JS — a comment stripper that tracks string, template and regex
    context, so a // inside a URL is not mistaken for a comment. Run over all 213
    TypeScript/JS files, and mutation-tested against a real code edit to prove it fires.

Both earned their keep. They caught, and I reverted, four edits that had strayed out of
scope: 10 it(...) / describe(...) test names, a published MCP description string, and
an assertion template literal.

Part B — docs/architecture/

Sixteen pages, 1164 lines, top-down and short: the system, then a page per backend
package and per frontend workspace package, then one page for what runs through all of
them. Eighteen Mermaid diagrams, each 5–12 nodes. Every relative link resolves (175
checked).

docs/architecture/README.md          the two halves, the one-way graph
  backend/README.md                  the layer stack, the four import contracts
    kernel.md  server.md  cli.md  mcp.md  formats.md  wire.md  jobs.md  inference.md
  frontend/README.md                 the package graph and each package's "never" column
    annotator.md  ui-core.md  app.md
  cross-cutting.md                   the two boundaries, capabilities, the lifecycle

Every claim was checked against HEAD before it was written — the four
[[tool.importlinter.contracts]] blocks, the nine format entry points, the three
package.json files, tsconfig.core.json, the ESLint boundary rules, the contents of
tests/architecture/, and the three transition tables dumped live rather than recalled.

The skills under .agents/skills/ and DESIGN.md stay authoritative: these pages link
to them and never restate them. docs/README.md gains the index row.

Also: the rule that would have prevented this

AGENTS.md had nothing to say about writing a comment, which is why 1814 of them said
#N. It now has a Comments and docstrings section under Rules, with an
Auto-invoke row pointing at it: be brief (one to three sentences); never explain with
an issue or PR number; cf. #N only where the history is genuinely load-bearing and
the comment already stands without it; never delete a comment that carries an invariant —
rewrite it shorter; and a pydantic model's or a route's docstring is published, so it
is written for a client reading the contract. CLAUDE.md is a git-ignored copy of that
file and tracks it automatically.

Gate

bash scripts/check.sh, in two invocations, both green. Not --fast.

python + frontend + generated   All checks passed.   (220s)
  pytest 3054 passed, 9 skipped · ruff check · ruff format · mypy kernel
  lint-imports 4/4 contracts kept · pnpm -r build/test/lint
  openapi drift · generated client drift · mcp tool reference drift · version sync

browser                          All checks passed.   exit 0
  e2e (chromium)  234 passed (1.4m)
  cycle (real server, real kernel)  1 passed (20.6s)

openapi.json and the generated client moved — deliberately

FastAPI copies a route's and a pydantic model's docstring verbatim into
openapi.json as description. Seven of those docstrings are in scope, so
test_the_committed_openapi_matches_the_application failed against the committed spec
until it was regenerated.

Both artifacts were regenerated, never hand-edited, in their own commit
(chore(api): regenerate the spec and the client after the docstring pass): 14 lines in
openapi.json, 31 in frontend/ui-core/src/generated/api.ts. The api.ts diff has
zero changed lines that are not JSDoc comment lines — no type, no path, no operation
id moved.

Needs human review

None. Every one of the 180 referenced issues was readable, so no comment was left
with a meaning I could not reconstruct, and nothing was guessed.

Banned surfaces

gh pr list --state open was empty at pre-flight and is empty now, so no file was
skipped on that ground. Issues #81 and #408 own no branch that this touches.

Protocol deviations

  1. Four kernel files were edited while feat/weights-integrity was still open.
    _weights.py, weights.py and two beside them were live on that branch when I
    touched them. It merged as PR feat(inference): a weights integrity check that re-reads every byte and purges damage #475 mid-flight (main4916e64); I rebased onto it
    cleanly, then swept the nine new #471 references the merge brought in
    (chore(kernel): the weights-integrity comments carry their reason too). No conflict
    survived, but the contact happened before the merge and is reported rather than
    quietly absorbed.
  2. Generated artifacts changed, which the dispatch put out of scope. Regenerating was
    the only alternative to leaving a failing gate; see the section above.
  3. AGENTS.md is outside both parts. Added on request mid-task.
  4. src/visionset/formats/lanes/README.md was left alone: it is prose documentation
    inside src/, and prose cites an issue deliberately.

Found, not fixed

Nothing. This change surfaced no bug.

Do not merge on my account — CI green is where this stops.

Every comment and docstring under `src/visionset/kernel` that cited an issue or
PR number now states the reason itself. A bare number is a pointer a reader in an
editor, a vendored copy or a fork cannot follow, so it explained nothing on its
own.

Where the prose already carried the explanation the number is simply gone; where
the meaning depended on the referenced issue the reason was read out of it and
written into the comment. Development narrative — what a shape used to be, which
slice added a field — is dropped, and the invariants those narratives were
wrapped around are kept, shorter.

cf. #474
The slice that landed as 4916e64 arrived after this branch forked, bringing nine
more comments whose explanation was an issue number. Same treatment: the reason
is written into the comment and the number is gone.

cf. #474
…number

The same rule applied outside the kernel: `src/visionset/server`, `cli`, `mcp`,
`formats`, `inference`, `jobs` and `wire`. A comment now carries its own reason
rather than a pointer to an issue a reader cannot open from the file.

Development narrative goes with the numbers — which slice added a field, what a
shape used to be, which run measured what — while the measurements themselves,
the invariants and the traps are kept and shortened. `src/visionset/formats/
lanes/README.md` is prose documentation and is deliberately untouched.

cf. #474
The same rule across `frontend/annotator`, `frontend/ui-core` and
`frontend/app`: a comment carries the reason itself rather than a number a
reader cannot open from the file. Where the prose already explained itself the
number is simply gone; where it did not, the reason was read out and written in.

Development narrative goes with the numbers — what a control used to be, which
slice moved it, which run found a defect — while the measurements, the
invariants and the traps are kept and shortened. A comment describing a defect
now describes the failure mode it guards against, in the present tense, which is
what a reader needs.

Test names are code rather than comments: `it(...)` and `describe(...)` strings
keep the references they carry, because changing one changes test output.

cf. #474
…h issue

The same rule over `tests/`, `scripts/` and `examples/`. A test docstring now
states the failure mode it pins — in the present tense, as a property of the
code — rather than naming the issue that found it, which a reader in an editor
cannot open.

Two things are deliberately left as they are, because neither is a comment: a
`def test_*` name or an `it(...)` string is code, and changing one changes test
output; and `scripts/export_mcp_tools.py`'s reference sits inside a string
literal that is published into `docs/mcp-tools.md`.

cf. #474
`docs/architecture/` explains the repository the way you would explain it to a new
senior engineer: an entry point with the whole system on one diagram, then a page
per layer, each with a Mermaid diagram GitHub renders natively.

Backend pages cover the kernel and its purity contract, the FastAPI adapter and
the committed OpenAPI contract, the CLI and MCP surfaces, the exporter plugin
system, the shared wire shapes, the background handlers and the inference
composition root. Frontend pages cover the headless annotator and its three
gates, `ui-core`, and the router shell. One cross-cutting page collects the two
machine-enforced boundaries, the capabilities contract and the three state
machines.

Every page links into the code and to the doc or skill that owns the behaviour
rather than restating it, because a restated rule goes stale silently. Every
claim was checked against HEAD — the import-linter contracts in `pyproject.toml`,
the entry-point group, each `package.json`, `tsconfig.core.json` and the
transition tables themselves.

`jobs.md` and `inference.md` are additions to the shape originally sketched:
both are real top-level packages carrying their own import contracts, and a tree
that skipped them would describe a distribution this is not.

cf. #474
FastAPI copies a route's docstring and a model's docstring verbatim into
`openapi.json` as the schema's `description`, so Part A's cleanup moved seven of
them. Both generated artifacts are regenerated from source rather than edited:
`scripts/export_openapi.py`, then `pnpm generate:client`.

The change is confined to description prose in both files — 15 insertions and 16
deletions in `api.ts`, every one of them inside a JSDoc comment, and no operation,
field, type or code touched. It is also the change worth having on a public
contract: a third-party client reading `description` could not open the issue
numbers those sentences used to cite.

cf. #474
`AGENTS.md` told agents which skill to read before writing an issue or a PR body
and said nothing about the thing they write far more often. Two rules, both
learned from the sweep this branch carries:

**Be brief.** One to three sentences for a rationale; a docstring says what a
thing does and why it exists, not how it came to be.

**Never explain with an issue number.** A bare `#160` is a pointer, and a reader
in an editor, a vendored copy or a fork cannot follow one — so it carries nothing.
Write what breaks without the code instead. A reference survives only where the
history is load-bearing and the comment already stands without it.

Two guards ride along, because both cost something here: a comment holding an
invariant is rewritten shorter and never deleted, and a pydantic or FastAPI
docstring is *published* — it reaches `openapi.json` and the generated client, so
it is written for the client and regenerated in the same change.

The local `CLAUDE.md` is a git-ignored copy of this file and was mirrored by hand.

cf. #474
@JArmandoAnaya
JArmandoAnaya merged commit 127bdf5 into main Aug 10, 2026
13 checks passed
@JArmandoAnaya
JArmandoAnaya deleted the chore/comment-cleanup-and-arch-docs branch August 10, 2026 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: comment cleanup and layered architecture docs

1 participant