docs: bring the design and testing pages in line with the shipped code - #111
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
A sweep of the docs against the code found statements that the implementation has overtaken: - capabilities-contract: pkg/verdict emits the refusal class on every refusal; the page said it did not yet. - testing: TM-8 is met by CI's demo job (make demo-check); the how-to-run table lists demo-check, check-capabilities, and the replay targets; the coverage table covers the executor, migrate, suggest, pull, capabilities, progress, and Supabase suites; Phase 3 obligations are recorded as done. - optimistic-attempt: a lock-timeout overrun is retried under the bounded RetryPolicy (three attempts by default, --lock-attempts 1 disables it); a statement-timeout overrun is never retried. The page said the form ran once. - cli-output-examples: create-name-mismatch and create-names-unverified are executor failure codes on a failed verdict (exit 1), not refusal reasons; the destructive example named DROP TABLE, which never reaches classification. - low-level-design and architecture: the package layout, the eight CLI commands, the executor's scope, and the plan report's versioning as shipped; the declarative front door is diff --desired, not a migrate flag. - SAFETY: internal/cli row lists pull and capabilities. - schemabot-integration: names schemadiff.ListManagedTables as the exported catalog query for undeclared-table enumeration. - capabilities, limitations, vision: the accepted-blocking primitive ships as a library call with exit 3 and no CLI flag yet, so "planned", "must run outside pg-sprite", and "every capability is reachable from the CLI" are qualified accordingly. Amp-Thread-ID: https://ampcode.com/threads/T-01a07fb2-9632-732b-bd7b-2f143a81bc06
…ADME The README's 'does not do yet' list now names the accepted-blocking passthrough as library-only, so the three capability pages move together. architecture.md no longer claims capabilities --json carries a format version: its version is the binary version, as the contract page says. The end-to-end diagrams in both design docs name all eight CLI verbs and the real migrate --alter / diff --desired entry points; the package map's pkg/executor row lists the greenfield create path and the passthrough primitive; and the 'contracts exist' list separates the two packages that hold only a package doc. Amp-Thread-ID: https://ampcode.com/threads/T-01a07fb2-9632-732b-bd7b-2f143a81bc06
d9c3ed7 to
8bbc411
Compare
|
🤖 Adversarial review 1/2 — are the corrections themselves correct? A docs PR whose thesis is "the docs assert things the code has overtaken" has to be reviewed the same way it was written: every claim checked against the code, not read for style. I checked all of them.
Every one holds. The Four findings. None of them is a wrong correction. 1. The same file's Exit inventory still carries the single-attempt framing this PR removedThe body's lead example is that That is the same statement, in the same file, in the form a reader is most likely to quote: a row in a typed inventory rather than a sentence in a narrative. With the retry documented above it, row 3a now under-describes its own exit — the lock was not granted within While in that table, 2. The cross-PR note is wrong about which files overlap, in the direction that mattersThe body says "This PR does not touch the passages #109 changes;
The good news, which the body could have claimed and did not: the two merge cleanly and the merged tree is green. I merged #109's current head with 3. The detector that let this drift in is one-directional, and the fix pattern landed in that same file this weekTwo executor codes lived in a table headed Refusal reasons — the table whose comment says "The set is closed and pinned by test ( for _, r := range Reasons() {
assert.Contains(t, doc, fmt.Sprintf("| `%s` |", string(r)), ...)
}Every reason must have a row; no row must be a reason. So the table can accumulate anything, and did. This PR removes the two symptoms and leaves the detector exactly as permissive — the third one lands the same way. The correction is a few lines and the pattern is already in the same file, added by #109: collect the doc's reason rows and 4. Three of the twelve corrections are mechanically pinnable, and this PR is the argument for pinning themThe sweep found a dozen statements that drifted silently over some months. That is the strongest possible evidence that prose-only synchronization does not hold, and the PR leaves the next dozen equally unprotected. Three corrections need only the AST-read-a-constant pattern
I would not ask for all three in this PR. But adding one — One edit changes a commitment rather than correcting a fact, and I think it is the right call. This review was generated by Claude Code (claude-opus-5). |
|
🤖 Review 2/2 — two lenses: OSS adoption, and integration ease for importers. Lens 1: OSS adoptionEvery one of these twelve corrections errs in the same direction, and it is the expensive one. The docs described a smaller product than ships: the accepted-blocking path "planned" when the primitive is exported, the bounded attempt single-shot when it retries, six commands when eight exist, the plan report unfrozen when The sharpest instance is Worth saying, because the twelve-item list makes it sound worse than it is: the doc set is in good shape. 512 relative links resolve. The two mislabeled executor codes were correct on five other pages and wrong on one, so the correction is bringing an outlier into line, not repairing a systemic error. The Lens 2: integration ease for schemabot and other orchestratorsThe single highest-value line in this PR for an importer is in The Two smaller additions that an orchestrator can act on immediately. One forward-looking note. This PR documents the library-first reality well, but the accounting for it lives in two hand-maintained places: the README's "does not do yet" list and the Approving. Every factual correction checks out against the code, the merge with #109 is clean and green, and the four findings in 1/2 are all follow-ups — the two I would take before merge are the Exit inventory row that still says the attempt runs once, and the one-directional refusal-reason test that let this class of drift in. This review was generated by Claude Code (claude-opus-5). |
aparajon
left a comment
aparajon
left a comment
There was a problem hiding this comment.
🤖 Approved. Every one of the twelve corrections checks out against the code — I verified each claim at its source rather than reading the diff for style: DefaultRetryPolicy's 3/100ms/1s and the flags that expose them, statement timeouts returning immediately, DROP TABLE reaching unsupported-statement through KindCatalogWork, the two create-* codes being executor.Code values rather than refusal reasons, destructiveOp's four ops with DEFAULT deliberately excluded, eight commands, --desired on diff, format_version 5, TM-8 met by CI's demo job, 512 relative links resolving, and make check-capabilities green. Four findings in the review comments, none blocking. The two worth a follow-up commit: the Exit inventory's row 3a in optimistic-attempt.md still says the lock was not granted "within lock_timeout", the same single-attempt framing this PR removed from the two prose sites 150 lines above; and the test that pins the refusal-reason table runs only one way — every reason must have a row, no row must be a reason — which is exactly how two executor codes sat in it, and the bidirectional pattern to fix it landed in that same file in #109. Also noted: five files overlap with #109, not the one the description claims, though I merged the two and the merged tree is green.
This stamp was left by Claude Code (claude-opus-5).
…n table both ways The Exit inventory's lock-budget row still described the bounded attempt as a single try, and the orchestrator table beside it told an operator to retry without saying the engine had already exhausted its own retries; both rows now carry the retry the prose above them describes. The test that pins the refusal-reason table checked only that every reason has a row, so a row for a token that is not a reason went unnoticed. It now compares the table's row set with verdict.Reasons() in both directions, the way the exit-code ladder test does. Amp-Thread-ID: https://ampcode.com/threads/T-01a07fb2-9632-732b-bd7b-2f143a81bc06
|
🤖 Delta re-review — The delta is a merge of The docs guards did run on it: So the four findings in my earlier review are all still open. Two of them changed shape when The detector finding is now twelve lines away from its own fixI argued that the refusal-reason pin only runs one way, so a table headed "the set is closed and pinned by test" can accumulate rows for things that are not reasons — which is how The merge brought the fix pattern into the same file.
One file, four pins on closed vocabularies, one of which is bidirectional. The cheapest version of this — collecting the doc's The body's cross-PR note is now a prediction about a merge that has already happenedThe body still says this PR "does not touch the passages #109 changes; Correcting my own finding:
|
aparajon
left a comment
aparajon
left a comment
There was a problem hiding this comment.
🤖 Re-approving on 9fe9f549. The delta is a pure auto-merge of main — the mechanically reproduced merge tree is byte-identical to the commit's — and the docs guards pass over the merged tables. Findings are in the delta re-review above; the one I would take before merge is the set-equality pin, whose bidirectional twin now sits twelve lines away in the same file.
This stamp was left by Claude Code (claude-opus-5).
The refusal-class map and the class and owner tables were checked only for missing rows, so a row for a value the contract does not carry would have gone unnoticed. Each is now compared with its constant set as a whole, sharing one table reader so the checks in this file take the same shape. docs/progress-report.md states a current format_version that no test kept honest; it now has the same pin as the plan and suggest reports. Amp-Thread-ID: https://ampcode.com/threads/T-01a07fb2-9632-732b-bd7b-2f143a81bc06
|
🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude) — block/pg-sprite pull/111, follow-up commit All four findings from the first pass, as reshaped by the delta re-review, are fixed: two landed in the already-pushed
Source: block/pg-sprite#111, review comments 5644388162 and 5644388493 and 5652294015 and reviews 5185711002 and 5190262735 at head |
2 participants
Why
A sweep of the docs against the code on
mainfound a dozen statements the implementation has overtaken. Each one tells a reader something false about what ships: that the refusalclassis not emitted yet, that CI does not run the built binary, that the optimistic attempt runs the statement once, that the CLI has six commands, that the plan report has not frozen its version, and that the accepted-blocking path is only planned. A reader who trusts the docs plans against a smaller product than the one they have.What
Eleven pages change; no code does. The commit message lists every correction by file. The ones that change what a user would do:
docs/optimistic-attempt.md: a lock-timeout overrun is retried under the boundedRetryPolicy(three attempts by default;--lock-attempts 1disables it), each attempt in a fresh transaction; a statement-timeout overrun is never retried. The page said "runs the submitted form once".docs/cli-output-examples.md:create-name-mismatchandcreate-names-unverifiedmove out of the refusal-reasons table into a note; they are executor failure codes on afailedverdict (exit 1) because theCREATE TABLEhas already committed. Thedestructiveexample no longer namesDROP TABLE, which never reaches classification and refuses asunsupported-statement.docs/testing.md: TM-8 is met by CI'sdemojob; the how-to-run table gainsdemo-check,check-capabilities, and thereplay*targets; the coverage table gains rows for the executor,migrate,suggest,pull, capabilities, progress, and Supabase suites; the Phase 3 obligations row is "Done".docs/capabilities.md,docs/limitations.md,docs/vision.md: the accepted-blocking path is a shipped library primitive (executor.ExecuteAcceptedBlocking,outcome: executed-without-online-safety, exit 3) with no CLI flag yet, so "planned", "must run outside pg-sprite", and "every capability is reachable from the CLI" are each qualified.docs/architecture.md,docs/low-level-design.md,SAFETY.md: eight CLI commands, the package layout as it exists, the plan report as a versioned contract (format_version5), anddiff --desiredrather than a--desiredflag onmigrate.How
Every correction was checked against the code before editing:
pkg/verdict/verdict.go(WithRefusal,WithAcceptedBlocking),pkg/migrate/refusal_registry.go(DROP TABLEisKindCatalogWork→unsupported-statement),pkg/executor/optimistic.go(DefaultRetryPolicy, "Statement timeouts and all other failures return immediately"),internal/cli/cli.go(command list and--lock-*flags),pkg/plan/plan.go(FormatVersion = 5),.github/workflows/ci.yml(make demo-check), and the test files each new coverage row links to. In the coverage table I left outpkg/checkpoint: its tests today cover onlyPhasestring and terminal-state helpers, which is too little to list as coverage of "durable resume".The hand-written region of
docs/capabilities.mdis the only part touched there;make check-capabilitiespasses.Risk
Low. Documentation only.
Testing
Beyond CI: a relative-link check over
README.md,SAFETY.md, anddocs/*.md(rg -o '\]\(([^)#h][^)]*)\)' --replace '$1', fragments stripped, each target tested with-e) reports no broken links.Bigger picture
Two related docs PRs are open from the same base: #109 surfaces the exit-code ladder in the README, and #110 splits
pkg/executor/native.gointocic_*files. This PR does not touch the passages #109 changes;docs/low-level-design.mdis edited by both but in different sections.Generated with Amp (Claude)