diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index eb877d6b..2e556857 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -702,14 +702,14 @@ jobs: with: go-version-file: go.mod - # Full ./internal/app (including schema --all assembly) already runs in the - # race:app shard. Keep this job focused on native auth/keychain paths so - # heavy Schema completeness tests cannot exhaust the 10m budget. - - name: Test macOS auth and Keychain paths with Race Detection - run: go test -v -race -count=1 -timeout=10m ./internal/keychain ./internal/auth + # Full app-package race (including schema --all assembly) already runs in + # the race:app shard. Keep this job focused on native auth/keychain paths + # so heavy Schema completeness tests cannot exhaust the budget. + - name: Test macOS auth and Keychain packages with Race Detection + run: go test -v -race -count=1 -timeout=6m ./internal/keychain ./internal/auth - - name: Test macOS auth migration and portable auth diagnostics - run: go test -v -race -count=1 -timeout=5m ./internal/app -run '^Test(CrossPlatformCoverage)?Auth(MigrateKeychain|StatusDiagnosticReportsCiphertextKeyMismatch|ExportRejectsWindowsDPAPIBackend|ImportRejectsWindowsDPAPIBackend)' + - name: Test macOS auth migration, Keychain diagnostics, and upgrade self-heal with Race Detection + run: go test -v -race -count=1 -timeout=5m ./internal/app -run '^(TestValidateNewBinary_RecoversFromUnsignedDarwin|Test(CrossPlatformCoverage)?Auth(MigrateKeychain|StatusDiagnosticReportsCiphertextKeyMismatch|ExportRejectsWindowsDPAPIBackend|ImportRejectsWindowsDPAPIBackend))' test-windows: name: Test (Windows) diff --git a/internal/app/upgrade_test.go b/internal/app/upgrade_test.go index ef5de305..2ca35f99 100644 --- a/internal/app/upgrade_test.go +++ b/internal/app/upgrade_test.go @@ -543,6 +543,13 @@ func TestIsLikelyAMFIKill(t *testing.T) { // validateNewBinary recovers via repairDarwinBinary (ad-hoc codesign) and // successfully re-executes the binary. We use go itself as a stand-in for the // new dws binary — it's a real signed Mach-O we can strip and re-sign. +// +// GitHub's hosted macOS runners do not reproduce the amfid kill, so in CI this +// test reports a skip that names the unverified path rather than implying the +// self-heal was exercised. Set DWS_REQUIRE_AMFI_SELF_HEAL=1 on a host that does +// enforce amfid to turn such a vacuous run into a hard failure. + +const requireAMFISelfHealEnv = "DWS_REQUIRE_AMFI_SELF_HEAL" func TestValidateNewBinary_RecoversFromUnsignedDarwin(t *testing.T) { if runtime.GOOS != "darwin" { @@ -573,15 +580,24 @@ func TestValidateNewBinary_RecoversFromUnsignedDarwin(t *testing.T) { t.Fatalf("strip signature: %v\n%s", err, out) } - // Sanity: confirm direct exec is killed. + // Sanity: confirm direct exec is killed. When it is not, repairDarwinBinary + // never runs and the rest of this test proves nothing — say so. if _, err := tryExecVersion(bin); err == nil { - t.Skip("unsigned binary executed without amfid kill — likely Intel Mac or SIP disabled") + const unverified = "amfid did not kill the unsigned binary, so repairDarwinBinary was NOT exercised" + if os.Getenv(requireAMFISelfHealEnv) == "1" { + t.Fatalf("%s (%s=1)", unverified, requireAMFISelfHealEnv) + } + t.Skipf("%s — host does not enforce amfid (Intel Mac, SIP disabled, or hosted runner)", unverified) } // validateNewBinary should self-heal and succeed. if err := validateNewBinary(bin, "dev"); err != nil { if strings.Contains(err.Error(), "signal: killed") { - t.Skipf("host security policy still rejects the ad-hoc signed test binary: %v", err) + const unverified = "host security policy still rejects the ad-hoc signed binary, so the self-heal outcome was NOT verified" + if os.Getenv(requireAMFISelfHealEnv) == "1" { + t.Fatalf("%s (%s=1): %v", unverified, requireAMFISelfHealEnv, err) + } + t.Skipf("%s: %v", unverified, err) } t.Fatalf("validateNewBinary did not recover: %v", err) } diff --git a/test/scripts/changelog_pr_gate_test.go b/test/scripts/changelog_pr_gate_test.go index 32651c2c..2587ca01 100644 --- a/test/scripts/changelog_pr_gate_test.go +++ b/test/scripts/changelog_pr_gate_test.go @@ -553,19 +553,27 @@ func TestChangelogPRFastPathWorkflowContract(t *testing.T) { t.Fatal("Code Admission workflow missing macOS test job boundaries") } darwinJob := admission[darwinStart:darwinEnd] - // Full ./internal/app race coverage lives in race:app; darwin stays focused on - // native auth/keychain plus a narrow auth-migration slice of internal/app. + // The macOS job no longer runs ./internal/app as a whole package, so it does + // not need the package-level race budget the Ubuntu shard gets — the Ubuntu + // "race: app" shard already covers everything except the natively-gated + // tests. Pinning the two focused commands replaces that budget assertion: + // it locks the per-step timeouts and blocks a whole-package regression. + // Keep portable DPAPI export/import diagnostics from this branch alongside + // main's unsigned-darwin self-heal coverage. for _, want := range []string{ - `go test -v -race -count=1 -timeout=10m ./internal/keychain ./internal/auth`, - `go test -v -race -count=1 -timeout=5m ./internal/app -run '^Test(CrossPlatformCoverage)?Auth(MigrateKeychain|StatusDiagnosticReportsCiphertextKeyMismatch|ExportRejectsWindowsDPAPIBackend|ImportRejectsWindowsDPAPIBackend)'`, + `go test -v -race -count=1 -timeout=6m ./internal/keychain ./internal/auth`, + `go test -v -race -count=1 -timeout=5m ./internal/app -run '^(TestValidateNewBinary_RecoversFromUnsignedDarwin|Test(CrossPlatformCoverage)?Auth(MigrateKeychain|StatusDiagnosticReportsCiphertextKeyMismatch|ExportRejectsWindowsDPAPIBackend|ImportRejectsWindowsDPAPIBackend))'`, } { if !strings.Contains(darwinJob, want) { - t.Errorf("macOS auth/keychain job missing contract %q", want) + t.Errorf("macOS native test job missing focused auth contract %q", want) } } if strings.Contains(darwinJob, "./internal/keychain ./internal/auth ./internal/app") { t.Error("macOS native test job must not repeat the complete internal/app race shard") } + if count := strings.Count(darwinJob, "./internal/app"); count != 1 { + t.Errorf("macOS native test job internal/app invocation count = %d, want 1 focused invocation", count) + } coverageStart := strings.Index(admission, "\n coverage:\n") coverageEnd := strings.Index(admission, "\n policy:\n") diff --git a/test/scripts/darwin_native_contract_test.go b/test/scripts/darwin_native_contract_test.go new file mode 100644 index 00000000..5f46bc8c --- /dev/null +++ b/test/scripts/darwin_native_contract_test.go @@ -0,0 +1,141 @@ +package scripts_test + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// TestMacOSNativeJobKeepsDarwinGatedAppTestsReachable couples the macOS test +// job's -run pattern to the set of darwin-gated tests in internal/app. +// +// The Ubuntu race shard runs the whole package but skips anything gated on +// runtime.GOOS != "darwin", and the platform coverage gate only runs +// ^(TestAllShortcuts|TestCrossPlatformCoverage). That makes the macOS job the +// only place a darwin-gated internal/app test can execute, so narrowing its +// -run pattern can silently orphan one — which is exactly what happened in +// #857 before review caught it. +func TestMacOSNativeJobKeepsDarwinGatedAppTestsReachable(t *testing.T) { + root, err := filepath.Abs(filepath.Join("..", "..")) + if err != nil { + t.Fatalf("Abs(repo root) error = %v", err) + } + + pattern := macOSAppRunPattern(t, root) + matcher, err := regexp.Compile(pattern) + if err != nil { + t.Fatalf("Compile(macOS -run pattern %q) error = %v", pattern, err) + } + + gated := darwinGatedTestNames(t, filepath.Join(root, "internal", "app")) + if len(gated) == 0 { + t.Fatal("found no darwin-gated tests in internal/app; the scanner is broken or the gate style changed") + } + + for _, name := range gated { + if !matcher.MatchString(name) { + t.Errorf( + "darwin-gated test %s is unreachable in CI: the Ubuntu shard skips it on Linux and the macOS -run pattern %q does not select it", + name, + pattern, + ) + } + } +} + +// macOSAppRunPattern extracts the -run pattern the macOS job applies to +// ./internal/app from the CI workflow. +func macOSAppRunPattern(t *testing.T, root string) string { + t.Helper() + + data, err := os.ReadFile(filepath.Join(root, ".github", "workflows", "ci.yml")) + if err != nil { + t.Fatalf("ReadFile(ci.yml) error = %v", err) + } + workflow := string(data) + + start := strings.Index(workflow, "\n test-darwin:\n") + end := strings.Index(workflow, "\n test-windows:\n") + if start < 0 || end <= start { + t.Fatal("ci.yml missing macOS test job boundaries") + } + + matches := regexp.MustCompile(`\./internal/app -run '([^']+)'`).FindAllStringSubmatch(workflow[start:end], -1) + if len(matches) != 1 { + t.Fatalf("macOS test job ./internal/app -run invocation count = %d, want exactly 1", len(matches)) + } + return matches[0][1] +} + +// darwinGatedTestNames returns every Test function in dir whose body gates on +// runtime.GOOS != "darwin". +func darwinGatedTestNames(t *testing.T, dir string) []string { + t.Helper() + + entries, err := filepath.Glob(filepath.Join(dir, "*_test.go")) + if err != nil { + t.Fatalf("Glob(%s) error = %v", dir, err) + } + + var names []string + fset := token.NewFileSet() + for _, entry := range entries { + file, parseErr := parser.ParseFile(fset, entry, nil, parser.SkipObjectResolution) + if parseErr != nil { + t.Fatalf("ParseFile(%s) error = %v", entry, parseErr) + } + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil || !strings.HasPrefix(fn.Name.Name, "Test") { + continue + } + if gatesOnNonDarwin(fn.Body) { + names = append(names, fn.Name.Name) + } + } + } + return names +} + +// gatesOnNonDarwin reports whether body contains a `runtime.GOOS != "darwin"` +// comparison, the idiom this repo uses to skip a test off macOS. +func gatesOnNonDarwin(body *ast.BlockStmt) bool { + found := false + ast.Inspect(body, func(node ast.Node) bool { + if found { + return false + } + binary, ok := node.(*ast.BinaryExpr) + if !ok || binary.Op != token.NEQ { + return true + } + if !isRuntimeGOOS(binary.X) { + return true + } + literal, ok := binary.Y.(*ast.BasicLit) + if !ok || literal.Kind != token.STRING { + return true + } + if literal.Value == `"darwin"` { + found = true + return false + } + return true + }) + return found +} + +// isRuntimeGOOS reports whether expr is the selector runtime.GOOS. +func isRuntimeGOOS(expr ast.Expr) bool { + selector, ok := expr.(*ast.SelectorExpr) + if !ok || selector.Sel.Name != "GOOS" { + return false + } + ident, ok := selector.X.(*ast.Ident) + return ok && ident.Name == "runtime" +}