The selector draws a workspace id apart instead of whole - #370
Conversation
A picker row was `{owner} | {id}`, where the id is
`<repo-slug>-<ref-slug>-<suffix>`. Two of those three parts are what
somebody choosing a workspace is looking for and the third is machinery:
the eight-character syllable suffix exists to keep two branches from
sharing an id, and reading it is no part of picking one. Rows are now
`{owner} | {repo} | {ref}`.
Both new halves come off the source devpod already reported, so the
picker still opens no records and reads no config: `<cache>/repos/
<owner>/<repo>/<id>` names the repo, and `workspace_id::ref_slug_of`
strips the repo prefix and the suffix off the id to leave the ref-slug.
That inverse lives beside the derivation it undoes, because a caller
spelling out the suffix width or the repo cap for itself would be the
second derivation to disagree with the first.
Three columns and not `owner/repo@ref`, which reads like a spec `dl`
would accept and is not one: `slug` collapses `/` and `-` alike, so a
retyped `feature-auth` row can address the other branch.
And the elision is never unconditional. `chosen` maps a picked row back
to its workspace by the row's own text with the first match winning, so
two rows drawn the same would be one workspace acted on in place of
another — `dl rm` among the verbs this picker opens for. Where a split
would collide, both rows go back to their whole ids, which puts the
suffix on screen in exactly the case it is doing work.
`public-api.txt` is edited by hand for the two added functions, copied
from the `owner_of` line they are shaped after; CI regenerates it.
`chosen` finds a picked row by matching its text with the first match winning, so two rows drawn alike act on one workspace. The key pass caught the split-versus-split case; it cannot see the cross-shape one, because a whole-name row keyed `<owner>\0<name>` and a split row keyed `<owner>\0<repo>\0<ref>` never share a key even when they draw the same string. A workspace dl did not clone, named `devlaunch | main`, drew `blooop | devlaunch | main` beside a clone of blooop/devlaunch@main -- and picking it removed the clone. The comment claiming the shapes could not collide argued it from the names devpod permits, which is the borrowed guarantee `sanitize_title` declines to rest on next door. `offered` now establishes it locally: draw the labels, and if any two are equal, every remaining split goes back to its id, which leaves one shape and one column that is unique per row.
`value` cuts the repo slug to twenty characters only when the id would otherwise overflow, so a reader has two spellings to try. Trying the full one first is right far more often, and it is not always right: for repo `aaaaaaaaaaaaaaaaaaaa-bbbb` at branch `bbbb-cccccccccc` the id used the cut spelling, and the full spelling also explains the string -- so the ref column read `cccccccccc`, a branch of that name being exactly what the row then claimed to be. Both readings really are derivable there, so there is no right answer to pick: it needs a repo slug over the cap with a dash at exactly the cap and a ref beginning with the segment after it. So an id both spellings explain, disagreeing, is refused, and the caller draws the id whole -- which is what `None` already meant. Conservative in one direction: some ids for such a repo are only derivable one way and are refused anyway. Recovering those means re-deriving the cut rule from a ref that has already been fitted, kept in step with `value` forever, for a prettier column on repositories named like that. A repo slug with no dash at the cap is read apart under exactly one spelling however long it is, which the test pins.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideSelector rows now show Sequence diagram for building unique workspace selector rowssequenceDiagram
participant Devpod
participant Selector
participant Listing
participant WorkspaceId
participant Picker
Devpod->>Selector: list workspaces
Selector->>Listing: owner_of(workspace, cache_dir)
Listing-->>Selector: owner
Selector->>Listing: repo_of(workspace, cache_dir)
Listing-->>Selector: repo
Selector->>Listing: ref_slug_of(workspace, cache_dir)
Listing->>WorkspaceId: ref_slug_of(id, repo)
WorkspaceId-->>Listing: ref slug or None
Listing-->>Selector: ref slug
Selector->>Picker: offered(workspaces, cache_dir)
Picker-->>Selector: split labels or whole ids when labels collide
Selector->>Picker: chosen(selected label)
Picker-->>Selector: workspace_id
Flow diagram for collision-safe selector label generationflowchart TD
A["Devpod workspace list"] --> B["Read clone layout: owner and repo"]
B --> C["Parse ref slug from workspace id"]
C --> D{"Owner, repo, and ref all available?"}
D -- No --> E["Draw owner | whole workspace id"]
D -- Yes --> F["Draw owner | repo | ref"]
E --> G["Check rendered labels for uniqueness"]
F --> G
G --> H{"Any collision?"}
H -- No --> I["Offer labels to picker"]
H -- Yes --> J["Replace colliding split rows with whole ids"]
J --> I
I --> K["chosen matches row text to workspace id"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
# Conflicts: # README.md
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.
Retrospective wf-review of #336, #350, #363 and #370 as one surface — the four
iterations on the fuzzy picker that landed inside about three hours, all four merged with
zero review. Because they are sequential iterations, the intermediate states no longer
exist; everything below is judged against current main (a5f7ed8) and every finding
was reproduced there before being written down. Findings are attributed to the PR that
introduced the surviving line. #360 (the invitation drawn inside the picker) was reviewed
separately and is out of scope.
Two axes, run independently and not merged or reranked.
Standards
Repo-documented standards (CLAUDE.md, README.md, docs/rust-rewrite-plan.md) first,
then the Fowler 12-smell baseline. Anything clippy/fmt enforces is skipped.
S1 — offered's distinctness guard compares padded labels, so the cross-shape collision #370 fixed reappears as soon as a third row widens the repo column (blocking)
rust/dl/src/select.rs:138 — if !all_distinct(&labels), where labels came from
drawn() and are already padded to the widest owner and the widest repo in this listing.
ec5c882 ("a workspace named after another row's columns drew the same row") added this
pass because a whole-name row <owner> | <name> can draw the same string as a split row
<owner> | <repo> | <ref>, and chosen maps a picked row back by its text with first match
winning. The pass is right for the two-row case its test pins — but the property it
establishes is "no two label strings are byte-equal", and the property the hazard is about
is "no two rows read the same to a person". Padding separates the two.
Reproduced on main by adding one unrelated third row whose repo name is wider:
// blooop/devlaunch @ main (dl's clone, split row)
// blooop/devlaunch @ feature/auth (dl's clone, split row)
// kinisi-robotics/kinisi_ros @ main (dl's clone — only here to widen the repo column)
// devpod workspace literally named "devlaunch | main", git source github.com/blooop/devlaunchdrawn by offered() on current main as:
blooop | devlaunch | main
blooop | devlaunch | feature-auth
kinisi-robotics | kinisi_ros | main
blooop | devlaunch | main <- the foreign workspace
Rows 1 and 4 differ by exactly one space. all_distinct therefore returns true, the
fallback never runs, and the picker offers two rows that a person scanning cannot tell
apart — one of which is a clone of blooop/devlaunch@main and the other a devpod workspace
somebody else made. dl rm is one of the verbs this picker opens for. chosen still
resolves each string to the right workspace, so nothing is mis-mapped internally; the
failure is that the human picks the wrong row, which is the same outcome ec5c882's commit
message describes ("picking it removed the clone"). Whether the guard fires is decided by
an unrelated row's repo name width.
shared_key (select.rs:250) has the opposite skew — it ignores padding but cannot see
across row shapes — so the module now carries two different notions of "the same row",
neither of which is the one that matters. That is the "one rule, two derivations" shape
devlaunch#55 defect #4 is about, and #370's own commit message cites it as the reason
ref_slug_of lives next to value.
Fix (verified): make both passes compare the unpadded rendering — the collision is
between what the rows say, not how wide they were printed:
fn shared_key(naming: &Naming) -> String {
label(naming, 0, 0)
}With that one substitution the whole dl suite stays green (112 lib + 5 pty + the rest) and
the probe above becomes:
blooop | devlaunch-main-zovomobo <- only the colliding row falls back
blooop | devlaunch | feature-auth
kinisi-robotics | kinisi_ros | main
blooop | devlaunch | main
which is also the scoped fallback README promises ("both rows go back to their full
ids") rather than the whole-listing one offered currently does. It makes the second pass's
cross-shape branch unreachable, leaving it as a cheap backstop for the same-id-in-two-contexts
case that is genuinely unfixable here.
S2 — the cross-shape fallback is listing-wide, which README says it is not (non-blocking, #370)
select.rs:145-150. When the second pass fires, every remaining split row goes back to
its whole id, not just the pair that collided. README says:
Two branches can therefore share the middle and right columns … When that happens both
rows go back to their full ids
and select.rs's own a_collision_only_pulls_in_the_rows_that_collide pins that scoping for
the first pass. One foreign workspace named like another row's columns therefore turns the
entire picker back into hashed ids. The code comment states this outright, so it is a known
choice rather than an oversight — but it contradicts the README sentence and the neighbouring
test's stated principle. S1's fix removes it as a side effect.
S3 — no column budget: one long repo name pushes the branch column off every row (non-blocking, #363/#370)
label() (select.rs:270) never truncates, and widest() pads every row to the widest
entry in the listing. Owner + repo + separators is unbounded, and skim truncates the display
at the terminal width — so a single clone of, say, kinisi-robotics/dependabot-github-actions-codecov-action
costs 15 + 40 + 6 = 61 columns before the branch column starts, on every row in the list. On
an 80-column terminal that leaves 19 for the column #370 exists to add; on a narrower one it
leaves none, for workspaces that have nothing to do with the long-named one. Nothing panics
and the rows stay searchable (skim matches the full text, not the visible part), but the
column can be invisible. A per-column cap with an ellipsis — or measuring the repo column
against the terminal width — would bound it.
S4 — widest's documented unicode property is unpinned, and is wrong for wide characters (non-blocking, #363)
select.rs:284:
/// The width of the widest of *texts*, in the characters a terminal draws and
/// `{:width$}` counts — not the bytes a non-ASCII owner or repo would measure.
The first half of that sentence is not true. A repository directory name may be non-ASCII
(the column is read off <cache>/repos/<owner>/<repo>/<id>, the directory, which is why
kinisi_ros keeps its underscore), and both chars().count() and {:width$} count
scalar values, not terminal cells. Reproduced on main:
blooop | devlaunch | main
blooop | 日本語 | main
— three characters padded to nine, drawn as twelve cells against nine. Cosmetic, and the
honest fix may just be to correct the doc comment; but see S6, because no test would notice
either way.
S5 — dead scaffolding from the superseded layouts (non-blocking)
rust/devlaunch-core/src/flows/listing.rs:191—SourceDescription's doc still says
"How a source reads indl --lsand in the fuzzy picker. One value for both columns
and one function for both callers, so the kind shown and the detail shown cannot come from
two different readings of the same source." #363 removed the picker as a caller; the
two-caller rationale is the whole justification for the type and it now has one caller
(listing.rs:1112,dl --ls) plus a.detailread inlifecycle.rs:2283.rust/dl/tests/picker.rs:512—prompt_row's doc: "a match row carriesowner | idand
the prompt carries only what was typed." That is #363's layout; #370 made it
owner | repo | ref. The heuristic itself (starts_with('>') && !contains('|')) still
works, so this is prose only.rust/dl/src/select.rs:1-66— the module header is now ~65 lines narrating four
superseded designs (Python's{id} | {kind} | {detail}, skim's default layout, the
{owner} | {id}intermediate, the invitation's old print site) in front of a ~470-line
module. Divergent Change: four PRs in three hours each rewrote this block. The history
belongs in CHANGELOG (where it already is, four times over); the header should describe
what the module does now.
S6 — test strength: three documented invariants survive mutation (non-blocking)
Mutate-and-observe against cargo test -p dl on main:
| mutation | suite |
|---|---|
widest: text.chars().count() → str::len() (bytes) |
green — 112 + 5 + all |
shared_key: drop both \0 delimiters |
green |
named: delete the whole first (shared_key) pass |
red (a_collision_only_pulls_in_the_rows_that_collide) |
label: swap repo/ref columns |
red |
The first two are properties the code's own doc comments state as load-bearing —
widest's "not the bytes a non-ASCII owner or repo would measure", and shared_key's "without
a delimiter no key could tell the repo a with ref bc from the repo ab with ref c" —
and nothing tests either. One offered() case with a non-ASCII repo directory and one with
(repo: "a", ref: "bc") beside (repo: "ab", ref: "c") under one owner would pin both.
Separately: tests/picker.rs's fake devpod never produces a split row. Its three
workspaces are two gitRepository sources and one /home/dev/myproject, so repo_of
answers None for all three and every pty row is the two-column <owner> | <name> shape.
#370 — the PR that made rows three columns and added the elision — added no pty test and
touched none. The harness #350 built for exactly this reason ("an option that is spelled
right and does nothing passes every one of them") does not cover the feature #370 shipped.
Adding one localFolder source under the scratch XDG_CACHE_HOME to World::new() would
put a split row on the screen for one line of JSON.
S7 — multi-select correctness: no defect found (#336)
Checked and could not break:
- Which verbs take several.
Verb::several_at_once(cli.rs:234) is an exhaustive
match, so a new verb cannot inherit single-select by omission.Arity::Onesets
multi: false, so TAB is inert rather than misleading forattach/--/restart/
recreate/reset, andinvitation()correctly withholds the TAB clause there. - Partial failure.
render_select(commands.rs:1097-1120) keeps going after a refusal
and latches the first non-Doneending. I specifically tried to break this with the
Ending::Child(Exit::Code(0))case — a success that is notEnding::Donewould latch and
swallow every later failure — butDevpodRefusedis only constructed behind
!exit.is_success()in bothworkspace_stop(lifecycle.rs:735) andworkspace_delete
(lifecycle.rs:992), and the five batching verbs all returnEnding::Doneon success
(Launched::Ready | AlreadyRunningforup/code/dotfiles, which do not attach). So
the contract holds today. It holds by coincidence of the other arms, though —if matches!(ending, Ending::Done)would be more robust as "latch the first ending whose
code()is non-zero". get_index.Rowcarries its position androws_ofpins[0, 1, 2]. The mutation
that reintroduces the original bug (removing theget_index/set_indeximpl) does go red.
One soft note: refresh.rearm() runs once per workspace, so a batch of ten spawns up to
ten detached dl --update-cache children, each driving devpod list. They cannot corrupt
the cache — write_atomically stages through a pid-unique temp name — but nine of them are
describing a world the batch is still changing. Re-arming once after the loop would be one
child describing the finished state, which is what the entry claims to want.
Spec
No spec available — for all four PRs.
closingIssuesReferences is empty on #336, #350, #363 and #370; no commit message on any of
the four branches carries a Closes/Fixes reference; and a search of all 160 issues in
blooop/devlaunch turns up no ticket matching any of the four titles. The only picker ticket
in the tracker is #359, which belongs to #360 and is out of scope here. The timeline
cross-references are release issues (#337, #372) and a CI issue (#378), not specs.
So there is nothing to quote a line of, and this axis reports no findings rather than
requirements reverse-engineered from the diffs. Four consecutive changes to one user-facing
surface with no written statement of intended behaviour ahead of the code is itself worth
naming: the acceptance criteria for "the columns are the owner and the workspace id" exist
only inside the PR that implemented it.
One thing that is checkable — repo-documented behaviour that the diff contradicts — and
belongs here rather than under Standards, since CLAUDE.md makes the docs a deliverable
("Keep README up to date … ensure the README matches"):
P1 — the shipped CHANGELOG entry for #336 documents multi-select for two spellings that do not have it (non-blocking, #336)
CHANGELOG.md:535-537, in the released [0.6.0] section and still on main:
dl rm,dl stop,dl up,dl codeanddl dotfiles(and the--rm/--stop
spellings) with no workspace named now open the picker in multi-select
Both parenthesised spellings are wrong on main, verified by driving cli::resolve:
dl --rm => Ok(Select { verb: Attach { rm: Yes }, devcontainer: None })
several_at_once = false <- single-select
dl --stop => Err(RetiredFlag(Stop)) <- never opens a picker at all
--stop is a retired flag (cli.rs:448, recognised only to be refused), and cli.rs:171
states the split explicitly — "This is --rm, and Verb::Remove is rm — docker's
split" — so dl --rm is an attach that cleans up after itself, which several_at_once
correctly refuses to batch. The claim is repeated verbatim in #336's PR body. README gets it
right (it lists only the five verbs), so this is CHANGELOG-only: drop the parenthesis.
Verdict
Comment. (GitHub refuses --request-changes on a same-account PR; these are merged, so
the written verdict is the gate.)
One finding I would have blocked on before merge:
- S1 —
offered's distinctness guard compares padded labels, so the cross-shape
collision #370 was written to close reappears as two rows one space apart as soon as any
third row widens the repo column. Reproduced ona5f7ed8; the fix is one line
(shared_key→label(naming, 0, 0)), keeps all 112 lib tests plus the 5 pty tests green,
and restores the scoped fallback README already promises.
Non-blocking follow-ups: S2 (listing-wide fallback vs README), S3 (no column budget),
S4 (wide-character padding), S5 (three stale doc sites from the superseded layouts),
S6 (two documented invariants survive mutation; no pty coverage of #370's split row),
P1 (CHANGELOG 0.6.0 claims multi-select for --rm/--stop).
The multi-select correctness this group's riskiest change rests on (S7) I could not
break: the arity matrix is exhaustive, the batch's first-failure latch holds for every ending
those five verbs can produce, and the get_index fix is genuinely pinned.
Spec axis: no spec available for any of the four.
1 participant
A picker row was
{owner} | {id}, where the id is<repo-slug>-<ref-slug>-<suffix>. Two of those three parts are what somebodychoosing a workspace is looking for; the third is machinery — eight characters of
hash, there so two branches cannot share an id, and no part of choosing one. Rows
are now
{owner} | {repo} | {ref}:Both new halves come off the source devpod already reported, so the picker still
opens no records and reads no config:
<cache>/repos/<owner>/<repo>/<id>names therepo, and
workspace_id::ref_slug_ofstrips the repo prefix and the suffix off theid to leave the ref-slug. That inverse lives beside the derivation it undoes,
because a caller spelling out the suffix width or the repo cap for itself would be
the second derivation to disagree with the first (devlaunch#55, defect #4).
Three columns and not
owner/repo@ref, which reads like a specdlwouldaccept and is not one:
slugcollapses/and-alike, so a retypedfeature-authrow can address the other branch.The elision is never unconditional
chosenmaps a picked row back to its workspace by the row's own text, first matchwinning — so two rows drawn alike act on one workspace, and
dl rmis one of theverbs this picker opens for. Where a split would collide, the rows go back to their
whole ids, which puts the suffix on screen in exactly the case it is doing work.
Self-review found two defects, both fixed here
cannot see a cross-shape collision: a whole-name row keyed
<owner>\0<name>and asplit row keyed
<owner>\0<repo>\0<ref>never share a key even when they draw thesame string. A foreign workspace named
devlaunch | maindrewblooop | devlaunch | mainbeside a clone ofblooop/devlaunch@main, and pickingit removed the clone.
offerednow establishes distinctness locally instead ofarguing it from the names devpod permits.
valuecutsthe repo slug to twenty characters only on overflow, so a reader has two spellings
to try, and for
aaaaaaaaaaaaaaaaaaaa-bbbbatbbbb-ccccccccccboth explain thestring. The ref column read
cccccccccc— a branch of that name being exactlywhat the row then claimed to be. Both readings are genuinely derivable, so an id
both spellings explain is refused and the caller draws it whole.
Known limits, not fixed here
Two workspaces sharing an id in two devpod contexts draw one row. That predates the
columns —
{owner} | {id}collided identically — and closing it means addressing aworkspace by more than its id.
ref_slug_ofis conservative in one direction: some ids for a repo whose slug has adash at exactly the cap are only derivable one way and are refused anyway.
Recovering those means re-deriving the cut rule from a ref that has already been
fitted, kept in step with
valueforever, for a prettier column on repositoriesnamed like that.
Checks
cargo test --workspace(1612),clippy -D warnings,fmt --check,pixi run lint.public-api.txtis edited by hand for the two added functions, copied fromthe
owner_ofline they are shaped after — this container has no nightly toregenerate it with, so CI's diff is the real check.
🤖 Generated with Claude Code
Summary by Sourcery
Show workspace picker entries as readable owner, repository, and branch columns while retaining full ids whenever abbreviation could make selection ambiguous.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: