The word grammar's --force rule is unguarded in the same way - #506
Conversation
The placement rule was covered by hand-written example lines, and between
them they never named rm or rme -- the only two verbs that turn cli.force
into a destroyed workspace. So the matrix proved --force was placed
correctly and nothing about what a misplacement would cost.
One test now walks every ordering of every subset of
{workspace, rm, rme, --force, --rm} through resolve, and checks each
against the rule as Python stated it (`"--force" in args[2:]`) plus the
verb table, rather than against force_placement's own phrasing. The pair
refusal is held over the same space.
It found that clap's two-word cap is not what people thought: it counts
values per occurrence, so `dl a b --force c` hands the grammar three
positionals where `dl a b c` is refused. Nothing forced escapes, because
the third word is refused first, but that refusal is what keeps
force_placement's "index 2 or later is the modifier" honest, not the cap.
The comment claiming a third word never arrives is corrected and pinned.
Globals stay position-free deliberately: no command that accepts --force
also takes a word, so there is nothing for a leading --force to be read
as. That premise is now a test.
Closes #354
Reviewer's GuideAdds exhaustive, independently specified coverage for Sequence diagram for force-aware workspace resolutionsequenceDiagram
participant Test
participant Clap
participant Resolve
participant Grammar
Test->>Clap: try_parse_from(argv)
alt clap accepts argv
Clap-->>Test: Cli
Test->>Resolve: resolve(cli, argv)
Resolve->>Grammar: evaluate workspace and verb slots
alt --force follows workspace and delete verb
Grammar-->>Resolve: Command::Workspace or Command::Select Remove force=true
else --rm and --force are both present
Grammar-->>Resolve: GrammarError::RmForced
else misplaced force or unsupported verb
Grammar-->>Resolve: non-destructive command or grammar error
end
Resolve-->>Test: outcome
else positional parsing refuses argv
Clap-->>Test: None
end
Flow diagram for exhaustive force-placement validationflowchart TD
A[Generate every ordering of subsets] --> B[Parse with Cli::try_parse_from]
B --> C{clap accepts argv?}
C -- No --> D[Skip clap refusal]
C -- Yes --> E[resolve cli argv]
E --> F[Compare destructive outcome with force_is_earned]
F --> G{--rm and --force present?}
G -- Yes --> H[Expect GrammarError::RmForced]
G -- No --> I[Record force-delete lines]
H --> J[Assert complete expected delete set]
I --> J
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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.
Reviewed on two independent axes in fresh context, with the enumeration re-derived by hand, the clap claim reproduced, and two of the four mutations replayed. Local gates match the PR body: cargo test --workspace green (1,410 in core, 151 in dl), clippy -D warnings clean, fmt --check clean, CI green, CHANGELOG entry under [Unreleased].
The two things this PR asks to be believed both check out.
The enumeration is total. every_ordering over a 5-word alphabet emits 325 lines, which is exactly sum(P(5,k) for k in 1..=5) = 5 + 20 + 60 + 120 + 120, with no duplicates. 259 of them survive clap; the other 66 are the contiguous-three-positional lines clap refuses at TooManyValues, correctly skipped by resolved.
The oracle is independent. force_is_earned is a predicate over the raw &[&str] line. It calls no implementation function, and in particular it does not consult force_placement, so a change to the implementation cannot drag the expectation along with it. Three of its five clauses restate Python ("--force" in args[2:] plus the verb table); the other two (the --rm pair, and "at most two non-flag words") are Rust-era rules, both honestly annotated as such, and both separately asserted in the same loop. deletes_by_force is also complete for the right reason rather than by luck: VerbWord::with (cli.rs:154) shows Remove is the only variant that reads force at all, so widening the alphabet with up/stop/kill/reset would add lines that cannot discriminate.
Non-vacuity spot-checked. Some(1) => Trailing goes red on three tests including the new walk. The three-word arm resolving instead of refusing reproduces the PR's claim to the number: 149 pass, exactly two red, and both are the guards this PR adds.
The clap discovery is real. dl a b c is TooManyValues; dl a b --force c hands resolve words = ["a","b","c"]. Worth adding: --force=true is also TooManyValues, so there is no bypass by that spelling either, and infer_long_args is off, so no abbreviation reaches cli.force without matching the == "--force" string compare.
Standards
-
assert!(walked > 100)(cli.rs:2134) is a weak guard over a space whose size is known exactly. Measured: 259 walked. The assertion tolerates losing 158 of them. Concretely, a regression that truncatedevery_orderingat length 4 would leave 205 walked and still match the namedforcedlist at:2122, because all six forced lines are three words long. The longest orderings are the ones the walk exists for, and they are the ones that can vanish silently.assert_eq!(walked, 259)costs nothing and closes it. Commented inline. Non-blocking. -
resolved(:2028) isparse(:1104) with.ok()?instead of a panic. TheCli::try_parse_from(once("dl").chain(...))incantation is now in the module three times.parseis expressible in terms ofresolved. Duplicated Code, small. Non-blocking. -
the_pair_is_refused_wherever_force_sits_in_the_line(:1959) is now strictly subsumed — all five of its lines fall inside the walk, which asserts the sameRmForcedat:2101. The PR keeps it deliberately and says so, which is defensible; noting it so the duplication reads as a decision rather than an oversight. Non-blocking. -
Doc-comment length is house style for this file, and I found no factual error in the new ones. CHANGELOG prose is in the clear:
test/test_docs_prose.pyscopes the dash rule toREADME.mdplusdocs/*.md, and the entry carries none regardless.
Spec
-
Ask 1 is discharged in one direction and left open in the other, and the open direction is the ticket's own sentence. #354: "state the word grammar's rule as the same invariant the flag-verb grammar now uses", because the absolute index "is equivalent ... only because clap caps positionals at 2 — an incidental fact, not a stated invariant, and exactly the kind of coincidence that stops being true without anyone noticing."
The PR fully addresses the trailing direction, and improves on the ticket: the cap is not what makes the index sound, the
_ =>refusal is, and that is now pinned. Butforce_placement(:796) counts position over every argv token, so a leading token shifts the index the other way.dl <someflag> ws --force rmleavescli.words = ["ws","rm"], never reaches the_ =>arm, and reads--forceat index 2 asTrailing— a force-delete, where the same line without the flag (dl ws --force rm) is refused asUnknownVerb.No such flag exists today. I probed every candidate:
--json/--sizecarryrequires = "ls"so clap refuses them;-y/--yesis refused aboveforce_placementat:902;--stop/--autormare refused byretired_flag()at:767; the--rm/--forcepair is refused at:922;--devcontaineris stripped by hand at:803-809; every remaining flag routes toglobal_command. So it is latent, not live — but it is five independent coincidences holding up one rule, none of them stated, none of them pinned, and the new matrix cannot see any of it because--rmis its only flag and the pair is refused before placement is ever read.Either count position over the positional-looking words rather than over all of argv, or add a guard that fails when a flag clap accepts on a workspace line is neither stripped by
force_placementnor refused above it. Blocking, narrowly: this is the invariant the ticket was opened to stop relying on. -
The aid claim in the PR body is right about the outcome and wrong about the evidence. #354: "aid's leading
--rm --forceform still loses force." The PR answers "Closed on main" and cites two tests. I ran aid:aid --rm --force owner/repo "fix it"emits["--rm","--force","owner/repo","--",…]—--forceat index 1, which is dl's verb slot. Neither cited test covers that; both peel a trailing suffix, andforce_never_lands_in_dls_verb_slot_whichever_order_it_was_typed_in(rewrite.rs:1278) assertsSome(2)for the only order it tries, despite the name. The emitted line is safe, but for a third reason neither test names: dl'scli.rm && cli.forcerefusal at:922runs ahead of placement. Nothing in aid pins it. Non-blocking, but the ticket item should be closed on the real reason.Refuted from the ticket, in the PR's favour:
lib.rs:336is stale as the PR says, and both live suggestions parse (dl <ws> rm --forceis in the walk's ownforcedlist). The globals rationale is genuine at:836-853, and pinning a premise as a test rather than as prose is this repo's habit. -
The README/GRAMMAR debt the ticket names is untouched. #354: "No README/GRAMMAR update accompanied #344 either; the two files must be edited together or it becomes the drift CLAUDE.md forbids." Nothing in
README.md,docs/cli.mdor theGRAMMARhelp const says--force's position is load-bearing;rm's entry says only "add --force to delete it anyway", sodl --force ws rmansweringUnknown workspace '--force'is undocumented. Nothing user-visible moved in this PR, which is why this is non-blocking, but the drift the ticket opened on is still there.
Verdict
Request changes — one blocking finding: Spec 1, the leading-token half of force_placement's absolute index. It is a small ask (pin the coincidence, or count position over the words), and it is the specific thing #354 was opened to remove.
Everything else is non-blocking: Standards 1 (walked > 100 should be == 259), Standards 2 and 3, Spec 2 (aid's ticket item closed on evidence that does not cover the case), Spec 3 (docs drift).
The coverage this PR adds is real, total, independently oracled and demonstrably non-vacuous. Nothing here argues against the approach.
| ], | ||
| "the set of lines that force-delete moved" | ||
| ); | ||
| assert!(walked > 100, "the walk collapsed to {walked} lines"); |
There was a problem hiding this comment.
The anti-shrink guard is much looser than the space it guards.
I measured it: every_ordering emits 325 lines (exactly sum(P(5,k) for k in 1..=5), no duplicates), and 259 of them survive clap. So walked is 259 and this assertion tolerates losing 158 of them.
That window is not hypothetical in the way it looks. Truncate the walk at length 4 and you get walked == 205, which passes here and still matches the named forced list above, because all six forced lines are three words long. The 120 five-word orderings could stop being generated and nothing in this test would say so. Those are the longest orderings, which is to say the ones this walk exists for.
The count is deterministic and knowable, so it can just be pinned:
assert_eq!(walked, 259, "the walk stopped covering the argv space");The alphabet-shrink cases are already caught (dropping rm, rme or --force breaks the forced list; dropping --rm drops walked to 64), so this is only about the generator itself losing depth. Which is precisely the defect class this map exists for.
There was a problem hiding this comment.
Pinned, and your two numbers were exact:
assert_eq!(
every_ordering(&["ws", "rm", "rme", "--force", "--rm"]).len(),
325,
"the generator is no longer every ordering of every subset of five"
);
assert_eq!(walked, 259, "the walk stopped covering the argv space");Both assertions passed first run against the measured values, which is its own small confirmation that we counted the same space.
Two assertions rather than one because they fail for different reasons and a single number cannot say which. 325 is the generator: every ordering of every subset, no duplicates, depth intact. 259 is what clap accepts of it. A change that made clap stricter would move only the second, and reading a failure that says "the generator lost depth" when clap changed under you is the kind of wrong diagnosis a pinned count is supposed to prevent.
Your truncation case is the one I checked it against: cut the walk at length 4 and walked is 205, which the old floor of 100 waved through while the forced list stayed green because all six forced lines are three words long. Both assertions now fail on it, and the generator one names the cause.
There was a problem hiding this comment.
Correcting one number in my own reply before it becomes folklore: under the length-4 truncation the generator emits 205 lines and walked is 175, not 205. 205 is the generator's count, which is what the first assertion reports; 175 is what survives clap, which is what the second reports.
Does not change the conclusion, and if anything sharpens the case for keeping both assertions: the two numbers move together but are not the same number, so a single pinned count would have named whichever half it happened to be measuring.
Verified rather than reasoned, since that is what I got wrong the first time:
assertion `left == right` failed: the generator is no longer every ordering of every subset of five
left: 205
right: 325
assertion `left == right` failed: the walk stopped covering the argv space
left: 175
right: 259
| // arrive. So this arm is reached, and it is what keeps `force_placement`'s | ||
| // "index 2 or later is the modifier" honest — a `--force` at index 2 with a | ||
| // name still after it is refused here rather than honoured. Pinned by | ||
| // `a_third_word_still_arrives_when_a_flag_splits_it`. |
There was a problem hiding this comment.
This comment is right, and it closes one of the two ways the absolute index can be wrong. The other one is still open.
The trailing direction is handled: a --force at index 2 with a name after it is refused here, and the discovery that the cap is not what does that work is a real correction.
But force_placement counts position over every argv token, not over the positional words, so a token before the --force shifts the index the other way. dl <flag> ws --force rm leaves cli.words = ["ws", "rm"], so it never reaches this arm, and --force sits at stream index 2 and reads as Trailing: a force-delete of ws, where the same line without the flag (dl ws --force rm) is refused as UnknownVerb.
I probed for a live instance and there is none, which is the good news and also the problem. It is latent behind five separate facts, none stated and none pinned:
--json/--sizecarryrequires = "ls", so clap refuses them on a workspace line--yesis refused at:902, aboveforce_placement--stop/--autormare refused byretired_flag()at:767- the
--rm/--forcepair is refused at:922 --devcontaineris stripped by hand at:803-809
Drop requires = "ls" from --json for any unrelated reason and dl ws --json --force rm becomes a force-delete, silently, with the new walk still green (its only flag is --rm, and the pair is refused before placement is read).
This is #354's own sentence, still true of the surviving half: "an incidental fact, not a stated invariant, and exactly the kind of coincidence that stops being true without anyone noticing."
Two ways out, either is small:
- Count position in
force_placementover the words that could occupy a slot, skipping anything else--prefixed. Then the index is the positional index by construction rather than by coincidence, and the rule really is the same invariant the flag-verb half uses. - Keep the index and pin the coincidence: a test asserting that every flag clap accepts alongside positional words is either stripped by
force_placementor refused above it. That fails loudly the day somebody adds the sixth flag.
There was a problem hiding this comment.
Took option 1: force_placement now counts words, skipping anything --prefixed and taking --devcontainer's value with it. The index is the positional index by construction, and the two lines you named now agree.
You were right that no live instance exists and right about why that is the problem. I went looking for the sixth flag from the other end and found the vehicle already built: aid takes any leading - word it does not recognise and pushes it into the options it emits ahead of the spec, without knowing what it means (finding 3 is the same mechanism). So the coincidence was not five rules deep on dl's side alone. It was five rules deep with another binary feeding it.
Proven on the exact line from your comment. Revert to token counting and a_flag_before_the_words_does_not_move_the_slots goes red with:
left: Some(Ok(Workspace { target: "ws", verb: Remove { force: true, after: LeaveTheShell }, devcontainer: None }))
right: Some(Err(UnknownVerb { target: "ws", word: "--force" }))
That is dl <flag> ws --force rm force-deleting ws. 150 other tests pass under that same revert, so the named test and the walk are the only things holding it.
The walk covers the leading direction too, and across the whole space rather than at one line: every ordering is resolved twice, once with a synthetic flag prefixed to argv while the Cli is parsed from the line without it. That is the shape every future flag has by the time resolve sees it, consumed out of the words and still in argv, so the guard does not depend on which flag somebody adds next. I preferred it to adding a real placement-shifting token to the alphabet because there isn't one to add: any token available today is one of the five that gets refused or stripped, and would exercise the refusal rather than the placement.
Behaviour is unchanged for every line anybody can type: 152 dl tests pass with no expectation edited. The differences are confined to lines already refused above force_placement, which is --rm-carrying argv.
Review of #506 found the leading half of the same coincidence still open. force_placement counted position over every argv token, so a flag clap had consumed -- gone from cli.words, still present in argv -- shifted every slot up by one. `dl <flag> ws --force rm` then read the verb-slot --force as trailing and force-deleted ws, where `dl ws --force rm` refuses. No spelling of <flag> exists today, and that was the problem rather than the reassurance: it held only because --json/--size require --ls, --yes and the retired spellings and the --rm/--force pair are each refused above, and --devcontainer is stripped by hand. Five unrelated rules are not an invariant, and aid pushes every leading `-` word it is handed straight through to dl without knowing what any of them mean. A slot is a place a word goes, so words are what is counted. Every line anybody can type today reads exactly as it did; 152 dl tests pass unchanged. Also from the review: - The walk now resolves every line twice, once with a synthetic flag ahead of it in argv, so the leading direction is guarded across the whole space rather than by the five refusals standing in for it. - walked is pinned at 259 and the generator at 325 rather than a floor of 100, which tolerated losing 158 lines: every forced line is three words long, so truncating the walk at length 4 passed both other assertions. - aid really does emit a leading pair, and neither test cited for closing that item covered it. Both halves are pinned now: aid's emission, and dl answering the pair ahead of reading placement. - parse is resolved().expect(), and the hand-listed pair test the walk subsumes is gone.
|
All four addressed in 3. The aid item was closed on wrong evidence, and you were rightI checked the emission path rather than the tests this time. Both flags before the workspace. Neither test I cited covers that, exactly as you said: both are about the trailing run, which is the position a recalled line grows in and therefore the position everybody wrote tests for. What I had actually verified was that the pair is refused, not that it is refused for a reason anything pins. It is refused because Pinned as two halves that meet at one written-down line: This is also what settled finding 1 for me. The missing sixth flag is not hypothetical when another binary forwards arbitrary leading flags into dl's argv without inspecting them. 4. Duplication and the subsumed test
README/GRAMMAR: nothing to change, and I want to be explicit rather than silently skip it. #354 raised that against #344, which changed which lines are refused. This PR changes no line anybody can type: the rule now counts words instead of tokens, and every difference falls on argv that is refused above Gates
|
|
CI note, so the empty checks list is not read as a failure. GitHub did not create a
Locally, from Whoever merges should get the checks back from a fresh event. If they do not reappear, an empty commit on the branch is the cheap way to re-fire them, and worth doing before merge rather than after: this is a branch protection would wave through on absence rather than block, which is its own small finding. |
|
The empty commit did not re-fire it either, so the hazard is still open. This one needs a human at merge time. Tried, in order: a normal push of It is specific to this PR rather than to the repo or the workflows. While
What is left for whoever merges. The PR carries codecov and GitGuardian and nothing else. |
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
1 participant
The flag-verb half of this ticket died with #369:
verb_commandis gone, and--rm --forceis refused unconditionally ahead of every ordering. What was left is the part #344 was reviewed for and never landed with: a guard over the argv space itself.What lands
One walk of every ordering of every subset of
{ws, rm, rme, --force, --rm}, each word used at most once, resolved throughresolve(Cli, argv). Each line is checked against the rule written from Python ("--force" in args[2:],args[0]the workspace andargs[1]the verb) plus the verb table, so the expectation is a second statement of the rule rather than a paraphrase offorce_placement. The pair refusal is held over the same space, which retires the guesswork in the hand-written list beside it.rmandrmeare in the alphabet, and that is the point of the ticket. They are the only two words that can turncli.forceinto a destroyed workspace; every other verb drops the flag on the floor. Without them a matrix proves--forceis placed right and nothing at all about what a misplacement would cost.How the guard was proven non-vacuous
Four mutations, each reverted after:
Some(1) => Trailing(the verb slot honours--forceagain, the #303 bug)dl ws --force rm, which resolved toRemove { force: true }Some(0) => Trailing(the workspace slot)dl --force ws rm, samedl ws rm --force rme, which force-deletedwscli.rm && cli.forcerefusal deleteddl ws rm --force rme --rmThe third is the one worth reading: 149 other tests pass under it, and only the two guards this PR adds go red. That is the ticket's latent hole made live,
--forceat index 2 with a name still after it, honoured as the modifier.The globals test is proven the same way: add
Chosen::Installtoaccepts_forceand it fails on the assertion that the one global taking a word must not also take--force.What the ticket claimed that is no longer true
--rm --forceform. Closed on main.rm_and_force_are_handed_to_dl_whole_so_dl_can_refuse_the_pairandforce_never_lands_in_dls_verb_slot_whichever_order_it_was_typed_incover both orderings, and the second asserts the emitted index.lib.rs:336advertises a line the grammar refuses. The line number is stale after --rm is docker's --rm: retire --stop, --autorm and the suffix override (0.9.0) #369 and there is nothing wrong at it. The refusal's suggestion (dl <ws> -- --force) is a line the grammar accepts.--forcecould be mistaken for a word in. A global line has none:global_commandrefuses a target outright, and--install, the one that does take a word, is not one of the two that accept--force. Written down as a test rather than as prose, so the premise fails loudly if somebody gives a force-accepting command a word.What it found
Something believed and untrue:
num_args = 0..=2counts values per occurrence.dl a b cisTooManyValuesat exit 2, butdl a b --force copens a second occurrence and all three words reach the grammar. So the_ =>arm that carried the comment "a third word never arrives here" is reached, and it is that arm's refusal, not the cap, that stops a trailing--forcefrom being honoured with a name still after it. Comment corrected, fact pinned. No behaviour change either way, since the refusal was already there.Gates
cargo test --workspace(1,410 in the core suite plus 20 others, all green),cargo clippy --locked --all-targets -- -D warnings,cargo fmt --check. CHANGELOG entry under[Unreleased].Closes #354
🤖 Generated with Claude Code
Summary by Sourcery
Harden
--forceargument handling by basing placement on workspace words, rejecting unsafe combinations consistently, and validating the grammar across the full argument space.Bug Fixes:
--forcefrom being misinterpreted when flags precede workspace arguments, avoiding unintended forced workspace deletion.--rm --forceis refused consistently regardless of argument ordering.Enhancements:
rmandrmeverbs.Documentation:
Tests: