Add on-demand heavy MCP activation commands#110
Add on-demand heavy MCP activation commands#110Citrus086 wants to merge 12 commits intosteipete:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33387017bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/cli/heavy-command.ts
Outdated
| if (active.length > 0) { | ||
| return active; | ||
| } |
There was a problem hiding this comment.
Merge config-backed actives with marker-based actives
listActiveHeavyMcps returns immediately when any marker file exists, so it never checks mcporter.json for additional heavy MCPs in mixed states. If one heavy MCP has a marker and another was manually configured (or lost its marker), heavy list under-reports active entries and heavy deactivate <name> incorrectly refuses to deactivate that config-backed MCP. This contradicts the intended fallback behavior for missing markers and can leave users unable to manage already-configured heavy servers through the CLI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4a4f3e9.
listActiveHeavyMcps now merges marker-backed actives with config-backed actives instead of returning early when markers exist. I also added regression coverage for mixed states so heavy list and heavy deactivate <name> behave correctly when one heavy MCP has a marker and another is only config-backed.
src/heavy/definition.ts
Outdated
| const parsed = JSON.parse(content); | ||
| return parsed as HeavyMcpDefinition; |
There was a problem hiding this comment.
Validate heavy definition JSON before casting
readHeavyMcpDefinition blindly casts parsed JSON to HeavyMcpDefinition without verifying that mcpServers exists and is an object. A malformed <name>.json file (e.g., missing mcpServers) will pass this function but then crash later in handleHeavyActivate/handleHeavyDeactivate/listActiveHeavyMcps when calling Object.entries or Object.keys on undefined, producing a runtime TypeError instead of a clear user-facing validation error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4a4f3e9.
readHeavyMcpDefinition now validates the parsed JSON shape before returning it and throws a clear Invalid heavy MCP definition ... error for malformed files. I also added a regression test covering the malformed-definition case.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a4f3e9d7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/cli/heavy-command.ts
Outdated
| return null; | ||
| } | ||
| const serverNames = Object.keys(definition.mcpServers); | ||
| return serverNames.every((serverName) => configuredServers.has(serverName)) ? name : null; |
There was a problem hiding this comment.
Match heavy definitions by content before marking active
listActiveHeavyMcps treats a heavy definition as active when all of its server names exist in mcporter.json, but it never checks whether those entries actually match the heavy definition. In projects that already define a server with the same key (e.g. a custom playwright config), heavy list will report it as active and heavy deactivate will delete that unrelated server entry, which is destructive config loss for users who never activated the heavy definition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1f699f8.
Config-backed fallback detection now requires an exact definition match, not just matching server names. That prevents same-name custom configs from being reported as active heavy MCPs or removed by heavy deactivate. I also added regression coverage for both list and deactivate in the same-name/custom-config case.
src/cli/heavy-command.ts
Outdated
| const activePath = path.join(paths.activeDir, `${name}.json`); | ||
| const availablePath = path.join(paths.availableDir, `${name}.json`); |
There was a problem hiding this comment.
Validate heavy MCP names before filesystem path joins
The CLI uses the raw <name> argument directly in path construction, so inputs containing path separators (for example ../../mcporter) can escape heavy/active and target arbitrary files. Because symlink creation failures are broadly caught and fallback marker writes are attempted, this can corrupt files like config/mcporter.json instead of just creating an active marker. Restrict names to safe basenames and reject traversal tokens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1f699f8.
Heavy MCP names are now validated before any filesystem path joins, and the same validation is shared by the heavy definition helpers. Unsafe names such as ../../mcporter are rejected up front, and I added a regression test that verifies no config file corruption occurs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f699f88f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/cli/heavy-command.ts
Outdated
| available.map(async (name) => { | ||
| const definition = await readHeavyMcpDefinition(paths.availableDir, name); | ||
| if (!definition) { |
There was a problem hiding this comment.
Skip unrelated invalid definitions when resolving active MCPs
listActiveHeavyMcps loads and validates every JSON under heavy/available, and any parse/validation error is thrown immediately. Since list, activate, and deactivate all call this helper, one malformed unrelated definition (for example heavy/available/broken.json) makes all heavy subcommands fail, including deactivating currently active MCPs. This turns a single bad file into a global outage for heavy management instead of isolating the failure to that definition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bad83e0.
listActiveHeavyMcps now skips unrelated invalid definitions during active-resolution fallback instead of failing the whole command. Targeted operations still validate the requested heavy definition directly, but unrelated broken JSON files no longer block list, activate, or deactivate. I also added regression coverage for activation, listing, and deactivation with an unrelated malformed definition present.
src/cli/heavy-command.ts
Outdated
| } catch { | ||
| // Fallback to creating a marker file if symlink fails (e.g., on Windows) | ||
| await fsPromises.writeFile(activePath, JSON.stringify({ activated: new Date().toISOString() }, null, 2)); |
There was a problem hiding this comment.
Guard symlink fallback to avoid clobbering definition files
The catch-all symlink fallback writes directly to activePath for every symlink error. If activation races and fsPromises.symlink fails with EEXIST, activePath can already be a symlink to the available definition file, and writeFile(activePath, ...) follows that symlink and overwrites the real <name>.json definition with marker content. Handle EEXIST separately (or only fallback on expected Windows permission errors) to prevent definition corruption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bad83e0.
Symlink fallback is no longer catch-all. EEXIST is handled as a no-op race, and the marker-file fallback only runs for explicit symlink-restriction error codes. I also added a regression test that simulates an EEXIST race and verifies the heavy definition file is not overwritten.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bad83e0cea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/cli/heavy-command.ts
Outdated
| const { config, path: configPath } = await loadRawConfig(options); | ||
|
|
||
| // Remove the server(s) from config | ||
| const definition = await readHeavyMcpDefinition(paths.availableDir, name); |
There was a problem hiding this comment.
Handle malformed target definitions during deactivate
If an already-active heavy MCP definition file becomes malformed, handleHeavyDeactivate calls readHeavyMcpDefinition and propagates its validation error, so mcporter heavy deactivate <name> aborts before removing config entries or marker files. This leaves users stuck in an active state they cannot clean up via the command, even though deactivation should be the recovery path for broken definition files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 31dbe81.
Activation now persists marker metadata with the heavy MCP's server names, and deactivation resolves removal targets from that marker before touching the definition file. That means a malformed target definition no longer blocks heavy deactivate <name> as long as the MCP was previously activated. I also added a regression test covering deactivation after corrupting the target definition file.
src/cli/heavy-command.ts
Outdated
| // Definition file was removed, try to remove by name | ||
| delete config.mcpServers?.[name]; |
There was a problem hiding this comment.
Avoid basename-only fallback when definition is missing
When the heavy definition file is missing, deactivation falls back to delete config.mcpServers?.[name], which assumes the file basename matches the configured server key. Valid definitions can contain different key names (or multiple keys), so this path can report Deactivated while leaving one or more heavy server entries in mcporter.json, creating a silent partial deactivation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 31dbe81.
Deactivation no longer falls back to deleting config.mcpServers?.[name]. It now uses persisted marker metadata to remove the exact server keys that were activated, so basename/key mismatches and multi-key definitions are handled correctly even if the definition file is missing. I added a regression test that activates a basename-different, multi-key heavy definition, deletes the definition file, and verifies all activated server entries are removed.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31dbe813c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const [serverName, serverDef] of Object.entries(definition.mcpServers)) { | ||
| config.mcpServers[serverName] = serverDef; |
There was a problem hiding this comment.
Prevent heavy activation from clobbering existing servers
activate blindly assigns each heavy definition entry into config.mcpServers, so an existing server with the same name but different settings is silently overwritten. When that heavy MCP is later deactivated, the command removes those server names outright, which can permanently delete a user's pre-existing custom configuration. This should either reject name collisions or preserve/restore the prior entries.
Useful? React with 👍 / 👎.
src/cli/heavy-command.ts
Outdated
| const active = await listActiveHeavyMcps(paths, options); | ||
| if (active.includes(name)) { | ||
| console.log(`Heavy MCP '${name}' is already active.`); | ||
| return; |
There was a problem hiding this comment.
Reconcile stale markers before reporting “already active”
activate short-circuits on active.includes(name), but activity currently includes any marker filename in heavy/active even if the corresponding servers are no longer present in mcporter.json. In that stale-marker state (for example after manual config edits), mcporter heavy activate <name> becomes a no-op and cannot reapply the heavy definition without an extra manual deactivate step.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32cc689636
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/cli/heavy-command.ts
Outdated
| if (!hasAnyConfiguredServers(config.mcpServers, marker.serverNames)) { | ||
| console.log(`Heavy MCP '${name}' is not active.`); | ||
| return; | ||
| } | ||
| serverNames = marker.serverNames; |
There was a problem hiding this comment.
Guard marker-based deactivate against config drift
When a marker file exists, deactivate only checks whether any listed server name is present and then deletes all marker-listed entries. This can remove user-managed configs after drift: for example, if playwright was activated, then edited to a custom command, heavy list correctly shows it as not active (definition mismatch) but heavy deactivate playwright still deletes that custom entry because the marker exists. The marker path should be validated against current definition equality (or at least require all marker entries to still match heavy-managed state) before deleting.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a988424d0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/cli/heavy-command.ts
Outdated
| if (!hasAllConfiguredServers(config.mcpServers, marker.serverNames)) { | ||
| console.log(`Heavy MCP '${name}' is not active.`); | ||
| return; | ||
| } | ||
| serverNames = marker.serverNames; |
There was a problem hiding this comment.
Prevent deleting drifted configs on definition read failure
When readHeavyMcpDefinition fails during heavy deactivate (for example, a malformed or unreadable <name>.json), this fallback only verifies that the marker's server names exist and then deletes those entries unconditionally. If a user has since customized one of those server configs, running deactivate in this error path will still erase the custom config. This should fail closed (or require a stronger match) instead of deleting by name presence alone.
Useful? React with 👍 / 👎.
src/heavy/definition.ts
Outdated
| } | ||
|
|
||
| const HeavyMcpDefinitionSchema = z.object({ | ||
| mcpServers: z.record(z.string(), RawEntrySchema), |
There was a problem hiding this comment.
Require non-empty heavy definitions
HeavyMcpDefinitionSchema accepts { "mcpServers": {} }, but downstream logic assumes a heavy definition has at least one server: empty definitions are treated as active by vacuous every(...) checks, so heavy list/activate can report an unactivated definition as active and write inconsistent marker metadata. Enforcing at least one server entry in the schema avoids this broken state.
Useful? React with 👍 / 👎.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c32a947fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/cli/heavy-command.ts
Outdated
| if (definition) { | ||
| if (isHeavyMcpDefinitionActiveInConfig(config.mcpServers, definition)) { | ||
| active.add(name); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Fall back to marker snapshot when definition drifts
When a marked heavy MCP still has an available/<name>.json file, this branch returns early and only checks the current definition. If that file is updated after activation (for example, version bump in args), list will report the MCP as inactive even though the marker snapshot and mcporter.json still match the active state. This creates contradictory UX (list says inactive, activate then fails with conflict, while deactivate can still succeed via marker metadata).
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07a9b048f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return Object.entries(definition.mcpServers).every(([serverName, definitionEntry]) => | ||
| isDeepStrictEqual(configuredServers?.[serverName], definitionEntry) | ||
| ); |
There was a problem hiding this comment.
Require exact heavy-server match before treating config as active
isHeavyMcpDefinitionActiveInConfig currently returns true when the configured entry is a superset of the definition, so if a heavy definition is edited after activation to remove one server, deactivate can treat the drifted definition as active, remove only the reduced set, and then delete the marker. This leaves stale heavy servers in mcporter.json with no marker metadata to clean them up later, which is a persistent config-corruption path whenever definitions evolve.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7a1901dc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const activeDefinition = findActiveHeavyDefinition( | ||
| config.mcpServers, | ||
| [markerDefinition, currentDefinition], | ||
| marker.serverNames |
There was a problem hiding this comment.
Stop blocking deactivate on stale marker server names
Passing marker.serverNames as the required server set here makes deactivate reject valid active states when the marker metadata is stale. A reproducible case is: definition originally had servers [A,B], marker keeps that list, definition later shrinks to [A], and config now matches [A]; heavy list still reports it active (from the available definition path), but heavy deactivate <name> prints “not active” and leaves A in config because this check forces a name-set mismatch.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8293a4481a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const serverName of serverNames) { | ||
| delete config.mcpServers?.[serverName]; |
There was a problem hiding this comment.
Preserve shared servers when deactivating heavy profiles
This deletion loop removes every server listed for the profile being deactivated without checking whether another active heavy profile still depends on the same server entry. If two heavy definitions overlap (for example, a bundle profile and a single-server profile both including chrome-devtools), deactivating one profile will silently remove the shared mcpServers entry and break the other profile’s active state. The deactivate path needs overlap-aware ownership checks (or should reject overlapping activations) before deleting shared server keys.
Useful? React with 👍 / 👎.
| await writeRawConfig(configPath, config); | ||
|
|
||
| // Remove active marker | ||
| await fsPromises.unlink(activePath).catch(() => {}); |
There was a problem hiding this comment.
Report marker unlink failures during deactivation
Swallowing all errors here can report a successful deactivation even when the marker file could not be removed (for example EACCES/EPERM on a read-only file). That leaves stale marker state behind and can cause a later activate to fail in writeActiveMarker, which does throw on non-ENOENT unlink errors. Only ENOENT should be ignored; other filesystem errors should be surfaced so users can fix permissions instead of getting inconsistent state.
Useful? React with 👍 / 👎.
|
@copilot review Fixed the shared server preservation issue:
Also verified that marker unlink error handling is already correct (using |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review Fixed the shared server preservation issue:
Also verified that marker unlink error handling is already correct (using |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
This adds a
heavyCLI command group for managing large MCP definitions that should only be loaded when needed.Commands added:
mcporter heavy listmcporter heavy activate <name>mcporter heavy deactivate <name>What changed
heavyinto the top-level CLI routersrc/heavy/module for heavy definition/path handlingsrc/cli/heavy-command.ts<name>.jsonfilesdist-bun/*.tar.gzas binary in.gitattributesto avoid false dirty-state reports from line-ending normalizationBehavior
Heavy MCP definitions live next to the resolved mcporter config:
activatemerges the selected heavy definition intomcporter.json.deactivateremoves the corresponding server entries.listshows available definitions and marks active ones.Active heavy MCPs are detected from both
heavy/active/markers and config-backed definitions already present inmcporter.json, so mixed states still list and deactivate correctly.Verification
Ran:
pnpm exec vitest run tests/heavy-command.test.ts tests/cli-help-shortcuts.test.tspnpm checkMotivation
Some MCP servers, such as
chrome-devtools, have large tool schemas that take up significant context. This makes them a good fit for on-demand activation rather than always-on config.Closes #109