Repository navigation
test: stop testing private internals through exports no caller needs - #23829
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| expect(cleanup?.[1]).toBe('jitteredSweepIntervalMs(30_000)') | ||
| }) | ||
|
|
||
| // A census, not a list of the timers that happen to be gated today: an ungated sweep runs in |
There was a problem hiding this comment.
The two removed assertions were the only checks that the assignment and credential cleanup timers use jitter. The remaining tests check the jitter calculation and maintenance-role gating, but would still pass if either timer changed to a fixed 30-second interval. Please retain a check of those timers’ wiring so that loss of load spreading is caught.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @@ -98,68 +71,6 @@ describe('mobile AI Vault resume target guards', () => { | |||
| { id: 'group-runtime', executionHostId: 'runtime:devbox' as const } | |||
There was a problem hiding this comment.
The removed case checked that a worktree’s explicit SSH host takes precedence over its local repo’s host. None of the remaining resolver tests supplies a worktree with an explicit hostId, so a change that incorrectly allows resume in that SSH workspace would go unnoticed. Please cover this case through resolveMobileAiVaultSessionResumeTarget.
| @@ -101,38 +100,6 @@ describe('mobile hosted review eligibility loader core', () => { | |||
| expect(local.identity).not.toBe(ssh.identity) | |||
There was a problem hiding this comment.
Removing the error-state assertion leaves no test that checks what the hook returns when an eligibility request fails. The remaining request tests cover successful responses; the error-state test elsewhere starts with an error state rather than producing one. Please test a null or rejected response through the hook and assert it returns { kind: 'error' }, so a failed refresh cannot silently leave Create PR eligible.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one documentation gap worth a look.
Reviewed changes
- Test deletions/trims — ~36 test files trimmed (813 lines removed); dropped tests that pinned constants, asserted cross-function consistency, greps of source text, or drove private helpers directly, keeping the behavior tests that run through real entry points.
- De-exports — ~15 symbols returned to module-private (
agent-history-resume-target,mobile-native-chat-message-text, the rendererlib/helpers,linear-scope-selector,task-page-default-repo-selection, etc.). - Test import rewrites —
useEditorExternalWatch*.test.tsand friends now importbuildEditorExternalWatchEventHandlerfromeditor-external-watch-event-reconciliationandselectEditorExternalWatchTargetsfromeditor-external-watch-targets, letting the compatibility delegates go. - Dead production code removed —
filesystem-directory-listing-limit.ts,format{Hourly,Daily,Adhoc}Version+ prerelease constants, thefilterByAutomationListSearch*family,getAiVaultResumeWorktreeTargetStatus+ wrappers,CODEX_BLOCKING_SERVER_REQUEST_METHODS, and more.
I independently re-ran the caller check (grep for every fully-deleted symbol: zero residual callers; the lone ci-shard-timings.json hit is inert, since that map only looks up discovered files), tsc for both the node and web projects, and targeted root tests for the rewritten imports — all clean. The deletions and de-exports look correct.
ℹ️ The description's removal inventory undercounts what the diff actually deletes
The "Production code removed" list names five symbol groups and the Testing section claims "0 hits for all 11," but the diff fully deletes roughly twice that many production symbols. Since the PR explicitly asks reviewers to push back on premature production deletions by name, the list should cover the whole set — otherwise a reviewer can sign off on the five groups and never look at the rest.
Technical details
# Unlisted production deletions
The five called-out groups are `filesystem-directory-listing-limit.ts`,
`format{Hourly,Daily,Adhoc}Version` (+3 constants), the
`filterByAutomationListSearch*` family, `getAiVaultResumeWorktreeTargetStatus` (+2
wrappers), and `CODEX_BLOCKING_SERVER_REQUEST_METHODS`. The diff additionally
removes (all verified to have zero remaining callers):
## Affected sites
- `src/shared/markdown-document-listing-limits.ts` — `isMarkdownDocumentListingCapacityError`, and the `MARKDOWN_DOCUMENT_LISTING_ERROR_MESSAGE` export
- `src/shared/quick-open-listing-limits.ts` — `createQuickOpenListingBudget`, `retainQuickOpenPath`, `QuickOpenListingBudget`, `QUICK_OPEN_LISTING_MAX_RETAINED_PATHS`, `QUICK_OPEN_LISTING_MAX_RETAINED_PATH_BYTES`
- `src/renderer/src/lib/new-workspace-project-options.ts` — `searchNewWorkspaceProjectOptions`, `buildNewWorkspaceFolderSourceOptions`, `getRepoIdFromNewWorkspaceFolderSourceOptionId`, `NEW_WORKSPACE_FOLDER_SOURCE_OPTION_PREFIX`, `NewWorkspaceProjectOptionBase`
- `src/main/ssh/ssh-remote-commands.ts` — `promoteRemoteTreeContentsCommand`, `commandInRemoteDirectory`
- `src/shared/grok-session-paths.ts` — `isGrokChatHistoryPath`
- `src/renderer/src/components/task-page-default-repo-selection.ts` — `getTaskProjectPickerRepos`
- `src/renderer/src/lib/worktree-palette-create-action.ts` — `WORKTREE_PALETTE_SELECTION_MOVE_KEYS`
- `src/renderer/src/lib/app-menu-paste.ts` — `shouldOwnAppMenuTextControlPaste`
- `src/renderer/src/lib/launch-structured-agent-session.ts` — `isDefinitiveStructuredAgentSessionCreateError`
## Required outcome
- The "Production code removed" list and the "all 11" count reflect the full set of
deleted production symbols/modules, so a reviewer can scope their check correctly.
## Open questions for the human
- Is `isMarkdownDocumentListingCapacityError` intentionally dropped rather than kept as
evidence that IPC/remote markdown capacity errors (Electron-wrapped) still need a
classifier, the way `mobile-file-directory-limit.ts` was kept? It is dead today, so
the code change is fine either way — this is only about whether the description's
"five groups" framing is complete.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Third audit wave. The detector looked for production modules exporting three
or more symbols that no production file imports — only tests do. That shape is
the authoring gate's fourth question failing: a test needing a production seam
no caller needs belongs at the real boundary instead.
Most hits were detector false positives and were left alone; the scanner misses
re-export barrels and dynamic imports, so every module was re-verified with rg
before any edit. Where a private predicate's behavior was already covered
through the module's real entry point, the duplicate cases are gone and the
symbol is module-private again. Where it was NOT covered anywhere else, the test
stays — this audit removes tests, it does not author replacements.
Production code deleted where tests were its only callers: the superseded
`filesystem-directory-listing-limit` module, the unused
`format{Hourly,Daily,Adhoc}Version` helpers and their orphaned prerelease
identifiers, the dead `filterByAutomationListSearch*` family superseded by
`matchAutomationListSearchRowKeys`, and the dead
`getAiVaultResumeWorktreeTargetStatus` copy of the live workspace branch.
Also drops two call-shape source greps in `relay-sweep-schedule.test.ts` that
asserted `index.ts` spells `jitteredSweepIntervalMs(30_000)`; the jitter math
has a behavioral owner at the top of the same file. The structural census that
counts role-gated vs total `setInterval(` calls stays — an ungated sweep runs in
every cell, and nothing else can catch that.
6104877 to
e48e922
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (56)
💤 Files with no reviewable changes (34)
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 tests and helper APIs across mobile, main-process, renderer, and shared code. It also changes several helper exports to module-local declarations and updates editor-watch tests to import helpers from dedicated modules. Some retained tests now use literal release-version strings or check indexed-field limits. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The editor-watch test import is valid, and no concrete issue remains that should delay merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected production paths retain their path-size and result limits, and no new security exposure was identified. The full consumer surface has not been established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 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 |
…3789, #23812, #23852) into brennanb2025/codex-home-single-writer #23744 moves every agent's folder pre-trust into the PTY spawn hook (and the structured Codex chat create), removing Codex's launch-prep and resume-prep trust writes. markCodexProjectTrusted now takes the config files to write. Conflicts: - agent-trust-presets.ts: #23744's configFiles signature and nested lane acquisition, with this branch's lane rationale (hook installs read and rewrite across awaits); main's "capture->restore window" wording described the grant snapshot this branch removed. - agent-trust-write-deadline.ts: the Codex writer waits only on managed-home installs' inline grant; the real-home background approval holds no lane. #23744's SSH/SFTP writer is gone, so it is no longer named; its 1.5 s short deadline is kept. - codex-trust-config-concurrent-launch.test.ts: this branch's tests (a launch's write lands while a grant session runs; the renamed sibling-failure case) with #23744's new call signature. Outside the conflicts: codex-launch-during-real-home-grant.test.ts passed launchAgent/workspacePath into launch prep, which #23744 removed; its launch helper now runs launch prep and then applyAgentWorkspaceTrust, as the spawn builder does, so it still proves a launch and its project trust land while the real-home approval hangs.
ELI5
Stacked on #23816. Some tests can only run because the production file was changed to expose an inner helper the real app never uses. That's backwards: the test drives a private detail through a door added just for it, so it passes while the behavior users actually get goes unchecked. This removes those tests where the same behavior is already checked through the real entry point, and closes the door again.
What Changed
Nothing changes for users. 1 test file deleted, ~30 trimmed, and several genuinely dead production modules/symbols removed.
The detector looked for production modules exporting 3+ symbols that no production file imports — only tests do. That shape is the authoring gate's fourth question failing: does the test need a production seam no production caller needs? If yes, it belongs at the real boundary.
For each module, per symbol:
exportkeyword removed, so the symbol is module-private again.Production code removed (tests were its only callers):
src/shared/filesystem-directory-listing-limit.ts— whole module. See the note below; this one has a story.format{Hourly,Daily,Adhoc}Versioninsrc/shared/release-channel.ts, plus three now-orphaned*_PRERELEASE_IDENTIFIERconstants. Zero callers repo-wide.filterByAutomationListSearch*/automationListSearchFieldsMatchfamily — superseded dead code; the live filter ismatchAutomationListSearchRowKeys, already owned byautomation-list-search-rows.test.ts.getAiVaultResumeWorktreeTargetStatusand two boolean wrappers — a dead parallel copy of the worktree branch inside the livegetAiVaultResumeWorkspaceTargetStatus.CODEX_BLOCKING_SERVER_REQUEST_METHODS— an array compared against a copied literal of the same constants; the array itself was dead.Also removes two call-shape source greps in
cloud/apps/relay/src/relay-sweep-schedule.test.tsthat assertedindex.tsspellsjitteredSweepIntervalMs(30_000)(one anchored on a log-message string). The jitter math has a behavioral owner at the top of that same file. The structural census beside them stays — it counts role-gated vs totalsetInterval(calls, survives renames, and guards a fleet-wide cost invariant: an ungated sweep runs in every cell, multiplying one table scan by the fleet size, and nothing else can catch that becauseindex.tsboots a server on import.Why
A test that reaches a private helper through an export added for it measures the helper, not the feature. The clamping example from this wave:
mobile-native-chat-message-text.test.tsdroveclampFontScaledirectly, whileuse-mobile-native-chat-pinch-gesture.test.tsalready assertsquantizeFontScale(5) === 1.8,quantizeFontScale(0.1) === 0.8, andquantizeFontScale(NaN) === 1through the function production actually imports. Same three contracts, one boundary stronger. The private test went; the bounds are still checked.The detector was mostly wrong, and that's the important caveat. On the first batch, 19 of 26 flagged modules were false positives — the import scanner misses re-export barrels, dynamic imports, and string-keyed access. So every module was re-verified with
rgbefore any edit, and false positives were left completely untouched. I'm reporting the precision because it bounds how much to trust the detector, not the review.Alternatives considered: rewriting each private test at the real boundary would be better than deleting, but it means authoring tests, which needs its own authoring-gate justification per test and does not belong in a removal PR. Hence the split: delete only where coverage already exists at the stronger boundary, and file the rest as follow-ups.
Two findings I deliberately did NOT act on
1. Two directory-listing guards are dead, and mobile listing looks unbounded.
src/shared/mobile-file-directory-limit.tsexports a complete guard (MAX_ENTRIES = 10_000,MAX_RETAINED_BYTES = 4 MiB,trackMobileFileDirectoryEntry,assertMobileFileDirectoryWithinLimit) and its only importer is its own test — nothing insrc/ormobile/wires it.filesystem-directory-listing-limit.tswas a near-identical duplicate (100_000 / 12 MiB) added by refactor #16177 and likewise never wired; that one is deleted here, because deleting a duplicate of an already-dead module changes no behavior.I left
mobile-file-directory-limit.tsin place even though it matches the "dead production code whose only callers are tests" pattern. It is the more plausible intended guard, and deleting it would erase the evidence that the bound is missing. Someone who owns mobile file listing should decide: wire it, or confirm another layer bounds the listing. Compareworkspace-space-scan-budget.tsandmarkdown-document-listing-limits.ts, which are wired.2. Needs a boundary rewrite (follow-up, not done here).
src/renderer/src/components/settings/AdvancedNetworkSettingsSection.tsxexports 7 helpers used only by its test; production imports just the component. The real behavior — an async settings reload must not clobber in-flight proxy edits — is covered nowhere else, and there is no component-level test. The right fix is to render the section and drive the inputs, then de-export. The test is kept as-is until someone does that.Linked Issue
N/A — test-suite hygiene, no pre-existing issue.
Visual Proof
N/A — no product code path changes. Production edits either remove an
exportkeyword or delete code with no non-test callers.Testing
Verified locally on macOS:
pnpm run typecheck— clean, run afterrm -f config/tsconfig.*.tsbuildinfo(a stale build info masked a real error earlier in this audit).cd mobile && tsc --noEmit— clean. This is a separate project the root typecheck does not cover, and it is the check that 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 for 140 gates.pnpm run check:code-quality:changed— passes, 0 findings across 57 changed files.cloud/apps/relay/src/relay-sweep-schedule.test.ts: 4 passed.rgfor residual references, including non-TypeScript callers: 0 hits for all 11.Not run locally: e2e and the Windows/Linux/SSH jobs — left to CI.
Removal-only; no new tests.
Review
Please push back on:
mobile-file-directory-limitcall. I chose to flag rather than delete. If you'd rather it go, it's a one-line removal — but the missing bound is the real question.relay-sweep-schedule.test.ts— I kept thesetIntervalcensus and dropped the two call-shape greps. If you think the census is equally brittle, say so.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)