Repository navigation
test: drop private-internal and boundary-census tests with behavioral owners - #23941
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -108,18 +107,4 @@ describe('copyScriptWithLocalModules', () => { | |||
| copyScriptWithLocalModules(join(sourceDir, 'entry.mjs'), destinationDir) | |||
There was a problem hiding this comment.
node-pty-job-ownership.cjs script also copies windows-pe-machine.cjs, which that script requires. The remaining tests use synthetic modules, so they would not catch a change to the real gate’s dependencies that leaves the packaged Windows addon check unable to load. Please retain a package-level check for this entry script.
| @@ -28,21 +26,6 @@ describe('AutomationTimeField helpers', () => { | |||
| expect(formatAutomationTimeInput(0, 0)).toBe('00:00') | |||
There was a problem hiding this comment.
AutomationTimeField.
| @@ -216,27 +182,9 @@ describe('isCommandOnPathForRelay', () => { | |||
| }) | |||
|
|
|||
| describe('hasAbsoluteCommandPath', () => { | |||
There was a problem hiding this comment.
hasAbsoluteCommandPath accepts a where.exe-style Windows drive path. The remaining assertion covers only Linux output, while Windows uses a separate parsing branch. If that branch regresses, an installed agent on a Windows SSH host could be reported missing without this suite catching it. Please retain that input through the relay lookup entry point.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The batch's stated invariant — every deleted case has a behavioral owner — fails for one file. The deleted mobile/src/tasks/mobile-task-navigation.test.ts was the only coverage for two behaviors that survive in production. Details inline.
Reviewed changes
- Removes dead production seams —
buildCommandLookupSpec,appendGitHubPRRequestedReviewers,microphoneSelectValueFromDeviceId, the automation host-picker filter/search helpers, the mobile task/host-stack wrappers,activateOpenedMobileSessionTab/refreshOpenedMobileSessionTabs, and theCLAUDE_*control catalog all have zero remaining callers after the change. - De-exports internal-only helpers —
getAutomationClockParts,formatAutomationTimeFromClockParts,SoftSplitSection,softSplitPaletteSection,measureDictationAudioChunk,isMonacoFindHostFocused. - Deletes three test files —
mobile-task-navigation.test.ts,confirmation-dialog-refresh-boundary.test.ts, andterminal-pane-quick-command-initialization.test.ts. - Trims source-grep and duplicate cases across the remaining 15 modified test files.
ℹ️ Nitpicks
measureDictationAudioChunk's peak upper clamp (dictation-audio-meter.ts:56) loses its only direct assertion — every remaining input throughanalyzeDictationAudioChunkstays within[-0.3, 1], so theclamp(peak, 0, 1)bound is never hit.- The generated-plugin sha256 pins in
hook-service.test.tsare gone; thehook-plugin-*.test.tssuite still exercises runtime behavior, but nothing now detects an unintended change to the exact plugin bytes shipped to WSL/SSH remotes.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| it('unsubscribes when mounting the host throws synchronously', () => { | ||
| const harness = navigationHarness({ index: 0, routes: [{ name: 'index' }] }) | ||
| const error = new Error('navigation failed') | ||
|
|
||
| expect(() => | ||
| navigateToMobileTasks( | ||
| harness.navigation, | ||
| { | ||
| push: () => { | ||
| throw error | ||
| }, | ||
| replace: vi.fn() | ||
| }, | ||
| 'host-1' | ||
| ) | ||
| ).toThrow(error) | ||
| expect(harness.unsubscribeState).toHaveBeenCalledOnce() | ||
| }) |
There was a problem hiding this comment.
This deletion has no behavioral owner. unsubscribes when mounting the host throws synchronously was the only test exercising navigateToHostStackRoute's catch/dispose path — mobile/src/navigation/host-stack-navigation.ts:200-206, which this PR does not change — and host-stack-navigation.test.ts has no throwing-push case. The same file's provider assertions were also the only coverage of mobileTasksRouteTarget's provider argument (mobile/src/tasks/mobile-task-navigation.ts:4-11), whose sole caller is use-open-mobile-tasks.ts:11. Consider re-homing both cases on the surviving entry points rather than dropping them.
Technical details
# Two surviving behaviors lose their only test
## Affected sites
- `mobile/src/navigation/host-stack-navigation.ts:200-206` — `try { addListener(...); router.push(...) } catch { dispose(); throw }`; the dispose-on-sync-throw guarantee is now untested. A regression here leaks the `state` listener (and its captured transition) because `dispose()` is the only thing that unsubscribes when `push` throws before the returned controller is handed back.
- `mobile/src/tasks/mobile-task-navigation.ts:4-11` — `mobileTasksRouteTarget(hostId, provider)` sets `params.taskSource`. No test passes a `provider` now.
- Deleted owners: `mobile/src/tasks/mobile-task-navigation.test.ts:186-203` (throw case) and `:60-95` / `:114-157` (provider / retarget-with-latest-provider cases).
## Required outcome
- The sync-throw dispose path is covered again, at its real entry point (`navigateToHostStackRoute`).
- `mobileTasksRouteTarget` with a `provider` is covered (it is the function the app still calls).
## Suggested approach
- Add a throwing-`push` case to `mobile/src/navigation/host-stack-navigation.test.ts`, asserting the listener is unsubscribed and the error rethrown.
- Add a `mobileTasksRouteTarget('host-1', 'linear')` assertion (e.g. next to `use-open-mobile-tasks`), or keep the equivalent case from the deleted file.
## Open questions for the human
- Was `mobile-task-navigation.test.ts` intended to be re-homed rather than deleted, given `mobileTasksRouteTarget` still ships?| it('converts between 12h clock parts and 24h storage', () => { | ||
| expect(getAutomationClockParts('00:05')).toEqual({ hour12: 12, minute: 5, period: 'AM' }) | ||
| expect(getAutomationClockParts('13:30')).toEqual({ hour12: 1, minute: 30, period: 'PM' }) | ||
| expect(getAutomationClockParts('12:00')).toEqual({ hour12: 12, minute: 0, period: 'PM' }) | ||
| expect(formatAutomationTimeFromClockParts({ hour12: 12, minute: 5, period: 'AM' })).toBe( | ||
| '00:05' | ||
| ) | ||
| expect(formatAutomationTimeFromClockParts({ hour12: 1, minute: 30, period: 'PM' })).toBe( | ||
| '13:30' | ||
| ) | ||
| expect(formatAutomationTimeFromClockParts({ hour12: 12, minute: 0, period: 'PM' })).toBe( | ||
| '12:00' | ||
| ) | ||
| }) |
There was a problem hiding this comment.
The conversion helpers are still live (AutomationTimeField.tsx:181,186,194), but no remaining test covers the midnight/noon boundary this case pinned. The interaction tests use 13:15 (PM), 01:15 (AM), and 09:xx; none renders an hour of 0 or 12, so hour % 12 === 0 ? 12 and the hour < 12 ? 'AM' : 'PM' split are now unexercised. A re-homed case rendering 00:05 and 12:00 and asserting the hour field plus the AM/PM button would restore it through the component.
Technical details
# 12-hour boundary no longer exercised
## Affected sites
- `src/renderer/src/components/automations/AutomationTimeField.tsx:27` — `getAutomationClockParts`: `hour12: hour % 12 === 0 ? 12 : hour % 12`, `period: hour < 12 ? 'AM' : 'PM'`.
- `src/renderer/src/components/automations/AutomationTimeField.tsx:36` — `formatAutomationTimeFromClockParts` inverse.
- Deleted owner: `AutomationTimeField.test.tsx:31-44` (old lines).
## Required outcome
- Midnight (`00:05` → 12:05 AM) and noon (`12:00` → 12:00 PM) round-trip through the rendered field.
## Suggested approach
- Extend the `AutomationTimeField interactions` describe with a render at `00:05` and one at `12:00`, asserting `Hour` value and the AM/PM button label, then a commit round-trip.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between f60e7b4dc73032f91be0de4df2d7e2170eaac3d4 and a83f92a. You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 70475e0 and f60e7b4dc73032f91be0de4df2d7e2170eaac3d4. 📒 Files selected for processing (29)
💤 Files with no reviewable changes (21)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe pull request removes selected exported helpers, types, and functions, and removes associated tests across configuration, mobile, main-process, and renderer code. Some implementations remain but are no longer exported. Other changes remove individual test cases or entire test suites. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change removes internal exports and redundant tests without an established runtime regression. No actionable merge-blocking issue remains in the supplied evidence; merge after normal CI checks pass. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
… owners Fourth audit wave, cut short by a session restart, so this lands the verified subset rather than the full batch. Removes private-predicate cases whose behavior is already covered through the module's real entry point, and de-exports the seams they reached for. Also drops three whole files whose every case was a duplicate or a call-shape grep. The source-grep vein is close to exhausted. One auditor reviewed 15 remaining flagged files and deleted nothing: what is left is mostly legitimate architectural ratchets that no type checker and no behavioral test can reach — AST fences banning `as`/`any` in an RPC operation region, discovered-vs-listed set equality over subscription sites, count ceilings on unchecked reply readers, and assertions on generated WebView bundles (no CDN URL, no `</script` tokenizer escape, parses at the Chrome 74 floor). Those stay.
f60e7b4 to
a83f92a
Compare
Comments Outside DiffThese findings could not be posted inline.
|
Fifth audit wave, renderer-scoped. Same shape as #23941: delete private-predicate cases whose behavior is already asserted through the module's real entry point, then make the symbol module-private again. Two whole files went as provider-local replays: `terminal-paste-payload-metadata` (same corpus, same inputs, same expectations as `lib/paste-payload-metadata`, just renamed symbols) and `repro-8784-ghe-avatar-fallback` (every case now asserted through the rendered component in `github-user-avatar.test.tsx`). Dead production code removed where tests were its only callers: `resolveImeModifierGesture`, `isCodexTerminalStartupCommand` (its codex branches are covered by `isKnownTuiAgentTerminalStartupCommand`), `reconcileSelectionKeys` (superseded by `reconcileSourceControlSelectionState`, which the hook calls), and `chatFontScaleShortcutLabels` (nothing renders those labels). Roughly three quarters of the flagged modules were detector false positives and were left untouched; the scanner cannot see re-export barrels, `.web.ts` platform variants, build-time overrides, or `*ForTests` inspection seams. `accumulateWorkItemPages` is deliberately NOT deleted despite having only test callers. It, the dead `filesystem-directory-listing-limit` module, and `resolveImeModifierGesture` all landed unwired in the same refactor (#16177), and unwired page accumulation is more likely a lost wiring step than dead weight. Deleting it would erase that evidence.
ELI5
Fourth batch of the test-suite cleanup. Same idea as #23815, #23816 and #23829: remove tests that check how the code is written rather than what it does, and tests that re-check something another test already proves better.
What Changed
Nothing changes for users. Test-suite only.
exportkeyword from the seams those tests reached for, so the helpers are module-private again.terminal-pane-quick-command-initialization.test.ts, sixtoContaingreps over two sources including a verbatim call-site spelling (useTerminalQuickCommandHosts(worktreeId, contextMenu.open)); andconfirmation-dialog-refresh-boundary.test.ts, which rendered a provider and assertedtypeof result.current === 'function'— a probe that passes whether or not the Fast Refresh split it is named for exists.describe('OpenCode hook plugin source')block insrc/main/opencode/hook-service.test.ts(140 lines, 9 cases). It greped the generated plugin string for exact text (let cachedEndpointKey = "";,currentSessionID = session.parentID;,indexOfordering, plus an sha256 "bytes stable across the module split" golden). Every contract it greped is owned by tests that actually load and drive the plugin —hook-plugin-module-contract.test.tsdynamic-imports it and feedspermission.v2.asked,hook-plugin-child-attention.test.tsdrivesquestion.asked→AskUserQuestion, and the opencode2 setup/lineage tests cover the endpoint file and child filtering.process.title,process.env.ORCA_PANE_KEY, a"return '/hook/omp'"pair) while keeping the@orca-managed-pi-extensionownership marker and the/hook/pivs/hook/prime-agentrouting assertions, which are real install contracts.Why
A test that reaches a private helper through an export added only for it measures the helper, not the feature — and it makes the export look load-bearing to anyone reading the module.
The important result of this wave is a negative one: the source-grep vein is close to exhausted. One auditor reviewed 15 remaining flagged files and deleted nothing. What survives the earlier waves is mostly legitimate architectural ratchets that neither the type checker nor any behavioral test can reach:
as/any/@ts-*inside the RPC operation region, with per-kind exceptions and a staleness check.pilot-scenarios.json.sendRequestport — a compile-time fence is impossible until the migration they count finishes.</scripttokenizer escape, and it must parse at the Chrome 74 floor. Nothing else catches an inline-script breakout or a syntax regression that only fails on an old WebView.Those all stay. Detector counts across the tree bear the trend out: source-text greps went 111 → 25 and cross-file duplicate test bodies went 49 clusters → 6 over the merged waves.
Scope note
This wave was cut short by a session restart that killed the auditors mid-run, so it lands the verified subset rather than the full batch. 282 of the 294 flagged leaked-internal modules remain unaudited and will follow in a later PR. Everything here is independently verified (below); nothing half-edited is included.
Linked Issue
N/A — test-suite hygiene, no pre-existing issue.
Visual Proof
N/A — no product code path changes. Production edits only remove an
exportkeyword.Testing
Verified locally on macOS:
pnpm run typecheck— clean, afterrm -f config/tsconfig.*.tsbuildinfo(a stale build info masked a real error earlier in this audit, so this is now routine).cd mobile && npx tsc --noEmit— clean. Separate project the root typecheck does not cover; it validates the mobile de-exports.mobile/scripts/check-tests-typecheck-ratchet.mjs— OK, 882 test files, 125 grandfathered.node config/scripts/check-reliability-gates.mjs— passes, 140 gates. Confirmed no gate names any deleted file.pnpm run check:code-quality:changed— passes, 0 findings.Not run locally: e2e and the Windows/Linux/SSH jobs — left to CI.
Removal-only; no new tests.
Review
Worth a look:
src/main/global-fetch-call-site-audit.test.tswas kept but is the most fragile thing in this area — a per-file line-count map oversrc/{main,cli,relay}where some entries count comments orgit fetchidentifiers, so an unrelated edit can break CI. It is kept because it is the only thing forcing a re-audit when a new globalfetch(lands, and an unread undici body crashes the whole process (orca#8695). It cannot be partially trimmed: any found file missing from the map fails its drift check. The right fix is to make its regex AST-based, not to delete it — filed as a follow-up rather than done here.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)