[Dani Akash (DaniAkash)]

Summary

The Codex extension started as a fork of the Claude Desktop extension's wrapper/discovery code and then fell behind while the Claude extension hardened its discovery, testing, and release story. This PR closes that delta.

The headline fix: discovery now reads the same ~/.browserclaw/ sources that claw-server (the BrowserOS agent browser) actually writes. The plugin previously read ~/.browseros/server.json — a path current builds never write — so it leaned entirely on a blind 9100/9200/9300 port sweep and failed whenever BrowserOS ran on a custom port.

What changed (as 8 successive commits)

1. Read from ~/.browserclaw/runtime.json + BROWSERCLAW_URL_OVERRIDE. Point discovery at the path claw-server writes; rename the override env var to match the Claude extension. BROWSEROS_URL_OVERRIDE stays accepted as a legacy alias so existing ~/.codex/config.toml entries keep working.
2. Multi-source on-disk discovery + /system/health. Port on-disk-discovery.js: runtime.json → mcp-manager/manifest.json → claw-server.log tail → default :9200, each probe-tested against the endpoint claw-server actually serves. Replaces the port sweep.
3. Tagged discovery outcomes + loopback guard. discoverBaseUrl returns running / override-not-loopback / override-unreachable / not-installed / installed-not-running; the wrapper renders a distinct, actionable message per state. The user override is restricted to loopback hostnames so a misconfigured value can't forward Codex's MCP traffic to a public host. (The single-flight reconnect, dedup'd logging, and signal-handler teardown were already present from the original port; the one delta — re-tagging a URL-found-but-connect-failed as installed-not-running — ships here since it's inseparable from the tagged-state handling.)
4. Unit tests. Loopback guard, env-var precedence, and the three on-disk readers. 28 tests, run in CI by a new node-only test.yml.
5. Smoke harness. Spawns the bundled dist/wrapper.js and drives it over stdio through the disconnect / loopback-rejection / happy paths.
6. Release workflow. On a v* tag: verify the tag matches both plugin.json and package.json, rebuild and check dist/ for drift, cut a GitHub Release with generated notes.
7. REVIEWER.md. Clean-machine-to-working-test walkthrough for plugin reviewers.
8. Version bump to 0.2.0.

Intentionally NOT changed

• The plugin slug (browseros), tool prefixes (mcp__browseros__*), marketplace identity, and the BrowserOS user-facing brand.
• The Bun-bundled dist/wrapper.js install model (Codex has no install hook, so a committed single-file bundle stays correct — no .mcpb, no shipped node_modules).
• SKILL.md and the plugin.json interface block (Codex-specific, no Claude counterpart).

Test plan

• npm test in plugins/browseros → 28 pass / 0 fail.
• bun run build reproduces the committed dist/ (build-check CI gate is green).
• Smoke harness verified end-to-end against a live claw-server: sad, non-loopback, and happy paths all pass; happy path connected and listed 23 forwarded tools.
• Discovery override + probe behavior spot-checked (dead loopback → null/unreachable; public/LAN/IPv6 → rejected; legacy env alias honored; new name wins over legacy).

[greptile-apps]

Greptile Summary

This PR brings the BrowserOS Codex plugin to feature parity with the Claude Desktop extension by overhauling discovery, adding tagged error states, and wiring up CI, tests, and a release workflow.

• Discovery rewrite: replaces the old ~/.browseros/server.json read + 3-port parallel sweep with a proper multi-source chain — runtime.json → mcp-manager/manifest.json → claw-server.log → default :9200 — each probe-tested against /system/health. The user-configurable override is now restricted to loopback addresses so a misconfigured config.toml entry cannot redirect MCP traffic off-machine.
• Tagged error states: discoverBaseUrl now returns a discriminant (running / override-not-loopback / override-unreachable / not-installed / installed-not-running) that the wrapper maps to distinct, actionable user messages; BROWSEROS_URL_OVERRIDE is preserved as a legacy alias.
• Testing and CI: 28 unit tests (loopback guard, env-var precedence, all three on-disk readers), a smoke harness driving the bundled dist/wrapper.js over stdio, a test.yml CI job, and a release.yml workflow that guards against version skew and dist drift.

Confidence Score: 3/5

The discovery and error-message machinery is sound for every named failure mode, but a gap in the catch block of tryOpenInner means unexpected connection errors produce a blank user-facing message — a silent regression on a code path that previously always emitted the down-message.

The blank-message regression in wrapper.js affects the else-branch of the inner-connect catch: any non-TransportConnectError leaves state.lastResult frozen at { state: 'running' }, so messageForResult returns '' and Codex surfaces an empty error to the user. That behavior is worse than what existed before this PR. The dead host === '[::1]' check in isLoopback is harmless in practice but is dead code by the team's own standard. Everything else — the multi-source on-disk discovery, the loopback guard, the tagged states, the CI workflows, and the test coverage — looks correct and well-structured.

plugins/browseros/server/wrapper.js — the else-branch of the TransportConnectError catch in tryOpenInner needs state.lastResult updated to prevent the empty-message regression.

Important Files Changed

Filename	Overview
plugins/browseros/server/wrapper.js	Updated to consume the tagged DiscoveryResult and render distinct user-facing messages per state. The else-branch of the TransportConnectError catch in tryOpenInner leaves state.lastResult as running, causing messageForResult to return an empty string for any unexpected inner-connect error — a regression from the old static message.
plugins/browseros/server/discovery.js	Major rewrite: replaces the old port-sweep with multi-source on-disk discovery (runtime.json → manifest → log → default) and returns a tagged discriminant. Contains a dead host === '[::1]' branch in isLoopback — URL.hostname always strips brackets from IPv6, so '::1' already handles IPv6 loopback correctly.
plugins/browseros/server/on-disk-discovery.js	New file with three pure reader functions (readRuntimeUrl, readManifestUrl, readLogUrl). Defensive throughout: all error paths return null, the log tail correctly discards partial first lines, and the manifest reader handles both string and tagged-object transport shapes.
plugins/browseros/test/discovery.test.js	Unit tests for loopback guard and env-var precedence. Covers rejection of public URLs, LAN IPs, and non-loopback IPv6, plus acceptance of all three loopback variants.
plugins/browseros/test/on-disk-discovery.test.js	Unit tests for all three on-disk readers using scratch temp dirs. Covers missing files, malformed JSON, wrong fields, both transport shapes, multi-line log parsing, and the 1 MiB tail-truncation edge case.
.github/workflows/release.yml	New release workflow: verifies tag matches both plugin.json and package.json versions, checks for dist/ drift, then cuts a GitHub Release. Third-party actions are pinned to immutable SHAs.
.github/workflows/test.yml	New CI workflow running unit tests on Node 20 for any PR touching server/ or test/. Minimal and correct; third-party actions are SHA-pinned.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[discoverBaseUrl] --> B{BROWSERCLAW_URL_OVERRIDE\nor BROWSEROS_URL_OVERRIDE set?}
    B -- yes --> C{isLoopback?}
    C -- no --> D[override-not-loopback]
    C -- yes --> E{probeHealth}
    E -- ok --> F[running / override]
    E -- fail --> G[override-unreachable]
    B -- no --> H[readRuntimeUrl\n~/.browserclaw/runtime.json]
    H --> I{normalizeUrl + probeHealth}
    I -- ok --> J[running / runtime]
    I -- fail --> K[readManifestUrl\nmcp-manager/manifest.json]
    K --> L{normalizeUrl + probeHealth\nif different from runtimeUrl}
    L -- ok --> M[running / manifest]
    L -- fail --> N[readLogUrl\nclaw-server.log tail]
    N --> O{normalizeUrl + probeHealth\nif different from previous}
    O -- ok --> P[running / log]
    O -- fail --> Q{any source == DEFAULT_URL?}
    Q -- none match --> R{probeHealth DEFAULT_URL\n127.0.0.1:9200}
    R -- ok --> S[running / default]
    R -- fail --> T{isBrowserclawInstalled?\n~/.browserclaw exists}
    T -- no --> U[not-installed]
    T -- yes --> V[installed-not-running]
    Q -- already tried --> T

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[discoverBaseUrl] --> B{BROWSERCLAW_URL_OVERRIDE\nor BROWSEROS_URL_OVERRIDE set?}
    B -- yes --> C{isLoopback?}
    C -- no --> D[override-not-loopback]
    C -- yes --> E{probeHealth}
    E -- ok --> F[running / override]
    E -- fail --> G[override-unreachable]
    B -- no --> H[readRuntimeUrl\n~/.browserclaw/runtime.json]
    H --> I{normalizeUrl + probeHealth}
    I -- ok --> J[running / runtime]
    I -- fail --> K[readManifestUrl\nmcp-manager/manifest.json]
    K --> L{normalizeUrl + probeHealth\nif different from runtimeUrl}
    L -- ok --> M[running / manifest]
    L -- fail --> N[readLogUrl\nclaw-server.log tail]
    N --> O{normalizeUrl + probeHealth\nif different from previous}
    O -- ok --> P[running / log]
    O -- fail --> Q{any source == DEFAULT_URL?}
    Q -- none match --> R{probeHealth DEFAULT_URL\n127.0.0.1:9200}
    R -- ok --> S[running / default]
    R -- fail --> T{isBrowserclawInstalled?\n~/.browserclaw exists}
    T -- no --> U[not-installed]
    T -- yes --> V[installed-not-running]
    Q -- already tried --> T

Loading

Comments Outside Diff (1)

1. plugins/browseros/server/wrapper.js, line 180-183 (link)

   When openInnerClient throws something other than TransportConnectError (the else branch), state.lastResult is never updated — it retains whatever discoverBaseUrl returned, which is { state: 'running', url: ... }. Every subsequent call to messageForResult(state.lastResult) then returns '' (the empty-string fast-path for state === 'running'), so the user sees a blank error message both via throw new TransportConnectError('') and via the isError: true tool-result. Before this PR a static BROWSEROS_DOWN_MESSAGE was always non-empty, so this is a regression for any unexpected inner-client error.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: plugins/browseros/server/wrapper.js
   Line: 180-183
   
   Comment:
   When `openInnerClient` throws something other than `TransportConnectError` (the `else` branch), `state.lastResult` is never updated — it retains whatever `discoverBaseUrl` returned, which is `{ state: 'running', url: ... }`. Every subsequent call to `messageForResult(state.lastResult)` then returns `''` (the empty-string fast-path for `state === 'running'`), so the user sees a blank error message both via `throw new TransportConnectError('')` and via the `isError: true` tool-result. Before this PR a static `BROWSEROS_DOWN_MESSAGE` was always non-empty, so this is a regression for any unexpected inner-client error.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
plugins/browseros/server/wrapper.js:180-183
When `openInnerClient` throws something other than `TransportConnectError` (the `else` branch), `state.lastResult` is never updated — it retains whatever `discoverBaseUrl` returned, which is `{ state: 'running', url: ... }`. Every subsequent call to `messageForResult(state.lastResult)` then returns `''` (the empty-string fast-path for `state === 'running'`), so the user sees a blank error message both via `throw new TransportConnectError('')` and via the `isError: true` tool-result. Before this PR a static `BROWSEROS_DOWN_MESSAGE` was always non-empty, so this is a regression for any unexpected inner-client error.

```suggestion
    } else {
      logError('inner connect threw', err)
      state.lastResult = { state: 'installed-not-running', url: null }
    }
    return null
```

### Issue 2 of 2
plugins/browseros/server/discovery.js:71-76
The `host === '[::1]'` branch is dead code. The WHATWG `URL` API's `.hostname` property strips brackets from IPv6 literals — `new URL('http://[::1]:1').hostname` returns `'::1'`, not `'[::1]'`. The correct `host === '::1'` check on the next line already handles IPv6 loopback; this check can never fire. The associated test (`accepts IPv6 loopback [::1]`) passes because `'::1'` matches, not because `'[::1]'` matches.

```suggestion
    return (
      host === '127.0.0.1' ||
      host === '::1' ||
      host === 'localhost'
    )
```

_{Reviews (1): Last reviewed commit: "chore(release): 0.2.0" | Re-trigger Greptile}