Skip to content

fix(browse): scope factory shutdown teardown to caller config#1652

Open
spacegeologist wants to merge 6 commits into
garrytan:mainfrom
spacegeologist:claude/tender-lovelace-DarGa
Open

fix(browse): scope factory shutdown teardown to caller config#1652
spacegeologist wants to merge 6 commits into
garrytan:mainfrom
spacegeologist:claude/tender-lovelace-DarGa

Conversation

@spacegeologist
Copy link
Copy Markdown

@spacegeologist spacegeologist commented May 22, 2026

Reviewer Guide

The core fix is intentionally narrow:

  • browse/src/server.tsbuildFetchHandler(...).shutdown() now tears down paths from the caller-provided cfg.config and cfg.chromiumProfile, not the module-level default config captured at import time.
  • browse/test/server-embedder-terminal-port.test.ts — regression coverage now uses per-test temp state dirs and asserts sibling daemon files are not touched.
  • CHANGELOG.md, VERSION, package.json, and TODOS.md are release/ship metadata for the resolved TODO item.

Maintainer note: the behavior fix is fully contained in browse/src/server.ts plus the focused test file. If release metadata is better batched into the next maintainer fix wave, the metadata-only files can be dropped without changing the code fix.

Why

The v1.42 ownsTerminalAgent work made the ownership boundary explicit, but shutdown() still resolved some cleanup targets from module-level config. That is invisible for the normal gstack CLI path because the CLI and module config point to the same state/profile locations.

Embedders and tests can pass a divergent cfg.config or cfg.chromiumProfile. In that case shutdown could remove terminal-port, terminal-internal-token, the state file, or Chromium singleton locks from the wrong location. This PR makes factory teardown respect the config the factory was built with.

Changes

  • Use cfg.config.stateFile for terminal discovery-file cleanup and state-file cleanup.
  • Use resolveChromiumProfile(cfg.chromiumProfile) for Chromium singleton-lock cleanup.
  • Move embedder shutdown tests away from the real resolved state dir and into per-test temp dirs.
  • Add regression tests for caller-passed state dirs and caller-passed Chromium profiles.
  • Remove the resolved TODOS.md item and update version/changelog metadata.

Risk / Scope

  • Low risk for the normal CLI path: the CLI still passes the same config/profile the old module-level defaults resolved to.
  • The changed behavior only matters for embedders or test harnesses that intentionally pass divergent config/profile paths.
  • The test change is also a safety improvement: the suite no longer backs up/restores a live daemon's terminal files.

Verification

  • bun test browse/test/server-embedder-terminal-port.test.ts — 6 pass, 0 fail.
  • bun run gen:skill-docs --dry-run.
  • package.json version matches VERSION.
  • git diff --check upstream/main...HEAD.

Note: an expanded local run, bun test browse/test/server-embedder-terminal-port.test.ts browse/test/config.test.ts, reached the new focused tests successfully, then hit 3 existing config.test.ts isServerHealthy failures from port 0 server startup in this environment. Those failures are unrelated to this PR.

claude and others added 5 commits May 22, 2026 08:22
…ule-level config

buildFetchHandler's shutdown() resolved its cleanup paths from the
module-level config captured at import time instead of the cfg.config the
caller passed in. Embedders that build the factory against a divergent
cfg.config (gbrowser phoenix overlay, a test harness on a temp dir) would
delete terminal-port / terminal-internal-token / the state file from the
wrong directory and clean Chromium singleton locks from the wrong profile.

Read cfg.config.stateFile and resolveChromiumProfile(cfg.chromiumProfile)
so teardown respects the config the factory was built with. The CLI path is
unchanged (it resolves the same paths either way).

https://claude.ai/code/session_019hZ9Xp2hGSQ5yJG36QCd23
Now that shutdown() honors cfg.config, the suite no longer has to back up
and restore a live daemon's terminal-port / terminal-internal-token to avoid
clobbering a running session. Each test gets its own mkdtempSync state dir.

Adds a regression test asserting shutdown cleans the caller's cfg.config dir
while leaving a sibling session's state dir untouched. Reverting the
server.ts fix turns 3 of the 5 tests red.

https://claude.ai/code/session_019hZ9Xp2hGSQ5yJG36QCd23
@spacegeologist spacegeologist changed the title fix(browse): scope factory shutdown() teardown to caller-passed cfg.config fix(browse): scope factory shutdown teardown to caller config May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants