From 7cd7199986059c52c18729cbec6377dbff4f6ba3 Mon Sep 17 00:00:00 2001 From: kapil971390 Date: Mon, 29 Jun 2026 19:15:29 +0530 Subject: [PATCH 1/2] fix(reviewer): success=False when CharacterAnimationReviewer finds issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contract test in test_character_animation_pipeline.py asserts result.success after the QA run, implying success should reflect whether QA passed. But CharacterAnimationReviewer always returned success=True even when issues were found and status was 'revise'. This creates a silent failure path: callers that gate on result.success (the standard ToolResult contract) would treat a broken rig as a clean pass without ever reading report['status']. Fix: return success=not issues so result.success is False when QA finds problems, consistent with the test contract and with how execution errors are already handled (success=False on exception paths). Also adds test_character_reviewer_success_false_when_qa_finds_issues to explicitly assert this contract — a broken rig with missing joints must produce status='revise', non-empty issues[], and success=False. --- .../test_character_animation_pipeline.py | 29 +++++++++++++++++++ tools/character/character_animation.py | 6 +++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/tests/contracts/test_character_animation_pipeline.py b/tests/contracts/test_character_animation_pipeline.py index 542df066..9a9f709d 100644 --- a/tests/contracts/test_character_animation_pipeline.py +++ b/tests/contracts/test_character_animation_pipeline.py @@ -186,6 +186,35 @@ def test_character_animation_smoke_flow(tmp_path): assert qa_report["checks"]["schema_valid"] is True +def test_character_reviewer_success_false_when_qa_finds_issues(): + """ + CharacterAnimationReviewer.success must be False when QA finds issues. + + 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 #. + """ + result = CharacterAnimationReviewer().execute( + { + # Minimal rig_plan with missing joints — will trigger schema issues + "rig_plan": {"characters": [{"id": "char1", "role": "lead"}]}, + "pose_library": {}, + "action_timeline": {}, + "review_level": "static", + } + ) + + qa_report = result.data["character_qa_report"] + 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): result = CharacterSpecGenerator().execute( { diff --git a/tools/character/character_animation.py b/tools/character/character_animation.py index 92bd1c7f..2842989f 100644 --- a/tools/character/character_animation.py +++ b/tools/character/character_animation.py @@ -888,8 +888,12 @@ 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=True, + success=not issues, data={"character_qa_report": report}, artifacts=artifacts, duration_seconds=round(time.time() - start, 2), From 5782f704b3ac343d2b59d2f08b7f43d30bc02ff0 Mon Sep 17 00:00:00 2001 From: kapil971390 Date: Tue, 30 Jun 2026 22:23:52 +0530 Subject: [PATCH 2/2] 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),