feat: implement issue #686 — canary #668 increment 5: Layer 0 — pure decision cores + codified 'decision logic in a tested script' standard#687
Conversation
…decision cores + codified 'decision logic in a tested script' standard
|
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: 41 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)
📝 WalkthroughWalkthroughThe PR extracts PR readiness gating into a pure, tested shell function, updates the reusable workflow to delegate decisions to it, expands decision-matrix coverage, and documents the pure decision-core standard. ChangesPR readiness decision flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant ReadinessCore
participant ChecksGate
participant GitHub
Workflow->>GitHub: Gather PR metadata, checks, and thread count
Workflow->>ReadinessCore: Pass collected PR facts
ReadinessCore->>ChecksGate: Evaluate required checks
ChecksGate-->>ReadinessCore: Return checks readiness
ReadinessCore-->>Workflow: Return decision class and exit status
Workflow-->>GitHub: Emit decision output and dispatch or skip
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 unified, pure readiness core function pr_auto_review_ready in ready-check.sh to evaluate PR readiness criteria, along with comprehensive BATS unit tests and updated documentation across several files. The review feedback highlights a potential Bash syntax error when parsing malformed or null JSON inputs with jq and suggests a robust fallback, as well as recommending additional test cases to cover these null-safety paths.
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 114-131: Update the “Decision Logic Lives in a Pure, Tested
Script” guidance to allow pure scripts under both scripts/** and
.github/scripts/**, matching the cited PR auto-review exemplar. Remove the
duplicated section from standards/agent-standards.md and replace it with a
cross-reference to the canonical guidance in AGENTS.md, preserving a single
source of truth.
In `@standards/agent-standards.md`:
- Around line 112-134: Replace the duplicated “Decision-Making Reusables — Pure,
Tested Decision Cores” guidance and exemplar table in
standards/agent-standards.md with a concise cross-reference to the canonical
AGENTS.md decision-core section, following the adjacent “BMAD Method Workflows”
convention. Preserve a clear pointer to that source without repeating its rules,
examples, or payoff text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42912ee6-7db0-42e1-b478-5493a9bdabf7
📒 Files selected for processing (6)
.github/scripts/pr-auto-review/README.md.github/scripts/pr-auto-review/lib/ready-check.sh.github/workflows/pr-auto-review-reusable.ymlAGENTS.mdstandards/agent-standards.mdtest/workflows/pr-auto-review/ready.bats
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
|
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
Dev-Lead — waiting on PR blockers (intent: fix-reviews)PR: #687 |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 1f630e01876b38c5bfdb9e1c5fcf2d7475397ade
Review mode: triage-approved (single reviewer)
Summary
Refactors the pr-auto-review readiness gate: criteria #1/#3/#4 move from inline workflow bash into a pure, unit-tested function (pr_auto_review_ready) in ready-check.sh, with the workflow reduced to I/O glue. Adds a 17-case decision-matrix bats suite and codifies the 'decision logic in a pure, tested script' standard in AGENTS.md (cross-referenced from standards/agent-standards.md). Fully implements issue #686 with no behavior change to the decision classes or the #680 required-checks gate.
Linked issue analysis
Closes #686 (increment 5 of #668). All acceptance criteria are met: (1) criteria #1/#3/#4 decision logic lives in pure functions in ready-check.sh and check-and-dispatch is thin I/O glue; (2) decision= outputs are preserved as literal strings (Layer 2 telemetry + canary structural grep unaffected) and the #680 required-vs-non-required gate is delegated verbatim to pr_auto_review_checks_ready; (3) full decision-matrix bats coverage including precedence, null/malformed inputs, no-checks, non-required-advisory-does-not-block (#680 pin), and self-check exclusion; (4) standard codified in AGENTS.md with the canary engine and ready-check.sh as exemplars; (5) ShellCheck green.
Findings
No blocking findings.
- Correctness: jq null/malformed-JSON handling in pr_auto_review_ready is sound (type guard + empty-output check, both covered by tests). Criterion precedence matches the prior inline behavior and is test-pinned.
- Security: no secrets, no new untrusted-input interpolation in the workflow; ${{ }} expressions used are safe contexts (github.repository, github.run_id). Secret-scanning MCP tool was unavailable this run; the gitleaks CI check passed and the diff contains only shell/test/markdown content.
- Minor (non-blocking): a non-numeric UNRESOLVED_COUNT would make the [ -gt ] test error and yield an empty decision class — unreachable in practice since the workflow derives it from a GraphQL length expression.
- Prior review threads (gemini null-safety, CodeRabbit standards-duplication) are all resolved; CodeRabbit's latest review is APPROVED.
- Note: the workflow now gathers all PR facts (checks, branch rules, GraphQL threads) before deciding, so draft/closed PRs incur a few extra API calls — an acceptable tradeoff for the pure-core design.
CI status
All required checks green on 1f630e0: Lint, ShellCheck, CodeQL, SonarCloud, agent-shield / AgentShield, Detect ecosystems, Agent Security Scan — plus Lint and bats (PR Auto-Review Tests), Secret scan (gitleaks), npm audit, CodeRabbit. The CANCELLED dev-lead / dispatch and dev-lead / ci-relay entries are non-required advisory agent checks and do not gate merge (confirmed against the main branch required-status-check rules).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 1f630e01876b38c5bfdb9e1c5fcf2d7475397ade
Review mode: triage-approved (single reviewer)
Summary
Refactor per issue #686 (canary #668 increment 5): extracts the remaining pr-auto-review readiness criteria (#1 open/non-draft, #3 CHANGES_REQUESTED, #4 unresolved threads) from inline workflow bash into a pure, unit-tested decision core (pr_auto_review_ready in ready-check.sh), adds a 20-case decision-matrix bats suite, and codifies the 'decision logic in a tested script' standard in AGENTS.md with a cross-reference in standards/agent-standards.md. Triage assessment confirmed: no security-sensitive surface touched, behavior-preserving refactor, all findings from prior tool reviews resolved.
Linked issue analysis
Issue #686 acceptance criteria are all substantively addressed: (1) criteria #1/#3/#4 now live in ready-check.sh as the pure pr_auto_review_ready function and check-and-dispatch is thin I/O glue; (2) decision= outputs are preserved and emitted as literal strings so the Layer 2 telemetry / canary structural greps keep working, and the #680 required-vs-non-required gate is delegated verbatim to pr_auto_review_checks_ready with a dedicated pin test; (3) the full decision matrix is covered in test/workflows/pr-auto-review/ready.bats (happy path, each skip class, no-checks/empty/null/malformed JSON, precedence ordering, self-check exclusion) under the PR Auto-Review Tests workflow (Lint and bats: SUCCESS); (4) the standard is codified in AGENTS.md citing both exemplars, with agent-standards.md cross-referencing instead of duplicating (per resolved CodeRabbit feedback); (5) ShellCheck green.
Findings
No blocking findings.
- Security: the workflow diff removes inline logic and calls the sourced lib; no new permissions, secrets, or untrusted-input interpolation into shell. CodeQL (actions), gitleaks, AgentShield, Agent Security Scan all green. The run_secret_scanning MCP tool was not available in this environment; the gitleaks CI check (SUCCESS) covers secret detection.
- Correctness: null-safety of CHECKS_JSON is handled (jq type guard + empty-string check) and tested for empty/null/malformed inputs; UNRESOLVED_COUNT defaults to 0. Precedence semantics (earlier criterion wins) are explicit and tested.
- Minor (non-blocking): the glue now gathers all facts (gh pr checks, branch rules, GraphQL threads) before deciding, so draft/closed PRs incur a few extra read-only API calls that were previously short-circuited. Decisions are unchanged; acceptable cost for the pure-core design.
- Prior tool reviews: CodeRabbit's CHANGES_REQUESTED was addressed and superseded by its APPROVED review; all 4 review threads (gemini x2, coderabbit x2) are resolved.
CI status
All required checks green: Lint, ShellCheck, Lint and bats (PR Auto-Review Tests), CodeQL, Analyze (actions), Secret scan (gitleaks), SonarCloud, npm audit, Agent Security Scan, agent-shield — all SUCCESS. CANCELLED entries are non-required dev-lead dispatch/ci-relay advisory contexts; SKIPPED entries are ecosystem audits not applicable to this repo. reviewDecision: APPROVED; no unresolved review threads.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
2 participants
Closes #686
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests