Head commit under review:a1b2e8d66b94f736e9ca5f8b9817063a6e4eed3f
Base / current main:3f77229e (fix(#792): fleet roster ENOENT actionable exit + installer heading printf (#818))
Reviewer: independent Review-of-Record lane (author lane = ms-804; this lane ≠ author)
Date: 2026-07-17
VERDICT: APPROVE
Summary
The fix replaces the catch-all *) shift ;; in tools/install.sh's arg parser with a fail-closed branch that prints Error: Unknown argument: <arg> plus a usage line to STDERR and exits 2, before any install/network work runs. --ref is hardened against consuming a following option-shaped token (--ref --bogus now errors instead of silently setting GIT_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 an npm shim on PATH — that npm is never invoked. No --next/MOSAIC_NEXT support was added. Full test suite passes (69/69 files, 1287/1287 tests) plus the framework shell test. Prospective merge onto origin/main is clean.
(b) Unknown positional fails closed? YES — 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).
(c) All prior flags preserved? YES — enumerated from parser: --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 against ref: develop), no errors introduced.
(d) --ref guard correct? YES — three cases verified live:
--ref --bogus → exit 2, Error: Unknown argument: --bogus (does NOT set GIT_REF=--bogus and silently continue).
--ref as last token (missing value) → exit 2, Error: Missing value for --ref.
--ref develop (legitimate value) → exit 0, GIT_REF correctly resolves to develop (confirmed via --check output: "Source: ... (ref: develop)").
Note: the guard rejects any --ref value 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.
(e) Red-first test adequate (fails pre-fix, asserts nonzero+stderr+no-install)? YES — confirmed by directly running the PRE-FIX version of the script (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, stderr contains Unknown argument: --bogus, npm shim never invoked) would fail against the old code. The spec's npm-shim technique for asserting "install not invoked" is a real behavioral check, not tautological — it points PATH at a fake npm binary and asserts the marker file it would create was never written.
(f) No --next scope creep? YES — grep -n "next\|NEXT" tools/install.sh returns no matches; the diff touches only the arg-parser block, installer_usage(), README/docs mentions, and the new spec file. MOSAIC_NEXT is not referenced anywhere in the diff.
Findings
No MAJOR or MINOR findings.
NIT (tools/install.sh, --ref value guard): rejecting any --ref value 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 "--ref value looks like a flag." Not a defect; purely a message-clarity nit. No fix required for merge.
NIT (test coverage breadth): the new spec covers only the two scenarios named in the PR description (bare unknown flag, --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.
$ git fetch origin main
* branch main -> FETCH_HEAD
$ git merge-tree --write-tree origin/main HEAD
3480b00214737e7f47bf9e1a19ae42e725e2ad42
$ echo $?
0
Exit code 0, single tree hash printed (3480b00214737e7f47bf9e1a19ae42e725e2ad42), no conflict markers / no additional output — git merge-tree --write-tree prints 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 to origin/main.
$ pnpm --filter @mosaicstack/mosaic test
...
✓ src/commands/install-arguments.spec.ts (2 tests) 49ms
...
Test Files 69 passed (69)
Tests 1287 passed (1287)
Start at 16:57:01
Duration 10.40s
> @mosaicstack/[email protected] test:framework-shell
> bash framework/tools/codex/test-pr-diff-context.sh
ok - Gitea PR mode fetches and diffs explicit non-main base and PR head refs
ok - empty code PR diff fails closed before Codex and auto-post
ok - empty security PR diff fails closed before Codex and auto-post
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
$ grep -n "next\|NEXT" tools/install.sh
(no matches)
--next / MOSAIC_NEXT is not present anywhere in tools/install.sh post-fix. Diff confirmed to touch only: arg-parser + installer_usage() in tools/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.
**REVIEW-OF-RECORD (exact-head, durable) — MS-LEAD independent lane**
PR #825 · issue #804 · head `a1b2e8d6` · base `3f77229e` · **VERDICT: APPROVE**
Author (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)
- **PR:** #825 — "fix(#804): reject unknown installer arguments"
- **Issue:** #804 — `tools/install.sh` silently discarded unknown flags (fail-open)
- **Head commit under review:** `a1b2e8d66b94f736e9ca5f8b9817063a6e4eed3f`
- **Base / current main:** `3f77229e` (`fix(#792): fleet roster ENOENT actionable exit + installer heading printf (#818)`)
- **Reviewer:** independent Review-of-Record lane (author lane = ms-804; this lane ≠ author)
- **Date:** 2026-07-17
## VERDICT: APPROVE
## Summary
The fix replaces the catch-all `*) shift ;;` in `tools/install.sh`'s arg parser with a fail-closed branch that prints `Error: Unknown argument: <arg>` plus a usage line to STDERR and exits `2`, before any install/network work runs. `--ref` is hardened against consuming a following option-shaped token (`--ref --bogus` now errors instead of silently setting `GIT_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 an `npm` shim on `PATH` — that `npm` is never invoked. No `--next`/`MOSAIC_NEXT` support was added. Full test suite passes (69/69 files, 1287/1287 tests) plus the framework shell test. Prospective merge onto `origin/main` is clean.
## Determinations
- (a) Unknown flags fail closed? **YES** — verified live: `bash tools/install.sh --check --bogus` → exit 2, STDERR `Error: Unknown argument: --bogus`, no install proceeds.
- (b) Unknown positional fails closed? **YES** — `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).
- (c) All prior flags preserved? **YES** — enumerated from parser: `--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 against `ref: develop`), no errors introduced.
- (d) `--ref` guard correct? **YES** — three cases verified live:
- `--ref --bogus` → exit 2, `Error: Unknown argument: --bogus` (does NOT set `GIT_REF=--bogus` and silently continue).
- `--ref` as last token (missing value) → exit 2, `Error: Missing value for --ref`.
- `--ref develop` (legitimate value) → exit 0, `GIT_REF` correctly resolves to `develop` (confirmed via `--check` output: "Source: ... (ref: develop)").
- Note: the guard rejects any `--ref` value 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.
- (e) Red-first test adequate (fails pre-fix, asserts nonzero+stderr+no-install)? **YES** — confirmed by directly running the PRE-FIX version of the script (`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`, `stderr` contains `Unknown argument: --bogus`, `npm` shim never invoked) would fail against the old code. The spec's `npm`-shim technique for asserting "install not invoked" is a real behavioral check, not tautological — it points `PATH` at a fake `npm` binary and asserts the marker file it would create was never written.
- (f) No `--next` scope creep? **YES** — `grep -n "next\|NEXT" tools/install.sh` returns no matches; the diff touches only the arg-parser block, `installer_usage()`, README/docs mentions, and the new spec file. `MOSAIC_NEXT` is not referenced anywhere in the diff.
## Findings
No MAJOR or MINOR findings.
- **NIT** (`tools/install.sh`, `--ref` value guard): rejecting any `--ref` value 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 "`--ref` value looks like a flag." Not a defect; purely a message-clarity nit. No fix required for merge.
- **NIT** (test coverage breadth): the new spec covers only the two scenarios named in the PR description (bare unknown flag, `--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 3f77229e
**CLEAN.**
```
$ git fetch origin main
* branch main -> FETCH_HEAD
$ git merge-tree --write-tree origin/main HEAD
3480b00214737e7f47bf9e1a19ae42e725e2ad42
$ echo $?
0
```
Exit code 0, single tree hash printed (`3480b00214737e7f47bf9e1a19ae42e725e2ad42`), no conflict markers / no additional output — `git merge-tree --write-tree` prints 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 to `origin/main`.
## Diff evidence
```
$ git diff --stat 3f77229e a1b2e8d6
README.md | 2 +
docs/guides/user-guide.md | 2 +
docs/scratchpads/804-install-unknown-flags.md | 86 ++++++++++++++++++++++
packages/mosaic/framework/defaults/README.md | 2 +
.../mosaic/src/commands/install-arguments.spec.ts | 61 +++++++++++++++
tools/install.sh | 25 ++++++-
6 files changed, 176 insertions(+), 2 deletions(-)
```
Commit sequence (TDD-compliant):
```
a1b2e8d6 fix(#804): reject unknown installer arguments
ed48ea03 test(#804): reproduce unknown installer argument
3f77229e fix(#792): fleet roster ENOENT actionable exit + installer heading printf (#818) <- base
```
Core `tools/install.sh` change (unified diff, verified line-by-line against the running script):
```diff
+installer_usage() {
+ printf 'Usage: install.sh [--check] [--framework] [--cli] [--ref <branch>] [--dev] [--yes|-y] [--no-auto-launch] [--uninstall]\n' >&2
+}
+
while [[ $# -gt 0 ]]; do
case "$1" in
--check) FLAG_CHECK=true; shift ;;
--framework) FLAG_CLI=false; shift ;;
--cli) FLAG_FRAMEWORK=false; shift ;;
- --ref) GIT_REF="${2:-main}"; shift 2 ;;
+ --ref)
+ if [[ $# -lt 2 ]] || [[ -z "$2" ]]; then
+ printf 'Error: Missing value for --ref\n' >&2
+ installer_usage
+ exit 2
+ fi
+ if [[ "$2" == -* ]]; then
+ printf 'Error: Unknown argument: %s\n' "$2" >&2
+ installer_usage
+ exit 2
+ fi
+ GIT_REF="$2"
+ shift 2
+ ;;
--dev) FLAG_DEV=true; shift ;;
--yes|-y) FLAG_YES=true; shift ;;
--no-auto-launch) FLAG_NO_AUTO_LAUNCH=true; shift ;;
--uninstall) FLAG_UNINSTALL=true; shift ;;
- *) shift ;;
+ *)
+ printf 'Error: Unknown argument: %s\n' "$1" >&2
+ installer_usage
+ exit 2
+ ;;
esac
done
```
## Test evidence (real command output)
### 1. Syntax check
```
$ bash -n tools/install.sh
(no output — pass)
```
### 2. Pre-fix defect reproduction (proves the red test is meaningful)
```
$ git show 3f77229e:tools/install.sh > /tmp/pre-fix-install.sh
$ bash /tmp/pre-fix-install.sh --check --bogus
PRE-FIX EXIT:0
Mosaic Stack Installer
Framework (~/.config/mosaic)
Installed: framework v3
Source: https://git.mosaicstack.dev/mosaicstack/stack (ref: main)
✔ Framework is installed.
@mosaicstack/mosaic (npm package)
Installed: @mosaicstack/[email protected]
Latest: @mosaicstack/[email protected]
✔ Up to date.
```
Confirms: pre-fix, `--bogus` is silently dropped and the installer proceeds to "success," exit 0.
### 3. Post-fix manual exercise (same worktree, fixed script)
```
$ bash tools/install.sh --check --bogus
Error: Unknown argument: --bogus
Usage: install.sh [--check] [--framework] [--cli] [--ref <branch>] [--dev] [--yes|-y] [--no-auto-launch] [--uninstall]
EXIT:2
$ bash tools/install.sh --check --ref --bogus
Error: Unknown argument: --bogus
Usage: install.sh [--check] [--framework] [--cli] [--ref <branch>] [--dev] [--yes|-y] [--no-auto-launch] [--uninstall]
EXIT:2
$ bash tools/install.sh --check --ref
Error: Missing value for --ref
Usage: install.sh [--check] [--framework] [--cli] [--ref <branch>] [--dev] [--yes|-y] [--no-auto-launch] [--uninstall]
EXIT:2
$ bash tools/install.sh --check --ref develop
EXIT:0
... Source: https://git.mosaicstack.dev/mosaicstack/stack (ref: develop) ...
✔ Framework is installed.
$ bash tools/install.sh justsomeword
Error: Unknown argument: justsomeword
Usage: install.sh [--check] [--framework] [--cli] [--ref <branch>] [--dev] [--yes|-y] [--no-auto-launch] [--uninstall]
EXIT:2
$ bash tools/install.sh --check --framework --ref develop --dev --yes --no-auto-launch
EXIT:0
... Framework is installed (ref: develop) ...
```
### 4. Full automated test suite
```
$ pnpm --filter @mosaicstack/mosaic test
...
✓ src/commands/install-arguments.spec.ts (2 tests) 49ms
...
Test Files 69 passed (69)
Tests 1287 passed (1287)
Start at 16:57:01
Duration 10.40s
> @mosaicstack/[email protected] test:framework-shell
> bash framework/tools/codex/test-pr-diff-context.sh
ok - Gitea PR mode fetches and diffs explicit non-main base and PR head refs
ok - empty code PR diff fails closed before Codex and auto-post
ok - empty security PR diff fails closed before Codex and auto-post
```
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
```
$ grep -n "next\|NEXT" tools/install.sh
(no matches)
```
`--next` / `MOSAIC_NEXT` is not present anywhere in `tools/install.sh` post-fix. Diff confirmed to touch only: arg-parser + `installer_usage()` in `tools/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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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.