fix(site/src/pages/AgentsPage/components/ChatElements/tools): delete unreachable WaitForExternalAuth tool code - #27684
Conversation
…unreachable WaitForExternalAuth tool code The backend never emits a wait_for_external_auth tool call, so the renderer, component, icon case, and stories could never fire. Remove them and the now-unused imports.
|
/coder-agents-review model:kimi-k3 thinking:xhigh |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 5 findings (5 Note), APPROVE. Review Finding inventoryFinding inventory, PR #27684Findings
Round logRound 1Panel (13 reviewers): Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Ging-react, Melody, Nami, Zoro, Knuckle, Komugi, Leorio, Luffy. Netero first pass: no findings. Panel: zero defects (no P0-P4, no Nits). 5 Notes: 1 informational resurrection warning (Netero), 2 convergent notes that the kept auth_required sibling path has no Go emitter (verified by orchestrator: only codersdk.TemplateVersionExternalAuth matches), 2 structural/positive notes. Reviewed against e819dd4..0109850. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean deletion, independently verified by the full panel: the premise holds (zero references to wait_for_external_auth in any Go source or the chatd tool registry at base and HEAD), the removal is complete (component, renderer, registry entry, icon case, stories, and all four orphaned imports), and the look-alike live ExecuteAuthRequiredTool/toProviderLabel flow was correctly identified and preserved. tsc --noEmit, biome, and the tools unit tests (160 passing) all confirm nothing dangles. As Luffy put it: "prove nobody calls it, take the whole costume off, keep the parts with a pulse."
No defects found (0 P0-P4, 0 Nits). Five notes below, all informational; the only one that may warrant action is CRF-2, which suggests the kept auth_required sibling path is dead by the same argument this PR just proved, and is worth a follow-up check against the chatd execute tool.
site/src/pages/AgentsPage/components/ChatElements/tools/ExecuteTool.tsx:224
Note [CRF-1] This is the second deletion of the same dead code. Commit bb3a363 ("delete the never-shipped external auth flow") already removed WaitForExternalAuthTool once; commit 4bf9b9d (#27430) reintroduced it, consistent with resurrection from a stale pre-cleanup branch during merge or rebase. Nothing to fix in this PR, but if wait_for_external_auth code reappears a third time, the source is long-lived branches forked before bb3a363. (Netero)
🤖
site/src/pages/AgentsPage/components/ChatElements/tools/ExecuteTool.tsx:224
Note [CRF-2] The kept ExecuteAuthRequiredTool and the toProviderLabel chain in toolVisibility.ts parse an auth_required execute-result payload that history says was removed from the backend on the same unmerged branch as wait_for_external_auth. I verified: grep -rn "auth_required" over all Go sources at HEAD returns zero matches, and authenticate_url exists only on the unrelated codersdk.TemplateVersionExternalAuth type. (Pariston, Zoro)
Orchestrator judgment: the verification holds up; the PR's own premise (no server emitter, frontend-only leftovers) applies with equal force to the path it keeps. The PR's stated scope is the wait_for_external_auth tool and the kept code is unchanged behavior, not a regression, so this is a note, not a defect. But it should not silently persist: this needs a human decision. Either confirm against the chatd execute tool's result construction and file a ticket for the sibling deletion (the ExecuteAuthRequiredTool component, the authenticateURL branch in getExecuteRenderData, the ExecuteAuthRequired story, and then-dead toProviderLabel plus its test block), or cite the backend emitter that keeps this path live.
🤖
🤖 This review was automatically generated with Coder Agents.
|
Thanks for the thorough review. I verified both notes independently before responding. CRF-1 (resurrection): Confirmed. This is the second deletion of the same code. CRF-2 (kept I deliberately scoped this PR to 🤖 This comment was generated with Coder Agents. |
…dead execute auth_required flow (#27687) Stacked on #27684. Addresses review note CRF-2 from that PR: the kept `auth_required` execute path is dead by the same premise that PR proved for `wait_for_external_auth`. The chatd execute tool's `ExecuteResult` struct (`coderd/x/chatd/chattool/execute.go:79-88`) has no `auth_required`, `authenticate_url`, or `provider_*` fields, so the execute tool cannot emit the payload this path parsed. The `authenticate_url` matches elsewhere in Go are the unrelated workspace-creation external-auth flow (`codersdk.TemplateVersionExternalAuth`). Per `bb3a363ed4`, the `auth_required` execute payload was written and removed on an unmerged branch before #22290 squash merged, so no server version ever emitted it. Removes: - `ExecuteAuthRequiredTool` and its `ExecuteRenderer` branch - the `authenticateURL`/`providerLabel` chain in `getExecuteRenderData`, and the `Boolean(data.authenticateURL)` disjunct in `shouldRenderExecuteTool` - the `ExecuteAuthRequired` Storybook story - the now-dead `toProviderLabel` helper and its test block - the `auth_required` visibility test case The `providerLabel` identifiers elsewhere under `site/src` (ModelSelector, ModelRow, AISettings) belong to the unrelated AI model/provider selector and are untouched. 🤖 This pull request was created with Coder Agents.
2 participants
The backend never emits a
wait_for_external_authtool call (no references in any Go source, the chatd tool registry, or anywhere outside the frontend), so the entire frontend rendering path for it was unreachable.Removes the
WaitForExternalAuthToolcomponent, its renderer andtoolRenderersentry, theToolIconcase, and the four Storybook stories, along with the imports that only they used (CheckIcon,LoaderIcon,LogInIcon, andtoProviderLabelinTool.tsx).Kept the separate, live
executeauth-required flow:ExecuteAuthRequiredTooland thetoProviderLabelusage intoolVisibility.tsbelong to theauthenticateURLpath, not this dead tool.Refs #27593
🤖 This pull request was created with Coder Agents.