Give the test suite a devpod namespace of its own - #106
Conversation
Running the e2e suite deleted the developer's real devpod workspaces. Three facts composed into it: `dl --purge` deletes every workspace `devpod list` returns, an e2e test exercises exactly that for real, and `devpod list` reads ~/.devpod, which no XDG_* variable relocates. The suite's existing XDG isolation therefore bought nothing against it, and the `-m 'not e2e'` default was the only thing in the way. Scope the process environment once, before collection, rather than add a fixture or a before-delete assertion. Everything the session spawns inherits it, including the `devpod list` inside `--purge`, so there is nothing a future test can forget to ask for. A per-run directory also makes the hardcoded `e2e-test-*` ids private to the run, so concurrent runs stop force-deleting each other's workspaces -- one mechanism instead of two overlapping ones. DEVPOD_HOME alone is not enough: devpod resolves its ssh config against the real $HOME regardless, so DEVPOD_SSH_CONFIG is scoped too. And a fresh devpod home has no providers at all, so the e2e directory seeds the docker provider into it. Production `--purge` behaviour is deliberately unchanged.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThis PR scopes all DevPod-related operations in the test suite to a per-run, scratch devpod namespace by setting DEVPOD_HOME and DEVPOD_SSH_CONFIG once in pytest_configure, and adds tests to ensure the suite can no longer see or mutate a developer’s real DevPod state while keeping e2e DevPod usage functional. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
+ Coverage 89.24% 89.96% +0.72%
==========================================
Files 12 15 +3
Lines 1636 1893 +257
==========================================
+ Hits 1460 1703 +243
- Misses 176 190 +14 see 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed in fresh context against two independent axes. The fixed point is b304cb6...7f90ada (5 files, +189/-1, all under test/). Checks green across py310–py313, prek, codecov, GitGuardian. Both axes verified their load-bearing claims on metal on this host, per the map's extra principle.
The core fix is sound and should land. Everything below is about the tests that guard it, not about the seam.
Standards
Tooling passes: pixi run test → 811 passed, 9 deselected; pixi run lint (ruff + ty + pylint) → clean, pylint 10.00/10, no working-tree drift.
test_each_run_gets_a_devpod_home_of_its_owndoes not test this PR's code — blocking —test/unit/test_devpod_scoping.py:47-57. The red state was reproduced by commenting outscope_devpod_to_this_run()intest/conftest.py: the first two unit tests go red for exactly their stated reason (assert NoneonDEVPOD_HOME/DEVPOD_SSH_CONFIG) — those two are genuine. This third one stays green. It callsmake_scoped_devpod_home(tmp_path)twice and assertsfirst != second, which istempfile.mkdtemp's own contract. It would still pass ifscope_devpod_to_this_run()hardcoded a fixed path — which is precisely the regression the PR description says it catches ("the per-run directory being 'simplified' into a fixed path"). That claim is false as written. Principle 4: a test must fail when the behavior is absent. Either assert across two calls ofscope_devpod_to_this_run()itself (saving/restoringos.environ), or delete the test.- The two-function split is a Middle Man — should-fix —
test/fixtures/devpod_scoping.py:31-33.make_scoped_devpod_homeis a one-line pass-through tomkdtempwhose only production caller is the line directly below it; it exists so the test above can call it, and that test is the one that proves nothing. Principle 2: deleting beats adding. - Swallowed setup failure in the e2e provider fixture — should-fix —
test/e2e/conftest.py:22-26.capture_output=Truewithcheck=Falseand the result discarded. Ifdevpod provider add docker --usefails, every e2e test fails later against a provider-less devpod home with an unrelated error, and the stderr explaining it was captured and thrown away. Best-effort teardown is defensible; a best-effort precondition is not.check=True, orpytest.fail(result.stderr). - The new fixture writes to whatever devpod home is live, unguarded — should-fix —
test/e2e/conftest.py:22.devpod provider add docker --useis an unconditional mutation, and--usechanges the default provider. Its safety rests entirely on the scoping this PR introduces, with nothing asserting the scoping is actually in effect. If the scoping ever regresses, the PR's own new fixture is the thing that reaches into the developer's real~/.devpod. Oneassert os.environ.get("DEVPOD_HOME")at the top makes that unrepresentable (principle 3) — cheap, and it is the one place a guard is worth more than a scope. - The isolation e2e assertion is vacuous where
~/.devpodis absent — nit —test/e2e/test_full_workflow.py:75. The glob was verified correct against this host (~/.devpod/contexts/default/workspaces/{pythontemplate,devlaunch-t1-vebilote,devlaunch-main-zovomobo}), so it is not vacuous here. But in CI or DinD, where~/.devpoddoes not exist,real_devpod_workspace_ids()returnsset()andisdisjointpasses unconditionally. Skip or assert the precondition. - Two rival notions of "devpod is available" — nit —
test/e2e/conftest.py:19usesshutil.which,test_full_workflow.py:21usesdevpod version. Duplicated Code / Divergent Change: they can disagree. DEVPOD_HOME_VAR/DEVPOD_SSH_CONFIG_VARdo not earn their keep — nit —devpod_scoping.py:27-28. Used only inside their own module; the one cross-module consumer (test/unit/test_devpod_scoping.py:22,38) uses bare string literals, unlikegh_auth.DISABLE_VARwhichtest/conftest.py:44genuinely imports. Speculative Generality — use them or delete them.test/fixtures/devpod_scoping.pydefines no fixture — nit. Every sibling intest/fixtures/exports at least one@pytest.fixture; this is a plain module. Mysterious Name / misplacement.
No environment conflict exists: the only other global-os.environ writers in the tree are git_fixtures.py:38-59 (XDG_CACHE_HOME, save/restore) and test_workspace_id.py:384 (monkeypatch.setenv("HOME")), neither touching DEVPOD_*. No pytest-xdist, and coverage run -m pytest is in-process, so every documented entry point (test, coverage, ci, ci-no-cover) reaches pytest_configure. Prose-length test names match the existing convention — not an outlier.
Spec
Core requirement: MET. "Running the e2e suite on a host deletes that host's real devpod workspaces. Make it impossible." pytest_configure (test/conftest.py:88) sets DEVPOD_HOME before collection; every subprocess the session spawns inherits it, including the devpod list inside dl --purge (devlaunch/dl.py:335-339).
DEVPOD_HOMEandDEVPOD_SSH_CONFIGare both real, not invented — satisfied, verified on metal.devpod version→v0.26.1. Global flags:--devpod-home string … You can also use DEVPOD_HOME.devpod up --help:--ssh-config string The path to the ssh config to modify, if empty will use ~/.ssh/config. You can also use DEVPOD_SSH_CONFIG. Spellings matchdevpod_scoping.py:24-25exactly.- No path spawns devpod with a replaced environment — satisfied. This was the biggest risk to the whole seam.
run_devpod(args, env=...)has exactly one caller passingenv—workspace_ssh(dl.py:1010), fed bygh_auth.ssh_args_and_env(), which returns{**os.environ, TOKEN_VAR: token}(gh_auth.py:171). Every e2eenv=is{**os.environ, "XDG_CACHE_HOME": …};DLRunnerdefaults todict(os.environ)(e2e_helpers.py:49). The one fully-replaced env (test_dl.py:2934) pointsPATHat an empty dir with no devpod reachable. The scoping survives everywhere. "makes the bad outcome unrepresentable rather than merely unlikely" — met at this seam. - The red test's red state is safe — satisfied. "The failing test cannot be 'run
--purgeand see what dies' — the red state of that test IS the damage." The e2e test only runsdevpod listand asserts disjointness (test_full_workflow.py:55-75). Independently re-run here by node id under-m e2e: passed in 1.52s, and this host's three real workspaces were byte-identical before and after. DEVPOD_SSH_CONFIGis subcommand-scoped, not global — should-fix, and it qualifies a map answer.DEVPOD_HOMEis a persistent root flag and covers every subcommand.DEVPOD_SSH_CONFIGis registered only onup(up: 1,ssh: 0,delete: 0,stop: 0,machine ssh: 0).upis the writer, so the harm named in the ticket is covered — butdevpod sshanddevpod deletestill resolve the real~/.ssh/config. Map #93's open question "whether devpod writes its~/.ssh/configblocks relative to$HOMEor toDEVPOD_HOME" is answered correctly, but the breadcrumb on #103 states it without the qualifier. The unit test's name —test_devpod_ssh_config_is_pointed_away_from_the_developers_ssh_config— promises more than the variable delivers. Narrow the docstring todevpod up, and add the qualifier to the map, or a reader in a year takes the broader claim at face value.- Subsumption of the per-run-id defect holds, with one unverified residual — nit. Teardown's
devpod delete --force(e2e_helpers.py:160-168) now sees only its own namespace, so concurrent runs cannot delete each other's workspaces — the ticket's second, milder instance is genuinely subsumed rather than merely deferred. Outside the namespace: docker container names are random (#95 metal),metadata.json/XDG is per-testtmp_path. Unverified: two concurrent runs both build ide2e-test-purge, whose image tag carries the id plus a hash — not checked whether the hash differs per source path. Not destructive of workspaces either way. - Non-cleanup justification is true on this host, but broader than its own rationale — nit.
/tmpis on/(not tmpfs),/usr/lib/tmpfiles.d/tmp.confhasD /tmp 1777 root root 30d, andsystemd-tmpfiles-clean.timeris active — so "a stale directory the OS will reap" is accurate. Sixdevlaunch-testrun-*dirs exist now, 4K–28K each. Two caveats worth a line in the docstring: (a) the rationale is about containers a run created, but a dir is created by every pytest process, including the ~811-test unit-only runs that never spawn devpod and leave a 4K empty dir behind; (b) the 30-day reaper is itself the orphaning event the docstring is trying to avoid — non-cleanup defers the problem rather than removing it. - No scope creep; production finding reported — satisfied. "Whether
dl --purgeitself should learn a narrower blast radius in production … is not this ticket." Zerodevlaunch/files touched. The production finding (--purgewould have deletedpythontemplate; theWorkspaceId/metadata.jsonownership predicate already exists and is unused) is posted on #103 for the map to graduate.
Not in the ticket, but adjacent and now known-incomplete: CLAUDE.md's scratch-XDG paragraph tells a developer to point XDG_CACHE_HOME/XDG_CONFIG_HOME at a scratch dir before running dl-next. This PR establishes that this does not scope devpod list, so a manual dl-next --purge still deletes everything. The suite is now safe; the documented manual workflow is not. One sentence naming DEVPOD_HOME/DEVPOD_SSH_CONFIG would close it — out of scope here, worth a follow-up.
Verdict
Request changes — material but small and mechanical. The seam is the right one, it was chosen for the right reason, and it was verified on metal at the exact place that does the deleting. Nothing here argues for a different design.
Blocking:
test_each_run_gets_a_devpod_home_of_its_own(test/unit/test_devpod_scoping.py:47-57) is green with the mechanism removed. It assertstempfile.mkdtemp's contract, not this PR's. Rewrite it againstscope_devpod_to_this_run()or delete it — and either way correct the PR description's claim that it catches the fixed-path regression. Deleting it also removes the only reasonmake_scoped_devpod_homeexists.
Should-fix before merge:
test/e2e/conftest.py:22— assertDEVPOD_HOMEis set before runningdevpod provider add docker --use. It is the one new unconditional write to a devpod home in this diff, and today nothing stops it landing in the developer's real one.test/e2e/conftest.py:22-26— stop swallowing the provider-install failure;check=Trueorpytest.fail(result.stderr).test/unit/test_devpod_scoping.py:31-43— narrow the ssh-config claim todevpod up, which is the only subcommandDEVPOD_SSH_CONFIGis registered on, and carry the qualifier to the #93 map answer.
Nits (2–8 in the axes above) are optional.
Review found the per-run-directory test green with the mechanism removed: it asserted `tempfile.mkdtemp`'s contract, not this suite's. It now drives `scope_devpod_to_this_run` itself, twice, so it goes red when the per-run directory becomes a fixed path -- the regression it exists to catch. That leaves `make_scoped_devpod_home` with no caller of its own, so it is inlined back into the one line below it. The e2e provider fixture is the suite's one unconditional write to a devpod home, and `--use` rewrites the default provider. It now asserts DEVPOD_HOME is set before writing, so a regression in the scoping cannot turn it into the thing that reaches into the developer's real ~/.devpod, and it raises the install failure instead of capturing the stderr and discarding it. `DEVPOD_SSH_CONFIG` is registered on `devpod up` alone (measured on devpod v0.26.1: `ssh`, `delete` and `stop` do not take `--ssh-config`), so the test is renamed to claim only what the variable delivers. Also: the e2e isolation assertion skips where there is no real ~/.devpod to be disjoint from rather than passing vacuously; the two rival devpod availability checks collapse into one in `fixtures/e2e_helpers`; the DEVPOD_*_VAR constants are used by their cross-module consumer instead of bare literals; and the module moves out of `test/fixtures/`, which is for modules that define fixtures.
The rationale is about containers a run created, but most runs are unit runs that never spawn devpod and leave an empty directory; and the /tmp reaping it leans on is the orphaning event it is avoiding, deferred rather than removed.
Addressed on 1 (blocking) —
|
Focused verification pass on Independent mutation transcriptsBaseline — Mutation A — Mutation B — per-run Mutation C (mine, not the author's) — a sneakier shape: keep the per-run directory unique but write a fixed So the second assertion is load-bearing, not decoration, and tests 1–2 correctly stay green under B and C — they are about location, not uniqueness. Each mutation restored with Per finding1 (was blocking) — VERIFIED, and the keep-over-delete call is right. The rewritten test drives 2 — VERIFIED, guard proven to fire before any write. Rather than infer it from statement order, I imported the fixture with 3 — VERIFIED, and 4 — VERIFIED, re-measured here rather than read off the reply. devpod Matches the docstring and the renamed test exactly. The qualifier is carried in three places, all checked: the module docstring, the test docstring, and — the part the review actually asked for — the map answer on #93, which now reads 5 (nits) — all four VERIFIED by inspection, not by the list. Vacuous assertion now Did the fixes introduce anything newNothing blocking. Import surface is intact — 820 tests collect with no errors, the default suite is
PR descriptionThe false claim is gone. The table row for Host state — before and after this pass
The e2e isolation test was run by node id ( VerdictApprove. All five findings are genuinely addressed, and the one that was blocking is addressed in the harder of the two available ways — the rewritten test now fails under three separate mutations of the behavior it claims to pin, including one the author did not try. CI green on all nine checks at |
`test-e2e` carried `depends-on = ["dev-add-docker"]`, ported from #69 where it predated #106. The suite repoints DEVPOD_HOME at a fresh mkdtemp in pytest_configure, before collection, so the provider that edge registers lands in the ambient ~/.devpod -- a namespace the run never reads. It bought the suite nothing and wrote to the developer's own devpod home as a side effect of running tests, which is the class of thing #106 existed to stop. test/e2e/conftest.py's session-scoped autouse fixture is what actually covers the e2e path, by installing the provider into the run's own home. Three more corrections that came out of the same review: - The addopts comment said the suite "creates and deletes workspaces in whichever devpod namespace it can reach". Untrue post-#106: it always scopes its own, unconditionally, before collection. - The marker prose lived twice, in `[tool.pytest.ini_options] markers` and in addinivalue_line calls that duplicate it wholesale. Deleting the copies removes the second sentence rather than keeping it in sync; --strict-markers still collects 22 e2e tests. - strip_jsonc_comments rejects trailing commas, which the spec allows, so "a devcontainer runtime has to be able to parse this file" overclaimed. The docstring now says what the helper is and names the stricter-than-runtime trade; the test is renamed after what it asserts. And postCreateCommand now says what its third link costs: an unreadable provider listing fails container creation, deliberately, because the alternative is a container that looks fine and fails on the first dl command.
1 participant
Closes #103.
The footgun
pytest -m e2eon a developer's machine deletes their real devpod workspaces. Three facts compose:dl --purgedeletes every workspacedevpod listreturns — it has no notion of "only mine".devpod listreads~/.devpod, which noXDG_*variable relocates. The suite already setsXDG_CACHE_HOMEon those calls; it buys nothing here.The
-m 'not e2e'default inpyproject.tomlwas the only thing standing in the way, and a default is not a safeguard — a misconfigured editor, a-m e2e, or an agent that did not read the marker config is enough.Measured on the host, at the exact seam that does the deleting —
dl's ownlist_workspaces(), the listpurge_all_data()iterates:--purgewould delete['devlaunch-main-zovomobo', 'devlaunch-t1-vebilote', 'pythontemplate'][]The seam, and why this one
Scope the process environment once in
pytest_configure, before collection, pointingDEVPOD_HOMEat a directory created for this run.The ticket offered three candidates. This one was picked because it is scoping rather than guarding:
pytest_configurecovers every subprocess the session spawns, because they all inherit this process's environment. That includes thedevpod listinsidedl --purge, which is the whole ballgame.devpod listis empty before any--forcedelete — an assertion at each call site is exactly the thing the next new test can omit. Same objection, worse ergonomics.e2e-test-*constants private to the run already, so two concurrent runs stop force-deleting each other's workspaces. Adding unique ids on top would be a second concept overlapping the first (principle 2).No new type: the alternative shape was a helper that builds the subprocess environment, but a helper can be bypassed by any
subprocess.runthat passesos.environdirectly, and the process environment cannot. The weaker-but-unbypassable option wins.Verified on metal, not read off docs
DEVPOD_HOME=<scratch> devpod list --output jsonreturns[]on a host with three real workspaces.DEVPOD_HOMEalone is not enough.devpod upwrites itsHost <id>.devpodblock to the real~/.ssh/configregardless — but--ssh-confighas an env twin,DEVPOD_SSH_CONFIG, so the same seam covers it with one more variable. This answers an open question on the map (Develop devlaunch inside its own devcontainer, on an isolated Docker daemon #93): devpod resolves the ssh config against neither$HOME-via-DEVPOD_HOMEnorDEVPOD_HOME— it is~/.ssh/configunlessDEVPOD_SSH_CONFIGsays otherwise.With one qualifier, added in review and re-measured on devpod v0.26.1. The two variables do not have the same reach.
--devpod-homeis a persistent root flag, soDEVPOD_HOMEcovers every subcommand (devpod delete --helplists it).--ssh-configis registered ondevpod upalone — counted on this host,up: 1,ssh: 0,delete: 0,stop: 0— soDEVPOD_SSH_CONFIGredirects only the subcommand that writes ssh config. That is the one that does the damage, so the suite is covered; butDEVPOD_SSH_CONFIGis not a global, and the module docstring and the unit test now say so rather than leaving a reader to assume otherwise.A fresh
DEVPOD_HOMEhas no providers at all, sodevpod upin it fails untildockeris installed there. Hencetest/e2e/conftest.py: autouse, session-scoped, so no new e2e test can forget it and no unit run pays for it.The destructive path itself, end to end.
dl --purge -ywas run for real against a synthetic scoped namespace holding one workspace. It deleted exactly that one; all three of the host's real workspaces survived.Tests
test_devpod_in_this_session_cannot_see_the_developers_workspaces(e2e)devpod listthe way the suite spawns everything and asserts it is disjoint from the ids read straight off~/.devpodon disk. Went red naming exactly the three real workspaces, green after the fix. Lives in the e2e directory so the assertion runs in the session that could do the damage.test_devpod_commands_are_pointed_away_from_the_developers_devpod_home(unit)$HOME.test_devpod_up_is_pointed_away_from_the_developers_ssh_config(unit)DEVPOD_HOMEset but the ssh config still the developer's. Named fordevpod upbecause that is the only subcommandDEVPOD_SSH_CONFIGreaches.test_each_run_gets_a_devpod_home_of_its_own(unit)docker_provider_in_scoped_devpod_home(e2e fixture)devpod provider add docker --userewrites the default provider of whichever home is live; the fixture refuses to run it unlessDEVPOD_HOMEis set.The three unit tests run on the default suite, not under
-m e2e— so removing the scoping breaks an ordinarypixi run testrather than waiting for the run that would do the damage.Each red state was reproduced on this host, by mutation, and restored. Commenting out
scope_devpod_to_this_run()inpytest_configureturns the first two unit tests red onassert NoneforDEVPOD_HOME/DEVPOD_SSH_CONFIG, and turns the e2e provider fixture into an error that names the write it refused. Replacing the per-runmkdtempwith a fixed path turnstest_each_run_gets_a_devpod_home_of_its_ownred onassert first != secondwith the two identical paths printed. The third test drivesscope_devpod_to_this_runitself, twice, rather than the directory-making it happens to use — an earlier revision assertedtempfile.mkdtemp's own contract and stayed green under both mutations, which is why the function it called (make_scoped_devpod_home) no longer exists.The scratch directory is deliberately not cleaned up: it holds the metadata devpod needs to find and delete the containers a run created, so removing it after a crashed run would orphan those containers with no way to reach them.
Deliberately not fixed
Production
dl --purgestill deletes every workspacedevpod listreturns. Per the ticket's scope note, this PR makes the test suite safe; whether--purgeshould learn a narrower blast radius in production is a separate question, raised on #103 for the map to graduate. Nodevlaunch/source file is touched by this PR.Not done
The full e2e suite was not run on this host. Under this change it would be safe to, but it builds real containers on a Docker daemon another effort is concurrently using. The destructive path was verified directly instead (item 4 above). Full default suite: 811 passed. Lint 10.00/10.
🤖 Generated with Claude Code
Summary by Sourcery
Scope all DevPod interactions in the test suite to a per-run namespace to prevent destructive operations from touching developers' real workspaces.
New Features:
Bug Fixes:
dl --purgeand other DevPod commands run by the test suite from deleting or modifying real developer workspaces and SSH config.Enhancements:
DEVPOD_HOMEandDEVPOD_SSH_CONFIGto a run-specific directory during pytest configuration.Tests: