This commit was merged in pull request #813.
This commit is contained in:
@@ -296,3 +296,234 @@ vitest **1252** · restore.spec **30** · durable-snapshot **41** · manifest-gu
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user