fix(daemon): share one user daemon across configurations - #346
Merged
Conversation
Per-config hosts and accidental launch identities could create duplicate retained connections and competing Chrome attachments. Treat resolved configurations as immutable views of one OS-user broker, and reserve a canonical Chrome owner whose launch and relay policy comes from the global configuration. Ordinary and generated clients reuse the same connection. Enforce view-local permissions and connection-scoped OAuth ownership while preserving HTTP credential isolation. Never replay uncertain calls; require verified retirement before reconnecting, and protect active or uncertain work from idle shutdown. Protocol v3 requires coordinated cutover from per-config daemons: drain old work and use the documented migration and verified-stop procedure. Retained connections remain shared MCP sessions; multi-call select-then-act workflows still require caller coordination. Local check, build, and tests pass with 1,790 tests and 26 existing skips. Source-blind synthetic probes cover reuse, lifetime, filters, idle protection, and recovery. Cross-platform CI and real-Chrome proof remain landing gates.
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review in progressClawSweeper is reviewing this revision. This supersedes any previous blocked status. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| `const net=require('node:net'),fs=require('node:fs'),{spawn}=require('node:child_process'); | ||
| const owned=spawn(process.execPath,['-e','setInterval(()=>{},1000)'],{stdio:'ignore'}); | ||
| const server=net.createServer(s=>s.on('data',chunk=>{const r=JSON.parse(chunk);s.end(JSON.stringify({id:r.id,ok:true,result:r.method==='status'?{pid:process.pid,socketPath:${JSON.stringify(socket)},protocolVersion:1,servers:[]}:true}));if(r.method==='stop'){server.close();setTimeout(()=>owned.kill(),250);}})); | ||
| server.listen(${JSON.stringify(socket)},()=>fs.writeFileSync(${JSON.stringify(metadata)},JSON.stringify({pid:process.pid,socketPath:${JSON.stringify(socket)}}),{mode:384}));`, |
| `const net=require('node:net'),fs=require('node:fs'),{spawn}=require('node:child_process'); | ||
| const owned=spawn(process.execPath,['-e','setInterval(()=>{},1000)'],{stdio:'ignore'}); | ||
| const server=net.createServer(s=>s.on('data',chunk=>{const r=JSON.parse(chunk);s.end(JSON.stringify({id:r.id,ok:true,result:r.method==='status'?{pid:process.pid,socketPath:${JSON.stringify(socket)},protocolVersion:1,servers:[]}:true}));if(r.method==='stop'){server.close();setTimeout(()=>owned.kill(),250);}})); | ||
| server.listen(${JSON.stringify(socket)},()=>fs.writeFileSync(${JSON.stringify(metadata)},JSON.stringify({pid:process.pid,socketPath:${JSON.stringify(socket)}}),{mode:384}));`, |
Target and batch Windows owner/start identity observations. Validate the UTF-8 completion envelope and fail closed when process identity is unknown. Cache Windows directory verification by filesystem identity and share in-flight host shutdown. Settle fixture requests and owned children before cleanup. Preserve existing timeouts, CI budgets, and platform coverage.
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed September 4, 2026, 7:14 PM ET / 23:14 UTC. ClawSweeper reviewWhat this changesReplace configuration-specific background processes with one authenticated user daemon that shares compatible MCP connections, preserves configuration-local tool filters, and reserves a canonical Chrome connection. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 10 items remain Keep open: the singleton behavior is distinct from current main and remains useful. The prior view-release finding is unresolved, and this review identifies three additional source-backed defects. Browser-authority proof also remains incomplete. Priority: P2 Review scores
Verification
How this fits togetherMCPorter's daemon keeps MCP servers connected between CLI calls. Ordinary and generated clients submit resolved configurations to the broker, which authorizes each request and routes it to a retained server connection. flowchart TD
A[CLI and generated clients] --> B[Authenticated local connection]
B --> C[Immutable configuration view]
C --> D[Tool permission checks]
D --> E[Connection identity and Chrome ownership]
E --> F[Serialized retained MCP connection]
F --> G[MCP server or Chrome relay]
G --> A
Before merge
Findings
Agent review detailsSecurityNeeds attention: No dependency or workflow execution changes were introduced, but retained Chrome authority needs queued-revocation proof before merge. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the single-owner design with safe legacy-directory migration, operation-scoped view lifetimes, broker-owned discovery metadata, bounded idle scheduling, and demonstrated Chrome authorization through final I/O. Do we have a high-confidence way to reproduce the issue? Yes, source establishes focused regression cases for a 0755 released daemon directory, same-client view replacement before RPC admission, description discovery without configured metadata, and idle delays above the native timer limit. These cases were not executed during this read-only review. Is this the best way to solve the issue? Not yet: the shared broker addresses a problem that main still leaves open, but the patch needs the identified lifecycle, migration, discovery, and timer repairs plus final-effect authority evidence. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against add13af3f0db. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Split the aggregate CLI workflow into seven named contracts that retain one shared fixture and broker throughout. Preserve every behavior assertion and fresh CLI invocation, with shared state valid for shuffled and selected cases and settled in-flight cleanup. Remove the 25-second aggregate wrapper without increasing timeouts: keep the original 15-second subprocess timeout and existing global test and hook defaults. No runtime or CI budget changes.
Comment on lines
+27
to
+31
| const operation = run( | ||
| process.platform === 'win32' ? process.execPath : '/bin/sh', | ||
| process.platform === 'win32' ? argv : ['-c', 'exec "$@"', 'fixture-launcher', process.execPath, ...argv], | ||
| { env: launchEnv, cwd: callerCwd, timeout: 15000 } | ||
| ); |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Repair legacy permissions only during explicit confirmed migration: accept ordinary current-user-owned 0755 directories, upgrade through no-follow owner-checked descriptors to 0700, and revalidate pathname identity. Inspection stays read-only; unsafe modes, owners and replacements fail closed. Retain captured client view epochs through registration, RPC setup and settlement. Read description metadata through the retained broker connection and reject pooled raw-connect or interactive-session bypasses. Slice broker and host idle timers within native bounds while preserving full deadlines. Revalidate queued Chrome authority before recovery and after connection awaits immediately before SDK dispatch; already-running operations are not retroactively revoked and this is not atomic network revocation. Validation: scoped-clean static review; pnpm check, build and full test pass with 1,861 passed and 26 existing skips. The initial liveness timing failure passed on unchanged focused/full reruns and is not claimed separately fixed. Synthetic CLI probes cover owner-checked 0755 migration, one-child metadata discovery and short-window retention of a 30-day idle setting. Fake clocks cover eventual expiry; actual MCP fixture counters cover same-client view lifetime and queued/final-dispatch revocation. No new Chrome attempts were made. Earlier live list/open/select proof applies to d8c37ce; live evaluation and parallel browser checks remain incomplete. The accepted partial-live disclosure remains in the PR. Fresh cross-platform CI is required for this commit before the parent decides whether to land.
steipete
marked this pull request as ready for review
September 5, 2026 01:21
![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: cbc0832c06
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+184
to
+187
| return this.sendRequest<{ view: string; generation: string }>('registerView', { | ||
| definitions: effective, | ||
| clientInfo: epoch.clientInfo, | ||
| }); |
There was a problem hiding this comment.
Each operation now registers a persistent view here, but both daemon fast-call paths in src/cli.ts create a fresh DaemonClient and return without calling release() (including failure paths). Consequently, repeated Chrome/Playwright/mobile invocations leave one abandoned view per process; 1,024 calls within the 15-minute expiry window exhaust the broker's view limit and make subsequent registrations fail with view_limit. Ensure these fast paths release the client in a finally block.
Useful? React with 👍 / 👎.
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
2 participants
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
Replace config-selected keep-alive daemon processes with one OS-user broker. Configuration files become immutable client views, while equivalent stdio definitions and aliases share retained connections. Caller directory and inherited shell nesting no longer create accidental connection identities; explicit launch settings remain meaningful.
Existing Chrome has a separately reserved connection owner. Canonical global configuration controls its launch and relay policy, and conflicting temporary configs cannot start another attachment, steal the owner, or weaken required relay routing. Unrelated isolated-browser definitions do not poison the supported owner. Ordinary and generated CLIs use the same default client identity and connection.
The broker enforces view-local tool permissions and connection-scoped OAuth context. Local IPC authenticates both peers before sending resolved configuration. Timeouts are not replayed, reconnects wait for verified retirement, and idle retirement cannot interrupt queued, active, or uncertain work. HTTP aliases deliberately retain separate credential ownership. The related normalization in PR #345 is incorporated rather than duplicated.
Review follow-ups
The actionable findings in the current review are addressed:
0755directories and upgrades them to0700with no-follow descriptor checks and pathname revalidation. Inspection remains read-only. Unsafe owners, symlinks, writable modes, and replacement races fail closed. Empty/stopped legacy installations can migrate and start normally.Verification
Fresh CI run 33935378655 passed all Linux, macOS, and Windows jobs on review-fix head
cbc0832c0628095a6f88f65465e61a7b8ac0431d.Local
pnpm check,pnpm build, andpnpm testpass on the combined fixes: 1,861 passed tests and 26 existing skips. The prior CI run 33927720741 passed Linux, macOS, and Windows ond8c37ce2accbdc8bbf71b94a21bb5b6873990e19and is historical evidence only; the fresh run above verifies these follow-ups.Independent source-blind CLI probes verify concurrent cold starts across config files and aliases, different caller directories, ordinary/generated reuse in both startup orders, actual child UUIDs and counters, filters, client-exit behavior, in-flight config replacement, idle protection, no replay, and generation-scoped recovery. Additional fixtures cover migration, same-client view replacement before RPC admission, exclusive metadata discovery, long timer boundaries, queued credential revocation, and post-connect dispatch checks.
Windows CI initially exposed process-observation and cleanup failures. Bounded targeted queries, validated completion data, unknown-identity refusal, reduced redundant ACL work, and correct cleanup ordering addressed them. The aggregate CLI workflow was separated into focused contracts retaining the same broker and real invocations. No runtime or CI timeout budgets or existing platform coverage were increased. Transient local SSE/liveness failures passed on unchanged focused/full reruns and are not claimed as separately fixed.
Real-Chrome smoke and remaining limit
On the earlier
d8c37ce2build, a real macOS handover retired four verified legacy daemons. An explicitly approved temporary canonical direct policy was used. After one successful foreground Chrome approval, fresh CLI processes listed pages, opened a disposable synthetic tab, and selected it. The daemon generation and single connection ID/generation remained unchanged between attachment and cleanup. The original relay-required configuration was restored byte-for-byte and the test owner was stopped.The evaluation request was rejected because the verifier omitted the current Chrome tool's required
pageId. This PR does not claim completed live evaluation, live parallel cross-config browser checks, or a new physical-browser revocation test. Those contracts have fixture coverage where stated. The basic real-browser integration, legacy retirement, and explicitly accepted live-proof limitation are recorded for landing. No further Chrome attachment attempts are authorized or included in this change.Compatibility and rollout
Protocol v3 requires coordinated migration from old per-config hosts. Upgrade invoking clients, drain old work, inspect
mcporter daemon migrate, then use the documented verified-stop and permissions-upgrade procedure. There is no silent fallback to old daemons or transfer of CDP sessions.One retained connection is a shared MCP session, not per-agent selected-page isolation. Individual calls serialize; multi-call select-then-act workflows still need coordination. Unsupported/direct Chrome definitions and raw programmatic Chrome connections fail explicitly rather than bypass ownership. Broker-owned metadata is available without exposing an SDK context across IPC. A crash, deliberate restart, or migration can still require fresh browser approval.
No version bump, changelog edit, publication, or release is included. These behavior and compatibility notes are context for the eventual release.