* feat(review): add comment-status helper for existing-thread triage
One deterministic pass over a PR's existing inline comments, replacing
the per-comment `gh api` fetches the orchestrating model used to make
during /review: anchor validity at the live head (outdated detection,
with a file-level exemption), whether the anchored file changed in the
reviewed worktree since each comment's commit and which commits touched
it (the re-check's candidate "fixed by" list), reply participation and
PR-author response, the blocker signal (same carriesBlockerSignal as
pr-context, so the two surfaces agree by construction), and
worktree-vs-live head drift.
Measured on a heavily discussed PR (72+ inline comments), a single
review run burned 20+ model turns re-deriving exactly these fields one
comment id at a time. SKILL.md now runs the subcommand in Step 1 and
routes the Step 6 re-check's status questions at the report; comment
bodies stay in the pr-context file under its untrusted-data preamble,
and a comment-status failure only warns — it is an index, not the
evidence, so it never sets the context-unavailable state.
* test(review): add comment-status to the subcommand registry expectations
* fix(review): comment-status review follow-ups — size warning, --host wiring, scope clauses
Addresses the review at d098e4feb:
- High: warn when the report exceeds read_file's truncation threshold,
mirroring pr-context — measured 53k chars on the benchmark PR, where a
single read lost 36 of 71 threads (24 blocker-flagged) and the cut JSON
did not parse. The warning points at jq first: the file is
machine-shaped in a way the Markdown context file is not.
- Medium: thread --host through — the SKILL.md command block now says to
pass it (each subcommand is its own process, so a host set elsewhere
cannot carry over), and comment-status joins the host-required lists in
SKILL.md and the code-review docs.
- Minor: Step 6's routing sentence now states both scope limits — the
report exists only when Step 1 wrote it (worktree mode), and it indexes
inline threads only; issue-/review-level blockers keep the context-file
walk.
- Minor: touchedByTotal exposes the real commit count behind the capped
touchedBy list, so a cut list is visible instead of reading as "the fix
is not among them".
- Nits: drop the never-read `side` field; guard authorReplied against a
deleted-author/deleted-replier '' === '' match; correct the force-push
doc comment (a shared object database usually retains the old commit,
so the range widens — fail-safe — rather than going unknown).
* fix(review): comment-status second-round follow-ups — CWD-safe pathspec, shared walk, stale flag
Addresses the LGTM-with-suggestions round:
- The git probe's pathspec is anchored with `:(top)`: run from a
subdirectory of the worktree, the old CWD-relative form returned empty
output with exit 0 and every thread read as "untouched since the
comment" — silent, and pointing the one direction this index must not
fail in. A real-git integration test now drives the probe from the
repo root AND a subdirectory (plus the cap, the memo, and the
missing-commit gate — none of which the injected-probe unit tests
could see).
- findRootId is imported from pr-context (made generic and exported)
instead of duplicated — the thread walk now agrees by construction,
like the blocker signal already did.
- Head drift is denormalized onto every thread as code.staleWorktree, so
a jq consumer of threads[] cannot skip the top-level flag by
construction.
- Commit existence is memoized per SHA (it never depended on the path),
halving git spawns on thread-heavy PRs; summarizeThreads gets a named
return interface like every other exported shape here.
- DESIGN.md gains the missing section: why comment-status is a separate
subcommand and why the second fetch of pulls/{n}/comments is
deliberate (process boundary — pr-context must stay pure-API for
lightweight mode; this one exists to join API facts with worktree git).
* fix(review): harden comment-status against untrusted-PR inputs
Addresses the security review round:
- Symlink --out: the command now runs from the trusted main checkout (so
a relative --out cannot be redirected through a symlink an untrusted PR
planted in its own worktree) and scopes its git queries to the worktree
with `git -C <worktreePath>`, which it locates itself. SKILL.md no
longer cd's into the worktree for it. The report lands in the main
checkout's .qwen/tmp alongside every sibling report.
- Non-ancestor comment commit: after a force-push the anchor commit can
survive in the shared object store without being on HEAD's history, so
`sinceSha..HEAD` is empty and a changed file reads as changed:false —
the one direction this index must not fail in. A single
`merge-base --is-ancestor` gate now returns 'unknown' for a
non-ancestor OR a missing commit, replacing the cat-file existence
check (one git process instead of two).
- Literal pathspec: the GitHub-supplied path is passed as
`:(top,literal)<path>` so a value like `:(exclude)a.ts` cannot be read
as pathspec magic and inspect unrelated files.
- Fetch-race drift: the live head is sampled before AND after the
comments fetch; a push landing mid-fetch (which would pair newer
anchor mappings with a stale comparison) is now detected, recorded as
both samples, and warned on distinctly from ordinary worktree lag.
- pr-context renders the root comment id in the Open and Already-discussed
sections, giving Step 6 a stable join key back to comment-status's
per-thread rootId (the blocker renderer already did this).
- Handler-level tests (mocked gh/git/fs) for the three drift outcomes,
plus real-git integration cases for the non-ancestor gate and the
literal pathspec. Multi-page pagination is a non-issue: gh api
--paginate merges top-level arrays into one (verified on the live
93-comment PR).
* fix(review): comment-status round 3 — precise staleWorktree, worktree-missing warning, discriminating pathspec test
Addresses three Suggestions:
- staleWorktree is now keyed on worktreeStale alone, not the headDrift
union. A head that merely moved between the two samples while the
worktree already matches the final head is NOT a superseded checkout,
so its threads no longer carry staleWorktree:true against the field's
documented meaning. headMovedDuringFetch stays a separate top-level
flag + warning.
- A missing worktree (comment-status run before fetch-pr or after
cleanup) now sets worktreeMissing on the report and prints a warning —
previously every thread degraded to code:'unknown' silently, readable
as "nothing changed".
- The literal-pathspec test now uses a discriminating pathspec
(`:(glob)pkg/**`): magic would match the changed file (true), literal
is a nonexistent filename (false), so asserting false actually fails if
the `:(top,literal)` prefix is dropped — the old `:(exclude)…` read
false under both interpretations. A plain-path control proves the probe
is live.
* test(review): make the comment-status negation test actually exercise negation
The body 'No blockers here' matched no BLOCKER_PATTERN (the bare plural
never triggers /\bblocking\b/ etc.), so isBlocker returned false before
the negation branch ran — false coverage. It now uses 'No blocking
issues', which matches the signal and must be suppressed by the leading
'No', plus an un-negated control that asserts true.
* test(review): assert per-thread staleWorktree and --host wiring; document ghApiAll merge contract
Two test gaps + one recurring-review clarification:
- The worktree-lag drift test now includes a thread and asserts
staleWorktree:true is denormalized onto its code object — the old
positive case had zero threads, so the denormalization loop iterated
over nothing and would pass even if the block were deleted.
- A --host test now asserts setGhHost is called with the argv host,
matching the presubmit analog; without it a dropped setGhHost would
silently target github.com for a GHE review.
- ghApiAll's doc now explains why one JSON.parse is correct on multi-page
output: gh --paginate MERGES top-level arrays into one (it does not emit
one array per page); the per-page-concat failure only affects
key-nested arrays, which is exactly why ghApiAllNested exists. Verified
on a 4-page 97-comment response.
* fix(review): comment-status degrades gracefully on failure per its SKILL.md contract
SKILL.md promises this command is an index, not evidence — "if it fails
(auth, network), warn and continue" — but runCommentStatus had no
try/catch, so an ensureAuthenticated() or gh throw propagated as an
unhandled rejection and killed the whole review. The runtime body is now
wrapped: any throw writes a minimal empty report ({prNumber, ownerRepo,
error, threads: []}) so downstream jq still parses, prints a
"comment-status failed" warning, and exits 0. Handler test pins the
auth-failure path (no throw, empty report, warning). Reported by
@yiliang114.
* test(review): pin comment-status owner_repo guard and truncation-size warning
Two untested paths flagged in review: the owner_repo-without-slash guard
(a caller error that must still throw, distinct from the runtime
graceful-degradation path) and the report-size warning that fires when
the JSON crosses read_file's truncation threshold.
* fix(review): comment-status degraded report carries the full shape, not a stripped one
The graceful-degradation path wrote { prNumber, ownerRepo, error,
threads: [] }, omitting headDrift/summary/headMovedDuringFetch/etc. A
consumer reading report.headDrift then got undefined (falsy = "no
drift"), silently mistaking a total index failure for a clean "nothing
moved" — and the review orchestrator keys code-fact warnings on exactly
that field. The degraded report now emits the same shape as the success
report with safe defaults plus `error`, so a consumer that checks `error`
sees the failure and one that reads a fact gets a neutral value, never a
misleading one. Reported by @doudouOUC.
* fix(review): degraded report's worktreeMissing must not contradict worktreeHeadSha: null
The catch-block report hardcoded worktreeMissing: false beside
worktreeHeadSha: null — a positive "worktree present" assertion the
success path (worktreeMissing = worktreeHeadSha === null) would never
make for a null head. A consumer reading worktreeMissing without gating
on error would conclude the worktree exists on a run where nothing is
known. Now true, matching the null head and the fail-safe reading (code
facts unavailable). Reported by qwen-code-ci-bot.
---------
Co-authored-by: verify <verify@local>
a470ba626c