feat: regenerate & document fixture files for UF presenter test suite

[robertolopezlopez]

Description

This PR regenerates and documents UFM presenter live fixtures.

Fixture catalog, workflow

• Add internal/presenters/testdata/ufm/README.md with per-fixture information.
• Link from CONTRIBUTING.md

generate-fixture fix

• Set INTERNAL_SNYK_CLI_USE_UNIFIED_TEST_API_FOR_OS_CLI_TEST=true for test* scans so OSS snyk test emits workflow.TestResult dumps (legacycli path does not).

Live fixture refresh (platform_hammerhead_testing)

• testresult_cli - snyk test . on snyk/cli
• webgoat / webgoat.ignore - snyk test . on OWASP WebGoat Java (shared input; expected output differs by test config)
• Regenerated expected SARIF/human/HTML snapshots for affected presenter cases

Live fixture refresh (platform_hammerhead_testing)

• testresult_cli - snyk test . on snyk/cli
• webgoat / webgoat.ignore - snyk test . on OWASP WebGoat Java (shared input; expected output differs by test config)
• Regenerated expected SARIF/human/HTML snapshots for affected presenter cases

multi_project

• Removed legacy tpwe (.NET) sub-project from committed multi_project.testresult.json and multi_project.sarif.json (personal project, no maintained source checkout).
• Regenerated human-readable expected output for the 6 remaining sub-projects.
• Not live-regenerated in this PR.

Extra

• go-git bump to 5.19.0 because of this
  • testify bump to 1.11.1 is automatic after go mod tidy

Checklist

• Tests added and all succeed (make test)
• Regenerated mocks, etc. (make generate)
• Linted (make lint)
• Test your changes work for the CLI -> chore: bump go-application-framework cli#6945
  1. Clone / pull the latest CLI main.
  2. Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
     • Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
  3. Run go mod tidy in the cliv2 directory.
  4. Run the CLI tests and do any required manual testing.
  5. Open a PR in the CLI repo now with the go.mod and go.sum changes.
  • Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.

[snyk-io]

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

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

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

[snyk-io]

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

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

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

[robertolopezlopez]

/describe

[snyk-pr-review-bot]

Preparing PR description...

[basti-snyk]

Should Fix — this change defangs Test_Findings_ConcurrentAccess. The new pre-loop call testResults[0].Findings(ctx) performs the lazy ReconstructFindings once, single-threaded, and memoizes into fullFindings. All 10 goroutines below then hit the fullFindings != nil fast-return and never exercise the concurrent first-access reconstruction path — which is the exact code this test exists to guard. A future race introduced in lazy reconstruction would now pass clean under -race.

Two fixes, ideally both:

1. Derive expectedCount without warming the object under test — e.g. parse the bytes into a separate NewSerializableTestResultFromBytes instance (or count FindingsData/ProblemRefs), so testResults[0] is still cold when the goroutines first hit it.
2. The old require.Len(..., 47) also guaranteed a non-trivial payload; the dynamic expectedCount passes vacuously if the fixture ever regenerates to 0 findings. Add require.Greater(t, expectedCount, 0) (or restore an absolute count, e.g. 55).

— AI review

[robertolopezlopez]

addressed with latest commit - thanks!

[basti-snyk]

ℹ️ Automated AI review summary — non-blocking. One inline Should Fix posted on the concurrency test. No approval is given; that's left to a human.

Verdict: well-documented, coherent fixture regeneration. Strong positive signal: the regeneration reflects a real behaviour change (switch to OSS unified snyk test, finding count 47→55) and the new fixtures are internally consistent across testresult/human-readable/SARIF (55/55/55), whereas the old fixtures were inconsistent (47/42/42) — evidence this corrected real drift rather than introducing a regression. The new README.md provenance catalog is excellent.

Should Fix (inline)

• Test_Findings_ConcurrentAccess is silently defanged by the pre-loop Findings(ctx) warm-up — the goroutines no longer race the lazy reconstruction path. See inline comment for the two-part fix.

Suggestions (non-blocking)

• scripts/generate-fixture.sh: the FF guard [[ "${SCAN_CMD}" == test* ]] under-matches whitespace/flag-first forms (e.g. " test .") and over-matches testfoo. Matching the first token ([[ "${scan_args[0]}" == "test" ]]) is more robust. The unified-Test-API env var + "equivalent command" are also duplicated across the script, cmd/ufm-fixture-tool/main.go doc, and README — a drift surface; consider a single source.
• README.md row for secrets.with-report reads as self-contradictory ("hand-maintained; do not regenerate via live scan" vs. modelling "REPORT=1 when dumping live secrets"). Worth disambiguating so a contributor doesn't overwrite the synthetic fixture.
• go.mod/go.sum: the bumps (testify, go-git 5.18→5.19 + transitives, x/crypto, x/sys, +cpuid) are benign but unrelated to "regenerate & document fixtures" — worth a line in the description or splitting out. Note: go-git 5.19.0 still carries vulns fixed in 5.19.1 (idxfile/packfile/dotgit), and pre-existing golang.org/x/net html vulns remain — neither introduced by this PR, but a follow-up bump (go-git → 5.19.1, x/net → 0.55.0) would clear them.

— AI review

[basti-snyk]

Should Fix (test coverage) — reachability rendering is no longer exercised by any fixture. Regenerating these inputs via the OSS snyk test path dropped all reachability data: testresult_cli.json went 26→0 reachability entries and webgoat.ignore.testresult.json 47→0, so every Reachability: … line disappeared from the golden output (cli.human.readable 9→0, webgoat.ignore.human.readable 42→0). The UFM presenter still renders reachability, but after this PR no fixture covers that branch — a future regression that breaks reachability rendering would pass the whole UFM golden suite clean.

This is new since my last review (the cli fixture also switched package.json→package-lock.json, which is what dropped the reachability-bearing input).

Suggest either retaining/adding one fixture that carries reachability data so the branch stays covered, or documenting in the README that reachability output is intentionally untested post-CLI-1510 with a follow-up ticket.

— AI review

[basti-snyk]

Suggestion (doc/fixture drift) — testresult_cli target-file claim contradicts the regenerated fixture. This line (and the catalog row at the top) say display-target-file is package.json, but the regenerated cli.human.readable now reports Testing (package-lock.json) — the new commits switched the cli scan to the lockfile. Update the README verify step to package-lock.json so a contributor regenerating doesn't think the run drifted.

— AI review

[basti-snyk]

ℹ️ Automated AI re-review (new commits since my 2026-06-23 pass) — non-blocking. No approval is given; that's left to a human.

Prior round resolved. The three items I flagged last time are addressed by the new commits: the Test_Findings_ConcurrentAccess defang is fixed (count now derived from a separate parse with a > 0 guard), the generate-fixture.sh FF guard now matches on the first token (scan_args[0]), and the dependency bumps landed cleanly (go-git → 5.19.1, x/net → 0.55.0) — a fresh Snyk SCA scan reports 0 vulns across all dependencies.

New since last review:

• Should Fix (test coverage): reachability rendering is no longer exercised by any fixture. The cli scan switched to package-lock.json, which (together with the webgoat.ignore regen) dropped every Reachability: line from the golden output (testresult_cli.json 26→0, webgoat.ignore.testresult.json 47→0; human-readable 9→0 and 42→0). The presenter still renders reachability, so this branch is now untested. Inline comment on cli.human.readable.
• Suggestion (doc drift): README still states testresult_cli display-target-file is package.json, but the regenerated fixture reports package-lock.json. Inline comment on README.md.

Other suggestions (non-blocking):

• Golden inputs and expected outputs were regenerated in the same pass with no presenter code change, so the new outputs are self-consistent by construction rather than independently verified — a quick spot-check of a few regenerated findings against the live snyk test console output (noted in the PR) would close that gap.
• README.md forward-references a python_pins fixture that doesn't exist yet ("Planned — Phase 2"); consider trimming until it lands.
• multi_project is documented as not reproducible via the documented recipe (hand-removed tpwe); a one-line "hand-maintained — do not live-regen" warning at the recipe site would prevent accidental overwrite.

— AI review

[basti-snyk]

Suggestion (non-blocking): this export runs before the PROJECT/ORG/NAME validation block at lines 32-42. If the script is sourced (. ./generate-fixture.sh) rather than executed, and validation exits early, INTERNAL_SNYK_CLI_USE_UNIFIED_TEST_API_FOR_OS_CLI_TEST=true leaks into the caller's interactive shell and silently affects later manual snyk runs. Moving the export to after the validation block (it's only needed at scan time) avoids the leak. In the normal executed-as-subprocess path this is harmless. — AI review

[basti-snyk]

ℹ️ Automated AI re-review summary — non-blocking. HEAD moved since the prior AI review, so this pass re-ran the four reviewer lenses against the new head.

Verdict: no Critical or Should-Fix blockers. The change is fixture regeneration + docs + a tooling/script change + routine dependency bumps. The human-readable golden test stays byte-exact and the SARIF comparison stays JSONEq with principled normalization, so coverage is not weakened in a way that loses meaning. The generate-fixture.sh feature-flag switch is the correct root-cause fix (only the unified Test API path emits workflow.TestResult).

New finding this pass: one inline Suggestion on scripts/generate-fixture.sh (env-var leak if the script is sourced and validation exits early).

Previously-posted finding still stands (not reposted): the concurrency test now derives the expected finding count from a second decode of the same fixture instead of the prior literal 47. This is no longer vuln-DB-fragile and still detects the race it targets, but it's a weaker count oracle — a deterministic wrong-but-nonzero regression in Findings() wouldn't be caught. Optional: assert the derived count against an independent constant.

Security: Snyk SCA clean (0 vulns; all dep bumps are forward upgrades). Snyk Code surfaced only a note-level path-traversal on the local dev ufm-fixture-tool (explicit CLI flags, not exploitable). No secrets in any changed file.

No approval is given; that's left to a human. — AI review

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Brittle Command Detection 🟡 [minor] The is_oss_test_scan function relies on the first word of SCAN_CMD being exactly 'test'. If SCAN_CMD is provided with leading environment variables (e.g., DEBUG=1 test .) or if the user uses an absolute path for a command, the check will fail. This prevents the required INTERNAL_SNYK_CLI_USE_UNIFIED_TEST_API_FOR_OS_CLI_TEST variable from being exported for OSS scans in those scenarios. is_oss_test_scan() { [[ "${scan_args[0]:-}" == "test" ]] } Weakened Test Assertion 🟡 [minor] In Test_Findings_ConcurrentAccess, replacing the hardcoded count (47) with a dynamic expectedCount derived from a fresh Findings() call within the same test execution reduces the test's ability to detect regression in the initial finding reconstruction. If expectedResults[0].Findings(ctx) itself returns an incorrect count due to a logic bug in json_test_result.go, the subsequent concurrent tests will validate against that incorrect baseline, merely proving consistency but not correctness. expectedResults, err := NewSerializableTestResultFromBytes(testResultsData) require.NoError(t, err) require.Len(t, expectedResults, 1) expectedFindings, complete, err := expectedResults[0].Findings(ctx) require.NoError(t, err) require.True(t, complete) expectedCount := len(expectedFindings) require.Greater(t, expectedCount, 0)

📚 Repository Context Analyzed This review considered 23 relevant code sections from 13 files (average relevance: 0.81) README.md (lines 1-48) internal/presenters/presenter_ufm.go (lines 1-234) pkg/utils/sarif/sarif.go (2 segments, lines 1-248) pkg/utils/ufm/json_test_result.go (2 segments, lines 1-207) internal/presenters/funcs.go (lines 1-251) pkg/local_workflows/connectivity_check_extension/connectivity/types.go (lines 1-192) pkg/local_workflows/ignore_workflow/ignore_workflow.go (lines 1-98) ... and 6 more file(s)

[CatalinSnyk]

issue: I think I understand what the intention of this was, but it's asserting the same thing against each other 😅 since just above testResults is going the same function call

	testResults, err := NewSerializableTestResultFromBytes(testResultsData)