Repository navigation
refactor(sdk): reuse client construction and scan data - #1374
mldangelo-oai wants to merge 6 commits into
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. |
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the current diff; no actionable issues found.
c626787 to
0ec7f6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec7f6e593
ℹ️ 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".
kmbroai
left a comment
There was a problem hiding this comment.
Re-reviewed the complete updated diff and the evals TypeScript configuration fix. The previous compilation blocker is resolved: both SDK and evals builds pass locally. All 532 focused regression tests passed, with 26 platform skips. Verified the compiled ACL parser on the minimum supported Node 22.13.0 runtime. Current CI has pending jobs but no failures; discussions rechecked. No actionable findings at this commit.
Withdrawing my approval while a newly surfaced progress-handling concern in the parent change is validated.
kmbroai
left a comment
There was a problem hiding this comment.
Restoring approval after validating the late progress-count concern against the exact base and this head: both reproduce identical behavior. Production callers never supplied the removed expectedFilesTotal option, so this change does not introduce that existing issue. The evals build fix remains verified; SDK/evals builds, 532 regression tests, and minimum Node 22.13.0 ACL checks passed. No actionable regression found in this diff. New CI jobs remain pending.
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the current diff; no actionable issues found.
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed the diff and relevant surrounding code at 0ec7f6e. No new correctness or regression issues found. Focused helper checks passed; the full suite was not rerun locally. I also checked the existing progress-filter comment against the base: production callers already omitted expectedFilesTotal, so the removed filter was inactive there before this change. Some CI checks were still running when reviewed.
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the current diff; no actionable issues found.
Summary
Use the existing SDK constructor for client creation and CLI runtime selection, removing a redundant internal factory. Reuse normalized scan data and existing parsing behavior to simplify the remaining runtime code.
Changes
Reuse the normalized target when recording scan recipes, use the shared JSON parser for saved usage events, and remove a redundant worker-capacity condition. Remove an unused internal progress option and expand the scanner-inventory test to cover both model messages and completed commands.
The remaining diff removes 47 net lines across nine files. It retains the callback helper, callback-order tests and shared fixtures now on main.
Testing
The focused SDK run passed 1,011 tests across 30 files, with 27 skipped. It covered callbacks, authentication, provider isolation, runtime selection, scan and policy operations, usage records, and Deep Scan worker recovery. The focused run includes the upstream callback capture-order regression and current resumed-scan fixtures.
Source-derived checks preserved the upstream callback and worker code, compared 16,961 saved-event and chunk cases, and matched 64 scan recipes generated from actual normalized targets. Both production progress callers retained identical behavior.
The inventory cases preserve the progress sequence from a 4,207-file preflight estimate to the actual 4,198-file inventory, for both message and command events. To repeat the central scenarios from
sdk/typescriptafter preparing dependencies and the bundled plugin:bun test --timeout 30000 --seed 12345 tests-ts/api.test.ts tests-ts/api-events.test.ts tests-ts/cli-project-config.test.ts tests-ts/runtime.test.ts tests-ts/cost.test.ts tests-ts/worker-progress.test.tsFrom
sdk/typescript,pnpm run build:plugin,pnpm run build:ci,pnpm run types,pnpm run format,pnpm run build:evals, andpnpm run build:examplespassed.Risk and rollout
Public commands, configuration, authentication, worker settings, and diagnostic text remain unchanged. CLI construction keeps its explicit runtime surface, and scan recipes keep an independent copy of their target paths.
Public disclosure review