fix(workflow): classify in-band provider errors as step failures

[Harbor404]

Summary

Fixes #638.

run_agent_step previously extracted last_message and returned
AgentStepResult(status=COMPLETED) without checking whether that text was a
provider refusal. Provider CLIs can exit cleanly after printing an error, so the
refusal sat in the answer slot and was published as the workflow deliverable.

This PR classifies provider-owned in-band errors before building the completed
result, propagates a structured provider_error kind through settlement and
run inspection, and maps the run-step HTTP response to 502 rather than 504.

Current implementation

Classifier safeguards. services/provider_error_classifier.py now applies
five independent guards:

1. provider-scoped signatures only;
2. only the first non-empty extracted line is considered;
3. output over PROVIDER_ERROR_MAX_CHARS is out of scope;
4. signatures must consume the whole line; and
5. when raw adapter context is available, the last matching occurrence must be
   provider chrome, not a rendered assistant message.

The ownership patterns are imported from the adapters that perform extraction
(claude_code, codex, kimi_cli, and mcode) rather than copied into the
classifier. grok_cli does not expose a response marker, so the production
three-argument path deliberately degrades to a miss instead of guessing from
answer-shaped prose. The two-argument API remains available for callers that
already hold independently trusted provider chrome.

Settlement and observation. StepExecutionError.kind reaches
workflow_run_step.error_kind, in-memory step state, live and cold
/workflows/runs/{id} results, and cao workflow status. A nullable run-level
workflow_run.kind records the terminal script verdict, so a caught provider
error cannot be misattributed to an unrelated later nonzero script exit.

HTTP behavior. /terminals/run-step returns 502 for both worker crashes
(error) and in-band provider refusals (provider_error), while timeouts remain
504. Callers are expected to branch on detail.kind.

For provider_error, a terminal created by this call is torn down
best-effort before the exception is raised when teardown=True; reused terminals
or teardown=False remain caller-owned. terminal_id is still returned for
reporting, and the bounded provider detail remains retrievable on the exception.

Testing

At head 90e7623a:

uv run python -m pytest \
  test/services/test_provider_error_classifier.py \
  test/services/test_agent_step.py \
  test/services/test_workflow_service.py \
  test/services/test_script_runner.py \
  test/services/test_settlement_rewire.py \
  test/services/test_journal_step_lifecycle.py \
  test/services/test_terminal_service_coverage.py \
  test/api/test_run_step.py \
  test/api/test_workflow_runs.py \
  test/cli/commands/test_workflow.py \
  test/clients/test_workflow_run_migration.py -q
# 526 passed

The focused set covers markerless-provider conservatism, marker-aware
provider/assistant ownership, provider scoping through the production entry
point, missing raw-context fallback, retry/cancel stale-kind clearing, the
run-level kind precedence fix, and the 502 API mapping.

Review notes

• The branch was synchronized with origin/main by merge, not rebase.
• The suite does not run a real external provider CLI end-to-end; ownership is
  covered through the real adapter extraction seams plus mocked terminal layers.
• Per review, distinguishing permanent provider refusals from transient ones
  for retry policy is left as a follow-up; this PR keeps the existing retry
  behavior.

Current diff: 25 files, +1713/-71.

[Copilot]

Copilot review overview

🟡 Changes recommended

The classifier has false-positive and missed-signature cases, truncates multiline refusal details, and does not fully satisfy the status-display requirement.

Review effort: Balanced
Findings: 4 · 1

Open (5)

• Preserve full provider output in workflow exceptions · New
• Require provider chrome colon to avoid API Error false positives · New
• Match 429 status code boundary for rate-limit responses · New
• Display structured error kind in workflow status · New
• Add API coverage for provider_error 502 mapping · New

What changed in this PR

Classifies in-band provider refusals as workflow failures rather than successful outputs.

Changes:

• Adds bounded, signature-based provider-error classification.
• Propagates structured provider_error failure metadata through workflow results.
• Maps provider failures to HTTP 502 and adds regression tests.

File	Description
provider_error_classifier.py	Defines provider-error signatures and guards.
agent_step.py	Raises structured errors for classified output.
workflow_service.py	Tracks and publishes step error kinds.
workflow_runtime.py	Adds StepResult.error_kind.
api/​main.py	Exposes error kinds and maps provider errors to 502.
test_provider_error_classifier.py	Tests classifier positives and guards.
test_agent_step.py	Tests agent-step classification behavior.
test_workflow_service.py	Tests failure persistence and replay prevention.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Harbor404]

Addressed in dc58388:

• API-error matching now requires the provider colon, so API Error handling should preserve context. is no longer a refusal.
• 429: ... and 429 Too Many Requests both classify via the status-code boundary.
• The exception now carries the full bounded detail, not only the matched first line.
• cao workflow status renders structured error_kind on each step.
• Added the request-level provider_error -> 502 API regression.

Verification: uv run --frozen pytest -q test/services/test_provider_error_classifier.py test/services/test_agent_step.py test/cli/commands/test_workflow.py test/api/test_run_step.py -> 155 passed; black --check and isort --check-only pass.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@55dd4a0). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #849   +/-   ##
=======================================
  Coverage        ?   92.55%           
=======================================
  Files           ?      242           
  Lines           ?    40485           
  Branches        ?        0           
=======================================
  Hits            ?    37469           
  Misses          ?     3016           
  Partials        ?        0           

Flag	Coverage Δ
unittests	92.55% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Harbor404]

The only failing check is the repository-wide Security Scan, which is also failing on main (run 36795561852). This PR does not change dependencies. Upstream PR #848 bumps urllib3/PyJWT for the advisories published on 2026-09-30 and explicitly says it unblocks Security Scan on every PR. I am leaving this branch free of unrelated dependency churn and will rerun after #848 lands.

[Harbor404]

Noting the Security Scan failure so it isn't mistaken for something this PR introduced.

The security job in ci.yml runs uv export --locked --format requirements-txt and then
scans that generated dependency graph with Trivy at severity: CRITICAL,HIGH and
exit-code: 1. So it is gating the locked dependency graph, not the source changes.

This PR touches only application code and tests:

src/cli_agent_orchestrator/api/main.py
src/cli_agent_orchestrator/cli/commands/workflow.py
src/cli_agent_orchestrator/models/workflow_runtime.py
src/cli_agent_orchestrator/services/agent_step.py
src/cli_agent_orchestrator/services/provider_error_classifier.py
src/cli_agent_orchestrator/services/workflow_service.py
test/... (5 test files)

uv.lock and pyproject.toml are unchanged, so the finding(s) are already present on main
and are not caused by this change. I have deliberately not bumped a dependency here: that
would be a separate, reviewable change and is outside this PR's scope.

Everything else on this PR is green (19 checks passing, including the Python matrix, lint,
and the workflow tests). Happy to open a follow-up for the dependency if you'd like.

[Harbor404]

Latest head ccc74133 is now waiting on action_required for the fork PR workflows (CI, cargo-deny, Secret Scan), so no new CI result is available yet. The previous run had every job green except Security Scan; the dependency CVEs it reported are fixed by merging current main (#848). Local verification on the current head: uv lock --check passes, 200 passed in the focused workflow/provider suite. Could a maintainer approve the pending workflow runs?

[Harbor404]

@haofeif @fanhongy could one of you approve the pending workflow runs for head ccc74133? GitHub shows CI, cargo-deny, and Secret Scan as fork-PR action_required. The prior real failure was only the base dependency Security Scan, which current main fixes; local focused tests are green.

[haofeif]

Reviewed head ccc74133705cdee53504dd01c22f3ca4ea959461 against base bd86e5f4a8720d26eba16f14fa49e08ad3e5f3f5.

Two P2 findings. Ordinary successful answers can be reclassified as provider failures, and the new structured kind is lost on the Python-script settlement path despite the immediate HTTP response correctly reporting it. The earlier specific colon/429/detail fixes, CLI renderer change, and request-level 502 coverage are present; the remaining general-classification and script-plumbing issues are detailed inline.

[fanhongy]

Review of PR #849 at 00012961bb36ec4faded9593e543823c5d3fefb8

Summary

The fix is in the right place. run_agent_step is the shared step substrate, so the YAML engine, /terminals/run-step and handoff all get it. It reuses the existing workflow_run_step.error_kind column, so there is no migration.

Since ccc74133, the script-tier settlement work from the open "carry the provider error kind" thread is complete. The kind now reaches:

• the in-memory step;
• the durable row;
• live and cold GET /workflows/runs/{id};
• the _finalize result.

The new tests fail against the ccc74133 source, so they do pin that behaviour.

Blocking problem: the head breaks an existing test that CI runs.

Other problems:

• The match → fullmatch rewrite in 00012961 dropped the Unknown model: <id> refusal form.
• Plausible one-line answers are still classified as refusals.
• Persisting error_kind for every script-tier StepExecutionError changes the run-level kind of successful script runs.
• A test edit silently moved an existing assertion into the new test.

Findings

P1: The settle_step signature change breaks an existing pinned test, so CI's unit-test job will fail

src/cli_agent_orchestrator/services/workflow_journal.py:1167

The problem. The new error_kind parameter breaks test/services/test_journal_step_lifecycle.py::test_settle_step_signature_has_no_attempts_parameter_and_returns_bool (L495-517). That test pins list(inspect.signature(settle_step).parameters) to the seven base parameters.

Evidence.

• At head the test fails: AssertionError: ... Left contains one more item: 'error_kind'.
• On base bd86e5f4 it passes.
• ci.yml runs all of test/, so the job will go red.
• The file is not one of the seven the PR touches, so the "328 passed" focused run never exercised it.
• No CI has run since ccc74133, which predates the parameter.

Fix.

• Add "error_kind" to the pinned list, keeping the "attempts" not in sig.parameters assertion. That assertion is the actual BR-6 intent of the test.
• Document the new parameter in the settle_step docstring.

P2: Unknown/Invalid/Unsupported model: <id> refusals are no longer classified

src/cli_agent_orchestrator/services/provider_error_classifier.py:72

The problem. The pattern model\s+(?:['"][^'"`\n]+['"`]|[:=].+)requires whitespace before the[:=]alternative. Withfullmatch, the natural colon forms all return None`:

• Unknown model: gpt-5.6-terra
• Invalid model: gpt-5.6-terra
• Unsupported model: gpt-5.6-terra

Meanwhile, Unknown model : x and Unknown model = x do match.

History. Commit 39416e2 (model\b\s*[:'"]) classified all three colon forms; 0001296` regressed them.

Impact. When the classifier misses a refusal, the step settles COMPLETED with the refusal text as its replayable output. That is exactly the #638 failure. No test uses the colon form, so the suite stays green.

Fix. Use:

(?:Unknown|Unsupported|Invalid|Undefined) model(?:\s+['"`][^'"`\n]+['"`]|\s*[:=]\s*.+)

Also add "Unknown model: gpt-5.6-terra" (and the Invalid/Unsupported variants) to _REFUSALS.

P2: Plausible one-line answers are still classified as provider failures

src/cli_agent_orchestrator/services/provider_error_classifier.py:62 (and the rows at L76-L100)

This is what remains of the open "do not classify ordinary short answers" thread at this head. Two paths are still open:

1. The :.* rows. api_error (L62) and connection_error (L100) end in :.*. Under fullmatch that is identical to a prefix match, so any first line starting API Error:, APIError: or ConnectionError: is classified. One example: API Error: none found - all 42 endpoints return 2xx.
2. The vocabulary rows. These accept complete one-line answers, for example:
   • Authentication failed.
   • Rate limit exceeded.
   • Quota exceeded.
   • rate_limit = 100
   • Model weights not found.
   • Model validation is not supported.
   • Unknown model 'gpt-4o-mini'

In neither path does provider take part in the verdict, because no row sets providers=.

Repro. I drove Authentication failed., Rate limit exceeded., Model weights not found. and the API Error: none found … line through the real run_agent_step with claude_code, mocking the terminal layer as TestInBandProviderError does. Each one raised StepExecutionError(kind="provider_error"), and the terminal was not torn down.

Impact. Take a worker asked to run an auth or rate-limit check and "answer in one line":

• /terminals/run-step returns 502.
• Every handoff returns success=False.
• The YAML engine retries up to 4 attempts. Each attempt creates a fresh terminal (teardown=True) and leaves it alive, because this raise skips teardown.

Fix.

• Scope the vocabulary rows, via providers=, to the adapters that actually print them.
• Anchor api_error and connection_error on adapter chrome rather than on :.*. One way is to require the parenthesised model id or an HTTP status after API Error.
• Add the lines above to _NON_REFUSALS.

P2: Successful script runs now report a failure kind from /workflows/runs/{id}/result

src/cli_agent_orchestrator/api/main.py:4558 (with src/cli_agent_orchestrator/services/script_runner.py:820 and :869)

The problem. The script tier now persists a durable error_kind for every StepExecutionError, including error and timeout, not only provider_error. The unchanged _resolve_error_kind (api/main.py:6521) reads that column before it looks at the run state.

So take a script that catches ShimHTTPError for one step and exits 0. That is the documented pattern in examples/workflow/workflow.py:114. Its run now resolves to a failure kind.

Repro. A scratch test used the real /terminals/run-step route:

1. Step s1 raises StepExecutionError(kind="error").
2. Step s2 succeeds.
3. The run is set to completed and the registry entry is popped.
4. GET /workflows/runs/{id}/result is called.

Tree	state	kind
base bd86e5f4	completed	None
head	completed	'error' ('provider_error' for a refusal)

Impact.

• The live blocking result for the same run comes from _finalize(state=COMPLETED, kind=None), so the live and cold reads now disagree.
• cao workflow result prints Kind: error under State: completed.
• This contradicts the resolver's documented RP-4 rule: "no kind is ever fabricated for a completed/non-terminal run".
• The same ordering lets a failed row's kind replace "cancelled" on a CANCELLED script run.
• YAML on_failure: continue runs could already reach this resolver state. This PR is what newly exposes the script tier.

Fix.

• In _resolve_error_kind, branch on the run state first:
  • COMPLETED or non-terminal → None;
  • CANCELLED → "cancelled";
  • FAILED → the durable kind, otherwise the inference.
• Add a script-tier regression test for a completed run that contains a caught failed step.

P2: An existing test silently lost its attempts assertion

test/services/test_script_runner.py:1121

The problem. test_script_result_preserves_provider_error_kind was inserted above the last line of test_completion_creates_step_state_when_missing.

• On base, that test ends with assert record.step_states["s1"].attempts == 1.
• At head, the same line is the last line of the new test.

The missing-seed settle path is covered only by test_completion_creates_step_state_when_missing, and it no longer checks the attempt count. Both tests still pass, so the loss is invisible.

Fix. Restore the assertion at the end of test_completion_creates_step_state_when_missing (L1076-1088). The new test can keep its own copy.

P3: Comments and docstrings that no longer match the code

• src/cli_agent_orchestrator/services/agent_step.py:809-811: says replay_single_step "need[s] the live pane". In fact replay_single_step tears down e.terminal_id on every StepExecutionError (workflow_service.py:1830).
• src/cli_agent_orchestrator/services/agent_step.py:16-21 and :574-577: the "Failure contract" and Raises: sections still list only timeout and error.
• src/cli_agent_orchestrator/services/provider_error_classifier.py:37: says "One start-anchored provider-error signature", but matching is now fullmatch.
• src/cli_agent_orchestrator/services/script_runner.py:752: says "THE CALLBACK TAKES (terminal_id, error, last_message, response_status=None)", but it now also takes error_kind.
• src/cli_agent_orchestrator/models/workflow_runtime.py:244-248: says "so no existing response shape changes". In fact model_dump() now emits error_kind: null on every step of every /result and --json body, including successful runs.

Validation

All runs were local and read-only, against the head checkout, with uv run --no-sync and -p no:cacheprovider.

Check	Result
The PR's 7 test files: test_provider_error_classifier.py, test_agent_step.py, test_workflow_service.py, test_run_step.py, cli/commands/test_workflow.py, test_script_runner.py, test_settlement_rewire.py	328 passed
test/services/test_journal_step_lifecycle.py	head: 1 failed, 29 passed (P1). Base bd86e5f4: the pinned test passes.
pytest test/services test/api test/cli test/cao_workflow examples/workflow/tests/ -m "not e2e"	2 failed, 6221 passed, 10 skipped. The failures are the P1 test and test_launch.py::test_launch_builtin_profile_resolves_role_defaults. The latter fails identically on base (a local agent-store override, so environmental).
black --check and isort --check-only on the 15 changed files	clean
mypy src/	head 153 errors in 17 files. The error set is identical to base with line numbers stripped, so there are no new errors.
Classifier probe at head (scratch)	Colon-form model refusals are missed (P2). The one-line answers listed above are classified.
run_agent_step false-positive repro (scratch, terminal layer mocked)	4 of 4 one-line answers raise provider_error, with no teardown.
Run-level kind repro through /terminals/run-step (scratch)	Completed script run: base kind=None, head kind='error' or 'provider_error'.

Not verified:

• Behaviour against a real provider CLI; all coverage, including mine, mocks the terminal layer.
• Windows.
• CI has not run on 39416e2 or 00012961.

Residual risk:

• The YAML journal error column, the run-step 502 detail.message, job state and handoff messages now carry up to 512 characters of unredacted worker or provider text. Only the script-tier journal row goes through _sanitise_error.
• A refusal shape outside the table still falls back to the pre-fix behaviour.

[Harbor404]

Addressed in d1a8b6cb503a1134e3d62473995e4fdf5cc5a74d.

• Kept settle_step's existing required arguments and SQL-owned attempts behavior; error_kind remains a trailing optional additive parameter, and the pinned signature test now asserts that compatible shape.
• Restored Unknown/Invalid/Unsupported model: <id> matching while removing the quoted/common one-line false positives. Classifier rows are provider-scoped, and API Error/rate-limit forms require structured provider evidence.
• Made run-level kind resolution state-first. A COMPLETED or CANCELLED script run no longer inherits a retained failed-step error_kind; the per-step diagnostic remains available on the result.
• Restored the moved attempts == 1 assertion and corrected the stale failure-contract, callback, and response-shape documentation.

Verification:

• focused provider/workflow/API/CLI/examples: 8585 passed, 19 skipped, 1 xfailed
• full CI-equivalent pytest: 12858 passed, 45 skipped, 1 xfailed
• black --check and isort --check-only: clean
• mypy src/: still reports the repository's pre-existing 153 errors; the changed lines add no new diagnostic, and this CI step is continue-on-error.

No force-push was used.

[haofeif]

Re-reviewed d1a8b6cb503a1134e3d62473995e4fdf5cc5a74d against bd86e5f4a8720d26eba16f14fa49e08ad3e5f3f5. New feedback received during closeout was reconciled at the same head.

Two P2 findings:

1. Assistant answers still become provider failures. services/provider_error_classifier.py:64 and :78 accept arbitrary prose after a three-digit value or a model label. Actual Claude Code extraction of API Error: 404 is the response for an unknown route., API Error: 200 endpoints were audited; none failed., and Unknown model: a model type absent from the serializer registry. yields valid assistant text, but the real run_agent_step raises provider_error after a normally completed terminal. Provider scoping and punctuation do not establish error origin. Tracked in the existing classification thread; the new non-error-status comment is a subset of this issue.
2. Cold FAILED-script results misattribute the run's cause. api/main.py:6551-6554 promotes an earlier caught step's kind over the script's terminal outcome. After a caught provider refusal or readiness timeout, a real Python subprocess exiting 1 for an unrelated author-side exception produces a live _drive_process result with kind="error"; the registry-cleared result instead says provider_error or timeout. The no-prior-step and ordinary-error controls agree. This verifies the new run-cause thread. Preserve the terminal run verdict separately from retained step diagnostics, or establish that the selected step actually caused the run failure.

The original script-settlement P2 is fixed: the structured kind reaches state, journal, live/cold step results, inspection, and CLI rendering. Completed/cancelled run verdicts are also preserved. The pinned settlement signature, colon-form refusal coverage, restored attempt-count assertion, and directly discussed docstring corrections are present. The FAILED-run attribution above is a separate remaining boundary.

[Copilot]

Copilot review overview

🟡 Changes recommended

The classifier accepts non-error status codes, and cold script results can misattribute unrelated run failures.

Review effort: Balanced
Findings: 2

Open (2)

• Use run-level terminal kind when restoring script-tier results · New
• Restrict provider error detection to HTTP 4xx and 5xx statuses · New

Resolved since last review (5)

• Display structured error kind in workflow status
• Match 429 status code boundary for rate-limit responses
• Require provider chrome colon to avoid API Error false positives
• Preserve full provider output in workflow exceptions
• Add API coverage for provider_error 502 mapping

[haofeif]

Verified at d1a8b6cb503a1134e3d62473995e4fdf5cc5a74d through the real /terminals/run-step settlement and _drive_process, including a real Python subprocess that exits 1 with an unrelated author-side exception. An earlier caught provider_error step gives live run kind error but cold kind provider_error; an earlier caught readiness timeout gives live error but cold timeout. No-prior-step and ordinary-error controls keep error on both surfaces. The state-first change fixes COMPLETED/CANCELLED, not this FAILED-run cause mismatch. Included as a second P2 in the updated review.

[Harbor404]

Fixed in de5bb288: workflow_run.kind is persisted as the run-level terminal verdict; _resolve_error_kind now prioritizes that run kind for FAILED rows and only falls back to the step error_kind for legacy rows. Added a regression where a caught provider_error (and likewise a caught timeout) precedes an unrelated nonzero script exit: live and cold results both report run-level error, while the retained step diagnostic remains available. Focused tests: 288 passed.

[Harbor404]

Fixed in de5bb288e4cc03e30f83087d81b10f7bbc39c861. Added nullable run-level workflow_run.kind, persisted by _finalize for every terminal script verdict; FAILED cold reads now prefer that run-level kind over any retained step kind. Regression covers caught provider_error and caught readiness timeout followed by an unrelated nonzero exit: live and cold results both remain error, while the step keeps its original diagnostic kind. Verification: focused 488 passed; provider/services/API/CLI/examples 8596 passed, 19 skipped, 74 deselected, 1 xfailed; full CI-equivalent pytest 13049 passed, 45 skipped, 21 deselected, 1 xfailed; black/isort clean; mypy baseline unchanged. No force-push.

[Harbor404]

Latest head de5bb288 includes the fixes for both current P2 threads: provider ownership is now checked against the raw adapter capture, and the terminal script verdict is persisted separately from retained step diagnostics. I reran the focused classifier, agent-step, workflow-run, and journal invariants on this head: 221 passed. The only remaining external blocker is that the fork workflows are action_required (CI run 36883041238, Secret Scan run 36883041074, cargo-deny run 36883041241), so no CI result exists for this SHA. Could a maintainer approve those runs and take a fresh look? No code changes are pending.

[gutosantos82]

PR Review: #849 — fix(workflow): classify in-band provider errors as step failures

Summary

Re-review at head de5bb288. The two fixes maintainers asked for are present and correct for the providers they target: run_agent_step now passes the raw adapter capture to classify_provider_error, and a match is rejected when the last rendered occurrence is owned by the assistant marker (claude_code ⏺/●, codex •/assistant:), so the three concrete claude_code prose examples from the open thread now complete normally; and a nullable run-level workflow_run.kind column, persisted by _finalize, makes _resolve_error_kind state-first so a caught provider_error step no longer misattributes an unrelated nonzero script exit on cold reads. The migration, journal plumbing, 502 mapping, CLI rendering, and safe fallbacks (None context, >512 chars, non-first-line) all check out, and the new behaviours are pinned by tests that fail on the previous head. One defect blocks: _provider_owns_error_line only knows markers for claude_code and codex, yet signature rows are also scoped to grok_cli, mcode, and kimi_cli; for those the guard can never set assistant_owned, so it returns found on any bare line — the exact false positive the maintainers flagged is still reachable for grok_cli, and the function's docstring says the opposite of what the code does. Request changes for that, and because a human CHANGES_REQUESTED is live at a head no maintainer has re-reviewed.

Blocking (must fix before merge)

• [correctness] src/cli_agent_orchestrator/services/provider_error_classifier.py _ASSISTANT_MARKERS + _provider_owns_error_line (if marker is None: continue branch) — Introduced. _ASSISTANT_MARKERS has entries only for claude_code and codex, but _ROWS scopes model_not_available to grok_cli, auth_or_quota to grok_cli and mcode, and connection_error to kimi_cli. When marker is None the loop continues past the marker test, assistant_owned can never become True, and the function returns found and not assistant_owned → effectively found, which is always True because the extracted first line came from that same capture. The docstring asserts "No marker vocabulary for a provider means no positive ownership evidence, so the caller must not classify it" — the code does the reverse. Reachability is concrete: grok's extract_last_message_from_script (providers/grok_cli.py:1288) renders answers as plain unmarked text, and classify_provider_error('grok_cli', 'Unknown model: a model type absent from the serializer registry.', script_output='Unknown model: …\n>') returns a match; likewise classify_provider_error('grok_cli', 'Authentication failed: no credentials configured', script_output=<bare line>). So the ownership defence that resolves the open P2 for claude_code/codex does not reach 3 of the 5 signatured providers. kimi_cli already has _KIMI_RESPONSE_MARKER_RE = ^[•●]\s (providers/kimi_cli.py:783) that the classifier simply never registers, which suggests a wiring oversight rather than intent. Fix: either (a) if marker is None: return False to match the docstring (making grok/mcode/kimi rows inert until markers exist), or (b) register the real markers for those adapters (and add a test driving a marker-less provider through classify_provider_error(..., script_output=…)). Path-weighted (services/, shared run_agent_step/handoff path): blocking.

Important (should fix)

• [conventions] src/cli_agent_orchestrator/services/terminal_service.py:3498 get_output_context — Introduced. logger.debug("get_output_context: %s unavailable for %s: %s", terminal_id, exc) has three %s placeholders and two arguments; when the except path fires, logging emits a format error instead of the diagnostic. Drop one placeholder ("get_output_context: %s unavailable: %s").
• [consistency] PR description ↔ implementation — The body does not mention either load-bearing change at this head: the raw-capture ownership check (_ASSISTANT_MARKERS, _provider_owns_error_line, terminal_service.get_output_context, the script_output= argument) or the run-level workflow_run.kind column (clients/database.py migration, RunRow.kind, update_run_state, the state-first _resolve_error_kind). The "Changes" section still lists only "three independent guards" and providers=; "Notes for Reviewer" says 348 insertions / a 121-line module / 162 lines of tests, but the diff is +1082/−58, the classifier is 212 lines, and 10 test files are touched. A maintainer reading the body cannot learn how their two conditions were addressed. Rewrite Summary/Changes/Notes to this head.
• [consistency] provider_error_classifier.py _ASSISTANT_MARKERS vs providers/codex.py:45 ASSISTANT_PREFIX_PATTERN and providers/claude_code.py:104 RESPONSE_MARKER_PATTERN — Introduced. The marker table is a second private copy of knowledge each adapter already owns; claude_code has already migrated its glyph once (⏺→●, GH #459) and nothing couples the two copies. The classifier's own docstring calls itself "the runtime-side companion of BaseProvider.get_error_message" (providers/base.py:191, already overridden in kimi_cli.py:2227) yet never consults it, so kimi now has two mechanisms answering the same question. At minimum: import the providers' patterns (or add a test asserting equality); better, route ownership through the provider instance.
• [correctness] services/agent_step.py ~L820 cross-pipeline equality — last_message comes from get_output(LAST) (tmux capture-pane render + provider extraction) while script_output comes from get_output_context (status-monitor history buffer); _provider_owns_error_line compares line == error_line / line[match.end():].strip() == error_line across these two renderings. strip_terminal_escapes collapses \r→\n and forward-cursor CSI to a single space, which need not reproduce capture-pane spacing. Divergence mostly biases to a safe miss, but a diverged marked line plus a bare duplicate elsewhere flips to a false positive. Every test hand-crafts script_output to contain the line verbatim, so this has no coverage; the body itself notes "not verified against a real CLI end-to-end". A real-adapter check for codex and claude_code would close this.
• [conventions] CHANGELOG.md ## [Unreleased] — Still no entry (noted at the prior head). The repo hand-maintains ### Fixed with issue-cited bullets; this changes user-visible behaviour (refusal → FAILED step, 502 instead of 504 on run-step, new error_kind/run-level kind on /result, GET /workflows/runs/{id} and cao workflow status). Add a ### Fixed bullet citing #638.
• [tests] test/services/test_provider_error_classifier.py — No test drives a marker-less in-table provider through the production 3-arg path (classify_provider_error(provider, output, script_output=…)); every grok_cli/mcode/kimi_cli test uses the 2-arg form, which is why the blocking defect is unpinned. Add one per provider (and the fixed expectation). Also carried forward: providers= scoping is tested only via ProviderErrorSignature.applies_to directly; no test feeds the same string to an eligible and an ineligible provider through classify_provider_error, so dropping the applies_to call inside the classify loop would pass.
• [correctness] services/agent_step.py + engine retry — Every family raises uniform kind="provider_error", so the engine retries deterministic refusals (unknown model, invalid key) like transient ones, relaunching a terminal per attempt and leaving each failed pane alive (matching the crash-path contract); a halt step with N retries can leave N live terminals. The slug already distinguishes transient from permanent but is discarded at the raise. Carry it (or a retryable flag). Acceptable as a follow-up.

Nits (optional)

• [conventions] docs/api.md:262 — Zero docs touched. The run-step section documents error/error_kind and the 200/4xx mapping but not the new provider_error → 502 mapping, nor provider_error as a kind value. One line keeps it in step (CODEBASE.md documentation-maintenance rule).
• [consistency] "guards" count — PR body says three, the module docstring (L14–20) says four, and neither counts the ownership check, which is now the central false-positive defence. Pick one framing and include ownership.
• [consistency] ProviderErrorMatch.line / .provider — Set in classify_provider_error, read only by tests (production reads .slug/.detail/.kind). The prior-head flags on providers/applies_to/.kind are now resolved.
• [tests] test/services/test_workflow_service.py — The two new in-memory clears (st.error_kind = None on the cancel/SKIPPED path :801 and after a retried step settles :848) are executed but unasserted; removing either lets _build_result (:950) surface a stale kind with no test failing.
• [tests] no-raw-context fallback — No dedicated test feeds a would-match output with script_output=None and asserts the step COMPLETES (the documented degrade-to-today contract).
• [tests] default provider — kiro_cli (CAO's default) and q_cli have no negative assertion through classify_provider_error; cheap insurance against a future row forgetting to scope.
• [correctness] api/main.py _resolve_error_kind — if isinstance(run_kind, str): return run_kind returns "" if row.kind is ever an empty string. Unreachable today.
• [consistency] scope — The 20-file spread is cohesive layering, not padding, but the run-level kind column fix is independent of the classifier fix and could stand alone. Maintainer's call.

Tests

517 passed at this head with the PR source on PYTHONPATH; the 6 failures (sqlite3.OperationalError: unable to open database file in test_get_run_status_200, test_cancel_run_unknown_404, and four resume tests) reproduce identically on the main baseline and are a sandbox DB-path artifact, not PR-caused. The new behaviours are genuinely pinned: test_provider_shaped_assistant_text_completes feeds API Error: 404 is the response for an unknown route. (matches [45]\d{2}) and asserts COMPLETED, which fails on the previous head; the migration test asserts the kind column; test_caught_step_kind_does_not_override_unrelated_run_failure asserts run-level error where the old column-first resolver returned provider_error. Of the four gaps noted at dc583882: prose negatives for the non-colon rows are closed (_NON_REFUSALS now has Model weights not found., Model validation is not supported., API Error: 200 endpoints…, …123 is not an HTTP status class.); /result steps[].error_kind is now asserted (test_completed_script_result_ignores_caught_step_failure_kind); providers= scoping through classify_provider_error is still open; clear-on-settle/clear-on-cancel is still unasserted. New gaps: marker-less in-table providers untested on the production 3-arg path (the blocking defect's root cause), no isolated no-raw-context fallback test, no kiro_cli/q_cli negative. Hygiene is good: classifier tests pure/sync, async tests under asyncio_mode=strict, terminal_service seams mocked (no real tmux), test_real_adapters_extract_and_preserve_ownership crosses the real Codex/ClaudeCode extraction boundary the maintainers' repro used.

Verification

Dynamic verification (verifier) did not return in time; not covered. Partial substitutes from the static reviewers: the tests reviewer ran the change-selected suites (517 passed, see Tests); the correctness and tests reviewers both executed the shipped classify_provider_error directly — ✓ VERIFIED None raw context never classifies, >512 chars never classifies, non-first-line never classifies, [PARTIAL RESPONSE] prefix never classifies, multi-turn last-occurrence-wins works for claude_code/codex; ✗ REFUTED the docstring claim that marker-less providers are not classified (classify_provider_error('grok_cli', 'Unknown model: a model type absent from the serializer registry.', script_output=…) → match; same for mcode auth prose); ✓ VERIFIED black --check / isort --check-only clean on the 10 changed source files; ✓ VERIFIED no ReDoS shape (all rows anchored/linear or bounded O(n²) on a ≤512-char first line) and the raw capture is used only transiently inside _provider_owns_error_line — never journaled, logged, or returned; what travels on the 502 and in the journal error column is the ≤512-char extracted detail, already surfaced on a successful step today.

Verdict

Request changes — the ownership guard that resolves the maintainers' false-positive concern for claude_code/codex is a no-op for grok_cli, mcode, and kimi_cli (code contradicts its docstring; the P2 example still classifies for grok_cli), and a human CHANGES_REQUESTED remains live at a head no maintainer has re-reviewed. Fix the marker-less branch, add the production-path tests, fix the logger.debug arg count, and refresh the PR body; the rest is approve-quality.

[Harbor404]

Re-reviewed the CHANGES_REQUESTED feedback on de5bb288; the follow-up is on d27b441f.

Blocking

• _provider_owns_error_line marker-less branch: fixed. The ownership guard now returns False when no adapter marker is registered, matching its docstring. kimi_cli and mcode now contribute their real extractor-owned marker patterns; grok_cli deliberately falls back to the conservative pre-Workflow runtime must classify in-band provider errors as step failures, not completed steps #638 behavior on the production three-argument path because its answers are unmarked. Added production-path tests for grok conservatism and kimi/mcode provider-chrome vs assistant-owned text.

Important

• get_output_context logging: fixed to two placeholders/two arguments.
• PR description: rewritten for d27b441f, including raw-capture ownership, run-level workflow_run.kind, current diff size, and the guard count.
• Marker duplication: _ASSISTANT_MARKERS now imports the adapter-owned patterns from claude_code, codex, kimi_cli, and mcode instead of keeping private copies.
• Cross-rendering equality: kept exact-line matching; any capture/extractor spacing divergence biases to a safe miss. This is covered through the real adapter extraction seams, but not with a live external provider binary, so I am not claiming live-CLI coverage.
• CHANGELOG.md and docs/api.md: added issue-cited entries for the behavior and 502 mapping.
• Marker-less/production-path and provider-scope tests: added through the three-argument and two-argument entry points.
• Retry policy: as the review noted, this is acceptable follow-up. Permanent vs transient slug/retryability is not changed here.

Nits / lower-risk corrections

• Guard count is now consistently five and includes ownership.
• Clear-on-settle and clear-on-cancel are now asserted; stale-kind cancel has its own regression.
• Missing raw context now has a production-path regression asserting COMPLETED.
• kiro_cli and q_cli have explicit negative cases through classify_provider_error.
• _resolve_error_kind now ignores an empty run kind and falls through; covered by a unit test.
• ProviderErrorMatch.line/provider and the 24-file scope required no additional change.

Earlier resolved threads (multiline detail, 429 form, status rendering, API 502 coverage, script settlement, run-cause attribution) were rechecked at this head; no additional changes were needed.

Verification on d27b441f: focused suite 495 passed; Black and isort clean on all changed files.

[gutosantos82]

PR Review: #849 — fix(workflow): classify in-band provider errors as step failures

Summary

Re-review at head d27b441f (follow-up commit on top of de5bb288, plus a merge from main). Every item from the previous review is resolved: _provider_owns_error_line now returns False when a provider has no registered assistant marker (services/provider_error_classifier.py:156-161), so on the only production path (services/agent_step.py:818, always the 3-argument form) grok_cli degrades to the conservative pre-#638 miss instead of the false positive maintainers flagged; _ASSISTANT_MARKERS imports the real adapter patterns (claude_code.EXTRACTION_RESPONSE_PATTERN, codex.ASSISTANT_PREFIX_PATTERN, kimi_cli.KIMI_RESPONSE_MARKER_RE, minimax_code.ASSISTANT_MARKER_PATTERN) with kimi_cli/minimax_code refactored to expose them without regex change; the logger.debug arity, _resolve_error_kind empty-string fall-through, CHANGELOG ### Fixed (#638) bullet, docs/api.md 502 line, guard count ("five"), and PR body size numbers (24 files, +1228/−61) all match the code. The new tests are real regressions — the grok 3-arg test and the empty-run-kind test fail on de5bb288, and the clear-on-settle/clear-on-cancel asserts fail when the clears are reverted. Security posture is good: the raw terminal capture is used transiently and never journaled, logged, or returned; migration SQL is static; signature regexes are bounded; no services→providers import cycle. What remains is one doc↔code drift in the run-step handler's authoritative contract docstring, an optional redaction consistency hardening, and carried-forward nits. Code-wise this is approve-quality; the merge gate is now entirely about maintainer re-review and CI, not the code.

Important (should fix)

• [consistency] src/cli_agent_orchestrator/api/main.py:4182-4188 run-step handler "Failure contract" docstring — Introduced. The docstring bills itself as the authoritative contract ("spelled out, not just inferable… the future engine caller depends on this") and still enumerates only "kind": "timeout"|"error" with error → 502, timeout → 504. The same handler now raises kind="provider_error" (:4572) and maps it to 502 (:4566-4568, if e.kind in ("error", KIND_PROVIDER_ERROR)). The inline comment and docs/api.md:207-209 were updated; the in-code contract was not. Add provider_error to the kind enumeration and the status-mapping line.
• [security] api/main.py run-step 502 body and services/workflow_service.py:308-330 agent-tier st.error — Lateral (the same text was surfaced as the step's successful last_message before this PR), but now inconsistent across tiers: the script-tier settlement runs the error through _sanitise_error() → redact_secrets(), while the agent-tier _run_step persists st.error = str(exc) unredacted and the 502 detail.message embeds provider_error.detail (≤512 chars) verbatim. If a provider's first error line echoes a key or token (auth refusals sometimes do), it reaches the HTTP response and the agent-tier run result unredacted. Bounded size and the non-persistence of the raw buffer keep this low; routing both through the same redact_secrets is a cheap consistency fix. Path-weighted (api/, services/) to Important.

Nits (optional)

• [consistency] PR body / _ROWS coverage honesty — grok_cli's two signature rows (model_not_available, auth_or_quota) can never fire on the production path now that the 3-arg form rejects marker-less providers; they are reachable only via the 2-arg API, for which there is no production caller. The body frames the 2-arg API as "for callers that already hold independently trusted chrome" — one honest sentence that grok is intentionally inert in production, and that signatures reach 5 of 13 adapters and markers 4 of 13, would save a maintainer the derivation.
• [tests] test/services/test_provider_error_classifier.py:146 test_marker_aware_three_arg_path_distinguishes_provider_chrome — Guards the kimi/mcode marker registration going forward (removing the entries fails it), but neither input flips vs de5bb288: the assistant-owned negative has no bare duplicate of the error line, so both heads return None. A case with a marked occurrence plus a bare duplicate of the same line would demonstrate the ownership verdict change for these two providers.
• [tests] real kimi/mcode extractors — test_real_adapters_extract_and_preserve_ownership crosses the real extract_last_message_from_script boundary only for codex and claude_code; the kimi/mcode case hand-crafts script_output. The cross-rendering spacing concern (capture-pane render vs get_output_context history buffer) therefore remains uncovered for the two newly wired providers. The failure mode is a safe miss, so not blocking.
• [correctness] services/agent_step.py:816-818 cross-rendering equality — Carried forward, acceptable: exact-line comparison between get_output(LAST) and get_output_context renderings biases any divergence to a miss (never a false FAILED). The theoretical flip (diverged marked line plus a bare duplicate) stays reachable and uncovered by a live CLI, which the author disclaims explicitly.
• [consistency] provider_error_classifier.py:20-22 module docstring — Pre-existing: still calls itself "the runtime-side companion of BaseProvider.get_error_message" yet never consults it (it imports marker patterns only), and the sentence has no main verb. kimi_cli consequently still answers "is this chrome a refusal?" via both its get_error_message override and its classifier marker.
• [consistency] provider_error_classifier.py:213 ProviderErrorMatch.provider / .line — Pre-existing: populated, read only by tests (production reads .slug/.detail/.kind). Author chose to keep them.
• [conventions] providers/kimi_cli.py:798 KIMI_RESPONSE_MARKER_RE = KIMI_RESPONSE_MARKER_RE — Correct back-compat alias of the module-level constant, but reads oddly; a one-line comment helps the next reader.
• [correctness] retry policy — Carried, acknowledged follow-up: every family raises uniform kind="provider_error", so deterministic refusals (unknown model, invalid key) are retried like transient ones, leaving one live terminal per attempt (matching the crash-path contract). The slug already distinguishes them but is discarded at the raise.

Tests

Change-selected suites run with the PR source on PYTHONPATH (pytest 8.4.1, asyncio_mode=strict): 114 passed for test_provider_error_classifier.py + test_agent_step.py; test_workflow_service.py and test_workflow_runs.py targeted tests all green; no PR-caused failures (two pydantic DeprecationWarnings unrelated). All six gaps from the previous review are closed and three were mutation-verified: test_markerless_provider_context_is_conservative (:124) flips match→None against the reconstructed de5bb288 ownership logic, closing the prior blocking defect; test_resolve_error_kind_ignores_empty_run_kind (test_workflow_runs.py:1778) fails on the old isinstance(run_kind, str) code; the clear-on-settle assert (test_workflow_service.py:176) and test_cancel_clears_a_prior_retry_error_kind (:529) each fail when the corresponding clear (workflow_service.py:848 / :801) is reverted; test_provider_scope_is_enforced_through_classifier (:114) feeds the same string to codex (match) and kiro_cli/q_cli (None) through the production entry point; test_missing_raw_context_degrades_to_completed (test_agent_step.py:1287) patches get_output_context → None and asserts COMPLETED. Hygiene is good: new async tests carry @pytest.mark.asyncio, tmux/terminal seams are mocked (_patch_terminal_layer, AsyncMock on run_agent_step), no real CLI is spawned. Imported marker symbols are the real shared objects; minimax_code._ASSISTANT_LINE_PATTERN is rebuilt textually identical to the pre-split regex. Remaining gaps are the two nits above (kimi/mcode flip case; real kimi/mcode extractor through the classifier); the logger.debug arity fix is untested but not meaningfully unit-testable.

Verification

Dynamic verification (verifier) did not return before synthesis; not covered as a separate angle. Partial substitutes from the static reviewers: ✓ VERIFIED the change-selected suites pass with the PR source (see Tests), and the verifier's in-progress output showed test_provider_error_classifier.py at 45 passed across eight consecutive runs (stability loop) before it had finished; ✓ VERIFIED classify_provider_error('grok_cli', 'Unknown model: …', script_output='…\n>') now returns None (the prior blocking repro) while bare kimi/mcode chrome still classifies and marker-owned lines do not; ✓ VERIFIED the new grok and empty-run-kind tests fail against de5bb288 logic and the clear-kind asserts fail under mutation; ✓ VERIFIED black --check (26.3.1, line-length 100) and isort --check-only --profile black clean on all 22 changed .py files; ✓ VERIFIED import cli_agent_orchestrator.services.provider_error_classifier succeeds with no circular import (providers import only services.settings_service, which is stdlib-only); ✓ VERIFIED migration SQL is a static literal and all kind writes are parameterised; ✓ VERIFIED the raw capture never reaches ProviderErrorMatch.detail, the journal, or a log line. ⁇ NOT VERIFIED: any live external CLI end-to-end (author disclaims this too); CI on this SHA (fork workflows action_required).

Verdict

Approve with nits — every finding from the previous review is resolved at d27b441f, the new tests genuinely pin the fixes, and the remaining items (the run-step handler contract docstring missing provider_error, the agent-tier/502 redaction inconsistency, and the carried nits) are small and non-blocking. Merge readiness now rests on maintainer re-review of the addressed P1/P2 threads and on CI running at this head.

[Harbor404]

Synced the branch with the latest origin/main to clear the merge conflict (via merge, not rebase).

• Merged origin/main (5828bd23) into fix/workflow-inband-provider-error.
• Only conflict was in CHANGELOG.md: both sides had prepended a new entry under [Unreleased] > Fixed (this PR's "In-band provider refusals" Workflow runtime must classify in-band provider errors as step failures, not completed steps #638 note, and main's install-overwrite guard fix(install): refuse an install that would silently overwrite another profile's installed artifacts #493 note). Resolved by keeping both entries — no wording removed and no other file was hand-edited.
• No code changes beyond the merge.

Focused verification on the merged head 628f7cf1:

uv run pytest test/services/test_provider_error_classifier.py \
  test/services/test_agent_step.py test/services/test_workflow_service.py \
  test/api/test_workflow_runs.py test/api/test_run_step.py \
  test/cli/commands/test_workflow.py test/services/test_journal_step_lifecycle.py \
  test/providers/test_kimi_cli_unit.py test/providers/test_minimax_code_unit.py \
  test/services/test_script_runner.py test/services/test_settlement_rewire.py \
  test/clients/test_workflow_run_migration.py -q -p no:cacheprovider
# 613 passed

uv run black --check / uv run isort --check-only on the changed source files: clean.

[haofeif]

Re-review at 628f7cf1 (vs main@5828bd23). Requesting changes for one P1.

Since d27b441f the branch only picked up two merges from main. The only conflict was in CHANGELOG, where both the #638 and #493 entries were kept, so the PR's own patch is unchanged. CI was green at 6211337a; the fork workflows at this head are awaiting maintainer approval.

Still fixed from earlier rounds: short-answer false positives, the run-level kind persisted by _finalize and preferred by _resolve_error_kind, marker-less providers falling back to pre-#638 behaviour, and CHANGELOG/docs.

[P1] The ownership guard keeps the fix from firing on real Claude Code and Codex refusals. With script_output, classify_provider_error only classifies an error line that appears without the adapter's assistant marker. The real CLIs render these errors differently:

• Claude Code prints API errors as an assistant line: ⏺ API Error: 400 Claude Code 2.1.236 does not support this model… (anthropics/claude-code#91345) and ● API Error: 400 invalid params… (anthropics/claude-code#92316). The sample in #638 has the Claude Code Bedrock shape API Error (<model>): 400 … (cf. anthropics/claude-code#98298). The guard treats the marked line as an answer and returns None. The Claude extractor only returns text that follows the last ⏺/● (claude_code.py:1392-1395), so in the raw capture the extracted error line carries the marker. For Claude Code, "extractable" and "unmarked" exclude each other by design. A hit would need the raw stream and capture-pane to render the same line differently.
• Codex prints upstream refusals as ■ unexpected status 400 Bad Request: {…} (openai/codex#6933) or ⚠️ stream error: unexpected status 400 … (openai/codex#4270). With no • after the user line, the extractor returns the text after that line (codex.py:1937-1940), and no _ROWS signature matches it.

The tests pass because their fixtures render these errors the opposite way from the real CLIs (see the inline comments). This is not a regression, since a miss leaves the step COMPLETED as before. However, Fixes #638 would close the issue while it still reproduces on both targeted providers.

Suggested direction: decide ownership from a provider-native error signal instead of marker absence. For example, Claude Code flags these entries "isApiErrorMessage": true in its session JSONL (anthropics/claude-code#96033), and Codex error chrome has its own ■ / ⚠️ stream error prefix. Add a fixture captured from a real session of each CLI.

[P3] The run-step failure contract omits provider_error (inline on api/main.py). gutosantos82's re-review raised this too.

[Harbor404]

Re-review at 166e6b3c. All three items from @haofeif's review are addressed:

• [P1] provider-native ownership. The marker-absence guard is gone. Claude Code's API errors are recognised on the response bullet they actually use (⏺/● API Error: <4xx/5xx>); Codex gets an unexpected_status signature scoped to its own ■ / ⚠️ stream error chrome, with the • assistant marker still disqualifying.
• [P1, test] the inverted ⏺ {refusal} assertion and test_agent_step.py::_context are fixed, and new fixtures transcribed from claude-code#91345/#92316 and codex#6933/#4270 are pinned by crossing the real adapter extractors.
• [P3] the run-step failure contract now lists kind="provider_error" and the 502 mapping in both the endpoint description and the docstring.

Verification (focused, no full run): 132 passed (classifier + agent_step); 432 passed (9-file set); 313 passed, 1 xfailed (extra API/provider files); black/isort clean; mypy clean on the classifier. No force-push.

[fanhongy]

Independent automated review commissioned by the PR author and performed by a separate CAO review agent. These findings are the agent's assessment, not the author's manual self-review.

Review of #849 at 166e6b3

Summary. The provider_error plumbing is sound. The kind reaches workflow_run_step.error_kind, the in-memory step state, the live and cold /workflows/runs/{id} views, cao workflow status, and the 502 mapping. The state-first _resolve_error_kind and the new workflow_run.kind column are consistent and tested.

The two problems are in the latest commit's ownership change and in the teardown policy on the new failure path:

• An ordinary short Claude Code answer can now fail a step that passes on main.
• Every failed attempt leaves a live terminal behind.

There are no P1 findings: 2 × P2 and 2 × P3.

Findings

P2: Claude Code's marker exemption turns off the ownership guard for every claude-scoped row, so short ordinary answers fail the step

src/cli_agent_orchestrator/services/provider_error_classifier.py:155, applied at :198. The claude_code scopes are at :76 (api_error), :99 (colon-form model_not_available) and :126 (rate_limited).

166e6b3 adds _ERRORS_RENDERED_ON_ASSISTANT_MARKER = {"claude_code"}, so a ⏺/●-marked occurrence now counts as provider chrome.

• ClaudeCodeProvider.extract_last_message_from_script only returns text after the last ⏺/● marker.
• So for Claude Code, the raw-context check now only asks whether the extracted line appears in the capture. That is the same condition the true-positive path needs, so it always passes in practice.
• That leaves the regex rows as the only guard, and the exemption covers every row scoped to claude_code, not just API Error: <status>. The commit's evidence (anthropics/claude-code#91345, #92316) supports only that one shape.

I reproduced this through the real Claude extractor and the production three-argument classifier. The answer was rendered as Claude's final ⏺ block after a ⏺ Bash(pytest -q) tool block:

Final Claude answer	head 166e6b3	628f7cf / main
429 Too Many Requests	rate_limited	answer
API Error: 404 Not Found	api_error	answer
Unknown model: `Invoice` isn't registered in admin.py, so I added it.	model_not_available	answer
Rate limit exceeded, retry after 60 seconds.	rate_limited	answer
The answer is 42.	answer	answer

Impact, in each caller:

• run_agent_step: the Unknown model: answer raises StepExecutionError(kind="provider_error").
• YAML workflow (default retries): a step whose answer is 429 Too Many Requests ends failed/provider_error after 4 attempts, and the run is failed. On main it completes on attempt 1.
• MCP handoff and /terminals/run-step: callers get a failure instead of the answer.

This reverses the priority the module itself states ("a MISSED classification degrades to today's behaviour; a false one would fail a step that really answered"), and #638's criterion 4. The extractor keeps only the last marker block, so short final blocks after tool calls are common, which widens the exposure.

The new test_marked_ordinary_answers_are_not_refusals does not catch this. None of its phrases matches any row, whatever the context, so it passes with or without the ownership guard.

Suggested fix:

1. Key the exemption on the (provider, slug) pair, so it covers only api_error for claude_code, the one shape with upstream evidence.
2. Then either drop claude_code from rate_limited and the colon-form model_not_available, or keep the marker veto for those rows.
3. A real Claude API Error: 4xx and an answer that starts the same way still look alike. To tell them apart, key on a Claude-native signal instead of the text. Options are the error styling in the raw stream before strip_terminal_escapes, or isApiErrorMessage in the session JSONL.
4. Add regression cases where a marked answer does match a row, such as the rows in the table above.

P2: provider_error ignores teardown=True, so each YAML retry leaves a healthy CLI terminal alive

The raise is at src/cli_agent_orchestrator/services/agent_step.py:817-824. The retry loop is at src/cli_agent_orchestrator/services/workflow_service.py:821-843.

The new branch raises without the if teardown and created_here: await _best_effort_teardown(...) that the success path runs a few lines later.

• The YAML drive loop calls run_agent_step(teardown=True) with a fresh terminal on every attempt.
• It overwrites st.terminal_id on each StepExecutionError.
• The comment at agent_step.py:811-816 says "the drive loop retains it on the step for inspection". In fact only the last attempt's terminal is kept, and the earlier ones are unreferenced live provider CLIs.

I reproduced this with workflow_service.start_run (terminal layer mocked; real engine, run_agent_step and classifier; default retries; Claude answer 429 Too Many Requests):

• Head: run failed after 4 attempts. Terminals t1–t4 were created and none deleted; only t4 stays on the step.
• main: run completed. t1 was created and deleted.

Why this matters:

• True refusals leak too. An unsupported model in an N-step fan-out leaves 4N idle CLI processes, not just the false positives above.
• Capped nodes block. On a node with server.max_terminals or CAO_MAX_TERMINALS, those tracked rows can block later terminal creation.
• The pane isn't needed to recover the text. Unlike error/timeout, the CLI here is healthy and idle, and the refusal text already travels on the exception (#638 criterion 2).

Suggested fix:

• Preferred: in the provider_error branch, call _best_effort_teardown(terminal_id, registry) when teardown and created_here before raising, and keep terminal_id on the exception for reporting.
• If keeping a pane is required: tear down the previous attempt's terminal in _run_step before retrying, so at most one is kept per step, and correct the comment.

P3: Raw context is fetched on every successful step, including providers that no row can match

src/cli_agent_orchestrator/services/agent_step.py:817

get_output_context runs before the classifier on every successful step. It does a terminal-metadata DB read, plus a tmux capture-pane when the rolling buffer is empty. It runs even when classify_provider_error will return None at once without using the context:

• kiro_cli (the default provider) and every other provider with no scoped rows;
• any output over 512 characters.

Check first with the cheap two-argument call (or check provider eligibility and length), and fetch the context only for a candidate match.

P3: terminal_service.get_output_context has no direct test

src/cli_agent_orchestrator/services/terminal_service.py:3495

The "return None means do not classify" contract is what stops capture failures from turning into step failures. None of its four branches is tested: no metadata, rolling buffer, empty buffer falling back to backend history, and exception.

The new agent_step tests patch it with create=True. The existing happy-path tests in test_agent_step.py now run through the real, unpatched function, which reads the developer's real terminal database.

Add a unit test for the four branches, and patch the function in _patch_terminal_layer.

Validation

Everything below is at head 166e6b3, read-only. pytest temp files went outside the checkout, with --no-cov -p no:cacheprovider, and the checkout was clean afterwards.

Tests I ran:

• PR-touched and adjacent provider tests, 14 files: 1138 passed, 3 skipped, 1 xfailed. The files are classifier, agent_step, workflow_service, script_runner, settlement_rewire, journal_step_lifecycle, run_step API, workflow_runs API, CLI workflow and run migration, plus the kimi, minimax, claude_code and codex unit tests.
• Workflow, replay, journal, handoff, orchestration, terminal_service and cao_workflow shim suites, plus examples/workflow/tests: 804 passed, 1 skipped.
• Format: black --check and isort --check-only on the 22 changed Python files are clean.

Probes I ran:

• the real Claude extractor plus the production classifier, at head and at 628f7cf (the table above);
• run_agent_step with a mocked terminal layer;
• a start_run terminal-count probe, on head and on base 5828bd2.

Checked from the acquisition run:

• Full CI-equivalent suite at 166e6b3 (with the .env-writing files ignored): 13313 passed, 10 failed.
  • Nine failures also fail on base 5828bd2 and come from the local environment: live local CAO sessions, a local agent-store override, and the AF_UNIX path length.
  • The tenth is test_fifo_reader.py::TestReaderThreadLifecycle::test_stop_right_after_writer_eof_does_not_leak, a FIFO open race (ENXIO) in a file this PR does not touch. I re-ran that file three times and it passed each time (46 passed).
• mypy src/: the normalized error set is identical to base (151 errors in 16 files on both).

Not verified:

• real provider CLIs end to end;
• CI on 166e6b3, which has not run because the fork workflows are awaiting approval.

The PR body still describes head d27b441f and the earlier marker-absence guard.

[Harbor404]

@fanhongy thanks — point-by-point at head 5f0f7055, and how it squares with @haofeif's P1.

Both reviews point the same way: stop inferring ownership from marker absence and key on the provider's own error signal. haofeif needed the real Claude/Codex refusals to fire; you needed ordinary short Claude answers to stop firing. Both are satisfied by narrowing Claude's signal to the one shape with upstream evidence, so nothing has to guess from the marker.

P2-1 (short-answer false positives) — fixed.

• The response-bullet exemption is now keyed on the (provider, slug) pair and contains only ("claude_code", "api_error") — the one shape with upstream evidence ([BUG] Fable 5.1 requires unstable release of Claude Code anthropics/claude-code#91345, #92316). (provider_error_classifier.py)
• claude_code is dropped from the rate_limited row and the colon-form model_not_available row, which have no evidence Claude renders them as chrome.
• Reproduced your probe through the REAL Claude extractor + the production three-argument classifier. Before/after:

Final Claude answer (last ⏺ block after a tool call)	166e6b3	5f0f7055
429 Too Many Requests	rate_limited	answer
Unknown model: … isn't registered …	model_not_available	answer
Rate limit exceeded, retry after 60 seconds.	rate_limited	answer
The answer is 42.	answer	answer
API Error: 400 Claude Code 2.1.236 does not support…	api_error	api_error

• Regression tests: test_marked_ordinary_answers_are_not_refusals now includes your exact row-matching phrases (you were right that it previously dodged every row), and test_short_bulleted_claude_answers_are_not_refusals pins the same at the run_agent_step seam.

On your step 3: an answer that is literally API Error: 404 Not Found is still classified — because API Error: <4xx/5xx> is precisely haofeif's P1 (the real refusal must fire) and the two are byte-identical from the capture alone. We chose the evidence-backed narrow signature over dropping the signal, and flagging the residual rather than hiding it: a non-textual Claude signal (isApiErrorMessage in the session JSONL, or the pre-strip_terminal_escapes error styling) would separate them, but neither is on this seam today — the runtime companion BaseProvider.get_error_message also reads only the terminal buffer.

P2-2 (leaked terminals) — fixed.

• run_agent_step's provider_error branch now honours teardown=True: it runs if teardown and created_here: await _best_effort_teardown(terminal_id, registry) before raising, and still attaches terminal_id to the exception. (agent_step.py) It follows your preferred option.
• The CLI there is healthy and idle and the refusal text already rides the exception (Workflow runtime must classify in-band provider errors as step failures, not completed steps #638 criterion 2), so there is no pane worth keeping; the old "retain for inspection" comment was wrong because the drive loop overwrites st.terminal_id every attempt, leaving the earlier ones as unreferenced live CLIs.
• Regression: test_provider_error_retries_do_not_leak_terminals drives the REAL run_agent_step through the YAML retry loop (terminal layer mocked). It fails on the old head (3 created, 0 torn down) and passes now (3 created, 3 reclaimed). test_provider_error_raises_and_tears_the_terminal_down and test_provider_error_on_a_reused_terminal_is_not_torn_down pin the seam and the reuse scope.

P3-1 (context fetched on every step) — fixed. run_agent_step now calls the cheap two-argument classifier first and only fetches get_output_context when a row actually matches (then re-classifies). test_raw_context_is_not_fetched_for_a_non_candidate_answer pins it, so ordinary answers never pay for the metadata read / capture-pane.

P3-2 (get_output_context untested) — fixed. Added TestGetOutputContext covering all four branches (no metadata / rolling buffer / backend-history fallback / exception). With P3-1 the happy-path tests no longer reach the real function at all — verified by making the real get_output_context raise and re-running test_agent_step.py (all 76 still pass).

Verification: 1150 passed, 3 skipped, 1 xfailed (classifier, agent_step, workflow_service, script_runner, settlement_rewire, journal_step_lifecycle, run_step API, workflow_runs API, CLI workflow, run migration, kimi, minimax, claude_code, codex); 546 passed (workflow/replay/journal, terminal_service, cao_workflow shim, examples). black + isort clean on the six touched files; mypy clean on both source files.

Thanks for the precise reproduction — the rendering table and the start_run terminal-count probe made both fixes unambiguous.

[haofeif]

Re-review at 5f0f7055 (merge-base 5828bd23): no P1/P2 remaining; one P3 inline.

Verified fixed — my review 5399562965

• P1 (ownership): _ERRORS_RENDERED_ON_ASSISTANT_MARKER scopes the marker exemption to ("claude_code", "api_error"), and the codex-only unexpected_status row matches Codex's own ■ / ⚠️ stream error chrome. Traced through the real Codex extractor: with no • after the user line the response starts at the next line, so those lines reach the classifier; a retry banner followed by a • answer stays an answer.
• P1 (test): test_real_cli_captures_are_classified_through_the_adapters runs the real ClaudeCodeProvider / CodexProvider extractors over the upstream renders (anthropics/claude-code#91345, anthropics/claude-code#92316, openai/codex#6933, openai/codex#4270); marked short answers and unchromed Codex text stay answers.
• P3: the run-step description and docstring now document provider_error → 502.

Verified fixed — fanhongy's 5399933356: the Claude marker exemption no longer covers other rows (429 Too Many Requests etc. complete); a terminal created by the call is torn down on provider_error while reused terminals are untouched (YAML retry test: 3 created / 3 reclaimed); the raw context is fetched only for a candidate match; TestGetOutputContext pins all four branches.

Accepted residual (not re-raising): a Claude answer whose first line is literally API Error: <4xx/5xx> … (≤512 chars) is byte-identical to the real refusal and still classifies, as the author documented.

Evidence: at this head, the 11 PR-touched test files pass locally (526 passed), plus the kimi_cli/minimax_code provider suites (864 passed, 3 skipped).

Merge gate: CI, Secret Scan and cargo-deny at 5f0f7055 are action_required (fork), so no CI has run on this head; the branch is also behind main.

[haofeif]

[P3] terminal_id is no longer always a live terminal. Since 5f0f7055, run_agent_step tears down a terminal it created before raising kind="provider_error" (agent_step.py L833-834). With the default teardown=True, this 502's terminal_id names a terminal that is already deleted, yet the contract here still promises "the live terminal". The same promise is now stale in:

• the docstring below (L4197-4198, "so a caller can report/clean it up");
• StepExecutionError (agent_step.py L136) and the Raises: note (L579);
• replay_single_step (workflow_service.py L1694 "leaves its worker ALIVE", L1805 "THE WORKER IS STILL RUNNING AT THIS POINT");
• outside this diff, utils/orchestration.py L1052-1053 and _cleanup_remote_terminal (L356): "run_agent_step only tears down on success".

Runtime behaviour is safe: remote cleanup treats the 404 as cleaned, and the replay's second teardown is best-effort and does not re-emit post_kill_terminal. Suggest stating that for provider_error a terminal created by the call is already torn down (unless teardown=False or reused), so terminal_id is for reporting only.

[haofeif]

Approving at 5f0f7055: every P1/P2 from my earlier reviews is verified fixed (details in review 5400426612). The remaining P3 (stale terminal_id "live terminal" wording) is non-blocking. CI has not run at this head yet because the fork workflows are awaiting maintainer approval.

[Harbor404]

Synced the branch with latest main to clear BEHIND.

• Merged origin/main (55dd4a0c) into fix/workflow-inband-provider-error with git merge; no rebase or force-push.
• No conflicts. The merge brought in the upstream CodeQL/CI/docs changes only; no PR business-code changes.
• Head updated: 5f0f7055 -> efb400e1 (chore(merge): sync with latest main).
• Focused verification on efb400e1: 643 passed across classifier, agent_step, workflow_service, run-step/workflow-runs API, CLI workflow, journal step lifecycle, Kimi, MiniMax, script runner, settlement rewire, and workflow-run migration tests.

Code side is approved by @haofeif at 5f0f7055; the remaining CHANGES_REQUESTED decision is from the older review and needs maintainer dismissal. No code changes were made for the remaining P3.

[Harbor404]

已 sync main，PR 改动仍就绪，head 90e7623a。本轮无实质代码改动；受影响模块 focused 测试 526 passed。此前 P1/P2 已处理并由 haofeif approve。