Skip to content

Add replayable closeout evidence gates#78

Merged
justin808 merged 6 commits into
mainfrom
codex/awf-b2-evidence-gates
Jul 5, 2026
Merged

Add replayable closeout evidence gates#78
justin808 merged 6 commits into
mainfrom
codex/awf-b2-evidence-gates

Conversation

@justin808

@justin808 justin808 commented Jul 5, 2026

Copy link
Copy Markdown
Member

Summary

  • Add replayable qa-evidence v1 and priority-finding-dispositions v1 closeout markers, plus a closeout-evidence-replay audit helper.
  • Add pr-check-completion-timing for selected post-merge CI timing audits.
  • Extend pr-ci-readiness so explicitly requested current-head hosted Actions runs cannot be ignored, including repos with no usable required checks.

Closes #53.
Closes #54.
Closes #55.
Closes #65.
Closes #66.

QA Evidence

  • QA lane: qa/evidence-gates on codex/awf-b2-evidence-gates; claim succeeded; last heartbeat review_fix_run_level_hosted_gate_validation_green; QA subagent Avicenna PASS.
  • Scope checked: workflows/pr-processing.md, workflows/post-merge-audit.md, skills/pr-batch, skills/post-merge-audit, skills/plan-pr-batch, docs/pr-batch-skills.md, bin/validate.
  • Tested at: codex/awf-b2-evidence-gates head d493c631d60c622340ffb0728bc5a67fa51414f3 after origin/main 8c2c12d2652d352defa137565acc66c34398a698 refresh.
  • Automated checks: ruby skills/post-merge-audit/bin/closeout-evidence-replay-test.rb; ruby skills/post-merge-audit/bin/pr-check-completion-timing-test.rb; ruby skills/pr-batch/bin/pr-ci-readiness-test.rb; git diff --check; bin/validate; local codex review passes before final hosted-review follow-ups.
  • Manual checks: replay samples for missing/incomplete/blocked/waived/required-not-applicable QA markers, invalid/fixed/waived/missing-waiver priority markers, selected-check pending/failing/cancel/late/status-API-no-timestamp/no-checks/error cases, requested hosted run pending/failing/stale/run-in-progress/success-no-job-fetch cases, and hosted validate failure replay for the fake gh shebang fixture.
  • Findings: none release-blocking; hosted validate fixture portability was fixed in 364481e8be323fa372551eea0a6645d69760442a; hosted review findings were fixed or dispositioned through d493c631d60c622340ffb0728bc5a67fa51414f3.
  • QA required: yes
  • QA required rationale: batch objective required owner qa/evidence-gates and replay samples/helper output/issue replay cases before ready advice.
  • QA lane status: satisfied
  • Release-blocking status: clear
  • Process-gap disposition: schema

Tests

  • ruby skills/post-merge-audit/bin/closeout-evidence-replay-test.rb
  • ruby skills/post-merge-audit/bin/pr-check-completion-timing-test.rb
  • ruby skills/pr-batch/bin/pr-ci-readiness-test.rb
  • git diff --check
  • bin/validate
  • local codex review passes before final hosted-review follow-ups

Merge Authority

  • merge_authority: auto_merge_when_gates_pass
  • Maintainer granted auto-merge in the closeout request; merge proceeds only after current-head gates, review threads, QA replay, and readiness checks are clean.

@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d58f7301-8ed0-40ca-841c-52d0cdd72719

📥 Commits

Reviewing files that changed from the base of the PR and between 8c2c12d and e218238.

📒 Files selected for processing (14)
  • CHANGELOG.md
  • bin/validate
  • docs/pr-batch-skills.md
  • skills/plan-pr-batch/SKILL.md
  • skills/post-merge-audit/SKILL.md
  • skills/post-merge-audit/bin/closeout-evidence-replay
  • skills/post-merge-audit/bin/closeout-evidence-replay-test.rb
  • skills/post-merge-audit/bin/pr-check-completion-timing
  • skills/post-merge-audit/bin/pr-check-completion-timing-test.rb
  • skills/pr-batch/SKILL.md
  • skills/pr-batch/bin/pr-ci-readiness
  • skills/pr-batch/bin/pr-ci-readiness-test.rb
  • workflows/post-merge-audit.md
  • workflows/pr-processing.md
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/awf-b2-evidence-gates

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ce1147b9cf

ℹ️ 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".

Comment thread skills/post-merge-audit/bin/pr-check-completion-timing-test.rb Outdated
Comment thread workflows/pr-processing.md Outdated
Comment thread skills/pr-batch/bin/pr-ci-readiness
@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review summary

Reviewed the diff (14 files, +1281/-22) focused on correctness, security, portability, and shell/Ruby helper safety.

Overall: this is a solid, well-tested change. Both new helpers (closeout-evidence-replay, pr-check-completion-timing) and the extended pr-ci-readiness use Open3.capture2/capture2 with array args exclusively (no shell string interpolation), so there's no command-injection surface even though the scripts consume PR-body/comment text and CLI-supplied repo/run-id values. Regexes used for marker parsing are linear (lazy .*? with no nested quantifiers), so no ReDoS concern from attacker-influenced PR bodies. Validation enums in closeout-evidence-replay match the documented marker schemas in workflows/pr-processing.md field-for-field. New helpers are executable, registered in bin/validate, and documented consistently across SKILL.md/workflows/*.md/docs/pr-batch-skills.md using the existing POST_MERGE_AUDIT_SKILL_DIR resolution-chain convention. No hardcoded consumer-repo commands, labels, branches, or paths were introduced in skills/ or workflows/ — the new --requested-hosted-run behavior and replay markers are described generically and deliberately deferred to the consumer repo's AGENTS.md seam.

Two minor items posted as inline comments:

  1. Doc completeness (workflows/pr-processing.md:580): says the replay helper reports only SATISFIED/BLOCKED/UNKNOWN, but the tool (and its own tests) can also return WAIVED/NOT_APPLICABLE as valid non-error verdicts. Worth aligning with the more precise phrasing already used in workflows/post-merge-audit.md.
  2. Efficiency nit (skills/pr-batch/bin/pr-ci-readiness:351): classify_requested_run always fetches the full paginated job list for a requested hosted run, even when the run-level status/conclusion already conclusively resolves it — avoidable extra gh api calls per requested run.

Neither is release-blocking. No security, portability, or correctness-bug findings beyond the two notes above.

Comment thread skills/post-merge-audit/bin/pr-check-completion-timing

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 364481e8be

ℹ️ 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".

Comment thread skills/post-merge-audit/bin/closeout-evidence-replay Outdated
Comment thread skills/pr-batch/bin/pr-ci-readiness
Comment thread skills/pr-batch/bin/pr-ci-readiness
@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review Summary

Reviewed the diff against main (8c2c12d), focusing on the new closeout-evidence-replay and pr-check-completion-timing helpers plus the pr-ci-readiness --requested-hosted-run extension.

Portability (AGENTS.md editing rules): No hardcoded consumer-repo commands, labels, branches, or paths were introduced in skills/ or workflows/. The POST_MERGE_AUDIT_SKILL_DIR resolution pattern matches the existing PR_BATCH_SKILL_DIR convention. Docs stay generic (selectors, run ids, and scope are all operator-supplied).

Shell/Ruby helper safety: All gh/git subprocess calls use Open3.capture2/capture2e with array args (no shell interpolation) — no command injection risk. Repo strings are validated against a strict OWNER/REPO regex, and --requested-hosted-run inputs are reduced to a digit-only run id before being interpolated into gh api paths. Executable bits and frozen_string_literal/double-quote style are consistent with the rest of bin/.

Findings posted inline:

  1. pr-check-completion-timing (skills/post-merge-audit/bin/pr-check-completion-timing:110-118) — treats any empty completedAt as "still pending," which can misclassify checks reported via the classic Status API (which never populate completedAt, even when pass/fail). This can mask a real post-merge failure as merely pending, since timing_verdict checks pending before failing.
  2. pr-ci-readiness (skills/pr-batch/bin/pr-ci-readiness:249-263) — when no required checks are usable, --requested-hosted-run fully replaces the advisory-list fallback rather than adding to it, so unrelated failing/pending advisory checks stop blocking readiness in that mode. This appears intentional (tested and documented in SKILL.md/--help), so flagged for maintainer awareness rather than as a clear bug.

Test coverage for the new helpers (marker replay edge cases, timing verdicts, requested-hosted-run pending/failing/stale/unknown cases) looks thorough and well-targeted. bin/validate wiring for the two new test files is correctly added.

Comment thread skills/pr-batch/bin/pr-ci-readiness-test.rb
Comment thread skills/pr-batch/bin/pr-ci-readiness
@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review summary

Reviewed the diff (closeout evidence replay markers/helper, pr-check-completion-timing, and the pr-ci-readiness --requested-hosted-run extension) for correctness, security, and portability.

Portability: No hardcoded consumer-repo commands, labels, branches, or release trackers found in the changed skills/ or workflows/ files. New docs consistently use the existing generic POST_MERGE_AUDIT_SKILL_DIR env-var/loaded-skill/repo-local seam pattern rather than embedding example commands. The pre-existing #3844 reference in workflows/pr-processing.md is untouched context, not new.

Shell/Ruby helper safety: Both new helpers (closeout-evidence-replay, pr-check-completion-timing) and the pr-ci-readiness changes consistently use Open3.capture2(*cmd) with array args (no shell), so there's no command-injection risk even though repo/run_id get string-interpolated into a single gh api <path> argument — that argument is never shell-evaluated. Repo format is validated with a strict OWNER/REPO regex before use.

Findings posted inline:

  1. skills/pr-batch/bin/pr-ci-readiness-test.rb — no test exercises a failing (conclusion: "failure") requested hosted run/job, so the NOT_READY-on-failure path of the new --requested-hosted-run feature (arguably its most safety-critical branch) is unverified.
  2. skills/pr-batch/bin/pr-ci-readiness — when no required checks are usable and --requested-hosted-run is passed, the full advisory check list is skipped entirely (confirmed by test_requested_hosted_success_is_ready_without_required_checks_despite_unrelated_advisory_pending), so unrelated failing/pending advisory checks become invisible to the gate. This matches the stated design intent, but it's a non-obvious side effect of what looks like an additive flag, and is only documented in prose docs, not in --help.

Everything else (the closeout-evidence-replay marker parsing/aggregation logic, the pr-check-completion-timing timing/verdict logic, and the stale-head/pending/job-drilldown logic in pr-ci-readiness) checked out against their test suites and looked correct on manual trace.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 220ca98317

ℹ️ 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".

Comment thread skills/post-merge-audit/bin/closeout-evidence-replay
@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review

Reviewed the diff (bin/validate, docs, SKILL.md/workflow updates, and the three Ruby helpers: closeout-evidence-replay, pr-check-completion-timing, and the pr-ci-readiness --requested-hosted-run extension) against the review focus areas: correctness, security, performance, portability, and shell/Ruby helper safety.

Portability — No consumer-repo commands, labels, branches, release trackers, or hardcoded paths found in the changed skills//workflows/ files. The POST_MERGE_AUDIT_SKILL_DIR / PR_BATCH_SKILL_DIR resolution pattern used in the new doc prose matches the existing convention already in skills/post-merge-audit/SKILL.md.

Shell/Ruby safety — All gh/gh api invocations use Open3.capture2 with array args (no shell interpolation), so there's no command-injection surface. run_id extraction (requested_hosted_run_id) is constrained to \d+ before being used in an API path. --repo is validated with a strict OWNER/REPO regex in both pr-ci-readiness and pr-check-completion-timing.

Correctness — Traced the logic for:

  • pr-ci-readiness's required→full fallback, cancel-row handling, and the new --requested-hosted-run gate (stale-head detection, job-fetch skip on already-successful runs, verdict escalation rules).
  • pr-check-completion-timing's bucket classification, including the fail/cancel-without-completedAtfailing (not pending) fix and pass-without-completedAttiming_unknown fix called out in the PR's priority-finding-dispositions marker.
  • closeout-evidence-replay's marker parsing/aggregation (verdict precedence BLOCKED > UNKNOWN > WAIVED > NOT_APPLICABLE > SATISFIED, waived-disposition requiring a waiver field, multi-marker aggregation).

All of this matches what the PR body's priority-finding-dispositions v1 marker claims was fixed in 51701865d604685ca7ee7b99fa335dddb7964466, and I didn't find a case where the implementation contradicts those claims.

Minor, non-blocking observations:

  • pr-check-completion-timing-test.rb covers the classification/bucket logic well but, unlike pr-ci-readiness-test.rb, has no tests for CLI arg validation (missing PR number, malformed --repo, no --select-* selector). Worth adding for parity/regression safety, not blocking.
  • The waived disposition path in closeout-evidence-replay (requiring a waiver field) isn't covered by a test where the field is missing — only the happy path with waiver present is tested.

No blocking correctness, security, or portability issues found. I was not able to execute the Ruby test suites directly in this sandbox (script execution required approval that wasn't available), so this is a static/manual review rather than a verified test run — the maintainer should confirm bin/validate is green before merge if that hasn't already happened.

@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review: replayable closeout evidence gates

Reviewed the diff (CHANGELOG, bin/validate, docs/pr-batch-skills.md, skills/plan-pr-batch, skills/post-merge-audit, skills/pr-batch, workflows/post-merge-audit.md, workflows/pr-processing.md) against the focus areas requested.

Correctness

  • closeout-evidence-replay: marker parsing/validation and the QA/priority verdict state machines (SATISFIED/WAIVED/BLOCKED/NOT_APPLICABLE/UNKNOWN) check out against the test suite, including the multi-marker aggregation ("worst verdict wins" is correct for a batch scope where each marker represents a different covered area — a BLOCKED marker anywhere in scope should keep the aggregate BLOCKED).
  • pr-check-completion-timing: bucket→verdict priority (SELECTED_CHECKS_FAILED > LATE_SELECTED_CHECKS > SELECTED_CHECKS_PENDING > UNKNOWN > clean) is sound, and the Status-API-row-without-completedAt handling correctly separates "failed" from "timing_unknown" per the fixed P1 finding.
  • pr-ci-readiness --requested-hosted-run: run-id/URL parsing only ever yields a \d+ id before it's interpolated into the gh api args, head-SHA staleness is checked before polling job status, and the completed+successful short-circuit correctly skips the job-fetch call. Verdict combination logic (pending/failing → NOT_READY; stale/unknown only escalates an already-READY base to UNKNOWN) matches the documented behavior and the new CLI help text.

Security

  • All gh/subprocess calls use array-form Open3.capture2/capture3 (no shell string interpolation), so PR-body/comment content and CLI-supplied repo/run-id values can't reach a shell. Regexes parsed in closeout-evidence-replay operate on fixed literal marker names (Regexp.escaped) with non-greedy bodies, so untrusted PR-body text doesn't create a ReDoS surface.

Portability

  • No consumer-repo-specific commands, labels, branches, release trackers, or paths found in the changed skills//workflows/ content — the new POST_MERGE_AUDIT_SKILL_DIR resolution guidance matches the existing env-var/loaded-skill/repo-local chain pattern already used elsewhere in skills/post-merge-audit/SKILL.md.

Minor, non-blocking

  • closeout-evidence-replay-test.rb covers missing/invalid fields for most required keys but doesn't have a dedicated case for a priority-finding-dispositions v1 marker missing only head_sha (all existing tests supply it). Not a functional concern since the code path is straightforward, just a small coverage gap if you're topping off the suite later.

No blocking issues found. This looks solid and consistent with the existing helper conventions in the repo.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d493c631d6

ℹ️ 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".

Comment thread skills/post-merge-audit/bin/pr-check-completion-timing
Comment thread skills/post-merge-audit/bin/closeout-evidence-replay
@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review summary

Reviewed the new closeout-evidence-replay and pr-check-completion-timing helpers, the pr-ci-readiness --requested-hosted-run extension, and the associated skill/workflow doc updates.

Correctness / logic — Traced the marker-parsing state machine (qa-evidence v1, priority-finding-dispositions v1), the requested-hosted-run classification (stale-head detection, run-vs-job status rollup, pending/failing/unknown bucketing), and the completion-timing verdict priority (failed > late > pending > unknown > clean) against the accompanying tests. All scenarios I traced by hand line up with the test expectations. One minor dead-code/doc-mismatch nit posted inline on closeout-evidence-replay:115.

Security — All gh invocations use array-form Open3/subprocess calls (no shell interpolation), so --repo, run ids, and regex selectors can't reach a shell. --repo is validated with a strict OWNER/REPO regex before use in both scripts. Run ids parsed from --requested-hosted-run are regex-extracted digits only. No injection surface found.

Portability (AGENTS.md "Editing Rules") — No consumer-repo commands, labels, branches, release trackers, or paths are hardcoded in the diff; all new skill/workflow prose reads generically and routes repo-specific values through the existing POST_MERGE_AUDIT_SKILL_DIR env-var/loaded-skill/repo-local seam, consistent with the pre-existing convention. New helpers live under skills/post-merge-audit/bin/, matching the skill-owns-its-helpers rule.

Shell/Ruby helper safety — Both new scripts fail closed on unexpected gh output (invalid JSON, non-zero exit with no output) rather than silently reporting success, and the --requested-hosted-run head-sha check correctly flags stale runs against an older head as UNKNOWN rather than READY.

No blocking issues found. Nice, thorough test coverage for the edge cases (Status-API rows without completedAt, cancelled-but-timestamped checks, multi-marker aggregation, stale hosted runs).

@justin808 justin808 merged commit be4df50 into main Jul 5, 2026
9 checks passed
@justin808 justin808 deleted the codex/awf-b2-evidence-gates branch July 5, 2026 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant