fix(migrations): defer v1.40.0.0 done-marker when jq is missing (#1581)

gstack-upgrade/migrations/v1.40.0.0.sh

@@ -34,6 +34,13 @@ NEW_PATTERNS=(
 )
 
 added_any=0
+# Set to 1 if a required step had to be skipped (e.g. jq missing for the
+# privacy-map patch). We do NOT write the done-marker in that case — so the
+# next /gstack-upgrade run will retry against an environment with jq
+# installed. See #1581: previously the done-marker was written
+# unconditionally, which silently dropped the privacy-map repair on boxes
+# without jq and federation sync kept missing eng-review test plans.
+skipped_required=0
 
 # ----- .brain-allowlist ---------------------------------------------------
 if [ -f "${ALLOWLIST}" ]; then
@@ -62,12 +69,23 @@ if [ -f "${PRIVACY}" ]; then
           added_any=1
         else
           rm -f "${PRIVACY}.tmp"
+          skipped_required=1
           echo "  [v1.40.0.0] WARN: jq failed to patch ${PRIVACY}; skipping pattern ${PATTERN}." >&2
         fi
       fi
     done
   else
-    echo "  [v1.40.0.0] WARN: jq not found; skipping privacy-map repair. Install jq and re-run gstack-upgrade, or run gstack-artifacts-init manually." >&2
+    skipped_required=1
+    echo "" >&2
+    echo "  [v1.40.0.0] *** ACTION REQUIRED ***" >&2
+    echo "  [v1.40.0.0] jq not found; cannot patch .brain-privacy-map.json." >&2
+    echo "  [v1.40.0.0] Federation sync will keep dropping /plan-eng-review test plans" >&2
+    echo "  [v1.40.0.0] until this runs. Install jq and re-run /gstack-upgrade:" >&2
+    echo "  [v1.40.0.0]   - macOS:  brew install jq" >&2
+    echo "  [v1.40.0.0]   - Debian/Ubuntu: sudo apt install jq" >&2
+    echo "  [v1.40.0.0]   - Fedora: sudo dnf install jq" >&2
+    echo "  [v1.40.0.0] (Migration done-marker NOT written — next /gstack-upgrade retries.)" >&2
+    echo "" >&2
   fi
 fi
 
@@ -82,10 +100,18 @@ if [ -f "${GITATTRS}" ]; then
   done
 fi
 
-# Mark done even if no patches needed — a fresh-init user's
+# Mark done only when nothing was skipped. A fresh-init user's
 # bin/gstack-artifacts-init now writes the pattern directly, so re-runs
-# should no-op. The touchfile keeps the migration runner from looping.
-touch "${DONE}"
+# should no-op and the marker gets written immediately. The touchfile keeps
+# the migration runner from looping on healthy installs.
+#
+# When skipped_required=1 (e.g. jq missing) we deliberately leave the
+# done-marker unwritten so the next /gstack-upgrade run retries the
+# privacy-map patch. The other files were patched on this pass and their
+# "already present" gates make re-running idempotent. See #1581.
+if [ "${skipped_required}" = "0" ]; then
+  touch "${DONE}"
+fi
 
 if [ "${added_any}" = "1" ]; then
   echo "  [v1.40.0.0] allowlist/privacy-map/gitattributes patched for /plan-eng-review test plans (idempotent)" >&2

test/artifacts-init-migration.test.ts

@@ -3,10 +3,28 @@
 // .brain-privacy-map.json, and .gitattributes.
 
 import { describe, expect, test, beforeEach } from 'bun:test';
-import { mkdtempSync, writeFileSync, readFileSync, existsSync, rmSync, mkdirSync } from 'fs';
+import { mkdtempSync, writeFileSync, readFileSync, existsSync, rmSync, mkdirSync, symlinkSync } from 'fs';
 import { tmpdir } from 'os';
 import { join } from 'path';
 
+// Build a fake bin directory that holds symlinks to every coreutil the
+// migration script invokes — but deliberately omits `jq`. Used by the
+// #1581 regression tests to simulate a host that hasn't installed jq.
+function makeNoJqBin(): string {
+  const dir = mkdtempSync(join(tmpdir(), 'mig-nojq-bin-'));
+  // Match the actual /usr/bin paths the migration script + bash need.
+  const utils = ['bash', 'sh', 'grep', 'sed', 'printf', 'touch', 'cat',
+    'rm', 'mv', 'mkdir', 'ls', 'dirname', 'basename', 'echo', 'env',
+    'command', 'date', 'awk', 'tr', 'find', 'tee', 'sort', 'head', 'tail'];
+  for (const util of utils) {
+    const src = `/usr/bin/${util}`;
+    if (existsSync(src)) {
+      try { symlinkSync(src, join(dir, util)); } catch { /* already there */ }
+    }
+  }
+  return dir;
+}
+
 const REPO_ROOT = new URL('..', import.meta.url).pathname;
 const MIGRATION = join(REPO_ROOT, 'gstack-upgrade', 'migrations', 'v1.38.1.0.sh');
 
@@ -330,4 +348,90 @@ describe('v1.40.0.0 migration', () => {
       rmSync(home, { recursive: true, force: true });
     }
   });
+
+  // Regression for #1581: when jq is missing on the host, the privacy-map
+  // block is skipped. Previously the done-marker was written anyway, so the
+  // next /gstack-upgrade no-op'd and the privacy-map never landed. The
+  // marker must now stay unwritten so the next run retries.
+  test('#1581: jq missing leaves done-marker unwritten and patches the rest', () => {
+    const home = setupFakeHome();
+    const fakeBinDir = makeNoJqBin();
+    try {
+      writeFileSync(join(home, '.gstack', '.brain-allowlist'), 'existing-line\n');
+      writeFileSync(join(home, '.gstack', '.brain-privacy-map.json'), JSON.stringify([
+        { pattern: 'projects/*/*-design-*.md', class: 'artifact' },
+      ], null, 2));
+      writeFileSync(join(home, '.gstack', '.gitattributes'), 'projects/*/*-design-*.md merge=union\n');
+
+      // PATH points only at the no-jq bin dir (coreutils symlinks, no jq).
+      // Mirrors a Debian box that hasn't apt-installed jq yet.
+      const proc = Bun.spawnSync({
+        cmd: ['/bin/bash', MIGRATION_V1_40],
+        env: { HOME: home, PATH: fakeBinDir },
+        stdout: 'pipe',
+        stderr: 'pipe',
+      });
+      const r = {
+        code: proc.exitCode ?? -1,
+        stderr: new TextDecoder().decode(proc.stderr),
+      };
+      expect(r.code).toBe(0);
+
+      // Allowlist and gitattributes were patchable without jq — both updated.
+      expect(readFileSync(join(home, '.gstack', '.brain-allowlist'), 'utf-8'))
+        .toContain('projects/*/*-eng-review-test-plan-*.md');
+      expect(readFileSync(join(home, '.gstack', '.gitattributes'), 'utf-8'))
+        .toContain('projects/*/*-eng-review-test-plan-*.md merge=union');
+
+      // Privacy-map untouched (no jq) — the eng-review entry didn't land.
+      const privacy = JSON.parse(readFileSync(join(home, '.gstack', '.brain-privacy-map.json'), 'utf-8'));
+      expect(privacy.find((e: any) => e.pattern === 'projects/*/*-eng-review-test-plan-*.md')).toBeUndefined();
+
+      // Crucially: done-marker NOT written, so /gstack-upgrade retries on
+      // the next run once jq is installed.
+      expect(existsSync(join(home, '.gstack', '.migrations', 'v1.40.0.0.done'))).toBe(false);
+
+      // User gets an actionable WARN block, not a single buried line.
+      expect(r.stderr).toContain('ACTION REQUIRED');
+      expect(r.stderr).toContain('Install jq');
+      expect(r.stderr).toContain('done-marker NOT written');
+    } finally {
+      rmSync(home, { recursive: true, force: true });
+      rmSync(fakeBinDir, { recursive: true, force: true });
+    }
+  });
+
+  // Companion to the regression above — re-running after installing jq
+  // completes the migration: privacy-map patched, done-marker now written.
+  test('#1581: retry after jq install completes the migration', () => {
+    const home = setupFakeHome();
+    const fakeBinDir = makeNoJqBin();
+    try {
+      writeFileSync(join(home, '.gstack', '.brain-allowlist'), 'existing\n');
+      writeFileSync(join(home, '.gstack', '.brain-privacy-map.json'), JSON.stringify([
+        { pattern: 'projects/*/*-design-*.md', class: 'artifact' },
+      ], null, 2));
+      writeFileSync(join(home, '.gstack', '.gitattributes'), 'placeholder\n');
+
+      // Run 1: simulate jq missing.
+      Bun.spawnSync({
+        cmd: ['/bin/bash', MIGRATION_V1_40],
+        env: { HOME: home, PATH: fakeBinDir },
+        stdout: 'pipe',
+        stderr: 'pipe',
+      });
+      expect(existsSync(join(home, '.gstack', '.migrations', 'v1.40.0.0.done'))).toBe(false);
+
+      // Run 2: jq now available (real PATH). Should finish the job.
+      const r2 = runMigrationV140(home);
+      expect(r2.code).toBe(0);
+      expect(existsSync(join(home, '.gstack', '.migrations', 'v1.40.0.0.done'))).toBe(true);
+
+      const privacy = JSON.parse(readFileSync(join(home, '.gstack', '.brain-privacy-map.json'), 'utf-8'));
+      expect(privacy.find((e: any) => e.pattern === 'projects/*/*-eng-review-test-plan-*.md')?.class).toBe('artifact');
+    } finally {
+      rmSync(home, { recursive: true, force: true });
+      rmSync(fakeBinDir, { recursive: true, force: true });
+    }
+  });
 });