Add validate-dependabot-pr Copilot CLI skill - #286
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new skill instructions include a hardcoded developer-local path and non-idempotent cleanup steps that can break the “always clean up” guarantee and make the workflow non-portable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new Copilot CLI skill intended to standardize how maintainers validate open Dependabot npm PRs for HAR compliance, dependency diffs, CI health, and local packaging/install verification before manually merging.
Changes:
- Introduces a
validate-dependabot-prskill that lists open Dependabot PRs and produces a per-PR merge-readiness report. - Documents HAR-compliance heuristics for
.npmrc,package.json, and lockfile"resolved"URLs. - Documents CI check expectations (
validate+build (22.19.0)) plus local worktree-based packaging/install validation and cleanup.
File summaries
| File | Description |
|---|---|
| .github/skills/validate-dependabot-pr.skill.md | Adds a Dependabot PR validation procedure (HAR + deps + CI + package/install) and reporting format. |
Review details
Suppressed comments (1)
.github/skills/validate-dependabot-pr.skill.md:113
- Cleanup currently
cds to a hardcoded local path and may abort if the worktree/branch doesn’t exist (which undermines the “always clean up” goal). Use the captured repo path and make cleanup idempotent by tolerating missing worktrees/branches.
cd /Users/ngupta/Documents/chef-workstation-hab-pkg/rel/CW26/vscode-chef
git worktree remove /tmp/vscode-chef-pr-<n> --force
git branch -D pr-<n>-validate
rm -f /tmp/vscode-chef-pr-<n>.vsix /tmp/pr-<n>-manifest.json
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d0bc552 to
25bd458
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new skill instructions contain a few reliability/accuracy issues (HAR URL matching, non-existent VSIX baseline reference, and assumptions about origin) that can cause incorrect compliance results or failed validation runs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/skills/validate-dependabot-pr.skill.md:85
git fetch origin "pull/<n>/head:..."assumes the user'soriginremote points atchef/vscode-chef. In a common fork setup,originis the user's fork and the fetch will fail. Fetch from the upstream repo URL explicitly (or otherwise avoid assuming remote names) to make the worktree checkout step reliable.
REPO_ROOT="$(pwd)"
git fetch origin "pull/<n>/head:pr-<n>-validate"
git worktree add /tmp/vscode-chef-pr-<n> pr-<n>-validate
cd /tmp/vscode-chef-pr-<n>
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new skill’s worktree fetch/cleanup steps are brittle in common local clone setups and can fail or leave behind scratch state unless made more robust.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
.github/skills/validate-dependabot-pr.skill.md:87
git fetch origin "pull/<n>/head:..."assumes the caller’soriginremote points atchef/vscode-chef. If a developer’s clone hasoriginset to a fork (common), this fetch will fail and the validation flow can’t proceed. Fetch from the canonical repo URL (or usegh pr checkout).
.github/skills/validate-dependabot-pr.skill.md:121- The cleanup commands can fail (and potentially abort the remaining cleanup) if the worktree/branch were never created due to an earlier failure. Making these cleanup steps tolerant (e.g.,
|| true) helps ensure scratch state is always removed as intended.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
b150c20 to
10baf4b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The skill file’s structure is likely not parseable as a Copilot CLI SKILL.md (front matter not properly delimited / instructions placement), which can prevent the skill from loading and make the feature non-functional.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
.github/skills/validate-dependabot-pr.skill.md:42
HAR_HOSTis derived from.npmrcvia a relative path, which will fail (or produce an empty/incorrect host) if the skill is invoked from a subdirectory or outside a local chef/vscode-chef checkout, leading to incorrect HAR compliance verdicts. Resolve the repo root first and read.npmrcfrom there.
.github/skills/validate-dependabot-pr.skill.md:85
REPO_ROOT="$(pwd)"depends on where the user ran the skill and may not point at the git repository root (e.g., if invoked from a subdirectory). Because cleanup relies onREPO_ROOT, prefer deriving it from git andcdthere before creating the worktree.
REPO_ROOT="$(pwd)"
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The HAR compliance detection command uses regex-based host exclusion (grep -v) which can incorrectly miss non-HAR resolved URLs and should be switched to fixed-string matching.
Review details
Suppressed comments (1)
.github/skills/validate-dependabot-pr.skill.md:45
- The HAR compliance command uses
grep -v "$HAR_HOST"which treats$HAR_HOSTas a regular expression; dots in hosts likepkg.harness.ioare regex wildcards, which can cause false negatives. Use fixed-string matching for this exclusion so only non-HAR resolved URLs are reported.
gh pr diff <n> --repo chef/vscode-chef | grep '"resolved"' | grep '^+' | grep -v "$HAR_HOST"
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The skill’s HAR bypass check can produce false-negative results if HAR_HOST parsing fails, and there’s also a documented-path mismatch with the PR description that should be reconciled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new skill instructions contain a correctness issue in the HAR bypass grep and brittle CI check naming that will cause false passes/failures unless updated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
.github/skills/validate-dependabot-pr/SKILL.md:75
- The CI section header hard-codes the matrix-expanded check name
build (22.19.0). Sincebuildis a matrix job in.github/workflows/ci.yml, the visible check name will change when the matrix changes, and this header will become stale.
This issue also appears on line 86 of the same file.
.github/skills/validate-dependabot-pr/SKILL.md:89
- The PASS/FAIL criteria are tied to a specific check name
build (22.19.0), which will break when the workflow matrix changes. Instead, treat the requirement as:validatemust pass and all check rows whose name starts withbuildmust pass for the latest commit.
- PASS only if both the `validate` and `build (22.19.0)` rows show `pass`
(or `SUCCESS` in the JSON form) for the latest commit. Anything else
(`fail`, `pending`, `queued`) is a FAIL — call out which job failed and
include its URL from the output.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The skill’s command snippets have a couple of correctness/robustness issues that can lead to false passes or failures and should be fixed before relying on it for PR validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The added skill contains a command snippet bug that would cause the HAR compliance extraction to malfunction and potentially report false passes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/skills/validate-dependabot-pr/SKILL.md:25
- The
.npmrcsnippet is presented as the repo’s literal.npmrc, but it includes two explanatory comment lines that are not actually present in the root.npmrc(see.npmrcin this repo). This makes the embedded baseline inaccurate and can confuse reviewers comparing against the real file.
# npm safety gate (days; 14-day cooldown for supply-chain attack mitigation)
# stricter than the HAR doc's 7-day default — treat 14 as this repo's
# authoritative floor; never suggest lowering it.
min-release-age=14
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Adds a Copilot CLI skill (.github/skills/validate-dependabot-pr.skill.md) that validates open Dependabot npm PRs against Harness Artifact Registry (HAR) compliance, summarizes dependency changes, confirms the ci workflow (validate + build) is green, and verifies the extension still packages and installs correctly. The skill only reports a per-PR merge-readiness verdict; it never approves, comments on, or merges PRs — merging remains a manual step. Signed-off-by: nikhil2611 <ngupta@progress.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The packaging-verification steps in the skill instructions cd into a scratch /tmp worktree but never captured the original repo directory, forcing the cleanup step to hardcode a developer-specific path. Capture `REPO_ROOT="$(pwd)"` before entering the worktree and use it in the cleanup step instead, so the instructions work for any checkout path. Signed-off-by: nikhil2611 <ngupta@progress.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Derive HAR host dynamically from .npmrc's registry= line instead of hardcoding pkg.harness.io, so the resolved-URL filter can't be fooled by other *.harness.io hosts. - Replace non-existent vscode-chef.vsix baseline size comparison with a concrete, portable non-empty/>100KB sanity check for the packaged .vsix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: nikhil2611 <ngupta@progress.com>
- Fetch PR ref from the canonical chef/vscode-chef URL instead of the origin remote, since a developer local origin may point at a personal fork where pull/<n>/head would not resolve. - Suffix worktree/branch cleanup commands with || true so a partial earlier failure (worktree or branch never created) does not abort the rest of cleanup and leave scratch state behind. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: nikhil2611 <ngupta@progress.com>
Copilot review flagged that the skill file's YAML front matter was never closed with a second `---`, and the operational instructions were nested under an `instructions:` YAML key instead of being a plain Markdown body. Per GitHub's documented format, Copilot CLI skills must live in `.github/skills/<skill-name>/SKILL.md`, with minimal delimited YAML front matter (name, description only) followed by a Markdown body containing the instructions. Moved `.github/skills/validate-dependabot-pr.skill.md` to `.github/skills/validate-dependabot-pr/SKILL.md`, converting the `instructions:` YAML block scalar into a normal Markdown body with proper heading structure, and folding the multi-line `description:` into a single flowed line as required by the frontmatter spec. Signed-off-by: nikhil2611 <ngupta@progress.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add an explicit non-empty guard for HAR_HOST before running the diff greps. Previously, if HAR_HOST failed to parse (e.g. .npmrc missing or edited locally), `grep -v "$HAR_HOST"` with an empty pattern would suppress all output, making the HAR bypass check silently false-negative (always appear clean). Now the check exits with an error instead. - Use a forced refspec (`+pull/<n>/head:...`) when fetching the PR branch into a local ref for packaging verification, so the fetch reliably updates the local branch even when Dependabot force-pushes (rebases or refreshes) an existing PR between validation runs. The PR description was also updated separately to reference the correct file path (.github/skills/validate-dependabot-pr/SKILL.md) after the previous restructuring commit, since it still described the old flat .skill.md location. Signed-off-by: nikhil2611 <ngupta@progress.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…hing resilient to Node version bumps
- grep -v "$HAR_HOST" treated dots as regex wildcards, risking false
negatives if a non-HAR host happened to match the pattern. Use
grep -F -v -- for literal string matching.
- The CI check hardcoded 'build (22.19.0)', which breaks silently
whenever the workflow's Node matrix version changes. Match the
'build (' prefix instead.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: nikhil2611 <ngupta@progress.com>
- Replace grep -A <n> with sed range extraction for .npmrc/package.json diff hunks so full hunks are captured regardless of length, instead of being cut off at a fixed line count. - Scope the resolved-URL bypass check to the package-lock.json diff section only, and append || true so a clean result (no non-HAR resolved lines) doesn't cause the pipeline to exit non-zero. - Pre-clean any lingering worktree/branch from a prior run before fetching, so repeated invocations of the skill don't fail on an already-checked-out branch or existing worktree directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: nikhil2611 <ngupta@progress.com>
- Align the .npmrc example in the skill doc with the repo's actual file: use standalone comment lines instead of an inline/continuation comment on min-release-age=14, preventing confusing copy/paste. - Fetch the PR diff once into $PR_DIFF and reuse it for the .npmrc, package.json, and package-lock.json extracts instead of calling gh pr diff three times per PR, reducing latency and rate-limit risk. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: nikhil2611 <ngupta@progress.com>
f90edc3 to
651b3c2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new skill’s HAR compliance instructions include a documented check that can yield false negatives (substring host matching) and should be tightened before relying on it for validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/skills/validate-dependabot-pr/SKILL.md:15
- The
.npmrcsnippet in this skill doesn’t match the repo’s actual root.npmrc(it includes extra commentary lines that aren’t present), which risks the skill drifting out of sync with the real HAR baseline. Consider either quoting the real.npmrccontents verbatim or rephrasing this section as “key required settings” instead of presenting it as the file itself.
This repo uses **npm only** (no pnpm-workspace.yaml or yarn.lock). Its HAR baseline
lives in the root `.npmrc`:
REQUIRED - DO NOT REMOVE OR CIRCUMVENT
HAR (Harness Artifact Registry) configuration for chef org
- **Files reviewed:** 1/1 changed files
- **Comments generated:** 2
- **Review effort level:** Lite
</details>
- Match the full registry origin plus trailing slash ($HAR_ORIGIN/) instead of a bare host substring ($HAR_HOST), so a domain like pkg.harness.io.attacker.example can no longer slip past the check. - Clarify that the local packaging step adds --out for a deterministic artifact path, which the CI build job itself does not pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: nikhil2611 <ngupta@progress.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The HAR compliance extraction commands use a sed range that typically terminates immediately, which can cause the “non-HAR resolved URL” check to falsely pass.
Review details
Suppressed comments (1)
.github/skills/validate-dependabot-pr/SKILL.md:60
- The
sed -n '/^diff --git a\/…/,/^diff --git /p'ranges terminate immediately because the start line (diff --git …) also matches the end pattern (^diff --git), so these commands typically print only the header line and the HAR bypass check can become a false pass. Use a range extractor that stops at the nextdiff --githeader (without including the current header as the end match).
printf '%s\n' "$PR_DIFF" | sed -n '/^diff --git a\/\.npmrc /,/^diff --git /p'
printf '%s\n' "$PR_DIFF" | sed -n '/^diff --git a\/package\.json /,/^diff --git /p'
printf '%s\n' "$PR_DIFF" | sed -n '/^diff --git a\/package-lock\.json /,/^diff --git /p' | grep '"resolved"' | grep '^+' | grep -F -v -- "$HAR_ORIGIN/" || true
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
…sing The prior sed -n '/start/,/end/p' range used '^diff --git ' as the end address, which also matches the start line itself (since every target start pattern like '^diff --git a/package.json ' is a stricter form of '^diff --git '). Same-line start/end address matching is handled inconsistently across sed implementations and can degenerate into printing only the header line, producing a false-negative HAR bypass check. Replace it with an explicit awk helper that only stops printing when a *subsequent* line begins a new diff --git header, which is unambiguous regardless of sed/awk dialect and was verified against a real PR diff. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: nikhil2611 <ngupta@progress.com>
There was a problem hiding this comment.
🟡 Changes recommended
The skill should explicitly avoid running local npm ci/packaging when HAR compliance fails or is unverifiable to prevent unsafe execution or non-HAR dependency resolution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Explicitly skip the local npm ci / vsce package / install steps when the HAR compliance check (a) fails or can't be verified (e.g. a weakened .npmrc, a non-HAR-resolved lockfile entry, or the HAR_HOST/HAR_ORIGIN empty-guard tripping). Previously the skill only gated local packaging on CI status, so a PR that failed HAR compliance but still had green CI could still run npm ci locally — risking lifecycle script execution or non-HAR dependency resolution. Report these as 'not run — HAR compliance failed/unverifiable' in the final table, mirroring the existing CI-red skip convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: nikhil2611 <ngupta@progress.com>
There was a problem hiding this comment.
🟢 Approval recommended
Change is limited to adding a new skill documentation file with no runtime/code-path impact, and the only feedback is a minor accuracy tweak in the .npmrc excerpt.
Review details
Suppressed comments (1)
.github/skills/validate-dependabot-pr/SKILL.md:24
- The
.npmrcexcerpt in this skill includes two comment lines about the HAR doc’s 7‑day default that do not exist in the repo’s actual.npmrc, which can mislead readers into thinking those lines are part of the enforced baseline being validated.
# npm safety gate (days; 14-day cooldown for supply-chain attack mitigation)
# stricter than the HAR doc's 7-day default — treat 14 as this repo's
# authoritative floor; never suggest lowering it.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Adds a Copilot CLI skill (.github/skills/validate-dependabot-pr/SKILL.md) that validates open Dependabot npm PRs against Harness Artifact Registry (HAR) compliance, summarizes dependency changes, confirms the ci workflow (validate + build) is green, and verifies the extension still packages and installs correctly. The skill only reports a per-PR merge-readiness verdict; it never approves, comments on, or merges PRs — merging remains a manual step.