docs: record the local-vs-CI split so local runs stop duplicating CI - #899
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughCLAUDE.md replaces a single command reminder with guidance on local test scope, CI workflows and filters, uncovered CI scenarios, and confirming that filtered tests actually ran. ChangesLocal and CI testing guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
CI already runs the full `--all-features --workspace` suite, doc tests, clippy and the fmt check on every non-draft PR that touches Rust code, but nothing said so — leaving "run the tests" to mean a full local workspace run that CI then repeats verbatim minutes later. State what CI covers, and scope local runs to the narrowest thing that covers the change. Record the two places CI does not cover: draft PRs, whose pull_request runs are suppressed by the draft guard and need either ready-for-review or a manual workflow_dispatch run; and the backend E2E suite, which is commented out in tests.yml and #[ignore]d. Also note that a green cargo run proves nothing on its own: `cargo test` with a filter matching no test exits 0 and prints "test result: ok", so a run must be confirmed by finding the expected test names in its log. Describes CI as of PR #898 (workflow_dispatch on both workflows, rust-toolchain.toml in the path filters, push limited to v*-dev). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
c317a43 to
c4e82c7
Compare
|
✅ Final review complete — no blockers (commit 48e3c98) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 69-70: Update the CI guidance around the draft PR instructions to
match the actual workflow triggers: either add workflow_dispatch support to both
tests.yml and clippy.yml, or remove the Actions-tab and gh workflow run
instructions from CLAUDE.md. Keep the remaining draft, ready_for_review, and
branch-head behavior accurate.
- Around line 51-58: Update the CI path-filter description in CLAUDE.md to match
the workflows: add tests/backend-e2e/** for tests.yml, remove
rust-toolchain.toml, and state that each workflow watches its own workflow file
rather than implying identical filters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The new guidance accurately describes most of the intended CI behavior after PR #898, but its draft-PR workaround runs only the test workflow and therefore omits formatting and Clippy from the documented gate. The path-filter description also incorrectly claims that every documentation-only change skips CI, even though changes under tests/backend-e2e/** trigger tests.yml.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `CLAUDE.md`:
- [BLOCKING] CLAUDE.md:69: Trigger both workflows for draft PR coverage
The documented command dispatches only `tests.yml`; each `gh workflow run` invocation selects one workflow. A contributor following this workaround gets the workspace and documentation tests but no formatting or Clippy check, even though this section presents manual dispatch as the alternative for CI coverage on draft PRs. Document both commands so the workaround exercises the complete gate described above.
- [SUGGESTION] CLAUDE.md:58: Document the backend-E2E path-filter exception
PR #898's final `tests.yml` path list includes `tests/backend-e2e/**`, so non-Rust documentation such as `tests/backend-e2e/README.md` triggers the complete test workflow. The blanket statement that documentation-only changes run neither workflow is therefore false. Record this exception so the section accurately explains when CI runs.
…ch claim Reconcile CLAUDE.md's local-vs-CI section with the actual tests.yml/clippy.yml triggers: path filters are per-workflow (each watches its own workflow file), tests.yml also watches tests/backend-e2e/**, rust-toolchain.toml is not an actual trigger path, and neither workflow declares workflow_dispatch so the manual-dispatch guidance is removed rather than left half-working. Co-Authored-By: Claude <noreply@anthropic.com>
lklimek
left a comment
lklimek
left a comment
There was a problem hiding this comment.
Automated grumpy-review (qa-engineer-marvin standing in for the trio, TRIVIAL-scale path, + an independent Codex Sol pass) against e909e68b.
6 findings surfaced across both reviewers; 2 (the workflow_dispatch claim and the path-filter description — both blocking) are not posted here because they duplicate CodeRabbit's already-open threads on this PR and both are already marked "✅ Addressed in commit e909e68" — fixed before this review even landed. The 4 remaining non-blocking findings below are new.
Positives (verified, not assumed): command table matches the live workflow YAML exactly, draft-PR gating claim is accurate, the backend-E2E-not-in-CI claim is accurate, safe-cargo.sh allowlist checks out, no secrets/scope creep in a docs-only 1-file diff.
🤖 Co-authored by Claudius the Magnificent AI Agent
| Because CI always runs the full sweep, locally you should: | ||
|
|
||
| - Run only the **narrowest scope covering your change** — `cargo test <test_name> --all-features`, or `cargo test --test kittest --all-features` for a UI change. Running the whole workspace suite locally only duplicates the run CI is about to do anyway. | ||
| - Always run `cargo +nightly fmt --all` before committing. It needs no compile, and `clippy.yml` fails the build on unformatted code. |
There was a problem hiding this comment.
🟡 Medium: documented local formatter doesn't match CI's formatter toolchain
This mandates cargo +nightly fmt --all, while clippy.yml installs Rust 1.92 (per rust-toolchain.toml) and runs cargo fmt --all -- --check on that toolchain. Nightly and 1.92 rustfmt can disagree on formatting, so the locally-mandated command isn't actually the one CI validates — the same local/CI mismatch this section exists to close. (Predates this PR — it echoes the repo's existing top-level "Always run cargo +nightly fmt" instruction — but this PR is the place restating it, so worth fixing here.)
Suggest: cargo fmt --all (honoring rust-toolchain.toml) or explicit cargo +1.92 fmt --all, unless nightly-only rustfmt options are actually in use — if so, say why nightly is required.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Fixed in 48e3c980 — switched to plain cargo fmt --all (honors the rust-toolchain.toml 1.92 pin) in both this bullet and the earlier "finalizing your work" sentence, matching what clippy.yml actually checks. Left the pre-existing "CI: Safe Cargo Wrapper" section's +nightly fmt untouched — that's a separate, pre-existing concern outside this docs-only PR's stated scope, but flagging it here in case it's worth its own follow-up.
🤖 Co-authored by Claudius the Magnificent AI Agent
|
|
||
| - Run only the **narrowest scope covering your change** — `cargo test <test_name> --all-features`, or `cargo test --test kittest --all-features` for a UI change. Running the whole workspace suite locally only duplicates the run CI is about to do anyway. | ||
| - Always run `cargo +nightly fmt --all` before committing. It needs no compile, and `clippy.yml` fails the build on unformatted code. | ||
| - Run `cargo clippy` locally only for the scope you touched (`-p <crate>`), or when you expect lint fallout. CI owns the `--all-features --all-targets` sweep. |
There was a problem hiding this comment.
🟢 Low: -p <crate> clippy-scoping advice doesn't apply to this repo
This repo has no [workspace] — root Cargo.toml is a single [package] name = "dash-evo-tool". -p <crate> has exactly one possible value here (the whole crate), so it provides zero scoping benefit over omitting -p entirely. Reads like carried-over multi-crate-workspace boilerplate.
Suggest: replace with something that actually narrows a clippy run in a single-package repo (e.g. --bin <name>), or drop the parenthetical if no real narrowing exists.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Fixed in 48e3c980 — dropped the -p <crate> parenthetical (confirmed: no [workspace], single package) and replaced with target-based scoping guidance (--bin dash-evo-tool).
🤖 Co-authored by Claudius the Magnificent AI Agent
|
|
||
| Because CI always runs the full sweep, locally you should: | ||
|
|
||
| - Run only the **narrowest scope covering your change** — `cargo test <test_name> --all-features`, or `cargo test --test kittest --all-features` for a UI change. Running the whole workspace suite locally only duplicates the run CI is about to do anyway. |
There was a problem hiding this comment.
🟢 Low: new "run bare cargo locally" guidance doesn't disambiguate from the safe-cargo.sh mandate
CLAUDE.md's existing "CI: Safe Cargo Wrapper" section mandates scripts/safe-cargo.sh "In GitHub Actions (Claude Code workflow)" instead of bare cargo. This new section's "locally"-framed bare-cargo instructions could plausibly be misread by an AI agent running inside the Claude Code review GitHub Action (itself a form of CI, but neither tests.yml nor clippy.yml) as applying to its own environment. Soft ambiguity, not a confirmed misread — but cheap to close.
Suggest: one clause cross-referencing the safe-cargo.sh rule so the two sections can't be read as conflicting for an agent running inside that workflow.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Fixed in 48e3c980 — added a cross-reference clause pointing an agent running inside the Claude Code review GitHub Action to the "CI: Safe Cargo Wrapper" section instead.
🤖 Co-authored by Claudius the Magnificent AI Agent
|
|
||
| CI is the full-suite backstop. Do not reproduce it locally. | ||
|
|
||
| On every **non-draft** PR that touches Rust code, and on pushes to `v*-dev`, GitHub Actions runs the complete gate: |
There was a problem hiding this comment.
🟢 Low: "complete gate"/"full sweep" wording overlooks intentionally-#[ignore]d unit tests
The default cargo test --all-features --workspace skips every #[ignore] test. Beyond the acknowledged backend-E2E lane, current HEAD also has at least one intentionally-ignored unit test (src/model/secret.rs:492, test_drop_zeroes_full_capacity) that stays manual. "Complete gate" / "full sweep," plus the advice to check for a test's name in the log, reads as too absolute — a test name can appear in the log with ignored status without having actually run.
Suggest: qualify to "the full non-ignored suite," note intentionally-ignored tests remain manual, and recommend checking per-test pass/ignore status (or executed-test count) rather than only searching the log for a name.
🤖 Co-authored by Claudius the Magnificent AI Agent — automated grumpy-review (Claude trio-equivalent + Codex Sol)
There was a problem hiding this comment.
Fixed in 48e3c980 — qualified to "full non-ignored-test gate," and the log-check guidance now calls out a concrete example (test_drop_zeroes_full_capacity in src/model/secret.rs) and says to confirm pass status, not just name presence.
🤖 Co-authored by Claudius the Magnificent AI Agent
Addresses live review findings on this PR: - Local fmt guidance said `cargo +nightly fmt`, but clippy.yml checks formatting on the rust-toolchain.toml-pinned 1.92, not nightly — switched to plain `cargo fmt --all` in both places it appeared. - `-p <crate>` clippy-scoping advice doesn't apply: this repo has no [workspace], so there is exactly one crate. Replaced with target-based scoping guidance. - Cross-referenced the "CI: Safe Cargo Wrapper" section so an agent running inside the Claude Code review GitHub Action isn't misled by the "locally" framing. - Softened "complete gate"/"full sweep" wording (a #[ignore]d unit test stays manual even on green CI) and pointed at a concrete example plus a pass-status check, not just log presence. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
At exact head 48e3c98, the full PR diff touches only CLAUDE.md's testing/CI-workflow guidance. Both prior Codex findings and all four review-thread comments are fixed. One PR-relevant inconsistency remains: the newly added line-49 cross-reference routes GitHub-Action agents to the Safe Cargo Wrapper section, whose formatter example still selects nightly instead of the Rust 1.92 toolchain clippy.yml enforces.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— general (failed),claude-sonnet-5— general (completed)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `CLAUDE.md`:
- [SUGGESTION] CLAUDE.md:89: Safe Cargo Wrapper fmt example still selects nightly, contradicting the pinned-toolchain guidance this PR just established
`scripts/safe-cargo.sh` forwards its arguments verbatim to Cargo, so `scripts/safe-cargo.sh +nightly fmt --all` runs nightly rather than the `rust-toolchain.toml`-pinned 1.92 toolchain that `clippy.yml` installs and checks via `cargo fmt --all -- --check`. This line predates the PR, but the newly added CLAUDE.md:49 sentence makes that section the explicit guidance for agents running inside the Claude Code review GitHub Action. Because the head commit's stated purpose is to fix the nightly-versus-pinned formatter mismatch and fixes the identical mismatch at CLAUDE.md:45/63, leaving this routed example unchanged undercuts the PR's own documentation goal. Update CLAUDE.md:89 and the matching usage comment in scripts/safe-cargo.sh to use `scripts/safe-cargo.sh fmt --all`.
…#903, dashpay#904) Pulled in 4 new upstream commits since the last sync (3b32bed): - dashpay#903 feat(ui): redesign welcome-screen experience-level selector as cards, with radio-button accessibility semantics. Only real conflict in this merge: tests/kittest/welcome_screen.rs, where both sides added tests — kept our create_wallet_path_shows_and_clears_onboarding_stepper test and pulled in upstream's 4 new card-accessibility tests (rebranded dash_evo_tool:: -> orchardpay:: in the process). - dashpay#904 fix(wallet): surface DAPI endpoint exhaustion — pins the exact Sdk generation a backend task ran against for error classification, fixing errors misattributed to a since-reloaded SDK. Touches backend_task/mod.rs, which our own pub mod orchardpay; addition also lives in; merged cleanly with no conflict. - dashpay#898 ci: allow manual test/clippy runs, cover rust-toolchain.toml. - dashpay#899 docs: record the local-vs-CI split in CLAUDE.md. Also corrects local fmt guidance from `cargo +nightly fmt` to plain `cargo fmt --all` (CI checks against the rust-toolchain.toml-pinned compiler, not nightly) — worth adopting going forward. One rebrand miss caught in the merged prose (`--bin dash-evo-tool` -> `--bin orchardpay`). All other files (CLAUDE.md, src/app.rs, src/backend_task/error.rs, src/model/user_role.rs, src/ui/dpns/dpns_contested_names_screen.rs, src/ui/welcome_screen.rs) auto-merged cleanly — none needed manual reconciliation beyond the one branding fix above. Verified: cargo build (both --all-features and default features), clippy -D warnings, cargo fmt --all -- --check, and the full test suite all green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Why this PR exists
Problem: CI already runs the full
--all-features --workspacesuite, doc tests, clippy and the fmt check on every non-draft PR that touches Rust code — butCLAUDE.mdnever said so. "Run the tests" therefore reads as a full local workspace run, which CI then repeats verbatim minutes later. Nothing recorded where CI doesn't cover you either.What breaks without it:
cargo test --all-features --workspacelocally — a long cold-ish build — then pushes, and CI runs the identical command again. The local run buys nothing the PR check wasn't about to prove.github.event.pull_request.draft != true. PRs here are opened as drafts by convention, so pushing to a draft and waiting on checks waits forever — no test, clippy, or fmt run is ever queued, and nothing says so.cargo test <filter>exits 0 and printstest result: okwhen the filter matches nothing, so a filtered run can be reported as passing without executing a single test.Blocking relationship: Targets feat: rewrite Dash Evo Tool onto the new platform-wallet #860's branch. The CI changes this text describes are in ci: allow manual test/clippy runs, cover rust-toolchain.toml, drop dead push branch #898 (against
v1.0-dev); this documents the resulting behaviour and is otherwise independent.What was done
Adds
CLAUDE.md§ Testing → "Local vs CI — avoid duplicate test runs":tests.yml,clippy.yml) and when — path-filtered to**/*.rs(which includesbuild.rs),**/Cargo.toml,Cargo.lock,.cargo/config.toml,rust-toolchain.toml, and the workflow file; a documentation-only change deliberately runs neither.cargo +nightly fmt --allalways (no compile, andclippy.ymlfails on unformatted code); local clippy only for the touched scope.ready_for_review, or a manualworkflow_dispatchrun — noting it tests the branch head, not the merge commit) and backend E2E (commented out intests.yml,#[ignore]d).Testing
Documentation only — no code changes, so no suite applies.
Merge-conflict checks, both clean:
v1.0-dev, then #860 + this merged inThis was split out of #898 precisely because it collides there: #860 rewrites the same region of
CLAUDE.md(it inserts a### GUI testingsection immediately above the anchor line this change edits), so carrying the text on av1.0-devbranch produced aCLAUDE.mdconflict when #860 merged. Basing it on #860 instead removes the collision.Text describes CI as of #898 (
workflow_dispatchon both workflows,rust-toolchain.tomlfiltered, push limited tov*-dev), so it should land after #898.Breaking changes
None — documentation only.
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit