fix(review): address post-merge P2 review feedback on the review skill

packages/cli/src/commands/review/comment-status.handler.test.ts

@@ -263,5 +263,50 @@ describe('comment-status handler', () => {
     // worktreeMissing must not contradict worktreeHeadSha: null.
     expect(report.worktreeHeadSha).toBeNull();
     expect(report.worktreeMissing).toBe(true);
+    // prAuthor is null (not '') when degraded: '' is reserved for a legitimately
+    // absent author on the success path, so a structural null keeps a total
+    // index failure distinguishable from a deleted PR-author account.
+    expect(report.prAuthor).toBeNull();
+  });
+
+  it('keeps the already-fetched comments when the second head sample fails', async () => {
+    // The comments were fetched successfully; a transient failure on the
+    // race-detection head sample (the SECOND gh pr view) must fall back to
+    // liveHeadBefore rather than discarding everything into a degraded report.
+    // clearAllMocks does not reset implementations, so pin auth to a no-op in
+    // case a prior test left a throwing one (this must NOT take the degraded
+    // path).
+    mocks.ensureAuthenticated.mockImplementation(() => {});
+    mocks.ghApiAll.mockReturnValue([
+      {
+        id: 1,
+        user: { login: 'r' },
+        path: 'a.ts',
+        line: 1,
+        original_commit_id: 's',
+      },
+    ]);
+    let n = 0;
+    mocks.gh.mockImplementation((..._args: string[]) => {
+      n += 1;
+      if (n === 1) {
+        return JSON.stringify({
+          author: { login: 'octocat' },
+          headRefOid: 'headA',
+        });
+      }
+      throw new Error('gh pr view: 502 Bad Gateway');
+    });
+    await run();
+    const report = reportWritten();
+    // Not degraded: no top-level error, comments survived.
+    expect(report.error).toBeUndefined();
+    expect(report.inlineComments).toBe(1);
+    expect(report.threads).toHaveLength(1);
+    // Fell back to the pre-fetch sample: liveHeadAfter === liveHeadBefore, so
+    // headMovedDuringFetch reads false (the safe "head did not move" default).
+    expect(report.liveHeadBefore).toBe('headA');
+    expect(report.liveHeadSha).toBe('headA');
+    expect(report.headMovedDuringFetch).toBe(false);
   });
 });

packages/cli/src/commands/review/comment-status.ts

