Add NOC LHP v2 coordinator adapter

[Svaag]

Summary

• publish NOC CaseService projections and heartbeats to the central coordinator
• serve bounded read-only network snapshots to any authenticated source loop
• accept plan-only network-change preparation and verification handoffs
• mirror eligible legacy Engineering/Knowledge work into LHP v2 during shadow migration

Safety

• active/mutating MCP tools are excluded
• network-change capability returns a plan only and preserves target-local gates
• legacy mirrors are marked shadow-only and never route legacy work to SOC

Validation

• Ruff and strict mypy
• 496 pytest tests, 3 skipped

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🏅 Score: 70

🧪 PR contains tests

🔒 Security concerns Yes. The handoff payload (which may originate from untrusted sources like webhooks or Discord) directly influences MCP tool arguments in _dispatch for noc.network_snapshot.read. Although the tool name is restricted to a read-only set, the arguments are not sanitized or validated. An attacker could craft arguments that cause a read-only tool to behave unexpectedly, potentially leaking sensitive data or causing resource exhaustion. This is a prompt-injection path at the agent/tool-execution boundary.

⚡ Recommended focus areas for review Unvalidated tool arguments In _dispatch, the arguments dict from the handoff payload is passed directly to mcp_runtime.call_tool without any schema validation or sanitization beyond checking it is a dict. An attacker who can inject a handoff (e.g., via a compromised webhook or Discord message) could craft arguments that cause a read-only tool like frr_vtysh_cmd to execute arbitrary commands or leak sensitive data. This is a prompt-injection path where untrusted content influences tool execution. async def _dispatch(self, capability: str, payload: dict[str, Any]) -> dict[str, Any]: if capability == "noc.network_snapshot.read": tool = str(payload.get("tool") or "") arguments = payload.get("arguments") if tool not in SNAPSHOT_TOOLS or tool in PROACTIVE_HEAVY_TOOLS: raise ValueError(f"tool {tool!r} is not allowed for coordinator snapshots") if not isinstance(arguments, dict): raise ValueError("snapshot arguments must be an object") result = await self.mcp_runtime.call_tool("hyrule", tool, arguments) return { "summary": f"NOC returned bounded read-only snapshot {tool}", "tool_result": _bounded(result), "source": "hyrule-mcp", } Unbounded retry loop The run_worker loop has no backoff or cap on retries. If the coordinator encounters persistent errors (e.g., database connection failure, coordinator unavailability), it will keep retrying every NOC_COORDINATOR_POLL_SECONDS (default 5) indefinitely. This can lead to resource exhaustion, log flooding, and delayed recovery. A capped retry with exponential backoff should be added. while True: try: await worker.run_once() except Exception as exc: log.warning("noc_coordinator_cycle_failed", error=type(exc).__name__) await asyncio.sleep(interval) Missing argument schema validation The snapshot tool arguments are not validated against expected schemas. Different MCP tools require specific fields (e.g., host, command for frr_vtysh_cmd). Passing arbitrary or malformed arguments can cause tool failures or unexpected behavior. This reduces robustness and could mask injection attempts. async def _dispatch(self, capability: str, payload: dict[str, Any]) -> dict[str, Any]: if capability == "noc.network_snapshot.read": tool = str(payload.get("tool") or "") arguments = payload.get("arguments") if tool not in SNAPSHOT_TOOLS or tool in PROACTIVE_HEAVY_TOOLS: raise ValueError(f"tool {tool!r} is not allowed for coordinator snapshots") if not isinstance(arguments, dict): raise ValueError("snapshot arguments must be an object") result = await self.mcp_runtime.call_tool("hyrule", tool, arguments) return { "summary": f"NOC returned bounded read-only snapshot {tool}", "tool_result": _bounded(result), "source": "hyrule-mcp", }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle claim failure gracefully The run_once method does not handle the case where client.claim fails (e.g., if another worker already claimed the handoff). If claim raises an exception, the handoff is not processed but the error is caught by the outer except Exception block, which incorrectly reports it as a failure. Instead, catch CoordinatorError specifically for the claim step and skip the handoff gracefully without logging a failure. app/coordination.py [88-137] async def run_once(self) -> dict[str, Any]: report: dict[str, Any] = { "projected_cases": 0, "mirrored_handoffs": 0, "checked": 0, "completed": [], "failed": [], } await self.client.heartbeat( LoopHeartbeat(loop_id="noc", status="active", summary="NOC coordinator worker active") ) report["projected_cases"] = await self._project_cases() report["mirrored_handoffs"] = await self._mirror_legacy_handoffs() for record in await self.client.inbox(status="queued"): capability = record.envelope.capability if capability not in { "noc.network_snapshot.read", "noc.network_change.prepare", "noc.verify", }: continue report["checked"] += 1 handoff_id = record.envelope.handoff_id try: await self.client.claim(handoff_id) + except CoordinatorError: + continue # Another worker claimed it + try: await self.client.progress(handoff_id, f"NOC processing {capability}") payload = await self._dispatch(capability, record.envelope.payload) await self.client.submit_result( HandoffResult( handoff_id=handoff_id, outcome="succeeded", summary=str(payload.pop("summary", f"NOC completed {capability}")), payload=payload, ) ) report["completed"].append(handoff_id) except Exception as exc: log.warning("noc_coordinator_handoff_failed", handoff_id=handoff_id, error=type(exc).__name__) try: await self.client.submit_result( HandoffResult( handoff_id=handoff_id, outcome="failed", summary=f"NOC failed {capability}: {type(exc).__name__}", ) ) except Exception: pass report["failed"].append({"handoff_id": handoff_id, "error": str(exc)[:300]}) return report Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that a claim failure (e.g., already claimed by another worker) should not be treated as a processing failure. The improved code catches CoordinatorError specifically and skips the handoff gracefully, which improves robustness in a concurrent environment. This is a high-impact improvement.	Medium
Security	Validate snapshot arguments for safety The _dispatch method for noc.network_snapshot.read passes arguments directly to call_tool without validating that it contains only safe, non-mutating parameters. An attacker or misconfigured caller could inject arguments that cause side effects (e.g., "command": "write memory"). Validate that the arguments do not contain any keys or values that could trigger configuration changes or destructive operations. app/coordination.py [139-152] async def _dispatch(self, capability: str, payload: dict[str, Any]) -> dict[str, Any]: if capability == "noc.network_snapshot.read": tool = str(payload.get("tool") or "") arguments = payload.get("arguments") if tool not in SNAPSHOT_TOOLS or tool in PROACTIVE_HEAVY_TOOLS: raise ValueError(f"tool {tool!r} is not allowed for coordinator snapshots") if not isinstance(arguments, dict): raise ValueError("snapshot arguments must be an object") + # Reject arguments that could trigger mutations + if any(k in arguments for k in ("action", "commit", "apply", "write", "save")): + raise ValueError("snapshot arguments contain potentially mutating keys") result = await self.mcp_runtime.call_tool("hyrule", tool, arguments) return { "summary": f"NOC returned bounded read-only snapshot {tool}", "tool_result": _bounded(result), "source": "hyrule-mcp", } Suggestion importance[1-10]: 7 __ Why: The suggestion adds a security check for potentially mutating argument keys, which is a valid concern. However, the specific keys checked (e.g., "action", "commit") may not cover all possible dangerous arguments, and the suggestion does not consider that the tool itself (e.g., frr_vtysh_cmd) might already be read-only. The improvement is moderate but not critical.	Medium