Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .claude/skills/kagent-dev/references/database-migrations.md
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,11 @@ These tests catch policy violations at PR time without needing a running databas

## Upgrade and rollback testing

Static analysis covers file *content*; round-trip tests cover *behavior* against a real Postgres. Beyond `runner_test.go` (rollback and concurrency), two release-to-release tests make the rollback promise real. Both are *Target — not yet enforced*.
Static analysis covers file *content*; round-trip tests cover *behavior* against a real Postgres. Beyond `runner_test.go` (rollback and concurrency), release-to-release coverage makes the rollback promise real.

**Previous-minor round-trip.** Seed a database at the previous minor's latest release with representative data, apply migrations up to `HEAD`, and assert the schema matches a clean `HEAD` install and the data survives; then reverse to the previous minor and assert the schema matches a clean previous-minor install and the data survives. This exercises every changed down file rather than only reviewing it.
**Previous-minor round-trip** (*Target — not yet enforced*). Seed a database at the previous minor's latest release with representative data, apply migrations up to `HEAD`, and assert the schema matches a clean `HEAD` install and the data survives; then reverse to the previous minor and assert the schema matches a clean previous-minor install and the data survives. This exercises every changed down file rather than only reviewing it.

**Query-level backward compatibility.** Run the previous minor's database test suite against a `HEAD`-migrated schema, proving old code's queries run against the newer schema — the exact property [ahead-schema tolerance](#rollback-and-ahead-schema-tolerance) relies on.
**Query-level backward compatibility.** A static check — `scripts/check-query-contraction.sh`, run by the `query-contraction-check` CI job — compiles a previous release's sqlc queries against the `HEAD` schema and fails if a migration dropped, renamed, or retyped a column or table an older query still reads. It catches column/table/type-shape contraction with no database, against two prior versions: the latest release reachable from `HEAD` and the previous stable line's latest patch (the `release/vX.Y.x` tip, via `scripts/prev-stable-version.sh`). The fuller property — running the previous minor's whole database *test suite* against a `HEAD`-migrated schema, which also covers semantic breaks a query still compiles against — remains a *Target — not yet enforced*.

## Downstream Extension Model

Expand Down
25 changes: 22 additions & 3 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,25 @@ jobs:
echo "::error::Kubectl logs -n kagent deployment/kagent-controller"
kubectl logs -n kagent deployment/kagent-controller

query-contraction-check:
name: Query Contraction Check
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v6
with:
# Full history + tags so the previous release's queries can be read.
fetch-depth: 0
fetch-tags: true
- name: Set up Go
uses: actions/setup-go@v6
with:
go-version-file: go/go.mod
cache: true
cache-dependency-path: go/go.sum
- name: Previous-release queries vs current schema
run: make -C go check-query-contraction

go-unit-tests:
runs-on: ubuntu-latest
steps:
Expand All @@ -169,7 +188,7 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v6
with:
go-version: "1.26"
go-version-file: go/go.mod
Comment thread
iplay88keys marked this conversation as resolved.
cache: true
cache-dependency-path: go/go.sum

Expand Down Expand Up @@ -302,7 +321,7 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v6
with:
go-version: "1.26"
go-version-file: go/go.mod
cache: true
cache-dependency-path: go/go.sum
- name: golangci-lint
Expand Down Expand Up @@ -375,7 +394,7 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v6
with:
go-version: "1.26"
go-version-file: go/go.mod
cache: true
cache-dependency-path: go/go.sum

Expand Down
8 changes: 8 additions & 0 deletions go/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@ generate: controller-gen sqlc-generate ## Generate DeepCopy methods and sqlc que
sqlc-generate: sqlc ## Generate type-safe Go code from SQL queries.
cd core/internal/database && $(SQLC) generate

# Compile the previous release's sqlc queries against the current schema (the
# migration files) and fail if a migration removed/renamed/retyped a column or
# table an old query still uses. Static (no database/cluster). Needs git tags
# fetched. TARGET_VERSIONS (space-separated) overrides the auto-derived release(s).
.PHONY: check-query-contraction
check-query-contraction: sqlc ## Verify the previous release's queries still type-check against the current schema
SQLC=$(SQLC) ../scripts/check-query-contraction.sh

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we just put this script in go/scripts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, I can move it on the other PR.


##@ Development

.PHONY: fmt
Expand Down
8 changes: 8 additions & 0 deletions go/core/internal/database/sqlc.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
# `schema` points at the migration files, so `sqlc generate` validates every
# query against the schema those migrations produce. This is a load-bearing
# migration-safety gate: a migration that drops/renames/retypes a column
# a current query uses makes codegen fail, and the manifests-check CI job catches
# the resulting drift. Do not "fix" such a failure by deleting the query — it is
# reporting a real schema/query incompatibility. The previous-release direction
# (old queries vs the new schema = the windowed-contraction invariant) is checked
# by scripts/check-query-contraction.sh.
version: "2"
sql:
- schema: ["../../pkg/migrations/core", "../../pkg/migrations/vector"]
Expand Down
125 changes: 125 additions & 0 deletions scripts/check-query-contraction.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
#!/usr/bin/env bash
# check-query-contraction.sh — query-contraction check.
#
# Compiles a PREVIOUS release's sqlc queries against the CURRENT schema (the
# migration files under go/core/pkg/migrations) and fails if a migration on this
# branch removed, renamed, or retyped a column or table that an older query still
# references — a change that would break that release's code against the new
# schema (the windowed-contraction invariant).
#
# It checks two targets, deduplicated:
# A) the latest released tag reachable from HEAD — the in-line previous release;
# catches a contraction introduced during the current line's development.
# B) the previous stable line's latest patch (release/vX.Y.x tip, via
# prev-stable-version.sh) — the supported rollback-window floor.
# Today these usually resolve to the same tag (one compile); they diverge once a
# new minor releases or the stable line gets a backport patch.
#
# Static: no database and no cluster. sqlc derives the schema from the migration
# files (see go/core/internal/database/sqlc.yaml), so "does every old query still
# type-check against the new schema" is answerable offline. It catches
# column/table/type-shape contraction; semantic breaks (a new NOT NULL, a
# tightened constraint, an index/ordering change) are out of scope for a static
# check and belong to a runtime regression suite.
#
# Inputs (env):
# TARGET_VERSIONS space-separated versions without leading 'v' to check
# instead of the auto-derived A/B (for local runs).
# SQLC sqlc binary to use (default: sqlc on PATH).
# REMOTE git remote for target B (default: origin).
set -euo pipefail

here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
repo_root="$(git -C "$here" rev-parse --show-toplevel)"
sqlc_bin="${SQLC:-sqlc}"
queries_path="go/core/internal/database/queries"
core_migrations="$repo_root/go/core/pkg/migrations/core"
vector_migrations="$repo_root/go/core/pkg/migrations/vector"

# Resolve the target versions.
targets=()
if [ -n "${TARGET_VERSIONS:-}" ]; then
read -ra targets <<<"${TARGET_VERSIONS}"
else
a="$(git -C "$repo_root" describe --tags --abbrev=0 2>/dev/null | sed 's/^v//' || true)"
b="$("$here/prev-stable-version.sh" 2>/dev/null || true)"
[ -n "${a}" ] && targets+=("${a}")
[ -n "${b}" ] && targets+=("${b}")
fi
if [ "${#targets[@]}" -eq 0 ]; then
echo "ERROR: no contraction target versions resolved; ensure tags are fetched and a release branch exists, or set TARGET_VERSIONS." >&2
exit 1
fi
# Deduplicate, preserving order (version strings are space- and glob-free).
targets=($(printf '%s\n' "${targets[@]}" | awk 'NF && !seen[$0]++'))

workroot="$(mktemp -d)"
trap 'rm -rf "$workroot"' EXIT

# A target resolved (non-empty version) but whose tag is absent locally means
# the checkout didn't fetch tags — a misconfiguration that would otherwise let
# the whole check pass having compiled nothing. Track the two outcomes so the
# post-loop guard can fail on that case while still allowing a legitimately
# empty run (every resolved target predates the sqlc query set).
compiled=0
missing_tag=0

check_target() {
local prev="$1"
local prev_tag="v${prev}"

if ! git -C "$repo_root" rev-parse -q --verify "refs/tags/${prev_tag}" >/dev/null; then
echo "NOTE: tag ${prev_tag} not present locally; skipping (fetch tags to include it)."
missing_tag=$((missing_tag + 1))
return 0
fi
if [ -z "$(git -C "$repo_root" ls-tree "$prev_tag" -- "$queries_path" 2>/dev/null)" ]; then
echo "NOTE: ${prev_tag} has no ${queries_path}; skipping (predates the sqlc query set)."
return 0
fi

# Self-contained sqlc project: sqlc resolves schema/queries relative to the
# config file, so stage everything under a per-target dir. Current migrations
# supply the schema; the previous release supplies the queries.
local wd="$workroot/$prev"
mkdir -p "$wd/schema/core" "$wd/schema/vector" "$wd/queries" "$wd/gen" "$wd/prev"
cp "$core_migrations"/*.sql "$wd/schema/core/"
cp "$vector_migrations"/*.sql "$wd/schema/vector/"
git -C "$repo_root" archive "$prev_tag" "$queries_path" | tar -x -C "$wd/prev"
cp "$wd/prev/$queries_path"/*.sql "$wd/queries/"

# Minimal config: the go_type overrides in the real sqlc.yaml only affect the
# generated Go types, not whether a query type-checks against the schema.
cat >"$wd/sqlc.yaml" <<'EOF'
version: "2"
sql:
- engine: "postgresql"
schema: ["schema/core", "schema/vector"]
queries: "queries"
gen:
go:
package: "dbgen"
out: "gen"
EOF

echo "=== Contraction check: queries@${prev_tag} vs current schema ==="
( cd "$wd" && "$sqlc_bin" compile -f sqlc.yaml )
echo "OK: ${prev_tag} queries still type-check against the current schema."
compiled=$((compiled + 1))
}

for t in "${targets[@]}"; do
check_target "$t"
done

# Guard against a vacuous green: if nothing compiled because resolved targets had
# no local tag, the checkout almost certainly didn't fetch tags. Fail loudly
# rather than report success on an empty run. An all-predate run (no missing
# tags) is legitimately empty and stays green.
if [ "$compiled" -eq 0 ]; then
if [ "$missing_tag" -gt 0 ]; then
echo "ERROR: no targets compiled — ${missing_tag} resolved version(s) had no local tag; fetch tags (fetch-depth: 0, fetch-tags: true) so the contraction check actually runs." >&2
exit 1
fi
echo "NOTE: no targets compiled; all resolved versions predate the sqlc query set."
fi
66 changes: 66 additions & 0 deletions scripts/prev-stable-version.sh

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be used by both this PR and the upgrade tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is shared and will be removed from the other PR!

Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#!/usr/bin/env bash
# prev-stable-version.sh — prints the latest released patch of the stable line
# immediately BELOW the line currently being built: the highest
# vMAJOR.MINOR.PATCH tag on the newest release/vMAJOR.MINOR.x branch whose
# MAJOR.MINOR is strictly less than the current line. This is the rollback-window
# floor a contraction must stay compatible with.
#
# The current line comes from the base/target branch:
# - release/vX.Y.x -> current line is X.Y, so this resolves to the newest
# release line below X.Y (release/v0.9.x -> 0.8.x).
# - main (or any non-release branch) -> the unreleased next minor, which sorts
# above every release line, so this resolves to the newest
# release line overall (-> 0.9.x).
# Source order for the current ref: CURRENT_REF override, GITHUB_BASE_REF (PR
# target), GITHUB_REF_NAME (push), then the checked-out branch.
#
# Prints nothing and exits 0 when no stable line exists below the current one
# (e.g. building the oldest release line), so the caller can skip that target.
# Uses `git ls-remote`, so it needs network to the remote but not the branch
# checked out locally. Output has no leading 'v'. Override the remote with REMOTE.
set -euo pipefail

remote="${REMOTE:-origin}"

# Current line MAJOR.MINOR, or empty for main / any non-release line.
current_ref="${CURRENT_REF:-${GITHUB_BASE_REF:-${GITHUB_REF_NAME:-$(git rev-parse --abbrev-ref HEAD 2>/dev/null || true)}}}"
current_minor=""
if [[ "${current_ref}" =~ ^release/v([0-9]+\.[0-9]+)\.x$ ]]; then
current_minor="${BASH_REMATCH[1]}"
fi

# All release lines on the remote, ascending by version (MAJOR.MINOR only).
lines="$(git ls-remote --heads "$remote" 'refs/heads/release/v*' 2>/dev/null \
| sed -nE 's#.*refs/heads/release/v([0-9]+\.[0-9]+)\.x$#\1#p' \
| sort -V)"
if [ -z "${lines}" ]; then
echo "ERROR: no release/vMAJOR.MINOR.x branch found on ${remote}" >&2
exit 1
fi

# ver_lt A B -> success when A < B by version sort.
ver_lt() { [ "$1" != "$2" ] && [ "$(printf '%s\n%s\n' "$1" "$2" | sort -V | head -1)" = "$1" ]; }

# Highest line strictly below the current line. With no current_minor (main /
# next), every line qualifies, so this lands on the newest line overall.
prev_minor=""
for l in ${lines}; do
if [ -z "${current_minor}" ] || ver_lt "$l" "${current_minor}"; then
prev_minor="$l"
fi
done
if [ -z "${prev_minor}" ]; then
# No stable line below the current one; let the caller skip that target.
exit 0
fi

esc="${prev_minor//./\\.}"
latest="$(git ls-remote --tags "$remote" 2>/dev/null \
| grep -oE "refs/tags/v${esc}\.[0-9]+$" \
| sed 's#refs/tags/v##' | sort -V | tail -1)"
if [ -z "${latest}" ]; then
echo "ERROR: no v${prev_minor}.PATCH release tags found on ${remote} (fetch tags?)" >&2
exit 1
fi

echo "${latest}"
Loading