fix(mcp): target OpenClaw workspace config - #8646
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe OpenClaw Mcporter adapter now resolves normalized workspace roots from agent configuration. Registration, inspection, verification, and removal use the resolved root. Registration uses project scope. Tests cover lifecycle behavior, layered configuration, drift protection, recovery paths, and custom workspaces. ChangesOpenClaw Mcporter workspace targeting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OpenClawAdapter
participant AgentConfig
participant Mcporter
OpenClawAdapter->>AgentConfig: resolve configuration directory
AgentConfig-->>OpenClawAdapter: normalized workspace root
OpenClawAdapter->>Mcporter: register with --root and project scope
Mcporter-->>OpenClawAdapter: registration result
OpenClawAdapter->>Mcporter: inspect with --root
Mcporter-->>OpenClawAdapter: server configuration
OpenClawAdapter->>Mcporter: remove owned configuration with --root
Mcporter-->>OpenClawAdapter: removal result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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
🧹 Nitpick comments (1)
src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.ts (1)
38-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert observed mcporter arguments instead of generated shell text.
These assertions inspect command strings with
toContain. They do not prove that the spawnedmcporterprocess receives--rootor that the add command receives--scope project.Use the fake executable to record
process.argv. Execute registration, inspection, and removal. Assert the observed arguments and the resulting workspace configuration.As per path instructions, tests should prove observable behavior through the public boundary instead of source-text shape.
Also applies to: 167-177
🤖 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/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.ts` around lines 38 - 45, Replace the command-string toContain assertions in the registration test with fake-executable argv recording. Exercise registration, inspection, and removal through the public adapter boundary, then assert the recorded mcporter arguments include the expected --root and --scope project values, along with the resulting workspace configuration. Apply the same change to the assertions around the additional referenced section.Source: Path instructions
🤖 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/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.ts`:
- Around line 106-113: Update the fake executable’s config handling in the test
so it locates the "config" argument and branches only on the immediately
following subcommand, rather than using process.argv.includes("get") or
includes("remove"). Preserve the existing get and remove responses while
ensuring server names and option values cannot select either branch.
---
Nitpick comments:
In `@src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.ts`:
- Around line 38-45: Replace the command-string toContain assertions in the
registration test with fake-executable argv recording. Exercise registration,
inspection, and removal through the public adapter boundary, then assert the
recorded mcporter arguments include the expected --root and --scope project
values, along with the resulting workspace configuration. Apply the same change
to the assertions around the additional referenced section.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8900d768-c117-4ee1-ae03-3e3661793485
📒 Files selected for processing (3)
src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.tssrc/lib/actions/sandbox/mcp-bridge-adapter-openclaw.tssrc/lib/actions/sandbox/mcp-bridge-adapter-status.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
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/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.ts`:
- Around line 196-198: Strengthen the config-get assertion in the migration test
by inspecting every matching entry in observedArgs and requiring the complete
argument list to use OPENCLAW_MCPORTER_ROOT, the expected config get subcommand,
server, and --json arguments. Preserve the test’s verification that config get
occurs while ensuring inspect and removal lifecycle paths use the authoritative
replacement root.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 52b69d26-4726-48ee-a703-3559874aeb2c
📒 Files selected for processing (1)
src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/mcp-bridge-status.ts (1)
113-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared agent-config helper in status checks.
src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.tsalready usesgetAgentConfigDir(entry.agent), but this status path still readsloadAgent(entry.agent).configPaths.dirdirectly. Use the shared helper so status and lifecycle operations use one authoritative config-directory resolution path.Proposed change
- openClawMcporterRoot(loadAgent(entry.agent).configPaths.dir), + openClawMcporterRoot(getAgentConfigDir(entry.agent)),Import
getAgentConfigDirfrom./mcp-bridge-state.As per path instructions, all in-scope callers must use one authoritative path during migration.
🤖 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/lib/actions/sandbox/mcp-bridge-status.ts` around lines 113 - 117, Update the status command construction around buildOpenClawMcporterInspectCommand to use getAgentConfigDir(entry.agent) from ./mcp-bridge-state instead of loadAgent(entry).configPaths.dir, adding the required import and preserving the existing openClawMcporterRoot flow.Source: Path instructions
🤖 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.
Nitpick comments:
In `@src/lib/actions/sandbox/mcp-bridge-status.ts`:
- Around line 113-117: Update the status command construction around
buildOpenClawMcporterInspectCommand to use getAgentConfigDir(entry.agent) from
./mcp-bridge-state instead of loadAgent(entry).configPaths.dir, adding the
required import and preserving the existing openClawMcporterRoot flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 831ea71d-f28e-4f59-a38d-e6bd2652f5f9
📒 Files selected for processing (6)
src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.tssrc/lib/actions/sandbox/mcp-bridge-adapter-openclaw.tssrc/lib/actions/sandbox/mcp-bridge-adapter-status.tssrc/lib/actions/sandbox/mcp-bridge-adapters.tssrc/lib/actions/sandbox/mcp-bridge-state.tssrc/lib/actions/sandbox/mcp-bridge-status.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.ts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.ts:96 removes only the project definition selected by --root. Existing NemoClaw releases wrote managed entries to mcporter's home scope. mcporter@0.7.3 merges home and project configurations, but config remove modifies only the resolved project file. After an upgrade or restart creates the project entry, removal deletes that entry and exposes the stale home entry again. NemoClaw can then remove its registry, provider, and policy state while OpenClaw still discovers the server; adding the same name again can also fail. Ownership-check and remove both the project entry and any matching legacy home entry, while preserving a drifted user-owned entry. Add a layered-config regression test that seeds a legacy home entry, registers the project entry, removes it, and proves effective lookup is absent and both owned entries are gone.
|
Cleanup now checks project and legacy home layers before changing either one, removes every owned copy, and verifies the effective entry is gone. Drifted entries remain untouched. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Default XDG cleanup is covered now too: with XDG_CONFIG_HOME unset, the managed entry under ~/.config/mcporter is removed and effective lookup is verified absent. @coderabbitai review |
|
✅ Action performedReview finished.
|
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/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.ts`:
- Around line 254-257: Update the test around
buildOpenClawMcporterInspectCommand to capture the result of runWithoutXdg,
assert its exit status is 0, and then assert the trimmed stdout equals "absent".
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 753fdc40-9516-405e-8eca-1433c89211ca
📒 Files selected for processing (2)
src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.test.tssrc/lib/actions/sandbox/mcp-bridge-adapter-openclaw.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/mcp-bridge-adapter-openclaw.ts
|
The default-XDG inspection now asserts both a successful exit and the expected absent result. @coderabbitai review |
|
✅ Action performedReview finished.
|
3a00e31 to
0ec1259
Compare
|
Rebased on current main. The 89 lifecycle tests and CLI typecheck still pass. @coderabbitai review |
|
|
0ec1259 to
8c488c6
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| fs.writeFileSync( | ||
| configState, | ||
| JSON.stringify({ | ||
| name: "github", | ||
| transport: "http", | ||
| baseUrl: "https://api.githubcopilot.com/mcp/", | ||
| headers: normalizedHeaders, | ||
| }), | ||
| ); |
| fs.writeFileSync( | ||
| homeConfigState, | ||
| JSON.stringify({ | ||
| name: "github", | ||
| transport: "http", | ||
| baseUrl: "https://user.example.test/mcp", | ||
| headers: normalizedHeaders, | ||
| }), | ||
| ); |
Fixes NVIDIA#8326 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
8c488c6 to
0dd3d5a
Compare
|
The test no longer uses check-then-write file assertions; absence is verified from directory contents and presence by direct reads. The 89 lifecycle tests and all pre-push typechecks pass. |
|
Cleanup now checks ownership across project, XDG, and legacy home layers, preserves drifted entries, and removes every matching managed entry. The branch is current with main; 89 lifecycle tests and all pre-push typechecks pass. Could you take another look at 3e450c6? |
3 participants
Summary
Store and inspect OpenClaw HTTP MCP registrations in the sandbox workspace project configuration. This prevents an existing workspace-level
mcporter.jsonentry from taking precedence over NemoClaw's managed HTTP registration and sending the agent down annpxinstallation path.Related Issue
Fixes #8326
Changes
/sandbox/.openclaw/workspaceas the explicit mcporter root for registration, inspection, and removal.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable0dd3d5a26; exact head3e450c631adds only the verified upstream branch update.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Deepak Jain deepujain@gmail.com