Skip to content

feat: [ENG-2068] Dream executor, CLI command, and daemon wiring#385

Merged
danhdoan merged 2 commits intoproj/dreamingfrom
feat/ENG-2068
Apr 13, 2026
Merged

feat: [ENG-2068] Dream executor, CLI command, and daemon wiring#385
danhdoan merged 2 commits intoproj/dreamingfrom
feat/ENG-2068

Conversation

@RyanNg1403
Copy link
Copy Markdown
Collaborator

Summary

  • Problem: No mechanism to execute dream (background memory consolidation) tasks or trigger them from CLI
  • Why it matters: Dream executor is the core orchestrator that all three operations (consolidate, synthesize, prune) plug into. Without it, the dream pipeline has no entry point.
  • What changed:
    • New DreamExecutor class with 8-step flow (pre-state capture, dream state load, curate log scan, stub operations, post-dream propagation, dream log write, state update, lock release/rollback)
    • New brv dream CLI command with --force, --format, --timeout flags
    • Transport schemas: added 'dream' to TaskTypeSchema + optional force boolean
    • agent-process.ts: DreamTrigger check + DreamExecutor dispatch in 'dream' case
    • curate-executor.ts: increments curationsSinceDream counter after Phase 4 (fail-open)
    • task-router.ts: forwards force field in TaskExecute message
  • What did NOT change (scope boundary): Operations are NO-OP stubs — consolidate/synthesize/prune implementations come in ENG-2060/ENG-2061/ENG-2062. No idle trigger wiring (ENG-2063). No review system integration.

Type of change

  • New feature

Scope (select all touched areas)

  • Server / Daemon
  • Shared (constants, types, transport events)
  • CLI Commands (oclif)

Linked issues

  • Related ENG-2068 (child of ENG-2059)
  • Depends on ENG-2065, ENG-2066, ENG-2067

Root cause (bug fixes only, otherwise write N/A)

N/A

Test plan

  • Coverage added:
    • Unit test
    • Manual verification only
  • Test file(s): test/unit/infra/executor/dream-executor.test.ts
  • Key scenario(s) covered:
    • Completed dream: log created, state updated (counter reset, lastDreamAt set, totalDreams incremented), lock released
    • Error path: error log saved, lock rolled back with priorMtime
    • First dream (lastDreamAt=null): curate logs not scanned
    • Subsequent dream: curate logs scanned with after filter
    • Trigger propagation: cli/agent-idle trigger value preserved in log
    • State preservation: pendingMerges and version carried through
    • Interactive: brv dream (gate skip), brv dream --force (success), --format json (both skip and completed), curation counter lifecycle

User-visible changes

  • New command: brv dream — runs background memory consolidation (gated by time, activity, queue, lock)
  • New command: brv dream --force / brv dream -f — skips time/activity/queue gates
  • New flag: brv dream --format json — JSON output
  • Curation counter: each brv curate increments curationsSinceDream in .brv/dream-state.json

Evidence

  • 132 dream tests passing (121 existing + 11 new executor tests)
  • Interactive testing: 11 scenarios verified (gate blocking, force bypass, JSON output, state lifecycle, lock behavior)

Checklist

  • Tests added or updated and passing (npm test)
  • Lint passes (npm run lint)
  • Type check passes (npm run typecheck)
  • Build succeeds (npm run build)
  • Commits follow Conventional Commits format
  • No breaking changes (or clearly documented above)
  • Branch is up to date with proj/dreaming

Risks and mitigations

  • Risk: AbortController timeout infrastructure is in place but unreachable in Phase 1 (operations are stubs)
    • Mitigation: Plan 6 (ENG-2064) explicitly wires abort signal checks between operations
  • Risk: Provider check blocks brv dream even though Phase 1 makes no LLM calls
    • Mitigation: Self-resolving when operations are implemented (ENG-2060/2061/2062)

Implements the dream execution orchestrator (8-step flow with NO-OP
operation stubs), `brv dream` CLI command with --force/--format flags,
and wires dreaming into the daemon task pipeline.

- DreamExecutor: lock lifecycle (release on success, rollback on error),
  dream state updates, curate log scanning for changed files, dream log
  persistence with processing→completed/error/partial status transitions
- CLI: `brv dream` (gated) and `brv dream --force` (skip gates 1-3),
  text and JSON output with proper skip/completed distinction
- schemas.ts: add 'dream' to TaskTypeSchema + optional force field
- agent-process.ts: DreamTrigger check + DreamExecutor dispatch
- curate-executor.ts: increment dream curation counter after Phase 4
- task-router.ts: forward force field in TaskExecute message
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 13, 2026

Claude finished @RyanNg1403's task in 4m 12s —— View job


PR Review: feat: [ENG-2068] Dream executor, CLI command, and daemon wiring

Tasks

  • Gather context and understand the request
  • Read changed files
  • Review src/server/infra/executor/dream-executor.ts
  • Review src/oclif/commands/dream.ts
  • Review src/server/core/domain/transport/schemas.ts
  • Review src/server/infra/daemon/agent-process.ts
  • Review src/server/infra/executor/curate-executor.ts
  • Review src/server/infra/process/task-router.ts
  • Review test/unit/infra/executor/dream-executor.test.ts
  • Post inline comments on specific issues
  • Post overall review summary with score

Overall Assessment: 3.5 / 5

The implementation is well-structured and follows established codebase patterns. The 8-step flow is clearly documented, the fail-open design is consistent with how CurateExecutor handles propagation, the lock lifecycle is correct (released only after state write succeeds), and the tests cover the critical success and error paths. The scope boundary (NO-OP stubs for Phase 1) is respected and clearly communicated.

That said, there are a few issues worth addressing before merge.


Issues Found

High Priority

findChangedFilesSinceLastDream return value silently discarded (dream-executor.ts:105)
The method returns Set<string> of changed files since the last dream, but the call discards it (await this.findChangedFilesSinceLastDream(...)). The filtering and access() calls run for nothing in Phase 1. More importantly, when operations are wired in ENG-2060/2061/2062, there's no obvious place to pick this up — the caller will need to explicitly store and pass it. → inline comment posted

TaskCreatedSchema missing 'dream' type (schemas.ts — unchanged line, schema inconsistency)
TaskCreatedSchema.type uses z.enum(['curate', 'curate-folder', 'query', 'search']) but omits 'dream'. TaskTypeSchema (line 634) and TaskExecuteSchema (line 394) both include it. When a dream task is broadcast to the project room via task:created, schema validation would fail. This is a 1-character fix: add 'dream' to the enum in TaskCreatedSchema. Fix this →

as 'json' | 'text' type assertion (dream.ts:56) — violates CLAUDE.md no as Type guideline → inline comment posted

result! non-null assertion (dream.ts:151) — violates CLAUDE.md no ! guideline → inline comment posted

Medium Priority

Magic string 'Dream skipped:' (dream.ts:141) — Brittle coupling between executor result format and CLI display logic. A constant or structured result would be safer. → inline comment posted

computeSummary never increments errors (dream-executor.ts:186) — The DreamLogSummary.errors field will always be zero even after operations are wired in, unless increment logic is added. → inline comment posted

Hardcoded trigger: 'cli' (agent-process.ts:524) — When ENG-2063 adds idle triggering, this will need updating. Threading trigger through TaskExecute now would make ENG-2063 a simpler change. → inline comment posted

Low Priority

Dynamic import of DreamStateService on every curate (curate-executor.ts:156) — A static import is cheaper and more idiomatic; the try/catch still provides the fail-open guarantee. → inline comment posted

finally block ternary (dream-executor.ts:182) — One-liner is harder to read and review; two explicit if/else statements make the lock lifecycle instantly clear. → inline comment posted

AbortController signal never passed to operations — Acknowledged in the PR, structurally correct for Phase 1. Add a // TODO(ENG-2064): pass controller.signal to operations comment so it's not forgotten.


Testing

Coverage is solid for a Phase 1 implementation:

  • ✅ Happy path (log created, state updated, lock released)
  • ✅ Error path (error log saved, lock rolled back with correct priorMtime)
  • ✅ First dream (no curate log scan)
  • ✅ Subsequent dream (curate log scanned with after filter)
  • ✅ Trigger propagation
  • ✅ State preservation (pendingMerges, version)
  • ✅ Log save failure on success path rolls back lock

Missing (acceptable for Phase 1):

  • The 'partial' status (AbortController abort) path — unreachable until operations use the signal
  • File existence filtering inside findChangedFilesSinceLastDream — the access() filtering logic in the private method is not exercised (the test uses a nonexistent project root so all files fail, but no test verifies that behavior explicitly)

Positive Notes

  • Lock lifecycle is correct: succeeded flag is set after state write, so lock is only released when the full sequence completes. Rollback on any failure path is guaranteed via finally.
  • Promise.all(checks) for parallel access() calls in findChangedFilesSinceLastDream is a nice touch — avoids the no-await-in-loop lint issue and is faster.
  • Fail-open design is consistent throughout (snapshot, propagation, curate counter increment) — dream never blocks on non-critical steps.
  • task-router.ts change is minimal and correct — conditional spread data.force === undefined ? {} : {force: data.force} properly handles the optional field without polluting messages with undefined.

Comment thread src/server/infra/executor/dream-executor.ts Outdated
Comment thread src/server/infra/executor/dream-executor.ts
Comment thread src/oclif/commands/dream.ts Outdated
Comment thread src/oclif/commands/dream.ts Outdated
Comment thread src/oclif/commands/dream.ts
Comment thread src/server/infra/daemon/agent-process.ts
Comment thread src/server/infra/executor/curate-executor.ts Outdated
Comment thread src/server/infra/executor/dream-executor.ts
Comment thread src/server/infra/executor/dream-executor.ts Outdated
Comment thread test/unit/infra/executor/dream-executor.test.ts Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 91b315664f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/server/infra/executor/dream-executor.ts
Comment thread src/server/infra/daemon/agent-process.ts
- Capture findChangedFilesSinceLastDream return value for future ops
- Document why computeSummary.errors is always 0 (log-level, not per-op)
- Remove `as` type assertion in dream CLI (use ternary narrowing)
- Remove `result!` non-null assertion (use nullish fallback)
- Static import DreamStateService in curate-executor (was dynamic)
- Explicit if/else for lock release/rollback readability
- Double-cast test stub for intentionality
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 13, 2026

Claude finished @RyanNg1403's task in 1m 59s —— View job


PR Review: feat: [ENG-2068] Dream executor, CLI command, and daemon wiring

Tasks

  • Gather context and understand the request
  • Read changed files and diff against base branch
  • Review src/server/infra/executor/dream-executor.ts
  • Review src/oclif/commands/dream.ts
  • Review src/server/core/domain/transport/schemas.ts
  • Review src/server/infra/daemon/agent-process.ts
  • Review src/server/infra/executor/curate-executor.ts
  • Review src/server/infra/process/task-router.ts
  • Review test/unit/infra/executor/dream-executor.test.ts
  • Post overall review summary with score

Overall Assessment: 4 / 5

Good follow-through on the previous round of feedback — most issues were addressed cleanly. The as 'json' | 'text' assertion is gone, result ?? '' replaces the ! assertion, DreamStateService is now statically imported, the finally block ternary is expanded, changedFiles is now captured (with eslint-disable and a comment), and computeSummary.errors has a clear doc comment. The test file also swaps the bare as ICipherAgent for the double-cast as unknown as ICipherAgent.

A few issues remain unfixed, one of which is a correctness bug.


Issues Remaining

High Priority

TaskCreatedSchema still missing 'dream' (schemas.ts:515)

TaskCreatedSchema.type is still z.enum(['curate', 'curate-folder', 'query', 'search']) while TaskExecuteSchema (line 394) and TaskTypeSchema (line 634) both include 'dream'. When a dream task is broadcast to the project room via task:created, Zod validation fails. This is a one-character fix:

// line 515 — add 'dream'
type: z.enum(['curate', 'curate-folder', 'dream', 'query', 'search']),

Fix this →

Processing-log write before try/finally (dream-executor.ts:74–87, Codex P1)

getNextId() (line 74) and save(processingEntry) (line 87) execute before the try block that holds the finally with lock release. If either call throws (e.g. disk-full or permission error), the dream lock acquired by DreamTrigger is never released or rolled back — subsequent dreams will be permanently blocked until lock staleness handling kicks in.

    let succeeded = false
    let logId: string
    try {
      logId = await this.deps.dreamLogStore.getNextId()
      const startedAt = Date.now()
      // ... processingEntry save inside try ...

Move both calls inside the try block, initialise logId with let, and ensure the finally handles the case where logId may still be undefined (e.g. skip the error log save).

Fix this →

Medium Priority

Magic string 'Dream skipped:' (dream.ts:141) — unchanged from previous review. The string couples CLI display logic to the executor's text format. If DreamTrigger.tryStartDream changes its skip message prefix, the check silently breaks and skip messages display as completions. A shared constant in shared/ (e.g. DREAM_SKIP_PREFIX) would protect this boundary. Fix this →

Hardcoded trigger: 'cli' (agent-process.ts:525) — unchanged. When ENG-2063 adds idle triggering, this will need updating. Threading trigger through TaskExecute now would make ENG-2063 a one-field addition rather than a call-site change. At minimum, add a // TODO(ENG-2063): thread trigger through TaskExecute comment.

getQueueLength: () => 0 (agent-process.ts:507, Codex P2) — this permanently short-circuits the queue-empty gate, so brv dream can fire even when the project queue is non-empty. The inline comment "Agent-level: this task is the only active one" explains the reasoning but the explanation is slightly misleading — the queue can still have other tasks waiting behind this one. If the intent is to always pass gate 3 at the agent level, document that explicitly: // Queue gate is intentionally bypassed at agent dispatch level — ENG-2063 may revisit.

Low Priority

caught! in test file (dream-executor.test.ts:124, 208) — The previous review fixed as ICipherAgent but caught!.message at lines 124 and 208 still uses non-null assertions. Since caught is Error | undefined and checked with expect(caught).to.be.instanceOf(Error) immediately above, TypeScript still can't narrow it. Use caught?.message or (caught as Error).message with as unknown as or a runtime guard:

// line 124
expect(caught?.message).to.equal('disk full')

Fix this →

Missing TODO(ENG-2064) for AbortController signal (dream-executor.ts, near Step 4) — flagged in previous review, still absent. One line prevents this from being forgotten when ENG-2060/2061/2062 land.


What Was Fixed vs Previous Review

Issue Status
findChangedFilesSinceLastDream return discarded ✅ Fixed — captured as changedFiles with ESLint disable + comment
as 'json' | 'text' assertion ✅ Fixed — type guard ternary
result! non-null assertion ✅ Fixed — result ?? ''
computeSummary errors never incremented ✅ Fixed — doc comment explains it's log-level, not per-op
Dynamic import of DreamStateService ✅ Fixed — static import at top of file
finally ternary readability ✅ Fixed — explicit if/else
agent = {} as ICipherAgent in tests ✅ Fixed — as unknown as ICipherAgent double-cast
TaskCreatedSchema missing 'dream' ❌ Still missing
Magic string 'Dream skipped:' ❌ Still present
Hardcoded trigger: 'cli' ❌ Still hardcoded
Processing-log write before try/finally ❌ Still unfixed (Codex P1)
caught! in tests ❌ Still present

Testing

No changes to test coverage in this revision. Coverage remains solid for Phase 1 — happy path, error path, lock lifecycle, trigger propagation, first vs. subsequent dream, state preservation. The partial path (AbortController abort) and findChangedFilesSinceLastDream file-existence filtering remain unexercised, which is acceptable for Phase 1.

Copy link
Copy Markdown
Collaborator

@danhdoan danhdoan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@danhdoan danhdoan merged commit 9477c4a into proj/dreaming Apr 13, 2026
6 checks passed
@RyanNg1403 RyanNg1403 deleted the feat/ENG-2068 branch April 14, 2026 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants