feat(policy): classify risk from trusted MCP annotations - #64
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds annotation-aware tool risk classification with provenance and confidence, configurable unknown-tool defaults, trusted upstream controls, routing and audit propagation, public configuration types, validation tests, and updated documentation. ChangesRisk classification
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
CodeRabbit exception documented before merge: its only current-head marker has remained “processing” since 2026-07-12T19:45:58Z (over four hours) with no submitted review or inline thread. Current head |
|
@coderabbitai review The required CodeRabbit check has remained in progress without a review for over four hours. Please retry the current head |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/mcp/server/miftah-server.ts`:
- Line 535: Update the routed-call metadata construction around
riskMetadata(mapped) to recompute trust from the resolved target/upstream rather
than inheriting the source snapshot’s trusted flag; apply the same correction to
the route preview’s riskMetadata(sourceTool). Ensure resolveTarget() or the
relevant riskMetadata path validates upstream identity in addition to schema
compatibility before marking the target trusted.
In `@tests/config-runtime-parity.test.ts`:
- Around line 100-110: Hoist the three inline regex literals used by the
validateConfig toThrow assertions into module-scope constants in
tests/config-runtime-parity.test.ts, then reuse those constants in the
assertions near the shown tests. Preserve each existing pattern and assertion
behavior.
In `@tests/fixtures/fake-upstream.mjs`:
- Around line 143-146: Update the TEST_CREATE_ITEM_ANNOTATIONS parsing near
createItemAnnotations to catch malformed JSON and rethrow or report a clear
test-fixture-specific error message. Preserve undefined when the environment
variable is absent and continue returning the parsed value for valid JSON.
In `@tests/risk-classification-docs-contract.test.ts`:
- Line 32: Hoist the static regular expression used by the changelog assertion
in the risk classification contract test to module scope, then reuse that named
regex in the toMatch call. Preserve the existing pattern and matching behavior
while satisfying e18e/prefer-static-regex.
🪄 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: 162de235-d23d-4f12-8487-9de529a6c699
📒 Files selected for processing (26)
CHANGELOG.mdREADME.mddocs/architecture.mddocs/config.mddocs/library-api.mddocs/security.mdsrc/audit/audit-trail.tssrc/audit/audit-types.tssrc/config/schema.tssrc/config/types.tssrc/index.tssrc/mcp/server/miftah-server.tssrc/mcp/server/operation-pipeline.tssrc/mcp/server/tool-registry.tssrc/policy/policy-engine.tssrc/policy/policy-types.tssrc/policy/risk-classifier.tstests/config-runtime-parity.test.tstests/config-schema-contract.test.tstests/fixtures/fake-upstream.mjstests/mcp-wrapper.test.tstests/package-contract.test.tstests/public-api.test.tstests/risk-classification-docs-contract.test.tstests/routing-policy.test.tstests/tool-registry.test.ts
|
Addressed all CodeRabbit findings in remote commit
Validated: focused pipeline/MCP tests, |
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)
714-729: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
src/mcp/server/miftah-server.ts:714-729AddoriginalNameto the cached-target match inmiftah_route_preview.fingerprintis based on the exposed client name, so a different underlying tool can still match onfingerprint+upstreamName; without the sameoriginalNameguard used inhandleUpstreamTool, preview/audit can surfacetrusted-upstream-annotationfor the wrong tool.🤖 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 714 - 729, Update the hasCompatibleCachedTarget calculation in miftah_route_preview to also require sourceTool.originalName and targetTool.originalName to match. Preserve the existing fingerprint and upstreamName checks, ensuring preview/audit only uses target risk metadata when all three tool identity fields match.
🤖 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 714-729: Update the hasCompatibleCachedTarget calculation in
miftah_route_preview to also require sourceTool.originalName and
targetTool.originalName to match. Preserve the existing fingerprint and
upstreamName checks, ensuring preview/audit only uses target risk metadata when
all three tool identity fields match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 29b578b4-1a5c-4aeb-a183-260c814110cc
📒 Files selected for processing (7)
src/mcp/server/miftah-server.tssrc/mcp/server/operation-pipeline.tstests/config-runtime-parity.test.tstests/fake-upstream-fixture.test.tstests/fixtures/fake-upstream.mjstests/operation-pipeline.test.tstests/risk-classification-docs-contract.test.ts
|
Follow-up CodeRabbit identity finding addressed in |
1 participant
Closes #26
Summary
riskSource/riskConfidence) in route preview and audit events without exposing raw annotations.UnknownToolRisk, update generated schema and docs, and add unit, integration, contract, type, and package coverage.Validation
npm run lintnpm run typechecknpm run buildnpm run smoke:clinpm_config_cache=/private/tmp/miftah-npm-cache npm run check:packNODE_OPTIONS=--require=<temporary localhost-only shim> npm run test:core— 232 passed, 11 expected platform skipsThis sandbox cannot resolve
localhost, bind127.0.0.1, or complete a fresh npm consumer install. The shim was untracked and removed; full package and remote transport validation remains required CI gating.Summary by CodeRabbit
New Features
UnknownToolRiskpublic configuration/type surface.Documentation
Bug Fixes