Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 147 additions & 22 deletions apps/backend/qa/fixer.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
)
from claude_agent_sdk import ClaudeSDKClient
from core.client import create_client
from core.model_fallback import MODEL_FALLBACK_CHAIN
from core.model_fallback import MODEL_FALLBACK_CHAIN, get_fallback_model
from core.providers import create_engine_provider
from core.providers.base import SessionConfig
from core.providers.config import ProviderConfig
Expand Down Expand Up @@ -106,15 +106,17 @@
spec_dir: Path,
fix_session: int,
verbose: bool = False,
recovery_guidance: str | None = None,
) -> tuple[str, str]:
"""Run one QA fixer pass through the provider-neutral runtime layer.

Mirror of :func:`run_qa_fixer_session` for direct-API providers. Unlike
the Claude path it does NOT reimplement the Claude-SDK recovery / model-
fallback loop: it does a single ``generic_edit`` pass (which already
provides mutation snapshots + transaction rollback) and relies on the
outer QA loop for re-validation and retries. Success is the file-based
``is_fixes_applied`` signal, so the verdict matches the Claude path.
The single-attempt primitive: it does one ``generic_edit`` pass (which
already provides mutation snapshots + transaction rollback). The
recovery / model-fallback loop that brings this to Claude parity lives in
:func:`run_qa_fixer_runtime_session`, which calls this once per attempt
and threads strategy guidance back in via ``recovery_guidance``. Success
is the file-based ``is_fixes_applied`` signal, so the verdict matches the
Claude path.

Returns ``(status, response_text)`` with ``"fixed"`` / ``"error"``.
"""
Expand Down Expand Up @@ -151,6 +153,8 @@
spec_dir=spec_dir,
fix_session=fix_session,
)
if recovery_guidance:
prompt += f"\n\n## RECOVERY STRATEGY\n\n{recovery_guidance}\n"

