[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]

No actionable comments were generated in the recent review.

Walkthrough

Sandbox finalization and recovery queries now define lifecycle, execution, port-exposure, and artifact-marker updates directly instead of using shared portability SQL helpers. The shared helper types and functions were removed. The pod-less drain revert now returns a boolean, and repair paths use cached SQL queries. Existing eligibility and execution-proof checks remain.

Possibly related issues

• perplexityai/SPACE issue 876 — The pull request changes the pod-less resident revert path related to retained-suspend recovery, but does not add typed quarantine evidence or operator release.

Possibly related PRs

• perplexityai/SPACE#1406 — This PR reports the same helper removal, signature change, and rewrites of the sandbox-store and controller call sites.

Merge Risk: Minimal · up to 05e41

This is a structural cleanup that is reported to produce the same SQL as before, with no schema or wire change. No concrete behavior regression was found, and the author's byte-identical SQL comparison and passing test and cluster runs support that. It is ready to merge once it is taken out of draft.

Security architecture risk: Low · up to 05e41

The inspected recovery safeguards and transaction boundaries are preserved, and no new security exposure was identified. Risk remains low rather than minimal because some pre-existing concurrency assumptions around snapshot updates and external pod-state observations were not independently verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The affected authority is existing control-plane database access to sandbox lifecycle, artifact markers, execution records, and port-application observations. Reviewed writes remain scoped to a sandbox ID or selected candidate IDs; the refactor does not demonstrate increased caller authority or cross-tenant reachability.

Trust Boundaries and Controls

• observed — Pod-less recovery retains its locked write gate: node, sandbox, and execution locks precede a repeated eligibility and generation check. The sandbox UPDATE separately re-evaluates recoverability, snapshot freshness, and ambiguity predicates. A refused update rolls back before execution and exposure cleanup.
• inferred — The two explicit started-recovery statements preserve separate proof shapes: absence of an active or routable execution, or a matching failed-recovery generation without placement identity or a newer active generation. Their caller-held sandbox lock and unchanged lifecycle guards prevent a successful repeated recovery from applying the same transition again.

Resilience and Maintainability Implications

• inferred — Failure containment remains based on single-statement finalization or caller-owned transactions. Recovery updates and subsequent cleanup do not gain a new intermediate commit point. This preserves the inspected rollback boundary if execution retirement or exposure reset fails.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title uses the required "area: summary" form with a lowercase area, but it begins with a "[cr-test]" tag and the summary begins with "write", which is lowercase. The summary describes the main cha…
Description Follows The Pr Template		The description contains all required sections and content: ## Summary explains what merging does; ## Validation lists checks and results; ## Severity and risk states severity does not apply and…
Bug Fixes Name A Regression Test		PASS: This pull request is a structural SQL-renderer cleanup, not a fix for an application-behavior defect. The diff replaces generated assignment fragments with inline statements and preserves the re…
Weakened Safeguards Are Justified		The authoritative diff changes 12 files: production Rust source and ARCHITECTURE.toml comments. It adds no test or CI configuration changes and removes no tests, assertions, lints, CI checks, allowl…
Public-Safe Content		No public-safety violation is present. The title, description, and reviewed diff contain no credentials, private keys, internal URLs or hostnames, cloud IDs or ARNs, workstation paths, private links, …

Full details: Title check

Explanation

The title uses the required "area: summary" form with a lowercase area, but it begins with a "[cr-test]" tag and the summary begins with "write", which is lowercase. The summary describes the main change: writing the portability assignment renderers' statements whole at their query sites and deleting the renderers. The changeset matches this, since the renderers are removed and the statements are inlined across sandbox-store and cluster-controller. The title is specific and accurately describes the main change. The bracketed "[cr-test]" prefix is an extra tag that the title requirements do not forbid, since the allowed form includes an area in square brackets, but it is not an area name.

Full details: Description Follows The Pr Template

Explanation

The description contains all required sections and content: ## Summary explains what merging does; ## Validation lists checks and results; ## Severity and risk states severity does not apply and gives low risk; and AI assistance: identifies Claude Code and the drafted content.

Full details: Bug Fixes Name A Regression Test

Explanation

PASS: This pull request is a structural SQL-renderer cleanup, not a fix for an application-behavior defect. The diff replaces generated assignment fragments with inline statements and preserves the reported SQL behavior. The bug-fix condition does not apply.

Full details: Weakened Safeguards Are Justified

Explanation

The authoritative diff changes 12 files: production Rust source and ARCHITECTURE.toml comments. It adds no test or CI configuration changes and removes no tests, assertions, lints, CI checks, allowlist entries, baselines, or known-failure entries. The architecture edit changes comments only; the sandbox_writes patterns and allow list remain unchanged.

Full details: Public-Safe Content

Explanation

No public-safety violation is present. The title, description, and reviewed diff contain no credentials, private keys, internal URLs or hostnames, cloud IDs or ARNs, workstation paths, private links, incident details, or undisclosed vulnerability details. The validation counts and timings are test results, not raw production logs or production measurements.

Recent review info

Automatic approval policy

Commit: 05e414ce7727.

Setting	Effective value	Source
Automatic approvals	Enabled	Global configuration
Merge risk	Moderate	Global configuration
Security risk	Not set	Global configuration

No overrides matched. The global limits apply.

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-mod1-head
• File: .coderabbit.yaml
• Commit: 05e414c
• Layers: Repository YAML; Enforced overrides: Organization
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 9d4af0a2-5a8f-4070-9020-8571e86c11a6

Commits

Reviewing files that changed from the base of the PR and between 478f2f4 and 05e414c.

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; 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.