fix(desktop): skip archived session queries - #4431
Phoenix500526 wants to merge 1 commit into
Conversation
|
@Astro-Han This PR fixes a concrete bug, and AppShell is currently the only composition point that holds both the authoritative archived state and the pending Archive/Delete state, so keeping the small query gate there is the simplest owner-correct fix. The monotonic token ratchet blocks even this composition-only change; moving it into a new helper or controller would add indirection and could make ownership worse, so what path did you intend for legitimate bug fixes that need small AppShell wiring without gaming the metric? |
8f1760f to
2dbcff1
Compare
|
Hi, @chihumyum This PR currently fails the AppShell architecture ratchet because a narrow bug fix adds query-gate wiring at the composition point. Given the migration tracked in #3439 and the guardrail introduced by #4088, should we hold this fix until the Conversation/AppShell ownership work provides its intended home, or is there an accepted way to land this small wiring now without mechanically moving it elsewhere just to satisfy the ledger? |
Thanks for raising this. Please do not hold the bug fix until the full Conversation/AppShell migration, and please do not raise the AppShell token baseline. |
2dbcff1 to
e1f0530
Compare
Done |
e1f0530 to
ff532da
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The ownership shape is right: session-catalog-state.ts is the single authority for both halves of the gate (the archived catalog fact and the refcounted Archive/Delete leases), Skills and Plan only consume isAutomaticQueryBlocked, Session Navigation takes the lease over the revision family around the Host mutation, and AppShell actually loses its pending Set instead of gaining wiring. session-query-gate.test.ts drives the real controller and covers nesting, in-flight fencing, and refresh after unblock. No P0/P1. Three things before merge.
P2: the branch is behind main and conflicts in four files (apps/desktop/e2e/code-scroll.spec.ts, apps/desktop/renderer-architecture.json, session-row-actions.ts, use-session-navigation-controller.ts). Please rebase and regenerate the architecture ledger; CI here passed on a stale base.
On user-visible behavior for archived tasks, I checked the reachable surfaces and the skip costs nothing there: the rail filters through sessionMatchesRail, the command palette skips archived rows, and Settings has restore/delete/purge but no open, so an archived task can never be the composer's session by a normal path. The one place it shows is when the active session is archived from outside the renderer (a Host-side family archive, or a second window): composer-mentions never settles, so liveCatalog stays loading: true, settled: undefined and the plus menu's Skills row goes permanently inert instead of settling empty. On main that same case settles empty after the Host error (and prints the log from #4430). Both are bad, this one is quieter, so I am not asking for a change, but a follow-up worth noting.
Remaining P3s are shape, not correctness: see the inline notes on the bootstrap filter, the synthetic state replacement, and the two spellings of the lease.
44d7e3d to
854f475
Compare
3007c56 to
18a3a89
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-read at 18a3a89 (the squashed push of 2026-09-06) against main (a03dd577bb). The content is what I reviewed on 2026-09-03 and it still holds: the archived fact and the lease count live together in session-catalog-state.ts, Skills and Plan only consume isAutomaticQueryBlocked, the lease is released in finally after refreshSessions() commits, and pendingSessionRowActionsRef leaves AppShell for the navigation controller, which is a net reduction there. Nothing new to ask for.
The one thing still standing is the one from last time: the branch conflicts with main on apps/desktop/renderer-architecture.json, so the green CI here ran on a stale base and the ratchet has not been exercised against the ledger it would actually merge into. Rebase once more and regenerate the ledger; if the AppShell ratchet then passes, this is ready.
Evidence boundary: static read; git merge-tree against current main is what reports the conflict.
18a3a89 to
c854526
Compare
Done |
Automatic Skills and Plan refreshes could reach the Runtime Host after a session entered archival, producing avoidable session errors. CLOSES apache#4430 Signed-off-by: Jiawei Zhao <Phoenix500526@163.com> Generated-by: OpenAI Codex
c854526 to
feb955a
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the rebase. Re-reviewed at head feb955a. The tree is identical to 18a3a89 apart from the base, so my earlier read still holds: the archived fact and the Archive/Delete lease live together in session-catalog-state.ts, navigation takes the lease by revision family and releases it in finally, Skills and Plan consume only isAutomaticQueryBlocked, and AppShell shrinks. That is the shape chihumyum asked for. The seven touched test files pass here (45/45) along with lint and format.
On the CI failure: the only red is one Electron e2e, workhub-layout.spec.ts:295 (second queued prompt not rendered under next_turn). This PR doesn't touch the composer queue, the assertion is unchanged on main, and main is green, so I read it as a flake, though I couldn't reproduce it locally either way. A rerun after rebase will tell.
What has changed since last time is main, and that needs one more pass:
- The base is 78 commits behind and
git merge-treereports real conflicts insession-row-actions.ts(touched by #5229),use-session-navigation-controller.ts, two navigation tests, and the generatedrenderer-architecture.json. ThewithAutomaticQueryBlockwrapping needs to be re-placed onmain's current shape rather than resolved mechanically, then the ledger regenerated. The AppShell ratchet should still come out belowmain's 13381 (this PR is net −2 tokens, −1useRef), but that has to be measured after rebase. mainalready fixed the bootstrap half of #4430 in #4980 (bootstrap-selection-lease.ts:44). This PR doesn't touch that source file any more, only its test, so the Summary line about excluding archived sessions from bootstrap selection describes work that is now upstream. See the inline note on the test.
Manual acceptance that remains: after rebase, walk #4430's steps (archive the selected session, watch for plan.query / session_archived and skill.catalog.invocable.query / internal_failure in the Host log) and confirm both are gone. No visual change, so no light/dark comparison needed. Separately, the Host returning a generic internal_failure for the skill query on an archived session is a Host-side classification gap worth its own issue; this PR only sidesteps it in the renderer.
AI assistance: I used Claude Code to read the CI log and run the suites; conclusions checked by me.
| expected: undefined, | ||
| }, | ||
| ] as const) { | ||
| it(`skips archived sessions when ${name}`, () => { |
There was a problem hiding this comment.
P2. The archived filter this exercises landed on main in #4980, and two of the three cases here duplicate skips archived rows when choosing the first unarchived conversation and leaves the new-task surface selected when all history is archived that main already has. They pass on main unchanged. The bootstrap-owned selection is archived case is the one new angle; keep that and drop the other two, and adjust the Summary so the checklist's "fail without it" claim matches.
| return; | ||
| } | ||
| await refresh(); | ||
| await refresh({ automatic: false }); |
There was a problem hiding this comment.
P3. The automatic path now has the requestVersion fence, but this user-mutation readback runs with the default isCurrent, so if the user approves and switches session before getPlanState returns, the stale result still lands in the new session's hook state. Pre-existing, but since the fence is right there, threading the same version check through run() would make the two paths consistent.
Summary
Prevent archived sessions from remaining automatic query targets by:
This reuses the existing pending row-action state, so renderer cleanup still happens only after the Runtime Host confirms the operation.
Fixes #4430
Verification
npm --workspace @maka/desktop run build:main— passednpm --workspace @maka/desktop run typecheck— passednpm run check:app-shell-hooks— passednpm --workspace @maka/desktop run check:architecture— passed (60 tests)AI use
Select exactly one:
Tool(s) and scope:
OpenAI Codex assisted with diagnosis, implementation, regression tests, and code review.
Checklist
Does this PR entail a change in behavior?