Clean fix, well-scoped. The transport-vs-configured provider split is the right architectural move, and Fix B's defensive parsing degrades gracefully if aibridge is fixed upstream. Tests cover both the positive path and the guard against false matches. CI is green.

Severity summary: 4 P3, 1 P4, 3 Nit, 3 Note.

The chatadvisor path (runner.go:53) still constructs GenerateAssistantOptions without ModelProvider, so Bedrock errors through the advisor would still say "Anthropic." The PR description acknowledges this. Eight reviewers flagged it (three at P3, five at Note). The advisor error surfaces as a raw string, not the structured classification, limiting the user-visible impact. This needs a human decision: file a ticket or explicitly accept the gap.

"If the URL portion contains { (e.g., a templated URL like https://example.com/{resource}/invoke), IndexByte finds that { instead of the JSON body's opening brace. The substring from that position is not valid JSON, so json.Unmarshal fails and the function falls back to returning the original ugly wrapper." (Mafuuu)

coderd/x/chatd/chatadvisor/runner.go:53

Note [CRF-9] The advisor constructs GenerateAssistantOptions without ModelProvider, so Bedrock errors through the advisor path still display "Anthropic." The PR description acknowledges this. The advisor error surfaces as err.Error() into AdvisorResult.Error, not as the structured ClassifiedError payload, which limits user-visible impact. This needs a human decision: file a ticket to track or explicitly accept the gap.

(Chopper P3, Ryosuke P3, Knov P3, Bisky Note, Mafuuu Note, Pariston Note, Leorio Note, Kite Note)

🤖

🤖 This review was automatically generated with Coder Agents.

Good progress. 9 of 11 findings from round 1 are addressed (all P3, P4, Nits, and one Note fixed). Two Notes remain without author response:

CRF-9 (Note, chatadvisor/runner.go:53): Advisor path doesn't thread ModelProvider. The round 1 review asked for a human decision: file a ticket or explicitly accept the gap. No response yet.

CRF-10 (Note, generation_preparer.go:590): debugProvider variable name carries user-visible semantics but keeps its debug-oriented name. No response yet.

Further review is blocked until the author responds to these two findings. A brief acknowledgment ("accepted as-is" or "will file a ticket") is sufficient to unblock.

🤖 This review was automatically generated with Coder Agents.

Strong round. Every round-1 finding the panel could verify is genuinely fixed at the root, not patched over: CRF-1's brace search is now anchored after the regex match with a dedicated regression test, the CRF-3 field rename eliminates the ErrorProvider/ModelProvider collision, CRF-5/7/8 comment fixes landed cleanly, CRF-10's debugProvider rename propagated fully (including the computer-use local), and CRF-11's duplicate struct is consolidated. CRF-9 was accepted by the author with a stated trade-off, and CRF-10 was fixed in 89f441a. Tests pass and the new chaterror suite is well-shaped.

This is the panel's first look at the round-2 fix commits (round 2 was blocked on author response), and it surfaced four new findings, all on the new test and fallback code: 3 P3, 1 P4.

Two themes worth calling out. First, the CRF-2 override test proves only half of Fix A: it asserts the error label flips to "bedrock" but never that the metric label stays on the transport provider, the documented invariant ("Metric labels intentionally still use the transport provider"). The retryable path where the two must diverge is exactly the case the test avoids. Second, the PR description says the relabel test "covers the provider relabel wiring end to end," but the test stubs ErrorProvider: "bedrock" directly and never exercises the resolveChatModel -> prepareGeneration -> generateAssistant resolution path; that claim overstates the coverage.

"swap provider for errorProvider in both RecordStreamRetry calls to 'simplify,' and every test stays green while Bedrock retries silently split off into a new 'bedrock' metric series and break the dashboards the PR set out to preserve." (Bisky)

🤖 This review was automatically generated with Coder Agents.

