Repository navigation
Rider sweep: the small debts this week's reviews parked - #222
Merged
Merged
Conversation
`bench_launch.py --record` writes a JSON shape `bench_points.py` reads, and nothing failed when they drifted. Each side was pinned against its own copy of the shape -- the writer's suite asserts the fields it just wrote, the reader's suite drives literals on purpose -- so a field renamed in the writer and in the writer's own assertions left both green while the trend stopped publishing. This runs one record through both shipped scripts end to end, with no hand-built dict between them, and names no field of its own so there is no third copy to drift.
It probes, lends and installs, and any of those may come up empty -- a host with nothing to lend, a container the binaries will not run in, an opt-out, a failed network install. It reports what it found and the session starts either way, so "ensure" named the answer rather than the work. `provision` is already the module's own word for this: the opt-out disables "tool provisioning" and the probe's happy answer is PROVISIONED. Call sites, comments and the tests that named it move with it.
- clone_repo's partial-clone deletion named ensure_repo as the gate. Its one caller is clone_if_missing now, reached from ensure_repo and prepare_cold alike, and what makes the rmtree safe is the RepoLock token both routes carry. - locks.py claimed the token made a second acquisition unwritable. It does not: ensure_repo is still public, still lock-taking, and still unmarked. Softened to what the token actually proves. - workspace_clone.py said the hardlink test asserts "same st_ino" (it compares (st_dev, st_ino)) and that the bare cache has no LFS objects (it is the repo's LFS store now). Both were made false by the PR that sat beside them. - fetch_interval=0 lost the comment that was its only rationale, and 0 means *always* fetch, so the surviving line read as its own opposite. - CHANGELOG never recorded that cold prep now serializes across the whole preparation. Unreleased, because 0.0.26 shipped before it did.
The two arms had byte-identical bodies kept apart by a stated rationale that is false: exhaustiveness comes from _unhandled_removal's NoReturn parameter, and a grouped isinstance narrows to it just as well -- which describe_source in the same module already relies on. README's "when none of it goes" listed conditions that are not sufficient for it: a sealed cache root over removable clones is a partial removal, not a nothing. Tightened to the condition that actually decides the arm.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideRefactors tooling provisioning API naming, tightens repo lock and clone invariants, clarifies documentation and comments around pruning, LFS and fetch intervals, and adds an integration-style test that pins the bench record schema across writer/reader scripts while keeping behavior unchanged elsewhere. Flow diagram for workspace tools provisioning via provision_toolsflowchart LR
User["Developer runs dl workspace_up or dl up"]
DLCLI["devlaunch.dl.workspace_up / _run_cli"]
RunDevpod["run_devpod (devpod up)"]
ToolsModule["devlaunch.tools.provision_tools"]
Workspace["Workspace container"]
User --> DLCLI
DLCLI --> RunDevpod
DLCLI --> ToolsModule
ToolsModule --> Workspace
subgraph ToolsProvisioning
ToolsModule --> Probe["Probe REQUIRED_TOOLS in workspace"]
ToolsModule --> Lend["Lend host tools into container"]
ToolsModule --> Install["Install tools when allowed"]
ToolsModule --> Answer["Return whether tools are now available"]
end
Answer --> DLCLI
Flow diagram for bench record schema pin between writer and readerflowchart LR
Test["test_bench_record_schema.py"]
BenchLaunch["bench_launch.py --record"]
RecordFile["Bench record file"]
BenchPoints["bench_points.py --require-stages-on"]
Test --> BenchLaunch
BenchLaunch --> RecordFile
Test --> BenchPoints
BenchPoints --> RecordFile
subgraph MutationCheck
SkewWriter["Intentionally skew field names in bench_launch.py"]
Observe["Observe which tests fail (new pin vs existing suites)"]
end
Test --> SkewWriter
SkewWriter --> Observe
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 #222 +/- ##
==========================================
+ Coverage 92.80% 92.96% +0.15%
==========================================
Files 24 24
Lines 3681 3679 -2
==========================================
+ Hits 3416 3420 +4
+ Misses 265 259 -6
🚀 New features to boost your workflow:
|
`dl <ws> up` against a container that is already running returns without ever calling `devpod up`, and tops the tools up on the way past. That top-up is what makes `up` the documented recovery for a workspace that missed provisioning, and nothing exercised it -- so renaming the call was enough to turn it into the only uncovered line in the diff. Mutation-checked: delete the top-up and the new test is the one that fails.
…neighbour Review found both items on the same seam -- a claim the code does not carry. The collapsed `isinstance(outcome, (RemovedWhatItCould, RemovedNothing))` rests on the two arms reporting alike, and this PR's own mutation testing showed nothing held that down: narrowing it to `(RemovedWhatItCould,)` left all 55 tests green, because no prune test ever reached a removal that removed nothing. The new test drives one -- an orphan clone sealed at its own root, with `--separate-git-dir` putting the repository outside so every entry under the seal is a file the seal governs and none of it comes away -- and asserts the partial arm's contract on it: named with what the system said, siblings still gone, exit 1. Mutation-checked: the narrowed isinstance now fails on this test and only this test. clone_repo's comment said it "is reached only through clone_if_missing, which cannot be called without the RepoLock token". True of production and not of the method: it is public, takes no token, and sixteen test sites call it directly and unlocked. Hedged to the production callers, with what the signature does not enforce said out loud -- the same overstatement class the locks.py edit in this PR exists to correct.
blooop
commented
Aug 15, 2026
blooop
left a comment
blooop
left a comment
Owner
Author
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review of #222 against ticket #221 at merge-base 82a60fe, plus one closing fix round (a46a17f). Preflight: all checks green, non-empty diff (13 files, +303/−74 pre-fix), MERGEABLE.
Standards
Nothing here blocks. Everything checked at head is true.
- Verified true (read against the code, not the diff): the prune collapse — a scratch 4-member union with a grouped
isinstanceand aNoReturnsink still errors on the fourth arm, so the new comment's justification holds.workspace_clone.py's(st_dev, st_ino)claim matches the integration test; the LFS comment matches_fill_cache_lfs_store; bothfetch_interval=0rationales are right;locks.py's hedge is accurate; the CHANGELOG entry's lock-cycle claims are pinned by dl.py:4432 and test_repo_lock_cycles.py. - Rename is complete. Zero
ensure_toolsanywhere at head — source, tests, docs. repo_manager.py:226-233overclaimed in the new direction: "reached only throughclone_if_missing" is true of shipped code but ~16 test sites callclone_repodirectly unlocked. [Resolved in a46a17f — hedged to "in production", with a second paragraph stating what the signature does not enforce.]- Non-blocking —
test_bench_record_schema.pyre-declaresPOINTS/run_pointsbyte-identically totest_bench_points.pywhile importing other helpers fromtest_timing. - Non-blocking — the README
--purge"none of it goes" rewrite rode along undeclared; it is correct (all threeRemovedNothingproducers enumerated) but user-facing. - Non-blocking — two staleness points:
tools.py:879"whether they are now there" is approximate on theLENDABLEarm; the follow-on test's class docstring still says "every workspace dl opens goes throughup" while the test asserts the opposite.
Spec
All 8 ticket items present and accurate; the binding out-of-scope list untouched. Nothing blocks.
- Item 5 — "a record written by the real writer parses through the real reader": satisfied as subprocess-to-subprocess, no hand-built dict, no record field named in the test. Mutation
"median"→"medians"in the writer → 5 failed; restored. - Item 4 — zero
ensure_toolsat head;provision_toolsmatches the body (probes, lends, installs; session starts either way). - Item 7 — precedent real (dl.py:1450); prune/purge suites unedited and passing; an injected fourth
Removalarm makestyerror exactly at the sentinel — the exhaustiveness claim is true, not asserted. README's rewrite replaces the false necessary condition the ticket flagged. - Item 3 — independently verified:
v0.0.26is an ancestor of PR #201's merge and that merge is not inv0.0.26;[Unreleased] → Changedcorrect. - Items 1/2/6/8 — each true against current code. (Item 1's absolutist wording — the Standards axis's finding — resolved in a46a17f.)
- Deviations: the codecov follow-on test verified real (replacing the top-up at dl.py:4522 fails exactly the new test); the sibling-comment and no-CHANGELOG calls sound. Deviation 2 — leaving the proven-uncovered
RemovedNothingarm — was judged acceptable but the weakest of the four: "the collapse is precisely what makes the two arms' interchangeability load-bearing … I'd have taken it." [Taken in a46a17f:test_a_directory_none_of_which_came_away_is_reported_the_same_way, built on a--separate-git-dirclone whose every entry is a file (the shape that genuinely reaches total refusal — a sealed normal clone is still a partial, as test_purge_partial_removal.py pins). The narrowing mutation now fails exactly this test (1 failed, 55 passed); restored, 56/56.]
Verdict
Approve — no blocking findings on either axis; the two residues both axes converged on (the prune-RemovedNothing pin, the "reached only through" overclaim) were closed during review as commit a46a17f, with the mutation and CI re-verified green. Remaining riders for future touchers: dedupe POINTS/run_points with test_bench_points.py, refresh the two stale docstrings named above. GitHub refuses --approve from the PR author's account, so this is posted as a comment; this written verdict governs. Merging with a merge commit per repo rules.
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.
Closes #221
One pass over the eight riders the reviews of PRs #201–#220 parked. The out-of-scope structural riders (shared e2e
in_container(), purge-runner dedupe,--beforeextraction, install.sh path-list pin) stay out.The eight
clone_repo's partial-clone deletion no longer namesensure_repoas the gateclone_if_missing, which cannot be called without theRepoLock;ensure_repoandprepare_coldboth reach it, so the lock is the invariant, not the routelocks.py's token paragraph softened to whatRepoLockprovesensure_repois still public, lock-taking and unmarked in its signature — the token removes the reason to re-lock, it is not a guard against it[Unreleased][Unreleased]is where it belongs and no retroactive-note convention is neededensure_tools→provision_tools, with every call site, comment and testprovisionis the module's own word (provisioning_disabled,ProbeResult.PROVISIONED)st_ino" and "the bare has no LFS objects"(st_dev, st_ino);_fill_cache_lfs_storemakes the bare the repo's LFS store, so the new text says what is true — the store is filled per ref, after this clone, which is why the smudge is still skippedisinstance(x, (A, B)); false rationale replaced; README's "none of it goes" tightenedtygreen (exhaustiveness still holds, asdescribe_sourcealready showed); prune/purge suites pass uneditedfetch_interval=0in both fixtures_should_fetchreturnselapsed > interval, so0means always. It is harmless becauselazy_fetchis its only reader and nothing here drives it, and the remotes are on local diskItem 5: the mutation check
The new
test/test_bench_record_schema.pyruns one record through both shipped scripts as subprocesses —bench_launch.py --recordagainst a stand-in launch built fromtiming.py's own constants, thenbench_points.pyhanded the file,--require-stages-onincluded. No hand-built dict, and it names no field of the record, so there is no third copy of the schema to drift.Field names skewed in the writer, one at a time:
bench_launch.pytest_timing.pytest_bench_points.pytotal_seconds→total_secs(run)seconds→secsruns→countmedian→medianstotal_secondsrenamed in the writer and intest_timing.py's own assertionsThe last row is the case this exists for: a rename carried out consistently on one side leaves both pre-existing suites green and the trend silently broken. All skews restored; the suite is green at HEAD.
Item 7: behaviour-neutrality, and one honest finding
The purge and prune suites are unedited and pass. Mutating the collapsed branch:
(RemovedWhatItCould, RemovedNothing)→(RemovedNothing,)— 2 tests fail. The partial arm is pinned.(RemovedWhatItCould, RemovedNothing)→(RemovedWhatItCould,)— 55 passed, andtest_dl.py,test_purge_partial_removal.pyandtest_disk_usage.pyall stay green too.So prune's
RemovedNothingarm is covered by nothing. The refactor is neutral by construction — both arms had byte-identical bodies before and share one now — but the suite does not prove it for that arm. I did not add a test for it: the ticket scopes this sweep to eight items and this would be a ninth. Rider for the next toucher: prune has no test that reachesRemovedNothing. (--purge's ownRemovedNothingarm is covered, intest_purge_partial_removal.py.)Notes
ensure_toolsrename.devlaunch.toolsis internal, the name appears in no README, CHANGELOG or docs, and no CLI surface moved. The one CHANGELOG entry here is item 3, which is a behaviour change that shipped without a line.test/unit/test_tools.py,test/unit/test_launch_serialization.pyandtest/test_timing.pypinned the old name and were updated deliberately with it; the class becameTestProvisionToolsand the mock localsprovision.One follow-on commit, after the first push
codecov/patchcame back red on exactly one line:dl <ws> upagainst a container that is already running returns without callingdevpod upand tops the tools up on the way past — and nothing exercised it, so renaming the call was enough to make it the only uncovered line in the diff. It is also the arm that makesupthe documented recovery for a workspace that missed provisioning, so it was worth a test rather than a waiver. Mutation-checked: delete the top-up and the new test is the one that fails.CI:
pixi run ciexit 0 — ruff, pylint 10.00/10,tyall checks passed, 1745 passed / 24 deselected. All PR checks green,codecov/patchincluded.🤖 Generated with Claude Code