Skip to content

fix(review-and-merge): Copilot review detection, and failure handling that misread transport faults as verdicts - #12

Merged
forhas merged 12 commits into
mainfrom
fix/copilot-author-matching-and-suppressed-comments
Aug 6, 2026
Merged

fix(review-and-merge): Copilot review detection, and failure handling that misread transport faults as verdicts#12
forhas merged 12 commits into
mainfrom
fix/copilot-author-matching-and-suppressed-comments

Conversation

@forhas

@forhas forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Started as two client-reported defects in the Copilot reviewer profile; grew to cover the failure-handling gaps found while driving this PR through the repo's own review loop. Every defect here caused the loop to silently do the wrong thing — no error, no warning.

Client-reported: Copilot review detection

One bot, two logins. The review object and its inline comments render under different logins:

Surface user.login user.node_id
GET /pulls/{n}/reviews copilot-pull-request-reviewer[bot] BOT_kgDOCnlnWA
GET /pulls/{n}/comments Copilot BOT_kgDOCnlnWA

Confirmed independently: GET /users/copilot-pull-request-reviewer[bot]{"login":"Copilot","node_id":"BOT_kgDOCnlnWA"}, while GET /users/Copilot 404s. The profile's rule — "author login exactly equals … never a substring test" — matched the review and zero of its inline comments. A reporter's poll returned inline=0 while the body said "generated 2 comments". A second, independent run reported the same thing.

Now accepts either login, attributes comments by pull_request_review_id, and reconciles each body's stated comment count against the comments actually retrieved. Uses an explicit two-login set rather than a ^[Cc]opilot prefix, which would also match a human copilot-fan or any future copilot-* app.

"Suppressed comments" were documented as boilerplate. Copilot withholds low-confidence findings into a <details><summary>Suppressed comments (N)</summary> block. In the reported case, a round headlined "generated no new comments" while that block held the most substantive finding of the review — a real bug where an RTT probe treated EOF as a fast success. The boilerplate row said to ignore "the review body's per-file <details> summary table", so a literal reading discarded it. Now explicitly not boilerplate, with its parse shape documented, and "no meaningful issues" requires no populated suppressed block.

Found while running this loop: failure handling

One root error in four variants — treating a transport failure as a semantic signal:

  1. Blind-retrying a trigger. A non-zero gh exit can mean the mutation applied and only the response was lost. Retrying double-triggers, corrupting the round count. Now re-reads the state the trigger would have changed and decides from that — generalizing the pattern step 5 already applied to the merge command.
  2. A failed state read scored as a state change. Hit live: the trigger timed out, the recovery read also failed and returned empty, and comparing empty against the baseline read as "it landed". It hadn't. The read must now return a definite answer before it decides anything.
  3. Any HTTP status treated as permanent rejection. An over-correction of feat: pure-dev plugin marketplace with quick-dev and notion-dev #1: this still misclassifies 500/502/503, 429, and rate-limit 403 as "Copilot not configured", permanently switching to the local fallback and reporting a config problem the repo doesn't have. Replaced with a three-bucket classification where only a permanent rejection is a verdict.
  4. No baseline before the first trigger. Recovery compared against "the round's previous trigger", undefined on the first post. Re-running on an already-reviewed PR is the normal path, and those PRs carry old @codex review comments that recovery would adopt as proof the new post landed.

Plus two correctness bugs in the Copilot lookup itself:

  • --paginate applies --jq per page. An aggregating filter emits one result per page, so $RID became multi-line and the next jq expression invalid. Reproduced: 3 output lines where 1 was expected. Fixed with --slurp + external jq (--slurp cannot combine with --jq), and the root cause documented in references/github-api.md, which previously covered only the "always --paginate" half.
  • last dropped concurrent reviews. A silence re-trigger can leave two requests outstanding, both submitting in one poll interval. Reproduced on this PR: 4 reviews / 6 inline comments, of which the last-only form retrieved 3.

Silence re-trigger gated on the request actually being gone (second client report). Observed Copilot latency ranged from <30s to ~16 min across rounds of one run, so the 10-minute window elapsing is not proof the request was dropped — re-triggering a slow request queues a duplicate review. The bot still listed in reviewRequests now proves the request is live; polling extends to a bounded ~30 min instead.

Repo hygiene

.claude/skills/review-and-merge/ is no longer gitignored. It mirrors the plugin skills, had already drifted 167 diff lines from quick-dev's, and fixes applied there were invisible to review. .claude/* stays ignored with a negation for .claude/skills/, so local session state remains untracked.

Verification

Instruction-only changes, so there is no test suite. What was actually verified:

  • Both Copilot logins and the shared node_id, against the live API.
  • The --paginate/--jq per-page behavior and the --slurp incompatibility, reproduced on gh 2.97.0.
  • The replacement multi-review jq, run against this PR: 4 reviews → 6 comments, versus 3 under the old form.
  • The .gitignore negation: skill tracked, settings.local.json still ignored.
  • All plugin/marketplace JSON manifests parse.

Versions: notion-dev 0.12.0 → 0.12.1, quick-dev 0.7.0 → 0.7.1.

Reviewed across 5 Codex rounds; 4 of the 6 findings were in code added during this PR, all confirmed against the API before being applied. Two review comments on this PR are Codex agent work reports claiming commits 96efc06 and 3e779c4 — neither object exists in this repository, and no code from them is included.

…t in both plugins

Reported by a notion-dev user running the review loop with reviewer=copilot.
Both defects caused the loop to silently under-review Copilot PRs.

1. Copilot renders under two logins for one bot account. The review object
   carries login `copilot-pull-request-reviewer[bot]`; its inline comments
   carry login `Copilot`. Both share user.node_id (BOT_kgDOCnlnWA) —
   GET /users/copilot-pull-request-reviewer[bot] returns login "Copilot",
   confirming the slug and the canonical login are the same identity.

   The profile's "author login exactly equals ... never a substring test"
   rule therefore matched the review and zero of its inline comments. A
   reporter's first poll returned inline=0 while the review body said
   "generated 2 comments" — a strict reading would have missed both
   round-1 findings and declared the round clean.

   Fix: the copilot profile now accepts either login, and step 4 attributes
   inline comments by pull_request_review_id rather than by login or
   timestamp (inline comments carry no submitted_at, so review-id
   attribution also fixes round attribution). Added a reconciliation check:
   if the body's "generated N comments" disagrees with the inline comments
   retrieved, the filter is wrong — do not proceed on the smaller number.

   An explicit two-login set is used rather than a ^[Cc]opilot prefix match,
   which would also match a human account like `copilot-fan` or any future
   copilot-* app and promote a non-reviewer's comment into a review round.

2. Copilot's "Suppressed comments (N)" <details> block holds real findings
   withheld for low confidence, and is frequently where the most
   substantive finding of a review lands. The boilerplate row said to ignore
   "the review body's per-file <details> summary table", so a literal
   reading discarded it; the "no meaningful issues" definition did not
   mention it at all, letting a review whose headline reads "generated no
   new comments" merge past a real bug.

   Fix: the boilerplate row is narrowed to exactly two regions and states
   that a suppressed-comments block is not boilerplate; "no meaningful
   issues" now requires no suppressed block with N >= 1 and states the
   headline is not sufficient on its own; the response-handling step gains
   the parse shape and an instruction to triage each entry under the
   judgment bar.

quick-dev carried byte-identical text and is fixed in the same way.

Left unchanged: the config schema, notion-dev:init, and the quick-dev README
cite copilot-pull-request-reviewer[bot] as the requested_reviewers API
target, which is correct in that position.
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: f34b1877cb

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

…t faults as not-configured

Found while running this repo's own review loop on PR #12: `gh` returned
`error connecting to api.github.com` twice — once on the checks gate, once
on the `@codex review` trigger post. Two defects in the same path.

1. Trigger idempotency. Step 3 posts the trigger with no guidance for a
   non-zero exit whose outcome is unknown. A transport failure can lose the
   response after the mutation was already applied, so a blind retry
   double-triggers: two reviews race on one poll baseline and the round
   counter is wrong for the rest of the run. Step 3 now requires re-reading
   the state the trigger would have changed (codex: the issue-comment list;
   copilot: `reviewRequests`) and deciding from that, with at most 3 backed-off
   retries. This generalizes the pattern step 5 already applies to the merge
   command, now stated as a safety rule for every mutating call.

2. Transport fault misread as configuration state. The copilot profile
   treated *any* non-zero exit from the reviewer-request as
   `reason=not-configured`, which permanently switches the run to the local
   fallback loop and makes the final report claim Copilot is not enabled for
   the repo. A brief network outage would trigger that misdiagnosis with no
   way to tell it apart from a real 422. `not-configured` now requires an
   actual HTTP error response from GitHub; a bare connection error carries no
   status and is retried instead.

