Say what Trips is, replace the Runner impl count with the shape, and guard the count - #415
Conversation
…hape The trait's doc said it was "implemented once for real processes and once for tests". There are nine impls: ProcessRunner, FakeRunner, and seven test-local wrappers over those two. Three of the wrappers hold a real ProcessRunner and hand it whatever they do not fake -- FakeDevpodRealGit, lifecycle's Devpod, workspace_clone's StubbedLfs -- which is the fact a reader currently learns by grepping and then by reading a fixture. A corrected count would rot the same way, so the doc states the shape instead: one production impl, one shared fake carrying the response table and DevpodMachine, and wrappers over those two, with the grep named as the enumeration. What it does say about the wrappers is the two shapes worth recognising before reading one -- part real (a fake devpod beside a real git) and recorder -- neither of which is a number. The half of that shape which is load-bearing is now asserted rather than claimed: tests/one_seam.rs scans the workspace and fails if anything but ProcessRunner implements Runner outside test code. A second real implementation is not a stale doc, it is a second way to start a process. Proven by adding one and watching it fail. Trips gets the doc #405 settled: it is a recorder, not a third fake devpod. It answers by call index from a list the test handed the constructor, never reads argv to decide anything, and holds no workspace state -- so a corpus row, which is given state -> argv -> exit + then state, has nothing to bind to at either end, and driving it over conformance.json would assert a fixture against itself. The next person asking why it is not in the corpus finds the answer at the definition. devpod.rs's module doc says the same thing from the other side: the two hand-written fakes are the whole population the corpus covers, and the Runner wrappers reach DevpodMachine through FakeRunner rather than being a third. Also Trip::script, which re-parses the argv setup_pass builds: it now says so, and `&self.argv[at + 1]` becomes `.get(at + 1).expect("a --command with a payload")`, so a payload that moved says which of the two assumptions broke instead of panicking on an unnamed index. Closes nothing; #409, under map #406.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThe PR replaces stale Runner implementation-count documentation with a durable description of the production seam, shared fake, and wrapper shapes, adds a source-scanning guard against new production implementations, and clarifies the separate roles of Trips and DevpodMachine in test and corpus coverage. Sequence diagram for the production Runner seam guardsequenceDiagram
participant Test as one_seam test
participant Walk as Workspace source scan
participant Impl as Runner implementations
Test->>Walk: scan workspace for impl Runner for
Walk->>Impl: classify production vs test code
Impl-->>Walk: ProcessRunner and test implementations
Walk-->>Test: production implementations
alt only ProcessRunner is production
Test-->>Test: pass
else another production implementation exists
Test-->>Test: fail
end
File-Level Changes
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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 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 at merge-base 57955a3...c7edddb. The guard test was run, and then attacked, in a scratch worktree; every probe below was actually compiled and run.
Preflight: ci, rust, e2e, prek, public-api, packaging, coverage green. review/gate are red only for the missing wf-review report.
Standards
The doc claims check out. grep -rn "impl Runner for" rust/ gives nine impls: ProcessRunner (devlaunch-runner/src/lib.rs:442), FakeRunner (devlaunch-test-support/src/fake_runner.rs:370), ScriptedRunner, StubbedLfs, Trips, Rebuilding<'_>, FakeGit, FakeDevpodRealGit, Devpod. Three hold a real ProcessRunner and fall through to it — FakeDevpodRealGit (flows/listing.rs), Devpod (flows/lifecycle.rs), StubbedLfs (flows/workspace_clone.rs) — and StubbedLfs's nuance is as the PR body describes: it routes on the lfs subcommand, and only capture reaches real processes. The Trips characterisation is accurate: record (provision.rs:2456) pushes argv into seen and returns answers[(seen.len()-1).min(answers.len()-1)], never consulting argv to decide, and the struct holds no workspace state. Trip::script's .get(at + 1).expect("a --command with a payload") returns the same element in every passing case (&String coerces to &str) and replaces an unnamed index panic with a named one — an improvement, no new panic path.
Now the guard, which is where the findings are. devlaunch-runner/tests/one_seam.rs passes as shipped (2/2), and it is not vacuous — the_scan_finds_the_implementations_it_is_meant_to_judge really does anchor on ProcessRunner and on ≥2 test impls, and a plain second production impl is caught. I dropped impl Runner for Sneaky {} into devlaunch-core/src/probe1.rs and got exactly the failure the PR promises:
assertion `left == right` failed: Runner is the one seam onto the OS, and ProcessRunner is meant to be the
only implementation of it outside test code. Found: [
"Sneaky (devlaunch-core/src/probe1.rs)",
"ProcessRunner (devlaunch-runner/src/lib.rs)",
]
left: 2
right: 1
1. CONFIRMED, and the most serious finding here — two trivial bypasses. implementer() matches only a line whose trimmed text begins literally with impl Runner for . I put both of these in devlaunch-core/src/ as ordinary production files, with probe1.rs removed, and the guard stayed green:
// devlaunch-core/src/probe2.rs
pub struct Sneaky2<'a>(&'a str);
impl<'a> Runner for Sneaky2<'a> {}
// devlaunch-core/src/probe3.rs
pub struct Sneaky3;
impl devlaunch_runner::Runner for Sneaky3 {}running 2 tests
test only_process_runner_does_real_work ... ok
test the_scan_finds_the_implementations_it_is_meant_to_judge ... ok
test result: ok. 2 passed; 0 failed
Neither spelling is exotic. The first is what a wrapper looks like — impl<R: Runner> Runner for Timed<R> — which is precisely the future the new doc anticipates; note that today's Rebuilding<'_> only escapes it because it happens to be written with '_ rather than impl<'a>. The second is the natural spelling in any crate that does not use devlaunch_runner::Runner, i.e. the likeliest form a genuinely new second seam would take. A guard whose whole job is "a second production impl must not arrive silently" currently lets both arrive silently. Matching ^\s*impl(<[^>]*>)?\s+(\w+::)*Runner\s+for\s+ would close both.
2. CONFIRMED — a false positive on legitimate future code. is_test_code requires the #[cfg(test)] line to be immediately followed by the mod ... { line. One ordinary attribute in between and every wrapper in that module reads as production:
// devlaunch-core/src/probe4.rs
#[cfg(test)]
#[allow(clippy::too_many_lines)]
mod tests {
struct LocalFake;
impl Runner for LocalFake {}
}Found: [
"LocalFake (devlaunch-core/src/probe4.rs)",
"ProcessRunner (devlaunch-runner/src/lib.rs)",
]
left: 2
right: 1
Six files in this tree carry an inline #[cfg(test)] mod tests { with a Runner wrapper below it. Adding #[allow(...)] or #[rustfmt::skip] to any of them breaks this test, with a message that accuses the author of adding a second production seam.
3. Low — the doc's own recommended enumeration now has false hits. The Runner doc tells the reader that grep -rn "impl Runner for" is the enumeration. This PR adds two lines that grep matches and that are not impls: devlaunch-runner/src/lib.rs:395 (the doc sentence quoting the grep) and one_seam.rs:32 (const OPENER: &str = "impl Runner for ";). The grep now returns 11 lines for 9 impls. Small, but it is the same self-invalidation the PR exists to end — worth --include=*.rs | grep -v const in the doc, or just saying "nine at time of writing, and here is how to recount".
4. Low — accuracy of the "Part real" bullet. It says such a wrapper "hands everything else, git above all, to real processes". True of FakeDevpodRealGit and Devpod; not of StubbedLfs, whose passthrough, session and detach are canned and never reach self.real. The PR body says the doc would say "hands it whatever they do not fake" — that phrasing is not what shipped, and it is the more accurate one.
5. Low — devpod.rs's new paragraph. "nothing else in the tree decides a devpod outcome from argv": the response table FakeRunner carries is argv-prefix keyed, and the paragraph three lines above it in that same module doc says it "short-circuits this machine entirely". So per-test scripted devpod outcomes are decided from argv and are not corpus-covered. The sentence is true only if scripted responses are read as not-a-fake, which is defensible but is the thing the sentence should say.
Nit: rust_sources skips directories by the literal name target, so a CARGO_TARGET_DIR under rust/ by any other name puts generated sources into the scan. Very low.
Spec
Against #409.
- "Say what
Tripsis ... a recorder, not a fake devpod: it answers by call index and never reads argv to decide an outcome, holds no workspace state, and therefore cannot be driven over a corpus row" — done, and accurate to the code. - "say which argv it is re-parsing (
setup_pass), and that theexpectis the guard ...--commandis inSSH_VALUE_FLAGS...conformance.json:258carries anssh ... --commandrow measured against v0.26.1" — done, all four facts present. - "Optional one-liner ...
.get(at + 1).expect("a --command with a payload")" — taken, verbatim. - "Do not write "seven" as a number that will rot the way "once" did — say the structure and name the three routers, since those are the ones a reader currently learns by grepping." — half done. The structure is stated; the three routers are not named. Only
FakeDevpodRealGitappears. The PR body describes this as "per the ticket's steer ... the three routers are not listed by name", but the ticket's steer, quoted above, says the opposite in the same sentence that forbids the count. Either a later steer superseded #409 and was not recorded there, or a stated requirement was dropped. This is the one place the diff and the spec disagree, and it wants reconciling on the ticket rather than in a review reply. - "Testing the outcome: No behaviour changes, so no new test." — the PR adds one anyway. More than asked for and a good instinct; it is also where findings 1 and 2 live.
Verdict
Request changes. The docs are accurate and the Trip::script change is a clean improvement — but the guard is the half of this PR that is meant to outlive the prose, and as written it can be walked past by two ordinary Rust spellings while breaking on an ordinary attribute. Blocking:
implementer()missesimpl<'a> Runner for Xandimpl devlaunch_runner::Runner for X— both demonstrated green above.opens_a_test_modulefalse-positives when any attribute sits between#[cfg(test)]andmod tests {— demonstrated red above.- The Spec disagreement on naming the three routers (#409 asks for them by name; the doc does not).
Non-blocking: findings 3-5 and the target nit.
Review found two spellings that walked past it and one that broke it, all three
reproduced. `impl<'a> Runner for X` and `impl devlaunch_runner::Runner for X`
dropped into devlaunch-core/src/ as production files left the guard green: it
matched the literal text "impl Runner for " at the start of a trimmed line, and
neither spelling starts that way. The first is the wrapper shape the trait's own
doc invites; the second is how any crate that has not imported the trait names
it, which is the likeliest form of a genuinely new second seam. Meanwhile one
#[allow(...)] between #[cfg(test)] and mod tests { made every wrapper in that
module read as production, because the gate was only honoured when the module
line was literally the next one. Six files here would have broken on that.
The instrument is still "read the source" -- the alternative is a hand-written
list of the wrappers, which is the artefact that rotted in the first place, and
nothing on stable enumerates trait impls from the compiler. What changed is the
unit: a token rather than a line. Comments and literals are blanked out first,
preserving offsets, so a brace in a comment cannot unbalance anything and a
quoted "impl Runner for " is not an impl; the trait is matched by the last
segment of whatever path names it, after optional generics; and #[cfg(test)]
gates a brace-matched region, skipping any attributes between it and the item.
That last one closes a fourth hole nobody had reached: the old scan called
everything below an inline test module test code, so the way past the guard was
to write underneath it.
Rejected: a syn dev-dependency, which is exact but pulls a second copy of syn
with the "full" feature into every build of this workspace for one test, and
sealing the trait, which cannot tell a production impl in devlaunch-core from a
#[cfg(test)] one in the same crate without exporting the seal.
Proof is seven cases running against scan() with source as a string, so no probe
file has to be dropped in the tree to exercise them. All seven were run against
the old line logic first: four red -- both bypasses, the false positive, and the
below-the-module hole. End to end, the reviewer's own probe2/probe3 now fail the
guard by name and probe4 does not.
Also from the review:
- #409 asks to "say the structure and name the three routers". The PR body
claimed the ticket steered away from naming them; reading it again, it does
not -- the same sentence that forbids the count asks for the names. So the
"Part real" bullet names FakeDevpodRealGit, lifecycle's Devpod and
workspace_clone's StubbedLfs, and says which of the three not to read by
analogy: StubbedLfs routes on the `git lfs` subcommand rather than the program,
and only its capture reaches ProcessRunner. The bullet said such a wrapper
hands "everything else" to real processes, which was true of two of the three.
- The doc's recommended `grep -rn "impl Runner for"` returned 11 lines for 9
impls, this PR having added two of the false hits -- self-refuting for a change
about enumerations that rot. The enumeration is now named as the scan, which
prints every impl with its file and verdict, with the grep kept as the eyeball
version and marked as over-reporting.
- devpod.rs claimed "nothing else in the tree decides a devpod outcome from
argv", which the response table three lines above it does. It now says why the
table is outside the corpus rather than pretending it is not an argv reader: a
scripted entry is one test saying what it wants back, not a claim about real
devpod.
- The scan skips a build directory by CACHEDIR.TAG as well as by the name
`target`, so a moved CARGO_TARGET_DIR cannot feed generated sources into it.
#409, under map #406.
|
Addressed in 1dead6d. All three blocking findings taken; the two low ones you 1 and 2 — the guard. Both were real, and the diagnosis behind them was too:
That last change closes a fourth hole you did not reach: the old scan called Proof. All nine green now. End to end with your own probes back in the tree: and Rejected on cost. A 3 — the spec disagreement. You are right, and the PR body was wrong. I 4 — the "Part real" bullet. Taken; it was overstated. It said such a wrapper 5 — Low — the self-refuting grep. Taken. The enumeration is now named as the scan Nit — Nothing left open. Gate from |
blooop
changed the title
1 participant
Builds #409, under map #406. The decision it writes down is #405's resolution.
What was verified before writing
grep -rn "impl Runner for" rust/returns nine:ProcessRunner(
devlaunch-runner/src/lib.rs:419),FakeRunner(
devlaunch-test-support/src/fake_runner.rs:370), and seven test-local wrappers(
ScriptedRunner,FakeDevpodRealGit,Devpod,StubbedLfs,FakeGit,Trips,Rebuilding). Three hold a realProcessRunnerand fall through toit:
FakeDevpodRealGit(flows/listing.rs:1208),Devpod(
flows/lifecycle.rs:3417),StubbedLfs(flows/workspace_clone.rs:4026). Allof #405's numbers hold.
One nuance found while checking:
StubbedLfsroutes on thelfssubcommandrather than the program name, and only
capturereaches the real runner — itspassthrough,sessionanddetachare canned. The doc says so.What changed
Runner's doc (devlaunch-runner/src/lib.rs) — the shape, not a count.One production impl, one shared fake carrying the response table and
DevpodMachine, wrappers over those two. The seven wrappers are not counted,because a count rots the way "once" did, but the three routers are named as
Say what Trips is, and correct the Runner impl count #409 asks:
FakeDevpodRealGit,Devpod,StubbedLfs, with the last flaggedas the one not to read by analogy. What replaces the count is the two shapes a
reader needs to recognise — part real and recorder.
Trips' doc (flows/provision.rs) — a recorder, not a third fake devpod;answers by call index, never reads argv to decide, holds no workspace state, so
a corpus row (
givenstate → argv → exit +thenstate) has nothing to bind toat either end, and driving it over
conformance.jsonwould assert a fixtureagainst itself. Points at
DevpodMachineas the fake the corpus does cover.Trip::script's doc — which argv it re-parses (setup_pass), and that theexpectis the guard, with--commandpinned bySSH_VALUE_FLAGSand acorpus row measured at v0.26.1. Took the ticket's optional one-liner:
&self.argv[at + 1]→.get(at + 1).expect("a --command with a payload"), soa moved payload names the assumption that broke. Test-side only.
devlaunch-test-support/src/devpod.rs's module doc — one paragraph sayingthe two hand-written fakes are the whole population the corpus has to cover,
and saying where the response table sits: it is an argv reader, and it is
outside the corpus because a scripted entry is one test saying what it wants
back, not a claim about real devpod.
The guard
devlaunch-runner/tests/one_seam.rs(new, 9 tests). It scans the workspace andfails if anything but
ProcessRunnerimplementsRunneroutside test code. Thatis the half of the new doc that is load-bearing and the half worth a test: a
wrapper appearing or disappearing is what the doc is now written to survive, but
a second production impl is not a stale sentence — it is a second way for
devlaunch to start a process, and the seam has stopped being one.
The unit is a token, not a line, which is the fix for round one's findings. The
first version matched the literal text
impl Runner forat the start of atrimmed line, and
impl<'a> Runner for Xandimpl devlaunch_runner::Runner for Xboth walked past it;#[cfg(test)]was only honoured whenmod tests {wasthe very next line, so an
#[allow(...)]between them made a whole test moduleread as production. Now comments and literals are blanked out first (offsets
preserved), the trait is matched by the last segment of whatever path names it
after optional generics, and
#[cfg(test)]gates a brace-matched region with anyattributes in between skipped. That last change also closes a hole nobody had
reached: the old scan called everything below an inline test module test code,
so the way past the guard was to write underneath it.
Seven of the nine tests run
scan()over source held as a string, so thespellings are exercised without dropping probe files into the tree. All seven
were run against the old line logic first and four were red — both bypasses, the
false positive, and the below-the-module hole. End to end, the review's own
probe2.rs/probe3.rsnow fail the guard by name andprobe4.rsdoes not.Rejected: a
syndev-dependency, which is exact but pulls a second copy ofsynwith the
fullfeature into every build of this workspace for one test; andsealing the trait, which cannot tell a production impl in
devlaunch-corefrom a#[cfg(test)]one in the same crate without exporting the seal.Two smaller things from the same review: the doc's recommended
grep -rn "impl Runner for"returned 11 lines for 9 impls, so the enumeration isnow named as the scan (which prints every impl with its file and verdict) and the
grep is kept as the eyeball version, marked as over-reporting; and the walk skips
a build directory by
CACHEDIR.TAGas well as by the nametarget.Gate
From
rust/:cargo test --workspacegreen (1253 + 41 + 114 + … all pass,including
aid's interactive suite — #401's flake did not appear),cargo clippy --locked --all-targets -- -D warningsclean,cargo fmt --checkclean.No behaviour changes outside the one test-side
expect.