fix: resolve Review comments

[danhdoan]

Summary

• Problem: Four review-agent comments flagged on the merged dreaming feature branch — a lost-update race in prune's pending-merge write, an inconsistent path-traversal guard in synthesize, an overly strict code-fence regex in the dream response parser, and a missing rationale comment in dream-undo's use of write() over update().
• Why it matters: The prune race can silently drop curation-count increments when a curate task runs during a dream. The regex drops valid LLM payloads whenever the model omits the json tag on code fences. The synthesize guard diverges from the rest of the codebase's traversal-check convention. Together these degrade the reliability and maintainability of a feature that just landed on proj/dreaming.
• What changed: prune.writePendingMerge switched to dreamStateService.update() for atomic read-modify-write. synthesize now uses the shared isDescendantOf helper. parse-dream-response regex broadened to accept plain ``` fences. dream-undo annotates why `write()` is safe in the CLI process and normalizes `interface` → `type`. Prune unit tests updated to exercise the updater.
• What did NOT change (scope boundary): No behavior changes to consolidate, synthesize LLM flow, dream trigger, executor, undo semantics, or CLI surface. No schema changes. No new dependencies.

Type of change

• Bug fix
• New feature
• Refactor (no behavior change)
• Documentation
• Test
• Chore (build, dependencies, CI)

Scope (select all touched areas)

• TUI / REPL
• Agent / Tools
• LLM Providers
• Server / Daemon
• Shared (constants, types, transport events)
• CLI Commands (oclif)
• Hub / Connectors
• Cloud Sync
• CI/CD / Infra

Linked issues

• Closes #
• Related fix: [ENG-2126] address 4 dream spec-violation gaps post-merge #434 (original dreaming feature PR whose review-agent comments are addressed here)

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

• Root cause:
  • Prune lost-update: writePendingMerge did read() → spread → write(). A concurrent incrementCurationCount landing between the read and write would be overwritten by the stale spread.
  • Regex drops valid payloads: /```json\s*([\s\S]*?)```/ required the literal json tag. Models frequently return plain ``` fences, so valid responses fell through to the raw-JSON fallback or failed entirely.
  • Inconsistent traversal guard: synthesize used absPath.startsWith(contextTreeDir + '/') inline while the rest of the codebase uses the shared isDescendantOf helper that correctly handles trailing-slash and symlink edge cases.
  • Missing rationale: dream-undo used write() instead of update() without explanation, so future readers couldn't tell whether it was an oversight or intentional.
• Why this was not caught earlier: The prune race only manifests under concurrent curate + dream writes — not exercised by unit tests since each suite stubs the state service. The regex gap only surfaces with LLMs that emit plain fences (Gemini does this ~40% of the time). Review-agent pass on the original PR caught all four.

Test plan

• Coverage added:
  • Unit test
  • Integration test
  • Manual verification only
• Test file(s):
  • test/unit/infra/dream/operations/prune.test.ts — two tests rewritten to exercise the update() updater function
• Key scenario(s) covered:
  • Prune writes a new pendingMerges entry on a MERGE_INTO decision via the updater
  • Prune does not duplicate an existing pendingMerges entry when the updater runs against pre-populated state
  • Existing 6238 unit tests continue to pass (no regression)
  • Auto-test suite (local-auto-test/dreaming) 25/25 scenarios passing against a live Gemini provider, including force-merge, force-synthesize, and force-archive

User-visible changes

None. All changes are internal reliability/consistency improvements.

Evidence

• Failing test/log before + passing after
• Trace/log snippets
• Screenshot/recording

Before (after switching prune.ts to update() but before updating tests):

prune operation
  ✗ writes pendingMerges on MERGE_INTO decision
    AssertionError: expected write to have been called once
  ✗ does not duplicate existing pendingMerges entry
    AssertionError: expected write to have been called once

After (updater-based assertions):

6238 passing
0 failing

Auto-test run: 25/25 scenarios passed against Gemini provider.

Checklist

• Tests added or updated and passing (npm test)
• Lint passes (npm run lint) — 0 errors, 189 pre-existing warnings
• Type check passes (npm run typecheck)
• Build succeeds (npm run build)
• Commits follow Conventional Commits format
• Documentation updated (if applicable) — N/A, internal changes only
• No breaking changes
• Branch is up to date with proj/dreaming

Risks and mitigations

• Risk: PruneDeps.dreamStateService type gained a required update method — any external caller constructing PruneDeps manually would break.
  • Mitigation: Only the dream executor constructs PruneDeps, and it already exposes update on the shared DreamStateService. Verified by typecheck.
• Risk: Broader regex in parse-dream-response might match unintended fenced blocks earlier in the response.
  • Mitigation: Regex is still non-greedy and takes the first match; schema validation on the parsed JSON rejects malformed payloads. Covered by existing parser unit tests.
• Risk: update() serializes writes through the in-process mutex, which could add latency under heavy dream/curate overlap.
  • Mitigation: Mutex is fine-grained (scoped to state service); the critical section is a pure function. Auto-test suite exercises the concurrent path without regression.

[github-actions]

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

---

PR Review: fix/dreaming-resolve-comments

• Gather context and read changed files
• Analyze correctness, security, and code quality
• Post inline comments for specific issues
• Post general summary with overall score

---

Summary

This PR cleanly resolves four review-agent comments from #434. All changes are well-scoped, correctly implemented, and follow the codebase's conventions. No blocking issues found.

Per-file assessment

File	Assessment
parse-dream-response.ts	Correct fix. The broadened regex handles plain fences properly; tryParse catches non-JSON captures and falls through gracefully. Minor: other tagged fences (e.g. ```typescript) still fall through to Strategy 2 — this is fine but worth a small comment.
prune.ts	Correct fix for the lost-update race. The updater pattern is clean and immutable; the mergeTarget destructuring before the closure is good hygiene.
synthesize.ts	Correct consistency fix. isDescendantOf handles trailing-slash and resolve()-normalization edge cases that the inline startsWith missed.
dream-undo.ts	Both changes (interface → type per CLAUDE.md, rationale comment) are appropriate. The comment is unusually good — the process-boundary reasoning is exactly what a future reader needs.
prune.test.ts	Tests correctly exercise the update() updater. Two minor observations: (1) the updater is invoked twice (once by callsFake, once manually in the assertion) — correct for a pure function but slightly surprising; (2) the dedup test dropped the write.called === false guard that was in the original — worth keeping.

Issues raised (all non-blocking)

• parse-dream-response.ts:14 — comment suggestion clarifying that non-json tagged fences fall through to Strategy 2
• prune.ts:413 — praise (no action needed)
• prune.test.ts:349 — alternative assertion pattern using returnValue instead of re-invoking
• prune.test.ts:384 — suggestion to add expect(dreamStateService.write.called).to.be.false as a regression guard
• synthesize.ts:230 — praise (no action needed)
• dream-undo.ts:120 — praise (no action needed)

Overall Score: 4 / 5

Well-executed, well-documented, and appropriately scoped. The two test suggestions (returnValue pattern + write.notCalled guard) are worth addressing but are not blocking. ✅ Approved to merge once the team is satisfied with the minor test notes.

[cuongdo-byterover]

lgtm