fix(mcp): quarantine invalid allowlist entries - #183
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 38 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 (4)
📝 WalkthroughPriority Level: P3 No actionable P0–P3 issues identified from the provided changes. The implementation appears to preserve valid entries, quarantine rejected entries, maintain fail-closed behavior, sanitize diagnostics, and report degraded policy state. WalkthroughMCP allowlist handling now supports degraded policies: invalid or ambiguous entries are quarantined, valid repositories remain available, filesystem aliases can resolve safely, and CLI/server diagnostics report sanitized rejection details. Confidence: 96%. ChangesMCP repository policy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant createMcpRepositoryPolicy
participant FilesystemRegistry
participant MCPBackend
MCPClient->>createMcpRepositoryPolicy: Build repository policy
createMcpRepositoryPolicy->>FilesystemRegistry: Resolve paths and filesystem identities
FilesystemRegistry-->>createMcpRepositoryPolicy: Valid repositories and rejected entries
createMcpRepositoryPolicy-->>MCPClient: Degraded or blocked policy
MCPClient->>MCPBackend: Call tool for resolved repository
MCPBackend-->>MCPClient: Result or access error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
evaOS review status: completedPR: #183 - fix(mcp): quarantine invalid allowlist entries evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #183 Review URL: #183 (review) |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #183 - fix(mcp): quarantine invalid allowlist entries
Head: 3fa87e14d180b9be7342de615a444b9c16d9a8e9 into main. Review event: COMMENT.
Provider: Unregistered provider id (builtin:zai-coding-plan, zcode (registry miss), model unknown).
Estimated review effort: 1/5 (~10 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
No finding categories.
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: sufficient - PR metadata mentions acceptable proof for each required validation recommendation.
Profile validation hints: Call out stale index, wrong repo identity, memory growth, and destructive analyze behavior.
Profile proof expectations: Look for focused CLI, index, query, or migration proof.
Related Context
Related issues/PRs: #182.
Suggested labels: none.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: gitnexus, code-intelligence, backend
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gitnexus/src/cli/doctor.ts (1)
279-301: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConfidence: 70%. Degraded MCP policy doesn't set a non-zero exit code.
process.exitCode = 1is only setif (!report.valid)(Line 299). For the new degraded branch,report.validistrue, sogitnexus doctor --mcp-config(or--json) exits0even when entries were rejected. GUARDRAILS.md's new remediation step tells operators to run this command to "repair the named environment key/entry," implying it's meant to be detectable/actionable — but any CI/automation gating on exit code won't notice a degraded policy at all, only a fully blocked one.🩺 Proposed fix
- if (!report.valid) process.exitCode = 1; + if (!report.valid || report.degraded) process.exitCode = 1;🤖 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 `@gitnexus/src/cli/doctor.ts` around lines 279 - 301, Update the exit-status handling in the options.mcpConfig branch of the doctor command so degraded reports with rejectedEntries also set process.exitCode to 1, including JSON output. Preserve the existing success status for fully valid, non-degraded reports and the current failure handling for invalid reports.
🤖 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 `@gitnexus/src/mcp/repository-policy.ts`:
- Around line 114-124: Move filesystem alias resolution out of the synchronous
tool-call path: update resolveRuntimeRepo(), repoForArgs(), and resolveRepo() so
they no longer invoke existingFilesystemIdentity() during request handling.
Prefer precomputing the filesystem alias map during startup and using it for
lookups, or remove the fallback if alias matching is not required; preserve
exact pathKey resolution and ensure runtime repository resolution performs no
synchronous realpathSync.native or statSync calls.
---
Outside diff comments:
In `@gitnexus/src/cli/doctor.ts`:
- Around line 279-301: Update the exit-status handling in the options.mcpConfig
branch of the doctor command so degraded reports with rejectedEntries also set
process.exitCode to 1, including JSON output. Preserve the existing success
status for fully valid, non-degraded reports and the current failure handling
for invalid reports.
🪄 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: 6b927a6a-0d28-4efc-91db-aeb2589e8576
📒 Files selected for processing (6)
GUARDRAILS.mdgitnexus/src/cli/doctor.tsgitnexus/src/cli/mcp.tsgitnexus/src/mcp/repository-policy.tsgitnexus/test/unit/mcp-config-doctor.test.tsgitnexus/test/unit/mcp-repository-policy.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (22)
- GitHub Check: tests / macos-latest (platform-sensitive) 1/3
- GitHub Check: tests / macos-latest (platform-sensitive) 3/3
- GitHub Check: tests / windows-latest (platform-sensitive) 3/3
- GitHub Check: tests / macos-latest (platform-sensitive) 2/3
- GitHub Check: tests / windows-latest (platform-sensitive) 2/3
- GitHub Check: tests / ubuntu / coverage 1/3
- GitHub Check: tests / windows-latest (platform-sensitive) 1/3
- GitHub Check: tests / ubuntu / coverage 2/3
- GitHub Check: tests / ubuntu / coverage 3/3
- GitHub Check: tests / tree-sitter ABI (windows-latest)
- GitHub Check: tests / tree-sitter ABI (ubuntu-latest)
- GitHub Check: tests / packaged install smoke (ubuntu-latest)
- GitHub Check: tests / tree-sitter ABI (macos-latest)
- GitHub Check: quality / typecheck
- GitHub Check: tests / benchmarks (GITNEXUS_BENCH)
- GitHub Check: tests / node floor compat (22.14)
- GitHub Check: tests / packaged install smoke (windows-latest)
- GitHub Check: autofix
- GitHub Check: Build gitnexus
- GitHub Check: Build gitnexus-web
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
gitnexus/**
📄 CodeRabbit inference engine (.cursor/rules/100-monorepo.mdc)
For CLI (
gitnexus/): Run tests withnpm test, integration tests withnpm run test:integration, and type-check withnpx tsc --noEmit
Files:
gitnexus/src/cli/mcp.tsgitnexus/test/unit/mcp-config-doctor.test.tsgitnexus/src/cli/doctor.tsgitnexus/test/unit/mcp-repository-policy.test.tsgitnexus/src/mcp/repository-policy.ts
gitnexus/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
gitnexus/**/*.{ts,tsx}: Shared code undergitnexus/src/core/ingestion/must not name languages; provide language-specific behavior throughLanguageProvider/ScopeResolverhooks, implementing and registeringScopeResolverinSCOPE_RESOLVERS.
Before editing any function, class, or method, run GitNexus upstream impact analysis and report the blast radius; warn the user before proceeding when risk is HIGH or CRITICAL.
Use GitNexusquery({search_query: ...})to explore unfamiliar code andcontext({name: ...})for complete symbol context instead of grepping.
Never rename symbols with find-and-replace; use GitNexusrename, which understands the call graph.
Follow the configured ESLint rules for TypeScript, React Hooks, and unused imports; runnpx eslint .because no lint script is defined.
Use the documented GitNexus architecture and scope-resolution contracts when changing indexing, call-resolution, or inheritance behavior.
Files:
gitnexus/src/cli/mcp.tsgitnexus/test/unit/mcp-config-doctor.test.tsgitnexus/src/cli/doctor.tsgitnexus/test/unit/mcp-repository-policy.test.tsgitnexus/src/mcp/repository-policy.ts
gitnexus/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Run CLI/core validation with
npm testandnpx tsc --noEmitwhen applicable.
Files:
gitnexus/src/cli/mcp.tsgitnexus/test/unit/mcp-config-doctor.test.tsgitnexus/src/cli/doctor.tsgitnexus/test/unit/mcp-repository-policy.test.tsgitnexus/src/mcp/repository-policy.ts
🔇 Additional comments (7)
gitnexus/src/mcp/repository-policy.ts (3)
22-33: LGTM!Also applies to: 88-90, 176-202
260-297: LGTM!Also applies to: 352-379
439-469: LGTM!Also applies to: 494-494, 503-511, 530-537
gitnexus/src/cli/mcp.ts (1)
92-115: LGTM!GUARDRAILS.md (1)
60-64: LGTM!gitnexus/test/unit/mcp-config-doctor.test.ts (1)
36-59: LGTM!Also applies to: 83-83
gitnexus/test/unit/mcp-repository-policy.test.ts (1)
4-6: LGTM!Also applies to: 176-235, 237-274, 276-309, 380-387
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 14891 tests passed 63 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Review disposition for the outside-diff doctor finding: fixed in 1dad094. A degraded policy still keeps valid MCP repositories usable, but |
evaOS review status: completedPR: #183 - fix(mcp): quarantine invalid allowlist entries evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #183 Review URL: #183 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #183 - fix(mcp): quarantine invalid allowlist entries
Head: 1dad0948e8321e50f251dd316a24e4b2f04dd4d0 into main. Review event: COMMENT.
Provider: Unregistered provider id (builtin:zai-coding-plan, zcode (registry miss), model unknown).
Estimated review effort: 1/5 (~10 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
No finding categories.
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: sufficient - PR metadata mentions acceptable proof for each required validation recommendation.
Profile validation hints: Call out stale index, wrong repo identity, memory growth, and destructive analyze behavior.
Profile proof expectations: Look for focused CLI, index, query, or migration proof.
Related Context
Related issues/PRs: #182.
Suggested labels: none.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: gitnexus, code-intelligence, backend
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
1 participant
Summary
Closes #182.
Reproduction fixed
The registry stored
/Volumes/LEXAR/repos/gitnexus-approved/evaos-hive, while one generated policy used/Volumes/LEXAR/repos/gitnexus-approved/evaOS-Hive. On the case-insensitive macOS volume those paths are the same inode, but electric.8 rejected the entry by string and blocked every Codex MCP repository operation, including NeonDiff.Validation
npx vitest run test/unit/mcp-repository-policy.test.ts test/unit/mcp-config-doctor.test.ts test/unit/doctor-readonly-cli.test.ts— 39 passednpx tsc --noEmit— passedvalid=true,degraded=true, only typo entry rejectedlist_reposexposed degraded status; NeonDiff and Hive queries succeeded; rejected repo call returned unavailable; startup log did not reveal configured valueSecurity boundary
This never falls back to unrestricted access. A rejected entry grants no access. A configured allowlist with zero resolved entries remains blocked, and default-repository validation remains blocking.