sdk%feat(test): introduce bsdk-util with bspcheck verb to validate against blockchain.dat linearized chains, add git filter helper script, extend Transaction's Checkable to validate sptx#13
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a git-range command runner, shared Python table utilities, a Rust ChangesPython tooling updates
bsdk-util Rust CLI
Transaction payload validation
Suggested reviewers
🚥 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. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)contrib/build_docs.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.16][ERROR]: unable to find a config; path contrib/git_filter.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.20][ERROR]: unable to find a config; path contrib/lint/lint_codeql.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.12][ERROR]: unable to find a config; path
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 |
|
Note This pull request has no conflicts! 🎊 🎉 🎊 |
|
@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
🤖 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 `@contrib/git_filter.py`:
- Around line 83-86: The list comprehension on line 85 uses
CommitResult(*line.split(" ", 1)) which assumes every log line contains both a
commit hash and a subject separated by a space. When a commit has an empty
subject, the split produces only one element, causing an unpacking error. Fix
this by modifying the unpacking logic to handle cases where the split returns
only one element, either by filtering out such lines or by ensuring a default
empty string is provided for missing subjects when constructing the CommitResult
instances.
In `@pkgs/dev/Cargo.toml`:
- Around line 24-25: The `full` feature in the Cargo.toml currently includes
both "std" and "serde", which violates the crate feature policy by implicitly
enabling serialization for all `full` consumers. Modify the `full` feature
definition to only include "std" (change `full = ["std", "serde"]` to `full =
["std"]`) and keep the `serde` feature as a separate, standalone optional
feature on its own line. This ensures that consumers who use the `full` feature
get only the standard library support and must explicitly opt into `serde` where
needed (such as directly in binary targets).
🪄 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: d187709e-6389-408d-a19c-a71d2641e811
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.gitignorecontrib/common.pycontrib/git_filter.pycontrib/lint_all.pypkgs/dev/Cargo.tomlpkgs/dev/build.rspkgs/dev/src/bin/bsdk_util/bspcheck.rspkgs/dev/src/bin/bsdk_util/logging.rspkgs/dev/src/bin/bsdk_util/main.rspkgs/dev/src/bin/bsdk_util/platform.rspkgs/dev/src/bin/bsdk_util/policy.rspkgs/primitives/src/payload/mod.rspkgs/primitives/src/transaction.rs
| full = ["std", "serde"] | ||
| serde = ["dep:serde", "dash-primitives/serde", "dash-types/serde"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep full as std-only to match crate feature policy.
full = ["std", "serde"] breaks the manifest contract and implicitly enables serialization for any full consumer. Keep
full = ["std"] and opt into serde explicitly where needed (for example, directly in bin).
As per coding guidelines, **/Cargo.toml: configure features with full = ["std"] and keep serde optional/separate.
Suggested diff
-full = ["std", "serde"]
+full = ["std"]
bin = [
"full",
+ "serde",
"dep:chrono",
"dep:ciborium",
"dep:clap",🤖 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 `@pkgs/dev/Cargo.toml` around lines 24 - 25, The `full` feature in the
Cargo.toml currently includes both "std" and "serde", which violates the crate
feature policy by implicitly enabling serialization for all `full` consumers.
Modify the `full` feature definition to only include "std" (change `full =
["std", "serde"]` to `full = ["std"]`) and keep the `serde` feature as a
separate, standalone optional feature on its own line. This ensures that
consumers who use the `full` feature get only the standard library support and
must explicitly opt into `serde` where needed (such as directly in binary
targets).
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contrib/git_filter.py (1)
236-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow or intentionally suppress the catch-all exception.
Ruff flags Line 236 with BLE001. Prefer catching the expected operational failures, or add a targeted
# noqa: BLE001if this CLI intentionally converts all unexpected exceptions intoRETCODE_ERR.🤖 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/git_filter.py` around lines 236 - 238, The broad catch-all exception handler using `except Exception as exc:` at line 236 triggers a Ruff BLE001 warning. Either narrow the exception handling to catch only the specific operational exceptions expected in this CLI context (such as FileNotFoundError, ValueError, etc.) and handle each appropriately, or if the CLI intentionally converts all unexpected exceptions to RETCODE_ERR, add a targeted `# noqa: BLE001` comment on the except line to explicitly suppress this warning.Source: Linters/SAST tools
🤖 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 `@contrib/git_filter.py`:
- Around line 205-213: The worktree is reused across multiple commits in the
loop iterating through commits, but files modified or generated by the
subprocess.run() call persist between iterations. After the subprocess.run()
execution completes and before the next _git_ok() checkout call in the commit
loop, add git reset and git clean commands using _git_ok() to clean the worktree
of any artifacts or modifications left by the previous command execution,
ensuring each commit is tested with a clean state.
- Around line 199-200: The print statement for args.env at line 200 is echoing
full K=V pairs which can leak credentials into CI logs. Instead of printing the
complete environment values, modify the logic to either print only the keys or
redact the values by showing only the key names with masked values (e.g.,
"KEY=***"). When iterating through args.env to build the output string, parse
each pair and reconstruct it with redacted values before joining and printing.
---
Nitpick comments:
In `@contrib/git_filter.py`:
- Around line 236-238: The broad catch-all exception handler using `except
Exception as exc:` at line 236 triggers a Ruff BLE001 warning. Either narrow the
exception handling to catch only the specific operational exceptions expected in
this CLI context (such as FileNotFoundError, ValueError, etc.) and handle each
appropriately, or if the CLI intentionally converts all unexpected exceptions to
RETCODE_ERR, add a targeted `# noqa: BLE001` comment on the except line to
explicitly suppress this warning.
🪄 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: d3a3c397-d33f-4e84-8c17-9d8f1fdd6487
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.gitignorecontrib/codeql/lib/policy.qllcontrib/common.pycontrib/git_filter.pycontrib/lint_all.pypkgs/dev/Cargo.tomlpkgs/dev/build.rspkgs/dev/src/bin/bsdk_util/bspcheck.rspkgs/dev/src/bin/bsdk_util/logging.rspkgs/dev/src/bin/bsdk_util/main.rspkgs/dev/src/bin/bsdk_util/platform.rspkgs/dev/src/bin/bsdk_util/policy.rspkgs/primitives/src/payload/mod.rspkgs/primitives/src/transaction.rs
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (11)
- pkgs/dev/src/bin/bsdk_util/platform.rs
- pkgs/dev/build.rs
- pkgs/dev/src/bin/bsdk_util/policy.rs
- pkgs/dev/Cargo.toml
- pkgs/primitives/src/transaction.rs
- contrib/lint_all.py
- pkgs/primitives/src/payload/mod.rs
- pkgs/dev/src/bin/bsdk_util/logging.rs
- pkgs/dev/src/bin/bsdk_util/main.rs
- contrib/common.py
- pkgs/dev/src/bin/bsdk_util/bspcheck.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@contrib/codeql/lib/policy.qll`:
- Around line 118-124: The `not isPrivateCrate(t)` condition placed in the
shared `isCheckableType` predicate is overly broad and suppresses both serde
derivation findings and derive-required checks for private crates. Remove `not
isPrivateCrate(t)` from the `isCheckableType` predicate to keep it as a general
eligibility check. Instead, apply the private crate condition specifically in
the missing-required-traits code path within the downstream
`contrib/codeql/attrib.ql` file where derive-only suppression is actually
needed, ensuring that only derive-required checks are gated for private crates
while serde derivation findings remain active.
🪄 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: 6b0eb282-bcf9-4bb2-8436-f9290d73014a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.gitignorecontrib/build_docs.pycontrib/codeql/lib/policy.qllcontrib/git_filter.pycontrib/lint/lint_codeql.pycontrib/lint/lint_javascript.pycontrib/lint/lint_markdown.pycontrib/lint/lint_python.pycontrib/lint/lint_rust.pycontrib/lint/lint_semgrep.pypkgs/dev/Cargo.tomlpkgs/dev/build.rspkgs/dev/src/bin/bsdk_util/bspcheck.rspkgs/dev/src/bin/bsdk_util/logging.rspkgs/dev/src/bin/bsdk_util/main.rspkgs/dev/src/bin/bsdk_util/platform.rspkgs/dev/src/bin/bsdk_util/policy.rspkgs/primitives/src/payload/mod.rspkgs/primitives/src/transaction.rspyproject.toml
✅ Files skipped from review due to trivial changes (8)
- contrib/lint/lint_markdown.py
- pyproject.toml
- contrib/lint/lint_python.py
- contrib/build_docs.py
- .gitignore
- contrib/lint/lint_semgrep.py
- contrib/lint/lint_codeql.py
- contrib/lint/lint_javascript.py
🚧 Files skipped from review as they are similar to previous changes (9)
- pkgs/dev/build.rs
- pkgs/dev/src/bin/bsdk_util/policy.rs
- pkgs/dev/src/bin/bsdk_util/platform.rs
- pkgs/dev/src/bin/bsdk_util/logging.rs
- pkgs/primitives/src/transaction.rs
- pkgs/primitives/src/payload/mod.rs
- pkgs/dev/Cargo.toml
- pkgs/dev/src/bin/bsdk_util/main.rs
- pkgs/dev/src/bin/bsdk_util/bspcheck.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contrib/git_filter.py (1)
81-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle empty commit subjects without crashing.
This same parsing hazard is still present:
%H %splus.strip()can leave only the hash for an empty-subject commit, soCommitResult(*line.split(" ", 1))raises before any command runs.Proposed fix
def _enumerate_commits(base: str, tip: str, cwd: str) -> list[CommitResult]: """List commits in base..tip order (oldest first).""" - out = _git_ok("log", "--reverse", "--format=%H %s", f"{base}..{tip}", cwd=cwd) + out = _git_ok("log", "--reverse", "--format=%H%x00%s", f"{base}..{tip}", cwd=cwd) return [ - CommitResult(*line.split(" ", 1)) + CommitResult(commit_hash, subject) for line in out.splitlines() if line.strip() + for commit_hash, _, subject in [line.partition("\x00")] ]#!/bin/bash # Verify Git emits a parseable record for an empty-subject commit. set -eu tmp="$(mktemp -d)" trap 'rm -rf "$tmp"' EXIT git -C "$tmp" init -q git -C "$tmp" config user.email test@example.invalid git -C "$tmp" config user.name tester git -C "$tmp" commit --allow-empty --allow-empty-message -m "" -q git -C "$tmp" log --reverse --format='%H %s' HEAD~1..HEAD | python3 - <<'PY' import sys for line in sys.stdin.read().strip().splitlines(): print(line.split(" ", 1)) PY🤖 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/git_filter.py` around lines 81 - 88, The commit parsing in _enumerate_commits is still unsafe for empty-subject commits because using "%H %s" with strip() can reduce a record to just the hash and make CommitResult(*line.split(" ", 1)) fail. Update the parsing in _enumerate_commits to handle missing subjects explicitly, preserving the hash and treating an empty subject as an empty string instead of splitting blindly; keep the fix localized to the CommitResult construction path.
🤖 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.
Duplicate comments:
In `@contrib/git_filter.py`:
- Around line 81-88: The commit parsing in _enumerate_commits is still unsafe
for empty-subject commits because using "%H %s" with strip() can reduce a record
to just the hash and make CommitResult(*line.split(" ", 1)) fail. Update the
parsing in _enumerate_commits to handle missing subjects explicitly, preserving
the hash and treating an empty subject as an empty string instead of splitting
blindly; keep the fix localized to the CommitResult construction path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca300668-1f51-4652-8f26-29648d1682ba
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.gitignorecontrib/build_docs.pycontrib/codeql/lib/policy.qllcontrib/common.pycontrib/git_filter.pycontrib/lint/lint_codeql.pycontrib/lint/lint_javascript.pycontrib/lint/lint_markdown.pycontrib/lint/lint_python.pycontrib/lint/lint_rust.pycontrib/lint/lint_semgrep.pycontrib/lint_all.pypkgs/dev/Cargo.tomlpkgs/dev/build.rspkgs/dev/src/bin/bsdk_util/bspcheck.rspkgs/dev/src/bin/bsdk_util/logging.rspkgs/dev/src/bin/bsdk_util/main.rspkgs/dev/src/bin/bsdk_util/platform.rspkgs/dev/src/bin/bsdk_util/policy.rspkgs/primitives/src/payload/mod.rspkgs/primitives/src/transaction.rspyproject.toml
✅ Files skipped from review due to trivial changes (7)
- pyproject.toml
- contrib/lint/lint_javascript.py
- contrib/lint/lint_markdown.py
- contrib/lint/lint_python.py
- contrib/lint/lint_codeql.py
- .gitignore
- contrib/build_docs.py
🚧 Files skipped from review as they are similar to previous changes (14)
- pkgs/dev/build.rs
- contrib/lint/lint_semgrep.py
- pkgs/dev/src/bin/bsdk_util/platform.rs
- contrib/codeql/lib/policy.qll
- pkgs/dev/src/bin/bsdk_util/policy.rs
- contrib/lint/lint_rust.py
- contrib/lint_all.py
- pkgs/primitives/src/payload/mod.rs
- pkgs/dev/Cargo.toml
- pkgs/primitives/src/transaction.rs
- pkgs/dev/src/bin/bsdk_util/main.rs
- contrib/common.py
- pkgs/dev/src/bin/bsdk_util/bspcheck.rs
- pkgs/dev/src/bin/bsdk_util/logging.rs
1 participant
Additional Information
{Addr,Service,NetInfo}{V1,V2}, define surrounding traits (Net{Addr,Info}), defineCheckable, add synthetic vectors, correct impl. errors #12Checklist