Enforce the eval count gate; read evals.path OR evals.paths - #1302
Conversation
….paths Companion to petry-projects/.github#779. Two things, both #755: 1. Read the new evals shape. `evals` now carries `path` (one set) OR `paths` (several, for a persona that wraps multiple skills — pr-review's deep-review + triage). check_evals handles both and requires dev/ + holdout/ splits in every set. Backward-compatible: qa-lead's single `path` still validates against both the live main schema and the #779 branch schema. 2. Make principle 5 partially real (finding 5). "No persona reaches stable without an eval gate" was documented but unenforced. Now: once a persona's status reaches its `evals.required_before` ring, each held-out set MUST carry >= `min_cases` real cases (blank lines ignored). A draft is exempt — draft is exactly when seed cases are placeholders. So a persona can no longer be promoted past required_before on the 5 synthetic starters. The SCORED gate (running the cases against a judge and requiring a pass rate) needs an eval harness and is deliberately NOT here — it is a separate follow-up. What is hermetically checkable (the held-out COUNT at promotion) is enforced now; what needs a runtime is tracked, not faked. Verification - bats tests/test_validate_personas.bats: 22/22 (16 + 6 new: paths all-splits, paths one-split-missing, draft-not-enforced, stable-enforced-fails, enough-cases-passes, blank-lines-ignored). - validate-personas.py against BOTH the live main schema and the #779 branch schema: OK (qa-lead is draft, so the count gate does not bite). Merge after #779 (the validator fetches the schema from .github main; until #779 lands, `evals.paths` is not yet permitted there — but qa-lead uses `path`, so main stays green either way). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 updates the persona validator to support multiple evaluation paths (evals.paths) and enforces a minimum case count (min_cases) for held-out evaluation sets once a persona reaches its designated promotion ring. Feedback on the changes includes a security recommendation to prevent path traversal when resolving evaluation directories, a performance optimization to read holdout files line-by-line rather than loading them entirely into memory, and a portability improvement to replace non-portable sed -i commands in the test suite with a redirect-and-rename pattern.
There was a problem hiding this comment.
Pull request overview
This PR updates the persona manifest validator to support both evals.path (single eval set) and evals.paths (multi-skill eval sets), and adds an enforcement gate that requires a minimum number of held-out eval cases once a persona reaches its evals.required_before ring (with draft explicitly exempt).
Changes:
- Add
check_evals()to validatedev/+holdout/splits for each eval set in eitherevals.pathorevals.paths. - Enforce
evals.min_casesfor held-out cases oncestatusreaches or passesevals.required_before(draft exempt). - Extend Bats coverage to exercise the new
evals.pathsshape and the count-gate behavior (including blank-line handling).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| personas/validate-personas.py | Adds support for evals.paths and implements the min_cases count gate tied to promotion rings. |
| tests/test_validate_personas.bats | Adds tests for multi-eval-set validation and for enforcing/ignoring the held-out case-count gate. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1302 |
|
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. |
* feat(personas): onboard dev-lead, pr-review, business-analyst as personas Formalizes the three existing agents under the persona standard and makes each addressable as @petry-projects/<role> (org team handles — teams created: dev-lead, pr-review, business-analyst; all closed, notifications disabled, 0 members). Closes the manifest half of #1281 / #1282 / #1283. Each persona (all status: draft): - personas/<id>/persona.yml — a compliant manifest documenting the REAL runtime: definition layers point at what exists (dev-lead → prompts/dev-lead/; pr-review → agents/pr-reviewer.md + prompts/; business-analyst → vendored bmad-method analyst v6.8.0 + prompts/incubation/). Triggers reflect reality, advisory by default, every write surface carries its gate_label (§4 rule 2). - prompts/<id>/advisory.md — a role-focused advisory the shared persona runner serves on @-mention (read-only, prints between sentinels, marker-first — the qa-lead contract). dev-lead advises on implementation/scoping (code is still written only by its label-triggered runtime, not this path); pr-review gives review guidance (the full tiered review runs on its existing path); business-analyst advises on framing/research/next-analysis-step. - README.md + seed held-out evals (draft-exempt from the count gate; expand before promotion). pr-review points evals.paths at its EXISTING skill sets (evals/deep-review/, evals/triage/) — the multi-skill case #779 unblocks. Decisions taken (from the onboarding issues): - @-addressability for all three (the initiative's goal). pr-review's legacy @donpetry-bot path is retained (real account, works); this adds the role handle. - business-analyst is the Analyst role (brainstorm/market-research/brief) ONLY; the PRD stays with a separate product-manager persona — the documented Analyst→PM handoff is preserved, not collapsed. - pr-review's canary-rings registration (#710) stays a separate follow-up; draft personas don't require it. Deliberately NOT here (tracked, per-issue): real (non-seed) eval sets; the product-manager persona; pr-review canary registration; and — for the mention paths to go live — enrolling these into the router soak (the framework is live only in the next ring today). MERGE ORDER: after petry-projects/.github#779 (schema: evals.paths) and #1302 (validator: reads paths + count gate). pr-review's manifest uses `paths`, which main's validator cannot parse until #1302 lands. Verification - validate-personas.py (with #779 schema + #1302 validator): 4/4 valid, invariants hold. - validate-cases.py: OK, 7 skills / 54 cases, no cross-split id overlap. - markdownlint: clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: dev-lead update (review-changes) [skip ci-relay] * chore: dev-lead update (review-changes) [skip ci-relay] * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 2de26138930a737d7fd71b0a12918b0ba43ef652
Review mode: triage-approved (single reviewer)
Summary
Adds an enforced eval-count gate to the persona validator: check_evals() reads both the legacy evals.path and the new evals.paths shape (companion petry-projects/.github#779, now merged), requires dev/ + holdout/ splits per set, and once a persona reaches its required_before ring requires >= min_cases non-blank held-out cases (draft exempt). 6 new bats tests cover both shapes and the gate boundaries. Triage assessment (low-risk, well-scoped) confirmed.
Linked issue analysis
No closing issue references. The PR body cites the remaining eval decisions from #755 (merged) and the companion schema change petry-projects/.github#779, which merged at 2026-07-18T17:04Z — the stated 'merge after #779' sequencing constraint is satisfied. The change substantively delivers the enforceable half of principle 5 as described.
Findings
- All 9 prior review threads (gemini, copilot) are resolved: the path-traversal concern is fixed with a resolve()+relative_to() containment check that fail()s (NoReturn) on absolute/../ paths; holdout counting now streams line-by-line; all sed -i uses replaced with portable redirect-and-rename.
- Minor, non-blocking: under a deliberately loose/stale --schema, a non-mapping evals value would raise AttributeError at evals.get() rather than a fail() diagnostic (the contract handle_slug follows). Schema validation guards this in normal operation; the copilot thread on it is resolved/outdated.
- Ring-order defaults (required_before -> stable, unknown status -> draft) fail open only for schema-invalid states, which the schema check rejects first.
- MCP secret-scanning tool unavailable in this run; gitleaks CI check is green.
CI status
All required checks green (unit-tests, bats, validate-personas, ShellCheck, CodeQL, SonarCloud, gitleaks, actionlint, agent-shield, CodeRabbit). Skipped checks are conditional dependency-audit/dependabot jobs not applicable to this diff.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
* feat(personas): onboard scrum-master (missed by #1303's auto-merge race) #1303 auto-merged on its 3-persona state moments before the scrum-master commit landed, so scrum-master never reached main (the squash even kept the old 3-persona title). The team petry-projects/scrum-master already exists; this lands the files. - personas/scrum-master/persona.yml — draft; framework-agent → vendored bmad-method sprint-planning (v6.8.0) with local_overrides → the org wrapper prompts/bmad/scrum-master.md, plus workflow-prompts → prompts/bmad. Advisory on mention/issues/discussion. Role, not a person (§1.6). - prompts/scrum-master/advisory.md — decomposition/scoping/sequencing advice (read-only/sentinel/marker contract). - README + seed held-out evals (churn-trap cases: too-coarse, missing AC, cyclic blocked_by, and a "don't over-engineer a sound plan" negative). Verification (against LIVE main — #779 schema + #1302 validator both merged): - validate-personas.py: 5/5 valid, invariants hold. - validate-cases.py: OK, 9 skills / 75 cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(bot): address bot feedback [skip ci-relay] * fix(bot): address bot feedback [skip ci-relay] --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
Companion to petry-projects/.github#779. Resolves the remaining eval decisions in #755.
1. Reads the new evals shape.
check_evalshandles bothpath(one set) andpaths(several, for multi-skill personas — pr-review's deep-review + triage), requiring dev/ + holdout/ splits in each. Backward-compatible: qa-lead's singlepathvalidates against both the live main schema and the #779 branch schema.2. Makes principle 5 partially real (finding 5). "No persona reaches stable without an eval gate" was documented but unenforced —
canary-rolloutnever readevals/. Now: once a persona's status reaches itsrequired_beforering, each held-out set must carry ≥min_casesreal cases (blank lines ignored). Draft is exempt — that's when seed cases are placeholders. So a persona can no longer be promoted pastrequired_beforeon the 5 synthetic starters.The scored gate (running cases against a judge) needs an eval harness and is deliberately a separate follow-up — what's hermetically checkable (the count at promotion) is enforced now; what needs a runtime is tracked, not faked.
bats tests/test_validate_personas.bats— 22/22 (6 new)Merge after #779. qa-lead uses
path, so main stays green regardless of order.🤖 Generated with Claude Code