feat(library): comment on markdown files in the project library - #1889
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extend the message-anchored comment system to support comments on library files viewed in FilePreviewModal. File comments are project-scoped (not session-scoped like message comments). Changes by layer: - Shared: CommentAnchor discriminated union, LibraryFileCommentThread type - DO migration 033: relax constraints (nullable session_id/message_id, add file_id, widen anchor_kind CHECK) - DO impl: file anchor support in create/list/get operations - API routes: 5 endpoints under /api/projects/:projectId/library/:fileId/comments - MCP tools: list_library_file_comment_threads, create_library_file_comment_thread - Frontend: FileCommentPanel, useLibraryFileComments hook (TanStack Query with optimistic updates), FilePreviewModal comment toggle + side panel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DO tests (5 new): create/list/idempotency, replies/status transitions, cross-anchor isolation, per-file limit enforcement, status+pagination. MCP tests (12): cursor→afterSequence mapping, empty list, fileId required, caller-derived field rejection, file-not-found, null/invalid cursor, agent author creation, body required, service error safety. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… tests - Fix import sort in FileCommentPanel.tsx and comments.ts - Remove autoFocus prop (jsx-a11y/no-autofocus) - Update MCP tests to 22 tests with more thorough coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… isolation Rework in response to specialist review. Two CRITICAL findings: 1. Migration 033 recreated comment_threads (and its two CASCADE children) by backing up, DROP TABLE, and re-inserting, in order to widen anchor_kind and relax NOT NULL. Durable Object SQLite has no time-travel recovery, so this is irreversible (rule 31, quality:do-migration-safety). Replaced with a purely additive migration creating three NEW tables for library file comments: library_file_comment_threads / _replies / _status_mutations. 2. Widening the shared table forced session_id to become nullable, which in turn made getCommentThread drop its sessionId parameter — silently removing the session-ownership check from reply/resolve/reopen so any project collaborator could mutate another session's message threads. Separate tables remove the need entirely: comments.ts and comment-contracts.ts are restored byte-for-byte to their main versions, so message comments keep session isolation by construction.
…e comments Fixes 2-5 and 7-8 of the review rework. DO layer - New library-file-comments.ts implements file threads/replies/status against the migration-033 tables. Every read and mutate is scoped by `file_id`, so a thread id alone can never reach a thread on another file. - comments.ts, comment-contracts.ts, project-data/index.ts, services/project-data.ts and services/message-comments.ts are restored to main for everything message-related, then extended additively. This is what restores the session isolation the first cut removed. - Extracted comment-normalization.ts (limits, body/quote/actor normalization, fingerprinting) so the two storage backends share one validator rather than drifting copies. HTTP layer - Extracted lib/comment-http.ts from chat-comments.ts and deleted the weaker duplicate in library-comments.ts. The library copy did not recognise errors that had crossed the DO RPC boundary (where the error class is lost), so an expected 404 surfaced as a 500. Both routers now use the one implementation. - Extracted services/library-file-comments.ts:assertLibraryFileInProject, the single file→project binding check, shared with the MCP tools. - Dropped the redundant file-existence query from reply/resolve/reopen: those reach their thread via WHERE id = ? AND file_id = ? inside the project's own DO, so an existing thread already proves the binding was checked at create time. Saves a D1 round trip per mutation.
- listCommentThreads(fileId, anchorKind) -> listFileCommentThreads(fileId) - Reuse the shared assertLibraryFileInProject binding check instead of a second copy of the D1 query. - boundCommentThread is now generic over the thread shape, so the library tools stop casting a LibraryFileCommentThread to MessageCommentThread to satisfy it.
- DO tests exercise library-file-comments.ts directly and add a 'file scoping' block: for get/reply/status, a real thread id under a different fileId must be rejected, each paired with an owner-path control. Verified discriminating by deleting the 'AND file_id = ?' predicate — exactly those three go red. - Added: 'sent' status rejected on a file thread (with a no-mutation assertion), empty fileId rejected, malformed-row skip on list reads (rule 50), and an assertion that the two anchor kinds live in physically distinct tables. - migrations.test.ts index count 57 -> 61 for the four new migration-033 indexes. - MCP tests follow listCommentThreads -> listFileCommentThreads.
…age anchor
Fixes 6, 9 and 10 of the review rework.
- Quote selection (the piece task-completion-validator found missing): the
rendered markdown body is now a data-comment-anchor, driving the existing
useCommentSelection / SelectionPopover / SelectionActionBar machinery. Selecting
text offers 'Comment on selection', which opens the panel with the quote
attached and prefilled in the composer. Coarse pointers get the bottom action
bar, fine pointers the popover — same split as message comments.
- fileThreadToUi forged { kind: 'message', messageId: thread.fileId } to satisfy
a message-typed prop, defeating the anchor discriminated union at exactly the
boundary it exists to protect. Introduced UiCommentThread — the anchor-agnostic
subset CommentThread/CommentThreadList actually render — and typed those
components on it. File threads now pass their real library_file anchor through.
- Mutations no longer invalidate on settle. onSuccess already writes the
authoritative server row into the cache, so the refetch doubled the round trips
on every comment. Invalidation now runs only on error, after rollback.
Closes the 'zero HTTP route test coverage' and 'no vertical-slice test' findings. routes/library-comments.test.ts (20 tests) — auth capability per verb, query param bounds, 201-vs-200 idempotent replay, fileId threading into every service call, the file/project binding (including a test that evaluates the drizzle predicate itself, so a check that dropped projectId cannot pass), proof that reply/resolve/reopen do NOT re-query D1, and the full DO error -> HTTP status mapping including an error that lost its class crossing the RPC boundary. integration/library-file-comments-vertical-slice.test.ts (10 tests) — the real route over real DO SQLite with real migrations, two projects with their own storage. Covers quote round-trip through storage, idempotent replay, full reply/resolve/reopen lifecycle, status filtering, sibling-file isolation, cross-project 404 with a no-write assertion, thread-id-replayed-against-another- file 404 with an owner-path control, and the per-file limit. The RPC hop is simulated at production fidelity — a plain Error carrying only name and message — so an instanceof/code-only error mapping cannot pass here while 500ing in production. Both guards verified discriminating: dropping projectId from the binding predicate fails exactly the two binding tests; dropping 'AND file_id = ?' fails exactly the three scoping tests. API suite: 7985 passed, 0 failed, 0 collection errors (594 files).
Two user-visible bugs the new behavioral tests caught. - upsertLibraryFileCommentThread matched on server id only, so the optimistic row — inserted under a locally generated id — was never replaced and the user saw their own comment twice. The message-comment upsert already matched on clientId too; both now share one generic upsertThreadBy helper. This became user-visible rather than transient once the redundant post-mutation refetch was removed. - A failed create/reply rolled the optimistic row back with no explanation, which looks exactly like nothing happening. The hook now records the reason and the panel renders it as a role=alert. CommentComposer catches the rejection so it stops escaping as an unhandled promise (it is submitted via `void submit()`) while still keeping the draft text for a retry. Adds apps/web/tests/unit/components/library-file-comments.test.tsx — 10 behavioral tests that render the panel and drive the real interactions: create, create-with-quote, cancel-quoted-draft, reply, resolve then reopen, empty state, load error, failed-create rollback + alert + draft retention, and an assertion that send-to-agent is never offered (Phase 2). Web suite: 3440 passed, 0 failed, 0 collection errors (287 files).
Caught by the new Playwright audit at 1280px: inside the file preview modal the 'Comment' popover rendered at z-dropdown (20) while the modal sits at z-dialog-backdrop (50), so the modal's own content intercepted the click and a desktop user could not start a quoted comment at all. Mobile was unaffected — SelectionActionBar already used z-panel (60). Popover gains an explicit `layerClassName` (default z-dropdown, unchanged for every existing caller) and SelectionPopover opts into z-panel, matching its coarse-pointer twin. Deterministic layering rather than relying on class order. Adds tests/playwright/library-file-comments-audit.spec.ts — 4 scenarios x 2 viewports (375 and 1280): empty, populated (long body, long quote, unbreakable URL, collapsed resolved thread), load error, and the full select-text -> comment-on-selection -> post flow, which asserts the quote reached the POST body rather than merely rendering. All 8 pass; 12 screenshots reviewed and all distinct.
- Task file moved back to tasks/active/ (it was archived before the reviewers' findings were addressed), research section corrected to describe the separate- tables design that was actually shipped, and all 11 review findings recorded. - Adds .claude/rules/63-widening-a-table-can-delete-an-auth-check.md. The class of bug: relaxing a scoping column to nullable so a second row kind can share a table silently removes every authorization predicate that used it — the scope parameter stops type-checking, gets deleted as a 'type fix', and nothing goes red.
Contributor
Two CI-only failures, both from scanners doing exactly their job. - quality:do-migration-safety pattern-matches migration SOURCE, so the literal 'ALTER ... CHECK' inside the explanatory comment tripped the unsafe-pattern rule. Reworded to prose. (Same shape as the earlier 'DROP TABLE' mention.) - quality:ast-checks flags any interpolation into sql.exec(), allowlisting only a fixed set of clause-builder identifiers. Inlined the column list literally, matching how comments.ts already writes these queries, and named the joined predicate `whereClause` so the parameterized-builder allowance applies. Weakening the scanner for a column list would blunt a real guard.
The quality gate failed on new_duplicated_lines_density (5.7% vs a 3% ceiling), and it was pointing at real copy-paste — the same class the architecture reviewer raised. Fixed rather than waived. - routes/mcp/comment-tool-helpers.ts (new): toolSuccess, rejectCallerDerivedFields, optionalString, requiredString, parseStatusFilter and mapCommentError were byte-identical in comment-tools.ts and library-file-comment-tools.ts. Both now import one copy, so the two agent-facing tool surfaces cannot drift on how they reject caller-supplied identity or map errors. - routes/library-comments.ts: the four handlers repeated the same resolve-params-then-authorize preamble, where a missing capability check would have been a silent one-line omission — extracted to authorizeScope(). Resolve and reopen differed only in the status they write, so they now share statusMutationHandler(). - useLibraryFileComments.ts: the four mutations repeated identical snapshot/rollback/report/upsert scaffolding — extracted to a module-scope useOptimisticThreadMutation() taking the request and the local edit. Kept at module scope rather than nested inside the hook, which is valid but reads as a smell. Behaviour unchanged: API 7985 passed / 0 failed, web 3440 passed / 0 failed, typecheck clean, lint 0 errors, ast-checks and file-sizes exit 0.
task-completion-validator re-run on the reworked diff returned PASS with no CRITICAL or HIGH. Its three lesser findings are fixed here rather than deferred. - The task file checked off 'unit tests for shared types' with no artifact. Added packages/shared/tests/comment-anchors.test.ts: union narrowing per variant, per-variant field exclusivity (a file anchor has no messageId and vice versa — if those ever collapsed into one optional-everything shape the storage separation would be unenforceable), null quotes, and a parity assertion between COMMENT_ANCHOR_KINDS and the union's variants so a kind cannot be added to one without the other. - Removed dead code: services/project-data.ts:getFileCommentThread and the matching ProjectData RPC method had zero callers, since Phase 1 has no send-to-agent for files. The module-level getFileCommentThread stays — that one IS the file-scoped ownership guard behind create/reply/status. - 'Comments persist across modal close/reopen' was only covered indirectly by the POST-then-GET slice. The Playwright audit now closes and reopens the modal and re-asserts the thread. On mobile the panel is a full-width overlay over the header, so it closes the panel first — which is the real mobile flow. Left as-is: library-file-comments.ts at 553 lines is over rule 18's 500-line soft threshold but under the 800-line mandatory one, quality:file-sizes passes, and it is smaller than the 698-line comments.ts it mirrors. API 7985/0, shared 590/0, Playwright audit 8/8, typecheck clean, lint 0 errors.
|
28 tasks
simple-agent-manager Bot
added a commit
that referenced
this pull request
Aug 23, 2026
#1890) * fix(web): stop remounting rendered markdown, which destroyed text selection Reported on production (Android): long-pressing text in a markdown library file selected one word and immediately deselected it, so the drag handles could never be used and quoting a passage was impossible. Raphaël also recalled something similar in chat message comments — same root cause, different trigger. RenderedMarkdown passed `components={{ ... }}` as an object literal inside its render body. react-markdown renders each node via `createElement(components[tag], ...)`, so every override got a NEW function identity on every render. React therefore saw a different component *type* for every heading, paragraph and list item and unmounted/remounted the entire document rather than reconciling it. A native Selection is anchored to real DOM nodes. Rebuilding those nodes drops the selection and its handles. In the file preview the trigger is immediate: the selection handler calls setSelection, which re-renders the modal, which rebuilt the paragraph under the user's finger. Chat hits the same trap on every poll and stream update. Fix: hoist the overrides to a module-scope MARKDOWN_COMPONENTS (they close over nothing from props) and memo the component so an unrelated parent re-render does not re-render the document at all. This is pre-existing — PR #1889 did not introduce it, it just added a surface that re-renders at exactly the wrong moment. #1883 previously fixed an adjacent symptom (the React snapshot being nulled) but not the underlying DOM churn. Regression test asserts DOM node identity across an unrelated re-render AND that a live Selection survives it, plus a control proving memo did not make the component stale. Verified discriminating: both assertions fail on the pre-fix code, the control passes on both. Web suite 3443 passed / 0 failed; Playwright audits 10/10; typecheck and lint clean. * docs: add rule 64 — unstable prop identity remounts subtrees Process fix for the markdown-remount bug. Covers the class (a config object built in a render body and handed to a library that uses its values as component types), what breaks when node identity is lost (selection, focus, IME, scroll, media position, transitions), and the DOM-identity assertion pattern that catches it — toBe, not toEqual, because a remount serializes identically. Also records the corollary to rule 62 this bug exposed: a test can use the real trigger and still miss the failure if it completes the interaction inside one render. The Playwright audit drove a real selection and passed while the feature was unusable, because it created and read the selection in the same tick and never sat through a re-render mid-gesture. * fix(web): address architecture review — spread regression, 4th duplicate, test hole Local architecture-reviewer findings, all fixed rather than deferred. HIGH — I had shipped `components={{ ...MARKDOWN_COMPONENTS }}` in the very commit that added the rule forbidding it. The reviewer verified it does not reintroduce the remount (react-markdown resolves per-tag, and a shallow spread copies each override by reference) but it allocates a fresh container every render for no benefit and erodes the hoist it copies. Now passes the constant. MEDIUM — GitDiffView.tsx carried a FOURTH independent copy of this renderer with the same inline-components bug, live in the diff viewer's full-file markdown mode. Selecting text there still deselected. Hoisted + memoized it too. Kept it separate from MarkdownRenderer because its typography genuinely differs; consolidating the copies is tracked, not silently ignored. MEDIUM — the reviewer also found MessageBubble.tsx had ALREADY solved this independently (hoisted overrides + memo, with a comment describing the same mechanism). So chat *message* text was never affected — correcting my earlier claim. Three independent hand-rolled fixes before this one is the argument for mechanical enforcement, below. Test hole — the DOM-stability tests drove their re-render through memo's bail-out, so memo alone satisfied them and they could not see the spread. Added a test that renders the UNMEMOIZED implementation, forcing a real re-render, and a new markdown-components-identity.test.tsx that mocks react-markdown to capture the components prop across renders and assert container + per-tag identity. Verified discriminating: reintroducing the spread fails the container assertion while per-tag correctly still passes, exactly matching the reviewer's analysis. Removed a tautological identity assertion I had written (spreading copies references, so it could never fail). Reusable enforcement — added sam/no-inline-markdown-components to eslint-plugin-sam, wired as an error. A prose rule was demonstrably not enough: the commit adding .claude/rules/64 violated it in the same diff. Verified the rule flags an inline literal AND a spread, and passes a hoisted constant. Web suite 3446 passed / 0 failed; typecheck and lint clean. * fix(web): stop the selection action bar covering the text it acts on Second, independent cause of the reported Android symptom, found by the local ui-ux-specialist and proven geometrically rather than by inspection. SelectionActionBar is `fixed bottom-0`, full width, ~97px tall, and — unlike SelectionPopover, which anchors to the selection's own rect — ignored the selection's position entirely. A diagnostic at 375x667 measured a selected paragraph at top:581/bottom:650 against a bar at top:570/bottom:667: the selection was completely inside the bar. The screenshot shows the paragraph fully hidden, surviving only as the truncated quote inside the bar itself. The lower drag handle is under there too, so the user sees a word select and then cannot extend it — the same user-visible symptom as the remount bug, from a different cause. It bites hardest in chat, where alignToBottom deliberately puts the newest message (the one most likely to be quoted) in exactly that band. Fix: latch the selection's rect alongside the quote (same reason the quote is latched — the browser may collapse the Selection before the bar renders), and flip the bar to the top when a bottom-pinned bar would overlap. Height is measured in useLayoutEffect rather than estimated, because it depends on how many lines the quote wraps to, and measuring before paint avoids a visible jump. Callers that pass no geometry keep the old bottom-pinned behaviour. The existing selection test could not catch this: it always selects the FIRST paragraph, which sits far from the bottom. Added a test that selects the lowest fully-visible paragraph, asserts the fixture actually put it in the danger band (so it cannot pass for the wrong reason), and asserts no overlap. Verified discriminating — disabling the flip fails it with 'action bar overlaps the selected text'. Audits 12/12 across both viewports; web suite 3446/0; typecheck and lint clean. Unrelated pre-existing finding: tests/playwright/chat-dom-bound-audit.spec.ts is fully red on main as well (0 passed / 6 failed locally, .sam-message-entry never renders). Not caused by this branch — verified by running it on main — and tracked separately rather than fixed here. --------- Co-authored-by: Raphaël Titsworth-Morin <raphael@raphaeltm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of library file commenting: users can comment on markdown files in the project library, including selecting text in the rendered preview and commenting on that selection. Replies, resolve, and reopen work; comments persist per file and are project-scoped rather than session-scoped. MCP tools let agents list and create file comment threads.
Critical implementation note for reviewers — the storage model changed after review. The first cut widened the existing
comment_threadstable (DO migration 032) to accept a second anchor kind. That required relaxingCHECK (anchor_kind = 'message')and droppingNOT NULLonsession_id/message_id, which SQLite cannot do in place — so it recreated three tables via backup → drop → restore on a Durable Object, where there is no time-travel recovery. Worse, nullablesession_idmadegetCommentThread(sql, sessionId, threadId)stop type-checking, so the scope parameter was deleted and reply/resolve/reopen silently stopped enforcing session ownership.The shipped design uses separate tables: migration 033 additively creates
library_file_comment_threads/_replies/_status_mutations. Message comment storage and code are byte-identical tomainagain, so session isolation holds by construction. The two anchor kinds are unified at the type layer (CommentAnchor), not in storage.Scope per Raphaël (2026-08-22): send-to-agent and agent-authored comments are Phase 2; markdown first, but the tables and types extend to other file types additively.
Validation
pnpm lint— 0 errors (6 pre-existing warnings inacp-clientandweb)pnpm typecheck— clean across all packagespnpm test— API 7985 passed / 0 failed / 0 collection errors (594 files); web 3440 passed / 0 failed / 0 collection errors (287 files). Counts reconciled via the JSON reporter, not a piped tail: web was 3430 without the new test file, +10 added.pnpm check:fastexit 0;pnpm quality:do-migration-safety,pnpm quality:migration-safety,pnpm quality:wrangler-bindingsall exit 0. Playwright audit 8/8 at 375x667 and 1280x800.Both new guards were verified discriminating: deleting
AND file_id = ?from the DO lookup reds exactly the three file-scoping tests; deletingprojectIdfrom the file/project binding predicate reds exactly the two binding tests. Both restored after verification.Staging Verification (REQUIRED for all code changes — merge-blocking)
sam/library-file-commenting@b512fad5f(the exact branch tip), conclusion success. Re-deployed and re-verified after the deduplication commit rather than relying on the earlier pass againstfb92665d6.app.sammy.partyviaPOST https://api.sammy.party/api/auth/token-loginwithSAM_PLAYWRIGHT_PRIMARY_USER, session reused fromstorageState(token-login is rate limited per source IP)packages/cloud-init/,packages/vm-agent/, DNS, TLS, orscripts/deploy/is touched.Staging Verification Evidence
Driven as a real user in a real browser against
app.sammy.party, on the deployed branch tip. 20/20 checks passed — run twice, againstfb92665d6and again againstb512fad5fafter the deduplication commit, with identical results.Project
01KTKXZ4ZZAT6MJFXRW1ZTQ7RB(hono), fileauth-explainer.md:POST .../commentsdata-comment-anchorPOSTcarries the selected textquote="A JWT (JSON Web Token) is a compact, signed token that encod…"POST .../repliesPOST .../resolvePOST .../reopenGETreturns 2 threads)Screenshot evidence in
.tmp/staging-shots/—07-quoted-posted.pngshows the posted comment rendering the selected paragraph as a blockquote above the body, which is the whole point of the feature.A first pass against
getting-started.mdsurfaced no selection affordance; that file is 22 bytes (a bare heading) with no selectable text block, not a defect. Re-run against a file with real prose passed every check.Cleanup: the three threads created during verification were resolved via the API afterwards. No workspaces or nodes were provisioned by this verification, so there is no VM capacity to release.
UI Compliance Checklist (Required for UI changes)
role="alert"; the selection affordance is a real button with an accessible name at both pointer types; the composer keeps itssr-onlylabelCommentThread,CommentThreadList,CommentComposer,CommentPrimitives,useCommentSelection,Popoverapps/web/tests/playwright/library-file-comments-audit.spec.ts, 4 scenarios x 2 viewports: empty, populated (long body, long quote, unbreakable URL, collapsed resolved thread), load error, and the full select → comment-on-selection → post flow.assertNoOverflowat every capture. 12 screenshots in.codex/tmp/playwright-screenshots/, all reviewed and all distinct in size.The audit caught a real defect: the desktop selection popover rendered at
z-dropdown(20) inside a modal atz-dialog-backdrop(50), so the modal's own content intercepted the click and a desktop user could not start a quoted comment at all.Popovergained an explicitlayerClassName(default unchanged for every existing caller) andSelectionPopoveropts intoz-panel, matching its coarse-pointer twinSelectionActionBar.End-to-End Verification (Required for multi-component changes)
Data Flow Trace
User selects text in the rendered markdown preview
→
apps/web/src/components/library/FilePreviewModal.tsx(data-comment-anchor={file.id}on the render container)→
apps/web/src/components/project-message-view/comments/useCommentSelection.ts:useCommentSelection()User clicks "Comment on selection"
→
FilePreviewModal.tsx:startQuotedComment()→setPendingQuote+setCommentsOpen(true)→
apps/web/src/components/library/FileCommentPanel.tsxrendersCommentComposerwithquote={pendingQuote}User submits
→
FileCommentPanel.tsx:handleCreateThread()→
apps/web/src/components/library/useLibraryFileComments.ts:createThreadMutation→
apps/web/src/lib/api/comments.ts:createLibraryFileCommentThread()→
POST /api/projects/:projectId/library/:fileId/commentsAPI authorizes and binds the file to the project
→
apps/api/src/routes/library-comments.ts→requireProjectCapability(db, projectId, userId, 'task:write')→
apps/api/src/services/library-file-comments.ts:assertLibraryFileInProject()(D1project_files, scoped on BOTHprojectIdandid)Service crosses the Durable Object boundary
→
apps/api/src/services/project-data.ts:createFileCommentThread()→stub.createFileCommentThread(input)→
apps/api/src/durable-objects/project-data/index.ts:createFileCommentThread()DO writes to file-scoped storage
→
apps/api/src/durable-objects/project-data/library-file-comments.ts:createFileCommentThread()→
INSERT INTO library_file_comment_threads(DO migration033-library-file-comment-threads)Response returns and the cache retires the optimistic row
→
apps/web/src/lib/query-options/comments.ts:upsertLibraryFileCommentThread()(matches on server id ORclientId)→ thread renders in
CommentThreadListReads/mutations of an existing thread re-enter at step 5 and are scoped
WHERE id = ? AND file_id = ?inlibrary-file-comments.ts, which is the ownership guard.Untested Gaps
None between the automated suite and the user flow:
apps/api/tests/integration/library-file-comments-vertical-slice.test.tsdrives the real HTTP routes over real DO SQLite with real migrations across two projects, and the Playwright audit drives the browser flow end to end at both viewports. The one deliberate simulation is the Durable Object RPC hop, reproduced at production fidelity (a plainErrorcarrying onlynameandmessage) — this is what proved the route error mapping must not rely oninstanceofor the classcodefield.Post-Mortem (Required for bug fix PRs)
What broke
The feature was implemented, self-reviewed as complete, and archived while carrying three defects: reply/resolve/reopen no longer enforced session ownership on message comments (any project collaborator could mutate another session's threads by id); the DO migration performed an unrecoverable table drop; and quote selection — the headline capability — was never built, so a user could not do the thing that prompted the request.
Root cause
One design decision cascaded: reuse
comment_threadsfor a second anchor kind (commit 160136d). Widening the table forced nullablesession_id; nullablesession_idforced the shared getter to drop its scope parameter; dropping it removed an authorization predicate. Each step read as a mechanical consequence of the previous one, so none was re-evaluated as a decision. SQLite's inability to relaxNOT NULLin place is what forced the destructive migration.Class of bug
A schema relaxation that removes an authorization predicate as a side effect. Making a scoping column nullable so a second row kind can share a table means every query that scoped on it must now treat it as optional — and the compiler is satisfied by simply deleting the parameter. Nothing is removed, no type breaks, no test goes red.
Why it wasn't caught
The plan committed to the shared-table design during research and never revisited it, so implementation inherited a premise rather than testing it. There were no HTTP route tests and no vertical-slice test, so the regression had nowhere to surface. The seven specialist reviewers did catch all of it — the process failure was archiving the task before their findings were addressed.
Process fix included in this PR
.claude/rules/63-widening-a-table-can-delete-an-auth-check.md(new)Post-mortem file
tasks/active/2026-08-22-library-file-commenting.md— "Specialist Review and Rework" and "Post-Mortem" sections.Specialist Review Evidence (Required for agent-authored PRs)
needs-human-reviewlabel added and merge deferred to humankind: 'message'anchor at the UI boundary, duplicatedverifyFileExists, duplicated create paths. Fixed in 67f700e, 40faa82verifyFileExistson reply/resolve/reopen; full-list refetch on every mutation. Fixed in 40faa82, 236643eExceptions (If any)
Agent Preflight (Required)
Classification
External References
Official documentation consulted: Cloudflare Durable Objects SQL storage semantics (no point-in-time recovery, which is why migration 033 is additive) and SQLite
ALTER TABLElimitations (CHECKconstraints andNOT NULLcannot be altered in place). No external API surface changed, so no Context7 lookup was required.Codebase Impact Analysis
apps/api/src/durable-objects/migrations.ts— additive migration 033apps/api/src/durable-objects/project-data/library-file-comments.ts— new file-scoped DO moduleapps/api/src/durable-objects/project-data/comment-normalization.ts— extracted shared validationapps/api/src/durable-objects/project-data/comments.ts,comment-contracts.ts,index.ts— message paths restored tomain, extended additivelyapps/api/src/lib/comment-http.ts— extracted shared HTTP error mapping and query parsingapps/api/src/routes/library-comments.ts,apps/api/src/routes/chat-comments.ts— routes now share one implementationapps/api/src/routes/mcp/library-file-comment-tools.ts,tool-definitions-library-file-comment-tools.ts— MCP toolsapps/api/src/services/library-file-comments.ts,apps/api/src/services/project-data.ts,apps/api/src/services/message-comments.tsapps/web/src/components/library/—FileCommentPanel.tsx,useLibraryFileComments.ts,FilePreviewModal.tsxapps/web/src/components/project-message-view/comments/—comment-utils.ts,CommentThread.tsx,CommentComposer.tsx,CommentPrimitives.tsxapps/web/src/lib/api/comments.ts,apps/web/src/lib/query-options/comments.tspackages/shared/src/types/comments.ts,packages/shared/src/types/index.tspackages/ui/src/components/Popover.tsx— new optionallayerClassName.claude/rules/63-widening-a-table-can-delete-an-auth-check.md,tasks/active/2026-08-22-library-file-commenting.mdDocumentation & Specs
N/A: no public docs site content changed.The feature is an in-app UI affordance with no self-hosting, configuration, or architecture implication. Agent-facing documentation is the new rule 63 and the task record.Constitution & Risk Check
COMMENT_*env vars via the sharedresolveCommentLimits; the per-file thread cap reusesCOMMENT_THREADS_PER_SESSION_MAX.fileIdis rejected rather than creating an unreachable thread; the file/project binding returns the same non-disclosing 404 for "missing" and "belongs to another project".CREATE TABLE IF NOT EXISTSonly; no drop, noALTERof existing tables;quality:do-migration-safetypasses.mainversions verbatim rather than editing them, so the diff for message comments is limited to the extracted shared helpers.