fix: use bare madar MCP launcher#502
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Review limit reached
More reviews will be available in 99 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR replaces the version-pinned ChangesMCP stdio server invocation refactor
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 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 `@docs/reference/cli-and-mcp.md`:
- Line 46: Update the sentence to reflect that the installer writes the absolute
project-specific path (constructed with join(projectDir, 'out', 'graph.json'))
into the generated MCP args rather than the literal relative path
"out/graph.json"; mention that the MCP configs call madar serve --stdio with
that absolute path written by the installer (the code that builds the path uses
join and projectDir when assembling the MCP args).
In `@src/infrastructure/install.ts`:
- Around line 1603-1605: The generated installer config must bind Claude/Cursor
to the installer package instance instead of always using PRIMARY_CLI_BIN_NAME;
change how directCliPath is determined so it uses
findPackageCliPath(packageRoot) whenever packageRoot is provided (not only when
isVscode), then set command = directCliPath ? process.execPath :
PRIMARY_CLI_BIN_NAME and args = directCliPath ? [directCliPath, ...cliArgs] :
cliArgs so the produced config references the package-root CLI. Update the
variables directCliPath, command and args accordingly (use findPackageCliPath,
packageRoot, process.execPath, PRIMARY_CLI_BIN_NAME, cliArgs).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 47830f44-e071-4b50-b606-7e169515a578
📒 Files selected for processing (3)
docs/reference/cli-and-mcp.mdsrc/infrastructure/install.tstests/unit/install.test.ts
| const directCliPath = isVscode ? findPackageCliPath(packageRoot) : undefined | ||
| const command = directCliPath ? process.execPath : npxCommand | ||
| const args = directCliPath ? [directCliPath, 'serve', '--stdio', graphPath] : npxArgs | ||
| const command = directCliPath ? process.execPath : PRIMARY_CLI_BIN_NAME | ||
| const args = directCliPath ? [directCliPath, ...cliArgs] : cliArgs |
There was a problem hiding this comment.
Keep Claude/Cursor bound to the installer's CLI, not whatever madar is on PATH.
These lines now ignore packageRoot for non-Copilot targets and always write command: "madar". That means the generated Claude/Cursor config no longer points at the same package instance that ran the install; it will pick up some other madar on PATH, or fail if none is available. That’s a real runtime regression from the previous package-root/version-bound flow.
Proposed fix
- const directCliPath = isVscode ? findPackageCliPath(packageRoot) : undefined
+ const directCliPath = findPackageCliPath(packageRoot)
const command = directCliPath ? process.execPath : PRIMARY_CLI_BIN_NAME
const args = directCliPath ? [directCliPath, ...cliArgs] : cliArgs🤖 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/infrastructure/install.ts` around lines 1603 - 1605, The generated
installer config must bind Claude/Cursor to the installer package instance
instead of always using PRIMARY_CLI_BIN_NAME; change how directCliPath is
determined so it uses findPackageCliPath(packageRoot) whenever packageRoot is
provided (not only when isVscode), then set command = directCliPath ?
process.execPath : PRIMARY_CLI_BIN_NAME and args = directCliPath ?
[directCliPath, ...cliArgs] : cliArgs so the produced config references the
package-root CLI. Update the variables directCliPath, command and args
accordingly (use findPackageCliPath, packageRoot, process.execPath,
PRIMARY_CLI_BIN_NAME, cliArgs).
There was a problem hiding this comment.
I intentionally left this as the bare madar command for Claude/Cursor. The user explicitly asked us to stop writing npx.cmd / version-pinned launchers into the generated repo-local MCP config and to use madar only as the command. Rebinding Claude/Cursor to process.execPath + <packageRoot cli path> would avoid npx, but it would also change the generated contract away from the requested madar launcher. VS Code Copilot keeps its existing direct CLI-path behavior because that integration already had a separate contract before this change.
There was a problem hiding this comment.
@mohanagy, understood — the bare madar launcher for Claude/Cursor is an explicit design choice to move away from version-pinned launchers, with the expectation that madar is available on the user's PATH. VS Code Copilot retains the direct-CLI-path contract because it predates this change and has a different integration model. I'll stand down on this one.
✏️ Learnings added
Learnt from: mohanagy
Repo: mohanagy/madar PR: 502
File: src/infrastructure/install.ts:1603-1605
Timestamp: 2026-06-03T22:17:35.181Z
Learning: In `src/infrastructure/install.ts`, the `installMcpServer` function intentionally uses the bare `madar` binary name (PRIMARY_CLI_BIN_NAME) as the MCP server command for Claude and Cursor targets. This is a deliberate design choice to avoid version-pinned `npx` launchers in generated repo-local MCP configs. VS Code Copilot (target === 'copilot') retains the older direct CLI-path wiring via `findPackageCliPath(packageRoot)` and `process.execPath` because it has a separate pre-existing contract. Do not suggest rebinding Claude/Cursor to `process.execPath + packageRoot CLI path`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mohanagy
Repo: mohanagy/madar PR: 343
File: tests/unit/compare-install-regression-fixture.test.ts:14-35
Timestamp: 2026-05-26T23:30:56.261Z
Learning: In the Madar codebase, "degraded" measurement validity state (install verified but no Madar MCP invocation) is covered by unit tests in `tests/unit/compare-native-agent.test.ts`. The regression fixture in `tests/unit/compare-install-regression-fixture.test.ts` is intentionally scoped to the no-install vs with-install A/B pair only, and also asserts `madar_mcp_call_count` for those two arms.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 participant
Summary
npxlaunchers to baremadar serve --stdionpxusage and repo-local installed MCP configsTesting
Summary by CodeRabbit