Skip to content

Commit 790769c

Browse files
lukstaficlaude
andcommitted
fix(mag): guard /compact enqueue from feedback-digest throw (PR #494 review)
Wrap the tryQueueFeedbackDigest("ludics") call in magBriefing() with try/catch so a queue-lock timeout or state-file write failure inside the digest path can no longer suppress the unconditional /compact enqueue. The auto-compact-after-checkpoints contract (task-a00fc0d9) requires /compact after every briefing; the round-1 swap exposed that contract to digest-side failures because /compact moved below the digest call. Add a regression test that spies on queueRequest to throw on the feedback-digest action and asserts (a) /compact still lands in mag/queue.jsonl as the second entry, and (b) the failure is logged. Mutation-verified: removing the try/catch makes the new test fail. Addresses Codex review comment on src/mag.ts:3320. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ebb5630 commit 790769c

2 files changed

Lines changed: 54 additions & 4 deletions

File tree

src/mag-auto-compact.test.ts

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
1+
import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test";
22
import { mkdirSync, readFileSync, writeFileSync } from "fs";
33
import { join } from "path";
44
import { withSyntheticHarness } from "./test-utils.ts";
@@ -71,6 +71,48 @@ describe("magBriefing auto-compact follow-up", () => {
7171
expect(items[items.length - 1]!.action).toBe("message");
7272
expect(items[items.length - 1]!.content).toBe("/compact");
7373
});
74+
75+
test("/compact still enqueues when feedback-digest enqueue throws", async () => {
76+
// Harness condition: spy on queue.queueRequest to throw on the
77+
// feedback-digest action only (simulating a queue-lock timeout or
78+
// state-file write failure inside tryQueueFeedbackDigest). Without the
79+
// try/catch around the digest call, the throw would propagate out of
80+
// magBriefing before /compact is enqueued, leaving the queue with only
81+
// the briefing entry. Mutation: removing the try/catch makes this test
82+
// fail because /compact never lands and `errSpy` never sees the warning.
83+
const queueMod = await import("./queue.ts");
84+
const origQueueRequest = queueMod.queueRequest;
85+
let calls = 0;
86+
const requestSpy = spyOn(queueMod, "queueRequest").mockImplementation(
87+
((req: Parameters<typeof origQueueRequest>[0]) => {
88+
calls++;
89+
if (req.action === "feedback-digest") {
90+
throw new Error("simulated queue-lock timeout");
91+
}
92+
return origQueueRequest(req);
93+
}) as typeof origQueueRequest,
94+
);
95+
const errSpy = spyOn(console, "error").mockImplementation(() => {});
96+
97+
try {
98+
const { magBriefing } = await import("./mag.ts");
99+
magBriefing(false);
100+
101+
const items = readQueue();
102+
// briefing was queued (call 1), feedback-digest threw (call 2),
103+
// /compact still queued (call 3). On disk: briefing + /compact only.
104+
expect(calls).toBeGreaterThanOrEqual(3);
105+
expect(items).toHaveLength(2);
106+
expect(items[0]!.action).toBe("briefing");
107+
expect(items[1]!.action).toBe("message");
108+
expect(items[1]!.content).toBe("/compact");
109+
const errLines = errSpy.mock.calls.map((c) => String(c[0] ?? ""));
110+
expect(errLines.some((l) => l.includes("feedback-digest enqueue failed"))).toBe(true);
111+
} finally {
112+
requestSpy.mockRestore();
113+
errSpy.mockRestore();
114+
}
115+
});
74116
});
75117

76118
describe("runMag health-check auto-compact follow-up", () => {

src/mag.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3317,9 +3317,17 @@ export function magBriefing(wait: boolean = true, timeout: number = 300): void {
33173317
// Auto-queue feedback-digest once daily alongside the briefing trigger.
33183318
// Lands before /compact so digest can consume the briefing's in-flight
33193319
// context (task-304a02a6 / docs/proposals/task-304a02a6-reorder-briefing-auto-queue.md).
3320-
const fdResult = tryQueueFeedbackDigest("ludics");
3321-
if (fdResult.queued) {
3322-
console.error("ludics: briefing queued feedback-digest for ludics");
3320+
// Wrapped so a digest enqueue failure (queue-lock timeout, state-file write
3321+
// error) cannot suppress the unconditional /compact below — the auto-compact
3322+
// contract (task-a00fc0d9) requires /compact after every briefing.
3323+
try {
3324+
const fdResult = tryQueueFeedbackDigest("ludics");
3325+
if (fdResult.queued) {
3326+
console.error("ludics: briefing queued feedback-digest for ludics");
3327+
}
3328+
} catch (err) {
3329+
const msg = err instanceof Error ? err.message : String(err);
3330+
console.error(`ludics: feedback-digest enqueue failed: ${msg}`);
33233331
}
33243332

33253333
// Auto-compact after briefing — checkpoint compaction (task-a00fc0d9 /

0 commit comments

Comments
 (0)