Repository navigation
feat: add kernel filesystem-policy engine adapter (feature-flagged) - #57
kevin-orellana wants to merge 3 commits into
Conversation
## Problem Native filesystem integration needs the existing Box policy authority. Landing that connection separately keeps platform enforcement out of this initial review. ## Solution Add a private adapter behind the non-default kernel-policy-integration build feature. HostedBox passes its existing engine and box identity to that adapter. Keep existing operation types, the fixed agent principal, and reported path spelling. Submit permission requests and outcome records separately. Return recording errors. Neither default nor feature-enabled builds install kernel hooks. Configuration, sandbox grants, and policy vocabulary remain unchanged. ## Tests The native_checks_keep_the_existing_actions_identity_and_path_spelling test pins action, operation, principal, and path compatibility. The admission_does_not_record_completion_and_outcomes_keep_request_spelling test pins separate outcome records. The shell_and_native_checks_share_one_history test pins history propagation through both real adapters. The native_adapter_submits_to_the_hosted_authority test pins production construction with the shared engine. The a_recording_error_reaches_the_caller test pins propagation of a controlled recording error. Deliberate defects in these contracts caused the respective tests to fail. On macOS 27.0.1, all-feature and no-default builds, format checks, and just clippy passed. The workspace script reported 4,040 passes, 92 temporary-path failures, and 17 ignored tests. All 92 affected cases passed with a short temporary directory. The deterministic suite passed 121 cases and skipped eight Linux-only cases. Environment-dependent workspace skips remain; no scripted examples exist. Linux builds, native interception, and storage-error fault injection were not tested.
| #[cfg(not(test))] | ||
| let record = PolicyEngine::record; | ||
| #[cfg(test)] | ||
| let record = self.record_outcome; |
There was a problem hiding this comment.
Issue: The #[cfg(not(test))] arm of record (let record = PolicyEngine::record;) is never compiled under cargo test, so the exact wiring that ships is never test-executed. The non-error tests exercise the functionally-equivalent default function pointer instead, and a_recording_error_reaches_the_caller overrides it. The two paths are equivalent today, but the seam means the shipped line and the tested line are not the same line.
Suggestion: Consider holding record_outcome in all builds (default PolicyEngine::record) rather than behind #[cfg(test)]. That removes the cfg split in record, and the production path becomes the one the tests drive. This is a minor, non-blocking point given the paths are equivalent and the PR already notes a real storage-device failure could not be injected.
There was a problem hiding this comment.
Good point. Looking into and considering adoption.
There was a problem hiding this comment.
Thanks for turning this around in fix: share outcome recorder dispatch across builds. I re-reviewed the new revision (03c4936): record_outcome is now a field in all builds defaulting to PolicyEngine::record, and record dispatches through it with no cfg split — so the shipped path is the tested path, and the error-injection test still exercises the same seam. Re-verified locally: cargo fmt --check, clippy (0 box warnings, no dead-code on the always-present field), the feature build, and all 5 native tests (582 total, 0 failed) pass. This resolves the suggestion — nice.
|
Assessment: Approve The PR delivers exactly what it claims: an inactive, feature-gated adapter that reuses the box's single policy authority and installs no kernel hooks. I built and tested it locally; findings below. Review Categories
Well-scoped foundation with tests that pin each behavioral claim, including deliberate-defect coverage. |
|
Assessment: Approve Re-reviewed after the restructure ( Review Categories
Clean relocation that improves cohesion with the other adapters while keeping the integration inactive and well-tested. |
Description
Kernel filesystem requests need the same Dogwood policy integration that already serves Strands Shell and the Egress component. This PR starts that integration behind an off-by-default feature flag.
Changes made
KernelPolicyAdapterfromadapters/kernel.rs, behind the off-by-defaultkernel-adapterfeature. Box'skernel-policy-integrationfeature enables it inside the trusted box process.HostedBoxcomponent gives it the existing engine and box identity.context.input.pathfield. They retain the current path-reporting convention. We will evaluateResourceseparately; any migration should update Strands Shell and kernel filesystem integration together.Policy engine integration notes
The adapter preserves the existing policy contract, including path reporting and outcome recording. Recording errors reach the caller.
A separate PR will translate operating-system filesystem events into policy-engine actions and test compatibility with Strands Shell.
Release notes
This is bootstrap code. Even with the feature enabled, it installs no kernel hooks and provides no kernel filesystem protection. It adds no runtime activation switch, configuration key, policy vocabulary, sandbox grant, or network interception. Later PRs will connect the operating-system callbacks.
Please review the shared-engine ownership, existing policy contract, and feature-flagged build decision.
Related Issues
Part of strands-agents/staging-boxy#43 (private tracking repository; access required).
Type of Change
New feature: inactive integration foundation.
Testing
Unit tests:
Five new unit tests exercise the kernel-policy engine adapter:
HostedBoxsubmits to that box's existing engine.These are adapter tests. They do not yet demonstrate kernel filesystem interception.
Local validation used macOS 27.0.1:
HostedBoxtest passed. Policy and Box builds, formatting, lint, and public API documentation checks passed. The kernel-only production dependency tree excludes Shell, Monty, and the Egress component.cargo fmt --all --check, andjust clippypassed. The lint command uses the repository's allowances for vendored Shell warnings.scripts/test-all.shinitially reported 4,040 passes, 92 failures, and 17 ignored tests. The review used an overlong temporary path, which exceeded macOS socket limits. All 92 affected cases passed when rerun with the standard temporary directory. Other environment-dependent skips remain; no scripted examples are present. This was not a green first invocation.Linux was not built or tested locally.
GitHub validation at this update:
Commit
504560emoves the adapter into the policy crate. GitHub validation for this revision is pending. See the PR checks for current results.Kernel filesystem interception was not tested. The tests simulate a recording failure; actual storage-device failures were not injected.
just check, orcargo test --workspace --all-features)cargo fmt --all --checkandjust clippy, using the repository's documented lint allowances.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.