Repository navigation
refactor(ci): consolidate build and tool setup - #1204
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. |
|
Codex Review: Didn't find any major issues. Hooray! 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
The shared action affects multiple platforms and architectures, while hosted Windows/macOS validation remains pending.
Review effort: Balanced
Findings: None
What changed in this PR
Consolidates repeated CI tool setup into a shared composite action and simplifies supporting build, package, and test code.
Changes:
- Reuses pinned pnpm and Node setup across workflows, preserving architecture and shell choices.
- Includes the shared action in sparse checkout and CI path-selection coverage.
- Simplifies compiler configuration, package inventory checks, and temporary-directory cleanup.
| File | Description |
|---|---|
| sdk/typescript/tsconfig.json | Removes redundant compiler settings and exclusions. |
| sdk/typescript/tsconfig.build.json | Removes redundant build exclusions. |
| sdk/typescript/tests-ts/test-reports.test.ts | Reuses temporary-directory cleanup helper. |
| sdk/typescript/tests-ts/container-ci-workflow.test.ts | Tests shared-action change triggers. |
| sdk/typescript/tests-ts/build-plugin.test.ts | Reuses temporary-directory cleanup helper. |
| sdk/typescript/scripts/compare-test-reports.mts | Simplifies report-matching expression. |
| sdk/typescript/scripts/check-plugin-source.mjs | Uses import.meta.dirname. |
| sdk/typescript/scripts/check-package.mjs | Consolidates allowed package entries. |
| sdk/typescript/scripts/build-plugin.mjs | Simplifies directory resolution and path validation. |
| plugins/codex-security/mcp-app/tsconfig.json | Removes redundant settings and formats includes. |
| .github/workflows/test-quality.yml | Adopts shared tool setup. |
| .github/workflows/node-ci.yml | Adopts shared setup and updates checkout and triggers. |
| .github/workflows/native-windows.yml | Delegates setup and compilation with explicit architecture. |
| .github/workflows/native-unix.yml | Delegates setup and compilation. |
| .github/workflows/native-musl.yml | Delegates setup and compilation. |
| .github/scripts/invoice-desk-target.mjs | Combines filtering and mapping. |
| .github/actions/setup-tools/action.yml | Adds shared setup with optional compilation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CI workflows repeat package-manager, Node and build setup. Move the repeated steps into a local composite action and simplify equivalent build and package checks.
3437595 to
7b2d678
Compare
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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
There was a problem hiding this comment.
Reviewed 7b2d678a93cdeea4db9496bfa557a13c5a88d724 against 775e48ffbf682075e60303eb019131aa2390c3b2. No serious introduced code findings remain. I am withholding approval until the new-head hosted CI finishes successfully.
All 17 files retain the same ordered added/removed PR contributions as the previous reviewed head, so the three independent full-diff reviews, including all test changes, remain applicable. I separately verified the one inherited touched-file change: the package allowlist entry from #1133.
Fresh validation on this head passed: native host build, plugin and SDK builds, types, formatting, 32 focused SDK tests, Ruff 0.16.9, portable source checks and their 9 tests, and the production package archive check (540 entries). All 801 tracked source blobs still match this head. Local platform was macOS arm64; no model execution, local Windows run, or installed-package smoke test.
The earlier head's five Ubuntu parallel-test timeouts remain unexplained; all five unchanged targeted local controls passed. That historical failure does not describe this head's CI. Current node-ci run 37152559999, attempt 1, is still unfinished.
alandelong-oai
left a comment
There was a problem hiding this comment.
Approving exact head 7b2d678 now that its hosted CI has completed successfully. The prior CI-only hold is cleared; node-ci attempt 1 passed with 66 successful jobs and one skipped job.
The three independent full-diff reviews, including all tests, remain applicable: all 17 ordered file contributions are unchanged, and the inherited package allowlist change was reconciled. Root verification found no serious introduced issue. Fresh validation at this head passed 32 focused SDK tests, nine source-check tests, builds, types, formatting, Ruff and a 540-entry production package archive. Reviewed source files remain unchanged. No duplicate reviews or test runs were needed for this CI completion; no installed-package smoke, model execution or local Windows run was performed.
Summary
CI workflows repeat package-manager, Node and build setup. Move the repeated steps into a local composite action and simplify equivalent build and package checks.
Changes
Testing
On rebased head
7b2d678a, Bun 1.4.2 workflow/report tests passed (38 cases) and build/package-safety tests passed (21). Ruff check and format, CI compilation, portable plugin source checks, SDK/MCP types, the normal formatter, SDK build, npm pack, and archive validation passed (540 entries). Installed-package smoke passed on Node 22.13.0 with a private temporary root. Hosted checks for this rebased head have not run yet.Earlier head
34375955passed Windows Node 22/24 verification and test shards, macOS test shards and installed-package smoke, standalone plugin checks on all three operating systems, all eight native build targets, package checks, source contracts, types, and workflow checks. Its Linux parallel test-quality job failed on 30-second timeouts in both the initial run and one retry, on disjoint sets of unchanged tests; the dependent inventory comparison correctly failed. These hosted results belong to the earlier head.Risk and rollout
The shared action affects multiple hosted platforms. The rebase preserves the patch's additions and deletions and includes the current package inventory from
main. Fresh hosted checks, including Linux parallel test quality, must pass before merging. Test timeouts, parallelism, and quality gates are unchanged.This patch has no source prerequisite within the series. Squash-merge it into
mainafter review and checks pass.Public disclosure review