feat(core,cli): surface and cancel auto-memory dream tasks#3836
Merged
Conversation
Adds `dream` as a fourth kind in the Background tasks pill + dialog, alongside agent / shell / monitor. Subscribes to MemoryManager via its existing subscribe() / listTasksByType() API and adapts MemoryTaskRecord into a DreamDialogEntry view-model. Zero changes to the core package. Filters out `pending` (sub-second transition) and `skipped` (every UserQuery that misses the gate creates one) records. Caps retained terminal entries at 3 since `MemoryManager.tasks` has no eviction path; without the cap, completed dreams would accumulate over the project's lifetime (mirrors MonitorRegistry's terminal cap pattern). Extract tasks are intentionally NOT surfaced — they fire on every UserQuery, would flood the pill, and the `memory_saved` toast in useGeminiStream already covers their completion signal. Read-only for now: cancellation requires MemoryManager.cancelTask + task_stop integration which lands in a follow-up PR. The dialog suppresses the "x stop" hint for dream entries until then to avoid silent no-op keystrokes. Refs #3634
The earlier comment compared the design to a non-qwen-code product; restate the rationale in terms of the local extract / dream split (extract fires every UserQuery and surfaces via memory_saved toast, dream fires after gates and warrants pill / dialog visibility).
…stop Wires cancellation for the auto-memory dream task kind: - `MemoryManager.cancelTask(taskId)` — aborts the dream's fork-agent via a new per-task AbortController, marks the record `cancelled` before aborting so the runDream catch path can detect user-intent and avoid overwriting with a generic `failed`. The existing finally block releases the consolidation lock as the agent unwinds. - `MemoryManager.getTask(id)` — point lookup helper so cross-cutting consumers like `task_stop` can route by id without a project root. - AbortSignal threaded through `scheduleDream` → `runDream` → `runManagedAutoMemoryDream` → `planManagedAutoMemoryDreamByAgent` → `runForkedAgent.abortSignal` (already supported). - `task_stop` tool gets a 4th dispatch branch: dream task ids look up via MemoryManager and route through `cancelTask`. Extract is intentionally NOT cancellable — it runs synchronously on the request loop, cancelling would interfere with the user's own turn. - `BackgroundTasksDialog` x-stop hint suppression for dream is removed (was a PR-1 placeholder); `cancelSelected` dream branch now calls `memoryManager.cancelTask`. - `MemoryTaskStatus` gains `'cancelled'`. Dream view-model widens status union and filter to surface cancelled entries (terminal cap continues to apply). Refs #3634
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the CLI Background tasks UX to include managed auto-memory dream (consolidation) tasks alongside existing agent/shell/monitor entries, wiring MemoryManager tasks into the pill + dialog list/detail views and adding cancellation plumbing.
Changes:
- Added a new background-task kind (
dream) to the footer pill and Background tasks dialog (list rows + detailed rendering). - Adapted
MemoryManagerdream task records into the unified dialog view-model, including filtering/capping terminal entries and subscribing for updates. - Introduced dream cancellation support across the stack (MemoryManager abort controllers, UI cancel routing, and
task_stoptool dispatch).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/tools/task-stop.ts | Adds task_stop support for cancelling MemoryManager dream tasks and extends not-running handling to dream. |
| packages/core/src/tools/task-stop.test.ts | Adds unit tests for dream cancellation routing and terminal/not-found behavior. |
| packages/core/src/memory/manager.ts | Adds cancelled status, per-dream AbortController tracking, plus getTask()/cancelTask() APIs and abort propagation into dream execution. |
| packages/core/src/memory/manager.test.ts | Adds tests validating cancelTask() behavior and that cancellation doesn’t degrade to failed. |
| packages/core/src/memory/dreamAgentPlanner.ts | Threads abortSignal into runForkedAgent for dream runs. |
| packages/core/src/memory/dream.ts | Threads abortSignal through dream execution helpers into the planner. |
| packages/cli/src/ui/hooks/useBackgroundTaskView.ts | Subscribes to MemoryManager and merges dream tasks into the unified dialog/pill entry list, including terminal-entry retention cap. |
| packages/cli/src/ui/hooks/useBackgroundTaskView.test.ts | Adds coverage for dream filtering/capping and subscription-driven refresh. |
| packages/cli/src/ui/contexts/BackgroundTaskViewContext.tsx | Routes cancel action for dream entries to MemoryManager.cancelTask(). |
| packages/cli/src/ui/components/background-view/BackgroundTasksPill.tsx | Adds dream counting/labeling in the pill (shell → agent → monitor → dream ordering). |
| packages/cli/src/ui/components/background-view/BackgroundTasksPill.test.tsx | Adds pill label tests for dream singular/plural, ordering, and terminal-mix behavior. |
| packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx | Adds [dream] row labeling and DreamDetailBody rendering; hint/footer and key handling will now show x stop for running dreams. |
| packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx | Adds dialog rendering tests for dream list/detail states and cancellation routing. |
Comments suppressed due to low confidence (1)
packages/core/src/memory/dreamAgentPlanner.ts:258
planManagedAutoMemoryDreamByAgentonly throws whenresult.status === 'failed'. When a dream is aborted via the newabortSignal,runForkedAgentcan return{ status: 'cancelled' }without throwing; this path will currently be treated as success byrunManagedAutoMemoryDream/MemoryManager.runDreamand can overwrite a user-cancelled dream ascompleted(and even bump metadata/rebuild index based on partial writes). Handle cancellation explicitly (e.g., throw unlessresult.status === 'completed', mirroringextractionAgentPlanner, or propagate a cancelled result upward so callers can preservecancelled).
if (result.status === 'failed') {
throw new Error(result.terminateReason || 'Dream agent failed');
}
return result;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- DreamDetailBody: comment said cancellation "lands in PR-2" but the same PR wires `cancelSelected` for dream entries. Reword to describe what's actually shipped + flag in-flight progress as the real follow-up. - task_stop: drop the unreachable `!aborted` error branch. The status guard above already confirms `running`, and `cancelTask` is synchronous; in this branch it cannot return false.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tasks
runForkedAgent maps AgentTerminateMode.CANCELLED to a resolved
{status: 'cancelled'} result rather than rejecting. The cancel-via-
task_stop path landed in the previous commit assumed the call would
throw — when it didn't, the runDream success path overwrote the
user-cancelled record with 'completed' AND bumped lastDreamAt
metadata, suppressing the next legitimate dream cycle.
Two-layer defense:
- dreamAgentPlanner now rethrows when the fork agent reports
cancelled status (mirrors the existing failed-status throw).
This is the source-of-truth fix.
- runDream now checks abortSignal.aborted after the await as
defense in depth. If anything in the call chain ever forgets to
propagate, this guard short-circuits the success path before
metadata write.
Updates the existing dreamAgentPlanner test that previously pinned
the buggy "returns cancelled without throwing" behavior. Adds a
manager test that simulates the resolves-on-abort scenario directly
to verify the consumer-side guard catches anything the planner
might miss.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… clarity
Three fixes from the post-cancellation-PR review:
- scheduleDream now sets an initial `progressText` ("Scheduled
managed auto-memory dream.") on the in-flight record. Without
this, the dialog Detail's Progress section stayed empty until
completion — the PR description's mid-flight screenshot showed
text that production never actually rendered.
- useBackgroundTaskView gates the MemoryManager.subscribe listener
on a dream-content signature. The manager fires for every task
transition (extract included, ~2x per UserQuery), but the dialog
has no extract surface; without this dedup each extract notify
forced a full 4-source re-merge + a fresh setEntries reference,
re-rendering the dialog and pill on entries that hadn't changed.
Added a test that pins the reference-stability invariant.
- task-stop comment was misleading — said "the status guard above
already confirmed running", but for the dream branch the running
check happens IN this branch (not earlier). Reworded.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…s, sync test helper
Four fixes from the latest review pass:
- cancelTask() now enforces the AbortController invariant. The old
`ac?.abort()` returned `true` even when no controller was found,
meaning callers could see a successful return while the dream was
not actually aborted (and would leak the consolidation lock until
the agent finished naturally). The controller is registered
synchronously alongside `status='running'`, so a missing controller
for a running record is a contract violation — return false without
flipping status so the caller knows the abort didn't take.
- useBackgroundTaskView's `refresh()` now reuses the dream snapshot
the memory listener fetched for its dedup gate. The previous version
re-read `listTasksByType('dream')` inside `computeDreamSig()` and
again inside `refresh()` — extra work AND a race window where the
gate signature could come from a different snapshot than the one
used to build dreamEntries. Single read, single source of truth.
- The `dream()` test helper widened to include `'cancelled'` so it
matches the production `MemoryTaskStatus` union. Added a small test
asserting cancelled dreams flow through the kind discriminator (the
dialog's terminal-cap window depends on showing the user the
outcome of the abort they just triggered).
- Dropped the stale "PR-1 read-only / PR-2 cancellation" block
comment above the dream-entries describe block — both are in this
PR now.
Collaborator
Author
Self-review notesWhat's good
Issues to address1. Redundant dream signal in Footer (UX) — blocker for clarity
2.
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Combined fixes for the latest review pass — self-review notes (U1, U2,
U5, U6) plus Copilot's three new comments (C1, C2, C3):
- **Footer dream-indicator dedupe (U1)**: removed `useDreamRunning` +
`✦ dreaming` right-column text. The new Background tasks pill
already counts dream tasks alongside agent / shell / monitor;
showing both produced two simultaneous signals for the same state.
- **task_stop dispatch surfaces extract distinctly (U2 + C1)**: a
new `TASK_STOP_NOT_CANCELLABLE` error type fires when the task id
resolves to a known-but-not-cancellable record (extract). Previously
extract ids fell through to `NOT_FOUND`, misleading the model into
thinking the id was never valid. Also surfaces the missing-controller
case from `cancelTask` as an explicit error rather than reporting
phantom success.
- **MemoryManager.cancelTask logs missing-controller violation (C2)**:
the silent `return false` for the missing-AbortController case now
emits a `debugLogger.warn` so the inconsistency is observable in
debug bundles. Without the log a runaway dream burning tokens would
leave no trail.
- **MemoryManager.subscribe taskType filter (C3)**: subscribers can
now opt into per-type notify routing via `subscribe(fn, { taskType })`.
Internal `notify()` calls pass the changed task's type so filtered
consumers wake only on relevant transitions. The bg-tasks UI hook
uses this to skip the per-UserQuery extract notify entirely — drops
the per-extract O(n) signature work to zero.
- **runDream guards against late-cancel overwrite (U5)**: the
success path now re-checks `abortSignal.aborted` between metadata
read/write and before the final `update({status: 'completed'})`.
Closes the ~tens-of-ms race window where `cancelTask` flipping
status to `'cancelled'` would silently lose to the success
continuation overwriting with `'completed'` + bumping
`lastDreamAt`.
- **DreamDialogEntry.endTime semantic comment (U6)**: documents that
`endTime` for cancelled records is the cancel-call moment (not the
fork unwind), so a future maintainer doesn't treat it as a real
fork-finish timestamp.
Tests: new `subscribe() taskType filter` describe block in manager,
new task_stop tests for `NOT_CANCELLABLE` (extract) and missing-
AbortController (dream); existing test renamed/widened.
Collaborator
Author
|
Self-review items addressed:
Plus the same commit picks up Copilot's three new comments — |
The pre-call abort checks in `runManagedAutoMemoryDream` close most of the late-cancel race window, but each metadata helper itself does read → mutate → write across two awaits. If the user cancels between those two awaits the write still happens, persisting `lastDreamAt` for an aborted run and suppressing the next legitimate dream cycle. Thread `abortSignal` into `bumpMetadata` and `updateDreamMetadataResult`; both now re-check between the read and the write, returning early without persisting when the signal has already fired. The pre-call checks remain as the first line of defense; this guards the race that opens after the call enters.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wenshao
commented
May 5, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Review findings
Suggestion: Telemetry dropped for cancelled-but-completed dreams
File: packages/core/src/memory/dream.ts:124 (logMemoryDream)
When a dream is cancelled after the fork agent completes but before logMemoryDream is reached, the abort guard (if (abortSignal?.aborted) return agentResult;) at line 100 returns early, skipping the telemetry call entirely. No MemoryDreamEvent is emitted, so cancelled dreams are undercounted in aggregate metrics and their duration/touched-topics are lost from observability.
Suggested fix: Emit telemetry before the abort checks, or add a separate logging call for the cancelled-but-completed path with status: 'cancelled'.
Suggestion: getTask() returns live internal MemoryTaskRecord reference
File: packages/core/src/memory/manager.ts (getTask method)
getTask(taskId) returns this.tasks.get(taskId) — a direct reference to the internal Map value. The current caller (task-stop.ts) only reads from it, but any future consumer that mutates the record would bypass update() and skip subscriber notifications, causing the dialog to desync.
Suggested fix: Return a shallow copy: return this.tasks.has(taskId) ? { ...this.tasks.get(taskId)! } : undefined;
wenshao
commented
May 5, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Two related fixes from the latest review pass:
- Move scheduler-gating metadata writes out of `runManagedAutoMemoryDream`
and into `MemoryManager.runDream`, sequenced AFTER the
status='completed' flip. The previous shape left a race window
where cancellation arriving during/after `fs.writeFile` could
persist `lastDreamAt` while the manager flipped status to
'cancelled' — visible UI ('Stopped') would disagree with the
scheduler gate (sees a recent successful dream), suppressing the
next legitimate dream cycle. The new order makes the gating
metadata write race-free: once status !== 'running', cancelTask
refuses, so any cancel arriving during the metadata write is
ignored. The remaining "cancel raced the synchronous status
update" window is handled by a post-update abort recheck that
restores 'cancelled' and skips the metadata write.
Drops the now-dead `bumpMetadata` helper from dream.ts; index
rebuild stays there since it's informational, not gating.
- Surface `releaseDreamLock` failures on the task record's metadata
(`lockReleaseError`). The previous fix logged-and-swallowed only,
so a Windows EPERM or ENOENT race would silently block subsequent
dreams as 'locked' with no UI signal explaining why dreaming had
stopped. Logger keeps emitting the warn for debug bundles.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wenshao
commented
May 5, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed end-to-end. Cancel-flow logic is solid and well-tested — the race-window analysis around the synchronous status flip + dual abort checks is precise, and the test pinning the "resolved-after-cancel" case is exactly the right defense. Approving in spirit; flagging one functional concern (metadata-write failure overwriting a completed dream) plus a few smaller polish items inline. Other risks called out in the PR description (MemoryTaskStatus widening, runForkedAgent API stability, params.config behavior change) all check out.
wenshao
commented
May 5, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
[Critical] packages/cli/src/ui/utils/backgroundWorkUtils.ts:9 — hasBlockingBackgroundWork() and resetBackgroundStateForSessionSwitch() still only account for background agents, monitors, and shells. This PR makes dream tasks visible/cancellable as first-class background work, but /clear and resume can still proceed while a dream is running because the memory manager is not checked or reset/cancelled. A dream fork-agent can continue after a session switch/reset, using the old session/project context while UI/core state has moved on. Include running dream records in hasBlockingBackgroundWork(config), and either cancel/drain running dreams or add a memory-manager reset/cancel path in resetBackgroundStateForSessionSwitch(). Add tests for /clear/resume with a running dream.
[Suggestion] packages/core/src/tools/task-stop.ts:236 — The task_stop tool description still says only agents and shells are cancelled, but the tool now supports monitors and cancellable dream tasks. This is model-facing metadata, so the model may not discover that it can stop dream consolidation tasks. Update the description to mention agents, shells, monitors, and cancellable dream tasks.
— gpt-5.5 via Qwen Code /review
wenshao
commented
May 5, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Posted Codex qreview finding from the latest review pass. Skipped the lock-release finding because an equivalent inline comment already exists on packages/core/src/memory/manager.ts:962.
wenshao
commented
May 5, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— deepseek-v4-pro via Qwen Code /review
Reverts the metadata-write behavior regression, plugs the
storeWith reentrancy hole, surfaces lock/metadata warnings in the
UI, and a handful of cleanups:
- Wrap gating-metadata read+write in try/catch (manager.ts). The
PR moved metadata writes from dream.ts (best-effort, swallowed)
to manager.ts (unguarded). A throw from readDreamMetadata /
writeDreamMetadata now propagates to the outer catch and
overwrites a successfully-completed dream with 'failed' — the
dream actually did its work and touched files are visible. New
catch logs + writes `metadataWriteError` on the record so the UI
can explain why the next dream may re-fire sooner than expected.
- Register the AbortController BEFORE storeWith in scheduleDream.
storeWith fires a notify; a subscriber synchronously calling
cancelTask(record.id) would otherwise see status='running' but
no controller, hitting the missing-controller defensive warn
path and reporting a phantom failure on a brand-new dream.
- Surface `lockReleaseError` and `metadataWriteError` in the dream
view-model and DreamDetailBody (rendered as warnings, not
errors, so the terminal status stays Completed). Previous fix
wrote them to record.metadata only — nothing in the cli read or
rendered them, so users still had no UI signal.
- Preserve `result.touchedTopics` on the unreachable cancel-raced-
status-update branch. If a future refactor introduces an await
there, the restored cancelled record would otherwise drop the
already-produced result; the UI would report a clean cancellation
even though memory files were already modified.
- Add `'cancelled'` to MemoryDreamEvent status union and emit a
cancelled telemetry event from the runDream catch path. Without
this a cancelled dream is indistinguishable from one that never
scheduled in the first place.
- Drop the dead abortSignal param from updateDreamMetadataResult
in dream.ts (no caller passes it after the manager.ts move).
- Swap declaration order of `computeDreamSig` and `refresh` in
useBackgroundTaskView.ts (TDZ-fragile against a future refactor
that adds a synchronous refresh call between them).
- Update the unreachable cancel-raced-update branch comment to
describe what's actually true ("defense-in-depth, unreachable
today") instead of the confusing "cancelTask flipped" path.
- cancelSelected now checks the cancelTask return value and logs
via debugLogger when false. Today this branch is unreachable
thanks to the controller-register-before-storeWith fix above,
but if a future refactor breaks the invariant the silent ignore
would let the user think the cancel took effect.
- Mirror the manual /dream metadata path's `recentSessionIdsSinceDream = []`
reset in the auto path — field is dead code today but keeping
the two write sites in sync avoids surprises.
Telemetry metric `recordMemoryDreamMetrics` widened to accept the
new 'cancelled' status (downstream consumer in loggers.ts).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The prior fix surfaced lockReleaseError on the dialog so the user knows
the lock release failed (Windows EPERM, ENOENT race, disk full, etc.) —
but until next process start, dreamLockExists() still sees a fresh-mtime
lock owned by an alive PID (us!) and silently suppresses every
subsequent scheduleDream() call as `{status: 'skipped',
skippedReason: 'locked'}`. The user sees the warning AND zero further
dream activity, and the staleness sweep that would clean the leaked
lock only runs at session start.
Adds a `dreamLockReleaseFailed` flag set in the catch. The next
scheduleDream() force-cleans the leaked lock file via fs.rm({force: true})
before the existence check, so dream scheduling resumes within the same
session. Best-effort: if even the forced rm fails (truly unrecoverable
filesystem state), falls through to the existing 'locked' skip path.
This is an incremental improvement on top of b00ecde's UI-surface fix.
The two together give the full story: warning visible → automatic
recovery on next attempt.
…dering Two follow-up suggestions from review: - **`duration_ms: 0` in cancelled-dream telemetry** (manager.ts): the user-cancel path emitted `MemoryDreamEvent` with `duration_ms: 0`, which would silently skew latency histograms / p95 metrics by treating cancelled dreams as instant. Capture `dreamStartMs = Date.now()` at the top of `runDream` and emit the real elapsed time in the cancel branch. - **Misleading index-rebuild comment** (dream.ts:75–84): the comment claimed the index rebuild "is still done before returning when topics were touched", but the code returns early on `abortSignal?.aborted` BEFORE the rebuild. Rewrote the comment to describe the actual cancel-aware ordering — abort returns partial result without rebuilding (rebuild is expensive; next dream cycle will rebuild against the latest files anyway), live path rebuilds only when topics changed. 22 / 22 manager tests pass; tsc clean.
wenshao
commented
May 5, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR.
— gpt-5.5 via Qwen Code /review
Collaborator
Author
Code reviewOverviewBrings managed auto-memory dream consolidation tasks into the unified Background tasks UI and makes them user-cancellable. 18 files, +1676/-100, scope-limited to dream visibility + cancellation. Extract intentionally excluded. StrengthsRace handling is the strongest part of this PR:
Test coverage: 44 new core + 52 new CLI tests covering cancel mid-run, cancel on completed/unknown, fork resolves despite cancel, subscribe filter routing, unsubscribe severance, terminal-cap eviction, pending/skipped filter, and dream-content signature dedup. Inverse paths (e.g. dedup not firing when content actually changed) are also pinned. Backward compatibility: Performance: Observations / minor
Verification
Risks
VerdictLGTM. Design anticipates multi-layer race scenarios, test coverage is comprehensive. The |
wenshao
commented
May 5, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No blocking issues found. CI all green (12/12 checks passed), build and 93 tests pass locally.
One minor note (not posted inline — low confidence): packages/core/src/memory/dream.ts\:86-97 — runManagedAutoMemoryDream calls logMemoryDream after await rebuildManagedAutoMemoryIndex without rechecking abortSignal. If cancellation fires during that I/O gap, telemetry emits 'updated' for a cancelled dream. Record state is correctly preserved by manager.ts defense-in-depth guards.
Code quality is high — precise cancellation flow, robust lock release, exhaustive discriminated unions in UI, comprehensive tests. LGTM! ✅
— deepseek-v4-pro via Qwen Code /review
tanzhenxin
approved these changes
May 6, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
This PR cleanly extends the unified Background tasks UI to a fourth kind (dream) and adds end-to-end cancellation; the cancellation race analysis is sound, test coverage is dense at the right places, and the design trade-offs are well-documented in the description.
Verdict
APPROVE — Worth landing as-is.
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 6, 2026
…nLM#3836) - fix(core): prevent auto-memory recall from blocking main request (QwenLM#3814) - feat(core,cli): surface and cancel auto-memory dream tasks (QwenLM#3836) - fix(sdk-python): standardize TAG_PREFIX to include v suffix (QwenLM#3832) Conflict resolutions: - Footer.tsx: removed stale dreamRunning/configInitMessage (not used in render) - BackgroundTaskViewContext.tsx: kept @hoptrendy/hopcode-core, added createDebugLogger - manager.test.ts: included upstream cancelTask + subscribe filter tests, replaced QWEN_CODE_MEMORY_LOCAL with HOPCODE_MEMORY_LOCAL - get-release-version.js: kept hopcode-sdk brand, adopted upstream v prefix fix Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
46 tasks
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
* feat(cli): surface auto-memory dream tasks in Background tasks dialog Adds `dream` as a fourth kind in the Background tasks pill + dialog, alongside agent / shell / monitor. Subscribes to MemoryManager via its existing subscribe() / listTasksByType() API and adapts MemoryTaskRecord into a DreamDialogEntry view-model. Zero changes to the core package. Filters out `pending` (sub-second transition) and `skipped` (every UserQuery that misses the gate creates one) records. Caps retained terminal entries at 3 since `MemoryManager.tasks` has no eviction path; without the cap, completed dreams would accumulate over the project's lifetime (mirrors MonitorRegistry's terminal cap pattern). Extract tasks are intentionally NOT surfaced — they fire on every UserQuery, would flood the pill, and the `memory_saved` toast in useGeminiStream already covers their completion signal. Read-only for now: cancellation requires MemoryManager.cancelTask + task_stop integration which lands in a follow-up PR. The dialog suppresses the "x stop" hint for dream entries until then to avoid silent no-op keystrokes. Refs #3634 * docs(cli): rephrase dream filter comment to focus on extract vs dream The earlier comment compared the design to a non-qwen-code product; restate the rationale in terms of the local extract / dream split (extract fires every UserQuery and surfaces via memory_saved toast, dream fires after gates and warrants pill / dialog visibility). * feat(core,cli): cancel dream consolidation tasks via dialog and task_stop Wires cancellation for the auto-memory dream task kind: - `MemoryManager.cancelTask(taskId)` — aborts the dream's fork-agent via a new per-task AbortController, marks the record `cancelled` before aborting so the runDream catch path can detect user-intent and avoid overwriting with a generic `failed`. The existing finally block releases the consolidation lock as the agent unwinds. - `MemoryManager.getTask(id)` — point lookup helper so cross-cutting consumers like `task_stop` can route by id without a project root. - AbortSignal threaded through `scheduleDream` → `runDream` → `runManagedAutoMemoryDream` → `planManagedAutoMemoryDreamByAgent` → `runForkedAgent.abortSignal` (already supported). - `task_stop` tool gets a 4th dispatch branch: dream task ids look up via MemoryManager and route through `cancelTask`. Extract is intentionally NOT cancellable — it runs synchronously on the request loop, cancelling would interfere with the user's own turn. - `BackgroundTasksDialog` x-stop hint suppression for dream is removed (was a PR-1 placeholder); `cancelSelected` dream branch now calls `memoryManager.cancelTask`. - `MemoryTaskStatus` gains `'cancelled'`. Dream view-model widens status union and filter to surface cancelled entries (terminal cap continues to apply). Refs #3634 * fix(core,cli): address review feedback on dream cancellation surface - DreamDetailBody: comment said cancellation "lands in PR-2" but the same PR wires `cancelSelected` for dream entries. Reword to describe what's actually shipped + flag in-flight progress as the real follow-up. - task_stop: drop the unreachable `!aborted` error branch. The status guard above already confirms `running`, and `cancelTask` is synchronous; in this branch it cannot return false. * fix(core): handle resolved-cancel path from runForkedAgent for dream tasks runForkedAgent maps AgentTerminateMode.CANCELLED to a resolved {status: 'cancelled'} result rather than rejecting. The cancel-via- task_stop path landed in the previous commit assumed the call would throw — when it didn't, the runDream success path overwrote the user-cancelled record with 'completed' AND bumped lastDreamAt metadata, suppressing the next legitimate dream cycle. Two-layer defense: - dreamAgentPlanner now rethrows when the fork agent reports cancelled status (mirrors the existing failed-status throw). This is the source-of-truth fix. - runDream now checks abortSignal.aborted after the await as defense in depth. If anything in the call chain ever forgets to propagate, this guard short-circuits the success path before metadata write. Updates the existing dreamAgentPlanner test that previously pinned the buggy "returns cancelled without throwing" behavior. Adds a manager test that simulates the resolves-on-abort scenario directly to verify the consumer-side guard catches anything the planner might miss. * fix(core,cli): address review feedback on dream UX, perf, and comment clarity Three fixes from the post-cancellation-PR review: - scheduleDream now sets an initial `progressText` ("Scheduled managed auto-memory dream.") on the in-flight record. Without this, the dialog Detail's Progress section stayed empty until completion — the PR description's mid-flight screenshot showed text that production never actually rendered. - useBackgroundTaskView gates the MemoryManager.subscribe listener on a dream-content signature. The manager fires for every task transition (extract included, ~2x per UserQuery), but the dialog has no extract surface; without this dedup each extract notify forced a full 4-source re-merge + a fresh setEntries reference, re-rendering the dialog and pill on entries that hadn't changed. Added a test that pins the reference-stability invariant. - task-stop comment was misleading — said "the status guard above already confirmed running", but for the dream branch the running check happens IN this branch (not earlier). Reworded. * fix(core,cli): tighten cancelTask contract, dedup dream snapshot reads, sync test helper Four fixes from the latest review pass: - cancelTask() now enforces the AbortController invariant. The old `ac?.abort()` returned `true` even when no controller was found, meaning callers could see a successful return while the dream was not actually aborted (and would leak the consolidation lock until the agent finished naturally). The controller is registered synchronously alongside `status='running'`, so a missing controller for a running record is a contract violation — return false without flipping status so the caller knows the abort didn't take. - useBackgroundTaskView's `refresh()` now reuses the dream snapshot the memory listener fetched for its dedup gate. The previous version re-read `listTasksByType('dream')` inside `computeDreamSig()` and again inside `refresh()` — extra work AND a race window where the gate signature could come from a different snapshot than the one used to build dreamEntries. Single read, single source of truth. - The `dream()` test helper widened to include `'cancelled'` so it matches the production `MemoryTaskStatus` union. Added a small test asserting cancelled dreams flow through the kind discriminator (the dialog's terminal-cap window depends on showing the user the outcome of the abort they just triggered). - Dropped the stale "PR-1 read-only / PR-2 cancellation" block comment above the dream-entries describe block — both are in this PR now. * fix(core,cli): address self-review + Copilot feedback on dream surface Combined fixes for the latest review pass — self-review notes (U1, U2, U5, U6) plus Copilot's three new comments (C1, C2, C3): - **Footer dream-indicator dedupe (U1)**: removed `useDreamRunning` + `✦ dreaming` right-column text. The new Background tasks pill already counts dream tasks alongside agent / shell / monitor; showing both produced two simultaneous signals for the same state. - **task_stop dispatch surfaces extract distinctly (U2 + C1)**: a new `TASK_STOP_NOT_CANCELLABLE` error type fires when the task id resolves to a known-but-not-cancellable record (extract). Previously extract ids fell through to `NOT_FOUND`, misleading the model into thinking the id was never valid. Also surfaces the missing-controller case from `cancelTask` as an explicit error rather than reporting phantom success. - **MemoryManager.cancelTask logs missing-controller violation (C2)**: the silent `return false` for the missing-AbortController case now emits a `debugLogger.warn` so the inconsistency is observable in debug bundles. Without the log a runaway dream burning tokens would leave no trail. - **MemoryManager.subscribe taskType filter (C3)**: subscribers can now opt into per-type notify routing via `subscribe(fn, { taskType })`. Internal `notify()` calls pass the changed task's type so filtered consumers wake only on relevant transitions. The bg-tasks UI hook uses this to skip the per-UserQuery extract notify entirely — drops the per-extract O(n) signature work to zero. - **runDream guards against late-cancel overwrite (U5)**: the success path now re-checks `abortSignal.aborted` between metadata read/write and before the final `update({status: 'completed'})`. Closes the ~tens-of-ms race window where `cancelTask` flipping status to `'cancelled'` would silently lose to the success continuation overwriting with `'completed'` + bumping `lastDreamAt`. - **DreamDialogEntry.endTime semantic comment (U6)**: documents that `endTime` for cancelled records is the cancel-call moment (not the fork unwind), so a future maintainer doesn't treat it as a real fork-finish timestamp. Tests: new `subscribe() taskType filter` describe block in manager, new task_stop tests for `NOT_CANCELLABLE` (extract) and missing- AbortController (dream); existing test renamed/widened. * fix(core,cli): tighten error semantics + comments on dream surface Four fixes from the latest review pass: - New `TASK_STOP_INTERNAL_ERROR` error type for the missing- AbortController contract violation. Previously the dispatcher reused `TASK_STOP_NOT_RUNNING`, which is misleading — the task IS running, cancellation just couldn't be delivered. Distinct type signals "this is unexpected, file a bug" vs `NOT_CANCELLABLE` which signals "expected behavior, use a different approach". - Reworded the `useBackgroundTaskView` filter comment. Said "every UserQuery that misses the gate creates one [skipped record]" but `scheduleDream` returns `{status: 'skipped'}` early without creating a record for most gate misses; only the acquireDreamLock/EEXIST race actually stores a `'skipped'` record. - Strengthened the `subscribe() unsubscribe` test. The previous version asserted "not called yet" without firing any notify after unsubscribe, so a regression that left the listener attached would still pass. Now schedules an extract before AND after the unsubscribe, verifying the call count doesn't increment. - Moved `const debugLogger = createDebugLogger(...)` below the full import block in manager.ts. Previous version sat between imports, violating eslint-plugin-import's `import/first` rule (didn't trip lint locally, but worth fixing before it does). * fix(core): skip scheduleDream early when params.config is missing `ScheduleDreamParams.config` is optional in the type so test paths can omit it, but production callers always pass one. Without a config, `runManagedAutoMemoryDream` throws because the fork-agent execution requires it. With dream tasks now visible in the Background tasks dialog, that throw becomes a noisy `failed` entry the user sees but didn't trigger. Convert the omitted-config case to the same `disabled` skip path that an explicitly-disabled config takes, so a no-config call short-circuits before any record is stored. Existing tests that relied on the old "no config = proceed past the disabled gate" behavior now pass an explicit `makeMockConfig()` (matching what they would do in any realistic scenario). New test pins the no-config skip behavior + asserts no record was stored (so a regression that drops the early skip would produce a visible failed entry in the dialog and fail the test). * fix(core): plug subscribe Map leak + dream.ts late-cancel ordering bug Two fixes from the latest review pass: - `MemoryManager.subscribe`'s typed-branch unsubscribe deleted the listener from its per-type Set but left the empty Set sitting in `subscribersByType`. Over a long-running session with repeated React mount/unmount of the bg-tasks view, that accumulates dead Map entries forever. Drop the entry when the bucket goes empty. - `runManagedAutoMemoryDream` writes metadata after the fork agent returns (`bumpMetadata` → `rebuildManagedAutoMemoryIndex` → `updateDreamMetadataResult`). If the user presses 'x' between the fork's success return and these writes, the writes proceed and bump `lastDreamAt` — leaving the visible UI ('Stopped') disagreeing with the scheduler gate (sees a recent successful dream and suppresses the next cycle). manager.ts already short-circuits its own metadata write via the post-await abort check, but it can't block writes that already happened inside dream.ts. Adds the same abort-signal check between each write step here. * fix(core): swallow releaseDreamLock errors so they don't poison outcome If `releaseDreamLock` throws inside the inner finally (e.g. filesystem error on the lock file), the exception propagates to the outer catch and overwrites a successfully-completed dream record with 'failed'. The on-disk metadata is already up-to-date at that point, so the user sees a contradictory state — `lastDreamAt` was bumped but the UI shows a failure. Wrap the release in a try/catch with `debugLogger.warn`. The lock file is still cleaned up on the next session via the existing staleness sweep, so swallowing the release error doesn't risk a permanent stuck lock. * fix(core): thread abortSignal into dream metadata writes The pre-call abort checks in `runManagedAutoMemoryDream` close most of the late-cancel race window, but each metadata helper itself does read → mutate → write across two awaits. If the user cancels between those two awaits the write still happens, persisting `lastDreamAt` for an aborted run and suppressing the next legitimate dream cycle. Thread `abortSignal` into `bumpMetadata` and `updateDreamMetadataResult`; both now re-check between the read and the write, returning early without persisting when the signal has already fired. The pre-call checks remain as the first line of defense; this guards the race that opens after the call enters. * fix(core): close dream cancel race + surface lock-release failures Two related fixes from the latest review pass: - Move scheduler-gating metadata writes out of `runManagedAutoMemoryDream` and into `MemoryManager.runDream`, sequenced AFTER the status='completed' flip. The previous shape left a race window where cancellation arriving during/after `fs.writeFile` could persist `lastDreamAt` while the manager flipped status to 'cancelled' — visible UI ('Stopped') would disagree with the scheduler gate (sees a recent successful dream), suppressing the next legitimate dream cycle. The new order makes the gating metadata write race-free: once status !== 'running', cancelTask refuses, so any cancel arriving during the metadata write is ignored. The remaining "cancel raced the synchronous status update" window is handled by a post-update abort recheck that restores 'cancelled' and skips the metadata write. Drops the now-dead `bumpMetadata` helper from dream.ts; index rebuild stays there since it's informational, not gating. - Surface `releaseDreamLock` failures on the task record's metadata (`lockReleaseError`). The previous fix logged-and-swallowed only, so a Windows EPERM or ENOENT race would silently block subsequent dreams as 'locked' with no UI signal explaining why dreaming had stopped. Logger keeps emitting the warn for debug bundles. * fix(core,cli): address 8 review findings on dream surface Reverts the metadata-write behavior regression, plugs the storeWith reentrancy hole, surfaces lock/metadata warnings in the UI, and a handful of cleanups: - Wrap gating-metadata read+write in try/catch (manager.ts). The PR moved metadata writes from dream.ts (best-effort, swallowed) to manager.ts (unguarded). A throw from readDreamMetadata / writeDreamMetadata now propagates to the outer catch and overwrites a successfully-completed dream with 'failed' — the dream actually did its work and touched files are visible. New catch logs + writes `metadataWriteError` on the record so the UI can explain why the next dream may re-fire sooner than expected. - Register the AbortController BEFORE storeWith in scheduleDream. storeWith fires a notify; a subscriber synchronously calling cancelTask(record.id) would otherwise see status='running' but no controller, hitting the missing-controller defensive warn path and reporting a phantom failure on a brand-new dream. - Surface `lockReleaseError` and `metadataWriteError` in the dream view-model and DreamDetailBody (rendered as warnings, not errors, so the terminal status stays Completed). Previous fix wrote them to record.metadata only — nothing in the cli read or rendered them, so users still had no UI signal. - Preserve `result.touchedTopics` on the unreachable cancel-raced- status-update branch. If a future refactor introduces an await there, the restored cancelled record would otherwise drop the already-produced result; the UI would report a clean cancellation even though memory files were already modified. - Add `'cancelled'` to MemoryDreamEvent status union and emit a cancelled telemetry event from the runDream catch path. Without this a cancelled dream is indistinguishable from one that never scheduled in the first place. - Drop the dead abortSignal param from updateDreamMetadataResult in dream.ts (no caller passes it after the manager.ts move). - Swap declaration order of `computeDreamSig` and `refresh` in useBackgroundTaskView.ts (TDZ-fragile against a future refactor that adds a synchronous refresh call between them). - Update the unreachable cancel-raced-update branch comment to describe what's actually true ("defense-in-depth, unreachable today") instead of the confusing "cancelTask flipped" path. - cancelSelected now checks the cancelTask return value and logs via debugLogger when false. Today this branch is unreachable thanks to the controller-register-before-storeWith fix above, but if a future refactor breaks the invariant the silent ignore would let the user think the cancel took effect. - Mirror the manual /dream metadata path's `recentSessionIdsSinceDream = []` reset in the auto path — field is dead code today but keeping the two write sites in sync avoids surprises. Telemetry metric `recordMemoryDreamMetrics` widened to accept the new 'cancelled' status (downstream consumer in loggers.ts). * fix(memory): same-session recovery when releaseDreamLock throws The prior fix surfaced lockReleaseError on the dialog so the user knows the lock release failed (Windows EPERM, ENOENT race, disk full, etc.) — but until next process start, dreamLockExists() still sees a fresh-mtime lock owned by an alive PID (us!) and silently suppresses every subsequent scheduleDream() call as `{status: 'skipped', skippedReason: 'locked'}`. The user sees the warning AND zero further dream activity, and the staleness sweep that would clean the leaked lock only runs at session start. Adds a `dreamLockReleaseFailed` flag set in the catch. The next scheduleDream() force-cleans the leaked lock file via fs.rm({force: true}) before the existence check, so dream scheduling resumes within the same session. Best-effort: if even the forced rm fails (truly unrecoverable filesystem state), falls through to the existing 'locked' skip path. This is an incremental improvement on top of b00ecde's UI-surface fix. The two together give the full story: warning visible → automatic recovery on next attempt. * fix(memory): real cancelled-dream duration + clarify index-rebuild ordering Two follow-up suggestions from review: - **`duration_ms: 0` in cancelled-dream telemetry** (manager.ts): the user-cancel path emitted `MemoryDreamEvent` with `duration_ms: 0`, which would silently skew latency histograms / p95 metrics by treating cancelled dreams as instant. Capture `dreamStartMs = Date.now()` at the top of `runDream` and emit the real elapsed time in the cancel branch. - **Misleading index-rebuild comment** (dream.ts:75–84): the comment claimed the index rebuild "is still done before returning when topics were touched", but the code returns early on `abortSignal?.aborted` BEFORE the rebuild. Rewrote the comment to describe the actual cancel-aware ordering — abort returns partial result without rebuilding (rebuild is expensive; next dream cycle will rebuild against the latest files anyway), live path rebuilds only when topics changed. 22 / 22 manager tests pass; tsc clean.
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
* feat(cli): surface auto-memory dream tasks in Background tasks dialog Adds `dream` as a fourth kind in the Background tasks pill + dialog, alongside agent / shell / monitor. Subscribes to MemoryManager via its existing subscribe() / listTasksByType() API and adapts MemoryTaskRecord into a DreamDialogEntry view-model. Zero changes to the core package. Filters out `pending` (sub-second transition) and `skipped` (every UserQuery that misses the gate creates one) records. Caps retained terminal entries at 3 since `MemoryManager.tasks` has no eviction path; without the cap, completed dreams would accumulate over the project's lifetime (mirrors MonitorRegistry's terminal cap pattern). Extract tasks are intentionally NOT surfaced — they fire on every UserQuery, would flood the pill, and the `memory_saved` toast in useGeminiStream already covers their completion signal. Read-only for now: cancellation requires MemoryManager.cancelTask + task_stop integration which lands in a follow-up PR. The dialog suppresses the "x stop" hint for dream entries until then to avoid silent no-op keystrokes. Refs QwenLM#3634 * docs(cli): rephrase dream filter comment to focus on extract vs dream The earlier comment compared the design to a non-qwen-code product; restate the rationale in terms of the local extract / dream split (extract fires every UserQuery and surfaces via memory_saved toast, dream fires after gates and warrants pill / dialog visibility). * feat(core,cli): cancel dream consolidation tasks via dialog and task_stop Wires cancellation for the auto-memory dream task kind: - `MemoryManager.cancelTask(taskId)` — aborts the dream's fork-agent via a new per-task AbortController, marks the record `cancelled` before aborting so the runDream catch path can detect user-intent and avoid overwriting with a generic `failed`. The existing finally block releases the consolidation lock as the agent unwinds. - `MemoryManager.getTask(id)` — point lookup helper so cross-cutting consumers like `task_stop` can route by id without a project root. - AbortSignal threaded through `scheduleDream` → `runDream` → `runManagedAutoMemoryDream` → `planManagedAutoMemoryDreamByAgent` → `runForkedAgent.abortSignal` (already supported). - `task_stop` tool gets a 4th dispatch branch: dream task ids look up via MemoryManager and route through `cancelTask`. Extract is intentionally NOT cancellable — it runs synchronously on the request loop, cancelling would interfere with the user's own turn. - `BackgroundTasksDialog` x-stop hint suppression for dream is removed (was a PR-1 placeholder); `cancelSelected` dream branch now calls `memoryManager.cancelTask`. - `MemoryTaskStatus` gains `'cancelled'`. Dream view-model widens status union and filter to surface cancelled entries (terminal cap continues to apply). Refs QwenLM#3634 * fix(core,cli): address review feedback on dream cancellation surface - DreamDetailBody: comment said cancellation "lands in PR-2" but the same PR wires `cancelSelected` for dream entries. Reword to describe what's actually shipped + flag in-flight progress as the real follow-up. - task_stop: drop the unreachable `!aborted` error branch. The status guard above already confirms `running`, and `cancelTask` is synchronous; in this branch it cannot return false. * fix(core): handle resolved-cancel path from runForkedAgent for dream tasks runForkedAgent maps AgentTerminateMode.CANCELLED to a resolved {status: 'cancelled'} result rather than rejecting. The cancel-via- task_stop path landed in the previous commit assumed the call would throw — when it didn't, the runDream success path overwrote the user-cancelled record with 'completed' AND bumped lastDreamAt metadata, suppressing the next legitimate dream cycle. Two-layer defense: - dreamAgentPlanner now rethrows when the fork agent reports cancelled status (mirrors the existing failed-status throw). This is the source-of-truth fix. - runDream now checks abortSignal.aborted after the await as defense in depth. If anything in the call chain ever forgets to propagate, this guard short-circuits the success path before metadata write. Updates the existing dreamAgentPlanner test that previously pinned the buggy "returns cancelled without throwing" behavior. Adds a manager test that simulates the resolves-on-abort scenario directly to verify the consumer-side guard catches anything the planner might miss. * fix(core,cli): address review feedback on dream UX, perf, and comment clarity Three fixes from the post-cancellation-PR review: - scheduleDream now sets an initial `progressText` ("Scheduled managed auto-memory dream.") on the in-flight record. Without this, the dialog Detail's Progress section stayed empty until completion — the PR description's mid-flight screenshot showed text that production never actually rendered. - useBackgroundTaskView gates the MemoryManager.subscribe listener on a dream-content signature. The manager fires for every task transition (extract included, ~2x per UserQuery), but the dialog has no extract surface; without this dedup each extract notify forced a full 4-source re-merge + a fresh setEntries reference, re-rendering the dialog and pill on entries that hadn't changed. Added a test that pins the reference-stability invariant. - task-stop comment was misleading — said "the status guard above already confirmed running", but for the dream branch the running check happens IN this branch (not earlier). Reworded. * fix(core,cli): tighten cancelTask contract, dedup dream snapshot reads, sync test helper Four fixes from the latest review pass: - cancelTask() now enforces the AbortController invariant. The old `ac?.abort()` returned `true` even when no controller was found, meaning callers could see a successful return while the dream was not actually aborted (and would leak the consolidation lock until the agent finished naturally). The controller is registered synchronously alongside `status='running'`, so a missing controller for a running record is a contract violation — return false without flipping status so the caller knows the abort didn't take. - useBackgroundTaskView's `refresh()` now reuses the dream snapshot the memory listener fetched for its dedup gate. The previous version re-read `listTasksByType('dream')` inside `computeDreamSig()` and again inside `refresh()` — extra work AND a race window where the gate signature could come from a different snapshot than the one used to build dreamEntries. Single read, single source of truth. - The `dream()` test helper widened to include `'cancelled'` so it matches the production `MemoryTaskStatus` union. Added a small test asserting cancelled dreams flow through the kind discriminator (the dialog's terminal-cap window depends on showing the user the outcome of the abort they just triggered). - Dropped the stale "PR-1 read-only / PR-2 cancellation" block comment above the dream-entries describe block — both are in this PR now. * fix(core,cli): address self-review + Copilot feedback on dream surface Combined fixes for the latest review pass — self-review notes (U1, U2, U5, U6) plus Copilot's three new comments (C1, C2, C3): - **Footer dream-indicator dedupe (U1)**: removed `useDreamRunning` + `✦ dreaming` right-column text. The new Background tasks pill already counts dream tasks alongside agent / shell / monitor; showing both produced two simultaneous signals for the same state. - **task_stop dispatch surfaces extract distinctly (U2 + C1)**: a new `TASK_STOP_NOT_CANCELLABLE` error type fires when the task id resolves to a known-but-not-cancellable record (extract). Previously extract ids fell through to `NOT_FOUND`, misleading the model into thinking the id was never valid. Also surfaces the missing-controller case from `cancelTask` as an explicit error rather than reporting phantom success. - **MemoryManager.cancelTask logs missing-controller violation (C2)**: the silent `return false` for the missing-AbortController case now emits a `debugLogger.warn` so the inconsistency is observable in debug bundles. Without the log a runaway dream burning tokens would leave no trail. - **MemoryManager.subscribe taskType filter (C3)**: subscribers can now opt into per-type notify routing via `subscribe(fn, { taskType })`. Internal `notify()` calls pass the changed task's type so filtered consumers wake only on relevant transitions. The bg-tasks UI hook uses this to skip the per-UserQuery extract notify entirely — drops the per-extract O(n) signature work to zero. - **runDream guards against late-cancel overwrite (U5)**: the success path now re-checks `abortSignal.aborted` between metadata read/write and before the final `update({status: 'completed'})`. Closes the ~tens-of-ms race window where `cancelTask` flipping status to `'cancelled'` would silently lose to the success continuation overwriting with `'completed'` + bumping `lastDreamAt`. - **DreamDialogEntry.endTime semantic comment (U6)**: documents that `endTime` for cancelled records is the cancel-call moment (not the fork unwind), so a future maintainer doesn't treat it as a real fork-finish timestamp. Tests: new `subscribe() taskType filter` describe block in manager, new task_stop tests for `NOT_CANCELLABLE` (extract) and missing- AbortController (dream); existing test renamed/widened. * fix(core,cli): tighten error semantics + comments on dream surface Four fixes from the latest review pass: - New `TASK_STOP_INTERNAL_ERROR` error type for the missing- AbortController contract violation. Previously the dispatcher reused `TASK_STOP_NOT_RUNNING`, which is misleading — the task IS running, cancellation just couldn't be delivered. Distinct type signals "this is unexpected, file a bug" vs `NOT_CANCELLABLE` which signals "expected behavior, use a different approach". - Reworded the `useBackgroundTaskView` filter comment. Said "every UserQuery that misses the gate creates one [skipped record]" but `scheduleDream` returns `{status: 'skipped'}` early without creating a record for most gate misses; only the acquireDreamLock/EEXIST race actually stores a `'skipped'` record. - Strengthened the `subscribe() unsubscribe` test. The previous version asserted "not called yet" without firing any notify after unsubscribe, so a regression that left the listener attached would still pass. Now schedules an extract before AND after the unsubscribe, verifying the call count doesn't increment. - Moved `const debugLogger = createDebugLogger(...)` below the full import block in manager.ts. Previous version sat between imports, violating eslint-plugin-import's `import/first` rule (didn't trip lint locally, but worth fixing before it does). * fix(core): skip scheduleDream early when params.config is missing `ScheduleDreamParams.config` is optional in the type so test paths can omit it, but production callers always pass one. Without a config, `runManagedAutoMemoryDream` throws because the fork-agent execution requires it. With dream tasks now visible in the Background tasks dialog, that throw becomes a noisy `failed` entry the user sees but didn't trigger. Convert the omitted-config case to the same `disabled` skip path that an explicitly-disabled config takes, so a no-config call short-circuits before any record is stored. Existing tests that relied on the old "no config = proceed past the disabled gate" behavior now pass an explicit `makeMockConfig()` (matching what they would do in any realistic scenario). New test pins the no-config skip behavior + asserts no record was stored (so a regression that drops the early skip would produce a visible failed entry in the dialog and fail the test). * fix(core): plug subscribe Map leak + dream.ts late-cancel ordering bug Two fixes from the latest review pass: - `MemoryManager.subscribe`'s typed-branch unsubscribe deleted the listener from its per-type Set but left the empty Set sitting in `subscribersByType`. Over a long-running session with repeated React mount/unmount of the bg-tasks view, that accumulates dead Map entries forever. Drop the entry when the bucket goes empty. - `runManagedAutoMemoryDream` writes metadata after the fork agent returns (`bumpMetadata` → `rebuildManagedAutoMemoryIndex` → `updateDreamMetadataResult`). If the user presses 'x' between the fork's success return and these writes, the writes proceed and bump `lastDreamAt` — leaving the visible UI ('Stopped') disagreeing with the scheduler gate (sees a recent successful dream and suppresses the next cycle). manager.ts already short-circuits its own metadata write via the post-await abort check, but it can't block writes that already happened inside dream.ts. Adds the same abort-signal check between each write step here. * fix(core): swallow releaseDreamLock errors so they don't poison outcome If `releaseDreamLock` throws inside the inner finally (e.g. filesystem error on the lock file), the exception propagates to the outer catch and overwrites a successfully-completed dream record with 'failed'. The on-disk metadata is already up-to-date at that point, so the user sees a contradictory state — `lastDreamAt` was bumped but the UI shows a failure. Wrap the release in a try/catch with `debugLogger.warn`. The lock file is still cleaned up on the next session via the existing staleness sweep, so swallowing the release error doesn't risk a permanent stuck lock. * fix(core): thread abortSignal into dream metadata writes The pre-call abort checks in `runManagedAutoMemoryDream` close most of the late-cancel race window, but each metadata helper itself does read → mutate → write across two awaits. If the user cancels between those two awaits the write still happens, persisting `lastDreamAt` for an aborted run and suppressing the next legitimate dream cycle. Thread `abortSignal` into `bumpMetadata` and `updateDreamMetadataResult`; both now re-check between the read and the write, returning early without persisting when the signal has already fired. The pre-call checks remain as the first line of defense; this guards the race that opens after the call enters. * fix(core): close dream cancel race + surface lock-release failures Two related fixes from the latest review pass: - Move scheduler-gating metadata writes out of `runManagedAutoMemoryDream` and into `MemoryManager.runDream`, sequenced AFTER the status='completed' flip. The previous shape left a race window where cancellation arriving during/after `fs.writeFile` could persist `lastDreamAt` while the manager flipped status to 'cancelled' — visible UI ('Stopped') would disagree with the scheduler gate (sees a recent successful dream), suppressing the next legitimate dream cycle. The new order makes the gating metadata write race-free: once status !== 'running', cancelTask refuses, so any cancel arriving during the metadata write is ignored. The remaining "cancel raced the synchronous status update" window is handled by a post-update abort recheck that restores 'cancelled' and skips the metadata write. Drops the now-dead `bumpMetadata` helper from dream.ts; index rebuild stays there since it's informational, not gating. - Surface `releaseDreamLock` failures on the task record's metadata (`lockReleaseError`). The previous fix logged-and-swallowed only, so a Windows EPERM or ENOENT race would silently block subsequent dreams as 'locked' with no UI signal explaining why dreaming had stopped. Logger keeps emitting the warn for debug bundles. * fix(core,cli): address 8 review findings on dream surface Reverts the metadata-write behavior regression, plugs the storeWith reentrancy hole, surfaces lock/metadata warnings in the UI, and a handful of cleanups: - Wrap gating-metadata read+write in try/catch (manager.ts). The PR moved metadata writes from dream.ts (best-effort, swallowed) to manager.ts (unguarded). A throw from readDreamMetadata / writeDreamMetadata now propagates to the outer catch and overwrites a successfully-completed dream with 'failed' — the dream actually did its work and touched files are visible. New catch logs + writes `metadataWriteError` on the record so the UI can explain why the next dream may re-fire sooner than expected. - Register the AbortController BEFORE storeWith in scheduleDream. storeWith fires a notify; a subscriber synchronously calling cancelTask(record.id) would otherwise see status='running' but no controller, hitting the missing-controller defensive warn path and reporting a phantom failure on a brand-new dream. - Surface `lockReleaseError` and `metadataWriteError` in the dream view-model and DreamDetailBody (rendered as warnings, not errors, so the terminal status stays Completed). Previous fix wrote them to record.metadata only — nothing in the cli read or rendered them, so users still had no UI signal. - Preserve `result.touchedTopics` on the unreachable cancel-raced- status-update branch. If a future refactor introduces an await there, the restored cancelled record would otherwise drop the already-produced result; the UI would report a clean cancellation even though memory files were already modified. - Add `'cancelled'` to MemoryDreamEvent status union and emit a cancelled telemetry event from the runDream catch path. Without this a cancelled dream is indistinguishable from one that never scheduled in the first place. - Drop the dead abortSignal param from updateDreamMetadataResult in dream.ts (no caller passes it after the manager.ts move). - Swap declaration order of `computeDreamSig` and `refresh` in useBackgroundTaskView.ts (TDZ-fragile against a future refactor that adds a synchronous refresh call between them). - Update the unreachable cancel-raced-update branch comment to describe what's actually true ("defense-in-depth, unreachable today") instead of the confusing "cancelTask flipped" path. - cancelSelected now checks the cancelTask return value and logs via debugLogger when false. Today this branch is unreachable thanks to the controller-register-before-storeWith fix above, but if a future refactor breaks the invariant the silent ignore would let the user think the cancel took effect. - Mirror the manual /dream metadata path's `recentSessionIdsSinceDream = []` reset in the auto path — field is dead code today but keeping the two write sites in sync avoids surprises. Telemetry metric `recordMemoryDreamMetrics` widened to accept the new 'cancelled' status (downstream consumer in loggers.ts). * fix(memory): same-session recovery when releaseDreamLock throws The prior fix surfaced lockReleaseError on the dialog so the user knows the lock release failed (Windows EPERM, ENOENT race, disk full, etc.) — but until next process start, dreamLockExists() still sees a fresh-mtime lock owned by an alive PID (us!) and silently suppresses every subsequent scheduleDream() call as `{status: 'skipped', skippedReason: 'locked'}`. The user sees the warning AND zero further dream activity, and the staleness sweep that would clean the leaked lock only runs at session start. Adds a `dreamLockReleaseFailed` flag set in the catch. The next scheduleDream() force-cleans the leaked lock file via fs.rm({force: true}) before the existence check, so dream scheduling resumes within the same session. Best-effort: if even the forced rm fails (truly unrecoverable filesystem state), falls through to the existing 'locked' skip path. This is an incremental improvement on top of 2af76a752's UI-surface fix. The two together give the full story: warning visible → automatic recovery on next attempt. * fix(memory): real cancelled-dream duration + clarify index-rebuild ordering Two follow-up suggestions from review: - **`duration_ms: 0` in cancelled-dream telemetry** (manager.ts): the user-cancel path emitted `MemoryDreamEvent` with `duration_ms: 0`, which would silently skew latency histograms / p95 metrics by treating cancelled dreams as instant. Capture `dreamStartMs = Date.now()` at the top of `runDream` and emit the real elapsed time in the cancel branch. - **Misleading index-rebuild comment** (dream.ts:75–84): the comment claimed the index rebuild "is still done before returning when topics were touched", but the code returns early on `abortSignal?.aborted` BEFORE the rebuild. Rewrote the comment to describe the actual cancel-aware ordering — abort returns partial result without rebuilding (rebuild is expensive; next dream cycle will rebuild against the latest files anyway), live path rebuilds only when topics changed. 22 / 22 manager tests pass; tsc clean.
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
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
Brings managed auto-memory dream consolidation tasks into the unified Background tasks UI and makes them cancellable. Today, dream tasks fire silently in the background (scheduled per UserQuery via
MemoryManager.scheduleDream) — the user only sees amemory_savedtoast on completion, with no signal about what's running, what it's reviewing, or what failed, and no way to stop a runaway consolidation that's burning model tokens.This PR adds:
1 shell, 1 dream)[dream] memory consolidation reviewing N sessionsx stopkeystroke from the dialog cancels viaMemoryManager.cancelTasktask_stoptool gets a 4th dispatch route so the model can also cancelMemoryTaskStatusgains'cancelled'; cancellation aborts the dream fork-agent and lets the existingrunDreamfinally block release the consolidation lock as the agent unwindsBehavior change for existing users
Heads-up for users on the auto-memory build: pre-PR, dreams were invisible — only a
memory_savedtoast announced completion. Post-PR, every fired dream surfaces in the footer pill (e.g.1 dream) and stays in the Background tasks dialog list for a brief retention window after completion. This is an additive change (no flag flip needed) but users who previously relied on the toast-only signal will now see the pill counter tick. No way to opt out in this PR — if that turns out to be noisy in practice, we can add a setting later.Scope
xkeystroke andtask_stoptoolrunForkedAgent'sAgentPathParamswith anonAssistantMessagecallback, which would ripple through 4 call sites (extract / dream / 2 followup planners). Independent enough to ship separately; wanted to keep the API change in its own PR for blast-radius isolation.extracttasks. They fire on every UserQuery, would flood the pill, thememory_savedtoast inuseGeminiStreamalready covers their completion signal, and they run on the request loop where cancelling would interfere with the user's own turn.task_stoprejects extract task ids withTASK_STOP_NOT_CANCELLABLE(distinct from NOT_FOUND) andMemoryManager.cancelTaskreturnsfalsefor non-dreamtaskType(defense in depth).Risks / blast radius
MemoryTaskStatuswidened with'cancelled': grepped all consumers, onlymanager.tsitself + the cli view-model switch on this enum. No exhaustive-switch downstream that would silently fall through.runForkedAgentAPI is unchanged: the cancellation work only uses the already-existing optionalabortSignalparameter onAgentPathParams. No call-site migration risk for the other 3 fork-agent consumers.runDream's existingfinally { await releaseDreamLock(...) }release the lock as the natural error propagation unwinds. No new cleanup code path means no new chance to leak the lock on edge cases.cancelTaskflips status to'cancelled'BEFORE aborting the signal so the runDream catch path can detect user-intent and skip the'failed'overwrite. Tested directly incancelTask()integration test.getManagedAutoMemoryEnabled()already; users not on that path see zero behavior change.Design notes
cancelTaskflips the record to'cancelled'BEFORE aborting the signal. TherunDreamcatch path then detectssignal.aborted && record.status === 'cancelled'and skips overwriting with a generic'failed'. Lock release stays in the existingfinallyblock — no separate cleanup path needed.MAX_RETAINED_TERMINAL_DREAMS = 3):MemoryManager.tasksis a plain Map with no eviction path. Without a cap on the view-model side, completed dreams accumulate over the project's lifetime. The cap mirrorsMonitorRegistry's terminal-cap pattern; running dreams are uncapped.pending/skippedfilter: every UserQuery creates a task record. Most get gated and end up'skipped'. Surfacing those would render dozens of useless rows per session.Rendering samples
Captured via
ink-testing-libraryat 80x30 (see new dialog tests).List mode — dream alongside other kinds:
Detail · running (with cancel hint shown):
Detail · completed:
Test plan
Automated:
npm run typecheck --workspaces --if-present— clean (cli + core + sdk + webui)npx vitest run --no-coverage src/memory/manager src/tools/task-stop— 31 core tests pass (6 new forcancelTask+ dream-awaretask_stop)npx vitest run --no-coverage src/ui/components/background-view src/ui/hooks/useBackgroundTaskView— 49 cli tests pass (14 new for dream)Manual smoke (concrete recipe — bypass the 24h / 5-session gate by setting both thresholds low and force a recent-session count):
1 dreamwhile it runs; survives terminal-cap pressure (running uncapped, terminal capped at 3 most-recent)xon a running dream → status flips toStopped, fork-agent unwinds, lock released for next dream cycletask_stopwith a running dream id → cancellation succeeds; model calls it again with a completed id → returnsTASK_STOP_NOT_RUNNINGtask_stopwith an extract task id → returnsTASK_STOP_NOT_CANCELLABLE(extract is intentionally not cancellable)Refs #3634
中文版
概述
把 managed auto-memory 的 dream（整理）任务接入统一的 Background tasks UI 并 让它可以被取消。当前 dream 任务每个 UserQuery 由
MemoryManager.scheduleDream静默调度 — 用户只能看到完成时一条memory_savedtoast，对正在跑什么、在 review 什么、是否失败完全没感知；如果 dream 跑飞烧 token 也没法停。本 PR 加入：
1 shell, 1 dream）[dream] memory consolidation reviewing N sessionsx stop键通过MemoryManager.cancelTask取消task_stop工具加第 4 路 dispatch，模型也能取消MemoryTaskStatus加'cancelled'；取消会 abort dream fork-agent，原有runDreamfinally 块在 agent unwind 时释放整理锁用户层面行为变化
给开了 auto-memory 构建的用户提个醒：本 PR 之前 dream 完全静默，只有完成时一条
memory_savedtoast。本 PR 之后每次 fire 的 dream 都会出现在 footer pill（如1 dream），并且在完成后短暂保留窗口期内停留在 Background tasks dialog 列表里。这是纯增量改动（不需要 flag 切换），但之前依赖 toast-only 信号的用户会看到 pill 计数器变。本 PR 没提供 opt-out — 如果实际用下来嫌吵再加 setting。范围
x键和task_stop工具取消runForkedAgent的AgentPathParams加onAssistantMessagecallback，会波及 4 个调用方（extract / dream / 2 个 followup planner）。独立性足够单开extract任务。每个 UserQuery 都跑，频率太高放进 pill 会刷屏；完成信号useGeminiStream里的memory_savedtoast 已经 cover；并且在 request loop 上跑，取消会干扰用户当前 turn。task_stop用TASK_STOP_NOT_CANCELLABLE显式拒绝 extract 任务 id（区别于 NOT_FOUND），MemoryManager.cancelTask对非 dreamtaskType返false（双层防御）风险 / 影响面
MemoryTaskStatus加'cancelled': grep 全 consumer，只有manager.ts自己 + cli view-model switch 这个 enum。下游没有 exhaustive switch 会被静默 fall throughrunForkedAgentAPI 没动: 取消逻辑只用了AgentPathParams已有的可选abortSignal参数。其他 3 个 fork-agent 调用方没有迁移风险runDream已有的finally { await releaseDreamLock(...) }在自然错误传播 unwind 时释放锁。没新清理代码意味着没新的边缘 case 漏锁可能cancelTask在 abort 信号之前把 status 翻到'cancelled'，runDream catch 路径检测到signal.aborted && status === 'cancelled'就跳过'failed'覆盖。cancelTask()集成测试直接 pingetManagedAutoMemoryEnabled()gate；不在那条路径上的用户行为零变化设计要点
cancelTask先把 record flip 到'cancelled'再 abort signal。runDreamcatch 路径检测signal.aborted && record.status === 'cancelled'跳过 generic'failed'覆盖。锁释放走原有finallyblock — 不需要新清理路径MAX_RETAINED_TERMINAL_DREAMS = 3):MemoryManager.tasks是普通 Map，无 eviction。view-model 这边不上 cap 的话，completed dream 会随项目生命周期无限累积。Cap 镜像MonitorRegistry的 terminal-cap 模式；running dream 不限pending/skippedfilter: 每个 UserQuery 创建一条 record，大部分被 gate 拦掉变'skipped'。surface 这些会 dialog 里堆几十行无用项渲染示例
通过
ink-testing-library在 80x30 抓帧（详见新 dialog test）。列表模式 — dream 跟其他 kind 并列：
Detail · running（带取消 hint）：
Detail · completed：
测试 plan
自动化：
npm run typecheck --workspaces --if-present— 干净（cli + core + sdk + webui）npx vitest run --no-coverage src/memory/manager src/tools/task-stop— 31+ core 测试通过（cancelTask + dream-aware task_stop 共加 6+ 个新测试）npx vitest run --no-coverage src/ui/components/background-view src/ui/hooks/useBackgroundTaskView— 49 cli 测试通过（dream 共 14 个新测试）手动 smoke（具体配方 — 把两个阈值降到最低绕开 24h / 5-session gate）：
1 dream；扛住 terminal-cap 压力（running 不限，terminal 只留最近 3 个）x→ 状态翻Stopped，fork-agent unwind，锁释放给下一轮 dreamtask_stop→ 取消成功；再用 completed id 调 → 返TASK_STOP_NOT_RUNNINGtask_stop→ 返TASK_STOP_NOT_CANCELLABLE（extract 故意不可取消）