Repository navigation
feat(protocol): generate the protocol documentation from the schemas - #43
Willi363363 wants to merge 1 commit into
Conversation
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
PR Summary by QodoGenerate protocol docs from Zod schemas and lock them with snapshots
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1. Wrong PLANS path build
|
|
|
||
| import { pages } from './pages.js'; | ||
|
|
||
| const PLANS = new URL('../../../../plans/', import.meta.url).pathname; |
There was a problem hiding this comment.
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
| 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'], |
There was a problem hiding this comment.
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
|
Closed by the stack collapse, not abandoned. Every commit of this pull request is in Three commit messages were reworded on the way — a This description and its review thread stay readable here. |
Step 1.10 of
plans/rewrite/phase-01-steps-protocol.md— the last step ofphase 1.
Stacked on #42 (step 1.9). Base is
feat/rewrite-phase-1-round, so the diff hereis step 1.10 alone.
C8.2 — the guarantee that had to be rebuilt or vanish
test_architecture_doc.pyholds this line today with regexes overhand-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/:README.mdwebsocket-client.mdwebsocket-server.mdrest.mdJSON Schema, not Zod internals
z.toJSONSchemais a supported surface;.defis not. Reading the supportedone means a Zod upgrade cannot silently reshape the output.
Verified biting — adding a field to
live_score:The lock and the generator are one code path
toMatchFileSnapshot, so regenerating is the same comparison writing instead offailing:
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
documentation limit: a message added to the protocol has to fit, or the pages
get split again.
Two details worth the diff
cursorCoordinategained a.describe(). A transform has no JSON Schema, so theone 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.grantrenders as its two shapes rather than a flat type, becausethat difference is the contract — the level-2 truth cannot exist at level 1:
The phase exit gate, checked
Since this closes the phase,
purity.test.tschecks the gate's load-bearingline: "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 5unable 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.tsandroom/events.ts, all of which mentionDate.now()orMath.random()in order to explain why they are absent. Apurity check that punishes the files that documented the rule is worse than
none.
Verified biting:
Date.now() * 0added totimeBonusForfails it.The gate, point by point
scoring.test.ts+grading.test.tssrc/room/domainpurity.test.tsbuild && test && lint && typecheckNot run: the Python backend tests — no
pytesthere. This change touches noPython; CI covers them.
Phase 1 is complete
All ten steps ticked. I left the phase's state at in progress in
plans/README.mddeliberately: the exit gate is passed, but nothing is mergedyet — twelve pull requests are waiting on the
revulabel. Flipping it belongsto whoever lands
feat/rewrite-phase-1intostaging.🤖 Generated with Claude Code
https://claude.ai/code/session_01533WTTbgmfCaDP5tmK8Gkv