Skip to content

fix(fx-dev): key Copilot review readiness on the head SHA - #23

Merged
fx merged 6 commits into
mainfrom
fix/copilot-review-readiness
Aug 10, 2026
Merged

fix(fx-dev): key Copilot review readiness on the head SHA#23
fx merged 6 commits into
mainfrom
fix/copilot-review-readiness

Conversation

@fx

@fx fx commented Aug 10, 2026

Copy link
Copy Markdown
Owner

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/fix is NOT in this PR (see Follow-ups).

Readiness is now keyed on the only sound signal available: a review whose commit_id equals the current headRefOid. The waiter emits REVIEWED_COMMIT_ID, PR_HEAD_SHA, and SUPPRESSED_COMMENTS=1|0 so a caller can verify the gate rather than trust the exit code.

The four defects, all confirmed empirically on PRs #19-#22

  • D1POST /pulls/N/requested_reviewers returns 200 with requested_reviewers: [], 7 times out of 7. A 200 here is never evidence a review was requested.
  • D2 — GraphQL requestReviews rejects Bot nodes outright (Could not resolve to User node … BOT_…), so it is not a workaround for D1.
  • D3 — the old check_review_requested keyed 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.
  • D4 — a review body can say "generated no new comments" while carrying a 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-review found 12 findings, 3 HIGH. The sharpest first:

  1. HIGH — the head-SHA re-confirmation failed open. [[ -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 both REVIEWED_COMMIT_ID and PR_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, and report_success refuses to emit a pair from an unconfirmed head read.
  2. HIGH — github/SKILL.md's blocking merge-gate table accepted a review of any commit (no commit_id scoping) — this branch's central defect, sitting in the repo's most authoritative gate table and contradicting the same file two sections later.
  3. HIGH — three surviving instructions still treated reviewRequests as meaningful. Since that field is always empty (D1), the negative branch always fired, producing the exact false conclusion D3 retires.
  4. MEDIUM — the timeout budget counted only sleep time, so wall clock overran the Bash tool's cap and the tool killed the script before it could return exit 1 — meaning the documented "re-run on exit 1" protocol never triggered. Now wall-clock via SECONDS.
  5. MEDIUM — the script's own 900 s default exceeded the cap the skill documents. Now consistent 480 s / timeout: 540000.
  6. MEDIUM — the suppressed-comments check was advisory: a printed banner, no machine-readable signal, no effect on the exit code, so callers branching on exit 0 recorded "gate passed" without reading the body. Now emits SUPPRESSED_COMMENTS.
  7. MEDIUM — only the newest review of a commit was inspected, while the script's own nudging makes two reviews of one commit normal; a later "no new comments" review masked an earlier suppressed block. Now greps across all review bodies for that commit.
  8. MEDIUM/LOW — unfiltered thread counts that made the gate unsatisfiable on any human thread; the stale "30-90 s" Copilot timing contradicting D3 in the same file; and in resolve-pr-feedback, an unscoped suppressed-block fetch, the login Copilot (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.sh is 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.md gains a "Known GitHub API Behaviour" section (D1-D4), mandatory suppressed-comments triage, and a gate redefined as three conjunctive conditions. dev/SKILL.md is deliberately limited to one line (the stale timing) to keep this PR scoped.

Test plan

Review coverage

Two of the four pre-PR review passes did not run: local codex and cr are both unauthenticated, as on every PR this session. codex login status reports "Logged in using ChatGPT" while codex review 401s 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 run codex 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:735 has the same unfiltered thread count fixed elsewhere here; dev/SKILL.md was limited to one line in this PR.
  • github/SKILL.md still says "Poll every 60 seconds for up to 15 minutes", superseded by the 15 s interval and 480 s x 3 budget.
  • Remaining trace-establishment work for coder/dev/team/fix.
  • Two cosmetic nits carried from PR feat(fx-dev): offer duvet adoption from setup and upgrade #22: the entries<, conditional-span readability, and grep '^\[tools\]' routing a [tools.rust]-only config to the wrong branch.
  • A durable opt-out for a declined duvet adoption (currently recurs once per session).

fx added 3 commits August 10, 2026 09:33
…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.
Copilot AI lite review requested due to automatic review settings August 10, 2026 17:02

Copilot AI 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.

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, and resolve-pr-feedback skill 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.

Comment thread plugins/fx-dev/skills/resolve-pr-feedback/SKILL.md
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.
@fx
fx requested a lite review from Copilot August 10, 2026 17:08

Copilot AI 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.

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.
@fx
fx requested a lite review from Copilot August 10, 2026 17:14

Copilot AI 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.

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_bodies swallows gh api failures (|| true), so a transient API error can make bodies empty and the script will emit SUPPRESSED_COMMENTS=0 and "(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 setting SUPPRESSED_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_NUMBER in gh pr view PR_NUMBER / /pulls/PR_NUMBER/..., but ${HEAD_SHA} is treated as a shell variable. This mixes placeholder styles and reads like PR_NUMBER is also a shell variable, which would fail if copy/pasted. Consider defining PR_NUMBER as 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_NUMBER placeholders with shell variables (e.g. ${HEAD_SHA}), which makes it look like PR_NUMBER is a defined variable when it isn't. Defining PR_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.
@fx
fx requested a lite review from Copilot August 10, 2026 17:32

Copilot AI 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.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@fx
fx merged commit 3240231 into main Aug 10, 2026
3 checks passed
@fx
fx deleted the fix/copilot-review-readiness branch August 10, 2026 18:04
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.

2 participants