GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add the review agent's prompt, answer schema and lane D command (#201 part 1) by mchwang · Pull Request #208 · codeabovelab/codeboost Conversation
|
| Item | Where |
|---|---|
| Rounds stored before fixes; crash and restart resume; fix commits attributed; the 3-round limit goes to needs human | Part 2 (stacked PR) |
| Recorded Claude output for the review agent replays through validation | test/review-recorded.test.ts. Blocked: the recordings are not committed yet. The user records them with their own token (below). Until then this test fails, so the test and real-docker checks are red. |
| Real-Docker gate passes | test/agent-review.test.ts and the existing Docker tests. CI result below. |
Recording (needs the user)
CODEBOOST_RUN_AUTH_PROBES=1 CLAUDE_CODE_OAUTH_TOKEN=... node scripts/record-review.ts
The script writes test/fixtures/review/recorded/claude-review-round1.json and claude-review-round2.json. They then need committing. Nothing from Claude's output has been verified yet. The first recording also shows whether Claude Code's --json-schema accepts every keyword the schema uses (pattern, the length limits, string-or-null).
Known limit and follow-up
The plan schema allows a plan too large to review in one prompt, even without its change texts: 30 items, each with 10 checks of up to 400 characters. prepareReview refuses such a plan, and part 2 then sends the task to needs human after its items have run. Follow-up #209 tracks it: either review such a plan in batches of items, or check before its items run that it can be reviewed.
Follow-up #210 moves the planning and review prompts from a command argument to stdin, as AGENTS.md asks; today the budget keeps them under the OS argument limit.
Screen files
This PR touches no screen files (web/public/*, web/server.ts). Part 2, not yet opened, changes about 25 lines of web/public/app.js and about 28 lines of web/server.ts. #200 is changing the same files.
Validation (head 7a71286, the last fully validated head; later heads are validated again before the next Copilot request)
Each gate is reported separately.
| Gate | Result |
|---|---|
| Focused suites (review-prompt, agent-policy, agent-adapter, agent-adapter-decode, agent-adapter-cleanup, agent-launch, planning-provider, vitest-config) | Pass. review-recorded fails only its "has a Claude recording for each round" check, as expected. |
| Full local suite (non-Docker) | 2,916 passed, 1 expected fail, 4 skipped, 2 failed (97 files, 3,770 s, load average about 100). The failures were review-recorded (expected: no recordings yet) and a 15 s timeout in pre-merge.test.ts > "keeps waiting while GitHub still reports the pre-push head…". That file is untouched by this PR, and it passes alone (112/112). Recorded as machine load, not a product failure. |
Type check (tsc --noEmit) |
Pass |
test/agent-review.test.ts on the real Docker daemon |
Pass on 7a71286 (1/1) |
CI test / real-docker |
Expected red until the recordings are committed (see above). |
Review record
Base 0913c8f throughout. Each round was a fresh independent agent that reviewed the full diff. It had not written the change.
| Round | Head reviewed | Findings | Fixed in |
|---|---|---|---|
| 1 | ccdc726 |
Answers could hold NUL or unpaired surrogates; problems could be too large for the next prompt; check text was kept on the wrong items; schema errors named planning; the Docker test was missing from the scheduling test | 80a0538 |
| 2 | 80a0538 |
The room reserved for the next round was measured on the wrong prompt; blank summaries were accepted; boundary arithmetic was untested; T9 rows were out of date | 4dced6a |
| 3 | 4dced6a |
The changed-file list made the reservation inexact; round 1 accepted earlier problems; the host-root prefix was shared | ac392ac |
| 4 | ac392ac |
The 12 KiB answer budget refused schema-valid answers | 1a7c82e |
| 5 | 1a7c82e |
Large prompts were refused instead of leaving parts out; duplicate problems; a Claude review with approved argv was accepted | 70b275c |
| 6 | 70b275c |
The order of what is left out; non-size refusals were hidden as "did not fit"; no cap on the problem count | 5c7aae7 |
| 7 | 5c7aae7 |
Round 2 could refuse a plan round 1 accepted; text was refused only where it fitted; the start call's decoding was untested | bb5716f |
| 8 | bb5716f |
A note could push out the part it names; a blank-line gap; the check-dispatch vendor in a test | eacb92c |
| 9 | eacb92c |
Note room changed between rounds; the default round limit was undocumented | 9f46ebc |
| 10 | 9f46ebc |
The start call's decoder was untested; ordering and NUL tests were missing; the comment-count reserve | 596d8c3 |
| 11 | 596d8c3 |
Over-reservation; a size-only refusal was untested; fix-phase decoding was untested | bd411f1 |
| 12 | bd411f1 |
No reserve for the description and lessons notes; the zero-listed earlier note had no heading; tests used padding that is not schema-valid | 1d5b061 |
| 13 | 1d5b061 |
The decoder fakes did not name the Claude Code version they copy | 794ddb7 |
| 14 | 794ddb7 |
The round-1 reserve for the description and lessons was untested; a later round could be refused for room it only reserved | f7f8167 |
| 15 | f7f8167 |
The round-1 refusal did not mention the room it keeps | 8be6f12 |
| 16 | 8be6f12 |
The 1 MiB answer limit was untested | 7a71286 |
| 17 | 7a71286 |
No findings (clean) | none |
| Copilot 1 | 7a71286 |
Validation read the caller's round instead of the prepared request's (fixed). Missing recordings keep CI red (open, waiting for the user's recording) | f10f918 |
| 18 | f10f918 |
A comment or a longer title added between rounds could still get a later round refused, so the PR's claim was false; the test padding built plans with empty change texts, which the schema refuses | 0a9e500 |
| 19 | 0a9e500 |
The PR body overstated what the Docker test proves; the lesson audit left out the latest findings; some test inputs in the cross-round tests were issues the loader cannot give (empty title, over 32 KiB) | ea90109 and this body |
| 20 | ea90109 |
The round 19 fix was incomplete: the order and comment-note tests still used issues the loader refuses | 282555f: every test issue has a title and fits in 32 KiB; the two blocks over 32 KiB that remain are marked as inputs the loader refuses |
| 21 | 282555f |
The lesson audit did not classify every finding; tests used earlier problems no answer can give and a lesson over 32 KiB; the title room relies on GitHub's limit, not the loader's; the oversized-plan follow-up had no issue | dd3ef41, #209, and this body |
| 22 | dd3ef41 |
The review prompt goes to Claude as a command argument, against the AGENTS.md stdin rule, and the audit did not record it | 01fb52f (docs), #210 |
| 23 | 01fb52f |
Two audit rows used classes the merge gate does not allow; the review turn-limit and sign-in fakes named no CLI version | e8078dd: built from Claude Code 2.1.281's recorded envelope; audit rows reclassified |
| 24 | e8078dd |
The review agent could not see what the plan changed, so it could not catch an unrelated edit in a declared file, as the design expects (medium; the user chose to fix it in part 1). The recording script left its temporary file when a write failed | 4890ba8 |
| 25 | 4890ba8 |
An in-tree .gitattributes the agent writes could hide its diffs; Git's prefix match put a directory's files into the diff of a file it replaced; diffs were read for every changed file with no bound; --ignore-submodules=none went against AGENTS.md |
cb5d178 (asynchronous, stoppable), a4144f5 |
| 26 | a4144f5 |
A file whose type changed showed only its old side; renames cost double and hid edits in renamed files over 32 KiB; a stop rejected before Git exited | 79764d0 |
| 27 | 79764d0 |
A rename between a file and a path under it hid the new content; the read bound counted only shown diffs; one large early diff pushed every later diff out of the prompt; the stand-in Git named no version and waited 1 s | 6dead1a |
| 28 | 6dead1a |
A rename into a directory of its own name still picked up other changed files there (and the new rule had the same gap); the read-bound comment said a later file can never be listed | dad493a |
| 29 | dad493a |
A leading byte-order mark was dropped from names, so a file could be listed and diffed as another; named excludes were capped by count, not bytes (E2BIG); building them scanned every path up front |
b932fa7 |
| 30 | b932fa7 |
On macOS the repository's core.precomposeunicode made a decomposed name's pathspec miss it (an empty or another file's diff); round 29's audit row cited the stdin rule as covering an exception to it; a type comment gave an old reason for a null diff |
a09d05e and this body |
| 31 | a09d05e |
Past Git's rename limit, renames silently became a delete and an add (a warning only on stderr), hiding a large renamed file's edits again; the architecture row did not mention the diffs or runner/review-diffs.ts |
5e44ff2 |
| Self-review | 5e44ff2 |
The stderr check kept only the first 8 KiB, so enough harmless hints could push the rename-skip warning out | 0cd1c9c: stderr is searched as it arrives, with carry-over between chunks; tested with 20,000 hints and a split warning |
| 32 | 0cd1c9c |
The PR body did not record 0cd1c9c; the reviewDiffs comment said no in-tree diff attributes apply, but a built-in driver attribute still changes hunk headers |
022798f and this body |
| 33 | 022798f |
A renamed or deleted file with a null diff lost what changed (the old path appeared nowhere; a deleted file was to be "read"); the body described part 2's PR as open; the recording script could delete a temporary file it did not create | 47bc62d and this body |
| 34 | 47bc62d |
A file turned into a submodule entry was described as left out but is listed as a type change with only the file's side; the rename-skip message and docs described Git's rule as "more than 1,000 adds and deletes" | fb3f536 and this body |
| 35 | fb3f536 |
The fixture said P2's new test cannot catch its bug; it can when the whole file runs, and what is wrong is its unmet check | f97854b |
| Recording 1 | fb3f536 (Claude Code 2.1.281, by the user) |
Round 1 found the planted isLabelled() edit but not P2's unmet check entry; round 2, told about it, found both. The replay test refused round 1, as it should. |
f97854b: the prompt now asks the agent to go through each check entry and report any the code does not meet as written. Not committed: the recordings answer the old prompt, so they must be made again. |
Self-review was done before each independent round. Round 17 is the clean independent pass that allows the Copilot request on 7a71286.
Review-lesson audit
Every finding is classified. The rules cited are in CLAUDE.md ("Review readiness") unless AGENTS.md is named.
- Claim rule: "Treat every behavioural claim … as something to verify … and give it a test that fails if the claim is false".
- Setup rule: "A test's setup must leave the state the production path would".
- Fakes rule: "A fake of an external tool … name the version checked".
- Audit rule: "Treat review-lesson extraction as a merge gate".
- Follow-up rule: "report … deferred follow-up issues".
- One-off (fitting): specific to this prompt's fitting algorithm (its reserve, order and notes). Each was fixed in the code, with a test that fails without the fix. No rule for other code follows from it.
| Round | Finding | Class |
|---|---|---|
| 1 | NUL or unpaired surrogates in answer text | One-off: a data-block limit, now refused at validation |
| 1 | Answer problems too large for the next prompt | One-off (fitting) |
| 1 | Check text kept on items without that check | One-off: the split rule, tested |
| 1 | Schema errors named planning | Claim rule |
| 1 | Docker test missing from the scheduling test | One-off: the list is now pinned by vitest-config.test.ts |
| 2 | Reserve measured on the wrong prompt | One-off (fitting) |
| 2 | Blank summaries accepted | One-off: schema pattern, tested |
| 2 | Byte arithmetic untested; T9 rows out of date | Claim rule |
| 3 | Changed-file list made the reserve inexact | One-off (fitting) |
| 3 | Round 1 accepted earlier problems | One-off: an input check, tested |
| 3 | Shared host-root prefix | One-off: own prefix, tested |
| 4 | 12 KiB answer budget refused valid answers | AGENTS.md: valid bounded input must not fail only because a budget is smaller |
| 5 | Large prompts refused instead of parts left out | AGENTS.md budget rule, as round 4 |
| 5 | Duplicate problems; approved argv accepted for a Claude review | One-off: validation and policy, tested |
| 6 | Order of what is left out; non-size refusals hidden; no problem cap | One-off (fitting) |
| 7 | Round 2 could refuse what round 1 accepted; text refused only where it fitted | One-off (fitting) |
| 7 | Start call's decoding untested | Claim rule |
| 8 | A note could push out its own part; a blank-line gap | One-off (fitting and template) |
| 8 | Check-dispatch vendor in a test | Setup rule |
| 9 | Note room changed between rounds | One-off (fitting) |
| 9 | Default round limit undocumented | Claim rule |
| 10 | Decoder, ordering and NUL tests missing | Claim rule |
| 10 | Comment-count reserve | One-off (fitting) |
| 11 | Over-reservation | One-off (fitting) |
| 11 | Size-only refusal and fix-phase decoding untested | Claim rule |
| 12 | No reserve for the description and lessons notes | One-off (fitting) |
| 12 | Zero-listed earlier note had no heading | One-off: template, tested |
| 12 | Padding that was not schema-valid | Setup rule |
| 13 | Decoder fakes named no CLI version | Fakes rule |
| 14 | Round-1 reserve for description and lessons untested | Claim rule |
| 14 | A later round refused for room it only reserved | One-off (fitting) |
| 15 | Round-1 refusal did not name the room it keeps | Claim rule (an error message is a claim) |
| 16 | 1 MiB answer limit untested | Claim rule |
| Copilot 1 | Validation read the caller's round after preparation | One-off: validation now reads only the frozen request, and a test changes the input after preparing. No other prepared request here keeps a reference to its input. |
| Copilot 1 | Missing recordings keep CI red | Covered by the claim rule: the replay test is the test for "recorded Claude output replays through validation", and it stays enabled. The finding stays open until the user's recordings are committed. This PR does not merge before then. |
| 18 | Claim that a later round fits was false for a comment or title added between rounds | Claim rule |
| 18 | Empty change texts in the padding (a repeat of round 12) | Setup rule, now pinned by an assertPlan test on the padding |
| 19 | PR body overstated the Docker test | Claim rule |
| 19 | Lesson audit left out findings | Audit rule |
| 19, 20 | Issues the loader cannot give (round 20: round 19's fix was incomplete) | Setup rule |
| 21 | Lesson audit did not classify every finding | Audit rule (this table) |
| 21 | Earlier problems no answer can give; a lesson over 32 KiB | Setup rule: the tests now build earlier problems through validateReviewAnswer |
| 21 | Title room relies on GitHub's 256-character limit, which the loader does not enforce | Claim rule: the docs and code comment now say so |
| 21 | The oversized-plan follow-up had no issue | Follow-up rule: #209 |
| 22 | The prompt is a command argument (AGENTS.md: "Pass text whose size follows user or agent input to a subprocess on stdin") | Covered by that AGENTS.md rule, with the fix deferred to follow-up #210 (every Claude prompt to stdin, then re-record). Until then the budget keeps the prompt under Linux's 128 KiB argument limit (tested), and NUL is refused first, as for planning. |
| 23 | Audit rows with classes the gate does not allow | Audit rule (rows reclassified) |
| 23 | Turn-limit fake named no CLI version | Fakes rule |
| 24 | The review agent could not see the diffs, against the design's claim | Claim rule (the design's claim had no test; the recording now asserts the planted unrelated edit is found) |
| 24 | Temporary file left when a write fails | AGENTS.md: "… through a unique temporary file opened exclusively, and delete it if the write fails". scripts/record-planning.ts has the same pattern; it is outside this PR. |
| 25 | In-tree attributes could hide a diff | One-off: specific to reading another agent's commits with Git, now --text, and tested with a * -diff attribute |
| 25 | A file's diff included files under it | Captured by the round 27 rule (Git's literal pathspec matches by prefix) |
| 25 | Diffs read with no bound | AGENTS.md: "Work that leaves a serializing path … needs its own concurrency bound". Reading now stops once the diffs fill a whole prompt, and runs asynchronously with a signal. |
| 25 | --ignore-submodules=none |
AGENTS.md: "pass --ignore-submodules on the command line"; now =all, tested with a gitlink |
| 26 | Type changes showed only the old side | Captured by the round 27 rule: the round 25 section split is replaced by exclude pathspecs, tested with a file turned symlink |
| 26 | Renames shown as a delete and an add | One-off: now -M with both paths, tested with a large renamed file and a one-line edit |
| 26 | A stop rejected before Git exited | AGENTS.md interleaving rule ("An abort error fires, then subprocess close arrives later"); now rejects on close, tested with a stand-in Git that ignores SIGTERM |
| 27 | Rename into a directory of its own name hidden | New rule in AGENTS.md ("Agent-controlled content"): a literal pathspec still matches every path under it; exclude them except a rename's other side, and test file-to-directory, type change and rename-into-directory. Rounds 25 and 26's pathspec rows are reclassified under it. |
| 27 | Read bound counted only shown diffs | AGENTS.md concurrency-bound rule, as round 25; reading now stops once the listed paths alone fill a prompt |
| 27 | One large diff pushed later diffs out | One-off (fitting): later files are now listed whole or with a null diff wherever they fit |
| 27 | Stand-in Git named no version; 1 s wait | Fakes rule (the stand-in now says it models no output the code reads) and validation-evidence rule (load-tolerant wait) |
| 28 | A rename into its own directory picked up other files there | Captured by the round 27 rule, whose wording this branch corrects: other changed paths under it are excluded by name, and its test now has another changed file there |
| 28 | Read-bound comment false | Claim rule: the comment and docs now say a later file can still be listed, with a null diff |
| 29 | Byte-order mark dropped from names | One-off: TextDecoder drops a leading U+FEFF by default; now ignoreBOM, tested with a BOM-prefixed name next to the plain one |
| 29 | Named excludes capped by count, not bytes | One-off, a recorded exception to the AGENTS.md stdin rule: git diff reads pathspecs only from its arguments (it has no --pathspec-from-file), so the agent-chosen names are bounded to 64 KiB of arguments, far under the OS limits, with a null diff past it. Tested. |
| 29 | Excludes built for every record up front | AGENTS.md concurrency-bound rule: built only for diffs that are read, with a binary search over the sorted paths |
| 30 | Decomposed names missed on macOS | AGENTS.md: "A host that folds case or Unicode normalization … answers differently"; core.precomposeunicode=false on every call, and an empty diff is null; tested with an NFD name next to its NFC twin |
| 30 | Audit row misclassified | Audit rule (row corrected) |
| 30 | Type comment gave an old reason | Claim rule (comment corrected) |
| 31 | Rename detection skipped silently | AGENTS.md: decide pass or fail by what a tool's warning means; the limit is pinned on the command line and the warning makes reviewDiffs refuse (ReviewDiffsRefused), tested with a lowered limit |
| 31 | Architecture row out of date | Claim rule (docs) |
| Self-review | Rename-skip warning could be pushed out of an 8 KiB buffer | AGENTS.md: "filter the output as it arrives so that no volume of benign messages can push a failure out of a bounded buffer" |
| 32 | Review record missed a commit | Review-readiness rule: "For each self-review, independent-agent and Copilot round, record …" |
| 32 | Hardening comment overstated | Claim rule (comment and docs narrowed) |
| 33 | Renames and deletes with a null diff lost what changed | Claim rule ("every file that differs" must be true with a null diff too): each entry now carries change and from, tested with a large rename and a large delete |
| 33 | Part 2's PR described as open | CLAUDE.md: "Distinguish requested, started, pushed, … states" |
| 33 | Recording could delete another run's temporary file | AGENTS.md: delete the temporary file "if the write fails" means the one this run opened; EEXIST now leaves it |
| 34 | Submodule type change described wrongly | Claim rule: docs and comment now say it shows only the file's side; tested |
| 34 | Rename-skip rule described wrongly | Claim rule: message, docs and this body now describe Git's rule (unpaired deletes × adds past the limit squared) |
| 35 | P2 defect described wrongly | Claim rule (fixture comment, earlier problem text and docs corrected) |
| Recording 1 | Round 1 missed an unmet check entry | One-off: a prompt finding from real vendor output, which the replay test exists to catch; the prompt's first rule is now explicit about check entries |
New rules: one, in AGENTS.md, from round 27 (Git pathspec prefix match). Every other finding is covered by an existing rule or is specific to this feature.
🤖 Generated with Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
…part 1) The review agent (design, 'Review rounds' step 1) answers one round as JSON: lane D's Claude review command now carries an answer schema, as planning's does, with planning's model budget. prompts/review.md and core/review-prompt.ts build the round's request with untrusted text in escaped data blocks, and validate the answer, splitting a problem that names several plan items into one per item. scripts/record-review.ts records real Claude output, which test/review-recorded.test.ts replays; a Docker test proves the command and schema reach the vendor. The runner does not run review rounds yet (part 2). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…nd 1 findings (#201 part 1) - Refuse answer text with NUL or unpaired surrogates, and answers whose problems would not fit the next round's prompt; keep that room in every round's prompt. - Give the next round only the previous round's problems. - Keep a problem's check text only on items that have that check entry. - Name the review in schema errors; share planning's root removal and output check. - Add the Docker test to the scheduling test; sweep with the real docker and a limit. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ound 2 findings (#201 part 1) - Reserve room on the next round's largest prompt: the longest round number and every declared path listed as changed; the last round keeps none. - Refuse blank summaries and details; let a problem name up to 30 items, as a plan has. - Test the byte arithmetic to the boundary, and the review command's lack of a shell. - Bring the T9 rows, architecture and adapter notes up to date for the review schema. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…dings (#201 part 1) - The review agent reads the declared files; without changed files the next round's prompt is this one's but for its round number, so the reservation is exact. - Round 1 takes no earlier problems; a clearer attempt ID refusal; no dead check. - The review's host roots get their own prefix; comments and T9 rows say what is tested. - The reservation test runs with ASCII text too, so a 2-byte error fails it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
#201 part 1) Review round 4 found that the 12 KiB answer budget refused schema-valid answers (AGENTS.md: valid bounded input must not fail only because a budget is smaller). The earlier problems are a hint, so the next round's prompt now lists as many as fit, in order, and says how many it left out. The prompt may use the review's whole 64 KiB model budget less its schema. validateReviewAnswer checks an answer without rebuilding its prompt. The CI guide covers recording the review fixtures. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…d fix review round 5 findings (#201 part 1) - When the prompt would not fit, it leaves out earlier problems, the issue's comments, its description, the lessons, then the plan's change texts, each with a note. Only a plan too large without its change texts is refused; the plan block may use the whole budget. - The earlier-problems fit lists every one when all fit, and drops the section when none does. - The same problem twice for one item is one problem. - A Claude review captured with approved argv is refused: it has no tool to run them. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…6 findings (#201 part 1) - The plan's items, intents, files and checks always go in; then, while they fit, the change texts, earlier problems, the issue's description, lessons and comments. A note names what was left out, including when no earlier problem fits. - Only a size refusal counts as not fitting; other refusals surface. - At most 10 problems of at most 10 items each per round. - The known limit (a plan too large without change texts) is documented. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… findings (#201 part 1) - The smallest prompt is the plan and the prompt's own text; codeboost's notes go in next, so round 2 never refuses a plan round 1 accepted. - Text a data block cannot hold is refused wherever it is. - decodeClaudeAnswer reads planning and review answers from structured_output, tested. - Comment and doc wording follow the code. Co-Authored-By: Claude Opus 5.5 <[email protected]>
- Notes are kept one by one when not all fit, and tried again once parts are in, so a part is left out unannounced only when the plan leaves no room for its note. - The template's earlier-problems section no longer leaves a blank-line gap. - The command-check dispatch test uses the runner vendor, as production does; the execute launcher's schema is tested as refused for a review. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ings (#201 part 1) - The smallest prompt keeps room for every note in its longest form and for the longest round number, so notes always go in, a note never pushes out the part it names, and every later round of a plan fits whenever round 1 did. - Docs say which duties are part 2's, and that the round limit is 3 by default. - Clearer turn-limit advice and test names. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…gs (#201 part 1) - A test stands in for setup and the supervisor to check the decoder the Claude start call uses, per phase. - Tests for the earlier-problems-before-description order and NUL in check text. - The comment-count reserve is the real count; the docs name the issue-change exception. - Clearer refusal text; the policy test imports the execute schema; the Docker test's Git keeps the developer's configuration out. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…dings (#201 part 1) - The smallest prompt already holds every note of a part it leaves out; only the round number and the earlier-problems note change between rounds, so only they are reserved. - A test pins that only a size refusal counts as not fitting. - The start call's decoding is tested for fix too; doc wording follows the code. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Reserve room for the issue description and lessons notes, give the zero-listed earlier-problems note its heading, and pad tests with schema-valid plans. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…d 13 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ew round 14 (#201 part 1) Round 1 keeps room for what can change between rounds; a later round no longer refuses for that room, so a comment added between rounds does not blame the plan. Test both the room round 1 keeps and the later round's own check. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Validation can drift from the captured round, and missing Claude recordings leave required replay and CI gates failing.
2 open issues
What changed in this PR
Adds the review-agent foundation for #201: bounded prompts, validated answers, and read-only Claude execution. The runner’s review loop, persistence, merge blocking, and UI remain in part 2.
Changes:
- Builds escaped, size-bounded review prompts and validates problems against plan items.
- Extends Claude’s structured-answer handling to the review phase.
- Adds recording tools, regression coverage, Docker scheduling, and implementation documentation.
| File | Description |
|---|---|
vitest.config.ts |
Schedules review Docker tests serially. |
test/vitest-config.test.ts |
Checks review Docker-test scheduling. |
test/review-recorded.test.ts |
Requires and validates real Claude recordings. |
test/review-prompt.test.ts |
Tests prompt budgets, validation, and review helpers. |
test/fixtures/review/recording-inputs.ts |
Defines recording inputs and provenance hashes. |
test/agent-supervisor.test.ts |
Uses questions for schema-free cancellation testing. |
test/agent-review.test.ts |
Adds a real-Docker review invocation probe. |
test/agent-policy.test.ts |
Tests review schemas, limits, and command restrictions. |
test/agent-launch.test.ts |
Uses questions for schema-free launch testing. |
test/agent-adapter.test.ts |
Tests review schema loading and answer decoding. |
test/agent-adapter-decode.test.ts |
Verifies decoding through the adapter’s start path. |
test/agent-adapter-cleanup.test.ts |
Keeps cleanup fixtures schema-free. |
scripts/record-review.ts |
Captures genuine Claude review answers. |
runner/review-agent.ts |
Adds read-only review execution helpers. |
runner/question-container.ts |
Allows review in shared read-only infrastructure. |
runner/production.ts |
Exports the execute schema for compatibility tests. |
runner/planning-provider.ts |
Shares cleanup and structured-output helpers. |
prompts/review.md |
Defines plan-based review instructions. |
prompts/review-answer.schema.json |
Defines bounded review-answer JSON. |
docs/implementation/review-rounds.md |
Documents part 1 and part 2 integration decisions. |
docs/implementation/ci.md |
Documents recording and replay procedures. |
docs/implementation/agent-isolation.md |
Updates review isolation and schema guarantees. |
docs/architecture.md |
Records the review-agent foundation and remaining scope. |
core/review-prompt.ts |
Builds prompts and validates per-item problems. |
core/planning-author.ts |
Supports configurable data-block limits and size errors. |
agents/policy.ts |
Adds structured review commands and restrictions. |
agents/adapters/claude.ts |
Loads review schemas and decodes structured answers. |
.github/workflows/ci.yml |
Excludes review Docker tests from parallel execution. |
.github/workflows/agent-isolation.yml |
Updates cleanup-time accounting comments. |
Review effort: Balanced
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Comment on lines
+15
to
+17
| it('has a Claude recording for each round', () => { | ||
| // Fails until scripts/record-review.ts has been run with credentials. Synthetic fixtures do not satisfy it. | ||
| expect(names).toEqual([...REVIEW_RECORDING_NAMES]); |
Contributor
Author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and the assertion stays on. The recordings need a real Claude token, so the repository owner must run scripts/record-review.ts. Once both files are committed, the gates will be rerun on that head. Until then this PR is not ready to merge. I am leaving this thread open until the recordings land.
🤖 Addressed by Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
…view (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ound 1, and pad tests with schema-valid change texts, from review round 18 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…19 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…om review round 20 (#201 part 1) The blocks over 32 KiB that remain are marked as inputs the loader refuses, kept to show prepareReview does not rely on that. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…room relies on GitHub's limit, and link follow-up #209, from review round 21 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…om review round 22 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…output, from review round 23 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
… part 1) The review prompt now carries the plan's base commit and each changed file's git diff, fitted like the other parts (as many as fit, in path order, with a fixed note for the rest), and asks the agent to report changes no item's intent covers. runner/review-diffs.ts makes the diffs with hardened Git. The recording repository commits the code before and after the plan, and plants an unrelated edit in a declared file. The recording script deletes its temporary file when a write fails. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ndary (#201 part 1) The runner prepares a review inside the server, so the per-file Git calls must not block it: they now run as child processes with a signal, keep at most 32 KiB of stdout and drain stderr. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Pass --text so an in-tree attribute cannot hide a diff, keep only a file's own section when Git's prefix match adds files under it, stop reading diffs once they fill a whole prompt, and keep Git out of submodules as AGENTS.md asks. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ted, from review round 26 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ble paths, and keep renames into a directory visible, from review round 27 (#201 part 1) Adds an AGENTS.md rule for Git's pathspec prefix match, which caused findings in review rounds 25, 26 and 27. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… name, from review round 28 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…bytes and build them lazily, from review round 29 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…as not shown, from review round 30 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…diffs in the architecture, from review round 31 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…art 1) AGENTS.md asks that no volume of benign messages can push a failure out of a bounded buffer; a warning split between chunks is still found. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ound 32 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…, and remove only a temporary file the recording opened, from review round 33 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…, from review round 34 (#201 part 1) Co-Authored-By: Claude Opus 5.5 <[email protected]>
…cribe the planted P2 defect accurately (#201 part 1) The first real recording (Claude Code 2.1.281, on fb3f536) found the planted unrelated edit in round 1 but missed P2's unmet check entry until round 2 was told about it. Review round 35 found the fixture's description of that defect inaccurate. Both change the prompts, so the recordings must be made again. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Copilot code review
Copilot
Copilot left review comments
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
None yet
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.