[ac-pplx]

Summary

pplx-space-sandbox-portability still rendered SET lists for sandboxes and for its executions and port exposures. The data-store rules forbid that: a statement stays whole at its query site, and only a whole portability boolean predicate may be interpolated. For example, repair::revert_failed_to_suspended built SET {sandbox_assignments}, restore_lease_expires_at = NULL, ..., so the text Postgres received could not be read at the query site.

Merging this:

• Writes each such statement out in full at its query site:
  • sandbox-store: drain, ingest, orphan, pod_tracking, repair, suspend;
  • cluster controller: pod-less drain give-up, started repair and failed-restorable repair stores.
• Builds a statement once in a LazyLock<String> where its only interpolations are portability predicates, or the constant repair row-state guards.
• Splits pod_tracking's started recovery into two whole statements, one per proof shape.
• Leaves the orphan and repair guard fragments interpolated; the reconciler's scans compose the same fragments.
• Deletes the renderers and the types only they used: clear_/write_/publish_clear_or_preserve_current_local_artifact_marker_assignments, suspended_/quarantined_k8s_finalization_sql, SuspendedK8sFinalizationSql, SuspendedTargetNodeAssignment and the execution and port-exposure assignment helpers.
• Changes drain::revert_pod_less_resident_to_suspended to return bool instead of the renderer.
• Updates the doc comments that named deleted functions, including node_local_backup_pending.rs, and the sandbox_writes comment in ARCHITECTURE.toml. The ratchet's patterns are unchanged.

Changed test expectations: none.

Validation

• Byte-identity: I ran the touched crates and their callers against a Postgres with log_statement=all, once on origin/main and once on this head, and compared the statements by sql! module path with the locator line number removed. A temporary test, not committed, ran the two OrphanGuard::LegacyDeleting statements, which no existing test reaches. All 15 variants match exactly:

  • drain pod-less revert;
  • ingest::finalize_suspended;
  • orphan recover and quarantine, each for PodName, Forgotten and LegacyDeleting;
  • pod_tracking ExecutionRetired and FailedRecovery;
  • repair started and failed;
  • suspend committed-artifact and missing-pod;
  • port-exposure reset in both repair stores;
  • give-up execution retire and port-exposure reset.

  Of the 3,242 statements logged, the only other difference was in run-generated EXPLAIN text, from UUID literals and a locator line number.

• Mutation: changing ingest's target_node_id = COALESCE(s.target_node_id, s.active_node_id) to s.target_node_id fails published_state_tests::lifecycle_writers::{ready_backup_projects_suspended_only_out_of_running_and_restoring_lifecycles, suspended_report_projects_only_out_of_running_and_restoring_lifecycles}.

• cargo fmt --all -- --check, scripts/check-rust-error-policy.sh and cargo clippy --workspace --locked --all-targets --all-features -- -D warnings pass. cargo arch reports 0 required findings, also against the base baseline and architecture.

• SPACE_CI_REQUIRE_TEST_DEPS=1 cargo nextest run --all-features on sandbox-store, sandbox-portability, cluster-controller-server, internal-api-server, api, snapshot-store and snapshot-retention: 4025 passed against a real Postgres, 0 dependency skips. The one skipped test is the ignored opt-in benchmark.

• Kind: a fresh cluster from this head (infra/space/kind/scripts/up.sh, otel telemetry) came up in 471 s. scripts/tests/smoke-kind-typed.sh all, with CI's admin-token and cold-receipt setup, passed 91/91 on the ECW surface and 42/42 on the native surface in 3902 s, with no reruns. Six console-UI scenarios were skipped because Playwright/Chromium isn't installed.

Severity and risk

Severity: does not apply; this is a structural cleanup with no behaviour change.

Risk: low. The SQL text is unchanged, so the only risk is in the Rust control flow around it, mainly bool replacing Option<SuspendedK8sFinalizationSql> in the pod-less give-up. Roll back by reverting the commit.

Mixed versions: there is no schema or wire change. Old and new binaries send the same statements, apart from sql! locator comments.

AI assistance: Claude Code drafted the code changes, the byte-identity harness and this description.

[ac-pplx]

@coderabbitai full review --use-credits

[coderabbitai]

Note

Some coding-guideline sources were skipped or only partly applied; the reason is listed next to each one. On a public repository, this repository's guideline files are counted without their names.

Skipped guideline sources: 2

2 guideline files in this repository — not fully prepared for this review; preparing guideline files took longer than 2 minutes

No actionable comments were generated in the recent review.

Walkthrough

The change removes the shared Kubernetes finalization SQL API and moves its update logic into sandbox store and controller queries. These queries update sandbox lifecycle and placement, active executions, open port exposures, and artifact markers. The pod-less revert now returns a boolean. Related documentation and comments are updated.

Possibly related PRs

• perplexityai/SPACE#1406: It makes the same renderer-removal change across overlapping store and controller code, including the boolean return type for pod-less revert.

Merge Risk: Minimal · up to 40a7e

This is a structural cleanup that writes the sandbox finalization SQL directly at each query site and removes the shared renderers. Review found no behavior change in lifecycle, placement, marker, execution or exposure updates. The risk of merging is minimal. The PR is marked as a preview-only draft, so it should not be merged as is.

Security architecture risk: Low · up to 40a7e

The inspected recovery and finalization paths preserve their existing identity checks, eligibility rules, and transaction boundaries. No introduced security issue was identified. Residual uncertainty remains because the assessment is a static comparison rather than end-to-end production verification.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The inspected mutations affect selected sandbox rows and their related execution, exposure, observation, and claimed-slot state. Exposure resets preserve user-desired open state while clearing application observations; they do not themselves create a new exposure request.

Trust Boundaries and Controls

• observed — Publication finalization retains its existing trusted-caller contract: selection checks the reporting node's assignment, suspended status, lifecycle and artifact evidence, and locks sandbox rows. The finalizer retains eligible-candidate gating and the published execution-identity predicate. It does not independently recheck every selection condition; that dependency predates this PR.
• observed — Suspend completion retains sandbox locking, exact generation and pod-UID checks, artifact identity checks, and explicit acceptance of an already-finalized projection only when no successor execution exists. The inlined store statement continues to rely on these caller-side controls.

Resilience and Maintainability Implications

• inferred — The inspected recovery paths preserve atomic database transitions: guarded non-matches do not commit dependent cleanup, and successful lifecycle changes and execution retirement remain in the same transaction. Drain retains node, sandbox, and execution locking plus generation revalidation. Orphan recovery and quarantine retain transactional reconcile-state cleanup, followed by best-effort pod cleanup after commit; that external cleanup boundary is unchanged.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title uses a lowercase area in square brackets, starts its summary with a lowercase word, describes the SQL rewrite and renderer removal, and has no trailing period, ticket ID, or internal link.
Description Follows The Pr Template		The description contains all required sections with real content: ## Summary states what merging does; ## Validation lists commands and results; ## Severity and risk gives severity and risk; and…
Bug Fixes Name A Regression Test		PASS. The pull request is a structural SQL cleanup, not an application behavior bug fix. The reviewed diff rewrites existing statements, removes renderers, and changes a return value; it changes no te…
Weakened Safeguards Are Justified		No weakened safeguard was introduced. The diff changes source files and comments only; it does not delete tests, loosen assertions, add allow/expect attributes, alter lint levels, or change CI/nextest…
Public-Safe Content		The diff and title contain no credentials, token values, private keys, internal URLs or hostnames, cloud IDs or ARNs, workstation paths, or real cluster names. The description contains generic CI, Pos…

Full details: Description Follows The Pr Template

Explanation

The description contains all required sections with real content: ## Summary states what merging does; ## Validation lists commands and results; ## Severity and risk gives severity and risk; and AI assistance: identifies Claude Code and the drafted parts.

Full details: Bug Fixes Name A Regression Test

Explanation

PASS. The pull request is a structural SQL cleanup, not an application behavior bug fix. The reviewed diff rewrites existing statements, removes renderers, and changes a return value; it changes no tests. The Validation section also names a lifecycle-test mutation check that fails when the ingest assignment is changed.

Full details: Weakened Safeguards Are Justified

Explanation

No weakened safeguard was introduced. The diff changes source files and comments only; it does not delete tests, loosen assertions, add allow/expect attributes, alter lint levels, or change CI/nextest/known-failure configuration. ARCHITECTURE.toml has identical parsed TOML values; only comments changed. The removed AssertSqlSafe wrappers are replaced by whole sql! literal statements, while runtime-built SQL retains AssertSqlSafe.

Full details: Public-Safe Content

Explanation

The diff and title contain no credentials, token values, private keys, internal URLs or hostnames, cloud IDs or ARNs, workstation paths, or real cluster names. The description contains generic CI, Postgres, and Kind test results only. It includes no raw production logs, query output, production measurements, or undisclosed vulnerability details.

Recent review info

Automatic approval policy

Commit: 40a7ee895611.

Setting	Effective value	Source
Automatic approvals	Enabled	Global configuration
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.

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/1406-r1-eval-head
• File: .coderabbit.yaml
• Commit: 40a7ee8
• Layers: Repository YAML; Enforced overrides: Organization
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 8b133cb7-fa3e-45b7-8061-7e89c2664797

Commits

Reviewing files that changed from the base of the PR and between b7c750e and 40a7ee8.

Files selected for processing: 12

• ARCHITECTURE.toml
• crates/libs/cluster/controller/server/src/failed_restorable_repair/store.rs
• crates/libs/cluster/controller/server/src/node_drain/store/pod_less_giveup.rs
• crates/libs/cluster/controller/server/src/started_sandbox_repair/store.rs
• crates/libs/internal/api/server/src/node_local_backup_pending.rs
• crates/libs/sandbox/portability/src/lib.rs
• crates/libs/sandbox/store/src/drain.rs
• crates/libs/sandbox/store/src/ingest.rs
• crates/libs/sandbox/store/src/orphan.rs
• crates/libs/sandbox/store/src/pod_tracking.rs
• crates/libs/sandbox/store/src/repair.rs
• crates/libs/sandbox/store/src/suspend.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 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 review evaluation finished.