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). 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-helpers.ts b/apps/daemon/src/app-helpers.ts index 72dc1270..cc01409b 100644 --- a/apps/daemon/src/app-helpers.ts +++ b/apps/daemon/src/app-helpers.ts @@ -28,3 +28,23 @@ export async function cachedProviderValue( cache.set(key, { expiresAt: now + ttlMs, value }); return value; } + +// Narrows a RuntimeConfig down to the spawn-arg subset OperationService wants, +// and coerces optionals to `null` so exactOptionalPropertyTypes stays happy. +export function runtimeSpawnArgs(runtime: { + command: string; + args: string[]; + displayName: string; + promptArg?: string | undefined; + sessionIdArg?: string | undefined; + resumeArg?: string | undefined; +}) { + return { + command: runtime.command, + args: runtime.args, + displayName: runtime.displayName, + promptArg: runtime.promptArg ?? null, + sessionIdArg: runtime.sessionIdArg ?? null, + resumeArg: runtime.resumeArg ?? null, + }; +} 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/app.ts b/apps/daemon/src/app.ts index 8d0b7c51..5a21abdc 100644 --- a/apps/daemon/src/app.ts +++ b/apps/daemon/src/app.ts @@ -32,13 +32,13 @@ import cors from "cors"; import express from "express"; import { ZodError } from "zod"; import { registerAgentSessionRoutes } from "./agent-session-routes.js"; -import { registerRestoreRoutes } from "./restore-routes.js"; -import { asyncRoute, cachedProviderValue } from "./app-helpers.js"; +import { asyncRoute, cachedProviderValue, runtimeSpawnArgs } from "./app-helpers.js"; 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 { deriveReadiness, workspaceAppHookSample } from "./readiness.js"; +import { registerRestoreRoutes } from "./restore-routes.js"; import { registerRuntimeUsageRoutes } from "./runtime-usage-routes.js"; import { registerScheduledAgentRoutes } from "./scheduled-agent-routes.js"; import { backfillIfEmpty } from "./scratchpad-history.js"; @@ -119,9 +119,8 @@ export function createDaemonApp(input: { payload, }; for (const client of sseClients) client.write(`event: ${type}\ndata: ${JSON.stringify(event)}\n\n`); - if (fsWatchers && (type === "workspace.updated" || type === "state.reconciled" || type === "repo.updated")) { + if (fsWatchers && (type === "workspace.updated" || type === "state.reconciled" || type === "repo.updated")) fsWatchers.reconcile(); - } }; // Terminal/ttyd proxy must register before the SPA fallback so it owns /terminals/*. @@ -505,14 +504,7 @@ export function createDaemonApp(input: { const input = CreateAgentSessionInputSchema.parse(req.body); const runtime = config.runtimes.find((candidate) => candidate.id === input.runtimeId); if (!runtime) return res.status(404).json({ error: "runtime_not_found" }); - const session = await operations.createAgentSession(input, { - command: runtime.command, - args: runtime.args, - displayName: runtime.displayName, - promptArg: runtime.promptArg ?? null, - sessionIdArg: runtime.sessionIdArg ?? null, - resumeArg: runtime.resumeArg ?? null, - }); + const session = await operations.createAgentSession(input, runtimeSpawnArgs(runtime)); emit("agent.updated", { workspaceId: session.workspaceId, sessionId: session.id }); res.status(202).json({ session }); }), @@ -566,10 +558,7 @@ export function createDaemonApp(input: { }), ); - app.get("/api/operations", (_req, res) => { - res.json({ operations: store.listOperations() }); - }); - + app.get("/api/operations", (_req, res) => res.json({ operations: store.listOperations() })); app.get("/api/operations/:operationId", (req, res) => { const operation = store.findOperation(String(req.params.operationId)); if (!operation) return res.status(404).json({ error: "operation_not_found" }); @@ -585,12 +574,12 @@ export function createDaemonApp(input: { if (typeof patch.name === "string" && patch.name.length) allowed.name = patch.name; if (typeof patch.worktreeParent === "string" && patch.worktreeParent.length) allowed.worktreeParent = patch.worktreeParent; - if (Array.isArray(patch.setupHookIds)) - allowed.setupHookIds = patch.setupHookIds.filter((id: unknown) => typeof id === "string"); - if (Array.isArray(patch.teardownHookIds)) - allowed.teardownHookIds = patch.teardownHookIds.filter((id: unknown) => typeof id === "string"); - if (Array.isArray(patch.providerIds)) - allowed.providerIds = patch.providerIds.filter((id: unknown) => typeof id === "string"); + const stringArrayKeys = ["setupHookIds", "teardownHookIds", "requestReviewHookIds", "providerIds"] as const; + for (const key of stringArrayKeys) { + if (Array.isArray((patch as Record)[key])) { + allowed[key] = ((patch as Record)[key] as unknown[]).filter((id) => typeof id === "string"); + } + } if (typeof patch.deployHookCommand === "string") allowed.deployHookCommand = patch.deployHookCommand.trim() || null; else if (patch.deployHookCommand === null) allowed.deployHookCommand = null; @@ -694,15 +683,13 @@ export function createDaemonApp(input: { emit, asyncRoute, }); - // Boot-sweep: close any 'running' run rows that were in flight when the - // daemon last died, sync the denormalized lastRunStatus cache on the - // affected agents, kill orphan background tmux sessions, and drain queued - // rows that were waiting on the failed in-flight predecessors. Best-effort: - // we don't want a sweep failure to block startup, but we DO want a signal - // because a silent failure leaves orphaned 'running' rows behind. - void scheduledAgents.recoverInFlightRuns().catch((error) => { - console.error("[citadel] scheduledAgents.recoverInFlightRuns failed:", error); - }); + // Boot-sweep: close any in-flight run rows, sync denormalized lastRunStatus, + // kill orphan background tmux sessions, drain queued rows blocked on failed + // predecessors. Best-effort: don't block startup, but log because silent + // failure leaves orphan 'running' rows behind. + void scheduledAgents + .recoverInFlightRuns() + .catch((error) => console.error("[citadel] scheduledAgents.recoverInFlightRuns failed:", error)); const mcpDeps = { config, store, operations, ttyd, scheduledAgents, scheduledAgentService, providerCache, emit }; registerMcpRoutes(app, asyncRoute, { @@ -712,7 +699,7 @@ export function createDaemonApp(input: { readMcpResource: (uri) => readMcpResource(store, config, uri), }); - registerWorkspaceExtraRoutes({ app, store, emit, asyncRoute, operations }); + registerWorkspaceExtraRoutes({ app, store, emit, asyncRoute, operations, config }); registerNamespaceRoutes({ app, store, operations, emit, asyncRoute }); registerScratchpadRoutes({ app, config, emit }); try { 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..c6951545 --- /dev/null +++ b/apps/daemon/src/daemon-mcp-tool-review.test.ts @@ -0,0 +1,212 @@ +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:unknown and refuses caller-supplied author OR runtimeId", async () => { + const { fixture, workspaceId } = seedFixture(); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + // Baseline: a plain call with no identity field stamps 'agent:unknown' + // (the schema doesn't declare runtimeId, and the daemon dispatcher does + // not honor caller-supplied identity until the transport surfaces it). + const addResp = await mcpCall<{ comment: ReviewComment }>(baseUrl, { + name: "add_review_comment", + arguments: { workspaceId, body: "hi from agent" }, + }); + expect(addResp.result.comment.author).toBe("agent:unknown"); + + // Spoof attempt #1: explicit `author` is refused. + const spoofAuthor = await mcpCall<{ error: string }>(baseUrl, { + name: "add_review_comment", + arguments: { workspaceId, body: "spoof", author: "operator" }, + }); + expect(spoofAuthor.result.error).toBe("author_not_allowed"); + + // Spoof attempt #2: undocumented `runtimeId` is also refused. + const spoofRuntime = await mcpCall<{ error: string }>(baseUrl, { + name: "add_review_comment", + arguments: { workspaceId, body: "spoof", runtimeId: "claude" }, + }); + expect(spoofRuntime.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" }, + }); + 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" }, + }); + 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 4b7f2534..a202984b 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"; @@ -36,6 +41,7 @@ import { updateBlock, writeScratchpad, } from "./scratchpad.js"; +import { readWorkspaceDiffSummary } from "./workspace-diff.js"; export type DaemonMcpDeps = { config: CitadelConfig; @@ -382,6 +388,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(), @@ -399,3 +414,133 @@ 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") { + // Reject any attempt to spoof author identity. The schema also marks + // additionalProperties:false, but the in-process dispatcher does not + // re-validate args against the schema, so we belt-and-braces here: + // both `author` (the documented schema field) and `runtimeId` (an + // undocumented field a caller might try to inject) are refused. Until + // the MCP transport exposes per-client identity, the daemon stamps + // 'agent:unknown'. + if ("author" in args || "runtimeId" 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 row = addReviewComment({ + store, + activity, + workspaceId, + body, + author: "agent:unknown", + 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: readWorkspaceDiffSummary(workspace.id, workspace.path), + }); + 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/apps/daemon/src/extra-routes.ts b/apps/daemon/src/extra-routes.ts index 316bfaa8..8599d5a5 100644 --- a/apps/daemon/src/extra-routes.ts +++ b/apps/daemon/src/extra-routes.ts @@ -1,9 +1,11 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; +import type { CitadelConfig } from "@citadel/config"; import type { SqliteStore } from "@citadel/db"; import type { OperationService } from "@citadel/operations"; import type express from "express"; +import { registerReviewRoutes } from "./review-routes.js"; type Emit = (type: string, payload: unknown) => void; type AsyncHandler = (req: express.Request, res: express.Response, next: express.NextFunction) => Promise; @@ -17,8 +19,10 @@ export function registerWorkspaceExtraRoutes(input: { emit: Emit; asyncRoute: AsyncRoute; operations: OperationService; + config: CitadelConfig; }) { - const { app, store, emit, asyncRoute, operations } = input; + const { app, store, emit, asyncRoute, operations, config } = input; + registerReviewRoutes({ app, store, config, asyncRoute }); app.get( "/api/workspaces/:workspaceId/deployed-apps", 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/review-routes.test.ts b/apps/daemon/src/review-routes.test.ts new file mode 100644 index 00000000..a4f37e79 --- /dev/null +++ b/apps/daemon/src/review-routes.test.ts @@ -0,0 +1,334 @@ +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("re-DELETE on a soft-deleted comment: stale token → 409, fresh token (post-tombstone) → 204", async () => { + const fixture = createFixture(); + const { workspaceId } = seedRepoAndWorkspace(fixture); + const { server } = createDaemonApp(fixture); + const baseUrl = await listen(server); + try { + const created = await postJson<{ comment: { id: string; updatedAt: string } }>( + `${baseUrl}/api/workspaces/${workspaceId}/review-comments`, + { body: "comment to delete twice" }, + ); + // First DELETE with the original token → 204. + const first = await fetch(`${baseUrl}/api/review-comments/${created.comment.id}`, { + method: "DELETE", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ ifUpdatedAtMatches: created.comment.updatedAt }), + }); + expect(first.status).toBe(204); + + // Re-DELETE with the same (now-stale) token → 409 with the tombstone. + const staleAgain = await fetch(`${baseUrl}/api/review-comments/${created.comment.id}`, { + method: "DELETE", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ ifUpdatedAtMatches: created.comment.updatedAt }), + }); + expect(staleAgain.status).toBe(409); + const conflictBody = (await staleAgain.json()) as { latest: { updatedAt: string } }; + const postTombstoneUpdatedAt = conflictBody.latest.updatedAt; + expect(postTombstoneUpdatedAt).not.toBe(created.comment.updatedAt); + + // Re-DELETE with the post-tombstone token → idempotent 204. + const second = await fetch(`${baseUrl}/api/review-comments/${created.comment.id}`, { + method: "DELETE", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ ifUpdatedAtMatches: postTombstoneUpdatedAt }), + }); + expect(second.status).toBe(204); + } finally { + await closeServer(server); + } + }); + + 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..7695e767 --- /dev/null +++ b/apps/daemon/src/review-routes.ts @@ -0,0 +1,223 @@ +import type { CitadelConfig } from "@citadel/config"; +import type { HookOutput, 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 { z } from "zod"; +import { readWorkspaceDiffSummary } from "./workspace-diff.js"; + +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) return res.status(404).json({ error: "comment_not_found" }); + // Soft-deleted: surface as 409 with the tombstone so the client can + // verify the token it tried to use is now stale. + if (existing.deletedAt) { + if (existing.updatedAt === parsed.data.ifUpdatedAtMatches) return res.status(204).end(); + return res.status(409).json({ error: "conflict", latest: existing }); + } + 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 = readWorkspaceDiffSummary(workspace.id, workspace.path); + 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 }); + }), + ); +} 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/daemon/src/status-monitor-wiring.ts b/apps/daemon/src/status-monitor-wiring.ts index da8bdf36..d2389d8d 100644 --- a/apps/daemon/src/status-monitor-wiring.ts +++ b/apps/daemon/src/status-monitor-wiring.ts @@ -103,8 +103,7 @@ export function buildStatusMonitorDeps( // tmux session name (e.g., daemon restart re-spawned the session before // /tmp was cleared). Treat the exit signal as absent so the live agent // doesn't get marked stopped. - const liveNewerThanExit = - liveStat !== null && exitStat !== null && liveStat.mtimeMs > exitStat.mtimeMs; + const liveNewerThanExit = liveStat !== null && exitStat !== null && liveStat.mtimeMs > exitStat.mtimeMs; const exitCode = exitStat && !liveNewerThanExit ? readAgentExitCode(name) : null; return { live: liveStat !== null, diff --git a/apps/daemon/src/terminal-routes.ts b/apps/daemon/src/terminal-routes.ts index e291bf72..2157a0f1 100644 --- a/apps/daemon/src/terminal-routes.ts +++ b/apps/daemon/src/terminal-routes.ts @@ -266,7 +266,10 @@ export function registerTerminalRoutes(input: { return; } } - res.status(502).type("text/plain").send(error instanceof Error ? error.message : "terminal_proxy_failed"); + res + .status(502) + .type("text/plain") + .send(error instanceof Error ? error.message : "terminal_proxy_failed"); }); }); diff --git a/apps/daemon/src/workspace-diff.ts b/apps/daemon/src/workspace-diff.ts index 87930b06..f3575b9b 100644 --- a/apps/daemon/src/workspace-diff.ts +++ b/apps/daemon/src/workspace-diff.ts @@ -12,6 +12,28 @@ import type { const MAX_DIFF_BYTES = 128 * 1024; const MAX_DIFF_FILES = 80; +export type WorkspaceDiffSummary = { + files: string[]; + addedLines: number; + deletedLines: number; + truncated: boolean; +}; + +export function readWorkspaceDiffSummary(workspaceId: string, cwd: string): WorkspaceDiffSummary { + try { + const diff = readWorkspaceDiff(workspaceId, cwd); + return { + files: diff.files.map((f) => f.path), + addedLines: diff.addedLines, + deletedLines: diff.deletedLines, + truncated: diff.truncated, + }; + } catch (error) { + // Mark truncated so hooks can distinguish "no changes" from "fetch failed". + return { files: [], addedLines: 0, deletedLines: 0, truncated: true }; + } +} + export function readWorkspaceDiff(workspaceId: string, cwd: string): WorkspaceDiff { const status = execGit(cwd, ["status", "--porcelain=v1", "-z"]); const paths = parseStatus(status); diff --git a/apps/web/src/inspector-review.tsx b/apps/web/src/inspector-review.tsx new file mode 100644 index 00000000..f1e5b69e --- /dev/null +++ b/apps/web/src/inspector-review.tsx @@ -0,0 +1,264 @@ +import type { + ReviewComment, + ReviewSuggestion, + ReviewSuggestionRun, + Workspace, + WorkspaceDiff, +} from "@citadel/contracts"; +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { useState } from "react"; +import { ApiError, api } from "./api.js"; + +export type ReviewTabProps = { + workspace: Workspace; + diff: WorkspaceDiff | undefined; + hasRequestReviewHook: boolean; + // True when the workspace has a PR open OR is on a non-default branch. + // AC2 limits Request-review button visibility to those contexts; comments + // are always available since file/line scoping doesn't depend on the PR. + hasReviewableContext: boolean; +}; + +export function ReviewTab(props: ReviewTabProps) { + return ( +
+ {props.hasReviewableContext ? ( + + ) : null} + +
+ ); +} + +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

+
+
+