refactor(ambient): remove redundant commands-awareness rule

[dean0x]

Summary

• The commands.md rule listed every /devflow:<name> command and restated the plan auto-execution trigger, both already available live via the session's available-skills list and the preamble hook directive at runtime.
• Ambient mode is now a single-component system: only the preamble UserPromptSubmit hook.
• Existing installs are cleaned up automatically — removeLegacyCommandsRule runs unconditionally before the early-return in both addAmbientHook and removeAmbientHook, so every devflow ambient --enable/--disable or devflow init purges any stale commands.md. No migration needed (applies ADR-001, avoids PF-001).

Changes

• Deleted: shared/rules/commands.md
• src/cli/commands/ambient.ts: Removed COMMANDS_RULE_CONTENT, installCommandsRule; renamed removeCommandsRule → removeLegacyCommandsRule; updated addAmbientHook and removeAmbientHook to call removeLegacyCommandsRule unconditionally before their early-return guards; updated command descriptions and log messages
• tests/ambient.test.ts: Pruned dead installCommandsRule and COMMANDS_RULE_CONTENT describe blocks; renamed removeCommandsRule suite; added ordering-invariant test proving the legacy rule is purged even on the early-return path; updated stubs from mkdir+writeFile to unlink
• Docs sweep: CLAUDE.md, README.md, docs/commands.md, docs/cli-reference.md, plugins/devflow-ambient/README.md and plugin.json, src/cli/plugins.ts, src/cli/commands/init.ts, .devflow/features/cli-rules/KNOWLEDGE.md

Breaking Changes

None. addAmbientHook and removeAmbientHook retain their existing signatures and return types.

Reviewer Focus Areas

• Cleanup ordering invariant: removeLegacyCommandsRule must run before the if (!changed) return settingsJson early-return in addAmbientHook, and before if (!removedPrompt && !removedClassification) return settingsJson in removeAmbientHook. Verify both call sites respect this.
• The new test purges legacy rule even when preamble hook already present guards this invariant.
• npm run build shows Found 12 rules in shared/rules/ (was 13). npm test passes 1521 tests.

[dean0x]

Finding 1: RELIABILITY (HIGH) — src/cli/commands/ambient.ts:67

Non-fatal cleanup of deprecated files

The current implementation propagates non-ENOENT errors (EACCES, EPERM, etc.) from removeLegacyCommandsRule(), which can silently abort the entire devflow init settings-configuration pass or crash devflow ambient --enable/--disable.

A best-effort cleanup of a deprecated artifact should never be fatal — it must not block the primary operation. Recommended fix:

export async function removeLegacyCommandsRule(): Promise<void> {
  try {
    await fs.unlink(COMMANDS_RULE_PATH);
  } catch (err) {
    const code = (err as NodeJS.ErrnoException).code;
    if (code === 'ENOENT') return; // already gone — success
    // Best-effort cleanup of a deprecated file: never let it abort the
    // caller's primary operation (enable/disable/init). Log and continue.
  }
}

This ensures the legacy file is purged whenever possible, but file system errors (permission denied, read-only FS, etc.) don't regress previously-working functionality.

---

Review feedback from devflow:code-review | 8 reviewers APPROVED

[dean0x]

Finding 2: CONSISTENCY + DOCUMENTATION (HIGH) — README.md:56

Docs sweep straggler: rule count should be 12

The PR swept rule-count references everywhere except README.md:56, which still reads: **Always-on rules.** 13 ultra-condensed engineering principles (~10 lines each).... Ground truth is 12 rules (shared/rules/*.md = 12 files post-deletion), and the sibling docs (CLAUDE.md:65, cli-rules KNOWLEDGE.md:30, Project Structure tree) all now correctly say "12".

Fix: Update README.md:56 from 13 ultra-condensed engineering principles → 12 ultra-condensed engineering principles.

---

Review feedback from devflow:code-review | Consistency + Documentation reviewers (96% confidence)

[dean0x]

Finding 3: DOCUMENTATION (MEDIUM) — .devflow/features/cli-rules/KNOWLEDGE.md:241

Inaccurate PF citation in cli-rules KNOWLEDGE.md

New gotcha line states: "The workflow-bucket predicate is `commands.length > 0` — the language-bucket comment notes this implicit contract is PF-007".

Two inaccuracies: (1) the source comment in src/cli/plugins.ts:732-735 makes no PF reference — it just describes the contract in plain English; (2) PF-007 in pitfalls.md:60 is "Editing globally installed hook scripts directly" — an unrelated topic. The citation misleads a future reader to an unrelated pitfall.

Fix: Either drop the "is PF-007" claim and reference the plugins.ts comment directly, or verify you're citing the correct PF number. Also confirm the parenthetical description matches plugins.ts actual content.

---

Review feedback from devflow:code-review | Documentation reviewer (90% confidence)

[dean0x]

Summary: PR #233 Code Review

Overall Verdict: 8 reviewers APPROVED with 3 blocking findings above. The refactor is architecturally sound and well-executed — the three findings are concrete, fixable issues (one reliability regression, two documentation stragglers).

Review Coverage

• Security, Architecture, Performance, Complexity, Consistency, Regression, Testing, Reliability, TypeScript, Documentation — all eight pillars reviewed in parallel
• Confidence weighted: 80%+ findings surfaced as inline comments above; 60-79% suggestions consolidated here

Sub-Threshold Findings (60-79% confidence)

Cleanup ordering vs. write atomicity (Reliability, 65% confidence)

• ambient.ts lines 103, 124: The legacy rule purge runs before the settings write. On --enable you delete the rule and may still fail to write settings.json. The two side effects are not atomic. Low impact since the rule is deprecated, but worth documenting in a comment that purge is irreversible before primary write succeeds.

Version-mark the legacy purge for eventual deletion (Code hygiene, 62% confidence)

• Consider adding a version marker comment (e.g. // TODO(v3.x): remove entire removeLegacyCommandsRule after sunset period) so future maintainers know this entire function is ephemeral cleanup. The CLAUDE.md context documents it as one-time cleanup (ADR-001), so the marker signals "this is temporary, safe to delete in a major version bump."

Anachronistic parenthetical in init.ts (Documentation, 61% confidence)

• init.ts:1088 has a comment parenthetical that predates the two-step plugin selection feature. Low blocking concern (comments drift with refactors), but consider auditing pre/post-refactor comments during manual acceptance testing.

Deduplication

The README "13" count issue was raised independently by both Consistency and Documentation reviewers (96% + 98% confidence) — posted once as Finding 2 above.

Key Strengths

• Excellent naming consistency (removeLegacyCommandsRule aligns with established patterns)
• Clean, bounded retry logic for the plugin selection loop
• Decisions alignment: purge sanctioned by ADR-001, avoids compat-scaffolding (PF-001)
• Comprehensive docs sweep (CLAUDE.md, plugin.json, cli-reference, KNOWLEDGE.md all updated)

---

Next Steps:

1. Fix the three blocking findings (reliability + docs stragglers)
2. Add test coverage for the EACCES/ENOENT error branches
3. Address sub-threshold suggestions at your discretion
4. Re-run reviews to confirm convergence

All review reports available in .devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/

[dean0x]

Testing: Missing mirror test for removeAmbientHook early-return path (90% confidence)

File: tests/ambient.test.ts:251

The new ordering-invariant test (line 94-104) proves addAmbientHook purges the legacy rule on its early-return path. But removeAmbientHook has identical structure — it calls removeLegacyCommandsRule() at ambient.ts:128 before its early-return at ambient.ts:130. No test asserts this purge runs on that path.

The closest test, 'is idempotent — safe to call when not present' (line 251), passes a Stop-only settings object that triggers the early-return, but only asserts result === input — never asserts the purge ran.

Drop-in fix: Add a mirror test to the removeAmbientHook suite:

```typescript
it('purges legacy rule even when nothing to remove (ordering invariant)', async () => {
// No ambient/classification hooks → removeAmbientHook takes the early-return path.
// removeLegacyCommandsRule MUST still run so stale commands.md files are cleaned up.
const input = JSON.stringify({ hooks: { Stop: [{ hooks: [{ type: 'command', command: 'stop.sh' }] }] } });
const result = await removeAmbientHook(input);
expect(result).toBe(input); // early-return preserved
expect(fs.unlink).toHaveBeenCalledWith(COMMANDS_RULE_PATH); // purge still ran
});
```

(The suite's beforeEach already stubs fs.unlink, so no extra setup is needed.)

---

Inline comment by Claude Code (devflow:git agent)

[dean0x]

Consistency: Stale shared/rules/commands.md in feature knowledge index (90% confidence)

File: .devflow/features/index.json (line 26)

This PR deletes shared/rules/commands.md, but the committed cli-rules knowledge-base index entry still lists "shared/rules/commands.md" in its referencedFiles array (line 26). The description keyword list (line 6) also still includes commands.md.

The companion KNOWLEDGE.md was updated correctly, so the index and KNOWLEDGE.md are now out of sync with each other and with the source tree.

Impact: referencedFiles drives staleness detection via git log against those paths. A deleted path can never produce a meaningful diff, so the entry is dead. More importantly, this is an internal inconsistency — every other doc artifact in the sweep was updated, but this metadata file was missed.

Fix: In .devflow/features/index.json:

1. Remove "shared/rules/commands.md" from the cli-rules.referencedFiles array
2. Drop commands.md from the description keyword list (replace with removeLegacyCommandsRule to match KNOWLEDGE.md)

---

Inline comment by Claude Code (devflow:git agent)

[dean0x]

PR Review Summary — 2026-06-02 Review Cycle 2

All 10 reviewers: APPROVED or APPROVED_WITH_CONDITIONS
Blocking issues: 0
Should-fix issues: 2 (both at ≥80% confidence, addressed as inline comments above)

---

Overall Verdict: APPROVED WITH CONDITIONS

The refactor is architecturally clean, technically correct, and well-tested. The two-component collapse (preamble hook + commands.md rule → preamble hook only) eliminates redundancy and improves separation of concerns. The cleanup-ordering invariant is correctly implemented across all three entry points (enable, disable, init).

Conditions for merge:

1. Add the missing symmetric test for removeAmbientHook's early-return purge path (testing.md, 90% confidence)
2. Remove stale shared/rules/commands.md entry from .devflow/features/index.json:26 referencedFiles and keyword list (consistency.md, 90% confidence)

---

Key Verification Highlights

✅ Cleanup-ordering invariant: Verified that removeLegacyCommandsRule() runs before both early-return guards in addAmbientHook (L107 before L109) and removeAmbientHook (L128 before L130)
✅ Build: npm run build reports 12 rules (was 13)
✅ Tests: 40/40 ambient tests pass; 1521 total tests pass
✅ No dead references: grep sweep confirms COMMANDS_RULE_CONTENT, installCommandsRule, commands-awareness, two-component all fully removed
✅ Cycle-1 fixes hold: Fail-safe error handling, README rule count, fabricated citation — all verified
✅ Type-clean: npx tsc --noEmit exit 0; strict mode clean
✅ Documentation sweep complete: CLAUDE.md, README, plugin metadata, KNOWLEDGE.md all consistent

---

Lower-Confidence Suggestions (60–79%, summary only)

These are informational observations that do not block merge:

1. KNOWLEDGE.md:199 phrasing clarity (documentation.md, 65% confidence)
   — The init-flow section mentions stale rules are cleaned "via LEGACY_RULE_NAMES loop," which could momentarily suggest the deleted commands-rule cleanup flows through that loop. The doc is internally consistent (other sections clarify the split), but optional: add a parenthetical clarifying that the commands-rule is handled separately by removeLegacyCommandsRule.

2. Architecture: legacy-cleanup placement (architecture.md, 65% confidence)
   — The codebase has a migration registry (migrations.ts) for one-time legacy purges, whereas this cleanup lives inline in ambient.ts. This is defensible — it matches the module's existing inline cleanup convention and runs more frequently (better purge guarantees). Noted only as a layering observation; no action required.

3. TypeScript: parameterless catch {} loses diagnostic context (typescript.md, 62% confidence)
   — The error object is intentionally discarded for fail-safe cleanup. Only worth future revisit if you need debug-trace visibility into unexpected unlink failures.

4. Testing: ordering-invariant test re-stubs mid-test (testing.md, 82% confidence)
   — The new test calls vi.restoreAllMocks() then re-stubs fs.unlink to assert the second call. This works correctly but couples the test to the first call's existence. Optional refactor: capture call counts before/after rather than restore/re-stub, or use vi.clearAllMocks() to preserve the stub.

---

Pre-Existing Issues (Not Raised)

One pre-existing pattern noted in TypeScript review (88% confidence, non-critical):

• Settings object mutated in place (ambient.ts:24–132) — The global "Immutable by default" principle is violated, but the mutation is contained to a parse-local object with no aliasing. This is pre-existing (PR only renames/deletes, doesn't introduce new mutation). Fix is optional and low-value relative to churn.

---

Summary generated by Claude Code (devflow:git agent)