Repository navigation
test: remove assertion-free probes, copied inventories and export-shape checks - #23816
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -131,24 +26,6 @@ describe('the page routes the manifest declares', () => { | |||
| expect(routePathnameFromKey('./h/[hostId]/_layout.tsx')).toBeNull() | |||
There was a problem hiding this comment.
Removing the comparisons with EXPECTED_PAGE_ROUTES leaves no independent check of every route’s grants in the built manifest. The remaining call-site tests catch missing grants but not extra ones. For example, adding native.clipboard.read to the tasks route would pass those checks, yet a shell without that capability would render the native screen instead. Please retain a check of the full route-grant contract.
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!
| ...fixture.args, | ||
| codexHomePath: null | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The deleted test checked that WSL, null-home, and custom-home launches complete without waiting for managed credentials. The remaining tests cover managed homes, so a change that makes one of these launches wait unnecessarily could pass the suite. Please keep a focused completion check for these three bypass paths.
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!
| @@ -55,31 +55,6 @@ describe('registerGitHubHandlers', () => { | |||
| }) | |||
There was a problem hiding this comment.
This deleted test checked successful IPC calls and their telemetry payloads for all five accepted star sources. The remaining successful IPC test covers only settings. A future change could drop or misattribute another source without failing these tests, leaving star telemetry incomplete. Please retain a behavior test for each accepted source.
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!
| @@ -511,7 +500,6 @@ describe('Codex WSL runtime hook install', () => { | |||
| expect((await _internals.installManagedHooksIntoWslRuntime(plan)).state).toBe('installed') | |||
|
|
|||
| const installed = JSON.parse(readFileSync(plan.configPath, 'utf-8')) as HooksConfig | |||
| expect(Object.keys(installed.hooks).sort()).toEqual([...managedEvents].sort()) | |||
| const managedCommand = installed.hooks.UserPromptSubmit[0]?.hooks?.[0]?.command | |||
There was a problem hiding this comment.
Removing the installed-event assertion leaves this WSL installation test checking only UserPromptSubmit. A WSL-specific change that omits another managed event could pass while that event’s hook no longer runs. Please keep a check that the installed WSL configuration contains all managed events.
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!
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Removal-only test hygiene, stacked on #23815: it deletes three categories of test that cannot fail for a behavior reason and de-exports one now-internal symbol. Reviewed against nwparker/test-audit-wave1.
- Assertion-free probes dropped —
cdp-ws-proxy"rejects inflight requests on stop",daemon-serverfire-and-forget write,ws-transportstart/stop, andmanaged-codex-auth-readiness"does not gate system/WSL/unmanaged custom homes". The unmanaged-homes case is genuinely unfailable: that path returnsundefinedsynchronously, and a regressed guard would resolvefalseafter a 1.5s wait rather than throw. - Copied inventories dropped — Relay
PRODUCTION_CAPACITY_CELL_IDS, xtermPNPM_DIFF_FLAGS, mobileEXPECTED_PAGE_ROUTES/SPELLED_COUNTS,BRIDGE_NATIVE_VERB_NAMES,BRIDGE_HAPTICS_KINDS, GitHub star sources, WSL managed hook events, andworkspace-list-picker-options.test.ts(the deleted file). Each list is still covered by a test that imports the constant it pins. - Export key-set/shape loops dropped —
opencode/hook-servicemodule surface, thecross-platform-path-guardscreate-factory filter, and theprocess-table-snapshotindex key set. The last duplicates the closed return type onProcessTableIndexOf. - Env-gated benchmark blocks dropped — the
ORCA_GIT_*_COALESCING_BENCH_JSONwriters instatus-read-coalescing.test.ts/upstream.test.ts. - Production —
mobileWebCheckArgsinconfig/scripts/run-mobile-web-app-checks.mjsis de-exported.
I checked the failure modes a deletion-only PR usually introduces and found none: mobileWebCheckArgs has no other consumer; PNPM_DIFF_FLAGS still has in-file users; the ORCA_GIT_*_COALESCING_BENCH_JSON env vars are referenced nowhere else; the deleted file is absent from gates, ci-shard-timings.json, and unit-file lists; and every remaining import is still used. The persisted-sort-values pin that went with the deleted mobile file is type-backed by the MobileSortMode union plus SORT_VALUES, so no cross-version contract is lost.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
78ef9d7 to
469b2a0
Compare
643e423 to
f634805
Compare
…pe checks Second audit wave, targeting three more junk patterns: - assertion-free cases that run code and assert nothing, so they pass no matter what the code does; - inventory literals re-typed from a production declaration, where the only way the assertion can fail is someone editing one of the two copies; - export key-set and export-shape loops (`typeof x === 'function'` over every export) that restate what TypeScript already enforces. Yield is much smaller than wave 1 on purpose: the assertion-free scanner has a high false-positive rate, because many flagged blocks assert through a shared helper or their oracle is "this must not throw". Those were kept. `mobileWebCheckArgs` in `config/scripts/run-mobile-web-app-checks.mjs` is de-exported — after the inventory comparison went away, nothing outside the module read it.
f634805 to
0878d76
Compare
Comments Outside DiffThese findings could not be posted inline.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThis change removes selected tests across configuration scripts, mobile code, and main and shared modules. The removed tests covered exact values and shapes, selected runtime behavior, and optional Git coalescing benchmarks. The Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Current behavior is unchanged, but future regressions in mobile routing, non-managed authentication workflows, or proxy cleanup could go undetected. These bounded test gaps are inexpensive to address, so merge risk is low. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 4 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
config/scripts/mobile-web-page-routes.test.mjs (1)
77-80: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompare the generated routes with the mapped declaration shape.
resolveMobileWebPageRoutes()omits emptyoptionalGrantsfields. Therefore, comparemanifest.routeswith the same mapped shape instead of comparing it directly withMOBILE_WEB_PAGE_ROUTES. This detects omitted or mis-mapped path and grant values without adding a separate route inventory.Suggested fix
+import { MOBILE_WEB_PAGE_ROUTES } from './mobile-web-page-routes.mjs' +import { resolveMobileWebPageRoutes } from './build-mobile-web-app-bundle.mjs' + +it('writes the declared page routes to the manifest', async () => { + const { manifest } = await buildMobileWebAppBundle(...) + expect(manifest.routes).toEqual( + resolveMobileWebPageRoutes( + routeKeys, + MOBILE_WEB_PAGE_ROUTES + ) + ) +})src/main/codex-accounts/managed-codex-auth-readiness.test.ts (1)
223-238: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRestore explicit bypass assertions for non-managed Codex homes.
waitForManagedCodexAuthReadymust returnundefinedfor custom homes, WSL targets, and a missing home. The removed test awaited these calls but never asserted their return values, so it would pass even if the helper started retrying unavailable credentials. No remaining test covers these three bypass branches.Suggested fix
it('separates an absent credential from one that cannot be read', () => { const fixture = createFixture() expect(readStoredCodexCredentialState(join(fixture.home, 'auth.json'))).toBe('missing') if (process.platform === 'win32' || process.getuid?.() === 0) { return } writeAuth(fixture.home, testChatGptAuth) chmodSync(join(fixture.home, 'auth.json'), 0o000) expect(readStoredCodexCredentialState(join(fixture.home, 'auth.json'))).toBe('unreadable') }) + + it('does not gate system, WSL, or unmanaged custom homes', () => { + const fixture = createFixture() + expect( + waitForManagedCodexAuthReady({ + ...fixture.args, + codexHomePath: join(fixture.root, 'custom-home') + }) + ).toBeUndefined() + expect( + waitForManagedCodexAuthReady({ + ...fixture.args, + target: { runtime: 'wsl', wslDistro: 'Ubuntu' } + }) + ).toBeUndefined() + expect( + waitForManagedCodexAuthReady({ + ...fixture.args, + codexHomePath: null + }) + ).toBeUndefined() + }) })src/main/browser/cdp-ws-proxy.test.ts (1)
770-805: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an invocation barrier before stopping the proxy.
The final
resolveCommand!({})call makes the test fail ifsendCommandnever runs. However, the 10 ms delay does not prove thatsendCommandran beforeproxy.stop()began. A delayed WebSocket message can start after shutdown begins, so the test may avoid the in-flight condition. Restore the test with a barrier resolved by thesendCommandmock.Suggested fix
it('rejects inflight requests on stop', async () => { let resolveCommand: (v: unknown) => void + let markCommandInvoked!: () => void + const commandInvoked = new Promise<void>((resolve) => { + markCommandInvoked = resolve + }) mock.webContents.debugger.sendCommand.mockImplementation( - () => - new Promise((r) => { + () => { + markCommandInvoked() + return new Promise((r) => { resolveCommand = r as (v: unknown) => void }) + } ) const client = await connect(endpoint) client.send(JSON.stringify({ id: 1, method: 'Page.enable', params: {} })) - await new Promise((r) => setTimeout(r, 10)) + await commandInvoked await proxy.stop()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 4ae27cc9-3f98-460e-86e2-bad2c9e7676c
📒 Files selected for processing (19)
cloud/dev/scripts/prepare-relay-production-capacity-canary.test.mjsconfig/scripts/mobile-web-page-routes.test.mjsconfig/scripts/pr-workflow-parallelism.test.mjsconfig/scripts/regenerate-xterm-patches.test.mjsconfig/scripts/run-mobile-web-app-checks.mjsmobile/src/mobile-web-shell/bridge/bridge-native-verbs.test.tsmobile/src/mobile-web-shell/page-haptics.test.tsmobile/src/worktree/workspace-list-picker-options.test.tssrc/main/browser/cdp-ws-proxy.test.tssrc/main/codex-accounts/managed-codex-auth-readiness.test.tssrc/main/codex/hook-service-wsl-runtime.test.tssrc/main/daemon/daemon-server.test.tssrc/main/git/status-read-coalescing.test.tssrc/main/git/upstream.test.tssrc/main/ipc/github-star-telemetry.test.tssrc/main/opencode/hook-service.test.tssrc/main/runtime/rpc/ws-transport.test.tssrc/shared/cross-platform-path-guards.test.tssrc/shared/process-table-snapshot.test.ts
💤 Files with no reviewable changes (14)
- src/main/runtime/rpc/ws-transport.test.ts
- src/shared/cross-platform-path-guards.test.ts
- mobile/src/worktree/workspace-list-picker-options.test.ts
- src/main/codex-accounts/managed-codex-auth-readiness.test.ts
- src/main/opencode/hook-service.test.ts
- config/scripts/pr-workflow-parallelism.test.mjs
- src/main/daemon/daemon-server.test.ts
- src/shared/process-table-snapshot.test.ts
- src/main/ipc/github-star-telemetry.test.ts
- src/main/browser/cdp-ws-proxy.test.ts
- config/scripts/regenerate-xterm-patches.test.mjs
- mobile/src/mobile-web-shell/bridge/bridge-native-verbs.test.ts
- src/main/codex/hook-service-wsl-runtime.test.ts
- src/main/git/status-read-coalescing.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
…3745, #23783, #22971, #22565, #22846, #23724, #23721, #23693, #23810) into #23059 #23671 (group chat rows by the turn that produced them) is ported onto #23059's turn-scope model, so there is one grouping mechanism, not two: - A host that states each row's turn scope groups by that scope, as before. #23671's journal-order derivation (rows between one turn record and the next belong to it) moves from the timing selector into src/shared/native-chat-turn-grouping.ts and runs only for a host that states no scope, replacing the positional fallback there. - #23671's anchor rules live in structuredAgentTurnAnchors, which membership, settled timing and the live clock all read: a turn whose opener key nothing resolves yet takes the send still in flight ahead of its record (Codex before its echo), else its own record. - The live turn is one key again. #23671 made row liveness follow the owning turn, so the bar key and the liveness key are always the same; nativeChatTurnMembership now returns only liveTurnKey (barTurnKey, activeTurnOpenedBy and turnKeysByItemId are gone). - A turn with no user bubble draws its bar above its first row (nativeChatTurnBarRows, desktop and mobile), and a turn's diff rollup goes under its last row. - The Codex full-history restore writes each record ahead of its items; that also fixes the derived turn scope on #23059's journal. #23026 (Stop from the moment a message is sent) is ported onto #23059's command turns: the cancel paths ask the translator for a command's provider turn instead of the removed compaction tracker, and a Stop that names no turn still ends a running /compact through its child. performCancel moved to structured-agent-session-turns-cancel.ts to keep the turns module under the line limit. #22090's awaitingInput replaces showTurnStatus/showLiveTurnActivity; the waiting-behind-/compact rows are kept.
ELI5
Stacked on #23815. That PR removed tests that read source code as text. This one removes three smaller categories: tests that run code and then check nothing at all, tests that retype a list the app already declares and check the two copies match, and tests that check "every export exists and is a function" — something the type checker already guarantees.
What Changed
Nothing changes for users. Test-suite only. 1 test file deleted, 16 trimmed.
src/shared/priority-semaphore.test.tshad a "works with zero waiters" case with no assertion at all, and an "allows up to N" case whose only assertion wastypeof release === 'function'.Object.keys(module)compared to a hand-written list, andfor (const fn of Object.values(mod)) expect(typeof fn).toBe('function'). TypeScript enforces both.One production change:
mobileWebCheckArgsinconfig/scripts/run-mobile-web-app-checks.mjsis de-exported. Once the inventory comparison went away, nothing outside the module read it.Why
These three patterns share a failure mode with the source greps in #23815: the assertion cannot fail for a reason a user would notice. An assertion-free case is worse than no test, because it occupies the filename where real coverage would be and reports green.
Deliberately low yield. The assertion-free scanner flagged 144 files; only 17 needed edits. The rest were false positives I told the auditors to keep, in two shapes worth naming so the next sweep doesn't re-flag them:
expectRejected(...), a table-driven runner, a custom matcher — so no literalexpectappears in the block.Alternatives considered: deleting every scanner hit unread would have been far faster and would have destroyed real coverage in both shapes above — so each flagged block was read and the helper followed before judging.
Linked Issue
N/A — test-suite hygiene, no pre-existing issue.
Visual Proof
N/A — no product code path changes. The single production edit only narrows a symbol's visibility.
Testing
Verified locally on macOS, on top of the rebased #23815:
pnpm tc— passes.pnpm run check:code-quality:changed— passes, 0 findings across 36 changed files.node config/scripts/check-reliability-gates.mjs— passes for 140 gates; confirmed no gate,cloud/package.jsonentry, or typecheck baseline names the deleted file.main, the 4 remaining consumers ofagent-launch-caller-profiles-test-harness.ts(whose dead fields were pruned in test: remove junk tests that assert source text instead of behavior #23815): 275 passed.Not run locally: e2e and the Windows/Linux/SSH jobs — left to CI.
Removal-only; no new tests. Verification is that the remaining suite passes and nothing references a removed file.
Review
Worth pushing back on: any case I classified as a legitimate "must not throw" oracle that you think was genuinely asserting nothing. That judgment call is the whole content of this PR, and it went the conservative way 127 times out of 144.
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
exportkeyword.src/shared/cross-platform-path-guards.test.tsandsrc/shared/process-table-snapshot.test.tswere trimmed of export-shape loops only; their platform behavior cases are intact.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)