Close the --force flag-verb bypass - #344
Conversation
dl --rm --force <ws> silently force-deleted: verb_command folded cli.force in wherever clap found the flag, while the word grammar recovers --force's position and refuses the verb slot. Route the flag-verb grammar through the same placement check; dl rm --force <ws> and dl --rm --force <ws> now earn the same refusal, and the trailing slot keeps meaning force. Refs #303
…lete dl --force <ws> --rm read the flag as force and deleted; the word grammar reads slot 0 as a workspace *name*. The flag-verb grammar now gives the same reading: the line targets a workspace called --force, earns the unknown-workspace refusal at runtime, and force-deletes nothing. Refs #303
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideAligns flag-verb grammar with word-verb grammar for --force placement so misparsed flag-based delete commands are refused or treated as workspace names, preventing unintended forced deletes. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Two-axis review at fixed point a1c6fe8...12a5d8f (git diff a1c6fe8f38b51fdd75f79550a9ffbeb877ab911e...HEAD). Preflight: ref resolves, diff non-empty (1 file, +87/-3), all 14 checks green. Spec is #303. Axes were run in isolated contexts and are not merged or reranked; the third section is the review stage's own probe of the safety property, and is reported separately for the same reason.
Standards
Passes. No blocking findings. The change is in the house style — why-comments, a total function over Cli, sum-typed placement, sentence-named tests.
- non-blocking — Doc contradicts the new code.
verb_command's doc-comment states the contract "The displaced words are named rather than dropped in silence — see [Overridden] — with one exception" (rust/dl/src/cli.rs:773-776). The newWorkspaceSlotarm returns.alone()(:816-821), sodl --force ws --rmsilently dropsws— confirmed live, the only output isUnknown workspace '--force', with no mention thatwswas ignored. That is a second exception, and the doc-comment is untouched by the diff. In a file where the doc-comments are the spec, this is the highest-value fix here. - non-blocking — Duplicated
VerbSlotarm. Theforce_placementmatch now appears in two places (:809-838vs:913-947). Half-refuted: hoisting intoresolvewould break globals (dl --purge --forcemust keepModifierNotAllowed,:703-708), and the twoWorkspaceSlotarms genuinely differ (Verb::Attach { autorm }vsword.with(false)). But theVerbSlotarm is byte-identical in both (:823-835=:929-941) and wants extracting — it is the piece a third caller would copy again. - nit — Test does not pin the displacement. The module has an
overridden()helper used elsewhere for exactly this question; the new tests (:1869-1885) assertparse()only, so the silent drop above is unasserted either way. Neighbouring tests carry failure messages (:1794); these two carry none. - nit — the bare
"--force"literal count is doubled by this diff (:817,:833, alongside:920,:940); a namedconstwould do. A payload onForcePlace::WorkspaceSlotwould not earn itself — the variant is only ever produced by matching the literal at:668. - refuted — the
UnknownVerbsuggestion wart is not new: at merge-basedl rm --force wsalready renders "Use 'dl rm -- --force'". This PR extends an existing wart consistently rather than creating one.
Spec
Passes. All three acceptance criteria hold, and no documented example stops resolving.
Acceptance criteria, traced through resolve → verb_command → force_placement:
dl --rm --force <ws>— stream["--rm","--force","ws"], index 1 →VerbSlot→UnknownVerb(:831). Refuses, deletes nothing. ✅dl --force <ws> --rm— index 0 →WorkspaceSlot→Workspace{target:"--force", verb: word.with(false)}(:816-821). Ticket: "a--forcein the workspace slot became an unknown workspace". Force dropped, runtime lookup fails, nothing deleted. ✅dl <ws> --rm --forceanddl --rm <ws> --force— index 2 →Trailing→verb = word.with(cli.force). Still forced remove. ✅
Documented examples, all still resolving: GRAMMAR:538 dl owner/repo 'review this pr' --rm --force; GRAMMAR:539 dl stop owner/repo --rm; README:153/:269 "a dl prune <ws> --force line recalled with --rm appended still removes <ws>"; GRAMMAR:530 "A verb with no workspace named picks interactively"; README:188/:1271 dl <ws> rm --force; and aid's main.rs:229 aid … fix the bug --rm --force, whose build_dl_args output is pinned at rust/aid/src/rewrite.rs:653 and lands Trailing.
Findings:
- non-blocking — aid's leading verb-flag form loses force.
rust/aid/src/rewrite.rs:343documents "A verb flag can also arrive before the spec (aid --rm owner/repo)".aid --rm --force owner/repoemits["--force","owner/repo","--rm"](--forceis not inSUFFIX_VERBS, so it lands indl_options) →WorkspaceSlot→ unknown workspace--force. Previously force-removed. Defensible —rewrite.rs:691-692already anticipates it — but an unannounced change on a documented shape. - non-blocking — The refusal advertises a line the same grammar refuses.
rust/dl/src/lib.rs:336rendersUnknownVerbas "Use 'dl {target} -- {word}'", sodl --rm --force wsprintsUse 'dl --rm -- --force' to run a shell command.— whichverb_commandthen rejects withA shell command can only be run by 'dl <workspace> -- <command>', not with 'rm'.(cli.rs:787, verified live). The ticket asks it to "refuse the waydl rm --force <ws>already does"; it does, but with advice naming a flag as the workspace. - non-blocking — Selector + force is no longer spellable.
GRAMMAR:520"add --force to delete it anyway" composed withGRAMMAR:530-531"A verb with no workspace named picks interactively … dl rm can clear five workspaces in one visit".dl --rm --forcewas the last working spelling (dl rm --forcealready refused on main) and now refuses. See B2 below. - non-blocking scope note —
--stopis folded in too (:625) thoughVerbWord::withignores force for Stop, sodl --stop --force wsgoes from harmless to refused. Within the letter of "Routeverb_commandthrough the same placement check". - nit — no README/GRAMMAR change (CLAUDE.md: "update the README.md to reflect the current tool behavior"). No doc line is falsified, but
GRAMMAR:520still states--forceunconditionally while three of four positions now refuse it.
Safety probe (review stage's own, neither axis)
The ticket is a guard on a destructive operation, so the stage probed the whole placement space rather than the cases the tests name. Method: a temporary matrix test over resolve in a throwaway worktree (reverted, nothing pushed), then the same lines through a real debug/dl under a scratch XDG_CACHE_HOME.
B1 — blocking. A --force that is not trailing still force-deletes on a flag-verb line.
dl --rm stop --force ws => Workspace { target: "ws", verb: Remove { force: true } }
dl --rm code --force ws => Workspace { target: "ws", verb: Remove { force: true } }
dl --rm prune --force ws => Workspace { target: "ws", verb: Remove { force: true } } (no override notice at all)
In each, --force sits ahead of the workspace name it deletes — exactly the placement this ticket set out to stop honouring. Cause: force_placement classifies index ≥ 2 as Trailing (:674), and on a flag-verb line the verb flag itself consumes a stream slot, so index 2 can still have positional words after it. The word grammar cannot reach this — clap caps positionals at num_args = 0..=2 (:419), so its stream is at most [w1, w2, --force] and index ≥ 2 really is the end of the line (verified: dl a b --force c is refused by clap's cap).
This is the same off-by-one seen from the other side in the passing cases: on a flag-verb line index 0 is the verb flag and index 1 is the workspace, yet the check reads them as workspace and verb. dl --rm --force ws refuses with target: "--rm" — the flag named as the workspace — which the new test hard-codes (:1875-1877). The two grammars agree on the verdict for the 3-token cases the tests name; the slot model underneath is shifted by one, and that shift is what leaves B1 open.
Not a regression (these shapes force-deleted at merge-base too), but the invariant the diff's own comment asserts — "--force means force only in the trailing slot, the one place Python read it" (:804-805) — is not what the code delivers, and a forced delete is still reachable from a non-trailing --force. Suggested fix: on a flag-verb line, count the slot stream with the verb flag excluded (or require --force to be genuinely last), and add the 4-token shapes above as red tests.
B2 — blocking. dl --rm --force regressed, unguarded and undecided.
At merge-base it resolved to Select { verb: Remove { force: true } } — pick interactively, force-remove. It now refuses (UnknownVerb { target: "--rm", word: "--force" }). No test in the diff covers the bare two-token form, and no breadcrumb on #303 records the trade. The grammar is left incoherent about it: the natural spelling refuses while the retired word still works —
dl --rm --force => REFUSED UnknownVerb { target: "--rm", word: "--force" }
dl --rm prune --force => Select { verb: Remove { force: true } }
Either outcome is arguable, but it should be a decision with a test on it, not a side effect. Note this is the same loss Spec #3 names from the doc side.
Everything else in the space behaves: --devcontainer stripping is consistent in both =-form and separated form; --yes and --autorm are refused ahead of the placement check so they cannot shift the index; --json/--size require --ls and the what group forbids a second verb flag, so no other token can enter the stream; a doubled --force is refused by clap; and --force after -- stays with the command.
Verdict
Request changes (posted as a comment — GitHub refuses a change request on a PR authored by the same account).
Blocking:
- B1 — a non-trailing
--forcestill force-deletes on flag-verb lines of the formdl --rm <word> --force <ws>; the guard's stated "trailing slot only" invariant is not what the code implements. - B2 —
dl --rm --force(selector + force) regressed with no test and no recorded decision, while the retired-word spellingdl --rm prune --forcestill works.
Non-blocking, worth folding in: the verb_command doc-comment now contradicts its own WorkspaceSlot arm (silent word drop), the duplicated VerbSlot arm, aid's leading --rm --force form, and the refusal that advertises a line the grammar refuses.
Both axes passed on their own terms; the blocking items come from the stage's safety probe of the placement space beyond the cases the tests name.
…luded The placement guard was counting the verb flag as a name slot, and a flag verb is not a name — it is the verb. That shifted every slot by one, so on a flag-verb line slot 2 was still mid-line rather than the end of it, and a `--force` sitting *ahead of the workspace it deleted* read as the trailing modifier and force-deleted: `dl --rm stop --force ws`, `dl ws --rm --force other`. The word spelling of the same shape, `dl stop --force ws`, has always refused it. The rule is now asked of the stream the flag is not in, and stated as "no name follows it" rather than "slot 2" — asking for slot 2 exactly would be the same off-by-one from the other end, since the flag supplies the verb and so the longest line left is a name, a name and a `--force`. Proved by a matrix over the argv space rather than more named cases, since named cases are what missed these: every ordering of up to four slot-carrying tokens must satisfy "a forced delete implies nothing named follows --force". It found `dl ws --rm --force other` on its own, which no report had named. The destructive end is pinned at the binary boundary too, against the scenario world's scratch cache, where the failing run really had removed the clone. Also folded in from the review: the duplicated verb-slot refusal is one function, `--force` is spelled once as a const, and `verb_command`'s doc-comment names the second exception to its "displaced words are named" contract instead of contradicting it. Refs #303 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard refuses `--force` in a name slot because it is ambiguous there — either the flag, or the workspace somebody meant to delete. A selector line carries no name at all, so that ambiguity cannot arise, and a refusal there guards nothing while costing a working invocation. The previous pass refused it with no test on the trade either way; this pins it, in both token orders and in the retired spelling `dl --rm prune --force` that says the same thing and has always resolved this way, so that a future tightening of the rule cannot take it back by accident. Also pinned at the binary boundary, as far as a headless run can go: the line has to reach the picker rather than the refusal. What the picker does with no terminal to draw on is its own business and is not what the test asserts. Refs #303 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Back from changes requested, both blockers reproduced red before being fixed. Two commits on top of B1 — a non-trailing
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Re-review of the two blocking findings at df385b1 (fast-forward over the reviewed 12a5d8f, 40ce0fd + df385b1, +297/-56 across cli.rs and tests/grammar.rs). I raised them, so this is the call on whether they are closed. Both resolved. I did not re-run the whole review; below is only my own findings plus what the new code newly introduces.
B1 — resolved, and by the right rule
flag_verb_force_placement (cli.rs:689-718) filters the verb flag out of the slot stream before asking the question, and reads --force as the modifier iff nothing follows it. Every shape from my report now refuses, with the word spelling's own sentence and the right word named:
dl --rm stop --force ws => Unknown command '--force'. Use 'dl stop -- --force' …
dl --rm code --force ws => Unknown command '--force'. Use 'dl code -- --force' …
dl --rm prune --force ws => 'prune' is no longer a workspace verb. …
dl --rm ws1 --force ws2 => Unknown command '--force'. Use 'dl ws1 -- --force' …
On whether "no name follows it" is the invariant at both ends — it is, and it is equivalent by construction rather than by luck. On any line reaching flag_verb_force_placement, the filtered stream is exactly the positional words plus --force: --yes (:860) and --autorm (:855) are refused ahead of it, --devcontainer and its value are stripped by slot_stream, the second verb flag is a clap what-group conflict, --json/--size need --ls (also in that group), a repeated flag is an ArgumentConflict, and everything past -- is outside the stream. So "--force is last in the stream" ⟺ "no positional word follows --force in argv" — the invariant I asked for. At the near end the builder's reasoning also holds: --force alone leaves cli.words empty, so the line resolves to Select and there is no name it could have been mistaken for. Asking for "slot 2 exactly" would indeed have been the same off-by-one mirrored, and would have cost dl --rm --force for nothing.
B2 — resolved
dl --rm --force, dl --force --rm and dl --rm prune --force all resolve to Select { verb: Remove { force: true } } (:1990-2016) — merge-base behaviour restored, the grammar no longer disagrees with itself about the retired spelling, and the trade is now a recorded decision with a test on it rather than a side effect.
The matrix claim, checked
The reported first red is real. At 12a5d8f, dl ws --rm --force other → Workspace { target: "ws", verb: Remove { force: true } } — a forced delete of ws with other still after --force. Nobody had named it; my report's shapes were dl --rm <word> --force <ws>, with the flag leading rather than sitting between the names. Same root cause, a shape I missed.
The property is the right one. "If the line resolves to a forced Remove, then among the non-verb-flag tokens --force is last" states the safety rule with no reference to slot numbers — which is precisely what the off-by-one defeated. It quantifies over the outcome reached rather than over expected errors, so it cannot pass by refusing everything.
Coverage does span the reachable space, and I pushed on it. longest = 4 is exactly the maximum count of slot-carrying tokens a flag-verb line can hold (2 positionals + verb flag + --force). I re-ran the same property in a throwaway worktree over a wider alphabet — rm, up, code added — and to five tokens: 5370 resolving lines, 620 of them forced deletes, no counterexample. Non-vacuous. Sensitivity checked by porting the identical probe onto 12a5d8f, where it fails on dl ws other --rm --force rm. I also checked the nine placements of a --devcontainer pair around a flag-verb line (=-form and separated, before/inside/after): slot stripping is correct in all of them. Branch's own suite green locally, all suites, zero failures.
Still outstanding — all non-blocking, none should hold this PR
- The word grammar is still on the absolute-index phrasing.
workspace_commandkeepsforce_placement, where index ≥ 2 ⇒Trailing(:684). That is equivalent to "no name follows" today only because clap caps positionals atnum_args = 0..=2(:419), making index 2 necessarily last. It is not equivalent by construction: widen that cap, or let one more unstripped flag into the stream, and the word half reproduces exactly the bug just fixed. And the new matrix would not catch it — its alphabet omitsrm, the only token that makes the word grammar produce a forced delete, so the property never exercises the half of the code this PR did not change. Addingrmto the alphabet costs one word and closes that (I verified it still passes today). Latent fragility, not a live defect. aid --rm --force owner/repostill loses its force (rewrite.rs:343documents the leading form;--forceis not inSUFFIX_VERBS, so it lands indl_optionsat slot 0). Unchanged from Spec #1.lib.rs:336still advertisesdl stop -- --force, whichverb_commandthen refuses (:846). Much improved — it now names a real word instead of the flag--rm— but still advice that does not work.- No README/GRAMMAR change;
GRAMMAR:520still states--forceunconditionally. - Globals have no placement rule at all:
dl --force --prune→Prune { yes: false, force: true }. Outside this ticket (clone directories, not workspaces, and it prints its plan and asks without-y) — noted so it is not mistaken for covered.
Also newly introduced and fine: force_in_the_verb_slot (:721-732) de-duplicates the VerbSlot arm and the FORCE const removes the doubled literal — both of my Standards notes; and the verb_command doc-comment now names its second exception (:833-839), closing the contradiction I flagged. dl --rm --force ws reads as workspace --force where the word spelling dl rm --force ws says UnknownVerb{target:"rm"} — a diagnostic-only divergence inherent to the flag holding no name slot; both refuse and neither deletes.
The tests/grammar.rs addition is the right kind of test: force_ahead_of_the_workspace_on_a_flag_verb_line_deletes_nothing asserts exit 1, the exact stderr, and world.clone_is_there() — it guards the destructive outcome end to end, not just the parse.
Verdict
Approve (posted as a comment — GitHub refuses an approval on a PR authored by the same account). B1 and B2 both resolved; the five items above are follow-up material, not blockers.
1 participant
dl --force <ws> --rmforce-deleted.verb_commandfoldedcli.forcein wherever clap happened to find the flag (cli.rs:756), whileworkspace_commandconsultsforce_placement(cli.rs:847-882) and refuses the same misparse for the word verbs. The guard's own rationale — "clap-parsed Rust silently accepted the flag and deleted" — applies identically to both spellings.Two slices, each red first:
--forcein the verb slot is refused, not obeyed. The flag-verb grammar now runs the same placement check as the word grammar, sodl rm --force <ws>anddl --rm --force <ws>earn the same refusal. The trailing slot keeps meaning force.--forceis a name, not a forced delete.dl --force <ws> --rmnow reads slot 0 the way the word grammar does — as a workspace name — so the line targets a workspace called--force, earns the unknown-workspace refusal at runtime, and force-deletes nothing.dl <ws> --rm --forcekeeps working throughout.Verified locally, run alone:
cargo test --workspace(1,578 pass, 20 suites),cargo clippy --locked --all-targets -- -D warnings,cargo fmt --checkall clean.Closes #303.
🤖 Generated with Claude Code
Summary by Sourcery
Align flag-based command parsing with positional force-placement rules to prevent ambiguous arguments from triggering forced workspace deletion.
Bug Fixes:
--forcefrom being honored when it appears in workspace or verb positions for flag-based commands, eliminating unintended forced deletions.--forceas a workspace name and preserve the corresponding unknown-workspace behavior.--forceusage working as intended.Enhancements:
Tests:
--forceplacements and safe forced-delete behavior.