fix(review): recover bilingual register from the live PR when the plan omits the Han flag

packages/cli/src/commands/review/compose-review.test.ts

@@ -23,6 +23,7 @@ import {
   verdictLine,
   type ComposeReviewInput,
   type ComposeReviewResult,
+  type PrBodyFetcher,
 } from './compose-review.js';
 
 vi.mock('../../utils/stdioHelpers.js', () => ({
@@ -2120,3 +2121,125 @@ describe('bilingual body — the PR author writes Chinese (prDescriptionHasHan)'
     ).toHaveLength(2);
   });
 });
+
+/**
+ * The plan flag is the deterministic path; this is the recovery for when it is
+ * missing. `fetch-pr` always writes `prDescriptionHasHan`, but a `plan-diff`
+ * plan never does, and an orchestrator that improvises the pipeline can hand
+ * `compose-review` a plan that is not `fetch-pr`'s report — which is how a
+ * Chinese-authored PR (#7686) shipped an English-only review while the four
+ * bot reviews before it, off a proper plan, were bilingual. When the flag is
+ * absent but the plan still names the PR, the register is recovered from the
+ * live description, which the caller cannot forge.
+ */
+describe('bilingual body — recovered from the live PR when the plan omits the flag', () => {
+  /** A covered plan with a PR identity but no `prDescriptionHasHan`, its mtime
+   *  kept old so its transcripts still read as newer than it. */
+  function namedPlanWithoutFlag(): string {
+    const p = coveredPlan();
+    const parsed = JSON.parse(readFileSync(p, 'utf8'));
+    delete parsed.prDescriptionHasHan;
+    parsed.ownerRepo = 'QwenLM/qwen-code';
+    parsed.prNumber = '7686';
+    writeFileSync(p, JSON.stringify(parsed));
+    const old = new Date(2020, 0, 1);
+    utimesSync(p, old, old);
+    return p;
+  }
+
+  /** A fetcher that records its calls, so a test can prove it was NOT reached. */
+  function recordingFetcher(body: string): PrBodyFetcher & { calls: number } {
+    const fn = ((_ownerRepo: string, _prNumber: string) => {
+      fn.calls++;
+      return body;
+    }) as PrBodyFetcher & { calls: number };
+    fn.calls = 0;
+    return fn;
+  }
+
+  it('folds in Chinese when the recovered description contains Han', () => {
+    const fetch = recordingFetcher('这个 PR 懒加载首次使用的依赖。');
+    const r = composeReview({
+      suggestionsInline: 1,
+      planPath: namedPlanWithoutFlag(),
+      prBodyFetcher: fetch,
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(fetch.calls).toBe(1);
+    // Both halves: the English rides above the fold, the Chinese inside it.
+    expect(r.body).toContain('<details>\n<summary>中文说明</summary>');
+    expect(r.body).toContain('Suggestions are inline.');
+    expect(r.body).toContain('建议见行内评论。');
+  });
+
+  it('stays English when the recovered description has no Han', () => {
+    const fetch = recordingFetcher(
+      'This PR lazy-loads first-use dependencies.',
+    );
+    const r = composeReview({
+      suggestionsInline: 1,
+      planPath: namedPlanWithoutFlag(),
+      prBodyFetcher: fetch,
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(fetch.calls).toBe(1);
+    expect(r.body).not.toContain('<details>');
+    expect(r.body).not.toContain('中文');
+  });
+
+  it('honours a recorded false without fetching — the English author is settled', () => {
+    // A real fetch-pr report that fetched the body and found no Han. Re-reading
+    // the live PR on every English review would be waste, and the recorded
+    // snapshot is the answer.
+    const p = coveredPlan();
+    const parsed = JSON.parse(readFileSync(p, 'utf8'));
+    parsed.prDescriptionHasHan = false;
+    parsed.ownerRepo = 'QwenLM/qwen-code';
+    parsed.prNumber = '7686';
+    writeFileSync(p, JSON.stringify(parsed));
+    const old = new Date(2020, 0, 1);
+    utimesSync(p, old, old);
+    const fetch = recordingFetcher('这段中文绝不该被读到。');
+    const r = composeReview({
+      suggestionsInline: 1,
+      planPath: p,
+      prBodyFetcher: fetch,
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(fetch.calls).toBe(0);
+    expect(r.body).not.toContain('<details>');
+  });
+
+  it('does not fetch when the plan carries no PR identity', () => {
+    const fetch = recordingFetcher('这段中文绝不该被读到。');
+    const r = composeReview({
+      suggestionsInline: 1,
+      planPath: coveredPlan(), // no ownerRepo/prNumber, no flag
+      prBodyFetcher: fetch,
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(fetch.calls).toBe(0);
+    expect(r.body).not.toContain('<details>');
+  });
+
+  it('falls back to English when the fetch throws — language never takes the review down', () => {
+    const boom: PrBodyFetcher = () => {
+      throw new Error('gh unreachable');
+    };
+    const r = composeReview({
+      suggestionsInline: 1,
+      planPath: namedPlanWithoutFlag(),
+      prBodyFetcher: boom,
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(r.event).toBe('COMMENT');
+    expect(r.body).not.toContain('<details>');
+    expect(r.body).not.toContain('中文');
+    expect(r.body).toContain('Suggestions are inline.');
+  });
+});

packages/cli/src/commands/review/compose-review.ts

@@ -30,6 +30,7 @@ import {
   TranscriptsUnavailableError,
 } from './lib/coverage.js';
 import { shellQuotePath } from './lib/shell-quote.js';
+import { gh } from './lib/gh.js';
 import {
   CRITICAL_PREFIX,
   SUGGESTION_PREFIX,
@@ -40,6 +41,13 @@ import {
 
 export type ReviewEvent = 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT';
 
+/**
+ * Reads a PR's description body, given its `owner/repo` and number. The one
+ * production implementation calls `gh pr view`; the bilingual fallback uses it
+ * to recover the Han signal from the live PR when the plan does not carry it.
+ */
+export type PrBodyFetcher = (ownerRepo: string, prNumber: string) => string;
+
 export interface ComposeReviewInput {
   /**
    * Critical findings anchored as inline `comments` entries.
@@ -95,6 +103,16 @@ export interface ComposeReviewInput {
    * anything that would change where the transcripts are found on a real run.
    */
   env?: NodeJS.ProcessEnv;
+  /**
+   * How the bilingual fallback reads the live PR body when the plan carries a
+   * PR identity but no `prDescriptionHasHan` (a `plan-diff` plan, or one an
+   * improvising orchestrator wired in place of `fetch-pr`'s report). A test
+   * seam ONLY: production leaves it undefined and the CLI reads the PR with
+   * `gh pr view`; a model cannot supply it, being a function that does not
+   * serialise into the input JSON — so it can neither force nor suppress the
+   * Chinese fold, which is the whole point of keeping the signal the CLI's own.
+   */
+  prBodyFetcher?: PrBodyFetcher;
   /** Step 1's lightweight `pr-context` fetch failed. */
   contextUnavailable?: boolean;
   presubmit?: {
@@ -589,11 +607,13 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult {
   // description contains Han characters, the posted body carries the complete
   // Chinese version collapsed under the English one — the shape this repo's
   // own PR descriptions use, decided by the plan the CLI wrote, never by the
-  // caller. Fragments with no deterministic translation (model-written
-  // findings, caller echoes, error interpolations) ride verbatim in both
-  // halves. The footer stays outside the fold, once. A `zh === en` body has
-  // nothing translated, so no empty fold is published.
-  const bilingual = bilingualFromPlan(input.planPath);
+  // caller. When the plan does not record the signal but still names the PR,
+  // the switch recovers it from the live description (see `bilingualFromPlan`).
+  // Fragments with no deterministic translation (model-written findings, caller
+  // echoes, error interpolations) ride verbatim in both halves. The footer
+  // stays outside the fold, once. A `zh === en` body has nothing translated, so
+  // no empty fold is published.
+  const bilingual = bilingualFromPlan(input.planPath, input.prBodyFetcher);
   const render = (parts: Bi[], sep: string): string => {
     const en = parts.map((p) => p.en).join(sep);
     if (en === '') return '';
@@ -1041,21 +1061,78 @@ interface Bi {
   zh: string;
 }
 
+/** A positive PR number, as `fetch-pr`/`plan-diff` write it (a string) or a
+ *  raw integer. `null`, `0`, `''` and non-numeric junk are 'no PR'. */
+function positivePrNumber(value: unknown): string | undefined {
+  if (typeof value === 'number')
+    return Number.isInteger(value) && value > 0 ? String(value) : undefined;
+  if (typeof value === 'string' && /^\d+$/.test(value) && Number(value) > 0)
+    return value;
+  return undefined;
+}
+
+/** The production reader: one `gh pr view` for the description body. */
+const fetchPrBodyViaGh: PrBodyFetcher = (ownerRepo, prNumber) => {
+  const json = gh(
+    'pr',
+    'view',
+    prNumber,
+    '--repo',
+    ownerRepo,
+    '--json',
+    'body',
+  );
+  return (JSON.parse(json) as { body?: string }).body ?? '';
+};
+
 /**
  * Whether the posted body carries the collapsed Chinese version: the plan
  * (fetch-pr's report) recorded Han characters in the PR description. The
  * signal is the CLI's own — never the caller's, who could otherwise toggle
  * the register of a certified body. A local plan has no such field, and a
  * plan that cannot be read defaults to English-only: the language must never
  * take the review down.
+ *
+ * A recorded `false` is authoritative: `fetch-pr` fetched the body and found
+ * no Han, so English-only is the answer and no network is spent — every
+ * English-authored PR review takes this path.
+ *
+ * The field being *absent* is a different state, and the one that shipped an
+ * English-only review over a Chinese-authored PR (#7686): `fetch-pr` always
+ * writes it, but a `plan-diff` plan never does, and an orchestrator that
+ * improvises the pipeline can wire `compose-review` at a plan that is not
+ * `fetch-pr`'s report at all. So when the flag is missing yet the plan still
+ * carries the PR's identity, recover the signal from the live PR — the real
+ * description, which the caller cannot fake, so this hardens the "signal is
+ * the CLI's own" property rather than loosening it. Any failure of that fetch
+ * falls back to English: the language must never take the review down.
  */
-function bilingualFromPlan(planPath: string | undefined): boolean {
+function bilingualFromPlan(
+  planPath: string | undefined,
+  fetchPrBody: PrBodyFetcher = fetchPrBodyViaGh,
+): boolean {
   if (!planPath) return false;
+  let plan: {
+    prDescriptionHasHan?: unknown;
+    ownerRepo?: unknown;
+    prNumber?: unknown;
+  };
   try {
-    const plan = JSON.parse(readFileSync(planPath, 'utf8')) as {
-      prDescriptionHasHan?: unknown;
-    };
-    return plan?.prDescriptionHasHan === true;
+    plan = JSON.parse(readFileSync(planPath, 'utf8'));
+  } catch {
+    return false;
+  }
+  if (typeof plan?.prDescriptionHasHan === 'boolean') {
+    return plan.prDescriptionHasHan;
+  }
+  const ownerRepo =
+    typeof plan?.ownerRepo === 'string' && plan.ownerRepo
+      ? plan.ownerRepo
+      : undefined;
+  const prNumber = positivePrNumber(plan?.prNumber);
+  if (!ownerRepo || !prNumber) return false;
+  try {
+    return /\p{Script=Han}/u.test(fetchPrBody(ownerRepo, prNumber));
   } catch {
     return false;
   }