Repository navigation
Conversation
A finished background task stayed on the native-chat strip as "running" forever when its pane was hidden while the task ended: the idle sweep then stopped the provider child, so the adapter answered `undefined` for `backgroundTaskState`. The subscribe/history path omitted the field on `undefined`, and the renderer reducer reads a missing field as "no change" - the stale monitoring state survived every resume, and Stop failed against a process that no longer knew the task id. The channel now folds `undefined` into `null` for subscribe and history reads: a conversation opened at rest has no live provider child, so it has no live background tasks. A resuming renderer receives the empty state and clears its copy; a live child still passes its real state through untouched. `publish` keeps its own edge-driven semantics. Fixes stablyai#24227 Generated-by: GLM-5.3-Flash (ZCode)
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| close() | ||
|
|
||
| expect(batch?.type).toBe('batch') | ||
| expect(batch?.backgroundTasks).toBe(null) |
There was a problem hiding this comment.
backgroundTasks: null, but it never applies that batch to renderer state that already contains a monitoring task. That is the reported failure path, so a future change could leave the task on the strip while these tests still pass. Please assert that the emitted batch clears an existing task.
Knowledge Base Used: Agent workspaces and sessions
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fair gap - the channel tests only pin the host's half of the contract. Added the renderer half in src/shared/structured-agent-session-reducer.test.ts ("reads an omitted batch field as no change and an explicit null as the empty roster"): a state already holding a monitoring roster survives a batch that omits the field (identity no-op), and the same state clears its roster - backgroundTasks becomes null with transcript items untouched - when a batch carries the explicit empty state. Verified the new test bites by mutating the reducer's undefined check into a nullish coalesce (only the new test goes red), then reverting. Layered coverage is now: channel tests assert the host folds at-rest undefined into null on subscribe/history; the reducer test asserts the renderer actually clears on that null batch.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The fix itself is correct, but pnpm tc:node fails on the new test file (5 TS errors). The PR's verification table only cites tc:web, which does not include src/main, so this went unnoticed. pnpm tc typechecks the node project (tsconfig.node.json includes src/main/**/*), so this blocks CI. Details inline.
Reviewed changes
- Empty-state fold in the background-task channel —
historyandsubscribenow coerce anundefinedadapter answer tonull, so an at-rest conversation reports an explicit empty background-task state instead of omitting the field (structured-agent-session-background-task-channel.ts:45and:71).publishkeeps its edge-driven semantics. - Regression coverage — a new
structured-agent-session-background-task-channel.test.tspins resume-after-child-gone (null), live-state passthrough, explicit adapternullpassthrough, and the history-read empty state.
The implementation is sound: a conversation opened at rest has no live provider child, so null is the correct empty state, and a live owner returns a real state or null and passes through untouched. The tests pass at runtime (4/4) and the two red-before cases genuinely pin the defect — before the fold, the subscribe batch omitted the field entirely, so toBe(null) fails on undefined. No behavioral issue found in the implementation.
ℹ️ Nitpicks
- After
?? null,backgroundTaskscan no longer beundefined, so the conditional spread...(backgroundTasks !== undefined ? { backgroundTasks } : {})is always true atstructured-agent-session-background-task-channel.ts:58and:76. Leaving it signals an omit-path that no longer exists; the field can be spread directly.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Repository guideline files applied to this review (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe background-task channel now includes Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to A resumed session whose provider is no longer live now clears stale background-task state. Live state and explicit null pass through unchanged, and the change is covered by channel and reducer tests. No merge-blocking risk remains. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is narrowly scoped and preserves reported live tasks. No new security exposure was demonstrated, but the distinction between absent tasks and unavailable task information is not fully established across failure and recovery paths. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…, cover null-batch clear Review follow-ups on stablyai#24239: The new channel test did not compile under tsconfig.node.json (vitest does not type-check, so the previous verification pass missed it): the adapter stub's `() => unknown` and the store stub's `undefined` return did not match the deps' signatures, and the deps literal lacked the required `journalDatabase` and `claimKeyId`. The fixture now fills the deps' shape the way the host-runtime-state test does - real per-directory journal database, typed background-task read, record store stub cast. `AgentSessionSubscribeEvent` is a discriminated union that includes the terminal `{ type: 'end' }` frame, so `.backgroundTasks` is not a shared property. The three assertions now narrow with an `Extract<..., { type: 'batch' }>` predicate instead of reading the last event blind. The resume contract's renderer half was untested: the reducer must apply a batch that carries `backgroundTasks: null` over a state that already holds monitoring tasks, clearing the stale roster, while an omitted field keeps meaning "no change". A reducer test locks both directions, so the channel's null fold cannot silently regress on either side. Generated-by: GLM-5.3-Flash (ZCode)
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Test fixture now typechecks — the channel fixture types
backgroundTaskStateagainst the adapter's real read, returnsnullfromgetRecord, and fills the requiredjournalDatabase/claimKeyId;pnpm tc:nodeexits clean. - Subscribe-event narrowing — all three assertion sites use a typed
batchpredicate rather than indexing the last event of the union. - Reducer lock-in case — a new case pins that an omitted batch field reads as no change (same object identity) while an explicit
nullreplaces the stale roster without disturbing transcript identity.
Both prior review threads (fixture typecheck, union narrowing) are addressed and retired. Runtime tests verified green (channel 4/4, reducer 14/14).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Superseded by upstream Closing in favor of the upstream fix. Thanks to greptile, pullfrog and coderabbit for the three review rounds that shaped the test coverage here; happy to revisit if the history read path turns out to still need the fold. |
Summary
Fixes #24227. A finished background task stayed on the native-chat strip as "running" forever (timer still counting, Stop failing) when its chat pane was hidden while the task ended.
Root cause (all five steps verified against
main):use-structured-agent-session-read.tsgatesowner.activate()onisVisible, so the task-end publish (backgroundTasks: null) reached zero subscribers.STRUCTURED_AGENT_SESSION_IDLE_MS), after whichadapter.backgroundTaskStateanswersundefinedrather thannull(claude-structured-session-adapter.ts:session ? backgroundTaskState(session) : undefined).undefined.Fix:
StructuredAgentSessionBackgroundTaskChannel.subscribeand.historyfoldundefinedintonullbefore opening the subscriber / answering a history read. A conversation opened at rest has no live provider child, so it has no live background tasks - a resuming renderer receives the empty state and clears its copy. A live child still passes its real state through untouched (AgentSessionSubscribersalready forwards explicitnull).publishkeeps its edge-driven semantics. The renderer backstop and Stop-settling variants from the issue are intentionally left alone: those only mask what the channel now reports correctly.Verification
expected undefined to be null- subscribe resume + history read), 2 lock-in cases greentsc --noEmit -p config/tsconfig.tc.web.jsonThe two red-before cases pin the exact defect: a resume after the provider child is gone must deliver
backgroundTasks: null, not omit the field.AI use
Implemented by GLM-5.3-Flash (ZCode) from the issue's root-cause analysis; root-cause chain re-verified line-by-line in this codebase before writing the fix, and the fix direction (main-side empty-state reporting) follows the issue's option 1.
Checklist