Skip to content

fix(mag): briefing queues /compact after feedback-digest (task-304a02a6)#494

Merged
lukstafi merged 2 commits intomainfrom
ludics/task-304a02a6-s1/root
May 4, 2026
Merged

fix(mag): briefing queues /compact after feedback-digest (task-304a02a6)#494
lukstafi merged 2 commits intomainfrom
ludics/task-304a02a6-s1/root

Conversation

@lukstafi
Copy link
Copy Markdown
Owner

@lukstafi lukstafi commented May 4, 2026

Summary

  • Swap order in magBriefing() (src/mag.ts) so the on-disk queue becomes briefing → feedback-digest → /compact. /compact had been wiping the briefing's in-flight context before feedback-digest could read it; now /compact lands last so digest sees the prior context and the next session starts fresh.
  • Rewrite the comment block above the /compact enqueue: drops the now-false "directly behind" / "before any later automated enqueues" rationale, names the actual reason ("LAST follow-up so the next session starts fresh"), and keeps the task-a00fc0d9 / auto-compact-after-checkpoints.md reference (auto-compact contract is unchanged).
  • Rename the regression test in src/mag-auto-compact.test.ts to assert the new invariant (/compact is the final queue item, feedback-digest sits between briefing and /compact when ungated). Add a sibling negative-control test that pre-seeds a pending feedback-digest to exercise the gated branch and confirm /compact still lands last (queue length 2).
  • Round 2 (Codex review): Wrap the tryQueueFeedbackDigest("ludics") call in try/catch so a queue-lock timeout or state-file write failure in the digest path cannot suppress the unconditional /compact enqueue (the auto-compact-after-checkpoints contract requires /compact after every briefing). Add a regression test that spies on queueRequest to throw on the feedback-digest action and asserts /compact still lands; mutation-verified by removing the try/catch.
  • magHealthCheck's /compact enqueue (src/mag.ts:3485) is untouched — it has no co-queued digest, so the ordering bug doesn't apply there.

Proposal: docs/proposals/task-304a02a6-reorder-briefing-auto-queue.md

Test plan

  • bun run typecheck
  • bun run lint
  • bun run build
  • bun test src/mag-auto-compact.test.ts — 8/8 pass (covers ungated 3-item ordering, gated 2-item ordering, digest-throws-still-enqueues-/compact, and the unchanged health-check path)

🤖 Generated with Claude Code

Swap order in magBriefing() so the on-disk queue becomes
briefing → feedback-digest → /compact. /compact had been wiping the
briefing context before feedback-digest could read it; now /compact
lands last so digest sees prior context and the next session starts
fresh. Comment block above /compact rewritten to drop the now-false
"directly behind" rationale; task-a00fc0d9 reference preserved.

Test renamed to assert the new invariant (last item is /compact,
middle item is feedback-digest in clean state) and a sibling test
covers the gated branch (length 2 with /compact still last).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ebb56301fb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/mag.ts Outdated
// Auto-queue feedback-digest once daily alongside the briefing trigger.
// Lands before /compact so digest can consume the briefing's in-flight
// context (task-304a02a6 / docs/proposals/task-304a02a6-reorder-briefing-auto-queue.md).
const fdResult = tryQueueFeedbackDigest("ludics");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard /compact enqueue from feedback-digest failures

magBriefing now calls tryQueueFeedbackDigest("ludics") before queuing /compact, but that helper can throw (for example, queue lock timeout in queueRequest or state-file write failures in markFeedbackDigestQueued). In those cases the function exits before enqueuing /compact, which breaks the stated “always compact after briefing” contract and can leave the next session with stale context. The /compact enqueue should be made failure-safe (e.g., via try/finally) so digest errors do not suppress compaction.

Useful? React with 👍 / 👎.

…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>
@lukstafi
Copy link
Copy Markdown
Owner Author

lukstafi commented May 4, 2026

note: branch state has drifted since this body was written (baseline: 1 commit at 2026-05-04T05:23:01Z, current: 2 commits). consider gh pr edit https://github.com/lukstafi/ludics/pull/494 to refresh.

@lukstafi lukstafi merged commit 3aa2088 into main May 4, 2026
1 check passed
@lukstafi lukstafi deleted the ludics/task-304a02a6-s1/root branch May 4, 2026 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant