Repository navigation
Make a workspace's source one value instead of a tag beside a parallel field - #132
Conversation
…ield
`Workspace` carried a `source_type` string beside a `source` string. Two of the
three tags said what `source` held -- a path for "local", a URL for "git" -- and
the third did not: "unknown" put `str(the raw devpod object)` into the same
field. The field was therefore a path for some values of the tag and a debug
repr for others, and nothing stopped `Workspace("ws", "git", "/home/dev/p", ...)`
from being built and believed.
The one reader that branched on the tag was repo discovery, an `if`/`elif` with
no `else`. The "unknown" arm fell off the end and left no trace, which is the
same outcome as a source it read fine and found no repo in.
Replaced with a sum type -- LocalFolder(path) | GitRepository(url) |
UnrecognisedSource(payload) -- so each arm carries only what that arm has, and
the arm devlaunch cannot read holds the object devpod sent rather than a
rendering of it. That arm is reachable rather than defensive: devpod's own
workspace source carries `image` and `container` alongside `localFolder` and
`gitRepository`, read off the shipped binary's JSON struct tags.
Every reader is now total over the arms, enforced by `ty` in CI via a local
NoReturn helper (`typing.assert_never` is 3.11+, this package supports 3.10).
Measured rather than assumed: an exhaustive handler passes, deleting an arm
fails, and adding a fourth arm to the union fails at the handler that used to
be complete.
- Repo discovery now warns, naming the workspace and the payload, instead of
dropping it. A `git` source it read fine and found no GitHub owner/repo in
stays quiet -- that is an ordinary answer, not a defect.
- The purge ownership predicate keeps its behaviour and its refusal of non-local
sources. Half of that refusal is now structural: the unreadable arm has no
path to test for containment, so the mistake is no longer expressible. A git
source does still carry a string that could be a path, so that arm is refused
by name and stays under test.
- `dl --ls` and the fzf picker ask one function for both columns, so the kind
shown and the detail shown cannot come from two different readings.
- A `source` that is not an object at all is refused by the listing parser,
where an unreadable listing already has an answer. It used to reach a
substring test: `"localFolder" in "/srv/localFolder/x"` is True.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideRefactors workspace source representation from a string/tag pair into a typed sum type, updates all consumers to handle the new union exhaustively, and improves handling and reporting of unreadable sources and malformed devpod listings. Flow diagram for parsing and handling workspace sourcesflowchart LR
A[devpod list JSON] --> B[parse_workspaces]
B --> C{each workspace entry}
C --> D[Workspace.from_json]
D --> E[parse_workspace_source]
E --> F{WorkspaceSource arm}
F --> G[LocalFolder]
F --> H[GitRepository]
F --> I[UnrecognisedSource]
G --> J[is_devlaunch_clone]
H --> J
I --> J
G --> K[discover_repos_from_workspaces]
H --> K
I --> K
G --> L[describe_source]
H --> L
I --> L
L --> M[print_workspaces / fuzzy_select_workspace]
I --> N[logging.warning in discover_repos_from_workspaces]
B --> O[UnreadableWorkspaceList if source is not an object]
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 #132 +/- ##
==========================================
+ Coverage 93.21% 94.03% +0.82%
==========================================
Files 18 18
Lines 2018 2046 +28
==========================================
+ Hits 1881 1924 +43
+ Misses 137 122 -15
🚀 New features to boost your workflow:
|
…sentinel `fuzzy_select_workspace` is one of the five call sites this change touches and had no tests at all, so the arm this ticket is about could have been dropped from the picker too and nothing would have said. It now has one: a workspace devpod describes as an image is offered in the list, and picking that row maps back to its id. `_unhandled_source(...)` calls join `raise AssertionError` on coverage's exclude list, for the same reason it is already there. `ty` runs in CI and rejects any call to that helper that is reachable, so in a build that type-checks those lines cannot execute -- counting them as missed coverage asks for a test that cannot be written without first breaking the type check.
Review finding, measured rather than argued: `git -C "" remote get-url origin`
exits 0, because `-C ""` is a no-op. So a `LocalFolder("")` reaching repo
discovery would be credited with whatever repository the person running `dl`
happened to be standing in. The guard against it was a truthiness test one level
inside the arm -- the same defect this change exists to remove, an empty string
acting as a sentinel inside the arm that is supposed to mean "a folder dl can
read" -- and deleting that guard left the whole suite green, because every
discovery test mocks the remote lookup.
Rejected at the parse boundary instead, where it becomes an unreadable source:
the arm now cannot hold an empty path or an empty URL, and the guard is gone
rather than tested.
Also pinned the parser's key precedence. That is the one link in the chain the
type checker cannot cover: a reader that misses an arm is a build failure, but
this is a producer, and a producer that quietly stops producing an arm
type-checks fine.
Two review findings, both measured on the branch rather than argued.
`{"localFolder": {"str_of_a_dict": "is back"}}` built a `LocalFolder` whose
`path` was a dict -- literally the defect this change exists to remove, one
level in -- and both readers that treat it as a path raised `TypeError` on it.
devpod's listing is JSON dl did not write, so a key now has to be non-empty text
before it counts as a folder or a URL; anything else is a source dl cannot read,
which is an arm, and the object is kept whole in it. Not an unreadable listing:
unlike a `source` that is not an object at all, there is an object here to keep.
The other half of the ticket's red test was vacuous. It asserted the warning
mentions "image" against a workspace called `from-an-image`, so stripping the
payload out of the message left it passing. Both halves now assert on strings
that appear nowhere else -- the id is `ws-42`, and the payload is the image tag.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two axes, run as independent agents in fresh context, each in its own git worktree so that nothing mutating could reach the branch while it was being committed to. Neither axis's findings were merged or reranked into the other's.
Standards
1. LocalFolder("") was representable, and the guard against it was load-bearing and untested. Measured, not argued: git -C "" remote get-url origin exits 0, because -C "" is a no-op — so a workspace with an empty folder would have been credited with whatever repository the person running dl happened to be standing in. The truthiness guard inside the arm was the only thing stopping it, and deleting that guard left 1076 passed, because every discovery test mocks the remote lookup. That is this ticket's own defect class one level down: an empty string acting as a sentinel inside the arm that is supposed to mean "a folder dl can read".
2. Key precedence was documented as load-bearing and was the one link the checker cannot cover. Swapping the two ifs survived the suite. Adding a fourth arm to the union produced 3 diagnostics — at each reader — and none at the parser, because a constructor is covariant. So a future arm would break every reader and then silently never be produced.
3. Repeated Switches — argued, and it is not one. Fowler's smell bites because a missed case is silent; measured here it is a build failure at 3/3 readers. Collapsing to polymorphism would drag display strings, the cache directory, and logging plus ws.id — which a source does not own — onto three-line records: Feature Envy traded for a smell the checker already pays for. Correct shape; left alone.
4. _unhandled_source — justified. It is the documented pre-3.11 assert_never; typing_extensions is not a dependency and py310 is in the CI matrix.
5. describe_source -> Tuple[str, str] — representable-but-harmless. Two values, two call sites, unpacked immediately. Noted and left.
Spec
Re-measured rather than read, per this map's fifth principle.
Satisfied. ty enforcement holds — deleting an arm errors, and adding a fourth arm to WorkspaceSource raises a diagnostic at each of the three readers. #107's --purge guard is carried and its battery stays dead: six purge mutants, all killed. The ticket's ask to check all five call sites first was done before building, and all five are handled. #131 was not touched (shutil.rmtree is untouched, no EACCES handling in the diff) and is marginally easier afterwards, being the same principle-3 shape.
Blocking — 1. The sentinel was still constructible at the parse boundary. Measured:
parse_workspace_source({"localFolder": {"str_of_a_dict": "is back"}})
-> LocalFolder(path={'str_of_a_dict': 'is back'})
is_devlaunch_clone and discover_repos_from_workspaces then both raise TypeError. Exactly the defect class this PR fixed one level up.
Blocking — 2. Half of the ticket's own red test was vacuous. Stripping the payload out of the warning message survived all 1076 tests: assert "image" in caplog.text passes on the workspace id "from-an-image" alone.
Non-blocking. The PR body overclaimed that "a workspace whose tag says git and whose value is a local path" is unrepresentable — GitRepository("/home/dev/project") is constructible, and the PR's own test deliberately builds one. Body corrected.
What changed in response
All four actionable findings are fixed on the branch.
- A key must be non-empty text before it counts as a folder or a URL. Anything else — empty, a nested object, a number, a list,
null— is a source dl cannot read, and the object is kept whole in that arm. Deliberately not an unreadable listing: unlike asourcethat is not an object at all, there is an object here to keep. - The empty-value truthiness guard inside repo discovery is deleted rather than tested, because the value can no longer arrive.
- Key precedence is pinned by a test, with the covariance gap stated in the docstring.
- Both halves of the red test now assert on strings that appear nowhere else: the workspace id is
ws-42, and the payload assertion is on the image tag. - The fuzzy picker — one of the five call sites, and the one with no tests at all before this — now has one: a workspace dl cannot read is offered in the list and selecting that row maps back to its id.
Mutation battery re-run against the fixed head: 17 mutants, 17 killed, no survivors — including the two the Spec axis found surviving. Run in its own worktree.
Verdict
Approve. No blocking findings outstanding. Both blocking Spec findings and both actionable Standards findings are fixed and pinned by tests that were red first. describe_source returning a bare pair is recorded as a nit and deliberately not changed.
Closes #127.
The shape that was there
Workspacecarried asource_typestring beside a parallelsourcestring.Two of the three tags described what
sourceheld — a path for"local", a URLfor
"git"— and the third did not:"unknown"putstr(the raw devpod object)into the same field. So the field was honest for two values of the tag and a
debug repr for the third, and nothing stopped
from being constructed and believed.
The one reader that branched on the tag was repo discovery: an
if/elifwithno
else. The"unknown"arm fell off the end and left no trace — the sameoutcome as a source it read fine and found no repo in.
The shape now
Workspace.source_typeandWorkspace.sourcebecome one field,source: WorkspaceSource. Each arm carries only what that arm actually has, and the armdevlaunch cannot read holds the object devpod sent rather than a rendering of it.
What that made unrepresentable
arm is its kind, so nothing can say
gitwhile holding what the code willthen treat as a path. (
GitRepository("/home/dev/project")is stillconstructible, and deliberately so —
devpod up <path-to-bare-repo>reallydoes record a path-shaped
gitRepository, and a test builds one. What is goneis the disagreement, not path-shaped strings.)
str(dict)in a field typed as a path or a URL. The unreadable arm has nopath field and no URL field for one to be smuggled into, and no arm's field can
be filled with a value of another type: a key has to be non-empty text before
it counts as a folder or a URL, so
{"localFolder": {...}}and{"localFolder": ""}are both sources dl cannot read rather than folders.Exhaustiveness is enforced, and that was measured rather than assumed
The
/constructive-modelingskill warns that Python exhaustiveness is decorativeunless a checker runs in CI, and says to compile the negative case. Run against
this repo's own pinned
ty:Expected Never, found C & ~A & ~BThat third row is the property worth having: adding a case breaks every consumer.
It holds for
isinstancechains and formatch.typing.assert_neveris 3.11+ and this package supports 3.10, so the sameenforcement comes from a local helper taking a
NoReturnparameter — verified toproduce identical diagnostics. When
Workspacechanged,tyflagged 35diagnostics across every consumer, which is the obligation propagation doing its
job.
The unreadable arm is reachable, not defensive
devpod's own workspace source carries
imageandcontaineralongsidelocalFolderandgitRepository— read off the shipped binary's JSON structtags, not from documentation. So
devpod up ubuntu:24.04produces exactly the armthat was being dropped in silence.
Not adding arms for
imageandcontainer: all five call sites would treat themidentically to "unreadable", so they would be dead arms bought at the cost of two
more cases in every reader.
The five call sites
dropping it. A
gitsource it read fine and found no GitHub owner/repo in staysquiet — that is an ordinary answer, not a defect, and warning about it would
train users past the warning that matters. A warning rather than a debug line
because
dlpins logging at INFO with no verbosity flag, so a debug line herewould be unreachable, which is the same silence.
--purgeownership predicate (dl --purge deletes workspaces devlaunch never created #107's, merged in dl --purge deletes only the workspaces devlaunch created #129) keeps itsbehaviour and its refusal of non-local sources. Half of that refusal is now
structural: the unreadable arm has no path to test for containment, so the
mistake is no longer expressible. A git source does still carry a string that
could be a path —
devpod up <path-to-bare-repo>records agitRepository, andnothing stops that repo living in the cache — so that arm is refused by name and
stays under test.
dl --lsand the fzf picker were the two call sites that only ever wantedthe tag. They now ask one function for both columns, so the kind shown and the
detail shown cannot come from two different readings of the same source.
sourcethat is not an object at all, where anunreadable listing already has an answer (
UnreadableWorkspaceList, from Stop reporting an unreadable workspace list as an empty one #122).It used to reach a substring test:
"localFolder" in "/srv/localFolder/x"isTrue, and indexing a string with"localFolder"is aTypeError.Verification
tyclean, ruff clean, pylint 10.00/10, and everyadded line of
dl.pycovered.nothing could sweep a live mutation into a commit. Covers every mutant dl --purge deletes workspaces devlaunch never created #107's
battery covered (git source counts as ours, cache root counts as a clone,
containment becomes a string prefix, everything is ours, the split puts
everything in
mine) plus eleven new ones for the arms, the parser and thedisplay — including the two the review found surviving. Survivors: none.
dl --lsagainst the host's realdevpod listrenders unchanged,and a listing carrying
{"image": "ubuntu:24.04"}was pushed through the realparse_workspaces→print_workspaces/describe_source/discover_repos_from_workspaces/workspace_ownershippaths — the row showsup in the listing, the warning names it, discovery still finds the repo beside
it, and ownership puts it in
foreign.This PR carries the release
A standalone
Release 0.0.19commit bumpspyproject.tomlandconda.recipe/recipe.yamltogether and adds one## [0.0.19]CHANGELOGsection covering four merged PRs, not just this one:
gatejob (Added); CI runs on every pull request, not only those targetingmain(Fixed)--purgedeletes only devlaunch's own workspaces, and names what it leaves (Changed ×2); scopingXDG_CACHE_HOMEnow actually protects it (Fixed)The
0.0.17and0.0.18sections are untouched.