diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 15cbe7c65..fb9f1f45c 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,10 @@ 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). + 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. # # Default 10m is tight: the full server-package suite under -race # measures ~13m locally on a developer laptop after BUG-851 (the @@ -214,7 +255,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 @@ -231,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: 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" + } >> "$GITHUB_STEP_SUMMARY" + - name: Build binary run: go build -o pad ./cmd/pad @@ -453,6 +514,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 +632,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 +643,25 @@ 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 + id: race + 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 +672,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 +684,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 -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 + # 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: - SHARD: ${{ matrix.shard }} + FAILED_STEP: "Go (PostgreSQL): Run tests with race detector against PostgreSQL" + OUTCOME: ${{ steps.race.outcome }} 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/ + echo "::error::$FAILED_STEP: $OUTCOME at $GITHUB_SHA" + { + 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" + } >> "$GITHUB_STEP_SUMMARY" 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 +810,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 +955,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..bf0c08452 --- /dev/null +++ b/.github/workflows/race.yml @@ -0,0 +1,157 @@ +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/ + + # 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() || cancelled() + env: + FAILED_JOB: Go race internal/server (SQLite ${{ matrix.shard }}/3) + run: | + echo "::error::$FAILED_JOB failed at $GITHUB_SHA" + { + 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" + } >> "$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/ + + + # 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() || cancelled() + env: + FAILED_JOB: Go race internal/server (PostgreSQL ${{ matrix.shard }}/3) + run: | + echo "::error::$FAILED_JOB failed at $GITHUB_SHA" + { + 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" + } >> "$GITHUB_STEP_SUMMARY" 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-census-allow.txt b/scripts/ci-pg-census-allow.txt new file mode 100644 index 000000000..9d05347ef --- /dev/null +++ b/scripts/ci-pg-census-allow.txt @@ -0,0 +1,7 @@ +# 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-pg-packages.txt b/scripts/ci-pg-packages.txt new file mode 100644 index 000000000..d83a7a4ef --- /dev/null +++ b/scripts/ci-pg-packages.txt @@ -0,0 +1,10 @@ +# 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 +./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..05a3b40b6 --- /dev/null +++ b/scripts/ci-pg-trace.py @@ -0,0 +1,79 @@ +#!/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 + +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 +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") + parser.add_argument("--show-output", action="store_true", + help="also stream human-readable go test Output events") + 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 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")): + 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 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__": + 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..411bc4577 --- /dev/null +++ b/scripts/ci-pg-trace_test.py @@ -0,0 +1,88 @@ +"""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", + "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", + 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) + + +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..6a0fcf63b --- /dev/null +++ b/scripts/ci-web-census/main.go @@ -0,0 +1,595 @@ +// 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, 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 + } + 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{} + } + 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{} + 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 + } + } + for _, src := range sources { + for _, imported := range src.imports { + pkg := "./" + strings.TrimPrefix(imported, module+"/") + 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" { + continue + } + pgReferences[src.pkg] = true + if pgPackages[src.pkg] || noTests[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 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] && !noTests[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 + } + pgAllow := 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("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()) + } + } + } + } + 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, pgAllow) + 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..7d55bf5cf --- /dev/null +++ b/scripts/ci-web-census/main_test.go @@ -0,0 +1,157 @@ +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 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{ + "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