Skip to main content

DB workflow rationale

This history was moved out of .github/workflows/db.yml when the workflow reached GitHub’s 500 KiB workflow-file limit. It remains the rationale for the load-bearing filter, gate-integrity, and derived-coverage controls.

Path-filter strategy (issue #720 regression + #654 original problem): Branch protection requires "Migrate Up + Down (PostgreSQL 16 + pgvector)" and "Schema drift check" on every PR. We previously used a paths: filter

  • companion db-skip.yml with paths-ignore: to cover both cases. That design fails on zero-file-diff PRs (e.g. #720 after its fix independently landed on main): neither paths: nor paths-ignore: matches when the diff is empty, so neither workflow fires and the required check stays "expected" forever.

Fix: mirror the security-scan.yml pattern. Trigger on every PR with no paths: filter. An internal changes job uses dorny/paths-filter to detect whether DB work is needed; heavy steps gate on the output.

ALLOW-LIST → DENY-LIST for the db filter (issue #2465): Until #2465 the db filter was a hand-maintained ALLOW-list of ~20 globs. A path-filtered step SKIPs rather than fails, and this job is a REQUIRED status check, so every path outside that list produced a load-bearing green from a job that stood up no database and ran no integration suite.

The list's own inline comments memorialised THREE separate escapes, each appended retroactively after someone noticed: internal/workflow/** (WF-28/#1906), cmd/agent-orchestrator/** + internal/temporalwf/** (#1881), and the four-package memory-recall set (#2296, whose defect "survived three PRs and a full dark deploy precisely because nothing executed it"). Three recorded escapes from one filter is not a maintenance lapse; enumeration is the wrong instrument.

It was also wrong on its own terms, and by a wide margin. This job runs ten go test invocations (see the steps below). Their first-party dependency closure — go list -tags integration -deps -test over all ten package globs — is 159 of the module's 289 packages, spanning 38 internal|cmd|pkg| test trees. The allow-list named 15. Uncovered by it and covered now: internal/gateway/** — incl. auth/bypass.go, the app.org_id GUC replant every RLS assertion in this lane depends on internal/rbac/, internal/auth/, internal/governance/, internal/task/, internal/agent/, internal/dashboard/, internal/metrics/, internal/knowledge/, internal/mcp/, internal/bus/, internal/testsupport/, pkg/*pb/, and 20 more — and internal/context/store/** ITSELF: only its migrations/ subdirectory was listed, never the Go SQL it migrates for. Also absent: Makefile (which defines the migrate-up/migrate-down targets the job invokes) and hack/verify-app-rw.sh.

So the filter is now a DENY-list: '**' plus explicit negations, evaluated with predicate-quantifier: 'every'. New packages, new SQL, new migrations and new integration suites are covered the day they land, instead of being skip-green until somebody remembers. Each exclusion carries its reason. Same idiom as wave2-smoke.yml (#2449) so the two required checks share one auditable shape.

MEASURED COST — this widening is NOT free, unlike #2449's (2026-08-03, 150 merged PRs replayed through dorny v3's exact matcher; 120 sampled job runs for durations): trigger rate 68.7% → 93.3% of PRs (37 PRs / 24.7% newly gated) job duration 550s median when it runs, 23s when it skips runner-minutes 642 → 858 per 100 PR runs (+216, +34%) required-check critical path, repo-wide median: UNCHANGED at 550s — this job already dominated on the >50% of PRs where it ran. required-check critical path on the 37 newly-gated PRs: 262s → 550s median (+288s). Zero of the 37 had another required check at or above 550s, so every one of them gets longer. That is the price of them being gated at all; do not buy it back by narrowing.

Widening this filter also strengthens #2449/PR #2464's cost claim rather than weakening it: that PR's "critical path unchanged because Migrate Up + Down already dominates" now holds on 93% of PRs instead of 69%.

The drift filter below is deliberately left as an allow-list: Schema drift check is not a required status check, and its input set really is small and derivable (the tool walks migration text plus the Go registry, with no database). Two justified shapes beat one forced one.

GATE-JOB FAILURE MUST NOT SKIP-GREEN THIS CHECK (issue #2473): #2465 (above) made the db FILTER honest. It did nothing about the changes JOB, which was still a single point of failure: needs: changes alone means that when changes FAILS — rather than reporting "nothing DB-relevant" — GitHub never starts migrations, so it reports skipped, and a skipped required check SATISFIES branch protection. Every step-level if: and every anti-skip NEEDLE guard in this file is downstream of that and cannot see it: the failure is one level up, at the job, so none of those steps exist to run.

Observed on 2026-07-31 (#2389, org Actions quota exhausted): every gate job died at start, all six required checks reported skipped, and PRs read mergeable with zero CI executed. The deny-list conversion did not help, because the filter never got to be evaluated.

Fix on migrations below, a pair that must be read together: 1. if: ${{ always() }} at job level so it STARTS whatever changes concluded; the job can then never report skipped. 2. A Gate integrity FIRST step failing closed unless every upstream job reached success AND changes.db is a literal 'true'/'false'. Without (2), (1) is worse than the original bug: a failed gate leaves needs.changes.outputs.db EMPTY, all ~30 gated steps below evaluate false, and this check reports green having stood up no database — the precise outcome #2465 was filed to end.

always() not !cancelled(): the latter SKIPS on cancelled runs, and skipped satisfies branch protection — the same defect, narrower.

This composes with, and does not replace, the Guard — the db path filter must stay a deny-list step in the changes job. That guard pins WHAT the filter decides; the pair above pins that a decision was made AT ALL. Note the ordering dependency the guard alone has: it lives in changes, which is not a required context, so if someone removed if: always() here the guard could go red without blocking anything. That is why the invariant is ALSO asserted statically by scripts/check-required-check-gating.py.

schema-drift is deliberately NOT converted: Schema drift check is not a required status check, so its skipping is a cost matter, not a branch protection bypass. If it is ever promoted, apply the pair first — exactly as the note on the drift allow-list above already says for the filter shape.

THE SECOND ALLOW-LIST IS ALSO GONE — -run (issues #2848, #2904): The db path filter above was one enumeration. Run integration tests carried another: a hand-maintained go test -run alternation, 31 entries, over test/integration/context. It failed the same way — a name absent from it did not run, and the REQUIRED check reported success anyway, with no skip, no warning and no count.

What it concealed, all found by inspection rather than by any lane: * 13 tests that had never executed in CI, 8 of them broken (#2849); * TestActivityFeedV1_ClearanceMatrix, whose B_L5_noLeakFromA case describes the exact cross-tenant read activity_feed_v1 shipped for 144 migrations (#2846); * a whole family (#2840's) registered so late that nothing in it had ever run on a board that was green for it; * TestSummaryLoader_KeepsNewestN_Chronological, which did not merely fail but PANICKED, truncating the run — repaired by #2903 (T1). It was also the ONE line in this file that git merge conflicts on, so each union-resolution was an opportunity to delete another PR's coverage silently.

And it bought nothing. Measured on the CI recipe (fresh pgvector:pg16, chain 222, post-T1): filtered 338 === RUN / 69.5s, unfiltered 345 / 68.8s. The "unfiltered is slower" belief came from a run a panic had truncated.

Replacement, same instrument as the deny-list conversion — derive, do not enumerate: test/lint/db_lane_coverage_test.go asserts that every func Test* in that package, under BOTH build-tag states, executes in at least one workflow invocation. It parses this file for the invocations and AST-walks the package for the population, so it cannot agree with CI by construction. Re-adding a -run here is not banned; it turns that check RED, by name, for every test the filter would newly exclude.

STILL OPEN and deliberately NOT closed here: 27 tests in that package sit behind //go:build integration and are invocable in NO lane (#2827). They pass when run alone; they cannot yet be composed into this lane because migration 126's down is non-total once mcp_servers holds a soft-deleted row whose only binding is also soft-deleted (#2952). Quarantined BY NAME in that check, which fails if the set grows or shrinks.

Refs: issue #654 (original path-filter problem), issue #720 (zero-file diff regression), issue #2465 (allow-list skip-as-pass, this fix), issue #2449 / PR #2464 (same defect class on Compose Smoke Test), issue #2473 (gate-job failure skip-green — the always() + Gate integrity pair), #2389 (the Actions quota incident that made it observed), #2848 / #2904 (the -run allow-list deletion + derived lane-coverage check), #2827 / #2849 / #2872 (the classes it closes or hands on).