[jcarosi]

Why

Claude Code can emit [claude-code:unrecognized_model] for a working gateway alias while returning a successful JSON result. Its official error reference describes this as a diagnostic that does not stop the request or switch models.

Upstream 9.3.2 (272a4020) still classifies the stderr marker as model unavailability before honoring the successful result. Flow discards the review text and rejects the result, or can enter the availability ladder when the model was not explicitly pinned.

Review focus

Start with run_claude_exec and _parse_claude_result in plugins/flow-next/scripts/flowctl.py, then the handler-level regressions in test_claude_review_commands.py.

A valid zero-exit success envelope takes precedence over the warning. Malformed envelopes, error results, nonzero process exits and timeouts still cannot supply a verdict. Genuine unavailable-model responses retain the existing bounded resolution behavior, and explicit model selections never silently change.

This existing PR also carries its previously reviewed shared fallback-cache correction: only successful fallback executions are cached, and failed cached selections are invalidated. The latest update adds two Claude regressions and merges upstream 9.3.2; it adds no new backend behavior, configuration or fallback.

Needs you

Repository-wide flowctl validate --all still reports 23 legacy errors in fn-16-ugn and fn-24-codex-first-class-citizen-native-plugin. The error set is identical on unmodified upstream 9.3.2. These historical records are outside this correction.

The upstream maintainer retains the merge decision. No hosted model review was requested.

Reproduce without a provider

The fake Claude process returns:

exit status: 0
stdout: {"type":"result","subtype":"success","is_error":false,"result":"<verdict>SHIP</verdict>","session_id":"11111111-2222-3333-4444-555555555555"}
stderr: [claude-code:unrecognized_model] {"model":"gateway-custom-model","query_source":"sdk"}

Run the real-handler regression:

PYTHONPATH=plugins/flow-next/tests python3 -m unittest test_claude_review_commands.ClaudeImplReview.test_ship_with_unknown_model_warning_keeps_explicit_model

With the regression transplanted onto unmodified upstream 272a4020, Flow returns 2 instead of the expected 0. On this branch it writes a SHIP receipt, retains claude:gateway-custom-model:medium, dispatches exactly once and creates no availability-cache entry. No LLM is invoked.

The negative cases cover malformed JSON and wrong-typed fields, genuine model errors, nonzero exits, and a timeout carrying partial SHIP output plus the warning. Failed attempts publish no verdict or new valid receipt, refund their reservation and do not silently fall back.

Verified

• Exact SHIP regression: red on unmodified upstream 272a4020, green with the correction.
• Focused Claude and model-resolution suites: 96 tests passed.
• python3 scripts/run_tests_parallel.py: 4,082 tests across 218 files; 0 failures, 0 errors, 7 skipped.
• uvx ruff@0.16.0 check ., tracker manifest freshness, Codex mirror freshness, documentation anchors and git diff --check: passed.
• Both contribution specs validate individually without errors or warnings.
• Independent packaged Flow review via codex:gpt-6.1-sol:xhigh: SHIP, no findings, finalized for candidate a0c9cab5 against upstream 272a4020. The reviewer completed its inspection and in-memory checks; its read-only sandbox prevented the temporary-directory suites, which passed in the coordinator's full run.
• Current GitHub CI will run on the pushed candidate; earlier CI results belong to the earlier head.

The historical Flow records remain bound to their recorded snapshots. The new review covers the current integrated candidate. This PR contains no private incident logs or application changes, and does not modify shared installations or permanent model routing.

Archived FN-13 offline replay

The original Claude JSON stdout was not retained. An offline replay placed the preserved complete result text and warning into a synthetic success envelope; the installed Flow-Next 9.1.1 adapter and unmodified upstream 9.3.2 rejected it, while PR head a0c9cab5 accepted it after one mocked dispatch. Synthetic negative cases covered genuine 404 model errors, nonzero transport exits, malformed JSON, missing fields, and error envelopes. This replay invoked no LLM and created no official review receipt.

[Copilot]

🟡 Changes recommended

A failed ladder-floor execution is still cached, causing later reviews to bypass stronger models and retry the failed floor.

1 open finding

• Avoid caching failed floor dispatch results · New

What changed in this PR

This PR hardens Claude review result classification, preserving valid warning-bearing reviews while rejecting malformed or failed executions.

Changes:

• Prioritizes valid zero-exit Claude result envelopes over stderr diagnostics.
• Adds regression coverage for review finalization, failures, fallback, caching, and sessions.
• Updates transport documentation, generated mirrors, changelog, and Flow records.

File	Description
plugins/​flow-next/​scripts/​flowctl.py	Implements strict Claude result and exit classification.
plugins/​flow-next/​tests/​test_claude_review_commands.py	Tests official review handlers and failure accounting.
plugins/​flow-next/​tests/​test_model_resolution.py	Updates the stderr-only fallback fixture.
plugins/​flow-next/​scripts/​flowctl_tracker/​MANIFEST.json	Refreshes the generated source hash.
plugins/​flow-next/​docs/​flowctl.md	Documents Claude transport and fallback rules.
plugins/​flow-next/​docs/​troubleshooting.md	Adds operator guidance for warning-bearing results.
plugins/​flow-next/​codex/​docs/​flow-next/​flowctl.md	Mirrors the runtime documentation.
plugins/​flow-next/​codex/​docs/​flow-next/​troubleshooting.md	Mirrors troubleshooting guidance.
CHANGELOG.md	Records the user-facing fix.
.flow/​specs/​fn-291-preserve-successful-claude-reviews-with.md	Defines requirements and boundaries.
.flow/​specs/​fn-291-preserve-successful-claude-reviews-with.json	Records specification state and review history.
.flow/​tasks/​fn-291-preserve-successful-claude-reviews-with.1.md	Captures completion evidence.
.flow/​tasks/​fn-291-preserve-successful-claude-reviews-with.1.json	Records task metadata and status.

🧠 Review effort: Balanced

---

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

[jcarosi]

Thanks for the review. I corrected the documentation findings: the troubleshooting example now matches the emitted fallback warning and closes its parenthetical; the fallback description no longer suggests the stderr tag rescues a non-matching error envelope; and the changelog now records successful-only fallback caching. The Codex documentation mirrors were regenerated.

I left the non-blocking P3 implementation cleanup suggestions unchanged. They identify redundant checks and internal diagnostic state, but no incorrect user-visible behavior; changing the classifier or dispatch state was not needed to address the reported documentation defects.

Validation after merging upstream 9.2.0: the focused Claude and model-resolution suites passed (27 and 67 tests), the full suite passed (4,083 tests, zero failures or errors, 7 skipped), and Ruff, the generated Codex mirror check, and the tracker manifest check passed. After the documentation-only corrections, mirror generation and its check, Ruff, and git diff --check passed again. Independent Copilot review with Claude Opus 5.5 at high effort returned SHIP for the three-axis implementation review, the scoped documentation correction review, and the FN291/FN292 completion review. The published head is b70df92c; it contains the upstream merge at 342334ec, documentation correction at b8b9a916, and review-record commit b70df92c. All hosted checks passed on that exact head, including the CI aggregate, unit suites on Linux/macOS/Windows, three-OS smoke tests, Python compatibility, and the Windows stub. The CI run and documentation link check both completed successfully.