fix(ship-receipt): name the non-ancestor targeted-read commit in RECEIPT_INVALID (GST-1700) (#4106)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

bin/gstack-ship-receipt

@@ -297,7 +297,7 @@ function cmdWrite(rest: string[], json: boolean): number {
   const product = reviewedProduct(repoRoot, base, head, pregateFile);
   let targeted: ReturnType<typeof parseReviewTargeted> | undefined;
   if (flag(rest, '--review-targeted') !== undefined) {
-    try { targeted = parseReviewTargeted(jsonArg(flag(rest, '--review-targeted')), product.product_patch_id ?? 'none', read => reviewedProduct(repoRoot, base, read, pregateFile).product_patch_id, read => targetedClassesSince(repoRoot, read, headSha ?? head, base), read => plainDocsSince(repoRoot, read, headSha ?? head, base)); } catch (e: any) { targeted = { ok: false, message: `--review-targeted: ${e.message}` }; }
+    try { targeted = parseReviewTargeted(jsonArg(flag(rest, '--review-targeted')), product.product_patch_id ?? 'none', read => reviewedProduct(repoRoot, base, read, pregateFile).product_patch_id, read => targetedClassesSince(repoRoot, read, headSha ?? head, base), read => plainDocsSince(repoRoot, read, headSha ?? head, base), read => spawnSync('git', ['merge-base', '--is-ancestor', read, headSha ?? head], { cwd: repoRoot, timeout: 30_000 }).status === 0); } catch (e: any) { targeted = { ok: false, message: `--review-targeted: ${e.message}` }; }
   }
   if (targeted && !targeted.ok) { process.stderr.write(resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `${targeted.message}; nothing written`) + '\n'); return EXIT.fail; }
   const approval = approvalForReceipt({ repoRoot, base, head, citationText: [bodyText, ...flags(rest, '--approval')].join('\n'), runDirs: flags(rest, '--approval-run') });

changelog.d/capy-gst-1700-non-ancestor-targeted-read.md

@@ -0,0 +1 @@
+- **`gstack-ship-receipt write` now says why a targeted record from a rewritten commit is refused.** When the commit the targeted reader read is not an ancestor of HEAD (usually after a rebase or a redone merge resolution), the RECEIPT_INVALID message now names that commit, says it is not an ancestor of HEAD, and says the record carries only from HEAD or an ancestor of it, even when the product patch is unchanged, so the targeted reader must run on HEAD. Before, it said the product changed after the read while printing two equal product patch ids, and lanes went looking for a product change that wasn't there (qm#2952). What passes and fails is unchanged. (GST-1700)

lib/ship-receipt-depth.ts

@@ -235,9 +235,9 @@ export type ReviewTargeted = NonNullable<ShipReceipt['review_targeted']>;
  * record binds only on an equal product patch, or with `plainDocs` across a change after the read that is plain docs
  * (plainDocsSince, GST-1195), and with `classesSince` also only when nothing changed after the read carries a targeted class (a
  * test change the gate scan tags is gate class though the product patch leaves tests out), so a record never vouches for work it
- * did not read (GST-986).
+ * did not read (GST-986). `ancestor` only words the refusal: a read commit that is not HEAD's ancestor is named as such (GST-1700).
  */
-export function parseReviewTargeted(value: unknown, productPatchId: string, productPatchOf: (head: string) => string | undefined, classesSince?: (read: string) => readonly TargetedClass[] | null, plainDocs?: (read: string) => boolean): { ok: true; targeted: ReviewTargeted } | { ok: false; message: string } {
+export function parseReviewTargeted(value: unknown, productPatchId: string, productPatchOf: (head: string) => string | undefined, classesSince?: (read: string) => readonly TargetedClass[] | null, plainDocs?: (read: string) => boolean, ancestor?: (read: string) => boolean): { ok: true; targeted: ReviewTargeted } | { ok: false; message: string } {
   const bad = (message: string) => ({ ok: false as const, message });
   if (!value || typeof value !== 'object' || Array.isArray(value)) return bad('review_targeted must be { classes, rules, brief_sha, status, findings, raised_by, head }');
   const v = value as Record<string, unknown>;
@@ -249,7 +249,7 @@ export function parseReviewTargeted(value: unknown, productPatchId: string, prod
   if (typeof v.findings !== 'number' || !Number.isInteger(v.findings) || v.findings < 0) return bad('review_targeted.findings must be an integer >= 0');
   if (typeof v.head !== 'string' || !/^[0-9a-f]{40,64}$/.test(v.head)) return bad('review_targeted.head must be the commit the targeted reader read (rerun gstack-review-dispatch verify --targeted-out with this gstack)');
   const read = productPatchOf(v.head);
-  if ((read !== productPatchId && !plainDocs?.(v.head)) || (classesSince && classesSince(v.head)?.length !== 0)) return bad(`the targeted reader read ${v.head.slice(0, 12)}, whose product patch ${read?.slice(0, 12) ?? 'cannot be read'} is not this receipt's ${productPatchId.slice(0, 12)}: the product changed after the read (only release stamps and a docs change with no numeric, results, prereg, eval, money or security claim carry the record), so run the targeted reader on HEAD and verify again`);
+  if ((read !== productPatchId && !plainDocs?.(v.head)) || (classesSince && classesSince(v.head)?.length !== 0)) return bad(ancestor && !ancestor(v.head) ? `the targeted reader read ${v.head.slice(0, 12)}, which is not an ancestor of HEAD (usually after a rebase or a redone merge resolution, which rewrites the commit the reader read): a targeted record carries only from HEAD or an ancestor of it${read === productPatchId ? ', even when the product patch is unchanged' : ''}, so run the targeted reader on HEAD, rerun gstack-review-dispatch verify --targeted-out and write again` : `the targeted reader read ${v.head.slice(0, 12)}, whose product patch ${read?.slice(0, 12) ?? 'cannot be read'} is not this receipt's ${productPatchId.slice(0, 12)}: the product changed after the read (only release stamps and a docs change with no numeric, results, prereg, eval, money or security claim carry the record), so run the targeted reader on HEAD and verify again`);
   const classes = TARGETED_CLASSES.filter(c => (v.classes as string[]).includes(c));
   return { ok: true, targeted: { classes, rules: v.rules as string, brief_sha: v.brief_sha, product_patch_id: productPatchId, status: v.status as ReviewTargeted['status'], findings: v.findings, raised_by: (v.raised_by as string).trim() } };
 }

test/review-targeted-record.test.ts

@@ -376,6 +376,27 @@ describe('a main merge after the read keeps the record when the PR\'s own change
     expect(evil.err).toContain(`the targeted reader read ${head.slice(0, 12)}`);
   });
 
+  // Value: protects=a record read on a merge that was later redone (same tree, a new commit) is refused with the real cause: the
+  //   read commit is not an ancestor of HEAD, so it does not carry even with the product patch unchanged (GST-1700, qm#2952: the message
+  //   blamed a product change and the lane chased one); fails_when=the refusal does not name the read commit, its ancestry and
+  //   the recovery, or names a product change; seam=gstack-ship-receipt write --review-targeted
+  test('a read on a redone merge resolution names the non-ancestor commit and the recovery', () => {
+    const { repo, head } = moneyBranch();
+    mainCommit(repo, mainBrings, 'main: budget ledger and secrets');
+    repo.git('merge', '-q', '--no-edit', 'origin/main');
+    const first = repo.git('rev-parse', 'HEAD');
+    const record = path.join(repo.root, 'targeted.json');
+    expect(verify(repo, targetedRead(repo, repo.git('merge-base', 'origin/main', 'HEAD'), 'h1', 'NO FINDINGS'), record).status).toBe(0);
+    expect(JSON.parse(fs.readFileSync(record, 'utf8')).head).toBe(first);
+    repo.git('reset', '-q', '--hard', head);
+    repo.git('merge', '-q', '--no-edit', '-m', 'redo the main merge', 'origin/main');
+    expect(repo.git('rev-parse', 'HEAD^{tree}')).toBe(repo.git('rev-parse', `${first}^{tree}`));
+    const redone = write(repo, ['--review-targeted', `@${record}`]);
+    expect(redone.status).toBe(EXIT.fail);
+    expect(redone.err).toContain(`the targeted reader read ${first.slice(0, 12)}, which is not an ancestor of HEAD (usually after a rebase or a redone merge resolution, which rewrites the commit the reader read): a targeted record carries only from HEAD or an ancestor of it, even when the product patch is unchanged, so run the targeted reader on HEAD, rerun gstack-review-dispatch verify --targeted-out and write again; nothing written)`);
+    expect(redone.err).not.toContain('the product changed after the read');
+  });
+
   // Value: protects=with no main merge, a branch commit after the read that moves the PR's money line to another block with the
   //   same context and function header still voids the record (position-free hunks compared equal); fails_when=a path whose
   //   base content did not move is judged by its line-number-free hunks; seam=targetedClassesSince