Skip to content

fix(pipeline): a run reported twice the findings a scan actually produced - #385

Merged
0xmanhnv merged 1 commit into
developfrom
fix/run-findings-double-count
Jul 31, 2026
Merged

fix(pipeline): a run reported twice the findings a scan actually produced#385
0xmanhnv merged 1 commit into
developfrom
fix/run-findings-double-count

Conversation

@0xmanhnv

Copy link
Copy Markdown
Collaborator

Observed on a real run

A scan on a live stack pushed 2 findings. Every layer agreed except one:

agent:    "Push completed: 2 findings created, 0 updated"
database:  2 rows in findings
pipeline_runs.total_findings:  4      <--

Why

OnStepCompleted calls stepRun.Complete(findingsCount), which stores the count
on the step run. calculateRunStats then sums exactly those step runs. Adding
findingsCount on top of that sum counts this step's findings a second time:

stepRun.Complete(findingsCount, output)          // step run now holds 2
...
completed, failed, skipped, findings := s.calculateRunStats(run)   // findings = 2
s.runRepo.UpdateStats(..., findings+findingsCount)                 // 2 + 2 = 4

It did not stay in one column

The doubled number also reached:

  • the audit event metadata, on both the success and failure branches
  • the user-facing message"Pipeline run completed successfully with N findings"

So a run that found 2 secrets told the operator it found 4, and left that claim in
the tamper-evident audit trail.

The tell

OnStepFailed (run.go:580) always computed this correctly, from
calculateRunStats alone. The two paths disagreed and only the quieter one was
right
— and nothing compares them, which is part of why it survived.

With multiple steps the old arithmetic compounded: every completion re-added the
current step's count on top of a sum that already included it.

Fix

Drop the extra addition at all four sites so the run total is derived purely from
the step runs, matching OnStepFailed.

How it was found

By running an actual scan end to end against a real agent binary — not by reading
the code. findings+findingsCount reads perfectly plausibly; it only looks wrong
once you have a real number to check it against. That is the argument for the
end-to-end coverage added in #383: this bug sat directly on the path those tests
now cover, and was invisible until something real ran through it.

Verification

  • TestScanLoop_RunFindingsCountIsNotDoubled asserts the run total equals what
    the step reported, and that the step run itself holds the true number.
  • Confirmed to fail against the old expression — re-introducing
    findings+findingsCount breaks it.
  • Full suite, gofmt and golangci-lint --new-from-rev=origin/develop clean, each
    checked by exit code.

…uced

A real scan on a live stack pushed 2 findings. The agent logged "Push completed:
2 findings created", the database held 2 — and the pipeline run recorded
total_findings = 4.

OnStepCompleted calls stepRun.Complete(findingsCount), which stores the count on
the step run. calculateRunStats then sums exactly those step runs. Adding
findingsCount on top of that sum counts this step's findings a second time.

The doubled number did not stay in one column. It also reached:

  - the audit event metadata for both the success and failure branches
  - the user-facing message, "Pipeline run completed successfully with N findings"

so a run that found 2 secrets told the operator it found 4, and left that claim
in the tamper-evident audit trail.

OnStepFailed (run.go:580) always computed this correctly from calculateRunStats
alone. The two paths disagreed and only the quieter one was right, which is part
of why it survived: nothing compares them.

Fix: drop the extra addition at all four sites so the run total is derived purely
from the step runs, matching OnStepFailed. With multiple steps the old arithmetic
compounded — every completion re-added the current step's count on top of a sum
that already included it.

Found by running an actual scan end to end against a real agent binary rather
than by reading the code; the arithmetic looks reasonable until you have a real
number to check it against. The regression test was confirmed to fail against the
old expression.
@0xmanhnv
0xmanhnv merged commit 26d31b1 into develop Jul 31, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant