fix(scripts): let parseVitestArgv keep every value of a repeated flag - #8002
Merged
baozhoutao merged 1 commit intoSep 6, 2026
Merged
Conversation
`parseVitestArgv` stored flags in a plain object (`flags[token] = next`), so a flag written twice on one command line kept only its last value. The guard's own readers never noticed — `:303` asks `--changed` for existence and `:357` asks `--root` for its one effective value, and neither is legitimately repeated — but the root `test:integration` script is `vitest run --project dom --project dom-heavy`, which the parser answers as `dom-heavy` with `dom` dropped and no symptom. The first reader to reuse the exported parser to ask "which projects does this command run" gets a confidently wrong answer. `flags` is left exactly as it was, last-wins, so the two scalar readers are byte-for-byte unaffected. A sibling `flagValues` map carries every occurrence of every flag in argv order; a flag seen once is a one-element array there, so a reader never needs a scalar-or-array fallback. All three recording sites (the `--flag=value` form, the space-separated value form and the bare boolean) route through one `record()` helper, so the two views cannot drift. The pin asserts both values come back in order for `--project dom --project dom-heavy`, that the scalar still reads the last one, and that a single `--project a` reads back as `['a']` — a non-empty-only assertion would pass against the unfixed parser. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MM7kaS4dPpYHV5BsMyu4tQ
Contributor
Author
|
Standing down on Generated by Claude Code |
baozhoutao
marked this pull request as ready for review
September 6, 2026 08:18
baozhoutao
deleted the
claude/issue-7329-parse-vitest-argv-repeated-flags
branch
September 6, 2026 08:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7329
parseVitestArgvstored flags in a plain object,flags[token] = next, so a flag written twice on one command line kept only its last value. The guard's own two readers never noticed —:303asks--changedfor existence,:357asks--rootfor its one effective value, and neither is legitimately repeated — but the roottest:integrationscript is real and repeated:Fed to the exported parser, that answers
--project=dom-heavyand dropsdomwith no symptom. Triage's boundary 1 is taken: fix the contract rather than document that it is narrow, because someone has already routed around this helper — PR #7327's pin carries its own extractor instead of reusing it, and a second router-around would be a third copy of the same extraction logic.The shape, and why the scalar readers are untouched
flagsis left exactly as it was: last-wins, one scalar per flag. A sibling mapflagValuescarries every occurrence of every flag in argv order./** @returns {{ …, flags: Record<string, string | true>, flagValues: Record<string, Array<string | true>> }} */Three properties motivated this over the alternatives:
flagsnever moved. Turningflagsinto arrays would have brokentypeof flags['--root'] === 'string'at:357andflags['--root'] ?? flags['-r']inrunner-package-test-entry-3746.test.ts; an array-or-scalar union would have pushed aArray.isArray()fallback into every call site — the lenient-fallback shape AGENTS.md #0.1 argues against.flagValuestoo, as a one-element array. That is the difference between a sibling map a reader can trust and one a reader has to guard: "which projects does this run" isflagValues['--project'] ?? [], neverflagValues[x] ?? [flags[x]].--flag=valueform, the space-separated value form, the bare boolean — route through onerecord(name, value)helper that writes both. Previously they were three independent assignments.Naming:
flagValuesoverrepeated, because the map holds every flag rather than only the repeated ones; a reader who seesrepeated['--project']come backundefinedfor a single--project awould reasonably add exactly the fallback this shape exists to avoid.The pin
In
scripts/__tests__/vitest-invocation-guard.test.ts, two cases in the existingparseVitestArgvdescribe:--project dom --project dom-heavyyields['dom', 'dom-heavy']— both values, in order — and the scalar still readsdom-heavy, and neither value leaked intopositionals.--project ayields['a'];--shard=1/4yields['1/4']and a bare--watchyields[true], so the other two spellings are pinned too.Per triage boundary 3, a non-empty-only assertion is deliberately not what is written: it passes against the unfixed parser.
Reverse verification
Two legs, because the cheap one is weak on its own.
Leg 1 — the pin against the unmodified parser (run before the fix): 2 failed | 35 passed. But it failed with
TypeError: Cannot read properties of undefined (reading '--project')— that proves the field is absent, not that the pin detects the defect.Leg 2 — value-level ablation on the committed implementation:
flagValueskept, only the accumulation broken ((flagValues[name] ??= []).push(value)→flagValues[name] = [value], i.e. last-wins in both views). On-disk proof, injected spellinggrep -c= 1 and deleted spellinggrep -c= 0, blob27e24f8→57d4435. Result:That is the card's defect verbatim. Restored with
git checkout HEAD -- <path>; blob hash back to27e24f8,git diff HEADempty.Gates (all at
92b4602)Test Files 3 passed (3)/Tests 52 passed (52)scripts/__tests__/suiteTest Files 107 passed (107)/Tests 3247 passed (3247)pnpm type-check:scriptspnpm lint:root✖ 32 problems (0 errors, 32 warnings)— 0 errors; per-file JSON says the parser contributes 0 messages and the test file's single warning (VITE_CONFIG_NAMEunused) is pre-existing, atorigin/main:282 and shifted to :307 by this diff's 25 added linespnpm check:control-bytes✅ check-control-bytes: OK (scanned 6439 tracked text file(s); skipped 85 binary).node scripts/check-changeset-presence.mjs✅ No source or published contract of a released package changed in this range, so no changeset is owed.—2 file(s) changed, 0 of them published sourcenode scripts/check-governed-queue-guard.mjs --test <the 2 paths>✅ NOT GOVERNED — 2 path(s) checked against 5 governed surface(s); none matched.check-node-esm-load.mjswas not run: it gradesdiscoverPackages(root).filter(p => !p.manifest.private)againstMIN_PACKAGES = 30published workspace packages and theirdist.scripts/is not a package and this diff publishes nothing, so it is out of that gate's scope.Live E2E (informational)is red on every branch today for an upstream reason (#7990 / objectstack#16186) and is not this diff's.Readers of
parseVitestArgv, re-grepped on the branchStill exactly three, and this confirms triage's correction to the card ("the guard is its only consumer" was wrong):
scripts/vitest-invocation-guard.mjs— itself.:332flags['--changed'](existence),:386flags['--root']/flags['-r'](value). Scalar; unaffected.scripts/__tests__/vitest-invocation-guard.test.ts— its own test, where the pin lands.scripts/__tests__/runner-package-test-entry-3746.test.ts— readsflags['--root'] ?? flags['-r']at:133andpositionalsat:193. Neither is a repeatable flag, so the defect stays latent as the card graded it, not live. Left untouched and green.scripts/__tests__/package-scripts-vitest-projects.test.tskeeps its own extractor untouched, per triage boundary 4: it is another PR's acceptance artefact, and its independence is the current protection. Whether to collapse it ontoflagValuesnow that this exists is that card owner's call, not this PR's.Out of scope
Filed #8001 (
finding): the same test file declaresVITE_CONFIG_NAMEfor the "directory has novitest.config.*, Vitest falls back tovite.config.*" case and never sweeps for it — an assertion described in a comment but never written, over the exact mechanism of objectui#3746. Different defect class, so not fixed here.🤖 Generated with Claude Code
https://claude.ai/code/session_01MM7kaS4dPpYHV5BsMyu4tQ
Generated by Claude Code