Fix TC-5274: Show fix patterns for all affected files - #352
Conversation
Problem: When scanning an RPM with a CVE affecting multiple files (e.g., openssl with 3 affected files: .c, .t, .eml), only the first C source file showed fix patterns. Test files and other formats were completely hidden from the report even though they had valid fixes. Root Cause: The _filter_review_snippets() function in cve_checker_report.py was too aggressive, filtering out: 1. Any file with /test/ in the path 2. Any file not ending in .c/.h/.cpp extensions This meant test files (.t) and data files (.eml) were excluded entirely, even when they were legitimately affected by the CVE and had fix patterns. Solution: 1. Modified _filter_review_snippets() to only exclude build-system files (CMakeLists.txt, Makefile, .cmake, .mk) while preserving all source files including tests and non-C/C++ files. 2. Improved _extract_snippets_from_patch() in code_agent_graph_defs.py to ensure one snippet per affected file is extracted first before adding additional snippets from files with multiple hunks. This prevents a single file with many changes from consuming all snippet slots. Impact: Users will now see fix patterns for ALL affected files regardless of file type (C, Perl, email, Python, etc.), making it clear what was actually fixed in each file. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…terns Co-Authored-By: Tamar Weisskopf <tweissko@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…rce only Applied consistent filtering to show only primary C/C++ source code files, excluding test files and build configuration from both sections. Co-Authored-By: Tamar Weisskopf <tweissko@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Tests the _is_source_code_file and _filter_to_source_code_files functions with the actual TC-5274 example case. Co-Authored-By: Tamar Weisskopf <tweissko@redhat.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PR Review — TC-5274
Nice cleanup: extracting the predicate into _is_source_code_file / _filter_to_source_code_files and applying it to both affected_files and review snippets removes the duplication and fixes the reported inconsistency. Snippet-filter behavior is preserved exactly, and the empty-result fallback is a sensible guard. A couple of things worth addressing below.
Important
/test/exclusion misses top-leveltest/directories, and the test gives false confidence. The"/test/" in file_pathcheck requires a leading slash, so a top-leveltest/foo.c(exactly how OpenSSL lays out its C test suite) is not matched, ends with.c, and is classified as source. The added assertion_is_source_code_file("test/recipes/80-test_cms.t") is Falsepasses only because.tisn't a source extension — the/test/branch is never actually exercised. A path liketest/bad_dtls_frag.cwould slip through. Considerfile_path.startswith("test/") or "/test/" in file_path, plus a test with a top-leveltest/*.cpath. (See inline comment.)
Suggestions
- Test coverage gaps: no cases for
CMakeLists/Makefileexclusion,.cpp/.cc/.cxxextensions, a top-leveltest/*.cfile, or the empty-result fallback (_filter_to_source_code_filesreturning the original list). The fallback is behaviorally important — if it regresses, reports could silently go empty — yet it's untested. Prefer plainassert-based pytest style over the trailingprint(...), and avoid thesys.path.insertmutation (it leaks into other tests) if a normal import /conftest.pycan be used.
| if "CMakeLists" in file_path or "Makefile" in file_path: | ||
| return False | ||
|
|
||
| if "/test/" in file_path: |
There was a problem hiding this comment.
"/test/" in file_path requires a leading slash, so a top-level test/*.c (e.g. OpenSSL's test/foo.c) is not excluded — it ends with .c and is treated as source, contradicting the "excluding test files" intent. Suggest if file_path.startswith("test/") or "/test/" in file_path: and add a test covering a top-level test/*.c path (the current .t assertion passes on extension, not on this branch).
- Added file_path.startswith('test/') to catch top-level test/*.c files
- Expanded test coverage with 12 comprehensive tests including the critical top-level test case
Fixes reviewer's main concern about /test/ check missing top-level test directories.
Co-Authored-By: Tamar Weisskopf <tweissko@redhat.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
47f015e to
ca00024
Compare
|
/test vulnerability-analysis-on-pr |
tmihalac
left a comment
tmihalac
left a comment
There was a problem hiding this comment.
A few correctness concerns on the new filtering helpers (posted inline). These focus on cases where the filter can silently alter which affected/source files reach the report.
| return False | ||
|
|
||
| # Only include C/C++ source files | ||
| return file_path.endswith((".c", ".h", ".cpp", ".cc", ".cxx")) |
There was a problem hiding this comment.
Whitelist can silently drop genuinely-affected non-C source in mixed lists. This admits only .c/.h/.cpp/.cc/.cxx. Because _filter_to_source_code_files only falls back to the original when the filtered result is empty, a mixed affected_files list keeps the .c files and silently discards real non-C source: .s/.S assembly and .pl perlasm generators (common in openssl — this PR's own example CVE), plus .hpp/.hh/.hxx headers. Note .hpp is also inconsistent with this module's own infer_language_from_path, which classifies it as C++. This works against the PR title "show fix patterns for all affected files." Consider broadening the extension set (at least .hpp/.hh/.hxx/.s/.S), or inverting the logic to exclude known-noise (tests/build) rather than whitelisting a narrow set.
There was a problem hiding this comment.
assembly files are not analyzed by the code agent
I will change the title as well
|
|
||
| def _filter_to_source_code_files(file_paths: list[str]) -> list[str]: | ||
| filtered = [f for f in file_paths if _is_source_code_file(f)] | ||
| return filtered if filtered else file_paths |
There was a problem hiding this comment.
Silent fallback to the unfiltered list, with no signal. When everything is filtered out (e.g. affected_files is all test/build files), this re-emits the full unfiltered list into the report with no logger.warning and no marker — reintroducing exactly the test/build noise this PR removes, presented as validated source. A module logger already exists and is used elsewhere in this file. Suggest logging a warning on the fallback path (and ideally rendering a distinct report status), e.g. if not filtered and file_paths: logger.warning(...) before returning.
| focusing the report on actual source code validation rather than tests or build files. | ||
| """ | ||
| # Exclude build-system files | ||
| if "CMakeLists" in file_path or "Makefile" in file_path: |
There was a problem hiding this comment.
Substring match on CMakeLists/Makefile matches anywhere in the path. This false-excludes legitimately named source such as src/MakefileParser.c or util/CMakeListsWriter.cpp, and anything under a directory whose name contains those substrings. Combined with the empty-only fallback, such a file can be silently dropped whenever other valid files exist. Prefer matching the basename / known suffixes (Makefile, Makefile.am, Makefile.in, CMakeLists.txt).
Fixes based on reviewer comments: 1. Build file matching: Match by basename instead of substring - Prevents false exclusion of files like src/MakefileParser.c - Only excludes actual build files: CMakeLists.txt, Makefile, etc. 2. Broader source file coverage: Add missing extensions - Added .hpp, .hh, .hxx for C++ headers - Added .s, .S for assembly files - Matches real-world openssl structure (assembly, perlasm) 3. Fallback logging: Warn when all files filtered - Log warning when falling back to unfiltered list - Makes silent fallback visible for debugging 4. Enhanced test coverage: 14 tests (was 12) - Test assembly file inclusion (.s, .S) - Test build file names in paths (MakefileParser.c) - Test additional C++ header extensions (.hpp, .hh, .hxx) - Test Makefile.in exclusion Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
TC-5274 is specifically for RPM workflow (C/C++ only). Assembly files (.s, .S) are not analyzed by the code agent and should not be included in the source file filter. Kept: .c, .h, .cpp, .cc, .cxx, .hpp, .hh, .hxx Removed: .s, .S Tests: 13 pass (removed assembly test) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Correct behavior when all affected files are tests/build: - Return empty list (not fallback to unfiltered) - Report shows 'not determined' for affected files - Report shows 'not shown' for fix patterns Rationale: - Test/build files are NOT actual source code - If only tests affected, no real source files affected - Empty list is semantically correct - Report has built-in handling for empty affected_files Updated warnings to use f-strings and removed ticket prefix. Updated test to verify empty list returned (not fallback). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
RPM workflow is C-only, not C++: - openssl: pure C library - libarchive: pure C library - No RPM packages use .cpp, .hpp, etc. Changed extensions from: .c, .h, .cpp, .cc, .cxx, .hpp, .hh, .hxx To: .c, .h (C-only) Updated docstring: 'C files only' (was 'C/C++ files') Updated tests: C++ extensions now excluded (13 tests pass) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changed docstring to generic 'RPM source code' instead of being C-specific, since the implementation includes C++ extensions (.cpp, .cc, .cxx). Updated tests to expect C++ files to be included: - test_cpp_files_included: now expects .cpp/.cc/.cxx to pass - test_filter_mixed_list: uses .cpp file - test_build_file_names_in_path_not_excluded: uses .cpp file All 13 tests pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changed module and test docstrings to be generic. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This analysis document was for investigation purposes and is not needed in the PR. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
2 participants
Fixes inconsistency where affected files section showed test files but fix patterns section did not.
Applied consistent filtering to both sections - now both show only C/C++ source code files, excluding test files and build configuration.
For RPM scan workflow only (C/C++ packages)
Fixes: TC-5274