Skip to content

fix(scan): classify missing release file and add --tag to local scans - #356

Merged
openshift-merge-bot[bot] merged 2 commits into
openshift:mainfrom
smith-xyz:fix/scan-local-tag-identity
Aug 6, 2026
Merged

fix(scan): classify missing release file and add --tag to local scans#356
openshift-merge-bot[bot] merged 2 commits into
openshift:mainfrom
smith-xyz:fix/scan-local-tag-identity

Conversation

@smith-xyz

Copy link
Copy Markdown
Contributor

Summary

  • Classify a missing /etc/redhat-release detected via Lstat as
    ErrDistributionFileMissing instead of leaking a raw lstat error that no
    config exception can match (previously only the ReadFile path was
    classified).
  • Add a --tag flag to scan local so tag-scoped config exceptions (e.g.
    [[tag.agentic-skills.ignore]]) can apply. Local scans previously used an
    empty mock tag name, making every tag ignore unreachable. The tag carries
    identity only — the scan root remains --path and is never resolved to a
    <path>/<tag> subdirectory (pinned by test).
  • Fixes the payload-scan-5-0 smoke test failure on the agentic-skills
    FROM-scratch data-only image: no OS layer, zero binaries, and the existing
    ErrOSNotCertified tag ignore could not fire under scan local.

Without --tag, behavior is unchanged: the mock tag name stays empty, no
ignores match, and the scan root normalization (filepath.Clean) is identical
to the previous filepath.Join(path, "").

Companion openshift/release change passes the tag name per image in the
payload-scan-* steps; this PR must merge first since old binaries reject the
new flag.

@openshift-ci
openshift-ci Bot requested review from rhmdnd and rphillips August 5, 2026 21:29
@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smith-xyz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2026
@qodo-for-rh-openshift

Copy link
Copy Markdown

PR Summary by Qodo

Fix local scans: classify missing /etc/redhat-release and add --tag identity

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Normalize missing /etc/redhat-release (Lstat/ReadFile) to ErrDistributionFileMissing.
• Add --tag to scan local so tag-scoped ignores can match local bundles.
• Pin tag-as-identity semantics: tag never changes the scan root path.
Diagram

graph TD
  A["CLI: scan local (main.go)"] --> B["Config: LocalTag (types.Config)"] --> C["RunLocalScan (scan.go)"] --> D["simulateLocalPayload(tag)"] --> E["validateTagLocal (scan.go)"] --> F["ValidateOS (validations_os.go)"]
  C --> G["Tag-scoped ignores (ConfigFile.TagIgnores)"]
  G --> E
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Infer tag identity from bundle metadata
  • ➕ No new CLI flag needed; less user input
  • ➕ Avoids mismatches between unpack source and provided tag
  • ➖ Requires a stable metadata source in the unpacked bundle
  • ➖ More coupling to bundle layout/producer and harder to validate across environments
2. Allow matching ignores without tag for local scans
  • ➕ Keeps local scan UX minimal
  • ➕ Avoids needing identity when scanning ad-hoc directories
  • ➖ Weakens the semantics of tag-scoped exceptions
  • ➖ Risks unintentionally applying ignores too broadly
3. Support multiple --tag values (fan-out scan)
  • ➕ Can validate one local bundle against multiple tag policies in one run
  • ➕ Useful for shared bundles or mirrored content
  • ➖ More complex reporting and exception semantics
  • ➖ Not necessary for the stated smoke-test failure and increases surface area

Recommendation: Keep the explicit --tag approach: it preserves strict tag-scoped ignore semantics, keeps path resolution unambiguous (pinned by test), and minimizes coupling to any particular local bundle layout. Consider metadata inference later only if a reliable, standardized bundle identifier becomes available.

Files changed (6) +240 / -16

Enhancement (2) +14 / -10
scan.goPropagate local tag identity; keep scan root independent of tag +11/-10

Propagate local tag identity; keep scan root independent of tag

• Threads cfg.LocalTag into the simulated local payload so TagReference.Name is meaningful for tag-scoped exceptions. Changes local validation to use filepath.Clean(bundlePath) instead of joining <path>/<tag>, preserving prior scan-root behavior while enabling tag identity matching.

internal/scan/scan.go

main.goAdd --tag flag to scan local and wire into config +3/-0

Add --tag flag to scan local and wire into config

