feat: verify upstream identities - #61
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 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 (1)
📝 WalkthroughWalkthroughAdds opt-in upstream identity fingerprint verification with strict configuration, in-memory caching, session invalidation, protected-operation gating, MCP management tooling, redacted auditing, doctor reporting, public types, documentation, and comprehensive tests. ChangesIdentity verification and configuration
MCP enforcement and readiness
Estimated code review effort: 4 (Complex) | ~60 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/doctor.ts (1)
505-568: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIdentity verification failures can be mislabeled as tool-discovery failures.
identities.verify(...)(line 537) executes inside the same try block aslistTools(session)and is caught by the same catch (line 555), which pushes aDOCTOR_CODES.TOOLS_DISCOVERYfailure/warning and callsrecordCapabilityFailure(..., "tools", ...). Ifverify()ever throws instead of returning a non-verified status (current tests only exercise the non-throwing failure path viaTEST_FAIL_CALL_TOOL), a healthy tool discovery would be incorrectly reported as failed and the upstream manager's "tools" capability tracking would be corrupted, obscuring the actual (identity) failure.Isolating identity verification into its own try/catch — pushing
unavailableIdentityCheck(target, targetText, "discovery")on error without touchingDOCTOR_CODES.TOOLS_DISCOVERYorrecordCapabilityFailure("tools", ...)— would make this robust regardless ofidentities.verify()'s exact throwing contract.♻️ Suggested refactor
try { const result = await listTools(session); runtime.manager.recordCapabilitySuccess(target.profile, "tools", target.upstreamName); checks.push( check( DOCTOR_CODES.TOOLS_DISCOVERY, result.truncated ? "warning" : "pass", targetText, result.truncated ? "Tool discovery returned a cursor. Additional tool pages are not currently exposed by the wrapper." : `Tool discovery completed with ${result.tools.length} item(s).`, result.truncated ? "Use only the currently exposed tools until the wrapper supports additional tool pages." : noAction() ) ); const fingerprints = visibleTools.get(target.profile) ?? new Map<string, string>(); visibleTools.set(target.profile, fingerprints); if (recordCollision(checks, target, fingerprints, result.tools, runtime.config.tooling?.collisionStrategy)) { incompleteProfiles.add(target.profile); } - const configuredIdentity = identities.status(target.profile, target.upstreamName); - if (configuredIdentity.status === "unconfigured") { - checks.push( - identityCheck( - "skipped", - targetText, - "No upstream identity verification is configured.", - "Configure profile identity verification to validate risky operations." - ) - ); - } else { - const identity = await identities.verify(target.profile, target.upstreamName, session); - const required = identityRequired(target); - checks.push( - identity.status === "verified" - ? identityCheck( - "pass", - targetText, - "Configured upstream identity verification completed.", - noAction() - ) - : identityCheck( - required ? "error" : "warning", - targetText, - "Configured upstream identity verification did not complete.", - "Review the configured expected fingerprint and identity probe before relying on risky operations." - ) - ); - } } catch (error) { incompleteProfiles.add(target.profile); runtime.manager.recordCapabilityFailure(target.profile, "tools", error, target.upstreamName); checks.push( check( DOCTOR_CODES.TOOLS_DISCOVERY, discoveryFailureStatus, targetText, "Tool discovery did not complete.", "Review upstream tool discovery before relying on this profile." - ), - unavailableIdentityCheck(target, targetText, "discovery") + ) ); + checks.push(unavailableIdentityCheck(target, targetText, "discovery")); + } + + try { + const configuredIdentity = identities.status(target.profile, target.upstreamName); + if (configuredIdentity.status === "unconfigured") { + checks.push( + identityCheck( + "skipped", + targetText, + "No upstream identity verification is configured.", + "Configure profile identity verification to validate risky operations." + ) + ); + } else { + const identity = await identities.verify(target.profile, target.upstreamName, session); + const required = identityRequired(target); + checks.push( + identity.status === "verified" + ? identityCheck("pass", targetText, "Configured upstream identity verification completed.", noAction()) + : identityCheck( + required ? "error" : "warning", + targetText, + "Configured upstream identity verification did not complete.", + "Review the configured expected fingerprint and identity probe before relying on risky operations." + ) + ); + } + } catch { + checks.push(unavailableIdentityCheck(target, targetText, "discovery")); }🤖 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/cli/doctor.ts` around lines 505 - 568, Separate the identities.verify flow from the listTools try/catch in the doctor check. Keep tool discovery success and failure handling, including recordCapabilityFailure(..., "tools", ...), limited to listTools-related errors; wrap identity verification in its own try/catch that adds unavailableIdentityCheck(target, targetText, "discovery") without adding a TOOLS_DISCOVERY failure or recording a tools capability failure.
🤖 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/config/schema.ts`:
- Around line 63-139: Keep the identity validation rules synchronized between
identitySchema in schema.ts and the isIdentityConfig runtime guard in
validate-config.ts. Any change to expected-field requirements, requiredForRisk
uniqueness, or JSON/text probe provider and field constraints must be applied
consistently in both validators so they accept and reject the same
configurations.
In `@src/mcp/server/miftah-server.ts`:
- Around line 108-111: In the fields schema construction within the reduce
callback, remove the redundant key-based ternary and assign the `"string"` type
directly. Keep the existing fields iteration and result structure unchanged.
In `@tests/config.test.ts`:
- Line 73: Hoist the static regular expressions used by the tests in
config.test.ts to module scope, including the
profiles.work.identity.requiredForRisk pattern and the probe.provider patterns
at the referenced assertions plus the expected.login pattern. Reuse these
module-level RegExp constants in the corresponding toThrow assertions to satisfy
prefer-static-regex without changing test behavior.
In `@tests/identity-docs-contract.test.ts`:
- Around line 8-18: Resolve the e18e lint failures in
tests/identity-docs-contract.test.ts by hoisting the repeated regex literals
used by unreleasedSection, identityVerificationSection, and the other reported
sections to module scope, then replacing matchAll spread-plus-map expressions
with Array.from(matches, mapper). Preserve all existing matching and mapping
behavior while removing the unnecessary intermediate arrays.
In `@tests/operation-pipeline.test.ts`:
- Around line 57-60: Update the finally cleanup blocks in the affected tests,
including the test around the client and wrapper close calls and the one around
lines 115-118, to remove each mkdtemp directory with recursive, forced removal.
Preserve the existing client and wrapper close operations and follow the cleanup
pattern used by the multi-upstream test.
---
Outside diff comments:
In `@src/cli/doctor.ts`:
- Around line 505-568: Separate the identities.verify flow from the listTools
try/catch in the doctor check. Keep tool discovery success and failure handling,
including recordCapabilityFailure(..., "tools", ...), limited to
listTools-related errors; wrap identity verification in its own try/catch that
adds unavailableIdentityCheck(target, targetText, "discovery") without adding a
TOOLS_DISCOVERY failure or recording a tools capability failure.
🪄 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: f050e60c-ece5-41f0-9522-9a1f5f0c4de6
📒 Files selected for processing (33)
CHANGELOG.mdREADME.mddocs/architecture.mddocs/cli.mddocs/config.mddocs/library-api.mddocs/security.mdsrc/audit/audit-trail.tssrc/audit/audit-types.tssrc/cli/doctor-report.tssrc/cli/doctor.tssrc/cli/exit-codes.tssrc/config/schema.tssrc/config/types.tssrc/config/validate-config.tssrc/identity/identity-manager.tssrc/identity/identity-types.tssrc/index.tssrc/mcp/server/miftah-server.tssrc/mcp/server/operation-pipeline.tssrc/upstream/upstream-process-manager.tssrc/upstream/upstream-session.tssrc/utils/errors.tstests/cli-exit-codes.test.tstests/config.test.tstests/doctor.test.tstests/fixtures/fake-upstream.mjstests/identity-docs-contract.test.tstests/identity-manager.test.tstests/mcp-wrapper.test.tstests/operation-pipeline.test.tstests/package-contract.test.tstests/public-api.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Resolved the outside-diff doctor finding in 4f27fee: tool discovery and identity verification now have separate error boundaries. A failed identity verification cannot mark tools discovery failed or mutate tools capability health. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/doctor.ts (1)
421-443: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMisleading "discovery" reason reused for identity-verification failures after discovery succeeded.
At line 575, the catch fires only when
discoveryCompletedis alreadytrue(tool discovery succeeded) butidentities.verify(...)/identities.status(...)throws. ReusingunavailableIdentityCheck(target, targetText, "discovery")here produces the message "Identity verification was skipped because tool discovery did not complete," which is inaccurate — discovery completed; the identity check itself failed unexpectedly. This undermines the intent of separating tool-discovery and identity-verification error boundaries: the code paths are separated, but the reported reason text isn't.🐛 Proposed fix: add a dedicated "verification" reason
- const unavailableIdentityCheck = (target: DoctorTarget, targetText: string, reason: "startup" | "discovery"): DoctorCheck => { + const unavailableIdentityCheck = ( + target: DoctorTarget, + targetText: string, + reason: "startup" | "discovery" | "verification" + ): DoctorCheck => { const configured = identities.status(target.profile, target.upstreamName); if (configured.status === "unconfigured") { + const prerequisite = + reason === "startup" ? "upstream startup" : reason === "discovery" ? "tool discovery" : "identity verification"; return identityCheck( "skipped", targetText, - `Identity verification was skipped because ${reason === "startup" ? "upstream startup" : "tool discovery"} did not complete.`, - `Resolve the ${reason === "startup" ? "startup" : "tool discovery"} check before retrying doctor.` + `Identity verification was skipped because ${prerequisite} did not complete.`, + `Resolve the ${prerequisite} check before retrying doctor.` ); } ...} catch { - checks.push(unavailableIdentityCheck(target, targetText, "discovery")); + checks.push(unavailableIdentityCheck(target, targetText, "verification")); }Also applies to: 543-577
🤖 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/cli/doctor.ts` around lines 421 - 443, The unavailableIdentityCheck helper currently conflates failed identity verification with incomplete tool discovery. Add a dedicated “verification” reason to its reason type and message handling, then update the catch path after discoveryCompleted in the identity verification flow to pass “verification”; preserve the existing “startup” and “discovery” messages for their respective paths.
🤖 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 `@tests/identity-docs-contract.test.ts`:
- Line 75: Replace the inline digit-grouping regex in the identityConfig
expectation with the existing digitGroupPattern constant, matching the reuse
already established in the surrounding assertions.
- Line 128: Update the doctor source assertion in the identity documentation
contract test to match the identityCheck("skipped" fragment while tolerating
arbitrary whitespace between tokens, rather than requiring the current
indentation. Preserve validation of the same semantic call and its skipped
argument.
---
Outside diff comments:
In `@src/cli/doctor.ts`:
- Around line 421-443: The unavailableIdentityCheck helper currently conflates
failed identity verification with incomplete tool discovery. Add a dedicated
“verification” reason to its reason type and message handling, then update the
catch path after discoveryCompleted in the identity verification flow to pass
“verification”; preserve the existing “startup” and “discovery” messages for
their respective paths.
🪄 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: ba1649e5-f372-4aa8-b8f5-b1f438950a7f
📒 Files selected for processing (8)
src/cli/doctor.tssrc/config/validate-config.tssrc/mcp/server/miftah-server.tstests/config.test.tstests/fixtures/fake-upstream.mjstests/identity-docs-contract.test.tstests/identity-manager.test.tstests/operation-pipeline.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Summary
Security impact
Validation
Implements #21; targeting
developmentintentionally, so the issue will be closed after merge evidence is verified.Summary by CodeRabbit
New Features
Documentation
Bug Fixes