Repository navigation
api: delete control-plane wrappers for node API calls nothing reaches - #1156
Merged
Merged
Conversation
|
Thanks for opening this pull request! A SPACE maintainer will take a look. CONTRIBUTING.md explains how we review outside contributions. |
ac-pplx
force-pushed
the
ac/api-delete-dead-node-api-wrappers
branch
from
October 30, 2026 18:10
92f0822 to
71c352e
Compare
ac-pplx
marked this pull request as ready for review
October 30, 2026 21:21
Collaborator
ReviewNo findings. What I checked:
Not checked: I didn't run cargo, and relied on this head's green CI ( Verified against |
the-gigi-pplx
approved these changes
Oct 30, 2026
the-gigi-pplx
left a comment
the-gigi-pplx
left a comment
Collaborator
There was a problem hiding this comment.
Approved. No findings; details in the review comment above.
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Oct 30, 2026
Remove nine api-crate wrapper methods that no route, controller or loop calls: the SandboxController spaced_ping_guest and spaced_get_status wrappers with their spaced_client_call! rows, and the TemplateController list_templates, get_template, update_template_settings, delete_template, create_pool, delete_pool and pool_status wrappers. The admin template delete route persists the operation for the cluster controller to execute, and the admin node-template delete calls the node API directly, so neither used the delete_template wrapper. The RPCs, protos, node-side handlers and client crate methods are unchanged. Drop the imports the deletions leave unused, lower the spaced.rs line cap in the architecture baseline, and remove a deleted wrapper's name from a Kubernetes substrate test comment. The Kind builder chart's comment on the trusted-policy allowlist cited the deleted TemplateController calls; it now cites the admin node-template deletes that still need the grant.
ac-pplx
force-pushed
the
ac/api-delete-dead-node-api-wrappers
branch
from
October 30, 2026 23:24
71c352e to
033a601
Compare
ac-pplx
added a commit
that referenced
this pull request
Nov 2, 2026
Every archcheck rule is to be required from the day it lands, with exceptions that only shrink: no new baseline entries, no raised caps, and stale entries removed. A rule that can't be enforced that way is deleted. The README now states this policy under "Configuration and modes". pattern_ratchet.direct_tables, also advisory, stays because open pull requests shrink it (#1202). The advisory file_size rule fails that test. Its baseline was seeded with 99 entries on 2026-10-13, reached 102, and lists 98 production files over 1,000 code lines today; #1156 and #1159 lowered caps. What can't be made required is listed_no_growth: in the 14 days to 2026-10-31, 149 of main's 371 first-parent commits (40%) added more lines than they deleted to one of the 98 listed files (git log --numstat, raw lines). Requiring it would block about that share of pull requests. The change removes the file_size module, its configuration code and its [rules.file_size] tables in ARCHITECTURE.toml, its [file_size] baseline table and base-copy read, its part of the generated header, and --write-baseline=file_size. ProductionFile's code_lines field and count_code_lines go too, since only file_size read them. The baseline was edited by hand, so the other tables keep their comments, and its header matches what --write-baseline now generates. Kept: the shared count ratchet (rules/ratchet.rs), which pattern_ratchet uses, and the code-line definition (context::code_lines). Two small changes follow from [file_size] no longer always being present: - Baseline::write puts one blank line between whichever parts it writes. Without this, a baseline with no table of caps gets a doubled blank line. - When the base baseline is not TOML and no rule compares tables with it, skipped() falls back to each rule's own lines. Without this, the warning ends in "; skipped". crate_names then reports the unreadable copy itself. Tests that used file_size for behaviour the pattern ratchets share now use a pattern_ratchet fixture. These cover renames (merged into pattern_ratchet_renames), --renames from git, the human report, rdjson (document, exact paths, stdout/stderr), base-baseline problems, the skip-line and no-base checks, implicit members, scoped and unscoped --write-baseline, and write errors. The rows of lenient_base_baseline and behaviours not covered elsewhere moved into the pattern_ratchet tables. Tests that only exercised file_size are deleted: its behaviours table, its modes test, its limit digit grouping, write_baseline_output's limit cases, and configuration rows for its keys. New assertions pin Baseline::write's layout, with and without a table of caps, and the one warning that names every pattern_ratchet instance's comparisons for a base copy that is not TOML. The layers tests change only their fixtures and the expectations that follow from them: their [file_size] lines are gone from configurations and baselines, so expected baseline line numbers move up by one, the warning for a base copy that is not TOML no longer names file_size's comparisons, and a scoped write keeps a stale [crate_names] entry where it kept a stale [file_size] one. The CI ordering script's comment no longer says the report lists file_size findings first. Its test fixtures use a pattern_ratchet code and table, with each case's meaning unchanged. Five api test modules' doc comments no longer cite the file-size guidance.
meefs
pushed a commit
to meefs/SPACE
that referenced
this pull request
Nov 4, 2026
…rplexityai#1222) Every archcheck rule is to be required from the day it lands, with exceptions that only shrink: no new baseline entries, no raised caps, and stale entries removed. A rule that can't be enforced that way is deleted. The README now states this policy under "Configuration and modes". pattern_ratchet.direct_tables, also advisory, stays because open pull requests shrink it (perplexityai#1202). The advisory file_size rule fails that test. Its baseline was seeded with 99 entries on 2026-10-13, reached 102, and lists 97 production files over 1,000 code lines today; perplexityai#1156 and perplexityai#1159 lowered caps. What can't be made required is listed_no_growth: in the 14 days to 2026-10-31, 149 of main's 371 first-parent commits (40%) added more lines than they deleted to one of the 97 listed files (git log --numstat, raw lines). Requiring it would block about that share of pull requests. Two sbx-mgr SQLite checks go too, since the required layers rule now covers them: layers.storage_access lets only stores, storage helpers and baselined edges depend on a database driver or a storage helper, and layers.store_edges lets only a store's listed users depend on it. - dependency_fence, the whole rule, with its sbxmgr_rusqlite and sbxmgr_sqlx instances. Every package they fenced (rusqlite, libsqlite3-sys, pplx-space-sqlite, sqlx, sqlx-core, sqlx-sqlite) is a [rules.layers.storage] driver or helper, which storage_access keeps out of the normal and build dependencies of every sbx-mgr crate but its two stores. - pattern_ratchet.sbxmgr_sqlite: twelve regexes for SQLite calls and SQL literals in sbx-mgr's crates, which aliases escaped; storage_access confines SQLite to the stores at the crate level instead. pattern_ratchet.sbxmgr_sqlite_schema stays, because layers does not see what it catches: the storage checks read only dependency edges, skip stores and helpers and tolerate baselined edges, and state.writes scans only stores' and helpers' write statements, so db-migrate, a store with sqlx and rusqlite, could name sbx-mgr's migrator again, and a node service with a baselined driver edge could open SANDBOX_MANAGER_DB. Its comment now says so instead of contrasting it with sbxmgr_sqlite. What this narrows: - pplx-space-sbxmgr-migrations, an l30_storage crate, may now take rusqlite, libsqlite3-sys and pplx-space-sqlite, which sbxmgr_rusqlite allowed only the store. CI's run_sbxmgr_production_checks, which asserts which crates bring rusqlite and pplx-space-sqlite into the server's normal graph, still rejects a normal dependency on either there, but not a build or dev dependency, nor libsqlite3-sys. - The migrations crate's sources are no longer scanned for SQL text; state.writes still reports a write statement there that names another store's table. - Dev-dependencies, which the fences covered and storage_access leaves to tests. - pplx-space-sbx-mgr's baselined storage_access edge to the pplx-space-common-jobs helper, which it uses for job types today and which sbxmgr_sqlite watched for calls into common-jobs' SQL functions. sbxmgr_sqlite's baseline table was empty. The rule's module, configuration and context code, its cli test, sbxmgr_sqlite's repository tests and their helper, and the rule's README section are deleted. The fence test's dev-dependency case was the only test of a finding on a dependency declared in a table of its own ([dependencies.sqlx]), which places every layers finding on such a dependency; layers_storage_checks gains that case. Comments in ARCHITECTURE.toml, run-rust-checks.sh, the sbx-mgr store's AGENTS.md and pplx-space-sqlite's crate docs now name storage_access where they named the fences. scripts/move_crates.py drops its check of dependency_fence member globs, and that check's test. The change removes the file_size module, its configuration code and its [rules.file_size] tables in ARCHITECTURE.toml, its [file_size] baseline table and base-copy read, its part of the generated header, and --write-baseline=file_size. ProductionFile's code_lines field and count_code_lines go too, since only file_size read them. The baseline was edited by hand, so the other tables keep their comments, and its header matches what --write-baseline now generates. Kept: the shared count ratchet (rules/ratchet.rs), which pattern_ratchet uses, and the code-line definition (context::code_lines). Two small changes follow from [file_size] no longer always being present: - Baseline::write puts one blank line between whichever parts it writes. Without this, a baseline with no table of caps gets a doubled blank line. - When the base baseline is not TOML and no rule compares tables with it, skipped() falls back to each rule's own lines. Without this, the warning ends in "; skipped". crate_names then reports the unreadable copy itself. Tests that used file_size for behaviour the pattern ratchets share now use a pattern_ratchet fixture. These cover renames (merged into pattern_ratchet_renames), --renames from git, the human report, rdjson (document, exact paths, stdout/stderr), base-baseline problems, the skip-line and no-base checks, implicit members, scoped and unscoped --write-baseline, and write errors. The rows of lenient_base_baseline and behaviours not covered elsewhere moved into the pattern_ratchet tables. Tests that only exercised file_size are deleted: its behaviours table, its modes test, its limit digit grouping, write_baseline_output's limit cases, and configuration rows for its keys. New assertions pin Baseline::write's layout, with and without a table of caps, and the one warning that names every pattern_ratchet instance's comparisons for a base copy that is not TOML. Apart from the layers_storage_checks case above, the layers tests change only their fixtures and the expectations that follow from them: their [file_size] lines are gone from configurations and baselines, so expected baseline line numbers move up by one, the warning for a base copy that is not TOML no longer names file_size's comparisons, and a scoped write keeps a stale [crate_names] entry where it kept a stale [file_size] one. The CI ordering script's comment no longer says the report lists file_size findings first. Its test fixtures use a pattern_ratchet code and table, with each case's meaning unchanged. Five api test modules' doc comments no longer cite the file-size guidance.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Deletes nine control-plane wrapper methods in the api crate that no route, controller or loop calls. They made the node-API RPCs behind them look used by the control plane.
SandboxController::spaced_ping_guestandspaced_get_status, plus their rows in thespaced_client_call!table onSpacedSandboxClient.TemplateController::list_templates,get_template,update_template_settings,create_pool,delete_pool,pool_statusanddelete_template.spaced.rsline cap inarchitecture-baseline.toml. A comment incontrollers/substrate/kubernetes/tests.rsnamed a deleted wrapper, so that name is removed.infra/space/kind/charts/space-node-api-local/values-builder.yamlcited the deletedTemplateControllercalls. It now cites the caller that still needs the grant:delete_node_templateincrates/libs/api/src/routes/admin/node_maintenance/node_templates.rs, which callsDeleteTemplateandDeleteTemplateBuildon builders. The allowlist itself is unchanged.No RPC, proto, node-side handler or client-crate method changes. Eight of the wrappers fronted RPCs that nothing else in this repository calls through node-api:
SpacedPingGuest,SpacedGetStatus,ListTemplates,GetTemplate,UpdateTemplateSettings,CreatePool,DeletePoolandPoolStatus.DeleteTemplateis still called directly by the admin node-template delete.Reachability: none of the nine names is called anywhere in
crates/,docs/,space_smoke/,scripts/orexamples/. Every other hit is a definition or is on a different type (the sbx-mgr and template-mgr CLIs and client crates, node-api server and sim, node-ctl, and the api's own route handlers of the same names). The onlystate.template_controllercallers aresubmit_template_build,poll_template_build_status,get_template_build_status_by_job_id,upload_template_build_blobandlist/upsert/disable_cluster_pool_config; several are written as.template_controllerand.method(on separate lines, so a single-line grep misses them. Nothing is reached through a trait object or generated code:SpacedSandboxClientis a concrete struct, and the macro rows existed only for the two deleted wrappers.RuntimeOperationBudget::RuntimeMetadatastays: the spaced host bridge maps its ownRuntimeMetadatabudget to it and uses that mapping.Validation
Rebased onto current main at
033a601e(the only conflict: #1167 had edited a line inside the deletedTemplateController::list_templates; kept the deletion). At that head:cargo check --workspace --all-features --all-targets, api clippy,cargo arch, and api nextest with Postgres and Redis (2128 passed).Run on the Rust code at this head (the last commit changes only a comment in a Kind values file):
cargo check --workspace --all-features --all-targets: finished, no warnings.cargo check -p pplx-space-api(withoutkubernetes): finished.cargo clippy -p pplx-space-api --locked --all-targets --all-features -- -D warnings: clean.cargo nextest run -p pplx-space-api --all-features --no-fail-fastagainst real Postgres: 2091 run, 2091 passed, 1 skipped by the nextest config; no test-dependency skips.cargo arch: exit 0, 0 required findings.No test was added or removed: nothing referenced the deleted wrappers, and a deletion with no remaining caller has no behavior to pin. The Kind smoke shards and the full-workspace clippy were not run locally: CI on this head.
Severity and risk
Severity: does not apply (dead-code cleanup). Risk: none at runtime, since nothing calls the deleted code and nothing about the wire, schema or config changes. Roll back by reverting the commits.
Rollout
Control-plane-only. No mixed-version concerns: node-api, sbx-mgr and template-mgr see the same RPC surface.
Textual overlaps with open pull requests; whichever lands second rebases:
TemplateController::list_templatesincontrollers/template.rs. The resolution keeps the deletion.sbx_mgr_client::protoimport list incontrollers/sandbox/mod.rs. The resolution keeps both import changes and re-runs rustfmt.AI assistance: Claude Code wrote the code change and this description.