From af627e758399bdbe5b07ad50ee73f32440fe36df Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Thu, 16 Jul 2026 17:30:19 -0500 Subject: [PATCH] fix(fleet): harden #791 upgrade rollback against find/reset failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second- and third-round independent-review reliability fixes on the keep-mode upgrade rollback path, plus accurate abort messaging. All fixed red-first with self-verifying controls in the rollback gate. Round 2 (blockers A/B, should-fix C): - install.sh: `trap 'restore_snapshot; exit 1' ERR INT TERM` so an INT/TERM mid-sync terminates instead of resuming past the interrupt and reporting success (a bash signal handler that only returns does not terminate). - manifest.{ts,sh}: reject a degenerate [framework] section whose entries are all empty or bare-dot (`/`, `./`, `.`, `..`) — it passed the non-empty guard yet yielded zero usable globs, silently resolving everything to operator. Parity via a shared `[^/.]` usable-glob test; TS throws ManifestError. - finalize.ts: classify the sync-abort message — a ManifestError is a pre-sync validation abort ("no files were changed"); any other error may be partial. Round 3 (blockers D1, D2): - install.sh: enumerate framework files with a checked temp file (_scan_or_die) instead of `< <(find …)` — process substitution discards find's exit status, so an EACCES/I/O failure mid-scan would truncate the file list yet leave the loop exiting 0, committing a partial upgrade as success (ERR trap never fires). - install.sh: guard the `rm -rf; mkdir -p` target reset inside restore_snapshot — a bare reset failing under set -e exits silently after partial deletion, never printing the snapshot-recovery pointer. Now checked like the cp -a restore: on failure it preserves the snapshot and tells the operator where. Tests: rollback gate 14→28 (Parts C/D/E with disabled-guard controls); new finalize-sync-abort.spec.ts (3). No secret value is ever emitted; snapshots stay 0700. Gates green: typecheck, lint, format:check, full mosaic vitest 1094, HARD GATE 193, rollback 28, migration 21. Refs #791 Co-Authored-By: Claude Opus 4.8 --- .woodpecker/ci.yml | 11 +- .../791-upgrade-config-protection.md | 91 +++++ packages/mosaic/framework/install.sh | 105 +++++- .../mosaic/framework/tools/_lib/manifest.sh | 40 ++- .../scripts/test-upgrade-manifest-guard.sh | 80 ++++- .../quality/scripts/test-upgrade-rollback.sh | 319 ++++++++++++++++++ .../src/framework/manifest-parity.spec.ts | 95 +++++- .../mosaic/src/framework/manifest.spec.ts | 64 ++++ packages/mosaic/src/framework/manifest.ts | 66 +++- .../src/stages/finalize-sync-abort.spec.ts | 136 ++++++++ packages/mosaic/src/stages/finalize.ts | 22 +- 11 files changed, 1003 insertions(+), 26 deletions(-) create mode 100644 packages/mosaic/framework/tools/quality/scripts/test-upgrade-rollback.sh create mode 100644 packages/mosaic/src/stages/finalize-sync-abort.spec.ts diff --git a/.woodpecker/ci.yml b/.woodpecker/ci.yml index c40d151..e3c1959 100644 --- a/.woodpecker/ci.yml +++ b/.woodpecker/ci.yml @@ -44,14 +44,19 @@ steps: # Blocking gate (#791): a framework upgrade must never write or delete an # operator-owned path. The HARD GATE proves an unanticipated operator sentinel - # survives a keep-mode reseed byte-identical (both rsync + cp paths); the - # migration matrix pins the v2→v3 contract-file semantics. Pure bash, no - # node_modules — runs early alongside sanitization. + # survives a keep-mode reseed byte-identical (with rsync present AND absent — + # keep mode is a single cp-based path that must not depend on rsync), and that a + # corrupt/empty/missing manifest aborts fail-closed leaving operator files + # untouched (B2/B3). The rollback gate proves a mid-sync failure is rolled back + # from the pre-update snapshot (B1). The migration matrix pins the v2→v3 + # contract-file semantics. Pure bash, no node_modules — runs early alongside + # sanitization. upgrade-guard: image: *node_image commands: - apk add --no-cache bash rsync - bash packages/mosaic/framework/tools/quality/scripts/test-upgrade-manifest-guard.sh + - bash packages/mosaic/framework/tools/quality/scripts/test-upgrade-rollback.sh - bash packages/mosaic/framework/tools/quality/scripts/test-install-migration.sh typecheck: diff --git a/docs/scratchpads/791-upgrade-config-protection.md b/docs/scratchpads/791-upgrade-config-protection.md index 21a7e32..4a5f39a 100644 --- a/docs/scratchpads/791-upgrade-config-protection.md +++ b/docs/scratchpads/791-upgrade-config-protection.md @@ -123,3 +123,94 @@ must survive upgrade. Two coupled deliverables landed in PR1: WITH the carve-out → deny-wins → operator, unprunable (GREEN). manifest.spec.ts 21/21 (was 18). - Gates all green: typecheck ✓ lint ✓ format:check ✓ · full mosaic vitest 1069 passed · HARD GATE 58/58 · migration 21/21. Committing FORWARD on the branch (NOT rebasing 34e55d4a out from under review). + +### MS-LEAD REQUEST CHANGES @ 0a5e703a → B1/B2/B3 fixed red-first (2026-07-16) +MS-LEAD returned REQUEST CHANGES (routed merge-blockers satisfied; 2 CRITICAL reliability defects from +the commissioned independent review). Fixed forward on the branch, red-first: +- **B1 (CRITICAL) — dead ERR trap.** install.sh had `set -euo pipefail` (no `-E`), so the + `trap restore_snapshot ERR` never fired for a failure inside sync_framework_keep() (function body) — + a mid-sync abort left a half-written target with NO rollback. Fix: `set -Eeuo pipefail` (errtrace) + + disarm the trap at the top of restore_snapshot() to prevent re-entrancy. New gate + `test-upgrade-rollback.sh`: injects a mid-sync `cp` EACCES (read-only divergent framework file); + Part A asserts the shipped installer rolls back (restore message fires AND target byte-identical to + pre-upgrade); Part B control strips `-E` and asserts the rollback message does NOT fire (dead trap) — + self-verifying red→green. 7/7. +- **B2/B3 (CRITICAL) — empty/unreadable/malformed manifest divergence.** Pre-fix: TS `parseManifest('')` + returned `{framework:[],operator:[]}` (NO throw) → silent no-op "Installation complete"; bash aborted + fragilely (the `_manifest_compile` `"${MANIFEST_OPERATOR[@]:-}"` artifact returned 1 with no message) + AND the CLI dispatch swallowed manifest_load's rc (no `|| exit`) so `resolve` exited 0 resolving + everything operator. Fix (fail-loud + identical both langs): + * TS `parseManifest`: throw on zero framework entries; `loadManifest`: wrap read error → + "Cannot read framework manifest …". + * bash `manifest_load`: explicit unreadable guard (`[[ ! -r ]]`) + zero-`[framework]` guard, both loud + stderr + return 1; `_manifest_compile` gets explicit `return 0` (kills the empty-array artifact); + CLI dispatch `manifest_load … || exit 1`. + * `finalize.ts`: wrap syncFramework → `spin.stop('Framework sync aborted …')` + rethrow (never falls + through to "Installation complete"). + Tests: manifest.spec.ts +5 fail-closed (empty/comment-only/operator-only/empty-section/missing); + manifest-parity.spec.ts +7 failure-mode parity (both reject empty/comment-only/operator-only/ + empty-section/entry-before-header/unknown-header/missing — TS throws, bash CLI exits non-zero+stderr); + HARD GATE +4 end-to-end fail-closed matrices (empty/operator-only/malformed/missing → abort non-zero, + manifest error surfaced, every operator sentinel byte-identical). RED proven by reverting + manifest.ts+manifest.sh to HEAD → 12 new tests fail; restore → 40/40 green. +- **Non-blocking addressed.** MEDIUM install.sh:222 find-empty now warns on a real failure instead of + blanket `|| true`. LOW: corrected the "both destructive paths rsync vs cp" overstatement in the HARD + GATE header + cp-fallback comment + ci.yml (keep mode is a single cp-based path; the rsync-present vs + -absent runs prove rsync-independence). `.pre-constitution.bak` triage: single-shot backup is + intentional (reconcile_framework_files backs up once), no change. +- Gates: typecheck ✓ lint ✓ format:check ✓ · full mosaic vitest 1081 passed (was 1069, +12) · HARD GATE + 118/118 (was 58) · rollback 7/7 (new) · migration 21/21. No --no-verify. Rollback test wired into + ci.yml upgrade-guard. Committing FORWARD (no rebase of 34e55d4a/0a5e703a). + +### Codex round 2 (pre-push self-review) → blockers A/B + should-fix C fixed red-first (2026-07-16) +Before committing round 1 I re-ran codex on the change set; it surfaced two fresh reliability defects +and one messaging defect on the SAME rollback/manifest path. Fixed forward, red-first: +- **Blocker-A (CRITICAL) — signal trap resumed instead of terminating.** A bash INT/TERM handler that + merely `restore_snapshot` (returns) does NOT terminate the script — execution RESUMES past the + interrupt, cleans the snapshot and reports success, leaving a partial post-interrupt update. Fix: + `trap 'restore_snapshot; exit 1' ERR INT TERM` so both the errtrace (ERR) and signal (INT/TERM) paths + exit non-zero. Rollback test Part C: a `cp` shim that `kill -TERM $PPID` mid-sync then succeeds (so + set -e never fires and only the signal path governs) → asserts abort non-zero + restore fires + does + NOT print "file phase complete"; control strips `exit 1` and asserts the buggy resume-to-success. +- **Blocker-B (CRITICAL) — degenerate `[framework]` section resolved everything operator.** A manifest + whose framework entries are all empty / bare-dot (`/`, `./`, `.`, `..`) passed the non-empty guard yet + yielded zero usable globs → nothing is framework → a keep-mode sync silently no-ops (bash resolved + `operator`, exit 0). Fix (both langs, parity): reject when no entry has a char other than `/`/`.` — + TS `isUsableFrameworkGlob` = `/[^/.]/.test(normalizeRel(glob))`, throws `ManifestError`; bash mirror + loops `[[ "$(_manifest_norm "$_g")" =~ [^/.] ]]`, loud stderr + return 1. Tests: manifest.spec.ts + `it.each(['/','./','.','..','/\n./'])` throw; parity +3 `expectBothReject` (root-slash/dot-slash/ + bare-dot). RED: reverting the guard makes `[framework]\n/` resolve `operator` exit 0. +- **Should-fix-C — misleading abort message.** finalize.ts printed one generic "may be partially + applied" for every sync failure. A `ManifestError` is a PRE-sync validation abort (manifest is + validated before any copy) → nothing was written; conflating it with a mid-copy failure misdirects + recovery. Fix: introduce `ManifestError` (exported from manifest.ts, thrown by every fail-closed + parse/load path), and classify in finalize.ts — ManifestError → "no files were changed"; any other → + "may be partially applied". New co-located `finalize-sync-abort.spec.ts` (3 tests) asserts both + branches re-throw the original error + the correct message, and that config writes are never reached. + RED proven by collapsing the classification → the ManifestError test fails. +- Gates: typecheck ✓ lint ✓ format:check ✓ · full mosaic vitest 1094 (was 1081, +3 finalize-abort; + manifest specs already counted) · HARD GATE 193/193 · rollback 14/14 · migration 21/21. + +### Codex round 3 (pre-push self-review) → blockers D1/D2 fixed red-first (2026-07-16) +Re-ran codex again; it found two more rollback-path gaps `set -E` cannot catch. Fixed forward, red-first: +- **Blocker-D1 (CRITICAL) — `find` scan failures swallowed by process substitution.** Both the overlay + copy and the scoped prune consumed `< <(find … -print0)`. Bash does NOT propagate the producer's exit + status to the `while`, so an EACCES/I/O failure mid-scan truncates the file list yet leaves the loop + exiting 0 → a partial upgrade commits and reports success; the ERR/restore trap never fires. Fix: + `_scan_or_die` runs `find … -print0 > "$tmp"` to completion, checks its status, and returns non-zero + (→ ERR trap → restore) on failure; both loops now read from the checked temp file. Rollback test + Part D: a `find` shim that fails every `-print0` scan → shipped installer aborts non-zero + restores + + emits "Could not enumerate framework files" + target byte-identical; control neuters the `# D1-GUARD` + `return 1` → find failure swallowed, upgrade wrongly reports "file phase complete", no rollback. +- **Blocker-D2 (CRITICAL) — silent `set -e` exit on a failed target reset.** restore_snapshot did a bare + `rm -rf "$TARGET_DIR"; mkdir -p "$TARGET_DIR"` (trap disarmed, under set -e). If `rm`/`mkdir` fails — + possibly after `rm` deleted part of the target — the script exits immediately, skipping the cp AND the + recovery pointer, leaving a half-removed target and an orphaned snapshot the operator can't locate. + Fix: `if ! rm -rf … || ! mkdir -p …; then fail "Snapshot restore could not reset … preserved at: + $SNAPSHOT_DIR — copy it back …"; return 1; fi` (tested like the cp -a check; snapshot NOT deleted). + Rollback test Part E: cp-poison triggers restore + an `rm` shim fails `rm -rf ` → shipped + emits the recovery pointer, the named snapshot dir survives, secret value never leaked; control deletes + the recovery line → operator gets no pointer. RED: reverting D1+D2 → 7 shipped/control assertions fail. +- Gates: typecheck ✓ lint ✓ format:check ✓ · full mosaic vitest 1094 · HARD GATE 193/193 · + rollback 28/28 (was 14, +14 for D1/D2 with controls) · migration 21/21. shellcheck clean on new lines. + No --no-verify. Committing FORWARD (no rebase of 34e55d4a/0a5e703a). diff --git a/packages/mosaic/framework/install.sh b/packages/mosaic/framework/install.sh index 6275f7f..e176ff5 100755 --- a/packages/mosaic/framework/install.sh +++ b/packages/mosaic/framework/install.sh @@ -1,5 +1,10 @@ #!/usr/bin/env bash -set -euo pipefail +# -E (errtrace): the ERR trap must propagate INTO functions and command +# substitutions. Without it the `trap restore_snapshot ERR` set below is dead +# code for any failure inside sync_framework_keep() (its whole body runs in a +# function) — a mid-sync failure would abort with a half-written target and NO +# rollback (#791 B1). Keep -E first so every later function inherits the trap. +set -Eeuo pipefail # ─── Mosaic Framework Installer ────────────────────────────────────────────── # @@ -62,14 +67,45 @@ step() { echo -e "\n${BOLD}$1${RESET}"; } SNAPSHOT_DIR="" make_snapshot() { is_existing_install || return 0 + # mktemp -d creates the dir 0700 — the snapshot (which mirrors operator config, + # possibly including secrets) is never world-readable. SNAPSHOT_DIR="$(mktemp -d "${TMPDIR:-/tmp}/mosaic-snapshot-XXXXXX")" - cp -a "$TARGET_DIR/." "$SNAPSHOT_DIR/" 2>/dev/null || true + # The snapshot MUST be complete: restore rebuilds the target from it, so a + # partial capture (unreadable file, disk-full, I/O error) would silently + # discard whatever it missed. If cp -a cannot copy the whole tree, abort NOW — + # before the restore trap is armed and before anything is mutated. Fail closed + # rather than proceed with a snapshot we cannot trust (#791 blocker-2). + if ! cp -a "$TARGET_DIR/." "$SNAPSHOT_DIR/"; then + fail "Could not capture a complete pre-upgrade snapshot of $TARGET_DIR — aborting before any changes were made (fail-closed)." + rm -rf "$SNAPSHOT_DIR"; SNAPSHOT_DIR="" + exit 1 + fi } restore_snapshot() { + # Disarm the trap first: restore runs under `set -e`, and a non-zero step + # inside it must not re-enter this handler (errtrace makes ERR fire in + # functions now). One restore attempt, then let the script exit non-zero. + trap - ERR INT TERM [[ -n "$SNAPSHOT_DIR" && -d "$SNAPSHOT_DIR" ]] || return 0 fail "Install interrupted/failed — restoring previous state from snapshot" - rm -rf "$TARGET_DIR"; mkdir -p "$TARGET_DIR" - cp -a "$SNAPSHOT_DIR/." "$TARGET_DIR/" 2>/dev/null || true + # Reset the target before rebuilding from the snapshot — but CHECK it. Under + # `set -e` (trap already disarmed) a bare `rm -rf; mkdir -p` that fails would + # exit the whole script immediately, after `rm` may have deleted part of the + # target, WITHOUT ever printing the recovery pointer below — the operator would + # be left with a half-removed target and no idea the snapshot survives in /tmp. + # Test the reset explicitly (like the cp -a below), and on failure keep the + # snapshot and tell the operator where it is (#791 blocker-D2). + if ! rm -rf "$TARGET_DIR" || ! mkdir -p "$TARGET_DIR"; then + fail "Snapshot restore could not reset $TARGET_DIR. Your previous configuration is preserved at: $SNAPSHOT_DIR — copy it back into $TARGET_DIR manually." + return 1 + fi + # Surface an incomplete restore instead of swallowing it: the snapshot is the + # last good copy, so if cp cannot fully rebuild the target we must NOT delete + # the snapshot — point the operator at it for manual recovery (#791 blocker-2). + if ! cp -a "$SNAPSHOT_DIR/." "$TARGET_DIR/"; then + fail "Snapshot restore did not complete cleanly. Your previous configuration is preserved at: $SNAPSHOT_DIR — copy it back into $TARGET_DIR manually." + return 1 + fi } cleanup_snapshot() { [[ -n "$SNAPSHOT_DIR" && -d "$SNAPSHOT_DIR" ]] && rm -rf "$SNAPSHOT_DIR"; SNAPSHOT_DIR=""; } @@ -174,7 +210,9 @@ sync_framework() { if [[ "$INSTALL_MODE" == "keep" ]]; then # The `mosaic update` path. Manifest-driven, never-deleting-outside-framework: # operator config is structurally protected (#791). No rsync --delete here. - manifest_load + # The manifest is already loaded+validated in main() BEFORE the snapshot/trap + # (a fail-closed manifest must abort without ever restoring over operator + # files — see the pre-flight in main, #791 blocker-1). sync_framework_keep return fi @@ -184,15 +222,36 @@ sync_framework() { sync_framework_overwrite } +# Enumerate a NUL-delimited file list via `find` into the temp file $1, failing +# CLOSED if find errors. We capture to a checked file instead of consuming +# `< <(find …)` directly because a process substitution discards the producer's +# exit status: an EACCES/I/O failure partway through a scan would truncate the +# list yet leave the reading `while` loop exiting 0, so a partial upgrade would +# commit and report success and the ERR/restore trap would never fire. Running +# find to completion first, then checking its status, turns that silent +# truncation into a fail-closed abort that the restore trap can act on (#791 +# blocker-D1). $1 after the shift is the scan root — named in the error. +_scan_or_die() { + local out="$1"; shift + if ! find "$@" -print0 > "$out"; then + fail "Could not enumerate framework files under '$1' — aborting before committing an incomplete sync (fail-closed)." + return 1 # D1-GUARD + fi +} + # Keep-mode sync: create/refresh framework-owned files and prune only retired # framework files inside shipped framework subtrees. Operator-owned and unknown # paths (fail-safe default) are never written and never deleted — the #791 HARD # GATE. Single code path (no rsync) so it is byte-for-byte parity-testable. sync_framework_keep() { - local src="$SOURCE_DIR" dst="$TARGET_DIR" abs rel root + local src="$SOURCE_DIR" dst="$TARGET_DIR" abs rel root list # 1) Overlay copy — every framework-owned source file, refreshed only when its # bytes changed (no mtime churn on unchanged files, never on operator files). + # The source scan is captured fail-closed (#791 blocker-D1): a find failure + # aborts the sync (→ ERR trap → restore) rather than silently truncating it. + list="$(mktemp)" + _scan_or_die "$list" "$src" -type f || { rm -f "$list"; return 1; } while IFS= read -r -d '' abs; do rel="${abs#"$src"/}" case "$rel" in @@ -202,24 +261,34 @@ sync_framework_keep() { if [[ -f "$dst/$rel" ]] && cmp -s "$abs" "$dst/$rel"; then continue; fi [[ "$rel" == */* ]] && mkdir -p "$dst/${rel%/*}" cp "$abs" "$dst/$rel" - done < <(find "$src" -type f -print0) + done < "$list" + rm -f "$list" # 2) Scoped prune — within each shipped framework subtree root, remove # framework-owned target files the current source no longer ships. Operator # carve-outs (e.g. tools/_lib/credentials.json) resolve to operator and are # skipped; unknown paths resolve to operator too — both are unreachable here. + # Each subtree scan is captured fail-closed for the same reason as the copy. while IFS= read -r root; do [[ -n "$root" && -d "$dst/$root" ]] || continue + list="$(mktemp)" + _scan_or_die "$list" "$dst/$root" -type f || { rm -f "$list"; return 1; } while IFS= read -r -d '' abs; do rel="${abs#"$dst"/}" case "$rel" in *.pre-constitution.bak) continue ;; esac [[ -f "$src/$rel" ]] && continue # still shipped manifest_is_framework "$rel" || continue rm -f "$abs" - done < <(find "$dst/$root" -type f -print0) + done < "$list" + rm -f "$list" # Drop framework dirs left empty by the prune (never touches a dir that still - # holds an operator file — those are never emptied). - find "$dst/$root" -type d -empty -delete 2>/dev/null || true + # holds an operator file — those are never emptied). A genuine find failure + # (unreadable dir) is surfaced as a warning rather than silently swallowed; + # the "directory not empty" races we tolerate are ignored via -delete's own + # rc, not by hiding stderr — so a real error is still visible to the operator. + if ! find "$dst/$root" -type d -empty -delete 2>/dev/null; then + warn "prune: could not fully sweep empty framework dirs under $root (left as-is)" + fi done < <(manifest_subtree_roots) } @@ -307,9 +376,23 @@ else ok "Install mode: overwrite" fi +# Pre-flight (keep mode): load + validate the framework manifest BEFORE taking a +# snapshot or arming the restore trap. A fail-closed manifest (missing / empty / +# malformed) must abort here WITHOUT deleting or restoring over operator files — +# the snapshot/restore path exists only for a genuine mid-sync mutation failure, +# not for a validation failure that has touched nothing yet (#791 blocker-1). +if [[ "$INSTALL_MODE" == "keep" ]]; then + manifest_load +fi + # Snapshot before any destructive file operation; restore on interrupt/failure. +# The trap MUST exit after restoring: a bash INT/TERM handler that merely returns +# does NOT terminate the script — execution would resume past the interrupt, +# clear the snapshot, and report success, leaving a partial post-interrupt update +# (#791 blocker-A). `restore_snapshot; exit 1` guarantees a non-zero exit for +# both the errtrace (ERR) and signal (INT/TERM) paths. make_snapshot -trap 'restore_snapshot' ERR INT TERM +trap 'restore_snapshot; exit 1' ERR INT TERM sync_framework diff --git a/packages/mosaic/framework/tools/_lib/manifest.sh b/packages/mosaic/framework/tools/_lib/manifest.sh index 874fbb2..20b79cc 100644 --- a/packages/mosaic/framework/tools/_lib/manifest.sh +++ b/packages/mosaic/framework/tools/_lib/manifest.sh @@ -110,6 +110,11 @@ _manifest_compile() { local g for g in "${MANIFEST_FRAMEWORK[@]:-}"; do [[ -n "$g" ]] && _manifest_compile_one "$g" F; done for g in "${MANIFEST_OPERATOR[@]:-}"; do [[ -n "$g" ]] && _manifest_compile_one "$g" O; done + # Explicit success: an empty operator array makes the final `[[ -n "" ]] && …` + # short-circuit to rc 1, which would otherwise become this function's (and + # manifest_load's) return code — a spurious failure (#791 B2). Never rely on + # the last loop's exit status here. + return 0 } # Load + compile the manifest. Rejects a malformed file the same way @@ -117,6 +122,13 @@ _manifest_compile() { manifest_load() { local file="${1:-}" [[ -n "$file" ]] || file="$(_manifest_default_root)/framework-manifest.txt" + # Fail CLOSED on a missing/unreadable manifest. Without this, `done < "$file"` + # aborts on a raw redirection error with no explanation; downstream that reads + # as "no framework paths" and an upgrade could no-op silently (#791 B2/B3). + if [[ ! -r "$file" ]]; then + echo "manifest: cannot read manifest file: $file — refusing to sync (fail-closed)." >&2 + return 1 + fi MANIFEST_FRAMEWORK=() MANIFEST_OPERATOR=() local section="" line @@ -139,7 +151,30 @@ manifest_load() { MANIFEST_OPERATOR+=("$line") fi done < "$file" + # An empty or comment-only manifest defines NO framework-owned paths. Treating + # that as valid would make every path resolve operator and an upgrade prune + # nothing / write nothing — a silent no-op indistinguishable from success. + # Fail loud instead, mirroring parseManifest()'s throw in manifest.ts (#791 B2). + if [[ ${#MANIFEST_FRAMEWORK[@]} -eq 0 ]]; then + echo "manifest: no [framework] entries in $file — refusing to sync (empty or malformed manifest)." >&2 + return 1 + fi + # An entry like `/` or `./` normalizes to nothing and compiles to a glob that + # matches no path — so a manifest whose only [framework] entries are degenerate + # passes the count guard above but leaves the framework matcher empty: every + # path resolves operator, the exact silent no-op we fail closed against. Require + # at least one entry with a real (non-slash, non-dot) character. Mirrors + # parseManifest()'s `isUsableFrameworkGlob` `/[^/.]/` test in manifest.ts (#791 blocker-B). + local _g _usable=0 + for _g in "${MANIFEST_FRAMEWORK[@]:-}"; do + if [[ "$(_manifest_norm "$_g")" =~ [^/.] ]]; then _usable=1; break; fi + done + if [[ "$_usable" -eq 0 ]]; then + echo "manifest: no usable [framework] entries in $file (every entry is empty or a bare dot segment) — refusing to sync (malformed manifest)." >&2 + return 1 + fi _manifest_compile + return 0 } # Fork-free: does $1 (a mosaic-home-relative path) match an operator glob? @@ -196,7 +231,10 @@ manifest_subtree_roots() { # CLI dispatch — only when executed directly, never when sourced. if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then set -o pipefail - manifest_load "${MANIFEST_FILE:-}" + # Propagate a fail-closed manifest_load (missing/empty/malformed) as a non-zero + # exit instead of continuing to resolve against empty compiled arrays — that is + # what lets the parity test assert bash and TS reject the same bad inputs (#791 B2). + manifest_load "${MANIFEST_FILE:-}" || exit 1 cmd="${1:-}" case "$cmd" in resolve) manifest_resolve "${2:?path required}" ;; diff --git a/packages/mosaic/framework/tools/quality/scripts/test-upgrade-manifest-guard.sh b/packages/mosaic/framework/tools/quality/scripts/test-upgrade-manifest-guard.sh index 5f2883d..f221e68 100644 --- a/packages/mosaic/framework/tools/quality/scripts/test-upgrade-manifest-guard.sh +++ b/packages/mosaic/framework/tools/quality/scripts/test-upgrade-manifest-guard.sh @@ -9,8 +9,16 @@ # must still update, and a retired framework file inside a shipped subtree must # still be pruned. No operator secret value may appear in installer output. # -# Runs the whole matrix twice: once with rsync available, once forcing the -# cp-based fallback (both destructive paths in install.sh must obey the manifest). +# Keep mode is a SINGLE code path (sync_framework_keep, a manifest-driven cp +# overlay + scoped prune — no rsync). The matrix still runs twice, once with +# rsync on PATH and once with it hidden, to prove the keep path is genuinely +# rsync-independent: it must obey the manifest identically whether or not rsync +# happens to be installed (rsync --delete is only ever used by overwrite mode, +# which has no operator state to protect). +# +# It also runs a fail-closed matrix (#791 B2/B3): an empty, operator-only, +# malformed, or missing manifest must ABORT the upgrade loudly and leave every +# operator path untouched — never silently no-op to "complete". # # Usage: bash test-upgrade-manifest-guard.sh set -uo pipefail @@ -131,6 +139,59 @@ run_matrix() { rm -rf "$H" "$E" "$OUT" } +# Fail-closed matrix (#791 B2/B3 + blocker-1): run install.sh from a COPY of the +# framework so the shipped manifest can be corrupted. Every corruption must abort +# the upgrade non-zero with a manifest error, leaving all operator sentinels +# byte-identical AND on the SAME inode. The inode check is the load-bearing part: +# manifest validation is hoisted BEFORE make_snapshot/the restore trap, so a bad +# manifest must abort without ever snapshotting, deleting, and restoring the +# target. Were validation still armed under the ERR trap, restore_snapshot would +# rm -rf + rebuild the target — same bytes but a NEW inode (broken hard links, +# changed ctime), which a content-only hash would miss (#791 blocker-1). +run_failclosed() { + local label="$1" mutate="$2" + local SRC H E OUT rc rel before_hash after_hash before_ino after_ino key + SRC=$(mktemp -d); H=$(mktemp -d); E=$(mktemp -d); OUT=$(mktemp) + cp -a "$FW/." "$SRC/" + case "$mutate" in + empty) : > "$SRC/framework-manifest.txt" ;; + operator-only) printf '[operator]\nSOUL.md\n*.local.md\n' > "$SRC/framework-manifest.txt" ;; + malformed) printf 'stray.md\n[framework]\nguides/**\n' > "$SRC/framework-manifest.txt" ;; + degenerate) printf '[framework]\n/\n./\n[operator]\nSOUL.md\n' > "$SRC/framework-manifest.txt" ;; + missing) rm -f "$SRC/framework-manifest.txt" ;; + esac + + seed_home "$H" + for rel in "${OPERATOR_SENTINELS[@]}"; do + key=$(echo "$rel" | tr / _) + sha256sum "$H/$rel" | awk '{print $1}' > "$E/$key.hash" + stat -c '%i' "$H/$rel" > "$E/$key.ino" + done + + MOSAIC_HOME="$H" MOSAIC_INSTALL_MODE=keep MOSAIC_SYNC_ONLY=1 bash "$SRC/install.sh" >"$OUT" 2>&1 + rc=$? + + chk "[fail-closed:$label] upgrade aborts non-zero" "[ '$rc' -ne 0 ]" + chk "[fail-closed:$label] refuses loudly with a manifest error" \ + "grep -qi 'manifest' '$OUT'" + for rel in "${OPERATOR_SENTINELS[@]}"; do + key=$(echo "$rel" | tr / _) + before_hash=$(cat "$E/$key.hash") + after_hash=$(sha256sum "$H/$rel" 2>/dev/null | awk '{print $1}') + chk "[fail-closed:$label] operator sentinel untouched: $rel" \ + "[ -n '$after_hash' ] && [ '$before_hash' = '$after_hash' ]" + before_ino=$(cat "$E/$key.ino") + after_ino=$(stat -c '%i' "$H/$rel" 2>/dev/null) + chk "[fail-closed:$label] operator sentinel not deleted/recreated (inode stable): $rel" \ + "[ -n '$after_ino' ] && [ '$before_ino' = '$after_ino' ]" + done + chk "[fail-closed:$label] operator secret value absent from output" \ + "! grep -q '$SECRET' '$OUT'" + + chmod -R u+w "$SRC" "$H" 2>/dev/null || true + rm -rf "$SRC" "$H" "$E" "$OUT" +} + echo "#791 upgrade manifest guard (HARD GATE):" # 1) rsync path (if available on this host). @@ -140,15 +201,24 @@ else echo " · rsync not installed — skipping rsync-path matrix" fi -# 2) cp fallback path — hide rsync behind a scratch PATH so install.sh takes the -# find/rm+cp branch. Provide the coreutils the fallback needs. +# 2) rsync-absent path — hide rsync behind a scratch PATH. Keep mode never calls +# rsync, so this must resolve identically to run (1); it proves the keep path +# does not silently depend on rsync being installed. (Provide the coreutils the +# installer needs on the stripped PATH.) FBIN=$(mktemp -d) for t in bash cp find mktemp rm mkdir chmod cmp sed grep cat dirname basename stat sha256sum awk tr; do p=$(command -v "$t" 2>/dev/null) && ln -s "$p" "$FBIN/$t" done -run_matrix "cp-fallback" env "PATH=$FBIN" +run_matrix "rsync-absent" env "PATH=$FBIN" rm -rf "$FBIN" +# 3) fail-closed matrix (#791 B2/B3) — corrupt the shipped manifest four ways. +run_failclosed "empty-manifest" empty +run_failclosed "operator-only" operator-only +run_failclosed "malformed-manifest" malformed +run_failclosed "degenerate-framework" degenerate +run_failclosed "missing-manifest" missing + echo echo "RESULT: $pass passed, $fail failed" [ "$fail" -eq 0 ] diff --git a/packages/mosaic/framework/tools/quality/scripts/test-upgrade-rollback.sh b/packages/mosaic/framework/tools/quality/scripts/test-upgrade-rollback.sh new file mode 100644 index 0000000..d0b91ba --- /dev/null +++ b/packages/mosaic/framework/tools/quality/scripts/test-upgrade-rollback.sh @@ -0,0 +1,319 @@ +#!/usr/bin/env bash +# test-upgrade-rollback.sh — the #791 B1 regression gate. +# +# A keep-mode upgrade takes a pre-update snapshot and installs an ERR/INT/TERM +# trap that restores it if the sync aborts midway (install.sh: make_snapshot + +# `trap restore_snapshot`). That trap is only reached if `set -E` (errtrace) is +# active — otherwise a failure INSIDE sync_framework_keep() (which runs entirely +# in a function) never fires the trap, and the upgrade aborts leaving a +# half-written target with NO rollback. This test proves: +# +# Part A (the gate): the shipped installer rolls back a mid-sync failure — +# the restore message fires, the corrupted file is put +# back, AND the whole target is byte-identical to its +# pre-upgrade state. +# Part B (the control): the SAME installer with `-E` stripped does NOT roll back +# (dead trap) — the mid-sync corruption survives, proving +# errtrace is load-bearing. If anyone removes `set -E`, +# Part A goes red. +# +# The mid-sync failure is injected with a PATH-shadowing `cp` shim rather than +# file permissions. The earlier 0400/EACCES approach was NOT portable: Woodpecker +# runs steps as root (node:24-alpine has no USER directive), and root overwrites a +# 0400 file, so the failure never fired and this gate silently passed (#791 +# blocker-3). The shim fails deterministically for one framework-owned +# destination regardless of uid, and — like a real interrupted cp (disk-full +# mid-write) — leaves a partially-written target behind, so rollback has real +# damage to undo and the control has real damage to expose. +# +# Usage: bash test-upgrade-rollback.sh +set -uo pipefail + +FW="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd)" # packages/mosaic/framework +INSTALL="$FW/install.sh" +ORIG_PATH="$PATH" + +# The `-E`-stripped control installer must live INSIDE $FW: install.sh derives +# SOURCE_DIR from its own path and `source`s $SOURCE_DIR/tools/_lib/manifest.sh, +# so a copy anywhere else aborts at the source line before ever reaching the sync +# loop — which would make the control a false negative. A root dotfile is +# operator-owned (unknown→operator), so the sync loop skips it. Clean up on exit. +STRIPPED="$FW/.install-rollback-control.tmp.sh" +NOEXIT="$FW/.install-noexit-control.tmp.sh" +D1CTRL="$FW/.install-d1guard-control.tmp.sh" +D2CTRL="$FW/.install-d2guard-control.tmp.sh" +rm -f "$STRIPPED" "$NOEXIT" "$D1CTRL" "$D2CTRL" +trap 'rm -f "$STRIPPED" "$NOEXIT" "$D1CTRL" "$D2CTRL"' EXIT + +pass=0; fail=0 +chk() { if eval "$2"; then echo " ✓ $1"; pass=$((pass + 1)); else echo " ✗ $1"; fail=$((fail + 1)); fi; } + +SECRET='SUPER-SECRET-TOKEN-do-not-log-b1' +# A framework-owned file the shim fails the copy of. The seeded target holds GOOD +# bytes; source ships different bytes, so sync_framework_keep() attempts the copy +# and the shim intercepts it. Root-level framework files sort before guides/, so +# several framework files are already refreshed when the copy reaches this one. +POISON_REL='guides/E2E-DELIVERY.md' +GOOD='GOOD-REFERENCE-CONTENT-pre-upgrade-b1' +GARBAGE='PARTIAL-WRITE-GARBAGE-mid-sync-b1' + +# A `cp` shim: for the poisoned destination, simulate an interrupted copy — write +# partial garbage to the target, then fail — otherwise delegate to the real cp +# (resolved via the ORIGINAL PATH so make_snapshot/restore still work). +make_cp_shim() { + local dir="$1" + cat > "$dir/cp" < "\$dest" 2>/dev/null || true + exit 1 ;; +esac +exec env PATH="$ORIG_PATH" cp "\$@" +SHIM + chmod +x "$dir/cp" +} + +# A `find` shim that fails every enumeration scan (`-print0`) as if it hit an +# EACCES/I/O error partway — it emits the real (here: complete) list first, then +# exits non-zero, exactly the class of failure a `< <(find …)` process +# substitution silently swallows. All non-`-print0` finds (e.g. the -delete +# sweep) delegate to the real find on the original PATH. Used to prove #791 +# blocker-D1: the shipped installer must honor find's exit status and roll back. +make_find_fail_shim() { + local dir="$1" + cat > "$dir/find" <` (the restore's target +# reset) and delegates every other rm to the real one. Used to prove #791 +# blocker-D2: when the target reset inside restore_snapshot fails, the installer +# must emit the manual-recovery pointer (snapshot path) instead of exiting +# silently under `set -e`. FAIL_RM_TARGET is exported into the installer env. +make_rm_fail_shim() { + local dir="$1" + cat > "$dir/rm" <<'SHIM' +#!/usr/bin/env bash +last="${@: -1}" +if [ -n "${FAIL_RM_TARGET:-}" ] && [ "$last" = "$FAIL_RM_TARGET" ]; then + exit 1 +fi +exec env PATH="$ORIG_PATH_FOR_RM" rm "$@" +SHIM + chmod +x "$dir/rm" +} + +seed_home() { + local H="$1" + mkdir -p "$H/agents" "$H/tools/_lib" "$H/memory" "$H/guides" + printf '# persona\n' > "$H/SOUL.md" # recognized install → keep mode + snapshot + printf 'MODEL=opus\n' > "$H/agents/coder0.conf" + printf '# operator memory\n' > "$H/memory/note.md" + printf 'TOKEN=%s\n' "$SECRET" > "$H/tools/_lib/credentials.json" + echo 3 > "$H/.framework-version" + # Good pre-upgrade bytes; source ships different bytes, so cp is attempted. + printf '%s' "$GOOD" > "$H/$POISON_REL" +} + +# Run one keep-mode upgrade against $1=installer, seeding a fresh home and a +# byte-for-byte reference of the pre-upgrade state, with the cp shim first on +# PATH. Echoes: "\t\t\t". +run_upgrade() { + local installer="$1" shim_maker="${2:-make_cp_shim}" H REF OUT SHIM rc + H=$(mktemp -d); REF=$(mktemp -d); OUT=$(mktemp); SHIM=$(mktemp -d) + seed_home "$H" + env PATH="$ORIG_PATH" cp -a "$H/." "$REF/" # pre-upgrade reference (real cp) + "$shim_maker" "$SHIM" + set +e + PATH="$SHIM:$ORIG_PATH" \ + MOSAIC_HOME="$H" MOSAIC_INSTALL_MODE=keep MOSAIC_SYNC_ONLY=1 bash "$installer" >"$OUT" 2>&1 + rc=$? + set -e 2>/dev/null || true + rm -rf "$SHIM" + printf '%s\t%s\t%s\t%s\n' "$rc" "$OUT" "$REF" "$H" +} + +echo "#791 upgrade rollback (B1 regression gate):" + +# ── Part A: the shipped installer must roll back a mid-sync failure ─────────── +IFS=$'\t' read -r rcA OUTA REFA HA < <(run_upgrade "$INSTALL") + +chk "[shipped] upgrade aborts non-zero on the injected mid-sync failure" \ + "[ '$rcA' -ne 0 ]" +chk "[shipped] restore_snapshot fires (rollback message present)" \ + "grep -q 'restoring previous state from snapshot' '$OUTA'" +chk "[shipped] the corrupted file is restored to its pre-upgrade bytes" \ + "[ \"\$(cat '$HA/$POISON_REL')\" = '$GOOD' ]" +chk "[shipped] target rolled back byte-identical to pre-upgrade state" \ + "diff -r '$REFA' '$HA' >/dev/null 2>&1" +chk "[shipped] operator secret value absent from installer output" \ + "! grep -q '$SECRET' '$OUTA'" + +# ── Part B: control — strip `-E`, the trap is dead, no rollback happens ─────── +# Proves errtrace is what makes the trap reachable. If `set -E` is ever removed +# from install.sh, Part A's rollback assertions fail exactly like this control. +sed 's/^set -Eeuo pipefail/set -euo pipefail/' "$INSTALL" > "$STRIPPED" +chk "[control] the -E strip actually changed the installer" \ + "! cmp -s '$INSTALL' '$STRIPPED'" + +IFS=$'\t' read -r rcB OUTB REFB HB < <(run_upgrade "$STRIPPED") +chk "[control] without -E the upgrade still aborts non-zero" \ + "[ '$rcB' -ne 0 ]" +# The load-bearing, deterministic proof of B1: without errtrace the ERR trap +# never fires for a failure inside sync_framework_keep(), so no rollback runs. +chk "[control] without -E the rollback message does NOT fire (dead trap)" \ + "! grep -q 'restoring previous state from snapshot' '$OUTB'" +chk "[control] without -E the mid-sync corruption survives (no rollback)" \ + "[ \"\$(cat '$HB/$POISON_REL')\" = '$GARBAGE' ]" + +# ── Part C: an INT/TERM interrupt must terminate, not resume (blocker-A) ────── +# A bash signal trap that merely returns lets the script continue past the +# interrupt — restoring the snapshot, then resuming the sync and reporting +# success. We inject a SIGTERM mid-sync with a cp that SUCCEEDS (so set -e never +# fires and ONLY the signal path governs), and assert the shipped installer +# restores AND exits without reporting success. The control strips `exit 1` from +# the trap and shows the buggy resume-to-success. +make_term_shim() { + local dir="$1" + cat > "$dir/cp" </dev/null # signal install.sh; the copy still succeeds + exec env PATH="$ORIG_PATH" cp "\$@" ;; +esac +exec env PATH="$ORIG_PATH" cp "\$@" +SHIM + chmod +x "$dir/cp" +} + +# Run one keep-mode upgrade with the SIGTERM shim. Echoes "\t\t". +run_signal_upgrade() { + local installer="$1" H OUT SHIM rc + H=$(mktemp -d); OUT=$(mktemp); SHIM=$(mktemp -d) + seed_home "$H" + make_term_shim "$SHIM" + set +e + PATH="$SHIM:$ORIG_PATH" \ + MOSAIC_HOME="$H" MOSAIC_INSTALL_MODE=keep MOSAIC_SYNC_ONLY=1 bash "$installer" >"$OUT" 2>&1 + rc=$? + set -e 2>/dev/null || true + rm -rf "$SHIM" + printf '%s\t%s\t%s\n' "$rc" "$OUT" "$H" +} + +IFS=$'\t' read -r rcC OUTC HC < <(run_signal_upgrade "$INSTALL") +chk "[signal] SIGTERM mid-sync aborts non-zero (trap exits, does not resume)" \ + "[ '$rcC' -ne 0 ]" +chk "[signal] restore_snapshot fires on the interrupt" \ + "grep -q 'restoring previous state from snapshot' '$OUTC'" +chk "[signal] does NOT resume to report sync success after the interrupt" \ + "! grep -q 'file phase complete' '$OUTC'" + +# Control: strip `exit 1` from the signal trap → the handler returns, the script +# resumes past the interrupt and wrongly reports success. In $FW so SOURCE_DIR resolves. +sed "s/trap 'restore_snapshot; exit 1' ERR INT TERM/trap 'restore_snapshot' ERR INT TERM/" \ + "$INSTALL" > "$NOEXIT" +chk "[control] the exit-strip actually changed the installer" \ + "! cmp -s '$INSTALL' '$NOEXIT'" +IFS=$'\t' read -r _rcD OUTD HD < <(run_signal_upgrade "$NOEXIT") +chk "[control] without 'exit 1' the trap resumes and reports sync success (the bug)" \ + "grep -q 'file phase complete' '$OUTD'" + +# ── Part D: a failed source/prune `find` scan must abort + roll back (D1) ───── +# A `< <(find …)` process substitution discards find's exit status, so an +# EACCES/I/O failure mid-scan would truncate the file list yet leave the reading +# loop exiting 0 — a partial upgrade committed and reported as success, with the +# ERR/restore trap never firing. The shipped installer captures the scan into a +# checked temp file (_scan_or_die) and aborts on failure. We inject a `find` that +# fails every `-print0` scan and assert the shipped installer rolls back. +IFS=$'\t' read -r rcE OUTE REFE HE < <(run_upgrade "$INSTALL" make_find_fail_shim) +chk "[find-fail] a failing framework scan aborts the upgrade non-zero" \ + "[ '$rcE' -ne 0 ]" +chk "[find-fail] restore_snapshot fires on the aborted scan" \ + "grep -q 'restoring previous state from snapshot' '$OUTE'" +chk "[find-fail] the abort is a fail-closed enumeration error (not a silent truncation)" \ + "grep -q 'Could not enumerate framework files' '$OUTE'" +chk "[find-fail] target rolled back byte-identical to pre-upgrade state" \ + "diff -r '$REFE' '$HE' >/dev/null 2>&1" + +# Control: neuter the D1 guard (turn its `return 1` into a no-op) so a find +# failure is swallowed exactly as `< <(find …)` would — the scan appears to +# succeed and the upgrade reports completion with NO rollback. +sed 's/return 1 # D1-GUARD/: # D1-GUARD-DISABLED/' "$INSTALL" > "$D1CTRL" +chk "[control] the D1-guard strip actually changed the installer" \ + "! cmp -s '$INSTALL' '$D1CTRL'" +IFS=$'\t' read -r _rcF OUTF REFF HF < <(run_upgrade "$D1CTRL" make_find_fail_shim) +chk "[control] with the D1 guard disabled the find failure is swallowed (no rollback)" \ + "! grep -q 'restoring previous state from snapshot' '$OUTF'" +chk "[control] with the D1 guard disabled the upgrade wrongly reports success" \ + "grep -q 'file phase complete' '$OUTF'" + +# ── Part E: a failed target reset inside restore must not exit silently (D2) ── +# restore_snapshot resets the target (`rm -rf; mkdir -p`) before rebuilding from +# the snapshot. Under `set -e` (trap disarmed) a bare reset that fails would exit +# the whole script immediately — after `rm` may have deleted part of the target — +# WITHOUT printing where the snapshot lives. We trigger a rollback (cp poison) AND +# fail the target reset (rm shim), then assert the shipped installer emits the +# manual-recovery pointer and preserves the snapshot. +run_rmfail_upgrade() { + local installer="$1" H OUT SHIM rc + H=$(mktemp -d); OUT=$(mktemp); SHIM=$(mktemp -d) + seed_home "$H" + make_cp_shim "$SHIM" # poison cp → triggers the abort + restore + make_rm_fail_shim "$SHIM" # rm -rf fails → exercises the D2 reset guard + set +e + PATH="$SHIM:$ORIG_PATH" ORIG_PATH_FOR_RM="$ORIG_PATH" FAIL_RM_TARGET="$H" \ + MOSAIC_HOME="$H" MOSAIC_INSTALL_MODE=keep MOSAIC_SYNC_ONLY=1 bash "$installer" >"$OUT" 2>&1 + rc=$? + set -e 2>/dev/null || true + rm -rf "$SHIM" + printf '%s\t%s\t%s\n' "$rc" "$OUT" "$H" +} + +IFS=$'\t' read -r rcG OUTG HG < <(run_rmfail_upgrade "$INSTALL") +chk "[reset-fail] a failed target reset still aborts non-zero" \ + "[ '$rcG' -ne 0 ]" +chk "[reset-fail] the manual-recovery pointer is emitted (not a silent set -e exit)" \ + "grep -q 'Snapshot restore could not reset' '$OUTG'" +chk "[reset-fail] the recovery message points at a preserved snapshot dir" \ + "grep -q 'preserved at: .*mosaic-snapshot' '$OUTG'" +SNAP_E="$(grep -o '/[^ ]*mosaic-snapshot[^ ]*' "$OUTG" | head -1)" +chk "[reset-fail] the named snapshot directory actually survives for recovery" \ + "[ -n '$SNAP_E' ] && [ -d '$SNAP_E' ]" +chk "[reset-fail] operator secret value never appears in installer output" \ + "! grep -q '$SECRET' '$OUTG'" + +# Control: delete the D2 recovery line so a failed reset returns non-zero with NO +# operator pointer — the observable defect (half-reset target, snapshot orphaned +# in /tmp with no path told to the operator). Proves the message is load-bearing. +sed '/Snapshot restore could not reset/d' "$INSTALL" > "$D2CTRL" +chk "[control] the D2-recovery strip actually changed the installer" \ + "! cmp -s '$INSTALL' '$D2CTRL'" +IFS=$'\t' read -r _rcH OUTH HH < <(run_rmfail_upgrade "$D2CTRL") +chk "[control] without the D2 recovery line the operator gets no snapshot pointer" \ + "! grep -q 'Snapshot restore could not reset' '$OUTH'" +[ -n "${SNAP_E:-}" ] && rm -rf "$SNAP_E" +# Reap any snapshot the reset-fail runs left in /tmp (reset failed → never cleaned). +grep -o '/[^ ]*mosaic-snapshot[^ ]*' "$OUTH" 2>/dev/null | head -1 | while read -r s; do rm -rf "$s"; done + +# Cleanup ($STRIPPED / $NOEXIT / $D1CTRL / $D2CTRL are also removed by the EXIT trap). +for d in "$HA" "$REFA" "$HB" "$REFB" "$HC" "$HD" "$HE" "$REFE" "$HF" "$REFF" "$HG" "$HH"; do rm -rf "$d"; done +rm -f "$OUTA" "$OUTB" "$OUTC" "$OUTD" "$OUTE" "$OUTF" "$OUTG" "$OUTH" \ + "$STRIPPED" "$NOEXIT" "$D1CTRL" "$D2CTRL" + +echo +echo "RESULT: $pass passed, $fail failed" +[ "$fail" -eq 0 ] diff --git a/packages/mosaic/src/framework/manifest-parity.spec.ts b/packages/mosaic/src/framework/manifest-parity.spec.ts index e58a47e..f2915f6 100644 --- a/packages/mosaic/src/framework/manifest-parity.spec.ts +++ b/packages/mosaic/src/framework/manifest-parity.spec.ts @@ -1,6 +1,6 @@ import { afterAll, describe, it, expect } from 'vitest'; -import { execFileSync } from 'node:child_process'; -import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -55,6 +55,19 @@ function bashSubtreeRoots(): string[] { .filter((s) => s.length > 0); } +/** + * Drive the bash resolver CLI against a manifest file and report how it exited. + * A fail-closed manifest must make the CLI exit non-zero with a message on + * stderr — never exit 0 having silently resolved everything to operator. + */ +function bashCli(manifestFile: string): { status: number; stderr: string } { + const res = spawnSync('bash', [MANIFEST_SH, 'resolve', 'CONSTITUTION.md'], { + encoding: 'utf-8', + env: { ...process.env, MANIFEST_FILE: manifestFile }, + }); + return { status: res.status ?? -1, stderr: res.stderr ?? '' }; +} + // Paths spanning every ownership class: framework single-files, framework // subtrees, operator declared trees, operator carve-out inside a framework // subtree, local overlays, and deliberately UNANTICIPATED paths (fail-safe). @@ -250,3 +263,81 @@ describe.skipIf(!hasBash)('bash ↔ TS manifest format-edge parity (§6.1, Decis ]); }); }); + +/** + * Failure-mode parity (#791 B2/B3). A bad manifest is the dangerous case: if the + * two resolvers DISAGREED on rejection — one throwing while the other quietly + * resolved everything to operator — an upgrade could fail loud on one code path + * and no-op on the other. So for every malformed/empty/missing manifest, BOTH + * must reject: TS throws, and the bash CLI exits non-zero with a stderr message. + */ +describe.skipIf(!hasBash)('bash ↔ TS manifest failure-mode parity (§6.1, B2/B3)', () => { + const tmp = mkdtempSync(join(tmpdir(), 'mf-failmode-')); + afterAll(() => rmSync(tmp, { recursive: true, force: true })); + + let seq = 0; + function writeFixture(text: string): string { + const file = join(tmp, `bad-manifest-${seq++}.txt`); + writeFileSync(file, text); + return file; + } + + // TS throws AND bash CLI exits non-zero with a non-empty stderr — identical rejection. + function expectBothReject(label: string, manifestFile: string): void { + expect(() => parseManifestFile(manifestFile), `TS accepted ${label}`).toThrow(); + const cli = bashCli(manifestFile); + expect(cli.status, `bash did not exit non-zero for ${label}`).not.toBe(0); + expect(cli.stderr.trim().length, `bash was silent for ${label}`).toBeGreaterThan(0); + } + + // Read the file for the TS side the same way loadManifest does, so both halves + // see identical bytes (loadManifest keys off a directory, not an arbitrary file). + function parseManifestFile(file: string): void { + parseManifest(readFileSync(file, 'utf-8')); + } + + it('both reject a completely empty manifest', () => { + expectBothReject('empty', writeFixture('')); + }); + + it('both reject a comment/blank-only manifest', () => { + expectBothReject('comment-only', writeFixture('# header only\n\n \n')); + }); + + it('both reject an operator-only manifest (zero framework paths)', () => { + expectBothReject('operator-only', writeFixture('[operator]\nSOUL.md\n*.local.md\n')); + }); + + it('both reject a [framework] section with no entries', () => { + expectBothReject('empty-framework-section', writeFixture('[framework]\n[operator]\nSOUL.md\n')); + }); + + it('both reject a [framework] entry that normalizes to an empty glob (/)', () => { + expectBothReject('root-slash-framework', writeFixture('[framework]\n/\n')); + }); + + it('both reject a [framework] entry that normalizes to nothing (./)', () => { + expectBothReject('dot-slash-framework', writeFixture('[framework]\n./\n[operator]\nSOUL.md\n')); + }); + + it('both reject [framework] entries that are only bare dot segments', () => { + expectBothReject('bare-dot-framework', writeFixture('[framework]\n.\n..\n')); + }); + + it('both reject an entry that appears before any section header', () => { + expectBothReject('entry-before-header', writeFixture('stray.md\n[framework]\nguides/**\n')); + }); + + it('both reject an unknown section header', () => { + expectBothReject('unknown-header', writeFixture('[bogus]\nx\n')); + }); + + it('both reject a missing manifest file (fail-closed, not empty result)', () => { + const missing = join(tmp, 'does-not-exist.txt'); + // TS: loadManifest would throw a read error; here read-then-parse throws on read. + expect(() => parseManifestFile(missing)).toThrow(); + const cli = bashCli(missing); + expect(cli.status).not.toBe(0); + expect(cli.stderr.trim().length).toBeGreaterThan(0); + }); +}); diff --git a/packages/mosaic/src/framework/manifest.spec.ts b/packages/mosaic/src/framework/manifest.spec.ts index 5db13bf..bc5ab5a 100644 --- a/packages/mosaic/src/framework/manifest.spec.ts +++ b/packages/mosaic/src/framework/manifest.spec.ts @@ -9,6 +9,7 @@ import { resolveOwnership, frameworkSubtreeRoots, planPrune, + ManifestError, type FrameworkManifest, } from './manifest.js'; @@ -49,6 +50,69 @@ describe('parseManifest', () => { }); }); +// Fail-closed parsing/loading (#791 B2/B3). An empty, comment-only, operator-only, +// or unreadable manifest must NOT resolve to "framework owns nothing" (which would +// make an upgrade a silent no-op). Both must throw so finalizeStage surfaces the +// abort instead of reporting "Installation complete". The bash reader rejects the +// same inputs — asserted for parity in manifest-parity.spec.ts. +describe('parseManifest / loadManifest fail closed on empty or unreadable input', () => { + it('throws on a completely empty manifest', () => { + expect(() => parseManifest('')).toThrow(/no \[framework\] paths/); + }); + + it('throws on a comment- and blank-only manifest (no entries at all)', () => { + expect(() => parseManifest('# just a header comment\n\n \n')).toThrow( + /no \[framework\] paths/, + ); + }); + + it('throws when only an [operator] section is present (zero framework paths)', () => { + expect(() => parseManifest('[operator]\nSOUL.md\n*.local.md\n')).toThrow( + /no \[framework\] paths/, + ); + }); + + it('throws on a [framework] header with no entries beneath it', () => { + expect(() => parseManifest('[framework]\n\n[operator]\nSOUL.md\n')).toThrow( + /no \[framework\] paths/, + ); + }); + + // Degenerate framework entries that pass the length check but normalize to a + // glob matching nothing — the manifest would silently protect the whole tree + // as operator (#791 blocker-B). Both `/` and `./` normalize to '' ; `.`/`..` + // are bare-dot segments. + it.each([['/'], ['./'], ['.'], ['..'], ['/\n./']])( + 'throws when the only [framework] entry (%j) normalizes to nothing usable', + (entry) => { + expect(() => parseManifest(`[framework]\n${entry}\n`)).toThrow( + /no usable \[framework\] paths/, + ); + }, + ); + + it('accepts a wildcard-only framework glob (** is usable)', () => { + expect(() => parseManifest('[framework]\n**\n')).not.toThrow(); + }); + + it('loadManifest throws a clear fail-closed error when the manifest file is missing', () => { + const missingRoot = fileURLToPath(new URL('./__no_such_framework_root__', import.meta.url)); + expect(() => loadManifest(missingRoot)).toThrow(/Cannot read framework manifest/); + }); + + // The distinct error type is what lets finalizeStage tell a pre-sync validation + // abort (nothing written) from a mid-sync filesystem failure (#791 blocker-C). + it('every fail-closed rejection is a ManifestError', () => { + expect(() => parseManifest('')).toThrow(ManifestError); + expect(() => parseManifest('[operator]\nSOUL.md\n')).toThrow(ManifestError); + expect(() => parseManifest('[framework]\n/\n')).toThrow(ManifestError); + expect(() => parseManifest('[bogus]\nx\n')).toThrow(ManifestError); + expect(() => parseManifest('stray.md\n[framework]\n')).toThrow(ManifestError); + const missingRoot = fileURLToPath(new URL('./__no_such_framework_root__', import.meta.url)); + expect(() => loadManifest(missingRoot)).toThrow(ManifestError); + }); +}); + describe('matchGlob', () => { it('matches an exact file', () => { expect(matchGlob('CONSTITUTION.md', 'CONSTITUTION.md')).toBe(true); diff --git a/packages/mosaic/src/framework/manifest.ts b/packages/mosaic/src/framework/manifest.ts index 05e5303..056a03b 100644 --- a/packages/mosaic/src/framework/manifest.ts +++ b/packages/mosaic/src/framework/manifest.ts @@ -15,6 +15,19 @@ import { readFileSync } from 'node:fs'; export type Ownership = 'framework' | 'operator'; +/** + * Thrown when the manifest is missing, empty, or malformed. A distinct type lets + * callers (e.g. finalizeStage) tell a pre-sync validation abort — where NO files + * were touched — apart from a generic mid-sync filesystem failure, and message + * the user accurately (#791 blocker-C). + */ +export class ManifestError extends Error { + constructor(message: string) { + super(message); + this.name = 'ManifestError'; + } +} + export interface FrameworkManifest { /** Globs the updater MAY create/overwrite, and prune only when retired. */ readonly framework: readonly string[]; @@ -49,21 +62,68 @@ export function parseManifest(text: string): FrameworkManifest { continue; } if (line.startsWith('[')) { - throw new Error(`Unknown manifest section header on line ${i + 1}: ${line}`); + throw new ManifestError(`Unknown manifest section header on line ${i + 1}: ${line}`); } if (section === null) { - throw new Error(`Manifest entry before any [section] header on line ${i + 1}: ${line}`); + throw new ManifestError( + `Manifest entry before any [section] header on line ${i + 1}: ${line}`, + ); } (section === 'framework' ? framework : operator).push(line); } + // Fail CLOSED on an empty or comment-only manifest. A manifest with zero + // framework-owned globs would make resolveOwnership() return `operator` for + // every path: an upgrade would prune nothing and refresh nothing — a silent + // no-op indistinguishable from success. Refuse loudly instead, mirroring the + // bash reader's `manifest_load` guard so both halves reject it identically (#791 B2). + if (framework.length === 0) { + throw new ManifestError( + 'Framework manifest defines no [framework] paths — refusing to proceed (empty or malformed manifest).', + ); + } + + // Fail CLOSED on framework entries that normalize to nothing usable. A manifest + // like `[framework]\n/` or `[framework]\n./` passes the length check above but + // every entry normalizes to an empty (or bare-dot) glob that matches no real + // path — so the compiled framework matcher is empty and every path resolves + // `operator`: the same silent no-op as an empty manifest. Require at least one + // entry with a real, non-dot character (the bash reader applies the identical + // `[^/.]` test, so both halves reject these inputs together — #791 blocker-B). + if (!framework.some(isUsableFrameworkGlob)) { + throw new ManifestError( + 'Framework manifest defines no usable [framework] paths (every entry is empty or a bare dot segment) — refusing to proceed (malformed manifest).', + ); + } + return { framework, operator }; } +/** + * A framework glob is usable only if, once normalized, it still contains a + * character other than `/` or `.` — i.e. it names a real path segment or a + * wildcard. `''`, `/`, `./`, `.`, `..` are all unusable (they compile to a glob + * that matches nothing). Kept byte-compatible with the bash `[[ =~ [^/.] ]]` + * test so TS and bash accept/reject exactly the same manifests. + */ +function isUsableFrameworkGlob(glob: string): boolean { + return /[^/.]/.test(normalizeRel(glob)); +} + /** Read and parse the manifest from a framework root directory. */ export function loadManifest(frameworkRoot: string): FrameworkManifest { - const text = readFileSync(`${frameworkRoot}/framework-manifest.txt`, 'utf-8'); + const file = `${frameworkRoot}/framework-manifest.txt`; + let text: string; + try { + text = readFileSync(file, 'utf-8'); + } catch (err) { + // A missing/unreadable manifest must fail closed with a clear message, not a + // raw ENOENT that a caller might mistake for an empty result set (#791 B2/B3). + throw new ManifestError( + `Cannot read framework manifest at ${file}: ${(err as Error).message} — refusing to sync (fail-closed).`, + ); + } return parseManifest(text); } diff --git a/packages/mosaic/src/stages/finalize-sync-abort.spec.ts b/packages/mosaic/src/stages/finalize-sync-abort.spec.ts new file mode 100644 index 0000000..119ba94 --- /dev/null +++ b/packages/mosaic/src/stages/finalize-sync-abort.spec.ts @@ -0,0 +1,136 @@ +/** + * Tests for the framework-sync abort messaging (#791 B2 + blocker-C). + * + * finalizeStage runs `config.syncFramework()` first, inside a try/catch. If the + * sync throws, the wizard must: + * 1. NEVER fall through to "Installation complete" — the error is re-raised so + * the process exits non-zero (#791 B2). + * 2. Classify the failure so recovery advice is accurate (#791 blocker-C): + * - ManifestError → a PRE-sync validation abort; nothing was written, so + * the message states "no files were changed". + * - any other error → may surface mid-copy, so the message must NOT claim + * nothing changed; it warns the state "may be partially applied". + * + * We assert on the spinner's stop() message (the user-visible line) and that the + * original error is re-thrown unchanged in both cases. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import type { WizardState } from '../types.js'; +import type { ConfigService } from '../config/config-service.js'; +import { ManifestError } from '../framework/manifest.js'; + +vi.mock('node:child_process', () => ({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any + spawnSync: vi.fn().mockReturnValue({ status: 0, stdout: '', stderr: '' }), +})); + +vi.mock('../platform/detect.js', () => ({ + getShellProfilePath: () => null, +})); + +import { finalizeStage } from './finalize.js'; + +function makeState(mosaicHome: string): WizardState { + return { + mosaicHome, + sourceDir: mosaicHome, + mode: 'quick', + installAction: 'keep', + soul: { agentName: 'TestBot', communicationStyle: 'direct' }, + user: {}, + tools: {}, + runtimes: { detected: [], mcpConfigured: false }, + selectedSkills: [], + }; +} + +function buildPrompter() { + const stop = vi.fn(); + const update = vi.fn(); + const prompter = { + intro: vi.fn(), + outro: vi.fn(), + note: vi.fn(), + log: vi.fn(), + warn: vi.fn(), + text: vi.fn(), + confirm: vi.fn(), + select: vi.fn(), + multiselect: vi.fn(), + groupMultiselect: vi.fn(), + spinner: vi.fn().mockReturnValue({ update, stop }), + separator: vi.fn(), + }; + return { prompter, stop }; +} + +function makeConfigService(syncFramework: ConfigService['syncFramework']): ConfigService { + return { + readSoul: vi.fn().mockResolvedValue({}), + readUser: vi.fn().mockResolvedValue({}), + readTools: vi.fn().mockResolvedValue({}), + writeSoul: vi.fn().mockResolvedValue(undefined), + writeUser: vi.fn().mockResolvedValue(undefined), + writeTools: vi.fn().mockResolvedValue(undefined), + syncFramework, + get: vi.fn(), + set: vi.fn(), + getSection: vi.fn(), + } as unknown as ConfigService; +} + +describe('finalizeStage — framework sync abort (#791 B2 + blocker-C)', () => { + let tmp: string; + + beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), 'mosaic-sync-abort-')); + }); + + afterEach(() => { + rmSync(tmp, { recursive: true, force: true }); + vi.clearAllMocks(); + }); + + it('re-throws a ManifestError and reports that no files were changed', async () => { + const err = new ManifestError('Framework manifest defines no usable [framework] paths'); + const { prompter, stop } = buildPrompter(); + const config = makeConfigService(vi.fn().mockRejectedValue(err)); + + await expect(finalizeStage(prompter, makeState(tmp), config)).rejects.toBe(err); + + // The abort message must state nothing was written (pre-sync validation). + expect(stop).toHaveBeenCalledWith(expect.stringContaining('no files were changed')); + // It must NOT fall through to a success line. + expect(stop).not.toHaveBeenCalledWith(expect.stringContaining('Installation complete')); + }); + + it('re-throws a non-ManifestError and warns the state may be partially applied', async () => { + const err = new Error('cp: write error mid-sync (disk full)'); + const { prompter, stop } = buildPrompter(); + const config = makeConfigService(vi.fn().mockRejectedValue(err)); + + await expect(finalizeStage(prompter, makeState(tmp), config)).rejects.toBe(err); + + // A generic mid-sync failure must NOT claim nothing changed… + expect(stop).toHaveBeenCalledWith(expect.stringContaining('may be partially applied')); + expect(stop).not.toHaveBeenCalledWith(expect.stringContaining('no files were changed')); + expect(stop).not.toHaveBeenCalledWith(expect.stringContaining('Installation complete')); + }); + + it('does not proceed to config writes when the sync aborts', async () => { + const err = new ManifestError('malformed manifest'); + const { prompter } = buildPrompter(); + const config = makeConfigService(vi.fn().mockRejectedValue(err)); + + await expect(finalizeStage(prompter, makeState(tmp), config)).rejects.toBe(err); + + // writeSoul/writeUser/writeTools are only reached after a successful sync. + expect(config.writeSoul).not.toHaveBeenCalled(); + expect(config.writeUser).not.toHaveBeenCalled(); + expect(config.writeTools).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/mosaic/src/stages/finalize.ts b/packages/mosaic/src/stages/finalize.ts index 4835f6e..95dd503 100644 --- a/packages/mosaic/src/stages/finalize.ts +++ b/packages/mosaic/src/stages/finalize.ts @@ -6,6 +6,7 @@ import type { WizardPrompter } from '../prompter/interface.js'; import type { ConfigService } from '../config/config-service.js'; import type { WizardState } from '../types.js'; import { getShellProfilePath } from '../platform/detect.js'; +import { ManifestError } from '../framework/manifest.js'; function linkRuntimeAssets(mosaicHome: string, skipClaudeHooks: boolean): void { const script = join(mosaicHome, 'bin', 'mosaic-link-runtime-assets'); @@ -160,7 +161,26 @@ export async function finalizeStage( // 1. Sync framework files (before config writes so identity files aren't overwritten) spin.update('Syncing framework files...'); - await config.syncFramework(state.installAction); + try { + await config.syncFramework(state.installAction); + } catch (err) { + // Stop the spinner loudly and re-raise so the process exits non-zero — never + // fall through to "Installation complete" on an aborted sync (#791 B2). + // A ManifestError is a PRE-sync validation abort: the manifest is loaded and + // validated before any file is written, so nothing was touched. Any other + // error can surface AFTER files were partially copied, so we must NOT claim + // "no files were changed" for it — that would misdirect recovery (#791 blocker-C). + if (err instanceof ManifestError) { + spin.stop( + 'Framework sync aborted — the framework manifest is missing, empty, or malformed; no files were changed.', + ); + } else { + spin.stop( + 'Framework sync aborted — the update did not complete and may be partially applied; see the error below.', + ); + } + throw err; + } // 2. Write config files (after sync so they aren't overwritten by source templates) if (state.installAction !== 'keep') {