Repository navigation
style(rust): repair the existing cargo fmt CI gate - #144
Merged
Merged
Conversation
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 45 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
hyperpolymath
added a commit
that referenced
this pull request
Oct 31, 2026
…ambiguous Further calibration of the wasm formatter against the 33 Rust files on main: it reproduces 26 exactly and differs on 7. Those 7 are the ground truth for where CI's rustfmt (which passes fmt+clippy on #143 and #144) is *stricter* than the emulator, and they name exactly two classes this change applies: * an assert whose whole call runs past ~60 columns goes vertical — measure: chains.rs keeps `assert_eq!(c.path.as_deref(), Some(".github/workflows/gov.yml"))` vertical at 65 columns; * a method chain past ~60 columns breaks at the dots — measure: startup.rs breaks `allow_list.is_some_and(...)` at 94 columns. Rejected, because the same 7 files contain the counter-example: zero_job.rs keeps `collect(&slug, &snapshot.files, runs, &GhRest, allow_list.as_ref(), notes)` inline at 86 columns, so the "calls over 60 go vertical" rule is not uniform and applying it would only add churn. This is the last formatting guess made without the real formatter: 7 samples support two rules and refute a third, and nothing here can settle the classes they do not cover. The remaining 9 lines over 100 columns are single string literals that cannot be split, which CI-clean code already carries at 145 columns (squabble-fight/src/lib.rs). Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Oct 31, 2026
…isation bug Two things, both consequences of rechecking rather than trusting the record. 1. The earlier two style commits reasoned from the wrong baseline. They measured the wasm formatter against untouched files on main and concluded it disagreed with this repository's rustfmt on 7 of 33 files, then rejected most of what it proposed. The measurement was right; the inference was backwards. Those 7 files are pre-existing violations, not evidence of intent -- main is not fmt-clean, and #144/#143 exist to repair exactly them. Re-measured against real rustfmt output (#144, produced by `rustup run stable cargo fmt`): the wasm formatter reproduces the eight reformatted files byte-for-byte, bar one hunk in squabble-core/src/chains.rs that is not a formatting change at all (`.map(RepoId::new)` -> closure). Validated as an oracle, it says three changes from b2b9f82 were wrong: two assert_eq! that rustfmt joins (56 and 54 columns of arguments, under fn_call_width=60) and one assert! it keeps inline. Reverted here. Audited with the oracle across every Rust file in the tree: * 0 files added or modified by this branch deviate from rustfmt output; * 7 workspace members inherited unformatted from main, all repaired by #144; * 3 more outside the workspace (two contracts/proven-tests fixtures and verification/squabble-creusot/src/model.rs, which root Cargo.toml excludes) that `cargo fmt --all` does not see. 2. A concurrent edit on this branch uppercased `status`/`conclusion` before `CheckRun::from_github`. That was not cosmetic: `from_github` matches UPPERCASE only, and GitHub's API reports lowercase, so a real failure was being read as `Pending` -- a red reported as "not concluded". Every existing test already asserted the corrected behaviour, so the fix made them capable of passing; none of them could run while fmt was red. Tests are added for the normalisation itself and for the new refusals (the `gh api .../jobs` wrapper, an object with no status/conclusion, non-string status, and a bare array), because a payload that is not a job must be a tool failure rather than a silent Pending. The Aspect 5 rewrite that arrived with those edits was checked for vacuity rather than assumed: it extracts 8 step names, and planting a directive name into the production source makes it fail (PASS=5 FAIL=2), so it still detects the leak it claims to. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
This was referenced Oct 31, 2026
Open
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
hyperpolymath
added a commit
that referenced
this pull request
Oct 31, 2026
…145) ## What this is `fight` learns one bit about a red check: that it *is* red. It then attributes the red from the **shape of the workflow** — a `CodeBuild` workflow becomes `EscalateToExpert{DispatchFix}`, a `Security` workflow becomes a scan, a reusable becomes an upstream owner. That attribution is only sound when the policy actually ran. A job that died in runner setup, provisioning or lock resolution is not a finding about the code, and handing it to a fixer invents work nobody can do. This PR makes that class nameable, and adds the harness needed to prove it builds. ## 1. `squabble-core::execution` — where a run stopped Five outcomes an estate report must keep distinct, plus a fail-closed non-answer: | class | meaning | |---|---| | `NotStarted` | no job exists (zero-job `startup_failure`) — attribution stays with `startup`/`zero-job`, not re-derived here | | `PreflightFailed{step}` | a declared setup/provisioning step (or GitHub's implicit `Set up job`) concluded failure: **provisioning, not a finding** | | `PrerequisiteMissing{step}` | a declared tool/credential/config step failed, so the policy step could not run | | `PolicyStepFailed{step}` | a policy step ran and failed — tells you *where*, never *why* | | `Verified{step}` | the job passed **and** a policy step recorded success | | `Unclassified{reason}` | one of five reasons; never a guess | - **Step vocabulary is host-supplied** (`execution_taxonomy.a2ml`), never compiled in; matching is **exact** after trimming, because this repository already paid once for abbreviation drift. - **Ordering**: preflight before prerequisite before policy, so nothing later can mask an earlier stop. - **No path to green**: constructs no `CheckRun`, proposes no `Move`, calls no `evaluate()`, drops no required context. `tests/aspect_tests.sh` gained **Aspect 5**, asserting those structural properties on the module's non-test, non-comment source — no toolchain needed. ## 2. `squabble phase <job.json>` — offline entry point Exit `0` (a classification was produced) or `2` (tool failure). There is deliberately **no exit code that means green**. Outside `squabble-contract/1.0.0`; the pinned surface is untouched. ## 3. `scripts/local-verify.sh` + planted controls Runs every gate CI runs into one `$TMPDIR` bundle, writes nothing into the repo. `tests/local_verify_test.sh` proves the harness's own vocabulary with planted failures in both directions (31 checks, no Rust, no network, no token). ## Evidence, as measured — updated after CI ran | claim | status | |---|---| | `Cargo check --workspace --all-targets --locked` | **PASSES on CI** (latest: `43af3b5`) — the new modules *and their test code* type-check | | `scripts/local-verify.sh`, `tests/local_verify_test.sh`, Aspect 5, contract conformance | **executed here**, 31/31 controls pass | | the 31 new unit tests | **type-checked, never executed** — `Cargo fmt` fails before the `Cargo test` job starts | | `cargo fmt --all -- --check` | **FAILS, inherited from `main`** — 0 files of this PR deviate; see below | | `clippy -D warnings` | never reached (downstream of fmt) | | anything about live GitHub | read through the API for the report; **not** exercised by this PR | ### The fmt failure is inherited, not introduced here — measured `cargo fmt` is red on this PR, but not because of this PR. `main` is not fmt-clean, and every branch cut from it inherits the failure — #116 fixed the same thing on 2026-09-30 and it came back within a month. Measured with rustfmt output reconstructed from #144 (which real `rustup run stable cargo fmt` produced; the reconstruction reproduces all eight of #144's files byte-for-byte except one hunk that is not a formatting change): | count | files | |---|---| | **0** | files added or modified **by this PR** that deviate from rustfmt output | | **7** | workspace members inherited unformatted from `main` — repaired by #144 (and identically by #143) | | **3** | further unformatted files outside the workspace that `cargo fmt --all` cannot see (two `contracts/proven-tests` fixtures, and `verification/squabble-creusot/src/model.rs`, which root `Cargo.toml` `exclude`s) | So the sequence is: **#144 or #143 merges first → this branch rebases → `fmt` passes here with no further edits → `clippy` and the 31 unit tests execute for the first time.** Tracked as #147 (which also records that #143 and #144 duplicate each other's reformatting and only one should land). An earlier version of this section claimed the formatter available here disagreed with the repository's, and left the residue for the owner to finish with one command. That reasoning was wrong: it calibrated against `main`, which is itself unformatted, so it read pre-existing violations as intent. Corrected in `43af3b5`, and the three layouts it had wrongly "adjudicated" are reverted. ## What this does not do No required context dropped, renamed or reconciled. No policy, allow-list, ruleset or pin touched. No issue or PR closed. No test weakened. ## Where it goes next `docs/reports/estate-integration/2026-10-31-ci-diagnosis-coverage.adoc` carries the role/ownership map, eight gaps (G-1…G-8), the owner-handoff table for inaccessible settings, and six proposals kept separate from what is implemented. Two matter for this class and are **labelled, not claimed**: **P-1** (route `PreflightFailed`/`PrerequisiteMissing` away from `EscalateToExpert{DispatchFix}`) and **P-2** (fetch step evidence for *red* checks). Both wait on a green compile-plus-test run. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
The scanner canary #143 fails Rust CI during
cargo fmt --all -- --check, with 29 pre-existing formatting diffs in seven files. This independent PR applies only mechanical formatting using stable rustfmt. Verifiedrustup run stable cargo fmt --all -- --checkandgit diff --check: both PASS. Signed commit 36418a5. Live compile, Clippy, tests and all other required checks remain necessary; no gates or proof obligations changed. Related: #143. The Creusot provisioning and Actions allowlist problems are separate.