Skip to content

fix(relay): normalize effective daemon identity - #345

Merged
steipete merged 2 commits into
mainfrom
steipete/serene-hopper-26ddbb
Sep 4, 2026
Merged

fix(relay): normalize effective daemon identity#345
steipete merged 2 commits into
mainfrom
steipete/serene-hopper-26ddbb

Conversation

@steipete

@steipete steipete commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Problem

Chrome relay daemon freshness included raw process environment values even when they resolved to the same runtime behavior. An unset MCPORTER_CHROME_DEVTOOLS_RELAY_POLICY and explicit prefer produced different hashes, as could process values fully overridden by a server's env map. The mismatch made the daemon client stop and restart an otherwise compatible daemon, potentially prompting for another Chrome direct-attachment approval.

Change

Build identity from each applicable server's effective relay settings. Reuse the routing resolvers to normalize policy/defaults/aliases, timeout defaults and clamping, validated endpoints, and default-profile spellings. Exclude fully overridden process values while preserving partial inheritance and raw command/relay-placeholder dependencies. Retain credential-directory and key-rotation identity, discovery/executable inputs, and relevant working-directory dependencies. Advance runtime identity metadata to version 2 so older metadata uses the existing replacement path.

Keep require fail-closed and leave config-path/layer socket and transport isolation unchanged. This is a daemon freshness fix, not a global transport-pool identity. Separate temporary configs, including relay-off configs, still produce separate daemons and are explicitly outside this change.

Verification

  • Added synthetic relay-identity regressions for equivalent defaults/aliases, per-server overrides, multi-server inheritance, placeholder dependencies, and discovery/launcher changes.
  • Extended daemon-client staleness fixtures to prove equivalent callers send only status and listTools, without stop/spawn, while meaningful policy/profile/endpoint/discovery changes and credential rotation remain stale.
  • pnpm check passed.
  • pnpm test passed after the Windows fixture corrections: 200 files and 1,840 tests passed; 4 files and 26 tests skipped, with live-test opt-ins disabled.
  • Focused relay, daemon-staleness, discovery, fixture-isolation, and config-import suites: 188 tests passed.
  • Initial Windows CI exposed test-isolation defects: clearing temp-directory variables removed the platform's temp root, case-insensitive aliases could restore incorrectly, and importer teardown assigned the string undefined instead of removing absent overrides. The follow-up is fixture-only: safe temp placement, alias-aware snapshots, Windows/POSIX credential containment, proper import-override restoration, and synthetic regressions. No production importer behavior, test skips, or timing budgets changed.
  • Independent Codex autoreview: no actionable findings at its default P0 threshold.

No real Chrome connections, live daemon operations, credential inspection, or user mcporter configuration edits were used for proof. No changelog or dependency-version changes.

Build daemon identity from effective per-server relay settings so semantically equivalent controls and fully overridden process values no longer restart compatible daemons. Normalize policy aliases, timeout defaults and clamping, endpoints, and default-profile spellings through the shared routing resolvers.

Preserve true policy, profile, endpoint, credential-directory and key-rotation, discovery, executable-selection, and placeholder boundaries. Keep require fail-closed. Advance identity metadata to version 2 for one-time replacement of older daemon metadata.

Config-path and layer isolation remain unchanged. Sharing daemons across separate temporary configs, including relay-off configs, is explicitly outside this normalization fix.

Verified unchanged patch: pnpm check passed; pnpm test passed with 199 files and 1825 tests, with 4 files and 26 tests skipped; focused suites passed 164 tests. Independent Codex autoreview was scoped-clean at the default P0 threshold. Proof used synthetic fixtures only.
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 81241188ca

ℹ️ 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 +625 to +630
endpoint:
policy === 'off'
? 'disabled'
: explicit.explicit
? (explicit.url?.toString() ?? 'invalid-endpoint')
: DEFAULT_RELAY_IDENTITY_URL,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep explicit and discovered endpoint modes distinct

When a daemon started without an explicit URL discovers endpoint X, a later caller setting MCPORTER_CHROME_DEVTOOLS_RELAY_URL=X produces the same identity and therefore reuses that daemon. The daemon retains its original environment, however, so after a transport recycle it performs discovery again and may connect to a newly reported endpoint Y, ignoring the caller's explicit X. Include whether the endpoint is explicit or discovered in the identity so this mode change restarts the daemon.

Useful? React with 👍 / 👎.

Keep relay fixtures inside the isolated home when clearing discovery
variables would otherwise make Windows temp roots disappear. Snapshot all
environment aliases before mutation so case-insensitive Windows keys are
restored correctly.

Guard synthetic relay credential paths using native path containment,
including Windows separators, case handling, and paths outside fixture
roots.

Restore absent importer fixture overrides by removing them instead of
assigning the string undefined, avoiding unintended paths and EISDIR.

These are fixture corrections only; production relay behavior is unchanged.
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 4, 2026
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 4, 2026, 4:59 PM ET / 20:59 UTC.

ClawSweeper review

What this changes

The PR changes Chrome DevTools relay daemon freshness identity to compare normalized effective per-server relay settings and adds regression and test-fixture coverage.

Merge readiness

Blocked before merge - 3 items remain

Keep this PR open: its normalized identity still equates a discovered relay endpoint with an explicitly configured endpoint, allowing a daemon with discovery-based behavior to be reused for a caller that requires a fixed endpoint.

Priority: P2
Reviewed head: 45bb65f5d54c2f0a89e72f50a7aad66cf8f86897

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The PR has substantial focused coverage, but a central daemon-reuse compatibility case remains incorrect.
Proof confidence 🌊 off-meta tidepool Not applicable: The author is a COLLABORATOR, so the external-contributor real-behavior-proof gate does not apply; the supplied synthetic identity and daemon-client fixtures are relevant supplemental evidence but do not cover the equal-endpoint mode transition.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The author is a COLLABORATOR, so the external-contributor real-behavior-proof gate does not apply; the supplied synthetic identity and daemon-client fixtures are relevant supplemental evidence but do not cover the equal-endpoint mode transition.
Evidence reviewed 5 items Introduced identity omits endpoint mode: The new per-server identity records policy, timeout, eligibility, credential state, discovery inputs, and working directory, but not whether the final endpoint was explicit or discovered. The later relay record reduces both modes to the same endpoint string.
Daemon reuse relies on this identity: Freshness compares the stored relay identity with one resolved from the later caller environment; equality preserves the original daemon and its original discovery behavior.
Existing focused tests miss the equal-endpoint transition: The staleness table covers an explicit URL that differs from the discovered/default endpoint, but not a daemon that discovered X followed by a caller explicitly selecting that same X.
Findings 1 actionable finding [P2] Keep explicit and discovered endpoint modes distinct
Security None None.

How this fits together

MCPorter reuses local keep-alive daemons for configured MCP servers. Relay policy, credentials, discovery, and endpoint selection determine whether the running daemon can be safely reused or must restart.

flowchart LR
  A[Server configuration] --> B[Effective relay settings]
  C[Process environment] --> B
  B --> D[Endpoint source]
  D --> E[Daemon identity]
  E --> F{Fresh daemon?}
  F -->|yes| G[Reuse daemon]
  F -->|no| H[Restart and connect relay]
Loading

Before merge

  • Keep explicit and discovered endpoint modes distinct (P2) - When a daemon originally discovers endpoint X, a later caller that explicitly configures X receives the same identity and reuses that daemon. The daemon keeps its original environment, so a later transport recycle performs discovery again and may connect to Y instead of the caller's fixed X. Encode endpoint mode in the identity and cover this equal-endpoint transition.
  • Resolve merge risk (P1) - An explicit endpoint equal to the daemon's currently discovered endpoint can reuse a discovery-mode daemon; after that daemon recycles its transport, it may rediscover a different endpoint and disregard the later caller's explicit selection.
  • Complete next step (P2) - Preserve explicit-versus-discovered relay endpoint mode in daemon identity and add a regression for discovered-X followed by explicit-X.

Findings

  • [P2] Keep explicit and discovered endpoint modes distinct — src/chrome-devtools-relay.ts:604-618
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +166/-121, tests +551/-80, docs +5/-1 The substantial regression suite still omits the central equal-endpoint mode-transition case.

Merge-risk options

Maintainer options:

  1. Preserve endpoint mode before merge (recommended)
    Encode explicit versus discovered endpoint mode in daemon identity and cover the equal-endpoint reuse transition with a freshness regression.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Preserve explicit-versus-discovered endpoint mode in relay identity and add focused identity and daemon-staleness regressions for discovered-X followed by explicit-X.

Technical review

Best possible solution:

Include the endpoint source or mode in the effective daemon identity, then add a focused identity and daemon-freshness regression for discovered-X followed by explicit-X.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible: start a daemon without an explicit URL that discovers endpoint X, then invoke it with an explicit URL of X. The identity matches although the retained daemon can later rediscover a different endpoint.

Is this the best way to solve the issue?

No. The normalization must retain explicit-versus-discovered endpoint mode, because equal endpoint strings do not guarantee identical behavior after transport recovery.

Full review comments:

  • [P2] Keep explicit and discovered endpoint modes distinct — src/chrome-devtools-relay.ts:604-618
    When a daemon originally discovers endpoint X, a later caller that explicitly configures X receives the same identity and reuses that daemon. The daemon keeps its original environment, so a later transport recycle performs discovery again and may connect to Y instead of the caller's fixed X. Encode endpoint mode in the identity and cover this equal-endpoint transition.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 9579ed64fed0.

Labels

Label changes:

  • add P2: The defect can make a caller's explicit relay endpoint stop being honored after an otherwise valid daemon reuse and later transport recycle.
  • add merge-risk: 🚨 compatibility: Merging can preserve a daemon whose endpoint-selection mode conflicts with a later caller's existing explicit relay configuration.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author is a COLLABORATOR, so the external-contributor real-behavior-proof gate does not apply; the supplied synthetic identity and daemon-client fixtures are relevant supplemental evidence but do not cover the equal-endpoint mode transition.

Label justifications:

  • P2: The defect can make a caller's explicit relay endpoint stop being honored after an otherwise valid daemon reuse and later transport recycle.
  • merge-risk: 🚨 compatibility: Merging can preserve a daemon whose endpoint-selection mode conflicts with a later caller's existing explicit relay configuration.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author is a COLLABORATOR, so the external-contributor real-behavior-proof gate does not apply; the supplied synthetic identity and daemon-client fixtures are relevant supplemental evidence but do not cover the equal-endpoint mode transition.

Evidence

Acceptance criteria:

  • [P1] pnpm check.
  • [P1] pnpm test.
  • [P1] Focused relay identity and daemon freshness tests covering discovered-X followed by explicit-X.

What I checked:

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Preserve explicit-versus-discovered endpoint mode in identity and add identity plus daemon-staleness coverage for the discovered-X to explicit-X transition.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit add13af into main Sep 4, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant