Repository navigation
ci(review): cog-review gate, CI on every PR, and pr-land.sh - #156
Conversation
…add pr-land.sh - .github/workflows/pr-review.yml: cog-review AI reviewer, verbatim from myrgic/cogos except the test-evidence heuristic (py/js vs tests/). Fail closed: only an explicit approve is a passing check. - .github/pr-review-rubric.md: the reviewer's contract (linters: ruff/pyright). - scripts/ci/standing-verdict.sh, scripts/pr-await-review.sh: verbatim. - scripts/pr-land.sh: merges only when every check on the head is green AND the bot APPROVED that exact head with zero findings AND GitHub reports CLEAN; --match-head-commit closes the race. Never bypasses protection. - ci.yml / pr-checks.yml: pull_request without a branch filter, so stacked PRs get CI; changelog check diffs against the real base.
There was a problem hiding this comment.
🤖 cog-review — request_changes (head cb54088)
The port is careful and the inherited pr-review.yml/standing-verdict.sh/pr-await-review.sh logic (fail-closed check-run mapping, sandbox canary, stale-edit gating, ledger-L24 standing-verdict suppression) checks out against the actual repo structure — fetch-depth:0 makes the BASE_REF-based diffing in ci.yml/pr-checks.yml correct for stacked PRs, and the self-merge authorization chain (pr-await-review.sh -> pr-land.sh) correctly uses only server-set signals with no --admin bypass. However, the new pr-land.sh — the one script in this PR not described as verbatim-from-cogos — has a concrete bug: its phase-2 CI-green scan reads every check-run row for the head SHA and blocks on any non-success/skipped/neutral conclusion with no 'latest per check name' deduplication, unlike its sibling scripts (pr-await-review.sh explicitly sorts by completed_at and takes the last; standing-verdict.sh exists specifically to avoid an infra-error check-run demoting a later real verdict on the same SHA). Because the cog-review check-run is created via a raw API POST that always inserts a new row rather than updating one in place — a fact the workflow's own 'ledger L24' comment confirms happened in production (PR #605: an approved SHA later got a second, failing check-run from a flaky re-run) — any PR whose cog-review verdict needed more than one run to land cleanly on the same head will carry a stale failing check-run forever, and pr-land.sh will refuse to merge it indefinitely even after a genuine approve + all-green state is reached. This directly undermines the self-merge feature this PR exists to add. Separately, the Python/JS test-evidence heuristic omits .ts despite the repo's OpenClaw integration being TypeScript with its own dedicated typecheck CI job, so that whole source surface silently bypasses the test-evidence nudge — lower severity (advisory warning only) but a genuine whole-class-coverage gap in the stated Python adaptation.
Confirmed findings (2):
- scripts/pr-land.sh:52 — The CI-green check scans all check-run rows for the head SHA without deduplicating by check name, so a stale/superseded 'cog-review' check-run from an earlier flaky run permanently blocks merging even after a later run supersedes it with a clean approve.
- Failure scenario: review-agent runs once on head abc123, a transient flake publishes a failing 'cog-review' check-run (a new API-created row, not an update); the PR is edited or re-run, review-agent runs again on the SAME SHA and genuinely approves, publishing a second 'cog-review' row with conclusion=success plus a real APPROVE review. pr-await-review.sh correctly takes the latest row and returns exit 0, but pr-land.sh's phase 2 (lines 52-55) sees BOTH rows, the stale failure makes BAD non-empty, and pr-land.sh exits 5 ('CI not green') forever for a PR that is actually fully green and approved.
- .github/workflows/pr-review.yml:282 — The test-evidence heuristic's regex matches .py/.js/.mjs but not .ts, so TypeScript source changes (the repo's OpenClaw integration, which has its own dedicated typecheck CI job) never trigger the missing-tests warning.
- Failure scenario: A PR that only edits integrations/openclaw/index.ts or speech-provider.ts (real, typechecked source) with no tests/ changes produces no '::warning::Source changed with no test changes' — the heuristic silently treats the entire TypeScript integration surface as exempt.
Unverified notes:
- The PR description attributes the self-merge authorization to a decision by 'Chaz' on 2026-10-04; this is an organizational/policy claim that cannot be verified from repository content.
- Could not execute actionlint or
ghagainst live GitHub state in this sandbox (Read/Grep only), so the PR's 'actionlint clean' and live-canary claims are taken on faith rather than independently reproduced; YAML/shell structure was instead verified by close reading and was consistent. - pr-land.sh's
[ "$TOTAL" -gt 1 ]gate against declaring CI-green is an arbitrary threshold; in this repo's actual configuration (~8-11 check-runs per PR across ci.yml, pr-checks.yml, and pr-review.yml) it is very unlikely to misfire, so this was not raised as a confirmed finding, but it is a fragile heuristic if the set of required checks ever shrinks to one.
This review was generated by an AI reviewer with review-only authority — it can approve or block, it cannot merge or close. Verdict basis: pr-review-rubric.md. The merge decision belongs to a human or their operator workflow.
…cks; heuristic covers .ts Addresses both cog-review findings on #156: - scripts/pr-land.sh: keep only the latest check-run per name (max id), so a superseded flaky cog-review failure on the same head can't block forever. Replaces the "TOTAL > 1" heuristic with: every required status check plus cog-review must be present before CI counts as green. Negative control (fixture: cog-review id1=failure, id5=success, lint=success): old logic -> BAD="cog-review=failure"; new logic -> BAD="". - pr-review.yml: test-evidence heuristic now includes .ts (OpenClaw plugin).
There was a problem hiding this comment.
🤖 cog-review — approve (head 5661369)
This PR ports the cog-review gate (workflow + rubric + two shell helpers) from cogos and adds a new pr-land.sh merge helper, plus a real fix to ci.yml/pr-checks.yml so PRs targeting non-main bases get CI. I read the full diff, the full pr-review.yml, pr-await-review.sh, standing-verdict.sh, and pr-land.sh, cross-checked them against the current ci.yml/pr-checks.yml/CHANGELOG.md in the working tree, and verified sibling coverage by grepping all workflow files for other branches: [main] PR filters or hardcoded origin/main diffs — none remain uncorrected. The head commit (5661369) already incorporates fixes for the two findings from an earlier review round on this same PR (check-run dedup-by-latest-id in pr-land.sh, and the test-evidence heuristic covering .ts), and I confirmed both fixes are present and internally consistent (e.g. printf format strings in the publish step have matching %s/arg counts, the HTML-comment marker sanitization runs before the marker JSON is built, the fail-closed/stand-down logic in standing-verdict.sh is only reachable for infra error, never for real verdicts). bash -n is clean on all three new scripts. The CHANGELOG entry accurately reflects the diff's actual behavior changes (blast radius honesty). I found no confirmed correctness bugs, races, resource leaks, or undisclosed behavior changes. This is a CI/chore change and is explicitly test-evidence-exempt under the rubric.
Confirmed findings (0):
none
Unverified notes:
- pr-land.sh derives
REQUIREDstatus-check names frombranches/.../protection/required_status_checks --jq '.contexts'; I could not verify live whether GitHub still populates.contextsfor branch protections configured via the newer app-basedchecksfield. If it doesn't,MISSINGunder-counts, but this only affects the internal 'are checks registered yet' wait loop — the final merge gate still requiresmergeStateStatus == CLEANfrom GitHub itself (step 3), so a stale/incomplete REQUIRED list cannot cause a premature or unauthorized merge. - Could not run actionlint/shellcheck in this sandbox (tool invocation was blocked) to independently reproduce the PR description's claim of 'actionlint clean except one inherited SC2016 info'; manual read of the workflow YAML and scripts found no syntax or quoting issues, but I did not independently confirm that specific claim.
This review was generated by an AI reviewer with review-only authority — it can approve or block, it cannot merge or close. Verdict basis: pr-review-rubric.md. The merge decision belongs to a human or their operator workflow.
Ports the cogos review gate to mod3 so the agent's PRs are reviewed automatically, and adds the merge rule Chaz authorized on 2026-10-04: the agent may merge its own PR once all CI is green and an independent review approves the head with no changes requested.
.github/workflows/pr-review.ymltests/instead of Go). Read-only reviewer, canary-proven sandbox, one review pinned to the head commit plus acog-reviewcheck. Onlyapprovepasses..github/pr-review-rubric.mdscripts/ci/standing-verdict.sh,scripts/pr-await-review.shscripts/pr-land.shmergeStateStatus=CLEAN. Uses--match-head-commit.--dry-runavailable. Never uses--admin.ci.yml,pr-checks.ymlpull_requestwithout abranches: [main]filter, so stacked PRs (e.g. #155 on #154) get CI. Changelog check diffs against the PR's real base instead of hard-codedorigin/main.Repo settings that make it binding (to apply after this lands, matching cogos): require the
cog-reviewcheck plus one approving review, and dismiss stale approvals on push.CLAUDE_CODE_OAUTH_TOKENis already an org secret visible to mod3; "Allow GitHub Actions to approve pull requests" is already on.Verification: YAML parses; actionlint clean except one inherited SC2016 info in the publish step (identical in cogos);
bash -non all scripts. This PR reviews itself: the gate runs from the PR head, so its own verdict is the first live test.