Skip to content

[go] Add test CLI setup helper - #2478

Draft
qmuntal wants to merge 3 commits into
github:mainfrom
qmuntal:go/testcli-setup
Draft

[go] Add test CLI setup helper#2478
qmuntal wants to merge 3 commits into
github:mainfrom
qmuntal:go/testcli-setup

Conversation

@qmuntal

@qmuntal qmuntal commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem

Downstream Go modules need a compatible Copilot CLI to run integration tests, but the Go SDK does not install one as a module dependency. Tests otherwise have to provision and version the CLI themselves or depend on this repository's Node workspace and its installed node_modules. That makes a normal go test workflow harder to reproduce outside the SDK monorepo.

Summary

  • add testcli.Setup for downstream integration tests to resolve and cache the platform CLI package compatible with the Go SDK
  • download npm package tarballs directly in Go with SHA-512 integrity verification and safe extraction, including the native runtime.node library used by in-process tests
  • share the registry download and extraction helpers with the bundler
  • document the helper and correct Go CLI path resolution guidance

Usage

Call testcli.Setup from the downstream test package's TestMain before running tests:

package integration_test

import (
    "fmt"
    "os"
    "testing"

    "github.com/github/copilot-sdk/go/testcli"
)

func TestMain(m *testing.M) {
    if err := testcli.Setup(); err != nil {
        fmt.Fprintf(os.Stderr, "setting up Copilot CLI: %v\n", err)
        os.Exit(1)
    }
    os.Exit(m.Run())
}

Setup honors an existing COPILOT_CLI_PATH. Otherwise, it resolves the CLI package pinned by the SDK version, verifies and caches it, and sets COPILOT_CLI_PATH for the test process.

Testing

  • go test ./testcli ./internal/npmregistry ./cmd/bundler -count=1
  • go vet ./testcli ./internal/npmregistry ./cmd/bundler

E2E and replay-harness tests were not run.

@qmuntal
qmuntal requested a review from a team as a code owner September 2, 2026 08:11
Copilot AI balanced review requested due to automatic review settings September 2, 2026 08:11
Comment thread go/testcli/setup.go Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The temporary-directory fallback permits cache poisoning, and the primary installation workflow lacks coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 High severity · 1 Low severity

New issues introduced by this change (2)
Severity Finding
High severity go/​testcli/​setup.go — A non-executable regular file is accepted here, so Setup reports success but the SDK later passes…
Low severity go/​testcli/​setup.go — The current Setup tests always provide COPILOT_CLI_PATH, so the helper's primary…
What changed in this PR

Adds a Go test helper that provisions and caches a compatible Copilot CLI runtime while sharing secure npm package handling with the bundler.

Changes:

  • Adds testcli.Setup with runtime discovery, integrity verification, and caching.
  • Introduces shared npm download and extraction utilities.
  • Updates Go CLI setup documentation and tests.
File Description
go/​testcli/​setup.go Implements test CLI provisioning and caching.
go/​testcli/​setup_test.go Tests metadata parsing and cache validation.
go/​testcli/​setup_external_test.go Tests configured-path handling.
go/​internal/​npmregistry/​package.go Adds npm download and extraction helpers.
go/​internal/​npmregistry/​package_test.go Tests integrity checks and safe extraction.
go/​cmd/​bundler/​main.go Reuses npm registry helpers.
go/​README.md Documents testcli.Setup.
docs/​setup/​local-cli.md Corrects Go CLI path resolution guidance.
docs/​setup/​bundled-cli.md Clarifies Go runtime precedence.
Suppressed comments (1)

go/testcli/setup.go:59

  • When UserCacheDir is unavailable on a Unix host, this falls back to a predictable directory beneath shared /tmp. Another local user can pre-create the public version/platform path with a matching package.json and .integrity; the pre-lock cache check then trusts and exposes their executable through COPILOT_CLI_PATH. Fail setup instead of using a shared cache root, or create a private per-user directory with mode 0700.
		cacheDir = os.TempDir()

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread go/testcli/setup.go
Comment thread go/testcli/setup.go Outdated
@SteveSandersonMS
SteveSandersonMS marked this pull request as draft September 2, 2026 12:50
@SteveSandersonMS

Copy link
Copy Markdown
Contributor

Thanks for the focused Go test helper and the integrity-safe extraction work. After rechecking the current runtime distribution, I believe this change should align with the GitHub Release flow introduced in #2463 rather than extend the old npm path.

Proposed changes:

  • Rebase or otherwise reconcile this PR with Use Copilot CLI releases for Node runtime #2463.
  • Download the pinned platform runtime archive from the github/copilot-cli GitHub Release, for example github-copilot-<version>-<platform>.tgz, and verify it against that release's SHA256SUMS.txt / trusted checksum mechanism.
  • Extract and publish prebuilds/<platform>/copilot-runtime (or copilot-runtime.exe on Windows) as COPILOT_CLI_PATH; do not use the top-level copilot/copilot.exe launcher.
  • Update cache validation and tests for the release archive layout, checksum verification, native wrapper, and runtime.node.
  • Recheck the shared bundler refactor so it does not preserve or reintroduce npm-only acquisition for the normal Go runtime path. If the Go npm path is intentionally retained, document that explicit exception and why it is still required.

The current tests prove the old npm package path, but not the release asset or native entrypoint now used by the updated runtime distribution. Please mark the PR ready for review again once this is reconciled.

For tracking, I've moved this to draft - please mark as ready to review when appropriate.

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.

4 participants