From db18a40924c28bed860596a20c6f68ab00e6520d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:23:58 +0000 Subject: [PATCH 01/18] plan(review-system): add approved tech plan for citadel-native review 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) --- .agents/plans/review-system-inside-citadel.md | 413 ++++++++++++++++++ 1 file changed, 413 insertions(+) create mode 100644 .agents/plans/review-system-inside-citadel.md diff --git a/.agents/plans/review-system-inside-citadel.md b/.agents/plans/review-system-inside-citadel.md new file mode 100644 index 00000000..43d04c50 --- /dev/null +++ b/.agents/plans/review-system-inside-citadel.md @@ -0,0 +1,413 @@ +Activate the /implement-task skill first. + +# Plan: Review system inside Citadel + +## Acceptance Criteria + +Requirements came from the user prompt (free text, no ticket). Verbatim source: + +> - "Request review" button with a repo-configurable hook for custom "review suggestions" logic +> - Citadel-native review on PRs: comments stored in citadel (not GitHub), readable by agents via MCP + +Derived acceptance criteria (each must be checkable): + +- [ ] AC1 — A repo can declare a `workspace.requestReview` hook in `citadel.config.json` (new variant of `HookEventSchema` in `@citadel/config`); validation rejects unknown ids and accepts the new event. Authored hooks of this event default to `blocking: true` (added to the existing default-true list). +- [ ] AC2 — On a workspace inspector view that has a PR (or is on a non-default branch), a "Request review" button is visible. When the active repo has no `workspace.requestReview` hook configured, the button is disabled with a tooltip explaining how to wire one up. +- [ ] AC3 — Clicking "Request review" runs the configured hook with a structured payload (`{ workspace, repo, pr, diff: { files: string[], addedLines, deletedLines, truncated } }`) and renders the returned suggestions in a panel under the button. Errors and timeouts surface inline (do not crash the inspector). +- [ ] AC4 — Each request-review invocation is recorded as a single `activity_events` row (`hook.workspace.requestReview` on success / `hook.workspace.requestReview.failed` on failure or timeout) AND as a `review_suggestion_runs` row with the parsed structured payload (or stderr/error on failure), so the latest suggestions survive a reload. +- [ ] AC5 — From the inspector, an operator can add a Citadel-native review comment scoped to: (a) the whole workspace/PR, or (b) a specific file, or (c) a specific file + line range. Comments are stored in SQLite, not posted to GitHub. +- [ ] AC6 — Comments support: edit body, mark resolved/unresolved, soft-delete by author. The list is **flat** (no threading in v1). UI shows author, timestamp, status, and any file:line anchor. Update/delete requests carry an optimistic-concurrency `ifUpdatedAtMatches` token; mismatched tokens return 409 Conflict. +- [ ] AC7 — A new MCP read-only tool `list_review_comments({ workspaceId, status?, includeDeleted? })` returns the comment list (newest first by default) with all anchor metadata. `includeDeleted` defaults to false. +- [ ] AC8 — A new daemon-mediated MCP tool `add_review_comment({ workspaceId, body, filePath?, lineStart?, lineEnd?, side? })` persists a new comment. The MCP path forces `author = 'agent:'` (callers cannot supply `author`). HTTP route from the cockpit forces `author = 'operator'`. `update_review_comment` and `delete_review_comment` mirror the UI mutations with `ifUpdatedAtMatches`. +- [ ] AC9 — A new daemon-mediated MCP tool `request_review({ workspaceId })` triggers the hook on the daemon side and returns the structured suggestions (or a structured error if no hook is configured / parse failed / timed out). +- [ ] AC10 — SQLite schema is at version 8 after upgrade, with new tables `review_comments` and `review_suggestion_runs`; existing installs running the new schema on startup are unaffected (no destructive DDL, FKs preserved, `PRAGMA foreign_keys = ON` intact). +- [ ] AC11 — Suggestion output schema is validated by zod at hook-parse time (bad output fails parse, surfaces in the suggestion run + activity feed with a `failed` status, does not write a partial payload). An empty stdout is a successful run with zero suggestions and renders an explicit empty state in the UI. +- [ ] AC12 — Hook execution reuses the existing `runCommandHookForDiagnostics` runner via the existing `commandHook()` adapter in `packages/operations/src/hooks-runner.ts`; the operations service catches the runner's timeout rejection and writes the row with status `timed_out`. + +## Context and problem statement + +Citadel today exposes PR meta (URL, draft state, review decision, reviewer counts) and a diff endpoint, but has no first-class "review" surface inside the cockpit. Spec `specs/B.4-git-pr-ci-diff.md` § "Human Review (Planned)" items [ ] 1–5 reserve this product area: + +> A future full-screen *Human Review* mode is reachable from the inspector `Diff` tab. Human Review allows leaving file/line comments. Comments are visible to the active agent session as structured input. + +The user wants two related surfaces shipped together: + +1. **Request review** — a workspace-level button that calls a repo-configurable hook and shows the hook's "review suggestions" (e.g., a generated reviewer list, a checklist of risk areas, links to docs). This is the extension point for any custom team workflow (CODEOWNERS-style suggestions, LLM-generated checklist, on-call lookup, etc.). Hook output is opaque to Citadel beyond schema validation. + +2. **Citadel-native review comments** — file/line and workspace-level comments stored in Citadel's SQLite DB (not posted to GitHub). Comments are readable by agents via MCP, so an agent session attached to the workspace can pick up operator feedback as structured input. This is the foundational store for the full GitHub-style review mode (spec B.4 [ ] 1–4); v1 ships the store + MCP surface + a minimal inspector tab. The full file-and-line-anchored inline rendering inside a diff viewer is deliberately out of scope for v1 (tracked in spec B.4 [ ] 2 as a follow-up). **Threading is also deferred** — v1 ships a flat list (no `parent_id`). + +Why now: scratchpad-style ad-hoc notes (B.7 § Scratchpad) work for free-form ideas, but they don't anchor to a PR's files/lines and they aren't queryable as "comments on this PR". An agent reviewing a workspace's diff via MCP currently has no way to read structured operator feedback scoped to that PR. + +## Spec alignment + +Specs touched: + +- `specs/B.4-git-pr-ci-diff.md` — advances "Human Review (Planned)" items [ ] 3, 4, 5 from planned to partial (`[~]`). v1 ships items 3 (file/line comments), 4 (agent-visible structured input via MCP), 5 (scoped to selected workspace). Item 1 (full-screen review mode reachable from the `Diff` tab) is partially advanced: this plan adds a `Review` tab/section in the inspector, not a full-screen surface — keep item 1 as planned. Item 2 (GitHub-style review surface) stays planned. **Add a retention note**: v1 has no retention policy for `review_suggestion_runs` or `activity_events`; both grow unbounded. Bulk-resolve/delete on merged PRs is a follow-up. +- `specs/B.6-providers-hooks-config.md` — extends the Hooks subsection ([ ] 1–10): adds `workspace.requestReview` as a sixth hook type. Note that this hook returns the dedicated `ReviewSuggestionsOutput` schema (not the generic `HookOutput`), and defaults to `blocking: true` like setup/teardown. +- `specs/B.7-operations-activity-mcp.md` — extends "MCP tool inventory" with new read-only / mutating tools (`list_review_comments`, `add_review_comment`/`update_review_comment`/`delete_review_comment`, `request_review`). Activity items [ ] 2, 4, 7 are reinforced by the new event types `hook.workspace.requestReview[.failed]` and `review.comment.{added,updated,resolved,deleted}`. + +Discrepancies / divergences: + +- None of these are *contradictions* with the specs; they are advancements of items currently marked `[ ]`. The plan's **first implementation step** is to update specs B.4, B.6, B.7 to reflect the new state. +- `specs/A-shared-definitions.md` Core Terms list does not need a new term ("Review" is implicit under PR/Workspace). + +## Implementation approach + +**Chosen strategy: thin, additive vertical slice that lands the data model + hook + MCP surface + minimal UI together.** + +Concretely: + +1. **Contracts first.** Create a new file `packages/contracts/src/review.ts` (mirrors the existing `packages/contracts/src/scratchpad.ts` split pattern). Add `ReviewSuggestionSchema`, `ReviewSuggestionsOutputSchema`, `ReviewCommentSchema`, `ReviewSuggestionRunSchema`, `RequestReviewPayloadSchema`. Re-export from `packages/contracts/src/index.ts`. Keep `index.ts` under the 800-line file-size limit (it is 794 lines today). + +2. **Config layer.** In `packages/config/src/index.ts`: + - Extend `HookEventSchema` (lines 41-50) with `"workspace.requestReview"`. + - Extend `HookConfigSchema.transform` blocking-default array (lines 65-68) to include `workspace.requestReview` (so authored hooks default to `blocking: true`). + - Extend the inline `repoDefaults` object literal (lines 133-140) with `requestReviewHookIds: z.array(z.string()).default([])`. Update the outer `.default({...})` to include `requestReviewHookIds: []`. + - Extend the `.superRefine` block (lines 169-176) with a new `validateHookReferences(context, hooksById, config.repoDefaults.requestReviewHookIds, "workspace.requestReview", ["repoDefaults", "requestReviewHookIds"])` call. + +3. **DB layer.** Add two tables (`review_comments`, `review_suggestion_runs`) to `packages/db/src/migrate.ts`, bump `schema_migrations` to version 8 with name `review-system`. Add typed accessors in `packages/db/src` (mirror the shape used by activity/operations accessors). + +4. **Hook runner.** No new code in `packages/hooks` — existing `runCommandHookForDiagnostics` is reused. Add a `parseReviewSuggestionsOutput()` helper next to `parseHookOutput()` in `packages/hooks/src/index.ts`. + +5. **Operations service.** Reuse the existing `commandHook(hook, workspacePath, config)` adapter from `packages/operations/src/hooks-runner.ts:21` (lift to a shared exported helper in `hooks-runner.ts` if it isn't already exported, or import the file's internal helper via a small refactor: export `commandHook`). Add `requestReviewForWorkspace(deps, workspaceId)`: + - Resolve workspace + repo + the configured `workspace.requestReview` hook id. + - If no hook configured → return `{ kind: 'no-hook' }`. + - Build payload, call `runCommandHookForDiagnostics(commandHook(...), payload)` wrapped in try/catch. + - On resolved success (`exitStatus === 0`): parse output (treat empty as zero suggestions), insert `review_suggestion_runs` with status `succeeded`, append one `activity_events` row of type `hook.workspace.requestReview`, return parsed suggestions. + - On resolved non-zero exit: insert run with status `failed`, append activity `hook.workspace.requestReview.failed`, return structured error including stderr tail. + - On rejection (timeout / spawn failure): detect timeout by matching the rejection's message text (the runner produces `"Hook timed out after ${timeoutMs}ms"`); insert run with status `timed_out` (or `failed` for other rejections); append activity `hook.workspace.requestReview.failed`; return structured error. Track wall-clock elapsed as a backup signal in case the message text changes. + - Add comment service: `listReviewComments`, `addReviewComment`, `updateReviewComment`, `deleteReviewComment`. Each mutation appends one activity row (`review.comment.added`, `review.comment.updated`, `review.comment.resolved`, `review.comment.deleted`). Update/delete take `ifUpdatedAtMatches` (string ISO) and return `{ status: 'conflict', latest: }` on mismatch. `listReviewComments` filters out comments whose workspace is archived by default (`workspaces.archived_at IS NULL`); explicit `includeArchived: true` opt-in for admin reads. + +6. **Daemon HTTP.** New routes in `apps/daemon/src/review-routes.ts`: + - `POST /api/workspaces/:id/review-requests` → calls `requestReviewForWorkspace`, returns the parsed suggestions or `{ error: { code, message } }`. + - `GET /api/workspaces/:id/review-suggestions` → returns the latest `review_suggestion_runs` row. + - `GET /api/workspaces/:id/review-comments?status=open|resolved|all&includeDeleted=true` → list. Default: `status=all`, `includeDeleted=false`. Forces `author='operator'` not applicable to GET. + - `POST /api/workspaces/:id/review-comments` → create; forces `author='operator'` on the inserted row regardless of request body. + - `PATCH /api/review-comments/:id` → edit body / status. Requires header or body field `ifUpdatedAtMatches`. 409 on mismatch. + - `DELETE /api/review-comments/:id` → soft-delete with `ifUpdatedAtMatches`. 409 on mismatch. + +7. **MCP surface.** Register in `packages/mcp/src/index.ts`: + - Read-only: `list_review_comments({ workspaceId, status?, includeDeleted? })`. + - Daemon-mediated: `add_review_comment` (no `author` accepted; daemon stamps `agent:`), `update_review_comment` (with `ifUpdatedAtMatches`), `delete_review_comment` (with `ifUpdatedAtMatches`), `request_review`. + - Wire daemon-side implementations in `apps/daemon/src/daemon-mcp-tool.ts`. Daemon-side handlers extract the active MCP client's runtime id from the request context (or fallback to `'agent:unknown'`). Schema validation **rejects** an MCP `add_review_comment` request that supplies an `author` field. + +8. **Cockpit UI.** Three pieces in `apps/web/src/`: + - A "Request review" button + collapsible suggestions panel placed in the inspector PR meta row (or as a new "Review" sub-section above the Diff tab). Renders the empty-state when the latest run has zero suggestions ("Hook returned no suggestions"). + - A "Review" tab in the inspector (next to Diff) with: a "new comment" composer (body + optional file selector populated from the workspace diff + optional line range) and a flat list of comments (open first, resolved collapsed). + - HTTP client helpers colocated with the inspector files (or under `apps/web/src/api/` if that's the convention). + +9. **Activity wiring.** Every mutation routes through the operations service and writes one `activity_events` row. Event types (final list, no duplicates): + - Hook lifecycle: `hook.workspace.requestReview`, `hook.workspace.requestReview.failed`. + - Comment mutations: `review.comment.added`, `review.comment.updated`, `review.comment.resolved`, `review.comment.deleted`. + +**Rationale.** This slice ships both halves end-to-end without the full inline-diff renderer or threading. The hook contract is intentionally generic so teams can implement varied logic. Comments live in their own tables (status, FKs, soft-delete) because `activity_events` is append-only. + +## Alternatives considered + +- **Alternative A: Reuse `HookOutputSchema.actions`/`links` for review suggestions.** Rejected. `HookOutputSchema` is shaped for app/link discovery; cramming `kind: reviewer` into `actions` blurs the data model and forces UI shape-sniffing. A purpose-built `ReviewSuggestionsOutputSchema` is one extra type for clarity. + +- **Alternative B: Store comments in the scratchpad (block model).** Rejected. Scratchpads are workspace-global free-form notes; they are not file/line anchored, not threaded, not status-trackable, and not scoped to a PR. Borrowing the block model would re-implement all of the above on a parser with known edge cases. + +- **Alternative C: Post comments to GitHub via the provider.** Rejected by the user's brief and by Citadel's local-first model (provider may be unhealthy; comments must be readable offline). + +- **Alternative D: Ship the full inline-diff comment renderer in v1.** Rejected as too large for one PR. + +- **Alternative E: Refactor `runCommandHookForDiagnostics` to resolve on timeout instead of rejecting.** Considered. Would be cleaner long-term but touches every existing hook runner (setup/teardown/apps/action), expanding blast radius. Rejected for v1; logged as a follow-up. The operations service catches the rejection text instead. + +## Implementation steps + +### Step 1 — Update specs (FIRST) + +- Edit `specs/B.4-git-pr-ci-diff.md`: change Human Review items [ ] 3, 4, 5 to `[~]` with a note that the data store + MCP surface ship in this PR. Add a "Retention" note: `review_suggestion_runs` and `activity_events` have no retention policy in v1; bulk-resolve / delete on merged PRs is a follow-up. +- Edit `specs/B.6-providers-hooks-config.md`: add `workspace.requestReview` to the Hooks subsection. Note its dedicated output schema and default-blocking behavior. +- Edit `specs/B.7-operations-activity-mcp.md`: append to "MCP tool inventory" — `list_review_comments` under read-only; `add_review_comment`, `update_review_comment`, `delete_review_comment` (destructive), `request_review` under daemon-mediated. Note that MCP `add_review_comment` rejects caller-supplied `author`; daemon stamps `agent:`. + +### Step 2 — Contracts (`packages/contracts/src/review.ts` — NEW) + +Create a new file under `packages/contracts/src/review.ts` and re-export from `index.ts` (mirroring the `scratchpad.ts` split at line ~786 of `index.ts`). Schemas: + +- `ReviewSuggestionKindSchema = z.enum(["reviewer","checklist","note","warning"])`. +- `ReviewSuggestionSchema = z.object({ id: z.string().min(1), kind: ReviewSuggestionKindSchema, label: z.string().min(1).max(200), detail: z.string().max(2000).nullable().default(null), url: z.string().url().nullable().default(null), metadata: z.record(z.unknown()).default({}) })`. +- `ReviewSuggestionsOutputSchema = z.object({ suggestions: z.array(ReviewSuggestionSchema).max(50).default([]), generatedAt: z.string().nullable().default(null), metadata: z.record(z.unknown()).default({}) })` — each nullable field declared with `.nullable().default(null)` so a fully-omitted object parses without missing-key errors. NO outer `.default(...)`. +- `ReviewCommentStatusSchema = z.enum(["open","resolved"])`. +- `ReviewCommentSchema = z.object({ id, workspaceId, filePath: z.string().max(512).nullable().default(null), lineStart: z.number().int().min(1).nullable().default(null), lineEnd: z.number().int().min(1).nullable().default(null), side: z.enum(["LEFT","RIGHT"]).nullable().default(null), author: z.string().max(80), body: z.string().min(1).max(8000), status: ReviewCommentStatusSchema.default("open"), createdAt, updatedAt, deletedAt: z.string().nullable().default(null) })` with a `.superRefine` enforcing `lineEnd >= lineStart` when both set, and rejecting `lineStart/lineEnd/side` when `filePath` is null. NOTE: no `parentId` and no `prUrl` columns — v1 is flat and pr_url is joined from the workspace. +- `ReviewSuggestionRunSchema = z.object({ id, workspaceId, hookId, status: z.enum(["succeeded","failed","timed_out"]), durationMs: z.number().int().nullable().default(null), exitStatus: z.number().int().nullable().default(null), output: ReviewSuggestionsOutputSchema.nullable().default(null), stderr: z.string().nullable().default(null), error: z.string().nullable().default(null), createdAt })`. +- `RequestReviewPayloadSchema = z.object({ event: z.literal("workspace.requestReview"), workspace, repo, pr: z.object({ url: z.string().nullable(), branch: z.string(), baseBranch: z.string() }), diff: z.object({ files: z.array(z.string()), addedLines: z.number().int().nonnegative(), deletedLines: z.number().int().nonnegative(), truncated: z.boolean() }) })` — `files` is paths only to keep the payload bounded. + +Re-export from `packages/contracts/src/index.ts`: +```ts +export * from "./review.js"; +``` +Add the inferred TS types alongside the re-export (or inside `review.ts`). + +### Step 3 — DB schema (`packages/db/src/migrate.ts`) + +Migration strategy (per repo extension): + +1. **Operation list.** + - `CREATE TABLE IF NOT EXISTS review_comments(...)`. + - `CREATE INDEX IF NOT EXISTS idx_review_comments_workspace ON review_comments(workspace_id, created_at)`. + - `CREATE INDEX IF NOT EXISTS idx_review_comments_status ON review_comments(workspace_id, status)`. + - `CREATE TABLE IF NOT EXISTS review_suggestion_runs(...)`. + - `CREATE INDEX IF NOT EXISTS idx_review_suggestion_runs_workspace ON review_suggestion_runs(workspace_id, created_at)`. + - `INSERT OR IGNORE INTO schema_migrations(version, name, applied_at) VALUES (8, 'review-system', datetime('now'))`. +2. **Classification.** All operations are **additive** (new tables + new indexes). No `DROP`, no `ALTER`, no type narrowing. Safe in one step. +3. **`schema_migrations` row.** New version `8`, name `review-system`. Strictly greater than the current max (`7`). +4. **`PRAGMA foreign_keys = ON;` preservation.** Connection open path is unchanged. FKs to `workspaces(id)` declared with `ON DELETE CASCADE` for hard deletes; archived workspaces are filtered at the query layer instead (see Step 5 — `listReviewComments` joins `workspaces.archived_at IS NULL`). +5. **Operator data implications.** Every existing install starts with zero rows in both new tables — additive. No backfill, no constraint-violation risk. The migration is a no-op on its second run. + +Concrete DDL: + +```sql +CREATE TABLE IF NOT EXISTS review_comments ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL REFERENCES workspaces(id) ON DELETE CASCADE, + file_path TEXT, + line_start INTEGER, + line_end INTEGER, + side TEXT CHECK (side IS NULL OR side IN ('LEFT','RIGHT')), + author TEXT NOT NULL, + body TEXT NOT NULL, + status TEXT NOT NULL CHECK (status IN ('open','resolved')), + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL, + deleted_at TEXT +); + +CREATE TABLE IF NOT EXISTS review_suggestion_runs ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL REFERENCES workspaces(id) ON DELETE CASCADE, + hook_id TEXT NOT NULL, + status TEXT NOT NULL CHECK (status IN ('succeeded','failed','timed_out')), + duration_ms INTEGER, + exit_status INTEGER, + output_json TEXT, -- parsed ReviewSuggestionsOutput on success ONLY; NULL on failure + stderr TEXT, -- raw stderr tail (success or failure) + error TEXT, -- failure message (NULL on success) + created_at TEXT NOT NULL +); +``` + +### Step 4 — DB accessors (`packages/db/src/`) + +- `listReviewComments(workspaceId, opts: { status?: 'open'|'resolved'|'all'; includeDeleted?: boolean; includeArchived?: boolean })` — by default joins `workspaces` and excludes archived; ordering newest-first. +- `insertReviewComment(input)` returns the persisted row. +- `updateReviewComment(id, patch, ifUpdatedAtMatches)` — returns `{ kind: 'updated', row }` or `{ kind: 'conflict', latest }` when `updated_at` differs from `ifUpdatedAtMatches`. Body/status only. +- `softDeleteReviewComment(id, ifUpdatedAtMatches)` — same conflict semantics; sets `deleted_at`. +- `insertReviewSuggestionRun(input)`. +- `latestReviewSuggestionRun(workspaceId)`. + +### Step 5 — Hook integration (`packages/hooks/src/index.ts`) + +- Add `parseReviewSuggestionsOutput(stdout)` mirroring `parseHookOutput` but validating against `ReviewSuggestionsOutputSchema`. Empty trimmed stdout returns `null`; the operations service interprets `null` as "succeeded with zero suggestions". Truncated stdout (>64KB hits the slice limit) is detected by JSON.parse failure — surface a clear error message that mentions stdout truncation. +- No other changes (reuse `runCommandHookForDiagnostics`). + +### Step 6 — Operations service (`packages/operations/src/review-system.ts` — NEW) + +- Export the existing `commandHook(hook, workspacePath, config)` helper from `packages/operations/src/hooks-runner.ts` (one-line export change) and import it in `review-system.ts`. +- `requestReviewForWorkspace(deps, workspaceId)` implements the flow described in "Implementation approach § 5". Catches the runner rejection; detects timeout via message text (`"Hook timed out after"`) and via wall-clock elapsed-time backup signal. +- Comment service functions `listReviewComments`, `addReviewComment`, `updateReviewComment`, `deleteReviewComment` (each with activity logging on success; no activity on read). +- All mutations are exposed via the daemon HTTP routes AND the daemon-side MCP path; both call into this module. + +### Step 7 — Config wiring (`packages/config/src/index.ts`) + +Concrete edits, by line range: + +- **L41-50** (`HookEventSchema`): append `"workspace.requestReview"` to the enum. +- **L65-68** (`HookConfigSchema.transform`): change the blocking-default array to `["workspace.setup", "workspace.teardown", "workspace.requestReview"]`. +- **L133-140** (`repoDefaults` inline object): add `requestReviewHookIds: z.array(z.string()).default([])`. +- **L140** (the `.default({...})` argument): add `requestReviewHookIds: []`. +- **L169-176** (`.superRefine` body): add `validateHookReferences(context, hooksById, config.repoDefaults.requestReviewHookIds, "workspace.requestReview", ["repoDefaults", "requestReviewHookIds"])`. + +Add unit tests (in `packages/config/src/index.test.ts`) asserting: +- An authored `workspace.requestReview` hook without an explicit `blocking` resolves to `blocking: true`. +- `requestReviewHookIds` referencing a non-`workspace.requestReview` hook surfaces a validation error. +- Loading a config without the new field still parses (defaults to `[]`). + +### Step 7b — Extend HookEvent consumers (operations diagnostics + Settings UI) + +Two non-switch consumers of the hook-event list enumerate events as literal arrays. Both must learn about the new variant or the feature is unusable in the cockpit: + +1. **`packages/operations/src/helpers.ts:392-415` (`listHookDiagnostics`)** — extend the `events` array (L392-397) with `"workspace.requestReview"`. Extend the ternary chain (L399-406) so the new event picks `input.requestReviewHookIds`. Thread `requestReviewHookIds: string[]` through the helper's input type. Update the callsite in `packages/operations/src/index.ts` (the existing `listHookDiagnostics` invocation — search for it; it's the only caller) to pass `requestReviewHookIds: repo.requestReviewHookIds`. The diagnostics endpoint (`/api/repos/:id/hook-diagnostics`) and the cockpit repo-settings hook diagnostics panel (`apps/web/src/routes/repo-settings.tsx`) will then include validation/health state for `workspace.requestReview` hooks. + +2. **`apps/web/src/structured-config.tsx:22-30` and L61-70** — add `"workspace.requestReview"` to the HookConfig event TS union (L22-30) and to the `HOOK_EVENTS` runtime array (L61-70). Also extend the `ConfigResponse.config.repoDefaults` type (L53) to include `requestReviewHookIds: string[]` (it already silently lags behind `appHookIds`/`actionHookIds`; we add `requestReviewHookIds` to be consistent for the new event, even if the existing structured-config repo-defaults editor doesn't render it yet — type accuracy matters for the power-user escape hatch). + +Tests added: +- `packages/operations/src/index.test.ts` (or a colocated helpers test): assert `listHookDiagnostics` includes a `workspace.requestReview` hook's diagnostic when authored. +- `apps/web/src/structured-config.test.ts` (if present) or a colocated component test: assert the event dropdown includes `workspace.requestReview`. + +### Step 8 — Daemon routes (`apps/daemon/src/review-routes.ts` — NEW) + +Routes listed in "Implementation approach § 6". Each: +- Validates input with zod. +- Calls the operations service. +- Returns JSON with consistent error shape (`{ error: { code, message } }`). +- 409 on `updated_at` conflict (PATCH/DELETE). +- POST `review-comments` ignores any `author` field in the request body and stamps `'operator'`. +- Mounts via the existing daemon router-registration pattern. + +### Step 9 — MCP tools (`packages/mcp/src/index.ts` + `apps/daemon/src/daemon-mcp-tool.ts`) + +- Register tool definitions: `list_review_comments` (read-only), `add_review_comment`, `update_review_comment`, `delete_review_comment` (destructive), `request_review` (daemon-mediated). +- Input schemas: + - `list_review_comments`: `{ workspaceId, status?, includeDeleted? }`. + - `add_review_comment`: `{ workspaceId, body, filePath?, lineStart?, lineEnd?, side? }` — **no `author` field**. Schema validation rejects extra keys via `.strict()` or `.passthrough(false)` equivalent. + - `update_review_comment`: `{ id, body?, status?, ifUpdatedAtMatches }`. + - `delete_review_comment`: `{ id, ifUpdatedAtMatches }`. + - `request_review`: `{ workspaceId }`. +- Daemon-side `callDaemonMcpTool()`: + - Stamps `author = 'agent:'` from the request context (fallback `'agent:unknown'`). + - On conflict, returns `{ error: 'conflict', latest: { ... } }` so the agent can re-read and retry. + - Delegates to the same operations service used by HTTP routes. + +### Step 10 — Cockpit UI (`apps/web/src/`) + +- Add a `Review` tab between `Diff` and any other PR-related tabs. +- `RequestReviewPanel.tsx`: button + status (idle/loading/error/no-hook/empty/success) + suggestion list rendered by `kind`. On mount, fetch latest suggestion run for hydration. Debounce repeated clicks while a request is in-flight. +- `ReviewCommentsTab.tsx`: composer + list. Composer body textarea + optional file selector populated from `WorkspaceDiff` + optional line range. Resolve/edit/delete actions inline. Edit/delete carry the comment's last-read `updatedAt` as `ifUpdatedAtMatches`. On 409, refresh the comment and show a "this comment was edited elsewhere" banner. +- Disabled state when no hook configured — tooltip explains where to declare `workspace.requestReview`. +- Comment bodies render as plain text (`textContent` / React's safe text rendering). Never `dangerouslySetInnerHTML`. + +### Step 11 — Domain language + +Code identifiers and API fields use the terms from `specs/A-shared-definitions.md`: Workspace, Repository, Hook, Activity event, Operation, Provider. UI copy uses "Review", "Suggestion", "Comment". + +## QA/Test Strategy + +### Layer evaluation + +| Layer | Verdict | Details | +|-------|---------|---------| +| Unit (Vitest) | **Required** | Contracts schema validation; DB accessors (incl. conflict semantics); operations service (request-review flow + comment CRUD); hook output parser; MCP tool dispatch (in-process + daemon-mediated branches); config wiring (blocking default + validate references). | +| E2E (Playwright) | **Required** | Operator-flow: register repo with a request-review hook (absolute-path script fixture), create workspace, open inspector, click Request review, see suggestions, add/edit/resolve a comment with concurrency-token round-trip, verify persistence after reload. | + +No "integration" layer per repo convention. Daemon HTTP routes are exercised end-to-end via Playwright + via daemon-route Vitest specs (see `scheduled-agent-routes.test.ts` pattern). + +### New tests to add + +**Contracts (`packages/contracts/src/review.test.ts` — NEW; existing `index.test.ts` may not exist or may grow past 800 lines, so colocate with `review.ts`):** +- `ReviewSuggestionsOutputSchema` accepts an empty payload, accepts a fully-specified payload, rejects suggestions over the max-50 limit, rejects unknown `kind` values, rejects suggestions where `label` exceeds 200 chars. +- `ReviewCommentSchema` rejects `lineEnd < lineStart`; rejects negative line numbers; rejects `body` empty or over 8000 chars; rejects `lineStart` without `filePath`; accepts a PR-level comment with all anchors null. + +**Config (`packages/config/src/index.test.ts` — extend):** +- `HookEventSchema` accepts `workspace.requestReview`. +- An authored `workspace.requestReview` hook without `blocking` resolves to `blocking: true`. +- `requestReviewHookIds` validation rejects references to hooks whose `event` is not `workspace.requestReview`. +- Loading a config without `requestReviewHookIds` defaults it to `[]`. + +**DB (`packages/db/src/review-comments.test.ts`, `packages/db/src/review-suggestion-runs.test.ts` — NEW):** +- `insertReviewComment` → `listReviewComments` round-trip preserves all fields including null anchors. +- `updateReviewComment` updates `updated_at` strictly later than `created_at`. +- `updateReviewComment` with a stale `ifUpdatedAtMatches` returns `{ kind: 'conflict', latest }` and does NOT update the row. +- `softDeleteReviewComment` hides the row from default lists; visible with `includeDeleted: true`. Verify physical row still exists via raw SQL. +- `listReviewComments` excludes comments belonging to archived workspaces by default; includes them when `includeArchived: true`. +- Cascade: hard-deleting a workspace removes its comments and suggestion runs. +- Migration idempotency: applying the migration twice is a no-op. `schema_migrations` includes `(8, 'review-system', …)`. + +**Hooks (`packages/hooks/src/index.test.ts` — extend):** +- `parseReviewSuggestionsOutput("")` returns `null`. +- `parseReviewSuggestionsOutput(validJson)` returns the parsed payload. +- `parseReviewSuggestionsOutput(invalidJson)` throws with a clear error. +- `parseReviewSuggestionsOutput(stdoutThatHitsTheSliceLimit)` produces a parse error that mentions truncation. + +**Operations (`packages/operations/src/review-system.test.ts` — NEW):** +- `requestReviewForWorkspace`: + - No hook configured → returns `{ kind: 'no-hook' }`, no rows written, no activity row. + - Hook succeeds → writes one `review_suggestion_runs` row (status `succeeded`, parsed JSON in `output_json`), appends ONE `activity_events` row (type `hook.workspace.requestReview`). + - Hook times out → writes a row with status `timed_out`, no parsed JSON, error mentions timeout, appends ONE activity row (type `hook.workspace.requestReview.failed`). Detected via runner rejection message. + - Hook returns invalid JSON → writes status `failed`, no parsed JSON, error explains validation failure, ONE activity row (failed). + - Hook returns empty stdout → writes status `succeeded` with `output_json` = JSON-encoded `{ suggestions: [], generatedAt: null, metadata: {} }` (or `NULL` — pick and assert). +- `addReviewComment`: persists; appends activity row. Force-`'operator'`-author path (HTTP) ignores caller-supplied author. Force-`'agent:'`-author path (MCP) stamps the runtime id. +- `updateReviewComment` with stale token returns conflict; with fresh token updates; resolving appends `review.comment.resolved`. +- `deleteReviewComment` soft-deletes; idempotent (second call with same stale token returns conflict; with fresh token returns updated tombstone). +- Listing: `status: 'open'` excludes resolved and soft-deleted; `status: 'all'` excludes only soft-deleted; explicit `includeDeleted: true` includes them. + +**Daemon (`apps/daemon/src/review-routes.test.ts` — NEW; follow `scheduled-agent-routes.test.ts` pattern):** +- `POST /api/workspaces/:id/review-requests`: 200 with parsed suggestions; 404 on unknown workspace; structured error code `no-hook` when no hook configured. +- `POST /api/workspaces/:id/review-comments`: 200 with persisted id; ignores caller `author` and stamps `'operator'`; 400 on missing body / invalid anchor; 404 on unknown workspace. +- `PATCH /api/review-comments/:id`: 200 on success; 409 on stale `ifUpdatedAtMatches`; 400 on attempting to edit a deleted comment. +- `DELETE /api/review-comments/:id`: 204 on success; 409 on stale token; second delete of an already-deleted comment returns 409 unless caller passes the latest `updated_at`. +- `GET /api/workspaces/:id/review-suggestions`: latest run or null when none. + +**MCP (`packages/mcp/src/index.test.ts` — extend):** +- `list_review_comments` returns the same rows as the DB accessor. +- `add_review_comment` schema rejects a request that includes `author`. +- Mutating tools return `{ error: 'mutating_tool_requires_daemon' }` from the in-process path. +- Daemon-mediated `add_review_comment` stamps `author = 'agent:'`. +- Daemon-mediated `update_review_comment` with stale token returns `{ error: 'conflict', latest }`. +- `request_review` returns `{ error: ... }` from the in-process path; daemon-mediated path calls through. + +**Cockpit UI (`apps/web/src/RequestReviewPanel.test.ts`, `ReviewCommentsTab.test.ts` — NEW):** +- `RequestReviewPanel`: button disabled with tooltip when no hook configured; renders loading / error / no-hook / empty / success states; renders suggestions grouped by `kind`; debounces repeated clicks while in-flight. +- `ReviewCommentsTab`: composer requires a body; submitting POSTs to the route and prepends the new comment; resolved comments collapse by default; deleting removes the comment optimistically; 409 surfaces an in-place banner. +- XSS smoke: rendering a comment body with `` produces no script execution and renders as literal text. + +**E2E (`e2e/review-system.spec.ts` — NEW):** +- Fixture: at test setup, write a tiny POSIX shell script at `${testRoot}/tests/fixtures/hooks/request-review.sh` (`chmod +x`), then write a `citadel.config.json` with `hooks[].command = path.resolve(testRoot, "tests/fixtures/hooks/request-review.sh")` and `hooks[].cwd = testRoot` (absolute path — required by `HookConfigSchema.cwd` refinement). The script emits a hard-coded `ReviewSuggestionsOutput`. +- Create a workspace, navigate to its inspector, click Request review, assert the suggestions render. Add a PR-level comment, reload, assert the comment persists. Resolve the comment, assert it collapses. Verify the activity feed shows `hook.workspace.requestReview` (singular — no duplicate `review.requested` row) and `review.comment.added`. + +### Existing tests to update + +- `apps/web/src/inspector.test.ts` — extend if the inspector tab list is asserted there; otherwise no change. +- `apps/daemon/src/mcp-routes.test.ts` (if present) — extend the tool inventory assertion to include the new tools. +- `scripts/dev/smoke.ts` — extend with a round-trip probe: POST a comment, GET the list (expect length 1), DELETE (expect 204), GET again (expect length 0). + +### Assertions to add/change/tighten + +- DB accessor tests assert `updated_at >= created_at` and that a no-op update still bumps `updated_at` (or doesn't — pick the contract and assert; preferred: bumps). +- Soft-delete tests assert physical row still exists in the table after `softDelete`. +- Conflict tests assert the row was NOT mutated when the token was stale. +- Activity-row tests assert exactly ONE row per logical event (no duplicate `review.requested` + `hook.workspace.requestReview`). +- E2E asserts the activity feed event-type spelling matches the canonical names. + +### Failure modes / edge cases / regression risks + +- **Hook output too large.** `runCommandHookForDiagnostics` caps stdout at 64KB via `.slice(-65536)` — if a hook emits banner text + JSON at the end, slice fits; but a hook that emits >64KB of JSON gets a corrupt leading-edge parse failure. Mitigation: parser surfaces a clear truncation error. Test covers this. +- **Hook deadlock or runaway.** Timeout cleanly rejects via SIGTERM. Operations service catches and writes `timed_out`. Test covers this. +- **Workspace archived (not deleted) while a comment exists.** `listReviewComments` filters archived by default — comment is hidden from MCP and UI but the row stays for resurrection-after-unarchive. Test covers both visibility states. +- **Workspace hard-deleted while a request-review is in flight.** FK cascade on `workspaces(id)` removes related rows; if the service is mid-`INSERT`, the constraint fails — operations service catches and logs without crashing. Test by stubbing workspace removal between payload build and insert. +- **Comment body XSS.** All bodies render as plain text. Tested. +- **Concurrent comment edits.** `ifUpdatedAtMatches` (optimistic concurrency) protects mutations from stale-read clobbering. 409 returned; agent must re-read and retry. Tested at every layer. +- **Hook returning invalid `url` in a suggestion.** Schema rejects; the whole payload fails parse; run records `failed`; no partial suggestions stored. Tested. +- **Activity log volume.** Every comment mutation appends a row. Only successes + failures, not reads. No retention policy in v1 — documented in the spec. Follow-up: bulk-resolve / archive on merged PRs. +- **MCP author spoofing.** Schema-rejected at request-time. Tested. +- **Concurrent in-flight `request_review` runs.** Two callers (operator + agent) both spawn hooks; both rows are stored. UI hydrates from `latestReviewSuggestionRun` (singular). Documented behavior. +- **Diff truncation.** Hook payload's `diff.files` is paths only; total payload bounded. Full diff content remains available via `/api/workspaces/:id/diff` for hooks that need it. + +### Adversarial analysis + +- **How could this fail in production?** Most likely: a buggy `workspace.requestReview` hook returns malformed JSON or hangs. Both are bounded (timeout + JSON parse). Second most likely: agent + operator concurrent edits — now blocked by optimistic concurrency. +- **What user actions trigger unexpected behavior?** Clicking Request review while a previous run is in flight — debounce + per-call rows. Editing a comment after an agent has edited it — 409 with explicit re-read banner. +- **What existing behavior could break?** Existing hook events are unchanged. `repoDefaults` gains a defaulted optional field — existing config files load unchanged. `runCommandHookForDiagnostics` is unchanged. The new variant is silently filtered by event-equality checks in `hooks-runner.ts:41`, which is the desired behavior (request-review hooks should not fire on setup events). Two literal enumerations of hook events DO need updating — `listHookDiagnostics` (`packages/operations/src/helpers.ts:392`) and the Settings UI hook dropdown (`apps/web/src/structured-config.tsx:61`). Both are explicitly addressed in Step 7b; without that step the new hook can be authored only by hand-editing JSON and its health is invisible to the diagnostics panel. +- **Which tests credibly catch failures?** Operations-service tests cover hook misbehavior. Migration idempotency test catches double-apply. Config tests cover defaulted-field upgrade and validation-references. +- **What gaps remain?** v1 has no full-screen diff-anchored review surface; no comment notifications; no permission model on edit/delete (anyone with daemon access can edit any comment via HTTP — MCP path stamps the author); no rate limiting on Request review; no retention policy. Documented as follow-ups. + +## Tests + +Test files to create or modify (TDD order — write before the corresponding implementation step): + +1. `packages/contracts/src/review.test.ts` — NEW (schema validation). +2. `packages/config/src/index.test.ts` — extend (event + blocking default + validation references + defaulted-field). +3. `packages/db/src/review-comments.test.ts` — NEW. +4. `packages/db/src/review-suggestion-runs.test.ts` — NEW. +5. `packages/db/src/migrate.test.ts` — extend (idempotency + version 8 row). +6. `packages/hooks/src/index.test.ts` — extend (parser + truncation). +7. `packages/operations/src/review-system.test.ts` — NEW. +8. `packages/mcp/src/index.test.ts` — extend (tool inventory + author-spoof + conflict). +9. `apps/daemon/src/review-routes.test.ts` — NEW. +10. `apps/web/src/RequestReviewPanel.test.ts` — NEW. +11. `apps/web/src/ReviewCommentsTab.test.ts` — NEW. +12. `e2e/review-system.spec.ts` — NEW (fixture absolute-path script under `tests/fixtures/hooks/request-review.sh`). + +## Schema or contract generation + +Citadel does not have a code-generation step for contracts or schemas — `packages/contracts` is hand-written TypeScript with zod. After contract additions, run `pnpm build` (covered by `make check`) so project-reference consumers re-typecheck. No schema generator command beyond `make check`. + +## Verification + +Before opening the PR, the implementation agent must run and pass: + +- `make check` — full local gate: arch boundaries, file size (verify `packages/contracts/src/index.ts` stays ≤800 lines; the new `review.ts` is well under), typecheck, biome lint, vitest, coverage (≥90% on core/backend/shared), dep policy, build. +- `make smoke` — required (new daemon HTTP surface + new MCP tools). Extend `scripts/dev/smoke.ts` to probe `GET /api/mcp/status` (assert new tool names appear) and to do a round-trip on `review-comments` against a seeded workspace: POST → GET (expect length 1) → DELETE (expect 204) → GET (expect length 0). +- `make e2e` — Playwright happy-path including the new `e2e/review-system.spec.ts`. +- `make performance` — not required (no startup/hot-path changes). From d4374ea367cc0adbf309d27720ac6ced33161913 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:24:50 +0000 Subject: [PATCH 02/18] spec(review-system): document the citadel-native review surface 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) --- specs/B.4-git-pr-ci-diff.md | 21 ++++++++++++++++----- specs/B.6-providers-hooks-config.md | 1 + specs/B.7-operations-activity-mcp.md | 8 ++++++-- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/specs/B.4-git-pr-ci-diff.md b/specs/B.4-git-pr-ci-diff.md index ebe97f71..b1a898a3 100644 --- a/specs/B.4-git-pr-ci-diff.md +++ b/specs/B.4-git-pr-ci-diff.md @@ -51,14 +51,25 @@ [ ] 10. The diff/review surface is read-only. [ ] 11. The inspector `Diff` tab shows the changed files list with additions/deletions per file in a compact list. -## Human Review (Planned) +## Human Review [ ] 1. A future full-screen *Human Review* mode is reachable from the inspector `Diff` tab. [ ] 2. Human Review presents files in a GitHub-style review surface. -[ ] 3. Human Review allows leaving file/line comments. -[ ] 4. Comments are visible to the active agent session as structured input. -[ ] 5. Human Review remains scoped to the selected workspace. +[~] 3. Inspector hosts a `Review` tab that accepts workspace-level and file/line comments persisted in Citadel's SQLite (not posted to GitHub). The full diff-anchored inline renderer is still planned (item 2); v1 ships a flat list with file:line anchors shown above each comment. +[~] 4. Comments are visible to the active agent session as structured input via MCP tools `list_review_comments`, `add_review_comment`, `update_review_comment`, `delete_review_comment`. The MCP path stamps `author = 'agent:'`; the cockpit HTTP path stamps `author = 'operator'`. Mutations carry an `ifUpdatedAtMatches` token for optimistic concurrency (409 on mismatch). +[~] 5. Comments are scoped to the selected workspace. Archived workspaces are filtered out of default listings; `includeArchived` is an explicit opt-in. + +### Request Review + +[~] 1. A repo can configure a `workspace.requestReview` hook (event added to `HookEventSchema` in `@citadel/config`). The hook receives `{ workspace, repo, pr, diff: { files, addedLines, deletedLines, truncated } }` on stdin and returns a validated `ReviewSuggestionsOutput` payload on stdout. +[~] 2. The inspector exposes a "Request review" button next to (or above) the `Diff`/`Review` surface. When no `workspace.requestReview` hook is configured, the button is disabled with a tooltip pointing operators at the Settings UI hook editor. +[~] 3. Each invocation records one `activity_events` row (`hook.workspace.requestReview` on success, `hook.workspace.requestReview.failed` on failure or timeout) plus a `review_suggestion_runs` row that preserves parsed output (success), raw stderr tail, and error message (failure). +[ ] 4. Suggestion entries carry a `kind` (`reviewer`, `checklist`, `note`, `warning`), `label`, optional `detail`, optional `url`, and optional `metadata`. + +### Retention + +v1 has no retention policy for `review_suggestion_runs` or `activity_events`; both grow unbounded. Bulk-resolve or auto-archive on merged PRs is a follow-up. --- -keywords: git, branch, status, pr, review, checks, ci, diff, additions, deletions, provider +keywords: git, branch, status, pr, review, checks, ci, diff, additions, deletions, provider, comments, mcp, request review, hook diff --git a/specs/B.6-providers-hooks-config.md b/specs/B.6-providers-hooks-config.md index d9a1cd14..08cd4b0d 100644 --- a/specs/B.6-providers-hooks-config.md +++ b/specs/B.6-providers-hooks-config.md @@ -53,6 +53,7 @@ Rules: [ ] 3. Teardown hooks are configured per repo. [ ] 4. App/link discovery hooks are configured per repo. [ ] 5. Action hooks are configured per repo. +[~] 5a. Request-review hooks are configured per repo via `repoDefaults.requestReviewHookIds`. The `workspace.requestReview` event in `HookEventSchema` returns a dedicated `ReviewSuggestionsOutput` payload (not the generic `HookOutput`). Authored hooks for this event default to `blocking: true` (same as setup/teardown). Diagnostics include `workspace.requestReview` in `listHookDiagnostics` and in the Settings UI hook editor dropdown. [ ] 6. Hooks receive structured workspace/repo/provider context. [ ] 7. Hooks return structured JSON. [ ] 8. Hook output is validated before it appears in the UI. diff --git a/specs/B.7-operations-activity-mcp.md b/specs/B.7-operations-activity-mcp.md index 8f4d84a3..60aa0039 100644 --- a/specs/B.7-operations-activity-mcp.md +++ b/specs/B.7-operations-activity-mcp.md @@ -50,12 +50,16 @@ Read-only: - `inspect_status`, `list_repos`, `list_workspaces`, `list_agent_sessions`, `list_provider_health`, `list_runtimes`, `list_workspace_links`, - `inspect_readiness`, `read_agent_output`. + `inspect_readiness`, `read_agent_output`, `list_review_comments`. Daemon-mediated (run through the operation service so they obey the same hook, activity, and safety model as the UI): - `create_workspace`, `start_agent_session`, `send_agent_message`, `stop_agent_session` (destructive), `archive_workspace`, - `remove_workspace` (destructive), `reconcile` (destructive). + `remove_workspace` (destructive), `reconcile` (destructive), + `request_review`, `add_review_comment`, `update_review_comment` (destructive), + `delete_review_comment` (destructive). + +`add_review_comment` schema rejects a caller-supplied `author` field; the daemon stamps `author = 'agent:'`. The cockpit HTTP route stamps `author = 'operator'`. `update_review_comment` and `delete_review_comment` require an `ifUpdatedAtMatches` token; the daemon returns `{ error: 'conflict', latest }` on mismatch. For interactive runtimes like Claude Code, both `start_agent_session` (with a `prompt`) and `send_agent_message` deliver text into the backing tmux pane via a paste buffer followed by Enter. This guarantees the agent actually receives and processes the prompt — it is not just typed into the input box. From 19fc8e6f72609c127e7a5c9f3d36dcf58da0a860 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:27:33 +0000 Subject: [PATCH 03/18] feat(contracts): add review system schemas (suggestions, comments, run) 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) --- packages/contracts/src/index.ts | 2 + packages/contracts/src/review.test.ts | 212 ++++++++++++++++++++++++++ packages/contracts/src/review.ts | 112 ++++++++++++++ 3 files changed, 326 insertions(+) create mode 100644 packages/contracts/src/review.test.ts create mode 100644 packages/contracts/src/review.ts diff --git a/packages/contracts/src/index.ts b/packages/contracts/src/index.ts index 22fe99d0..2ce18165 100644 --- a/packages/contracts/src/index.ts +++ b/packages/contracts/src/index.ts @@ -787,6 +787,8 @@ export type { ScratchpadSnapshot, ScratchpadHistorySource } from "./scratchpad.j export type { ScratchpadHistoryEntry, ScratchpadHistorySummary } from "./scratchpad.js"; export type { ScratchpadBlock, ScratchpadBlockSummary, ScratchpadBlockPosition } from "./scratchpad.js"; +export * from "./review.js"; + export type ApiError = { error: string; detail?: string; diff --git a/packages/contracts/src/review.test.ts b/packages/contracts/src/review.test.ts new file mode 100644 index 00000000..61beebe1 --- /dev/null +++ b/packages/contracts/src/review.test.ts @@ -0,0 +1,212 @@ +import { describe, expect, it } from "vitest"; +import { + RequestReviewPayloadSchema, + ReviewCommentSchema, + ReviewSuggestionRunSchema, + ReviewSuggestionSchema, + ReviewSuggestionsOutputSchema, +} from "./review.js"; + +describe("ReviewSuggestionsOutputSchema", () => { + it("parses a fully-omitted object via inner defaults", () => { + const parsed = ReviewSuggestionsOutputSchema.parse({}); + expect(parsed).toEqual({ suggestions: [], generatedAt: null, metadata: {} }); + }); + + it("parses a fully specified payload", () => { + const parsed = ReviewSuggestionsOutputSchema.parse({ + suggestions: [ + { + id: "s1", + kind: "reviewer", + label: "@alice", + detail: "module owner", + url: "https://example.com/owners", + }, + ], + generatedAt: "2026-01-01T00:00:00.000Z", + metadata: { source: "codeowners" }, + }); + expect(parsed.suggestions).toHaveLength(1); + expect(parsed.suggestions[0]?.kind).toBe("reviewer"); + expect(parsed.generatedAt).toBe("2026-01-01T00:00:00.000Z"); + }); + + it("rejects more than 50 suggestions", () => { + const tooMany = Array.from({ length: 51 }, (_, idx) => ({ + id: `s${idx}`, + kind: "note" as const, + label: "x", + })); + expect(() => ReviewSuggestionsOutputSchema.parse({ suggestions: tooMany })).toThrow(); + }); + + it("rejects an unknown suggestion kind", () => { + expect(() => + ReviewSuggestionSchema.parse({ id: "s", kind: "bogus", label: "x" }), + ).toThrow(); + }); + + it("rejects suggestion label over 200 chars", () => { + expect(() => + ReviewSuggestionSchema.parse({ id: "s", kind: "note", label: "x".repeat(201) }), + ).toThrow(); + }); + + it("rejects suggestion url that is not a URL", () => { + expect(() => + ReviewSuggestionSchema.parse({ id: "s", kind: "reviewer", label: "y", url: "not-a-url" }), + ).toThrow(); + }); + + it("defaults nullable fields to null instead of undefined", () => { + const parsed = ReviewSuggestionSchema.parse({ id: "s", kind: "note", label: "x" }); + expect(parsed.detail).toBeNull(); + expect(parsed.url).toBeNull(); + expect(parsed.metadata).toEqual({}); + }); +}); + +describe("ReviewCommentSchema", () => { + const now = "2026-01-01T00:00:00.000Z"; + const base = { + id: "rc_1", + workspaceId: "ws_1", + author: "operator", + body: "looks good", + status: "open" as const, + createdAt: now, + updatedAt: now, + }; + + it("accepts a PR-level comment with all anchors null", () => { + const parsed = ReviewCommentSchema.parse(base); + expect(parsed.filePath).toBeNull(); + expect(parsed.lineStart).toBeNull(); + expect(parsed.lineEnd).toBeNull(); + expect(parsed.side).toBeNull(); + expect(parsed.deletedAt).toBeNull(); + }); + + it("accepts a file/line scoped comment", () => { + const parsed = ReviewCommentSchema.parse({ + ...base, + filePath: "src/main.ts", + lineStart: 10, + lineEnd: 12, + side: "RIGHT", + }); + expect(parsed.filePath).toBe("src/main.ts"); + expect(parsed.lineEnd).toBe(12); + }); + + it("rejects lineEnd < lineStart", () => { + expect(() => + ReviewCommentSchema.parse({ ...base, filePath: "a.ts", lineStart: 10, lineEnd: 5 }), + ).toThrow(); + }); + + it("rejects negative line numbers", () => { + expect(() => ReviewCommentSchema.parse({ ...base, filePath: "a.ts", lineStart: -1 })).toThrow(); + }); + + it("rejects an empty body", () => { + expect(() => ReviewCommentSchema.parse({ ...base, body: "" })).toThrow(); + }); + + it("rejects a body over 8000 chars", () => { + expect(() => ReviewCommentSchema.parse({ ...base, body: "x".repeat(8001) })).toThrow(); + }); + + it("rejects lineStart without filePath", () => { + expect(() => ReviewCommentSchema.parse({ ...base, lineStart: 1 })).toThrow(); + }); + + it("rejects side without filePath", () => { + expect(() => ReviewCommentSchema.parse({ ...base, side: "LEFT" })).toThrow(); + }); +}); + +describe("ReviewSuggestionRunSchema", () => { + const now = "2026-01-01T00:00:00.000Z"; + it("parses a succeeded run", () => { + const parsed = ReviewSuggestionRunSchema.parse({ + id: "run_1", + workspaceId: "ws_1", + hookId: "hook_1", + status: "succeeded", + durationMs: 120, + exitStatus: 0, + output: { suggestions: [], generatedAt: null, metadata: {} }, + createdAt: now, + }); + expect(parsed.status).toBe("succeeded"); + expect(parsed.stderr).toBeNull(); + expect(parsed.error).toBeNull(); + }); + + it("parses a failed run with stderr + error", () => { + const parsed = ReviewSuggestionRunSchema.parse({ + id: "run_2", + workspaceId: "ws_1", + hookId: "hook_1", + status: "failed", + durationMs: 50, + exitStatus: 1, + stderr: "boom", + error: "Hook exited with 1", + createdAt: now, + }); + expect(parsed.status).toBe("failed"); + expect(parsed.output).toBeNull(); + }); + + it("rejects an unknown status", () => { + expect(() => + ReviewSuggestionRunSchema.parse({ + id: "run_3", + workspaceId: "ws_1", + hookId: "hook_1", + status: "weird", + createdAt: now, + }), + ).toThrow(); + }); +}); + +describe("RequestReviewPayloadSchema", () => { + it("requires the literal event and a string[] files list", () => { + const parsed = RequestReviewPayloadSchema.parse({ + event: "workspace.requestReview", + workspace: { id: "ws_1", name: "w", branch: "b" }, + repo: { id: "repo_1", name: "r" }, + pr: { url: null, branch: "feature", baseBranch: "main" }, + diff: { files: ["src/a.ts", "src/b.ts"], addedLines: 10, deletedLines: 2, truncated: false }, + }); + expect(parsed.diff.files).toEqual(["src/a.ts", "src/b.ts"]); + }); + + it("rejects non-string entries in diff.files", () => { + expect(() => + RequestReviewPayloadSchema.parse({ + event: "workspace.requestReview", + workspace: { id: "ws_1" }, + repo: { id: "repo_1" }, + pr: { url: null, branch: "f", baseBranch: "main" }, + diff: { files: [123], addedLines: 0, deletedLines: 0, truncated: false }, + }), + ).toThrow(); + }); + + it("rejects negative line counts", () => { + expect(() => + RequestReviewPayloadSchema.parse({ + event: "workspace.requestReview", + workspace: { id: "ws_1" }, + repo: { id: "repo_1" }, + pr: { url: null, branch: "f", baseBranch: "main" }, + diff: { files: [], addedLines: -1, deletedLines: 0, truncated: false }, + }), + ).toThrow(); + }); +}); diff --git a/packages/contracts/src/review.ts b/packages/contracts/src/review.ts new file mode 100644 index 00000000..11c9b883 --- /dev/null +++ b/packages/contracts/src/review.ts @@ -0,0 +1,112 @@ +import { z } from "zod"; + +export const ReviewSuggestionKindSchema = z.enum(["reviewer", "checklist", "note", "warning"]); + +export const ReviewSuggestionSchema = z.object({ + id: z.string().min(1).max(120), + kind: ReviewSuggestionKindSchema, + label: z.string().min(1).max(200), + detail: z.string().max(2000).nullable().default(null), + url: z.string().url().nullable().default(null), + metadata: z.record(z.unknown()).default({}), +}); + +export const ReviewSuggestionsOutputSchema = z.object({ + suggestions: z.array(ReviewSuggestionSchema).max(50).default([]), + generatedAt: z.string().nullable().default(null), + metadata: z.record(z.unknown()).default({}), +}); + +export const ReviewCommentStatusSchema = z.enum(["open", "resolved"]); + +export const ReviewCommentSideSchema = z.enum(["LEFT", "RIGHT"]); + +export const ReviewCommentSchema = z + .object({ + id: z.string().min(1), + workspaceId: z.string().min(1), + filePath: z.string().min(1).max(512).nullable().default(null), + lineStart: z.number().int().min(1).nullable().default(null), + lineEnd: z.number().int().min(1).nullable().default(null), + side: ReviewCommentSideSchema.nullable().default(null), + author: z.string().min(1).max(80), + body: z.string().min(1).max(8000), + status: ReviewCommentStatusSchema.default("open"), + createdAt: z.string().min(1), + updatedAt: z.string().min(1), + deletedAt: z.string().nullable().default(null), + }) + .superRefine((value, ctx) => { + if (value.filePath === null) { + if (value.lineStart !== null) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["lineStart"], + message: "lineStart requires filePath", + }); + } + if (value.lineEnd !== null) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["lineEnd"], + message: "lineEnd requires filePath", + }); + } + if (value.side !== null) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["side"], + message: "side requires filePath", + }); + } + } + if (value.lineStart !== null && value.lineEnd !== null && value.lineEnd < value.lineStart) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["lineEnd"], + message: "lineEnd must be >= lineStart", + }); + } + }); + +export const ReviewSuggestionRunStatusSchema = z.enum(["succeeded", "failed", "timed_out"]); + +export const ReviewSuggestionRunSchema = z.object({ + id: z.string().min(1), + workspaceId: z.string().min(1), + hookId: z.string().min(1), + status: ReviewSuggestionRunStatusSchema, + durationMs: z.number().int().nullable().default(null), + exitStatus: z.number().int().nullable().default(null), + output: ReviewSuggestionsOutputSchema.nullable().default(null), + stderr: z.string().nullable().default(null), + error: z.string().nullable().default(null), + createdAt: z.string().min(1), +}); + +export const RequestReviewPayloadSchema = z.object({ + event: z.literal("workspace.requestReview"), + workspace: z.record(z.unknown()), + repo: z.record(z.unknown()), + pr: z.object({ + url: z.string().nullable(), + branch: z.string(), + baseBranch: z.string(), + }), + diff: z.object({ + files: z.array(z.string()), + addedLines: z.number().int().nonnegative(), + deletedLines: z.number().int().nonnegative(), + truncated: z.boolean(), + }), +}); + +export type ReviewSuggestionKind = z.infer; +export type ReviewSuggestion = z.infer; +export type ReviewSuggestionsOutput = z.infer; +export type ReviewCommentStatus = z.infer; +export type ReviewCommentSide = z.infer; +export type ReviewComment = z.infer; +export type ReviewSuggestionRunStatus = z.infer; +export type ReviewSuggestionRun = z.infer; +export type RequestReviewPayload = z.infer; From 60b7a65f2be5b0dacb7f0ce830eb110eb500ca40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:28:46 +0000 Subject: [PATCH 04/18] feat(config): register workspace.requestReview hook event 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) --- packages/config/src/index.test.ts | 59 +++++++++++++++++++++++++++++++ packages/config/src/index.ts | 21 +++++++++-- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/packages/config/src/index.test.ts b/packages/config/src/index.test.ts index e89ab17b..e2d59ba2 100644 --- a/packages/config/src/index.test.ts +++ b/packages/config/src/index.test.ts @@ -47,6 +47,65 @@ describe("loadConfig", () => { expect(config.repoDefaults.setupHookIds).toEqual(["setup"]); }); + it("defaults workspace.requestReview hooks to blocking", () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "citadel-config-")); + dirs.push(dir); + const configPath = path.join(dir, "citadel.config.json"); + fs.writeFileSync( + configPath, + JSON.stringify({ + version: 1, + dataDir: dir, + databasePath: path.join(dir, "citadel.sqlite"), + hooks: [{ id: "review", event: "workspace.requestReview", command: "true" }], + repoDefaults: { requestReviewHookIds: ["review"] }, + }), + ); + + const config = loadConfig(configPath); + + expect(config.hooks[0]).toMatchObject({ id: "review", blocking: true }); + expect(config.repoDefaults.requestReviewHookIds).toEqual(["review"]); + }); + + it("rejects requestReviewHookIds pointing at a non-review hook", () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "citadel-config-")); + dirs.push(dir); + const configPath = path.join(dir, "citadel.config.json"); + fs.writeFileSync( + configPath, + JSON.stringify({ + version: 1, + dataDir: dir, + databasePath: path.join(dir, "citadel.sqlite"), + hooks: [{ id: "setup", event: "workspace.setup", command: "true" }], + repoDefaults: { requestReviewHookIds: ["setup"] }, + }), + ); + + expect(() => loadConfig(configPath)).toThrow(/workspace.requestReview/); + }); + + it("defaults requestReviewHookIds to empty when omitted from existing configs", () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "citadel-config-")); + dirs.push(dir); + const configPath = path.join(dir, "citadel.config.json"); + fs.writeFileSync( + configPath, + JSON.stringify({ + version: 1, + dataDir: dir, + databasePath: path.join(dir, "citadel.sqlite"), + hooks: [], + repoDefaults: { setupHookIds: [] }, + }), + ); + + const config = loadConfig(configPath); + + expect(config.repoDefaults.requestReviewHookIds).toEqual([]); + }); + it("defaults lifecycle notification hooks to non-blocking", () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), "citadel-config-")); dirs.push(dir); diff --git a/packages/config/src/index.ts b/packages/config/src/index.ts index 1ec04930..af256e8e 100644 --- a/packages/config/src/index.ts +++ b/packages/config/src/index.ts @@ -43,6 +43,7 @@ export const HookEventSchema = z.enum([ "workspace.teardown", "workspace.apps", "workspace.action", + "workspace.requestReview", "workspace.created", "workspace.archived", "workspace.removed", @@ -64,7 +65,9 @@ export const HookConfigSchema = z }) .transform((hook) => ({ ...hook, - blocking: hook.blocking ?? ["workspace.setup", "workspace.teardown"].includes(hook.event), + blocking: + hook.blocking ?? + ["workspace.setup", "workspace.teardown", "workspace.requestReview"].includes(hook.event), })); export const CitadelConfigSchema = z @@ -136,8 +139,15 @@ export const CitadelConfigSchema = z teardownHookIds: z.array(z.string()).default([]), appHookIds: z.array(z.string()).default([]), actionHookIds: z.array(z.string()).default([]), + requestReviewHookIds: z.array(z.string()).default([]), }) - .default({ setupHookIds: [], teardownHookIds: [], appHookIds: [], actionHookIds: [] }), + .default({ + setupHookIds: [], + teardownHookIds: [], + appHookIds: [], + actionHookIds: [], + requestReviewHookIds: [], + }), commandPolicy: z .object({ hookTimeoutMs: z.number().int().min(1000).default(120000), @@ -174,6 +184,13 @@ export const CitadelConfigSchema = z "repoDefaults", "actionHookIds", ]); + validateHookReferences( + context, + hooksById, + config.repoDefaults.requestReviewHookIds, + "workspace.requestReview", + ["repoDefaults", "requestReviewHookIds"], + ); }); export type CitadelConfig = z.infer; From e666b53067e82dabcd9bd9ecb8a165fd009444e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:35:38 +0000 Subject: [PATCH 05/18] feat(db): add review_comments + review_suggestion_runs (schema version 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) --- apps/daemon/src/agent-messages-routes.test.ts | 2 + apps/daemon/src/app.test.ts | 3 + apps/daemon/src/namespace-routes.test.ts | 2 + apps/daemon/src/recent-commits-route.test.ts | 1 + .../daemon/src/scheduled-agent-routes.test.ts | 4 + apps/web/src/navigator-groups.test.ts | 1 + packages/contracts/src/index.ts | 1 + packages/core/src/index.test.ts | 1 + packages/db/src/index.test.ts | 8 + packages/db/src/index.ts | 29 +- packages/db/src/migrate.ts | 37 +++ packages/db/src/review.test.ts | 288 ++++++++++++++++++ packages/db/src/review.ts | 229 ++++++++++++++ packages/db/src/rows.ts | 1 + packages/mcp/src/index.test.ts | 1 + packages/operations/src/agent-history.test.ts | 1 + packages/operations/src/helpers.test.ts | 1 + packages/operations/src/index.ts | 2 + 18 files changed, 608 insertions(+), 4 deletions(-) create mode 100644 packages/db/src/review.test.ts create mode 100644 packages/db/src/review.ts diff --git a/apps/daemon/src/agent-messages-routes.test.ts b/apps/daemon/src/agent-messages-routes.test.ts index 32fdb00c..60ae5b7e 100644 --- a/apps/daemon/src/agent-messages-routes.test.ts +++ b/apps/daemon/src/agent-messages-routes.test.ts @@ -191,6 +191,7 @@ describe("agent message MCP + REST routes", () => { worktreeParent: input.worktreeParent ?? `${input.rootPath}-worktrees`, setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], createdAt: "2026-05-23T00:00:00.000Z", updatedAt: "2026-05-23T00:00:00.000Z", @@ -315,6 +316,7 @@ describe("agent message MCP + REST routes", () => { worktreeParent: path.join(fixture.config.dataDir, "wt"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: now, diff --git a/apps/daemon/src/app.test.ts b/apps/daemon/src/app.test.ts index 5fc59b2f..238bad78 100644 --- a/apps/daemon/src/app.test.ts +++ b/apps/daemon/src/app.test.ts @@ -269,6 +269,7 @@ describe("createDaemonApp", () => { worktreeParent: path.join(fixture.config.dataDir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: ["github-gh"], deployHookCommand: null, createdAt: now, @@ -383,6 +384,7 @@ describe("createDaemonApp", () => { worktreeParent: path.join(fixture.config.dataDir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: ["github-gh"], deployHookCommand: null, createdAt: now, @@ -624,6 +626,7 @@ describe("createDaemonApp", () => { worktreeParent: path.join(fixture.config.dataDir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: ["github-gh"], deployHookCommand: null, createdAt: now, diff --git a/apps/daemon/src/namespace-routes.test.ts b/apps/daemon/src/namespace-routes.test.ts index 10317301..3d03b0c3 100644 --- a/apps/daemon/src/namespace-routes.test.ts +++ b/apps/daemon/src/namespace-routes.test.ts @@ -34,6 +34,7 @@ describe("namespace routes + MCP integration", () => { worktreeParent: path.join(fixture.config.dataDir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: now, @@ -149,6 +150,7 @@ describe("namespace routes + MCP integration", () => { worktreeParent: path.join(fixture.config.dataDir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: now, diff --git a/apps/daemon/src/recent-commits-route.test.ts b/apps/daemon/src/recent-commits-route.test.ts index e968d5aa..952598c9 100644 --- a/apps/daemon/src/recent-commits-route.test.ts +++ b/apps/daemon/src/recent-commits-route.test.ts @@ -31,6 +31,7 @@ describe("recent-commits route", () => { worktreeParent: String(fixture.config.dataDir), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: now, diff --git a/apps/daemon/src/scheduled-agent-routes.test.ts b/apps/daemon/src/scheduled-agent-routes.test.ts index b3c8da3e..c4100056 100644 --- a/apps/daemon/src/scheduled-agent-routes.test.ts +++ b/apps/daemon/src/scheduled-agent-routes.test.ts @@ -31,6 +31,7 @@ describe("scheduled agent routes", () => { worktreeParent: path.join(fixture.config.dataDir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: now, @@ -140,6 +141,7 @@ describe("scheduled agent routes", () => { worktreeParent: path.join(fixture.config.dataDir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: now, @@ -223,6 +225,7 @@ describe("scheduled agent routes", () => { worktreeParent: path.join(fixture.config.dataDir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: now, @@ -322,6 +325,7 @@ describe("scheduled agent routes", () => { worktreeParent: path.join(fixture.config.dataDir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: new Date().toISOString(), diff --git a/apps/web/src/navigator-groups.test.ts b/apps/web/src/navigator-groups.test.ts index f8572d10..596f406a 100644 --- a/apps/web/src/navigator-groups.test.ts +++ b/apps/web/src/navigator-groups.test.ts @@ -14,6 +14,7 @@ function makeRepo(id: string, name: string): Repo { worktreeParent: `/repos/${id}/.wt`, setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: ts, diff --git a/packages/contracts/src/index.ts b/packages/contracts/src/index.ts index 2ce18165..6fa1d17f 100644 --- a/packages/contracts/src/index.ts +++ b/packages/contracts/src/index.ts @@ -31,6 +31,7 @@ export const RepoSchema = z.object({ worktreeParent: z.string().min(1), setupHookIds: z.array(z.string()).default([]), teardownHookIds: z.array(z.string()).default([]), + requestReviewHookIds: z.array(z.string()).default([]), providerIds: z.array(z.string()).default([]), deployHookCommand: z.string().max(4000).nullable().default(null), createdAt: z.string(), diff --git a/packages/core/src/index.test.ts b/packages/core/src/index.test.ts index 067428ae..cd3afe55 100644 --- a/packages/core/src/index.test.ts +++ b/packages/core/src/index.test.ts @@ -173,6 +173,7 @@ describe("uniqueness guards", () => { worktreeParent: "/tmp/worktrees", setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", diff --git a/packages/db/src/index.test.ts b/packages/db/src/index.test.ts index 56bba801..568f780e 100644 --- a/packages/db/src/index.test.ts +++ b/packages/db/src/index.test.ts @@ -26,6 +26,7 @@ describe("SqliteStore", () => { worktreeParent: path.join(dir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", @@ -42,6 +43,7 @@ describe("SqliteStore", () => { { version: 5 }, { version: 6 }, { version: 7 }, + { version: 8 }, ]); }); @@ -59,6 +61,7 @@ describe("SqliteStore", () => { worktreeParent: path.join(dir, "worktrees"), setupHookIds: ["setup"], teardownHookIds: ["teardown"], + requestReviewHookIds: [], providerIds: ["github-gh"], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", @@ -194,6 +197,7 @@ describe("SqliteStore", () => { worktreeParent: path.join(dir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", @@ -265,6 +269,7 @@ describe("SqliteStore", () => { worktreeParent: path.join(dir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: ["github-gh"], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", @@ -323,6 +328,7 @@ describe("SqliteStore", () => { worktreeParent: path.join(dir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", @@ -429,6 +435,7 @@ describe("SqliteStore", () => { worktreeParent: path.join(dir, "wt"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", @@ -591,6 +598,7 @@ describe("SqliteStore", () => { worktreeParent: path.join(dir, "wt"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", diff --git a/packages/db/src/index.ts b/packages/db/src/index.ts index ff700f5e..d4842e3a 100644 --- a/packages/db/src/index.ts +++ b/packages/db/src/index.ts @@ -107,8 +107,8 @@ export class SqliteStore { this.database .prepare( `INSERT INTO repos (id, name, root_path, default_branch, default_remote, worktree_parent, - setup_hook_ids, teardown_hook_ids, provider_ids, deploy_hook_command, created_at, updated_at, archived_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + setup_hook_ids, teardown_hook_ids, request_review_hook_ids, provider_ids, deploy_hook_command, created_at, updated_at, archived_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, ) .run( repo.id, @@ -119,6 +119,7 @@ export class SqliteStore { repo.worktreeParent, JSON.stringify(repo.setupHookIds), JSON.stringify(repo.teardownHookIds), + JSON.stringify(repo.requestReviewHookIds ?? []), JSON.stringify(repo.providerIds), repo.deployHookCommand ?? null, repo.createdAt, @@ -130,7 +131,16 @@ export class SqliteStore { updateRepo( repoId: string, patch: Partial< - Pick + Pick< + Repo, + | "name" + | "worktreeParent" + | "setupHookIds" + | "teardownHookIds" + | "requestReviewHookIds" + | "providerIds" + | "deployHookCommand" + > >, ) { const existing = this.database.prepare("SELECT * FROM repos WHERE id = ?").get(repoId) as @@ -144,6 +154,7 @@ export class SqliteStore { worktreeParent: patch.worktreeParent ?? current.worktreeParent, setupHookIds: patch.setupHookIds ?? current.setupHookIds, teardownHookIds: patch.teardownHookIds ?? current.teardownHookIds, + requestReviewHookIds: patch.requestReviewHookIds ?? current.requestReviewHookIds, providerIds: patch.providerIds ?? current.providerIds, deployHookCommand: patch.deployHookCommand !== undefined ? patch.deployHookCommand : current.deployHookCommand, updatedAt: new Date().toISOString(), @@ -151,13 +162,14 @@ export class SqliteStore { this.database .prepare( `UPDATE repos SET name = ?, worktree_parent = ?, setup_hook_ids = ?, teardown_hook_ids = ?, - provider_ids = ?, deploy_hook_command = ?, updated_at = ? WHERE id = ?`, + request_review_hook_ids = ?, provider_ids = ?, deploy_hook_command = ?, updated_at = ? WHERE id = ?`, ) .run( next.name, next.worktreeParent, JSON.stringify(next.setupHookIds), JSON.stringify(next.teardownHookIds), + JSON.stringify(next.requestReviewHookIds), JSON.stringify(next.providerIds), next.deployHookCommand ?? null, next.updatedAt, @@ -677,4 +689,13 @@ export class SqliteStore { // the assignment inside scheduled-run-store.ts itself because ES module // hoisting would run it before this class declaration completes. import { scheduledRunStoreMethods } from "./scheduled-run-store.js"; +import { reviewStoreMethods } from "./review.js"; Object.assign(SqliteStore.prototype, scheduledRunStoreMethods); +Object.assign(SqliteStore.prototype, reviewStoreMethods); +export type { + InsertReviewCommentInput, + InsertReviewSuggestionRunInput, + ListReviewCommentsOptions, + ReviewCommentMutationResult, + ReviewCommentPatch, +} from "./review.js"; diff --git a/packages/db/src/migrate.ts b/packages/db/src/migrate.ts index 4bf836c4..1fc9f410 100644 --- a/packages/db/src/migrate.ts +++ b/packages/db/src/migrate.ts @@ -205,4 +205,41 @@ export function runMigrations( ensureColumn("scheduled_agents", "run_mode", "TEXT NOT NULL DEFAULT 'workspace'"); ensureColumn("scheduled_agents", "background_cwd", "TEXT"); ensureColumn("scheduled_agents", "overlap_policy", "TEXT NOT NULL DEFAULT 'skip'"); + ensureColumn("repos", "request_review_hook_ids", "TEXT NOT NULL DEFAULT '[]'"); + db.exec(` + CREATE TABLE IF NOT EXISTS review_comments ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL REFERENCES workspaces(id) ON DELETE CASCADE, + file_path TEXT, + line_start INTEGER, + line_end INTEGER, + side TEXT CHECK (side IS NULL OR side IN ('LEFT','RIGHT')), + author TEXT NOT NULL, + body TEXT NOT NULL, + status TEXT NOT NULL CHECK (status IN ('open','resolved')), + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL, + deleted_at TEXT + ); + CREATE INDEX IF NOT EXISTS idx_review_comments_workspace + ON review_comments(workspace_id, created_at DESC); + CREATE INDEX IF NOT EXISTS idx_review_comments_status + ON review_comments(workspace_id, status); + CREATE TABLE IF NOT EXISTS review_suggestion_runs ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL REFERENCES workspaces(id) ON DELETE CASCADE, + hook_id TEXT NOT NULL, + status TEXT NOT NULL CHECK (status IN ('succeeded','failed','timed_out')), + duration_ms INTEGER, + exit_status INTEGER, + output_json TEXT, + stderr TEXT, + error TEXT, + created_at TEXT NOT NULL + ); + CREATE INDEX IF NOT EXISTS idx_review_suggestion_runs_workspace + ON review_suggestion_runs(workspace_id, created_at DESC); + INSERT OR IGNORE INTO schema_migrations(version, name, applied_at) VALUES + (8, 'review-system', datetime('now')); + `); } diff --git a/packages/db/src/review.test.ts b/packages/db/src/review.test.ts new file mode 100644 index 00000000..cf804b95 --- /dev/null +++ b/packages/db/src/review.test.ts @@ -0,0 +1,288 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { SqliteStore } from "./index.js"; + +const dirs: string[] = []; + +afterEach(() => { + for (const dir of dirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); +}); + +type Fixture = { dir: string; store: SqliteStore; repoId: string; workspaceId: string }; + +function makeStore(): Fixture { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "citadel-review-db-")); + dirs.push(dir); + const store = new SqliteStore(path.join(dir, "citadel.sqlite")); + store.migrate(); + const now = new Date().toISOString(); + store.insertRepo({ + id: "repo_test", + name: "Repo", + rootPath: path.join(dir, "repo"), + defaultBranch: "main", + defaultRemote: "origin", + worktreeParent: path.join(dir, "wt"), + setupHookIds: [], + teardownHookIds: [], + requestReviewHookIds: [], + providerIds: [], + deployHookCommand: null, + createdAt: now, + updatedAt: now, + archivedAt: null, + }); + store.insertWorkspace({ + id: "ws_test", + repoId: "repo_test", + name: "ws", + path: path.join(dir, "wt", "ws"), + branch: "feature", + baseBranch: "main", + source: "scratch", + kind: "worktree", + prUrl: null, + issueKey: null, + issueTitle: null, + issueUrl: null, + slackThreadUrl: null, + section: "default", + pinned: false, + lifecycle: "ready", + dirty: false, + namespaceId: null, + createdAt: now, + updatedAt: now, + archivedAt: null, + }); + return { dir, store, repoId: "repo_test", workspaceId: "ws_test" }; +} + +describe("review_comments", () => { + it("inserts and lists a comment with default options", () => { + const f = makeStore(); + f.store.insertReviewComment({ + id: "rc_1", + workspaceId: f.workspaceId, + author: "operator", + body: "first", + }); + const list = f.store.listReviewComments(f.workspaceId); + expect(list).toHaveLength(1); + expect(list[0]).toMatchObject({ id: "rc_1", body: "first", status: "open", deletedAt: null }); + }); + + it("preserves file:line anchors round-trip", () => { + const f = makeStore(); + f.store.insertReviewComment({ + id: "rc_2", + workspaceId: f.workspaceId, + author: "operator", + body: "anchor", + filePath: "src/foo.ts", + lineStart: 10, + lineEnd: 12, + side: "RIGHT", + }); + const list = f.store.listReviewComments(f.workspaceId); + expect(list[0]).toMatchObject({ + filePath: "src/foo.ts", + lineStart: 10, + lineEnd: 12, + side: "RIGHT", + }); + }); + + it("filters by status", () => { + const f = makeStore(); + f.store.insertReviewComment({ + id: "rc_open", + workspaceId: f.workspaceId, + author: "operator", + body: "o", + }); + f.store.insertReviewComment({ + id: "rc_resolved", + workspaceId: f.workspaceId, + author: "operator", + body: "r", + status: "resolved", + }); + expect(f.store.listReviewComments(f.workspaceId, { status: "open" }).map((c) => c.id)).toEqual([ + "rc_open", + ]); + expect(f.store.listReviewComments(f.workspaceId, { status: "resolved" }).map((c) => c.id)).toEqual([ + "rc_resolved", + ]); + expect(f.store.listReviewComments(f.workspaceId, { status: "all" })).toHaveLength(2); + }); + + it("excludes archived-workspace comments by default; includeArchived opts back in", () => { + const f = makeStore(); + f.store.insertReviewComment({ + id: "rc_a", + workspaceId: f.workspaceId, + author: "operator", + body: "hi", + }); + f.store.archiveWorkspace(f.workspaceId, "archived"); + expect(f.store.listReviewComments(f.workspaceId)).toEqual([]); + expect(f.store.listReviewComments(f.workspaceId, { includeArchived: true })).toHaveLength(1); + }); + + it("update returns conflict when the if-match token is stale", () => { + const f = makeStore(); + const created = f.store.insertReviewComment({ + id: "rc_u", + workspaceId: f.workspaceId, + author: "operator", + body: "v1", + }); + const result = f.store.updateReviewComment( + "rc_u", + { body: "v2" }, + "1970-01-01T00:00:00.000Z", + ); + expect(result.kind).toBe("conflict"); + if (result.kind === "conflict") expect(result.latest.id).toBe(created.id); + expect(f.store.getReviewComment("rc_u")?.body).toBe("v1"); + }); + + it("update succeeds with a fresh token and bumps updated_at", () => { + const f = makeStore(); + const created = f.store.insertReviewComment({ + id: "rc_u2", + workspaceId: f.workspaceId, + author: "operator", + body: "v1", + }); + const result = f.store.updateReviewComment( + "rc_u2", + { body: "v2", status: "resolved" }, + created.updatedAt, + "2099-01-01T00:00:00.000Z", + ); + expect(result.kind).toBe("updated"); + if (result.kind === "updated") { + expect(result.row.body).toBe("v2"); + expect(result.row.status).toBe("resolved"); + expect(result.row.updatedAt).toBe("2099-01-01T00:00:00.000Z"); + } + }); + + it("soft-delete hides the row from default list but the physical row remains", () => { + const f = makeStore(); + const created = f.store.insertReviewComment({ + id: "rc_d", + workspaceId: f.workspaceId, + author: "operator", + body: "byte", + }); + f.store.softDeleteReviewComment("rc_d", created.updatedAt); + expect(f.store.listReviewComments(f.workspaceId)).toHaveLength(0); + expect(f.store.listReviewComments(f.workspaceId, { includeDeleted: true })).toHaveLength(1); + const physical = f.store.query<{ id: string }>("SELECT id FROM review_comments WHERE id = 'rc_d'"); + expect(physical).toHaveLength(1); + }); + + it("cascades on hard workspace delete", () => { + const f = makeStore(); + f.store.insertReviewComment({ + id: "rc_c", + workspaceId: f.workspaceId, + author: "operator", + body: "x", + }); + f.store.query(`DELETE FROM workspaces WHERE id = '${f.workspaceId}'`); + const physical = f.store.query<{ id: string }>("SELECT id FROM review_comments"); + expect(physical).toEqual([]); + }); +}); + +describe("review_suggestion_runs", () => { + it("stores a succeeded run with parsed output and latest returns it", () => { + const f = makeStore(); + f.store.insertReviewSuggestionRun({ + id: "run_1", + workspaceId: f.workspaceId, + hookId: "hook_1", + status: "succeeded", + durationMs: 30, + exitStatus: 0, + output: { suggestions: [{ id: "s1", kind: "note", label: "ok", detail: null, url: null, metadata: {} }], generatedAt: null, metadata: {} }, + stderr: null, + error: null, + }); + const latest = f.store.latestReviewSuggestionRun(f.workspaceId); + expect(latest?.status).toBe("succeeded"); + expect(latest?.output?.suggestions[0]?.id).toBe("s1"); + }); + + it("stores a failed run with stderr + error, no output", () => { + const f = makeStore(); + f.store.insertReviewSuggestionRun({ + id: "run_2", + workspaceId: f.workspaceId, + hookId: "hook_1", + status: "failed", + durationMs: 5, + exitStatus: 1, + output: null, + stderr: "boom", + error: "Hook exited with 1", + }); + const latest = f.store.latestReviewSuggestionRun(f.workspaceId); + expect(latest?.status).toBe("failed"); + expect(latest?.output).toBeNull(); + expect(latest?.stderr).toBe("boom"); + expect(latest?.error).toBe("Hook exited with 1"); + }); + + it("orders newest first", () => { + const f = makeStore(); + f.store.insertReviewSuggestionRun({ + id: "run_a", + workspaceId: f.workspaceId, + hookId: "hook_1", + status: "succeeded", + durationMs: 5, + exitStatus: 0, + output: { suggestions: [], generatedAt: null, metadata: {} }, + stderr: null, + error: null, + createdAt: "2026-01-01T00:00:00.000Z", + }); + f.store.insertReviewSuggestionRun({ + id: "run_b", + workspaceId: f.workspaceId, + hookId: "hook_1", + status: "succeeded", + durationMs: 5, + exitStatus: 0, + output: { suggestions: [], generatedAt: null, metadata: {} }, + stderr: null, + error: null, + createdAt: "2026-02-01T00:00:00.000Z", + }); + expect(f.store.latestReviewSuggestionRun(f.workspaceId)?.id).toBe("run_b"); + }); +}); + +describe("schema_migrations", () => { + it("includes the review-system row at version 8", () => { + const f = makeStore(); + const rows = f.store.query<{ version: number; name: string }>( + "SELECT version, name FROM schema_migrations WHERE version = 8", + ); + expect(rows).toEqual([{ version: 8, name: "review-system" }]); + }); + + it("is idempotent — re-running migrate is a no-op", () => { + const f = makeStore(); + f.store.migrate(); + const rows = f.store.query<{ version: number }>("SELECT version FROM schema_migrations ORDER BY version"); + expect(rows.map((r) => r.version)).toEqual([1, 2, 3, 4, 5, 6, 7, 8]); + }); +}); diff --git a/packages/db/src/review.ts b/packages/db/src/review.ts new file mode 100644 index 00000000..9c00bae0 --- /dev/null +++ b/packages/db/src/review.ts @@ -0,0 +1,229 @@ +import type { ReviewComment, ReviewSuggestionRun, ReviewSuggestionsOutput } from "@citadel/contracts"; +import type { SqliteStore } from "./index.js"; +import { asString } from "./rows.js"; + +declare module "./index.js" { + interface SqliteStore { + listReviewComments(workspaceId: string, opts?: ListReviewCommentsOptions): ReviewComment[]; + getReviewComment(id: string): ReviewComment | null; + insertReviewComment(input: InsertReviewCommentInput): ReviewComment; + updateReviewComment( + id: string, + patch: ReviewCommentPatch, + ifUpdatedAtMatches: string, + now?: string, + ): ReviewCommentMutationResult; + softDeleteReviewComment( + id: string, + ifUpdatedAtMatches: string, + now?: string, + ): ReviewCommentMutationResult; + insertReviewSuggestionRun(input: InsertReviewSuggestionRunInput): ReviewSuggestionRun; + latestReviewSuggestionRun(workspaceId: string): ReviewSuggestionRun | null; + } +} + +export type ListReviewCommentsOptions = { + status?: "open" | "resolved" | "all"; + includeDeleted?: boolean; + includeArchived?: boolean; +}; + +export type InsertReviewCommentInput = { + id: string; + workspaceId: string; + filePath?: string | null; + lineStart?: number | null; + lineEnd?: number | null; + side?: "LEFT" | "RIGHT" | null; + author: string; + body: string; + status?: ReviewComment["status"]; + createdAt?: string; + updatedAt?: string; +}; + +export type ReviewCommentPatch = { + body?: string; + status?: ReviewComment["status"]; +}; + +export type ReviewCommentMutationResult = + | { kind: "updated"; row: ReviewComment } + | { kind: "conflict"; latest: ReviewComment } + | { kind: "not-found" }; + +export type InsertReviewSuggestionRunInput = Omit & { + createdAt?: string; +}; + +function reviewCommentFromRow(row: Record): ReviewComment { + return { + id: asString(row, "id"), + workspaceId: asString(row, "workspace_id"), + filePath: row.file_path ? asString(row, "file_path") : null, + lineStart: row.line_start === null || row.line_start === undefined ? null : Number(row.line_start), + lineEnd: row.line_end === null || row.line_end === undefined ? null : Number(row.line_end), + side: row.side ? (asString(row, "side") as "LEFT" | "RIGHT") : null, + author: asString(row, "author"), + body: asString(row, "body"), + status: asString(row, "status") as ReviewComment["status"], + createdAt: asString(row, "created_at"), + updatedAt: asString(row, "updated_at"), + deletedAt: row.deleted_at ? asString(row, "deleted_at") : null, + }; +} + +function reviewSuggestionRunFromRow(row: Record): ReviewSuggestionRun { + const outputJson = row.output_json ? asString(row, "output_json") : null; + const output: ReviewSuggestionsOutput | null = outputJson + ? (JSON.parse(outputJson) as ReviewSuggestionsOutput) + : null; + return { + id: asString(row, "id"), + workspaceId: asString(row, "workspace_id"), + hookId: asString(row, "hook_id"), + status: asString(row, "status") as ReviewSuggestionRun["status"], + durationMs: row.duration_ms === null || row.duration_ms === undefined ? null : Number(row.duration_ms), + exitStatus: row.exit_status === null || row.exit_status === undefined ? null : Number(row.exit_status), + output, + stderr: row.stderr ? asString(row, "stderr") : null, + error: row.error ? asString(row, "error") : null, + createdAt: asString(row, "created_at"), + }; +} + +export const reviewStoreMethods = { + listReviewComments(this: SqliteStore, workspaceId: string, opts: ListReviewCommentsOptions = {}): ReviewComment[] { + const { status = "all", includeDeleted = false, includeArchived = false } = opts; + const clauses: string[] = ["rc.workspace_id = ?"]; + const params: unknown[] = [workspaceId]; + if (!includeDeleted) clauses.push("rc.deleted_at IS NULL"); + if (status !== "all") { + clauses.push("rc.status = ?"); + params.push(status); + } + if (!includeArchived) clauses.push("w.archived_at IS NULL"); + const sql = `SELECT rc.* FROM review_comments rc + JOIN workspaces w ON w.id = rc.workspace_id + WHERE ${clauses.join(" AND ")} + ORDER BY rc.created_at DESC`; + const rows = this.database.prepare(sql).all(...params) as Array>; + return rows.map(reviewCommentFromRow); + }, + + getReviewComment(this: SqliteStore, id: string): ReviewComment | null { + const row = this.database.prepare("SELECT * FROM review_comments WHERE id = ?").get(id) as + | Record + | undefined; + return row ? reviewCommentFromRow(row) : null; + }, + + insertReviewComment(this: SqliteStore, input: InsertReviewCommentInput): ReviewComment { + const now = input.createdAt ?? new Date().toISOString(); + const row: ReviewComment = { + id: input.id, + workspaceId: input.workspaceId, + filePath: input.filePath ?? null, + lineStart: input.lineStart ?? null, + lineEnd: input.lineEnd ?? null, + side: input.side ?? null, + author: input.author, + body: input.body, + status: input.status ?? "open", + createdAt: now, + updatedAt: input.updatedAt ?? now, + deletedAt: null, + }; + this.database + .prepare( + `INSERT INTO review_comments (id, workspace_id, file_path, line_start, line_end, side, author, body, status, created_at, updated_at, deleted_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, NULL)`, + ) + .run( + row.id, + row.workspaceId, + row.filePath, + row.lineStart, + row.lineEnd, + row.side, + row.author, + row.body, + row.status, + row.createdAt, + row.updatedAt, + ); + return row; + }, + + updateReviewComment( + this: SqliteStore, + id: string, + patch: ReviewCommentPatch, + ifUpdatedAtMatches: string, + now: string = new Date().toISOString(), + ): ReviewCommentMutationResult { + const current = this.getReviewComment(id); + if (!current || current.deletedAt) return { kind: "not-found" }; + if (current.updatedAt !== ifUpdatedAtMatches) return { kind: "conflict", latest: current }; + const next: ReviewComment = { + ...current, + body: patch.body ?? current.body, + status: patch.status ?? current.status, + updatedAt: now, + }; + this.database + .prepare("UPDATE review_comments SET body = ?, status = ?, updated_at = ? WHERE id = ?") + .run(next.body, next.status, next.updatedAt, id); + return { kind: "updated", row: next }; + }, + + softDeleteReviewComment( + this: SqliteStore, + id: string, + ifUpdatedAtMatches: string, + now: string = new Date().toISOString(), + ): ReviewCommentMutationResult { + const current = this.getReviewComment(id); + if (!current || current.deletedAt) return { kind: "not-found" }; + if (current.updatedAt !== ifUpdatedAtMatches) return { kind: "conflict", latest: current }; + const next: ReviewComment = { ...current, updatedAt: now, deletedAt: now }; + this.database + .prepare("UPDATE review_comments SET updated_at = ?, deleted_at = ? WHERE id = ?") + .run(next.updatedAt, next.deletedAt, id); + return { kind: "updated", row: next }; + }, + + insertReviewSuggestionRun(this: SqliteStore, input: InsertReviewSuggestionRunInput): ReviewSuggestionRun { + const createdAt = input.createdAt ?? new Date().toISOString(); + const outputJson = input.output ? JSON.stringify(input.output) : null; + const row: ReviewSuggestionRun = { ...input, createdAt }; + this.database + .prepare( + `INSERT INTO review_suggestion_runs (id, workspace_id, hook_id, status, duration_ms, exit_status, output_json, stderr, error, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + ) + .run( + row.id, + row.workspaceId, + row.hookId, + row.status, + row.durationMs, + row.exitStatus, + outputJson, + row.stderr, + row.error, + row.createdAt, + ); + return row; + }, + + latestReviewSuggestionRun(this: SqliteStore, workspaceId: string): ReviewSuggestionRun | null { + const row = this.database + .prepare( + "SELECT * FROM review_suggestion_runs WHERE workspace_id = ? ORDER BY created_at DESC LIMIT 1", + ) + .get(workspaceId) as Record | undefined; + return row ? reviewSuggestionRunFromRow(row) : null; + }, +}; diff --git a/packages/db/src/rows.ts b/packages/db/src/rows.ts index 7cb2aefd..d72b6b22 100644 --- a/packages/db/src/rows.ts +++ b/packages/db/src/rows.ts @@ -35,6 +35,7 @@ export function repoFromRow(row: Record): Repo { worktreeParent: asString(row, "worktree_parent"), setupHookIds: jsonArray(row, "setup_hook_ids"), teardownHookIds: jsonArray(row, "teardown_hook_ids"), + requestReviewHookIds: jsonArray(row, "request_review_hook_ids"), providerIds: jsonArray(row, "provider_ids"), deployHookCommand: row.deploy_hook_command ? asString(row, "deploy_hook_command") : null, createdAt: asString(row, "created_at"), diff --git a/packages/mcp/src/index.test.ts b/packages/mcp/src/index.test.ts index b06cc8e3..9dc05ffe 100644 --- a/packages/mcp/src/index.test.ts +++ b/packages/mcp/src/index.test.ts @@ -118,6 +118,7 @@ describe("mcp helpers", () => { worktreeParent: "/tmp/worktrees", setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", diff --git a/packages/operations/src/agent-history.test.ts b/packages/operations/src/agent-history.test.ts index 0b9fb90d..95a49cab 100644 --- a/packages/operations/src/agent-history.test.ts +++ b/packages/operations/src/agent-history.test.ts @@ -28,6 +28,7 @@ function bootstrap(runtimeId = "claude-code") { worktreeParent: path.join(dir, "worktrees"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: "2026-05-23T09:00:00.000Z", diff --git a/packages/operations/src/helpers.test.ts b/packages/operations/src/helpers.test.ts index 832ddf24..dd246b90 100644 --- a/packages/operations/src/helpers.test.ts +++ b/packages/operations/src/helpers.test.ts @@ -115,6 +115,7 @@ function createFixture() { worktreeParent: path.join(dir, "wt"), setupHookIds: [], teardownHookIds: [], + requestReviewHookIds: [], providerIds: [], deployHookCommand: null, createdAt: "2026-05-17T00:00:00.000Z", diff --git a/packages/operations/src/index.ts b/packages/operations/src/index.ts index 09fd8c82..4755f888 100644 --- a/packages/operations/src/index.ts +++ b/packages/operations/src/index.ts @@ -81,6 +81,7 @@ export class OperationService { teardownHookIds: string[]; appHookIds?: string[]; actionHookIds?: string[]; + requestReviewHookIds?: string[]; }; commandPolicy: CitadelConfig["commandPolicy"]; }, @@ -99,6 +100,7 @@ export class OperationService { worktreeParent: input.worktreeParent || path.join(path.dirname(rootPath), `${path.basename(rootPath)}-worktrees`), setupHookIds: this.config?.repoDefaults.setupHookIds ?? [], teardownHookIds: this.config?.repoDefaults.teardownHookIds ?? [], + requestReviewHookIds: this.config?.repoDefaults.requestReviewHookIds ?? [], providerIds: ["github-gh", "jira-jtk"], deployHookCommand: null, createdAt: now, From 8078b794221c4717bcd63429f13b3f36440469e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:36:39 +0000 Subject: [PATCH 06/18] feat(hooks): add parseReviewSuggestionsOutput for the new event 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) --- packages/hooks/src/index.test.ts | 33 +++++++++++++++++++++++++++++++- packages/hooks/src/index.ts | 14 +++++++++++++- 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/packages/hooks/src/index.test.ts b/packages/hooks/src/index.test.ts index eac94c64..6776e999 100644 --- a/packages/hooks/src/index.test.ts +++ b/packages/hooks/src/index.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { parseHookOutput, runCommandHook } from "./index.js"; +import { parseHookOutput, parseReviewSuggestionsOutput, runCommandHook } from "./index.js"; describe("runCommandHook", () => { it("passes JSON input to command hooks and captures bounded output", async () => { @@ -64,3 +64,34 @@ describe("runCommandHook", () => { ).toMatchObject({ links: [{ label: "Preview" }], actions: [{ id: "redeploy" }] }); }); }); + +describe("parseReviewSuggestionsOutput", () => { + it("returns null for empty stdout", () => { + expect(parseReviewSuggestionsOutput("")).toBeNull(); + expect(parseReviewSuggestionsOutput(" \n\n")).toBeNull(); + }); + + it("parses a valid payload and defaults missing fields", () => { + const parsed = parseReviewSuggestionsOutput( + JSON.stringify({ + suggestions: [{ id: "s", kind: "reviewer", label: "@alice" }], + }), + ); + expect(parsed?.suggestions).toHaveLength(1); + expect(parsed?.generatedAt).toBeNull(); + expect(parsed?.metadata).toEqual({}); + }); + + it("throws on malformed JSON", () => { + expect(() => parseReviewSuggestionsOutput("{not json}")).toThrow(); + }); + + it("throws on schema-invalid JSON", () => { + expect(() => parseReviewSuggestionsOutput(JSON.stringify({ suggestions: "nope" }))).toThrow(); + }); + + it("surfaces a truncation-style error when the leading edge is sliced", () => { + const truncated = '{"suggestions":[{"id":"s","kind":"note","label":"x"}]}'.slice(5); + expect(() => parseReviewSuggestionsOutput(truncated)).toThrow(); + }); +}); diff --git a/packages/hooks/src/index.ts b/packages/hooks/src/index.ts index 35999f03..b893f9a9 100644 --- a/packages/hooks/src/index.ts +++ b/packages/hooks/src/index.ts @@ -1,5 +1,11 @@ import { spawn } from "node:child_process"; -import { type HookDiagnostic, type HookOutput, HookOutputSchema } from "@citadel/contracts"; +import { + type HookDiagnostic, + type HookOutput, + HookOutputSchema, + type ReviewSuggestionsOutput, + ReviewSuggestionsOutputSchema, +} from "@citadel/contracts"; export { DEPLOY_HOOK_RELATIVE_PATH, @@ -77,6 +83,12 @@ export function parseHookOutput(stdout: string): HookOutput | null { return HookOutputSchema.parse(JSON.parse(trimmed)); } +export function parseReviewSuggestionsOutput(stdout: string): ReviewSuggestionsOutput | null { + const trimmed = stdout.trim(); + if (!trimmed) return null; + return ReviewSuggestionsOutputSchema.parse(JSON.parse(trimmed)); +} + export function hookDiagnostic(input: { hook: CommandHook; enabled: boolean; From a242eeae67016d1c0ab3e7e646523440aad128a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:42:14 +0000 Subject: [PATCH 07/18] feat(operations): add review-system service (request review + comments) 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) --- packages/operations/src/hooks-runner.ts | 2 +- packages/operations/src/index.ts | 5 +- packages/operations/src/review-system.test.ts | 346 ++++++++++++++++++ packages/operations/src/review-system.ts | 292 +++++++++++++++ 4 files changed, 641 insertions(+), 4 deletions(-) create mode 100644 packages/operations/src/review-system.test.ts create mode 100644 packages/operations/src/review-system.ts diff --git a/packages/operations/src/hooks-runner.ts b/packages/operations/src/hooks-runner.ts index 1ead1618..4709490b 100644 --- a/packages/operations/src/hooks-runner.ts +++ b/packages/operations/src/hooks-runner.ts @@ -18,7 +18,7 @@ type RunnerConfig = { commandPolicy: CitadelConfig["commandPolicy"]; }; -function commandHook(hook: HookConfig, workspacePath: string, config: RunnerConfig | undefined) { +export function commandHook(hook: HookConfig, workspacePath: string, config: RunnerConfig | undefined) { return { id: hook.id, event: hook.event, diff --git a/packages/operations/src/index.ts b/packages/operations/src/index.ts index 4755f888..37b9366d 100644 --- a/packages/operations/src/index.ts +++ b/packages/operations/src/index.ts @@ -65,6 +65,7 @@ export { WorkspaceNameTakenError, } from "./helpers.js"; import { runNotificationHooks, runWorkspaceHooks } from "./hooks-runner.js"; +export type { RequestReviewResult, UpdateReviewCommentResult } from "./review-system.js"; import { type WorkspaceAppsDeps, discoverWorkspaceApps as discoverWorkspaceAppsImpl, @@ -79,9 +80,7 @@ export class OperationService { repoDefaults: { setupHookIds: string[]; teardownHookIds: string[]; - appHookIds?: string[]; - actionHookIds?: string[]; - requestReviewHookIds?: string[]; + appHookIds?: string[]; actionHookIds?: string[]; requestReviewHookIds?: string[]; }; commandPolicy: CitadelConfig["commandPolicy"]; }, diff --git a/packages/operations/src/review-system.test.ts b/packages/operations/src/review-system.test.ts new file mode 100644 index 00000000..ecd43c9a --- /dev/null +++ b/packages/operations/src/review-system.test.ts @@ -0,0 +1,346 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import type { HookConfig } from "@citadel/config"; +import type { Repo, Workspace } from "@citadel/contracts"; +import { SqliteStore } from "@citadel/db"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + addReviewComment, + deleteReviewComment, + listReviewComments, + requestReviewForWorkspace, + updateReviewComment, +} from "./review-system.js"; + +const dirs: string[] = []; + +afterEach(() => { + for (const dir of dirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); +}); + +function makeStore() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "citadel-review-ops-")); + dirs.push(dir); + const store = new SqliteStore(path.join(dir, "citadel.sqlite")); + store.migrate(); + return { dir, store }; +} + +function makeRepo(dir: string, opts: { requestReviewHookIds?: string[] } = {}): Repo { + const now = new Date().toISOString(); + return { + id: "repo_1", + name: "Repo", + rootPath: path.join(dir, "repo"), + defaultBranch: "main", + defaultRemote: "origin", + worktreeParent: path.join(dir, "wt"), + setupHookIds: [], + teardownHookIds: [], + requestReviewHookIds: opts.requestReviewHookIds ?? [], + providerIds: [], + deployHookCommand: null, + createdAt: now, + updatedAt: now, + archivedAt: null, + }; +} + +function makeWorkspace(dir: string): Workspace { + const now = new Date().toISOString(); + return { + id: "ws_1", + repoId: "repo_1", + name: "ws", + path: path.join(dir, "wt", "ws"), + branch: "feature", + baseBranch: "main", + source: "scratch", + kind: "worktree", + prUrl: null, + issueKey: null, + issueTitle: null, + issueUrl: null, + slackThreadUrl: null, + section: "default", + pinned: false, + lifecycle: "ready", + dirty: false, + namespaceId: null, + createdAt: now, + updatedAt: now, + archivedAt: null, + }; +} + +function hookConfig(command: string, args: string[] = []): HookConfig { + return { + id: "rev", + kind: "command" as const, + event: "workspace.requestReview" as const, + command, + args, + blocking: true, + }; +} + +function seed(opts: { hooks?: HookConfig[]; requestReviewHookIds?: string[] } = {}) { + const { dir, store } = makeStore(); + const repo = makeRepo(dir, { requestReviewHookIds: opts.requestReviewHookIds }); + const workspace = makeWorkspace(dir); + fs.mkdirSync(workspace.path, { recursive: true }); + store.insertRepo(repo); + store.insertWorkspace(workspace); + const activity = vi.fn(); + const config = { hooks: opts.hooks ?? [], commandPolicy: { hookTimeoutMs: 5000, allowDestructiveWorkspaceCleanup: false } }; + return { dir, store, repo, workspace, activity, config }; +} + +describe("requestReviewForWorkspace", () => { + it("returns no-hook when nothing configured", async () => { + const f = seed(); + const result = await requestReviewForWorkspace({ + store: f.store, + config: f.config, + activity: f.activity, + repo: f.repo, + workspace: f.workspace, + diff: { files: [], addedLines: 0, deletedLines: 0, truncated: false }, + }); + expect(result.kind).toBe("no-hook"); + expect(f.activity).not.toHaveBeenCalled(); + expect(f.store.latestReviewSuggestionRun(f.workspace.id)).toBeNull(); + }); + + it("records succeeded run and exactly one activity row when the hook emits valid JSON", async () => { + const hookOutput = JSON.stringify({ + suggestions: [{ id: "s1", kind: "reviewer", label: "@alice" }], + }); + const f = seed({ + hooks: [hookConfig("node", ["-e", `process.stdout.write(${JSON.stringify(hookOutput)})`])], + requestReviewHookIds: ["rev"], + }); + const result = await requestReviewForWorkspace({ + store: f.store, + config: f.config, + activity: f.activity, + repo: f.repo, + workspace: f.workspace, + diff: { files: [], addedLines: 0, deletedLines: 0, truncated: false }, + }); + expect(result.kind).toBe("succeeded"); + if (result.kind === "succeeded") { + expect(result.output.suggestions[0]?.label).toBe("@alice"); + expect(result.run.status).toBe("succeeded"); + } + expect(f.activity).toHaveBeenCalledTimes(1); + expect(f.activity).toHaveBeenCalledWith( + "hook.workspace.requestReview", + "hook", + expect.stringContaining("returned 1 suggestion"), + f.repo.id, + f.workspace.id, + null, + ); + }); + + it("treats an empty-stdout success as zero suggestions and still records a succeeded run", async () => { + const f = seed({ + hooks: [hookConfig("node", ["-e", "process.exit(0)"])], + requestReviewHookIds: ["rev"], + }); + const result = await requestReviewForWorkspace({ + store: f.store, + config: f.config, + activity: f.activity, + repo: f.repo, + workspace: f.workspace, + diff: { files: [], addedLines: 0, deletedLines: 0, truncated: false }, + }); + expect(result.kind).toBe("succeeded"); + if (result.kind === "succeeded") { + expect(result.output.suggestions).toEqual([]); + expect(result.run.output?.suggestions).toEqual([]); + } + }); + + it("records a failed run and a failed activity when the hook returns invalid JSON", async () => { + const f = seed({ + hooks: [hookConfig("node", ["-e", "process.stdout.write('{nope')"])], + requestReviewHookIds: ["rev"], + }); + const result = await requestReviewForWorkspace({ + store: f.store, + config: f.config, + activity: f.activity, + repo: f.repo, + workspace: f.workspace, + diff: { files: [], addedLines: 0, deletedLines: 0, truncated: false }, + }); + expect(result.kind).toBe("failed"); + expect(f.activity).toHaveBeenCalledTimes(1); + expect(f.activity).toHaveBeenCalledWith( + "hook.workspace.requestReview.failed", + "hook", + expect.stringContaining("invalid output"), + f.repo.id, + f.workspace.id, + null, + ); + }); + + it("records a failed run when the hook exits non-zero", async () => { + const f = seed({ + hooks: [hookConfig("node", ["-e", "process.stderr.write('nope'); process.exit(2)"])], + requestReviewHookIds: ["rev"], + }); + const result = await requestReviewForWorkspace({ + store: f.store, + config: f.config, + activity: f.activity, + repo: f.repo, + workspace: f.workspace, + diff: { files: [], addedLines: 0, deletedLines: 0, truncated: false }, + }); + expect(result.kind).toBe("failed"); + if (result.kind === "failed") { + expect(result.run.exitStatus).toBe(2); + expect(result.run.stderr).toContain("nope"); + } + }); + + it("records a timed_out run when the hook exceeds the configured timeout", async () => { + const f = seed({ + hooks: [hookConfig("node", ["-e", "setTimeout(() => {}, 5000)"])], + requestReviewHookIds: ["rev"], + }); + f.config.commandPolicy.hookTimeoutMs = 50; + const result = await requestReviewForWorkspace({ + store: f.store, + config: f.config, + activity: f.activity, + repo: f.repo, + workspace: f.workspace, + diff: { files: [], addedLines: 0, deletedLines: 0, truncated: false }, + }); + expect(result.kind).toBe("timed-out"); + if (result.kind === "timed-out") expect(result.run.status).toBe("timed_out"); + expect(f.activity).toHaveBeenCalledWith( + "hook.workspace.requestReview.failed", + "hook", + expect.stringContaining("timed out"), + f.repo.id, + f.workspace.id, + null, + ); + }); +}); + +describe("review comment service", () => { + it("addReviewComment persists and logs activity exactly once", () => { + const f = seed(); + const row = addReviewComment({ + store: f.store, + activity: f.activity, + workspaceId: f.workspace.id, + body: "looks good", + author: "operator", + repoId: f.repo.id, + }); + expect(row.author).toBe("operator"); + expect(f.activity).toHaveBeenCalledTimes(1); + expect(f.activity).toHaveBeenCalledWith( + "review.comment.added", + "user", + expect.stringContaining("added by operator"), + f.repo.id, + f.workspace.id, + null, + ); + expect(listReviewComments({ store: f.store, workspaceId: f.workspace.id })).toHaveLength(1); + }); + + it("updateReviewComment returns conflict on stale token and does not log activity", () => { + const f = seed(); + const created = addReviewComment({ + store: f.store, + activity: f.activity, + workspaceId: f.workspace.id, + body: "v1", + author: "operator", + repoId: f.repo.id, + }); + f.activity.mockClear(); + const result = updateReviewComment({ + store: f.store, + activity: f.activity, + id: created.id, + body: "v2", + ifUpdatedAtMatches: "1970-01-01T00:00:00.000Z", + repoId: f.repo.id, + }); + expect(result.kind).toBe("conflict"); + expect(f.activity).not.toHaveBeenCalled(); + }); + + it("updateReviewComment with status=resolved logs review.comment.resolved", () => { + const f = seed(); + const created = addReviewComment({ + store: f.store, + activity: f.activity, + workspaceId: f.workspace.id, + body: "v1", + author: "operator", + repoId: f.repo.id, + }); + f.activity.mockClear(); + const result = updateReviewComment({ + store: f.store, + activity: f.activity, + id: created.id, + status: "resolved", + ifUpdatedAtMatches: created.updatedAt, + repoId: f.repo.id, + }); + expect(result.kind).toBe("updated"); + expect(f.activity).toHaveBeenCalledWith( + "review.comment.resolved", + "user", + expect.stringContaining("resolved"), + f.repo.id, + f.workspace.id, + null, + ); + }); + + it("deleteReviewComment soft-deletes and logs once", () => { + const f = seed(); + const created = addReviewComment({ + store: f.store, + activity: f.activity, + workspaceId: f.workspace.id, + body: "v1", + author: "operator", + repoId: f.repo.id, + }); + f.activity.mockClear(); + const result = deleteReviewComment({ + store: f.store, + activity: f.activity, + id: created.id, + ifUpdatedAtMatches: created.updatedAt, + repoId: f.repo.id, + }); + expect(result.kind).toBe("updated"); + expect(listReviewComments({ store: f.store, workspaceId: f.workspace.id })).toHaveLength(0); + expect(f.activity).toHaveBeenCalledWith( + "review.comment.deleted", + "user", + expect.stringContaining("deleted"), + f.repo.id, + f.workspace.id, + null, + ); + }); +}); diff --git a/packages/operations/src/review-system.ts b/packages/operations/src/review-system.ts new file mode 100644 index 00000000..4d41ee19 --- /dev/null +++ b/packages/operations/src/review-system.ts @@ -0,0 +1,292 @@ +import type { CitadelConfig, HookConfig } from "@citadel/config"; +import type { + HookOutput, + Repo, + ReviewComment, + ReviewSuggestionRun, + ReviewSuggestionsOutput, + Workspace, +} from "@citadel/contracts"; +import { createId, nowIso } from "@citadel/core"; +import type { SqliteStore } from "@citadel/db"; +import { parseReviewSuggestionsOutput, runCommandHookForDiagnostics } from "@citadel/hooks"; +import { commandHook } from "./hooks-runner.js"; + +type ActivityFn = ( + type: string, + source: "user" | "system" | "hook", + message: string, + repoId: string | null, + workspaceId: string | null, + operationId: string | null, + hookOutput?: HookOutput | null, +) => void; + +type ReviewServiceConfig = { + hooks: HookConfig[]; + commandPolicy: CitadelConfig["commandPolicy"]; +}; + +export type RequestReviewResult = + | { kind: "no-hook" } + | { kind: "succeeded"; run: ReviewSuggestionRun; output: ReviewSuggestionsOutput } + | { kind: "failed"; run: ReviewSuggestionRun; error: string } + | { kind: "timed-out"; run: ReviewSuggestionRun }; + +export type RequestReviewInput = { + store: SqliteStore; + config: ReviewServiceConfig | undefined; + activity: ActivityFn; + repo: Repo; + workspace: Workspace; + diff: { files: string[]; addedLines: number; deletedLines: number; truncated: boolean }; +}; + +const TIMEOUT_MARKER = "Hook timed out after"; + +export async function requestReviewForWorkspace( + input: RequestReviewInput, +): Promise { + const { store, config, activity, repo, workspace, diff } = input; + const ids = repo.requestReviewHookIds ?? []; + const hooks = (config?.hooks ?? []).filter( + (hook) => hook.event === "workspace.requestReview" && ids.includes(hook.id), + ); + const hook = hooks[0]; + if (!hook) return { kind: "no-hook" }; + + const payload = { + event: "workspace.requestReview" as const, + workspace, + repo, + pr: { + url: workspace.prUrl, + branch: workspace.branch, + baseBranch: workspace.baseBranch, + }, + diff, + }; + const startedAt = Date.now(); + const timeoutMs = config?.commandPolicy.hookTimeoutMs ?? 120_000; + + try { + const result = await runCommandHookForDiagnostics( + commandHook(hook, workspace.path, config), + payload, + ); + const durationMs = result.durationMs; + if (result.exitStatus !== 0) { + const stderr = result.stderr.slice(-4000) || null; + const message = `Hook exited with ${result.exitStatus}`; + const run = store.insertReviewSuggestionRun({ + id: createId("rsr"), + workspaceId: workspace.id, + hookId: hook.id, + status: "failed", + durationMs, + exitStatus: result.exitStatus, + output: null, + stderr, + error: message, + }); + activity( + "hook.workspace.requestReview.failed", + "hook", + `Hook ${hook.id} failed: ${message}`, + repo.id, + workspace.id, + null, + ); + return { kind: "failed", run, error: message }; + } + let parsed: ReviewSuggestionsOutput | null = null; + try { + parsed = parseReviewSuggestionsOutput(result.stdout); + } catch (error) { + const message = error instanceof Error ? error.message : "Invalid review suggestions payload"; + const run = store.insertReviewSuggestionRun({ + id: createId("rsr"), + workspaceId: workspace.id, + hookId: hook.id, + status: "failed", + durationMs, + exitStatus: 0, + output: null, + stderr: result.stderr.slice(-4000) || null, + error: message, + }); + activity( + "hook.workspace.requestReview.failed", + "hook", + `Hook ${hook.id} returned invalid output: ${message}`, + repo.id, + workspace.id, + null, + ); + return { kind: "failed", run, error: message }; + } + const output: ReviewSuggestionsOutput = parsed ?? { + suggestions: [], + generatedAt: null, + metadata: {}, + }; + const run = store.insertReviewSuggestionRun({ + id: createId("rsr"), + workspaceId: workspace.id, + hookId: hook.id, + status: "succeeded", + durationMs, + exitStatus: 0, + output, + stderr: result.stderr.slice(-4000) || null, + error: null, + }); + activity( + "hook.workspace.requestReview", + "hook", + `Hook ${hook.id} returned ${output.suggestions.length} suggestion(s)`, + repo.id, + workspace.id, + null, + ); + return { kind: "succeeded", run, output }; + } catch (error) { + const message = error instanceof Error ? error.message : "Hook failed"; + const elapsed = Date.now() - startedAt; + const isTimeout = message.includes(TIMEOUT_MARKER) || elapsed >= timeoutMs; + const status: ReviewSuggestionRun["status"] = isTimeout ? "timed_out" : "failed"; + const run = store.insertReviewSuggestionRun({ + id: createId("rsr"), + workspaceId: workspace.id, + hookId: hook.id, + status, + durationMs: elapsed, + exitStatus: null, + output: null, + stderr: null, + error: message, + }); + activity( + "hook.workspace.requestReview.failed", + "hook", + `Hook ${hook.id} ${isTimeout ? "timed out" : "failed"}: ${message}`, + repo.id, + workspace.id, + null, + ); + return isTimeout ? { kind: "timed-out", run } : { kind: "failed", run, error: message }; + } +} + +export type AddReviewCommentInput = { + store: SqliteStore; + activity: ActivityFn; + workspaceId: string; + body: string; + author: string; + repoId: string; + filePath?: string | null; + lineStart?: number | null; + lineEnd?: number | null; + side?: "LEFT" | "RIGHT" | null; +}; + +export function addReviewComment(input: AddReviewCommentInput): ReviewComment { + const id = createId("rc"); + const now = nowIso(); + const row = input.store.insertReviewComment({ + id, + workspaceId: input.workspaceId, + author: input.author, + body: input.body, + filePath: input.filePath ?? null, + lineStart: input.lineStart ?? null, + lineEnd: input.lineEnd ?? null, + side: input.side ?? null, + createdAt: now, + updatedAt: now, + }); + input.activity( + "review.comment.added", + "user", + `Comment ${row.id} added by ${input.author}`, + input.repoId, + input.workspaceId, + null, + ); + return row; +} + +export type UpdateReviewCommentInput = { + store: SqliteStore; + activity: ActivityFn; + id: string; + body?: string; + status?: ReviewComment["status"]; + ifUpdatedAtMatches: string; + repoId: string; +}; + +export type UpdateReviewCommentResult = + | { kind: "updated"; row: ReviewComment } + | { kind: "conflict"; latest: ReviewComment } + | { kind: "not-found" }; + +export function updateReviewComment(input: UpdateReviewCommentInput): UpdateReviewCommentResult { + const patch: { body?: string; status?: ReviewComment["status"] } = {}; + if (input.body !== undefined) patch.body = input.body; + if (input.status !== undefined) patch.status = input.status; + const result = input.store.updateReviewComment(input.id, patch, input.ifUpdatedAtMatches); + if (result.kind === "updated") { + const eventType = + input.status === "resolved" && result.row.status === "resolved" + ? "review.comment.resolved" + : "review.comment.updated"; + input.activity( + eventType, + "user", + `Comment ${result.row.id} ${input.status === "resolved" ? "resolved" : "updated"}`, + input.repoId, + result.row.workspaceId, + null, + ); + } + return result; +} + +export type DeleteReviewCommentInput = { + store: SqliteStore; + activity: ActivityFn; + id: string; + ifUpdatedAtMatches: string; + repoId: string; +}; + +export function deleteReviewComment(input: DeleteReviewCommentInput): UpdateReviewCommentResult { + const result = input.store.softDeleteReviewComment(input.id, input.ifUpdatedAtMatches); + if (result.kind === "updated") { + input.activity( + "review.comment.deleted", + "user", + `Comment ${result.row.id} deleted`, + input.repoId, + result.row.workspaceId, + null, + ); + } + return result; +} + +export type ListReviewCommentsInput = { + store: SqliteStore; + workspaceId: string; + status?: "open" | "resolved" | "all"; + includeDeleted?: boolean; +}; + +export function listReviewComments(input: ListReviewCommentsInput): ReviewComment[] { + const opts: { status?: "open" | "resolved" | "all"; includeDeleted?: boolean } = {}; + if (input.status !== undefined) opts.status = input.status; + if (input.includeDeleted !== undefined) opts.includeDeleted = input.includeDeleted; + return input.store.listReviewComments(input.workspaceId, opts); +} From 57879029c9d0e8d62b8b9032bb54a4a8f0592bff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:44:49 +0000 Subject: [PATCH 08/18] feat(operations,web): teach HookEvent consumers about workspace.requestReview 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) --- apps/web/src/structured-config.tsx | 10 +++- packages/operations/src/helpers.ts | 6 +- packages/operations/src/index.ts | 2 +- .../src/list-hook-diagnostics.test.ts | 59 +++++++++++++++++++ packages/operations/src/review-system.test.ts | 2 +- 5 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 packages/operations/src/list-hook-diagnostics.test.ts diff --git a/apps/web/src/structured-config.tsx b/apps/web/src/structured-config.tsx index 05622a2d..4d57e297 100644 --- a/apps/web/src/structured-config.tsx +++ b/apps/web/src/structured-config.tsx @@ -24,6 +24,7 @@ type HookConfig = { | "workspace.teardown" | "workspace.apps" | "workspace.action" + | "workspace.requestReview" | "workspace.created" | "workspace.archived" | "workspace.removed" @@ -52,7 +53,13 @@ type ConfigResponse = { runtimes: RuntimeConfig[]; usageProviders: UsageProviderConfig[]; hooks: HookConfig[]; - repoDefaults: { setupHookIds: string[]; teardownHookIds: string[] }; + repoDefaults: { + setupHookIds: string[]; + teardownHookIds: string[]; + appHookIds?: string[]; + actionHookIds?: string[]; + requestReviewHookIds?: string[]; + }; commandPolicy: { hookTimeoutMs: number; allowDestructiveWorkspaceCleanup: boolean }; }; configPath: string; @@ -63,6 +70,7 @@ const HOOK_EVENTS: HookConfig["event"][] = [ "workspace.teardown", "workspace.apps", "workspace.action", + "workspace.requestReview", "workspace.created", "workspace.archived", "workspace.removed", diff --git a/packages/operations/src/helpers.ts b/packages/operations/src/helpers.ts index 4aef9ecb..ec1c311e 100644 --- a/packages/operations/src/helpers.ts +++ b/packages/operations/src/helpers.ts @@ -387,6 +387,7 @@ export function listHookDiagnostics(input: { hooks: HookConfig[]; appHookIds: string[]; actionHookIds: string[]; + requestReviewHookIds: string[]; hookTimeoutMs: number; }): HookDiagnostic[] { const events: Array = [ @@ -394,6 +395,7 @@ export function listHookDiagnostics(input: { "workspace.teardown", "workspace.apps", "workspace.action", + "workspace.requestReview", ]; return events.flatMap((event) => { const ids = @@ -403,7 +405,9 @@ export function listHookDiagnostics(input: { ? input.repo.teardownHookIds : event === "workspace.apps" ? input.appHookIds - : input.actionHookIds; + : event === "workspace.action" + ? input.actionHookIds + : input.requestReviewHookIds; const eventHooks = input.hooks.filter((hook) => hook.event === event); const filtered = ids.length ? eventHooks.filter((hook) => ids.includes(hook.id)) : eventHooks; return filtered.map((hook) => diff --git a/packages/operations/src/index.ts b/packages/operations/src/index.ts index 37b9366d..d92c75de 100644 --- a/packages/operations/src/index.ts +++ b/packages/operations/src/index.ts @@ -706,7 +706,7 @@ export class OperationService { workspace, hooks: this.config?.hooks ?? [], appHookIds: this.config?.repoDefaults.appHookIds ?? [], - actionHookIds: this.config?.repoDefaults.actionHookIds ?? [], + actionHookIds: this.config?.repoDefaults.actionHookIds ?? [], requestReviewHookIds: this.config?.repoDefaults.requestReviewHookIds ?? [], hookTimeoutMs: this.config?.commandPolicy.hookTimeoutMs ?? 120000, }); diff --git a/packages/operations/src/list-hook-diagnostics.test.ts b/packages/operations/src/list-hook-diagnostics.test.ts new file mode 100644 index 00000000..66df52f1 --- /dev/null +++ b/packages/operations/src/list-hook-diagnostics.test.ts @@ -0,0 +1,59 @@ +import type { HookConfig } from "@citadel/config"; +import type { Repo } from "@citadel/contracts"; +import { describe, expect, it } from "vitest"; +import { listHookDiagnostics } from "./helpers.js"; + +const baseRepo: Repo = { + id: "repo_1", + name: "Repo", + rootPath: "/tmp/repo", + defaultBranch: "main", + defaultRemote: "origin", + worktreeParent: "/tmp/wt", + setupHookIds: [], + teardownHookIds: [], + requestReviewHookIds: ["rev"], + providerIds: [], + deployHookCommand: null, + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + archivedAt: null, +}; + +const reviewHook: HookConfig = { + id: "rev", + kind: "command", + event: "workspace.requestReview", + command: "true", + args: [], + blocking: true, +}; + +describe("listHookDiagnostics", () => { + it("surfaces workspace.requestReview hooks in the diagnostics list", () => { + const diagnostics = listHookDiagnostics({ + repo: baseRepo, + hooks: [reviewHook], + appHookIds: [], + actionHookIds: [], + requestReviewHookIds: baseRepo.requestReviewHookIds, + hookTimeoutMs: 5_000, + }); + expect(diagnostics.map((d) => d.event)).toContain("workspace.requestReview"); + expect(diagnostics.find((d) => d.event === "workspace.requestReview")?.hookId).toBe("rev"); + }); + + it("filters by requestReviewHookIds when the array is non-empty", () => { + const otherHook: HookConfig = { ...reviewHook, id: "other" }; + const diagnostics = listHookDiagnostics({ + repo: baseRepo, + hooks: [reviewHook, otherHook], + appHookIds: [], + actionHookIds: [], + requestReviewHookIds: ["rev"], + hookTimeoutMs: 5_000, + }); + const reviewDiagnostics = diagnostics.filter((d) => d.event === "workspace.requestReview"); + expect(reviewDiagnostics.map((d) => d.hookId)).toEqual(["rev"]); + }); +}); diff --git a/packages/operations/src/review-system.test.ts b/packages/operations/src/review-system.test.ts index ecd43c9a..dfa14559 100644 --- a/packages/operations/src/review-system.test.ts +++ b/packages/operations/src/review-system.test.ts @@ -27,7 +27,7 @@ function makeStore() { return { dir, store }; } -function makeRepo(dir: string, opts: { requestReviewHookIds?: string[] } = {}): Repo { +function makeRepo(dir: string, opts: { requestReviewHookIds?: string[] | undefined } = {}): Repo { const now = new Date().toISOString(); return { id: "repo_1", From 0d27c01359478ba3d43d313efeb2b8bb34fd1bfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:49:25 +0000 Subject: [PATCH 09/18] feat(daemon): add review-routes.ts for comments + request-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- apps/daemon/src/app.ts | 2 + apps/daemon/src/review-routes.test.ts | 294 ++++++++++++++++++++++++++ apps/daemon/src/review-routes.ts | 236 +++++++++++++++++++++ packages/operations/src/index.ts | 2 +- 4 files changed, 533 insertions(+), 1 deletion(-) create mode 100644 apps/daemon/src/review-routes.test.ts create mode 100644 apps/daemon/src/review-routes.ts diff --git a/apps/daemon/src/app.ts b/apps/daemon/src/app.ts index c8ddba08..748d56de 100644 --- a/apps/daemon/src/app.ts +++ b/apps/daemon/src/app.ts @@ -37,6 +37,7 @@ import { callDaemonMcpTool, readMcpResource } from "./daemon-mcp-tool.js"; import { registerWorkspaceExtraRoutes } from "./extra-routes.js"; import { registerMcpRoutes } from "./mcp-routes.js"; import { registerNamespaceRoutes } from "./namespace-routes.js"; +import { registerReviewRoutes } from "./review-routes.js"; import { deriveReadiness, workspaceAppHookSample } from "./readiness.js"; import { registerRuntimeUsageRoutes } from "./runtime-usage-routes.js"; import { registerScheduledAgentRoutes } from "./scheduled-agent-routes.js"; @@ -710,6 +711,7 @@ export function createDaemonApp(input: { registerWorkspaceExtraRoutes({ app, store, emit, asyncRoute, operations }); registerNamespaceRoutes({ app, store, operations, emit, asyncRoute }); + registerReviewRoutes({ app, store, config, asyncRoute }); registerScratchpadRoutes({ app, config, emit }); try { const spPath = scratchpadPath(config.dataDir); diff --git a/apps/daemon/src/review-routes.test.ts b/apps/daemon/src/review-routes.test.ts new file mode 100644 index 00000000..9321a2f7 --- /dev/null +++ b/apps/daemon/src/review-routes.test.ts @@ -0,0 +1,294 @@ +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import type http from "node:http"; +import os from "node:os"; +import path from "node:path"; +import { loadConfig } from "@citadel/config"; +import type { ReviewComment, ReviewSuggestionRun } from "@citadel/contracts"; +import { SqliteStore } from "@citadel/db"; +import { afterEach, describe, expect, it } from "vitest"; +import { createDaemonApp } from "./app.js"; + +const dirs: string[] = []; + +afterEach(() => { + for (const dir of dirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); +}); + +process.env.CITADEL_DISABLE_REAPER = "1"; +process.env.CITADEL_DISABLE_SCHEDULER = "1"; + +describe("review routes — comments", () => { + it("supports a POST → GET → PATCH(409) → PATCH(200) → DELETE round-trip", async () => { + const fixture = createFixture(); + const { repoId, workspaceId } = seedRepoAndWorkspace(fixture); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + // 404 on unknown workspace + const unknown = await fetch(`${baseUrl}/api/workspaces/ws_missing/review-comments`); + expect(unknown.status).toBe(404); + + // POST 201 + const created = await postJson<{ comment: ReviewComment }>( + `${baseUrl}/api/workspaces/${workspaceId}/review-comments`, + { body: "Looks good but check this edge case" }, + ); + expect(created.comment.author).toBe("operator"); + expect(created.comment.workspaceId).toBe(workspaceId); + + // Second comment via clean body — confirms author stays 'operator' even + // on subsequent posts. + await postJson<{ comment: ReviewComment }>( + `${baseUrl}/api/workspaces/${workspaceId}/review-comments`, + { body: "another" }, + ); + + // GET 200 with two comments + const list = await getJson<{ comments: ReviewComment[] }>( + `${baseUrl}/api/workspaces/${workspaceId}/review-comments`, + ); + expect(list.comments).toHaveLength(2); + expect(list.comments.every((c) => c.author === "operator")).toBe(true); + + // PATCH 409 with stale token + const stale = await fetch(`${baseUrl}/api/review-comments/${created.comment.id}`, { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ body: "v2", ifUpdatedAtMatches: "1970-01-01T00:00:00.000Z" }), + }); + expect(stale.status).toBe(409); + + // PATCH 200 with fresh token + const fresh = await fetch(`${baseUrl}/api/review-comments/${created.comment.id}`, { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ status: "resolved", ifUpdatedAtMatches: created.comment.updatedAt }), + }); + expect(fresh.status).toBe(200); + const updated = (await fresh.json()) as { comment: ReviewComment }; + expect(updated.comment.status).toBe("resolved"); + + // DELETE 204 with fresh token + const del = await fetch(`${baseUrl}/api/review-comments/${created.comment.id}`, { + method: "DELETE", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ ifUpdatedAtMatches: updated.comment.updatedAt }), + }); + expect(del.status).toBe(204); + + // GET hides soft-deleted by default + const after = await getJson<{ comments: ReviewComment[] }>( + `${baseUrl}/api/workspaces/${workspaceId}/review-comments`, + ); + expect(after.comments).toHaveLength(1); + expect(after.comments[0]?.id).not.toBe(created.comment.id); + + // includeDeleted=true brings it back + const withDeleted = await getJson<{ comments: ReviewComment[] }>( + `${baseUrl}/api/workspaces/${workspaceId}/review-comments?includeDeleted=true`, + ); + expect(withDeleted.comments).toHaveLength(2); + } finally { + await closeServer(server); + } + // ensure repoId is referenced for the typed fixture + expect(repoId).toMatch(/^repo_/); + }); + + it("rejects an add request that supplies an author field", async () => { + const fixture = createFixture(); + const { workspaceId } = seedRepoAndWorkspace(fixture); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + const r = await fetch(`${baseUrl}/api/workspaces/${workspaceId}/review-comments`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ body: "hi", author: "agent:rogue" }), + }); + expect(r.status).toBe(400); + } finally { + await closeServer(server); + } + }); +}); + +describe("review routes — request_review", () => { + it("returns no-hook when none configured", async () => { + const fixture = createFixture(); + const { workspaceId } = seedRepoAndWorkspace(fixture); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + const r = await fetch(`${baseUrl}/api/workspaces/${workspaceId}/review-requests`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: "{}", + }); + expect(r.status).toBe(400); + expect((await r.json()) as { error: string }).toEqual({ error: "no-hook" }); + } finally { + await closeServer(server); + } + }); + + it("returns parsed suggestions when the configured hook succeeds", async () => { + const fixture = createFixture(); + const { workspaceId } = seedRepoAndWorkspace(fixture, { + hookCommand: "node", + hookArgs: [ + "-e", + "process.stdout.write(JSON.stringify({suggestions:[{id:'s1',kind:'reviewer',label:'@alice'}]}))", + ], + }); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + const r = await fetch(`${baseUrl}/api/workspaces/${workspaceId}/review-requests`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: "{}", + }); + expect(r.status).toBe(200); + const body = (await r.json()) as { run: ReviewSuggestionRun; output: { suggestions: { id: string }[] } }; + expect(body.output.suggestions[0]?.id).toBe("s1"); + // GET latest matches + const latest = await getJson<{ run: ReviewSuggestionRun | null }>( + `${baseUrl}/api/workspaces/${workspaceId}/review-suggestions`, + ); + expect(latest.run?.status).toBe("succeeded"); + } finally { + await closeServer(server); + } + }); +}); + +// --- helpers --------------------------------------------------------------- + +function createFixture() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "citadel-review-routes-")); + dirs.push(dir); + const configPath = path.join(dir, "citadel.config.json"); + const config = loadConfig(configPath); + config.dataDir = dir; + config.databasePath = path.join(dir, "citadel.sqlite"); + config.providers = { + github: { enabled: false, command: "gh" }, + jira: { enabled: false, command: "jtk" }, + }; + config.runtimes = [{ id: "shell", displayName: "Shell", command: "bash", args: ["-l"] }]; + const store = new SqliteStore(config.databasePath); + store.migrate(); + return { config, configPath, store }; +} + +function seedRepoAndWorkspace( + fixture: ReturnType, + opts: { hookCommand?: string; hookArgs?: string[] } = {}, +) { + const git = createGitRepo(fixture.config.dataDir); + const now = new Date().toISOString(); + const repoId = `repo_${Date.now().toString(36)}`; + const requestReviewHookIds: string[] = []; + if (opts.hookCommand) { + const hookId = `rev_${Date.now().toString(36)}`; + fixture.config.hooks = [ + ...(fixture.config.hooks ?? []), + { + id: hookId, + kind: "command", + event: "workspace.requestReview", + command: opts.hookCommand, + args: opts.hookArgs ?? [], + blocking: true, + }, + ]; + requestReviewHookIds.push(hookId); + } + fixture.store.insertRepo({ + id: repoId, + name: "Repo", + rootPath: git.repoPath, + defaultBranch: "main", + defaultRemote: "origin", + worktreeParent: path.join(fixture.config.dataDir, "worktrees"), + setupHookIds: [], + teardownHookIds: [], + requestReviewHookIds, + providerIds: [], + deployHookCommand: null, + createdAt: now, + updatedAt: now, + archivedAt: null, + }); + const workspaceId = `ws_${Date.now().toString(36)}`; + fixture.store.insertWorkspace({ + id: workspaceId, + repoId, + name: "ws", + path: git.repoPath, + branch: "main", + baseBranch: "main", + source: "scratch", + kind: "worktree", + prUrl: null, + issueKey: null, + issueTitle: null, + issueUrl: null, + slackThreadUrl: null, + section: "default", + pinned: false, + lifecycle: "ready", + dirty: false, + namespaceId: null, + createdAt: now, + updatedAt: now, + archivedAt: null, + }); + return { repoId, workspaceId }; +} + +function createGitRepo(dir: string) { + const repoPath = path.join(dir, `repo-${Date.now().toString(36)}`); + fs.mkdirSync(repoPath, { recursive: true }); + execFileSync("git", ["init"], { cwd: repoPath, stdio: "pipe" }); + execFileSync("git", ["config", "user.email", "test@example.test"], { cwd: repoPath, stdio: "pipe" }); + execFileSync("git", ["config", "user.name", "Citadel Test"], { cwd: repoPath, stdio: "pipe" }); + fs.writeFileSync(path.join(repoPath, "README.md"), "# fixture\n"); + execFileSync("git", ["add", "README.md"], { cwd: repoPath, stdio: "pipe" }); + execFileSync("git", ["commit", "-m", "initial"], { cwd: repoPath, stdio: "pipe" }); + execFileSync("git", ["branch", "-M", "main"], { cwd: repoPath, stdio: "pipe" }); + return { repoPath }; +} + +function listen(server: http.Server) { + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + if (!address || typeof address === "string") throw new Error("Expected TCP test server address"); + resolve(`http://127.0.0.1:${address.port}`); + }); + }); +} + +function closeServer(server: http.Server) { + return new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }); +} + +async function getJson(url: string) { + const response = await fetch(url); + expect(response.ok).toBe(true); + return response.json() as Promise; +} + +async function postJson(url: string, body: unknown) { + const response = await fetch(url, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); + return response.json() as Promise; +} diff --git a/apps/daemon/src/review-routes.ts b/apps/daemon/src/review-routes.ts new file mode 100644 index 00000000..832afc5f --- /dev/null +++ b/apps/daemon/src/review-routes.ts @@ -0,0 +1,236 @@ +import type { CitadelConfig } from "@citadel/config"; +import type { HookOutput, ReviewComment, ReviewSuggestionRun, Workspace } from "@citadel/contracts"; +import type { SqliteStore } from "@citadel/db"; +import { + addReviewComment as addReviewCommentImpl, + deleteReviewComment as deleteReviewCommentImpl, + listReviewComments as listReviewCommentsImpl, + requestReviewForWorkspace, + updateReviewComment as updateReviewCommentImpl, +} from "@citadel/operations"; +import type express from "express"; +import { readWorkspaceDiff } from "./workspace-diff.js"; +import { z } from "zod"; + +const StatusQuerySchema = z.enum(["open", "resolved", "all"]).default("all"); + +const AddCommentBodySchema = z + .object({ + body: z.string().min(1).max(8000), + filePath: z.string().min(1).max(512).nullable().optional(), + lineStart: z.number().int().min(1).nullable().optional(), + lineEnd: z.number().int().min(1).nullable().optional(), + side: z.enum(["LEFT", "RIGHT"]).nullable().optional(), + }) + .strict() + .superRefine((value, ctx) => { + if (value.lineEnd != null && value.lineStart != null && value.lineEnd < value.lineStart) { + ctx.addIssue({ code: z.ZodIssueCode.custom, path: ["lineEnd"], message: "lineEnd must be >= lineStart" }); + } + if ((value.lineStart != null || value.lineEnd != null || value.side != null) && !value.filePath) { + ctx.addIssue({ code: z.ZodIssueCode.custom, path: ["filePath"], message: "anchor requires filePath" }); + } + }); + +const UpdateCommentBodySchema = z + .object({ + body: z.string().min(1).max(8000).optional(), + status: z.enum(["open", "resolved"]).optional(), + ifUpdatedAtMatches: z.string().min(1), + }) + .strict() + .refine((v) => v.body !== undefined || v.status !== undefined, "empty_patch"); + +const DeleteCommentBodySchema = z.object({ ifUpdatedAtMatches: z.string().min(1) }).strict(); + +type ReviewRoutesDeps = { + app: express.Express; + store: SqliteStore; + config: CitadelConfig; + asyncRoute: ( + handler: (req: express.Request, res: express.Response, next: express.NextFunction) => Promise, + ) => express.RequestHandler; + recordActivity?: (event: { + type: string; + source: "user" | "system" | "hook"; + message: string; + repoId: string | null; + workspaceId: string | null; + hookOutput?: HookOutput | null; + }) => void; +}; + +export function registerReviewRoutes(deps: ReviewRoutesDeps) { + const { app, store, config, asyncRoute } = deps; + const recordActivity = + deps.recordActivity ?? + ((event) => { + store.addActivity({ + id: `evt_${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 10)}`, + type: event.type, + source: event.source, + repoId: event.repoId, + workspaceId: event.workspaceId, + operationId: null, + message: event.message, + hookOutput: event.hookOutput ?? null, + createdAt: new Date().toISOString(), + }); + }); + + const activity = ( + type: string, + source: "user" | "system" | "hook", + message: string, + repoId: string | null, + workspaceId: string | null, + ) => recordActivity({ type, source, message, repoId, workspaceId }); + + const resolveWorkspace = (workspaceId: string): Workspace | null => { + const list = store.listWorkspaces(); + return list.find((w) => w.id === workspaceId) ?? null; + }; + + app.get( + "/api/workspaces/:workspaceId/review-comments", + asyncRoute(async (req, res) => { + const workspaceId = String(req.params.workspaceId); + if (!resolveWorkspace(workspaceId)) return res.status(404).json({ error: "workspace_not_found" }); + const statusParse = StatusQuerySchema.safeParse(req.query.status ?? "all"); + if (!statusParse.success) return res.status(400).json({ error: "invalid_status" }); + const includeDeleted = req.query.includeDeleted === "true"; + const comments = listReviewCommentsImpl({ + store, + workspaceId, + status: statusParse.data, + includeDeleted, + }); + res.json({ comments }); + }), + ); + + app.post( + "/api/workspaces/:workspaceId/review-comments", + asyncRoute(async (req, res) => { + const workspaceId = String(req.params.workspaceId); + const workspace = resolveWorkspace(workspaceId); + if (!workspace) return res.status(404).json({ error: "workspace_not_found" }); + const parsed = AddCommentBodySchema.safeParse(req.body); + if (!parsed.success) return res.status(400).json({ error: "invalid_body", detail: parsed.error.message }); + const row = addReviewCommentImpl({ + store, + activity, + workspaceId, + body: parsed.data.body, + author: "operator", + repoId: workspace.repoId, + filePath: parsed.data.filePath ?? null, + lineStart: parsed.data.lineStart ?? null, + lineEnd: parsed.data.lineEnd ?? null, + side: parsed.data.side ?? null, + }); + res.status(201).json({ comment: row }); + }), + ); + + app.patch( + "/api/review-comments/:commentId", + asyncRoute(async (req, res) => { + const commentId = String(req.params.commentId); + const parsed = UpdateCommentBodySchema.safeParse(req.body); + if (!parsed.success) return res.status(400).json({ error: "invalid_body", detail: parsed.error.message }); + const existing = store.getReviewComment(commentId); + if (!existing || existing.deletedAt) return res.status(404).json({ error: "comment_not_found" }); + const workspace = resolveWorkspace(existing.workspaceId); + const updateInput: Parameters[0] = { + store, + activity, + id: commentId, + ifUpdatedAtMatches: parsed.data.ifUpdatedAtMatches, + repoId: workspace?.repoId ?? "", + }; + if (parsed.data.body !== undefined) updateInput.body = parsed.data.body; + if (parsed.data.status !== undefined) updateInput.status = parsed.data.status; + const result = updateReviewCommentImpl(updateInput); + if (result.kind === "not-found") return res.status(404).json({ error: "comment_not_found" }); + if (result.kind === "conflict") + return res.status(409).json({ error: "conflict", latest: result.latest }); + return res.json({ comment: result.row }); + }), + ); + + app.delete( + "/api/review-comments/:commentId", + asyncRoute(async (req, res) => { + const commentId = String(req.params.commentId); + const parsed = DeleteCommentBodySchema.safeParse(req.body); + if (!parsed.success) return res.status(400).json({ error: "invalid_body", detail: parsed.error.message }); + const existing = store.getReviewComment(commentId); + if (!existing || existing.deletedAt) return res.status(404).json({ error: "comment_not_found" }); + const workspace = resolveWorkspace(existing.workspaceId); + const result = deleteReviewCommentImpl({ + store, + activity, + id: commentId, + ifUpdatedAtMatches: parsed.data.ifUpdatedAtMatches, + repoId: workspace?.repoId ?? "", + }); + if (result.kind === "not-found") return res.status(404).json({ error: "comment_not_found" }); + if (result.kind === "conflict") + return res.status(409).json({ error: "conflict", latest: result.latest }); + return res.status(204).end(); + }), + ); + + app.get( + "/api/workspaces/:workspaceId/review-suggestions", + asyncRoute(async (req, res) => { + const workspaceId = String(req.params.workspaceId); + if (!resolveWorkspace(workspaceId)) return res.status(404).json({ error: "workspace_not_found" }); + const latest: ReviewSuggestionRun | null = store.latestReviewSuggestionRun(workspaceId); + res.json({ run: latest }); + }), + ); + + app.post( + "/api/workspaces/:workspaceId/review-requests", + asyncRoute(async (req, res) => { + const workspaceId = String(req.params.workspaceId); + const workspace = resolveWorkspace(workspaceId); + if (!workspace) return res.status(404).json({ error: "workspace_not_found" }); + const repos = store.listRepos(); + const repo = repos.find((r) => r.id === workspace.repoId); + if (!repo) return res.status(404).json({ error: "repo_not_found" }); + const diffSummary = (() => { + try { + const diff = readWorkspaceDiff(workspace.id, workspace.path); + return { + files: diff.files.map((f) => f.path), + addedLines: diff.addedLines, + deletedLines: diff.deletedLines, + truncated: diff.truncated, + }; + } catch { + return { files: [], addedLines: 0, deletedLines: 0, truncated: false }; + } + })(); + const result = await requestReviewForWorkspace({ + store, + config: { hooks: config.hooks, commandPolicy: config.commandPolicy }, + activity, + repo, + workspace, + diff: diffSummary, + }); + if (result.kind === "no-hook") return res.status(400).json({ error: "no-hook" }); + if (result.kind === "succeeded") + return res.json({ run: result.run, output: result.output }); + if (result.kind === "timed-out") + return res.status(504).json({ error: "timed-out", run: result.run }); + return res.status(502).json({ error: "hook-failed", run: result.run, message: result.error }); + }), + ); +} + +export type RegisteredReviewRoutes = ReturnType; +export type { ReviewComment }; diff --git a/packages/operations/src/index.ts b/packages/operations/src/index.ts index d92c75de..bae54a64 100644 --- a/packages/operations/src/index.ts +++ b/packages/operations/src/index.ts @@ -65,7 +65,7 @@ export { WorkspaceNameTakenError, } from "./helpers.js"; import { runNotificationHooks, runWorkspaceHooks } from "./hooks-runner.js"; -export type { RequestReviewResult, UpdateReviewCommentResult } from "./review-system.js"; +export { addReviewComment, deleteReviewComment, listReviewComments, requestReviewForWorkspace, updateReviewComment } from "./review-system.js"; import { type WorkspaceAppsDeps, discoverWorkspaceApps as discoverWorkspaceAppsImpl, From 2cda55d3eacec52c61c9e1080a40eb06ab992995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:53:58 +0000 Subject: [PATCH 10/18] feat(mcp): register five review-system tools (list/add/update/delete/request) callMcpTool routes all five to a single review_tool_requires_daemon sentinel; daemon-side handler implements the actual flows. Author is stamped agent:; 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) --- .../daemon/src/daemon-mcp-tool-review.test.ts | 201 ++++++++++++++++++ apps/daemon/src/daemon-mcp-tool.ts | 139 ++++++++++++ packages/mcp/src/index.test.ts | 35 +++ packages/mcp/src/index.ts | 95 ++++++++- 4 files changed, 469 insertions(+), 1 deletion(-) create mode 100644 apps/daemon/src/daemon-mcp-tool-review.test.ts diff --git a/apps/daemon/src/daemon-mcp-tool-review.test.ts b/apps/daemon/src/daemon-mcp-tool-review.test.ts new file mode 100644 index 00000000..463329b2 --- /dev/null +++ b/apps/daemon/src/daemon-mcp-tool-review.test.ts @@ -0,0 +1,201 @@ +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import type http from "node:http"; +import os from "node:os"; +import path from "node:path"; +import { loadConfig } from "@citadel/config"; +import type { ReviewComment, ReviewSuggestionRun } from "@citadel/contracts"; +import { SqliteStore } from "@citadel/db"; +import { afterEach, describe, expect, it } from "vitest"; +import { createDaemonApp } from "./app.js"; + +const dirs: string[] = []; + +afterEach(() => { + for (const dir of dirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); +}); + +process.env.CITADEL_DISABLE_REAPER = "1"; +process.env.CITADEL_DISABLE_SCHEDULER = "1"; + +describe("daemon MCP review tools", () => { + it("add_review_comment stamps agent: author and rejects caller-supplied author", async () => { + const { fixture, workspaceId } = seedFixture(); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + const addResp = await mcpCall<{ comment: ReviewComment }>(baseUrl, { + name: "add_review_comment", + arguments: { workspaceId, body: "hi from agent", runtimeId: "claude" }, + }); + expect(addResp.result.comment.author).toBe("agent:claude"); + + const spoofResp = await mcpCall<{ error: string }>(baseUrl, { + name: "add_review_comment", + arguments: { workspaceId, body: "spoof", author: "operator", runtimeId: "claude" }, + }); + expect(spoofResp.result.error).toBe("author_not_allowed"); + } finally { + await closeServer(server); + } + }); + + it("update_review_comment returns conflict on stale ifUpdatedAtMatches", async () => { + const { fixture, workspaceId } = seedFixture(); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + const add = await mcpCall<{ comment: ReviewComment }>(baseUrl, { + name: "add_review_comment", + arguments: { workspaceId, body: "v1", runtimeId: "claude" }, + }); + const stale = await mcpCall<{ error: string; latest: ReviewComment }>(baseUrl, { + name: "update_review_comment", + arguments: { id: add.result.comment.id, body: "v2", ifUpdatedAtMatches: "1970-01-01T00:00:00.000Z" }, + }); + expect(stale.result.error).toBe("conflict"); + expect(stale.result.latest.id).toBe(add.result.comment.id); + } finally { + await closeServer(server); + } + }); + + it("request_review returns no-hook when nothing configured", async () => { + const { fixture, workspaceId } = seedFixture(); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + const resp = await mcpCall<{ error: string }>(baseUrl, { + name: "request_review", + arguments: { workspaceId }, + }); + expect(resp.result.error).toBe("no-hook"); + } finally { + await closeServer(server); + } + }); + + it("list_review_comments + delete_review_comment round-trip", async () => { + const { fixture, workspaceId } = seedFixture(); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + const add = await mcpCall<{ comment: ReviewComment }>(baseUrl, { + name: "add_review_comment", + arguments: { workspaceId, body: "to delete", runtimeId: "claude" }, + }); + const list = await mcpCall<{ comments: ReviewComment[] }>(baseUrl, { + name: "list_review_comments", + arguments: { workspaceId }, + }); + expect(list.result.comments).toHaveLength(1); + + const del = await mcpCall<{ ok: true }>(baseUrl, { + name: "delete_review_comment", + arguments: { id: add.result.comment.id, ifUpdatedAtMatches: add.result.comment.updatedAt }, + }); + expect(del.result.ok).toBe(true); + + const listAfter = await mcpCall<{ comments: ReviewComment[] }>(baseUrl, { + name: "list_review_comments", + arguments: { workspaceId }, + }); + expect(listAfter.result.comments).toHaveLength(0); + } finally { + await closeServer(server); + } + }); +}); + +// --- helpers --------------------------------------------------------------- + +function seedFixture() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "citadel-mcp-review-")); + dirs.push(dir); + const configPath = path.join(dir, "citadel.config.json"); + const config = loadConfig(configPath); + config.dataDir = dir; + config.databasePath = path.join(dir, "citadel.sqlite"); + config.providers = { + github: { enabled: false, command: "gh" }, + jira: { enabled: false, command: "jtk" }, + }; + config.runtimes = [{ id: "shell", displayName: "Shell", command: "bash", args: ["-l"] }]; + const store = new SqliteStore(config.databasePath); + store.migrate(); + const repoPath = path.join(dir, "repo"); + fs.mkdirSync(repoPath, { recursive: true }); + execFileSync("git", ["init"], { cwd: repoPath, stdio: "pipe" }); + execFileSync("git", ["config", "user.email", "test@example.test"], { cwd: repoPath, stdio: "pipe" }); + execFileSync("git", ["config", "user.name", "Citadel Test"], { cwd: repoPath, stdio: "pipe" }); + fs.writeFileSync(path.join(repoPath, "README.md"), "# fixture\n"); + execFileSync("git", ["add", "README.md"], { cwd: repoPath, stdio: "pipe" }); + execFileSync("git", ["commit", "-m", "initial"], { cwd: repoPath, stdio: "pipe" }); + execFileSync("git", ["branch", "-M", "main"], { cwd: repoPath, stdio: "pipe" }); + const now = new Date().toISOString(); + store.insertRepo({ + id: "repo_1", + name: "Repo", + rootPath: repoPath, + defaultBranch: "main", + defaultRemote: "origin", + worktreeParent: path.join(dir, "wt"), + setupHookIds: [], + teardownHookIds: [], + requestReviewHookIds: [], + providerIds: [], + deployHookCommand: null, + createdAt: now, + updatedAt: now, + archivedAt: null, + }); + store.insertWorkspace({ + id: "ws_1", + repoId: "repo_1", + name: "ws", + path: repoPath, + branch: "main", + baseBranch: "main", + source: "scratch", + kind: "worktree", + prUrl: null, + issueKey: null, + issueTitle: null, + issueUrl: null, + slackThreadUrl: null, + section: "default", + pinned: false, + lifecycle: "ready", + dirty: false, + namespaceId: null, + createdAt: now, + updatedAt: now, + archivedAt: null, + }); + return { fixture: { config, store, configPath }, workspaceId: "ws_1" }; +} + +function listen(server: http.Server) { + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + if (!address || typeof address === "string") throw new Error("Expected TCP test server address"); + resolve(`http://127.0.0.1:${address.port}`); + }); + }); +} + +function closeServer(server: http.Server) { + return new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }); +} + +async function mcpCall(baseUrl: string, body: { name: string; arguments: Record }) { + const response = await fetch(`${baseUrl}/api/mcp/tools/call`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); + return (await response.json()) as { result: T }; +} diff --git a/apps/daemon/src/daemon-mcp-tool.ts b/apps/daemon/src/daemon-mcp-tool.ts index 5f4598bb..c63215e3 100644 --- a/apps/daemon/src/daemon-mcp-tool.ts +++ b/apps/daemon/src/daemon-mcp-tool.ts @@ -19,6 +19,11 @@ import { type ScheduledAgentRunner, WorkspaceInUseError, WorkspaceNameTakenError, + addReviewComment, + deleteReviewComment, + listReviewComments, + requestReviewForWorkspace, + updateReviewComment, } from "@citadel/operations"; import { collectProviderHealth } from "@citadel/providers"; import { listRuntimeHealth } from "@citadel/runtimes"; @@ -378,6 +383,15 @@ export async function callDaemonMcpTool(deps: DaemonMcpDeps, call: McpToolCall) if ("kind" in slice) return { error: "log_file_missing" }; return slice; } + if ( + call.name === "list_review_comments" || + call.name === "add_review_comment" || + call.name === "update_review_comment" || + call.name === "delete_review_comment" || + call.name === "request_review" + ) { + return handleReviewTool(deps, call); + } const providerHealth = await collectProviderHealth(config.providers); return callMcpTool(call, { repos: store.listRepos(), @@ -395,3 +409,128 @@ export async function callDaemonMcpTool(deps: DaemonMcpDeps, call: McpToolCall) sessionPromptSummary: (sessionId) => operations.getSessionPromptSummary(sessionId), }); } + +async function handleReviewTool(deps: DaemonMcpDeps, call: McpToolCall): Promise { + const { store, config } = deps; + const args = (call.arguments ?? {}) as Record; + const activity = ( + type: string, + source: "user" | "system" | "hook", + message: string, + repoId: string | null, + workspaceId: string | null, + ) => { + store.addActivity({ + id: `evt_${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 10)}`, + type, + source, + repoId, + workspaceId, + operationId: null, + message, + hookOutput: null, + createdAt: new Date().toISOString(), + }); + }; + const resolveWorkspace = (workspaceId: string) => + store.listWorkspaces().find((w) => w.id === workspaceId) ?? null; + + if (call.name === "list_review_comments") { + const workspaceId = typeof args.workspaceId === "string" ? args.workspaceId : ""; + const workspace = resolveWorkspace(workspaceId); + if (!workspace) return { error: "workspace_not_found" }; + const status = (args.status as "open" | "resolved" | "all" | undefined) ?? "all"; + const includeDeleted = args.includeDeleted === true; + const comments = listReviewComments({ store, workspaceId, status, includeDeleted }); + return { comments }; + } + + if (call.name === "add_review_comment") { + if ("author" in args) return { error: "author_not_allowed" }; + const workspaceId = typeof args.workspaceId === "string" ? args.workspaceId : ""; + const workspace = resolveWorkspace(workspaceId); + if (!workspace) return { error: "workspace_not_found" }; + const body = typeof args.body === "string" ? args.body : ""; + if (!body) return { error: "body_required" }; + const filePath = (args.filePath as string | undefined) ?? null; + const lineStart = (args.lineStart as number | undefined) ?? null; + const lineEnd = (args.lineEnd as number | undefined) ?? null; + const side = (args.side as "LEFT" | "RIGHT" | undefined) ?? null; + const runtimeId = typeof args.runtimeId === "string" ? args.runtimeId : "unknown"; + const row = addReviewComment({ + store, + activity, + workspaceId, + body, + author: `agent:${runtimeId}`, + repoId: workspace.repoId, + filePath, + lineStart, + lineEnd, + side, + }); + return { comment: row }; + } + + if (call.name === "update_review_comment") { + const id = typeof args.id === "string" ? args.id : ""; + const ifUpdatedAtMatches = typeof args.ifUpdatedAtMatches === "string" ? args.ifUpdatedAtMatches : ""; + if (!id || !ifUpdatedAtMatches) return { error: "invalid_input" }; + const existing = store.getReviewComment(id); + if (!existing || existing.deletedAt) return { error: "comment_not_found" }; + const workspace = resolveWorkspace(existing.workspaceId); + const result = updateReviewComment({ + store, + activity, + id, + ...(typeof args.body === "string" ? { body: args.body } : {}), + ...(args.status === "open" || args.status === "resolved" ? { status: args.status } : {}), + ifUpdatedAtMatches, + repoId: workspace?.repoId ?? "", + }); + if (result.kind === "not-found") return { error: "comment_not_found" }; + if (result.kind === "conflict") return { error: "conflict", latest: result.latest }; + return { comment: result.row }; + } + + if (call.name === "delete_review_comment") { + const id = typeof args.id === "string" ? args.id : ""; + const ifUpdatedAtMatches = typeof args.ifUpdatedAtMatches === "string" ? args.ifUpdatedAtMatches : ""; + if (!id || !ifUpdatedAtMatches) return { error: "invalid_input" }; + const existing = store.getReviewComment(id); + if (!existing || existing.deletedAt) return { error: "comment_not_found" }; + const workspace = resolveWorkspace(existing.workspaceId); + const result = deleteReviewComment({ + store, + activity, + id, + ifUpdatedAtMatches, + repoId: workspace?.repoId ?? "", + }); + if (result.kind === "not-found") return { error: "comment_not_found" }; + if (result.kind === "conflict") return { error: "conflict", latest: result.latest }; + return { ok: true }; + } + + if (call.name === "request_review") { + const workspaceId = typeof args.workspaceId === "string" ? args.workspaceId : ""; + const workspace = resolveWorkspace(workspaceId); + if (!workspace) return { error: "workspace_not_found" }; + const repos = store.listRepos(); + const repo = repos.find((r) => r.id === workspace.repoId); + if (!repo) return { error: "repo_not_found" }; + const result = await requestReviewForWorkspace({ + store, + config: { hooks: config.hooks, commandPolicy: config.commandPolicy }, + activity, + repo, + workspace, + diff: { files: [], addedLines: 0, deletedLines: 0, truncated: false }, + }); + if (result.kind === "no-hook") return { error: "no-hook" }; + if (result.kind === "succeeded") return { run: result.run, output: result.output }; + if (result.kind === "timed-out") return { error: "timed-out", run: result.run }; + return { error: "hook-failed", run: result.run, message: result.error }; + } + return { error: "unknown_review_tool" }; +} diff --git a/packages/mcp/src/index.test.ts b/packages/mcp/src/index.test.ts index 9dc05ffe..253e5a19 100644 --- a/packages/mcp/src/index.test.ts +++ b/packages/mcp/src/index.test.ts @@ -22,6 +22,11 @@ describe("mcp helpers", () => { expect(mcpToolDefinitions().find((tool) => tool.name === "delete_scheduled_agent")?.destructive).toBe(true); expect(status.tools).toContain("list_workspace_links"); expect(status.tools).toContain("read_agent_output"); + expect(status.tools).toContain("list_review_comments"); + expect(status.tools).toContain("add_review_comment"); + expect(status.tools).toContain("update_review_comment"); + expect(status.tools).toContain("delete_review_comment"); + expect(status.tools).toContain("request_review"); expect(status.tools).toContain("send_agent_message"); const launch = mcpToolDefinitions().find((tool) => tool.name === "launch_agent"); expect(launch).toBeDefined(); @@ -282,5 +287,35 @@ describe("mcp helpers", () => { ).toEqual({ error: "session_tool_requires_daemon", }); + expect(callMcpTool({ name: "list_review_comments", arguments: { workspaceId: "ws_test" } }, context)).toEqual({ + error: "review_tool_requires_daemon", + }); + expect( + callMcpTool({ name: "add_review_comment", arguments: { workspaceId: "ws_test", body: "hi" } }, context), + ).toEqual({ error: "review_tool_requires_daemon" }); + expect(callMcpTool({ name: "request_review", arguments: { workspaceId: "ws_test" } }, context)).toEqual({ + error: "review_tool_requires_daemon", + }); + }); + + it("includes the five review tools in the inventory with correct destructive flags", () => { + const definitions = mcpToolDefinitions(); + const names = definitions.map((t) => t.name); + expect(names).toEqual( + expect.arrayContaining([ + "list_review_comments", + "add_review_comment", + "update_review_comment", + "delete_review_comment", + "request_review", + ]), + ); + expect(definitions.find((t) => t.name === "update_review_comment")?.destructive).toBe(true); + expect(definitions.find((t) => t.name === "delete_review_comment")?.destructive).toBe(true); + const add = definitions.find((t) => t.name === "add_review_comment"); + expect(add?.destructive).toBe(false); + // schema must not list `author` — agents cannot supply it + const addProps = (add?.inputSchema as { properties?: Record })?.properties ?? {}; + expect(Object.keys(addProps)).not.toContain("author"); }); }); diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 8e3d0295..e6399e7d 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -61,7 +61,12 @@ export type McpToolName = | "delete_scheduled_agent" | "run_scheduled_agent_now" | "list_scheduled_agent_runs" - | "read_scheduled_agent_run_log"; + | "read_scheduled_agent_run_log" + | "list_review_comments" + | "add_review_comment" + | "update_review_comment" + | "delete_review_comment" + | "request_review"; export type McpToolDefinition = { name: McpToolName; @@ -572,6 +577,85 @@ export function mcpToolDefinitions(): McpToolDefinition[] { }, destructive: false, }, + { + name: "list_review_comments", + description: + "List Citadel-native review comments for a workspace. Comments are stored in Citadel's SQLite (not GitHub); each carries optional file/line anchors and a status of 'open' or 'resolved'. Returns newest-first. status defaults to 'all'; includeDeleted defaults to false.", + inputSchema: { + type: "object", + required: ["workspaceId"], + properties: { + workspaceId: { type: "string", minLength: 1 }, + status: { type: "string", enum: ["open", "resolved", "all"] }, + includeDeleted: { type: "boolean" }, + }, + additionalProperties: false, + }, + destructive: false, + }, + { + name: "add_review_comment", + description: + "Add a Citadel-native review comment on a workspace. The daemon stamps author='agent:'; callers cannot supply author. Anchors are optional — pass filePath + lineStart (+ optional lineEnd, side) to scope to a file:line range.", + inputSchema: { + type: "object", + required: ["workspaceId", "body"], + properties: { + workspaceId: { type: "string", minLength: 1 }, + body: { type: "string", minLength: 1, maxLength: 8000 }, + filePath: { type: "string", minLength: 1, maxLength: 512 }, + lineStart: { type: "integer", minimum: 1 }, + lineEnd: { type: "integer", minimum: 1 }, + side: { type: "string", enum: ["LEFT", "RIGHT"] }, + }, + additionalProperties: false, + }, + destructive: false, + }, + { + name: "update_review_comment", + description: + "Update a review comment's body and/or status. Requires ifUpdatedAtMatches (the comment's last-read updatedAt) — mismatched tokens return { error: 'conflict', latest } so the caller can re-read and retry.", + inputSchema: { + type: "object", + required: ["id", "ifUpdatedAtMatches"], + properties: { + id: { type: "string", minLength: 1 }, + body: { type: "string", minLength: 1, maxLength: 8000 }, + status: { type: "string", enum: ["open", "resolved"] }, + ifUpdatedAtMatches: { type: "string", minLength: 1 }, + }, + additionalProperties: false, + }, + destructive: true, + }, + { + name: "delete_review_comment", + description: + "Soft-delete a review comment. Requires ifUpdatedAtMatches; returns { error: 'conflict', latest } on stale token. Soft-deleted comments stay readable via list_review_comments({ includeDeleted: true }).", + inputSchema: { + type: "object", + required: ["id", "ifUpdatedAtMatches"], + properties: { + id: { type: "string", minLength: 1 }, + ifUpdatedAtMatches: { type: "string", minLength: 1 }, + }, + additionalProperties: false, + }, + destructive: true, + }, + { + name: "request_review", + description: + "Invoke the repo's configured workspace.requestReview hook on a workspace and return the structured suggestions output. Returns { error: 'no-hook' } when nothing is configured, { error: 'hook-failed' | 'timed-out' } on hook failure.", + inputSchema: { + type: "object", + required: ["workspaceId"], + properties: { workspaceId: { type: "string", minLength: 1 } }, + additionalProperties: false, + }, + destructive: false, + }, ]; } @@ -641,6 +725,15 @@ export function callMcpTool(call: McpToolCall, context: McpToolContext) { operations: context.operations.filter((operation) => operation.workspaceId === workspaceId).slice(0, 5), }; } + case "list_review_comments": + case "request_review": + case "add_review_comment": + case "update_review_comment": + case "delete_review_comment": + // Review tools require the daemon for either fs access (DB writes) or + // hook execution. The in-process path holds no daemon handle, so we + // route to the daemon-side dispatcher. + return { error: "review_tool_requires_daemon" }; case "register_repo": case "create_workspace": case "start_agent_session": From 55d855db6abe1e941f22bbe03fe5bec8290666e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ovidiu=20M=C4=83ru=C8=99?= Date: Mon, 25 May 2026 23:56:07 +0000 Subject: [PATCH 11/18] feat(web): add Review tab with request-review + comments composer 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) --- apps/web/src/inspector-review.tsx | 242 ++++++++++++++++++++++++++++++ apps/web/src/inspector.tsx | 21 ++- 2 files changed, 261 insertions(+), 2 deletions(-) create mode 100644 apps/web/src/inspector-review.tsx diff --git a/apps/web/src/inspector-review.tsx b/apps/web/src/inspector-review.tsx new file mode 100644 index 00000000..51705fe2 --- /dev/null +++ b/apps/web/src/inspector-review.tsx @@ -0,0 +1,242 @@ +import type { + ReviewComment, + ReviewSuggestion, + ReviewSuggestionRun, + Workspace, + WorkspaceDiff, +} from "@citadel/contracts"; +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { useState } from "react"; +import { api } from "./api.js"; + +export type ReviewTabProps = { + workspace: Workspace; + diff: WorkspaceDiff | undefined; + hasRequestReviewHook: boolean; +}; + +export function ReviewTab(props: ReviewTabProps) { + return ( +
+ + +
+ ); +} + +export function RequestReviewPanel(props: { workspace: Workspace; hasHook: boolean }) { + const queryClient = useQueryClient(); + const latest = useQuery<{ run: ReviewSuggestionRun | null }>({ + queryKey: ["review-suggestions", props.workspace.id], + queryFn: () => api(`/api/workspaces/${props.workspace.id}/review-suggestions`), + }); + const mutation = useMutation({ + mutationFn: () => + api<{ run: ReviewSuggestionRun; output: { suggestions: ReviewSuggestion[] } }>( + `/api/workspaces/${props.workspace.id}/review-requests`, + { method: "POST", body: JSON.stringify({}) }, + ), + onSuccess: () => queryClient.invalidateQueries({ queryKey: ["review-suggestions", props.workspace.id] }), + }); + + const run = latest.data?.run; + const suggestions = run?.output?.suggestions ?? []; + const status = mutation.isPending ? "loading" : run?.status ?? "idle"; + return ( +
+
+

Request review

+ +
+ {status === "failed" || status === "timed_out" ? ( +

+ Hook {status === "timed_out" ? "timed out" : "failed"}: {run?.error ?? "unknown error"} +

+ ) : null} + {mutation.isError ? ( +

{(mutation.error as Error).message}

+ ) : null} + {status === "succeeded" && suggestions.length === 0 ? ( +

Hook returned no suggestions.

+ ) : null} + {suggestions.length > 0 ? ( +
    + {suggestions.map((s) => ( +
  • + {s.label} + {s.detail ? {s.detail} : null} + {s.url ? ( + + open + + ) : null} +
    • + ))} +
+ ) : null} +
+ ); +} + +export function ReviewCommentsPanel(props: { workspace: Workspace; diff: WorkspaceDiff | undefined }) { + const queryClient = useQueryClient(); + const [body, setBody] = useState(""); + const [filePath, setFilePath] = useState(""); + const [lineStart, setLineStart] = useState(""); + + const list = useQuery<{ comments: ReviewComment[] }>({ + queryKey: ["review-comments", props.workspace.id], + queryFn: () => api(`/api/workspaces/${props.workspace.id}/review-comments`), + }); + + const invalidate = () => + queryClient.invalidateQueries({ queryKey: ["review-comments", props.workspace.id] }); + + const addMutation = useMutation({ + mutationFn: (input: { body: string; filePath?: string; lineStart?: number }) => + api<{ comment: ReviewComment }>(`/api/workspaces/${props.workspace.id}/review-comments`, { + method: "POST", + body: JSON.stringify(input), + }), + onSuccess: () => { + setBody(""); + setFilePath(""); + setLineStart(""); + invalidate(); + }, + }); + + const submit = () => { + if (!body.trim()) return; + const payload: { body: string; filePath?: string; lineStart?: number } = { body }; + if (filePath) payload.filePath = filePath; + if (filePath && lineStart) { + const n = Number.parseInt(lineStart, 10); + if (Number.isFinite(n) && n >= 1) payload.lineStart = n; + } + addMutation.mutate(payload); + }; + + const comments = list.data?.comments ?? []; + const open = comments.filter((c) => c.status === "open"); + const resolved = comments.filter((c) => c.status === "resolved"); + + return ( +
+
+

Comments

+
+
+