diff --git a/.devflow/decisions/decisions.md b/.devflow/decisions/decisions.md index 6c872858..d21c724f 100644 --- a/.devflow/decisions/decisions.md +++ b/.devflow/decisions/decisions.md @@ -1,4 +1,4 @@ - + # Architectural Decisions Append-only. Status changes allowed; deletions prohibited. @@ -65,3 +65,39 @@ Append-only. Status changes allowed; deletions prohibited. - **Decision**: Implement a single global `DEVFLOW_HOOK_DEBUG=1` env var toggle exposed as `devflow debug --enable/--disable/--status`, covering ALL hooks via a shared `scripts/hooks/debug-trace` helper script. Stored in `~/.claude/settings.json` env block so it survives reinstalls. - **Consequences**: When debugging any hook issue, all hooks emit traces simultaneously — enabling cross-hook interaction visibility. Per-feature toggles would require enabling multiple flags and could miss interactions between hooks (e.g., sidecar-capture writing a queue entry that sidecar-dispatch reads). The shared helper means debug tracing is consistent across all hooks and can be updated in one place. - **Source**: self-learning:obs_h9bw3c + +## ADR-008: LLM-vs-plumbing principle: artifact content must be LLM-authored — deterministic scripts must not write memory, observations, ADR/PF bodies, or knowledge bases + +- **Date**: 2026-06-01 +- **Status**: Accepted +- **Context**: investigation found dead deterministic promotion code (process-observations, calculateConfidence, tryImmediatePromotion) that was writing artifact files via threshold calculations — contradicting the system design +- **Decision**: all artifact content (working memory, learning observations, ADR/PF bodies, knowledge bases) MUST be authored by an LLM agent +- **Consequences**: artifact quality requires semantic intelligence that deterministic thresholds cannot provide +- **Source**: self-learning:obs_7xk9qm + +## ADR-009: Sidecar processor must be spawned at SessionStart — not via additionalContext injection — because soft context directives are unreliably acted upon when a user task is present + +- **Date**: 2026-06-01 +- **Status**: Accepted +- **Context**: original sidecar design injected SIDECAR directives via additionalContext (UserPromptSubmit hook) and relied on the model to spawn a background processor +- **Decision**: move processor spawning entirely to SessionStart (session-start-context hook) — a clean hook event where no competing user task is present +- **Consequences**: SessionStart fires before any user turn is visible to the model — there is no competing user request, so the spawn directive receives full attention +- **Source**: self-learning:obs_p3r8wn + +## ADR-010: Interactive devflow init always installs on user scope — interactive scope prompt removed, --scope flag retained + +- **Date**: 2026-06-01 +- **Status**: Accepted +- **Context**: devflow init interactively prompted user vs local/project install scope, adding unwanted friction since user scope is the intended default for interactive installs +- **Decision**: remove the interactive Installation scope prompt and hardcode interactive scope to user, while keeping the --scope CLI flag and non-TTY auto-detection unchanged so scripted and local installs (--scope local) continue to work +- **Consequences**: interactive users effectively always want user scope (~/.claude) so the prompt was noise +- **Source**: self-learning:obs_scopeu1 + +## ADR-011: Interactive plugin selection split into two sequential multiselects (workflow then language plugins); custom grid rejected + +- **Date**: 2026-06-01 +- **Status**: Accepted +- **Context**: the single interactive plugin multiselect conflated workflow/command plugins (plan, implement, code-review) with language/ecosystem plugins (typescript, react, go), making selection unclear +- **Decision**: present interactive plugin selection as two sequential @clack multiselects — Step 1 workflow plugins, Step 2 language plugins — partitioned by a pure partitionSelectablePlugins helper +- **Consequences**: clearer mental model and discoverability +- **Source**: self-learning:obs_plug2st diff --git a/.devflow/decisions/pitfalls.md b/.devflow/decisions/pitfalls.md index aedce55b..5d7f5eb1 100644 --- a/.devflow/decisions/pitfalls.md +++ b/.devflow/decisions/pitfalls.md @@ -1,4 +1,4 @@ - + # Known Pitfalls Area-specific gotchas, fragile areas, and past bugs. @@ -65,3 +65,12 @@ Area-specific gotchas, fragile areas, and past bugs. - **Resolution**: Always edit source files (`scripts/hooks/`), run `npm run build`, then run `devflow init` to reinstall. Never directly edit installed copies at `~/.devflow/scripts/` or `~/.claude/`. The same rule applies to any other installed artifact (commands, agents, skills). - **Status**: Active - **Source**: self-learning:obs_n4rs8t + +## PF-008: Using additionalContext for critical maintenance directives — models deprioritize soft context when competing with an active user task, causing markers to silently accumulate + +- **Area**: sidecar consumption architecture, Claude Code hook additionalContext +- **Issue**: injecting critical background directives via additionalContext (system-reminder) relies on the model to act on them when a user question is also present — in practice the model almost always prioritizes answering the user, leaving maintenance markers unprocessed +- **Impact**: markers accumulated for weeks across all projects (alefy, autobeat, devflow) with no errors surfaced — purely silent backlog growth +- **Resolution**: anchor critical directives to hook events where no user task competes (SessionStart is the correct hook) +- **Status**: Active +- **Source**: self-learning:obs_m5v2xt diff --git a/.devflow/docs/design/init-flow-simplification.2026-06-01_1345.md b/.devflow/docs/design/init-flow-simplification.2026-06-01_1345.md new file mode 100644 index 00000000..c88a105f --- /dev/null +++ b/.devflow/docs/design/init-flow-simplification.2026-06-01_1345.md @@ -0,0 +1,191 @@ +--- +type: design-artifact +version: 1 +status: APPROVED +issue: null +title: "Init flow simplification: user-scope-only, bug-analysis listing, two-step plugin selection" +slug: init-flow-simplification +created: 2026-06-01T13:45:00Z +execution-strategy: SINGLE_CODER +context-risk: LOW +--- + +# Init Flow Simplification + +## 1. Problem Statement + +Three rough edges in `devflow init` (`src/cli/commands/init.ts`): + +1. **Scope prompt is unwanted.** Init interactively asks whether to install on user + scope (`~/.claude/`) or local/project scope (`./.claude/`). Going forward, interactive + init should always install on user scope — no project-scope prompt. +2. **`/bug-analysis` is missing from the end-of-init command list.** When the + bug-analysis feature shipped, its command was never added to the `WORKFLOW_ORDER` + list printed at the end of init, so users never see it advertised. +3. **Plugin selection mixes two kinds of plugins in one list.** The single plugin + multiselect mixes workflow/command plugins (`plan`, `implement`, `code-review`, …) + with language/ecosystem plugins (`typescript`, `react`, `go`, …). Splitting this into + two sequential steps is clearer. + +**Target users:** Anyone running `devflow init` interactively (first-time install and re-init). + +## 2. Acceptance Criteria + +- AC1: Interactive `devflow init` no longer shows the "Installation scope" prompt and + always installs on user scope. +- AC2: The `--scope` CLI flag and non-TTY auto-detection (both already → `user` by + default) continue to work unchanged; `--scope local` still functions for scripted use. +- AC3: The end-of-init "Available commands" note includes `/bug-analysis` (when the + bug-analysis plugin is installed). +- AC4: A regression test fails if any selectable command-bearing plugin's command is + absent from `WORKFLOW_ORDER`. +- AC5: Interactive plugin selection is presented as two sequential multiselects: + Step 1 = workflow/command plugins, Step 2 = language/ecosystem plugins. +- AC6: Both steps are optional individually, but the combined selection must be non-empty; + if empty, the user is re-prompted (bounded) rather than silently installing nothing. + This permits a language-only interactive install (zero workflow plugins). +- AC7: The `--plugin=` non-interactive path is unchanged (single combined parse). +- AC8: `partitionSelectablePlugins` correctly buckets plugins and applies the existing + exclusions (`devflow-core-skills`, `devflow-ambient`, `devflow-audit-claude`). +- AC9: `npm run build` and `npm test` pass. + +## 3. Scope + +**v1 (included):** +- Remove interactive scope prompt; hardcode interactive scope to `user`. +- Add `/bug-analysis` to `WORKFLOW_ORDER`; extract `WORKFLOW_ORDER` to `plugins.ts`. +- Split plugin multiselect into two steps with combined non-empty validation. +- Pure helper `partitionSelectablePlugins`; short hints for `explore`/`research`/`release`/`bug-analysis`. +- Two new unit tests (partition + WORKFLOW_ORDER coverage). + +**Deferred (explicitly later):** +- Deeper scope removal — stripping the `local` branch from `uninstall.ts` and `paths.ts`, + removing the `--scope` flag entirely. (User: "later, we will go into a deeper removal.") + +**Excluded:** +- Custom 2-column grid multiselect rendering (rejected — `@clack` multiselect is + single-column and shows the description only for the focused row; a grid would require + a custom prompt component, not worth it). +- Any change to universal skill installation (skills remain installed from all plugins). + +## 4. Gap Analysis Results + +- **Language-only interactive install (resolved):** Requiring ≥1 workflow plugin would + block installing only language plugins. Resolution (AC6): both steps optional + combined + non-empty validation with a bounded re-prompt. Skills install universally regardless, and + language plugins still contribute plugin-scoped rules, so a language-only install is meaningful. +- **`WORKFLOW_ORDER` was local to the init function (root cause of the `/bug-analysis` + omission):** No test could see it. Resolution: extract to an exported const + coverage test (AC4). +- **`required: true` cancel semantics:** The removed scope `p.select` had `isCancel` + handling; each new multiselect must keep its own `isCancel` handling. +- **Reliability:** The re-prompt loop must have a fixed upper bound (project reliability + rule — no unbounded loops). Bound = 3 attempts, then graceful cancel. + +## 5. Execution Strategy + +**SINGLE_CODER.** One file is the primary surface (`init.ts`); `plugins.ts` gains two +small pure exports; `tests/plugins.test.ts` gains two tests. Changes are interdependent +and small — no benefit to parallel or sequential coders. + +## 6. Implementation Plan + +### Step 1 — `plugins.ts`: extract `WORKFLOW_ORDER` + add `partitionSelectablePlugins` +- Export `WORKFLOW_ORDER` (the ordered command list) from `plugins.ts`, inserting + `'/bug-analysis'` after `'/self-review'`: + `['/research','/explore','/plan','/implement','/code-review','/resolve','/self-review','/bug-analysis','/debug','/release','/audit-claude']` +- Add pure helper: + ```ts + export function partitionSelectablePlugins(plugins: PluginDefinition[]): { + workflow: PluginDefinition[]; + language: PluginDefinition[]; + } + ``` + - Exclude `devflow-core-skills`, `devflow-ambient`, `devflow-audit-claude` (current init exclusions). + - `workflow` = remaining with `commands.length > 0`; `language` = remaining with `commands.length === 0`. + +### Step 2 — `init.ts`: remove the interactive scope prompt +- File: `src/cli/commands/init.ts` ~200-213. +- Delete the trailing `else { … p.select('Installation scope') … }` block and its + `isCancel` handling. `scope` stays `'user'` by default (line 184); `options.scope` + (189-195) and `!process.stdin.isTTY` (196-198) branches are untouched. + +### Step 3 — `init.ts`: two-step plugin selection (interactive branch ~296-339) +- Import `partitionSelectablePlugins` from `plugins.ts`. +- Build `{ workflow, language }`. For each, map to `{ value, label: name.replace('devflow-',''), hint }` + using the existing `pluginHints` map (extended with `explore`, `research`, `release`, `bug-analysis`). +- `preSelected` for Step 1 = non-optional workflow plugins (current logic). Step 2 preselect = none. +- Bounded selection loop (max 3 attempts): + - `p.multiselect({ message: 'Step 1 — Workflow plugins', options: workflowChoices, initialValues: preSelected, required: false })` + - `p.multiselect({ message: 'Step 2 — Language plugins', options: languageChoices, required: false })` + - Handle `isCancel` on each (cancel → exit 0, as today). + - Merge into `selectedPlugins`. If non-empty → break. If empty → `p.log.warn('Select at least one plugin.')` and retry. + - After 3 empty attempts → `p.cancel('Installation cancelled — no plugins selected.')`, exit 0. +- `--plugin` path (287-295) unchanged. + +### Step 4 — `init.ts`: use exported `WORKFLOW_ORDER` +- Replace the local `WORKFLOW_ORDER` const (~1222) with the import from `plugins.ts`. + Existing filtering/printing logic (1227-1234) is unchanged. + +### Step 5 — Tests (`tests/plugins.test.ts`) +- `partitionSelectablePlugins`: workflow bucket contains command plugins (e.g. `devflow-plan`, + `devflow-bug-analysis`); language bucket contains `devflow-typescript` etc.; excluded plugins + appear in neither bucket. +- `WORKFLOW_ORDER` coverage: for every selectable command-bearing plugin (workflow bucket), + each of its `commands` is present in `WORKFLOW_ORDER`. (This test would have caught the + original `/bug-analysis` omission.) + +## 7. Patterns to Follow + +- Pure, exported helpers in `plugins.ts` mirroring `buildAssetMaps` / `buildFullSkillsMap` + (`src/cli/plugins.ts:604-640`) — keep detection/partition logic pure and unit-tested. +- `@clack/prompts` multiselect usage as in current `init.ts:327` (`message`, `options`, + `initialValues`, `required`, `isCancel` guard). +- Test style: `tests/plugins.test.ts` `describe`/`it` over `DEVFLOW_PLUGINS` and pure helpers. + +## 8. Integration Points + +- `src/cli/commands/init.ts` — interactive scope branch (~184-214), interactive plugin + selection (~296-339), end-of-init command note (~1222-1234). +- `src/cli/plugins.ts` — new `WORKFLOW_ORDER` export + `partitionSelectablePlugins`. +- `tests/plugins.test.ts` — two new tests. +- Unchanged but verified: `--scope` flag (init.ts:145/189), non-TTY branch (196), + `parsePluginSelection` (`--plugin` path), `manifest.scope` persistence, `uninstall.ts`. + +## 9. Design Review Results + +- **No over-engineering:** No custom prompt component; reuse stock `@clack` multiselect. +- **No hidden no-op:** Label-padding alignment was rejected because `@clack` shows the + description only for the focused row — padding would have no visible effect. +- **Bounded loop:** Re-prompt loop has a fixed cap (3) per the reliability rule. +- **Root-cause fix:** `/bug-analysis` omission is fixed *and* guarded by a coverage test, + not just patched once. + +## 10. Risk Assessment + +**Context risk: LOW.** +- Interactive-only UI changes; `--plugin` and `--scope` non-interactive paths untouched. +- Behavior change: interactive init can no longer choose local scope (intended). `--scope local` remains. +- Behavior change: plugin selection is two prompts instead of one (intended). +- Unresolved risks: none. Deferred work (deep scope removal) is tracked in Scope §3. + +## 11. PR Description Guidance + +### Problem Being Solved +Three init-flow rough edges: an unwanted user/local scope prompt, a missing `/bug-analysis` +entry in the post-install command list, and a single plugin list that conflated workflow and +language plugins. + +### Key Changes to Highlight +- Interactive init always installs on user scope (scope prompt removed; `--scope` flag kept). +- `/bug-analysis` now appears in the "Available commands" note, guarded by a coverage test. +- Plugin selection split into two steps: workflow plugins, then language plugins. +- New pure helper `partitionSelectablePlugins` + exported `WORKFLOW_ORDER`, both unit-tested. + +### Breaking Changes +None for scripted use (`--plugin`, `--scope` unchanged). Interactive scope choice is +removed by design; deep local-scope removal is deferred. + +### Reviewer Focus Areas +- The bounded re-prompt loop and combined non-empty validation (language-only path). +- `isCancel` handling on both new multiselects. +- Correct exclusions in `partitionSelectablePlugins`. diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/architecture.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/architecture.md new file mode 100644 index 00000000..f57cf89f --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/architecture.md @@ -0,0 +1,82 @@ +# Architecture Review Report + +**Branch**: feat/init-flow-simplification -> main +**Date**: 2026-06-01_1725 +**PR**: #232 +**Scope**: src/cli/plugins.ts, src/cli/commands/init.ts, tests/plugins.test.ts + +## Summary of Assessment + +The refactor is architecturally sound. The orchestrator's specific questions resolve favorably: + +- **WORKFLOW_ORDER placement**: Correctly moved from a function-local `const` inside `init.ts` to an exported `const` in `plugins.ts` — single source of truth. The registry module (`plugins.ts`) is the right home: it already owns `PluginDefinition`, `DEVFLOW_PLUGINS`, and the command declarations that WORKFLOW_ORDER mirrors. The regression-guard test ties the two together. `/bug-analysis` is placed after `/self-review`, consistent with ADR-004 (bug-analysis is a separate post-pipeline workflow, not part of the main review pipeline) — its ordering position reflects that separation rather than embedding it in the review flow. +- **partitionSelectablePlugins mirrors existing pattern**: Yes, appropriately. It sits alongside `buildAssetMaps`/`buildFullSkillsMap`/`buildRulesMap` as a pure, exported, side-effect-free transformer over `PluginDefinition[]`. Same signature shape (takes plugins, returns derived structure), same testability profile, documented as pure/non-mutating/deterministic. This is the correct DIP-friendly boundary: `init.ts` depends on the abstraction (the partition contract) rather than re-deriving classification inline. +- **Exclusion set sharing**: The `EXCLUDED` set now lives in exactly one production location (`plugins.ts:723`). The previous inline filter in `init.ts` (old lines 315-324) was deleted. The test re-declares its own `EXCLUDED` set (`plugins.test.ts:310`) — this is an acceptable independent test oracle, not production duplication. No coupling concern. +- **Two-step selection loop**: Well-structured. Bounded (`MAX_ATTEMPTS = 3`, satisfies the reliability rule on bounded loops), handles empty buckets, handles cancel at each step, and fails closed (`p.cancel` + `process.exit(0)`) on exhausted attempts. See one MEDIUM observation below on the warning placement. +- **Layering/coupling between init.ts and plugins.ts**: Improved. `init.ts` (orchestration/UI) now consumes two pure exports from `plugins.ts` (registry/domain) instead of duplicating classification rules. Dependency points one direction: command → registry. No leakage of UI concerns into `plugins.ts`. + +No blocking issues found. The items below are MEDIUM/LOW and informational. + +--- + +## Issues in Your Changes (BLOCKING) + +None. + +--- + +## Issues in Code You Touched (Should Fix) + +### MEDIUM + +**Re-prompt warning is unreachable on the final attempt's failure path** — `src/cli/commands/init.ts:367-372` +**Confidence**: 82% +- Problem: Inside the loop, after computing `combined`, the "Select at least one plugin." warning is gated by `if (attempts < MAX_ATTEMPTS)`. With `MAX_ATTEMPTS = 3`, a user who selects nothing three times sees the warning after attempts 1 and 2, then on attempt 3 gets `p.cancel('Installation cancelled — no plugins selected.')`. This is logically correct, but the structure couples the "retry hint" and the "give up" decision into one branch evaluated *after* the prompts run. The loop therefore always runs the prompts at least once even when `attempts === MAX_ATTEMPTS` is about to fail — which is the intended behavior, but the control flow reads as if the warn/cancel are alternatives to *re-prompting* rather than post-prompt feedback. Minor SRP smell: the loop body mixes "collect selection" with "decide whether to terminate." +- Impact: Maintainability only — no functional defect. A future editor could misread the `attempts < MAX_ATTEMPTS` guard as protecting the prompt rather than the warning. +- Fix: Optional clarity improvement — extract the terminal decision: +```ts +if (combined.length > 0) { selectedPlugins = combined; break; } +const isLastAttempt = attempts >= MAX_ATTEMPTS; +if (isLastAttempt) { + p.cancel('Installation cancelled — no plugins selected.'); + process.exit(0); +} +p.log.warn('Select at least one plugin.'); +``` +This separates "did we get a selection" from "are we out of attempts" and removes the nested `if/else` after the `break`. + +### LOW + +**Language-bucket dependency on `commands.length === 0` is an implicit contract** — `src/cli/plugins.ts:729` +**Confidence**: 80% +- Problem: `partitionSelectablePlugins` classifies a plugin as "language" purely by `commands.length === 0`. This is an inferred classification rather than an explicit property (e.g., the existing `optional` flag, or a `category` field). Today every command-less selectable plugin happens to be a language/ecosystem plugin, so the heuristic is correct. But the bucket is *named* `language` while its actual predicate is "command-less" — a semantic gap. If a future non-language, command-less plugin is added (e.g., a config-only or skills-only plugin), it would silently land in the "Step 2 — Language plugins" UI section. +- Impact: Latent mislabeling risk; no current defect. The function's own doc comment says "command-less, optional language/ecosystem" — conflating two independent attributes. +- Fix: Either rename the bucket to `commandless` to match the actual predicate, or assert the intended invariant (every command-less selectable plugin is `optional`). The existing test "command-less plugins land in language bucket" already encodes the heuristic; a complementary test asserting `optional === true` for everything in the language bucket would lock the semantic intent. + +--- + +## Pre-existing Issues (Not Blocking) + +None within scope worth flagging at CRITICAL severity. + +--- + +## Suggestions (Lower Confidence) + +- **WORKFLOW_ORDER typed as `string[]` rather than a literal/branded command type** — `src/cli/plugins.ts:701` (Confidence: 65%) — Command identifiers are stringly-typed across both `WORKFLOW_ORDER` and `PluginDefinition.commands`. A shared `Command` union or branded type would let the compiler catch a misspelled `/code-reveiw` in WORKFLOW_ORDER instead of relying on the runtime regression-guard test. Out of scope for this PR but worth noting as a consistency improvement. +- **`installedSet` rebuild on every init** — `src/cli/commands/init.ts:1256` (Confidence: 60%) — `pluginsToInstall.flatMap(p => p.commands)` recomputes the available-commands set inline; could be a small helper colocated with WORKFLOW_ORDER in plugins.ts (e.g., `orderCommands(plugins)`) to keep all command-ordering logic in one module. Minor. + +--- + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 1 | 1 | +| Pre-existing | - | - | 0 | 0 | + +**Architecture Score**: 9 +**Recommendation**: APPROVED + +The refactor strengthens the layering: classification rules and display ordering now live in the registry module as pure exports, `init.ts` consumes them as abstractions, and the exclusion set is no longer duplicated in production code. WORKFLOW_ORDER placement is correct and ADR-004-consistent. The two findings are maintainability-grade (loop-body SRP clarity, implicit language-bucket contract) and do not block merge. diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/complexity.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/complexity.md new file mode 100644 index 00000000..f3401a2e --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/complexity.md @@ -0,0 +1,86 @@ +# Complexity Review Report + +**Branch**: feat-init-flow-simplification -> main +**Date**: 2026-06-01_1725 +**Scope**: src/cli/plugins.ts, src/cli/commands/init.ts, tests/plugins.test.ts (PR #232) + +## Assessment Summary + +The bounded selection loop is **readable and explainable in under 5 minutes**. Control flow +is shallow (max 3 nesting levels), the loop bound is explicit (`MAX_ATTEMPTS = 3`, satisfies +the reliability rule), and the `isCancel` guards, empty-bucket skips, retry warning, and final +cancel are all flat and sequential rather than over-nested. `partitionSelectablePlugins` is an +appropriately simple, pure, non-mutating helper. No blocking complexity issues. + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +## Issues in Code You Touched (Should Fix) + +### MEDIUM +None at >=80% confidence. The two-step structure is near-duplicated but each block is small +(~13 lines) and the duplication is not severe enough to warrant extraction — see Suggestions. + +## Pre-existing Issues (Not Blocking) +None observed within scoped files relevant to complexity. + +## Suggestions (Lower Confidence) + +- **Step 1 / Step 2 multiselect blocks are structurally near-identical** - + `src/cli/commands/init.ts:329-358` (Confidence: 70%) — Each step repeats the same shape: + default `[] `, `if (choices.length > 0)`, `await p.multiselect(...)`, `if (p.isCancel) { cancel; + exit }`, assign. A small local helper (e.g. `promptBucket(message, choices, initialValues?)` + returning `string[]`) would remove the duplication and the repeated cancel-and-exit boilerplate. + This is a readability nicety, not a defect — the current form is already clear and the + duplication is only twofold. Extract only if a third step is ever added. + +- **`combined.length > 0` / `attempts < MAX_ATTEMPTS` re-check at loop tail** - + `src/cli/commands/init.ts:360-372` (Confidence: 62%) — The success-break and the + warn-vs-final-cancel branch read clearly, but the `attempts < MAX_ATTEMPTS` test is evaluated + twice (loop condition + tail). This is correct and intentional (warn on non-final attempts, + hard-cancel on the last), just a minor cognitive double-take. Optional inline comment on the + tail `else` ("final attempt exhausted") would remove the need to mentally map it back to the + loop bound. + +## Detailed Findings + +### partitionSelectablePlugins (plugins.ts:719-737) — clean +- Cyclomatic complexity ~3 (one loop, one `continue`, one `if/else`). Well under thresholds. +- Pure, no mutation (verified: pushes to local arrays only, input untouched), deterministic, + preserves input ordering. The `EXCLUDED` set is a named constant with a doc comment listing + why each entry is excluded — exactly the readability pattern the complexity skill recommends + over magic filtering. The docstring matches the implementation. No issues. + +### Bounded selection loop (init.ts:326-373) — within all thresholds +- **Nesting depth**: max 3 (`while` > `if (choices.length)` > `if (isCancel)`). Threshold is 4. +- **Loop bound**: explicit (`MAX_ATTEMPTS = 3`), commented with rationale. Satisfies reliability. +- **Cyclomatic complexity**: ~7-8 decision points across the body — within the HIGH-warning band + (5-10) but not over it, and the branches are independent guard clauses rather than nested + conditionals, so effective reasoning load is low. +- **Control-flow clarity**: the three exit modes (cancel-on-step-cancel, success-break, + exhaustion-cancel) are each a flat guard, not interleaved. Explainable quickly. +- **Empty-bucket skips**: `if (choices.length > 0)` correctly leaves the corresponding + `*Selected` as `[]`, and `combined` handles the all-empty case via the retry/cancel tail. + No dead path, no infinite-no-progress risk (the bound terminates regardless of bucket state). + +### Tests (plugins.test.ts) — appropriately structured +- `partitionSelectablePlugins` suite (8 cases) tests behavior (bucket membership, exclusion, + disjointness, coverage, no-mutation, ordering, empty input) rather than implementation. Good. +- The `audit-claude is excluded` test now delegates to `partitionSelectablePlugins` instead of + re-implementing the exclusion filter inline — this removes duplicated logic and is a net + complexity reduction. No test is over-coupled to internals. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Complexity Score**: 9 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/consistency.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/consistency.md new file mode 100644 index 00000000..7dd8e1df --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/consistency.md @@ -0,0 +1,75 @@ +# Consistency Review Report + +**Branch**: feat/init-flow-simplification -> main (PR #232) +**Date**: 2026-06-01 17:25 +**Scope**: `src/cli/plugins.ts`, `src/cli/commands/init.ts`, `tests/plugins.test.ts` + +## Summary of Assessment + +The PR splits the single plugin multiselect into a two-step Workflow/Language flow, extracts +`partitionSelectablePlugins` + `WORKFLOW_ORDER` into `plugins.ts`, and adds tests. Overall the +new code is **highly consistent** with surrounding conventions: the new helper matches the +signature/style of its sibling builders, the multiselect prompts reuse the established @clack +patterns, and the new tests mirror the existing `describe`/`it`/custom-message conventions exactly. +All 39 tests in `tests/plugins.test.ts` pass. Only minor (LOW) style observations were found; none +block the merge. + +### Targeted question answers + +- **Does `partitionSelectablePlugins` match the sibling helpers?** Yes. It uses the same + `(plugins: PluginDefinition[])` parameter name and type as `buildAssetMaps`/`buildRulesMap`, + the same exported-function + JSDoc-block style, the same object-return shape (`buildAssetMaps` + also returns a named-property object), and the same `for (const plugin of plugins)` iteration + idiom. The JSDoc is more thorough than siblings (documents purity/exclusions), which is an + improvement, not an inconsistency. +- **Do the new multiselect prompts match existing @clack usage?** Yes. `p.multiselect`, + `p.isCancel`, `p.cancel('Installation cancelled.')`, `process.exit(0)` all match the prior + single-multiselect block and the flags multiselect later in the file. +- **Does `pluginHints` extension match tone/length?** Yes — short lowercase fragments, same + comma-separated style and brevity as existing entries (one minor ordering nit below). +- **Do the new tests match `tests/plugins.test.ts` conventions?** Yes — top-level `describe` + per export, `expect(value, 'message')` style, fixtures derived from `DEVFLOW_PLUGINS`. +- **Naming consistency (`WORKFLOW_ORDER`, `choices`, `initialValues`)?** Consistent. `choices` + became `workflowChoices`/`languageChoices` and `initialValues`→`workflowInitialValues`, which + is the correct disambiguation pattern; @clack option keys (`options`, `initialValues`) unchanged. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) + +None. + +## Suggestions (Lower Confidence) + +- **`pluginHints` key order diverges from `DEVFLOW_PLUGINS` registry order** — `src/cli/commands/init.ts:283-302` (Confidence: 70%) — In the registry, workflow plugins are ordered `... debug, explore, research, release, self-review, bug-analysis`. In the `pluginHints` map the new entries place `bug-analysis` before `self-review` (`... debug, explore, research, release, bug-analysis, self-review, ...`). Because `pluginHints` is a key-based lookup, order is purely cosmetic and has zero behavioral impact, but aligning it with the registry order (and with `WORKFLOW_ORDER`, which also lists `self-review` before `bug-analysis`) would keep the three lists scannable side-by-side. + +- **`workflowChoices` and `languageChoices` are byte-identical `.map` bodies** — `src/cli/commands/init.ts:306-316` (Confidence: 65%) — The two `.map(pl => ({ value, label, hint }))` blocks are duplicated verbatim. A single local helper (e.g. `const toChoice = (pl) => ({...})`) would remove the duplication. This is a mild DRY/style point, not a convention violation — the existing file already inlines similar small maps — so it is optional. + +- **Test re-declares the implementation's `EXCLUDED` set** — `tests/plugins.test.ts:310` (Confidence: 62%) — The `partitionSelectablePlugins` describe defines a local `EXCLUDED` set that mirrors the constant inside the function under test (`plugins.ts:723`). This is an accepted "independent restatement of the contract" testing pattern and is fine; flagging only because the rest of this test file tends to derive expectations from `DEVFLOW_PLUGINS`/getter functions rather than restating literals. If the exclusion list ever changes, both copies must be updated. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Consistency Score**: 9 +**Recommendation**: APPROVED + +Notes: +- `WORKFLOW_ORDER` array order matches its own JSDoc comment (`self-review -> bug-analysis -> debug`) + and the regression test (`/bug-analysis` after `/self-review`) — internally consistent. +- The bounded `MAX_ATTEMPTS = 3` selection loop with an inline reliability-rule comment is a new + pattern in this file but aligns with the project's reliability rule (no unbounded loops); not a + consistency violation. +- DECISIONS_CONTEXT: PF-005 and PF-007 reviewed; neither applies to a consistency defect in this + diff (changes correctly edit `src/cli/` source, and the new helper does not duplicate an existing + agent capability). No citation warranted. diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/performance.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/performance.md new file mode 100644 index 00000000..60d8ae9f --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/performance.md @@ -0,0 +1,81 @@ +# Performance Review Report + +**Branch**: feat/init-flow-simplification -> main +**Date**: 2026-06-01_1725 +**Scope**: src/cli/plugins.ts, src/cli/commands/init.ts, tests/plugins.test.ts (init flow refactor) +**PR**: #232 + +## Context + +The init command runs **once per install** — a human-interactive CLI path gated on +`p.multiselect`/`p.confirm` prompts that block on user input. CPU-time micro-performance is +irrelevant here; the only performance concerns worth flagging are (a) new blocking I/O or +network calls on the path, (b) accidental repeated work inside loops, and (c) algorithmic +regressions. All three were checked. No benchmark theater applied for the n≈21 pure function. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None relevant to performance within the reviewed scope. + +## Verification Summary (claims confirmed) + +**`partitionSelectablePlugins` — O(n), pure, no I/O** (plugins.ts:719-737) +- Single `for` loop over `plugins` (n≈21). `EXCLUDED` is a `Set` → O(1) `.has()` per element. + `plugin.commands.length > 0` is O(1) property access. No nested loops → **O(n)** total. +- No I/O, no network, no `await`. Does not mutate the input array (pushes to fresh local + arrays). Deterministic, preserves `DEVFLOW_PLUGINS` ordering within each bucket. +- Replaces two prior inline `.filter()` chains (old init.ts:315-325, two filters + two maps) + with one O(n) pass + two maps → strictly fewer array allocations. Minor net improvement, no regression. + +**Bounded selection loop — no repeated work, fixed bound** (init.ts:322-373) +- `while (attempts < MAX_ATTEMPTS)` with `MAX_ATTEMPTS = 3`; `attempts++` is the first + statement and `break` fires on success. Bounded; no unbounded `while(true)`. (avoids + unbounded-loop reliability rule.) +- `workflowChoices`, `languageChoices`, and `workflowInitialValues` are all computed **once + before** the loop (init.ts:304-320), not recomputed per iteration. No accidental repeated + work inside the loop body. +- The only per-iteration cost is `await p.multiselect(...)` — an interactive prompt that + blocks on human input, not CPU or I/O. Re-prompting up to 3x on empty selection is intended + UX, not a performance concern. + +**No new blocking I/O or network calls** +- The changed regions add no `readFileSync`/`writeFileSync`/`execSync`/`fetch`. The partition + is pure in-memory; the loop only awaits interactive prompts. +- The recommended-path `Promise.all` parallelizing project discovery + safe-delete version + check (init.ts:462) is pre-existing context, not introduced by this diff, and already + parallelizes independent I/O correctly. + +**`WORKFLOW_ORDER` hoist** (plugins.ts:701-705) +- Moved from a per-invocation local array (old init.ts:1222) to a module-level constant — + allocated once at module load instead of once per `init` call. Negligible but positive; no + regression. The downstream `Set`-based `installedSet.has()` filter is O(n) and unchanged. + +## Decisions / Knowledge Applied + +- **PF-005** (read in full): about not framing a capability as novel without checking the + existing agent roster — a design/workflow pitfall, not a performance concern. Does not apply + to this pure-function + bounded-loop change. No citation warranted. +- **FEATURE_KNOWLEDGE cli-rules**: concerns the rules subsystem, not the partition/loop logic + under review. No performance-relevant gotcha applies. + +## Suggestions (Lower Confidence) + +None. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Performance Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/regression.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/regression.md new file mode 100644 index 00000000..200c7b23 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/regression.md @@ -0,0 +1,82 @@ +# Regression Review Report + +**Branch**: feat/init-flow-simplification -> main +**PR**: #232 +**Date**: 2026-06-01 17:25 +**Scope**: src/cli/plugins.ts, src/cli/commands/init.ts, tests/plugins.test.ts +**Diff command**: `git diff main...HEAD` + +## Verdict Summary + +No blocking regressions found. Every critical contract called out in the review +brief is intact. The build compiles (`npm run build:cli`) and all 39 plugins tests +pass. The one notable behavior change (interactive init can no longer choose `local` +scope) is intentional and documented in the design doc (AC1). + +One important scope-accuracy note: the PR-context claim that the removed code was +"ONLY the interactive scope `p.select` else-block" is **inaccurate**. The diff also +refactors the plugin-selection multiselect into a two-step workflow/language flow +with a bounded retry loop. This is by design (design doc names "two-step plugin +selection" as an explicit goal), so it is not an accidental deletion — but reviewers +relying on the narrow scope statement should be aware the change surface is larger. + +## Critical Contract Verification (from review brief) + +| Contract | Status | Evidence | +|----------|--------|----------| +| `--scope user`/`--scope local` resolve | PRESERVED | init.ts:189-195 normalizes `options.scope`, unchanged | +| `--scope local` creates ./.claude + ./.devflow | PRESERVED | init.ts:914-921 `if (scope === 'local')` mkdir branch untouched | +| `options.scope` branch | PRESERVED | init.ts:189-195 | +| `!process.stdin.isTTY` non-TTY branch (default user + info log) | PRESERVED | init.ts:196-198 | +| `--hud-only` scope path | PRESERVED | init.ts:186-188 (user scope) + 202-268 (hud-only block) | +| `--plugin=` / parsePluginSelection path | PRESERVED | init.ts:272-280, unchanged | +| Manifest records `scope` | PRESERVED | init.ts:1314 (`scope,` in manifestData); hud-only manifest at 257 | +| "Available commands" filters WORKFLOW_ORDER + includes /bug-analysis | PRESERVED + IMPROVED | init.ts:1256-1262 now uses imported `WORKFLOW_ORDER` (plugins.ts:701) which includes `/bug-analysis` | +| Removed code = ONLY interactive scope else-block | PARTIALLY TRUE | Scope else-block removed (intended); plugin multiselect ALSO refactored (intended per design doc) | + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) + +None. + +## Suggestions (Lower Confidence) + +- **Two-step plugin loop re-prompts both steps on retry** - `init.ts:326-373` (Confidence: 65%) — On an empty-selection retry, the bounded loop re-shows BOTH Step 1 (workflow) and Step 2 (language) from scratch rather than only the empty one. Minor UX friction (a user who picked workflow plugins but skipped language, yielding a non-empty `combined`, never hits this — the loop only retries when `combined.length === 0`, so functionally correct). Not a regression; the old single multiselect had no retry at all. + +- **Degenerate empty-bucket loop** - `init.ts:326-373` (Confidence: 60%) — If both `workflowChoices` and `languageChoices` were empty, the loop would spin 3 times prompting nothing then `exit(0)`. Cannot occur with the real `DEVFLOW_PLUGINS` registry (always has command-bearing + language plugins), so no practical regression. The bounded-loop guard (MAX_ATTEMPTS=3, reliability rule) is correctly applied. + +## Regression-Safety Notes (informational, no action) + +- **`required: true` -> `required: false` on plugin multiselects**: The old single multiselect used `required: true` (clack rejects empty + has its own cancel semantics). The new steps use `required: false` and enforce non-empty via the application-level bounded loop (init.ts:362-372), which `p.cancel` + `exit(0)` on exhaustion. Net contract — "init cannot proceed with zero plugins" — is preserved. Design doc explicitly flagged this cancel-semantics shift (§ "required: true cancel semantics"). + +- **`initialValues` parity**: Old flow pre-selected non-optional plugins (`preSelected`). New flow pre-selects non-optional workflow plugins (`workflowInitialValues`, init.ts:318-320). Language step has no `initialValues` — correct, since all 8 language plugins are `optional: true` (verified by existing test plugins.test.ts:170-175) and were never pre-selected before. Default-selection behavior preserved. + +- **Local `WORKFLOW_ORDER` -> exported `WORKFLOW_ORDER`**: The deleted local array (init.ts old:1222-1226) omitted `/bug-analysis`. The new exported constant (plugins.ts:701-705) includes it. This is a latent-bug fix: the shipped `devflow-bug-analysis` command would previously never have rendered in the "Available commands" note. Now covered by a regression-guard test (plugins.test.ts:400-412). + +- **Intentional behavior change (documented)**: Interactive `devflow init` on a TTY can no longer choose `local` scope — it silently defaults to `user`. This matches design-doc AC1 ("no longer shows the Installation scope prompt and always installs on user scope") and AC2 (`--scope local` retained for scripted use). Not flagged as a regression. + +- **PF-006 awareness (silent contract shift)**: No silent external-API contract shift introduced. The clack prompt API usage (`p.multiselect`, `p.isCancel`, `p.cancel`) is consistent with existing call sites elsewhere in the file. The `required: false` change is deliberate, not an undocumented API drift. + +## Validation Performed + +- `npm run build:cli` — compiles clean (tsc, no errors) +- `npx vitest run tests/plugins.test.ts` — 39 passed, 0 failed, 0 skipped +- Consumer scan: `WORKFLOW_ORDER` / `partitionSelectablePlugins` imported only by init.ts and the test — no orphaned/broken consumers +- Current-state read of init.ts:183-199 confirms scope block matches the diff + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Regression Score**: 9/10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/reliability.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/reliability.md new file mode 100644 index 00000000..cd240685 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/reliability.md @@ -0,0 +1,120 @@ +# Reliability Review Report + +**Branch**: feat/init-flow-simplification -> main +**PR**: #232 +**Date**: 2026-06-01_1725 +**Scope**: `src/cli/plugins.ts`, `src/cli/commands/init.ts`, `tests/plugins.test.ts` + +## Iron Law Assessment + +> Every operation must terminate and every resource must be bounded. + +The two-step plugin-selection retry loop and `partitionSelectablePlugins` were audited +against the reliability rule (every loop/retry has an explicit fixed upper bound). Both +satisfy the Iron Law. Detailed verification below. + +### Bounded selection loop — VERIFIED CORRECT + +`src/cli/commands/init.ts:322-373` + +- Fixed bound: `const MAX_ATTEMPTS = 3` (line 323); loop is `while (attempts < MAX_ATTEMPTS)` + (line 326). No `while(true)`, no unbounded recursion. +- Counter placement: `attempts++` is the first statement of the body (line 327), so the + guard at line 367 (`if (attempts < MAX_ATTEMPTS)`) compares the already-incremented value. + Termination trace: + - Iter 1: attempts=1, empty -> 1<3 true -> warn, continue + - Iter 2: attempts=2, empty -> 2<3 true -> warn, continue + - Iter 3: attempts=3, empty -> 3<3 false -> `p.cancel(...)` + `process.exit(0)` + No off-by-one: a 4th attempt is impossible and the final-attempt warning/cancel branch + fires exactly once. Non-empty selection breaks early (line 362-365). +- Dual termination guarantee (defense in depth): even if the `process.exit(0)` on the + terminal branch were removed, the `while` condition still caps iterations at 3. Both + guards agree on the same bound. Sound. + +### isCancel handling — VERIFIED CLEAN + +`src/cli/commands/init.ts:338-341`, `353-356` + +- Both steps check `p.isCancel(...)` immediately after each `multiselect`, call + `p.cancel('Installation cancelled.')`, then `process.exit(0)`. +- No partial state is committed: `selectedPlugins` is only assigned at line 363 after a + successful non-empty `combined`. A cancel at step 1 or step 2 exits before any plugin + state is persisted. The setup-mode `isCancel` (line 401-404) follows the same clean-exit + pattern. No resource is left half-initialized. + +### Empty-bucket guard — VERIFIED + +`src/cli/commands/init.ts:331`, `347` + +- Each `multiselect` is gated by `if (workflowChoices.length > 0)` / `if (languageChoices.length > 0)`. +- A bucket with zero choices is skipped, so `p.multiselect` is never invoked with an empty + `options` array. This correctly avoids a @clack empty-options crash (the primary + reliability hazard for this UI). The respective `*Selected` array defaults to `[]`, and + `combined` aggregates whatever was selected. + +### partitionSelectablePlugins — VERIFIED DETERMINISTIC + +`src/cli/plugins.ts:719-737` + +- Pure function: iterates the input once, pushes into two fresh local arrays, returns them. + No mutation of the input (test `does not mutate the input array` confirms), no I/O, no + hidden/module-level state. The `EXCLUDED` Set is constructed locally per call. +- Empty input: the `for` loop body never executes; returns `{ workflow: [], language: [] }`. + Covered by test `returns empty buckets for empty input` (line 381-385). +- Determinism: output ordering follows input ordering (test `preserves DEVFLOW_PLUGINS + ordering within each bucket`); buckets are disjoint and exhaustive over non-excluded + plugins (tests `buckets are disjoint`, `workflow + language covers all selectable`). + +### Test coverage — STRONG + +`tests/plugins.test.ts` adds disjointness, exhaustiveness, ordering, no-mutation, +empty-input, and a `WORKFLOW_ORDER` regression guard ensuring every workflow command is +listed. This is appropriate behavior-focused coverage for the new pure function and the +display-order invariant. + +## Issues in Your Changes (BLOCKING) + +None. The bounded loop, isCancel handling, empty-bucket guard, and partition function all +satisfy the reliability rule. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) + +None within scope. + +## Suggestions (Lower Confidence) + +- **Both-buckets-empty produces three silent warnings** — `src/cli/commands/init.ts:326-373` + (Confidence: 65%) — If both `workflowChoices` and `languageChoices` were empty, the loop + runs all 3 iterations issuing `p.log.warn('Select at least one plugin.')` twice with no + intervening prompt, then cancels. This is unreachable with the real `DEVFLOW_PLUGINS` + registry (workflow plugins always exist), so it is not a defect today. An explicit + precondition assert before the loop (e.g. assert at least one of the two buckets is + non-empty) would convert a confusing degenerate path into a clear fail-fast, matching the + reliability rule's "assert preconditions in production code" guidance. + +- **Loop guard at line 367 is logically redundant with the `while` condition** — + `src/cli/commands/init.ts:367` (Confidence: 60%) — Because the terminal branch calls + `process.exit(0)`, the `attempts < MAX_ATTEMPTS` re-check is belt-and-suspenders. This is + acceptable defensive style and need not change; noted only for awareness. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Reliability Score**: 9 +**Recommendation**: APPROVED + +Notes: +- DECISIONS_CONTEXT: PF-005 read and confirmed not applicable (it concerns assuming a + capability does not exist before checking the agent roster — unrelated to loop bounds). + No ADR/PF citation warranted. +- FEATURE_KNOWLEDGE (`cli-rules`): scoped to the rules system, no anti-patterns or gotchas + bear on the plugin-selection loop. No findings derived from it. +- PRIOR_RESOLUTIONS: (none). diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/resolution-summary.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/resolution-summary.md new file mode 100644 index 00000000..502a3209 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/resolution-summary.md @@ -0,0 +1,47 @@ +# Resolution Summary + +**Branch**: feat/init-flow-simplification -> main +**Date**: 2026-06-01_1725 +**Review**: .devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725 +**Command**: /resolve + +## Decisions Citations + +- applies ADR-008 — batch-1, issue-3 (deterministic CLI plumbing logic) +- avoids PF-005 — batch-1, issue-3 (verified @clack types before removing casts) +- avoids PF-007 — batch-1 (edited src/cli source only, not installed copies) + +## Statistics +| Metric | Value | +|--------|-------| +| Total Issues | 9 | +| Fixed | 7 | +| False Positive | 2 | +| Deferred | 0 | +| Blocked | 0 | + +## Fixed Issues +| Issue | File:Line | Commit | +|-------|-----------|--------| +| Extract pure `combineSelection`/`shouldRetry` from interactive loop + 10 unit tests | src/cli/commands/init.ts:322-373, tests/init-logic.test.ts | 2de84c5 | +| Add reverse WORKFLOW_ORDER regression guard (every ORDER entry maps to a real registry command) | tests/plugins.test.ts:400-412 | 2de84c5 | +| Remove redundant `as string[]` casts (isCancel already narrows) | src/cli/commands/init.ts:342,357 | 2de84c5 | +| Loop clarity refactor — separate "got selection" from "out of attempts" via `isLastAttempt` | src/cli/commands/init.ts:367-372 | 2de84c5 | +| Clarifying comment on `language` bucket predicate (commands.length === 0 implicit contract) | src/cli/plugins.ts:729 | 2de84c5 | +| Reorder `pluginHints` so `self-review` precedes `bug-analysis` (matches WORKFLOW_ORDER) | src/cli/commands/init.ts:283-302 | 2de84c5 | +| Extract shared `toChoice` helper, dedupe workflow/language `.map` bodies | src/cli/commands/init.ts:306-316 | 2de84c5 | + +## False Positives +| Issue | File:Line | Reasoning | +|-------|-----------|-----------| +| Test re-declares EXCLUDED set instead of importing | tests/plugins.test.ts:310 | Intentional independent test oracle. Exporting the internal `EXCLUDED` Set solely for tests would leak a private implementation detail; drift risk is low, independence value higher. Won't fix. | +| Add precondition assert for both-empty buckets | src/cli/commands/init.ts (loop) | Noise over value — the loop only runs inside the isTTY branch where buckets are non-empty in practice; the degenerate case is already covered by `combineSelection` return type and `shouldRetry` tests. Won't fix. | + +## Deferred to Tech Debt +None. + +## Blocked +None. + +## Notes +All 9 reviewers returned APPROVED / APPROVED_WITH_CONDITIONS with zero blocking issues. The 3 should-fix items (≥80% confidence) and 4 low-confidence suggestions were resolved directly; 2 suggestions were validated as won't-fix with reasoning. Full suite: 1528 passed, 0 failed. Build clean under strict TypeScript. diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/review-summary.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/review-summary.md new file mode 100644 index 00000000..e1790ed2 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/review-summary.md @@ -0,0 +1,249 @@ +# Code Review Summary + +**Branch**: feat/init-flow-simplification -> main (PR #232) +**Date**: 2026-06-01_1725 +**Reviewed by**: 9 specialized reviewers (security, architecture, performance, complexity, consistency, regression, testing, reliability, typescript) + +--- + +## Merge Recommendation: APPROVED + +The refactor is well-executed and ready to merge. Zero blocking issues in changed code across all reviewers. Two should-fix findings (testing: extract interactive loop logic and add bidirectional regression guard; typescript: remove redundant type casts) are correctness improvements, not defects. All critical contracts preserved. The change strengthens the layering by moving classification and display-ordering logic into the registry module and includes a latent-bug fix (exports `/bug-analysis` in WORKFLOW_ORDER, which was previously omitted). + +--- + +## Issue Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | Total | +|----------|----------|------|--------|-----|-------| +| Blocking | 0 | 0 | 0 | - | 0 | +| Should Fix | - | 0 | 3 | - | 3 | +| Pre-existing | - | - | 0 | 0 | 0 | + +**Reviewers Summary**: +- **APPROVED**: 8 reviewers (security, architecture, performance, complexity, consistency, regression, reliability, typescript) +- **APPROVED_WITH_CONDITIONS**: 1 reviewer (testing — conditions are the 2 should-fix findings) + +--- + +## Blocking Issues + +None. No CRITICAL or HIGH severity issues in your changes. + +--- + +## Should-Fix Issues (High Confidence) + +### 1. Extract and Unit-Test the Interactive Selection-Loop Logic + +**Location**: `src/cli/commands/init.ts:322-373` +**Confidence**: 88% +**Category**: Testing (should-fix) +**Severity**: MEDIUM + +**Problem**: +The bounded-retry selection loop (bounded retry, empty-bucket skip, combined-selection accept condition) is implemented inline in the command action, directly interleaved with `p.multiselect()` / `p.isCancel()` / `process.exit()` calls. None of it is unit-tested — only static verification. The codebase precedent (e.g., `parsePluginSelection`, `computeGitignoreAppend`, `mergeDenyList` extracted and tested in `tests/init-logic.test.ts`) expects decision logic to be separated from I/O. + +Three behaviors carry real regression risk with zero test coverage: +1. Retry-then-cancel boundary (does attempt 3 with empty selection actually exit vs loop forever?) +2. Empty-bucket skip (if `workflowChoices` is empty, is `workflowSelected` correctly left `[]`?) +3. Combine/accept rule (when is `combined.length > 0` true and the loop breaks?) + +**Impact**: A refactor of any of these would not fail any test. + +**Fix**: +Extract a pure reducer and a decision predicate: + +```typescript +// In tests/init-logic.test.ts, add: +function combineSelection( + workflowSelected: string[], + languageSelected: string[] +): { plugins: string[]; accepted: boolean } { + const plugins = [...workflowSelected, ...languageSelected]; + return { plugins, accepted: plugins.length > 0 }; +} + +function shouldContinueRetry(attempt: number, maxAttempts: number, accepted: boolean): boolean { + return !accepted && attempt < maxAttempts; +} + +// Test cases: +test('combineSelection accepts non-empty combined', () => { + expect(combineSelection(['a'], [])).toEqual({ plugins: ['a'], accepted: true }); + expect(combineSelection([], ['b'])).toEqual({ plugins: ['b'], accepted: true }); + expect(combineSelection(['a'], ['b'])).toEqual({ plugins: ['a', 'b'], accepted: true }); +}); + +test('combineSelection rejects empty both-buckets', () => { + expect(combineSelection([], [])).toEqual({ plugins: [], accepted: false }); +}); + +test('shouldContinueRetry bounded at maxAttempts', () => { + expect(shouldContinueRetry(1, 3, false)).toBe(true); // retry + expect(shouldContinueRetry(2, 3, false)).toBe(true); // retry + expect(shouldContinueRetry(3, 3, false)).toBe(false); // exit +}); +``` + +Move the logic in `init.ts` to call the extracted helpers. Leave only the `p.multiselect` / `isCancel` / `exit` plumbing inline — that path is genuinely hard to unit-test. + +--- + +### 2. Make WORKFLOW_ORDER Regression Guard Bidirectional + +**Location**: `tests/plugins.test.ts:400-412` +**Confidence**: 82% +**Category**: Testing (should-fix) +**Severity**: MEDIUM + +**Problem**: +The regression-guard test labeled "every workflow plugin command appears in WORKFLOW_ORDER" iterates workflow plugins and asserts each command is present in WORKFLOW_ORDER. This is directionally asymmetric — it catches "command added to a plugin but forgotten in WORKFLOW_ORDER" but NOT the symmetric case: a stale/renamed entry left *in* WORKFLOW_ORDER that no longer maps to any plugin command, or a command from an excluded plugin accidentally added. + +Example: if `/audit-claude` (an excluded plugin) were accidentally added to WORKFLOW_ORDER, the current forward-direction test would not catch it. + +**Impact**: For the specific `/bug-analysis` scenario, the existing explicit `contains('/bug-analysis')` test (line 389) + forward check would catch a drop. But the guard is incomplete. + +**Fix**: +Add a reverse-direction assertion. Build the set of all known command strings and assert every WORKFLOW_ORDER entry is a known command: + +```typescript +test('reverse: every WORKFLOW_ORDER command exists in known plugins', () => { + const knownCommands = new Set( + DEVFLOW_PLUGINS.flatMap(p => p.commands) + ); + for (const cmd of WORKFLOW_ORDER) { + expect(knownCommands.has(cmd), `WORKFLOW_ORDER contains unknown command: ${cmd}`).toBe(true); + } +}); +``` + +Together with the existing forward check, this makes the guard bidirectional and catches stale/typo'd entries in both directions. + +--- + +### 3. Remove Redundant `as string[]` Type Casts (Consistency with Best Practice) + +**Location**: `src/cli/commands/init.ts:342, 357` +**Confidence**: 88% +**Category**: TypeScript (should-fix) +**Severity**: MEDIUM + +**Problem**: +After `if (p.isCancel(step1))` guard, TypeScript has already narrowed `step1` from `symbol | string[]` to `string[]`. The subsequent `as string[]` assertion is redundant — it asserts a type the compiler has already proven. Redundant assertions weaken type-narrowing guarantees and can mask real mismatches if the option value type ever changes. + +```typescript +// Current (redundant): +if (p.isCancel(step1)) { ... process.exit(0); } +workflowSelected = step1 as string[]; // ← redundant; compiler already knows step1: string[] + +// Better: +if (p.isCancel(step1)) { ... process.exit(0); } +workflowSelected = step1; // ← no cast needed; type guard does the work +``` + +**Impact**: Low runtime risk (the cast is correct today), but it suppresses future compiler help and trains readers that the cast is necessary when it is not. + +**Fix**: +Remove both assertions: + +```typescript +// src/cli/commands/init.ts:342 +workflowSelected = step1; // was: step1 as string[] + +// src/cli/commands/init.ts:357 +languageSelected = step2; // was: step2 as string[] +``` + +Note: This pattern is consistent with other uses of `isCancel` narrowing in the same file (e.g., setup-mode at line 401-404). The codebase also uses similar conventions elsewhere (`pluginSelection as string[]`, `flagSelection as string[]`), so the change improves the most modern approach; leaving them is defensible if the team prefers the existing convention. + +--- + +## Suggestions (Lower Confidence ≤79%) + +**Not blocking the merge; reported for optional improvement.** + +### Clarity & Maintainability + +- **Loop-body SRP clarity** — `init.ts:367-372` (Architecture, 82%) — The `attempts < MAX_ATTEMPTS` guard couples the "retry hint" and the "give up" decision. Optional refactor: + ```typescript + const isLastAttempt = attempts >= MAX_ATTEMPTS; + if (isLastAttempt) { + p.cancel('Installation cancelled — no plugins selected.'); + process.exit(0); + } + p.log.warn('Select at least one plugin.'); + ``` + +- **Two-step multiselect blocks are near-identical** — `init.ts:329-358` (Complexity, 70%) — Each step repeats the same shape. Optional extraction only if a third step is ever added. + +- **Language-bucket heuristic is implicit** — `plugins.ts:729` (Architecture, 80%) — The "language" bucket is classified purely by `commands.length === 0` rather than an explicit category field. Optional assertion test: every command-less selectable plugin is `optional: true`. + +### Type Safety + +- **`WORKFLOW_ORDER` could be `readonly`** — `plugins.ts:701` (TypeScript, 65%) — Typed as mutable `string[]`; could be `as const` for immutability. Defers to existing codebase convention (plain mutable arrays throughout the file). + +### Deduplication + +- **pluginHints key order diverges from DEVFLOW_PLUGINS registry** — `init.ts:283-302` (Consistency, 70%) — Cosmetic; aligning would keep the three lists scannable side-by-side. + +- **workflowChoices and languageChoices use identical `.map` bodies** — `init.ts:306-316` (Consistency, 65%) — Optional local helper to remove duplication. + +--- + +## Key Findings (Positive) + +### Cross-Reviewer Consensus + +All reviewers independently confirmed several strengths: + +1. **Bounded loop design**: Security, Reliability, and Complexity all praised the explicit `MAX_ATTEMPTS = 3` guard, which avoids unbounded re-prompt loops and aligns with the reliability rule. No off-by-one defects. + +2. **Pure function extraction**: Architecture, Performance, and Testing all confirmed `partitionSelectablePlugins` is correctly extracted as a side-effect-free transformer that mirrors the existing pattern of sibling helpers (`buildAssetMaps`, `buildRulesMap`). No I/O, no mutation, O(n) algorithm. + +3. **Latent-bug fix**: Regression reviewer noted the new exported `WORKFLOW_ORDER` includes `/bug-analysis`, which the old function-local array did not. This fix is now covered by a regression-guard test. + +4. **Strengthened layering**: Architecture confirmed that moving classification rules and display ordering into the registry module (instead of inline in `init.ts`) improves DIP (Dependency Inversion Principle) — `init.ts` depends on abstractions, not implementation details. + +### Test Coverage + +- **partitionSelectablePlugins test suite is well-designed**: 8 test cases covering behavior (bucket membership, exclusion, immutability, ordering, disjointness, completeness, empty input). Tests are behavior-focused, not implementation-coupled. + +- **WORKFLOW_ORDER regression guard**: Catches additions of new commands; forward direction is correct and catches `/bug-analysis` drop. + +- **All 39 tests pass**: `npx vitest run tests/plugins.test.ts` returns 39 passed, 0 failed, 0 skipped. + +### Security & Reliability + +- **Security**: Pristine. No injection surface, no new trust boundary, all user input validated via existing upstream validators. Bounded retry avoids unbounded loops. + +- **Reliability**: Loop bound is explicit (3 attempts), counter placement is correct (no off-by-one), `isCancel` handling is clean (exit before state commit), empty-bucket guards prevent @clack crashes. + +- **TypeScript**: Strict-mode clean (`tsc --noEmit -p tsconfig.json`), no `any` types, proper `isCancel` symbol-result narrowing at all call sites. + +--- + +## Convergence Status + +**Cycle**: 1 (First Review) +**Prior Resolution**: (none) +**Assessment**: First cycle — no prior false positives to cross-reference. All 9 reviewers independently reached consensus (0 blocking issues, 3 should-fix items, mostly LOW suggestions). + +--- + +## Action Plan + +1. **Extract and unit-test the selection-loop decision logic** (testing: 88% confidence) — Add `combineSelection()` and `shouldContinueRetry()` helpers in `tests/init-logic.test.ts` with 4-6 test cases. Update `init.ts` to call the extracted helpers. + +2. **Add reverse-direction regression assertion** (testing: 82% confidence) — Ensure every WORKFLOW_ORDER entry maps to a known plugin command, not just the forward direction. + +3. **Remove redundant `as string[]` casts** (typescript: 88% confidence) — Let `isCancel` type narrowing do the work at lines 342 and 357. + +4. *(Optional, LOW priority)* Address the 3 LOW suggestions if time permits (loop-body clarity refactor, pluginHints ordering, language-bucket implicit contract). + +--- + +## Sign-Off + +**Recommendation**: Merge after addressing the 3 should-fix items above. All blocking criteria satisfied; no critical or high-severity issues in your changes. The refactor improves code organization, adds important test coverage, and fixes a latent bug in command display ordering. + diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/security.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/security.md new file mode 100644 index 00000000..ddff3e52 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/security.md @@ -0,0 +1,76 @@ +# Security Review Report + +**Branch**: feat/init-flow-simplification -> main (PR #232) +**Date**: 2026-06-01_1725 +**Scope**: `src/cli/plugins.ts`, `src/cli/commands/init.ts`, `tests/plugins.test.ts` +**Focus**: Input validation (plugin selection / `--plugin`), path handling, injection surface, secrets + +## Summary of Assessment + +This is a CLI refactor of the `devflow init` flow. The changes are: (1) removal of the +interactive scope `p.select()` prompt, (2) a new exported `WORKFLOW_ORDER` array (adds +`/bug-analysis`, moved from a function-local const), and (3) a new pure +`partitionSelectablePlugins()` function plus splitting one plugin multiselect into two +sequential multiselects wrapped in a bounded retry loop. + +I traced the security-relevant data flows the prompt called out: + +- **`--plugin` parsing** (`parsePluginSelection`, init.ts:108) — unchanged by this PR, but it + is the upstream validator for the only user-controlled string that reaches plugin selection. + It splits on `,`, normalizes, and **validates against an allowlist** (`validNames.includes`). + Invalid names trigger `process.exit(1)`. The downstream consumer + (`DEVFLOW_PLUGINS.filter(p => selectedPlugins.includes(p.name))`, init.ts:938-940) only ever + yields registry plugin objects — no user string is interpolated into a path. Safe. +- **Interactive selection** — `partitionSelectablePlugins` returns slices of the static, + hardcoded `DEVFLOW_PLUGINS` registry. The multiselect `value` fields are registry `pl.name` + strings, never free-form user input. `selectedPlugins` therefore can only contain known + plugin names. No injection surface introduced. +- **Path handling** — all `fs.mkdir`/path joins in the touched code (`claudeDir`, `devflowDir`, + `pluginsDir`) derive from `getInstallationPaths(scope)` and `__dirname`, not from plugin + selection. The `--scope local` mkdir path is computed from scope resolution, which is + validated (`/^(user|local)$/i` at the Commander layer plus the explicit recheck at + init.ts:190-195). Not affected by this refactor. +- **Secrets** — none introduced. No credentials, tokens, or `Math.random()` for security values + in the diff. +- **`WORKFLOW_ORDER`** — a static array of literal command-name strings consumed only for + display ordering (`p.note`). No injection or trust-boundary concern. + +The refactor is security-neutral. The new `partitionSelectablePlugins` is a pure, side-effect-free +function operating on a trusted in-process registry. No new trust boundary, no new external input, +no new filesystem path derived from user data. The author also added a bounded retry loop +(`MAX_ATTEMPTS = 3`) which avoids an unbounded re-prompt loop (aligns with the reliability rule, +though that is a robustness rather than security property). + +`avoids PF-005` — verified actual code rather than assuming the validator behavior: +`parsePluginSelection` was read directly to confirm allowlist enforcement. + +## Issues in Your Changes (BLOCKING) + +None. No security defects with >=80% confidence were found in the added/modified lines. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None at CRITICAL severity. (Per methodology, pre-existing non-critical issues in untouched lines +are not reported.) The `--plugin` validator and scope mkdir handling were reviewed as adjacent +trust boundaries and found sound. + +## Suggestions (Lower Confidence) + +None meeting the 60-79% bar. The change is a clean, well-tested refactor with strong unit +coverage for the new pure function (disjointness, completeness, no-mutation, ordering, empty +input) and `WORKFLOW_ORDER` (membership, dedup, regression guard against orphan commands). + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Security Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/testing.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/testing.md new file mode 100644 index 00000000..558c89b9 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/testing.md @@ -0,0 +1,141 @@ +# Testing Review Report + +**Branch**: feat-init-flow-simplification -> main (PR #232) +**Date**: 2026-06-01_1725 +**Scope**: `src/cli/plugins.ts`, `src/cli/commands/init.ts`, `tests/plugins.test.ts` + +## Executive Summary + +The new tests for `partitionSelectablePlugins` are well-designed and behavior-focused. They cover +every dimension the prompt asked about: bucketing (workflow/language), exclusions, immutability, +source-order preservation, disjointness, completeness (workflow + language = all selectable), and +empty input. `npx vitest run tests/plugins.test.ts` passes 39/39. + +The pure, deterministic logic (`partitionSelectablePlugins`, `WORKFLOW_ORDER`) was correctly +extracted into `plugins.ts` and is thoroughly tested. The one genuine gap is that the **interactive +two-step selection loop in `init.ts` (bounded retry, `isCancel`, empty-bucket skip, combine logic) +was NOT extracted** into a testable function — it lives inline in the command action and is only +verified statically. This is a real but MEDIUM-severity gap given the @clack interactive dependency. + +There is one substantive correctness concern in the WORKFLOW_ORDER regression guard: it is +**directionally asymmetric** and would not catch every kind of "dropped command" the comment claims. + +--- + +## Issues in Your Changes (BLOCKING) + +None. No blocking test-quality defects in the changed lines. + +--- + +## Issues in Code You Touched (Should Fix) + +### MEDIUM + +**Interactive two-step selection loop is untested — logic not extracted for testability** — `src/cli/commands/init.ts:322-373` +**Confidence**: 88% +- Problem: The new selection-loop logic — bounded retry (`MAX_ATTEMPTS = 3`), empty-bucket skip + (`if (workflowChoices.length > 0)` / `if (languageChoices.length > 0)`), `isCancel` handling, + and the `combined = [...workflowSelected, ...languageSelected]` / `combined.length > 0` accept + condition — is implemented inline inside the command action, directly interleaved with + `p.multiselect()` / `p.isCancel()` / `process.exit()` calls. None of it is unit-tested. The + testable decision logic was not separated from the @clack I/O, so the only verification is static + reading. `tests/init-logic.test.ts` extracts and tests sibling helpers (`parsePluginSelection`, + `computeGitignoreAppend`, `mergeDenyList`, etc.), which sets the precedent that this logic should + have been extracted too. +- Impact: Three behaviors carry real regression risk with zero test coverage: + (1) the retry-then-cancel boundary (does attempt 3 with empty selection actually exit vs loop + forever?), (2) the empty-bucket skip (if `workflowChoices` is empty, is `workflowSelected` + correctly left `[]` rather than blocking?), and (3) the combine/accept rule. A refactor of any + of these would not fail any test. +- Fix: Extract a pure reducer, e.g. + `combineSelection(workflowSelected: string[], languageSelected: string[]): { plugins: string[]; accepted: boolean }`, + and a `shouldRetry(attempt, maxAttempts, accepted)` predicate. Unit-test the accept condition + (non-empty combine), the empty-both-buckets case, and the bounded-retry exhaustion. Leave only + the `p.multiselect`/`isCancel`/`exit` plumbing inline. This mirrors the existing + `parsePluginSelection` extraction pattern and keeps the @clack flow (genuinely hard to unit-test) + as the only untested seam. Note the @clack interactive path itself is reasonably left untested — + the issue is that the *decision* logic was not separated from it. + +**WORKFLOW_ORDER regression guard is directionally asymmetric — comment overstates coverage** — `tests/plugins.test.ts:400-412` +**Confidence**: 82% +- Problem: The test labeled "every workflow plugin command appears in WORKFLOW_ORDER (regression + guard)" iterates `workflow` plugins and asserts each plugin command is present in + `WORKFLOW_ORDER`. This catches "command added to a plugin but forgotten in WORKFLOW_ORDER". It + does NOT catch the symmetric case the prompt asks about — a command *dropped* from a plugin while + also dropped from WORKFLOW_ORDER, or a stale/renamed entry left *in* WORKFLOW_ORDER that no longer + maps to any plugin command. The set membership is one-directional (plugin commands ⊆ + WORKFLOW_ORDER), never the reverse (WORKFLOW_ORDER entries ⊆ known commands). +- Impact: For the specific `/bug-analysis` scenario: dropping it from WORKFLOW_ORDER while the + plugin still declares it *would* be caught here (good), and is *also* caught by the explicit + `contains('/bug-analysis')` test at line 389. But the regression guard as written cannot catch a + WORKFLOW_ORDER entry that drifts out of sync in the reverse direction (e.g., `/audit-claude` is in + WORKFLOW_ORDER but its plugin is excluded from the `workflow` bucket, so a typo'd or stale + WORKFLOW_ORDER entry for an excluded/nonexistent command goes undetected). +- Fix: Add the reverse-direction assertion. Build the set of all known command strings from + `DEVFLOW_PLUGINS.flatMap(p => p.commands)` and assert every `WORKFLOW_ORDER` entry is a known + command. Together with the existing forward check this makes the guard bidirectional and would + catch stale/typo'd entries. (The forward guard correctly excludes `/audit-claude` since it is not + in the workflow bucket; the reverse guard must allow it because it is intentionally in + WORKFLOW_ORDER — assert membership in the full command set, not the workflow-bucket set.) + +--- + +## Pre-existing Issues (Not Blocking) + +None relevant to this scope. + +--- + +## Suggestions (Lower Confidence) + +- **`partitionSelectablePlugins` immutability test is shallow** — `tests/plugins.test.ts:362-366` + (Confidence: 70%) — `expect(DEVFLOW_PLUGINS).toEqual(inputCopy)` where `inputCopy = [...DEVFLOW_PLUGINS]` + is a shallow copy of the same element references. It verifies the array isn't reordered/mutated but + not that nested `commands`/`skills` arrays are untouched. The function only reads, so risk is low; + a structural deep-equal snapshot would harden it. +- **Order-preservation test recomputes the expected order with the same predicate as the implementation** + — `tests/plugins.test.ts:368-379` (Confidence: 65%) — The expected `registryWorkflowOrder` is derived + with `filter(commands.length > 0)`, the exact predicate the implementation uses. This couples the test + to the implementation's classification rule rather than asserting against an independent expected + ordering, so a shared bug in the predicate would pass both. A hardcoded expected name sequence would + be a stronger oracle. +- **No assertion that `workflow` and `language` together exclude duplicates across buckets at the + element level** (Confidence: 62%) — Disjointness is tested by name (line 354-360); fine in practice + since plugin names are unique, but an explicit total-count-vs-unique-name check would be marginally + stronger. + +--- + +## Coverage Assessment vs. Prompt Checklist + +| Dimension asked about | Covered? | Test | +|----------------------|----------|------| +| Buckets (workflow / language) | Yes | lines 312-338 | +| Exclusions (core-skills / ambient / audit-claude in neither) | Yes | lines 340-346, 194-201 | +| Immutability (input not mutated) | Yes (shallow) | lines 362-366 | +| Source-order preservation | Yes (impl-coupled oracle) | lines 368-379 | +| Disjointness | Yes | lines 354-360 | +| Empty input | Yes | lines 381-385 | +| Completeness (workflow+language = selectable) | Yes | lines 348-352 | +| WORKFLOW_ORDER drop of /bug-analysis caught? | Partially — caught by explicit `contains` test (389) and forward guard; reverse-direction drift NOT caught (400-412) | — | +| Behavior-focused, not impl-coupled? | Mostly yes; two order/immutability tests lean toward impl coupling | — | +| Interactive loop logic testable/tested? | No — inline in init.ts, only static verification | MEDIUM finding above | + +--- + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 2 | - | +| Pre-existing | - | - | 0 | 0 | + +**Testing Score**: 8/10 +**Recommendation**: APPROVED_WITH_CONDITIONS + +The `partitionSelectablePlugins` test suite is strong and the function was properly extracted. Two +should-fix items remain: (1) extract and unit-test the interactive selection-loop decision logic +(the @clack flow itself is fairly left untested, but the bounded-retry/empty-bucket/combine logic +should not be), and (2) make the WORKFLOW_ORDER regression guard bidirectional so it catches stale +or dropped entries in both directions. Neither blocks merge. diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/typescript.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/typescript.md new file mode 100644 index 00000000..1f57ee9c --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1725/typescript.md @@ -0,0 +1,102 @@ +# TypeScript Review Report + +**Branch**: feat/init-flow-simplification -> main +**Date**: 2026-06-01_1725 +**Scope**: src/cli/plugins.ts, src/cli/commands/init.ts, tests/plugins.test.ts +**PR**: #232 + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +The TypeScript quality of this change is strong. `tsc --noEmit -p tsconfig.json` +passes cleanly with `strict: true`. No `any` types were introduced, no unsafe +runtime casts, and the new public function carries an explicit, correct return +type with a thorough doc comment. + +## Issues in Code You Touched (Should Fix) + +### MEDIUM +**Redundant `as string[]` casts on @clack multiselect results (2 occurrences)** — Confidence: 88% +- `src/cli/commands/init.ts:342` (`workflowSelected = step1 as string[]`) +- `src/cli/commands/init.ts:357` (`languageSelected = step2 as string[]`) +- Problem: `p.multiselect` is typed as `Promise`, and + `p.isCancel(value): value is symbol` is a proper type guard. After the + `if (p.isCancel(step1)) { ...process.exit(0); }` block, TypeScript already + narrows `step1` from `symbol | string[]` to `string[]` (the option `value`s + are `pl.name`, a `string`, so `Value` infers to `string`). The `as string[]` + assertion is therefore redundant — it asserts a type the compiler has already + proven. Redundant assertions are a mild code smell: they suppress future + compiler help if the option value type ever changes (e.g., to a union or + branded type), silently masking a real mismatch instead of erroring. +- Impact: Low runtime risk (the cast is correct today), but it weakens the + type-narrowing guarantee and trains readers that the cast is necessary when + it is not. +- Fix: Drop the assertions and let `isCancel` narrowing do the work: + ```typescript + // step1 is narrowed to string[] after the isCancel guard above + workflowSelected = step1; // was: step1 as string[] + ... + languageSelected = step2; // was: step2 as string[] + ``` +- Note: This pattern matches the **pre-existing** convention in the same file + (`pluginSelection as string[]` in the old code; `flagSelection as string[]` + at init.ts:710, `viewModeChoice as ...`, `teamsChoice as boolean`, etc.). + Because the new casts are consistent with established local style and are + type-correct, this is a should-fix consistency-with-best-practice item, not a + blocker. If the team prefers to keep the codebase-wide cast convention, + leaving these is defensible — but the cleaner direction is to remove them + (and ideally the pre-existing ones in a follow-up). + +## Pre-existing Issues (Not Blocking) +None within the reviewed scope that meet the CRITICAL bar for pre-existing code. +(The `as boolean` / `as 'user' | 'local'` cast convention on @clack `select` +results predates this PR and is out of scope.) + +## Suggestions (Lower Confidence) + +- **`WORKFLOW_ORDER` could be `readonly`/`as const`** - `src/cli/plugins.ts:701` + (Confidence: 65%) — Typed as mutable `string[]`. Since it is a fixed canonical + display order consumed read-only (`.filter`/`.indexOf`/`Set` in init.ts and + tests), `as const` or `readonly string[]` would express immutability in the + type and enable literal narrowing. Held below threshold because the existing + codebase convention in plugins.ts is plain mutable arrays + (`DEVFLOW_PLUGINS: PluginDefinition[]`, `LEGACY_RULE_NAMES: string[]`) with no + `as const` usage anywhere in the file — so `string[]` is the consistent choice + here, and changing it would diverge from local style. + +- **`EXCLUDED` Set is duplicated between source and test** - `src/cli/plugins.ts:723` + and `tests/plugins.test.ts:310` (Confidence: 60%) — The exclusion set is + re-declared verbatim in the test rather than imported/exported, so the two can + drift. Minor; arguably intentional to keep the test as an independent oracle. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 1 | - | +| Pre-existing | - | - | 0 | 0 | + +### What is correct (positive findings) +- `partitionSelectablePlugins` return type is **explicit and correct**: + `{ workflow: PluginDefinition[]; language: PluginDefinition[] }` + (plugins.ts:719-722). Function is pure, non-mutating, deterministic — and the + test suite verifies all of these (`does not mutate the input array`, + `buckets are disjoint`, `preserves ordering`, `empty input` edge case). +- `Set` is used appropriately for O(1) exclusion membership (`EXCLUDED.has`) + and elsewhere (`installedSet`, `workflowOrderSet`). +- `isCancel` symbol-result narrowing is handled correctly at every call site — + the `symbol` branch always `process.exit`s, so subsequent code sees the + narrowed value type. +- No `any` types; `JSON.parse` results are typed `unknown` and guarded + (`typeof parsed === 'object'`, `!Array.isArray`) before access (init.ts:1133). +- Explicit return type present on the new exported function. `WORKFLOW_ORDER` + has an explicit type annotation. +- `import type { PluginDefinition }` correctly uses a type-only import. + +**TypeScript Score**: 9/10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/architecture.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/architecture.md new file mode 100644 index 00000000..05e50283 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/architecture.md @@ -0,0 +1,83 @@ +# Architecture Review Report + +**Branch**: feat/init-flow-simplification -> main +**PR**: #232 +**Date**: 2026-06-01_1857 + +## Summary Assessment + +This is a clean, well-factored refactor. The two new pure helpers (`combineSelection`, +`shouldRetry` in init.ts; `partitionSelectablePlugins`, `WORKFLOW_ORDER` in plugins.ts) follow +good separation-of-concerns: pure decision logic is extracted from the I/O-bound prompt loop, +making it independently testable. The implementation faithfully matches the documented decisions +(applies ADR-010, applies ADR-011) and the `cli-rules` feature knowledge partition assumptions. +Prior-cycle false positives (test re-declaring EXCLUDED; both-empty precondition assert) are not +re-raised — verified still correct against current code. + +No blocking architectural issues found. + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) + +None of CRITICAL severity in untouched lines. The `init` action handler is a very large function +(~1150 lines), which is a pre-existing god-function concern, but the diff does not meaningfully +worsen it — the new logic was extracted into pure helpers rather than inlined, which slightly +improves the situation. Informational only; not blocking per the Iron Law. + +## Suggestions (Lower Confidence) + +- **Duplicated `EXCLUDED` exclusion set across modules** - `src/cli/plugins.ts:723` (Confidence: 65%) + — `partitionSelectablePlugins` defines `EXCLUDED = {core-skills, ambient, audit-claude}` as the + authoritative selectable-plugin filter, but downstream init logic re-derives core-skills/ambient + membership separately (e.g. `init.ts:964`, `init.ts:969` re-`find` those plugins to force-install + them). These are conceptually the same "always-installed / not-user-selectable" classification + expressed in two places. Consider a single `category` or `selectable` field on `PluginDefinition` + as the single source of truth (the helper's own comment at plugins.ts:732-735 anticipates exactly + this). Lower confidence because the two uses serve different purposes (selection filtering vs. + forced-install) and consolidating is a broader change than this PR's scope. avoids PF-005 (the + capability — a category field — does not yet exist, so verify before assuming). + +- **`partitionSelectablePlugins` couples "language" semantics to "command-less"** - + `src/cli/plugins.ts:729` (Confidence: 62%) — The bucket predicate is `commands.length > 0 ? + workflow : language`, which is a structural proxy for a semantic category. The inline comment + (plugins.ts:732-735) already documents this as an intentional, acknowledged simplification and + flags the future-proofing path. Matches the `cli-rules` KNOWLEDGE.md documented pattern + (workflow plugins carry commands; language plugins carry only rules). Noted as a watch-item, not + a defect — flagging only because the "language" label will silently mis-bucket any future + command-less non-language plugin. The code already calls this out, so no action needed now. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 1 | + +**Architecture Score**: 9 +**Recommendation**: APPROVED + +## Decision Alignment Notes + +- **applies ADR-010** — Interactive scope prompt removed; `scope` hardcoded to `'user'` for the + interactive TTY path (init.ts:210), `--scope` flag retained and validated (init.ts:215-221), + non-TTY auto-detection unchanged (init.ts:222-224). Implementation matches decision intent. +- **applies ADR-011** — Plugin selection split into two sequential `p.multiselect` calls (Step 1 + Workflow init.ts:355, Step 2 Language init.ts:371), partitioned by the pure + `partitionSelectablePlugins` helper. Combined non-empty validation via `combineSelection` + + bounded re-prompt (`MAX_ATTEMPTS = 3`, `shouldRetry`) — no custom grid. Matches decision intent. +- Bounded loop satisfies the reliability rule (no unbounded `while(true)`); exit-on-exhaustion is + handled by `shouldRetry` before the `while` condition re-evaluates, so there is no silent + fall-through with an empty `selectedPlugins` (verified init.ts:349-395). +- `WORKFLOW_ORDER` is exported and consumed read-only (`.filter`/`.indexOf`/Set membership only) — + no shared-mutable-state coupling risk. Bidirectional regression guards in tests are solid. diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/complexity.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/complexity.md new file mode 100644 index 00000000..85563cb3 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/complexity.md @@ -0,0 +1,89 @@ +# Complexity Review Report + +**Branch**: feat/init-flow-simplification -> main +**Date**: 2026-06-01 18:57 +**PR**: #232 + +## Scope + +Reviewed the init-flow simplification changes in `src/cli/commands/init.ts` and +`src/cli/plugins.ts`. Focus per orchestrator: the interactive two-step selection +loop and the new pure helpers (`partitionSelectablePlugins`, `combineSelection`, +`shouldRetry`, `WORKFLOW_ORDER`). applies ADR-011 (two-step plugin selection, +bounded re-prompt max 3, combined non-empty validation). + +Cross-cycle note: prior resolution (cycle 2) already performed a loop-clarity +refactor (separated "got selection" from "out of attempts"), extracted +`combineSelection`/`shouldRetry`/`toChoice`. The two prior false positives +(test re-declares EXCLUDED set; precondition assert for both-empty buckets) +were NOT re-raised — verified neither was re-introduced. + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +### MEDIUM +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +The new code is well-factored for complexity: + +- **`partitionSelectablePlugins`** (`plugins.ts:719-741`) — single loop, one + early `continue`, nesting depth 2, ~22 lines, cyclomatic complexity ~3. + Pure, documented, deterministic. Within all metric thresholds. applies ADR-011. +- **`combineSelection`** (`init.ts:129-135`) — 3 lines of logic, complexity 1. +- **`shouldRetry`** (`init.ts:144-147`) — 2 branches, complexity 2. +- **Bounded selection loop** (`init.ts:349-395`) — explicit `MAX_ATTEMPTS = 3` + bound (avoids unbounded-loop reliability pitfall), max nesting depth 3 + (`while` -> `if bucket non-empty` -> `if isCancel`), ~46 lines including two + symmetric multiselect blocks. The two `if (p.isCancel(...))` blocks are + near-duplicates but consolidating them would require wrapping `p.multiselect`, + which would reduce clarity more than the duplication costs — not worth flagging. + +## Pre-existing Issues (Not Blocking) + +**`initCommand.action()` handler is very long (~1150 lines)** — `init.ts:193-1347` +**Confidence**: 95% +- Problem: The `.action()` callback is a single ~1150-line function far exceeding + the >50-line CRITICAL threshold, with the Recommended/Advanced branch spanning + hundreds of lines of inline prompt construction. +- Category: Pre-existing — this PR adds only ~50 net lines to a function that was + already this size; the length is not introduced by this change. +- Per the Iron Law (never block for pre-existing issues) and the consolidation + rule (pre-existing reported only if CRITICAL), this is informational. A future + refactor could extract `collectAdvancedFeatureChoices()` and + `applyRecommendedDefaults()` to bring the handler under control, but that is + out of scope for this PR and should be a separate effort. + +## Suggestions (Lower Confidence) + +- **`shouldRetry`'s `accepted` parameter is dead at its sole callsite** - + `init.ts:390` (Confidence: 65%) — The only caller invokes `shouldRetry` after + an `if (accepted) break`, so `accepted` is always `false` there, making the + `if (accepted) return false` branch unreachable in production. The parameter + exists for standalone unit-testing of the predicate (intentional per prior + cycle's extraction), so this is a deliberate testability tradeoff, not a defect. + No change recommended. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 1 | 0 | + +**Complexity Score**: 9 +**Recommendation**: APPROVED + +The changes reduce complexity rather than add it: the prior single conflated +multiselect is replaced by two clearly-named steps backed by small pure helpers, +all within complexity/length/nesting thresholds. The retry loop is explicitly +bounded. No blocking or should-fix complexity issues in the diff. The only +notable item (the giant `.action()` handler) is pre-existing and informational. diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/consistency.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/consistency.md new file mode 100644 index 00000000..83b742a2 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/consistency.md @@ -0,0 +1,62 @@ +# Consistency Review Report + +**Branch**: feat/init-flow-simplification -> main +**PR**: #232 +**Date**: 2026-06-01_1857 + +## Summary of Analysis + +Reviewed the init-flow simplification against existing `init.ts` / `plugins.ts` conventions +(clack prompt patterns, helper naming, export style, JSDoc style). The changes are +notably consistent with the surrounding codebase — the new code reads like the old code. + +Cross-referenced against `DECISIONS_CONTEXT`: both relevant decisions directly bless this PR. +- **applies ADR-010** — scope-prompt removal keeps `--scope` flag + non-TTY auto-detection (diff `init.ts` retains both `options.scope` and `!process.stdin.isTTY` branches, only the interactive `p.select` scope prompt was deleted). +- **applies ADR-011** — two-step workflow/language multiselect partitioned by the pure `partitionSelectablePlugins` helper, exactly as the decision specifies. + +Cross-referenced against feature knowledge `cli-rules/KNOWLEDGE.md` (verified current, not stale +in the touched areas): the helper placement (`combineSelection`/`shouldRetry` in `init.ts`, +`partitionSelectablePlugins`/`WORKFLOW_ORDER` in `plugins.ts`) matches the documented convention verbatim. + +Cross-cycle awareness (PRIOR_RESOLUTIONS, cycle 2): the two prior false positives — (1) the test +re-declaring the `EXCLUDED` set as an independent oracle, and (2) a precondition assert for the +both-empty buckets case — were NOT re-raised. Verified neither was re-introduced by new code. + +### Consistency checks passed + +- **Naming** — `combineSelection`, `shouldRetry`, `partitionSelectablePlugins` are camelCase, matching existing helpers `buildAssetMaps`, `buildRulesMap`, `getAllRuleNames`, `isValidRuleName`. +- **Export style** — `export const WORKFLOW_ORDER: string[]` carries an explicit `string[]` annotation, matching the sibling exports `LEGACY_COMMAND_NAMES: string[]`, `LEGACY_SKILL_NAMES: string[]`, `LEGACY_RULE_NAMES: string[]`. (Note: the old inline `const WORKFLOW_ORDER` had no annotation — the moved/exported version is *more* consistent with module conventions, not less.) +- **clack prompt patterns** — both new `p.multiselect` steps follow the identical `isCancel → p.cancel(...) → process.exit(0)` shape used by every other prompt in the file. +- **pluginHints ordering** — Record key order (plan, implement, code-review, resolve, debug, explore, research, release, self-review, bug-analysis) exactly mirrors `DEVFLOW_PLUGINS` declaration order, which is what actually drives display via `partitionSelectablePlugins` (order-preserving). Functionally correct; not WORKFLOW_ORDER-keyed, and does not need to be. +- **WORKFLOW_ORDER `/bug-analysis` placement** — inserted after `/self-review`, consistent with the JSDoc-described pipeline order and asserted by the new regression-guard tests. +- **Test conventions** — new `describe`/`it` blocks match existing structure, message style, and `expect` assertion patterns in both test files. + +### Justified deviation (not a finding) + +- The two new multiselects use `required: false` whereas the removed single multiselect used `required: true`. This is a necessary and intentional change: empty-selection enforcement moved into the bounded retry loop via `combineSelection().accepted` + `shouldRetry`. It also aligns with the existing flags multiselect, which already uses `required: false`. Justified per the Iron Law (deviation has explicit rationale, documented inline and in ADR-011). + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None at CRITICAL severity (per Iron Law, only CRITICAL pre-existing issues are reported). + +## Suggestions (Lower Confidence) + +- **JSDoc "pure function" tagline phrasing varies slightly** — `src/cli/commands/init.ts:127` and `:142` say "Pure function — no I/O, no side effects; extracted for testability." while `src/cli/plugins.ts:716` says "Pure function — does not mutate the input array; ... deterministic; no I/O." (Confidence: 65%) — Same intent, minor wording drift across the three new pure helpers. Harmonizing the phrasing would marginally improve consistency, but this is style-preference territory and below the bar for a tracked finding. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Consistency Score**: 9/10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/performance.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/performance.md new file mode 100644 index 00000000..b0d292ff --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/performance.md @@ -0,0 +1,70 @@ +# Performance Review Report + +**Branch**: feat/init-flow-simplification -> main +**Date**: 2026-06-01_1857 +**PR**: #232 + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +## Issues in Code You Touched (Should Fix) +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) +None at >=80% confidence worth flagging. (Pre-existing `Array.includes` inside +`filter` at `init.ts:961` is O(n*m) but over a fixed ~21-element list run once +at install — sub-microsecond, not a concern.) + +## Suggestions (Lower Confidence) + +None. The change is algorithmically clean across its entire perf surface. + +## Analysis Notes + +This is a small interactive CLI change. Per the orchestrator hint, the perf +surface is limited to: (1) any accidental O(n^2) over plugin lists, and +(2) repeated work in the bounded selection loop. Both were examined directly: + +**`partitionSelectablePlugins` (`plugins.ts:719-741`)** — Single pass over the +plugin list with an `EXCLUDED` `Set` for O(1) membership checks and direct +`push` into pre-allocated buckets. O(n), no nested iteration, no allocation +churn. Clean. (applies ADR-011 — supports two-step selection.) + +**`combineSelection` (`init.ts:129-135`)** — Two-array spread, O(n). No issue. + +**`shouldRetry` (`init.ts:144-147`)** — Pure O(1) arithmetic. No issue. + +**Bounded selection loop (`init.ts:345-395`)** — Correctly bounded at +`MAX_ATTEMPTS = 3` (avoids unbounded retry). Critically, all derived data — +`workflowChoices`, `languageChoices`, and `workflowInitialValues` +(`init.ts:338-343`) — is computed ONCE *before* the loop. The loop body only +re-issues the interactive `p.multiselect` prompts, which is inherent to retry +semantics (the user must re-select). No `partitionSelectablePlugins` call, +`.map`, or `.filter` is repeated per attempt. No accidental O(n^2) over the +plugin list. This is the exact "repeated work in the bounded loop" risk the +hint called out, and it is not present. + +**`WORKFLOW_ORDER` consumption (`init.ts:1278-1279`)** — `installedSet` built +once via `flatMap` into a `Set`, then `WORKFLOW_ORDER.filter(cmd => +installedSet.has(cmd))` does O(n) iteration with O(1) lookups. Moving the array +from a local const to an exported module-level const has zero runtime cost. +Clean. + +**Cross-cycle awareness**: PRIOR_RESOLUTIONS (cycle 2) listed two false +positives (test EXCLUDED redeclaration; both-empty precondition assert) — +neither is performance-related, so no conflict and nothing re-raised. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Performance Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/regression.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/regression.md new file mode 100644 index 00000000..a575bcba --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/regression.md @@ -0,0 +1,92 @@ +# Regression Review Report + +**Branch**: feat/init-flow-simplification -> main +**PR**: #232 +**Date**: 2026-06-01_1857 + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +## Issues in Code You Touched (Should Fix) +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) +None of CRITICAL severity. (`/ambient` command is intentionally absent from `WORKFLOW_ORDER` +— it was never present in the old local copy either, and `devflow-ambient` is excluded from the +forward regression guard via `partitionSelectablePlugins`. Not a regression.) + +## Suggestions (Lower Confidence) + +- **Interactive `local`-scope install path is now unreachable** — `src/cli/commands/init.ts:209-225` + (Confidence: 70%) — Removing the interactive scope prompt (applies ADR-010) means interactive + TTY users can no longer select `local` scope and thus never reach the interactive-driven + `createDocsStructure` / `updateGitignore` / local `.claudeignore` branches. This is the + documented intent (ADR-010: "--scope flag and non-TTY auto-detection unchanged"), and + `--scope local` still reaches all of that logic, so it is behavior-by-design rather than a + defect. Noted only so a future reader does not mistake the now-dead interactive `local` path + for an accidental drop. + +## Regression Verification Performed + +The orchestrator flagged four specific regression risks. Each was verified against current code: + +1. **WORKFLOW_ORDER moved from init.ts to plugins.ts — other importers?** + `grep` across `src/` and `tests/` confirms the symbol is referenced only by `plugins.ts` + (definition), `init.ts:21` (import) + `init.ts:1279` (use), and `tests/plugins.test.ts`. + No external importer broke. The local duplicate in init.ts was deleted and replaced by the + import — value is identical except for the intentional `/bug-analysis` addition. CLI builds + clean (`tsc` exit 0). applies ADR-011. + +2. **`--scope` flag and non-TTY install path preserved?** + `init.ts:209-225` retains: `--hud-only` → user, `--scope` normalize+validate (exits on + invalid), `!isTTY` → user (with log line). Only the trailing interactive `else` (the scope + `p.select` prompt) was removed; `scope` falls through to its `'user'` initializer for + interactive TTY. Exactly matches ADR-010 ("hardcode interactive scope to user, while keeping + the --scope CLI flag and non-TTY auto-detection unchanged"). + +3. **Removed scope prompt dropped downstream scope-dependent logic?** + All scope consumers remain intact: `needsDiscovery = ... scope === 'user'`, multi-project + `.claudeignore`, `updateGitignore`/`createDocsStructure` (local), security deny-list + (`scope === 'user'`). None were deleted. The only consequence is that interactive runs always + take the `user` branch — see Suggestion above. No logic removed. + +4. **`partitionSelectablePlugins` exclusions dropped a previously-selectable plugin?** + Old single-multiselect filter excluded `{core-skills, ambient, audit-claude}`; new `EXCLUDED` + set is identical. Workflow bucket = `commands.length > 0` and non-excluded → plan, implement, + code-review, resolve, debug, explore, research, release, self-review, bug-analysis (all + non-optional → all pre-selected via `workflowInitialValues`, matching old `preSelected` + semantics). Language bucket = command-less optional plugins (typescript, react, accessibility, + ui-design, go, java, python, rust), none pre-selected (matches old behavior where optional + plugins were not pre-selected). No previously-selectable plugin dropped. avoids PF-007 (the + `commands.length > 0` partition contract is documented inline). + +Additional checks: +- **Empty-selection guard**: bounded retry loop (`MAX_ATTEMPTS = 3`), `attempts++` before steps, + `shouldRetry(attempts, MAX, accepted)` correctly returns false on attempt 3 → graceful cancel. + Old flow used `required: true` on a single multiselect; the new flow replaces that hard + requirement with a bounded re-prompt + cancel — functionally equivalent (cannot proceed with + zero plugins). Both steps now `required: false`, with non-emptiness enforced by `combineSelection`. +- **Forward + reverse WORKFLOW_ORDER guards** present (`tests/plugins.test.ts:400,414`), plus + duplicate-entry check. The reverse guard (prior-cycle addition) is intact. +- **Tests**: 116/116 pass (`tests/plugins.test.ts` + `tests/init-logic.test.ts`); `combineSelection` + (5 cases), `shouldRetry` (4 cases), `partitionSelectablePlugins` (8 cases) all covered. + +## Cross-Cycle Awareness +PRIOR_RESOLUTIONS parsed (Cycle 2). The two prior false positives — (1) test re-declaring the +EXCLUDED set, (2) precondition assert for both-empty buckets — were NOT re-raised. The prior +"reverse WORKFLOW_ORDER regression guard" fix is verified present at `tests/plugins.test.ts:414`. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Regression Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/reliability.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/reliability.md new file mode 100644 index 00000000..f9853909 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/reliability.md @@ -0,0 +1,83 @@ +# Reliability Review Report + +**Branch**: feat/init-flow-simplification -> main +**Date**: 2026-06-01_1857 +**PR**: #232 + +## Scope + +Reviewed the two-step plugin-selection retry loop and supporting pure helpers +(`combineSelection`, `shouldRetry`, `partitionSelectablePlugins`, `WORKFLOW_ORDER`) +in `src/cli/commands/init.ts` and `src/cli/plugins.ts`. Focus per orchestrator: verify +the re-prompt loop is bounded, terminates on exhaustion, handles `isCancel` on both +multiselects, and that the both-empty / language-only branches cannot infinite-loop +or crash. applies ADR-011 (bounded re-prompt loop — max 3 attempts, graceful cancel). + +## Verification Findings (no issues — bounds confirmed) + +The reliability Iron Law (every loop has a fixed upper bound, terminates explicitly) +is satisfied. Traced the loop at `init.ts:349-395`: + +- **Bound**: `while (attempts < MAX_ATTEMPTS)` with `MAX_ATTEMPTS = 3` and `attempts++` + as the first statement of the body. Fixed upper bound of 3 iterations. avoids the + unbounded-retry anti-pattern. +- **Termination paths**: (1) `accepted` → `break` (line 387); (2) exhaustion → + `process.exit(0)` via `!shouldRetry(...)` (lines 390-392); (3) natural `while` + exit when `attempts` reaches 3. No fall-through to installation with an empty + selection — `selectedPlugins` is only assigned inside the `accepted` branch. +- **Off-by-one check**: on the 3rd attempt `attempts === 3`, `shouldRetry(3, 3, false)` + returns `3 < 3 === false`, so `!shouldRetry` is true and the flow exits. Confirmed + exactly 3 prompts maximum, never a 4th, never a skipped exhaustion. Verified against + the `shouldRetry` tests in `tests/init-logic.test.ts`. +- **isCancel handling**: both multiselects guard `p.isCancel` and call + `p.cancel(...)` + `process.exit(0)` (Step 1 lines 361-364; Step 2 lines 376-379). + No unhandled cancel sentinel can leak into `combineSelection`. +- **Language-only path**: when `workflowChoices.length === 0`, Step 1 is skipped and + `workflowSelected` stays `[]`; Step 2 alone can satisfy `accepted`. `combineSelection` + returns `accepted = plugins.length > 0`, so a language-only selection is accepted + correctly and the loop terminates. No crash, no infinite loop. +- **Non-TTY fallback**: non-TTY without `--plugin` leaves `selectedPlugins = []`, which + later resolves to `DEVFLOW_PLUGINS.filter(p => !p.optional)` (line 960). Defined, + unchanged fallback — no undefined-deref risk. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None within the reliability lens for the changed regions. + +## Suggestions (Lower Confidence) + +None at reportable confidence. (See Cross-Cycle note below regarding the both-empty +precondition assert, which is intentionally omitted.) + +## Cross-Cycle Awareness + +Per `PRIOR_RESOLUTIONS` (Cycle 2), two findings were classified FALSE_POSITIVE and are +NOT re-raised: + +1. Test re-declares the `EXCLUDED` set — intentional. Verified still intentional; not + re-raised. +2. Precondition assert for the both-empty buckets case — the team decided the assert is + noise over value. I independently traced the both-empty degenerate case: the loop + skips both prompts, warns up to 3 times, then `process.exit(0)`. This is **bounded + and terminating** (no Iron Law violation), so I found no new evidence of an + unbounded or crashing path that would justify overriding the prior decision. Not + re-raised. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Reliability Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/review-summary.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/review-summary.md new file mode 100644 index 00000000..e13c1a38 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/review-summary.md @@ -0,0 +1,66 @@ +# Code Review Summary + +**Branch**: feat/init-flow-simplification -> main +**Date**: 2026-06-01_1857 + +## Merge Recommendation: APPROVED + +All 9 specialized reviewers (Security, Architecture, Performance, Complexity, Consistency, Regression, Testing, Reliability, TypeScript) returned APPROVED with zero blocking issues. The init-flow simplification is production-ready. + +**Rationale**: +- Zero CRITICAL/HIGH blocking issues across all domains +- Zero MEDIUM should-fix issues in changed code +- All new pure helpers (`combineSelection`, `shouldRetry`, `partitionSelectablePlugins`) are well-factored, tested (116/116 pass), and type-safe +- Two-step plugin selection with bounded retry (max 3 attempts) satisfies ADR-010 (scope prompt removal) and ADR-011 (two-step selection) +- Strong cross-cycle convergence: Cycle 1 FP ratio was 22%, well below 70% threshold; both prior false positives (test EXCLUDED oracle, both-empty precondition assert) correctly stand as intentional design choices + +## Issue Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | Total | +|----------|----------|------|--------|-----|-------| +| Blocking | 0 | 0 | 0 | - | 0 | +| Should Fix | - | 0 | 0 | - | 0 | +| Pre-existing | - | - | 0 | 0 | 0 | + +## Critical & High Bugs + +None found. + +## Action Items + +No mandatory fixes required for merge. The following are optional, low-priority suggestions from reviewers (60-70% confidence): + +1. **Duplicated EXCLUDED exclusion logic** (Architecture, 65%) — `partitionSelectablePlugins` and downstream init logic both define/derive the core-skills/ambient/audit-claude membership. Consider a single `category` or `selectable` field on `PluginDefinition` as future consolidation (out of scope for this PR, flagged for awareness). + +2. **JSDoc phrasing consistency** (Consistency, 65%) — The three new pure helpers use slightly variant "pure function" taglines (init.ts vs plugins.ts). Harmonizing wording would improve style consistency, but this is below the actionable bar. + +3. **WORKFLOW_ORDER immutability hardening** (TypeScript, 65%) — Exporting as `readonly string[]` or `as const` would prevent accidental mutation by importers. Current `string[]` is used correctly (read-only operations only) and compiles cleanly; this is a nice-to-have. + +4. **Interactive local-scope path now unreachable** (Regression, 70%) — Removing the interactive scope prompt (ADR-010) means interactive TTY users can no longer select `local` scope and reach local-config branches. This is documented intent; `--scope local` still works. No defect, just a code-path observation for future readers. + +5. **Loop integration test coverage** (Testing, 62%) — The two-step multiselect loop's branch wiring and `isCancel` guards are thin glue over tested pure helpers. A focused integration harness would be nice-to-have but not necessary (risk is low given the tested underpinnings). + +## Verification Checklist + +| Item | Status | Details | +|------|--------|---------| +| TypeScript Compilation | ✅ Clean | `tsc --noEmit` passes with `strict: true` | +| Test Suite | ✅ 116/116 Pass | `vitest run` — partitionSelectablePlugins (8 cases), combineSelection (5), shouldRetry (4), WORKFLOW_ORDER (4) | +| Security | ✅ Approved | Plugin names allowlisted, `--scope` validated, managed-settings path intact, pure helpers carry no attack surface | +| Architecture | ✅ Approved | Well-factored pure helpers, good separation of concerns, ADR-010/ADR-011 faithfully implemented | +| Performance | ✅ Approved | O(n) partition algorithm, bounded retry loop (no repeated work in loop body), no accidental O(n^2) | +| Complexity | ✅ Approved | New code within all thresholds (nesting ≤3, cyclomatic ≤3 per helper), explicitly bounded retry | +| Consistency | ✅ Approved | Naming, export style, clack patterns, pluginHints ordering all match existing conventions | +| Regression | ✅ Approved | WORKFLOW_ORDER move verified (only init.ts imports), `--scope` flag and non-TTY paths preserved, exclusion list identical, no previously-selectable plugins dropped | +| Testing | ✅ Approved | Behavior-focused, covers edge cases (empty buckets, exhaustion), no spying on internals, AAA structure throughout | +| Reliability | ✅ Approved | Retry loop has fixed bound (MAX_ATTEMPTS=3), terminates explicitly on exhaustion, no fall-through with empty selection, `isCancel` handled on both multiselects | + +## Convergence Status + +**Cycle**: 2 +**Prior Resolution**: Available +**Prior FP Ratio**: 2/9 = 22% (Cycle 1) +**Assessment**: Converging cleanly — Cycle 1 delivered 7 fixes and 2 correctly-identified false positives. This cycle shows no reversion of prior fixes and no regression of intentional design choices. FP ratio well below 70% threshold. + +--- + +**Next step**: Merge to main. diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/security.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/security.md new file mode 100644 index 00000000..b08a412e --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/security.md @@ -0,0 +1,84 @@ +# Security Review Report + +**Branch**: feat/init-flow-simplification -> main +**PR**: #232 +**Date**: 2026-06-01_1857 +**Cycle**: 2 + +## Scope + +The diff touches `src/cli/commands/init.ts`, `src/cli/plugins.ts`, and two test files. +This is an interactive CLI init-flow refactor: scope prompt removal (always user scope +interactively, `applies ADR-010`), two-step plugin selection (`applies ADR-011`), and a +`/bug-analysis` entry added to the post-install command list. No network I/O, database +access, authentication, cryptography, secret handling, or output-rendering surface is +introduced or modified. + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +## Issues in Code You Touched (Should Fix) +None. + +## Pre-existing Issues (Not Blocking) +None at CRITICAL severity. Per the Iron Law, pre-existing non-critical issues in untouched +lines are not reported. + +## Suggestions (Lower Confidence) +None. + +## Security Verification Notes + +The following security-relevant aspects of the diff were verified and found sound — recorded +for traceability, not as findings: + +1. **Plugin-name input is allowlisted** — `--plugin` values flow through + `parsePluginSelection(...)` which rejects unknown names and exits before use + (`init.ts:299-306`). Interactive selections come from a fixed `DEVFLOW_PLUGINS` registry + via `partitionSelectablePlugins`. No user-controlled free text reaches filesystem path + construction (`DEVFLOW_PLUGINS.filter(...)`, `init.ts:961`). No path-traversal or injection + surface introduced. + +2. **`--scope` remains validated** — Commander enforces `/^(user|local)$/i` and the action + re-validates against `'user' | 'local'` (`init.ts:215-222`). The interactive scope prompt + removal (`applies ADR-010`) hardcodes interactive scope to `user`; `--scope local` and + non-TTY auto-detection are unchanged. No security control is bypassed. + +3. **Managed-settings / sudo flow intact** — The security deny-list and managed-settings + (sudo) path still require explicit `scope === 'user' && isTTY` gating plus a separate + confirmation step (`init.ts:846-896`, `1251-1258`). The scope change does not auto-elevate + or silently write system files; `managedSettingsConfirmed` defaults false and recommended + mode deliberately stays on `user` security mode. + +4. **Pure helpers carry no attack surface** — `combineSelection`, `shouldRetry` + (`init.ts:129-147`), and `partitionSelectablePlugins` (`plugins.ts:719-741`) are pure array + operations with no I/O, deserialization, or dynamic dispatch. + +5. **Bounded re-prompt loop** — `MAX_ATTEMPTS = 3` prevents an unbounded prompt loop; + exhaustion cancels cleanly via `process.exit(0)` before any filesystem mutation, so no + partial-write or resource-leak exposure. + +6. **Cancellation is pre-mutation** — All `isCancel` branches on both multiselects exit before + the installation phase begins (`init.ts:361-364, 376-379, 391-393`), leaving no + half-written settings/manifest state. + +## Cross-Cycle Awareness + +PRIOR_RESOLUTIONS parsed successfully. The two prior false positives (test re-declares +EXCLUDED set; precondition assert for both-empty buckets) are non-security and were not +re-raised. No prior Fixed issue was reverted in this diff. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Security Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/testing.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/testing.md new file mode 100644 index 00000000..4060d219 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/testing.md @@ -0,0 +1,46 @@ +# Testing Review Report + +**Branch**: feat/init-flow-simplification -> main +**Date**: 2026-06-01_1857 +**PR**: #232 + +## Summary of Coverage Assessment + +The new pure helpers are well-covered as behavior-focused unit tests (applies ADR-011 — two-step selection, combined non-empty validation, bounded re-prompt max 3): + +- **`partitionSelectablePlugins`** (8 cases in `tests/plugins.test.ts`): workflow bucketing, language bucketing, exclusion of all 3 EXCLUDED plugins, total coverage (workflow+language = selectable count), disjointness, no-mutation, ordering preservation, and empty-input. Strong — asserts observable output (bucket membership, counts, order), not internals. +- **`combineSelection`** (5 cases) + **`shouldRetry`** (4 cases) in `tests/init-logic.test.ts`: workflow-only, language-only, both-buckets, both-empty rejection, ordering; retry true/false across accepted, attempts-remaining, ceiling-reached, and beyond-ceiling. Covers the edge cases called out in the focus brief (empty buckets / out-of-attempts). +- **`WORKFLOW_ORDER`** (4 cases): contains `/bug-analysis`, `/bug-analysis` after `/self-review`, forward regression guard (every workflow command present), reverse regression guard (every entry is a real command), no duplicates. The `/bug-analysis` regression guard requested in the PR description is present and correct. + +All 116 tests across the two files pass (`vitest run`). Suite is behavior-focused, no spying on internals, no flaky patterns, AAA structure throughout. + +Per Cross-Cycle Awareness: the two prior false positives (independent EXCLUDED test-oracle re-declaration at `plugins.test.ts:310`; precondition assert for both-empty buckets) are NOT re-raised — both remain valid intentional choices and current code has not re-introduced the underlying concern. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. The selection-loop orchestration (`init.ts:349-395`) is intentionally thin glue over the now-tested pure helpers; the residual untested surface (`p.multiselect` calls, `p.isCancel` branches, `process.exit(0)`) requires DI/extraction of the @clack prompts, which would be over-extraction inconsistent with the prior-cycle FP ruling. Not blocking. + +## Pre-existing Issues (Not Blocking) + +None relevant to testing focus. + +## Suggestions (Lower Confidence) + +- **`combineSelection` lacks an immutability assertion on inputs** - `tests/init-logic.test.ts:79` (Confidence: 65%) — The function spreads both args into a new array (correct, non-mutating), and `partitionSelectablePlugins` has an explicit no-mutation test, but `combineSelection` has no parallel assertion that `workflowSelected`/`languageSelected` are left untouched. Low value since the spread is obviously pure, but it would make the immutability contract symmetric across the two new pure helpers. + +- **No test exercises the empty-bucket → loop-skip interaction end-to-end** - `init.ts:352-381` (Confidence: 62%) — The loop's `if (workflowChoices.length > 0)` / `if (languageChoices.length > 0)` guards (the language-only and workflow-only paths through the *real loop*, plus the both-empty-bucket guard) are covered only indirectly via `partitionSelectablePlugins([])` + the `combineSelection`/`shouldRetry` units. The guards themselves are simple and the helpers are tested, so the risk is low; a focused harness would only matter if the loop's branch wiring later drifts. The `isCancel` exit paths (`init.ts:361,376`) are likewise unverified — acceptable given they are thin `process.exit(0)` glue. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Testing Score**: 9 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/typescript.md b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/typescript.md new file mode 100644 index 00000000..13be9603 --- /dev/null +++ b/.devflow/docs/reviews/feat-init-flow-simplification/2026-06-01_1857/typescript.md @@ -0,0 +1,69 @@ +# TypeScript Review Report + +**Branch**: feat/init-flow-simplification -> main (PR #232) +**Date**: 2026-06-01 18:57 + +## Summary of Analysis + +Scope: type safety of the new init-flow helpers in `src/cli/plugins.ts` and +`src/cli/commands/init.ts` — `partitionSelectablePlugins`, `combineSelection`, +`shouldRetry`, the exported `WORKFLOW_ORDER`, and the two-step `@clack` +multiselect / `isCancel` narrowing. + +Verification performed: +- `tsc --noEmit -p tsconfig.json` → clean (zero errors). `strict: true` is on. +- Confirmed `@clack` types directly (avoids PF-005 — checked rather than assumed): + `isCancel(value: unknown): value is symbol` and + `multiselect(opts): Promise`. +- Confirmed `PluginDefinition` shape and helper signatures match the documented + `cli-rules` knowledge base (verified against current code per staleness note; + signatures still accurate). + +**Result: no blocking, should-fix, or pre-existing TypeScript issues.** The new +code is type-sound. One low-confidence stylistic suggestion below. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None within the TypeScript focus area. + +## Suggestions (Lower Confidence) + +- **`WORKFLOW_ORDER` could be `readonly`/`as const`** - `src/cli/plugins.ts:701` (Confidence: 65%) — Exported as mutable `string[]`. It is a canonical display-order constant consumed only via read-only operations (`.filter`, `.indexOf`, `.includes`) in `init.ts:1278-1279`. Typing it `as const` (or `readonly string[]`) would prevent accidental mutation by any importer and document intent. Not a defect — purely a hardening nicety; the current typing compiles cleanly and is used correctly. + +## Notes on Verified Non-Issues + +- **`@clack` narrowing is sound (no casts needed)** — In both `step1`/`step2` + branches, the `if (p.isCancel(stepN)) process.exit(0)` guard narrows the + `symbol | string[]` union to `string[]` (since options derive from + `toChoice` → `value: pl.name` which is `string`, so `Value` infers as + `string`). The subsequent `workflowSelected = step1` / `languageSelected = step2` + assignments to `string[]` are type-safe with no `as string[]` cast. This + confirms the prior-cycle removal of redundant `as string[]` casts was correct + and complete — re-adding them would be redundant. (Prior FP — not re-raised.) +- **`combineSelection` / `shouldRetry` return types** — Both have explicit return + types (`{ plugins: string[]; accepted: boolean }` and `boolean`), are pure, and + carry no `any`. Sound. +- **`partitionSelectablePlugins` return type** — Explicit + `{ workflow: PluginDefinition[]; language: PluginDefinition[] }`; no mutation of + input; no `any`. Sound. +- Per Cross-Cycle Awareness: the two prior false positives (test re-declaring the + `EXCLUDED` set as an independent oracle; the both-empty-bucket precondition + assert) were verified against current code and are **not** re-raised. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**TypeScript Score**: 9/10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/architecture.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/architecture.md new file mode 100644 index 00000000..a663788a --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/architecture.md @@ -0,0 +1,129 @@ +# Architecture Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-01_2352 + +## Summary Verdict + +This is a clean subtractive refactor. The cleanup-via-toggle pattern is **architecturally +sound** and explicitly sanctioned by ADR-001 (clean-break philosophy permits "one-time cleanup +items: legacy hook file removal" as the named exception to no-migration). No new abstractions +introduced; the change collapses ambient to a single component (preamble hook) and reduces the +ambient.ts surface area (81 lines net reduction in source, 102 → fewer in tests). No SOLID, +coupling, or layering violations found in the changed lines. No dangling references to the +removed exports (`installCommandsRule`, `COMMANDS_RULE_CONTENT`, `removeCommandsRule`). No +migration entry added to `migrations.ts` — avoids PF-001, applies ADR-001. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. The refactor is internally consistent and well-documented. + +## Pre-existing Issues (Not Blocking) + +None surfaced that meet the CRITICAL bar for pre-existing reporting. + +## Focus-Question Findings + +### 1. Is the cleanup-via-toggle pattern (vs migration) architecturally sound? — YES + +The pattern delegates legacy-file purging to `removeLegacyCommandsRule()`, invoked +unconditionally from both `addAmbientHook` (ambient.ts:103) and `removeAmbientHook` +(ambient.ts:124), and reached on every `devflow init` via the always-run +`removeAmbientHook` → conditional `addAmbientHook` pass (init.ts:1131-1132). + +This is the correct home for the cleanup, not a migration, because: +- The `commands` rule was **never** part of the plugin rules system — it was always + ambient-managed (confirmed: `commands` appears in no plugin manifest `rules` array, and + `LEGACY_RULE_NAMES` was already `[]` on main, untouched by this PR). The generic + rule-cleanup loop (init.ts:1065) operates on plugin-system rules; routing an + ambient-managed file through it would have leaked an ambient concern into the generic + rules path. Keeping cleanup in ambient.ts preserves the existing ownership boundary + documented in the cli-rules KNOWLEDGE.md. +- ADR-001 names "legacy hook file removal" as a permitted clean-break exception. A rule + file purged via the feature's own toggle is the direct analogue — applies ADR-001. +- It avoids PF-001 (no compat/migration layer added for a removal refactor). + +The `removeLegacyCommandsRule` error contract is correct: swallows ENOENT (idempotent), +propagates EACCES and other errors (ambient.ts:63-69). This matches the established +ambient hook-cleanup discipline. + +### 2. Is COMMANDS_RULE_PATH retention justified? — YES + +`COMMANDS_RULE_PATH` (ambient.ts:21) must survive because it is the only remaining +identifier of the file to purge. `COMMANDS_RULE_CONTENT` was correctly deleted (no longer +written), but the path constant is load-bearing for the cleanup and for the test assertion +(`tests/ambient.test.ts` asserts `fs.unlink` called with `COMMANDS_RULE_PATH`). The doc +comment was updated to reflect its new purpose (purge-only). Retention is necessary and +correctly scoped — no architectural debt. + +### 3. Does the ordering invariant (cleanup before early-return) belong here? — YES, with a minor durability note + +The invariant — `removeLegacyCommandsRule()` must run *before* the `if (!changed) return` +early-return in `addAmbientHook` (ambient.ts:103-105) and before +`if (!removedPrompt && !removedClassification) return` in `removeAmbientHook` +(ambient.ts:124-126) — is the right design. If purge ran after the early-return, an install +whose hook is already registered would silently skip cleanup, leaving the stale file +forever. Placing the unconditional side-effect before the conditional return is the cleanest +expression of "settings unchanged, but disk side-effect still required." + +This ordering is now protected by an explicit regression test +(`tests/ambient.test.ts`: "purges legacy rule even when preamble hook already present +(ordering invariant)"), which is the correct safeguard for an invariant that is otherwise +invisible to the type system. This is the right home — the invariant is intrinsic to these +two functions' contracts and is co-located with the code it governs. + +Minor observation (LOW, non-blocking): the two functions now mix a pure settings transform +(returns new JSON string, no I/O) with an impure filesystem side-effect +(`removeLegacyCommandsRule`). This is a small departure from "compose pure transforms" +(global engineering principle #4/immutability and the project's parse-at-boundary leaning). +It is justified here by the pragmatic need to guarantee the side-effect across all call +paths (init + enable + disable) without duplicating the call at three sites, and the +side-effect is idempotent and well-documented. Not worth changing — flagging only for +awareness that `addAmbientHook` is no longer a pure function despite its +string-in/string-out signature. + +### 4. Boundary between ambient-managed rules and the plugin rules system — CLEAN + +The boundary is correctly preserved and, if anything, simplified: +- Plugin rules system: `LEGACY_RULE_NAMES` (plugins.ts:693, still `[]`) + the init.ts:1065 + loop handle *plugin-declared* rule cleanup. Untouched by this PR. +- Ambient-managed cleanup: `removeLegacyCommandsRule` handles the one historically + ambient-owned rule file. Self-contained in ambient.ts. + +There is no overlap or double-ownership: the two mechanisms target disjoint sets (plugin +rules vs the single ambient `commands.md`). `shared/rules/commands.md` was deleted, the +ambient plugin manifest declares no `rules` field (so the build's "declared rule missing +from shared/rules" guard cannot trip), and no plugin manifest references `commands`. The +KNOWLEDGE.md was updated to reflect the removal accurately. The boundary remains a clean +single-owner-per-file model. + +## Suggestions (Lower Confidence) + +- **Eventual removal of `removeLegacyCommandsRule` + `COMMANDS_RULE_PATH`** - + `src/cli/commands/ambient.ts:21,63` (Confidence: 70%) — Per the KNOWLEDGE.md pruning + convention for `LEGACY_RULE_NAMES` ("entries can be removed after 2 major versions"), + this purge path is itself temporary cleanup code. Consider adding a TODO/version marker + noting when it can be deleted, so it does not become permanent cruft (the exact concern + ADR-001 guards against). + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 1 | + +**Architecture Score**: 9 +**Recommendation**: APPROVED + +Rationale: Subtractive refactor that removes a redundant component, preserves all ownership +boundaries, applies ADR-001 and avoids PF-001, and ships with a regression test for the one +non-obvious invariant (cleanup-before-early-return). The single LOW note (impure side-effect +inside an otherwise-pure transform) is justified and not worth changing. The 70% suggestion +(version-marking the temporary purge code) is the only forward-looking hygiene item. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/complexity.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/complexity.md new file mode 100644 index 00000000..d3b32e63 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/complexity.md @@ -0,0 +1,52 @@ +# Complexity Review Report + +**Branch**: refactor-remove-ambient-commands-rule (PR #233) -> main +**Date**: 2026-06-01_2352 +**Focus**: Verify the code-removal refactor actually reduced complexity, left no dead code, no orphaned constants/imports, no vestigial indirection, and no confusing leftover comments. + +## Summary of Verification + +This is a net-deletion refactor (+150 / -233). I verified each removal-specific concern: + +| Check | Result | +|-------|--------| +| Removed symbols (`COMMANDS_RULE_CONTENT`, `installCommandsRule`, `removeCommandsRule`) fully gone | PASS — zero references in `src/` or `tests/` | +| `COMMANDS_RULE_PATH` still genuinely used (not vestigial) | PASS — referenced by `removeLegacyCommandsRule` (unlink target) + 2 call sites + tests | +| `removeLegacyCommandsRule` wired into both add/remove paths | PASS — `ambient.ts:103` and `ambient.ts:124` | +| Orphaned imports after deletion | PASS — `path` import in test still used at line 478+; `tsc --noEmit` clean | +| Dead test scaffolding | PASS — `installCommandsRule`/`COMMANDS_RULE_CONTENT` describe blocks + dual-source drift guard removed; replacement ordering-invariant test added | +| Build/manifest references to deleted `shared/rules/commands.md` | PASS — ambient plugin `rules: []` was already empty (rule was always ambient.ts-managed, never in manifest); no build logic references the deleted file | +| Plugin metadata cleanup (`plugin.json` keywords/description) | PASS — `commands`/`awareness` keywords removed, description narrowed | +| TypeScript typecheck | PASS — `npx tsc --noEmit` produced no errors | + +The refactor genuinely reduces complexity: it removes a ~27-line embedded heredoc constant (`COMMANDS_RULE_CONTENT`), an entire `installCommandsRule` function, a dual-source synchronization invariant (the constant had to be kept byte-identical with `shared/rules/commands.md`, guarded by a drift test), and the corresponding test surface. The remaining `removeLegacyCommandsRule` is a focused single-purpose cleanup function. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) + +None reported (Iron Law: pre-existing issues block only if CRITICAL; none found). + +## Suggestions (Lower Confidence) + +- **Stale-ish historical comment referencing "commands rule"** - `src/cli/commands/init.ts:1088` (Confidence: 65%) — The legacy-cleanup comment `// Ambient simplification: session-start-classification removed (plan detection + commands rule)` still names "commands rule" as a thing the removed classification hook related to. Now that the commands rule itself is also gone, the parenthetical reads slightly anachronistically. It is a historical annotation on a legacy-file purge list (not load-bearing), so this is borderline. Optional: trim to `(plan detection)` for clarity. Not flagged higher because the comment documents *why* a legacy file is being purged, and the reference is historically accurate. + +- **Filesystem side-effect inside JSON-transform functions** - `src/cli/commands/ambient.ts:103,124` (Confidence: 60%) — `addAmbientHook`/`removeAmbientHook` are named/shaped like pure `(json) -> json` transforms but perform an `fs.unlink` via `removeLegacyCommandsRule`. This mixed concern predates the PR (the old code called `installCommandsRule` in the same spot writing a file), so the refactor does not worsen it — it is arguably cleaner now (unlink vs mkdir+write). Noting only because the refactor was an opportunity to separate the purge into the CLI action handler. Not blocking; out of scope for a deletion-only PR. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Complexity Score**: 9 +**Recommendation**: APPROVED + +The refactor accomplishes its stated goal cleanly. Complexity is genuinely reduced (removed a duplicated heredoc constant, a synchronization invariant + its drift test, and an install function). No dead code, no orphaned constants or imports, no vestigial indirection. `COMMANDS_RULE_PATH` remains genuinely used as the unlink target for purging legacy files from prior installs. The two suggestions are minor, out-of-scope-for-deletion-PR observations, both below the 80% reporting threshold. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/consistency.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/consistency.md new file mode 100644 index 00000000..ff4c85f8 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/consistency.md @@ -0,0 +1,58 @@ +# Consistency Review Report + +**Branch**: refactor-remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-01_2352 +**Focus**: Naming consistency, JSDoc style, docs-sweep internal consistency (rule count + command-awareness removal) + +## Summary of Verification + +- **Actual rule count**: `ls shared/rules/*.md` → **12 files** (accessibility, engineering, go, java, python, quality, react, reliability, rust, security, typescript, ui-design). The `commands.md` rule was deleted. Documentation claiming "12 rules" is therefore CORRECT against ground truth. +- **Docs sweep**: CLAUDE.md (3 spots), docs/cli-reference.md, docs/commands.md, plugin.json, plugin README, cli-rules KNOWLEDGE.md, and the Project Structure tree were all updated to "12 rules" / single-component ambient. +- **One straggler found**: README.md still says "13 ... engineering principles" (see HIGH below). +- **Naming**: `removeLegacyCommandsRule` follows the established `remove{Noun}` + `LEGACY_` qualifier conventions. Consistent. +- **Decisions alignment**: The legacy-file purge is explicitly sanctioned by ADR-001 ("one-time cleanup items (legacy hook file removal)") and avoids PF-001 (no migration/compat scaffolding added). Positive. + +## Issues in Your Changes (BLOCKING) + +### HIGH + +**Docs sweep straggler — README.md still claims "13" rules** — `README.md:56` +**Confidence**: 96% +- Problem: 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) load on every prompt...`. The committed README diff only touched lines 19 and 44 (the two "command awareness" sentences) and missed the count on line 56. Ground truth is 12 rules (`shared/rules/*.md` = 12 files), and the sibling docs (CLAUDE.md line 65, cli-rules KNOWLEDGE.md line 30, Project Structure tree line 82) all now say "12". This is the exact internal-consistency drift the sweep was meant to eliminate. +- Impact: User-facing README contradicts CLAUDE.md and the actual shipped rule set. The PR description states the docs were "swept ... to reflect 12 rules" — this line falsifies that claim. +- Fix: Update `README.md:56` from `13 ultra-condensed engineering principles` to `12 ultra-condensed engineering principles`. (The parenthetical language list "TypeScript, React, Go, Python, Java, Rust" omits Java/accessibility/ui-design but that phrasing pre-dates this PR and is out of scope.) + +## Issues in Code You Touched (Should Fix) + +_None._ The `ambient.ts` rename, JSDoc rewrites, and test updates are internally consistent: +- `removeLegacyCommandsRule` matches the `remove{Hook|MemoryHooks|ContextHook|HudStatusLine}` verb-noun family, and the `Legacy` qualifier is consistent with the codebase's `LEGACY_HOOK_FILES` / `LEGACY_RULE_NAMES` / `LEGACY_SKILL_NAMES` / `LEGACY_COMMAND_NAMES` naming. +- `COMMANDS_RULE_PATH` retained as an UPPER_SNAKE const (matches `PREAMBLE_HOOK_MARKER`). +- The removed `D1:` JSDoc on `COMMANDS_RULE_CONTENT` left no orphaned D-series references (grep confirmed clean). +- No stale references to `installCommandsRule` / `removeCommandsRule` / `COMMANDS_RULE_CONTENT` anywhere in src/tests/docs/plugins (grep confirmed clean). +- Test file `tests/ambient.test.ts` import + comments + the new ordering-invariant test (`purges legacy rule even when preamble hook already present`) all match the new behavior. + +## Pre-existing Issues (Not Blocking) + +**Stale `referencedFiles` entry in feature index** — `.devflow/features/index.json:26` +**Confidence**: 88% +- Problem: The `cli-rules` knowledge base's `referencedFiles` array still lists `shared/rules/commands.md`, which this PR deletes. The KNOWLEDGE.md body (committed) was correctly updated to "12 rules total", but the index entry pointing at the now-deleted file would mark the KB perpetually `[STALE]` via git-log staleness detection. +- Why not blocking: `git diff main...HEAD` shows `.devflow/features/index.json` has **No changes** in this PR's commits — it appears in `git status` only as uncommitted working-tree state, so it is outside this PR's committed scope. +- Fix (separate, or fold into this PR if index.json gets committed): remove `"shared/rules/commands.md"` from the `cli-rules.referencedFiles` array via `devflow knowledge refresh cli-rules`, or hand-edit the entry. + +## Suggestions (Lower Confidence) + +_None._ + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 1 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 1 | 0 | + +**Consistency Score**: 8/10 +**Recommendation**: CHANGES_REQUESTED + +The refactor is clean and the naming/JSDoc/test consistency is excellent. One straggler (README.md "13" rules) directly contradicts the rest of the swept docs and the actual 12-file rule set — it should be fixed before merge to satisfy the PR's own stated goal. The index.json stale reference is informational (uncommitted, out of scope). diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/documentation.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/documentation.md new file mode 100644 index 00000000..fcaaddc9 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/documentation.md @@ -0,0 +1,83 @@ +# Documentation Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-01_2352 +**Focus**: Doc-heavy sweep — verify accuracy and completeness of the commands-rule removal sweep + +## Scope Verified + +Ground-truth checks performed against current code/filesystem: +- `shared/rules/*.md` count = **12** (commands.md removed) — confirms the "12 rules" target. +- JSDoc on `COMMANDS_RULE_PATH`, `removeLegacyCommandsRule`, `addAmbientHook`, `removeAmbientHook` in `src/cli/commands/ambient.ts`. +- Repo-wide grep for "command awareness", "commands rule", "13 rules". +- KNOWLEDGE.md `referencedFiles` and "12 rules" body claims. + +--- + +## Issues in Your Changes (BLOCKING) + +### HIGH +**Stale rule count: README.md still says "13 rules"** — `README.md:56` +**Confidence**: 98% +- Problem: The PR description states "13→12 rules in two places," but `README.md:56` was not updated: + > "**Always-on rules.** 13 ultra-condensed engineering principles (~10 lines each) load on every prompt..." + Every other doc was correctly updated to 12 (CLAUDE.md:65, CLAUDE.md:82, KNOWLEDGE.md lines 30/249, cli-reference, plugin.json). This single line is now the only place claiming 13, directly contradicting the actual `shared/rules/` count (12) and the rest of the sweep. README is the user-facing front door, so this is the most visible drift. +- Impact: Actively misleading — users reading the headline feature list get a wrong count that conflicts with `devflow rules --list`. +- Fix: Change "13 ultra-condensed engineering principles" → "12 ultra-condensed engineering principles". +- Category: Blocking (line is part of the doc sweep this PR owns, even though the diff did not touch it; the sweep's stated goal was to fix all rule-count mentions). +- Note: `README.md:67` ("3 MCPs 2 rules" in the HUD sample) is a per-project *installed-count* example, NOT the total. It is correct and should not be changed. + +--- + +## Issues in Code You Touched (Should Fix) + +### MEDIUM +**Inaccurate PF citation in KNOWLEDGE.md** — `.devflow/features/cli-rules/KNOWLEDGE.md:241` +**Confidence**: 90% +- Problem: New gotcha line added in this PR states: + > "The workflow-bucket predicate is `commands.length > 0` — the language-bucket comment notes this implicit contract is **PF-007** (source only; not enforced by types)." + Two inaccuracies: (1) the actual source comment in `src/cli/plugins.ts:732-735` makes **no PF reference at all** — it just describes the implicit contract in plain English; (2) **PF-007** in `.devflow/decisions/pitfalls.md:60` is "Editing globally installed hook scripts directly instead of source + rebuild + reinstall" — an unrelated topic. The citation points the reader to a pitfall that has nothing to do with plugin-bucket partitioning. +- Impact: A future maintainer who follows the "PF-007" pointer to understand the partition contract lands on an unrelated hook-editing pitfall — code-comment drift / wrong cross-reference. (Note: PF-007 is not in this review's DECISIONS_CONTEXT index; verified directly against pitfalls.md.) +- Fix: Either (a) drop the "is PF-007" claim and just say "documented as an inline comment in `plugins.ts`", or (b) if a pitfall genuinely covers this, cite the correct PF number. Also verify the parenthetical "the language-bucket comment notes..." matches what `plugins.ts` actually says. +- Category: Should-Fix (file authored in this PR; not user-facing, but it is consumed automatically as FEATURE_KNOWLEDGE by every workflow, so inaccuracy propagates). + +--- + +## Pre-existing Issues (Not Blocking) + +None identified relevant to this focus. + +--- + +## Verified Accurate (no action needed) + +These were explicitly checked against ground truth and are correct: + +1. **CLAUDE.md Ambient Mode section (line 49)** — Now "Single-component system," describes only the `preamble` plan-detection hook plus the auto-removal of the legacy `commands.md` rule. Internally consistent, no self-contradiction, no remaining claim that a live commands rule exists. Accurate. +2. **Rule count = 12 everywhere except README:56** — CLAUDE.md:65, CLAUDE.md:82 (project-structure tree), KNOWLEDGE.md:30/249, cli-reference.md:59, plugin.json, ambient README all correctly say 12 / drop the "+1 ambient-managed (commands)" clause. +3. **JSDoc accuracy** — `COMMANDS_RULE_PATH` (ambient.ts:15-21) correctly states the path now exists only to purge the legacy file; `removeLegacyCommandsRule` (58-62) accurately documents idempotency and ENOENT-only swallowing; `addAmbientHook` (71-77) and `removeAmbientHook` (109-115) accurately document the unconditional pre-early-return purge. Matches implementation. +4. **KNOWLEDGE.md `referencedFiles`** — Never listed `shared/rules/commands.md` (lists the 4 core rules + source files); adds `src/cli/commands/ambient.ts`. No permanent-staleness risk from a removed referenced file. +5. **No doc tells users a commands-awareness rule exists** — All surviving `commands.md` mentions are either (a) intentional descriptions of the auto-purge of the *legacy* file, (b) the unrelated `docs/commands.md` / `references/commands.md` files, or (c) historical "Related" context in KNOWLEDGE.md:266. None present it as a current feature. +6. **plugin.json keywords** — `"commands"` and `"awareness"` removed; description reduced to "Plan auto-detection." Accurate. +7. **README.md:19 / :44 and docs/commands.md:173 / ambient README** — All "two-component"/"command awareness" prose correctly reduced to single plan-detection component. + +--- + +## Suggestions (Lower Confidence) + +None. + +--- + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 1 | 0 | - | +| Should Fix | - | 0 | 1 | - | +| Pre-existing | - | - | 0 | 0 | + +**Documentation Score**: 8/10 +**Recommendation**: CHANGES_REQUESTED + +The sweep is ~95% complete and high quality — CLAUDE.md, plugin.json, cli-reference, ambient README, and most of KNOWLEDGE.md are accurate and self-consistent, and the JSDoc faithfully reflects the new purge-only behavior. Two fixes block approval: (1) one missed rule-count instance in the user-facing README (13→12), and (2) an inaccurate PF-007 cross-reference in the cli-rules knowledge base. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/performance.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/performance.md new file mode 100644 index 00000000..7a627586 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/performance.md @@ -0,0 +1,68 @@ +# Performance Review Report + +**Branch**: refactor-remove-ambient-commands-rule (PR #233) -> main +**Date**: 2026-06-01 23:52 + +## Scope & Verification + +PR removes the always-on `commands.md` ambient rule (`shared/rules/commands.md`, 26 +lines) and collapses ambient mode to the preamble hook only. A new +`removeLegacyCommandsRule()` purges the stale rule file from prior installs. + +The review focus was: (1) confirm no new per-prompt / per-session I/O, and (2) confirm +the new `fs.unlink` is not on a hot path. + +**Verification performed:** + +- `scripts/hooks/preamble` (the per-prompt UserPromptSubmit hook) — **unchanged** in this + diff (`git diff main...HEAD -- scripts/hooks/preamble` returns nothing). The hot path is + untouched. Confirmed. +- `removeLegacyCommandsRule()` (`ambient.ts:63-69`) performs a single `fs.unlink` on + `~/.claude/rules/devflow/commands.md`, swallowing ENOENT. +- Call-graph traced — `removeLegacyCommandsRule` is invoked only from `addAmbientHook` + (`ambient.ts:103`) and `removeAmbientHook` (`ambient.ts:124`). Those two are called only + from: + - `ambient.ts:211,223` — `devflow ambient --enable/--disable` CLI handlers + - `init.ts:1131-1132` — one-time `devflow init` settings pass + - `uninstall.ts:275,371` — `devflow uninstall` + + All are cold, operator-invoked CLI commands. None run per-prompt or per-session at + runtime. The unlink is NOT on any hot path. Confirmed. + +**Net runtime effect:** one fewer always-on rule file loaded into every Claude Code +session context (a per-session context reduction, as the PR claims). No new runtime I/O +is introduced — the only added I/O (`fs.unlink`) lives on cold CLI paths. The plan's +performance claims hold. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None relevant to performance. + +## Suggestions (Lower Confidence) + +- **Idempotent unlink runs unconditionally on every init/enable/disable** - + `ambient.ts:103,124` (Confidence: 70%) — `removeLegacyCommandsRule` issues an `fs.unlink` + syscall even when the legacy file was already purged on a prior run, so steady-state + `devflow init` runs pay one wasted ENOENT syscall. This is negligible (sub-100µs, cold + path, runs at most once per CLI invocation) and the unconditional design is intentional + per the doc comment (guarantees cleanup). Not worth changing; noted only for + completeness. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Performance Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/regression.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/regression.md new file mode 100644 index 00000000..3e729e59 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/regression.md @@ -0,0 +1,104 @@ +# Regression Review Report + +**Branch**: refactor-remove-ambient-commands-rule (PR #233) -> main +**Date**: 2026-06-01_2352 +**Diff command**: `git diff main...HEAD` + +## Summary of Verification + +This PR removes the ambient "commands awareness" rule feature. Regression-relevant +changes: +- Removed exports: `installCommandsRule`, `COMMANDS_RULE_CONTENT` +- Renamed export: `removeCommandsRule` → `removeLegacyCommandsRule` +- Behavior change in `addAmbientHook`: previously WROTE the rule (`installCommandsRule`), + now DELETES the legacy rule (`removeLegacyCommandsRule`) +- Deleted source: `shared/rules/commands.md` +- Removed the corresponding test blocks (`installCommandsRule`, `COMMANDS_RULE_CONTENT`), + renamed the `removeCommandsRule` block, added an ordering-invariant test + +All five focus concerns from the review brief were verified and cleared. No regressions found. + +This is a clean-break removal consistent with the project's standing philosophy +(`applies ADR-001` — no migration code for refactors; `avoids PF-001` — no backward-compat +shim/alias/deprecated-flag layer). The retained `removeLegacyCommandsRule` is a one-time +purge of a file previously written into users' `~/.claude/rules/devflow/`, not a compat shim. + +## Focus-Concern Verification + +1. **Removed exports have no non-test consumers** — VERIFIED. + `grep` across `src/`, `scripts/`, `tests/` for `installCommandsRule`, `COMMANDS_RULE_CONTENT`, + and the old `removeCommandsRule` name returns zero matches (exit 1). A repo-wide `.ts` sweep + (excluding node_modules/dist) also returns nothing. `npm run build:cli` (tsc) compiles clean — + confirms no dangling imports. + +2. **`addAmbientHook` behavior change is safe for callers** — VERIFIED. + Sole production caller is `src/cli/commands/init.ts:1131-1132`: + `removeAmbientHook(content)` then conditional `addAmbientHook(cleaned, devflowDir)`. + - When ambient enabled: both functions run; both purge the legacy rule (idempotent). + - When ambient disabled: only `removeAmbientHook` runs; it still purges the legacy rule. + Nothing in init.ts or the standalone command relies on the rule being *written* — the only + downstream effect was the now-removed `commands.md` file. No consumer reads that file + programmatically (no `rules/commands` reference exists in src/scripts/shared/plugins). + +3. **Ordering invariant (cleanup before early-return)** — VERIFIED. + In `addAmbientHook` (ambient.ts:102-105) `await removeLegacyCommandsRule()` runs *before* + `if (!changed) return settingsJson`. In `removeAmbientHook` (ambient.ts:123-126) the purge + likewise runs before `if (!removedPrompt && !removedClassification) return`. The standalone + `ambient --enable` "already enabled" early-return (ambient.ts:212-215) skips only the settings + *write*; the rule purge already executed inside `addAmbientHook` as a side effect. No no-op path + leaves a stale file behind. A dedicated regression test now asserts this + ("purges legacy rule even when preamble hook already present (ordering invariant)", + tests/ambient.test.ts:94-105). + +4. **`preamble` hook genuinely unchanged** — VERIFIED. + `git diff main...HEAD --name-only | grep preamble` returns nothing; the preamble hook script + is not in the diff. + +5. **No test still expects the old write behavior** — VERIFIED. + The `installCommandsRule` describe block and the `COMMANDS_RULE_CONTENT` describe block (which + asserted `fs.writeFile` and content sync against `shared/rules/commands.md`) are both deleted. + The `removeCommandsRule` block is renamed to `removeLegacyCommandsRule`. `beforeEach` stubs were + flipped from `fs.writeFile`/`fs.mkdir` to `fs.unlink`. Full suite: `npx vitest run tests/ambient.test.ts` + → 40 passed, 0 failed. + +### Additional checks +- **Build integrity of deleting `shared/rules/commands.md`**: The CLAUDE.md build contract fails + if a *declared* rule is missing. `devflow-ambient/.claude-plugin/plugin.json` has no `rules` array + and no `commands` reference — the rule was managed by ambient.ts directly, not the plugin rules + system. No other plugin.json declares `commands`. `npm run build:cli` succeeds. No build regression. +- **Doc/manifest consistency**: `plugins.ts`, plugin.json description/keywords, init.ts `--ambient` + option text, and the ambient command help text were all updated to drop "command awareness". + Coherent with the feature removal. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None relevant to this diff. + +## Suggestions (Lower Confidence) + +- **Operational rebuild/reinstall dependency** — `src/cli/commands/ambient.ts` (Confidence: 65%) — + The legacy-rule purge only fires on ambient enable/disable/init code paths. Users who never re-run + one of those paths will retain a stale `~/.claude/rules/devflow/commands.md` indefinitely. This is + acceptable under the clean-break philosophy (the rule is inert and harmless), but worth a one-line + note in release docs (relates to PF-003's rebuild-and-reinstall theme). Not a code regression. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Regression Score**: 10 +**Recommendation**: APPROVED + +Decisions cited: applies ADR-001, avoids PF-001. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/reliability.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/reliability.md new file mode 100644 index 00000000..501a207f --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/reliability.md @@ -0,0 +1,134 @@ +# Reliability Review Report + +**Branch**: refactor-remove-ambient-commands-rule -> main (PR #233) +**Date**: 2026-06-01_2352 +**Focus**: Error-handling correctness in `removeLegacyCommandsRule` and its callers + +## Verdict on the Posed Question + +The PR_DESCRIPTION states the design intent: *"removeLegacyCommandsRule unlinks a path, +swallows ENOENT, propagates other errors. Called before the early-return in +addAmbientHook/removeAmbientHook."* + +**Assessment: propagating non-ENOENT errors (EACCES etc.) from a best-effort legacy-file +purge is the wrong reliability posture.** A cleanup-of-a-deprecated-artifact step should be +fail-safe — its failure must never abort the primary operation (enabling/disabling ambient +mode, or initializing devflow) that previously succeeded. This violates the project +reliability rule's *"fail-safe defaults"* principle and creates a new failure mode where a +cosmetic cleanup error breaks unrelated, working functionality. + +The clean-break purge itself is appropriate and aligns with the user's standing clean-break +philosophy (relevant context: PF-001) — the issue is purely the error-propagation posture of +the cleanup, not the decision to purge. + +--- + +## Issues in Your Changes (BLOCKING) + +### HIGH + +**Cleanup failure aborts the entire init settings-configuration pass** — `src/cli/commands/ambient.ts:63-69`, surfacing at `src/cli/commands/init.ts:1131-1132` +**Confidence**: 90% +- Problem: In `init.ts`, `removeAmbientHook` (line 1131) and `addAmbientHook` (line 1132) + both call `removeLegacyCommandsRule()` unconditionally, before their early-returns. These + run at the very top of a single read-modify-write block that subsequently configures + **memory hooks, HUD statusLine, the always-on context hook, Claude Code flags, and + viewMode** (lines 1134-1166). The whole block is wrapped in a silent `catch {}` at + `init.ts:1176` whose comment only anticipates `settings.json may not exist yet`. +- Impact: If `~/.claude/rules/devflow/commands.md` exists but is not writable/unlinkable + (EACCES, EPERM, EBUSY on Windows, EROFS), `removeLegacyCommandsRule` now throws. The throw + fires before memory/HUD/context/flags/viewMode are applied, so `content` is never written + (line 1168 is never reached) and the exception is silently eaten by the outer `catch {}`. + Net effect: a single unwritable *deprecated* rule file silently disables memory hooks, HUD, + the context hook, and flag configuration during `devflow init` — none of which have anything + to do with the commands rule. Before this PR there was no commands-rule unlink in this path, + so this is a newly introduced regression in a previously-working init flow. +- Fix: Make the legacy purge non-fatal. Swallow all errors from the cleanup (it is a + best-effort purge of a deprecated artifact), or at most log a warning. Preferred: + ```typescript + export async function removeLegacyCommandsRule(): Promise { + 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. + // (use the caller's logger if one is threaded through, otherwise swallow) + } + } + ``` + If observability matters, thread a logger or return a `Result`/boolean the caller can + surface as a dim warning — but do not `throw`. + +**`devflow ambient --enable` now throws for a user lacking write perms to the legacy rule file** — `src/cli/commands/ambient.ts:103` and `:124` +**Confidence**: 88% +- Problem: Unlike `init.ts`, the `ambientCommand` action (lines 210-230) has **no + surrounding try/catch** around `addAmbientHook`/`removeAmbientHook`. A propagated EACCES + from `removeLegacyCommandsRule` therefore escapes to the top level and crashes the command + with an unhandled rejection / stack trace. +- Impact: A user who previously could run `devflow ambient --enable` (or `--disable`) + successfully will now get a hard failure — and crucially, the failure is in cleanup of a + *deprecated* file, after which the actual hook write at `ambient.ts:217`/`:228` never + happens. The primary operation regresses from "succeeds with a stale file left behind" to + "fails entirely." Fail-safe defaults dictate that a deprecated-artifact purge must not be + able to block enabling the feature. +- Fix: Same root fix as above — make `removeLegacyCommandsRule` non-fatal. Once the cleanup + cannot throw on EACCES, both the init path and the ambient-command path retain their prior + success behavior while still purging the file whenever permissions allow. + +--- + +## Issues in Code You Touched (Should Fix) + +### MEDIUM + +**No test covers the non-ENOENT error path** — `tests/ambient.test.ts:16-105, 168` +**Confidence**: 85% +- Problem: Every test stubs `fs.unlink` to resolve successfully + (`vi.spyOn(fs, 'unlink').mockResolvedValue(undefined)`). The branch that this PR + deliberately introduces — `if (code !== 'ENOENT') throw err` at `ambient.ts:67` — has zero + coverage. The ENOENT-swallow branch is also untested. +- Impact: The exact behavior the PR description calls out as the design decision (propagate + non-ENOENT) is unverified, so a future change to the error posture (e.g. the fail-safe fix + recommended above) could silently flip behavior with no failing test. For a function whose + whole reason to exist is error discrimination, the error branches are the contract. +- Fix: Add two cases: (1) `fs.unlink` rejects with `{ code: 'ENOENT' }` → `removeLegacyCommandsRule` + resolves (no throw); (2) `fs.unlink` rejects with `{ code: 'EACCES' }` → assert the chosen + posture. If you adopt the fail-safe fix, this case asserts it resolves without throwing; if + the current propagate-posture is kept, it asserts it rejects. Either way the contract is pinned. + +--- + +## Pre-existing Issues (Not Blocking) + +None material to reliability in the changed files. The `filterHookEntries` / +`isAmbient` / `isClassification` helpers are bounded (single `.filter`/`.some` over a +finite settings array) and contain no unbounded loops, retries, or allocation hot paths. +The devflow-dir inference block (`ambient.ts:193-208`) already guards `JSON.parse` with a +try/catch and falls back to a canonical default — that is correct fail-safe behavior and a +good model for the cleanup function to follow. + +## Suggestions (Lower Confidence) + +- **Cleanup ordering vs. write atomicity** — `ambient.ts:103`, `:124` (Confidence: 65%) — the + legacy purge runs before the settings write, so on the `--enable` path you delete the rule + file and then may still fail to write settings.json (line 217). The two side effects aren't + atomic. Low impact since the rule file is deprecated, but worth noting that the purge commits + an irreversible delete before the primary write succeeds. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 2 | - | - | +| Should Fix | - | - | 1 | - | +| Pre-existing | - | - | 0 | 0 | + +**Reliability Score**: 6 +**Recommendation**: CHANGES_REQUESTED + +The two HIGH findings share one root cause: a best-effort cleanup of a deprecated artifact +is allowed to throw on EACCES/EPERM, which aborts a previously-working operation. The answer +to the posed question is that cleanup failures here should be **non-fatal** (fail-safe), not +propagated. Making `removeLegacyCommandsRule` swallow (or log-and-continue on) all errors +resolves both blocking findings; add the EACCES/ENOENT tests to pin the contract. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/resolution-summary.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/resolution-summary.md new file mode 100644 index 00000000..1d869217 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/resolution-summary.md @@ -0,0 +1,42 @@ +# Resolution Summary + +**Branch**: refactor/remove-ambient-commands-rule -> main +**Date**: 2026-06-01_2352 +**Review**: .devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352 +**Command**: /resolve + +## Decisions Citations + +- applies ADR-001 — batch-1, batch-2 (clean-break cleanup; no migration added) +- avoids PF-007 — batch-2, cli-rules-KNOWLEDGE.md:241 (removed fabricated citation per verbatim-only rule) + +## Statistics +| Metric | Value | +|--------|-------| +| Total Issues | 3 | +| Fixed | 3 | +| False Positive | 0 | +| Deferred | 0 | +| Blocked | 0 | + +## Fixed Issues +| Issue | File:Line | Commit | +|-------|-----------|--------| +| removeLegacyCommandsRule propagated non-ENOENT errors — could abort `devflow init` (silent catch disabled memory/HUD/flags) and crash `ambient --enable/--disable`. Made fail-safe: swallows ALL errors (best-effort cleanup of a deprecated file must never abort the caller). | src/cli/commands/ambient.ts:65-73 | 424ca9a | +| Coupled test flip: `removeLegacyCommandsRule` EACCES case re-asserted from `.rejects` to `.resolves` to pin the new fail-safe contract. | tests/ambient.test.ts:~341-345 | 424ca9a | +| Rule-count straggler: README user-facing line said "13 ultra-condensed engineering principles"; actual count is 12 (shared/rules/*.md). | README.md:56 | 424ca9a | +| Inaccurate cross-reference: gotcha cited PF-007 (about editing installed hook scripts) for the partition predicate. Dropped the fabricated citation; replaced with a plain description matching the source comment. | .devflow/features/cli-rules/KNOWLEDGE.md:241 | 424ca9a | + +## False Positives +(none) + +## Deferred to Tech Debt +(none) + +## Blocked +(none) + +## Notes +- All three review findings (1 HIGH reliability, 1 HIGH consistency/documentation, 1 MEDIUM documentation) fixed directly — no tech debt deferred. +- Two parallel Resolver batches raced on `git commit`; git serialized them into a single commit (`424ca9a`). All four file changes landed intact. The commit MESSAGE describes only the docs fixes and under-describes the bundled `ambient.ts` reliability fix + test flip — cosmetic message inaccuracy only; the diff itself is complete and correct. Commit was already pushed, so amending the message would require a force-push (not done without explicit approval). +- Verification: `npm run build` clean (reports 12 rules); `npx vitest run tests/ambient.test.ts` → 40 pass / 0 fail. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/review-summary.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/review-summary.md new file mode 100644 index 00000000..d03691f1 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/review-summary.md @@ -0,0 +1,121 @@ +# Code Review Summary + +**Branch**: refactor-remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-01_2352 + +## Merge Recommendation: CHANGES_REQUESTED + +Two blocking issues must be resolved before merge: + +1. **Reliability**: `removeLegacyCommandsRule` propagates non-ENOENT errors (EACCES, EPERM), causing cleanup failures to abort `devflow init` and `ambient --enable/--disable` entirely. A best-effort cleanup of a deprecated artifact must be fail-safe and never block primary operations. + +2. **Documentation**: README.md:56 still claims "13 ultra-condensed engineering principles" when the actual rule set is 12 (commands.md deleted). This directly contradicts all other docs (CLAUDE.md, KNOWLEDGE.md, cli-reference) and undermines the PR's stated goal of a complete rule-count sweep. + +Additionally, one should-fix issue in cli-rules KNOWLEDGE.md cites an incorrect PF number for the plugin-bucket partition contract. + +--- + +## Issue Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 3 | 0 | - | +| Should Fix | - | 0 | 1 | - | +| Pre-existing | - | - | 0 | 1 | + +--- + +## Blocking Issues + +**1. Cleanup failure aborts init settings-configuration pass** — `src/cli/commands/ambient.ts:63-69` +- **Severity**: HIGH +- **Confidence**: 90% +- **Problem**: `removeLegacyCommandsRule` propagates EACCES/EPERM/EBUSY errors from `fs.unlink`. In `init.ts:1131-1132`, this function is called unconditionally before applying memory hooks, HUD, context hooks, flags, and viewMode. If the legacy `~/.claude/rules/devflow/commands.md` file is not writable, the throw aborts the entire settings-configuration block. The outer `catch {}` silently swallows the error, so users get no feedback that their init failed. +- **Impact**: A previously-working `devflow init` now silently fails to apply memory/HUD/flags/viewMode configuration due to a cleanup failure on a *deprecated* file unrelated to those features. Before this PR, there was no unlink in this path — this is a regression. +- **Fix**: Make the legacy purge non-fatal. Swallow all errors from cleanup (it is best-effort removal of a deprecated artifact): + ```typescript + export async function removeLegacyCommandsRule(): Promise { + 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 deprecated file; never abort caller's + // primary operation (enable/disable/init). Log warning if desired, + // but do not throw. + } + } + ``` + +**2. `devflow ambient --enable` crashes on cleanup failure** — `src/cli/commands/ambient.ts:103` and `:124` +- **Severity**: HIGH +- **Confidence**: 88% +- **Problem**: Unlike `init.ts`, the `ambientCommand` action (lines 210-230) wraps no try/catch around `addAmbientHook`/`removeAmbientHook`. A propagated EACCES from `removeLegacyCommandsRule` escapes to the top level as an unhandled rejection with stack trace. +- **Impact**: A user who previously could run `devflow ambient --enable` successfully will now get a hard crash if the legacy rule file is not writable — a regression from prior success to failure. The failure occurs in cleanup of a *deprecated* file, after which the actual hook write never happens (fail-safe defaults violated). +- **Fix**: Same root fix as issue #1 — make `removeLegacyCommandsRule` non-fatal. Once cleanup cannot throw, both `init.ts` and the ambient command paths retain prior success behavior. +- **Note**: This is the same root cause as issue #1 (error propagation), but surfaces differently in the two call paths (silently eaten vs. unhandled). + +**3. README.md still claims "13 rules" instead of 12** — `README.md:56` +- **Severity**: HIGH +- **Confidence**: 98% (deduplicated from consistency + documentation reviewers) +- **Problem**: The PR description states "13→12 rules" in a complete sweep, but `README.md:56` was not updated: + > "**Always-on rules.** 13 ultra-condensed engineering principles (~10 lines each) load on every prompt..." + All other docs were correctly updated (CLAUDE.md:65, CLAUDE.md:82, KNOWLEDGE.md:30/249, cli-reference, plugin.json). This single line in the user-facing README is now the only place claiming 13 — directly contradicting the actual `shared/rules/` count (12 = accessibility, engineering, go, java, python, quality, react, reliability, rust, security, typescript, ui-design). +- **Impact**: Users reading the headline feature list get a wrong count that conflicts with `devflow rules --list` and all internal docs. Actively misleading during the most visible user interaction (README). +- **Fix**: Change "13 ultra-condensed engineering principles" → "12 ultra-condensed engineering principles" at `README.md:56`. +- **Note**: `README.md:67` ("3 MCPs 2 rules" in the HUD sample) is a per-project *installed-count* example and is correct. + +--- + +## Suggestions (Lower Confidence) + +**No test covers the non-ENOENT error path** — `tests/ambient.test.ts`, `src/cli/commands/ambient.ts:66-68` +- **Severity**: MEDIUM +- **Confidence**: 85% +- **Problem**: All tests stub `fs.unlink` to resolve successfully. The error-handling branch — `if (code !== 'ENOENT') throw err` — has zero coverage. The ENOENT-swallow branch is also untested. +- **Impact**: The exact behavior the PR calls out (propagate non-ENOENT) is unverified. A future change to error handling could flip behavior silently with no failing test. +- **Fix**: After fixing the error-propagation issue (#1/#2), add two test cases: + 1. `fs.unlink` rejects with `{ code: 'ENOENT' }` → function resolves (no throw) + 2. `fs.unlink` rejects with `{ code: 'EACCES' }` → function resolves without throwing (after fix) + +--- + +## Should-Fix Issues + +**Inaccurate PF citation in KNOWLEDGE.md** — `.devflow/features/cli-rules/KNOWLEDGE.md:241` +- **Severity**: MEDIUM +- **Confidence**: 90% +- **Problem**: New gotcha text states: + > "The workflow-bucket predicate is `commands.length > 0` — the language-bucket comment notes this implicit contract is **PF-007** (source only; not enforced by types)." + Two inaccuracies: (1) the actual source comment in `src/cli/plugins.ts:732-735` makes **no PF reference**; (2) **PF-007** (`.devflow/decisions/pitfalls.md:60`) is "Editing globally installed hook scripts directly instead of source + rebuild + reinstall" — unrelated to plugin-bucket partitioning. +- **Impact**: A future maintainer following the PF-007 pointer lands on an unrelated pitfall; code-comment drift. KNOWLEDGE.md is auto-consumed as FEATURE_KNOWLEDGE by every workflow, so inaccuracy propagates. +- **Fix**: Either drop the "is PF-007" claim and cite the actual source comment, or find/create the correct pitfall if one genuinely covers this topic. + +--- + +## Pre-existing Issues (Not Blocking) + +**Eventual removal of temporary cleanup code** — `src/cli/commands/ambient.ts:21,63` +- **Severity**: LOW (informational only) +- **Confidence**: 70% +- **Problem**: Per KNOWLEDGE.md pruning convention, cleanup code like `removeLegacyCommandsRule` + `COMMANDS_RULE_PATH` can be removed after 2 major versions. Currently unmarked. +- **Recommendation**: Consider adding a version marker or TODO noting when this temporary code can be deleted, so it doesn't become permanent cruft (the exact concern ADR-001 guards against). + +--- + +## Convergence Status + +**Cycle**: 1 (first review) +**Prior Resolution**: None (no prior cycles) +**Assessment**: First cycle — no baseline comparison available + +--- + +## Action Plan + +1. **Fix error propagation in `removeLegacyCommandsRule`** — Make cleanup non-fatal by swallowing all errors except distinguishing ENOENT. This unblocks both the init path and the ambient-command path. +2. **Update README.md:56** — Change "13 ultra-condensed" → "12 ultra-condensed". +3. **Fix or drop the PF-007 citation** in cli-rules KNOWLEDGE.md:241 — verify against actual source and correct the cross-reference. +4. **Add error-path tests** for `removeLegacyCommandsRule` after fixing the error contract. +5. **Consider version-marking** the temporary purge code for eventual removal (optional, LOW priority). diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/security.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/security.md new file mode 100644 index 00000000..79b38312 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/security.md @@ -0,0 +1,91 @@ +# Security Review Report + +**Branch**: refactor-remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-01_2352 + +## Summary of Review Scope + +Focused on the filesystem deletion path introduced by this PR +(`removeLegacyCommandsRule` -> `fs.unlink`), its error handling, and any +path-injection surface. Reviewed `src/cli/commands/ambient.ts`, +`src/cli/commands/init.ts`, `src/cli/plugins.ts`, `tests/ambient.test.ts`, +and the deleted `shared/rules/commands.md`. Docs changes are out of scope for +a security lens. + +No blocking, should-fix, or pre-existing security issues at >=80% confidence. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None at CRITICAL severity. (Per the Iron Law, only CRITICAL pre-existing +issues are reported; none found.) + +## Suggestions (Lower Confidence) + +None. + +## Security Assessment Detail + +The deletion path was examined closely and is sound: + +- **No path-injection surface** (`ambient.ts:21`, `ambient.ts:63-69`). + `COMMANDS_RULE_PATH` is a hardcoded constant derived solely from + `os.homedir()` via `path.join(os.homedir(), '.claude', 'rules', 'devflow', + 'commands.md')`. No user input, CLI argument, env var, or settings.json + value flows into the path passed to `fs.unlink`. There is no traversal, + concatenation, or interpolation. The unlink targets a single fixed file. + +- **Correct, scoped error handling** (`ambient.ts:64-68`). ENOENT is swallowed + for idempotency; all other errors (e.g. EACCES) propagate via `throw err`. + This aligns with the "fail honestly" principle — it does not silently + swallow permission errors. Behavior is verified by tests at + `tests/ambient.test.ts:329-345` (exists, ENOENT-swallow, EACCES-rethrow). + +- **Ordering invariant is safe** (`ambient.ts:102-103`, `ambient.ts:123-124`). + The purge runs unconditionally before the early-return in both + `addAmbientHook` and `removeAmbientHook`. Since the target is fixed and the + operation idempotent, running it on every enable/disable/init carries no + security risk (no TOCTOU concern — single atomic unlink of a constant path). + Covered by `tests/ambient.test.ts:94-105`. + +- **No mass-deletion risk.** The function deletes exactly one file. No + globbing, no directory recursion (`rm -rf` style), no rmdir. Even if + `os.homedir()` were unexpectedly empty, the resolved path would be a + relative `.claude/rules/devflow/commands.md`, and unlink would simply + ENOENT or fail — no broad deletion. + +- **`devflowDir` inference unchanged** (`ambient.ts:194-208`). The + `path.resolve(hookBinary, '..', '..', '..')` logic that consumes a value + from `settings.json` is pre-existing on `main` (verified via + `git show main:...`) and not modified by this PR. It reads from the CLI's + own trusted local config, not external attacker input, so no injection + surface regardless. Out of scope as unchanged code; no CRITICAL concern. + +- **No secrets, no injection, no auth surface.** `plugins.ts` and `init.ts` + changes are description/help-text string edits only. The deleted + `shared/rules/commands.md` was static documentation content with no + executable surface. + +Decisions context: `applies ADR-001` (no migration code — the PR purges the +legacy file at the enable/disable boundary rather than via a migration, and +`avoids PF-001`). This is a clean-break cleanup, consistent with the +documented architecture and `ADR-008` (deterministic plumbing only). + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|--------------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Security Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/testing.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/testing.md new file mode 100644 index 00000000..2b081cb5 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/testing.md @@ -0,0 +1,96 @@ +# Testing Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**Date**: 2026-06-01_2352 +**PR**: #233 + +## Scope + +Verified `tests/ambient.test.ts` against the source refactor in +`src/cli/commands/ambient.ts`. The PR removes the `commands` awareness rule: +`COMMANDS_RULE_CONTENT` and `installCommandsRule()` are deleted, `removeCommandsRule()` +is renamed to `removeLegacyCommandsRule()`, and both `addAmbientHook`/`removeAmbientHook` +now *purge* the legacy rule (via `fs.unlink`) instead of writing it. All 40 tests in the +suite pass (`npx vitest run tests/ambient.test.ts` → pass: 40, fail: 0). + +## Focus-Question Findings + +**(1) Ordering-invariant test exercises the early-return path AND asserts unlink — CONFIRMED.** +`tests/ambient.test.ts:94-105`. The first `addAmbientHook('{}')` installs the preamble +hook. The second call passes `withHook`, so `hasPreamble` is true → the `if (!hasPreamble)` +block is skipped, `changed` stays `false`, `removeLegacyCommandsRule()` runs at +`ambient.ts:103` *before* the `if (!changed) return settingsJson` early-return at +`ambient.ts:105`. The test asserts `fs.unlink` was called with `COMMANDS_RULE_PATH`. This +genuinely guards the ordering invariant (purge-before-early-return) — if a future edit moved +the purge after the early-return, this test would fail. Good behavioral coverage. + +**(2) removeLegacyCommandsRule tests are behavior-focused, not over-mocked — CONFIRMED.** +`tests/ambient.test.ts:324-345`. The three cases (unlink-on-exists, ENOENT swallow, EACCES +propagate) mock only the `fs.unlink` boundary — which is exactly the OS boundary the function +wraps — and assert observable contract behavior (call happened / resolves / rejects with code). +This is mock-at-the-boundary, not internal coupling. The ENOENT-swallow and EACCES-propagate +tests directly exercise the `if (code !== 'ENOENT') throw` branch at `ambient.ts:67`, covering +both branch arms. Appropriate use of stubs/spies per the taxonomy. + +**(3) No meaningful coverage lost when installCommandsRule/COMMANDS_RULE_CONTENT suites were +deleted — CONFIRMED.** The deleted `describe('installCommandsRule')` and +`describe('COMMANDS_RULE_CONTENT')` suites tested symbols that no longer exist in source +(`installCommandsRule`, `COMMANDS_RULE_CONTENT` are both removed from `ambient.ts`). The +dual-source drift guard (`COMMANDS_RULE_CONTENT` === `shared/rules/commands.md`) is correctly +gone because both sides of the equality were deleted (`shared/rules/commands.md` confirmed +DELETED on disk). The new purge behavior (`removeLegacyCommandsRule` + the unconditional +purge in both add/remove paths) is fully covered: unlink-happy-path, ENOENT, EACCES, the +add-path ordering invariant, and the remove-path purge (exercised transitively by the +`removeAmbientHook` suite, whose `beforeEach` stubs `fs.unlink`). No behavioral gap. + +**(4) No brittle mocking / implementation-coupling introduced by the flip — CONFIRMED, with +one minor observation.** The flip from `mkdir`+`writeFile` stubs to a single `unlink` stub is +correct and reduces mock surface. The assertion `fs.unlink` toHaveBeenCalledWith(COMMANDS_RULE_PATH) +asserts an observable filesystem side-effect (the function's documented contract: "delete the +legacy file"), not an internal code path — acceptable for a function whose entire purpose is a +side-effect. See the single MEDIUM observation below regarding the `vi.restoreAllMocks()` / +re-stub dance inside the ordering test. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) + +None. + +## Suggestions (Lower Confidence) + +- **In-test `vi.restoreAllMocks()` then re-stub is a mild smell** — `tests/ambient.test.ts:98-100` + (Confidence: 70%) — The ordering test restores all mocks mid-body then re-stubs `fs.unlink` + purely to reset the spy's call history before the asserted call. `vi.clearAllMocks()` (or + `mockClear()` on the spy) would reset call history without tearing down and rebuilding the + spy, making the intent ("ignore the first call, assert the second") clearer and less coupled + to spy-lifecycle mechanics. Behavior is correct as written; this is readability only. + +- **Add-path purge not asserted with explicit "exactly once" semantics** — `tests/ambient.test.ts:94-105` + (Confidence: 62%) — The ordering test asserts the purge *happened* on the early-return path + but does not pin that it runs exactly once per call. Low value given the function is a simple + idempotent unlink; noted only for completeness. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|--------------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Testing Score**: 9/10 +**Recommendation**: APPROVED + +The test changes are a clean, behavior-focused mirror of the source refactor. Coverage of the +new purge behavior is complete (happy path, ENOENT, EACCES, and the load-bearing +purge-before-early-return ordering invariant). Deleted suites tested now-nonexistent symbols +and a dual-source guard whose second source was also deleted — no real coverage was lost. +Mocking stays at the `fs` boundary throughout. The only nits are stylistic (mock-reset +ergonomics) and do not block. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/typescript.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/typescript.md new file mode 100644 index 00000000..a4bed18e --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-01_2352/typescript.md @@ -0,0 +1,83 @@ +# TypeScript Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-01_2352 + +## Scope + +TypeScript-specific quality review of the three changed source files: +- `src/cli/commands/ambient.ts` (removed `COMMANDS_RULE_CONTENT` const + `installCommandsRule`/`removeCommandsRule` functions; renamed cleanup helper to `removeLegacyCommandsRule`; repurposed call sites in `addAmbientHook`/`removeAmbientHook`) +- `src/cli/commands/init.ts` (description string + ambient-hook call sites) +- `src/cli/plugins.ts` (plugin description string) + +Verification performed: +- `npx tsc --noEmit` — passed clean, no errors +- grep for dangling references to removed symbols (`installCommandsRule`, `removeCommandsRule`, `COMMANDS_RULE_CONTENT`) across `src/` and `tests/` — none found +- grep confirmed all imports in `ambient.ts` remain used (`fs`, `path`, `os`, `p`, `color`, `getClaudeDirectory`/`getDevFlowDirectory`, type-only `HookMatcher`/`Settings`) +- `decisions`/`pitfalls` cross-check (ADR-001, PF-001) + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +### MEDIUM +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None at CRITICAL severity. (Per the Iron Law, only CRITICAL pre-existing issues are reported; none found.) + +## Suggestions (Lower Confidence) + +None. + +## Detailed Verification Against Review Brief + +The review brief called out specific TS concerns. Each was checked and confirms clean: + +1. **Explicit return types** — All three functions carry explicit return types: + - `removeLegacyCommandsRule(): Promise` (`ambient.ts:63`) + - `addAmbientHook(settingsJson: string, devflowDir: string): Promise` (`ambient.ts:78`) + - `removeAmbientHook(settingsJson: string): Promise` (`ambient.ts:117`) + +2. **No `any`** — None introduced. The error handling uses `unknown`-style narrowing via typed assertion (see #4). + +3. **Error typing / `NodeJS.ErrnoException` narrowing** — `ambient.ts:66-68` narrows the caught error correctly: + `if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err;` + This swallows only the missing-file case and re-throws everything else (e.g. `EACCES`). The `tests/ambient.test.ts:344` case asserts EACCES propagation, confirming the narrowing behaves as intended. The `err as NodeJS.ErrnoException` assertion is the idiomatic Node pattern for reading `.code` off a caught error and is appropriate here (not an `any` escape hatch — the assertion is to a specific interface, and only the `.code` discriminant is read). + +4. **No unused imports** — Confirmed by manual trace. `os` is still consumed by `COMMANDS_RULE_PATH` (`ambient.ts:21`), `fs` by `fs.unlink` (`ambient.ts:65`). Note `tsconfig.json` does not set `noUnusedLocals`, so `tsc` would not have caught a stray import — manual verification was required and passed. + +5. **Async/Promise handling** — Both cleanup call sites correctly `await` the now-async cleanup: + - `ambient.ts:103` — `await removeLegacyCommandsRule();` inside `addAmbientHook` + - `ambient.ts:124` — `await removeLegacyCommandsRule();` inside `removeAmbientHook` + - `init.ts:1131-1132` — both `removeAmbientHook` and `addAmbientHook` are awaited: + `const cleanedForAmbient = await removeAmbientHook(content);` + `content = ambientEnabled ? await addAmbientHook(cleanedForAmbient, devflowDir) : cleanedForAmbient;` + No floating promises, no missing `await`. + +## Decisions / Pitfalls + +- **applies ADR-001** (clean-break, no migration code) and **avoids PF-001** (migration code on a rename refactor): The retained `removeLegacyCommandsRule` + `COMMANDS_RULE_PATH` purge a stale file the tool itself previously installed. This is cleanup of a self-authored install artifact, not transformation/preservation of user data — consistent with the clean-break philosophy and distinct from the PF-001 anti-pattern. No violation. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**TypeScript Score**: 10 +**Recommendation**: APPROVED + +This is a clean deletion-and-rename refactor. Type safety is preserved throughout, error narrowing is correct and test-covered, all promises are awaited, no `any` was introduced, and no dead references or unused imports remain. `tsc --noEmit` passes. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/architecture.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/architecture.md new file mode 100644 index 00000000..14a858a8 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/architecture.md @@ -0,0 +1,90 @@ +# Architecture Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-02_0013 + +## Summary of Verification + +Reviewed the collapse of ambient mode from two components (preamble hook + commands.md rule) +to one (preamble hook only). Focus areas: cleanup-ordering invariant, single-component +architectural cleanliness, layering, and dead-reference detection. + +Verification performed: +- Read full `src/cli/commands/ambient.ts` — confirmed cleanup-ordering invariant holds. +- `tsc` clean compile (`npm run build:cli`) — no dangling references to removed symbols. +- `npx vitest run tests/ambient.test.ts` — 40/40 pass, including invariant tests (lines 96-104). +- grep across `src/ tests/ shared/ plugins/ scripts/ docs/` — zero dead references to + `COMMANDS_RULE_CONTENT`, `installCommandsRule`, or the deleted `shared/rules/commands.md`. +- Confirmed `devflow-ambient` plugin manifest `rules: []` (commands rule fully de-registered). +- Read ADR-001 + PF-001 full bodies via decisions files. + +**The cleanup-ordering invariant is correctly implemented.** `removeLegacyCommandsRule()` is +called at `ambient.ts:107` (before the `if (!changed) return` at line 109) and at `ambient.ts:128` +(before the `if (!removedPrompt && !removedClassification) return` at line 130). The init path +(`init.ts:1131`) always calls `removeAmbientHook` regardless of enable/disable, so all three entry +points (enable, disable, init) unconditionally purge the stale file. Tests at +`tests/ambient.test.ts:96-104` assert the purge runs even when the hook is unchanged. + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +The single-component collapse is architecturally clean. Removing the `commands.md` rule eliminates +a redundant concern (the preamble hook already documents plan auto-execution; the static rule was +duplicative passive reference). This **improves** separation of concerns — ambient mode now has one +reason to change (plan detection) rather than two. No SOLID, coupling, or leaky-abstraction +violations introduced. The `removeLegacyCommandsRule` function is appropriately deep: a simple +interface (no args, void return, idempotent, fail-safe) hiding the legacy-cleanup concern. + +The clean-break-with-cleanup approach `applies ADR-001` — which explicitly sanctions "one-time +cleanup items (legacy hook file removal)" as the permitted exception to the no-compat-code rule — +and `avoids PF-001` by not introducing a migration registry entry, version tracking, or +backward-compat layer for what is a simple rename/removal refactor. + +## Pre-existing Issues (Not Blocking) + +None identified in the reviewed scope. + +## Suggestions (Lower Confidence) + +- **Legacy-cleanup placement vs migration registry** - `src/cli/commands/ambient.ts:65-73` + (Confidence: 65%) — The codebase has a dedicated migration registry + (`src/cli/utils/migrations.ts`, e.g. `purge-orphaned-sidecar-judgment-state`) for one-time + legacy-file purges, whereas this cleanup lives inline in the ambient lifecycle. This is a + defensible choice — it matches the module's existing `LEGACY_HOOK_MARKER`/`isLegacy` inline + cleanup convention and runs on every ambient toggle (more frequent than a once-per-machine + migration), giving stronger purge guarantees. Noting only as a layering observation; no action + required. A future cleanup pass could consolidate inline legacy-purges into the migration + registry for a single cleanup locus, but that is out of scope here and would arguably reduce + the purge frequency that makes the current approach robust. + +## Cross-Cycle Awareness + +PRIOR_RESOLUTIONS (Cycle 1) reported 3 fixed issues: `removeLegacyCommandsRule` made fail-safe, +README rule count corrected to 12, and the fabricated PF-007 citation removed from KNOWLEDGE.md. +Verified against current code: +- `removeLegacyCommandsRule` swallows all errors via bare `catch` (`ambient.ts:66-72`) — confirmed fixed. +- Rule count is 12 in `CLAUDE.md:65` ("4 core + 8 language/UI") — confirmed fixed. +- No PF-007 citation present in the KNOWLEDGE.md diff — confirmed fixed. + +None of these are re-raised. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Architecture Score**: 9/10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/complexity.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/complexity.md new file mode 100644 index 00000000..00a7bf72 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/complexity.md @@ -0,0 +1,61 @@ +# Complexity Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-02_0013 + +## Summary Verdict + +This is a clean net-deletion refactor that genuinely reduces complexity. The removal of +`COMMANDS_RULE_CONTENT` (a 26-line template literal) and `installCommandsRule()` eliminates a +whole responsibility (rule materialization) from `ambient.ts`, leaving only best-effort cleanup +of the deprecated file. No blocking or should-fix complexity issues were found. + +Verification performed: +- `npm run build:cli` (tsc) compiles cleanly — no dead references break the build. +- `grep` across `src/ tests/ scripts/ shared/ plugins/` for `installCommandsRule`, + `COMMANDS_RULE_CONTENT`, and `removeCommandsRule` (word-boundary) returns zero leftover + references to removed symbols. +- `tests/ambient.test.ts` imports and mocks updated to match (`fs.unlink` stub replaces + `fs.mkdir`/`fs.writeFile`); a new ordering-invariant test covers the early-return purge path. +- Feature knowledge (`cli-rules` KNOWLEDGE.md) already documents this exact design — the + unconditional `removeLegacyCommandsRule()` call in both add/remove paths matches the + documented Gotcha (line 239) and Key Files (line 253) notes. No deviation from documented patterns. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None at MEDIUM+ confidence/severity. The action handler in `ambientCommand` (`ambient.ts:157-235`, +~78 lines) is the longest unit in the file, exceeding the 50-line warning threshold. However it is +flat sequential CLI orchestration (read settings -> status branch -> resolve devflowDir -> enable +branch -> disable branch), each branch guarded by early-returns with max nesting depth 3. It reads +top-to-bottom without requiring a control-flow diagram, so it does not violate the 5-minute Iron Law. +This handler is also largely pre-existing (only comment/string lines changed in this PR). Not worth +refactoring in this deletion-focused PR. + +## Suggestions (Lower Confidence) + +- **Comment-to-code ratio on `removeLegacyCommandsRule`** - `ambient.ts:58-73` (Confidence: 65%) — + The function is 3 lines of logic with 11 lines of doc/inline comments explaining the swallow-all-errors + fail-safe rationale. The verbosity is justified (a bare `catch {}` swallowing EACCES/EPERM would + otherwise look like a bug and invite a "re-throw non-ENOENT" regression — exactly the prior-cycle + fix), so this is acceptable, not a defect. Optional: a one-line `// best-effort cleanup; see ADR/PF` + pointer would be terser, but the current explicit form is defensible. No action required. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 1 | + +**Complexity Score**: 9 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/consistency.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/consistency.md new file mode 100644 index 00000000..4340e3ed --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/consistency.md @@ -0,0 +1,57 @@ +# Consistency Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main (PR #233) +**Date**: 2026-06-02_0013 +**Focus**: Consistency of the docs sweep — complete removal of all `commands.md` / commands-rule references, rule-count consistency (must read 12 everywhere), and naming consistency of `removeLegacyCommandsRule`. + +## Scope Note + +The PR deletes the ambient-managed `commands` rule (`shared/rules/commands.md`), renames `removeCommandsRule` → `removeLegacyCommandsRule`, deletes `COMMANDS_RULE_CONTENT` / `installCommandsRule`, and performs a documentation sweep across CLAUDE.md, README.md, docs/commands.md, docs/cli-reference.md, plugin README + plugin.json, plugins.ts, init.ts, and the cli-rules KNOWLEDGE.md. + +Per Cross-Cycle Awareness (PRIOR_RESOLUTIONS): the three Cycle-1 fixes (fail-safe error handling, README count 13→12, removed fabricated PF-007 citation) were verified as still applied and are NOT re-raised. The README now correctly reads "12 ultra-condensed engineering principles" (README.md:56) and `removeLegacyCommandsRule` swallows all errors fail-safe (ambient.ts:65-73). + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +### MEDIUM + +**Stale `referencedFiles` entry points at the deleted `shared/rules/commands.md`** — `.devflow/features/index.json:26` (committed HEAD) +**Confidence**: 90% +- Problem: 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 (HEAD:.devflow/features/index.json line 26). The `description` keyword list on line 6 also still includes `commands.md`. The companion KNOWLEDGE.md was updated correctly (its `referenced_files` frontmatter lists only the 4 core rules and the body documents the removal + the new `removeLegacyCommandsRule()` keyword), so the index and the 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 (per the Feature Knowledge Bases design). A deleted path can never produce a meaningful diff, so the entry is dead. More importantly, it is an internal inconsistency: every other doc artifact in the sweep was updated to reflect the deletion, but this metadata file was missed — the same class of straggler the PR set out to eliminate. +- Fix: In `.devflow/features/index.json`, remove `"shared/rules/commands.md"` from the `cli-rules.referencedFiles` array, and drop `commands.md` from the `description` keyword list (replace with `removeLegacyCommandsRule` to match KNOWLEDGE.md frontmatter line 4). Note: the uncommitted working-tree change to this file refreshes `lastUpdated` and keywords but does NOT remove the stale `referencedFiles` entry — the fix must explicitly delete line 26. + +## Pre-existing Issues (Not Blocking) + +None relevant to this focus. + +## Suggestions (Lower Confidence) + +None. + +## Verification Performed (no issues found) + +The following consistency checks passed — recorded so the synthesizer can see coverage: + +- **Rule count = 12 everywhere**: CLAUDE.md ("Currently 12 rules: 4 core + 8 language/UI" + "shared/rules/ # 12 rules"), README.md:56 ("12 ultra-condensed engineering principles"), and KNOWLEDGE.md ("12 rules total: 4 core + 8 language/ecosystem"; "flat .md files (12 total)") all agree. No surviving "13 rules" in any tracked file outside `.devflow/docs/reviews/` (historical artifacts, out of scope). +- **No "two-component" / "command awareness" stragglers**: `git grep` for `command awareness|two-component|13 rules|command listing` across `*.md`/`*.ts`/`*.json` (excluding review/audit/bug-analysis report dirs) returned zero hits. CLAUDE.md now says "Single-component system"; docs/commands.md dropped the "two components" framing to a single "Plan auto-detection" bullet; plugin README and cli-reference both read "plan auto-detection" only. +- **Plugin metadata consistent**: `plugins/devflow-ambient/.claude-plugin/plugin.json` description = "Plan auto-detection", keywords pruned to `[ambient, plan, detection]` (removed `commands`, `awareness`); `src/cli/plugins.ts` ambient entry matches with the same description and `rules: []`. No plugin declares a `commands` rule. +- **Dead-symbol sweep clean**: `installCommandsRule` and `COMMANDS_RULE_CONTENT` have zero references in live source/tests/docs — all remaining mentions are confined to `.devflow/docs/reviews/` history. +- **Naming consistency of `removeLegacyCommandsRule`**: Follows the established verb-first `removeXxx` convention used by siblings (`removeAmbientHook`, `removeMemoryHooks`, `removeContextHook`, `removeHudStatusLine`, `removeManagedSettings`). The `Legacy` qualifier is meaningful and aligns with the existing `LEGACY_*` constant vocabulary (`LEGACY_RULE_NAMES`, `LEGACY_COMMAND_NAMES`, `LEGACY_SKILL_NAMES`, `LEGACY_HOOK_FILES`) — it correctly signals "purge a deprecated file" vs. "remove an active feature". The rename is applied consistently across `ambient.ts` (definition + 2 call sites) and `tests/ambient.test.ts` (import, describe block, comments); no old `removeCommandsRule` name survives in live code. +- **init.ts cleanup path**: init.ts has no direct commands-rule reference; legacy cleanup runs indirectly via `addAmbientHook`/`removeAmbientHook` (both call `removeLegacyCommandsRule` unconditionally before their early-return). This matches the CLAUDE.md/KNOWLEDGE.md claim that purge happens "on every `devflow ambient --enable/--disable` or `devflow init`" and preserves the Cycle-1 fail-safe fix. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 1 | - | +| Pre-existing | - | - | 0 | 0 | + +**Consistency Score**: 9 +**Recommendation**: APPROVED_WITH_CONDITIONS + +The documentation sweep is thorough and internally consistent across all prose docs, plugin metadata, source, and tests — rule count reads 12 everywhere and no "two-component" / "command awareness" / removed-symbol stragglers remain. The single condition is the stale `shared/rules/commands.md` entry in `.devflow/features/index.json` `referencedFiles` (and the `commands.md` keyword in its description), which points at a file this PR deleted and is the one place the sweep missed. Low-risk, mechanical fix. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/documentation.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/documentation.md new file mode 100644 index 00000000..f9218ad5 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/documentation.md @@ -0,0 +1,93 @@ +# Documentation Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-02_0013 +**Focus**: Documentation drift after removal of the ambient `commands.md` rule + +## Summary of Verification Performed + +This PR is a documentation sweep removing all references to the deleted ambient +`commands-awareness` rule (`shared/rules/commands.md`, 26 lines, deleted). I verified +every changed doc against the actual current code state, and ran a repo-wide sweep for +stale tokens. The documentation is accurate and consistent. No blocking, should-fix, or +pre-existing documentation issues at >=80% confidence. + +### Code-vs-doc accuracy checks (all PASS) + +| Doc claim | Verified against | Result | +|-----------|------------------|--------| +| `removeLegacyCommandsRule()` exists, runs unconditionally in both `addAmbientHook` and `removeAmbientHook` before early-return (KNOWLEDGE.md:239, :253) | `src/cli/commands/ambient.ts:65,107,128` | Accurate | +| `COMMANDS_RULE_PATH` constant exists (KNOWLEDGE.md:253) | `ambient.ts:21` | Accurate | +| `installCommandsRule` / `COMMANDS_RULE_CONTENT` no longer live | Absent from `ambient.ts`; removed from `tests/ambient.test.ts` imports | Accurate | +| `devflow-ambient` has `rules: []` (KNOWLEDGE.md:93) | `src/cli/plugins.ts:172` | Accurate | +| `LEGACY_RULE_NAMES` currently empty (KNOWLEDGE.md:99, :217) | `src/cli/plugins.ts:693` `= []` | Accurate | +| "Currently 12 rules: 4 core + 8 language/UI" (CLAUDE.md, README, KNOWLEDGE.md:30,:249) | `shared/rules/` = 12 files (4 core + 8 lang) | Accurate (4+8=12) | +| "Build reports 12 rules" (PR desc) | `build-plugins.ts:230` logs dynamic `availableRules.size` | Accurate, not hardcoded | +| ADR-001 citation "clean break philosophy" (KNOWLEDGE.md:263) | `.devflow/decisions/decisions.md:6` heading verbatim | Valid verbatim citation | + +### Repo-wide straggler sweep (all CLEAN) + +Searched the full repo (excluding `node_modules`, `dist/`, `.devflow/docs/`, `.git/`): + +- `commands-awareness`, `commands-rule`, `two-component`/`two component`: **0 hits** +- `13 rules`, `13 ultra`: **0 hits** +- `installCommandsRule`, `COMMANDS_RULE_CONTENT` (outside the deleted test code): **0 hits** +- `command awareness`, `command-awareness`, `passive command`: **0 hits** outside review docs +- `docs/reference/`: **0** stale rule counts or commands-rule references + +Note: `shared/rules/commands.md` (the file shown in `git diff --stat`) IS the deleted +ambient rule — confirmed deleted (`ls` returns "No such file or directory"). It is distinct +from the `commands.md` documentation file under `docs/`, which was updated correctly. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None at CRITICAL severity. + +## Cross-Cycle Awareness (PRIOR_RESOLUTIONS verified) + +Cycle 1 fixed 3 issues. I verified each against current code — all hold, none re-raised: + +1. **Fail-safe error handling** — `removeLegacyCommandsRule()` wraps `fs.unlink` in try/catch + (`ambient.ts:66-67`); ENOENT/errors are swallowed. Confirmed fail-safe. Not re-raised. +2. **README "13 -> 12" correction** — `README.md:56` now reads "12 ultra-condensed engineering + principles". Confirmed corrected. Not re-raised. +3. **Fabricated PF-007 citation removed** — KNOWLEDGE.md:241 (partition predicate gotcha) now + contains a plain description with no PF-007 citation. The only ADR/PF citation in any changed + doc is ADR-001 (KNOWLEDGE.md:263), which is verbatim-valid per DECISIONS_CONTEXT. No fabricated + or unverifiable citations remain. Not re-raised. + +## Suggestions (Lower Confidence) + +- **KNOWLEDGE.md:199 phrasing** — `.devflow/features/cli-rules/KNOWLEDGE.md:199` (Confidence: 65%) + — The init-flow section says stale rules "are cleaned up via `LEGACY_RULE_NAMES` loop." Since + `LEGACY_RULE_NAMES` is currently empty and the deleted `commands` rule is deliberately handled by + `removeLegacyCommandsRule()` in ambient.ts (not the init loop), a reader could momentarily infer + the commands-rule cleanup flows through this loop. The doc is internally consistent (lines 99 and + 239 clarify the split), so this is a minor clarity nit, not drift. Optional: add "(the deleted + `commands` rule is purged separately by `removeLegacyCommandsRule`, not this loop)". + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Documentation Score**: 10 +**Recommendation**: APPROVED + +The documentation sweep is accurate, internally consistent, and free of stale references to +the deleted rule. CLAUDE.md, README, docs/cli-reference.md, docs/commands.md, the ambient +plugin README/plugin.json, and the heavily-modified cli-rules KNOWLEDGE.md all correctly +describe ambient mode as a single-component (preamble-hook-only) system with 12 rules. All +ADR/PF citations are verbatim-valid. Prior cycle-1 fixes hold. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/performance.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/performance.md new file mode 100644 index 00000000..5d186170 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/performance.md @@ -0,0 +1,78 @@ +# Performance Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**Date**: 2026-06-02_0013 +**PR**: #233 + +## Scope + +Focused review of whether the unconditional `removeLegacyCommandsRule` call on +every `addAmbientHook` / `removeAmbientHook` (i.e. every `devflow ambient +--enable/--disable` and `devflow init`) introduces meaningful overhead. + +**Verdict**: No meaningful overhead. The added work is a single `fs.unlink` +syscall — one async I/O operation, no directory scans, no loops, no synchronous +blocking I/O. The PR author's own assessment ("a single unlink — likely +negligible") is correct. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None. + +## Suggestions (Lower Confidence) + +- **Redundant double `unlink` during `init`** — `src/cli/commands/init.ts:1131-1132` (Confidence: 90%) — When ambient is enabled, init calls `removeAmbientHook` (line 1131, which calls `removeLegacyCommandsRule`) immediately followed by `addAmbientHook` (line 1132, which calls it again). The legacy file is already gone after the first call, so the second `fs.unlink` always hits ENOENT and is swallowed. Cost is one wasted syscall per init — truly negligible, not worth restructuring the clean remove-then-add upgrade flow to avoid. Noted only for completeness. + +## Performance Analysis Detail + +Verified against current code at `src/cli/commands/ambient.ts:65-73`: + +```ts +export async function removeLegacyCommandsRule(): Promise { + try { + await fs.unlink(COMMANDS_RULE_PATH); + } catch { + // swallow all errors — best-effort cleanup + } +} +``` + +Performance characteristics of the added hot-path work: + +| Concern | Finding | +|---------|---------| +| Directory scans | None — `COMMANDS_RULE_PATH` is a fixed precomputed path (`ambient.ts:21`); no `readdir`/glob/walk. | +| Loops | None — single `unlink`, not invoked inside any iteration. | +| Synchronous/blocking I/O | None — uses `fs.promises.unlink` (async), does not block the event loop [3][16]. | +| I/O cost | One filesystem metadata operation. SSD unlink latency ~100µs [7]; ENOENT path (file already absent, the steady-state case) returns even faster without touching data blocks. | +| Allocation | None significant — no buffers read, no JSON parsed by this function. | + +The call placement (`addAmbientHook:107`, `removeAmbientHook:128`) runs once per +invocation, before the early-return. These functions execute only during +interactive CLI commands (`devflow ambient`, `devflow init`) — not in any +request path, hook hot path, or per-turn loop. A single sub-millisecond syscall +on an interactive CLI operation that already performs `readFile` + `writeFile` +of `settings.json` is immeasurable relative to existing work. + +Cross-cycle awareness: PRIOR_RESOLUTIONS Cycle 1 fixed items (fail-safe error +handling, README count, fabricated citation) are not performance-related and +were not re-raised; current code at `ambient.ts:65-73` confirms the fail-safe +catch is present. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Performance Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/regression.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/regression.md new file mode 100644 index 00000000..373c59d3 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/regression.md @@ -0,0 +1,96 @@ +# Regression Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-02_0013 + +## Scope + +Refactor removing the legacy ambient "commands awareness" rule. Removed exports +`COMMANDS_RULE_CONTENT` and `installCommandsRule`; renamed `removeCommandsRule` → +`removeLegacyCommandsRule`; deleted `shared/rules/commands.md`. Reviewed against the +PR's stated intent (exports removed, rename, signatures of `addAmbientHook`/ +`removeAmbientHook` unchanged, build now reports 12 rules) — all claims verified. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None. + +## Pre-existing Issues (Not Blocking) + +None. + +## Verification Performed + +All four orchestrator-flagged regression vectors were investigated by grepping the +full repo beyond the diff, inspecting `main` state, and running the build + tests. + +1. **Remaining importers of removed/renamed exports** — `grep -rn` across `src/` and + `tests/` for `installCommandsRule`, `COMMANDS_RULE_CONTENT`, `removeCommandsRule\b` + returns ZERO matches. No dangling consumers. The renamed `removeLegacyCommandsRule` + is correctly consumed in `src/cli/commands/ambient.ts:107,128` and imported in + `tests/ambient.test.ts:4`. (Confidence: 99%) + +2. **Deleting `commands.md` breaks build / plugin manifests** — No regression. + - `scripts/build-plugins.ts:230` (`Found ${availableRules.size} rules`) is an + informational count over a directory scan, NOT a hard assertion. Live build run + succeeds and prints "Found 12 rules in shared/rules/" (was 13). + - The build's only failure path for rules (`build-plugins.ts:189`, + `Rule "X" not found`) fires only for rules *declared* in a plugin's `rules` array. + No plugin ever declared `commands`: `src/cli/plugins.ts` shows `devflow-ambient` + with `rules: []`, and the only `commands` token in any plugin.json on `main` was a + `keywords` entry (line 15), removed in this diff — never a rules-array entry. + - The rules installer is plugin-declaration-driven (`plugins.ts` PluginDefinition.rules), + so the orphan `commands.md` in `shared/rules/` was never installed by the rules + feature. CLAUDE.md already documents "12 rules: 4 core + 8 language/UI" — consistent + with the post-deletion state; no doc drift introduced. (Confidence: 97%) + +3. **Rename breaks callers in init.ts/uninstall.ts** — No regression. `init.ts:24,39` + imports and re-exports only `addAmbientHook, removeAmbientHook, hasAmbientHook` — + none of the removed/renamed symbols. `addAmbientHook`/`removeAmbientHook` signatures + and return types are unchanged (verified in diff). (Confidence: 98%) + +4. **Cleanup preserved for upgrading users with `commands.md` on disk** — Preserved on + ALL paths. `removeLegacyCommandsRule()` runs *before* every early-return: + - `addAmbientHook` (ambient.ts:107) — purges before the `if (!changed) return` guard, + so even when the hook already exists the stale file is removed. + - `removeAmbientHook` (ambient.ts:128) — purges before the `if (!removedPrompt...)` guard. + - `init.ts:1130-1132` uses remove-then-add unconditionally, so `removeAmbientHook` + (and thus the purge) executes on every `init` regardless of ambient on/off. + Test `tests/ambient.test.ts:94` ("purges legacy rule even when preamble hook already + present (ordering invariant)") explicitly asserts this. The error handling was + widened to fail-safe (swallows EACCES/EPERM/EROFS, not just ENOENT) per the prior + resolve cycle — confirmed best-effort cleanup never aborts the primary operation. + (Confidence: 97%) + +## Cross-Cycle Awareness + +PRIOR_RESOLUTIONS (cycle 1) fixed: fail-safe error handling, README count→12, removed +fabricated citation. None re-raised — verified current code retains all three fixes +(ambient.ts:65-72 swallows all errors; CLAUDE.md/README report 12 rules). + +## Decisions Applied + +- `applies ADR-001` (clean-break): removed exports outright with no deprecation shims — + consistent with the clean-break decision. No consumers remain, so no migration needed. +- `avoids PF-001` (no migration code for rename refactors): the rename + `removeCommandsRule → removeLegacyCommandsRule` ships no back-compat alias; all call + sites updated in the same change. Correct per the pitfall. + +## Suggestions (Lower Confidence) + +None. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Regression Score**: 10 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/reliability.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/reliability.md new file mode 100644 index 00000000..c855669f --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/reliability.md @@ -0,0 +1,95 @@ +# Reliability Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**PR**: #233 +**Date**: 2026-06-02_0013 + +## Scope + +Focus: reliability of the unconditional best-effort cleanup call +(`removeLegacyCommandsRule`) and its fail-safe error handling, per Cycle 1 +resolution. Primary file under review: `src/cli/commands/ambient.ts`. Callers +verified: `addAmbientHook`, `removeAmbientHook` (ambient.ts), and the init +read-modify-write pass (`src/cli/commands/init.ts:1126-1132`). + +Cross-cycle awareness applied: PRIOR_RESOLUTIONS reports Cycle 1 fixed exactly +this fail-safe behavior. Per instructions, I did NOT re-raise the original +propagation bug as new; I verified the fix is correct, complete, and +appropriately scoped. + +Decisions applied: change is a clean-break removal of a deprecated rule with +purge-on-toggle cleanup (consistent with `applies ADR-001` clean-break posture). + +## Verification of Cycle 1 Fix (no new blocking findings) + +The fail-safe is **correct, complete, and correctly scoped**. Evidence: + +1. **Swallow is minimal and correctly scoped** — `ambient.ts:66-72`. The + `try` block wraps exactly one statement: `await fs.unlink(COMMANDS_RULE_PATH)`. + The surrounding hook logic (`JSON.parse`, `filterHookEntries`, + `JSON.stringify`) lives in the *caller* functions, entirely outside this + try/catch. The fail-safe therefore cannot mask errors from `addAmbientHook` / + `removeAmbientHook` — those still propagate normally (e.g. a corrupt + settings.json still throws at `JSON.parse`, `ambient.ts:83`/`122`). This was + the key concern in the review brief and it checks out. + +2. **Unconditional execution before early-return is real** — the call sits at + `ambient.ts:107` (before `if (!changed) return`) and `ambient.ts:128` + (before `if (!removedPrompt && !removedClassification) return`). The + ordering invariant is locked by a regression test (`ambient.test.ts:94-105`, + "purges legacy rule even when preamble hook already present"). + +3. **Swallowing ALL errors (not just ENOENT) is the right call here.** The + try body is a single `unlink` of one fixed, deprecated path. The complete + error universe is non-actionable: ENOENT (already gone — the idempotent + happy path), EACCES/EPERM/EROFS (unwritable filesystem — nothing the caller + can do, and never worth aborting `devflow init` / ambient toggle). There is + no genuine logic bug a narrower catch could surface, because the function + performs no computation that could be wrong — only a side-effecting unlink. + A narrow `if (code !== 'ENOENT') throw` would *reintroduce* the Cycle 1 + defect. Swallow-all is justified and the inline comment documents the + reasoning accurately. + +4. **Idempotency / quiet on absent file** — `fs.unlink` on a missing path + rejects with ENOENT, which is swallowed → no output, no throw. Safe and + quiet. Covered by `ambient.test.ts:335-339`. + +5. **No unbounded behavior** — no loops, retries, or pagination. A single + fixed-path unlink. Iron Law (bounded termination) satisfied trivially. + +6. **Test coverage is complete for the fail-safe contract** — ENOENT swallow + (`:335`), non-ENOENT/EACCES swallow (`:341`), happy-path unlink (`:329`), + and ordering invariant (`:94`) are all asserted. + +## Issues in Your Changes (BLOCKING) + +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) + +None at reportable confidence within the reliability lens. + +## Suggestions (Lower Confidence) + +- **Silent swallow emits no debug trace** - `src/cli/commands/ambient.ts:68` + (Confidence: 65%) — A persistent EACCES/EPERM means the deprecated + `commands.md` survives forever with zero diagnostic signal, so the cleanup + silently never converges. This is acceptable for a deprecated-file purge, but + a one-line debug log (gated on the existing `DEVFLOW_HOOK_DEBUG` / debug-trace + facility) inside the catch would make a stuck cleanup observable without + changing the fail-safe contract. Not blocking — purely diagnostic. + +## Summary + +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 0 | + +**Reliability Score**: 9 +**Recommendation**: APPROVED diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/resolution-summary.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/resolution-summary.md new file mode 100644 index 00000000..237e98f9 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/resolution-summary.md @@ -0,0 +1,42 @@ +# Resolution Summary + +**Branch**: refactor/remove-ambient-commands-rule -> main +**Date**: 2026-06-02_0013 +**Review**: .devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013 +**Command**: /resolve + +## Decisions Citations + +(none — fixes were mechanical test/metadata corrections; no ADR/PF bodies cited) + +## Statistics +| Metric | Value | +|--------|-------| +| Total Issues | 3 | +| Fixed | 3 | +| False Positive | 0 | +| Deferred | 0 | +| Blocked | 0 | + +## Fixed Issues +| Issue | File:Line | Commit | +|-------|-----------|--------| +| Missing symmetric ordering-invariant test for `removeAmbientHook` early-return path — `removeLegacyCommandsRule()` runs at ambient.ts:128 before the early-return at :130, but no test asserted the purge fires on that path (a regression moving it after the early-return would have passed all tests). Added mirror test `purges legacy rule even when nothing to remove (ordering invariant)`. | tests/ambient.test.ts:~258 | 9024222 | +| Brittle re-stub in the `addAmbientHook` ordering test — `vi.restoreAllMocks()`+re-stub discarded `beforeEach` isolation and coupled to the first call's side effect. Replaced with `vi.clearAllMocks()` (preserves the stub, resets counts). | tests/ambient.test.ts:98 | 9024222 | +| Stale `referencedFiles` entry — `.devflow/features/index.json` cli-rules entry still listed the deleted `shared/rules/commands.md` (skews git-log staleness detection) and kept `commands.md` in the description keywords. Removed the dead path; replaced the keyword with `removeLegacyCommandsRule` to match KNOWLEDGE.md. | .devflow/features/index.json:26 | 9024222 | + +## False Positives +(none) + +## Deferred to Tech Debt +(none) + +## Blocked +(none) + +## Notes +- Cycle 2 of review→resolve. All 3 review findings (1 HIGH testing, 1 MEDIUM testing, 1 MEDIUM consistency) fixed directly — no tech debt deferred. +- Two LOW-confidence (65-70%) suggestions in testing.md were explicitly classified by the reviewer as "acceptable, not a defect" (EACCES is a representative non-ENOENT case for the bare `catch {}`; fs.unlink spy is the correct seam) — not actioned, no change warranted. +- Test count: 40 → 41 (`npx vitest run tests/ambient.test.ts` passes). index.json validated as well-formed JSON. +- Commit `9024222` staged only `tests/ambient.test.ts` and `.devflow/features/index.json`; runtime `.devflow/decisions/` + `.devflow/docs/` artifacts and the two stray junk files left unstaged. +- The 8 other reviewers (security, architecture, performance, complexity, regression, reliability, typescript, documentation) returned clean APPROVED — no issues to resolve. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/review-summary.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/review-summary.md new file mode 100644 index 00000000..2766edb6 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/review-summary.md @@ -0,0 +1,128 @@ +# Code Review Summary + +**Branch**: refactor/remove-ambient-commands-rule -> main (PR #233) +**Date**: 2026-06-02_0013 + +## Merge Recommendation: CHANGES_REQUESTED + +The branch is well-executed overall (8 of 10 reviewers approved cleanly, all prior-cycle fixes verified), but **two testing gaps must be closed before merge**: the missing symmetric test for `removeAmbientHook`'s early-return purge path (HIGH confidence), and the brittle mock dance in the existing test (MEDIUM confidence). Additionally, one metadata sync in the feature knowledge index (consistency, MEDIUM confidence). All three are low-risk and straightforward to fix. + +## Issue Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | Total | +|----------|----------|------|--------|-----|-------| +| Blocking | 0 | 0 | 0 | - | 0 | +| Should Fix | - | 1 | 1 | - | 2 | +| Pre-existing | - | - | 1 | 0 | 1 | + +## Blocking Issues + +None. The deletion/refactor of the ambient rule is architecturally sound, properly cleaned up, and introduces no security, performance, regression, or reliability concerns. + +## Should-Fix Issues (Changes You Touched) + +All issues are in `tests/ambient.test.ts`. Testing coverage for the fail-safe cleanup is incomplete. + +### HIGH — Missing symmetric ordering-invariant test for `removeAmbientHook` early-return path + +**Location**: `tests/ambient.test.ts:251` (should add mirror test) +**Confidence**: 90% +**Category**: Testing +**Problem**: +- The new ordering-invariant test (line 94-104) proves `addAmbientHook` purges the legacy rule even when taking its early-return path (when the preamble hook already exists). +- But `removeAmbientHook` has the *same* structure: it calls `removeLegacyCommandsRule()` at `ambient.ts:128` *before* its early-return at `ambient.ts:130`. +- No test asserts that `fs.unlink` is called on this path for `removeAmbientHook`. The closest test ("is idempotent — safe to call when not present") only asserts `result === input` and does NOT assert the purge ran. +- **Risk**: A regression that moved `removeLegacyCommandsRule()` *after* the early-return in `removeAmbientHook` would silently pass all current tests. + +**Fix**: +Add a mirror test in the `removeAmbientHook` suite (the `beforeEach` already stubs `fs.unlink`): +```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 +}); +``` + +--- + +### MEDIUM — Brittle mock restore/re-stub dance + +**Location**: `tests/ambient.test.ts:98-100` +**Confidence**: 82% +**Category**: Testing +**Problem**: +- The ordering-invariant test calls `vi.restoreAllMocks()` then re-stubs `fs.unlink` to "assert on the second call." +- This approach works but is brittle: it discards call-count isolation and couples the test to the fact that the *first* `addAmbientHook` also calls `unlink`. +- A cleaner assertion reasons only about the second call without tearing down and rebuilding the spy. +- **Impact**: Readability and robustness nit; the test does correctly prove the invariant, but future maintainers may break it accidentally. + +**Fix**: +Avoid the restore/re-stub dance. Use `vi.clearAllMocks()` (preserves the stub implementation) instead of `vi.restoreAllMocks()` (removes it), or capture and assert the call-count delta: +```typescript +// Before the second addAmbientHook: +const callsBefore = (fs.unlink as Mock).mock.calls.length; +await addAmbientHook(withHook, withoutHook, { rules: [], knowledge: false, decisions: false }); +expect((fs.unlink as Mock).mock.calls.length).toBeGreaterThan(callsBefore); +``` + +--- + +## Pre-existing Issues (Not Blocking) + +### MEDIUM — Stale `referencedFiles` entry in feature knowledge index + +**Location**: `.devflow/features/index.json:26` (and line 6 keyword list) +**Confidence**: 90% +**Category**: Consistency +**Problem**: +- This PR deletes `shared/rules/commands.md`, but the committed feature knowledge index entry for `cli-rules` still lists `"shared/rules/commands.md"` in its `referencedFiles` array. +- The `description` keyword list on line 6 also still includes `commands.md`. +- The companion `KNOWLEDGE.md` was correctly updated, so the index and KNOWLEDGE.md are now out of sync with each other and with the source tree. +- **Risk**: `referencedFiles` drives staleness detection via `git log` against those paths. A deleted path can never produce a meaningful diff, so the entry is dead. It's an internal inconsistency — every other doc artifact in the sweep was updated, but this metadata file was missed. + +**Fix**: +In `.devflow/features/index.json`, remove `"shared/rules/commands.md"` from the `cli-rules.referencedFiles` array, and drop `commands.md` from the `description` keyword list (replace with `removeLegacyCommandsRule` to match KNOWLEDGE.md frontmatter line 4). + +--- + +## Suggestions (Lower Confidence) + +None above 60% confidence that change normal development flow. + +--- + +## Cross-Cycle Verification + +**Cycle 1** (2026-06-01_2352): Fixed 3 issues — fail-safe error handling, README count 13→12, removed fabricated PF-007 citation. + +**Cycle 2 Verification**: +- ✅ Fail-safe error handling (`ambient.ts:66-72`) — present and correct; all reviewers (security, reliability, testing, typescript) re-verified +- ✅ README rule count → 12 — verified in `CLAUDE.md:65`, `README.md:56`, `KNOWLEDGE.md`; no survivor "13 rules" +- ✅ PF-007 citation removed — no fabricated or invalid ADR/PF citations remain in any changed doc +- ✅ No regressions: All prior fixes hold in current code + +**False Positive Ratio from Cycle 1**: 0/3 = 0% (all 3 prior issues were real and fixed) + +**Assessment**: First cycle identified real issues; second cycle converging well. This is a normal two-cycle pattern for a documentation-heavy refactor. + +--- + +## Convergence Status + +**Cycle**: 2 +**Prior Resolution**: Available (3 issues fixed in Cycle 1) +**Prior FP Ratio**: 0/3 = 0% +**Assessment**: Converging. Cycle 1 found and fixed 3 real issues with zero false positives. Cycle 2 identified 2 testing gaps (orthogonal to Cycle 1 fixes) and 1 metadata sync issue (pre-existing, informational). The branch quality is solid; the remaining 3 items are straightforward fixes. + +--- + +## Action Plan + +1. **Add mirror ordering-invariant test for `removeAmbientHook`** (HIGH) — 5 min, tests/ambient.test.ts +2. **Simplify test mock setup** (MEDIUM) — 5 min, tests/ambient.test.ts +3. **Update `.devflow/features/index.json`** (MEDIUM, pre-existing) — 2 min, remove stale referencedFiles entry and update keywords + +**Merge gate**: Complete items 1-2 (blocking). Item 3 (pre-existing metadata) may be deferred to a follow-up if preferred. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/security.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/security.md new file mode 100644 index 00000000..deb07f90 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/security.md @@ -0,0 +1,83 @@ +# Security Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main (PR #233) +**Date**: 2026-06-02_0013 +**Diff**: `git diff main...HEAD` + +## Scope + +Deletion/refactor of CLI install plumbing in `src/cli/commands/ambient.ts` plus a +docs/manifest/test sweep. Security focus per orchestrator brief: path handling, +error swallowing that could hide security-relevant failures, and injection in +shell/file operations. No runtime data-handling, auth, crypto, or network code is +touched. `applies ADR-001` / `avoids PF-001` — the change is a clean-break deletion +with best-effort legacy cleanup and correctly adds no migration/compat code. + +## Issues in Your Changes (BLOCKING) + +### CRITICAL +None. + +### HIGH +None. + +### MEDIUM +None. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +Note on the `removeLegacyCommandsRule` catch-all (`ambient.ts:65-73`): the function +now swallows ALL errors when unlinking `COMMANDS_RULE_PATH`. This was flagged and +intentionally resolved in cycle 1 (PRIOR_RESOLUTIONS: "made fail-safe — swallows ALL +errors so best-effort cleanup never aborts the caller"). Re-verified as NOT a security +issue and NOT regressed: +- `COMMANDS_RULE_PATH` is a fixed constant (`os.homedir()` + literal segments, + `ambient.ts:21`) — no user/attacker input flows into the path, so no traversal or + injection surface. +- The swallowed operation is `unlink` of a deprecated documentation file. The only + failure outcome is that a benign stale `commands.md` survives — this grants no + access, leaks no data, and weakens no security control. Swallowing here does not + hide a security-relevant failure. +Per Cross-Cycle Awareness, not re-raised. + +## Pre-existing Issues (Not Blocking) + +The `devflowDir` inference at `ambient.ts:196-212` derives a directory from the +`Stop` hook command string in `settings.json` (`stopHook.split(' ')[0]` → +`path.resolve(..)`), then `addAmbientHook` interpolates it into the hook command +written back to `settings.json` (`path.join(devflowDir, 'scripts','hooks','run-hook') + ' preamble'`, +`ambient.ts:98`). This code is unchanged by this PR (context lines only — not added +or modified in the diff), so it is pre-existing and out of scope per the Iron Law. +It is also low-risk in practice: the source is the user's own local `settings.json` +(a trust boundary the user already controls), and the value is written as a hook +command path, not executed by this CLI. Noting for awareness only — not blocking. + +## Suggestions (Lower Confidence) + +None. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 0 | 1 | + +**Security Score**: 10 +**Recommendation**: APPROVED + +### Rationale +- Pure deletion/refactor of install plumbing. No new injection, path-traversal, auth, + crypto, secret, or network surface introduced. +- File path (`COMMANDS_RULE_PATH`) is a fixed, non-tainted constant — no traversal risk. +- File ops use `fs.unlink` on a constant path; no shell invocation, no string-built + commands, no `exec`/`spawn`. +- The catch-all error swallowing is best-effort cleanup of a benign deprecated file + and hides no security-relevant failure; intentionally resolved in cycle 1 and not + regressed. +- All references to removed exports (`installCommandsRule`, `removeCommandsRule`, + `COMMANDS_RULE_CONTENT`) are fully cleaned from `src/` and `tests/` — verified no + dangling imports. +- Aligns with ADR-001 (clean break, no migration code) and avoids PF-001. diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/testing.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/testing.md new file mode 100644 index 00000000..df4b7579 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/testing.md @@ -0,0 +1,99 @@ +# Testing Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**Date**: 2026-06-02_0013 +**Focus**: testing (PR #233) +**Diff**: `git diff main...HEAD -- tests/ambient.test.ts` + +## Verification Summary + +All 40 tests in `tests/ambient.test.ts` pass (`npx vitest run` → pass: 40 fail: 0). Source under +test (`src/cli/commands/ambient.ts`) read in full to verify assertions against actual behavior. +Cross-cycle: the Cycle-1 EACCES flip (`.rejects` → `.resolves`, line 341-344) correctly pins the +new fail-safe contract — not re-raised. + +Answers to the focused questions: + +1. **Ordering-invariant test proves the invariant** — YES. `addAmbientHook(withHook, ...)` takes the + early-return path (`changed` stays false because `hasPreamble` is true), and + `removeLegacyCommandsRule()` runs at `ambient.ts:107` *before* the `if (!changed) return` at + `ambient.ts:109`. The test asserting `fs.unlink` was called genuinely exercises the early-return + purge path. Valid. +2. **Fail-safe (swallows ALL errors)** — adequately tested. Source uses a bare `catch {}` + (not code-discriminated), so the single non-ENOENT case (EACCES) is representative of the entire + non-ENOENT branch. ENOENT + EACCES together prove the contract. Minor comment/coverage mismatch + noted below. +3. **Deleted tests genuinely dead** — YES. `installCommandsRule` and `COMMANDS_RULE_CONTENT` no + longer exist as exports in `ambient.ts`. The deleted `describe('installCommandsRule')` and + `describe('COMMANDS_RULE_CONTENT')` blocks tested removed symbols. No lost coverage of live behavior. +4. **Symmetric coverage gap** — `removeAmbientHook`'s unconditional purge on its early-return path is + NOT proven by any test. See HIGH finding below. + +## Issues in Your Changes (BLOCKING) + +None. The changed tests are correct, pass, and the deletions are clean. + +## Issues in Code You Touched (Should Fix) + +### HIGH +**Missing symmetric ordering-invariant test for `removeAmbientHook` early-return path** — `tests/ambient.test.ts:251` — Confidence: 90% +- Problem: The new ordering-invariant test (line 94-104) proves `addAmbientHook` purges the legacy + rule on its early-return path. But `removeAmbientHook` has the *same* structure — it calls + `removeLegacyCommandsRule()` at `ambient.ts:128` *before* its early-return at `ambient.ts:130` + (`if (!removedPrompt && !removedClassification) return`). No test asserts `fs.unlink` is called 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 it only asserts `result === input` — + it never asserts the purge ran. The prompt explicitly asks whether the unconditional invocation in + BOTH functions is covered; for `removeAmbientHook` it is not. A regression that moved + `removeLegacyCommandsRule()` after the early-return in `removeAmbientHook` would pass all current + tests. +- Fix: Add a mirror test in 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.) + +### MEDIUM +**Ordering-invariant test re-stubs mid-test in a brittle way** — `tests/ambient.test.ts:98-100` — Confidence: 82% +- Problem: The test calls `vi.restoreAllMocks()` then re-stubs `fs.unlink` to "assert on the second + call." This works, but it discards the call-count isolation the `beforeEach` provides and couples + the test to the fact that the *first* `addAmbientHook` also calls `unlink`. A cleaner assertion + reasons only about the second call rather than tearing down and rebuilding the spy. This is a + readability/robustness nit, not a correctness defect — the test does prove the invariant. +- Fix: Avoid the restore/re-stub dance; assert the call count delta instead, e.g. capture + `(fs.unlink as Mock).mock.calls.length` before the second `addAmbientHook` and assert it increased, + or clear with `vi.clearAllMocks()` (preserves the stub implementation) instead of + `vi.restoreAllMocks()` (removes it, forcing the re-stub). + +## Pre-existing Issues (Not Blocking) + +None relevant to the testing lens within the changed scope. + +## Suggestions (Lower Confidence) + +- **Fail-safe comment over-promises vs. test coverage** - `tests/ambient.test.ts:341` (Confidence: 70%) + — The renamed test exercises only EACCES; the source comment (`ambient.ts:61`) enumerates + "ENOENT, EACCES, EPERM, EROFS, etc." Because the `catch {}` is not code-discriminated, EACCES is a + sufficient representative, so this is acceptable — optionally add a non-Error throw (e.g. + `mockRejectedValue('weird')`) to pin that even non-Error rejections are swallowed. +- **`fs.unlink` spy assertion is boundary-coupled** - `tests/ambient.test.ts:104` (Confidence: 65%) + — Asserting on the `fs.unlink` spy is implementation-coupled, but `removeLegacyCommandsRule` returns + `void` and its only contract IS the unlink side-effect at a fixed path, so spying on the fs boundary + is the correct seam here. Noted for awareness, not a defect. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 1 | 1 | - | +| Pre-existing | - | - | 0 | 0 | + +**Testing Score**: 8 +**Recommendation**: APPROVED_WITH_CONDITIONS diff --git a/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/typescript.md b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/typescript.md new file mode 100644 index 00000000..b20cf380 --- /dev/null +++ b/.devflow/docs/reviews/refactor-remove-ambient-commands-rule/2026-06-02_0013/typescript.md @@ -0,0 +1,111 @@ +# TypeScript Review Report + +**Branch**: refactor/remove-ambient-commands-rule -> main +**Date**: 2026-06-02_0013 +**PR**: #233 + +## Scope + +TypeScript-relevant changes are confined to `src/cli/commands/ambient.ts`. The +`src/cli/commands/init.ts` and `src/cli/plugins.ts` diffs are text/comment-only +(option description string `'...plan auto-detection and command awareness'` → +`'...plan auto-detection'`) — no functional TypeScript to review. `tests/ambient.test.ts` +was reviewed for type-safety regressions. + +Verification performed: +- `npx tsc --noEmit` → exit 0 (clean typecheck, strict mode). +- `grep` across `src/` for `removeCommandsRule` / `installCommandsRule` / + `COMMANDS_RULE_CONTENT` → zero dangling references (rename + deletion fully propagated). +- Read `Settings`/`HookMatcher` types in `src/cli/utils/hooks.ts`. +- Read ADR-001 (clean-break) and `cli-rules` KNOWLEDGE.md. + +## Targeted Review (per prompt focus) + +| Focus item | Finding | Verdict | +|------------|---------|---------| +| Renamed fn signature | `removeLegacyCommandsRule(): Promise` — explicit return type, async | PASS | +| async/Promise of unlink | `await fs.unlink(...)` inside `try`; awaited at both call sites (`addAmbientHook` L107, `removeAmbientHook` L128) before early-return | PASS | +| Typed catch / error swallowing | `catch { }` (parameterless, documented) — no untyped `err`, no `any`, no type-info loss | PASS | +| Explicit return types | All exported fns annotated (`Promise`, `Promise`, `boolean`) | PASS | +| Settings immutability | In-place mutation of parsed object — see Pre-existing note below | ACCEPTABLE (boundary, local) | +| Result-type convention | Boundary exception is justified & codebase-consistent — see analysis | PASS | + +## Issues in Your Changes (BLOCKING) + +None. The changed TypeScript is type-clean under strict mode and the renamed +function is correctly typed. + +## Issues in Code You Touched (Should Fix) + +None at >=80% confidence. + +## Pre-existing Issues (Not Blocking) + +**Settings object mutated in place rather than returned immutably** — `ambient.ts:24-45`, `82-132` +**Confidence**: 88% +- Problem: `filterHookEntries` mutates `settings.hooks[eventName]` (reassignment + + `delete`) and `addAmbientHook`/`removeAmbientHook` push onto and delete keys of the + parsed `settings` object directly. This violates the global "Immutable by default — + return new objects, never mutate parameters" principle. +- Why not blocking: This is **pre-existing** structure — the mutation pattern predates + this PR (the diff only renames a function and deletes the install path; it does not + introduce new mutation). Per the review Iron Law, pre-existing non-CRITICAL issues are + informational. The mutated object is a freshly `JSON.parse`d local (never a shared + reference or a parameter the caller retains), so the blast radius is contained and there + is no aliasing hazard. Severity is MEDIUM, not CRITICAL. +- Fix (separate PR, optional): have `filterHookEntries` return a new `Settings` rather than + mutating, and build the updated hooks with spreads. Low value relative to churn given the + object is parse-local. + +## Boundary Exception Analysis (Result vs throw) + +The prompt asks whether the no-throw / Result-type convention is honored OR a justified +boundary exception applies. **A justified boundary exception applies — and the new code +strengthens it rather than weakening it:** + +- `addAmbientHook`/`removeAmbientHook` call `JSON.parse(settingsJson)` without a Result + wrapper. This is unchanged by the PR and is consistent with the entire `src/cli/` + module, which treats CLI commands as I/O boundaries that throw on malformed input + (confirmed by `cli-rules` KNOWLEDGE.md: `buildRulesMap` "throws on invalid names" as an + intentional early-catch boundary pattern, lines 97/219/238). The `JSON.parse` throw + propagates to commander's action handler — acceptable for a CLI entry point. +- The one place the PR *adds* error handling — `removeLegacyCommandsRule`'s `catch {}` — is + correctly a **non-throwing, non-Result best-effort cleanup**, which is the right call: + it is fire-and-forget purge of a deprecated file and must never abort the caller's + primary settings write. Swallowing here is intentional and well-documented (L69-72), + and is covered by dedicated tests (ENOENT + EACCES, test L335-345). This is the correct + shape for best-effort cleanup; a Result type would be over-engineering for a void cleanup + whose failure the caller deliberately ignores. + +## Decisions / Knowledge Applied + +- **applies ADR-001** (clean-break): `removeLegacyCommandsRule` is precisely the + "legacy hook file removal" one-time-cleanup item ADR-001 carves out as acceptable, while + the compat payload (`COMMANDS_RULE_CONTENT`, `installCommandsRule`) was fully deleted + rather than shimmed. The refactor is a clean break with no lingering backward-compat + cruft — directly in line with the decision. +- **cli-rules KNOWLEDGE.md**: confirms the CLI layer's accepted throw-at-boundary + convention, validating the Result-exception above. + +## Cross-Cycle Awareness + +PRIOR_RESOLUTIONS (Cycle 1) fixed: fail-safe error handling, README count→12, removed +fabricated citation. None re-raised here — the fail-safe `catch {}` is present and correct +in current code; this review does not cite any ADR/PF beyond ADR-001 which is verbatim in +DECISIONS_CONTEXT. + +## Suggestions (Lower Confidence) + +- **Parameterless `catch {}` loses the error object for any future diagnostic logging** - + `ambient.ts:68` (Confidence: 62%) — Intentional and documented; only flag if you later + want debug-trace visibility into unexpected unlink failures (e.g. EBUSY). Not an issue today. + +## Summary +| Category | CRITICAL | HIGH | MEDIUM | LOW | +|----------|----------|------|--------|-----| +| Blocking | 0 | 0 | 0 | - | +| Should Fix | - | 0 | 0 | - | +| Pre-existing | - | - | 1 | 0 | + +**TypeScript Score**: 9/10 +**Recommendation**: APPROVED diff --git a/.devflow/features/cli-rules/KNOWLEDGE.md b/.devflow/features/cli-rules/KNOWLEDGE.md index 2c5c166b..7816573e 100644 --- a/.devflow/features/cli-rules/KNOWLEDGE.md +++ b/.devflow/features/cli-rules/KNOWLEDGE.md @@ -1,13 +1,14 @@ --- feature: cli-rules name: Rules System CLI -description: "Use when adding new rules, modifying the rules install flow, implementing rule shadowing, or wiring rules into init/uninstall. Keywords: rules, shared/rules, rulesMap, buildRulesMap, isValidRuleName, LEGACY_RULE_NAMES, rulesEnabled, devflow rules, ~/.claude/rules/devflow, installRuleFile." +description: "Use when adding new rules, modifying the rules install flow, implementing rule shadowing, or wiring rules into init/uninstall. Keywords: rules, shared/rules, rulesMap, buildRulesMap, isValidRuleName, LEGACY_RULE_NAMES, rulesEnabled, devflow rules, ~/.claude/rules/devflow, installRuleFile, removeLegacyCommandsRule, ambient.ts, partitionSelectablePlugins, WORKFLOW_ORDER, combineSelection, shouldRetry." category: architecture directories: [src/cli/commands/, src/cli/utils/, shared/rules/, scripts/] referencedFiles: - src/cli/commands/rules.ts - src/cli/commands/init.ts - src/cli/commands/uninstall.ts + - src/cli/commands/ambient.ts - src/cli/plugins.ts - src/cli/utils/installer.ts - src/cli/utils/manifest.ts @@ -17,7 +18,7 @@ referencedFiles: - shared/rules/quality.md - shared/rules/reliability.md created: 2026-05-10 -updated: 2026-05-27 +updated: 2026-06-01 --- # Rules System CLI @@ -26,9 +27,7 @@ updated: 2026-05-27 Rules are ultra-condensed, always-on engineering principle files (~10 lines each) installed as flat `.md` files to `~/.claude/rules/devflow/`. Claude Code loads them automatically on every prompt, filling the guidance gap for quick edits that don't trigger a full skill pipeline. The system mirrors the skill build pipeline exactly: rules live in `shared/rules/`, are declared in `plugin.json` manifests and `DEVFLOW_PLUGINS`, distributed to plugins at build time, and installed (or shadowed) at runtime. -Unlike skills, which install universally from all plugins, rules are **plugin-scoped**: only rules belonging to the currently installed plugins are installed. This keeps core rules (security, engineering, quality, reliability) always present and optional-plugin rules (typescript, react, accessibility, ui-design, go, java, python, rust) only present when the user has that plugin installed. There are currently 13 rules total: 4 core + 8 language/ecosystem + 1 ambient-managed (`commands`). - -The `commands` rule is special: its source file lives in `shared/rules/commands.md` (added in v2.x ambient simplification), but it is managed by `ambient.ts` directly — NOT by the plugin rules system. `devflow ambient --enable` writes it to `~/.claude/rules/devflow/commands.md` and `devflow ambient --disable` removes it. It is not declared in any plugin's `rules` array, not returned by `getAllRuleNames()`, not affected by `devflow rules --enable/--disable`, and not cleaned up by `LEGACY_RULE_NAMES`. The `shared/rules/commands.md` file exists purely as a canonical copy for `ambient.ts` to reference its stable content hash. +Unlike skills, which install universally from all plugins, rules are **plugin-scoped**: only rules belonging to the currently installed plugins are installed. This keeps core rules (security, engineering, quality, reliability) always present and optional-plugin rules (typescript, react, accessibility, ui-design, go, java, python, rust) only present when the user has that plugin installed. There are currently 12 rules total: 4 core + 8 language/ecosystem. ## System Context @@ -69,13 +68,11 @@ paths: ["**/*.ts", "**/*.tsx"] - Bullet enforcement principles (4-5 lines) ``` -This two-tier design is what makes language rules low-cost: a Go rule never loads during TypeScript edits. The init Advanced-mode note shown to users describes this: *"They only load when you edit or generate code in a matching language — e.g., TypeScript rules activate for .ts files, Go rules for .go files. Not loaded all at once; minimal token cost."* - -Rules must be ultra-concise — ~10-15 lines total. Longer explanations belong in a skill, not a rule. +This two-tier design is what makes language rules low-cost: a Go rule never loads during TypeScript edits. Rules must be ultra-concise — ~10-15 lines total. Longer explanations belong in a skill, not a rule. ### Plugin Declaration -Rules are added to `PluginDefinition` in `src/cli/plugins.ts` via the required `rules` field (`string[]`). Core rules belong on `devflow-core-skills`; language-specific rules belong on their respective optional plugin. All 8 optional language/ecosystem plugins carry rules — typescript, react, accessibility, ui-design, go, java, python, rust. Non-language optional plugins (devflow-audit-claude) and all workflow plugins (devflow-implement, devflow-plan, devflow-code-review, devflow-resolve, devflow-debug, devflow-explore, devflow-research, devflow-release, devflow-self-review, devflow-bug-analysis, devflow-ambient) have `rules: []`. Only `devflow-core-skills` and the 8 language/UI plugins carry rules through the plugin system (the `commands` rule is managed by `ambient.ts` separately): +Rules are added to `PluginDefinition` in `src/cli/plugins.ts` via the required `rules` field (`string[]`). Core rules belong on `devflow-core-skills`; language-specific rules belong on their respective optional plugin. All 8 optional language/ecosystem plugins carry rules — typescript, react, accessibility, ui-design, go, java, python, rust. Non-language optional plugins (devflow-audit-claude) and all workflow plugins have `rules: []`. Only `devflow-core-skills` and the 8 language/UI plugins carry rules through the plugin system: ```typescript // In DEVFLOW_PLUGINS: @@ -93,7 +90,7 @@ Rules are added to `PluginDefinition` in `src/cli/plugins.ts` via the required ` // all follow the same pattern — one rule per plugin, same name as plugin suffix ``` -Plugins that have no rules must still include `rules: []` — the field is required on `PluginDefinition` (not optional). +Plugins that have no rules must still include `rules: []` — the field is required on `PluginDefinition` (not optional). `devflow-ambient` has `rules: []` — its legacy `commands` rule was removed; any stale `~/.claude/rules/devflow/commands.md` file is purged automatically on every `devflow ambient --enable/--disable` or `devflow init`. Four helper functions in `plugins.ts` serve distinct scopes: - `getAllRuleNames()` — unique names across ALL plugins, sorted (used by `devflow rules --list`) @@ -105,8 +102,6 @@ Four helper functions in `plugins.ts` serve distinct scopes: `scripts/build-plugins.ts` extends the skill/agent build to handle rules. The key difference from skills: rules are **flat files** (not directories), so no recursive copy is needed. The build script reads `plugin.json`'s `rules` array, clears and recreates the plugin's `rules/` directory, then copies each `shared/rules/{name}.md` into `plugins/{plugin}/rules/{name}.md`. The build fails with exit 1 if a declared rule is missing from `shared/rules/`. -The `shared/rules/` directory is optional — the build script warns but does not fail if it doesn't exist (rules are new; older devflow installs may lack it). - ### Install Flow Rule installation is handled by `installRuleFile`, an exported function in `src/cli/utils/installer.ts`. It is called from both `installViaFileCopy` (during init) and the `devflow rules --enable` command. Shadow resolution is centralized here: @@ -138,8 +133,6 @@ export async function installRuleFile( } ``` -`installViaFileCopy` calls this via `Promise.all([...rulesMap.entries()].map(...))` after creating the target directory. Shadow check: `~/.devflow/rules/{name}.md` overrides the built plugin source. - Key install properties: - Target: `~/.claude/rules/devflow/{name}.md` (flat, no subdirectory nesting) - Shadow: `~/.devflow/rules/{name}.md` overrides the Devflow source — same pattern as skills but for a flat file @@ -147,9 +140,7 @@ Key install properties: ### Manifest Tracking -`ManifestData.features.rules: boolean` tracks whether rules are enabled. The manifest reader in `src/cli/utils/manifest.ts` self-heals — when reading a manifest that lacks the `rules` key, it defaults to `true` (rules-on is the safe default for upgrades from pre-rules installs). - -The full `ManifestData.features` object (as of v2.x) tracks: `teams`, `ambient`, `memory`, `learn`, `hud`, `knowledge`, `decisions`, `rules`, `flags: string[]`, and `viewMode?: ViewMode`. When adding new toggleable features, extend this interface and add the corresponding self-heal default in `readManifest`. +`ManifestData.features.rules: boolean` tracks whether rules are enabled. The manifest reader in `src/cli/utils/manifest.ts` self-heals — when reading a manifest that lacks the `rules` field, it defaults to `true` (rules-on is the safe default for upgrades from pre-rules installs). ### `devflow rules` Command @@ -162,66 +153,115 @@ The `rules` command in `src/cli/commands/rules.ts` has four subcommands: | `--status` | Lists installed rules with owner plugin (shortened) and `[shadowed]` tag | | `--list` | Lists ALL available rules from all plugins with install indicator (✓/✗) | -The `--enable` path resolves the source directory relative to the compiled CLI's location (`path.resolve(__dirname, '../..'), 'plugins'`), not the source tree — this is the built `dist/plugins/` path. It also wipes the rules directory before reinstalling, mirroring the full-install init flow so that rules from previously uninstalled plugins are cleaned up. - Two private helpers are top-level named functions in `rules.ts` (not inline): - `isShadowed(devflowDir, ruleName)` — `fs.access` on `~/.devflow/rules/{name}.md`; returns `Promise` -- `formatRuleRow(name, devflowDir, ownerMap, suffix)` — builds a colorized display row; takes the `ownerMap` (a `Map` from `buildRulesMap`) as an explicit parameter. Both `--status` and `--list` build their own `buildRulesMap(DEVFLOW_PLUGINS)` call locally and pass it in — there is no module-level constant. +- `formatRuleRow(name, devflowDir, ownerMap, suffix)` — builds a colorized display row; both `--status` and `--list` build their own `buildRulesMap(DEVFLOW_PLUGINS)` call locally and pass it in — there is no module-level constant. + +## Init Flow Integration (Updated: Two-Step Plugin Selection) + +**Scope**: The interactive scope prompt was removed in feat(init). User scope is now the default for all TTY interactive runs. Only `--scope` flag or non-TTY path can set `local` scope. Non-TTY detects and logs "Non-interactive mode detected, using scope: user". + +**Two-step plugin selection**: `devflow init` (TTY, no `--plugin`) now presents two sequential `p.multiselect` prompts instead of one: +- **Step 1 — Workflow plugins**: All command-bearing plugins (excluding `devflow-core-skills`, `devflow-ambient`, `devflow-audit-claude`). Pre-selected: non-optional workflow plugins. +- **Step 2 — Language plugins**: All command-less selectable plugins (language/ecosystem). Nothing pre-selected. + +The split is computed by `partitionSelectablePlugins(DEVFLOW_PLUGINS)` in `plugins.ts`, which returns `{ workflow, language }` buckets. This is a pure function — no I/O, no mutation of the input array, deterministic, no side effects. + +**Bounded retry loop**: A `while (attempts < MAX_ATTEMPTS)` loop (MAX_ATTEMPTS = 3) guards both steps: +```typescript +const { plugins: combined, accepted } = combineSelection(workflowSelected, languageSelected); +if (accepted) { selectedPlugins = combined; break; } +if (!shouldRetry(attempts, MAX_ATTEMPTS, accepted)) { + p.cancel('Installation cancelled — no plugins selected.'); + process.exit(0); +} +p.log.warn('Select at least one plugin.'); +``` + +Two exported pure functions power the loop: +- `combineSelection(workflowSelected, languageSelected)` → `{ plugins: string[], accepted: boolean }` — merges the two arrays; `accepted` is true iff the union is non-empty. +- `shouldRetry(attempt, maxAttempts, accepted)` → `boolean` — returns true iff the selection was empty AND the attempt ceiling has not been reached; returns false when accepted or exhausted (caller exits on false + !accepted). + +Both functions are exported from `init.ts` for unit testing (same pattern as `parsePluginSelection`). Tests in `tests/init.test.ts` cover: accept-on-non-empty, empty-both-buckets, retry-exhaustion, and mid-loop iteration. + +**WORKFLOW_ORDER is now exported from `plugins.ts`**: +```typescript +export const WORKFLOW_ORDER: string[] = [ + '/research', '/explore', '/plan', '/implement', + '/code-review', '/resolve', '/self-review', '/bug-analysis', + '/debug', '/release', '/audit-claude', +]; +``` +`init.ts` imports it from `plugins.ts` rather than keeping a local duplicate. A regression guard test in `tests/plugins.test.ts` verifies every entry has a real backing command in `DEVFLOW_PLUGINS` (bidirectional: WORKFLOW_ORDER ⊆ commands AND commands ⊆ WORKFLOW_ORDER for the non-excluded set). `/bug-analysis` was added to WORKFLOW_ORDER in this same commit — the regression guard catches future omissions. + +**Excluded from plugin selection buckets**: `devflow-core-skills` (always installed), `devflow-ambient` (always installed), `devflow-audit-claude` (installable via `--plugin` only). + +**Rules in init**: `rulesEnabled` defaults to `true`. In Recommended mode, applied silently (no prompt). In Advanced mode, an explicit `p.note()` explains the per-language token model, followed by `p.confirm()`. CLI flag `--rules`/`--no-rules` overrides in both modes. `buildRulesMap(pluginsToInstall)` is called with the user's selected plugins — rules from non-selected optional plugins are excluded. Rules directory is NOT wiped on init (only on `devflow rules --enable`); stale rules are cleaned up via `LEGACY_RULE_NAMES` loop. ## Component Interactions -**init → rules**: During `devflow init`, `rulesEnabled` defaults to `true`. In **Recommended mode** (`--recommended`, non-TTY, or user chooses Recommended at the setup-mode prompt), the value is applied silently — rules status appears only in the printed summary note, no prompt is shown. In **Advanced mode**, an explicit `p.note()` explains the per-language token model, followed by `p.confirm()`. CLI flags (`--rules`/`--no-rules`) override the default in both modes. Once decided, `buildRulesMap(pluginsToInstall)` builds the name→plugin map passed to `installViaFileCopy` (when enabled), or an empty Map is used (when disabled). Rules are **overwritten per-file** by `installViaFileCopy` — the rules directory itself is not wiped on init. Stale renamed or removed rules are cleaned up via the `LEGACY_RULE_NAMES` loop (analogous to `LEGACY_SKILL_NAMES` for skills). If disabled, a post-install step removes the entire `~/.claude/rules/devflow/` directory. +**init → rules**: `rulesEnabled` flows through to `buildRulesMap(pluginsToInstall)` → `installViaFileCopy`. When disabled, post-install removes `~/.claude/rules/devflow/` entirely. -**uninstall → rules**: Full uninstall (`removeAllDevFlow`) includes `~/.claude/rules/devflow/` in its target directory list. Selective plugin uninstall (`computeAssetsToRemove`) computes which rules to remove using the same "retained by remaining plugins" logic as skills and agents — `removeSelectedPlugins` removes per-rule files from `~/.claude/rules/devflow/`. +**uninstall → rules**: Full uninstall (`removeAllDevFlow`) includes `~/.claude/rules/devflow/` in its target list. Selective plugin uninstall (`computeAssetsToRemove`) computes which rules to remove using the same "retained by remaining plugins" logic as skills. -**list → rules**: `devflow list` shows `rules` in the Features line of the installation summary when `manifest.features.rules` is true. +**list → rules**: `devflow list` shows `rules` in the Features line when `manifest.features.rules` is true. **build → install**: Rules are not installed from `shared/rules/` directly at runtime — the installer reads from `plugins/{plugin}/rules/`, which is the build output. Always run `npm run build` after modifying `shared/rules/` before testing install. +**plugins.ts → init.ts**: `partitionSelectablePlugins`, `WORKFLOW_ORDER`, `combineSelection`, `shouldRetry` are all exported from their respective modules and imported by `init.ts`. `combineSelection` and `shouldRetry` are in `init.ts` (not `plugins.ts`). + ## Constraints - Rules have no namespace prefix (unlike skills which install as `devflow:{name}/`). The directory `~/.claude/rules/devflow/` itself provides the namespace. -- Rules are plugin-scoped by design — no `buildFullRulesMap()` equivalent exists. If you need a rule in all installs, put it in `devflow-core-skills`. -- `LEGACY_RULE_NAMES` in `plugins.ts` is currently empty — the first rules are new. Add entries here when renaming or removing a rule. -- The `paths` frontmatter key must always be present — Claude Code uses it to determine loading scope. Core rules use `paths: []` (global); language rules use a glob array (file-type-scoped). Omitting the key entirely may break rule loading. -- `buildRulesMap` throws if any rule name fails the `isValidRuleName` check — misconfigured `plugin.json` entries are caught at map-build time, not at path-construction time. +- Rules are plugin-scoped by design — no `buildFullRulesMap()` equivalent exists. +- `LEGACY_RULE_NAMES` in `plugins.ts` is currently empty. Add entries when renaming or removing a rule. +- The `paths` frontmatter key must always be present. Core rules use `paths: []` (global); language rules use a glob array (file-type-scoped). Omitting the key may break rule loading. +- `buildRulesMap` throws if any rule name fails `isValidRuleName` — misconfigured `plugin.json` entries are caught at map-build time, not at path-construction time. +- `partitionSelectablePlugins` uses the presence of `commands.length > 0` as the sole criterion for the workflow bucket — command-less selectable plugins always land in the language bucket. If a non-language command-less plugin is added, update the bucket name or add an explicit category field. ## Anti-Patterns -- **Adding a language rule to `devflow-core-skills`**: Core rules install for every user. Language-specific rules (TypeScript, React, Go) belong in their optional plugin so users who don't use that language don't pay the token cost. -- **Using `paths: []` on a language-specific rule**: Language rules must scope to their file types (e.g. `paths: ["**/*.ts", "**/*.tsx"]`). Using `paths: []` makes them load on every prompt for every user, eliminating the per-language token savings. -- **Using a file-type path on a core rule**: Core rules (security, engineering, quality) must use `paths: []` — they apply cross-language. A path filter would silently skip them for non-matching files. -- **Installing rules from `shared/rules/` directly at runtime**: The installer reads from `plugins/{plugin}/rules/` (build output), not `shared/rules/`. Skipping `npm run build` after editing a rule will silently install the old version. -- **Using a skill for ultra-concise guidance**: If content fits in ~15 lines and applies universally, prefer a rule. Rules load on every prompt with zero user action; skills require the router or explicit invocation. -- **Long rule files**: Rules should be ~10-15 lines. If a rule grows beyond ~20 lines, extract the detail into a skill's `references/` directory and keep only the iron law in the rule. +- **Adding a language rule to `devflow-core-skills`**: Core rules install for every user. Language-specific rules belong in their optional plugin. +- **Using `paths: []` on a language-specific rule**: Language rules must scope to their file types. Using `paths: []` makes them load on every prompt, eliminating per-language token savings. +- **Using a file-type path on a core rule**: Core rules (security, engineering, quality) must use `paths: []` — they apply cross-language. +- **Installing rules from `shared/rules/` directly at runtime**: The installer reads from `plugins/{plugin}/rules/` (build output). Skipping `npm run build` silently installs the old version. +- **Unbounded plugin selection loop**: The bounded `while (attempts < MAX_ATTEMPTS)` + `shouldRetry` guard is the pattern — never replace with `while (true)`. +- **Long rule files**: Rules should be ~10-15 lines. If a rule grows beyond ~20 lines, extract the detail into a skill's `references/` directory. - **Omitting `rules: []` on a plugin**: The `rules` field is required on `PluginDefinition`. Omitting it causes TypeScript errors at build time. ## Gotchas -- **Rules ARE wiped on full install but not on partial**: `installViaFileCopy` wipes `~/.claude/rules/devflow/` at the start of a full install (alongside commands and agents). On a partial install (`devflow init --plugin=typescript`), the rules directory is NOT wiped — only per-plugin assets are overwritten. Use `devflow rules --enable` to get a clean reinstall of the current plugin set — it always wipes first regardless of install mode. -- **`devflow rules --enable` resolves plugin dirs from dist/**: The command computes the plugins directory as `path.resolve(__dirname, '../..', 'plugins')` relative to the compiled CLI file. In development, this means running the command against `dist/plugins/`, so you must build before running. -- **Shadow files are flat, not directories**: Skills shadow at `~/.devflow/skills/{name}/` (a directory). Rules shadow at `~/.devflow/rules/{name}.md` (a flat file). The `isShadowed` check uses `fs.access()` on the flat path, not `fs.stat()` for a directory. -- **Manifest defaults `rules: true` on read**: Old manifests without the `rules` field are read as `rules: true`. This means upgrading users get rules enabled automatically, which is the desired behavior but worth knowing when reading the manifest. -- **`buildRulesMap` throws on invalid names**: If a `plugin.json` declares a rule name with uppercase letters, dots, or slashes, `buildRulesMap` throws immediately. This is intentional — catch misconfiguration early rather than silently writing a path-traversal-susceptible file. -- **`commands.md` is ambient-managed, not plugin-managed**: `shared/rules/commands.md` exists in the source tree but is NOT declared in any plugin's `rules` array. It is installed/removed exclusively by `ambient.ts` (`devflow ambient --enable/--disable`). Calling `devflow rules --enable` will NOT install `commands.md`. Calling `devflow rules --disable` will NOT remove it. `devflow rules --list` will NOT show it. This is intentional — commands awareness is an ambient mode feature, not a rules-system feature. The `shared/rules/commands.md` file exists so `ambient.ts` can embed its content as a constant (`COMMANDS_RULE_CONTENT`) with a stable source of truth. -- **Rules have no runtime sentinel**: Unlike knowledge (`.devflow/features/.disabled`), decisions (`.devflow/decisions/.disabled`), memory (`.devflow/memory/.working-memory-disabled`), and learn (`.devflow/memory/.learning-disabled`), rules have no `.disabled` file sentinel. Both `manageSentinel` and `writeSidecarConfig` calls in `init.ts` conspicuously omit rules — this is intentional. The sidecar system (which writes `.devflow/sidecar/config.json` entries for memory, learning, decisions, and knowledge to coordinate background agents) has no entry for rules because rules have no background agent: they are static files loaded by Claude Code directly. Disabling rules is a destructive operation: `devflow rules --disable` removes `~/.claude/rules/devflow/` entirely, and `devflow init --no-rules` does the same. There is no way to temporarily suppress rules without removing the files themselves. -- **Core vs language rules have different token behavior**: Core rules (security, engineering, quality, reliability) load on every prompt regardless of file type. Language rules only activate when Claude is working with a matching file. A user without the TypeScript plugin pays zero cost for TypeScript rules — but a user with it only pays the cost when editing `.ts`/`.tsx` files. -- **manifest.ts contains a `kb → knowledge` migration self-heal**: `readManifest` detects `features.kb` and migrates it to `features.knowledge` in-place (ADR-001 clean-break applies to install-time assets like rules, skills, commands — not to disk data that users cannot easily migrate themselves). This is the only backward-compat code in `manifest.ts`; do not add more. For rules, `LEGACY_RULE_NAMES` in `plugins.ts` is the correct pattern when renaming rule files — no manifest migration needed. +- **Rules ARE wiped on full install but not on partial**: `installViaFileCopy` wipes `~/.claude/rules/devflow/` at the start of a full install. On a partial install (`devflow init --plugin=typescript`), the rules directory is NOT wiped. Use `devflow rules --enable` to get a clean reinstall — it always wipes first. +- **`devflow rules --enable` resolves plugin dirs from dist/**: Computes the plugins directory relative to the compiled CLI file. Must build before running. +- **Shadow files are flat, not directories**: Skills shadow at `~/.devflow/skills/{name}/` (a directory). Rules shadow at `~/.devflow/rules/{name}.md` (a flat file). +- **Manifest defaults `rules: true` on read**: Old manifests without the `rules` field are read as `rules: true`. Upgrading users get rules enabled automatically. +- **`buildRulesMap` throws on invalid names**: Uppercase letters, dots, or slashes in a `plugin.json` rules entry cause an immediate throw — intentional early-catch. +- **`commands.md` has been removed**: The ambient-managed commands rule no longer exists. Any stale `~/.claude/rules/devflow/commands.md` from prior installs is purged automatically by `removeLegacyCommandsRule()` which runs unconditionally in both `addAmbientHook` and `removeAmbientHook`. `devflow rules --enable/--disable` never touched it and still does not. +- **Scope prompt removed**: Interactive TTY runs no longer ask for scope — user scope is the automatic default. The `--scope` flag still works (for `local` installs or scripted `user` overrides), and non-TTY still logs and defaults to `user`. +- **Two-step selection requires `partitionSelectablePlugins` for bucket assignment**: Do NOT sort or filter `DEVFLOW_PLUGINS` manually in init code. Always delegate to `partitionSelectablePlugins`. The workflow-bucket predicate is `commands.length > 0` — the language-bucket is every command-less selectable plugin (implicit convention; not enforced by types). +- **`WORKFLOW_ORDER` regression guard is bidirectional**: `tests/plugins.test.ts` verifies WORKFLOW_ORDER entries correspond to real commands AND that commands not in the excluded set are covered. Adding a new workflow command requires updating WORKFLOW_ORDER or the test will fail. +- **Rules have no runtime sentinel**: Unlike knowledge (`.devflow/features/.disabled`), decisions, memory, and learn, rules have no `.disabled` file. Disabling rules is destructive: `devflow rules --disable` removes the directory entirely. There is no temporary suppression path. +- **Core vs language rules have different token behavior**: Core rules load on every prompt. Language rules only activate when Claude is working with a matching file type. +- **manifest.ts contains a `kb → knowledge` migration self-heal**: `readManifest` detects `features.kb` and migrates it to `features.knowledge` in-place. This is the only backward-compat code in `manifest.ts`; do not add more. For rules, `LEGACY_RULE_NAMES` is the correct pattern when renaming rule files. ## Key Files -- `shared/rules/` — source of truth for all rule content; flat `.md` files (13 total, including `commands.md`) -- `src/cli/plugins.ts` — `DEVFLOW_PLUGINS` `rules` field, `buildRulesMap()`, `getAllRuleNames()`, `isValidRuleName()`, `LEGACY_RULE_NAMES` +- `shared/rules/` — source of truth for all rule content; flat `.md` files (12 total) +- `src/cli/plugins.ts` — `DEVFLOW_PLUGINS` `rules` field, `buildRulesMap()`, `getAllRuleNames()`, `isValidRuleName()`, `LEGACY_RULE_NAMES`, `WORKFLOW_ORDER`, `partitionSelectablePlugins()` +- `src/cli/commands/init.ts` — `rulesEnabled` flag; two-step plugin selection with `partitionSelectablePlugins`; `combineSelection`, `shouldRetry` pure helpers (exported for tests); `WORKFLOW_ORDER` import; Recommended-mode silent apply vs Advanced-mode note+confirm; `buildRulesMap(pluginsToInstall)`; `LEGACY_RULE_NAMES` stale-file cleanup loop - `src/cli/commands/rules.ts` — `devflow rules` command (enable/disable/status/list) -- `src/cli/commands/ambient.ts` — manages `commands.md` rule via `COMMANDS_RULE_PATH` / `COMMANDS_RULE_CONTENT` / `installCommandsRule()` / `removeCommandsRule()`; this is the ONLY manager for that rule +- `src/cli/commands/ambient.ts` — purges legacy `commands.md` via `COMMANDS_RULE_PATH` / `removeLegacyCommandsRule()`; called unconditionally from `addAmbientHook` and `removeAmbientHook` so stale files are cleaned up on every enable/disable/init - `src/cli/utils/installer.ts` — `installRuleFile` (exported); `installViaFileCopy` rules section -- `src/cli/commands/init.ts` — `rulesEnabled` flag (default `true`); Recommended-mode silent apply vs Advanced-mode note+confirm; `buildRulesMap(pluginsToInstall)`; `LEGACY_RULE_NAMES` stale-file cleanup loop; post-install removal of rules dir when disabled - `src/cli/commands/uninstall.ts` — `computeAssetsToRemove` includes rules; `removeAllDevFlow` removes rules dir; `removeSelectedPlugins` removes per-rule files - `src/cli/utils/manifest.ts` — `ManifestData.features.rules` with `true` self-heal default - `scripts/build-plugins.ts` — build-time distribution from `shared/rules/` → `plugins/*/rules/` +- `tests/plugins.test.ts` — `partitionSelectablePlugins` (8 cases) + `WORKFLOW_ORDER` regression guard (4 cases, bidirectional) +- `tests/init.test.ts` — `combineSelection` and `shouldRetry` unit tests ## Related -- ADR-001: No migration code for devflow refactors — clean break philosophy (applies: `LEGACY_RULE_NAMES` starts empty; when rules are renamed, add legacy names there without migration logic) +- ADR-001: No migration code for devflow refactors — clean break philosophy - Skills system (parallel architecture): `src/cli/utils/installer.ts` `installViaFileCopy` skills section is the model rules followed - Feature flags: `src/cli/utils/flags.ts` — another toggleable feature using the same manifest.features pattern +- Ambient simplification (c51114d): introduced `commands.md` rule + ambient-managed separation +- Init flow simplification (5143d73–154899b): two-step selection, `partitionSelectablePlugins`, `WORKFLOW_ORDER` export, `combineSelection`/`shouldRetry` diff --git a/.devflow/features/hooks/KNOWLEDGE.md b/.devflow/features/hooks/KNOWLEDGE.md index 14f79e27..d91779c7 100644 --- a/.devflow/features/hooks/KNOWLEDGE.md +++ b/.devflow/features/hooks/KNOWLEDGE.md @@ -11,8 +11,11 @@ referencedFiles: - scripts/hooks/lib/staleness.cjs - scripts/hooks/json-helper.cjs - scripts/hooks/sidecar-capture + - scripts/hooks/sidecar-collect-tasks + - scripts/hooks/sidecar-dispatch - scripts/hooks/sidecar-evaluate - scripts/hooks/sidecar-recover + - scripts/hooks/eval-curation - scripts/hooks/session-start-context - shared/skills/sidecar/SKILL.md - src/cli/commands/decisions.ts @@ -160,12 +163,14 @@ No `claude -p` subprocess is spawned for individual features. The sidecar skill ## Anti-Patterns -- **Editing installed copies** — always edit `scripts/hooks/`, then `npm run build` + `devflow init`. Changes to `~/.devflow/scripts/hooks/` are silently overwritten on reinstall (avoids PF-007). -- **Calling `decisions-append` during curation** — it acquires `.decisions.lock` internally; calling it while holding that lock deadlocks. Use Edit tool for deprecation (avoids SKILL.md explicit warning). +- **Editing installed copies** — always edit `scripts/hooks/`, then `npm run build` + `devflow init`. Changes to `~/.devflow/scripts/hooks/` are silently overwritten on reinstall (PF-007). +- **Calling `decisions-append` during curation** — it acquires `.decisions.lock` internally; calling it while holding that lock deadlocks. Use Edit tool for deprecation (SKILL.md explicit warning). - **Holding a lock across tool calls** — the processor's lock lifecycle must be: acquire → single Bash → release. Never span multiple tool calls under one lock. - **Spawning multiple background agents** — the main model makes exactly one `Agent(run_in_background: true)` call. The processor handles all task types sequentially inside one agent. -- **Assuming `sidecar-dispatch` injects the SIDECAR directive** — after the LLM refactor, `sidecar-dispatch` is capture-only (UserPromptSubmit); the SIDECAR directive is emitted by `session-start-context` (SessionStart). `sidecar-dispatch` no longer drives the processor. +- **Assuming `sidecar-dispatch` injects the SIDECAR directive** — `sidecar-dispatch` is capture-only (UserPromptSubmit); the SIDECAR directive is emitted by `session-start-context` (SessionStart) (ADR-009). +- **Using `additionalContext` for critical directives** — models deprioritize `additionalContext` when a user question is present; critical maintenance directives must be anchored to SessionStart (PF-008). - **Using `decisions-usage-scan.cjs` to read cite counts** — it is a write-path tool that increments counts from session text. Read `.decisions-usage.json` directly for reporting or curation decisions. +- **Writing artifact content in deterministic scripts** — memory, observations, ADR/PF bodies, and knowledge bases must be authored by the LLM processor; plumbing scripts handle only structural writes (ADR-008). ## Gotchas @@ -180,10 +185,13 @@ No `claude -p` subprocess is spawned for individual features. The sidecar skill ## Key Files - `scripts/hooks/sidecar-capture` — Stop hook; queue append + memory marker write (120s throttle) -- `scripts/hooks/sidecar-evaluate` — SessionEnd hook; orchestrator sourcing eval-* modules -- `scripts/hooks/sidecar-recover` — stale `.processing` recovery helper; per-type thresholds -- `scripts/hooks/session-start-context` — SessionStart hook; recover → collect → emit directive -- `scripts/hooks/json-helper.cjs` — plumbing ops: `merge-observation`, `decisions-append`, atomic writes +- `scripts/hooks/sidecar-dispatch` — UserPromptSubmit hook; capture-only (user turn append to pending-turns queue); no directive emission +- `scripts/hooks/sidecar-evaluate` — SessionEnd hook; orchestrator sourcing eval-* modules (eval-learning, eval-decisions, eval-knowledge, eval-curation, eval-reinforce, eval-helpers) +- `scripts/hooks/eval-curation` — sourced by sidecar-evaluate; writes `curation.{session}.json` marker on 7-day throttle (D58) +- `scripts/hooks/sidecar-recover` — sourced helper; stale `.processing` recovery per-type thresholds; JUST_RECOVERED guard; orphaned pending-turns recovery +- `scripts/hooks/sidecar-collect-tasks` — sourced helper; collects pending `.json` markers into `_SIDECAR_TASKS`; COLLECT_LIMIT=50 FIFO; deletes disabled-feature markers +- `scripts/hooks/session-start-context` — SessionStart hook (no set -e); three independent sections: 1.5 decisions TL;DR, 1.75 learned behaviors, 2 sidecar spawn directive +- `scripts/hooks/json-helper.cjs` — plumbing ops: `merge-observation`, `decisions-append`, atomic writes; does NOT contain judgment logic - `scripts/hooks/lib/transcript-filter.cjs` — two-channel filter: USER_SIGNALS + DIALOG_PAIRS - `scripts/hooks/lib/staleness.cjs` — annotates log entries with `mayBeStale` based on file existence - `scripts/hooks/lib/feature-knowledge.cjs` — KB index, staleness checks, `updateIndex`, slug validation diff --git a/.devflow/features/index.json b/.devflow/features/index.json index 184fe79c..3bd40f06 100644 --- a/.devflow/features/index.json +++ b/.devflow/features/index.json @@ -3,7 +3,7 @@ "features": { "cli-rules": { "name": "Rules System CLI", - "description": "Use when adding new rules, modifying the rules install flow, implementing rule shadowing, or wiring rules into init/uninstall. Keywords: rules, shared/rules, rulesMap, buildRulesMap, isValidRuleName, LEGACY_RULE_NAMES, rulesEnabled, devflow rules, ~/.claude/rules/devflow, installRuleFile, commands.md, ambient.ts.", + "description": "Use when adding new rules, modifying the rules install flow, implementing rule shadowing, or wiring rules into init/uninstall. Keywords: rules, shared/rules, rulesMap, buildRulesMap, isValidRuleName, LEGACY_RULE_NAMES, rulesEnabled, devflow rules, ~/.claude/rules/devflow, installRuleFile, removeLegacyCommandsRule, ambient.ts, partitionSelectablePlugins, WORKFLOW_ORDER, combineSelection, shouldRetry.", "directories": [ "src/cli/commands/", "src/cli/utils/", @@ -22,10 +22,9 @@ "shared/rules/security.md", "shared/rules/engineering.md", "shared/rules/quality.md", - "shared/rules/reliability.md", - "shared/rules/commands.md" + "shared/rules/reliability.md" ], - "lastUpdated": "2026-05-27T19:30:17.997Z", + "lastUpdated": "2026-06-01T20:16:11.526Z", "createdBy": "devflow-knowledge" }, "hooks": { @@ -42,13 +41,16 @@ "scripts/hooks/lib/staleness.cjs", "scripts/hooks/json-helper.cjs", "scripts/hooks/sidecar-capture", + "scripts/hooks/sidecar-collect-tasks", + "scripts/hooks/sidecar-dispatch", "scripts/hooks/sidecar-evaluate", "scripts/hooks/sidecar-recover", + "scripts/hooks/eval-curation", "scripts/hooks/session-start-context", "shared/skills/sidecar/SKILL.md", "src/cli/commands/decisions.ts" ], - "lastUpdated": "2026-05-31T22:27:20.021Z", + "lastUpdated": "2026-06-01T10:36:05.464Z", "createdBy": "devflow-knowledge" } } diff --git a/CLAUDE.md b/CLAUDE.md index d0d6a4fe..248f4989 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -26,7 +26,7 @@ Plugin marketplace with 21 plugins (12 core + 9 optional language/ecosystem), ea | `devflow-release` | Adaptive project release with learned configuration | Optional | | `devflow-self-review` | Self-review (Simplifier + Scrutinizer) | No | | `devflow-bug-analysis` | Proactive bug finding with static and semantic analysis | No | -| `devflow-ambient` | Ambient mode — plan auto-detection and command awareness | No | +| `devflow-ambient` | Ambient mode — plan auto-detection | No | | `devflow-core-skills` | Auto-activating quality enforcement | No | | `devflow-audit-claude` | Audit CLAUDE.md files (optional) | No | | `devflow-typescript` | TypeScript language patterns (optional) | No | @@ -46,7 +46,7 @@ Commands with Teams Variant ship as `{name}.md` (parallel subagents) and `{name} **Working Memory**: Three shell-script hooks (`scripts/hooks/`) replace the old 8-hook system with a sidecar architecture. Toggleable via `devflow memory --enable/--disable/--status` or `devflow init --memory/--no-memory`. Feature state is stored in `.devflow/sidecar/config.json` (primary source of truth); runtime sentinels (`.devflow/memory/.working-memory-disabled`, `.devflow/memory/.learning-disabled`) provide defense-in-depth for hooks that read files directly. `sidecar-capture` (Stop hook) — captures user/assistant turns to `.devflow/memory/.pending-turns.jsonl` queue and writes `.devflow/sidecar/memory.json` marker when throttle has expired (>2 min); uses atomic temp+mv for marker writes, PID-unique temp files to prevent concurrent clobbering, and mkdir-based locking for queue overflow truncation across concurrent sessions. `sidecar-dispatch` (UserPromptSubmit hook) — **capture-only**: appends the user turn to `.pending-turns.jsonl`; it does NOT emit any SIDECAR directive (directive emission moved to `session-start-context`). `sidecar-evaluate` (SessionEnd hook) — orchestrator that sources `eval-helpers` + 5 feature modules (`eval-reinforce`, `eval-learning`, `eval-decisions`, `eval-knowledge`, `eval-curation`) after shared setup; each module uses `${VAR:?}` fail-fast guards and `_MODULENAME_` variable prefixes for namespace isolation; evaluates whether to write learning, decisions, knowledge, or curation sidecar markers; writes per-session marker files (e.g., `learning.{session_id}.json`) using atomic temp+mv; uses mkdir-based locking (`sidecar-lock`) to serialize reinforcement operations across concurrent sessions. Always-on SessionStart hook (`session-start-context`) — recovers stale `.processing` markers (via `sidecar-recover` → `sidecar_recover_stale`), collects pending markers (via `sidecar-collect-tasks`), and emits a **SIDECAR MAINTENANCE** directive instructing the main model to spawn ONE background `sidecar-processor` agent (`Agent(run_in_background:true)`, `devflow:sidecar` skill); directive emission is throttled to 120s. The sidecar processor claims each marker atomically (`{type}.{session}.json` → `{type}.{session}.processing`), heartbeats by touching `.processing`, performs all detection/matching/materialization/curation via LLM, then deletes the marker (or renames to `.failed` on error). SessionStart hook (`session-start-memory`) → injects previous memory + git state as `additionalContext` on `/clear`, startup, or compact; gated by `.working-memory-disabled` sentinel. PreCompact hook → saves git state + WORKING-MEMORY.md snapshot. Updates `.devflow/memory/WORKING-MEMORY.md` with structured sections (`## Now`, `## Progress`, `## Decisions`, `## Modified Files`, `## Context`, `## Session Log`). The sidecar processor uses rename-to-claim for queue consumption (atomically renames `.pending-turns.jsonl` → `.pending-turns.processing`) to prevent concurrent session appends from being lost during processing. Disabling memory writes `memory: false` to sidecar config — hooks remain registered (shared across features). `removeMemoryHooks` (used by `devflow init --no-memory`) also removes pre-sidecar legacy hooks (`prompt-capture-memory`, `stop-update-memory`, `stop-update-learning`, `session-end-learning`, `session-end-decisions`, `session-end-knowledge-refresh`) from upgrading users. Use `devflow memory --clear` to clean up pending queue files across projects. Zero-ceremony context preservation. -**Ambient Mode**: Two-component system for zero-overhead session enhancement. UserPromptSubmit hook (`preamble`) detects structured implementation plans — when a prompt contains `## Goal`, `## Steps`, and `## Files` markers, it outputs a directive to invoke `devflow:implement` via the Skill tool. Zero overhead for normal prompts — hook outputs nothing. Commands awareness rule (`~/.claude/rules/devflow/commands.md`) lists available `/devflow:` commands and documents the plan auto-execution trigger for passive reference. The rule is installed/removed by `devflow ambient --enable/--disable`; it is managed by `ambient.ts` directly, not by the rules plugin system. Toggleable via `devflow ambient --enable/--disable/--status` or `devflow init`. +**Ambient Mode**: Single-component system for zero-overhead session enhancement. UserPromptSubmit hook (`preamble`) detects structured implementation plans — when a prompt contains `## Goal`, `## Steps`, and `## Files` markers, it outputs a directive to invoke `devflow:implement` via the Skill tool. Zero overhead for normal prompts — hook outputs nothing. Any legacy `commands.md` rule left by prior installs is auto-removed on every `devflow ambient --enable/--disable` or `devflow init`. Toggleable via `devflow ambient --enable/--disable/--status` or `devflow init`. **Self-Learning**: The sidecar processor (a background LLM agent spawned at SessionStart) handles all detection and materialization. Transcript content is split into two channels by `scripts/hooks/lib/transcript-filter.cjs`: `USER_SIGNALS` (plain user messages) and `DIALOG_PAIRS` (prior-assistant + user turns). The processor receives these channels from sidecar markers and uses LLM judgment — not deterministic thresholds — for detection, semantic deduplication, and promotion decisions. @@ -62,7 +62,7 @@ Debug logs stored at `~/.devflow/logs/{project-slug}/`. **Feature Knowledge Bases**: Per-feature `.devflow/features/` directory containing KNOWLEDGE.md files that capture area-specific patterns, conventions, architecture, and gotchas. Knowledge bases are created as side-effects of implementation (`/implement` Phase 12), loaded automatically across all workflows via `FEATURE_KNOWLEDGE` variable (companion to `DECISIONS_CONTEXT`), and use staleness detection via git log against `referencedFiles`. Index at `.devflow/features/index.json` (object keyed by slug). Managed via `devflow knowledge list|create|check|refresh|remove`. Knowledge agent (sonnet) structures exploration outputs into KNOWLEDGE.md. `apply-feature-knowledge` skill provides consumption algorithm for agents. `.devflow/features/.knowledge.lock` is gitignored (transient lock directory for concurrent index writes, added automatically by `devflow init`). `devflow knowledge list` — List all feature knowledge bases with staleness status. `devflow knowledge create ` — Create a new knowledge base via claude -p exploration. `devflow knowledge check` — Check all knowledge bases for staleness. `devflow knowledge refresh [slug]` — Refresh stale knowledge base(s). `devflow knowledge remove ` — Remove a knowledge base and its index entry. Note: `/debug` keeps FEATURE_KNOWLEDGE orchestrator-local (investigation workers examine code without pre-loaded context). Toggleable via `devflow knowledge --enable/--disable/--status` or `devflow init --knowledge/--no-knowledge`. SessionEnd hook auto-refreshes stale knowledge bases (throttled to once per 2 hours, max 3 per run). `.devflow/features/.disabled` sentinel gates Phase 12 generation and refresh hook. -**Rules**: Ultra-concise, always-on engineering principle files (~10-15 lines each) installed to `~/.claude/rules/devflow/` as flat `.md` files. Claude Code loads them automatically on every prompt — no hooks required — filling the guidance gap for quick edits that don't trigger a full skill pipeline. Rules flow through the same four-stage pipeline as skills: authored in `shared/rules/`, distributed to `plugins/*/rules/` at build time, installed (or shadowed) at runtime, and activated automatically. Unlike skills (which install universally from all plugins), rules are **plugin-scoped**: only rules belonging to selected plugins are installed. This keeps core rules (`security`, `engineering`, `quality`, `reliability` from `devflow-core-skills`) always present, and language/ecosystem rules (`typescript`, `react`, `go`, etc.) present only when the user has that plugin installed. Shadow overrides: `~/.devflow/rules/{name}.md` overrides the Devflow source. Toggleable via `devflow rules --enable/--disable/--status/--list` or `devflow init --rules/--no-rules`. Stored in manifest `features.rules: boolean` (self-heals to `true` on old manifests). Currently 13 rules: 4 core + 8 language/UI + 1 ambient-managed (commands). `paths: []` YAML frontmatter must remain — it signals Claude Code to apply the rule globally. +**Rules**: Ultra-concise, always-on engineering principle files (~10-15 lines each) installed to `~/.claude/rules/devflow/` as flat `.md` files. Claude Code loads them automatically on every prompt — no hooks required — filling the guidance gap for quick edits that don't trigger a full skill pipeline. Rules flow through the same four-stage pipeline as skills: authored in `shared/rules/`, distributed to `plugins/*/rules/` at build time, installed (or shadowed) at runtime, and activated automatically. Unlike skills (which install universally from all plugins), rules are **plugin-scoped**: only rules belonging to selected plugins are installed. This keeps core rules (`security`, `engineering`, `quality`, `reliability` from `devflow-core-skills`) always present, and language/ecosystem rules (`typescript`, `react`, `go`, etc.) present only when the user has that plugin installed. Shadow overrides: `~/.devflow/rules/{name}.md` overrides the Devflow source. Toggleable via `devflow rules --enable/--disable/--status/--list` or `devflow init --rules/--no-rules`. Stored in manifest `features.rules: boolean` (self-heals to `true` on old manifests). Currently 12 rules: 4 core + 8 language/UI. `paths: []` YAML frontmatter must remain — it signals Claude Code to apply the rule globally. **Three independent self-learning pipelines** (all toggleable separately): - `devflow learn --enable/--disable` — Learning pipeline (workflow + procedural detection, materialized by sidecar processor from USER_SIGNALS) @@ -79,7 +79,7 @@ Debug logs stored at `~/.devflow/logs/{project-slug}/`. devflow/ ├── shared/skills/ # 42 skills (single source of truth) ├── shared/agents/ # 15 shared agents (single source of truth) -├── shared/rules/ # 13 rules (single source of truth; flat .md files) +├── shared/rules/ # 12 rules (single source of truth; flat .md files) ├── plugins/devflow-*/ # 21 plugins (12 core + 9 optional language/ecosystem) ├── docs/reference/ # Detailed reference documentation ├── scripts/ # Helper scripts (statusline, docs-helpers) diff --git a/README.md b/README.md index e5c872a0..c3182176 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ Claude Code is powerful. But every session starts from scratch. Context evaporat Devflow fixes this. Install once, forget about it. Your code gets better automatically. -It provides passive command awareness via always-on rules, and detects when you paste a structured plan so it can auto-execute — no classification overhead for normal prompts. Complex tasks get an advanced TDD and EDD harness with quality gates at every step. +It detects when you paste a structured plan so it can auto-execute — no classification overhead for normal prompts. Complex tasks get an advanced TDD and EDD harness with quality gates at every step. ## See it work @@ -41,7 +41,7 @@ you: add rate limiting to the /api/upload endpoint ## What you get -**Ambient intelligence.** Devflow gives commands passive awareness via always-on rules, and detects structured plans for automatic execution — normal prompts get zero overhead, no classification step. You never invoke it manually. Init and forget. +**Ambient intelligence.** Devflow detects structured plans for automatic execution — normal prompts get zero overhead, no classification step. You never invoke it manually. Init and forget. **Memory that persists.** Session context survives restarts, `/clear`, and context compaction. Your AI picks up exactly where it left off. Architectural decisions and known pitfalls accumulate in `.devflow/decisions/` and inform every future session. No manual bookkeeping. @@ -53,7 +53,7 @@ you: add rate limiting to the /api/upload endpoint **Skill shadowing.** Override any built-in skill with your own version. Drop a file into `~/.devflow/skills/{name}/` and the installer uses yours instead of the default — same activation, your rules. -**Always-on rules.** 13 ultra-condensed engineering principles (~10 lines each) load on every prompt — security, quality, and language-specific guidance (TypeScript, React, Go, Python, Java, Rust). Rules install from your selected plugins only, so a Go project won't get React rules. Override any rule via `~/.devflow/rules/{name}.md`. +**Always-on rules.** 12 ultra-condensed engineering principles (~10 lines each) load on every prompt — security, quality, and language-specific guidance (TypeScript, React, Go, Python, Java, Rust). Rules install from your selected plugins only, so a Go project won't get React rules. Override any rule via `~/.devflow/rules/{name}.md`. **Full lifecycle.** `/plan` takes a feature idea through codebase exploration, gap analysis, design review, and outputs a plan document ready for `/implement`. `/implement` accepts that plan document (or an issue or task description directly) and drives it through coding, validation, and refinement to a PR. `/debug` investigates bugs with competing hypotheses in parallel. `/self-review` runs Simplifier + Scrutinizer quality passes. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 78df72c9..f1b35b67 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -56,7 +56,7 @@ npx devflow-kit init --plugin=implement,code-review # Install multiple | `devflow-release` | Core | Adaptive release with learned configuration | | `devflow-self-review` | Core | Simplifier + Scrutinizer | | `devflow-bug-analysis` | Core | Proactive bug finding with static and semantic analysis | -| `devflow-ambient` | Core | Ambient mode (plan auto-detection and command awareness) | +| `devflow-ambient` | Core | Ambient mode (plan auto-detection) | | `devflow-core-skills` | Core | Auto-activating quality skills | | `devflow-audit-claude` | Optional | CLAUDE.md file audit | | `devflow-typescript` | Language | TypeScript patterns | diff --git a/docs/commands.md b/docs/commands.md index 093b0376..e1617f1c 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -170,7 +170,6 @@ Incremental by default — only analyzes commits since the last run. Findings ar ## Ambient Mode -Not a command — a zero-overhead enhancement with two components: +Not a command — a zero-overhead enhancement: -1. **Plan auto-detection** — A `UserPromptSubmit` hook detects structured plans (containing `## Goal`, `## Steps`, and `## Files`) and auto-invokes `/implement`. Normal prompts produce zero output. -2. **Command awareness** — An always-on rule (`~/.claude/rules/devflow/commands.md`) lists available `/devflow:` commands for passive reference. +**Plan auto-detection** — A `UserPromptSubmit` hook detects structured plans (containing `## Goal`, `## Steps`, and `## Files`) and auto-invokes `/implement`. Normal prompts produce zero output. diff --git a/plugins/devflow-ambient/.claude-plugin/plugin.json b/plugins/devflow-ambient/.claude-plugin/plugin.json index dd02d262..82f04bc8 100644 --- a/plugins/devflow-ambient/.claude-plugin/plugin.json +++ b/plugins/devflow-ambient/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "devflow-ambient", - "description": "Plan auto-detection and command awareness", + "description": "Plan auto-detection", "author": { "name": "Dean0x" }, @@ -11,9 +11,7 @@ "keywords": [ "ambient", "plan", - "detection", - "commands", - "awareness" + "detection" ], "agents": [ "coder", diff --git a/plugins/devflow-ambient/README.md b/plugins/devflow-ambient/README.md index 12de4fe1..a0453027 100644 --- a/plugins/devflow-ambient/README.md +++ b/plugins/devflow-ambient/README.md @@ -1,19 +1,18 @@ # devflow-ambient -Ambient mode — plan auto-detection and command awareness. A `UserPromptSubmit` hook detects structured implementation plans and invokes the implement workflow. A commands rule provides passive command reference. +Ambient mode — plan auto-detection. A `UserPromptSubmit` hook detects structured implementation plans and invokes the implement workflow automatically. ## Activation ```bash -devflow ambient --enable # Register ambient mode hook and install commands rule -devflow ambient --disable # Remove hook and commands rule +devflow ambient --enable # Register ambient mode hook +devflow ambient --disable # Remove hook devflow ambient --status # Check if enabled ``` ## How It Works -1. **Plan detection** — When a prompt contains `## Goal`, `## Steps`, and `## Files` sections, the preamble hook outputs a directive to invoke `devflow:implement` via the Skill tool -2. **Command awareness** — The `~/.claude/rules/devflow/commands.md` rule lists all available `/devflow:` commands and documents the plan auto-execution trigger +**Plan detection** — When a prompt contains `## Goal`, `## Steps`, and `## Files` sections, the preamble hook outputs a directive to invoke `devflow:implement` via the Skill tool. Normal prompts produce zero overhead — the hook exits without output. diff --git a/shared/rules/commands.md b/shared/rules/commands.md deleted file mode 100644 index a8cd5814..00000000 --- a/shared/rules/commands.md +++ /dev/null @@ -1,26 +0,0 @@ ---- -name: commands -description: Available devflow workflow commands and plan auto-execution -paths: [] ---- - -# Devflow Workflow Commands - -Use `/devflow:` to trigger a workflow: - -- `plan` — Design implementation plans with gap analysis and design review -- `implement` — Execute tasks through implementation, quality gates, and PR creation -- `code-review` — Branch review with specialized parallel reviewers -- `resolve` — Process review/analysis issues — validate, fix, or defer -- `debug` — Competing hypothesis investigation with parallel agents -- `explore` — Codebase exploration with structured analysis -- `research` — Multi-type research with trust-aware synthesis -- `release` — Adaptive release with learned configuration -- `self-review` — Simplifier (code clarity) then Scrutinizer (9-pillar quality) -- `bug-analysis` — Proactive bug finding in changed code - -## Plan Auto-Execution - -When a prompt is a structured implementation plan (contains `## Goal`, `## Steps`, -and `## Files` sections), this is a plan handoff from a prior planning session. -Invoke `devflow:implement` via the Skill tool to execute it. diff --git a/src/cli/commands/ambient.ts b/src/cli/commands/ambient.ts index b6f41e89..6613d2b6 100644 --- a/src/cli/commands/ambient.ts +++ b/src/cli/commands/ambient.ts @@ -13,44 +13,13 @@ const LEGACY_HOOK_MARKER = 'ambient-prompt'; const CLASSIFICATION_HOOK_MARKER = 'session-start-classification'; /** - * Path where the commands rule is installed. - * Managed by ambient.ts directly — NOT by the rules plugin system. + * Path where the legacy commands rule was installed. + * The commands rule was removed — this path now exists only to purge the + * legacy file from prior installs. Managed by ambient.ts directly (not the + * plugin rules system), so only ambient enable/disable/init paths clean it up. */ export const COMMANDS_RULE_PATH = path.join(os.homedir(), '.claude', 'rules', 'devflow', 'commands.md'); -/** - * D1: Content of the commands awareness rule. - * Installed to COMMANDS_RULE_PATH when ambient mode is enabled. - * The `paths: []` frontmatter signals Claude Code to apply this rule globally. - */ -export const COMMANDS_RULE_CONTENT = `--- -name: commands -description: Available devflow workflow commands and plan auto-execution -paths: [] ---- - -# Devflow Workflow Commands - -Use \`/devflow:\` to trigger a workflow: - -- \`plan\` — Design implementation plans with gap analysis and design review -- \`implement\` — Execute tasks through implementation, quality gates, and PR creation -- \`code-review\` — Branch review with specialized parallel reviewers -- \`resolve\` — Process review/analysis issues — validate, fix, or defer -- \`debug\` — Competing hypothesis investigation with parallel agents -- \`explore\` — Codebase exploration with structured analysis -- \`research\` — Multi-type research with trust-aware synthesis -- \`release\` — Adaptive release with learned configuration -- \`self-review\` — Simplifier (code clarity) then Scrutinizer (9-pillar quality) -- \`bug-analysis\` — Proactive bug finding in changed code - -## Plan Auto-Execution - -When a prompt is a structured implementation plan (contains \`## Goal\`, \`## Steps\`, -and \`## Files\` sections), this is a plan handoff from a prior planning session. -Invoke \`devflow:implement\` via the Skill tool to execute it. -`; - /** Filter hook entries from a parsed Settings object for a given event. Returns true if any were removed. */ function filterHookEntries( settings: Settings, @@ -87,32 +56,28 @@ const isClassification = (matcher: HookMatcher) => matcher.hooks.some((h) => h.command.includes(CLASSIFICATION_HOOK_MARKER)); /** - * Install the commands awareness rule file. - * Idempotent — always overwrites with current content. - */ -export async function installCommandsRule(): Promise { - await fs.mkdir(path.dirname(COMMANDS_RULE_PATH), { recursive: true }); - await fs.writeFile(COMMANDS_RULE_PATH, COMMANDS_RULE_CONTENT, 'utf-8'); -} - -/** - * Remove the commands awareness rule file. + * Remove the legacy commands awareness rule file left by prior installs. * Idempotent — no-op if the file does not exist. - * Only swallows ENOENT; other errors (e.g. EACCES) propagate. + * Fail-safe: swallows ALL errors (ENOENT, EACCES, EPERM, EROFS, etc.). + * This is best-effort cleanup of a deprecated file; it must never abort + * the primary operation (hook write or settings.json update) that calls it. */ -export async function removeCommandsRule(): Promise { +export async function removeLegacyCommandsRule(): Promise { try { await fs.unlink(COMMANDS_RULE_PATH); - } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + } catch { + // Intentionally swallow all errors — cleanup is best-effort. + // ENOENT = already gone (idempotent); EACCES/EPERM/EROFS = unwritable + // filesystem. Neither should abort the caller's primary operation. } } /** - * Add the ambient UserPromptSubmit hook and write the commands awareness rule. + * Add the ambient UserPromptSubmit hook and remove any legacy commands rule. * Removes any legacy `ambient-prompt` hook first, then adds the new `preamble` hook. - * Writes COMMANDS_RULE_CONTENT to COMMANDS_RULE_PATH for passive command awareness. - * Idempotent — hook checked before adding, rule always overwritten. + * Removes any legacy commands rule left by prior installs. + * Idempotent — hook checked before adding; legacy rule purge runs unconditionally + * (before the early-return) to ensure stale files are always cleaned up. */ export async function addAmbientHook(settingsJson: string, devflowDir: string): Promise { const settings: Settings = JSON.parse(settingsJson); @@ -138,18 +103,18 @@ export async function addAmbientHook(settingsJson: string, devflowDir: string): changed = true; } - // --- Write commands awareness rule --- - await installCommandsRule(); + // --- Purge legacy commands rule (runs before early-return so stale files are always removed) --- + await removeLegacyCommandsRule(); if (!changed) return settingsJson; return JSON.stringify(settings, null, 2) + '\n'; } /** - * Remove the ambient hooks from settings JSON and delete the commands rule. + * Remove the ambient hooks from settings JSON and purge any legacy commands rule. * Removes preamble + legacy from UserPromptSubmit. * Also removes stale SessionStart classification hook from previous installs. - * Deletes COMMANDS_RULE_PATH if present. + * Purges legacy COMMANDS_RULE_PATH if present (runs before early-return). * Idempotent — returns unchanged JSON if neither prompt hook nor stale classification was present. * Preserves other hooks. Cleans empty arrays/objects. */ @@ -159,8 +124,8 @@ export async function removeAmbientHook(settingsJson: string): Promise { // Clean up stale classification hooks from previous installs (no longer registered) const removedClassification = filterHookEntries(settings, 'SessionStart', isClassification); - // Delete commands rule if it exists - await removeCommandsRule(); + // Purge legacy commands rule (runs before early-return so stale files are always removed) + await removeLegacyCommandsRule(); if (!removedPrompt && !removedClassification) return settingsJson; return JSON.stringify(settings, null, 2) + '\n'; @@ -185,9 +150,9 @@ interface AmbientOptions { } export const ambientCommand = new Command('ambient') - .description('Enable or disable ambient mode (plan auto-detection and command awareness)') - .option('--enable', 'Register ambient mode hooks and install commands rule') - .option('--disable', 'Remove ambient mode hooks and uninstall commands rule') + .description('Enable or disable ambient mode (plan auto-detection)') + .option('--enable', 'Register ambient mode hook') + .option('--disable', 'Remove ambient mode hook') .option('--status', 'Check if ambient mode is enabled') .action(async (options: AmbientOptions) => { const hasFlag = options.enable || options.disable || options.status; @@ -249,13 +214,13 @@ export const ambientCommand = new Command('ambient') if (options.enable) { const updated = await addAmbientHook(settingsContent, devflowDir); if (updated === settingsContent) { - // Hook already exists but rule may still need writing — addAmbientHook writes it anyway + // Hook already exists — addAmbientHook purges any legacy rule anyway p.log.info('Ambient mode already enabled'); return; } await fs.writeFile(settingsPath, updated, 'utf-8'); p.log.success('Ambient mode enabled — plan detection hook registered'); - p.log.info(color.dim('Structured plans auto-execute; command listing available as a rule')); + p.log.info(color.dim('Structured plans auto-execute')); } if (options.disable) { diff --git a/src/cli/commands/init.ts b/src/cli/commands/init.ts index 8db92978..a999d987 100644 --- a/src/cli/commands/init.ts +++ b/src/cli/commands/init.ts @@ -173,7 +173,7 @@ export const initCommand = new Command('init') .option('--plugin ', 'Install specific plugin(s), comma-separated (e.g., implement,code-review)') .option('--teams', 'Enable Agent Teams (peer debate, adversarial review)') .option('--no-teams', 'Disable Agent Teams (use parallel subagents instead)') - .option('--ambient', 'Enable ambient mode (plan auto-detection and command awareness)') + .option('--ambient', 'Enable ambient mode (plan auto-detection)') .option('--no-ambient', 'Disable ambient mode') .option('--memory', 'Enable working memory (session context preservation)') .option('--no-memory', 'Disable working memory hooks') @@ -552,7 +552,7 @@ export const initCommand = new Command('init') ambientEnabled = options.ambient; } else { p.note( - 'Detects implementation plans and provides command awareness.\n' + + 'Detects implementation plans and auto-executes them.\n' + 'When the first message is a structured plan, automatically\n' + 'invokes the implement workflow to execute it.\n\n' + 'Zero overhead for normal prompts.', diff --git a/src/cli/plugins.ts b/src/cli/plugins.ts index ebf91d07..1309c34a 100644 --- a/src/cli/plugins.ts +++ b/src/cli/plugins.ts @@ -144,7 +144,7 @@ export const DEVFLOW_PLUGINS: PluginDefinition[] = [ }, { name: 'devflow-ambient', - description: 'Plan auto-detection and command awareness', + description: 'Plan auto-detection', commands: ['/ambient'], agents: ['coder', 'validator', 'simplifier', 'scrutinizer', 'evaluator', 'tester', 'skimmer', 'reviewer', 'git', 'synthesizer', 'resolver', 'designer', 'knowledge', 'researcher'], skills: [ diff --git a/tests/ambient.test.ts b/tests/ambient.test.ts index 23215bed..568a84f0 100644 --- a/tests/ambient.test.ts +++ b/tests/ambient.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { promises as fs } from 'fs'; import * as path from 'path'; -import { addAmbientHook, removeAmbientHook, hasAmbientHook, installCommandsRule, removeCommandsRule, COMMANDS_RULE_CONTENT, COMMANDS_RULE_PATH } from '../src/cli/commands/ambient.js'; +import { addAmbientHook, removeAmbientHook, hasAmbientHook, removeLegacyCommandsRule, COMMANDS_RULE_PATH } from '../src/cli/commands/ambient.js'; import type { StreamResult } from './integration/helpers.js'; import { hasSkillInvocations, @@ -15,10 +15,9 @@ function textResult(text: string, skills: string[] = []): StreamResult { describe('addAmbientHook', () => { beforeEach(() => { - // installCommandsRule() writes to COMMANDS_RULE_PATH — stub out the underlying - // fs operations so unit tests produce no real filesystem side-effects. - vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined); - vi.spyOn(fs, 'writeFile').mockResolvedValue(undefined); + // removeLegacyCommandsRule() unlinks COMMANDS_RULE_PATH — stub out the + // underlying fs operations so unit tests produce no real filesystem side-effects. + vi.spyOn(fs, 'unlink').mockResolvedValue(undefined); }); afterEach(() => { @@ -92,6 +91,17 @@ describe('addAmbientHook', () => { expect(second).toBe(first); }); + it('purges legacy rule even when preamble hook already present (ordering invariant)', async () => { + // When the hook is already registered addAmbientHook takes the early-return path. + // removeLegacyCommandsRule MUST still run so stale commands.md files are cleaned up. + const withHook = await addAmbientHook('{}', '/home/user/.devflow'); + vi.clearAllMocks(); + + await addAmbientHook(withHook, '/home/user/.devflow'); + + expect(fs.unlink).toHaveBeenCalledWith(COMMANDS_RULE_PATH); + }); + it('preserves other settings', async () => { const input = JSON.stringify({ statusLine: { type: 'command', command: 'statusline.sh' }, @@ -153,10 +163,7 @@ describe('addAmbientHook', () => { describe('removeAmbientHook', () => { beforeEach(() => { - // removeCommandsRule() unlinks COMMANDS_RULE_PATH; installCommandsRule() used by - // addAmbientHook in shared setup — stub all fs side-effects to keep tests pure. - vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined); - vi.spyOn(fs, 'writeFile').mockResolvedValue(undefined); + // removeLegacyCommandsRule() unlinks COMMANDS_RULE_PATH — stub fs side-effects to keep tests pure. vi.spyOn(fs, 'unlink').mockResolvedValue(undefined); }); @@ -246,6 +253,17 @@ describe('removeAmbientHook', () => { expect(result).toBe(input); }); + it('purges legacy rule even when nothing to remove (ordering invariant)', async () => { + // When neither a prompt hook nor a classification hook is present removeAmbientHook + // takes the early-return path. removeLegacyCommandsRule MUST still run so stale + // commands.md files are cleaned up regardless of the hook state. + const input = JSON.stringify({ hooks: { Stop: [{ hooks: [{ type: 'command', command: 'stop.sh' }] }] } }); + const result = await removeAmbientHook(input); + + expect(result).toBe(input); + expect(fs.unlink).toHaveBeenCalledWith(COMMANDS_RULE_PATH); + }); + it('preserves other settings', async () => { const input = JSON.stringify({ statusLine: { type: 'command' }, @@ -312,65 +330,34 @@ describe('removeAmbientHook', () => { }); }); -describe('installCommandsRule', () => { - beforeEach(() => { - vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined); - vi.spyOn(fs, 'writeFile').mockResolvedValue(undefined); - }); - - afterEach(() => { - vi.restoreAllMocks(); - }); - - it('creates the parent directory recursively', async () => { - await installCommandsRule(); - expect(fs.mkdir).toHaveBeenCalledWith( - expect.stringContaining('rules/devflow'), - { recursive: true }, - ); - }); - - it('writes COMMANDS_RULE_CONTENT to COMMANDS_RULE_PATH', async () => { - await installCommandsRule(); - expect(fs.writeFile).toHaveBeenCalledWith(COMMANDS_RULE_PATH, COMMANDS_RULE_CONTENT, 'utf-8'); - }); - - it('is idempotent — overwrites existing file without error', async () => { - await expect(installCommandsRule()).resolves.toBeUndefined(); - await expect(installCommandsRule()).resolves.toBeUndefined(); - expect(fs.writeFile).toHaveBeenCalledTimes(2); - }); -}); - -describe('removeCommandsRule', () => { +describe('removeLegacyCommandsRule', () => { afterEach(() => { vi.restoreAllMocks(); }); it('unlinks COMMANDS_RULE_PATH when file exists', async () => { vi.spyOn(fs, 'unlink').mockResolvedValue(undefined); - await removeCommandsRule(); + await removeLegacyCommandsRule(); expect(fs.unlink).toHaveBeenCalledWith(COMMANDS_RULE_PATH); }); it('swallows ENOENT — idempotent when file does not exist', async () => { const enoent = Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); vi.spyOn(fs, 'unlink').mockRejectedValue(enoent); - await expect(removeCommandsRule()).resolves.toBeUndefined(); + await expect(removeLegacyCommandsRule()).resolves.toBeUndefined(); }); - it('re-throws non-ENOENT errors — e.g. EACCES', async () => { + it('swallows non-ENOENT errors — e.g. EACCES (fail-safe cleanup)', async () => { const eacces = Object.assign(new Error('EACCES'), { code: 'EACCES' }); vi.spyOn(fs, 'unlink').mockRejectedValue(eacces); - await expect(removeCommandsRule()).rejects.toMatchObject({ code: 'EACCES' }); + await expect(removeLegacyCommandsRule()).resolves.toBeUndefined(); }); }); describe('hasAmbientHook', () => { beforeEach(() => { - // addAmbientHook (called in one test) triggers installCommandsRule — stub fs side-effects. - vi.spyOn(fs, 'mkdir').mockResolvedValue(undefined); - vi.spyOn(fs, 'writeFile').mockResolvedValue(undefined); + // addAmbientHook (called in one test) triggers removeLegacyCommandsRule — stub fs side-effects. + vi.spyOn(fs, 'unlink').mockResolvedValue(undefined); }); afterEach(() => { @@ -433,38 +420,6 @@ describe('hasAmbientHook', () => { }); }); -describe('COMMANDS_RULE_CONTENT', () => { - it('matches shared/rules/commands.md source file', async () => { - // Dual-source guard: COMMANDS_RULE_CONTENT in ambient.ts and shared/rules/commands.md - // must stay in sync. This test detects drift so either source can be updated confidently. - const sourceFile = path.resolve(__dirname, '../shared/rules/commands.md'); - const diskContent = await fs.readFile(sourceFile, 'utf-8'); - expect(COMMANDS_RULE_CONTENT).toBe(diskContent); - }); - - it('has paths: [] frontmatter for global application', () => { - expect(COMMANDS_RULE_CONTENT).toContain('paths: []'); - }); - - it('lists all 10 devflow commands', () => { - const commands = ['plan', 'implement', 'code-review', 'resolve', 'debug', 'explore', 'research', 'release', 'self-review', 'bug-analysis']; - for (const cmd of commands) { - expect(COMMANDS_RULE_CONTENT, `COMMANDS_RULE_CONTENT missing command: ${cmd}`).toContain(`\`${cmd}\``); - } - }); - - it('mentions plan auto-execution with the three required section markers', () => { - expect(COMMANDS_RULE_CONTENT).toContain('Plan Auto-Execution'); - expect(COMMANDS_RULE_CONTENT).toContain('## Goal'); - expect(COMMANDS_RULE_CONTENT).toContain('## Steps'); - expect(COMMANDS_RULE_CONTENT).toContain('## Files'); - }); - - it('instructs invoking devflow:implement skill for plan handoffs', () => { - expect(COMMANDS_RULE_CONTENT).toContain('devflow:implement'); - }); -}); - describe('parseStreamEvent', () => { it('extracts skills from assistant tool_use events', () => { const event = {