feat(qa): bring direct-API qa_fixer to Claude recovery parity - #316
Merged
Conversation
The runtime qa_fixer path (run_qa_fixer_via_runtime) did a single generic_edit pass with no recovery — unlike the Claude qa_fixer, which wraps each attempt in a RecoveryManager loop (exponential backoff, model fallback, commit rollback, strategy guidance, skip/escalate to the dead-letter queue). That parity gap is one reason QA roles stayed behind the AUTO_CODE_QA_DIRECT_RUNTIME opt-in. Close the gap: - run_qa_fixer_runtime_session now drives the same RecoveryManager loop the Claude path uses: up to MAX_FIXER_ITERATIONS attempts, classify -> determine_recovery_action -> dispatch (retry/backoff, model fallback via get_fallback_model + MODEL_FALLBACK_CHAIN, rollback, continue, skip->stuck, escalate->dead-letter). Model fallback rebuilds the runtime session with the next provider model between attempts. - Extracted _build_qa_fixer_runtime_session so each attempt can swap the model; the single-pass run_qa_fixer_via_runtime stays the per-attempt primitive and gained an optional recovery_guidance arg threaded into the next prompt. - The Claude path (run_qa_fixer_session) is untouched. Tests: 4 new recovery-loop cases (model fallback -> success, guidance threading, escalate, skip->stuck) plus the existing 3 pass — 10/10 in tests/test_qa_fixer_runtime.py. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Oleg Miagkov <mrobenner@gmail.com>
Contributor
📝 WalkthroughWalkthroughThe QA fixer runtime gains a recovery loop powered by ChangesRecovery Loop and Fallback
Sequence DiagramsequenceDiagram
participant Caller
participant RecoveryLoop as run_qa_fixer_runtime_session
participant RecoveryMgr as RecoveryManager
participant SessionBuilder as _build_qa_fixer_runtime_session
participant SinglePass as run_qa_fixer_via_runtime
Caller->>RecoveryLoop: call with config
RecoveryLoop->>RecoveryMgr: init with escalation/backoff strategy
loop attempt until fixed/stuck/escalate
RecoveryLoop->>SessionBuilder: build runtime session
SessionBuilder->>SinglePass: return configured session
RecoveryLoop->>SinglePass: run with recovery_guidance from prior action
SinglePass-->>RecoveryLoop: return fixed/error result
alt transient error
RecoveryLoop->>RecoveryMgr: classify and request recovery
RecoveryMgr-->>RecoveryLoop: retry action with next fallback model/strategy
RecoveryLoop->>RecoveryLoop: update current_model, queue backoff
else fatal error or escalate
RecoveryLoop->>RecoveryMgr: request escalate action
RecoveryMgr-->>RecoveryLoop: escalate action with reason
RecoveryLoop-->>Caller: return (escalate, reason)
else skip/stuck
RecoveryLoop->>RecoveryMgr: record failed outcome
RecoveryLoop-->>Caller: return (stuck, reason)
else fixed
RecoveryLoop-->>Caller: return (fixed, result)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/qa/fixer.py`:
- Around line 290-298: The recovery_guidance value must be consumed (cleared)
before awaiting run_qa_fixer_via_runtime to avoid leaking the same guidance into
later retries; change the call site so you capture the current guidance into a
local (e.g., rg = recovery_guidance), immediately set recovery_guidance = None,
and then pass that local (rg) into run_qa_fixer_via_runtime; ensure you
reference the existing symbols run_qa_fixer_via_runtime, recovery_guidance, and
the surrounding fix_session/runtime_session context so the one-shot semantics
match the approach used in agents.coder.
In `@tests/test_qa_fixer_runtime.py`:
- Around line 185-309: Add tests that exercise the queued recovery branch and
the exception-path retry behavior: create a _FakeRecoveryManager script that
returns a pending recovery action with wait_seconds and a flag to trigger
rollback, then assert that run_qa_fixer_via_runtime is called again after the
pending action and that the session's rollback_to_commit (or equivalent) was
invoked on the next attempt; also add a test where run_qa_fixer_via_runtime
raises an exception on the first call and returns ("fixed", ...) on retry, and
assert the RecoveryManager's strategy guidance is consumed exactly once (first
retry gets the guidance, subsequent calls do not), referencing
run_qa_fixer_runtime_session, run_qa_fixer_via_runtime, _FakeRecoveryManager,
and _recovery_action to locate the code under test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a274974-4375-4497-a5ad-dc32f3203188
📒 Files selected for processing (2)
apps/backend/qa/fixer.pytests/test_qa_fixer_runtime.py
Comment on lines
+290
to
+298
| status, response_text = await run_qa_fixer_via_runtime( | ||
| runtime_session, | ||
| project_dir, | ||
| spec_dir, | ||
| fix_session, | ||
| verbose=verbose, | ||
| recovery_guidance=recovery_guidance, | ||
| ) | ||
| recovery_guidance = None |
Contributor
There was a problem hiding this comment.
Consume recovery_guidance before awaiting the attempt.
run_qa_fixer_via_runtime() uses the guidance while building the prompt. If that await raises, Line 298 never executes, so the same guidance leaks into the next recovery attempt and can stack stale instructions on later retries.
🐛 Proposed fix
- status, response_text = await run_qa_fixer_via_runtime(
+ attempt_guidance = recovery_guidance
+ recovery_guidance = None
+ status, response_text = await run_qa_fixer_via_runtime(
runtime_session,
project_dir,
spec_dir,
fix_session,
verbose=verbose,
- recovery_guidance=recovery_guidance,
+ recovery_guidance=attempt_guidance,
)
- recovery_guidance = NoneBased on learnings, apps/backend/agents/coder.py:1195-1200 clears recovery guidance immediately after adding the ## RECOVERY STRATEGY section, so this runtime path should preserve the same one-shot contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/qa/fixer.py` around lines 290 - 298, The recovery_guidance value
must be consumed (cleared) before awaiting run_qa_fixer_via_runtime to avoid
leaking the same guidance into later retries; change the call site so you
capture the current guidance into a local (e.g., rg = recovery_guidance),
immediately set recovery_guidance = None, and then pass that local (rg) into
run_qa_fixer_via_runtime; ensure you reference the existing symbols
run_qa_fixer_via_runtime, recovery_guidance, and the surrounding
fix_session/runtime_session context so the one-shot semantics match the approach
used in agents.coder.
Comment on lines
+185
to
+309
| @pytest.mark.asyncio | ||
| async def test_runtime_fixer_recovers_via_model_fallback(tmp_path): | ||
| """A failed attempt retries on the next model in the fallback chain.""" | ||
| from qa import fixer | ||
|
|
||
| built_models: list[str] = [] | ||
|
|
||
| def fake_build(*, provider_name, runtime_mode, model, project_dir, fix_session): | ||
| built_models.append(model) | ||
| return MagicMock() | ||
|
|
||
| via = AsyncMock(side_effect=[("error", "boom"), ("fixed", "done")]) | ||
| fake_rm = _FakeRecoveryManager( | ||
| action_script=[_recovery_action("retry", use_model_fallback=True)] | ||
| ) | ||
|
|
||
| with ( | ||
| patch("qa.fixer._build_qa_fixer_runtime_session", side_effect=fake_build), | ||
| patch("qa.fixer.run_qa_fixer_via_runtime", new=via), | ||
| patch("qa.fixer.RecoveryManager", return_value=fake_rm), | ||
| ): | ||
| status, response = await fixer.run_qa_fixer_runtime_session( | ||
| provider_name="openai", | ||
| runtime_mode="generic_edit", | ||
| model="gpt-5.2", | ||
| project_dir=tmp_path, | ||
| spec_dir=tmp_path, | ||
| fix_session=1, | ||
| ) | ||
|
|
||
| assert status == "fixed" | ||
| assert response == "done" | ||
| # Second attempt rebuilt the session with the next model in the chain. | ||
| assert built_models == ["gpt-5.2", "gpt-5"] | ||
| assert via.await_count == 2 | ||
| assert fake_rm.outcomes == [True] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_runtime_fixer_threads_recovery_guidance_into_next_attempt(tmp_path): | ||
| """Strategy guidance from a retry action is passed into the next prompt.""" | ||
| from qa import fixer | ||
|
|
||
| via = AsyncMock(side_effect=[("error", "boom"), ("fixed", "done")]) | ||
| strategy = types.SimpleNamespace(guidance="TRY HARDER", description="d") | ||
| fake_rm = _FakeRecoveryManager( | ||
| action_script=[_recovery_action("retry", strategy=strategy)] | ||
| ) | ||
|
|
||
| with ( | ||
| patch("qa.fixer._build_qa_fixer_runtime_session", return_value=MagicMock()), | ||
| patch("qa.fixer.run_qa_fixer_via_runtime", new=via), | ||
| patch("qa.fixer.RecoveryManager", return_value=fake_rm), | ||
| ): | ||
| await fixer.run_qa_fixer_runtime_session( | ||
| provider_name="openai", | ||
| runtime_mode="generic_edit", | ||
| model="gpt-5.2", | ||
| project_dir=tmp_path, | ||
| spec_dir=tmp_path, | ||
| fix_session=1, | ||
| ) | ||
|
|
||
| # First attempt: no guidance; second attempt: guidance threaded in. | ||
| assert via.await_args_list[0].kwargs["recovery_guidance"] is None | ||
| assert via.await_args_list[1].kwargs["recovery_guidance"] == "TRY HARDER" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_runtime_fixer_escalates_when_recovery_exhausted(tmp_path): | ||
| """An escalate action returns ('escalate', reason) for the dead-letter queue.""" | ||
| from qa import fixer | ||
|
|
||
| via = AsyncMock(return_value=("error", "boom")) | ||
| fake_rm = _FakeRecoveryManager( | ||
| action_script=[_recovery_action("escalate", reason="dead-letter")] | ||
| ) | ||
|
|
||
| with ( | ||
| patch("qa.fixer._build_qa_fixer_runtime_session", return_value=MagicMock()), | ||
| patch("qa.fixer.run_qa_fixer_via_runtime", new=via), | ||
| patch("qa.fixer.RecoveryManager", return_value=fake_rm), | ||
| ): | ||
| status, response = await fixer.run_qa_fixer_runtime_session( | ||
| provider_name="openai", | ||
| runtime_mode="generic_edit", | ||
| model="gpt-5.2", | ||
| project_dir=tmp_path, | ||
| spec_dir=tmp_path, | ||
| fix_session=1, | ||
| ) | ||
|
|
||
| assert status == "escalate" | ||
| assert response == "dead-letter" | ||
| assert via.await_count == 1 | ||
| assert fake_rm.outcomes == [False] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_runtime_fixer_marks_stuck_on_skip(tmp_path): | ||
| """A skip action returns ('stuck', ...) and records a failed outcome.""" | ||
| from qa import fixer | ||
|
|
||
| via = AsyncMock(return_value=("error", "boom")) | ||
| fake_rm = _FakeRecoveryManager( | ||
| action_script=[_recovery_action("skip", reason="circular_fix")] | ||
| ) | ||
|
|
||
| with ( | ||
| patch("qa.fixer._build_qa_fixer_runtime_session", return_value=MagicMock()), | ||
| patch("qa.fixer.run_qa_fixer_via_runtime", new=via), | ||
| patch("qa.fixer.RecoveryManager", return_value=fake_rm), | ||
| ): | ||
| status, response = await fixer.run_qa_fixer_runtime_session( | ||
| provider_name="openai", | ||
| runtime_mode="generic_edit", | ||
| model="gpt-5.2", | ||
| project_dir=tmp_path, | ||
| spec_dir=tmp_path, | ||
| fix_session=1, | ||
| ) | ||
|
|
||
| assert status == "stuck" | ||
| assert "circular_fix" in response | ||
| assert fake_rm.outcomes == [False] |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add coverage for the queued recovery branch and exception-path retries.
These cases only exercise retry/skip/escalate after run_qa_fixer_via_runtime() returns ("error", ...). They never assert the pending_recovery_action branch that applies wait_seconds and rollback_to_commit() on the next iteration, and they do not cover the exception path where guidance must be consumed exactly once. A regression in either path would currently pass this file.
As per coding guidelines, tests/**: Ensure tests are comprehensive and follow pytest conventions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_qa_fixer_runtime.py` around lines 185 - 309, Add tests that
exercise the queued recovery branch and the exception-path retry behavior:
create a _FakeRecoveryManager script that returns a pending recovery action with
wait_seconds and a flag to trigger rollback, then assert that
run_qa_fixer_via_runtime is called again after the pending action and that the
session's rollback_to_commit (or equivalent) was invoked on the next attempt;
also add a test where run_qa_fixer_via_runtime raises an exception on the first
call and returns ("fixed", ...) on retry, and assert the RecoveryManager's
strategy guidance is consumed exactly once (first retry gets the guidance,
subsequent calls do not), referencing run_qa_fixer_runtime_session,
run_qa_fixer_via_runtime, _FakeRecoveryManager, and _recovery_action to locate
the code under test.
Source: Coding guidelines
This was referenced Jun 8, 2026
OBenner
added a commit
that referenced
this pull request
Jun 12, 2026
Live-build finding (2026-06-12): when a gpt-5.2 coder session failed and the recovery manager recommended model fallback, the inline fallback block guessed a Claude shorthand from the model name and defaulted ANY non-Claude model to "sonnet". The chain then yielded "haiku", resolve_model_id() expanded it to claude-haiku-4-5-20251001, and the retry sent a Claude model id to the OpenAI provider — 404 model_not_found, fatal for the whole build. Use core.model_fallback.get_fallback_model() instead: it looks up the ACTUAL current model and returns its same-provider fallback (gpt-5.2 -> gpt-5, claude-sonnet-4-5 -> claude-haiku-4-5, gemini-2.5-flash -> gemini-2.0-flash). This is the same helper the QA fixer already uses for its retry parity (#316), so coder recovery now matches that behavior. Signed-off-by: Oleg Miagkov <mrobenner@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OBenner
added a commit
that referenced
this pull request
Jun 13, 2026
Live-build finding (2026-06-13, spec 901): with QA roles routed to a direct provider (qa_reviewer/qa_fixer on openai via the promotion gate, now that #317 dropped the opt-in), the QA runtime session was built with the QA phase-model DEFAULT — which phase_config resolves to a claude-* id — and OpenAI 404'd on claude-sonnet-4-5-20250929. This is the QA-path analog of the coder fix in #337: once routing correctly sends QA to the direct provider, the Claude-centric phase default leaks straight into the session. New ProviderConfig.coherent_session_model() returns the provider's own configured model when the requested model is a Claude-family id and the provider is non-Claude; otherwise it passes the request through unchanged. Both QA runtime shims (run_qa_reviewer_runtime_session, _build_qa_fixer_runtime_session) now route their model through it before create_session. The qa_fixer recovery loop already uses get_fallback_model() (#316 parity), so once the initial model is the provider's, the fallback chain (gpt-5.2 -> gpt-5 -> ...) stays coherent. Tests: 5 helper cases (claude->provider, explicit preserved, None, claude-provider unchanged, no-configured-model keeps request) + existing QA runtime / factory suites. 85 passed. Signed-off-by: Oleg Miagkov <mrobenner@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Half of the autonomy goal is non-coder agent promotion.
qa_reviewer/qa_fixeralready run through the runtime layer, but behind a double gate:AUTO_CODE_QA_DIRECT_RUNTIME=trueopt-in and the direct-API promotion gate. A key reason the manual opt-in still exists: the runtimeqa_fixerwas a singlegeneric_editpass with no recovery, unlike the Claudeqa_fixerwhich wraps each attempt in aRecoveryManagerloop.This PR closes that parity gap (the prerequisite for dropping the opt-in in a follow-up).
What changes (
apps/backend/qa/fixer.py)run_qa_fixer_runtime_sessionnow drives the sameRecoveryManagerloop the Claude path uses:MAX_FIXER_ITERATIONSattempts,classify_failure→determine_recovery_action→ dispatch: retry (exponential backoff), model fallback (get_fallback_model+MODEL_FALLBACK_CHAIN, e.g.gpt-5.2→gpt-5→gpt-4o), rollback to a commit, continue, skip→stuck, escalate→dead-letter queue._build_qa_fixer_runtime_sessionextracted so each attempt can swap the model.run_qa_fixer_via_runtimestays the per-attempt primitive; gained an optionalrecovery_guidancearg threaded into the next prompt (mirrors the Claude path's## RECOVERY STRATEGYinjection).run_qa_fixer_session) is untouched — zero risk to the existing flow.Tests
4 new recovery-loop cases + the existing 3 → 10/10 in
tests/test_qa_fixer_runtime.py:escalate→("escalate", reason),skip→("stuck", …).tests/test_qa_runtime_integration.py(15) +tests/test_qa_reviewer_runtime.pyalso green.Next
Follow-up PR: drop the
AUTO_CODE_QA_DIRECT_RUNTIMEopt-in now thatqa_fixeris at parity, so QA roles promote on the evidence gate alone (driven byAUTO_CODE_AUTONOMY=safe).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests