fix(agent-gaia): stop the npm sidecar attaching to a foreign server

[kovtcharov]

Three ways npx @amd-gaia/gaia could mislead you, and one reason none of them was caught.

It reported success for a server it didn't own. Run gaia serve twice: the second sidecar fails to bind 8141 and exits, but the health probe gets an "ok" from the first instance and wins the race, so the CLI prints a ready URL for someone else's server. Ctrl+C then reports "already exited" and leaves the real one running.

It killed your sidecar when your own app handled an error. The crash and signal handlers reaped before checking whether we own the exit, so a host with its own uncaughtException listener — which keeps running by design — lost its sidecar anyway and got an unexplained ECONNREFUSED on the next request. Merely importing this package changed the semantics of the host's error handling.

It removed your tools from the child's PATH. Stripping our own gaia shim dropped the shim's whole directory, so on a Homebrew or ~/.local/bin layout python3, lemonade-server and the real gaia disappeared — and the TUI then reported "the gaia CLI is not on PATH", an error we had caused. A directory holding only our shim is still dropped; a shared one now moves to the end of PATH instead.

And gaia run never finished the job it documents. It stages a verified binary where the daemon looks, but never wrote the .installed record that means "completed install" — so the TUI treated the frozen REST sidecar as its own stdio child and filled the chat with uvicorn's startup log. Writing it also means the daemon supervises the process and mints its caller-auth token, which is the intended path.

The reason all of this shipped: the package had no PR-time CI at all. Only the release workflow touched it. That's added here, mirroring the email package's.

Also: the ~200MB download is now streamed and hashed incrementally rather than buffered whole (~400MB RSS peak), with the integrity gate unchanged — unverified bytes still never reach the final path.

🔍 Technical details

• startSidecar asserts the child is alive after the health wait, and the caller's abort signal is linked into getJson so an in-flight probe aborts the moment the child dies rather than at the next poll.
• weOwnTheExit() gates the reap on listenerCount === 1; process.on("exit", reapAllSync) stays as the backstop.
• Sentinel fields are a cross-repo contract with InstalledAgent.to_dict(). It is written on a cache hit too, so installs staged by an earlier release repair themselves.
• Smaller fixes: resolveSidecarPath/resolveTuiPath now verify against binaries.lock.json before returning a path to spawn; per-command rejection of --port/--component/--cache-dir where they do nothing; taskkill exit code + stderr surfaced; shutdown de-registers only on confirmed exit; cmdServe removes its signal handlers; --base-url requires https (with --allow-insecure-base-url); guarded JSON.parse; strict-digit port parsing.
• Not fixed, deliberately: no auth token is minted in spawnSidecar. That is a behaviour change to a published package and a product decision — raised in docs(agent-gaia): correct the npm sidecar docs against the code #3076.

Test plan

• cd hub/agents/gaia/npm && npm ci && npm run build && npm test — 136 pass (101 before)
• The new CI job runs exactly that on every PR touching hub/agents/gaia/npm/**
• Sentinel accepted by the real consumers, not just shaped like it: writing the exact JSON this emits, then calling gaia.hub.installer.read_sentinel and the daemon's _hub_installed_binary, returns a verified FetchResult (using hub-installed verified binary … v0.1.1)
• Untestable and stated as such rather than papered over: the taskkill branches (win32-only, no spawnable long-lived surrogate) and shutdown's survivor path (nothing can ignore SIGKILL)

[github-actions]

Request changes

This hardens the npx @amd-gaia/gaia launcher — it now records the install the daemon looks for, refuses flags a command would silently ignore, streams downloads instead of buffering them, re-verifies binaries before spawning them, and finally gets a PR-time CI job. The direction and the test coverage are both good. Three things need another look before merge.

The "second gaia serve no longer succeeds against someone else's server" fix probably won't fire. The new guard only trips if our own child has already died by the time the health and version probes finish. In the real conflict an incumbent server answers both probes in milliseconds while the ~200MB frozen sidecar is still unpacking, so the child is very much alive and the check passes — handing back a handle for a server we don't own, which is the bug the changelog says is fixed. A quick check that the port is already answering before spawning would be deterministic.

Fetching for another platform now corrupts the local install record. gaia fetch --platform win32-x64 on Linux writes the "gaia is installed" marker into the daemon's own directory, pointing at a Windows binary. The daemon then believes it has a usable agent and tries to run it. The marker should only be written when the fetch is for the machine it's running on.

The memory win misses the path users actually hit. Fresh downloads now stream, but the far more common already-cached run still reads the whole ~200MB binary into memory to check its hash — the same peak the changelog says was removed.

Real-world evidence

N/A — no evidence bundle was produced for this PR, and this environment can't reach the GitHub API, so I couldn't check whether the PR description links evidence elsewhere. The changed surface is a CLI (gaia run / fetch / serve from the npm package); the natural artifact is a real run of each against a local mirror. The new CI job does build and run the vitest suite on every PR, which covers the logic well, and the verdict above rests on static review plus that suite.

🔍 Technical details

Issues

🟡 assertOurs is timing-dependent and likely never fires in the real port-conflict case (src/lifecycle.ts:624, called at :656 / :661)

waitForHealth issues its first probe with no initial delay, so an incumbent sidecar already bound to 8141 answers /health in a few ms. checkVersion follows immediately. Our freshly spawned child — a PyInstaller-frozen binary that takes seconds to unpack before it even attempts the bind — is still alive at both assertOurs calls, so startSidecar returns the handle and cmdServe prints a ready URL for the foreign server. The child then dies with EADDRINUSE seconds later.

The test passes only because it forces the inverse timing: versionDelayMs: 750 on the stub plus diesInstantly() (a child that exits in ~25ms). That's the opposite ratio of the production case.

A deterministic version of the same check is a pre-flight probe before the spawn:

// in startSidecar, before spawnSidecar(opts)
const port = opts.port ?? DEFAULT_PORT;
if (await portAnswers(`http://${opts.host ?? DEFAULT_HOST}:${port}`)) {
  throw new SidecarExitedError(/* the same "already bound to port N" message */);
}

Keeping assertOurs as a backstop is still worthwhile — it's the pre-flight check that makes the guarantee real.

🟡 The .installed sentinel is written for a cross-platform fetch (src/fetch.ts:390, and the cache-hit copy at :304)

fetchBinary writes the sentinel whenever component === "sidecar", regardless of opts.platformKey. --sidecar-dir defaults to daemonSidecarCacheDir(), so on Linux:

npx @amd-gaia/gaia fetch --component sidecar --platform win32-x64

drops ~/.gaia/agents/gaia/gaia-agent.exe plus a sentinel claiming { artifact_kind: "binary", executable: "gaia-agent.exe" }. _hub_installed_binary (src/gaia/daemon/sidecars/fetch.py:126) then finds the file, re-hashes it successfully, and returns it as the agent binary — a wrong-arch executable the daemon will try to spawn. Before this PR the same fetch was inert, because the daemon's lock path looked for gaia-agent and re-downloaded.

Gate both call sites on the fetch being for this host:

  if (opts.component === "sidecar" && platformKey === currentPlatformKey()) {
    await writeInstalledSentinel({

🟡 The cache-hit path still buffers the whole binary (src/fetch.ts:169)

fileSha256 uses fsp.readFile, so every gaia run against an already-installed sidecar allocates ~200MB to compute the hash — the exact peak the ### Security changelog entry says was removed. fileSha256Sync in src/lifecycle.ts:966 already does this incrementally; the async twin should too:

export async function fileSha256(filePath: string): Promise<string | null> {
  const hash = crypto.createHash("sha256");
  let fh: fsp.FileHandle;
  try {
    fh = await fsp.open(filePath, "r");
  } catch (e) {
    const code = (e as NodeJS.ErrnoException).code;
    if (code === "ENOENT") return null;
    throw new Error(
      `cannot read the cached binary at ${filePath} to check its hash: ${(e as Error).message}. ` +
        "Fix the permissions on that path, or pass a different --cache-dir / --sidecar-dir.",
      { cause: e },
    );
  }
  try {
    for await (const chunk of fh.createReadStream()) hash.update(chunk);
  } finally {
    await fh.close();
  }
  return hash.digest("hex");
}

Nits

🟢 Unknown and boolean flags are still silently ignored (src/cli.ts:431) — VALUE_FLAG_SCOPE only covers VALUE_FLAGS, so gaia version --force, gaia version --allow-insecure-base-url, and any typo'd boolean (gaia run --forc) parse to true and vanish. That's the same class of problem the PR sets out to fix; an allowlist check for unrecognised flag names in main would close it.

🟢 installed_at is rewritten on every run (src/fetch.ts:95) — the cache-hit branch rewrites the sentinel unconditionally, so the recorded install time becomes "last launch". Worth preserving the existing installed_at when the sentinel is already valid, so the field means what its name says.

Strengths

• The tests earn their keep. pathWithoutOwnShim's shared-vs-exclusive bin-dir cases build real directories rather than asserting on strings, the .installed tests assert the exact three fields _hub_installed_binary gates on and that executable stays a bare filename, and url.test.ts pins the ReDoS rewrite that previously had no coverage at all.
• The .installed sentinel is documented as the cross-repo contract it actually is — SPEC §4.1 names InstalledAgent.to_dict(), the three load-bearing fields, and the read_sentinel path-separator rule, all of which check out against src/gaia/hub/installer.py.
• Docs stayed in sync the way CLAUDE.md asks: README, SPEC, SKILL, and CHANGELOG all carry the lifecycle, exit-code, and error-class changes, rather than one of them drifting.
• weOwnTheExit fixes a genuinely nasty library-etiquette bug — reaping before checking whether the host owned the signal — and the POSIX test proves the sidecar survives a host-handled SIGINT.
• The CI workflow mirrors test_agent_email_npm.yml closely enough to be obviously correct, including the thin-tarball assertion.

[github-actions]

Verdict: Approve

This is a careful, well-tested batch of fixes to the @amd-gaia/gaia npm sidecar launcher. The sentinel-write bug was a real silent failure (chat filling with uvicorn logs), the SHA re-verify on spawn closes a genuine pre-spawn substitution window, the streaming download eliminates a ~400MB RSS peak, and the signal-handler cleanup means Ctrl+C keeps working after serve exits. Every change is matched by tests and every affected doc surface (README, SPEC, SKILL, CHANGELOG) is updated in the same commit — exactly the pattern CLAUDE.md requires.

A few things worth noting:

• The weOwnTheExit check is evaluated at signal delivery time (not at registration), so a host library that adds its own SIGINT handler after installCleanupHandlers still works correctly.
• shutdown's liveSidecars.delete move to after process exit (not before the kill) is intentional and correct: a sidecar that survives both kill windows stays in the set so the process-exit reaper gets a second chance.
• assertFlagsInScope correctly fires before the command switch, so version --base-url https://… is refused before the scheme check even runs.
• --allow-insecure-base-url is not in VALUE_FLAG_SCOPE, so it's silently ignored on commands like version — harmless, since base-url IS in scope and is refused first.

No blocking issues found.

🔍 Technical details

streamToFile / incomplete-file cleanup (fetch.ts): the outer catch calls fsp.rm(tmp, { force: true }) to clean up even when tmp was never created (network error before open) — { force: true } makes this a no-op, so the cleanup path is always safe.

fileSha256Sync is synchronous and O(binary size) (lifecycle.ts:973-987): resolveSidecarPath/resolveTuiPath now hash a ~200MB file on each call. SPEC and SKILL both say "resolve once at startup"; the { lock } pass-through avoids a redundant lock read. No action needed — just confirming the design choice is correctly documented.

isExclusivelyOurs file-name comparison (cli.ts:532-545): the stem comparison is case-sensitive. On Windows with mixed-case filenames (GAIA, Gaia.cmd), the function returns false (treats the dir as shared and moves it to the end rather than dropping it) — which is the safe, non-destructive branch. No problem.

Thin-package invariant check in CI (.github/workflows/test_agent_gaia_npm.yml:77-103): the node -e inline script reads pack[0]?.files from the npm pack --dry-run --json output. The ?.[0] guard handles an empty array correctly (won't crash, files defaults to []), and the explicit process.exit(1) on offenders means CI fails loudly — consistent with the no-silent-fallbacks rule.

[github-actions]

Verdict: Approve with suggestions

This is a well-executed batch of hardening fixes. The sentinel write, streaming download, binary-verify-before-spawn, signal-handler cleanup, and foreign-server detection are all correctly implemented and tested. One 🟡 finding and one 🟢 nit below.

---

🟡 assertNotAForeignServer silences typed errors, so a proxy on the port produces the wrong diagnosis.

The catch block inside assertNotAForeignServer returns for any exception, including HttpError (non-2xx) and MalformedResponseError (proxy HTML 200). In those cases the child is dead, the port is occupied, but the function concludes "nothing is listening" and the caller rethrows HealthTimeoutError — "did not become healthy within Nms" — instead of SidecarExitedError. Not a crash, but a user debugging a proxy misconfiguration gets misleading output.

The primary case (second gaia serve with another gaia instance already up) is handled correctly. This is edge-only, but worth fixing since MalformedResponseError was added in this very PR precisely for the proxy scenario.

🟢 cmdRun / cmdFetch are missing from assertFlagsInScope test coverage.

The ignored-flags test includes ["--base-url", ["version", "--base-url", "..."]] but has no case asserting cmdRun refuses --port or --component, and cmdFetch refuses --port. The VALUE_FLAG_SCOPE table covers them, but the test matrix does not exercise every cell.

🔍 Technical details

assertNotAForeignServer fix — catch ECONNREFUSED specifically:

lifecycle.ts, around the assertNotAForeignServer function:

  try {
    if ((await health(sidecar.baseUrl, 1_000)).status !== "ok") return;
  } catch (e) {
    // ECONNREFUSED / ENOTFOUND → port is genuinely free; typed GaiaErrors
    // (HttpError, MalformedResponseError) mean something IS listening — fall
    // through to assertOurs().
    const code = (e as NodeJS.ErrnoException).code ?? "";
    if (code === "ECONNREFUSED" || code === "ENOTFOUND" || code === "ECONNRESET") return;
    // Also treat a HealthTimeoutError as "not listening" — no timely reply.
    if (e instanceof HealthTimeoutError) return;
  }

Alternatively, a simpler fix is to only return when health() throws something without a port answer:

  } catch (e) {
    if (!(e instanceof GaiaError)) return; // ECONNREFUSED etc. — nothing there
    // GaiaError means something answered but is not us
  }

Either way, the intent is: treat "connection refused / timed out" as "port is free", and let "port has a live server that misbehaves" fall through to assertOurs().

Missing test cases for assertFlagsInScope:

test/cli.test.ts IGNORED table — add:

["--port", ["run", "--port", "9000"]],
["--component", ["run", "--component", "tui"]],
["--port", ["fetch", "--port", "9000"]],

[github-actions]

Verdict: Approve with suggestions

A thorough set of bug fixes and security improvements (sentinel write, port pre-flight, streaming downloads, binary verification at resolve time). Tests are extensive. One 🟡 finding and two small nits.

---

🟡 Several tests hardcode ports that the CI machine may already have bound — those tests then fail with PortInUseError instead of the expected error type, silently mis-diagnosing the failure.

The test "keeps the health-timeout error when the port is genuinely free" depends on port 8189 being free; if it isn't, startSidecar exits with PortInUseError before spawning anything, so the assertion for HealthTimeoutError fails. The same applies to ports 8193, 8194, and 8195 used in the auto-cleanup ownership tests. A freePort() helper already exists in the file — use it for the free-port case, and bind-then-release for tests that need a live listener.

🟢 src/cli.ts — the help text for serve options embeds a backtick-escaped template literal in a tagged template string; the ``` is correct JavaScript but reads awkwardly. A plain string literal avoids the escape.

🟢 src/lifecycle.ts — fileSha256Sync allocates a 1 MiB buffer with Buffer.allocUnsafe. Since the buffer is fully overwritten before each hash.update(buf.subarray(0, n)) call, this is safe, but Buffer.alloc (zeroing) would remove the "do not read past n" contract from the call site entirely.

🔍 Technical details

🟡 Hardcoded ports

test/lifecycle.test.ts line ~2094:

// uses port 8189 — would PortInUseError if taken
const p = startSidecar({ binaryPath: diesInstantly(), port: 8189, ... });
await expect(p).rejects.toBeInstanceOf(HealthTimeoutError);

Lines ~2197, ~2228, ~2253 use 8195, 8194, 8193 respectively. Fix:

const free = await freePort();
const p = startSidecar({ binaryPath: diesInstantly(), port: free, ... });

For tests that need a listener already present on the port, bind a server first and pass its dynamic port.

🟢 Backtick escape in help text (src/cli.ts, serve-options block):

  --base-url, --sidecar-dir, --force as above. \`serve\` never downloads the TUI,

Using a plain string or a regular template literal avoids the ``` reader-speed bump.

🟢 Buffer.allocUnsafe in fileSha256Sync (src/lifecycle.ts):
Buffer.alloc keeps the invariant in one place rather than relying on the reader to notice the subarray(0, n) guard.

[kovtcharov]

All three fixed — and the first one was a fair hit on the core of the change, not a detail.

The guard didn't fire in the real case. You're right: an incumbent answers /health in milliseconds while the ~200MB frozen sidecar is still unpacking, so the child is alive at both assertOurs calls and we hand back a handle for a server we don't own. The tests passed only because they forced the inverse ratio. The port is now probed before anything is spawned, so the refusal no longer depends on which side wins a race — a taken port raises PortInUseError and nothing is started. assertOurs and the health-wait re-probe stay as backstops for the narrower case where something binds after the check.

The cross-platform sentinel was a regression I introduced. fetch --platform win32-x64 on Linux wrote a record claiming a Windows binary was installed, which the daemon would re-hash successfully and then fail to exec. Gated on component === "sidecar" && platformKey === currentPlatformKey(), both call sites.

The cache-hit hash now reads in chunks, so the changelog's memory claim is true on both paths instead of just the download.

The changelog entries for the port conflict and the sentinel were rewritten to describe what the code now actually does — the old port-conflict wording described the mechanism you showed doesn't happen.

🔍 Technical details

145 tests pass (137 before). Each fix was verified by neutering it and confirming the new test fails.

One test comment was walked back as overclaiming: the holder's ready-file only guarantees the holder is listening before the child exits, not which backstop then fires. Both raise the same error so the test is stable, but neutering one backstop alone still passes it — the comment now says that rather than implying determinism it doesn't have.

[github-actions]

Verdict: Approve with suggestions

Solid batch of fixes: port pre-flight, streaming downloads, chunked hashing, sentinel writing, signal ownership, and a new CI job. Documentation updated across all four surfaces (README, SPEC, SKILL, CHANGELOG) in one commit — exactly right. One 🟡 finding in the new tests risks making the CI job you just added intermittently unreliable.

---

🟡 The new CI job you're wiring may fail spuriously because the test it exercises hardcodes a port.

lifecycle.test.ts — "keeps the health-timeout error when the port is genuinely free" — calls startSidecar with port: 8189. startSidecar now opens with a TCP pre-flight (portInUse); if 8189 happens to be bound on the runner, that pre-flight throws PortInUseError instead of HealthTimeoutError and the assertion fails. The freePort() helper is defined twelve lines above in the same file specifically for this pattern — use it.

The rest of the hardcoded ports (8193, 8194, 8195) go through spawnSidecar, which skips the pre-flight, so they are safe.

---

🟢 --allow-insecure-base-url is not in VALUE_FLAG_SCOPE, so gaia version --allow-insecure-base-url silently passes assertFlagsInScope and is ignored rather than refused. Minor omission given that --base-url itself is already blocked for version.

🔍 Technical details

Port 8189 — lifecycle.test.ts line 303:

// before
const p = startSidecar({
  binaryPath: diesInstantly(),
  port: 8189,          // ← hardcoded; if taken, portInUse() → PortInUseError
  autoCleanup: false,
  healthTimeoutMs: 3_000,
});

// after
const port = await freePort();    // freePort() defined at line ~2050 in the same file
const p = startSidecar({
  binaryPath: diesInstantly(),
  port,
  autoCleanup: false,
  healthTimeoutMs: 3_000,
});

--allow-insecure-base-url scope — cli.ts VALUE_FLAG_SCOPE: the table only covers value flags, and boolean flags like allow-insecure-base-url are not listed, so no assertFlagsInScope check fires for them. Adding it is straightforward but low-stakes; it only matters if a user accidentally passes it somewhere it does nothing.