[ac-pplx]

Summary

A planned change renames sandbox_k8s_executions to sandbox_execution_records and node_k8s to node_identities in place, leaving compatibility views under the old names. This change adds the test infrastructure to check, before those migrations are written, that today's code works unchanged through the views. It changes no migration and no production code.

• Opt-in schema variant. pplx-space-db-test-fixtures applies a candidate DDL file to the migrated test template after the migrations when SPACE_TEST_POSTGRES_SCHEMA_VARIANT=execution-record-views is set. The file is crates/test_support/db/test/fixtures/schema-variants/execution_record_views.sql, marked as a test candidate and not a migration. Unset or empty, the template is unchanged. An unknown name, or a value that is not valid UTF-8, fails template creation before the migrations run, so a typo cannot pass as a variant run. The candidate renames the tables and pod_materialized_at, k8s_node_name and k8s_node_uid, adds the new columns, makes pod_name and pod_namespace nullable, and creates the views. The views keep the old column lists in order, including the three runtime_upgrade_* execution columns from [db] record a stay-stopped runtime upgrade decision on its Kubernetes execution #1304, and use WITH CASCADED CHECK OPTION. It also grants space_credmgr the base column reads the invoker views need. The service logins' privileges on the views come from space_migrator's default privileges and bootstrap-roles, as for any new relation.

• Test-side fixes. Two api test helpers create row triggers on sandbox_k8s_executions, which Postgres refuses on a view (42809). They now name the table through pplx_space_db_test_fixtures::physical_table(...), which returns the renamed table in variant mode and the old name otherwise.

• Pins. crates/l30_storage/db/migrate/src/bootstrap_roles/tests/execution_record_view_tests.rs runs statement shapes from today's code and tests as the real bootstrap-roles logins. The server is migrated with control-plane up and the candidate is applied as space_migrator. Shapes covered:

  • targetless ON CONFLICT DO NOTHING … RETURNING, and the retired-generation INSERT … SELECT … WHERE NOT EXISTS
  • the claims' generation allocation MAX(generation) + 1 through the view
  • the node inventory's aliased ON CONFLICT (node_id) DO UPDATE … WHERE node_k8s.k8s_node_name IS DISTINCT FROM EXCLUDED…
  • the check option refusing a non-k8s row
  • the base default filling an omitted managed_by
  • FOR UPDATE OF … NOWAIT and SKIP LOCKED on view joins
  • LOCK TABLE through a view, as the api's create-readiness contention test takes it (it locks view and base)
  • UPDATE … FROM
  • statements prepared before the rename (re-resolved to the views, kept prepared with the same prepare_time, no 0A000)
  • the views' privileges from default privileges alone, for space_user, space_creds and space_ro
  • invoker-view privilege checks for each view separately, including the space_credmgr column grants

  Under nextest they share one bare Postgres server that the bare-postgres setup script (ci/scripts/setup-nextest-postgres.sh --bare; its default mode is unchanged) starts for the run. The first pin bootstraps and migrates a template database on it under an advisory lock, and each pin clones a database of its own from it. They still share cluster-global state: the roles bootstrap provisions and a probe role the template build creates. A pin must not change it; the invoker pin's second bootstrap is safe only because it converges the shared roles to the values the template build gave them. Under plain cargo test each pin starts its own container. They need Docker, so they stay in the nextest privileged group. The LOCK TABLE pin's blocked writes use a 20 ms lock_timeout (was 200 ms): lock_timeout bounds only lock waits, so an unblocked write succeeds whatever the bound.

• Docs. The fixtures crate README ("Schema variants") and a pointer in docs/rust/space-conventions.md explain how to run the variant. There is no CI job for it: it would repeat the whole Rust test job.

Constraints for the real migrations. The Postgres behaviour in 1 and 2 is pinned on PG16 and PG17; what it means for the production callers comes from reading their code, and 3 comes from reading the candidate:

1. Invoker views check every base column the view reads. Through a security_invoker view, Postgres checks the caller's base-table SELECT on every column the view's query reads, not only the ones the caller selects. The service logins are unaffected: bootstrap-roles grants space_user and space_creds SELECT, INSERT, UPDATE, DELETE on both base tables, and the credential manager connects as space_creds. A session that holds only the space_credmgr capability role is affected, such as the credential-injection store's resolution test (SET LOCAL ROLE space_credmgr). With its old column grants plus managed_by, its reads through either view fail with 42501. The candidate therefore grants space_credmgr base SELECT on every column the views read, so that role can now read pod_uid and the other base columns directly. The view's column grants still refuse pod_uid through the view. A view that checks base privileges as its owner, the default without security_invoker, would keep that role's old base fence. Every column later added to either table must also be added to the view and to that grant.

2. Base unique indexes still arbitrate rows the views hide. A targetless ON CONFLICT DO NOTHING arbitrates every unique index on the base table, and the views hide other substrates' rows:

   • with a non-k8s active execution, the execution claim through the view returns no row, even though the view shows no active execution;
   • claims allocate their generation as MAX(generation) + 1 through the view, while (sandbox_id, generation) is unique across substrates, so a hidden row of any state at that generation also makes the claim return no row, and every retry allocates the same generation;
   • the node inventory upsert ON CONFLICT (node_id) DO UPDATE on a hidden node_identities row fails with 44000 (new row violates check option for view "node_k8s").

   In both claim cases, the claims' follow-up read through the view finds no active execution: the api claim fails with failed to create active Kubernetes execution row, and the placement claim's read finds no row. None of this happens today, because no non-k8s rows exist yet.

3. Nullable pod_name/pod_namespace. Making them nullable means an insert through the view no longer rejects a NULL pod_name. The views' WHERE managed_by = 'k8s' does not restore that. Today's code never writes one.

Validation

At bac756cd (rebased on main past #1407, #1408 and #1426), on a Linux host with Docker, Rust 1.99.1 and nextest 0.9.133, on PG16:

• cargo nextest run -p pplx-space-db-migrate -p pplx-space-db-test-fixtures -p pplx-space-api --locked --all-features --run-ignored all --ignore-default-filter --retries 1 --no-fail-fast with SPACE_CI_REQUIRE_TEST_DEPS=1, once unset and once with SPACE_TEST_POSTGRES_SCHEMA_VARIANT=execution-record-views: 2602/2602 passed in each mode, no skips, no dependency-skip marker, identical per-test results. python3 ci/scripts/check-test-durations.py target/nextest/default/junit.xml ci/test-duration-budget.toml passes after both. A bogus variant value fails template setup.
• The pplx-space-db-migrate privileged group, run as root from the build as CI does, 12 threads, three times, then the duration check: the five pins took 0.49-0.66 s each, and the check passed. With a container per pin they took 2.7-4.6 s, and the check failed on all three runs. PG17 (postgres:17-alpine): 16/16. Plain cargo test, a container per pin: 5/5.
• Each pin's key assertion fails under a mutation of what it pins: execution view column order; node_k8s check option; the LOCK TABLE released before the blocked writes; the execution view's managed_by filter; node_k8s without security_invoker.
• cargo fmt --all -- --check, cargo check --workspace --all-targets --all-features --locked, and cargo clippy -p pplx-space-db-migrate --all-targets --all-features -- -D warnings: clean. bash ci/scripts/run-archcheck.sh "$(git rev-parse origin/main)" "": no findings.

At this head (ef1f9305, which adds review fixes on top: comments on the shared roles, and the test helper's bare-server handling in pg_test_support.rs, which now errors on a non-UTF-8 server URL and fails, rather than skips, when a published server is unreachable): the migrate privileged group passed 16/16 with the duration check (pins 0.51-0.61 s); plain cargo test 5/5; fmt and clippy clean. CI on GitHub at bac756cd ran the bare setup in the privileged container and the pins in 0.53-0.72 s.

From the rebase across the crate move: when main gained #1304's runtime_upgrade_* execution columns, two pins failed as they should (views_keep_the_old_row_types_and_writes_reach_the_base_tables on the row type, statements_prepared_before_the_rename_run_through_the_views with 0A000 cached plan must not change result type) until the candidate's view and grant carried them.

From an earlier head, before the rebase across the crate move, and not rerun here: the PG17 runs, the full rdeps(pplx-space-db-test-fixtures) comparison over 46 packages on PG16 and PG17 (no per-test difference between modes or versions), clippy, and the checks that each pin's key assertion fails without the property it pins. Before the trigger fix, a PG16 variant run of the api suite had five variant-only failures, all 42809 "sandbox_k8s_executions" is a view, in controllers::sandbox::lifecycle::characterization_tests::kubernetes and controllers::substrate::kubernetes::tests::kubernetes_tests.

Severity and risk

Severity: does not apply (test infrastructure). Risk: with the variable unset the fixtures behave as before, as the default-mode run above shows. The pins add five tests to the privileged CI half, about 0.5-0.7 s each, on one Postgres server the bare-postgres setup script starts for the run; the script's default mode is unchanged. Roll back by reverting the commit.

Rollout

None: no migration and no production code. Mixed-version: does not apply. The candidate DDL runs only in test databases: the shared fixtures apply it when the variable is set, and the five pins always apply it, each in its own database on their shared server (its own container under cargo test).

Not covered

• The fixture suites run as the superuser, so a variant run checks privileges only where a test switches role (the credential-injection resolution test, as space_credmgr). The pins cover the bootstrap-roles logins.
• No CI job runs the variant.
• Once the real rename lands, the default physical_table result names a view and the pins' candidate no longer applies. A crates change must make them work on either schema before that migration, because the migration-isolation guard rejects changes to physical_table, the api test helpers and the pins in a migration PR.
• The constraints above need decisions before the real migrations; they are tracked separately.

AI assistance: An AI agent (Claude Code) wrote the code, tests, candidate DDL, documentation, and this description, rebased the change across the crate move and onto main past #1407, #1408 and #1426, and restructured the pins to share one server under the test-time gate.

[ac-pplx]

@coderabbitai full review --use-credits

[coderabbitai]

Automatic approval withheld

Automatic approval is disabled by the configured approval policy for this pull request. Review comments and requests for changes continue normally.

Commit: 51c6cfd23ce5.

No actionable comments were generated in the recent review.

Walkthrough

The change adds a selectable PostgreSQL schema variant with renamed execution and node tables and compatibility views. Fixture setup can apply the variant to a migrated template, and test helpers can resolve physical table names. New migrator integration tests exercise view behavior and privileges. Nextest and PostgreSQL test support add a bare-server setup for those tests.

Possibly related PRs

• perplexityai/SPACE#1309: Implements the same execution-record compatibility-view schema variant and privileged PostgreSQL tests.
• perplexityai/SPACE#1310: Changes bootstrap grant convergence for excluded relations and compatibility-view privileges.

Merge Risk: Minimal · up to 51c6c

This adds an opt-in PostgreSQL test schema variant and tests for the compatibility views without changing production behavior or defaults. Default test runs are unaffected. The variant is not run in CI, so it depends on the author's reported runs. The commit messages mark the branch as preview-only and not to be merged, so confirm that is intended before merging.

Security architecture risk: Low · up to 51c6c

The changes are confined to database-test infrastructure and do not change deployed migrations. The candidate deliberately broadens a test role’s base-table read access, and its privileged setup must remain on disposable test servers.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Privileged setup has authority over the selected PostgreSQL cluster, not only an individual clone. The bare-server override selects a host and port, while connections use postgres and setup can drop its named template and rewrite cluster-global roles. Normal nextest provisioning contains that authority in a separate disposable server; manually supplied targets remain a test-runner trust boundary.

Trust Boundaries and Controls

• observed — The new migrator entrypoints are compile-time test-gated. Candidate application in the privilege pins assumes space_migrator so view ownership and default grants reflect migration-role behavior. The pins independently remove base-column grants and assert permission failures, with transactional rollback restoring each tested grant state.

Resilience and Maintainability Implications

• observed — General fixture clones carry a DatabaseCleanup owner. The new pin clones instead rely on server-lifetime cleanup: their creation path has no per-clone cleanup owner, and script-managed runs remove the entire container. A caller-supplied bare server has no Rust-owned container handle, so its retained databases and cluster-global roles remain the caller’s responsibility.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title starts with the lowercase bracketed area "[cr-test]" and uses a lowercase summary that accurately describes running Postgres suites against execution-record compatibility views. It has no tr…
Description Follows The Pr Template		The description contains all required template content: a substantive "## Summary" section; a "## Validation" section naming commands and results; a "## Severity and risk" section stating severity and…
Bug Fixes Name A Regression Test		PASS: This pull request is test infrastructure, not an application bug fix. The diff adds compatibility-view fixtures and migration tests, and changes test-only helpers in `crates/.../characterization…
Weakened Safeguards Are Justified		The pull request does not introduce an unjustified weakened safeguard. The diff adds five assertion-heavy tests and assigns them to the privileged nextest group with a dedicated bare-Postgres setup. I…
Public-Safe Content		No public-safety violation found. The diff contains no credentials, tokens, private keys, internal URLs or hostnames, cloud ARNs, workstation paths, private links, production logs, or incident details…

Full details: Title check

Explanation

The title starts with the lowercase bracketed area "[cr-test]" and uses a lowercase summary that accurately describes running Postgres suites against execution-record compatibility views. It has no trailing period, ticket ID, or internal link.

Full details: Description Follows The Pr Template

Explanation

The description contains all required template content: a substantive "## Summary" section; a "## Validation" section naming commands and results; a "## Severity and risk" section stating severity and risk; and an "AI assistance:" line describing the tool and drafted or changed parts.

Full details: Bug Fixes Name A Regression Test

Explanation

PASS: This pull request is test infrastructure, not an application bug fix. The diff adds compatibility-view fixtures and migration tests, and changes test-only helpers in crates/.../characterization_tests and crates/.../kubernetes/tests.rs; it does not change application behavior. The regression-test requirement is therefore not applicable.

Full details: Weakened Safeguards Are Justified

Explanation

The pull request does not introduce an unjustified weakened safeguard. The diff adds five assertion-heavy tests and assigns them to the privileged nextest group with a dedicated bare-Postgres setup. It does not remove tests, assertions, lints, CI checks, allowlist entries, or repository guards. The two trigger helpers now target physical tables because the compatibility names are views; adjacent comments and the PR description state this reason. The new tests may return when no bare server exists, and the PR description explicitly documents that behavior for unavailable test dependencies.

Full details: Public-Safe Content

Explanation

No public-safety violation found. The diff contains no credentials, tokens, private keys, internal URLs or hostnames, cloud ARNs, workstation paths, private links, production logs, or incident details. The only URL-like values are parameterized local PostgreSQL test connection strings, and 127.0.0.1 is a test value. The title and description contain no disallowed sensitive content.

Recent review info

Automatic approval policy

Commit: 51c6cfd23ce5.

Setting	Effective value	Source
Automatic approvals	Disabled	Override 4
Merge risk	Low	Override 2
Security risk	Not set	Global configuration

Matching overrides (numbered as in the resolved configuration):

• Override 2 — any author; paths: **, !ui/**; merge risk: Low.
• Override 4 — any author; paths: .coderabbit.yaml, .coderabbit.yml, coderabbit.yaml, coderabbit.yml, .coderabbit.config.ts, **/AGENTS.md, **/CLAUDE.md, .agents/**, .claude/**, .claude-plugin/**, CONTRIBUTING.md, docs/versioning.md, docs/rust/space-conventions.md, docs/rust/space-review-lessons.md, .github/**, ci/**, !ci/scripts/setup-nextest-*.sh, scripts/source_guards/**, scripts/check*, clippy.toml, deny.toml, .gitleaks.toml, .pre-commit-config.yaml, lefthook.yaml, rust-toolchain.toml, .cargo/**, supply-chain/**, pyproject.toml, uv.lock, **/package.json, **/package-lock.json, **/go.mod, **/go.sum, **/requirements*.txt, **/requirements*.in, proto/**, db/**, crates/l40_api/api/src/auth.rs, crates/l40_api/api/src/auth/**, crates/l40_api/api/src/middleware/auth*, crates/l40_api/api/src/middleware/auth/**, crates/l40_api/api/src/routes/authz.rs, crates/l10_foundation/bearer/**, crates/l10_foundation/rustls/**, crates/l10_foundation/guest/**, crates/l10_foundation/pii/**, shim/**, docker/**, templates/**, infra/space/aws/**, infra/space/cluster/**, infra/space/identity/**, infra/space/observability/**, infra/space/packer/**, infra/space/kind/charts/**, LICENSE, SECURITY.md; automatic approvals: disabled.

Any matching disabled override withholds approval, regardless of risk. Otherwise, the strictest explicitly set limit wins for each score. Limits omitted by all matching overrides inherit the global configuration.

Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.

Run configuration

• Source mode: Auto
• Repository: perplexityai/SPACE
• Branch: cr-test/1309-v4-head
• File: .coderabbit.yaml
• Commit: 51c6cfd
• Layers: Repository YAML; Enforced overrides: Organization
• Review profile: CHILL
• Plan: Enterprise
• Run ID: ad159fb5-aea8-4863-bce9-91dfb9eab236

Commits

Reviewing files that changed from the base of the PR and between 093e7c0 and 51c6cfd.

Files ignored due to path filters: 1

• Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock

Files selected for processing: 14

• .config/nextest.toml
• ci/scripts/setup-nextest-postgres.sh
• crates/l30_storage/db/migrate/Cargo.toml
• crates/l30_storage/db/migrate/src/bootstrap_roles/tests.rs
• crates/l30_storage/db/migrate/src/bootstrap_roles/tests/execution_record_view_tests.rs
• crates/l30_storage/db/migrate/src/bootstrap_roles/tests/postgres_bootstrap_tests.rs
• crates/l30_storage/db/migrate/src/pg_test_support.rs
• crates/l40_api/api/src/controllers/sandbox/lifecycle/characterization_tests/kubernetes.rs
• crates/l40_api/api/src/controllers/substrate/kubernetes/tests.rs
• crates/test_support/db/test/fixtures/README.md
• crates/test_support/db/test/fixtures/schema-variants/execution_record_views.sql
• crates/test_support/db/test/fixtures/src/lib.rs
• crates/test_support/db/test/fixtures/src/tests.rs
• docs/rust/space-conventions.md

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 1 remain after this review.

[github-actions]

Thanks for opening this pull request! A SPACE maintainer will take a look.

CONTRIBUTING.md explains how we review outside contributions.

[coderabbitai]

Action performed

Full review finished.

[ac-pplx]

Closing: CodeRabbit configuration test finished; the tested configuration is in #1496.