Skip to content

fix(ci): repair main after 956f883e - #6551

Merged
ten9876 merged 1 commit into
aethersdr:mainfrom
skerker:fix/6550-codeql-deepcw-coverage
Oct 21, 2026
Merged

ten9876 merged 1 commit into
aethersdr:mainfrom
skerker:fix/6550-codeql-deepcw-coverage

Conversation

@skerker

@skerker skerker commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Fixes #6550

Summary

CodeQL / analyze (cpp) stops at "Check first-party compilation coverage" on main since 956f883e (#6300). In a configure without ONNX Runtime, as on the CodeQL runner, src/core/DeepCwEngine.cpp and src/core/DeepCwCommitter.cpp are compiled only by deepcw_ctc_test (aethercore compiles them under if(ORT_FOUND)), and that target is not in CODEQL_BUILD_TARGETS. This adds it, as the workflow's note says to ("add the target owning the named input here and dispatch a run"), and brings the comment above the list up to date.

Constitution principle honored

Principle VIII — the coverage check stays as strict as it is; the list grows to meet it.

Validation

The coverage check prints every first-party input the selected targets miss (missing = baseline - selected in tools/check_codeql_coverage.py). On the runner that set is exactly these two files (39052461796, and unchanged through 480a9aa8), and deepcw_ctc_test compiles both (tests/tests.cmake). Full Suite already builds and runs deepcw_ctc_test on Linux. CodeQL runs on pushes to main, so the first main run after merge confirms the fix on the runner.

Test plan

Checklist

  • Commits are signed
  • No new flat-key AppSettings calls — n/a, no source change
  • Code is clean-room — n/a, no source change
  • All meter UI uses MeterSmoother — n/a, no UI change
  • Documentation updated — n/a, no user-visible change
  • Security-sensitive changes reference a GHSA — n/a

— authored by agent (Claude Code) on behalf of @skerker

Add deepcw_ctc_test to CODEQL_BUILD_TARGETS. Without ONNX Runtime it is
the only target that compiles src/core/DeepCwEngine.cpp and
src/core/DeepCwCommitter.cpp, so the coverage check fails without it.

Fixes aethersdr#6550

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@skerker
skerker marked this pull request as ready for review October 17, 2026 20:00
@skerker
skerker requested a review from a team as a code owner October 17, 2026 20:00

@NF0T NF0T left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right fix, made at the place the workflow says to make it. I checked each claim in the body myself rather than going on the description.

Diagnosis holds.

  • deepcw_ctc_test (tests/tests.cmake:9913) compiles exactly DeepCwEngine.cpp and DeepCwCommitter.cpp. It sits outside any if(ORT_FOUND) and links nothing.
  • The only other places those files are compiled, aethercore (CMakeLists.txt:3223-3224) and deepcw_replay (:3577), are both inside if(ORT_FOUND). So in the runner's no-ORT configure, nothing in CODEQL_BUILD_TARGETS compiles them.
  • The missing-input set has not drifted. I pulled the coverage step from both ends of the failure run. On the first failing run (39052461796, 956f883e) and on the latest (39074256008, 450f7b39, which already includes the KAT500 merge), the list is the same two files: 691 inputs in total, 689 selected. Twelve consecutive main pushes have failed on this, and the PR still closes the whole gap.
  • The added target builds and passes in the CI image. Full Suite 39061705286 shows Test #833: deepcw_ctc_test ... Passed. (That run's three failures, control_receive_test and the two rtl_capture_worker tests, have nothing to do with this.) So the Build step won't fail where the coverage step used to.

Scope: one file, exactly what the title and body claim.

File Claimed Verdict
.github/workflows/codeql.yml: target list +1 yes in scope
.github/workflows/codeql.yml: comment "two" → "three" small tests yes in scope, and accurate

Nothing is deleted except the old comment wording. There's no new public surface and no source change. Preference vs. fix: this is a fix. The authority for it is the workflow's own instruction at lines 89-90.

Constitution: Principle VIII fits. The coverage guard stays as strict as it was, and the list grows to meet it rather than the check being relaxed. The commit is signed (verified: true).

CI: CodeQL has no pull_request trigger (deliberate, per #4655), so none of the green checks here exercise this change. The body says so and points to the first post-merge main run as confirmation, which is honest. The evidence above (the set arithmetic in check_codeql_coverage.py, plus Full Suite building the target in the same image) makes me confident it will pass.

What I tried to break

  • Did the missing set grow after #6300? No. It's identical at 956f883e and 450f7b39.
  • Does the target need ONNX, Qt, or anything the runner lacks? No. It has no target_link_libraries, includes only the two DeepCW headers and the std library, and Full Suite builds it in the CI container.
  • Is there another target that should own these files instead? No. Every other owner is gated on ORT_FOUND.
  • No test sources changed, so there was no socket-boundary concern. Nothing here is reachable from the app, so a bridge session doesn't apply.

Verdict: approve. Nothing blocks this. codeql.yml is @aethersdr/maintainers (CODEOWNERS:296), so this approval covers the Tier 3 gate only. Merging needs @ten9876 or @jensenpat.

Forward guidance (non-blocking)

  1. The workflow keeps a workflow_dispatch trigger "so a change to this file can be checked before it merges", and line 90 says to "dispatch a run". A branch dispatch would have turned "the first main run confirms it" into evidence on the PR itself. It's worth doing next time, since main has been red for a dozen pushes and a second miss would cost another merge cycle.
  2. Coverage here means preprocessor-branch coverage of the no-ORT build. CodeQL will now extract DeepCwEngine.cpp, but only its #else branches. The HAVE_ONNX sections (roughly lines 63-125, 228-265 and 305-364, the ONNX session code) are still never analyzed. The same applies to every ORT-gated source (DeepCwRxBackend.cpp, DeepFistCwModel.cpp, …), which isn't in the baseline at all. This PR doesn't change that, and it shouldn't be fixed here. If the maintainers want the neural decoders scanned, the CodeQL image would need ONNX Runtime. That's a separate posture decision.
  3. For the squash, a descriptive subject would read better in git log than "repair main after 956f883", for example: fix(ci): CodeQL builds deepcw_ctc_test so the DeepCW sources keep coverage. Principle VIII. (#6551)

Thanks for the tight diagnosis and for keeping the fix to the one line the workflow asks for.

73,
Ryan NF0T


👨🏼‍💻 Co-authored by Claude Opus 5.5

@jensenpat jensenpat self-assigned this Oct 18, 2026

@jensenpat jensenpat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 42529ffff8f09e72c5d24670bc7d78551bad850c. No code blockers found: this addresses #6550 by adding the unconditional, model-free target that owns the two missing compilation inputs. The coverage guard, triggers, permissions, action pins, and security policy are unchanged.

Scope Claimed in body Verdict
.github/workflows/codeql.yml:83-88, describe the three supplemental targets Yes Accurate and in scope
.github/workflows/codeql.yml:94, add deepcw_ctc_test to the scan build Yes Closes the reported missing-source gap

Blockers: no code blockers. Merge gate: this workflow is Tier 1 (.github/CODEOWNERS:296), owned by @aethersdr/maintainers. GitHub reports REVIEW_REQUIRED and requests that team. I am reviewing under jensenpat; the current CODEOWNERS roster lists that account under infrastructure/reviewers, and lists ten9876 under maintainers. This comment is not a substitute for the required maintainer approval.

Validation and attacks:

  • Independently downloaded the compilation-coverage artifact from CodeQL run 39074256008. Its baseline has 691 first-party inputs, its selected set has 689, and the exact difference is src/core/DeepCwCommitter.cpp and src/core/DeepCwEngine.cpp. Adding the two inputs from this target produces an empty missing set (691/691).
  • Traced every production owner: both aethercore and deepcw_replay gate these sources on ORT_FOUND; tests/tests.cmake:9835-9838 declares the added target unconditionally with both sources and no linked libraries. The runner log independently confirms ONNX Runtime is absent.
  • Extracted those four target declaration lines verbatim, adjusting source paths only, into an isolated CMake project. cmake --build /private/tmp/pr6551-target-proof/build -j22 built the exact PR-head test and both DeepCW translation units without ONNX. QT_QPA_PLATFORM=offscreen ctest --test-dir /private/tmp/pr6551-target-proof/build -j22 --no-tests=error: 1/1 passed. This is a focused macOS target build, not a full application or Linux CodeQL run.
  • All five posted PR checks pass; the commit signature is verified; there are no review threads; git diff --check passes. GitHub reports MERGEABLE, so no rebase is needed. Existing checks describe their original PR run and do not establish a fresh run against today's main.
  • No test sources changed or sockets were introduced. No application, backend, UI, RX, or TX code changes: bridge/hardware checks do not exercise this CI-only change. Automated code-review skill was unavailable; this was a manual adversarial pass.

Non-blocking limits: a complete Linux CodeQL extraction/analysis with the revised target list has not been run during this review. The local target build and actual runner source inventory verify the specific coverage fix; they do not claim that full scan passed. ONNX-enabled branch analysis remains outside this change's scope.

Recommendation: ready on technical grounds; merge after the required maintainer approval. Principle VIII is preserved: grow the compiled target set rather than relax coverage.

@jensenpat jensenpat removed their assignment Oct 18, 2026

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at 42529ff. deepcw_ctc_test is the only target that compiles DeepCwEngine.cpp and DeepCwCommitter.cpp in a configure without ONNX Runtime. The CodeQL C++ run at 86e90af still lists exactly those two files as missing, and no first-party source has changed on main since then. Merging re-runs CodeQL on main, and that run confirms the fix.

@ten9876
ten9876 merged commit 99fdc61 into aethersdr:main Oct 21, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CodeQL analyze (cpp) failing on main since #6300: coverage check omits the DeepCW sources

4 participants