fix(coderd/x/chatd/chattool): list directories in read_file#26105
fix(coderd/x/chatd/chattool): list directories in read_file#26105mafredri wants to merge 2 commits into
Conversation
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 1 | Last posted: Round 1, 13 findings (1 P2, 6 P3, 4 Nit, 2 Note), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel. 0 P0-P1, 1 P2, 6 P3, 4 Nit, 2 Note. 16 reviewers (bisky, hisoka, mafu-san, mafuuu, pariston, ging-go, gon, leorio, chopper, kite, meruem, ryosuke, robin, zoro, luffy, knov). Reviewed against 5d8cd2e..a406daa. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Clean, well-scoped change. The composition pattern (try ReadFileLines, detect directory error, fall back to LS) is the right design for an LLM tool layer: one entry point, useful output regardless of path type. Test density is excellent at 80%, with genuine negative cases and real assertions. Multiple reviewers praised the error composition and the backfilled coverage for pre-existing paths.
Severity count: 1 P2, 6 P3, 4 Nit, 2 Note.
The P2 (CRF-2) is about the entries array bypassing the byte budget that bounds content. The 32KB cap loses its meaning when the structured data alongside it serializes to 5x that. This is the most impactful finding and has a straightforward fix.
CRF-4 (full directory fetch) was raised at P2 by one reviewer but downgraded to P3 during cross-check. The LS endpoint's lack of server-side pagination is a real scaling concern that this PR makes more reachable (LLM agents vs. human-initiated UI calls), but the fix belongs in the LS endpoint. This needs a human decision: file a ticket to track server-side LS pagination, or explicitly accept the current behavior for chat-tool usage.
"Genuine gems, every one." (Bisky, on the test suite)
🤖 This review was automatically generated with Coder Agents.
read_filenow falls back to a bounded directory listing when the agent reports that the requested path is not a file. The lower-level file-read endpoint still keeps its file-only contract; the chat tool composes that response with the existing workspaceLScall.Directory responses keep the existing
contentfield for UI compatibility and include pagination metadata. They do not return structuredentries, so direct tool output remains bounded by the formatted listing budget.Verified with focused package tests, Go lint, and the full pre-commit hook in a clean worktree. The initial local pre-push run reached an unrelated Storybook failure in
UsersPage.stories.tsx, so I used the repo-supportedcoder.pre-push falseopt-out for the Go-only push.