Applies to both plugins. No version bump: 0.12.1 / 0.7.1 from the previous
commit are unreleased.
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@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: d38a05d1d8

ℹ️ 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 plugins/notion-dev/skills/review-and-merge/SKILL.md Outdated
Codex round 2 (P1, PR #12): the copilot review-id snippet used
`gh api --paginate ... --jq '[...]|last|.id'`, which applies the filter
per page and emits one result per page. On a PR with more than one page of
reviews $RID becomes multi-line and the interpolated select() is invalid jq,
so the Copilot loop cannot process the response.

Reproduced on gh 2.97.0: the same shape against a 3-page endpoint printed
three lines.

The suggested remedy (slurp within --jq) is not available — gh rejects
`--slurp` combined with `--jq`. Applied instead: --slurp to wrap pages, then
filter with external jq, flattening with .[][]. Verified end to end on this
PR: returns review 4876529773 and its 1 inline comment by
pull_request_review_id.

Fixed in both plugins. Root cause also documented in
references/github-api.md, which previously covered only the "always
--paginate" half and never warned that --paginate applies --jq per page —
the gap that produced this bug.
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@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: 59d5fb410d

ℹ️ 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 plugins/notion-dev/skills/review-and-merge/SKILL.md Outdated
forhas added 2 commits August 6, 2026 19:13
…tted review

Codex round 3 (P2, PR #12): the recovery check added in d38a05d decided
whether a failed copilot reviewer-request had landed by looking only at
`gh pr view --json reviewRequests`. But the profile states the bot is
auto-removed from reviewRequests the moment it submits, so absence is
ambiguous across two states:

  - the request never landed (retry is correct), and
  - the request landed and Copilot already submitted its review

A review completing inside the recovery window therefore read as "nothing
landed" and sent a duplicate request — the exact double-trigger the fix was
introduced to prevent.

The check now accepts either signal as proof the request landed: the bot
listed in reviewRequests, or a Copilot review submitted after the timestamp
captured immediately before that attempt (dual-login match). Only when
neither holds does it retry. Case (b) also notes the found review is the
round's response and must be handled, not re-requested.

No analogous gap on the codex side: that check looks for the `@codex review`
issue comment, which is never auto-removed, so its absence is unambiguous.

Applied in both plugins.
Hit live while driving PR #12 through this repo's own review loop. The
round-4 trigger failed with `dial tcp … i/o timeout`, the recovery check ran
per d38a05d, and the state re-read ALSO failed — returning empty. The
comparison against the baseline saw "3 → empty", read that as a state
change, and concluded the trigger had landed. It had not: the real count was
still 3.

The rule added in d38a05d says to re-read the state and let it decide, but
never required the read itself to succeed. That is the same error class it
was written to fix — conflating a transport failure with a semantic signal —
one level up, and it fails in both directions: a false "it landed" silently
skips the round, a false "it didn't" double-triggers.

The recovery step now requires the read to return a definite answer before
it decides anything, and retries the read (not the mutation) until it does.

Applied in both plugins.
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@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: 47de210035

ℹ️ 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 plugins/quick-dev/skills/review-and-merge/SKILL.md Outdated
Comment thread plugins/quick-dev/skills/review-and-merge/SKILL.md Outdated
Comment thread plugins/notion-dev/skills/review-and-merge/SKILL.md
…seline the first trigger

Codex round 4 (PR #12) — three P1 findings, all valid, all in code added
earlier in this branch.

1. `last` dropped concurrent reviews. The review-id lookup selected only the
   newest review submitted after the trigger timestamp. A silence re-trigger
   can leave two requests outstanding and both can submit inside one poll
   interval, so the other review's findings went untriaged and its threads
   unresolved — which then blocks the merge gate.

   Reproduced on this PR: 4 codex reviews carrying 6 inline comments; the
   `last`-only form retrieved 3. Exactly half lost.

   Now collects every matching review id and selects comments by membership,
   handles every review rather than the newest, and reconciles each review
   body's "generated N comments" against that review's own comments.

2. Transient HTTP responses were classified as `not-configured`. d38a05d
   fixed "any non-zero exit" to "any HTTP error status", which still
   misclassifies 500/502/503, 429, and rate-limit 403 as a permanent
   configuration problem — and listed 403 as a not-configured example, when
   a rate-limit 403 is among the likeliest transient failures here.

   Replaced with a three-bucket classification: permanent rejection (422, or
   403/404 whose message says the feature is not enabled) is the only
   verdict; transient responses and transport failures are both retried.
   `gh` exits non-zero "for any reason" (gh help exit-codes), so neither the
   exit code nor the presence of a status separates them — the message does.

3. No baseline before the first trigger. Recovery looked for a comment
   "newer than the round's previous trigger", undefined on the first post.
   Re-running the loop on an already-reviewed PR is the normal path (step 2
   exists for it), and such a PR carries older `@codex review` comments, so
   recovery could adopt a stale comment as proof the new post landed and
   then poll from its timestamp. Now captures an attempt baseline before
   every post, including the first; copilot's existing pre-call timestamp
   serves as its baseline.

Findings 1 and 2 applied in both plugins; finding 3 also in this repo's
local skill copy, which shares the codex recovery path.
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@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: cf385033fa

ℹ️ 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 plugins/notion-dev/skills/review-and-merge/SKILL.md
…track the local skill copy

Two client reports from a separate notion-dev 0.12.0 run (STO-75).

1. copilot-response-latency — NOT previously handled. The silence rule
   treated ~10 minutes without a response as grounds to re-trigger. Observed
   latency on one PR ranged from under 30 seconds to ~16 minutes across
   rounds of the same run, so the window elapsing does not mean the request
   was dropped; re-triggering a merely-slow request queues a second review
   and produces a duplicate round. The reporter avoided this only by
   re-checking PR state before acting on the timeout.

   The window is now a prompt to verify, not a verdict. For copilot, the bot
   still being listed in reviewRequests proves the request is live — never
   re-trigger in that state, keep polling to a bounded ~30 minutes. For
   codex, which has no pending marker, require a definite re-read first (a
   failed read is not silence). If a duplicate round happens anyway, the
   multi-review handling added in cf38503 ensures both reviews are triaged
   rather than the newer masking the older.

2. copilot-inline-author — already fixed in f34b187; this is independent
   corroboration from a second run. No further change.

Also: stop gitignoring this repo's own copy of the review-and-merge skill.
It mirrors the plugin skills and had already drifted 167 diff lines from
quick-dev's; keeping it untracked meant fixes applied there were invisible
to review and lost to everyone else. `.claude/*` stays ignored with a
negation for `.claude/skills/`, so local session state (settings.local.json
etc.) remains untracked.
@forhas forhas changed the title fix: Copilot review author matching and suppressed-comments blind spot in both plugins fix(review-and-merge): Copilot review detection, and failure handling that misread transport faults as verdicts Aug 6, 2026
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@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: 6f51b48a1a

ℹ️ 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 .claude/skills/review-and-merge/SKILL.md Outdated
Comment thread plugins/quick-dev/skills/review-and-merge/SKILL.md
Comment thread plugins/quick-dev/skills/review-and-merge/SKILL.md
…seline across a silence retry, sync the local mirror

Codex round 6 (PR #12) — three P2 findings, all valid, all introduced by
earlier commits on this branch.

1. 422 was treated as Copilot-specific. GitHub documents 422 on
   requested_reviewers as "Validation failed, or the endpoint has been
   spammed", so the same status covers a genuine not-enabled rejection, an
   ordinary validation error, and spam-protection throttling. The
   three-bucket rule required a message check for 403/404 but let 422
   through on status alone, so a throttle would permanently switch to the
   local fallback and report a configuration problem that does not exist.
   422 now carries the same message requirement; spam-protection 422 is
   listed in the transient bucket.

2. The silence re-trigger refreshed the round's trigger timestamp. Combined
   with the submitted_at > $TS filter, a review submitting after the last
   pre-retry read but before the new timestamp lands below the refreshed
   baseline and is omitted from $RIDS — findings untriaged, threads
   unresolved, merge gate blocked far from the cause. A silence retry is the
   same logical round and now keeps the original timestamp; only a
   next-round trigger refreshes it. Pairs with the round-4 multi-review fix:
   that one tolerates two reviews arriving, this one keeps the earlier one
   inside the query window at all.

3. The newly-tracked local skill copy was stale. 6f51b48 tracked it with a
   rationale saying it mirrors the plugin skills while it was still the
   Codex-only fork — unconditional @codex review, hard-coded 10-round cap,
   no reviewer resolution, none of this PR's Copilot fixes — which made the
   drift durable instead of fixing it. It is now byte-identical to
   plugins/quick-dev/skills/review-and-merge/ (verified with diff -r), and
   .claude/skills/README.md records the mirroring rule and re-sync check.

Findings 1 and 2 applied in both plugins and carried into the mirror.
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@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: 887dd4799c

ℹ️ 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 .gitignore
Comment thread plugins/quick-dev/skills/review-and-merge/SKILL.md
…cision timestamps

Codex round 7 — two P2 findings, both valid.

1. Second-precision timestamps have a boundary the baseline cannot survive.
   GitHub's submitted_at is second-precision, so a review submitted inside
   the same second the baseline was captured compares equal to $TS and is
   dropped by the strict `>` filter — the round reads as silent, gets
   re-triggered or classified reason=silent, and its findings go untriaged.
   Loosening to `>=` does not help: it re-admits the previous round's review
   whenever the two share a second.

   Replaced the comparison rather than adjusting it. Each trigger now
   snapshots the ids of reviews the reviewer has already submitted ($SEEN);
   a new review is one whose id is absent from that snapshot, with no
   wall-clock comparison anywhere. Applied at all three sites that compared
   timestamps — the round's $RIDS query, trigger-recovery check (b), and the
   new-response test — and extended to codex, which has the same precision.

   The silence-retry freeze from the previous round still holds but for a
   new reason: refreshing $SEEN mid-round would *include* a review that
   arrived just before the refresh, orphaning it the same way the old
   timestamp refresh did. Both $TS and $SEEN are now frozen across a silence
   retry. Updated that rationale, which described a query form that no
   longer exists.

2. The .gitignore negation was directory-wide. `!.claude/skills/` exposed
   every project-local skill in a clone, not just the mirror — and this
   skill's own `git add -A` would sweep them into a review commit. Narrowed
   to `!.claude/skills/review-and-merge/`, re-ignoring the directory
   contents in between so git still descends into it.

   Verified with a decoy skill: mirror not ignored, decoy ignored,
   settings.local.json ignored, `git add -An .claude/` sweeps nothing.

Both applied in both plugins and carried into the local mirror.
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@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: b54bb717d3

ℹ️ 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 plugins/quick-dev/skills/review-and-merge/SKILL.md Outdated
Comment thread plugins/quick-dev/skills/review-and-merge/SKILL.md
…fore triggering

Codex round 8 — two P2 findings, both in the previous commit.

1. Step 4's definition of a reviewer response still required "created after
   the round's trigger timestamp". The queries had moved to the $SEEN id
   snapshot but the prose governing them had not, so the skill contradicted
   itself and a literal reading of step 4 reintroduced the second-precision
   equality case the snapshot removed. The definition is now id-based, with
   the reason stated inline so it is not simplified back to a timestamp.

2. The snapshot pipeline masked its own failures. `SEEN=$(gh api ... | jq)`
   reports only jq's status, and jq with no input exits 0 and prints
   nothing, so a failed fetch yields a silently empty $SEEN. Empty means
   every already-submitted review looks new and a stale one is triaged as
   this round's; unset means the interpolated index() is invalid jq and the
   round cannot be read at all. Both end in a completed review being
   re-triggered or classified silent — the exact failure the snapshot
   prevents, reintroduced through its own construction.

   Added `set -o pipefail` and an explicit validation step: fetch and parse
   must both succeed, the result must be a parseable JSON array ([] is valid
   and means none yet), and the read is retried until it is. Only then is
   the trigger sent. The read is safe to retry, unlike the trigger.

Both applied in both plugins and carried into the local mirror.
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@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

5. If the round counter is below the cap and the round **produced code changes**: increment the counter, re-trigger the bound reviewer per its profile (codex: re-comment `@codex review`; copilot: re-run the reviewer-request command), return to the top of the loop. Do **not** re-trigger when nothing changed: if every finding in the round was rejected with rationale — including rounds whose findings were only theoretical or insignificant, declined under the step-2 judgment bar — the reviewer would repeat the same findings; resolve the threads and treat the loop as ended.

P1 Badge Keep polling until duplicate requests have settled

When a silence retry leaves both the original and replacement requests outstanding, one review can arrive in the current poll while the other arrives later. This no-code-change branch ends the loop immediately after handling the first response; the all-$RIDS query only covers reviews visible in that poll, so a later body-only Copilot review creates no unresolved thread and can arrive after the final read without blocking the merge gate. Fresh evidence beyond the earlier aggregation issue is that the revised control flow still performs no additional poll or pending-request check before exiting here; wait until all requests from the logical round have settled before treating it as complete.

ℹ️ 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 plugins/quick-dev/skills/review-and-merge/SKILL.md Outdated
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

Re: the P1 in review 4877401452's body — "Keep polling until duplicate requests have settled" (body-level, so no thread to reply in; tracked by review id per the non-inline feedback rule).

Agreed and applied, and I can confirm the failure empirically because it happened to me while driving this PR.

In round 7 my poll returned on the first review and exited. A second review carrying two valid P2 findings landed moments later. I only caught them because I happened to re-query review threads before the merge gate — had I gone straight from the poll result to merging, both would have been merged past. That is your scenario exactly, and it occurred without even needing a silence retry to create the duplicate.

Your point about the body-only case is what makes this a P1 rather than a nuisance: a late Copilot review with findings only in its summary body creates no inline thread, so the all-threads-resolved merge gate cannot catch it. The one safety net that would otherwise cover a missed review does not apply to precisely the review shape Copilot produces most often.

Applied: before a round may be treated as complete — in either branch, code-changed or nothing-changed — a settle poll is now required. Poll once more (~60–90s) and, for copilot, re-check reviewRequests; if the bot is still listed or any id appears that is absent from $SEEN, the round has not settled, so handle the new review and repeat. Only a settle poll that adds nothing may end the round.

I also fixed my own loop tooling when this bit me in round 7 — it now carries a 90-second settle window — but the skill text, which is what anyone else follows, still said to exit on the first response. That gap is what your finding closes.

… settle poll before ending a round

Codex round 9 — two P1 findings, both valid.

1. Inline comments were tested against a snapshot they were never in. Step 3
   snapshotted codex reviews and issue comments; step 4 defined a response
   as any reviewer review OR COMMENT absent from $SEEN. On any PR already
   carrying codex inline comments — the normal case, since step 2 exists to
   process pre-existing feedback — every old inline comment reads as new, so
   the poll returns instantly on already-handled feedback, the round is
   scored complete, and the loop walks to merge without waiting for the
   review it just requested.

   Inline comments are now never matched against the snapshot. $SEEN covers
   reviews plus codex issue comments (its standalone quota/unavailability
   notices); inline comments are collected solely via
   pull_request_review_id in $RIDS, the path the copilot profile already
   used. This also removes the last structural asymmetry between the two
   reviewer paths, which is where several findings in this PR originated.

2. A round could be declared complete before its requests settled. A silence
   retry can leave two requests outstanding, so a second review can arrive
   after the one just handled, and the $RIDS query only saw what existed
   when it ran. The dangerous case is a late body-only Copilot review: it
   creates no inline thread, so the all-threads-resolved merge gate cannot
   catch it and its findings merge past silently.

   Observed for real in round 7 of this PR: the poll returned on the first
   review and exited; a second review carrying two valid P2 findings landed
   moments later, caught only because review threads were re-queried before
   the merge gate.

   Both branches of the loop now require a settle poll (~60-90s, plus a
   reviewRequests re-check for copilot) that adds nothing before the round
   may end.

Both applied in both plugins and carried into the local mirror.
@forhas

forhas commented Aug 6, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@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: 3da55fd9aa

ℹ️ 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 plugins/quick-dev/skills/review-and-merge/SKILL.md Outdated
…he immutable snapshot

Codex round 10 (final, cap) — one P1, introduced by the previous commit.

$SEEN is deliberately the immutable pre-trigger snapshot; that immutability
is what makes it safe across a silence retry (round 6). A response just
handled is therefore by construction absent from it, so the settle poll
added in 3da55fd rediscovered the already-triaged response on every pass,
declared the round unsettled, and looped forever — never reaching another
round or the merge.

Round 9's finding was that the loop ended too early and could merge past
findings; the fix for it made the loop never end at all. A hang rather than
a silent miss: more visible, but strictly worse, since the workflow cannot
complete.

Now tracks $HANDLED — ids triaged during this round — alongside $SEEN. A
response is new only when absent from both. The reason $SEEN alone
deadlocks is stated inline so the two sets are not later collapsed back into
one.

Also bounded the copilot settle path: "bot still listed in reviewRequests"
now waits under the same ~30-minute total as the silence rule rather than
indefinitely.

Applied in both plugins and the local mirror.

NOTE: merged at the round cap. This commit and 3da55fd were not themselves
reviewed by a round.
@forhas
forhas merged commit da7dbae into main Aug 6, 2026
@forhas
forhas deleted the fix/copilot-author-matching-and-suppressed-comments branch August 6, 2026 18:52
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