feat(review): prompt templates, structured review output, opt-in stop review gate - #19
feat(review): prompt templates, structured review output, opt-in stop review gate#19freema wants to merge 1 commit into
Conversation
… review gate
Ported from openai/codex-plugin-cc's review chain, adapted to the
zero-deps runtime:
- prompts/*.md: review prompt moved out of JS string arrays into
reviewable templates ({{UPPER_SNAKE}} placeholders, new lib/prompts.mjs
loader/interpolator with blank-line collapse); prompts/ excluded from
prettier so template whitespace stays intentional
- schemas/review-output.schema.json + lib/review-output.mjs: reviews
append a fenced json verdict block (verdict/summary/findings/next_steps,
hand-rolled validation); parsed data stored on the job record as
`review`, raw fence stripped from the human-facing summary; parsing
never fails a review
- stop review gate (opt-in, per repo): Stop hook runs a Cursor review of
the turn with an ALLOW:/BLOCK: first-line contract; BLOCK surfaces
{"decision":"block"} so the turn continues. stop_hook_active
short-circuits (no loops); missing/logged-out Cursor degrades to a
stderr note; also warns about still-running jobs on stop
- lib/config.mjs: per-repo config at <state-root>/config/<repo-hash>.json;
/cursor:setup --enable-review-gate / --disable-review-gate toggle, gate
status shown in doctor/--json
- test stub understands `cursor-agent status` (CURSOR_AGENT_STUB_AUTH)
- tests: prompts, config, review-output parser, gate hook
(allow/block/loop-guard/logged-out)
freema
left a comment
freema
left a comment
There was a problem hiding this comment.
CodeForge Review
Verdict: request_changes | Score: 6/10
Well-structured feature (stop review gate, structured review output, externalized prompt templates) with solid unit test coverage for the new lib modules, but the gate's core context-passing likely doesn't work against Claude Code's real Stop hook payload, and there are a few test-coverage and duplication gaps in the new wiring.
General Issues
- [MINOR] The Stop hook now runs unconditionally on every turn for every plugin user, even when the review gate is disabled (the previous default), adding new per-turn subprocess/git overhead that didn't exist before.
Suggestion: Short-circuit earlier when no config exists for the repo, or document the added per-turn cost.
- [SUGGESTION] Schema declares
additionalProperties: falsebut the hand-rolled validator never enforces it, so the documented contract is stricter than what's actually validated.Suggestion: Either relax the schema annotation or add an unknown-key check to the validator.
Reviewed by CodeForge
| */ | ||
| async function runGateReview(input, root) { | ||
| const template = loadPromptTemplate('stop-review-gate'); | ||
| const last = String(input.last_assistant_message ?? '').trim(); |
There was a problem hiding this comment.
[MAJOR] Reads input.last_assistant_message, a field Claude Code's Stop hook JSON payload does not appear to provide (the documented schema is session_id/transcript_path/cwd/hook_event_name/stop_hook_active). In production this is likely always empty, so the reviewer never gets the previous turn's response text.
Suggestion: Parse the transcript at
input.transcript_pathto extract the last assistant message instead of reading a non-existent field.
|
|
||
| const sessionId = | ||
| (typeof input.session_id === 'string' && input.session_id) || process.env[SESSION_ID_ENV]; | ||
| const running = listJobs(root).filter( |
There was a problem hiding this comment.
[MINOR] Reimplements session-scoped running-job filtering inline instead of reusing filterJobsForSession/findRunningJobs from lib/jobs.mjs, risking drift between the two implementations.
Suggestion: Use
filterJobsForSession(findRunningJobs(root), sessionId).
| return { bin, checks, mcps, allOk }; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[MINOR] toggleReviewGate and the new doctor check have no test coverage anywhere in the diff.
Suggestion: Add a setup.test.mjs case exercising
--enable-review-gate/--disable-review-gatethroughmain().
| result.exitCode === 0 && summary.success && warnings.length === 0 ? 'done' : 'failed'; | ||
| // The prompt asks the reviewer to append a fenced ```json verdict block | ||
| // (schemas/review-output.schema.json). When it parses, store the data on | ||
| // the record and hide the raw fence from the human-facing summary. |
There was a problem hiding this comment.
[MINOR] No end-to-end test verifies that a fixture with a trailing json block actually gets stored as review on the job record and stripped from the summary via the real runReview/updateJob path.
Suggestion: Add a structured-output fixture and assert on the stored job record.
1 participant
Summary
Third of four PRs porting the high-value patterns from
openai/codex-plugin-cc. Stacked on #18 (extends itshooks/hooks.json); merge #18 first, then retarget this tomain.prompts/review.md; newlib/prompts.mjs({{UPPER_SNAKE}}interpolation, blank-line collapse for empty optional blocks). Prompts are now reviewable as prose.prompts/is excluded from prettier — markdown reflow was rewriting model-facing whitespace (it even turned a---separator into a setext heading).schemas/review-output.schema.json(verdict, findings with severity/file/line, next steps). Hand-rolled validation in `lib/review-output.mjs` (zero-deps). On success the data lands on the job record (`review`, visible via `/cursor:result --json`) and the JSON fence is stripped from the human-facing summary. Models that ignore the instruction just produce an unstructured review — parsing never fails a run./cursor:setup --enable-review-gate. AStophook has a Cursor model review the turn's edits (prompts/stop-review-gate.md, first-lineALLOW:/BLOCK:contract); a BLOCK emits{"decision":"block","reason":…}so Claude keeps working. Hardening beyond the codex original:stop_hook_activeshort-circuit (a block can never loop), and missing/logged-out Cursor degrades to a stderr note instead of trapping the session. The hook also warns about still-running jobs at stop time.lib/config.mjsat<state-root>/config/<repo-hash>.json(outsidejobs/), gate status surfaced in--doctor/--json.Test plan
tests/prompts.test.mjs,tests/config.test.mjs,tests/review-output.test.mjs(valid/invalid blocks, last-block wins, strip),tests/gate.test.mjs(ALLOW silent, BLOCK decision JSON, loop guard, disabled no-op, logged-out degrade; stub extended with astatusauth mode).npm test: 119/119 green.npm run lint: clean.