Skip to content

fix(qa): persist direct-API qa_reviewer verdict via a qa_status.json artifact - #348

Closed
OBenner wants to merge 2 commits into
claude/priceless-banach-3935b8from
claude/naughty-wozniak-899d70
Closed

fix(qa): persist direct-API qa_reviewer verdict via a qa_status.json artifact#348
OBenner wants to merge 2 commits into
claude/priceless-banach-3935b8from
claude/naughty-wozniak-899d70

Conversation

@OBenner

@OBenner OBenner commented Jun 18, 2026

Copy link
Copy Markdown
Owner

Summary

The direct-API (generic_edit) qa_reviewer had the same latent status-persistence bug that #342 fixed for the qa_fixer. build_qa_reviewer_prompt — especially its previous_error self-correction branch — tells the model to record its verdict by editing implementation_plan.json's qa_signoff with Claude's Write/Edit tools and absolute paths. On a non-Claude provider running through 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 the reviewer could never persist qa_signoff, and run_qa_reviewer_via_runtime returned "error" instead of "approved"/"rejected". QA on e.g. OpenAI never recorded a verdict.

This applies the qa_fixer pattern, adapted for the reviewer's richer verdict (approved/rejected + issues_found, vs the fixer's single fixes_applied flag).

Changes (apps/backend/qa/reviewer.py)

  • build_qa_reviewer_prompt gains runtime_status_relspec: str | None = None (default None → the Claude path is byte-for-byte unchanged). When set, an addendum appended last OVERRIDES the Write/Edit/absolute-path guidance: edit via write_file with workspace-relative paths, and record the verdict by writing <relspec>/qa_status.json with status ("approved"/"rejected") plus issues_found when rejected, then call finish.
  • run_qa_reviewer_via_runtime folds <spec_dir>/qa_status.json into qa_signoff via the new merge_runtime_qa_reviewer_status_artifact when no usable verdict landed in the plan, then re-reads. The existing update_qa_signoff_with_coverage and Graphiti save_session_memory steps are preserved.
  • run_qa_reviewer_runtime_session async-closes the provider session (OpenAICompatibleSession.aclose()) in a finally, mirroring _aclose_agent_session in qa/fixer.py, so the AsyncOpenAI connection pool is released in-loop rather than at interpreter exit.

Tests

tests/test_qa_reviewer_runtime.py (mirrors the qa_fixer suite): scripted runtime session writes qa_status.json → approved/rejected merged into qa_signoff; no-artifact stays "error"; prompt-addendum content + its precedence over the self-correction block; per-session async close.

apps/backend/.venv/bin/python -m pytest tests/test_qa_reviewer_runtime.py -p no:cacheprovider -q
# 16 passed

(The Claude-path tests/test_qa_reviewer.py fails to collect in this environment — a pre-existing agents.runtime.qa_phase_routing import error unrelated to this change, confirmed by reproducing it with these edits stashed out.)

Stacking

Stacked on #342 (the qa_fixer fix), which adds OpenAICompatibleSession.aclose(). The base is set to that branch so the diff shows only the reviewer changes; GitHub will retarget this PR to develop automatically once #342 merges.

🤖 Generated with Claude Code

OBenner and others added 2 commits June 13, 2026 11:07
…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>
…artifact

The generic_edit (direct-API) qa_reviewer had the same latent
status-persistence bug just fixed for the qa_fixer. build_qa_reviewer_prompt
(especially its previous_error self-correction branch) instructs the model to
record its verdict by editing implementation_plan.json's qa_signoff with
Claude's Write/Edit tools and absolute paths. On a non-Claude provider running
through 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
the reviewer could never persist qa_signoff and run_qa_reviewer_via_runtime
returned "error" instead of "approved"/"rejected" — QA on e.g. OpenAI never
recorded a verdict.

Apply the qa_fixer pattern, adapted for the reviewer's richer verdict:

- build_qa_reviewer_prompt gains a runtime_status_relspec param (default None,
  so the Claude path is unchanged). When set, an addendum appended last
  OVERRIDES the Write/Edit/absolute-path guidance: edit with write_file using
  workspace-relative paths, and record the verdict by writing
  <relspec>/qa_status.json with status ("approved"/"rejected") plus
  issues_found when rejected, then call finish.
- run_qa_reviewer_via_runtime folds <spec_dir>/qa_status.json into qa_signoff
  via merge_runtime_qa_reviewer_status_artifact when no usable verdict landed
  in the plan, then re-reads. The existing update_qa_signoff_with_coverage and
  Graphiti save_session_memory steps are preserved.
- run_qa_reviewer_runtime_session async-closes the provider session
  (OpenAICompatibleSession.aclose()) in a finally, mirroring
  _aclose_agent_session in qa/fixer.py, so the AsyncOpenAI connection pool is
  released in-loop rather than at interpreter exit.

Regression tests in tests/test_qa_reviewer_runtime.py mirror the qa_fixer
suite: scripted runtime session writes qa_status.json -> approved/rejected
merged into qa_signoff; no-artifact stays "error"; the prompt-addendum content
and its precedence over the self-correction block; per-session async close.

Stacked on #342 (the qa_fixer fix), which adds OpenAICompatibleSession.aclose().

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Oleg Miagkov <mrobenner@gmail.com>
@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (3)
  • develop
  • release/*
  • hotfix/*

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7657813b-de15-4320-af19-15437767c661

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/naughty-wozniak-899d70

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud

Copy link
Copy Markdown

@OBenner

OBenner commented Jun 18, 2026

Copy link
Copy Markdown
Owner Author

Closing as a duplicate. This exact fix is already merged to develop via #340 ("persist direct-API QA reviewer verdict and close its async client"), which is more complete — it also clears the status artifact (_clear_runtime_qa_signoff_artifact) and adds a dedicated test_openai_compat_session_lifecycle.py. Nothing to merge here.

@OBenner OBenner closed this Jun 18, 2026
@OBenner
OBenner deleted the claude/naughty-wozniak-899d70 branch June 18, 2026 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant