feat(governance): unify policy engine on Capability + migrate MCP#495
Conversation
Drops the PermissionRequest bridge from the policy decision path: policies now receive the structured Capability variant directly, together with Provenance and a dispatcher-supplied description used for the user-facing prompt. Capability-blind policies become capability-aware in one step - they can reach FileWrite.path, HttpFetch.url, McpInvoke.server, etc. Adds a structural hard-denial layer on DefaultPermissionPolicy that overrides every mode (including auto-accept) for writes/deletes targeting .env*, .ssh/, .aws/, .gnupg/, credentials.json, id_rsa, id_ed25519, .netrc, .git/config, /etc/*. Matching is segment-level (not String.contains) and paths are normalized first so /tmp/../etc/hosts cannot bypass the /etc/ rule. Tests cover all 4 modes, the hard-denial cases, false-positive guards (dotenv-notes.md, docs/etc/notes.md, .git/HEAD), the traversal-bypass regression, and confirm the dispatcher's rich description still reaches the prompt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
McpToolBridge now produces a structured Capability.McpInvoke(server, method) instead of falling through the LegacyToolUse bridge. With this change every active adapter (Bash, File, Browser, Skill, HTTP, OsScript, Memory, SubAgentSpawn, ScreenCapture, MCP) goes through the same structured permission + audit path - the unified-capability-abstraction goal in runtime-governance.md is met. Stores the serverName as a field (was only available at create-time before), threaded through the private constructor + factory. Args payload is intentionally not carried on McpInvoke - it can be huge and may carry secrets; policies decide on (server, method) alone. Audit log entries for MCP calls now show @type=McpInvoke with server/method fields rather than @type=LegacyToolUse, making them filterable in dashboard timelines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates permission evaluation to structured Capability + Provenance + description, adds a structural hard-denial layer for sensitive file targets, updates PermissionManager and sub-agent wiring to run structural checks, makes MCP tools CapabilityAware with toCapability inference, and updates tests accordingly. ChangesPermission Policy Capability Refactoring
Sequence DiagramsequenceDiagram
participant SubAgent
participant ToolRegistry
participant Tool
participant PermissionManager
participant PermissionPolicy
SubAgent->>ToolRegistry: request tool by name
ToolRegistry->>Tool: retrieve tool
alt Tool implements CapabilityAware
Tool->>PermissionManager: toCapability(args) => Capability
PermissionManager->>PermissionPolicy: evaluateStructural(Capability)
alt structural denied
PermissionPolicy-->>PermissionManager: Denied (reason)
PermissionManager->>SubAgent: deny (structural reason)
else no structural denial
PermissionManager->>PermissionPolicy: evaluate(Capability, Provenance, description)
PermissionPolicy-->>PermissionManager: Approved / NeedsUserApproval / Denied
PermissionManager->>SubAgent: final decision
end
else legacy tool
SubAgent->>PermissionManager: legacy routing -> LegacyToolUse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f6a85481f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 1. Structural hard denial — overrides every mode, including auto-accept. | ||
| var denial = checkHardDenial(capability); | ||
| if (denial != null) return denial; |
There was a problem hiding this comment.
Because this hard-denial layer is only inside PermissionPolicy.evaluate, it is skipped whenever PermissionManager.check returns early for a session blanket approval before invoking the policy. In a session where the user previously chose “always allow” for write_file/edit_file, a later structured FileWrite to .env, .ssh/*, or /etc/* is auto-approved and never reaches this new sensitive-path check, so the invariant that sensitive paths are refused regardless of mode is not actually enforced. Put the structural denial ahead of the blanket-approval shortcut, or have the manager consult it before returning the remembered approval.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@aceclaw-mcp/build.gradle.kts`:
- Line 5: Change the dependency scope from implementation to api for the
aceclaw-security project because aceclaw-mcp exposes types from that module
(e.g., Capability, CapabilityAware, and McpToolBridge.toCapability()) in its
public API; update the dependency declaration that currently uses
implementation(project(":aceclaw-security")) to use
api(project(":aceclaw-security")) so downstream modules get transitive access to
those exported types.
In
`@aceclaw-security/src/main/java/dev/aceclaw/security/DefaultPermissionPolicy.java`:
- Around line 104-107: Add explicit null guards at the public policy boundary by
calling Objects.requireNonNull on method parameters used downstream: in
DefaultPermissionPolicy.evaluate(Capability capability, Provenance provenance,
String description) validate capability and description (and provenance if it is
dereferenced later) at the top using Objects.requireNonNull(..., "paramName") so
nulls are rejected early before calls to checkHardDenial(capability), risk(...),
or formatPrompt(...); this keeps NPEs actionable and follows the guideline for
parameters passed to downstream methods like checkHardDenial, risk, and
formatPrompt.
- Around line 215-221: The DANGEROUS case in DefaultPermissionPolicy currently
includes a trailing colon which, combined with the colon in the return
String.format("The agent wants to %s: %s", action, description), produces a
double colon; update the DANGEROUS branch in the switch on capability.risk() to
remove the trailing ":" (or alternatively remove the ":" in the format string)
so that the variable action contains no colon and the final String.format call
yields a single colon before description (locate the switch producing action and
the return String.format in DefaultPermissionPolicy).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 760f1acf-b127-4515-826c-bb47a346d235
⛔ Files ignored due to path filters (1)
docs/runtime-governance.mdis excluded by!**/*.md,!docs/**
📒 Files selected for processing (11)
aceclaw-daemon/src/test/java/dev/aceclaw/daemon/ToolPermissionRouterTest.javaaceclaw-mcp/build.gradle.ktsaceclaw-mcp/src/main/java/dev/aceclaw/mcp/McpToolBridge.javaaceclaw-mcp/src/test/java/dev/aceclaw/mcp/McpToolBridgeTest.javaaceclaw-security/src/main/java/dev/aceclaw/security/DefaultPermissionPolicy.javaaceclaw-security/src/main/java/dev/aceclaw/security/PermissionManager.javaaceclaw-security/src/main/java/dev/aceclaw/security/PermissionPolicy.javaaceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.javaaceclaw-security/src/test/java/dev/aceclaw/security/PermissionManagerAuditTest.javaaceclaw-security/src/test/java/dev/aceclaw/security/PermissionManagerCapabilityTest.javaaceclaw-security/src/test/java/dev/aceclaw/security/PermissionManagerTest.java
Codex P1 on #495: the hard-denial layer was only inside DefaultPermissionPolicy.evaluate, but PermissionManager.check returns early on a session-blanket approval before calling evaluate. A user who clicked "always allow write_file" would auto-approve a later FileWrite(.env) - the "overrides all modes" invariant of the hard-denial layer didn't actually hold. Splits hard-denials onto a new PermissionPolicy.evaluateStructural that returns a typed @nullable PermissionDecision.Denied. PermissionManager runs it FIRST, before the blanket lookup, so structural denials can't be routed past by any allowlist state. Regression test pins the bypass scenario: PermissionManagerCapabilityTest.structuralDenialFiresEvenWithSessionBlanketApproval. Existing structural-denial unit tests now exercise evaluateStructural directly; mode tests still exercise evaluate. Also addresses CodeRabbit findings on the same PR: - DefaultPermissionPolicy.evaluate now Objects.requireNonNull-guards capability, provenance, and description at the public boundary. - The DANGEROUS prompt no longer emits a double colon ("...action:: description") - the trailing colon on the descriptor was redundant with the one in the format string. Skipped: CodeRabbit's suggestion to switch aceclaw-mcp's implementation(":aceclaw-security") to api(...). aceclaw-tools also implements CapabilityAware via implementation(...); flipping mcp alone breaks repo convention, and downstream callers (aceclaw-daemon) already import aceclaw-security directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 2 pushed in Codex P1 — hard-denial bypassed by session blanket: ✅ fixed. Split structural rules onto a new CodeRabbit — null guards on CodeRabbit — double colon in DANGEROUS prompt: ✅ fixed. The descriptor's trailing CodeRabbit — switch aceclaw-mcp's security dep from |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
aceclaw-security/src/main/java/dev/aceclaw/security/PermissionManager.java (1)
142-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the method Javadoc to match the new evaluation order.
The header above this overload still says "session allowlist first, then policy" and "constructing a
PermissionRequeston the fly", but this implementation now runsevaluateStructural(...)before the allowlist and callspolicy.evaluate(capability, provenance, description)directly. Inaceclaw-security, that stale contract makes the new non-bypassable ordering easy to misunderstand. As per coding guidelines,aceclaw-security/**: Review security module changes with extra scrutiny. Check for permission bypasses, HMAC integrity issues, and unsafe defaults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aceclaw-security/src/main/java/dev/aceclaw/security/PermissionManager.java` around lines 142 - 175, The Javadoc for this PermissionManager overload is stale: it claims "session allowlist first, then policy" and mentions constructing a PermissionRequest, but the implementation now calls policy.evaluateStructural(capability) before checking sessionApprovals and invokes policy.evaluate(capability, provenance, description) directly; update the method Javadoc to describe the new evaluation order (structural denials first, then session-blanket allowlist, then full policy.evaluate with capability/provenance/description), remove any reference to an on-the-fly PermissionRequest, and call out that structural denials are non-bypassable so reviewers should pay special attention to HMAC/integrity and unsafe-default concerns.
🧹 Nitpick comments (2)
aceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.java (2)
307-310: ⚡ Quick winGuard the downcast with an explicit type assertion first.
At Line 309, casting directly to
PermissionDecision.NeedsUserApprovalcan fail with aClassCastExceptionand hide intent. Assert type first, then read prompt.Suggested tightening
var decision = evaluate(policy, write("/tmp/foo.txt"), "Write /tmp/foo.txt (123 chars of new content)"); - var prompt = ((PermissionDecision.NeedsUserApproval) decision).prompt(); + assertInstanceOf(PermissionDecision.NeedsUserApproval.class, decision); + var prompt = ((PermissionDecision.NeedsUserApproval) decision).prompt();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.java` around lines 307 - 310, The test currently downcasts decision to PermissionDecision.NeedsUserApproval without checking type; update DefaultPermissionPolicyTest to first assert the runtime type of decision (e.g., that decision is an instance of PermissionDecision.NeedsUserApproval) before casting and calling prompt(), so replace the direct cast on the decision variable (from the evaluate(...) call) with an explicit type assertion and only then read prompt() via ((PermissionDecision.NeedsUserApproval) decision).prompt().
167-182: ⚡ Quick winStrengthen structural-denial assertions to verify
Denied, not just non-null.At Line 169 and similar cases,
assertNotNull(...)can pass even ifevaluateStructural(...)returns the wrong subtype. AssertPermissionDecision.Deniedexplicitly so these tests pin the hard-denial contract.Suggested tightening
- var decision = STRUCTURAL.evaluateStructural(write("/repo/.env")); - assertNotNull(decision); - assertTrue(decision.reason().contains("sensitive path")); + var decision = STRUCTURAL.evaluateStructural(write("/repo/.env")); + assertInstanceOf(PermissionDecision.Denied.class, decision); + assertTrue(decision.reason().contains("sensitive path"));- assertNotNull(STRUCTURAL.evaluateStructural(write("/repo/.env.local"))); + assertInstanceOf(PermissionDecision.Denied.class, + STRUCTURAL.evaluateStructural(write("/repo/.env.local")));Also applies to: 198-217, 225-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.java` around lines 167 - 182, The tests currently assert non-null on STRUCTURAL.evaluateStructural(write(...)) which allows wrong subtypes; update the assertions in methods writingDotEnvLocalIsStructurallyDenied, writingDotEnvProductionIsStructurallyDenied (and the other mentioned ranges) to explicitly check for a Denied decision by asserting the returned PermissionDecision is an instance/equals PermissionDecision.Denied (or using decision.isDenied()) after calling STRUCTURAL.evaluateStructural(write(...)), and when applicable also assert the decision.reason() contains the expected "sensitive path" text (as done in writingDotEnvIsStructurallyDenied) to firmly pin the hard-denial contract for evaluateStructural.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@aceclaw-security/src/main/java/dev/aceclaw/security/PermissionManager.java`:
- Around line 142-175: The Javadoc for this PermissionManager overload is stale:
it claims "session allowlist first, then policy" and mentions constructing a
PermissionRequest, but the implementation now calls
policy.evaluateStructural(capability) before checking sessionApprovals and
invokes policy.evaluate(capability, provenance, description) directly; update
the method Javadoc to describe the new evaluation order (structural denials
first, then session-blanket allowlist, then full policy.evaluate with
capability/provenance/description), remove any reference to an on-the-fly
PermissionRequest, and call out that structural denials are non-bypassable so
reviewers should pay special attention to HMAC/integrity and unsafe-default
concerns.
---
Nitpick comments:
In
`@aceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.java`:
- Around line 307-310: The test currently downcasts decision to
PermissionDecision.NeedsUserApproval without checking type; update
DefaultPermissionPolicyTest to first assert the runtime type of decision (e.g.,
that decision is an instance of PermissionDecision.NeedsUserApproval) before
casting and calling prompt(), so replace the direct cast on the decision
variable (from the evaluate(...) call) with an explicit type assertion and only
then read prompt() via ((PermissionDecision.NeedsUserApproval)
decision).prompt().
- Around line 167-182: The tests currently assert non-null on
STRUCTURAL.evaluateStructural(write(...)) which allows wrong subtypes; update
the assertions in methods writingDotEnvLocalIsStructurallyDenied,
writingDotEnvProductionIsStructurallyDenied (and the other mentioned ranges) to
explicitly check for a Denied decision by asserting the returned
PermissionDecision is an instance/equals PermissionDecision.Denied (or using
decision.isDenied()) after calling STRUCTURAL.evaluateStructural(write(...)),
and when applicable also assert the decision.reason() contains the expected
"sensitive path" text (as done in writingDotEnvIsStructurallyDenied) to firmly
pin the hard-denial contract for evaluateStructural.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1b08c34-e1cb-4a6c-a475-2a54ab08a7f3
📒 Files selected for processing (5)
aceclaw-security/src/main/java/dev/aceclaw/security/DefaultPermissionPolicy.javaaceclaw-security/src/main/java/dev/aceclaw/security/PermissionManager.javaaceclaw-security/src/main/java/dev/aceclaw/security/PermissionPolicy.javaaceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.javaaceclaw-security/src/test/java/dev/aceclaw/security/PermissionManagerCapabilityTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- aceclaw-security/src/main/java/dev/aceclaw/security/DefaultPermissionPolicy.java
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 511f30c543
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public Capability toCapability(JsonNode args) { | ||
| return new Capability.McpInvoke(serverName, mcpToolName); |
There was a problem hiding this comment.
When an MCP server exposes a file-writing method, this new adapter always reports McpInvoke(server, method) and drops the args containing the target path, so DefaultPermissionPolicy.evaluateStructural never sees a FileWrite/FileDelete and cannot apply the sensitive-path denial. In a session where the user previously remembered approval for mcp__<server>__write_file on a safe path (or in auto-accept), a later MCP call writing .env/.ssh/* is approved because the structural check returns null for McpInvoke. Fresh evidence beyond the earlier session-blanket finding is that this new MCP CapabilityAware path never emits the file capability that the hard-denial switch matches.
Useful? React with 👍 / 👎.
…les fire Second Codex P1 on #495: McpToolBridge.toCapability() always emitted Capability.McpInvoke(server, method) and dropped the args carrying the target path, so DefaultPermissionPolicy.evaluateStructural never saw a FileWrite/FileDelete for MCP-driven writes. An MCP filesystem server's write_file(path=".env") slipped past the structural hard-denial layer that the built-in write_file tool refuses. Best-effort inference at the MCP boundary: when the method name has clear write/delete intent (write_*, create_*, edit_*, append_*, put_*, save_*, delete_*, remove_*, unlink_*, rm_*, plus *_write/*_delete/etc. suffixes) AND the args carry a path-shaped field (path, file_path, filepath, filename, file), emit Capability.FileWrite(path, OVERWRITE) or Capability.FileDelete(path) instead of McpInvoke. Conservative on both sides: ambiguous method names or missing path args fall through to McpInvoke and get the standard prompt. The args payload itself is still NOT retained on either variant - secrecy and size concerns are unchanged; policies decide on (server, method) / path alone. Also addresses CodeRabbit's repeated Major finding on aceclaw-mcp/build.gradle.kts: flipped implementation(":aceclaw-security") to api(...) since McpToolBridge.toCapability() exposes Capability in its public return type. aceclaw-tools has the same shape and still uses implementation(); a follow-up consistency PR can flip it too. Tests: 8 new cases in McpToolBridgeTest covering each inference path (write/delete verbs, missing path arg, opaque method name, read method, file_path alias) plus an end-to-end integration test composing toCapability through DefaultPermissionPolicy.evaluateStructural to pin the MCP file-write-to-.env denial. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 3 in Codex P1 — MCP file methods bypass the structural denial layer: ✅ fixed. CodeRabbit — Args payload is still not retained on either variant — same secrecy/size rationale as before. Inference happens at the boundary where the args JSON is available, then is discarded. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22fdab7136
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| String name = fileName.toString(); | ||
| if (SENSITIVE_FILENAMES.contains(name)) { | ||
| return deniedSensitive(verb, path); | ||
| } | ||
| // .env, .env.local, .env.production all match — but NOT | ||
| // dotenv-notes.md or env-template (basename must START with .env). | ||
| if (name.startsWith(".env")) { |
There was a problem hiding this comment.
On case-insensitive filesystems such as the default Windows/macOS setups, the new hard-denial layer can be bypassed with differently cased names: .ENV.local, .SSH/config, or Credentials.json refer to the same sensitive locations/operators' files but do not match these case-sensitive comparisons, so accept-edits/auto-accept can still approve the write/delete. Normalize the basename and each segment with a stable locale before comparing when enforcing this invariant.
Useful? React with 👍 / 👎.
Codex P2 on #495: on case-insensitive filesystems (default macOS APFS, Windows NTFS), `.ENV` and `.env` resolve to the same underlying file. Structural denials that compared case-sensitively could be bypassed with differently-cased names (`.ENV.local`, `.SSH/config`, `Credentials.json`, `/ETC/hosts`) while still hitting the same sensitive file. Lowercase the basename + each path segment under Locale.ROOT before comparing. SENSITIVE_FILENAMES and SENSITIVE_PATH_SEGMENTS sets were already lowercase; only the comparison side changed. Locale.ROOT chosen for stability — avoids the Turkish-locale i/İ surprise that LANG-dependent toLowerCase() would introduce. Six regression tests pin each surface area: .ENV basename, .Env.local prefix, Credentials.json basename, .SSH segment, .GIT segment, /ETC absolute-prefix. Linux behavior is unchanged for the common case; the small false-positive surface (Linux file literally named `.ENV` would now be denied) is intentional — the structural layer prefers a slightly conservative answer over a cross-platform bypass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 4 in Codex P2 — case-insensitive bypass on macOS/Windows: ✅ fixed. Lowercase basename + each segment under I had actually noted this case-sensitivity gap in my own self-review and dismissed it as a Linux-correct edge case — that was wrong. The structural layer needs to be cross-platform safe (especially given the project runs on macOS heavily, default APFS being case-insensitive). Thanks for the catch. Locale.ROOT chosen over the default locale to avoid the Turkish |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b9acbbf4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static final Pattern WRITE_VERB = Pattern.compile( | ||
| "^(write|create|edit|append|put|save)(_.*)?$|.*_(write|create|edit|append|save)$"); | ||
|
|
||
| /** Method names with clear file-delete intent (matched against lowercased mcpToolName). */ | ||
| private static final Pattern DELETE_VERB = Pattern.compile( | ||
| "^(delete|remove|unlink|rm)(_.*)?$|.*_(delete|remove|unlink)$"); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier opaque-MCP finding is that this new inference only recognizes explicit write/delete verbs and top-level path-style fields, so filesystem MCP methods such as move_file/rename with a destination target still fall through as McpInvoke. In a session with auto-accept or a remembered approval for that MCP method, moving/renaming a file to /repo/.env or under .ssh/ never becomes FileWrite/FileDelete, so DefaultPermissionPolicy.evaluateStructural cannot enforce the sensitive-path hard denial the commit is trying to guarantee.
Useful? React with 👍 / 👎.
Third Codex P1 on #495: the earlier write/delete inference missed two-arg ops on MCP filesystem servers. `move_file(source=safe.txt, destination=.env)` and `rename(old_path, new_path=.env)` both fell through as McpInvoke, so structural denial never saw the sensitive destination. Adds inference for move/rename/copy: - If a destination-style field resolves (destination, dest, target, to, new_path, output_path, output) → emit FileWrite(dest, OVERWRITE). Catches "write to .env via move/copy" — the destination-write attack. - Else if the op is a move (not a copy) and a source-style field resolves (source, src, from, old_path, input_path, input) → emit FileDelete(source). Catches "move .env elsewhere" which effectively deletes the sensitive source. Conservative as before: ambiguous patterns fall through to McpInvoke with the standard prompt. The COPY_VERB pattern distinguishes copies (don't delete source) from moves (do). Note on capability classification: emitting FileWrite for a benign non-sensitive move means the capability type differs from the literal MCP semantics. The session-blanket lookup is keyed by tool name (mcp__<server>__<method>), not capability variant, so existing "always allow move_file" approvals still work. Tests: 6 new cases in McpToolBridgeTest covering each inference path (move/rename/copy with destination, move with sensitive source, copy with sensitive source — should NOT delete-infer), plus an end-to-end test composing toCapability through evaluateStructural. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 5 in Codex P1 — move/rename ops bypass structural denial: ✅ fixed. Extended
23 tests passing in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f813df01b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Path dst = safePath(extractField(args, DESTINATION_FIELDS)); | ||
| if (dst != null) return new Capability.FileWrite(dst, WriteMode.OVERWRITE); |
There was a problem hiding this comment.
When a move/rename has both a safe destination and a sensitive source, this returns only FileWrite(dst), so evaluateStructural checks the destination and never sees that the source (for example /repo/.env or ~/.ssh/config) is being removed. In accept-edits/auto-accept or with a remembered MCP approval, move_file(source="/repo/.env", destination="/tmp/env.bak") is therefore approved even though it effectively deletes a protected path; emit/check the source delete as well for non-copy operations instead of skipping it whenever a destination is present.
Useful? React with 👍 / 👎.
Fourth Codex P1 on #495: when a move/rename has a safe destination but a sensitive source, the "destination wins" rule emitted FileWrite(safe-dest) and the structural denial never saw the sensitive source being deleted. `move_file(source=/repo/.env, destination=/tmp/env.bak)` in auto-accept or with a remembered MCP approval was therefore approved despite effectively deleting a protected path. The fix probes both sides explicitly using a new public static DefaultPermissionPolicy.isSensitivePath(Path) — extracted from the existing denyIfSensitivePath logic so the bridge can reach the same rule set without duplication. Disambiguation order: 1. dst is sensitive -> FileWrite(dst) (denied via destination) 2. move + src is sensitive -> FileDelete(src) (denied via source) 3. dst present -> FileWrite(dst) (benign, standard prompt) 4. move + src present -> FileDelete(src) (benign delete prompt) 5. nothing -> McpInvoke Self-review preemptive fixes folded in: - WRITE_VERB / DELETE_VERB branches now cascade to DESTINATION_FIELDS / SOURCE_FIELDS when PATH_FIELDS is empty, so weirdly-named single writes like `write_to(destination=...)` still get classified. - Bridge javadoc spells out which method-name patterns produce which Capability variant, and the audit-log @type=FileWrite-for-moves surprise is documented. Tests: 4 new cases - safe-dst sensitive-src move (FileDelete), copy from sensitive source (stays FileWrite, copies don't delete), benign move on both sides (FileWrite as before), write_to with destination-style arg (cascade). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 6 in Codex P1 — moves that remove sensitive sources: ✅ fixed. Exposed
Regression test: Self-review preemptive fixes also in this commit:
71 tests now passing across |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 627392a70c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isMove && src != null && DefaultPermissionPolicy.isSensitivePath(src)) { | ||
| return new Capability.FileDelete(src); | ||
| } | ||
| if (dst != null) return new Capability.FileWrite(dst, WriteMode.OVERWRITE); |
There was a problem hiding this comment.
When an MCP move_file/rename has a destination and both paths are non-sensitive, this emits FileWrite, whose WRITE risk is auto-approved in accept-edits mode (DefaultPermissionPolicy approves READ/WRITE). I checked the dispatcher path: MCP tools used to default to EXECUTE in StreamingAgentHandler, so the same move previously prompted; now move_file(source="/tmp/a", destination="/tmp/b") can silently remove the source even though Capability.FileDelete is intentionally DANGEROUS to avoid auto-approving deletions. Model non-copy moves as requiring the delete-side risk, or otherwise keep them at MCP/execute risk when they remove a source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
aceclaw-mcp/src/main/java/dev/aceclaw/mcp/McpToolBridge.java (1)
62-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd null guards to factory method parameters.
serverName,mcpTool, andclientare used immediately (string concatenation, method calls onmcpTool, constructor pass-through) and should be validated at the boundary. A nullserverNamewould produce malformed qualified names like"mcp__null__..."instead of failing fast.Suggested fix
+import java.util.Objects; + public static McpToolBridge create(String serverName, McpSchema.Tool mcpTool, McpSyncClient client) { + Objects.requireNonNull(serverName, "serverName"); + Objects.requireNonNull(mcpTool, "mcpTool"); + Objects.requireNonNull(client, "client"); + var qualifiedName = "mcp__" + serverName + "__" + mcpTool.name();Per coding guidelines: "Use
Objects.requireNonNull(param, "param")on method parameters used in.equals()or passed to downstream calls."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@aceclaw-mcp/src/main/java/dev/aceclaw/mcp/McpToolBridge.java` around lines 62 - 77, Add null-checks at the start of McpToolBridge.create: validate serverName, mcpTool, and client with Objects.requireNonNull (e.g., Objects.requireNonNull(serverName, "serverName")) so the method fails fast instead of producing malformed qualifiedName or NPEs later; perform these checks before using serverName, calling mcpTool.name()/mcpTool.inputSchema(), or passing values into the McpToolBridge constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@aceclaw-mcp/src/main/java/dev/aceclaw/mcp/McpToolBridge.java`:
- Around line 199-222: The verb regexes (WRITE_VERB, DELETE_VERB,
MOVE_DEST_VERB, COPY_VERB) currently miss the documented short-suffix forms
(_put, _rm, _mv, _cp) so methods using those suffixes fall back to McpInvoke;
update each Pattern so their suffix-side alternation includes those forms: add
"_put" to WRITE_VERB's suffix alternatives, "_rm" to DELETE_VERB, and
"_mv"/"_cp" to MOVE_DEST_VERB (and ensure COPY_VERB covers "_cp" as a copy
suffix), keeping the same anchored/full-word semantics and existing prefix
alternatives (e.g., add the short tokens into the appropriate prefix group like
^(put|...|cp) or add them to the trailing _... groups) so methods like foo_put,
foo_rm, foo_mv, foo_cp match the intended verb classes.
---
Outside diff comments:
In `@aceclaw-mcp/src/main/java/dev/aceclaw/mcp/McpToolBridge.java`:
- Around line 62-77: Add null-checks at the start of McpToolBridge.create:
validate serverName, mcpTool, and client with Objects.requireNonNull (e.g.,
Objects.requireNonNull(serverName, "serverName")) so the method fails fast
instead of producing malformed qualifiedName or NPEs later; perform these checks
before using serverName, calling mcpTool.name()/mcpTool.inputSchema(), or
passing values into the McpToolBridge constructor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d9d6204-df4f-4e9d-a2ec-93d4e8ca5c1f
📒 Files selected for processing (5)
aceclaw-mcp/build.gradle.ktsaceclaw-mcp/src/main/java/dev/aceclaw/mcp/McpToolBridge.javaaceclaw-mcp/src/test/java/dev/aceclaw/mcp/McpToolBridgeTest.javaaceclaw-security/src/main/java/dev/aceclaw/security/DefaultPermissionPolicy.javaaceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.java
✅ Files skipped from review due to trivial changes (1)
- aceclaw-mcp/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (2)
- aceclaw-security/src/main/java/dev/aceclaw/security/DefaultPermissionPolicy.java
- aceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.java
… guards Codex P2 on #495: a benign MCP move like move_file(/tmp/a, /tmp/b) emitted FileWrite(dst) whose WRITE risk auto-approves in accept-edits mode, silently removing the source. Pre-#495 MCP tools defaulted to EXECUTE risk so the same move prompted. FileDelete is DANGEROUS — the closest match for "this operation removes a file" — so move semantics now always emit FileDelete(src) when src resolves and the op isn't a copy. Copies still emit FileWrite(dst) because they leave the source intact and the WRITE risk is honest. Disambiguation order in inferFileCapability(move case): 1. dst sensitive → FileWrite(dst) (denied via destination) 2. move + src present → FileDelete(src) (DANGEROUS risk, denied if src is sensitive too) 3. dst present → FileWrite(dst) (pure copy or move-without-src; WRITE risk is honest here) CodeRabbit findings on the same commit: * `McpToolBridge.create` now Objects.requireNonNull-guards serverName, mcpTool, client so a null param fails fast instead of producing "mcp__null__..." or NPE-ing later. Three new tests pin each guard. * Verb regexes now include the short-form suffixes documented in the javadoc but missing from the patterns: WRITE_VERB: adds _put DELETE_VERB: adds _rm MOVE_DEST_VERB: adds _mv, _cp COPY_VERB: adds _cp So blob_put / blob_rm / blob_mv / blob_cp classify like the prefix forms do (4 new tests). 35 tests pass in McpToolBridgeTest (was 32). Full ./gradlew build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 7 in Codex P2 — preserve DANGEROUS risk on MCP moves: ✅ fixed. Benign moves now emit CodeRabbit — CodeRabbit / self-review — verb regex short-form suffixes: ✅ fixed. The patterns were asymmetric — prefix included 35 tests passing in Disambiguation order in the move branch is now:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dedaa25ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * the first non-null textual match is extracted. | ||
| */ | ||
| private static final List<String> PATH_FIELDS = List.of( | ||
| "path", "file_path", "filepath", "filename", "file"); |
There was a problem hiding this comment.
When an MCP filesystem server uses the common camelCase schema shape, e.g. write_file({"filePath":"/repo/.env"}), this list does not match the target path, so inferFileCapability falls back to McpInvoke instead of FileWrite. In auto-accept mode, or after a remembered approval for the same MCP method on a safe path, the sensitive-path structural denial is therefore skipped even though the method is a file write; include camelCase variants or normalize field names before matching.
Useful? React with 👍 / 👎.
…ostic Codex P2 on #495: MCP filesystem servers in the wild use mixed key conventions - the official server uses snake_case (file_path, new_path), many third-party servers use camelCase (filePath, newPath). The previous snake_case-only extractField missed camelCase variants, so write_file({"filePath": "/repo/.env"}) fell through as McpInvoke and the structural sensitive-path denial was skipped. Normalize field names (lowercase + strip underscores) before matching: filePath, file_path, FilePath, FILEPATH all map to "filepath" against the canonical entry in PATH_FIELDS. Single pass over args builds a normalized -> original map; lookup is then O(1) per key in the canonical list. Locale.ROOT for stable case-folding. Coverage extends to all three lists (PATH_FIELDS, DESTINATION_FIELDS, SOURCE_FIELDS) since they all flow through extractField. Three new tests: camelCase path on write, camelCase destination (newPath) on move with sensitive value, uppercase FILEPATH. 38 tests pass in McpToolBridgeTest (was 35). Full build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 10 in Codex P1 — sub-agent path bypasses structural denial: ✅ fixed. The flow was:
A parent session blanket-approving Fix routed through 3 layers:
Two new regression tests:
All 3 existing callers updated to use the new constructor. 8 tests pass in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c771e7ff78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| private Capability inferFileCapability(JsonNode args) { | ||
| if (args == null || args.isNull() || !args.isObject()) return null; | ||
| String name = mcpToolName.toLowerCase(Locale.ROOT); |
There was a problem hiding this comment.
When an MCP filesystem server uses camelCase or kebab-case tool names such as writeFile, deleteFile, or move-file, this lowercases the method but leaves word boundaries/separators untouched, while the verb regexes only recognize bare or underscore-delimited names. The call then falls back to McpInvoke, so in auto-accept or after a remembered approval for that MCP method, writeFile({"filePath":"/repo/.env"}) never reaches the FileWrite/FileDelete structural denial. Normalize method names or match camel/kebab separators before applying these patterns.
Useful? React with 👍 / 👎.
…classify Codex P1 on #495 (round 11): MCP servers using camelCase tool names (writeFile, deleteFile) or kebab-case (write-file) bypassed the snake_case-only verb regexes. The current lowercase-only normalization left "writeFile" as "writefile" which doesn't match `^write(_.*)?$`, so the call fell back to McpInvoke and the structural denial layer was skipped. New normalizeMethodName(s): 1. Replace `-` and `.` with `_` (kebab and dotted forms). 2. Insert `_` between lowercase-or-digit followed by uppercase (camelCase split — also folds PascalCase since the first capital has no lowercase prev). 3. Lowercase under Locale.ROOT. Trace: writeFile → write_File → write_file. write-file → write_file. WriteFile → Write_File → write_file. write_file (already snake) → unchanged. 5 new test cases: camelCase write, kebab-case write, camelCase delete, camelCase move, PascalCase write. 43 tests pass in McpToolBridgeTest (was 38). Full build green. This commit closes the round of Codex-driven bypass discoveries on the MCP capability inference. Remaining edge cases (nested args, unusual field naming, multi-arg ops with non-standard verbs) intentionally fall through to McpInvoke and get the standard MCP prompt — the structural denial layer covers the common attack surface (write/delete/move/copy with path/destination/source fields in snake/camel/kebab forms) and McpInvoke + per-tool approval covers the rest. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 11 in This commit closes the Codex-driven loop on MCP capability inference. Each of the 11 rounds addressed a real bypass Codex flagged adjacent to the previous fix; the surface is now covered for the common attack patterns (write/delete/move/copy verbs in snake/camel/kebab/Pascal forms, with Known remaining limitations — left for follow-up to keep this PR scoped:
Final test count across changed modules:
Full If anything else surfaces I'll address it in a follow-up PR — this one is at a natural stopping point. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7e8b2b145
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static final List<String> DESTINATION_FIELDS = List.of( | ||
| "destination", "dest", "target", "to", "new_path", "output_path", "output"); |
There was a problem hiding this comment.
This whitelist still misses common two-path schema names such as destinationPath/destination_path (and similarly sourcePath/source_path): normalizeFieldName turns those into destinationpath, which does not match destination. For an MCP filesystem server using copy_file({"source":"/tmp/a","destinationPath":"/repo/.env"}), inference falls back to McpInvoke instead of FileWrite(.env), so auto-accept or a remembered MCP approval bypasses the new sensitive-path structural denial.
Useful? React with 👍 / 👎.
…dings Round-12 follow-up to the post-review pass on #495. High-severity (own review): structural denials reached via the sub-agent path (PermissionManager.checkStructural) were invisible to the audit log — only the main dispatcher's check(...) path audited. Forensics on "what did sub-agents try and get refused?" was blind. PermissionManager.checkStructural now takes (Capability, Provenance, allowlistKey) and writes a v2 audit entry when the structural layer denies. The main dispatcher path is unaffected (it runs policy.evaluateStructural directly, not via this method). SubAgentStructuralCheck extended with sessionId so the daemon-side probe can build a Provenance and audit the denial under the originating session. The daemon lambda threads sessionId through Provenance.fromNullableSessionId and into PermissionManager.checkStructural. Medium (own review): stale javadoc on PermissionManager.check claimed "PolicyEngine will eventually consume the structured Capability; method bridges via PermissionRequest" — both clauses are now false. Replaced with a 3-step pipeline description (structural -> blanket -> policy). Cleanup nits: - PermissionManager.hasAnySessionApproval was a documented compat shim with zero production callers — it would re-introduce the cross-session leak it warned about if anyone added a new caller. Removed. Stale doc references in SubAgentPermissionChecker and ToolPermissionChecker scrubbed too. - SENSITIVE_FILENAMES extended: id_ecdsa, .npmrc, .pypirc, service-account.json (GCP keys, npm/PyPI tokens). - SENSITIVE_PATH_SEGMENTS extended: .kube, .docker (Kubernetes auth tokens, Docker registry credentials). Tests: - PermissionManagerAuditTest: 2 new cases pinning that checkStructural audits denials AND writes nothing when no rule applies. Suite now 9. - DefaultPermissionPolicyTest: 6 new cases for each new sensitive filename / path segment. Suite now 50. - SubAgentPermissionCheckerTest: 8 (lambda signature updated for 3-arg sessionId). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-13 follow-up to the independent re-review of f77a13f. L1 finding from re-review: PermissionManager.checkStructural documents null Provenance as "allowed, falls back to daemonInternal()", but the new test suite only exercised the null branch on the no-rule path (checkStructuralWritesNoAuditWhenNoRuleApplies). A future caller dropping Provenance would hit the null-fallback on the denial branch which had no test coverage. Added checkStructuralNullProvenanceFallsBackToDaemonInternal pinning: - Returns the denial (non-null) - Audit entry written with sessionId=null (daemonInternal carries no session) - Falls back to capability.allowlistKey() ("FileWrite") when allowlistKey is also null N1 cleanup: import dev.aceclaw.security.Provenance in AceClawDaemon instead of fully-qualified inline. Matches the style of neighbouring security imports. PermissionManagerAuditTest: 10 tests (was 9). Full build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Post-review follow-up: rounds 12+13 ( Round 12 (
Round 13 (
Final test counts:
Independent re-review of the combined diff: no high or medium findings, ready to merge. The remaining deferred items from the holistic review ( |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@aceclaw-core/src/main/java/dev/aceclaw/core/agent/SubAgentPermissionChecker.java`:
- Around line 66-75: The check(...) method lacks a null-guard for the toolName
parameter, which can cause downstream predicate failures when toolName is null;
add an explicit null check at the start of SubAgentPermissionChecker.check by
calling Objects.requireNonNull(toolName, "toolName") (or equivalent) before
invoking structuralCheck.denyReason(toolName, inputJson, sessionId) so callers
get a clear NPE message and downstream checks receive a non-null toolName.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 430-441: The current catch of broad Exception around
subAgentMapper.readTree(inputJson) and aware.toCapability(argsNode) (variables
inputJson, subAgentMapper, aware, cap) swallows unexpected errors and returns
null, letting requests bypass hard-denial logic; change the handler to only
catch expected parse/validation exceptions (e.g., JsonProcessingException or
your capability-validation exception) and treat other exceptions as fatal by
rethrowing them (or convert them into an explicit deny result) so the code never
"fails open" — do not return null for unexpected exceptions and ensure the block
around toCapability either catches only known parse/validation errors or
rethrows unexpected exceptions to enforce the structural hard-denial invariant.
In
`@aceclaw-security/src/main/java/dev/aceclaw/security/DefaultPermissionPolicy.java`:
- Around line 196-237: Update isSensitivePath to resolve symbolic links before
performing checks: call rawPath.toRealPath() at the start (use the
canonical/canonicalized Path for all subsequent logic that currently uses path =
rawPath.normalize()), and fall back to the normalized path only if toRealPath
throws an IOException; preserve all existing checks (fileName/nameLower,
SENSITIVE_FILENAMES, startsWith(".env"), segment iteration, ".git"/"config"
handling, and absolute /etc check) but operate on the resolved Path returned by
toRealPath so symlinked targets cannot bypass the denylist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: badb8b40-e64c-4e9e-a4ab-92f780210317
📒 Files selected for processing (13)
aceclaw-core/src/main/java/dev/aceclaw/core/agent/SubAgentPermissionChecker.javaaceclaw-core/src/main/java/dev/aceclaw/core/agent/SubAgentStructuralCheck.javaaceclaw-core/src/main/java/dev/aceclaw/core/agent/ToolPermissionChecker.javaaceclaw-core/src/test/java/dev/aceclaw/core/agent/SubAgentPermissionCheckerTest.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/BackgroundTaskIntegrationTest.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/SubAgentIntegrationTest.javaaceclaw-mcp/src/main/java/dev/aceclaw/mcp/McpToolBridge.javaaceclaw-mcp/src/test/java/dev/aceclaw/mcp/McpToolBridgeTest.javaaceclaw-security/src/main/java/dev/aceclaw/security/DefaultPermissionPolicy.javaaceclaw-security/src/main/java/dev/aceclaw/security/PermissionManager.javaaceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.javaaceclaw-security/src/test/java/dev/aceclaw/security/PermissionManagerAuditTest.java
✅ Files skipped from review due to trivial changes (1)
- aceclaw-core/src/main/java/dev/aceclaw/core/agent/ToolPermissionChecker.java
🚧 Files skipped from review as they are similar to previous changes (2)
- aceclaw-mcp/src/main/java/dev/aceclaw/mcp/McpToolBridge.java
- aceclaw-mcp/src/test/java/dev/aceclaw/mcp/McpToolBridgeTest.java
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36b86e3cf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (dst != null && DefaultPermissionPolicy.isSensitivePath(dst)) { | ||
| return new Capability.FileWrite(dst, WriteMode.OVERWRITE); | ||
| } | ||
| if (src != null && (isMove || DefaultPermissionPolicy.isSensitivePath(src))) { | ||
| return new Capability.FileDelete(src); |
There was a problem hiding this comment.
When an installation supplies a custom PermissionPolicy.evaluateStructural with a different sensitive-path set than DefaultPermissionPolicy, this adapter decides which side of a copy/move the policy is allowed to see using the default policy instead. For example, a policy that structurally denies copying from /org/secret/* will receive FileWrite(/tmp/copy) for copy_file(source="/org/secret/key", destination="/tmp/copy"), so its source-side hard denial never runs and accept-edits can auto-approve the write. The capability boundary should preserve both relevant paths or defer this disambiguation to the active policy rather than calling DefaultPermissionPolicy here.
Useful? React with 👍 / 👎.
McpToolBridge had grown to 408 lines, of which ~200 lines were
MCP-method-name pattern matching, JSON-field lookup, and disambiguation
logic for move/copy semantics. The class was simultaneously:
1. A `Tool` adapter for the MCP protocol (its actual job)
2. A method-name classifier (verb regexes + camelCase normalization)
3. A field-name lookup with case/underscore normalization
4. A capability builder with sensitivity-aware disambiguation
Pure structural refactor: pull (2)(3)(4) into a focused package-private
helper class. McpToolBridge.toCapability is now 3 lines and the bridge
is back to ~160 lines of "MCP adapter for Tool".
Before: McpToolBridge.java 408 lines
After: McpToolBridge.java 162 lines (-60%)
McpCapabilityInference 293 lines (new)
Behavior is identical. All 43 tests in McpToolBridgeTest pass
unchanged - the existing transitive coverage proves the integration.
Direct unit tests for McpCapabilityInference can land in a follow-up
if we want package-internal coverage too.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…leMove variant Closes the two 🟡 maintainability/observability concerns from the holistic review: 1. Cross-module coupling. McpCapabilityInference used to call DefaultPermissionPolicy.isSensitivePath directly, baking a specific policy implementation's rule set into the MCP adapter. Moved the rule set to a policy-independent utility class SensitivePaths. Both DefaultPermissionPolicy.evaluateStructural and McpCapabilityInference now call SensitivePaths.matches. aceclaw-mcp no longer imports DefaultPermissionPolicy. 2. Audit log misclassification. Move/copy ops emitted FileWrite(dst) or FileDelete(src) to coerce the structural-denial layer into checking the right side. Audit log showed @type=FileDelete for benign moves and copy-from-sensitive operations — confusing for operators querying by capability type. Added Capability.FileMove(source, destination, deletesSource): - Risk: DANGEROUS for moves (source is removed), WRITE for copies. - DataFlow: BOTH (move ingests source, egresses to destination). - DefaultPermissionPolicy.evaluateStructural has a new FileMove case that checks both endpoints in one pass: destination first (the direct write-to-sensitive attack), then source (move-removes- sensitive OR copy-reads-sensitive). - McpCapabilityInference.inferMoveCopy is now pure data classification (~12 lines, no policy probing). Both src and dst → FileMove; one side missing → graceful degradation to FileWrite or FileDelete. Audit log now accurately records @type=FileMove for move/copy operations. Operators querying "all delete events" no longer get false positives from copy ops. Tests: - New SensitivePathsTest: 29 cases directly pinning the rule set (basenames, segments, .git/config, /etc/, case-insensitivity, traversal safety, false-positive guards). - CapabilityTest exhaustiveness sentinel updated for FileMove. - McpToolBridgeTest move/copy block rewritten: 11 new/replaced cases covering FileMove emission, degraded paths (src-only/dst-only), risk classification (DANGEROUS vs WRITE), and end-to-end structural denial via both src and dst sensitivity. All 127+ tests pass across changed modules. Full ./gradlew build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 16 in 🟡 → ✅ Cross-module coupling: Sensitive-path rules now live in 🟡 → ✅ Audit log @type misclassification: New
Risk classification:
127+ tests pass across affected modules. New Independent re-review verdict: LGTM, ship it. The PR is now at a much cleaner architectural baseline than where it was before this round: every module has a single responsibility, the audit log is honest, and the rule set is policy-independent. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3834ce9ab6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Path dst = safePath(extractField(args, DESTINATION_FIELDS)); | ||
| Path src = safePath(extractField(args, SOURCE_FIELDS)); |
There was a problem hiding this comment.
For move/rename MCP schemas that use path for the source and new_path/destination for the target, this only searches SOURCE_FIELDS, so path is dropped and the call degrades to FileWrite(dst). In accept-edits/auto-accept or with a remembered MCP approval, rename({"path":"/repo/.env","new_path":"/tmp/env.bak"}) then checks only the safe destination and can remove/read a sensitive source without hitting the structural denial; include path/file_path as source aliases for two-arg move/rename operations before falling back to destination-only.
Useful? React with 👍 / 👎.
Windows CI surfaced 5 failures in SensitivePaths / DefaultPermissionPolicy
tests around the /etc/ absolute-prefix rule:
writingUnderEtcIsStructurallyDenied
pathTraversalBypassIsBlocked
uppercaseEtcPrefixIsStructurallyDenied
etcHostsMatches
pathTraversalToEtcIsNormalized
Root cause: Path.of("/etc/hosts").isAbsolute() returns false on Windows
because Windows path semantics require a drive letter or UNC for
absoluteness. The previous check guarded the /etc/ match behind
isAbsolute(), so the rule silently never fired on Windows.
The agent's INTENT is what matters here: "write /etc/hosts" is a
system-config write regardless of host OS. Replaced the
isAbsolute()+getName(0) check with an OS-independent string prefix
check on the normalized path (render with forward slashes, lowercase,
then check for "/etc/" prefix or exact "/etc" match).
Trace:
Linux Path.of("/etc/hosts").toString() = "/etc/hosts" -> matches
Windows Path.of("/etc/hosts").toString() = "\etc\hosts" -> matches
(after \-to-/ replace)
Path.of("/tmp/../etc/hosts").normalize() = "/etc/hosts" -> matches
Path.of("docs/etc/notes.md").toString() = "docs/etc/notes.md"
-> does NOT match (no leading /)
All existing tests pass; the Windows CI failures will resolve.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
macOS CI surfaced this test failing on PR #495 (unrelated to the PR's actual changes — this is in aceclaw-core which the branch didn't touch). Root cause: the test sets a 100ms wall-time budget, then does Thread.sleep(200) and asserts isExhausted(). The 100ms buffer between timer deadline and assertion relies on the SCHEDULER thread getting CPU within 100ms of the timer's deadline. On a busy CI runner that's not guaranteed — macOS GitHub Actions runners are particularly prone to ~200ms scheduling delays. The test's intent is "verify the scheduler-based timer fires on its own" (not via belt-and-suspenders checkBudget). The polling pattern preserves that intent while tolerating slow CI scheduling. Same pattern as the existing timeBudget_beltAndSuspendersCheck but without explicitly invoking checkBudget. 2s deadline is generous: typical fire time is <150ms even on slow runners. Other tests in this file have similar tight buffers (timeBudget_*, resetWallClock_cancelsOldAndStartsNew, hardWallTimer_cancelsToken, softWallTimer_setsFlag). They didn't fail this run but use the same pattern; if they go flaky later, apply the same polling fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eba2232bc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * normalize-before-match, segment-level (not substring) comparisons — | ||
| * see class-level javadoc for the full rule set. | ||
| */ | ||
| public static boolean matches(Path rawPath) { |
There was a problem hiding this comment.
When the requested path is an existing symlink or hard link with a safe-looking name, this lexical normalize() check never sees the sensitive target, while the built-in write path ultimately uses Files.writeString(...) and follows symlinks by default. For example, in accept-edits/auto-accept, writing /repo/tmp_link where tmp_link -> /repo/.env is approved because SensitivePaths.matches(/repo/tmp_link) is false, but the write still overwrites .env; resolve existing paths (falling back to lexical checks for new files) before applying the sensitive-path rule.
Useful? React with 👍 / 👎.
Three reviewer findings on the unified-policy-engine PR:
- Codex P1 (McpCapabilityInference): rename({path,new_path}) ignored
`path` as a source candidate. With SOURCE_FIELDS missing, src=null
and the call degraded to FileWrite(dst), so a sensitive source like
/repo/.env never reached evaluateStructural. Fall back to PATH_FIELDS
on the source side when SOURCE_FIELDS produces nothing -- explicit
source/src/from still wins when both are present. +2 regression
tests in McpToolBridgeTest.
- CodeRabbit minor (SubAgentPermissionChecker.check): toolName was
forwarded downstream without a null-guard, violating the project's
"Objects.requireNonNull on params passed downstream" rule.
- CodeRabbit major (AceClawDaemon structuralCheck): the lambda caught
bare Exception and returned null, letting any RuntimeException from
toCapability silently bypass the structural layer the PR promises
overrides every approval. Split: JsonProcessingException (LLMs emit
bad JSON occasionally) falls through to the standard gate; any
other RuntimeException is a bug in inference code and now fails
closed with a logged denial reason naming the tool. Extracted the
probe into package-private static subAgentStructuralProbe so the
exception contract is unit-testable; +7 tests in
SubAgentStructuralProbeTest covering happy / denial / malformed-
JSON-fallthrough / runtime-fail-closed / empty-input / null-cap /
policy-must-be-reached paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
loadMigratesLegacyV1CandidateAndRewritesFile used fixed timestamps (2026-02-20..22) that silently expired on 2026-05-22 when they crossed CandidateStore.DEFAULT_RETENTION (90 days). The load-time maintenance pass started purging the legacy entry before the assertion ran, so the test surfaced as a migration regression even though the migration was working correctly. Switch to clock-relative dates (now - 1d, now - 3d) so the test stays inside the retention window regardless of when it runs. Same drive-by pattern as 1f2b4d7's watchdog fix on this branch; unrelated to the unified-policy-engine work but was blocking local full-suite runs from going green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29b7a48010
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (String key : keys) { | ||
| String original = argKeysByNormalized.get(normalizeFieldName(key)); | ||
| if (original != null) { | ||
| var node = args.get(original); | ||
| if (node != null && node.isTextual()) { |
There was a problem hiding this comment.
extractField returns the first matching alias from keys and never checks whether other equivalent aliases in the same payload carry a different path. For MCP servers that only honor one canonical field and ignore extras, an input like {"path":"/tmp/safe","file_path":"/repo/.env"} can make inference classify the safe value while execution uses the sensitive one, which defeats the structural sensitive-path denial in auto-accept or after remembered approval. Please detect conflicting alias values and deny/fallback instead of first-match selection.
Useful? React with 👍 / 👎.
Closes CodeRabbit major / Codex P2 on #495 -- the previous lexical matcher could be bypassed via a symlink whose alias name doesn't trip any rule but whose target does: ~/.ssh -> symlink at /tmp/safe-dir agent writes /tmp/safe-dir/id_rsa -> lexical match on /tmp/safe-dir/id_rsa misses (.ssh not in path) -> Files.writeString follows the symlink and lands at ~/.ssh/id_rsa SensitivePaths.matches() now runs the lexical rules first (cheap, catches the common cases) and on a miss canonicalises through Path.toRealPath() and re-runs. For new-file writes the target doesn't exist yet -- the resolver walks up to the deepest existing ancestor, takes its real path, then re-appends the missing tail. So: /tmp/safe-dir/new_key -> resolves parent -> ~/.ssh/new_key -> .ssh segment match -> deny /tmp/safe-dir (symlink to safe) -> resolves to safe target -> allow Lexical-first matters: /etc/hosts canonicalises to /private/etc/hosts on macOS, which doesn't lexically match the /etc/ prefix rule. The first pass catches the original path before resolution moves it. Adds matchesLexical() as a public IO-free entry point for callers that need the rule set without filesystem side effects (audit dry- runs, symbolic analysis). +7 tests using real symlinks under @tempdir, gracefully assumeTrue-skipped on filesystems that refuse symlink creation (Windows without dev-mode, locked-down sandboxes). Still best-effort, not airtight: TOCTOU window between the permission check and the actual write can swap a symlink underneath. True enforcement remains the still-pending sandbox layer in runtime-governance.md. This closes the easy bypasses available to a malicious or buggy MCP server. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e4a1c2fb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static String normalizeFieldName(String s) { | ||
| return s.toLowerCase(Locale.ROOT).replace("_", ""); |
There was a problem hiding this comment.
McpCapabilityInference only strips underscores in normalizeFieldName, so common JSON-schema keys like file-path, source-path, or destination-path do not match any PATH_FIELDS/SOURCE_FIELDS/DESTINATION_FIELDS aliases. In that case infer(...) falls back to McpInvoke, which means sensitive-path structural denial is skipped in auto-accept mode or after remembered MCP approval (for example, write-file({"file-path":"/repo/.env"})).
Useful? React with 👍 / 👎.
| if (SENSITIVE_PATH_SEGMENTS.contains(seg)) return true; | ||
| if (".git".equals(seg)) sawDotGit = true; | ||
| } | ||
| if (sawDotGit && "config".equals(nameLower)) return true; |
There was a problem hiding this comment.
The .git/config check currently triggers whenever any path contains a .git segment and the basename is config, so paths like /repo/.git/hooks/config or /repo/.git/worktrees/x/config are denied even though the class comment says only .git/config should be sensitive and the rest of .git/ should remain writable. This creates unnecessary hard denials for legitimate git/plugin workflows.
Useful? React with 👍 / 👎.
…rface Closes follow-up #497 (CodeRabbit major on #495). Every built-in tool in :aceclaw-tools (WriteFileTool, EditFileTool, BashExecTool, GrepSearchTool, GlobSearchTool, BrowserTool, AppleScriptTool, WebSearchTool, etc.) implements CapabilityAware and returns dev.aceclaw.security.Capability from toCapability(). That makes :aceclaw-security part of this module's public Java API surface; a downstream module reading those signatures needs Capability on its classpath transitively. implementation() works at runtime (the JAR is on the modulepath) but breaks compile against the tools API -- callers can't even reference the return type without redeclaring the dep. api() removes that silent foot-gun and matches the scope already used by :aceclaw-mcp since the McpToolBridge migration. No code changes; no observable runtime behavior change. Verified :aceclaw-tools, :aceclaw-daemon, :aceclaw-cli build clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This reverts commit 5e4a1c2. On second look this fix doesn't pay for the contract change it introduces. Threat model on review: - LLM-generated paths are literal -- the agent doesn't construct symlink bypasses on its own. - Malicious MCP server is the only realistic adversary, and trust to MCP servers is given at install time, not at call time. If you installed a malicious MCP server you have bigger problems than symlink shenanigans. - A workspace-resident attacker who can plant symlinks can also write the sensitive file directly -- the symlink trick isn't the shortest path. And even against those: the fix has a TOCTOU window I documented in the javadoc -- the symlink can be re-pointed between the check and the write, so it's not airtight. True enforcement needs the OS-level sandbox (Seatbelt/bubblewrap) that runtime-governance.md still marks as 🚧. Application-layer symlink resolution is a defense-in-depth placebo until that layer lands. Cost of carrying the fix: - Turns SensitivePaths.matches() from a pure function into one that does FS IO, breaking callers who relied on the cheap/ deterministic contract (hot paths, non-blocking contexts, tests that used synthetic paths). - ~90 lines of code + 135 lines of test for ~5% of the actual security gain. - When the sandbox layer eventually lands, it'll re-do this classification at the kernel-syscall level and we'll want the application-layer version gone. Codex P2 / CodeRabbit major flagged "no symlink resolution" by pattern, not by threat model. Accepting their finding here was a mistake -- the original known-limitation in the PR description was the right call. Restoring it in the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…toggle The structural hard-denial layer added in #480 was unconditionally active and would break any workflow that legitimately writes .env templates, .git/config entries, sources from /etc/, etc. Make it opt-in instead -- default off preserves pre-#480 behaviour so upgrades don't silently change observable agent behaviour. The layer is now controlled by `security.denySensitivePaths` in ~/.aceclaw/config.json (or {project}/.aceclaw/config.json): // Default (absent or false): no structural denials, policy is // mode-only -- a write to .env follows the standard prompt flow // and the user can approve. {} // Opt-in: hard-deny writes/deletes against .env*, .ssh/, .aws/, // .gnupg/, .kube/, .docker/, .git/config, credentials.json, // id_rsa, /etc/*, etc. Overrides every mode (including auto-accept) // and every prior approval (session blanket, sub-agent dispatch). { "security": { "denySensitivePaths": true } } DefaultPermissionPolicy gets a new (String mode, boolean denySensitivePaths) constructor; existing constructors default the flag to false for source-compatibility. When the flag is off, evaluateStructural() short-circuits to null and the policy is pure mode-based. Tests that pin the structural rules now construct the policy with the flag explicitly true (the rules themselves are unchanged; the test setup just opts in). +4 new policy tests covering the toggle itself (default off, both constructor variants stay off, explicit true enables). +3 config-loading tests pinning the JSON shape (empty/absent -> off, security.denySensitivePaths=true -> on, explicit false -> off). Daemon logs a one-line INFO when the flag is on so operators can confirm the layer is engaged in production logs. The toggle is intentionally single-knob, not per-rule. Users who need more granularity (custom patterns, per-rule disable) can ask in a follow-up -- 99% of operators want either "default" or "all on", not "deny .env but allow .ssh/". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Closes the two gaps
docs/runtime-governance.mdflagged as 🚧:McpToolBridgemigrated toCapabilityAware. Every active adapter (Bash, File, Browser, Skill, HTTP, OsScript, Memory, SubAgentSpawn, ScreenCapture, MCP) now enters the structured permission pipeline.PermissionPolicy.evaluatetakes(Capability, Provenance, description)instead of a flatPermissionRequest. Policies pattern-match on the sealed variant and can reachFileWrite.path,HttpFetch.url,McpInvoke.server, etc.Adds:
security.denySensitivePathsin~/.aceclaw/config.json). When enabled, overrides every mode (includingauto-accept) and every approval (session blanket, sub-agent dispatch) for writes/deletes targeting credentials and operator-critical paths:.env*,.ssh/,.aws/,.gnupg/,.kube/,.docker/,credentials.json,id_rsa/id_ed25519/id_ecdsa,.netrc,.npmrc,.pypirc,service-account.json,.git/config,/etc/*. Paths are normalized and case-folded underLocale.ROOTso/tmp/../etc/hostsand/repo/.ENVcannot bypass. Default-off is deliberate — an upgrade past feat(security): unified Capability sealed type + provenance chain (#465 Layer 1) #480 must not silently start refusing writes that prior agent flows depended on.SensitivePathsutility — policy-independent rule set. BothDefaultPermissionPolicy.evaluateStructuralandMcpCapabilityInferencecall it so MCP-side classification isn't coupled to a specific policy implementation.Capability.FileMove(source, destination, deletesSource)variant — carries both endpoints of a move/copy soevaluateStructuralcan check both sides in one pass. Audit log now records@type=FileMovefor actual moves/copies (previously misclassified asFileDeleteby the inference-side coercion).FileWrite/FileDelete/FileMoveso the structural rules fire uniformly across built-in tools and MCP servers.write_file" approval cannot route a sub-agent past the hard-denial layer. Denials are audited under the originating session's provenance.docs/runtime-governance.mdstatus table flipped: both rows now ✅ shipped.Test plan
./gradlew build— all 13 modules pass./gradlew :aceclaw-security:test— ~150 tests includingDefaultPermissionPolicyTest(50),SensitivePathsTest(29),PermissionManagerAuditTest(10),CapabilityTest(29)./gradlew :aceclaw-mcp:test— 44 tests including the full MCP capability inference surface./gradlew :aceclaw-daemon:test— all integration tests pass with updatedToolPermissionRouterTest./gradlew :aceclaw-core:test—SubAgentPermissionCheckerTest(8) covers the structural-denial-overrides-allowlist invariantKnown limitations
These are intentional and tracked as follow-ups, not bugs:
write_*,move_file,copyFile, …) and JSON args are looked up by canonical field names (path,destination, …). False positives possible — e.g.write_log(path=...)would be classified asFileWriteeven if it's a logging API. False negatives possible — an obscurely named file op that doesn't match the patterns falls back toMcpInvokeand gets the standard prompt. The standard prompt is also the floor protection, so false negatives are never bypasses. Follow-up Schema-aware MCP capability inference (reduce regex false positives) #496 tracks moving to schema-aware classification./repo/tmp_link → ~/.ssh/config) could route a write through. Considered and explicitly rejected at the application layer because (a) LLM-generated paths are literal, not symlink-bypass-shaped; (b) MCP trust is given at install time, not call time; (c) any application-layer resolver has a TOCTOU window that defeats it against an actual adversary. True enforcement needs the OS-level sandbox (Seatbelt/bubblewrap, still 🚧 inruntime-governance.md), which closes this at the syscall boundary without TOCTOU./private/etc/...(canonical resolution of/etc/) is not in the/etc/rule's match set. Agents normally use the/etc/form directly.params.pathoroptions.pathwon't have inference fire. Top-level keys cover the common case.Follow-up issues
🤖 Generated with Claude Code