ci: flag pointer-only changes in recipe-evidence gate#1166
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates the recipe evidence gate script to treat changes to recipes/evidence/.yaml as making a recipe "affected" (Rule 4), adds detection of orphan evidence pointers that exist at HEAD but lack a corresponding recipes/overlays/.yaml, adjusts the early-exit and affected-overlays table emission to account for orphans, emits a Markdown section listing orphan pointers (incrementing warnings), and exports orphan_pointers=<orphan_count> in the job outputs. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
LGTM — correctly closes the evidence-only-PR gap (finding A): a pointer-only PR (like the recent test #1164) now gets verified instead of silently skipped. Reuses the existing grep -qxF whole-line match and integrates cleanly into the affected accumulation. Two minor, non-blocking edge-case notes inline.
| # unverified. `grep -qxF` matches the whole line, so a sibling like | ||
| # `recipes/evidence/${name}.yaml.bak` doesn't trigger it. | ||
| if [[ "$include" == "false" ]]; then | ||
| if printf '%s\n' "$changed_files" | grep -qxF "recipes/evidence/${name}.yaml"; then |
There was a problem hiding this comment.
Minor / non-blocking: this loop only iterates existing recipes/overlays/*.yaml, so a pointer added for a recipe with no matching overlay (typo'd slug, or a retired recipe) is silently skipped — it never hits an overlay iteration, so a misnamed evidence file wouldn't be flagged. Optional: diff added recipes/evidence/*.yaml against the overlay set to catch orphans.
There was a problem hiding this comment.
Good catch — addressed. Added an orphan-pointer check after the discovery loop that diffs the changed recipes/evidence/*.yaml against the overlay set, exactly as you suggested. It flags any added/modified pointer whose slug has no matching recipes/overlays/<slug>.yaml (typo'd slug or evidence for a retired recipe) in a dedicated "Orphan evidence pointers" section, counted as a warning.
Two details worth noting:
- Only pointers present on disk at HEAD are considered, so a deleted pointer is correctly not an orphan.
- The
count==0 & orphans>0path is handled: the early "no overlays affected" exit now requires both to be zero, and the affected-recipes table is guarded behindcount>0so it isn't rendered empty.
Verified the detection in isolation across has-overlay / typo'd-slug / retired-recipe / deleted / non-evidence-path cases.
| done <<<"$values_files" | ||
| fi | ||
|
|
||
| # Rule 4: the recipe's own evidence pointer was added or modified. |
There was a problem hiding this comment.
Minor / non-blocking: git diff BASE...HEAD lists deletions too, so removing a pointer also marks the recipe affected → downstream reports it as missing-pointer (graceful, per existing handling). Probably intended; just flagging that a pointer delete and a pointer add both route through Rule 4.
There was a problem hiding this comment.
Yes — intended, thanks for flagging it explicitly. Deleting a pointer while its overlay still exists should flag the recipe: it now lacks evidence, and downstream renders the graceful "missing-pointer" warning.
The one edge — retiring a recipe by deleting both the pointer and its overlay in the same PR — produces no false warning: the overlay loop no longer iterates it, and the new orphan check skips it because the deleted pointer isn't on disk at HEAD.
2 participants
Summary
Teach the recipe-evidence gate to flag a leaf recipe when the PR adds or modifies that recipe's own evidence pointer (
recipes/evidence/<slug>.yaml), not just when an overlay/ancestor/values file changes.Motivation / Context
The discovery loop in
recipe-evidence-check.shflagged a recipe as "affected" only via rules 1–3 (overlay changed, ancestor overlay changed, or referenced component values file changed). A PR that only refreshes an evidence pointer — exactly the workflow the script's own "How to refresh evidence" section prescribes (cp ./out/pointer.yaml recipes/evidence/<slug>.yaml) — touched none of those, so the gate reported "No leaf overlays affected" and the freshly-submitted pointer was neverevidence verify'd or digest-compared. This closes that gap.Fixes: N/A
Related: N/A
Type of Change
Component(s) Affected
.github/scripts/recipe-evidence-check.sh(warning-only recipe-evidence gate)Implementation Notes
recipes/evidence/<slug>.yamlis in the diff. Mirrors Rule 1'sgrep -qxFwhole-line exact match, so a sibling like…/<slug>.yaml.bakdoesn't false-trigger.-r <overlay>, so a pointer without a matching leaf overlay couldn't be verified anyway.promote_allpath is unaffected (every leaf is already included there), matching rules 1–3.paths: recipes/**filter already coversrecipes/evidence/**, so a pointer-only PR triggers the job; and the discovery loop is unconditional, so it evaluates Rule 4 even when no overlay/values file changed.basename-vs-criteria-slug naming limitation already tracked in the header (out of scope here).Testing
bash -n .github/scripts/recipe-evidence-check.sh # syntax OKRule 4's matching logic was exercised in isolation against five diff scenarios (it is pure
grep, independent ofyq):…/<slug>.yaml.baksibling → not flaggedThe full script could not be run end-to-end locally because the sandbox's
yqis the Python flavor while the script targets Goyq eval(as in CI); the new rule itself is yq-independent.Risk Assessment
Rollout notes: N/A — gate is warning-only.
Checklist
make testwith-race) — N/A, no Go changesmake lint) — N/A, shell-only change;bash -nclean (shellcheck unavailable in sandbox)git commit -S)