feat(library): comment on markdown files in the project library

.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

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)
+      `);
+    },
+  },
 ];
 
 /**

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';
   }