diff --git a/cli/aidd_docs/tasks/2026_07/2026_07_29_e8-03-generic-update-tools/plan.md b/cli/aidd_docs/tasks/2026_07/2026_07_29_e8-03-generic-update-tools/plan.md new file mode 100644 index 00000000..95d2452e --- /dev/null +++ b/cli/aidd_docs/tasks/2026_07/2026_07_29_e8-03-generic-update-tools/plan.md @@ -0,0 +1,116 @@ +--- +objective: Collapse UpdateAiToolsUseCase and UpdateIdeToolsUseCase, two line-for-line identical fan-out use-cases, into one generic implementation parameterized by tool-ID type and predicate. +status: implemented +--- + +# Generic update-tools fan-out + +## Background + +`update-ai-tools-use-case.ts` and `update-ide-tools-use-case.ts` (both 81 lines) were diffed +line by line before writing anything. They were identical in every respect except three: + +1. The ID type: `AiToolId` vs `IdeToolId`. +2. The predicate used to filter installed tool IDs when no explicit `toolArg` is given: + `isAiToolId`, imported from `domain/models/tool-ids.js`, vs `isIdeToolId`, imported from + `domain/tools/registry.js` — different source modules, not just different names. +3. The exported names: `UpdateAiToolsInput`/`Result`/`UseCase` vs the Ide equivalents. + +Everything else — the `execute()` body, `resolveTargetIds`, `updateTargets`, the +`BulkConflictState` construction, the constructor injection order — was byte-identical. + +## What was extracted + +A new generic class `UpdateToolsUseCase` in +`src/application/use-cases/global/update-tools-use-case.ts` holds the entire orchestration. +It takes the predicate as a fourth constructor argument: + +```ts +constructor( + private readonly manifestRepo: ManifestRepository, + private readonly versionReader: VersionReader, + private readonly updateOneToolUseCase: UpdateOneToolUseCase, + private readonly isTargetToolId: (id: string) => id is T +) {} +``` + +`resolveTargetIds` calls `manifest.getInstalledToolIds().filter(this.isTargetToolId)` — +exactly the call shape the two originals already used (`.filter(isAiToolId)` / +`.filter(isIdeToolId)|`), just with the predicate coming from a field instead of a +module-level import. No `category: "ai" | "ide"` string exists anywhere in the class; there +is nothing to branch on. + +`update-ai-tools-use-case.ts` and `update-ide-tools-use-case.ts` were kept, but reduced to +thin subclasses: + +```ts +export class UpdateAiToolsUseCase extends UpdateToolsUseCase { + constructor(manifestRepo, versionReader, updateOneToolUseCase) { + super(manifestRepo, versionReader, updateOneToolUseCase, isAiToolId); + } +} +``` + +Each file also re-exports `UpdateAiToolsInput`/`Result` (and the Ide equivalents) as type +aliases over the generic `UpdateToolsInput`/`UpdateToolsResult`, so nothing downstream +needs to know the generic type exists. + +## Decisions + +| Decision | Why | +|---|---| +| Generic base class + subclassing, not a `category` field | The story explicitly forbids branching control flow on a category string. Subclassing fixes the type parameter and predicate at construction, so there is no runtime branch at all — the two categories never meet inside one code path. | +| Predicate injected via constructor, not resolved from a lookup table keyed by category | Matches the existing call shape (`.filter(isAiToolId)`) exactly and keeps the generic class free of any knowledge that "ai" and "ide" exist. | +| Old classes kept as subclasses rather than deleted | Every construction site (`deps.ts`) and every test file constructs `new UpdateAiToolsUseCase(manifestRepo, versionReader, updateOneToolUseCase)` with exactly 3 arguments. Subclassing preserves that exact constructor shape, so `deps.ts` and both existing test files needed zero changes — no wiring, no test edits, no risk of touching an assertion. | +| Generic file placed in `global/` next to its two subclasses, not in `shared/` | Precedent in this codebase: `InstallContentSectionUseCase` (a comparable generic engine used by 4 sibling use-cases) lives in `install/` alongside its callers, not in `shared/`. `shared/` in this codebase is reserved for logic reused across *different* top-level directories (e.g. `update-one-tool-use-case.ts`, `resolve-update-decision-use-case.ts`). The generic update-tools engine is only ever used by its two `global/` subclasses, so it stays in `global/`. | +| No new tests written | Both originals were already at 100% branch/statement coverage; the existing test suites for `UpdateAiToolsUseCase` and `UpdateIdeToolsUseCase` exercise the generic class transitively through the subclasses. Coverage was re-measured after the refactor (see Verification) to confirm the claim rather than assume it. | + +## BulkConflictState scoping — checked, not assumed + +The story's hard requirement: `BulkConflictState` must stay scoped per invocation of +`execute()`, never hoisted to a field, a module-level singleton, or anything shared between +calls — otherwise an "overwrite all" choice from one `update` command run could leak into a +later, unrelated run. + +In the generic class, `BulkConflictState` is instantiated with `const bulkState = new +BulkConflictState();` as a local variable inside `execute()` (line 45 of +`update-tools-use-case.ts`), never as a class field and never at module scope. This is +identical to where it was constructed in both originals (line 38 of each old file). A grep +of the new file for `BulkConflictState` confirms the only two references are the import and +this one local instantiation inside `execute()`. + +This was also verified empirically, not just by reading the code: the existing test +`"resolveConflictBulk called once for overwrite-all then not again for second tool"` in +`update-ai-tools-use-case.unit.test.ts` runs two tools (`claude`, `cursor`) through a single +`execute()` call with a mocked prompter, and asserts `resolveConflictBulkMock` is called +exactly once — proving the bulk decision persists *within* one invocation across the tools +it updates. This test passed unmodified against the generic implementation. Nothing in this +refactor introduces a second invocation observing state from a first, because the state +lives in a local variable that goes out of scope when `execute()` returns. + +## Verification + +1. `npx tsc --noEmit` — `TypeScript: No errors found`. +2. `pnpm test` from `cli/` — 197 test files, 2136 tests, all passing, 0 failures. Same count + as the pre-refactor baseline. No test file was modified. +3. Biome, one file per invocation via `./node_modules/.bin/biome check --write `: + - `update-tools-use-case.ts` — "Checked 1 file in 29ms. No fixes applied." + - `update-ai-tools-use-case.ts` — "Checked 1 file in 2ms. No fixes applied." + - `update-ide-tools-use-case.ts` — "Checked 1 file in 2ms. No fixes applied." +4. Mutation test: changed `resolveTargetIds` in the generic class so that an explicit + `toolArg` resolved to `[]` instead of `[toolArg]`. Ran + `npx vitest run --project=unit tests/application/use-cases/global/update-ai-tools-use-case.unit.test.ts tests/application/use-cases/global/update-ide-tools-use-case.unit.test.ts`. + Result: 2 failures out of 7 tests — + `UpdateAiToolsUseCase > single toolArg > updates only the specified tool when toolArg is provided` + in `update-ai-tools-use-case.unit.test.ts`, and + `UpdateIdeToolsUseCase > single toolArg > updates only the specified tool when toolArg is provided` + in `update-ide-tools-use-case.unit.test.ts`. Both the ai-side and the ide-side test file + failed on the same single-line mutation, which is the expected signature of a genuinely + shared implementation — a bug in the generic class cannot be contained to one category. + Reverted the mutation; `pnpm test` re-run afterward: 197 files, 2136 tests, all passing + again. +5. Coverage of the generic implementation, measured with + `npx vitest run --project=unit --project=integration --coverage --coverage.include='src/application/use-cases/global/update-tools-use-case.ts' --coverage.reporter=json-summary --coverage.reportsDirectory=/tmp/cov-upd`: + `update-tools-use-case.ts` — 100% statements (56/56), 100% branches (9/9), 100% functions + (4/4), 100% lines (56/56). The two subclass wrapper files were also measured separately + and are each 100% statements/branches/functions/lines. diff --git a/cli/src/application/use-cases/global/update-ai-tools-use-case.ts b/cli/src/application/use-cases/global/update-ai-tools-use-case.ts index 95365709..852c3675 100644 --- a/cli/src/application/use-cases/global/update-ai-tools-use-case.ts +++ b/cli/src/application/use-cases/global/update-ai-tools-use-case.ts @@ -1,81 +1,20 @@ -import { Manifest } from "../../../domain/models/manifest.js"; import type { AiToolId } from "../../../domain/models/tool-ids.js"; import { isAiToolId } from "../../../domain/models/tool-ids.js"; import type { ManifestRepository } from "../../../domain/ports/manifest-repository.js"; import type { VersionReader } from "../../../domain/ports/version-reader.js"; -import type { ToolId } from "../../../domain/tools/registry.js"; -import { BulkConflictState } from "../shared/resolve-update-decision-use-case.js"; -import type { - GlobalExecutionError, - UpdateOneToolUseCase, -} from "../shared/update-one-tool-use-case.js"; +import type { UpdateOneToolUseCase } from "../shared/update-one-tool-use-case.js"; +import type { UpdateToolsInput, UpdateToolsResult } from "./update-tools-use-case.js"; +import { UpdateToolsUseCase } from "./update-tools-use-case.js"; -export interface UpdateAiToolsInput { - toolArg?: AiToolId; - projectRoot: string; - userForce: boolean; - interactive: boolean; -} - -export interface UpdateAiToolsResult { - updatedTools: { toolId: ToolId; fileCount: number }[]; - errors: GlobalExecutionError[]; -} +export type UpdateAiToolsInput = UpdateToolsInput; +export type UpdateAiToolsResult = UpdateToolsResult; -export class UpdateAiToolsUseCase { +export class UpdateAiToolsUseCase extends UpdateToolsUseCase { constructor( - private readonly manifestRepo: ManifestRepository, - private readonly versionReader: VersionReader, - private readonly updateOneToolUseCase: UpdateOneToolUseCase - ) {} - - async execute(input: UpdateAiToolsInput): Promise { - const { toolArg, projectRoot, userForce, interactive } = input; - const manifest = (await this.manifestRepo.load()) ?? Manifest.create(); - const targetIds = this.resolveTargetIds(manifest, toolArg); - const version = this.versionReader.get(); - const errors: GlobalExecutionError[] = []; - const bulkState = new BulkConflictState(); - const updatedTools = await this.updateTargets( - targetIds, - manifest, - projectRoot, - version, - errors, - { - userForce, - interactive, - bulkState, - } - ); - return { updatedTools, errors }; - } - - private resolveTargetIds(manifest: Manifest, toolArg: AiToolId | undefined): AiToolId[] { - if (toolArg !== undefined) return [toolArg]; - return manifest.getInstalledToolIds().filter(isAiToolId); - } - - private async updateTargets( - targetIds: AiToolId[], - manifest: Manifest, - projectRoot: string, - version: string, - errors: GlobalExecutionError[], - options: { userForce: boolean; interactive: boolean; bulkState: BulkConflictState } - ): Promise<{ toolId: ToolId; fileCount: number }[]> { - const updated: { toolId: ToolId; fileCount: number }[] = []; - for (const toolId of targetIds) { - const entry = await this.updateOneToolUseCase.execute( - toolId, - manifest, - projectRoot, - version, - errors, - options - ); - if (entry) updated.push(entry); - } - return updated; + manifestRepo: ManifestRepository, + versionReader: VersionReader, + updateOneToolUseCase: UpdateOneToolUseCase + ) { + super(manifestRepo, versionReader, updateOneToolUseCase, isAiToolId); } } diff --git a/cli/src/application/use-cases/global/update-ide-tools-use-case.ts b/cli/src/application/use-cases/global/update-ide-tools-use-case.ts index 5aef2671..495558d8 100644 --- a/cli/src/application/use-cases/global/update-ide-tools-use-case.ts +++ b/cli/src/application/use-cases/global/update-ide-tools-use-case.ts @@ -1,81 +1,20 @@ -import { Manifest } from "../../../domain/models/manifest.js"; import type { IdeToolId } from "../../../domain/models/tool-ids.js"; import type { ManifestRepository } from "../../../domain/ports/manifest-repository.js"; import type { VersionReader } from "../../../domain/ports/version-reader.js"; -import type { ToolId } from "../../../domain/tools/registry.js"; import { isIdeToolId } from "../../../domain/tools/registry.js"; -import { BulkConflictState } from "../shared/resolve-update-decision-use-case.js"; -import type { - GlobalExecutionError, - UpdateOneToolUseCase, -} from "../shared/update-one-tool-use-case.js"; +import type { UpdateOneToolUseCase } from "../shared/update-one-tool-use-case.js"; +import type { UpdateToolsInput, UpdateToolsResult } from "./update-tools-use-case.js"; +import { UpdateToolsUseCase } from "./update-tools-use-case.js"; -export interface UpdateIdeToolsInput { - toolArg?: IdeToolId; - projectRoot: string; - userForce: boolean; - interactive: boolean; -} - -export interface UpdateIdeToolsResult { - updatedTools: { toolId: ToolId; fileCount: number }[]; - errors: GlobalExecutionError[]; -} +export type UpdateIdeToolsInput = UpdateToolsInput; +export type UpdateIdeToolsResult = UpdateToolsResult; -export class UpdateIdeToolsUseCase { +export class UpdateIdeToolsUseCase extends UpdateToolsUseCase { constructor( - private readonly manifestRepo: ManifestRepository, - private readonly versionReader: VersionReader, - private readonly updateOneToolUseCase: UpdateOneToolUseCase - ) {} - - async execute(input: UpdateIdeToolsInput): Promise { - const { toolArg, projectRoot, userForce, interactive } = input; - const manifest = (await this.manifestRepo.load()) ?? Manifest.create(); - const targetIds = this.resolveTargetIds(manifest, toolArg); - const version = this.versionReader.get(); - const errors: GlobalExecutionError[] = []; - const bulkState = new BulkConflictState(); - const updatedTools = await this.updateTargets( - targetIds, - manifest, - projectRoot, - version, - errors, - { - userForce, - interactive, - bulkState, - } - ); - return { updatedTools, errors }; - } - - private resolveTargetIds(manifest: Manifest, toolArg: IdeToolId | undefined): IdeToolId[] { - if (toolArg !== undefined) return [toolArg]; - return manifest.getInstalledToolIds().filter(isIdeToolId); - } - - private async updateTargets( - targetIds: IdeToolId[], - manifest: Manifest, - projectRoot: string, - version: string, - errors: GlobalExecutionError[], - options: { userForce: boolean; interactive: boolean; bulkState: BulkConflictState } - ): Promise<{ toolId: ToolId; fileCount: number }[]> { - const updated: { toolId: ToolId; fileCount: number }[] = []; - for (const toolId of targetIds) { - const entry = await this.updateOneToolUseCase.execute( - toolId, - manifest, - projectRoot, - version, - errors, - options - ); - if (entry) updated.push(entry); - } - return updated; + manifestRepo: ManifestRepository, + versionReader: VersionReader, + updateOneToolUseCase: UpdateOneToolUseCase + ) { + super(manifestRepo, versionReader, updateOneToolUseCase, isIdeToolId); } } diff --git a/cli/src/application/use-cases/global/update-tools-use-case.ts b/cli/src/application/use-cases/global/update-tools-use-case.ts new file mode 100644 index 00000000..4f2c1113 --- /dev/null +++ b/cli/src/application/use-cases/global/update-tools-use-case.ts @@ -0,0 +1,88 @@ +import { Manifest } from "../../../domain/models/manifest.js"; +import type { ManifestRepository } from "../../../domain/ports/manifest-repository.js"; +import type { VersionReader } from "../../../domain/ports/version-reader.js"; +import type { ToolId } from "../../../domain/tools/registry.js"; +import { BulkConflictState } from "../shared/resolve-update-decision-use-case.js"; +import type { + GlobalExecutionError, + UpdateOneToolUseCase, +} from "../shared/update-one-tool-use-case.js"; + +export interface UpdateToolsInput { + toolArg?: T; + projectRoot: string; + userForce: boolean; + interactive: boolean; +} + +export interface UpdateToolsResult { + updatedTools: { toolId: ToolId; fileCount: number }[]; + errors: GlobalExecutionError[]; +} + +/** + * Fans out an update across every installed tool of one category (AI or IDE). + * The category is fixed by the `isTargetToolId` predicate injected at construction, + * so subclasses only need to supply that predicate — the orchestration is identical + * for both categories. + */ +export class UpdateToolsUseCase { + constructor( + private readonly manifestRepo: ManifestRepository, + private readonly versionReader: VersionReader, + private readonly updateOneToolUseCase: UpdateOneToolUseCase, + private readonly isTargetToolId: (id: string) => id is T + ) {} + + async execute(input: UpdateToolsInput): Promise { + const { toolArg, projectRoot, userForce, interactive } = input; + const manifest = (await this.manifestRepo.load()) ?? Manifest.create(); + const targetIds = this.resolveTargetIds(manifest, toolArg); + const version = this.versionReader.get(); + const errors: GlobalExecutionError[] = []; + // Scoped to this invocation only: a fresh instance per execute() call, never a field, + // so an "overwrite all" choice cannot leak into a later, unrelated update run. + const bulkState = new BulkConflictState(); + const updatedTools = await this.updateTargets( + targetIds, + manifest, + projectRoot, + version, + errors, + { + userForce, + interactive, + bulkState, + } + ); + return { updatedTools, errors }; + } + + private resolveTargetIds(manifest: Manifest, toolArg: T | undefined): T[] { + if (toolArg !== undefined) return [toolArg]; + return manifest.getInstalledToolIds().filter(this.isTargetToolId); + } + + private async updateTargets( + targetIds: T[], + manifest: Manifest, + projectRoot: string, + version: string, + errors: GlobalExecutionError[], + options: { userForce: boolean; interactive: boolean; bulkState: BulkConflictState } + ): Promise<{ toolId: ToolId; fileCount: number }[]> { + const updated: { toolId: ToolId; fileCount: number }[] = []; + for (const toolId of targetIds) { + const entry = await this.updateOneToolUseCase.execute( + toolId, + manifest, + projectRoot, + version, + errors, + options + ); + if (entry) updated.push(entry); + } + return updated; + } +}