fix: make the up failed-boot test hermetic, discriminating, and leak-proof - #17
Merged
Merged
Conversation
The test's premise is 'no services.yaml in cwd', inherited from the bun test process (the checkout root). A stray `offbook init` in the root silently inverts it: the spawned server boots successfully, the test fails, and the detached server leaks onto the pinned fixture ports — after which every later run fails differently, with the R-043 port-conflict attribution instead of the doctor hint. Pin cwd to an empty dir under the test's tmp for the test's duration, and `down` the run-dir in the finally so a future regression tears the server down instead of leaking it. Verified by planting init artifacts in the checkout root: the test now passes and leaks no listeners.
Two gaps from the 2026-08-12 adversarial review round (recorded in the docs/archive/intake/2026-08-12-first-light-acceptance-fixes.md addendum): - The boot case's only assertion (the doctor hint) also appears in the generic preflight-busy message, so a stray squatter on 19141/12997 greened the test with the boot path never running. It now also asserts the boot-only 'server failed to start' header. Red-verified: with a planted listener on 19141 the case fails on the new assertion; without it, 45/0. - The finally's down cannot reach a bound-but-never-ready server (up clears the runfile after the readiness deadline), so that regression class leaked silently. A bind-probe now asserts all three fixture ports are free on the green path; the probe was verified against a real busy port. Residual: a red run of that class still leaves the orphan for a human — the test can no longer pass while leaking.
Boot-capable tests inherit process.cwd() into the spawned server's projectDir; the 2026-08-12 incident (stray init at the checkout root inverted the no-services.yaml premise and leaked the server onto the pinned fixture ports) is now recorded with the pattern that prevents it.
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
1 participant
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.
What this PR does
This PR changes one test and adds one working note. The test is
up: busy port and failed boot both point at doctorintest/cli-dispatch.test.ts. The tool code does not change.Why
An incident showed three weak points in the test.
The incident: a stray
offbook initat the checkout root added aservices.yamlfile. The test believed that the current directory has noservices.yaml. The spawned server found the file and started correctly. The test failed. The server stayed alive and kept the fixture ports. Each later test run then failed with the wrong error message.A review of the incident fix found two more weak points:
downcommand cannot stop a server that binds its ports but does not become ready. Theupcommand removes the runfile after the deadline. Thedowncommand then finds nothing. The server stays alive and the test stays green.The commits
78c9c5a— The test now sets the current directory to an empty scratch directory. Thefinallyblock sets it back and runsdown. A stray file at the checkout root cannot change the test premise.73b9022— The failed-boot case now also examines the output for "server failed to start". Only the boot path writes this header. A bind-probe then makes sure that the three fixture ports are free. A server that stays alive makes the test fail. It cannot stay hidden.38282b3— A working note inAGENTS.mdrecords the incident and the pattern. New boot-capable tests must obey the pattern: set the current directory, make sure that the ports are free, and examine the output for a path-specific marker.Test evidence
Each change has a planted check:
initfiles at the checkout root again. The old test failed and leaked. The new test passed and did not leak.Gates
bun scripts/check-docs.ts: exit 0.bun run lint: exit 0.bun run typecheck: exit 0.bun test: 570 pass, 0 fail, exit 0.Known limit
A server that binds its ports but does not become ready stays alive after a red run. The test cannot stop it, because the runfile is gone. A person must stop it. The test now shows the leak. Before, the test hid it.
Relation to PR #16
This PR is independent of PR #16. The two branches touch different files. The merge order is free.