From 225e6e4ccdaa86bee526198d5e71524ba4f95c90 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 11:54:18 -0700 Subject: [PATCH] fix(decision-log): --supersede silently discarded the replacement decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The supersede/redact branch appended the retirement event and exited before the JSON argument was ever read — a user recording a reversal WITH its replacement lost the replacement, and the payload finder's first-non-flag-arg predicate would have mistaken the target id for JSON anyway. Payloads are now identified by their leading brace, validated BEFORE any write, and appended FIRST (retirement second), so the only visible interleaving under a crash is both-active — recoverable, never lost. The replacement carries supersedes: provenance. Bare --supersede (the documented reversal-without-replacement) stays legal; --redact with a payload now refuses instead of dropping it. Ported from time-attack/gstack (GStack 2), tests included. Co-authored-by: Sina Matian Co-Authored-By: Claude Fable 5 --- bin/gstack-decision-log | 59 ++++++++++++++++++++++--------- test/gstack-decision-bins.test.ts | 33 +++++++++++++++++ 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/bin/gstack-decision-log b/bin/gstack-decision-log index 17708980b..6ee838955 100755 --- a/bin/gstack-decision-log +++ b/bin/gstack-decision-log @@ -51,39 +51,64 @@ if (args.includes("--compact")) { process.exit(0); } +// The payload is identified by its leading `{`, not by "first non-flag arg" — a +// `--supersede '{...}'` call would otherwise mistake the target id for the payload. +const jsonArg = args.find((a) => a.trimStart().startsWith("{")); + +/** Parse + validate a decision payload. Exits 1 (nothing persisted) when it's bad. */ +function validPayload(raw: string): DecisionEvent { + let obj: Partial; + try { + obj = JSON.parse(raw); + } catch { + process.stderr.write("gstack-decision-log: invalid JSON\n"); + process.exit(1); + } + if (obj.scope === "branch" && !obj.branch) obj.branch = gitBranch(); + const res = validateDecide(obj); + if (!res.ok) { + process.stderr.write(`gstack-decision-log: ${res.error}\n`); + process.exit(1); + } + return res.event; +} + const supersedeId = flagValue(args, "--supersede"); const redactId = flagValue(args, "--redact"); if (supersedeId || redactId) { const kind = supersedeId ? "supersede" : "redact"; const targetId = (supersedeId || redactId) as string; + if (targetId.trimStart().startsWith("{")) { + process.stderr.write(`gstack-decision-log: --${kind} needs the target decision id before the replacement JSON\n`); + process.exit(1); + } + if (kind === "redact" && jsonArg) { + process.stderr.write( + "gstack-decision-log: --redact expunges and takes no replacement; log the replacement in its own call so it isn't dropped\n", + ); + process.exit(1); + } + // Validate the replacement BEFORE anything is written, then append it FIRST and + // retire the old one SECOND. Appends are individually atomic, so the only visible + // interleaving is "both active" (recoverable); the reverse order could retire the + // old decision and lose the replacement the user was recording. + const replacement = jsonArg ? { ...validPayload(jsonArg), supersedes: targetId } : undefined; + if (replacement) appendEvent(paths, replacement); appendEvent(paths, makeRefEvent(kind, targetId, { source: "agent" })); rebuildSnapshot(paths); enqueue(); - console.log(`${kind}: ${targetId}`); + console.log(replacement ? `${kind}: ${targetId} -> ${replacement.id}` : `${kind}: ${targetId}`); process.exit(0); } -const jsonArg = args.find((a) => !a.startsWith("--")); if (!jsonArg) { process.stderr.write( "gstack-decision-log: provide a JSON decision, or --supersede/--redact , or --compact\n", ); process.exit(1); } -let obj: Partial; -try { - obj = JSON.parse(jsonArg); -} catch { - process.stderr.write("gstack-decision-log: invalid JSON\n"); - process.exit(1); -} -if (obj.scope === "branch" && !obj.branch) obj.branch = gitBranch(); -const res = validateDecide(obj); -if (!res.ok) { - process.stderr.write(`gstack-decision-log: ${res.error}\n`); - process.exit(1); -} -appendEvent(paths, res.event); +const event = validPayload(jsonArg); +appendEvent(paths, event); rebuildSnapshot(paths); enqueue(); -console.log(res.event.id); +console.log(event.id); diff --git a/test/gstack-decision-bins.test.ts b/test/gstack-decision-bins.test.ts index 219dbe9b2..5ae6155cd 100644 --- a/test/gstack-decision-bins.test.ts +++ b/test/gstack-decision-bins.test.ts @@ -63,6 +63,39 @@ describe("gstack-decision-log", () => { const r = log("not json", true); expect(r.code).toBe(1); }); + test("--supersede with a replacement body records the replacement, linked to the old id", () => { + const id = log('{"decision":"old-call","scope":"repo","source":"user"}').out; + const out = logFlag( + `--supersede ${id} '{"decision":"new-call","rationale":"better","scope":"repo","source":"user"}'`, + ); + expect(out).toContain(id); + expect(search()).toContain("new-call"); // the replacement is NOT silently dropped + expect(search()).not.toContain("old-call"); + const arr = JSON.parse(search("--json")); + expect(arr.find((d: any) => d.decision === "new-call")?.supersedes).toBe(id); + }); + test("--supersede with an INVALID replacement persists nothing (old stays active)", () => { + const id = log('{"decision":"keep-me","scope":"repo","source":"user"}').out; + let code = 0; + try { + logFlag(`--supersede ${id} '{"decision":""}'`); + } catch (e: any) { + code = e.status || 1; + } + expect(code).toBe(1); + expect(search()).toContain("keep-me"); // not retired by a failed replacement + }); + test("--redact refuses a replacement body instead of dropping it", () => { + const id = log('{"decision":"redact-target","scope":"repo","source":"user"}').out; + let code = 0; + try { + logFlag(`--redact ${id} '{"decision":"would-be-lost","scope":"repo","source":"user"}'`); + } catch (e: any) { + code = e.status || 1; + } + expect(code).toBe(1); + expect(search()).toContain("redact-target"); // nothing happened at all + }); }); describe("gstack-decision-search", () => {