All three round-3 P3 findings are resolved in 87a5aa8: CRF-12 (test renamed to ..._ErrorProviderOverridesTransportLabel, comment corrected), CRF-13 (TestGenerateAssistant_StreamRetryRecordsMetric now sets ErrorProvider: "bedrock" and asserts both that the classified error uses "bedrock" and that the retry metric keeps provider: "test-provider"), and CRF-15 (the providerErrorDetail fallback now calls unwrapTransportErrorMessage, with TestClassify_UnwrapsTransportWrapperInMessageFallback locking it in). Nice, targeted fixes.

One finding is unaddressed and has no response:

CRF-14 (P4, generation.go:614): the resolveChatModel -> prepareGeneration -> generateAssistant seam that populates ResolvedProvider/ErrorProvider is untested, so deleting the field copy would silently revert the relabel. The reviewers explicitly said a full taskStarter integration test would be disproportionate, so this does not require code. It needs only a one-line decision: accept the gap as-is, or file a ticket. A bare acknowledgment unblocks the panel.

Further review is blocked until CRF-14 gets a response (accept the gap, file a ticket, or push a test).

🤖 This review was automatically generated with Coder Agents.

First panel view of the round-3 fix commits (rounds 2 and 4 were churn-guard blocked). All three round-3 P3 fixes verified by the panel, with mutation checks: CRF-12 (test renamed and comment corrected), CRF-13 (the metric-vs-error invariant is now genuinely guarded; the retryable path fires and asserts Classify(err).Provider == "bedrock" while the retry counter keeps provider: "test-provider", so a swap of either label turns the test red), and CRF-15 (the providerErrorDetail message fallback now unwraps the transport wrapper, with a non-tautological test). The accepted gaps CRF-9 and CRF-14 still hold against the current code. Nice, root-cause fixes.

One regression surfaced, introduced by a later commit. 1 P3, 1 Nit.

Commit 7f86340, labeled refactor ("collapse jsonErrorMessage to one unmarshal"), is not behavior-preserving. It addressed CRF-11 (a Note about a duplicate struct) but narrowed the function's tolerance: a single decode into the combined struct fails whenever error is present as a non-object (string, array, number), silently dropping a usable top-level message. Two reviewers caught this independently with side-by-side old-vs-new output, and I reproduced it: {"message":"rate limited","error":"rate_limit"} returns the message under the old two-pass code and "" under the new code. The fix-chain went net-negative here: a Note-level cleanup introduced a P3 that re-emits the exact bug this PR fixes.

"a 'refactor' commit should not introduce a silent behavioral divergence. The existing tests don't cover a non-object error alongside a message, so the regression is invisible." (Kite)

🤖 This review was automatically generated with Coder Agents.

Clean verification round, and the end of the line for this PR's findings. Both round-5 fixes are verified complete by the panel and Netero, each against the current code rather than the author's word:

CRF-16 (P3, the fix-chain regression): jsonErrorMessage now decodes error as json.RawMessage and attempts the nested decode lazily, falling back to the top-level message when error is a non-object. This restores the two-pass tolerance the CRF-11 consolidation dropped without reintroducing the duplicate struct. Five reviewers independently traced the branch table (non-object string/array/number/null error, empty {}, nested precedence) and confirmed TestClassify_KeepsTopLevelMessageWhenErrorIsNonObject fails under the old code and passes under the new. Nested precedence stays guarded.

CRF-17 (Nit): resolvedModel renamed to cuResolvedModel; the computer-use destructuring is now uniformly cu-prefixed.

The two author-accepted gaps (CRF-9 advisor path, CRF-14 field-copy seam) still hold against the current code; no reviewer found new evidence to reopen them.

Across six rounds this PR took 17 findings (1 of them a fix-chain regression introduced by an earlier fix) to root-cause resolution or explicit human acceptance, each fix shipping with a regression test. All findings are now resolved or accepted. Nothing left on the board.

"Boring, in the way a closed case is boring." (Hisoka)

🤖 This review was automatically generated with Coder Agents.