@@ -307,6 +307,11 @@ interface CommentStatusArgs {
   pr_number: string;
   owner_repo: string;
   out: string;
+  // Read by the handler via setGhHost before runCommentStatus. Declared here so
+  // the full contract is visible at the type level: if a future cleanup moves
+  // the setGhHost call inside runCommentStatus, dropping host becomes a type
+  // error rather than a silent runtime regression.
+  host?: string;
 }
 
 async function runCommentStatus(args: CommentStatusArgs): Promise<void> {
@@ -347,20 +352,32 @@ async function runCommentStatus(args: CommentStatusArgs): Promise<void> {
       `repos/${owner}/${repo}/pulls/${prNumber}/comments`,
     ) as RawStatusComment[];
 
-    const liveHeadAfter =
-      (
-        JSON.parse(
-          gh(
-            'pr',
-            'view',
-            prNumber,
-            '--repo',
-            ownerRepo,
-            '--json',
-            'headRefOid',
-          ),
-        ) as { headRefOid?: string }
-      ).headRefOid ?? '';
+    // Second head sample for race detection (headMovedDuringFetch). This is an
+    // enhancement, not a core fact: if it fails transiently AFTER the comments
+    // were already fetched, fall back to liveHeadBefore instead of discarding
+    // the whole report (comments + anchor facts are already in hand). With the
+    // fallback, headMovedDuringFetch reads false — the safe default (assume the
+    // head did not move) — and worktreeStale still compares the worktree against
+    // liveHeadBefore, so a usable drift signal survives.
+    let liveHeadAfter = liveHeadBefore;
+    try {
+      liveHeadAfter =
+        (
+          JSON.parse(
+            gh(
+              'pr',
+              'view',
+              prNumber,
+              '--repo',
+              ownerRepo,
+              '--json',
+              'headRefOid',
+            ),
+          ) as { headRefOid?: string }
+        ).headRefOid ?? liveHeadBefore;
+    } catch {
+      // Race-detection sample unavailable; liveHeadBefore is a usable fallback.
+    }
 
     const worktree = worktreePath(prNumber);
     const worktreeHeadSha = gitOpt('-C', worktree, 'rev-parse', 'HEAD');
@@ -478,7 +495,12 @@ async function runCommentStatus(args: CommentStatusArgs): Promise<void> {
           prNumber,
           ownerRepo,
           error: msg,
-          prAuthor: '',
+          // null, not '': the success path emits '' only for a legitimately
+          // absent author (deleted account). A degraded run knows nothing about
+          // the author, so a structural null (matching worktreeHeadSha below)
+          // keeps a consumer that displays the author name from rendering a
+          // blank as if it were a real empty value.
+          prAuthor: null,
           liveHeadSha: '',
           liveHeadBefore: '',
           worktreeHeadSha: null,

packages/cli/src/commands/review/submit.test.ts

@@ -14,6 +14,7 @@ import {
   mkdtempSync,
   mkdirSync,
   readFileSync,
+  readdirSync,
   rmSync,
   utimesSync,
   writeFileSync,
@@ -756,4 +757,13 @@ describe('submit receipt (producer half of the audit contract)', () => {
     const receipt = JSON.parse(readFileSync(receiptPath(), 'utf8'));
     expect(receipt.reviewIds).toEqual([7, 8]);
   });
+
+  it('writes atomically, leaving no .tmp sibling behind', () => {
+    ghMock.mockImplementationOnce(() => JSON.stringify({ id: 42 }));
+    runSubmit(authorizedPost());
+    expect(readFileSync(receiptPath(), 'utf8')).toContain('"reviewIds"');
+    const tmpDir = join(dir, '.qwen', 'tmp');
+    const leftovers = readdirSync(tmpDir).filter((f) => f.endsWith('.tmp'));
+    expect(leftovers).toEqual([]);
+  });
 });

packages/cli/src/commands/review/submit.ts

@@ -44,7 +44,8 @@
 // caller's.
 
 import type { CommandModule } from 'yargs';
-import { mkdirSync, readFileSync, writeFileSync } from 'node:fs';
+import { atomicWriteFileSync } from '@qwen-code/qwen-code-core';
+import { mkdirSync, readFileSync } from 'node:fs';
 import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
 import { ghWithInput, setGhHost } from './lib/gh.js';
 import { REVIEW_TMP_DIR, tmpFile } from './lib/paths.js';
@@ -563,10 +564,9 @@ export function runSubmit(args: SubmitArgs): void {
       const priorIds = readReceiptIds(receiptPath);
       const reviewIds = [...new Set([...priorIds, reviewId])];
       mkdirSync(REVIEW_TMP_DIR, { recursive: true });
-      writeFileSync(
+      atomicWriteFileSync(
         receiptPath,
         `${JSON.stringify({ reviewIds, event, postedAt: new Date().toISOString() })}\n`,
-        'utf8',
       );
     }
   } catch {

packages/core/src/skills/bundled/review/SKILL.md

@@ -130,7 +130,7 @@ Based on the parsed `target.type`:
 
     **`read_file` returns the first `truncateToolOutputThreshold` characters (25 000 by default) and sets `isTruncated`. Read that flag.** On a PR with a long history the context file exceeds it — `pr-context` prints a `warning:` line naming the size and any headings past the cut. When it does, page the remainder with `offset`/`limit` before Step 3, and pass the _whole_ file's contents onward. A review that never reached the open-comment section will report "no blockers" without having seen a single one of them.
 
-  - **Fetch the comment STATUS index** (worktree mode, whenever the context file reports existing inline comments). Run it **from the main checkout, exactly like the other subcommands** — do NOT `cd` into the worktree for it: it locates the PR worktree itself and scopes its git queries there with `git -C`, while writing its `--out` report into the trusted main-checkout `.qwen/tmp` alongside the others. (Running it from inside the untrusted worktree would let a PR redirect that relative `--out` through a planted symlink.)
+  - **Fetch the comment STATUS index** (worktree mode **only** — skip it in lightweight mode, where no worktree exists). Note the guard is worktree presence, **not** "the context file reports inline comments": `pr-context` runs in both modes and reports existing inline comments either way, so that signal alone would send a lightweight run at a command it cannot serve. When a worktree exists, run it whenever the context file reports existing inline comments, **from the main checkout, exactly like the other subcommands** — do NOT `cd` into the worktree for it: it locates the PR worktree itself and scopes its git queries there with `git -C`, while writing its `--out` report into the trusted main-checkout `.qwen/tmp` alongside the others. (Running it from inside the untrusted worktree would let a PR redirect that relative `--out` through a planted symlink.)
 
     ```bash
     "${QWEN_CODE_CLI:-qwen}" review comment-status <pr_number> <owner>/<repo> \
@@ -679,7 +679,7 @@ If the user responds with "post comments" (or similar intent like "yes post them
 
 ## Step 7: Submit PR review
 
-**You do not post. `qwen review submit` posts, and it refuses when the run is not authorised.** That is a claim about **every write path to the pull request**, not one API route: no `gh api repos/.../pulls/<n>/reviews` (not to submit, not to "test" an anchor), no `gh pr comment`, no `gh pr review`, no `gh issue comment`, no `gh api` with POST/PATCH/PUT/DELETE against the PR's `issues/*` or `pulls/*` endpoints, and no editing or deleting existing comments. **You do not author PR-facing prose at all** — `compose-review` computes the review body from structured state (the verdict, the downgrade reasons, the body-Criticals), and there is no free-text field to pass through it; a free-form note you want to add is a note for the **terminal summary**, which the user reads, not for the pull request. The only text that reaches the PR is that computed body plus the inline finding comments, and both ride the one sanctioned write below. Dogfooded the hard way: a run that had lost these instructions to four context compressions decided its findings were "all duplicates", never called submit, and hand-posted a consolidated summary with `gh pr comment` — a write with no authorisation gate, no downgrade semantics, no `posted` fact, and no completion line; nothing downstream could tell it had happened. `cleanup` now audits the review window and flags issue comments by the reviewing account (submit never posts one — see Step 9), so that bypass is at least named in the terminal — a tripwire, not permission. The one write in this skill lives behind a check:
+**The whole rule in one sentence, so it survives even when the rest is compressed away: never run a `gh` command that writes to the pull request — `qwen review submit` is the only write path in this skill, and it refuses when the run is not authorised.** Everything below only spells out what "writes" covers so a compressor cannot quietly narrow it to a single API route. It is **every write path to the PR**, not one: no `gh api repos/.../pulls/<n>/reviews` (not to submit, not to "test" an anchor), no `gh pr comment`, no `gh pr review`, no `gh issue comment`, no `gh api` with POST/PATCH/PUT/DELETE against the PR's `issues/*` or `pulls/*` endpoints, and no editing or deleting existing comments. **You do not author PR-facing prose at all** — `compose-review` computes the review body from structured state (the verdict, the downgrade reasons, the body-Criticals), and there is no free-text field to pass through it; a free-form note you want to add is a note for the **terminal summary**, which the user reads, not for the pull request. The only text that reaches the PR is that computed body plus the inline finding comments, and both ride the one sanctioned write below. Dogfooded the hard way: a run that had lost these instructions to four context compressions decided its findings were "all duplicates", never called submit, and hand-posted a consolidated summary with `gh pr comment` — a write with no authorisation gate, no downgrade semantics, no `posted` fact, and no completion line; nothing downstream could tell it had happened. `cleanup` now audits the review window and flags issue comments by the reviewing account (submit never posts one — see Step 9), so that bypass is at least named in the terminal — a tripwire, not permission. The one write in this skill lives behind a check:
 
 ```bash
 "${QWEN_CODE_CLI:-qwen}" review submit \