feat: strip all logging from release binaries#27
Closed
jacderida wants to merge 1 commit intoWithAutonomi:mainfrom
Closed
feat: strip all logging from release binaries#27jacderida wants to merge 1 commit intoWithAutonomi:mainfrom
jacderida wants to merge 1 commit intoWithAutonomi:mainfrom
Conversation
Greptile SummaryThis PR introduces a Key concerns:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/logging.rs | New module introducing cfg(debug_assertions)-gated wrapper macros for all tracing levels (trace, debug, info, warn, error). Macros expand to nothing in release builds, correctly eliminating callsite metadata from production binaries. |
| src/lib.rs | Adds pub mod logging and a crate-wide #![cfg_attr(not(debug_assertions), allow(unused_variables))] lint suppression. The latter is overly broad and could mask unrelated unused-variable bugs in release builds. |
| .github/workflows/ci.yml | Adds no-logs-in-release CI job that builds the release binary and checks for tracing callsite strings. The grep pattern matches /home/ paths, which will catch callsites from cargo git/registry deps in CI — meaning the job will fail while the upstream ant-evm PR remains open. |
| Cargo.toml | Points saorsa-core, saorsa-pqc, and ant-evm at personal git fork branches. Correct for in-flight dep PRs, but must be reverted to published semver versions before merging to main. |
| src/config.rs | Gates the log_level field and default_log_level function behind cfg(debug_assertions), with consistent handling in Default impl. Change is clean and minimal. |
| src/bin/saorsa-node/cli.rs | Correctly gates all four logging CLI args (--log-level, --log-format, --log-dir, --log-max-files) and the config.log_level assignment behind cfg(debug_assertions). |
| src/bin/saorsa-node/main.rs | Entire tracing-subscriber initialization block (including file appender, rolling config, and guard) gated behind cfg(debug_assertions). Imports split correctly with cfg attributes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cargo build] --> B{debug_assertions?}
B -- "debug build" --> C[logging macros → tracing::info!/debug!/etc.]
B -- "release build" --> D[logging macros → empty block, no-op]
C --> E[tracing-subscriber initialized\nlog_level / log_format / log_dir\nCLI args available]
D --> F[tracing-subscriber NOT initialized\nlog args hidden from --help\nzero callsite metadata in binary]
E --> G[Binary contains\ncallsite strings:\nfile.rs:line]
F --> H[Binary contains\nNO callsite strings\nCI check passes]
subgraph "src/logging.rs macros"
M1["trace! / debug! / info!\nwarn! / error!"]
M2["#[cfg(debug_assertions)]\n{ tracing::info!(...) }"]
M1 --> M2
end
subgraph "CI: no-logs-in-release"
CI1[cargo build --release --bin saorsa-node]
CI2["strings binary | grep 'event src/\|/home/'"]
CI3{MATCHES > 0?}
CI1 --> CI2 --> CI3
CI3 -- yes --> CI4[exit 1 FAIL]
CI3 -- no --> CI5[PASS]
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/lib.rs
Line: 41
Comment:
**Overly broad unused-variable suppression**
`#![cfg_attr(not(debug_assertions), allow(unused_variables))]` silences ALL unused-variable warnings across the entire crate in release builds, not just those arising from the logging macro no-ops. Any variable that is computed but never consumed in a release build (for reasons unrelated to logging) will be silently accepted — a potential source of hidden bugs.
A narrower approach is to suppress the lint only on the specific variables that are only consumed through logging. For example, variables passed exclusively to `info!`/`debug!`/etc. calls can be annotated individually:
```rust
let value = compute_something();
let _ = value; // suppress only this variable in release
```
Or, if the intent is purely to suppress the variables used inside the `#[cfg(debug_assertions)]` blocks in the binary entry points (which are already cfg-gated at the block level), those blocks can use `let _ = var;` inside each `#[cfg(debug_assertions)]` path instead of a crate-wide suppression.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 95-99
Comment:
**CI check will fail due to remaining dep callsites**
The grep pattern `'^event (src/|/home/).*\.rs:[0-9]+'` matches absolute paths rooted at `/home/`, which covers the GitHub Actions cargo registry path (`/home/runner/.cargo/registry/src/…`) and git checkouts (`/home/runner/.cargo/git/checkouts/…`).
The PR test plan explicitly notes *"3 remain from published deps, will drop to 0 when dep PRs merge"* — specifically `maidsafe/autonomi` (ant-evm) has a pending upstream PR. In CI, those 3 callsites will have paths like `/home/runner/.cargo/git/checkouts/autonomi-.../…/file.rs:42`, which match `/home/`, causing `MATCHES` to be 3 and the job to exit 1.
This means the `no-logs-in-release` job will fail on every CI run of this PR until the upstream `ant-evm` dependency is updated to a version with logging stripped. Consider either:
1. Temporarily raising the threshold (`if [ "$MATCHES" -gt 3 ]`) with a comment explaining it will tighten once deps land, or
2. Scoping the pattern more narrowly to only match paths originating from this repository's own sources (e.g. `^event src/.*\.rs:[0-9]+`) to exclude dep callsites.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: Cargo.toml
Line: 35-37
Comment:
**Git-branch dependencies should not be merged to main**
`saorsa-core`, `saorsa-pqc`, and `ant-evm` all point to personal fork branches (`jacderida/…`). This is intentional while the dep PRs are open, but merging this into `main` with git-branch deps would break reproducible builds, prevent `cargo publish`, and make `cargo update` unreliable.
Ensure this PR is not merged until all three upstream PRs land and these entries are reverted to published semver versions:
```toml
saorsa-core = "0.18" # or whichever version ships the no-logging change
saorsa-pqc = "0.6"
ant-evm = "0.1.20"
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: b6f6663
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure all logging/tracing is fully stripped from release binaries by routing logging through cfg(debug_assertions)-gated macros and disabling log-related CLI/config/subscriber setup outside debug builds.
Changes:
- Introduces
src/logging.rswrapper macros (trace!,debug!,info!,warn!,error!) that compile to no-ops in release builds. - Replaces
tracing::*imports/usages across the codebase to use crate-provided macros, and gates CLI/config log options + tracing-subscriber initialization behindcfg(debug_assertions). - Adds a CI job that inspects the release binary with
stringsto detect lingering tracing callsite metadata.
Reviewed changes
Copilot reviewed 21 out of 27 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/upgrade/signature.rs | Switches debug logging import to crate macro. |
| src/upgrade/rollout.rs | Switches debug logging import to crate macro. |
| src/upgrade/monitor.rs | Switches debug/info/warn imports to crate macros. |
| src/upgrade/mod.rs | Switches debug/info/warn imports to crate macros. |
| src/upgrade/apply.rs | Switches debug/error/info/warn imports to crate macros. |
| src/storage/lmdb.rs | Switches debug/trace/warn imports to crate macros. |
| src/storage/handler.rs | Switches debug/info/warn imports to crate macros. |
| src/payment/verifier.rs | Switches debug/info imports to crate macros. |
| src/payment/single_node.rs | Switches info import to crate macro. |
| src/payment/quote.rs | Switches debug import + one error log callsite to crate macro. |
| src/payment/metrics.rs | Switches debug/info/warn imports to crate macros. |
| src/node.rs | Switches debug/error/info/warn imports to crate macros. |
| src/logging.rs | Adds cfg(debug_assertions)-gated wrapper macros around tracing. |
| src/lib.rs | Exposes logging module and adjusts linting for release builds. |
| src/devnet.rs | Switches debug/info/warn imports to crate macros. |
| src/config.rs | Gates log_level config field and default behind debug builds. |
| src/client/self_encrypt.rs | Switches info/warn imports to crate macros. |
| src/client/quantum.rs | Switches debug/info/warn imports to crate macros. |
| src/client/chunk_protocol.rs | Switches debug/warn imports to crate macros. |
| src/bin/saorsa-node/main.rs | Gates tracing-subscriber init + uses crate info macro. |
| src/bin/saorsa-node/cli.rs | Gates logging CLI args and log_level config application. |
| src/bin/saorsa-devnet/main.rs | Gates tracing-subscriber init + uses crate info macro. |
| src/bin/saorsa-devnet/cli.rs | Gates devnet --log-level behind debug builds. |
| src/bin/saorsa-cli/main.rs | Gates tracing-subscriber init + uses crate info macro. |
| src/bin/saorsa-cli/cli.rs | Gates CLI --log-level behind debug builds. |
| Cargo.toml | Points deps to git branches with corresponding logging-stripping changes. |
| .github/workflows/ci.yml | Adds “no-logs-in-release” binary strings inspection job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jacderida
force-pushed
the
chrisoneil/v2-124-strip-logging-from-release
branch
from
b6f6663 to
515b061
Compare
Add logging.rs module with cfg(debug_assertions)-gated wrapper macros
that compile to nothing in release builds. This ensures no tracing
callsite metadata (module paths, file paths, field names) is present
in release binaries.
Changes:
- Replace all `use tracing::{...}` with `use crate::{...}` in src/
- Gate logging CLI args (--log-level, --log-format, --log-dir,
--log-max-files) behind cfg(debug_assertions)
- Gate tracing-subscriber initialization in all binaries
- Gate log_level config field behind cfg(debug_assertions)
- Point dependency crates at git branches with matching changes
- Add no-logs-in-release CI job
Part of V2-124.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jacderida
force-pushed
the
chrisoneil/v2-124-strip-logging-from-release
branch
from
515b061 to
13575f6
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure release binaries contain zero tracing/logging callsite metadata by routing all logging through cfg(debug_assertions)-gated wrapper macros, and by gating log-related CLI/config fields and tracing-subscriber initialization to debug builds only.
Changes:
- Introduce
src/logging.rswrapper macros (trace!/debug!/info!/warn!/error!) that compile to no-ops in release. - Replace
tracing::{...}logging imports withcrate::{...}macros across the codebase and gate tracing initialization + logging CLI/config behindcfg(debug_assertions). - Add a CI job that builds the release binary and checks it for tracing callsite strings via
strings.
Reviewed changes
Copilot reviewed 20 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/upgrade/signature.rs | Switch debug logging import to crate macro. |
| src/upgrade/rollout.rs | Switch debug logging import to crate macro. |
| src/upgrade/monitor.rs | Switch logging imports to crate macros; adjust log-only variables. |
| src/upgrade/mod.rs | Switch logging imports to crate macros. |
| src/upgrade/apply.rs | Switch logging imports to crate macros. |
| src/storage/lmdb.rs | Switch logging imports to crate macros; adjust log-only error binding. |
| src/storage/handler.rs | Switch logging imports to crate macros; introduce log-only computed values. |
| src/payment/verifier.rs | Switch logging imports to crate macros; keep tracing::enabled! guards. |
| src/payment/single_node.rs | Switch info logging import to crate macro. |
| src/payment/quote.rs | Switch debug logging import to crate macro; keep tracing::enabled! guards. |
| src/payment/metrics.rs | Switch logging imports to crate macros; adjust log-only variables/bindings. |
| src/node.rs | Switch logging imports to crate macros; adjust log-only variables/bindings. |
| src/logging.rs | Add cfg(debug_assertions)-gated logging macros intended to compile out in release. |
| src/lib.rs | Add logging module to the crate. |
| src/devnet.rs | Switch logging imports to crate macros; adjust log-only variables/bindings. |
| src/config.rs | Gate log_level config field + default behind cfg(debug_assertions). |
| src/client/self_encrypt.rs | Switch logging imports to crate macros; adjust log-only variables/bindings. |
| src/client/quantum.rs | Switch logging imports to crate macros; keep tracing::enabled! and add log-only computed values. |
| src/client/chunk_protocol.rs | Switch logging imports to crate macros; adjust log-only variables/bindings. |
| src/bin/saorsa-node/main.rs | Gate tracing-subscriber init and log-format support behind cfg(debug_assertions). |
| src/bin/saorsa-node/cli.rs | Gate logging CLI args and config wiring behind cfg(debug_assertions). |
| src/bin/saorsa-devnet/main.rs | Gate tracing-subscriber init behind cfg(debug_assertions). |
| src/bin/saorsa-devnet/cli.rs | Gate --log-level behind cfg(debug_assertions). |
| src/bin/saorsa-cli/main.rs | Gate tracing-subscriber init behind cfg(debug_assertions). |
| src/bin/saorsa-cli/cli.rs | Gate --log-level behind cfg(debug_assertions). |
| .github/workflows/ci.yml | Add “no-logs-in-release” job using strings to detect tracing callsite artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
286
to
290
| if tracing::enabled!(tracing::Level::DEBUG) { | ||
| let xorname_hex = hex::encode(xorname); | ||
| let quote_count = payment.peer_quotes.len(); | ||
| debug!("Verifying EVM payment for {xorname_hex} with {quote_count} quotes"); | ||
| let _xorname_hex = hex::encode(xorname); | ||
| let _quote_count = payment.peer_quotes.len(); | ||
| debug!("Verifying EVM payment for {_xorname_hex} with {_quote_count} quotes"); | ||
| } |
Comment on lines
159
to
162
| if tracing::enabled!(tracing::Level::DEBUG) { | ||
| let content_hex = hex::encode(content); | ||
| debug!("Generated quote for {content_hex} (size: {data_size}, type: {data_type})"); | ||
| let _content_hex = hex::encode(content); | ||
| debug!("Generated quote for {_content_hex} (size: {data_size}, type: {data_type})"); | ||
| } |
Comment on lines
133
to
136
| let address = request.address; | ||
| let addr_hex = hex::encode(address); | ||
| debug!("Handling PUT request for {addr_hex}"); | ||
| let _addr_hex = hex::encode(address); | ||
| debug!("Handling PUT request for {_addr_hex}"); | ||
|
|
Comment on lines
+527
to
530
| let _total_amount: Amount = prepared.iter().map(|p| p.payment.total_amount()).sum(); | ||
| let _chunk_count = prepared.len(); | ||
| info!("Batch payment for {_chunk_count} chunks: {_total_amount} atto total"); | ||
|
|
Comment on lines
+544
to
551
| let _unique_tx_count = { | ||
| let mut txs: Vec<_> = tx_hash_map.values().collect(); | ||
| txs.sort(); | ||
| txs.dedup(); | ||
| txs.len() | ||
| }; | ||
| info!("Batch payment successful: {unique_tx_count} on-chain transaction(s) for {chunk_count} chunks"); | ||
| info!("Batch payment successful: {_unique_tx_count} on-chain transaction(s) for {_chunk_count} chunks"); | ||
|
|
Comment on lines
175
to
178
| if tracing::enabled!(tracing::Level::DEBUG) { | ||
| let addr_hex = hex::encode(address); | ||
| debug!("Querying saorsa network for chunk: {addr_hex}"); | ||
| let _addr_hex = hex::encode(address); | ||
| debug!("Querying saorsa network for chunk: {_addr_hex}"); | ||
| } |
Collaborator
Author
|
This approach will not be implemented. |
mickvandijke
added a commit
that referenced
this pull request
Apr 1, 2026
Add unit and e2e tests covering the remaining Section 18 scenarios: Unit tests (32 new): - Quorum: #4 fail→abandoned, #16 timeout→inconclusive, #27 single-round dual-evidence, #28 dynamic threshold undersized, #33 batched per-key, #34 partial response unresolved, #42 quorum-derived paid-list auth - Admission: #5 unauthorized peer, #7 out-of-range rejected - Config: #18 invalid config rejected, #26 dynamic paid threshold - Scheduling: #8 dedup safety, #8 replica/paid collapse - Neighbor sync: #35 round-robin cooldown skip, #36 cycle completion, #38 snapshot stability mid-join, #39 unreachable removal + slot fill, #40 cooldown peer removed, #41 cycle termination guarantee, consecutive rounds, cycle preserves sync times - Pruning: #50 hysteresis prevents premature delete, #51 timestamp reset on heal, #52 paid/record timestamps independent, #23 entry removal - Audit: #19/#53 partial failure mixed responsibility, #54 all pass, #55 empty failure discard, #56 repair opportunity filter, response count validation, digest uses full record bytes - Types: #13 bootstrap drain, repair opportunity edge cases, terminal state variants - Bootstrap claims: #46 first-seen recorded, #49 cleared on normal E2e tests (4 new): - #2 fresh offer with empty PoP rejected - #5/#37 neighbor sync request returns response - #11 audit challenge multi-key (present + absent) - Fetch not-found for non-existent key Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
Summary
logging.rsmodule withcfg(debug_assertions)-gated wrapper macrosuse tracing::{...}withuse crate::{...}across 16 source files--log-level,--log-format,--log-dir,--log-max-files) behindcfg(debug_assertions)log_levelconfig field behindcfg(debug_assertions)no-logs-in-releaseCI job that checks the release binary withstringsLogging macros compile to nothing in release builds, ensuring zero tracing callsite metadata in production binaries. Debug builds retain full logging with all CLI args available.
Depends on:
Part of V2-124.
Test plan
cargo build(debug) — passescargo build --release— passes, zero tracing callsite strings in binarycargo test --lib— 263 tests passsaorsa_node::callsite strings (3 remain from published deps, will drop to 0 when dep PRs merge)🤖 Generated with Claude Code