feat: [ENG-2121] improve dream task TUI display with meaningful content

[RyanNg1403]

Summary

• Dream executor now returns a formatted summary (counts of consolidated/synthesized/pruned operations) instead of a raw log ID
• CLI dream command sets meaningful content field ("Memory consolidation") so TUI input area isn't blank
• Daemon idle-trigger dream tasks also get meaningful content

Test plan

• brv dream --force shows formatted result in TUI expanded view
• TUI input area displays "Memory consolidation (force)" instead of blank
• Zero-ops dream shows "No changes needed" message
• Existing dream-executor unit tests updated and passing
• Full test suite passing (6164 tests)
• Code quality audit passed

[github-actions]

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

---

PR Review: feat: [ENG-2121] improve dream task TUI display with meaningful content

Tasks

• Gather context and read changed files
• Review src/oclif/commands/dream.ts
• Review src/server/infra/executor/dream-executor.ts
• Review src/server/infra/daemon/brv-server.ts
• Review test/unit/infra/executor/dream-executor.test.ts
• Post inline comments on specific issues
• Post overall summary with score

---

Summary

Score: 3 / 5

The PR cleanly achieves its stated goal — replacing blank content fields and raw log IDs with meaningful user-facing strings. The formatResult method is straightforward, the brv-server.ts change is safe, and the CLI path simplification is correct. Three issues need attention before merge:

---

Issues Found

1. Breaking JSON API change — src/oclif/commands/dream.ts line 212

The JSON output field was renamed from logId to result. logId was a stable identifier (e.g. drm-1234); result is now a human-readable multiline string. Any consumer that reads data.logId from brv dream --format json will silently get undefined. This should either preserve logId for backwards compatibility, or be explicitly called out as a breaking change in the PR description and changelog.

2. errors field silently dropped from output — src/server/infra/executor/dream-executor.ts lines 340–355

DreamLogSummary has an errors field, but formatResult never includes it in the output. If any operations failed, the user sees either a counts line (without the error count) or "No changes needed — context tree is up to date" — the latter being actively misleading when errors occurred. The condition for "No changes needed" should gate on errors === 0 as well.

3. Insufficient test coverage for formatResult — test/unit/infra/executor/dream-executor.test.ts line 65–70

The only updated test covers the zero-ops path. The new method has distinct branches for when operations exist, for flaggedForReview > 0, and (after fixing issue #2) for errors > 0. Per repo standards (80% minimum, critical paths must be covered), tests for the non-zero-count paths are required.

---

Minor Observations

• brv-server.ts: Clean, no issues. The idle-trigger label "Memory consolidation (idle trigger)" is a good distinguisher.
• skipped detection (result?.startsWith('Dream skipped:')): Pre-existing string-prefix heuristic; the PR doesn't make it worse, but it's fragile — worth a follow-up issue.
• The multiline format from formatResult (parts joined with \n) works correctly for this.log() output; the TUI content field is set separately so there's no rendering concern there.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 34f8496804

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

[github-actions]

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

---

Tasks

• Gather context and read changed files
• Review src/oclif/commands/dream.ts
• Review src/server/infra/executor/dream-executor.ts
• Review src/server/infra/daemon/brv-server.ts
• Review test/unit/infra/executor/dream-executor.test.ts
• Post overall summary with score

---

PR Review: feat: [ENG-2121] improve dream task TUI display with meaningful content

Score: 4 / 5

The fix commit addresses all three issues from the previous review: errors is now surfaced in formatResult, the "No changes needed" branch is correctly gated on errors === 0, and test coverage for the new method is solid. One issue remains.

---

Remaining Issue

logId in JSON output is still undefined for dream tasks — src/oclif/commands/dream.ts line 212

The fix adds {logId, result, ...} to restore backwards compat, but logId in the onCompleted payload comes from the transport layer's lifecycle hooks (task-router.ts:301). The only registered lifecycle hook is curateLogHandler, and it explicitly skips non-curate task types (CURATE_TASK_TYPES = ['curate', 'curate-folder'] — curate-log-handler.ts:188). For dream tasks, task.logId is always undefined, so payload.logId is always undefined.

The result:

• Before this PR: data.logId = "drm-1234" (the raw dream log ID, because result from the executor was just the log ID string)
• After this PR: data.logId = undefined, data.result = "Dream completed (drm-1234)\n..." (log ID now only accessible by parsing a human-readable string)

Consumers that depended on data.logId still get undefined. The dream log ID can only be recovered by string-parsing data.result, which is fragile.

The minimal fix is to extract the dream log ID in onCompleted and pass it explicitly:

// In onCompleted, result now looks like "Dream completed (drm-1234)\nNo changes needed..."
const dreamLogId = result?.match(/^Dream completed \(([^)]+)\)/)?.[1]
writeJsonResponse({
  command: 'dream',
  data: skipped
    ? {reason: result, status: 'skipped', taskId: tid}
    : {logId: dreamLogId, result, status: 'completed', taskId: tid},
  success: true,
})

A cleaner but larger fix would be to change formatResult to return {logId: string; formatted: string} and thread logId through the transport payload, but that's a bigger scope change.

Fix this →

---

What Was Fixed (Good Work)

• errors surfaced: formatResult now appends "X operations failed" and the "No changes needed" branch correctly gates on errors === 0. The edge case where errors > 0 but counts are zero no longer emits the misleading message.
• Test coverage: Four test cases now cover all distinct branches of formatResult — zero-ops, non-zero counts, flaggedForReview, and errors. The use of as unknown as to access the private method is a pragmatic tradeoff given the method can't be easily reached through the public interface without complex mocking.
• brv-server.ts: Clean. The idle-trigger label "Memory consolidation (idle trigger)" correctly distinguishes background dreams from CLI-triggered ones.

---

Minor Observations

• flaggedForReview + zero counts: When consolidated/synthesized/pruned are all 0 but flaggedForReview > 0, the output is "No changes needed — context tree is up to date\n2 operations flagged for review". The two lines contradict each other semantically. The condition for "No changes needed" could additionally check summary.flaggedForReview === 0, but this is a judgment call on intended UX.
• skipped detection (result?.startsWith('Dream skipped:')): Pre-existing fragile string-prefix heuristic — not introduced by this PR.

[github-actions]

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

---

Tasks

• Gather context and read changed files
• Review src/oclif/commands/dream.ts
• Review src/server/infra/executor/dream-executor.ts
• Review src/server/infra/daemon/brv-server.ts
• Review test/unit/infra/executor/dream-executor.test.ts
• Post overall summary with score

---

PR Review: feat: [ENG-2121] improve dream task TUI display with meaningful content

Score: 4.5 / 5

All three issues from previous reviews are resolved. The logId extraction approach is a pragmatic tradeoff with an acknowledged comment; two nits remain.

---

What Was Fixed (Good Work)

• logId backwards compat: dreamLogId is extracted from the formatted result string via regex, and both logId and result appear in the JSON payload. The comment correctly explains why transport logId is always undefined for dream tasks.
• Non-JSON path simplified: The duplicate skipped/non-skipped branches are correctly collapsed into a single this.log(result ?? '') since formatResult now handles the "Dream completed" header uniformly.
• errors surfaced: "No changes needed" is correctly gated on errors === 0 && flaggedForReview === 0. Errors get their own output line.
• Test coverage: Five cases now cover all branches of formatResult — zero-ops, counts present, flaggedForReview, flaggedForReview-only (no "No changes needed"), and errors.

---

Remaining Issues

nit — Grammar: "1 operations" — src/server/infra/executor/dream-executor.ts lines 352, 356

When errors === 1 or flaggedForReview === 1, the output reads:

• "1 operations failed" (should be "1 operation failed")
• "1 operations flagged for review" (should be "1 operation flagged for review")

// suggestion
parts.push(`${summary.errors} ${summary.errors === 1 ? 'operation' : 'operations'} failed`)
// ...
parts.push(`${summary.flaggedForReview} ${summary.flaggedForReview === 1 ? 'operation' : 'operations'} flagged for review`)

The test on line 98 asserts the current grammatically-incorrect string, so it would need updating too. Fix this →

nit — Fragile coupling between dream.ts regex and formatResult header — src/oclif/commands/dream.ts line 209

The regex /^Dream completed \(([^)]+)\)/ hard-codes the exact header format produced by formatResult. If the wording changes (rewording, i18n, etc.), dreamLogId silently becomes undefined with no compiler or test catching it. The comment is a good mitigant, but a cleaner long-term fix would be to return a structured object from formatResult:

private formatResult(logId: string, summary: DreamLogSummary): {logId: string; formatted: string} {
  // ...
  return {logId, formatted: parts.join('\n')}
}

This is a scope increase, so treating it as a follow-up is reasonable. Leaving the comment in place is the right call for now.

---

Minor Observations

• [^)]+ regex safety: The DreamLogEntry.id schema constrains IDs to /^drm-\d+$/, so the regex can never mis-capture. Safe as written.
• skipped detection (result?.startsWith('Dream skipped:')): Pre-existing fragile heuristic — not introduced by this PR.
• brv-server.ts: Clean. "Memory consolidation (idle trigger)" correctly distinguishes background from CLI-triggered dreams.

---

[github-actions]

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

---

Tasks

• Gather context and read changed files
• Review src/oclif/commands/dream.ts
• Review src/server/infra/executor/dream-executor.ts
• Review src/server/infra/daemon/agent-process.ts + brv-server.ts
• Review src/server/infra/process/task-router.ts
• Review test/unit/infra/executor/dream-executor.test.ts
• Post overall summary with score

---

PR Review: feat: [ENG-2121] improve dream task TUI display with meaningful content

Score: 4.5 / 5

The second nit from the previous review — fragile coupling between the dream.ts regex and formatResult's header format — is fully resolved. The logId is now properly threaded through the transport as a structured value (agent-process.ts → task-router.ts → dream.ts onCompleted), so dream.ts receives it directly from the event payload with no string parsing required. This is the right fix.

The architecture across all six files is clean and consistent. One nit from the prior review remains.

---

What Was Fixed (Good Work)

• logId transport threading: agent-process.ts now destructures {logId, result} from dreamExecutor.executeWithAgent() and passes logId in the COMPLETED transport event. task-router.ts resolves it via task?.logId ?? eventLogId — correctly preferring the lifecycle-hook logId for curate tasks and falling back to the executor-provided logId for dream tasks. dream.ts onCompleted receives it cleanly from the event. No more regex, no more fragile coupling. ✅
• formatResult errors + gating: errors is surfaced; "No changes needed" is correctly gated on errors === 0 && flaggedForReview === 0. ✅
• Test coverage: Five distinct branches tested — zero-ops, counts present, flaggedForReview with counts, flaggedForReview-only (no "No changes needed"), and errors. The as unknown as cast to access the private method is a reasonable pragmatic tradeoff. ✅
• brv-server.ts: "Memory consolidation (idle trigger)" correctly distinguishes background from CLI-triggered dreams. ✅

---

Remaining Issue

nit — Grammar: "1 operations" — src/server/infra/executor/dream-executor.ts lines 352, 356

This was called out in the prior review and is still present. When errors === 1 or flaggedForReview === 1, the output reads:

• "1 operations failed" (should be "1 operation failed")
• "1 operations flagged for review" (should be "1 operation flagged for review")

The test at line 92 also asserts the incorrect string ('1 operations flagged for review'), so both need updating together.

// suggestion — dream-executor.ts lines 351–357
if (summary.errors > 0) {
  parts.push(`${summary.errors} ${summary.errors === 1 ? 'operation' : 'operations'} failed`)
}

if (summary.flaggedForReview > 0) {
  parts.push(`${summary.flaggedForReview} ${summary.flaggedForReview === 1 ? 'operation' : 'operations'} flagged for review`)
}

And update the test assertion on line 92:

expect(result).to.include('1 operation flagged for review')  // was: '1 operations flagged for review'

Fix this →

---

Minor Observations

• skipped detection (result?.startsWith('Dream skipped:')): Pre-existing fragile string-prefix heuristic — not introduced by this PR.
• [^)]+ regex safety (in task-router.ts context): No longer relevant — the regex was removed entirely in this revision. ✅
• result! implicit non-null in agent-process.ts: let result: string may be flagged by the compiler as potentially unassigned after the switch if TypeScript's control-flow analysis can't see that all reachable paths assign it (e.g., if type is an unexpected value). Pre-existing pattern — not introduced here.