CI: take strict-validate's targets from the manifest, not from the workflow - #32
Conversation
…rkflow The job listed `qe benchmark audit .` by hand. That is a second source of truth for which plugins exist, and nothing couples it to marketplace.json: add a plugin, forget the workflow, and it is silently never strict-validated — the exact failure the job exists to prevent, arriving quietly. Nothing in the contributor checklist would have caught it either. There is no live gap today (three plugins, three names, exact match), so this is closing the hole before something falls in rather than fixing a break. scripts/validate.py gains --print-sources, which resolves each entry through the existing resolve_source() and prints one directory per line. Reusing that function matters: it already handles both the `./name` string form and the object form, so the workflow cannot disagree with the validator about what a source means. On a malformed manifest it prints nothing and exits 1 — verified by breaking the qe source on purpose — and the step turns that into a job failure rather than validating an empty list, because "no targets" must never read as "nothing to check". python3 rather than python: this job installs Node, not Python, and relies on the runner's preinstalled interpreter.
There was a problem hiding this comment.
🟡 Not ready to approve
The new --print-sources path can succeed on marketplace schema problems that the normal validator treats as errors, and it emits misleading diagnostics for certain malformed plugins entries.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR removes a “second source of truth” from CI by deriving the strict-validate job’s per-plugin targets directly from the marketplace manifest, using a new scripts/validate.py --print-sources mode that reuses existing source resolution logic.
Changes:
- Add
--print-sourcestoscripts/validate.pyto print resolved plugin source directories (one per line) for CI consumption. - Update
.github/workflows/validate.ymlto build thestrict-validatetarget list from--print-sourcesoutput (plus the marketplace root), and to fail the job if the plugin list cannot be read.
File summaries
| File | Description |
|---|---|
| scripts/validate.py | Adds --print-sources to emit plugin directories derived from marketplace.json using resolve_source(). |
| .github/workflows/validate.yml | Replaces the hard-coded plugin roster with a derived target list from --print-sources, ensuring CI stays coupled to the manifest. |
Review details
Suppressed comments (1)
scripts/validate.py:194
- This branch conflates two different manifest errors (non-object entry vs missing
name) into the same message, which makes debugging malformedpluginsentries harder. It would be clearer to emit the same type-specific error that the main validation path uses when an entry is not an object.
for entry in plugins:
if not isinstance(entry, dict) or not entry.get("name"):
error("marketplace.json: plugin entry missing `name`")
continue
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Copilot review on #32. Two modes of one script disagreeing about whether a manifest is valid is the smell, and both halves reproduced: plugins as a dict -> exit 1, but the message read "no plugins registered" where the real fault was a wrong type owner removed -> --print-sources exit 0, full validation exit 1 The second is the one that matters. It cannot let a broken manifest through today, because the sibling `validate` job runs full validation on the same commit — but a mode that answers "fine" about something the other rejects is a trap for whoever next reaches for it, and nothing states that the narrow mode is only safe alongside the wide one. Both now share main()'s top-level checks, and the not-a-list case is distinguished from the empty case so the message names the actual fault.
The
strict-validatejob listed its targets by hand:for target in qe benchmark audit .. That is a second source of truth for which plugins exist, with nothing coupling it tomarketplace.json— add a plugin, forget the workflow, and it is silently never strict-validated. That is the exact failure this job exists to prevent, arriving quietly. Nothing in the contributor checklist would have caught it either.There is no live gap today — three plugins, three names, exact match. This closes the hole before something falls in, which is why it is the smallest and most deferrable of the maintenance PRs.
How
scripts/validate.pygains--print-sources, which resolves each catalogue entry through the existingresolve_source()and prints one directory per line. Reusing that function is the point: it already handles both the"./name"string form and the object form, so the workflow cannot end up disagreeing with the validator about what asourcemeans.On a malformed manifest it prints nothing and exits 1 — verified by breaking
qe's source on purpose:The workflow step turns that into a job failure rather than iterating an empty list, because "no targets" must never read as "nothing to check" — the same principle as the version guard's exit 2 when it cannot see the base.
python3rather thanpython: this job installs Node, not Python, so it relies on the runner's preinstalled interpreter.Verification
Simulated the exact step locally — resolved targets
qe benchmark audit ., all four✔ Validation passed. YAML re-parses; normalvalidate.pyoutput is unchanged. No version bumps:scripts/and.github/are repo-level, and the guard agrees.