Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ThomasK33
changed the title
|
@codex review |
🟢 e2e stories (Haiku): 14 passed22 agent steps · 7 replayed from cache · 15 model calls · 46.3k tokens (28% cached) All 14 tests in 9 files
e2e 0.20.0 · 1m 25s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.0k tokens (32% cached) All 3 tests in 3 files
e2e 0.20.0 · 18.2s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 5.3s · 2 targets (web, phone) · run artifacts |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f33ced46b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2f33ced to
b8e5235
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8e5235154
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b8e5235 to
ff84a04
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff84a0431e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ff84a04 to
e05c12a
Compare
|
@codex review |
|
P3-3 (node/common dead code, stack #7558): status of the 53 node/common 48 entries are deleted across #7554, #7555, #7556 (all three in the merge queue), #7557, #7600 and #7601. I ran this PR's checker against the top of that stack. It reports those 48 as stale and finds nothing new. I keep 5 exports. Please move them from
If this PR merges first, I remove the stale keys from both lists in my open PRs and make these moves myself. Generated with |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e05c12aa06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
…nd barrel shadowing; exempts only workflow meta/default
…onJS, filters shadowed names in namespace uses, skips import-type qualifiers, resolves index.jsx
64ed0f6 to
f8d1727
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8d1727823
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
P3-3 final state: #7554, #7555, #7556, #7557, #7565, #7600 and #7601 are all merged on main. I ran this PR's checker (head Generated with |
…ept exports to allowed
|
P3-2: applied P3-3's handoff in f03ad32 (merge of main + allowlist update). I verified it: on main the checker reported exactly 48 stale |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f03ad32166
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Early stop (AGENTS.md PR Workflow, cases 2 and 4). This PR needs a human decision before it can merge.
Current state of the final head
Options for the maintainer:
My recommendation is A. Every remaining gap is a missed finding or a case that does not exist on main, and the gate already catches newly added test-only exports. That catch is proven in |
|
P3-2 update while this PR waits for the early-stop decision above. Main moved after the final head I did not push that change to this PR, because a push starts a seventh Codex round. It is ready as P3-4 also flagged the story helpers in |
…ommon (coder#7600) ## Summary Delete the type aliases, interfaces and re-exports in node/common that nothing references. P3-2's new check (`check-test-only-exports`, coder#7561) lists each of them in `pendingDeletion` for this workstream. Deleted (no file imports them. Most hits for these names are other declarations with the same name): - `z.infer` aliases: `AgentPluginInstallSource` (and its unused re-export from `appConfigOnDisk.ts`), `AnthropicSpeed`, `FastModePreviousServiceTier`, `ThemePreferenceConfig`, `RawQueryColumn` and `RawQueryResult` (`analytics.ts`; `queries.ts` declares its own), `ArtifactVersionSource`, `SkillsCliCommand`, `RuntimeEnablementOverrides` (and its unused re-export), `EvaluationInput`, `EvaluationProviderOptions`, `SessionTapeEventLine`, `SessionTapeLine` (and `SessionTapeLineSchema`, which only that alias used), the three `AutoRetry*Event` aliases in `stream.ts` (`retryManager.ts` declares the ones production uses), `StatusUpdateToolArgs`, `PlanReviewSnapshotRecord`, `PlanReviewFeedbackReply`, `PlanReviewThreadReply` and `MappedCodexHookEvent`. - `PersistedKeyScope`, and the localStorage-era `ReviewState` and `ReviewsState` interfaces in `types/review.ts`. The DI tag `ReviewState` and `ChatPane`'s local `ReviewsState` are other declarations. - Re-exports nothing imports: `CLAUDE_DESIGN_SERVER_NAME` from `claudeDesignService.ts` (callers import `@/common/constants/claudeDesign`), and `AskUserQuestionOptionSchema` from `toolDefinitions.ts`. Test-only aliases. Each test now types its fixture with the production type: - `ProviderModelEntry` (`providerModelEntry.ts`): `coderOauthService.test.ts` imports the `@/common/orpc/types` one, like every other caller. - `AWSCredentialStatus` (`orpc/types.ts`): `api.test.ts` uses `ProviderConfigInfo["aws"]`. - `PlanReviewFeedbackComment`: `prepareFeedback.test.ts` uses `PlanReviewFeedbackRecord["comments"]`. ## Validation `make typecheck` and `make lint-eslint` pass. The touched suites pass (415 tests). I ran P3-2's checker from coder#7561 against this stack, and it reports every deleted entry as stale and nothing new. Part of test-audit phase 3, workstream P3-3. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high -->
…ion paths (coder#7601) ## Summary Delete four node/common helpers that only tests called, and move their tests to the code production runs. All four are in P3-2's `pendingDeletion` list (coder#7561). | Deleted | Why it was dead | Tests now | |---|---|---| | `src/cli/serverSecurity.ts` (`resolveServerAuthToken`, `isLoopbackHost`) | Nothing imports the module. `xum server` uses `serverAuthToken.ts`, an older policy with its own tests. | Deleted with the module. The live resolver keeps `serverAuthToken.test.ts`. | | `withReviewStateSection` (`reviewState.ts`) | `ReviewStateService.importLegacy` imports through deltas. Only two test doubles called it. | The doubles (`ReviewStateStore.test.ts` and the story mock) call `assignSection`. | | `ensureWorkspaceScratchDir` (`workspaceScratchDir.ts`) | Host scratch dirs come from `ensureScratchDirForSpec`. | The mkdir-failure case moved to `runtimeScratchDir.test.ts`. The why-comment moved to `ensureScratchDirForSpec`. The path tests stay on `getWorkspaceScratchDir`. | | `buildToolCatalogOverview` (`toolCatalog.ts`) | Production builds the overview inside `prepareToolSearch`. | The 8 overview assertions read the `tool_catalog_search` description that `prepareToolSearch` builds (`advertisedOverview` helper). | ## Kept (P3-2 moves these to `allowed`) - `computePriorHistoryFingerprint`: the reference implementation that the async twin's tests compare against. - `replayFixture.ts` `appendReplayFixtureCompactionBoundary`, `generateReplayFixtureSession`: the replay golden-fixture harness. - `PhoneChrome`: the type of the generated `PHONE_CHROME_JSON` skeleton. Only tests parse that JSON. - `updateCodeWorkspaceFile`: the unit boundary of the `.code-workspace` edit algorithm (23 tests). `syncProjectCodeWorkspace` runs the same `updateCodeWorkspaceFileLocked` under its own lock. ## Validation I broke the production code for each moved test, ran it, saw it fail, and restored the code: - `ensureScratchDirForSpec` host case with a throwing mkdir: the moved failure test fails. - In `buildOverview`/`groupCatalogByServer`: the "other tools" label, the ", and more" suffix, the total budget, the name sort, and the label clamp. Each mutant fails at least one overview test. `make typecheck`, `make lint-eslint`, `fmt-check` and `check-test-routing` pass. P3-2's checker passes on this stack once the stale entries are removed. Only the four kept items above (five entries) stay. Part of test-audit phase 3, workstream P3-3. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high -->
## Summary Deletes eight browser exports that only tests called, plus two story helpers that nothing calls. The tests now go through the code that production runs. No app behavior changes. ## Background coder#7561 (P3-2) adds `check-test-only-exports` and lists these symbols under `pendingDeletion` with owner P3-4 (frontend test audit). This PR clears every P3-4 entry except `DraftAttachmentMatchesChatAttachment` (see below). | Export | Why it was dead | What the test does now | | --- | --- | --- | | `GoalsSection` | Settings never mounts it. Only its child `GoalDefaultsControls` ships (in `GoalDefaultsSection`). | Renders `GoalDefaultsControls`, the same inputs. | | `highlightDiffChunk` | One-chunk wrapper. `DiffRenderer` calls `highlightDiffChunks`. | A local helper calls `highlightDiffChunks([chunk])` and asserts one result. | | `migrateWorkspaceStorage` | coder#6620 removed its last caller. | Its two tests guarded only its own "keep the source on failure" branch, so they go with it. | | `getSuggestedModels`, `calculateTPS`, `calculateTokenCount`, `DeltaRecord`, `filterWorkflowDisplayOnlyMessages` | Browser re-exports of `@/common` functions that only tests imported. | Tests import from the `@/common` module directly. | | `textContrastOnBase`, `opaqueBackgroundOwner` (story helpers) | coder#6978 removed their only caller, the project-badge story. | None. | `DraftAttachmentMatchesChatAttachment` stays: it is a compile-time drift check. Without `export`, `noUnusedLocals` and `no-unused-vars` both reject it, so coder#7561 needs to move it to `allowed`. ## Validation - I ran coder#7561's checker (`scripts/check-test-only-exports.ts` at f8d1727) on this tree. It reports all eight targeted entries as stale, which means they are gone. - The six touched test files pass (92 tests). `make static-check` and `make crap-check` pass. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$36.68`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=36.68 -->
Summary
New static check
check-test-only-exports(inmake static-check): it fails when an export of a production file undersrc/has no non-test reference, unless an allowlist entry gives a reason. Stacked on #7552, which adds the shared typescript guard.Background
The 2026-10-26 audit found about 8 dead functions that only tests call (two orphaned by feature removals) and 21 exports that nothing references.
check-test-seam-commentsonly reads comments, so all of them passed it. This check reads references instead.Implementation
isProductionSource, shared with the seam checker). Tests never count as references, and they are never parsed, so the run takes about 3 s and peaks under 300 MB.@/, relative paths, ESM.jssuffixes, index files, and named orexport *barrels), a namespace import,import(),require()or a require wrapper (requireDesktopModuleinmain.ts), animport("x").Ttype, or a use in the export's own file. Property names (o.x,{ x: 1 }) do not count.src/node/builtinSkills/**/*.js) are skipped: the workflow runtime reads theirmetaanddefaultexports by contract.settingsStoryUtils.tsxnow counts as test support (*StoryUtils/*StoryHelpersnames) inisProductionSource.scripts/check-test-only-exports.allowlist.json):allowed(35): 12...ForTestsreset hooks, 15 exports used outsidesrc/(vite.config.ts, scripts/, vscode/src, packages/chat-components, .storybook/test-runner.ts, the Makefile), 1 story-mock type, 1 compile-time type guard, 1 tests/ipc builder kept next to the mock router's parser, and 5 exports P3-3 kept on purpose (a reference oracle, two replay-fixture helpers, a generated-data type, a unit boundary with 23 tests).pendingDeletion(11, was 64): findings that the P3-4 workstream owns. P3-3 deleted 48 of its 53 (🤖 refactor: delete the dead sanitizeErrorMessageForDisplay chain #7554, 🤖 refactor: delete the test-only isValidCustomProviderId #7555, 🤖 tests: move wrapper tests to the functions production calls #7556, 🤖 refactor: delete node/common exports that nothing references #7557, 🤖 tests: typed ORPCContext doubles replace as-unknown-as casts in orpc tests #7565, 🤖 refactor: delete unreferenced type aliases and re-exports in node/common #7600, 🤖 tests: delete test-only helpers in node/common and test the production paths #7601) and kept 5 inallowed. The list only shrinks: its keys must equalFROZEN_PENDING_DELETIONin the script. An owner deletes each export, or moves it toallowedwith a reason.pendingDeletion handoff (11)
P3-4 (test-audit phase 3, frontend)
src/browser/features/RightSidebar/ArtifactsTab/artifactDocument.tsbuildSandboxedHtmlDocumentsrc/browser/features/Settings/Sections/GoalsSection.tsxGoalsSectionsrc/browser/hooks/useModelsFromSettings.tsgetSuggestedModelssrc/browser/stores/DraftStore.tsDraftAttachmentMatchesChatAttachmentsrc/browser/utils/highlighting/highlightDiffChunk.tshighlightDiffChunksrc/browser/utils/messages/StreamingTPSCalculator.tscalculateTPSsrc/browser/utils/messages/StreamingTPSCalculator.tscalculateTokenCountsrc/browser/utils/messages/StreamingTPSCalculator.tsDeltaRecordsrc/browser/utils/rightSidebarLayout.tsaddToolToFocusedTabsetsrc/browser/utils/workflowRunMessages.tsfilterWorkflowDisplayOnlyMessagessrc/browser/utils/workspaceStorage.tsmigrateWorkspaceStorageDeferred follow-ups
export { x } from "./a") are not checked as exports of the barrel yet. Checking them surfaces 32 more findings on main (compatibility re-exports instreamManager.ts,appConfigOnDisk.tsand others). Tracked in 🤖 tests: P3-2 seam-check test-audit residuals #7604 (item 1): a stacked PR after this one lands widens the gate and deletes or allowlists those 32. Owner: P3-2.Validation
export *anddefault, the require wrapper, import types, overload dedupe, default imports, named forwarding, local uses, dynamic imports) fails a test.export function newTestOnlyHelper()tosrc/common/utils/drafts.tsplus a test that imports it fails the check withdrafts.ts:243 export \newTestOnlyHelper` has no non-test reference` (exit 1). Without it the check passes.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high