From d24cd1c023db0a5df4adafc9babc052f267329ff Mon Sep 17 00:00:00 2001
From: Stefan Neamtu <stefan.neamtu@nearone.org>
Date: Mon, 18 May 2026 18:23:57 +0200
Subject: [PATCH 1/2] fix(migration): defer v1.40.0.0 done-marker until every
 repair succeeds (#1581)

The v1.40.0.0 migration unconditionally `touch`ed its done-marker, even
when the jq-gated `.brain-privacy-map.json` patch was skipped because jq
was missing on the user's machine. On subsequent runs, the script
short-circuited on the marker so the privacy-map repair never landed.
Federation sync then silently dropped `/plan-eng-review` test plans.

Track every failure mode via a single `incomplete` flag: jq missing,
malformed JSON, jq mutation failure, tempfile creation failure, `mv`
failure, allowlist append failure, gitattributes append failure. The
marker is written only when `incomplete=0`, so the migration runner
retries on the next /gstack-upgrade once the prerequisites are met.
---
 gstack-upgrade/migrations/v1.40.0.0.sh | 93 +++++++++++++++++++-------
 1 file changed, 69 insertions(+), 24 deletions(-)

diff --git a/gstack-upgrade/migrations/v1.40.0.0.sh b/gstack-upgrade/migrations/v1.40.0.0.sh
index d21c18ba3a..e482d57a6e 100755
--- a/gstack-upgrade/migrations/v1.40.0.0.sh
+++ b/gstack-upgrade/migrations/v1.40.0.0.sh
@@ -13,6 +13,12 @@
 #
 # Idempotent: each insertion is gated on `not already present` so re-running
 # the migration is a no-op.
+#
+# Done-marker discipline (#1581): the marker is only written when every
+# required repair either succeeded or was provably unnecessary. Tracking
+# happens via the `incomplete` flag; on any failure path (missing jq, broken
+# JSON, append failure, mv failure) we set `incomplete=1` and skip the touch
+# so the migration runner retries on the next /gstack-upgrade.
 
 set -u
 
@@ -34,19 +40,30 @@ NEW_PATTERNS=(
 )
 
 added_any=0
+incomplete=0
 
 # ----- .brain-allowlist ---------------------------------------------------
 if [ -f "${ALLOWLIST}" ]; then
   for PATTERN in "${NEW_PATTERNS[@]}"; do
     if ! grep -Fq -- "${PATTERN}" "${ALLOWLIST}" 2>/dev/null; then
       if grep -q '^# ---- USER ADDITIONS BELOW' "${ALLOWLIST}" 2>/dev/null; then
-        sed -i.bak "/^# ---- USER ADDITIONS BELOW/i\\
+        if sed -i.bak "/^# ---- USER ADDITIONS BELOW/i\\
 ${PATTERN}
-" "${ALLOWLIST}" && rm -f "${ALLOWLIST}.bak"
-        added_any=1
+" "${ALLOWLIST}" 2>/dev/null; then
+          rm -f "${ALLOWLIST}.bak"
+          added_any=1
+        else
+          echo "  [v1.40.0.0] WARN: failed to insert ${PATTERN} into ${ALLOWLIST}; will retry on next upgrade." >&2
+          rm -f "${ALLOWLIST}.bak" 2>/dev/null || true
+          incomplete=1
+        fi
       else
-        printf '%s\n' "${PATTERN}" >> "${ALLOWLIST}"
-        added_any=1
+        if printf '%s\n' "${PATTERN}" >> "${ALLOWLIST}" 2>/dev/null; then
+          added_any=1
+        else
+          echo "  [v1.40.0.0] WARN: failed to append ${PATTERN} to ${ALLOWLIST}; will retry on next upgrade." >&2
+          incomplete=1
+        fi
       fi
     fi
   done
@@ -55,19 +72,39 @@ fi
 # ----- .brain-privacy-map.json -------------------------------------------
 if [ -f "${PRIVACY}" ]; then
   if command -v jq >/dev/null 2>&1; then
-    for PATTERN in "${NEW_PATTERNS[@]}"; do
-      if ! jq -e --arg p "${PATTERN}" 'map(select(.pattern == $p)) | length > 0' "${PRIVACY}" >/dev/null 2>&1; then
-        if jq --arg p "${PATTERN}" '. += [{"pattern": $p, "class": "artifact"}]' "${PRIVACY}" > "${PRIVACY}.tmp" 2>/dev/null; then
-          mv "${PRIVACY}.tmp" "${PRIVACY}"
-          added_any=1
-        else
-          rm -f "${PRIVACY}.tmp"
-          echo "  [v1.40.0.0] WARN: jq failed to patch ${PRIVACY}; skipping pattern ${PATTERN}." >&2
+    # Validate JSON shape up front. We won't try to repair a corrupt file —
+    # bail out and leave for manual fix.
+    if ! jq -e . "${PRIVACY}" >/dev/null 2>&1; then
+      echo "  [v1.40.0.0] WARN: ${PRIVACY} is not valid JSON; skipping privacy-map repair. Fix manually or run gstack-artifacts-init." >&2
+      incomplete=1
+    else
+      for PATTERN in "${NEW_PATTERNS[@]}"; do
+        if ! jq -e --arg p "${PATTERN}" 'map(select(.pattern == $p)) | length > 0' "${PRIVACY}" >/dev/null 2>&1; then
+          tmp=$(mktemp "${PRIVACY}.tmp.XXXXXX" 2>/dev/null)
+          if [ -z "${tmp}" ] || [ ! -f "${tmp}" ]; then
+            echo "  [v1.40.0.0] WARN: failed to create tempfile for ${PRIVACY}; skipping pattern ${PATTERN}." >&2
+            incomplete=1
+            continue
+          fi
+          if jq --arg p "${PATTERN}" '. += [{"pattern": $p, "class": "artifact"}]' "${PRIVACY}" > "${tmp}" 2>/dev/null; then
+            if mv "${tmp}" "${PRIVACY}" 2>/dev/null; then
+              added_any=1
+            else
+              echo "  [v1.40.0.0] WARN: failed to rewrite ${PRIVACY}; skipping pattern ${PATTERN}." >&2
+              rm -f "${tmp}"
+              incomplete=1
+            fi
+          else
+            echo "  [v1.40.0.0] WARN: jq mutation failed for ${PRIVACY}; skipping pattern ${PATTERN}." >&2
+            rm -f "${tmp}"
+            incomplete=1
+          fi
         fi
-      fi
-    done
+      done
+    fi
   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
+    incomplete=1
   fi
 fi
 
@@ -76,19 +113,27 @@ if [ -f "${GITATTRS}" ]; then
   for PATTERN in "${NEW_PATTERNS[@]}"; do
     RULE="${PATTERN} merge=union"
     if ! grep -Fq -- "${RULE}" "${GITATTRS}" 2>/dev/null; then
-      printf '%s\n' "${RULE}" >> "${GITATTRS}"
-      added_any=1
+      if printf '%s\n' "${RULE}" >> "${GITATTRS}" 2>/dev/null; then
+        added_any=1
+      else
+        echo "  [v1.40.0.0] WARN: failed to append rule to ${GITATTRS}; will retry on next upgrade." >&2
+        incomplete=1
+      fi
     fi
   done
 fi
 
-# Mark done even if no patches needed — 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}"
-
-if [ "${added_any}" = "1" ]; then
-  echo "  [v1.40.0.0] allowlist/privacy-map/gitattributes patched for /plan-eng-review test plans (idempotent)" >&2
+if [ "${incomplete}" = "0" ]; then
+  # Mark done — every required repair either succeeded or was provably
+  # unnecessary. A fresh-init user's bin/gstack-artifacts-init now writes the
+  # pattern directly, so re-runs no-op. The touchfile keeps the migration
+  # runner from looping.
+  touch "${DONE}"
+  if [ "${added_any}" = "1" ]; then
+    echo "  [v1.40.0.0] allowlist/privacy-map/gitattributes patched for /plan-eng-review test plans (idempotent)" >&2
+  fi
+else
+  echo "  [v1.40.0.0] INFO: marker not written; gstack-upgrade will retry once prerequisites are met." >&2
 fi
 
 # NEVER `git commit + push` from this migration. The user controls when the

From da19325796d93b4b8e2f0fdf95b1a884e05d23a3 Mon Sep 17 00:00:00 2001
From: Stefan Neamtu <stefan.neamtu@nearone.org>
Date: Mon, 18 May 2026 19:00:03 +0200
Subject: [PATCH 2/2] test(migration): unit tests for v1.40.0.0 deferred
 done-marker fix (#1581)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

8 cases pinning the fix:

- Case 1 (happy path): jq present, fresh privacy-map → all three files
  patched, marker written.
- Case 2 (regression for #1581): jq missing, privacy-map present →
  marker must NOT be written. Fails against the buggy script, passes
  against the fix.
- Case 3 (recovery): jq missing, then jq restored → patch lands on
  second run.
- Case 4 (idempotency): privacy-map already has correct entry →
  no mutation, marker written.
- Case 5 (fresh-init): privacy-map file absent → allowlist + gitattrs
  patched, marker written.
- Case 6 (malformed JSON): broken privacy-map JSON → no marker, no
  mutation.
- Case 7 (jq mutation failure): fake jq returning 1 → no marker,
  tempfile cleaned up.
- Case 8 (allowlist append failure): read-only allowlist → no marker.

Tests use spawnSync('bash', [MIGRATION], …) with isolated tmpHomes.
"jq missing" sets PATH to a curated dir of symlinks to standard utils,
omitting jq; "jq mutation fails" uses an `exit 1` shim. Avoids
blanket-clearing PATH (which would hide bash/grep/etc).
---
 ...gstack-upgrade-migration-v1_40_0_0.test.ts | 324 ++++++++++++++++++
 1 file changed, 324 insertions(+)
 create mode 100644 test/gstack-upgrade-migration-v1_40_0_0.test.ts

diff --git a/test/gstack-upgrade-migration-v1_40_0_0.test.ts b/test/gstack-upgrade-migration-v1_40_0_0.test.ts
new file mode 100644
index 0000000000..7bea330074
--- /dev/null
+++ b/test/gstack-upgrade-migration-v1_40_0_0.test.ts
@@ -0,0 +1,324 @@
+/**
+ * gstack-upgrade/migrations/v1.40.0.0.sh — migration script unit tests.
+ *
+ * Per #1581: the original script unconditionally `touch`ed its done-marker even
+ * when the jq-gated privacy-map patch was skipped. The fix defers `touch ${DONE}`
+ * until every required repair either succeeded or was provably unnecessary.
+ *
+ * The "regression case" that this file pins is case 2: jq missing + privacy-map
+ * present → no done-marker. Against the buggy script, case 2 fails (marker is
+ * written despite skipped patch); against the fix it passes.
+ *
+ * Strategy: each test sets up an isolated tmpHome with controlled fixture
+ * content, and runs the migration via `spawnSync('bash', [MIGRATION], …)`.
+ * For "jq missing" we point PATH at a curated dir of symlinks to the standard
+ * utilities the script uses, omitting jq. For "jq mutation fails" we point PATH
+ * at a dir containing a jq shim that exits 1.
+ */
+
+import { describe, test, expect, beforeEach, afterEach } from "bun:test";
+import * as fs from "fs";
+import * as os from "os";
+import * as path from "path";
+import { spawnSync } from "child_process";
+
+const ROOT = path.resolve(import.meta.dir, "..");
+const MIGRATION = path.join(
+  ROOT,
+  "gstack-upgrade",
+  "migrations",
+  "v1.40.0.0.sh",
+);
+
+const NEW_PATTERN = "projects/*/*-eng-review-test-plan-*.md";
+const REAL_PATH = "/usr/bin:/bin:/opt/homebrew/bin";
+
+let tmpHome: string;
+let gstackHome: string;
+let migrationDir: string;
+let donePath: string;
+let allowlistPath: string;
+let privacyPath: string;
+let gitattrsPath: string;
+
+beforeEach(() => {
+  tmpHome = fs.mkdtempSync(path.join(os.tmpdir(), "gstack-mig-v1400-"));
+  gstackHome = path.join(tmpHome, ".gstack");
+  migrationDir = path.join(gstackHome, ".migrations");
+  donePath = path.join(migrationDir, "v1.40.0.0.done");
+  allowlistPath = path.join(gstackHome, ".brain-allowlist");
+  privacyPath = path.join(gstackHome, ".brain-privacy-map.json");
+  gitattrsPath = path.join(gstackHome, ".gitattributes");
+  fs.mkdirSync(gstackHome, { recursive: true });
+});
+
+afterEach(() => {
+  try {
+    fs.chmodSync(gstackHome, 0o755);
+    if (fs.existsSync(allowlistPath)) fs.chmodSync(allowlistPath, 0o644);
+    if (fs.existsSync(privacyPath)) fs.chmodSync(privacyPath, 0o644);
+    if (fs.existsSync(gitattrsPath)) fs.chmodSync(gitattrsPath, 0o644);
+    fs.rmSync(tmpHome, { recursive: true, force: true });
+  } catch {}
+});
+
+/**
+ * Construct a PATH-style directory of symlinks to standard utilities the
+ * migration script needs (mkdir, grep, sed, mv, rm, mktemp, cat, touch, printf,
+ * command, etc.). Optionally omit jq, or substitute a shim.
+ */
+function makeCuratedPath(opts: { jq?: "missing" | "shim-fail" | "real" } = {}): string {
+  const dir = fs.mkdtempSync(path.join(os.tmpdir(), "gstack-mig-path-"));
+  const utils = [
+    "bash",
+    "sh",
+    "mkdir",
+    "grep",
+    "sed",
+    "mv",
+    "rm",
+    "mktemp",
+    "cat",
+    "touch",
+    "printf",
+    "command",
+    "echo",
+    "test",
+    "[",
+    "tee",
+    "true",
+    "false",
+    "ls",
+    "chmod",
+  ];
+  const realDirs = REAL_PATH.split(":");
+  for (const u of utils) {
+    for (const d of realDirs) {
+      const src = path.join(d, u);
+      if (fs.existsSync(src)) {
+        try {
+          fs.symlinkSync(src, path.join(dir, u));
+        } catch {}
+        break;
+      }
+    }
+  }
+  const jq = opts.jq ?? "real";
+  if (jq === "real") {
+    for (const d of realDirs) {
+      const src = path.join(d, "jq");
+      if (fs.existsSync(src)) {
+        try {
+          fs.symlinkSync(src, path.join(dir, "jq"));
+        } catch {}
+        break;
+      }
+    }
+  } else if (jq === "shim-fail") {
+    const shim = path.join(dir, "jq");
+    fs.writeFileSync(
+      shim,
+      `#!/usr/bin/env bash\necho "fake jq: refusing" >&2\nexit 1\n`,
+      { mode: 0o755 },
+    );
+  }
+  // jq === "missing" → don't add anything
+  return dir;
+}
+
+function run(opts: { path?: string } = {}) {
+  const env = {
+    HOME: tmpHome,
+    PATH: opts.path ?? REAL_PATH,
+  };
+  return spawnSync("bash", [MIGRATION], {
+    env,
+    encoding: "utf-8",
+    cwd: tmpHome,
+  });
+}
+
+function freshPrivacyMap() {
+  fs.writeFileSync(
+    privacyPath,
+    JSON.stringify(
+      [{ pattern: "projects/*/*-some-other-*.md", class: "artifact" }],
+      null,
+      2,
+    ),
+  );
+}
+
+function freshAllowlist() {
+  fs.writeFileSync(
+    allowlistPath,
+    "# header\nprojects/*/*-some-other-*.md\n# ---- USER ADDITIONS BELOW\n",
+  );
+}
+
+function freshGitattrs() {
+  fs.writeFileSync(gitattrsPath, "projects/*/*-some-other-*.md merge=union\n");
+}
+
+describe("migrations/v1.40.0.0.sh", () => {
+  test("case 1: jq present, fresh privacy-map — all three files patched, marker written", () => {
+    freshAllowlist();
+    freshPrivacyMap();
+    freshGitattrs();
+
+    const r = run();
+
+    expect(r.status).toBe(0);
+    expect(fs.existsSync(donePath)).toBe(true);
+
+    const allowlist = fs.readFileSync(allowlistPath, "utf-8");
+    expect(allowlist).toContain(NEW_PATTERN);
+
+    const privacy = JSON.parse(fs.readFileSync(privacyPath, "utf-8"));
+    expect(
+      privacy.some(
+        (e: any) => e.pattern === NEW_PATTERN && e.class === "artifact",
+      ),
+    ).toBe(true);
+
+    const gitattrs = fs.readFileSync(gitattrsPath, "utf-8");
+    expect(gitattrs).toContain(`${NEW_PATTERN} merge=union`);
+  });
+
+  test("case 2 (regression for #1581): jq missing, privacy-map exists — marker NOT written, text patches still applied", () => {
+    freshAllowlist();
+    freshPrivacyMap();
+    freshGitattrs();
+
+    const noJq = makeCuratedPath({ jq: "missing" });
+    const r = run({ path: noJq });
+
+    expect(r.status).toBe(0);
+    expect(r.stderr).toMatch(/jq not found/);
+
+    // Done-marker must NOT be written — this is the whole point of the fix.
+    expect(fs.existsSync(donePath)).toBe(false);
+
+    // Text-only patches still landed (they don't need jq).
+    expect(fs.readFileSync(allowlistPath, "utf-8")).toContain(NEW_PATTERN);
+    expect(fs.readFileSync(gitattrsPath, "utf-8")).toContain(
+      `${NEW_PATTERN} merge=union`,
+    );
+
+    // Privacy-map untouched (still missing the new entry).
+    const privacy = JSON.parse(fs.readFileSync(privacyPath, "utf-8"));
+    expect(privacy.some((e: any) => e.pattern === NEW_PATTERN)).toBe(false);
+  });
+
+  test("case 3: jq missing, then jq restored — second run completes patch and writes marker", () => {
+    freshAllowlist();
+    freshPrivacyMap();
+    freshGitattrs();
+
+    // First run with jq missing
+    const noJq = makeCuratedPath({ jq: "missing" });
+    const r1 = run({ path: noJq });
+    expect(r1.status).toBe(0);
+    expect(fs.existsSync(donePath)).toBe(false);
+
+    // Second run with jq restored
+    const r2 = run();
+    expect(r2.status).toBe(0);
+    expect(fs.existsSync(donePath)).toBe(true);
+
+    const privacy = JSON.parse(fs.readFileSync(privacyPath, "utf-8"));
+    expect(
+      privacy.some(
+        (e: any) => e.pattern === NEW_PATTERN && e.class === "artifact",
+      ),
+    ).toBe(true);
+  });
+
+  test("case 4: jq present, privacy-map already has correct entry — idempotent, marker written", () => {
+    freshAllowlist();
+    fs.writeFileSync(
+      privacyPath,
+      JSON.stringify(
+        [{ pattern: NEW_PATTERN, class: "artifact" }],
+        null,
+        2,
+      ),
+    );
+    freshGitattrs();
+
+    const r = run();
+    expect(r.status).toBe(0);
+    expect(fs.existsSync(donePath)).toBe(true);
+
+    const privacy = JSON.parse(fs.readFileSync(privacyPath, "utf-8"));
+    const matches = privacy.filter((e: any) => e.pattern === NEW_PATTERN);
+    expect(matches.length).toBe(1);
+    expect(matches[0].class).toBe("artifact");
+  });
+
+  test("case 5: jq present, privacy-map file missing — allowlist + gitattrs patched, marker written", () => {
+    freshAllowlist();
+    // No privacy-map file
+    freshGitattrs();
+
+    const r = run();
+    expect(r.status).toBe(0);
+    expect(fs.existsSync(donePath)).toBe(true);
+    expect(fs.existsSync(privacyPath)).toBe(false);
+
+    expect(fs.readFileSync(allowlistPath, "utf-8")).toContain(NEW_PATTERN);
+    expect(fs.readFileSync(gitattrsPath, "utf-8")).toContain(
+      `${NEW_PATTERN} merge=union`,
+    );
+  });
+
+  test("case 6: jq present, privacy-map JSON malformed — no marker, error logged, no mutation", () => {
+    freshAllowlist();
+    fs.writeFileSync(privacyPath, "{ this is not json [");
+    freshGitattrs();
+
+    const r = run();
+    expect(r.status).toBe(0);
+    // No marker — broken JSON should NOT be papered over.
+    expect(fs.existsSync(donePath)).toBe(false);
+    // Privacy-map content untouched.
+    expect(fs.readFileSync(privacyPath, "utf-8")).toBe("{ this is not json [");
+  });
+
+  test("case 7: jq present but mutation fails (shim exit 1) — no marker, tempfile cleaned up", () => {
+    freshAllowlist();
+    freshPrivacyMap();
+    freshGitattrs();
+
+    const fakeJq = makeCuratedPath({ jq: "shim-fail" });
+    const r = run({ path: fakeJq });
+
+    expect(r.status).toBe(0);
+    expect(fs.existsSync(donePath)).toBe(false);
+
+    // Tempfile cleanup: no leftover *.tmp.* sidecars.
+    const leftovers = fs
+      .readdirSync(gstackHome)
+      .filter((n) => n.startsWith(".brain-privacy-map.json.tmp."));
+    expect(leftovers.length).toBe(0);
+  });
+
+  test("case 8: allowlist append fails (read-only file, no USER ADDITIONS marker) — no marker, warn logged", () => {
+    // Allowlist WITHOUT the "# ---- USER ADDITIONS BELOW" marker — the script
+    // falls into the plain `printf >>` append path. Make the file read-only
+    // so the append fails (sed -i.bak on macOS silently no-ops on read-only
+    // files, so we have to take the printf path to exercise this).
+    fs.writeFileSync(
+      allowlistPath,
+      "# header\nprojects/*/*-some-other-*.md\n",
+    );
+    freshPrivacyMap();
+    freshGitattrs();
+    fs.chmodSync(allowlistPath, 0o444);
+
+    const r = run();
+    expect(r.status).toBe(0);
+    // Marker must NOT be written when a required repair failed.
+    expect(fs.existsSync(donePath)).toBe(false);
+  });
+});