Repository navigation
fix(ci): align secret scanner permissions and enable verified canaries - #143
Merged
Merged
Conversation
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. |
Contributor
|
Caution Review failedThe pull request is closed. Summary
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description gives detailed context, change details, testing evidence, and verification requirements. However, it does not follow the repository template: the required Summary, Changes, RSR Quality… | Restructure the description to use the required template headings. Add the Summary, Changes, RSR Quality Checklist, and Testing sections. Mark each applicable checklist item and state whether the current PR-triggered scan completed with thr… |
| Linked Issues check |  |
The workflow retains the PR and push triggers, keeps the immutable reusable reference, adds workflow_dispatch, and reduces the caller permissions to contents: read. These changes address the Secre… |
Provide reviewable evidence from a current Secret Scanner run that starts successfully and contains the three scanner jobs. Confirm the current allow-list configuration if the run evidence depends on the reported live settings change. |
-
Check name Status Explanation Title check The title clearly describes the permission changes and the addition of workflow canaries. It is concise and related to the main changes. Out of Scope Changes check All reviewed changes are limited to .github/workflows/secret-scanner.yml. Adding manual dispatch supports the canary requested by [#141]. Correcting the permission guidance and removing unused permi…Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Out of Scope Changes check
-
Explanation
All reviewed changes are limited to
.github/workflows/secret-scanner.yml. Adding manual dispatch supports the canary requested by [#141]. Correcting the permission guidance and removing unused permissions support the same workflow repair. No unrelated scanners, checks, or triggers are removed.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. 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 0 functions across 0 files. (1 skipped: 1 unsupported.)
-
Full details: Description check
-
Explanation
The description gives detailed context, change details, testing evidence, and verification requirements. However, it does not follow the repository template: the required Summary, Changes, RSR Quality Checklist, and Testing headings are missing, and the checklist items are not completed.
Resolution
Restructure the description to use the required template headings. Add the Summary, Changes, RSR Quality Checklist, and Testing sections. Mark each applicable checklist item and state whether the current PR-triggered scan completed with three scanner jobs.
Full details: Linked Issues check
-
Explanation
The workflow retains the PR and push triggers, keeps the immutable reusable reference, adds
workflow_dispatch, and reduces the caller permissions tocontents: read. These changes address the Secret Scanner part of [#141]. However, the available evidence does not show a new run with three scanner jobs. The PR description states that this verification is still pending. The live Actions allow-list change is also outside the reviewed repository diff.Resolution
Provide reviewable evidence from a current Secret Scanner run that starts successfully and contains the three scanner jobs. Confirm the current allow-list configuration if the run evidence depends on the reported live settings change.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
|
✅ Coding Agent task started: View task and status The task will inspect the CI failures, validate its fix, and commit the fix to this branch automatically.
⏭️ Checks skipped: 4 · already failing on `main` (not caused by this PR)
|
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
|
hyperpolymath
added a commit
that referenced
this pull request
Oct 31, 2026
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. Verified `rustup run stable cargo fmt --all -- --check` and `git 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.
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.
Related: #141 (zero-job Secret Scanner startup failures).
The canonical selected-actions list has now been repaired live to include the narrowly scoped
metadatastician/standards/.github/workflows/*@*pattern, without disabling selected-actions or SHA pinning. Older startup-failure workflow runs cannot be retried, so this PR deliberately creates a new PR-triggered scan and addsworkflow_dispatchfor future verifiable, on-demand canaries.The wrapper previously carried stale claims that the reusable still required
actions:readandpull-requests:write, and itsscanjob unnecessarily grantedactions:readandsecurity-events:write. The pinned8e48b1creusable instead declarescontents:readper job, with no secrets inheritance, SARIF upload or workflow API access. This PR removes excess grants and corrects the misleading guidance.No scanners, checks, or push/PR triggers are removed. The workflow still invokes the same immutable reusable SHA, including Gitleaks, Rust secrets and shell secrets.
actionlint .github/workflows/secret-scanner.ymlandgit diff --checkpassed locally; commit2e5c0b7is signed. Verify the new PR run produces three actual scanner jobs rather than the earlierjobCount=0. Do not close #141 until a current run has completed successfully.