refactor(review): split SKILL.md into a core body plus verdict-gated reference files

packages/cli/src/commands/review/run-skill-parity.test.ts

@@ -33,10 +33,7 @@ const repoRoot = resolve(
   '..',
 );
 
-const SKILL_PATH = join(
-  repoRoot,
-  'packages/core/src/skills/bundled/review/SKILL.md',
-);
+const SKILL_DIR = join(repoRoot, 'packages/core/src/skills/bundled/review');
 
 /** The `{target}` token, rendered per class exactly as the skill defines it. */
 const TARGETS = {
@@ -46,14 +43,31 @@ const TARGETS = {
 };
 
 describe('run pins match the bundled skill templates', () => {
-  const skill = existsSync(SKILL_PATH)
-    ? readFileSync(SKILL_PATH, 'utf8').replace(/\r\n/g, '\n')
+  // The skill is a corpus since #9787, and each oracle reads only the
+  // files the owning step is GUARANTEED to see: the composed-name template
+  // lives in the core SKILL.md (injected on every run), the report stems in
+  // references/persistence.md (the one reference loaded before Step 8 on
+  // every run that has a Step 8). Accepting a template from ANY corpus file
+  // would stay green on a move into a verdict-gated file that many runs
+  // never load, while those runs improvise artifact names.
+  const coreSkill = existsSync(join(SKILL_DIR, 'SKILL.md'))
+    ? readFileSync(join(SKILL_DIR, 'SKILL.md'), 'utf8').replace(/\r\n/g, '\n')
     : null;
+  const step8Corpus = (() => {
+    if (coreSkill === null) return null;
+    const persistence = join(SKILL_DIR, 'references', 'persistence.md');
+    return existsSync(persistence)
+      ? `${coreSkill}\n${readFileSync(persistence, 'utf8').replace(/\r\n/g, '\n')}`
+      : null;
+  })();
 
   // A sparse or partial checkout has no skill to read; the pins are still
   // covered by run.test.ts's own cases. Failing here would report a checkout
   // shape as a contract drift.
-  const itWithSkill = skill === null ? it.skip : it;
+  const itWithSkill = coreSkill === null ? it.skip : it;
+  // The stems oracle additionally reads references/persistence.md, so a
+  // checkout that has SKILL.md but not that file must skip it too.
+  const itWithStep8 = step8Corpus === null ? it.skip : it;
 
   itWithSkill('composedNameFor renders Step 6’s --out template', () => {
     // The template as the skill writes it, e.g.
@@ -66,7 +80,7 @@ describe('run pins match the bundled skill templates', () => {
     // class, so no future suffix character has to be foreseen.
     const m =
       /--out\s+\.qwen\/tmp\/(qwen-review-\{target\}-composed\.json)(?=\s|$)/.exec(
-        skill as string,
+        coreSkill as string,
       );
     // A null here means SKILL.md no longer writes that `--out` line: update
     // composedNameFor and this oracle together to the new template.
@@ -78,11 +92,11 @@ describe('run pins match the bundled skill templates', () => {
     }
   });
 
-  itWithSkill('reportPatternFor accepts Step 8’s report stems', () => {
+  itWithStep8('reportPatternFor accepts Step 8’s report stems', () => {
     // The stems as the skill lists them, e.g.
     //   `.qwen/reviews/<YYYY-MM-DD>-<HHMMSS>-pr-<number>.md`
     const stems = [
-      ...(skill as string).matchAll(
+      ...(step8Corpus as string).matchAll(
         /`\.qwen\/reviews\/<YYYY-MM-DD>-<HHMMSS>-([^`]+)\.md`/g,
       ),
     ].map((s) => s[1]);

packages/core/src/skills/bundled-skills.integration.test.ts

@@ -63,4 +63,15 @@ describe('bundled SKILL.md files', () => {
       fs.existsSync(path.join(datavizDir, 'references', 'anti-patterns.md')),
     ).toBe(true);
   });
+
+  it('ships the review verdict-gated reference files with the bundled skill', () => {
+    const reviewDir = path.join(bundledDir, 'review');
+
+    expect(fs.existsSync(path.join(reviewDir, 'SKILL.md'))).toBe(true);
+    for (const name of ['posting.md', 'persistence.md', 'aone.md']) {
+      expect(fs.existsSync(path.join(reviewDir, 'references', name))).toBe(
+        true,
+      );
+    }
+  });
 });

packages/core/src/skills/bundled/review/SKILL.test.ts

@@ -20,10 +20,24 @@ const skillDir = path.dirname(fileURLToPath(import.meta.url));
 const POINTER_RE = /\(measured; DESIGN\.md — ([^()\n]+(?:\([^()\n]*\))?)\)/g;
 const POINTER_OPEN = '(measured; DESIGN.md — ';
 
-function skillBody(): string {
+// The verdict-gated reference files (#9787): Step 7, Step 8 and the Aone
+// paths live beside the core body and are read on demand. The split moved
+// whole sections verbatim, so every revert guard below governs the full
+// corpus, whichever file the guarded text now lives in.
+const REFERENCE_FILES = ['posting.md', 'persistence.md', 'aone.md'];
+
+function coreBody(): string {
   return fs.readFileSync(path.join(skillDir, 'SKILL.md'), 'utf8');
 }
 
+function referenceBody(name: string): string {
+  return fs.readFileSync(path.join(skillDir, 'references', name), 'utf8');
+}
+
+function skillBody(): string {
+  return [coreBody(), ...REFERENCE_FILES.map(referenceBody)].join('\n');
+}
+
 function incidentPointers(body: string): string[] {
   return [...body.matchAll(POINTER_RE)].map(([, title]) => title.trim());
 }
@@ -609,6 +623,42 @@ describe('bundled review skill', () => {
     );
   });
 
+  it('keeps the presubmit example on the host rule', () => {
+    // Revert guard: presubmit was the one Step 7 subcommand example
+    // missing the host flag; on an auth-config-only GHE clone a dropped
+    // `--host` routes its platform queries at github.com — the same
+    // failure class the meta pins above guard.
+    const body = skillBody();
+    expect(body).toContain(
+      '[--new-findings .qwen/tmp/qwen-review-{target}-new-findings.json] \\\n  [--host <host>]',
+    );
+  });
+
+  it('pins the publish-assets weave as the last, all-or-nothing step', () => {
+    // Revert guard: `--findings-out` is written only after the push and
+    // the manifest succeed; without the clause the artifact's failure
+    // contract is unstated, and a mid-publish failure reads as a partial
+    // weave or a reason not to re-run.
+    const body = skillBody();
+    expect(body).toContain(
+      'the `--findings-out` rewrite runs only after every file has landed and the manifest is written',
+    );
+    expect(body).toContain(
+      'a run that fails partway through the push is completed by an idempotent re-run',
+    );
+  });
+
+  it('names the deferral channel in the bodyCriticals sources', () => {
+    // Revert guard: compose-review relocates a `Critical` entry written
+    // into `deferredSuggestions` into the body Criticals (a Critical is
+    // never deferred); the bodyCriticals bullet must name that mechanical
+    // relocation beside the two model-written sources.
+    const body = skillBody();
+    expect(body).toContain(
+      'a `Critical` entry placed in `deferredSuggestions` is relocated here, never deferred',
+    );
+  });
+
   it('keeps the lightweight capture on fetch-diff with the plan-diff host note', () => {
     // Revert guard: restoring a prose `gh pr diff > file` here (or dropping
     // the plan-diff --host note) must fail a test, not slip through — the
@@ -845,6 +895,17 @@ describe('bundled review skill', () => {
     expect(body).not.toContain(
       '`pr-context` and `comment-status` have no Aone backing',
     );
+    // The last three skip residues this change removes — the setup-batch
+    // parenthetical, the comment-status guard clause, and the Step 6
+    // no-report clause. The positive assertions above stay green if a
+    // merge resolution or partial revert re-adds any of them, while Aone
+    // runs skip comment-status again; the replacement contract is the
+    // a1-backed report's existence in Step 6's re-check.
+    expect(body).not.toContain('drops out of the batch');
+    expect(body).not.toContain('leaving a two-call batch');
+    expect(body).not.toContain('the command has no backing');
+    expect(body).not.toContain('skips the command with the Step 1 batch');
+    expect(body).toContain('on an Aone target it runs a1-backed');
   });
 
   it('keeps the corrected Aone --comment contract, not merge residue', () => {
@@ -951,4 +1012,104 @@ describe('bundled review skill', () => {
     );
     expect(new Set(advertised)).toEqual(new Set(declared));
   });
+
+  it('ships the verdict-gated reference files beside the core body', () => {
+    // The split (#9787) moves whole steps, not rules: the core keeps the
+    // gates and the invariants that bind runs which never load a file, and
+    // each reference owns one conditional territory.
+    for (const name of REFERENCE_FILES) {
+      expect(referenceBody(name).length).toBeGreaterThan(1000);
+    }
+    expect(referenceBody('posting.md')).toContain('# Step 7: Submit PR review');
+    expect(referenceBody('persistence.md')).toContain(
+      '# Step 8: Save review report and cache',
+    );
+    expect(referenceBody('aone.md')).toContain('# Aone Code paths');
+  });
+
+  it('gates every reference file on the verdict in the core body', () => {
+    // A run must learn from the injected core alone WHICH file to read and
+    // when; a gate that moved into the file it gates would be unreadable.
+    const core = coreBody();
+    expect(core).toContain('**Reference files, gated by this verdict.**');
+    // Pin each enumeration prefix together with its load-condition clause
+    // as ONE contiguous substring: checked separately, a rewrite that swaps
+    // two clauses between bullets ships green while a report-only run loads
+    // the wrong file. The gating is the mechanism this split introduces.
+    expect(core).toContain(
+      '`references/posting.md` — Step 7 (authorisation, anchors, presubmit, `submit`, the 422/head-drift recovery, `publish-assets`). Load it when, and only when, posting is live',
+    );
+    expect(core).toContain(
+      '`references/persistence.md` — Step 8 (report, artifact registration, incremental cache). Load it before Step 8 on every run except cross-repo lightweight mode',
+    );
+    expect(core).toContain(
+      '`references/aone.md` — the Aone paths (see the Aone note below). Load it before `match-remote` when the target is Aone',
+    );
+  });
+
+  it('keeps the write prohibition and the posting gates in the core body', () => {
+    // The one-sentence write ban and the PR-only/high-only posting rule must
+    // bind a run that never loads posting.md — the bypass they guard against
+    // does not wait for the gate file.
+    const core = coreBody();
+    expect(core).toContain(
+      '`qwen review submit` is the only write path in this skill',
+    );
+    expect(core).toContain('Posting is a PR-only, high-only action');
+    // The step headings stay in core so every "Step 7" / "Step 8" cross-
+    // reference in the corpus resolves to the pointer that forwards.
+    expect(core).toContain('## Step 7: Submit PR review');
+    expect(core).toContain('## Step 8: Save review report and cache');
+    // The compose-state field list relocated to Step 6 references the
+    // never-in-body rule whose full text moved to posting.md; the entry must
+    // restate the rule's substance so a report-only run (which never loads
+    // posting.md) still sees why a Suggestion must not ride the review body.
+    expect(core).toContain('does not filter review bodies');
+  });
+
+  it('moved the sections whole — no step body duplicated across files', () => {
+    const core = coreBody();
+    const corpus = skillBody();
+    // Distinctive openings of the moved sections: present in exactly one
+    // file of the corpus, and absent from the core. The corpus-wide count
+    // alone would pass a revert that keeps a section in the core, and the
+    // absence-from-core alone passes a copy duplicated BETWEEN the
+    // reference files — an Aone --comment run loads both posting.md and
+    // aone.md, so one run would then obey two potentially divergent
+    // copies of the same step.
+    expect(corpus.match(/\*\*Use the "Create Review" API/g)).toHaveLength(1);
+    expect(corpus.match(/### Report persistence/g)).toHaveLength(1);
+    expect(
+      corpus.match(/run `\/review` \*\*from inside a clone of that repo\*\*/g),
+    ).toHaveLength(1);
+    expect(core).not.toContain(
+      '**Use the "Create Review" API to submit verdict + inline comments',
+    );
+    expect(core).not.toContain('### Report persistence');
+    expect(core).not.toContain(
+      'run `/review` **from inside a clone of that repo**',
+    );
+    // The compose-state field list relocated from Step 7 to Step 6's Verdict
+    // section: one copy in the corpus, in the core.
+    expect(corpus.match(/- `modelId` — for the footer\./g)).toHaveLength(1);
+    expect(core).toContain('- `modelId` — for the footer.');
+  });
+
+  it('keeps template tokens out of the raw-loaded reference files', () => {
+    // BundledSkillLoader interpolates only the core body it injects; the
+    // reference files are read raw via read_file, so a token there reaches
+    // the run unreplaced: a literal `(v{{cliVersion}})` draft footer is
+    // one stripReviewFooter cannot match (the version span excludes
+    // braces), so every posted comment carries the broken token above the
+    // canonical footer, and a `{{model}}` copied into the cache JSON
+    // fails the next round's same-model anchor gate.
+    for (const name of REFERENCE_FILES) {
+      expect(referenceBody(name)).not.toMatch(/\{\{[^}]+\}\}/);
+    }
+    // The reference files' footer templates name YOUR_MODEL_ID, whose
+    // value the loader prepends to the injected core body — but only when
+    // the core body carries a model token; without one the declaration
+    // vanishes and the templates dangle.
+    expect(/{{model}}|YOUR_MODEL_ID/.test(coreBody())).toBe(true);
+  });
 });