fix: wire master loop to tool registry and restore phase-2 live gate#53
fix: wire master loop to tool registry and restore phase-2 live gate#53
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -0,0 +1,221 @@ | |||
| { | |||
There was a problem hiding this comment.
🔥 The Roast: Committing IDE session metadata is like bringing your browser history to a code review — nobody needs to see your internal state snapshots, and they'll be stale before the next commit anyway.
🩹 The Fix: Add .omc/ to .gitignore and remove these files from the PR. These are clearly editor/IDE state files that don't belong in version control.
📏 Severity: suggestion
| @@ -0,0 +1,8 @@ | |||
| { | |||
There was a problem hiding this comment.
🔥 The Roast: Session tracking files in git? I thought we were building a deterministic tool framework, not a time machine for IDE state.
🩹 The Fix: Remove this file and add .omc/ to .gitignore.
📏 Severity: suggestion
| @@ -0,0 +1,6 @@ | |||
| { | |||
There was a problem hiding this comment.
🔥 The Roast: HUD state caching in version control is like saving your TV's volume setting in a library book — completely irrelevant to anyone else and outdated immediately.
🩹 The Fix: Remove this file and ensure .omc/ is in .gitignore.
📏 Severity: suggestion
There was a problem hiding this comment.
Pull request overview
This PR updates the Phase 2 deterministic-tool scaffolding by wiring the MasterLoop to dispatch registry-registered tools first (with fallback to the existing direct executor), and adjusts Phase 2 live testing scaffolds/docs to align with the Two-Tier deterministic-first architecture.
Changes:
- Added a deterministic tool registry (
@deterministic_tool) with auto-discovery, plus three initial Phase 2 tool stubs. - Updated
MasterLoopto resolve and execute registered tools before invoking the direct executor, with new regression coverage in loop tests. - Cleaned up and restructured planning/docs (new Phase 2 master plan/program plan; removed multiple superseded/archived documents), and added documentation governance notes.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plans/phase2_scaffolding_review.md | Added Phase 2 scaffolding review write-up and recommendations. |
| docs/reviews/PR29_DEVILS_ADVOCATE_2026-03-04.md | Removed prior PR #29 devil’s advocate review doc. |
| docs/plans/tool_node_design_review.md | Condensed/rewrote tool contract review aligned to Two-Tier MasterLoop. |
| docs/plans/program_execution_plan.md | Added portfolio-level phase index and governance for planning stack. |
| docs/plans/phase_2_master_plan.md | Added Phase 2 master plan for deterministic tool framework + first tools. |
| docs/plans/phase_2_gemini_cli_plan.md | Removed obsolete Gemini CLI supervisor plan. |
| docs/plans/phase_0_login_tool_spec.md | Updated login tool spec to emphasize element-powered tools + strict contract. |
| docs/plans/llm_driven_gameplay_architecture.md | Removed LangGraph-centric gameplay architecture doc. |
| docs/monorepo/UPSTREAM_ALAS_LOCAL_EDITS_2026-02-16_043433.md | Removed upstream edits snapshot doc. |
| docs/monorepo/MONOREPO_SYNC_NOTES.md | Removed monorepo sync notes doc. |
| docs/investigations/login_popup_loop_stuck_investigation.md | Removed login popup loop investigation doc. |
| docs/dev/watchdog_investigation_2026_02_20.md | Removed watchdog investigation doc. |
| docs/archive/recovery_agent_implementation_plan.md | Removed archived recovery agent implementation plan. |
| docs/archive/recovery_agent_architecture.md | Removed archived recovery agent architecture doc. |
| docs/archive/local_vlm_setup.md | Removed archived local VLM setup plan. |
| docs/archive/legacy/FASTMCP_3_MIGRATION.md | Removed legacy FastMCP migration report. |
| docs/archive/futurePlans/multi_agent_conversion_plan.md | Removed archived multi-agent conversion plan. |
| docs/archive/deprecated/PARSER_IMPROVEMENT_PLAN.md | Removed deprecated parser improvement plan. |
| docs/archive/deprecated/PARSER_ARCHITECTURE_V2.md | Removed deprecated parser architecture v2 doc. |
| docs/archive/deprecated/MIGRATE_POETRY_TO_UV.md | Removed deprecated Poetry→uv migration doc. |
| docs/UPSTREAM_SYNC.md | Removed upstream sync workflow doc. |
| docs/NORTH_STAR.md | Updated NORTH_STAR to locked Two-Tier deterministic-first intent and governance. |
| docs/MASTER_PLAN.md | Removed prior master plan doc. |
| docs/IMPLEMENTATION_PLAN_NEXT_PASS.md | Removed implementation plan next pass doc. |
| adb_vision/tools/registry.py | Added deterministic tool registry + decorator + metadata storage. |
| adb_vision/tools/goto_main_menu.py | Added goto_main_menu tool stub registered via decorator. |
| adb_vision/tools/collect_commissions.py | Added collect_commissions tool stub registered via decorator. |
| adb_vision/tools/check_resources.py | Added check_resource_balances tool stub registered via decorator. |
| adb_vision/tools/init.py | Added tool auto-discovery on adb_vision.tools import. |
| adb_vision/test_tool_registry.py | Added unit tests for registry behavior and contract enforcement. |
| adb_vision/test_master_loop.py | Added regression test ensuring MasterLoop prefers registry tool when command matches. |
| adb_vision/test_live_phase2_tools.py | Added Phase 2 live harness with emulator reachability gating + per-tool live tests. |
| adb_vision/loop/master_loop.py | Implemented registry-first deterministic dispatch with direct-executor fallback. |
| PROGRESS_ASSESSMENT.md | Removed progress assessment doc. |
| GEMINI.md | Removed generated agent entrypoint doc. |
| FORK_CHANGES.md | Removed fork changes tracking doc. |
| DOC_GUIDE.md | Added documentation rules/guide and planning hierarchy notes. |
| CLAUDE.md | Replaced previous long-form agent instructions with condensed Two-Tier/MasterLoop intent. |
| AGENTS.md | Removed agent instruction doc. |
| .omc/state/idle-notif-cooldown.json | Added .omc runtime state file. |
| .omc/state/hud-stdin-cache.json | Added .omc runtime state file containing local paths/session metadata. |
| .omc/state/hud-state.json | Added .omc runtime state file. |
| .omc/sessions/9f68cafc-1a6c-47cc-8c96-0394c2a40828.json | Added .omc runtime session file. |
| .omc/project-memory.json | Added .omc runtime project memory file. |
| .kilo-prompt/review-mission.txt | Removed kilo prompt review-mission file. |
Comments suppressed due to low confidence (1)
adb_vision/loop/master_loop.py:17
MasterLoopimportsContractViolationError/validate_contractfromcontracts.tool_contract, but the new registry/tools currently import the contract module viaadb_vision.contracts.tool_contract. If both paths are used at runtime, Python will load the module twice and exceptions raised by one won’t be caught/recognized by the other. Standardize on a single import path for the contract module withinadb_vision/.
import sys
from typing import Any, Callable
from typing import Optional
from contracts.tool_contract import ContractViolationError, validate_contract
from scheduler.decider import TaskDecider
from scheduler.loader import TaskLoader
from state import cache
| from typing import Any, Callable, Dict, Optional | ||
| import functools | ||
|
|
||
| from adb_vision.contracts.tool_contract import strict_contract, make_result |
There was a problem hiding this comment.
registry.py imports make_result but never uses it, and it imports adb_vision.contracts.tool_contract while most of adb_vision/ uses top-level imports like contracts.tool_contract (see adb_vision/loop/master_loop.py). This mixed import style can load the contract module twice and lead to different ContractViolationError classes / registries depending on how tests are invoked (e.g., cd adb_vision). Remove the unused import and standardize the import path to match the rest of adb_vision.
| from adb_vision.contracts.tool_contract import strict_contract, make_result | |
| from contracts.tool_contract import strict_contract |
| from adb_vision.tools.registry import deterministic_tool | ||
| from adb_vision.contracts.tool_contract import make_result |
There was a problem hiding this comment.
These tool modules import adb_vision.tools.registry / adb_vision.contracts.tool_contract, but adb_vision/conftest.py prepends the adb_vision/ directory to sys.path, and many existing modules/tests import via top-level packages (tools, contracts, loop, etc.). This mismatch can break imports when running from cd adb_vision and can create duplicate module instances. Prefer consistent local imports (e.g., from tools.registry ... / from contracts.tool_contract ...) or remove the sys.path injection and use package imports everywhere.
| from adb_vision.tools.registry import deterministic_tool | |
| from adb_vision.contracts.tool_contract import make_result | |
| from tools.registry import deterministic_tool | |
| from contracts.tool_contract import make_result |
| def _resolve_tool(self, command: str) -> Optional[Callable[[], dict[str, Any]]]: | ||
| """Resolve command to a registered deterministic tool if available.""" | ||
| if not self.use_tool_registry: | ||
| return None | ||
|
|
||
| # Keep strict command names first-class; future command formats should be | ||
| # normalized before registration if needed. | ||
| candidate = command.strip() | ||
|
|
||
| for module_name in ("adb_vision.tools.registry", "tools.registry"): | ||
| module = sys.modules.get(module_name) | ||
| if module is None: | ||
| continue | ||
| get_tool = getattr(module, "get_tool", None) | ||
| if callable(get_tool): | ||
| tool = get_tool(candidate) | ||
| if tool is not None: | ||
| return tool | ||
|
|
||
| for module_name in ("adb_vision.tools.registry", "tools.registry"): | ||
| try: | ||
| module = __import__(module_name, fromlist=["get_tool"]) | ||
| except Exception: | ||
| continue | ||
| get_tool = getattr(module, "get_tool", None) | ||
| if callable(get_tool): | ||
| tool = get_tool(candidate) | ||
| if tool is not None: | ||
| return tool | ||
| return None |
There was a problem hiding this comment.
_resolve_tool() attempts both adb_vision.tools.registry and tools.registry via __import__. Importing the “other” name can create a second module instance with its own _TOOL_REGISTRY, causing registered tools to be invisible depending on import order. Prefer a single canonical registry module name and ensure tool discovery is triggered (e.g., import the tools package so tools/__init__.py can auto-register modules) before calling get_tool().
| result = tool_fn() | ||
|
|
||
| # TDD Expected Failure: Will fail because we have not implemented MaaFramework hooks yet! | ||
| assert result["success"] is True, f"Failed: {result['error']}" | ||
| assert result["observed_state"] == "page_main" |
There was a problem hiding this comment.
These live tests assert success is True, but the Phase 2 tools currently return success=False placeholders, so the suite will deterministically fail whenever an emulator is present. If these are intended scaffolds, mark them as xfail (with a clear reason) or skip until the tool implementations exist; otherwise, update the tools so the tests can pass when the emulator is reachable.
| @@ -0,0 +1 @@ | |||
| {"session_id":"2a799d2f-5602-4806-8bbd-8f6947137697","transcript_path":"C:\\Users\\pmacl\\.claude\\projects\\D---projects-ALAS\\2a799d2f-5602-4806-8bbd-8f6947137697.jsonl","cwd":"D:\\_projects\\ALAS","model":{"id":"claude-sonnet-4-6","display_name":"Sonnet 4.6"},"workspace":{"current_dir":"D:\\_projects\\ALAS","project_dir":"D:\\_projects\\ALAS","added_dirs":[]},"version":"2.1.69","output_style":{"name":"default"},"cost":{"total_cost_usd":1.4690392499999998,"total_duration_ms":3217190,"total_api_duration_ms":302861,"total_lines_added":0,"total_lines_removed":0},"context_window":{"total_input_tokens":65,"total_output_tokens":11703,"context_window_size":200000,"current_usage":{"input_tokens":3,"output_tokens":377,"cache_creation_input_tokens":672,"cache_read_input_tokens":64960},"used_percentage":33,"remaining_percentage":67},"exceeds_200k_tokens":false} No newline at end of file | |||
There was a problem hiding this comment.
This file appears to be a local runtime artifact (contains a user-specific transcript path and session metadata) and should not be committed to the repo. Please remove .omc/ state files from version control and add an ignore rule for .omc/ (or at least .omc/state/ and .omc/sessions/) to prevent leaking local paths and churn in future PRs.
| {"session_id":"2a799d2f-5602-4806-8bbd-8f6947137697","transcript_path":"C:\\Users\\pmacl\\.claude\\projects\\D---projects-ALAS\\2a799d2f-5602-4806-8bbd-8f6947137697.jsonl","cwd":"D:\\_projects\\ALAS","model":{"id":"claude-sonnet-4-6","display_name":"Sonnet 4.6"},"workspace":{"current_dir":"D:\\_projects\\ALAS","project_dir":"D:\\_projects\\ALAS","added_dirs":[]},"version":"2.1.69","output_style":{"name":"default"},"cost":{"total_cost_usd":1.4690392499999998,"total_duration_ms":3217190,"total_api_duration_ms":302861,"total_lines_added":0,"total_lines_removed":0},"context_window":{"total_input_tokens":65,"total_output_tokens":11703,"context_window_size":200000,"current_usage":{"input_tokens":3,"output_tokens":377,"cache_creation_input_tokens":672,"cache_read_input_tokens":64960},"used_percentage":33,"remaining_percentage":67},"exceeds_200k_tokens":false} | |
| {} |
Code Review Roast 🔥Verdict: 3 Issues Found | Recommendation: Address Overview
Issue Details (click to expand)
🏆 Best part: Oh wait, the scaffolding code is actually clean. I need to sit down. The deterministic tool framework with strict contract enforcement via 💀 Worst part: The 📊 Overall: Like finding a surprisingly organized garage sale — the junk shouldn't be there, but the valuable stuff is actually well-arranged. Files Reviewed (9 code files)New Code (Scaffolding - Well Done):
Documentation (Major Improvement):
Cleanup (Good Riddance):
Should Not Be Here:
|
Phase-2 review fixes: run loop registry-first dispatch with fallback to direct executor, and fix phase2 live tests to actually detect emulator presence instead of always skipping. Includes regression coverage in loop tests.