Review: Execution Governance Enhancement

This PR introduces solid foundational concepts for LLM-augmented governance, and the opt-in default, lazy initialization, and graceful degradation are all the right instincts. However there are critical issues that must be addressed before merge.

Recommendation: Request Changes

🔴 Critical Issues

1. Blocking synchronous LLM call in async FastAPI request path (src/governance/middleware.py:221)

GovernanceMiddleware.evaluate() is synchronous. When enhancement is enabled, it calls create_enhanced_plan() → planner.enhance() → llm.complete(), which is a synchronous HTTP call to the Anthropic API. In FastAPI, any synchronous blocking I/O inside a request handler stalls the event loop for every other in-flight request. LLM calls routinely take 1–5 seconds — this is a serious throughput regression when enhancement.enabled=true.

Fix options:

• Wrap the LLM call in asyncio.to_thread() / loop.run_in_executor()
• Make the governance evaluation path fully async
• Run enhancement out-of-band (fire-and-forget, post-response)

2. anthropic is a hard required dependency (pyproject.toml:11)

Every deployment now installs the Anthropic SDK even when enhancement is disabled. Move it to an optional extras group:

[project.optional-dependencies]
llm = ["anthropic>=0.30.0"]

3. Blocklist sanitization insufficient for a security layer (src/governance/planner.py:153)

_sanitize_for_llm redacts known field names (token, password, etc.), but any unknown sensitive field — api_token, bearer, private_key, x-api-key — passes through to the Anthropic API. For a governance/security system an allowlist of permitted fields is the correct posture, not a blocklist.

4. No JSON Schema validation of LLM output (src/governance/planner.py:274)

The schema is sent to the LLM as prompt instructions, but _build_enhanced_plan never validates the returned JSON against the schema before constructing EnhancedExecutionPlan. Malformed or adversarially crafted LLM output could produce unexpected model state.

Fix: use jsonschema.validate(enhanced_dict, schema) before calling _build_enhanced_plan.

🟠 Architectural Concerns

5. frozen=False breaks the immutability contract (src/governance/models.py:402)

Every other model in models.py uses frozen=True. EnhancedExecutionPlan introduces frozen=False because mutable ExecutionState is embedded in the plan. The better pattern: keep the plan frozen, manage ExecutionState as a separate mutable object held externally by an execution context or engine.

6. ~510 lines of dead execution layer (src/execution/)

engine.py (373 lines), executor.py (70 lines), injected_context.py (67 lines) are never called from the main application. Dead code increases review surface and maintenance burden. Defer to a follow-up PR once the execution path is actually wired in.

7. execution-plan-big.json is unused (config/execution-plan-big.json)

This 326-line file is never referenced in code and contains a personal reference ("from Yi's enforcement layer"). Remove it or move it to a docs/reference directory.

8. Fragile schema path resolution (src/governance/middleware.py:103)

Schema path is resolved by navigating Path(patterns_path).parent.parent, hard-wiring the assumption that patterns_path lives at config/intent-patterns.json. If that assumption breaks, schema resolution silently returns None and enhance() raises a RuntimeError. Pass schema_path as an explicit config key with an absolute path, or resolve relative to the project root via an env var.

🟡 Security Concerns

9. Prompt injection surface (src/governance/planner.py:40)

ENHANCE_PLAN_PROMPT embeds raw tool call arguments (which come from an upstream LLM) directly into the prompt with no isolation. A crafted tool argument like "Ignore all previous instructions and..." constitutes a prompt injection attack. Wrap the injected plan JSON in an explicit data-only block with clear delimiter instructions.

10. Silent exception swallowing in governance layer (src/governance/middleware.py:279)

except Exception as e: ... return None is fine for graceful degradation, but in a security governance layer it's important that enhancement failures are observable as metrics, not just as log lines. Consider incrementing a counter (e.g., Prometheus) so that a bug that silently disables enhancement can be detected via alerting rather than log scraping.

Minor Issues

