fix(#89): recalc review fixes (#128) — notify reload, SSE shape, manifest-less root guard - #129
Merged
Merged
Conversation
…ad, SSE shape, root guard Three review findings on PR #128, failing-first: - notify handler ignores kind="recalc": an MCP-driven recalc never reloads buckaroo sessions or invalidates the companion LRUs, unlike /api/recalc. - recalc SSE event shape drifts: /api/recalc emits {kind,remap,step}; the cross-process notify path emits payload.model_dump() (remap under extra, no step). Pins a shared _recalc_sse_event normalizer + the MCP _notify forwarding the checkpoint step. - recalc()'s root guard checks the entry DIRECTORY, but the walk indexes verdicts (manifest-keyed); a dir-without-manifest root KeyErrors the walk. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lc SSE shape, guard manifest-less roots (#128) Three review findings on PR #128: 1. /internal/notify now handles kind="recalc" like project_reset — invalidate the result/compare LRUs and reload buckaroo sessions. An MCP-driven recalc (the primary path) arrives via _notify, so without this the companion kept serving stale viewer state that only the in-process /api/recalc route cleaned up (#80's communication-gap class). 2. Both recalc emitters publish through a single _recalc_sse_event(remap, step) helper, so the SSE event is the same {kind, remap, step} shape regardless of surface. The MCP _notify now forwards report.checkpoint_step too; the notify handler republishes normalized instead of the raw payload.model_dump() that buried remap under extra and dropped step. 3. recalc()'s root guard checks the manifest, not the directory. verdicts (and the cone) are keyed by manifest-bearing entries, so a dir-without-manifest root (a crashed mid-build leftover) used to enter the cone and KeyError the walk at verdicts[hash]; it is now dropped and the run reports clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The three review findings on PR #128 that missed its merge. #128 (reactive recalc,
stage 2/3) merged at
c212a92; these follow-up fixes were committed only on thelocal branch afterward, so they never reached
main. This PR brings them on as afocused, TDD-ordered pair — failing tests first, then the fix.
What lands
/internal/notifyhandleskind="recalc": it invalidates the result/compare LRUs and reloads buckaroosessions — the same cleanup
/api/recalcand/api/resetalready do — so a recalcdriven from the MCP process doesn't leave the separate companion process serving
stale state (caching: reset-to-revision doesn't invalidate the in-process cached_result_expr LRU — a warmed expensive entry then reads a pruned snapshot path and fails #80's cross-process gap).
_recalc_sse_event(remap, step)normalizer, used by both the in-process
/api/recalcroute and the cross-process/internal/notifyrepublish, so the event a frontend sees can't depend on whichsurface emitted it. (The notify path previously republished the raw payload,
burying
remapunderextraand droppingstep.) The MCP_notify("recalc", …)now forwards the checkpoint
steptoo.recalc(). Drop roots whose entry dir has nomanifest.json(a crashed mid-build leftover, or a caller typo) by gating on themanifest rather than the directory — the walk indexes manifest-keyed verdicts, so a
dir-without-manifest root would
KeyErrorthe walk.TDD
First commit (
69a4bb4) is the three findings as failing tests; the fix commitfollows. The five tests fail on
main's code and pass after the fix; pushedtest-first so the failure is visible on CI before the fix lands.
🤖 Generated with Claude Code