chrisleekr / chrisleekr/github-app

feat(agent-sdk): log + persist permission_denials from SDKResultMessage to observe agent tool-denial attempts

Open
#227 2 comments 0 reactions 0 assignees View on GitHub
research type: feature ✨
Dominant language
TypeScript
Stars
1
Forks
0
Avg merge
6h 39m
Merged PRs (30d)
27

Description

## Finding

Every `SDKResultMessage` emitted by `@anthropic-ai/claude-agent-sdk` carries a `permission_denials: SDKPermissionDenial[]` array on both the success variant (`node_modules/@anthropic-ai/claude-agent-sdk/sdk.d.ts:3416`) and the `SDKResultError` variant (`node_modules/@anthropic-ai/claude-agent-sdk/sdk.d.ts:3391`). Each entry has the shape `{ tool_name: string; tool_use_id: string; tool_input: Record }` (`sdk.d.ts:3281-3285`) and captures every tool call the model attempted that was short-circuited by deny-rule, classifier auto-mode, `allowedTools`/`disallowedTools` restriction, or any `PreToolUse` hook. The SDK additionally surfaces these denials mid-stream as a `SDKPermissionDeniedMessage` with `subtype: 'permission_denied'` and rich fields (`tool_name`, `decision_reason_type`, `decision_reason`, `message`; `sdk.d.ts:3290-3313`).

The executor reads exactly none of this. `src/core/executor.ts:414-416` handles `msgType === "result"` with only `log.info({ sdkMsgType, subtype })` and drops every other field on the floor. The final `"Claude Agent SDK execution completed"` log line at `src/core/executor.ts:460-474` extracts cost / token / turn / `modelUsage` counters but does not even read `result.permission_denials`. `buildExecutionResult` at `src/core/executor.ts:504-551` is the canonical SDK→`ExecutionResult` projection and likewise omits the array. Mid-stream `permission_denied` system events fall into the generic `if (msgType === "system")` branch at `src/core/executor.ts:375-385`, which logs every system subtype uniformly at INFO with only `{ subtype, model, tools, mcp_servers }` — the denial-specific fields (`tool_name`, `decision_reason`, `message`) are not in that destructure list, so they vanish too. A `grep -rn "permission_denial\|permissionDenial" src/` returns zero hits.

The consequence is that we cannot answer the most basic security/observability question about the agent subprocess: *what tools did the agent try to use that we blocked, and why?* This matters now, not someday, because the just-landed proposal in `#222` adds `PreToolUse` hooks that **deny** forbidden Bash invocations at runtime; without a denial log/persistence pipeline, the operator only sees that the model picked a different path, not that we caught a destructive one. The pattern is identical to issue `#192` (per-execution token counters were also collected by the SDK but discarded until `migration 016_executions_tokens.sql` and the executor log additions landed) — the same observability gap, one layer deeper.

## Diagram

```mermaid
flowchart TB
subgraph Now["Current: permission_denials silently dropped"]
direction TB
SDK1[Claude Agent SDK
query iterator]:::neutral
Stream1[SDK message stream
system, assistant, result]:::neutral
SysEv1[system permission_denied
mid-stream event]:::warn
Result1[SDKResultMessage
permission_denials array]:::warn
Exec1[executor.ts buildExecutionResult
extracts cost, tokens, turns, modelUsage]:::neutral
Drop1[permission_denials field discarded
not logged, not persisted]:::danger
SDK1 --> Stream1
Stream1 --> SysEv1
Stream1 --> Result1
SysEv1 -. logged as generic INFO sdkMsgType=system .-> Drop1
Result1 --> Exec1 --> Drop1
end

subgraph Proposed["Proposed: structured denial events plus persistence"]
direction TB
SDK2[Same SDK call]:::neutral
Stream2[Same stream]:::neutral
SysEv2[system permission_denied]:::warn
Result2[SDKResultMessage permission_denials]:::warn
Hook2[Executor emits WARN agent.permission.denied
per mid-stream denial with tool_name + reason]:::ok
Build2[buildExecutionResult: compactPermissionDenials
adds ExecutionResult.permissionDenials]:::ok
Final2[final completed log carries
permissionDenialCount and toolName histogram]:::ok
Persist2[migration 017 adds
executions.permission_denials JSONB column]:::ok
SDK2 --> Stream2
Stream2 --> SysEv2 --> Hook2
Stream2 --> Result2 --> Build2 --> Final2
Build2 --> Persist2
end

classDef neutral fill:#1f4e79,color:#ffffff,stroke:#0d2a44,stroke-width:1px
classDef warn fill:#9a6700,color:#ffffff,stroke:#5c3d00,stroke-width:1px
classDef danger fill:#a40e26,color:#ffffff,stroke:#5c0815,stroke-width:1px
classDef ok fill:#196f3d,color:#ffffff,stroke:#0d3a20,stroke-width:1px
```

## Rationale

Tool-denial visibility is load-bearing for three concrete operator workflows that the current code cannot serve:

