feat(v2): skills overhaul - shadow paths and init migration

[dean0x]

Summary

Complete V2 skills overhaul with shadow path support and init system migration. Enables users to override installed skills without full rebuild while maintaining backward compatibility with old naming schemes.

Changes

• Shadow Skills Architecture: New ~/.devflow/skills/{name}/ path allows user overrides of installed skills (~/.claude/skills/devflow:{name}/)
• Init Migration: Updated init command to detect and migrate shadow skill overrides from old V2 naming convention
• Test Coverage: Expanded reference tests for integration helpers, preamble drift detection, and old-name discovery
• Runtime Alignment: Cross-component alignment tests ensure reviewer focus areas match orchestrator expectations

Breaking Changes

None — fully backward compatible with existing shadow skill overrides.

Testing

• All tests pass (134/134)
• Integration tests verify shadow path detection
• Cross-component alignment validated
• Install path + shadow path coverage broadened

Related Issues

Implements V2 skills infrastructure from project roadmap.

---

Created by Claude Code

[dean0x]

CRITICAL: Focus name mismatch

The review-methodology SKILL.md Integration table was changed to use testing as the focus name (line 110), but the canonical focus name used by the reviewer agent, code-review commands, and review-orchestration skill is tests. This creates a dispatch mismatch.

Fix: Change line 110 from:

| `testing` | devflow:testing |

to:

| `tests` | devflow:testing |

This matches the pattern used everywhere else: the Focus column uses the focus identifier (tests), the Pattern Skill column uses the skill reference (devflow:testing).

Confidence: 95% (Consistency review)
Category: Blocking

---

Claude Code Review • /code-review

[dean0x]

Should Fix: Unused import

The import { promises as fs } on line 11 is never used. All file operations in this file use the sync imports from line 10 (readFileSync, readdirSync, statSync). This is dead code that should be removed.

Fix: Remove this entire line:

import { promises as fs } from 'fs';

Confidence: 95% (TypeScript review)
Category: Should Fix

---

Claude Code Review • /code-review

[dean0x]

Should Fix: Nested try/catch control flow complexity

The migrateShadowOverrides function uses nested try/catch blocks with fs.access to determine file existence. The catch blocks carry semantic meaning but are empty, which inverts the reader's expectations. The nesting depth and inverted control flow reduce readability.

Fix: Extract an explicit existence check helper:

async function exists(p: string): Promise<boolean> {
  try { await fs.access(p); return true; } catch { return false; }
}

export async function migrateShadowOverrides(devflowDir: string): Promise<{ migrated: number; warnings: string[] }> {
  const shadowsRoot = path.join(devflowDir, 'skills');
  let migrated = 0;
  const warnings: string[] = [];

  for (const [oldName, newName] of SHADOW_RENAMES) {
    const oldShadow = path.join(shadowsRoot, oldName);
    const newShadow = path.join(shadowsRoot, newName);

    if (!await exists(oldShadow)) continue;
    if (await exists(newShadow)) {
      warnings.push(`Shadow '${oldName}' found alongside '${newName}' -- keeping '${newName}', old shadow at ${oldShadow}`);
      continue;
    }
    await fs.rename(oldShadow, newShadow);
    migrated++;
  }

  return { migrated, warnings };
}

Confidence: 85% (Complexity review)
Category: Should Fix

---

Claude Code Review • /code-review

[dean0x]

Blocking: Sequential I/O in migrateShadowOverrides

The function performs sequential fs.access and fs.rename calls inside a for...of loop over SHADOW_RENAMES (7 entries). Each iteration issues 1-2 filesystem calls sequentially, totaling up to 14 sequential I/O operations. These are independent per rename entry and should be parallelized.

Fix: Use Promise.all with map:

export async function migrateShadowOverrides(devflowDir: string): Promise<{ migrated: number; warnings: string[] }> {
  const shadowsRoot = path.join(devflowDir, 'skills');
  const warnings: string[] = [];

  const results = await Promise.all(
    SHADOW_RENAMES.map(async ([oldName, newName]) => {
      const oldShadow = path.join(shadowsRoot, oldName);
      const newShadow = path.join(shadowsRoot, newName);
      try {
        await fs.access(oldShadow);
        try {
          await fs.access(newShadow);
          warnings.push(`Shadow '${oldName}' found alongside '${newName}' — keeping '${newName}', old shadow at ${oldShadow}`);
          return 0;
        } catch {
          await fs.rename(oldShadow, newShadow);
          return 1;
        }
      } catch {
        return 0;
      }
    })
  );

  return { migrated: results.reduce((sum, n) => sum + n, 0), warnings };
}

