Fix the two failing admin request-pinning tests on develop - #11124
Merged
Conversation
The native test harness boots Portduino in simulated mode, and
wouldEncryptWithPKC() short-circuits to false whenever
portduino_config.force_simradio is set. noteOutgoingAdminRequest() derives its
pin from that predicate:
keyValid = haveDestKey && wouldEncryptWithPKC(&p, p.channel, haveDestKey);
so under the harness no outgoing admin request is ever key-pinned, and
responseIsSolicited() admits a plaintext response to a PKC-pinned request.
test_pinned_request_keeps_its_key_after_an_unpinned_request and
test_request_to_keyed_node_pins_the_stored_key have therefore failed since
#11092 added them, on develop and on every branch that merges it.
Instrumenting the predicate shows every other term already satisfied
(haveDestKey=1, isFromUs=1, private_key=32, unicast, ADMIN_APP, channel
LongFast) with wouldEncryptWithPKC=0, leaving only the simradio guard.
Clear the flag in setUp so the fixture models a real device. Skipping PKC under
force_simradio is correct for a simulated radio - there is no PKC to pin - so
the predicate is left alone rather than relaxed to make a test pass. The only
sim-mode path in this suite is AdminModule's exit_simulator intercept, which no
test here exercises.
Native suite: 38 suites, 723/723 cases, no sanitizer findings.
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (30)
Build artifacts expire on 2026-08-20. Updated for |
Contributor
📝 WalkthroughWalkthroughThe admin session repro test now includes Portduino glue on Portduino builds and disables forced simradio behavior during test setup, allowing PKC key-pinning logic to run in a non-simulated device configuration. ChangesPortduino admin session test
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@test/test_admin_session_repro/test_main.cpp`:
- Around line 164-170: Shorten the comment above portduino_config.force_simradio
to one or two lines, retaining only the essential rationale that simulated mode
disables PKC and prevents the pinning tests from exercising their assertions.
Leave the assignment unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f24ad154-3c4c-4c03-9e84-f54e3e7dc1ab
📒 Files selected for processing (1)
test/test_admin_session_repro/test_main.cpp
Comment on lines
+164
to
+170
| #ifdef ARCH_PORTDUINO | ||
| // The native test harness boots Portduino in simulated mode, and wouldEncryptWithPKC() | ||
| // hard-disables PKC whenever force_simradio is set. Left true, no outgoing admin request is | ||
| // ever key-pinned, so the pinning tests below cannot exercise what they are asserting. | ||
| // Model a real (non-sim) device instead. | ||
| portduino_config.force_simradio = false; | ||
| #endif |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Shorten the explanatory comment.
The four-line comment exceeds the project’s one-or-two-line comment limit. Keep only the essential rationale; the force_simradio assignment is already self-explanatory.
Proposed revision
`#ifdef` ARCH_PORTDUINO
- // The native test harness boots Portduino in simulated mode, and wouldEncryptWithPKC()
- // hard-disables PKC whenever force_simradio is set. Left true, no outgoing admin request is
- // ever key-pinned, so the pinning tests below cannot exercise what they are asserting.
- // Model a real (non-sim) device instead.
+ // Disable simulated-radio gating so these tests exercise PKC pinning.
portduino_config.force_simradio = false;
`#endif`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #ifdef ARCH_PORTDUINO | |
| // The native test harness boots Portduino in simulated mode, and wouldEncryptWithPKC() | |
| // hard-disables PKC whenever force_simradio is set. Left true, no outgoing admin request is | |
| // ever key-pinned, so the pinning tests below cannot exercise what they are asserting. | |
| // Model a real (non-sim) device instead. | |
| portduino_config.force_simradio = false; | |
| #endif | |
| `#ifdef` ARCH_PORTDUINO | |
| // Disable simulated-radio gating so these tests exercise PKC pinning. | |
| portduino_config.force_simradio = false; | |
| `#endif` |
🤖 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 `@test/test_admin_session_repro/test_main.cpp` around lines 164 - 170, Shorten
the comment above portduino_config.force_simradio to one or two lines, retaining
only the essential rationale that simulated mode disables PKC and prevents the
pinning tests from exercising their assertions. Leave the assignment unchanged.
Source: Coding guidelines
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.
develop is currently red on
test-native. Two tests intest/test_admin_session_repro/have failed since #11092 added them, and every branch that merges develop inherits the failure (e.g. #10967).Root cause
The native test harness boots Portduino in simulated mode (the run prints
Running in simulated mode.,PortduinoGlue.cpp:362).wouldEncryptWithPKC()short-circuits on that:noteOutgoingAdminRequest()derives the pin from that predicate:so
keyValidis never set, nothing is pinned, andresponseIsSolicited()admits a plaintext response to what should be a PKC-pinned request — exactly the two assertions.Instrumenting the predicate confirms every other term was already satisfied, leaving only the simradio guard:
Fix
Clear the flag in
setUpso the fixture models a real device.Skipping PKC under
force_simradiois correct for a simulated radio — there is no PKC to pin — so this deliberately does not relaxwouldEncryptWithPKC()to make a test pass. The change is test-only.The only sim-mode path in this suite is AdminModule's
exit_simulatorintercept, which no test here exercises, so clearing the flag cannot affect a sibling test.Verification
Full native suite on this branch (Docker,
coverageenv, ASan/LSan):PASSFor contrast, develop at
d587f0848reports724 test cases: 2 failed, 721 succeeded.🤖 Generated with Claude Code
Summary by CodeRabbit