Improve first thought#291
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
zvigrinberg
left a comment
There was a problem hiding this comment.
@RedTanny Thank you for the PR.
Please see my comments.
| ReferenceHints, | ||
| AdvisoryContent, | ||
| GitSearchReport, | ||
| CONFIDENCE_THRESHOLD_MIN, |
There was a problem hiding this comment.
@RedTanny This ( CONFIDENCE_THRESHOLD_MIN) is not being used at all.
There was a problem hiding this comment.
| prompt = PATCH_ANALYSIS_PROMPT.format( | ||
| vuln_id=vuln_id, | ||
| vulnerability_type=result.cve_understanding.vulnerability_type if result.cve_understanding else "", | ||
| affected_component=result.cve_understanding.affected_component if result.cve_understanding else "", | ||
| root_cause=result.cve_understanding.root_cause if result.cve_understanding else "", | ||
| functions_touched_summary=functions_summary, | ||
| patch_content=patch_content, | ||
| ) |
There was a problem hiding this comment.
@RedTanny This is not being used at all..
What is being used is at
So it's either a duplication with redundant var , or worse even a bug.
There was a problem hiding this comment.
|
|
||
| basename = Path(file_path).name.lower() | ||
| for fs in self.file_summaries: | ||
| if basename in fs.file_path.lower(): |
There was a problem hiding this comment.
@RedTanny This is not safe enough, as for example both reasonable.c and sonable.c will match, while it's obvious these are two different files.
another example log.c prelog.c.
Maybe full check after transforming both sides to lowercases?
There was a problem hiding this comment.
| for fs in files: | ||
| # Per-file invocation to get structured output per file | ||
| file_prompt = PATCH_ANALYSIS_PROMPT.format( | ||
| vuln_id=vuln_id, | ||
| vulnerability_type=result.cve_understanding.vulnerability_type if result.cve_understanding else "", | ||
| affected_component=result.cve_understanding.affected_component if result.cve_understanding else "", | ||
| root_cause=result.cve_understanding.root_cause if result.cve_understanding else "", | ||
| functions_touched_summary=format_functions_touched_summary([fs]), | ||
| patch_content=_format_patch_content_for_files(parsed_patch, [fs]), | ||
| ) | ||
| file_intel: PerFileIntel = await per_file_llm.ainvoke([SystemMessage(content=file_prompt)]) | ||
| file_intel.file_path = fs.file_path # Ensure path is set | ||
| results.append(file_intel) | ||
|
|
||
| span.set_output({ | ||
| "files_analyzed": len(results), | ||
| "files": [r.file_path for r in results], | ||
| }) | ||
| return results |
There was a problem hiding this comment.
@RedTanny Why running for each file LLM linearly ?
All files are independent, right?
So better IMO to create tasks for all files, and then run all tasks concurrently using asyncio.gather, it will improve performance and will make it much more rapid.
There was a problem hiding this comment.
@zvigrinberg
What's happening:
Lines 608-620: sequential LLM calls per file inside run_patch_analysis()
Lines 630-633: batch1 and batch2 already run in parallel via asyncio.gather
So there's already some parallelism at the batch level. The suggestion is to add a second layer of parallelism inside each batch.
Concerns with blindly parallelizing:
Rate limiting — LLM APIs often have rate limits (requests/min, tokens/min). Firing all file calls simultaneously could trigger throttling or 429 errors.
Intentional backpressure — Sequential calls may be deliberate to avoid overwhelming the LLM service, especially if file count is variable (could be 2 files or 20).
Marginal gain — If typical file count per batch is small (2-4 files), the latency improvement may be minimal vs. added complexity.
because the above i tend to keep it as is
|
/retest |
|
Caution There are some errors in your PipelineRun template.
|
|
/test vulnerability-analysis-on-pr |
|
Just FYI, with the multi-prompting framework coming, these prompts will be moved to centralized locations, with separate copies per model and if necessary, per environment (I know of Java-specific prompts for certain tool usage chains). So please be aware of this coming change, and a big rebase/merge. |
@etsien But the RPM analysis is out of scope for multi repo... ( this could be a future enhancement POST-GA that if the TPA guys wants to prioritize it will happen). So it's expected to be quite smooth rebase. |
… and llm didn't follow prompt less accurate
Implements issues 01-06 of intel gathering refactor: - FileChangeSummary model and extraction (01) - file_changes span output (02) - CVE_UNDERSTANDING_PROMPT (03) - PATCH_ANALYSIS_PROMPT (04) - IntelGatheringResult object (05) - Dual-path intel gathering with enable_new_intel_flow config (06) Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
2601f5a to
97f5d75
Compare
Summary
Refactors L1 intel gathering so the checker extracts CVE understanding and patch intel separately (per-file), then feeds focused context into Source Grep observations. Also includes related checker bugfixes and OSIDB identify improvements from this branch.
Why
The old single-prompt intel extraction mixed CVE narrative with full-patch analysis, which hurt keyword quality on large patches and gave observations little file-specific guidance. Splitting the flow improves search targeting and keeps large patches within the context budget.
High-level feature (intel refactor)
IntelGatheringResultaggregates per-file intel and converts back toVulnerabilityIntelfor existing prompts/reportsTesting
Added unit coverage for the refactor and related empty/error tool handling:
test_file_change_summary.py— patch → per-file summaries / token estimatestest_cve_understanding_prompt.py— Prompt 1 formatting and keyword rulestest_patch_analysis_prompt.py— Prompt 2/3 formatting and functions summary helpertest_intel_gathering_result.py— lookup, aggregation, focused-context formattingtest_new_intel_flow.py— file priority + token-budget batch splittest_focused_observations.py— file filter extraction + focused context injectiontest_check_empty_output.py— tool-error detection via status/prefix (not mid-content"error:")tests/test_package_identifier.py— OSIDB affectedness / identify regression coverageBug fixes
b31b9d9): advisory/OSIDB URLs were missed when mining commit/advisory links for intel.8ab9421): weak reference hints no longer keep searching repos; flow goes to the agent instead.cafce28): removed extra prompt/intel fields that added noise and made the LLM less reliable.47f8c01): dropped confusing “Patch file:” labeling from the checker report viewer.6c0576d): clarified grep tool instructions/schema so the LLM calls Source Grep correctly.4132d2b): fixed PackageIdentifier affectedness regression after OSIDB support.Also in this branch
PackageIdentifier(55ac04d)"error:"in source are not treated as tool failuresTest plan
Initial_Intelligence_Gatheringshowsfile_changes+ CVE understandingfocused_context_used=true