All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Add `mosaic fleet regen`, a projection-only recovery command that rebuilds each `fleet/agents/<name>.env.generated` from the `roster.yaml` SSOT after an upgrade or partial write leaves the generated projections stale or missing. - Dry-run by default; `--write` applies; `--json` for machine output. Reuses the merged reconciler's projection plumbing (projectRosterV2AgentGeneratedEnv + the generated-env boundary) rather than reimplementing fleet logic. - Structurally NEVER issues a lifecycle/restart call — regen recovers config only; a recordingRunner gate proves no runner invocation ever occurs. - Serializes against agent CRUD and reconcile via BOTH fleet locks (roster.yaml.mutation.lock + roster.yaml.reconcile.lock), acquired mutation-then-reconcile and released in reverse; both are non-blocking `wx` locks that throw on contention, so no deadlock is possible. - Hardens the shared managed-lock helper: ownership-proving tokened lock reused for both locks with per-lock fault labels; init-failure cleanup no longer strands a just-created lock (dev/ino guard, with a persisted-token fallback when the post-create stat itself fails); acquire-unwind surfaces a lock cleanup fault instead of dropping it. - Resolves personas the SAME way reconcile does by forwarding configured rolesDir/overrideDir, so a custom-persona-root deployment cannot have reconcile accept a roster that regen rejects. - Report/output is secrev-safe: paths and counts only, never projected values. - Docs: upgrade-safety-and-recovery runbook + fleet-local-canary note. Tests are TDD red-first with co-located specs (regen spec: 26 tests covering dry-run/write dispositions, the never-restarts gate, all lock regressions, and persona-root wiring). A residual check-then-unlink TOCTOU in the lock release remains (byte-identical to the merged reconcile lock; unreachable within the `wx` writer protocol); its true fix is an fd-held advisory lock adopted by all fleet writers, tracked as a separate follow-up. Part of #791 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
530 lines
43 KiB
Markdown
530 lines
43 KiB
Markdown
# Scratchpad — #791 Upgrade config protection (ms-791 worker lane)
|
||
|
||
**Lane:** web1:ms-791 → reports to MS-LEAD (web1:mosaic-100). Do NOT contact Jason/Mos directly.
|
||
**Worktree:** `/home/hermes/agent-work/stack-agents-dir-791`, branch `feat/791-upgrade-config-protection`
|
||
off `origin/main` `9745bc3f` (verified exact head).
|
||
|
||
## Mission prompt (verbatim intent)
|
||
Protect operator-owned config under `~/.config/mosaic` from framework-upgrade wipes. Ratified
|
||
combination (Mos-approved, do NOT re-litigate): (b) strict ownership separation [PRIMARY] + (a)
|
||
transactional pre-update snapshot [safety net] + (d) regeneration-from-SSOT [recovery]. (c) periodic
|
||
timer DEFERRED. HARD GATE: unit test that an upgrade run touches NO path outside the manifest.
|
||
Design-first: write design doc, send to MS-LEAD, WAIT for confirmation before impl.
|
||
|
||
## Session 1 (2026-07-16) — Phase 1 design
|
||
|
||
### Evidence gathered (wipe mechanism, file/line)
|
||
- `mosaic update` → `update-checker.ts:509` `buildReseedCommand` → `bash install.sh`
|
||
(`MOSAIC_SYNC_ONLY=1`, `MOSAIC_INSTALL_MODE=keep`).
|
||
- Wipe = `packages/mosaic/framework/install.sh:199` `rsync -a --delete` + `PRESERVE_PATHS` denylist
|
||
(`install.sh:47`). cp-fallback `install.sh:223` `find ... -exec rm -rf`.
|
||
- Denylist gaps → WIPED: `agents/*.conf`, `policy/*.md`, `*.local.md`, harvester/SOP,
|
||
`tools/_lib/credentials.json`.
|
||
- Stale comment `update-checker.ts:492` claims `*.local` preserved — PRESERVE_PATHS has no such entry.
|
||
- TS path `file-adapter.ts:157` → `file-ops.ts:66` `syncDirectory` = non-destructive copy-overlay, BUT
|
||
its preserve list (`file-adapter.ts:164`) already DRIFTED from install.sh (missing `fleet/backlog`,
|
||
`fleet/roles.local`). Evidence for single shared manifest SSOT.
|
||
- Existing snapshot (`install.sh:76`) = /tmp, crash-trap only, deleted on success → inadequate; no
|
||
`mosaic restore`.
|
||
- `fleet-reconciler.ts:93,234` already has `regenerate-projections-from-roster` phase separate from
|
||
lifecycle → `mosaic fleet regen` = thin projection-only wrapper (no restart), no FCM-M4/M5 preemption.
|
||
|
||
### Design decisions
|
||
- **(b)** Invert to allow-list: shared `framework/framework-manifest.json` (framework globs +
|
||
operatorReserved carve-outs); resolve per-path, deny-wins; **UNKNOWN ⇒ operator (fail-safe)**.
|
||
Mechanism: drop `--delete`; non-deleting bulk copy + explicit manifest-scoped prune pass (iterate
|
||
framework globs only → operator/unknown structurally unreachable). Pure prune-planner fn for tests.
|
||
- **(a)** Snapshot to `~/.local/state/mosaic/backups/pre-update-<ts>/` 0700/0600, retention N=5,
|
||
post-sync verify+restore, `mosaic restore --list/--from`. No secret values in output.
|
||
- **(d)** `mosaic fleet regen` projection-only, preview-first, never restart.
|
||
- HARD GATE test includes a deliberately-unanticipated operator path to prove fail-safe default.
|
||
- **PR split:** PR1 manifest+guard (root fix, ships alone) → PR2 snapshot/restore (secrev) → PR3
|
||
regen+docs. PR2/PR3 depend on PR1.
|
||
|
||
### Status
|
||
Design doc written: `docs/design/791-upgrade-config-protection.md`. Sent to MS-LEAD.
|
||
|
||
## Session 1 (cont.) — MS-LEAD CONFIRMED → Phase 2 GO
|
||
All 4 asks approved. Binding conditions:
|
||
- TDD tests-first, red-first proof per PR; ≥85% new-code; co-located `*.spec.ts`; never `--no-verify`.
|
||
- HARD GATE test (§2.4, unanticipated sentinel survives byte-identical + mtime unchanged) = MERGE-BLOCKING for PR1.
|
||
- Manifest-completeness test (§6.2) required.
|
||
- Bash+TS read ONE shared `framework-manifest.json`; parity test (§6.1) required (closes #631 drift class).
|
||
- UNKNOWN⇒operator (rule 3) non-negotiable. Keep prune-planner PURE.
|
||
- `fleet regen`: NEVER restart; dry-run default, `--write` to apply; "never issues restart" test mandatory.
|
||
- Independent review every PR; PR2 dedicated secrev.
|
||
- One PR at a time through DAG. Report PR1 exact head + red→green evidence for review commission.
|
||
|
||
### Now: implementing PR1 (manifest + resolver + non-deleting sync + scoped prune + guard tests).
|
||
|
||
## Session 2 (2026-07-16) — PR1 built, tests-first, red→green proven
|
||
|
||
Deviation noted to MS-LEAD in PR: manifest is `framework-manifest.txt` (line-oriented), NOT `.json`.
|
||
Rationale: keep the bash installer free of a python3/jq dependency. The "ONE shared file, parity-
|
||
tested" requirement is honored — `manifest-parity.spec.ts` drives the bash resolver as a subprocess
|
||
and asserts byte-identical ownership vs the TS resolver over 34 probe paths spanning every class.
|
||
|
||
### PR1 artifacts
|
||
- SSOT: `packages/mosaic/framework/framework-manifest.txt` ([framework]/[operator], deny-wins, fail-safe).
|
||
- TS resolver: `src/framework/manifest.ts` (pure: parse/matchGlob/resolveOwnership/frameworkSubtreeRoots/
|
||
planPrune) + `manifest.spec.ts` (18 tests incl. planPrune property test + §6.2 completeness).
|
||
- Bash resolver: `framework/tools/_lib/manifest.sh` (compiled globs → fork-free `manifest_is_framework`;
|
||
CLI `resolve|subtree-roots|classify`). Sourced by install.sh.
|
||
- HARD GATE (§2.4): `framework/tools/quality/scripts/test-upgrade-manifest-guard.sh` — keep-mode reseed,
|
||
10 operator sentinels (incl. unanticipated `unknown-operator-dir/x`, `harvester/sop.md`,
|
||
`fleet/my-fleet.yaml`) survive byte-identical + mtime-unchanged; retired framework file pruned;
|
||
secret value absent from output. RED=31 fail (orig install.sh) → GREEN=48 pass (fixed).
|
||
- install.sh: keep mode now manifest-driven (`sync_framework_keep`, no `--delete`); overwrite unchanged.
|
||
PRESERVE_PATHS denylist deleted.
|
||
- TS sync: `file-ops.syncDirectory` gains `isOperatorOwned` guard; `file-adapter.syncFramework` derives
|
||
it from `loadManifest` — hardcoded (drifted) preservePaths deleted. Fixture uses the REAL manifest.
|
||
- Parity: `manifest-parity.spec.ts` (§6.1) — bash↔TS agree on 34 paths + subtree roots.
|
||
- Migration matrix `test-install-migration.sh`: F6 flipped — `my-fleet.yaml` now MUST survive (fail-safe).
|
||
- CI: new merge-blocking `upgrade-guard` step (`.woodpecker/ci.yml`) runs both bash suites (adds rsync).
|
||
- update-checker.ts reseed comment corrected to the manifest model.
|
||
|
||
### Gates (all green)
|
||
- `pnpm typecheck` ✓ · `pnpm lint` ✓ · `pnpm format:check` ✓
|
||
- Full mosaic vitest: 1062 passed (cli-smoke needs `pnpm build` first — build-artifact dep, not this change).
|
||
- HARD GATE 48/48 · migration 21/21 · parity 3/3 · manifest 18/18 · file-adapter 8/8.
|
||
|
||
### PR opened + reported (2026-07-16)
|
||
- **PR #802** http://git.mosaicstack.dev/mosaicstack/stack/pulls/802 — base `main`@`9745bc3f`,
|
||
head `34e55d4a` (commit `feat(mosaic): manifest-owned upgrade guard…`). 15 files, +1160/-142.
|
||
- Reported PR head + red→green evidence to MS-LEAD (web1:mosaic-100); queued (lead busy).
|
||
Standing by for the independent-review commission at head `34e55d4a`.
|
||
- **TWO items flagged to MS-LEAD for decision (awaiting reply):**
|
||
1. Deviation `.txt` vs `.json` — confirm accept (parity-tested) or convert to `.json`+jq.
|
||
2. `pr-create -i 791` appended `Fixes #791` → would auto-close the tracking issue on PR1 merge
|
||
while PR2/PR3 remain. Recommended edit to `Part of #791`; awaiting go-ahead to patch PR body.
|
||
- DO NOT start PR2/PR3 until PR1 merges (DAG; one PR at a time).
|
||
|
||
### MS-LEAD ruling → #797 ledger-survival sentinel folded into PR1 (2026-07-16)
|
||
MS-LEAD ruled both my decisions: (1) `.txt` format ACCEPTED (parity must be strict/merge-blocking incl.
|
||
format edge cases + negative probe); (2) trailer `Fixes #791`→`Part of #791` APPROVED (patched PR #802
|
||
body via Gitea API — tracking issue no longer auto-closes on PR1 merge). Plus Mos-ELEVATED merge-blocker
|
||
(spec `~/agent-work/planning/epic-796/791-ledger-survival-sentinel-SPEC.md`): #797 Runtime Session Ledger
|
||
must survive upgrade. Two coupled deliverables landed in PR1:
|
||
- (i) Carve-out: `fleet/run/**` was ALREADY an explicit `[operator]` entry — glob matches the spec's
|
||
pinned `fleet/run/**` EXACTLY, so NO divergence to route back to planner-opus. Strengthened its comment
|
||
to name the ledger (`fleet/run/sessions/` events.ndjson + ledger.json) so it is unmistakably load-bearing.
|
||
- (ii) HARD-GATE sentinel: seeded populated ledger (events.ndjson 3 events + ledger.json node+edge+gen,
|
||
0600 under 0700) into test-upgrade-manifest-guard.sh sentinels; asserts byte-identical + mtime-unchanged
|
||
+ dir-perms unchanged. Negative control (retired framework file IS pruned) relabeled explicitly.
|
||
HARD GATE now 58/58 (was 48).
|
||
- Decision-1 parity hardening: format-edge fixtures (comments/blanks/whitespace, duplicate+overlapping
|
||
globs deny-wins, section/glob-ordering independence) + explicit UNKNOWN→operator negative probe, driven
|
||
through BOTH resolvers via MANIFEST_FILE override. Parity 7/7 (was 3).
|
||
- RED-FIRST honesty note: the bash ledger sentinel stays GREEN even against the pre-fix installer (the
|
||
ledger was incidentally safe from the rsync --delete bug; overall pre-fix run 30/58 as expected). The
|
||
carve-out's TRUE load-bearing value (deny-wins if framework ownership ever broadens to `fleet/**`) is
|
||
isolated by a dedicated resolver-seam red→green in manifest.spec.ts: WITHOUT `fleet/run/**` operator
|
||
entry + hypothetical `fleet/**` framework → ledger resolves framework and planPrune DELETES it (RED);
|
||
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 <TARGET>` → 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).
|
||
|
||
## Session 3 (2026-07-16) — PR1 MERGED, starting PR2 (durable snapshot + restore + secrev)
|
||
PR1 (#802) squash-merged → main `32a0ffba`; issue #791 stays open (3-PR DAG umbrella). Independent Opus
|
||
adversarial/security review APPROVED at head `af627e75` (Gitea RoR cmt 17892); lead ran rollback 28/28 +
|
||
HARD GATE 193/193 green; CI #1877 green. PR2 UNBLOCKED.
|
||
|
||
PR2 branch: `feat/791-pr2-snapshot-restore` off `origin/main` 32a0ffba. Same treatment applies:
|
||
tests-first red-first, independent review + durable Gitea Reviewer-of-Record comment BEFORE MS-LEAD runs
|
||
the queue guard/merge. Report PR2 number + exact head when ready. PR body: `Part of #791` (NOT Fixes).
|
||
|
||
### PR2 scope (ratified §3/§5 of design doc, Mos-approved — do NOT re-litigate)
|
||
- **(a) Durable pre-update snapshot** to `${XDG_STATE_HOME:-~/.local/state}/mosaic/backups/pre-update-<UTC-ts>/`
|
||
— OUTSIDE ~/.config/mosaic and any repo. Perms dir 0700 / files 0600 (umask 077 + explicit chmod).
|
||
Scope = operator-owned surface that EXISTS (operatorReserved paths), not the framework tree. Taken
|
||
BEFORE any mutation. Retention N=5 (`MOSAIC_BACKUP_RETENTION`), prune older.
|
||
- **Post-sync verify + selective restore**: diff operator surface vs snapshot; (b) should never touch
|
||
operator paths, so ANY diff = manifest bug → restore affected paths + warn loudly. (a) catches a (b) miss.
|
||
- **`mosaic restore`** (TS CLI): `--list` (default, dry-run) enumerates snapshots by ts; `--from <ts>`
|
||
restores over operator surface, confirmation-gated. Counts/paths only.
|
||
- **Secret-safety (secrev)**: snapshot/restore NEVER emit file contents; only paths/counts. Tests assert
|
||
0700/0600 AND that a secret value seeded in tools/_lib/credentials.json never appears in any output.
|
||
|
||
### PR2 implementation status (2026-07-16, ready-for-review)
|
||
All three tasks implemented, red-first proven, unit-green:
|
||
- **Task #10 — durable snapshot (install.sh)**: `backup_root()`/`enumerate_operator_files()`/
|
||
`prune_durable_snapshots()`/`make_durable_snapshot()` wired into keep-mode main() after `manifest_load`,
|
||
before any mutation. umask 077 + explicit chmod 700/600. UTC ts, collision suffix. FAIL-OPEN (a backup
|
||
failure never aborts the upgrade it protects). Retention `MOSAIC_BACKUP_RETENTION` (default 5), in-place
|
||
`sort -r -o` prune (no `mv` — stays inside the rsync-absent coreutils whitelist).
|
||
- **Task #11 — post-sync verify net (install.sh)**: `verify_operator_surface()` runs after sync (trap
|
||
disarmed), `cmp -s` each snapshot file vs target; restores any diverged/missing operator file + warns
|
||
loudly (a divergence = manifest bug). VERIFY-NET wired before `cleanup_snapshot`.
|
||
- **Task #12 — `mosaic restore` (TS)**: `src/commands/restore.ts` + co-located spec (19 tests).
|
||
`--list` default (dry-run enumerate), `--from <ts>` confirmation-gated restore, `--dry-run`, `--yes`/
|
||
`MOSAIC_ASSUME_YES`. Injectable `confirm` for testability (proceed/decline/env-bypass covered). Restored
|
||
files forced 0600. Registered in `cli.ts`. Path convention mirrors install.sh `backup_root()`.
|
||
- **CI**: `.woodpecker/ci.yml` upgrade-guard runs the new `test-upgrade-durable-snapshot.sh` gate.
|
||
- **Gates green**: typecheck ✓ lint ✓ format:check ✓ · full mosaic vitest 1241 (+5) ·
|
||
durable-snapshot 26/26 · manifest-guard 193/193 · rollback 28/28 · migration 21/21.
|
||
Est. new-code coverage ≈93% (only the interactive readline default + process.exit-on-error uncovered).
|
||
- Regression fixed: PR2's `date`/`sort`/`mv` broke the rsync-absent manifest-guard PATH whitelist →
|
||
made date/sort fail-open, replaced `mv` with in-place `sort -o`, added `date sort` to the test whitelist
|
||
+ isolated `XDG_STATE_HOME`. All 193 manifest-guard assertions green under restricted PATH.
|
||
- Codex code-review + security-review (secrev) run on the uncommitted diff before commit.
|
||
|
||
### PR2 review round 1 — findings + remediations (2026-07-16, pre-PR)
|
||
Codex code-review returned **request-changes** (1 blocker + 3 should-fix); Codex security-review returned
|
||
**high** (1 high + 1 medium). Deduped to 5 distinct defects, ALL legitimate, ALL fixed FORWARD, each with
|
||
a red-first regression test whose control neuters exactly the guard under test:
|
||
|
||
- **A · BLOCKER — verify net undid the legacy bin/ migration (install.sh).** On a pre-v2 install `bin/**`
|
||
is operator-classified, so the durable snapshot captured it; `run_migrations()` deletes bin/ on purpose,
|
||
but `verify_operator_surface()` then saw it "missing" and healed it back — the migration would be silently
|
||
undone forever once the version stamps. **Fix:** `MIGRATION_REMOVED_PATHS[]` recorded by run_migrations
|
||
(`bin`,`rails`) + `is_migration_removed()` skip in the verify loop (`# MIGRATION-SKIP-GUARD`).
|
||
**Test:** Part 6 — v1 fixture with bin/; shipped keeps it removed + stamps v3; control (guard stripped)
|
||
wrongly restores bin/tool.sh.
|
||
- **B · HIGH (CWE-59) — restore/verify wrote secrets THROUGH a symlink (install.sh + restore.ts).** An
|
||
attacker swapping an operator path (e.g. tools/_lib/credentials.json) for a symlink after the snapshot
|
||
would make `cp`/`copyFileSync` write the snapshot's secret out through the link. **Fix (bash):** refuse a
|
||
symlinked ancestor (`has_symlinked_parent`), drop a symlinked leaf before restore
|
||
(`# SYMLINK-LEAF-GUARD`). **Fix (TS):** reuse audited `secure-file.ts` — `assertCanonicalContainment`
|
||
+ `ensureManagedDirectory` on every dst, open the leaf `O_NOFOLLOW|O_CREAT|O_TRUNC` 0600 (ELOOP =
|
||
fail-closed). **Tests:** Part 7 (shipped leaves external exfil target untouched, restores a real 0600
|
||
file; control leaks the secret through the link) + restore.spec symlinked-leaf/ancestor cases (red-first).
|
||
- **C · MEDIUM/should-fix (CWE-22) — `--from` traversal escaped the backup root (restore.ts).**
|
||
`join(root, from)` accepted `../poison`. **Fix:** validate the selector against
|
||
`^\d{8}T\d{6}Z(?:-\d+)?$`, build exactly `join(root,'pre-update-'+ts)`, `lstat` (reject symlinked snap
|
||
dir). **Test:** restore.spec `it.each` of 6 malformed selectors + `--from ../poison` fail-closed (red-first).
|
||
- **D · should-fix — verify `mkdir -p` unguarded under set -e (install.sh).** A parent replaced by a
|
||
regular file aborted the installer before the recovery pointer printed. **Fix:** guard `mkdir -p`, warn
|
||
+ `continue` on failure (keeps healing remaining files).
|
||
- **E · should-fix — snapshot `umask 077` leaked process-global (install.sh).** Later sync copies/dirs
|
||
inherited 0600/0700. **Fix:** save `old_umask`, restore on EVERY return path (`# UMASK-RESTORE-NORMAL`).
|
||
**Test:** Part 8 — synced framework file is 0644 while the secret backup stays 0600; control (restore
|
||
stripped) makes the synced file 0600.
|
||
|
||
**Full gate suite re-run after fixes (all green):** typecheck ✓ · lint ✓ · format:check ✓ · full mosaic
|
||
vitest **1252** · restore.spec **30** · durable-snapshot **41** · manifest-guard 193 · rollback 28 ·
|
||
migration 21. shellcheck clean on all new lines; new test markers mirror the existing `# VERIFY-NET`
|
||
anchor convention. NOTE: codex self-review does NOT satisfy the independent-review gate — an independent
|
||
(author≠reviewer) review + durable Gitea Reviewer-of-Record comment is still required before MS-LEAD merges.
|
||
|
||
## Session 4 (2026-07-16) — PR2 MERGED, PR3 built (fleet regen — recovery layer)
|
||
PR2 (#811) squash-merged → main `31607a4a`; issue #791 stays open (final PR of the 3-PR DAG). Independent
|
||
exact-head RoR at `d12c5f78` APPROVE (Gitea cmt 17904); #1882 green; busybox-portable Part 7 control fix
|
||
verified in-Alpine. PR3 UNBLOCKED.
|
||
|
||
PR3 branch: `feat/791-pr3-fleet-regen` off `origin/main` 31607a4. Same discipline: tests-first red-first,
|
||
independent review + durable Gitea RoR BEFORE MS-LEAD runs the queue guard/merge. PR body `Part of #791`.
|
||
|
||
### PR3 scope (ratified §4/§7 of design doc) — `mosaic fleet regen`
|
||
Projection-only recovery command: rebuilds each `fleet/agents/<name>.env.generated` from `roster.yaml`
|
||
(SSOT). Dry-run default; `--write` applies; `--json` machine output. Structural guarantee: NO code path to
|
||
systemd lifecycle — **never restarts an agent**. Single-SSOT: reuses `projectRosterV2AgentGeneratedEnv`
|
||
(extracted, shared with the reconciler apply path) so regen and reconcile cannot drift. Secrev: paths +
|
||
counts only, never the rendered KEY=value body.
|
||
|
||
New files: `commands/fleet-regen-command.ts` (+ `.spec.ts`), guide `docs/guides/upgrade-safety-and-recovery.md`
|
||
(three-layer model: PR1 manifest ownership → PR2 snapshot/restore → PR3 regen; do-NOT-restart-before-verify
|
||
runbook), regen reference added to `docs/guides/fleet-local-canary.md`. Wired in `commands/fleet.ts`.
|
||
|
||
### Independent review (3 reviewers: subagent code-reviewer + codex code-review + codex security) → 4 fixes, red-first
|
||
- **A · BLOCKER (codex) — regen mutated/deleted legacy operator env.** `applyPreparedAgentEnvironmentProjection`
|
||
also writes `.env.local`/`.env.quarantine` and unlinks legacy `.env`. Violated projection-only contract.
|
||
**Fix:** NEW generated-only boundary primitives `prepareGeneratedAgentEnvironmentProjection` +
|
||
`applyPreparedGeneratedAgentEnvironmentProjection` (write ONLY `<name>.env.generated`). regen now has no
|
||
code path that touches `.env`/`.env.local`/`.env.quarantine`. **Test:** projection-only leaves legacy `.env`
|
||
verbatim, no local/quarantine fabricated.
|
||
- **B · should-fix (codex + subagent + security) — partial write on mid-loop failure.** Interleaved
|
||
prepare/apply left earlier agents written when a later agent failed prepare. **Fix:** PREPARE ALL agents
|
||
before writing ANY (mirrors reconciler `defaultPrepareProjections`). **Test:** 2nd agent's projection
|
||
pre-seeded 0644 → prepare rejects → coder0 NOT written, exit 1.
|
||
- **C · subagent — semantic-validation bypass.** Default readRoster skipped `validateRosterV2Semantics`, so
|
||
a tampered protected-class `tool_policy` would be silently projected. **Fix:** default readRoster now runs
|
||
`validateRosterV2Semantics` (persona resolution + protected-class match), rolesDir/overrideDir defaults
|
||
mirroring the reconciler. **Test:** merge-gate agent w/ tool_policy=code → fails closed, no write.
|
||
- **D · MEDIUM (codex security, CWE-362) — concurrent-reconcile race.** regen `--write` wrote without the
|
||
reconcile lock. **Fix:** `--write` acquires `acquirePrivateReconcileLock(mosaicHome)` for the whole
|
||
read-prepare-apply sequence, released in `finally`; dry-run stays lock-free. **Test:** pre-held lock →
|
||
regen fails closed, no write.
|
||
|
||
**Gate suite after fixes (all green):** typecheck ✓ · lint ✓ · format:check ✓ · full mosaic vitest **1265**
|
||
(regen spec 13, incl. 4 new red-first regressions). NOTE: codex self-review does NOT satisfy the
|
||
independent-review gate — an independent (author≠reviewer) review + durable Gitea RoR is still required
|
||
before MS-LEAD merges. STOP at PR-open for MS-LEAD's exact-head review; do NOT self-merge.
|
||
|
||
## Session 5 — PR3 review round 2 (finding L + M1/M2/M3), red-first fixes
|
||
|
||
Second review pass on the lock-cleanup plumbing surfaced one round-1 residual (L) and three round-2
|
||
findings (M1 blocker, M2/M3 should-fix). All fixed red-first (RED proven per-finding, then GREEN).
|
||
|
||
- **L · should-fix (codex r1) — mutation-lock release swallowed unlink failures.** regen's
|
||
`acquirePrivateRosterMutationLock` release copied CRUD's `unlink().catch(()=>{})`, hiding a stale
|
||
`roster.yaml.mutation.lock`. **Fix:** its release PROPAGATES the unlink fault (finding-J stale-lock
|
||
warning then fires for this lock too). **Test:** acquire real lock, `rm` it, assert `release()` rejects.
|
||
- **M1 · BLOCKER (codex r2) — replacement-lock race.** The propagating release from L did an
|
||
UNCONDITIONAL `unlink(lockPath)` without proving ownership. If the lock is cleared + re-created by
|
||
another writer mid-op, regen deletes the STRANGER's live lock → a third writer enters → mutual
|
||
exclusion defeated. **Fix (reuse, not reimplement):** generalized the reconciler's ownership-proving
|
||
lock body into shared `acquirePrivateManagedRosterLock(mosaicHome, lockLeaf, busyMessage, openLock)`;
|
||
`acquirePrivateReconcileLock` delegates to it (behavior-identical: same leaf/codes/messages), and a NEW
|
||
hardened `acquirePrivateRosterMutationLock` (now in fleet-reconciler.ts, leaf `roster.yaml.mutation.lock`)
|
||
records dev/ino + ownership token and RE-PROVES ownership (`assertLockOwnership`) before unlinking —
|
||
fails closed as `lock-cleanup-failed` if replaced. Removed the crud-based export; reverted
|
||
`acquireMutationLock` (fleet-agent-crud.ts) to its original inline empty-file/swallowing-release form
|
||
(CRUD behavior intentionally unchanged). Compatibility: CRUD empty-file `wx` and regen tokened `wx`
|
||
contend on the same path but never co-own (wx winner owns; loser → concurrent-mutation), so the token
|
||
is only ever read back by the same regen invocation. **Test:** acquire, `rm`+recreate lock (new inode),
|
||
assert `release()` rejects AND the replacement survives (not unlinked).
|
||
- **M2 · should-fix (codex r2) — acquire-unwind fault dropped.** The acquire-failure catch discarded
|
||
`releaseFleetLocks`' return (a possible fault on the already-held first lock). **Fix:** capture and
|
||
augment — `const releaseFault = await releaseFleetLocks(releases); throw augmentWithLockCleanupFault(error, releaseFault);`
|
||
(symmetric to finding J). **Test:** mutation lock acquires w/ faulting release + reconcile acquire
|
||
throws → thrown error mentions stale/lock, nothing written.
|
||
- **M3 · should-fix (codex r2 + subagent REQUEST-CHANGES) — cleanup warning named only reconcile lock.**
|
||
Finding L made the mutation-lock release fault reachable, so the `cleanup` marker can originate from
|
||
EITHER lock. **Fix:** `formatFleetRegenReport`'s WARNING now names BOTH `roster.yaml.mutation.lock` and
|
||
`roster.yaml.reconcile.lock`, matching `augmentWithLockCleanupFault`. **Test:** fault the mutation-lock
|
||
release specifically → report names both lock files.
|
||
|
||
**Refactor note (no cycle):** neither fleet-reconciler nor fleet-agent-crud imports the other; regen
|
||
imports lock acquirers from fleet-reconciler and the projection mapping from fleet-reconciler. The two
|
||
reconcile-lock reviewers reconciled: independent reviewer validated acquire-time empty-file compatibility
|
||
(preserved), codex flagged RELEASE-time replacement race (closed by ownership proof) — non-contradictory.
|
||
|
||
**Gate suite after fixes (all green):** typecheck ✓ · lint ✓ · format:check ✓ · full mosaic vitest **1275**
|
||
(regen spec 23, incl. 7 red-first lock regressions E/F/G/K/L/M1/M2/M3). RED proven per-finding by
|
||
temporary revert before re-applying each fix. Independent (author≠reviewer) review of M1/M2/M3 + codex
|
||
code/security re-run in flight. STOP at PR-open for MS-LEAD's exact-head review + durable Gitea RoR; do
|
||
NOT self-merge; #791 umbrella stays OPEN; PR body `Part of #791`.
|
||
|
||
### Round 3 review (after M1/M2/M3) — independent review PASS + codex residual-TOCTOU disposition
|
||
Three reviewers on the post-M1/M2/M3 head:
|
||
- **Independent (subagent, author≠reviewer) — PASS.** Verified M1/M2/M3 all correctly fixed; "never
|
||
restarts" is STRUCTURAL (runner never referenced in executable code); no secrets; no deadlock (only
|
||
regen holds both locks); tests meaningful (assert inode preservation + exact lock-file names). Raised:
|
||
- **should-fix #1 (fixed, red-first):** generalizing the lock helper left `assertSafeLockLeafIfPresent`/
|
||
`assertLockOwnership` hardcoding "reconciliation lock" in thrown messages → a MUTATION-lock fault
|
||
misreported as the reconcile lock, undercutting M3's accurate-diagnosis goal. **Fix:** thread
|
||
`lockLabel = fleet/<leaf>` through both helpers + the generic lock-io messages, so every fault names
|
||
the actual lock file. Red-first: strengthened the M1 test to assert `/roster\.yaml\.mutation\.lock/`
|
||
(RED: got "reconciliation lock"; GREEN after). Also resolves nit #3 (generic-message drift).
|
||
- **nit #2 (fixed):** `FleetRegenResult.cleanup` JSDoc still said "the shared reconcile lock"; now names
|
||
both locks (regen holds both).
|
||
- **nit #4 (fixed):** removed the redundant duplicate `assertLockOwnership` call before unlink
|
||
(pre-existing in merged main; harmless but dead — dropped since the fn was already being touched).
|
||
- **Codex security — clean (risk: none).** Validates roster semantics, constrains env values, no shell
|
||
eval, no secret output, generated-only writes, serialized against both locks.
|
||
- **Codex code — request-changes, 1 "blocker": residual check-then-unlink TOCTOU.** Between the final
|
||
`assertLockOwnership` and the path-based `unlink`, an external actor could vacate our inode and a new
|
||
writer grab the path, so the unlink deletes the stranger's lock. **Disposition: documented known
|
||
limitation, NOT fixed in PR3.** Rationale: (1) byte-identical to the MERGED, shipped reconcile-lock
|
||
release on origin/main (fleet-reconciler.ts L654-659) — not introduced here; (2) UNREACHABLE within the
|
||
`wx` writer protocol — no Mosaic writer removes a lock it doesn't own (wx fails EEXIST while our inode
|
||
exists), so only external interference can vacate our inode in the sub-instruction window; (3) the
|
||
ownership guard DOES close the reachable case (stale-lock reaper/operator cleared our lock + another
|
||
writer took it BEFORE release began → fail closed, don't delete stranger's lock); (4) the true atomic
|
||
fix — fd-held advisory lock (flock/lockf) adopted by ALL fleet writers (CRUD + reconcile + regen) — is
|
||
a cross-cutting mechanism change touching merged CRUD + reconciler, out of scope for a projection-only
|
||
recovery PR. Documented honestly in the acquirer doc + M1 test comment. **The binding independent
|
||
review did NOT treat this as a blocker.** Recommendation to MS-LEAD: proceed to PR-open + spin a
|
||
SEPARATE follow-up issue for the fd-advisory-lock migration; MS-LEAD adjudicates scope at exact-head
|
||
review (merge authority).
|
||
|
||
**Gates after round-3 fixes (all green):** typecheck ✓ · lint ✓ · format:check ✓ · full mosaic vitest
|
||
**1275** (regen spec 23). Fresh codex code re-run in flight to confirm no NEW issues from the label fix.
|
||
|
||
---
|
||
|
||
## Session 6 — Round 4/5 convergence (stranded-lock robustness)
|
||
|
||
**Two independent reviewers converged on the SAME should-fix** on the init-failure cleanup path,
|
||
strengthening confidence it was real:
|
||
|
||
- **Codex code-review-5 — 0 blockers, 1 should-fix.** "Stat failure after lock creation strands the new
|
||
lock." When `handle.stat()` ITSELF fails right after the `wx` create (transient EIO/EBADF), `created`
|
||
is `undefined`, so `removeOwnedLockLeafBestEffort` had `if (!created) return;` → no cleanup → the
|
||
just-created `roster.yaml.mutation.lock`/`reconcile.lock` is stranded, permanently blocking future
|
||
regen + CRUD. (Notably NO blocker, and the TOCTOU is no longer flagged in code-review as of r5.)
|
||
- **Independent delta reviewer (author≠reviewer, pr-review-toolkit) — no blockers, same should-fix.**
|
||
Independently flagged the identical `!created` gap; validated FIX 1 (label threading — no call site
|
||
missed, codes unchanged, no test depended on old text) and FIX 2 (dev/ino-guarded cleanup, best-effort,
|
||
happy-path release reuses captured dev/ino) as correct. Suggested an unconditional best-effort unlink
|
||
in the `!created` branch; I took the **safer** variant below.
|
||
- **Codex security-review-5 — 0 crit / 0 high / 1 medium.** The single medium is the SAME residual
|
||
check-then-unlink TOCTOU already dispositioned in round 3 (its own remediation = "migrate every writer
|
||
to an fd-held advisory lock" = the follow-up issue). No new security finding. No secrets.
|
||
|
||
**Fix (red-first, safer than an unconditional unlink):** thread the persisted random `token` into
|
||
`removeOwnedLockLeafBestEffort`. Two independent ownership proofs now: primary dev/ino (unchanged), and a
|
||
**fallback** when the post-create stat failed — read the leaf and unlink ONLY if its content equals our
|
||
`randomUUID()` token. Only OUR lock carries that token, so a CRUD (empty) or differently-tokened
|
||
replacement is never deleted. `tokenPersisted` guards passing the token (only after `writeFile` lands).
|
||
Doubly-degenerate case (stat fails AND token write never landed) leaves the lock in place rather than
|
||
risk deleting a stranger's file — requires two independent fs faults on a just-created fd; documented.
|
||
|
||
- **Red-first proof:** new test `does not strand the lock file when the post-create stat itself fails`
|
||
injects a real `wx` create + a Proxy handle whose `stat()` rejects (writeFile/close succeed), asserts
|
||
`exists(lockPath) === false`. RED before fix (`expected true to be false` — lock stranded); GREEN after.
|
||
- **Also fixed (delta nit #3):** `fleet-regen-command.ts` `acquireRosterMutationLock` JSDoc said "CRUD's
|
||
private lock"; the default is the reconciler's hardened ownership-proving acquirer for the same
|
||
`fleet/roster.yaml.mutation.lock` path. Corrected.
|
||
- **PR-description note (delta nit #2):** FIX 1 also collapsed a pre-existing duplicate back-to-back
|
||
`assertLockOwnership` call in the release closure (identical args, no intervening logic) into one — a
|
||
no-op simplification of merged code, not a behavior change. Called out so a future reader doesn't
|
||
wonder if the duplicate had a purpose.
|
||
|
||
**Gates after round-4 fixes (all green):** typecheck ✓ · lint ✓ · format:check ✓ · full mosaic vitest
|
||
**1277** (regen spec now 25: +1 stat-failure stranded-lock regression). Residual TOCTOU still deferred to
|
||
the fd-advisory-lock follow-up issue; MS-LEAD adjudicates scope at exact-head review (merge authority).
|
||
|
||
---
|
||
|
||
## Session 6 — Round 6 (persona-root wiring)
|
||
|
||
**Codex code-review-6 — 0 blockers, 1 should-fix (NEW, distinct from the lock work).** "Forward
|
||
configured persona directories to regen." `registerFleetRegenCommand` was registered at
|
||
`fleet.ts:2069` with only `{ runner, mosaicHome }`, discarding `deps.reconcileDeps.rolesDir` /
|
||
`overrideDir`. The regen command ALREADY has those seams (validates roster semantics via
|
||
`validateRosterV2Semantics({ rolesDir, overrideDir })`, defaulting to `<mosaicHome>/fleet/roles{,.local}`),
|
||
but the top-level wiring never forwarded the configured roots. **Impact:** in a deployment with custom
|
||
persona roots, `fleet reconcile` (which honors the overrides) would ACCEPT a roster while `fleet regen`
|
||
REJECTS the same roster (persona resolution against the wrong default dir) — blocking the recovery
|
||
command and violating the documented "resolves personas the SAME way reconcile does" contract.
|
||
|
||
**Fix (red-first):** forward `rolesDir`/`overrideDir` from `deps.reconcileDeps` into
|
||
`registerFleetRegenCommand` at `fleet.ts:2069`. Red-first test `forwards configured persona roots
|
||
(rolesDir/overrideDir) from reconcileDeps into regen`: seeds personas ONLY under a custom root, leaves
|
||
the default `<home>/fleet/roles` empty, registers with `reconcileDeps: { rolesDir, overrideDir }`, and
|
||
requires `fleet regen` to SUCCEED. RED before fix (`expected 1 not to be 1` — regen validated against the
|
||
empty default and exited 1); GREEN after.
|
||
|
||
**Codex security-review-6 — 0 crit / 0 high / 1 medium.** Same residual check-then-unlink TOCTOU, now
|
||
noted at BOTH the release closure and the init-cleanup path; remediation = fd-held advisory lock across
|
||
all writers = the SAME deferred follow-up item. No new security finding, no secrets.
|
||
|
||
**Independent confirmation review of the token-fallback fix (Session 6/round 4) — PASS, no findings.**
|
||
All 7 verification points confirmed; reviewer mechanically reverted `removeOwnedLockLeafBestEffort` to
|
||
the pre-fix `if (!created) return;` and re-ran the new test → RED (`expected true to be false`),
|
||
confirming the test genuinely pins the fix; restored after. No lint/type issues; doc-comment accurate.
|
||
|
||
**Gates after round-6 fix (all green):** typecheck ✓ · lint ✓ · format:check ✓ · full mosaic vitest
|
||
**1278** (regen spec now 26: +1 persona-root wiring regression).
|
||
|
||
---
|
||
|
||
## Session 6 — Round 7 convergence (review CLOSED for PR-open)
|
||
|
||
- **Codex code-review-7 — 0 blockers, 1 should-fix = the residual TOCTOU** (previously a "blocker" in r3,
|
||
dropped in r4/r5, now re-surfaced as a should-fix). **Codex security-review-7 — 0 crit / 0 high /
|
||
1 medium = the SAME residual TOCTOU.** Codex has CONVERGED: the only remaining finding across both
|
||
streams is that one race, whose own remediation is "fd-held advisory lock shared by all fleet writers"
|
||
= the deferred follow-up. No new distinct finding; the wiring fix introduced nothing.
|
||
- **Independent confirmation review of the persona-root wiring fix — PASS, no findings.** Reviewer
|
||
mechanically reverted the two forwarded lines → RED (`Roster v2 agent "coder0" class "code" does not
|
||
resolve to a readable persona` → exit 1), restored → GREEN (26 regen + 204 fleet tests). Confirmed the
|
||
optional-chaining fallback preserves default-deployment behavior and no type/lint issue.
|
||
|
||
**Review disposition for PR-open:** ALL actionable findings fixed red-first across rounds 3–6 (label
|
||
threading, stranded-lock on init failure, stat-failure strand, persona-root wiring). The residual
|
||
check-then-unlink TOCTOU is the ONLY open item and is DEFERRED to a follow-up issue (fd-advisory-lock
|
||
migration across CRUD + reconcile + regen) — byte-identical to merged origin/main's reconcile-lock
|
||
release, unreachable within the `wx` writer protocol (no Mosaic writer removes a lock it doesn't own;
|
||
only external `rm`/a stale-lock reaper can vacate the inode mid-release), and its true fix is a
|
||
cross-cutting mechanism change out of scope for a projection-only recovery PR. Two independent human-agent
|
||
reviews (author≠reviewer) treated it as non-blocking. MS-LEAD adjudicates scope at exact-head review
|
||
(merge authority); recommendation = proceed to PR-open + spin the follow-up issue.
|
||
|
||
**Final gates (all green):** typecheck ✓ · lint ✓ · format:check ✓ · full mosaic vitest **1278**
|
||
(regen spec 26). No secret values in any snapshot/projection/report output (counts + paths only). Regen
|
||
NEVER issues a lifecycle/restart call (load-bearing recordingRunner gate). STOP at PR-open for MS-LEAD's
|
||
exact-head review + durable Reviewer-of-Record before any merge; do NOT self-merge.
|