feat(review): citadel-native review system (request-review hook + comments + MCP)

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,

apps/daemon/src/app-helpers.ts

@@ -28,3 +28,23 @@ export async function cachedProviderValue<T>(
   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,
+  };
+}

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,

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<string, unknown>)[key])) {
+          allowed[key] = ((patch as Record<string, unknown>)[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 {

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<string>((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<void>((resolve, reject) => {
+    server.close((error) => (error ? reject(error) : resolve()));
+  });
+}
+
+async function mcpCall<T>(baseUrl: string, body: { name: string; arguments: Record<string, unknown> }) {
+  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 };
+}