feat: simplify ambient mode to plan detection + command awareness

[dean0x]

Summary

Implements ambient mode for Devflow — a zero-overhead session enhancement system with plan auto-detection and command awareness. Ambient mode consists of two independent, toggleable components:

1. Plan Auto-Detection: UserPromptSubmit hook detects structured implementation plans (marked with ## Goal, ## Steps, ## Files) and emits a directive to invoke devflow:implement via the Skill tool.
2. Command Awareness: Always-on rule installed to ~/.claude/rules/devflow/commands.md documents available /devflow:<name> commands and the auto-execution trigger.

Key Features

• Zero Overhead: Plan detection hook only outputs when a plan is detected; normal prompts are unaffected
• Toggleable: Enable/disable via devflow ambient --enable/--disable or devflow init
• Independent Components: Can be configured separately
• Passive Reference: Commands rule provides documentation without changing behavior

Implementation Details

• Ambient mode state stored in manifest features.ambient: boolean
• Hook implementation via preamble hook for plan detection
• Commands rule managed directly by ambient.ts command (not through rules plugin system)
• SessionStart hook injects plan marker context if plan is detected in current session
• Feature toggleable separately from other Devflow systems (memory, learning, decisions, knowledge, rules)

Testing

Added comprehensive tests covering:

• Hook addition/removal lifecycle
• Rule file creation with stable content hash
• Ambient mode enable/disable transitions
• Edge cases (already enabled, already disabled, invalid states)
• Cleanup of stale hooks/rules

Closes #220

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

[dean0x]

src/cli/commands/ambient.ts:26 — Dual-ownership of commands rule content (90% confidence)

The COMMANDS_RULE_CONTENT string literal duplicates shared/rules/commands.md. This creates a maintenance hazard — any future edit to the command list must be made in two places. The PR description notes this is managed by ambient.ts directly, but keeping identical content in two locations risks drift.

Fix: Read the rule content from the shared source at build time instead of embedding as a constant, OR remove shared/rules/commands.md entirely if ambient.ts is the sole source of truth.

[dean0x]

src/cli/commands/ambient.ts:95 — addAmbientHook mixes I/O with JSON transformation (85% confidence)

This function now performs filesystem operations (fs.mkdir, fs.writeFile) as side effects while also transforming settings JSON. This violates separation of concerns — the function name suggests it manages hooks in settings, but it also manages files on disk. Callers don't expect I/O side effects from what appears to be a pure JSON transformation.

Fix: Extract rule file management into a separate function that callers invoke explicitly:

const updated = addAmbientHook(settingsJson, devflowDir); // pure JSON
await writeCommandsRule(); // explicit side effect

[dean0x]

src/cli/commands/ambient.ts:141 — removeAmbientHook drops stale hook cleanup (92% confidence - BLOCKING)

Line 145 calls filterHookEntries(settings, 'SessionStart', isClassification) to clean stale hooks, but the return value is discarded. If a user has ONLY a stale classification hook (no UserPromptSubmit), the mutation is lost because line 154 only checks removedPrompt.

Fix: Capture and check both:

const removedClassification = filterHookEntries(settings, 'SessionStart', isClassification);
if (!removedPrompt && !removedClassification) return settingsJson;

[dean0x]

src/cli/commands/ambient.ts:148 — Error swallowing in removeAmbientHook (82% confidence)

The catch block swallows ALL fs.unlink errors, not just ENOENT. Permission errors are hidden, leaving stale rule files active while the user believes ambient mode is disabled.

Fix: Only swallow ENOENT:

try {
  await fs.unlink(COMMANDS_RULE_PATH);
} catch (err: unknown) {
  if ((err as NodeJS.ErrnoException).code !== 'ENOENT') {
    throw err;
  }
}

[dean0x]

src/cli/plugins.ts — LEGACY_SKILL_NAMES missing namespaced entries (95% confidence - BLOCKING)

17 skill directories were deleted, including 15 with devflow: namespaces. Existing users who run devflow init will retain these stale directories because LEGACY_SKILL_NAMES lacks the namespaced entries.

Add to LEGACY_SKILL_NAMES:

// v2.x ambient simplification: deleted router, triage, and guided skills
'devflow:router',
'devflow:implement:triage', 'devflow:implement:guided',
'devflow:debug:triage', 'devflow:debug:guided',
'devflow:explore:triage', 'devflow:explore:guided',
'devflow:plan:triage', 'devflow:plan:guided',
'devflow:review:triage', 'devflow:review:guided',
'devflow:research:triage', 'devflow:research:guided',
'devflow:release:triage', 'devflow:release:guided',

[dean0x]

shared/rules/commands.md:24 — False documentation claim (92% confidence)

The file claims detection fires only on "the first message in a session", but scripts/hooks/preamble checks every prompt with no first-message guard. The CLAUDE.md description is accurate ("when a prompt contains"), making this documentation actively misleading.

Fix: Change "When the first message in a session is..." to "When a prompt is..."

[dean0x]

tests/ambient.test.ts:17-140 — Unit tests write real files (90% confidence - BLOCKING)

addAmbientHook and removeAmbientHook now call fs.mkdir, fs.writeFile, and fs.unlink on the real filesystem during unit tests. This violates test isolation — tests must not produce side effects on the developer's home directory.

Fix: Mock filesystem operations:

beforeEach(() => {
  vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined);
  vi.spyOn(fs, 'writeFile').mockResolvedValue();
  vi.spyOn(fs, 'unlink').mockResolvedValue();
});

afterEach(() => {
  vi.restoreAllMocks();
});

[dean0x]

PR #227 Review Summary

Review Timestamp: 2026-05-25_2233
High-Confidence Findings: 7 (≥80%)
Created: 7 inline comments above
Files Reviewed: 10

Lower-Confidence Issues (60-79%) — Summary Only

• Architecture: Missing function to distinguish hasAmbientHook and commands rule presence (82% MEDIUM)
• Documentation: README rule count not updated (88% HIGH) — says "12" but should be "13"
• Documentation: README "See it work" example implies auto-execution without classification pipeline (85% MEDIUM)
• Regression: orphaned shared/rules/commands.md from build system (85% MEDIUM)
• Performance: Unconditional file writes in addAmbientHook (70% MEDIUM)
• Testing: Missing stale classification hook edge case test (82% MEDIUM)
• Testing: Integration test bypasses actual preamble hook with hardcoded systemPrompt (80% MEDIUM)
• Testing: Preamble hook pattern-matching has no direct unit test coverage (85% MEDIUM)
• Security: Plan marker detection could match unintended content (65% LOW)

Recommendation

CHANGES_REQUESTED — Fix the 7 blocking issues (marked above) before merge. The PR achieves a significant and well-motivated architectural simplification, but the dual-source rule content, function side-effect mixing, stale hook cleanup bug, and missing legacy cleanup entries must be resolved for production quality.

---

Review conducted by devflow:git agent with 90% minimum inline comment confidence threshold. See .devflow/docs/reviews/feat-ambient-mode/2026-05-25_2233/ for full detail.

[dean0x]

Code Review — High-Confidence Findings (≥80%)

Inline Comments Posted Below

Summary: 4 high-confidence issues identified:

• 2 security concerns (pre-existing broad catch blocks)
• 1 architecture concern (dual-source content duplication)
• 1 blocking test gap (new exported helpers lack direct tests)

All findings are detailed as inline comments on the affected lines. Pre-existing issues are marked as such; the testing gap is a blocking issue for this PR.

[dean0x]

Inline Comments: High-Confidence Findings (≥80%)

Below are the high-confidence blocking and should-fix issues identified in the review:

1. Testing Gap (BLOCKING) - src/cli/commands/ambient.ts:93-109

Missing direct tests for extracted helper functions (Confidence: 82%)

The new exported functions installCommandsRule() and removeCommandsRule() are tested only indirectly via addAmbientHook/removeAmbientHook with fs mocked. There are no direct unit tests for:

• ENOENT handling in removeCommandsRule() (line 107) — should swallow, not re-throw
• Non-ENOENT errors in removeCommandsRule() — should re-throw
• mkdir + writeFile flow in installCommandsRule()

Suggestion: Add describe blocks with direct mocks for each helper.

---

2. Security (Should-Fix) - src/cli/commands/ambient.ts:217

Broad catch swallows all errors in settings.json read (Confidence: 82%, Pre-existing)

The catch block catches all errors (ENOENT, EACCES, etc.). If settings.json exists with permission issues or is corrupted, the code silently creates '{}' and overwrites the file, potentially destroying configuration.

This mirrors the issue you correctly fixed in removeCommandsRule() at line 107. For consistency, narrow the catch to ENOENT only.

---

3. Security (Should-Fix) - src/cli/commands/ambient.ts:244

Broad catch at JSON.parse (Confidence: 80%, Pre-existing)

The catch swallows any error from JSON.parse(settingsContent). Silent error swallowing could mask filesystem permission issues.

Suggestion: Add a comment explaining the fallback is safe, or narrow to specific error types.

---

4. Architecture (Should-Fix) - src/cli/commands/ambient.ts:26-52

Dual-source content duplication (Confidence: 82%)

The COMMANDS_RULE_CONTENT constant is a verbatim copy of shared/rules/commands.md. Although the new sync test (lines 383–389) guards against drift, there are now two sources of truth for the same content.

The sync test is a good stopgap, but architectural direction should be single-source. Consider a build-time approach or runtime file read.

---

Summary

Category	Count
Blocking Tests	1
Should-Fix (Pre-existing Security)	2
Should-Fix (Architecture)	1

Recommendation: Address the test gap before merge. The others improve the codebase.

---

Review completed by devflow:git — High-confidence findings (≥80%) only