diff --git a/.cargo-husky/hooks/pre-push b/.cargo-husky/hooks/pre-push index b0d303b9..64abafaf 100755 --- a/.cargo-husky/hooks/pre-push +++ b/.cargo-husky/hooks/pre-push @@ -74,6 +74,14 @@ scripts/check-rehomed-docs.sh --base "${upstream:-origin/main}" cargo clippy --workspace --no-default-features --all-targets -- -D warnings +# The shipped (feature-off) configuration — see scripts/gate.sh for the full why (#1418). The line +# above cannot see it: `--all-targets` pulls dev units in, and dev-dep features then unify into the +# normal build, so the lib is linted with `instruments` ON while the release binary links it OFF. +# It belongs here and not only in the gate because this hook is the only check that runs on EVERY +# push (#1144), and the pass costs ~1 s warm. Measured: without it, this hook returns 0 on a planted +# E0599 that breaks `cargo build --release --no-default-features`. +cargo clippy --workspace --no-default-features -- -D warnings + if [ -z "${pkgs// /}" ]; then echo "pre-push: no crate-owned changes detected; fmt + clippy only." exit 0 diff --git a/crates/openpulse-modem/src/engine.rs b/crates/openpulse-modem/src/engine.rs index fa700b3d..e40d034b 100644 --- a/crates/openpulse-modem/src/engine.rs +++ b/crates/openpulse-modem/src/engine.rs @@ -14,9 +14,16 @@ use openpulse_core::conv::ConvCodec; use openpulse_core::dcd::DcdState; use openpulse_core::error::{ModemError, PluginError}; use openpulse_core::fec::{ - apply_window_retransmit, combine_llrs_map, combine_llrs_map_in_ranges, - encode_window_retransmit, hard_decide, FecCodec, FecMode, Interleaver, ShortFecCodec, - SoftCombiner, WindowArqFeedback, DEFAULT_INTERLEAVER_DEPTH, + combine_llrs_map, hard_decide, FecCodec, FecMode, Interleaver, ShortFecCodec, + DEFAULT_INTERLEAVER_DEPTH, +}; +// Reached only from `#[cfg(feature = "instruments")]` methods, so the shipped library does not use +// them (#1418). They must carry the same cfg or the feature-off build warns — which no lint pass saw +// until #1418 added one, because `--all-targets` turns `instruments` on. +#[cfg(feature = "instruments")] +use openpulse_core::fec::{ + apply_window_retransmit, combine_llrs_map_in_ranges, encode_window_retransmit, SoftCombiner, + WindowArqFeedback, }; use openpulse_core::frame::Frame; use openpulse_core::hpx::{HpxEvent, HpxSession, HpxState, HpxTransition}; @@ -6927,6 +6934,10 @@ impl ModemEngine { } /// Baseband-I/Q counterpart of [`stage_modulate_payload`](Self::stage_modulate_payload). + /// + /// Its only caller is [`transmit_iq`](Self::transmit_iq), which is itself instruments-only, so + /// this carries the same cfg (#1418). + #[cfg(feature = "instruments")] fn stage_modulate_payload_iq( &self, plugin: &dyn openpulse_core::plugin::ModulationPlugin, diff --git a/docs/dev/project/traceability.md b/docs/dev/project/traceability.md index 6c2b9114..c3797c1f 100644 --- a/docs/dev/project/traceability.md +++ b/docs/dev/project/traceability.md @@ -15,6 +15,58 @@ and the actually-observed results per change. --- +## 2026-09-19 — the gate's `--all-targets` hid the shipped configuration; #1418 + +**Change.** `scripts/gate.sh` and `.cargo-husky/hooks/pre-push` each ran exactly one clippy pass, +`--workspace --no-default-features --all-targets -- -D warnings`. `--all-targets` puts dev units in +scope and resolver 2 then unifies dev-dependency features into the normal build, so the +`openpulse-modem` lib was always linted with `instruments` **on** — while +`cargo build --release --no-default-features` (`release.yml:60`) links it **off**. No pass anywhere +linted the shipped configuration. + +**Design decision (reviewed by Fable, `docs/dev/reviews/review-1418-shipped-config-lint.md`).** Add a +second pass rather than change the flag, and keep it `--workspace`. Dropping `--all-targets` would +stop linting test code — the rot that put an unused binding in `session_key.rs`. Narrowing to +`-p openpulse-modem --lib` would miss a *downstream* crate's production code calling an instruments +item, because that crate's lib builds against the ON modem. The review corrected three things in my +framing: the feature has **three** enabling dev-deps (`openpulse-modem/Cargo.toml:41`, +`openpulse-daemon:92`, `openpulse-kiss:59`), not just #1277's self dev-dep, so removing that one +would not have closed it; the trigger is *dev units in scope*, for which `--all-targets` is only a +proxy (`--tests` alone flips it, `--bins`/`--lib` do not); and during a gate run downstream crates +link the **ON** lib, so the claim is about the shipped binaries, not about downstream crates. + +**Severity, corrected.** Filed first as two cosmetic warnings; it is a build-break class. An ungated +production caller of an instruments-only accessor is invisible to both automated checks and breaks +the release build. Corollary: #1277's "a production call cannot compile without a visible +`Cargo.toml` diff" was enforced by **release builds alone**, not by the gate or the hook. + +**Implementation.** `scripts/gate.sh:192` adds +`run_step "cargo clippy (shipped cfg) -D warns" cargo clippy --workspace --no-default-features -- -D warnings`; +`.cargo-husky/hooks/pre-push:76` adds the same command (it is the only check on every push, #1144, and +costs ~1 s warm). `crates/openpulse-modem/src/engine.rs` gates the two items the new pass then found: +five `openpulse_core::fec` imports moved to an `#[cfg(feature = "instruments")]` `use`, and +`stage_modulate_payload_iq` (:6930, whose only caller `transmit_iq` is instruments-only) given the +same cfg. + +**Tests → results (actually run, at `8b5896fa` + this branch).** + +- *Sabotage, the discriminating one* — planted `pub fn zz_probe_1418(e: &ModemEngine) -> u64 { e.notch_blocks_processed() }` + in `engine.rs`, then `scripts/gate.sh --quick`: the OLD step printed + `cargo clippy -D warnings ok` and the NEW step printed + `cargo clippy (shipped cfg) -D warns FAILED (exit 101)`. The old step passing is what makes the + failure attributable to the new pass rather than to the probe being loud. +- Pre-fix, both live instances under the new pass: `cargo clippy --workspace --no-default-features` + → 2 warnings (`--all-targets` → 0). Post-fix: both passes rc=0. +- Per-unit features read from cargo's `--message-format=json` `compiler-artifact` records: + `--all-targets` and `--tests` → `['instruments']`; `--bins`, `--lib`, no-target-flag → `[]`. +- Full gate: see the `GATE:` line recorded on PR #1419. + +**Scope, stated narrowly.** This makes the shipped configuration *linted*; it was already +*type-checked* in `ci.yml`'s macOS build, `cross check` and `release.yml` — all release-scoped. Do not +read this entry as "the OFF configuration was never compiled". + +--- + ## 2026-09-19 — CAP-33 does not own the engine's OTA arm, and should not; #1403 **Decision (maintainer, 2026-09-19): no map change.** `Refactors:` is **structural** — #1402 diff --git a/docs/dev/reviews/review-1418-shipped-config-lint.md b/docs/dev/reviews/review-1418-shipped-config-lint.md new file mode 100644 index 00000000..d575f4a5 --- /dev/null +++ b/docs/dev/reviews/review-1418-shipped-config-lint.md @@ -0,0 +1,86 @@ +--- +project: openpulsehf +doc: docs/dev/reviews/review-1418-shipped-config-lint.md +status: resolved +last_updated: 2026-09-19 +--- + +# Review — #1418, linting the shipped (feature-off) configuration + +## Prompt + +Fable was asked to **falsify**, not confirm, a finding and its write-up before either entered an +issue. The framing sent was: "the gate's own `--all-targets` turns the `instruments` feature ON, so +the library is never linted in the configuration downstream crates and shipped binaries link." It was +given the full apparatus (five clippy invocations with their warning counts), the draft issue text, +and six numbered attack points — the sharpest being #4, *"is 'never linted in the configuration +downstream crates link' actually true? Downstream workspace crates build openpulse-modem as a +dependency during the gate run — does that build see instruments on or off? If off, my headline claim +is wrong. Test this hard."* House rules on pipelines-never-carry-verdicts, positive controls for +grep-derived absences, and no build dirs in the worktree were included. + +## Verdict + +**Finding upheld, three corrections, and a re-classification from cosmetic to build-break.** + +1. **Mechanism confirmed but mis-attributed.** I blamed the #1277 self dev-dep alone. There are + **three** sites enabling `instruments`: `crates/openpulse-modem/Cargo.toml:41`, + `crates/openpulse-daemon/Cargo.toml:92`, `crates/openpulse-kiss/Cargo.toml:59`. Under `--workspace` + any one suffices, so removing the self dev-dep would not have closed it. Verified independently by + grep over all `Cargo.toml`. +2. **`--all-targets` is a proxy, not the trigger.** The trigger is *dev units in scope*; `--tests` + alone flips it, `--bins`/`--lib` do not. Resolved per-unit from cargo's `--message-format=json` + `compiler-artifact` records — `cargo tree -e features` cannot discriminate, as it prints the dev + edge either way. +3. **Point 4 was half-wrong, and the correction matters.** During the gate run there is exactly ONE + non-test `openpulse_modem` lib unit, features `['instruments']`, and all 14 dependents link it — so + in the gate, downstream crates link the **ON** lib. It is the **shipped binaries** (`release.yml:60`) + that link OFF. "Never linted" survives; "downstream crates link OFF" does not, and the issue text + was changed accordingly. Also corrected: OFF *is* type-checked (macOS build, `cross check`, + `release.yml`) — never **linted**. Do not let "never linted" harden into "never compiled". +4. **Severity raised.** I had filed it as two cosmetic warnings. A planted **ungated production + caller** of an instruments-only accessor is invisible to both automated checks and breaks the + release build — re-derived here rather than taken on trust: + + ``` + cargo clippy --workspace --no-default-features --all-targets -- -D warnings -> rc=0 (gate) + cargo test -p openpulse-modem --no-default-features --no-run -> rc=0 (hook) + cargo clippy -p openpulse-daemon --no-default-features -- -D warnings -> rc=101 E0599 + ``` + + Corollary: #1277's "a production call cannot compile without a visible `Cargo.toml` diff" is + enforced by **release builds alone**. +5. **Fix shape cleared, with two constraints.** Must be an **added** pass (dropping `--all-targets` + stops linting test code — the `session_key.rs` rot), and must be **`--workspace`**: narrowing to + `-p openpulse-modem --lib` would miss a downstream crate's production code calling an instruments + item, since that crate's lib builds against the ON modem. Runtime 9.1 s cold / ~1 s warm. +6. **Bottom line: file separately, not as a comment on #1380** — #1380 closes with a + `--features cpal-backend` PR that would leave this buried under a closed issue, and the fix is a + different gate step. Maintainer agreed; filed as #1418 and the #1380 comment reduced to a + cross-reference. + +Four line numbers in my draft were off by ~1; Fable's were right and were used. + +## Consumer + +`scripts/gate.sh:192` (the new `run_step`) and `.cargo-husky/hooks/pre-push:76`. Both are the direct +production callers — this change *is* verification machinery, so its consumer is the gate itself. The +downstream consumer of the property is `.github/workflows/release.yml:60` +(`cargo build --release … --no-default-features`), the build whose configuration went unlinted. + +## Prior art + +`grep -n "clippy" scripts/gate.sh .cargo-husky/hooks/pre-push` → one invocation each, both +`--all-targets`, no feature-off pass anywhere. `grep -RniE "all-targets|dev-depend" skills/` over the +skills tree (with a known-present control phrase to prove the filter fires) → no existing rule. #1380 +is the nearest existing mechanism and is the **inverse** case (feature off, code unchecked); its +proposed `cargo check --features cpal-backend` does not reach this. + +## Twins + +The hook is the twin of the gate and got the same pass — it is the only check that runs on every push +(#1144), and it was measured passing the planted E0599. The other candidate twins were checked and are +**not** affected: an awk census over every tracked `Cargo.toml`, scoped to `[dev-dependencies]` +sections enabling features on a workspace sibling, returns exactly the three `instruments` sites and +nothing else — so no other crate has this shape. `ci.yml`'s release-scoped jobs already compile OFF. `openpulse-cli`'s `cpal-backend` is the #1380 case, tracked +separately. diff --git a/scripts/gate.sh b/scripts/gate.sh index 4e0d419d..cbf0e10a 100755 --- a/scripts/gate.sh +++ b/scripts/gate.sh @@ -189,6 +189,21 @@ drift_check run_step "cargo fmt --check" cargo fmt --all -- --check || rc_total=1 drift_check run_step "cargo clippy -D warnings" cargo clippy --workspace --no-default-features --all-targets -- -D warnings || rc_total=1 +drift_check +# The SHIPPED configuration, which the step above structurally cannot see (#1418). `--all-targets` +# puts dev units in scope, and resolver 2 then unifies dev-dependency features into the normal build +# — three dev-deps enable `openpulse-modem`'s `instruments` (its own Cargo.toml:41, +# openpulse-daemon:92, openpulse-kiss:59), and under `--workspace` any one suffices. So the step +# above always lints the lib with `instruments` ON, while `cargo build --release +# --no-default-features` (release.yml) links it OFF. Measured: an ungated production caller of an +# instruments-only item returned rc=0 from the step above AND from the pre-push hook, and rc=101 +# (E0599) from `cargo clippy -p openpulse-daemon --no-default-features`. +# +# This is an ADDED pass, not a changed flag: dropping `--all-targets` above would stop linting test +# code, which is how an unused binding sat in session_key.rs. It must stay `--workspace` — a +# DOWNSTREAM crate's production code calling an instruments item also escapes, because that crate's +# lib builds against the ON modem. ~1 s warm; 9.1 s from a cold modem lib. +run_step "cargo clippy (shipped cfg) -D warns" cargo clippy --workspace --no-default-features -- -D warnings || rc_total=1 TEST_CMD="none" if [ "$MODE" = "full" ]; then