Skip to content

Fix what a clean-room reconstruction of the spec found - #539

Open
blooop wants to merge 4 commits into
mainfrom
claude/clean-room-arch-review-re7fvf
Open

Fix what a clean-room reconstruction of the spec found#539
blooop wants to merge 4 commits into
mainfrom
claude/clean-room-arch-review-re7fvf

Conversation

@blooop

@blooop blooop commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Three fixes, all of them defects found by reconstructing this tool's specification from its observable surface alone, with the implementation withheld.

Updated for f0d2e4d. An adversarial review found nine problems in the first three commits, one of them a rule this description asserted and the grammar does not implement. The fourth commit fixes them and this description reflects the corrected state.

How they were found

Five agents were each given a sealed corpus and asked to write the implementation spec they would hand to an engineer: README.md, the six user-facing docs/ pages, verbatim dl --help and aid --help, and an index of all 2,496 test names with no bodies. No source, no CLAUDE.md, no development.md, and neither archival planning document, since rust-rewrite-plan.md states the layer decision outright and would have given the answer away.

The primary result is not in this diff and is worth recording: three of the five reconstructed the real architecture (four layers, runner to clients to domain to flows, dependencies strictly downward, the crate split with devlaunch-runner as a leaf, render.rs owning every user-facing byte). One inferred that a fitness test must exist asserting only one type spawns processes, which is rust/devlaunch-runner/tests/one_seam.rs. The architecture is legible from outside the implementation. What the exercise actually produced was a list of places where the documented behaviour is wrong, underspecified, or self-contradictory, which is what is fixed here.

Every claim was verified against the tree before being acted on. One in the same batch was verified and rejected: that the delete guard inspects a recorded clone while the delete removes a derived one. Both route through the same resolve_clone_path, so it is not a defect and nothing here changes it.

1. The README said the wrong thing about the terminal title

README.md said dl blooop/devlaunch names the pane devlaunch-main-3j1t, "the workspace id, the same string dl --ls prints and the container's hostname carries". docs/workspace-tools.md says the opposite about the same command: the pane is devlaunch@main, and it contrasts that against exactly the string the README asserted.

The docs page is right. titled() in rust/devlaunch-core/src/flows/launch.rs answers workspace.label() whenever the devpod id is the derived one, which is every launch that does not name a recorded id, and an_id_metadata_recorded_is_titled_by_that_id_and_not_by_the_triples_label pins the single arm that differs. The README was describing behaviour that workspace-tools.md already calls what it "used to be".

The corrected sentence names both of the title's transformations. Dropping the hashed suffix alone gives devlaunch-main; the dash before the branch is also spelled @.

2. Where --force may sit was undocumented, and the first attempt got the rule wrong

docs/cli.md writes the flag in the trailing position a dozen times without saying the position is load-bearing. The rule has two halves, and the first version of this PR documented only the first and generalised it into a law:

  • Ahead of the verb it refuses. In the verb slot it is read as the verb (Unknown command '--force'); in the workspace slot as the workspace (Unknown workspace '--force'). Both exit 1, nothing deleted.
  • Past the verb it is read, but only rm and rme do anything with it. up, stop, kill, code, restart, recreate, reset and dotfiles take the flag and drop it silently. That is the one place this grammar discards a word rather than refusing it, and kill is the case worth remembering, since the README says there is no dl WORKSPACE kill --force to type and in fact it parses.
  • The selector form cannot be forced at all. With no workspace there is no slot after the verb, so a bare dl rm --force is the verb-slot refusal, and its suggested dl rm -- --force would run the flag as a shell command.
  • A global command has no slots, so dl --force --prune and dl --prune --force are one line.

The original text claimed "a flag that has landed somewhere it cannot mean what you meant refuses rather than being dropped", which is true of the first bullet and false of the second. cli.rs says so twice already ("the flag is the no-op it is on up and stop"; "every other verb drops it on the floor"). Verified against the built binary rather than inferred.

It is now its own section beside the other grammar sections rather than inside ### --rm: the throwaway workspace, where it had separated "It stops at work that is nowhere else" from the --rm that sentence's "It" refers to, handing the reader --force as the antecedent: the exact thing that goes past that check.

3. A second copy of one fact, now diffed against the code

The terminal title is stated in three places: the README's feature list, docs/workspace-tools.md where the reasoning lives, and flows/launch.rs where it is computed. The standing rule permits the copies only with a test beside them that diffs them.

