fix(brief): persist category on story:track:v1 so threads card stops showing 8/8 General#3751
Merged
Conversation
…nup (U1) May 17 brief shipped 8/8 threads cards tagged 'General' — the upstream EventCategory classifier in parseRssXml has been computing meaningful values (conflict|health|diplomatic|…) all along, but buildStoryTrackHsetFields was discarding the field before the row hit Redis. By the time the composer read story:track:v1 back, filterTopStories' `|| 'General'` fallback was the only thing left. Adds 'category' as a sibling HSET write, defensive on missing/non-string upstream values (empty string falls through filterTopStories' 'General' default, graceful degradation for pre-PR rows). cache-keys.ts cleanup in the same change-block: - Removes dead export STORY_TRACKING_TTL_S = 172800 (zero call sites) - Corrects stale "TTL for all: 48h" docs — actual TTLs are split: STORY_TTL = 7d for the story:track row, DIGEST_ACCUMULATOR_TTL = 48h for the accumulator. The 48h-everywhere doc was leftover from before the split and caused the round-1 ce-doc-review TTL misreading (corrected in the plan; codifying here too). - Adds 'category' (and the recent isOpinion/isFeelGood) to the documented HSET-fields list. See docs/plans/2026-05-17-002-fix-persist-story-track-category-plan.md for the full design (5 rounds of codex review + post-5 fixes). 13 tests pass; biome clean (pre-existing warning on unrelated line).
Sibling to U1: stories.push now carries `category: typeof track.category === 'string' ? track.category : ''`, matching the defensive shape of the existing `description` read at the same site. The envelope reaches filterTopStories with the meaningful category value (no more 'General' fallback in steady state); pre-PR residue rows missing the field gracefully degrade to 'General' via filterTopStories' existing `|| 'General'` fallback. Also sweeps three stale doc comments in scripts/lib/brief-compose.mjs (:543, :585-586, :625-627) that documented story:track:v1 as not carrying category. Post-U2 those statements are wrong; the new comments describe the actual flow (carried + word-wise Title-Cased once in shared/brief-filter.js at envelope build). 2 new source-textual tests in greenfield tests/digest-buildDigest-category-passthrough.test.mjs (T6 wiring shape, T7 wiring location — must be in stories.push, not in the opinion or feel-good filter blocks).
…ries (U3)
Normalize once at the envelope-build site (filterTopStories' out.push)
so all three downstream consumers — threads card (brief-compose.mjs:812),
magazine story-page (brief-render.js:653), public-thread fallback
(brief-render.js:1296) — see the same Title-Case form. Fixing only the
threads card (the original plan's first instinct) would have shipped
case-inconsistency: 'Conflict' in threads, 'conflict' on story pages.
Word-wise (`\b[a-z]` regex), not first-letter-only:
- filterTopStories is shared with composeBriefForRule callers that
pass multi-word legacy categories like 'world politics' (see :294-300
comment). First-letter-only would corrupt those to 'World politics'.
- Word-wise handles both single-word EventCategory enum values
('conflict' → 'Conflict') AND multi-word legacy ('world politics' →
'World Politics'). Idempotent on already-Capitalized inputs since
`\b[a-z]` doesn't match uppercase.
Cap-key (pairKey) stays on RAW `category` (computed before titleCase
fires at out.push). T11 + T11b lock this structural ordering:
- T11 (behavioral): mixed-case fixtures 'conflict' + 'Conflict' with
cap=1 → both survive (different raw keys), both display as 'Conflict'.
Inverse outcome from a hypothetical buggy cap-on-display.
- T11b (source-textual): asserts `pairKey` computation appears BEFORE
`titleCase(category)` in source order. Belt-and-suspenders.
10 new test cases in tests/brief-filter.test.mjs:
T9 (lowercase → Title), T10 (all 13 EventCategory enum values
parameterized), T10b (multi-word preservation), T11 + T11b (cap-key
ordering), T12 (General fallback idempotent), T13 (defensive empty /
non-string).
384 tests pass across the touched surfaces (brief-filter + brief-from-digest
+ brief-llm + brief-composer + persistence + buildDigest tests).
Pre-existing biome complexity warning on filterTopStories unchanged.
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR fixes the root cause of all 8 threads-card entries displaying "General" by persisting the
Confidence Score: 4/5Safe to merge. The changes are narrow plumbing additions with no schema migrations, no new async paths, and defensive The three-layer fix is mechanical and mirrors the isFeelGood PR exactly. All changed call sites are covered by T1–T4, T6–T7, and T9–T13, with a 384-test sweep reported clean. The only comments are a minor JSDoc inaccuracy in
Important Files Changed
Sequence DiagramsequenceDiagram
participant RSS as parseRssXml
participant HSET as buildStoryTrackHsetFields
participant Redis as Redis story:track:v1
participant BD as buildDigest
participant FTS as filterTopStories
participant Comp as brief-compose / brief-render
RSS->>HSET: item.category (classifyByKeyword / enrichWithAiCache)
Note over HSET: NEW: 'category', typeof item.category === 'string' ? item.category : ''
HSET->>Redis: HSET … category 'conflict'
Redis->>BD: "HGETALL → track.category = 'conflict'"
Note over BD: NEW: category: typeof track.category === 'string' ? track.category : ''
BD->>FTS: "stories[].category = 'conflict' ('' for pre-stamp residue)"
Note over FTS: asTrimmedString(raw.category) || 'General'<br/>pairKey = source + KEY_DELIM + category (cap on raw)<br/>NEW: displayCategory = titleCase(category)
FTS->>Comp: "envelope story.category = 'Conflict'"
Note over Comp: threads card + story pages +<br/>public-thread stubs all see same Title-Case value
Reviews (1): Last reviewed commit: "feat(brief): word-wise titleCase on enve..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+126
to
+130
| * @param {string} v | ||
| * @returns {string} | ||
| */ | ||
| function titleCase(v) { | ||
| if (typeof v !== 'string' || v.length === 0) return v; |
Contributor
There was a problem hiding this comment.
@returns {string} JSDoc tag is inaccurate: when v is not a string or is empty, the function returns v as-is (preserving the original type). In practice the call site in filterTopStories always passes a non-empty string (after || 'General' has already fired), so this never matters at runtime — but the doc and the guard comment ("so the existing || 'General' fallback stays intact") slightly misframe the interaction since || 'General' fires before titleCase is invoked, not after.
Suggested change
| * @param {string} v | |
| * @returns {string} | |
| */ | |
| function titleCase(v) { | |
| if (typeof v !== 'string' || v.length === 0) return v; | |
| * @param {string} v | |
| * @returns {string} the input with every word's first letter uppercased; | |
| * non-string or empty inputs are returned unchanged (the `|| 'General'` | |
| * fallback in `filterTopStories` already ensures a non-empty string | |
| * reaches this helper at the normal call site). | |
| */ | |
| function titleCase(v) { | |
| if (typeof v !== 'string' || v.length === 0) return v; |
Owner
Author
There was a problem hiding this comment.
Fixed in 870ab00: @param → unknown, @returns → string | unknown to reflect the type-preserving non-string/empty branch. Reworded the guard comment to clarify that asTrimmedString(raw.category) || 'General' (line :384) fires BEFORE titleCase is invoked at out.push, so the type-preserving branch is never reached in practice — kept as defense-in-depth for future callers.
Comment on lines
+59
to
+61
| const closeIdx = buildDigestBody.indexOf('});', storiesPushIdx); | ||
| assert.ok(closeIdx !== -1, 'stories.push must have a closing `});`'); | ||
| const pushBlock = buildDigestBody.slice(storiesPushIdx, closeIdx); |
Contributor
There was a problem hiding this comment.
stories.push block delimiter is fragile
indexOf('});') finds the first }); after stories.push({, which coincides with the real closing delimiter only because the current object literal is flat. If a future change adds a nested call whose result ends in }); inside stories.push (e.g. a .map() or .filter() callback), pushBlock will be truncated and the category inclusion check will silently pass on a wrong slice. Consider asserting the full pattern category: typeof track.category against buildDigestBody with a positional constraint against storiesPushIdx instead.
Owner
Author
There was a problem hiding this comment.
Fixed in 870ab00: replaced indexOf('});') with a brace-depth-tracking scanner that finds the matching close at the same depth. Future nested-object refactors of stories.push (.map() callbacks etc.) will no longer truncate the search slice. Also tightened the per-line assertion from includes substring match to assert.match regex — locks the exact category: typeof track.category === 'string' ? track.category : '' defensive shape, so coercion or alt defensive patterns would fail rather than silently pass.
…ings Three substantive findings from the 8-reviewer ce-code-review on PR #3751 that survived 5 rounds of codex review: 1. (P2) LLM cache orphan cascade: brief:llm:digest:v6, brief:llm:whymatters:v4, and brief:llm:description:v2 all fold `s.category` into their cache-key hash via hashBriefStory / hashDigestInput. Pre-PR every story carried 'General'; post-PR carries per-story Title-Cased EventCategory. Every cached row was about to silently stale with no prefix bump. Bumped all three: digest v6→v7, whymatters v4→v5, description v2→v3. 2. (P2) Cap relaxation during pre-stamp residue rollout: residue rows resolve to 'General' (capital, from the `|| 'General'` fallback in filterTopStories) while fresh post-PR rows carry lowercase 'general' (from classifyByKeyword's no-match fallback). The cap-key `'Reuters\x1fGeneral'` vs `'Reuters\x1fgeneral'` would have produced different buckets → cap silently bypassed for the residue subset for up to 7d STORY_TTL. That's exactly the editorial-clutter failure mode PR #3697 was created to prevent. Fix: case-fold the cap-key with `category.toLowerCase()` — titleCase at out.push stays unchanged. Updated T11 to reflect the new semantics + added T11c critical- regression locking the residue/fresh-general cap-grouping behavior. 3. (P2) Missing end-to-end integration test: added e2e assertion in tests/brief-from-digest-stories.test.mjs covering digestStory(category) → composeBriefFromDigestStories → envelope.category Title-Cased. The 3-step inferential chain (U1 lowercase write → U2 source-textual wiring → U3 titleCase at out.push) now has a single behavioral lock. Also: trivial correctness fix — stale `shared/brief-filter.js:365` reference in the HSET write comment updated to `:384` (the asTrimmedString fallback line shifted when U3 added the titleCase helper). 386 tests pass.
This was referenced May 17, 2026
Owner
Author
/ce-code-review trailRan 8-reviewer review (correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, adversarial, kieran-typescript) at `8dc087bb1..6f40264`. Adversarial caught 3 substantive findings that survived 5 rounds of pre-merge Codex review. Applied in `e99864465`:
Plus 1 trivial silent fix (stale `shared/brief-filter.js:365` → `:384` line reference in HSET comment). Deferred as follow-up issues:
Skipped:
Test posture post-fix:386 tests pass across full touched surface (brief-filter + brief-from-digest-stories + brief-llm + brief-composer + persistence + buildDigest + neighbours). Pre-existing biome warnings unchanged. Coverage notes:
🤖 Generated by `/ce-code-review` |
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.
This was referenced May 17, 2026
koala73
added a commit
that referenced
this pull request
May 18, 2026
* fix(brief): forbid weak temporal stitching in digest lead
May 17 brief shipped a lead that stapled the WHO Ebola declaration
and the Israel/Hezbollah escalation into a single sentence with
"This declaration comes as…" — two unrelated top stories joined by
an editorial connective, with no shared actor, causal link, or
geographic theatre.
DIGEST_PROSE_SYSTEM_BASE now:
- Tells the model to lead with ONE primary story and only reference
a second when there is a substantive link (shared actor, causal
connection, direct policy consequence, same geographic theatre).
- Carries a dedicated BANNED stitching-phrases section enumerating
the common offenders ("this comes as", "meanwhile", "in other
news", "elsewhere", etc.).
Cache prefix bumped v7 → v8 (brief-llm.mjs:23 comment, :962 key)
to invalidate any v7 rows that were generated against the stitching-
tolerant prompt — without the bump, stale stitched leads would
serve for the full 4h TTL after deploy.
Tests:
- New `forbids weak stitching connectives in the lead` test asserts
the prompt contains the anti-stitching instruction, the BANNED
section, the enumerated phrases, and the substantive-link
allowlist.
- Bumped cache-prefix assertion v7 → v8 with an added negative
assertion that no v7 rows are written.
Pattern reference: PR #3751 followed the same prompt-change → cache-
prefix-bump → assertion-update pattern when category persistence
landed (v6 → v7).
* test(brief): tighten anti-stitching assertion (P3 review fix)
PR #3769 review noted the regression test only checked 4 of the 10
banned phrases and scanned the entire system prompt — so a phrase
removed from the dedicated BANNED section could still pass on a
duplicate mention in the lead-field instructions.
Fix:
- Extract the BANNED stitching phrases section as a bounded substring
(from the section header up to the next `Threads:` instruction
bullet).
- Assert all 10 banned phrases appear inside that substring, not
the broader system prompt. A regression that removes a phrase
from the section now fires even if the lead instruction still
mentions it.
102/102 brief-llm tests pass.
koala73
added a commit
that referenced
this pull request
May 18, 2026
…gence (#3752) (#3759) * docs(brief): document synthesis-prompt vs display category-case divergence (#3752) Issue #3752, surfaced by /ce-code-review adversarial finding A-004 on PR #3751: `digestStoryToSynthesisShape` feeds the LLM synthesis prompt with the canonical lowercase EventCategory enum value (`'conflict'`, `'health'`, …), while the envelope path goes through filterTopStories' out.push which Title-Cases for display. Both paths read from the same upstream `s.category`; the divergence is downstream and intentional but was previously undocumented. Option (3) from the issue triage — documentation-only, no code change. Rationale: synthesis prompts benefit from the canonical lowercase enum as semantic anchor (LLM training distribution sees category labels as bare nouns more often than Title-Cased headings); display surfaces benefit from human-readable Title-Case. Two annotation sites added, cross-referencing each other: - scripts/lib/brief-compose.mjs:digestStoryToSynthesisShape JSDoc + inline at the `category:` field write - shared/brief-filter.js:filterTopStories out.push block comment Both note "if you change the case behavior at one site, audit the other" so a future refactor that consolidates the two paths is deliberate rather than accidental. Options (1)/(2) — apply titleCase at the synthesis path or upstream at buildDigest — would have changed LLM prompt content for every digest, every user (non-deterministic prompt drift) AND required a cache-prefix bump (digest:v7→v8) only one day after the v6→v7 bump that PR #3751's review-driven fixes already shipped. Doc-only resolves the audit-trail concern without operational noise. 269 tests pass; biome clean on touched files (pre-existing complexity warning on filterTopStories unchanged). Closes #3752. * docs(brief): clarify both category fallback layers in digestStoryToSynthesisShape (#3759 review) PR-review finding on #3759: the JSDoc said pre-stamp residue rows gracefully degrade via filterTopStories' fallback "elsewhere" — but this function does NOT go through filterTopStories. It has its own local guard at the category: field write (`typeof s?.category === 'string' ? s.category : 'General'`). The original wording made the local guard look redundant when it's actually load-bearing on the synthesis-prompt path. Updated the JSDoc to name BOTH fallback layers explicitly: 1. Local guard here (synthesis-prompt path) 2. filterTopStories' asTrimmedString || 'General' (envelope/display path) + explicit note that removing either guard leaves the corresponding path exposed to residue rows on deploy. Doc-only; no behavior change. No new tests needed.
koala73
added a commit
that referenced
this pull request
Jul 6, 2026
…he bump-history comment (#4967 review P3) The test claimed #4944 U4 "bumped v2->v3 alongside category persistence" (that was PR #3751) and never seeded a v3 row — so it didn't directly prove THIS PR's v3->v4 eviction. Now seeds v1+v2+v3 stale rows with the correct per-era history and asserts none is served while the fresh row lands under v4. Claude-Session: https://claude.ai/code/session_01Xs7LsyQQJqngcqjt5oGcpx
koala73
added a commit
that referenced
this pull request
Jul 6, 2026
…he bump-history comment (#4967 review P3) The test claimed #4944 U4 "bumped v2->v3 alongside category persistence" (that was PR #3751) and never seeded a v3 row — so it didn't directly prove THIS PR's v3->v4 eviction. Now seeds v1+v2+v3 stale rows with the correct per-era history and asserts none is served while the fresh row lands under v4. Claude-Session: https://claude.ai/code/session_01Xs7LsyQQJqngcqjt5oGcpx
koala73
added a commit
that referenced
this pull request
Jul 6, 2026
…he bump-history comment (#4967 review P3) The test claimed #4944 U4 "bumped v2->v3 alongside category persistence" (that was PR #3751) and never seeded a v3 row — so it didn't directly prove THIS PR's v3->v4 eviction. Now seeds v1+v2+v3 stale rows with the correct per-era history and asserts none is served while the fresh row lands under v4. Claude-Session: https://claude.ai/code/session_01Xs7LsyQQJqngcqjt5oGcpx
koala73
added a commit
that referenced
this pull request
Jul 6, 2026
…he bump-history comment (#4967 review P3) The test claimed #4944 U4 "bumped v2->v3 alongside category persistence" (that was PR #3751) and never seeded a v3 row — so it didn't directly prove THIS PR's v3->v4 eviction. Now seeds v1+v2+v3 stale rows with the correct per-era history and asserts none is served while the fresh row lands under v4. Claude-Session: https://claude.ai/code/session_01Xs7LsyQQJqngcqjt5oGcpx
koala73
added a commit
that referenced
this pull request
Jul 6, 2026
…he bump-history comment (#4967 review P3) The test claimed #4944 U4 "bumped v2->v3 alongside category persistence" (that was PR #3751) and never seeded a v3 row — so it didn't directly prove THIS PR's v3->v4 eviction. Now seeds v1+v2+v3 stale rows with the correct per-era history and asserts none is served while the fresh row lands under v4. Claude-Session: https://claude.ai/code/session_01Xs7LsyQQJqngcqjt5oGcpx
koala73
added a commit
that referenced
this pull request
Jul 6, 2026
…he bump-history comment (#4967 review P3) The test claimed #4944 U4 "bumped v2->v3 alongside category persistence" (that was PR #3751) and never seeded a v3 row — so it didn't directly prove THIS PR's v3->v4 eviction. Now seeds v1+v2+v3 stale rows with the correct per-era history and asserts none is served while the fresh row lands under v4. Claude-Session: https://claude.ai/code/session_01Xs7LsyQQJqngcqjt5oGcpx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the May 17 brief item #1 (all 8 threads cards tagged
General). Root cause:parseRssXml:485has been computing meaningfulEventCategoryvalues (conflict | health | diplomatic | …) all along, butbuildStoryTrackHsetFieldswas discarding the field before the row hit Redis. By the timefilterTopStoriesreadstory:track:v1back, the|| 'General'fallback was the only thing left.PR #3697 (threads-from-walk) made the latent gap visible by reading
digest.threads[].categoryfor display. This PR closes the gap PR #3697 exposed.Depends on: PR #3748 (
feelgood-classifier, merged as2a5bf8436) + PR #3750 (opinion-classifierpathname backport, merged as8dc087bb1). Both onorigin/main.Implementation Units
story:track:v1HSET + cache-keys.ts cleanupserver/worldmonitor/news/v1/list-feed-digest.ts,server/_shared/cache-keys.ts,tests/news-story-track-description-persistence.test.mtsscripts/seed-digest-notifications.mjs,scripts/lib/brief-compose.mjs,tests/digest-buildDigest-category-passthrough.test.mjsshared/brief-filter.js,tests/brief-filter.test.mjsDesign highlights (Codex 5-round review)
shared/brief-filter.js, not at any display surface.categoryis rendered by 3 consumers (brief-compose.mjs:812,brief-render.js:653,brief-render.js:1296); normalizing once at the envelope-build site guarantees case-consistency across all three.filterTopStoriesis shared withcomposeBriefForRulecallers that pass multi-word legacy categories like'world politics'. First-letter-only would corrupt those to'World politics'. Word-wise (\b[a-z]) handles both single-word EventCategory enum values AND multi-word legacy values correctly. Idempotent on already-Capitalized inputs.category. Title-casing happens only atout.push, AFTER pairKey is computed. T11 (behavioral mixed-case differential) + T11b (source-textual ordering) lock this structural invariant.classifyByKeyword(title, variant)IS feasible (_classifier.ts:346), but ingest-timecategorycan be AI-adjusted viaenrichWithAiCache(list-feed-digest.ts:678,725). A read-time fallback would silently downgrade AI-adjusted verdicts back to keyword-only. Persistence captures the AI verdict; the rollout-window cost is acceptable.buildDigestreadsZRANGEBYSCORE(accKey, windowStartMs, now)wherewindowStartMsis per-rule. Daily users see residual'General'for ~24-48h; weekly users for up to ~7d (bounded bySTORY_TTL = 7d).DIGEST_ACCUMULATOR_TTL = 48his whole-key EXPIRE, not member pruning. Acceptable because residue = today's behavior ('General'), not a regression.STORY_TRACKING_TTL_S = 172800export at:15and corrects the stale "TTL for all: 48h" comment block — the exact confusion that triggered a round-1 plan correction.Test plan
tests/news-story-track-description-persistence.test.mts(3 new: T1 + T2-T4 defensive)tests/digest-buildDigest-category-passthrough.test.mjs(T6 wiring, T7 location)tests/brief-filter.test.mjs(T9, T10 parameterized × 13 enum, T10b multi-word, T11 + T11b ordering invariant, T12 fallback, T13 defensive)brief-filter+brief-from-digest-stories+brief-llm+brief-composer-rule-dedup+ persistence + buildDigest tests — no regressionsfilterTopStoriesunchanged — U3 adds 1 helper + 1 line at out.push)npx tsc --noEmiton modified TypeScript file: clean'General'for up to ~7 days.Plan + review trail
docs/plans/2026-05-17-002-fix-persist-story-track-category-plan.md(committed in U1)🤖 Generated with Claude Code