refactor(attest): record attestations through one attestation submitter

[dangrondahl]

Every attest command repeated the same steps for sending an attestation: build the URL, build the form, tar the evidence and delete the tarball afterwards, handle dry-run, log success, join the --assert failures, and reword the server's error. This PR moves those steps into one attestationSubmitter (cmd/kosli/attestationSubmit.go). Each command now just builds its payload and calls submit(...).

It is a pure refactor: output and request bodies should be unchanged.

Migrated: generic, custom, decision, junit, snyk, sonar, jira, sbom, and the four attest pullrequest commands.

Not migrated:

• attest override, which posts JSON rather than a form.
• attest artifact, which uses a different endpoint.

Removed and moved:

• prepareAttestationForm was a pass-through and is deleted, along with newAttestationForm.
• getPathOfEvidenceFileToUpload and wrapAttestationError moved out of cli_utils.go and attestation.go into the new file.

sbom: it now hands over its already-read bytes in a new evidence field and no longer builds its own form. The guarantee that the uploaded bytes are exactly the bytes that were hashed still holds and is still tested.

Testing

• 12 new module tests run against an httptest server. They check:
  • the URL, and the data_json and attachment_file parts
  • that the tarball is deleted after both a successful and a rejected request
  • the success message, and that dry-run sends nothing
  • that the "requires at least one of…" server error is reworded
  • that assert failures are joined in order, also after a server error, and dropped in dry-run
  • that sbom's in-memory evidence is uploaded
• I did not run the live-server suites locally because no local server was available, so they are left to CI.
• Instead, I built main and this branch and diffed their output for 15 commands. Each ran in dry-run, against a stub that returns 201, and against a stub that returns 400. All 45 runs matched byte for byte:
  • The commands covered generic (with and without attachments), custom, decision, junit, snyk, jira (± --assert), github/gitlab PR (± --assert), sbom (CycloneDX, SPDX, invalid file) and a missing attachment.
  • The PR commands were pointed at a stub GitHub/GitLab.

Known differences from main

These only affect error paths:

• An invalid --host is now reported after the payload is resolved, not before.
• On the PR commands, a bad --attachments now fails after the "found N pull request(s)" line, not before.
• The rare warning when the evidence tarball can't be deleted now has the same wording in every command. Some commands used to include the path in it.

Checklist

• Docs are autogenerated from CLI help — make sure the help text reflects your changes (no help text changes)
• Helm chart (charts/k8s-reporter/) updated, if needed. Note: these changes live in a separate PR (n/a)
• Terraform provider and related changes (terraform-provider-kosli, terraform-aws-evidence-reporter, terraform-aws-kosli-reporter) updated, if needed (n/a)

[claude]

Claude finished @dangrondahl's task in 2m 25s —— View job

---

Review of PR #1243 — attestationSubmitter refactor

• Static analysis (build, vet, mod tidy)
• Review diff
• Post findings

Static analysis

Check	Result
go build ./...	✅ passes
go vet ./cmd/... ./internal/...	✅ passes
go mod tidy + git diff --exit-code go.mod go.sum	✅ no changes

I couldn't run go test in this environment because it needs approval. The new unit tests are left to CI.

Findings

I found no bugs or regressions. I checked each migrated command against main line by line:

• Success messages are unchanged. That includes custom:<type> and <provider> <label>.
• URL slugs are unchanged. decision and sbom still go to system.
• jira --assert errors keep their order and their \nError: join: "no references" comes before "not found/unconfirmed".
• PR --assert errors keep their order and join in the same way.
• The server-error rewording still covers assert errors. wrapAttestationError runs on the joined error, as it did before.
• Dry-run still drops assert failures.

One nit, posted inline:

• cmd/kosli/attestSbom.go:265: the doc comment on evidence() was only partly reflowed. The first line is about 130 columns long. There is a suggestion block on the comment.

Optional, not blocking:

• attestationSubmitter.form covers three different evidence sources: in-memory, none, and file paths. That is easy to follow now. If a fourth source ever comes along, a small evidenceSource interface would read better than more branches. You don't need it yet.
• Three earlier review points are fixed in 2595236, and the fixes are pinned by tests:
  • the staging directory is now removed when an attachment copy fails
  • the logger is now passed in rather than taken from a global
  • form now rejects evidence and attachments together

What's good

• Clean seam. The submitter takes its client, host, org, token, dry-run flag and logger as fields, and newAttestationSubmitter() is the only place that reads globals. That makes the httptest tests possible without a live server.
• Assert failures are values. assertFailures []error replaces two copies of the errString string building. It also makes the rule "record first, then fail" explicit and testable.
• The sbom guarantee still holds. evidence() still takes the bytes as a parameter, so the bytes that are uploaded are always the bytes that were hashed. TestSbomUploadsTheBytesItHashed still checks this.
• The tests are well targeted. Tarball cleanup is checked after both a success and a server rejection, and the staging directory is checked after a failed copy. The fake server's handler also avoids require off the test goroutine.
• Thorough testing for a pure refactor. You compared 45 command outputs between main and this branch, and listed the known differences on error paths up front.

I'd be happy to see this merged once CI is green.

[claude]

Follow-up review of latest commit (2595236)

All three previously raised findings are resolved:

1. tmpDir defer / logger DI consistency — resolved. The RemoveAll(tmpDir) defer now runs right after MkdirTemp, and getPathOfEvidenceFileToUpload now takes logger *log.Logger as a parameter, so it goes through the submitter's injected logger seam.
2. require.* on the handler goroutine — resolved. The fake server now uses assert.NoError plus http.Error(...); return.
3. Silent attachment drop when evidence is also set — resolved. form now rejects evidence+attachments combined, pinned by TestAttestationSubmitterRejectsEvidenceTogetherWithAttachments.

No new bugs/regressions found in the changed lines (cmd/kosli/attestationSubmit.go, cmd/kosli/attestationSubmit_test.go, cmd/kosli/cli_utils_test.go).

[dangrondahl]

Thanks /improve-codebase-architecture 🙌