feat(review): prompt templates, structured review output, opt-in stop review gate - #21
Conversation
…, --json output Modelled on openai/codex-plugin-cc's session tracking, adapted to the zero-deps runtime: - hooks/hooks.json + scripts/session-hook.mjs: SessionStart exports the Claude session id (and CLAUDE_PLUGIN_DATA) into CLAUDE_ENV_FILE; SessionEnd cancels the session's still-running jobs so a closed session no longer leaves detached workers and cursor-agent running unattended. Other sessions' jobs and unattributed jobs are never touched. - createJob stamps sessionId from CURSOR_PLUGIN_CC_SESSION_ID - /cursor:status default view scoped to the current session (plus unattributed jobs) with a hidden-rows hint; --all lifts scope and cap; new lib/jobs.mjs#filterJobsForSession - --json on status/result/setup for scripting (raw job records / doctor report); setup's doctor split into gather + render - pluginHome(): CURSOR_PLUGIN_CC_HOME > existing ~/.cursor-plugin-cc > CLAUDE_PLUGIN_DATA/state > ~/.cursor-plugin-cc; jobs/<repo-hash>/ layout unchanged - tests: session stamping, session filter, hook handlers (env-file write, selective cancel), state-root precedence - docs: README session-lifecycle section, AGENTS.md state-root rule, CHANGELOG Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… 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 PR (prompt templates externalized, structured review output, opt-in stop-review gate) with good test coverage for the pure-logic pieces. However, the stop-review-gate hook reads a last_assistant_message field from the Claude Code Stop hook payload that the harness does not actually provide (the real payload exposes transcript_path, not the message text), so the 'Previous Claude response' context the design and CHANGELOG describe will silently never populate in production. A few smaller consistency/coverage gaps round out the review.
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] runGateReview reads input.last_assistant_message to build the CLAUDE_RESPONSE_BLOCK context for the reviewer prompt, but Claude Code's documented Stop hook JSON payload only includes session_id, transcript_path, cwd, hook_event_name, and stop_hook_active — there is no last_assistant_message field. In real usage this will always be undefined, so last is always '' and CLAUDE_RESPONSE_BLOCK interpolates to an empty string every time, even though the CHANGELOG/README describe the gate as reviewing 'that turn's edits' with 'Previous Claude response: …' context. The gate will still run (grounded only in whatever the reviewer inspects on its own via tools), but the intended context-passing never actually works.
Suggestion: Read
input.transcript_path(the JSONL transcript Claude Code writes) and extract the last assistant message from it, the way other hook-consuming tools do, instead of assuming the message is inlined in the hook's stdin JSON. Add a gate.test.mjs case that constructs input the way the real harness does (transcript_path pointing at a fixture JSONL) to catch this kind of payload-shape drift.
| "title": "Cursor review structured output", | ||
| "description": "The fenced ```json block a /cursor:review run appends after its Markdown findings. Parsed by scripts/lib/review-output.mjs and stored on the job record as `review`.", | ||
| "type": "object", | ||
| "additionalProperties": false, |
There was a problem hiding this comment.
[MINOR] The schema declares additionalProperties: false at both the top level and per-finding, but the hand-rolled validator in scripts/lib/review-output.mjs (isValidFinding, parseReviewOutput) never rejects objects with unexpected extra keys — it only checks that the required fields exist and have the right type. The schema and the actual runtime contract have quietly diverged.
Suggestion: Either drop
additionalProperties: falsefrom the schema (since it's documentation-only and not enforced) or add an explicit extra-keys check in the validator so the two stay in sync.
| * @param {boolean} enable | ||
| * @returns {Promise<number>} | ||
| */ | ||
| async function toggleReviewGate(enable) { |
There was a problem hiding this comment.
[MINOR] No test exercises toggleReviewGate/the --enable-review-gate/--disable-review-gate CLI paths in setup.mjs; config.test.mjs only covers the underlying lib/config.mjs functions directly, not the command wiring (flag parsing, stdout messaging, or the new 'stop review gate' doctor check).
Suggestion: Add a setup.mjs test (or extend an existing one) that runs
main(['--enable-review-gate'])/main(['--disable-review-gate'])and asserts both the persisted config and the printed confirmation text.
| Object.prototype.hasOwnProperty.call(variables, key) ? String(variables[key] ?? '') : '', | ||
| ); | ||
| // Optional blocks interpolate to '' and would leave runs of blank lines. | ||
| return filled.replace(/\n{3,}/g, '\n\n'); |
There was a problem hiding this comment.
[SUGGESTION] interpolateTemplate's blank-line collapse (/\n{3,}/g → \n\n) runs over the fully-assembled template, including the interpolated BODY (the raw diff/file content passed into the review prompt). Any diff hunk that legitimately contains 3+ consecutive blank lines gets silently collapsed to 2, so the text handed to the reviewer is not byte-for-byte the diff that was collected.
Suggestion: Collapse blank-line runs only around known optional placeholders (e.g. run the collapse before substituting BODY, or scope it to lines adjacent to a placeholder) rather than over the final interpolated string, so arbitrary diff content is never mutated.
1 participant
Summary
Replaces #19, which was auto-closed when its stacked base branch (
feat/session-lifecycle, #18) was deleted on merge. Content is identical, now rebased onmain; see #19 for the original description and discussion.prompts/*.md+lib/prompts.mjsinterpolator (prompts/excluded from prettier — reflow was rewriting model-facing whitespace).schemas/review-output.schema.json, hand-rolled validation inlib/review-output.mjs, stored on the job record asreview./cursor:setup --enable-review-gate,Stophook, ALLOW:/BLOCK: contract, loop guard viastop_hook_active, degrade-to-note when Cursor is missing/logged out).lib/config.mjs.Test plan
npm test: 123/123 green after mergingmainin.npm run lint+npm run typecheck: clean.