fix(security): audit sub-agent structural denials; tighten review fin…

…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>

aceclaw-core/src/main/java/dev/aceclaw/core/agent/SubAgentPermissionChecker.java

@@ -16,12 +16,11 @@
  *
  * <h3>Per-session scope (#457)</h3>
  *
- * Pre-#457 this checker used a daemon-wide
- * {@code hasAnySessionApproval(toolName)} lookup that returned true if any
- * session had approved the tool — meaning a sub-agent in workspace B would
- * silently inherit a "remember" decision the user made in workspace A.
- * Now keyed on {@code (sessionId, toolName)} so each session's allow-list
- * stays in its own scope.
+ * Pre-#457 this checker used a daemon-wide allow-list lookup that returned
+ * true if any session had approved the tool — meaning a sub-agent in
+ * workspace B would silently inherit a "remember" decision the user made
+ * in workspace A. Now keyed on {@code (sessionId, toolName)} so each
+ * session's allow-list stays in its own scope.
  */
 public final class SubAgentPermissionChecker implements ToolPermissionChecker {
 
@@ -69,8 +68,10 @@ public ToolPermissionResult check(String toolName, String inputJson, String sess
         // session-blanket approval for the tool name (e.g. "always allow
         // write_file") MUST NOT let a sub-agent target .env / .ssh /
         // /etc/ etc. — the "overrides every approval" invariant of the
-        // hard-denial layer (Codex P1 on #495).
-        String structuralReason = structuralCheck.denyReason(toolName, inputJson);
+        // hard-denial layer (Codex P1 on #495). The sessionId is forwarded
+        // so the daemon-side probe can attribute the audit entry to the
+        // originating session, matching the main-dispatcher's audit shape.
+        String structuralReason = structuralCheck.denyReason(toolName, inputJson, sessionId);
         if (structuralReason != null) {
             return ToolPermissionResult.denied(structuralReason);
         }

aceclaw-core/src/main/java/dev/aceclaw/core/agent/SubAgentStructuralCheck.java

@@ -25,16 +25,23 @@ public interface SubAgentStructuralCheck {
 
     /**
      * Returns a non-null denial reason when {@code (toolName, inputJson)}
-     * matches a structural denial rule. Returns {@code null} when no
-     * structural rule applies — the caller then defers to the normal
-     * read-only / session-approval logic.
+     * matches a structural denial rule, given the calling sub-agent's
+     * {@code sessionId}. Returns {@code null} when no structural rule
+     * applies — the caller then defers to the normal read-only /
+     * session-approval logic.
+     *
+     * <p>{@code sessionId} is forwarded so the daemon-side implementation
+     * can build a {@code Provenance} and write an audit entry tagged with
+     * the originating session, matching the main-dispatcher's audit
+     * shape. {@code null} is allowed for daemon-internal callers that have
+     * no session in scope.
      *
      * <p>Implementations must not throw on malformed input — they should
      * return {@code null} for unparseable args so the standard fall-through
      * logic decides, rather than crash the dispatcher.
      */
-    String denyReason(String toolName, String inputJson);
+    String denyReason(String toolName, String inputJson, String sessionId);
 
     /** No-op probe — returns {@code null} for every input. */
-    SubAgentStructuralCheck NONE = (toolName, inputJson) -> null;
+    SubAgentStructuralCheck NONE = (toolName, inputJson, sessionId) -> null;
 }

aceclaw-core/src/main/java/dev/aceclaw/core/agent/ToolPermissionChecker.java

@@ -12,11 +12,11 @@
  *
  * The 3-arg {@link #check(String, String, String)} entry point carries the
  * owning session id so a per-session allow-list cannot leak across sessions.
- * Pre-#457 the sub-agent path used a daemon-wide {@code hasAnySessionApproval}
- * lookup that returned true if <em>any</em> session had approved the tool —
- * meaning a sub-agent in workspace B would inherit a "remember" approval
- * granted in workspace A. The 3-arg form lets the daemon wire a per-session
- * predicate so that leak is closed.
+ * Pre-#457 the sub-agent path used a daemon-wide allow-list lookup that
+ * returned true if <em>any</em> session had approved the tool — meaning a
+ * sub-agent in workspace B would inherit a "remember" approval granted in
+ * workspace A. The 3-arg form lets the daemon wire a per-session predicate
+ * so that leak is closed.
  *
  * <p>The legacy 2-arg form is preserved as a deprecated default that
  * delegates with a {@code null} sessionId — implementations should override

aceclaw-core/src/test/java/dev/aceclaw/core/agent/SubAgentPermissionCheckerTest.java

@@ -121,7 +121,7 @@ void structuralDenialOverridesSessionApproval() {
         // must fire BEFORE the allow-list shortcut so the hard-denial
         // invariant holds for sub-agent dispatch too.
         BiPredicate<String, String> approveEverything = (sid, tool) -> true;
-        SubAgentStructuralCheck refuseEnvWrites = (toolName, inputJson) -> {
+        SubAgentStructuralCheck refuseEnvWrites = (toolName, inputJson, sid) -> {
             if ("write_file".equals(toolName) && inputJson != null && inputJson.contains(".env")) {
                 return "Refusing to write sensitive .env";
             }
@@ -151,7 +151,7 @@ void structuralDenialOverridesReadOnlyAllowlist() {
         // since structural rules only cover writes/deletes), they must
         // still take precedence.
         BiPredicate<String, String> denyAll = (sid, tool) -> false;
-        SubAgentStructuralCheck refuseReadOnly = (toolName, inputJson) -> "blocked";
+        SubAgentStructuralCheck refuseReadOnly = (toolName, inputJson, sid) -> "blocked";
         var checker = new SubAgentPermissionChecker(
                 READ_ONLY, denyAll, refuseReadOnly);
 

aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java

@@ -413,9 +413,16 @@ private void wireAgentHandler(Path workingDir) {
         // shortcut, so a prior "always allow write_file" approval cannot
         // route a sub-agent past the .env / .ssh / /etc/ hard-denial layer
         // (Codex P1 on #495).
+        //
+        // Audit attribution: the denial is recorded via
+        // permissionManager.checkStructural which writes a v2 audit entry
+        // tagged with the originating session's Provenance and the tool
+        // name as the allowlistKey. Without that audit, a sub-agent's
+        // attempt to write .env would be invisible to forensics — the
+        // dispatcher main path's audit hook is bypassed here.
         var subAgentToolRegistry = toolRegistry;
         var subAgentMapper = objectMapper;
-        SubAgentStructuralCheck structuralCheck = (toolName, inputJson) -> {
+        SubAgentStructuralCheck structuralCheck = (toolName, inputJson, sessionId) -> {
             var toolOpt = subAgentToolRegistry.get(toolName);
             if (toolOpt.isEmpty()) return null;
             if (!(toolOpt.get() instanceof CapabilityAware aware)) return null;
@@ -433,7 +440,8 @@ private void wireAgentHandler(Path workingDir) {
                 return null;
             }
             if (cap == null) return null;
-            var denial = permissionManager.checkStructural(cap);
+            var provenance = dev.aceclaw.security.Provenance.fromNullableSessionId(sessionId);
+            var denial = permissionManager.checkStructural(cap, provenance, toolName);
             return denial == null ? null : denial.reason();
         };
 

aceclaw-security/src/main/java/dev/aceclaw/security/DefaultPermissionPolicy.java

@@ -50,7 +50,11 @@ public final class DefaultPermissionPolicy implements PermissionPolicy {
             "credentials.json",
             ".netrc",
             "id_rsa",
-            "id_ed25519");
+            "id_ed25519",
+            "id_ecdsa",
+            ".npmrc",
+            ".pypirc",
+            "service-account.json");
 
     /**
      * Path segments that, when present anywhere in the path, mark the file
@@ -63,7 +67,9 @@ public final class DefaultPermissionPolicy implements PermissionPolicy {
     private static final Set<String> SENSITIVE_PATH_SEGMENTS = Set.of(
             ".ssh",
             ".aws",
-            ".gnupg");
+            ".gnupg",
+            ".kube",
+            ".docker");
 
     private final String mode;
 

aceclaw-security/src/main/java/dev/aceclaw/security/PermissionManager.java

@@ -124,10 +124,21 @@ public PermissionDecision check(Capability capability, Provenance provenance) {
      *       tools — no UX regression during migration.</li>
      * </ul>
      *
-     * <p>Pipeline is unchanged: session allowlist first, then policy.
-     * PolicyEngine (#465 Scope #2) will eventually consume the structured
-     * {@link Capability} directly; until then this method bridges by
-     * constructing a {@link PermissionRequest} on the fly.
+     * <h4>Pipeline (#480 PR 4 / #495)</h4>
+     *
+     * <ol>
+     *   <li><b>Structural denial</b> ({@link PermissionPolicy#evaluateStructural}):
+     *       cross-cutting refusals like "never write to {@code .env}" run
+     *       FIRST so they cannot be bypassed by a prior "always allow X"
+     *       approval. Audited and returned immediately when matched.</li>
+     *   <li><b>Session blanket approval</b>: if the originating tool has been
+     *       blanket-approved in this session (per-session, #456), auto-approve
+     *       and audit with the {@code session-blanket-approval} marker.</li>
+     *   <li><b>Policy evaluation</b> ({@link PermissionPolicy#evaluate}):
+     *       the structured {@link Capability} + {@link Provenance} +
+     *       description go to the policy directly — no flat-record bridge.
+     *       Result audited.</li>
+     * </ol>
      */
     public PermissionDecision check(
             Capability capability,
@@ -255,14 +266,42 @@ private void audit(
      * applies; the caller then routes through whatever approval logic is
      * appropriate for its context.
      *
-     * <p>No audit record is written here — the caller is expected to either
-     * approve (and audit elsewhere) or deny (and audit the denial in its
-     * own surface). Audit duplication between this method and a follow-up
-     * {@link #check} call would inflate the on-disk log.
+     * <p>When the structural layer denies, an audit entry is written so
+     * sub-agent attempts to write {@code .env} / {@code .ssh/} / etc. are
+     * observable in the same on-disk record as main-dispatcher denials.
+     * Without this entry, a successful structural denial on the sub-agent
+     * path would be invisible to forensics — the caller (the sub-agent
+     * permission checker) can't audit because it doesn't depend on
+     * {@code aceclaw-security}'s audit types.
+     *
+     * <p>No double-audit risk on the main dispatcher path: that path runs
+     * {@code policy.evaluateStructural} directly inside
+     * {@link #check(Capability, Provenance, String, String)} (which has its
+     * own audit hook) and does NOT route through this method.
+     *
+     * @param capability  the structured capability to test
+     * @param provenance  audit context recorded when a denial is written.
+     *                    {@code null} is allowed for callers that have no
+     *                    session in scope; uses
+     *                    {@link Provenance#daemonInternal()} as a stand-in.
+     * @param allowlistKey audit context — the originating tool name, so
+     *                     {@code grep toolName=...} on the on-disk log
+     *                     keeps working uniformly across denial sources.
      */
-    public PermissionDecision.Denied checkStructural(Capability capability) {
+    public PermissionDecision.Denied checkStructural(
+            Capability capability,
+            Provenance provenance,
+            String allowlistKey) {
         Objects.requireNonNull(capability, "capability");
-        return policy.evaluateStructural(capability);
+        var denial = policy.evaluateStructural(capability);
+        if (denial != null) {
+            var prov = provenance != null ? provenance : Provenance.daemonInternal();
+            String key = allowlistKey != null ? allowlistKey : capability.allowlistKey();
+            log.debug("Permission denied (structural, out-of-band): key={}, reason={}",
+                    key, denial.reason());
+            audit(key, capability, prov, denial, null);
+        }
+        return denial;
     }
 
     /**
@@ -313,27 +352,4 @@ public boolean hasSessionApproval(String sessionId, String toolName) {
         return allow != null && allow.contains(toolName);
     }
 
-    /**
-     * Returns whether ANY session has blanket-approved the given tool.
-     *
-     * <p>Compatibility shim for the sub-agent permission checker: that
-     * checker is constructed once at daemon startup and doesn't have a
-     * session in scope at check time, so it can't ask the per-session
-     * variant. Until the sub-agent path threads sessionId through
-     * {@code ToolPermissionChecker} (separate refactor), this preserves
-     * the pre-#456 behaviour where a session-approved tool was reachable
-     * by sub-agents anywhere in the daemon.
-     *
-     * <p>This is permissive on purpose: a sub-agent in session B inherits
-     * approvals granted in session A. The right scoping is per-session,
-     * but losing the approval-flow entirely (always-deny) would regress
-     * usability. Tracked as a follow-up.
-     */
-    public boolean hasAnySessionApproval(String toolName) {
-        Objects.requireNonNull(toolName, "toolName");
-        for (var allow : sessionApprovals.values()) {
-            if (allow.contains(toolName)) return true;
-        }
-        return false;
-    }
 }

aceclaw-security/src/test/java/dev/aceclaw/security/DefaultPermissionPolicyTest.java

@@ -249,6 +249,43 @@ void deletingDotEnvIsStructurallyDenied() {
         assertNotNull(STRUCTURAL.evaluateStructural(delete("/repo/.env")));
     }
 
+    @Test
+    void writingKubeConfigIsStructurallyDenied() {
+        // .kube/config — Kubernetes auth tokens.
+        assertNotNull(STRUCTURAL.evaluateStructural(write("/home/u/.kube/config")));
+    }
+
+    @Test
+    void writingDockerConfigIsStructurallyDenied() {
+        // .docker/config.json — registry credentials.
+        assertNotNull(STRUCTURAL.evaluateStructural(write("/home/u/.docker/config.json")));
+    }
+
+    @Test
+    void writingNpmrcIsStructurallyDenied() {
+        // .npmrc — npm registry auth tokens.
+        assertNotNull(STRUCTURAL.evaluateStructural(write("/repo/.npmrc")));
+    }
+
+    @Test
+    void writingPypircIsStructurallyDenied() {
+        // .pypirc — PyPI upload credentials.
+        assertNotNull(STRUCTURAL.evaluateStructural(write("/home/u/.pypirc")));
+    }
+
+    @Test
+    void writingIdEcdsaIsStructurallyDenied() {
+        // SSH ECDSA private key (separate from RSA / ED25519).
+        assertNotNull(STRUCTURAL.evaluateStructural(write("/home/u/id_ecdsa")));
+    }
+
+    @Test
+    void writingServiceAccountJsonIsStructurallyDenied() {
+        // GCP service account keys are commonly named this; named here so
+        // the rule fires even outside the .ssh/.aws/.gnupg segments.
+        assertNotNull(STRUCTURAL.evaluateStructural(write("/tmp/service-account.json")));
+    }
+
     @Test
     void nonFileCapabilityIsNotStructurallyDenied() {
         // BashExec, HttpFetch, etc. fall through structural checks today —

aceclaw-security/src/test/java/dev/aceclaw/security/PermissionManagerAuditTest.java

@@ -123,6 +123,68 @@ void noAuditLogAttachedIsANoOp() {
                 .isInstanceOf(PermissionDecision.Approved.class);
     }
 
+    @Test
+    void checkStructuralWritesAuditOnDenial(@TempDir Path tmp) throws IOException {
+        // Round-12 follow-up review on #495: structural denials reached via
+        // the sub-agent path (PermissionManager.checkStructural) used to be
+        // 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. Now checkStructural writes its own entry.
+        dev.aceclaw.security.PermissionPolicy structuralDeny = new dev.aceclaw.security.PermissionPolicy() {
+            @Override public PermissionDecision evaluate(
+                    dev.aceclaw.security.Capability cap,
+                    dev.aceclaw.security.Provenance prov,
+                    String desc) {
+                return new PermissionDecision.Approved();
+            }
+            @Override public PermissionDecision.Denied evaluateStructural(
+                    dev.aceclaw.security.Capability cap) {
+                return new PermissionDecision.Denied("sensitive path");
+            }
+        };
+        var auditLog = CapabilityAuditLog.create(tmp);
+        var pm = new PermissionManager(structuralDeny, auditLog);
+
+        var cap = new dev.aceclaw.security.Capability.FileWrite(
+                java.nio.file.Path.of("/repo/.env"),
+                dev.aceclaw.security.WriteMode.OVERWRITE);
+        var prov = dev.aceclaw.security.Provenance.fromNullableSessionId("sess-X");
+
+        var result = pm.checkStructural(cap, prov, "write_file");
+
+        assertThat(result).isNotNull();
+        var entries = auditLog.readVerified();
+        assertThat(entries).hasSize(1);
+        var entry = entries.getFirst();
+        assertThat(entry.decisionKind()).isEqualTo("DENIED");
+        assertThat(entry.toolName()).isEqualTo("write_file");
+        assertThat(entry.sessionId()).isEqualTo("sess-X");
+        assertThat(entry.reason()).isEqualTo("sensitive path");
+    }
+
+    @Test
+    void checkStructuralWritesNoAuditWhenNoRuleApplies(@TempDir Path tmp) throws IOException {
+        // No double-cost: when no structural rule matches the capability,
+        // checkStructural returns null and writes no audit entry.
+        dev.aceclaw.security.PermissionPolicy noStructural = new dev.aceclaw.security.PermissionPolicy() {
+            @Override public PermissionDecision evaluate(
+                    dev.aceclaw.security.Capability cap,
+                    dev.aceclaw.security.Provenance prov,
+                    String desc) {
+                return new PermissionDecision.Approved();
+            }
+        };
+        var auditLog = CapabilityAuditLog.create(tmp);
+        var pm = new PermissionManager(noStructural, auditLog);
+
+        var cap = new dev.aceclaw.security.Capability.FileWrite(
+                java.nio.file.Path.of("/tmp/safe.txt"),
+                dev.aceclaw.security.WriteMode.OVERWRITE);
+
+        assertThat(pm.checkStructural(cap, null, "write_file")).isNull();
+        assertThat(auditLog.readVerified()).isEmpty();
+    }
+
     @Test
     void everyCheckCallProducesExactlyOneEntry(@TempDir Path tmp) throws IOException {
         var auditLog = CapabilityAuditLog.create(tmp);