fix: safely classify PostHog command wrapper reads - #109
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMiftah adds strict PostHog command-wrapper risk classification, integrates argument-aware metadata into calls and route previews, and centralizes policy enforcement. Tests and documentation cover trusted canonical commands, fail-closed invalid cases, local overrides, and changelog contracts. ChangesPostHog command risk and enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/policy/posthog-command-wrapper.ts`:
- Around line 9-11: Extract destructivePattern, writePattern, and readPattern
from src/policy/risk-classifier.ts lines 5-7 into a shared module such as
src/policy/risk-patterns.ts, then import and reuse them in both
src/policy/risk-classifier.ts lines 5-7 and
src/policy/posthog-command-wrapper.ts lines 9-11; remove the duplicate
declarations while preserving the existing regex behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3076e4cf-2cef-4314-87c0-fece4c94adbb
📒 Files selected for processing (14)
CHANGELOG.mddocs/architecture.mddocs/config.mddocs/security.mdsrc/mcp/server/miftah-server.tssrc/mcp/server/operation-pipeline.tssrc/policy/policy-types.tssrc/policy/posthog-command-wrapper.tssrc/policy/risk-classifier.tstests/fixtures/fake-upstream.mjstests/mcp-wrapper.test.tstests/posthog-command-wrapper.test.tstests/preset-docs-contract.test.tstests/risk-classification-docs-contract.test.ts
|
@coderabbitai review Review-state note for #110: the only inline finding is resolved by CodeRabbit on commit 57f6e2e, and the CodeRabbit status check is successful. GitHub still retains the original CHANGES_REQUESTED review object anchored to 0bff102. Requesting a fresh current-head review so the review state reflects the resolved thread. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/server/miftah-server.ts (1)
1526-1539: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecognize one-entry named upstream configurations in cold previews.
With
upstreams: { posthog: ... }containing one canonical PostHog endpoint, this returns no metadata becauseconfig.upstreamis absent. A later discoveredexectarget carries its named upstream and can classify the same command as read, so preview and invocation can disagree. Derive the sole configured upstream name whenconfig.upstreamshas exactly one entry; keep multi-upstream previews conservative.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server/miftah-server.ts` around lines 1526 - 1539, Update isOfficialPosthogCommandToolName to derive the sole configured upstream name from config.upstreams when it contains exactly one entry, then pass that name to isOfficialPosthogCommandUpstream for cold-preview classification. Preserve the existing config.upstream behavior, and remain conservative by returning no metadata when multiple named upstreams are configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/mcp/server/miftah-server.ts`:
- Around line 1526-1539: Update isOfficialPosthogCommandToolName to derive the
sole configured upstream name from config.upstreams when it contains exactly one
entry, then pass that name to isOfficialPosthogCommandUpstream for cold-preview
classification. Preserve the existing config.upstream behavior, and remain
conservative by returning no metadata when multiple named upstreams are
configured.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d276e7ca-3076-40f5-93b1-d3b89f1c9e7a
📒 Files selected for processing (4)
src/mcp/server/miftah-server.tssrc/mcp/server/operation-pipeline.tssrc/policy/posthog-command-wrapper.tssrc/policy/risk-classifier.ts
|
Follow-up ticket #116 is fixed in 1a1e0c3. It adds cold-preview parity for exactly one named PostHog upstream and preserves conservative behavior for multiple named upstreams. Focused regression, parser tests, typecheck, lint, and build pass. @coderabbitai review |
|
✅ Action performedReview finished.
|
1 participant
What changed
Fixes #106, fixes #107, fixes #108, fixes #110, fixes #114.
Miftah now recognizes the strict PostHog command-wrapper grammar only when the configured upstream is the literal canonical endpoint
https://mcp.posthog.com/mcp. Read/discovery commands can run under destructive-routing safeguards without a per-user routing rule.The change also:
execcommands as destructive;miftah_route_preview, so preview no longer reports allow when the real call is blocked;:443port rather than relying on URL normalization;Root cause
The official PostHog MCP exposes a generic
execwrapper. Miftah correctly treated the unknown outer name as destructive, but that blocked documented read commands. Route preview evaluated policy without the subsequent explicit-routing enforcement step, so it could disagree with an actual call.Validation
npm test— 1,063 passed, 22 platform-gated skipsnpm run test:corenpm run test:coverage— 1,063 passed, 95.49% lines / 92.03% branchesnpm run lintnpm run typechecknpm run buildnpm run smoke:clinpm run check:packnpm run test:packageFocused coverage includes canonical read behavior, write/destructive/malformed rejection, noncanonical query and explicit-port rejection, override precedence, preview/actual enforcement parity, audit redaction, and documented helper contracts.
Summary by CodeRabbit