From 2f7a9b53c0208eecd161a0d1a27b53251060785f Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 2 Nov 2026 04:42:21 +0000 Subject: [PATCH 1/6] test(ci): PAD_TEST_PG_TRACE marks every test Postgres connection, and ci-pg-trace.py extracts them (TASK-4224 part 3) --- cmd/pad/postgres_client_test.go | 15 ++++ internal/store/nul_native_pg_test.go | 1 + .../store/nul_repair_differential_test.go | 1 + internal/store/nulguard_test.go | 7 ++ internal/store/pgtrace_test.go | 16 ++++ internal/store/store_test.go | 31 ++++--- internal/store/storetest/pgtrace.go | 19 +++++ internal/store/storetest/pgtrace_test.go | 83 +++++++++++++++++++ internal/store/storetest/postgres.go | 27 +++--- scripts/ci-pg-packages.txt | 7 ++ scripts/ci-pg-trace.py | 67 +++++++++++++++ scripts/ci-pg-trace_test.py | 67 +++++++++++++++ 12 files changed, 318 insertions(+), 23 deletions(-) create mode 100644 internal/store/pgtrace_test.go create mode 100644 internal/store/storetest/pgtrace.go create mode 100644 internal/store/storetest/pgtrace_test.go create mode 100644 scripts/ci-pg-packages.txt create mode 100755 scripts/ci-pg-trace.py create mode 100644 scripts/ci-pg-trace_test.py diff --git a/cmd/pad/postgres_client_test.go b/cmd/pad/postgres_client_test.go index 50938ae02..25048a26a 100644 --- a/cmd/pad/postgres_client_test.go +++ b/cmd/pad/postgres_client_test.go @@ -2,6 +2,7 @@ package main import ( "database/sql" + "errors" "net/url" "os" "os/exec" @@ -9,6 +10,7 @@ import ( "strings" "testing" + "github.com/PerpetualSoftware/pad/internal/store/storetest" "github.com/google/uuid" ) @@ -111,6 +113,7 @@ func TestPostgresBackupRestoreConnection(t *testing.T) { if _, err := admin.Exec("CREATE DATABASE " + name); err != nil { t.Fatal(err) } + storetest.TracePGConnected(t, "TestPostgresBackupRestoreConnection.admin") t.Cleanup(func() { if _, err := admin.Exec("DROP DATABASE " + name + " WITH (FORCE)"); err != nil { t.Error(err) @@ -134,6 +137,7 @@ func TestPostgresBackupRestoreConnection(t *testing.T) { if _, err := source.Exec("CREATE TABLE recovery_probe (value text); INSERT INTO recovery_probe VALUES ('restored row')"); err != nil { t.Fatal(err) } + storetest.TracePGConnected(t, "TestPostgresBackupRestoreConnection.source") // A wrong ambient database must not override the explicit URI. t.Setenv("PGDATABASE", "not_the_requested_database") t.Setenv("PGPASSWORD", "not_the_explicit_password") @@ -177,16 +181,19 @@ func TestPostgresBackupRestoreConnection(t *testing.T) { if err := backup.Execute(); err != nil { t.Fatal(err) } + storetest.TracePGConnected(t, "TestPostgresBackupRestoreConnection.pg_dump") t.Setenv("PAD_DATABASE_URL", convert(targetURL)) restore := dbRestoreCmd() restore.SetArgs([]string{"--force", archive}) if err := restore.Execute(); err != nil { t.Fatal(err) } + storetest.TracePGConnected(t, "TestPostgresBackupRestoreConnection.psql") var value string if err := target.QueryRow("SELECT value FROM recovery_probe").Scan(&value); err != nil || value != "restored row" { t.Fatalf("round trip: value=%q err=%v", value, err) } + storetest.TracePGConnected(t, "TestPostgresBackupRestoreConnection.target") } t.Run("SQL failure rolls back and overrides psqlrc", func(t *testing.T) { psqlrc := filepath.Join(t.TempDir(), "psqlrc") @@ -203,6 +210,13 @@ func TestPostgresBackupRestoreConnection(t *testing.T) { restore.SetArgs([]string{"--force", failedArchive}) if err := restore.Execute(); err == nil { t.Error("restore returned success after a SQL error rolled back its transaction") + } else { + // psql exit 3 means a script error with ON_ERROR_STOP, after + // connecting and beginning the --single-transaction transaction. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 3 { + storetest.TracePGConnected(t, "TestPostgresBackupRestoreConnection.psql.sql_error") + } } var value string if err := target.QueryRow("SELECT value FROM recovery_probe").Scan(&value); err != nil || value != "restored row" { @@ -214,6 +228,7 @@ func TestPostgresBackupRestoreConnection(t *testing.T) { if err := validRestore.Execute(); err != nil { t.Fatal(err) } + storetest.TracePGConnected(t, "TestPostgresBackupRestoreConnection.psql") if err := target.QueryRow("SELECT value FROM recovery_probe").Scan(&value); err != nil || value != "restored row" { t.Fatalf("valid restore after SQL error: value=%q err=%v", value, err) } diff --git a/internal/store/nul_native_pg_test.go b/internal/store/nul_native_pg_test.go index 1c97c1501..9339afd9f 100644 --- a/internal/store/nul_native_pg_test.go +++ b/internal/store/nul_native_pg_test.go @@ -39,6 +39,7 @@ func TestNativePostgresAgreesWithTheCorpus(t *testing.T) { if _, err := db.Exec(`DROP TABLE IF EXISTS nul_differential`); err != nil { t.Fatalf("drop: %v", err) } + tracePGConnected(t, "TestNativePostgresAgreesWithTheCorpus") // Both column classes, so each corpus case lands on the column its own // classing names — the same choice the store's leg makes. if _, err := db.Exec(`CREATE TABLE nul_differential (id TEXT PRIMARY KEY, txt TEXT, doc JSONB)`); err != nil { diff --git a/internal/store/nul_repair_differential_test.go b/internal/store/nul_repair_differential_test.go index a1a96680b..a844286eb 100644 --- a/internal/store/nul_repair_differential_test.go +++ b/internal/store/nul_repair_differential_test.go @@ -135,6 +135,7 @@ func TestNativePostgresAcceptsEveryRepairedCorpusValue(t *testing.T) { if _, err := db.Exec(`DROP TABLE IF EXISTS nul_repair_differential`); err != nil { t.Fatalf("drop: %v", err) } + tracePGConnected(t, "TestNativePostgresAcceptsEveryRepairedCorpusValue") if _, err := db.Exec( `CREATE TABLE nul_repair_differential (id TEXT PRIMARY KEY, txt TEXT, doc JSONB)`, ); err != nil { diff --git a/internal/store/nulguard_test.go b/internal/store/nulguard_test.go index 9732b4ddd..9ba3d9e63 100644 --- a/internal/store/nulguard_test.go +++ b/internal/store/nulguard_test.go @@ -437,6 +437,13 @@ func TestWrapperAdvertisesExactlyWhatTheBaseDoes(t *testing.T) { return nil //nolint:nilerr // a driver that cannot prepare this tells us nothing } defer st.Close() + if p.base == "pgx" { + helper := "TestWrapperAdvertisesExactlyWhatTheBaseDoes.raw" + if db == wrapDB { + helper = "TestWrapperAdvertisesExactlyWhatTheBaseDoes.guarded" + } + tracePGConnected(t, helper) + } out["StmtExecContext"] = isIface[driver.StmtExecContext](st) out["StmtQueryContext"] = isIface[driver.StmtQueryContext](st) out["stmt NamedValueChecker"] = isIface[driver.NamedValueChecker](st) diff --git a/internal/store/pgtrace_test.go b/internal/store/pgtrace_test.go new file mode 100644 index 000000000..e5d3e5515 --- /dev/null +++ b/internal/store/pgtrace_test.go @@ -0,0 +1,16 @@ +package store + +import ( + "os" + "testing" +) + +// Duplicated from storetest.TracePGConnected: storetest imports store, so these +// white-box tests cannot import it. Keep the gate and marker format in sync. +// A nil tb is reserved for TestMain template teardown, after tests finish. +func tracePGConnected(tb testing.TB, helper string) { + if os.Getenv("PAD_TEST_PG_TRACE") == "1" && tb != nil { + tb.Helper() + tb.Log("PAD_PG_CONNECTED helper=" + helper) + } +} diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 078c47533..d0c38db6e 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -199,14 +199,14 @@ func testStorePostgres(t *testing.T, baseURL string) *Store { defer pgTemplateMu.RUnlock() pgTemplateOnce.Do(func() { - pgTemplateName, pgTemplateErr = buildPostgresTemplate(baseURL) + pgTemplateName, pgTemplateErr = buildPostgresTemplate(t, baseURL) }) if pgTemplateErr != nil { t.Fatalf("build postgres template: %v", pgTemplateErr) } // Connect to the default "pad" database to issue CREATE/DROP DATABASE. - adminStore, err := newPostgresConn(baseURL) + adminStore, err := newPostgresConn(t, baseURL) if err != nil { t.Fatalf("connect to postgres for admin: %v", err) } @@ -235,14 +235,15 @@ func testStorePostgres(t *testing.T, baseURL string) *Store { // behind on exactly the path that most needs cleaning up. Pre-existing // and asymmetric with the twin; corrected here because this change // makes the two helpers a matched pair (codex round 2, P2). - dropPostgresTemplateDB(baseURL, dbName) + dropPostgresTemplateDB(t, baseURL, dbName) t.Fatalf("open test postgres store: %v", err) } + tracePGConnected(t, "testStorePostgres") t.Cleanup(func() { s.Close() // Drop the test database. - if admin, err := newPostgresConn(baseURL); err == nil { + if admin, err := newPostgresConn(t, baseURL); err == nil { admin.db.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS %s WITH (FORCE)", dbName)) admin.Close() } @@ -260,11 +261,11 @@ func testStorePostgres(t *testing.T, baseURL string) *Store { // LEAK POSTURE: a binary killed mid-run leaves its pad_tmpl_* database behind, // bounded by the container's lifetime — the test stack is per-worktree // (TASK-2708) and `make test-pg` tears it down with -v. -func buildPostgresTemplate(baseURL string) (string, error) { +func buildPostgresTemplate(tb testing.TB, baseURL string) (string, error) { atomic.AddInt32(&pgTemplateBuilds, 1) name := "pad_tmpl_" + uuid.New().String()[:8] - admin, err := newPostgresConn(baseURL) + admin, err := newPostgresConn(tb, baseURL) if err != nil { return "", fmt.Errorf("connect to postgres for admin: %w", err) } @@ -277,9 +278,10 @@ func buildPostgresTemplate(baseURL string) (string, error) { // The one time this binary pays for the migration chain. s, err := NewPostgres(replaceDBName(baseURL, name)) if err != nil { - dropPostgresTemplateDB(baseURL, name) + dropPostgresTemplateDB(tb, baseURL, name) return "", fmt.Errorf("migrate template database %s: %w", name, err) } + tracePGConnected(tb, "buildPostgresTemplate") // Stamp the template with a marker a freshly-migrated database cannot have. // Without it this mechanism is untestable: a clone and a per-test migration // produce otherwise identical databases, so reverting the TEMPLATE clause @@ -293,11 +295,11 @@ func buildPostgresTemplate(baseURL string) (string, error) { // reason. KEEP IN SYNC. if _, err := s.db.Exec(fmt.Sprintf("COMMENT ON TABLE schema_migrations IS '%s'", pgTemplateMarker(name))); err != nil { s.Close() - dropPostgresTemplateDB(baseURL, name) + dropPostgresTemplateDB(tb, baseURL, name) return "", fmt.Errorf("stamp template %s: %w", name, err) } if err := s.Close(); err != nil { - dropPostgresTemplateDB(baseURL, name) + dropPostgresTemplateDB(tb, baseURL, name) return "", fmt.Errorf("close template connection %s: %w", name, err) } return name, nil @@ -318,24 +320,27 @@ func removePostgresTemplate() { return } if baseURL := os.Getenv("PAD_TEST_POSTGRES_URL"); baseURL != "" { - dropPostgresTemplateDB(baseURL, pgTemplateName) + // TestMain runs after every testing.T has finished; no live TB can log. + dropPostgresTemplateDB(nil, baseURL, pgTemplateName) } pgTemplateName = "" } -func dropPostgresTemplateDB(baseURL, name string) { - if admin, err := newPostgresConn(baseURL); err == nil { +func dropPostgresTemplateDB(tb testing.TB, baseURL, name string) { + if admin, err := newPostgresConn(tb, baseURL); err == nil { admin.db.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS %s WITH (FORCE)", name)) admin.Close() } } // newPostgresConn opens a raw postgres connection (no migrations). -func newPostgresConn(connStr string) (*Store, error) { +func newPostgresConn(tb testing.TB, connStr string) (*Store, error) { db, err := openPostgresDB(connStr) if err != nil { return nil, err } + // openPostgresDB has already completed a successful Ping. + tracePGConnected(tb, "newPostgresConn") return &Store{db: db, dialect: &postgresDialect{}}, nil } diff --git a/internal/store/storetest/pgtrace.go b/internal/store/storetest/pgtrace.go new file mode 100644 index 000000000..158b3e00d --- /dev/null +++ b/internal/store/storetest/pgtrace.go @@ -0,0 +1,19 @@ +package storetest + +import ( + "os" + "testing" +) + +// TracePGConnected records a test Postgres connection only when explicitly +// enabled. Call after a successful Ping, query, or migration, never sql.Open. +// helper must be a stable name, with no connection details. +// A nil tb is reserved for template teardown from TestMain, where no live test +// remains to receive a log. Those teardown-only connections cannot be traced +// through testing.TB; per-test and error-path cleanup always pass a live tb. +func TracePGConnected(tb testing.TB, helper string) { + if os.Getenv("PAD_TEST_PG_TRACE") == "1" && tb != nil { + tb.Helper() + tb.Log("PAD_PG_CONNECTED helper=" + helper) + } +} diff --git a/internal/store/storetest/pgtrace_test.go b/internal/store/storetest/pgtrace_test.go new file mode 100644 index 000000000..a916f691d --- /dev/null +++ b/internal/store/storetest/pgtrace_test.go @@ -0,0 +1,83 @@ +package storetest + +import ( + "fmt" + "os" + "os/exec" + "strings" + "testing" +) + +type pgTraceRecorder struct { + testing.TB + lines []string +} + +func (*pgTraceRecorder) Helper() {} +func (r *pgTraceRecorder) Log(args ...any) { + r.lines = append(r.lines, fmt.Sprint(args...)) +} + +func TestTracePGConnected(t *testing.T) { + // Merely configuring Postgres must never enable tracing. + t.Setenv("PAD_TEST_POSTGRES_URL", "configured-but-unused") + for _, tc := range []struct { + trace string + want string + }{ + {"", ""}, + {"0", ""}, + {"true", ""}, + {"1", "PAD_PG_CONNECTED helper=probe"}, + } { + t.Run("trace="+tc.trace, func(t *testing.T) { + t.Setenv("PAD_TEST_PG_TRACE", tc.trace) + if tc.trace == "" { + if err := os.Unsetenv("PAD_TEST_PG_TRACE"); err != nil { + t.Fatal(err) + } + } + r := &pgTraceRecorder{} + TracePGConnected(r, "probe") + if got := strings.Join(r.lines, "\n"); got != tc.want { + t.Fatalf("log = %q, want %q", got, tc.want) + } + }) + } +} + +func TestNewPostgres_TraceGate(t *testing.T) { + if os.Getenv("PAD_TEST_POSTGRES_URL") == "" { + t.Skip("PAD_TEST_POSTGRES_URL not set") + } + if os.Getenv("PAD_PG_TRACE_TEST_CHILD") == "1" { + _ = NewPostgres(t) + return + } + for _, trace := range []string{"", "1"} { + t.Run("trace="+trace, func(t *testing.T) { + cmd := exec.Command(os.Args[0], "-test.run=^TestNewPostgres_TraceGate$", "-test.v") + for _, entry := range os.Environ() { + if !strings.HasPrefix(entry, "PAD_TEST_PG_TRACE=") && !strings.HasPrefix(entry, "PAD_PG_TRACE_TEST_CHILD=") { + cmd.Env = append(cmd.Env, entry) + } + } + cmd.Env = append(cmd.Env, "PAD_PG_TRACE_TEST_CHILD=1") + if trace != "" { + cmd.Env = append(cmd.Env, "PAD_TEST_PG_TRACE="+trace) + } + output, err := cmd.CombinedOutput() + if err != nil { + // Connection errors can contain credentials; do not echo output. + t.Fatalf("Postgres trace probe failed: %v", err) + } + got := strings.Contains(string(output), "PAD_PG_CONNECTED") + if got != (trace == "1") { + t.Fatalf("marker present = %t for trace=%q", got, trace) + } + if trace == "1" && !strings.Contains(string(output), "PAD_PG_CONNECTED helper=storetest.NewPostgres\n") { + t.Fatal("missing marker from the connected cloned store") + } + }) + } +} diff --git a/internal/store/storetest/postgres.go b/internal/store/storetest/postgres.go index 7d288ef3e..5753c1f80 100644 --- a/internal/store/storetest/postgres.go +++ b/internal/store/storetest/postgres.go @@ -81,7 +81,7 @@ func NewPostgres(t *testing.T) *store.Store { defer pgTemplateMu.RUnlock() pgTemplateOnce.Do(func() { - pgTemplateName, pgTemplateErr = buildPGTemplate(baseURL) + pgTemplateName, pgTemplateErr = buildPGTemplate(t, baseURL) }) if pgTemplateErr != nil { t.Fatalf("storetest: build pg template: %v", pgTemplateErr) @@ -99,6 +99,7 @@ func NewPostgres(t *testing.T) *store.Store { if err != nil { t.Fatalf("storetest: clone test database %s from template %s: %v", dbName, pgTemplateName, err) } + TracePGConnected(t, "storetest.NewPostgres.admin") // The clone arrives already migrated, so store.NewPostgres's own migrate() // is a fast no-op — schema_migrations came with the copy. Deliberately NOT @@ -106,12 +107,13 @@ func NewPostgres(t *testing.T) *store.Store { // the fixture honest, and it is the same choice NewSQLite makes. s, err := store.NewPostgres(replaceDBName(baseURL, dbName)) if err != nil { - dropPostgresDB(baseURL, dbName) + dropPostgresDB(t, baseURL, dbName) t.Fatalf("storetest: open cloned test postgres store: %v", err) } + TracePGConnected(t, "storetest.NewPostgres") t.Cleanup(func() { _ = s.Close() - dropPostgresDB(baseURL, dbName) + dropPostgresDB(t, baseURL, dbName) }) return s } @@ -128,7 +130,7 @@ func NewPostgres(t *testing.T) *store.Store { // the Postgres container is per-worktree (TASK-2708) and `make test-pg` tears // its stack down with -v, so an orphaned template dies with the container that // holds it. Not worth a reaper. -func buildPGTemplate(baseURL string) (string, error) { +func buildPGTemplate(tb testing.TB, baseURL string) (string, error) { atomic.AddInt32(&pgBuildCount, 1) name := "pad_tmpl_" + strings.ReplaceAll(uuid.New().String()[:8], "-", "") @@ -140,14 +142,16 @@ func buildPGTemplate(baseURL string) (string, error) { _ = admin.Close() return "", fmt.Errorf("create template database %s: %w", name, err) } + TracePGConnected(tb, "storetest.buildPGTemplate.admin") _ = admin.Close() // The one time this binary pays for the migration chain. s, err := store.NewPostgres(replaceDBName(baseURL, name)) if err != nil { - dropPostgresDB(baseURL, name) + dropPostgresDB(tb, baseURL, name) return "", fmt.Errorf("migrate template database %s: %w", name, err) } + TracePGConnected(tb, "storetest.buildPGTemplate") // Stamp the template with a marker a FRESHLY-MIGRATED database cannot // have. This is what makes the mechanism testable at all: a clone and a // per-test migration produce otherwise byte-identical databases, so no @@ -169,11 +173,11 @@ func buildPGTemplate(baseURL string) (string, error) { // inside the same second as the template's and produce an identical set. if _, err := s.DB().Exec(fmt.Sprintf("COMMENT ON TABLE schema_migrations IS '%s'", templateMarker(name))); err != nil { _ = s.Close() - dropPostgresDB(baseURL, name) + dropPostgresDB(tb, baseURL, name) return "", fmt.Errorf("stamp template %s: %w", name, err) } if err := s.Close(); err != nil { - dropPostgresDB(baseURL, name) + dropPostgresDB(tb, baseURL, name) return "", fmt.Errorf("close template connection %s: %w", name, err) } return name, nil @@ -192,18 +196,21 @@ func dropPGTemplate() { return } if baseURL := os.Getenv("PAD_TEST_POSTGRES_URL"); baseURL != "" { - dropPostgresDB(baseURL, pgTemplateName) + // TestMain runs after every testing.T has finished; no live TB can log. + dropPostgresDB(nil, baseURL, pgTemplateName) } pgTemplateName = "" } -func dropPostgresDB(baseURL, dbName string) { +func dropPostgresDB(tb testing.TB, baseURL, dbName string) { admin, err := sql.Open("pgx", baseURL) if err != nil { return } defer admin.Close() - _, _ = admin.Exec("DROP DATABASE IF EXISTS " + dbName + " WITH (FORCE)") + if _, err := admin.Exec("DROP DATABASE IF EXISTS " + dbName + " WITH (FORCE)"); err == nil { + TracePGConnected(tb, "storetest.dropPostgresDB") + } } // replaceDBName swaps the database name in a postgres connection URL, preserving diff --git a/scripts/ci-pg-packages.txt b/scripts/ci-pg-packages.txt new file mode 100644 index 000000000..962e36c35 --- /dev/null +++ b/scripts/ci-pg-packages.txt @@ -0,0 +1,7 @@ +# CANDIDATE: finalized only against a full traced PG run. +./cmd/pad +./internal/appstore +./internal/mcp +./internal/server +./internal/store +./internal/store/storetest diff --git a/scripts/ci-pg-trace.py b/scripts/ci-pg-trace.py new file mode 100755 index 000000000..3afaebc0f --- /dev/null +++ b/scripts/ci-pg-trace.py @@ -0,0 +1,67 @@ +#!/usr/bin/env python3 +"""Print packages with successful test Postgres connections from go test -json. + +Usage: PAD_TEST_PG_TRACE=1 go test -json ... | python3 scripts/ci-pg-trace.py \ + --expect scripts/ci-pg-packages.txt + +The candidate list is evidence only; this command does not check the test run's +exit status. Preserve that separately (for example, with shell pipefail). +""" + +import argparse +import json +from pathlib import Path +import posixpath +import sys + + +def normalize_package(package, module): + if package == "." or package.startswith("./"): + package = module + "/" + package + return posixpath.normpath(package) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--expect", type=Path, help="expected package list") + args = parser.parse_args() + + connected = set() + try: + for number, line in enumerate(sys.stdin, 1): + if not line.strip(): + continue + try: + event = json.loads(line) + except json.JSONDecodeError: + parser.error(f"invalid JSON on stdin line {number}") + if (event.get("Action") == "output" + and "PAD_PG_CONNECTED" in event.get("Output", "") + and event.get("Package")): + connected.add(event["Package"]) + + expected = None + if args.expect is not None: + go_mod = Path(__file__).resolve().parent.parent / "go.mod" + module = next(line.split()[1] for line in go_mod.read_text().splitlines() + if line.startswith("module ")) + expected = {normalize_package(line.strip(), module) + for line in args.expect.read_text().splitlines() + if line.strip() and not line.lstrip().startswith("#")} + except OSError as err: + parser.error(str(err)) + + for package in sorted(connected): + print(package) + if expected is not None: + for package in sorted(connected - expected): + print(f"unexpected: {package}", file=sys.stderr) + for package in sorted(expected - connected): + print(f"warning: expected package never connected: {package}", file=sys.stderr) + if connected - expected: + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/ci-pg-trace_test.py b/scripts/ci-pg-trace_test.py new file mode 100644 index 000000000..5ab168ea1 --- /dev/null +++ b/scripts/ci-pg-trace_test.py @@ -0,0 +1,67 @@ +"""CLI regression cases for the Postgres connection evidence extractor.""" + +import json +from pathlib import Path +import subprocess +import sys +import tempfile +import unittest + + +ROOT = Path(__file__).resolve().parent.parent +SCRIPT = ROOT / "scripts" / "ci-pg-trace.py" +MODULE = "github.com/PerpetualSoftware/pad" + + +def event(package, output="PAD_PG_CONNECTED helper=probe\n", action="output"): + return {"Action": action, "Package": package, "Test": "TestProbe", "Output": output} + + +class TraceTest(unittest.TestCase): + def test_cli(self): + cases = [ + ("empty", [], None, 0, [], []), + ("sort and deduplicate", [event(MODULE + "/internal/store"), + event(MODULE + "/cmd/pad"), + event(MODULE + "/internal/store")], + None, 0, [MODULE + "/cmd/pad", MODULE + "/internal/store"], []), + ("only output events with packages", + [event("ignored", action="pass"), event("ignored", "ordinary log\n"), + {"Action": "output", "Output": "PAD_PG_CONNECTED helper=probe"}], + None, 0, [], []), + ("relative and import paths", + [event(MODULE + "/cmd/pad"), event(MODULE + "/internal/store")], + "# candidate\n\n./cmd/pad/\n" + MODULE + "/internal/store\n", + 0, [MODULE + "/cmd/pad", MODULE + "/internal/store"], []), + ("unexpected and missing", + [event(MODULE + "/internal/server"), event(MODULE + "/cmd/pad")], + "./internal/store\n", + 1, [MODULE + "/cmd/pad", MODULE + "/internal/server"], + ["unexpected: " + MODULE + "/cmd/pad", + "unexpected: " + MODULE + "/internal/server", + "warning: expected package never connected: " + MODULE + "/internal/store"]), + ("missing is only a warning", [], "./internal/store\n", 0, [], + ["warning: expected package never connected: " + MODULE + "/internal/store"]), + ("package output without a test", + [{"Action": "output", "Package": MODULE + "/internal/store", + "Output": "PAD_PG_CONNECTED helper=probe\n"}], "./internal/store\n", + 0, [MODULE + "/internal/store"], []), + ] + for name, events, expected, code, packages, diagnostics in cases: + with self.subTest(name=name), tempfile.TemporaryDirectory() as tmp: + args = [sys.executable, str(SCRIPT)] + if expected is not None: + expect = Path(tmp) / "expected.txt" + expect.write_text(expected) + args += ["--expect", str(expect)] + result = subprocess.run( + args, input="".join(json.dumps(row) + "\n" for row in events), + text=True, capture_output=True, cwd=tmp, check=False, + ) + self.assertEqual(result.returncode, code, result.stderr) + self.assertEqual(result.stdout.splitlines(), packages) + self.assertEqual(result.stderr.splitlines(), diagnostics) + + +if __name__ == "__main__": + unittest.main() From 4e23d5478ec0f91db643963daaa5b792a4fec6ce Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 2 Nov 2026 06:46:27 +0000 Subject: [PATCH 2/6] ci: add scope, result, and source census guards (TASK-4224) Add fail-closed path classification and aggregate-result checks, checked-in web parity policy, AST source censuses, and human-readable PG trace streaming. The strict PG census currently rejects scripts/diagnose-version-ties: its main can reach PostgreSQL, it has no tests, and it is absent from the package list. Keep that list unchanged as instructed. Its classification needs a ruling before this branch can merge; no silent exception is added. --- scripts/ci-pg-census-allow.txt | 5 + scripts/ci-pg-trace.py | 4 + scripts/ci-pg-trace_test.py | 10 + scripts/ci-result.py | 55 +++ scripts/ci-result_test.py | 88 +++++ scripts/ci-scope.py | 103 ++++++ scripts/ci-scope_test.py | 100 ++++++ scripts/ci-web-census-allow.txt | 9 + scripts/ci-web-census/main.go | 546 +++++++++++++++++++++++++++++ scripts/ci-web-census/main_test.go | 123 +++++++ scripts/ci-web-parity.txt | 14 + 11 files changed, 1057 insertions(+) create mode 100644 scripts/ci-pg-census-allow.txt create mode 100644 scripts/ci-result.py create mode 100644 scripts/ci-result_test.py create mode 100644 scripts/ci-scope.py create mode 100644 scripts/ci-scope_test.py create mode 100644 scripts/ci-web-census-allow.txt create mode 100644 scripts/ci-web-census/main.go create mode 100644 scripts/ci-web-census/main_test.go create mode 100644 scripts/ci-web-parity.txt diff --git a/scripts/ci-pg-census-allow.txt b/scripts/ci-pg-census-allow.txt new file mode 100644 index 000000000..2801fc4c6 --- /dev/null +++ b/scripts/ci-pg-census-allow.txt @@ -0,0 +1,5 @@ +# Packagereason. Valid only while storetest usage is NewSQLite/Cleanup. +./internal/decision storetest.NewSQLite only; no PostgreSQL constructor. +./internal/oauth storetest.NewSQLite only; Cleanup cannot create a PostgreSQL template. +# Hash-pinned graph boundary: sql.Open here only retrieves Driver(), then closes; it never dials. +./internal/store.registerGuardedDrivers sha256:ca328ba0209b1304b14957e6e36d0f45712ae7f2731bd5190fb9a05da1224190 Driver registration for SQLite and PG only; no Ping/Conn/query. Any source change requires review. diff --git a/scripts/ci-pg-trace.py b/scripts/ci-pg-trace.py index 3afaebc0f..55d79e676 100755 --- a/scripts/ci-pg-trace.py +++ b/scripts/ci-pg-trace.py @@ -24,6 +24,8 @@ def normalize_package(package, module): def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--expect", type=Path, help="expected package list") + parser.add_argument("--show-output", action="store_true", + help="also stream human-readable go test Output events") args = parser.parse_args() connected = set() @@ -35,6 +37,8 @@ def main(): event = json.loads(line) except json.JSONDecodeError: parser.error(f"invalid JSON on stdin line {number}") + if args.show_output and event.get("Action") == "output": + print(event.get("Output", ""), end="", flush=True) if (event.get("Action") == "output" and "PAD_PG_CONNECTED" in event.get("Output", "") and event.get("Package")): diff --git a/scripts/ci-pg-trace_test.py b/scripts/ci-pg-trace_test.py index 5ab168ea1..dbba23016 100644 --- a/scripts/ci-pg-trace_test.py +++ b/scripts/ci-pg-trace_test.py @@ -63,5 +63,15 @@ class TraceTest(unittest.TestCase): self.assertEqual(result.stderr.splitlines(), diagnostics) +class HumanOutputTest(unittest.TestCase): + def test_output_can_be_mirrored_without_losing_trace(self): + row = event(MODULE + '/internal/store', 'ordinary test output\n') + result = subprocess.run([sys.executable, str(SCRIPT), '--show-output'], + input=json.dumps(row) + '\n', text=True, + capture_output=True, check=False) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, 'ordinary test output\n') + + if __name__ == "__main__": unittest.main() diff --git a/scripts/ci-result.py b/scripts/ci-result.py new file mode 100644 index 000000000..b4185289a --- /dev/null +++ b/scripts/ci-result.py @@ -0,0 +1,55 @@ +#!/usr/bin/env python3 +"""Validate the complete CI needs graph; the sole required check is CI result.""" +import json +import os +import sys + +SCOPED = {'go': 'go', 'go-postgres': 'go', 'go-server-shards': 'go', + 'go-web-parity': 'parity', 'nix': 'nix'} +REQUIRED = {'changes', 'native-smoke', 'web', 'e2e', *SCOPED} + + +def evaluate(needs, event): + errors = [] + if event not in ('push', 'pull_request'): + errors.append(f'unsupported event: {event}') + if not isinstance(needs, dict): + return ['needs must be an object'] + if set(needs) != REQUIRED: + errors.append(f'dependency mismatch: missing={sorted(REQUIRED - set(needs))}, ' + f'extra={sorted(set(needs) - REQUIRED)}') + changes = needs.get('changes', {}) + outputs = changes.get('outputs', {}) if isinstance(changes, dict) else {} + if not isinstance(outputs, dict): + outputs = {} + for key in ('go', 'web', 'nix', 'parity'): + if outputs.get(key) not in ('true', 'false'): + errors.append(f'missing/invalid changes output: {key}') + elif event == 'push' and outputs[key] != 'true': + errors.append(f'push must enable {key}') + for job in sorted(REQUIRED): + record = needs.get(job) + status = record.get('result') if isinstance(record, dict) else None + if status == 'success': + continue + if (status == 'skipped' and event == 'pull_request' and + job in SCOPED and outputs.get(SCOPED[job]) == 'false'): + continue + errors.append(f'{job}: {status!r} is not an authorized result') + return errors + + +def main(): + try: + errors = evaluate(json.loads(os.environ['CI_NEEDS']), os.environ['CI_EVENT_NAME']) + except (KeyError, ValueError) as err: + errors = [str(err)] + for error in errors: + print('::error::' + error, file=sys.stderr) + if not errors: + print('Every required CI dependency passed or was correctly scoped out.') + return bool(errors) + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/scripts/ci-result_test.py b/scripts/ci-result_test.py new file mode 100644 index 000000000..e7adb0af1 --- /dev/null +++ b/scripts/ci-result_test.py @@ -0,0 +1,88 @@ +"""The merge gate must fail closed on skipped or incomplete dependencies.""" +import importlib.util +import json +import os +from pathlib import Path +import subprocess +import sys +import unittest + +SCRIPT = Path(__file__).with_name('ci-result.py') +spec = importlib.util.spec_from_file_location('result', SCRIPT) +result = importlib.util.module_from_spec(spec) +spec.loader.exec_module(result) + + +def needs(): + data = {k: {'result': 'success'} for k in ( + 'changes', 'go', 'go-postgres', 'go-server-shards', 'go-web-parity', + 'nix', 'native-smoke', 'web', 'e2e')} + data['changes']['outputs'] = {'go': 'false', 'web': 'true', 'nix': 'false', 'parity': 'false'} + return data + + +class ResultTest(unittest.TestCase): + def test_expected_skips(self): + data = needs() + for job in ('go', 'go-postgres', 'go-server-shards', 'go-web-parity', 'nix'): + data[job]['result'] = 'skipped' + self.assertEqual(result.evaluate(data, 'pull_request'), []) + + def test_rejects(self): + cases = [] + for job, flag in [('go', 'go'), ('go-postgres', 'go'), ('go-server-shards', 'go'), + ('nix', 'nix'), ('go-web-parity', 'parity')]: + data = needs() + data['changes']['outputs'][flag] = 'true' + data[job]['result'] = 'skipped' + cases.append((f'unexpected {job} skip', data, 'pull_request')) + for job in needs(): + for status in ('failure', 'cancelled', 'unknown', None): + data = needs() + data[job]['result'] = status + cases.append((f'{job} {status}', data, 'pull_request')) + data = needs() + del data[job] + cases.append((f'missing {job}', data, 'pull_request')) + for job in ('native-smoke', 'web', 'e2e', 'changes'): + data = needs() + data[job]['result'] = 'skipped' + cases.append((f'nonlisted skip {job}', data, 'pull_request')) + for job in ('go', 'nix', 'go-web-parity'): + data = needs() + data[job]['result'] = 'skipped' + cases.append(('push skip ' + job, data, 'push')) + for flag in ('go', 'web', 'nix', 'parity'): + data = needs() + del data['changes']['outputs'][flag] + cases.append(('missing output ' + flag, data, 'pull_request')) + cases.append(('unsupported event', needs(), 'schedule')) + for name, data, event in cases: + with self.subTest(name=name): + self.assertTrue(result.evaluate(data, event)) + + def test_push(self): + data = needs() + data['changes']['outputs'] = dict.fromkeys(('go', 'web', 'nix', 'parity'), 'true') + self.assertEqual(result.evaluate(data, 'push'), []) + + def test_cli_contract(self): + for status, want in [('success', 0), ('failure', 1), ('cancelled', 1), ('skipped', 1)]: + data=needs() + data['changes']['outputs']['go']='true' + data['go']['result']=status + proc=subprocess.run([sys.executable,str(SCRIPT)], env={**os.environ, + 'CI_NEEDS':json.dumps(data), 'CI_EVENT_NAME':'pull_request'}, + capture_output=True, text=True) + self.assertEqual(proc.returncode,want,proc.stderr) + + def test_cli_malformed(self): + for raw in ('not json', 'null', '[]', '{}'): + proc = subprocess.run([sys.executable, str(SCRIPT)], env={**os.environ, + 'CI_NEEDS': raw, 'CI_EVENT_NAME': 'pull_request'}, + capture_output=True, text=True) + self.assertNotEqual(proc.returncode, 0, proc.stdout) + + +if __name__ == '__main__': + unittest.main() diff --git a/scripts/ci-scope.py b/scripts/ci-scope.py new file mode 100644 index 000000000..219b53082 --- /dev/null +++ b/scripts/ci-scope.py @@ -0,0 +1,103 @@ +#!/usr/bin/env python3 +"""Fail-closed CI scope. PRs compare exact head against merge-base(base, head). + +GO: everything outside web/. PARITY: any web/ path. WEB: always. +NIX: dependency/packaging policy, deliberately not every derivation source. +Workflows and scripts/ci-* force all jobs. Pushes always run everything. +""" +import fnmatch +import json +import os +from pathlib import Path +import re +import subprocess +import sys + +NIX_FILES = {'go.mod', 'go.sum', 'web/package.json', 'web/package-lock.json', + 'web/npm-shrinkwrap.json', 'web/yarn.lock', 'web/pnpm-lock.yaml', + 'web/bun.lock', 'web/bun.lockb'} + + +def classify(paths): + outputs = dict(go=False, web=True, nix=False, parity=False) + for path in paths: + if path.startswith(('.github/workflows/', 'scripts/ci-')): + return dict.fromkeys(outputs, True) + outputs['go'] |= not path.startswith('web/') + outputs['parity'] |= path.startswith('web/') + outputs['nix'] |= (path.startswith('nix/') or + ('/' not in path and fnmatch.fnmatchcase(path, 'flake.*')) or + path in NIX_FILES) + return outputs + + +def git(*args, cwd=None): + return subprocess.check_output(['git', *args], cwd=cwd) + + +def decode_paths(data): + if not data: + return [] # A successful, genuinely empty diff is valid. + if not data.endswith(b'\0'): + raise ValueError('diff is not NUL terminated') + paths = [os.fsdecode(p) for p in data[:-1].split(b'\0')] + if any(not p or p.startswith('/') or any(s in ('.', '..') for s in p.split('/')) + for p in paths): + raise ValueError('invalid repository-relative diff path') + return paths + + +def pr_paths(base, head, cwd=None): + for sha in (base, head): + if not isinstance(sha, str) or not re.fullmatch(r'[0-9a-fA-F]{40}', sha): + raise ValueError('PR base and head must be exact commit SHAs') + git('cat-file', '-e', sha + '^{commit}', cwd=cwd) + ancestor = git('merge-base', base, head, cwd=cwd).decode('ascii').strip() + if not re.fullmatch(r'[0-9a-fA-F]{40}', ancestor): + raise ValueError('missing or malformed merge base') + return decode_paths(git('diff', '--name-only', '-z', '--no-renames', ancestor, head, cwd=cwd)) + + +def has_local_dependencies(go_mod, package): + # A replacement can introduce extra local inputs. Until classified, keep + # Nix on for every PR; do the same for npm workspaces/file dependencies. + if re.search(r'^\s*replace\b', go_mod, re.MULTILINE) or package.get('workspaces'): + return True + return any(version.startswith(('file:', 'link:', 'workspace:')) + for kind in ('dependencies', 'devDependencies', 'optionalDependencies') + for version in package.get(kind, {}).values()) + + +def for_event(event, payload): + if event == 'push': + return dict.fromkeys(('go', 'web', 'nix', 'parity'), True) + if event != 'pull_request': + raise ValueError('unsupported CI event: ' + event) + try: + pr = payload['pull_request'] + outputs = classify(pr_paths(pr['base']['sha'], pr['head']['sha'])) + root = Path(__file__).resolve().parent.parent + if has_local_dependencies((root / 'go.mod').read_text(), + json.loads((root / 'web/package.json').read_text())): + outputs['nix'] = True + return outputs + except (KeyError, TypeError) as err: + raise ValueError('PR event lacks exact base/head SHAs') from err + + +def main(): + try: + payload = json.loads(Path(os.environ['GITHUB_EVENT_PATH']).read_text()) + outputs = for_event(os.environ['GITHUB_EVENT_NAME'], payload) + text = ''.join(f'{key}={str(value).lower()}\n' for key, value in outputs.items()) + print(text, end='') + with open(os.environ['GITHUB_OUTPUT'], 'a') as output: + output.write(text) + except (OSError, ValueError, KeyError, subprocess.CalledProcessError) as err: + print(f'::error::CI scope failed: {err}', file=sys.stderr) + return 1 + return 0 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/scripts/ci-scope_test.py b/scripts/ci-scope_test.py new file mode 100644 index 000000000..5e70c28bb --- /dev/null +++ b/scripts/ci-scope_test.py @@ -0,0 +1,100 @@ +"""Path policy and real git diff regression cases (including hostile filenames).""" +import importlib.util +import os +from pathlib import Path +import subprocess +import tempfile +import unittest +from unittest.mock import patch + +SCRIPT = Path(__file__).with_name('ci-scope.py') +spec = importlib.util.spec_from_file_location('scope', SCRIPT) +scope = importlib.util.module_from_spec(spec) +spec.loader.exec_module(scope) + + +class ScopeTest(unittest.TestCase): + def test_paths(self): + for path, want in [ + ('web/static/a', (False, True, False, True)), + ('web/src/a.ts', (False, True, False, True)), + ('internal/store/a.go', (True, True, False, False)), + ('go.mod', (True, True, True, False)), + ('go.sum', (True, True, True, False)), + ('.github/workflows/new.yml', (True, True, True, True)), + ('scripts/ci-web-parity.txt', (True, True, True, True)), + ('scripts/ci-web-census/main.go', (True, True, True, True)), + ('README.md', (True, True, False, False)), + ('nix/package.nix', (True, True, True, False)), + ('flake.lock', (True, True, True, False)), + ('web/package-lock.json', (False, True, True, True)), + ('web/package.json', (False, True, True, True)), + ('web/file with spaces\nand newline', (False, True, False, True)), + ]: + with self.subTest(path=path): + got = scope.classify([path]) + self.assertEqual(tuple(got[k] for k in ('go', 'web', 'nix', 'parity')), want) + + def test_local_dependency_guard(self): + cases = [ + ('module example.invalid/p\n', {}, False), + ('replace example.invalid/dep => ../dep\n', {}, True), + ('replace (\n example.invalid/dep => ./dep\n)\n', {}, True), + ('module example.invalid/p\n', {'workspaces': ['packages/*']}, True), + ('module example.invalid/p\n', {'dependencies': {'dep': 'file:../dep'}}, True), + ('module example.invalid/p\n', {'devDependencies': {'dep': 'workspace:*'}}, True), + ] + for go_mod, package, want in cases: + with self.subTest(go_mod=go_mod, package=package): + self.assertEqual(scope.has_local_dependencies(go_mod, package), want) + + def test_push_and_errors(self): + self.assertTrue(all(scope.for_event('push', {}).values())) + for event, payload in [('schedule', {}), ('pull_request', {}), + ('pull_request', {'pull_request': {'base': {'sha': 'bad'}, 'head': {'sha': 'bad'}}})]: + with self.subTest(event=event, payload=payload), self.assertRaises(ValueError): + scope.for_event(event, payload) + for data in [b'file', b'\0', b'../outside\0', b'/absolute\0', b'a\0\0']: + with self.subTest(data=data), self.assertRaises(ValueError): + scope.decode_paths(data) + self.assertEqual(scope.decode_paths(b''), []) + for returns in ( + [subprocess.CalledProcessError(1, 'cat-file')], + [b'', b'', subprocess.CalledProcessError(1, 'merge-base')], + [b'', b'', b'not-a-sha\n'], + [b'', b'', b'a'*40+b'\n', subprocess.CalledProcessError(1, 'diff')]): + with self.subTest(returns=returns), patch.object(scope, 'git', side_effect=returns): + with self.assertRaises((subprocess.CalledProcessError, ValueError)): + scope.pr_paths('a'*40, 'b'*40) + + def test_merge_base_deletions_and_rename(self): + with tempfile.TemporaryDirectory() as tmp: + def git(*args): + return subprocess.check_output(['git', '-C', tmp, *args]).decode().strip() + def commit(): + git('add', '-A') + git('-c', 'user.name=Test', '-c', 'user.email=test@example.invalid', 'commit', '-qm', 'fixture') + return git('rev-parse', 'HEAD') + git('init', '-q') + root = Path(tmp) + (root / 'web').mkdir() + (root / 'web' / 'old\n name').write_text('original') + ancestor = commit() + (root / 'base-only.go').write_text('base') + base = commit() + # Detached test-fixture history only; never changes the task worktree. + git('checkout', '-q', '--detach', ancestor) + (root / 'web' / 'old\n name').rename(root / 'new name.go') + commit() + (root / 'web' / 'other space').write_text('second commit') + head = commit() + paths = scope.pr_paths(base, head, cwd=tmp) + self.assertEqual(set(paths), {'web/old\n name', 'new name.go', 'web/other space'}) + self.assertTrue(scope.classify(paths)['go']) + self.assertTrue(scope.classify(paths)['parity']) + with self.assertRaises(subprocess.CalledProcessError): + scope.pr_paths('f'*40, head, cwd=tmp) + + +if __name__ == '__main__': + unittest.main() diff --git a/scripts/ci-web-census-allow.txt b/scripts/ci-web-census-allow.txt new file mode 100644 index 000000000..9e6138a0c --- /dev/null +++ b/scripts/ci-web-census-allow.txt @@ -0,0 +1,9 @@ +# Fileexact literal or embed patternreviewed reason. +embed.go all:web/build Production frontend embed; parity builds web before compiling Go. +embed.go web/build-materializer/materializer.js Production materializer embed; parity builds and runs the real bundle. +cmd/pad/cmd_server.go web/build fs.Sub of the reviewed production frontend embed, exercised by parity's web build. +internal/store/items_test.go expected false when only web/skill items exist Diagnostic source labels (web versus skill), not a repository path. +internal/server/plan3555_unguarded_write_test.go %s: cli/content=%v cli/fields=%v web/content=%v, want %v/%v/%v Diagnostic counters by API surface, not repository paths. +internal/server/bug3372_attribution_test.go %s: author=%v created_by=%v source=%v user_id=%v, want Alice/user/web/%s Diagnostic attribution values, not a repository path. +internal/materialize/materialize.go materialize: bundle did not define Materializer (a placeholder? run `node scripts/build-materializer.mjs` in web/) Build-help diagnostic; the real embedded bundle is covered by the full internal/materialize parity entry. +internal/server/bug3372_attribution_test.go create: created_by=%v source=%v, want user/web Diagnostic attribution values ending in the web source label, not a repository directory. diff --git a/scripts/ci-web-census/main.go b/scripts/ci-web-census/main.go new file mode 100644 index 000000000..f2dc26d8a --- /dev/null +++ b/scripts/ci-web-census/main.go @@ -0,0 +1,546 @@ +// ci-web-census checks all tracked Go sources, independent of build tags. +// It conservatively checks references, including diagnostic strings and fixture +// paths. Constants, concatenations and path joins are resolved without executing +// code. A dynamic suffix after a known web root is an error, even in a listed +// package. Calls and function values propagate through local and imported helpers. +package main + +import ( + "crypto/sha256" + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "os/exec" + "path" + "regexp" + "sort" + "strconv" + "strings" +) + +const module = "github.com/PerpetualSoftware/pad" +const unknown = "\x00" + +var webPath = regexp.MustCompile(`(^|[^a-zA-Z0-9_-])web/`) + +type source struct { + file, pkg string + tree *ast.File + imports map[string]string +} + +type unit struct { + src *source + name string + node ast.Node + refs map[string]bool + web, pg bool +} + +func packagePath(file string) string { + dir := path.Dir(file) + if dir == "." { + return "." + } + return "./" + dir +} + +func literal(e ast.Expr) string { + if lit, ok := e.(*ast.BasicLit); ok && lit.Kind == token.STRING { + value, _ := strconv.Unquote(lit.Value) + return value + } + return "" +} + +// resolve preserves unknown pieces rather than losing a partially known path. +// Unknown prefixes (a repository or temp root) don't hide a known web suffix; +// unknown suffixes cannot be silently accepted as a resolved reference. +func resolve(e ast.Expr, globals map[string]ast.Expr, seen map[ast.Expr]bool) string { + if e == nil || seen[e] { + return unknown + } + seen[e] = true + defer delete(seen, e) + switch x := e.(type) { + case *ast.BasicLit: + if x.Kind == token.STRING { + return literal(x) + } + case *ast.ParenExpr: + return resolve(x.X, globals, seen) + case *ast.BinaryExpr: + if x.Op == token.ADD { + return resolve(x.X, globals, seen) + resolve(x.Y, globals, seen) + } + case *ast.Ident: + if x.Obj != nil { + switch d := x.Obj.Decl.(type) { + case *ast.ValueSpec: + for i, n := range d.Names { + if n.Name == x.Name && i < len(d.Values) { + return resolve(d.Values[i], globals, seen) + } + } + case *ast.AssignStmt: + for i, n := range d.Lhs { + if n == x || identName(n) == x.Name { + if i < len(d.Rhs) { + return resolve(d.Rhs[i], globals, seen) + } + } + } + } + } + if value, ok := globals[x.Name]; ok { + return resolve(value, globals, seen) + } + case *ast.CallExpr: + if sel, ok := x.Fun.(*ast.SelectorExpr); ok { + base := identName(sel.X) + if imported, ok := globals["#import:"+base]; ok { + switch literal(imported) { + case "path/filepath": + base = "filepath" + case "path": + base = "path" + case "fmt": + base = "fmt" + default: + base = "" + } + } + if base == "fmt" && sel.Sel.Name == "Sprintf" && len(x.Args) > 0 { + format := resolve(x.Args[0], globals, seen) + args := []any{} + for _, arg := range x.Args[1:] { + value := resolve(arg, globals, seen) + if strings.Contains(value, unknown) { + return format + unknown + } + args = append(args, value) + } + return fmt.Sprintf(format, args...) + } + if (base == "filepath" || base == "path") && sel.Sel.Name == "Join" { + parts := []string{} + for _, arg := range x.Args { + parts = append(parts, resolve(arg, globals, seen)) + } + return strings.Join(parts, "/") + } + if (base == "filepath" || base == "path") && (sel.Sel.Name == "Clean" || sel.Sel.Name == "FromSlash" || sel.Sel.Name == "Abs") && len(x.Args) == 1 { + return resolve(x.Args[0], globals, seen) + } + } + } + return unknown +} + +func identName(e ast.Expr) string { + if x, ok := e.(*ast.Ident); ok { + return x.Name + } + return "" +} + +func isWeb(value string) bool { + value = strings.ReplaceAll(value, "\\", "/") + return webPath.MatchString(value) || strings.HasSuffix(value, "/web") +} + +func audit(files map[string]string, parity, allow map[string]string, pgPackages map[string]bool, sqliteAllow map[string]string) []string { + errors := []string{} + sources := []*source{} + globals := map[string]map[string]ast.Expr{} + units := []*unit{} + for file, text := range files { + tree, err := parser.ParseFile(token.NewFileSet(), file, text, parser.ParseComments) + if err != nil { + errors = append(errors, err.Error()) + continue + } + src := &source{file: file, pkg: packagePath(file), tree: tree, imports: map[string]string{}} + sources = append(sources, src) + if globals[src.pkg] == nil { + globals[src.pkg] = map[string]ast.Expr{} + } + for _, imp := range tree.Imports { + value := literal(imp.Path) + alias := path.Base(value) + if imp.Name != nil { + alias = imp.Name.Name + } + src.imports[alias] = value + if alias == "." && (value == "os" || value == "io/fs" || value == "path" || value == "path/filepath") { + errors = append(errors, file+": unresolved filesystem dot import") + } + } + for _, decl := range tree.Decls { + if gen, ok := decl.(*ast.GenDecl); ok && (gen.Tok == token.CONST || gen.Tok == token.VAR) { + for _, spec := range gen.Specs { + v := spec.(*ast.ValueSpec) + for i, n := range v.Names { + if i < len(v.Values) { + globals[src.pkg][n.Name] = v.Values[i] + } + } + } + } + } + } + for _, src := range sources { + for _, decl := range src.tree.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + units = append(units, &unit{src: src, name: d.Name.Name, node: d, refs: map[string]bool{}}) + case *ast.GenDecl: + if d.Tok == token.IMPORT { + continue + } + for _, spec := range d.Specs { + if typ, ok := spec.(*ast.TypeSpec); ok { + units = append(units, &unit{src: src, name: typ.Name.Name, node: typ.Type, refs: map[string]bool{}}) + } + if v, ok := spec.(*ast.ValueSpec); ok { + // Each binding has its own node in the reference graph, so constants + // and function aliases used from another file cannot disappear. + for i, n := range v.Names { + if i < len(v.Values) { + units = append(units, &unit{src: src, name: n.Name, node: v.Values[i], refs: map[string]bool{}}) + } + } + } + } + } + } + for _, group := range src.tree.Comments { + for _, comment := range group.List { + if !strings.HasPrefix(comment.Text, "//go:embed ") { + continue + } + for _, pattern := range strings.Fields(strings.TrimPrefix(comment.Text, "//go:embed ")) { + pattern = strings.Trim(pattern, "\"`") + if (isWeb(strings.TrimPrefix(pattern, "all:")) || pattern == "web" || pattern == "all:web") && allow[src.file+"\t"+pattern] == "" { + // Embeds cannot be selected with -run. Cover the whole package or + // explicitly review the production embed in the allow-file. + if filter, ok := parity[src.pkg]; !ok || filter != "" { + errors = append(errors, src.file+": uncovered web embed "+pattern) + } + } + } + } + } + } + byID := map[string][]*unit{} + for _, u := range units { + byID[u.src.pkg+"."+u.name] = append(byID[u.src.pkg+"."+u.name], u) + } + for _, u := range units { + bindings := map[string]ast.Expr{} + for name, value := range globals[u.src.pkg] { + bindings[name] = value + } + for alias, imported := range u.src.imports { + bindings["#import:"+alias] = &ast.BasicLit{Kind: token.STRING, Value: strconv.Quote(imported)} + } + usesSQL, pgDriverPiece := false, false + selectorNames := map[*ast.Ident]bool{} + ast.Inspect(u.node, func(n ast.Node) bool { + if n == nil { + return true + } + if expr, ok := n.(ast.Expr); ok { + value := resolve(expr, bindings, map[ast.Expr]bool{}) + if strings.Contains(value, "pgx") || strings.Contains(value, "postgres") { + pgDriverPiece = true + } + if isWeb(value) && allow[u.src.file+"\t"+value] == "" { + u.web = true + // An unknown root before web is safe for scope classification; a + // dynamic component beneath it must be made mechanically resolvable. + start := strings.Index(strings.ReplaceAll(value, "\\", "/"), "web/") + if start >= 0 && strings.Contains(value[start:], unknown) { + errors = append(errors, u.src.file+": unresolved web path in "+u.name) + } + } + } + switch x := n.(type) { + case *ast.BasicLit: + value := literal(x) + if value == "PAD_TEST_"+"POSTGRES_URL" || value == "PAD_TEST_"+"POSTGRES_TOOLS_URL" { + u.pg = true + } + case *ast.Ident: + if selectorNames[x] { + break + } + u.refs[u.src.pkg+"."+x.Name] = true + if dotted := u.src.imports["."]; strings.HasPrefix(dotted, module+"/") { + u.refs["./"+strings.TrimPrefix(dotted, module+"/")+"."+x.Name] = true + } + if x.Name == "NewPostgres" || x.Name == "testStorePostgres" || x.Name == "newPostgresConn" { + u.pg = true + } + case *ast.SelectorExpr: + selectorNames[x.Sel] = true + imported := u.src.imports[identName(x.X)] + if imported == "" { + id := u.src.pkg + "." + x.Sel.Name + for _, target := range byID[id] { + if fn, ok := target.node.(*ast.FuncDecl); ok && fn.Recv != nil { + u.refs[id] = true + } + } + } + if strings.HasPrefix(imported, module+"/") { + u.refs["./"+strings.TrimPrefix(imported, module+"/")+"."+x.Sel.Name] = true + } + if x.Sel.Name == "NewPostgres" || (strings.Contains(imported, "jackc/pgx") && + (x.Sel.Name == "Connect" || x.Sel.Name == "ConnectConfig" || x.Sel.Name == "ConnectWithOptions" || x.Sel.Name == "New" || x.Sel.Name == "NewWithConfig")) { + u.pg = true + } + case *ast.CallExpr: + if sel, ok := x.Fun.(*ast.SelectorExpr); ok { + imported := u.src.imports[identName(sel.X)] + if (imported == "os" || imported == "io/fs") && len(x.Args) > 0 { + if resolve(x.Args[0], bindings, map[ast.Expr]bool{}) == "web" { + u.web = true + } + } + if imported == "os" && (sel.Sel.Name == "Getenv" || sel.Sel.Name == "LookupEnv") && len(x.Args) > 0 { + env := resolve(x.Args[0], bindings, map[ast.Expr]bool{}) + if strings.HasPrefix(env, "PAD_TEST_POSTGRES") { + u.pg = true + } + } + if imported == "database/sql" && (sel.Sel.Name == "Open" || sel.Sel.Name == "OpenDB") { + usesSQL = true + driver := unknown + if len(x.Args) > 0 { + driver = resolve(x.Args[0], bindings, map[ast.Expr]bool{}) + } + if strings.Contains(driver, "pg") || strings.Contains(driver, "postgres") || strings.Contains(driver, unknown) { + u.pg = true + } + } + } + } + return true + }) + if usesSQL && pgDriverPiece { + u.pg = true + } + } + // Reviewed nonconnecting helpers are graph boundaries only while their + // exact source hash matches. Their own PG references still need coverage. + boundaries := map[string]bool{} + for id, review := range sqliteAllow { + if !strings.HasPrefix(review, "sha256:") { + continue + } + for _, u := range byID[id] { + text := files[u.src.file][int(u.node.Pos())-1 : int(u.node.End())-1] + want := strings.Fields(review)[0] + if fmt.Sprintf("sha256:%x", sha256.Sum256([]byte(text))) != want { + errors = append(errors, u.src.file+": reviewed nonconnecting helper changed: "+u.name) + } else { + boundaries[id] = true + } + } + } + // Resolve wrappers and function values to a fixed point, across packages too. + for changed := true; changed; { + changed = false + for _, u := range units { + for ref := range u.refs { + if boundaries[ref] { + continue + } + if sqliteAllow[u.src.pkg] != "" && ref == "./internal/store/storetest.Cleanup" { + continue + } + for _, target := range byID[ref] { + if target.pg && !u.pg { + u.pg = true + changed = true + } + if target.web && !u.web { + u.web = true + changed = true + } + } + } + } + } + // Without go/types, a selector on an imported receiver cannot be resolved + // safely. Importing a package exposing PG-capable methods requires review. + pgMethods := map[string]bool{} + for _, u := range units { + if fn, ok := u.node.(*ast.FuncDecl); ok && fn.Recv != nil && ast.IsExported(u.name) && u.pg { + pgMethods[u.src.pkg] = true + } + } + for _, src := range sources { + for _, imported := range src.imports { + pkg := "./" + strings.TrimPrefix(imported, module+"/") + if pgMethods[pkg] && !pgPackages[src.pkg] { + errors = append(errors, src.file+": unresolved PostgreSQL-capable receiver import "+imported) + } + } + // Imports of PG test helpers require review even if calls are indirect or + // unresolved. SQLite exceptions are deliberately restricted to two symbols. + for alias, imp := range src.imports { + if imp != module+"/internal/store/storetest" || pgPackages[src.pkg] { + continue + } + if sqliteAllow[src.pkg] == "" { + errors = append(errors, src.file+": unclassified PostgreSQL-capable storetest import") + continue + } + ast.Inspect(src.tree, func(n ast.Node) bool { + if sel, ok := n.(*ast.SelectorExpr); ok && identName(sel.X) == alias && sel.Sel.Name != "NewSQLite" && sel.Sel.Name != "Cleanup" { + errors = append(errors, src.file+": SQLite-only exception used "+sel.Sel.Name) + } + return true + }) + } + } + for _, u := range units { + if u.pg && !pgPackages[u.src.pkg] { + errors = append(errors, u.src.file+": unlisted PostgreSQL reference/wrapper "+u.name) + } + if !u.web { + continue + } + filter, covered := parity[u.src.pkg] + if !covered { + errors = append(errors, u.src.file+": uncovered web reference in "+u.name) + continue + } + if filter == "" { + continue + } + re, err := regexp.Compile(filter) + if err != nil { + errors = append(errors, "invalid parity regex: "+err.Error()) + continue + } + if strings.HasPrefix(u.name, "Test") && !re.MatchString(u.name) { + errors = append(errors, u.src.file+": web reader "+u.name+" is outside parity -run") + } else if !strings.HasPrefix(u.name, "Test") { + found := false + for _, caller := range units { + if caller.src.pkg == u.src.pkg && strings.HasPrefix(caller.name, "Test") && caller.web { + found = true + } + } + if !found { + errors = append(errors, u.src.file+": unresolved web helper caller "+u.name) + } + } + } + sort.Strings(errors) + return errors +} + +func readLines(file string) ([]string, error) { + data, err := os.ReadFile(file) + if err != nil { + return nil, err + } + lines := []string{} + for _, line := range strings.Split(string(data), "\n") { + if strings.TrimSpace(line) != "" && !strings.HasPrefix(strings.TrimSpace(line), "#") { + lines = append(lines, line) + } + } + return lines, nil +} + +func run() error { + lines, err := readLines("scripts/ci-web-parity.txt") + if err != nil { + return err + } + parity := map[string]string{} + for _, line := range lines { + fields := strings.Fields(line) + if len(fields) > 2 || !strings.HasPrefix(fields[0], "./") { + return fmt.Errorf("invalid parity entry: %q", line) + } + filter := "" + if len(fields) == 2 { + filter = fields[1] + if _, err := regexp.Compile(filter); err != nil { + return err + } + } + if _, exists := parity[fields[0]]; exists { + return fmt.Errorf("duplicate parity package: %s", fields[0]) + } + parity[fields[0]] = filter + } + allow := map[string]string{} + lines, err = readLines("scripts/ci-web-census-allow.txt") + if err != nil { + return err + } + for _, line := range lines { + fields := strings.SplitN(line, "\t", 3) + if len(fields) != 3 || fields[2] == "" { + return fmt.Errorf("allow entry needs file, exact pattern, reason: %q", line) + } + allow[fields[0]+"\t"+fields[1]] = fields[2] + } + pg := map[string]bool{} + lines, err = readLines("scripts/ci-pg-packages.txt") + if err != nil { + return err + } + for _, line := range lines { + pg[strings.TrimSpace(line)] = true + } + sqlite := map[string]string{} + lines, err = readLines("scripts/ci-pg-census-allow.txt") + if err != nil { + return err + } + for _, line := range lines { + fields := strings.SplitN(line, "\t", 2) + if len(fields) != 2 || fields[1] == "" { + return fmt.Errorf("SQLite allow entry needs package and reason: %q", line) + } + sqlite[fields[0]] = fields[1] + } + output, err := exec.Command("git", "ls-files", "-z", "--", "*.go").Output() + if err != nil { + return err + } + files := map[string]string{} + for _, file := range strings.Split(strings.TrimSuffix(string(output), "\x00"), "\x00") { + data, err := os.ReadFile(file) + if err != nil { + return err + } + files[file] = string(data) + } + if len(files) == 0 { + return fmt.Errorf("no tracked Go sources") + } + errors := audit(files, parity, allow, pg, sqlite) + if len(errors) > 0 { + return fmt.Errorf("source census failed:\n%s", strings.Join(errors, "\n")) + } + fmt.Printf("Web and PostgreSQL census passed (%d tracked Go files).\n", len(files)) + return nil +} + +func main() { + if err := run(); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} diff --git a/scripts/ci-web-census/main_test.go b/scripts/ci-web-census/main_test.go new file mode 100644 index 000000000..d03b4e444 --- /dev/null +++ b/scripts/ci-web-census/main_test.go @@ -0,0 +1,123 @@ +package main + +import ( + "crypto/sha256" + "fmt" + "strings" + "testing" +) + +func TestCensus(t *testing.T) { + tests := []struct { + name, source, manifest, allow string + pg bool + want string + }{ + {name: "unlisted web reader", source: `package p; import "os"; func TestRead(t *testing.T) { os.ReadFile("../../web/foo") }`, want: "web"}, + {name: "covered reader", source: `package p; func TestRead(t *testing.T) { read("../../web/foo") }`, manifest: "^TestRead$"}, + {name: "wrong test filter", source: `package p; func TestRead(t *testing.T) { read("../../web/foo") }`, manifest: "^TestOther$", want: "TestRead"}, + {name: "embed", source: "package p\n//go:embed web/build\nvar ui string", want: "web"}, + {name: "reviewed embed", source: "package p\n//go:embed web/build\nvar ui string", allow: "web/build"}, + {name: "unknown formatted path", source: `package p; import "fmt"; func TestRead(t *testing.T) { read(fmt.Sprintf("web/%s", fromEnv())) }`, manifest: ".*", want: "unresolved"}, + {name: "unknown suffix", source: `package p; func TestRead(t *testing.T) { read(filepath.Join("..", "web", fromEnv())) }`, manifest: ".*", want: "unresolved"}, + {name: "filesystem dot import", source: `package p; import . "path/filepath"; func TestRead(t *testing.T) { read(Join("..","web",fromEnv())) }`, manifest: ".*", want: "unresolved"}, + {name: "unrelated dot import", source: `package p; import . "net/http"; var method=MethodGet`}, + {name: "alias join", source: `package p; import fp "path/filepath"; const dir="web"; func TestRead(t *testing.T) { read(fp.Join("..", dir, "foo")) }`, want: "web"}, + {name: "alias unknown suffix", source: `package p; import fp "path/filepath"; const dir="web"; func TestRead(t *testing.T) { read(fp.Join("..", dir, fromEnv())) }`, manifest: ".*", want: "unresolved"}, + {name: "joined pieces", source: `package p; func TestRead(t *testing.T) { read(path.Join("..", "web", "foo")) }`, want: "web"}, + {name: "constant and concatenation", source: `package p; const root = "we" + "b/"; func TestRead(t *testing.T) { read(root + "foo") }`, want: "web"}, + {name: "helper covered", source: `package p; func helper() { read("web/foo") }; func TestRead(t *testing.T) { helper() }`, manifest: "^TestRead$"}, + {name: "helper uncovered caller", source: `package p; func helper() { read("web/foo") }; func TestRead(t *testing.T) { helper() }; func TestOther(t *testing.T) { helper() }`, manifest: "^TestRead$", want: "TestOther"}, + {name: "pg wrapper", source: `package p; func wrapper() { storetest.NewPostgres(t) }; func TestRead(t *testing.T) { wrapper() }`, want: "PostgreSQL"}, + {name: "listed pg", source: `package p; func wrapper() { storetest.NewPostgres(t) }`, pg: true}, + {name: "pg env getter alias", source: `package p; import "os"; var getter=os.Getenv; func TestRead(t *testing.T) { getter("PAD_TEST_POSTGRES_URL") }`, want: "PostgreSQL"}, + {name: "pg env", source: `package p; import "os"; var dsn = os.Getenv("PAD_TEST_POSTGRES_TOOLS_URL")`, want: "PostgreSQL"}, + {name: "PG error type cannot connect", source: `package p; import "github.com/jackc/pgx/v5/pgconn"; func check() { var err *pgconn.PgError; _=err }`}, + {name: "conditional PG driver", source: `package p; import "database/sql"; func wrapper(pg bool) { driver:="sqlite"; if pg { driver="pgx" }; sql.Open(driver,dsn) }`, want: "PostgreSQL"}, + {name: "pg driver", source: `package p; import "database/sql"; func wrapper() { sql.Open("pgx", dsn) }`, want: "PostgreSQL"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + parity := map[string]string{} + if tt.manifest != "" { + parity["./probe"] = tt.manifest + } + allow := map[string]string{} + if tt.allow != "" { + allow["probe/reader_test.go\t"+tt.allow] = "reviewed fixture" + } + pg := map[string]bool{} + if tt.pg { + pg["./probe"] = true + } + errs := audit(map[string]string{"probe/reader_test.go": tt.source}, parity, allow, pg, nil) + got := strings.Join(errs, "\n") + if tt.want == "" && got != "" { + t.Fatal(got) + } + if tt.want != "" && !strings.Contains(got, tt.want) { + t.Fatalf("wanted %q; got %q", tt.want, got) + } + }) + } +} + +func TestPGTransitiveImports(t *testing.T) { + files := map[string]string{ + "helper/helper.go": `package helper; func Open() { storetest.NewPostgres(t) }`, + "other/use_test.go": `package other; import h "github.com/PerpetualSoftware/pad/helper"; func TestUse(t *testing.T) { h.Open() }`, + } + errs := audit(files, nil, nil, map[string]bool{"./helper": true}, nil) + if !strings.Contains(strings.Join(errs, "\n"), "other") { + t.Fatalf("unlisted indirect PG user passed: %v", errs) + } +} + +func TestSQLiteException(t *testing.T) { + for _, constructor := range []string{"NewSQLite", "NewPostgres"} { + files := map[string]string{"internal/decision/use_test.go": `package decision; import "github.com/PerpetualSoftware/pad/internal/store/storetest"; func TestUse(t *testing.T) { storetest.` + constructor + `(t) }`} + errs := audit(files, nil, nil, map[string]bool{"./internal/store/storetest": true}, map[string]string{"./internal/decision": "NewSQLite only"}) + if (len(errs) == 0) != (constructor == "NewSQLite") { + t.Fatalf("%s: %v", constructor, errs) + } + } +} + +func TestReviewedNonconnectingBoundary(t *testing.T) { + body := `func Register() { sql.Open("pgx", "") }` + files := map[string]string{ + "helper/helper.go": `package helper; import "database/sql"; ` + body + `; func SQLite() { Register() }`, + "other/use_test.go": `package other; import "github.com/PerpetualSoftware/pad/helper"; func TestUse(t *testing.T) { helper.SQLite() }`, + } + review := map[string]string{"./helper.Register": fmt.Sprintf("sha256:%x registration only", sha256.Sum256([]byte(body)))} + pg := map[string]bool{"./helper": true} + if errs := audit(files, nil, nil, pg, review); len(errs) != 0 { + t.Fatal(errs) + } + files["helper/helper.go"] = strings.Replace(files["helper/helper.go"], `sql.Open("pgx", "")`, `sql.Open("pgx", ""); db.Ping()`, 1) + if errs := audit(files, nil, nil, pg, review); len(errs) == 0 { + t.Fatal("changed reviewed helper passed") + } +} + +func TestPGMethodWrapper(t *testing.T) { + files := map[string]string{ + "helper/helper.go": `package helper; type Helper struct{}; func (h Helper) Open() { storetest.NewPostgres(t) }`, + "other/use_test.go": `package other; import h "github.com/PerpetualSoftware/pad/helper"; func TestUse(t *testing.T) { x:=h.Helper{}; x.Open() }`, + } + errs := audit(files, nil, nil, map[string]bool{"./helper": true}, nil) + if !strings.Contains(strings.Join(errs, "\n"), "other") { + t.Fatalf("unlisted PG method user passed: %v", errs) + } +} + +func TestTestingCleanupIsNotStoretestCleanup(t *testing.T) { + files := map[string]string{ + "internal/store/storetest/helper.go": `package storetest; import "os"; func NewSQLite(t *testing.T) { t.Cleanup(func(){}) }; func Cleanup() { os.Getenv("PAD_TEST_POSTGRES_URL") }`, + "internal/decision/use_test.go": `package decision; import "github.com/PerpetualSoftware/pad/internal/store/storetest"; func TestUse(t *testing.T) { storetest.NewSQLite(t) }`, + } + errs := audit(files, nil, nil, map[string]bool{"./internal/store/storetest": true}, map[string]string{"./internal/decision": "NewSQLite only"}) + if len(errs) != 0 { + t.Fatal(errs) + } +} diff --git a/scripts/ci-web-parity.txt b/scripts/ci-web-parity.txt new file mode 100644 index 000000000..7e3c43a78 --- /dev/null +++ b/scripts/ci-web-parity.txt @@ -0,0 +1,14 @@ +# One package per line; optional second field is the exact go test -run regex. +# The census checks each web-referencing test and every caller of its helpers. +./cmd/pad ^TestPushRefusalCodesMatchTheWebAllowList$ +./cmd/depages +./internal/attachments ^TestMIMEFamilies(CoverAllowlist|FixtureHasNoStrays|KeysAreNormalized)$ +./internal/cli ^Test(FormatChangesForDisplayMatchesTheJSFixture|AgentNameCopyMatchesResolverOrder|DetectProjectCapturesMakeAndNestedWebSignals|BuildWorkspaceContext)$ +./internal/collab ^TestSchemaVersionMatchesWebClient$ +./internal/links ^TestMarkdownCitationsNameLiveSymbols$ +./internal/models ^Test(TerminalListsMatchWeb|NegativeTerminalsMatchTheWebCopy|TitleLimit.*)$ +./internal/server ^Test(CopyPreflightDropReasonsAreRenderedByTheDialog|SharePageDoesNotImportCollab|NotificationEnrichment_PathMatchesTheWebRouteTree|DerivedSSENamesMatchTheLegacyWireVocabulary)$ +./internal/server/render ^Test(ImgPaintableMimes_MatchesTypeScript|RenderAttachment_ParityWithTypeScript)$ +./internal/materialize +./internal/buildtools ^TestInstallRefresh_WebStampCheckUsesTheCallersCheckout$ +./scripts/ci-web-census From 458622afe69252788ef4b51506cea713dfd3397e Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 2 Nov 2026 06:46:37 +0000 Subject: [PATCH 3/6] ci: wire path-scoped CI result and main-only race workflow (TASK-4224) Keep unconditional Web, native smoke, E2E, and advisory PR checks; scope the Go, PostgreSQL, shard coverage, web parity, and Nix jobs through changes. Move Nix/Heal into CI, replace the E2E aggregate with CI result, and move server race shards to the push-only Race workflow with stop-the-line reports. Preserve race flags, assertions, timeouts, action pins, and Heal permissions. Wire scoped PR PostgreSQL packages and full traced main runs. The strict source census intentionally blocks until the diagnostic-command classification noted in the preceding commit is resolved. --- .github/workflows/ci.yml | 452 ++++++++++++++++++++++++++----------- .github/workflows/nix.yml | 239 -------------------- .github/workflows/race.yml | 153 +++++++++++++ 3 files changed, 467 insertions(+), 377 deletions(-) delete mode 100644 .github/workflows/nix.yml create mode 100644 .github/workflows/race.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 15cbe7c65..b27656570 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,8 +7,9 @@ on: branches: [main] concurrency: - group: ci-${{ github.ref }} - cancel-in-progress: true + # Per-SHA on main: never evict a pending run or its vendorHash heal. + group: ci-${{ github.event_name == 'push' && github.sha || github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} permissions: contents: read @@ -28,7 +29,51 @@ env: # execute attacker code in CI. Bump the SHA + comment together when updating. jobs: + changes: + name: changes + runs-on: ubuntu-latest + timeout-minutes: 10 + outputs: + go: ${{ steps.scope.outputs.go }} + web: ${{ steps.scope.outputs.web }} + nix: ${{ steps.scope.outputs.nix }} + parity: ${{ steps.scope.outputs.parity }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + + # Checkout's merge ref normally contains both parents. Fetch exact event + # commits too, so a moved base or fork branch cannot change classification. + - name: Ensure exact PR commits are available + if: github.event_name == 'pull_request' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + git cat-file -e "$BASE_SHA^{commit}" || git fetch --no-tags origin "$BASE_SHA" + git cat-file -e "$HEAD_SHA^{commit}" || git fetch --no-tags origin "$HEAD_SHA" + + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + + - name: Test CI policy and trace scripts + run: python3 -m unittest scripts/ci-scope_test.py scripts/ci-result_test.py scripts/ci-pg-trace_test.py + + - name: Test source census + run: go test ./scripts/ci-web-census -count=1 + + - name: Census all tracked Go sources for web and PostgreSQL references + run: go run ./scripts/ci-web-census + + - name: Classify paths + id: scope + run: python3 scripts/ci-scope.py + go: + needs: changes + if: needs.changes.outputs.go == 'true' # Backstop only. A hung TEST is caught by -timeout=45m on the steps # below, which fails fast AND prints the goroutine dump that tells # you which test hung; this cap exists because a job with no @@ -181,14 +226,9 @@ jobs: run: go test -timeout=45m ./... - name: Run tests with race detector - # Runs on both push-to-main AND pull_request. Previously gated to - # main only because GitHub Actions minutes were billed on private - # repos; the repo is public now, so PR minutes are free and we'd - # rather catch race regressions on the contributing branch than - # after merge. See BUG-1371 (also dropped test-only bcrypt cost via - # TestMain, which brought this step back under the then-30m budget — - # it did NOT raise the budget; BUG-1913 later did, 30m→45m, when the - # suite outgrew it again). + if: github.event_name == 'push' + # Dave's ruling (TASK-4224): race runs after merge on main only. + # PRs keep the non-race suite; a main failure is stop-the-line. # # Default 10m is tight: the full server-package suite under -race # measures ~13m locally on a developer laptop after BUG-851 (the @@ -214,7 +254,7 @@ jobs: # Measured: -25% on 267 serial server tests, -40% on a 40-test probe. # # internal/server is NOT in this step: it runs as the three - # go-server-race shards below (TASK-4141). Alone it took 2519.644s + # go-server-race shards in race.yml (TASK-4141). Alone it took 2519.644s # of this step's 2700s budget on main. The exclusion is asserted, # not trusted: the step fails unless exactly one package was # removed and it is internal/server. That catches a grep that @@ -453,6 +493,8 @@ jobs: run: go test ./internal/materialize/ -run '^(TestSupervisor.*|TestParseMemLimit|TestConfigFromEnv|TestParsePSRSS|TestPSRSSLive|TestParseStatVsize|TestStderrOOMMarkers|TestDeepNestFrameShallow)$' -count=1 -timeout 20m -v go-postgres: + needs: changes + if: needs.changes.outputs.go == 'true' # Backstop only. A hung TEST is caught by -timeout=45m on the steps # below, which fails fast AND prints the goroutine dump that tells # you which test hung; this cap exists because a job with no @@ -569,6 +611,7 @@ jobs: - name: Run tests against PostgreSQL env: PAD_TEST_POSTGRES_URL: "postgres://pad:pad@localhost:5432/pad?sslmode=disable" + PAD_TEST_POSTGRES_TOOLS_URL: "postgres://pad:pad@localhost:5432/pad?sslmode=disable" # Explicit -timeout: see the SQLite "Run tests" step. This is the # binary that actually blew the 10m default — internal/store creates a # fresh database AND replays the full migration chain per test @@ -579,13 +622,24 @@ jobs: # exceeded 10m, i.e. >2.14x this box on that package. (An earlier # version of this comment said "~2x slower, which put store over # 10m" — 280s x 2 is 9m20s, so that explained nothing.) - run: go test -timeout=45m ./... -count=1 + run: | + set -euo pipefail + if [ "$GITHUB_EVENT_NAME" = pull_request ]; then + mapfile -t packages < <(sed '/^[[:space:]]*#/d; /^[[:space:]]*$/d' scripts/ci-pg-packages.txt) + test "${#packages[@]}" -gt 0 + go test -timeout=45m -count=1 "${packages[@]}" + else + PAD_TEST_PG_TRACE=1 go test -json -timeout=45m -count=1 ./... \ + | tee /tmp/pg-full.jsonl \ + | python3 scripts/ci-pg-trace.py --expect scripts/ci-pg-packages.txt --show-output + fi - name: Run tests with race detector against PostgreSQL + if: github.event_name == 'push' env: PAD_TEST_POSTGRES_URL: "postgres://pad:pad@localhost:5432/pad?sslmode=disable" - # Runs on both push-to-main AND pull_request — see SQLite race-step - # comment for the public-repo / BUG-1371 reasoning. + PAD_TEST_POSTGRES_TOOLS_URL: "postgres://pad:pad@localhost:5432/pad?sslmode=disable" + # Push-to-main only, per Dave's TASK-4224 ruling. # # Headroom over the default 10m. PostgreSQL adds latency on # every CREATE/DROP, and the PLAN-866 attachment work pushed the @@ -596,7 +650,7 @@ jobs: # suite's aggregate runtime crossed the old budget (this job # variant failed main at f235a04 with the same timeout panic). # checkptr off for modernc.org/... only: see the SQLite race step (BUG-3483). - # internal/server runs as the go-server-race-postgres shards instead + # internal/server runs as the go-server-race-postgres shards in race.yml instead # (TASK-4141; it took 2658.534s of 2700s here on main). The exclusion # is asserted as in the SQLite race step: exactly one package # removed, and it is internal/server. @@ -608,114 +662,9 @@ jobs: [ "$removed" = github.com/PerpetualSoftware/pad/internal/server ] || { printf '::error::the race step must exclude exactly internal/server; it excluded:\n%s\n' "${removed:-(nothing)}"; exit 1; } go test $GO_RACE_FLAGS -timeout=45m -count=1 $pkgs - # internal/server under -race, as 3 shards per driver (TASK-4141). In the - # ./... race steps it took 2519.644s (SQLite) and 2658.534s (PostgreSQL) of - # the 2700s per-binary budget on main, sharing ~4 vCPU with every other - # package. No test or family is more than ~4% of its wall and ~88% of it is - # serial tests, so there is no slow test to fix; splitting it is the lever. - # scripts/test-shard.sh says how tests are assigned, and go-server-shards - # proves the shards cover the package exactly once. - go-server-race: - # Backstop only, as on the go job: -timeout=45m fires first. - timeout-minutes: 75 - name: Go race internal/server (SQLite ${{ matrix.shard }}/3) - runs-on: ubuntu-latest - strategy: - fail-fast: false - matrix: - shard: [1, 2, 3] - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 - with: - go-version: "1.26" - - - name: Allow Go to fetch the toolchain go.mod pins - # See the identical step in the `go` job. - run: echo "GOTOOLCHAIN=auto" >> "$GITHUB_ENV" - - - name: Create web build placeholder for embed - run: mkdir -p web/build && echo "placeholder" > web/build/.gitkeep - - - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 - with: - node-version: "24" - cache: "npm" - cache-dependency-path: web/package-lock.json - - # Built for real, as in the `go` job: internal/server's tests run it. - - name: Build the materializer bundle for embed - working-directory: web - run: npm ci && node scripts/build-materializer.mjs - - - name: Run internal/server shard with race detector - env: - SHARD: ${{ matrix.shard }} - run: | - run=$(scripts/test-shard.sh regex ./internal/server/ 3 "$SHARD" $GO_RACE_FLAGS) - go test $GO_RACE_FLAGS -timeout=45m -run "$run" ./internal/server/ - - go-server-race-postgres: - # Backstop only, as on the go-postgres job: -timeout=45m fires first. - timeout-minutes: 75 - name: Go race internal/server (PostgreSQL ${{ matrix.shard }}/3) - runs-on: ubuntu-latest - strategy: - fail-fast: false - matrix: - shard: [1, 2, 3] - services: - postgres: - # Same image and digest as the go-postgres job; bump them together. - image: mirror.gcr.io/library/postgres:17-alpine@sha256:b0f9560a2de083e2cc7382e75f808c7381a32852a7ec49117deedb300e552b24 - env: - POSTGRES_USER: pad - POSTGRES_PASSWORD: pad - POSTGRES_DB: pad - ports: - - 5432:5432 - options: >- - --health-cmd "pg_isready -U pad" - --health-interval 5s - --health-timeout 3s - --health-retries 10 - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 - with: - go-version: "1.26" - - - name: Allow Go to fetch the toolchain go.mod pins - # See the identical step in the `go` job. - run: echo "GOTOOLCHAIN=auto" >> "$GITHUB_ENV" - - - name: Create web build placeholder for embed - run: mkdir -p web/build && echo "placeholder" > web/build/.gitkeep - - - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 - with: - node-version: "24" - cache: "npm" - cache-dependency-path: web/package-lock.json - - # Built for real, as in the `go` job: internal/server's tests run it. - - name: Build the materializer bundle for embed - working-directory: web - run: npm ci && node scripts/build-materializer.mjs - - # internal/server's tests need no PostgreSQL client tools (no pg_dump - # or psql), so the PGDG install the go-postgres job does is not here. - - name: Run internal/server shard with race detector against PostgreSQL - env: - PAD_TEST_POSTGRES_URL: "postgres://pad:pad@localhost:5432/pad?sslmode=disable" - SHARD: ${{ matrix.shard }} - run: | - run=$(scripts/test-shard.sh regex ./internal/server/ 3 "$SHARD" $GO_RACE_FLAGS) - go test $GO_RACE_FLAGS -timeout=45m -count=1 -run "$run" ./internal/server/ - go-server-shards: + needs: changes + if: needs.changes.outputs.go == 'true' # The shards are only a gate if together they run every test exactly # once. A shard bug drops tests SILENTLY, which is worse than the timeout # it replaces, so this job fails the workflow unless Go's own -run @@ -822,8 +771,7 @@ jobs: # ran mobile-chromium alone, 63 passed and 553 skipped, in 1.9m). And the # projects are lopsided: desktop-chromium carried 1222s of the 1359s of # test time (572 of 635 passing tests). Desktop is therefore sharded in - # two and mobile runs whole. The "E2E (Playwright)" job below is the one - # check that says whether E2E passed. + # two and mobile runs whole. The "CI result" job below checks the combined E2E matrix result. strategy: fail-fast: false matrix: @@ -968,22 +916,250 @@ jobs: path: web/test-results/ retention-days: 7 - # The single E2E verdict. Every reader of CI (merge rules, waiters, people) - # keys on the check named "E2E (Playwright)", and the repo has no branch - # protection to list the shard jobs by name, so this job keeps that name and - # is green only when EVERY matrix job succeeded. `if: always()` is load-bearing: - # without it a failed or cancelled matrix job SKIPS this job, and a skipped check - # reads as neither red nor missing. - e2e-result: - name: E2E (Playwright) - needs: e2e + go-web-parity: + name: Go web parity + needs: changes + if: needs.changes.outputs.parity == 'true' + runs-on: ubuntu-latest + timeout-minutes: 30 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version: "24" + cache: npm + cache-dependency-path: web/package-lock.json + - name: Build web and the real materializer bundle + working-directory: web + run: npm ci && npx vite build && node scripts/build-materializer.mjs + - name: Run SQLite web parity tests from the census manifest + run: | + set -euo pipefail + while read -r package filter; do + [[ -z "$package" || "$package" == \#* ]] && continue + if [ -n "$filter" ]; then + go test -timeout=45m -count=1 "$package" -run "$filter" + else + go test -timeout=45m -count=1 "$package" + fi + done < scripts/ci-web-parity.txt + + nix: + needs: changes + if: needs.changes.outputs.nix == 'true' + name: Nix build & check + runs-on: ubuntu-latest + # NO write permission here, deliberately — see `push-vendor-hash` below. + # This job builds; it never pushes. + outputs: + vendor_hash_bumped: ${{ steps.vendorhash.outputs.bumped }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: DeterminateSystems/nix-installer-action@3138316df39ed29be04236d7ffc686fa525866aa # v23 + + # THE RED-BASELINE FIX (TASK-2954). `vendorHash` pins the Go module set by + # content hash; Dependabot updates go.mod/go.sum and has no idea this file + # exists, so EVERY Go bump PR failed the steps below on a hash mismatch, + # structurally and forever. A check that is always red is not a check — + # a bump that genuinely breaks the build looked identical, at a glance, to + # one that only moved the hash. + # + # This recomputes the hash in the WORKING TREE, so the steps below judge + # the build with this ref's actual module set. Nothing is pushed from here + # and no token is used: a Dependabot PR run gets a read-only GITHUB_TOKEN + # (it runs as if from a fork), and the `permissions` key does NOT lift that + # — only a repository-wide setting does, which would hand fork PRs write + # tokens too. The push therefore happens on `push: main` instead, in the + # job below, where the run is not Dependabot's. + # + # It runs on every run, not just Dependabot's: a human's own go.mod change + # moves the hash the same way, and a check that is honest only for one + # author is the shape this item exists to remove. + # + # It runs BEFORE `nix flake check`, which builds the package too and would + # hit the same mismatch first. It is deliberately not a gate: if the build + # fails for any reason other than a go-modules hash mismatch, the script + # exits non-zero, this step still succeeds, and the real steps below report + # the real failure. The gate stays exactly where it was. + - name: Recompute vendorHash before judging the build + id: vendorhash + run: | + set -o pipefail + if nix build .#default --print-build-logs 2>&1 | tee /tmp/nix-build.log; then + echo "Build is green; vendorHash is current." + exit 0 + fi + if ! new_hash="$(nix/bump-vendor-hash.sh /tmp/nix-build.log)"; then + echo "Not a go-modules hash mismatch — leaving it to the build steps below." + exit 0 + fi + echo "vendorHash -> $new_hash" + echo "bumped=true" >> "$GITHUB_OUTPUT" + + - name: Carry the recomputed package.nix to the push job + if: steps.vendorhash.outputs.bumped == 'true' + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: recomputed-package-nix + path: nix/package.nix + retention-days: 1 + + - name: nix flake check + run: nix flake check --print-build-logs + + - name: nix build + run: nix build .#default --print-build-logs + + - name: Smoke test binary + run: | + ./result/bin/pad --version + ./result/bin/pad --help + + # The Go toolchain here exists only to install govulncheck for the + # scan step below — the pad binary under test was built by Nix above. + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: "1.26" + + - name: Scan Nix-built binary with govulncheck + # CI's main govulncheck job scans a `go build` binary, which honours + # go.mod's `toolchain go1.26.10` line — but nixpkgs pins + # GOTOOLCHAIN=local, so the Nix artifact is built with whatever Go + # patch the nixos-26.05 channel ships and can carry stdlib advisories + # the main gate can no longer see (BUG-2567). This scans the artifact + # that actually ships via Nix, against nix/accepted-advisories.txt: + # known-accepted advisories stay green and visible, any NEW advisory + # fails the job, and a warning fires when an accepted advisory clears + # (the signal to prune the list and eventually close BUG-2567). + # + # govulncheck pinned to the same release as ci.yml — bump the two + # together. GOTOOLCHAIN=auto on the install mirrors ci.yml's + # rationale: setup-go exports GOTOOLCHAIN=local, and if govulncheck's + # own go.mod ever requires a patch newer than setup-go's resolved + # one, `auto` lets Go fetch it instead of failing the install. + run: | + GOTOOLCHAIN=auto go install golang.org/x/vuln/cmd/govulncheck@v1.2.0 + GOVULNCHECK="$(go env GOPATH)/bin/govulncheck" nix/vulnscan.sh result/bin/pad + + # THE ONLY JOB WITH A WRITE TOKEN, and it exists as a separate job precisely so + # that the token is never minted in a context an untrusted change could reach. + # + # An `if:` on a STEP is not a security boundary — the job would still hold a + # write-capable GITHUB_TOKEN, and `actions/checkout` persists it in the + # workspace for every later command, so any change to this file inside a PR + # could use it. An `if:` on a JOB is different in kind: the job does not start, + # and no write token is issued for that run at all. + # + # WHY ON `push: main` AND NOT ON THE PR (ruled on TASK-2954's trail). A + # Dependabot PR run gets a read-only token whatever `permissions` says. The + # merge, though, is authored by a human, so the `push` run that follows is an + # ordinary run with an ordinary token. The PR's own check was already honest, + # because the job above recomputes in the working tree; `main` is what needs + # the committed fix, and it gets it from the run of the merge itself — or, + # when that run loses the push race, from the run of the next push to main. + # The two rejected alternatives were a PAT in the Dependabot secret store (a + # standing credential for a once-a-week fix) and enabling write tokens for + # pull-request workflows (which would give them to FORK PRs on a public repo — + # the surface CONVE-2438 exists to keep closed). + # + # THE LOOP TERMINATES BECAUSE THIS PUSH TRIGGERS NOTHING. A push made with + # the default `GITHUB_TOKEN` creates no workflow runs, so the commit this job + # pushes gets no run at all. Observed on the first heal this fix produced + # (BUG-2974, day 62): `gh run list --branch main` spans cbfc073e -> + # 31d11e76 -> 0b16be49 and skips the heal commit 249a8f87 entirely. Read the + # BRANCH listing, not `gh run list --commit` — that query also returned + # nothing for 0b16be49, which demonstrably has runs, so it cannot tell + # "no runs" from "no answer". The loop is closed by construction rather than + # by being green. + # + # THE CORRECTED-TREE TERMINATOR IS THE BACKSTOP, AND IT HAS NEVER RUN. If + # this push did trigger a run, the build job above would find the hash + # correct, print "Build is green; vendorHash is current.", never set + # `bumped`, and this job would not start — the FIX's own effect on the build, + # holding regardless of what any push touched. That is an argument, not a + # measurement: nothing has ever exercised it. It becomes load-bearing the + # moment this push uses a PAT or a GitHub App token, which is the obvious + # edit for anyone who wants CI to validate the healed tree. Making that + # change inherits an untested terminator; exercise it before relying on it. + # + # A CONSEQUENCE OF THE SAME MECHANISM, not a defect: the healed tip carries + # no CI of its own. Its tree is the merge commit plus the one hash line that + # merge's own Nix run computed and built against, and the next push to main + # builds on it normally. + # + # THE RANGE-VS-STATE DISTINCTION IS LOAD-BEARING, AND IT WAS GOT WRONG ONCE + # (BUG-2974). This + # comment used to name the loop guard as a gate asking whether the push had + # touched `go.mod`/`go.sum`, and that gate existed as a step below. It is the + # right question for loop prevention and the WRONG one for recovery: after a + # lost push race the tree that needs healing was written by an EARLIER push, + # so every later merge that did not itself move the module set refused to fix + # it, exited GREEN, and left main carrying a hash a clean nix build rejects. + # The gate is gone. The state comparison below is the only gate, and being a + # question about state it is equally true for a hash this push broke and one + # an earlier push broke and could not land. + # + # If another merge lands while this job runs, the push is REJECTED as a + # non-fast-forward and this job goes RED rather than overwriting anything — + # and the heal is genuinely not lost now, because the next push to main meets + # the same stale hash, recomputes it, and heals it from its own run. + # + # `needs: nix` without `if: always()` is the second half: this runs only when + # the build job SUCCEEDED, so a module set whose corrected tree still fails + # `nix flake check`, the smoke test or the vulnerability scan is never + # committed to main. + push-vendor-hash: + name: Heal main's vendorHash + needs: nix + if: >- + github.event_name == 'push' && + github.ref == 'refs/heads/main' && + needs.nix.outputs.vendor_hash_bumped == 'true' + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # Nothing here reads history any more — BUG-2974 removed the gate that + # did. The depth stays full rather than dropping to the default shallow + # clone: this is the clone the heal commit is pushed from, full depth is + # what every run of this job has ever used, and a shallow push is not a + # variable worth introducing on a job that runs only when a Go bump + # actually moves the module set. + fetch-depth: 0 + + - uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 + with: + name: recomputed-package-nix + path: nix/ + + # THE GATE, and the only one, lives in the script: does what main COMMITTED + # differ from what the build job RECOMPUTED? The artifact exists only + # because the build job found a mismatch, but comparing there keeps the job + # honest about the tree it is actually looking at rather than about what + # another job reported. It is a file rather than a `run:` block so that + # nix/heal-vendor-hash_test.sh can drive it against a real git remote — + # BUG-2974 shipped green for a day because nothing could exercise it. + - name: Commit and push the recomputed hash + run: nix/heal-vendor-hash.sh + env: + SHA: ${{ github.sha }} + + ci-result: + name: CI result if: always() + needs: [changes, go, go-postgres, go-server-shards, go-web-parity, nix, native-smoke, web, e2e] runs-on: ubuntu-latest timeout-minutes: 2 steps: - - name: Every E2E job succeeded + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Require every applicable job to succeed env: - SHARDS: ${{ needs.e2e.result }} - run: | - echo "e2e matrix result: $SHARDS" - test "$SHARDS" = "success" + CI_NEEDS: ${{ toJSON(needs) }} + CI_EVENT_NAME: ${{ github.event_name }} + run: python3 scripts/ci-result.py diff --git a/.github/workflows/nix.yml b/.github/workflows/nix.yml deleted file mode 100644 index 8e7acfcbc..000000000 --- a/.github/workflows/nix.yml +++ /dev/null @@ -1,239 +0,0 @@ -name: Nix - -on: - push: - branches: [main] - pull_request: - branches: [main] - -concurrency: - # PR runs share a group per ref and supersede each other — a newer push to the - # same PR makes the older run's answer worthless. - # - # PUSH runs get a group of their OWN, one per commit, and this is deliberate - # (TASK-2954). A run on `main` may owe main a vendorHash heal, and an evicted - # run never performs it. `cancel-in-progress: false` is not enough on its own — - # GitHub holds only ONE pending run per group, so a third push evicts the - # queued second, and the eviction looks exactly like a run that decided there - # was nothing to do. A per-commit group cannot be evicted by anything. - # - # What an evicted heal costs is now DELAY, not the fix. Since BUG-2974 the - # heal is gated on the committed hash rather than on what the push touched, so - # the next push to main meets the same stale hash and heals it from its own - # run. The per-commit group is what makes the heal land on the merge that - # caused it instead of waiting for whatever lands next. - # - # What that trades for: two main runs can reach the push step at once, and the - # loser gets a non-fast-forward rejection and goes red. That is the right - # direction — a red job is read, and the next push to main heals anyway. - group: nix-${{ github.event_name == 'push' && github.sha || github.ref }} - cancel-in-progress: ${{ github.event_name == 'pull_request' }} - -permissions: - contents: read - -# All third-party Actions are pinned to a 40-char commit SHA with a trailing -# '# vX.Y.Z' comment so a compromised maintainer or moved tag cannot silently -# execute attacker code in CI. Bump the SHA + comment together when updating. - -jobs: - nix: - name: Nix build & check - runs-on: ubuntu-latest - # NO write permission here, deliberately — see `push-vendor-hash` below. - # This job builds; it never pushes. - outputs: - vendor_hash_bumped: ${{ steps.vendorhash.outputs.bumped }} - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - - uses: DeterminateSystems/nix-installer-action@3138316df39ed29be04236d7ffc686fa525866aa # v23 - - # THE RED-BASELINE FIX (TASK-2954). `vendorHash` pins the Go module set by - # content hash; Dependabot updates go.mod/go.sum and has no idea this file - # exists, so EVERY Go bump PR failed the steps below on a hash mismatch, - # structurally and forever. A check that is always red is not a check — - # a bump that genuinely breaks the build looked identical, at a glance, to - # one that only moved the hash. - # - # This recomputes the hash in the WORKING TREE, so the steps below judge - # the build with this ref's actual module set. Nothing is pushed from here - # and no token is used: a Dependabot PR run gets a read-only GITHUB_TOKEN - # (it runs as if from a fork), and the `permissions` key does NOT lift that - # — only a repository-wide setting does, which would hand fork PRs write - # tokens too. The push therefore happens on `push: main` instead, in the - # job below, where the run is not Dependabot's. - # - # It runs on every run, not just Dependabot's: a human's own go.mod change - # moves the hash the same way, and a check that is honest only for one - # author is the shape this item exists to remove. - # - # It runs BEFORE `nix flake check`, which builds the package too and would - # hit the same mismatch first. It is deliberately not a gate: if the build - # fails for any reason other than a go-modules hash mismatch, the script - # exits non-zero, this step still succeeds, and the real steps below report - # the real failure. The gate stays exactly where it was. - - name: Recompute vendorHash before judging the build - id: vendorhash - run: | - set -o pipefail - if nix build .#default --print-build-logs 2>&1 | tee /tmp/nix-build.log; then - echo "Build is green; vendorHash is current." - exit 0 - fi - if ! new_hash="$(nix/bump-vendor-hash.sh /tmp/nix-build.log)"; then - echo "Not a go-modules hash mismatch — leaving it to the build steps below." - exit 0 - fi - echo "vendorHash -> $new_hash" - echo "bumped=true" >> "$GITHUB_OUTPUT" - - - name: Carry the recomputed package.nix to the push job - if: steps.vendorhash.outputs.bumped == 'true' - uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 - with: - name: recomputed-package-nix - path: nix/package.nix - retention-days: 1 - - - name: nix flake check - run: nix flake check --print-build-logs - - - name: nix build - run: nix build .#default --print-build-logs - - - name: Smoke test binary - run: | - ./result/bin/pad --version - ./result/bin/pad --help - - # The Go toolchain here exists only to install govulncheck for the - # scan step below — the pad binary under test was built by Nix above. - - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 - with: - go-version: "1.26" - - - name: Scan Nix-built binary with govulncheck - # CI's main govulncheck job scans a `go build` binary, which honours - # go.mod's `toolchain go1.26.10` line — but nixpkgs pins - # GOTOOLCHAIN=local, so the Nix artifact is built with whatever Go - # patch the nixos-26.05 channel ships and can carry stdlib advisories - # the main gate can no longer see (BUG-2567). This scans the artifact - # that actually ships via Nix, against nix/accepted-advisories.txt: - # known-accepted advisories stay green and visible, any NEW advisory - # fails the job, and a warning fires when an accepted advisory clears - # (the signal to prune the list and eventually close BUG-2567). - # - # govulncheck pinned to the same release as ci.yml — bump the two - # together. GOTOOLCHAIN=auto on the install mirrors ci.yml's - # rationale: setup-go exports GOTOOLCHAIN=local, and if govulncheck's - # own go.mod ever requires a patch newer than setup-go's resolved - # one, `auto` lets Go fetch it instead of failing the install. - run: | - GOTOOLCHAIN=auto go install golang.org/x/vuln/cmd/govulncheck@v1.2.0 - GOVULNCHECK="$(go env GOPATH)/bin/govulncheck" nix/vulnscan.sh result/bin/pad - - # THE ONLY JOB WITH A WRITE TOKEN, and it exists as a separate job precisely so - # that the token is never minted in a context an untrusted change could reach. - # - # An `if:` on a STEP is not a security boundary — the job would still hold a - # write-capable GITHUB_TOKEN, and `actions/checkout` persists it in the - # workspace for every later command, so any change to this file inside a PR - # could use it. An `if:` on a JOB is different in kind: the job does not start, - # and no write token is issued for that run at all. - # - # WHY ON `push: main` AND NOT ON THE PR (ruled on TASK-2954's trail). A - # Dependabot PR run gets a read-only token whatever `permissions` says. The - # merge, though, is authored by a human, so the `push` run that follows is an - # ordinary run with an ordinary token. The PR's own check was already honest, - # because the job above recomputes in the working tree; `main` is what needs - # the committed fix, and it gets it from the run of the merge itself — or, - # when that run loses the push race, from the run of the next push to main. - # The two rejected alternatives were a PAT in the Dependabot secret store (a - # standing credential for a once-a-week fix) and enabling write tokens for - # pull-request workflows (which would give them to FORK PRs on a public repo — - # the surface CONVE-2438 exists to keep closed). - # - # THE LOOP TERMINATES BECAUSE THIS PUSH TRIGGERS NOTHING. A push made with - # the default `GITHUB_TOKEN` creates no workflow runs, so the commit this job - # pushes gets no run at all. Observed on the first heal this fix produced - # (BUG-2974, day 62): `gh run list --branch main` spans cbfc073e -> - # 31d11e76 -> 0b16be49 and skips the heal commit 249a8f87 entirely. Read the - # BRANCH listing, not `gh run list --commit` — that query also returned - # nothing for 0b16be49, which demonstrably has runs, so it cannot tell - # "no runs" from "no answer". The loop is closed by construction rather than - # by being green. - # - # THE CORRECTED-TREE TERMINATOR IS THE BACKSTOP, AND IT HAS NEVER RUN. If - # this push did trigger a run, the build job above would find the hash - # correct, print "Build is green; vendorHash is current.", never set - # `bumped`, and this job would not start — the FIX's own effect on the build, - # holding regardless of what any push touched. That is an argument, not a - # measurement: nothing has ever exercised it. It becomes load-bearing the - # moment this push uses a PAT or a GitHub App token, which is the obvious - # edit for anyone who wants CI to validate the healed tree. Making that - # change inherits an untested terminator; exercise it before relying on it. - # - # A CONSEQUENCE OF THE SAME MECHANISM, not a defect: the healed tip carries - # no CI of its own. Its tree is the merge commit plus the one hash line that - # merge's own Nix run computed and built against, and the next push to main - # builds on it normally. - # - # THE RANGE-VS-STATE DISTINCTION IS LOAD-BEARING, AND IT WAS GOT WRONG ONCE - # (BUG-2974). This - # comment used to name the loop guard as a gate asking whether the push had - # touched `go.mod`/`go.sum`, and that gate existed as a step below. It is the - # right question for loop prevention and the WRONG one for recovery: after a - # lost push race the tree that needs healing was written by an EARLIER push, - # so every later merge that did not itself move the module set refused to fix - # it, exited GREEN, and left main carrying a hash a clean nix build rejects. - # The gate is gone. The state comparison below is the only gate, and being a - # question about state it is equally true for a hash this push broke and one - # an earlier push broke and could not land. - # - # If another merge lands while this job runs, the push is REJECTED as a - # non-fast-forward and this job goes RED rather than overwriting anything — - # and the heal is genuinely not lost now, because the next push to main meets - # the same stale hash, recomputes it, and heals it from its own run. - # - # `needs: nix` without `if: always()` is the second half: this runs only when - # the build job SUCCEEDED, so a module set whose corrected tree still fails - # `nix flake check`, the smoke test or the vulnerability scan is never - # committed to main. - push-vendor-hash: - name: Heal main's vendorHash - needs: nix - if: >- - github.event_name == 'push' && - github.ref == 'refs/heads/main' && - needs.nix.outputs.vendor_hash_bumped == 'true' - runs-on: ubuntu-latest - permissions: - contents: write - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - # Nothing here reads history any more — BUG-2974 removed the gate that - # did. The depth stays full rather than dropping to the default shallow - # clone: this is the clone the heal commit is pushed from, full depth is - # what every run of this job has ever used, and a shallow push is not a - # variable worth introducing on a job that runs only when a Go bump - # actually moves the module set. - fetch-depth: 0 - - - uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 - with: - name: recomputed-package-nix - path: nix/ - - # THE GATE, and the only one, lives in the script: does what main COMMITTED - # differ from what the build job RECOMPUTED? The artifact exists only - # because the build job found a mismatch, but comparing there keeps the job - # honest about the tree it is actually looking at rather than about what - # another job reported. It is a file rather than a `run:` block so that - # nix/heal-vendor-hash_test.sh can drive it against a real git remote — - # BUG-2974 shipped green for a day because nothing could exercise it. - - name: Commit and push the recomputed hash - run: nix/heal-vendor-hash.sh - env: - SHA: ${{ github.sha }} diff --git a/.github/workflows/race.yml b/.github/workflows/race.yml new file mode 100644 index 000000000..361c0c013 --- /dev/null +++ b/.github/workflows/race.yml @@ -0,0 +1,153 @@ +name: Race + +on: + push: + branches: [main] + workflow_dispatch: + +concurrency: + group: race-${{ github.sha }} + cancel-in-progress: false + +permissions: + contents: read + +env: + # Keep identical to CI's shard coverage and non-server race flags. + GO_RACE_FLAGS: "-race -gcflags=modernc.org/...=-d=checkptr=0" + +jobs: + # internal/server under -race, as 3 shards per driver (TASK-4141). In the + # ./... race steps it took 2519.644s (SQLite) and 2658.534s (PostgreSQL) of + # the 2700s per-binary budget on main, sharing ~4 vCPU with every other + # package. No test or family is more than ~4% of its wall and ~88% of it is + # serial tests, so there is no slow test to fix; splitting it is the lever. + # scripts/test-shard.sh says how tests are assigned, and go-server-shards + # proves the shards cover the package exactly once. + go-server-race: + # Backstop only, as on the go job: -timeout=45m fires first. + timeout-minutes: 75 + name: Go race internal/server (SQLite ${{ matrix.shard }}/3) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + shard: [1, 2, 3] + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: "1.26" + + - name: Allow Go to fetch the toolchain go.mod pins + # See the identical step in the `go` job. + run: echo "GOTOOLCHAIN=auto" >> "$GITHUB_ENV" + + - name: Create web build placeholder for embed + run: mkdir -p web/build && echo "placeholder" > web/build/.gitkeep + + - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version: "24" + cache: "npm" + cache-dependency-path: web/package-lock.json + + # Built for real, as in the `go` job: internal/server's tests run it. + - name: Build the materializer bundle for embed + working-directory: web + run: npm ci && node scripts/build-materializer.mjs + + - name: Run internal/server shard with race detector + env: + SHARD: ${{ matrix.shard }} + run: | + run=$(scripts/test-shard.sh regex ./internal/server/ 3 "$SHARD" $GO_RACE_FLAGS) + go test $GO_RACE_FLAGS -timeout=45m -run "$run" ./internal/server/ + + - name: Stop the line on main race failure + if: failure() + env: + FAILED_JOB: Go race internal/server (SQLite ${{ matrix.shard }}/3) + run: | + echo "::error::$FAILED_JOB failed at $GITHUB_SHA" + { + echo '# STOP THE LINE: main race failure' + echo "Job: $FAILED_JOB" + echo "Commit: $GITHUB_SHA" + echo "Run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" + } >> "$GITHUB_STEP_SUMMARY" + + go-server-race-postgres: + # Backstop only, as on the go-postgres job: -timeout=45m fires first. + timeout-minutes: 75 + name: Go race internal/server (PostgreSQL ${{ matrix.shard }}/3) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + shard: [1, 2, 3] + services: + postgres: + # Same image and digest as the go-postgres job; bump them together. + image: mirror.gcr.io/library/postgres:17-alpine@sha256:b0f9560a2de083e2cc7382e75f808c7381a32852a7ec49117deedb300e552b24 + env: + POSTGRES_USER: pad + POSTGRES_PASSWORD: pad + POSTGRES_DB: pad + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U pad" + --health-interval 5s + --health-timeout 3s + --health-retries 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: "1.26" + + - name: Allow Go to fetch the toolchain go.mod pins + # See the identical step in the `go` job. + run: echo "GOTOOLCHAIN=auto" >> "$GITHUB_ENV" + + - name: Create web build placeholder for embed + run: mkdir -p web/build && echo "placeholder" > web/build/.gitkeep + + - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version: "24" + cache: "npm" + cache-dependency-path: web/package-lock.json + + # Built for real, as in the `go` job: internal/server's tests run it. + - name: Build the materializer bundle for embed + working-directory: web + run: npm ci && node scripts/build-materializer.mjs + + # internal/server's tests need no PostgreSQL client tools (no pg_dump + # or psql), so the PGDG install the go-postgres job does is not here. + - name: Run internal/server shard with race detector against PostgreSQL + env: + PAD_TEST_POSTGRES_URL: "postgres://pad:pad@localhost:5432/pad?sslmode=disable" + PAD_TEST_POSTGRES_TOOLS_URL: "postgres://pad:pad@localhost:5432/pad?sslmode=disable" + SHARD: ${{ matrix.shard }} + run: | + run=$(scripts/test-shard.sh regex ./internal/server/ 3 "$SHARD" $GO_RACE_FLAGS) + go test $GO_RACE_FLAGS -timeout=45m -count=1 -run "$run" ./internal/server/ + + + - name: Stop the line on main race failure + if: failure() + env: + FAILED_JOB: Go race internal/server (PostgreSQL ${{ matrix.shard }}/3) + run: | + echo "::error::$FAILED_JOB failed at $GITHUB_SHA" + { + echo '# STOP THE LINE: main race failure' + echo "Job: $FAILED_JOB" + echo "Commit: $GITHUB_SHA" + echo "Run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" + } >> "$GITHUB_STEP_SUMMARY" From e234251718bcbbc3f9c41c45ebbf4478ff2a4986 Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 2 Nov 2026 08:39:20 +0000 Subject: [PATCH 4/6] ci: the PG census admits a test-less operator command only while it has no tests (TASK-4224) --- scripts/ci-pg-census-allow.txt | 4 +- scripts/ci-web-census/main.go | 69 +++++++++++++++++++++++++----- scripts/ci-web-census/main_test.go | 34 +++++++++++++++ 3 files changed, 96 insertions(+), 11 deletions(-) diff --git a/scripts/ci-pg-census-allow.txt b/scripts/ci-pg-census-allow.txt index 2801fc4c6..9d05347ef 100644 --- a/scripts/ci-pg-census-allow.txt +++ b/scripts/ci-pg-census-allow.txt @@ -1,5 +1,7 @@ -# Packagereason. Valid only while storetest usage is NewSQLite/Cleanup. +# Packagereason. SQLite reviews are valid only while storetest usage is NewSQLite/Cleanup. ./internal/decision storetest.NewSQLite only; no PostgreSQL constructor. ./internal/oauth storetest.NewSQLite only; Cleanup cannot create a PostgreSQL template. +# no-tests: reviewed operator commands; valid only with zero *_test.go files and PG references. +./scripts/diagnose-version-ties no-tests: operator command; connects only to a DSN from its own flags, never under go test (BUG-3751 diagnostic). # Hash-pinned graph boundary: sql.Open here only retrieves Driver(), then closes; it never dials. ./internal/store.registerGuardedDrivers sha256:ca328ba0209b1304b14957e6e36d0f45712ae7f2731bd5190fb9a05da1224190 Driver registration for SQLite and PG only; no Ping/Conn/query. Any source change requires review. diff --git a/scripts/ci-web-census/main.go b/scripts/ci-web-census/main.go index f2dc26d8a..6a0fcf63b 100644 --- a/scripts/ci-web-census/main.go +++ b/scripts/ci-web-census/main.go @@ -151,18 +151,38 @@ func isWeb(value string) bool { return webPath.MatchString(value) || strings.HasSuffix(value, "/web") } -func audit(files map[string]string, parity, allow map[string]string, pgPackages map[string]bool, sqliteAllow map[string]string) []string { +func audit(files map[string]string, parity, allow map[string]string, pgPackages map[string]bool, pgAllow map[string]string) []string { errors := []string{} + // Operator reviews classify packages, not graph boundaries. Keep them separate + // from SQLite reviews so they cannot suppress storetest.Cleanup propagation. + noTests := map[string]bool{} + sqliteAllow := map[string]string{} + for pkg, review := range pgAllow { + if strings.HasPrefix(review, "no-tests:") { + noTests[pkg] = true + } else { + sqliteAllow[pkg] = review + } + } sources := []*source{} globals := map[string]map[string]ast.Expr{} units := []*unit{} for file, text := range files { + pkg := packagePath(file) + if _, reviewed := noTests[pkg]; reviewed && strings.HasSuffix(file, "_test.go") { + noTests[pkg] = false + errors = append(errors, file+": no-tests entry invalid: package must be added to ci-pg-packages.txt, or re-reviewed") + } tree, err := parser.ParseFile(token.NewFileSet(), file, text, parser.ParseComments) if err != nil { errors = append(errors, err.Error()) continue } - src := &source{file: file, pkg: packagePath(file), tree: tree, imports: map[string]string{}} + if _, reviewed := noTests[pkg]; reviewed && tree.Name.Name != "main" { + noTests[pkg] = false + errors = append(errors, file+": no-tests entry requires package main") + } + src := &source{file: file, pkg: pkg, tree: tree, imports: map[string]string{}} sources = append(sources, src) if globals[src.pkg] == nil { globals[src.pkg] = map[string]ast.Expr{} @@ -378,7 +398,11 @@ func audit(files map[string]string, parity, allow map[string]string, pgPackages // Without go/types, a selector on an imported receiver cannot be resolved // safely. Importing a package exposing PG-capable methods requires review. pgMethods := map[string]bool{} + pgReferences := map[string]bool{} for _, u := range units { + if u.pg { + pgReferences[u.src.pkg] = true + } if fn, ok := u.node.(*ast.FuncDecl); ok && fn.Recv != nil && ast.IsExported(u.name) && u.pg { pgMethods[u.src.pkg] = true } @@ -386,14 +410,21 @@ func audit(files map[string]string, parity, allow map[string]string, pgPackages for _, src := range sources { for _, imported := range src.imports { pkg := "./" + strings.TrimPrefix(imported, module+"/") - if pgMethods[pkg] && !pgPackages[src.pkg] { - errors = append(errors, src.file+": unresolved PostgreSQL-capable receiver import "+imported) + if pgMethods[pkg] { + pgReferences[src.pkg] = true + if !pgPackages[src.pkg] && !noTests[src.pkg] { + errors = append(errors, src.file+": unresolved PostgreSQL-capable receiver import "+imported) + } } } // Imports of PG test helpers require review even if calls are indirect or // unresolved. SQLite exceptions are deliberately restricted to two symbols. for alias, imp := range src.imports { - if imp != module+"/internal/store/storetest" || pgPackages[src.pkg] { + if imp != module+"/internal/store/storetest" { + continue + } + pgReferences[src.pkg] = true + if pgPackages[src.pkg] || noTests[src.pkg] { continue } if sqliteAllow[src.pkg] == "" { @@ -408,8 +439,13 @@ func audit(files map[string]string, parity, allow map[string]string, pgPackages }) } } + for pkg := range noTests { + if !pgReferences[pkg] { + errors = append(errors, pkg+": stale no-tests entry: package has no PostgreSQL references") + } + } for _, u := range units { - if u.pg && !pgPackages[u.src.pkg] { + if u.pg && !pgPackages[u.src.pkg] && !noTests[u.src.pkg] { errors = append(errors, u.src.file+": unlisted PostgreSQL reference/wrapper "+u.name) } if !u.web { @@ -503,7 +539,7 @@ func run() error { for _, line := range lines { pg[strings.TrimSpace(line)] = true } - sqlite := map[string]string{} + pgAllow := map[string]string{} lines, err = readLines("scripts/ci-pg-census-allow.txt") if err != nil { return err @@ -511,9 +547,22 @@ func run() error { for _, line := range lines { fields := strings.SplitN(line, "\t", 2) if len(fields) != 2 || fields[1] == "" { - return fmt.Errorf("SQLite allow entry needs package and reason: %q", line) + return fmt.Errorf("PostgreSQL allow entry needs package and reason: %q", line) + } + pgAllow[fields[0]] = fields[1] + if strings.HasPrefix(fields[1], "no-tests:") { + // Check the directory too: even untracked or ignored test files invalidate + // the review. The source audit below also checks tracked, build-tagged tests. + entries, err := os.ReadDir(fields[0]) + if err != nil { + return err + } + for _, entry := range entries { + if !entry.IsDir() && strings.HasSuffix(entry.Name(), "_test.go") { + return fmt.Errorf("%s: no-tests entry invalid: package must be added to ci-pg-packages.txt, or re-reviewed (%s exists)", fields[0], entry.Name()) + } + } } - sqlite[fields[0]] = fields[1] } output, err := exec.Command("git", "ls-files", "-z", "--", "*.go").Output() if err != nil { @@ -530,7 +579,7 @@ func run() error { if len(files) == 0 { return fmt.Errorf("no tracked Go sources") } - errors := audit(files, parity, allow, pg, sqlite) + errors := audit(files, parity, allow, pg, pgAllow) if len(errors) > 0 { return fmt.Errorf("source census failed:\n%s", strings.Join(errors, "\n")) } diff --git a/scripts/ci-web-census/main_test.go b/scripts/ci-web-census/main_test.go index d03b4e444..7d55bf5cf 100644 --- a/scripts/ci-web-census/main_test.go +++ b/scripts/ci-web-census/main_test.go @@ -83,6 +83,40 @@ func TestSQLiteException(t *testing.T) { } } +func TestPGNoTestsException(t *testing.T) { + for _, tt := range []struct { + name, testFile, source, want string + }{ + {name: "operator command without tests"}, + {name: "operator command gains tests", testFile: "operator/main_test.go", want: "ci-pg-packages.txt, or re-reviewed"}, + {name: "build excluded external test still invalidates review", testFile: "operator/main_windows_test.go", want: "ci-pg-packages.txt, or re-reviewed"}, + {name: "stale entry without PG references", source: `package main; func main() {}`, want: "stale no-tests"}, + {name: "library is not an operator command", source: `package operator; func Open() { storetest.NewPostgres(t) }`, want: "no-tests entry requires package main"}, + } { + t.Run(tt.name, func(t *testing.T) { + source := tt.source + if source == "" { + source = `package main; import "database/sql"; func main() { sql.Open("pgx", dsn) }` + } + files := map[string]string{"operator/main.go": source} + if tt.testFile != "" { + files[tt.testFile] = "package main\n" + if strings.HasSuffix(tt.testFile, "_windows_test.go") { + files[tt.testFile] = "//go:build windows\n\npackage main_test\n" + } + } + errs := audit(files, nil, nil, nil, map[string]string{"./operator": "no-tests: reviewed operator command"}) + got := strings.Join(errs, "\n") + if tt.want == "" && got != "" { + t.Fatal(got) + } + if tt.want != "" && !strings.Contains(got, tt.want) { + t.Fatalf("wanted %q; got %q", tt.want, got) + } + }) + } +} + func TestReviewedNonconnectingBoundary(t *testing.T) { body := `func Register() { sql.Open("pgx", "") }` files := map[string]string{ From 65cbd986075faa0f5822279767aace4f4a49536c Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 2 Nov 2026 13:29:44 +0000 Subject: [PATCH 5/6] ci: the PG trace guard fails on drift both ways; main race failures are loud everywhere (TASK-4224 r1) - ci-pg-trace.py --expect now fails when a listed package never connected, and when no connection was seen at all, not only on an unexpected package. An empty trace (a renamed helper, a lost env gate) used to pass vacuously. Red first: three table cases. - ci-pg-packages.txt: the list was validated by the full traced run; its CANDIDATE label is replaced by what it is and what enforces it. - ci.yml: the non-server race steps in Go and Go (PostgreSQL) get the same STOP THE LINE step race.yml has (::error:: plus a job summary), keyed on the race step's own outcome. - race.yml: the stop-the-line step also fires on a cancelled run. Claude-Session: https://claude.ai/code/session_01Pa22hLbfHJSsjBT1mPFoPF --- .github/workflows/ci.yml | 39 +++++++++++++++++++++++++++++++++++++ .github/workflows/race.yml | 8 ++++++-- scripts/ci-pg-packages.txt | 5 ++++- scripts/ci-pg-trace.py | 28 ++++++++++++++++---------- scripts/ci-pg-trace_test.py | 17 +++++++++++++--- 5 files changed, 81 insertions(+), 16 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b27656570..5ed679657 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -226,6 +226,7 @@ jobs: run: go test -timeout=45m ./... - name: Run tests with race detector + id: race if: github.event_name == 'push' # Dave's ruling (TASK-4224): race runs after merge on main only. # PRs keep the non-race suite; a main failure is stop-the-line. @@ -271,6 +272,26 @@ jobs: [ "$removed" = github.com/PerpetualSoftware/pad/internal/server ] || { printf '::error::the race step must exclude exactly internal/server; it excluded:\n%s\n' "${removed:-(nothing)}"; exit 1; } go test $GO_RACE_FLAGS -timeout=45m $pkgs + # A race failure on main is stop-the-line (Dave's TASK-4224 ruling), the + # same treatment race.yml gives the internal/server shards. Keyed on the + # race step's own outcome, so an ordinary test failure earlier in the job + # (which skips this step) is not reported as a race failure. + - name: Stop the line on main race failure + if: >- + always() && github.event_name == 'push' && + (steps.race.outcome == 'failure' || steps.race.outcome == 'cancelled') + env: + FAILED_STEP: "Go: Run tests with race detector" + OUTCOME: ${{ steps.race.outcome }} + run: | + echo "::error::$FAILED_STEP: $OUTCOME at $GITHUB_SHA" + { + echo '# STOP THE LINE: main race failure' + echo "Step: $FAILED_STEP ($OUTCOME)" + echo "Commit: $GITHUB_SHA" + echo "Run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" + } >> "$GITHUB_STEP_SUMMARY" + - name: Build binary run: go build -o pad ./cmd/pad @@ -635,6 +656,7 @@ jobs: fi - name: Run tests with race detector against PostgreSQL + id: race if: github.event_name == 'push' env: PAD_TEST_POSTGRES_URL: "postgres://pad:pad@localhost:5432/pad?sslmode=disable" @@ -662,6 +684,23 @@ jobs: [ "$removed" = github.com/PerpetualSoftware/pad/internal/server ] || { printf '::error::the race step must exclude exactly internal/server; it excluded:\n%s\n' "${removed:-(nothing)}"; exit 1; } go test $GO_RACE_FLAGS -timeout=45m -count=1 $pkgs + # Same stop-the-line treatment as the SQLite race step in the go job. + - name: Stop the line on main race failure + if: >- + always() && github.event_name == 'push' && + (steps.race.outcome == 'failure' || steps.race.outcome == 'cancelled') + env: + FAILED_STEP: "Go (PostgreSQL): Run tests with race detector against PostgreSQL" + OUTCOME: ${{ steps.race.outcome }} + run: | + echo "::error::$FAILED_STEP: $OUTCOME at $GITHUB_SHA" + { + echo '# STOP THE LINE: main race failure' + echo "Step: $FAILED_STEP ($OUTCOME)" + echo "Commit: $GITHUB_SHA" + echo "Run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" + } >> "$GITHUB_STEP_SUMMARY" + go-server-shards: needs: changes if: needs.changes.outputs.go == 'true' diff --git a/.github/workflows/race.yml b/.github/workflows/race.yml index 361c0c013..8a11dbb65 100644 --- a/.github/workflows/race.yml +++ b/.github/workflows/race.yml @@ -65,8 +65,10 @@ jobs: run=$(scripts/test-shard.sh regex ./internal/server/ 3 "$SHARD" $GO_RACE_FLAGS) go test $GO_RACE_FLAGS -timeout=45m -run "$run" ./internal/server/ + # failure() alone misses a cancelled run, which leaves main with no race + # result at this SHA: that is stop-the-line too (TASK-4224 r1 H3). - name: Stop the line on main race failure - if: failure() + if: failure() || cancelled() env: FAILED_JOB: Go race internal/server (SQLite ${{ matrix.shard }}/3) run: | @@ -139,8 +141,10 @@ jobs: go test $GO_RACE_FLAGS -timeout=45m -count=1 -run "$run" ./internal/server/ + # failure() alone misses a cancelled run, which leaves main with no race + # result at this SHA: that is stop-the-line too (TASK-4224 r1 H3). - name: Stop the line on main race failure - if: failure() + if: failure() || cancelled() env: FAILED_JOB: Go race internal/server (PostgreSQL ${{ matrix.shard }}/3) run: | diff --git a/scripts/ci-pg-packages.txt b/scripts/ci-pg-packages.txt index 962e36c35..d83a7a4ef 100644 --- a/scripts/ci-pg-packages.txt +++ b/scripts/ci-pg-packages.txt @@ -1,4 +1,7 @@ -# CANDIDATE: finalized only against a full traced PG run. +# The packages whose tests connect to the test PostgreSQL. The PR "Go (PostgreSQL)" +# job runs only these. Validated against a full traced PG run of ./... (TASK-4224). +# On push to main, ci-pg-trace.py --expect fails unless the packages that connected +# equal this list, in either direction. ./cmd/pad ./internal/appstore ./internal/mcp diff --git a/scripts/ci-pg-trace.py b/scripts/ci-pg-trace.py index 55d79e676..05a3b40b6 100755 --- a/scripts/ci-pg-trace.py +++ b/scripts/ci-pg-trace.py @@ -4,8 +4,13 @@ Usage: PAD_TEST_PG_TRACE=1 go test -json ... | python3 scripts/ci-pg-trace.py \ --expect scripts/ci-pg-packages.txt -The candidate list is evidence only; this command does not check the test run's -exit status. Preserve that separately (for example, with shell pipefail). +With --expect, the connecting packages must EQUAL the list. A package that +connected but is not listed, a listed package that never connected, or no +connection at all exits 1, so the guard cannot pass vacuously when the trace +marker stops being emitted. Without --expect it only prints the packages. + +This command does not check the test run's exit status. Preserve that +separately (for example, with shell pipefail). """ import argparse @@ -57,14 +62,17 @@ def main(): for package in sorted(connected): print(package) - if expected is not None: - for package in sorted(connected - expected): - print(f"unexpected: {package}", file=sys.stderr) - for package in sorted(expected - connected): - print(f"warning: expected package never connected: {package}", file=sys.stderr) - if connected - expected: - return 1 - return 0 + if expected is None: + return 0 + unexpected = connected - expected + missing = expected - connected + for package in sorted(unexpected): + print(f"unexpected: {package}", file=sys.stderr) + for package in sorted(missing): + print(f"expected package never connected: {package}", file=sys.stderr) + if not connected: + print("no PAD_PG_CONNECTED markers: the trace saw no connection at all", file=sys.stderr) + return 1 if unexpected or missing or not connected else 0 if __name__ == "__main__": diff --git a/scripts/ci-pg-trace_test.py b/scripts/ci-pg-trace_test.py index dbba23016..411bc4577 100644 --- a/scripts/ci-pg-trace_test.py +++ b/scripts/ci-pg-trace_test.py @@ -39,9 +39,20 @@ class TraceTest(unittest.TestCase): 1, [MODULE + "/cmd/pad", MODULE + "/internal/server"], ["unexpected: " + MODULE + "/cmd/pad", "unexpected: " + MODULE + "/internal/server", - "warning: expected package never connected: " + MODULE + "/internal/store"]), - ("missing is only a warning", [], "./internal/store\n", 0, [], - ["warning: expected package never connected: " + MODULE + "/internal/store"]), + "expected package never connected: " + MODULE + "/internal/store"]), + # A listed package that stopped connecting means the list (or the + # trace marker) drifted: fatal, not a warning (TASK-4224 r1 H1). + ("listed package missing while others connect", + [event(MODULE + "/cmd/pad")], "./cmd/pad\n./internal/store\n", + 1, [MODULE + "/cmd/pad"], + ["expected package never connected: " + MODULE + "/internal/store"]), + # No marker at all means the trace is broken (renamed helper, env + # gate lost): it must not pass vacuously. + ("empty trace fails", [], "./internal/store\n", 1, [], + ["expected package never connected: " + MODULE + "/internal/store", + "no PAD_PG_CONNECTED markers: the trace saw no connection at all"]), + ("empty trace fails even against an empty list", [], "# only comments\n", 1, [], + ["no PAD_PG_CONNECTED markers: the trace saw no connection at all"]), ("package output without a test", [{"Action": "output", "Package": MODULE + "/internal/store", "Output": "PAD_PG_CONNECTED helper=probe\n"}], "./internal/store\n", From 70365265810e6bd7b9e6432c745a0296582c5966 Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 2 Nov 2026 13:30:56 +0000 Subject: [PATCH 6/6] ci: the stop-the-line summary heading names a cancelled race run as well as a failed one (TASK-4224 r2) Claude-Session: https://claude.ai/code/session_01Pa22hLbfHJSsjBT1mPFoPF --- .github/workflows/ci.yml | 4 ++-- .github/workflows/race.yml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5ed679657..fb9f1f45c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -286,7 +286,7 @@ jobs: run: | echo "::error::$FAILED_STEP: $OUTCOME at $GITHUB_SHA" { - echo '# STOP THE LINE: main race failure' + echo '# STOP THE LINE: a main race run failed or did not finish' echo "Step: $FAILED_STEP ($OUTCOME)" echo "Commit: $GITHUB_SHA" echo "Run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" @@ -695,7 +695,7 @@ jobs: run: | echo "::error::$FAILED_STEP: $OUTCOME at $GITHUB_SHA" { - echo '# STOP THE LINE: main race failure' + echo '# STOP THE LINE: a main race run failed or did not finish' echo "Step: $FAILED_STEP ($OUTCOME)" echo "Commit: $GITHUB_SHA" echo "Run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" diff --git a/.github/workflows/race.yml b/.github/workflows/race.yml index 8a11dbb65..bf0c08452 100644 --- a/.github/workflows/race.yml +++ b/.github/workflows/race.yml @@ -74,7 +74,7 @@ jobs: run: | echo "::error::$FAILED_JOB failed at $GITHUB_SHA" { - echo '# STOP THE LINE: main race failure' + echo '# STOP THE LINE: a main race run failed or did not finish' echo "Job: $FAILED_JOB" echo "Commit: $GITHUB_SHA" echo "Run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" @@ -150,7 +150,7 @@ jobs: run: | echo "::error::$FAILED_JOB failed at $GITHUB_SHA" { - echo '# STOP THE LINE: main race failure' + echo '# STOP THE LINE: a main race run failed or did not finish' echo "Job: $FAILED_JOB" echo "Commit: $GITHUB_SHA" echo "Run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID"