Skip to content

fix(deps): resolve vscode-jsonrpc/node module resolution for @github/copilot-sdk#698

Closed
diberry wants to merge 2 commits intodevfrom
squad/fix-vscode-jsonrpc-ci
Closed

fix(deps): resolve vscode-jsonrpc/node module resolution for @github/copilot-sdk#698
diberry wants to merge 2 commits intodevfrom
squad/fix-vscode-jsonrpc-ci

Conversation

@diberry
Copy link
Copy Markdown
Collaborator

@diberry diberry commented Mar 30, 2026

Root Cause Analysis

\@github/copilot-sdk@0.1.32\'s \dist/session.js\ imports \�scode-jsonrpc/node\ without a \.js\ extension. Since \�scode-jsonrpc@8.2.1\ ships without an \�xports\ field in its \package.json\, Node.js 22+ strict ESM resolution cannot resolve the subpath import — it requires either an explicit \�xports\ map or a full file extension.

The repo already has a fix for this: \packages/squad-cli/scripts/patch-esm-imports.mjs\ (added for issue #449). This script injects the missing \�xports\ field into \�scode-jsonrpc/package.json\ and patches the SDK import as defense-in-depth. It runs as a \postinstall\ hook on \squad-cli\.

However, the CI workflows use \
pm ci --ignore-scripts\, which skips all lifecycle hooks — including the postinstall that runs the patch. The patch never executes in CI, so the ESM resolution fails.

Solution

Add an explicit \
ode packages/squad-cli/scripts/patch-esm-imports.mjs\ step after \
pm ci --ignore-scripts\ in both affected CI jobs:

  1. samples-build — validates sample projects build against the SDK
  2. export-smoke-test — dynamically imports all SDK subpath exports

This is a 2-line change. No new scripts, no dependency changes, no version bumps. We reuse the existing battle-tested patch.

Impact

Unblocks:

  • \samples-build\ CI job (broken since ~Mar 29 14:00 UTC)
  • \�xport-smoke-test\ CI job (same root cause, would fail on subpaths that transitively load \�scode-jsonrpc/node\)

Testing

Verified locally on Node.js 22:
\\

Before patch — fails

import('vscode-jsonrpc/node')
FAIL: Cannot find module 'vscode-jsonrpc/node'

After running patch-esm-imports.mjs — succeeds

import('vscode-jsonrpc/node')
OK: ['AbstractMessageBuffer', 'AbstractMessageReader', ...]
\\

Upstream: github/copilot-sdk#707

diberry and others added 2 commits March 29, 2026 18:19
Scribe operations complete:

1. ORCHESTRATION LOG: Filed 9 agent verdict summaries at
   .squad/orchestration-log/2026-03-30T00-46-prd120-review/

   - Flight: PRD delivered, ready for team review
   - EECOM: APPROVE WITH CHANGES (defer CI gate to v0.10.1, 18h scope)
   - Booster: APPROVED WITH CONDITIONS (state machine parsing, 80-120h)
   - Procedures: APPROVED WITH TIER 1 GUARDRAILS (3 blockers for Flight)
   - FIDO: CONDITIONAL GO (54-72 tests required)
   - RETRO: CONDITIONAL APPROVAL (4 security checkpoints)
   - Surgeon: APPROVED WITH CONDITIONS (changelog automation)
   - PAO: APPROVED WITH RECOMMENDATIONS (4 doc pages required)
   - Network: APPROVED (zero-dependency maintained)

2. SESSION LOG: Recorded team review summary at
   .squad/log/2026-03-30T00-46-prd120-review.md

3. DECISION MERGE: Merged 12 inbox files to decisions.md:
   - CI deletion guard + Copilot git safety rules (2026-03-26)
   - Versioning policy (2026-03-29)
   - PRD-120 review verdicts (2026-03-30)
   Deleted inbox files after merge.

4. CROSS-AGENT HISTORY: Appended team update 📌 entries to:
   - flight/history.md
   - eecom/history.md
   - booster/history.md
   - procedures/history.md
   - fido/history.md
   - retro/history.md
   - surgeon/history.md
   - pao/history.md
   - network/history.md

Tier 1 blockers for Flight decision:
  1. Define schedule.json template location
  2. Confirm upgrade confirmation flow
  3. Update sync-templates.mjs for new templates

Team cleared for implementation phase.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The samples-build and export-smoke-test CI jobs use npm ci --ignore-scripts,
which skips squad-cli's postinstall hook that patches vscode-jsonrpc for Node
22+ strict ESM resolution. @github/copilot-sdk@0.1.32 imports
'vscode-jsonrpc/node' (no .js extension), but vscode-jsonrpc@8.2.1 has no
exports field, so ESM resolution fails.

Fix: explicitly run the existing patch-esm-imports.mjs script after npm ci
in both CI jobs. The script injects the missing exports map into
vscode-jsonrpc/package.json, making all subpath imports resolve correctly.

Ref: #449

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 30, 2026 04:01
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

This PR addresses Node 22+ strict ESM resolution failures affecting @github/copilot-sdk consumption by ensuring the existing patch-esm-imports.mjs workaround runs in CI despite npm ci --ignore-scripts. It also includes a large set of .squad/ decision/history log updates.

Changes:

  • Run node packages/squad-cli/scripts/patch-esm-imports.mjs after npm ci --ignore-scripts in the samples-build and export-smoke-test CI jobs.
  • Consolidate several inbox decision entries into .squad/decisions.md and delete the corresponding inbox files.
  • Add PRD-120 review summary entries across multiple agent history logs.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
.github/workflows/squad-ci.yml Adds explicit invocation of the ESM patch script in CI jobs that build/import the SDK.
.squad/decisions.md Adds a “Recent Reviews & Decisions” section consolidating prior inbox entries.
.squad/decisions/inbox/retro-copilot-git-safety.md Deleted after consolidation into decisions.md.
.squad/decisions/inbox/flight-versioning-policy.md Deleted after consolidation into decisions.md.
.squad/decisions/inbox/booster-ci-deletion-guard.md Deleted after consolidation into decisions.md.
.squad/agents/surgeon/history.md Adds PRD-120 review summary entry (contains future-dated section).
.squad/agents/retro/history.md Adds PRD-120 review summary entry (contains future-dated section).
.squad/agents/procedures/history.md Adds PRD-120 review summary entry (contains future-dated section).
.squad/agents/pao/history.md Adds PRD-120 review summary entry (contains future-dated section).
.squad/agents/network/history.md Adds PRD-120 review summary entry (contains future-dated section).
.squad/agents/flight/history.md Adds PRD-120 review summary entry (contains future-dated section + minor formatting).
.squad/agents/fido/history.md Adds PRD-120 review summary entry (contains future-dated section + minor formatting).
.squad/agents/eecom/history.md Adds PRD-120 review summary entry (contains future-dated section).
.squad/agents/booster/history.md Adds PRD-120 review summary entry (contains future-dated section).

📌 **Team update (2026-03-26T06:41:00Z — Crash Recovery Execution & Community PR Review):** Post-CLI crash recovery completed: Round 1 baseline verified (5,038 tests ✅ green), Round 2 executed duplicate closures (#605/#604/#602) and 9-PR community batch review. FIDO approved 3 PRs (#625 notification-routing, #603 Challenger agent, #608 security policy—merged via Coordinator) and issued change requests on 6 PRs identifying systemic issues: changeset package naming (4 PRs used unscoped `squad-cli` instead of `@bradygaster/squad-cli`); file paths (2 PRs placed files at root instead of correct package structure). Quality gate result: high-bar community acceptance—approved 3/9 (33%), change-request 6/9 (67%), 0 rejections. PR #592 (legacy, high-quality) also merged. All actions complete; dev branch remains green. Decision inbox merged and deleted. Next: Monitor 6 change-request PRs for author responses.
📌 **Team update (2026-03-30T00:46:00Z — PRD-120 Test Strategy Review Verdict: CONDITIONAL GO):** FIDO completed comprehensive test strategy review for PRD-120. Verdict: **CONDITIONAL GO** — approval contingent on test strategy implementation before code review begins. PRD architecturally sound with clear acceptance criteria; however, introduces significant testing complexity across init, upgrade, schedule, config, and CI workflows. Risk: subtle regressions in upgrade path, false positives in CI gate, feature flag sprawl, backward compatibility breaks. Conditions: GO on implementation IF test strategy matrix is committed before code starts; FAIL on merge IF PRs lack tests for the matrix; NO-GO on general availability IF regression tests don't confirm 80%+ coverage. Test matrix needed: 54–72 tests across 5 files: init.test.ts (12–15 tests, new install paths), upgrade.test.ts (18–20 tests, migration), schedule.test.ts (10–12 tests, enable/disable), features.test.ts (8–10 tests, flag overrides), ci-gate.integration.ts (6–8 tests, gate workflow). Backward compatibility: pre-existing test suite must pass at 80%+ coverage. Full review filed at `.squad/orchestration-log/2026-03-30T00-46-prd120-review/FIDO.md`. Decision merged to decisions.md.

📌 **Team update (2026-03-26T06:41:00Z — Crash Recovery Execution & Community PR Review):**Post-CLI crash recovery completed: Round 1 baseline verified (5,038 tests ✅ green), Round 2 executed duplicate closures (#605/#604/#602) and 9-PR community batch review. FIDO approved 3 PRs (#625 notification-routing, #603 Challenger agent, #608 security policy—merged via Coordinator) and issued change requests on 6 PRs identifying systemic issues: changeset package naming (4 PRs used unscoped `squad-cli` instead of `@bradygaster/squad-cli`); file paths (2 PRs placed files at root instead of correct package structure). Quality gate result: high-bar community acceptance—approved 3/9 (33%), change-request 6/9 (67%), 0 rejections. PR #592 (legacy, high-quality) also merged. All actions complete; dev branch remains green. Decision inbox merged and deleted. Next: Monitor 6 change-request PRs for author responses.
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

Minor formatting: there’s no space after the bold marker ("...Review):**Post-CLI..."). Add a space after :** for readability and consistency with other history entries.

Suggested change
📌 **Team update (2026-03-26T06:41:00Z — Crash Recovery Execution & Community PR Review):**Post-CLI crash recovery completed: Round 1 baseline verified (5,038 tests ✅ green), Round 2 executed duplicate closures (#605/#604/#602) and 9-PR community batch review. FIDO approved 3 PRs (#625 notification-routing, #603 Challenger agent, #608 security policy—merged via Coordinator) and issued change requests on 6 PRs identifying systemic issues: changeset package naming (4 PRs used unscoped `squad-cli` instead of `@bradygaster/squad-cli`); file paths (2 PRs placed files at root instead of correct package structure). Quality gate result: high-bar community acceptance—approved 3/9 (33%), change-request 6/9 (67%), 0 rejections. PR #592 (legacy, high-quality) also merged. All actions complete; dev branch remains green. Decision inbox merged and deleted. Next: Monitor 6 change-request PRs for author responses.
📌 **Team update (2026-03-26T06:41:00Z — Crash Recovery Execution & Community PR Review):** Post-CLI crash recovery completed: Round 1 baseline verified (5,038 tests ✅ green), Round 2 executed duplicate closures (#605/#604/#602) and 9-PR community batch review. FIDO approved 3 PRs (#625 notification-routing, #603 Challenger agent, #608 security policy—merged via Coordinator) and issued change requests on 6 PRs identifying systemic issues: changeset package naming (4 PRs used unscoped `squad-cli` instead of `@bradygaster/squad-cli`); file paths (2 PRs placed files at root instead of correct package structure). Quality gate result: high-bar community acceptance—approved 3/9 (33%), change-request 6/9 (67%), 0 rejections. PR #592 (legacy, high-quality) also merged. All actions complete; dev branch remains green. Decision inbox merged and deleted. Next: Monitor 6 change-request PRs for author responses.

Copilot uses AI. Check for mistakes.
if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' && steps.changes.outputs.skip != 'true'
run: |
npm ci --ignore-scripts
node packages/squad-cli/scripts/patch-esm-imports.mjs
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

The patch script currently logs warnings and returns booleans, but it doesn’t fail the step if it can’t find/patch the dependency. Consider adding an explicit verification right after this line (e.g., a quick Node ESM import of vscode-jsonrpc/node or a check that node_modules/vscode-jsonrpc/package.json has an exports map) so CI fails fast with a clear message if the patch didn’t apply.

Suggested change
node packages/squad-cli/scripts/patch-esm-imports.mjs
node packages/squad-cli/scripts/patch-esm-imports.mjs
node -e "let pkg;try{pkg=require('vscode-jsonrpc/package.json');}catch(e){console.error('::error::Failed to load vscode-jsonrpc/package.json after patch; ensure vscode-jsonrpc is installed and the patch script ran successfully.');process.exit(1);}if(!pkg || !pkg.exports){console.error('::error::vscode-jsonrpc patch did not apply: missing \"exports\" map in node_modules/vscode-jsonrpc/package.json');process.exit(1);}console.log('Verified vscode-jsonrpc patch: exports map present in node_modules/vscode-jsonrpc/package.json.');"

Copilot uses AI. Check for mistakes.
📌 **Team update (2026-03-26T06:41:00Z — Crash Recovery Execution Complete):** Post-CLI crash recovery executed in 3 rounds. Round 1: Flight audited PR/issue state (found #617 merged, #619 conflicting, 3 dupes #605/#604/#602 open); FIDO verified baseline (5,038 tests ✅ green); Scribe merged stale inbox. Round 2: Flight closed 3 duplicate PRs with rationale; Procedures rebased PR #619 (model catalog) onto dev, resolved 3 merge conflicts, merged; FIDO reviewed 9 community PRs—approved 3 (#625/#603/#608), requested changes on 6 (package naming, file paths). Round 3: Coordinator merged 3 approved PRs. **10 PRs merged total** (6 merge-plan, 3 community, 1 legacy #592). **3 PRs closed** as duplicates. **6 PRs awaiting author revisions**. **Dev branch green** (5,038 tests). All merge-plan sequence complete. Draft #567 parked pending requirements. Decision inbox merged to decisions.md and deleted. Next: Monitor change-request PRs for author responses.
📌 **Team update (2026-03-30T00:46:00Z — PRD-120 Team Review Complete):** PRD-120 (Cron Disable, CI Gating, Feature Management) cleared for implementation by full team. 9 agents reviewed across 5 domains: runtime, CI/CD, templates, testing, security, release management, user communication, packaging. Verdicts: EECOM APPROVE WITH CHANGES (defer CI gate to v0.10.1, 18h scope), Booster APPROVED WITH CONDITIONS (state machine parsing, 80–120h), Procedures APPROVED WITH TIER 1 GUARDRAILS (3 blockers: template location, upgrade flow, sync script), FIDO CONDITIONAL GO (54–72 tests required), RETRO CONDITIONAL APPROVAL (4 security checkpoints), Surgeon APPROVED WITH CONDITIONS (changelog automation), PAO APPROVED WITH RECOMMENDATIONS (4 doc pages), Network APPROVED (zero-dependency maintained). Three Tier 1 blockers for Flight decision: (1) define schedule.json template location, (2) confirm upgrade confirmation flow, (3) update sync-templates.mjs. Orchestration logs filed at `.squad/orchestration-log/2026-03-30T00-46-prd120-review/`. Session log at `.squad/log/2026-03-30T00-46-prd120-review.md`. Decisions merged to decisions.md. Inbox cleared. Ready for implementation phase.

📌 **Team update (2026-03-26T06:41:00Z — Crash Recovery Execution Complete):**Post-CLI crash recovery executed in 3 rounds. Round 1: Flight audited PR/issue state (found #617 merged, #619 conflicting, 3 dupes #605/#604/#602 open); FIDO verified baseline (5,038 tests ✅ green); Scribe merged stale inbox. Round 2: Flight closed 3 duplicate PRs with rationale; Procedures rebased PR #619 (model catalog) onto dev, resolved 3 merge conflicts, merged; FIDO reviewed 9 community PRs—approved 3 (#625/#603/#608), requested changes on 6 (package naming, file paths). Round 3: Coordinator merged 3 approved PRs. **10 PRs merged total** (6 merge-plan, 3 community, 1 legacy #592). **3 PRs closed** as duplicates. **6 PRs awaiting author revisions**. **Dev branch green** (5,038 tests). All merge-plan sequence complete. Draft #567 parked pending requirements. Decision inbox merged to decisions.md and deleted. Next: Monitor change-request PRs for author responses.
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

Minor formatting: there’s no space after the bold marker ("...Complete):**Post-CLI..."). This reads like a typo in the rendered markdown; add a space after :** for consistency with other entries.

Suggested change
📌 **Team update (2026-03-26T06:41:00Z — Crash Recovery Execution Complete):**Post-CLI crash recovery executed in 3 rounds. Round 1: Flight audited PR/issue state (found #617 merged, #619 conflicting, 3 dupes #605/#604/#602 open); FIDO verified baseline (5,038 tests ✅ green); Scribe merged stale inbox. Round 2: Flight closed 3 duplicate PRs with rationale; Procedures rebased PR #619 (model catalog) onto dev, resolved 3 merge conflicts, merged; FIDO reviewed 9 community PRs—approved 3 (#625/#603/#608), requested changes on 6 (package naming, file paths). Round 3: Coordinator merged 3 approved PRs. **10 PRs merged total** (6 merge-plan, 3 community, 1 legacy #592). **3 PRs closed** as duplicates. **6 PRs awaiting author revisions**. **Dev branch green** (5,038 tests). All merge-plan sequence complete. Draft #567 parked pending requirements. Decision inbox merged to decisions.md and deleted. Next: Monitor change-request PRs for author responses.
📌 **Team update (2026-03-26T06:41:00Z — Crash Recovery Execution Complete):** Post-CLI crash recovery executed in 3 rounds. Round 1: Flight audited PR/issue state (found #617 merged, #619 conflicting, 3 dupes #605/#604/#602 open); FIDO verified baseline (5,038 tests ✅ green); Scribe merged stale inbox. Round 2: Flight closed 3 duplicate PRs with rationale; Procedures rebased PR #619 (model catalog) onto dev, resolved 3 merge conflicts, merged; FIDO reviewed 9 community PRs—approved 3 (#625/#603/#608), requested changes on 6 (package naming, file paths). Round 3: Coordinator merged 3 approved PRs. **10 PRs merged total** (6 merge-plan, 3 community, 1 legacy #592). **3 PRs closed** as duplicates. **6 PRs awaiting author revisions**. **Dev branch green** (5,038 tests). All merge-plan sequence complete. Draft #567 parked pending requirements. Decision inbox merged to decisions.md and deleted. Next: Monitor change-request PRs for author responses.

Copilot uses AI. Check for mistakes.
if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' && steps.changes.outputs.skip != 'true'
run: |
npm ci --ignore-scripts
node packages/squad-cli/scripts/patch-esm-imports.mjs
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

Same as the samples-build job: because patch-esm-imports.mjs does not exit non-zero when it can’t patch anything, this step can silently succeed and leave the later build/import failure to surface with a less targeted error. Add a small verification right after the patch step to ensure vscode-jsonrpc/node (and/or the patched exports) is actually resolvable under Node 22 ESM rules.

Suggested change
node packages/squad-cli/scripts/patch-esm-imports.mjs
node packages/squad-cli/scripts/patch-esm-imports.mjs
node --input-type=module -e "import 'vscode-jsonrpc/node';"

Copilot uses AI. Check for mistakes.
Comment on lines 351 to 356
if: steps.flag.outputs.skip == 'false' && steps.label.outputs.skip != 'true' && steps.changes.outputs.skip != 'true'
run: |
npm ci --ignore-scripts
node packages/squad-cli/scripts/patch-esm-imports.mjs
npm run build -w packages/squad-sdk

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

The PR description/title describes a narrow CI fix (“2-line change” to run the existing patch script), but this PR also includes substantial updates to .squad/decisions.md and multiple agent history.md files. To keep review risk and revert scope low, consider splitting the .squad/ documentation/history updates into a separate PR or updating the PR description to explicitly cover them.

Copilot uses AI. Check for mistakes.
diberry added a commit that referenced this pull request Mar 30, 2026
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>
@diberry
Copy link
Copy Markdown
Collaborator Author

diberry commented Mar 30, 2026

Superseded by #699 — consolidated with #697. Both PRs touch squad-ci.yml for the same root cause; shipping as one clean PR.

@diberry diberry closed this Mar 30, 2026
tamirdresher pushed a commit that referenced this pull request Mar 30, 2026
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>
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.

2 participants