feat(agent_orchestrator): lock down deprecated tool bridge#58
feat(agent_orchestrator): lock down deprecated tool bridge#58Coldaine wants to merge 1 commit intofeat/adb-vision-audit-pilotfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR tightens the legacy agent_orchestrator/alas_mcp_server.py surface by restricting the deprecated generic alas_call_tool(...) bridge behind an allowlist / unsafe env override, and standardizes audit logging via the canonical parent/child JSONL audit schema in agent_orchestrator.
Changes:
- Replace ad hoc action logging in
alas_mcp_server.pywith canonical audit events (record_event/record_command) and nested parent/child correlation. - Gate the deprecated
alas_call_tool(...)bridge behindREMOTE_ALLOWED_ALAS_TOOLSandALAS_ALLOW_UNSAFE_STATE_MACHINE_CALLS. - Add/adjust tests and docs so blocked delegate attempts are audit-visible instead of silent.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/dev/logging.md | Documents guardrails around keeping the MCP surface explicit and the deprecated bridge disabled by default. |
| docs/ROADMAP.md | Records completion of locking down the deprecated generic tool bridge behind an env override. |
| docs/ARCHITECTURE.md | Clarifies that alas_mcp_server.py is deprecated and its generic bridge is disabled by default. |
| agent_orchestrator/test_mcp_audit.py | Adds coverage for subprocess audit events and parent/child nesting via audit_cli_call. |
| agent_orchestrator/test_integration_mcp.py | Updates integration test to validate adb_tap via patched _adb_run rather than ALAS click methods. |
| agent_orchestrator/test_alas_mcp.py | Adds coverage for allowlist behavior and default-blocking of alas_call_tool, with fixture cleanup for the unsafe flag. |
| agent_orchestrator/mcp_audit.py | Implements canonical audit logging with nested child-event support (event ids, context propagation, blob summarization). |
| agent_orchestrator/alas_mcp_server.py | Switches to canonical audit logging and enforces allowlist/unsafe override for the deprecated tool bridge. |
| TDD_IMPLEMENTATION_PLAN.md | Updates deprecation note to reflect that the generic internal tool bridge is disabled by default. |
|
|
||
| def _save_screenshot_png(data_b64: str, seq: int) -> str: | ||
| def _save_screenshot_png(data_b64: str) -> str: | ||
| """Save a base64 PNG to mcp_screenshots/<seq>_<ts>.png; return the path.""" |
There was a problem hiding this comment.
The _save_screenshot_png docstring still claims screenshots are saved as mcp_screenshots/<seq>_<ts>.png, but the implementation now uses {time.time_ns()}_{pid}.png. Please update the docstring to match the actual naming scheme (or reintroduce a sequence/timestamp if that format is desired).
| """Save a base64 PNG to mcp_screenshots/<seq>_<ts>.png; return the path.""" | |
| """Save a base64 PNG to mcp_screenshots/<ns_timestamp>_<pid>.png; return the path.""" |
| record_event( | ||
| tool_name="state_machine.transition", | ||
| arguments={"page": page}, | ||
| status="success", | ||
| result_summary=f"navigated to {page}", | ||
| duration_ms=0, | ||
| event_type="delegate", |
There was a problem hiding this comment.
alas_goto() logs a delegate audit event with duration_ms=0, which makes the audit trail misleading (other delegate events record real durations). Consider measuring and logging the actual elapsed time for the transition, similar to alas_call_tool() and alas_login_ensure_main().
Summary
agent_orchestratoralas_call_tool(...)bridge behind an explicit allowlist and unsafe env overrideTesting
cd agent_orchestrator && uv run pytest -q