• Introduces a new scan local flag --tag and copies its value into config.LocalTag before invoking RunLocalScan. Enables tag-scoped config exceptions for local bundle scans without changing default behavior when omitted.

main.go

Bug fix (1) +11 / -6
validations_os.goUnify missing release-file handling for Lstat and ReadFile paths +11/-6

Unify missing release-file handling for Lstat and ReadFile paths

• Introduces classifyReleaseFileError to map os.ErrNotExist to ErrDistributionFileMissing consistently. Uses this classification both when resolving the target path (Lstat/GetTargetPath failures) and when reading the release file, enabling config exceptions to match missing-file conditions.

internal/validations/validations_os.go

Tests (2) +214 / -0
scan_test.goAdd regression tests for data-only images and tag-as-identity semantics +109/-0

Add regression tests for data-only images and tag-as-identity semantics

• Adds coverage for FROM-scratch/data-only bundles: without tag identity the scan surfaces ErrDistributionFileMissing, and with a matching tag ignore OS validation is skipped. Adds a test ensuring --tag never changes the scan root to <path>/<tag>.

internal/scan/scan_test.go

validations_os_test.goAdd ValidateOS test matrix including missing file and dangling symlink +105/-0

Add ValidateOS test matrix including missing file and dangling symlink

• Adds table-driven tests to verify missing release files and dangling symlinks are classified as ErrDistributionFileMissing, and that certified/uncertified distributions behave as expected. Also validates the warning behavior when the certified distribution allowlist is empty.

internal/validations/validations_os_test.go

Other (1) +1 / -0
types.goAdd Config.LocalTag field for local scan identity +1/-0

Add Config.LocalTag field for local scan identity

• Extends the Config struct with a LocalTag string, used to carry release tag identity for local scanning and exception matching.

internal/types/types.go

@qodo-for-rh-openshift

qodo-for-rh-openshift Bot commented Aug 5, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. Comment uses passive voice ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
Newly added test comments in internal/scan/scan_test.go use passive voice (e.g., "validation is
skipped", "it is never resolved", "were joined"), which obscures the actor and reduces clarity.
Rewrite these comments in active voice to comply with the documentation style requirement.
Code

internal/scan/scan_test.go[R149-152]

+// TestRunLocalScanDataOnlyImage covers FROM-scratch data-only images (no OS
+// layer, no binaries): without tag identity the scan fails with
+// ErrDistributionFileMissing; with --tag and a matching tag ignore the OS
+// validation is skipped.
Evidence
PR Compliance ID 393 requires active voice in newly added or modified comments, and the cited
locations in internal/scan/scan_test.go show passive constructions:
internal/scan/scan_test.go[149-152] contains "validation is skipped",
internal/scan/scan_test.go[218-220] includes "it is never resolved", and
internal/scan/scan_test.go[247-248] uses phrasing like "were joined", each demonstrating
noncompliant passive voice.

Rule 393: Use active voice in documentation and comments
internal/scan/scan_test.go[149-152]
internal/scan/scan_test.go[218-220]
internal/scan/scan_test.go[247-248]

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

## Issue description
Newly added comments use passive voice (e.g., `validation is skipped`, `it is never resolved`, `were joined`), which reduces clarity about who/what performs the action. The compliance checklist (PR Compliance ID 393) requires active voice in documentation and comments.

## Issue Context
These are test comments documenting `--tag` behavior: one describes OS validation being skipped, another notes that `--tag` does not affect scan root resolution, and a third explains the failure mode if code were to join the tag into the scan root.

## Fix Focus Areas
- internal/scan/scan_test.go[149-152]
- internal/scan/scan_test.go[218-220]
- internal/scan/scan_test.go[247-248]

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


Grey Divider

Context used
✅ Compliance rules (platform): 13 rules

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread internal/scan/scan_test.go Outdated
@smith-xyz

Copy link
Copy Markdown
Contributor Author

/override ci/prow/payload-scan-5-0

this is to fix that issue, will need a change in openshift/release

@openshift-ci

openshift-ci Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@smith-xyz: Overrode contexts on behalf of smith-xyz: ci/prow/payload-scan-5-0

Details

In response to this:

/override ci/prow/payload-scan-5-0

this is to fix that issue, will need a change in openshift/release

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci

openshift-ci Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@smith-xyz: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@richardsonnick

Copy link
Copy Markdown

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2026
@openshift-merge-bot
openshift-merge-bot Bot merged commit 33d5671 into openshift:main Aug 6, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants