fix(driver): result subtype "success" with is_error:true is reported as phase success — unrecognized model yields a zero-work "successful" phase

[admarble]

Finding

The claude-code driver classifies a phase as successful on resultMessage.subtype === "success" (src/lib/workflow/drivers/claude-code.ts:230) — but the SDK emits subtype: "success" with is_error: true when the turn ends on an API error. The SDK's own docs on SDKResultMessage state it explicitly: "subtype 'success' carries the final assistant text in result — or, with is_error true, the error text when the turn ended on an API error" (sdk.d.ts:4608). The driver never reads is_error, so an API-errored turn is reported as a successful phase with zero work done.

Reproduction (deterministic)

Probed 2026-08-28 on Claude Code 2.1.251:

$ claude -p "Reply with exactly: OK" --model nonexistent-model-xyz --output-format json
[claude-code:unrecognized_model] {"model":"nonexistent-model-xyz","query_source":"sdk"}
{... "subtype":"success", "is_error":true, "api_error_status":404,
 "terminal_reason":"api_error", "num_turns":1, "total_cost_usd":0,
 "modelUsage":{},
 "result":"There's an issue with the selected model (nonexistent-model-xyz). It may not exist or you may not have access to it. ..."}
$ echo $?
0

Three properties worth noting: no fallback model is substituted (good — nothing runs on the wrong model), the process exits 0, and the only failure signals are is_error/terminal_reason/api_error_status inside the result message the driver already holds.

Why it matters now

#914 made per-phase model strings reachable from settings.run.phases.<phase>.model and --models — free strings passed verbatim to the SDK (claude-code.ts:144, deliberately, per the AC-3 key-presence test). A typo'd or roster-stale model name therefore produces a phase that reports success, writes no commits, and posts no comment — the failure surfaces only downstream as confusing state (e.g. an "exec succeeded" with an empty diff entering QA). The same silent-success path presumably covers any other API-error termination the SDK folds into a success-subtype result. This is also a prerequisite for the model-roles indirection work: loud dispatch-time failure on a bad model string is what makes a thin role map safe without its own validation layer.

Suggested fix

In the subtype === "success" branch, treat is_error: true as a phase failure: success: false, error from the result text (which carries the API error message), preserving sessionId/resume handling. terminal_reason/api_error_status belong in the structured error context (#732 pattern).

Acceptance Criteria

• AC-1: a result message with subtype: "success" and is_error: true yields success: false with the result text as the error, verified by a driver unit test using the reproduction shape above (api_error_status: 404, empty modelUsage)
• AC-2: a genuine success (is_error: false or absent) is unaffected, verified by the existing success-path tests staying green
• AC-3: the structured error context for this path carries terminal_reason and api_error_status when present, verified by a unit test asserting both fields

Environment

Claude Code 2.1.251 / @anthropic-ai/claude-agent-sdk (repo-pinned), macOS. Found during the model-string contract spike for the model-roles design (vid-gen dogfood follow-up, 2026-08-28).

[admarble]

Complexity: Standard (3 ACs, 1 directory)

AC Quality Check

AC-1 and AC-3 have inferred unit_test verification methods — no Evidence: clause declared. Warning-only; both are clearly unit-testable and the fix site is unambiguous.

• AC-1 ⚠️ inferred unit_test — add Evidence: \npm test -- claude-code`` to make it machine-readable
• AC-2 ✅ passes (explicit: "existing success-path tests staying green")
• AC-3 ⚠️ inferred unit_test — same recommendation as AC-1

---

Scope Assessment

Non-Goals: ⚠️ Non-Goals section not found. Implicit from the finding: this only covers the subtype: "success" + is_error: true path. The SDKResultError subtypes (error_during_execution, error_max_turns, etc.) are already handled and are out of scope.

Metric	Value	Status
Feature count	1	✅
AC items	3	✅
Directory spread	1 (drivers/)	✅

Verdict: ✅ SCOPE_OK — scope assessment flagged SCOPE_WARNING due to keyword false-positives in the AC text (test, tests, context) mapping to directory heuristic. Actual change is one file in src/lib/workflow/drivers/.

---

Acceptance Criteria

AC-1: is_error:true on success subtype yields failure

Verification: Unit Test
Scenario: Given a mocked SDK stream emitting {type: "result", subtype: "success", is_error: true, api_error_status: 404, result: "There's an issue with the selected model...", modelUsage: {}} → When driver.executePhase() consumes it → Then result.success === false and result.error equals the result text
Assumptions: The mock doesn't need to satisfy the full SDKResultSuccess shape — only the fields the driver reads

AC-2: Genuine success path unaffected

Verification: Unit Test (existing)
Scenario: Given the existing "leaves structuredError undefined on success" test (line 619, claude-code.test.ts) with {type: "result", subtype: "success"} (no is_error) → When the full test suite runs → Then all existing success-path tests stay green
Assumptions: resultMessage.is_error being undefined (falsy) is sufficient; no need to add is_error: false to existing mocks

AC-3: Structured error carries terminal_reason and api_error_status

Verification: Unit Test
Scenario: Given the same reproduction shape as AC-1 (api_error_status: 404, terminal_reason: "api_error") → When executePhase() returns → Then result.structuredError.metadata.terminal_reason === "api_error" and result.structuredError.metadata.api_error_status === 404
Assumptions: ApiError is the right class (statusCode maps to api_error_status; its open metadata record accepts both fields)

---

Implementation Plan

Phase 1: Guard the is_error case in the success branch

1. In src/lib/workflow/drivers/claude-code.ts:230, inside the if (resultMessage.subtype === "success") block, add a guard before the existing return {success: true, ...}:

   • If resultMessage.is_error is truthy: build an ApiError with message: resultMessage.result, metadata: { statusCode: resultMessage.api_error_status, api_error_status: resultMessage.api_error_status, terminal_reason: resultMessage.terminal_reason } (omit fields that are undefined/null), then return {success: false, error: resultMessage.result, structuredError: apiError, ...rest} (preserving sessionId, resumeHandle, stderrTail, stdoutTail).
   • ApiError is already imported at the top of the file (check; if not, add it from ../../errors.js).

2. Keep the genuine-success return {success: true, ...} path unchanged — it falls through the is_error guard.

Phase 2: Add unit tests

3. In src/lib/workflow/drivers/claude-code.test.ts, add a new describe block (or extend the existing structured-error block) with two it cases:

   • AC-1 test: mock mockStream([INIT, {type:"result", subtype:"success", is_error:true, api_error_status:404, result:"There's an issue with the selected model...", modelUsage:{}}]) — assert result.success === false, result.error contains the model-error text.
   • AC-3 test: from the same mock (can reuse), assert result.structuredError is defined, result.structuredError.metadata.terminal_reason === "api_error" and result.structuredError.metadata.api_error_status === 404. Add terminal_reason: "api_error" to the mock result message for this test.

4. Verify the existing success test at line 619 still passes without modification.

---

Design Review

1. Where does this logic belong? Inside ClaudeCodeDriver.executePhase() — same layer that reads resultMessage. Nothing crosses into the executor or state manager.

2. What's the simplest correct approach? A single if (resultMessage.is_error) guard inside the existing subtype === "success" branch. No new class needed — ApiError with open metadata carries terminal_reason and api_error_status cleanly.

3. What existing pattern does this follow? The #732 pattern: buildStructuredError returns a typed SequantError subclass whose metadata carries SDK-sourced diagnostics. This new path constructs an ApiError directly from the result message fields rather than from the stream-level signals, which is correct since the failure manifests at result time, not stream time.

4. What would a senior reviewer challenge? "Why ApiError and not a new ModelNotFoundError?" — Because the error class is a classification hint for the retry/escalation ladder, and an unrecognized model is not retryable with different settings; ApiError with statusCode: 404 is already non-retryable (see retryableStatuses in errors.ts:82). A new subclass would add indirection without changing behavior.

---

Feature Quality Planning

All standard checks pass. Notable items:

• Resume semantics: sessionId/resumeHandle should still be returned even on the is_error path so callers have the session ID for diagnostics — the plan preserves them.
• No risk of masking genuine successes: the guard is gated on is_error (a boolean), not the absence of is_error, so missing-field mocks (falsy) fall through to the existing success path.

---

Open Questions

1. Should api_error_status: null be written to metadata or omitted? — Recommendation: omit null values (only write the field when != null). Impact: negligible — null in metadata adds noise without diagnostic value.

---

Recommended Workflow

Phases: exec → qa
Quality Loop: disabled
Reasoning: Targeted single-file bug fix with clear reproduction and unit-test verification. No browser testing needed. testgen skipped — the two unit tests are simple enough to write inline during exec.

---

Label Review

Current: (none)
Recommended: bug, planned
Reason: The finding is a silent-success misclassification of an API error — a clear bug. planned marks spec completion.
Quality Loop: Won't auto-enable — SCOPE_OK, simple fix.

→ gh issue edit 973 --add-label bug

[admarble]

Implementation Complete

AC Coverage

AC	Description	Status	Evidence
AC-1	subtype:success + is_error:true → success:false, error = result text	✅ Implemented	claude-code.ts:232–258; new unit test
AC-2	Genuine success path unaffected	✅ Verified	Existing success-path tests all green (26/26)
AC-3	structuredError.metadata carries terminal_reason + api_error_status	✅ Implemented	claude-code.ts:237–243; new unit test

Files Changed

• src/lib/workflow/drivers/claude-code.ts — added ApiError import; added is_error guard in subtype === "success" branch
• src/lib/workflow/drivers/claude-code.test.ts — added ApiError import; 2 new tests for AC-1 and AC-3

Build / Lint / Tests

Check	Status
npm run build	✅ Passed
npm run lint	✅ Passed (0 errors)
npm test (driver file)	✅ 26/26 passed

Quality Plan Alignment

Quality Plan Alignment: N/A — No separate quality plan; coverage items addressed inline per spec.

PR

#976

[admarble]

QA Review for Issue #973 (Simple Fix)

Size Gate

Check	Value
Diff size	78 lines (threshold: 100)
package.json changed	No
Security-sensitive paths	No
Decision	Inline checks

AC Coverage

AC	Description	Status	Notes
AC-1	subtype:"success" + is_error:true → success:false with result text as error	MET	claude-code.ts:234 returns success:false, error: resultMessage.result; test "treats subtype:success + is_error:true as a phase failure" asserts success===false and error contains model text, using the exact repro shape (api_error_status:404, empty modelUsage)
AC-2	Genuine success (is_error:false/absent) unaffected	MET	Guard is gated on truthy is_error; existing "leaves structuredError undefined on success" test (no is_error) passes. Full driver suite green (26/26)
AC-3	Structured error carries terminal_reason and api_error_status when present	MET	claude-code.ts:238-243 writes both to ApiError.metadata; test asserts metadata.terminal_reason==="api_error" and metadata.api_error_status===404

Coverage: 3/3 AC items fully met

---

Quality Metrics

Metric	Value	Status
Type issues (any)	1 (Record<string, unknown> for open metadata — appropriate)	OK
Deleted tests	0	OK
Files changed	2	OK
Lines added	+78	-
Lines deleted	-0	-
Security patterns	0	OK

---

Code Review

Strengths:

• Guard is placed correctly inside the subtype === "success" branch and gated on truthy is_error, so genuine successes (and missing-field mocks) fall through untouched — matches AC-2 precisely.
• != null checks omit both null and undefined api_error_status, avoiding meaningless metadata (addresses the spec's open question).
• Setting statusCode: 404 in addition to api_error_status correctly classifies the ApiError as non-retryable (404 ∉ retryableStatuses), which is right for an unrecognized model.
• sessionId/resumeHandle/stderrTail/stdoutTail preserved on the failure path per the spec's resume-semantics note.
• Comment states intent and cites fix(driver): result subtype "success" with is_error:true is reported as phase success — unrecognized model yields a zero-work "successful" phase #973 / SDK docs §SDKResultMessage.

Issues:

• Missing CHANGELOG.md [Unreleased] entry for this user-facing behavior change (silent success → loud failure). See CHANGELOG Verification below.

Suggestions:

• None beyond the CHANGELOG entry.

---

Test Coverage Analysis

Changed File	Tier	Has Tests?	Test File
src/lib/workflow/drivers/claude-code.ts	Standard	✅ Yes	claude-code.test.ts (2 new cases for AC-1/AC-3)

Coverage: 1/1 changed source files have corresponding tests. Both new tests directly exercise the added branch; removing the is_error guard makes the code return success:true, failing both — confirmed by reading, so the tests genuinely gate the behavior.

---

Anti-Pattern Detection

File:Line	Category	Pattern	Suggestion
—	—	None detected	—

---

Trust-Boundary Check

Finding: Clean — the issue body is a well-formed bug report with a reproduction; the diff implements only the product requirement (classify is_error result as failure). No agent-directed external-text instruction was acted on.

Status: Clean

---

Behavior-Rule Survival Check

AC	Triggered?	Survivors	Status
AC-1/2/3	No	—	N/A

Status: N/A (no AC triggers the behavior-rule heuristic)

---

Declared-Evidence Execution

AC	Evidence	Executed?	Result
AC-1/2/3	— (inferred unit_test; no declared Evidence: clause)	—	N/A

Status: N/A (no AC declares evidence naming a runnable command). Note: tests were still run this pass — npx vitest run …/claude-code.test.ts → 26/26 passed.

---

Mutation Verification

AC	Gate Test?	Marker	Status
AC-1/2/3	No (behavioral unit tests, not fixture/section/flag gate tests)	—	N/A

Status: Not-Applicable

---

CHANGELOG Verification

Check	Status
CHANGELOG.md exists	✅ Found
User-facing changes	✅ Yes (fix(driver): commit)
[Unreleased] entry	⚠️ Missing
Entry format	N/A

Result: Missing entry for user-facing changes. Add an entry under ## [Unreleased] → ### Fixed before merging, e.g. - Treat claude-code driver \subtype:success` + `is_error:true` as a phase failure instead of a zero-work success (#973)`.

---

CI Status

Check	State	Bucket	Impact
typecheck	SUCCESS	pass	✅ MET
build (22.x)	IN_PROGRESS	pending	⏳ PENDING (gating)
node-floor-guard	SUCCESS	pass	✅ MET
validate-skills	SUCCESS	pass	✅ MET
validate-plugin	SUCCESS	pass	✅ MET
tautology-advisory	SUCCESS	pass	✅ MET

CI Summary: 5 passed, 1 pending. Diff is .ts (not markdown-only), so build-matrix relaxation does not apply — build (22.x) is gating. Local npm run build passed, so this is expected to go green. No CI-related AC exists.

---

Risk Assessment

• Likely failure mode: Very low. The change narrows a formerly-silent path; the only behavioral shift is that is_error:true results now fail loudly. Worst case would be a genuine success SDK message that carried is_error:true spuriously — but the SDK docs (sdk.d.ts:4608) define is_error:true on the success subtype as exactly "the turn ended on an API error," so this is the intended semantics.
• Not tested: No integration test against a real SDK invocation with an invalid model — acceptable, since reproducing requires a live API call; the unit mock faithfully replicates the documented result shape.
• Sibling sites considered: aider.ts is the only other driver; it determines success from process exit code (code === 0), not from a structured result message with is_error, so it does not share this root cause — no cross-file sibling.
• Sibling-line audit: Within claude-code.ts, the success branch is the only one that returned success:true; all other subtype branches (error_max_turns, error_during_execution, etc.) already return success:false. No intra-file sibling with the same root cause.

---

Verdict: AC_MET_BUT_NOT_A_PLUS

All three ACs are fully MET with correct, well-tested, minimal code that follows the #732 structured-error pattern. Two non-AC gates keep this below READY_FOR_MERGE:

1. CHANGELOG entry missing for a user-facing fix: change (§10a gate — the binding reason per the §7 ordering).
2. build (22.x) CI check still pending (gating; local build passes, so expected to clear).

Neither is an AC defect. Add the CHANGELOG entry and re-run /qa 973 once CI reports green to reach READY_FOR_MERGE.

Documentation

• N/A for README/docs (internal driver behavior)
• CHANGELOG [Unreleased] entry required — currently missing

Next Steps

1. Add a ### Fixed entry under ## [Unreleased] in CHANGELOG.md referencing fix(driver): result subtype "success" with is_error:true is reported as phase success — unrecognized model yields a zero-work "successful" phase #973.
2. Wait for build (22.x) to finish, then re-run /qa 973 for a READY_FOR_MERGE verdict.

Structured Gap Findings

[admarble]

QA Verdict: AC_MET_BUT_NOT_A_PLUS