feat(security): Provenance + CapabilityAware + WriteFileTool migration (#480 PR 2/3)#482
Merged
Merged
Conversation
#480 PR 2/3) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughStructured permission routing added: tools can implement CapabilityAware to convert JSON args into Capability; PermissionManager gains capability-based check overloads and Provenance types; ToolPermissionRouter centralizes routing with legacy fallbacks; WriteFileTool and daemon handler updated; tests added for capabilities, provenance, routing, and write-file conversion. ChangesStructured Permission & Provenance System
Sequence DiagramsequenceDiagram
autonumber
participant Agent as StreamingAgentHandler
participant Router as ToolPermissionRouter
participant Tool as WriteFileTool
participant PM as PermissionManager
participant Policy as PermissionPolicy
Agent->>Router: check(tool, inputJson, sessionId, desc, level, pm, mapper)
Router->>Tool: (if CapabilityAware) toCapability(json)
alt toCapability succeeds
Tool-->>Router: Capability
Router->>PM: check(capability, Provenance.fromNullableSessionId(sessionId), allowlistKey, description)
PM->>PM: sessionAllowlist lookup by allowlistKey
alt session approval exists
PM-->>Router: PermissionDecision.Approved
else
PM->>Policy: evaluate(PermissionRequest(allowlistKey, description, risk))
Policy-->>PM: PermissionDecision
PM-->>Router: PermissionDecision (and audit)
end
else toCapability throws
Router->>PM: checkLegacy(...) (construct PermissionRequest, pass sessionId)
PM->>Policy: evaluate(...)
Policy-->>PM: PermissionDecision
PM-->>Router: PermissionDecision
end
Router-->>Agent: PermissionDecision
Agent->>Tool: proceed or deny based on decision
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related Issues
Possibly Related PRs
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: d46d622501
ℹ️ 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".
Comment on lines
+3997
to
+4001
| var capability = capAware.toCapability(objectMapper.readTree(finalInputJson)); | ||
| var provenance = Provenance.legacy(sessionId); | ||
| decision = permissionManager.check( | ||
| capability, provenance, delegate.name(), toolDescription); | ||
| } catch (RuntimeException | java.io.IOException toCapErr) { |
There was a problem hiding this comment.
The try block currently wraps both toCapability(...) and permissionManager.check(...), so any RuntimeException from the structured permission path (for example, policy/audit/runtime errors) is treated as a conversion failure and silently retried through the legacy PermissionRequest path. In that scenario, a real failure in the new capability-based pipeline is masked and can be downgraded to a less strict decision flow for CapabilityAware tools instead of surfacing the error.
Useful? React with 👍 / 👎.
Codex review on #482: the try block previously wrapped permissionManager.check too, so a runtime error from the structured policy/audit path was silently retried via the legacy path — masking real bugs and potentially downgrading the decision. Narrow the catch to just args-to-Capability conversion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java`:
- Around line 3996-4021: The code currently treats a null return from
CapabilityAware.toCapability(...) the same as a parse/exception failure and
falls back to the legacy PermissionRequest path; to fix this, fail fast when
toCapability returns null (since it’s documented non-null) instead of
downgrading to legacy. Concretely, inside the try block where you call
capAware.toCapability(objectMapper.readTree(finalInputJson)) check the returned
capability and, if it is null, throw an IllegalStateException (including
delegate.name() and any context) so the error surfaces; keep the existing catch
for RuntimeException/IOException so real parse/convert exceptions still fall
back to the legacy permission path. Ensure you reference
CapabilityAware.toCapability, the local capability variable, and
permissionManager.check/PermissionRequest code paths when making the change.
In `@aceclaw-security/src/main/java/dev/aceclaw/security/PermissionManager.java`:
- Around line 105-106: The two-arg overload public PermissionDecision
check(Capability capability, Provenance provenance) dereferences capability
before validation; add the same null guards as the four-arg path by calling
Objects.requireNonNull(capability, "capability") and
Objects.requireNonNull(provenance, "provenance") (or equivalent null checks used
elsewhere) before invoking capability.allowlistKey() and
capability.displayLabel(), then forward to the existing check(capability,
provenance, ...) call; this ensures consistent NPE messages and adheres to the
guideline about using Objects.requireNonNull for parameters used in downstream
calls or equals().
In
`@aceclaw-security/src/test/java/dev/aceclaw/security/PermissionManagerCapabilityTest.java`:
- Around line 120-133: The test currently uses APPROVE which masks whether
Provenance.daemonInternal() bypasses session allowlist; change the
PermissionManager construction from APPROVE to DENY (use the DENY policy
constant) so the global policy would deny by default, keep the
pm.approveForSession("sess-X", "FileRead") call to simulate a session approval,
call pm.check(new Capability.FileRead(Path.of("/etc/hosts")),
Provenance.daemonInternal()) as before, and update the assertion to expect
PermissionDecision.Denied (e.g.,
assertThat(decision).isInstanceOf(PermissionDecision.Denied.class)) to prove
daemonInternal() skips the session allowlist.
In `@aceclaw-tools/src/main/java/dev/aceclaw/tools/WriteFileTool.java`:
- Around line 163-170: The toCapability method uses Files.exists(filePath) to
decide WriteMode, which treats indeterminate existence (e.g., permission errors)
as CREATE_NEW; update to check three states instead: compute filePath via
resolveFilePath(args.get("file_path").asText()), then if Files.exists(filePath)
use WriteMode.OVERWRITE, else if Files.notExists(filePath) use
WriteMode.CREATE_NEW, otherwise throw an IllegalArgumentException (or similar)
to reject indeterminate cases before constructing new
Capability.FileWrite(filePath, mode).
🪄 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: c7ad6d57-dd6b-4353-83c8-7565db52ecb5
📒 Files selected for processing (11)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.javaaceclaw-security/src/main/java/dev/aceclaw/security/Capability.javaaceclaw-security/src/main/java/dev/aceclaw/security/CapabilityAware.javaaceclaw-security/src/main/java/dev/aceclaw/security/PermissionManager.javaaceclaw-security/src/main/java/dev/aceclaw/security/Provenance.javaaceclaw-security/src/main/java/dev/aceclaw/security/ProvenanceLink.javaaceclaw-security/src/test/java/dev/aceclaw/security/PermissionManagerCapabilityTest.javaaceclaw-security/src/test/java/dev/aceclaw/security/ProvenanceLinkTest.javaaceclaw-security/src/test/java/dev/aceclaw/security/ProvenanceTest.javaaceclaw-tools/src/main/java/dev/aceclaw/tools/WriteFileTool.javaaceclaw-tools/src/test/java/dev/aceclaw/tools/WriteFileToolCapabilityTest.java
- StreamingAgentHandler: fail fast when toCapability() returns null (contract violation), only fall back to legacy on actual exception. - PermissionManager.check(Capability, Provenance): add Objects.requireNonNull guards matching the 4-arg path. - WriteFileTool.toCapability: reject indeterminate filesystem state (Files.exists==Files.notExists==false) instead of silently classifying as CREATE_NEW. - PermissionManagerCapabilityTest.daemonInternalProvenanceSkipsAllowlistLookup: switch policy to DENY so the assertion actually proves the bypass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bypass) Codex P1 from PR #481 review (deferred at the time): GET/HEAD requests still ship URL query strings, headers, and cookies — those are real exfiltration vectors. Classifying them as INGRESS-only let a 'block-egress' policy be bypassed by encoding payload in a GET querystring. Single answer (BOTH) for every method removes the whole class of bypass; finer-grained per-payload inspection belongs in PolicyEngine (#465 Scope #2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…onRouter Provenance.legacy → fromNullableSessionId. The previous name implied legacy-only callers, but the dispatcher uses it too (PR 3 will fill in the chain). The new name describes what callers actually have — a possibly-null raw String id — without diluting the migration grep signal. ToolPermissionRouter extracts the structured-vs-legacy permission branching from StreamingAgentHandler so the contract is testable without standing up the agent loop. New ToolPermissionRouterTest pins all six branches: structured path, legacy path, toCapability throw → fallback, toCapability null → fail-fast, policy errors propagate, null sessionId works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the conversionThrew flag + double-null-check with an early-return from the catch block plus a small checkLegacy helper. Same behavior, but each path now has a single return and the previously unreachable (conversionThrew && capability != null) branch disappears. 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: 9737bd9b33
ℹ️ 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".
| PermissionManager permissionManager, | ||
| ObjectMapper mapper) { | ||
| Objects.requireNonNull(delegate, "delegate"); | ||
| Objects.requireNonNull(inputJson, "inputJson"); |
There was a problem hiding this comment.
ToolPermissionRouter.check now hard-fails with Objects.requireNonNull(inputJson), but the surrounding execution path intentionally tolerates missing/invalid tool args (e.g., PermissionAwareTool.execute catches parse errors and continues, and ContentBlock.ToolUse can carry nullable input). With this guard, any tool call that arrives without an arguments payload throws an NPE before permission evaluation or legacy fallback, turning a previously recoverable flow into an unhandled failure. Please treat null args as an empty object (or route directly to legacy) instead of rejecting at the router boundary.
Useful? React with 👍 / 👎.
Codex P2 on #482: ContentBlock.ToolUse and PermissionAwareTool's parse path both legitimately deliver null/malformed args — the previous dispatcher absorbed those via the try-catch around mapper.readTree. My refactor added Objects.requireNonNull(inputJson) at the router boundary, which converted that recoverable flow into an NPE crash. Drop the guard, document the contract, add a test that null inputJson falls back to legacy without reaching toCapability. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second of three PRs for #480 (runtime governance Layer 1, foundation for #465).
CapabilityAwareinterface — opt-in companion toTool. A tool implements it to advertise structuredCapabilityvariants instead of the flat(toolName, level)shape.Provenance+ProvenanceLinkrecords — the "how did we get here" chain (root prompt, plan step, sub-agent depth, retry history). Optional fields are empty in PR 2; PR 3 wires them through the agent loop.PermissionManager.check(...)now has three entry points sharing one decision/audit pipeline:check(PermissionRequest, String)— legacy shim, unchanged callers see no behavior change.check(Capability, Provenance)— convenience overload for daemon-internal callers that have no originating tool name.check(Capability, Provenance, String allowlistKey, String description)— dispatcher form. Caller passes the originating tool's name as the allowlist key + the rich human description, so existing "always allow X" approvals survive migration and the user prompt stays as detailed as before.StreamingAgentHandlerdispatcher uses the 4-arg form when the tool isCapabilityAware; falls back to the legacyPermissionRequestpath on bad args (logged) or for non-migrated tools.WriteFileToolis the first tool migrated. ItstoCapability(...)checksFiles.existsto pickWriteMode.CREATE_NEWvsOVERWRITE, so a create-only policy can refuse overwrites without re-stat'ing the filesystem.Migration safety
write_file" approvals continue to auto-approve after the tool migrates — pinned byPermissionManagerCapabilityTest.sessionBlanketApprovalSurvivesToolMigrationViaDispatcherKey.hasAnySessionApproval(toolName)) keeps working because the dispatcher passes the tool name as the allowlist key.buildToolDescription(...)produces — pinned byfourArgPathPassesCallerSuppliedDescriptionToPolicy.Test plan
:aceclaw-security:test— newPermissionManagerCapabilityTest,ProvenanceTest,ProvenanceLinkTest:aceclaw-tools:test— newWriteFileToolCapabilityTest(CREATE_NEW vs OVERWRITE pair, blank-args rejection):aceclaw-daemon:test— existing dispatcher integration tests still passOut of scope (PR 3)
PromptId/PlanStepIdthrough the agent loop (Provenance optional fields stay empty in PR 2).Capability/Provenancepayloads.ReadFileTool,EditFileTool,BashExecTool,GlobSearchTool,GrepSearchTool— done piecemeal in follow-ups.Summary by CodeRabbit
New Features
Refactor
Tests