result = await run_runtime_session(
runtime_session,
Expand Down Expand Up @@ -186,21 +190,18 @@
return "error", "QA fixer did not apply fixes (runtime path)"


async def run_qa_fixer_runtime_session(
def _build_qa_fixer_runtime_session(
*,
provider_name: str,
runtime_mode: str,
model: str,
project_dir: Path,
spec_dir: Path,
fix_session: int,
verbose: bool = False,
) -> tuple[str, str]:
"""Build a direct-provider runtime session and run the QA fixer on it.
) -> object:
"""Build a direct-provider runtime session for one QA fixer attempt.

Thin orchestration shim mirroring ``run_qa_reviewer_runtime_session``;
builds the provider session + runtime adapter then delegates to
:func:`run_qa_fixer_via_runtime`.
Rebuilt per recovery attempt so model fallback can swap the underlying
provider model between attempts.
"""
from agents.runtime import create_runtime_session

Expand All @@ -218,19 +219,143 @@
extra={"agent_type": "qa_fixer"},
)
)
runtime_session = create_runtime_session(
return create_runtime_session(
provider_name=provider_name,
agent_session=session,
runtime_mode=runtime_mode,
project_dir=project_dir,
agent_type="qa_fixer",
)
return await run_qa_fixer_via_runtime(
runtime_session,
project_dir,
spec_dir,
fix_session,
verbose=verbose,


async def run_qa_fixer_runtime_session(

Check failure on line 231 in apps/backend/qa/fixer.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 36 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=OBenner_Auto-Coding&issues=AZ6n88YnxmPCWMBDs0SC&open=AZ6n88YnxmPCWMBDs0SC&pullRequest=316
*,
provider_name: str,
runtime_mode: str,
model: str,
project_dir: Path,
spec_dir: Path,
fix_session: int,
verbose: bool = False,
) -> tuple[str, str]:
"""Run a QA fixer on a direct-provider runtime session, with recovery.

Brings the direct-API qa_fixer to Claude parity: wraps the single-pass
:func:`run_qa_fixer_via_runtime` in the same ``RecoveryManager`` loop the
Claude path uses — exponential backoff, model fallback (rebuilding the
runtime session with the next model in ``MODEL_FALLBACK_CHAIN``), commit
rollback, strategy guidance threaded into the next prompt, and
skip/escalate to the dead-letter queue when recovery is exhausted.

Returns ``(status, response_text)`` where status is ``"fixed"``,
``"stuck"``, or ``"escalate"``.
"""
recovery_manager = RecoveryManager(spec_dir=spec_dir, project_dir=project_dir)
fixer_subtask_id = f"qa_fixer_{fix_session}"

pending_recovery_action: RecoveryAction | None = None
current_model = model
recovery_guidance: str | None = None
last_error: str | None = None

for fixer_iteration in range(1, MAX_FIXER_ITERATIONS + 1):
# Apply any backoff / rollback queued by the previous attempt.
if pending_recovery_action is not None:
if pending_recovery_action.wait_seconds > 0:
await asyncio.sleep(pending_recovery_action.wait_seconds)
if pending_recovery_action.action == "rollback" and (
pending_recovery_action.target
):
recovery_manager.rollback_to_commit(pending_recovery_action.target)
pending_recovery_action = None

recovery_manager.record_attempt(
fixer_subtask_id,
session=fix_session,
success=False,
approach=(
f"QA fixer runtime session {fix_session}, "
f"iteration {fixer_iteration} (model={current_model})"
),
)

try:
runtime_session = _build_qa_fixer_runtime_session(
provider_name=provider_name,
runtime_mode=runtime_mode,
model=current_model,
project_dir=project_dir,
fix_session=fix_session,
)
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
Comment on lines +290 to +298

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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 = None

Based 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.

if status == "fixed":
recovery_manager.record_outcome(fixer_subtask_id, success=True)
return "fixed", response_text
last_error = response_text or "QA fixer did not apply fixes (runtime path)"
error_message = f"QA fixer runtime error: {last_error}"
except Exception as e: # noqa: BLE001 - classify and recover, never crash

Check warning on line 304 in apps/backend/qa/fixer.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Fix the syntax of this issue suppression comment.

See more on https://sonarcloud.io/project/issues?id=OBenner_Auto-Coding&issues=AZ6n88YnxmPCWMBDs0SD&open=AZ6n88YnxmPCWMBDs0SD&pullRequest=316
last_error = str(e)
error_message = f"QA fixer runtime error: {e}"
debug_error("qa_fixer", error_message)

failure_type = recovery_manager.classify_failure(
error_message, fixer_subtask_id
)
recovery_action = recovery_manager.determine_recovery_action(
failure_type, fixer_subtask_id
)
recovery_manager.record_recovery_notification(
fixer_subtask_id, failure_type, recovery_action
)

if recovery_action.action == "retry":
pending_recovery_action = recovery_action
if recovery_action.use_model_fallback:
fallback_model = get_fallback_model(current_model)
if fallback_model:
current_model = fallback_model
if recovery_action.strategy:
recovery_guidance = recovery_action.strategy.guidance
continue
if recovery_action.action == "rollback":
pending_recovery_action = recovery_action
continue
if recovery_action.action == "continue":
continue
if recovery_action.action == "skip":
recovery_manager.mark_subtask_stuck(
fixer_subtask_id, recovery_action.reason
)
recovery_manager.record_outcome(
fixer_subtask_id, success=False, error=last_error
)
return "stuck", f"Fixer stuck: {recovery_action.reason}"
if recovery_action.action == "escalate":
recovery_manager.mark_subtask_stuck(
fixer_subtask_id, recovery_action.reason
)
recovery_manager.record_outcome(
fixer_subtask_id, success=False, error=last_error
)
return "escalate", recovery_action.reason
# Unknown action — retry defensively on the next iteration.

recovery_manager.record_outcome(
fixer_subtask_id,
success=False,
error=last_error or "Max fixer iterations reached",
)
return (
"stuck",
f"Fixer stuck after exhausting all recovery attempts: {last_error}",
)


Expand Down
Loading
Loading