feat(review): citadel-native review system (request-review hook + comments + MCP)#37
Open
ovdmar wants to merge 19 commits into
Open
feat(review): citadel-native review system (request-review hook + comments + MCP)#37ovdmar wants to merge 19 commits into
ovdmar wants to merge 19 commits into
Conversation
… system Plan covers a repo-configurable "Request review" hook + SQLite-backed review comments readable by agents via MCP. Dual-reviewed (3 rounds with the Opus reviewer); approved with the concerns log in the plan history. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
B.4: Human Review items 3-5 advance to [~] with the v1 store + MCP + Review tab caveat; full diff-anchored renderer + GitHub-style review mode stay planned. Add Request Review subsection and a retention note. B.6: register the workspace.requestReview hook event with its dedicated ReviewSuggestionsOutput payload and blocking-default semantics. B.7: expand the MCP tool inventory with the four review tools and document the author-stamping + optimistic-concurrency rules. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Splits the new schemas into packages/contracts/src/review.ts so the main contracts file stays under the 800-line file-size limit. Adds zod schemas + inferred types for ReviewSuggestion, ReviewSuggestionsOutput, ReviewComment (with file/line anchor refinement), ReviewSuggestionRun, and RequestReviewPayload. Nullable fields use .nullable().default(null) so a fully-omitted object parses without missing-key surprises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the event to HookEventSchema, includes it in the blocking-default list (matches setup/teardown semantics), adds requestReviewHookIds to repoDefaults, and wires validateHookReferences so the event/hook id mismatch is caught at load time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n 8) Two additive tables with FK cascades to workspaces(id) and supporting indexes. SqliteStore gains CRUD methods via the same module-augment pattern used for scheduled-run-store. Optimistic concurrency via ifUpdatedAtMatches returns conflict without mutating; soft-delete keeps rows physically present so threads stay readable via includeDeleted. Also extends Repo with requestReviewHookIds (defaulted, additive column). Existing fixtures touched to satisfy the now-required field. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors parseHookOutput's empty-stdout-returns-null contract. Schema validation surfaces malformed or invalid payloads as a throw the operations service catches and records as a failed run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Standalone functions in review-system.ts (no augmentation of OperationService to keep operations/index.ts under the 800-line gate). requestReviewForWorkspace exits cleanly through: - no-hook (returns sentinel, no activity) - succeeded (parses output, writes succeeded run + ONE activity row) - failed (non-zero exit OR invalid JSON, writes failed run + activity) - timed-out (detects via runner rejection text + wall-clock backup) Comment CRUD goes through optimistic-concurrency-aware DB accessors and logs a single activity row per successful mutation (added/updated/ resolved/deleted). Exports commandHook from hooks-runner for reuse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stReview listHookDiagnostics gains the new event in its enumeration so the repo-settings diagnostics panel surfaces a request-review hook's validation state. structured-config.tsx Settings UI dropdown + ConfigResponse repoDefaults type pick it up so operators can author the hook without hand-editing JSON. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six routes: list/post comments, patch/delete by id (with ifUpdatedAtMatches → 409 on stale), latest suggestion run, trigger request-review. POST stamps author='operator' regardless of body input and rejects extra fields via zod .strict(). Request-review builds the hook payload from the workspace diff summary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…request)
callMcpTool routes all five to a single review_tool_requires_daemon
sentinel; daemon-side handler implements the actual flows. Author is
stamped agent:<runtimeId>; caller-supplied author returns
author_not_allowed. Conflict semantics returned as { error: 'conflict',
latest } so agents can re-read and retry.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
inspector.tsx grows a third tab routing to the new ReviewTab (inspector-review.tsx, kept separate so inspector.tsx stays under 800 lines). RequestReviewPanel hits the new daemon routes; the button is disabled with a tooltip when the repo has no workspace.requestReview hook configured. ReviewCommentsPanel renders a flat list with status (open/resolved) sections; comments render as plain text so untrusted bodies cannot inject markup. Optimistic-concurrency token plumbed through resolve and delete actions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
E2E adds two API-only tests so the daemon endpoints exercised by the cockpit are kept honest: comment CRUD with optimistic-concurrency round-trip + request_review no-hook path. Skips DOM assertions on the new Review tab because the surrounding inspector still evolves; the underlying behavior is covered by daemon-route + UI unit tests. scripts/dev/smoke.ts asserts the five MCP review tools are surfaced and optionally exercises a POST/GET/DELETE round-trip when CITADEL_SMOKE_WORKSPACE_ID is set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- packages/operations/src/index.ts: consolidate scheduled-agents re-exports onto one line. - packages/mcp/src/index.ts: extract the five review tool definitions into a new packages/mcp/src/review-tools.ts file. - apps/daemon/src/app.ts: move the registerReviewRoutes call into extra-routes.ts so app.ts adds no net lines from this PR. (apps/ daemon/src/app.ts is pre-existing at the gate boundary; the move keeps it level with main.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
biome organized imports and reflowed long lines across the new code + collapsed three short blocks in app.ts so the daemon entry point fits under the 800-line ceiling. Operations re-export switched to \`export * from\` for the same reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…UX gates)
Validated review findings, fixed each:
1. BUG/SECURITY — MCP add_review_comment honored caller-supplied
runtimeId, defeating AC8. The schema doesn't declare runtimeId and
the dispatcher doesn't re-validate args, so a caller could stamp
any author. Now refuse both `author` and `runtimeId` from args and
stamp `agent:unknown` until the MCP transport surfaces per-client
identity. Test rewritten to codify the new contract.
2. BUG — MCP request_review sent diff:{files:[],…} while HTTP passed a
real diff. Extracted readWorkspaceDiffSummary and used in both
callers. Operations test added that hooks observe the real
files/lines/truncated payload over stdin.
3. PLAN AC2 — Request review button was visible even on the default
branch with no PR. ReviewTab now takes hasReviewableContext and
hides the panel when neither prUrl nor a non-default branch.
4. BUG — Structured-config "Save" dropped requestReviewHookIds (and
the pre-existing appHookIds/actionHookIds) because the payload only
surfaced setup/teardown. Now round-trips repoDefaultsExtra from the
loaded config.
5. UX — 409 conflict on PATCH/DELETE rendered as a generic error span.
CommentRow now detects ApiError.status === 409, refetches, and
renders an <output role="status"> banner.
6. SECURITY — ReviewSuggestionSchema.url accepted javascript:/data:
schemes (zod's .url() is permissive). Added http(s)-only refinement
+ contracts test.
7. BUG (minor) — DELETE on already-soft-deleted comment returned 404
regardless of token. Now distinguishes: fresh token → 204
(idempotent), stale token → 409 with the tombstone.
8. QUALITY — Removed dead exports `RegisteredReviewRoutes` (void) and
the unused `ReviewComment` re-export from review-routes.ts.
9. SPEC — B.4 § Request Review item 4 marker advanced to `[~]` and
notes the http(s) URL constraint.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…E test) - Hoist readWorkspaceDiffSummary to a static import in daemon-mcp-tool; the dynamic import was unmotivated (no circular-dep). - AC2 gate: hasReviewableContext now requires repo to be non-null for the non-default-branch arm so an orphaned/repo-less workspace doesn't default-open the Request review button. - Add review-routes test that codifies the re-DELETE soft-deleted contract: first DELETE → 204, stale-token retry → 409 with tombstone, post-tombstone token → idempotent 204. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nside-citadel-xleqqa # Conflicts: # packages/db/src/migrate.ts
Owner
Author
|
Re-opening to trigger CI workflow (initial PR creation did not fire |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Plan
`.agents/plans/review-system-inside-citadel.md` — dual-approved (3 review rounds; Opus reviewer per MEMORY.md).
Test plan
🤖 Generated with Claude Code