feat(coderd/x/chatd): agent-created file attachments in chat#24280
feat(coderd/x/chatd): agent-created file attachments in chat#24280ethanndickson merged 1 commit intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ethanndickson
changed the title
Documentation CheckUpdates Needed
Automated review via Coder Tasks |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba37790e60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f84c3c3a74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b181308ae0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ethanndickson
force-pushed
the
ethan/chat-file-attachments
branch
from
b181308 to
09619fc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09619fc6d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ethanndickson
force-pushed
the
ethan/chat-file-attachments
branch
from
09619fc to
f2f766d
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ethanndickson
force-pushed
the
ethan/chat-file-attachments
branch
from
2429f26 to
0c202b2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c202b25ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ethanndickson
force-pushed
the
ethan/chat-file-attachments
branch
from
0c202b2 to
b865dc9
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
coder/coderd/x/chatd/chattool/proposeplan.go
Lines 101 to 103 in b865dc9
Attach propose_plan output via response metadata
buildAssistantPartsForPersist only promotes attachments from ToolResultContent.ClientMetadata (AttachmentPartsFromMetadata), but propose_plan returns a plain JSON body and never calls WithAttachments. When an agent runs propose_plan, the stored file ID is not converted into a persisted type:"file" assistant part, so the plan artifact is missing from the new attachment pipeline even though the file was stored successfully.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Solid plumbing for durable chat attachments. The storage transaction pattern in storeChatAttachment (insert + link in one tx) is the right shape, and the MIME classification pipeline is well-tested with a ~1.2:1 test-to-code ratio. The SVG-blocking intent is good. The subagent artifact isolation tests (TestWaitAgentDoesNotRelay*) prove a real security property.
3 P2, 7 P3, 2 Nit, 2 Note. The P2s are: (1) HasSVGRootElement false-positives on any text file mentioning <svg>, (2) unbounded io.ReadAll on agent response in storeWorkspaceAttachment, and (3) recording files orphaned when parent chat linking fails.
Convergent finding: four reviewers independently flagged propose_plan bypassing ClassifyStoredMediaType. Note the interaction with F4: routing propose_plan through ClassifyStoredMediaType today would trigger the same false-positive SVG blocking on legitimate Markdown plans that mention SVG. Fix F4 first, then F7 becomes safe to unify.
"A file at a .md path whose content begins with <svg> would be stored as text/markdown without triggering HasSVGRootElement. The nosniff header prevents browser reinterpretation, so there is no exploitable XSS. But the promise 'SVG content is blocked before storage' does not hold on this path." (Mafuuu, on the perimeter inconsistency)
🤖 This review was automatically generated with Coder Agents.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
🤖
Solid extraction of MIME policy into coderd/chatfiles. The storeChatAttachment transaction pattern is the right shape, the defense-in-depth layering on uploads is well done, and the assistant-attachment-not-replayed-to-LLM design is correct.
2 P2, 1 P3, 1 Nit across 4 inline comments. The P2s were independently validated with test cases.
ethanndickson
force-pushed
the
ethan/chat-file-attachments
branch
from
11095b3 to
6c530e8
Compare
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 2. All 17 R1 findings verified fixed by the panel (6 reviewers, unanimous). The fixes are thorough: the SVG parser now rejects non-whitespace CharData before the root element, io.LimitReader caps the agent response, recording artifacts use transactional insert+link, propose_plan routes through classification, PDFs are download-only, and the duplicate maps are consolidated. The regression tests are substantive.
5 new P3, 1 Nit, 1 Note. The primary convergent finding: storeRecordingArtifact is the sole remaining path that bypasses the chatfiles classification pipeline this PR establishes. Four reviewers flagged it independently.
"Two trust tiers for the same chat_files table: user uploads are byte-verified and allowlist-gated; recording artifacts trust the caller's label entirely." (Hisoka, on the recording bypass)
🤖 This review was automatically generated with Coder Agents.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 3. All R2 fixes verified by the panel (4 reviewers). The recording classification fix (PrepareRecordingArtifact with byte-level type verification), screenshot streaming decoder, and test hardening all landed correctly.
F29 (per-entry attachment rejection) closed by panel vote (4/4 accept). All four reviewers verified that current producers emit exactly one attachment per tool result, and the per-result degradation in buildAssistantPartsForPersist is sufficient for this contract.
F31 (control characters in filenames) acknowledged by author but no ticket linked. The deferral promise without a ticket is a drop, not a deferral. This needs a human decision.
2 new P3, 1 Nit. The remaining open findings are minor: a missing integration-level test for the content-validation gate in storeChatAttachment, recording logs that lack entity IDs for production triage, and a stale comment referencing a deleted constant.
"If someone refactored storeChatAttachment and dropped or moved the PrepareStoredFile call, no test would break." (Bisky, on the missing rejection test)
coderd/exp_chats_test.go:6881
Nit Comment references deleted constant maxChatFileName. Update to chatfiles.MaxStoredFileNameBytes or just "255 bytes". (Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
🤖
All 4 findings from our R1 review are confirmed fixed. The SVG parser now correctly rejects non-whitespace CharData, recordings use transactional insert+link, the duplicate maps are consolidated (with PDF now download-only), and the suggested test cases were added.
The R2 fixes also look solid — PrepareRecordingArtifact byte-verifies recordings against expected types, storeLinkedChatFileTx is shared across both paths, and io.LimitReader caps the agent/base64 reads.
Fresh review found 1 P3 (convergent, 3 reviewers) and 1 test coverage gap across 2 inline comments. No blockers.
ethanndickson
force-pushed
the
ethan/chat-file-attachments
branch
from
deeba77 to
0e4bde5
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 4 blocked. F33 and F34 were addressed in 0e4bde5 (PrepareStoredFile rejection test and recording log identifiers). F31 was also addressed in the same commit (control character stripping in NormalizeStoredFileName).
F32 (Nit) remains silent: the comment at coderd/exp_chats_test.go:6894 still references the deleted constant maxChatFileName instead of chatfiles.MaxStoredFileNameBytes. This was folded into the R3 review body because it was outside the diff. Further review is blocked until the author responds or pushes a fix for this finding.
🤖 This review was automatically generated with Coder Agents.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 5. All prior findings resolved. The panel (4 reviewers) verified the R3/R4 fixes: PrepareStoredFile rejection test, recording log identifiers, control character stripping, stale comment update. Hisoka probed 10 distinct attack/edge surfaces and came up empty. The code is in strong shape after 5 rounds and 36 findings tracked across 4 panel reviews.
1 Nit, 1 Note remaining. Both are minor housekeeping.
"Boring code gets silence. This earned it." (Hisoka, after probing upload body ordering, double limits, peak memory, PDF serving, recording artifacts, XML/SVG classification, upload compatibility, cap enforcement, assistant replay, and UTF-8 truncation)
🤖 This review was automatically generated with Coder Agents.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Round 6. All 38 findings across 6 rounds are resolved. Four reviewers found nothing new. Hisoka probed concurrent cap races, multi-byte truncation boundaries, XML passthrough classification, and recording rollback paths with no findings.
This PR tracked 3 P2s, 12 P3s, 4 Nits, 4 Notes, and 1 panel-closed contested finding over 5 panel reviews. Every finding was addressed by the author within one round. The coderd/chatfiles package is a clean extraction with consistent validation contracts, transactional storage, and thorough test coverage (~1.5:1 test-to-code ratio).
"Six rounds in. Every thread I pulled held." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
ethanndickson
force-pushed
the
ethan/chat-file-attachments
branch
from
87b1c7e to
83d9c9b
Compare
|
@mafredri agent is happy :) |
ethanndickson
force-pushed
the
ethan/chat-file-attachments
branch
from
83d9c9b to
cf835bc
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| func insertLinkedChatFile( | ||
| ctx context.Context, | ||
| t *testing.T, | ||
| db database.Store, | ||
| chatID uuid.UUID, | ||
| ownerID uuid.UUID, | ||
| organizationID uuid.UUID, | ||
| name string, | ||
| mediaType string, | ||
| data []byte, | ||
| ) uuid.UUID { | ||
| t.Helper() | ||
|
|
||
| file, err := db.InsertChatFile(ctx, database.InsertChatFileParams{ | ||
| OwnerID: ownerID, | ||
| OrganizationID: organizationID, | ||
| Name: name, | ||
| Mimetype: mediaType, | ||
| Data: data, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| rejected, err := db.LinkChatFiles(ctx, database.LinkChatFilesParams{ | ||
| ChatID: chatID, | ||
| MaxFileLinks: int32(codersdk.MaxChatFileIDs), | ||
| FileIds: []uuid.UUID{file.ID}, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.Zero(t, rejected) | ||
|
|
||
| return file.ID | ||
| } |
There was a problem hiding this comment.
reminder for self: make dbfake helpers for this in #24497
4 participants
Agents can already see workspace files and take screenshots, but users could not download those artifacts from chat. This PR adds durable chat attachments to chatd.
attach_file, explicitcomputerscreenshot actions (not the automatic post-action screenshots), andpropose_plannow fetch bytes over the agent connection, store them inchat_files, link them to the chat, and carry attachment metadata in tool responses sobuildAssistantPartsForPersistcan materialize ordinarytype:"file"assistant parts that the chat file APIs serve.The same storage helpers are reused for other artifact-producing paths.
wait_agentrecordings and thumbnails are stored as chat files and linked back to the parent chat, with best-effort relinking so parent chats retain those artifacts without leaving orphaned rows when chat-file caps reject links.storeChatAttachmentwraps insert + link in one transaction, files are capped at 10 MB each and 20 per chat, and serving defaults toContent-Disposition: attachmentwith an explicit inline-safe allowlist.This PR also consolidates chat-file media policy in
coderd/chatfiles. Uploads and tool-generated attachments share byte-based MIME detection, SVG blocking, inline-safety rules, and compatibletext/plainrefinement for JSON, CSV, and Markdown. Prompt construction still only inlines synthetic pasted text for model consumption; assistant-created attachments are persisted for the user and intentionally not replayed into later LLM turns.UI follow-up lives in #24281.
Relates to CODAGT-91