Skip to content

feat(agent): gate destructive PostHog MCP exec sub-tools#1991

Merged
skoob13 merged 4 commits intomainfrom
feat/exec-tool-permissions
May 5, 2026
Merged

feat(agent): gate destructive PostHog MCP exec sub-tools#1991
skoob13 merged 4 commits intomainfrom
feat/exec-tool-permissions

Conversation

@skoob13
Copy link
Copy Markdown
Contributor

@skoob13 skoob13 commented May 4, 2026

Problem

The PostHog MCP exposes a single exec dispatcher (mcp__posthog__exec) that runs subcommands like call dashboard-update {...}. Once a user approves mcp__posthog__exec once (e.g. via "always allow"), every subsequent call flows through silently — including destructive writes (update, partial-update, delete, destroy). This violates least-privilege expectations: an approval intended for read traffic ends up authorizing arbitrary live-data mutations.

Changes

  • New posthog-exec-gate.ts matches PostHog exec tools, extracts the sub-tool from the command string, and flags the destructive subset.
  • canUseTool re-gates destructive sub-tools at sub-tool granularity. Approvals can be persisted per sub-tool (posthogApprovedExecTools in .claude/settings.local.json) via SettingsManager.addPostHogExecApproval. auto/bypassPermissions modes still skip the gate.
  • PreToolUseHook now returns permissionDecision: "ask" when an existing settings allow rule would otherwise short-circuit a destructive PostHog sub-tool, forcing the SDK to invoke canUseTool. Returning { continue: true } was insufficient — the SDK falls back to its default permission flow which re-checks the same allow rule.

How did you test this code?

  • New unit tests: posthog-exec-gate.test.ts (regex coverage), permission-handlers.test.ts (gate flow), settings.test.ts (approval persistence), hooks.test.ts (deferral behavior).
  • Manually reproduced in PostHog Code with a mcp__posthog__exec allow rule in ~/.claude/settings.local.json: call dashboard-update {...} now triggers an approval prompt; call experiment-get {...} does not.

Publish to changelog?

no

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 4, 2026

Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
packages/agent/src/adapters/claude/permissions/posthog-exec-gate.test.ts:54-84
**Prefer parameterised tests**

Multiple assertions are grouped into a single `it` block here (and throughout this file), which makes it hard to see which specific input fails. The same pattern appears in `isPostHogExecTool` and `extractPostHogSubTool` tests. Using `it.each` surfaces the failing case in the test name without any extra effort:

```typescript
it.each([
  ["experiment-update", true],
  ["feature-flag-delete", true],
  ["notebooks-destroy", true],
  ["experiment-partial-update", true],
  ["update-something", true],
  ["delete", true],
  ["experiment-get", false],
  ["feature-flag-list", false],
  ["experiment-create", false],
  ["insights-pause", false],
  ["get-updated-events", false],
  ["deleter-test", false],
])("isPostHogDestructiveSubTool(%s) === %s", (input, expected) => {
  expect(isPostHogDestructiveSubTool(input)).toBe(expected);
});
```

The same refactor applies to the `isPostHogExecTool` and `extractPostHogSubTool` describes.

### Issue 2 of 3
packages/agent/src/adapters/claude/hooks.test.ts:230-313
**Prefer parameterised tests**

The four `createPreToolUseHook` tests share the same shape — build a settings stub, create the hook, call it with an input, assert on the result — but are written as separate `test()` blocks. An `it.each` table would cover all cases with one declaration and surface the failing case in the test name:

```typescript
it.each([
  [
    "destructive PostHog exec sub-tool is deferred via ask",
    "mcp__posthog__exec",
    { command: 'call dashboard-update {"id": 1, "name": "x"}' },
    { hookSpecificOutput: { permissionDecision: "ask" } },
  ],
  [
    "partial-update is also deferred",
    "mcp__posthog__exec",
    { command: 'call cohorts-partial-update {"id": 1}' },
    { hookSpecificOutput: { permissionDecision: "ask" } },
  ],
  [
    "non-destructive PostHog sub-tool is allowed",
    "mcp__posthog__exec",
    { command: 'call experiment-get {"id": 1}' },
    { hookSpecificOutput: { permissionDecision: "allow" } },
  ],
])("%s", async (_, toolName, toolInput, expected) => { … });
```

### Issue 3 of 3
packages/agent/src/adapters/claude/permissions/permission-handlers.ts:557-565
**OnceAndOnlyOnce: denial path duplicated**

The block at the bottom of `handlePostHogExecApprovalFlow` is byte-for-byte identical to the tail of `handleMcpApprovalFlow`. Extracting a shared helper would satisfy the simplicity rule and make future changes to the denial message or `interrupt` logic apply to both flows automatically:

```typescript
async function buildDenialResult(
  context: ToolHandlerContext,
  response: { _meta?: unknown },
): Promise<ToolPermissionResult> {
  const feedback = (response._meta?.customInput as string | undefined)?.trim();
  const message = feedback
    ? `User refused permission to run tool with feedback: ${feedback}`
    : "User refused permission to run tool";
  await emitToolDenial(context, message);
  return { behavior: "deny", message, interrupt: !feedback };
}
```

Both `handleMcpApprovalFlow` and `handlePostHogExecApprovalFlow` can then delegate to this helper.

Reviews (1): Last reviewed commit: "fix: remove logs" | Re-trigger Greptile

Comment thread packages/agent/src/adapters/claude/hooks.test.ts
@skoob13 skoob13 requested a review from a team May 4, 2026 10:57
Copy link
Copy Markdown
Contributor

@tatoalo tatoalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooof, this is great, thanks!

@skoob13 skoob13 merged commit f248727 into main May 5, 2026
15 checks passed
@skoob13 skoob13 deleted the feat/exec-tool-permissions branch May 5, 2026 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants