Two deferred items from the audit pass on PR #67, both non-blocking. Tracking them here so they don't get lost.
Context
PR #67 (declarative policy engine + denial explanation + dry-run, closing #42, #48, #43) went through a respond-audit-respond review cycle. The audit surfaced six findings; three must-fixes (canonical docs + comparative test) and one trivial fix (annotation widening) were resolved in 5a4303f. The two below were deferred.
D. policy_dsl.py is 503 lines (AGENTS.md asks for ≤300)
AGENTS.md "Quality bar" says: "Keep modules ≤ 300 lines. Split if needed." After PR #67 the file sits at 503 lines:
Similar over-budget files in the policy/kernel area: policy.py (520 lines), kernel.py (466 lines).
Proposed change
Extract _parse and _parse_rule into a small policy_dsl_parser.py module, leaving DeclarativePolicyEngine.evaluate and .explain in policy_dsl.py. Re-export the public symbols (DeclarativePolicyEngine, PolicyMatch, PolicyRule) from policy_dsl.py unchanged so __init__.py doesn't need to move.
Same pattern can be applied to policy.py (extract RateLimiter into rate_limit.py).
Acceptance criteria
E. Dry-run not explicitly tested with HTTP and MCP drivers
Issue #43's acceptance criterion includes "Works with all driver types (InMemory, HTTP, MCP)". The dry-run short-circuit happens before driver dispatch (kernel.py:248), so the mode is provably driver-agnostic — but tests/test_kernel.py only exercises InMemoryDriver. A future refactor that moved driver lookup before the if dry_run: block would not be caught by current tests.
Proposed change
Add a parameterised test that registers an HTTPDriver and an MCPDriver (constructed via MCPDriver.from_stdio against a no-op echo command, or a fake SessionFactory) and asserts:
kernel.invoke(token, principal=..., args={}, dry_run=True) returns a DryRunResult for both driver types
driver.execute() is never called (mock-and-assert)
DryRunResult.driver_id matches the registered driver
Acceptance criteria
Related
Two deferred items from the audit pass on PR #67, both non-blocking. Tracking them here so they don't get lost.
Context
PR #67 (declarative policy engine + denial explanation + dry-run, closing #42, #48, #43) went through a respond-audit-respond review cycle. The audit surfaced six findings; three must-fixes (canonical docs + comparative test) and one trivial fix (annotation widening) were resolved in 5a4303f. The two below were deferred.
D.
policy_dsl.pyis 503 lines (AGENTS.md asks for ≤300)AGENTS.md"Quality bar" says: "Keep modules ≤ 300 lines. Split if needed." After PR #67 the file sits at 503 lines:main._parse_rule(~50 lines) and a refactoredexplain()with deny-rule disambiguation (~40 lines).Similar over-budget files in the policy/kernel area:
policy.py(520 lines),kernel.py(466 lines).Proposed change
Extract
_parseand_parse_ruleinto a smallpolicy_dsl_parser.pymodule, leavingDeclarativePolicyEngine.evaluateand.explaininpolicy_dsl.py. Re-export the public symbols (DeclarativePolicyEngine,PolicyMatch,PolicyRule) frompolicy_dsl.pyunchanged so__init__.pydoesn't need to move.Same pattern can be applied to
policy.py(extractRateLimiterintorate_limit.py).Acceptance criteria
policy_dsl.py≤ 300 linespolicy_dsl_parser.py(new) holds_parse+_parse_ruleand their helpersfrom agent_kernel import DeclarativePolicyEngine, PolicyMatch, PolicyRule)make cigreenE. Dry-run not explicitly tested with HTTP and MCP drivers
Issue #43's acceptance criterion includes "Works with all driver types (InMemory, HTTP, MCP)". The dry-run short-circuit happens before driver dispatch (kernel.py:248), so the mode is provably driver-agnostic — but
tests/test_kernel.pyonly exercisesInMemoryDriver. A future refactor that moved driver lookup before theif dry_run:block would not be caught by current tests.Proposed change
Add a parameterised test that registers an
HTTPDriverand anMCPDriver(constructed viaMCPDriver.from_stdioagainst a no-opechocommand, or a fakeSessionFactory) and asserts:kernel.invoke(token, principal=..., args={}, dry_run=True)returns aDryRunResultfor both driver typesdriver.execute()is never called (mock-and-assert)DryRunResult.driver_idmatches the registered driverAcceptance criteria
HTTPDriverMCPDriver(or a documented skip if the MCP test dependency setup is too heavy for a unit test)driver.executewas not calledmake cigreenRelated