refactor: [ENG-2884] remove legacy LLM-driven brv dream path#689
Merged
Conversation
Tool-mode subcommands (`brv dream scan / finalize / undo / sessions /
cancel`, shipped in ENG-2858) cover the same workflow deterministically
— daemon enumerates candidates, agent decides via `brv curate`, daemon
archives via `finalize`. No provider required. With tool-mode as the
project default the legacy LLM-driven entry surface is orphaned, so
remove it cleanly.
`brv dream` (no subcommand) is now a thin topic root that prints a
migration hint. `--force`, `--timeout`, `--undo` on the topic root are
gone; users hit `brv dream undo` instead. The `buildUndoDeps` export
stays here since the `undo` subcommand imports it.
Removed
- `src/oclif/commands/dream.ts` LLM-driven `run()` body (was a 200-line
block dispatching dream tasks through the daemon). Topic root keeps
the `buildUndoDeps` helper for the `undo` subcommand.
- `case 'dream'` in `agent-process.ts` — daemon-side LLM dispatch.
- `AgentIdleTimeoutPolicy.onAgentIdle` dream-eligibility branch and
`preDispatchCheck` for `task.type === 'dream'` in `brv-server.ts`.
The auto-dispatch had no provider-config guard, so on tool-mode
projects it would silently try to dispatch and fail — removal closes
that latent issue. Idle agents are now just cleaned up.
- `src/server/infra/executor/dream-executor.ts` + its test.
- `src/server/infra/dream/dream-trigger.ts` + its test.
- `src/server/infra/dream/operations/{consolidate,synthesize,prune}.ts`
+ their tests. Tool-mode subcommands use `src/server/infra/dream/
tool-mode/*` instead.
- `src/server/infra/dream/parse-dream-response.ts` +
`dream-response-schemas.ts` + their tests — no consumer after the
operations files are gone.
- `src/oclif/lib/timeout-deprecation.ts` + its test — only consumer
was the legacy dream command.
Trimmed
- `test/commands/dream.test.ts` rewritten as a 1-case smoke that
asserts the new topic-root behaviour (prints the subcommand listing).
- `test/unit/infra/runtime-signals/sidecar-failure-logging.test.ts`
dropped the `dream operations` describe block (consolidate / prune
sidecar warnings). Other describe blocks (curate-tool, archive
service, manifest, search, commit-6 wiring) keep their coverage.
Kept
- `src/server/infra/dream/dream-undo.ts`, `dream-state-service.ts`,
`dream-lock-service.ts`, `dream-log-store.ts` — LLM-agnostic services
reused by the tool-mode pipeline.
- `src/server/infra/dream/tool-mode/*.ts` — the subcommand pipeline.
- `src/oclif/commands/dream/{scan,finalize,sessions,cancel,undo}.ts`.
Verified
- npm run typecheck: clean
- npm run lint: 0 errors (256 pre-existing warnings)
- npm test: 8337 passing, 16 pending, 0 failing (was 8521 — net -184
from the dead test deletions; matches the deleted file inventory).
Migration
- `brv dream` (no subcommand) now prints "Use a subcommand: brv dream
{scan|finalize|undo|sessions|cancel}" and exits 0.
- `brv dream --force` / `--timeout` / `--undo` removed. Use the
subcommands.
- Background dream is no longer daemon-scheduled; the agent decides
when to run `brv dream scan` (e.g. post-curate burst, on session
start). The v2 model is agent-driven cadence.
cuongdo-byterover
requested review from
bao-byterover and
leehpham
as code owners
Contributor
|
Claude finished @cuongdo-byterover's task in 5m 32s —— View job Claude PR Review — ENG-2884 (remove legacy LLM-driven
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…lose schema/post-work straggler gates
Two should-fix items + comment cleanup from the reviewer:
1. DREAM_REMOVED_FLAGS was exported and unit-tested but never consumed.
`brv dream --timeout 30` printed the topic-root hint instead of the
migration message that names --timeout. Wired
`findRemovedFlagMessage(this.argv, DREAM_REMOVED_FLAGS)` into the
topic root with the same JSON-envelope / this.error branching the
curate and query commands use.
2. Dropped 'dream' from the transport schemas + post-work gate so any
straggler `type: 'dream'` task that still hits the daemon (older
client, stale queue, internal stray producer) fails at the schema
boundary instead of silently completing with result = ''.
- TaskTypeSchema + TaskExecuteSchema type enums: 'dream' removed.
- TaskExecuteSchema.force field: removed (dream was the only consumer).
- TaskExecuteSchema.trigger enum: 'agent-idle' removed (idle-trigger
dispatch is gone). 'cli' and 'manual' remain.
- TaskCreateRequestSchema.force field: removed.
- agent-process.ts post-work gate: dropped `type === 'dream'`.
- task-router.ts: dropped the orphan `data.force` plumbing.
Cosmetic
- task-router.test.ts: swapped the `type: 'dream'` fixtures (used to
exercise the generic preDispatchCheck plumbing) to `type: 'curate'`.
The tests stay — they cover the gate mechanism, not dream-specific
logic.
- review.ts description: dropped the stale `brv dream needsReview`
bullet. Tool-mode dream-finalize writes `needsReview: false`
unconditionally; the toggle has no dream-side effect to describe.
- Cleared stale `dream-executor` comment refs in curate-session.ts,
dream-state-service.ts, agent-process.ts, and two test files.
- Topic-root exit code: kept exit 0 (matches oclif topic-root default)
with a clarifying code comment noting the deliberate choice.
Kept
- `'agent-idle'` in dream-log-schema.ts — backwards-read compat for
existing dream-log entries on disk. No new entries will carry that
trigger; existing files still parse.
Verified
- npm run typecheck clean
- npm run lint 0 errors (256 pre-existing warnings)
- npm test: 8337 passing, 16 pending, 0 failing
Contributor
|
Claude finished @cuongdo-byterover's task in 3m 52s —— View job Claude PR Review — ENG-2884 (synchronize)
SummaryScore: 4.5 / 5 — Both should-fix items from the prior review are cleanly addressed and the cosmetic loose ends ( Previous review items: status
Praise — the Inline comments (both nits)
Verification stance — spot-checked grep across the repo: no orphaned |
…nt path normalization) The dream-scan output emits candidate paths with .html (e.g. auth/jwt.html) and the documented dream→curate merge workflow tells the agent to write the survivor at that path. The writer previously appended .html unconditionally, producing auth/jwt.html.html on disk while reporting status: done and bypassing the path-exists guard — silent data corruption during merge: the survivor's merged content landed in a stale file while the agent archived the loser thinking the merge had succeeded. topicPathToFilePath now strips a trailing .html before appending. Both path="x/y" (bare convention) and path="x/y.html" (the form dream-scan emits) collapse to the same on-disk file at x/y.html and trigger the path-exists guard identically. 4 new tests in html-writer.test.ts cover: .html-suffixed path writes to single-extension file; path-exists fires across both forms in both orderings; --overwrite leaves only one file.
… capture pre-archive mtime/signals so dream undo is bit-exact Three coupled changes that touch the search-service contract and the tool-mode dream finalize/undo path. Search service contract: - ISearchKnowledgeService gains refreshIndex(): invalidates the cached MiniSearch index so the next search() call rebuilds from disk regardless of TTL. Required because the service's 5s TTL fast-path would otherwise serve a cached index that pre-dates just-loaded topics, surfacing zero candidates on a fresh scan. - SearchKnowledgeOptions gains combineWith?: 'AND' | 'auto' | 'OR'. Default 'auto' preserves the historical AND-first-with-OR-fallback behavior. Internal SearchOptions references the public option's type to keep the two declarations from drifting if a new value is added. - bm25-pair-discovery passes combineWith: 'OR'. Multi-word distinctive titles (e.g. "Redis Caching Layer") AND-combine to terms only the source itself contains; OR-mode lets cross-pairs surface based on any-term overlap. Dream finalize / undo signal+mtime restore: - finalizeDreamSession captures each archived file's mtimeMs (via stat) and current RuntimeSignals (via runtimeSignalStore.get) before the rename, and returns them on the result alongside previousTexts. - DreamLogSchema PRUNE op gains optional previousSignals + previousMtimes (backward-compat — older logs still undo via fallback). - agent-process dream-finalize branch writes the new metadata into each PRUNE op. - DreamUndoDeps gains optional runtimeSignalStore. undoPrune restores mtime via utimes() and signals via runtimeSignalStore.set() when the PRUNE op carries the new fields; falls back gracefully when not. - buildUndoDeps threads runtimeSignalStore through. scanDreamCandidates now invokes searchService.refreshIndex() after loadToolModeTopics and before the parallel candidate-generator block, so the first scan after seed produces full results. Tests cover: refreshIndex called pre-discovery; combineWith: 'OR' passed to search; mtime+signal capture on finalize; mtime+signal restoration on undo; backward-compat fallback path; mock updates for the extended ISearchKnowledgeService in unrelated test files.
Both subcommands are intentional v1 placeholders — the daemon doesn't
persist session state, so dream sessions always returns [] and dream
cancel is a no-op. The previous surface advertised them as if they
were functional: descriptions said 'List active...' / 'Discard a...',
JSON envelopes returned success-looking shapes ({sessions: [], status:
ok} / {status: cancelled}), and machine-readable consumers that
branched on the response had no way to know the daemon was never
consulted.
This is a cheap-honesty fix, not a real persistence implementation.
Real persistence is a separate ticket.
Changes:
- DreamSessions.description prefixed with '[v1 stub]' and now names
the stateless behavior inline.
- DreamCancel.description prefixed with '[v1 stub]' and now names the
no-op behavior inline.
- Both JSON envelopes gain a 'note' field disclosing v1 status so
machine consumers see the caveat at the same key they'd branch on.
- Topic-root listing in brv dream's run() body flags both subcommands
as '[v1 stub]' in the one-liner hint.
4 tests in test/commands/dream.test.ts cover: each static description
contains 'v1 stub'; each --format json envelope's data.note discloses
stateless / no-op semantics.
…, worked example A skill-discovery sub-agent test surfaced gaps in §7 (brv dream): prose described the surface but no sample JSON envelopes existed, prune-signal thresholds were left to the agent to guess, and there was no end-to-end walkthrough — so consumers had to grep the source to learn field shapes. Additions to §7: - Sample dream scan --format json envelope (sessionId + candidates with one example each of link / merge / prune / synthesize shapes, including signals + daysSinceModified for prune). - Sample dream finalize response (archived, skipped, logId, status). - Prune signal thresholds inline: importance < 35 → low-importance, draft >60d / validated >120d → stale-mtime, maturity 'core' never surfaced. - 'Worked example — prune the stalest topic (no curate detour)': three-block scan → finalize → undo walkthrough. - Path-attribute warning in the merge bullet so agents stripping the scan-emitted .html before <bv-topic path=> see the guidance. - Updated undo description to reflect the symmetric mtime + signal restoration shipped in this PR — was 'mtime resets to current time'; is 'content, original mtime, and sidecar runtime signals... all restored'. Companion change in prune-candidates.ts: sync-hint comment near the threshold constants pointing at SKILL.md §7, so future threshold changes don't silently drift the agent-facing docs.
…566ac) Reviewer caught a real race in the refreshIndex() shipped in f5566ac. The orphan-builder problem: T0: caller A invokes search() — acquireIndex starts buildA; state.buildingPromise = buildA. The IIFE body is still awaiting fileSystem.listDirectory and the eventual buildFreshIndex. T1: scanDreamCandidates calls refreshIndex(). Old code cleared state.cachedIndex AND state.buildingPromise immediately and returned. T2: caller C invokes search() — sees both fields cleared, kicks off a NEW build (buildB). state.buildingPromise = buildB. T3: buildA finally completes and writes state.cachedIndex = freshIndexA (line 1016 of acquireIndex). T4: buildB completes and writes state.cachedIndex = freshIndexB. If buildA happens to settle AFTER buildB (e.g. slower disk read, larger source set), the older freshIndexA wins — reintroducing the exact TTL-stale-data scenario Fix #2 was meant to prevent. Realistic on a daemon with concurrent brv search / brv query / dream-scan tasks. Fix per reviewer's accepted shape: refreshIndex() now AWAITS any in-flight buildingPromise before clearing cached state. By the time the await returns, the in-flight build has either published its result (which is then cleared) or rejected (swallowed — callers don't care about that build's outcome, they just want a clean slate). No orphan publisher can write back after invalidation because the publishing point is now in the past relative to the clear. New test file search-knowledge-service-refresh.test.ts covers: - the contract under test (refreshIndex MUST NOT resolve while a build is in flight) - idempotent behavior on cold state (no in-flight build) - graceful clear when the in-flight build rejects Not a follow-up sub-agent regression replay: the race manifests only under concurrent operations on a single SearchKnowledgeService instance, which the 12-scenario test matrix (per-scenario daemons, serial workflow) does not exercise. The unit test at the contract boundary is the right pin.
RyanNg1403
requested review from
RyanNg1403,
danhdoan and
ngduyanhece
as code owners
Contributor
|
Claude finished @RyanNg1403's task in 6m 38s —— View job Claude PR Review — ENG-2884 (synchronize)
SummaryScore: 4.5 / 5 — Five additional non-merge commits since the previous review (7a397ba … 8c7c1cc) move beyond "address review feedback" and into real follow-up hardening: the orphan-builder race in Status of prior-review nits
New observations from this round (all non-blocking — full detail inline)
Verification stance — grep across the repo: no orphaned |
Five items from the latest codex review batch on PR #689. - agent-process.ts: drop the dead-code `?? ''` fallback on previousTexts. archived[] paths are invariant-guaranteed to have all three fields (text + mtime + signals) populated together, so the fallback was already unreachable — and if it ever fired, the resulting `{[path]: undefined}` would violate the new PRUNE schema's z.record(z.string(), z.number()) for previousMtimes. All three lookups now agree: trust the invariant uniformly. - search-knowledge-service.ts: extend refreshIndex docstring with a maintainer note explaining that the explicit `state.buildingPromise = undefined` after the await is defensive (acquireIndex's finally block already nulls it). Also documents the benign double-build inefficiency when a fresh acquireIndex() races into the gap between await-resolve and clear — not a correctness issue, just non-obvious. - SKILL.md §7: soften the merge bullet's path-attribute warning. The prior "drop the trailing .html — the writer always appends it" instruction was load-bearing before the path-doubling fix shipped; now that the writer normalizes idempotently (both bare and .html-suffixed forms resolve to the same on-disk file), the instruction reduces to agent-facing cognitive load. Updated to "either form works; the writer normalizes" so the agent doesn't have to track which form scan emitted. - dream.test.ts: add two regression tests for the DREAM_REMOVED_FLAGS wire-up in dream.ts's run() body. (1) text mode: --timeout 30 rejects with this.error throwing before this.log runs; (2) json mode: --timeout 30 --format json emits a JSON envelope with success:false / data.status:'error' / data.error containing '--timeout'. Pins the early-exit ordering so a future refactor that drops the findRemovedFlagMessage(this.argv, ...) call fails the suite instead of silently regressing to pre-removal state. - schemas.ts: hoist TaskTypeSchema declaration above TaskExecuteSchema (the schema previously sat ~100 lines below) and replace TaskExecuteSchema.type's inline z.enum(...) with a reference to TaskTypeSchema. The doc block at the original declaration site claimed it was the single source of truth, but the inline enum on TaskExecuteSchema.type silently violated that — the only outlier; TaskCreatedSchema.type (now line 605) and TaskCreateRequestSchema.type already referenced TaskTypeSchema. Hoist needed to avoid a Zod TDZ at module load. No behavioral change; schemas now share one literal enum source.
Contributor
|
Claude finished @RyanNg1403's task in 2m 59s —— View job Claude PR Review — ENG-2884 (synchronize)
SummaryScore: 5 / 5 — One new follow-up commit since the 15:14Z review ( Status of prior-review items
Praise —
Tiny non-issue worth mentioning in passing — Verification stance — grep confirms no orphaned imports of PR ready to merge from my read. · branch |
danhdoan
approved these changes
May 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Tool-mode subcommands (
brv dream scan / finalize / undo / sessions / cancel) cover the same workflow deterministically — daemon enumerates candidates, agent decides viabrv curate, daemon archives viafinalize. No provider required. With tool-mode as the project default the legacy LLM-driven entry surface is orphaned, so remove it cleanly.brv dream(no subcommand) is now a thin topic root that prints a migration hint.--force,--timeout,--undoon the topic root are gone; users hitbrv dream undoinstead. ThebuildUndoDepsexport stays here since theundosubcommand imports it.Scope expansion (5 polish commits on top of the original refactor)
After the legacy removal landed, a 12-scenario regression test against the surviving tool-mode surface surfaced 6 pre-existing P1/P2 defects. Without the legacy escape hatch, consumer agents are now forced through these rough edges, so they ship in the same PR rather than as a follow-up.
7a397ba0strip trailing.htmlintopicPathToFilePath*.html.htmlwhen the agent puts.htmlin<bv-topic path=>. Closes a silent-data-corruption window in the documented dream→curate merge workflow:dream scanemits candidate paths with.html; the agent following the docs literally would have created a stale survivor while archiving the loser.f5566acdsearch-servicerefreshIndex+combineWith; capture pre-archive mtime/signals for bit-exact undoISearchKnowledgeServicegainsrefreshIndex()soscanDreamCandidatesbypasses the 5s TTL fast-path — first scan after seed now returns full results, no warm-up needed. (b)SearchKnowledgeOptionsgainscombineWith?: 'AND' | 'auto' | 'OR'; dream pair-discovery passes'OR'so multi-word distinctive titles (e.g. "Redis Caching Layer") surface cross-pairs. (c)finalizeDreamSessioncaptures pre-archivemtimeMs+ sidecarRuntimeSignals;DreamLogSchemaPRUNE op carries them as optional fields;undoPrunerestores viautimes()+runtimeSignalStore.set(). Undo is now bit-exact for content + mtime + signals — pruned topics that re-qualify re-surface on the next scan.2e138a6cdisclose v1-stub status ondream sessions/dream cancel[v1 stub], JSON envelopes carry anotefield disclosing stateless/no-op behavior, topic-root listing flags both. Honest surface without doing the real persistence work, which stays a separate ticket.53004fd7expand SKILL.md §7 with sample envelopes, thresholds, worked exampledream scan+dream finalizeJSON envelopes, prune-signal thresholds inline (importance < 35, draft >60d / validated >120d), end-to-end "prune the stalest topic" walkthrough, path-attribute warning, and updates the undo description for the new bit-exact behavior. Companion sync-hint comment inprune-candidates.tsso future threshold changes can't drift the docs.8c7c1cc3await in-flight build inrefreshIndex(race fix onf5566acd)state.buildingPromisewhile a build was running let the orphan build write back tostate.cachedIndexafter invalidation, reintroducing TTL-stale behavior under concurrentbrv search+ dream-scan.refreshIndex()now awaits any in-flight build before clearing — by the time it returns, no orphan publisher can write back.Removed (original scope)
src/oclif/commands/dream.tsLLM-drivenrun()body (was a 200-line block dispatching dream tasks through the daemon). Topic root keeps thebuildUndoDepshelper for theundosubcommand.case 'dream'inagent-process.ts— daemon-side LLM dispatch.AgentIdleTimeoutPolicy.onAgentIdledream-eligibility branch andpreDispatchCheckfortask.type === 'dream'inbrv-server.ts. The auto-dispatch had no provider-config guard, so on tool-mode projects it would silently try to dispatch and fail — removal closes that latent issue. Idle agents are now just cleaned up.src/server/infra/executor/dream-executor.ts+ its test.src/server/infra/dream/dream-trigger.ts+ its test.src/server/infra/dream/operations/{consolidate,synthesize,prune}.ts+ their tests. Tool-mode subcommands usesrc/server/infra/dream/tool-mode/*instead.src/server/infra/dream/parse-dream-response.ts+dream-response-schemas.ts+ their tests — no consumer after the operations files are gone.src/oclif/lib/timeout-deprecation.ts+ its test — only consumer was the legacy dream command.Trimmed
test/commands/dream.test.tsrewritten as a 1-case smoke that asserts the new topic-root behaviour (prints the subcommand listing), then extended in2e138a6cwith 4 tests for v1-stub disclosure (descriptions + JSON note).test/unit/infra/runtime-signals/sidecar-failure-logging.test.tsdropped thedream operationsdescribe block (consolidate / prune sidecar warnings). Other describe blocks (curate-tool, archive service, manifest, search, commit-6 wiring) keep their coverage.Kept
src/server/infra/dream/dream-undo.ts,dream-state-service.ts,dream-lock-service.ts,dream-log-store.ts— LLM-agnostic services reused by the tool-mode pipeline.dream-undo.tsextended inf5566acdwith optionalruntimeSignalStoredep and mtime/signal restore inundoPrune.src/server/infra/dream/tool-mode/*.ts— the subcommand pipeline.dream-session.tsextended inf5566acd(refreshIndex call + mtime/signal capture in finalize).src/oclif/commands/dream/{scan,finalize,sessions,cancel,undo}.ts.sessions.tsandcancel.tsextended in2e138a6c.Verified
npm run typecheck: cleannpm run lint -- --quiet: 0 errorsnpm run build: cleannpm test: 8379 passing, 16 pending, 0 failingtype: 'dream'IS rejected;force/trigger: 'agent-idle'are silently stripped because the surrounding schemas aren't.parse'd at runtime today). Every previously-flagged P1/P2 defect verified patched.Migration
brv dream(no subcommand) now printsUse a subcommand: brv dream {scan|finalize|undo|sessions [v1 stub]|cancel [v1 stub]}and exits 0.--force/--timeout/--undo) removed. Use the subcommands.brv dream sessions/brv dream cancelare v1 stubs (disclosed in help text and JSON output). Real persistence is a separate ticket.brv dream undonow restores file content + pre-archive mtime + sidecar runtime signals (bit-exact). Pruned topics that re-qualify re-surface on the next scan.brv dream scan(e.g. post-curate burst, on session start). The v2 model is agent-driven cadence.What did NOT change (scope boundary)
Deliberately deferred to follow-up tickets:
dream sessions/dream cancel(v1 stubs are the bridge).force,trigger: 'agent-idle') — fields are behaviorally inert butTaskCreateRequestSchema/TaskExecuteSchemaaren't.parse'd at runtime today.dream undoexits 0 on no-op (JSON envelope'ssuccess: falseis the current scripted-caller signal); deadrestoredArchivesfield in undo response; empty archive dir shells after undo; server-injectedcreatedat/updatedatdespite prompt wording.Type of change
Scope
Test plan
User-visible changes
brv dreamtopic root now lists subcommands with[v1 stub]markers on sessions/cancel.brv dream sessions --helpandbrv dream cancel --helpdescriptions begin with[v1 stub]and document v1 stateless/no-op behavior.brv dream sessions --format jsonandbrv dream cancel --format jsoncarry anotefield disclosing v1 status.brv dream undorestores bit-exact mtime + sidecar signals (previously only content was restored; mtime reset to wall-clock and signals to defaults).brv dream scanfirst call after a fresh seed now returns full candidate set (previously empty on cold-cache, only warmed up on a second call).Risks and mitigations
DreamLogSchemaPRUNE op gained optionalpreviousSignals+previousMtimesfields.SearchKnowledgeOptions.combineWithandISearchKnowledgeService.refreshIndex()change the search-service contract.combineWithdefault'auto'preserves the historical AND-first-with-OR-fallback behavior; only dream pair-discovery opts in to'OR'.refreshIndex()is additive; no caller is required to invoke it.DreamUndoDeps.runtimeSignalStorenewly required for signal restore.undoPruneno-ops the signal restore when the dep isn't injected.buildUndoDepsindream.tswires it through; the only callers that need updating are external embedders.