fix(brief): address 2 Greptile P2 inline comments on PR #3751

koala73 · koala73 · commit 870ab00409ec · 2026-05-17T19:46:31.000+01:00
1. shared/brief-filter.js:115-134 — titleCase JSDoc accuracy. The function returns input unchanged when v is non-string or empty (preserving type), not always `string`. Updated `@param` to `unknown` and `@returns` to `string | unknown`. Clarified the guard-comment framing: the `|| 'General'` fallback at :384 runs BEFORE titleCase is invoked at out.push, so the type-preserving branch is never reached at the current call site — kept as defense-in-depth for future callers. 2. tests/digest-buildDigest-category-passthrough.test.mjs:55-78 — replaced brittle `indexOf('});')` with a brace-depth-tracking scanner that finds the matching close at the same depth. Old approach would silently truncate the search slice if a future refactor added a nested object literal ending in `});` inside stories.push (e.g. a `.map()` callback) — Greptile P2 concern. Also tightened the per-line assertion from substring `includes` to regex `match` so coercion or alt defensive patterns would fail the test rather than silently pass on partial match. Both nits surfaced by Greptile's review of commit 6f40264 (last reviewed commit pre-cache-prefix-bump). Test posture: 284 tests pass across the touched + neighboring surfaces.
diff --git a/shared/brief-filter.js b/shared/brief-filter.js
@@ -119,12 +119,18 @@ function clip(v, cap) {
  * AND space-bearing legacy categories that other `filterTopStories`
  * callers pass through (e.g. `composeBriefForRule` with `'world politics'
  * \u2192 'World Politics'`). First-letter-only would corrupt the multi-word
- * case (`'world politics' \u2192 'World politics'`). Empty / non-string
- * inputs are returned unchanged so the existing `|| 'General'` fallback
- * stays intact.
+ * case (`'world politics' \u2192 'World politics'`).
  *
- * @param {string} v
- * @returns {string}
+ * Defense-in-depth: non-string and empty-string inputs are returned
+ * unchanged (preserving the input type). At the only current call site
+ * (`out.push` below), `category` has already been resolved to a
+ * non-empty string via the `asTrimmedString(raw.category) || 'General'`
+ * line above, so the type-preserving branch is never reached in
+ * practice \u2014 it exists so a future caller passing `null`/`undefined`
+ * doesn't throw.
+ *
+ * @param {unknown} v
+ * @returns {string | unknown} Title-Cased string when input is non-empty string; input unchanged otherwise.
  */
 function titleCase(v) {
   if (typeof v !== 'string' || v.length === 0) return v;
diff --git a/tests/digest-buildDigest-category-passthrough.test.mjs b/tests/digest-buildDigest-category-passthrough.test.mjs
@@ -50,18 +50,43 @@ describe('U2: buildDigest carries track.category through to the envelope', () =>
 
   it('T7: the passthrough is in stories.push, not in the isOpinion/isFeelGood filter blocks', () => {
     // Find the stories.push site. The category passthrough must appear
-    // inside the object literal (between `stories.push({` and the
-    // matching `});`).
+    // by scanning forward from `stories.push({` and matching braces at
+    // the same depth to find the literal's closing `});`. Earlier this
+    // test used `indexOf('});')` which would silently truncate the
+    // search slice if a future change introduced a nested object literal
+    // ending in `});` inside stories.push (e.g. a `.map()` callback) —
+    // Greptile P2 review of PR #3751. Brace-depth tracking is robust to
+    // that without locking the test to the current flat-literal shape.
     const storiesPushIdx = buildDigestBody.indexOf('stories.push({');
     assert.ok(storiesPushIdx !== -1, 'stories.push site must exist in buildDigest');
-    // Find the matching `});` — pragmatic: search forward for the next
-    // `});` (the object literal here is shallow, no nested object).
-    const closeIdx = buildDigestBody.indexOf('});', storiesPushIdx);
-    assert.ok(closeIdx !== -1, 'stories.push must have a closing `});`');
+    let depth = 1; // we start AFTER the opening `{` of `stories.push({`
+    let closeIdx = -1;
+    const scanStart = storiesPushIdx + 'stories.push({'.length;
+    for (let i = scanStart; i < buildDigestBody.length; i += 1) {
+      const ch = buildDigestBody[i];
+      if (ch === '{') depth += 1;
+      else if (ch === '}') {
+        depth -= 1;
+        if (depth === 0) {
+          // Expect the matching `}` to be followed by `);` (the
+          // function-call close). Confirm before recording the slice end.
+          if (buildDigestBody.slice(i, i + 3) === '});') {
+            closeIdx = i + 3;
+          }
+          break;
+        }
+      }
+    }
+    assert.ok(closeIdx !== -1, 'stories.push must have a matched `});` at the same brace depth');
     const pushBlock = buildDigestBody.slice(storiesPushIdx, closeIdx);
-    assert.ok(
-      pushBlock.includes('category: typeof track.category'),
-      'category passthrough must live inside the stories.push object literal',
+
+    // Full-pattern assertion (not just substring containment): lock the
+    // exact defensive-typeof shape so a coercion or different defensive
+    // pattern would fail this test rather than passing on partial match.
+    assert.match(
+      pushBlock,
+      /category:\s*typeof\s+track\.category\s*===\s*'string'\s*\?\s*track\.category\s*:\s*''/,
+      'category passthrough must live inside the stories.push object literal with the exact defensive-typeof shape',
     );
 
     // Negative-space: the opinion / feel-good filter blocks `continue`
