feat: track chat file associations with chat_file_links on chats - #23537

read with mine own two eyes and deemed acceptable

Pull request overview

Backend-only change to persist and expose chat↔file associations so the Agents UI can later render a “Files” tab.

Changes:

• Add chats.file_ids uuid[] with migration + dump updates, and plumb it through generated DB models/queries.
• Add DB queries to append file IDs with deduplication and to fetch lightweight file metadata by ID.
• Link referenced file IDs on message creation/edit and tool-created file insert; hydrate Chat.files on single-chat GET.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Clean approach to linking files with chats. The file_ids UUID[] column mirrors the existing mcp_server_ids pattern, the SQL dedup via unnest + array_agg(DISTINCT) is solid, and the dbauthz wiring is correct.

1 P1, 2 P2, 3 P3, 2 Obs, 4 nits across 12 inline comments.

Meruem: "convertChatFileMetadata maps four of six fields and skips two. Every GET /chats/{chat} response with files will include owner_id: 00000000-... for each file."

Hisoka: "Three handlers call appendChatFileIDs. All three only append. No code path ever removes a UUID from chats.file_ids. What happens when someone uploads twenty images across ten edit cycles? Forty files in the panel, half of them ghosts."

🤖 This review was generated with the help of Coder Agents.

Round 2 review. The prior round's findings were substantially addressed: OwnerID/OrganizationID mapping, behavioral tests, stale response hydration, docs accuracy, input-part re-scan, JSON tag consistency, and query comments are all resolved. Good work.

Remaining: 4x P2, 4x P3, 3x Note. One P2 is re-raised from a deferral with no tracking ticket.

"The interest rate: an active AI coding session using propose_plan appends a file per tool call. Twenty iterations means twenty UUIDs. That is harmless today. A hundred sessions on the same chat thread means the array approaches sizes where the unnest-reaggregate UPDATE starts showing up in query latency, and the per-row overhead in list queries stops being free. There is no cap on the array, no dbpurge job, and no ticket tracking one." -- Knuckle

🤖 This review was automatically generated with Coder Agents.

Round 3. The core feature is solid: the migration is safe, the SQL dedup works, db2sdk conversion maps all fields, and tests now cover the critical paths. The round 1/2 fixes addressed most findings. This round's six reviewers converged on the cap enforcement logic.

2x P2, 1x P3, 2x Note. Five still-open nits/notes from round 1.

Mafuuu: "Once a chat reaches MaxChatFileIDs, no message or edit can reference any file at all, even ones already linked. The chat becomes file-hostile at its own cap."

Knuckle's center-of-gravity recommendation: move the cap enforcement into the SQL AppendChatFileIDs query itself (e.g. WHERE array_length(file_ids, 1) < 20), which would eliminate the double-counting bug, the TOCTOU race, and the snapshot divergence in one structural change. The SQL already does the dedup. Let it also do the cap.

5 unresolved nits/notes from round 1 with no author response: #comment-2993904968 (DISTINCT comment), #comment-2993904974 (metadata query comment), #comment-3008493046 (updated_at bump on no-op), #comment-3008493049 (pubsub omits files), #comment-2993904980 (two independent linking paths).

🤖 This review was automatically generated with Coder Agents.

Round 4. The latest commit is a strong structural improvement. Moving cap enforcement into SQL via AppendChatFileIDs :execrows with a WHERE array_length(...) <= @max_file_ids guard eliminates four findings from round 3 in one change: the double-counting bug, the TOCTOU race, the snapshot divergence, and the error-classification problem. The conditional updated_at bump addresses a round-1 nit. Best-effort linking with warnings is a clean contract.

1x P2, 1x P3, 2x Note. No blockers.

Knuckle: "The CTE subquery reads a stale snapshot under concurrent access, losing file IDs. One-line fix: FOR UPDATE." Mafuuu independently flagged the same CTE concurrency gap.

3 unresolved nits from round 1 with no author response: #comment-2993904968 (DISTINCT comment), #comment-2993904974 (metadata query comment), #comment-2993904980 (two independent linking paths).

🤖 This review was automatically generated with Coder Agents.

self-re-reviewed

Round 5. All prior findings addressed. No new blockers.

The FOR UPDATE in the CTE serializes concurrent appends correctly. linkFilesToChat now distinguishes DB errors from cap exceeded with accurate user-facing messages. Test coverage for warning paths is solid: FileCapExceeded, FileCapOnCreate, DedupFileIDs, and ToolCreatedFilesLinked all verify the right behavior at boundaries. The round-1 nits (DISTINCT comment, metadata query comment, pubsub comment) have replies and code changes.

One design-level observation (not a blocker): AppendChatFileIDs applies all-or-nothing batch semantics. Sending 21 files when the cap is 20 links 0, not 20. The FileCapOnCreate test documents this explicitly. If partial linking is desired later, the SQL would need per-element insertion or a different CTE structure.

The deferred append-only/no-pruning issue (coder/internal#1434) and the two-independent-paths observation remain as known design debt.

Mafuuu, on the batch semantics: "A user one file over the limit gets nothing linked instead of getting the maximum possible. This is a user-visible rough edge that the future Files tab will surface."

🤖 This review was automatically generated with Coder Agents.

Round 5 self-review (post-merge with main). All prior-round findings are resolved or acknowledged. The merge was clean after conflict resolution and make gen/fmt/lint pass. 8 reviewers, 0 new blockers.

1 P2, 2 P3, 1 Nit across 4 inline comments. Two observations noted inline.

🤖 This review was automatically generated with Coder Agents.

Round 6 self-review (post join-table refactor). 5 reviewers, focused on the database refactor and cross-checking against all prior mafredri comments.

Fixed: P1 migration missing FK constraints, P2 migration number collision with main, P3 collapsed nolint directive, stale comment. Pushed as afb9be4ac.

1 accepted tradeoff noted below.

🤖 This review was automatically generated with Coder Agents.