From 5782f704b3ac343d2b59d2f08b7f43d30bc02ff0 Mon Sep 17 00:00:00 2001 From: kapil971390 Date: Tue, 30 Jun 2026 22:23:52 +0530 Subject: [PATCH] fix(character-reviewer): align success contract with visual_qa pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per maintainer feedback on PR #227: - Revert success=not issues back to success=True — tool execution succeeded even when QA finds issues; verdict lives in status/issues - Update test to assert the real contract: success=True + status='revise' + issues non-empty, matching how compose-director actually gates - Consistent with visual_qa.py: success=True, verdict in validation_passed --- .../contracts/test_character_animation_pipeline.py | 14 ++++++-------- tools/character/character_animation.py | 6 +----- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/tests/contracts/test_character_animation_pipeline.py b/tests/contracts/test_character_animation_pipeline.py index 9a9f709d..4c765936 100644 --- a/tests/contracts/test_character_animation_pipeline.py +++ b/tests/contracts/test_character_animation_pipeline.py @@ -188,11 +188,12 @@ def test_character_animation_smoke_flow(tmp_path): def test_character_reviewer_success_false_when_qa_finds_issues(): """ - CharacterAnimationReviewer.success must be False when QA finds issues. + CharacterAnimationReviewer surfaces QA failures via status/issues, not success. - Callers such as the compose-director gate on result.success to decide - whether to proceed. If success is always True, a broken rig silently - passes the gate. See PR #166 (closed) and the follow-up fix in #. + success=True means the tool executed successfully; the QA verdict lives in + character_qa_report.status and character_qa_report.issues — matching the + pattern used by visual_qa.py (success=True, verdict in validation_passed). + compose-director gates on report.status, not result.success. """ result = CharacterAnimationReviewer().execute( { @@ -205,14 +206,11 @@ def test_character_reviewer_success_false_when_qa_finds_issues(): ) qa_report = result.data["character_qa_report"] + assert result.success is True, "tool execution must succeed even when QA finds issues" assert qa_report["status"] == "revise", ( f"Expected status='revise' for a broken rig, got '{qa_report['status']}'" ) assert len(qa_report["issues"]) > 0, "Expected at least one issue for a broken rig" - assert result.success is False, ( - "success must be False when QA finds issues — " - "callers gate on result.success without inspecting report['status']" - ) def test_character_style_is_normalized_for_schema(tmp_path): diff --git a/tools/character/character_animation.py b/tools/character/character_animation.py index 2842989f..92bd1c7f 100644 --- a/tools/character/character_animation.py +++ b/tools/character/character_animation.py @@ -888,12 +888,8 @@ class CharacterAnimationReviewer(BaseTool): }, } artifacts = _write_json(inputs.get("output_path"), report) - # success=False when QA finds issues so callers can gate on result.success - # without needing to inspect report["status"]. This mirrors the contract - # asserted in tests/contracts/test_character_animation_pipeline.py and - # is consistent with how visual_qa.py surfaces validation failures. return ToolResult( - success=not issues, + success=True, data={"character_qa_report": report}, artifacts=artifacts, duration_seconds=round(time.time() - start, 2),