Skip to content

fix(merge-gate): check_bot_anchoring must FETCH the inline comment count, not just read a field that is never present - #419

Closed
jaylfc wants to merge 1 commit into
masterfrom
exec/tsk-q7dnww
Closed

fix(merge-gate): check_bot_anchoring must FETCH the inline comment count, not just read a field that is never present#419
jaylfc wants to merge 1 commit into
masterfrom
exec/tsk-q7dnww

Conversation

@jaylfc

@jaylfc jaylfc commented Aug 30, 2026

Copy link
Copy Markdown
Owner

CARD TITLE (intent, not commit subject): fix(merge-gate): check_bot_anchoring must FETCH the inline comment count, not just read a field that is never present

Autonomous build of board card tsk-q7dnww.

Replace gh pr view --json reviews (which never carries comments.totalCount)
with a gh api graphql query that includes reviews.nodes.comments.totalCount.
Derive OWNER and REPO from gh repo view --json owner,name instead of
hardcoding jaylfc/taosmd. Update the reviews iteration to read
data.reviews.nodes and the has_inline check to use comments.totalCount
instead of the absent includesCreatedEdit field.

Existing fixtures updated to the GraphQL nodes shape. pr_inline_only.json
captured from real PR #267 data (kilo-code-bot, empty body, 7 inline comments,
anchored to head). PR #279's two tests ported to the new payload shape, using
string replacement to derive the pre-fix script so they cannot drift.

Verification (6-PR loop, matching expected outcome exactly):
#267 SUCCESS, #272 SUCCESS, #276 SUCCESS
#274 FAILED, #275 FAILED, #277 FAILED

Fixture proof -- comm -23 fx.paths live.paths printed nothing, no fabricated
keys in pr_inline_only.json.

Positive control: #230 and #241 still print SUCCESS (non-empty-body branch
untouched). #244 now prints FAILED because its headRefOid 69a93ba does not
match the review commit fa0a516... (head ref state change since the card
was written; same result with the old gh pr view --json format, so this is
not a regression from the code change).

Tests: 8 passed (uv run --extra dev pytest tests/test_merge_gate.py -q)

Files:
.../tsk-q7dnww-bot-anchoring-inline-comments.md | 7 +++
scripts/merge-gate/check_bot_anchoring.sh | 30 +++++++++--
tests/fixtures/merge_gate/pr_anchored_head.json | 40 ++++++++-------
tests/fixtures/merge_gate/pr_anchored_old_sha.json | 23 +++++----
tests/fixtures/merge_gate/pr_human_only.json | 23 +++++----
tests/fixtures/merge_gate/pr_inline_only.json | 21 ++++++++
tests/test_merge_gate.py | 59 +++++++++++++++++++++-
7 files changed, 161 insertions(+), 42 deletions(-)

… GraphQL

Replace `gh pr view --json reviews` (which never carries comments.totalCount)
with a `gh api graphql` query that includes reviews.nodes.comments.totalCount.
Derive OWNER and REPO from `gh repo view --json owner,name` instead of
hardcoding jaylfc/taosmd. Update the reviews iteration to read
data.reviews.nodes and the has_inline check to use comments.totalCount
instead of the absent includesCreatedEdit field.

Existing fixtures updated to the GraphQL nodes shape. pr_inline_only.json
captured from real PR #267 data (kilo-code-bot, empty body, 7 inline comments,
anchored to head). PR #279's two tests ported to the new payload shape, using
string replacement to derive the pre-fix script so they cannot drift.

Verification (6-PR loop, matching expected outcome exactly):
#267 SUCCESS, #272 SUCCESS, #276 SUCCESS
#274 FAILED, #275 FAILED, #277 FAILED

Fixture proof -- comm -23 fx.paths live.paths printed nothing, no fabricated
keys in pr_inline_only.json.

Positive control: #230 and #241 still print SUCCESS (non-empty-body branch
untouched). #244 now prints FAILED because its headRefOid 69a93ba does not
match the review commit fa0a516... (head ref state change since the card
was written; same result with the old `gh pr view --json` format, so this is
not a regression from the code change).

Tests: 8 passed (uv run --extra dev pytest tests/test_merge_gate.py -q)
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 51 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c5e49148-7a53-4543-af03-2e13182626f5

📥 Commits

Reviewing files that changed from the base of the PR and between e0429ac and f11070d.

📒 Files selected for processing (7)
  • changelog.d/tsk-q7dnww-bot-anchoring-inline-comments.md
  • scripts/merge-gate/check_bot_anchoring.sh
  • tests/fixtures/merge_gate/pr_anchored_head.json
  • tests/fixtures/merge_gate/pr_anchored_old_sha.json
  • tests/fixtures/merge_gate/pr_human_only.json
  • tests/fixtures/merge_gate/pr_inline_only.json
  • tests/test_merge_gate.py

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.

@gitar-bot

gitar-bot Bot commented Aug 30, 2026

Copy link
Copy Markdown

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

Gitar

@jaylfc

jaylfc commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Adjudicated CLEAN. Waiting on a merge call, not on review.

Recording the verdict here because until now it existed only in my own notes, and a PR with no verdict on it reads as unadjudicated to whoever opens it next.

Gate state on head f11070d4, read from .../check-runs rather than the combined commit status:

Gitar                  success
doc-gate               success
trailing-newline-gate  success
deleted-symbols-gate   success
test                   success
witness-token-gate     success
normalise-handle-gate  success
Kilo Code Review       FAILURE   <- "Review failed: Assistant service is unavailable"

The single red is a service outage, not a finding, which is why the PR shows UNSTABLE. Seven of seven real gates are green. Inside the ci run, step 7 Lint is success and step 8 Tests is success (1795 passed, 12 skipped); lint precedes tests in that job, so a green lint plus a green test step means the suite genuinely ran.

Correction to this PR's own premise. The body claims the gate always-passed. It does not: includesCreatedEdit is present but always false, so the gate false-NEGATIVED. The practical effect is that this change LOOSENS the gate rather than tightening it. All 18 fail-open probes still fail closed afterwards, so the loosening opens no hole, but the title describes the opposite of what the diff does.

Caveat that still stands: grep -rn "merge-gate" .github/ is empty. Re-confirmed 2026-09-04 with a control (5 other scripts/* ARE referenced, so the grep discriminates). These scripts are wired into no workflow, so this is a correct fix to a gate nothing currently runs, and CI green proves nothing about it. Tracked as tsk-ergobm.

Honest staleness note: the suite figure above was measured when master was 0baa9af9. Master is now bb8dfc09 (#458 landed after this PR's CI ran). GitHub still reports this MERGEABLE against current master; the behind-ness is not a conflict.

@jaylfc

jaylfc commented Sep 6, 2026

Copy link
Copy Markdown
Owner Author

Closing on Jay's per-PR call (2026-09-06). scripts/merge-gate/check_bot_anchoring.sh is referenced nowhere under .github/ (confirmed on card tsk-ergobm, 2026-09-04), so no workflow runs the gate this PR hardens. Until tsk-ergobm decides whether the gate is wired in or removed, review spend on it is not justified. Source card closed in the same action; the hardening gets re-carded if and when the gate is wired.

@jaylfc jaylfc closed this Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant