fix(retro): make history migration safe against symlinks and concurrent runs (#3749 review)

Review pass 1 findings: a .context/retros that resolves outside its worktree
(or to the shared history itself) is left alone, so migration can no longer
delete the history it points at; moves are no-clobber with retry; a copy
identical to any same-date snapshot collapses; RETRO_FILE is reserved as {}
so overlapping runs in two worktrees never share a path; --next must be a
YYYY-MM-DD date; a missing option value no longer loops forever. Step 12
names RETRO_HISTORY_ERROR instead of reading it as a stale helper.

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

bin/gstack-retro-metrics

@@ -35,9 +35,12 @@
 #   state root's projects/<slug>/retros, shared by every worktree of the
 #   repository) and one RETRO_PRIOR line per snapshot, newest first. Moves any
 #   legacy .context/retros/*.json from this worktree and the main checkout
-#   into it first: identical copies collapse, a same-name snapshot that
-#   differs takes the next free number for its date. --next also prints
-#   RETRO_FILE, the next unused <date>-<n>.json path.
+#   into it first: a copy identical to a stored snapshot of the same date
+#   collapses, a same-name snapshot that differs takes the next free number
+#   for its date, and a .context/retros that resolves outside its worktree
+#   is left alone. --next <YYYY-MM-DD> also reserves the next unused
+#   <date>-<n>.json (written as {} so a concurrent run cannot take it) and
+#   prints it as RETRO_FILE.
 
 GSTACK_BUN_CONFIG=/dev/null; case "${OSTYPE:-}" in msys*|cygwin*) if command -v cygpath >/dev/null 2>&1; then GSTACK_BUN_CONFIG=NUL; fi ;; esac
 
@@ -46,11 +49,11 @@ SINCE="7 days ago"
 UNTIL=""
 while [ $# -gt 0 ]; do
   case "$1" in
-    --base)  BASE="$2"; shift 2 ;;
-    --since) SINCE="$2"; shift 2 ;;
-    --until) UNTIL="$2"; shift 2 ;;
+    --base)  BASE="$2"; shift 2 || break ;;
+    --since) SINCE="$2"; shift 2 || break ;;
+    --until) UNTIL="$2"; shift 2 || break ;;
     --history) HISTORY=1; shift ;;
-    --next)  NEXT_DATE="$2"; shift 2 ;;
+    --next)  NEXT_DATE="$2"; shift 2 || break ;;
     *) shift ;;
   esac
 done
@@ -71,21 +74,37 @@ if [ -n "${HISTORY:-}" ]; then
   fi
   _HD="$_GH/projects/$_SLUG/retros"
   mkdir -p "$_HD" || { echo "RETRO_HISTORY_ERROR: cannot create $_HD"; exit 0; }
+  if [ -n "${NEXT_DATE:-}" ]; then
+    case "$NEXT_DATE" in
+      [0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]) ;;
+      *) echo "RETRO_HISTORY_ERROR: --next takes a YYYY-MM-DD date, got '$NEXT_DATE'"; exit 0 ;;
+    esac
+  fi
+  _HDR=$(cd -P "$_HD" && pwd)
   _TOP=$(git rev-parse --show-toplevel 2>/dev/null || true)
   _MAIN=$(git worktree list --porcelain 2>/dev/null | sed -n '1s/^worktree //p')
   _MOVED=0
   for _TREE in "$_TOP" "$_MAIN"; do
     [ -n "$_TREE" ] && [ -d "$_TREE/.context/retros" ] || continue
-    for _F in "$_TREE"/.context/retros/*.json; do
-      [ -f "$_F" ] || continue
+    _SRC=$(cd -P "$_TREE/.context/retros" 2>/dev/null && pwd) || continue
+    _TR=$(cd -P "$_TREE" 2>/dev/null && pwd) || continue
+    case "$_SRC" in "$_TR"/*) ;; *) continue ;; esac
+    [ "$_SRC" != "$_HDR" ] || continue
+    for _F in "$_SRC"/*.json; do
+      [ -f "$_F" ] && [ ! -L "$_F" ] || continue
       _B=$(basename "$_F")
+      _DUP=""
+      for _E in "$_HD/$_B" "$_HD/${_B%-*}"-*.json; do
+        [ -f "$_E" ] && cmp -s "$_F" "$_E" && { _DUP=1; break; }
+      done
+      if [ -n "$_DUP" ]; then rm -f "$_F"; continue; fi
       _DEST="$_HD/$_B"
-      if [ -e "$_DEST" ] && cmp -s "$_F" "$_DEST"; then
-        rm -f "$_F"; continue
-      fi
       _N=1
-      while [ -e "$_DEST" ]; do _N=$((_N + 1)); _DEST="$_HD/${_B%-*}-$_N.json"; done
-      mv "$_F" "$_DEST" && _MOVED=$((_MOVED + 1))
+      while [ -e "$_F" ] && [ "$_N" -le 1000 ]; do
+        [ -e "$_DEST" ] || mv -n "$_F" "$_DEST" 2>/dev/null
+        [ -e "$_F" ] || { _MOVED=$((_MOVED + 1)); break; }
+        _N=$((_N + 1)); _DEST="$_HD/${_B%-*}-$_N.json"
+      done
     done
     rmdir "$_TREE/.context/retros" 2>/dev/null || true
   done
@@ -94,7 +113,10 @@ if [ -n "${HISTORY:-}" ]; then
   ls -t "$_HD"/*.json 2>/dev/null | sed 's/^/RETRO_PRIOR: /'
   if [ -n "${NEXT_DATE:-}" ]; then
     _N=1
-    while [ -e "$_HD/$NEXT_DATE-$_N.json" ]; do _N=$((_N + 1)); done
+    until ( set -C; printf '{}\n' > "$_HD/$NEXT_DATE-$_N.json" ) 2>/dev/null; do
+      _N=$((_N + 1))
+      [ "$_N" -le 1000 ] || { echo "RETRO_HISTORY_ERROR: cannot reserve a snapshot file in $_HD"; exit 0; }
+    done
     echo "RETRO_FILE: $_HD/$NEXT_DATE-$_N.json"
   fi
   exit 0

retro/SKILL.md

@@ -512,7 +512,7 @@ _RM="$HOME/.claude/skills/gstack/bin/gstack-retro-metrics"
 "$_RM" --history --next "<today>"
 ```
 
-`RETRO_PRIOR:` lines are prior snapshots, newest first. No `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`.
+`RETRO_PRIOR:` lines are prior snapshots, newest first; a file holding only `{}` is a reservation, not a snapshot. `RETRO_HISTORY_ERROR:` means history is unavailable: tell the user, show its fix, and skip history and the Step 13 save. Otherwise no `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`.
 
 **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead):
 ```
@@ -533,7 +533,7 @@ Deep sessions:      3      →    5           ↑2
 
 After computing all metrics (including streak) and loading any prior history for comparison, draft the tweetable summary and the "3 Things to Improve" items using the format in Step 14, then save a JSON snapshot. The Step 14 narrative must reuse this exact summary and these exact items. `streak_days` is the live **team** streak from Step 11 (0 when broken); put the personal streak in `user_streak_days`.
 
-Use the Write tool to save it to Step 12's `RETRO_FILE` with this schema:
+Step 12 reserved `RETRO_FILE` as `{}`: Read it, then use the Write tool to replace it with this schema:
 ```json
 {
   "date": "2026-03-08",

retro/SKILL.md.tmpl

@@ -357,7 +357,7 @@ _RM="{{RUNTIME_ROOT}}/bin/gstack-retro-metrics"
 "$_RM" --history --next "<today>"
 ```
 
-`RETRO_PRIOR:` lines are prior snapshots, newest first. No `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`.
+`RETRO_PRIOR:` lines are prior snapshots, newest first; a file holding only `{}` is a reservation, not a snapshot. `RETRO_HISTORY_ERROR:` means history is unavailable: tell the user, show its fix, and skip history and the Step 13 save. Otherwise no `RETRO_FILE:` line means a stale helper: skip history and suggest `/gstack-upgrade`.
 
 **If prior retros exist:** Load the most recent one with the same `window` using the Read tool; if none matches, disclose that and skip historical deltas. Calculate deltas for available key metrics and include a **Trends vs Last Retro** section (in `compare` mode use the freshly computed prior period instead):
 ```
@@ -378,7 +378,7 @@ Deep sessions:      3      →    5           ↑2
 
 After computing all metrics (including streak) and loading any prior history for comparison, draft the tweetable summary and the "3 Things to Improve" items using the format in Step 14, then save a JSON snapshot. The Step 14 narrative must reuse this exact summary and these exact items. `streak_days` is the live **team** streak from Step 11 (0 when broken); put the personal streak in `user_streak_days`.
 
-Use the Write tool to save it to Step 12's `RETRO_FILE` with this schema:
+Step 12 reserved `RETRO_FILE` as `{}`: Read it, then use the Write tool to replace it with this schema:
 ```json
 {
   "date": "2026-03-08",

test/regression-3749-retro-history-worktree.test.ts

@@ -105,8 +105,9 @@ describe('#3749 retro history is shared across linked worktrees', () => {
     fs.writeFileSync(file!, JSON.stringify({ date: '2026-11-01', window: '7d' }));
 
     git(main, 'worktree', 'remove', '--force', wt);
-    expect(sh(LOAD, main).stdout).toContain(file!);
-    expect(sh(LOAD, main).stdout).toContain(`RETRO_FILE: ${path.join(history, '2026-11-01-2.json')}`);
+    const fromMain = sh(LOAD, main).stdout;
+    expect(fromMain).toContain(file!);
+    expect(fromMain).toContain(`RETRO_FILE: ${path.join(history, '2026-11-01-2.json')}`);
   });
 
   test('migration keeps every snapshot: duplicates collapse, same-name differing snapshots get the next number', () => {
@@ -117,12 +118,33 @@ describe('#3749 retro history is shared across linked worktrees', () => {
 
     const r = sh(LOAD, wt);
     expect(r.status).toBe(0);
-    expect(fs.readdirSync(history).sort()).toEqual(['2026-10-18-1.json', '2026-10-25-1.json', '2026-10-25-2.json']);
+    expect(fs.readdirSync(history).sort()).toEqual(['2026-10-18-1.json', '2026-10-25-1.json', '2026-10-25-2.json', '2026-11-01-1.json']);
     const froms = ['2026-10-25-1.json', '2026-10-25-2.json'].map((f) => JSON.parse(fs.readFileSync(path.join(history, f), 'utf-8')).from).sort();
     expect(froms).toEqual(['main', 'worktree']);
     for (const tree of [main, wt]) expect(fs.existsSync(path.join(tree, '.context', 'retros'))).toBe(false);
   });
 
+  test('a .context/retros symlinked to the shared history is left alone, so migration cannot delete it', () => {
+    const { wt, history } = layout();
+    const kept = snapshot(history, '2026-10-25-1.json', { date: '2026-10-25', window: '7d' });
+    fs.mkdirSync(path.join(wt, '.context'), { recursive: true });
+    fs.symlinkSync(history, path.join(wt, '.context', 'retros'));
+
+    const r = sh(LOAD, wt);
+    expect(r.status).toBe(0);
+    expect(fs.readFileSync(kept, 'utf-8')).toContain('2026-10-25');
+    expect(r.stdout).toContain(`RETRO_PRIOR: ${kept}`);
+  });
+
+  test('RETRO_FILE is reserved, so two runs in different worktrees never get the same path', () => {
+    const { main, wt, history } = layout();
+    const first = sh(LOAD, wt).stdout.match(/^RETRO_FILE: (.+)$/m)?.[1];
+    const second = sh(LOAD, main).stdout.match(/^RETRO_FILE: (.+)$/m)?.[1];
+    expect(first).toBe(path.join(history, '2026-11-01-1.json'));
+    expect(second).toBe(path.join(history, '2026-11-01-2.json'));
+    expect(fs.readFileSync(first!, 'utf-8').trim()).toBe('{}');
+  });
+
   test('the gbrain prior-retros query reads the same state-root history', () => {
     expect(TMPL).toContain('glob: "{gstack_state_root}/projects/{repo_slug}/retros/*.json"');
     expect(TMPL).not.toContain('glob: ".context/retros/*.json"');