sdk%lint: introduce CodeQL and semgrep linters, add shim, derivation, disambiguation rules, resolve violations, add common helper for lint infra#5
Merged
Conversation
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. 📝 WalkthroughWalkthroughThis PR establishes comprehensive static analysis infrastructure by reorganizing CI workflows, consolidating Python lint tooling with shared helpers, deploying custom CodeQL libraries and queries for Rust analysis, integrating Semgrep pattern matching, and enforcing consistent coding standards across the codebase. ChangesStatic Analysis Infrastructure
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Warning This pull request may have conflicts, please coordinate with the authors of these pull requests. Potential conflicts |
5 tasks
kwvg
changed the title
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (3)
.github/workflows/build_msrv.yml (1)
21-24: 💤 Low valueOptional security hardening: consider setting
persist-credentials: false.For defense-in-depth, consider explicitly setting
persist-credentials: falsein checkout steps to prevent theGITHUB_TOKENfrom persisting in the workspace and potentially leaking through artifacts or logs.🔒 Set persist-credentials to false
- name: Checkout uses: actions/checkout@v6 with: fetch-depth: 0 + persist-credentials: falseAnd for the build job:
- name: Checkout uses: actions/checkout@v6 + with: + persist-credentials: falseAlso applies to: 92-93
🤖 Prompt for 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. In @.github/workflows/build_msrv.yml around lines 21 - 24, The checkout steps using actions/checkout@v6 (the step with name "Checkout" and any other actions/checkout@v6 occurrences) should explicitly set persist-credentials: false under the with: block to avoid persisting GITHUB_TOKEN in the workspace; update the with: block on the existing checkout step (currently containing fetch-depth: 0) and any other checkout steps in the file to include persist-credentials: false while preserving existing keys like fetch-depth.contrib/lint/lint_codeql.py (1)
65-74: 💤 Low valueSimple substring matching may produce false positives.
The checks
if "Serialize" in line:andif "Deserialize" in line:will match these strings anywhere in the line, including:
- Comments:
// Implements Serialize for Foo- String literals:
"Serialize"- Compound identifiers:
DeserializeOwned,CustomSerializeThis could inflate
hasSerdeMentionresults. If precision is important, consider regex matching for derive attributes or trait bounds.🎯 More precise matching with regex
+import re + def _generate_source_lines( source_root: Path, query_dir: Path, ) -> Path: ... + serde_pattern = re.compile(r'\b(Serialize|Deserialize)\b') for rs_file in sorted(source_root.rglob("*.rs")): ... for lineno, line in enumerate(text.splitlines(), 1): stripped = line.lstrip() - if "Serialize" in line: + match = serde_pattern.search(line) + if match and "Serialize" in match.group(0): serde_rows.append( f' file = "{rel}" and line = {lineno}' f' and trait = "Serialize"', ) - if "Deserialize" in line: + if match and "Deserialize" in match.group(0): serde_rows.append( f' file = "{rel}" and line = {lineno}' f' and trait = "Deserialize"', )🤖 Prompt for 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. In `@contrib/lint/lint_codeql.py` around lines 65 - 74, The substring checks using if "Serialize" in line / if "Deserialize" in line produce false positives; update the logic that populates serde_rows (and any hasSerdeMention usage) to use regex whole-word/attribute matching instead: match derive attributes (e.g., a regex that finds #[...derive(...Serialize...)]/#[...derive(...Deserialize...)]), impl/trait occurrences as whole words (e.g., \bSerialize\b and \bDeserialize\b) and avoid matching inside string literals/comments by focusing on attribute/impl/trait patterns; apply these regexes where serde_rows is appended to ensure only real derives/trait uses are captured.contrib/lint/common.py (1)
39-46: 💤 Low valueConsider more robust workspace detection.
The substring check
"[workspace]" in cargo.read_text(...)could theoretically match[workspace]in a comment or string literal. While unlikely in practice forCargo.tomlfiles, a more robust check would parse the TOML or use a regex to match the section header at line start.🔍 More robust detection using regex
def is_workspace_root(d: Path) -> bool: """Return True if *d* looks like a Cargo workspace root.""" + import re cargo = d / "Cargo.toml" - return ( - cargo.is_file() - and "[workspace]" in cargo.read_text(encoding="utf-8") - and (d / "pkgs").is_dir() - ) + if not cargo.is_file() or not (d / "pkgs").is_dir(): + return False + text = cargo.read_text(encoding="utf-8") + return bool(re.search(r'^\s*\[workspace\]', text, re.MULTILINE))🤖 Prompt for 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. In `@contrib/lint/common.py` around lines 39 - 46, The current is_workspace_root function uses a substring check which can match inside comments or strings; update it to robustly detect a TOML section header by reading the Cargo.toml and using a multiline regex to match a line that starts with an optional whitespace followed by exactly "[workspace]" (e.g. re.search(r'^\s*\[workspace\]\s*$', content, re.MULTILINE)), keeping the existing checks for cargo.is_file() and (d / "pkgs").is_dir(); ensure you still read the file with encoding="utf-8" and return the boolean result of the regex search.
🤖 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 @.github/workflows/build_msrv.yml:
- Around line 51-58: The workflow restores the cargo registry with the "Restore
cargo registry" step (uses: actions/cache/restore@v5) in both the lint and build
jobs but never saves it back, so add a corresponding cache save step (uses:
actions/cache/save@v5) in each job after the job's dependency installation/build
steps; use the same path block (~/.cargo/registry and ~/.cargo/git) and the same
key (cargo-deps-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('Cargo.lock')
}}) and restore-keys to persist updates to the cache for future runs.
In `@CLAUDE.md`:
- Line 12: The CLAUDE.md "eager traits" guidance currently omits Default but the
codebase eagerly derives Default on several types (e.g. types::uint,
primitives::validation, primitives::script, primitives::block_header,
script::key_id, p2p_core::service_flags, num::compact, num::arith256); fix by
choosing one of two actions and applying it consistently: either (A) update
CLAUDE.md to explicitly include Default as an eagerly-derived trait and list any
intentional exceptions, or (B) remove the unnecessary #[derive(Default)] from
the noted types (or their structs/enums) so the docs and code match; ensure the
change references the specific types above and update the CLAUDE.md wording to
reflect the decided policy and any exceptions.
---
Nitpick comments:
In @.github/workflows/build_msrv.yml:
- Around line 21-24: The checkout steps using actions/checkout@v6 (the step with
name "Checkout" and any other actions/checkout@v6 occurrences) should explicitly
set persist-credentials: false under the with: block to avoid persisting
GITHUB_TOKEN in the workspace; update the with: block on the existing checkout
step (currently containing fetch-depth: 0) and any other checkout steps in the
file to include persist-credentials: false while preserving existing keys like
fetch-depth.
In `@contrib/lint/common.py`:
- Around line 39-46: The current is_workspace_root function uses a substring
check which can match inside comments or strings; update it to robustly detect a
TOML section header by reading the Cargo.toml and using a multiline regex to
match a line that starts with an optional whitespace followed by exactly
"[workspace]" (e.g. re.search(r'^\s*\[workspace\]\s*$', content, re.MULTILINE)),
keeping the existing checks for cargo.is_file() and (d / "pkgs").is_dir();
ensure you still read the file with encoding="utf-8" and return the boolean
result of the regex search.
In `@contrib/lint/lint_codeql.py`:
- Around line 65-74: The substring checks using if "Serialize" in line / if
"Deserialize" in line produce false positives; update the logic that populates
serde_rows (and any hasSerdeMention usage) to use regex whole-word/attribute
matching instead: match derive attributes (e.g., a regex that finds
#[...derive(...Serialize...)]/#[...derive(...Deserialize...)]), impl/trait
occurrences as whole words (e.g., \bSerialize\b and \bDeserialize\b) and avoid
matching inside string literals/comments by focusing on attribute/impl/trait
patterns; apply these regexes where serde_rows is appended to ensure only real
derives/trait uses are captured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 36e7e2fb-8674-4113-a28c-8c4937614094
⛔ Files ignored due to path filters (133)
pkgs/num/src/compact.rsis excluded by!**/*.rspkgs/num/src/hash.rsis excluded by!**/*.rspkgs/num/src/hash_encoding.rsis excluded by!**/*.rspkgs/num/src/lib.rsis excluded by!**/*.rspkgs/num/src/prelude.rsis excluded by!**/*.rspkgs/num/src/serialize.rsis excluded by!**/*.rspkgs/num/src/util.rsis excluded by!**/*.rspkgs/num/tests/arith.rsis excluded by!**/*.rspkgs/num/tests/hash.rsis excluded by!**/*.rspkgs/p2p_core/src/encode.rsis excluded by!**/*.rspkgs/p2p_core/src/error.rsis excluded by!**/*.rspkgs/p2p_core/src/lib.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/addr.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/cfcheckpt.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/cfheaders.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/cfilter.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/govobj.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/govobjvote.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/govsync.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/headers.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/headers2.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/inv.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/mnlistdiff.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/mod.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/ping.rsis excluded by!**/*.rspkgs/p2p_core/src/msg/version.rsis excluded by!**/*.rspkgs/p2p_core/src/prelude.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/command.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/compressed_header.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/governance.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/inventory.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/magic.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/mn_list.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/mod.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/net_addr.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/service_flags.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/short_id.rsis excluded by!**/*.rspkgs/p2p_core/src/primitives/user_agent.rsis excluded by!**/*.rspkgs/p2p_core/src/v2/mod.rsis excluded by!**/*.rspkgs/p2p_core/tests/addrv2.rsis excluded by!**/*.rspkgs/p2p_core/tests/mnlistdiff.rsis excluded by!**/*.rspkgs/params/src/lib.rsis excluded by!**/*.rspkgs/params/src/prelude.rsis excluded by!**/*.rspkgs/params/src/types.rsis excluded by!**/*.rspkgs/pkc/bench/main.rsis excluded by!**/*.rspkgs/pkc/src/bls_chia/agg.rsis excluded by!**/*.rspkgs/pkc/src/bls_chia/mod.rsis excluded by!**/*.rspkgs/pkc/src/bls_chia/pk.rsis excluded by!**/*.rspkgs/pkc/src/bls_chia/ser.rsis excluded by!**/*.rspkgs/pkc/src/bls_chia/sig.rsis excluded by!**/*.rspkgs/pkc/src/bls_chia/sk.rsis excluded by!**/*.rspkgs/pkc/src/bls_chia/threshold.rsis excluded by!**/*.rspkgs/pkc/src/bls_ietf/agg.rsis excluded by!**/*.rspkgs/pkc/src/bls_ietf/mod.rsis excluded by!**/*.rspkgs/pkc/src/bls_ietf/pk.rsis excluded by!**/*.rspkgs/pkc/src/bls_ietf/sig.rsis excluded by!**/*.rspkgs/pkc/src/bls_ietf/sk.rsis excluded by!**/*.rspkgs/pkc/src/bls_ietf/threshold.rsis excluded by!**/*.rspkgs/pkc/src/common/bls/mod.rsis excluded by!**/*.rspkgs/pkc/src/common/bls/threshold.rsis excluded by!**/*.rspkgs/pkc/src/k256/pk.rsis excluded by!**/*.rspkgs/pkc/src/k256/sig.rsis excluded by!**/*.rspkgs/pkc/src/k256/sk.rsis excluded by!**/*.rspkgs/pkc/src/lib.rsis excluded by!**/*.rspkgs/pkc/src/worker.rsis excluded by!**/*.rspkgs/pkc/tests/bls_chia_llmq.rsis excluded by!**/*.rspkgs/pkc/tests/bls_ietf_llmq.rsis excluded by!**/*.rspkgs/pow/src/bmw/simd.rsis excluded by!**/*.rspkgs/pow/src/cubehash/simd.rsis excluded by!**/*.rspkgs/pow/src/echo/simd.rsis excluded by!**/*.rspkgs/pow/src/groestl/mod.rsis excluded by!**/*.rspkgs/pow/src/groestl/simd.rsis excluded by!**/*.rspkgs/pow/src/jh/mod.rsis excluded by!**/*.rspkgs/pow/src/jh/simd.rsis excluded by!**/*.rspkgs/pow/src/keccak/mod.rsis excluded by!**/*.rspkgs/pow/src/lib.rsis excluded by!**/*.rspkgs/pow/src/luffa/consts.rsis excluded by!**/*.rspkgs/pow/src/luffa/mod.rsis excluded by!**/*.rspkgs/pow/src/luffa/simd.rsis excluded by!**/*.rspkgs/pow/src/prelude.rsis excluded by!**/*.rspkgs/pow/src/shavite/simd.rsis excluded by!**/*.rspkgs/pow/src/simd_hash/simd.rsis excluded by!**/*.rspkgs/pow/src/skein/simd.rsis excluded by!**/*.rspkgs/pow/src/util/aes/mod.rsis excluded by!**/*.rspkgs/pow/src/worker.rsis excluded by!**/*.rspkgs/pow/tests/blake.rsis excluded by!**/*.rspkgs/pow/tests/bmw.rsis excluded by!**/*.rspkgs/pow/tests/common/mod.rsis excluded by!**/*.rspkgs/pow/tests/cubehash.rsis excluded by!**/*.rspkgs/pow/tests/echo.rsis excluded by!**/*.rspkgs/pow/tests/groestl.rsis excluded by!**/*.rspkgs/pow/tests/jh.rsis excluded by!**/*.rspkgs/pow/tests/keccak.rsis excluded by!**/*.rspkgs/pow/tests/luffa.rsis excluded by!**/*.rspkgs/pow/tests/shavite.rsis excluded by!**/*.rspkgs/pow/tests/simd_hash.rsis excluded by!**/*.rspkgs/pow/tests/skein.rsis excluded by!**/*.rspkgs/primitives/src/block.rsis excluded by!**/*.rspkgs/primitives/src/block_header.rsis excluded by!**/*.rspkgs/primitives/src/codec.rsis excluded by!**/*.rspkgs/primitives/src/error.rsis excluded by!**/*.rspkgs/primitives/src/gov.rsis excluded by!**/*.rspkgs/primitives/src/lib.rsis excluded by!**/*.rspkgs/primitives/src/outpoint.rsis excluded by!**/*.rspkgs/primitives/src/payload/assetlock.rsis excluded by!**/*.rspkgs/primitives/src/payload/assetunlock.rsis excluded by!**/*.rspkgs/primitives/src/payload/cbtx.rsis excluded by!**/*.rspkgs/primitives/src/payload/mnhftx.rsis excluded by!**/*.rspkgs/primitives/src/payload/mod.rsis excluded by!**/*.rspkgs/primitives/src/payload/proregtx.rsis excluded by!**/*.rspkgs/primitives/src/payload/proupregtx.rsis excluded by!**/*.rspkgs/primitives/src/payload/prouprevtx.rsis excluded by!**/*.rspkgs/primitives/src/payload/proupservtx.rsis excluded by!**/*.rspkgs/primitives/src/payload/quorum.rsis excluded by!**/*.rspkgs/primitives/src/prelude.rsis excluded by!**/*.rspkgs/primitives/src/script.rsis excluded by!**/*.rspkgs/primitives/src/serialize.rsis excluded by!**/*.rspkgs/primitives/src/support.rsis excluded by!**/*.rspkgs/primitives/src/transaction.rsis excluded by!**/*.rspkgs/primitives/src/tx_in.rsis excluded by!**/*.rspkgs/primitives/src/tx_out.rsis excluded by!**/*.rspkgs/primitives/src/tx_types.rsis excluded by!**/*.rspkgs/primitives/src/validation.rsis excluded by!**/*.rspkgs/primitives/src/wire.rsis excluded by!**/*.rspkgs/primitives/tests/util/mod.rsis excluded by!**/*.rspkgs/script/src/key_id.rsis excluded by!**/*.rspkgs/script/src/lib.rsis excluded by!**/*.rspkgs/script/src/opcode.rsis excluded by!**/*.rspkgs/script/src/prelude.rsis excluded by!**/*.rspkgs/types/src/hex.rsis excluded by!**/*.rspkgs/types/src/lib.rsis excluded by!**/*.rspkgs/types/src/prelude.rsis excluded by!**/*.rspkgs/types/src/uint.rsis excluded by!**/*.rs
📒 Files selected for processing (29)
.github/workflows/build_msrv.yml.github/workflows/build_stable.yml.github/workflows/lint_all.yml.github/workflows/pr_comment.yml.github/workflows/pr_tag.ymlCLAUDE.mdcontrib/codeql/.gitignorecontrib/codeql/attrib.qlcontrib/codeql/codeql-pack.lock.ymlcontrib/codeql/import.qlcontrib/codeql/lib/files.qllcontrib/codeql/lib/filters.qllcontrib/codeql/lib/fmt.qllcontrib/codeql/lib/imports.qllcontrib/codeql/lib/policy.qllcontrib/codeql/lib/traits.qllcontrib/codeql/qlpack.ymlcontrib/codeql/secret.qlcontrib/lint/all_lint.pycontrib/lint/common.pycontrib/lint/lint_codeql.pycontrib/lint/lint_javascript.pycontrib/lint/lint_python.pycontrib/lint/lint_semgrep.pycontrib/semgrep/prelude.ymlcontrib/semgrep/serde.ymldocs/guide_rust.mdpkgs/primitives/Cargo.tomlpyproject.toml
💤 Files with no reviewable changes (1)
- .github/workflows/lint_all.yml
Comment on lines
+51
to
+58
| - name: Restore cargo registry | ||
| uses: actions/cache/restore@v5 | ||
| with: | ||
| path: | | ||
| ~/.cargo/registry | ||
| ~/.cargo/git | ||
| key: cargo-deps-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('Cargo.lock') }} | ||
| restore-keys: cargo-deps-${{ runner.os }}-${{ runner.arch }}- |
There was a problem hiding this comment.
Cargo registry cache is restored but never saved.
Both the lint and build jobs restore the cargo registry cache (lines 51-58 and 98-105), but neither job saves it. This means:
- The workflow depends on other workflows (like
build_stable.yml) to populate the cache - If this workflow runs in isolation (e.g., a develop branch push), the cache won't be updated when
Cargo.lockchanges - Cache misses and stale dependencies may degrade performance over time
💾 Add cache save steps
Add a save step to the lint job after line 70:
- name: Run linters
run: python3 contrib/lint/all_lint.py
+ - name: Save cargo registry
+ if: always()
+ uses: actions/cache/save@v5
+ with:
+ path: |
+ ~/.cargo/registry
+ ~/.cargo/git
+ key: cargo-deps-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('Cargo.lock') }}
+
- name: Save CodeQL cachesAnd add a save step to the build job after line 111:
- name: Test workspace
run: cargo test --workspace --features full,_internal
+
+ - name: Save cargo registry
+ if: always()
+ uses: actions/cache/save@v5
+ with:
+ path: |
+ ~/.cargo/registry
+ ~/.cargo/git
+ key: cargo-deps-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('Cargo.lock') }}Also applies to: 98-105
🧰 Tools
🪛 zizmor (1.25.2)
[error] 52-52: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for 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.
In @.github/workflows/build_msrv.yml around lines 51 - 58, The workflow restores
the cargo registry with the "Restore cargo registry" step (uses:
actions/cache/restore@v5) in both the lint and build jobs but never saves it
back, so add a corresponding cache save step (uses: actions/cache/save@v5) in
each job after the job's dependency installation/build steps; use the same path
block (~/.cargo/registry and ~/.cargo/git) and the same key (cargo-deps-${{
runner.os }}-${{ runner.arch }}-${{ hashFiles('Cargo.lock') }}) and restore-keys
to persist updates to the cache for future runs.
Collaborator
Author
There was a problem hiding this comment.
The registry is source-based and improvements have been force pushed, we did away with the unnecessary keying.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat .github/workflows/build_msrv.ymlLength of output: 3122
@kwvg, understood — I can see the simplified key (cargo-deps-${{ hashFiles('Cargo.lock') }} without OS/arch segments) in the updated file. The design makes sense: the cargo cache is populated by another workflow and consumed here, so a separate save step isn't needed. Thanks for the clarification.
✏️ Learnings added
Learnt from: kwvg
Repo: dashpay/base-sdk PR: 5
File: .github/workflows/build_msrv.yml:0-0
Timestamp: 2026-06-06T06:09:05.124Z
Learning: In the `dashpay/base-sdk` repository, the cargo registry cache in `.github/workflows/build_msrv.yml` is intentionally restore-only (`actions/cache/restorev5`). The cache is populated by another workflow (e.g., `build_stable.yml`) and only consumed in this workflow. The cache key deliberately omits `runner.os` and `runner.arch` segments as they were deemed unnecessary for this setup.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| `SCREAMING_SNAKE_CASE` for constants. Acronyms as words (`TxId` not `TXID`). Getters omit `get_` prefix. | ||
| - **Type safety**: newtypes over primitives when semantics differ, enums over booleans, make invalid states | ||
| unrepresentable. Derive `Clone`, `Debug`, `PartialEq`, `Eq`, `Hash`, `Default` eagerly. | ||
| unrepresentable. Derive `Clone`, `Debug`, `PartialEq`, `Eq`, `Hash` eagerly. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all derive attributes that include Default
rg -nP --type=rust 'derive\([^)]*\bDefault\b[^)]*\)' pkgs/Repository: dashpay/base-sdk
Length of output: 886
Fix mismatch between CLAUDE.md “eager traits” guidance and the repo’s Default derives
The current guideline snippet shows eagerly deriving Clone, Debug, PartialEq, Eq, Hash (no Default), but there are many existing Default derives across pkgs/:
pkgs/types/src/uint.rs:34pkgs/primitives/src/validation.rs:59pkgs/primitives/src/script.rs:19pkgs/primitives/src/block_header.rs:17pkgs/script/src/key_id.rs:14pkgs/p2p_core/src/primitives/service_flags.rs:15pkgs/num/src/compact.rs:16,20pkgs/num/src/arith256.rs:22
Align the intended policy: either remove Default derives where appropriate in this PR/follow-up, or update CLAUDE.md to reflect whether Default is meant to be eagerly derived (and list any explicit exceptions).
🤖 Prompt for 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.
In `@CLAUDE.md` at line 12, The CLAUDE.md "eager traits" guidance currently omits
Default but the codebase eagerly derives Default on several types (e.g.
types::uint, primitives::validation, primitives::script,
primitives::block_header, script::key_id, p2p_core::service_flags, num::compact,
num::arith256); fix by choosing one of two actions and applying it consistently:
either (A) update CLAUDE.md to explicitly include Default as an eagerly-derived
trait and list any intentional exceptions, or (B) remove the unnecessary
#[derive(Default)] from the noted types (or their structs/enums) so the docs and
code match; ensure the change references the specific types above and update the
CLAUDE.md wording to reflect the decided policy and any exceptions.
Collaborator
Author
There was a problem hiding this comment.
We don't prohibit deriving Default, we just don't enforce it in linting.
There was a problem hiding this comment.
@kwvg, understood — Default derivation is permitted but not mandated by the linter, so the CLAUDE.md guidance correctly reflects only what is enforced. I've noted this for future reviews.
✏️ Learnings added
Learnt from: kwvg
Repo: dashpay/base-sdk PR: 5
File: CLAUDE.md:12-12
Timestamp: 2026-06-06T06:09:25.351Z
Learning: In `dashpay/base-sdk`, deriving `Default` on types is allowed but not enforced by linting. The "eager derives" list in `CLAUDE.md` (Clone, Debug, PartialEq, Eq, Hash) reflects only what the linter *requires*, not what is *prohibited*. Types may still derive `Default` at the developer's discretion without violating project policy.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
kwvg
changed the title
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
1 participant
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.
Motivation
The next few pull requests will be streamlining the codec infrastructure and common traits expected from consensus-defining types. As part of preparing the codebase for these changes, we needed a way to implement the Development Guide programatically to ensure that codebase consistency, maintainability and a saner public API.
This is achieved through semantic analysis (using CodeQL) and textual analysis (using semgrep). Both have limitations, namely that CodeQL's Rust support is relatively new (only a year old, see blog) and lacks file:line querying equivalent to Java or C++ (example) and semgrep has no positional awareness.
So with the help of some linter preprocessing, we can achieve partial file:line awareness while allowing semgrep to more efficiently implement rules that can be represented in RegEx. This should give us enough flexibility to at a minimum, prevent regressions in code quality.
Additional Information
Dependency for sdk%feat: introduce trait
BaseCodecand{impl,codec}_*helper macros to reduce boilerplate, enforcecreate::preludeuse, drop premature limits #4CodeQL is not open-source software. The libraries, queries and other scaffolding are MIT-licensed (see github/codeql) but the engine (at github/codeql-cli-binaries) is subject to proprietary license terms (source)
These terms permit us to use CodeQL without obtaining a license for GitHub Advanced Security (a paid offering) as we fulfil the criteria laid out for unpaid use.
dashpay/base-sdk is MIT-licensed and hosted and maintained on GitHub
Our CI/CD infrastructure is based on GitHub Actions, hosted on
Github.comA change in either conditions would require us to re-evaluate our CodeQL usage though the alternatives are not encouraging.
Implementing skip support (required as CodeQL is a time-consuming step) required commonly agreed return codes which prompted a refactoring of the Python lint infrastructure in the form of introducing a common helper and moving to exception-based early-exit.
Skip support is absent for all other tests and will fail if the required binaries are missing.
Because CodeQL requires compilation, the lint runner and MSRV job in the stable build runner were merged together and spun off, this means we lose per-crate MSRV validation (but this is compensated for by workspace MSRV validation) which should additionally compensate moving away from the single vCPU runner due to the overall fewer jobs executed.
The lint and build jobs are parallel as CodeQL is sufficiently time-consuming that the MSRV build and validate is faster. The benefit of caching that comes from a sequential build is minimal.
Breaking Changes
None expected.
How Has This Been Tested?
Checklist