fix(#804): reject unknown installer arguments
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
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
|
||||||
|
|||||||
@@ -43,12 +43,44 @@ Implement Part 1 of Gitea issue #804 only: `tools/install.sh` must reject every
|
|||||||
- 2026-07-17: Confirmed the parser has no legitimate positional arguments and currently drops all unmatched input via `*) shift ;;`.
|
- 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: 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: 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
|
## 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.
|
- 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
|
## Risks and blockers
|
||||||
|
|
||||||
- Part 2 remains owner-gated under #805 and is intentionally excluded.
|
- Part 2 remains owner-gated under #805 and is intentionally excluded.
|
||||||
- Test harness must isolate `$HOME` and shim npm so the pre-fix installer cannot mutate operator state or contact the registry.
|
- 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.
|
||||||
|
|||||||
@@ -8,34 +8,54 @@ import { describe, expect, it } from 'vitest';
|
|||||||
|
|
||||||
const INSTALLER_PATH = fileURLToPath(new URL('../../../../tools/install.sh', import.meta.url));
|
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', () => {
|
describe('installer arguments', () => {
|
||||||
it('rejects an unknown argument before installation starts', async () => {
|
it('rejects an unknown argument before installation starts', async () => {
|
||||||
const home = await mkdtemp(join(tmpdir(), 'mosaic-installer-args-'));
|
expectUnknownArgument(await runInstaller(['--cli', '--bogus']));
|
||||||
const bin = join(home, 'bin');
|
});
|
||||||
const npmMarker = join(home, 'npm-called');
|
|
||||||
|
|
||||||
try {
|
it('does not let --ref consume an unknown option', async () => {
|
||||||
await mkdir(bin);
|
expectUnknownArgument(await runInstaller(['--cli', '--ref', '--bogus']));
|
||||||
const npmShim = join(bin, 'npm');
|
|
||||||
await writeFile(npmShim, `#!/bin/sh\n: > "${npmMarker}"\n`);
|
|
||||||
await chmod(npmShim, 0o755);
|
|
||||||
|
|
||||||
const result = spawnSync('bash', [INSTALLER_PATH, '--cli', '--bogus'], {
|
|
||||||
encoding: 'utf8',
|
|
||||||
env: {
|
|
||||||
...process.env,
|
|
||||||
HOME: home,
|
|
||||||
MOSAIC_NO_COLOR: '1',
|
|
||||||
PATH: `${bin}:${process.env.PATH ?? ''}`,
|
|
||||||
},
|
|
||||||
});
|
|
||||||
|
|
||||||
expect(result.status).not.toBe(0);
|
|
||||||
expect(result.stderr).toContain('Unknown argument: --bogus');
|
|
||||||
expect(result.stderr).toMatch(/Usage: .*install\.sh/);
|
|
||||||
expect(existsSync(npmMarker)).toBe(false);
|
|
||||||
} finally {
|
|
||||||
await rm(home, { recursive: true, force: true });
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -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