Repository navigation
refactor(cli): simplify command setup and fixtures - #1208
Conversation
Repository fixtures and hydration tools duplicate Git process setup. Share the existing invocation patterns with their actual consumers.
SDK entrypoints and tests repeat runtime, authentication and option setup. Share the setup at existing owners and introduce internal helpers with their first consumers.
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. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The refactoring preserves existing behavior and retains focused coverage for signals, diagnostics, settings, and fixtures.
Review effort: Balanced
Findings: None
What changed in this PR
Refactors shared CLI setup and test fixtures while preserving command behavior, cancellation handling, and diagnostics.
Changes:
- Consolidates signal handling, scan-setting selection, output capture, and credential filtering.
- Reuses temporary-directory, mock, pagination, error, and publication fixtures.
- Strengthens command-level cancellation and diagnostic assertions.
| File | Description |
|---|---|
sdk/typescript/src/cli.ts |
Consolidates CLI setup and signal handling. |
sdk/typescript/src/cli-help.ts |
Simplifies option-label formatting. |
sdk/typescript/tests-ts/support/linear-pagination.ts |
Adds an empty-page fixture. |
sdk/typescript/tests-ts/cli-fixtures.ts |
Expands shared CLI fixtures and types. |
sdk/typescript/tests-ts/skeleton.test.ts |
Reuses output capture fixtures. |
sdk/typescript/tests-ts/mock-scan.test.ts |
Reuses mocks and temporary directories. |
sdk/typescript/tests-ts/linear.test.ts |
Reuses pagination helpers. |
sdk/typescript/tests-ts/github.test.ts |
Replaces manual tracking with mocks. |
sdk/typescript/tests-ts/cli-workbench.test.ts |
Consolidates workbench fixtures and assertions. |
sdk/typescript/tests-ts/cli-verify-fix.test.ts |
Shares Linear and mock fixtures. |
sdk/typescript/tests-ts/cli-suggest-owners.test.ts |
Shares directories and adds error coverage. |
sdk/typescript/tests-ts/cli-skills.test.ts |
Reuses directory, error, and invocation fixtures. |
sdk/typescript/tests-ts/cli-signals.test.ts |
Reuses security and error fixtures. |
sdk/typescript/tests-ts/cli-serve.test.ts |
Reuses stream capture. |
sdk/typescript/tests-ts/cli-scan-prompts.test.ts |
Reuses directories and mocks. |
sdk/typescript/tests-ts/cli-scan-import.test.ts |
Simplifies failures and diagnostic assertions. |
sdk/typescript/tests-ts/cli-publish.test.ts |
Consolidates publication fixtures and mocks. |
sdk/typescript/tests-ts/cli-project-config.test.ts |
Shares client, directory, and security fixtures. |
sdk/typescript/tests-ts/cli-policy.test.ts |
Consolidates policy fixtures and mocks. |
sdk/typescript/tests-ts/cli-patch.test.ts |
Reuses patch, Git, pagination, and directory helpers. |
sdk/typescript/tests-ts/cli-patch-trust.test.ts |
Reuses temporary-directory creation. |
sdk/typescript/tests-ts/cli-patch-results.test.ts |
Extracts Node-backed Codex fixture setup. |
sdk/typescript/tests-ts/cli-launcher.test.ts |
Reuses temporary-directory creation. |
sdk/typescript/tests-ts/cli-history-paths.test.ts |
Uses mock call assertions. |
sdk/typescript/tests-ts/cli-feedback.test.ts |
Reuses directory and error fixtures. |
sdk/typescript/tests-ts/cli-export.test.ts |
Shares scan-copy, directory, and mock helpers. |
sdk/typescript/tests-ts/cli-diagnostics.test.ts |
Reuses security and warning fixtures. |
sdk/typescript/tests-ts/cli-deep-scan-summary.test.ts |
Uses shared errors and mocks. |
sdk/typescript/tests-ts/cli-dedupe.test.ts |
Simplifies failure and invocation assertions. |
sdk/typescript/tests-ts/cli-cloud-publish.test.ts |
Consolidates publication setup and mocks. |
sdk/typescript/tests-ts/cli-classify-severity.test.ts |
Strengthens exact diagnostic assertions. |
sdk/typescript/tests-ts/cli-authentication.test.ts |
Reuses authentication, security, and JSON fixtures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CLI commands repeat option and signal-handler setup. Share those paths and their fixtures while preserving the current command surface and output.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 635f52367b486ea18d20d0f089daf92510700700 against its own base aa2b7867856b46440c6b671cb29f2f21a43b700d with exactly three independent high-reasoning full-diff reviews, including every test diff, plus root verification. No serious introduced code defect identified.
857 focused SDK tests passed (1 skipped); fresh native host build, plugin/SDK builds, types and formatting passed.
Local validation was on macOS arm64. No model execution, local Windows run or full installed-package smoke. Reviewed sources remained unchanged. This review covers this exact head; it does not establish combined-stack validation against current main.
635f523 to
474d325
Compare
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # sdk/typescript/src/security-policy.ts # sdk/typescript/tests-ts/api-policy.test.ts # sdk/typescript/tests-ts/api-post-scan.test.ts # sdk/typescript/tests-ts/component-scan.test.ts # sdk/typescript/tests-ts/security-policy.test.ts
# Conflicts: # sdk/typescript/src/cli.ts # sdk/typescript/tests-ts/cli-export.test.ts
# Conflicts: # sdk/typescript/tests-ts/support/shell.ts
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 10ef46c06ca8917452e538cfb36932fe7a968960 against 33746cd2e1766cf658fffa72d8adf7a4c6422828. Exactly three fresh independent HIGH full-diff reviews, including every test diff, plus root verification are complete. No serious introduced product finding remains.
Approval is withheld while inherited installed-package/container checks fail on the cleanup assertion described in #1207 (package-behavior.mjs:202). This is a base fixture/cleanup-contract failure at this head, not a new finding in this PR contribution. Correct it upstream and rerun hosted checks.
Local validation: 862 SDK tests passed,1 skipped. Fresh macOS arm64 native host, plugin and SDK builds, types and formatting passed. Reviewed sources are unchanged. No model execution, local Windows run or full installed-package smoke. Source review is complete; merge readiness is not established.
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed ec065c2e986cab1c532437abb3fab546fea230cb against d519e01451b45a46ea2e9b6d3fd28489f8089d99. All 33 ordered file contributions and touched base/head blobs match the previously reviewed contribution, so the three independent full-diff passes and 862 passed / 1 skipped SDK validation are retained without duplicate reviews or tests. Root checked the inherited package lifecycle fixture change and the exact current GitHub diff; no serious introduced findings remain.
The inherited fixture passes against the #1207 production tarball with existing exact-version dependencies. This is not a clean installed-package smoke of this head; clean installation is blocked by unavailable registry versions. No model execution, local Windows run, or source edits.
This approves the reviewed source. Current-head hosted CI remains unfinished, and approval does not establish merge readiness.
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for 5891e200a5dcaa6ff6fe1635f572740a5f2929fc against dff043688e8e0d7cb9079525eec82ab61eed195f.
Retained exactly three independent HIGH full-diff review passes, including every test diff. Independently verified the actual GitHub three-dot diff, identical ordered contributions, all 33 touched paths at both base and head (bytes and modes), and the three historical report hashes. No actionable introduced source finding remains. No additional full source pass or local test was run for this restack; prior validation retains its original revision, execution time, and limitations.
Hosted CI was still unfinished at the separate 2026-10-04 05:14 UTC observation.
This is source approval, not a merge-readiness or combined-integration claim. No fresh model execution, local Windows validation, native rebuild, or clean installed-package smoke test is claimed.
Summary
CLI commands repeat option and signal-handler setup. Share those paths and their fixtures while preserving the current command surface and output.
Changes
Testing
Risk and rollout
Commands, flags, accepted values and defaults do not change. SIGINT/SIGTERM exit behavior and raw error text remain covered by the command-level checks.
Targets
main, withdff04368merged. The original refactor contribution is unchanged; conflicts from the landed parent PRs are resolved.Public disclosure review