Skip to content

Refactor MR creation flow in CreateFromVulnerabilityDataService

Michael Beckerrequested to merge
497193-refactor-vulnerability-remediation-mr-flow into master

What does this MR do and why?

Context

We support two workflows that generate patch MRs for a given vulnerability.

  1. supplied patch files (Vulnerabilities::Remediation) or
  2. LLM generated patches ("Duo Vulnerability Resolution")

While debugging and resolving a bug in a separate MR, I noticed that we check which one of these workflows we are currently doing throughout the MR creation process.1

It is nice to branch the logic at the top level, so that we:

  • don't need the "which workflow am I in?" checks
  • changes in one workflow have less of a risk to affecting the other2
  • can get a high level overview of all preconditions, errors, and the workflow by reading execute

This Change

This change is extracted from an MR regarding a bug in the LLM patch generation logic.

The major structural change is that the logic checking whether we are doing a normal patch or an LLM patch has been moved to the top-level of execute

Some additional changes are that:

  • precondition assertions have been moved to the top level
  • error handling has been moved to the top level
  • the CreateFromVulnerabilityDataService now handles all StandardErrors in the two workflows and passes back a ServiceResponse error

Followup work

In the MR I extracted this from, I am moving all of the llm patch generation logic into its own (unit tested) class.

I think that the LLM response extraction code could undergo the same treatment. We have experimenting with new prompts on the medium term road-map, and having expanded test coverage there could give us assistance if the prompt format ends up changing.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

related to: #536007
related to: #497193 (closed)
related to: #526882
resolves: #536029 (closed)

  1. For all the hairy details with regards to the bug investigation, you can begin reading in this issue ↩

  2. As mentioned in the "Followup work" section, the bug fix this was spawned from involves significant changes to the patch logic. Getting these structural changes in first results in a simpler diff. The simpler diff can help focus the MR review on the logic change ↩

Edited by Michael Becker

Merge request reports