Fix findings upload during dry

[SeanTConrad]

Why are these changes needed?

A November release had changes to fix the scan from crashing when the machine had no Internet access. These changes accidentally introduced a bug where the findings file would not upload on a "dry" run.

Related issue number

Checks

• [ x ] I've added tests (if relevant) corresponding to the changes introduced in this PR.
• [ x ] I've made sure all auto checks have passed.

Summary by Sourcery

Bug Fixes:

• Fixed a bug where the findings file was not uploaded during dry runs.

[sourcery-ai]

Reviewer's Guide by Sourcery

This PR fixes a bug where the findings file was not uploaded during a dry run. It modifies the agent.py file to remove a conditional check that prevented the findings file from being uploaded when self.config.dry was true, and updates the test_dry.py file to assert that the findings file is now uploaded during a dry run.

Sequence diagram for findings upload during dry run

sequenceDiagram
    participant Agent
    participant Config
    participant Upload

    Note over Agent: Changes to handle dry run
    Agent->>Config: Check dry run status
    Config-->>Agent: Return dry status
    Agent->>Agent: Process findings
    Note over Agent: Remove conditional check
    Agent->>Upload: Upload findings file
    Note over Upload: Upload checks should_upload internally
    Upload-->>Agent: Upload complete

Loading

Flow diagram for findings upload logic

flowchart TD
    A[Start] --> B[Process Repository]
    B --> C[Generate Findings]
    C --> D{Findings Success?}
    D -->|Yes| E[Upload Findings]
    D -->|No| F[Skip Processing]
    E --> G[End]
    F --> G

    note[Upload happens regardless of dry run status]

Loading

File-Level Changes

Change	Details	Files
Always upload findings file during dry runs	Removed the conditional check based on self.config.dry before uploading the findings file. Added a comment explaining that the findings file should always be uploaded, even during dry runs, and that the .upload method handles the check for whether the upload should actually occur.	src/verinfast/agent.py
Updated test to verify findings file upload during dry runs	Added assertions to verify that the findings file is present in the output directory after a dry run. Added assertions to check for specific log messages related to the attempted upload of the findings file during a dry run.	tests/test_dry.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time. You can also use
  this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey @SeanTConrad - I've reviewed your changes and they look great!

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🟢 Security: all looks good
• 🟢 Testing: all looks good
• 🟢 Complexity: all looks good
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.55%. Comparing base (8a89f21) to head (7edbef7).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #657      +/-   ##
==========================================
- Coverage   77.57%   77.55%   -0.03%     
==========================================
  Files          29       29              
  Lines        2181     2179       -2     
==========================================
- Hits         1692     1690       -2     
  Misses        489      489              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aylusltd]

this will cause errors when we attempt to upload files that don't exist, won't it? We've had that before where one bit fails

[SeanTConrad]

The check for "if Path(git_output_file).exists():" was added two years ago, but there's not a similar check for all the other uploads. So all the others write this to the log:

2025-01-13 11:04:05 /home/stconrad/verinfast/src/verinfast/agent.py@168 upload: File does not exist: /home/stconrad/veri
nfast/tests/results_dry/small-test-repo.git.git.log.json
2025-01-13 11:04:05 /home/stconrad/verinfast/src/verinfast/agent.py@168 upload: File does not exist: /home/stconrad/veri
nfast/tests/results_dry/small-test-repo.git.sizes.json
2025-01-13 11:04:05 /home/stconrad/verinfast/src/verinfast/agent.py@168 upload: File does not exist: /home/stconrad/veri
nfast/tests/results_dry/small-test-repo.git.stats.json
2025-01-13 11:04:05 /home/stconrad/verinfast/src/verinfast/agent.py@168 upload: File does not exist: /home/stconrad/veri
nfast/tests/results_dry/small-test-repo.git.findings.json

Upload has an error check:

    def upload(self, file: str, route: str, source: str = '', isJSON=True):
        if not self.config.shouldUpload:
            self.log(
                msg='Skipping Uploads',
                tag=f"Skipping uploading {file} for {source} to {self.config.baseUrl}/{route}.",
                display=True
            )
            return True
        if not Path(file).exists():
            self.log(
                msg=f"File does not exist: {file}"
            )
            return False

I think we added the error check to upload, but never cleaned up git, because it was first.