The conformance corpus's guards can lose a row silently - #510
Conversation
The corpus could lose a row with every suite green. The guard asked whether certain flag names appeared somewhere across all rows, so deleting the regression row for the very defect it was built for left both cargo test and pytest passing: a sibling row still mentioned the flag. Rows now carry a stable id, and both drivers hold the roll call of ids they expect to find. A missing one fails by name on both sides, and the Rust driver also counts the rows it ran. The shape checks are keyed to row ids rather than to the file as a whole, so moving a flag out of the row meant to carry it fails even while some other row mentions it. The honesty guard on each row's provenance ran on the Rust side alone, so a corpus edited from the Python side could lose it silently. Both drivers enforce it now, along with row ids being well formed. The five value-flag tables were parallel in two languages with nothing comparing them, and only about a third of the names have a corpus row to catch a drop behaviourally. Each driver now compares its own live tables against the other language's source. Every row that once read unverified is measured: those were the rows needing a provisioned workspace, and PR #353's review provisioned one and ran them.
Reviewer's GuideStrengthens the shared devpod conformance corpus by giving every row a cross-language identity and provenance contract, binding regression shapes to named rows, and making both fake implementations cross-check their value-flag tables so deleted or inconsistent expectations fail loudly. Sequence diagram for cross-language conformance validationsequenceDiagram
participant Corpus as Conformance corpus
participant Rust as Rust driver
participant Python as Python driver
participant Shim as Devpod shim
participant RustFake as Rust fake
Corpus->>Rust: load rows and expected ids
Rust->>Rust: validate row ids and provenance
Rust->>Rust: run rows and compare count to roll call
Rust->>Shim: read value-flag tables
Rust->>Rust: compare shim tables with Rust expectations
Corpus->>Python: load rows and expected ids
Python->>Python: validate row ids and provenance
Python->>RustFake: read value-flag consts
Python->>Python: compare Rust tables with Python expectations
Python->>Python: validate named regression row shapes
Entity relationship diagram for conformance corpus identitieserDiagram
CORPUS_ROW {
string id PK
string verified
string rationale
}
RUST_DRIVER {
string expected_id
}
PYTHON_DRIVER {
string expected_id
}
CORPUS_ROW ||--o{ RUST_DRIVER : validated_by
CORPUS_ROW ||--o{ PYTHON_DRIVER : validated_by
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
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 669df03 (git diff 669df03...HEAD), in fresh context that never saw this code written. Two axes ran as independent parallel subagents; the mutation section at the end is my own work and I say so plainly. Preflight: ref resolves, diff non-empty (4 files, +558/-56), all 15 checks green.
Everything below was run against a git archive copy of the PR head under a scratch CARGO_TARGET_DIR, cargo 1.98.0 and pytest 9.0.3. Baselines first: cargo test -p devlaunch-test-support conformance 5/5, pytest test/unit/test_devpod_conformance.py 29/29, cargo test --workspace all green.
Standards
No blocking findings. The duplication this PR adds is load-bearing rather than accidental: ROLL_CALL and REQUIRED_SHAPES exist in both languages because one copy is what the ticket is about, and the two copies cannot drift apart in practice — the corpus is the referee. I checked: adding a row and updating only one roll call makes the other driver fail on its unexpected branch, so the two lists are forced into agreement transitively. They are identical today (24 ids, same order; 9 shapes, same order, verified mechanically).
1. The roll call of tables is unguarded, unlike the roll call of rows. (minor)
conformance.rs:398-404 and test_devpod_conformance.py:319-325 each hardcode the same five table names, plus stop special-cased. Nothing asserts that this list is the complete set of *_VALUE_FLAGS symbols either source defines. Verified: adding a sixth-plus table to rust/devlaunch-test-support/src/devpod.rs alone leaves both suites green. That is the same artifact class the ticket distrusts, one level up — a hand-maintained list with no completeness check. It is speculative today (a new table would arrive with a new subcommand), which is why it is minor, but the fix is small: derive the names by scanning each source for *_VALUE_FLAGS and assert the derived set equals the enumerated one.
2. shim_table's quote-counting mis-attributes behaviour-neutral edits to the fakes. (minor)
conformance.rs:431-448 reads the shim's sets with .split('"').skip(1).step_by(2), which treats any double-quoted text between = { and the first } as a flag name. Three plausible edits, all verified:
| edit to the shim (behaviour unchanged) | what the Rust test says |
|---|---|
a comment inside the set: # devpod calls this an "additional feature" |
the fakes "disagree", listing a phantom "additional feature" flag |
_UP_VALUE_FLAGS = frozenset({...}) (matching _STOP's own style) |
"the shim no longer defines _UP_VALUE_FLAGS" — it does |
_STOP_VALUE_FLAGS: frozenset[str] = frozenset() |
"the shim's stop table is no longer empty" — it is empty |
Every one fails in the safe direction, which is the important half. But each blames the wrong thing, and the third and the mirror check at test_devpod_conformance.py:334 are exact-string matches on a whole line, so a formatter or a type annotation breaks them. Filtering parsed entries to those starting with - would kill the first; matching the empty tables by parse rather than by literal line would kill the other two.
Not findings, checked and clear. No docs/ page or the README mentions the corpus, so no doc-sync obligation is missed. The CHANGELOG entry is present under [Unreleased] / ### Fixed. why still needs #[allow(dead_code)], which #353's review already flagged as an argument for a tagged enum; this PR does not make it worse.
Spec
One finding worth acting on before merge.
1. One of the eight provenance upgrades claims a measurement the cited record does not contain. (major)
The ticket says: "Either upgrade those rows to measured with the method recorded, or reword them to say 'not measured here' rather than implying it cannot be done." The PR body cites PR #353's review as the record. Seven of the eight rows check out against it exactly. list-leaves-workspaces-alone does not.
The row now reads (test/fixtures/devpod/conformance.json):
measured: devpod list --output json at v0.26.1 with a workspace provisioned lists it and leaves it Running, on the same alpine devcontainer under a scratch DEVPOD_HOME.
PR #353's review (review 5000623596) enumerates what it measured against the provisioned workspace, and list is not in it:
"…every one measured out as the row says —
stop→Stopped;statustwice → unchanged;ssh --workdir / <ws> --command …on a Stopped workspace → 0 and Running;up <ws>→ Running;delete <ws> --ignore-not-foundanddelete <ws2>→ 0 and gone."
It also scopes itself by name — "Rows 4, 5, 7, 15, 21 and the inherited halves of 16/17" — which is seven, and row 23 is not among them. Its only list measurement is the empty-machine one (list --output json → []/0), which was already measured before this PR. The row also seeds two workspaces, one Running and one Stopped, and asserts both unchanged; the cited run had one alpine devcontainer plus a second workspace that was deleted.
This is the exact defect the honesty guard exists to police, and the guard cannot catch it: every_row_says_how_it_was_verified checks only that the line opens with measured. Either narrow the wording to what #353 actually recorded, or measure it and say so. One line.
2. Gap 3 is closed, not half-closed. The scope call is right. (no action)
The spec's remedy clause is "The value-flag tables are now parallel in two languages with nothing comparing them". A diff of the tables is precisely what that asks for, and it is there and it works in both directions. The "only ~14 of ~44 flag names have a corpus row" clause is the diagnosis of why the missing diff was dangerous, not a demand for 30 more rows — and generating them would have to mark them unverified, trading the honesty problem for a coverage number. I probed the residual: emptying UP_VALUE_FLAGS on both sides passes the table diff vacuously but fails five corpus rows behaviourally, so the covered flags are still guarded by behaviour and the uncovered ones are guarded by the diff. The mode that actually happened — one side changes — is caught for all 44.
3. Both corrections to the ticket verified. (no action)
Eight rows changed provenance, not seven (ids 4, 5, 7, 15, 16, 17, 21, 23 by position; row 17's is a wording follow-on from row 16). Six tables exist, not five, and stop's emptiness is asserted on both sides.
4. "Red first for each" holds. Every mutation the spec names fails a test that names the row or the flag. See below.
Mutation probes (mine, not an axis)
Ten mutations on a pristine copy of the PR head. All reverted; the review worktree is clean.
| mutation | cargo test -p devlaunch-test-support conformance |
pytest test_devpod_conformance.py |
|---|---|---|
delete delete-missing-with-ignore-not-found |
3 fail: roll call names the id, "ran 23 rows and the roll call names 24", shape guard |
1 fails, naming the id |
| add a row, touch neither roll call | 2 fail: "carries rows this driver does not name: [stop-already-stopped-workspace]", count check |
1 fails, naming the new id |
| two rows sharing one id | uniqueness assert fires (24 vs 25) | fires |
verified → who knows |
(its own side) | test_every_row_says_how_it_was_verified fails |
move --init-env behind up's positional |
fails, naming the row | fails, naming the row |
drop --gidmap from the shim's table |
fails | — |
drop --userns from the Rust table |
— | fails |
| add a 7th value-flag table to one fake | green | green (Standards 1) |
quoted comment inside the shim's up set |
fails with a phantom flag (Standards 2) | — |
frozenset({...}) / : frozenset[str] in the shim |
fails claiming the table is gone / non-empty (Standards 2) | — |
The add-a-row direction is loud on both drivers, by name — the unexpected branch, not just the Rust count check. That was the thing I most expected to be one-sided and it is not.
Neither cross-language parser asserts a nonzero parse, but the guard is not vacuous. Both hard-fail when their marker is absent ("the shim no longer defines X"; assert match), and a parse yielding zero entries is caught transitively because all five compared tables are non-empty on the live side. The only silent case is both sides genuinely empty, which the corpus rows then catch for every covered flag. An explicit assert!(!parsed.is_empty()) on each side would make that reasoning local instead of inferred; worth adding for clarity, not for safety.
Verdict
Request changes, on Spec 1 alone. (Posted as a comment: GitHub refuses --request-changes on a PR authored by the same account. The written verdict is the gate.)
The mechanism is sound and I could not break it in the direction that matters: every way I found to lose a row, shadow a row, downgrade provenance or split the two flag tables fails by name on both drivers. The one blocker is a sentence, not a design — list-leaves-workspaces-alone now asserts a measurement that PR #353's review does not record, in the one file whose whole purpose is that its claims are true. Reword it or measure it.
Non-blocking, in the order I would act on them:
- Standards 1 — nothing checks that the enumerated value-flag tables are all of them; a seventh is silent on both suites.
- Standards 2 —
shim_table's quote-counting and the two exact-linestopchecks fail loudly on edits that change no behaviour, blaming the fakes for a disagreement they do not have. - An explicit nonzero-parse assertion in each cross-language parser, so the guard's non-vacuity is stated rather than inferred.
| "name": "list leaves every workspace as it found it", | ||
| "why": "Same read-only claim as status, across the whole machine.", | ||
| "verified": "measured for exit and emptiness on a fresh home; the no-side-effects claim is inherited from `devpod list --help` and both fakes agreeing.", | ||
| "verified": "measured: `devpod list --output json` at v0.26.1 with a workspace provisioned lists it and leaves it Running, on the same alpine devcontainer under a scratch DEVPOD_HOME.", |
There was a problem hiding this comment.
The citation does not cover this row. (major)
The PR body upgrades eight rows to measured on the strength of PR #353's review. Seven check out against it exactly. This one does not.
Review 5000623596 enumerates what it measured against the provisioned workspace, and list is not in it:
"every one measured out as the row says --
stop->Stopped;statustwice -> unchanged;ssh --workdir / <ws> --command ...on a Stopped workspace -> 0 and Running;up <ws>-> Running;delete <ws> --ignore-not-foundanddelete <ws2>-> 0 and gone."
It also names its own scope -- "Rows 4, 5, 7, 15, 21 and the inherited halves of 16/17" -- which is seven rows, and this is not one of them. Its only list measurement is the empty-machine one (list --output json -> []/0), which was already measured before this PR.
The row also seeds two workspaces, one Running and one Stopped, and asserts both unchanged; the cited run had one alpine devcontainer plus a second workspace that was deleted.
every_row_says_how_it_was_verified cannot catch this -- it checks only that the line opens with measured -- so it is the honesty defect the guard polices, sitting inside the PR that strengthens the guard. Either narrow the wording to what #353 recorded (the base text, "measured for exit and emptiness on a fresh home; the no-side-effects claim is inherited from devpod list --help and both fakes agreeing", was accurate), or measure it and say so.
There was a problem hiding this comment.
You are right, and this was the one finding I could not argue with: I read #353's review as having provisioned a workspace and therefore covering everything that needed one, when what it actually enumerates is seven behaviours and list against a provisioned workspace is not among them. Its only list measurement is the empty-machine row, which was already measured before this PR.
Fixed by taking the ticket's other branch for this row rather than inventing a run. It now reads:
unverified for the claim this row is about: PR #353's review provisioned a workspace but never ran
listagainst it, so that a listing disturbs nothing is not measured here -- it is inherited fromdevpod list --helpand both fakes agreeing. What was measured is the exit and the empty case:devpod list --output jsonat v0.26.1 on a fresh home exits 0 and prints[].
It opens with unverified rather than measured because the claim the row exists to make is the no-side-effects one, and that is the half nobody has run. The measured half is named rather than dropped, so the line says which is which instead of averaging them, which is the distinction the whole file is organised around.
So the count is seven upgrades and one rewording, not eight upgrades. The PR body, the CHANGELOG entry and the corpus's own header note all say that now.
Worth recording that your finding is exactly the failure the honesty guard cannot see: it checks the opening word, and a wrong claim opening with measured passes. Making it check the citation would mean parsing prose against a review body, which I do not think is worth building. What caught it was a reader with the cited record open, which is the control that was supposed to catch it.
Review of #510 found two more copies of the shape this ticket is about. The tables saying which devpod flags take a value were written out once per fake, and the guard for them parsed the other language's source: a comment carrying a quote inside a table reported a phantom flag, frozenset({...}) read as the table vanishing, and the empty-table checks were exact-line matches a formatter could break. All failed safe and all blamed the wrong thing. They are one file now, test/fixtures/devpod/value_flags.json, for the same reason the corpus is one file. The Rust fake reads it through a Table enum, so a call site cannot ask for a table that does not exist and adding one is a change the compiler walks you through. The shim reads it by path beside itself, and exits 78 naming the file if it cannot, because a shim with no tables reads every value flag as bare and makes its value the workspace. Both source parsers are gone with it. What is left is a roll call of tables on each side, the same guard the rows have: the file's keys against the Rust enum's, and the file's keys against the shim's own namespace. A seventh table in one place fails by name, which it did not before.
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md # test/fixtures/devpod_shim.py
|
All three addressed, and the two minors turned out to share one fix. Head is Spec 1 (blocking): the citation did not cover the rowConceded without argument. #353's review enumerates seven behaviours and Standards 1 and 2: one fix, because they were one problemYou flagged the table roll call being unguarded, and the source parsers mis-attributing behaviour-neutral edits, and suggested a shared fixture would delete both. It does, so I took it rather than the smaller step.
Two things made it viable, and I checked both before starting rather than assuming:
On the Rust side the tables are reached through a Your What changed about the guarantee, stated plainly. The old diff detected the two fakes disagreeing. They now cannot disagree, which is strictly better, but it is a different claim and worth being explicit about: the mode where one side changes is unrepresentable rather than caught. The residual is unchanged from what you probed. A flag dropped from the shared file changes both fakes together, and for the ~14 covered flags the corpus rows catch it behaviourally on both drivers (verified: removing New mutation probes
The three parser mutations from your table are not reproducible: there is no parser to fool. Gates, and one thing about CI
Main moved three times during this round, twice on the CHANGELOG's Not merging. |
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
1 participant
The conformance corpus is the one place the two fake devpods' expectations live, and its guards had holes that let the file shrink quietly. All three gaps the ticket names are closed, each one red first.
The mutations that passed on main
1. A row could be deleted with everything green. Removing the regression row for the
delete --ignore-not-founddrift, which is the defect the whole corpus was built for:The old guard asked whether certain flag names appeared somewhere across all rows. The sibling row that puts
--ignore-not-foundahead of the positional still mentioned it, so nothing noticed.2. The provenance guard was Rust-only. Rewriting a row's
verifiedline towho knows:3. The value-flag tables were parallel in two languages with nothing comparing them. Dropping
--gidmapfrom the shim'suptable, so the two fakes disagree on whether it consumes the next argv element:What the guards do now
Rows carry a stable
id, and both drivers hold the roll call of ids they expect to find. A row cannot leave without failing by name on both sides, and the Rust driver also counts the rows it actually ran against the roll call's length, so a shorter corpus fails the driver as well as the guard. The shape checks for the flagsdlsends are keyed to row ids rather than to the file as a whole, so moving--init-envbehindup's positional fails even while another row still mentions it.The honesty guard (every row opens its
verifiedwithmeasuredorunverified, every row says why it is there, every id is well formed) runs on both sides now. An invariant enforced on one side of a file two suites edit is an invariant with a hole in it.The value-flag tables are one shared file,
test/fixtures/devpod/value_flags.json, which both fakes read. They were two hand-written copies with nothing comparing them; the first version of this PR added a diff of the two sources, and review found that parsing source text mis-attributed behaviour-neutral edits (a comment carrying a quote reported a phantom flag,frozenset({...})read as the table vanishing). One file deletes both parsers and the whole class. The Rust fake reaches the tables through aTableenum, so a call site cannot ask for one that does not exist; the shim reads the file beside itself and exits 78 naming it if it cannot, because a shim with no tables reads every value flag as bare and makes its value the workspace. Each driver checks the fake it drives against the file: the file's keys against the enum's, and the file's keys against the shim's own namespace.That changes the guarantee rather than only strengthening it, so plainly: the two fakes disagreeing used to be detected and is now unrepresentable. The residual is what it was. A flag dropped from the shared file changes both fakes together, and the ~14 flags with corpus rows catch that behaviourally on both drivers; the ~30 without are unguarded, exactly as an identical edit to both tables was before.
The same mutations after
--ignore-not-foundregression rowthe_corpus_answers_the_roll_callandevery_row_of_the_corpus_holds_for_the_in_process_fake(ran 23 rows, roll call names 24) and the shape guard failtest_the_corpus_answers_the_roll_call,test_the_rows_the_decision_named_keep_their_shapefailverifiedbecomeswho knows--init-envbehindup's positional, flag still in the file_MACHINE_VALUE_FLAGSin the shim--init-envremoved from the shared fileupcorpus rows failuptable emptiedProvenance
Seven of the eight rows carrying an unmeasured claim now read
measured, with the method recorded: those needed a provisioned workspace, and PR #353's review provisioned one (docker provider, alpine devcontainer, scratchDEVPOD_HOME) and ran them.The eighth,
list-leaves-workspaces-alone, does not, and that was review's blocking finding: #353's review never ranlistagainst the provisioned workspace, so the upgrade claimed a measurement its own citation did not contain. It takes the ticket's other branch instead and opens withunverified, naming the half that was measured (the exit and the empty case) rather than averaging the two. The honesty guard cannot catch that class, since it reads the opening word and not the citation; a reader with the cited record open is the control that did.Two corrections to the ticket
unverified. Five open with the word; three more (sshwith a trailing--workdir,ssh --workdirbefore the workspace,listleaving workspaces alone) opened withmeasuredand then inherited part of the claim. Seven distinct behaviours were inherited across those eight rows, which is what the count was tracking.stophas none of its own, and both drivers assert that its list is empty rather than treating an empty parse as one.Gates
cargo test --workspace: all okcargo clippy --locked --all-targets -- -D warnings: cleancargo fmt --check: cleanpixi run test: 612 passed, 6 skippedRe-run on the merged tree after main moved three times during review (twice on the CHANGELOG's
[Unreleased], once ondevpod_shim.py's own docstring). Each merge keeps every line main brought and places this entry under[Unreleased] / ### Fixed, never inside the shipped[0.25.0]where a textual merge puts it.Closes #357
🤖 Generated with Claude Code
Summary by Sourcery
Strengthen devpod conformance corpus safeguards so missing rows, altered expectations, and fake-driver flag mismatches fail explicitly in both test implementations.
Bug Fixes:
Enhancements:
Documentation:
Tests: