fix(qa): persist direct-API QA reviewer verdict and close its async client - #340
Merged
Conversation
…lient
The direct-API (generic_edit) QA reviewer never recorded a verdict, so a
full build always exited qa_failed even when the implementation was sound.
Two causes:
- Sign-off persistence: the reviewer shares the Claude-oriented prompt,
which tells the model to write qa_signoff with the Write/Edit tools at
the absolute path {spec_dir}/implementation_plan.json. In generic_edit
those tools do not exist (the tools are write_file/replace_text) and
resolve_workspace_path rejects absolute paths, so the sign-off was never
written and get_qa_signoff_status stayed empty.
- Client lifecycle: OpenAICompatibleSession.close() only dropped the
AsyncOpenAI/httpx client reference; it was never aclose()d, leaking the
TCP transport and flooding the QA loop with "TCPTransport closed" errors
at GC time.
Fix:
- Add OpenAICompatibleSession.aclose() that awaits the client's own close()
in the same event loop, and close the reviewer session in a finally in
run_qa_reviewer_runtime_session.
- Add a runtime-only prompt addendum (build_qa_reviewer_prompt's new
runtime_signoff_relspec) that overrides the Claude guidance: use
write_file with workspace-relative paths and record the verdict in a
small qa_signoff.json. merge_runtime_qa_signoff_artifact folds that file
into implementation_plan.json deterministically, so we never rely on the
model editing the large plan JSON. The Claude SDK path is unchanged
(relspec defaults to None).
Tests: scripted runtime sessions that write the verdict via the plan and
via the qa_signoff.json side-file assert run_qa_reviewer_via_runtime
returns approved/rejected; merge-helper, prompt-addendum, and aclose()
lifecycle tests added.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Oleg Miagkov <mrobenner@gmail.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 46 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds async cleanup support for OpenAI-compatible sessions and implements a runtime QA sign-off persistence mechanism. Runtime adapters now write standalone ChangesRuntime QA Reviewer with Async Session Lifecycle
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 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: 4
🤖 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/reviewer.py`:
- Around line 332-397: The merged runtime qa_signoff side-file (artifact =
spec_dir / RUNTIME_QA_SIGNOFF_FILENAME) is left in place and can be replayed
later; after a successful merge (i.e., after plan["qa_signoff"] is set and
save_implementation_plan(spec_dir, plan) returns True) delete the artifact to
prevent stale replay, catching and logging any OSError when unlinking but not
treating unlink errors as merge failures; only perform the unlink when
save_implementation_plan succeeds and do not remove the artifact earlier or on
save failure.
- Around line 767-777: The current cleanup returns unconditionally after an
aclose() exception and skips the sync close() fallback; fix by making the early
return occur only on successful await of session.aclose() — move the return (or
set a closed flag and return) inside the try block right after await aclose() so
that if await aclose() raises, execution falls through to the close() branch;
keep the except logging for session.aclose() as-is and preserve the
contextlib.suppress wrapper around session.close().
- Around line 373-381: The code currently mis-coerces coverage_passed and
accepts arbitrary items for issues_found; change the handling so coverage_passed
is only set when the source is a real bool or a canonical string/number that can
be safely parsed (e.g. if raw["coverage_passed"] is bool use it, else if it's a
str map lower() of "true"/"false" (or "1"/"0") to True/False, otherwise
skip/leave unset), and change issues handling so issues =
raw.get("issues_found") is accepted only if it's a list then filtered to keep
only dict items (e.g. issues_filtered = [i for i in issues if isinstance(i,
dict)]) before assigning signoff["issues_found"] = issues_filtered (default to
[] if missing or no valid dicts); keep the existing tests_passed dict handling
as-is.
In `@tests/test_qa_reviewer_runtime.py`:
- Around line 263-274: The test
test_merge_runtime_qa_signoff_artifact_rejects_bad_payloads should be extended
to cover malformed field types and stale-artifact reuse: add parametrized
payloads where "coverage_passed" is a non-boolean (e.g., "true" or 1) and where
"issues_found" is not a list of dicts (e.g., a list of strings or a non-list),
and assert for each that merge_runtime_qa_signoff_artifact(tmp_path,
qa_session=1) returns False and the plan's "qa_signoff" remains None; also add a
case that simulates reusing an old qa_signoff.json across runs (e.g., write a
valid-looking payload but with a timestamp/session marker indicating an earlier
run) and assert the function rejects it (returns False and leaves plan
unchanged). Use the existing helper _write_plan and the qa_signoff.json write
sequence used in the test to place these new cases.
🪄 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: e1af64b4-4b3b-4087-89b2-42650822560b
📒 Files selected for processing (4)
apps/backend/core/providers/adapters/openai_compat.pyapps/backend/qa/reviewer.pytests/test_openai_compat_session_lifecycle.pytests/test_qa_reviewer_runtime.py
…input
Address review feedback on the direct-API QA reviewer sign-off:
- Stale replay: clear any leftover qa_signoff.json before the runtime
reviewer runs (so a merge only ever reflects the current session) and
remove the file after a successful merge. Prevents a verdict from a prior
session being replayed when the plan signoff was reset for revalidation.
- Field types: only carry coverage_passed through when it is a real bool
(bool("false") is True), and filter issues_found to dict items so the
rejection path and QA report never crash on a stray non-dict entry.
- Cleanup fallback: in _aclose_agent_session, fall through to the sync
close() when aclose() raises instead of returning early.
Tests: malformed coverage_passed/issues_found sanitation, real-bool
coverage retention, artifact consumption, stale-file clearing before a run,
and the aclose()->close() fallback.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Oleg Miagkov <mrobenner@gmail.com>
Owner
Author
|
Thanks for the review — addressed all four findings in 3b72adf:
All 53 tests across |
|
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.
Problem
After #338 fixed the model-404, the direct-API (
generic_edit) QA reviewer path starts on OpenAI, but a full build still endsqa_failed(exit 2): the runtime reviewer never persists its verdict, soget_qa_signoff_statusis always empty and the loop reports "QA agent did not update implementation_plan.json (runtime path)", retries 3×, and fails.Repro (from
apps/backend, self-contained spec):Root cause
Sign-off never written (the
qa_failedcause). The runtime reviewer shares the Claude-oriented prompt, which tells the model to recordqa_signoffusing theWrite/Edittools at the absolute path{spec_dir}/implementation_plan.json. Ingeneric_editthose tools don't exist (the real tools arewrite_file/replace_text), andresolve_workspace_pathrejects absolute paths outright:The Claude path works because it has a dedicated
update_qa_statusMCP tool plus realWrite/Edit.Leaked async client (log noise / resource leak).
OpenAICompatibleSession.close()only nulledself._client; theAsyncOpenAI/httpx transport was neveraclose()d. All QA iterations run under oneasyncio.run, and the reviewer session was never closed, so each iteration leaked a transport that firedRuntimeError: ... TCPTransport closedat GC time. (Noise, not the abort — each iteration builds a fresh client in the same loop.)Fix
OpenAICompatibleSession.aclose()— awaits the client's ownclose()in the same event loop (best-effort, never raises);run_qa_reviewer_runtime_sessioncloses the session in afinally.build_qa_reviewer_prompt(..., runtime_signoff_relspec=...)) overrides the Claude guidance: usewrite_file+ workspace-relative paths and drop a smallqa_signoff.json.merge_runtime_qa_signoff_artifact()folds that file intoimplementation_plan.jsonin Python, so we never depend on the model editing the large plan JSON (dodging both the tool-name and absolute-path traps). A direct plan write is still honored first. The Claude SDK path is unchanged (relspecdefaults toNone).Tests (48 passing, ruff clean)
test_qa_reviewer_runtime.py: scripted runtime sessions writing the verdict via the plan and via theqa_signoff.jsonside-file →run_qa_reviewer_via_runtimereturnsapproved/rejected; no-verdict →error; merge-helper, prompt-addendum, and Claude-path-unchanged cases.test_openai_compat_session_lifecycle.py:aclose()awaits the client, clears state, and swallows a teardown error.test_qa_runtime_integration.py,test_qa_fixer_runtime.py,test_provider_factory.py,test_agent_runtime.py.Follow-up (not in this PR)
run_qa_fixer_via_runtimehas the identical latent issue (relies on the model writingfixes_appliedinto the plan). It doesn't block a self-contained spec reachingapproved(the fixer only runs on rejection), so it's left for a focused follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests