Harden model-groups config persistence: fsync, locking, symlink containment, backup-on-overwrite

[grzegorznowak]

Context

Found during code review of PR #27 (model modalities work). These four HIGH findings are pre-existing — verified present at the PR base commit 4efc9cf (model-groups/store.ts before any modality changes). PR #27 only touched this file in passing; the root causes live in the persistence layer, not the modality feature.

Findings

1. No fsync on temp-file + rename → power-loss data loss (HIGH)

model-groups/store.ts saveModelGroups: writes a temp file, then renameSync over the target. The rename is visibility-atomic, but neither temp file nor directory is fsynced. A power loss / kernel crash after the rename can leave the committed name pointing at missing, zero-length, or stale data. Ordinary process termination is handled (temp is cleaned), but crash durability is not.

• Evidence: identical atomic-write block at base store.ts:134-148.
• Fix direction: fs.openSync + fs.fsyncSync on temp before rename; optionally fsync the directory.

2. No lock/CAS on read-modify-write CRUD → lost-update race (HIGH)

createGroup/updateGroup/renameGroup/deleteGroup/moveGroup all do load → mutate in memory → save. Two concurrent processes (e.g. two pi sessions, or an editor + pi) can both load, each save a full replacement groups object, and the later writer silently drops the earlier writer's changes. The save path preserves only root opaque keys, not concurrently-added groups.

• Fix direction: file lock (e.g. proper lockfile around read-modify-write) or version-based CAS (re-read + compare version before rename).

3. Symlink-following path resolution → cross-scope integrity/security (HIGH)

modelGroupsPath builds project-scope path as path.join(cwd, ".pi", "pi-agenticoding", "model-groups.json") with no containment or no-follow check. A repository can include a .pi symlink pointing at the user's global agent dir (or any chosen dir), causing project-scope CRUD to read/write the global model-groups.json (or another location).

• Evidence: byte-identical at base store.ts:27-30.
• Fix direction: resolve the config dir and refuse (or contain) paths that escape cwd via symlinks / externally-controlled components.

4. Direct saveModelGroups overwrites corrupt JSON without backup (HIGH)

The load path backs up corrupt/schema-invalid files to .bak, but the exported low-level saveModelGroups reads the existing file with try { JSON.parse } catch { /* load recovery owns malformed content */ } and silently replaces it — destroying the only recoverable copy of the malformed data. Any caller that doesn't go through the load-recovery path (or an external script) can permanently lose config.

• Evidence: base store.ts:140 (catch { /* load recovery owns malformed content */ }).
• Fix direction: saveModelGroups should refuse to overwrite when the existing file fails to parse (requiring an explicit recovery/backup step), mirroring loadScopeConfig's refusal when backup failed.

Scope

Pre-existing debt; explicitly not part of PR #27. Fix in a dedicated hardening change. Lower-severity neighbors (v3 runtime down-conversion via API, moveGroup partial-move window, file-mode not preserved on rewrite) tracked separately if desired.