fix: [ENG-2126] address 4 dream spec-violation gaps post-merge by RyanNg1403 · Pull Request #434 · campfirein/byterover-cli

RyanNg1403 · 2026-04-16T10:35:01Z

Summary

Problem: Four implementation gaps in proj/dreaming where the shipped code violated the spec at notes/byterover-dream/. All filed together because they share the same project surface and are all "spec says X, code does Y" bugs: counter race in the curation gate, pendingMerges never consumed by the next consolidate, queue gate (Gate 3) bypassed on the CLI dispatch path, and partial/error dreams losing the operations audit trail (which also broke brv dream --undo for partial runs).
Why it matters: Auto-dream at agent-idle could silently fail to trigger; prune's MERGE_INTO suggestions silently lingered forever; brv dream competed with queued tasks instead of yielding; and a dream that committed real merges before timing out left the context tree modified with no audit trail and no recovery path.
What changed: Generic update(updater) on DreamStateService backed by a per-file mutex, with all dream-state RMW callers (incrementCurationCount, executor's step-7 reset, consolidate's pendingMerges clear) routed through it; loadAndClearPendingMerges() consumer at the start of consolidate per notes/byterover-dream/6-dream-undo-and-cross-cycle.md:89-143; preDispatchCheck hook on TaskRouter so the daemon pre-checks gate 3 on the CLI path (matching the existing idle-trigger pre-check); allOperations accumulator hoisted above the try block so partial/error log entries carry the ops that did complete, with createReviewEntries invoked for partial dreams.
What did NOT change (scope boundary): Dream operation semantics (consolidate / synthesize / prune behavior), gate definitions and ordering, dream-log schema, undo schema, CLI surface, oclif command flags. The 4 fixes are surgical — no refactor of surrounding code.

Type of change

Scope (select all touched areas)

Linked issues

Closes #
Related # — Linear ENG-2126 (link); stacks on proj/dreaming after enhance: [ENG-2128] inline file contents in operation prompts #432 (ENG-2128)

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

Root cause:
- Issue 1 (counter race): incrementCurationCount did unguarded read-modify-write on dream-state.json. Comment at dream-state-service.ts:27-28 claimed "single-writer assumption" but AGENT_MAX_CONCURRENT_TASKS=5 (constants.ts:94) allows up to 5 parallel curates per project — increments were lost to last-writer-wins. Two adjacent RMW sites (executor step-7 reset, consolidate pendingMerges clear) had the same race shape.
- Issue 2 (pendingMerges): prune.ts:417-424 wrote SUGGEST_MERGE decisions into dreamState.pendingMerges for the next dream's Consolidate to pick up — the consumer was specified in notes/byterover-dream/6-dream-undo-and-cross-cycle.md:89-143 but never wired in consolidate.ts. Entries sat in state forever.
- Issue 3 (queue gate): agent-process.ts:513 constructed DreamTrigger with getQueueLength: () => 0 and a comment "this task is the only active one" — false under AGENT_MAX_CONCURRENT_TASKS=5. The idle-trigger path pre-checked gates 1-3 in brv-server.ts:260 before dispatch, but the CLI path went straight through with the agent-side gate 3 effectively disabled.
- Issue 4 (partial-log): dream-executor.ts:149 declared allOperations after the three op awaits; the catch handler had nothing to reference and hardcoded operations:[] and summary:zeroes. A consolidate that committed real merges followed by a synthesize timeout left files modified with no log record and brv dream --undo as a no-op.
Why this was not caught earlier:
- Issue 1: Per-test mocks satisfy a single-writer assumption that the daemon's concurrency model violates; no concurrent-write regression test existed. The 2 sibling RMW sites (executor + consolidate) needed a follow-up because the original Issue 1 fix only serialized incrementCurationCount against itself — flagged by PE review on this branch and addressed in the second-pass commit.
- Issue 2: Spec exists at notes/byterover-dream/6-...md but the consumer-side work was lost in the parent ticket split: ENG-2064 (parent) was marked Duplicate after splitting into ENG-2069 (Consolidate) and ENG-2070 (Undo); the cross-cycle consumer fell through the cracks.
- Issue 3: The agent-side comment "this task is the only active one" was technically aspirational and the daemon pre-check looked complete on the path it was tested on (idle); the CLI path's missing pre-check wasn't covered.
- Issue 4: Pre-existing tests covered happy-path success; no regression test exercised the timeout/error path's log contents.

Test plan

Coverage added:
- Unit test
- Integration test
- Manual verification only
Test file(s):
- test/unit/infra/dream/dream-state-service.test.ts — concurrent-increment + update() race regressions
- test/unit/infra/dream/operations/consolidate.test.ts — pendingMerges consumption describe block (6 tests)
- test/unit/infra/executor/dream-executor.test.ts — partial-log preservation on budget abort + non-abort error + review entries from partial
- test/unit/infra/process/task-router.test.ts — eligible dispatch, ineligible short-circuit with skip reason, pre-check fail-open, no-op when absent
Key scenario(s) covered:
- Counter race: 10x and 20x interleaved increments via alternating DreamStateService instances + interleaved update() ↔ incrementCurationCount calls — both preserve all writes
- pendingMerges: sourceFile present-on-disk added to changedFiles; stale sourceFile silently skipped; pendingMerges cleared after the run regardless of LLM outcome; mergeTarget/reason surfaced in prompt; no-op when service absent or list empty
- Gate 3: CLI dream short-circuits with Dream skipped: Queue not empty (N tasks pending); --force bypasses; daemon pre-check errors fail open (agent gates 1+2 still apply)
- Partial-log: budget-abort and non-abort-error log paths both write the accumulated ops + computeSummary(...); review entries created for review-flagged ops that did complete on partial runs

User-visible changes

None for normal brv dream usage — CLI surface, flags, output format, and dream-log schema are unchanged. Behavior changes visible only in edge cases:

