fix(decisions): update --review to deprecate in rendered markdown

[dean0x]

fix(decisions): update --review to deprecate in rendered markdown

When deprecating an observation via decisions --review observations mode, also update the Status field in the rendered decisions/pitfalls markdown file, matching the behavior in learn --review.

Fixes

• Deprecation in JSON log was not reflected in rendered markdown files
• Inconsistency between learn and decisions --review behavior

Testing

• Manual verification of decisions --review deprecation workflow
• Markdown file updates checked

Co-Authored-By: Claude noreply@anthropic.com

[dean0x]

Lock Inconsistency: fs.mkdir vs acquireMkdirLock 🔒

File: src/cli/commands/decisions.ts (lines 586-594)
Confidence: 85%
Category: BLOCKING

The decisions --review observations mode uses a bare fs.mkdir call for lock acquisition without stale-lock recovery. If a background process crashes and leaves .decisions.lock behind, this command will permanently fail until the user manually removes the lock.

Comparison: Your learn.ts --review (line 976) uses the robust acquireMkdirLock which includes:

• Stale lock detection (auto-removes locks >60s old)
• Retry logic with 30s timeout
• Graceful degradation

Suggested Fix:

import { acquireMkdirLock } from './learn.js';

// Replace lines 586-594:
const decisionsLockDir = path.join(memoryDir, '.decisions.lock');
const lockAcquired = await acquireMkdirLock(decisionsLockDir);
if (!lockAcquired) {
  p.log.error('Decisions system is currently running. Try again in a moment.');
  return;
}

Note: acquireMkdirLock would need to be exported from learn.ts or moved to a shared utility.

---

Claude Code Review | PR #206

[dean0x]

Capacity Review Lock Strategy 🔐

File: src/cli/commands/decisions.ts (lines 827-828)
Confidence: 82%
Category: MEDIUM (Should Fix)

The capacity review batch deprecation loop calls updateDecisionsStatus for each entry, relying on per-call internal locking. However, the comment states "no outer lock needed (no reentrancy issue since calls are sequential)" — this overlooks that a concurrent background decisions agent could interleave writes between individual calls.

Compare with your observations mode (line 586) which holds the full lock for the entire loop — use the same pattern:

const decisionsLockDir = path.join(memoryDir, '.decisions.lock');
const lockAcquired = await acquireMkdirLock(decisionsLockDir);
if (!lockAcquired) {
  p.log.error('Decisions system is currently running. Try again in a moment.');
  return;
}
try {
  // ... existing batch deprecation loop (lines 829-841) ...
} finally {
  try { await fs.rmdir(decisionsLockDir); } catch { /* already cleaned */ }
}

---

Claude Code Review | PR #206

[dean0x]

Type Safety: Unvalidated usageData Narrowing ⚠️

File: src/cli/commands/decisions.ts (line 775)
Confidence: 80%
Category: MEDIUM

After validating the top-level structure, the code casts via as typeof usageData without validating individual entries. A malformed entry (e.g., { cites: "not a number" }) would pass through and cause NaN in the sort comparator (line 785), breaking sort order.

Suggested Fix:

// Safe coercion in the sort comparator:
const aCites = typeof aUsage.cites === 'number' ? aUsage.cites : 0;
const bCites = typeof bUsage.cites === 'number' ? bUsage.cites : 0;
if (aCites !== bCites) return aCites - bCites;

---

Claude Code Review | PR #206

[dean0x]

Duplicated Promotion Logic 📋

File: scripts/hooks/json-helper.cjs (lines 1014-1041, 1752-1778)
Confidence: 85%
Category: BLOCKING

The identical "immediate type promotion on first creation" block is copy-pasted into both process-observations (new-entry path) and merge-observation (new-entry path). Any future change to the promotion protocol must be applied in two places — a maintenance risk.

Suggested Fix: Extract a shared helper:

function tryImmediatePromotion(entry) {
  if (entry.type !== 'decision' && entry.type !== 'pitfall') return;
  const th = THRESHOLDS[entry.type] || THRESHOLDS.procedural;
  if (entry.confidence >= th.promote && entry.quality_ok === true) {
    const firstSeenMs = new Date(entry.first_seen).getTime();
    const spread = (Date.now() - firstSeenMs) / 1000;
    if (!isNaN(firstSeenMs) && spread >= th.spread) {
      entry.status = 'ready';
    }
  }
}

Then call from both process-observations and merge-observation after constructing the new entry.

---

Claude Code Review | PR #206

[dean0x]

Type Guard Duplication 🔄

File: src/cli/commands/decisions.ts (lines 31-39)
Confidence: 82%
Category: SHOULD FIX

The isCountActiveResult was moved from learn.ts as a "local copy" with a comment stating "decisions.ts does not import from learn.ts for this guard." However, you already import 5 symbols from learn.ts (lines 22-29). Adding a 6th import is consistent with the existing pattern.

Suggested Fix: Export from learn.ts and import:

// learn.ts:
export function isCountActiveResult(val: unknown): val is { count: number } {
  return (
    typeof val === 'object' && val !== null && 'count' in val &&
    typeof (val as any).count === 'number'
  );
}

// decisions.ts:
import { ..., isCountActiveResult } from './learn.js';

This maintains single-source-of-truth and eliminates maintenance burden if the shape ever changes.

---

Claude Code Review | PR #206

[dean0x]

PR Review Summary: fix-threshold-promotion-bug (#206)

✅ Strengths

• Bug fix is correct: Decision/pitfall observations now promote to ready on first creation when quality_ok=true. Well-tested across both entry paths.
• Clean refactor: Capacity review moved from learn --review to decisions --review (its proper home, applies ADR-001).
• Regression protection: All 65 existing tests pass; workflow/procedural observations unaffected.

---

📊 Medium-Confidence Issues (75-79%)

Monolithic action handler (Complexity, 85%):

• decisions.ts --action() is 819 lines with 11 top-level branches. Recommend extracting each flag handler into named async functions, then route via simple dispatcher.

Test logic duplication (Testing, 85%):

• Capacity review tests replicate filtering/sorting inline instead of importing production functions. Extract filterEligibleEntries() and sortByLeastUsed() as shared functions tested directly.

Type-label formatting inconsistency (Consistency, 82%):

• decisions.ts:603 uses ternary: obs.type === 'decision' ? 'Decision' : 'Pitfall'
• learn.ts:989 uses generic: obs.type.charAt(0).toUpperCase() + obs.type.slice(1)
• Fix: Use consistent generic pattern in both files.

Inconsistent reason formatting (Consistency, 82%):

• decisions --review builds reason manually while learn --review uses shared formatStaleReason(). Export helper and use in both.

---

💡 Lower-Confidence Suggestions (60-79%)

• Subprocess spawning overhead (decisions.ts:862-878, 82%): Two execFileSync('node') calls for count-active (~100-200ms total). Could inline logic for zero-overhead.
• Sequential lock cycles (decisions.ts:830-841, 85%): N lock+read+write pairs for batch deprecation; group by file for single lock/write per file.
• Missing edge-case tests (decisions.ts:700-741, 80%): Markdown parser lacks tests for missing Date, missing Status fields.
• Help text mismatch (learn.ts:482, 65%): Help still says "at capacity" after feature moved to decisions.

---

Recommendation

CHANGES_REQUESTED — The five blocking issues (≥80% confidence, marked in inline comments) must be resolved. Medium-confidence refactoring can be deferred if preferred.

Generated by Claude Code Review

[dean0x]

Code Review Findings

Note: Creating inline comments for high-confidence (≥80%) blocking findings from 8 reviewer reports.

Blocking Issues Identified

1. Type Safety: DecisionsEntry.status should use literal union (85% confidence)

File: src/cli/commands/decisions.ts:139
The status field allows any string but the code only uses: 'Accepted', 'Active', 'Deprecated', 'Superseded', 'Unknown'. Using a discriminated union would catch type errors at compile-time.

2. Type Safety: DecisionsEntry.file should use literal union (82% confidence)

File: src/cli/commands/decisions.ts:137
The file field is typed as string but only ever holds 'decisions' or 'pitfalls'.

3. Incomplete Refactoring: Promotion logic duplication (85% confidence)

File: scripts/hooks/json-helper.cjs:1017-1030
The existing-entry promotion path still uses inline code instead of the extracted tryImmediatePromotion helper. Two near-identical paths violate DRY.

4. Test Anti-Pattern: D28 notification tests replicate inline logic (85% confidence)

File: tests/decisions/cli-subcommands.test.ts:505-565
Same anti-pattern that was fixed elsewhere in the PR (filterEligibleEntries, sortByLeastUsed) was not applied here. Tests validate a logic copy, not actual code.

5. Test Inconsistency: First capacity test doesn't call filterEligibleEntries (90% confidence)

File: tests/decisions/cli-subcommands.test.ts:436-455
Manually filters inline while other tests in the same block call the exported function. Inconsistent pattern.

6. Lock Release Asymmetry (85% confidence)

File: src/cli/commands/decisions.ts:729
Lock acquisition was hardened with timeouts and stale detection, but release still uses bare fs.rmdir. No companion releaseMkdirLock export.

---

Lower-Confidence Suggestions (60-79%)

• Architecture: learn.ts becoming a utility barrel exporting 8+ symbols for decisions.ts — violates SRP. Consider extracting to shared utilities (82% but architectural, not code-blocking).
• Performance: Lock timeout in --review observations might be too long for interactive CLI (72% confidence).
• Testing: No direct unit tests for tryImmediatePromotion helper (82% confidence, pre-existing).

---

Summary: 6 blocking findings (5 require code changes), 3 medium-severity architectural/pattern issues. PR is otherwise a clean refactoring that reduces duplication across multiple modules.

Claude Code | devflow:git

[dean0x]

Review Findings - Inline Comments

Creating inline comments for high-confidence (≥80%) findings from code review. Processing 6 findings across 3 files.

[dean0x]

Type Safety: VALID_STATUSES set (src/cli/commands/decisions.ts:136)

The VALID_STATUSES set is typed as Set<string> while it should mirror the DecisionsEntryStatus union. If a new status is added to the union but not to the set, the compiler won't catch the mismatch. Confidence: 85%

Fix: Type the set as Set<DecisionsEntryStatus> to enforce compile-time validation:

const VALID_STATUSES: Set<string> = new Set<DecisionsEntryStatus>([
  'Accepted', 'Active', 'Deprecated', 'Superseded', 'Unknown',
]);

Or derive both from a single source to prevent drift.

---

Claude Code Review | devflow:review:orch

[dean0x]

Type Safety: updateDecisionsStatus parameter (src/cli/commands/learn.ts:389)

The updateDecisionsStatus function accepts newStatus: string instead of the now-defined DecisionsEntryStatus union type. This allows any arbitrary string to be written as a status, losing compile-time safety. All three callers pass 'Deprecated' which is a valid status. Confidence: 82%

Fix: Narrow the parameter type to enforce only valid statuses at compile time:

export async function updateDecisionsStatus(
  filePath: string,
  anchorId: string,
  newStatus: DecisionsEntryStatus,
): Promise<boolean> {
  // ...
}

This requires importing DecisionsEntryStatus in learn.ts or using a shared types module.

---

Claude Code Review | devflow:review:orch

[dean0x]

Security: Unsanitized artifact_path (src/cli/commands/decisions.ts:730-738)

The obs.artifact_path field (sourced from local JSONL log) is split on # and the path portion is passed to updateDecisionsStatus for filesystem write. If a corrupted or adversarially crafted log entry contains a path outside .memory/decisions/ (e.g., ../../.claude/settings.json#ADR-001), the function could overwrite arbitrary markdown files. Confidence: 82%

Fix: Add a path containment check before calling updateDecisionsStatus:

const absPath = path.isAbsolute(decisionsFilePath)
  ? decisionsFilePath
  : path.join(process.cwd(), decisionsFilePath);
const expectedDir = path.join(memoryDir, 'decisions');
if (!absPath.startsWith(expectedDir + path.sep)) {
  p.log.warn(`Skipping out-of-bounds artifact path: ${decisionsFilePath}`);
} else {
  const updated = await updateDecisionsStatus(absPath, anchorId, 'Deprecated');
  // ...
}

Risk is mitigated by local-only writes, but defense-in-depth is prudent.

---

Claude Code Review | devflow:review:orch

[dean0x]

Test Coverage: Type annotation missing (tests/decisions/cli-subcommands.test.ts:444-449)

The allEntries array at line 444 is constructed with plain object literals without a DecisionsEntry[] type annotation. Since this PR tightens DecisionsEntry.status from string to DecisionsEntryStatus and file to 'decisions' | 'pitfalls', these test objects bypass the new type constraints via TypeScript inference. Any typo in status or file values won't be caught at compile time. Confidence: 85%

Fix: Add explicit type annotation:

const allEntries: DecisionsEntry[] = [
  { id: 'ADR-001', pattern: 'Use X', file: 'decisions', filePath: '/tmp/decisions.md', status: 'Accepted', createdDate: '2026-01-01' },
  // ...
];

Note: The other two tests in this describe block correctly use DecisionsEntry[] annotation.

---

Claude Code Review | devflow:review:orch

[dean0x]

Test Coverage: toDecisionsStatus normalizer not tested (src/cli/commands/decisions.ts:139-141)

The PR introduces a new toDecisionsStatus function that normalizes raw markdown status strings into the DecisionsEntryStatus union, silently mapping unrecognized strings to 'Unknown'. This boundary validator has no unit tests. If the set of valid statuses diverges from what markdown files contain, the silent fallback could cause downstream logic to misclassify entries without test detection. Confidence: 88%

Fix: Add tests for toDecisionsStatus (export it or test indirectly). At minimum:

describe('toDecisionsStatus normalizer', () => {
  it('passes through valid statuses', () => {
    for (const s of ['Accepted', 'Active', 'Deprecated', 'Superseded', 'Unknown']) {
      expect(toDecisionsStatus(s)).toBe(s);
    }
  });
  it('maps unrecognized strings to Unknown', () => {
    expect(toDecisionsStatus('Archived')).toBe('Unknown');
    expect(toDecisionsStatus('')).toBe('Unknown');
  });
});

---

Claude Code Review | devflow:review:orch

[dean0x]

Test Coverage: acquireMkdirLock EEXIST discrimination not tested (src/cli/commands/learn.ts:339-341)

The PR hardens acquireMkdirLock to re-throw unexpected filesystem errors (EACCES, EPERM) instead of swallowing them as "lock held". This is a meaningful behavioral change — previously, any mkdir failure would be retried for 30 seconds, potentially spinning on permissions errors. However, this new behavior has no test coverage. The function is not tested anywhere in the test suite. Confidence: 82%

Fix: Add tests for EEXIST discrimination:

describe('acquireMkdirLock', () => {
  it('re-throws non-EEXIST errors from mkdir', async () => {
    // Mock fs.mkdir to throw EACCES
    await expect(acquireMkdirLock('/root/forbidden-lock')).rejects.toThrow();
  });
  it('returns true when lock directory is created successfully', async () => {
    const lockDir = path.join(tmpDir, 'test-lock');
    const result = await acquireMkdirLock(lockDir);
    expect(result).toBe(true);
    await fs.rmdir(lockDir);
  });
});

---

Claude Code Review | devflow:review:orch

[dean0x]

Summary: PR #206 Review Comments

Successfully created 6 inline comments for high-confidence (≥80%) findings. Each comment links to specific files and line ranges with actionable fixes.

Inline Comments Posted

1. ✅ Type Safety: VALID_STATUSES set (85% confidence) — decisions.ts:136
2. ✅ Type Safety: updateDecisionsStatus parameter (82% confidence) — learn.ts:389
3. ✅ Security: Unsanitized artifact_path (82% confidence) — decisions.ts:730-738
4. ✅ Test Coverage: Missing type annotation (85% confidence) — test.ts:444-449
5. ✅ Test Coverage: toDecisionsStatus not tested (88% confidence) — decisions.ts:139-141
6. ✅ Test Coverage: acquireMkdirLock not tested (82% confidence) — learn.ts:339-341

Lower-Confidence Suggestions (60-79%) — Not Posted as Inline

These should be considered but have lower confidence:

• Test data uses inferred string types for status instead of DecisionsEntryStatus (65%)
• newStatus parameter untyped in function signature (65%) — overlaps with comment feat: Language-Agnostic Global CLAUDE.md with TypeScript Auditor #2
• anchorId from artifact_path not validated (62%)
• D28 notification tests could verify threshold parameter usage (70%)
• clearCapacityNotifications tests use untyped object literals (65%)

Deduplication

• Existing PR comments: 0 (clean slate)
• New comments created: 6
• Skipped (lower confidence): 5

Status: READY | All findings documented with suggested fixes

---

Claude Code Review | devflow:review:orch