From b6ea9159d82f8584947a9efb8ed473b46d08eed5 Mon Sep 17 00:00:00 2001
From: CRT <hi@crt.fyi>
Date: Wed, 21 Oct 2026 05:16:46 +0700
Subject: [PATCH] review-details: a reviewContact that is present but has empty
 fields (or no usable email) is named as such with its file, instead of 'no
 review contact'

---
 cli/lib/credentials.ts                | 11 +++++++++--
 tests/console-guardrails-1021.test.ts | 20 +++++++++++++++++++-
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/cli/lib/credentials.ts b/cli/lib/credentials.ts
index 27f9662..34c890c 100644
--- a/cli/lib/credentials.ts
+++ b/cli/lib/credentials.ts
@@ -254,11 +254,18 @@ export function findReviewContact(root: string): { contact: ReviewContact | null
     if (!existsSync(f)) { checked.push(`${f} (missing)`); continue; }
     let st: Store;
     try { st = JSON.parse(readFileSync(f, 'utf8')) as Store; } catch (e) { checked.push(`${f} (unreadable: ${(e as Error).message})`); continue; }
+    let present = '';
     for (const k of REVIEW_CONTACT_KEYS) {
-      const c = contactOf(atPath(st, k));
+      const raw = atPath(st, k);
+      const c = contactOf(raw);
       if (c) return { contact: c, from: `${f} ${k}`, checked };
+      // a key that is there but holds no usable email (empty strings copied from a template): say exactly that
+      if (!present && raw && typeof raw === 'object' && !Array.isArray(raw)) {
+        const vals = Object.values(raw as Record<string, unknown>).filter(v => typeof v === 'string') as string[];
+        present = vals.every(v => !v.trim()) ? `${k} found but all its fields are empty` : `${k} found but its email is missing or a placeholder`;
+      }
     }
-    checked.push(`${f} (keys ${REVIEW_CONTACT_KEYS.join(', ')}: none with an email)`);
+    checked.push(present ? `${f} (${present}: fill firstName, lastName, phone, email)` : `${f} (keys ${REVIEW_CONTACT_KEYS.join(', ')}: none with an email)`);
   }
   return { contact: null, from: null, checked };
 }
diff --git a/tests/console-guardrails-1021.test.ts b/tests/console-guardrails-1021.test.ts
index 1035f82..cb7651b 100644
--- a/tests/console-guardrails-1021.test.ts
+++ b/tests/console-guardrails-1021.test.ts
@@ -168,8 +168,10 @@ describe('6. the review contact', () => {
     writeFileSync(join(top, '.storebound', 'credentials.json'), JSON.stringify({ reviewContact: { firstName: 'Ada' } }));
     const m = noContactMessage(top);
     expect(m).toContain(join(top, '.storebound', 'credentials.json'));
-    expect(m).toContain('reviewContact, apple.reviewContact');
+    expect(m).toContain('reviewContact found but its email is missing');
     expect(m).toContain('--contact-from');
+    writeFileSync(join(top, '.storebound', 'credentials.json'), JSON.stringify({ other: 1 }));
+    expect(noContactMessage(top)).toContain('reviewContact, apple.reviewContact');
   });
 });
 
@@ -189,3 +191,19 @@ describe('7. one-line errors for missing required values', () => {
     expect(p.exitCode).toBe(1);
   });
 });
+
+test('a reviewContact that is there with empty fields is named as such, not "no review contact"', async () => {
+  const { mkdtempSync, mkdirSync, writeFileSync, rmSync } = await import('node:fs');
+  const { tmpdir } = await import('node:os');
+  const { join } = await import('node:path');
+  const { findReviewContact, noContactMessage } = await import('../cli/lib/credentials');
+  const d = mkdtempSync(join(tmpdir(), 'sb-contact-empty-'));
+  try {
+    mkdirSync(join(d, '.storebound'), { recursive: true });
+    mkdirSync(join(d, '.git'));
+    writeFileSync(join(d, '.storebound', 'credentials.json'), JSON.stringify({ reviewContact: { firstName: '', lastName: '', phone: '', email: '' } }));
+    const r = findReviewContact(d);
+    expect(r.contact).toBeNull();
+    expect(noContactMessage(d, r.checked)).toContain('reviewContact found but all its fields are empty');
+  } finally { rmSync(d, { recursive: true, force: true }); }
+});