Skip to content

fix: improve cxx detector scoping#415

Closed
Dragoy wants to merge 2 commits intopeteromallet:mainfrom
Dragoy:fix/cxx-detector-scoping
Closed

fix: improve cxx detector scoping#415
Dragoy wants to merge 2 commits intopeteromallet:mainfrom
Dragoy:fix/cxx-detector-scoping

Conversation

@Dragoy
Copy link
Contributor

@Dragoy Dragoy commented Mar 13, 2026

Summary

  • teach C/C++ test coverage to treat CMake target source membership as direct test coverage
  • filter C/C++ security findings to the scoped first-party file set and disable the unsound generic unused-import phase for C++
  • add regression tests covering CMake coverage mapping, external-header security filtering, and C++ phase configuration

Test Plan

  • python -m pytest desloppify/languages/cxx/tests/test_init.py desloppify/tests/lang/common/test_treesitter_analysis_direct.py desloppify/languages/cxx/tests/test_coverage.py desloppify/languages/cxx/tests/test_security.py -q

Copy link
Owner

@peteromallet peteromallet left a comment

Choose a reason for hiding this comment

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

Review

Three focused improvements to the C++ plugin — all addressing real problems. The implementation is sound.

Looks good

  • Security scoping: filtering findings to the scoped first-party file set is the right approach. Empty entry["file"] resolves to CWD which safely gets filtered out.
  • CMake coverage mapping: the _CMAKE_SOURCE_SPEC_RE regex handles common patterns well, and the test-tree ancestor walk is correctly scoped.
  • Disabling unused-imports for C++ — correct call, #include semantics don't map to the generic tree-sitter unused-import phase.
  • _extract_import_name refactor: the new ordering (strip path separators → strip extensions → split namespace separators) correctly handles cases like vendor/json.hpp.

Issues to fix

  1. Duplicate testtest_detect_cxx_security_ignores_findings_outside_scanned_files (~line 93 of test_security.py) and test_detect_cxx_security_ignores_findings_outside_scoped_files (~line 613) are nearly identical. Remove one.

  2. Missing newline at EOF in test_init.py and test_security.py — will likely fail linting.

Minor suggestions

  1. Extra blank lines added between functions in test_coverage.py and mapping_imports.py create noise — the codebase typically uses standard PEP 8 spacing.

  2. _discover_additional_test_mapping_files is exported in mapping_imports.__all__ with a leading underscore — slightly unusual for a public export. Since it's called cross-module, consider dropping the underscore.

  3. No test for _parse_cmake_source_specs with target_sources() or CMake comments — would be good to add.

Solid PR overall. Fix the duplicate test and EOF issues and it's good to merge.

@peteromallet
Copy link
Owner

Merged onto the 0.9.9 branch (7fe01b4) with minor cleanup — removed a duplicate test and added missing EOF newlines. Thanks @Dragoy!

peteromallet added a commit that referenced this pull request Mar 13, 2026
- Filter C/C++ security findings to scoped first-party file set
- CMake-based test coverage mapping via add_executable/add_library/target_sources
- Disable unsound generic unused-import phase for C++
- Fix _extract_import_name for C++ header extensions (.h, .hh, .hpp)
- Remove duplicate test, add missing EOF newlines

Co-Authored-By: Dragoy <Dragoy@users.noreply.github.com>
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.

2 participants