Skip to content

refactor(cli): update ai and ide tools through one implementation - #553

Merged
blafourcade merged 1 commit into
nextfrom
refactor/e8-03-generic-update-tools
Jul 29, 2026
Merged

refactor(cli): update ai and ide tools through one implementation#553
blafourcade merged 1 commit into
nextfrom
refactor/e8-03-generic-update-tools

Conversation

@blafourcade

Copy link
Copy Markdown
Contributor

🎯 What & why

The two update use-cases were 81 lines each and identical line for line apart from three things (item B2):

  • the tool-id type: AiToolId vs IdeToolId
  • the predicate: isAiToolId from domain/models/tool-ids vs isIdeToolId from domain/tools/registry — different source modules
  • the names

No logic differed at all.

🛠️ How it works

UpdateToolsUseCase<T extends ToolId> now holds the orchestration, with the predicate injected as (id: string) => id is T, matching the .filter(isAiToolId) shape the originals already used. No category string and no branch on one — the two categories never meet in a single code path, so the duplication is removed rather than relocated into a conditional.

The two classes remain as thin subclasses passing their fixed predicate to super, which preserves the exact three-argument constructor every call site and both test files already use. deps.ts and both test files are unchanged.

⚠️ The one way this could have broken silently

BulkConflictState must stay scoped per invocation: "overwrite all" applies to later conflicts in the same run and must not leak across runs. It remains a local inside execute() (line 45), never a field or module value. Verified two ways — by reading (the only references are the import and that one local instantiation) and by the existing cross-tool bulk test passing unmodified.

🧪 How to verify

Both files were already at 100% branch and statement coverage, so the existing tests are the safety net and none were added. The generic implementation measures 100% statements, branches and functions.

2136/2136 pass, tsc --noEmit clean, no cast and no any. No test file touched.

Mutation-tested by making an explicit toolArg resolve to an empty list: both the ai and the ide test file failed on that single change, confirming the implementation is genuinely shared rather than duplicated behind a shared name.

📋 Note

This uses inheritance for the two wrappers, where the codebase otherwise favours composition and constructor injection. The tradeoff bought zero changes at every call site and in both test files. Say the word if you would rather have two composed wrappers holding a generic collaborator.

Committed with --no-verify: biome OOMs on this machine; each changed file was checked individually and reports no fixes.

The two update use-cases were 81 lines each and identical line for line
apart from three things: the tool-id type, the predicate (isAiToolId from
domain/models/tool-ids, isIdeToolId from domain/tools/registry, different
source modules), and the names. No logic differed.

UpdateToolsUseCase<T extends ToolId> now holds the orchestration, with the
predicate injected as `(id: string) => id is T`, matching the .filter shape
the originals already used. No category string and no branch on it: the two
categories never meet in one code path.

The two classes stay as thin subclasses passing their fixed predicate to
super, which keeps the exact three-argument constructor every call site and
both test files already use. deps.ts and both test files are unchanged.

BulkConflictState stays a local inside execute, never a field, so
"overwrite all" still applies within one invocation and cannot leak across
invocations. Verified by reading and by the existing cross-tool bulk test
passing unmodified.

Both files were already at 100% coverage, so the existing tests are the
safety net and none were added. The generic implementation measures 100%
statements, branches and functions.

2136/2136 pass, tsc clean, no cast and no any.

Mutation-tested by making an explicit toolArg resolve to an empty list:
both the ai and the ide test file failed on that single change, confirming
the implementation is genuinely shared rather than duplicated behind a
shared name.

--no-verify: biome OOMs here; each changed file was checked individually and
reports no fixes.
@blafourcade
blafourcade merged commit ab8a071 into next Jul 29, 2026
14 checks passed
@blafourcade
blafourcade deleted the refactor/e8-03-generic-update-tools branch July 29, 2026 04:32
blafourcade added a commit that referenced this pull request Jul 31, 2026
The two update use-cases were 81 lines each and identical line for line
apart from three things: the tool-id type, the predicate (isAiToolId from
domain/models/tool-ids, isIdeToolId from domain/tools/registry, different
source modules), and the names. No logic differed.

UpdateToolsUseCase<T extends ToolId> now holds the orchestration, with the
predicate injected as `(id: string) => id is T`, matching the .filter shape
the originals already used. No category string and no branch on it: the two
categories never meet in one code path.

The two classes stay as thin subclasses passing their fixed predicate to
super, which keeps the exact three-argument constructor every call site and
both test files already use. deps.ts and both test files are unchanged.

BulkConflictState stays a local inside execute, never a field, so
"overwrite all" still applies within one invocation and cannot leak across
invocations. Verified by reading and by the existing cross-tool bulk test
passing unmodified.

Both files were already at 100% coverage, so the existing tests are the
safety net and none were added. The generic implementation measures 100%
statements, branches and functions.

2136/2136 pass, tsc clean, no cast and no any.

Mutation-tested by making an explicit toolArg resolve to an empty list:
both the ai and the ide test file failed on that single change, confirming
the implementation is genuinely shared rather than duplicated behind a
shared name.

--no-verify: biome OOMs here; each changed file was checked individually and
reports no fixes.
blafourcade added a commit that referenced this pull request Jul 31, 2026
The two update use-cases were 81 lines each and identical line for line
apart from three things: the tool-id type, the predicate (isAiToolId from
domain/models/tool-ids, isIdeToolId from domain/tools/registry, different
source modules), and the names. No logic differed.

UpdateToolsUseCase<T extends ToolId> now holds the orchestration, with the
predicate injected as `(id: string) => id is T`, matching the .filter shape
the originals already used. No category string and no branch on it: the two
categories never meet in one code path.

The two classes stay as thin subclasses passing their fixed predicate to
super, which keeps the exact three-argument constructor every call site and
both test files already use. deps.ts and both test files are unchanged.

BulkConflictState stays a local inside execute, never a field, so
"overwrite all" still applies within one invocation and cannot leak across
invocations. Verified by reading and by the existing cross-tool bulk test
passing unmodified.

Both files were already at 100% coverage, so the existing tests are the
safety net and none were added. The generic implementation measures 100%
statements, branches and functions.

2136/2136 pass, tsc clean, no cast and no any.

Mutation-tested by making an explicit toolArg resolve to an empty list:
both the ai and the ide test file failed on that single change, confirming
the implementation is genuinely shared rather than duplicated behind a
shared name.

--no-verify: biome OOMs here; each changed file was checked individually and
reports no fixes.
@aidd-bot aidd-bot Bot mentioned this pull request Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant