Skip to content

Improve first thought#291

Open
RedTanny wants to merge 12 commits into
RHEcosystemAppEng:mainfrom
RedTanny:improve-first-thought
Open

Improve first thought#291
RedTanny wants to merge 12 commits into
RHEcosystemAppEng:mainfrom
RedTanny:improve-first-thought

Conversation

@RedTanny

Copy link
Copy Markdown
Collaborator

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)

  • Prompt 1: CVE understanding (no patch required) — vulnerability type, component, ordered grep keywords
  • Prompt 2/3: Per-file patch analysis — functions/patterns per changed file, with token-budget batching for large patches
  • Result model: IntelGatheringResult aggregates per-file intel and converts back to VulnerabilityIntel for existing prompts/reports
  • Focused observations: when Source Grep targets a file, prepend that file’s intel into the observation context
  • New flow is now the sole intel path (dual-path flag removed)

Testing

Added unit coverage for the refactor and related empty/error tool handling:

  • test_file_change_summary.py — patch → per-file summaries / token estimates
  • test_cve_understanding_prompt.py — Prompt 1 formatting and keyword rules
  • test_patch_analysis_prompt.py — Prompt 2/3 formatting and functions summary helper
  • test_intel_gathering_result.py — lookup, aggregation, focused-context formatting
  • test_new_intel_flow.py — file priority + token-budget batch split
  • test_focused_observations.py — file filter extraction + focused context injection
  • test_check_empty_output.py — tool-error detection via status/prefix (not mid-content "error:")
  • tests/test_package_identifier.py — OSIDB affectedness / identify regression coverage

Bug fixes

  • Missing ADVISORY URL patterns / OSIDB in URL mining (b31b9d9): advisory/OSIDB URLs were missed when mining commit/advisory links for intel.
  • Weak hints continued into repo search (8ab9421): weak reference hints no longer keep searching repos; flow goes to the agent instead.
  • Noisy prompt regression (cafce28): removed extra prompt/intel fields that added noise and made the LLM less reliable.
  • “Patch file:” in report (47f8c01): dropped confusing “Patch file:” labeling from the checker report viewer.
  • Source Grep tool-call guidance (6c0576d): clarified grep tool instructions/schema so the LLM calls Source Grep correctly.
  • Identify regression (4132d2b): fixed PackageIdentifier affectedness regression after OSIDB support.

Also in this branch

  • OSIDB affectedness support in PackageIdentifier (55ac04d)
  • Stricter tool-error handling in observation/build agents so successful greps containing "error:" in source are not treated as tool failures

Test plan

  • Unit tests listed above
  • Local CVE run: Phoenix Initial_Intelligence_Gathering shows file_changes + CVE understanding
  • File-filtered Source Grep: observation span has focused_context_used=true
  • Spot-check known CVE verdicts for unexpected flips

@RedTanny
RedTanny requested review from TamarW0 and zvigrinberg July 13, 2026 07:40
@vbelouso

vbelouso commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@zvigrinberg zvigrinberg added enhancement New feature or request optimization labels Jul 13, 2026

@zvigrinberg zvigrinberg 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.

@RedTanny Thank you for the PR.
Please see my comments.

ReferenceHints,
AdvisoryContent,
GitSearchReport,
CONFIDENCE_THRESHOLD_MIN,

@zvigrinberg zvigrinberg Jul 13, 2026

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.

@RedTanny This ( CONFIDENCE_THRESHOLD_MIN) is not being used at all.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +606 to +613
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,
)

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.

@RedTanny This is not being used at all..

What is being used is at

https://github.com/RHEcosystemAppEng/exploit-iq-agent/pull/291/changes#diff-7e2d863e8e84f6d82687b61d6ae8e02d8abb5b2592f130abcc7df14d362fad07R620-R627

So it's either a duplication with redundant var , or worse even a bug.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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


basename = Path(file_path).name.lower()
for fs in self.file_summaries:
if basename in fs.file_path.lower():

@zvigrinberg zvigrinberg Jul 13, 2026

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.

@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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +618 to +636
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

@zvigrinberg zvigrinberg Jul 13, 2026

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.

@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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@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

@TamarW0

TamarW0 commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

/retest

@exploit-iq-pac

Copy link
Copy Markdown

Caution

There are some errors in your PipelineRun template.

PipelineRun Error
vulnerability-analysis-on-pr CEL expression evaluation error: expression "event == \"pull_request\" &&\n!body.pull_request.draft &&\n(target_branch == \"main\" || target_branch == \"rh-aiq-main\") &&\n(\"src/**\".pathChanged() || \"metrics_lib/**\".pathChanged() || \"pyproject.toml\".pathChanged() || \"uv.lock\".pathChanged() || \"Dockerfile\".pathChanged() || \".dockerignore\".pathChanged())\n" failed to evaluate: no such key: pull_request

@TamarW0

TamarW0 commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

/test vulnerability-analysis-on-pr

@etsien

etsien commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

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.

@zvigrinberg

Copy link
Copy Markdown
Collaborator

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.

RedTanny and others added 12 commits July 26, 2026 07:28
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>
@RedTanny
RedTanny force-pushed the improve-first-thought branch from 2601f5a to 97f5d75 Compare July 26, 2026 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants