Repository navigation
refactor(sdk): share progress and event parsing - #1209
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. 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". |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Final approval should follow the required restacking onto main and reconciliation of overlapping helpers with #1210.
Review effort: Balanced
Findings: None
What changed in this PR
Refactors SDK parsing, progress, cost, and presentation code while preserving behavior.
Changes:
- Adds shared JSON and integer-validation helpers.
- Simplifies progress, cost, dashboard, history, and CLI rendering.
- Consolidates test fixtures, mocks, and JSON-lines writing.
| File | Description |
|---|---|
sdk/typescript/src/value.ts |
Adds shared JSON parsing. |
sdk/typescript/src/worker-progress.ts |
Reuses parsing and integer helpers. |
sdk/typescript/src/deep-progress.ts |
Consolidates progress validation. |
sdk/typescript/src/cost-model.ts |
Reuses token-count validation. |
sdk/typescript/src/cost.ts |
Simplifies progress aggregation. |
sdk/typescript/src/scan-activity.ts |
Reuses JSON parsing and simplifies extraction. |
sdk/typescript/src/scan-dashboard.ts |
Simplifies dashboard formatting. |
sdk/typescript/src/scan-history-renderer.ts |
Simplifies configuration rendering. |
sdk/typescript/src/patch-tui.tsx |
Simplifies detail formatting. |
sdk/typescript/src/cli.ts |
Simplifies summary and progress presentation. |
sdk/typescript/tests-ts/support/json.ts |
Adds JSON-lines fixture writer. |
sdk/typescript/tests-ts/scan-logs.test.ts |
Reuses fixture helpers. |
sdk/typescript/tests-ts/cli-scan-logs.test.ts |
Reuses JSON and error helpers. |
sdk/typescript/tests-ts/cost.test.ts |
Reuses JSON-lines and directory helpers. |
sdk/typescript/tests-ts/deep-progress.test.ts |
Uses mocks and abort-event utilities. |
sdk/typescript/tests-ts/findings-dashboard.test.ts |
Consolidates timer and promise fixtures. |
sdk/typescript/tests-ts/scan-dashboard.test.ts |
Consolidates mocks and failing output fixtures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Progress, activity and cost paths repeat scalar and JSON parsing. Reuse internal helpers and simplify presentation without changing the rendered diagnostics or cost arithmetic.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed ce53558401667b309c940df97bd38e872f7a1e3f 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.
177 focused SDK tests passed; 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.
ce53558 to
5121708
Compare
|
Codex Review: Didn't find any major issues. Keep them coming! 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/tests-ts/support/shell.ts
|
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.
Source review approved for e4a7c08a6e44aef094cfcd475af9713d6e2a6d3f against d519e01451b45a46ea2e9b6d3fd28489f8089d99.
Completed exactly three fresh independent HIGH reasoning passes over the full PR diff and every test diff, followed by independent synthesis. No actionable introduced finding remains.
Fresh validation passed the native host and SDK/plugin builds, TypeScript and formatting checks, and 177 focused SDK tests.
Hosted CI was still pending at the latest check (2026-10-03T23:28:38.342694+00:00), with no failed jobs observed on this head. This source approval is not a statement that the PR is ready to merge. No model execution, local Windows validation, or full installed-package smoke test is claimed.
|
Codex Review: Didn't find any major issues. Chef's kiss. 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 a2d27594d72aa6d8642b8bd75887a15003089915 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 17 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 at the 2026-10-04 05:14 UTC observation was unfinished and contained a failed macOS shard: CodexSecurity policy API > does not load instructions from the artifact checkout timed out after 30 seconds (1013 passed, 25 skipped, 1 failed). The complete test file is unchanged at the prior reviewed head, current head, and base. The cause is unknown; neither an introduced source defect nor flakiness has been established. See https://github.com/openai/codex-security/actions/runs/37178778995/job/111367588338.
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.
|
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.
Source review approved for 12d8fa3d206eaec18c3bf313c165f49de09dd3b5 against base 634532d7f7fb21ccca49b4bca9bb9fa1df9308d6.
Completed exactly three fresh independent HIGH reviews of the full actual GitHub PR diff: 16 touched files, including all six changed test/support paths, plus root synthesis. The changed contribution supersedes the previous source reviews. No supported introduced finding remains.
Fresh focused validation: all five changed SDK test files passed (174 tests); plugin/SDK builds, type checks, formatting, production build and archive check passed. Root verified all 814 tracked source entries and all 540 archive members against the saved validation tree. Fourteen native artifacts were retained after executable source, dependency and artifact identity checks; no native compilation was run. Initial validation preparation rejected a README-only native difference before product execution; correcting that evidence check did not require rerunning tests.
The separate 08:33 UTC exact-head CI observation remains pending, with no failed jobs observed. The earlier macOS timeout belongs to the old a2d27594 head; its cause remains unknown and is not assigned to this head. Source approval and these focused checks do not establish completed CI, whole-stack integration, clean installed-package smoke, local Windows validation or model behavior.
Summary
Progress, activity and cost paths repeat scalar and JSON parsing. Reuse internal helpers and simplify presentation without changing the rendered diagnostics or cost arithmetic.
Changes
Testing
Risk and rollout
Raw diagnostics, terminal escaping, event handling and cost totals remain compatibility boundaries. This patch owns the CLI presenter changes; command plumbing belongs to the separate CLI follow-up.
Targets
main, with634532d7merged. Shared-helper conflicts are resolved by retaining the functions required by both parents. The resulting whole source tree is identical to the previously validated combined integration tree.The related CLI, findings, publication, worker-launch, coordinator and CI follow-ups have landed. Their changes remain intact.
Public disclosure review