[zibi-pplx]

Summary

Merging this takes the cleanup states out of sbx-manager's persisted lifecycle state machine. The edge table listed cleanup_required and cleanup_failed as lifecycle states, and can_transition_to admitted moves between them.

• Lifecycle edges go from lifecycle sources, or from rows a cleanup owns, to a lifecycle target; Edge::new refuses a cleanup target.
• The seven writers that hand a row to cleanup or record its failure move to a separate cleanup-edge table targeting CleanupState::Required and Failed.
• can_transition_to is lifecycle-only: a cleanup-owned row admits only Delete and quarantine, as before.

Every edge reads as the same combined statuses as before, so update_state_from and the admission answers production asks are unchanged.

Validation

• Tests: persisted_edges_disagree_with_the_admission_machine_only_as_allowlisted, no_edge_lists_its_own_target_as_a_source and a_cleanup_owned_row_admits_only_delete_and_quarantine.
• each_edge_reads_exactly_its_listed_sources pins every edge's sources and target, so a dropped edge, or one widened into a pair another edge already writes, fails.
• The edge-table test compares cleanup edges as the combined status their rows read as; two allowlist rows become WriteForbidden. A model of that test was checked against the old and new tables.
• Locally: rustfmt --check on every changed Rust file. Rust was not compiled locally, so CI on this head is the build and test evidence.

Severity and risk

Severity: does not apply.

Risk: a typing change to the edge table; behaviour is meant to be unchanged. Roll back by reverting.

Stack

Part of a stack that moves CleanupRequired and CleanupFailed out of the sandbox lifecycle into their own cleanup state. Merge in order; this PR depends on #1560.

1. [shim] keep a Stop sbx-manager refused before admission as refused #1552 shim: keep a Stop refused before admission as refused (independent fix, merged)
2. [control-plane] drop cleanup_required from the stored lifecycle vocabulary #1553 control-plane: drop cleanup_required from the stored lifecycle vocabulary
3. [proto] publish whether a node-local cleanup owns a sandbox row #1554 proto: publish whether a node-local cleanup owns a sandbox row
4. [sbx-mgr] store cleanup ownership apart from the sandbox lifecycle #1555 sbx-mgr: store cleanup ownership apart from the sandbox lifecycle
5. [sbx-mgr] publish cleanup ownership on State, Wait and List #1556 sbx-mgr: publish cleanup ownership on State, Wait and List
6. [shim] decide a cleanup-owned row by its cleanup state, not its lifecycle #1557 shim: decide a cleanup-owned row by its cleanup state
7. [sbx-mgr-protocol] decode an owning cleanup to its combined status #1558 sbx-mgr-protocol: decode an owning cleanup to its combined status
8. [sbx-mgr] count sandbox rows by lifecycle and cleanup state #1559 sbx-mgr: count sandbox rows by lifecycle and cleanup state
9. [events] carry a transition's cleanup state apart from its lifecycle #1560 events: carry a transition's cleanup state apart from its lifecycle
10. [sbx-mgr] keep cleanup out of the sandbox lifecycle state machine #1561 sbx-mgr: keep cleanup out of the sandbox lifecycle state machine ← this PR
11. [sbx-mgr] publish the lifecycle under a cleanup behind a switch #1562 sbx-mgr: publish the lifecycle under a cleanup behind a switch

Each PR keeps today's published behaviour; only the last adds a switch, off by default, that publishes the lifecycle under a cleanup.

AI assistance: an AI coding agent drafted the code, tests and this description; the author reviewed them, and a second AI agent reviewed the change. Perplexity Computer rebased the stack, repaired integration conflicts, restored a test assertion in #1553, and updated this disclosure.

Created with Perplexity

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The change adds CleanupState to represent required and failed cleanup ownership separately from lifecycle states. Lifecycle transitions from cleanup-owned states are restricted to Deleting or Quarantined. Database edge definitions distinguish lifecycle edges from cleanup edges. The state writer converts edge sources to combined legacy statuses for its existing predicates. Tests cover both edge inventories and cleanup-status writes.

Possibly related PRs

• perplexityai/SPACE#1555: Stores cleanup ownership separately and exposes combined legacy statuses used by this change.

Merge Risk: Moderate · up to d03dc

This refactors the state-machine edge tables behind sandbox lifecycle and cleanup writes, with the intent of keeping behavior identical. The edge tests are thorough, but the change touches storage write guards and was not compiled locally. Confirm CI is green before merging.

Security architecture risk: Low · up to 67f23

The inspected database paths preserve cleanup ownership and quarantine controls, and the flagged entrypoints are tests rather than exposed interfaces. Remaining uncertainty concerns compatibility outside inspected callers and build and test execution, not a demonstrated security regression.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated security-relevant surface is sandbox-row lifecycle and cleanup control in the local store. The flagged test declarations do not establish additional attacker-controlled reachability, and the inspected persistence predicates do not show expanded authority over sandbox rows.

Trust Boundaries and Controls

• observed — Inspected cleanup request, failure, retry and completion paths retain transactions and episode, phase or current-intent predicates. Repeated requests can return AlreadyRequested or Superseded, and stale completion checks episode currentness before changing lifecycle state or deleting a row.

Resilience and Maintainability Implications

• inferred — The ownership split does not show a newly stranded cleanup or recovery state in the inspected paths: dedicated failure and retry writes remain available, lifecycle deletion and quarantine remain admitted, and completion retains episode-currentness checks. This conclusion is based on source inspection rather than executed interruption or concurrency tests.

Pre-merge checks: 6

Check name	Status	Explanation
Title check		The title describes the cleanup and lifecycle separation. It meets the area, casing, and formatting requirements.
Description Follows The Pr Template		The description includes a summary, validation results, severity and risk, and an AI assistance line that identifies the coding agent and the parts it drafted.
Bug Fixes Name A Regression Test		This PR is a state-machine refactor, not an application bug fix. The diff preserves the combined cleanup edge statuses. The Validation section also names `a_cleanup_owned_row_admits_only_delete_and_qu…
Weakened Safeguards Are Justified		The changed cleanup-transition tests have a specific reason in the description and code comments. The replacement test checks all cleanup-owned source and target pairs. The allowlist changes retain th…
Agent Tooling Runs No New Commands		The only changed agent guideline contains prose. The diff does not change agent manifests, skills, commands, hooks, or server configuration.
Public-Safe Content		The diff, title, and description contain no content covered by the failure criteria.

Full details: Bug Fixes Name A Regression Test

Explanation

This PR is a state-machine refactor, not an application bug fix. The diff preserves the combined cleanup edge statuses. The Validation section also names a_cleanup_owned_row_admits_only_delete_and_quarantine, which checks the changed transition rule.

Full details: Weakened Safeguards Are Justified

Explanation

The changed cleanup-transition tests have a specific reason in the description and code comments. The replacement test checks all cleanup-owned source and target pairs. The allowlist changes retain their reasons and track the new classification. The AGENTS.md edit does not weaken a rule or change review guidance.

Recent review info

Automatic approval policy

Commit: d03dc1161e87.

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: zibi/cleanup-state-9-state-machine
• File: .coderabbit.yaml
• Commit: d03dc11
• Layers: Repository YAML; Enforced overrides: Organization
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 30c2acdc-1e4c-4b46-8a23-474ebebc9dc8

Commits

Reviewing files that changed from the base of the PR and between 6e39603 and d03dc11.

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

[the-gigi-pplx]

Review — 2 findings

The change is correct as written. Dumping every edge's (legacy source → legacy target, kind) relation and every can_transition_to pair at the base and at this head, then diffing them, gives identical results for all 185 edge rows. The only difference is that can_transition_to no longer admits CleanupFailed → CleanupRequired or CleanupRequired → CleanupFailed. Those are the two intended allowlist rows, and no production caller reaches can_transition_to or transition with a cleanup target.

Medium

1. crates/l30_storage/sbx/mgr/store/src/db/tests/state_write_table.rs:131-149: the edge-table test can't catch several source-set changes, including the two properties the split is meant to keep.
   • Why: classify merges edges into one entry per (from, to) pair and records only which kinds of edge write it. So removing an edge whose pair another edge also writes passes, and so does widening an edge into a pair that's already allowlisted.
   • What I ran: each of these mutations passed the store crate's state_write_table, update_state_from and state::tests (310 tests):
     • removing QUARANTINE_CLEANUP_ADOPTION;
     • removing ORPHAN_CLEANUP_INSERT;
     • STOP_COMPLETE from of(&[Stopping]).or_owned_by(Failed);
     • CLEANUP_COMPLETE_STOPPED also from plain Stopping;
     • SUSPEND_ARTIFACT_CLEANUP with .or_owned_by(Failed).
   • What was caught: removing POOL_CLONE_DISPOSAL_REARM, widening CLEANUP_FAILURE with Started, and dropping CLEANUP_HANDOFF's .or_owned_by(Failed) all fail the allowlist test. ORPHAN_SUSPENDED_INSERT => CleanupRequired fails the Edge::new const assert.
   • The effect encoding is sound: an Edge always clears the cleanup state, because the generic writer sets cleanup_state = NULL, and a CleanupEdge always sets its target. That's how the type stands in for the plan's I2 declarations.
   • What isn't pinned: that STOP_COMPLETE excludes cleanup-owned rows, and that CLEANUP_COMPLETE_* reads only from them. Those edges are documentation-only today (no production caller passes them to update_state_from), so a wrong table misleads readers rather than breaking a write.
   • Suggested fix: a per-edge snapshot (name → legacy sources), or explicit assertions on those two properties.

Low

2. state_write.rs:11: one module-doc line runs past 120 characters.

Verified against 58067a35970669f3fdb7f2e8bb450201d4fcd3ce. The baseline is 315 of 315, including cleanup_state_split and every_allowed_state_transition_tracks_the_change_time. STOP_COMPLETE's legacy sources are exactly {Stopping}, and the generic writer filters on legacy_state IN (...), so it refuses a stopping + failed row. CLEANUP_COMPLETE_* and CLEANUP_QUARANTINE read from exactly {CleanupRequired, CleanupFailed}. Not checked: the specialised writers' own SQL, which this PR doesn't touch.

[the-gigi-pplx]

Approved. 2 non-blocking findings in the review comment above.

[zibi-pplx]

Thanks @the-gigi-pplx. Rebased the stack on main (47f3aa39); this PR's head is now f2ae9543c468.

1. Per-edge pin. Added each_edge_reads_exactly_its_listed_sources: a 68-row table of every lifecycle and cleanup edge's legacy sources and target, in declaration order, compared to EDGES and CLEANUP_EDGES. Your five mutations (a removed edge, STOP_COMPLETE or CLEANUP_COMPLETE_STOPPED widened, SUSPEND_ARTIFACT_CLEANUP with .or_owned_by(Failed)) each change a row. I generated the table from a model of the edges that reproduces the base allowlist exactly.
2. Long line. Wrapped.

The pull request changed after this approval. CodeRabbit approves again after it reviews the current head.

The pull request changed after this approval. CodeRabbit approves again after it reviews the current head.

The pull request changed after this approval. CodeRabbit approves again after it reviews the current head.