docs: remove unused paused and completed chat statuses, add requires_action#24264
Conversation
…tion The paused and completed chat statuses are defined in the enum but never set by any production code path. The agent transitions between waiting, pending, running, error, and requires_action only. Also adds the missing requires_action status which is actively used when dynamic tool calls are pending.
mafredri
left a comment
There was a problem hiding this comment.
Clean docs fix that aligns the documented chat statuses with what production code actually writes. Good analysis in the PR description tracing every UpdateChatStatus call site.
One nit, one note. No blocking findings.
"The byte sequence e2 80 94 at 'Idle --- newly created' is a Unicode em-dash." -- Netero, squinting at hex dumps
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Panel review, round 2. One P3, two P4s (outside diff). No blocking findings.
The PR description's call-site audit is thorough and verifiable. The five documented statuses match the exhaustive set of statuses written by production code. Good change.
Correction on round 1 findings:
- The em-dash nit (#comment-3066723267) was wrong. There is no project convention against em-dashes. The
docs/directory contains 195 em-dash occurrences across 34 files, including this same file on lines 9, 68, 69. Five panel reviewers independently verified this. The "no emdashes" rule is a constraint on reviewer output, not a documentation convention. Closing. - The OpenAPI divergence note (#comment-3066723271) overstated the scope.
ChatStatusis absent fromswagger.jsonentirely because experimental chat endpoints omit swagger annotations. The actual divergence is between human docs (5 statuses) and DB enum / SDK constants (7 statuses). No generated client is affected.
P4 coderd/exp_chats_test.go:698-702 -- Valid-status assertion accepts dead ChatStatusCompleted (never written by production code) and omits live ChatStatusRequiresAction (written when dynamic tool calls are pending). A chat legitimately in requires_action would fail this test with a confusing error, while a chat that somehow reached completed would pass silently. (Bisky P4)
P4 coderd/x/chatd/integration_test.go:203-204 -- Integration test switches on ChatStatusCompleted as a valid terminal state. Since no production code path writes completed, this branch is dead and would silently accept an invalid transition as success. Same class as the finding above. (Meruem P4)
Neither P4 is introduced by this PR, but this PR's assertion that completed is dead code makes them visible.
"They don't know what a dynamic tool is, how it differs from a non-dynamic tool, or what endpoint to hit." -- Leorio, reading the docs like a newcomer
🤖 This review was automatically generated with Coder Agents.
Removes
pausedandcompletedfrom the documented chat statuses and adds the missingrequires_actionstatus.Why: Both
pausedandcompletedexist in the DB enum and SDK constants but are never set by any production code path. Meanwhilerequires_action(set when dynamic tool calls are pending) was missing from the docs entirely.Analysis: why these statuses are dead code
The only place chat status is terminally determined is the deferred cleanup in
processChat. The status defaults towaitingand is only overwritten to:error— on panic or processing failurepending— on shutdown, queued message promotion, or external promoterequires_action— on pending dynamic tool callswaiting— on interrupt or success (the default)Every other
UpdateChatStatuscall in the codebase writes one of the same five statuses:chatd.go:1276—pending(edit/resend)chatd.go:1334—waiting(archive)chatd.go:1765—pending(promote)chatd.go:2337—waiting(setChatWaiting)chatd.go:2650—pending(insertChatMessage)chatd.go:2878—pending(Close/release)chatd.go:5984—pendingorerror(stale recovery)exp_chats.go:2311—waiting(interrupt fallback)That's an exhaustive list of every production
UpdateChatStatuscall. Neitherpausednorcompletedis written anywhere — they only appear in test fixtures.