fix(cli): decide shim ownership from what it launches - #260

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Both findings were correct and are fixed in 0d561f2.

"Do not treat a generic vera directory as ownership proof" — valid. exec /opt/vera/bin/rg launches rg, and my rule claimed and deleted it.

"Classify only command positions" — valid, and it is the substance of #249. The issue asked for program-position ownership specifically, and my first commit delivered comment-filtering and component-wise matching but not position, so an argument still counted as a launch.

Rather than pick between your two suggested criteria I checked what the installer actually writes. packages/npm-cli/bin/vera.js:275-289 emits:

#!/bin/sh
exec "<vera_home>/bin/<version>/<target>/vera" "$@"

and the Windows branch is the same shape with vera.exe. So a genuine launcher is always both named vera and inside the Vera home — which means tightening to "file name is a Vera launcher, or lexically inside vera_home" costs no real coverage, and the bare-directory rule that produced your false positive can simply go. There is no node_modules/vera/cli.js case to preserve; the shim execs the binary directly.

Ownership now comes from the program each command runs. Lines split on the command separators first so a chained cd /tmp && exec vera is still ours, with exec/command/nohup/env and NAME=value prefixes stepped over to reach the program itself.

Both of your cases are now regression tests, and each one discriminates on its own — reinjecting only the component rule fails the /opt/vera/bin/rg case, and reinjecting only the all-tokens scan fails the echo "$HOME/.vera/bin/vera" case, each with 19 others passing. The positive test now builds its fixture from the installer's own shape and the resolved vera_home instead of a path I invented.

172 passed, cargo fmt --check clean, clippy at master's baseline of 7.

All four findings valid, fixed in 3f75b49. Two of them were the same dishonesty as #212 from the opposite direction — leaving Vera's own launcher on PATH while reporting a clean uninstall — so thank you for both.

|| should be && (CodeRabbit + cubic). Correct. /opt/other/bin/vera is named the same and is not ours. Requiring both halves costs nothing real: packages/npm-cli/bin/vera.js:275-289 only ever writes a launcher that is both named vera and inside the Vera home.

Separator inside quotes invents a command (cubic P1). Correct, and constructible in the deleting direction: echo "see; <vera_home>/bin/vera" split on ; yields a second "command" whose program is a real Vera path, so a foreign launcher got removed.

Spaces in VERA_HOME (cubic P2). Correct, and the worse half. split_whitespace cut the quoted binary path in two, so the genuine shim was never recognized and stayed on PATH while complete: true was reported.

Both splits are now a single quote-aware scan that tracks ", ' and backtick, so separators and spaces inside a quoted path are data.

$HOME fixture (cubic P2). Correct, and a good catch on why it passed — the name alone used to be enough, so the containment half was never exercised. The chained-launch and argument-position fixtures now build from the resolved roots.vera_home via a {home} placeholder substituted inside the loop.

Each fix is reinjection-checked separately:

174 passed, cargo fmt --check clean, clippy at master's baseline of 7.

Valid, fixed in c513b89.

You are right, and the gap is exactly where you place it: is_comment_line only recognizes a comment that starts a line, so my scanner parsed straight through an inline #. Your example is the deleting direction — echo safe # ; exec "<vera_home>/.../vera" — where the shell never runs Vera but the scanner sees a separator and a Vera path in program position.

An unquoted # at a word boundary now ends the line. Two details I kept deliberately:

• Commands before the # are still evaluated. exec "<vera_home>/.../vera" # note is a genuine launch and must stay ours, so the scanner breaks out of the character loop but still evaluates the tokens it has collected.
• A # inside a word is not a comment, per POSIX word rules — token.is_empty() is the boundary test. Pinned by a_hash_inside_a_word_does_not_start_a_comment, using a version directory like 1.0#rc1, so a future tightening cannot silently start treating mid-word hashes as comments and lose our own launcher.

Both directions are regression-tested. Reinjection: removing the one '#' if token.is_empty() => break arm fails an_inline_comment_ends_the_line and nothing else (23 pass).

176 passed, cargo fmt --check clean, clippy at master's baseline of 7.

Four findings this round; all valid, fixed in 355f5f5. One was already addressed.

Escaped separators (P1). Correct. \\ escapes in the shell family and ^ in the batch family, and only in front of a character whose special meaning is actually being suppressed. That last part matters in both directions: escaping unconditionally would eat the backslashes out of C:\\Vera\\vera.exe and the caret out of a legitimate unix path, turning a fix for a false deletion into a cause of a false survival.

# is not a comment in batch (P2). Correct, and it is the failure direction I care most about — truncating a .cmd line at # loses our own launcher. The family is now decided once per file from the markers the installer itself writes (@echo off, %*), so our Windows shim identifies as batch by construction.

# after an empty quoted word (P2). Correct, and a sharp catch. "" leaves the decoded token empty while the lexical word has started, so echo ""# ; exec … stopped the scan before a real launch. The boundary test now tracks word start rather than token emptiness.

NAME=value recognised by name (P2). Correct — deciding on punctuation in the value meant an assignment to a path was taken for the program, so the real program was never examined. Adopted your predicate.

Inline # (P1). Already fixed in ba2e13e, pushed a few minutes before this review ran; you were reading the previous commit.

Each fix is reinjection-checked separately, and each fails only its own test (27 pass, 1 fails) — escaped separator, environment assignment, empty-quoted-word, batch-hash.

One correction on my own work while I am here: my first batch fixture put the # inside quotes, so it never reached the rule it claimed to test and passed with the fix reverted. Rewritten as echo # && "<vera_home>/…/vera" %*, which does fail when the batch exemption is removed. Worth flagging rather than quietly fixing, since a test that cannot fail is worse than no test.

180 passed, cargo fmt --check clean, clippy at master's baseline of 7.

Both valid, fixed in ea384bc. Both were created by the previous round's fix, which is the pattern worth naming: every round here has been the family-detection or escaping rule being almost right.

looks_like_batch misfires on a shell script containing %* (P1). Correct, and the consequence chain is exactly as you describe: wrong family swaps the escape character, \\; stops being an escape, the separator splits, and a Vera path lands in program position. A shebang now settles the family before any marker is consulted, because the kernel will run that file through a shell whatever else it contains. Markers are only consulted when there is no shebang to ask.

NAME=value is not a batch prefix form (P1). Correct. cmd has no such form, so on a batch line a leading NAME=... is the program, and skipping it handed the next token the program slot. The rule is now shell-only.

Reinjection, each separately: removing the shebang check fails a_shebang_settles_the_family_against_a_stray_batch_marker; removing the !batch gate fails a_batch_line_has_no_environment_assignment_prefix. 29 pass in each case.

182 passed, cargo fmt --check clean, clippy at master's baseline of 7 (checked with --tests this time as well, after #262 showed me that omitting it hides warnings in test code).

Valid, fixed in bee8bb7, and it is the right correction to the previous round rather than a new area: I made the family known and then went on applying shell syntax to both sides of it, which is half a fix.

Both of your cases reproduce, one in each direction:

• ; in batch. Ordinary argument text to cmd, so echo safe; "<vera path>" splits into a command that never runs and whose first token is a Vera path. Foreign launcher deleted.
• ' in batch. cmd quotes with " alone, so an apostrophe opened a quote that swallowed the rest of the line and hid the launch after &. Our own shim left on PATH, which is the vera uninstall leaves a cargo-installed binary on PATH and still reports a complete removal #212 failure again.

In batch mode the separators are now & and |, and " is the only quote. Regressions for both, each reinjection-checked on its own: restoring ; as a batch separator fails a_semicolon_is_not_a_batch_command_separator; restoring the apostrophe as a batch quote fails an_apostrophe_does_not_quote_in_a_batch_shim. 31 pass in each case.

184 passed, cargo fmt --check clean, clippy with --tests at master's baseline of 7.

One more, found by self-review rather than by a reviewer, in ee03356.

Given the shape of the last six rounds I went looking for the constructs the scanner had not been asked about yet, and wrote the cases before checking whether they passed. Four of five failed:

All four are the #212 direction: our own launcher stays on PATH while the uninstall reports success. The cause is that the scanner only ever looked at the first word of a command, so a launch anywhere else in one was invisible. Parentheses group commands in both families, so they now bound a command the way a separator does, and the shell keywords and grouping tokens that can precede a program join the words already stepped over.

Reinjection, separately: removing the paren handling and removing the keywords each fail a_launch_inside_grouping_or_a_conditional_is_still_ours, 32 others passing.

Also worth recording, since it nearly became a false negative in the test rather than the code: my first version of that fixture had {{ instead of {, because I reached for Rust format-escaping in a plain string literal. It failed for the wrong reason and would have passed for the wrong reason once I 'fixed' the parser to match. Caught because the assertion prints the body.

185 passed, cargo fmt --check clean, clippy with --tests at master's baseline of 7.

Both valid, fixed in b4727e7. Both are consequences of the previous commit, and they are the same mistake in mirror image: a rule applied where it is not the syntax.

Shell if condition (P1). Correct. A shell if takes a command list as its condition, so if exec "<vera>"; then is a launch, and reading if as the program left our own shim on PATH. Your caveat is the important half: batch if takes a comparison, not a command, so the keyword list is now shell-only. Stepping over if in batch would hand the program slot to the comparison.

Parentheses (P1). Correct, and my previous commit split on them unconditionally, which is how an echoed (path) became a command. Parentheses now open a group only where they are group syntax: at the start of a command, or standing alone as their own word. Attached to text they are argument characters. A closing paren only ends a group that was actually opened, so a stray ) cannot end a command either.

That keeps the four grouping cases from the last round working while dropping the two false positives you found. Reinjection, each separately: removing if from the keyword list fails a_shell_condition_command_is_still_a_launch; removing the paren guard fails parentheses_around_an_argument_do_not_open_a_group. 34 pass in each case.

187 passed, cargo fmt --check clean, clippy with --tests at master's baseline of 7.

Both valid, fixed in 90a67b4.

case arm (P1). Correct. An arm terminates its pattern with ) and never opens one, so *) exec "<vera>" ;; sat behind a paren no group accounted for and our own shim stayed on PATH. A bare ) now bounds a command, gated on the script actually containing case/esac.

The gate is a deliberate approximation and worth naming as one: outside that construct a lone ) is argument text, and treating it as syntax is what deleted the echoed (path) you caught last round. So the two rules pull in opposite directions and I resolved it by scope rather than by preferring one. If a script both uses case and echoes a parenthesized path in our home directory, this still errs toward deleting; I would rather state that than pretend the heuristic is exact.

Quoted keywords (P2). Correct, and it generalizes further than the four you listed: the same hole existed for "exec", which is in the other prefix list. Tokens now carry whether any part of them was quoted, and only unquoted words are read as syntax, so "if" "<vera path>" runs a program called if and is left alone. The regression covers if, while, until, ! and exec.

Reinjection, each separately: removing the case_arms term fails a_launch_in_a_case_arm_is_still_ours; ignoring quotedness fails a_quoted_keyword_is_a_program_name_not_syntax. 36 pass in each case.

One thing I caught on myself before pushing: threading quotedness through raised clippy from master's 7 to 13, all value assigned to \word_quoted` is never readfrom redundant resets. Removed; back to 7 with--tests`.

189 passed, cargo fmt --check clean.

Stepping back from round nine, because the round count is now itself evidence and I would rather put the argument in front of you than send a tenth patch.

Nine rounds, and rounds eight and nine were caused by my own previous fix. The findings have all been the same kind: quoting, escapes, comments, dialect differences, grouping, keyword scope, case arms. That is not a run of bad luck, it is what happens when a decision depends on interpreting a language, and the finding rate is not obviously converging to zero.

The parser is solving a problem that does not exist. Uninstall never has to understand arbitrary shell. It only has to recognize the shim we wrote. There are exactly two, byte for byte:

• packages/npm-cli/bin/vera.js:280-284 — #!/bin/sh\nexec "{binary}" "$@"\n and @echo off\r\n"{binary}" %*\r\n
• packages/python-cli/src/vera_ai_wrapper/__main__.py:228-231 — the same two, character for character

And the binary they name is already recorded. install.json carries binary_path, state.rs:49 exposes it, and upgrade.rs:125 already uses exactly that to find the installed executable. So the convention for "which file is ours" exists in this codebase already, and uninstall is the one place not using it.

That gives a rule with no lexical surface at all:

A PATH entry is ours when it is one of the two templates naming the recorded binary_path, or a symlink resolving to it. Anything else is left alone.

Every finding from rounds three through nine becomes unreachable rather than fixed, because none of those constructs can appear in a byte-comparison. It is also strictly more conservative in the direction that matters: an unrecognized file is never deleted.

Two things I do not know, which is why this is a question rather than a commit:

1. Older shims. If earlier releases wrote a different template, matching only the current two would stop recognizing them, and the honest fallback is the recorded binary_path plus a substring check on the file, or simply leaving them and reporting it.
2. Ordering. Step 2 removes the Vera home, so install.json has to be read before that rather than after.

I am happy either way, and the current PR stands on its own: 189 tests, every fix reinjection-checked, all threads answered. But if you would rather have the smaller thing, say so and I will open it as a separate PR against the same issue and close this one, rather than keep patching a parser toward a spec that is not written down anywhere.

Both valid, fixed in 37685f9.

case_arms too coarse (P1). Correct. Asking whether the words appear anywhere in the file meant a comment or a string mentioning them made every ) a command boundary, in the deleting direction. The block is now tracked line by line: opened only by a line whose first word is case and which also contains in, closed by esac as a word.

Assignment with a quoted value (P1). Correct, and it is my own round-nine fix biting: the quoted-word rule ran first and read NAME="value" as a quoted program name, so the launch after it was never examined. An assignment is recognized by its name, and the name is unquoted even when the value is not, so that check now runs first. Reinjecting the ordering fails two tests rather than one, which is the right shape: it also breaks the unquoted case from an earlier round.

191 passed, each fix reinjection-checked, cargo fmt --check clean, clippy with --tests at master's baseline of 7.

This is the third consecutive round whose findings were caused by the previous round's fix, and I want to put that number next to the proposal above rather than let it pass as normal.

Rounds 8, 9 and 10 were each introduced by the fix for rounds 7, 8 and 9. Every one has been a P1, and most have been in the deleting direction, which is the unrecoverable one. The tests accumulate and each individual fix is right, but the defect rate is not falling, and that is the signal that matters more than any single finding.

The alternative in my previous comment removes the surface rather than patching it: match the two byte-identical templates the installers write against the binary_path already recorded in install.json, exactly as upgrade.rs:125 does. Every finding from round three onward becomes unreachable, because none of these constructs can appear in a byte comparison.

I will keep this PR correct and answered for as long as it is open. But if you have a view on the smaller design, now is the point where it saves the most work: I would rather build that than send an eleventh patch to a parser whose spec is not written down anywhere.

All three findings are valid, and I am not going to send a twelfth patch for them. Here is why, and what I did instead.

What you found, confirmed:

1. esac inside an inline comment is read as syntax, and a punctuation-attached esac is missed, because case state is derived from raw text rather than from the tokens the scanner already produces.
2. A fully quoted "NAME=value" command is treated as an assignment prefix, so the shim is deleted even though the shell runs a program by that name. This one is my round-ten fix biting: I moved the assignment check ahead of the quoted-word rule to fix the opposite bug, and created this.
3. case following another command on the same line is missed, because opens_case_block requires it to be the first word.

This is the fourth consecutive round whose findings were caused by the previous round's fix. Rounds 8, 9, 10 and 11, every one a P1, alternating between deleting somebody else's launcher and leaving ours behind. I started the correct fix for these three: it needs case state derived from scanned tokens rather than raw text, and per-token tracking of whether quoting began before the =. Partway through I stopped, because I was refactoring a parser to satisfy a specification that does not exist anywhere, and the four-round trend says the twelfth patch produces a thirteenth finding.

#263 is the same fix without the parser, opened before this round landed: match the two byte-identical templates the installers write against the binary_path already recorded in install.json, exactly as upgrade.rs:125 does. All three of your findings become unreachable there rather than fixed, because none of esac, quoting, or command position can appear in a byte comparison. Its test suite runs this PR's entire eleven-round corpus of foreign launchers and every one survives by construction.

This PR stays at 37685f9, which is correct and green as far as round ten: 191 tests, every fix reinjection-checked, clippy at baseline. I am marking it draft so it does not get merged ahead of the choice, not because the work is wrong. If you prefer this approach, say so and I will finish the round-eleven fixes properly; if you prefer #263, close this one.

I would rather hand you a decision with both options built than keep patching toward whichever spec the next round implies.

Thank you @citron07r for the careful work on this alternative — appreciated.

Closing as superseded by #263 (merged as bca5a8c, head 61ed289) after adjudicating both heads on the exact current master (fbf27ea -> bca5a8c). Both PRs harden #249's shim-ownership decision, but they take different mechanisms:

• fix(cli): decide shim ownership from what it launches #260 (program-position ownership, 195 tests, 1680-line uninstall.rs) parses shell (grouping, conditionals, case/esac, batch rem/::, quoting, env/NAME=value prefixes, lexical normalization, symlink_metadata dangling handling) to decide the program position. Validated in worktree b3332ca: 195 total tests (39 uninstall) pass, clippy clean. Remaining review threads: 3 unresolved — P1 case/esac in inline comments is treated as syntax, P1 fully-quoted NAME=value is misread as assignment, P2 case after ;/&& is missed. These are load-bearing parsing edge cases.

• fix(cli): recognize the shim by matching what the installer wrote (alternative to #260) #263 (byte-identical installer-template matching, 183 tests, 1325-line uninstall.rs) matches the file byte-for-byte against the two templates the installers actually write (sh template with exec and cmd template with echo off), with install.json binary_path containment and symlink hop limit 40, cargo_bin_dir resolution. Validated in worktree 61ed289: 183 total tests (27 uninstall) pass, clippy clean, Validate+MSRV green on up-to-date head 61ed289, master CI green on bca5a8c. Remaining threads: 2 (env set_var in test — test-only, cargo_bin scan for non-default CARGO_HOME — follow-up, acknowledged). No P1s; mechanism is simpler and avoids parsing.

Decision: merge #263. Reasoning: simpler, authoritative mechanism (template match) is easier to audit and has no P1 correctness gaps; test strength is comparable (27 focused uninstall tests covering templates, symlinks, install.json vs 39 broader but parsing-sensitive), and review-cleanliness is better (0 P1 vs 2 P1). PR #260's approach is more general but carries parser complexity and open P1s.

Crediting @citron07r for both alternatives — #260 informed the comparison and remains a valuable reference if template drift ever requires parsing. No action needed on this PR.