[janicduplessis]

Description

stim stop leaves the adb reverse tcp:<port> tcp:<port> that android --device added on a physical phone. The forward outlives the workspace, and once Stim hands that port to another workspace's Metro, apps on the phone that open localhost:<port> reach that other server (#4132). #4105 removes the same reverse for owned emulators; a --device phone is used, not owned, and was skipped.

The device lease cannot carry this: a --device run releases its run lease when the command exits, so no lease names the phone by the time stop runs. Nothing else recorded the serial.

Solution

Before each android --device launch against Metro, the run records { serial, ports } under a new phoneReverses key in the workspace state.json, keyed by device slot. The ports of repeated runs on the same serial accumulate, so a Metro port that changed between runs is covered too. stop and worktree remove (reclaim) read that record, list adb reverse on the recorded serial, remove only tcp: reverses whose host side is a recorded port, and drop the record. Every other reverse on the phone stays. The adb calls go through the existing exec wrapper with the same bounded timeout as the emulator path, and an unreachable phone does not fail the stop.

stop follows #4105's ordering rule: it removes the reverses right before it signals Metro or finds none listening or no port reserved. It leaves them when it refuses to signal an unverified supervisor, since the workspace still owns the port then. stop --slot keeps Metro running and removes only that slot's phone.

This adds state for a physical device beyond its lease, which CLAUDE.md invariant 2 said did not exist. I updated that paragraph, the agent and cleanup guides, and website/docs/commands.md. The record is the issue's own "Fix idea". Tell me if you would rather keep the serial out of state.json.

The record is dropped after each attempt even when adb could not list the phone (timeout, unplugged, unauthorized). An adb reverse dies with its USB transport, so for an unplugged phone that is correct; a transient adb failure on a connected phone would leave the reverse behind.

Not handled: if a slot's --device run moves to a different phone, the previous phone's reverse stays, because the record is replaced. It is removed when that phone is unplugged or the adb server restarts.

Test plan

• New tests in stop.test.ts and reclaim.test.ts seed the record and assert the exact adb calls. They fail on origin/main (6 failures) and pass here:
  • removal before the supervisor signal, with no dev server, and with no reserved port;
  • nothing removed and the record kept for an unverified supervisor;
  • --slot removes only that slot's phone;
  • an offline phone does not fail the stop;
  • reclaim removes the reverses.
• android-command.test.ts: a physical run records { serial, ports: [8082] } and no other serial in workspace state.
• Real adb on RFCR7081Q9L, using only a forward I added on unused port 48711 (the phone's existing tcp:8085 forward is not mine and was left in place). recordPhoneReverse then removePhoneReverses against a scratch STIM_HOME:
  • before: UsbFfs tcp:8085 tcp:8085 and UsbFfs tcp:48711 tcp:48711
  • after: UsbFfs tcp:8085 tcp:8085
• Not run: a full stim android --device and stim stop on the phone. The machine's load average is about 1000, so I skipped the native build.
• pnpm run format:check, lint, build, typecheck, knip, test:runtime pass. In pnpm test, 11 files failed at that load; 9 pass when rerun. server/device-host.test.ts (timeouts) and scripts/agent-benchmark/macos-processes.test.mjs (sandbox-exec) still fail, in files this change does not touch.

Fixes #4132

[janicduplessis]

Review of the diff against origin/main. Findings are ordered by severity; I found no blocker.

Findings

1. A state-lock failure aborts stop before it signals Metro. stopSupervisor and the "Metro identity same" branch of stopMetro call await releaseReverses() before any try/catch. removePhoneReverses ends in updateWorkspaceStateAsync, which can throw on lock timeout, abort or a write error. A contended or broken state.json lock would then make stop throw before the SIGTERM. The next statement in stopSupervisor wraps withWorkspaceStateLockAsync(... requestDevServerStop ...) in try {} catch {}, so the codebase already treats that lock as fallible on this path. Wrap the phone-reverse step so an error is reported and stop continues, as the PR body says for an unreachable phone. Only the adb part is covered by that today.

2. The record is dropped even when removal was not confirmed. runFileQuietAsync turns every failure into null: an adb timeout (5 s), an adb server restart, an unauthorized device, or a phone that is unplugged. If reverse --list returns null, nothing is removed and removePhoneReverses still deletes the record. For an unplugged phone that is right, because the reverse is gone with the transport. For a transient adb failure on a connected phone the reverse leaks, and stop printed nothing, so nobody can tell. Consider a report line when --list yields null, or keep the record in that case. If you keep the current behavior, say so in the PR body, since the "unreachable phone" test only covers dropping the record.

3. The final update compares only the serial. In the closing updateWorkspaceStateAsync, remaining[key]?.serial === done.serial deletes the whole slot record. stop reads the record, runs adb for seconds, then writes. A concurrent android --device run on the same slot and serial that adds a new port in that window loses that port, and its reverse leaks. Compare ports as well, or delete only the ports that were removed. This is low likelihood.

4. The docs overstate the scope. AGENTS.md says "exactly that reverse and nothing else". The code matches any tcp: device port whose host side equals a recorded port. That includes a reverse the user added earlier to the same host port. One case is --no-metro-check with no reservation: the port falls back to 8081, so stop removes the user's own tcp:8081 tcp:8081. The emulator path has the same semantics. Reword to "the reverses that point at the recorded Metro ports".

Repo-rule items

5. Comment policy. The JSDoc on removePhoneReverses restates the code and links an internal Stim issue. That is neither an external constraint nor an API contract. Delete it. The emulator comment it was copied from stays because it names adb's services.cpp.

6. Guide placement. The new sentence in guide/agent.ts is cleanup detail about adb reverse. CLAUDE.md keeps the root guide agent for actions, safety and routing, and puts specifics in topics. guide/cleanup.ts and the website already carry it, so the agent.ts sentence can go.

7. Tests.

   • The stop.test.ts assertion expect(readWorkspaceState(tmpRoot)?.phoneReverses !== undefined).toBe(expected.length === 0) reads backwards. State what is kept, for example toBeDefined() in the no-removal case and toBeUndefined() otherwise, or split the cases.
   • notePhoneReverse has several skip conditions: not physical, remoteDevice, non-Metro runtime, metroPort not a number. Only the positive case is tested. A regression that records an emulator or a release run would make stop run adb against a serial the run never reversed. One negative test, for a release variant or an emulator, would cover it.
   • There is no test for Metro that is running but not Stim's (not-managed). There the reverse and the record are intentionally kept.

Checked and fine

• Ordering in stop.ts: removal runs before SIGTERM on the supervisor, and in the orphan-server, missing and no-reserved-port branches. It is skipped for an unverified supervisor and for not-managed or refused Metro, where the workspace still owns the port. stop --slot X builds a project with metroPort: null, so it takes the reservedPort === null branch and removes only that slot's phone.
• reclaim.ts: the removal runs inside withIdleWorkspace, before Metro is killed and before the workspace directory is emptied or archived.
• Record coverage in launch.ts: every physical Metro launch records before runtime.launch. metroPort is always a number for the Metro runtime. Release, embedded-JS and remote launches add no reverse, so skipping them is correct. A launch that fails after the reverse is added is still recorded.
• Fallback reverse: the tcp:8081 -> tcp:<port> fallback is covered because matching is on the host side.
• Source rules: no non-ASCII characters in the changed files under packages/.
• Tests: pnpm test packages/stim-cli/src/__tests__/stop.test.ts -t "adb reverses" passes (6 tests). The new tests import phone-reverses.ts, so they fail on the old code.
• Known gap: moving a slot to a different phone replaces the record, and the old phone keeps its reverse. The PR body already lists this.

[janicduplessis]

Re-review of cc26199 (fix: keep stop going when the phone reverse record cannot be updated).

No blocking findings.

• The ports filter in removePhoneReverses is correct: it matches on serial, subtracts only the ports this call removed, and deletes the slot record only when no ports remain. A port recorded by a concurrent run on the same serial between the adb removal and the state update is kept.
• The catch in releasePhoneReverses rethrows on abort and otherwise reports a warning. phoneReversesRemoved is set before the attempt, so a failure is not retried in the same stop. The record is left in place on failure, so a later stop or worktree remove retries it. runOperationFileQuiet already turns adb failures into null, so the catch is only reached by state read/write or lock errors. That matches the 'unreachable phone' test still passing.
• Non-blocking: the new catch branch (state update failing) has no test, and nothing exercises a partial-ports update. Both are narrow, and adding either would need a failing or racing state write, so I would leave them.
• Non-blocking: the dropped guide sentence means agent guidance no longer says that stop and worktree remove remove the adb reverse on a physical Android phone. AGENTS.md still documents it. Restore it if you want the guide to keep matching the behavior.

I did not run the test suite.