feat(review-remote-pr): detect disabled Code Quality and skip it cleanly - #419
Merged
Conversation
…s from staleness record-tier0.yml pushes a post-merge [skip ci] commit to main after every merge; merge-gate.sh blocked the next queued PR on base: stale=yes, forcing a second merge-down and CI run per PR (agent-kit#394, 2026-08-22 queue #387-#389). gh-pr-state.sh now exempts a base advance whose new commits touch only the repository-declared AGENT_GENERATED_PATHS prefixes from staleness; any other base advance still stales. Documented in onboard-repo and the dispatch-plan generated-path handling, with a fixture test covering a base advance confined to bench/results/tier0.jsonl. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
The base-staleness exemption added in f4c8728 was correct but inert for agent-kit's own record-tier0.yml commits: this repository's declared AGENT_GENERATED_PATHS (.agent/board.json only) did not cover bench/results/tier0.jsonl, so the issue's own Evidence-section defect stayed live on merge. Adds bench/results/ to the declaration, a regression-pin test asserting this repo's own config covers bench/results/tier0.jsonl, and a note on the dual meaning of AGENT_GENERATED_PATHS in its schema template. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
…names Adversarial review of PR #416 (Codex gpt-5.6-terra, xhigh) confirmed two fail-open gaps in base_advance_is_automation_only: (1) GitHub compare API caps .files at 300 entries per page with no pagination follow-through here, so a base advance touching 301+ files could report stale=no if only the first 300 (all declared) were read; (2) a rename only checked the new filename, so a rename FROM an undeclared path INTO a declared one (or vice versa) reported stale=no despite moving application code. Both now fail closed: a files count at or above the named COMPARE_FILES_PAGE_CAP (300) is treated as unreadable evidence, and both filename and previous_filename must match a declared prefix for a renamed entry. Adds regression-pin tests for each. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
…of coderabbit=none gh-pr-state.sh derived provider: coderabbit=... from an issue-comment phrase scan, which an APPROVED review with zero actionable threads (or a CHANGES_REQUESTED review with inline threads only) never matches -- PR #386 read coderabbit=none for 15 wasted one-minute rounds after CodeRabbit had already approved it. The real signal is the reviews endpoint itself: an acknowledgement is a plain issue comment, never a review submission, so it can never be mistaken for one. provider_state() now reports the most recent terminal review directly (state=APPROVED|CHANGES_REQUESTED|COMMENTED threads=N since=TIMESTAMP), falling back to the rate-limit/none scan only when no review has landed yet. review-transition.sh gains a lightweight --observe --since TIMESTAMP mode that answers "has the review landed since I triggered it" without repeating the full ready-transition and provider-spend flow, returning LANDED (or PENDING); its TRIGGERED/ALREADY_SPENT results now carry that since= boundary. merge-gate.sh accepts LANDED as evidence-green, exactly like AUTO_REVIEW/ALREADY_SPENT. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
… into feat/issue-395 # Conflicts: # tests/test-gh-pr-state.sh
…eanly AGENT_REVIEW_PROVIDERS=github-code-quality was accepted at plan time even when the repository had GitHub Code Quality disabled, and code-quality-state.sh then died mid-gate on a raw 403. code-quality-state.sh gains --probe: a single lightweight request that decides state=enabled/not-enabled/unknown, treating only a confirmed "not enabled" 403 message as proof of disablement -- every other failure (network, auth/scope, 5xx) reports unknown and fails closed. review-provider-config.sh --probe uses it once to downgrade the declared github-code-quality plan line on a confirmed not-enabled, leaving every other provider and the default (no --probe) call path -- including review-transition.sh -- byte-for-byte unchanged. The review-remote-pr Step 5 recipe probes before fetching findings instead of dying on the 403, and merge-gate.sh accepts --code-quality-scan-state not-enabled as a pass, since a disabled repository has no scan to wait for. Closes #403. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Adversarial review of PR #418 found that neither gh-pr-state.sh's provider_state() nor review-transition.sh's new --observe path checked a review's own commit_id: a CodeRabbit review submitted against an OLD head can finish AFTER the PR advances to a new one, with a submitted_at that looks perfectly current. That let a stale review (of code nobody has reviewed) be reported/accepted as landed evidence for the current head -- the exact failure this issue set out to fix, arriving from the other direction. Both selectors now require the review's commit_id to match the current head (fetched fresh for --observe, read from the already-fetched PR metadata for the digest) before calling it landed/reviewed. A terminal review that exists but targets a different head is reported distinctly (stale-head / STALE_HEAD) -- never reviewed/LANDED (misrepresents stale evidence as current) and never none/PENDING (hides that a review exists at all). merge-gate.sh blocks STALE_HEAD exactly like an in-flight TRIGGERED review. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
… into feat/issue-403
Collaborator
Author
|
This was written agentically; verify its assertions: Adversarial review receipt
🤖 Co-authored by Claude Opus 5 (root). |
auto-merge.md still described --code-quality-scan-state as complete|pending after issue #403 added a third value; the STALE_HEAD provider-result direction was already current (issue #395 documented it in the same file). Updates the bullet to name not-enabled and explain it gates exactly like complete, while an unreadable probe (network failure, auth/scope 403, a 5xx) still blocks. Prose only, no behavior change. Co-Authored-By: Claude claude-sonnet-5 <noreply@anthropic.com>
6 tasks
Integrates the advanced base into this branch ahead of its own retarget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thewrz
marked this pull request as ready for review
August 23, 2026 22:38
thewrz
pushed a commit
that referenced
this pull request
Aug 23, 2026
Integrates the advanced base into this branch ahead of its own retarget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This was written agentically; verify its assertions:
Why
AGENT_REVIEW_PROVIDERS=github-code-qualitywas accepted at plan time even when the repository has GitHub Code Quality disabled. Nothing checked, socode-quality-state.shfailed mid-gate with403 Code quality is not enabled for this repository— a declaration accepted at plan time that only fails much later, at the worst moment.What
code-quality-state.sh --probereportsstate=enabled|not-enabled|unknown.review-provider-config.sh --probe(opt-in) emitsprovider=github-code-quality mode=none source=declared reason=not-enabledwhen the probe decides the feature is off, andmerge-gate.sh --code-quality-scan-statenow acceptsnot-enabled, treating it exactly as it treatscomplete.pendingremains the only blocker.Decisions
ghbeing unreachable all resolve tounknownand fail closed. Disabling a review gate because a request happened to fail would hide exactly what the gate exists to catch, so "the probe failed" is never treated as "the feature is off".review-transition.shis outside this change's write set and parses everyreview-provider-config.shline against the catalog's staticmodefor each provider. An unconditional probe would have changed output under an existing caller. Default (no--probe) output is byte-for-byte unchanged, pinned by a compatibility test, so that call site is provably unaffected.review-remote-pr/SKILL.mdsits at its exact pinned token ceiling; all documentation landed inreferences/provider-rules.md, matching fix(review-remote-pr): gh-pr-state.sh provider line hides a landed CodeRabbit review (APPROVED/CHANGES_REQUESTED reads as coderabbit=none) #395's precedent. No ceiling ratchet, and no collision with feat(review-remote-pr): keep per-PR RUN_DIR pointers under the excluded .agent/ so a resumed session can find its evidence #405 downstream.Known follow-up
pr-to-green/references/auto-merge.mdstill describes--code-quality-scan-stateascomplete|pendingin prose and does not mentionnot-enabled. The mechanism is correct and tested; only that prose is stale. It sits outside this change's declared write set, and was reported rather than silently widened into it — it will be corrected in a follow-up touch on this branch.Diff size
operational.linesas reported below, across 7 files, measured against the chain base — within the ~500-line guideline.Testing
bash tests/test-code-quality-state.sh→ 12/12 (new file)bash tests/test-review-provider-config.sh→ 26/26 (16 pre-existing + 10 new)bash tests/test-pr-to-green-merge-gate.sh→ 80/80 (78 pre-existing + 2 new)bash tests/run-tests.sh→ ALL GREEN (run independently at root review, on the chain-cumulative tree)bash tests/lint-skill-size.sh agentkit/skills→ 0 violations, no ceiling raiseshellcheckon every changed script → clean apart from pre-existing repo-wide SC1091/SC2034 notices🤖 Co-authored by Claude Sonnet 5 (worker) and Claude Opus 5 (root review).
Closes #403