fix: reject unknown installer arguments #825
Assignees
jason.woltje
Clear assignees
jason.woltje
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: mosaicstack/stack#825
Reference in New Issue
Block a user
Delete Branch "fix/804-install-unknown-flags"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
--reffrom consuming an option-shaped unknown argumentVerification
pnpm --filter @mosaicstack/mosaic test(69 files, 1,287 tests)pnpm lintpnpm typecheckpnpm format:checkbash -n tools/install.shCloses #804.
REVIEW-OF-RECORD (exact-head, durable) — MS-LEAD independent lane
PR #825 · issue #804 · head
a1b2e8d6· base3f77229e· VERDICT: APPROVEAuthor (ms-804 pi/sol) ≠ reviewer (PROCESS/LANE-attested). Full-suite observed 69/69 files, 1287/1287 tests; defect empirically reproduced pre-fix; merge-tree clean
3480b002.Review of Record — PR #825 (issue #804)
tools/install.shsilently discarded unknown flags (fail-open)a1b2e8d66b94f736e9ca5f8b9817063a6e4eed3f3f77229e(fix(#792): fleet roster ENOENT actionable exit + installer heading printf (#818))VERDICT: APPROVE
Summary
The fix replaces the catch-all
*) shift ;;intools/install.sh's arg parser with a fail-closed branch that printsError: Unknown argument: <arg>plus a usage line to STDERR and exits2, before any install/network work runs.--refis hardened against consuming a following option-shaped token (--ref --bogusnow errors instead of silently settingGIT_REF=--bogus), and against being the last, value-less argument. All eight previously-recognized flags are untouched. A new Vitest spec (install-arguments.spec.ts) exercises both the plain unknown-flag case and the--ref-swallowing case, asserting nonzero exit, the STDERR message, and — via annpmshim onPATH— thatnpmis never invoked. No--next/MOSAIC_NEXTsupport was added. Full test suite passes (69/69 files, 1287/1287 tests) plus the framework shell test. Prospective merge ontoorigin/mainis clean.Determinations
bash tools/install.sh --check --bogus→ exit 2, STDERRError: Unknown argument: --bogus, no install proceeds.bash tools/install.sh justsomeword→ exit 2, same STDERR pattern (the catch-all*)branch handles positionals and flags identically, since the script has no legitimate positional arguments by design).--check,--framework,--cli,--ref <val>,--dev,--yes|-y,--no-auto-launch,--uninstall. Ran all of them together (--check --framework --ref develop --dev --yes --no-auto-launch) → exit 0, correct behavior (framework-only check againstref: develop), no errors introduced.--refguard correct? YES — three cases verified live:--ref --bogus→ exit 2,Error: Unknown argument: --bogus(does NOT setGIT_REF=--bogusand silently continue).--refas last token (missing value) → exit 2,Error: Missing value for --ref.--ref develop(legitimate value) → exit 0,GIT_REFcorrectly resolves todevelop(confirmed via--checkoutput: "Source: ... (ref: develop)").--refvalue starting with-("$2" == -*) as "Unknown argument," which is intentionally conservative — no legitimate git ref starts with-, so this has no false-positive risk in practice.git show 3f77229e:tools/install.sh) with--check --bogus: it exited 0 and printed full success output ("Up to date."), proving the defect and that the new spec's assertions (status !== 0,stderrcontainsUnknown argument: --bogus,npmshim never invoked) would fail against the old code. The spec'snpm-shim technique for asserting "install not invoked" is a real behavioral check, not tautological — it pointsPATHat a fakenpmbinary and asserts the marker file it would create was never written.--nextscope creep? YES —grep -n "next\|NEXT" tools/install.shreturns no matches; the diff touches only the arg-parser block,installer_usage(), README/docs mentions, and the new spec file.MOSAIC_NEXTis not referenced anywhere in the diff.Findings
No MAJOR or MINOR findings.
tools/install.sh,--refvalue guard): rejecting any--refvalue starting with-is a slightly blunt heuristic — a git ref can theoretically start with-in extremely contrived cases (not realistic for this project's branch-naming conventions), and the error message reuses the generic "Unknown argument" wording rather than something--ref-specific like "--refvalue looks like a flag." Not a defect; purely a message-clarity nit. No fix required for merge.--ref-swallow). It does not add a case for an unknown bare positional argument (e.g.justsomeword) even though that path shares the same catch-all branch. I verified this path manually (see 4b above) and it behaves correctly, but an explicit test would have made the coverage self-evident without relying on manual review. Not blocking — the shared code path and manual verification are sufficient evidence for this merge gate.No fail-open paths, no STDOUT-vs-STDERR mixups, no shell-quoting bugs found.
printf '%s\n' "$1"/"$2"usage is properly quoted throughout the new code; no word-splitting or globbing hazards introduced.Merge cleanliness vs
3f77229eCLEAN.
Exit code 0, single tree hash printed (
3480b00214737e7f47bf9e1a19ae42e725e2ad42), no conflict markers / no additional output —git merge-tree --write-treeprints only a resulting tree OID with a clean merge; a conflicted merge would additionally print conflicted-file info and exit nonzero. No modification made to the branch or toorigin/main.Diff evidence
Commit sequence (TDD-compliant):
Core
tools/install.shchange (unified diff, verified line-by-line against the running script):Test evidence (real command output)
1. Syntax check
2. Pre-fix defect reproduction (proves the red test is meaningful)
Confirms: pre-fix,
--bogusis silently dropped and the installer proceeds to "success," exit 0.3. Post-fix manual exercise (same worktree, fixed script)
4. Full automated test suite
All 69 test files / 1287 tests pass. Framework shell test (3/3
ok) passes. No skipped or pending tests observed in output.No scope creep
--next/MOSAIC_NEXTis not present anywhere intools/install.shpost-fix. Diff confirmed to touch only: arg-parser +installer_usage()intools/install.sh, doc mentions of the flag list in README.md / user-guide.md / framework defaults README, and the new spec file. No installer logic beyond the arg-validation fix.