Skip to content

feat(protocol): generate the protocol documentation from the schemas - #43

Closed
Willi363363 wants to merge 1 commit into
feat/rewrite-phase-1-roundfrom
feat/rewrite-phase-1-protocol-docs
Closed

Willi363363 wants to merge 1 commit into
feat/rewrite-phase-1-roundfrom
feat/rewrite-phase-1-protocol-docs

Conversation

@Willi363363

Copy link
Copy Markdown
Owner

Step 1.10 of plans/rewrite/phase-01-steps-protocol.md — the last step of
phase 1.

Stacked on #42 (step 1.9). Base is feat/rewrite-phase-1-round, so the diff here
is step 1.10 alone.

C8.2 — the guarantee that had to be rebuilt or vanish

test_architecture_doc.py holds this line today with regexes over
hand-written Markdown. The documentation can drift and be corrected
afterwards, and the mechanism dies with the Python.

Generated, it cannot describe a protocol that does not exist. The only way to
change the document is to change the contract.

Four pages under plans/protocol/:

page lines contents
README.md 59 index, the 15 error codes, the 13 item ids
websocket-client.md 83 the 13 messages a client sends
websocket-server.md 134 the 15 the server sends
rest.md 161 the 9 routes, request and response

JSON Schema, not Zod internals

z.toJSONSchema is a supported surface; .def is not. Reading the supported
one means a Zod upgrade cannot silently reshape the output.

Verified biting — adding a field to live_score:

× matches what is committed at protocol/websocket-client.md
  → Snapshot mismatched

The lock and the generator are one code path

toMatchFileSnapshot, so regenerating is the same comparison writing instead of
failing:

pnpm --filter @wikifake/protocol docs

No new dependency, and no second code path that could produce a different
document from the one the test checks.

What the pages are asserted to keep

  • Under 200 lines. A generated file does not get to ignore the repository's
    documentation limit: a message added to the protocol has to fit, or the pages
    get split again.
  • A single trailing newline, which the hygiene check requires.

Two details worth the diff

cursorCoordinate gained a .describe(). A transform has no JSON Schema, so the
one field whose tolerance is the interesting part would have read unknown.
It now reads number clamped to [0,1]; anything else becomes 0.

hint_unlocked.grant renders as its two shapes rather than a flat type, because
that difference is the contract — the level-2 truth cannot exist at level 1:

- `grant` — one of 2 shapes
  - shape 1
    - `level` — `1`
  - shape 2
    - `level` — `2`
    - `truth` — string (min 1 char)
    - `paragraphIndex` — integer (≥ 1)

The phase exit gate, checked

Since this closes the phase, purity.test.ts checks the gate's load-bearing
line: "no access to the clock, the network or the disk in domain".

No rule file reads a clock, draws a number, schedules a timer, reaches the
network, or imports a Node built-in. Nothing breaks when that is violated — a
Date.now() in a reducer just makes a timeout test take five minutes and phase 5
unable to move the timer onto BullMQ — which is precisely why it needs a test.

Comments are stripped before searching. My first version flagged three files:
scoring.ts, room/topics.ts and room/events.ts, all of which mention
Date.now() or Math.random() in order to explain why they are absent. A
purity check that punishes the files that documented the rule is worse than
none.

Verified biting: Date.now() * 0 added to timeBonusFor fails it.

The gate, point by point

gate evidence
C2 and C3.3 cases as pure unit tests 44 tests in scoring.test.ts + grading.test.ts
reducer covered transition by transition, the two missing ones included 84 tests under src/room/
no clock, network or disk in domain purity.test.ts
build && test && lint && typecheck green
pnpm test         domain 233 · protocol 220 · config 16 · env 7
pnpm typecheck    4 packages
pnpm lint         4 packages
pnpm build        nothing to build — source-level exports
pnpm format:check
bash scripts/checks.sh staged

Not run: the Python backend tests — no pytest here. This change touches no
Python; CI covers them.

Phase 1 is complete

All ten steps ticked. I left the phase's state at in progress in
plans/README.md deliberately: the exit gate is passed, but nothing is merged
yet — twelve pull requests are waiting on the revu label. Flipping it belongs
to whoever lands feat/rewrite-phase-1 into staging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01533WTTbgmfCaDP5tmK8Gkv

Step 1.10 of phase 1, and the last one. Four pages under `plans/protocol/`,
produced from the Zod schemas and compared to what is committed.

C8.2 — this is the guarantee that had to be reimplemented or disappear
without a sound. `test_architecture_doc.py` holds the line today with
regexes over hand-written Markdown, so the documentation can drift and be
corrected afterwards. Generated, it cannot describe a protocol that does not
exist: the only way to change the document is to change the contract.

The generator reads JSON Schema rather than Zod internals. `z.toJSONSchema`
is a supported surface and `.def` is not, so a Zod upgrade cannot silently
reshape the output. Verified biting: adding a field to `live_score` fails the
comparison.

The lock is `toMatchFileSnapshot`, so regenerating is the same comparison
writing instead of failing — `pnpm --filter @wikifake/protocol docs`. No new
dependency, and no second code path that could produce a different document
from the one the test checks.

Two things the pages are asserted to keep: the 200-line documentation limit,
which a generated file does not get to ignore, and a single trailing newline,
which the repository's hygiene check requires.

`cursorCoordinate` gained a `.describe()`: a transform has no JSON Schema, so
the one field whose tolerance is the interesting part would have read
`unknown`.

The phase exit gate is checked too, in `purity.test.ts`: no rule file reads a
clock, draws a number, schedules a timer, reaches the network or imports a
Node built-in. That guarantee is what the whole package rests on and nothing
breaks when it is violated — a `Date.now()` in a reducer just makes a
timeout test take five minutes. Comments are stripped before searching, or
the files that took the trouble to explain the rule would be the ones
flagged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01533WTTbgmfCaDP5tmK8Gkv
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Generate protocol docs from Zod schemas and lock them with snapshots

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Generate four protocol Markdown pages directly from Zod schemas via JSON Schema.
• Lock docs to committed files with Vitest snapshots and enforce repo doc hygiene rules.
• Add a purity gate preventing clock/network/disk access inside domain rules.
Diagram

graph TD
  ZodSchemas["Protocol Zod schemas"] --> Render["render.ts (JSON Schema -> Markdown)"] --> Pages["pages.ts (build pages)"] --> DocsTest["docs.test.ts (snapshot lock)"] --> PlansDocs[/"plans/protocol/*.md"/]
  PkgJson["protocol package.json (docs script)"] --> DocsTest
  DomainRules["domain/src/**/*.ts rules"] --> PurityTest["purity.test.ts (forbidden effects)"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Generate OpenAPI + docs site (zod-to-openapi/Redoc)
  • ➕ Standardized schema/documentation toolchain
  • ➕ Better ecosystem support for REST consumers
  • ➖ Adds dependencies and configuration surface
  • ➖ Doesn’t naturally cover WebSocket message unions the same way
  • ➖ May produce verbose output that breaks the 200-line/page constraint
2. Render directly from Zod AST (.def)
  • ➕ Richer access to Zod-specific constructs (transforms, refinements)
  • ➕ Potentially more precise output without needing describe()
  • ➖ Couples docs to Zod internals; upgrades can silently change output
  • ➖ Harder to keep stable/reviewable across versions
3. Keep handwritten Markdown + regex lock
  • ➕ No generator code to maintain
  • ➕ Easy to tweak prose formatting
  • ➖ Docs can drift and be “fixed later” without contract change
  • ➖ Lock is language/tooling-specific and brittle over time

Recommendation: Keep the PR’s approach (JSON Schema-based generation + file snapshots). It minimizes dependency risk, keeps the documentation mechanically tied to the contract, and makes regeneration the same code path as verification. The small manual additions (e.g., .describe() on transforms) are an acceptable, explicit escape hatch for schema features that do not serialize to JSON Schema.

Files changed (13) +923 / -4

Enhancement (3) +330 / -1
pages.tsGenerate four protocol Markdown pages from Zod schema catalogues +129/-0

Generate four protocol Markdown pages from Zod schema catalogues

• Implements page assembly for protocol index, WebSocket client/server messages, and REST routes. Adds a generated-file warning header and ensures output is trimmed and ends with a single newline, delegating field rendering to describeSchema().

packages/protocol/src/docs/pages.ts

render.tsRender JSON Schema from Zod into Markdown field descriptions +197/-0

Render JSON Schema from Zod into Markdown field descriptions

• Adds a JSON Schema-driven renderer (via z.toJSONSchema) that produces one-line type descriptions plus nested object field bullet lists. Handles bounds/patterns/defaults, records, arrays, and renders discriminated unions of object variants as multiple "shapes" when that distinction is part of the contract.

packages/protocol/src/docs/render.ts

primitives.tsDescribe cursorCoordinate transform for generated docs +4/-1

Describe cursorCoordinate transform for generated docs

• Adds a .describe() to cursorCoordinate so generated documentation reflects the clamp/tolerance semantics (since transforms do not emit JSON Schema). Keeps the runtime behavior the same while improving documentation fidelity.

packages/protocol/src/primitives.ts

Tests (3) +148 / -1
purity.test.tsAdd domain purity gate to forbid effects in rule sources +83/-0

Add domain purity gate to forbid effects in rule sources

• Introduces a Vitest suite that enumerates non-test domain .ts files (excluding a small harness allowlist) and fails if forbidden patterns are present. Strips block comments and full-line // comments to avoid flagging explanatory prose while still catching executable usage of Node built-ins, time, randomness, timers, fetch, or process.env.

packages/domain/src/purity.test.ts

docs.test.tsSnapshot-lock generated protocol documentation to committed plans files +57/-0

Snapshot-lock generated protocol documentation to committed plans files

• Adds a test that generates the four protocol pages, asserts the expected page set, and matches each page against a file snapshot under plans/. Also enforces repository constraints: max 200 lines per doc and exactly one trailing newline.

packages/protocol/src/docs/docs.test.ts

inferred-types.test.tsExclude docs generator files from inferred-types contract assertions +8/-1

Exclude docs generator files from inferred-types contract assertions

• Extends NOT_CONTRACTS to ignore docs/render.ts and docs/pages.ts so the inferred-types test continues to target protocol contracts rather than documentation tooling.

packages/protocol/src/inferred-types.test.ts

Documentation (6) +443 / -1
README.mdDocument generated protocol docs location and regeneration workflow +5/-0

Document generated protocol docs location and regeneration workflow

• Adds the protocol docs entry to the plans index and clarifies that plans/protocol is generated from packages/protocol. Documents the regeneration command and notes that CI will catch hand edits via the snapshot lock.

plans/README.md

README.mdAdd generated protocol index page (errors, item ids, page map) +59/-0

Add generated protocol index page (errors, item ids, page map)

• Introduces the generated index page listing the protocol sub-pages plus the closed unions of error codes and item identifiers, with a generator warning header.

plans/protocol/README.md

rest.mdAdd generated REST routes and payload documentation +161/-0

Add generated REST routes and payload documentation

• Adds generated REST documentation listing nine routes with request/response field bullet lists and constraints, including union-shape rendering for the hint grant payload.

plans/protocol/rest.md

websocket-client.mdAdd generated WebSocket client-to-server message documentation +83/-0

Add generated WebSocket client-to-server message documentation

• Adds generated documentation for the thirteen client-sent messages, rendering each message’s schema fields and constraints. Includes cursor coordinate tolerance text from schema descriptions.

plans/protocol/websocket-client.md

websocket-server.mdAdd generated WebSocket server-to-client message documentation +134/-0

Add generated WebSocket server-to-client message documentation

• Adds generated documentation for the fifteen server-sent messages, including nested field shapes and union variants where contractually significant (e.g., hint_unlocked.grant).

plans/protocol/websocket-server.md

phase-01-core.mdMark phase 1.10 (generated protocol documentation) as done +1/-1

Mark phase 1.10 (generated protocol documentation) as done

• Updates the phase checklist to indicate the generated protocol documentation step is completed.

plans/rewrite/phase-01-core.md

Other (1) +2 / -1
package.jsonAdd docs script for regenerating protocol docs snapshots +2/-1

Add docs script for regenerating protocol docs snapshots

• Adds a "docs" npm script that runs the docs snapshot tests in update mode (vitest -u) to regenerate committed protocol documentation. Keeps existing test script intact with trailing comma adjustment.

packages/protocol/package.json

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Wrong PLANS path build 🐞 Bug ≡ Correctness
Description
packages/protocol/src/docs/docs.test.ts derives a filesystem path using URL.pathname, which
preserves percent-encoding and is not a cross-platform path; this can make snapshots fail to locate
files when the repo path contains spaces or on Windows. Use fileURLToPath (and path.join) to create
a valid absolute path before passing it to toMatchFileSnapshot.
Code

packages/protocol/src/docs/docs.test.ts[19]

+const PLANS = new URL('../../../../plans/', import.meta.url).pathname;
Evidence
The test currently uses URL.pathname as a filesystem path; Node documents that pathname
percent-encodes characters and that fileURLToPath is the supported conversion to a platform-valid
absolute path. The repo already uses fileURLToPath for similar ESM directory calculations elsewhere.

packages/protocol/src/docs/docs.test.ts[15-20]
packages/domain/src/purity.test.ts[14-19]
🌐 Node docs state that URL.pathname percent-encodes invalid URL characters, and that url.fileURLToPath() ensures correct decoding of percent-encoded characters and returns a cross-platform valid absolute path string.

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`docs.test.ts` constructs `PLANS` using `new URL(...).pathname`, but `pathname` can be percent-encoded and is not guaranteed to be a valid platform path string. This can break file snapshot matching when directories contain spaces or on Windows.

### Issue Context
The snapshot matcher expects a real filesystem path. Node’s `fileURLToPath()` exists specifically to decode percent-encoded characters and return a platform-correct absolute path.

### Fix Focus Areas
- packages/protocol/src/docs/docs.test.ts[15-20]

### Suggested change
- Replace `const PLANS = new URL(...).pathname` with something like:
 - `import { fileURLToPath } from 'node:url';`
 - `import path from 'node:path';`
 - `const PLANS_DIR = fileURLToPath(new URL('../../../../plans/', import.meta.url));`
 - In the test: `toMatchFileSnapshot(path.join(PLANS_DIR, pathFromGenerated))`
- Avoid string concatenation for paths to prevent mixed separators (`C:\...` + `protocol/...`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Purity test builtin bypass 🐞 Bug ☼ Reliability
Description
packages/domain/src/purity.test.ts attempts to forbid Node built-ins but only checks for the
substring "from 'node:"; imports like import fs from 'fs', from "node:fs", dynamic
import('node:fs'), or require('fs') are not caught. This weakens the test’s stated guarantee
(“no disk/network/clock in domain”) by allowing straightforward false negatives.
Code

packages/domain/src/purity.test.ts[R58-60]

+const FORBIDDEN: readonly (readonly [RegExp, string])[] = [
+  [/\bfrom 'node:/, 'imports a Node built-in: the rules must run anywhere'],
+  [/\bDate\.now\(\)/, 'reads the clock: time is a parameter'],
Evidence
The new forbidden-pattern list explicitly claims to block Node built-in imports, but the implemented
regex only matches one specific textual form and does not cover other standard import forms. The
file’s own header describes the guarantee as excluding disk/network access, which Node built-ins
enable.

packages/domain/src/purity.test.ts[1-10]
packages/domain/src/purity.test.ts[57-61]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new “purity” gate claims to prevent importing Node built-ins, but its regex only matches `from 'node:`. That misses common/bypassing forms (`'fs'`, double quotes, dynamic import, require), so the test can pass while a rule actually depends on Node.

### Issue Context
This test is intended as an exit gate for the package’s purity guarantee. If it misses easy cases, it provides a false sense of safety.

### Fix Focus Areas
- packages/domain/src/purity.test.ts[57-68]

### Suggested change
Preferred (robust):
- In the test (not in domain code), import `builtinModules` from `node:module`.
- Extract module specifiers from the source via a small parser approach (TypeScript AST via `typescript` if already available, or a conservative regex that matches both `import ... from 'x'` and `import('x')` / `require('x')`).
- Fail if any specifier is in `builtinModules` or starts with `node:`.

Minimal regex improvement (less robust):
- Replace `/\bfrom 'node:/` with patterns that catch:
 - `from ['"]node:`
 - `from ['"]<builtin>['"]` (at least `fs`, `path`, `os`, `http`, `https`, `crypto`, etc.)
 - `\bimport\(['"]node:` and `\brequire\(['"]` for builtins.

Keep comment-stripping if you want, but the import check should not depend on the presence of the literal substring `from 'node:`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Web pages:
  +2 more
Review mode: ⚖️ Balanced

Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo


import { pages } from './pages.js';

const PLANS = new URL('../../../../plans/', import.meta.url).pathname;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Wrong plans path build 🐞 Bug ≡ Correctness

packages/protocol/src/docs/docs.test.ts derives a filesystem path using URL.pathname, which
preserves percent-encoding and is not a cross-platform path; this can make snapshots fail to locate
files when the repo path contains spaces or on Windows. Use fileURLToPath (and path.join) to create
a valid absolute path before passing it to toMatchFileSnapshot.
Agent Prompt
### Issue description
`docs.test.ts` constructs `PLANS` using `new URL(...).pathname`, but `pathname` can be percent-encoded and is not guaranteed to be a valid platform path string. This can break file snapshot matching when directories contain spaces or on Windows.

### Issue Context
The snapshot matcher expects a real filesystem path. Node’s `fileURLToPath()` exists specifically to decode percent-encoded characters and return a platform-correct absolute path.

### Fix Focus Areas
- packages/protocol/src/docs/docs.test.ts[15-20]

### Suggested change
- Replace `const PLANS = new URL(...).pathname` with something like:
  - `import { fileURLToPath } from 'node:url';`
  - `import path from 'node:path';`
  - `const PLANS_DIR = fileURLToPath(new URL('../../../../plans/', import.meta.url));`
  - In the test: `toMatchFileSnapshot(path.join(PLANS_DIR, pathFromGenerated))`
- Avoid string concatenation for paths to prevent mixed separators (`C:\...` + `protocol/...`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +58 to +60
const FORBIDDEN: readonly (readonly [RegExp, string])[] = [
[/\bfrom 'node:/, 'imports a Node built-in: the rules must run anywhere'],
[/\bDate\.now\(\)/, 'reads the clock: time is a parameter'],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Purity test builtin bypass 🐞 Bug ☼ Reliability

packages/domain/src/purity.test.ts attempts to forbid Node built-ins but only checks for the
substring "from 'node:"; imports like import fs from 'fs', from "node:fs", dynamic
import('node:fs'), or require('fs') are not caught. This weakens the test’s stated guarantee
(“no disk/network/clock in domain”) by allowing straightforward false negatives.
Agent Prompt
### Issue description
The new “purity” gate claims to prevent importing Node built-ins, but its regex only matches `from 'node:`. That misses common/bypassing forms (`'fs'`, double quotes, dynamic import, require), so the test can pass while a rule actually depends on Node.

### Issue Context
This test is intended as an exit gate for the package’s purity guarantee. If it misses easy cases, it provides a false sense of safety.

### Fix Focus Areas
- packages/domain/src/purity.test.ts[57-68]

### Suggested change
Preferred (robust):
- In the test (not in domain code), import `builtinModules` from `node:module`.
- Extract module specifiers from the source via a small parser approach (TypeScript AST via `typescript` if already available, or a conservative regex that matches both `import ... from 'x'` and `import('x')` / `require('x')`).
- Fail if any specifier is in `builtinModules` or starts with `node:`.

Minimal regex improvement (less robust):
- Replace `/\bfrom 'node:/` with patterns that catch:
  - `from ['"]node:`
  - `from ['"]<builtin>['"]` (at least `fs`, `path`, `os`, `http`, `https`, `crypto`, etc.)
  - `\bimport\(['"]node:` and `\brequire\(['"]` for builtins.

Keep comment-stripping if you want, but the import check should not depend on the presence of the literal substring `from 'node:`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@Willi363363

Copy link
Copy Markdown
Owner Author

Closed by the stack collapse, not abandoned.

Every commit of this pull request is in willi363/refonte via #114, which
merged the whole rewrite in one go: the stack was strictly linear, so the top
branch was an ancestor-of-nothing and a descendant-of-everything, and the merge
was a fast-forward with no conflict possible. The history keeps one commit per
step, which is what squash-per-step was there to produce.

Three commit messages were reworded on the way — a wip: type, a 74-character
subject and a (web,realtime) scope, none of which scripts/checks.sh allows.
The trees are byte-identical.

This description and its review thread stay readable here.

@Willi363363
Willi363363 deleted the feat/rewrite-phase-1-protocol-docs branch August 29, 2026 16:10
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.

1 participant