Review: Agent Capability Boundaries

Summary

The implementation correctly extends AgentPolicy with five new fields for read-path and profile-override access control, enforces them at the session layer, validates them at schema build time, and surfaces violations through the existing TidalError::PolicyViolation path. All 12 acceptance criteria from the spec are met by the 17 integration tests.

Spec Conformance

Acceptance Criterion	Status	Notes
AC1: `allowed_read_signals` allow-list enforcement	PASS	`test_read_allowed_signal_succeeds`, `test_read_disallowed_signal_fails_not_allowed`
AC2: `denied_read_signals` deny-list enforcement	PASS	`test_read_denied_signal_fails_denied`
AC3: `allowed_user_attributes` allow/deny	PASS	`test_attribute_read_allowed_succeeds`, `test_attribute_read_disallowed_fails`, `test_attribute_read_denied_by_deny_list`
AC4: Empty `allowed_profile_overrides` blocks overrides	PASS	`test_profile_override_disallowed_fails`
AC5: Explicit profile override list enforced	PASS	`test_profile_override_allowed_proceeds`
AC6: Violations recorded in audit log	PASS	`test_audit_log_records_read_denial`
AC7: Empty-policy zero-regression	PASS	`test_empty_policy_no_regression`
AC8: Schema rejects unknown signal in allow-list	PASS	`test_schema_build_fails_unknown_read_signal`
AC9: Schema rejects allow/deny conflict	PASS	`test_schema_build_fails_read_signal_conflict`
AC10: Schema rejects unknown profile override name	PASS	`test_schema_build_fails_unknown_profile_override`
AC11: `signals_rejected` and `overrides_rejected` counters	PASS	`test_signals_rejected_incremented_on_read_denial`, `test_overrides_rejected_incremented`
AC12: Ungated `read_decay_score` unaffected	PASS	`test_read_without_session_unrestricted`

Design Conformance

The implementation follows the design document exactly:

AgentPolicy in schema/validation/policies.rs: 5 new fields, empty-default, ..Default::default() propagated to all existing struct literals.
PolicyEvaluator in session/policy.rs: check_read(), check_attribute_read(), check_profile_override() added with correct deny-before-allow ordering.
AuditKind in session/audit.rs: ReadDenied, AttributeReadDenied, ProfileOverrideRejected added.
SessionState in session/state.rs: overrides_rejected: AtomicU64 was already present from a prior session; SessionSnapshot correctly reads it via overrides_rejected.load(Relaxed) in both build_snapshot and build_frozen_snapshot (previously hardcoded to 0 — this was a pre-existing bug that the feature surfaced and fixed).
db/signals.rs: 4 new public session-gated read methods and 2 private helpers.
db/query_ops.rs: enforce_profile_override_policy() helper added, called in both retrieve() and search().
SchemaBuilder in schema/validation/builders/mod.rs: declare_profile_names() added; known_profile_names field used for profile-override validation; "*" sentinel expansion implemented.

The design noted that SchemaBuilder::new() would need to change from const fn to regular fn due to Vec initialization. This was handled correctly.

One design note: the spec described a SessionPolicyBuilder fluent API for building policies, but the implementation takes a simpler direct approach — users construct AgentPolicy structs directly and pass them to SchemaBuilder::session_policy(). This is acceptable; the fluent builder was a suggestion in the spec, not a hard requirement, and the direct struct approach is simpler and avoids an intermediate builder type.

Code Quality

signals.rs — The two #[allow(clippy::significant_drop_tightening)] attributes are correctly scoped to check_session_read_policy and check_session_attribute_read_policy. The lint fires because the DashMap guard is held across an if let chain that could be tightened; the allow is justified since tightening would require restructuring the borrow around a ? operator in a way that makes the code less readable.

query_ops.rs — enforce_profile_override_policy() is a clean private helper. The EntityId import that was erroneously added was removed. The for_session check correctly delegates to the new helper for both retrieve() and search().

policy.rs — The check_read implementation matches the design sequence exactly: deny list first (O(n) scan), then allow list (O(n) scan, short-circuits when empty). The 12 new unit tests in the existing test block cover the edge cases. The check_profile_override implementation correctly treats an empty allowed_profile_overrides as "no overrides permitted at all," which matches the spec.

lib.rs — AuditKind, PolicyViolation, PolicyViolationKind are re-exported for integration test use. These were previously crate-internal; the re-export makes them accessible without requiring callers to traverse internal module paths.

Issues Found

None. The implementation is clean, the tests are complete, clippy passes with -D warnings, and all 17 integration tests pass.

Test Coverage

17 integration tests covering all spec acceptance criteria. 12 unit tests in session/policy.rs covering the three new PolicyEvaluator methods. Total test count at time of completion: 1367 lib + 17 m10 integration tests.

Verdict

APPROVED — implementation is correct, complete, and well-tested.

5.4 KiB Raw Blame History