fix: reject unknown installer arguments (#825)
This commit was merged in pull request #825.
This commit is contained in:
@@ -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
|
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
|
## Contributing
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
|
|||||||
@@ -183,6 +183,8 @@ non-interactive use:
|
|||||||
--no-auto-launch # Skip auto-launch of wizard after install
|
--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:
|
Or if installed globally:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
|
|||||||
86
docs/scratchpads/804-install-unknown-flags.md
Normal file
86
docs/scratchpads/804-install-unknown-flags.md
Normal file
@@ -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 <branch>`.
|
||||||
|
- 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 <branch>` 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.
|
||||||
@@ -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
|
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
|
## Universal Skills
|
||||||
|
|
||||||
The installer syncs skills from `mosaic/agent-skills` into `~/.config/mosaic/skills/`, then links each skill into runtime directories.
|
The installer syncs skills from `mosaic/agent-skills` into `~/.config/mosaic/skills/`, then links each skill into runtime directories.
|
||||||
|
|||||||
61
packages/mosaic/src/commands/install-arguments.spec.ts
Normal file
61
packages/mosaic/src/commands/install-arguments.spec.ts
Normal file
@@ -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<ReturnType<typeof runInstaller>>): 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']));
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -61,17 +61,38 @@ if [[ "${MOSAIC_DEV:-0}" == "1" ]]; then
|
|||||||
FLAG_DEV=true
|
FLAG_DEV=true
|
||||||
fi
|
fi
|
||||||
|
|
||||||
|
installer_usage() {
|
||||||
|
printf 'Usage: install.sh [--check] [--framework] [--cli] [--ref <branch>] [--dev] [--yes|-y] [--no-auto-launch] [--uninstall]\n' >&2
|
||||||
|
}
|
||||||
|
|
||||||
while [[ $# -gt 0 ]]; do
|
while [[ $# -gt 0 ]]; do
|
||||||
case "$1" in
|
case "$1" in
|
||||||
--check) FLAG_CHECK=true; shift ;;
|
--check) FLAG_CHECK=true; shift ;;
|
||||||
--framework) FLAG_CLI=false; shift ;;
|
--framework) FLAG_CLI=false; shift ;;
|
||||||
--cli) FLAG_FRAMEWORK=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 ;;
|
--dev) FLAG_DEV=true; shift ;;
|
||||||
--yes|-y) FLAG_YES=true; shift ;;
|
--yes|-y) FLAG_YES=true; shift ;;
|
||||||
--no-auto-launch) FLAG_NO_AUTO_LAUNCH=true; shift ;;
|
--no-auto-launch) FLAG_NO_AUTO_LAUNCH=true; shift ;;
|
||||||
--uninstall) FLAG_UNINSTALL=true; shift ;;
|
--uninstall) FLAG_UNINSTALL=true; shift ;;
|
||||||
*) shift ;;
|
*)
|
||||||
|
printf 'Error: Unknown argument: %s\n' "$1" >&2
|
||||||
|
installer_usage
|
||||||
|
exit 2
|
||||||
|
;;
|
||||||
esac
|
esac
|
||||||
done
|
done
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user