diff --git a/changelog.d/fixed-quality-sdk-helper-test-hermetic.md b/changelog.d/fixed-quality-sdk-helper-test-hermetic.md new file mode 100644 index 0000000000..c4a5d1ff2c --- /dev/null +++ b/changelog.d/fixed-quality-sdk-helper-test-hermetic.md @@ -0,0 +1 @@ +- Tests: the copilot SDK-helper probe tests in `pkg/dashboard` are now hermetic. `TestProbeCopilotModelsSDK_AbsentHelperYieldsSentinel` and `TestProbeCopilotModelsSDK_HelperNotInstalled` used to run whatever was installed at the production helper path, so on any host that ships `copilot-models.mjs` without stored copilot auth (every live agent host) the "absent helper" sentinel never fired and the whole `pkg/dashboard` suite went red with `Not authenticated`. The helper path is now a test-seam var (`setCopilotSDKHelperPathForTest`, mirroring the `knowledge.SetBaseDirForTest` convention) pointed at a temp path, and a new `TestProbeCopilotModelsSDK_FailingHelperIsNotAbsent` covers the previously untestable-on-CI third state — helper present but exiting nonzero (the #7365 case) — via a stub script instead of the real helper. diff --git a/src/pkg/dashboard/cli_models.go b/src/pkg/dashboard/cli_models.go index 300d6105e3..6ace0c6197 100644 --- a/src/pkg/dashboard/cli_models.go +++ b/src/pkg/dashboard/cli_models.go @@ -114,17 +114,6 @@ const ( // require a plausible editor identifier. copilotEditorVersion = "vscode/1.99.0" - // copilotSDKHelperPath is where the image installs the Node helper that - // lists Copilot models through the official @github/copilot-sdk (source: - // bin/copilot-models.mjs, COPYed by src/Dockerfile). The SDK spawns the - // pinned copilot CLI as a JSON-RPC server, so the probe rides the CLI's - // own stored auth and TLS handling — auth configurations the raw-HTTP - // probe below cannot reach (verified live against copilot CLI 1.0.59 in a - // hive pod: 25 models via stored device-flow auth behind the egress - // proxy). Absent outside the image, in which case the SDK probe is - // skipped instantly. - copilotSDKHelperPath = "/usr/local/bin/copilot-models.mjs" - // copilotSDKNodeBinary runs the helper. The helper is plain-Node ESM; it // is invoked explicitly (not via shebang) so no exec bit is needed. copilotSDKNodeBinary = "node" @@ -269,6 +258,31 @@ const ( // it at hermetic servers and exercise fallback branches without network. var copilotUserEndpointURL = "https://api.github.com/copilot_internal/user" +// copilotSDKHelperPath is where the image installs the Node helper that +// lists Copilot models through the official @github/copilot-sdk (source: +// bin/copilot-models.mjs, COPYed by src/Dockerfile). The SDK spawns the +// pinned copilot CLI as a JSON-RPC server, so the probe rides the CLI's +// own stored auth and TLS handling — auth configurations the raw-HTTP +// probe below cannot reach (verified live against copilot CLI 1.0.59 in a +// hive pod: 25 models via stored device-flow auth behind the egress +// proxy). Absent outside the image, in which case the SDK probe is +// skipped instantly. A var (not const) only so tests can point it at a +// hermetic path regardless of what the host has installed; production +// never reassigns it. +var copilotSDKHelperPath = "/usr/local/bin/copilot-models.mjs" + +// setCopilotSDKHelperPathForTest repoints the SDK helper script path for the +// lifetime of t, mirroring the knowledge.SetBaseDirForTest convention. +func setCopilotSDKHelperPathForTest(t interface { + Helper() + Cleanup(func()) +}, path string) { + t.Helper() + old := copilotSDKHelperPath + copilotSDKHelperPath = path + t.Cleanup(func() { copilotSDKHelperPath = old }) +} + // --- Static fallback lists (kept CURRENT — July 2026) --- // claudeStaticModels is the fallback offered when the Claude models probe diff --git a/src/pkg/dashboard/cli_models_sdk_severity_test.go b/src/pkg/dashboard/cli_models_sdk_severity_test.go index bfd73a33d4..cafc565c52 100644 --- a/src/pkg/dashboard/cli_models_sdk_severity_test.go +++ b/src/pkg/dashboard/cli_models_sdk_severity_test.go @@ -15,6 +15,9 @@ import ( "context" "errors" "fmt" + "os" + "os/exec" + "path/filepath" "strings" "testing" ) @@ -39,15 +42,45 @@ func TestCopilotSDKHelperAbsentIsASentinel(t *testing.T) { // The real exec path must produce the sentinel when the helper script is not // installed, otherwise the sentinel is dead code and every dev machine starts -// emitting a WARN. +// emitting a WARN. The helper path is repointed at a path that does not exist +// so the assertion holds regardless of whether the host image ships the real +// helper (on live agent hosts it exists but is unauthenticated, which used to +// flip this test to a hard FAIL — the third state the old version, which ran +// whatever was at the production path, never accounted for). func TestProbeCopilotModelsSDK_AbsentHelperYieldsSentinel(t *testing.T) { - if _, err := execCopilotSDKHelper(context.Background(), ""); err != nil { - if !errors.Is(err, errCopilotSDKHelperAbsent) { - t.Fatalf("helper absence did not yield the sentinel: %v", err) - } - return + setCopilotSDKHelperPathForTest(t, filepath.Join(t.TempDir(), "copilot-models.mjs")) + _, err := execCopilotSDKHelper(context.Background(), "") + if err == nil { + t.Fatal("exec of a nonexistent helper unexpectedly succeeded") + } + if !errors.Is(err, errCopilotSDKHelperAbsent) { + t.Fatalf("helper absence did not yield the sentinel: %v", err) + } +} + +// The converse on the same exec path: a helper that EXISTS and fails (#7365 — +// exits nonzero) must NOT match the absence sentinel, or the failure would be +// logged at INFO and stay invisible. Uses a stub script so the assertion never +// depends on the host's real helper or its auth state. +func TestProbeCopilotModelsSDK_FailingHelperIsNotAbsent(t *testing.T) { + if _, err := exec.LookPath("node"); err != nil { + t.Skip("node not on PATH; cannot exercise the helper exec path") + } + stub := filepath.Join(t.TempDir(), "copilot-models.mjs") + if err := os.WriteFile(stub, []byte("console.error('stub failure'); process.exit(1);\n"), 0o644); err != nil { + t.Fatal(err) + } + setCopilotSDKHelperPathForTest(t, stub) + _, err := execCopilotSDKHelper(context.Background(), "") + if err == nil { + t.Fatal("failing stub helper unexpectedly succeeded") + } + if errors.Is(err, errCopilotSDKHelperAbsent) { + t.Fatalf("a real helper failure matched the absence sentinel: %v", err) + } + if !strings.Contains(err.Error(), "stub failure") { + t.Errorf("helper stderr not folded into the error: %v", err) } - t.Skip("copilot SDK helper present on this machine; skipping absence check") } // The severity split itself, asserted on real emitted log records. diff --git a/src/pkg/dashboard/cli_models_sdk_test.go b/src/pkg/dashboard/cli_models_sdk_test.go index b2138af1bc..99d7df3af3 100644 --- a/src/pkg/dashboard/cli_models_sdk_test.go +++ b/src/pkg/dashboard/cli_models_sdk_test.go @@ -3,6 +3,7 @@ package dashboard import ( "context" "errors" + "path/filepath" "testing" ) @@ -145,13 +146,15 @@ func TestQueryCLIModels_SDKResultsFeedRetention(t *testing.T) { // TestProbeCopilotModelsSDK_HelperNotInstalled verifies the real exec path // degrades instantly (no hang, no panic) when the helper script is absent — -// the situation on dev machines and CI runners. +// the situation on dev machines and CI runners. The helper path is repointed +// at a nonexistent file so the check is hermetic on hosts that DO ship the +// helper (where the old version exec'd the real helper — a network-dependent +// probe whose outcome tracked the host's copilot auth state). func TestProbeCopilotModelsSDK_HelperNotInstalled(t *testing.T) { + setCopilotSDKHelperPathForTest(t, filepath.Join(t.TempDir(), "copilot-models.mjs")) s := &Server{logger: testLogger()} if _, err := s.probeCopilotModelsSDK(""); err == nil { - // The helper is installed only inside the hive image; if this machine - // actually has it, the probe may legitimately succeed — skip then. - t.Skip("copilot SDK helper present on this machine; skipping absence check") + t.Fatal("probe with an absent helper unexpectedly succeeded") } }