Auto-dream at agent-idle now triggers reliably under concurrent curates (previously could silently miss the activity threshold).
brv dream (no --force) prints Dream skipped: Queue not empty (N tasks pending) when the agent has queued work, instead of running on top of it.
brv dream --undo now works on partial dreams (previously a no-op when the dream timed out mid-pipeline).
dream-log/*.json for partial/error dreams now contains the operations that did complete (previously operations: []).

Evidence

Unit tests: All 308 dream-related unit tests pass post-rebase (dream-state-service, consolidate, synthesize, prune, dream-executor, dream-trigger, dream-undo, task-router).
Auto-test suite (local-auto-test/dreaming/run.sh): 24/24 scenarios passing on this branch. Notable:
- force-merge: produces 1 CONSOLIDATE/MERGE op as expected
- force-synthesize: produces 1 SYNTHESIZE/CREATE op (confidence 1.0, 3 sources from 3 fixture domains)
- force-archive: produces PRUNE/ARCHIVE op, file removed and stub created in _archived/
Interactive verification per issue (in squash commit message):
- Issue 1: 5 brv curate --detach + brv dream --force in parallel → all 5 increments accounted for in dream-state.json (4 consumed by the dream, 1 preserved post-step-7-reset)
- Issue 2: seeded a pendingMerge in dream-state.json, ran brv dream --force — first CONSOLIDATE op was the sourceFile → mergeTarget pair, pendingMerges cleared after the run
- Issue 3: 8 parallel curates (overflow the 5-slot agent pool so 3 queue), immediately ran brv dream → Dream skipped: Queue not empty (3 tasks pending). Once queue drained, the same brv dream completed normally
- Issue 4: 3 regression tests force budget abort + non-abort error, asserting partial log + review-entry creation

Checklist

Tests added or updated and passing (npm test)
Lint passes (npm run lint — 0 errors; 186 pre-existing warnings unrelated to this PR)
Type check passes (npm run typecheck)
Build succeeds (npm run build)
Commits follow Conventional Commits format
Documentation updated (N/A — no user-facing CLI/schema change)
No breaking changes
Branch is up to date with main — branch is rebased on the latest proj/dreaming (which is itself ahead of main via the dreaming feature work)

Risks and mitigations

Risk: preDispatchCheck hook fail-open posture for gate 3 (Issue 3). If the daemon's gate-3 check throws, the dream proceeds anyway.
- Mitigation: Fail-open is intentional — gate 3 is a UX nicety (yield to busy queues), not a safety gate. Gates 1 (time) and 2 (activity) remain re-checked at the agent as defense-in-depth, so the only consequence of a fail-open is that one dream may run alongside queued tasks. Lock-based safety (Gate 4) is unaffected.
Risk: Per-file AsyncMutex adds a small amount of serialization overhead to dream-state writes (Issue 1).
- Mitigation: The lock is held only for the duration of the read-modify-write window (microseconds in practice). All dream-state writes are already infrequent compared to other agent work. Module-level mutex registry is keyed by absolute file path so independent DreamStateService instances pointing at the same project share ordering without affecting other projects.
Risk: Partial-dream review entries may surface ops in brv review pending that the user didn't expect to see (Issue 4).
- Mitigation: This is the intended behavior per spec (notes/byterover-dream/6...md:181): "always write log and update state (even on partial/error)" implies review wiring is preserved. Without this, a partial dream's MERGE that wrote real files would never appear in review. Gated on allOperations being non-empty so early failures (zero ops completed) still produce no review entries — the "no log → no review entries" invariant is preserved.

Fixes four implementation gaps in proj/dreaming where shipped code violated the spec at notes/byterover-dream/. Filed together because they share the same surface and are all "spec says X, code does Y" bugs. Issue 1 — Counter race in incrementCurationCount With AGENT_MAX_CONCURRENT_TASKS=5, parallel curates raced on the read-modify-write of dream-state.json, dropping increments (empirically 7 parallel curates → counter at 3, 4 lost). Activity gate could stay locked silently, blocking auto-dream at agent-idle. Fix: serialize all writers via a generic update(updater) on DreamStateService, backed by a per-file AsyncMutex. incrementCurationCount, dream-executor's step-7 reset, and consolidate's pendingMerges clear all route through update() so no two writers can interleave their RMW windows. Issue 2 — pendingMerges never consumed by consolidate prune.ts wrote SUGGEST_MERGE entries to dreamState.pendingMerges for the next dream's Consolidate to pick up, but the consumer was never wired — entries sat forever, MERGE_INTO suggestions never landed. Fix: per notes/byterover-dream/6-dream-undo-and-cross-cycle.md:89-143, add loadAndClearPendingMerges() at the start of consolidate(): read pendingMerges, parallel-stat each sourceFile, add existing ones to changedFiles, surface mergeTarget+reason as non-binding LLM hints, clear pendingMerges unconditionally regardless of LLM outcome. Stale sourceFiles (deleted between dreams) are silently skipped. Issue 3 — Gate 3 (queue) bypassed on CLI dispatch path DreamTrigger was constructed with getQueueLength: () => 0 at the agent side. The idle-trigger path pre-checked gates 1-3 before dispatch, but the CLI path went straight to the agent with gate 3 effectively disabled, so brv dream (no --force) ran even when other tasks were queued. Fix: add a preDispatchCheck hook to TaskRouter; daemon wires the same getQueueLength resolver as the idle path; only dream tasks without --force are pre-checked. Pre-check errors fail open (agent gates 1+2 stay as defense-in-depth). Issue 4 — Partial dreams lost operations and broke undo allOperations was assembled only after all three ops succeeded; on timeout/error the catch block hardcoded operations:[] and zero summary. A consolidate that committed real file merges then a synthesize timeout would leave the context tree modified with no audit trail and no undo path (dream-undo reads operations from the log). Fix: hoist allOperations above the try block, push results incrementally after each step, use the accumulator in both partial and error log entries. Also run createReviewEntries for review-flagged ops that did complete on partial runs (gated on non-empty allOperations to preserve the "no log → no review entries" invariant on early failure). Tests - 28 new unit tests covering each fix's failure mode (counter race regression, pendingMerges round-trip, gate 3 short-circuit, partial-log preservation, review-entry creation from partial) - All 308 dream-related unit tests pass Verified interactively - Issue 1: 5 brv curate --detach + brv dream --force in parallel — all 5 increments accounted for in dream-state.json (4 consumed, 1 preserved) - Issue 2: seeded pendingMerge — first CONSOLIDATE op was the sourceFile→mergeTarget pair, pendingMerges cleared after the run - Issue 3: 8 parallel curates → CLI dream skipped with "Queue not empty (3 tasks pending)" - Issue 4: 3 regression tests force budget abort + non-abort error, asserting partial log + review entry creation

github-actions · 2026-04-16T10:35:15Z

Claude finished @RyanNg1403's task in 3m 58s —— View job

PR Review — fix: [ENG-2126] 4 dream spec-violation gaps

Gather context: read changed files
Review dream-state-service.ts (mutex / update() fix)
Review consolidate.ts (pendingMerges consumption)
Review dream-executor.ts (partial-log fix)
Review task-router.ts + transport-handlers.ts + brv-server.ts + agent-process.ts (queue gate fix)
Review test files
Post final review summary with inline comments

Overall Score: 4.5 / 5

Verdict: Approve with minor notes. All four fixes are correct, well-scoped, and well-tested. The surgical framing holds — no surrounding code was unnecessarily touched. A handful of minor issues found, none blocking.

Issue-by-Issue Assessment

Issue 1 — Counter race (`dream-state-service.ts`) ✅

The module-level mutex registry keyed on absolute path is the right abstraction. The generic update(updater) method cleanly composes with incrementCurationCount, and the AsyncMutex.withLock wrapper makes the RMW atomic. Two inline notes posted:

The stateMutexes map grows unboundedly (bounded by project count in practice — acceptable, but document for future auditors).
write() is public and bypasses the mutex; direct callers outside update() are unsafe. A JSDoc cross-reference to update() would help.

Issue 2 — `pendingMerges` never consumed (`consolidate.ts`) ✅

loadAndClearPendingMerges is cleanly isolated and correctly uses update() for the atomic clear. Two minor notes:

If the initial unguarded read() throws, the update() to clear pendingMerges is never called, leaving stale entries. Low-priority since both calls would likely fail together if the file is unreadable.
The double-access pattern (unguarded read() for hints + guarded update() for clear) is correct but should be commented to explain the deliberate split.

Issue 3 — Queue gate bypassed on CLI path (`task-router.ts` + `brv-server.ts`) ✅

The preDispatchCheck hook is cleanly injectable and task-type-agnostic. The makeDreamPreCheckTrigger factory with the intentionally throwing tryAcquire lock is an excellent guard against accidental lock acquisition at the daemon layer. One note:

When check.eligible === false, handleTaskCompleted is called internally, which fires lifecycle hooks' onTaskCompleted for the skipped task. Hooks that count completions would include skipped dreams. Low priority but worth documenting.

Issue 4 — Partial/error dreams lose operations audit trail (`dream-executor.ts`) ✅

The fix is minimal and precise: hoisting allOperations above the try block and threading it into the catch. The runOperations incremental-push contract is well documented and the test helper (makePartialRunExecutor subclass) correctly exercises the abort and non-abort error paths without a full LLM round-trip.

Test Coverage ✅

All four fix areas have dedicated test coverage:

Concurrent increment tests (10× and 20× parallel increments, interleaved with step-7-style resets) — correctly model the race.
pendingMerges describe block (6 tests) — covers present-on-disk, missing, cleared-after-run, LLM hints, and backward-compat cases.
Gate 3 (eligible dispatch, ineligible short-circuit, fail-open, no-op when absent) — matches the four scenarios from the PR.
Partial-log (budget-abort + non-abort-error + review-entry creation) — subclass injection avoids needing an LLM mock.

One test-design nitpick posted inline (asserting on write.calledOnce rather than the updated state itself).

No Issues Found In

The stateMutexes key normalization via resolve() handles relative paths correctly.
The getQueueLength closure in brv-server.ts captures the variable binding (not value), so agentPool is correctly resolved after assignment.
agent-process.ts getQueueLength: () => 0 is now accurately commented as intentional (daemon pre-checks gate 3).
The fail-open posture on pre-check errors is correctly scoped to gate 3 (a UX nicety), and gates 1 + 2 remain re-checked at the agent as defense-in-depth.
Atomic writes (tmp + rename) in dream-state-service.ts are preserved correctly.
runOperations incremental push correctly preserves partial work even if synthesize or prune throw after consolidate succeeds.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cec2dd944d

ℹ️ 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".

Addresses 10 review items from RyanNg1403 (Claude Code) and ChatGPT Codex on PR #434. Two real bugs (1× P1, 1× P2) found by Codex; the rest are documentation / test-assertion improvements from RyanNg. P1 (Codex) — task-router pre-dispatch skip polluted AgentPool counter preDispatchCheck's ineligible branch routed through handleTaskCompleted, which unconditionally calls agentPool.notifyTaskCompleted(projectPath). But the task was never submitted to the pool, so decrementing activeTasks under-represents real load and can let drainQueue dispatch an extra queued task beyond per-agent concurrency. Fix: extracted handleTaskSkippedByPreCheck — emits task:completed to client + project room and calls moveToCompleted, but skips notifyTaskCompleted AND lifecycle hooks (also resolves RyanNg #5 which flagged the lifecycle-hook side of the same call site). Three new regression tests assert the pool counter and onTaskCompleted hooks are not touched on skip, while the project broadcast still fires. P2 (Codex) — duplicate review entries on late dream failures dream-executor's success path runs createReviewEntries (step 6b), then dreamStateService.update (step 7). If step 7 throws, control jumps to catch which re-invokes createReviewEntries on the same allOperations, producing duplicate pending review entries. Fix: reviewEntriesWritten boolean set after the success-path call; catch-path call gated on !reviewEntriesWritten in addition to the existing allOperations.length > 0 invariant. One new regression test forces step 7 to throw after step 6b succeeds and asserts createReviewEntries is invoked exactly once. Doc / test improvements - dream-state-service.ts: comment on stateMutexes' monotonic growth bound (#1); JSDoc on write() cross-referencing update() to warn against bypassing the mutex (#2). - consolidate.ts: JSDoc on loadAndClearPendingMerges explaining the intentional two-phase access pattern (unguarded read for hints + mutex-guarded update for clear) and the fail-open behavior when the initial read throws (#3, #4). - consolidate.test.ts: assert update.calledOnce instead of write.calledOnce — surfaces intent (the contract) rather than the stub's implementation detail (#8). Verification - Lint: 0 errors (perfectionist/sort-classes flagged class-member order during audit; resolved by repositioning handleTaskSkippedByPreCheck alphabetically between handleTaskError and handleTaskStarted). - Typecheck: clean. - Unit tests: 6238 passing, 16 pre-existing pending, 0 failing. - /brv-code-quality skill: passed all 6 audit gates.

github-actions · 2026-04-16T11:05:17Z

Claude finished @RyanNg1403's task in 2m 7s —— View job

PR Review — fix: [ENG-2126] 4 dream spec-violation gaps (post-feedback update)

Gather context and understand the PR
Read changed implementation files
Read changed test files
Review Codex P1 fix: handleTaskSkippedByPreCheck isolation
Review Codex P2 fix: reviewEntriesWritten guard
Review previous Claude review feedback was addressed
Post final review summary

Overall Score: 5 / 5

Verdict: Approve. The follow-up commit (799f97a) correctly addresses all 10 review items from the first pass — including two real bugs (Codex P1 and P2), four documentation improvements, and a test assertion fix. All four original fixes are intact; the additions are surgical and well-tested. One very minor nit below, nothing blocking.

Second-Pass Assessment

Codex P1 — Pool counter corrupted on pre-dispatch skip ✅ Fixed

handleTaskSkippedByPreCheck (task-router.ts:673) is correctly isolated:

Does NOT call agentPool.notifyTaskCompleted — the pool's activeTasks counter is never touched for a task that never reached submitTask.
Does NOT fire onTaskCompleted lifecycle hooks — metrics/counters see only real completions.
Still emits task:completed to the client and the project room, and calls moveToCompleted.

The JSDoc at lines 658–672 is clear and accurately captures the invariant. Three regression tests cover all three behaviors (pool counter, hooks, broadcast).

Codex P2 — Duplicate review entries on late dream failures ✅ Fixed

The reviewEntriesWritten boolean (dream-executor.ts:113) is correctly placed above the try block and set to true at line 177 immediately after the success-path createReviewEntries call. The catch-path guard at line 231 (allOperations.length > 0 && !reviewEntriesWritten) prevents the second call when step 7 throws after step 6b succeeds. The regression test (dream-executor.test.ts:582) forces exactly this scenario and asserts createReviewEntries.callCount === 1.

Documentation improvements ✅

All four documentation items from the first-pass review are addressed:

dream-state-service.ts:18–21 — stateMutexes monotonic-growth note added.
dream-state-service.ts:88–95 — write() JSDoc cross-referencing update().
consolidate.ts:84–100 — loadAndClearPendingMerges JSDoc explaining the intentional two-phase access pattern and fail-open behavior.
consolidate.test.ts:453 — primary assertion changed to update.calledOnce (the contract) with a comment explaining why.

One Remaining Nit

handleTaskSkippedByPreCheck never calls hook.cleanup(taskId) (task-router.ts:673–694).

Every other terminal path — handleTaskCompleted, handleTaskError, handleTaskCancelled — calls notifyHooksCompleted/Error/Cancelled, which has a finally { hook.cleanup?.(taskId) } block. handleTaskSkippedByPreCheck skips hook notification entirely (which is correct for onTaskCompleted), but that means cleanup() is also never invoked for tasks that had onTaskCreate run.

Today's CurateLogHandler only allocates per-task state for curate tasks, so in practice this is a no-op for dream skips. But the invariant "every task that goes through runCreateHooks will eventually see cleanup()" is broken for skipped tasks, and a future hook author could unknowingly introduce a leak.

Suggested fix: add a bare cleanup sweep in handleTaskSkippedByPreCheck:

// Cleanup hooks (no onTaskCompleted fired — skip is not a real completion)
for (const hook of this.lifecycleHooks) {
  hook.cleanup?.(taskId)
}

This is low-priority since it requires a future hook to break, but it closes the gap cheaply. Fix this →