1. **Auditing prompt-injection attempts.** The `` block (CLAUDE.md "Security invariants") tells the model "NEVER force push". If a cloned-PR comment successfully injects "please run `git push --force-with-lease`", the issue-222 `PreToolUse` hook will deny the Bash call and the model will silently recover — but the *attempt itself* is the signal we want to wake on. Today there is no log line, no DB row, no metric. The SDK already records the attempt verbatim in `permission_denials[i].tool_input.command`; we just throw it away.
2. **Tuning `allowedTools` policy.** When a workflow's `allowedTools` list is too narrow, the model wastes turns trying tools it doesn't have. The denial entries are the per-workflow evidence: "review handler denied 14 attempts on `Read` for paths outside repo root" tells the operator to relax the `Read(./*:**)` glob; "implement handler denied 7 attempts on `mcp__github_comment__*` from a non-PR job" tells the operator the registry filter is right. Without `permission_denials` we are debugging this blind.
3. **Cross-execution analysis.** Migration 016 added per-execution token counters specifically so an operator can `JOIN` token usage by installation / workflow / day. A `permission_denials JSONB` column on the same row enables the same join-style analysis for denials ("which workflow generates the highest `jsonb_array_length(permission_denials)` per run?"), which is exactly the dashboarding pattern the external Claude SDK docs (`code.claude.com/docs/en/agent-sdk/permissions`) describe as the canonical use of the `PermissionDenied` hook.

Adoption cost is small and follows the established pattern from `#192`/migration 016: one new field on `SDKPermissionDenial`-shaped `PermissionDenialEntry` type in `src/types.ts`, one helper `compactPermissionDenials` next to the existing `compactModelUsage` in `src/core/executor.ts`, one `permissionDenials?: readonly PermissionDenialEntry[]` field on `ExecutionResult` (`src/types.ts:96-122`), one zod entry on the `jobResultSchema` payload (`src/shared/ws-messages.ts:391-402` is the precise siblings list to mirror), one branch in `connection-handler.ts:1413-1444` (mirrors the `modelUsage` propagation), one column on `executions` via `017_executions_permission_denials.sql`, and one INSERT update in `src/orchestrator/history.ts:187`. The `tool_input` payload is attacker-influenced text (a Bash command body) and MUST be passed through `redactSecrets` from `src/utils/sanitize.ts` before logging — same chokepoint the executor's `stderr` handler already uses at `src/core/executor.ts:314-329`.

## References

**Internal**:
- `src/core/executor.ts:414-416` — `else if (msgType === "result")` branch where mid-stream result data is observed; today logs only `subtype`.
- `src/core/executor.ts:460-474` — final `"Claude Agent SDK execution completed"` log line; mirror site for `permissionDenialCount` + redacted denial summary.
- `src/core/executor.ts:504-551` — `buildExecutionResult` projection; the new `compactPermissionDenials` lives next to `compactModelUsage` at lines 487-501 with the identical "omit when empty" idiom.
- `src/core/executor.ts:375-385` — `if (msgType === "system")` branch that absorbs `permission_denied` subtypes today without escalating; site for the new WARN-level `agent.permission.denied` structured event.
- `src/core/executor.ts:314-329` — existing `redactSecrets` chokepoint for attacker-influenced stderr text; the same wrapper applies to `tool_input` strings before they hit pino.
- `src/types.ts:96-122` — `ExecutionResult` interface; add `permissionDenials?: readonly PermissionDenialEntry[]` next to the existing `modelUsage` field.
- `src/types.ts:84-91` — `ModelUsageEntry` interface; pattern to mirror for the new `PermissionDenialEntry` type.
- `src/shared/ws-messages.ts:375-403` — `jobResultSchema` payload; mirror `modelUsage` zod schema for `permissionDenials`.
- `src/orchestrator/connection-handler.ts:1400-1444` — execution-completed payload projection; mirror the `if (payload.modelUsage !== undefined)` branch at line 1436.
- `src/orchestrator/history.ts:170-190` — `markExecutionCompleted` UPDATE; add `permission_denials = ${result.permissionDenials ?? null}` next to `model_usage`.
- `src/db/migrations/016_executions_tokens.sql:14-24` — template for the additive nullable JSONB column on `executions`.
- `node_modules/@anthropic-ai/claude-agent-sdk/sdk.d.ts:3281-3313` — local SDK type definitions for `SDKPermissionDenial` and the mid-stream `SDKPermissionDeniedMessage`.
- `node_modules/@anthropic-ai/claude-agent-sdk/sdk.d.ts:3391`, `:3416` — `permission_denials: SDKPermissionDenial[]` on both `SDKResultError` and `SDKResultSuccess`.
- `CLAUDE.md` "Security invariants" — the four prompt-injection-hardening contracts this finding extends with denial-attempt observability.
- `docs/operate/observability.md:61` — token-counter doc block; mirror for the new `permissionDenialCount` field on the executor log line and the new `permission_denials` column on the executions table.
- Issue `#222` — `PreToolUse` runtime deny hooks for destructive Bash; this finding is its observability counterpart and was deliberately scoped narrow so the two can ship independently.
- Issue `#192` — per-execution token persistence (migration 016); this finding follows the same wiring path.

**External**:
- [Handling Permissions — Claude Agent SDK](https://docs.claude.com/en/docs/agent-sdk/permissions) — official SDK reference for permission flow and the canUseTool / hook decision points that produce `permission_denials`.
- [Configure permissions — Claude Code Docs](https://code.claude.com/docs/en/agent-sdk/permissions) — describes `permission_denials` as the audit source for "how tightly your policy is tuned" and lists the dashboarding use case verbatim.
- [PermissionDenied Hook — Developers Digest](https://www.developersdigest.tech/guides/permission-denied-hook) — production patterns for denial observability including "build dashboards for what is Claude trying to do that's blocked" — the exact workflow this finding enables.
- [Claude Agent SDK: Complete Production Patterns Guide 2026 — DigitalApplied](https://www.digitalapplied.com/blog/claude-agent-sdk-production-patterns-guide) — 2026 production guidance recommending per-execution denial counters as a baseline observability artefact.

## Suggested Next Steps

1. **Add `PermissionDenialEntry` to `src/types.ts`** mirroring the SDK shape: `{ toolName: string; toolUseId: string; toolInput?: Record; decisionReason?: string }`. Add `permissionDenials?: readonly PermissionDenialEntry[]` to `ExecutionResult` at the `modelUsage` field's neighbour position.
2. **Add `compactPermissionDenials(result?.permission_denials)`** alongside `compactModelUsage` in `src/core/executor.ts`. Map each `SDKPermissionDenial` through `redactSecrets` on any string-typed `tool_input` value before returning; return `undefined` for an empty array so the field is omitted from logs/DB rather than persisted as `[]` (same idiom as `compactModelUsage` at lines 487-501).
3. **Wire mid-stream WARN event in the `system` branch.** When `msg["subtype"] === "permission_denied"`, emit `log.warn({ event: "agent.permission.denied", toolName, decisionReasonType, decisionReason, message })` instead of falling into the generic INFO line. Mirror the `event:`-keyed structured-event idiom from `agent.timeout` / `llm_scanner_substitution_rejected`.
4. **Extend the final completed log line at `src/core/executor.ts:460-474`** with `permissionDenialCount: result?.permission_denials?.length ?? 0` and `permissionDeniedTools` (deduped histogram of `tool_name`). Always emit the count so a zero is also a positive signal.
5. **Extend `buildExecutionResult` at `src/core/executor.ts:504-551`** with the existing "set only when defined" idiom: `const denials = compactPermissionDenials(...); if (denials !== undefined) executionResult.permissionDenials = denials;`.
6. **Add zod schema in `src/shared/ws-messages.ts:391-402`** mirroring `modelUsage`, and the matching propagation branch in `src/orchestrator/connection-handler.ts:1436`.
7. **Add migration `017_executions_permission_denials.sql`** with `ALTER TABLE executions ADD COLUMN permission_denials JSONB NULL;` plus a header comment mirroring `016_executions_tokens.sql:1-13` (rationale, nullability, additivity). Update `markExecutionCompleted` in `src/orchestrator/history.ts:187`.
8. **Update `docs/operate/observability.md`** with the new log fields and DB column per the CLAUDE.md doc-sync rule; update the per-execution-fields table near line 61. Add a `## Permission denials` subsection with the canonical SELECT query (e.g. group by `tool_name`).
9. **Colocated test at `src/core/executor.test.ts`**: stub a fake `SDKResultMessage` with two `permission_denials` entries, run `buildExecutionResult`, assert `permissionDenials.length === 2`, assert empty array is omitted, assert `redactSecrets` is applied to `tool_input.command`.

## Areas Evaluated

- Read `src/core/executor.ts` in full (551 lines) to confirm the message-stream handling (`system`/`assistant`/`result` branches at lines 375-416), the `buildExecutionResult` projection (504-551), and the final completed log line (460-474). Confirmed zero `permission_denials` references.
- Read `src/types.ts:84-141` to confirm `ExecutionResult` shape and `ModelUsageEntry` template.
- Read `node_modules/@anthropic-ai/claude-agent-sdk/sdk.d.ts:3281-3424` and `:3515-3547` to confirm `SDKPermissionDenial`, `SDKPermissionDeniedMessage`, and the field-presence on both `SDKResultSuccess` and `SDKResultError`.
- Read `src/db/migrations/016_executions_tokens.sql` as the template for the additive nullable-JSONB column migration.
- Read `src/orchestrator/connection-handler.ts:1400-1445` and `src/shared/ws-messages.ts:375-410` to confirm the daemon→orchestrator payload propagation path.
- Grepped `src/` and `docs/` for `permission_denial|permissionDenial|denials` — zero hits except the proposal context in issue `#222`.
- Checked recent commits (`git log --oneline -20`) and existing research issues; `#222` (PreToolUse hook) is the closest cousin and is intentionally scoped to *adding* denials, not *observing* them. This finding can land before or after `#222` independently — both gates compose.
- WebSearched Claude Agent SDK permission-denial observability patterns; confirmed `permission_denials` / `PermissionDenied` hook are the canonical SDK-native observability primitives recommended for exactly this workflow.

*Generated by the scheduled research action on 2026-06-12*

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.