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
archcheck: require every remaining base comparison - #1360
#1222 states archcheck's policy: a behaviour is required from the day it
lands, and a baseline only shrinks. Merging this makes the base comparisons
that were still advisory required, so a pull request that adds a baseline
entry or raises a cap fails instead of being annotated:
all five behaviours of pattern_ratchet.direct_tables;
no_new_entries and no_raised_caps of kubernetes_access, capability_identity, substrate_literals and sandbox_writes;
no_new_entries of rootfs_copy and node_manager_commands;
crate_names.stale_entries.
The tree has no findings for these, so the baseline is unchanged.
The pull request that adds a pattern_ratchet instance seeds its table, as
for the layers checks: while the base branch has no such instance, each entry
is an advisory no_new_entries finding. With --base-architecture, "no such
instance" means the base ARCHITECTURE.toml doesn't configure it; a base that
configures it without a table tolerates nothing, and a base configuration that
can't be read counts as configuring it. Without this, adding an instance would
fail on its own entries.
Merge groups now run the same base comparisons, against the group's base_sha, through ci/scripts/run-archcheck.sh, which the pull request
path now calls too: a required finding fails the group, without reviewdog.
Before, the merge queue ran none of them, including the already-required layers comparisons and crate_names.no_new. A group whose trees a passing
Pull CI run already checked still reuses that run.
Three behaviours stay advisory because they are notices, not checks: crate_names.policy_changed and layers.policy_changed report a policy edit
for review, and layers.state.pending is how a new table lands, declared
before its migration. The README says so in place of "they are being made
required". The README, the CLI's --write-baseline advice, the substrate AGENTS.md, the rootfs-backing design note and the comments in ARCHITECTURE.toml no longer describe a hand-added entry as a reviewed
exception that is only reported. The README also says that an instance's
name, paths, patterns and modes are policy, changed only in review (archcheck
does not report such edits for pattern ratchets yet), and that a listed
file's matches move into the allowed paths before the file is split.
cargo test -p pplx-space-archcheck: all pass. The new pattern_ratchet_seeds_a_new_instance covers a base without the instance
(advisory, exit 0), no --base-architecture (seeded, exit 0), a base that
configures the instance without a table (required, exit 1), a base baseline
with the table while the base configuration lacks the instance, and a base
configuration that can't be read (warning, tolerates nothing). Each fails
when its part of the change is reverted.
ci/scripts/run-archcheck.sh on scratch commits: a merge-group-like base
and a pull-request-like synthetic merge that add a kubernetes_access
entry both exit 1; clean ones exit 0; an empty base warns, skips the
comparisons and exits 0. It also runs under bash 3.2 and 4.3 (checked in
containers with a stub cargo). shellcheck is clean.
cargo arch: exit 0, no findings.
The README's "check a branch as CI does" recipe against archcheck: delete file_size and the checks the layers rule covers #1222's head, with
its ARCHITECTURE.toml as --base-architecture: exit 0, no required
findings; the one advisory finding is crate_names.policy_changed reporting
this change to crate_names.stale_entries.
Severity: does not apply; this changes which existing findings fail.
Risk: pull requests that add a baseline entry or raise a cap fail once this
lands, including #559 and #806 as they stand; their new writes need to go
through the owning store, or the change needs a configuration edit reviewed in ARCHITECTURE.toml. A queued pull request whose last Pull CI compared with an
older base is compared again in its merge group, and fails there if it adds an
entry or raises a cap. Splitting a listed file into modules also fails while
the file has matches, since --renames pairs only whole-file renames and the
new files' entries are new: move the matches out first. Moving whole files is
unaffected. Roll back by reverting this commit, which also removes the merge
queue's comparisons.
AI assistance: Claude Code drafted the code, tests and this description.
Summary by CodeRabbit
CI
Architecture checks now run for pull requests and merge queues. Pull requests receive review annotations; merge-queue checks can block a change.
Architecture checks
Several architecture rules now fail on new violations instead of only reporting them.
Baseline comparisons account for newly configured rules and detected file renames; missing or unreadable comparison data may cause checks to be skipped.
Documentation
Updated guidance explains rule enforcement, baseline updates, merge-queue checks, and limitations of root-file copy detection.
#1222 states the policy: a behaviour is required from the day it lands
and a baseline only shrinks. This makes the behaviours that were still
advisory required:
- all five behaviours of pattern_ratchet.direct_tables;
- no_new_entries and no_raised_caps of kubernetes_access,
capability_identity, substrate_literals and sandbox_writes;
- no_new_entries of rootfs_copy and node_manager_commands;
- crate_names.stale_entries.
A pull request that adds a baseline entry or raises a cap in any of
these tables now fails instead of being annotated, and so does a stale
crate_names entry. The tree has no findings for them, so the baseline is
unchanged.
Merge groups now run the same comparison against the group's base_sha,
through ci/scripts/run-archcheck.sh, which pull requests share, and a
required finding fails the group.
The pull request that adds a pattern_ratchet instance seeds its table,
as for the layers checks: while the base branch has no such instance,
each entry is an advisory no_new_entries finding, and a base that
configures the instance without a table tolerates nothing.
Three behaviours stay advisory because they are notices, not checks:
crate_names.policy_changed and layers.policy_changed report a policy
edit for review, and layers.state.pending is how a new table lands,
declared before its migration.
The README, archcheck's not-added advice, the rootfs-backing design
note, the substrate AGENTS.md, split_rust_module.py and the comments in
ARCHITECTURE.toml no longer describe a new entry as a reviewed
exception that is only reported.
No actionable comments were generated in the recent review.
Walkthrough
Archcheck now seeds pattern-ratchet baselines based on base configuration, and selected architecture checks use required modes. A shared script handles base files and renames. CI invokes it for pull requests and merge groups, with event-specific status handling.
The ratchet engine reads configured instances from the base architecture and distinguishes comparison, seeding, and skipped states. CLI output, documentation, and tests describe and verify these cases.
Selected pattern-ratchet behaviors change from advisory to required. The architecture contract and related documentation describe the affected checks and entry rules.
Pull-request and merge-group CI ci/scripts/run-archcheck.sh, .github/workflows/rust-static-ci.yml, crates/tools/archcheck/README.md
The shared script prepares base architecture and baseline data, detects renames, and runs cargo arch. The workflow uses event-specific status handling and runs reviewdog only for pull requests.
sequenceDiagram
participant Workflow as rust-static-ci.yml
participant Script as run-archcheck.sh
participant Cargo as cargo arch
participant Reviewdog as reviewdog
Workflow->>Script: pass event-specific base and output path
Script->>Script: load base files and detect renames
Script->>Cargo: run architecture comparison
Cargo-->>Script: return findings and status
alt Pull request
Script-->>Workflow: write rdjson and return status
Workflow->>Reviewdog: annotate findings when archcheck succeeds
else Merge group
Script-->>Workflow: emit text and return status
end
Loading
Suggested reviewers:zibi-pplx
Merge Risk: Minimal · up to ddfba
No actionable merge-blocking issue was established. The architecture checks are ready to merge after normal checks.
Security architecture risk: Low · up to ddfba
The change makes more repository checks mandatory and applies them in the merge queue. Remaining risk is concentrated in policy edits and recovery paths where unavailable comparisons produce warnings rather than automatically blocking a merge.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The affected enforcement scope is repository-wide architecture acceptance for pull requests and merge groups. A comparison skip can remove multiple baseline checks from that run; it is not scoped to a single changed file.
Trust Boundaries and Controls
observed — Current policy remains editable in the pull request. Renaming an instance seeds a new identity, and changing patterns, allowed paths, or modes can weaken enforcement. These are explicitly documented review-controlled policy changes; the checker is not an independent authorization boundary against an author permitted to change that policy.
Resilience and Maintainability Implications
observed — Failure to resolve the base or obtain a base copy remains warning-only preparation behavior: affected flags are omitted and checks continue. This fallback predates the PR's shared runner, so it is an existing assurance limitation rather than an established newly introduced bypass.
Hardening Proposals
proposed — Consider explicit pattern-ratchet policy-change reporting for instance renames, matcher changes, allowed-path changes, and mode reductions. This would make deliberate resets more visible during review; it is supplementary governance, not a fix for a verified privilege escalation.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title clearly and concisely describes the main change: making the remaining archcheck base comparisons required.
Description check
The description includes complete Summary, Validation, Severity and risk, Stack, and AI assistance sections. It explains the behavior changes, test evidence, risks, rollback, and merge dependency.
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 13 functions across 8 files. (5 skipped: 5 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 3b15c772-6f7d-4a23-a185-c44741e408d9
Commits
Reviewing files that changed from the base of the PR and between e23b68c and ddfbae0.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
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.
Summary
#1222 states archcheck's policy: a behaviour is required from the day it
lands, and a baseline only shrinks. Merging this makes the base comparisons
that were still advisory required, so a pull request that adds a baseline
entry or raises a cap fails instead of being annotated:
pattern_ratchet.direct_tables;no_new_entriesandno_raised_capsofkubernetes_access,capability_identity,substrate_literalsandsandbox_writes;no_new_entriesofrootfs_copyandnode_manager_commands;crate_names.stale_entries.The tree has no findings for these, so the baseline is unchanged.
The pull request that adds a
pattern_ratchetinstance seeds its table, asfor the layers checks: while the base branch has no such instance, each entry
is an advisory
no_new_entriesfinding. With--base-architecture, "no suchinstance" means the base
ARCHITECTURE.tomldoesn't configure it; a base thatconfigures it without a table tolerates nothing, and a base configuration that
can't be read counts as configuring it. Without this, adding an instance would
fail on its own entries.
Merge groups now run the same base comparisons, against the group's
base_sha, throughci/scripts/run-archcheck.sh, which the pull requestpath now calls too: a required finding fails the group, without reviewdog.
Before, the merge queue ran none of them, including the already-required
layerscomparisons andcrate_names.no_new. A group whose trees a passingPull CI run already checked still reuses that run.
Three behaviours stay advisory because they are notices, not checks:
crate_names.policy_changedandlayers.policy_changedreport a policy editfor review, and
layers.state.pendingis how a new table lands, declaredbefore its migration. The README says so in place of "they are being made
required". The README, the CLI's
--write-baselineadvice, the substrateAGENTS.md, the rootfs-backing design note and the comments inARCHITECTURE.tomlno longer describe a hand-added entry as a reviewedexception that is only reported. The README also says that an instance's
name, paths, patterns and modes are policy, changed only in review (archcheck
does not report such edits for pattern ratchets yet), and that a listed
file's matches move into the allowed paths before the file is split.
Validation
At this head:
cargo fmt --check,cargo clippy -p pplx-space-archcheck --all-targets --all-features --locked -- -D warnings:clean.
cargo test -p pplx-space-archcheck: all pass. The newpattern_ratchet_seeds_a_new_instancecovers a base without the instance(advisory, exit 0), no
--base-architecture(seeded, exit 0), a base thatconfigures the instance without a table (required, exit 1), a base baseline
with the table while the base configuration lacks the instance, and a base
configuration that can't be read (warning, tolerates nothing). Each fails
when its part of the change is reverted.
ci/scripts/run-archcheck.shon scratch commits: a merge-group-like baseand a pull-request-like synthetic merge that add a
kubernetes_accessentry both exit 1; clean ones exit 0; an empty base warns, skips the
comparisons and exits 0. It also runs under bash 3.2 and 4.3 (checked in
containers with a stub
cargo).shellcheckis clean.cargo arch: exit 0, no findings.its
ARCHITECTURE.tomlas--base-architecture: exit 0, no requiredfindings; the one advisory finding is
crate_names.policy_changedreportingthis change to
crate_names.stale_entries.exit 1. [space] fence every placement writer against the provider-terminate path #559 crosses
direct_tablesinsandbox/updates.rsand raises threesandbox_writescaps; Enforce template lookup scope after compatible rollout #806 raises one. With today's configuration both areonly annotated.
Severity and risk
Severity: does not apply; this changes which existing findings fail.
Risk: pull requests that add a baseline entry or raise a cap fail once this
lands, including #559 and #806 as they stand; their new writes need to go
through the owning store, or the change needs a configuration edit reviewed in
ARCHITECTURE.toml. A queued pull request whose last Pull CI compared with anolder base is compared again in its merge group, and fails there if it adds an
entry or raises a cap. Splitting a listed file into modules also fails while
the file has matches, since
--renamespairs only whole-file renames and thenew files' entries are new: move the matches out first. Moving whole files is
unaffected. Roll back by reverting this commit, which also removes the merge
queue's comparisons.
Stack
Stacked on #1222; merge after it.
AI assistance: Claude Code drafted the code, tests and this description.
Summary by CodeRabbit