Repository navigation
fix(ci): repair main after 956f883e - #6551
Conversation
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>
NF0T
left a comment
There was a problem hiding this comment.
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 exactlyDeepCwEngine.cppandDeepCwCommitter.cpp. It sits outside anyif(ORT_FOUND)and links nothing.- The only other places those files are compiled,
aethercore(CMakeLists.txt:3223-3224) anddeepcw_replay(:3577), are both insideif(ORT_FOUND). So in the runner's no-ORT configure, nothing inCODEQL_BUILD_TARGETScompiles 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 consecutivemainpushes 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_testand the twortl_capture_workertests, 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
956f883eand450f7b39. - 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)
- The workflow keeps a
workflow_dispatchtrigger "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 firstmainrun confirms it" into evidence on the PR itself. It's worth doing next time, sincemainhas been red for a dozen pushes and a second miss would cost another merge cycle. - Coverage here means preprocessor-branch coverage of the no-ORT build. CodeQL will now extract
DeepCwEngine.cpp, but only its#elsebranches. TheHAVE_ONNXsections (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. - For the squash, a descriptive subject would read better in
git logthan "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
left a comment
There was a problem hiding this comment.
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 issrc/core/DeepCwCommitter.cppandsrc/core/DeepCwEngine.cpp. Adding the two inputs from this target produces an empty missing set (691/691). - Traced every production owner: both
aethercoreanddeepcw_replaygate these sources onORT_FOUND;tests/tests.cmake:9835-9838declares 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 -j22built 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 --checkpasses. GitHub reportsMERGEABLE, 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.
ten9876
left a comment
There was a problem hiding this comment.
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.
Fixes #6550
Summary
CodeQL / analyze (cpp)stops at "Check first-party compilation coverage" onmainsince956f883e(#6300). In a configure without ONNX Runtime, as on the CodeQL runner,src/core/DeepCwEngine.cppandsrc/core/DeepCwCommitter.cppare compiled only bydeepcw_ctc_test(aethercorecompiles them underif(ORT_FOUND)), and that target is not inCODEQL_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 - selectedintools/check_codeql_coverage.py). On the runner that set is exactly these two files (39052461796, and unchanged through480a9aa8), anddeepcw_ctc_testcompiles both (tests/tests.cmake). Full Suite already builds and runsdeepcw_ctc_teston Linux. CodeQL runs on pushes tomain, so the firstmainrun after merge confirms the fix on the runner.Test plan
Checklist
AppSettingscalls — n/a, no source changeMeterSmoother— n/a, no UI change— authored by agent (Claude Code) on behalf of @skerker