Fix Skill Validation results comment: define missing agentCount + harden comment posting - #36003
Conversation
…den comment posting The Skill Validation 'Post comment' step crashed with 'ReferenceError: agentCount is not defined'. The badge array referenced agentCount but it was never defined or produced, while skillCount was. This silently broke the results comment on every Skill Validation run. - Add an agent-count producer to the 'Save results artifact' step (counts top-level *.md files in .github/agents), written to agent-count.txt alongside skill-count.txt and uploaded/downloaded through the same static-check-results artifact. - Define agentCount in the 'Post comment' github-script step mirroring skillCount/specCount (read agent-count.txt with a try/catch fallback to '?'). - Wrap badge + body construction in a try/catch so a future undefined variable or formatting error can no longer sink the whole job; on failure it logs via console.error/core.warning and posts a minimal plain-text fallback comment so results always post. Introduced 2026-06-16 in commit 1c0c0eb (PR #35713). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36003Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36003" |
…accurate The new agent-count producer counts *.md files under .github/agents, but the static-check job's sparse-checkout only materialized .github/skills and .github/plugin.json. Without .github/agents checked out, the find matched nothing and the Agents badge always reported 0 in CI. Add .github/agents to that job's sparse-checkout (mirroring why skillCount is correct — .github/skills is already listed). Only the static-check job produces the count, so the other jobs' sparse-checkout lists are left unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial multi-model review — PR #36003
Methodology: 3 independent reviewers (different model families) with adversarial consensus, run over 2 rounds (find → fix → re-review until clean). Code-only review; CI status is out of scope.
Round 1 — 1 finding (fixed)
agent_count would always be 0 in CI. The new producer counts *.md under .github/agents, but the static-check job's sparse-checkout only materialized .github/skills and .github/plugin.json, so the directory was never checked out and the new Agents badge would silently misreport 0.
Consensus: 2/3 reviewers (the third's edge-case note corroborated the mechanism). Verified against source.
Resolved in 85cbf3a by adding .github/agents to that job's sparse-checkout (only static-check produces the count, so the other jobs' checkouts were intentionally left unchanged).
Round 2 — clean
✅ 3/3 reviewers: no remaining issues. Re-verified end-to-end:
agentCountis defined (mirroringskillCount/specCount) and in scope before its badge use; not shadowed.- Producer→consumer artifact wiring intact:
sv-results/agent-count.txt→static-check-results→static-results/agent-count.txt. - try/catch hardening is correct:
let body;is assigned on both paths;content/authorLine/badgesstay scoped inside the try; the fallback uses only pre-computed values (marker, statuses,headSha7,runUrl) so it can't rethrow, and preserves the marker so the upsert still updates (not duplicates) the comment. - Errors are logged (
console.error+core.warning), not swallowed. .github/agentsholds 6 markdown files → badge now reads a meaningful count.- Under
pull_request_target, the file count is attacker-influenceable but purely cosmetic —find | wc -lnever reads/executes file content, and the new log lines emit only an integer/JS-error string (no secret exposure).
Test coverage: No automated tests exercise this workflow (standard for CI YAML); the embedded github-script was validated locally via yaml.safe_load + node --check. The next Skill Validation run on a PR is the real-world check.
Prior reviews: none.
Verdict posted as a comment (not approve/request-changes) — approval remains a human decision.
kubaflo
left a comment
There was a problem hiding this comment.
Note
🤖 This review was automatically generated by a multi-model AI review system (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro). Three models independently reviewed the code, then cross-pollinated their findings to produce this consolidated review.
Multi-Model Review — PR #36003 Round 1
Verdict: ✅ LGTM (Unanimous: Opus, GPT, Gemini)
Confidence: High
Summary
Perfect unanimous approval across all three models. This PR correctly fixes a real ReferenceError with clean, low-risk code that mirrors proven patterns. No bugs, no security issues, no disagreements during cross-pollination.
What This PR Fixes
Problem: The skill-validation workflow comment was crashing with:
ReferenceError: agentCount is not defined
The code called badge('Agents', String(agentCount), ...) but never declared agentCount, so the comment posting failed.
Root Cause Confirmed (All 3 Reviewers):
agentCountvariable was missing- Producer (
static-checkjob) writesagent-count.txtbut consumer never reads it - Sparse-checkout includes
.github/agentsdirectory, so the count is valid
Verification (All 3 Reviewers Agreed)
✅ Exactly one declaration — const agentCount at line 899, no redeclaration SyntaxError
✅ Artifact path correct — sv-results/agent-count.txt → static-check-results artifact → static-results/ directory
✅ Safe fallback — Mirrors proven skillCount pattern with '?' fallback
✅ Catch block cannot re-throw — Uses only variables declared before the try block
✅ Fallback preserves marker — Upsert logic still works if count read fails
✅ YAML valid — No syntax errors
Independent Triangulation
All three models reviewed independently and reached the same conclusion:
- Opus: LGTM (1 optional suggestion about agent count glob pattern — verified as correct by GPT)
- GPT: LGTM (confirmed glob matches repository conventions, including non-
.agent.mdfiles) - Gemini: LGTM (verified fix resolves
ReferenceErrorand error handling is safe)
Cross-Pollination Notes
- Zero disagreements — All findings aligned
- Glob pattern confirmed correct — Two reviewers independently verified the broad
*.mdglob is intentional (countsmaui-expert-reviewer.mdwhich doesn't use.agent.mdsuffix) - Failing check explained — GPT noted the currently-failing CI run uses the old
pull_request_targetbase workflow, so the failure doesn't invalidate this PR - Defense-in-depth validated — Catch block can't re-throw because it only uses variables declared before the
try
The fix is correct, complete, and low-risk. Ready for merge.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
The bug
The Skill Validation workflow's
Post results commentjob was failing on itsPost commentstep with:The
github-scriptstep builds a badge array that referencesagentCount:…but
agentCountwas never defined in the script and never produced as an artifact, whereasskillCountis defined (readsstatic-results/skill-count.txt). The script threw while constructing the comment body — before any API call — so the results comment was never posted.This was introduced on 2026-06-16 in commit
1c0c0eb200(#35713, "Style skill validation results comment") and has silently broken the results comment on every Skill Validation run since.It is a real, deterministic workflow bug — not a permissions issue (the job already has
pull-requests: write+issues: write) and not a merge/close race (the failure is a JSReferenceError, not a 403).The fix
Self-contained to
.github/workflows/skill-validation.yml:Save results artifactstep (the one that writesskill-count.txt/spec-count.txt), add an agent count written toagent-count.txtin the samesv-results/dir, flowing through the samestatic-check-resultsupload/download wiring:Post commentgithub-script step, defineagentCountmirroringskillCount/specCountexactly (readstatic-results/agent-count.txtinside a try/catch that falls back to'?'), placed alongside the existing definitions so it's in scope before the badge array.try/catch. On any failure it logs viaconsole.errorandcore.warning(no silent swallowing) and posts a minimal plain-text fallback body (keeping the<!-- skill-validation-results -->marker so the upsert still finds/updates the comment). The results comment now always posts even if the badge formatting breaks.Validation
yaml.safe_load).agentCountis defined (script line ~898) before its only use in the badge array (line ~1178).node --check(balanced braces / valid scope) when wrapped in an async function (it uses top-levelawait, which github-script permits).The embedded JS can't be executed locally (depends on the
github/context/coreruntime), so dynamic behavior wasn't run; logic was reviewed by hand.