From a240ad2b81ff35036a32565f45b3b7f92699f02a Mon Sep 17 00:00:00 2001 From: Raph Date: Mon, 3 Aug 2026 23:09:58 +0800 Subject: [PATCH 1/2] ci: focus macOS auth race tests --- .github/workflows/ci.yml | 7 +++++-- test/scripts/changelog_pr_gate_test.go | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6e3df6c7..c7070484 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -693,8 +693,11 @@ jobs: with: go-version-file: go.mod - - name: Test macOS auth and Keychain paths with Race Detection - run: go test -v -race -count=1 -timeout=10m ./internal/keychain ./internal/auth ./internal/app + - name: Test macOS auth and Keychain packages with Race Detection + run: go test -v -race -count=1 -timeout=10m ./internal/keychain ./internal/auth + + - name: Test macOS auth migration and Keychain diagnostics with Race Detection + run: go test -v -race -count=1 -timeout=5m ./internal/app -run '^TestAuth(MigrateKeychain|StatusDiagnosticReportsCiphertextKeyMismatch)' test-windows: name: Test (Windows) diff --git a/test/scripts/changelog_pr_gate_test.go b/test/scripts/changelog_pr_gate_test.go index 8418b64a..5107f44f 100644 --- a/test/scripts/changelog_pr_gate_test.go +++ b/test/scripts/changelog_pr_gate_test.go @@ -537,6 +537,27 @@ func TestChangelogPRFastPathWorkflowContract(t *testing.T) { t.Error("full race shards must retain enough package-level time for internal/app") } + darwinStart := strings.Index(admission, "\n test-darwin:\n") + darwinEnd := strings.Index(admission, "\n test-windows:\n") + if darwinStart < 0 || darwinEnd <= darwinStart { + t.Fatal("Code Admission workflow missing macOS test job boundaries") + } + darwinJob := admission[darwinStart:darwinEnd] + 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 '^TestAuth(MigrateKeychain|StatusDiagnosticReportsCiphertextKeyMismatch)'`, + } { + if !strings.Contains(darwinJob, 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") if coverageStart < 0 || coverageEnd <= coverageStart { From 2d3f820f915b4b324b93881af845e45a38254fa7 Mon Sep 17 00:00:00 2001 From: chichuan Date: Tue, 4 Aug 2026 17:59:59 +0800 Subject: [PATCH 2/2] ci: keep the darwin-gated upgrade self-heal test reachable on macOS Narrowing the macOS internal/app -run pattern orphaned TestValidateNewBinary_RecoversFromUnsignedDarwin: it is the only runtime.GOOS != "darwin" gated test in the package, the Ubuntu race shard skips it on Linux, and the platform coverage gate only runs ^(TestAllShortcuts|TestCrossPlatformCoverage). No CI job selected it any more, so it could never run or fail again. - Add the self-heal test back to the macOS -run pattern. - Add the (CrossPlatformCoverage)? group the Windows pattern already has, which also recovers TestCrossPlatformCoverageAuthMigrateKeychainRemainingBranches. - Attribute vacuous runs: the two skip paths now name the branch that went unverified, and DWS_REQUIRE_AMFI_SELF_HEAL=1 escalates such a run to a hard failure on a host that does enforce amfid. GitHub's hosted macOS runners do not reproduce the amfid kill, so this test has been silently skipping there all along. - Restore step-timeout headroom: 10m + 5m exactly equalled the 15m job budget, leaving none for setup. The keychain/auth step drops to 6m (measured 2m43s). - Add a contract test that couples the macOS -run pattern to the set of darwin-gated tests in internal/app, so the next narrowing fails loudly instead of silently orphaning one. --- .github/workflows/ci.yml | 6 +- internal/app/upgrade_test.go | 22 ++- test/scripts/changelog_pr_gate_test.go | 4 +- test/scripts/darwin_native_contract_test.go | 141 ++++++++++++++++++++ 4 files changed, 165 insertions(+), 8 deletions(-) create mode 100644 test/scripts/darwin_native_contract_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c7070484..6b7d0376 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -694,10 +694,10 @@ jobs: go-version-file: go.mod - name: Test macOS auth and Keychain packages with Race Detection - run: go test -v -race -count=1 -timeout=10m ./internal/keychain ./internal/auth + run: go test -v -race -count=1 -timeout=6m ./internal/keychain ./internal/auth - - name: Test macOS auth migration and Keychain diagnostics with Race Detection - run: go test -v -race -count=1 -timeout=5m ./internal/app -run '^TestAuth(MigrateKeychain|StatusDiagnosticReportsCiphertextKeyMismatch)' + - 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))' 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 5107f44f..c7c2a94f 100644 --- a/test/scripts/changelog_pr_gate_test.go +++ b/test/scripts/changelog_pr_gate_test.go @@ -544,8 +544,8 @@ func TestChangelogPRFastPathWorkflowContract(t *testing.T) { } darwinJob := admission[darwinStart:darwinEnd] 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 '^TestAuth(MigrateKeychain|StatusDiagnosticReportsCiphertextKeyMismatch)'`, + `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))'`, } { if !strings.Contains(darwinJob, want) { t.Errorf("macOS native test job missing focused auth contract %q", want) 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" +}