diff --git a/.claude/rules/63-widening-a-table-can-delete-an-auth-check.md b/.claude/rules/63-widening-a-table-can-delete-an-auth-check.md new file mode 100644 index 0000000000..efb386b7de --- /dev/null +++ b/.claude/rules/63-widening-a-table-can-delete-an-auth-check.md @@ -0,0 +1,116 @@ +# Making a Scoping Column Nullable Silently Deletes Every Check That Used It + +## When This Applies + +Any schema change that relaxes a column from `NOT NULL` to nullable, or widens a +`CHECK` constraint, where that column is **also used as a scoping predicate** — +`WHERE session_id = ?`, `WHERE project_id = ?`, `WHERE user_id = ?`, +`WHERE file_id = ?`. In this codebase that means D1 migrations, Durable Object +SQLite migrations, and the query helpers layered over them. + +It applies with equal force when the motivation is benign: "this table now serves +a second kind of row, and the second kind has no session". + +## Why This Rule Exists + +Library file commenting (idea `01M0N1250YESBW2R497KXDZVSC`) needed comments +anchored to a file rather than a chat message. The plan reused the existing +`comment_threads` table and widened it: `anchor_kind` gained `'library_file'`, +and `session_id` / `message_id` became nullable because a file comment has +neither. + +That is where the authorization check died. `getCommentThread(sql, sessionId, +threadId)` had scoped its lookup `WHERE id = ? AND session_id = ?`, and its +`UPDATE`s carried the same predicate. With `session_id` nullable, that signature +no longer type-checked for file threads — so the parameter was removed. The +resulting `getCommentThread(sql, threadId)` compiled cleanly, every existing test +passed, and **reply / resolve / reopen stopped enforcing session ownership**: any +project collaborator could mutate any other session's message threads by id. + +Nothing was deleted. No type broke. No test went red. An `AND session_id = ?` +quietly became unnecessary and then absent. + +The same widening had a second consequence: SQLite cannot `ALTER ... CHECK` or +drop a `NOT NULL`, so it forced a table recreation — a drop-and-restore on a +Durable Object, which has no time-travel recovery (rule 31). + +## Class of Bug + +**A schema relaxation that removes an authorization predicate as a side effect.** + +The tells: + +- A migration makes a column nullable _because a new row kind does not have it_. +- A shared function's scope parameter becomes optional (`sessionId?: string | null`) + or disappears, and the diff reads as a type fix rather than a security change. +- A `WHERE` clause loses a conjunct, or an `UPDATE ... WHERE id = ?` no longer + carries the tenant/scope column. +- The new row kind and the old one now share a table, an index, and a getter. + +It is the schema-level sibling of rule 51 (never trust a client-supplied +identifier): here the server stops _having_ the identifier to check against. + +## Hard Requirements + +1. **Prefer a separate table over a nullable scoping column.** When a new row + kind does not have the column that scopes the existing kind, that is strong + evidence the two are different entities. Separate tables keep every existing + predicate intact by construction, keep the migration additive, and let each + kind carry its own non-null scope. Unify the kinds at the **type** layer + (a discriminated union) rather than in storage. + +2. **If you widen anyway, enumerate every query that reads the column** — + `WHERE`, `UPDATE`, `DELETE`, unique indexes and constraints — and state, per + query, whether it is an authorization predicate. List them in the PR. This is + the schema analogue of rule 44's "enumerate every writer". + +3. **A scope parameter may never be deleted in the same change that makes its + column nullable.** If a signature must change, the new kind gets its own + entry point with its own non-null scope (`getFileCommentThread(sql, fileId, +threadId)`). Do not make the shared one accept `null`. + +4. **Every entry point keeps a non-null scope in its predicate.** Whatever scopes + a row — `session_id`, `file_id`, `project_id` — belongs in the `WHERE` of every + read and every mutate for that row, not just the read. + +## Required Tests + +- **Cross-scope attack, per mutating entry point.** A real id from scope A, + addressed through scope B. Assert rejection AND that nothing mutated (version + unchanged, no rows written). +- **An owner-path control beside every attack case** (rule 28). "Nothing was torn + down" is also satisfied by the endpoint being broken outright. +- **Proven discriminating.** Delete the scope conjunct from the predicate; + exactly the attack tests must go red while the owner controls stay green. + Verify this once, then restore. +- **A separation assertion** when the fix is separate tables: each getter must + return `null` for the other kind's id, and the row counts must confirm the two + live in physically distinct tables. +- **At production RPC fidelity.** If the guard sits behind a Durable Object hop, + the test's error path must reproduce what actually crosses it — a plain `Error` + with only `name` and `message`. An `instanceof`- or `code`-based mapping passes + a richer simulation and 500s in production. + +## Quick Compliance Check + +Before merging a migration that relaxes a constraint: + +- [ ] The new row kind genuinely belongs in this table, and separate tables were + considered and rejected in writing +- [ ] Every query reading the relaxed column is enumerated in the PR, each marked + authorization-predicate or not +- [ ] No scope parameter was removed or made optional in this change +- [ ] Every read and mutate still carries a non-null scope predicate +- [ ] Cross-scope attack tests exist per entry point, each with an owner control +- [ ] The pair was verified discriminating by deleting the predicate + +## References + +- Task: `tasks/active/2026-08-22-library-file-commenting.md` (moves to + `tasks/archive/` on completion) +- Implementation: `apps/api/src/durable-objects/project-data/library-file-comments.ts`, + DO migration `033-library-file-comment-threads` +- `.claude/rules/31-migration-safety.md` — why the recreation this forced is unrecoverable on a DO +- `.claude/rules/51-server-side-node-class-gates.md` — the server must decide from values it verified +- `.claude/rules/28-credential-resolution-fallback-tests.md` — SQL-predicate guards need a real SQL engine, and every attack case needs an owner control +- `.claude/rules/44-dual-write-migration-enumerate-writers.md` — enumerate every path before a storage change diff --git a/apps/api/src/durable-objects/migrations.ts b/apps/api/src/durable-objects/migrations.ts index 962ce177b6..171d3a2d77 100644 --- a/apps/api/src/durable-objects/migrations.ts +++ b/apps/api/src/durable-objects/migrations.ts @@ -1055,6 +1055,103 @@ export const MIGRATIONS: Migration[] = [ `); }, }, + { + name: '033-library-file-comment-threads', + run: (sql) => { + // Library file comments live in their OWN tables, entirely separate from the + // message comment tables created in migration 032. + // + // The obvious alternative — widening `comment_threads` to allow a + // `library_file` anchor — cannot be done additively: SQLite cannot change a + // CHECK constraint or remove a NOT NULL in place, so it would require recreating + // `comment_threads` and its two CASCADE children. Durable Object SQLite has no + // point-in-time recovery, so dropping a table here is unrecoverable + // (.claude/rules/31-migration-safety.md, `pnpm quality:do-migration-safety`). + // + // Separate tables also keep message-comment session isolation intact by + // construction: a file thread simply cannot be reached by a session-scoped + // query, so no message-comment code path needs to learn about nullable + // session_id. The two anchor kinds are joined at the type layer + // (`CommentAnchor` in packages/shared/src/types/comments.ts), not in storage. + // + // Phase 2 anchor kinds for other file types extend `library_file_comment_threads` + // additively (new nullable columns), never by widening the message tables. + + sql.exec(` + CREATE TABLE IF NOT EXISTS library_file_comment_threads ( + id TEXT PRIMARY KEY, + file_id TEXT NOT NULL, + anchor_kind TEXT NOT NULL DEFAULT 'library_file' CHECK (anchor_kind = 'library_file'), + quote TEXT, + body TEXT NOT NULL, + author_type TEXT NOT NULL CHECK (author_type IN ('human', 'agent')), + author_id TEXT NOT NULL, + author_name TEXT, + status TEXT NOT NULL DEFAULT 'open' CHECK (status IN ('open', 'sent', 'resolved')), + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + sequence INTEGER NOT NULL, + version INTEGER NOT NULL DEFAULT 1, + client_mutation_id TEXT, + client_mutation_fingerprint TEXT, + resolved_at INTEGER, + resolved_by_type TEXT CHECK (resolved_by_type IS NULL OR resolved_by_type IN ('human', 'agent')), + resolved_by_id TEXT, + resolved_by_name TEXT, + reopened_at INTEGER, + reopened_by_type TEXT CHECK (reopened_by_type IS NULL OR reopened_by_type IN ('human', 'agent')), + reopened_by_id TEXT, + reopened_by_name TEXT, + UNIQUE(file_id, client_mutation_id) + ) + `); + sql.exec(` + CREATE INDEX IF NOT EXISTS idx_library_file_comment_threads_file_sequence + ON library_file_comment_threads(file_id, sequence) + `); + sql.exec(` + CREATE INDEX IF NOT EXISTS idx_library_file_comment_threads_status + ON library_file_comment_threads(file_id, status, sequence) + `); + + sql.exec(` + CREATE TABLE IF NOT EXISTS library_file_comment_replies ( + id TEXT PRIMARY KEY, + thread_id TEXT NOT NULL REFERENCES library_file_comment_threads(id) ON DELETE CASCADE, + file_id TEXT NOT NULL, + body TEXT NOT NULL, + author_type TEXT NOT NULL CHECK (author_type IN ('human', 'agent')), + author_id TEXT NOT NULL, + author_name TEXT, + created_at INTEGER NOT NULL, + sequence INTEGER NOT NULL, + client_mutation_id TEXT, + client_mutation_fingerprint TEXT, + UNIQUE(thread_id, client_mutation_id) + ) + `); + sql.exec(` + CREATE INDEX IF NOT EXISTS idx_library_file_comment_replies_thread_sequence + ON library_file_comment_replies(thread_id, sequence) + `); + + sql.exec(` + CREATE TABLE IF NOT EXISTS library_file_comment_status_mutations ( + thread_id TEXT NOT NULL REFERENCES library_file_comment_threads(id) ON DELETE CASCADE, + file_id TEXT NOT NULL, + client_mutation_id TEXT NOT NULL, + target_status TEXT NOT NULL CHECK (target_status IN ('open', 'sent', 'resolved')), + thread_version INTEGER NOT NULL, + created_at INTEGER NOT NULL, + PRIMARY KEY (thread_id, client_mutation_id) + ) + `); + sql.exec(` + CREATE INDEX IF NOT EXISTS idx_library_file_comment_status_mutations_file + ON library_file_comment_status_mutations(file_id, created_at) + `); + }, + }, ]; /** diff --git a/apps/api/src/durable-objects/project-data/comment-contracts.ts b/apps/api/src/durable-objects/project-data/comment-contracts.ts index ea073acda0..74432fb31e 100644 --- a/apps/api/src/durable-objects/project-data/comment-contracts.ts +++ b/apps/api/src/durable-objects/project-data/comment-contracts.ts @@ -1,6 +1,8 @@ import type { CommentAuthor, + CommentReply, CommentStatus, + LibraryFileCommentThread, MessageCommentReply, MessageCommentThread, } from '@simple-agent-manager/shared'; @@ -55,6 +57,62 @@ export type ListCommentThreadsResult = { hasMore: boolean; }; +// --------------------------------------------------------------------------- +// Library-file-anchored comments +// +// File comments are stored in their own tables (DO migration 033) and are +// project+file scoped rather than session scoped. Keeping the inputs separate +// from the message-comment inputs above means no message-comment code path ever +// has to treat `sessionId` as optional — which is what removed session isolation +// in the first cut of this feature. +// --------------------------------------------------------------------------- + +export type CreateFileCommentThreadInput = { + fileId: string; + body: string; + quote?: string | null; + clientMutationId?: string | null; + actor: CommentActor; +}; + +export type CreateFileCommentReplyInput = { + fileId: string; + threadId: string; + body: string; + clientMutationId?: string | null; + actor: CommentActor; +}; + +export type ListFileCommentThreadsInput = { + fileId: string; + status?: CommentStatus | null; + afterSequence?: number | null; + limit?: number | null; +}; + +export type UpdateFileCommentStatusInput = { + fileId: string; + threadId: string; + status: CommentStatus; + clientMutationId?: string | null; + actor: CommentActor; +}; + +export type FileCommentThreadMutationResult = { + thread: LibraryFileCommentThread; + idempotent: boolean; + changed: boolean; +}; + +export type FileCommentReplyMutationResult = FileCommentThreadMutationResult & { + reply: CommentReply; +}; + +export type ListFileCommentThreadsResult = { + threads: LibraryFileCommentThread[]; + hasMore: boolean; +}; + export const COMMENT_NOT_FOUND = 'COMMENT_NOT_FOUND'; export const COMMENT_VALIDATION = 'COMMENT_VALIDATION'; export const COMMENT_IDEMPOTENCY_CONFLICT = 'COMMENT_IDEMPOTENCY_CONFLICT'; @@ -63,7 +121,9 @@ export const COMMENT_LIMIT_EXCEEDED = 'COMMENT_LIMIT_EXCEEDED'; export class CommentNotFoundError extends Error { readonly code = COMMENT_NOT_FOUND; - constructor(readonly resource: 'Chat session' | 'Message' | 'Comment thread') { + constructor( + readonly resource: 'Chat session' | 'Message' | 'Comment thread' | 'Library file' + ) { super(`${resource} not found`); this.name = 'CommentNotFoundError'; } diff --git a/apps/api/src/durable-objects/project-data/comment-normalization.ts b/apps/api/src/durable-objects/project-data/comment-normalization.ts new file mode 100644 index 0000000000..67f1b606b7 --- /dev/null +++ b/apps/api/src/durable-objects/project-data/comment-normalization.ts @@ -0,0 +1,131 @@ +/** + * Anchor-agnostic comment validation, limits, and column mapping. + * + * Shared by the message-anchored implementation (`comments.ts`) and the + * library-file-anchored implementation (`library-file-comments.ts`) so the two + * storage backends cannot drift on body/quote length limits, idempotency-key + * handling, or actor normalization. + */ + +import { + DEFAULT_COMMENT_BODY_MAX_LENGTH, + DEFAULT_COMMENT_IDEMPOTENCY_KEY_MAX_LENGTH, + DEFAULT_COMMENT_LIST_LIMIT_DEFAULT, + DEFAULT_COMMENT_LIST_LIMIT_MAX, + DEFAULT_COMMENT_QUOTE_MAX_LENGTH, + DEFAULT_COMMENT_REPLIES_PER_THREAD_MAX, + DEFAULT_COMMENT_THREADS_PER_SESSION_MAX, +} from '@simple-agent-manager/shared'; + +import { type CommentActor, CommentValidationError } from './comment-contracts'; +import type { Env } from './types'; + +export type CommentLimits = { + bodyMaxLength: number; + quoteMaxLength: number; + idempotencyKeyMaxLength: number; + listDefaultLimit: number; + listMaxLimit: number; + threadsPerSessionMax: number; + repliesPerThreadMax: number; +}; + +function positiveInteger(value: string | undefined, fallback: number): number { + const parsed = Number.parseInt(value || '', 10); + return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; +} + +export function resolveCommentLimits(env: Env): CommentLimits { + const listDefaultLimit = positiveInteger( + env.COMMENT_LIST_LIMIT_DEFAULT, + DEFAULT_COMMENT_LIST_LIMIT_DEFAULT + ); + const listMaxLimit = positiveInteger(env.COMMENT_LIST_LIMIT_MAX, DEFAULT_COMMENT_LIST_LIMIT_MAX); + return { + bodyMaxLength: positiveInteger(env.COMMENT_BODY_MAX_LENGTH, DEFAULT_COMMENT_BODY_MAX_LENGTH), + quoteMaxLength: positiveInteger(env.COMMENT_QUOTE_MAX_LENGTH, DEFAULT_COMMENT_QUOTE_MAX_LENGTH), + idempotencyKeyMaxLength: positiveInteger( + env.COMMENT_IDEMPOTENCY_KEY_MAX_LENGTH, + DEFAULT_COMMENT_IDEMPOTENCY_KEY_MAX_LENGTH + ), + listDefaultLimit, + listMaxLimit: Math.max(listDefaultLimit, listMaxLimit), + threadsPerSessionMax: positiveInteger( + env.COMMENT_THREADS_PER_SESSION_MAX, + DEFAULT_COMMENT_THREADS_PER_SESSION_MAX + ), + repliesPerThreadMax: positiveInteger( + env.COMMENT_REPLIES_PER_THREAD_MAX, + DEFAULT_COMMENT_REPLIES_PER_THREAD_MAX + ), + }; +} + +export function resolveCommentListLimit(env: Env, requested?: number | null): number { + const limits = resolveCommentLimits(env); + const limit = + typeof requested === 'number' && Number.isFinite(requested) && requested > 0 + ? Math.floor(requested) + : limits.listDefaultLimit; + return Math.min(limit, limits.listMaxLimit); +} + +export function normalizeBody(body: string, limits: CommentLimits): string { + const normalized = body.trim(); + if (!normalized) throw new CommentValidationError('body is required'); + if (normalized.length > limits.bodyMaxLength) { + throw new CommentValidationError(`body must be ${limits.bodyMaxLength} characters or fewer`); + } + return normalized; +} + +export function normalizeQuote( + quote: string | null | undefined, + limits: CommentLimits +): string | null { + if (quote === null || quote === undefined) return null; + const normalized = quote.trim(); + if (!normalized) return null; + if (normalized.length > limits.quoteMaxLength) { + throw new CommentValidationError(`quote must be ${limits.quoteMaxLength} characters or fewer`); + } + return normalized; +} + +export function normalizeClientMutationId( + clientMutationId: string | null | undefined, + limits: CommentLimits +): string | null { + if (clientMutationId === null || clientMutationId === undefined) return null; + const normalized = clientMutationId.trim(); + if (!normalized) return null; + if (normalized.length > limits.idempotencyKeyMaxLength) { + throw new CommentValidationError( + `clientMutationId must be ${limits.idempotencyKeyMaxLength} characters or fewer` + ); + } + return normalized; +} + +export function normalizeActor(actor: CommentActor): CommentActor { + if (actor.kind !== 'human' && actor.kind !== 'agent') { + throw new CommentValidationError('actor kind must be human or agent'); + } + const id = actor.id.trim(); + if (!id) throw new CommentValidationError('actor id is required'); + const name = actor.name?.trim() || null; + return { kind: actor.kind, id, name }; +} + +export function fingerprint(value: unknown): string { + return JSON.stringify(value); +} + +export function actorFromColumns( + kind: 'human' | 'agent' | null, + id: string | null, + name: string | null +): CommentActor | null { + if (!kind || !id) return null; + return { kind, id, name }; +} diff --git a/apps/api/src/durable-objects/project-data/comments.ts b/apps/api/src/durable-objects/project-data/comments.ts index 44ca1f47e5..9ab5cb978f 100644 --- a/apps/api/src/durable-objects/project-data/comments.ts +++ b/apps/api/src/durable-objects/project-data/comments.ts @@ -1,14 +1,5 @@ import type { MessageCommentReply, MessageCommentThread } from '@simple-agent-manager/shared'; -import { - COMMENT_STATUSES, - DEFAULT_COMMENT_BODY_MAX_LENGTH, - DEFAULT_COMMENT_IDEMPOTENCY_KEY_MAX_LENGTH, - DEFAULT_COMMENT_LIST_LIMIT_DEFAULT, - DEFAULT_COMMENT_LIST_LIMIT_MAX, - DEFAULT_COMMENT_QUOTE_MAX_LENGTH, - DEFAULT_COMMENT_REPLIES_PER_THREAD_MAX, - DEFAULT_COMMENT_THREADS_PER_SESSION_MAX, -} from '@simple-agent-manager/shared'; +import { COMMENT_STATUSES } from '@simple-agent-manager/shared'; import * as v from 'valibot'; import { createModuleLogger } from '../../lib/logger'; @@ -30,6 +21,16 @@ import { type ListCommentThreadsResult, type UpdateCommentStatusInput, } from './comment-contracts'; +import { + actorFromColumns, + fingerprint, + normalizeActor, + normalizeBody, + normalizeClientMutationId, + normalizeQuote, + resolveCommentLimits, + resolveCommentListLimit, +} from './comment-normalization'; import { parseRow } from './row-schemas'; import type { Env } from './types'; import { generateId } from './types'; @@ -46,6 +47,7 @@ export { CommentNotFoundError, CommentValidationError, }; +export { resolveCommentLimits, resolveCommentListLimit }; export type { CommentActor, CommentReplyMutationResult, @@ -57,16 +59,6 @@ export type { UpdateCommentStatusInput, }; -type CommentLimits = { - bodyMaxLength: number; - quoteMaxLength: number; - idempotencyKeyMaxLength: number; - listDefaultLimit: number; - listMaxLimit: number; - threadsPerSessionMax: number; - repliesPerThreadMax: number; -}; - const ThreadRowSchema = v.object({ id: v.string(), session_id: v.string(), @@ -111,94 +103,6 @@ const ReplyRowSchema = v.object({ type ThreadRow = v.InferOutput; -function positiveInteger(value: string | undefined, fallback: number): number { - const parsed = Number.parseInt(value || '', 10); - return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; -} - -export function resolveCommentLimits(env: Env): CommentLimits { - const listDefaultLimit = positiveInteger( - env.COMMENT_LIST_LIMIT_DEFAULT, - DEFAULT_COMMENT_LIST_LIMIT_DEFAULT - ); - const listMaxLimit = positiveInteger(env.COMMENT_LIST_LIMIT_MAX, DEFAULT_COMMENT_LIST_LIMIT_MAX); - return { - bodyMaxLength: positiveInteger(env.COMMENT_BODY_MAX_LENGTH, DEFAULT_COMMENT_BODY_MAX_LENGTH), - quoteMaxLength: positiveInteger(env.COMMENT_QUOTE_MAX_LENGTH, DEFAULT_COMMENT_QUOTE_MAX_LENGTH), - idempotencyKeyMaxLength: positiveInteger( - env.COMMENT_IDEMPOTENCY_KEY_MAX_LENGTH, - DEFAULT_COMMENT_IDEMPOTENCY_KEY_MAX_LENGTH - ), - listDefaultLimit, - listMaxLimit: Math.max(listDefaultLimit, listMaxLimit), - threadsPerSessionMax: positiveInteger( - env.COMMENT_THREADS_PER_SESSION_MAX, - DEFAULT_COMMENT_THREADS_PER_SESSION_MAX - ), - repliesPerThreadMax: positiveInteger( - env.COMMENT_REPLIES_PER_THREAD_MAX, - DEFAULT_COMMENT_REPLIES_PER_THREAD_MAX - ), - }; -} - -export function resolveCommentListLimit(env: Env, requested?: number | null): number { - const limits = resolveCommentLimits(env); - const limit = - typeof requested === 'number' && Number.isFinite(requested) && requested > 0 - ? Math.floor(requested) - : limits.listDefaultLimit; - return Math.min(limit, limits.listMaxLimit); -} - -function normalizeBody(body: string, limits: CommentLimits): string { - const normalized = body.trim(); - if (!normalized) throw new CommentValidationError('body is required'); - if (normalized.length > limits.bodyMaxLength) { - throw new CommentValidationError(`body must be ${limits.bodyMaxLength} characters or fewer`); - } - return normalized; -} - -function normalizeQuote(quote: string | null | undefined, limits: CommentLimits): string | null { - if (quote === null || quote === undefined) return null; - const normalized = quote.trim(); - if (!normalized) return null; - if (normalized.length > limits.quoteMaxLength) { - throw new CommentValidationError(`quote must be ${limits.quoteMaxLength} characters or fewer`); - } - return normalized; -} - -function normalizeClientMutationId( - clientMutationId: string | null | undefined, - limits: CommentLimits -): string | null { - if (clientMutationId === null || clientMutationId === undefined) return null; - const normalized = clientMutationId.trim(); - if (!normalized) return null; - if (normalized.length > limits.idempotencyKeyMaxLength) { - throw new CommentValidationError( - `clientMutationId must be ${limits.idempotencyKeyMaxLength} characters or fewer` - ); - } - return normalized; -} - -function normalizeActor(actor: CommentActor): CommentActor { - if (actor.kind !== 'human' && actor.kind !== 'agent') { - throw new CommentValidationError('actor kind must be human or agent'); - } - const id = actor.id.trim(); - if (!id) throw new CommentValidationError('actor id is required'); - const name = actor.name?.trim() || null; - return { kind: actor.kind, id, name }; -} - -function fingerprint(value: unknown): string { - return JSON.stringify(value); -} - function ensureSession(sql: SqlStorage, sessionId: string): void { const row = sql.exec('SELECT id FROM chat_sessions WHERE id = ? LIMIT 1', sessionId).toArray()[0]; if (!row) throw new CommentNotFoundError('Chat session'); @@ -235,15 +139,6 @@ function nextReplySequence(sql: SqlStorage, threadId: string): number { return (typeof row?.max_sequence === 'number' ? row.max_sequence : 0) + 1; } -function actorFromColumns( - kind: 'human' | 'agent' | null, - id: string | null, - name: string | null -): CommentActor | null { - if (!kind || !id) return null; - return { kind, id, name }; -} - function mapReply(row: unknown): MessageCommentReply { const r = parseRow(ReplyRowSchema, row, 'comment_reply'); return { diff --git a/apps/api/src/durable-objects/project-data/index.ts b/apps/api/src/durable-objects/project-data/index.ts index 265df34292..4d1ef52639 100644 --- a/apps/api/src/durable-objects/project-data/index.ts +++ b/apps/api/src/durable-objects/project-data/index.ts @@ -36,6 +36,7 @@ import * as durability from './durability-foundation'; import * as ideas from './ideas'; import * as idleCleanup from './idle-cleanup'; import * as knowledge from './knowledge'; +import * as libraryFileComments from './library-file-comments'; import * as mailbox from './mailbox'; import * as materialization from './materialization'; import * as messagePersistence from './message-persistence'; @@ -521,6 +522,41 @@ export class ProjectData extends DurableObject { return { thread: result.thread, idempotent: result.idempotent }; } + // --- Library file comments ------------------------------------------------ + // Separate storage from message comments (DO migration 033). Callers must have + // already verified the file belongs to this project — the DO has no D1 access. + + listFileCommentThreads( + input: libraryFileComments.ListFileCommentThreadsInput + ): libraryFileComments.ListFileCommentThreadsResult { + return libraryFileComments.listFileCommentThreads(this.sql, this.env, input); + } + + createFileCommentThread(input: libraryFileComments.CreateFileCommentThreadInput) { + const result = this.ctx.storage.transactionSync(() => + libraryFileComments.createFileCommentThread(this.sql, this.env, input) + ); + return { thread: result.thread, idempotent: result.idempotent }; + } + + createFileCommentReply(input: libraryFileComments.CreateFileCommentReplyInput) { + const result = this.ctx.storage.transactionSync(() => + libraryFileComments.createFileCommentReply(this.sql, this.env, input) + ); + return { + thread: result.thread, + reply: result.reply, + idempotent: result.idempotent, + }; + } + + updateFileCommentThreadStatus(input: libraryFileComments.UpdateFileCommentStatusInput) { + const result = this.ctx.storage.transactionSync(() => + libraryFileComments.updateFileCommentThreadStatus(this.sql, this.env, input) + ); + return { thread: result.thread, idempotent: result.idempotent }; + } + materializeSession(sessionId: string): void { materialization.materializeSession(this.sql, sessionId); } diff --git a/apps/api/src/durable-objects/project-data/library-file-comments.ts b/apps/api/src/durable-objects/project-data/library-file-comments.ts new file mode 100644 index 0000000000..76ed96c0de --- /dev/null +++ b/apps/api/src/durable-objects/project-data/library-file-comments.ts @@ -0,0 +1,553 @@ +/** + * Library-file-anchored comment threads. + * + * Storage is entirely separate from message comments (DO migration 033): + * `library_file_comment_threads` / `_replies` / `_status_mutations`. File + * comments are project+file scoped, not session scoped, so nothing here reads or + * writes `chat_sessions`, and no message-comment query can reach a file thread. + * + * Validation, limits, and actor mapping are shared with the message + * implementation via `comment-normalization.ts` so the two cannot drift. + * + * File existence is NOT verified here — `project_files` lives in D1 and the DO + * has no D1 access. Callers must verify the file belongs to the project before + * calling in (see `assertLibraryFileInProject` in services/library-file-comments.ts). + */ + +import type { CommentReply, LibraryFileCommentThread } from '@simple-agent-manager/shared'; +import { COMMENT_STATUSES } from '@simple-agent-manager/shared'; +import * as v from 'valibot'; + +import { createModuleLogger } from '../../lib/logger'; +import { + CommentIdempotencyConflictError, + CommentLimitExceededError, + CommentNotFoundError, + CommentValidationError, + type CreateFileCommentReplyInput, + type CreateFileCommentThreadInput, + type FileCommentReplyMutationResult, + type FileCommentThreadMutationResult, + type ListFileCommentThreadsInput, + type ListFileCommentThreadsResult, + type UpdateFileCommentStatusInput, +} from './comment-contracts'; +import { + actorFromColumns, + fingerprint, + normalizeActor, + normalizeBody, + normalizeClientMutationId, + normalizeQuote, + resolveCommentLimits, + resolveCommentListLimit, +} from './comment-normalization'; +import { parseRow } from './row-schemas'; +import type { Env } from './types'; +import { generateId } from './types'; + +export type { + CreateFileCommentReplyInput, + CreateFileCommentThreadInput, + FileCommentReplyMutationResult, + FileCommentThreadMutationResult, + ListFileCommentThreadsInput, + ListFileCommentThreadsResult, + UpdateFileCommentStatusInput, +}; + +const log = createModuleLogger('project_data.library_file_comments'); + +const FileThreadRowSchema = v.object({ + id: v.string(), + file_id: v.string(), + quote: v.nullable(v.string()), + body: v.string(), + author_type: v.picklist(['human', 'agent']), + author_id: v.string(), + author_name: v.nullable(v.string()), + status: v.picklist(COMMENT_STATUSES), + created_at: v.number(), + updated_at: v.number(), + sequence: v.number(), + version: v.number(), + client_mutation_id: v.nullable(v.string()), + resolved_at: v.nullable(v.number()), + resolved_by_type: v.nullable(v.picklist(['human', 'agent'])), + resolved_by_id: v.nullable(v.string()), + resolved_by_name: v.nullable(v.string()), + reopened_at: v.nullable(v.number()), + reopened_by_type: v.nullable(v.picklist(['human', 'agent'])), + reopened_by_id: v.nullable(v.string()), + reopened_by_name: v.nullable(v.string()), +}); + +const FileReplyRowSchema = v.object({ + id: v.string(), + thread_id: v.string(), + file_id: v.string(), + body: v.string(), + author_type: v.picklist(['human', 'agent']), + author_id: v.string(), + author_name: v.nullable(v.string()), + created_at: v.number(), + sequence: v.number(), + client_mutation_id: v.nullable(v.string()), +}); + +type FileThreadRow = v.InferOutput; + +function normalizeFileId(fileId: string): string { + const normalized = fileId.trim(); + if (!normalized) throw new CommentValidationError('fileId is required'); + return normalized; +} + +function nextThreadSequence(sql: SqlStorage, fileId: string): number { + const row = sql + .exec( + 'SELECT COALESCE(MAX(sequence), 0) AS max_sequence FROM library_file_comment_threads WHERE file_id = ?', + fileId + ) + .toArray()[0]; + return (typeof row?.max_sequence === 'number' ? row.max_sequence : 0) + 1; +} + +function nextReplySequence(sql: SqlStorage, threadId: string): number { + const row = sql + .exec( + 'SELECT COALESCE(MAX(sequence), 0) AS max_sequence FROM library_file_comment_replies WHERE thread_id = ?', + threadId + ) + .toArray()[0]; + return (typeof row?.max_sequence === 'number' ? row.max_sequence : 0) + 1; +} + +function mapReply(row: unknown): CommentReply { + const r = parseRow(FileReplyRowSchema, row, 'library_file_comment_reply'); + return { + id: r.id, + threadId: r.thread_id, + author: { kind: r.author_type, id: r.author_id, name: r.author_name }, + body: r.body, + createdAt: r.created_at, + sequence: r.sequence, + clientMutationId: r.client_mutation_id, + }; +} + +function mapThread(r: FileThreadRow, replies: CommentReply[]): LibraryFileCommentThread { + return { + id: r.id, + fileId: r.file_id, + anchor: { kind: 'library_file', fileId: r.file_id, quote: r.quote }, + author: { kind: r.author_type, id: r.author_id, name: r.author_name }, + body: r.body, + status: r.status, + createdAt: r.created_at, + updatedAt: r.updated_at, + sequence: r.sequence, + version: r.version, + clientMutationId: r.client_mutation_id, + resolvedAt: r.resolved_at, + resolvedBy: actorFromColumns(r.resolved_by_type, r.resolved_by_id, r.resolved_by_name), + reopenedAt: r.reopened_at, + reopenedBy: actorFromColumns(r.reopened_by_type, r.reopened_by_id, r.reopened_by_name), + replies, + }; +} + +/** + * Per-row fault isolation: one malformed legacy row must not fail the whole + * list read. See .claude/rules/50-list-read-row-fault-isolation.md. + */ +function readReplies(sql: SqlStorage, threadIds: string[]): Map { + const byThread = new Map(); + for (const threadId of threadIds) byThread.set(threadId, []); + if (threadIds.length === 0) return byThread; + + const placeholders = threadIds.map(() => '?').join(', '); + const rows = sql + .exec( + `SELECT id, thread_id, file_id, body, author_type, author_id, author_name, + created_at, sequence, client_mutation_id + FROM library_file_comment_replies + WHERE thread_id IN (${placeholders}) + ORDER BY thread_id ASC, sequence ASC`, + ...threadIds + ) + .toArray(); + + for (const row of rows) { + try { + const reply = mapReply(row); + if (reply.threadId) byThread.get(reply.threadId)?.push(reply); + } catch (err) { + log.warn('library_file_comments.reply_row_skipped', { + rowId: typeof row.id === 'string' ? row.id : null, + threadId: typeof row.thread_id === 'string' ? row.thread_id : null, + error: String(err), + }); + } + } + return byThread; +} + +function parseThreadRows(rows: unknown[]): FileThreadRow[] { + const parsed: FileThreadRow[] = []; + for (const row of rows) { + try { + parsed.push(parseRow(FileThreadRowSchema, row, 'library_file_comment_thread')); + } catch (err) { + const record = row && typeof row === 'object' ? (row as Record) : {}; + log.warn('library_file_comments.thread_row_skipped', { + rowId: typeof record.id === 'string' ? record.id : null, + error: String(err), + }); + } + } + return parsed; +} + +function hydrateThreads(sql: SqlStorage, rows: unknown[]): LibraryFileCommentThread[] { + const parsedRows = parseThreadRows(rows); + const replies = readReplies( + sql, + parsedRows.map((row) => row.id) + ); + return parsedRows.map((row) => mapThread(row, replies.get(row.id) ?? [])); +} + +/** + * Reads a thread scoped to its file. The `file_id = ?` predicate is the + * ownership guard: a thread id alone must never be enough to read or mutate a + * thread belonging to a different file. + */ +export function getFileCommentThread( + sql: SqlStorage, + fileId: string, + threadId: string +): LibraryFileCommentThread | null { + const rows = sql + .exec( + `SELECT id, file_id, quote, body, author_type, author_id, author_name, + status, created_at, updated_at, sequence, version, client_mutation_id, + resolved_at, resolved_by_type, resolved_by_id, resolved_by_name, + reopened_at, reopened_by_type, reopened_by_id, reopened_by_name + FROM library_file_comment_threads + WHERE id = ? AND file_id = ? + LIMIT 1`, + threadId, + fileId + ) + .toArray(); + const row = parseThreadRows(rows)[0]; + if (!row) return null; + const replies = readReplies(sql, [threadId]); + return mapThread(row, replies.get(threadId) ?? []); +} + +export function listFileCommentThreads( + sql: SqlStorage, + env: Env, + input: ListFileCommentThreadsInput +): ListFileCommentThreadsResult { + const fileId = normalizeFileId(input.fileId); + const limit = resolveCommentListLimit(env, input.limit); + const conditions = ['file_id = ?']; + const params: Array = [fileId]; + + if (input.status) { + conditions.push('status = ?'); + params.push(input.status); + } + if (input.afterSequence !== null && input.afterSequence !== undefined) { + conditions.push('sequence > ?'); + params.push(input.afterSequence); + } + + // Named `whereClause` deliberately: every fragment in `conditions` is a literal + // with `?` placeholders, and that identifier is what the sql-injection scanner + // recognises as a parameterized clause builder (scripts/quality/ast-checks.ts). + const whereClause = conditions.join(' AND '); + const rows = sql + .exec( + `SELECT id, file_id, quote, body, author_type, author_id, author_name, + status, created_at, updated_at, sequence, version, client_mutation_id, + resolved_at, resolved_by_type, resolved_by_id, resolved_by_name, + reopened_at, reopened_by_type, reopened_by_id, reopened_by_name + FROM library_file_comment_threads + WHERE ${whereClause} + ORDER BY sequence ASC + LIMIT ?`, + ...params, + limit + 1 + ) + .toArray(); + + const hasMore = rows.length > limit; + return { + threads: hydrateThreads(sql, hasMore ? rows.slice(0, limit) : rows), + hasMore, + }; +} + +export function createFileCommentThread( + sql: SqlStorage, + env: Env, + input: CreateFileCommentThreadInput +): FileCommentThreadMutationResult { + const limits = resolveCommentLimits(env); + const fileId = normalizeFileId(input.fileId); + const actor = normalizeActor(input.actor); + const body = normalizeBody(input.body, limits); + const quote = normalizeQuote(input.quote, limits); + const clientMutationId = normalizeClientMutationId(input.clientMutationId, limits); + const requestFingerprint = fingerprint([ + 'file_thread', + fileId, + body, + quote, + actor.kind, + actor.id, + ]); + + if (clientMutationId) { + const existing = sql + .exec( + `SELECT id, client_mutation_fingerprint + FROM library_file_comment_threads + WHERE file_id = ? AND client_mutation_id = ? + LIMIT 1`, + fileId, + clientMutationId + ) + .toArray()[0]; + if (existing) { + if (existing.client_mutation_fingerprint !== requestFingerprint) { + throw new CommentIdempotencyConflictError(); + } + const thread = getFileCommentThread(sql, fileId, String(existing.id)); + if (!thread) throw new CommentNotFoundError('Comment thread'); + return { thread, idempotent: true, changed: false }; + } + } + + const countRow = sql + .exec('SELECT COUNT(*) AS count FROM library_file_comment_threads WHERE file_id = ?', fileId) + .toArray()[0]; + if ((typeof countRow?.count === 'number' ? countRow.count : 0) >= limits.threadsPerSessionMax) { + throw new CommentLimitExceededError( + `file comment thread limit of ${limits.threadsPerSessionMax} reached` + ); + } + + const id = generateId(); + const now = Date.now(); + const sequence = nextThreadSequence(sql, fileId); + sql.exec( + `INSERT INTO library_file_comment_threads + (id, file_id, anchor_kind, quote, body, author_type, author_id, author_name, + status, created_at, updated_at, sequence, version, client_mutation_id, + client_mutation_fingerprint) + VALUES (?, ?, 'library_file', ?, ?, ?, ?, ?, 'open', ?, ?, ?, 1, ?, ?)`, + id, + fileId, + quote, + body, + actor.kind, + actor.id, + actor.name, + now, + now, + sequence, + clientMutationId, + clientMutationId ? requestFingerprint : null + ); + + const thread = getFileCommentThread(sql, fileId, id); + if (!thread) throw new CommentNotFoundError('Comment thread'); + return { thread, idempotent: false, changed: true }; +} + +export function createFileCommentReply( + sql: SqlStorage, + env: Env, + input: CreateFileCommentReplyInput +): FileCommentReplyMutationResult { + const limits = resolveCommentLimits(env); + const fileId = normalizeFileId(input.fileId); + const actor = normalizeActor(input.actor); + const body = normalizeBody(input.body, limits); + const clientMutationId = normalizeClientMutationId(input.clientMutationId, limits); + const requestFingerprint = fingerprint([ + 'file_reply', + input.threadId, + body, + actor.kind, + actor.id, + ]); + + const thread = getFileCommentThread(sql, fileId, input.threadId); + if (!thread) throw new CommentNotFoundError('Comment thread'); + + if (clientMutationId) { + const existing = sql + .exec( + `SELECT id, client_mutation_fingerprint + FROM library_file_comment_replies + WHERE thread_id = ? AND client_mutation_id = ? + LIMIT 1`, + input.threadId, + clientMutationId + ) + .toArray()[0]; + if (existing) { + if (existing.client_mutation_fingerprint !== requestFingerprint) { + throw new CommentIdempotencyConflictError(); + } + const authoritative = getFileCommentThread(sql, fileId, input.threadId); + const reply = authoritative?.replies.find((candidate) => candidate.id === existing.id); + if (!authoritative || !reply) throw new CommentNotFoundError('Comment thread'); + return { thread: authoritative, reply, idempotent: true, changed: false }; + } + } + + const countRow = sql + .exec( + 'SELECT COUNT(*) AS count FROM library_file_comment_replies WHERE thread_id = ?', + input.threadId + ) + .toArray()[0]; + if ((typeof countRow?.count === 'number' ? countRow.count : 0) >= limits.repliesPerThreadMax) { + throw new CommentLimitExceededError( + `comment reply limit of ${limits.repliesPerThreadMax} reached` + ); + } + + const id = generateId(); + const now = Date.now(); + const sequence = nextReplySequence(sql, input.threadId); + sql.exec( + `INSERT INTO library_file_comment_replies + (id, thread_id, file_id, body, author_type, author_id, author_name, + created_at, sequence, client_mutation_id, client_mutation_fingerprint) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + id, + input.threadId, + fileId, + body, + actor.kind, + actor.id, + actor.name, + now, + sequence, + clientMutationId, + clientMutationId ? requestFingerprint : null + ); + sql.exec( + `UPDATE library_file_comment_threads + SET updated_at = ?, version = version + 1 + WHERE id = ? AND file_id = ?`, + now, + input.threadId, + fileId + ); + + const authoritative = getFileCommentThread(sql, fileId, input.threadId); + const reply = authoritative?.replies.find((candidate) => candidate.id === id); + if (!authoritative || !reply) throw new CommentNotFoundError('Comment thread'); + return { thread: authoritative, reply, idempotent: false, changed: true }; +} + +export function updateFileCommentThreadStatus( + sql: SqlStorage, + env: Env, + input: UpdateFileCommentStatusInput +): FileCommentThreadMutationResult { + const limits = resolveCommentLimits(env); + const fileId = normalizeFileId(input.fileId); + const actor = normalizeActor(input.actor); + const clientMutationId = normalizeClientMutationId(input.clientMutationId, limits); + if (!COMMENT_STATUSES.includes(input.status)) { + throw new CommentValidationError('status must be open, sent, or resolved'); + } + + const current = getFileCommentThread(sql, fileId, input.threadId); + if (!current) throw new CommentNotFoundError('Comment thread'); + + if (clientMutationId) { + const existing = sql + .exec( + `SELECT target_status + FROM library_file_comment_status_mutations + WHERE thread_id = ? AND client_mutation_id = ? + LIMIT 1`, + input.threadId, + clientMutationId + ) + .toArray()[0]; + if (existing) { + if (existing.target_status !== input.status) throw new CommentIdempotencyConflictError(); + return { thread: current, idempotent: true, changed: false }; + } + } + + const now = Date.now(); + let changed = false; + if (current.status !== input.status) { + changed = true; + if (input.status === 'resolved') { + sql.exec( + `UPDATE library_file_comment_threads + SET status = 'resolved', resolved_at = ?, resolved_by_type = ?, resolved_by_id = ?, + resolved_by_name = ?, updated_at = ?, version = version + 1 + WHERE id = ? AND file_id = ?`, + now, + actor.kind, + actor.id, + actor.name, + now, + input.threadId, + fileId + ); + } else if (input.status === 'open') { + sql.exec( + `UPDATE library_file_comment_threads + SET status = 'open', reopened_at = ?, reopened_by_type = ?, reopened_by_id = ?, + reopened_by_name = ?, updated_at = ?, version = version + 1 + WHERE id = ? AND file_id = ?`, + now, + actor.kind, + actor.id, + actor.name, + now, + input.threadId, + fileId + ); + } else { + // 'sent' is a message-comment concept (send-to-agent). File comments have + // no send-to-agent path in Phase 1; reject rather than silently no-op. + throw new CommentValidationError('library file comments support open or resolved only'); + } + } + + const authoritative = getFileCommentThread(sql, fileId, input.threadId); + if (!authoritative) throw new CommentNotFoundError('Comment thread'); + + if (clientMutationId) { + sql.exec( + `INSERT INTO library_file_comment_status_mutations + (thread_id, file_id, client_mutation_id, target_status, thread_version, created_at) + VALUES (?, ?, ?, ?, ?, ?)`, + input.threadId, + fileId, + clientMutationId, + input.status, + authoritative.version, + now + ); + } + + return { thread: authoritative, idempotent: false, changed }; +} diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index b51a8d3d3f..41d74d3d5e 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -97,6 +97,7 @@ import { } from './routes/interactive-preview-host'; import { knowledgeRoutes } from './routes/knowledge'; import { libraryRoutes } from './routes/library'; +import { libraryCommentRoutes } from './routes/library-comments'; import { mailboxRoutes } from './routes/mailbox'; import { mcpRoutes } from './routes/mcp'; import { missionRoutes } from './routes/missions'; @@ -727,6 +728,7 @@ app.route('/api/projects/:projectId/sessions', chatRoutes); app.route('/api/projects/:projectId/cached-commands', cachedCommandRoutes); app.route('/api/projects/:projectId/activity', activityRoutes); app.route('/api/projects/:projectId/library', libraryRoutes); +app.route('/api/projects/:projectId/library', libraryCommentRoutes); app.route('/api/projects/:projectId/agent-profiles/:profileId/runtime', profileRuntimeRoutes); app.route('/api/projects/:projectId/agent-profiles', agentProfileRoutes); app.route('/api/projects/:projectId/skills/:skillId/runtime', skillRuntimeRoutes); diff --git a/apps/api/src/lib/comment-http.ts b/apps/api/src/lib/comment-http.ts new file mode 100644 index 0000000000..49887c4322 --- /dev/null +++ b/apps/api/src/lib/comment-http.ts @@ -0,0 +1,136 @@ +/** + * HTTP-boundary helpers shared by the message-comment and library-file-comment + * routes. + * + * These were duplicated across `routes/chat-comments.ts` and + * `routes/library-comments.ts`. The copies drifted: only the chat one handled + * errors that had crossed the Durable Object RPC boundary, where the error class + * is lost and only `name`/`message`/`code` survive — so the library routes turned + * an expected 404 into a 500. One implementation, used by both + * (.claude/rules/24-no-duplicate-ui-controls.md). + */ + +import type { CommentStatus } from '@simple-agent-manager/shared'; +import { COMMENT_STATUSES } from '@simple-agent-manager/shared'; +import type { Context } from 'hono'; + +import type { Env } from '../env'; +import { getAuth } from '../middleware/auth'; +import { errors } from '../middleware/error'; +import { + CommentIdempotencyConflictError, + CommentLimitExceededError, + CommentNotFoundError, + CommentValidationError, +} from '../services/project-data'; + +export function parseCommentStatus(rawStatus?: string): CommentStatus | null { + if (!rawStatus) return null; + const status = rawStatus.trim().toLowerCase(); + if (COMMENT_STATUSES.includes(status as CommentStatus)) return status as CommentStatus; + throw errors.badRequest('status must be open, sent, or resolved'); +} + +export function parsePositiveIntegerQuery(name: string, rawValue?: string): number | null { + if (!rawValue) return null; + const parsed = Number.parseInt(rawValue, 10); + if (!Number.isFinite(parsed) || parsed <= 0) { + throw errors.badRequest(`${name} must be a positive integer`); + } + return parsed; +} + +export function parseNonNegativeIntegerQuery(name: string, rawValue?: string): number | null { + if (!rawValue) return null; + const parsed = Number.parseInt(rawValue, 10); + if (!Number.isFinite(parsed) || parsed < 0) { + throw errors.badRequest(`${name} must be a non-negative integer`); + } + return parsed; +} + +export function getCommentActor(c: Context<{ Bindings: Env }>) { + const auth = getAuth(c); + return { + kind: 'human' as const, + id: auth.user.id, + name: auth.user.name ?? auth.user.email ?? null, + }; +} + +function getCommentErrorName(err: unknown): string | null { + return err instanceof Error ? err.name : null; +} + +function getCommentErrorCode(err: unknown): string | null { + if (!err || typeof err !== 'object') return null; + const code = (err as { code?: unknown }).code; + return typeof code === 'string' ? code : null; +} + +function getCommentNotFoundResource(err: unknown): string { + if (err && typeof err === 'object') { + const resource = (err as { resource?: unknown }).resource; + if (typeof resource === 'string') return resource; + } + const message = err instanceof Error ? err.message : ''; + if (message.startsWith('Message ')) return 'Message'; + if (message.startsWith('Comment thread ')) return 'Comment thread'; + if (message.startsWith('Chat session ')) return 'Chat session'; + if (message.startsWith('Library file ')) return 'Library file'; + return 'Resource'; +} + +/** + * A CommentNotFoundError that crossed the DO RPC boundary arrives as a plain + * Error — the class and the `code` property are both gone. Match on the exact + * messages the DO can produce. + */ +function isSerializedCommentNotFoundError(err: unknown): boolean { + if (!(err instanceof Error)) return false; + return ( + err.message === 'Chat session not found' || + err.message === 'Message not found' || + err.message === 'Comment thread not found' || + err.message === 'Library file not found' + ); +} + +export function rethrowCommentError(err: unknown): never { + const code = getCommentErrorCode(err); + const name = getCommentErrorName(err); + if ( + err instanceof CommentValidationError || + code === 'COMMENT_VALIDATION' || + name === 'CommentValidationError' + ) { + throw errors.badRequest(err instanceof Error ? err.message : 'Invalid comment request'); + } + if ( + err instanceof CommentNotFoundError || + code === 'COMMENT_NOT_FOUND' || + name === 'CommentNotFoundError' || + isSerializedCommentNotFoundError(err) + ) { + throw errors.notFound(getCommentNotFoundResource(err)); + } + if ( + err instanceof CommentIdempotencyConflictError || + code === 'COMMENT_IDEMPOTENCY_CONFLICT' || + name === 'CommentIdempotencyConflictError' + ) { + throw errors.conflict( + err instanceof Error + ? err.message + : 'clientMutationId already belongs to a different comment mutation' + ); + } + if ( + err instanceof CommentLimitExceededError || + code === 'COMMENT_LIMIT_EXCEEDED' || + name === 'CommentLimitExceededError' + ) { + throw errors.unprocessable(err instanceof Error ? err.message : 'Comment limit exceeded'); + } + throw err; +} diff --git a/apps/api/src/routes/chat-comments.ts b/apps/api/src/routes/chat-comments.ts index 034e478b35..834ed19b3a 100644 --- a/apps/api/src/routes/chat-comments.ts +++ b/apps/api/src/routes/chat-comments.ts @@ -1,12 +1,18 @@ import type { CommentStatus } from '@simple-agent-manager/shared'; -import { COMMENT_STATUSES } from '@simple-agent-manager/shared'; import { drizzle } from 'drizzle-orm/d1'; import { type Context, Hono } from 'hono'; import * as schema from '../db/schema'; import type { Env } from '../env'; +import { + getCommentActor, + parseCommentStatus, + parseNonNegativeIntegerQuery, + parsePositiveIntegerQuery, + rethrowCommentError, +} from '../lib/comment-http'; import { requireRouteParam } from '../lib/route-helpers'; -import { getAuth, getUserId } from '../middleware/auth'; +import { getUserId } from '../middleware/auth'; import { errors } from '../middleware/error'; import { requireProjectCapability } from '../middleware/project-auth'; import { jsonValidator } from '../schemas/_validator'; @@ -25,110 +31,6 @@ import * as projectDataService from '../services/project-data'; export const chatCommentRoutes = new Hono<{ Bindings: Env }>(); -function parseCommentStatus(rawStatus?: string): CommentStatus | null { - if (!rawStatus) return null; - const status = rawStatus.trim().toLowerCase(); - if (COMMENT_STATUSES.includes(status as CommentStatus)) return status as CommentStatus; - throw errors.badRequest('status must be open, sent, or resolved'); -} - -function parsePositiveIntegerQuery(name: string, rawValue?: string): number | null { - if (!rawValue) return null; - const parsed = Number.parseInt(rawValue, 10); - if (!Number.isFinite(parsed) || parsed <= 0) { - throw errors.badRequest(`${name} must be a positive integer`); - } - return parsed; -} - -function parseNonNegativeIntegerQuery(name: string, rawValue?: string): number | null { - if (!rawValue) return null; - const parsed = Number.parseInt(rawValue, 10); - if (!Number.isFinite(parsed) || parsed < 0) { - throw errors.badRequest(`${name} must be a non-negative integer`); - } - return parsed; -} - -function getCommentActor(c: Context<{ Bindings: Env }>) { - const auth = getAuth(c); - return { - kind: 'human' as const, - id: auth.user.id, - name: auth.user.name ?? auth.user.email ?? null, - }; -} - -function getCommentErrorName(err: unknown): string | null { - return err instanceof Error ? err.name : null; -} - -function getCommentErrorCode(err: unknown): string | null { - if (!err || typeof err !== 'object') return null; - const code = (err as { code?: unknown }).code; - return typeof code === 'string' ? code : null; -} - -function getCommentNotFoundResource(err: unknown): string { - if (err && typeof err === 'object') { - const resource = (err as { resource?: unknown }).resource; - if (typeof resource === 'string') return resource; - } - const message = err instanceof Error ? err.message : ''; - if (message.startsWith('Message ')) return 'Message'; - if (message.startsWith('Comment thread ')) return 'Comment thread'; - if (message.startsWith('Chat session ')) return 'Chat session'; - return 'Resource'; -} - -function isSerializedCommentNotFoundError(err: unknown): boolean { - if (!(err instanceof Error)) return false; - return ( - err.message === 'Chat session not found' || - err.message === 'Message not found' || - err.message === 'Comment thread not found' - ); -} - -function rethrowCommentError(err: unknown): never { - const code = getCommentErrorCode(err); - const name = getCommentErrorName(err); - if ( - err instanceof projectDataService.CommentValidationError || - code === 'COMMENT_VALIDATION' || - name === 'CommentValidationError' - ) { - throw errors.badRequest(err instanceof Error ? err.message : 'Invalid comment request'); - } - if ( - err instanceof projectDataService.CommentNotFoundError || - code === 'COMMENT_NOT_FOUND' || - name === 'CommentNotFoundError' || - isSerializedCommentNotFoundError(err) - ) { - throw errors.notFound(getCommentNotFoundResource(err)); - } - if ( - err instanceof projectDataService.CommentIdempotencyConflictError || - code === 'COMMENT_IDEMPOTENCY_CONFLICT' || - name === 'CommentIdempotencyConflictError' - ) { - throw errors.conflict( - err instanceof Error - ? err.message - : 'clientMutationId already belongs to a different comment mutation' - ); - } - if ( - err instanceof projectDataService.CommentLimitExceededError || - code === 'COMMENT_LIMIT_EXCEEDED' || - name === 'CommentLimitExceededError' - ) { - throw errors.unprocessable(err instanceof Error ? err.message : 'Comment limit exceeded'); - } - throw err; -} - function rethrowCommentDirectiveError(err: unknown): never { if (!isMessageCommentServiceError(err)) { throw err; diff --git a/apps/api/src/routes/library-comments.ts b/apps/api/src/routes/library-comments.ts new file mode 100644 index 0000000000..4906912b19 --- /dev/null +++ b/apps/api/src/routes/library-comments.ts @@ -0,0 +1,179 @@ +/** + * Library file comment routes. + * + * Project+file scoped (not session scoped). Mounted at + * /api/projects/:projectId/library — auth is inherited from the `projectsRoutes` + * wildcard, and every handler additionally asserts the caller's project + * capability before touching data. + */ +import { drizzle } from 'drizzle-orm/d1'; +import { type Context, Hono } from 'hono'; + +import * as schema from '../db/schema'; +import type { Env } from '../env'; +import { + getCommentActor, + parseCommentStatus, + parseNonNegativeIntegerQuery, + parsePositiveIntegerQuery, + rethrowCommentError, +} from '../lib/comment-http'; +import { requireRouteParam } from '../lib/route-helpers'; +import { getUserId } from '../middleware/auth'; +import { requireProjectCapability } from '../middleware/project-auth'; +import { jsonValidator } from '../schemas/_validator'; +import { + CommentStatusMutationSchema, + CreateCommentReplySchema, + CreateLibraryFileCommentThreadSchema, +} from '../schemas/comments'; +import { assertLibraryFileInProject } from '../services/library-file-comments'; +import * as projectDataService from '../services/project-data'; + +export const libraryCommentRoutes = new Hono<{ Bindings: Env }>(); + +type LibraryCommentScope = { + projectId: string; + fileId: string; + threadId: string; +}; + +/** + * Resolves the route params and authorizes the caller for this project. + * + * Every handler below needs exactly this preamble, so it lives in one place — + * a handler that forgot the capability check would otherwise be a one-line + * omission with no visible symptom. + */ +async function authorizeScope( + c: Context<{ Bindings: Env }>, + capability: 'task:read' | 'task:write' +): Promise { + const userId = getUserId(c); + const projectId = requireRouteParam(c, 'projectId'); + const fileId = requireRouteParam(c, 'fileId'); + const db = drizzle(c.env.DATABASE, { schema }); + + await requireProjectCapability(db, projectId, userId, capability); + + return { projectId, fileId, threadId: c.req.param('threadId') ?? '' }; +} + +/** + * GET /api/projects/:projectId/library/:fileId/comments + */ +libraryCommentRoutes.get('/:fileId/comments', async (c) => { + const { projectId, fileId } = await authorizeScope(c, 'task:read'); + + try { + await assertLibraryFileInProject(c.env, projectId, fileId); + const result = await projectDataService.listFileCommentThreads(c.env, projectId, { + fileId, + status: parseCommentStatus(c.req.query('status')), + afterSequence: parseNonNegativeIntegerQuery('afterSequence', c.req.query('afterSequence')), + limit: parsePositiveIntegerQuery('limit', c.req.query('limit')), + }); + return c.json(result); + } catch (err) { + rethrowCommentError(err); + } +}); + +/** + * POST /api/projects/:projectId/library/:fileId/comments + */ +libraryCommentRoutes.post( + '/:fileId/comments', + jsonValidator(CreateLibraryFileCommentThreadSchema), + async (c) => { + const { projectId, fileId } = await authorizeScope(c, 'task:write'); + const body = c.req.valid('json'); + + try { + // Only the entry points that can introduce a NEW fileId need the binding + // check. Reply/resolve/reopen reach their thread through a + // `WHERE id = ? AND file_id = ?` lookup inside this project's own durable + // object, so an existing thread already proves it was checked at create + // time (.claude/rules/60-request-io-and-bundle-budgets.md). + await assertLibraryFileInProject(c.env, projectId, fileId); + const result = await projectDataService.createFileCommentThread(c.env, projectId, { + fileId, + body: body.body, + quote: body.quote ?? null, + clientMutationId: body.clientMutationId ?? null, + actor: getCommentActor(c), + }); + return c.json(result, result.idempotent ? 200 : 201); + } catch (err) { + rethrowCommentError(err); + } + } +); + +/** + * POST /api/projects/:projectId/library/:fileId/comments/:threadId/replies + */ +libraryCommentRoutes.post( + '/:fileId/comments/:threadId/replies', + jsonValidator(CreateCommentReplySchema), + async (c) => { + const { projectId, fileId, threadId } = await authorizeScope(c, 'task:write'); + const body = c.req.valid('json'); + + try { + const result = await projectDataService.createFileCommentReply(c.env, projectId, { + fileId, + threadId, + body: body.body, + clientMutationId: body.clientMutationId ?? null, + actor: getCommentActor(c), + }); + return c.json(result, result.idempotent ? 200 : 201); + } catch (err) { + rethrowCommentError(err); + } + } +); + +/** + * Resolve and reopen differ only in the status they write, so they share a + * handler rather than existing as two near-identical copies. + */ +function statusMutationHandler(status: 'resolved' | 'open') { + return async (c: Context<{ Bindings: Env }>) => { + const { projectId, fileId, threadId } = await authorizeScope(c, 'task:write'); + const body = c.req.valid('json' as never) as { clientMutationId?: string | null }; + + try { + return c.json( + await projectDataService.updateFileCommentThreadStatus(c.env, projectId, { + fileId, + threadId, + status, + clientMutationId: body.clientMutationId ?? null, + actor: getCommentActor(c), + }) + ); + } catch (err) { + rethrowCommentError(err); + } + }; +} + +/** + * POST /api/projects/:projectId/library/:fileId/comments/:threadId/resolve + */ +libraryCommentRoutes.post( + '/:fileId/comments/:threadId/resolve', + jsonValidator(CommentStatusMutationSchema), + statusMutationHandler('resolved') +); + +/** + * POST /api/projects/:projectId/library/:fileId/comments/:threadId/reopen + */ +libraryCommentRoutes.post( + '/:fileId/comments/:threadId/reopen', + jsonValidator(CommentStatusMutationSchema), + statusMutationHandler('open') +); diff --git a/apps/api/src/routes/mcp/comment-tool-helpers.ts b/apps/api/src/routes/mcp/comment-tool-helpers.ts new file mode 100644 index 0000000000..ad29b5b7c8 --- /dev/null +++ b/apps/api/src/routes/mcp/comment-tool-helpers.ts @@ -0,0 +1,99 @@ +/** + * Shared JSON-RPC plumbing for the agent-facing comment tools. + * + * `comment-tools.ts` (message-anchored) and `library-file-comment-tools.ts` + * (library-file-anchored) had byte-identical copies of all of this. One + * implementation, so the two tool surfaces cannot drift on how they reject + * caller-supplied identity, coerce params, or map errors + * (.claude/rules/24-no-duplicate-ui-controls.md). + */ + +import type { MessageCommentThreadStatus } from '@simple-agent-manager/shared'; + +import { log } from '../../lib/logger'; +import { isMessageCommentServiceError } from '../../services/message-comments'; +import { + INTERNAL_ERROR, + INVALID_PARAMS, + jsonRpcError, + type JsonRpcResponse, + jsonRpcSuccess, +} from './_helpers'; + +/** + * Fields the server derives from the verified MCP token. An agent supplying any + * of them is either confused or attempting to act as someone else; either way + * the call is rejected rather than silently overridden. + */ +const CALLER_DERIVED_FIELDS = [ + 'projectId', + 'userId', + 'author', + 'authorId', + 'authorKind', + 'authorDisplayName', + 'provenance', +]; + +export function toolSuccess(requestId: string | number | null, value: unknown): JsonRpcResponse { + return jsonRpcSuccess(requestId, { + content: [{ type: 'text', text: JSON.stringify(value) }], + }); +} + +export function rejectCallerDerivedFields( + requestId: string | number | null, + params: Record +): JsonRpcResponse | null { + for (const field of CALLER_DERIVED_FIELDS) { + if (Object.prototype.hasOwnProperty.call(params, field)) { + return jsonRpcError( + requestId, + INVALID_PARAMS, + `${field} is derived from the verified MCP token and must not be supplied` + ); + } + } + return null; +} + +export function optionalString(params: Record, field: string): string | null { + const value = params[field]; + return typeof value === 'string' && value.trim() ? value.trim() : null; +} + +export function requiredString( + requestId: string | number | null, + params: Record, + field: string +): string | JsonRpcResponse { + const value = optionalString(params, field); + if (!value) return jsonRpcError(requestId, INVALID_PARAMS, `${field} is required`); + return value; +} + +export function parseStatusFilter( + requestId: string | number | null, + params: Record +): MessageCommentThreadStatus | 'all' | JsonRpcResponse { + const raw = optionalString(params, 'status') ?? 'open'; + if (raw === 'open' || raw === 'sent' || raw === 'resolved' || raw === 'all') return raw; + return jsonRpcError(requestId, INVALID_PARAMS, 'status must be open, sent, resolved, or all'); +} + +export function mapCommentError( + requestId: string | number | null, + err: unknown, + logTag: string, + logContext: Record +): JsonRpcResponse { + if (isMessageCommentServiceError(err)) { + const code = err.code === 'unavailable' ? INTERNAL_ERROR : INVALID_PARAMS; + return jsonRpcError(requestId, code, err.message); + } + log.warn(logTag, { + ...logContext, + error: err instanceof Error ? err.message : String(err), + }); + return jsonRpcError(requestId, INTERNAL_ERROR, 'Comment tool failed'); +} diff --git a/apps/api/src/routes/mcp/comment-tools.ts b/apps/api/src/routes/mcp/comment-tools.ts index e87fb633f8..b0aceb8ff5 100644 --- a/apps/api/src/routes/mcp/comment-tools.ts +++ b/apps/api/src/routes/mcp/comment-tools.ts @@ -17,75 +17,19 @@ import { clampCommentListLimit, createProjectDataMessageCommentAdapter, getMessageCommentConfig, - isMessageCommentServiceError, type MessageCommentStorageAdapter, normalizeCommentBody, normalizeCommentQuote, } from '../../services/message-comments'; +import { INVALID_PARAMS, jsonRpcError, type JsonRpcResponse, type McpTokenData } from './_helpers'; import { - INTERNAL_ERROR, - INVALID_PARAMS, - jsonRpcError, - type JsonRpcResponse, - jsonRpcSuccess, - type McpTokenData, -} from './_helpers'; - -const CALLER_DERIVED_FIELDS = [ - 'projectId', - 'userId', - 'author', - 'authorId', - 'authorKind', - 'authorDisplayName', - 'provenance', -]; - -function textContent(value: unknown): string { - return JSON.stringify(value); -} - -function toolSuccess(requestId: string | number | null, value: unknown): JsonRpcResponse { - return jsonRpcSuccess(requestId, { - content: [ - { - type: 'text', - text: textContent(value), - }, - ], - }); -} - -function rejectCallerDerivedFields( - requestId: string | number | null, - params: Record -): JsonRpcResponse | null { - for (const field of CALLER_DERIVED_FIELDS) { - if (Object.prototype.hasOwnProperty.call(params, field)) { - return jsonRpcError( - requestId, - INVALID_PARAMS, - `${field} is derived from the verified MCP token and must not be supplied` - ); - } - } - return null; -} - -function optionalString(params: Record, field: string): string | null { - const value = params[field]; - return typeof value === 'string' && value.trim() ? value.trim() : null; -} - -function requiredString( - requestId: string | number | null, - params: Record, - field: string -): string | JsonRpcResponse { - const value = optionalString(params, field); - if (!value) return jsonRpcError(requestId, INVALID_PARAMS, `${field} is required`); - return value; -} + mapCommentError, + optionalString, + parseStatusFilter, + rejectCallerDerivedFields, + requiredString, + toolSuccess, +} from './comment-tool-helpers'; function getStorage( env: Env, @@ -142,32 +86,6 @@ function isRpcError(value: { sessionId: string } | JsonRpcResponse): value is Js return 'jsonrpc' in value; } -function mapCommentError( - requestId: string | number | null, - err: unknown, - logTag: string, - logContext: Record -): JsonRpcResponse { - if (isMessageCommentServiceError(err)) { - const code = err.code === 'unavailable' ? INTERNAL_ERROR : INVALID_PARAMS; - return jsonRpcError(requestId, code, err.message); - } - log.warn(logTag, { - ...logContext, - error: err instanceof Error ? err.message : String(err), - }); - return jsonRpcError(requestId, INTERNAL_ERROR, 'Comment tool failed'); -} - -function parseStatus( - requestId: string | number | null, - params: Record -): MessageCommentThreadStatus | 'all' | JsonRpcResponse { - const raw = optionalString(params, 'status') ?? 'open'; - if (raw === 'open' || raw === 'sent' || raw === 'resolved' || raw === 'all') return raw; - return jsonRpcError(requestId, INVALID_PARAMS, 'status must be open, sent, resolved, or all'); -} - export async function handleListMessageCommentThreads( requestId: string | number | null, params: Record, @@ -179,7 +97,7 @@ export async function handleListMessageCommentThreads( if (identityError) return identityError; const session = await resolveCallerSession(requestId, params, tokenData, env); if (isRpcError(session)) return session; - const status = parseStatus(requestId, params); + const status = parseStatusFilter(requestId, params); if (typeof status !== 'string') return status; const config = getMessageCommentConfig(env); diff --git a/apps/api/src/routes/mcp/index.ts b/apps/api/src/routes/mcp/index.ts index 92bcd068e5..2accc25fcb 100644 --- a/apps/api/src/routes/mcp/index.ts +++ b/apps/api/src/routes/mcp/index.ts @@ -77,6 +77,10 @@ import { handleSearchKnowledge, handleUpdateKnowledge, } from './knowledge-tools'; +import { + handleCreateLibraryFileCommentThread, + handleListLibraryFileCommentThreads, +} from './library-file-comment-tools'; import { handleDisplayFromLibrary, handleDownloadLibraryFile, @@ -343,6 +347,14 @@ mcpRoutes.post('/', async (c) => { return c.json( await handleReopenMessageCommentThread(requestId, toolArgs, tokenData, c.env) ); + case 'list_library_file_comment_threads': + return c.json( + await handleListLibraryFileCommentThreads(requestId, toolArgs, tokenData, c.env) + ); + case 'create_library_file_comment_thread': + return c.json( + await handleCreateLibraryFileCommentThread(requestId, toolArgs, tokenData, c.env) + ); case 'send_message_to_subtask': return c.json(await handleSendMessageToSubtask(requestId, toolArgs, tokenData, c.env)); case 'stop_subtask': diff --git a/apps/api/src/routes/mcp/library-file-comment-tools.ts b/apps/api/src/routes/mcp/library-file-comment-tools.ts new file mode 100644 index 0000000000..7b69660b56 --- /dev/null +++ b/apps/api/src/routes/mcp/library-file-comment-tools.ts @@ -0,0 +1,143 @@ +/** + * MCP library-file comment tools. + * + * Project-scoped (no session resolution needed). Agents can list and create + * comment threads on library files without a chat session. + * + * Cross-references: apps/api/src/routes/library-comments.ts (HTTP routes) + */ +import type { Env } from '../../env'; +import { assertLibraryFileInProject } from '../../services/library-file-comments'; +import { + boundCommentThread, + boundCommentThreadSummary, + buildAgentCommentAuthor, + clampCommentListLimit, + getMessageCommentConfig, + normalizeCommentBody, + normalizeCommentQuote, +} from '../../services/message-comments'; +import * as projectDataService from '../../services/project-data'; +import { INVALID_PARAMS, jsonRpcError, type JsonRpcResponse, type McpTokenData } from './_helpers'; +import { + mapCommentError, + optionalString, + parseStatusFilter, + rejectCallerDerivedFields, + requiredString, + toolSuccess, +} from './comment-tool-helpers'; + +function parseCursor(cursor: string | null): number | null { + if (!cursor) return null; + const parsed = Number.parseInt(cursor, 10); + if (!Number.isFinite(parsed) || parsed < 0) return null; + return parsed; +} + +/** + * Wraps the shared file->project binding check into this module's JSON-RPC error + * shape. The query itself lives in services/library-file-comments.ts so the HTTP + * routes and these tools cannot diverge on what "the file belongs to this + * project" means. + */ +async function verifyFileInProject( + requestId: string | number | null, + env: Env, + projectId: string, + fileId: string +): Promise { + try { + await assertLibraryFileInProject(env, projectId, fileId); + return null; + } catch { + return jsonRpcError(requestId, INVALID_PARAMS, 'Library file not found'); + } +} + +export async function handleListLibraryFileCommentThreads( + requestId: string | number | null, + params: Record, + tokenData: McpTokenData, + env: Env +): Promise { + const identityError = rejectCallerDerivedFields(requestId, params); + if (identityError) return identityError; + const fileId = requiredString(requestId, params, 'fileId'); + if (typeof fileId !== 'string') return fileId; + const status = parseStatusFilter(requestId, params); + if (typeof status !== 'string') return status; + + const fileError = await verifyFileInProject(requestId, env, tokenData.projectId, fileId); + if (fileError) return fileError; + + const config = getMessageCommentConfig(env); + const limit = clampCommentListLimit(params.limit, config); + const cursor = optionalString(params, 'cursor'); + const afterSequence = parseCursor(cursor); + + try { + const result = await projectDataService.listFileCommentThreads(env, tokenData.projectId, { + fileId, + status: status === 'all' ? null : status, + afterSequence, + limit, + }); + const lastThread = result.threads.at(-1); + const nextCursor = + result.hasMore && typeof lastThread?.sequence === 'number' + ? String(lastThread.sequence) + : null; + return toolSuccess(requestId, { + threads: result.threads.map((thread) => boundCommentThreadSummary(thread, config)), + hasMore: result.hasMore, + nextCursor, + }); + } catch (err) { + return mapCommentError(requestId, err, 'mcp.library_file_comments.list_failed', { + projectId: tokenData.projectId, + fileId, + }); + } +} + +export async function handleCreateLibraryFileCommentThread( + requestId: string | number | null, + params: Record, + tokenData: McpTokenData, + env: Env +): Promise { + const identityError = rejectCallerDerivedFields(requestId, params); + if (identityError) return identityError; + const fileId = requiredString(requestId, params, 'fileId'); + if (typeof fileId !== 'string') return fileId; + const bodyRaw = requiredString(requestId, params, 'body'); + if (typeof bodyRaw !== 'string') return bodyRaw; + + const fileError = await verifyFileInProject(requestId, env, tokenData.projectId, fileId); + if (fileError) return fileError; + + const config = getMessageCommentConfig(env); + const body = normalizeCommentBody(bodyRaw, config); + if (!body) return jsonRpcError(requestId, INVALID_PARAMS, 'body is required'); + + const author = buildAgentCommentAuthor(tokenData); + + try { + const result = await projectDataService.createFileCommentThread(env, tokenData.projectId, { + fileId, + body, + quote: normalizeCommentQuote(optionalString(params, 'quote'), config), + clientMutationId: null, + actor: author, + }); + return toolSuccess(requestId, { + thread: boundCommentThread(result.thread, config), + }); + } catch (err) { + return mapCommentError(requestId, err, 'mcp.library_file_comments.create_failed', { + projectId: tokenData.projectId, + fileId, + }); + } +} diff --git a/apps/api/src/routes/mcp/tool-definitions-library-file-comment-tools.ts b/apps/api/src/routes/mcp/tool-definitions-library-file-comment-tools.ts new file mode 100644 index 0000000000..64440acc6a --- /dev/null +++ b/apps/api/src/routes/mcp/tool-definitions-library-file-comment-tools.ts @@ -0,0 +1,62 @@ +/** + * MCP tool definitions — library-file comment tools. + * + * Project-scoped (not session-scoped). Agents can list and create comment + * threads on library files without needing a chat session. + */ + +export const LIBRARY_FILE_COMMENT_TOOLS = [ + { + name: 'list_library_file_comment_threads', + description: + 'List comment threads on a project library file. Unlike message comments, these are project-scoped and do not require a chat session. Returns thread summaries; use get_message_comment_thread for full replies on message threads.', + inputSchema: { + type: 'object' as const, + properties: { + fileId: { + type: 'string', + description: 'The library file ID to list comments for (required).', + }, + status: { + type: 'string', + enum: ['open', 'sent', 'resolved', 'all'], + description: 'Filter by thread status. Defaults to open.', + }, + cursor: { + type: 'string', + description: 'Opaque pagination cursor from the previous response.', + }, + limit: { + type: 'number', + description: 'Max threads to return (default 10, server capped).', + }, + }, + required: ['fileId'], + additionalProperties: false, + }, + }, + { + name: 'create_library_file_comment_thread', + description: + 'Create an agent-authored comment thread on a project library file. Author identity and provenance are derived from your verified MCP token; do not provide author fields. The comment is project-scoped and does not require a chat session.', + inputSchema: { + type: 'object' as const, + properties: { + fileId: { + type: 'string', + description: 'The library file ID to comment on (required).', + }, + body: { + type: 'string', + description: 'Comment body (server sanitized and length capped).', + }, + quote: { + type: 'string', + description: 'Optional short quote from the file for citation context.', + }, + }, + required: ['fileId', 'body'], + additionalProperties: false, + }, + }, +]; diff --git a/apps/api/src/routes/mcp/tool-definitions.ts b/apps/api/src/routes/mcp/tool-definitions.ts index edd6ae4863..1960fe9ffe 100644 --- a/apps/api/src/routes/mcp/tool-definitions.ts +++ b/apps/api/src/routes/mcp/tool-definitions.ts @@ -18,6 +18,7 @@ export { COMMENT_TOOLS } from './tool-definitions-comment-tools'; export { DEPLOYMENT_TOOLS } from './tool-definitions-deployment-tools'; export { INCIDENT_TOOLS } from './tool-definitions-incident-tools'; export { KNOWLEDGE_TOOLS } from './tool-definitions-knowledge-tools'; +export { LIBRARY_FILE_COMMENT_TOOLS } from './tool-definitions-library-file-comment-tools'; export { LIBRARY_TOOLS } from './tool-definitions-library-tools'; export { MISSION_TOOLS } from './tool-definitions-mission-tools'; export { ORCHESTRATION_TOOLS } from './tool-definitions-orchestration-tools'; @@ -35,6 +36,7 @@ import { COMMENT_TOOLS } from './tool-definitions-comment-tools'; import { DEPLOYMENT_TOOLS } from './tool-definitions-deployment-tools'; import { INCIDENT_TOOLS } from './tool-definitions-incident-tools'; import { KNOWLEDGE_TOOLS } from './tool-definitions-knowledge-tools'; +import { LIBRARY_FILE_COMMENT_TOOLS } from './tool-definitions-library-file-comment-tools'; import { LIBRARY_TOOLS } from './tool-definitions-library-tools'; import { MISSION_TOOLS } from './tool-definitions-mission-tools'; import { ORCHESTRATION_TOOLS } from './tool-definitions-orchestration-tools'; @@ -57,6 +59,7 @@ export const MCP_TOOLS = [ ...LIBRARY_TOOLS, ...ORCHESTRATION_TOOLS, ...COMMENT_TOOLS, + ...LIBRARY_FILE_COMMENT_TOOLS, ...TRIGGER_TOOLS, ...INCIDENT_TOOLS, ...PROFILE_TOOLS, diff --git a/apps/api/src/schemas/comments.ts b/apps/api/src/schemas/comments.ts index 45ef59538a..42c53c209a 100644 --- a/apps/api/src/schemas/comments.ts +++ b/apps/api/src/schemas/comments.ts @@ -23,6 +23,12 @@ export const CommentStatusMutationSchema = v.object({ clientMutationId: v.optional(v.nullable(v.string())), }); +export const CreateLibraryFileCommentThreadSchema = v.object({ + body: v.string(), + quote: v.optional(v.nullable(v.string())), + clientMutationId: v.optional(v.nullable(v.string())), +}); + export const SendCommentDirectiveSchema = v.object({ body: v.optional(v.string()), clientMutationId: v.optional(v.nullable(v.string())), diff --git a/apps/api/src/services/library-file-comments.ts b/apps/api/src/services/library-file-comments.ts new file mode 100644 index 0000000000..f4be3cb504 --- /dev/null +++ b/apps/api/src/services/library-file-comments.ts @@ -0,0 +1,45 @@ +/** + * Library file comment authorization. + * + * `project_files` lives in D1; comment threads live in the per-project + * ProjectData Durable Object, which has no D1 access. So the file→project + * binding has to be proven here, at the control plane, before any DO call. + * + * Shared by the HTTP routes (`routes/library-comments.ts`) and the MCP tools + * (`routes/mcp/library-file-comment-tools.ts`) — the two had separate copies of + * this query (.claude/rules/24-no-duplicate-ui-controls.md). + */ + +import { drizzle } from 'drizzle-orm/d1'; + +import * as schema from '../db/schema'; +import { CommentNotFoundError } from '../durable-objects/project-data/comment-contracts'; +import type { Env } from '../env'; + +/** + * Proves `fileId` names a library file belonging to `projectId`. + * + * Throws `CommentNotFoundError('Library file')` for both "no such file" and + * "file belongs to another project" so the response cannot be used to probe for + * the existence of another project's files (.claude/rules/11-fail-fast-patterns.md, + * "Project-Scoped Read Requirements"). + * + * Only the entry points that can create a thread for a *new* fileId need this: + * list and create. Reply/resolve/reopen reach their thread through a + * `WHERE id = ? AND file_id = ?` lookup inside the project's own DO, so an + * existing thread is already proof the binding was checked at create time — + * re-checking costs a D1 round trip and proves nothing new + * (.claude/rules/60-request-io-and-bundle-budgets.md). + */ +export async function assertLibraryFileInProject( + env: Env, + projectId: string, + fileId: string +): Promise { + const db = drizzle(env.DATABASE, { schema }); + const file = await db.query.projectFiles.findFirst({ + where: (f, { and, eq }) => and(eq(f.projectId, projectId), eq(f.id, fileId)), + columns: { id: true }, + }); + if (!file) throw new CommentNotFoundError('Library file'); +} diff --git a/apps/api/src/services/message-comments.ts b/apps/api/src/services/message-comments.ts index ba9c04d56c..13a5a6838f 100644 --- a/apps/api/src/services/message-comments.ts +++ b/apps/api/src/services/message-comments.ts @@ -310,10 +310,19 @@ export function boundCommentThreadSummary< }; } -export function boundCommentThread( - thread: MessageCommentThread, - config: MessageCommentConfig -): MessageCommentThread { +/** + * Generic over the thread shape so message threads and library-file threads + * share one bounding implementation — both need bodies and quotes clamped to the + * same configured limits before they reach an agent. + */ +export function boundCommentThread< + T extends { + body: string; + anchor: { quote: string | null }; + sourceMessage?: { quote: string | null } | null; + replies: Array<{ body: string }>; + }, +>(thread: T, config: MessageCommentConfig): T { const summary = boundCommentThreadSummary(thread, config); return { ...summary, diff --git a/apps/api/src/services/project-data.ts b/apps/api/src/services/project-data.ts index 7ab52dc143..77dba34a0a 100644 --- a/apps/api/src/services/project-data.ts +++ b/apps/api/src/services/project-data.ts @@ -12,9 +12,11 @@ import type { CheckpointEpisode, CheckpointEpisodeTransitionInput, CommentAuthor, + CommentReply, CommentStatus, CreateCheckpointEpisodeInput, DeliveryState, + LibraryFileCommentMutationResponse, MessageClass, MessageCommentListResponse, MessageCommentMutationResponse, @@ -28,8 +30,13 @@ import type { ProjectData } from '../durable-objects/project-data'; import type { CreateCommentReplyInput, CreateCommentThreadInput, + CreateFileCommentReplyInput, + CreateFileCommentThreadInput, ListCommentThreadsInput, + ListFileCommentThreadsInput, + ListFileCommentThreadsResult, UpdateCommentStatusInput, + UpdateFileCommentStatusInput, } from '../durable-objects/project-data/comment-contracts'; import { CommentNotFoundError } from '../durable-objects/project-data/comment-contracts'; export { @@ -488,6 +495,50 @@ export async function updateCommentThreadStatus( ); } +// --- Library file comments --------------------------------------------------- +// Every entry point is `fileId`-scoped. Callers must have already proven the file +// belongs to `projectId` (see assertLibraryFileInProject) — the DO has no D1. + +export async function listFileCommentThreads( + env: Env, + projectId: string, + input: ListFileCommentThreadsInput +): Promise { + return callProjectDataWithRetry(env, projectId, 'listFileCommentThreads', (stub) => + stub.listFileCommentThreads(input) + ); +} + +export async function createFileCommentThread( + env: Env, + projectId: string, + input: CreateFileCommentThreadInput +): Promise { + return callProjectDataNoRetry(env, projectId, 'createFileCommentThread', (stub) => + stub.createFileCommentThread(input) + ); +} + +export async function createFileCommentReply( + env: Env, + projectId: string, + input: CreateFileCommentReplyInput +): Promise { + return callProjectDataNoRetry(env, projectId, 'createFileCommentReply', (stub) => + stub.createFileCommentReply(input) + ); +} + +export async function updateFileCommentThreadStatus( + env: Env, + projectId: string, + input: UpdateFileCommentStatusInput & { status: CommentStatus } +): Promise { + return callProjectDataNoRetry(env, projectId, 'updateFileCommentThreadStatus', (stub) => + stub.updateFileCommentThreadStatus(input) + ); +} + /** Materialize all stopped sessions that haven't been indexed yet. */ export async function materializeAllStopped( env: Env, diff --git a/apps/api/tests/integration/library-file-comments-vertical-slice.test.ts b/apps/api/tests/integration/library-file-comments-vertical-slice.test.ts new file mode 100644 index 0000000000..2e3a5e5d61 --- /dev/null +++ b/apps/api/tests/integration/library-file-comments-vertical-slice.test.ts @@ -0,0 +1,395 @@ +/** + * Vertical slice: HTTP request -> route -> validation -> ProjectData service + * boundary -> REAL durable-object SQLite (better-sqlite3, real migrations) and + * back out as a JSON response. + * + * The only stubs are the two genuine system boundaries: + * - D1 `project_files` (the file->project binding), seeded with realistic rows + * for two projects so cross-project access is actually exercised; + * - the ProjectData RPC hop, which here delegates straight into the real + * library-file-comments module instead of a Durable Object stub. + * + * Nothing in between is mocked, so a break anywhere along the path — schema, + * error mapping, fileId threading, SQL scoping — fails this test. + * See .claude/rules/35-vertical-slice-testing.md. + */ +import Database from 'better-sqlite3'; +import { drizzle } from 'drizzle-orm/d1'; +import { Hono } from 'hono'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { Env } from '../../src/env'; +import { AppError } from '../../src/middleware/error'; +import { createSqlStorage } from '../unit/durable-objects/sql-storage-test-utils'; + +const PROJECT_A = 'project-a'; +const PROJECT_B = 'project-b'; +const FILE_A = 'file-in-project-a'; +const FILE_A2 = 'other-file-in-project-a'; +const FILE_B = 'file-in-project-b'; + +/** Seeded D1 `project_files` rows. */ +const PROJECT_FILES = [ + { id: FILE_A, projectId: PROJECT_A }, + { id: FILE_A2, projectId: PROJECT_A }, + { id: FILE_B, projectId: PROJECT_B }, +]; + +const harness = vi.hoisted(() => ({ + /** Per-project durable object storage, keyed exactly as idFromName(projectId) would be. */ + storages: new Map(), + env: {} as Record, +})); + +vi.mock('drizzle-orm/d1'); +vi.mock('../../src/middleware/auth', () => ({ + requireAuth: () => vi.fn((_c: unknown, next: () => Promise) => next()), + requireApproved: () => vi.fn((_c: unknown, next: () => Promise) => next()), + getUserId: () => 'user-1', + getAuth: () => ({ + user: { + id: 'user-1', + email: 'ada@example.com', + name: 'Ada', + avatarUrl: null, + role: 'user', + status: 'active', + }, + session: { id: 'session-1', expiresAt: new Date('2030-01-01T00:00:00Z') }, + }), +})); +vi.mock('../../src/middleware/project-auth', () => ({ + requireProjectAccess: vi.fn(), + // Membership is orthogonal to what this slice proves; the caller is a member of + // both projects so the ONLY thing that can reject a cross-project request is + // the file->project binding itself. + requireProjectCapability: vi.fn().mockResolvedValue({ id: 'authorized-project' }), +})); + +// The service layer's only job here is the RPC hop. Replace it with a direct call +// into the real DO module against that project's real SQLite. +vi.mock('../../src/services/project-data', async () => { + const contracts = await vi.importActual< + typeof import('../../src/durable-objects/project-data/comment-contracts') + >('../../src/durable-objects/project-data/comment-contracts'); + const fileComments = await vi.importActual< + typeof import('../../src/durable-objects/project-data/library-file-comments') + >('../../src/durable-objects/project-data/library-file-comments'); + + const sqlFor = (projectId: string): SqlStorage => { + const storage = harness.storages.get(projectId); + if (!storage) throw new Error(`no durable object storage seeded for ${projectId}`); + return storage; + }; + // Cloudflare RPC reconstructs a thrown error on the caller side as a PLAIN + // Error carrying only `name` and `message`. The subclass identity and any + // class fields — including the `code` these errors define — do not survive. + // Reproduce that, otherwise the route's error mapping gets tested against a + // richer error than production ever sees and an `instanceof`/`code`-only + // mapping would pass here while 500ing in production. + const throughRpc = (fn: () => T): T => { + try { + return fn(); + } catch (err) { + if (!(err instanceof Error)) throw err; + const serialized = new Error(err.message); + serialized.name = err.name; + throw serialized; + } + }; + + return { + ...contracts, + listFileCommentThreads: async (_env: Env, projectId: string, input: unknown) => + throughRpc(() => + fileComments.listFileCommentThreads(sqlFor(projectId), harness.env as never, input as never) + ), + createFileCommentThread: async (_env: Env, projectId: string, input: unknown) => + throughRpc(() => + fileComments.createFileCommentThread(sqlFor(projectId), harness.env as never, input as never) + ), + createFileCommentReply: async (_env: Env, projectId: string, input: unknown) => + throughRpc(() => + fileComments.createFileCommentReply(sqlFor(projectId), harness.env as never, input as never) + ), + updateFileCommentThreadStatus: async (_env: Env, projectId: string, input: unknown) => + throughRpc(() => + fileComments.updateFileCommentThreadStatus( + sqlFor(projectId), + harness.env as never, + input as never + ) + ), + }; +}); + +import { runMigrations } from '../../src/durable-objects/migrations'; +import { libraryCommentRoutes } from '../../src/routes/library-comments'; + +let databases: Database.Database[] = []; + +function createApp() { + const app = new Hono<{ Bindings: Env }>(); + app.onError((err, c) => { + if (err instanceof AppError) return c.json(err.toJSON(), err.statusCode as never); + return c.json({ error: 'INTERNAL_ERROR', message: err.message }, 500); + }); + app.route('/api/projects/:projectId/library', libraryCommentRoutes); + return app; +} + +function get(projectId: string, fileId: string, query = '') { + return createApp().request( + `https://api.test/api/projects/${projectId}/library/${fileId}/comments${query}`, + { method: 'GET' }, + { DATABASE: {} } as Env + ); +} + +function post(path: string, body: unknown) { + return createApp().request( + `https://api.test${path}`, + { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }, + { DATABASE: {} } as Env + ); +} + +function commentsPath(projectId: string, fileId: string) { + return `/api/projects/${projectId}/library/${fileId}/comments`; +} + +describe('library file comments — vertical slice', () => { + beforeEach(() => { + vi.clearAllMocks(); + harness.storages.clear(); + harness.env = {}; + databases = []; + + for (const projectId of [PROJECT_A, PROJECT_B]) { + const db = new Database(':memory:'); + databases.push(db); + const sql = createSqlStorage(db); + runMigrations(sql); + harness.storages.set(projectId, sql); + } + + vi.mocked(drizzle).mockReturnValue({ + query: { + projectFiles: { + // Faithful enough to the drizzle contract to actually evaluate the + // predicate: a binding check that ignored projectId would pass here + // and then fail the cross-project tests below. + findFirst: async ({ + where, + }: { + where: ( + columns: { projectId: string; id: string }, + ops: { + and: (...parts: boolean[]) => boolean; + eq: (column: string, value: string) => boolean; + } + ) => boolean; + }) => + PROJECT_FILES.find((row) => + where( + { projectId: row.projectId, id: row.id }, + { + and: (...parts: boolean[]) => parts.every(Boolean), + eq: (column: string, value: string) => column === value, + } + ) + ), + }, + }, + } as never); + }); + + afterEach(() => { + for (const db of databases) db.close(); + }); + + it('carries a quoted comment from POST through real storage to a later GET', async () => { + const created = await post(commentsPath(PROJECT_A, FILE_A), { + body: 'This paragraph contradicts the section above.', + quote: 'the quick brown fox', + clientMutationId: 'client-1', + }); + + expect(created.status).toBe(201); + const createdPayload = (await created.json()) as { + thread: { id: string; anchor: { kind: string; fileId: string; quote: string } }; + }; + expect(createdPayload.thread.anchor).toEqual({ + kind: 'library_file', + fileId: FILE_A, + quote: 'the quick brown fox', + }); + + // A fresh request must read it back out of storage — not out of a mock. + const listed = await get(PROJECT_A, FILE_A); + expect(listed.status).toBe(200); + const listedPayload = (await listed.json()) as { + threads: Array<{ id: string; body: string; status: string; anchor: { quote: string } }>; + hasMore: boolean; + }; + expect(listedPayload.hasMore).toBe(false); + expect(listedPayload.threads).toHaveLength(1); + expect(listedPayload.threads[0]).toMatchObject({ + id: createdPayload.thread.id, + body: 'This paragraph contradicts the section above.', + status: 'open', + anchor: { quote: 'the quick brown fox' }, + }); + }); + + it('replays an identical clientMutationId as 200 without creating a second thread', async () => { + const payload = { body: 'Duplicate submit', clientMutationId: 'client-dup' }; + + const first = await post(commentsPath(PROJECT_A, FILE_A), payload); + const second = await post(commentsPath(PROJECT_A, FILE_A), payload); + + expect(first.status).toBe(201); + expect(second.status).toBe(200); + + const listed = (await (await get(PROJECT_A, FILE_A)).json()) as { threads: unknown[] }; + expect(listed.threads).toHaveLength(1); + }); + + it('runs a full reply / resolve / reopen lifecycle against real storage', async () => { + const created = (await ( + await post(commentsPath(PROJECT_A, FILE_A), { body: 'Please clarify' }) + ).json()) as { thread: { id: string } }; + const threadId = created.thread.id; + const base = `${commentsPath(PROJECT_A, FILE_A)}/${threadId}`; + + const replied = await post(`${base}/replies`, { body: 'Clarified below' }); + expect(replied.status).toBe(201); + + const resolved = await post(`${base}/resolve`, {}); + expect(resolved.status).toBe(200); + expect(((await resolved.json()) as { thread: { status: string } }).thread.status).toBe( + 'resolved' + ); + + const reopened = await post(`${base}/reopen`, {}); + expect(reopened.status).toBe(200); + expect(((await reopened.json()) as { thread: { status: string } }).thread.status).toBe('open'); + + const listed = (await (await get(PROJECT_A, FILE_A)).json()) as { + threads: Array<{ status: string; replies: Array<{ body: string }> }>; + }; + expect(listed.threads[0].status).toBe('open'); + expect(listed.threads[0].replies.map((r) => r.body)).toEqual(['Clarified below']); + }); + + it('filters by status through the whole stack', async () => { + const keepOpen = (await ( + await post(commentsPath(PROJECT_A, FILE_A), { body: 'Still open' }) + ).json()) as { thread: { id: string } }; + const toResolve = (await ( + await post(commentsPath(PROJECT_A, FILE_A), { body: 'Will be resolved' }) + ).json()) as { thread: { id: string } }; + + await post(`${commentsPath(PROJECT_A, FILE_A)}/${toResolve.thread.id}/resolve`, {}); + + const open = (await (await get(PROJECT_A, FILE_A, '?status=open')).json()) as { + threads: Array<{ id: string }>; + }; + expect(open.threads.map((t) => t.id)).toEqual([keepOpen.thread.id]); + + const resolved = (await (await get(PROJECT_A, FILE_A, '?status=resolved')).json()) as { + threads: Array<{ id: string }>; + }; + expect(resolved.threads.map((t) => t.id)).toEqual([toResolve.thread.id]); + }); + + describe('tenant and file isolation', () => { + it('keeps comments on sibling files in the same project separate', async () => { + await post(commentsPath(PROJECT_A, FILE_A), { body: 'About file A' }); + await post(commentsPath(PROJECT_A, FILE_A2), { body: 'About the other file' }); + + const fileA = (await (await get(PROJECT_A, FILE_A)).json()) as { + threads: Array<{ body: string }>; + }; + const fileA2 = (await (await get(PROJECT_A, FILE_A2)).json()) as { + threads: Array<{ body: string }>; + }; + + expect(fileA.threads.map((t) => t.body)).toEqual(['About file A']); + expect(fileA2.threads.map((t) => t.body)).toEqual(['About the other file']); + }); + + it("404s when a project's own route names another project's file", async () => { + // The caller is authorized for PROJECT_A and supplies a real fileId — it + // just belongs to PROJECT_B. The binding check is the only thing standing + // between that request and a thread attached to a foreign file. + const response = await post(commentsPath(PROJECT_A, FILE_B), { body: 'Cross-project' }); + + expect(response.status).toBe(404); + + // And nothing was written into either project's storage. + for (const projectId of [PROJECT_A, PROJECT_B]) { + const rows = harness.storages + .get(projectId)! + .exec('SELECT COUNT(*) AS c FROM library_file_comment_threads') + .toArray()[0] as { c: number }; + expect(rows.c).toBe(0); + } + }); + + it('404s when a thread id from one file is replayed against another file', async () => { + const created = (await ( + await post(commentsPath(PROJECT_A, FILE_A), { body: 'Owned by file A' }) + ).json()) as { thread: { id: string } }; + + const stolen = await post( + `${commentsPath(PROJECT_A, FILE_A2)}/${created.thread.id}/resolve`, + {} + ); + expect(stolen.status).toBe(404); + + // Owner-path control: the same call on the correct file succeeds, so the + // 404 above is the scoping guard and not a broken resolve endpoint. + const owner = await post( + `${commentsPath(PROJECT_A, FILE_A)}/${created.thread.id}/resolve`, + {} + ); + expect(owner.status).toBe(200); + }); + + it('does not leak one project’s threads into another project', async () => { + await post(commentsPath(PROJECT_A, FILE_A), { body: 'Project A comment' }); + await post(commentsPath(PROJECT_B, FILE_B), { body: 'Project B comment' }); + + const a = (await (await get(PROJECT_A, FILE_A)).json()) as { + threads: Array<{ body: string }>; + }; + const b = (await (await get(PROJECT_B, FILE_B)).json()) as { + threads: Array<{ body: string }>; + }; + + expect(a.threads.map((t) => t.body)).toEqual(['Project A comment']); + expect(b.threads.map((t) => t.body)).toEqual(['Project B comment']); + }); + }); + + it('surfaces a durable-object validation failure as 400 after the RPC hop strips the error class', async () => { + const response = await post(commentsPath(PROJECT_A, FILE_A), { body: ' ' }); + + expect(response.status).toBe(400); + }); + + it('enforces the configured per-file thread limit end to end', async () => { + harness.env.COMMENT_THREADS_PER_SESSION_MAX = '1'; + + expect((await post(commentsPath(PROJECT_A, FILE_A), { body: 'First' })).status).toBe(201); + expect((await post(commentsPath(PROJECT_A, FILE_A), { body: 'Second' })).status).toBe(422); + + // The cap is per file, so a sibling file is unaffected. + expect((await post(commentsPath(PROJECT_A, FILE_A2), { body: 'Elsewhere' })).status).toBe(201); + }); +}); diff --git a/apps/api/tests/unit/durable-objects/comments.test.ts b/apps/api/tests/unit/durable-objects/comments.test.ts index a70ac9e88e..774e3e3ded 100644 --- a/apps/api/tests/unit/durable-objects/comments.test.ts +++ b/apps/api/tests/unit/durable-objects/comments.test.ts @@ -3,6 +3,7 @@ import { beforeEach, describe, expect, it } from 'vitest'; import { runMigrations } from '../../../src/durable-objects/migrations'; import * as comments from '../../../src/durable-objects/project-data/comments'; +import * as fileComments from '../../../src/durable-objects/project-data/library-file-comments'; import type { Env } from '../../../src/durable-objects/project-data/types'; import { createSqlStorage } from './sql-storage-test-utils'; @@ -429,3 +430,435 @@ describe('ProjectData message comments', () => { expect(firstPage.threads.map((thread) => thread.id)).toEqual([first.id, second.id]); }); }); + +describe('ProjectData library file comments', () => { + let db: Database.Database; + let sql: SqlStorage; + let env: Env; + + beforeEach(() => { + db = new Database(':memory:'); + sql = createSqlStorage(db); + env = {} as Env; + runMigrations(sql); + }); + + function seedSession(sessionId: string): void { + sql.exec( + `INSERT INTO chat_sessions (id, topic, started_at) + VALUES (?, ?, ?)`, + sessionId, + `topic ${sessionId}`, + Date.now() + ); + } + + function seedMessage(sessionId: string, messageId: string, sequence: number): void { + sql.exec( + `INSERT INTO chat_messages + (id, session_id, role, content, tool_metadata, created_at, sequence) + VALUES (?, ?, ?, ?, ?, ?, ?)`, + messageId, + sessionId, + 'assistant', + `message ${messageId}`, + null, + Date.now() + sequence, + sequence + ); + } + + it('creates, lists, and idempotently replays file-anchored threads', () => { + const created = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-1', + body: ' Needs review ', + quote: ' selected text ', + clientMutationId: 'file-thread-key-1', + actor: HUMAN, + }); + + expect(created.idempotent).toBe(false); + expect(created.changed).toBe(true); + expect(created.thread).toMatchObject({ + fileId: 'file-1', + anchor: { kind: 'library_file', fileId: 'file-1', quote: 'selected text' }, + author: HUMAN, + body: 'Needs review', + status: 'open', + sequence: 1, + version: 1, + clientMutationId: 'file-thread-key-1', + replies: [], + }); + // A file thread is not session-scoped and must never carry a sessionId. + expect('sessionId' in created.thread).toBe(false); + + const replay = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-1', + body: 'Needs review', + quote: 'selected text', + clientMutationId: 'file-thread-key-1', + actor: HUMAN, + }); + expect(replay.idempotent).toBe(true); + expect(replay.changed).toBe(false); + expect(replay.thread.id).toBe(created.thread.id); + + expect(() => + fileComments.createFileCommentThread(sql, env, { + fileId: 'file-1', + body: 'Different body', + clientMutationId: 'file-thread-key-1', + actor: HUMAN, + }) + ).toThrow(comments.CommentIdempotencyConflictError); + + const listed = fileComments.listFileCommentThreads(sql, env, { fileId: 'file-1' }); + expect(listed.hasMore).toBe(false); + expect(listed.threads).toHaveLength(1); + expect(listed.threads[0]).toMatchObject({ + id: created.thread.id, + fileId: 'file-1', + sequence: 1, + }); + }); + + it('appends replies and status transitions on file threads', () => { + const created = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-1', + body: 'File thread', + clientMutationId: 'ft-key', + actor: HUMAN, + }); + + const firstReply = fileComments.createFileCommentReply(sql, env, { + fileId: 'file-1', + threadId: created.thread.id, + body: 'First reply', + clientMutationId: 'reply-key-1', + actor: HUMAN, + }); + const replayReply = fileComments.createFileCommentReply(sql, env, { + fileId: 'file-1', + threadId: created.thread.id, + body: 'First reply', + clientMutationId: 'reply-key-1', + actor: HUMAN, + }); + const secondReply = fileComments.createFileCommentReply(sql, env, { + fileId: 'file-1', + threadId: created.thread.id, + body: 'Second reply', + clientMutationId: 'reply-key-2', + actor: HUMAN, + }); + + expect(firstReply.idempotent).toBe(false); + expect(replayReply.idempotent).toBe(true); + expect(replayReply.reply.id).toBe(firstReply.reply.id); + expect(secondReply.thread.replies.map((r) => r.sequence)).toEqual([1, 2]); + // version: 1 (create) + 1 (reply 1) + 1 (reply 2) = 3 + expect(secondReply.thread.version).toBe(3); + + const resolved = fileComments.updateFileCommentThreadStatus(sql, env, { + fileId: 'file-1', + threadId: created.thread.id, + status: 'resolved', + clientMutationId: 'status-key-1', + actor: HUMAN, + }); + expect(resolved.thread.status).toBe('resolved'); + expect(resolved.thread.resolvedBy).toEqual(HUMAN); + expect(resolved.thread.version).toBe(4); + + const resolvedReplay = fileComments.updateFileCommentThreadStatus(sql, env, { + fileId: 'file-1', + threadId: created.thread.id, + status: 'resolved', + clientMutationId: 'status-key-1', + actor: HUMAN, + }); + expect(resolvedReplay.idempotent).toBe(true); + expect(resolvedReplay.changed).toBe(false); + + const reopened = fileComments.updateFileCommentThreadStatus(sql, env, { + fileId: 'file-1', + threadId: created.thread.id, + status: 'open', + clientMutationId: 'status-key-2', + actor: HUMAN, + }); + expect(reopened.thread.status).toBe('open'); + expect(reopened.thread.reopenedBy).toEqual(HUMAN); + expect(reopened.thread.version).toBe(5); + }); + + it('rejects the message-only "sent" status on a file thread', () => { + const created = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-1', + body: 'File thread', + actor: HUMAN, + }); + + expect(() => + fileComments.updateFileCommentThreadStatus(sql, env, { + fileId: 'file-1', + threadId: created.thread.id, + status: 'sent', + actor: HUMAN, + }) + ).toThrow(comments.CommentValidationError); + + // The rejected transition must not have mutated the thread. + const after = fileComments.getFileCommentThread(sql, 'file-1', created.thread.id); + expect(after).toMatchObject({ status: 'open', version: 1 }); + }); + + describe('file scoping', () => { + // Every read and mutate is scoped by file_id. A thread id alone must never + // be enough to reach a thread that belongs to a different file — this is the + // file-comment analogue of message-comment session isolation. + + it('does not return a thread through the wrong fileId', () => { + const owned = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-owner', + body: 'Owned thread', + actor: HUMAN, + }); + + // Control: the correct fileId does resolve the thread. + expect(fileComments.getFileCommentThread(sql, 'file-owner', owned.thread.id)).toMatchObject({ + id: owned.thread.id, + }); + // Attack: a real thread id under someone else's fileId resolves to nothing. + expect(fileComments.getFileCommentThread(sql, 'file-other', owned.thread.id)).toBeNull(); + }); + + it('refuses to reply to a thread through the wrong fileId', () => { + const owned = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-owner', + body: 'Owned thread', + actor: HUMAN, + }); + + expect(() => + fileComments.createFileCommentReply(sql, env, { + fileId: 'file-other', + threadId: owned.thread.id, + body: 'Injected reply', + actor: HUMAN, + }) + ).toThrow(comments.CommentNotFoundError); + + // Nothing was written. + const after = fileComments.getFileCommentThread(sql, 'file-owner', owned.thread.id); + expect(after?.replies).toEqual([]); + expect(after?.version).toBe(1); + + // Owner-path control: the legitimate fileId still works. + const ok = fileComments.createFileCommentReply(sql, env, { + fileId: 'file-owner', + threadId: owned.thread.id, + body: 'Legitimate reply', + actor: HUMAN, + }); + expect(ok.reply.body).toBe('Legitimate reply'); + }); + + it('refuses to change status of a thread through the wrong fileId', () => { + const owned = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-owner', + body: 'Owned thread', + actor: HUMAN, + }); + + expect(() => + fileComments.updateFileCommentThreadStatus(sql, env, { + fileId: 'file-other', + threadId: owned.thread.id, + status: 'resolved', + actor: HUMAN, + }) + ).toThrow(comments.CommentNotFoundError); + + expect(fileComments.getFileCommentThread(sql, 'file-owner', owned.thread.id)).toMatchObject({ + status: 'open', + }); + + // Owner-path control. + const ok = fileComments.updateFileCommentThreadStatus(sql, env, { + fileId: 'file-owner', + threadId: owned.thread.id, + status: 'resolved', + actor: HUMAN, + }); + expect(ok.thread.status).toBe('resolved'); + }); + }); + + it('keeps file threads and message threads in separate storage', () => { + seedSession('session-x'); + seedMessage('session-x', 'message-x', 1); + + const messageThread = comments.createCommentThread(sql, env, { + sessionId: 'session-x', + messageId: 'message-x', + body: 'Message comment', + clientMutationId: 'msg-key', + actor: HUMAN, + }); + + const fileThread = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-2', + body: 'File comment', + clientMutationId: 'file-key', + actor: HUMAN, + }); + + const sessionList = comments.listCommentThreads(sql, env, { sessionId: 'session-x' }); + expect(sessionList.threads).toHaveLength(1); + expect(sessionList.threads[0]).toMatchObject({ + id: messageThread.thread.id, + anchor: { kind: 'message', messageId: 'message-x' }, + }); + + const fileList = fileComments.listFileCommentThreads(sql, env, { fileId: 'file-2' }); + expect(fileList.threads).toHaveLength(1); + expect(fileList.threads[0]).toMatchObject({ + id: fileThread.thread.id, + anchor: { kind: 'library_file', fileId: 'file-2' }, + }); + + // Neither getter can reach across into the other table. + expect(comments.getCommentThread(sql, 'session-x', fileThread.thread.id)).toBeNull(); + expect(fileComments.getFileCommentThread(sql, 'file-2', messageThread.thread.id)).toBeNull(); + + // The tables are physically distinct. + const messageRows = sql + .exec('SELECT COUNT(*) AS c FROM comment_threads') + .toArray()[0] as { c: number }; + const fileRows = sql + .exec('SELECT COUNT(*) AS c FROM library_file_comment_threads') + .toArray()[0] as { c: number }; + expect(messageRows.c).toBe(1); + expect(fileRows.c).toBe(1); + }); + + it('enforces per-file thread limit', () => { + env.COMMENT_THREADS_PER_SESSION_MAX = '1'; + + fileComments.createFileCommentThread(sql, env, { + fileId: 'file-3', + body: 'First file thread', + actor: HUMAN, + }); + + expect(() => + fileComments.createFileCommentThread(sql, env, { + fileId: 'file-3', + body: 'Second file thread', + actor: HUMAN, + }) + ).toThrow(comments.CommentLimitExceededError); + + // The limit is per file, not global. + const otherFile = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-4', + body: 'Thread on different file', + actor: HUMAN, + }); + expect(otherFile.thread.fileId).toBe('file-4'); + }); + + it('rejects an empty fileId rather than creating an unreachable thread', () => { + expect(() => + fileComments.createFileCommentThread(sql, env, { + fileId: ' ', + body: 'Orphan', + actor: HUMAN, + }) + ).toThrow(comments.CommentValidationError); + }); + + it('lists file threads with status and afterSequence filters', () => { + const first = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-5', + body: 'First', + actor: HUMAN, + }).thread; + const second = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-5', + body: 'Second', + actor: HUMAN, + }).thread; + const third = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-5', + body: 'Third', + actor: HUMAN, + }).thread; + + fileComments.updateFileCommentThreadStatus(sql, env, { + fileId: 'file-5', + threadId: second.id, + status: 'resolved', + actor: HUMAN, + }); + + const openThreads = fileComments.listFileCommentThreads(sql, env, { + fileId: 'file-5', + status: 'open', + }); + expect(openThreads.threads.map((t) => t.id)).toEqual([first.id, third.id]); + + const resolvedThreads = fileComments.listFileCommentThreads(sql, env, { + fileId: 'file-5', + status: 'resolved', + }); + expect(resolvedThreads.threads).toMatchObject([{ id: second.id, status: 'resolved' }]); + + const afterFirst = fileComments.listFileCommentThreads(sql, env, { + fileId: 'file-5', + afterSequence: 1, + }); + expect(afterFirst.threads.map((t) => t.id)).toEqual([second.id, third.id]); + + const afterSecond = fileComments.listFileCommentThreads(sql, env, { + fileId: 'file-5', + afterSequence: 2, + }); + expect(afterSecond.threads.map((t) => t.id)).toEqual([third.id]); + + const firstPage = fileComments.listFileCommentThreads(sql, env, { + fileId: 'file-5', + limit: 2, + }); + expect(firstPage.hasMore).toBe(true); + expect(firstPage.threads.map((t) => t.id)).toEqual([first.id, second.id]); + }); + + it('skips a malformed row instead of failing the whole list read', () => { + const good = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-6', + body: 'Good one', + actor: HUMAN, + }).thread; + fileComments.createFileCommentThread(sql, env, { + fileId: 'file-6', + body: 'Bad one', + actor: HUMAN, + }); + const alsoGood = fileComments.createFileCommentThread(sql, env, { + fileId: 'file-6', + body: 'Also good', + actor: HUMAN, + }).thread; + + // Corrupt the middle row the way legacy data or a schema tightening would: + // SQLite is dynamically typed, so a text value survives the NOT NULL column + // but fails the valibot v.number(). See rule 50. + sql.exec(`UPDATE library_file_comment_threads SET created_at = 'not-a-number' WHERE sequence = 2`); + + const listed = fileComments.listFileCommentThreads(sql, env, { fileId: 'file-6' }); + expect(listed.threads.map((t) => t.id)).toEqual([good.id, alsoGood.id]); + }); +}); diff --git a/apps/api/tests/unit/durable-objects/migrations.test.ts b/apps/api/tests/unit/durable-objects/migrations.test.ts index fa6b166a95..361bc611ec 100644 --- a/apps/api/tests/unit/durable-objects/migrations.test.ts +++ b/apps/api/tests/unit/durable-objects/migrations.test.ts @@ -400,7 +400,9 @@ describe('DO Migrations', () => { // active-parent (030) and idempotency (031) indexes are CREATE UNIQUE // INDEX and are counted separately // message-anchored comments: 6 from migration 032 - expect(indexes.length).toBe(57); + // library-file comments: 4 from migration 033 (2 threads, 1 replies, + // 1 status_mutations) + expect(indexes.length).toBe(61); }); }); }); diff --git a/apps/api/tests/unit/routes/library-comments.test.ts b/apps/api/tests/unit/routes/library-comments.test.ts new file mode 100644 index 0000000000..450f2936f7 --- /dev/null +++ b/apps/api/tests/unit/routes/library-comments.test.ts @@ -0,0 +1,383 @@ +/** + * HTTP route tests for library file comments. + * + * Mocks only at the system boundaries the routes actually cross — D1 (via the + * drizzle query used by assertLibraryFileInProject), the project-auth middleware, + * and the ProjectData service. Everything between the request and those + * boundaries is the real code path. + */ +import { drizzle } from 'drizzle-orm/d1'; +import { Hono } from 'hono'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { Env } from '../../../src/env'; +import { AppError } from '../../../src/middleware/error'; + +const mocks = vi.hoisted(() => { + class CommentNotFoundError extends Error { + readonly code = 'COMMENT_NOT_FOUND'; + constructor( + readonly resource: 'Chat session' | 'Message' | 'Comment thread' | 'Library file' + ) { + super(`${resource} not found`); + this.name = 'CommentNotFoundError'; + } + } + class CommentValidationError extends Error { + readonly code = 'COMMENT_VALIDATION'; + constructor(message: string) { + super(message); + this.name = 'CommentValidationError'; + } + } + class CommentIdempotencyConflictError extends Error { + readonly code = 'COMMENT_IDEMPOTENCY_CONFLICT'; + constructor() { + super('clientMutationId already belongs to a different comment mutation'); + this.name = 'CommentIdempotencyConflictError'; + } + } + class CommentLimitExceededError extends Error { + readonly code = 'COMMENT_LIMIT_EXCEEDED'; + constructor(message: string) { + super(message); + this.name = 'CommentLimitExceededError'; + } + } + + return { + findFirst: vi.fn(), + requireProjectCapability: vi.fn(), + listFileCommentThreads: vi.fn(), + createFileCommentThread: vi.fn(), + createFileCommentReply: vi.fn(), + updateFileCommentThreadStatus: vi.fn(), + CommentIdempotencyConflictError, + CommentLimitExceededError, + CommentNotFoundError, + CommentValidationError, + }; +}); + +vi.mock('drizzle-orm/d1'); +vi.mock('../../../src/middleware/auth', () => ({ + requireAuth: () => vi.fn((_c: unknown, next: () => Promise) => next()), + requireApproved: () => vi.fn((_c: unknown, next: () => Promise) => next()), + getUserId: () => 'user-1', + getAuth: () => ({ + user: { + id: 'user-1', + email: 'user@example.com', + name: 'Ada', + avatarUrl: null, + role: 'user', + status: 'active', + }, + session: { id: 'session-1', expiresAt: new Date('2030-01-01T00:00:00Z') }, + }), +})); +vi.mock('../../../src/middleware/project-auth', () => ({ + requireProjectAccess: vi.fn(), + requireProjectCapability: mocks.requireProjectCapability, +})); +vi.mock('../../../src/services/project-data', () => ({ + CommentIdempotencyConflictError: mocks.CommentIdempotencyConflictError, + CommentLimitExceededError: mocks.CommentLimitExceededError, + CommentNotFoundError: mocks.CommentNotFoundError, + CommentValidationError: mocks.CommentValidationError, + listFileCommentThreads: mocks.listFileCommentThreads, + createFileCommentThread: mocks.createFileCommentThread, + createFileCommentReply: mocks.createFileCommentReply, + updateFileCommentThreadStatus: mocks.updateFileCommentThreadStatus, +})); + +import { libraryCommentRoutes } from '../../../src/routes/library-comments'; + +const FILE_ID = 'file-1'; +const THREAD_ID = 'thread-1'; + +const thread = { + id: THREAD_ID, + fileId: FILE_ID, + anchor: { kind: 'library_file' as const, fileId: FILE_ID, quote: null }, + author: { kind: 'human' as const, id: 'user-1', name: 'Ada' }, + body: 'Needs clarification', + status: 'open' as const, + createdAt: 1, + updatedAt: 1, + sequence: 1, + version: 1, + clientMutationId: null, + resolvedAt: null, + resolvedBy: null, + reopenedAt: null, + reopenedBy: null, + replies: [], +}; + +function createApp() { + const app = new Hono<{ Bindings: Env }>(); + app.onError((err, c) => { + if (err instanceof AppError) return c.json(err.toJSON(), err.statusCode as never); + return c.json({ error: 'INTERNAL_ERROR', message: err.message }, 500); + }); + app.route('/api/projects/:projectId/library', libraryCommentRoutes); + return app; +} + +function request(path: string, init?: RequestInit) { + return createApp().request(`https://api.test${path}`, init, { DATABASE: {} } as Env); +} + +function jsonPost(path: string, body: unknown) { + return request(path, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }); +} + +const BASE = `/api/projects/project-1/library/${FILE_ID}/comments`; + +describe('library file comment routes', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(drizzle).mockReturnValue({ + query: { projectFiles: { findFirst: mocks.findFirst } }, + } as never); + mocks.findFirst.mockResolvedValue({ id: FILE_ID }); + mocks.requireProjectCapability.mockResolvedValue({ id: 'project-1' }); + mocks.listFileCommentThreads.mockResolvedValue({ threads: [thread], hasMore: false }); + mocks.createFileCommentThread.mockResolvedValue({ thread, idempotent: false }); + mocks.createFileCommentReply.mockResolvedValue({ + thread: { ...thread, replies: [{ id: 'reply-1' }] }, + reply: { id: 'reply-1' }, + idempotent: false, + }); + mocks.updateFileCommentThreadStatus.mockResolvedValue({ + thread: { ...thread, status: 'resolved' }, + idempotent: false, + }); + }); + + describe('GET /:fileId/comments', () => { + it('lists threads under task:read with bounded query params', async () => { + const response = await request(`${BASE}?status=open&afterSequence=3&limit=10`); + + expect(response.status).toBe(200); + await expect(response.json()).resolves.toEqual({ threads: [thread], hasMore: false }); + expect(mocks.requireProjectCapability).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + 'user-1', + 'task:read' + ); + expect(mocks.listFileCommentThreads).toHaveBeenCalledWith(expect.anything(), 'project-1', { + fileId: FILE_ID, + status: 'open', + afterSequence: 3, + limit: 10, + }); + }); + + it('rejects a non-numeric limit before reaching the durable object', async () => { + const response = await request(`${BASE}?limit=abc`); + + expect(response.status).toBe(400); + expect(mocks.listFileCommentThreads).not.toHaveBeenCalled(); + }); + + it('rejects an unknown status before reaching the durable object', async () => { + const response = await request(`${BASE}?status=archived`); + + expect(response.status).toBe(400); + expect(mocks.listFileCommentThreads).not.toHaveBeenCalled(); + }); + }); + + describe('POST /:fileId/comments', () => { + it('creates a thread under task:write and returns 201', async () => { + const response = await jsonPost(BASE, { + body: 'Needs clarification', + quote: 'the selected text', + clientMutationId: 'mutation-1', + }); + + expect(response.status).toBe(201); + expect(mocks.requireProjectCapability).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + 'user-1', + 'task:write' + ); + expect(mocks.createFileCommentThread).toHaveBeenCalledWith(expect.anything(), 'project-1', { + fileId: FILE_ID, + body: 'Needs clarification', + quote: 'the selected text', + clientMutationId: 'mutation-1', + actor: { kind: 'human', id: 'user-1', name: 'Ada' }, + }); + }); + + it('returns 200 rather than 201 when the write was an idempotent replay', async () => { + mocks.createFileCommentThread.mockResolvedValue({ thread, idempotent: true }); + + const response = await jsonPost(BASE, { body: 'Needs clarification' }); + + expect(response.status).toBe(200); + }); + + it('rejects a whitespace-only body with 400', async () => { + // Body length/emptiness is validated once, in the durable object's shared + // normalizeBody, rather than duplicated into the route schema — matching + // how the message-comment routes do it. The route's job is to surface that + // rejection as a 400 rather than a 500. + mocks.createFileCommentThread.mockRejectedValue( + new mocks.CommentValidationError('body is required') + ); + + const response = await jsonPost(BASE, { body: ' ' }); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toMatchObject({ message: 'body is required' }); + }); + + it('rejects a request with no body field at all before reaching the durable object', async () => { + const response = await jsonPost(BASE, { quote: 'orphan quote' }); + + expect(response.status).toBe(400); + expect(mocks.createFileCommentThread).not.toHaveBeenCalled(); + }); + }); + + describe('POST /:fileId/comments/:threadId/replies', () => { + it('replies with the fileId threaded through so the DO can scope the lookup', async () => { + const response = await jsonPost(`${BASE}/${THREAD_ID}/replies`, { + body: 'Agreed', + clientMutationId: 'reply-mutation-1', + }); + + expect(response.status).toBe(201); + expect(mocks.createFileCommentReply).toHaveBeenCalledWith(expect.anything(), 'project-1', { + fileId: FILE_ID, + threadId: THREAD_ID, + body: 'Agreed', + clientMutationId: 'reply-mutation-1', + actor: { kind: 'human', id: 'user-1', name: 'Ada' }, + }); + }); + }); + + describe('POST /:fileId/comments/:threadId/{resolve,reopen}', () => { + it.each([ + ['resolve', 'resolved'], + ['reopen', 'open'], + ])('%s sends status %s scoped by fileId', async (action, status) => { + const response = await jsonPost(`${BASE}/${THREAD_ID}/${action}`, {}); + + expect(response.status).toBe(200); + expect(mocks.updateFileCommentThreadStatus).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + { + fileId: FILE_ID, + threadId: THREAD_ID, + status, + clientMutationId: null, + actor: { kind: 'human', id: 'user-1', name: 'Ada' }, + } + ); + }); + }); + + describe('file/project binding', () => { + it('404s and touches no comment storage when the file is not in this project', async () => { + mocks.findFirst.mockResolvedValue(undefined); + + const response = await request(BASE); + + expect(response.status).toBe(404); + expect(mocks.listFileCommentThreads).not.toHaveBeenCalled(); + }); + + it('404s on create when the file is not in this project', async () => { + mocks.findFirst.mockResolvedValue(undefined); + + const response = await jsonPost(BASE, { body: 'Sneaky' }); + + expect(response.status).toBe(404); + expect(mocks.createFileCommentThread).not.toHaveBeenCalled(); + }); + + it('checks the binding before authorizing storage, not after', async () => { + // The lookup must be scoped to BOTH the file and the project. Scoping to + // the file alone would let a caller attach comments to another project's + // file through their own project's durable object. + await request(BASE); + + const where = mocks.findFirst.mock.calls[0]?.[0]?.where; + expect(typeof where).toBe('function'); + const captured: string[] = []; + where( + { projectId: 'projectId', id: 'id' }, + { + and: (...parts: string[]) => parts.join(' AND '), + eq: (column: string, value: string) => { + captured.push(`${column}=${value}`); + return `${column}=${value}`; + }, + } + ); + expect(captured).toEqual(['projectId=project-1', `id=${FILE_ID}`]); + }); + + it('does not re-query D1 for reply/resolve/reopen', async () => { + // Those reach their thread via WHERE id = ? AND file_id = ? inside the + // project's own durable object, so an existing thread already proves the + // binding was checked at create time. Re-checking is a wasted round trip. + await jsonPost(`${BASE}/${THREAD_ID}/replies`, { body: 'Agreed' }); + await jsonPost(`${BASE}/${THREAD_ID}/resolve`, {}); + await jsonPost(`${BASE}/${THREAD_ID}/reopen`, {}); + + expect(mocks.findFirst).not.toHaveBeenCalled(); + }); + + it('propagates an authorization failure without touching comment storage', async () => { + mocks.requireProjectCapability.mockRejectedValue(new AppError(403, 'FORBIDDEN', 'Denied')); + + const response = await jsonPost(BASE, { body: 'Not mine' }); + + expect(response.status).toBe(403); + expect(mocks.findFirst).not.toHaveBeenCalled(); + expect(mocks.createFileCommentThread).not.toHaveBeenCalled(); + }); + }); + + describe('durable object error mapping', () => { + it.each([ + ['CommentValidationError', () => new mocks.CommentValidationError('body is required'), 400], + ['CommentNotFoundError', () => new mocks.CommentNotFoundError('Comment thread'), 404], + ['CommentIdempotencyConflictError', () => new mocks.CommentIdempotencyConflictError(), 409], + ['CommentLimitExceededError', () => new mocks.CommentLimitExceededError('too many'), 422], + ])('maps %s to %i', async (_name, makeError, expected) => { + mocks.createFileCommentThread.mockRejectedValue(makeError()); + + const response = await jsonPost(BASE, { body: 'Needs clarification' }); + + expect(response.status).toBe(expected); + }); + + it('maps an error that lost its class crossing the DO RPC boundary to 404, not 500', async () => { + // Cloudflare RPC serializes a thrown error down to name/message — the class + // and the `code` property do not survive. The first cut of these routes only + // matched on the class, so this surfaced as an INTERNAL_ERROR. + const serialized = new Error('Comment thread not found'); + serialized.name = 'Error'; + mocks.createFileCommentReply.mockRejectedValue(serialized); + + const response = await jsonPost(`${BASE}/${THREAD_ID}/replies`, { body: 'Agreed' }); + + expect(response.status).toBe(404); + }); + }); +}); diff --git a/apps/api/tests/unit/routes/mcp-library-file-comments.test.ts b/apps/api/tests/unit/routes/mcp-library-file-comments.test.ts new file mode 100644 index 0000000000..4e8ded273c --- /dev/null +++ b/apps/api/tests/unit/routes/mcp-library-file-comments.test.ts @@ -0,0 +1,542 @@ +import type { LibraryFileCommentThread } from '@simple-agent-manager/shared'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { Env } from '../../../src/env'; +import type { JsonRpcResponse, McpTokenData } from '../../../src/routes/mcp/_helpers'; + +// --------------------------------------------------------------------------- +// Mocks — must be declared before importing the handlers +// --------------------------------------------------------------------------- + +const findFirstMock = vi.fn(); + +vi.mock('../../../src/services/project-data', () => ({ + createFileCommentThread: vi.fn(), + listFileCommentThreads: vi.fn(), +})); + +vi.mock('drizzle-orm/d1', () => ({ + drizzle: () => ({ + query: { + projectFiles: { findFirst: findFirstMock }, + }, + }), +})); + +vi.mock('../../../src/lib/logger', () => ({ + log: { warn: vi.fn(), error: vi.fn(), info: vi.fn(), debug: vi.fn() }, +})); + +import { + handleCreateLibraryFileCommentThread, + handleListLibraryFileCommentThreads, +} from '../../../src/routes/mcp/library-file-comment-tools'; +import * as projectDataService from '../../../src/services/project-data'; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function makeToken(overrides: Partial = {}): McpTokenData { + return { + taskId: 'task-1', + projectId: 'project-1', + userId: 'user-1', + workspaceId: 'workspace-1', + chatSessionId: 'session-1', + agentSessionId: 'agent-session-1', + createdAt: '2026-08-22T00:00:00.000Z', + ...overrides, + }; +} + +function makeEnv(overrides = {}): Env { + return { + MCP_COMMENT_LIST_LIMIT: '5', + MCP_COMMENT_LIST_MAX: '25', + MCP_COMMENT_BODY_MAX_LENGTH: '4000', + MCP_COMMENT_QUOTE_MAX_LENGTH: '1000', + DATABASE: {} as D1Database, + ...overrides, + } as unknown as Env; +} + +function makeFileThread( + overrides: Partial = {} +): LibraryFileCommentThread { + return { + id: 'thread-1', + fileId: 'file-1', + anchor: { + kind: 'library_file', + fileId: 'file-1', + quote: 'some quoted code', + }, + author: { + kind: 'agent', + id: 'agent-session-1', + displayName: 'SAM agent', + }, + body: 'Please address this feedback', + status: 'open', + createdAt: 1000, + updatedAt: 1000, + resolvedAt: null, + replyCount: 0, + lastReplyAt: null, + replies: [], + ...overrides, + }; +} + +function parseToolResponse(response: JsonRpcResponse): unknown { + const result = response.result as { content: Array<{ text: string }> }; + return JSON.parse(result.content[0]?.text ?? '{}') as unknown; +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe('MCP library-file comment tools', () => { + beforeEach(() => { + vi.clearAllMocks(); + // Default: file exists + findFirstMock.mockResolvedValue({ id: 'file-1' }); + }); + + // ----------------------------------------------------------------------- + // handleListLibraryFileCommentThreads + // ----------------------------------------------------------------------- + + describe('handleListLibraryFileCommentThreads', () => { + it('lists file comment threads with cursor-to-afterSequence mapping', async () => { + const threads = [ + { ...makeFileThread({ id: 't-1' }), sequence: 5 }, + { ...makeFileThread({ id: 't-2' }), sequence: 8 }, + ]; + vi.mocked(projectDataService.listFileCommentThreads).mockResolvedValue({ + threads, + hasMore: true, + }); + + const response = await handleListLibraryFileCommentThreads( + 'req-1', + { fileId: 'file-1', cursor: '3', limit: 10 }, + makeToken(), + makeEnv() + ); + + expect(response.error).toBeUndefined(); + expect(vi.mocked(projectDataService.listFileCommentThreads)).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + expect.objectContaining({ + fileId: 'file-1', + afterSequence: 3, + }) + ); + + const data = parseToolResponse(response) as { + threads: unknown[]; + hasMore: boolean; + nextCursor: string | null; + }; + expect(data.hasMore).toBe(true); + // nextCursor is the string form of the last thread's sequence + expect(data.nextCursor).toBe('8'); + expect(data.threads).toHaveLength(2); + }); + + it('returns empty list without error', async () => { + vi.mocked(projectDataService.listFileCommentThreads).mockResolvedValue({ + threads: [], + hasMore: false, + }); + + const response = await handleListLibraryFileCommentThreads( + 'req-2', + { fileId: 'file-1' }, + makeToken(), + makeEnv() + ); + + expect(response.error).toBeUndefined(); + expect(parseToolResponse(response)).toEqual({ + threads: [], + hasMore: false, + nextCursor: null, + }); + }); + + it('rejects caller-derived identity fields', async () => { + const response = await handleListLibraryFileCommentThreads( + 'req-3', + { fileId: 'file-1', projectId: 'evil-project' }, + makeToken(), + makeEnv() + ); + + expect(response.error?.code).toBe(-32602); + expect(response.error?.message).toContain('projectId is derived'); + expect(vi.mocked(projectDataService.listFileCommentThreads)).not.toHaveBeenCalled(); + }); + + it('validates fileId is required', async () => { + const response = await handleListLibraryFileCommentThreads( + 'req-4', + {}, + makeToken(), + makeEnv() + ); + + expect(response.error?.code).toBe(-32602); + expect(response.error?.message).toContain('fileId is required'); + expect(vi.mocked(projectDataService.listFileCommentThreads)).not.toHaveBeenCalled(); + }); + + it('returns error when file does not exist', async () => { + findFirstMock.mockResolvedValue(null); + + const response = await handleListLibraryFileCommentThreads( + 'req-5', + { fileId: 'nonexistent' }, + makeToken(), + makeEnv() + ); + + expect(response.error?.code).toBe(-32602); + expect(response.error?.message).toContain('Library file not found'); + expect(vi.mocked(projectDataService.listFileCommentThreads)).not.toHaveBeenCalled(); + }); + + it('passes null afterSequence when cursor is absent', async () => { + vi.mocked(projectDataService.listFileCommentThreads).mockResolvedValue({ + threads: [], + hasMore: false, + }); + + await handleListLibraryFileCommentThreads( + 'req-6', + { fileId: 'file-1' }, + makeToken(), + makeEnv() + ); + + expect(vi.mocked(projectDataService.listFileCommentThreads)).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + expect.objectContaining({ afterSequence: null }) + ); + }); + + it('passes null afterSequence when cursor is non-numeric', async () => { + vi.mocked(projectDataService.listFileCommentThreads).mockResolvedValue({ + threads: [], + hasMore: false, + }); + + await handleListLibraryFileCommentThreads( + 'req-7', + { fileId: 'file-1', cursor: 'not-a-number' }, + makeToken(), + makeEnv() + ); + + expect(vi.mocked(projectDataService.listFileCommentThreads)).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + expect.objectContaining({ afterSequence: null }) + ); + }); + + it('filters by status when provided', async () => { + vi.mocked(projectDataService.listFileCommentThreads).mockResolvedValue({ + threads: [], + hasMore: false, + }); + + await handleListLibraryFileCommentThreads( + 'req-8', + { fileId: 'file-1', status: 'resolved' }, + makeToken(), + makeEnv() + ); + + expect(vi.mocked(projectDataService.listFileCommentThreads)).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + expect.objectContaining({ status: 'resolved' }) + ); + }); + + it('passes null status for "all" filter', async () => { + vi.mocked(projectDataService.listFileCommentThreads).mockResolvedValue({ + threads: [], + hasMore: false, + }); + + await handleListLibraryFileCommentThreads( + 'req-9', + { fileId: 'file-1', status: 'all' }, + makeToken(), + makeEnv() + ); + + expect(vi.mocked(projectDataService.listFileCommentThreads)).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + expect.objectContaining({ status: null }) + ); + }); + + it('rejects invalid status values', async () => { + const response = await handleListLibraryFileCommentThreads( + 'req-10', + { fileId: 'file-1', status: 'invalid' }, + makeToken(), + makeEnv() + ); + + expect(response.error?.code).toBe(-32602); + expect(response.error?.message).toContain('status must be'); + }); + + it('clamps limit to configured maximum', async () => { + vi.mocked(projectDataService.listFileCommentThreads).mockResolvedValue({ + threads: [], + hasMore: false, + }); + + await handleListLibraryFileCommentThreads( + 'req-11', + { fileId: 'file-1', limit: 999 }, + makeToken(), + makeEnv({ MCP_COMMENT_LIST_MAX: '10' }) + ); + + expect(vi.mocked(projectDataService.listFileCommentThreads)).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + expect.objectContaining({ limit: 10 }) + ); + }); + + it('returns nextCursor as null when hasMore is false', async () => { + vi.mocked(projectDataService.listFileCommentThreads).mockResolvedValue({ + threads: [{ ...makeFileThread(), sequence: 3 }], + hasMore: false, + }); + + const response = await handleListLibraryFileCommentThreads( + 'req-12', + { fileId: 'file-1' }, + makeToken(), + makeEnv() + ); + + expect(response.error).toBeUndefined(); + const parsed = parseToolResponse(response) as { nextCursor: string | null }; + expect(parsed.nextCursor).toBeNull(); + }); + + it('handles service errors safely without leaking stack traces', async () => { + vi.mocked(projectDataService.listFileCommentThreads).mockRejectedValue( + new Error('raw backend stack with token=secret') + ); + + const response = await handleListLibraryFileCommentThreads( + 'req-13', + { fileId: 'file-1' }, + makeToken(), + makeEnv() + ); + + expect(response.error).toEqual({ + code: -32603, + message: 'Comment tool failed', + }); + }); + }); + + // ----------------------------------------------------------------------- + // handleCreateLibraryFileCommentThread + // ----------------------------------------------------------------------- + + describe('handleCreateLibraryFileCommentThread', () => { + it('creates file comment thread with agent author derived from token', async () => { + const createdThread = makeFileThread({ + id: 'ct-1', + author: { kind: 'agent', id: 'agent-session-1', displayName: 'SAM agent' }, + }); + vi.mocked(projectDataService.createFileCommentThread).mockResolvedValue({ + thread: createdThread, + idempotent: false, + }); + + const response = await handleCreateLibraryFileCommentThread( + 'req-20', + { fileId: 'file-1', body: 'feedback', quote: 'some code' }, + makeToken(), + makeEnv() + ); + + expect(response.error).toBeUndefined(); + expect(vi.mocked(projectDataService.createFileCommentThread)).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + expect.objectContaining({ + fileId: 'file-1', + body: 'feedback', + quote: 'some code', + clientMutationId: null, + actor: { kind: 'agent', id: 'agent-session-1', displayName: 'SAM agent' }, + }) + ); + + const data = parseToolResponse(response) as { thread: { id: string } }; + expect(data.thread.id).toBe('ct-1'); + }); + + it('rejects caller-derived identity fields', async () => { + const response = await handleCreateLibraryFileCommentThread( + 'req-21', + { fileId: 'file-1', body: 'feedback', projectId: 'project-2' }, + makeToken(), + makeEnv() + ); + + expect(response.error?.code).toBe(-32602); + expect(response.error?.message).toContain('projectId is derived'); + expect(vi.mocked(projectDataService.createFileCommentThread)).not.toHaveBeenCalled(); + }); + + it('rejects all caller-derived fields individually', async () => { + const derivedFields = [ + 'userId', + 'author', + 'authorId', + 'authorKind', + 'authorDisplayName', + 'provenance', + ]; + + for (const field of derivedFields) { + vi.clearAllMocks(); + findFirstMock.mockResolvedValue({ id: 'file-1' }); + + const response = await handleCreateLibraryFileCommentThread( + 'req-derived', + { fileId: 'file-1', body: 'feedback', [field]: 'spoofed' }, + makeToken(), + makeEnv() + ); + + expect(response.error?.code).toBe(-32602); + expect(response.error?.message).toContain(`${field} is derived`); + } + }); + + it('validates fileId is required', async () => { + const response = await handleCreateLibraryFileCommentThread( + 'req-22', + { body: 'feedback' }, + makeToken(), + makeEnv() + ); + + expect(response.error?.code).toBe(-32602); + expect(response.error?.message).toContain('fileId is required'); + }); + + it('validates body is required', async () => { + const response = await handleCreateLibraryFileCommentThread( + 'req-23', + { fileId: 'file-1' }, + makeToken(), + makeEnv() + ); + + expect(response.error?.code).toBe(-32602); + expect(response.error?.message).toContain('body is required'); + }); + + it('returns error when file does not exist', async () => { + findFirstMock.mockResolvedValue(null); + + const response = await handleCreateLibraryFileCommentThread( + 'req-24', + { fileId: 'nonexistent-file', body: 'feedback' }, + makeToken(), + makeEnv() + ); + + expect(response.error?.code).toBe(-32602); + expect(response.error?.message).toContain('Library file not found'); + expect(vi.mocked(projectDataService.createFileCommentThread)).not.toHaveBeenCalled(); + }); + + it('handles service errors safely without leaking stack traces', async () => { + vi.mocked(projectDataService.createFileCommentThread).mockRejectedValue( + new Error('DO exploded with credentials=leak') + ); + + const response = await handleCreateLibraryFileCommentThread( + 'req-25', + { fileId: 'file-1', body: 'feedback' }, + makeToken(), + makeEnv() + ); + + expect(response.error).toEqual({ + code: -32603, + message: 'Comment tool failed', + }); + // Ensure no raw error message leaked + expect(response.error?.message).not.toContain('exploded'); + expect(response.error?.message).not.toContain('credentials'); + }); + + it('passes quote as null when not provided', async () => { + vi.mocked(projectDataService.createFileCommentThread).mockResolvedValue({ + thread: makeFileThread(), + idempotent: false, + }); + + await handleCreateLibraryFileCommentThread( + 'req-26', + { fileId: 'file-1', body: 'feedback' }, + makeToken(), + makeEnv() + ); + + expect(vi.mocked(projectDataService.createFileCommentThread)).toHaveBeenCalledWith( + expect.anything(), + 'project-1', + expect.objectContaining({ + quote: null, + }) + ); + }); + + it('uses projectId from token, not from params', async () => { + vi.mocked(projectDataService.createFileCommentThread).mockResolvedValue({ + thread: makeFileThread(), + idempotent: false, + }); + + await handleCreateLibraryFileCommentThread( + 'req-27', + { fileId: 'file-1', body: 'feedback' }, + makeToken({ projectId: 'my-project' }), + makeEnv() + ); + + expect(vi.mocked(projectDataService.createFileCommentThread)).toHaveBeenCalledWith( + expect.anything(), + 'my-project', + expect.anything() + ); + }); + }); +}); diff --git a/apps/api/tests/unit/routes/mcp.test.ts b/apps/api/tests/unit/routes/mcp.test.ts index 493cf63f56..2b1e1089bd 100644 --- a/apps/api/tests/unit/routes/mcp.test.ts +++ b/apps/api/tests/unit/routes/mcp.test.ts @@ -581,7 +581,10 @@ describe('MCP Routes', () => { expect(toolNames).toContain('get_incident'); expect(toolNames).toContain('claim_incident'); expect(toolNames).toContain('resolve_incident'); - expect(body.result.tools).toHaveLength(111); + // Library file comment tools + expect(toolNames).toContain('list_library_file_comment_threads'); + expect(toolNames).toContain('create_library_file_comment_thread'); + expect(body.result.tools).toHaveLength(113); }); it('should include MUST call directive in get_instructions description', async () => { diff --git a/apps/web/src/components/library/FileCommentPanel.tsx b/apps/web/src/components/library/FileCommentPanel.tsx new file mode 100644 index 0000000000..1725aa6a3f --- /dev/null +++ b/apps/web/src/components/library/FileCommentPanel.tsx @@ -0,0 +1,122 @@ +import { Spinner } from '@simple-agent-manager/ui'; +import { X } from 'lucide-react'; + +import type { MessageCommentAction } from '../../lib/api/comments'; +import { CommentComposer } from '../project-message-view/comments/CommentComposer'; +import { CommentThreadList } from '../project-message-view/comments/CommentThread'; +import { FOCUS_RING } from './types'; +import { useLibraryFileComments } from './useLibraryFileComments'; + +interface FileCommentPanelProps { + projectId: string; + fileId: string; + /** Text the user selected in the preview, attached to the thread they are about to create. */ + pendingQuote?: string | null; + onClearPendingQuote?: () => void; + onClose: () => void; +} + +export function FileCommentPanel({ + projectId, + fileId, + pendingQuote, + onClearPendingQuote, + onClose, +}: FileCommentPanelProps) { + const { comments, loading, error, mutationError, createThread, reply, resolve, reopen } = + useLibraryFileComments(projectId, fileId); + + // These reject on failure by design: CommentComposer catches, keeps the draft + // text, and leaves the reason to be rendered from `mutationError` below. + const handleCreateThread = async (body: string, _action: MessageCommentAction) => { + await createThread(body, pendingQuote ?? undefined); + onClearPendingQuote?.(); + }; + + const handleReply = async (threadId: string, body: string, _action: MessageCommentAction) => { + await reply(threadId, body); + }; + + const handleResolve = async (threadId: string) => { + try { + await resolve(threadId); + } catch { + /* surfaced via mutationError */ + } + }; + + const handleReopen = async (threadId: string) => { + try { + await reopen(threadId); + } catch { + /* surfaced via mutationError */ + } + }; + + return ( +
+
+

Comments

+ +
+ +
+ {loading && ( +
+ +
+ )} + + {error && !loading && ( +

{error}

+ )} + + {!loading && !error && ( + + )} +
+ +
+ {mutationError && ( +

+ {mutationError} +

+ )} + { + if (pendingQuote) { + onClearPendingQuote?.(); + return; + } + onClose(); + }} + /> +
+
+ ); +} diff --git a/apps/web/src/components/library/FilePreviewModal.tsx b/apps/web/src/components/library/FilePreviewModal.tsx index 882042af1f..41c65827de 100644 --- a/apps/web/src/components/library/FilePreviewModal.tsx +++ b/apps/web/src/components/library/FilePreviewModal.tsx @@ -1,5 +1,5 @@ import { Spinner } from '@simple-agent-manager/ui'; -import { AlertTriangle, Code, Download, Eye, X } from 'lucide-react'; +import { AlertTriangle, Code, Download, Eye, MessageSquare, X } from 'lucide-react'; import { useCallback, useEffect, useRef, useState } from 'react'; import { createPortal } from 'react-dom'; @@ -14,11 +14,21 @@ import { isPreviewableImageMime, } from '../../lib/file-utils'; import { RenderedMarkdown, SyntaxHighlightedCode } from '../MarkdownRenderer'; +import { + SelectionActionBar, + SelectionPopover, +} from '../project-message-view/comments/CommentPrimitives'; +import { + useCoarsePointer, + useCommentSelection, +} from '../project-message-view/comments/useCommentSelection'; import { ImageViewer } from '../shared-file-viewer/ImageViewer'; +import { FileCommentPanel } from './FileCommentPanel'; import { InteractiveHtmlPreview } from './InteractiveHtmlPreview'; import { type FileWithTags, FOCUS_RING } from './types'; export interface FilePreviewModalProps { + projectId: string; file: FileWithTags; previewUrl: string; onClose: () => void; @@ -83,11 +93,18 @@ function ViewToggle({ ); } -export function FilePreviewModal({ file, previewUrl, onClose, onDownload }: FilePreviewModalProps) { +export function FilePreviewModal({ + projectId, + file, + previewUrl, + onClose, + onDownload, +}: FilePreviewModalProps) { const dialogRef = useRef(null); const [pdfLoading, setPdfLoading] = useState(true); const [pdfError, setPdfError] = useState(false); - + const [commentsOpen, setCommentsOpen] = useState(false); + const [pendingQuote, setPendingQuote] = useState(null); // Pass the filename so an octet-stream/empty stored type (agent uploads) still // resolves to its real previewable type from the extension. const isImage = isPreviewableImageMime(file.mimeType, file.filename); @@ -98,6 +115,24 @@ export function FilePreviewModal({ file, previewUrl, onClose, onDownload }: File const htmlTooLarge = isHtml && file.sizeBytes > FILE_PREVIEW_LOAD_MAX_BYTES; const [mdViewMode, setMdViewMode] = useState<'rendered' | 'source'>('rendered'); + + // Selecting text in the rendered markdown offers "Comment on selection", which + // opens the panel with the quote attached. Reuses the same selection machinery + // as message comments — the preview body just declares itself an anchor. + const previewBodyRef = useRef(null); + const coarsePointer = useCoarsePointer(); + const { selection, clear: clearSelection } = useCommentSelection( + isMarkdown && mdViewMode === 'rendered', + previewBodyRef + ); + + const startQuotedComment = useCallback(() => { + if (!selection) return; + setPendingQuote(selection.quote); + setCommentsOpen(true); + clearSelection(); + window.getSelection()?.removeAllRanges(); + }, [clearSelection, selection]); // HTML defaults to the running preview. Source is the alternate view, and is fetched lazily so // opening an artifact costs one request (the signed-URL mint) instead of two. const [htmlViewMode, setHtmlViewMode] = useState<'preview' | 'source'>('preview'); @@ -246,6 +281,20 @@ export function FilePreviewModal({ file, previewUrl, onClose, onDownload }: File Download + - - ) : ( -