[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

Sandbox finalization and recovery paths now build SQL directly instead of using the removed portability SQL builders. The statements update sandbox lifecycle and artifact markers, retire executions, and reset open port exposures. The pod-less revert returns a boolean result. The sandbox-write ratchet counts SQL statement heads and no longer counts portability-renderer calls.

Possibly related PRs

• perplexityai/SPACE#1406: The summary describes the same removal of portability finalization helpers and inline SQL changes.

Merge Risk: Minimal · up to 19b89

This is a structural cleanup that writes sandbox lifecycle SQL directly at its query sites and removes the shared builders. The generated SQL and the pod-less revert rollback behavior match the prior code, so no user-facing behavior change is expected, and no merge-blocking risk remains.

Security architecture risk: Minimal · up to 19b89

The reviewed recovery paths retain their identity checks, safety conditions, and all-or-nothing updates. The reorganization does not introduce a material security exposure in those paths.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The affected authority is privileged control-plane recovery: controller scans can select eligible Kubernetes sandboxes across the database, while individual repair writes bind the selected sandbox ID. Node-driven suspended finalization is constrained by active-node ownership and execution evidence. The sensitive outcomes are lifecycle and placement changes, artifact-marker publication or clearing, execution retirement, and exposure-status resets.

Trust Boundaries and Controls

• observed — Publication finalization retains generation and pod-UID matching for published artifacts and relies on caller-selected, locked candidates. Its generic executor interface still permits use outside a transaction, but that reliance predates this PR. The inspected production publication path supplies one transaction covering finalization, receipt cleanup, and recovery recording, with events emitted after commit.

Resilience and Maintainability Implications

• inferred — The inspected recovery transitions preserve failure containment under repetition, concurrency, and interruption. Pod-less drain rechecks guards and generation under node, sandbox, and execution locks, then commits cleanup together. Pod-tracking recovery retains sandbox locking, generation-scoped retirement, and post-commit events; retirement failure prevents a successful recovery commit. No new partial-commit window was found in these paths.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title uses the bracketed lowercase area "[cr-test]" and describes the main change: inlining portability-renderer statements and deleting the renderers. It has no trailing period, ticket ID, or int…
Description Follows The Pr Template		The description contains all required sections with real content: “## Summary” explains what merging does; “## Validation” names checks and results; “## Severity and risk” gives severity and risk; and…
Bug Fixes Name A Regression Test		PASS: This is a structural SQL-rendering cleanup, not an application behavior defect fix. The diff inlines existing SQL assignments, removes portability renderers, and changes the pod-less helper to r…
Weakened Safeguards Are Justified		No unjustified safeguard weakening found. ARCHITECTURE.toml removes only the two sandbox_writes patterns for portability renderer calls, and the description specifically explains that those render…
Public-Safe Content		No sensitive public-safety content is present in the reviewed diff, title, or description. The diff contains no credentials, token values, private keys, URLs, hostnames, ARNs, cloud IDs, workstation p…

Full details: Title check

Explanation

The title uses the bracketed lowercase area "[cr-test]" and describes the main change: inlining portability-renderer statements and deleting the renderers. It has no trailing period, ticket ID, or internal link.

Full details: Description Follows The Pr Template

Explanation

The description contains all required sections with real content: “## Summary” explains what merging does; “## Validation” names checks 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: This is a structural SQL-rendering cleanup, not an application behavior defect fix. The diff inlines existing SQL assignments, removes portability renderers, and changes the pod-less helper to return whether its update changed a row. The contributor explicitly reports no behavior change. The check therefore does not require a regression test.

Full details: Weakened Safeguards Are Justified

Explanation

No unjustified safeguard weakening found. ARCHITECTURE.toml removes only the two sandbox_writes patterns for portability renderer calls, and the description specifically explains that those renderers are deleted and their SQL is written whole at each query site. architecture-baseline.toml changes comments only; its caps remain unchanged. No tests, assertions, lints, CI checks, or allowlists are removed or weakened.

Full details: Public-Safe Content

Explanation

No sensitive public-safety content is present in the reviewed diff, title, or description. The diff contains no credentials, token values, private keys, URLs, hostnames, ARNs, cloud IDs, workstation paths, or cluster names. The description reports test validation on a fresh Kind cluster and a real Postgres instance, but it does not include raw logs, query output, or production measurements.

Recent review info

Automatic approval policy

Commit: 19b89cd64652.

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-r2-eval-head
• File: .coderabbit.yaml
• Commit: 19b89cd
• Layers: Repository YAML; Enforced overrides: Organization
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 37509fe6-9e94-42a1-93f5-0b5d672e6dee

Commits

Reviewing files that changed from the base of the PR and between 1e62f17 and 19b89cd.

Files selected for processing: 13

• ARCHITECTURE.toml
• architecture-baseline.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; 4 remain after this review.

[coderabbitai]

Action performed

Full review finished.

[github-actions]

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

CONTRIBUTING.md explains how we review outside contributions.

[ac-pplx]

Closing: CodeRabbit review evaluation finished.