feat: implement issue #747 — [Phase 1] Make the Skill Eval Report visible — surface regression/error outcomes (no longer silent)#754
Conversation
…ible — surface regression/error outcomes (no longer silent)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
More reviews will be available in 23 minutes and 30 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new bash script notify-eval-health.sh along with comprehensive BATS tests to manage and de-duplicate GitHub tracking issues for daily Skill Eval outcomes. The script handles pass, regression, and error outcomes, updating or closing tracking issues accordingly. The review feedback highlights a robustness issue where the script could crash under set -e if the report JSON is valid but incomplete (e.g., missing .failed or .cases keys), and provides a code suggestion to safely default these values in jq.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-bot-comment (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: c622d1437ae6703ab2b1609d44ebae95e912a326
Review mode: triage-approved (single reviewer)
Summary
Phase-1 visibility layer for the daily Skill Eval Report (#747): a new offline-tested notifier (notify-eval-health.sh) opens/updates/closes a single de-duplicated eval-health tracking issue on regression/error and stays silent on healthy passes, plus a least-privilege issues:write grant and notify step wired into skill-eval-report.yml. Confirms the triage low-risk assessment.
Linked issue analysis
Closes #747. All acceptance criteria are satisfied: (1) regression/error produces a visible, de-duplicated tracking issue (not just an Actions annotation); (2) healthy pass with no open issue makes zero noise; (3) workflow stays non-blocking — no gating added; (4) least-privilege: only issues:write added, contents:read retained for checkout, with tests asserting no contents:write / write-all; (5) an offline stub test (tests/test_eval_health_notify.bats, no network) covers open/update/close + de-dupe and is wired into lint.yml.
Findings
No blocking findings.
- Security: workflow triggers only on schedule + workflow_dispatch (no pull_request_target), top-level permissions: {}, job re-grants only issues:write. Notify-step env uses trusted GitHub context (github.token, github.repository, steps.eval.outputs.outcome validated by the script's case guard) — no untrusted-input injection surface. Script quotes variables, uses mktemp + --body-file, and ignores label-create failures idempotently.
- Resolved review thread: gemini-code-assist flagged a potential jq crash on a valid-but-incomplete report under set -e. Fixed in notify-eval-health.sh via // defaults (.score // "N/A", .failed // "0", etc.) and a guarded array iteration (.cases | select(type == "array") // []). Thread is resolved.
- Idempotency/de-dupe: lookup keys on a stable per-skill title scoped by the eval-health label and open state; verified by tests for create-once, update-in-place, recovery-close, and no-op-on-healthy.
CI status
All required checks green. SUCCESS: Lint, shellcheck/ShellCheck, bats, unit-tests, CodeQL (actions + python), agent-shield/AgentShield, Agent Security Scan, Secret scan (gitleaks), SonarCloud, validate-agent-profiles, gh-aw-compile, review, CodeRabbit. Benign SKIPPED: dependency-audit ecosystem jobs, dependabot-automerge, dev-lead/ci-relay. No failing checks.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #754 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
|
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 669fd6c47fab4c98ad351718f08eb7c86d0dd552
Review mode: triage-approved (single reviewer)
Summary
Phase-1 visibility layer for the daily Skill Eval Report (#747): a new offline-tested notifier (scripts/evals/notify-eval-health.sh) opens/updates/closes a single de-duplicated eval-health tracking issue on regression/error and stays silent on healthy passes, plus a least-privilege issues:write grant and an always() notify step wired into skill-eval-report.yml. Confirms the triage low-risk assessment; the prior bot approval at c622d14 still holds since the only intervening commits are merge-from-main (no change to the PR's own 5 files).
Linked issue analysis
Closes #747. Acceptance criteria satisfied: (1) regression/error produces a visible, de-duplicated tracking issue rather than just an Actions annotation; (2) a healthy pass with no open issue makes zero noise, and a pass after a prior open issue comments + closes it (recovery); (3) the workflow stays non-blocking — no gating added, notify step is report-only; (4) least-privilege — only issues:write is added while contents:read is retained for checkout, with tests asserting no contents:write and no write-all; (5) an offline stub test (tests/test_eval_health_notify.bats, no network) covers open/update/close + de-dupe and is wired into lint.yml.
Findings
No blocking findings.
- Security: skill-eval-report.yml triggers on schedule + workflow_dispatch only (no pull_request_target). Notify-step env uses trusted GitHub context (github.token, github.repository, github.run_id) and steps.eval.outputs.outcome, which the script validates via an explicit case guard before use — no untrusted-input injection surface. The script runs set -euo pipefail, quotes variables, uses mktemp + --body-file, and idempotently ignores label-create failures.
- Resolved review thread: gemini-code-assist flagged a high-priority jq crash under set -e on a valid-but-incomplete report (missing .failed/.cases). The current head fixes it via jq // defaults (.score // "N/A", .failed // "0", etc.) and a guarded array iteration ((.cases | select(type == "array") // [])). Verified resolved at the reviewed SHA.
- Idempotency/de-dupe: lookup keys on a stable per-skill title scoped by the eval-health label and open state; bats tests verify create-once, update-in-place, recovery-close, and no-op-on-healthy.
- SonarCloud Quality Gate passed (0 new issues, 0 security hotspots).
CI status
All required checks green. SUCCESS: Lint, shellcheck/ShellCheck, bats, unit-tests, validate-agent-profiles, gh-aw-compile, Compile agentic workflows, CodeQL (actions + python), agent-shield/AgentShield, Agent Security Scan, Secret scan (gitleaks), holdout-guard, SonarCloud + SonarCloud Code Analysis, CodeRabbit. Benign SKIPPED: dependency-audit ecosystem jobs (npm/pnpm/pip/cargo/go), dependabot-automerge. CANCELLED: dev-lead/dispatch and dev-lead/ci-relay — these are the Dev-Lead Agent's own concurrency-cancelled jobs (known-benign per #608/#609), not real CI failures. review/review is this in-progress run. No failing checks.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Two regressions from the review-agent's push (b6f12fa): 1. It DELETED the CRLF test that Gemini specifically requested and that guards the '\r'-stripping fix. Restored — without it, a future revert of the strip would pass CI silently. 2. It changed the prompt to `gh api "$COMMENT_URL"` (correct target: an API url), but the router (#754, merged) currently sends `.html_url` — and `gh api` cannot fetch a github.com browser url, so the comment-body read fails. The prompt already degrades gracefully ("if that fails, proceed from the item"), so the soak still works; but the contract must be made real. The fix is in the ROUTER, not here: it should send the comment's `.url` (API) not `.html_url`. Companion PR on petry-projects/.github does that. Documented the contract in the prompt so the two sides agree. Verification - bats tests/persona_runner.bats: 22/22 (CRLF test back). - shellcheck --severity=warning -x: clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(personas): the persona runtime — dispatch side of the mention framework Closes the loop the router opened. petry-projects/.github#754 detects an `@petry-projects/<role>` mention and fires a `persona-mention` repository_dispatch at this repo; nothing listened. This is what listens. Architecture: ONE runner, single-homed The runner is NOT fanned out to the fleet. Every mention across every repo funnels through the router's dispatch to THIS repo, so the runner lives only here and is called locally (uses: ./). That is why it needs no channel tag or canary registration — unlike the router (`persona-mention`), which IS consumed by fleet repos and rolls out through the rings. One runner serves every persona, convention-driven (prompts/<id>/advisory.md), never one workflow per persona (§4.1: per-agent workflows are the drift the framework exists to prevent). The recursion marker is closed from BOTH sides The router skips any comment carrying '<!-- persona:'. This runner's prompt and its pr_comment_has_marker guard REQUIRE every advisory's first line to be '<!-- persona:<id> -->'. So a persona's own comment can never re-summon it or any persona named in the thread — the .github-private#860 failure (1,481 acks in 4.5h), closed from the writing side. Contents - prompts/qa-lead/advisory.md — qa-lead's advisory behaviour: read the vendored bmad-tea skill by path, assess test-risk (risk vs value, no manufactured risk), post ONE advisory comment. Advisory only — no writes, no PRs, no labels. - scripts/lib/persona-runner.sh — the pure core: payload re-validation (this runner is a SEPARATE trust boundary — a repository_dispatch can be sent by anything with a token, so a malformed persona id must never reach a path interpolation), advisory-prompt resolution by convention, and the shared marker. - .github/workflows/persona-runner-reusable.yml — the runtime: resolve prompt → install claude-code → run headless → the prompt posts one marked comment. PAT (not github.token) because the item lives in a DIFFERENT repo. Rate-limit is a soft skip, matching ci-failure-analyst. - .github/workflows/persona-runner.yml — thin caller: maps the dispatch payload to the reusable's inputs, nothing else. - tests/persona_runner.bats — 15 tests, weighted to the marker and payload re-validation (path traversal, bad ids, missing prompt → rc 3 not a crash). - lint.yml — wires the suite into CI (a bats suite with no runner is unenforced). A persona with no prompts/<id>/advisory.md yet is a clean no-op ("no runtime wired"), not a failure — a persona may declare an address before its runtime exists. NOT wired live here. persona-runner-reusable.yml is workflow_call-only, so per the org standard it must be set disabled_manually after merge (GitHub ignores the flag for workflow_call). And no mention reaches it until the router release is cut and .github-private adopts the `persona-mention` stub — the `next`-ring enablement, which follows in sequence. This PR is the runtime; turning it on is the next step. Verification - bats tests/persona_runner.bats: 15/15. - shellcheck --severity=warning -x scripts/lib/persona-runner.sh: clean. - actionlint (CI's ignores) on both workflows: clean. - prompts/** is a markdownlint ignore (agent instructions, not prose). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reviews): address review comments [skip ci-relay] * fix(personas): make the recursion marker MECHANICALLY enforced (review) Copilot's most important finding: the agent posted its OWN comment, so the marker was only prompt-REQUESTED, not enforced. An agent that forgot it would post an unmarked comment → self-summon → #860 (1,481 acks in 4.5h). My "closed from both sides" claim was false. Restructured, not patched. Read/write split - The AGENT now runs with github.token, which reads the PUBLIC source repo but CANNOT post to it (scoped to THIS repo). It PRINTS the advisory between sentinels; it never receives GH_PAT_WORKFLOWS. - The WORKFLOW extracts the body, guarantees the marker (pr_ensure_marker — prepends if the agent omitted it), and is the ONLY writer, using the PAT. So an agent that forgets the marker cannot post an unmarked comment because it cannot post at all. The guard is now mechanical. New helpers pr_extract_advisory / pr_ensure_marker, tested to the invariant "the posted body always starts with the marker". Other review fixes - Gemini (high): pr_comment_has_marker kept the trailing '\r' from GitHub's CRLF bodies, so the guard silently never matched. Strip it; CRLF test added. - Copilot: resolve step conflated rc 2 (malformed/hostile id) with rc 3 (no runtime). Now rc 2 FAILS loudly, rc 3 is the benign skip. - Copilot: concurrency group keyed only on item_number, which is empty for discussions → all discussion mentions for one persona+repo collapsed into one group. Added comment_url to the key. - Copilot: prompt used $COMMENT_URL (an html_url) with `gh api`, referenced an undefined $BODY, and put a parenthetical on the marker line. Rewritten: the agent derives the API url, prints between sentinels (no self-post, no $BODY), and the marker is alone on the first body line. - Gemini: `head -400` → `head -n 400 || true` (POSIX + no SIGPIPE under pipefail). Discussion posting is explicitly a no-op-with-notice for now (the issue-comments endpoint doesn't serve discussions); wired when the discussion path lands. Verification - bats tests/persona_runner.bats: 21/21 (15 + 6: CRLF, sentinel extraction x2, marker-prepend, unchanged-when-present, and the post-body invariant). - shellcheck --severity=warning -x scripts/lib/persona-runner.sh: clean. - actionlint (CI ignores) on both workflows: clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reviews): address review comments [skip ci-relay] * fix(personas): restore the CRLF test; pin the COMMENT_URL contract Two regressions from the review-agent's push (b6f12fa): 1. It DELETED the CRLF test that Gemini specifically requested and that guards the '\r'-stripping fix. Restored — without it, a future revert of the strip would pass CI silently. 2. It changed the prompt to `gh api "$COMMENT_URL"` (correct target: an API url), but the router (#754, merged) currently sends `.html_url` — and `gh api` cannot fetch a github.com browser url, so the comment-body read fails. The prompt already degrades gracefully ("if that fails, proceed from the item"), so the soak still works; but the contract must be made real. The fix is in the ROUTER, not here: it should send the comment's `.url` (API) not `.html_url`. Companion PR on petry-projects/.github does that. Documented the contract in the prompt so the two sides agree. Verification - bats tests/persona_runner.bats: 22/22 (CRLF test back). - shellcheck --severity=warning -x: clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
Registering `persona-mention` in canary-rings.json (#754) is not enough to cut its release: cut-release.sh keeps its OWN hardcoded agent allowlist (valid_agent / cross_repo_agent), and the router was absent from it — so `cut-release.sh persona-mention 1.0.0 --channel next` failed with "unknown agent". This is the two-registry drift the framework fights, in the release tooling. persona-mention is a cross-repo agent exactly like pr-review-mention: its reusable lives in petry-projects/.github, this repo holds only the caller stub, so its release/channel tags resolve against .github (CROSS_REPO_TARGET). Added it beside pr-review-mention in cross_repo_agent(), the usage comment, and the unknown-agent error string. Verification - bats tests/test_cut_release.bats: 61/61 (2 new: valid_agent + cross_repo_agent accept persona-mention). - shellcheck --severity=warning -x scripts/cut-release.sh: clean. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Closes #747
Implemented by dev-lead agent. Please review.