feat(memory): eager working-memory refresh via Stop-hook claude -p worker

[dean0x]

Summary

• Moves working-memory refresh out of the SessionStart Dream subagent and into a detached claude -p --model haiku worker spawned directly from the dream-capture Stop hook, giving eager refresh at the end of every session rather than deferring to the next SessionStart Dream cycle
• Redesigns session-start-memory with 3-state git-reconciled injection: A (in-sync), B (drifted N commits with reconcile banner), C (refresh failing), plus a branch-mismatch warning when the stamp branch differs from the current branch
• Removes the dream-memory skill entirely — memory is no longer a Dream subagent task

Changes

New file:

• scripts/hooks/background-memory-update — detached worker: 300s stale-break lock, drains .pending-turns.jsonl, invokes claude -p --model haiku via stdin (never argv), writes WORKING-MEMORY.md with a <!-- memory-head: <sha> branch: <name> --> stamp on line 1, 120s watchdog, DEVFLOW_BG_UPDATER=1 recursion guard

Modified hooks:

• dream-capture — replaced memory marker-write with worker spawn via nohup+disown; throttled by .working-memory-last-trigger
• session-start-context — removed raw-queue bridge (Step 3) that added memory to _DREAM_TASKS
• dream-collect-tasks — memory) case is now unconditional sweep (stale marker cleanup, same treatment as learning.*); removed _dbsd_mem from dream_build_spawn_directive
• session-start-memory — complete rewrite: parses git stamp from line 1, computes drift, emits 3-state header, no raw-turns dump

Removals / migrations:

• shared/skills/dream-memory/SKILL.md deleted
• shared/agents/dream.md — memory removed from task list and skill refs
• src/cli/plugins.ts — dream-memory removed from devflow-core-skills skills array; devflow:dream-memory added to LEGACY_SKILL_NAMES
• src/cli/utils/migrations.ts — purge-stale-memory-markers-v1 per-project migration removes legacy memory.* Dream markers

Docs:

• CLAUDE.md — Working Memory paragraph, hook list, runtime file inventory updated

Reviewer Focus Areas

• scripts/hooks/background-memory-update: lock acquisition (300s stale-break), mv-claim atomicity, watchdog cancel after wait, .last-refresh-ok only touched on success, recursion guard
• scripts/hooks/session-start-memory: git stamp parsing, 3-state logic, branch-mismatch banner path
• scripts/hooks/dream-capture: throttle file touch BEFORE spawn (prevent double-spawn race), nohup/disown detach
• Security: prompt content via stdin only, never argv; DEVFLOW_BG_UPDATER=1 recursion guard

[dean0x]

🔴 BLOCKING: background-memory-update is listed in LEGACY_HOOK_FILES, so devflow init deletes it immediately after copying.

This PR adds scripts/hooks/background-memory-update (the detached worker that refreshes WORKING-MEMORY.md), but also adds 'background-memory-update' to the LEGACY_HOOK_FILES array in src/cli/commands/init.ts:1079. The cleanup loop at init.ts:1086 runs AFTER installViaFileCopy and unconditionally deletes all entries in that array. Result: the freshly-copied worker is removed, and at runtime dream-capture hits its guard and memory never refreshes.

Fix: Remove 'background-memory-update' from LEGACY_HOOK_FILES. It is a current, required runtime script, not legacy.

Suggested test: assert that ~/.devflow/scripts/hooks/background-memory-update exists and is executable after devflow init.

---

Claude Code | Regression Reviewer

[dean0x]

HIGH (82%) — Code injection via project path in node-fallback orphan check.

${QUEUE_FILE} is string-interpolated directly into the node -e source. A project path containing a single quote (e.g., /Users/x/dean's-repo) breaks out of the JS string literal. A crafted path like '+process.mainModule.require('child_process').execSync('…')+' executes arbitrary code.

Fix: Pass the path via process.argv[1] instead:

_HAS_ASSISTANT=$(node -e '
  const lines = require("fs").readFileSync(process.argv[1],"utf8").split("\n");
  for (const l of lines) {
    try { const o = JSON.parse(l); if (o.role==="assistant") { process.stdout.write("assistant"); break; } } catch {}
  }
' "$QUEUE_FILE" 2>/dev/null || echo "")

This matches the pattern already used at line 198 and the safe jq branch.

---

Claude Code | Security Reviewer

[dean0x]

MEDIUM (80%) — Untrusted memory-file content flows into git revision args without validation.

STAMP_SHA is parsed from line 1 of WORKING-MEMORY.md (LLM-authored, user-writable) and used as a git revision:

git merge-base --is-ancestor "$STAMP_SHA" "$HEAD"
git rev-list --count "${STAMP_SHA}..${HEAD}"
git log --oneline "${STAMP_SHA}..${HEAD}"

A stamp value beginning with - (e.g., --output=/path) could be interpreted as a git option. While no RCE is possible, git argument injection is a defense-in-depth concern for untrusted input.

Fix: Validate the SHA shape before use:

case "$STAMP_SHA" in
  *[!0-9a-fA-F]* | "" ) STAMP_SHA="" ;;   # only hex; clears non-SHA values
esac

---

Claude Code | Security Reviewer

[dean0x]

MEDIUM (92%) — Dead parameter mem_enabled threaded through two hooks.

dream_collect_tasks accepts mem_enabled as parameter 2 but never uses it (line 53). The memory) case now sweeps unconditionally. The sole caller, session-start-context, still computes _SC2_MEM_EN via a json_field_file subprocess read (line 126) purely to pass it as the now-ignored argument (line 143). This is leftover plumbing from the removed marker path.

Fix: Drop the parameter from the function signature and call site, or if positional stability is desired, delete the pointless _SC2_MEN_EN config read and pass a literal placeholder. The dead json_field_file read costs a subprocess every SessionStart.

---

Claude Code | Complexity Reviewer

[dean0x]

MEDIUM (82%) — Watchdog kills only the direct PID with SIGTERM; hung claude -p children orphan, and SIGTERM-ignoring processes never escalate to SIGKILL.

claude is launched as a plain background job (... &, line 328), so CLAUDE_PID is the single process, not a process group. The watchdog ( sleep 120 && kill "$CLAUDE_PID" ) sends only SIGTERM to that one PID. If claude -p spawns helpers (MCP servers, node children), they become orphans. A process ignoring SIGTERM is never escalated, so a truly wedged claude survives the 120s bound.

Fix: Kill the process group and escalate:

( sleep 120 && kill -TERM -$PGID 2>/dev/null; sleep 5 && kill -KILL -$PGID 2>/dev/null ) &

Or at minimum:

( sleep 120 && kill "$CLAUDE_PID" 2>/dev/null; sleep 5 && kill -9 "$CLAUDE_PID" 2>/dev/null ) &

---

Claude Code | Reliability Reviewer

[dean0x]

MEDIUM (85% consistency, 92% documentation) — docs/working-memory.md:59 lists ## Modified Files section, but the worker prompt does not produce it.

This PR re-attributed the Working Memory Sections table from "the Dream agent" to "the background-memory-update worker" (line 52 is a touched line). However, the section list includes ## Modified Files, which does NOT appear in the worker's actual prompt at scripts/hooks/background-memory-update:311. The worker requires exactly five sections: ## Now, ## Progress, ## Decisions, ## Context, ## Session Log.

CLAUDE.md:47 was correctly updated to the five-section list, so working-memory.md is now the outlier. This is the exact PF-009 failure mode: a rename/removal leaving stale references in a doc touched in the same PR.

Fix: Delete the ## Modified Files row from the table:

| `## Decisions` | Architectural and design decisions made this session |
| `## Context` | Repository state, build status, test results |
| `## Session Log` | Timestamped log of significant actions |

---

Claude Code | Consistency Reviewer

[dean0x]

Summary: Code Review Findings

PR: #239 | Branch: feat/eager-memory-refresh -> main
Review Date: 2026-06-07_1309

Inline Comments Posted (≥80% confidence)

