feat: [ENG-2070] Dream undo — revert last dream from previousTexts

[RyanNg1403]

Summary

• Problem: No way to revert dream operations if consolidation produces unwanted results
• Why it matters: Users need a safety net to trust automated consolidation — undo builds that trust
• What changed: Added brv dream --undo CLI flag and dream-undo.ts reversal logic
• What did NOT change (scope boundary): No daemon task type changes, no schema changes, no agent/LLM involvement

Type of change

• New feature

Scope (select all touched areas)

• Server / Daemon
• CLI Commands (oclif)

Linked issues

• Closes ENG-2070
• Related ENG-2060

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/dream/dream-undo.test.ts
• Key scenario(s) covered:
  • Precondition checks: no dream, log not found, already undone, invalid status
  • MERGE undo: restore from previousTexts, delete new output files
  • TEMPORAL_UPDATE undo: restore original content
  • CROSS_REFERENCE: skip (non-destructive)
  • SYNTHESIZE undo: delete created file (forward-compatible)
  • PRUNE/ARCHIVE: archive service restore (forward-compatible)
  • PRUNE/KEEP: no-op, PRUNE/SUGGEST_MERGE: remove from pendingMerges
  • Post-undo: log marked undone, state rewound, manifest rebuilt
  • Mixed operations reversed in order, partial failure resilience
  • Interactive: ran 10+ curate commands, dream with 3 MERGEs + 1 CROSS_REF, undo restored all 9 files (MD5 verified), double undo error, re-dream after undo, JSON output

User-visible changes

• New brv dream --undo flag reverts the last dream
• Text output: Undone dream drm-XXXX with file counts
• JSON output: { status: "undone", dreamId, restoredFiles, deletedFiles, restoredArchives, errors }
• Error messages: "No dream to undo", "Dream already undone", "Cannot undo dream with status: X"

Evidence

Interactive testing verified:

• 3 MERGE operations undone: 6 deleted files recreated, 3 output files restored to original content
• MD5 hash comparison confirmed byte-identical restoration
• State correctly rewound: lastDreamAt: null, totalDreams decremented
• Re-dream ran immediately after undo (proved lastDreamAt reset works)

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

Risks and mitigations

• Risk: Undo runs directly from CLI without daemon — bypasses daemon project resolution
  • Mitigation: Uses resolveProject() (same utility daemon uses) for project resolution. Pure file I/O with no agent/LLM needed.
• Risk: CROSS_REFERENCE frontmatter links not reverted on undo
  • Mitigation: By design — non-destructive changes. Documented in plan as accepted limitation.

[github-actions]

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

---

PR Review: feat: [ENG-2070] Dream undo

Tasks

• Gather context and read changed files
• Review src/oclif/commands/dream.ts
• Review src/server/infra/dream/dream-undo.ts
• Review test/unit/infra/dream/dream-undo.test.ts
• Post inline comments and overall summary

---

Overall Assessment — Score: 3 / 5

The feature is well-scoped, the happy-path logic is sound, and the test coverage hits the most important cases (precondition guards, each operation type, partial failures, state rewind). That said, there are a security issue and a data-loss case that need fixing before merge.

---

Critical

#	Location	Issue
1	dream-undo.ts:163–189	Path traversal — previousTexts keys are written via join(contextTreeDir, filePath) with no check that the resolved path stays inside contextTreeDir. The DreamLogStore itself calls this out as a concern ("security: prevents path traversal") for IDs but the same guard is absent here. Fix: validate fullPath.startsWith(contextTreeDir + '/') before mkdir/writeFile. Same applies to outputFile on line 174/206. Fix this →
2	dream-undo.ts:200–208	Data loss on SYNTHESIZE/UPDATE — the undo handler deletes the output file for both CREATE and UPDATE. For an UPDATE, the file existed before the dream and deleting it destroys the pre-existing content. Either add previousTexts to SynthesizeOperationSchema and restore it, or throw a clear error for UPDATE instead of silently deleting. Fix this →

Major

#	Location	Issue
3	dream-undo.ts:89	as number cast on log.completedAt violates CLAUDE.md ("Avoid as Type assertions"). After the throw on line 55, TypeScript narrows log to {status:'completed'|'partial', completedAt:number} — the cast and the 'completedAt' in log guard are both dead code. Replace with plain log.completedAt.
4	dream-undo.ts:108–113	lastDreamLogId is not cleared after undo, leaving state pointing to an undone dream. lastDreamAt: null and lastDreamLogId: null should be set together for semantic consistency.

Minor

#	Location	Issue
5	dream-undo.ts:83–85	Silent manifest rebuild failure — errors are completely invisible. Add to result.errors instead of swallowing.
6	dream-undo.ts:174, 206	.catch(() => {}) swallows all errors including permission failures; only ENOENT is expected — rethrow anything else.
7	dream.ts:136	resolveProject() ?? process.cwd() silently operates in a non-BRV directory. Fail fast with a clear message instead.
8	dream-undo.ts:127–143	No exhaustiveness check in undoOperation / undoConsolidate / undoPrune switch statements — unknown future operation types silently no-op.

Tests

#	Location	Issue
9	dream-undo.test.ts:266	No test for SYNTHESIZE/UPDATE — the most dangerous case goes uncovered.
10	dream-undo.test.ts:341	State rewind test doesn't assert lastDreamLogId is cleared.

---

What's good: The dependency-injection design on DreamUndoDeps is clean and makes the unit tests easy to write. Reversing operations in order is correct. The partial-failure resilience (continue on error, collect errors) is the right pattern. Test coverage for the happy paths and precondition guards is solid.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7a3e8f0fbc

ℹ️ 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 2m 22s —— View job

---

PR Review: feat: [ENG-2070] Dream undo (re-review after feedback)

Tasks

• Gather context
• Read src/oclif/commands/dream.ts
• Read src/server/infra/dream/dream-undo.ts
• Read test/unit/infra/dream/dream-undo.test.ts
• Verify FileContextTreeManifestService.buildManifest signature
• Assess which prior issues were addressed
• Post updated review

---

Re-review: feat: [ENG-2070] Dream undo — Score: 4 / 5

Good progress since the first pass. Both critical issues are resolved and most majors were cleaned up. Three items still need attention before merge.

---

Still Open

Bug (was Codex P2): buildManifest receives wrong path — manifest rebuilt from invalid location

dream-undo.ts:82

manifestService.buildManifest(contextTreeDir) passes the context-tree directory as the argument, but FileContextTreeManifestService.buildManifest treats its argument as the project base directory and internally appends /.brv/context-tree:

// file-context-tree-manifest-service.ts:58-59
const baseDir = directory ?? this.config.baseDirectory ?? process.cwd()
const contextTreeDir = join(baseDir, BRV_DIR, CONTEXT_TREE_DIR)

Passing <project>/.brv/context-tree as directory causes the service to scan <project>/.brv/context-tree/.brv/context-tree — a path that doesn't exist. The scan returns empty arrays, so the manifest is silently clobbered to empty after every undo. (No error is thrown because an empty directory scan is valid.)

Fix: call without arguments so the constructor's baseDirectory: projectRoot is used, or pass projectRoot explicitly.

// dream-undo.ts — add projectRoot to DreamUndoDeps
export type DreamUndoDeps = {
  // ...
  projectRoot: string
}

// then call:
await manifestService.buildManifest(projectRoot)

Fix this →

Major (unchanged): lastDreamLogId not cleared after undo

dream-undo.ts:108–113

After undo, dreamStateService.write sets lastDreamAt: null but leaves lastDreamLogId pointing at the now-undone dream. Any code that reads state.lastDreamLogId to find the "most recent dream" will get an ID for a dream with status: 'undone'.

await dreamStateService.write({
  ...state,
  lastDreamAt: null,
  lastDreamLogId: null,   // ← add this
  pendingMerges,
  totalDreams: Math.max(0, state.totalDreams - 1),
})

Fix this →

Test gap (follows from above): state rewind test doesn't assert lastDreamLogId cleared

dream-undo.test.ts:365–377

// Add this assertion to "rewinds dream state" test:
expect(writtenState.lastDreamLogId).to.be.null

---

Minor

#	Location	Issue
1	dream-undo.ts:123	as NodeJS.ErrnoException in unlinkSafe is the same as Type pattern that was correctly removed elsewhere. Use a type guard: const err = error as {code?: string}; if (err.code !== 'ENOENT') throw error — or the standard instanceof Error && 'code' in error pattern.
2	dream.ts:136	resolveProject()?.projectRoot ?? process.cwd() still silently falls back to cwd when not in a BRV project — command runs against arbitrary directory without warning. Consider logging a warning at minimum.

---

Fixed Since Last Review

#	Issue	Status
C1	Path traversal via previousTexts keys	✅ Fixed — safePath() uses resolve() + startsWith check
C2	SYNTHESIZE/UPDATE deletes pre-existing file	✅ Fixed — throws Cannot undo SYNTHESIZE/UPDATE
M1	as number cast on log.completedAt	✅ Fixed — plain log.completedAt
M2	Silent manifest rebuild failure	✅ Fixed — adds to result.errors
m1	.catch(() => {}) swallows non-ENOENT errors	✅ Fixed — unlinkSafe() helper rethrows non-ENOENT
T1	No test for SYNTHESIZE/UPDATE	✅ Fixed — test at line 268

The safePath implementation is correct: resolve() is the right function (not join) because it handles .. segments in absolute path context; the startsWith guard covers both the path-traversal case and absolute-path injection.

---

[danhdoan]

lgtm