feat(tests): add comprehensive agentic test infrastructure - #29
Conversation
Implements PAL consensus review findings for agentic capability testing: Core Infrastructure: - Add MetricsEmitter protocol with InMemoryMetricsCollector and LoggingMetricsEmitter - Extend LoopOrchestrator with structured logging and metrics emission - Add observability to LearningLoopOrchestrator with session tracking Test Coverage (502 tests): - tests/loop/test_loop_invariants.py: All 8 termination conditions - tests/loop/test_learning_loop.py: 43 tests for skill learning lifecycle - tests/loop/test_loop_pal_integration.py: E2E feedback pipeline validation - tests/loop/test_loop_metrics.py: Per-iteration metrics emission - tests/loop/test_score_pattern_detector.py: 71 tests with noisy data patterns MCP Contract Validation: - tests/mcp/test_mcp_contracts.py: Schema validation for PAL/Rube tools - tests/mcp/test_pal_response_parsing.py: PAL response structure tests - tests/mcp/test_rube_response_parsing.py: Rube response handling - tests/mcp/test_live_mcp_client.py: HTTP fallback path tests - tests/mcp/test_sanitization.py: PII redaction verification CI Integration: - Add .github/workflows/agentic-tests-mcp.yml for nightly smoke tests - FakeMCPServer with fixture replay and call history tracking Consensus verdict: Tests validate agentic MACHINERY (orchestration logic) but not agentic CAPABILITIES (real improvement with live LLM/MCP). This is necessary but insufficient for full production confidence. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Tony363, your pull request is larger than the review limit of 150000 diff characters
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds pluggable metrics and logging to loop orchestration and learning, a nightly multi-tier GitHub Actions workflow for MCP/PAL testing, and extensive deterministic MCP and loop test infrastructure (fake MCP server, contract validators, sanitizers, fixtures, and many new tests). Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (20)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (22)
tests/mcp/live_mcp_client.py (3)
307-311: Consolidate imports at the top of the module.The
reandCallableimports should be moved to the top with the other standard library imports for better organization per PEP8 conventions.🔎 Suggested organization
from dataclasses import asdict, dataclass, field from enum import Enum -from typing import Any, Dict, Optional +from typing import Any, Callable, Dict, Optional +import re logger = logging.getLogger(__name__)Then remove the duplicate imports from lines 307-308.
493-495: Consolidate Path, datetime, and timedelta imports at module top.These imports should be moved to the top of the file with other standard library imports. Note that
dataclassis already imported at line 33.
429-432: Remove redundant local datetime import.
datetimeis imported at module level on line 494. This local import is unnecessary.🔎 Proposed fix
- from datetime import datetime - # Build the raw capture entrytests/mcp/README.md (1)
7-19: Add language identifier to fenced code block.The fenced code block should specify a language for better rendering and linting compliance. Consider adding
textorplaintext:🔎 Proposed fix
-``` +```text tests/mcp/ ├── conftest.py # FakeMCPServer and fixturestests/loop/test_loop_metrics.py (1)
10-10: Remove unused pytest import.The
pytestimport is not used in this file. No pytest marks or custom fixtures are defined.🔎 Proposed fix
-import pytest - from core.loop_orchestrator import LoopOrchestratortests/mcp/test_pal_response_parsing.py (1)
10-18: Remove unused imports.The imports
pytest,FakeMCPServer,FakePALCodeReviewResponse, andFakePALDebugResponseare not used in this file. The test fixtures (fake_mcp_server,pal_codereview_with_issues, etc.) are provided by conftest.py via pytest's fixture discovery.🔎 Proposed fix
-import pytest - from core.pal_integration import PALReviewSignal, incorporate_pal_feedback from core.types import QualityAssessment -from tests.mcp.conftest import ( - FakeMCPServer, - FakePALCodeReviewResponse, - FakePALDebugResponse, -)tests/mcp/test_mcp_contracts.py (1)
10-19: Remove unused imports.The imports
pytest,FakePALCodeReviewResponse,FakePALDebugResponse, andFakeRubeSearchToolsResponseare not used in this file. Test fixtures are provided by conftest.py via pytest's fixture discovery.🔎 Proposed fix
-import pytest - from tests.mcp.conftest import ( FakeMCPResponse, FakeMCPServer, - FakePALCodeReviewResponse, - FakePALDebugResponse, FakeRubeMultiExecuteResponse, - FakeRubeSearchToolsResponse, )tests/mcp/test_sanitization.py (1)
7-23: Remove unused imports.The imports
tempfile,Path,pytest, andFixtureStalenessReportare not used in this file. Thetmp_pathfixture (provided by pytest) already providesPathobjects, andFixtureStalenessReportis returned bycheck_fixture_stalenessbut only accessed via attributes, not used for type annotations in this test file.🔎 Proposed fix
from __future__ import annotations import json -import tempfile from datetime import datetime, timedelta -from pathlib import Path - -import pytest from tests.mcp.live_mcp_client import ( - FixtureStalenessReport, _apply_global_sanitization, _sanitize_dict_strings, check_fixture_staleness,tests/mcp/test_rube_response_parsing.py (1)
12-17: Remove unused imports.Static analysis correctly identifies that
FakeMCPServer,FakeRubeMultiExecuteResponse, andFakeRubeSearchToolsResponseare imported but not directly used in this file. These types are accessed via pytest fixtures (fake_mcp_server,rube_multi_execute_partial_failure) defined inconftest.py, so the explicit imports here are unnecessary.Proposed fix
from tests.mcp.conftest import ( FakeMCPResponse, - FakeMCPServer, - FakeRubeMultiExecuteResponse, - FakeRubeSearchToolsResponse, )tests/loop/test_loop_pal_integration.py (2)
16-21: Remove unused imports.
FakeMCPServer,FakePALCodeReviewResponse, andFakePALDebugResponseare accessed via pytest fixtures rather than directly. TheFixtureSkillInvokerimport at line 16 is also flagged but appears to be used (e.g., line 314).Proposed fix
-from tests.mcp.conftest import ( - FakeMCPServer, - FakePALCodeReviewResponse, - FakePALDebugResponse, -)
529-538: Unused variableresult.The
resultfromorchestrator.run()is assigned but never used. Either add assertions on it or use_to indicate intentional discard.Proposed fix
with patch.object(orchestrator, "assessor", assessor), \ patch.object(orchestrator, "_record_iteration", patched_record): - result = orchestrator.run({"task": "test"}, invoker) + orchestrator.run({"task": "test"}, invoker) assert len(contexts_captured) == 3tests/mcp/contract_helpers.py (2)
9-9: Remove unused import.
Optionalis imported but not used in the file.Proposed fix
-from typing import Any, Optional +from typing import Any
89-100: Consider documenting empty list behavior more explicitly.The comment at lines 99-100 notes that empty lists pass the type check, but this means an empty reference list will match a candidate list of any element type. This is probably acceptable for API testing but worth documenting in the docstring.
Consider adding to the docstring:
Note: Empty lists in the reference will match any list in the candidate, regardless of element types, since no element comparison is performed.tests/loop/conftest.py (3)
14-18: Remove unused imports flagged by static analysis.
Callable,Optional, andTerminationReasonare imported but never used in this file.🔎 Proposed fix
-from typing import Callable, Optional +from typing import Any import pytest -from core.types import LoopConfig, QualityAssessment, TerminationReason +from core.types import LoopConfig, QualityAssessmentNote: If you need
Anyfor type hints, add it; otherwise remove the typing import entirely if not used.
23-32: Remove unused MCP imports or document their intended future use.
FakePALDebugResponseandFakeRubeSearchToolsResponseare imported but never used. Either remove them or add a comment explaining they're reserved for planned fixtures.🔎 Proposed fix
try: from tests.mcp.conftest import ( FakeMCPServer, FakePALCodeReviewResponse, - FakePALDebugResponse, - FakeRubeSearchToolsResponse, ) _MCP_FIXTURES_AVAILABLE = True except ImportError: _MCP_FIXTURES_AVAILABLE = False
102-108: Consider logging or warning when fixture file is missing.When the fixture file doesn't exist, returning an empty dict silently may mask test configuration issues. Consider adding a debug log or documenting this behavior.
🔎 Proposed enhancement
+import logging + +logger = logging.getLogger(__name__) + def load_fixture_scenario(name: str) -> dict: """Load a recorded scenario from fixtures directory.""" fixture_path = Path(__file__).parent / "fixtures" / f"{name}.json" if fixture_path.exists(): with open(fixture_path) as f: return json.load(f) + logger.debug("Fixture file not found: %s", fixture_path) return {}tests/mcp/test_contract_validation.py (2)
16-27: Remove unusedAnyimport.Static analysis correctly identifies that
Anyis imported but not used.🔎 Proposed fix
-from typing import Any, Optional +from typing import Optional
434-446: Consider removing or using theexpected_schemavariables.These variables document the expected schema but are never used, triggering static analysis warnings. Move the schema documentation to docstrings or comments instead.
🔎 Proposed approach - use comments instead
def test_document_pal_codereview_schema(self, fake_mcp_server): """Document the expected PAL codereview response schema.""" response = fake_mcp_server.invoke( "mcp__pal__codereview", CANONICAL_PAL_REQUESTS["mcp__pal__codereview"], ) - # This test serves as documentation of the expected schema - expected_schema = { - "success": bool, - "data": { - "issues_found": list, # List of {severity, description} - "review_type": str, # "quick", "full", "security" - ... - }, - } + # Expected schema: + # { + # "success": bool, + # "data": { + # "issues_found": list, # List of {severity, description} + # "review_type": str, # "quick", "full", "security" + # ... + # }, + # } # Verify structure matches documentation assert isinstance(response["success"], bool)Also applies to: 462-469
core/metrics.py (1)
199-207: Replace magic number withlogging.DEBUGconstant.Using
10as the default level requires readers to know logging level values. Use the named constant for clarity.🔎 Proposed fix
+import logging + class LoggingMetricsEmitter: ... - def __init__(self, logger: Any, level: int = 10) -> None: # 10 = DEBUG + def __init__(self, logger: Any, level: int = logging.DEBUG) -> None: """Initialize with a logger instance. Args: logger: Python logger instance level: Log level for metric emissions (default: DEBUG) """Note:
loggingis not currently imported in this module, so you'd need to add the import.tests/loop/test_loop_invariants.py (2)
17-17: Remove unusedFixtureSkillInvokerimport.This import is flagged by static analysis as unused. The test file uses the
fixture_skill_invokerpytest fixture instead of instantiatingFixtureSkillInvokerdirectly.🔎 Proposed fix
from core.loop_orchestrator import LoopOrchestrator from core.types import LoopConfig, TerminationReason -from tests.loop.conftest import FixtureAssessor, FixtureSkillInvoker +from tests.loop.conftest import FixtureAssessor
219-231: Use theresultvariable or replace with_.Line 226 assigns
resultbut never uses it, triggering static analysis warnings. Either useresultin an assertion or replace with_to indicate intentional discard.🔎 Proposed fix
def test_score_history_recorded(self, fixture_skill_invoker): """Score history should be recorded for each iteration.""" assessor = FixtureAssessor(scores=[50.0, 60.0, 70.0]) config = LoopConfig(quality_threshold=65.0) orchestrator = LoopOrchestrator(config) with patch.object(orchestrator, "assessor", assessor): - result = orchestrator.run({"task": "implement"}, fixture_skill_invoker) + _ = orchestrator.run({"task": "implement"}, fixture_skill_invoker) # Should have scores for all iterations until quality met assert len(orchestrator.score_history) >= 2tests/mcp/conftest.py (1)
29-29: Remove unusedMagicMockimport.Static analysis correctly identifies that
MagicMockis imported but never used in this file.🔎 Proposed fix
from pathlib import Path from typing import Any, Dict, List, Optional -from unittest.mock import MagicMock import pytest
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.github/workflows/agentic-tests-mcp.ymlcore/loop_orchestrator.pycore/metrics.pycore/skill_learning_integration.pypytest.initests/conftest.pytests/core/test_metrics.pytests/loop/__init__.pytests/loop/conftest.pytests/loop/fixtures/improving_scores.jsontests/loop/fixtures/oscillating_scores.jsontests/loop/fixtures/stagnating_scores.jsontests/loop/test_learning_loop.pytests/loop/test_loop_invariants.pytests/loop/test_loop_metrics.pytests/loop/test_loop_pal_integration.pytests/loop/test_score_pattern_detector.pytests/mcp/README.mdtests/mcp/__init__.pytests/mcp/conftest.pytests/mcp/contract_helpers.pytests/mcp/fixtures/captured/.gitkeeptests/mcp/live_mcp_client.pytests/mcp/test_contract_validation.pytests/mcp/test_live_mcp_client.pytests/mcp/test_mcp_contracts.pytests/mcp/test_pal_response_parsing.pytests/mcp/test_rube_response_parsing.pytests/mcp/test_sanitization.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code targets 3.8+ with Black (88 cols), Flake8, and MyPy; use 4-space indents
Use snake_case for Python module and variable names
Use PascalCase for Python agent classes aligned with their persona names
Files:
tests/mcp/test_rube_response_parsing.pytests/loop/test_learning_loop.pytests/mcp/contract_helpers.pytests/loop/test_loop_pal_integration.pytests/mcp/test_mcp_contracts.pytests/conftest.pytests/mcp/__init__.pycore/metrics.pycore/skill_learning_integration.pycore/loop_orchestrator.pytests/mcp/test_contract_validation.pytests/loop/test_score_pattern_detector.pytests/mcp/live_mcp_client.pytests/mcp/test_live_mcp_client.pytests/mcp/test_sanitization.pytests/core/test_metrics.pytests/loop/test_loop_invariants.pytests/loop/__init__.pytests/loop/test_loop_metrics.pytests/mcp/conftest.pytests/loop/conftest.pytests/mcp/test_pal_response_parsing.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Mirror production paths when adding tests (tests//test_.py) and name test functions test_
Mark slower test journeys with @pytest.mark.slow or @pytest.mark.integration per pyproject.toml
Include fixtures that validate requires_evidence guardrails and .superclaude_metrics outputs whenever agent workflows, telemetry, or auto-implementation logic changes
Files:
tests/mcp/test_rube_response_parsing.pytests/loop/test_learning_loop.pytests/mcp/contract_helpers.pytests/loop/test_loop_pal_integration.pytests/mcp/test_mcp_contracts.pytests/conftest.pytests/mcp/__init__.pytests/mcp/test_contract_validation.pytests/loop/test_score_pattern_detector.pytests/mcp/live_mcp_client.pytests/mcp/test_live_mcp_client.pytests/mcp/test_sanitization.pytests/core/test_metrics.pytests/loop/test_loop_invariants.pytests/loop/__init__.pytests/loop/test_loop_metrics.pytests/mcp/conftest.pytests/loop/conftest.pytests/mcp/test_pal_response_parsing.py
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Markdown files should wrap text near 100 characters
Files:
tests/mcp/README.md
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: Tony363/SuperClaude PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T08:20:43.637Z
Learning: Applies to tests/**/*.py : Include fixtures that validate requires_evidence guardrails and .superclaude_metrics outputs whenever agent workflows, telemetry, or auto-implementation logic changes
📚 Learning: 2025-12-15T08:20:43.637Z
Learnt from: CR
Repo: Tony363/SuperClaude PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T08:20:43.637Z
Learning: Applies to tests/**/*.py : Include fixtures that validate requires_evidence guardrails and .superclaude_metrics outputs whenever agent workflows, telemetry, or auto-implementation logic changes
Applied to files:
.github/workflows/agentic-tests-mcp.ymltests/mcp/README.mdtests/mcp/fixtures/captured/.gitkeeptests/loop/test_loop_pal_integration.pytests/mcp/test_mcp_contracts.pytests/conftest.pytests/mcp/__init__.pytests/loop/fixtures/improving_scores.jsontests/loop/test_score_pattern_detector.pytests/mcp/test_sanitization.pytests/core/test_metrics.pytests/loop/test_loop_invariants.pytests/loop/__init__.pytests/loop/test_loop_metrics.pytests/loop/conftest.py
📚 Learning: 2025-12-15T08:20:43.637Z
Learnt from: CR
Repo: Tony363/SuperClaude PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T08:20:43.637Z
Learning: Pull Requests should describe risk surface, highlight configuration changes (e.g., MCP updates), and note any follow-up tasks for consensus, telemetry, or cleanup tooling
Applied to files:
tests/mcp/README.md
📚 Learning: 2025-12-15T08:20:43.637Z
Learnt from: CR
Repo: Tony363/SuperClaude PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T08:20:43.637Z
Learning: Applies to tests/**/*.py : Mark slower test journeys with pytest.mark.slow or pytest.mark.integration per pyproject.toml
Applied to files:
pytest.ini
📚 Learning: 2025-12-15T08:20:43.637Z
Learnt from: CR
Repo: Tony363/SuperClaude PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T08:20:43.637Z
Learning: Collect coverage for SuperClaude and setup packages in test runs
Applied to files:
tests/mcp/__init__.pytests/loop/__init__.py
📚 Learning: 2025-12-28T00:31:56.586Z
Learnt from: CR
Repo: Tony363/SuperClaude PR: 0
File: archive/python-sdk-v5/core/AGENTS.md:0-0
Timestamp: 2025-12-28T00:31:56.586Z
Learning: Follow the iteration pattern: Delegate → Evaluate (quality score) → Iterate (if score < 70 with feedback) → Accept (when score ≥ 70)
Applied to files:
tests/loop/fixtures/improving_scores.jsoncore/loop_orchestrator.py
📚 Learning: 2025-12-28T00:31:56.586Z
Learnt from: CR
Repo: Tony363/SuperClaude PR: 0
File: archive/python-sdk-v5/core/AGENTS.md:0-0
Timestamp: 2025-12-28T00:31:56.586Z
Learning: Evaluate every Task output with a quality score (0-100): accept if 90-100 (production-ready), review if 70-89 (acceptable), or auto-iterate if <70 (needs improvement)
Applied to files:
tests/loop/fixtures/improving_scores.jsoncore/loop_orchestrator.py
🧬 Code graph analysis (13)
tests/loop/test_loop_pal_integration.py (5)
core/loop_orchestrator.py (3)
LoopOrchestrator(41-464)run(110-365)_record_iteration(374-421)core/pal_integration.py (3)
PALReviewSignal(18-182)incorporate_pal_feedback(226-265)generate_review_signal(33-87)core/types.py (1)
LoopConfig(32-61)tests/loop/conftest.py (3)
FixtureAssessor(36-70)FixtureSkillInvoker(74-99)pal_codereview_with_issues(225-244)tests/mcp/conftest.py (7)
FakeMCPServer(140-334)FakePALCodeReviewResponse(59-73)FakePALDebugResponse(77-91)pal_codereview_with_issues(352-369)set_response(266-268)invoke(291-318)to_dict(45-55)
tests/mcp/test_mcp_contracts.py (1)
tests/mcp/conftest.py (20)
FakeMCPResponse(37-55)FakeMCPServer(140-334)FakePALCodeReviewResponse(59-73)FakePALDebugResponse(77-91)FakeRubeMultiExecuteResponse(121-137)FakeRubeSearchToolsResponse(95-117)pal_codereview_response(346-348)to_dict(45-55)pal_codereview_with_issues(352-369)pal_debug_response(373-375)rube_search_response(379-381)rube_multi_execute_partial_failure(385-407)invoke(291-318)set_error_response(270-272)set_timeout_response(274-278)get_call_count(320-322)get_last_call(324-329)reset(331-334)set_response(266-268)from_fixtures(157-216)
core/skill_learning_integration.py (3)
core/loop_orchestrator.py (2)
LoopOrchestrator(41-464)run(110-365)core/metrics.py (1)
MetricsEmitter(88-112)core/types.py (1)
TerminationReason(14-28)
core/loop_orchestrator.py (2)
core/metrics.py (2)
MetricsEmitter(88-112)noop_emitter(115-124)core/types.py (4)
LoopConfig(32-61)TerminationReason(14-28)QualityAssessment(65-85)LoopResult(117-158)
tests/mcp/test_contract_validation.py (3)
tests/mcp/conftest.py (3)
FakeMCPServer(140-334)fake_mcp_server(338-342)invoke(291-318)tests/mcp/contract_helpers.py (2)
assert_schema_matches(12-100)schema_diff(135-198)tests/mcp/live_mcp_client.py (6)
FailureCategory(43-55)MCPInvocationResult(59-92)invoke_real_mcp(95-145)log_invocation_result(274-296)is_success(80-82)to_json(72-77)
tests/loop/test_score_pattern_detector.py (1)
core/termination.py (4)
check_insufficient_improvement(87-107)detect_oscillation(12-52)detect_stagnation(55-84)should_terminate(110-152)
tests/mcp/live_mcp_client.py (1)
tests/mcp/test_contract_validation.py (1)
invoke_real_mcp(96-122)
tests/mcp/test_live_mcp_client.py (1)
tests/mcp/live_mcp_client.py (6)
FailureCategory(43-55)MCPInvocationResult(59-92)_try_http_mcp(148-271)invoke_real_mcp(95-145)is_success(80-82)to_json(72-77)
tests/mcp/test_sanitization.py (2)
tests/mcp/live_mcp_client.py (6)
FixtureStalenessReport(499-506)_apply_global_sanitization(362-366)_sanitize_dict_strings(369-378)check_fixture_staleness(509-586)register_tool_sanitizer(334-359)sanitize_capture(381-408)archive/python-sdk-v5/Commands/executor/ast_analysis.py (1)
report(246-254)
tests/core/test_metrics.py (1)
core/metrics.py (9)
InMemoryMetricsCollector(127-187)LoggingMetricsEmitter(190-224)MetricsEmitter(88-112)noop_emitter(115-124)get(156-161)get_all(163-165)count(167-169)filter_by_tags(171-183)clear(185-187)
tests/loop/test_loop_invariants.py (3)
core/loop_orchestrator.py (2)
LoopOrchestrator(41-464)run(110-365)core/types.py (1)
LoopConfig(32-61)tests/loop/conftest.py (9)
FixtureAssessor(36-70)FixtureSkillInvoker(74-99)fixture_assessor_quality_met(112-114)fixture_skill_invoker(142-144)fixture_assessor_oscillating(118-120)fixture_assessor_stagnating(124-126)fixture_assessor_insufficient_improvement(136-138)fixture_skill_invoker_with_errors(148-154)loop_config_with_timeout(184-190)
tests/loop/test_loop_metrics.py (3)
core/loop_orchestrator.py (2)
LoopOrchestrator(41-464)run(110-365)core/metrics.py (4)
InMemoryMetricsCollector(127-187)get(156-161)filter_by_tags(171-183)get_all(163-165)core/types.py (1)
LoopConfig(32-61)
tests/loop/conftest.py (2)
core/types.py (1)
LoopConfig(32-61)tests/mcp/conftest.py (5)
FakeMCPServer(140-334)FakePALCodeReviewResponse(59-73)FakePALDebugResponse(77-91)FakeRubeSearchToolsResponse(95-117)reset(331-334)
🪛 GitHub Actions: CI
tests/core/test_metrics.py
[error] 10-10: ruff check failed: F401 'pytest' imported but unused.
🪛 GitHub Check: CodeQL
tests/mcp/test_rube_response_parsing.py
[notice] 10-10: Unused import
Import of 'pytest' is not used.
[notice] 12-17: Unused import
Import of 'FakeRubeSearchToolsResponse' is not used.
Import of 'FakeMCPServer' is not used.
Import of 'FakeRubeMultiExecuteResponse' is not used.
tests/loop/test_learning_loop.py
[notice] 17-21: Unused import
Import of 'LoopResult' is not used.
[notice] 11-11: Unused import
Import of 'Any' is not used.
Import of 'Dict' is not used.
tests/mcp/contract_helpers.py
[notice] 9-9: Unused import
Import of 'Optional' is not used.
tests/loop/test_loop_pal_integration.py
[notice] 531-531: Unused local variable
Variable result is not used.
[notice] 17-21: Unused import
Import of 'FakePALDebugResponse' is not used.
Import of 'FakeMCPServer' is not used.
Import of 'FakePALCodeReviewResponse' is not used.
[notice] 16-16: Unused import
Import of 'FixtureSkillInvoker' is not used.
[notice] 11-11: Unused import
Import of 'pytest' is not used.
tests/mcp/test_mcp_contracts.py
[notice] 10-10: Unused import
Import of 'pytest' is not used.
[notice] 12-19: Unused import
Import of 'FakePALDebugResponse' is not used.
Import of 'FakeRubeSearchToolsResponse' is not used.
Import of 'FakePALCodeReviewResponse' is not used.
tests/mcp/test_contract_validation.py
[notice] 16-16: Unused import
Import of 'Any' is not used.
[notice] 434-434: Unused local variable
Variable expected_schema is not used.
[notice] 462-462: Unused local variable
Variable expected_schema is not used.
tests/loop/test_score_pattern_detector.py
[warning] 348-348: Variable defined multiple times
This assignment to 'final_window' is unnecessary as it is redefined before this value is used.
[notice] 352-352: Unused local variable
Variable osc is not used.
tests/mcp/test_live_mcp_client.py
[notice] 26-26: Unused import
Import of 'requests' is not used.
tests/mcp/test_sanitization.py
[notice] 10-10: Unused import
Import of 'tempfile' is not used.
[notice] 12-12: Unused import
Import of 'Path' is not used.
[notice] 14-14: Unused import
Import of 'pytest' is not used.
[notice] 16-23: Unused import
Import of 'FixtureStalenessReport' is not used.
tests/core/test_metrics.py
[warning] 30-30: Use of the return value of a procedure
The result of noop_emitter is used even though it is always None.
tests/loop/test_loop_invariants.py
[notice] 226-226: Unused local variable
Variable result is not used.
[notice] 17-17: Unused import
Import of 'FixtureSkillInvoker' is not used.
[notice] 13-13: Unused import
Import of 'pytest' is not used.
tests/loop/test_loop_metrics.py
[notice] 10-10: Unused import
Import of 'pytest' is not used.
tests/mcp/conftest.py
[notice] 29-29: Unused import
Import of 'MagicMock' is not used.
tests/loop/conftest.py
[notice] 24-29: Unused import
Import of 'FakePALDebugResponse' is not used.
Import of 'FakeRubeSearchToolsResponse' is not used.
tests/mcp/test_pal_response_parsing.py
[notice] 10-10: Unused import
Import of 'pytest' is not used.
[notice] 14-18: Unused import
Import of 'FakePALDebugResponse' is not used.
Import of 'FakeMCPServer' is not used.
Import of 'FakePALCodeReviewResponse' is not used.
🪛 GitHub Check: Quality Gate
tests/loop/test_learning_loop.py
[failure] 19-19: Ruff (F401)
tests/loop/test_learning_loop.py:19:5: F401 core.types.LoopResult imported but unused
[failure] 11-11: Ruff (F401)
tests/loop/test_learning_loop.py:11:25: F401 typing.Dict imported but unused
[failure] 11-11: Ruff (F401)
tests/loop/test_learning_loop.py:11:20: F401 typing.Any imported but unused
tests/core/test_metrics.py
[failure] 172-172: Ruff (E731)
tests/core/test_metrics.py:172:9: E731 Do not assign a lambda expression, use a def
[failure] 10-10: Ruff (F401)
tests/core/test_metrics.py:10:8: F401 pytest imported but unused
tests/loop/conftest.py
[failure] 28-28: Ruff (F401)
tests/loop/conftest.py:28:9: F401 tests.mcp.conftest.FakeRubeSearchToolsResponse imported but unused; consider using importlib.util.find_spec to test for availability
[failure] 27-27: Ruff (F401)
tests/loop/conftest.py:27:9: F401 tests.mcp.conftest.FakePALDebugResponse imported but unused; consider using importlib.util.find_spec to test for availability
[failure] 18-18: Ruff (F401)
tests/loop/conftest.py:18:55: F401 core.types.TerminationReason imported but unused
[failure] 14-14: Ruff (F401)
tests/loop/conftest.py:14:30: F401 typing.Optional imported but unused
[failure] 14-14: Ruff (F401)
tests/loop/conftest.py:14:20: F401 typing.Callable imported but unused
🪛 Gitleaks (8.30.0)
tests/mcp/test_live_mcp_client.py
[high] 116-116: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
tests/mcp/README.md
[uncategorized] ~38-~38: The official name of this software platform is spelled with a capital “H”.
Context: ...egration tests with real MCP tools. See .github/workflows/agentic-tests-mcp.yml. ## K...
(GITHUB)
[uncategorized] ~198-~198: The official name of this software platform is spelled with a capital “H”.
Context: ...gration Live MCP tests run nightly via .github/workflows/agentic-tests-mcp.yml: - Ru...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
tests/mcp/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (78)
tests/loop/__init__.py (1)
1-8: Excellent module documentation.The docstring clearly enumerates the test categories within this package and aligns well with the PR's comprehensive test infrastructure objectives. This follows Python best practices for package-level documentation.
Note: Based on coding guidelines, ensure that actual test files mirror production paths (e.g.,
tests/loop/test_<module>.py) and include fixtures validatingrequires_evidenceguardrails and.superclaude_metricsoutputs whenever agent workflows or telemetry changes are tested.tests/mcp/__init__.py (1)
1-7: Module docstring clearly documents MCP test scope.Provides helpful context for developers on the test coverage (contracts, parsing, fixtures).
tests/conftest.py (1)
152-155: Helpful documentation clarifying fixture organization.The comments accurately explain pytest's auto-discovery of local
conftest.pyfiles and guide developers to find MCP and loop-specific fixtures in their respective directories.pytest.ini (1)
28-29: New markers properly defined for live and nightly test tiers.The markers are correctly wired with test implementations. Tests marked with
@pytest.mark.livecheckMCP_LIVE_TESTING_ENABLEDviapytest.skip()logic (see test_contract_validation.py:279-280 and live_mcp_client.py:121), and the requirement is documented in docstrings and README. Infrastructure integration is complete.tests/loop/fixtures/oscillating_scores.json (1)
1-20: LGTM!The oscillating scores fixture is well-structured with a clear alternating pattern (up/down) that correctly exhibits 4 direction changes. The data aligns with the expected termination reason and supports pattern detection tests.
tests/loop/fixtures/improving_scores.json (1)
1-20: LGTM!The improving scores fixture correctly demonstrates the iteration pattern where scores start below the quality threshold (70) and improve until exceeding it. The average improvement of 8.0 points per iteration is accurately calculated, and the structure aligns with the quality score evaluation pattern from project conventions.
.github/workflows/agentic-tests-mcp.yml (2)
48-86: Well-structured tiered testing approach.The workflow correctly implements a multi-tier testing strategy with appropriate blocking/non-blocking semantics. Tier 1 (offline) provides fast feedback, while live tests are properly gated by credential availability.
499-554: Summary job correctly aggregates results with appropriate failure semantics.The distinction between blocking failures (offline tests, contract validation) and non-blocking smoke tests is well-implemented. The summary provides clear visibility into the test run status.
tests/mcp/live_mcp_client.py (3)
95-145: Robust error handling with comprehensive failure categorization.The
invoke_real_mcpfunction provides excellent observability with structured results, latency tracking, and clear failure categories. The fallback chain (HTTP → not available) is well-designed.
314-331: Comprehensive PII sanitization patterns.The global sanitization patterns cover common sensitive data types (emails, API keys, credit cards, phone numbers, IP addresses). The approach of redacting public IPs while preserving private ranges is thoughtful for debugging purposes.
1-25: Remove the unused placeholderinvoke_real_mcpfunction (lines 96-122).The test file already imports and uses the real implementation from
live_mcp_client.py(imported at line 25, called at lines 351 and 398). The placeholder function is dead code and should be removed.Likely an incorrect or invalid review comment.
tests/mcp/fixtures/captured/.gitkeep (1)
1-10: Helpful documentation in .gitkeep.Including usage instructions directly in the .gitkeep is a nice touch for discoverability. The example commands clearly show how to capture fixtures and how they integrate with FakeMCPServer.
tests/core/test_metrics.py (2)
28-31: CodeQL warning is a false positive here.The test intentionally verifies that
noop_emitterreturnsNone. This is valid test behavior to confirm the function's contract.
34-124: Comprehensive test coverage for InMemoryMetricsCollector.The tests thoroughly exercise all collector methods including
get,get_all,count,filter_by_tags, andclear. The protocol conformance check ensures type safety. Based on learnings, this aligns well with validating.superclaude_metricsoutputs for agent workflows.tests/loop/fixtures/stagnating_scores.json (1)
1-21: LGTM!The stagnating scores fixture correctly demonstrates a plateau pattern with variance (0.025) well below the stagnation threshold (2.0). The scores cluster tightly around 65, accurately representing a stagnating loop that should terminate early.
tests/mcp/README.md (2)
21-147: LGTM! Comprehensive testing infrastructure documentation.The documentation clearly explains the three-tier testing approach (unit, contract validation, smoke), provides practical examples for FakeMCPServer usage, and documents environment variables and fixture capture workflows. This will be valuable for developers working with MCP integrations.
148-203: LGTM! Clear guidance for extending and running tests.The step-by-step instructions for adding new MCP tools are well-structured, and the test execution examples cover key scenarios (unit only, live validation, capture mode). The CI integration notes provide sufficient context for nightly testing workflows.
tests/loop/test_loop_metrics.py (4)
40-131: LGTM! Comprehensive metrics emission tests.The test class thoroughly validates loop lifecycle metrics (started, completed, duration, iterations, quality score, errors) with clear assertions and helpful inline comments explaining enum values.
133-178: LGTM! Per-iteration metrics thoroughly tested.The tests validate per-iteration duration, quality score, and quality delta metrics with clear assertions. The delta calculation test includes helpful comments explaining the expected progression (50.0, 15.0, 15.0).
180-214: LGTM! Termination reason tagging validated.The tests confirm that loop completion metrics include the correct
termination_reasontag for bothQUALITY_METandMAX_ITERATIONSscenarios, ensuring observability of loop outcomes.
216-227: LGTM! No-op emitter behavior validated.The test confirms that
LoopOrchestratorruns successfully without an explicit metrics emitter, ensuring metrics infrastructure is optional and doesn't introduce coupling.tests/mcp/test_pal_response_parsing.py (4)
21-155: LGTM! Comprehensive PAL codereview response parsing tests.The tests thoroughly validate PAL codereview response parsing, including field extraction, issue severity ordering, improvements capping (max 10), duplicate prevention, and error handling. The inline comment at lines 81-82 explaining the
insert(0, ...)reversal behavior is particularly helpful.
157-221: LGTM! PAL debug response parsing and signal generation validated.The tests cover PAL debug response parsing (hypothesis, confidence, relevant_files) and debug signal generation for oscillation and stagnation scenarios, ensuring the signal structure and context are correct.
223-311: LGTM! Comprehensive PAL review signal generation tests.The tests thoroughly validate
PALReviewSignalgeneration across various scenarios: basic structure, quality context inclusion, auto review type selection based on score and iteration, custom model configuration, and final validation signals. Good coverage of the signal generation API.
313-354: LGTM! PAL response state update flow validated.The tests validate the state update pipeline (signal→call→response→state), ensuring issues are correctly incorporated into
improvements_needed, feedback is stored, and edge cases (empty feedback, no issues) are handled gracefully.tests/mcp/test_mcp_contracts.py (6)
22-101: LGTM! Comprehensive PAL codereview contract tests.The tests thoroughly validate the PAL codereview response schema, including all required fields, correct types, issue structure, and valid enumerations for severity, review_type, and confidence. This ensures strong contract adherence.
103-143: LGTM! PAL debug contract validated.The tests confirm that PAL debug responses contain all required fields, that
hypothesisis a non-empty string, and thatconfidenceuses valid enumeration values.
145-187: LGTM! Rube search tools contract validated.The tests validate the Rube search tools schema, including required fields, tool structure, and tool_slug format conventions (uppercase with underscores), ensuring consistent naming across the MCP ecosystem.
189-229: LGTM! Rube multi-execute contract and consistency validated.The tests validate the Rube multi-execute response schema and ensure consistency between
all_succeededandpartial_failureflags, catching potential logic errors in response construction.
231-284: LGTM! MCP response envelope structure validated.The tests comprehensively validate the MCP response envelope structure across success, error, timeout, and unknown tool scenarios, ensuring consistent response formatting.
286-450: LGTM! Fake MCP server infrastructure thoroughly tested.The tests comprehensively validate the
FakeMCPServerbehavior (call history, reset, custom responses) and fixture loading capabilities (JSON/JSONL parsing, precedence, invalid fixture handling, malformed JSON resilience). This ensures the test infrastructure is reliable and robust.tests/mcp/test_sanitization.py (4)
26-98: LGTM! Comprehensive global sanitization tests.The tests thoroughly validate PII and sensitive data sanitization across multiple patterns (emails, bearer tokens, AWS keys, phone numbers, credit cards, public IPs, long tokens). The test for preserving private IP addresses (lines 85-91) is particularly important for avoiding over-sanitization of internal network configurations.
100-149: LGTM! Recursive dictionary sanitization validated.The tests validate recursive sanitization across nested dictionaries, lists of strings, and lists of dicts. The test for preserving non-string values (int, bool, float, None) ensures sanitization is selective and doesn't corrupt non-sensitive data.
151-198: LGTM! Tool-specific sanitization registration and error handling validated.The tests validate tool-specific sanitizer registration, verify the two-pass sanitization pipeline (global then tool-specific), and ensure errors in tool sanitizers don't crash the capture process. The error handling test (lines 186-197) is particularly important for resilience.
200-306: LGTM! Comprehensive fixture staleness checking tests.The tests thoroughly validate fixture staleness checking across various scenarios: empty/nonexistent directories, fresh/stale fixtures, JSONL per-line checking, missing
captured_atwarnings, and custom threshold configuration. This ensures the staleness detection is robust and configurable.tests/mcp/test_rube_response_parsing.py (3)
20-109: Well-structured test coverage for Rube search tools.The test class comprehensively covers success parsing, tool schema extraction, empty results handling, and error responses. The test methods are clearly named and follow the
test_<behavior>convention per coding guidelines.
112-248: Thorough multi-execute response parsing tests.Good coverage of partial failure detection, error extraction from failed tools, and complete failure scenarios. The tests correctly validate both the
all_succeededflag and individual tool result inspection.
339-366: Good timeout handling coverage.The timeout tests correctly verify the error structure including metadata presence. Consider adding a test that validates the
timeout_secondsvalue in the metadata matches the configured timeout.tests/loop/test_loop_pal_integration.py (2)
366-439: Excellent E2E test for PAL feedback pipeline.This test validates the critical P0 requirement that PAL feedback from iteration N flows to iteration N+1. The patching approach to simulate external MCP processing is well-documented and the assertions thoroughly verify the feedback incorporation.
126-200: Comprehensive PAL feedback incorporation tests.Good coverage of severity-based ordering (critical prepended, medium appended), deduplication, and the max-10 cap. These tests align with the retrieved learnings about validating requires_evidence guardrails.
tests/mcp/test_live_mcp_client.py (3)
114-119: Test API key is a safe fixture value (Gitleaks false positive).The Gitleaks scanner flagged
test-api-key-12345as a potential secret. This is a test fixture value used for mocking HTTP requests and poses no security risk. Consider adding a comment to suppress future scanner warnings, or use a more obviously fake value likefake-test-key-for-unit-tests.
617-683: Good high-level invoke_real_mcp tests.The tests properly verify NOT_CONFIGURED behavior, default timeout usage (30s), and custom timeout propagation. This provides good coverage of the public interface.
685-737: Thorough MCPInvocationResult tests.Good coverage of the
is_successproperty for all failure categories andto_jsonserialization including enum handling. This ensures the result dataclass behaves correctly.tests/loop/test_score_pattern_detector.py (3)
20-90: Excellent parametrized oscillation tests.Comprehensive coverage of clean patterns, noisy patterns, borderline cases, and parameter sensitivity. The test descriptions in the parametrize decorator provide good documentation.
197-243: Good integration tests for should_terminate.The tests correctly verify termination priority rules (oscillation > stagnation > insufficient_improvement) which aligns with the implementation in
core/termination.py.
245-291: Thorough edge case coverage.Tests for empty history, single score, two scores, extreme values, negative scores, and float precision issues provide excellent boundary condition coverage.
tests/loop/test_learning_loop.py (4)
25-91: Well-designed fixtures for skill learning tests.The mock fixtures provide good isolation for testing the learning loop without external dependencies. The
patched_skill_dependenciescomposite fixture cleanly patches all skill-related dependencies.
93-124: Good initialization tests.Verifies inheritance, default states, and unique session ID generation. The session ID length assertion (12 chars) documents the expected UUID prefix format.
284-352: Comprehensive skill extraction tests.Good use of parametrized tests to verify extraction only occurs on
QUALITY_METtermination. This aligns with the design that skills should only be learned from successful sessions.
508-556: Domain detection tests cover key patterns.The parametrized tests for task keywords and file extensions provide good coverage. The comment at line 521-522 about "Build ML pipeline" matching "ui" before "data" is a helpful edge case note.
core/loop_orchestrator.py (5)
86-108: Good backward-compatible initialization with observability.The optional
loggerandmetrics_emitterparameters with sensible defaults (logging.getLogger(__name__)andnoop_emitter) ensure existing code continues to work. Theloop_idgeneration enables log correlation across iterations.
133-143: Appropriate loop lifecycle metrics.Emitting
loop.started.countat the start with comprehensive logging context (max_iterations, quality_threshold, pal_review_enabled) provides good observability for debugging and monitoring.
337-365: Complete loop completion metrics.Good coverage of completion metrics including duration, iteration count, and final quality score. The
termination_reasontag enables filtering by outcome type in dashboards.
388-421: Per-iteration metrics provide granular visibility.Emitting
loop.iteration.duration.seconds,loop.iteration.quality_score.gauge, andloop.iteration.quality_delta.gaugeenables detailed analysis of loop behavior. The debug log with comprehensive context is valuable for troubleshooting.
59-84: Thread safety documentation is valuable.The explicit warning that
LoopOrchestratoris NOT thread-safe with guidance to create new instances per task/thread prevents subtle concurrency bugs. This is an important addition to the docstring.tests/mcp/contract_helpers.py (2)
12-101: Well-designed recursive schema matcher.Good handling of edge cases:
- Null/None handling with clear error messages
- Int/float interchangeability for numeric types
allow_extra_keysflag for flexible validation- Clear path reporting in assertion messages
- First-element comparison for list schemas (documented assumption)
135-197: Useful schema_diff for diagnostic output.Unlike
assert_schema_matches, this collects all differences which is valuable for debugging schema drift. The implementation correctly handles the same edge cases (nullability, numeric types).core/skill_learning_integration.py (5)
53-81: LGTM! Comprehensive observability documentation.The docstring clearly documents the additional metrics emitted by the learning orchestrator, with usage examples showing how to integrate with
InMemoryMetricsCollector. This aligns well with the parent class's observability pattern.
83-103: LGTM! Constructor properly wires observability through to parent.The constructor correctly passes
loggerandmetrics_emitterto the superclass, enabling unified observability across both the base orchestrator and learning-specific features.
156-166: LGTM! Proper metrics emission at loop initialization.The skills applied count is emitted immediately after injection, and the logging includes the skill IDs for traceability. The f-string in the log message is acceptable for dynamic content.
180-200: Consider emitting count of 0 when no skill is extracted.Currently,
learning.skills.extracted.countis only emitted with value1when extraction is attempted. This is correct, but the metric is emitted regardless of whether a skill was actually extracted (thesuccesstag handles this). The logic is sound.
336-351: LGTM! Auto-promotion metrics and logging are well-structured.The promotion path correctly emits metrics with a reason tag and includes comprehensive log context with
loop_id,session_id, andskill_idfor correlation.tests/loop/conftest.py (3)
35-70: LGTM! Well-designed deterministic assessor fixture.The
FixtureAssessorprovides predictable scoring for testing loop termination conditions. The use ofmin()to clamp index access prevents out-of-bounds errors, and thereset()method enables fixture reuse.
73-99: LGTM! Clean skill invoker implementation.The
FixtureSkillInvokermirrors the assessor pattern with sensible defaults in__post_init__and proper index clamping for repeated calls.
214-243: LGTM! MCP fixtures properly guarded with availability checks.The
fake_mcp_serverandpal_codereview_with_issuesfixtures correctly skip tests when MCP fixtures are unavailable, preventing import errors from breaking the test suite. The cleanup viaserver.reset()in the yield fixture is appropriate.Based on learnings, these fixtures support validation of agent workflow telemetry and observability.
tests/mcp/test_contract_validation.py (2)
125-196: LGTM! Comprehensive helper function tests.The
TestContractHelpersclass thoroughly exercises the schema comparison utilities, covering edge cases like nested dicts, lists, numeric type interchangeability, and both single-error and multi-error scenarios.
261-274: LGTM! Well-documented live test configuration.The class docstring clearly explains the purpose, required environment variables, and observability features. The
@pytest.mark.liveand@pytest.mark.nightlymarkers properly gate these tests.core/metrics.py (3)
1-80: LGTM! Excellent module documentation.The docstring provides comprehensive guidance on metric naming conventions, type suffixes, available metrics across components, and integration examples for Prometheus and StatsD. This is exemplary documentation for a pluggable observability interface.
87-112: LGTM! Well-designed protocol with runtime checking.The
@runtime_checkabledecorator enablesisinstance()checks at runtime, which is useful for validation. The protocol's__call__signature matches the expected emitter interface.
127-187: LGTM! Comprehensive test collector with useful query methods.
InMemoryMetricsCollectorprovides essential testing utilities:get()for last value,get_all()for history,count()for emission counts, andfilter_by_tags()for dimensional queries. Theclear()method enables test isolation.tests/loop/test_loop_invariants.py (3)
20-49: LGTM! Quality threshold termination tests are well-structured.Tests cover both immediate quality met (first iteration) and delayed quality met (later iterations), verifying the correct termination reason and iteration count.
67-79: LGTM! Critical safety invariant test for hard max 5.This test validates the P0 safety requirement that max_iterations cannot exceed 5, even when explicitly configured higher. The assertion on line 74 verifies the config is capped before the loop runs.
322-364: LGTM! Comprehensive safety invariant validation.The
TestLoopSafetyInvariantsclass ensures critical guarantees: hard max of 5 iterations, guaranteed termination, and always-set termination reason. These tests protect against regressions in safety-critical behavior.Based on learnings, these tests validate requires_evidence guardrails for agent workflow changes.
tests/mcp/conftest.py (5)
36-55: LGTM! Clean base response dataclass.
FakeMCPResponseprovides a solid foundation with sensible defaults. Theto_dict()method correctly omits optional fields when not set, matching real MCP response structure.
58-117: LGTM! Well-designed specialized response classes.Each response class (
FakePALCodeReviewResponse,FakePALDebugResponse,FakeRubeSearchToolsResponse,FakeRubeMultiExecuteResponse) uses__post_init__to provide realistic default data structures that match the expected MCP schemas.
140-264: LGTM! Robust FakeMCPServer with fixture loading.The
from_fixturesclassmethod handles both single-fixture.jsonand multi-fixture.jsonlfiles with proper error handling. The invoke method's resolution order (captured fixtures → defaults → error) is clearly documented and implemented correctly.
331-334: Verify thatreset()should preserve captured fixtures.Currently
reset()clearscall_historyand resets default responses, but preservescaptured_fixtures. This seems intentional for test isolation while keeping loaded fixtures, but verify this is the desired behavior.
337-421: LGTM! Comprehensive pytest fixtures for MCP testing.The fixtures cover standard responses, error scenarios (partial failure, timeout), and malformed responses. The
fake_mcp_serverfixture properly yields and resets, ensuring test isolation.
| - name: Check for MCP credentials | ||
| id: check_secrets | ||
| run: | | ||
| if [ -n "${{ secrets.MCP_API_KEY }}" ] && [ -n "${{ secrets.MCP_API_BASE_URL }}" ]; then | ||
| echo "has_credentials=true" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "has_credentials=false" >> $GITHUB_OUTPUT | ||
| echo "::warning::MCP credentials not configured. Skipping live contract tests." | ||
| fi |
There was a problem hiding this comment.
Avoid interpolating secrets directly in shell conditionals.
Using ${{ secrets.MCP_API_KEY }} directly in the shell if statement could expose the secret in workflow logs if the condition fails or debug logging is enabled. Use environment variable indirection instead.
🔎 Proposed fix
- name: Check for MCP credentials
id: check_secrets
+ env:
+ MCP_API_KEY: ${{ secrets.MCP_API_KEY }}
+ MCP_API_BASE_URL: ${{ secrets.MCP_API_BASE_URL }}
run: |
- if [ -n "${{ secrets.MCP_API_KEY }}" ] && [ -n "${{ secrets.MCP_API_BASE_URL }}" ]; then
+ if [ -n "$MCP_API_KEY" ] && [ -n "$MCP_API_BASE_URL" ]; then
echo "has_credentials=true" >> $GITHUB_OUTPUT
else
echo "has_credentials=false" >> $GITHUB_OUTPUT
echo "::warning::MCP credentials not configured. Skipping live contract tests."
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Check for MCP credentials | |
| id: check_secrets | |
| run: | | |
| if [ -n "${{ secrets.MCP_API_KEY }}" ] && [ -n "${{ secrets.MCP_API_BASE_URL }}" ]; then | |
| echo "has_credentials=true" >> $GITHUB_OUTPUT | |
| else | |
| echo "has_credentials=false" >> $GITHUB_OUTPUT | |
| echo "::warning::MCP credentials not configured. Skipping live contract tests." | |
| fi | |
| - name: Check for MCP credentials | |
| id: check_secrets | |
| env: | |
| MCP_API_KEY: ${{ secrets.MCP_API_KEY }} | |
| MCP_API_BASE_URL: ${{ secrets.MCP_API_BASE_URL }} | |
| run: | | |
| if [ -n "$MCP_API_KEY" ] && [ -n "$MCP_API_BASE_URL" ]; then | |
| echo "has_credentials=true" >> $GITHUB_OUTPUT | |
| else | |
| echo "has_credentials=false" >> $GITHUB_OUTPUT | |
| echo "::warning::MCP credentials not configured. Skipping live contract tests." | |
| fi |
🤖 Prompt for AI Agents
In .github/workflows/agentic-tests-mcp.yml around lines 171 to 179, the step is
interpolating secrets directly into the shell conditional which can leak them in
logs; change the step to map the GitHub secrets to environment variables via the
step's env (e.g., env: MCP_API_KEY: ${{ secrets.MCP_API_KEY }} MCP_API_BASE_URL:
${{ secrets.MCP_API_BASE_URL }}) and then test presence using the environment
variables inside the script (e.g., if [ -n "$MCP_API_KEY" ] && [ -n
"$MCP_API_BASE_URL" ]; then ...), avoiding any direct use of ${{ secrets.* }}
inside the shell, and ensure you do not print or echo the secret values
anywhere.
| def test_gradual_improvement_with_setbacks(self): | ||
| """Overall improvement with occasional setbacks.""" | ||
| scores = [50.0, 55.0, 53.0, 58.0, 56.0, 61.0, 59.0, 64.0] | ||
|
|
||
| # Each window of 3 might show oscillation | ||
| # This is a challenge - the algorithm may flag this | ||
| final_window = scores[-3:] # [59.0, 64.0] - wait, that's only 2 | ||
| final_window = scores[-4:-1] # [61.0, 59.0, 64.0] | ||
|
|
||
| # Up-down-up pattern might trigger oscillation | ||
| osc = detect_oscillation(final_window, window=3) | ||
| # This is actually oscillating in the short term | ||
|
|
There was a problem hiding this comment.
Incomplete test with dead code.
This test has issues:
final_windowat line 348 is immediately overwritten at line 349 (dead assignment)oscat line 352 is assigned but never used for any assertion- The test doesn't assert anything meaningful
Either complete the test with proper assertions or remove it.
Option 1: Complete the test
def test_gradual_improvement_with_setbacks(self):
"""Overall improvement with occasional setbacks."""
scores = [50.0, 55.0, 53.0, 58.0, 56.0, 61.0, 59.0, 64.0]
# Each window of 3 might show oscillation
- # This is a challenge - the algorithm may flag this
- final_window = scores[-3:] # [59.0, 64.0] - wait, that's only 2
- final_window = scores[-4:-1] # [61.0, 59.0, 64.0]
-
- # Up-down-up pattern might trigger oscillation
- osc = detect_oscillation(final_window, window=3)
- # This is actually oscillating in the short term
+ # The last 3 scores [59.0, 64.0] - note: scores[-3:] gives [61.0, 59.0, 64.0]
+ final_window = scores[-3:] # [61.0, 59.0, 64.0]
+
+ # Up-down-up pattern in short-term window triggers oscillation
+ assert detect_oscillation(final_window, window=3) is True
+
+ # But overall trend is improving - this is a known limitation
+ # where short-term oscillation detection may stop beneficial loopsOption 2: Remove incomplete test
- def test_gradual_improvement_with_setbacks(self):
- """Overall improvement with occasional setbacks."""
- scores = [50.0, 55.0, 53.0, 58.0, 56.0, 61.0, 59.0, 64.0]
-
- # Each window of 3 might show oscillation
- # This is a challenge - the algorithm may flag this
- final_window = scores[-3:] # [59.0, 64.0] - wait, that's only 2
- final_window = scores[-4:-1] # [61.0, 59.0, 64.0]
-
- # Up-down-up pattern might trigger oscillation
- osc = detect_oscillation(final_window, window=3)
- # This is actually oscillating in the short term📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_gradual_improvement_with_setbacks(self): | |
| """Overall improvement with occasional setbacks.""" | |
| scores = [50.0, 55.0, 53.0, 58.0, 56.0, 61.0, 59.0, 64.0] | |
| # Each window of 3 might show oscillation | |
| # This is a challenge - the algorithm may flag this | |
| final_window = scores[-3:] # [59.0, 64.0] - wait, that's only 2 | |
| final_window = scores[-4:-1] # [61.0, 59.0, 64.0] | |
| # Up-down-up pattern might trigger oscillation | |
| osc = detect_oscillation(final_window, window=3) | |
| # This is actually oscillating in the short term | |
| def test_gradual_improvement_with_setbacks(self): | |
| """Overall improvement with occasional setbacks.""" | |
| scores = [50.0, 55.0, 53.0, 58.0, 56.0, 61.0, 59.0, 64.0] | |
| # Each window of 3 might show oscillation | |
| # The last 3 scores [59.0, 64.0] - note: scores[-3:] gives [61.0, 59.0, 64.0] | |
| final_window = scores[-3:] # [61.0, 59.0, 64.0] | |
| # Up-down-up pattern in short-term window triggers oscillation | |
| assert detect_oscillation(final_window, window=3) is True | |
| # But overall trend is improving - this is a known limitation | |
| # where short-term oscillation detection may stop beneficial loops |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 348-348: Variable defined multiple times
This assignment to 'final_window' is unnecessary as it is redefined before this value is used.
[notice] 352-352: Unused local variable
Variable osc is not used.
- Remove unused imports (F401) across test files - Convert lambda to def function in test_metrics.py (E731) - Remove unused variable assignments (F841) - Move imports to top of live_mcp_client.py (E402) - Remove duplicate invoke_real_mcp function (F811) - Fix ai-review.yml: move timeout_minutes to job level All ruff checks now pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
|
||
| def test_noop_returns_none(self): | ||
| """Noop emitter should return None.""" | ||
| result = noop_emitter("test.metric", 1) |
Check warning
Code scanning / CodeQL
Use of the return value of a procedure Warning test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix this class of problem you should stop capturing or using the return value of functions that are intended to be procedures, especially if they always return None. Instead, call the function for its side effects only.
Here, the single problematic usage is in test_noop_returns_none:
result = noop_emitter("test.metric", 1)
assert result is NoneThe best fix, without changing existing runtime functionality, is to remove the unnecessary assignment and assertion, and instead simply call noop_emitter as in the previous test. Since the class already has test_noop_accepts_metric, which tests that noop_emitter can be called without error, we can safely remove test_noop_returns_none entirely. This avoids using the procedure’s return value, keeps the test suite meaningful, and does not alter any production behavior.
Only tests/core/test_metrics.py needs to be edited, and no new imports, helper methods, or definitions are required.
| @@ -23,12 +23,7 @@ | ||
| noop_emitter("test.metric.count", 1) | ||
| noop_emitter("test.metric.gauge", 42.5, {"tag": "value"}) | ||
|
|
||
| def test_noop_returns_none(self): | ||
| """Noop emitter should return None.""" | ||
| result = noop_emitter("test.metric", 1) | ||
| assert result is None | ||
|
|
||
|
|
||
| class TestInMemoryMetricsCollector: | ||
| """Tests for the in-memory metrics collector.""" | ||
|
|
|
|
||
| # Each window of 3 might show oscillation | ||
| # This is a challenge - the algorithm may flag this | ||
| final_window = scores[-3:] # [59.0, 64.0] - wait, that's only 2 |
Check warning
Code scanning / CodeQL
Variable defined multiple times Warning test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix “variable defined multiple times” where the earlier value is never used, you verify that the later assignment is the intended one and then delete the earlier redundant assignment (while preserving any right‑hand‑side side effects, if present).
Here, in tests/loop/test_score_pattern_detector.py within TestRealWorldScenarios.test_gradual_improvement_with_setbacks, final_window is set twice in a row; only the second value is used in the call to detect_oscillation. Both right‑hand sides are pure slice operations on a list, so there are no side effects to preserve. The intent is also explained in the comments: line 348 notes that scores[-3:] only gives 2 elements (which is wrong, but the comment suggests the author reconsidered the slice) and then line 349 sets final_window = scores[-4:-1] (3 elements) which matches the comment “Each window of 3 might show oscillation.” The best minimal fix is to delete the first, unused assignment line 348 and keep the second one, leaving the rest of the test unchanged.
No new imports, methods, or definitions are required; we only remove the redundant line.
| @@ -345,7 +345,6 @@ | ||
|
|
||
| # Each window of 3 might show oscillation | ||
| # This is a challenge - the algorithm may flag this | ||
| final_window = scores[-3:] # [59.0, 64.0] - wait, that's only 2 | ||
| final_window = scores[-4:-1] # [61.0, 59.0, 64.0] | ||
|
|
||
| # Up-down-up pattern might trigger oscillation |
| value: Metric value (int, float, or other numeric type) | ||
| tags: Optional key-value tags for metric dimensions | ||
| """ | ||
| ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix a "statement has no effect" issue caused by a bare ... expression in a function body, replace it with a proper no-op like pass, or refactor the construct so the method is abstract or not executed at runtime. For protocol or interface-like methods in normal .py files, the most conservative fix is to keep the signature unchanged and replace ... with pass, which preserves behavior (the function still does nothing) but avoids a meaningless expression statement.
For this specific code, we should update the body of MetricsEmitter.__call__ in core/metrics.py so that line 112 uses pass instead of .... This keeps MetricsEmitter as a structural Protocol with the same interface and semantics while satisfying the static analysis rule. No new imports or helper methods are required; only the single line inside the method needs to change.
Concretely:
- In
core/metrics.py, within theMetricsEmitterprotocol’s__call__definition (around lines 99–112), replace the body line...withpass. - Do not modify the signature or docstring.
| @@ -109,7 +109,7 @@ | ||
| value: Metric value (int, float, or other numeric type) | ||
| tags: Optional key-value tags for metric dimensions | ||
| """ | ||
| ... | ||
| pass | ||
|
|
||
|
|
||
| def noop_emitter( |
| from tests.mcp.conftest import ( # noqa: F401 | ||
| FakeMCPServer, | ||
| FakePALCodeReviewResponse, | ||
| FakePALDebugResponse, | ||
| FakeRubeSearchToolsResponse, | ||
| ) |
Check notice
Code scanning / CodeQL
Unused import Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix unused imports you either (1) delete the unused imports, or (2) use them meaningfully. Here, there is no use of FakePALDebugResponse or FakeRubeSearchToolsResponse in the snippet, and we shouldn’t add artificial uses just to silence the warning.
The best fix without changing functionality is to keep the try/except block that imports MCP fixtures (so any module-level side effects still run) but remove the specific unused symbols from the import list. We will still import at least one symbol (e.g., FakeMCPServer, FakePALCodeReviewResponse) so that the import of tests.mcp.conftest occurs. This addresses both alert variants at once.
Concretely, in tests/loop/conftest.py, lines 23–28, we will modify the from tests.mcp.conftest import (...) statement to drop FakePALDebugResponse and FakeRubeSearchToolsResponse, keeping the rest of the import and the surrounding logic unchanged. No additional methods, imports, or definitions are needed.
| @@ -23,8 +23,6 @@ | ||
| from tests.mcp.conftest import ( # noqa: F401 | ||
| FakeMCPServer, | ||
| FakePALCodeReviewResponse, | ||
| FakePALDebugResponse, | ||
| FakeRubeSearchToolsResponse, | ||
| ) | ||
|
|
||
| _MCP_FIXTURES_AVAILABLE = True |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Summary
Multi-Model Consensus Review
This PR addresses findings from a PAL consensus review (Gemini-2.5-Pro + GPT-5.2):
What's Now Tested ✅
What Remains Untested ❌
Test plan
pytest tests/loop/- All loop tests passpytest tests/mcp/- All MCP contract tests passpytest tests/core/test_metrics.py- Metrics tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.