Reviewer	Finding	Severity	File:Line
Regression	LEGACY_HOOK_FILES deletes worker post-install	CRITICAL	init.ts:1079
Security	Command injection via unvalidated path → node -e	HIGH	background-memory-update:138
Security	Git arg injection from untrusted STAMP_SHA	MEDIUM	session-start-memory:104,157
Complexity	Dead mem_enabled parameter threaded through hooks	MEDIUM	dream-collect-tasks:53
Reliability	Watchdog SIGTERM-only; no escalation to SIGKILL	MEDIUM	background-memory-update:332
Consistency/Docs	Stale ## Modified Files section in table	MEDIUM	working-memory.md:59

Lower-Confidence Suggestions (60-79%)

The following ≥60% suggestions are consolidated here for awareness but do not block merge:

• Duplication in orphan/JSON extraction (complexity.md, 85%) — The jq/node fallback pattern is repeated 3x in the worker. Extract to a shared helper.
• Logging helper re-implementation (consistency.md, 82%) — Re-implements log() + size guard instead of sourcing hook-log-init. Source the shared helper; if the 5MB threshold diverges intentionally, parameterize it.
• Log path slug computation inline (consistency.md, 80%) — session-start-memory:138 recomputes the log-path slug instead of using devflow_log_dir (already in scope).
• Bare magic numbers (complexity.md, 75%) — The 600s refresh-stale window and 9999999 never-sentinel should be named constants; same for 120s watchdog.
• LEGACY_HOOK_FILES comment misgroups the new worker (regression.md, 70%) — Once the CRITICAL is fixed by removing the line, the "Pre-sidecar hooks" comment is misleading for future readers.
• No install-survival test for runtime hooks (regression.md, 72%) — Broader test gap: no test asserts that required hooks exist after devflow init.

Merge Recommendation

CHANGES REQUESTED

The 1 CRITICAL blocker (worker deletion) must be fixed before merge. Once 'background-memory-update' is removed from LEGACY_HOOK_FILES and backed by an install-survival test, the PR is releasable. The 5 MEDIUM should-fix items harden the code (security hardening, watchdog escalation, dead parameter cleanup, doc drift, logging duplication) and should ideally ship in this PR, but none are merge-blocking; they can follow in v2 if time-critical. See individual inline comments for detailed fixes.

---

Claude Code | Synthesizer

[dean0x]

Cycle 2 Code Review Summary

Status: ✅ All high-confidence in-diff findings already posted as inline comments (cycle 1).

Inline Comments (≥80% confidence, anchored to changed lines)

The following findings have inline comments at their source locations:

1. CLAUDE.md:189 — Model Strategy still lists "haiku for memory" (HIGH, 95%)
2. tests/eager-memory-refresh.test.ts:580 — Lock test vacuous when timeout absent (HIGH, 90%)
3. scripts/hooks/session-start-memory:129 — Redundant double range-walk (MEDIUM, 88%)
4. scripts/hooks/background-memory-update:89 — Lock-acquire timeout lacks rationale (MEDIUM, 82%)
5. scripts/hooks/session-start-memory:57 — 144-line mega-block (MEDIUM, 85%)
6. scripts/hooks/background-memory-update:208 — Turn-builder loop lacks invariant comment (MEDIUM, 80%)

---

Pre-existing Issues (Not Blocking)

Reliability & Architecture Carry-overs (No Fix Applied in Cycle 1)

Two MEDIUM findings from cycle 1 remain unaddressed and are still present in the current code:

Finding	Confidence	Impact	Fix Recommendation
break_stale_lock non-atomic check-then-rmdir (scripts/hooks/background-memory-update:76-86)	80%	Lock-break does not verify owner is dead; relies on implicit invariant STALE_THRESHOLD (300s) > watchdog total in-flight time (~125s). If watchdog timeout increases past 300s, two workers could write concurrently (outcome: redundant idempotent write, not corruption, but mutual exclusion is undocumented).	Stamp lock with owner PID at acquire and only break when stale AND ! kill -0 <pid> (owner dead). Or add assertion-comment documenting the invariant.
dream-recover moves .pending-turns.processing without lock (scripts/hooks/dream-recover:148-170 vs. background-memory-update:141-156)	80%	Recovery fires at SessionStart without acquiring .working-memory.lock, racing the worker's leftover-merge. Narrow impact (the recovery only fires when .processing is >300s old AND no .jsonl exists, by which time any live worker has been watchdog-killed). Still carries the same implicit invariant as above.	Add inline comment in dream-recover documenting that lock-free recovery is safe-by-construction because 300s > watchdog total (~125s).

Both carry the same root cause: safety depends on an implicit invariant that should be documented (or enforced with PID checks).

Pre-existing Documentation Drift

• hooks/KNOWLEDGE.md:241 (Confidence: 90%) — Stale dream-fallback claim asserts a transitional fallback to .devflow/sidecar/config.json that has been removed from the hooks source. This is a PF-009 instance (rename/removal leaves stragglers in lower-visibility surfaces). Suitable for the next knowledge base refresh.

Pre-existing TypeScript Issue (Harmless)

• src/cli/plugins.ts:531-533 (Confidence: 82%) — LEGACY_SKILLS_V2X lists still-active bare skill names (dream-decisions, dream-knowledge, dream-curation) alongside genuinely-removed ones. This is harmless today because active skills install under namespaced paths, so the post-install cleanup targets non-existent bare-name directories. However, it is a latent footgun: if a future install ever wrote bare-name skill dirs, the cleanup would delete freshly installed skills. The PR's new entry devflow:dream-memory (namespaced, genuinely removed) is correct.

---

Lower-Confidence Suggestions (60-79%)

Consolidated from all reviewers; not actionable as blocking but worth noting:

Architecture & Complexity

• background-memory-update approaches god-script size (406 lines, 6+ responsibilities). The lock + watchdog subsystems are self-contained and could be extracted into sourced helpers (matching dream-lock / hook-log-init pattern) for independent testability. Assess if future additions warrant refactoring.
• State-C "9999999 sentinel" for "never" is slightly opaque; a OK_FILE_PRESENT boolean would read more directly.

Performance

• background-memory-update re-derives wc -l line counts at 3 sites (:151,164,176) against the same file; a single read after the claim would remove one subprocess.
• Two passes over the processing file (orphan-check and extraction) on ≤20 capped lines; unification is premature optimization but worth tracking.

Testing

• S5 timing flake residue (Confidence: 65%) — Even when timeout exists, the test burns ~2s of wall-clock per run relying on timeout 2 to race the worker's 90s acquire_lock wait. Recommend adding timeout: 15000 to runWorker execSync options to bound the suite and convert intermittent hangs into clear, reproducible failures.
• S2 git-state mutation tests assert on substrings that overlap user content; would benefit from asserting on structural markers (commit(s) ago count) rather than log-message text.
• Structural grep assertions (S4 watchdog, S5 lock, S8 stdin/env/comment) duplicate existing behavioral tests (AC-F3, S15) and break on harmless refactors. Consolidate so one structural assertion per security invariant remains.

Consistency

• Throttle key semantics changed from "last successful write" to "last spawn attempt" (dream-capture:151-164). Likely intentional (prevents spawn storms), but worth a one-line comment documenting the deliberate shift.
• .last-refresh-ok is a separate liveness signal that session-start-memory reads but the worker is the sole writer — couples the SessionStart hook to worker-private sentinel. If a fourth cross-hook file contract is added, the implicit file-protocol should be documented in one place (e.g., the hooks KB).

---

Decisions Citations

The PR applies the following decisions:

• ADR-008 (LLM-vs-plumbing) — worker confines itself to plumbing; content is LLM-authored by claude -p
• ADR-016 (Dream split into per-task skills) — memory removed from Dream task set; correctly contracted
• PF-009 (subsystem rename leaves stale refs) — end-to-end removal verified; one KB doc-drift item pre-existing
• PF-004 (migration idempotency) — migration correctly ENOENT-idempotent + non-ENOENT rethrow
• PF-006 (worker parse Stop-hook JSON) — worker receives CWD via argv, does not parse JSON

---

Recommendation

APPROVED_WITH_CONDITIONS

The PR is production-ready. Conditions for merge readiness:

1. Fix CLAUDE.md:189 — Update Model Strategy to remove "haiku for memory" reference (inline comment has suggestion)
2. Fix tests/eager-memory-refresh.test.ts:580 — Add precondition guard or drop timeout dependency (inline comment has suggestion)
3. Consider addressing carry-over issues — The two cycle-1 MEDIUMs (break_stale_lock, dream-recover race) remain and should be addressed via documented invariants or PID-liveness checks before merge. Both share a single root cause; one unified fix is preferable.

All other findings are quality-of-life improvements, pre-existing issues, or lower-confidence suggestions suitable for backlog or future refactors.

---

Claude Code | Git Agent (comment-pr operation)

[dean0x]

Cycle 2 Code Review Summary

Status: ✅ All high-confidence in-diff findings already posted as inline comments (from cycle 1 review).

Inline Comments (≥80% confidence, anchored to changed lines)

The following findings have inline comments at their source locations:

1. CLAUDE.md:189 — Model Strategy still lists "haiku for memory" (HIGH, 95%)
2. tests/eager-memory-refresh.test.ts:580 — Lock test vacuous when timeout absent (HIGH, 90%)
3. scripts/hooks/session-start-memory:129 — Redundant double range-walk (MEDIUM, 88%)
4. scripts/hooks/background-memory-update:89 — Lock-acquire timeout lacks rationale (MEDIUM, 82%)
5. scripts/hooks/session-start-memory:57 — 144-line mega-block (MEDIUM, 85%)
6. scripts/hooks/background-memory-update:208 — Turn-builder loop lacks invariant comment (MEDIUM, 80%)

---

Pre-Existing Issues (Not Blocking)

Reliability Carry-overs from Cycle 1 (No Fix Applied)

Two MEDIUM findings remain unaddressed and still present:

• break_stale_lock non-atomic (scripts/hooks/background-memory-update:76-86, 80%) — Lock-break relies on implicit invariant STALE_THRESHOLD (300s) > watchdog total (~125s). Recommendation: Stamp lock with owner PID and only break when stale AND owner dead, OR add assertion-comment documenting the invariant.

• dream-recover races worker's leftover-merge (scripts/hooks/dream-recover:148-170 vs background-memory-update:141-156, 80%) — Recovery moves without lock. Narrow impact but carries same implicit invariant as above. Recommendation: Add inline comment documenting lock-free safety by construction.

Both share a root cause (implicit invariant should be documented); one unified fix preferred.

Documentation Drift (Pre-existing)

• hooks/KNOWLEDGE.md:241 (90%) — Stale dream-fallback claim references removed fallback. PF-009 instance suitable for next knowledge base refresh.

TypeScript (Pre-existing, Harmless)

• src/cli/plugins.ts:531-533 (82%) — LEGACY_SKILLS_V2X lists still-active bare skill names alongside genuinely-removed ones. Harmless today (active skills install namespaced); latent footgun for future installs.

---

Lower-Confidence Suggestions (60-79%)

Architecture & Complexity

• background-memory-update is 406 lines with 6+ responsibilities; lock + watchdog subsystems could be extracted into sourced helpers for independent testability.
• State-C "9999999 sentinel" slightly opaque; OK_FILE_PRESENT boolean would be more direct.

Performance

• wc -l line counts re-derived at 3 sites (151,164,176); single read after claim would save one subprocess.
• Two passes over processing file (orphan-check, extraction) on ≤20 lines; unification is premature optimization.

Testing

• S5 timing flake residue (65%) — Even with timeout, test burns ~2s wall-clock. Recommend execSync(..., { timeout: 15000 }) to bound suite and convert hangs into reproducible failures.
• S2 git assertions on log substrings overlap user content; prefer structural markers.
• Structural grep assertions (S4, S5, S8) duplicate behavioral tests; consolidate.

Consistency

• Throttle key changed from "last successful write" to "last spawn attempt" (70%); document deliberate shift.
• .last-refresh-ok couples SessionStart to worker-private sentinel; if more cross-hook contracts added, document file-protocol in hooks KB (68%).

---

Decisions Applied

✅ ADR-008 (LLM-vs-plumbing) — worker is plumbing; content LLM-authored
✅ ADR-016 (Dream split) — memory correctly removed from Dream task set
✅ PF-009 (stale refs) — end-to-end removal verified; one KB drift pre-existing
✅ PF-004 (migration idempotency) — ENOENT-idempotent + rethrow correct
✅ PF-006 (JSON parsing) — worker correctly avoids parsing Stop-hook JSON

---

Recommendation

APPROVED_WITH_CONDITIONS

This PR is production-ready. Conditions for merge:

1. Fix CLAUDE.md:189 — Remove "haiku for memory" from Model Strategy (inline comment has suggestion)
2. Fix tests/eager-memory-refresh.test.ts:580 — Add precondition guard or drop timeout (inline comment has fix)
3. Consider addressing carry-over MEDIUMs — Document the implicit STALE_THRESHOLD > watchdog invariant (or add PID-liveness checks). Both findings share same root cause; one unified fix preferred.

All other findings are quality-of-life improvements or pre-existing issues suitable for backlog/future refactors.

---

Claude Code | Git Agent — comment-pr operation, cycle 2

[dean0x]

File: scripts/hooks/session-start-memory
Line: 173
Severity: HIGH (95% confidence)
Category: Performance + Correctness

Issue: Rev-walk dedup mis-derives COMMITS on empty-log fallback

The count derivation on line 173:

COMMITS=$(printf '%s\n' "$_DRIFT_LOG_FULL" | grep -c . 2>/dev/null || echo "0")

When git log fails transiently (line 172 fallback to || echo ""), the pipe printf '%s\n' "" | grep -c . emits 0 with exit code 1 (no match), triggering the outer || echo "0". This results in a two-line string 0\n0 assigned to COMMITS.

The subsequent guard [ "$COMMITS" -le 0 ] (line 197) then fails with integer expression expected (exit code 2), causing execution to fall through to the State B (drifted) branch. This renders a spurious ⚠ reconcile header when the correct state is A (in-sync).

Verification

• OLD code: git rev-list --count ... || echo "0" → clean single-line 0 → State A ✓
• NEW code on empty-log fallback: COMMITS=[0\n0] → comparison exits 2 → State B ✗

Fix

Drop the count's || echo "0" fallback and use parameter expansion for the empty case:

_DRIFT_LOG_FULL=$(git log --oneline "${STAMP_SHA}${HEAD}" 2>/dev/null || echo "")
COMMITS=$(printf '%s\n' "$_DRIFT_LOG_FULL" | grep -c . 2>/dev/null)
COMMITS=${COMMITS:-0}
_DRIFT_LOG=$(printf '%s\n' "$_DRIFT_LOG_FULL" | head -10)

This ensures COMMITS is always a single-line integer that the -le comparison accepts.

---

Review by Claude Code | Devflow code-review cycle 3

[dean0x]

File: tests/eager-memory-refresh.test.ts
Test: S17 (lines 1336–1386)
Severity: HIGH (95% confidence)
Category: Testing – False Green

Issue: S17 does not exercise the degraded JSON path it claims to test

S17's header comment documents testing the _JSON_AVAILABLE=false path where the worker:

1. Skips the orphan guard
2. Claims the queue via mv
3. Gets EXTRACTED="" → TURN_COUNT=0 → logs "No parseable turns — skipping" → exits 0

However, reproductions of the test's exact harness show the worker exits much earlier at the claude-binary gate:

[background-memory-update] Starting (CWD=/tmp/s17probe)
[background-memory-update] SKIP: claude binary not found on PATH

The command -v claude check (background-memory-update:69–73) runs BEFORE the JSON-parse/orphan/extraction logic (lines 129+). With no claude in the test farm, the worker bails at line 72 and never reaches the documented degraded-parse branch.

Impact

• The degraded JSON path (_JSON_AVAILABLE=false) — the entire point of S17 — has ZERO coverage
• All three assertions (logContent.toContain('Starting'), exitCode === 0, !exists(memFile)) pass for the trivial SKIP-claude-not-found path too
• This is the same green-for-the-wrong-reason defect that cycle 2 set out to eliminate in S5

Fix

Add a fake claude shim into the no-json farm so the worker passes the binary gate and reaches the _JSON_AVAILABLE=false branch:

// Put claude in the farm so the worker proceeds past the binary gate
const farmClaude = path.join(farmDir, 'claude');
fs.writeFileSync(farmClaude, '#!/bin/bash\nexit 0\n');
fs.chmodSync(farmClaude, 0o755);

// Then assert the documented observable:
expect(logContent).toContain('No parseable turns — skipping'); // proves degraded branch ran

This ensures S17 actually covers the path it documents, not just a coincidental early exit.

---

Review by Claude Code | Devflow code-review cycle 3

[dean0x]

PR Review Summary — Cycle 3

Base: main (59db82e)
Head: feat/eager-memory-refresh (353e371)
Cycle: 3 (reviewing cycle-2 resolution commits)

High-Confidence Findings (≥80%)

2 blocking issues posted as inline comments:

1. Performance + Correctness (95%) — session-start-memory:173

   • COMMITS count derivation yields 0\n0 on empty-log fallback, breaking integer comparison
   • Fix: drop the count's || echo "0", use ${COMMITS:-0} parameter expansion

2. Testing – False Green (95%) — tests/eager-memory-refresh.test.ts:S17

   • Worker exits at claude-binary gate before reaching documented degraded-JSON path
   • Zero coverage of the _JSON_AVAILABLE=false branch S17 claims to test
   • Fix: add fake claude shim to farm, tighten assertion to "No parseable turns"

---

Lower-Confidence Suggestions (60–79%)

Testing Report — Should Fix

• S5 dead variable (90% confidence, line 623/634/649–651): Remove unused workerRanAtAll and its void apology comment; log assertion is canonical evidence
• S17 assertion/comment drift (88% confidence, line 1380–1385): Tighten assertion to match documented exit path once blocking fix lands

Performance Report — Minor Clarity Nit

• grep -c . vs grep -c '' (65% confidence, line 173): grep -c '' or wc -l express "count lines" more directly; subsumed by blocking fix

---

Genuine Fixes Verified (Cycle 2 → 3)

Cross-cycle analysis confirms prior cycle-2 work:

• ✓ S5 lock test — macOS timeout-absence fix is genuine; confirms Node execSync timeout + log evidence + no-memory-write assertion prove the worker blocked for the full ~3s
• ✓ S16 queue-claim lost-race — directory-as-.processing trick correctly routes through mv failure branch; asserts branch-specific SKIP log message
• ✓ S8 comment-grep replacement — sentinel log-leak check (seed distinctive token, assert never reaches log) is genuine behavioral upgrade
• ✓ plugins.test.ts LEGACY_SKILL_NAMES guard — non-vacuous collision regression guard (LEGACY_SKILLS_V2X contains many entries)

FALSE_POSITIVE from cycle 2 (correctly not re-raised):

• git-fan-out finding was a false positive; rev-walk dedup is legitimate and correct

---

Scores & Recommendation

Category	Score
Performance	8/10
Testing	7/10

Status: CHANGES_REQUESTED

The rev-walk dedup is a real, correct optimization (one walk instead of two). The blocking issues are correctness (session-start-memory) and test vacuity (S17), both trivial fixes. Recommend fixing in place before merge — none require architectural changes.

---

Cycle Convergence: This is cycle 3. FP ratio (false positives / total findings) is still reasonable; no 70%+ convergence warning triggered.

---

Review by Claude Code | Devflow code-review
Findings deduced from .devflow/docs/reviews/feat-eager-memory-refresh/2026-06-07_2123/