From d3bf52898b6db2fc7845d4a29af4600973844443 Mon Sep 17 00:00:00 2001 From: "jason.woltje" Date: Fri, 17 Jul 2026 22:16:51 +0000 Subject: [PATCH] fix: reject unknown installer arguments (#825) --- README.md | 2 + docs/guides/user-guide.md | 2 + docs/scratchpads/804-install-unknown-flags.md | 86 +++++++++++++++++++ packages/mosaic/framework/defaults/README.md | 2 + .../src/commands/install-arguments.spec.ts | 61 +++++++++++++ tools/install.sh | 25 +++++- 6 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 docs/scratchpads/804-install-unknown-flags.md create mode 100644 packages/mosaic/src/commands/install-arguments.spec.ts diff --git a/README.md b/README.md index ffb6025..8651460 100644 --- a/README.md +++ b/README.md @@ -349,6 +349,8 @@ bash tools/install.sh --yes # Non-interactive, accept all defaults bash tools/install.sh --no-auto-launch # Skip auto-launch of wizard ``` +The installer rejects unrecognized flags or positional arguments before making changes and prints the supported-option usage. + ## Contributing ```bash diff --git a/docs/guides/user-guide.md b/docs/guides/user-guide.md index e2e82e5..370bb8a 100644 --- a/docs/guides/user-guide.md +++ b/docs/guides/user-guide.md @@ -183,6 +183,8 @@ non-interactive use: --no-auto-launch # Skip auto-launch of wizard after install ``` +Unrecognized flags or positional arguments fail before installation starts and print the supported-option usage. + Or if installed globally: ```bash diff --git a/docs/scratchpads/804-install-unknown-flags.md b/docs/scratchpads/804-install-unknown-flags.md new file mode 100644 index 0000000..d2f9e46 --- /dev/null +++ b/docs/scratchpads/804-install-unknown-flags.md @@ -0,0 +1,86 @@ +# Issue #804 — fail closed on unknown installer arguments + +## Objective + +Implement Part 1 of Gitea issue #804 only: `tools/install.sh` must reject every unrecognized flag or argument with an actionable STDERR error and nonzero exit before installation starts. + +## Scope and constraints + +- Preserve all currently recognized options and behavior, including `-y` and `--ref `. +- No positional arguments are currently accepted by the parser. +- Do not add `--next`, `MOSAIC_NEXT`, prerelease routing, or any Part 2 behavior. +- TDD is mandatory: add and observe a failing process-level regression test before changing `tools/install.sh`. +- Worker lifecycle ends after branch push, PR creation, and coordinator notification; do not merge or close #804. +- Existing launcher-owned changes in `.mosaic/orchestrator/mission.json` and `.mosaic/orchestrator/session.lock` are out of scope and must not be committed. + +## Requirements and acceptance criteria + +- Unknown input names the offending argument on STDERR. +- STDERR includes a short installer usage hint. +- Exit status is nonzero. +- The installer does not invoke npm or otherwise proceed into installation. +- Existing recognized flags remain unchanged. + +## Plan + +1. Add a process-level Vitest regression using the installer test location under `packages/mosaic/src/commands/`. +2. Run the focused test and record the expected RED failure. +3. Commit the RED test as `test(#804): ...`. +4. Replace the parser catch-all with a fail-closed STDERR error and usage hint. +5. Update concise installer-facing documentation without introducing prerelease behavior. +6. Run focused tests, shell syntax validation, package tests, lint, typecheck, and format checks. +7. Run independent review tooling and remediate findings. +8. Commit as `fix(#804): ...`, queue-guard, push, open a PR containing `Closes #804.`, notify the coordinator, and exit. + +## Budget + +- No explicit token cap supplied. +- Working estimate: 8K tokens; narrow two-file behavior/test change plus concise docs and delivery gates. + +## Progress + +- 2026-07-17: Loaded mission state, issue #804, delivery/QA/documentation rails, and relevant TDD/Vitest/pnpm/Gitea skills. +- 2026-07-17: Confirmed the parser has no legitimate positional arguments and currently drops all unmatched input via `*) shift ;;`. +- 2026-07-17: Installed locked workspace dependencies with a worktree-local pnpm store; no lockfile changes. +- 2026-07-17: Added the process-level unknown-argument regression with an isolated `$HOME` and npm shim. +- 2026-07-17: Replaced the silent catch-all with STDERR error + usage output and exit 2 before preflight or installation. +- 2026-07-17: Initial Codex code review found an unknown option could still be consumed as the `--ref` value. Added a second RED reproducer, then rejected option-shaped/missing `--ref` values without changing valid `--ref ` behavior. The review's launcher-state note is handled by excluding both `.mosaic/orchestrator/` files from commits. +- 2026-07-17: Updated README, user guide, and packaged framework README with the fail-closed argument contract. No API, auth, admin, sitemap/navigation, or publishing surface changed. + +## Verification + +- RED: `pnpm --filter @mosaicstack/mosaic exec vitest run src/commands/install-arguments.spec.ts` — expected failure: installer exited `0` instead of nonzero at the exit-status assertion; confirms the test reproduces the silent-drop defect before production changes. +- Remediation RED: the added `--cli --ref --bogus` case exited `0`, proving `--ref` could swallow an unknown option before the guard was added. +- GREEN: `pnpm --filter @mosaicstack/mosaic exec vitest run src/commands/install-arguments.spec.ts src/commands/install-heading.spec.ts` — 2 files, 3 tests passed. +- Situational process check: unknown positional input exited 2, named the input on STDERR, printed usage, and did not call the npm shim. +- `bash -n tools/install.sh` — passed. +- Bare `--ref` process check — exited 2 with `Missing value for --ref` and usage. +- `pnpm --filter @mosaicstack/mosaic test` — 69 files, 1,287 tests passed; framework shell checks passed. The first attempt lacked generated `dist/cli.js`; `pnpm --filter @mosaicstack/mosaic build` restored the required test precondition and the full rerun passed. +- `pnpm lint` — 23/23 tasks passed. +- `pnpm typecheck` — 42/42 tasks passed. +- `pnpm format:check` — passed. +- Codex code re-review against `origin/main` — `approve`, 0 blockers/should-fix/suggestions. +- Codex security re-review against `origin/main` — risk `none`, 0 findings. + +## Acceptance evidence + +| Criterion | Evidence | +| --- | --- | +| Unknown input is named on STDERR | Process-level Vitest assertions for `--bogus`, including after `--ref` | +| Short usage hint is printed on STDERR | Vitest usage regex + manual process output | +| Exit is nonzero | Vitest status assertions and manual exit 2 | +| Installation does not proceed | Isolated npm shim marker remains absent | +| Recognized behavior is preserved | Parser cases are unchanged except validation of malformed `--ref`; full Mosaic package suite passed | +| Part 2 is excluded | No `--next`, `MOSAIC_NEXT`, dist-tag, or prerelease routing changes | + +## Documentation checklist + +- Current canonical `docs/PRD.md` remains unchanged; issue #804 and the coordinator brief supply this bounded defect's acceptance contract. +- Updated installer behavior in root README, user guide, and packaged framework README in the same logical change set. +- API/OpenAPI, auth/permissions, admin operations, developer architecture, sitemap/navigation, and external publishing are not affected. +- Scratchpad remains under `docs/scratchpads/`; no root-hygiene changes. + +## Risks and blockers + +- Part 2 remains owner-gated under #805 and is intentionally excluded. +- No implementation blocker remains. Independent coordinator RoR, CI, merge, and issue closure remain pending after worker handoff. diff --git a/packages/mosaic/framework/defaults/README.md b/packages/mosaic/framework/defaults/README.md index 629c0bf..ecffb6a 100644 --- a/packages/mosaic/framework/defaults/README.md +++ b/packages/mosaic/framework/defaults/README.md @@ -177,6 +177,8 @@ bash tools/install.sh --cli # npm CLI only (skip framework) bash tools/install.sh --ref v1.0 # Install from a specific git ref ``` +The installer rejects unrecognized flags or positional arguments before making changes and prints the supported-option usage. + ## Universal Skills The installer syncs skills from `mosaic/agent-skills` into `~/.config/mosaic/skills/`, then links each skill into runtime directories. diff --git a/packages/mosaic/src/commands/install-arguments.spec.ts b/packages/mosaic/src/commands/install-arguments.spec.ts new file mode 100644 index 0000000..203b667 --- /dev/null +++ b/packages/mosaic/src/commands/install-arguments.spec.ts @@ -0,0 +1,61 @@ +import { spawnSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { chmod, mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +const INSTALLER_PATH = fileURLToPath(new URL('../../../../tools/install.sh', import.meta.url)); + +async function runInstaller(args: string[]): Promise<{ + status: number | null; + stderr: string; + npmCalled: boolean; +}> { + const home = await mkdtemp(join(tmpdir(), 'mosaic-installer-args-')); + const bin = join(home, 'bin'); + const npmMarker = join(home, 'npm-called'); + + try { + await mkdir(bin); + const npmShim = join(bin, 'npm'); + await writeFile(npmShim, `#!/bin/sh\n: > "${npmMarker}"\n`); + await chmod(npmShim, 0o755); + + const result = spawnSync('bash', [INSTALLER_PATH, ...args], { + encoding: 'utf8', + env: { + ...process.env, + HOME: home, + MOSAIC_NO_COLOR: '1', + PATH: `${bin}:${process.env.PATH ?? ''}`, + }, + }); + + return { + status: result.status, + stderr: result.stderr, + npmCalled: existsSync(npmMarker), + }; + } finally { + await rm(home, { recursive: true, force: true }); + } +} + +function expectUnknownArgument(result: Awaited>): void { + expect(result.status).not.toBe(0); + expect(result.stderr).toContain('Unknown argument: --bogus'); + expect(result.stderr).toMatch(/Usage: .*install\.sh/); + expect(result.npmCalled).toBe(false); +} + +describe('installer arguments', () => { + it('rejects an unknown argument before installation starts', async () => { + expectUnknownArgument(await runInstaller(['--cli', '--bogus'])); + }); + + it('does not let --ref consume an unknown option', async () => { + expectUnknownArgument(await runInstaller(['--cli', '--ref', '--bogus'])); + }); +}); diff --git a/tools/install.sh b/tools/install.sh index 62c07a8..7692517 100755 --- a/tools/install.sh +++ b/tools/install.sh @@ -61,17 +61,38 @@ if [[ "${MOSAIC_DEV:-0}" == "1" ]]; then FLAG_DEV=true fi +installer_usage() { + printf 'Usage: install.sh [--check] [--framework] [--cli] [--ref ] [--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