The first version diffed the two prose pages against each other, which pins agreement rather than truth: had the drift been repaired by editing the docs page down to the README's wrong string, that guard would have passed while its message claimed titled() had been consulted. It had not. The guard now holds both pages against the module that computes the title, which is the one copy that cannot be wrong without the behaviour being wrong. Both drift directions were checked, including that inverted repair.

Anchoring the regex on the example command fixed two more: it no longer matches inside "every prompt renames the pane", which docs/workspace-tools.md and flows/provision.rs both say of the prompt's own later write, and set equality no longer forbids docs/ from documenting the examples that are titled differently, which it does. All three copies now cite the guard, so whoever edits one learns the other two exist.

This is the judgment call in the PR. If you would rather single-source the fact than guard the copies, that work is confined to the third and fourth commits.

Deliberately not fixed here

  • dl --ls is serial, one devpod status per workspace. Now #540.
  • Identity has accreted a stored layer under a derived design. reconcile.rs joins by path and never by id, plus a resumable two-phase migration. Already in flight in recent commits.
  • The picker's identity is row text, which is what generates the collision-column machinery, while every_row_carries_its_own_index_or_marking_cannot_accumulate shows indices already exist.

Worth a separate look: roughly thirty tests pin behaviour byte-for-byte against the retired Python build, an oracle nothing can run any more, while names_python_refuses_for_exotic_unicode_are_accepted_here records where the parity is deliberately partial.

Testing

test/ minus e2e: 662 pass. Doc guards (test_docs_prose, test_docs_links, test_citations_resolve, the new guard) 284 pass; test_readme_cli_doc and test_bash_completion 50 pass against a freshly built dl. The two test_devcontainer_manifest failures in this container are pre-existing on an unmodified tree and need pixi, which is not installed here. No Rust behaviour changed; the only rust/ edit is a doc comment naming the guard.

blooop added 3 commits August 30, 2026 16:56
README said `dl blooop/devlaunch` names the pane `devlaunch-main-3j1t`, "the
workspace id, the same string `dl --ls` prints and the container's hostname
carries". docs/workspace-tools.md:511 says the opposite about the same command:
the pane is `devlaunch@main`, the id read for a person with the hashed suffix
off, *where* `dl --ls` and the hostname say `devlaunch-main-3j1t`.

The docs page is right. `titled()` in flows/launch.rs:2850 answers
`workspace.label()` whenever the devpod id is the derived one, which is every
launch that does not name a recorded id, and `an_id_metadata_recorded_is_titled_
by_that_id_and_not_by_the_triples_label` pins the one arm that differs.

The README sentence is the pre-change behaviour that workspace-tools.md:530
already describes as what it "used to be". Nothing caught it: the README's guard
(test_readme_cli_doc.py) holds flags to being mentioned, not claims to being
true, so the most read sentence about this feature was the least checked one.

Found by a clean-room reconstruction of the spec from the CLI surface, the docs
and the test names alone, with the implementation withheld.
docs/cli.md writes `dl <ws> rm --force` a dozen times and never says that the
position is load-bearing. It is: `--force` has to follow both the workspace and
the verb. In the verb slot it is read as the verb (`Unknown command '--force'`)
and in the workspace slot as the workspace (`Unknown workspace '--force'`), both
exit 1 with nothing deleted. A global command has no slot for it to fall into,
so `dl --force --prune` and `dl --prune --force` are one line.

Two rules for one spelling, discoverable only by being refused, and written down
until now only in the names of the tests that pin them. This says it once beside
the docker paragraph that already explains which `--force` is which, and cites
those tests so the rule and its guard are one lookup apart.
The terminal title is stated on two pages: the README's feature list, where a
reader meets it, and docs/workspace-tools.md, where the reasoning for the
spelling lives. That is a second hand-maintained copy of one fact, which this
repository allows only with a test beside it that diffs the copies.

The rule earned the test rather than being applied on principle. The two pages
had already drifted apart on this exact sentence, in opposite directions, about
the same example command, and nothing failed: the README's guard holds a flag to
being mentioned, not a claim to being true.

The instrument is the sentence both pages already write, "names the pane `<x>`",
so neither carries a marker for this test's benefit and a page that drops the
claim fails here rather than passing quietly. Reverting the README to its old
wording reproduces the failure.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @blooop, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 5 days and 4 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@sourcery-ai