• planner.py: logger = getLogger(__name__) is between import groups — violates PEP 8
• engine.py, executor.py, client.py lack trailing newlines
• generate() was reordered without implementation changes — adds diff noise
• Many unit-level tests in tests/integration/test_enhanced_plan.py belong in tests/unit/
• Duplicate section comment at line 2957 in the large test file

✅ Positives

• Enhancement is opt-in (enabled: false default) — correct safe default
• Lazy LLM client initialization prevents unnecessary API key validation on startup
• Graceful degradation: enhancement failures never block the governance pipeline
• SECURITY_AUDIT prefix log entries for LLM API calls
• Recursive sanitization implementation is solid
• Comprehensive test coverage (~2,500 lines)

Follow-up Review: Execution Governance

Significant progress since the first review — thank you for the thorough set of fixes. Six of the ten original issues are now fully resolved. One critical issue remains and must be fixed before merge.

✅ Fixed Since Last Review

🔴 Critical: Blocking synchronous I/O still present

LLMClient.complete() uses the synchronous Anthropic SDK (Anthropic().messages.create()). create_enhanced_plan() is called directly from evaluate(), which runs in the FastAPI request/response cycle. FastAPI workers are async — any synchronous blocking call in the request path stalls the event loop and prevents all other requests from being served for the duration of that call.

With DEFAULT_TIMEOUT = 60 seconds, enabling enhancement under load could stall the proxy for up to a minute per request, rendering it effectively unavailable.

This issue was explicitly called out in the first review and is still not addressed.

The fix is straightforward — choose one of:

# Option A: run in thread pool (minimal change)
import asyncio

async def create_enhanced_plan(self, ...) -> EnhancedExecutionPlan | None:
    return await asyncio.to_thread(self._create_enhanced_plan_sync, ...)

def _create_enhanced_plan_sync(self, ...) -> EnhancedExecutionPlan | None:
    ...  # existing implementation

# Option B: use the async Anthropic client
from anthropic import AsyncAnthropic

async def complete(self, prompt: str, temperature: float = 0) -> str:
    response = await self.client.messages.create(...)
    return response.content[0].text

Option A is the lowest-risk change and keeps the existing implementation intact.

🟠 Remaining Architectural Concerns

EnhancedExecutionPlan.frozen=False still breaks the immutability contract

All 16 other models in models.py use frozen=True. EnhancedExecutionPlan remains the sole exception. This is a design smell that should at minimum be addressed with a clear module-level comment explaining why the contract is broken here and what invariants callers must maintain. Better: extract ExecutionState into a separate mutable container held by the engine, and freeze the plan itself.

Dead execution layer still present

src/execution/engine.py, executor.py, and injected_context.py remain in the codebase and are never called from the main application. The engine improvements (off-by-one retry fix, completed_at ordering, _should_skip stub) are good work, but they're going into code that's never exercised in production. This is maintenance debt. A # EXPERIMENTAL - not wired into main app module docstring on each file would at least make this explicit.

execution-plan-big.json — noted as intentional

Acknowledged that you're keeping this as a reference schema. If it's meant to guide future development, move it to docs/ or add a # REFERENCE ONLY - not used in production comment at the top of the file so reviewers don't have to wonder.

Minor

• client.py imports anthropic twice with separate try/except ImportError blocks in __init__ and complete(). Since the import is cached by Python's module system, the second block in complete() is redundant — from anthropic import APIError will succeed if __init__ passed. The guard in complete() can be removed.
• jsonschema.ValidationError raised inside enhance() is caught by create_enhanced_plan()'s broad except Exception handler and becomes a WARNING log + None return. This is intentional (best-effort enhancement) but worth a comment to that effect at the enhance() call site.

Verdict

The security improvements (allowlist sanitization, schema validation, prompt injection mitigation) are solid and well-implemented. But the event loop blocking issue is a functional correctness problem that affects any deployment that enables enhancement — it's not a future concern, it's a current bug. Fix the async issue and this PR is close to merge-ready.

LGTM