🔒 Aisle Security Analysis

✅ We scanned this PR and did not find any security vulnerabilities.

_{Aisle supplements but does not replace security review.}

Analyzed PR: #53565 at commit 6c168f5

_{Last updated on: 2026-04-25T20:51:21Z}

AI-Assisted PR Checklist

• Mark as AI-assisted in the PR title or description
• Note the degree of testing (untested / lightly tested / fully tested)
  • Fully tested: pnpm build, pnpm check, and pnpm test all pass. Two new regression tests added (sandbox tool-policy constants + pipeline filtering).
• Include prompts or session logs if possible (super helpful!)
  • Auto-maintain pipeline run with Claude + Codex committee analysis, fix, two-round post-fix review, and revision cycle. Codex caught the gateway over-exposure in round 1, leading to a narrower fix (cron-only).
• Confirm you understand what the code does
  • Moves cron from DEFAULT_TOOL_DENY to DEFAULT_TOOL_ALLOW in sandbox constants, since cron uses callGatewayTool (WebSocket RPC to host gateway) and never executes inside the Docker sandbox. Adds conditional system prompt hint gated on availableTools.has("cron"). Keeps gateway in DENY because it exposes admin actions (restart, config.apply, etc.).
• If you have access to Codex, run codex review --base origin/main locally and address the findings before asking for review
• Resolve or reply to bot review conversations after you address them

Greptile Summary

This PR correctly removes cron from DEFAULT_TOOL_DENY and adds it to DEFAULT_TOOL_ALLOW in the sandbox constants, fixing a bug where the cron tool was unavailable in sandbox.mode='all' sessions. Since the cron tool is dispatched via callGatewayTool WebSocket RPC and never executes inside the Docker container, denying it at the sandbox policy layer was incorrect and caused the model to hallucinate fake cron calls.

• constants.ts: Clean change; removes cron from the deny list and adds it to the allow list with an explanatory comment.
• system-prompt.ts: Adds a cron sandbox hint gated on availableTools.has("cron"), following the same pattern used for other capability hints in the sandbox section.
• tool-policy.test.ts: Adds two regression tests. The tests are functionally correct but share duplicate assertions — both compute the same resolved policy and both assert isToolAllowed(policy, "cron") === true. The second test's only unique assertion is the gateway check.

Note: The cron tool retains its ownerOnly protection via applyOwnerOnlyToolPolicy, so non-owner senders still cannot access cron even after this sandbox policy change.

Confidence Score: 5/5

• Safe to merge — the fix is correct, well-tested, and does not introduce regressions; the owner-only protection layer for cron remains intact.
• The core change (moving cron from deny to allow in the sandbox default policy) is logically sound and matches the documented behavior of the cron tool (gateway-routed, not container-executed). Tests cover the primary and regression cases. The only finding is a minor test duplication (P2 style), which does not affect correctness or production safety.
• No files require special attention.

This is a comment left during a code review.
Path: src/agents/tool-policy.test.ts
Line: 229-247

Comment:
**Duplicate assertions across the two new tests**

The second test (`#50303`) re-asserts `isToolAllowed(policy, "cron") === true` and rebuilds the same `resolved`/`policy` from `resolveSandboxToolPolicyForAgent(undefined, undefined)` that the first test already covers. The only net-new assertion is the `gateway` check. Consider either merging the two tests or removing the duplicate `cron` assertion from the second test to keep the suite DRY.

```suggestion
  it("default sandbox policy allows cron (gateway-routed, not containerized)", () => {
    const resolved = resolveSandboxToolPolicyForAgent(undefined, undefined);
    const policy: SandboxToolPolicy = { allow: resolved.allow, deny: resolved.deny };
    expect(isToolAllowed(policy, "cron")).toBe(true);
    expect(DEFAULT_TOOL_ALLOW).toContain("cron");
    expect(DEFAULT_TOOL_DENY).not.toContain("cron");
    // browser requires container execution — should remain denied
    expect(isToolAllowed(policy, "browser")).toBe(false);
    // gateway exposes admin actions — should remain denied
    expect(isToolAllowed(policy, "gateway")).toBe(false);
  });
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(sandbox): allow cron tool in sandbox..." | Re-trigger Greptile}

@BradGroux Thanks for the thorough analysis — this is exactly the kind of assessment I was hoping for when I tagged you.

I agree with your conclusion: ownerOnly is the correct enforcement layer, and the sandbox deny-list was never a real containment boundary for a gateway-routed tool. The UX cost of blocking (hallucinated cron calls) outweighs the theoretical defense-in-depth benefit.

Your read-only vs mutation split suggestion is interesting — cron.list and cron.status in sandbox with mutations blocked would be a clean middle ground. That feels like a good follow-up PR rather than scope creep here.

No objections from me on merging as-is. The persistence concern is noted and worth revisiting if sandbox threat modeling evolves.

The sandbox escape concern via sessionTarget has been evaluated by maintainers (see fabianwilliams and BradGroux discussion). The ownerOnly tool-level gate is the enforcement boundary, not the sandbox deny-list. Cron is gateway-routed (WebSocket RPC), never containerized, so the deny-list was incorrectly blocking it.

Rebased onto current upstream/main. Resolved one conflict in src/agents/system-prompt.ts:

• Upstream removed the browserNoVncUrl field entirely from sandboxInfo in the gap. Kept only this PR's actual contribution (the cron availability message), dropped the now-dead noVNC line so the rebased commit doesn't reference a removed field.

Verified availableTools is in scope at the use site (declared earlier in buildAgentSystemPrompt).

Force-pushed (with-lease) to your branch — maintainerCanModify was on. Watching CI; will squash-merge when green.

Per @pSteiny's directive: fix-and-merge velocity.

Status update: rebased onto current upstream/main, all conflicts resolved, PR-specific checks green (33+ ✓). Approved.

However, blocked by 3 pre-existing main breakages unrelated to this PR's diff:

• checks-node-agentic-plugin-sdk — failure in src/plugin-sdk/facade-loader.test.ts > plugin-sdk facade loader > uses native Jiti import for Windows dist facade loads
  • expected { interopDefault: true, …(3) } to deeply equal ObjectContaining {"tryNative": true}
  • Looks like the test expects tryNative: true in the createJiti call but the implementation now passes interopDefault: true (likely from 9e9aa4722a fix(plugins): load mirrored runtime deps through ESM-safe aliases — test/impl drift)
• checks-node-agentic-plugins — same root cause likely
• checks-node-channels — separate, haven't dug in

This PR doesn't touch src/plugin-sdk/ (verified via git diff upstream/main..HEAD -- src/plugin-sdk/ — empty). Recent main CI runs show a pattern of cancellations, suggesting main isn't consistently green right now.

Pinging @pSteiny / plugin-sdk owners to take a look at the facade-loader test/impl drift. Once main goes green, this PR will rebase cleanly and can merge. Holding here rather than admin-merging since the failures are real (just not ours).

This assigned pull request has been marked as stale after being open for 27 days.
Please add updates or it will be closed.

This assigned pull request has been automatically marked as stale after being open for 27 days.
Please add updates or it will be closed.

Closing due to inactivity.
If you believe this PR should be revived, post in #clawtributors on Discord to talk to a maintainer.
That channel is the escape hatch for high-quality PRs that get auto-closed.