ci: audit issue lifecycle without mutation - #470
Conversation
|
Warning Review limit reached
Next review available in: 22 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a read-only Ruby CLI that audits issue metadata and pull-request closure semantics using live GitHub data or JSON fixtures. Adds Markdown reporting, exit codes, and a Bash harness covering valid, invalid, and stubbed live cases. ChangesLifecycle audit
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant GitHubReadOnly
participant audit_pull_request
participant render
CLI->>GitHubReadOnly: load_live(repo, pr_number)
GitHubReadOnly->>CLI: issues, pull request, commits, default branch
CLI->>audit_pull_request: validate relationship and closing semantics
audit_pull_request->>render: violations
render->>CLI: Markdown PASS or FAIL report
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/scripts/issue-lifecycle-audit.rb (3)
35-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden the
ghinvocation: stderr string matching and no timeout.Two points (note: the ast-grep command-injection hint is a false positive — the command is passed as an argument vector, so no shell is involved):
stderr.include?("HTTP 404")couples 404 detection togh's human-readable output; a wording change silently turns "issue not visible" into a hard audit error.capture3has no timeout, so a hungghcall hangs the audit indefinitely.♻️ Suggested direction
- def get(path, allow_404 = false) + def get(path, allow_404: false) stdout, stderr, status = Open3.capture3( "gh", "api", "--method", "GET", "-H", "Accept: application/vnd.github+json", "-H", "X-GitHub-Api-Version: #{API_VERSION}", - path + "-i", path ) - return nil if allow_404 && !status.success? && stderr.include?("HTTP 404") + return nil if allow_404 && !status.success? && stderr.match?(/\bHTTP\s*404\b/)Wrapping the call in
Timeout.timeout(orgh --max-time-equivalent supervision) also avoids an unbounded stall.🤖 Prompt for 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. In @.github/scripts/issue-lifecycle-audit.rb around lines 35 - 46, Harden get by detecting allowed 404 responses from a stable structured status signal rather than matching stderr text, while preserving the existing nil result for allow_404. Wrap the Open3.capture3 invocation in a timeout using the script’s established timeout constant or configuration, and ensure timeout failures propagate as clear request errors instead of hanging indefinitely.Source: Linters/SAST tools
216-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer capture-group destructuring over
Regexp.last_match.The enumerator + global-match-state idiom works but depends on
$~being refreshed per iteration; directscandestructuring is equivalent and self-evident.♻️ Proposed refactor
- text.to_s.to_enum(:scan, CLOSING_PATTERN).map do - match = Regexp.last_match - { "keyword" => match[1], "repository" => match[2], "number" => match[3].to_i } - end + text.to_s.scan(CLOSING_PATTERN).map do |keyword, repository, number| + { "keyword" => keyword, "repository" => repository, "number" => number.to_i } + end🤖 Prompt for 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. In @.github/scripts/issue-lifecycle-audit.rb around lines 216 - 221, Update closing_keywords to destructure the capture groups directly from each CLOSING_PATTERN match returned by scan, removing the Regexp.last_match dependency while preserving the keyword, repository, and integer-converted number fields.
316-343: 🚀 Performance & Scalability | 🔵 TrivialFull-repo mode scales linearly in sequential API calls.
Each open PR costs a detail call plus paginated commits, and each audited issue costs a hydration call. On a busy repo this is hundreds of serial
ghinvocations with no rate-limit handling or backoff. Consider a secondary-rate-limit-aware retry and (later) batching via GraphQL when this graduates to a scheduled workflow.🤖 Prompt for 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. In @.github/scripts/issue-lifecycle-audit.rb around lines 316 - 343, Update load_live and its GitHubReadOnly API-call flow to add secondary-rate-limit-aware retries with backoff for detail, pagination, and issue hydration requests, while preserving existing results and error behavior for non-rate-limit failures. Do not address GraphQL batching in this change..github/scripts/test-issue-lifecycle-audit.sh (2)
173-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the exit-2 ERROR contract.
The harness verifies exit 0 and exit 1 but never exit 2, so the auditor's ERROR path (unknown case,
--fixturewithout--case,--fixturecombined with--repo) is untested — including the summary-file write on error.💚 Suggested helper
+run_error() { + local expected="$1" + shift + local output audit_exit + set +e + output="$(ruby "$auditor" "$@" 2>&1)" + audit_exit=$? + set -e + test "$audit_exit" -eq 2 + grep -Fq "$expected" <<<"$output" +}Then, for example:
run_error "--fixture requires --case" --fixture "$tmp_dir/cases.json".🤖 Prompt for 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. In @.github/scripts/test-issue-lifecycle-audit.sh around lines 173 - 196, Add a run_error helper alongside run_pass and run_fail in the audit test script to execute the auditor with arbitrary arguments, assert exit status 2, verify the expected error text, and confirm it is written to the summary file when applicable. Add cases covering an unknown case, --fixture without --case, and --fixture combined with --repo.
242-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo stub branches are unreachable, and full-repo live mode is never exercised.
With issue 10 already returned by the issues list and
total_blocked_by= 0, the auditor never requestsrepos/example/repo/issues/10or.../dependencies/blocked_by, so those branches only look like coverage. Combined with the fact that only--pr 88is run, the hydration paths (including the parent lookup flagged in.github/scripts/issue-lifecycle-audit.rblines 113-124) and the full-repository mode advertised in the PR objectives are untested.Suggest adding a second live stub scenario:
--repo example/repowith no--pr, an issue that is not in the issues-list page (forcing the per-issue fetch), and a non-zerototal_blocked_by(forcing the dependency fetch) — plus a*)branch assertion so unexpected paths fail loudly, which it already does.🤖 Prompt for 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. In @.github/scripts/test-issue-lifecycle-audit.sh around lines 242 - 280, The live audit test currently exercises only PR mode, leaving per-issue hydration and dependency fetching untested. Extend the stub data and test flow around the live_stub_dir/gh fixture to add a full-repository invocation without --pr, using an issue absent from the issues-list response and with non-zero issue_dependencies_summary.total_blocked_by so repos/example/repo/issues/<id> and its dependencies endpoint are requested; assert the expected exit/output, while retaining the existing unexpected-path failure branch.
🤖 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 @.github/scripts/issue-lifecycle-audit.rb:
- Around line 378-401: Move the OptionParser construction and parse! call into
the existing begin/rescue flow in the script, keeping the options and parser
definitions unchanged. Ensure OptionParser::ParseError from invalid values or
unknown flags is handled by the same error-reporting path that emits the ERROR
summary and returns the dedicated CLI-error exit status rather than the
violations status.
---
Nitpick comments:
In @.github/scripts/issue-lifecycle-audit.rb:
- Around line 35-46: Harden get by detecting allowed 404 responses from a stable
structured status signal rather than matching stderr text, while preserving the
existing nil result for allow_404. Wrap the Open3.capture3 invocation in a
timeout using the script’s established timeout constant or configuration, and
ensure timeout failures propagate as clear request errors instead of hanging
indefinitely.
- Around line 216-221: Update closing_keywords to destructure the capture groups
directly from each CLOSING_PATTERN match returned by scan, removing the
Regexp.last_match dependency while preserving the keyword, repository, and
integer-converted number fields.
- Around line 316-343: Update load_live and its GitHubReadOnly API-call flow to
add secondary-rate-limit-aware retries with backoff for detail, pagination, and
issue hydration requests, while preserving existing results and error behavior
for non-rate-limit failures. Do not address GraphQL batching in this change.
In @.github/scripts/test-issue-lifecycle-audit.sh:
- Around line 173-196: Add a run_error helper alongside run_pass and run_fail in
the audit test script to execute the auditor with arbitrary arguments, assert
exit status 2, verify the expected error text, and confirm it is written to the
summary file when applicable. Add cases covering an unknown case, --fixture
without --case, and --fixture combined with --repo.
- Around line 242-280: The live audit test currently exercises only PR mode,
leaving per-issue hydration and dependency fetching untested. Extend the stub
data and test flow around the live_stub_dir/gh fixture to add a full-repository
invocation without --pr, using an issue absent from the issues-list response and
with non-zero issue_dependencies_summary.total_blocked_by so
repos/example/repo/issues/<id> and its dependencies endpoint are requested;
assert the expected exit/output, while retaining the existing unexpected-path
failure branch.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 44570434-77ec-4f24-affc-38e52cb66967
📒 Files selected for processing (2)
.github/scripts/issue-lifecycle-audit.rb.github/scripts/test-issue-lifecycle-audit.sh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4a8b2d7de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Issue relationship
Closes #458
Change contract
Add one read-only auditor and focused fixture/live checks for issue metadata, native relationship, and PR-closing lifecycle rules, without adding a workflow entrypoint or mutating repository state.
Out of scope
GitHub Actions workflow entrypoint (#467), required-check enforcement (#460), historical metadata reconciliation (#459), and any issue, pull-request, label, or repository-setting mutation.
Dependency / merge order
#456 is merged. #467 follows only after this auditor lands; it owns the observational workflow and stable check.
Focused validation
ruby -c .github/scripts/issue-lifecycle-audit.rbbash -n .github/scripts/test-issue-lifecycle-audit.shshellcheck .github/scripts/test-issue-lifecycle-audit.sh./.github/scripts/test-issue-lifecycle-audit.shruby .github/scripts/issue-lifecycle-audit.rb --repo proerror77/monday --pr 470— PASS, 0 violationsruby .github/scripts/issue-lifecycle-audit.rb --repo proerror77/monday— expected read-only FAIL, 93 current violationsgit diff --checkRollout / rollback impact
Read-only only. Rollback is a normal revert of this PR; no issue metadata, branch protection, workflow, or runtime target is changed.