test(tool-output): stop expressing "unwritable path" as a magic absolute path - #4722
Open
DaoyuanLi2816 wants to merge 1 commit into
Open
test(tool-output): stop expressing "unwritable path" as a magic absolute path#4722DaoyuanLi2816 wants to merge 1 commit into
DaoyuanLi2816 wants to merge 1 commit into
Conversation
…ute path `test_returns_none_on_invalid_path` and `test_fallback_when_disk_write_fails` both need an `outputs_path` that `os.makedirs` refuses to create, so they can reach `_externalize`'s `except OSError: return None` branch. They spell that as the literal path `/dev/null/cannot-mkdir-here`, which only works where `/dev/null` is a character device. On Windows it is an ordinary relative path, so `os.makedirs` succeeds, both tests fail, and the suite writes real files to `C:\dev\null\cannot-mkdir-here\ .tool-results\` -- outside any temporary directory, at the drive root. Running the backend suite a few times leaves dozens of stray files behind. The comment above the first test records that this is the second time the same assumption has broken: `/nonexistent/...` was silently created by `mkdir -p` when CI ran as root in a container, and `/dev/null/...` was the fix. Both encode a guess about the environment rather than the condition under test. Use a regular file as the parent component instead. Creating a directory below a file fails with an `OSError` subclass on every platform -- `NotADirectoryError` (errno 20) on POSIX, `FileNotFoundError` (errno 2) on Windows -- so the branch is reached deterministically, and the path lives inside the test's own `TemporaryDirectory`, so nothing is written outside it. Verified both spellings on Linux (WSL Ubuntu, non-root) and Windows; only the file-as-parent form fails on both. The two tests still have teeth: dropping `_externalize`'s `except OSError` guard makes both fail rather than pass. Tests only -- no production code or documented behaviour changes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the backend tool-output tests deterministic across platforms by replacing a POSIX-specific “unwritable path” assumption (/dev/null/...) with a guaranteed-invalid outputs_path derived from a temporary directory containing a regular file.
Changes:
- Add
_unwritable_outputs_path()context manager to generate anoutputs_paththat reliably makesos.makedirsfail on all platforms. - Update
TestExternalize::test_returns_none_on_invalid_pathto use the new helper instead of a hard-coded/dev/null/...path. - Update
TestWrapToolCallFallback::test_fallback_when_disk_write_failsto use the new helper, preventing stray file creation on Windows.
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.
Why
Two tests in
test_tool_output_budget_middleware.pyneed anoutputs_paththatos.makedirsrefuses to create, so they can reach_externalize'sexcept OSError: return Nonebranch and the truncation fallback behind it. Bothspell that condition as a literal path,
/dev/null/cannot-mkdir-here.That only holds where
/dev/nullis a character device. On Windows it is anordinary relative path, so
os.makedirssucceeds and:TestExternalize::test_returns_none_on_invalid_pathandTestWrapToolCallFallback::test_fallback_when_disk_write_failsboth fail;C:\dev\null\cannot-mkdir-here\.tool-results\— outside any temporary directory, at the drive root. 66 stray files had
accumulated there on my machine, one per run of each affected test.
The comment above the first test records that this is the second time the same
assumption has broken.
/nonexistent/...came first and was silently created bymkdir -pwhen CI ran as root in a container;/dev/null/...was the fix forthat. Both encode a guess about the environment rather than the condition
actually under test, so each one holds only until it meets an environment nobody
had in mind.
What changed
Tests only. The two tests now build their unwritable path from a regular file:
a
TemporaryDirectorycontaining a plain file, with theoutputs_pathpointingbelow that file.
Creating a directory under a file fails with an
OSErrorsubclass on everyplatform, so the branch under test is reached deterministically, and the whole
path lives inside the test's own temporary directory, so nothing is written
outside it. Measured, rather than assumed:
/dev/null/...NotADirectoryError, errno 20NotADirectoryError, errno 20FileNotFoundError, errno 2C:\dev\null\...No production code changes, and no documented behaviour changes, so
README.mdand
AGENTS.mdneed no update here.Surface area
frontend/backend/applanggraph.json, or prompt changedocker/or sandboxed executionskills/backend/pyproject.tomlorfrontend/package.jsonBug fix verification
backend/tests/test_tool_output_budget_middleware.py::TestExternalize::test_returns_none_on_invalid_pathand
::TestWrapToolCallFallback::test_fallback_when_disk_write_failsmain, green on this branch — on Windows, which is where the oldspelling breaks.
outcome, not a no-op: the point is not to turn the tests green on POSIX but
to stop the condition under test from depending on the host to supply it.
Checked rather than assumed — I ran
main's version of both tests on Linux(2 passed) and this branch's version (2 passed), in a clean Ubuntu checkout.
_externalize'sexcept OSError: return Noneguard aroundos.makedirsmakes both failrather than pass —
NotADirectoryErroron Linux,FileNotFoundErroronWindows — so they really do exercise that branch.
C:\dev\null\...stops growingacross runs on this branch (66 before, 66 after).
Validation
Linux (clean Ubuntu checkout of this branch, Python 3.12):
Windows 11 (same branch):
To be clear about scope: this is not a bid to support Windows
I ran the whole suite on Windows to see where this sits, and it reports
88 failed, 10939 passed. The rest are POSIX permission bits, symlink
privileges, path separators, hostPath mounts,
pnpm/nginx shell assumptions —a different and much larger question, and one I am not proposing to open.
Please read this PR as two tests that no longer depend on the host, not as
step 1 of a Windows-support effort.
The reason it is still worth taking on a Linux-only project: the two arguments
above hold there too. A test that writes outside its own
TemporaryDirectoryis a hygiene bug wherever it runs, and the assumption that broke first —
/nonexistent/...being uncreatable — broke on Linux, in your CI container,because the process was root. The current spelling narrowed that assumption
rather than removing it. Deriving the path from a real file removes it.
Happy to close this if you would rather keep the magic-path form.