Feature/expert review workflow

[JanKrivanek]

Add two GitHub Agentic Workflows that invoke the expert-reviewer agent for code review: /review slash command + auto-trigger on PR open. Shared config via review-shared.md.

(recreated from #55)

[github-actions]

Expert Code Review — PR #56: Feature/expert review workflow

This PR adds two GitHub Agentic Workflows (auto-trigger on PR open + /review slash command) sharing a PAT-pool rotation action and a common prompt, backed by the expert-reviewer.md agent definition.

Applied all 24 review dimensions from .github/agents/expert-reviewer.md, weighted by file location (.github/ → Build Infrastructure Care, Security, Documentation, Scope discipline).

---

Dimension Summary

#	Dimension	Verdict
1	Backwards Compatibility	✅ LGTM — new files only
2	ChangeWave Discipline	✅ N/A — workflow infra
3	Performance & Allocation	✅ N/A — workflow infra
4	Test Coverage	⚠️ MINOR — no tests for bash action
5	Error Message Quality	✅ N/A — workflow infra
6	Logging & Diagnostics	✅ LGTM — ::warning:: annotation used correctly
7	String Comparison	✅ N/A — workflow infra
8	API Surface Discipline	✅ N/A — workflow infra
9	MSBuild Target Conventions	✅ N/A — workflow infra
10	Design Before Implementation	✅ LGTM — README serves as design doc
11	Cross-Platform Correctness	✅ LGTM — Linux-only runners, seq available
12	Code Simplification	⚠️ MINOR — one unquoted variable
13	Concurrency & Thread Safety	✅ N/A — workflow infra
14	Naming Precision	✅ LGTM — clear, consistent names
15	SDK Integration Boundaries	✅ N/A — workflow infra
16	Idiomatic C# Patterns	✅ N/A — workflow infra
17	File I/O & Path Handling	✅ LGTM — $GITHUB_OUTPUT quoted correctly
18	Documentation Accuracy	⚠️ MODERATE — agent-codebase mismatch
19	Build Infrastructure Care	✅ LGTM — all actions pinned to full SHAs
20	Scope & PR Discipline	✅ LGTM — coherent single concern
21	Evaluation Model Integrity	✅ N/A — workflow infra
22	Correctness & Edge Cases	⚠️ MODERATE — invalid RANDOM_SEED silent failure
23	Dependency Management	✅ LGTM — all versions SHA-pinned with tag comments
24	Security Awareness	✅ LGTM — fork protection, membership check, XPIA defense, minimal permissions

---

MODERATE

[MODERATE] Documentation Accuracy — Agent-Codebase Mismatch

File: .github/agents/expert-reviewer.md

The agent is titled "Expert MSBuild Reviewer" and its 24 dimensions, 12 knowledge areas, and folder hotspot mapping are explicitly MSBuild-specific (ChangeWave gating, binary log format, Evaluator.cs/Expander.cs hot paths, MSBuildNameIgnoreCaseComparer, etc.). This repository is ML.NET, not MSBuild — the agent will flag non-existent issues (e.g., "no ChangeWave gate") and miss relevant ML.NET concerns (e.g., IDataView cursor safety, TorchSharp tensor lifetime, ONNX model loading).

If this is intentional (testing in a fork before deploying to an MSBuild repo), a comment to that effect in the agent frontmatter would prevent confusion for future maintainers. Otherwise, the agent's knowledge areas and folder hotspot table need to be updated for ML.NET.

Recommendation: Either add a frontmatter comment noting this agent is designed for MSBuild repos, or update the agent's knowledge areas and hotspot table to reflect ML.NET's architecture.

---

[MODERATE] Correctness — Invalid RANDOM_SEED Silently Zeroes Randomness

File: .github/actions/select-copilot-pat/action.yml, line ~46

if [ -n "$RANDOM_SEED" ]; then
  RANDOM=$RANDOM_SEED   # ← no validation
fi
PAT_INDEX=$(( RANDOM % ${#PAT_NUMBERS[@]} ))

Scenario: A caller passes RANDOM_SEED=abc (non-numeric) or a negative value. Bash silently sets RANDOM to 0 when assigned a non-integer, producing a deterministic (always index 0) selection. No warning is emitted.

Recommendation: Validate the seed before use:

if [ -n "$RANDOM_SEED" ]; then
  if ! [[ "$RANDOM_SEED" =~ ^[0-9]+$ ]]; then
    echo "::warning::Invalid random-seed value '${RANDOM_SEED}'; ignoring and using system randomness."
  else
    RANDOM=$RANDOM_SEED
  fi
fi

---

MINOR

[MINOR] Code Simplification — Unquoted Array Expansion

File: .github/actions/select-copilot-pat/action.yml, line ~43

PAT_NUMBERS+=(${i})   # ← unquoted

Best practice is PAT_NUMBERS+=("${i}"). Functionally equivalent for single-digit numbers but avoids word-splitting on any future value expansion.

[MINOR] Test Coverage — No Tests for Composite Action

File: .github/actions/select-copilot-pat/action.yml

The bash logic (secret filtering, random selection, seed handling, empty-pool warning) has no automated tests. Given the security-sensitive nature of PAT selection, a simple BATS (Bash Automated Testing System) or GitHub Actions workflow test for the key scenarios (all empty, one non-empty, seed override, invalid seed) would increase confidence. Not blocking — workflow actions rarely ship with unit tests — but worth tracking.

---

NITs

• review-on-open.agent.md / review.agent.md: The <!-- Body provided by shared/review-shared.md --> comment is correct but may confuse contributors unfamiliar with the {{#runtime-import}} mechanism. A brief note pointing to the gh aw compile docs would help.
• Compiled lock files: The .agent.lock.yml files (1200+ and 1265+ lines each) are auto-generated. Including them is correct, but the large diff obscures the intentional changes in the 8-line .agent.md source files. Consider adding the lock files to .gitattributes with linguist-generated=true to reduce diff noise.

---

Security Observations (Informational — No Issues Found)

• ✅ Fork protection: review-on-open checks github.event.pull_request.head.repo.id == github.repository_id
• ✅ /review command gates on admin,maintainer,write role membership
• ✅ XPIA prompt-injection defense injected at workflow build time
• ✅ PAT secrets stay in pre_activation (trusted job); only the selected index number crosses job boundaries
• ✅ All poutine:ignore untrusted_checkout_exec suppressions are on scripts running from ${RUNNER_TEMP} (pre-installed tooling), not from untrusted checkout content

---

Verdict

COMMENT (no blocking or major issues). The two moderate findings are worth addressing before merge — particularly the agent-codebase mismatch, which will cause noisy or misleading reviews on ML.NET PRs. The RANDOM_SEED validation is a minor hardening improvement.

• Backwards Compat
• ChangeWave Discipline
• Performance
• Test Coverage (MINOR)
• Error Messages
• Logging
• String Comparisons
• API Surface
• MSBuild Targets
• Design
• Cross-Platform
• Code Simplification (MINOR)
• Concurrency
• Naming
• SDK Boundaries
• Idiomatic C#
• File I/O
• Documentation (MODERATE — agent-codebase mismatch)
• Build Infrastructure
• Scope Discipline
• Evaluation Model
• Correctness (MODERATE — invalid seed silent failure)
• Dependency Management
• Security

Generated by Expert Code Review (on open) for issue #56 · ● 1.1M · ◷