You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
stores: own the three unowned tables that one store alone writes - #1693
archcheck's state checks list 90 live tables that no store declares ([layers.unowned]). This gives an owner to the three that exactly one store writes, where that store is the one whose data the table is:
runtime_environments → pplx-space-runtime-environment-catalog-store. Its crate docs already describe it as the catalog's data access, and its record_node_certifications and recompute_all_edges are the table's only production writers. Operators set retired_at by hand, and tests seed the table directly.
node_k8s → pplx-space-node-registration-store. Its register_k8s_node is the only statement in any store's or storage helper's production code that writes the table. Other crates write it only from test fixtures, and its rows go with the nodes row through ON DELETE CASCADE when the api or the cluster controller deletes a node. nodes, which the same function upserts, stays unowned, because most of its writes are in the api and the cluster controller.
warm_pool_policies → pplx-space-cluster-pool. Its WarmPoolStore reads the policies for autoscaling and deletes the disabled ones together with their pool configs. The admin route's upsert in pplx-space-api stays inline until it moves behind the store.
No other store writes any of these, so the declarations add no state.writes finding and need no co-writer grant. [layers.unowned] goes from 90 to 87. Nothing changes in SQL, schema, migrations, metrics or wire formats.
Tables that one store writes only as a step of another table's transition, or that two stores write, keep their entries until their owner is decided.
Validation
cargo arch and ci/scripts/run-archcheck.sh against the base: no required findings.
cargo fmt --check, cargo check --workspace --all-targets, and clippy and nextest on the three crates: pass.
run-rust-checks.shclippy, isolated-crates and tests-unprivileged: pass, apart from the guest-process and node-mgr privileged-path tests, which fail on hosts without those paths.
Severity and risk
Severity: does not apply. Risk: a manifest metadata change that only archcheck reads, so nothing changes at build or run time. Roll back by reverting.
AI assistance: Claude Code did the census, the change and this description.
No actionable comments were generated in the recent review.
Walkthrough
The PostgreSQL state declarations now include node_k8s, runtime_environments, and warm_pool_policies. The architecture baseline removes these tables from the PostgreSQL unowned-table list.
Merge Risk: Minimal · up to e4b52
This is a metadata-only change that records which storage crates own three database tables. It does not alter runtime behavior, schemas, or APIs, and it leaves no concrete merge risk.
Security architecture risk: Minimal · up to 6f3bf
The changes declare existing storage ownership without changing database access, privileges, schemas, or runtime behavior. No introduced or worsened security issue was identified. Existing writes outside the declared stores remain an important limitation of exclusive ownership.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The changed authority is limited to static ownership classification for three tables. The PR adds no runtime database privileges, externally reachable entrypoints, or deployment changes.
Trust Boundaries and Controls
observed — Existing write-ownership checks permit declared owners or granted co-writers and scan storage packages and designated helpers. They do not cover every non-storage writer or indirect database write. Removing the unowned entries records ownership; it does not establish a database authorization boundary.
Pre-merge checks: 6
Check name
Status
Explanation
Title check
The title uses the lowercase area prefix "stores:" followed by a colon and a lowercase summary, matching the required format. It states the main change: the three unowned tables get owner declarations…
Description Follows The Pr Template
The PR description contains all four required items, each with real content. This check tests presence only, so no repository tools were needed. 1. Summary: A ## Summary section states what merg…
Bug Fixes Name A Regression Test
The check applies only when the PR fixes a defect in application behavior under crates/, shim/, or ui/. This PR does not fix a defect. Its title, description, and objectives describe adding state-owne…
Weakened Safeguards Are Justified
The PR changes four files: architecture-baseline.toml and three crate Cargo.toml manifests. None of them removes or weakens a test, assertion, lint, CI check, or repository guard. - architecture-basel…
Agent Tooling Runs No New Commands
The check does not apply to this diff. The pull request changes four files: architecture-baseline.toml and the [package.metadata.space.state] sections of three Cargo.toml files (`crates/l30_storag…
Public-Safe Content
The diff touches four files: architecture-baseline.toml and three Cargo.toml manifests. Each change only adds or removes table names in [package.metadata.space.state] lists and the unowned-table…
Full details: Title check
Explanation
The title uses the lowercase area prefix "stores:" followed by a colon and a lowercase summary, matching the required format. It states the main change: the three unowned tables get owner declarations because one store alone writes each of them. It has no trailing period, ticket ID, or internal link. Its wording is accurate to the objectives, which list the three tables mapped to their stores.
Full details: Description Follows The Pr Template
Explanation
The PR description contains all four required items, each with real content. This check tests presence only, so no repository tools were needed. 1. Summary: A ## Summary section states what merging does. It gives the owner for each of the three tables (runtime_environments, node_k8s, warm_pool_policies) and the resulting change from 90 to 87 unowned tables. 2. Validation: A ## Validation section names the commands run (cargo arch, ci/scripts/run-archcheck.sh, cargo fmt --check, cargo check, clippy, nextest, run-rust-checks.sh stages) and gives each result. It also notes the two privileged-path test failures and their cause. 3. Severity and risk: A ## Severity and risk section states "Severity: does not apply." It gives the risk as a manifest metadata change read only by archcheck, with rollback by reverting. 4. AI assistance: A line starts with "AI assistance:" and names the tool and its drafted parts: "Claude Code did the census, the change and this description." No item is missing or empty.
Full details: Bug Fixes Name A Regression Test
Explanation
The check applies only when the PR fixes a defect in application behavior under crates/, shim/, or ui/. This PR does not fix a defect. Its title, description, and objectives describe adding state-owner declarations for three tables: runtime_environments, node_k8s, and warm_pool_policies. The raw summary shows the changes are to architecture-baseline.toml (removing three unowned-table entries) and to Cargo.toml feature and metadata lists in four crates. The author states the change is "a manifest metadata change that only archcheck reads" and that "nothing changes at build or run time." No bug, regression, or failing behavior is named anywhere in the PR. Because the PR is not an application bug fix, the check passes and no regression test is required.
Full details: Weakened Safeguards Are Justified
Explanation
The PR changes four files: architecture-baseline.toml and three crate Cargo.toml manifests. None of them removes or weakens a test, assertion, lint, CI check, or repository guard. - architecture-baseline.toml (lines 212-263 of the diff): removes node_k8s, runtime_environments, and warm_pool_policies from the postgres unowned-table list. This shrinks an exception list, so the archcheck guard becomes stricter. The check fails on additions to baselines, not removals. - crates/l30_storage/cluster/pool/Cargo.toml: adds warm_pool_policies to [package.metadata.space.state] postgres. This is an ownership declaration, not a suppression. - crates/l30_storage/node/registration/store/Cargo.toml: changes postgres = [] to postgres = ["node_k8s"]. This is an ownership declaration. - crates/l30_storage/runtime/environment/catalog/store/Cargo.toml: adds runtime_environments to the postgres list. This is an ownership declaration. A search of the full diff found no added #[allow] or #[expect], no skip, xfail, or ignore markers, no continue-on-error, no SPACE_TEST_DEPS_OPTIONAL, and no edits to AGENTS.md, CLAUDE.md, CONTRIBUTING.md, or review guidance. No [lints] table changed. The PR author is ac-pplx, and the check does not depend on author identity.
Full details: Agent Tooling Runs No New Commands
Explanation
The check does not apply to this diff. The pull request changes four files: architecture-baseline.toml and the [package.metadata.space.state] sections of three Cargo.toml files (crates/l30_storage/cluster/pool/Cargo.toml, crates/l30_storage/node/registration/store/Cargo.toml, crates/l30_storage/runtime/environment/catalog/store/Cargo.toml). The changes list database table ownership only. None of the files is an agent plugin manifest (plugin.json, plugin.yaml), a SKILL.md, a command or skill file, a guideline file, or an MCP/LSP/hooks configuration. A search of the changed paths for those patterns returned no matches. The diff adds no hook, MCP server, LSP server, frontmatter key, inline shell substitution, or other content that agent tools run. The PR author is ac-pplx, but the check does not use author identity.
Full details: Public-Safe Content
Explanation
The diff touches four files: architecture-baseline.toml and three Cargo.toml manifests. Each change only adds or removes table names in [package.metadata.space.state] lists and the unowned-table list. I found no credentials, tokens, private keys, internal hostnames or URLs, cloud account IDs, ARNs, cluster or environment names, personal workstation paths, production logs or measurements, incident details, or undisclosed vulnerability descriptions. The PR title and description name crates, tables, and test categories. The description refers to a test fixture and an operator action in general terms. It mentions "Claude Code" as AI assistance. It also gives validation results and a rollback plan, which contain no sensitive values. The PR objectives match the diff.
Recent review info
Automatic approval policy
Commit: e4b5278aeb70.
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.
Reviewing files that changed from the base of the PR and between 1b7fc58 and e4b5278.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
archcheck's state checks list 90 live tables that no store declares in
[layers.unowned]. Three of them have exactly one store writing them, and
that store is the one whose data they are:
- runtime_environments: pplx-space-runtime-environment-catalog-store.
Its crate docs already describe it as the catalog's data access, and
its record_node_certifications and recompute_all_edges are the table's
only production writers. Operators set retired_at by hand, and tests
seed the table directly.
- node_k8s: pplx-space-node-registration-store. Its register_k8s_node is
the only statement in any store's or storage helper's production code
that writes the table. Other crates write it only from test fixtures,
and its rows go with the nodes row through ON DELETE CASCADE when the
api or the cluster controller deletes a node. nodes, which the same
function upserts, stays unowned, because most of its writes are in
the api and the cluster controller.
- warm_pool_policies: pplx-space-cluster-pool. Its WarmPoolStore reads
the policies for autoscaling and deletes the disabled ones along with
their pool configs. The admin route's upsert in the api crate stays
inline until it moves behind the store.
Each store now declares its table, and the three entries leave
[layers.unowned] (90 to 87). No other store writes any of them, so the
declarations add no state.writes finding and need no co-writer grant.
No SQL, schema or migration changes.
Some tables keep their entries until their owner is decided: tables
that one store writes only as a step of another table's transition
(the sandbox store's sibling rows, the migrator's bootstrap password
reset), and tables that two stores write.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
shehan-pplx
left a comment
Summary
archcheck's state checks list 90 live tables that no store declares (
[layers.unowned]). This gives an owner to the three that exactly one store writes, where that store is the one whose data the table is:runtime_environments→pplx-space-runtime-environment-catalog-store. Its crate docs already describe it as the catalog's data access, and itsrecord_node_certificationsandrecompute_all_edgesare the table's only production writers. Operators setretired_atby hand, and tests seed the table directly.node_k8s→pplx-space-node-registration-store. Itsregister_k8s_nodeis the only statement in any store's or storage helper's production code that writes the table. Other crates write it only from test fixtures, and its rows go with thenodesrow throughON DELETE CASCADEwhen the api or the cluster controller deletes a node.nodes, which the same function upserts, stays unowned, because most of its writes are in the api and the cluster controller.warm_pool_policies→pplx-space-cluster-pool. ItsWarmPoolStorereads the policies for autoscaling and deletes the disabled ones together with their pool configs. The admin route's upsert inpplx-space-apistays inline until it moves behind the store.No other store writes any of these, so the declarations add no
state.writesfinding and need no co-writer grant.[layers.unowned]goes from 90 to 87. Nothing changes in SQL, schema, migrations, metrics or wire formats.Tables that one store writes only as a step of another table's transition, or that two stores write, keep their entries until their owner is decided.
Validation
cargo archandci/scripts/run-archcheck.shagainst the base: no required findings.cargo fmt --check,cargo check --workspace --all-targets, and clippy and nextest on the three crates: pass.run-rust-checks.shclippy,isolated-cratesandtests-unprivileged: pass, apart from the guest-process and node-mgr privileged-path tests, which fail on hosts without those paths.Severity and risk
Severity: does not apply. Risk: a manifest metadata change that only archcheck reads, so nothing changes at build or run time. Roll back by reverting.
AI assistance: Claude Code did the census, the change and this description.