feat(v2): track3 ambient refinements — skill renames, detection-only preamble

[dean0x]

Summary

• Renamed 9 skills to shorter names: ambient-router→router, implementation-orchestration→implement, debug-orchestration→debug, plan-orchestration→plan, review-orchestration→review, resolve-orchestration→resolve, pipeline-orchestration→pipeline, implementation-patterns→patterns, search-first→research
• Renamed hook ambient-prompt → preamble and replaced self-contained preamble (with inline skill mappings) with a detection-only version that tells Claude to load devflow:router for skill mappings
• Trimmed router/SKILL.md (~130 lines): removed Step 5 agent orchestration table (details now live in individual orchestration skills), removed examples column from intent table, simplified transparency rules to single line
• Added TDD enforcement block to implement/SKILL.md as defense-in-depth
• Removed SessionStart router injection (Section 2 deleted — obsolete now that preamble is detection-only)
• Updated all cross-references: plugin manifests, CLI registry (LEGACY_SKILL_NAMES + SHADOW_RENAMES for migration), agent frontmatter, CLAUDE.md, docs, command files, READMEs, and all tests
• Branding: Ambient: classification prefix → DevFlow: throughout

Test plan

• npm run build passes (38 skills, 17 plugins, correct asset distribution)
• npm test passes — 584/584 tests pass (0 failures)
• Preamble drift detection test updated to reflect new detection-only preamble format
• skill-references tests updated (review-orchestration path → review/, ambient-prompt → preamble)
• skill-namespace test passes (new bare names added to LEGACY_SKILL_NAMES for migration cleanup)

[dean0x]

Code Review Findings

Creating inline comments for high-confidence (≥80%) blocking and should-fix issues from 9 review reports.

Summary of Issues Found

• 6 code issues (Complexity, Architecture, Performance, TypeScript, Testing, Regression)
• 2 documentation issues (HIGH severity - blocking)

Processing comments now...

[dean0x]

Detailed Review Findings (≥80% confidence)

🔴 BLOCKING Issues (6 total)

1. Dual JSON.parse in addAmbientHook — src/cli/commands/ambient.ts:48-51

• Confidence: 85% (Architecture, Performance)
• Issue: Line 50 calls removeLegacyAmbientHook(settingsJson) which parses JSON → serializes, then line 51 immediately parses the result again. This is inefficient and couples the functions through string serialization.
• Fix: Extract shared helper filterHookEntries that operates on parsed Settings objects, eliminating both this inefficiency AND the duplication between removeLegacyAmbientHook and removeAmbientHook (flagged by Complexity review).

2. Timer leak in runClaudeStreaming grace window — tests/integration/helpers.ts:113-116

• Confidence: 82% (Testing), 90% (TypeScript)
• Issue: When skills are detected, an 8-second grace timer is started but never cleared if the process closes before 8s elapse. This orphaned timer keeps Node.js event loop alive and delays test completion.
• Fix: Store grace timer reference and clear it in close/error handlers.

3. Dead export: isFirstToolASkill — tests/integration/helpers.ts:188-192

• Confidence: 85% (Testing), 95% (TypeScript)
• Issue: Exported but never used anywhere. Name implies it checks ordering (first tool is Skill) but actually just checks result.skills.length > 0 — identical to hasSkillInvocations.
• Fix: Remove lines 184-192.

4. Stale skill count: '38' should be '39' — CLAUDE.md:53, README.md:51/64, docs/reference/file-organization.md:12

• Confidence: 95% (Documentation)
• Issue: New explore skill added but all four doc locations still say "38 skills".
• Fix: Update all four locations to "39 skills".

5. Missing explore skill in Tier 1 catalog — docs/reference/skills-architecture.md

• Confidence: 92% (Documentation)
• Issue: New orchestration skill explore exists but missing from the Foundation Skills table.
• Fix: Add row to Tier 1 table: | explore | Codebase analysis, flow tracing, architecture mapping | Ambient EXPLORE intent |

6. Code duplication: removeLegacyAmbientHook + removeAmbientHook — src/cli/commands/ambient.ts:16-41 + 91-118

• Confidence: 85% (Complexity)
• Issue: Both functions share ~25 lines of identical structure (parse JSON, filter UserPromptSubmit, clean empty objects, serialize). Only difference is filter predicate.
• Fix: Extract filterHookEntries(json, predicate) helper; both become one-liners. (Also fixes blocking issue feat: Add project-agnostic release automation workflow #1 above.)

---

SHOULD-FIX Issues (4 total — architectural/quality)

7. LEGACY_SKILL_NAMES array is unbounded — src/cli/plugins.ts:227-364

• Confidence: 80%-82% (Architecture, Performance, Complexity)
• Issue: Now ~100+ entries across 7 comment-delimited sections, growing with every rename. During init, all 100+ entries are fs.rm'd regardless of existence.
• Fix: Group by version/era using helper arrays, flatten once. Or derive from SHADOW_RENAMES + ancient names.

8. SHADOW_RENAMES has no validation — src/cli/plugins.ts:372-398

• Confidence: 80% (Complexity)
• Issue: Array maps old→new names with no compile-time check that new names actually exist. Typo in new-name silently fails.
• Fix: Add test validating every SHADOW_RENAMES[1] value exists in getAllSkillNames().

9. Preamble hook references devflow:router by convention, not contract — scripts/hooks/preamble:40-44

• Confidence: 82% (Architecture)
• Issue: Preamble tells Claude to "Load devflow:router skill FIRST" but no runtime validation that skill is installed. If router is renamed again, preamble becomes dead-end with no fallback.
• Fix: Add explicit SYNC comment in both preamble and router SKILL.md to document the coupling.

10. Resolver agent gains TDD skill without test coverage — shared/agents/resolver.md:5

• Confidence: 82% (Regression)
• Issue: Resolver frontmatter adds new devflow:test-driven-development skill (not just rename). This changes resolver behavior to insist on regression tests before fixes. No tests validate this behavior.
• Fix: Add test in tests/skill-references.test.ts confirming Resolver includes TDD skill, or accept as deliberate enhancement (TDD in Resolver = quality improvement).

---

Report Summary

Category	Count	Highest Confidence
BLOCKING	6	95% (skill count)
SHOULD-FIX	4	82% (multiple)
Total ≥80%	10	—

Flagged by: 9 reviewers (Architecture, Testing, Performance, Regression, Documentation, TypeScript, Security, Complexity, Consistency)

---

Generated by DevFlow Code Review Agent

[dean0x]

Action Items & Priority

Immediate (HIGH priority - blocks merge)

• Update skill count '38' → '39' in CLAUDE.md, README.md (2x), file-organization.md
• Add explore skill to skills-architecture.md Tier 1 table
• Fix dual JSON.parse in addAmbientHook (refactor with shared filterHookEntries helper)
• Fix code duplication in removal functions (use shared helper)

Follow-up (SHOULD-FIX - architectural improvements)

• Clear orphaned grace timer in runClaudeStreaming
• Remove dead isFirstToolASkill function
• Restructure LEGACY_SKILL_NAMES array (group by version/era)
• Add validation test for SHADOW_RENAMES entries
• Add SYNC comment documenting preamble↔router coupling
• Add test coverage for Resolver+TDD skill integration

Code Review Statistics

• Review Reports Analyzed: 9 (Architecture, Testing, Performance, Regression, Documentation, TypeScript, Security, Complexity, Consistency)
• Total Findings: 10 (≥80% confidence)
• Multi-Reviewer Consensus: 3 issues flagged by 2+ reviewers
• Rate Limiting: None encountered

See comments above for full context, confidence levels, and suggested fixes for each issue.