Skip to content

validate-pr.yaml passes without checking any manifest #37

Description

@ecv

Summary

The plugin catalog's pull request check reports success without validating anything, and every run of it has passed that way, including the ones that admitted the manifests now in the catalog.

A green check means the job found no files to examine, not that the manifests are valid.

Two separate defects produce that, and fixing either one alone turns a silent pass into a confusing failure.

How it passes

.github/workflows/validate-pr.yaml finds the files to check with:

files=$(git diff --name-only origin/main...HEAD -- plugins/ | tr '\n' ' ')

actions/checkout@v7 does a shallow, single-branch fetch, so no origin/main ref exists in the workspace. The command fails:

fatal: bad revision 'origin/main...HEAD'

The job does not fail with it. git is piped into tr, so the pipeline exits with tr's status, which is 0, and bash -e sees nothing wrong. files ends up empty, and both later steps run for f in ; do, iterating zero times.

Run 34407569997 shows it: fatal: bad revision in the "Find changed plugin manifests" step, then no Validating schema: or Checking URIs in: line for any file, then success.

A second bug behind the first

Fixing only the git diff would expose this one. The URI verification step opens its heredoc as <<'EOF', which is quoted, so the shell does not expand $f inside it. Python receives the literal string:

with open("$f") as fh:

That raises FileNotFoundError on the first real file. Both need fixing together, or the first fix turns a silent pass into a confusing failure.

What good looks like

  • A manifest with a bad schema, an unreachable URI, or a wrong sha256 fails the check
  • The check cannot pass by finding no files to examine

Notes

milo-os/cli-plugins hit this and solved it by not diffing at all. Its validate.yaml runs scripts/verify_manifests.py over every manifest in the repo on each run. The catalog is small, the assets are the real ones, and a manifest can break for reasons unrelated to the files a PR touches. That approach is worth copying rather than repairing the diff.

Found while opening #35, where the green check was taken at face value before it was checked. The manifest in that PR was verified by hand instead: schema against schema/plugin-v1alpha1.json, and all six archives downloaded and hashed.

One detail for whoever does this: ajv validate needs -c ajv-formats to run against this schema at all. The uri format makes ajv reject the schema outright without it, so the current invocation would fail even with files to check.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions