Conversation
|
/coder-agents-review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ 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". |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review (Netero only). The full review panel has not yet reviewed this PR; these are mechanical findings to address before the panel spends parallel review time.
The display-only attach_file fallback is a clean design: unsupported files get shown to the user with inline playback, and the bytes are stripped before provider requests. The UnsupportedAttachmentTypeError subclass with instanceof gating, the inner catch fallback, and the Zod schema union are all well-structured.
1 P2, 2 P3, 1 Nit.
"base64 of a non-empty buffer is always non-empty, so displayFile.data.length > 0 is always true when the function returns. The assert on line 64 never fires." (Netero, on the redundant guard)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
@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". |
There was a problem hiding this comment.
Panel review (12 reviewers + Netero). All five R1 findings (DEREM-1 through DEREM-5) verified fixed; clean work.
The display-only fallback design is sound: the UnsupportedAttachmentTypeError gating, the shared displayOnlyFileParts.ts module, the request-time stripping, and the Zod schema union are well-structured. The fix earns its complexity. Pariston tried to build a case against the approach and could not: "the problem is correctly understood, the display-only result + request-time stripping is proportional to it."
1 P2, 9 P3, 5 Nit, 2 Note.
The P2 (DEREM-9) is a type-safety gap: DisplayOnlyFilePart reuses the AI SDK's own file-data type, so the only barrier between 10MB of video bytes and the provider is remembering to call extractAttachmentsFromToolOutput. The defense is procedural, not structural.
DEREM-12 deserves explicit attention: adding txt, csv, json to the display-only map silently changes the attach_file contract for text files. Before this PR, attach_file("config.json") errored and directed the model to file_read. Now it "succeeds" with a display-only card. This needs a human decision.
"Any new code path that constructs provider messages without routing through extractAttachmentsFromToolOutput would silently forward up to 10 MB of video or audio to the LLM provider, despite the UI displaying 'not sent to the model as a file attachment.'" (Mafuuu, on the type-safety gap)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
@codex review |
|
/coder-agents-review |
|
@codex review |
There was a problem hiding this comment.
17 of 18 R2 findings addressed in 2257aca. Excellent turnaround, including the discriminated union refactor (DEREM-15) and the display_file type rename (DEREM-9).
Further review is blocked until one remaining finding is acknowledged:
DEREM-25 (Note, toolResultAttachments.ts:105): The model receives two overlapping text notices for every display-only file. The original tool handler text and the rewriter's byte-replacement placeholder both reach the provider. This is informational, not a bug. A brief acknowledgment or "accepted" is sufficient to unblock.
🤖 This review was automatically generated with Coder Agents.
This comment has been minimized.
This comment has been minimized.
|
Acknowledged DEREM-25: accepted. The duplicate model-visible notices are intentional for now: the tool result records the user-visible outcome and the request-time replacement documents that bytes were stripped before provider submission. |
|
/coder-agents-review |
|
@codex review |
This comment has been minimized.
This comment has been minimized.
|
Addressed Codex source-file fallback feedback in fdfc06e: unknown extensions now use the octet-stream display fallback only after binary-content sniffing, and common source/text files (including extensionless text like Makefile) continue to reject so the model can use file_read. |
|
/coder-agents-review |
|
@codex review |
This comment has been minimized.
This comment has been minimized.
|
Addressed the Codex exact-prose test feedback in 86bd853: the attach_file test now asserts behavioral text properties instead of pinning the full notice copy. |
|
/coder-agents-review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ 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". |
|
/coder-agents-review DEREM-25 has been acknowledged as accepted in #3242 (comment) and again in the resolved DEREM-25 thread. Please re-check for any remaining open issues. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ 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". |
|
/coder-agents-review |
1 similar comment
|
/coder-agents-review |
There was a problem hiding this comment.
Combined R4/R5 review. The R4 panel review (7 reviewers + Netero) could not be posted due to persistent GitHub API 422 errors (the PR head advanced while retrying, invalidating diff positions). These findings are from R4's panel and are posted here for the first time.
All prior fixes verified (DEREM-1 through DEREM-25). DEREM-26 (P2, missing text extensions) addressed in fdfc06e with expanded TEXT_EXTENSIONS_REQUIRING_FILE_READ and binary-content sniffing. CI is green.
1 P3, 3 Nit, 1 Note remaining.
DEREM-27 is the only substantive open finding: three R4 reviewers independently proposed the same defensive fallback for malformed display_file parts. The Nits and Note are cleanup items.
Pariston (R4): "I tried to build a case against this and could not. The problem is correctly understood. The solution is proportional."
🤖 This review was automatically generated with Coder Agents.
| continue; | ||
| } | ||
|
|
||
| if (isDisplayOnlyFilePart(item)) { |
There was a problem hiding this comment.
P3 [DEREM-27] isDisplayOnlyFilePart guard requires full metadata validity (providerOptions.mux.displayOnly === true and typeof size === "number"). A display_file part from chat.jsonl with corrupted or schema-drifted providerOptions fails the guard and falls through to newValue.push(item) with up to ~13MB of base64.
Three R4 reviewers converged on the same fix: after the isDisplayOnlyFilePart block, add:
const itemRecord = item as Record<string, unknown>;
if (itemRecord.type === "display_file" && typeof itemRecord.data === "string") {
didChange = true;
newValue.push({ type: "text", text: "[Display-only file removed before provider request.]" });
continue;
}(Mafuuu P3, Meruem P3, Kite P3)
🤖
| // chat history never persists unexpectedly large base64 payloads. | ||
| export const MAX_ATTACH_FILE_SIZE_BYTES = 10 * 1024 * 1024; // 10MB | ||
|
|
||
| class UnsupportedAttachmentTypeError extends Error { |
There was a problem hiding this comment.
Nit [DEREM-28] UnsupportedAttachmentTypeError is vestigial after the DEREM-15 discriminated union fix. No code checks instanceof; the class provides no behavioral differentiation. Replace with plain Error in createUnsupportedAttachmentError.
(Meruem)
🤖
| function filterResultForDisplay(result: unknown): unknown { | ||
| if (!isToolContentResult(result)) { | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Nit [DEREM-29] filterResultForDisplay duplicates the type === "media" stripping from GenericToolCall.tsx:36. A shared utility would eliminate the copy.
(Zoro)
🤖
| } | ||
|
|
||
| return { | ||
| type: "display", |
There was a problem hiding this comment.
Nit [DEREM-30] readAttachmentFromPath wrapper calls resolvePathWithinCwd redundantly; readAttachFileFromPath already resolved the path and result.file.resolvedPath is available.
(Zoro)
🤖
| }; | ||
| } | ||
|
|
||
| export function isDisplayOnlyFilePart(value: unknown): value is DisplayOnlyFilePart { |
There was a problem hiding this comment.
Note [DEREM-31] isDisplayOnlyFilePart rejection path has no direct unit tests. Tested indirectly via stripping tests; risk is low. Worth covering cheaply.
(Bisky)
🤖
There was a problem hiding this comment.
BLOCKED. The 5 open findings from R4 (posted in R5 after GitHub API recovery) have no author response yet. Further review is blocked until the author responds or pushes fixes.
Open findings:
- DEREM-27 (P3)
toolResultAttachments.ts:146: Defensive fallback for malformeddisplay_fileparts. - DEREM-28 (Nit)
readAttachmentFromPath.ts:20: VestigialUnsupportedAttachmentTypeError. - DEREM-29 (Nit)
AttachFileToolCall.tsx:55:filterResultForDisplayduplication. - DEREM-30 (Nit)
readAttachmentFromPath.ts:297: Redundant path resolution. - DEREM-31 (Note)
displayOnlyFileParts.ts:55:isDisplayOnlyFilePartrejection untested.
For the 3 Nits and 1 Note, a brief acknowledgment ("accepted" / "will fix" / "deferred") is sufficient.
🤖 This review was automatically generated with Coder Agents.
1 participant
Summary
Shows unsupported
attach_fileoutputs (for example.webm) in the chat UI instead of failing the tool outright, while preventing those unsupported file bytes from being sent to providers as model attachments.Background
attach_filepreviously returned an error when a readable file was not one of the model-supported attachment types. This made files such as videos invisible to the user even though Mux could still display them usefully in the transcript.Implementation
Unsupported readable files now fall back to a display-only
file-dataresult tagged withproviderOptions.mux.displayOnly. The attach-file tool renderer shows display-only files, including inline video/audio playback when the browser supports the media type, and request-time tool-result rewriting strips the bytes before provider calls while leaving a textual notice for the model.Validation
make static-checkmake typecheck && make fmt-check && make lintattach_filesmoke test for.webmfallback and provider-rewrite byte strippingsharpbecauselibstdc++.so.6is missing.Risks
Low-to-moderate risk in attach-file rendering and request rewriting paths. Supported attachments keep the existing media path; unsupported display-only files are explicitly marked and stripped before provider requests.
Generated with
mux• Model:openai:gpt-5.5• Thinking:high• Cost:$14.15