fix(store): enforce shell-stream authority for sidebar summary flags (upstream #1996) - #70
Merged
Conversation
…(upstream pingdotgg#1996) Removes the client-derived buildSidebarThreadSummary path that was overwriting server-authoritative sidebar flags during the window between a detail-stream write and the next shell event. Matches the stream- separation contract established in upstream pingdotgg#1996 and required by MEMORY.md ("ghost Pending Approval badges on resolved threads"). Changes in apps/web/src/store.ts: - Delete buildSidebarThreadSummary() + getLatestUserMessageAt() (only used by the broken path). - Remove session-logic imports: derivePendingApprovals, derivePendingUserInputs, findLatestProposedPlan, hasActionableProposedPlan (their tests in session-logic.test.ts still exercise them). - writeThreadState (detail stream): stop computing nextSummary; drop the sidebarThreadSummariesEqual block. Detail stream no longer touches sidebarThreadSummaryById. - buildThreadState (legacy read-model path): drop sidebarThreadSummaryById from the return type Pick; stop populating it. The shell stream is the sole writer. - Add doc comments to writeThreadState + writeThreadShellState explaining stream ownership, mirroring upstream pingdotgg#1996's guidance. Changes in apps/web/src/store.test.ts: - Update the "shell events are authoritative for sidebar summary flags" describe block's two tests to reflect the new contract: detail-stream events do NOT populate sidebarThreadSummaryById; shell events are the sole writer. Extended both tests to also verify the later-shell-event transition (true → false) still wins authoritatively. What we deliberately did NOT port from upstream pingdotgg#1996: - Structural reorganization of store.ts (initialEnvironmentState commentary, ensureThreadRegistered helper extraction). MarCode's Incremental Event Handling & Structural Sharing (FEATURES.md §1) already covers this functionally via syncServerReadModel. Dragging in upstream's cosmetic reshuffle risks regression for no behavioral win.
- Split upstream pingdotgg#1996 into two ledger entries for the two MarCode PRs that port it: PR #69 (sidebar row timestamp behavioral fix, 524e93a) and PR #70 (store.ts shell-stream-authority correctness refactor, 506b808). - Remove "NOT yet equivalent" note in the Already-equivalent section now that both halves are ported. - Drop pingdotgg#1996 from Pending real work; only pingdotgg#2246 remains. - Add plan-file pointer (/Users/tyulyukov/.claude/plans/cached-napping-sundae.md) to the pingdotgg#2246 row so the next cycle's porter has the staged-commit strategy. Corrects the earlier claim that pingdotgg#1996's store.ts refactor was "cleanup with no behavioral delta" — it's a real correctness fix aligning MarCode with the stream-separation contract MEMORY.md already requires.
3 tasks
# Conflicts: # UPSTREAM_DIVERGENCE.md
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Summary
Correctness fix. Removes the client-derived
buildSidebarThreadSummarypath that was re-deriving server-authoritative sidebar flags (hasPendingApprovals,hasPendingUserInput,hasActionableProposedPlan,latestUserMessageAt) from detail state during everywriteThreadState/ legacy read-model sync — overwriting the shell stream's server-computed values in the window before the next shell event arrives.This is exactly the bug MEMORY.md warned against ("ghost 'Pending Approval' badges on resolved threads"), which upstream #1996 fixes by partitioning write ownership between the shell stream and the detail stream.
Why this was missed
My UPSTREAM_DIVERGENCE.md and PR #69 both claimed the
store.tschanges in upstream pingdotgg#1996 were "cleanup with no behavioral delta." Wrong. The describe blockstore.test.ts:1345"shell events are authoritative for sidebar summary flags" already encodes the right invariant, but the two tests inside it were relying onapplyOrchestrationEvent("thread.activity-appended", …)populatingsidebarThreadSummaryByIdviabuildSidebarThreadSummary— i.e., they were asserting the buggy intermediate state.Changes
apps/web/src/store.tsbuildSidebarThreadSummary()(lines 319-340 pre-fix) andgetLatestUserMessageAt()(its only caller)../session-logic:derivePendingApprovals,derivePendingUserInputs,findLatestProposedPlan,hasActionableProposedPlan. (Their own unit tests insession-logic.test.tsstill exercise them — keep the functions, stop importing them here.)writeThreadState(detail-stream writer): dropnextSummarycomputation +sidebarThreadSummariesEqual-guarded write block. Add doc comment documenting stream ownership.buildThreadState(legacy read-model full rebuild): dropsidebarThreadSummaryByIdfrom the return-typePickand stop populating it. The shell stream is the sole writer;syncServerReadModel(MarCode-exclusive fallback) preserves any existing sidebar summary via its...statespread.writeThreadShellState: add doc comment mirroring upstream's stream-ownership guidance.apps/web/src/store.test.tsThe two tests inside
describe("shell events are authoritative for sidebar summary flags")are rewritten to match the corrected contract:sidebarThreadSummaryById. Afterthread.activity-appended/thread.proposed-plan-upserted,selectSidebarThreadSummaryByRefreturnsundefined— the detail stream did nothing.makeThreadUpsertedShellEvent(thread, { hasPendingUserInput: true })sets the flag on;{ hasPendingUserInput: false }clears it.What I deliberately did NOT port from upstream pingdotgg#1996
store.ts(theinitialEnvironmentStatecommentary block, theensureThreadRegisteredhelper extraction into a named fn, theretainThreadScopedRecordhelper changes). MarCode's Incremental Event Handling & Structural Sharing (FEATURES.md §1) already covers this functionally viasyncServerReadModel. Dragging in upstream's cosmetic reshuffle risks regression for no behavioral win.Test plan
bun run typecheck— clean monorepo-wideapps/webtest suite — 1095 / 1095 passing (including the rewritten regression-guard describe block)oxlint— 0 errors (67 pre-existing warnings)