Skip to content

fix(ci): combined CI workarounds (#697 + #698)#699

Merged
tamirdresher merged 1 commit intodevfrom
squad/fix-ci-combined-697-698
Mar 30, 2026
Merged

fix(ci): combined CI workarounds (#697 + #698)#699
tamirdresher merged 1 commit intodevfrom
squad/fix-ci-combined-697-698

Conversation

@diberry
Copy link
Copy Markdown
Collaborator

@diberry diberry commented Mar 30, 2026

Summary

Consolidates PRs #697 and #698, which both modify .github/workflows/squad-ci.yml to fix the same root cause: @github/copilot-sdk's ESM import of vscode-jsonrpc/node (missing .js extension) breaking CI on Linux runners.

What each PR did

PR Approach Effect
#697 Skip workaround — added SKIP_SAMPLES (streaming-chat) and SKIP_EXPORTS (./client) lists to bypass the failing tests Tests skipped entirely
#698 Proper fix — run the existing patch-esm-imports.mjs script after npm ci --ignore-scripts Root cause patched, all tests run

Resolution

#698's approach wins. The repo already has packages/squad-cli/scripts/patch-esm-imports.mjs (added for issue #449) that patches the missing ESM extension. It runs as a postinstall hook, but CI uses npm ci --ignore-scripts which skips lifecycle hooks. Adding an explicit node packages/squad-cli/scripts/patch-esm-imports.mjs step after npm ci in both affected jobs fixes the actual problem — making #697's skip-based workaround unnecessary.

What changed

.github/workflows/squad-ci.yml — 2 lines added:

  1. samples-build job (line ~354): node packages/squad-cli/scripts/patch-esm-imports.mjs after npm ci --ignore-scripts
  2. export-smoke-test job (line ~725): Same patch step after npm ci --ignore-scripts

Root cause

@github/copilot-sdk@0.1.32's dist/session.js imports vscode-jsonrpc/node without a .js extension. Since vscode-jsonrpc@8.2.1 has no exports field, Node.js 22+ strict ESM resolution fails. The existing patch script injects the missing exports field into vscode-jsonrpc/package.json.

Re-enable conditions

Remove the patch steps when @github/copilot-sdk ships ESM-compliant imports (i.e., vscode-jsonrpc/node.js instead of vscode-jsonrpc/node). Upstream: github/copilot-sdk#707

Why consolidate

Both PRs touch the same file with overlapping intent. Shipping them separately would create a confusing git history and the skip workaround from #697 would immediately become dead code once #698's fix lands.

Refs: #697, #698

Combines two CI fixes that both modify squad-ci.yml:
- #697: Skip ./client export smoke test + streaming-chat sample
  (workaround for @github/copilot-sdk ESM bug — vscode-jsonrpc/node)
- #698: Run existing patch-esm-imports.mjs after npm ci --ignore-scripts
  (proper fix — patches missing ESM extension at the source)

Resolution: #698's approach (running the patch script) fixes the root
cause, making #697's skip-based workaround unnecessary. The existing
patch-esm-imports.mjs (added for issue #449) was already solving this
problem but was skipped in CI because npm ci --ignore-scripts bypasses
the postinstall hook that runs it.

Both changes target the same workflow file and should ship together.

Refs: #697, #698

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Consolidates prior CI mitigations into the workflow by explicitly running the existing ESM patch script after npm ci --ignore-scripts, addressing Node 22+ ESM resolution failures caused by @github/copilot-sdk importing vscode-jsonrpc/node without a .js extension.

Changes:

  • Run packages/squad-cli/scripts/patch-esm-imports.mjs in samples-build after dependency install.
  • Run the same patch step in export-smoke-test after dependency install.

@diberry
Copy link
Copy Markdown
Collaborator Author

diberry commented Mar 30, 2026

✅ CI Review: APPROVED

Verdict: Correct consolidation. The ESM patch is properly placed and applied uniformly.

Workflow Correctness

ESM patch placement is correct. The patch runs immediately after
pm ci --ignore-scripts\ and before downstream tasks (
pm run build), allowing it to fix the vscode-jsonrpc import issue before TypeScript compilation attempts to resolve it.

Job Coverage

Both affected jobs patched consistently:

  • \samples-build\ job: patch added at line 354
  • \�xport-smoke-test\ job: patch added at line 724

No gaps. Both jobs have identical CI sequence:
pm ci --ignore-scripts\ →
ode packages/squad-cli/scripts/patch-esm-imports.mjs\ → build/test.

Idempotency

Patch is idempotent. The \patch-esm-imports.mjs\ script (packages/squad-cli/scripts/patch-esm-imports.mjs) modifies vscode-jsonrpc's package.json to inject missing \�xports\ field. Running multiple times writes the same value—safe.

File Scope

Only squad-ci.yml modified. Diff shows 2 lines added (both patch steps), 0 .squad files touched. Clean scope.

Note: This consolidation eliminates the skip-based workaround (#697 approach), allowing all tests to run. The upstream issue (#707 in copilot-sdk) will determine when to remove the patch step. 🛡️

@diberry
Copy link
Copy Markdown
Collaborator Author

diberry commented Mar 30, 2026

FIDO's Quality Review ✓

Verdict: APPROVED — Sound consolidation with correct root-cause fix.

Correctness ✅

The ESM patch approach is solid. PR #698's strategy (run existing \patch-esm-imports.mjs\ after
pm ci --ignore-scripts) correctly addresses the root cause:

  • @github/copilot-sdk@0.1.32\ imports \�scode-jsonrpc/node\ without .js\ extension
  • \�scode-jsonrpc@8.2.1\ lacks an \�xports\ field, breaking Node 22+ strict ESM resolution
  • The patch script injects proper exports + defense-in-depth rewrite of the import
  • Existing script already used in postinstall; CI bypass requires explicit invocation ✓

Scope ✅

Minimal and correct. Only .github/workflows/squad-ci.yml\ modified:

Regression Risk ✅ (Low)

Supersession ✓

Both PRs properly closed:

Edge Cases Considered ✓

Sign-off: Ready to merge. No test coverage concerns — existing tests now run unblocked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants