Three more from the refactor review: a duplicated host check, a dead waiter, and a rule with an unwritten exception

[brillout]

Findings 8–10 from the second review pass over #1536, one commit each. Follows #1571 and #1572.

e029e82 — one answer to "is this host local", and a test on the copy that decides access

isLoopbackHost existed twice and the two disagreed:

host	src/loopback-host.ts	dashboard/lib/profiles.ts
127.0.0.1	true	true
127.0.0.2	true	false
127.1.2.3	true	false
127.evil.com	false	false

Nothing was exploitable — the token gate and the Host check use the strict one; the browser's copy only labels the connection and remembers the local origin. What it was is two answers to one question, with the loose implementation sitting where someone looking for it would find it first.

loopback-host.ts imports nothing, so it is already the leaf shape client.ts re-exports for the browser — the same arrangement preference-defaults.ts uses to keep both sides on one set of values. The dashboard reads it from there now and its copy is gone.

The part worth more than the deduplication: the strict implementation had no test at all, while the loose one had three. The module that decides whether a request skips the token now has its own — the full 127.0.0.0/8 range, the rebound 127.evil.com name, bind-all and routable addresses, and the Host header's port and IPv6 brackets.

8a5d596 — the CLI's own interrupt waiter went out with the background daemon

waitForInterrupt in cli.ts had no caller. D4b made the daemon foreground-only and runDaemon blocks until signalled — cli.ts says exactly that two hundred lines above. The promise-on-SIGINT was what detached mode needed to stay alive after handing the dashboard off.

It is the only uncalled non-exported function in 147 modules, which is a fair report on the rest of that sweep.

e8354db — the removal rule admits the one deletion that is meant to lose work

MEMORY.md says anything deleted must already be on the remote, with no exception. deleteProjectAgent breaks that deliberately: it force-removes the worktree and discards uncommitted work, because the session is being thrown away — its doc calls it "the one destructive-of-history action, which is why the surfaces that call it confirm first."

The entry is the half that is wrong. Making delete push first would publish the work of a user who pressed a button meaning "throw this away". So the rule now binds what the framework does on its own initiative — the sweeps, teardown, retention — and names the confirmed delete as the exception.

What the same pass checked and found sound

E4 (seven jobs, one setInterval, no sweep kept a timer), E5 (removeProjectWorktree really is the one implementation behind every retention surface; stale-branch refuses anything with an open PR or no PR history), E1 (agent.ts: "Nothing stops a session for spending"), no unused dependencies, and — apart from the one above — no logic duplicated across the packages A7 merged.

Verification

After pnpm clean: both typechecks, a full build, 1452 node tests (+10: the new loopback suite), 773 dashboard tests (81 files) (−1: the loose copy's test moved to the module that owns the behaviour).

---

Generated by Claude Code