Skip to content
Prev Previous commit
fix(daemon): tolerate null inputJson in ToolPermissionRouter
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>
  • Loading branch information
xinhuagu and claude committed May 3, 2026
commit 94ee5b56bd91c0229a9a0f84657a735efa07b146
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ private ToolPermissionRouter() { /* static-only */ }
* Routes a tool call to the structured or legacy permission entry point.
*
* @param delegate the tool being invoked (may or may not be {@link CapabilityAware})
* @param inputJson the JSON args the LLM supplied to the tool
* @param inputJson the JSON args the LLM supplied to the tool, or {@code null}
* / malformed if upstream couldn't produce a payload — those
* cases fall back to the legacy permission path (no crash)
* @param sessionId the owning session id, or {@code null} for daemon-internal calls
* @param description rich human-readable description for the approval prompt
* @param fallbackLevel risk level used when going through the legacy path
Expand All @@ -76,11 +78,15 @@ static PermissionDecision check(
PermissionManager permissionManager,
ObjectMapper mapper) {
Objects.requireNonNull(delegate, "delegate");
Objects.requireNonNull(inputJson, "inputJson");
Objects.requireNonNull(description, "description");
Objects.requireNonNull(fallbackLevel, "fallbackLevel");
Objects.requireNonNull(permissionManager, "permissionManager");
Objects.requireNonNull(mapper, "mapper");
// inputJson is intentionally NOT null-guarded — upstream (e.g.
// ContentBlock.ToolUse, PermissionAwareTool's parse path) tolerates
// missing/invalid arguments and the dispatcher previously absorbed
// them in the try-catch below. Hard-failing here would convert a
// recoverable flow into an unhandled crash. (Codex review on #482.)

if (!(delegate instanceof CapabilityAware capAware)) {
return checkLegacy(delegate, description, fallbackLevel, sessionId, permissionManager);
Expand All @@ -90,6 +96,9 @@ static PermissionDecision check(
try {
capability = capAware.toCapability(mapper.readTree(inputJson));
} catch (RuntimeException | IOException toCapErr) {
// Catches NPE from mapper.readTree(null), IOException from
// malformed JSON, IllegalArgumentException from toCapability's
// own arg validation — all fall back to the legacy prompt path.
log.warn("CapabilityAware tool {} rejected args; falling back to legacy permission path: {}",
delegate.name(), toCapErr.getMessage());
return checkLegacy(delegate, description, fallbackLevel, sessionId, permissionManager);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,33 @@ void permissionManagerErrorPropagates() {
.hasMessageContaining("policy bug");
}

@Test
void nullInputJsonFallsBackToLegacyInsteadOfCrashing() {
// Upstream (ContentBlock.ToolUse, PermissionAwareTool's parse
// path) can deliver a tool call with no arguments payload. The
// router must treat that as "couldn't build a Capability, prompt
// via legacy" rather than throw NPE — otherwise a recoverable
// upstream flow becomes an unhandled crash. (Codex review on
// #482, second pass.)
var policy = new CapturingPolicy();
var pm = new PermissionManager(policy);
var tool = new SpyCapabilityAwareTool();

var decision = ToolPermissionRouter.check(
tool, null, "sess-1", "fallback prompt", PermissionLevel.WRITE, pm, MAPPER);

assertThat(decision).isInstanceOf(PermissionDecision.Approved.class);
assertThat(tool.toCapabilityCalls.get())
.as("null input never reaches toCapability — readTree fails first, caught, legacy fallback")
.isZero();
assertThat(policy.last.get().toolName())
.as("legacy fallback was used")
.isEqualTo("spy_tool");
assertThat(policy.last.get().level())
.as("legacy fallback uses caller-supplied level")
.isEqualTo(PermissionLevel.WRITE);
}

@Test
void daemonInternalCallNullSessionIdGoesThroughBothPaths() {
// Cron, boot scripts, and other daemon-internal callers pass
Expand Down
Loading