Use ValidateVSAAndComparePolicy for ec validate image VSA skip

[st3penta]

IsValidVSA only checked Found && !Expired, so forged VSAs could skip
the entire validation pipeline (EC-1842). ValidateVSAAndComparePolicy
already handles this for ec validate vsa: it verifies signatures,
checks predicate status, and compares policy. Wire it into the image
path too.

Add --vsa-public-key to ec validate image, required when --vsa-upload
is set. Change ValidateImageWithVSACheck to take *vsa.VSAValidationConfig
instead of *vsa.VSAChecker + time.Duration.

Breaking change: --vsa-upload users need to also pass --vsa-public-key.
Default Konflux pipelines are unaffected.

Ref: https://issues.redhat.com/browse/EC-1998

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The validate image command now requires a VSA public key for VSA uploads. VSA-aware image validation uses VSAValidationConfig for signature, predicate, policy, expiration, and effective-time checks, with fallback to normal validation when VSA validation does not pass.

Changes

VSA validation flow

Layer / File(s)	Summary
CLI VSA configuration cmd/validate/image.go, cmd/validate/image_test.go	The command adds --vsa-public-key, stores the key path, rejects VSA uploads without it, and supplies complete VSA settings. Tests cover the required option and updated VSA upload invocations.
Image validation and fallback internal/image/validate.go, internal/image/validate_test.go	ValidateImageWithVSACheck accepts VSAValidationConfig, performs comprehensive VSA validation, skips normal validation after success, and falls back after failed, missing, or unavailable VSA data. Tests cover these outcomes.
Acceptance scenarios and documentation features/vsa.feature, docs/modules/ROOT/pages/ec_validate_image.adoc	The documentation and VSA scenarios include the required public key across storage, expiration, backend, and failure cases.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant ValidateImageWithVSACheck
  participant UploadBackedRetriever
  participant ValidateVSAAndComparePolicy
  participant ImageValidation
  CLI->>ValidateImageWithVSACheck: pass VSAValidationConfig
  ValidateImageWithVSACheck->>UploadBackedRetriever: retrieve VSA envelope
  UploadBackedRetriever-->>ValidateImageWithVSACheck: return envelope or retrieval error
  ValidateImageWithVSACheck->>ValidateVSAAndComparePolicy: validate VSA
  ValidateVSAAndComparePolicy-->>ValidateImageWithVSACheck: return validation result
  ValidateImageWithVSACheck->>ImageValidation: continue when VSA validation fails or is unavailable

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: using ValidateVSAAndComparePolicy to control VSA-based validation skipping for ec validate image.
Description check	✅ Passed	The description explains what and why, documents the breaking change, and includes a related Jira ticket reference.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:00 AM UTC · Ended 9:09 AM UTC

Commit: 87c4a29 · View workflow run →

[qodo-for-conforma]

PR Summary by Qodo

Harden ec validate image VSA skip path with full VSA verification

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Replace weak VSA skip check with signature, predicate, and policy-equivalence validation.
• Add --vsa-public-key and require it when --vsa-upload is used.
• Update unit tests to cover the new flag requirement and skip-path behavior.

Diagram

graph TD
  A(["ec validate image"]) --> B["Build VSAValidationConfig"] --> C["ValidateImageWithVSACheck"] --> D["ValidateVSAAndComparePolicy"] --> E{"VSA passed?"}
  E -->|"yes"| F["Skip full validation"]
  E -->|"no / error"| G["Run full image validation"]

  subgraph Legend
    direction LR
    _cli(["CLI entry"]) ~~~ _step["Processing step"] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make signature verification optional via explicit insecure flag

• ➕ Avoids breaking change for existing --vsa-upload users who don’t have a public key handy
• ➕ Keeps current behavior available for local/dev workflows
• ➖ Reintroduces a foot-gun on a security boundary (skip path)
• ➖ More surface area to document and test; higher chance of misconfiguration

2. Call `VSAChecker.CheckExistingVSAWithVerification` directly from image path

• ➕ Avoids duplicating/propagating VSAValidationConfig fields that image doesn’t strictly need
• ➖ Diverges from the already-established validate vsa implementation path
• ➖ Harder to keep policy-equivalence logic consistent across commands

Recommendation: Current approach is the best default: it reuses the existing ValidateVSAAndComparePolicy logic (already used by ec validate vsa) to harden a security-sensitive skip path. Requiring --vsa-public-key when --vsa-upload is set is an appropriate breaking change because skipping validation without signature verification is unsafe; if a softer migration is needed later, consider an explicit opt-in insecure flag rather than implicit behavior.

Files changed (4) +158 / -100

Bug fix (2) +32 / -16

image.goRequire VSA public key and wire full VSA validation into skip path +17/-3

Require VSA public key and wire full VSA validation into skip path

• Adds '--vsa-public-key' flag and enforces it when '--vsa-upload' is provided. Replaces the old VSA checker/expiration parameters with a 'VSAValidationConfig' and routes the skip decision through full VSA validation (signature, predicate, and policy equivalence).

cmd/validate/image.go

validate.goReplace 'IsValidVSA' skip check with 'ValidateVSAAndComparePolicy' +15/-13

Replace 'IsValidVSA' skip check with 'ValidateVSAAndComparePolicy'

• Changes 'ValidateImageWithVSACheck' to accept '*vsa.VSAValidationConfig' and use 'ValidateVSAAndComparePolicy' to decide whether to skip validation. Improves logging to surface signature verification and predicate outcomes when skipping or falling back.

internal/image/validate.go

Tests (2) +126 / -84

image_test.goUpdate CLI tests for new '--vsa-public-key' requirement +45/-0

Update CLI tests for new '--vsa-public-key' requirement

• Updates existing VSA upload/format tests to pass the new flag. Adds a dedicated test asserting '--vsa-public-key' is required when '--vsa-upload' is set.

cmd/validate/image_test.go

validate_test.goRewrite VSA skip-path unit tests around 'VSAValidationConfig' results +81/-84

Rewrite VSA skip-path unit tests around 'VSAValidationConfig' results

• Replaces the old mock checker approach with a mock retriever feeding DSSE envelopes representing passing/failing predicates. Adds coverage for skip-on-pass and fallback behavior when VSA retrieval/validation fails.

internal/image/validate_test.go

[qodo-for-conforma]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. validate_test.go build tag not first ✗ Dismissed 📜 Skill insight ≡ Correctness

Description

internal/image/validate_test.go is a modified _test.go file but its first line is a copyright
header, not a //go:build ... tag, violating the required build-tag-on-first-line convention.

Code

internal/image/validate_test.go[R430-433]

+type mockVSARetriever struct {
+	envelope *ssldsse.Envelope
+	err      error
+}

Relevance

●●● Strong

Build-tag-first-line compliance for unit tests; repository has recent unit-tag related enforcement
changes.

PR-#2698

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 3743 requires the first line of any new or modified Go test file to be a build tag.
In internal/image/validate_test.go, the first line is a copyright comment and the `//go:build
unit` tag appears later, so it does not meet the rule.

internal/image/validate_test.go[1-20]
Skill: pr-checklist

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Go compliance requires that any modified `*_test.go` file has a build tag on the **first line** (e.g., `//go:build unit`). `internal/image/validate_test.go` currently has `//go:build unit` at line 17, after the license header.

## Issue Context
The file was modified in this PR (new VSA config/envelope helpers and updated tests), so it must conform to the build-tag-on-first-line rule.

## Fix Focus Areas
- internal/image/validate_test.go[1-20]
- internal/image/validate_test.go[430-447]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. image_test.go build tag not first ✗ Dismissed 📜 Skill insight ≡ Correctness

Description

cmd/validate/image_test.go is a modified _test.go file but its first line is a copyright header,
not a //go:build ... tag, so it will fail the required build-tag-on-first-line convention.

Code

cmd/validate/image_test.go[R1547-1550]

+func TestValidateImageCommand_VSAPublicKeyRequired(t *testing.T) {
+	// --vsa-public-key is required when --vsa-upload is set
+	validateImageCmd := validateImageCmd(happyValidator())
+	cmd := setUpCobra(validateImageCmd)

Relevance

●●● Strong

Build-tag-first-line is a hard compliance rule; test file hygiene changes are typically accepted.

PR-#2678
PR-#2698

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 3743 requires the first line of any new or modified Go test file to be a build tag.
In cmd/validate/image_test.go, the first line is a copyright comment and the //go:build unit tag
appears later, so it does not meet the rule.

cmd/validate/image_test.go[1-20]
Skill: pr-checklist

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Go compliance requires that any modified `*_test.go` file has a build tag on the **first line** (e.g., `//go:build unit`). `cmd/validate/image_test.go` currently has `//go:build unit` at line 17, after the license header.

## Issue Context
The file was modified in this PR (new test added), so it must conform to the build-tag-on-first-line rule.

## Fix Focus Areas
- cmd/validate/image_test.go[1-20]
- cmd/validate/image_test.go[1547-1550]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Nil vsaConfig panic ✗ Dismissed 🐞 Bug ☼ Reliability

Description

ValidateImageWithVSACheck dereferences vsaConfig.VSAExpiration in trace logging before any nil
check, so a nil vsaConfig will panic when tracing is enabled. The downstream validator already
returns a clean error for nil validation config, but this dereference prevents that path.

Code

internal/image/validate.go[180]

+		trace.Logf(ctx, "", "image=%q vsa-expiration=%v", comp.ContainerImage, vsaConfig.VSAExpiration)

Relevance

●●● Strong

Deterministic nil-deref panic risk; similar reliability/panic-hardening changes have been accepted.

PR-#3386

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new trace log line directly dereferences vsaConfig, while the validator explicitly treats nil
config as an error—showing the deref is unnecessary and creates a panic risk.

internal/image/validate.go[173-184]
internal/validate/vsa/validator.go[42-49]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`internal/image.ValidateImageWithVSACheck` uses `vsaConfig.VSAExpiration` in a trace log message without checking `vsaConfig != nil`. If a caller ever passes nil (even accidentally), tracing-enabled runs will panic.

## Issue Context
`vsa.ValidateVSAAndComparePolicy` already returns an error when passed nil config, so `ValidateImageWithVSACheck` can fail gracefully if it validates `vsaConfig` first.

## Fix Focus Areas
- internal/image/validate.go[176-185]

### Suggested approach
- Add an early guard:
 - if `vsaConfig == nil`, return `ValidateImage(...)` (fallback) or return a clear error (depending on intended contract).
- Move the trace.Logf to after the nil guard, or log a nil-safe value.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Attestation time disables skip ✓ Resolved 🐞 Bug ≡ Correctness

Description

ec validate image accepts --effective-time=attestation, but the new VSA skip path forwards that
string to vsa.ValidateVSAAndComparePolicy, whose effective-time parser only supports now or
RFC3339. As a result, policy-equivalence checking never passes for attestation, so VSA-based
skipping is effectively disabled (and logs will misleadingly report “invalid effective time”).

Code

cmd/validate/image.go[R379-382]

+								VSAExpiration: data.vsaExpiration,
+								PublicKeyPath: data.vsaPublicKey,
+								PolicySpec:    data.policy.Spec(),
+								EffectiveTime: data.effectiveTime,

Relevance

●●● Strong

VSA skip path correctness bug; team historically accepts VSA validation/flag logic fixes.

PR-#2690
PR-#3080

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The validate-image CLI explicitly supports attestation as an --effective-time value, but VSA
validation’s ParseEffectiveTime does not; since ValidateVSAAndComparePolicy calls that parser
during policy comparison, an attestation value prevents a passing result and therefore prevents
skipping.

cmd/validate/image.go[556-561]
cmd/validate/image.go[371-384]
internal/validate/vsa/validator.go[123-135]
internal/validate/vsa/validator.go[249-257]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`validate image` supports `--effective-time` values like `attestation`, but the VSA policy-equivalence path only parses `now` or RFC3339. When `attestation` is used, `ValidateVSAAndComparePolicy` treats it as invalid and returns a non-passing result, preventing the intended “skip validation if VSA is valid & policy-equivalent” optimization.

## Issue Context
- `cmd/validate/image.go` documents and uses `attestation` for effective time.
- `internal/validate/vsa/validator.go` has its own `ParseEffectiveTime` with different semantics.

## Fix Focus Areas
- cmd/validate/image.go[371-384]
- cmd/validate/image.go[556-561]
- internal/validate/vsa/validator.go[123-135]
- internal/validate/vsa/validator.go[249-257]

### Suggested approach
- Reuse `internal/policy.ParseEffectiveTime(...)` semantics (supports `now`, `attestation`, RFC3339, and YYYY-MM-DD) or otherwise normalize `data.effectiveTime` before passing into VSA validation.
- If `attestation` cannot be resolved safely in the skip path, handle it explicitly (e.g., skip policy comparison with a clear reason code/message) instead of treating it as “invalid effective time”.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

✅ Compliance rules (platform): 36 rules

✅ Skills: build-and-lint, pr-checklist, write-tests

Tip of the day

💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/image/validate_test.go`:
- Around line 451-457: Update createPassingVSAEnvelope to define a PolicySpec
with the expected source and policy, assign it to the returned validation data,
and build the predicate.policy from that same specification so the passing test
exercises policy comparison.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: e34f50d2-f28d-4e42-9640-91153a2911eb

📥 Commits

Reviewing files that changed from the base of the PR and between 0250ec5 and 9880bdf.

📒 Files selected for processing (4)

• cmd/validate/image.go
• cmd/validate/image_test.go
• internal/image/validate.go
• internal/image/validate_test.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:10 AM UTC · Completed 9:27 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

Review

Findings

High

• [breaking-cli] cmd/validate/image.go:299 — Breaking CLI change: --vsa-upload with default --vsa-expiration (168h, non-zero) now requires --vsa-public-key. Any existing CI pipeline, script, or external integration that invokes ec validate image --vsa-upload <backend> without --vsa-public-key will fail with a new error. The security motivation is sound (EC-1842), but the breaking change needs release notes and consideration of whether a deprecation warning for one release cycle would reduce external consumer breakage.
  Remediation: Document the breaking change in release notes. Consider whether a deprecation warning (log warning and proceed without key) for one release cycle would reduce breakage, or confirm the hard break is intentional given the security severity.

• [missing-doc] THREAT_MODEL.md:97 — The CLI arguments table in section 3.1 lists --vsa-signing-key and --vsa-upload as ec validate image flags but does not include the new --vsa-public-key flag. This flag is security-relevant: it controls VSA signature verification for the validation-skip path and is required when --vsa-upload is set with non-zero expiration.
  Remediation: Add a row to the section 3.1 table: --vsa-public-key | Operator-controlled | Public key for VSA signature verification. Required when --vsa-upload is set and --vsa-expiration > 0.

Medium

• [stale-doc] internal/validate/vsa/DESIGN.md — The VSA DESIGN.md describes only the expiration-based skip model but does not mention the validation steps introduced in this PR: signature verification via public key and policy equivalence comparison. The description of when validation is skipped is now incomplete.
  Remediation: Update DESIGN.md to describe that VSA validation now includes signature verification (via --vsa-public-key) and policy comparison, in addition to expiration checking, before a VSA can be used to skip re-validation.

• [breaking-config] tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml:469 — The Konflux task avoids the new --vsa-public-key requirement by adding --vsa-expiration=0, which disables VSA skip optimization entirely. Downstream Konflux pipeline consumers who previously relied on the default 168h VSA expiration to skip redundant validations lose that optimization.
  Remediation: Consider adding an optional VSA_PUBLIC_KEY parameter to the Tekton task so pipeline owners who want VSA skip optimization can opt in by providing a public key.

Low

• [test-inadequate] internal/image/validate_test.go:480 — The 'VSA passed - skip validation' test sets IgnoreSignatureVerification: true, but production always uses the Go zero value (false), meaning signature verification is always enabled at runtime. The acceptance tests in features/vsa.feature exercise the full path, but a unit test with IgnoreSignatureVerification: false and an invalid-signature envelope would improve coverage of the security fix at the unit level.

• [code-organization] cmd/validate/image.go:299 — The new validation block has a comment (// Require --vsa-public-key when the VSA skip path is active) but sits outside the if data.vsaEnabled guard without a section comment explaining the separation. Adding a section comment would clarify that vsaEnabled controls VSA output while this check controls the VSA input/skip path.

• [error-handling-idiom] cmd/validate/image.go:299 — Minor: the error message uses "with" (--vsa-upload is set with --vsa-expiration > 0) where "and" would be more precise for the conjunction between two independent conditions.

• [cli-documentation] cmd/validate/image.go:601 — The --vsa-public-key flag help text accurately states the technical condition, but since --vsa-expiration defaults to 168h (non-zero), the flag is effectively required whenever --vsa-upload is used unless users explicitly set --vsa-expiration=0.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run

Review

Findings

High

• [missing-docs] THREAT_MODEL.md:97 — Section 3.1 lists --vsa-signing-key and --vsa-upload as security-relevant flags but does not include the new --vsa-public-key flag. This flag controls the public key used for VSA signature verification during the validation-skip path and is security-critical.
  Remediation: Add --vsa-public-key to the section 3.1 table with description: "Public key used to verify VSA signatures during the validation-skip path. Required when --vsa-upload is set and --vsa-expiration > 0. (ec validate image only)."

• [CLI breaking change] cmd/validate/image.go:299 — The new validation makes --vsa-public-key mandatory when --vsa-upload is set and --vsa-expiration > 0 (default 168h). Any existing script or CI that invokes ec validate image --vsa-upload <backend> without --vsa-public-key will now fail. The PR body acknowledges this and the Tekton task is updated with --vsa-expiration=0 to mitigate.
  Remediation: Ensure release notes clearly document that --vsa-upload users must now also pass --vsa-public-key (or set --vsa-expiration=0 to disable the skip path). Verify all known downstream CI configurations are updated.

Medium

• [stale-docs] internal/validate/vsa/DESIGN.md:21 — DESIGN.md states "The signing key is the same key used for the original image validation." This PR introduces a separate --vsa-public-key flag, allowing a different key for VSA signature verification than the image validation key. The design doc's claim is now misleading.
  Remediation: Update the DSSE Signing section to note that VSA signing and verification keys can be configured independently from the image signing key.

• [Tekton task contract] tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml:470 — The Tekton task adds --vsa-expiration=0 inside the DSSE format conditional, which means Konflux pipelines will never skip validation based on existing VSAs. This is a behavioral change for any deployment that relied on the default 168h VSA caching.
  Remediation: Confirm Konflux pipeline consumers do not rely on VSA skip behavior. Document the change in the task changelog.

Low

• [missing-docs] THREAT_MODEL.md:209 — Threat CA-6 ("VSA-based validation skip") mitigation description now understates the actual security posture. The PR adds mandatory signature verification and policy equivalence checking, but the threat model only mentions "DSSE signing is mandatory" and "configurable expiration."

• [test adequacy] internal/image/validate_test.go:477 — No test case covers the scenario where ValidateVSAAndComparePolicy returns an error from signature verification failure with a real envelope. The existing tests cover retriever errors but not validation-level signature failures.

• [Tekton task contract - missing parameter] tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml:470 — The Tekton task does not expose a VSA_PUBLIC_KEY parameter. It sidesteps the requirement by hardcoding --vsa-expiration=0, but this limits future opt-in to VSA signature verification without a task version bump.

• [logic error] cmd/validate/image.go:299 — The --vsa-public-key validation fires for all --vsa-upload users with non-zero expiration, including generation-only users who may not need the skip path. This is intentional (the PR body documents it as a breaking change) and the Tekton task mitigates with --vsa-expiration=0.

• [edge case] internal/image/validate_test.go:465 — The "VSA passed - skip validation" test sets IgnoreSignatureVerification: true, so it does not exercise the full production signature verification path where IgnoreSignatureVerification defaults to false.

• [scope creep] features/__snapshots__/validate_image.snap — Deletion of 1152 lines of orphaned snapshot data (for the commented-out "many components and sources" scenario, EC-1023) is unrelated to the VSA security fix.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (2)

Review

Findings

High

• [breaking-cli] cmd/validate/image.go:299 — Breaking CLI contract: existing users of --vsa-upload without --vsa-public-key will now get an error when --vsa-expiration > 0 (the default is 168h). Any automation or CI pipeline invoking ec validate image --vsa-upload <backend> without also passing --vsa-public-key will fail. The PR description acknowledges this breaking change and the Konflux task is updated with --vsa-expiration=0 to avoid the break, but external consumers are not covered.
  Remediation: Ensure all known downstream consumers are updated simultaneously. Document this in release notes as a breaking change. Consider whether a deprecation warning (proceeding without signature verification) would be appropriate for a transition period, though note this would leave the EC-1842 security hole open during the transition.

Medium

• [fail-open] internal/validate/vsa/validator.go:99 — The predicate status check uses predicateStatus != "" && predicateStatus != "passed", which means an empty or missing Status field in the VSA predicate is treated the same as "passed" — the function continues to policy comparison and can return Passed: true. This is a pre-existing issue in ValidateVSAAndComparePolicy (not introduced by this PR), but this PR extends its attack surface to the image path. Mitigated by mandatory signature verification in the image path and policy comparison.
  Remediation: Treat an empty predicateStatus as a validation failure. Change the condition to: if predicateStatus != "passed" { return fail }.

• [breaking-config] tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml:470 — Adding --vsa-expiration=0 disables the VSA skip path entirely in Konflux pipelines. Previously with --vsa-upload set, the default --vsa-expiration=168h meant an existing valid VSA could cause validation to be skipped. Now full validation runs every time. This is intentional per the PR description but is a behavioral change to the task's 0.1 version.

Low

• [edge-case] cmd/validate/image.go:298 — The validation condition and the runtime gate both use > 0 for vsaExpiration, which is currently consistent. However, IsVSAExpired treats a zero threshold as "never expires". Adding a comment documenting why > 0 is the correct threshold and its relationship to the runtime gate would prevent future maintenance errors.

• [naming-convention] cmd/validate/image.go:684 — The new struct field vsaPublicKey holds a file path but omits Path from the name. The analogous field in cmd/validate/vsa.go is publicKeyPath, and the VSAValidationConfig struct uses PublicKeyPath. Consider renaming to vsaPublicKeyPath for consistency.

• [error-handling-idiom] internal/image/validate.go:188 — When ValidateVSAAndComparePolicy returns an error, the error is logged as a warning and validation proceeds with full image validation. This fail-open pattern is the same as the pre-existing code and is actually more secure (runs full validation rather than blocking), but signature verification failures are silently demoted to warnings.

• [fail-open] internal/validate/vsa/validator.go:124 — Policy comparison is gated by if len(data.PolicySpec.Sources) > 0. If PolicySpec has no sources, the check is skipped and Passed: true is returned. In the image path, the policy is populated from data.policy.Spec() which goes through PreProcessPolicy, so empty sources are unlikely in practice.

• [code-organization] cmd/validate/image.go:378 — The mapping if vsaEffectiveTime == "attestation" { vsaEffectiveTime = policy.Now } is introduced inline without a comment explaining why "attestation" maps to "now" for VSA validation. The reasoning (attestation-relative time is not meaningful for VSA policy comparison) is non-obvious.

• [stale-doc] THREAT_MODEL.md:97 — Section 3.1 lists --vsa-signing-key and --vsa-upload as ec validate image entry points but does not list the new --vsa-public-key flag.

• [stale-doc] THREAT_MODEL.md:209 — Threat CA-6 mitigation text describes only DSSE signing and expiration. After this PR, the VSA skip path also performs signature verification, predicate status checking, and policy comparison.

• [stale-doc] .cursor/rules/vsa_functionality.mdc:336 — References IsValidVSA as the primary method for checking VSA validity. The image validation path now uses ValidateVSAAndComparePolicy.

• [stale-doc] .cursor/rules/vsa_functionality.mdc:345 — References ValidateVSAWithPolicyComparison and ValidationData struct, which have been renamed to ValidateVSAAndComparePolicy and VSAValidationConfig.

• [breaking-cli] features/__snapshots__/vsa.snap:50 — Warning log message format changed from Failed to check for existing VSA... to Failed to validate existing VSA.... Log messages are not a formal API contract but downstream log parsing could be affected.

---

Labels: PR is a security fix with a breaking CLI change affecting VSA validation. 'bug' reflects the security vulnerability fix (EC-1842). 'major' reflects the documented breaking change to --vsa-upload requiring --vsa-public-key.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (3)

Review

Findings

High

• [breaking-cli] cmd/validate/image.go:296 — New mandatory --vsa-public-key flag required when --vsa-upload is set with --vsa-expiration > 0. Since --vsa-expiration defaults to 168h (non-zero), any existing caller passing --vsa-upload without --vsa-public-key gets a hard error. The PR body acknowledges this breaking change.
  Remediation: Ensure changelogs/release notes clearly document this breaking change and the migration path (--vsa-upload users must add --vsa-public-key).

Medium

• [missing-documentation] THREAT_MODEL.md:97 — Section 3.1 lists --vsa-signing-key and --vsa-upload as ec validate image flags but does not include the new --vsa-public-key flag. This security-relevant flag provides the public key for VSA signature verification.
  Remediation: Add a row to the section 3.1 table for --vsa-public-key.

• [stale-doc] THREAT_MODEL.md:209 — Threat CA-6 (VSA-based validation skip) states mitigation is "DSSE signing is mandatory for VSAs. Configurable expiration." This PR adds signature verification, predicate status checking, and policy equivalence comparison — the mitigation description no longer reflects the improved security posture.
  Remediation: Update CA-6 mitigation to include the new verification steps.

• [stale-doc] internal/validate/vsa/DESIGN.md:6 — Purpose section describes VSA skip behavior based on expiration only. The PR now also verifies signature, checks predicate status, and compares policy. The documented behavior no longer matches the implementation.
  Remediation: Update to mention signature verification, predicate status checking, and policy equivalence.

• [missing-doc] docs/modules/ROOT/pages/types-of-attestations-and-manifests.adoc:60 — The VSA section lists --vsa, --vsa-signing-key, and --vsa-upload but does not mention --vsa-public-key. Users following this documentation would not know about the new requirement.
  Remediation: Add a bullet point about the --vsa-public-key requirement.

Low

• [test-inadequate] internal/image/validate_test.go:574 — TestValidateImageWithVSACheck_FlagCombinations discards the *output.Output return value and only asserts NoError. Should assert output is non-nil to confirm full validation ran rather than the VSA skip path returning (nil, nil).
  Remediation: Change _, err = ... to out, err := ... and add assert.NotNil(t, out).

• [scope-mismatch] features/__snapshots__/validate_image.snap — 1152 lines of "many components and sources" test snapshot removed. Confirm this removal is caused by behavioral changes in this PR rather than unrelated cleanup.

• [missing-authorization] No linked GitHub issue. The PR references external Jira EC-1998 and EC-1842 but traceability relies on an external issue tracker.

• [scope-creep] tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml:470 — The --vsa-expiration=0 addition disables the VSA skip path for Konflux. This is a reasonable companion change but the rationale could be more explicitly documented.

• [breaking-cli] tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml:470 — The task does not add a VSA_PUBLIC_KEY parameter. If VSA skip is later desired in Konflux, a new parameter will be needed.

---

Labels: PR introduces breaking CLI change and has multiple documentation gaps for a security-relevant feature

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (4)

Review

Findings

Low

• [help-text-inconsistency] cmd/validate/image.go:599 — The --vsa-public-key flag help text says "Required when --vsa-upload is set" but the actual enforcement gate also requires --vsa-expiration > 0. Since --vsa-expiration defaults to 168h, the help text is practically correct for default usage, but could confuse users who explicitly set --vsa-expiration=0. Consider updating to: "Required when --vsa-upload is set and --vsa-expiration is greater than zero."

• [style-consistency] internal/image/validate_test.go — Test helpers createPassingVSAEnvelope() and createFailedVSAEnvelope() build JSON via string concatenation rather than struct marshaling. Minor style deviation; the concatenated strings only contain RFC3339 timestamps which do not require JSON escaping, so the fragility risk is low.

Previous run (5)

Review

Findings

High

• [breaking-cli] cmd/validate/image.go:299 — The new validation requiring --vsa-public-key when --vsa-upload is set is a breaking change for any external CI/CD pipeline or automation that currently invokes ec validate image --vsa-upload ... without --vsa-public-key. Those invocations will now fail with exit code 1. The PR body acknowledges this and states default Konflux pipelines are unaffected, but any non-default downstream consumer will break.
  Remediation: Consider whether a deprecation period or a minor version bump is warranted. If the security fix (EC-1842) necessitates an immediate break, document the migration path in release notes.

Medium

• [edge-case] internal/validate/vsa/validator.go:254 — VSA validator's ParseEffectiveTime only handles "now" and RFC3339 formats, while the policy package additionally accepts YYYY-MM-DD dates. When a user passes --effective-time 2025-01-15 (date-only), the VSA path returns a non-passing ValidationResult with message "invalid effective time", causing validation to fall back to full image validation. This silently disables VSA caching for date-only effective times. (Pre-existing issue in validator.go, newly exposed by this PR's wiring.)
  Remediation: Add YYYY-MM-DD fallback parsing to vsa.ParseEffectiveTime, or reuse policy.ParseEffectiveTime.

Low

• [scope-intent] cmd/validate/image.go:375 — The effectiveTime handling translates the "attestation" sentinel value to policy.Now specifically for the VSA path. The design choice is defensible since attestation-time semantics are meaningless for VSA policy comparison, but a brief code comment would improve clarity.

• [stale-doc] THREAT_MODEL.md:97 — Section 3.1 lists --vsa-signing-key and --vsa-upload as ec validate image-specific flags but does not include the new --vsa-public-key flag, which crosses a trust boundary (operator-controlled public key for VSA signature verification).

• [test-inadequate] internal/image/validate_test.go:459 — No test case covering the error path where ValidateVSAAndComparePolicy returns an error from signature verification failure. The warn-and-continue behavior is tested implicitly via the retriever-error path, but an explicit signature-verification-error test case would improve coverage of this security-critical code path.

• [stale-doc] docs/modules/ROOT/pages/types-of-attestations-and-manifests.adoc:63 — The VSA section lists --vsa-signing-key and --vsa-upload for ec validate image but does not mention --vsa-public-key, which is now required when --vsa-upload is set.

• [stale-reference] internal/validate/vsa/vsa.go:554 — CreateVSACheckerFromUploadFlags is still exported and defined but after this PR removes the only production caller, it becomes dead code.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (6)

Review

Findings

High

• [logic-error] cmd/validate/image.go:299 — The validation len(data.vsaUpload) > 0 && data.vsaPublicKey == "" unconditionally requires --vsa-public-key whenever --vsa-upload is set. However, --vsa-upload is also used for uploading newly generated VSAs (e.g., tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml:469), where no public key is needed. The public key is only consumed in the VSAValidationConfig struct passed to ValidateImageWithVSACheck, which is only reached when data.vsaExpiration > 0. The in-repo Konflux task uses --vsa-upload without --vsa-public-key or --vsa-expiration and will break with this change, contradicting the PR claim that "Default Konflux pipelines are unaffected."
  Remediation: Narrow the guard to if len(data.vsaUpload) > 0 && data.vsaExpiration > 0 && data.vsaPublicKey == "" { ... }. This ensures the public key is only required when the VSA skip/verification path is active. See also: [backward-compatibility] finding.

Medium

• [backward-compatibility] cmd/validate/image.go:299 — The new --vsa-public-key flag is enforced whenever --vsa-upload is set, making this a breaking CLI change broader than necessary. The public key is only consumed when data.vsaExpiration > 0 activates the VSA skip/verification path. Existing users of --vsa-upload for upload-only workflows (without --vsa-expiration) will encounter unexpected errors. See also: [logic-error] finding.

Low

• [test-inadequate] internal/image/validate_test.go — No test verifies fallback behavior when the VSA has a valid signature and "passed" status but the policy does not match. Since the core purpose of this PR is to add policy comparison to the image validation path, a test for the policy-mismatch-fallback path would strengthen coverage.
• [stale-doc] THREAT_MODEL.md:97 — Section 3.1 "CLI arguments and flags" lists --vsa-signing-key and --vsa-upload as security-relevant flags for ec validate image but does not include the new --vsa-public-key flag. Since --vsa-public-key is a trust-boundary input for signature verification, it should be listed.
• [policy-comparison-bypass] internal/validate/vsa/validator.go:124 — Pre-existing: ValidateVSAAndComparePolicy skips policy comparison when PolicySpec.Sources is empty and still returns Passed: true. Mitigated in practice because image validation always provides a policy with sources.
• [log-message-contract] features/__snapshots__/vsa.snap — Warning log message format changed from "Failed to check for existing VSA" to "Failed to validate existing VSA...failed to check existing VSA". If any monitoring matches on the old text, it may need updating.
• [error-handling-idiom] cmd/validate/image.go:299 — Error message uses "required when --vsa-upload is set" while the existing pattern at line 292 uses "required for --attestation-format=dsse". Minor format inconsistency.
• [exported-function-signature] internal/image/validate.go:175 — ValidateImageWithVSACheck signature changed from (*vsa.VSAChecker, time.Duration) to (*vsa.VSAValidationConfig). Located under internal/, so no cross-repo impact.
• [scope-creep] features/__snapshots__/validate_image.snap — 1152 lines of dead snapshot data removed (the "many components and sources" fixture whose scenario is commented out). While valid cleanup, this is unrelated to the security fix.
• [validation-grouping] cmd/validate/image.go:299 — The new validation is placed outside the if data.vsaEnabled block. This is actually correct: --vsa-upload with --vsa-expiration can activate the skip path without --vsa, so the check should not be inside the data.vsaEnabled guard.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (7)

Review

Findings

Medium

• [missing-new-flag] THREAT_MODEL.md:98 — The new --vsa-public-key flag (required when --vsa-upload is set) is not listed in THREAT_MODEL.md section 3.1, which inventories ec validate image CLI flags including --vsa-signing-key and --vsa-upload. Since this flag controls a security-relevant trust boundary (public key for VSA signature verification), it should be included in the threat model's entry-point inventory.
  Remediation: Add a row for --vsa-public-key in the section 3.1 table.

Low

• [incomplete-flag-listing] docs/modules/ROOT/pages/types-of-attestations-and-manifests.adoc:63 — The VSA section lists --vsa, --vsa-signing-key, --vsa-upload but does not mention the new --vsa-public-key requirement when --vsa-upload is set.
  Remediation: Add a note that --vsa-public-key is required when --vsa-upload is set.

• [test integrity] internal/image/validate_test.go — Unit tests for ValidateImageWithVSACheck all set IgnoreSignatureVerification: true. Signature verification is tested at the validator layer (validator_test.go) and in acceptance tests (features/vsa.feature), so coverage exists — but the integration through ValidateImageWithVSACheck specifically is not covered at the unit level.

• [import grouping] internal/image/validate_test.go:49 — The ecapi import is placed in its own blank-line-separated group. While validator.go uses the same pattern, most files in the codebase place ecapi in the third-party group. Minor inconsistency.

• [stale reference] internal/validate/vsa/vsa.go:554 — CreateVSACheckerFromUploadFlags is no longer called from any production code after this PR. It remains exported and tested but is now dead code.

• [edge case] cmd/validate/image.go — The --vsa-public-key validation requires the flag even when --vsa-expiration=0 (where the VSA skip path would never execute). Conservatively correct but could produce a confusing error for users who set expiration to 0 while keeping --vsa-upload for VSA generation.

Previous run (8)

Review

Findings

Medium

• [missing-doc] THREAT_MODEL.md:97 — Section 3.1 enumerates CLI flags for ec validate image including --vsa-signing-key and --vsa-upload, but does not list the new --vsa-public-key flag. While the flag is documented in section 3.8 for ec validate vsa (line 166), section 3.1 should be updated to reflect that --vsa-public-key now also applies to ec validate image.
  Remediation: Add a row to the section 3.1 table for --vsa-public-key with trust level ‘Operator-controlled’.

• [stale-doc] internal/validate/vsa/DESIGN.md:21 — States ‘The signing key is the same key used for the original image validation.’ This PR introduces --vsa-public-key as a separate key for VSA signature verification, distinct from --vsa-signing-key used for signing.
  Remediation: Update to reflect that VSA signing and verification use dedicated keys (--vsa-signing-key and --vsa-public-key).

• [breaking-cli] cmd/validate/image.go:299 — --vsa-upload now requires --vsa-public-key to be set, which is a breaking change to the CLI contract. Any CI/CD pipeline or script using ec validate image --vsa-upload ... without --vsa-public-key will fail. The PR body acknowledges this and states default Konflux pipelines are unaffected.
  Remediation: Ensure release notes communicate the breaking change and migration path.

Low

• [test-inadequate] internal/image/validate_test.go:443 — The PR adds test cases for ‘VSA passed - skip validation’ and ‘VSA predicate failed’ paths, improving coverage of the new result.Passed branching logic. The base-branch tests only exercised the error/fallback path via a mock retriever that always returned errors.

• [input-validation-gap] cmd/validate/image.go:597 — The --vsa-public-key flag accepts a file path but does not validate that the file exists in PreRunE. Errors surface later during signature verification. Consistent with existing --vsa-signing-key pattern.

• [stale-reference] internal/validate/vsa/vsa.go:554 — CreateVSACheckerFromUploadFlags still exists but is no longer called from the image validation path after this PR. Consider deprecating or removing it if no other callers remain.

• [code-organization] cmd/validate/image.go:682 — The vsaPublicKey struct field is placed after vsaExpiration instead of adjacent to vsaSigningKey. Similarly, --vsa-public-key is registered between --vsa-expiration and --attestation-output-dir rather than next to --vsa-signing-key. Grouping key-related fields and flags together would improve readability.

Previous run (9)

Review

Findings

Medium

• [edge-case] internal/validate/vsa/validator.go:251 — ParseEffectiveTime in the vsa package only handles "now" and RFC3339 values. When --effective-time=attestation is used, data.effectiveTime flows into VSAValidationConfig.EffectiveTime, and ParseEffectiveTime fails to parse it, causing ValidateVSAAndComparePolicy to return Passed: false. The caller falls through to full validation (safe), but the VSA caching optimization is silently defeated for --effective-time attestation users.
  Remediation: Add an "attestation" case in the vsa package’s ParseEffectiveTime, or handle it before constructing VSAValidationConfig.

• [breaking-cli] cmd/validate/image.go:299 — Adding a required --vsa-public-key flag when --vsa-upload is set is a breaking change to the CLI contract. Existing automation invoking ec validate image --vsa-upload ... without --vsa-public-key will now fail. The PR body acknowledges this as intentional.
  Remediation: Document in release notes. Consider a deprecation warning in one release before making it a hard error, if external consumers exist beyond default Konflux pipelines.

• [missing-doc] THREAT_MODEL.md:97 — THREAT_MODEL.md section 3.1 lists security-relevant CLI flags including --vsa-signing-key and --vsa-upload but does not include the new --vsa-public-key flag, which controls cryptographic verification of VSA signatures.
  Remediation: Add a row for --vsa-public-key in the section 3.1 table.

Low

• [edge-case] cmd/validate/image.go:299 — The validation requires --vsa-public-key whenever --vsa-upload is set, regardless of --vsa-expiration. A user who sets --vsa-expiration 0 to disable VSA checking is forced to provide a key that is never used.
  Remediation: Consider gating on vsaExpiration > 0 as well.

• [error-message-style] cmd/validate/image.go:298 — Error message phrasing differs from adjacent validation: "--vsa-public-key is required when --vsa-upload is set" vs existing "--vsa-signing-key required for --attestation-format=dsse".
  Remediation: Align style, e.g., "--vsa-public-key required when --vsa-upload is set".

• [stale-doc] .cursor/rules/vsa_functionality.mdc:336 — References ValidateVSAWithPolicyComparison and ValidationData which were renamed to ValidateVSAAndComparePolicy and VSAValidationConfig in a prior change. The ValidateImageWithVSACheck signature documentation is also outdated.
  Remediation: Update API references in .cursor/rules/vsa_functionality.mdc.

---

Labels: PR modifies CLI flag validation for VSA security hardening and addresses a red team finding about VSA bypass

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:29 AM UTC · Completed 9:49 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:19 AM UTC · Ended 10:25 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:26 AM UTC · Completed 10:47 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 10:50 AM UTC · Ended 11:11 AM UTC

Commit: 87c4a29 · View workflow run →

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/image/validate_test.go`:
- Around line 529-532: Strengthen fallback validation assertions in
internal/image/validate_test.go at lines 529-532 and 571-573: when expectSkip is
false, assert that the output from ValidateImageWithVSACheck is non-nil,
retaining the returned out value at lines 571-573 before asserting it. Keep the
existing error assertions and skip behavior unchanged.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: a88a7a2b-e0e6-4c73-b138-4cd4a468e64b

📥 Commits

Reviewing files that changed from the base of the PR and between 942d085 and 5a66a85.

📒 Files selected for processing (1)

• internal/image/validate_test.go

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/validate/image.go	93.33%	1 Missing ⚠️
internal/image/validate.go	88.88%	1 Missing ⚠️

Flag	Coverage Δ
acceptance	54.45% <75.00%> (+0.02%)	⬆️
generative	12.27% <0.00%> (-0.02%)	⬇️
integration	23.59% <16.66%> (-0.01%)	⬇️
unit	72.21% <54.16%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/validate/image.go	91.32% <93.33%> (+<0.01%)	⬆️
internal/image/validate.go	76.97% <88.88%> (+4.78%)	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:29 PM UTC · Completed 4:08 PM UTC

Commit: 87c4a29 · View workflow run →

[st3penta]

/fs-fix rebase on top of main

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 12:49 PM UTC · Ended 12:52 PM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:53 PM UTC · Completed 1:12 PM UTC

Commit: 87c4a29 · View workflow run →

invisible review

[simonbaird]

I'm trying to understand this. Docs say "Add --vsa-public-key to ec validate image, required when --vsa-upload is set.", but they don't say what it's used for. I guess it's used for doing a sig check on an existing VSA found in the storage backend specified by --vsa-upload, is that right?

I'm thinking maybe --vsa-upload is a badly chosen param name. It sounds like a verb kinda.

[simonbaird]

See my comment and linked PR about the removed snapshot data. Other than that, lgtm.

[st3penta]

I'm trying to understand this. Docs say "Add --vsa-public-key to ec validate image, required when --vsa-upload is set.", but they don't say what it's used for. I guess it's used for doing a sig check on an existing VSA found in the storage backend specified by --vsa-upload, is that right?

Yes that's correct. Btw, the same args are used both for vsa creation and vsa retrieval, and this creates some confusion imo.
I agree on the --vsa-upload arg name, but i'd say it's out of scope for this PR. We have EC-2006 about deciding what to do with the VSA code, i think it fits there more than here.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:55 AM UTC · Completed 10:13 AM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

noisy nitpicks

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:00 PM UTC · Completed 2:52 PM UTC

Commit: 87c4a29 · View workflow run →

[fullsend-ai-retro]

Retro: PR #3489 — Use ValidateVSAAndComparePolicy for VSA-based validation skipping

Overview

Human-authored security fix (st3penta, co-authored with Claude Opus 4.6) that hardened the VSA validation-skip path in ec validate image. Replaced the weak IsValidVSA check (only checked Found && !Expired) with full signature verification, predicate status checking, and policy comparison via ValidateVSAAndComparePolicy. Added --vsa-public-key flag. 8 files changed (+201/-100). Open Aug 11–19 (8 days), merged after human approval by simonbaird.

Timeline

• Aug 11 09:00 — PR opened. Three agent review bots (fullsend, qodo, coderabbit) begin reviewing.
• Aug 11 09:27–14:43 — 6 fullsend review cycles. Cycle 4 catches a real logic error: guard requires --vsa-public-key for all --vsa-upload users, breaking Konflux pipelines. Author fixes in commit 5649a8a0 (gates on vsaExpiration > 0). Cycle 6 APPROVES.
• Aug 12 — robnester-rh (human) flags help text inconsistency re: --vsa-expiration > 0 condition. Author fixes.
• Aug 13–14 — 4 more fullsend review cycles, now REQUEST_CHANGES for documentation gaps (THREAT_MODEL.md, DESIGN.md). Author requests /fs-fix rebase on top of main.
• Aug 18 — simonbaird (human) questions deleted snapshot data, discovers it's tied to still-open issue EC-1023. Creates follow-up PR Restore commented out acceptance test scenario #3505. Gives "lgtm" with that caveat.
• Aug 19 09:54 — Final fullsend review run (#32240017636, $9.26, 19 min). Still REQUEST_CHANGES on doc gaps.
• Aug 19 13:55 — simonbaird formally APPROVES.
• Aug 19 13:59 — PR merged with outstanding agent REQUEST_CHANGES.

What went well

1. Agent caught a real bug early. The logic-error finding (guard too broad for --vsa-expiration=0 case) was identified by the review agent on Aug 11 before any human reviewer noticed it, and the author fixed it promptly.
2. Challenger sub-agent filtered false positives. The final review run's challenger correctly removed 5 findings including a false positive about a "missing test" that actually existed in the PR diff.
3. Agent and humans were complementary. Agent excelled at: documentation gap detection (THREAT_MODEL.md, DESIGN.md — no human flagged these), code-level correctness, breaking-change analysis. Humans excelled at: domain-specific edge cases (local@ backend divergence — robnester-rh), cross-issue context (snapshot data tied to EC-1023 — simonbaird), UX-level concerns (flag naming confusion — simonbaird).
4. Security assessment was sound. The agent correctly validated fail-closed error handling, mandatory signature verification, and proper PreRunE gating.

What could improve

1. 10 review cycles with repeated findings. The breaking-cli finding appeared in nearly every review cycle with near-identical wording. The review bot did not deduplicate or reference its prior findings.
2. Verdict oscillation. Reviews went COMMENTED (3x) → DISMISSED (2x) → APPROVED → CHANGES_REQUESTED (2x) → DISMISSED (2x). The approval-then-reversal for unchanged documentation findings erodes trust.
3. 50 fullsend.yaml dispatch runs, only 2 actually dispatched. 48 runs evaluated and skipped with "No stage matched," consuming CI minutes.
4. ~$90+ total review cost. 10 successful review runs at ~$9/run, plus 4 cancelled runs.

Existing issues with new evidence from this retro

• fullsend#1013 and agents#721: The breaking-cli finding was raised in ~8 of 10 review cycles with nearly identical wording. Strong evidence for content-based deduplication.
• agents#685: Verdict flipped from APPROVE to REQUEST_CHANGES for unchanged doc findings, demonstrating the need to persist prior severity assessments.
• fullsend#2992 and fullsend#2599: 10 completed review runs on one PR supports the case for per-PR review budgets or circuit breakers.
• agents#106: The breaking-cli finding was re-raised after the author explicitly acknowledged it as intentional and documented for release notes.
• fullsend#893: 48 of 50 dispatch runs evaluated and skipped without dispatching any agent work.

Proposals filed

• Review agent should check for remaining codebase references when test data is deleted (in fullsend-ai/agents)
• Add AGENTS.md guidance for Go file header conventions and security-fix review expectations (in conforma/cli)