fix(brief): derive On-The-Desk threads from the ordered story walk (F7 / PR E)#3697
Merged
Conversation
…7 / PR E) Phase 6 of the brief-pipeline plan. The "On The Desk" threads page was synthesised by the LLM as an INDEPENDENT editorial judgment, with no constraint tying it to the rendered story walk. On 2026-05-13 that produced the F7 bug: the threads page listed topics in an order the story walk didn't follow, and a story (hantavirus) was covered by no thread at all. Fix (the plan's directive — "regenerate threads from the FINAL ordered story list rather than from an independent judgment"): - New `deriveThreadsFromOrderedStories(stories)` in brief-compose.mjs — one thread per topic-cluster, in walk order. `orderBriefCandidates` already emits same-cluster stories contiguously, so a consecutive-run group on `clusterId` reproduces the walk's block order without the transient topic key (deliberately not written onto BriefStory). tag = the cluster's category; teaser = the cluster lead's `description`. - Wired into `composeAndStoreBriefForUser` AFTER `enrichBriefEnvelopeWithLLM`, so the teaser is the LLM per-story editorial sentence (filter-stage `description || headline` fallback guarantees non-empty). Overwrites `digest.threads` (+ `publicThreads` when present); re-asserts the envelope and falls back to the prior threads if the derived shape somehow fails. The LLM still emits `synthesis.threads` — it stays the haystack `checkLeadGrounding` (PR #3667) inspects — only the RENDERED threads change. Threads now structurally cannot disagree with the walk, and every cluster is covered (the renderer paginates >6, so no story is orphaned). 13 new unit + integration tests; 407 brief/digest tests pass; biome clean (the 2 pre-existing cognitive-complexity warnings on buildDigest / main() are unchanged).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR closes the 2026-05-13 "On The Desk" threads regression (F7) by replacing the LLM's independent thread ordering with one derived deterministically from the final ordered story walk. The new
Confidence Score: 4/5Safe to merge; the change is well-scoped, the fallback chain prevents any regression in the shipped envelope, and the LLM synthesis path is unaffected. A multi-story cluster whose lead story has no usable text would be silently dropped from the derived threads output — subsequent members are not promoted because lastClusterId is updated even when no thread is emitted. The filter layer prevents this in normal operation, but the gap is untested and there is no warning emitted if it happens. Outside this edge case the logic is sound and the double-assertion fallback pattern means no broken envelope can reach Redis. The deriveThreadsFromOrderedStories function in scripts/lib/brief-compose.mjs (lines 636-640) deserves a second look for the skipped-lead/same-cluster scenario described in the comment. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[composeAndStoreBriefForUser] --> B[composeBriefFromDigestStories\nbuilds envelope with LLM synthesis.threads]
B --> C{BRIEF_LLM_ENABLED?}
C -- yes --> D[enrichBriefEnvelopeWithLLM\nper-story descriptions filled in]
C -- no --> E[finalEnvelope = unenriched]
D --> F{assertBriefEnvelope passes?}
F -- yes --> G[finalEnvelope = enriched]
F -- no --> E
G --> H[deriveThreadsFromOrderedStories\nfinalEnvelope.data.stories]
E --> H
H --> I{derivedThreads.length > 0?}
I -- no --> J[keep prior threads\nfallback path]
I -- yes --> K[build withThreads envelope\noverwrite digest.threads + publicThreads]
K --> L{assertBriefEnvelope passes?}
L -- yes --> M[finalEnvelope = withThreads]
L -- no --> N[console.warn\nkeep prior threads]
M --> O[SETEX to Redis]
J --> O
N --> O
Reviews (1): Last reviewed commit: "fix(brief): derive On-The-Desk threads f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+636
to
+640
| // Skip a cluster lead with no usable text rather than emit an | ||
| // invalid `{tag, teaser:''}` the renderer's assert would reject. | ||
| if (teaser.length > 0) threads.push({ tag, teaser }); | ||
| lastClusterId = clusterId; | ||
| started = true; |
Contributor
There was a problem hiding this comment.
When the lead story of a multi-story cluster has neither description nor headline, the thread is skipped but lastClusterId is still updated to that cluster's ID. Every subsequent story in the same cluster then fails the boundary check (clusterId !== lastClusterId is false, clusterId === null is false) and is skipped too — the entire cluster disappears from the output with no warning. The existing test ("a story with neither description nor headline is skipped") only exercises singleton clusters with a different clusterId next, so this gap is untested. The filter-stage guarantee of a non-empty string prevents it in normal flow, but a console.warn or promotion of the next member would make a future regression visible.
Merged
7 tasks
koala73
added a commit
that referenced
this pull request
May 17, 2026
…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.
koala73
added a commit
that referenced
this pull request
May 17, 2026
…showing 8/8 General (#3751) * feat(brief): persist category on story:track:v1 + cache-keys doc cleanup (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). * feat(brief): pass track.category through buildDigest's stories.push (U2) 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). * feat(brief): word-wise titleCase on envelope category at filterTopStories (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. * fix(brief): review-driven fixes from /ce-code-review adversarial findings 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. * fix(brief): address 2 Greptile P2 inline comments on PR #3751 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 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
PR E — the final phase (Phase 6 / Finding F7) of the brief-pipeline plan (
docs/plans/2026-05-14-001-…). PRs A–D are merged; this is independent, branched frommain.The "On The Desk" threads page was synthesised by the LLM as an independent editorial judgment, with nothing tying it to the rendered story walk. On 2026-05-13 that produced F7: the threads page listed topics in an order the story walk didn't follow, and a story (hantavirus) was covered by no thread at all.
Change
Per the plan's directive — "regenerate threads from the FINAL ordered story list rather than from an independent judgment":
deriveThreadsFromOrderedStories(stories)inbrief-compose.mjs— one thread per topic-cluster, in walk order.orderBriefCandidatesalready emits same-cluster stories contiguously, so a consecutive-run group onclusterIdreproduces the walk's block order exactly — no need for the transient topic key (which is deliberately not written ontoBriefStory).tag= the cluster'scategory;teaser= the cluster lead'sdescription.composeAndStoreBriefForUserafterenrichBriefEnvelopeWithLLM, so the teaser is the LLM per-story editorial sentence (the filter-stagedescription || headlinefallback guarantees a non-empty string). Overwritesdigest.threads(+publicThreadswhen present), re-asserts the envelope, and falls back to the prior threads if the derived shape somehow fails.Why this is safe / scoped
synthesis.threads— it stays the haystackcheckLeadGrounding(PR fix(brief): grounding gate on digest synthesis — block hallucinated leads #3667) inspects. Only the rendered threads change.publicThreadskept in sync — derived threads carry no personalised content, so the share surface renders the same set.{tag, teaser}envelope contract is unchanged;assertBriefEnvelopere-validates post-swap.Test plan
tests/brief-from-digest-stories.test.mjs: walk-order match, one-thread-per-cluster, contiguous multi-story cluster collapses to one, no-orphan coverage (the May 13 hantavirus case), teaser→headline fallback, tag→"General" fallback, empty-teaser skip, defensive null-clusterId, empty/non-array input, and an integration test that derived threads still passassertBriefEnvelope.tsx --test.noExcessiveCognitiveComplexitywarnings (buildDigest,main()) are unchanged; this change adds zero new warnings.