Repository navigation
Conversation
Divergence row 32. `dl <ws> --rm` and `dl <ws> --rm -- <cmd>` now hand over a session and delete the workspace once it ends -- what `--autorm` did, under the name docker gives it. The `rm` verb is unchanged and is the only way to delete one now, so the grammar is `docker rm` beside `docker run --rm` and neither spelling has to be read twice. Three withdrawals pay for it, each recognised and refused at exit 1 rather than reinterpreted or dropped to clap's exit 2: - `--autorm`, which is a rename and says so. - `--stop`, whose only reason to exist was being a flag (rows 15 and 30). It cannot stay a *cancelling* suffix beside a `--rm` that runs the line: the two look like a pair and would act oppositely. - row 30's suffix override itself, so `aid <ws> <prompt> --rm` runs the prompt and deletes afterwards, and `dl prune <ws> --force --rm` is row 31's refusal. That takes `Overridden`, `pick_target`, `verb_command` and the override notice with it. `--force` still does not compose with `--rm` -- docker keeps `-f` on `rm` too -- and `dl <ws> rm --rm` is refused as the two requests it is. Retired flags are answered before anything else a line got wrong, since both moved on account of `--rm`'s new meaning. aid peels the retired spellings so dl refuses them by name instead of joining them into a prompt, and builds no agent command for such a line, so nothing is booted on the way to exit 1.
Deleting `overridden_notice` left its `// The suffix verbs' notice` banner standing over nothing, immediately above the next section's banner.
`Suffix { options, retired }` let `retired: true` sit beside options holding no
retired spelling, and the reverse -- a state nothing constructs but the type
allowed. Derived at the one place that asks instead, which takes the struct with
it.
… is RmForced The pair is checked before the words are read, so that line answers `--force does not apply to --rm` and not the prune retirement. Without --force it is the retirement. Both readings now pinned by a test, since the claim was prose nothing enforced.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @blooop, your pull request is larger than the review limit of 150000 diff characters
Reviewer's GuideAligns devlaunch’s Sequence diagram for --rm session cleanupsequenceDiagram
participant User
participant dl
participant Workspace
participant Session
User->>dl: invoke --rm
dl->>Workspace: launch or attach
dl->>Session: hand over shell or command
Session-->>dl: session ends
dl->>Workspace: rm_on_exit_removing
dl->>Workspace: remove workspace and clone
Sequence diagram for aid handling --rm and retired flagssequenceDiagram
participant User
participant aid
participant dl
participant Agent
User->>aid: prompt with --rm
aid->>dl: prompt and --rm
dl->>Agent: run agent session
Agent-->>dl: session ends
dl->>dl: remove workspace
User->>aid: prompt with --autorm or --stop
aid->>dl: retired spelling without agent command
dl-->>User: refusal at exit 1
Flow diagram for the new dl workspace removal grammarflowchart LR
Input["dl workspace arguments"] --> Decision{"Request type"}
Decision -->|"rm verb"| Now["Delete workspace now"]
Decision -->|"--rm on attach or command"| Session["Run session or command"]
Session --> Exit["Session ends"]
Exit --> Later["Delete workspace"]
Decision -->|"--force with rm"| Forced["Delete now despite unsaved work"]
Decision -->|"--force with --rm"| Refuse["Refuse at exit 1"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
main released 0.8.0 while this branch was being written, so this release is 0.9.0 -- still the 0.x minor bump that signals a breaking CLI change. Two conflicts, both taken as this branch's semantics plus main's feature: - `build_dl_args` gained main's `herdr: Option<&herdr::Session>` parameter, and every call site this branch wrote now passes it. `build_agent_command` had already merged cleanly with the same parameter. - CHANGELOG keeps both entries under their own versions. The tests main deleted with the suffix override -- `a_verb_flag_before_the_spec_is _the_same_request` and the `Task::Verb` arms -- stay deleted; main only reformatted them for the new argument.
blooop
changed the title
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.
Retrospective review of a merged PR. #369 landed with no review (Sourcery was rate-limited, no wf-review ran), so this is a wf-review run after the fact. Every finding below was re-checked against current origin/main (a5f7ed8) and is reported only if it still reproduces there; anything a later PR fixed was dropped.
- Fixed point:
git diff 8c70516...ce62130(base = the merge commit's first parent, head =ce62130). - Spec: no ticket —
closingIssuesReferencesis empty and the branch isautorm1. The written spec is divergence row 32 ofdocs/rust-rewrite-plan.md, which the repo's own rule makes normative ("Additions require a PR that updates this table"), restated by the PR body,CHANGELOG[0.9.0], and README's--rmsections.
Standards
Repo-documented standards first (CLAUDE.md, AGENTS.md, docs/rust-rewrite-plan.md, README), then the Fowler baseline. Clippy/fmt-enforced items skipped.
S1 — non-blocking — rust/aid/src/rewrite.rs:236 (build_boot_args). The function's docstring states an invariant the function does not hold:
"This is what the interactive flow runs in the background while the prompt is being typed, so it deliberately carries no suffix options:
--rmbesideupis a pair dl refuses by name…"
It filters spec_options but copies dl_options verbatim (rewrite.rs:237), and parse_aid_args puts a leading --rm into dl_options (rewrite.rs:367-370) — the spelling rm_before_the_spec_is_the_same_request (rewrite.rs:817) pins as "the same request". Reproduced end-to-end on main under a pty:
$ aid --rm owner/repo
Booting owner/repo in the background. Type the prompt for claude and press Enter to launch…
> fix the bug
aid -> dl --rm owner/repo -- 'IS_SANDBOX=1 claude --dangerously-skip-permissions '\''fix the bug'\'''
--rm deletes the workspace when a session ends, so it applies to 'dl <workspace>' and
'dl <workspace> -- <command>', not to 'up'. Use 'dl <workspace> rm' to delete a workspace now.
aid: the background boot exited 1; launching in the foreground...
The banner claims a boot that never happened, the whole prewarm-while-you-type overlap is silently lost, and the user gets a refusal line for a line that is entirely valid. The trailing spelling (aid owner/repo --rm) boots correctly — verified side by side. Two rules collide here and the collision is unguarded: an_unknown_leading_flag_boots_with_the_same_flag (rewrite.rs:948) says a leading flag goes to both the boot and the attach, and build_boot_args's docstring says --rm must reach neither up. Fix: classify a leading --rm/--stop/--autorm/--force at rewrite.rs:367-370 into spec_options / Task::Retired the way the trailing peel does, so leading and trailing really are the same request; rm_before_the_spec_is_the_same_request then needs its built[0] assertion updated to the (correct) behind-the-spec order.
S2 — non-blocking — rust/aid/src/rewrite.rs:928 (the_boot_line_is_options_then_spec_then_up_and_nothing_else). The one test guarding the boot line drives it with --autorm, which now yields Task::Retired; build_boot_args is only ever reached from interactive::collect_prompt, which requires Task::Agent (interactive.rs:163-171). So the test can never exercise the path it is named for, and its comment ("--autorm beside up is a pair dl refuses") names a spelling dl now refuses as retired, not as an up/--rm pair. Fix: use --rm --force in the argv and reword the comment. (Making that change is also what would have caught S1.)
S3 — non-blocking — rust/aid/src/rewrite.rs:281 (peel_suffix) and rewrite.rs:880. The "options come out ahead of modifiers, and that is not cosmetic" rationale is now false. resolve answers retired flags first (cli.rs:616) and workspace_command returns RmForced before force_placement (cli.rs:771), so every peelable run — the only words that can be in one are --rm, --stop, --autorm, --force — gets the same dl answer in either order. Verified: dl ws --force --rm and dl ws --rm --force both print the --force does not apply to --rm sentence. The comment also still names --autorm as the worked example. Fix: keep the reorder if a stable emitted line is wanted, but say that is the reason; or drop it with its test.
S4 — non-blocking — rust/dl/src/commands.rs:1065. render_select's doc still offers dl --stop as the live example of the selector honouring its verb; that line is now an exit-1 retirement refusal. The PR swept the same doc block 35 lines lower. Fix: dl stop.
S5 — non-blocking — rust/dl/src/cli.rs:1670-1672. a_workspace_called_force_still_reads_that_way_without_autorm, and its comment "for every line that does not also say --autorm", name the retired spelling; the guard is about --rm. Fix: rename to …_without_rm and update the comment.
S6 — non-blocking — rust/aid/src/rewrite.rs:172. [Suffix::options] is a dangling intra-doc link: the Suffix struct was deleted by this PR's own follow-up commit (38eae67) and the reference was not updated. cargo doc -p aid --no-deps warns unresolved link to 'Suffix::options' — no item named 'Suffix' in scope. Same class at rewrite.rs:177 ("--autorm would survive either side") and rewrite.rs:294 ("Divergence row 30, aid's half" — row 30 is the row this PR superseded). Fix: point at peel_suffix/SUFFIX_OPTIONS and row 32.
S7 — non-blocking — docs/rust-rewrite-plan.md:194 (row 30). The row's own rule is that a row cites the tests that pin it. Row 30 still says "Pinned by the suffix_verb tests in dl/src/cli.rs and aid/src/rewrite.rs" — this PR deleted every one of them (three existed at 8c70516, none exist now), so a superseded row asserts a pin that no longer exists. Fix: amend row 30's pinning clause to say the tests went with the behaviour, and point at row 32's list.
Nothing else: README, --help, dl.bash and row 32's cited tests are in step with the grammar, and the Fowler baseline turned up no smell this diff introduced — the RmOnExit / Verb::Attach{rm} / Family::Launch{verb, rm} shape is a genuine de-primitivisation of the old Overridden/Suffix{options, retired} pair.
Spec
Row 32 read as acceptance criteria, each checked against the built binary on main.
The dl grammar honours row 32 in full. Every spelling and position of --autorm, --stop, prune, rm, --force and --rm refuses at exit 1 with the right sentence; retired flags are answered first (dl --ls --stop, dl --force --autorm, dl --install x --stop, dl -y --autorm all name the retirement); dl prune <ws> --rm is row 31's refusal and dl prune <ws> --force --rm is the pair; dl <ws> rm --rm is refused; Overridden, pick_target and the override notice are gone. Beyond the hand-named cases I ran #344's argv-space property as a temporary matrix over {ws, other, rm, stop, prune, --force, --rm, --autorm, --stop, --devcontainer, v, --, -y, --prune} to length 4 (and {ws, rm, --force, --rm, --devcontainer, v} to length 5, 2 122 lines): no escapes for "a forced delete implies nothing named follows --force", "--rm and --force together always yield RmForced", "a retired spelling always refuses", and "--rm never produces an immediate delete and rm never defers". The matrix bites — disabling force_placement produces 66 escapes, and removing the cli.rm && cli.force guard produces 284 — so those are real exercises, not vacuous passes. The --rm-and-signals claim is honoured too and is pinned end to end by a_ctrl_c_that_reaches_dl_mid_session_leaves_a_throwaway_workspace_standing (dl/tests/interrupt.rs:424); README's "How you exit decides whether it fires" agrees, and edd59e3 corrected the last of that prose after the fact.
Every finding is in aid, and all three share one root cause: a retired or session spelling is recognised only by the trailing peel, so any other position falls through to Task::Agent.
P1 — non-blocking — rust/aid/src/rewrite.rs:367-370 + :381. Row 32:
"aid peels the retired spellings so dl refuses them by name instead of joining them into a prompt, and builds no agent command for such a line, so nothing is booted on the way to exit 1."
A leading retired spelling is classified as an ordinary dl option, so Task::Agent is chosen and a full agent command is built. On main, under a pty:
$ aid --autorm owner/repo
Booting owner/repo in the background. Type the prompt for claude and press Enter to launch…
> fix the bug
aid -> dl --autorm owner/repo -- 'IS_SANDBOX=1 claude --dangerously-skip-permissions '\''fix the bug'\'''
--autorm is now spelled --rm: … Use 'dl <workspace> rm' to delete one now.
aid: the background boot exited 1; launching in the foreground...
--autorm is now spelled --rm: … Use 'dl <workspace> rm' to delete one now.
A boot child is spawned, the "Booting …" banner is printed, the user is made to type a prompt that will never be used, and the refusal is then printed twice. The trailing spelling gets the intended one-line refusal with no ceremony. The final answer is right — dl refuses by name at exit 1 — but the three things the sentence promises about the way there are all false. Row 32's own named pin, a_retired_spelling_starts_no_agent_and_is_handed_to_dl_to_refuse (rewrite.rs:600), covers only the trailing position. Fix: as S1 — classify at the leading branch too, and extend that test to the leading position.
P2 — non-blocking — rust/aid/src/rewrite.rs:381. Same sentence, "instead of joining them into a prompt". A retired spelling anywhere that is not the trailing run is joined into the prompt and the launch proceeds:
$ aid owner/repo --autorm fix the bug
aid -> dl owner/repo -- 'IS_SANDBOX=1 claude --dangerously-skip-permissions '\''--autorm fix the bug'\'''
Cloning repository git@github.com:owner/repo.git …
No refusal; the workspace is cloned and the agent would be handed --autorm fix the bug as its prompt. This one is arguably by design — peel_suffix's docstring bounds the exception to "only at the very end of the line", and a_prompt_that_merely_mentions_a_suffix_flag_is_still_a_prompt deliberately pins that aid <ws> explain --rm please stays prompt text. But the bound that is right for a live flag is not obviously right for a retired one: --rm inside a prompt can be prose, --autorm cannot be anything but the flag. Either widen the retired-spelling scan to the whole argv, or narrow row 32's sentence to say the peel is trailing-only.
P3 — non-blocking — rust/dl/src/commands.rs:1065. Independently flagged by both axes: the doc comment still advertises dl --stop as a working selector line, which row 32 ("--stop" … retired) made an exit-1 refusal.
Minor, not worth a fix: a clap-level error preempts the retirement (dl --autorm a b c, dl --refresh --ls --autorm → exit 2), so row 32's "answered before anything else the line got wrong" holds for dl's own diagnostics but not for clap's.
Issue #303
#303 (Close the --force flag-verb bypass) is dead on main and can be closed. verb_command — the function the issue names at cli.rs:756 — was deleted entirely by this PR, and --rm is no longer a delete verb, so neither of the issue's two repros can force anything: dl --force <ws> --rm and dl --rm --force <ws> both exit 1 with --force does not apply to --rm and delete nothing. So does dl <ws> --rm --force, the line #344 wanted to keep force-deleting, and so does dl ws --rm --force other, the case #344's matrix found. #354's sub-item "aid's leading --rm --force form still loses force" is also dead — all three orderings now reach RmForced.
What #369 did not bring across is the guard: the property lives only as five hand-named orderings at cli.rs:1645-1662, and #344's exhaustive matrix never landed. The property does hold today — I verified it over the argv space as described above — but nothing in the repo would notice if it stopped. That residue is exactly #354, which is open and should stay open; closing #303 should reference it.
Verdict
Comment. No blocking findings. The safety properties this PR exists to establish all hold on current main and are exercised, not merely asserted: --rm never deletes immediately, rm never defers, --force never composes with --rm in any argv ordering I could construct, every retired spelling refuses at exit 1 by name from every position dl can see it, and the removal is genuinely absent from the signal path.
The one real defect is in aid, and it is a single root cause with a single fix: retired and session spellings are recognised only by the trailing peel, so a leading one is passed through as an opaque dl option. That breaks the interactive prewarm for aid --rm <ws> (S1) and makes aid --autorm <ws> boot, banner and prompt on its way to a refusal row 32 says it reaches with nothing booted (P1). Worth one ticket covering S1 + P1 + S2's test, with P2 settled as either a widening or a wording change.
The remaining items are documentation and test-name drift left by the rename (S3-S7, P3) — cheap, and each one is the kind of second copy of a fact that CLAUDE.md's own doc rule exists to prevent.
1 participant
Divergence row 32.
--rmbecomes docker's--rm— the workspace goes when thesession ends — and the
rmverb stays the only way to delete one now. The wholegrammar is then
docker rmbesidedocker run --rm, and neither spelling has to beread twice to work out which was meant.
What is withdrawn
Three things, each recognised and refused at exit 1 with the spelling that
replaced it — never dropped to clap's
unexpected argumentat exit 2, which namesthe spelling and not the replacement:
--autormis a rename and says so. The behaviour above is what it always did.--stop, whose only reason to exist was being a flag (rows 15 and 30). Itcannot stay a cancelling suffix beside a
--rmthat runs the line: the two looklike a pair and would act oppositely.
aid <ws> 'review this pr' --rmnow runs the reviewand deletes afterwards, where it used to delete instead and print
--rm overrode the rest of the line.dl prune <ws> --rmis row 31's refusal.For "I am done with this workspace":
dl <ws> rmnames it,dl rmpicks it and TABmarks several — which for a long
aidprompt line is fewer keystrokes than recallingit to append to. What is genuinely gone is deleting a workspace without naming or
picking it, by appending to whatever the last line happened to be.
Notes
--forcestill does not compose with--rm; that isdl <ws> rm --force, which iswhere docker keeps its
-ftoo.dl <ws> rm --rmis refused as the two requests it is.on account of
--rm's new meaning.aidpeels the retired spellings so dl refuses them by name rather than joining theminto a prompt, and builds no agent command for such a line — nothing is booted on the
way to exit 1.
Overridden,pick_target,verb_commandand the override notice are gone with theoverride: 857 lines deleted against 737 added.
Version bumped to 0.8.0 — breaking CLI change on a 0.x line.
🤖 Generated with Claude Code
Summary by Sourcery
Adopt Docker-style
--rmsession cleanup while retiring the legacy automatic-removal and suffix-command flag behaviors.New Features:
--rmdelete a workspace after an attached shell, command, or agent session ends, matching Docker’srun --rmsemantics.Bug Fixes:
Enhancements:
aidprompts continue running with trailing--rm, while immediate deletion remains available through thermverb.--stopand rename--autormwith replacement guidance, refuse conflicting--rmcombinations, and preserve safe cleanup behavior and session exit statuses.Build:
Documentation:
rmand session-end deletion with--rm, including retired spellings and migration guidance.Tests: