Skip to content

feat(lint): gate CI on duplicate Rust logic and deduplicate the tree#1403

Merged
nachog00 merged 1 commit into
devfrom
DRY_lints
Jul 16, 2026
Merged

feat(lint): gate CI on duplicate Rust logic and deduplicate the tree#1403
nachog00 merged 1 commit into
devfrom
DRY_lints

Conversation

@zancas

@zancas zancas commented Jul 15, 2026

Copy link
Copy Markdown
Member

What this does

This PR adds a CI gate that fails when new duplicate Rust logic enters the tree, and deduplicates every existing finding first so the gate lands at zero-tolerance thresholds with an empty ignore list.

The gate

The workbench bin check-code-duplication runs cargo-dupes' AST-normalizing analyzer (mpecan/cargo-dupes, published as dupes-core/dupes-rust, pinned =0.2.1 in the workbench lockfile) over three scopes: packages/ (excluding the generated zaino-proto tonic output and in-crate tests/ module directories), live-tests/, and tools/. Identifiers normalize to positional placeholders and literals are erased, so a renamed copy of a function still matches. Any exact or near duplicate group of at least 50 AST nodes fails the build. The floor of 50 was calibrated empirically against this tree: it is the level at which every reported group was a genuine dedup-worthy twin, while lower settings flag structurally-rhyming but semantically unrelated small functions. The calibration rationale and the escape hatch (annotated entries in .dupes-ignore.toml, created via code-dupes ignore <fingerprint> --reason "...") are documented in the bin's module docs.

The gate runs locally as lint-code-duplication under makers lint, and in CI as a step in the existing repo-guards job, so it executes on every pull request on a GitHub-hosted runner. It was negative-tested: a planted duplicate with renamed identifiers fails with exit code 1 and a per-member report naming each twin's file and line range.

The deduplication

Every finding the detector reported was fixed with a plain function or generic type — no macros — per the repo DRY rule:

  • The six copy-pasted stream newtypes in zaino-state/src/stream.rs collapse into one generic ChannelStream<T>; the original six names survive as type aliases, so no call site changed.
  • FinalisedTxOutSetInfoAccumulator::apply_added_output/apply_removed_output share one body parameterized by a private OutputDelta enum; the XOR fold that combine also repeated is one method.
  • The median-time-past walk, duplicated between MockchainSource and the validator connector (and carrying a TODO prescribing exactly this fix), is one function generic over the per-block fetch closure.
  • The ~130-line get_block_range/get_block_range_nullifiers twins share spawn_block_range_stream, parameterized over the per-block mapping (identity vs compact_block_to_nullifiers).
  • RpcRequestError classification happens once in RpcRequestFallback::classify, adopted by the MempoolError, NonFinalisedStateError, and FinalisedStateError conversions. The gate itself surfaced the third copy after the first two were unified.
  • Deduplicating the Broadcast/BroadcastSubscriber Debug impls fixes a copy-paste bug: the subscriber printed itself as "Broadcast" and labeled its watch::Receiver a Sender.
  • BlockData::new and BlockMetadata::new were false twins — unrelated types whose field-copy constructors merely rhyme structurally — so both no-value positional constructors are deleted in favor of named struct literals at their call sites.
  • In live-tests, the five helper twins shared by e2e/tests/devtool.rs and devtool_zcashd.rs (plus their near-duplicate launch preambles) become generic bodies in e2e::devtool, with the semantically load-bearing difference — which pool a shield lands in — lifted into an explicit parameter; the clientless get_block_header oracle closures share clientless::assert_get_block_header_matches; and the orchard/ironwood coinbase predicates share is_valid_shielded_coinbase over a CoinbaseRewardPool enum.

Verification

cargo check --all-targets, cargo clippy --all-targets, and cargo fmt --all --check are green on the production set, as are the workbench crate's fmt/clippy/tests and cargo check --all-targets for the e2e and clientless crates. Two caveats are pre-existing and were verified by stash-testing the clean tree: the --all-features clippy failure in write_core.rs (transparent_address_history_experimental bit-rot), and the zcashd_support feature path not compiling (nothing forwards legacy-stack to zcash_local_net), which leaves the devtool_zcashd.rs changes consistent with the zebrad column but compiler-unverified.

🤖 Generated with Claude Code

Add a duplicate-logic gate: the workbench bin `check-code-duplication`
runs cargo-dupes' AST-normalizing analyzer (crates.io `dupes-core` /
`dupes-rust`, pinned =0.2.1) over `packages/` (excluding the generated
`zaino-proto` output and in-crate `tests/` modules), `live-tests/`, and
`tools/`, and fails on any exact or near duplicate group. Identifiers
normalize to positional placeholders, so renamed copies still match.
The gate runs locally via the `lint-code-duplication` task under
`makers lint` and in CI as a `repo-guards` step, so it executes on
every pull request. `min_nodes` 50 was calibrated empirically: it is
the floor at which every reported group in this tree was a genuine
twin; the calibration rationale and the annotated `.dupes-ignore.toml`
escape hatch are documented in the bin. The thresholds are zero because
this commit also deduplicates every existing finding.

In zaino-state, the six copy-pasted stream newtypes collapse into one
generic `ChannelStream<T>` with six type aliases; the accumulator's
added/removed-output methods share one body parameterized by a private
`OutputDelta` enum; the median-time-past walk (which carried a TODO
prescribing exactly this) is one function generic over the per-block
fetch; the ~130-line `get_block_range` / `get_block_range_nullifiers`
twins share `spawn_block_range_stream`, parameterized over the
per-block mapping; and `RpcRequestError` classification happens once in
`RpcRequestFallback::classify`, adopted by the `MempoolError`,
`NonFinalisedStateError`, and `FinalisedStateError` conversions — the
third of which the new gate itself surfaced. Deduplicating the
`Broadcast` / `BroadcastSubscriber` `Debug` impls also fixes a
copy-paste bug: the subscriber printed itself as "Broadcast" and
labeled its `watch::Receiver` a `Sender`. The `BlockData::new` and
`BlockMetadata::new` constructors were pure field copies flagged as
structural twins; both are deleted in favor of named struct literals at
their call sites.

In the live tests, the five helper twins shared by `devtool.rs` and
`devtool_zcashd.rs` (plus their near-duplicate launch preambles) become
generic bodies in `e2e::devtool`, with the expected shield pool
(Ironwood under NU6.3 heights, Orchard on the zcashd column) passed
explicitly; the clientless `get_block_header` oracle closures share
`clientless::assert_get_block_header_matches`; and the orchard/ironwood
coinbase predicates share `is_valid_shielded_coinbase` over a
`CoinbaseRewardPool` enum. The `zcashd_support` feature path does not
currently compile from this tree (nothing forwards `legacy-stack` to
zcash_local_net — a pre-existing condition of the zcashd deprecation,
see docs/adr/0005), so the `devtool_zcashd.rs` changes are consistent
with the zebrad column but compiler-unverified.

The gate was negative-tested: a planted duplicate with renamed
identifiers fails the bin with exit code 1 and a per-member report, and
removing it restores a passing run over 138 files with zero groups.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@zancas
zancas requested review from idky137 and nachog00 and removed request for idky137 July 15, 2026 22:01
@nachog00
nachog00 merged commit d98984d into dev Jul 16, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants