Repository navigation
chore(test): drop test-only tool-use interfaces from shared/tools - #2258
Conversation
What: remove the AttemptCompletionToolUse and WriteToFileToolUse interface declarations from src/shared/tools.ts and retype their three spec consumers to the generic ToolUse<"attempt_completion"> / ToolUse<"write_to_file">. Why: both interfaces had zero production references at origin/main; the only importers were three specs. ToolUse<TName> already resolves nativeArgs through NativeToolArgs, so the named aliases added no type information over the generic form. knip could not report them because knip.json exempts src/shared/*.ts with a "!" suffix, so the deadness was proven by importer grep instead. Impact: no behavior change. Type-level only: params widens from a Pick of one or two keys to Partial<Record<ToolParamName, string>>, which accepts a superset, and nativeArgs keeps its exact shape. src and webview-ui typecheck, knip dev and production stay clean, and the affected suites (1246 tests across core/tools and core/task) pass. The TOOL_DISPLAY_NAMES row from the same plan was already removed upstream by 1cbc0f4 (Zoo-Code-Org#527), and the directoryExists dedup was stopped because the two copies enforce different error contracts (the roo-config copy re-throws on EACCES and is test-locked; the custom-instructions copy swallows all errors).
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one): 1
Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Replace specialized tool types in tests src/shared/tools.ts, src/core/task/__tests__/Task.persistence.spec.ts, src/core/task/__tests__/truncated-native-tool-args.spec.ts, src/core/tools/__tests__/attemptCompletionTool.spec.ts |
The two specialized interfaces were removed. Completion and file-write test fixtures now use ToolUse with the corresponding tool name. Test scenarios and assertions are unchanged. |
Priority:
Estimated code review effort: 1 (Trivial) | ~4 minutes
Change: Other
Merge Risk: 
cc2cb
-
This type-only cleanup has no identified behavioral regression or merge-blocking issue. It is mergeable subject to normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Description check The description is detailed and explains the implementation, scope, testing, and known failures. It does not follow the required template. It does not include a Closes: #<issue>entry for an approve…Add the required template sections. Link an approved GitHub Issue with Closes: #<number>, complete the pre-submission checklist, state whether documentation updates are required, and add the contributor Discord username. Retain the existi…-
Check name Status Explanation Title check The title clearly and concisely describes the main change: removing test-only tool-use interfaces from shared/tools. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Regression Evidence PASS. The PR changes only TypeScript types and test imports. It removes two interfaces and replaces their fixtures with ToolUse<"attempt_completion">andToolUse<"write_to_file">.ToolUsestill …Security Boundaries No changed path meets a security failure condition. The PR only removes two TypeScript interfaces from src/shared/tools.tsand changes test annotations toToolUse<"attempt_completion">or `ToolUse…Persistence Integrity PASS — The pull request changes only test type annotations/imports and removes two interfaces from src/shared/tools.ts. It does not change a persistence operation, write path, await behavior, atomic…Lifecycle Resource Cleanup PASS. The pull request changes only TypeScript type annotations in three test files and removes two exported interfaces from src/shared/tools.ts. The diff does not add or alter listeners, watchers, …Full details: Regression Evidence
-
Explanation
PASS. The PR changes only TypeScript types and test imports. It removes two interfaces and replaces their fixtures with
ToolUse<"attempt_completion">andToolUse<"write_to_file">.ToolUsestill provides the samenativeArgsmappings for both tools, and the test assertions and runtime code are unchanged. The affected specs retain focused coverage for completion, persistence, and truncated native arguments. No UI behavior changed, so no Playwright snapshot is required.
Full details: Security Boundaries
-
Explanation
No changed path meets a security failure condition. The PR only removes two TypeScript interfaces from
src/shared/tools.tsand changes test annotations toToolUse<"attempt_completion">orToolUse<"write_to_file">. The diff contains no runtime logic, input execution, secret handling, PII flow, approval checks, or allowlist changes.
Full details: Persistence Integrity
-
Explanation
PASS — The pull request changes only test type annotations/imports and removes two interfaces from
src/shared/tools.ts. It does not change a persistence operation, write path, await behavior, atomicity, rollback, partial-failure handling, or default propagation. No concrete changed persistence path can trigger the stated failure condition.
Full details: Lifecycle Resource Cleanup
-
Explanation
PASS. The pull request changes only TypeScript type annotations in three test files and removes two exported interfaces from
src/shared/tools.ts. The diff does not add or alter listeners, watchers, providers, timers, tasks, disposal logic, cancellation logic, or restart logic. No changed lifecycle path can leak a resource or duplicate work.
-
Full details: Description check
-
Explanation
The description is detailed and explains the implementation, scope, testing, and known failures. It does not follow the required template. It does not include a
Closes: #<issue>entry for an approved issue or the required completed pre-submission checklist. It also omits the required documentation impact and contributor contact sections.Resolution
Add the required template sections. Link an approved GitHub Issue with
Closes: #<number>, complete the pre-submission checklist, state whether documentation updates are required, and add the contributor Discord username. Retain the existing implementation and verification details under the matching template headings.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: The required review sequence passed. Remaining merge requirements apply. Review-state labels are managed by this workflow; do not edit them manually. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
coderabbit-review-active
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-actions
Bot
added
awaiting-maintainer
What: empty retrigger commit, no content change. Why: theme-fixtures died in @vscode/test-electron 'Resolving version...' download (recorded infra signature; no fixture diff emitted). Same base is green on Zoo-Code-Org#2254/Zoo-Code-Org#2256/Zoo-Code-Org#2257 minutes apart; diff is type-declaration-only with no causal path. No admin rerun rights on the org repo; push-retrigger is the documented recovery. Impact: all CI legs re-run on identical trees (cc2cb36 content == this commit's tree).
github-actions
Bot
added
awaiting-maintainer
github-actions
Bot
added
the
community-approved
github-actions
Bot
removed
awaiting-maintainer
Summary
Removes two test-only interface declarations from
src/shared/tools.tsand retypes their spec consumers to the genericToolUse<TName>. This is the residual of the diet wave's PR-D2 plan, re-baselined against currentorigin/main(159c42e15).Net: 38 insertions / 52 deletions across 4 files (net −14 L).
Per-row evidence
Every row was re-proven against
origin/mainbefore any edit.WriteToFileToolUsesrc webview-ui/src apps packages scripts; onlytruncated-native-tool-args.spec.ts:24,63,79,103AttemptCompletionToolUseattemptCompletionTool.spec.ts:3+ 31 sites,Task.persistence.spec.ts:23,536. Theapps/clihit is a local functionlooksLikeAttemptCompletionToolUse, not the type.TOOL_DISPLAY_NAMESgit grep -c TOOL_DISPLAY_NAMES origin/main→ exit 1, zero hits. Removed by1cbc0f4d6(#527), which cut 122 L fromtools.ts.directoryExistsdedupWhy knip did not catch the two interfaces
Corrected mechanism (per review seat, verified against knip 5.88.1 source): the
!suffix onknip.json:49does NOT exempt files from unused-export reporting (default mode strips it before globbing; production mode keeps it as a project pattern). knip stayed quiet for two other reasons:AttemptCompletionToolUsecarried a@publicJSDoc tag (knip's should-ignore handler suppresses tagged exports in all modes), andWriteToFileToolUsewas referenced only from test files (graph members in default mode,@internal-suppressed in production). Net effect unchanged: knip could not have caught this row, and deadness was proven by importer grep.Why the
directoryExistsrow was stoppedThe two copies enforce different error contracts:
EACCESsrc/core/prompts/sections/custom-instructions.ts:35falsesrc/services/roo-config/index.ts:111Empirically confirmed by replicating both bodies against a simulated
EACCES:This is observable, not theoretical:
src/services/roo-config/__tests__/index.spec.ts:127-132(rejects.toThrow("Permission denied")).loadRuleFilescall site (custom-instructions.ts:485) has no try/catch, and it propagates throughaddCustomInstructions→Task.getSystemPrompt. So a permission-denied.roo/rulesdirectory silently skips rules under A but would abort prompt construction under B.SkillsManager.ts:182-190classifies viaisNotFoundError(error)and recordspass.failedPathsfor anything else.Unifying them is a behavior change requiring a product decision, not a dedup. Kept open; recommended follow-up is a separate PR that picks the strict contract and adds an explicit tolerant wrapper at the prompt-assembly call site.
Type-safety note
The rewiring is type-identical, not a widening of what the tests exercise.
ToolUse<"write_to_file">resolvesnativeArgstoNativeToolArgs["write_to_file"]={ path: string; content: string }(src/shared/tools.ts:108-131), exactly what the deleted interface inherited; same forattempt_completion→{ result: string }.paramswidens from aPickof one or two keys toPartial<Record<ToolParamName, string>>, which accepts a superset.Verification
pnpm --dir src run check-typespnpm --dir webview-ui run check-typespnpm --dir packages/types run buildpnpm knip/pnpm knip:productionvitest run core/tools/__tests__/ core/task/__tests__/vitest run(src)src/eslint-suppressions.jsonPre-existing failures (verified on clean base)
Both failing specs reproduce identically with all changes stashed:
api/providers/__tests__/mistral.spec.tsAbortErrorvsTimeoutErrorat:9321 failed | 49 passed__tests__/dist_assets.spec.tsCombined base run:
2 failed (2) | 41 failed | 49 passed (90)— matches the full-suite failure counts exactly.Out of scope
The original PR-D2 plan is much smaller than written, per
docs/261031_0001_session_caching-diet-wave/research-report-diet-rebaseline.md:SearchAndReplaceTool.ts(2 L re-export shim) + its 15 L spec1cbc0f4d6, #527)origin/main. The liveSearchReplaceTool.tsis a different file (presentAssistantMessage.ts:26) and was not touched.TOOL_DISPLAY_NAMESrow entries1cbc0f4d6)directoryExistsdedupAlso untouched per hard exclusions:
WorkspacePathResolver.ts,versionToken.ts(owner-flagged, knip-exempt), the ChatRow placeholder, ui-wrappers, the i18n sweep, andpackages/types/src/__tests__/provider-settings.test.ts.#1735 independence
No overlap. This PR touches only
src/shared/tools.tsand threesrc/**/__tests__/*.spec.tsfiles;provider-settings.test.tsis untouched. The branchfix/delete-profile-purge-configis not present on the remote, and no merge commit referencing #1735 exists onorigin/main. No rebase conflict is possible from this side.Hygiene
No changeset, no CHANGELOG entry (per
AGENTS.md). No zero-byte files, no conflict markers. Index and worktree line endings are LF for all four files (git ls-files --eol).