Skip to content

[Refactor] Remove no-explicit-any suppressions: batch 3 (integrations) - #2309

Draft
zoomote2[bot] wants to merge 1 commit into
mainfrom
refactor/no-explicit-any-batch-3-integrations
Draft

zoomote2[bot] wants to merge 1 commit into
mainfrom
refactor/no-explicit-any-batch-3-integrations

Conversation

@zoomote2

@zoomote2 zoomote2 Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

​Created by Roomote. Follow up by mentioning @roomote, in the web UI, or in Discord.

Related GitHub Issue

Part of the no-explicit-any suppression elimination campaign. This is batch 3. It follows #2281 (batch 1) and #2286 (batch 2). The campaign tracks the suppressions ledger in src/eslint-suppressions.json; this batch has no separate tracking issue.

Description

Batch 3 removes every no-explicit-any suppression in src/integrations. The ledger shrinks from 3,315 suppressions across 292 files to 2,884 suppressions across 269 files. That removes all 431 suppressed anys in the 23 integrations files, and the ledger entry for each file is gone.

The fixes are real type fixes. No as any, no @ts-ignore, and no moved suppressions. Assertions and runtime behavior stay unchanged. The main patterns:

  • Private members are accessed with bracket notation (diffViewProvider["relPath"]), per AGENTS.md.
  • Test doubles use the typed helpers in src/test-utils/vscode.ts, or full structural vscode.Tab / TabGroup doubles. Double assertions appear once per double at its construction site, with a comment that names the members the double provides.
  • Theme JSON in getTheme.ts now uses recursive JsonValue / ParsedTheme types instead of any.
  • execSync failure paths in the terminal stream helpers use an ExecException intersection instead of error: any.
  • The __TEST_ENV__ global uses a proper var declaration instead of (global as any).

Two production files change: src/integrations/theme/getTheme.ts (JSON types plus one documented theme conversion) and src/integrations/editor/DiffViewProvider.ts (typed rejection handler). All other changes are test files. No user-facing behavior changes.

Test Procedure

Validation ran after a rebase onto latest main (debd3c7). All gates pass:

  • Focused tests: all src/integrations suites pass. 564 test files: 564 passed, 23 skipped (platform-gated shell and PowerShell suites). Per-file runs: DiffViewProvider.spec 71/71, TerminalProfile.spec 57/57, TerminalProcess.spec 41/41, TerminalRegistry.spec 19/19, TerminalProcess.test 7/7, ExecaTerminalProcess.spec 19/19, OutputInterceptor 25/25, open-file 8/8, EditorUtils 8/8, InterpretExitCode 9/9, kimi-code oauth + export-markdown 35/35.
  • Lint: pnpm lint passes with zero warnings across all 12 workspace packages. Each touched file was pruned with eslint --prune-suppressions --max-warnings=0.
  • Type checks: pnpm check-types passes (root project plus 11 turbo tasks).
  • Full extension suite: pnpm --dir src test passes. 12,276 tests passed, 39 skipped (platform-gated).
  • Repo-wide pnpm test: 14/14 turbo tasks pass. The first full run had one transient failure in the extension suite that matches the known order-dependent extension.spec.ts flake. Two later full runs pass clean. Recorded, not chased.
  • Pre-commit hooks pass on the committed tree (staged-file ESLint at --max-warnings=0, Prettier, mutation judgement).

Guardrail scan since #2279: main added no new suppressions. Main added one direct vi.clearAllMocks() call in webview-ui/.../SettingsView.spec.tsx (#2271). The webview workspace has no reset helper, and the #2279 guardrail covers the extension workspace only, so this matches the current webview convention. Recorded here, not changed in this batch.

Pre-Submission Checklist

  • Issue Linked: Covered by the campaign; follows [Refactor] Remove no-explicit-any suppressions: batch 1 (utils) #2281 and [Refactor] Remove no-explicit-any suppressions: batch 2 (small directories) #2286 (no separate issue for this batch).
  • Scope: Changes cover one campaign batch: src/integrations suppressions only.
  • Self-Review: Full diff reviewed. No as any, no @ts-ignore, and no new suppressions. The ledger change removes exactly 23 files; no entry is added or raised.
  • Writing Style: Title and description follow Simplified Technical English.
  • Testing: All touched files keep their focused suites green; no test assertions changed.
  • Test Helpers: Doubles use src/test-utils/vscode.ts helpers, or single construction-site double assertions with comments naming the provided members.
  • Visual Snapshot: Not applicable. No UI change.
  • Documentation Impact: No documentation updates are required. No functionality changed.
  • Contribution Guidelines: Read and followed.

Visual Snapshots

Not applicable. This change is type-level only. No rendered UI changes.

Videos

Not applicable. No interaction or animation changes.

Documentation Updates

  • No documentation updates are required.
  • Yes, documentation updates are required.

No functionality changed, so the docs repository needs no companion PR.

Additional Notes

Ledger delta: 3,315 suppressions across 292 files → 2,884 suppressions across 269 files (−431 suppressions, −23 files). The next batch can pick up any remaining directory. The workspace baseline needs packages/build compiled and pnpm --dir src bundle run before __tests__/dist_assets.spec.ts can pass; the sandbox needed that step once.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one): 1
  • coderabbit-review-active

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

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.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@zoomote2

zoomote2 Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor Author

2 issues outstanding. See session

  • src/integrations/misc/__tests__/performance/processCarriageReturns.benchmark.ts:363 — dropping [lineLimit] makes the truncateOutput benchmark early-return the input unchanged, so it no longer measures truncation; the old variadic args were forwarded and the limit can be passed directly with the new signature.
  • src/integrations/editor/__tests__/DiffViewProvider.spec.ts:1528-1530 — duplicated identical if (!activeDiffEditor) guard left over from the refactor.

Reviewed 0f236c4

@zoomote2

zoomote2 Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Self-review of the full diff (24 files: 23 campaign files + the ledger). Summary of what I checked and where reviewers should look:

Verified clean

  • Zero as any, @ts-ignore, or new suppressions anywhere in the diff. The ledger change removes exactly the 23 integrations entries; no entry is added or raised (semantic diff against main, not just textual).
  • No test assertions changed. Doubles follow the AGENTS.md convention: typed helpers from src/test-utils/vscode.ts where they fit, otherwise one double assertion at the construction site with a comment naming the provided members (Task, DecorationController, WorkspaceEdit, vscode.Terminal, TerminalProcess, TerminalShellExecution doubles).
  • All 431 removed anys are replaced with precise types, generics, or unknown + narrowing. Focus areas: full structural vscode.Tab/TabGroup doubles that keep instanceof checks real (TerminalRegistry/TerminalProfile), ExecException intersections for execSync failures (streamUtils, ProcessExec.bash), and recursive JsonValue/ParsedTheme types for theme JSON.

Behavior-equivalent changes a reviewer should confirm are acceptable

  • DiffViewProvider.ts rejection handler: non-Error rejection reasons now render via String(err) instead of undefined in the message. Error rejections keep the exact same message.
  • getTheme.ts mergeJson: the object-merge branch now casts the recursion inputs to JsonObject. Values that reach that branch are plain objects in valid theme JSON; malformed-JSON edge cases (e.g. null first values) previously resolved through the function's own try/catch fallback and now resolve through the final else-assignment with the same result.
  • processCarriageReturns.benchmark.ts: dropped a dead [lineLimit] variadic argument (the closure already captured lineLimit); the generic signature now models the args it actually forwards.
  • Some test event payloads gained required members the real VS Code types carry (selections: [], kind: undefined, exit status narrowing guards). These were previously implicit through any.

Environment notes

  • Validation matrix in the PR body ran after rebasing onto debd3c72c. One transient extension-suite failure on the first repo-wide run matches the known order-dependent extension.spec.ts flake; two subsequent full runs were clean.
  • The sandbox needed packages/build compiled plus pnpm --dir src bundle once before __tests__/dist_assets.spec.ts could pass. That is a workspace baseline precondition, not a change from this PR.
  • The single new vi.clearAllMocks() on main lives in a webview spec and matches the webview workspace's convention (no reset helper exists there; the [Refactor] Add test-utils guardrails and convert remaining stragglers #2279 guardrail covers the extension workspace only). Left as is, recorded in the PR body.

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Review status

This PR was opened by an automated account. A human maintainer must verify the change intent, provenance, and validation before merging.

Current step: Mark the PR ready. Required CI must pass before CodeRabbit starts.

Review-state labels are managed by this workflow; do not edit them manually. community-approved is managed the same way — do not add or remove it manually. It signals a fresh community code approval for the current head as an advisory priority only; maintainer review is still required.


console.log("\n--- Function 3: truncateOutput ---")
runPerformanceTest("truncateOutput", truncateOutput, testData, iterations, [lineLimit])
runPerformanceTest("truncateOutput", truncateOutput, testData, iterations)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drops the truncation limit, so the "Function 3: truncateOutput" benchmark no longer measures truncation. runPerformanceTest forwards its variadic args (fn(input, ...args)), so the old call ran truncateOutput(testData, 500). With no limit, truncateOutput early-returns the input unchanged (src/integrations/misc/extract-text.ts:254-256, if (!lineLimit && !characterLimit) return content), turning the benchmark into a pass-through and invalidating its throughput numbers. The closure-captured lineLimit mentioned in the PR description belongs to the separate "Full Pipeline" benchmark below, not this call site.

The new generic signature can still carry the argument: pass it as a variadic arg instead.

Suggested change
runPerformanceTest("truncateOutput", truncateOutput, testData, iterations)
runPerformanceTest("truncateOutput", truncateOutput, testData, iterations, lineLimit)

Comment on lines +1528 to +1530
if (!activeDiffEditor) {
throw new Error("activeDiffEditor was not set")
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated guard: this identical if (!activeDiffEditor) check appears twice in a row (the earlier one at the top of the block already narrows the type). Looks like a leftover from the refactor — one of the two can be removed.

This branch has not been deployed

No deployments
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