fix(mcp): scope artifacts to client sessions#18
Merged
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:
WalkthroughThreads a per-client artifact-root from stdio proxies through daemon ingress into MCP sessions so relative screenshot/PDF paths resolve against the calling client’s artifact root; adds daemon-issued artifact-root tokens, loopback/auth gating, containment/symlink checks, atomic writes, and registry-private session metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Proxy as StdIO_Proxy
participant Ingress as Daemon_Ingress
participant Session as MCP_Session
participant FS as Filesystem
Client->>Proxy: connect (optionally includes `client_artifact_root` + token)
Proxy->>Ingress: WS upgrade (daemon URL may include artifact_root + token)
Ingress->>Ingress: validate token (constant-time), enforce loopback/auth rules
Ingress->>Session: open/create session with pinned artifact_root_context
Session->>FS: resolve_artifact_path → prepare_artifact_destination
Session->>FS: write_artifact_file (temp + rename, revalidate canonical path)
FS-->>Session: success or `tool_error(INVALID_ARGUMENT / TOOL_FAILED)`
Session-->>Client: initialize/response (resolved path or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR scopes screenshot/PDF artifact persistence to the calling MCP client session (rather than the shared daemon’s working directory) by threading a per-connection artifact root through daemon ingress → MCP protocol session → session registry → tool router.
Changes:
- Add per-session
artifact_root/artifact_root_realand use them when persisting screenshot/PDF outputs. - Pass the stdio proxy’s cwd to the daemon via a
client_artifact_rootquery param, and plumb it into session creation. - Add unit/integration tests plus spec/README/changelog updates documenting per-client artifact roots.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/local-multiplexed-browser-mcp-spec.md | Documents per-client artifact persistence and new internal session fields. |
| specs/local-multiplexed-browser-mcp-changelog.md | Changelog entry for per-client artifact roots. |
| local-mcp-bun/src/artifacts.ts | New centralized helpers for root resolution, path resolution, and destination validation. |
| local-mcp-bun/src/tool_router.ts | Persist artifacts relative to the calling session’s artifact root. |
| local-mcp-bun/src/session_registry.ts | Store artifact root context on each new session. |
| local-mcp-bun/src/mcp_protocol_session.ts | Thread artifact root context into open_session for (re)initialization flows. |
| local-mcp-bun/src/daemon_ingress_server.ts | Parse client_artifact_root from connection URL and attach to WS session data. |
| local-mcp-bun/src/stdio_proxy_server.ts | Append client_artifact_root to the daemon URL (defaults to proxy cwd). |
| local-mcp-bun/src/types.ts | Extend agent_session type with artifact root fields. |
| local-mcp-bun/tests/unit/stdio_proxy_server.test.ts | Unit coverage for encoded artifact-root query param handling. |
| local-mcp-bun/tests/integration/parity_remaining.test.ts | Integration coverage for per-session artifact path resolution. |
| local-mcp-bun/tests/integration/daemon_ingress_cleanup.test.ts | Integration coverage for daemon-ingress artifact root handoff + rejection of relative roots. |
| local-mcp-bun/README.md | Documents artifact path behavior in shared-daemon mode. |
| agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md | Implementation/verification notes for the change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+248
to
+262
| private resolve_connection_artifact_root(request_url: URL): artifact_root_context { | ||
| if (!request_url.searchParams.has("client_artifact_root")) { | ||
| return resolve_artifact_root(); | ||
| } | ||
|
|
||
| const raw_artifact_root = request_url.searchParams.get("client_artifact_root")?.trim() ?? ""; | ||
| if (raw_artifact_root.length === 0) { | ||
| throw new Error("client_artifact_root must be a non-empty absolute path"); | ||
| } | ||
|
|
||
| if (!isAbsolute(raw_artifact_root)) { | ||
| throw new Error("client_artifact_root must be an absolute path"); | ||
| } | ||
|
|
||
| return resolve_artifact_root(raw_artifact_root); |
There was a problem hiding this comment.
client_artifact_root is accepted from the request URL and only validated as a non-empty absolute path. Because resolve_artifact_root() + later artifact writes effectively trust this root, a client can scope artifact writes to arbitrary directories (e.g. /) and overwrite files permitted by the daemon user—especially risky if MCP_DAEMON_HOST binds beyond loopback or auth is disabled. Consider restricting client_artifact_root to loopback-only deployments, requiring auth when it is provided, and/or gating this override behind an explicit config flag so remote/unauthenticated ingress can’t expand write scope via a query param.
Greptile SummaryThis PR scopes screenshot and PDF artifact storage to per-client sessions by propagating a client-supplied Confidence Score: 4/5Safe to merge — no P0/P1 issues found; all concerns from previous review threads appear addressed. The change is large (2400+ lines) and touches security-sensitive filesystem and session-scoping paths. No new critical or likely-bugs were found: the atomic write pattern, token validation, boundary checks, and SESSION_NOT_FOUND fix all look correct. Score is capped at 4 rather than 5 due to the complexity and the two P2 observations (redundant parent traversal in post-rename validation, undocumented loopback vs non-loopback resolution asymmetry). local-mcp-bun/src/artifacts.ts (new security-critical module) and local-mcp-bun/src/daemon_ingress_server.ts (resolve_connection_artifact_root asymmetry) deserve the closest review attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Proxy as stdio_proxy_server
participant Ingress as daemon_ingress_server
participant Session as mcp_protocol_session
participant Router as tool_router
participant Registry as session_registry
participant Artifacts as artifacts.ts
Proxy->>Proxy: build_daemon_url_with_client_artifact_root(cwd)
Proxy->>Ingress: WebSocket GET /mcp?client_artifact_root=path
Ingress->>Ingress: validate daemon-issued artifact root credential
Ingress->>Artifacts: resolve_artifact_root(path) loopback-eager or non-loopback-lazy
Ingress->>Session: new mcp_protocol_session(artifact_root_context)
Session->>Router: initialize open_session(client, artifact_root_ctx)
Router->>Artifacts: resolve_artifact_root_context_input(ctx)
Router->>Registry: create_session(client, auth_mode, resolved_ctx)
Registry->>Registry: store record = session + artifact_root_context
Session->>Registry: get_session_artifact_root_context then pin_artifact_root_context
Session->>Router: tools/call browser_take_screenshot
Router->>Artifacts: resolve_artifact_path(session_ctx, requested_path)
Router->>Artifacts: write_artifact_file(ctx, absolute_path, bytes)
Artifacts->>Artifacts: prepare_artifact_destination x3 TOCTOU guards
Artifacts->>Artifacts: open temp file O_CREAT O_EXCL O_NOFOLLOW
Artifacts->>Artifacts: renameSync temp to final
Artifacts->>Artifacts: validate_written_artifact_file post-rename check
Prompt To Fix All With AIThis is a comment left during a code review.
Path: local-mcp-bun/src/artifacts.ts
Line: 281-301
Comment:
**Post-rename `prepare_artifact_destination` re-validates unrenamed path**
`validate_written_artifact_file` calls `prepare_artifact_destination(root_context, absolute_path, ...)` (line 287) after the rename has already placed the file. On Linux `anchored_path` is `/proc/self/fd/${fd}/${destination_name}`, while `absolute_path` is the original logical destination. `prepare_artifact_destination` will call `prepare_artifact_parent_directory` on `dirname(absolute_path)`, which traverses and revalidates every parent segment via `validate_artifact_parent_segment`. This is a repeated traversal of potentially many segments (one `lstatSync` + `realpathSync` per segment) solely for the post-write security check.
A narrower post-rename check — just `lstatSync(absolute_path)` (symlink guard) followed by the two `realpathSync` comparisons — would give the same TOCTOU protection with less filesystem I/O and without triggering the parent-segment loop a fourth time.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: local-mcp-bun/src/daemon_ingress_server.ts
Line: 261-291
Comment:
**Loopback vs non-loopback artifact-root resolution asymmetry**
For loopback connections, `resolve_artifact_root(raw_artifact_root)` is called eagerly at HTTP-upgrade time (line 290), so an invalid path immediately returns HTTP 400. For non-loopback connections, the same call is wrapped in a thunk `() => resolve_artifact_root(raw_artifact_root)` (line 287) and deferred until the `initialize` RPC, where the error surfaces as a JSON-RPC `-32603` internal error rather than a connection-level 400.
This means non-loopback clients see a confusing protocol-level error instead of a rejected upgrade. A brief inline comment explaining that deferred resolution is intentional here — so auth is checked before validating the path — would make the design intent clear to future maintainers.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: local-mcp-bun/src/session_registry.ts
Line: 57-61
Comment:
**Default parameter calls `resolve_default_artifact_root()` on every invocation**
The default value `artifact_root_context: artifact_root_context = resolve_default_artifact_root()` is evaluated at each call site when the argument is omitted. `resolve_default_artifact_root()` calls `realpathSync(process.cwd())` (with a `tmpdir()` fallback), so every `create_session` call that skips the parameter incurs a synchronous filesystem stat. In practice this is only hit from tests and the internal `open_authenticated_session` path (which always passes the context), but a lazy closure or explicit `undefined` guard would make the cost opt-in rather than implicit.
How can I resolve this? If you propose a fix, please make it concise.Reviews (32): Last reviewed commit: "fix(mcp): scope artifacts to client sess..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
local-mcp-bun/src/tool_router.ts (1)
2052-2058: Consider preserving the original error code for tool_error instances.When
prepare_artifact_destinationthrows atool_error(e.g., for symlink or boundary violations), wrapping it in a newINVALID_ARGUMENTerror loses the original error code context. The caller seesINVALID_ARGUMENTfor both path validation failures and filesystem I/O errors.♻️ Optional: preserve original tool_error codes
try { prepare_artifact_destination(artifact_root_context, absolute_path, requested_path); await Bun.write(absolute_path, bytes); } catch (error) { + if (error instanceof tool_error) { + throw error; + } const message = error instanceof Error ? error.message : String(error); throw new tool_error("INVALID_ARGUMENT", `failed to persist ${tool_name} to ${absolute_path}`, false, { path: absolute_path, cause: message, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/tool_router.ts` around lines 2052 - 2058, The catch block in the persist flow is converting every error into a new tool_error with code "INVALID_ARGUMENT", losing original tool_error codes from prepare_artifact_destination (e.g., symlink/boundary errors); update the catch in the persist function that wraps errors into tool_error so that if the caught error is an instance of tool_error you either rethrow it or construct the new tool_error using the caught error's code (error.code) and preserve its metadata (path/cause) while still adding context (e.g., "failed to persist ..."); reference the existing catch handling around prepare_artifact_destination and the tool_error constructor to locate where to apply this conditional-preserve logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@local-mcp-bun/src/tool_router.ts`:
- Around line 2052-2058: The catch block in the persist flow is converting every
error into a new tool_error with code "INVALID_ARGUMENT", losing original
tool_error codes from prepare_artifact_destination (e.g., symlink/boundary
errors); update the catch in the persist function that wraps errors into
tool_error so that if the caught error is an instance of tool_error you either
rethrow it or construct the new tool_error using the caught error's code
(error.code) and preserve its metadata (path/cause) while still adding context
(e.g., "failed to persist ..."); reference the existing catch handling around
prepare_artifact_destination and the tool_error constructor to locate where to
apply this conditional-preserve logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c6a2f56-254f-47c8-8ed3-b340933c83f6
📒 Files selected for processing (14)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/src/types.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/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: 9cd5687953
ℹ️ 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".
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
9cd5687 to
4ebe2ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
local-mcp-bun/src/stdio_proxy_server.ts (1)
18-33: Normalizeartifact_rootbefore attaching it to the daemon URL.
build_daemon_url_with_client_artifact_rootis exported; normalizing here avoids accidental relative-path propagation from future callers.♻️ Suggested hardening
+import { resolve as resolve_path } from "node:path"; import { is_loopback_host } from "./config"; @@ export function build_daemon_url_with_client_artifact_root( daemon_url: string, artifact_root = process.cwd(), attach_client_artifact_root?: boolean, ): string { @@ - url.searchParams.set("client_artifact_root", artifact_root); + url.searchParams.set("client_artifact_root", resolve_path(artifact_root)); return url.toString(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/stdio_proxy_server.ts` around lines 18 - 33, The function build_daemon_url_with_client_artifact_root should normalize the artifact_root before attaching it to the URL to prevent propagating relative paths; update the function to resolve/normalize artifact_root (e.g., using path.resolve or path.normalize, or fs.realpathSync if you want canonical paths) right after the parameter is read (and before url.searchParams.set), and then set the "client_artifact_root" query param to that normalized absolute path; ensure you add the required import for the path/fs utility you choose and keep the early returns (url.toString()) unchanged.local-mcp-bun/tests/unit/stdio_proxy_server.test.ts (1)
5-49: Consider table-driven tests to reduce repetition.The setup/assert pattern repeats across cases; a small table-driven structure would make future edge-case additions cheaper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/tests/unit/stdio_proxy_server.test.ts` around lines 5 - 49, Convert the repeated test cases in stdio_proxy_server.test.ts into a table-driven loop that iterates over an array of test cases and asserts the same expectations against build_daemon_url_with_client_artifact_root outputs; for each case include fields like name, inputUrl, artifactRoot (use resolve(...) in the case setup), allowRemote (boolean or omitted), and expected values (e.g., expectedPathname, expectedQueryPairs or expectedClientArtifactRoot), then replace the individual test(...) blocks with a single test.each or forEach that constructs daemon_url via build_daemon_url_with_client_artifact_root(inputUrl, artifactRoot, allowRemote), parses it with new URL(daemon_url) and performs the same pathname and searchParams assertions per case. Ensure the case that supplies an already-encoded client_artifact_root uses encodeURIComponent in the inputUrl and that the explicit remote/allowed flags are represented in the table so all original behaviors are preserved.agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md (1)
31-33: Minor readability nit: repeated bullet prefix.Three consecutive bullets start with “PR review follow-up:”; varying the prefix slightly would improve scanability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md` around lines 31 - 33, The three consecutive list items all start with the same prefix "PR review follow-up:" which reduces scanability; update the second and third bullets to use varied prefixes (e.g., "Lint/Checks:", "Tests:" or "CI notes:") or remove the duplicate prefix so only the first bullet keeps "PR review follow-up:" and the following lines list the commands `bun run --cwd local-mcp-bun lint:spec`/`lint:docs`/`check:popup-ui` and the test commands (`bun test ...`, `bun run --cwd local-mcp-bun test:unit`/`test:integration`/`test:hard-gate`) without repeating the same leading phrase—locate the three lines containing the literal "PR review follow-up:" and change their prefixes accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/local-multiplexed-browser-mcp-spec.md`:
- Line 158: Replace the phrase "loopback-only" with the hyphenated compound
adjective "localhost-only" in the FR-011 requirement text (the "Local Security
Boundary" entry) so the row reads "...MUST bind the browser bridge
localhost-only, SHOULD support..." ensuring the readability change is applied to
that requirement string (FR-011 / "Local Security Boundary").
---
Nitpick comments:
In `@agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md`:
- Around line 31-33: The three consecutive list items all start with the same
prefix "PR review follow-up:" which reduces scanability; update the second and
third bullets to use varied prefixes (e.g., "Lint/Checks:", "Tests:" or "CI
notes:") or remove the duplicate prefix so only the first bullet keeps "PR
review follow-up:" and the following lines list the commands `bun run --cwd
local-mcp-bun lint:spec`/`lint:docs`/`check:popup-ui` and the test commands
(`bun test ...`, `bun run --cwd local-mcp-bun
test:unit`/`test:integration`/`test:hard-gate`) without repeating the same
leading phrase—locate the three lines containing the literal "PR review
follow-up:" and change their prefixes accordingly.
In `@local-mcp-bun/src/stdio_proxy_server.ts`:
- Around line 18-33: The function build_daemon_url_with_client_artifact_root
should normalize the artifact_root before attaching it to the URL to prevent
propagating relative paths; update the function to resolve/normalize
artifact_root (e.g., using path.resolve or path.normalize, or fs.realpathSync if
you want canonical paths) right after the parameter is read (and before
url.searchParams.set), and then set the "client_artifact_root" query param to
that normalized absolute path; ensure you add the required import for the
path/fs utility you choose and keep the early returns (url.toString())
unchanged.
In `@local-mcp-bun/tests/unit/stdio_proxy_server.test.ts`:
- Around line 5-49: Convert the repeated test cases in
stdio_proxy_server.test.ts into a table-driven loop that iterates over an array
of test cases and asserts the same expectations against
build_daemon_url_with_client_artifact_root outputs; for each case include fields
like name, inputUrl, artifactRoot (use resolve(...) in the case setup),
allowRemote (boolean or omitted), and expected values (e.g., expectedPathname,
expectedQueryPairs or expectedClientArtifactRoot), then replace the individual
test(...) blocks with a single test.each or forEach that constructs daemon_url
via build_daemon_url_with_client_artifact_root(inputUrl, artifactRoot,
allowRemote), parses it with new URL(daemon_url) and performs the same pathname
and searchParams assertions per case. Ensure the case that supplies an
already-encoded client_artifact_root uses encodeURIComponent in the inputUrl and
that the explicit remote/allowed flags are represented in the table so all
original behaviors are preserved.
🪄 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: CHILL
Plan: Pro
Run ID: f6893d98-5f6d-4705-824f-fdc60b1e99b7
📒 Files selected for processing (15)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/src/types.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (1)
- specs/local-multiplexed-browser-mcp-changelog.md
🚧 Files skipped from review as they are similar to previous changes (3)
- local-mcp-bun/README.md
- local-mcp-bun/src/artifacts.ts
- local-mcp-bun/tests/integration/daemon_ingress_cleanup.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ebe2eabae
ℹ️ 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 proxy_server = new stdio_proxy_server({ | ||
| daemon_url: `ws://${options.daemon_host}:${options.daemon_port}/mcp`, | ||
| connect_timeout_ms: options.daemon_connect_timeout_ms, | ||
| attach_client_artifact_root: is_loopback_host(options.daemon_host) || daemon_health.auth_enabled, |
There was a problem hiding this comment.
start_proxy_runtime decides whether to append client_artifact_root using options.daemon_host, but ingress acceptance is enforced against the daemon's own bind host (daemon_ingress_server.resolve_connection_artifact_root). If a shared daemon is bound to 0.0.0.0 without auth but a proxy connects through 127.0.0.1, this line forces attach_client_artifact_root=true, the proxy always sends the query param, and the daemon rejects the WebSocket upgrade with a 400. That creates a hard connection failure for this deployment shape instead of falling back to daemon-cwd artifacts.
Useful? React with 👍 / 👎.
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
4ebe2ea to
1930808
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md (1)
32-34: Consider varying repeated “PR review follow-up” bullet prefixes.Small docs readability nit: three adjacent bullets begin with the same phrase, which makes scanning harder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md` around lines 32 - 34, The three adjacent bullets all start with the identical prefix "PR review follow-up:" which reduces scanability; update those bullet prefixes to be varied and descriptive (for example change the first to "Tests (unit/integration):", the second to "Lint/Docs/Checks:", and the third to "Test suites / hard-gate:"), keeping the same file/command text intact so readers can quickly identify which entry is which and improve readability.local-mcp-bun/src/stdio_proxy_server.ts (1)
9-16: Avoid reparsingdaemon_urlin attach-decision path.
build_daemon_url_with_client_artifact_rootalready constructsURL, butshould_attach_client_artifact_rootreparses the same string. Passing hostname (or URL) keeps this logic single-source and slightly simpler.♻️ Suggested refactor
-function should_attach_client_artifact_root(daemon_url: string, configured?: boolean): boolean { +function should_attach_client_artifact_root(daemon_host: string, configured?: boolean): boolean { if (typeof configured === "boolean") { return configured; } - const url = new URL(daemon_url); - return is_loopback_host(url.hostname); + return is_loopback_host(daemon_host); } export function build_daemon_url_with_client_artifact_root( daemon_url: string, artifact_root = process.cwd(), attach_client_artifact_root?: boolean, ): string { const url = new URL(daemon_url); if (url.searchParams.has("client_artifact_root")) { return url.toString(); } - if (!should_attach_client_artifact_root(daemon_url, attach_client_artifact_root)) { + if (!should_attach_client_artifact_root(url.hostname, attach_client_artifact_root)) { return url.toString(); }Also applies to: 23-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/stdio_proxy_server.ts` around lines 9 - 16, The helper should_attach_client_artifact_root currently reparses daemon_url; instead change its API to accept the already-constructed URL or hostname (e.g., accept a URL object or hostname string) and use that to call is_loopback_host without calling new URL(...). Update all call sites that pass build_daemon_url_with_client_artifact_root's result to pass the URL.hostname or the URL object directly and remove the duplicate parsing; apply the same refactor to the similar helper variant used elsewhere so the attach-decision logic is driven from a single source of truth (build_daemon_url_with_client_artifact_root -> should_attach_client_artifact_root).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md`:
- Around line 32-34: The three adjacent bullets all start with the identical
prefix "PR review follow-up:" which reduces scanability; update those bullet
prefixes to be varied and descriptive (for example change the first to "Tests
(unit/integration):", the second to "Lint/Docs/Checks:", and the third to "Test
suites / hard-gate:"), keeping the same file/command text intact so readers can
quickly identify which entry is which and improve readability.
In `@local-mcp-bun/src/stdio_proxy_server.ts`:
- Around line 9-16: The helper should_attach_client_artifact_root currently
reparses daemon_url; instead change its API to accept the already-constructed
URL or hostname (e.g., accept a URL object or hostname string) and use that to
call is_loopback_host without calling new URL(...). Update all call sites that
pass build_daemon_url_with_client_artifact_root's result to pass the
URL.hostname or the URL object directly and remove the duplicate parsing; apply
the same refactor to the similar helper variant used elsewhere so the
attach-decision logic is driven from a single source of truth
(build_daemon_url_with_client_artifact_root ->
should_attach_client_artifact_root).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 273f2d08-5ccd-49ce-b60b-ad091c7fb093
📒 Files selected for processing (15)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/src/types.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (3)
- local-mcp-bun/src/types.ts
- local-mcp-bun/tests/unit/stdio_proxy_server.test.ts
- local-mcp-bun/src/artifacts.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- specs/local-multiplexed-browser-mcp-changelog.md
- local-mcp-bun/src/daemon_controller.ts
- local-mcp-bun/src/session_registry.ts
- local-mcp-bun/README.md
- local-mcp-bun/src/daemon_ingress_server.ts
- local-mcp-bun/tests/integration/daemon_ingress_cleanup.test.ts
- specs/local-multiplexed-browser-mcp-spec.md
- local-mcp-bun/src/tool_router.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1930808bd8
ℹ️ 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 proxy_server = new stdio_proxy_server({ | ||
| daemon_url: `ws://${options.daemon_host}:${options.daemon_port}/mcp`, | ||
| connect_timeout_ms: options.daemon_connect_timeout_ms, | ||
| attach_client_artifact_root: is_loopback_host(daemon_health.daemon_host) || daemon_health.auth_enabled, |
There was a problem hiding this comment.
This forces attach_client_artifact_root on whenever daemon_health.auth_enabled is true, which makes the proxy append its local process.cwd() even when connecting to a daemon on another host (or through a remote proxy). On connect, ingress validates client_artifact_root against the daemon filesystem (resolve_connection_artifact_root -> resolve_artifact_root), so a client-local path that does not exist remotely causes a 400 upgrade failure and the proxy cannot start. Restrict automatic attachment to loopback daemon URLs (or require explicit opt-in) so authenticated non-loopback deployments do not hard-fail due to mismatched filesystems.
Useful? React with 👍 / 👎.
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
1930808 to
e6da265
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
local-mcp-bun/tests/unit/daemon_spawn_target.test.ts (1)
68-75: Consider adding a test forMCP_ATTACH_CLIENT_ARTIFACT_ROOT="1".The implementation in
daemon_controller.ts(line 118) accepts both"1"and"true"for the force flag. Adding a test case for"1"would ensure both accepted values are covered.💡 Suggested additional test case
test("supports explicit non-loopback artifact-root attachment opt-in", () => { expect( should_attach_client_artifact_root_to_daemon( { daemon_host: "0.0.0.0" }, { MCP_ATTACH_CLIENT_ARTIFACT_ROOT: "true" }, ), ).toBe(true); + expect( + should_attach_client_artifact_root_to_daemon( + { daemon_host: "0.0.0.0" }, + { MCP_ATTACH_CLIENT_ARTIFACT_ROOT: "1" }, + ), + ).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/tests/unit/daemon_spawn_target.test.ts` around lines 68 - 75, Add a unit test that mirrors the existing "true" case but uses MCP_ATTACH_CLIENT_ARTIFACT_ROOT: "1" to ensure the force flag accepted by should_attach_client_artifact_root_to_daemon handles the numeric string variant; update the test suite in daemon_spawn_target.test.ts to call should_attach_client_artifact_root_to_daemon with { daemon_host: "0.0.0.0" } and env { MCP_ATTACH_CLIENT_ARTIFACT_ROOT: "1" } and assert the result is true so both "1" and "true" code paths are covered.local-mcp-bun/src/stdio_proxy_server.ts (1)
50-54: Please confirm the living-spec status update for this implementation change.This PR modifies
local-mcp-bun/implementation paths, but I don’t see a corresponding living-spec status update in the provided diff context.As per coding guidelines:
local-mcp-bun/**/*: Pull requests that modify implementation paths under local-mcp-bun/ SHOULD include corresponding living-spec status updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/stdio_proxy_server.ts` around lines 50 - 54, This change updates how daemon paths are constructed via build_daemon_url_with_client_artifact_root (affecting this.daemon_url) but lacks the required living-spec status update; add or update the living-spec status entry for the local-mcp-bun implementation change to reflect the new client artifact root behavior (include the spec identifier, change summary, and status token) so the repository living-spec for local-mcp-bun/**/* is in sync with the code change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@local-mcp-bun/src/stdio_proxy_server.ts`:
- Around line 50-54: This change updates how daemon paths are constructed via
build_daemon_url_with_client_artifact_root (affecting this.daemon_url) but lacks
the required living-spec status update; add or update the living-spec status
entry for the local-mcp-bun implementation change to reflect the new client
artifact root behavior (include the spec identifier, change summary, and status
token) so the repository living-spec for local-mcp-bun/**/* is in sync with the
code change.
In `@local-mcp-bun/tests/unit/daemon_spawn_target.test.ts`:
- Around line 68-75: Add a unit test that mirrors the existing "true" case but
uses MCP_ATTACH_CLIENT_ARTIFACT_ROOT: "1" to ensure the force flag accepted by
should_attach_client_artifact_root_to_daemon handles the numeric string variant;
update the test suite in daemon_spawn_target.test.ts to call
should_attach_client_artifact_root_to_daemon with { daemon_host: "0.0.0.0" } and
env { MCP_ATTACH_CLIENT_ARTIFACT_ROOT: "1" } and assert the result is true so
both "1" and "true" code paths are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d44a7cc6-95f4-4444-ba41-9b185f1730d0
📒 Files selected for processing (16)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/src/types.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (1)
- specs/local-multiplexed-browser-mcp-changelog.md
🚧 Files skipped from review as they are similar to previous changes (5)
- local-mcp-bun/src/types.ts
- local-mcp-bun/src/session_registry.ts
- local-mcp-bun/README.md
- local-mcp-bun/src/daemon_ingress_server.ts
- specs/local-multiplexed-browser-mcp-spec.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6da265c34
ℹ️ 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".
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
e6da265 to
d2970fe
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md (1)
34-42: Consider consolidating repetitive verification entries.The verification section documents thorough testing across multiple review loops, which is excellent. However, the repetitive sentence structure flagged by static analysis could be improved for readability.
📝 Optional formatting improvement
Consider grouping by review iteration or using a more scannable format:
## Verification ### Initial Implementation - `bun test local-mcp-bun/tests/unit/stdio_proxy_server.test.ts ...` passed. - `bun run --cwd local-mcp-bun lint:spec` passed. - `LOCAL_MCP_TEST_BRIDGE_PORT=48777 bun run --cwd local-mcp-bun test:hard-gate` passed (port collision resolved). ### Review Loop 1 (PR follow-up) - Unit/integration tests: passed. - Linting (spec, docs, popup-ui): passed. - Hard-gate tests: passed. ### Review Loop 2 - `bun test local-mcp-bun/tests/unit/stdio_proxy_server.test.ts ...` passed. - Linting and hard-gate: passed. ...This groups related commands and reduces the repetitive sentence prefix pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md` around lines 34 - 42, Consolidate the repetitive verification bullets in the Verification section by grouping entries per review iteration and summarizing repeated commands (e.g., the lines beginning with "- PR review follow-up:", "- Second review loop:", "- Third review loop:", and "- Fourth review loop:") into a single block per iteration; replace repeated full command sentences with concise summaries like "unit/integration tests: passed", "lint: spec/docs/popup-ui: passed", and "hard-gate: passed (LOCAL_MCP_TEST_BRIDGE_PORT=48777 when applicable)" so the verification remains complete but more scannable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.md`:
- Around line 34-42: Consolidate the repetitive verification bullets in the
Verification section by grouping entries per review iteration and summarizing
repeated commands (e.g., the lines beginning with "- PR review follow-up:", "-
Second review loop:", "- Third review loop:", and "- Fourth review loop:") into
a single block per iteration; replace repeated full command sentences with
concise summaries like "unit/integration tests: passed", "lint:
spec/docs/popup-ui: passed", and "hard-gate: passed
(LOCAL_MCP_TEST_BRIDGE_PORT=48777 when applicable)" so the verification remains
complete but more scannable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83e69893-9442-4a6c-913a-291a78de4884
📒 Files selected for processing (16)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/src/types.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (1)
- local-mcp-bun/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- local-mcp-bun/tests/unit/stdio_proxy_server.test.ts
- local-mcp-bun/src/daemon_controller.ts
- local-mcp-bun/src/mcp_protocol_session.ts
- specs/local-multiplexed-browser-mcp-spec.md
- local-mcp-bun/README.md
- specs/local-multiplexed-browser-mcp-changelog.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2970fedb2
ℹ️ 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".
| daemon_health: Pick<daemon_health_snapshot, "daemon_host">, | ||
| env: Record<string, string | undefined>, | ||
| ): boolean { | ||
| return is_loopback_host(daemon_health.daemon_host) || should_force_attach_client_artifact_root(env); |
There was a problem hiding this comment.
should_attach_client_artifact_root_to_daemon always enables client_artifact_root when the daemon reports a loopback bind host, which makes loopback-tunneled remote daemons (for example via SSH port-forwarding) send the proxy machine’s local process.cwd() to the remote ingress with no way to disable it. In that deployment, ingress resolves client_artifact_root on the remote filesystem and can reject the WebSocket upgrade with 400 when the local path does not exist remotely, so proxy startup fails even though the tunnel itself is healthy.
Useful? React with 👍 / 👎.
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
d2970fe to
2ca4722
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ca472267b
ℹ️ 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".
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
2ca4722 to
1fb218c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fb218c171
ℹ️ 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".
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
1fb218c to
3c63b99
Compare
There was a problem hiding this comment.
💡 Codex Review
Redact artifact-root token from connection error messages
After this change, this.daemon_url includes client_artifact_root and client_artifact_root_token, but connection failures embed the full URL in thrown errors. Those errors are surfaced to stderr/logs, so transient daemon connectivity failures can leak the capability token (and local workspace path) to any log reader, weakening the token gate introduced for client_artifact_root.
ℹ️ 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".
Comment on lines
+436
to
+437
| if (attach_client_artifact_root && !artifact_root_token) { | ||
| throw new Error("daemon state is missing artifact root token; restart the shared daemon"); |
There was a problem hiding this comment.
When loopback attachment is enabled by default, this hard throw makes proxy startup fail even if /health already confirmed the daemon is reachable. Any deployment where the state file is missing or mismatched (for example, MCP_DAEMON_STATE_PATH differs between daemon/proxy, or tmp-state metadata is cleaned up) becomes a full connection outage instead of a degraded fallback to daemon-cwd artifact writes. Consider disabling attach_client_artifact_root when the token is unavailable rather than aborting startup.
Useful? React with 👍 / 👎.
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)
local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts (1)
264-268:⚠️ Potential issue | 🟡 MinorThe poll still times out after 15s.
This change only increases Bun's overall test timeout.
wait_for_condition(..., 15_000, 250)still fails after 15 seconds, so slower daemon shutdowns will keep flaking unless the inner deadline moves too.Suggested direction
- }, 15_000, 250); -}, 20_000); + }, 20_000, 250); +}, 25_000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts` around lines 264 - 268, The inner poll timeout passed to wait_for_condition is still 15_000ms causing the poll to fail before the test-level timeout; update the call to wait_for_condition(…, <newDeadline>, 250) so the inner deadline equals or exceeds the test timeout (e.g., change 15_000 to 20_000 or a shared constant) so fetch_daemon_health(daemon_port) is allowed enough time for slower daemon shutdowns to return undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@local-mcp-bun/src/daemon_controller.ts`:
- Around line 494-517: The daemon state is being written only after creating
daemon_ingress_server (which begins serving immediately), creating a race where
/health can succeed before state is persisted; move the call to
write_daemon_state(options.daemon_state_path, state) so the state file is
persisted before the ingress listener is exposed (i.e., call
write_daemon_state(...) prior to new daemon_ingress_server(...)), or
alternatively modify daemon_ingress_server construction to delay binding until
after write_daemon_state; update any references that assume ingress exists
(e.g., the on_idle_timeout closure using ingress?.stop and runtime.stop) to keep
behavior unchanged.
- Around line 508-514: The on_idle_timeout callback currently updates
state.updated_at and calls write_daemon_state which, if it throws, prevents
ingress.stop(), runtime.stop(), and process.exit(). Make the state write
best-effort by wrapping the write_daemon_state call in a try/catch (or move it
into a try block with teardown in a finally) so that any error is caught/logged
but does not abort execution; ensure ingress.stop() and runtime.stop() run (and
await) in the finally (or after the catch) and that process.exit(0) is always
invoked to guarantee shutdown. Keep references: on_idle_timeout,
state.updated_at, write_daemon_state, ingress.stop, runtime.stop, and
options.daemon_state_path.
---
Outside diff comments:
In `@local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts`:
- Around line 264-268: The inner poll timeout passed to wait_for_condition is
still 15_000ms causing the poll to fail before the test-level timeout; update
the call to wait_for_condition(…, <newDeadline>, 250) so the inner deadline
equals or exceeds the test timeout (e.g., change 15_000 to 20_000 or a shared
constant) so fetch_daemon_health(daemon_port) is allowed enough time for slower
daemon shutdowns to return undefined.
🪄 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: CHILL
Plan: Pro
Run ID: cedc426f-0255-44ff-b608-3a4e87d292a2
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (1)
- local-mcp-bun/src/session_registry.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- local-mcp-bun/src/stdio_proxy_server.ts
- specs/local-multiplexed-browser-mcp-spec.md
- local-mcp-bun/src/artifacts.ts
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
6442eb7 to
78323aa
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts (1)
264-268:⚠️ Potential issue | 🟡 MinorExtend the polling window, not just the test budget.
Line 267 still caps
wait_for_condition(...)at15_000, so this test will keep failing early even though the outer Bun timeout is now20_000. If the daemon shutdown path is what got slower, the extra 5 seconds never gets used.Minimal fix
await wait_for_condition(async () => { const health = await fetch_daemon_health(daemon_port); return typeof health === "undefined"; - }, 15_000, 250); + }, 20_000, 250); }, 20_000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts` around lines 264 - 268, The test's internal polling timeout in wait_for_condition(...) is shorter than the outer test timeout, so extend the polling window to match the outer timeout: update the wait_for_condition call that uses fetch_daemon_health(daemon_port) to use 20_000 (or the same value as the outer test timeout) instead of 15_000 so the extra time is actually available for the daemon shutdown path to complete.
🧹 Nitpick comments (1)
local-mcp-bun/src/session_registry.ts (1)
50-55: Centralizeartifact_root_contextcloning.This manual copy now exists here and again in
local-mcp-bun/src/mcp_protocol_session.tsLines 23-34. Ifartifact_root_contextpicks up another field, one side will silently strip it. Please move the clone helper into./artifactsand reuse it from both call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/session_registry.ts` around lines 50 - 55, The clone_artifact_root_context function duplicates logic present in mcp_protocol_session (lines 23-34); move this helper into the shared ./artifacts module and update both callers to import and use it so future fields aren’t silently dropped. Specifically, create a single exported cloneArtifactRootContext (or reuse the existing name) in ./artifacts, copy the current implementation there, then replace the local clone_artifact_root_context in session_registry.ts and the ad-hoc copier in mcp_protocol_session.ts with imports that call the shared function (update any type names to match existing artifact_root_context type exports).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts`:
- Around line 264-268: The test's internal polling timeout in
wait_for_condition(...) is shorter than the outer test timeout, so extend the
polling window to match the outer timeout: update the wait_for_condition call
that uses fetch_daemon_health(daemon_port) to use 20_000 (or the same value as
the outer test timeout) instead of 15_000 so the extra time is actually
available for the daemon shutdown path to complete.
---
Nitpick comments:
In `@local-mcp-bun/src/session_registry.ts`:
- Around line 50-55: The clone_artifact_root_context function duplicates logic
present in mcp_protocol_session (lines 23-34); move this helper into the shared
./artifacts module and update both callers to import and use it so future fields
aren’t silently dropped. Specifically, create a single exported
cloneArtifactRootContext (or reuse the existing name) in ./artifacts, copy the
current implementation there, then replace the local clone_artifact_root_context
in session_registry.ts and the ad-hoc copier in mcp_protocol_session.ts with
imports that call the shared function (update any type names to match existing
artifact_root_context type exports).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a540c6ee-bd45-457f-831f-91c792eaec72
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (1)
- local-mcp-bun/tests/integration/parity_remaining.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- local-mcp-bun/tests/unit/session_registry.test.ts
- local-mcp-bun/README.md
- specs/local-multiplexed-browser-mcp-spec.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78323aaf95
ℹ️ 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".
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
78323aa to
6325971
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
local-mcp-bun/src/tool_router.ts (1)
2074-2085:⚠️ Potential issue | 🟠 MajorUse
TOOL_FAILEDfor raw filesystem write failures.
write_artifact_file(...)still throws plain I/O errors after validation. Recasting all non-tool_errorfailures asINVALID_ARGUMENTtells the client theirpathis wrong when the server actually failed to write the file.🛠️ Proposed fix
try { await write_artifact_file(artifact_root_context, absolute_path, requested_path, bytes); } catch (error) { if (error instanceof tool_error) { throw error; } const message = error instanceof Error ? error.message : String(error); - throw new tool_error("INVALID_ARGUMENT", `failed to persist ${tool_name} to ${absolute_path}`, false, { + throw new tool_error("TOOL_FAILED", `failed to persist ${tool_name} to ${absolute_path}`, false, { path: absolute_path, cause: message, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/tool_router.ts` around lines 2074 - 2085, The current catch in the write flow treats any non-tool_error as INVALID_ARGUMENT, which misreports filesystem write failures; update the catch block handling for errors thrown by write_artifact_file (function name) so that if error is not an instance of tool_error you rethrow a new tool_error with code "TOOL_FAILED" (instead of "INVALID_ARGUMENT"), keep the message `failed to persist ${tool_name} to ${absolute_path}`, include the path and the original error message as cause, and preserve existing behavior for tool_error instances.
🧹 Nitpick comments (2)
local-mcp-bun/README.md (1)
1-4: Consider updating the living-spec for artifact root scoping.This PR introduces significant artifact root scoping behavior. Per coding guidelines, PRs modifying
local-mcp-bun/paths should include corresponding living-spec status updates inspecs/local-multiplexed-browser-mcp-spec.mdorspecs/local-multiplexed-browser-mcp-changelog.md.As per coding guidelines: "Pull requests that modify implementation paths under local-mcp-bun/ SHOULD include corresponding living-spec status updates".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/README.md` around lines 1 - 4, The PR adds artifact root scoping changes under local-mcp-bun but does not update the living-spec; update either specs/local-multiplexed-browser-mcp-spec.md (preferred) or specs/local-multiplexed-browser-mcp-changelog.md to record the new behavior: add a short section describing the artifact root scoping semantics, list the specific changes introduced by this PR (behavior, scope, and any compatibility implications), include the PR author and a brief rationale, and mark the living-spec status (e.g., "draft", "proposed", or "accepted") so the implementation change under local-mcp-bun is tracked in the spec.local-mcp-bun/tests/unit/stdio_proxy_server.test.ts (1)
113-137: Restore the originalprocess.cwddescriptor, not just its value.These tests replace a built-in property with a custom descriptor and then restore only the function value. That leaves
process.cwdwith different flags for later tests.♻️ Proposed fix
- const original_cwd = process.cwd; + const original_cwd_descriptor = Object.getOwnPropertyDescriptor(process, "cwd"); Object.defineProperty(process, "cwd", { configurable: true, value: () => { throw new Error("cwd unavailable"); }, }); @@ } finally { - Object.defineProperty(process, "cwd", { - configurable: true, - value: original_cwd, - }); + if (original_cwd_descriptor) { + Object.defineProperty(process, "cwd", original_cwd_descriptor); + } }Also applies to: 142-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/tests/unit/stdio_proxy_server.test.ts` around lines 113 - 137, The test temporarily replaces process.cwd but only restores its value; capture the original property descriptor via Object.getOwnPropertyDescriptor(process, "cwd") before redefining and then restore the full descriptor with Object.defineProperty(process, "cwd", originalDescriptor) in the finally block (instead of setting only value), updating both occurrences around build_daemon_url_with_client_artifact_root to preserve configurable/writable/enumerable flags and the original function reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@local-mcp-bun/src/tool_router.ts`:
- Around line 2074-2085: The current catch in the write flow treats any
non-tool_error as INVALID_ARGUMENT, which misreports filesystem write failures;
update the catch block handling for errors thrown by write_artifact_file
(function name) so that if error is not an instance of tool_error you rethrow a
new tool_error with code "TOOL_FAILED" (instead of "INVALID_ARGUMENT"), keep the
message `failed to persist ${tool_name} to ${absolute_path}`, include the path
and the original error message as cause, and preserve existing behavior for
tool_error instances.
---
Nitpick comments:
In `@local-mcp-bun/README.md`:
- Around line 1-4: The PR adds artifact root scoping changes under local-mcp-bun
but does not update the living-spec; update either
specs/local-multiplexed-browser-mcp-spec.md (preferred) or
specs/local-multiplexed-browser-mcp-changelog.md to record the new behavior: add
a short section describing the artifact root scoping semantics, list the
specific changes introduced by this PR (behavior, scope, and any compatibility
implications), include the PR author and a brief rationale, and mark the
living-spec status (e.g., "draft", "proposed", or "accepted") so the
implementation change under local-mcp-bun is tracked in the spec.
In `@local-mcp-bun/tests/unit/stdio_proxy_server.test.ts`:
- Around line 113-137: The test temporarily replaces process.cwd but only
restores its value; capture the original property descriptor via
Object.getOwnPropertyDescriptor(process, "cwd") before redefining and then
restore the full descriptor with Object.defineProperty(process, "cwd",
originalDescriptor) in the finally block (instead of setting only value),
updating both occurrences around build_daemon_url_with_client_artifact_root to
preserve configurable/writable/enumerable flags and the original function
reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6031d6b2-4d26-486b-9fb4-bbdddcd18866
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (2)
- local-mcp-bun/tests/unit/artifacts.test.ts
- local-mcp-bun/tests/unit/daemon_spawn_target.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- local-mcp-bun/tests/unit/session_registry.test.ts
- local-mcp-bun/src/stdio_proxy_server.ts
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
6325971 to
2e923be
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@local-mcp-bun/src/daemon_controller.ts`:
- Around line 679-680: The second wait (wait_for_proxy_daemon_state) can run a
fresh full timeout and let failures take up to ~2× daemon_connect_timeout_ms;
compute a single deadline before the first wait and pass the remaining time (or
the absolute deadline) into both waits so the total is capped by
daemon_connect_timeout_ms. Concretely: capture now + daemon_connect_timeout_ms
as a deadline, pass that deadline or remainingMs into wait_for_daemon_health and
then into wait_for_proxy_daemon_state (or extend wait_for_proxy_daemon_state to
accept a deadline argument), and ensure start_proxy_runtime is only called after
the second wait honors the same deadline. Use the symbols
daemon_connect_timeout_ms, wait_for_daemon_health, wait_for_proxy_daemon_state,
and start_proxy_runtime to locate and update the calls.
In `@local-mcp-bun/tests/unit/daemon_spawn_target.test.ts`:
- Around line 255-293: The test schedules a setTimeout that writes the daemon
state file but never cancels it, causing a possible ENOENT if the temp_dir is
removed first; fix by capturing the timer handle (e.g., const writerTimer =
setTimeout(...)) and call clearTimeout(writerTimer) in the finally block before
removing temp_dir so the delayed write cannot run after cleanup; update the test
surrounding the setTimeout/wait_for_proxy_daemon_state call to use the captured
timer and clear it in finally.
🪄 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: CHILL
Plan: Pro
Run ID: 2f1dbae5-35bb-47ba-8b43-44ab904cf95c
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (1)
- local-mcp-bun/tests/unit/session_registry.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- local-mcp-bun/tests/unit/stdio_proxy_server.test.ts
- local-mcp-bun/README.md
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
2e923be to
dfbf2e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
local-mcp-bun/src/daemon_controller.ts (1)
596-635:⚠️ Potential issue | 🟡 MinorThe connect timeout still isn't a true end-to-end cap.
This deadline now bounds the health/state polling calls, but the final WebSocket dial in
start_proxy_runtime()still gets the fulloptions.daemon_connect_timeout_ms, and both wait loops can oversleep their remaining budget by another poll/request interval. In the failure path, startup can still run well past the configured timeout. Please carry the remaining budget into the proxy connect step and clamp each poll/sleep against the same deadline.Also applies to: 648-694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/daemon_controller.ts` around lines 596 - 635, The startup timeout isn't enforced end-to-end in start_proxy_runtime: compute a hard deadline at start of the overall startup (Date.now() + options.daemon_connect_timeout_ms), then use the remaining millis for the proxy connect and for any polling/sleeps; clamp each poll/sleep interval to Math.max(0, Math.min(remainingMs, intendedInterval)) and pass remainingMs into stdio_proxy_server (connect_timeout_ms) instead of the original full options.daemon_connect_timeout_ms. Update the other analogous block (the duplicate at the later occurrence) the same way so both polling loops and the final WebSocket dial share the same deadline budget.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@local-mcp-bun/src/artifacts.ts`:
- Around line 15-16: artifact_file_write_flags currently clears O_NOFOLLOW on
win32 which allows a TOCTOU symlink swap between the pathname validation and the
subsequent open(); fix by either disallowing pathname-based artifact writes on
Windows (throw when process.platform === "win32" for the code path that uses the
validated pathname) or by opening against a verified parent directory handle:
after performing the pathname validation, open the parent directory with
O_DIRECTORY|O_NOFOLLOW, verify it is inside the session root, then perform an
openat-style open of the basename relative to that parent FD (or equivalent
Bun/Node API that opens relative to a directory handle) instead of calling
open(targetPath, ...); update uses of artifact_file_write_flags and the open()
call that follows the validation to use the parent-handle relative open, or gate
the operation on platform as described.
In `@local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts`:
- Around line 264-268: The test's outer timeout is too small relative to the
polling loop: increase the outer test timeout for the test that calls
wait_for_condition(..., 20_000, 250) so the test runner won't kill the test
while the polling runs; update the test invocation (the test(...) call
surrounding the polling) to use a larger timeout (e.g., 40_000 or 60_000) to
provide headroom for setup and wait_for_condition to complete, ensuring
references to wait_for_condition and the test(...) wrapper remain unchanged.
---
Duplicate comments:
In `@local-mcp-bun/src/daemon_controller.ts`:
- Around line 596-635: The startup timeout isn't enforced end-to-end in
start_proxy_runtime: compute a hard deadline at start of the overall startup
(Date.now() + options.daemon_connect_timeout_ms), then use the remaining millis
for the proxy connect and for any polling/sleeps; clamp each poll/sleep interval
to Math.max(0, Math.min(remainingMs, intendedInterval)) and pass remainingMs
into stdio_proxy_server (connect_timeout_ms) instead of the original full
options.daemon_connect_timeout_ms. Update the other analogous block (the
duplicate at the later occurrence) the same way so both polling loops and the
final WebSocket dial share the same deadline budget.
🪄 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: CHILL
Plan: Pro
Run ID: 02352a0f-ab2e-45c9-bd61-7d934c96ae2f
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (1)
- local-mcp-bun/tests/unit/artifacts.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- local-mcp-bun/tests/unit/session_registry.test.ts
- local-mcp-bun/README.md
- specs/local-multiplexed-browser-mcp-spec.md
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
dfbf2e3 to
1722622
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/local-multiplexed-browser-mcp-changelog.md`:
- Line 31: The release note sentence containing "Artifact parent creation now
revalidates the walked parent immediately before each nested directory
create..." should replace the phrase "directory create" with "directory
creation" for correct grammar; update the sentence in the
specs/local-multiplexed-browser-mcp-changelog.md where the phrase appears (look
for the exact substring "nested directory create") to read "nested directory
creation" while preserving the rest of the sentence and punctuation.
🪄 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: CHILL
Plan: Pro
Run ID: b2448274-9fdc-4101-8e20-1d6ae1dbdad0
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (4)
- local-mcp-bun/src/artifacts.ts
- local-mcp-bun/tests/unit/daemon_spawn_target.test.ts
- local-mcp-bun/src/tool_router.ts
- local-mcp-bun/src/daemon_controller.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- local-mcp-bun/tests/unit/session_registry.test.ts
- local-mcp-bun/src/stdio_proxy_server.ts
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
1722622 to
65ea9d4
Compare
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)
local-mcp-bun/src/daemon_controller.ts (1)
653-698:⚠️ Potential issue | 🟠 MajorThe shared connect deadline still isn't a hard cap.
Passing
remaining_timeout_ms()here does not fully bound startup time yet.wait_for_daemon_health()still performs 750 ms health requests per poll, andwait_for_proxy_daemon_state()still sleeps a full 200 ms between polls. If only a small budget remains, either helper can run pastdaemon_connect_timeout_ms.🛠️ Suggested direction
-async function wait_for_daemon_health( +async function wait_for_daemon_health( daemon_host: string, daemon_port: number, - timeout_ms: number, + deadline_ms: number, ): Promise<daemon_health_snapshot | undefined> { - const started_at = Date.now(); - - while (Date.now() - started_at < timeout_ms) { - const health = await fetch_daemon_health(daemon_host, daemon_port, daemon_health_request_timeout_ms); + while (true) { + const remaining_ms = remaining_timeout_ms(deadline_ms); + if (remaining_ms <= 0) { + return undefined; + } + + const health = await fetch_daemon_health( + daemon_host, + daemon_port, + Math.min(daemon_health_request_timeout_ms, remaining_ms), + ); if (health) { return health; } - await new Promise((resolve_promise) => { - setTimeout(resolve_promise, daemon_health_poll_interval_ms); - }); + await sleep(Math.min(daemon_health_poll_interval_ms, remaining_timeout_ms(deadline_ms))); } - - return undefined; }Apply the same remaining-budget clamp inside
wait_for_proxy_daemon_state().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/daemon_controller.ts` around lines 653 - 698, The remaining-connect deadline passed from the caller isn't enforced inside wait_for_proxy_daemon_state, so its internal 200ms sleep (and wait_for_daemon_health's 750ms polls) can overrun the budget; update wait_for_proxy_daemon_state to accept the remaining timeout (already passed in caller as remaining_timeout_ms(daemon_connect_deadline_ms)) and clamp any internal waits/sleep/dial timeouts to Math.min(current_remaining, configured_delay) before each poll, aborting immediately when remaining <= 0; similarly ensure any calls inside it to wait_for_daemon_health use the clamped remaining timeout instead of fixed 750ms so the overall daemon_connect_deadline_ms is a hard cap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@local-mcp-bun/src/daemon_controller.ts`:
- Around line 263-267: The helper proxy_needs_matching_daemon_state currently
returns only should_attach_client_artifact_root_to_daemon(...), so it skips
waiting for the daemon /health state when auto-auth is requested; update
proxy_needs_matching_daemon_state to also return true when the proxy is using
auto auth (e.g. env["MCP_AUTH_TOKEN"] === "auto") so
resolve_proxy_auth_token_for_log() gets the matching-state wait; in short,
change proxy_needs_matching_daemon_state to return
should_attach_client_artifact_root_to_daemon(...) || env["MCP_AUTH_TOKEN"] ===
"auto" (or equivalent detection of auto auth) and keep references to
proxy_needs_matching_daemon_state, should_attach_client_artifact_root_to_daemon,
and resolve_proxy_auth_token_for_log.
In `@local-mcp-bun/src/tool_router.ts`:
- Around line 2094-2101: The response built by persist_artifact_if_requested()
for SESSION_NOT_FOUND can be overwritten because call_tool() unconditionally
invokes mark_session_recovered(agent_session_id) which calls get_session() and
may throw SESSION_NOT_FOUND; fix by making mark_session_recovered tolerant of
missing sessions or by wrapping its call in call_tool() with a try/catch that
ignores SESSION_NOT_FOUND errors so the original saved:false artifact response
from persist_artifact_if_requested() is returned unchanged (refer to
persist_artifact_if_requested, call_tab_scoped_tool, call_tool,
mark_session_recovered, get_session, and the SESSION_NOT_FOUND error code).
---
Outside diff comments:
In `@local-mcp-bun/src/daemon_controller.ts`:
- Around line 653-698: The remaining-connect deadline passed from the caller
isn't enforced inside wait_for_proxy_daemon_state, so its internal 200ms sleep
(and wait_for_daemon_health's 750ms polls) can overrun the budget; update
wait_for_proxy_daemon_state to accept the remaining timeout (already passed in
caller as remaining_timeout_ms(daemon_connect_deadline_ms)) and clamp any
internal waits/sleep/dial timeouts to Math.min(current_remaining,
configured_delay) before each poll, aborting immediately when remaining <= 0;
similarly ensure any calls inside it to wait_for_daemon_health use the clamped
remaining timeout instead of fixed 750ms so the overall
daemon_connect_deadline_ms is a hard cap.
🪄 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: CHILL
Plan: Pro
Run ID: 65398e69-6494-4db5-ad7d-d3f3afbf413b
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (2)
- local-mcp-bun/src/daemon_ingress_server.ts
- local-mcp-bun/tests/integration/daemon_ingress_cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- local-mcp-bun/tests/unit/session_registry.test.ts
- local-mcp-bun/README.md
- local-mcp-bun/src/session_registry.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
local-mcp-bun/src/tool_router.ts (1)
2094-2101:⚠️ Potential issue | 🔴 CriticalThe new
saved: falsefallback is still overwritten by session recovery.This branch now builds a safe artifact response for
SESSION_NOT_FOUND, butcall_tool()still unconditionally callsmark_session_recovered()at Line 999. If the session really disappeared between capture and persistence, that recovery call throwsSESSION_NOT_FOUNDand replaces the response you just returned.🛠️ Minimal fix
- this.session_registry.mark_session_recovered(agent_session_id); + try { + this.session_registry.mark_session_recovered(agent_session_id); + } catch (error) { + if (!(error instanceof tool_error) || error.code !== "SESSION_NOT_FOUND") { + throw error; + } + } return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/tool_router.ts` around lines 2094 - 2101, The new branch in tool_router (the tool_error handling that returns saved: false, path: requested_path, artifact_error: error.to_payload()) is being overwritten because call_tool() still unconditionally invokes mark_session_recovered() which can throw tool_error with code "SESSION_NOT_FOUND"; change call_tool() so that the call to mark_session_recovered() is wrapped to specifically catch a tool_error with code "SESSION_NOT_FOUND" and, when caught, preserve and return the previously built artifact response instead of letting the recovery exception replace it (alternatively skip mark_session_recovered() if the earlier SESSION_NOT_FOUND branch was taken); reference call_tool(), mark_session_recovered(), tool_error, and the saved:false artifact response to locate where to add the targeted catch/conditional.local-mcp-bun/src/daemon_controller.ts (1)
263-268:⚠️ Potential issue | 🟡 MinorProxy may skip waiting for daemon state when auto-auth is requested without artifact-root attachment.
proxy_needs_matching_daemon_state()currently only checksshould_attach_client_artifact_root_to_daemon(). WhenMCP_AUTH_TOKEN=autois set but artifact-root attachment is disabled (e.g., non-loopback daemon or explicitMCP_ATTACH_CLIENT_ARTIFACT_ROOT=0), the proxy won't wait for a matching daemon state, soresolve_proxy_auth_token_for_log()may not find a usable token to print.🛠️ Suggested fix
function proxy_needs_matching_daemon_state( daemon_target: Pick<daemon_run_options, "daemon_host">, env: Record<string, string | undefined>, ): boolean { - return should_attach_client_artifact_root_to_daemon(daemon_target, env); + const auto_requested = + get_non_empty_string(env.MCP_AUTH_TOKEN) === "auto" || get_non_empty_string(env.MCP_AUTH_AUTO) === "1"; + + return auto_requested || should_attach_client_artifact_root_to_daemon(daemon_target, env); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/daemon_controller.ts` around lines 263 - 268, proxy_needs_matching_daemon_state currently only returns should_attach_client_artifact_root_to_daemon(...) so when MCP_AUTH_TOKEN=auto but artifact-root attachment is disabled the proxy won't wait for the daemon state and resolve_proxy_auth_token_for_log may miss a token; update proxy_needs_matching_daemon_state to also return true when env["MCP_AUTH_TOKEN"] === "auto" (i.e., return should_attach_client_artifact_root_to_daemon(...) || env["MCP_AUTH_TOKEN"] === "auto") so the proxy waits for a matching daemon state whenever auto-auth is requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@local-mcp-bun/src/stdio_proxy_server.ts`:
- Around line 55-57: The current logic only sets client_artifact_root_token when
URL lacks that param, so stale tokens persist; change the block that references
artifact_root_token and url.searchParams.has("client_artifact_root_token") so
that when artifact_root_token is present you always write the fresh token into
the URL (e.g. remove the conditional check and call
url.searchParams.set("client_artifact_root_token", artifact_root_token)
unconditionally or replace any existing value), ensuring the new daemon-issued
token replaces any stale client_artifact_root_token before reconnecting.
---
Duplicate comments:
In `@local-mcp-bun/src/daemon_controller.ts`:
- Around line 263-268: proxy_needs_matching_daemon_state currently only returns
should_attach_client_artifact_root_to_daemon(...) so when MCP_AUTH_TOKEN=auto
but artifact-root attachment is disabled the proxy won't wait for the daemon
state and resolve_proxy_auth_token_for_log may miss a token; update
proxy_needs_matching_daemon_state to also return true when env["MCP_AUTH_TOKEN"]
=== "auto" (i.e., return should_attach_client_artifact_root_to_daemon(...) ||
env["MCP_AUTH_TOKEN"] === "auto") so the proxy waits for a matching daemon state
whenever auto-auth is requested.
In `@local-mcp-bun/src/tool_router.ts`:
- Around line 2094-2101: The new branch in tool_router (the tool_error handling
that returns saved: false, path: requested_path, artifact_error:
error.to_payload()) is being overwritten because call_tool() still
unconditionally invokes mark_session_recovered() which can throw tool_error with
code "SESSION_NOT_FOUND"; change call_tool() so that the call to
mark_session_recovered() is wrapped to specifically catch a tool_error with code
"SESSION_NOT_FOUND" and, when caught, preserve and return the previously built
artifact response instead of letting the recovery exception replace it
(alternatively skip mark_session_recovered() if the earlier SESSION_NOT_FOUND
branch was taken); reference call_tool(), mark_session_recovered(), tool_error,
and the saved:false artifact response to locate where to add the targeted
catch/conditional.
🪄 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: CHILL
Plan: Pro
Run ID: 603ad88e-e11b-4386-b445-4692e39878a4
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (3)
- local-mcp-bun/README.md
- local-mcp-bun/tests/unit/daemon_spawn_target.test.ts
- local-mcp-bun/tests/integration/daemon_ingress_cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts
- local-mcp-bun/tests/unit/session_registry.test.ts
- specs/local-multiplexed-browser-mcp-spec.md
- local-mcp-bun/tests/unit/mcp_protocol_session.test.ts
- local-mcp-bun/src/mcp_protocol_session.ts
- local-mcp-bun/src/daemon_ingress_server.ts
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
65ea9d4 to
969dca6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
local-mcp-bun/src/stdio_proxy_server.ts (1)
55-57:⚠️ Potential issue | 🟠 MajorAlways replace stale
client_artifact_root_tokenvalues.If the incoming daemon URL already carries
client_artifact_root_token, this branch preserves the old value instead of swapping in the fresh daemon-issued token. After token rotation, the proxy reconnects with a stale token and ingress rejects the client artifact root.🛠️ Minimal fix
- if (artifact_root_token && !url.searchParams.has("client_artifact_root_token")) { + if (artifact_root_token) { url.searchParams.set("client_artifact_root_token", artifact_root_token); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/stdio_proxy_server.ts` around lines 55 - 57, The existing conditional (if (artifact_root_token && !url.searchParams.has("client_artifact_root_token")) ...) preserves a stale client_artifact_root_token when the daemon issues a new one; always overwrite the query param when artifact_root_token is present. Replace that condition so that when artifact_root_token is truthy you call url.searchParams.set("client_artifact_root_token", artifact_root_token) unconditionally (remove the !url.searchParams.has(...) check) to ensure token rotation always updates the URL.local-mcp-bun/src/daemon_controller.ts (1)
263-268:⚠️ Potential issue | 🟡 Minor
proxy_needs_matching_daemon_statemay miss auto-auth scenarios.This helper only checks
should_attach_client_artifact_root_to_daemon(), butresolve_proxy_auth_token_for_log()at lines 254-260 also requires matching daemon state to print the auto-auth token. When artifact-root attachment is disabled butMCP_AUTH_TOKEN=autois set, the proxy won't wait for matching state, so the token log may not appear.Consider including auto-auth detection:
Suggested fix
function proxy_needs_matching_daemon_state( daemon_target: Pick<daemon_run_options, "daemon_host">, env: Record<string, string | undefined>, ): boolean { - return should_attach_client_artifact_root_to_daemon(daemon_target, env); + const auto_requested = + get_non_empty_string(env.MCP_AUTH_TOKEN) === "auto" || get_non_empty_string(env.MCP_AUTH_AUTO) === "1"; + + return auto_requested || should_attach_client_artifact_root_to_daemon(daemon_target, env); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/daemon_controller.ts` around lines 263 - 268, The function proxy_needs_matching_daemon_state currently only calls should_attach_client_artifact_root_to_daemon and misses cases where resolve_proxy_auth_token_for_log needs the daemon to reach matching state (e.g. when MCP_AUTH_TOKEN=auto); update proxy_needs_matching_daemon_state to also return true when the environment indicates auto-auth is requested (check env["MCP_AUTH_TOKEN"] === "auto" or the existing helper used by resolve_proxy_auth_token_for_log) so the proxy will wait for the daemon state necessary to print the auto-auth token; keep the existing artifact-root check and combine with the auto-auth check (logical OR) so either condition triggers waiting for matching daemon state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@local-mcp-bun/src/daemon_controller.ts`:
- Around line 263-268: The function proxy_needs_matching_daemon_state currently
only calls should_attach_client_artifact_root_to_daemon and misses cases where
resolve_proxy_auth_token_for_log needs the daemon to reach matching state (e.g.
when MCP_AUTH_TOKEN=auto); update proxy_needs_matching_daemon_state to also
return true when the environment indicates auto-auth is requested (check
env["MCP_AUTH_TOKEN"] === "auto" or the existing helper used by
resolve_proxy_auth_token_for_log) so the proxy will wait for the daemon state
necessary to print the auto-auth token; keep the existing artifact-root check
and combine with the auto-auth check (logical OR) so either condition triggers
waiting for matching daemon state.
In `@local-mcp-bun/src/stdio_proxy_server.ts`:
- Around line 55-57: The existing conditional (if (artifact_root_token &&
!url.searchParams.has("client_artifact_root_token")) ...) preserves a stale
client_artifact_root_token when the daemon issues a new one; always overwrite
the query param when artifact_root_token is present. Replace that condition so
that when artifact_root_token is truthy you call
url.searchParams.set("client_artifact_root_token", artifact_root_token)
unconditionally (remove the !url.searchParams.has(...) check) to ensure token
rotation always updates the URL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 375f63e4-7204-4c2e-aab0-4580e32dfc3b
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (4)
- local-mcp-bun/src/mcp_protocol_session.ts
- specs/local-multiplexed-browser-mcp-spec.md
- local-mcp-bun/tests/unit/daemon_spawn_target.test.ts
- local-mcp-bun/src/artifacts.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- local-mcp-bun/tests/unit/session_registry.test.ts
- local-mcp-bun/README.md
- local-mcp-bun/tests/integration/daemon_ingress_cleanup.test.ts
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
969dca6 to
1088559
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
local-mcp-bun/src/mcp_protocol_session.ts (1)
133-138:⚠️ Potential issue | 🟠 MajorPin the resolved artifact root after the first successful session open.
When
artifact_root_contextis function-backed, Line 133 uses it to open the initial session and Line 385 re-evaluates it again during implicit rebind. If that path is deleted/replaced between those two events, the same MCP connection can silently rebind onto a different canonical root and bypass the root-pinning guarantees added for artifact writes. Snapshot the resolved context from the opened session and reuse that copy for later rebinds.Suggested hardening
- private readonly artifact_root_context: artifact_root_context_input; + private artifact_root_context: artifact_root_context_input; @@ - const result = this.runtime.tool_router.open_session(client_name, token, this.artifact_root_context); + const result = this.runtime.tool_router.open_session(client_name, token, this.artifact_root_context); + this.artifact_root_context = this.runtime.session_registry.get_session_artifact_root_context( + result.agent_session_id, + ); const session = this.runtime.session_registry.get_session(result.agent_session_id); @@ result = this.runtime.tool_router.open_session( this.initialized_client_name, this.initialized_token, this.artifact_root_context, ); + this.artifact_root_context = this.runtime.session_registry.get_session_artifact_root_context( + result.agent_session_id, + );Also applies to: 385-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/mcp_protocol_session.ts` around lines 133 - 138, The artifact_root_context may be a function and is re-evaluated later, allowing silent rebinding; after successfully opening the initial session via runtime.tool_router.open_session and retrieving the session with runtime.session_registry.get_session(result.agent_session_id), capture and store the resolved artifact root from that session (e.g., snapshot session.artifact_root or similar) into a new instance field (like this.pinned_artifact_root_context) only if not already set, and then change later implicit rebind logic (the code paths around set_initialized_agent_session_id and the rebind at lines ~385-389) to prefer this.pinned_artifact_root_context instead of re-evaluating artifact_root_context so subsequent rebinds use the original pinned context.
♻️ Duplicate comments (1)
local-mcp-bun/src/tool_router.ts (1)
999-1000:⚠️ Potential issue | 🔴 CriticalGuard the post-call recovery mark when the session disappears.
persist_artifact_if_requested()now preserves a late-close race as{ saved: false, artifact_error }, but Line 999 still callsmark_session_recovered()unconditionally. Sincesession_registry.mark_session_recovered()still throwsSESSION_NOT_FOUND, that fallback response is replaced before Line 1000 returns.Minimal fix
- this.session_registry.mark_session_recovered(agent_session_id); + try { + this.session_registry.mark_session_recovered(agent_session_id); + } catch (error) { + if (!(error instanceof tool_error) || error.code !== "SESSION_NOT_FOUND") { + throw error; + } + } return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/src/tool_router.ts` around lines 999 - 1000, The post-call unguarded call to session_registry.mark_session_recovered(agent_session_id) can throw SESSION_NOT_FOUND and overwrite the earlier { saved: false, artifact_error } result from persist_artifact_if_requested(); change this to either (a) check session existence before calling (e.g. session_registry.has_session(agent_session_id)) or (b) wrap the mark_session_recovered call in a try/catch that only swallows SESSION_NOT_FOUND and rethrows other errors, so the original result variable is returned unchanged; update the block around persist_artifact_if_requested, mark_session_recovered, and the return of result to use this guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@local-mcp-bun/src/artifacts.ts`:
- Around line 268-283: The current flow creates and renames temp files by
mutable path strings (build_artifact_temp_path, temp_path, renameSync(temp_path,
absolute_path)), which allows a TOCTOU symlink/dir-move to publish outside the
pinned root; fix by anchoring operations to the already-validated parent
directory: after prepare_artifact_destination verifies the parent, open the
parent directory as a stable dirfd (open(..., O_DIRECTORY)), create the temp
file inside that directory (use the dirfd-based create or a pathname under
/proc/self/fd/<dirfd>), write using the opened file handle, then perform an
atomic rename using renameat (or an equivalent fs.renameat API) from the temp
filename to the final name relative to the same dirfd instead of
renameSync(temp_path, absolute_path); ensure to close the dirfd and file handle
in finally blocks.
---
Outside diff comments:
In `@local-mcp-bun/src/mcp_protocol_session.ts`:
- Around line 133-138: The artifact_root_context may be a function and is
re-evaluated later, allowing silent rebinding; after successfully opening the
initial session via runtime.tool_router.open_session and retrieving the session
with runtime.session_registry.get_session(result.agent_session_id), capture and
store the resolved artifact root from that session (e.g., snapshot
session.artifact_root or similar) into a new instance field (like
this.pinned_artifact_root_context) only if not already set, and then change
later implicit rebind logic (the code paths around
set_initialized_agent_session_id and the rebind at lines ~385-389) to prefer
this.pinned_artifact_root_context instead of re-evaluating artifact_root_context
so subsequent rebinds use the original pinned context.
---
Duplicate comments:
In `@local-mcp-bun/src/tool_router.ts`:
- Around line 999-1000: The post-call unguarded call to
session_registry.mark_session_recovered(agent_session_id) can throw
SESSION_NOT_FOUND and overwrite the earlier { saved: false, artifact_error }
result from persist_artifact_if_requested(); change this to either (a) check
session existence before calling (e.g.
session_registry.has_session(agent_session_id)) or (b) wrap the
mark_session_recovered call in a try/catch that only swallows SESSION_NOT_FOUND
and rethrows other errors, so the original result variable is returned
unchanged; update the block around persist_artifact_if_requested,
mark_session_recovered, and the return of result to use this guard.
🪄 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: CHILL
Plan: Pro
Run ID: 3e1d8227-51fc-4c64-b30c-fdefbac3af4d
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (2)
- local-mcp-bun/tests/unit/daemon_spawn_target.test.ts
- specs/local-multiplexed-browser-mcp-spec.md
🚧 Files skipped from review as they are similar to previous changes (2)
- local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts
- local-mcp-bun/tests/unit/session_registry.test.ts
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
1088559 to
125c084
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
local-mcp-bun/README.md (1)
1-3:⚠️ Potential issue | 🟠 MajorInclude living-spec status updates for the local-mcp-bun changes.
Per coding guidelines, PRs modifying implementation paths under
local-mcp-bun/must include corresponding status updates in eitherspecs/local-multiplexed-browser-mcp-spec.mdorspecs/local-multiplexed-browser-mcp-changelog.md. This PR modifieslocal-mcp-bun/README.mdbut does not update the specification files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/README.md` around lines 1 - 3, This PR updates local-mcp-bun/README.md but misses the required living-spec status note; add a concise status update describing the change (author, summary of modifications, and any behavioral/compatibility impact) to either specs/local-multiplexed-browser-mcp-spec.md or specs/local-multiplexed-browser-mcp-changelog.md, referencing "local-mcp-bun/README.md" and including a date and short rationale so the spec reflects the implementation change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@local-mcp-bun/src/artifacts.ts`:
- Around line 301-359: Add a short code comment in write_artifact_file (near the
use of open_stable_parent_directory / prepare_artifact_destination and the
renameSync step) documenting that on macOS we cannot use fd-relative
openat/renameat semantics because Node.js/libuv do not expose fd-relative
wrappers (so the implementation must rely on repeated
prepare_artifact_destination + O_NOFOLLOW rather than true fd-anchoring);
reference the limitation and why fd-anchored operations were not used (libuv
lacks openat/renameat wrappers) so future readers understand the
platform-specific constraint.
---
Outside diff comments:
In `@local-mcp-bun/README.md`:
- Around line 1-3: This PR updates local-mcp-bun/README.md but misses the
required living-spec status note; add a concise status update describing the
change (author, summary of modifications, and any behavioral/compatibility
impact) to either specs/local-multiplexed-browser-mcp-spec.md or
specs/local-multiplexed-browser-mcp-changelog.md, referencing
"local-mcp-bun/README.md" and including a date and short rationale so the spec
reflects the implementation change.
🪄 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: CHILL
Plan: Pro
Run ID: ba16a6d5-8ef7-4320-b2cb-0488ed1f56e4
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (3)
- specs/local-multiplexed-browser-mcp-spec.md
- local-mcp-bun/tests/unit/daemon_spawn_target.test.ts
- local-mcp-bun/src/stdio_proxy_server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- local-mcp-bun/tests/unit/session_registry.test.ts
KnotFalse
force-pushed
the
per-client-artifact-roots
branch
from
125c084 to
ad03216
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
local-mcp-bun/tests/unit/stdio_proxy_server.test.ts (1)
155-182: Consider using a more stable approach for accessing private members in tests.Accessing the private
daemon_urlviaas unknown as { daemon_url: string }works but is fragile if the internal structure changes. This is acceptable for unit tests but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@local-mcp-bun/tests/unit/stdio_proxy_server.test.ts` around lines 155 - 182, The test is reaching into a private member by casting the stdio_proxy_server instance to an unknown type to read daemon_url, which is fragile; add a stable public accessor on stdio_proxy_server (e.g., getDaemonUrl() or a public getter daemon_url) that returns the constructed daemon URL and update the test to call that accessor instead of using the (proxy as unknown as { daemon_url: string }) cast; ensure the accessor respects attach_client_artifact_root and artifact_root_token behavior so the existing assertions on client_artifact_root and client_artifact_root_token still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@local-mcp-bun/tests/unit/stdio_proxy_server.test.ts`:
- Around line 155-182: The test is reaching into a private member by casting the
stdio_proxy_server instance to an unknown type to read daemon_url, which is
fragile; add a stable public accessor on stdio_proxy_server (e.g.,
getDaemonUrl() or a public getter daemon_url) that returns the constructed
daemon URL and update the test to call that accessor instead of using the (proxy
as unknown as { daemon_url: string }) cast; ensure the accessor respects
attach_client_artifact_root and artifact_root_token behavior so the existing
assertions on client_artifact_root and client_artifact_root_token still pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86bd51a6-1291-450b-808e-82f9cd9fac8b
📒 Files selected for processing (19)
agent-logs/2026-04-24T151451-0500-per-client-artifact-roots.mdlocal-mcp-bun/README.mdlocal-mcp-bun/src/artifacts.tslocal-mcp-bun/src/daemon_controller.tslocal-mcp-bun/src/daemon_ingress_server.tslocal-mcp-bun/src/mcp_protocol_session.tslocal-mcp-bun/src/session_registry.tslocal-mcp-bun/src/stdio_proxy_server.tslocal-mcp-bun/src/tool_router.tslocal-mcp-bun/tests/integration/daemon_ingress_cleanup.test.tslocal-mcp-bun/tests/integration/daemon_singleton_proxy.test.tslocal-mcp-bun/tests/integration/parity_remaining.test.tslocal-mcp-bun/tests/unit/artifacts.test.tslocal-mcp-bun/tests/unit/daemon_spawn_target.test.tslocal-mcp-bun/tests/unit/mcp_protocol_session.test.tslocal-mcp-bun/tests/unit/session_registry.test.tslocal-mcp-bun/tests/unit/stdio_proxy_server.test.tsspecs/local-multiplexed-browser-mcp-changelog.mdspecs/local-multiplexed-browser-mcp-spec.md
✅ Files skipped from review due to trivial changes (3)
- local-mcp-bun/src/stdio_proxy_server.ts
- local-mcp-bun/src/tool_router.ts
- local-mcp-bun/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- local-mcp-bun/tests/integration/daemon_singleton_proxy.test.ts
- local-mcp-bun/tests/unit/artifacts.test.ts
- specs/local-multiplexed-browser-mcp-spec.md
- local-mcp-bun/src/artifacts.ts
Contributor
Author
Merge activity
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests