refactor(#198): extract FormatFinding and LogFindings helpers

[fullsend-ai-coder]

Add shared helper functions to internal/security/ for formatting Finding details, eliminating duplicated formatting loops across scan.go. FormatFinding returns "[severity] name: detail" using the Name field (pattern name/category) consistently, resolving the Scanner vs Name inconsistency where scan.go:125 used f.Scanner while other callers used f.Name.

Changes:

• Add FormatFinding(f Finding) string to internal/security/scanner.go
• Add LogFindings(w io.Writer, findings []Finding) to scanner.go
• Update 3 callers in internal/cli/scan.go to use FormatFinding
• Add unit tests for both helpers in scanner_test.go

The Name field identifies what was found (e.g. "github_pat", "zero_width") which is more actionable for users than Scanner which identifies the scanner that detected it. The choice is documented in FormatFinding godoc.

Note: bootstrap_scan.go logs findings at aggregate level only (count and severity) with no per-finding formatting loops, so no changes were needed there.

---

Closes #198

Post-script verification

• Branch is not main/master (agent/198-format-finding-helper)
• Secret scan passed (gitleaks — f96750babbed5ada406a9ae04e8068449701d9c7..HEAD)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[fullsend-ai-review]

Review

Findings

Medium

• [behavioral change] internal/cli/scan.go:196 — The scan output subcommand's per-finding log line changes from name: detail (no severity bracket) to [severity] name: detail (with severity bracket). This is an undocumented format change to stderr diagnostic output that could affect log parsers matching the old format. The PR description mentions resolving the Scanner vs Name inconsistency but does not mention the addition of the severity prefix at this call site.
  Remediation: Acknowledge this intentional format change in the PR description. If backward compatibility of CLI stderr output matters, consider whether this needs a deprecation notice.

Low

• [incomplete-scope] internal/cli/scan.go:125 — Issue Extract shared helper for formatting security finding details #198 states that both bootstrap_scan.go and scan.go should be updated, but the PR only updates scan.go. This is correct — bootstrap_scan.go has no per-finding formatting loops (it only logs aggregate-level messages like counts and severity). The PR should note why bootstrap_scan.go was excluded from scope.
• [incomplete migration] internal/cli/scan.go:305 — Two other finding-formatting locations remain with inline fmt.Sprintf: scan.go:305 (URL scan, uses StepFail with URL context) and run.go:1718 (sanitized finding, includes file path context). These have legitimately different formatting needs that FormatFinding does not address.
• [documentation_comment_style] internal/security/scanner.go:118 — The FormatFinding doc comment could more clearly separate the purpose statement from the Scanner vs Name design rationale. The first sentence is good; the subsequent justification is useful but verbose for a godoc comment.
• [dead code] internal/security/scanner.go:127 — LogFindings has zero callers in the codebase. All three updated call sites use FormatFinding directly with printer.StepWarn, which is incompatible with the io.Writer interface LogFindings requires. This adds exported API surface and an io import with no current consumer.

Previous run

Review of PR #226

Verdict: Approve

This PR extracts FormatFinding and LogFindings helpers into internal/security/scanner.go, replacing three inline fmt.Sprintf calls in internal/cli/scan.go. The refactoring is well-scoped to the work authorized by issue #198 and resolves the Scanner vs Name field inconsistency documented there.

What was checked

• Correctness: The new helpers are logically correct. Tests cover the primary format, the Name-over-Scanner choice, empty fields, multiple/single/nil findings for LogFindings, and line-count assertions. No existing tests were weakened.
• Security: No injection risks. FormatFinding uses %s verb formatting to stderr — no execution context. No auth, permissions, or secrets changes.
• Intent & coherence: All changes trace directly to issue Extract shared helper for formatting security finding details #198's scope. The approach (helpers alongside HasCriticalFindings in scanner.go) matches the existing codebase pattern. bootstrap_scan.go correctly excluded — confirmed it has no per-finding formatting loops.
• Style & conventions: Naming (FormatFinding, LogFindings), godoc format, test organization, and API shape all follow established patterns in scanner.go.
• Documentation: No in-repo documentation references the old format patterns. No staleness introduced.

Observations

Severity	Category	Location	Description
low	behavior-change	internal/cli/scan.go:125	The scan input subcommand now displays f.Name (e.g. "aws_key") instead of f.Scanner (e.g. "secret_redactor"). This is an intentional fix authorized by issue #198, but it does change diagnostic output.
low	behavior-change	internal/cli/scan.go:196	The scan output subcommand now includes a [severity] bracket in per-finding output (previously showed only name: detail). This makes the format consistent across all three callers but is a minor output change.
low	dead-code	internal/security/scanner.go	LogFindings is exported and tested but has no callers in this PR or the existing codebase. It was proposed in the issue as a convenience helper — acceptable as a forward-looking API addition, but currently unused.

All observations are low severity — none block merge.

---

Labels: PR modifies internal/security/ scanner package and internal/cli/ scan commands

[guyoron1]

/fs-review