sourcery-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Reviewer's Guide

This documentation-focused PR fixes an incorrect README terminal-title example, specifies positional --force parsing behavior, and adds a prose regression test that keeps the duplicated title claim synchronized across the README and workspace-tools documentation.

Flow diagram for positional force flag handling

flowchart TD
    A[Parse CLI arguments] --> B{Command is global?}
    B -->|Yes| C[Accept --force in either position]
    B -->|No| D{--force follows workspace and verb?}
    D -->|Yes| E[Execute workspace verb]
    D -->|No| F[Return argument error and delete nothing]
Loading

Flow diagram for synchronized terminal-title documentation

flowchart LR
    A[README title claim] --> C[Title consistency regression test]
    B[workspace-tools title claim] --> C
    C -->|Claims agree| D[Documentation passes]
    C -->|Claims differ or missing| E[Test fails]
Loading

File-Level Changes

Change Details Files
Correct the documented terminal-title behavior to match the implementation and existing workspace-tools documentation.
  • Replace the README’s obsolete derived-id title with the human-readable workspace label.
  • Clarify that listing and hostname output retain the derived workspace id.
  • Preserve the distinction for recorded devpod ids, as covered by the existing implementation test.
README.md
Document positional parsing rules for --force across workspace verbs and global commands.
  • Explain that workspace removal requires --force after both the workspace and verb.
  • Document the distinct errors and no-op behavior for misplaced workspace-verb flags.
  • Document order-independent placement for global commands and cite the grammar tests.
docs/cli.md
Add regression coverage preventing the duplicated terminal-title documentation from drifting.
  • Extract pane-name claims from README.md and docs/workspace-tools.md.
  • Require both pages to continue stating the claim before comparing them.
  • Fail when the extracted claims differ.
test/test_title_claim_agrees.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.13%. Comparing base (741b935) to head (f0d2e4d).

Additional details and impacted files
Flag Coverage Δ
python 42.98% <ø> (ø)
rust 95.43% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
shipped code (rust) 95.43% <ø> (-0.03%) ⬇️
harness and tooling (python) 42.98% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

A fresh-context review of this branch found the `--force` paragraph asserting a
rule the grammar does not implement, and the title guard pinning something
weaker than it claimed. Both are fixed here.

**The paragraph generalised two cases into a law.** It said a misplaced `--force`
"refuses rather than being dropped". True of the workspace and verb slots; false
of everything past the verb, where only `rm` and `rme` read the flag and
`up`, `stop`, `kill`, `code`, `restart`, `recreate`, `reset` and `dotfiles` take
it and discard it in silence. cli.rs says so twice already ("the flag is the
no-op it is on `up` and `stop`"; "every other verb drops it on the floor") and
README says there is no `dl <ws> kill --force` to type, which in fact parses.
Verified against the built binary rather than inferred. The section now states
both halves, names `kill` as the one worth remembering, and adds the selector
form, which cannot be forced at all: `dl rm --force` is the verb-slot refusal and
its suggested `dl rm -- --force` would run the flag as a shell command.

It also moved. Sitting inside `### --rm: the throwaway workspace` it separated
"It stops at work that is nowhere else" from the `--rm` that sentence's "It"
refers to, and handed the reader `--force` as the antecedent: the exact thing
that goes past that check. It is its own section now, beside the other
grammar sections, and cites the 259-ordering matrix test that is its thesis.

**The guard pinned agreement, not truth.** It diffed two prose pages, so the
inverted repair, editing the docs page down to the README's wrong string, would
have passed it while reporting that `titled()` had been consulted. It had not.
`flows/launch.rs` carries the same sentence verbatim and is the one copy that
cannot be wrong without the behaviour being wrong, so the prose is now held
against it. Both drift directions are checked, including that inverted one.

Anchoring the regex on the example command fixes two more: it no longer matches
inside "every prompt *renames* the pane", which the docs and provision.rs both
say of the prompt's own later write, and set equality no longer forbids `docs/`
from documenting the examples that are titled differently, which it does.

Also: the README named one of the title's two transformations, and dropping the
suffix alone gives `devlaunch-main`, not `devlaunch@main`. Both are named now,
with the id-named case the docs page already covers. And all three copies cite
the guard, so whoever edits one learns the other two exist.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant