fix(fx-dev): key Copilot review readiness on the head SHA - #23
Conversation
…quests
The copilot-review waiter gated its poll on `requested_reviewers`, a signal
that does not work. Four behaviours, all confirmed against real PRs on this
repo:
D1 POST .../requested_reviewers with the Copilot bot returns 200 with an
empty `requested_reviewers` array every time, and a follow-up GET is empty
too. The response is never evidence of anything.
D2 The GraphQL `requestReviews` fallback cannot work at all — `userIds`
rejects Bot nodes outright.
D3 Because readiness was keyed on D1's dead field, the script returned exit 2
("No Copilot review requested") against reviews that were genuinely
delivered — on one PR it would have done so at every point in a 12m42s
window including after the review landed. The documented recovery ("go
request one, then re-run") looped forever, since the request is a no-op.
D4 A zero unresolved-thread count is not a clean review. Review bodies can
say "generated no new comments" while carrying a `Suppressed comments`
block of real findings that create no thread at all.
The waiter now polls `/pulls/{n}/reviews` at 15s intervals for a review whose
`commit_id` equals the PR's current `headRefOid` — the only sound signal — and
re-confirms against a fresh head read before declaring success, so a push
landing mid-poll cannot be satisfied by the review it superseded. It still
issues the review request as a delivery nudge but never branches on the
result. Exit 1 is now unambiguously a timeout ("re-run to keep waiting"),
never a verdict; exit 2 is retired and documented as never returned so no
caller reinstates a recovery keyed to the broken signal. Success emits
`REVIEWED_COMMIT_ID` and `PR_HEAD_SHA` so callers verify the match themselves,
and prints the review body with the suppressed block flagged.
SKILL.md records D1-D4 with their evidence so future runs do not rediscover
them, promotes the suppressed-comments read to a mandatory step whose omission
fails the review, and states the gate passes only on a head-matching review
whose suppressed block has been triaged. Its foreground timeout guidance also
exceeded the Bash tool's 600000ms cap, so it now prescribes 540s runs re-run
up to 3 times. github/SKILL.md's "treat a 422 as success" advice was the same
D1 mistake and is corrected.
Minor, not patch: no caller branching on exit 0/1/3 breaks, but this adds a
machine-readable stdout contract and a new mandatory gate condition.
Verified: script exits 0 with the correct commit_id on PRs 19, 20, 21 and 22;
the suppressed-comments warning fires on 22 (a review reporting "no new
comments" with 0 threads and a real finding); the timeout path exits 1 with a
stale-review diagnostic on 17. duvet query -c implementation 16/16, report
--ci clean.
plugins/fx-dev/skills/dev/scripts/wait-for-copilot-review.sh was dead code: nothing referenced it by path, and dev/SKILL.md's "bundled copilot-review waiter" points at the copilot-review skill's script. It also implemented the exact defect this branch fixes. With zero occurrences of headRefOid or commit_id, it matched any Copilot review by author login and reported "already present", so it would exit 0 on a review of a superseded commit. This is a deliberate widening beyond the two files the fix targeted. Leaving a dead merge-gate script that implements the bug we just fixed is a hazard: deletion is zero-risk because nothing references it, while a future wiring-up would silently reintroduce a lying merge gate.
The head-SHA doctrine landed in the previous two commits but its own instructions still contradicted it in twelve places, several of them on merge-gate paths. This closes the gap. Waiter script: - The success re-confirmation FAILED OPEN. `current_head_sha` returns empty on a FAILED API read, and the guard treated empty as "head unchanged", so a transient read failure could print a superseded SHA as both REVIEWED_COMMIT_ID and PR_HEAD_SHA — passing the caller's mandated equality check on a stale review. Empty now keeps waiting, and a SHA pair is emitted only from a head read that succeeded. The per-poll head-read failure is logged instead of silently retaining the old value. - Timeout was budgeted against accumulated sleep time, so a 540 s run took 578-650 s of wall clock and the caller's Bash-tool timeout killed the script before it could return exit 1 — making the "re-run on exit 1" protocol and the timeout diagnostics unreachable. Budget is now wall clock (`SECONDS`), and the final sleep is clamped to the deadline. - Default timeout was 900 s, over the 600 s Bash-tool cap the skill layer already documents. Lowered to 480 s, paired with `timeout: 540000`. - The suppressed-comments check was advisory: a banner with no machine-readable signal, so a caller branching on exit 0 recorded "gate passed" without reading the body. Now emits `SUPPRESSED_COMMENTS=1|0` beside the two SHAs, and the skill requires the caller to check it. - Only the newest review of the head SHA was inspected for suppressed comments. Two reviews of one commit are routine, so a later "generated no new comments" masked an earlier suppressed block. Greps all bodies for the commit. github/SKILL.md: - The blocking merge-gate table verified "Copilot review RECEIVED" with no `commit_id` scoping — it accepted a review of any commit, contradicting the same file twice over. Scoped to the head SHA, plus a blocking row for suppressed-comments triage and a Copilot-login filter on the thread row. - Removed the "check if the review is pending" instructions. That field is empirically always empty, so the negative branch fired on every PR and produced exactly the false conclusion D3 forbids. Replaced with head-scoped review queries; Status Interpretation no longer keys any row off a review request. - "Poll every 60 seconds for 15 minutes" now defers to the waiter. resolve-pr-feedback/SKILL.md (deliberate widening, justified): The brief scoped this to copilot-review, but every finding here is an instance of the same D1/D3/D4 defects this branch establishes as doctrine, this skill is itself a merge-gate path, and the login bug made it categorize zero Copilot threads on every PR — so the resolver was never invoked while the skill reported nothing to do. Shipping doctrine that its own sibling skill contradicts is worse than a wider diff. - Copilot thread author login was `Copilot`, which never occurs; the real value is `copilot-pull-request-reviewer`. Fixed in both the table and the categorization step. - The suppressed-comments fetch was unscoped, so after a push it greps the PREVIOUS commit's body and reports D4 satisfied for uncovered code. - Convergence retained D3-forbidden "was requested" framing; the only condition is received for the current head SHA. Its verification snippet used `| last` without `// empty`, printing literal `null` for comparison against a SHA under "these MUST match". - The final thread count was unfiltered, so one open human thread — which fx-dev:github forbids touching — made it permanently unsatisfiable. Also corrected the "Copilot is fast (30-90 s)" claim in copilot-review and in one line of dev/SKILL.md; both contradicted the 85 s-12 m 42 s range recorded in the same doctrine that the three-run budget is derived from. Verification: the waiter exits 0 with REVIEWED_COMMIT_ID == PR_HEAD_SHA on merged PRs #19-#22, reporting SUPPRESSED_COMMENTS=1 for #22 (real suppressed block) and 0 for the others. A simulated unreadable head makes it exit 1 emitting no SHA pair, where the pre-fix script exits 0 with an identical pair. duvet query -c implementation 16/16 and report --ci pass.
There was a problem hiding this comment.
Pull request overview
This PR hardens the fx-dev Copilot merge gate by redefining “review readiness” as “a Copilot review whose commit_id matches the PR’s current headRefOid”, and updates the associated skills/scripts/docs to avoid relying on empirically unreliable requested_reviewers / reviewRequests signals.
Changes:
- Reworked the Copilot waiter script to poll for a review scoped to the current head SHA, emit machine-checkable outputs (
REVIEWED_COMMIT_ID,PR_HEAD_SHA,SUPPRESSED_COMMENTS), and budget timeouts against wall-clock. - Updated
github,copilot-review, andresolve-pr-feedbackskill docs to key readiness/convergence on delivered reviews for the head SHA and to require suppressed-comments triage. - Removed dead/unsafe legacy waiter script and bumped plugin + marketplace versions.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md | Updates Copilot author matching and head-SHA-scoped suppressed-comments checks; improves automated-thread filtering guidance. |
| plugins/fx-dev/skills/github/SKILL.md | Tightens merge gate definition to require a Copilot review of the current head SHA and suppressed-comments triage; removes “pending request” heuristics. |
| plugins/fx-dev/skills/dev/SKILL.md | Updates observed Copilot latency guidance used for budgeting reviewer waits. |
| plugins/fx-dev/skills/dev/scripts/wait-for-copilot-review.sh | Deletes unused legacy script that could pass the gate on stale reviews. |
| plugins/fx-dev/skills/copilot-review/SKILL.md | Adds documented “known API behavior” and redefines the gate as head-SHA-scoped + suppressed-comments-aware. |
| plugins/fx-dev/skills/copilot-review/scripts/wait-for-copilot-review.sh | Implements head-SHA-scoped polling with fail-closed reconfirmation and machine-readable outputs. |
| plugins/fx-dev/.claude-plugin/plugin.json | Bumps fx-dev version to 3.8.0. |
| .claude-plugin/marketplace.json | Bumps marketplace metadata version to 0.21.0. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot flagged a missing `sort` before `group_by(.)` in the resolve-pr-feedback unresolved-thread tally, claiming group_by only groups adjacent equal elements. That is false: jq's group_by(f) sorts by f internally before grouping. Verified on jq-1.8.2 — grouping an unsorted array yields the same output as prefixing an explicit sort. Add the rule to REVIEW.md rather than adding a redundant sort, so the false positive is suppressed for every reviewer that reads the file instead of encoding a wrong belief about jq into the code.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
plugins/fx-dev/skills/copilot-review/scripts/wait-for-copilot-review.sh:125
- PR_NUMBER validation claims to require a positive integer, but the regex accepts "0" (and any other all-digit string). PR numbers start at 1, so this will misclassify an invalid PR number as valid and later fail in less clear ways.
if [[ ! "$PR_NUMBER" =~ ^[0-9]+$ ]]; then
plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md:193
- This paragraph says "count" unresolved threads, but the jq pipeline now returns a per-reviewer breakdown array (not a single count). Rewording avoids callers expecting an integer output.
Re-query to count remaining unresolved threads **from automated reviewers only**.
…nd resolver
Both findings came from a Copilot `Suppressed comments (2)` block on this PR's
review. Suppressed comments create no review threads, so there is nowhere on the
PR to reply to or resolve them — the disposition is recorded here instead.
Finding 1 — wait-for-copilot-review.sh argument validation.
The error message promised "PR_NUMBER must be a positive integer" but the regex
`^[0-9]+$` accepted `0`, so `0` passed validation and failed later in a much less
obvious way. Tightened to `^[1-9][0-9]*$`, so `0` (and `00`) now exit 3 with the
same clear message as a non-numeric argument. Resolved: fixed.
The TIMEOUT_SECONDS argument was checked for the same class of mismatch and does
not have it — it states "non-negative integer", `^[0-9]+$` accepts exactly that,
and `TIMEOUT=0` behaves consistently as a zero budget (immediate exit 1). Left
unchanged.
Finding 2 — resolve-pr-feedback SKILL.md described the wrong return shape.
The prose said to "count remaining unresolved threads", but the jq pipeline
returns a per-reviewer breakdown array, not a single integer — a caller following
the prose would expect a number. Reworded to state the actual return shape, and
fixed the second stale description in the convergence loop ("If count > 0" ->
"If the breakdown array is non-empty"). Resolved: fixed.
Verified: `bash -n` clean; `0`, `00`, and `abc` all exit 3; PR 20 still exits 0
with a matching SHA pair. `duvet query -c implementation` 16/16 exit 0;
`duvet report --ci` exit 0.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
plugins/fx-dev/skills/copilot-review/scripts/wait-for-copilot-review.sh:279
review_bodiesswallowsgh apifailures (|| true), so a transient API error can makebodiesempty and the script will emitSUPPRESSED_COMMENTS=0and "(review has an empty body)" even though the body fetch failed. That can incorrectly certify the suppressed-comments gate as clean. Consider treating body-fetch failures as "unknown" and failing closed by settingSUPPRESSED_COMMENTS=1(and printing a warning / placeholder body).
bodies=$(review_bodies "$reviewed_sha")
if printf '%s' "$bodies" | grep -qi 'Suppressed comments'; then
suppressed=1
fi
echo ""
echo "Copilot review received for ${reviewed_sha:0:7} (state: ${state})"
echo "REVIEWED_COMMIT_ID=${reviewed_sha}"
echo "PR_HEAD_SHA=${head_now}"
echo "SUPPRESSED_COMMENTS=${suppressed}"
echo ""
echo "=== Copilot Review Body (every review of ${reviewed_sha:0:7}) ==="
if [[ -n "$bodies" ]]; then
printf '%s\n' "$bodies"
else
echo "(review has an empty body)"
fi
plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md:118
- The bash snippet uses literal
PR_NUMBERingh pr view PR_NUMBER//pulls/PR_NUMBER/..., but${HEAD_SHA}is treated as a shell variable. This mixes placeholder styles and reads likePR_NUMBERis also a shell variable, which would fail if copy/pasted. Consider definingPR_NUMBERas a variable (or using a consistent<PR_NUMBER>placeholder) and then referencing it consistently.
HEAD_SHA=$(gh pr view PR_NUMBER --json headRefOid --jq '.headRefOid')
gh api "/repos/OWNER/REPO/pulls/PR_NUMBER/reviews" \
--jq "[.[] | select(.user.login | startswith(\"copilot-pull-request-reviewer\")) | select(.commit_id == \"${HEAD_SHA}\") | .body] | join(\"\n\n----- (next review of this commit) -----\n\n\")"
plugins/fx-dev/skills/github/SKILL.md:526
- This code block mixes literal
PR_NUMBERplaceholders with shell variables (e.g.${HEAD_SHA}), which makes it look likePR_NUMBERis a defined variable when it isn't. DefiningPR_NUMBER(or using a consistent<PR_NUMBER>placeholder) would make the snippet safer to copy/paste.
REPO_NWO=$(gh repo view --json nameWithOwner --jq '.nameWithOwner')
HEAD_SHA=$(gh pr view PR_NUMBER --json headRefOid --jq '.headRefOid')
# Reviews of the CURRENT head — this is coverage.
gh api "/repos/${REPO_NWO}/pulls/PR_NUMBER/reviews" \
--jq "[.[] | select(.user.login | startswith(\"copilot-pull-request-reviewer\")) | select(.commit_id == \"${HEAD_SHA}\") | {state, submitted_at}]"
Resolves the three findings from Copilot's `Suppressed comments (3)` block on
this PR. Suppressed comments create NO review thread, so no thread existed to
reply to or resolve — per the copilot-review skill's own D4, the disposition
record belongs in the commit message, and this is it.
Finding 1 (HIGH) — wait-for-copilot-review.sh: `review_bodies` swallowed
`gh api` failures with `|| true`. A transient API error yielded an empty
`bodies`, the `Suppressed comments` grep matched nothing, and the script
printed `SUPPRESSED_COMMENTS=0` plus "(review has an empty body)" — certifying
the suppressed-comments gate clean when the fetch had actually failed. That is
the same fail-open shape as the head re-read fixed earlier on this branch, left
behind in the body read.
FIXED by making the check three-state and failing closed:
1 = fetch succeeded, block present
0 = fetch succeeded, no block
unknown = fetch FAILED, the check could not be performed
The states are separated by `gh`'s EXIT STATUS, never by empty output: a
Copilot review with a genuinely empty body is legal and observed, so "empty"
and "never fetched" are different facts that look identical on stdout.
`review_bodies` now carries no `|| true` and no `2>/dev/null`, so the failure is
loud, and `report_success` prints an explicit "THE CHECK DID NOT RUN" warning.
Documented as D5 in the script header and in copilot-review/SKILL.md, whose
caller-facing contract now states that anything other than a definite `0` means
the gate is NOT clean.
Finding 2 — resolve-pr-feedback/SKILL.md: the suppressed-comments snippet mixed
placeholder styles, using literal `PR_NUMBER`/`OWNER`/`REPO` alongside the real
shell variable `${HEAD_SHA}`, so it read as though `PR_NUMBER` were defined and,
if pasted, built a request against a path containing the literal text.
FIXED by giving every non-GraphQL snippet real shell variables, assigned at the
top of the snippet so it is copy-pasteable as-is — applied to all three such
snippets in the file, not only the one flagged. The GraphQL bodies keep their
inline-literal style, which the file mandates for a real reason (`-f query='...'`
is single-quoted, and `$` is GraphQL's own variable sigil). That note is now
scoped to GraphQL explicitly, with the bash convention stated beside it, so the
mix cannot be reintroduced by accident.
Finding 3 — github/SKILL.md: the identical defect in the "has Copilot reviewed
the current head" snippet. FIXED the same way, across all four affected snippets
in the file — two using bare literals, two using `<NUMBER>`/`<PR_NUMBER>`
angle-bracket placeholders next to real variables. Both GraphQL bodies gained
the "literal substitution, no shell expansion" note they lacked, one of which
also clarifies that its `$head` is a jq variable, not a shell one.
`|| true` / `2>/dev/null` audit of wait-for-copilot-review.sh, since this
pattern has now produced two HIGH findings in this single file:
- review_bodies FAIL-OPEN -> fixed (finding 1).
- latest_review_commit FAIL-OPEN -> fixed. On a failed read it
returned empty, and the timeout path then stated "Copilot has never
submitted a review on this PR" as fact — an assertion about the PR derived
from an API error. It now distinguishes unknown from none.
- current_head_sha SAFE by design. Empty means "unknown" and every
caller treats it as a failed read, never as confirmation.
- check_review_submitted SAFE. Empty means "not reviewed yet", so a
failure degrades to continued waiting and at worst a spurious exit 1, which
is documented as not a verdict. Fails in the safe direction.
- count_copilot_threads SAFE. Already three-state: returns "unknown" on
a failed read and validates that the result is numeric. Context-only anyway.
- request_review_nudge SAFE by design (D1): the response proves nothing
and is deliberately discarded.
- gh --version || echo 0.0.0 SAFE. The fallback fails the minimum-version
comparison and exits 3, so it fails closed.
- REPO_NWO=$(gh repo view ...) Informational, not changed. It has no
`|| true`, so under `set -e` a failure exits immediately — but with gh's
status, and exit 1 is this script's documented TIMEOUT code rather than the
environment-error 3. Not fail-open (no reassuring value is emitted), so it
is left alone as out of scope for these findings.
Verification: `bash -n` clean, and every bash snippet in both edited SKILL.md
files parses under `bash -n`. A simulated body-fetch failure (a gh shim failing
only the `.body]` call) yields SUPPRESSED_COMMENTS=unknown where the pre-fix
script yielded 0. PR 20, whose real answer is 0, also flips to unknown under
that shim, while a shim returning an empty body successfully still yields 0 —
so the three states are genuinely distinguished rather than collapsed.
Regression: PR 22 still exits 0 with a matching SHA pair and
SUPPRESSED_COMMENTS=1, PR 20 gives SUPPRESSED_COMMENTS=0, and the arguments
`0`, empty, `abc`, `1.5` and `-3` all still exit 3. duvet query -c
implementation passes 16/16 (exit 0); duvet report --ci exits 0.
Why
The Copilot merge gate could report a pass it never earned. Because every merge decision downstream rests on that gate, fixing it takes priority over the remaining trace-establishment work — a gate that lies is worse than a gate that is missing, since a missing one is at least visible. The remaining trace-establishment work for
coder/dev/team/fixis NOT in this PR (see Follow-ups).Readiness is now keyed on the only sound signal available: a review whose
commit_idequals the currentheadRefOid. The waiter emitsREVIEWED_COMMIT_ID,PR_HEAD_SHA, andSUPPRESSED_COMMENTS=1|0so a caller can verify the gate rather than trust the exit code.The four defects, all confirmed empirically on PRs #19-#22
POST /pulls/N/requested_reviewersreturns 200 withrequested_reviewers: [], 7 times out of 7. A 200 here is never evidence a review was requested.requestReviewsrejects Bot nodes outright (Could not resolve to User node … BOT_…), so it is not a workaround for D1.check_review_requestedkeyed on that always-empty field, so the script returned exit 2 ("No Copilot review requested") against delivered reviews. On one PR it would have done so throughout a 12m42s window including after the review had landed; on a repo without a ruleset auto-request, its documented recovery loops forever.Suppressed comments (N)block holding real findings that create no review thread. Confirmed 3 times. One such finding, applied as Copilot suggested, would have introduced the very bug it claimed to report.Review findings fixed
A pre-PR
/code-reviewfound 12 findings, 3 HIGH. The sharpest first:[[ -z "$confirm_head" || … ]]treated an unreadable head as confirmed, and since the head-read helper swallows errors, a transient 502 made the script print the stale SHA as bothREVIEWED_COMMIT_IDandPR_HEAD_SHA— so the caller's mandated equality check passed on a stale review. The safeguard built to catch stale reviews would itself have certified one. Now fails closed, andreport_successrefuses to emit a pair from an unconfirmed head read.github/SKILL.md's blocking merge-gate table accepted a review of any commit (nocommit_idscoping) — this branch's central defect, sitting in the repo's most authoritative gate table and contradicting the same file two sections later.reviewRequestsas meaningful. Since that field is always empty (D1), the negative branch always fired, producing the exact false conclusion D3 retires.SECONDS.timeout: 540000.SUPPRESSED_COMMENTS.resolve-pr-feedback, an unscoped suppressed-block fetch, the loginCopilot(which never occurs, so it categorized zero Copilot threads on every PR), and convergence keyed on "was requested".Notable
dev/scripts/wait-for-copilot-review.shis deleted. It was dead code — no path referenced it — and it matched any Copilot review, so anything that did wire it up would have passed the gate on a superseded commit.copilot-review/SKILL.mdgains a "Known GitHub API Behaviour" section (D1-D4), mandatory suppressed-comments triage, and a gate redefined as three conjunctive conditions.dev/SKILL.mdis deliberately limited to one line (the stale timing) to keep this PR scoped.Test plan
REVIEWED_COMMIT_ID == PR_HEAD_SHASUPPRESSED_COMMENTSis 0 for feat(ci): add duvet requirements traceability for marketplace structure #19/feat(fx-dev): spec and trace agent authority boundaries #20/feat(fx-dev): teach spec-writer duvet mode and authority-not-procedure #21 and 1 for feat(fx-dev): offer duvet adoption from setup and upgrade #22 — the asymmetry proving fix 6; feat(fx-dev): offer duvet adoption from setup and upgrade #22's head review says "generated no new comments" while carrying the block3ac4c7dhas two reviews; with the order reversed,| lastmisses the suppressed block while the all-bodies join finds itbash -nclean; non-numeric PR number and negative timeout both return 3dev/scripts/wait-for-copilot-review.shduvet query -c implementationexits 0 at 16/16;duvet report --ciexits 0validatepasses (verified by this PR's CI)Requirements traceabilitypasses (verified by this PR's CI)Review coverage
Two of the four pre-PR review passes did not run: local
codexandcrare both unauthenticated, as on every PR this session.codex login statusreports "Logged in using ChatGPT" whilecodex review401s with "Could not validate your refresh token" — which is itself the same class of defect this PR fixes: a status signal that succeeds independently of the function working. The user has been asked to runcodex login.The irony is worth stating plainly: a PR that fixes a lying gate was reviewed by a degraded review matrix. Weigh the Copilot review on this PR accordingly.
Follow-ups (not in this PR)
dev/SKILL.md:735has the same unfiltered thread count fixed elsewhere here;dev/SKILL.mdwas limited to one line in this PR.github/SKILL.mdstill says "Poll every 60 seconds for up to 15 minutes", superseded by the 15 s interval and 480 s x 3 budget.coder/dev/team/fix.entries<,conditional-span readability, andgrep '^\[tools\]'routing a[tools.rust]-only config to the wrong branch.