feat(learning): v2 unified self-learning + project knowledge

[dean0x]

Summary

• Unified 4-type self-learning extractor (workflow, procedural, decision, pitfall) replaces the three retrospective knowledge writers previously embedded in command phases
• Project knowledge (decisions.md, pitfalls.md) is now written by the background learning hook via render-ready, with per-type thresholds and feedback reconciliation
• New HUD row + devflow learn --review / --purge-legacy-knowledge for triage; knowledge-persistence skill is now a format spec only

Closes #177

What changed

Core infrastructure

• scripts/hooks/lib/transcript-filter.cjs (new) — two-channel filter (USER_SIGNALS + DIALOG_PAIRS)
• scripts/hooks/json-helper.cjs — per-type THRESHOLDS, ops render-ready / reconcile-manifest / merge-observation / knowledge-append, shared acquireMkdirLock
• scripts/hooks/background-learning — unified 4-type extractor prompt + D7 v1→v2 migration
• scripts/hooks/session-start-memory — reconciler call on each session start (deletion/edit feedback)

Commands / plugins

• Removed retrospective knowledge writer phases from /implement, /code-review, /debug, /resolve
• shared/skills/knowledge-persistence/SKILL.md rewritten as format spec only (no Write tool)
• Dropped knowledge-persistence from devflow-implement, devflow-code-review, devflow-resolve plugin manifests (kept in debug/plan/ambient as a reader)

HUD + CLI

• src/cli/hud/learning-counts.ts + component — promoted-knowledge row with attention flag
• devflow learn --review — interactive triage with lock + atomic writes
• devflow learn --purge-legacy-knowledge — removes pre-v2 command-phase-written entries

Docs + tests

• CLAUDE.md, README.md, docs/self-learning.md, docs/working-memory.md updated for 4-type system
• 80+ new unit tests (filter, thresholds, 4 renderers, reconcile, merge-observation, staleness, HUD counts, review command, migration)
• 3 new integration tests under tests/integration/learning/
• Pre-existing tests/shell-hooks.test.ts + tests/learn.test.ts updated for per-type thresholds and quality_ok gate

All D1–D16 design decisions are documented as JSDoc at implementation sites.

Test plan

• npm test — 760/760 passing (35 test files, 2.50s)
• npm run build — clean (tsc + plugin distribution + HUD)
• Tester scenarios 12/12 pass (reconciler deletion/edit, 4 render targets, pitfall dedup, HUD populated/empty, --review happy path, --purge-legacy-knowledge, D7 migration, command files clean of writers, plugin manifests updated, SKILL is format spec)
• Scrutinizer (b1134ca) — 8 P0 + 3 P1 issues resolved
• Evaluator (b2efaf5) — ALIGNED after D7 migration + threshold docs + needsReview JSDoc fixes
• Post-merge: run devflow learn --purge-legacy-knowledge on projects with pre-v2 entries

Known notes

• tests/integration/learning/end-to-end.test.ts runs via npm run test:integration (separate vitest config) — not in default suite
• Pre-existing flaky tests/integration/ambient-activation.test.ts (main-branch issue, design-review router drift) is orthogonal to this PR

[dean0x]

CRITICAL — Install ordering regression ⚠️ 92% confidence

Shadow-override migration is running AFTER installViaFileCopy, not before. This breaks V1→V2 upgraders:

1. installViaFileCopy looks for shadows at V2 names (~/.devflow/skills/software-design/) → not found
2. Installs stock skill to ~/.claude/skills/devflow:software-design/
3. runMigrations renames V1 shadow (core-patterns) → V2 name
4. Migration marked applied; shadow now at correct path; but stock content already in ~/.claude/skills/
5. Next init re-reads shadow at correct location but doesn't re-run installer

User impact: Customized shadows on V1→V2 upgraders are silently lost on first init.

Fix: Move the runMigrations block (current lines 888-912) above installViaFileCopy (line 762). Per-project migrations depend on discoveredProjects which is computed earlier, so no wiring changes needed.

[dean0x]

HIGH — Busy-wait CPU spin in lock acquisition 95% confidence

The mkdir-lock retry loop uses a synchronous CPU spin instead of sleeping:

while (Date.now() < end) { /* empty loop burns CPU */ }

Under contention with json-helper.cjs render-ready holding the lock, this pegs one core at 100% for up to 2 seconds. This runs on every Stop hook (every assistant turn), so impact is per-turn.

Fix: Replace with Atomics.wait:

const SLEEP_BUF = new Int32Array(new SharedArrayBuffer(4));
function syncSleep(ms) { Atomics.wait(SLEEP_BUF, 0, 0, ms); }

// Replace lines 64-66
syncSleep(10);  // Yield instead of burning CPU

Or convert to async and use await new Promise(r => setTimeout(r, 10)).

[dean0x]

HIGH — Shadow migration warnings silently dropped 92% confidence

The registry wrapper (lines 55-58) awaits migrateShadowOverridesRegistry without destructuring the returned { migrated, warnings }. User-facing warnings about shadow name conflicts are lost.

The original inline code in init.ts produced:

• Migrated N shadow override(s) to V2 names
• Shadow 'X' found alongside 'Y' — keeping 'Y', old shadow at ... (per conflict)

These warnings are now silently dropped.

Fix: Either extend Migration.run to optionally return metadata:

run: async (ctx) => {
  const { migrated, warnings } = await migrateShadowOverridesRegistry(ctx.devflowDir);
  // Surface via console or return from run()
}

Or add an optional onSuccess hook to Migration entries so the runner can invoke it with the result.

[dean0x]

HIGH — Unsafe JSON.parse assignment bypasses type 92% confidence

notifications = JSON.parse(...) assigns directly to a variable typed as Record<string, NotificationFileEntry> with no runtime validation. JSON.parse returns any, so this coerces arbitrary JSON into the declared type.

If the file is malformed but parses (array [], primitive 42, null, or object with wrong structure), subsequent Object.entries(notifications) / writeback will corrupt the file further via line 1243's writeFileAtomic(... JSON.stringify(notifications)).

Fix: Add a type guard matching the pattern in applyConfigLayer (lines 275-288):

function isNotificationMap(v: unknown): v is Record<string, NotificationFileEntry> {
  return typeof v === 'object' && v !== null && !Array.isArray(v)
    && Object.values(v).every(e =>
      typeof e === 'object' && e !== null
      && (e.active === undefined || typeof e.active === 'boolean')
      // ... validate other fields
    );
}

let notifications: Record<string, NotificationFileEntry> = {};
try {
  const parsed: unknown = JSON.parse(await fs.readFile(notifPath, 'utf-8'));
  if (isNotificationMap(parsed)) notifications = parsed;
} catch { /* no file */ }

[dean0x]

HIGH — Type assertion bypasses runtime validation (88% confidence)

severity: (worst.entry.severity as NotificationData['severity']) ?? 'dim'

The source is typed as string | undefined, but coerced to 'dim' | 'warning' | 'error' without runtime check.

Fix: Use a type guard before assignment.

[dean0x]

HIGH — Per-project parallel sweep unbounded (82% confidence)

Promise.allSettled(discoveredProjects.map(...)) has no concurrency limit. With 50-200 projects, this risks EMFILE (too many open files) on macOS (ulimit -n = 256).

Fix: Use parallelMap helper with 8-16 worker limit to serialize filesystem access.

[dean0x]

CRITICAL — Teams-variant commands still invoke knowledge-persistence (95% confidence)

Base .md commands removed their "Record Pitfalls/Decisions" phases (D8 refactor), but -teams.md variants were NOT updated. Teams users still get instructions to use a skill that no longer has Write capability.

Affected: code-review-teams.md, resolve-teams.md, debug-teams.md, implement-teams.md

Fix: Apply same D8 removals to all four teams variants. Users installing with --teams lose knowledge capture — behavioral regression.

[dean0x]

HIGH — Test isolation violation (95% confidence)

E2E test writes to real ~/.claude/projects/ and ~/.devflow/logs/. If test aborts, artifacts leak into live user directory.

Fix: Override HOME in beforeEach/afterEach to contain test artifacts in tmp directory (pattern already in tests/migrations.test.ts:136-148).

[dean0x]

SUMMARY: PR Comments Created

Inline Comments

Created 8 inline comments for CRITICAL/HIGH blocking issues (≥80% confidence):

1. CRITICAL — Install ordering regression (92%) — Shadow migration runs after install, losing user customizations
2. HIGH — Busy-wait CPU spin (95%) — Lock acquisition pegs CPU, runs on every Stop hook
3. HIGH — Shadow migration warnings dropped (92%) — User-facing conflict warnings silently lost
4. HIGH — Unsafe JSON.parse in notifications (92%) — Malformed files corrupt without error
5. HIGH — Type assertion bypasses severity validation (88%)
6. HIGH — Unbounded per-project concurrency (82%) — EMFILE risk with 50+ projects
7. CRITICAL — Teams-variant regression (95%) — Commands instruct skill use that no longer works
8. HIGH — E2E test isolation violation (95%) — Leaks to live user ~/.claude/projects/

Deduplicated Findings

• Teams-variant inconsistency: Flagged in both Regression and Documentation reviews
• MigrationContext empty-string sentinels: Flagged in both Consistency and Architecture reviews
• knowledge-persistence skill role change: Mentioned in Documentation and multiple command files

Lower-Confidence Findings (60-79%)

Reserved for summary comment (not posted as inline). Includes:

• Migration O(N²) writes during cold init
• Legacy-knowledge-purge nesting complexity
• HUD render hot path re-reading files
• Various test coverage gaps

Rate Limiting

• 1 second delay between API calls ✓
• Remaining API quota checked (not enforced in this run)

---

Next Steps: Review findings, apply fixes, and return for follow-up verification.

[dean0x]

PR Comments: Code Review Findings

Overview

This is an incremental review of 10 commits (0dd9e24...HEAD) addressing prior review findings on security hardening, architecture refinement, and knowledge-system consistency. All three prior CRITICAL regressions are resolved. However, four new HIGH-severity findings block merge, and several MEDIUM findings should be addressed.

HIGH-Severity Findings (Blocking)

1. Race-Tolerance Divergence in Atomic-Write Pattern (Confidence: 95%)

Files: src/cli/commands/learn.ts:395-407, src/cli/utils/legacy-knowledge-purge.ts:50-61, src/cli/utils/migrations.ts:157-180, scripts/hooks/json-helper.cjs:137-146, scripts/hooks/knowledge-usage-scan.cjs:113-123

Problem: The wx-flag atomic-write pattern spreads to 5 sites, but race-tolerance diverges between TypeScript and JavaScript implementations:

• TS sites (learn.ts, legacy-knowledge-purge.ts, migrations.ts): After EEXIST, call await fs.unlink(tmp) with no try/catch, so if another writer races ahead and unlinks the stale file, this operation throws ENOENT.
• JS sites (json-helper.cjs, knowledge-usage-scan.cjs): Wrap the unlink in try { fs.unlinkSync(...) } catch { /* race */ }, tolerating the concurrent unlink.

This is a correctness bug: the documented intent is "stale tmp — unlink and retry once"; the TS sites violate the documented contract by not handling the expected race condition.

Fix: Extract a shared writeFileAtomicExclusive(filePath, content) helper in src/cli/utils/fs-atomic.ts that mirrors the JS race semantics, and import it from all three TS sites. The helper must wrap the unlink in try/catch:

try { await fs.unlink(tmp); } catch { /* race — already removed */ }

---

2. isNotificationMap Defined Twice with Divergent Strength (Confidence: 98%)

Files: src/cli/hud/notifications.ts:28-30 vs src/cli/commands/learn.ts:31-36

Problem: Both files define isNotificationMap(v): v is Record<string, NotificationEntry> for the same .memory/.notifications.json file, but enforce different invariants:

• notifications.ts (weak): Accepts { foo: 42 } (primitive entry values) ✗
• learn.ts (strong): Rejects non-object entries, walks Object.values(...).every(entry => typeof entry === 'object') ✓

A file shape passing the HUD guard but failing learn.ts's guard will be treated inconsistently. This is the exact "said one thing in one place, another elsewhere" pattern the consistency review was designed to catch.

Fix: Extract both into a shared module src/cli/utils/notifications-shape.ts with the stronger guard and import from both sites.

---

3. Unnecessary as Migration<...> Casts Defeat Discriminated-Union Narrowing (Confidence: 92%)

File: src/cli/utils/migrations.ts:265, src/cli/utils/migrations.ts:307

Problem: Inside runMigrations, after narrowing on migration.scope === 'global' (line 256) and migration.scope === 'per-project' (line 280), the code casts to as Migration<'global'> and as Migration<'per-project'>. Empirically verified: TypeScript narrows correctly without the casts. Running npx tsc --noEmit passes cleanly. The casts teach future maintainers that the discriminated union is insufficient (it isn't) and silently hide bugs where mismatched contexts are passed through the cast.

Fix: Remove both casts and rely on the discriminant narrowing.

---

4. D-Tag Naming Inconsistent Across Security Commits (Confidence: 90%)

Files: src/cli/commands/learn.ts:27, 37, 125 (D-SEC1/2/3), src/cli/utils/legacy-knowledge-purge.ts:45, 48 (D35), src/cli/hud/notifications.ts:24-30 (no tag), src/cli/utils/migrations.ts:16, 74, 121, 223, 258, 282, 294 (D30–D38), scripts/hooks/json-helper.cjs:131-135 (no tag)

Problem: Per user feedback ("design-decisions-jsdoc"), every load-bearing decision must carry a D-series JSDoc comment. This PR mixes three schemes:

1. D-SEC1/2/3 (learn.ts only)
2. D30–D38 (migrations.ts, legacy-knowledge-purge.ts)
3. No tag (notifications.ts, json-helper.cjs)

Additionally, D35 is used for two unrelated decisions (concurrency cap in migrations.ts:282 vs TOCTOU fix in legacy-knowledge-purge.ts:45), causing collision ambiguity.

Fix: Normalize to numeric scheme. Recommend: keep migrations.ts:282 as D35, renumber legacy-knowledge-purge.ts:45 to D39, and add D-tags to notifications.ts and json-helper.cjs guards using fresh unused numbers.

---

5. Lock Helpers Diverge: Three Names, Two Timeouts, One Contract (Confidence: 92%)

Files: src/cli/commands/learn.ts:357 (acquireMkdirLock), src/cli/utils/legacy-knowledge-purge.ts:68-90 (acquireMkdirLock), scripts/hooks/json-helper.cjs:429 (acquireLock), scripts/hooks/background-learning:70-80 (bash acquire_lock with timeout=90, STALE_THRESHOLD=300)

Problem: The documented contract (shared/skills/knowledge-persistence/SKILL.md:117-121) specifies "30 s timeout, 60 s stale". The TS code matches (both call acquireMkdirLock(30_000, 60_000)), but the bash hook uses very different values (timeout=90, STALE_THRESHOLD=300), and the function names drift (acquireMkdirLock vs acquireLock vs acquire_lock).

Fix:

1. Rename scripts/hooks/json-helper.cjs::acquireLock → acquireMkdirLock (unify naming)
2. Update bash hook to use timeout=30, STALE_THRESHOLD=60, or document why it needs different values
3. Cross-reference the skill's lock contract from all three implementations

---

6. runMigrations Integration Seam Tests Are Misleading Duplicates (Confidence: 93%)

File: tests/init-logic.test.ts:857-969

Problem: The added describe block claims to "test the integration between init and runMigrations," but the three tests import runMigrations directly and call it directly — they never invoke init.ts or any init-level function. All three tests are functional duplicates of existing coverage in tests/migrations.test.ts. The actual integration seam (init.ts:769-794 calling runMigrations with projectsForMigration fallback, running BEFORE installViaFileCopy per PF-007) is completely untested.

Fix: Either delete the duplicate tests and rely on migrations.test.ts for unit coverage, or rewrite them to actually test the integration: factor out the migration-execution block into an exported helper and test it end-to-end, including the ordering guarantee and the projectsForMigration fallback logic.

---

7. Security Hardening to knowledge-usage-scan.cjs Has No Test Coverage (Confidence: 90%)

File: scripts/hooks/knowledge-usage-scan.cjs (commit ab20b47)

Problem: Three security fixes in this file have zero test coverage:

1. Reject relative --cwd input with process.exit(2) (CWE-23 path-traversal)
2. Replace busy-spin with Atomics.wait (zero-CPU sleep)
3. Write .tmp with wx flag + EEXIST retry (symlink TOCTOU)

A regression dropping the path.isAbsolute check would not fail any test — the scanner would silently accept ../../../etc/passwd as cwd.

Fix: Add three targeted tests mirroring tests/legacy-knowledge-purge.test.ts:218-244:

• Reject relative cwd with exit code 2
• Do not follow symlink at .knowledge-usage.json.tmp (TOCTOU)
• Smoke-test that Atomics.wait does not peg CPU

---

Summary Table

Category	CRITICAL	HIGH	MEDIUM	LOW
Blocking	0	6	-	-
Should Fix	-	0	8	-
Pre-existing	-	-	-	-

Additional MEDIUM Findings (Should Fix)

• Architecture: Contract drift between ctx.devflowDir parameter and internal state file location (migrations.ts:237-243)
• Architecture: writeFileAtomic duplicated 4× — extract shared helper (migrations.ts, learn.ts, legacy-knowledge-purge.ts, json-helper.cjs)
• Architecture: runMigrations uses direct os.homedir() hurting testability (migrations.ts:243)
• Complexity: runMigrations touches 5 thresholds at once — extract scope branches into helpers (migrations.ts:236-347)
• Complexity: init.ts migration-runner block adds 4 sequential output loops — extract reporter (init.ts:768-794)
• Consistency: staleness.cjs writes learning-log non-atomically, breaking atomic-write contract elsewhere (staleness.cjs:92)
• Testing: writeExclusive helper in json-helper.cjs has no dedicated test
• Testing: New runtime type guards have no direct test coverage (isSeverity, isNotificationMap, isCountActiveResult, isRawObservation)
• TypeScript: const parsed = JSON.parse(raw) at learn.ts:1127 relies on implicit any property access

Recommendation

CHANGES_REQUESTED — Merge blocked until all six HIGH findings are addressed. The fixes are narrowly scoped (extract helpers, remove casts, normalize tags, rewrite tests). Once addressed, the PR delivers substantial security hardening, architectural refinement, and testing improvements.

---

Incremental Review Context

This is an incremental review of PR #181 resolving prior findings. The commit range covers 10 commits addressing:

• PF-007 (migrations-after-installer) — resolved via install ordering fix ✓
• PF-008 (teams-variant drift) — resolved via synchronized phase removal ✓
• PF-009 (busy-wait in per-turn hooks) — resolved via Atomics.wait ✓
• PF-010 (unvalidated JSON.parse) — partially resolved via new runtime guards (remaining casts need fixing)

Full test suite: 848/848 passing. CLI build: clean.