Ignacy Łątka 13f43bd0cd test(chromium): pin the ports-file guard and the readiness failure code (#862) ...

Follow-up to #709, which merged while this was still under review. #709
landed the port fix; these are the two gaps a review pass over it turned
up, both in the same two files.

## The ports-file guard was pinned by nothing

#714 added `vi.mock("../src/utils/chromium-discovery", …)` to
`boot-electron-spawn-error.test.ts` so a completed boot stops persisting
its port into the developer's real `~/.argent/chromium-cdp-ports.json`.
Nothing asserted it still worked.

Measured, on the pre-change file: deleting the mock leaves the suite
**green at 9/9** while `$HOME/.argent/chromium-cdp-ports.json` is
created containing a live ephemeral port. Replacing it with a
call-through variant — the shape `list-devices.test.ts:76-81` already
uses on this same module — does the same:

```ts
vi.mock("../src/utils/chromium-discovery", async () => {
  const actual = await vi.importActual<...>("../src/utils/chromium-discovery");
  return { ...actual,
    trackChromiumPort: vi.fn(actual.trackChromiumPort),
    untrackChromiumPort: vi.fn(actual.untrackChromiumPort) };
});
```
→ `Tests 9 passed (9)`, and the ports file contains `[55770]`.

That is how the file accumulated entries before #714, so the guard
rotting back is not hypothetical. Each file now scopes `HOME` with
#714's own `scopeTempHome()` helper and asserts, after a completed boot,
that no ports file exists. Both mutations now fail.

**All three files that carry this mock are fixed, not just the one.**
`boot-electron-kill.test.ts` and `boot-electron-ready-race.test.ts` hold
the byte-identical mock — under a comment naming each other — and each
completes a real boot. Measured before this change: deleting the mock
left `kill` green at **15/15** while eleven live ports, one per boot,
were written; the call-through variant did the same to `ready-race`
**while still satisfying its own
`expect(trackChromiumPort).toHaveBeenCalledWith(port)` at line 175**,
which is precisely why the absence assertion is the right shape.

Asserting the absence rather than
`expect(trackChromiumPort).toHaveBeenCalledWith(port)` is deliberate:
the call is already pinned by `boot-electron-ready-race.test.ts:175`,
and a spy assertion only proves the export *is* a spy — the call-through
mutation satisfies it while still writing.

## The readiness timeout was pinned by its wording, not its code

The catch-path test matched `/CDP never became reachable/`.
`FAILURE_CODES.CHROMIUM_ELECTRON_CDP_TIMEOUT` had **no assertion
anywhere in the repo** — `grep -rn CHROMIUM_ELECTRON_CDP_TIMEOUT
packages/ --include=*.ts` outside `dist/` hits only the throw site and
the enum. Swapping the throw site to `CHROMIUM_ELECTRON_SPAWN_FAILED`
left `boot-electron-spawn-error` + `-kill` + `-ready-race` +
`failure-classification` + `chromium-discovery` at **60/60 passed**.

So the half agents and telemetry bucket on was free to change, while a
harmless reword would have broken the test. Now pinned by code via
`getFailureSignal`, matching how `boot-electron-ready-race.test.ts:137`
pins its own failure path. It still discriminates the vacuity the
message match was added for: a nonexistent `appPath` yields
`CHROMIUM_ELECTRON_APP_PATH_INVALID` and fails. `failure_stage` is
pinned alongside it, as `ready-race` already does for its own path —
`electron_cdp_ready` likewise had no assertion anywhere.

## Two rationales corrected

- The catch-path comment blamed "no HTTP server on the port" for the
probe failing. `fetch` declines port 1 as a WHATWG bad port *before it
dials* — verified with `net.connect` hooked: **0 sockets opened** for
port 1, 2 sockets for ports 2, 1023 and 19222. No listener is involved
either way, and the file's own constant JSDoc says so.
- The `chromium-discovery.test.ts` constant pointed at "a `listen(0)`
fake CDP server **in a sibling test file**", when the server that makes
the point is `startFakeCdpServer` in this same file, above the constant.
- It also credited "anything in the unprivileged range" with being
bindable by `listen(0)`. `listen(0)` draws from the kernel's *ephemeral*
range — `net.inet.ip.portrange` 49152-65535 on macOS,
`ip_local_port_range` 32768-60999 on the Ubuntu runner. That narrower
range is what put the old `43211` at risk; calling it "unprivileged"
also revives the privilege reasoning the sibling constant explicitly
disowns.

## `chromium-discovery.test.ts` gets a second layer too

It is the one file here that drives the **real** `persistPorts`, and its
only guard was a single `beforeAll` line setting
`ARGENT_CHROMIUM_PORTS_FILE`. That line is pinned — deleting it fails 3
tests — but the write lands before the detection, and `persistPorts`
merges rather than replaces, so on a real machine the test ports would
be appended for `list-devices` to probe afterwards. It now scopes `HOME`
as well, so a lapsed redirect falls back somewhere disposable. Measured
with a pre-seeded `$HOME/.argent/chromium-cdp-ports.json` = `[9222]`:
deleting the redirect still fails 3 tests, and the seeded file comes
back **unchanged**.

## Verification

| mutation | before | after |
|---|---|---|
| delete the module mock | 9/9 pass, ports file written | fails |
| call-through `vi.fn(actual.trackChromiumPort)` | 9/9 pass, ports file
written | fails |
| swap `CHROMIUM_ELECTRON_CDP_TIMEOUT` at the throw site | 60/60 pass |
fails |
| nonexistent `appPath` in the catch-path test | — | fails
(`APP_PATH_INVALID`) |
| swap `failure_stage` at the throw site | — | fails |
| delete / call-through mock in `boot-electron-kill` | 15/15 pass, 11
ports written | 11 failed \| 4 passed |
| call-through mock in `boot-electron-ready-race` | 2/2 pass | 1 failed
\| 1 passed |

Full `packages/tool-server` in 6 shards: 4303 passed, 1 skipped, every
shard exit 0, and no `.argent` directory created under a redirected
`HOME`. `eslint` 0, `prettier --check` 0, `tsc --build` 0, `tsc -p
tsconfig.test.json` 0, `knip` 0 — exit codes read directly.

2026-08-21 12:00:50 +02:00

17 KiB

Raw Permalink Blame History

View Raw