Repository navigation
Guard dl.bash's tables against the grammar, and complete the five missing flags - #351
Conversation
The completion script offered six flags where the argument grammar declares thirteen user-facing ones, so --prune, --purge, --reconcile, --refresh, --rm, --stop and --autorm had been uncompletable for releases with nothing failing. The new test derives the expected list from cli.rs's Cli struct instead of restating it: hidden flags drop out by rule, and the four that only modify a line already naming a command are a hand-written table with a reason each, so a flag added to the grammar breaks the test until somebody decides which side it belongs on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three assertions over the list offered after a workspace: it is exactly the grammar's reserved verbs, it never offers a word the grammar retired (which the CLI recognises only to refuse), and every flag in it is one the grammar accepts. Each was checked red by mutation: dropping `reset`, adding `prune`, and adding a `--bogus` each fail the matching test and no other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The flag a value follows is written down three times — the completion script switches to completing a value after it, aid has to tell such a value from the workspace spec, and the grammar declares it — so all three are compared against each other rather than two of them against a literal. aid's own flag list is derived from aid's agent table, so adding an agent cannot leave its --flag uncompletable. The two flags beside them (help and version) are hand-written with a reason: aid's grammar is not a clap grammar and has no table to derive them from. Both checked red by mutation: adding a non-value flag to the script's value list and dropping --gemini from aid's each fail their own test and no other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideAdds missing dl first-argument flags to the bash completion script and introduces a Rust test suite that parses source files to diff the script’s hand-written tables against the dl and aid grammars, ensuring future drift is caught automatically. Flow diagram for completion table consistency checksflowchart LR
Grammar[dl Cli and verb grammar] --> Extract[Source-text extractors]
AidGrammar[aid agent table] --> Extract
Script[dl.bash hand-written tables] --> Compare[completion_tables.rs comparisons]
Extract --> Compare
Compare --> Result{Tables consistent?}
Result -->|yes| Pass[Tests pass]
Result -->|no| Fail[Test failure identifies drift]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 in fresh context. Both axes ran as independent subagents in their own worktrees at 117f469; neither saw the other's output, and each finding below is one the axis reported after actually running the probe. Preflight: merge base a1c6fe8 resolves, diff is 2 files / +458, all 14 checks green including gate.
Standards
Tooling is clean: cargo clippy --locked --all-targets -- -D warnings and cargo fmt --check both exit 0; the 6 new tests pass.
Extractor robustness — probed by reformatting, mostly clean. Seven meaning-preserving reformats of cli.rs/rewrite.rs, each one run: a multi-line #[arg(...)] goes red at rust/dl/tests/completion_tables.rs:147; an attribute or doc comment inserted between attribute and field goes red at :156; pub struct Cli goes red at :134; an AGENTS entry collapsed to ("claude", Agent { goes red at :253 naming what it found; two fields reordered stays correctly green. No reformat produced a silent pass. That is the important result and it holds.
- nit — wrapping
const VERBS … ] =\n [fails at:197with a message that dumps ~20 lines of unrelated source:split_once("] = ")scans forward intoRETIREDinstead of anchoring to the named table.
aid_agents can pass vacuously — should-fix. With AGENTS emptied, names.len() == 0 and body.matches("Agent {").count() == 0, so the self-check at :253 passes on 0 == 0; only the outer diff at :445 went red. grammar_words has the same 0 == 0 hole — RETIRED is saved by the explicit !retired.is_empty() at :354, VERBS is not. The module doc at :24-26 claims "every extractor asserts it found something", and for these two that is not true. One assert!(!names.is_empty()) each closes it, and it matters more here than usual because self-validating extractors are this file's whole anti-vacuity argument.
assert!(flags.len() > 5) (:171) — nit. 17 flags declared, 13 user-facing. Any silent subset loss already fails the outer diffs, so the specific number is arbitrary.
The DL_VALUE_OPTIONS derivation is not honest — should-fix. Verified by mutation: adding profile: Option<String> to Cli, to DL_VALUE_OPTIONS, and to dl's own bash tables still leaves :445 red, demanding --profile in aid's flag list under the message "the completion script's aid flags have drifted from aid's agent table". "dl flags whose value is a separate word" and "the dl pass-throughs aid's completion offers" coincide at --devcontainer and nowhere else; a non-value pass-through has no derivation at all. Reuse at :438 reads as derived when it is a coincidence.
NOT_OFFERED_FIRST is justified; the comment shipped in dl.bash is not — should-fix. Running the built binary: dl --json --ls prints [], dl --force --ls prints "--force means nothing for --ls.", dl --yes --prune runs. All four withheld flags can be the first word, so rust/devlaunch-core/completions/dl.bash:60-63's "none of them is ever the first word" is false. The test's own doc at :290 is right for the other reason — sourcing the script confirms dl --yes <TAB> and dl --ls <TAB> return nothing. requires = "ls" verified at rust/dl/src/cli.rs:431,435.
- nits — neither exception table's reason string is ever asserted or printed on failure, so they are documentation in tuple clothing; "these two" for three entries is fine (
--help/-hare one flag); aid's help/version really are literals (rust/aid/src/main.rs:75,80), so "no table to derive" holds.
Placement and duplication — clean. Cross-crate ../ has precedent (rust/aid/tests/rewrite.rs:50, rust/devlaunch-core/src/clients/devpod.rs:1977); no crate sets publish and no workflow runs cargo publish, and the packaging job builds a wheel without tests, so vendored-tree breakage is unreachable. devlaunch-core has no tests/ dir, so dl/tests is the only workspace-level home. test/test_bash_completion.py:137-168 asserts subsets in real bash and never could catch drift; the new test asserts contents but never that the table is actually offered — complementary, not contradictory.
Spec
Measured against #311: "completions/dl.bash:58 is missing seven current flags (--prune --purge --reconcile --refresh --rm --stop --autorm) … Add a test diffing the script's extracted tables against the source-of-truth tables."
The named defect is fixed and the guard was genuinely red. Restoring only the old global_opts line onto the new test fails every_user_facing_dl_flag_is_offered_for_a_first_argument (:317) with a diff of exactly those seven flags — no more, no fewer. All three tables the ticket names are diffed: VERBS (rust/dl/src/cli.rs:38), aid's AGENTS (rust/aid/src/rewrite.rs:51), and the flag list. Scope held: only dl.bash and the new test changed, no visibility widened.
Real bash checked, not assumed. Sourcing dl.bash and driving _dl_completion via COMP_LINE/COMPREPLY: dl --pr→--prune, dl --a→--autorm, dl --r→--refresh --reconcile --rm, dl --s→--stop, no duplicates; dl --l→--ls, dl <ws> <TAB>→verbs, aid --c→--claude --codex, dl blooop/, dl dev all unregressed. test/test_bash_completion.py passes (25), and it does not itself cover the new flags.
Mutation results — each applied, run, restored:
| mutation | went red |
|---|---|
old global_opts line |
first-argument (exactly the 7) |
visible #[arg(long)] bool added to Cli |
first-argument |
hidden #[arg(long, hide = true)] added |
none (correct) |
Option<String> flag added |
first-argument + value-taking |
verb added to / removed from VERBS |
workspace-verbs (both) |
second word in RETIRED, still offered |
retired-verb |
--bogus in ws_cmds |
phantom-flag |
| 4th agent added / agent removed | aid-agents (both) |
2nd DL_VALUE_OPTIONS entry |
value-taking + aid-agents |
short-only #[arg(short = 'w')] |
none — hole |
stale NOT_OFFERED_FIRST entry (--nonexistent) |
none — hole |
hidden --update-cache offered in ws_cmds |
none — hole |
- should-fix — short-only flags are invisible to the guard. Adding
#[arg(short = 'w')] widget: boolbuilds,dl --helplists-was user-facing, and all six tests pass.grammar_flagskeys onlongonly (:158-167), so against the file's own promise at:9-10— "a flag added to the grammar completes nothing, and nothing fails" — a gap remains. (A field with no#[arg]at all is not a hole: clap panics, "Argument 'widget' is positional and it must take a value".) - nit — the escape hatch is unvalidated. A
NOT_OFFERED_FIRSTentry naming a flag the grammar no longer has passes silently. Assertingwithheld ⊆ grammar flagsis one line, and without it the exception table can outlive its exceptions. - nit — asymmetry between the two halves of the diff.
every_flag_offered_beside_a_workspace_is_a_flag_the_grammar_acceptscompares against all grammar flags including hidden ones (:372), sows_cmdsmay offer--update-cacheundetected — contra the PR body's "Flags the grammar markshide = truedrop out by rule". - should-fix, scope-defensible — the declined gap is wider than the PR says.
dl --autorm <TAB>offers nothing (verified in real bash). Butparse(&["--autorm", "ws"]),parse(&["--stop", "ws"])andparse(&["--rm", "ws", "--force"])are all tested-valid forms (rust/dl/src/cli.rs:1648,1396,1398), so newly offering these flags in first position creates three dead-end completions, not the one the PR body names. Deferring the fix is consistent with the ticket's "this guard is the tactical stop-loss either way"; understating the blast radius is not.
Verdict
Request changes — the guard is real and well-built (10 of 13 mutations caught, no reformat produced a silent pass, red-before-green independently reproduced), but three cheap fixes should land before merge, and two of them are places where the PR's own claims are not true of the code:
aid_agentsandgrammar_wordscan pass on 0 == 0 (:253,:210), while the module doc at:24-26says every extractor asserts it found something. Addassert!(!names.is_empty())/ the same forVERBS. This is the file's central anti-vacuity mechanism, so the exception is worth closing rather than documenting.dl.bash:60-63states something false — all four withheld flags can be the first word; what is true is that the script offers nothing after a leading--. Reword to the reason the test's own doc gives at:290.DL_VALUE_OPTIONSreused as aid's pass-through table (:438) will mis-fire on the next value-taking flag and blame "aid's agent table" for it. Either hand-write aid's pass-throughs inAID_FLAGS_BESIDE_THE_AGENTSwith a reason, like the other exceptions, or say in a comment that the two sets are asserted equal on purpose.
Non-blocking, worth a line each: short-only flags (Spec 1) and hidden flags beside a workspace (Spec 3) are documented gaps in the guard's coverage rather than defects in what it does check; the NOT_OFFERED_FIRST subset assertion (Spec 2), the flags.len() > 5 floor and the VERBS error message are nits. The PR body should say three dead-end completions, not one (Spec 4).
Two things this branch got wrong once main caught up with it, and the first is the guard reporting on itself. `--stop` and `--autorm` were user-facing flags when this list was written. Main retired both (`--rm` is docker's `--rm` now) and marked them `hide = true`, so offering them completes a line the CLI answers with "that spelling moved". The test said so on the first run after the merge, which is the whole claim it makes — the list is five flags longer than it was, not seven. They come out with no entry in `NOT_OFFERED_FIRST`, deliberately: hidden flags drop by rule, so a retirement needs no edit to the test, only to the list. The comment in the script now says that, because "why is a flag the grammar declares missing from here" is the question the table has to answer on its own. The second is worse, and it is not about flags. `ci.yml`'s `rust` job names one test binary per step -- the split that gives each a timeout naming the wedged suite -- and `completion_tables` was in no step. `cargo build --all-targets` compiled it, clippy checked it, and nothing ran it: a guard against drift that would itself have drifted silently, which is the exact failure mode the file's own `gate` comment describes as "a check nobody ran reads exactly like a check that passed". Named now, in the `rust` job and in `rust-coverage`'s suite list. Checked by mutation both ways round, twelve edits: dropping and inventing a first-argument flag, dropping a verb, offering a retired one, inventing a workspace flag, adding a non-value flag to `value_opts`, dropping an agent from aid's list -- and, on the grammar's side, a new flag, a new value-taking flag, a newly hidden `--rm`, a ninth verb, a fourth agent. Each reddens its own test and no other; all six pass restored. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
blooop
changed the title
|
Updated from It is five flags now, not seven. Both come out of the list with no entry in The guard was in no CI step. Mutation-checked both directions, 12 edits. Script side: dropped
Pre-existing and left alone: |
1 participant
Closes #311
devlaunch-core/completions/dl.bashcarries three tables copied in by hand — theflags
dltakes as a first argument, the workspace verbs, and one--flagpercoding agent for the
aidit also serves. The originals live in two other crateswith nothing linking them, and the drift had already happened: the script offered
six flags where the grammar declares thirteen user-facing ones, so
--prune,--purge,--reconcile,--refresh,--rm,--stopand--autormhad beenuncompletable for releases with nothing failing.
rust/dl/tests/completion_tables.rsis the diff that fails instead, and the sevenflags are added so it passes.
What is derived and what is not
Everything the guard can derive, it derives — the flag list from
Cli's fields,the verbs from the reserved-verb table, the retired words from the retired table,
the agents from
aid's agent table. Two small tables are hand-written with areason per entry, and that is the mechanism rather than a shortcut: a flag added
to the grammar breaks the test until somebody either completes it or writes down
why it is not completed.
--json,--size,--yes,--force— because none of them is ever the first word; they modify a line thatalready named a command.
hide = truedrop out by rule, not by hand, so a newhidden flag needs no edit here.
The one fact written down three times — which flag a value follows — is compared
three ways rather than two-against-a-literal: the script's
value_opts, aid'sDL_VALUE_OPTIONS, and the grammar's non-boolflag field.Why the tables are read as source text
Every original is private to its crate (
dl'sCliand verb table,aid's agenttable). Widening a type's visibility so a test can see it is a worse trade than
reading the file it is written in, so the extractors parse text — and each one
asserts the count it found against a count declared in the source, so a parser
that silently matched nothing fails instead of passing vacuously.
Red before green
Slice 1 was genuinely red: the first-argument diff came back missing exactly the
seven flags the ticket names. The other three tables were already consistent, so
each of their guards was checked by mutation — dropping
reset, addingprune,adding a
--bogus, adding a non-value flag tovalue_opts, dropping--geminifrom aid's list — confirming each fails its own test and no other, then restored.
Deliberately not in this PR
tactical stop-loss that makes the hand-written copy safe to keep until then, and
the test's own docs say so.
dl --autorm <TAB>still offersnothing, because the script ends completion once the first word starts with
--.A real gap, and a behaviour change rather than a table diff.
Checks
cargo test --workspace,cargo clippy --locked --all-targets -- -D warningsandcargo fmt --checkare clean.test/test_bash_completion.py, which sources thescript in a real bash, passes unchanged (25 tests). No source file outside the
completion script and the new test was touched.
🤖 Generated with Claude Code
Summary by Sourcery
Keep Bash completions aligned with the command grammars by adding the missing flags and enforcing table consistency in tests.
New Features:
dlflags.Bug Fixes:
dlgrammar andaidagent definitions, including retired verbs and unsupported flags.Enhancements:
dlflags, workspace verbs, value-taking options, andaidagent flags.CI:
Tests: