fix(browse): scope factory shutdown teardown to caller config#1652
Open
spacegeologist wants to merge 6 commits into
Open
fix(browse): scope factory shutdown teardown to caller config#1652spacegeologist wants to merge 6 commits into
spacegeologist wants to merge 6 commits into
Conversation
…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
The P3 "shutdown() reads module-level config" item shipped in this branch. https://claude.ai/code/session_019hZ9Xp2hGSQ5yJG36QCd23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewer Guide
The core fix is intentionally narrow:
browse/src/server.ts—buildFetchHandler(...).shutdown()now tears down paths from the caller-providedcfg.configandcfg.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, andTODOS.mdare release/ship metadata for the resolved TODO item.Maintainer note: the behavior fix is fully contained in
browse/src/server.tsplus 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
ownsTerminalAgentwork made the ownership boundary explicit, butshutdown()still resolved some cleanup targets from module-levelconfig. 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.configorcfg.chromiumProfile. In that case shutdown could removeterminal-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
cfg.config.stateFilefor terminal discovery-file cleanup and state-file cleanup.resolveChromiumProfile(cfg.chromiumProfile)for Chromium singleton-lock cleanup.TODOS.mditem and update version/changelog metadata.Risk / Scope
Verification
bun test browse/test/server-embedder-terminal-port.test.ts— 6 pass, 0 fail.bun run gen:skill-docs --dry-run.package.jsonversion matchesVERSION.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 existingconfig.test.tsisServerHealthyfailures fromport 0server startup in this environment. Those failures are unrelated to this PR.