fix(qa): persist direct-API qa_fixer status via a qa_status.json artifact - #342
Merged
Conversation
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 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 |
| ``fixes_applied`` artifact was merged and persisted. | ||
| """ | ||
| # Lazy import to avoid the qa.criteria -> ... -> qa.reviewer/fixer cycle. | ||
| from .criteria import load_implementation_plan, save_implementation_plan |
…fact The generic_edit qa_fixer could not record "fixes_applied" on a non-Claude provider, so a REJECTED -> fix -> re-QA loop on e.g. OpenAI stalled. The qa_fixer.md prompt tells the model to write qa_signoff into implementation_plan.json with Claude's Write/Edit tools and absolute paths, but on the generic_edit runtime those tools don't exist (the real tools are write_file/replace_text) and resolve_workspace_path rejects absolute paths, so ready_for_qa_revalidation was never set and is_fixes_applied stayed false. - build_qa_fixer_prompt gains runtime_status_relspec; when set it appends a generic_edit addendum that OVERRIDES the Claude Write/Edit/absolute-path guidance: edit with write_file using workspace-relative paths, and record completion by writing a workspace-relative qa_status.json (status=fixes_applied, ready_for_qa_revalidation, fix_session, fixes_summary) then calling finish. - run_qa_fixer_via_runtime computes the relspec via spec_dir.relative_to( project_dir) and, when is_fixes_applied is false, deterministically folds <spec_dir>/qa_status.json into qa_signoff (load/save_implementation_plan) before re-checking — matching the Claude path's file-based verdict. - run_qa_fixer_runtime_session async-closes each per-attempt provider session in the same event loop; add OpenAICompatibleSession.aclose() to await the AsyncOpenAI connection-pool close so rebuilt-per-attempt sessions don't leak sockets. Add regression tests in tests/test_qa_fixer_runtime.py: a scripted runtime session that writes qa_status.json -> "fixed" with the artifact merged into qa_signoff, the prompt-addendum content, per-attempt async close, and the OpenAICompatibleSession.aclose() primitive. Signed-off-by: Oleg Miagkov <mrobenner@gmail.com>
OBenner
force-pushed
the
claude/priceless-banach-3935b8
branch
from
June 18, 2026 09:24
3e98c5c to
664b8a8
Compare
|
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
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.



Problem
On a direct-API provider (e.g. OpenAI via the
generic_editruntime), the qa_fixer can't record its status, so aREJECTED → fix → re-QAloop stalls.prompts/qa_fixer.mdtells the model to writeqa_signoffintoimplementation_plan.jsonusing Claude'sWrite/Edittools and absolute paths — but ingeneric_editthose tools don't exist (the real tools arewrite_file/replace_text) andresolve_workspace_pathrejects absolute paths. The result:ready_for_qa_revalidationis never set,is_fixes_appliedstays false, and the fixer reportserroreven after applying real fixes.Fix
Give the direct-API fixer a file-based status contract it can actually satisfy, mirroring the file-based verdict the Claude path already relies on:
build_qa_fixer_promptgainsruntime_status_relspec. When set, ageneric_editaddendum overrides the ClaudeWrite/Edit/absolute-path guidance: edit withwrite_fileusing workspace-relative paths, and record completion by writing a workspace-relativeqa_status.json(status="fixes_applied",ready_for_qa_revalidation,fix_session,fixes_summary), thenfinish. The Claude path is unchanged (param defaults toNone).run_qa_fixer_via_runtimecomputes the spec dir relative to the project root and, whenis_fixes_appliedis false, folds<spec_dir>/qa_status.jsonintoqa_signoff(load_/save_implementation_plan), then re-checks. Pre-existing signoff fields are preserved.run_qa_fixer_runtime_sessionasync-closes each per-attempt provider session in afinally(rebuilt per attempt by the recovery loop). AddsOpenAICompatibleSession.aclose()to await the underlyingAsyncOpenAIconnection-pool close — the syncclose()only drops references, leaking sockets.Tests
tests/test_qa_fixer_runtime.py— 18 passed (10 existing + 8 new), ruff clean:New coverage: scripted runtime session writes
qa_status.json→ returns"fixed"with the artifact merged intoqa_signoff; no-artifact stays"error"(loop must not falsely pass); the prompt-addendum content; per-attempt async close; and theOpenAICompatibleSession.aclose()primitive.Follow-up (not in this PR)
The qa_reviewer runtime path (
run_qa_reviewer_via_runtime) has the same latent status-persistence bug — it also can't writeqa_signoffon a direct-API provider. Its verdict is richer (approved/rejected +issues_found), so it needs the same pattern with an extended artifact schema. Tracked separately to keep this change focused.🤖 Generated with Claude Code