This eliminates ~7-14ms of sequential filesystem latency from every init run.

Confidence: 82% (Performance review)
Category: Blocking

---

Claude Code Review • /code-review

[dean0x]

Should Fix: Test I/O consistency — sync vs async pattern

This new test file uses readFileSync, readdirSync, and statSync throughout (~30 call sites), while all other test files in the codebase (init-logic.test.ts, skill-namespace.test.ts, skills.test.ts) use async patterns with fs.promises.

While the file does import promises as fs on line 11, it's never used. Sync I/O blocks the Node.js event loop and can cause slower test execution, particularly on network mounts or slow storage. More importantly, this breaks test suite consistency.

Fix: Replace sync calls with async equivalents to match the project's test convention:

• readFileSync(path) → await fs.readFile(path, 'utf-8')
• readdirSync(path) → await fs.readdir(path)
• statSync(path) → await fs.stat(path)

This also allows removal of the unused line 11 import.

Confidence: 80% (Tests review)
Category: Should Fix

---

Claude Code Review • /code-review

[dean0x]

Should Fix: Non-null assertions after expect

Two locations in this test use the pattern expect(match).toBeTruthy() followed by match![1], which uses TypeScript non-null assertion (!). While the preceding expect guarantees truthy at test time, this is an anti-pattern — prefer type narrowing over assertions. If the regex changes and match becomes null, the ! would cause a runtime crash rather than a clear test failure on the expect line.

Fix: Use a guarded narrowing or helper function:

// Option 1: Guarded check
if (!coreMatch) {
  expect.unreachable('review-orchestration should list 7 core reviewers');
}
const coreReviewers = coreMatch[1].split(',').map(s => s.trim());

// Option 2: Helper function
function expectMatch(value: RegExpMatchArray | null, msg: string): RegExpMatchArray {
  expect(value, msg).toBeTruthy();
  return value!; // Single point of assertion
}

Confidence: 82% (TypeScript review)
Category: Should Fix

---

Claude Code Review • /code-review

[dean0x]

Code Review Summary: feat/v2-skills-overhaul (PR #168)

Inline Comments Created

I've created 7 inline comments with issues ≥80% confidence:

BLOCKING (Must fix before merge):

1. Focus name mismatch (line 110, review-methodology): testing vs tests dispatcher — 95% confidence
2. Sequential I/O in migrateShadowOverrides (line 66, init.ts): Parallelizable with Promise.all — 82% confidence
3. Nested try/catch complexity (line 70, init.ts): Extract exists() helper for readability — 85% confidence

SHOULD FIX (Address before merge):
4. Unused import (line 11, skill-references.test.ts): Dead code — 95% confidence
5. Test sync I/O (line 10, skill-references.test.ts): Break consistency, use async like other tests — 80% confidence
6. Non-null assertions (line 871, skill-references.test.ts): Use type narrowing instead — 82% confidence
7. Silent error swallowing (line 98, helpers.ts): Narrows catch to transient errors only — 82% confidence

Lower Confidence Findings (60-79%)

Summary comment only, no inline comments:

• Partial naming convention applied (82%) — 6 skills retain -patterns suffix while 7 were renamed. May be intentional for follow-up PR. Suggest adding a comment in code/PR describing the plan if deliberate.
• Plugin name / skill name divergence (65%) — devflow-frontend-design (plugin) maps to ui-design (skill). Explicit mapping exists and is tested, but asymmetry worth noting.
• OLD_SKILL_NAMES triple nested loop (82%) — Complex test scan with 8 allowlist checks per line. Test-only code, acceptable but could extract to named helper if it grows further.

Overall Assessment

Scores:

• Architecture: 8/10
• Complexity: 8/10
• Consistency: 8/10
• Performance: 9/10
• Regression: 9/10
• Security: 9/10
• Tests: 8/10
• TypeScript: 8/10

Verdict: CHANGES_REQUESTED

This PR executes an excellent large-scale skill rename with robust testing (950-line new test file with 29 tests, all passing). The architecture is sound. Once the 7 blocking/should-fix comments are addressed, this is ready to merge.

Key Strengths:

• Rename-proof test design (skills derived at runtime, never hardcoded)
• Cross-component alignment tests catching focus name dispatch bugs
• Strong shadow override migration pattern
• Comprehensive coverage across 143 files with zero old-name leakage

---

Claude Code Review • /code-review

[dean0x]

Code Review Summary: PR #168

Blocking Issues (≥80% Confidence) — Require Fixes Before Merge

1. Race Condition in migrateShadowOverrides (85% confidence)

File: src/cli/commands/init.ts:68-96

The function uses Promise.all to parallelize shadow migration, but three old skill names map to the same target:

• git-safety → git
• git-workflow → git
• github-patterns → git

When a user has both git-safety/ and git-workflow/ shadow directories, both check shadowExists('git') concurrently. If both see false, both attempt fs.rename to the same destination. The second silently overwrites the first, losing one user's shadow override.

Impact: Data loss — shadow override content silently overwrites.

Fix: Either revert to sequential for...of (simplest) or group SHADOW_RENAMES by target and process each group sequentially while different targets run in parallel.

---

2. Missing devflow-git-workflow in LEGACY_SKILL_NAMES (95% confidence)

File: src/cli/plugins.ts:209-318

The array includes devflow-git-safety and devflow-github-patterns, but devflow-git-workflow is missing. Users with old prefixed installs will have ~/.claude/skills/devflow-git-workflow/ left orphaned after upgrade.

Fix: Add 'devflow-git-workflow' alongside devflow-git-safety and devflow-github-patterns (around line 213-214).

---

3. Missing devflow: Prefixed Old Names in LEGACY_SKILL_NAMES (92% confidence)

File: src/cli/plugins.ts:288-317

The PR adds devflow:complexity-patterns, etc. for cleanup of the six pattern-suffix removals, but omits devflow:git-safety, devflow:git-workflow, and devflow:github-patterns. Users who installed between the namespace migration and this PR will have orphaned devflow:git-* directories.

Fix: Add these three entries:

// v2.0.0 skill renames: prefixed old names for the 3 git consolidation removals
'devflow:git-safety',
'devflow:git-workflow',
'devflow:github-patterns',

---

4. Stale Skill Count in Documentation (85-90% confidence)

File: docs/reference/file-organization.md:12

Comment states (31 skills) but should be (37 skills) to match actual directory count. The PR modified this file (renamed git-workflow/ to git/) but did not update the count.

Fix: Change (31 skills) to (37 skills).

---

Should-Fix Issues (80-90% Confidence)

5. Missing tests → testing Rename in README.md (90% confidence)

File: README.md:19,31

The PR renames the reviewer focus from tests to testing everywhere except README.md. Lines 19 and 31 still reference tests as a focus name.

---

6. Unified git Skill Exceeds Size Guideline (82% confidence)

File: shared/skills/git/SKILL.md (254 lines)

Project guidelines target 120-150 lines per SKILL.md. This consolidated skill is nearly double. Move GitHub API (lines 192-218) and Sensitive File Detection (lines 169-188) sections to references/ files.

---

7. Non-Null Assertions in New Test (82-85% confidence)

File: tests/build.test.ts:55-56

Uses match![1] after .toBeTruthy() instead of using the expect.unreachable pattern applied elsewhere in this PR for proper type narrowing.

---

Summary by Severity

Severity	Count	Status
BLOCKING	4	Require fixes
SHOULD-FIX	3	Recommended before merge

Overall recommendation: CHANGES_REQUESTED

All findings are from comprehensive multi-reviewer analysis (security, architecture, performance, complexity, consistency, regression, testing, TypeScript, documentation reviewers). The race condition in migrateShadowOverrides is the most critical — it causes silent data loss when multiple old shadow names compete for the same target.

---

Review comments generated from 9 reviewer reports with 80%+ confidence findings only. Lower-confidence suggestions (60-79%) available in individual review reports.