fix(digest): brief filter-drop instrumentation + cache-key correctness#3387
Conversation
buildDigest filters by rule.sensitivity BEFORE dedup, but digestFor memoized only on (variant, lang, windowStart). Stricter-sensitivity users in a shared bucket inherited the looser populator's pool, producing the wrong story set and defeating downstream topic-grouping adjacency once filterTopStories re-applied sensitivity. Solution 1 from docs/plans/2026-04-24-004-fix-brief-topic-adjacency-defects-plan.md.
Adds an optional onDrop metrics callback to filterTopStories and threads
it through composeBriefFromDigestStories. The seeder aggregates counts
per composed brief and emits one structured log line per user per tick:
[digest] brief filter drops user=<id> sensitivity=<s> in=<count>
dropped_severity=<n> dropped_url=<n> dropped_headline=<n>
dropped_shape=<n> out=<count>
Decides whether the conditional Solution 3 (post-filter regroup) is
warranted by quantifying how often post-group filter drops puncture
multi-member topics in production. No behaviour change for callers
that omit onDrop.
Solution 0 from docs/plans/2026-04-24-004-fix-brief-topic-adjacency-defects-plan.md.
Review surfaced two P2 gaps in the filter-drop telemetry that weakened its diagnostic purpose for Sol-3 gating: 1. Cap-truncation silent drop: filterTopStories broke on `out.length >= maxStories` BEFORE the onDrop emit sites, so up to (DIGEST_MAX_ITEMS - MAX_STORIES_PER_USER) stories per user were invisible. Added a 'cap' reason to DropMetricsFn and emit one event per skipped story so `in - out - sum(dropped_*) == 0` reconciles. 2. Wipeout invisibility: composeAndStoreBriefForUser only logged drop stats for the WINNING candidate. When every candidate composed to null, the log line never fired — exactly the wipeout case Sol-0 was meant to surface. Now tracks per-candidate drops and emits an aggregate `outcome=wipeout` line covering all attempts. Also tightens the digest-cache-key sensitivity regex test to anchor inside the cache-key template literal (it would otherwise match the unrelated `chosenCandidate.sensitivity ?? 'high'` in the new log line). PR review residuals from docs/plans/2026-04-24-004-fix-brief-topic-adjacency-defects-plan.md ce-code-review run 20260424-232911-37a2d5df.
The ce-code-review skill writes per-run artifacts (reviewer JSON, synthesis.md, metadata.json) under .context/compound-engineering/. These are local-only — neither tracked nor linted.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThis PR ships two targeted fixes: a one-line cache-key correctness fix that adds
Confidence Score: 3/5Safe to merge with a one-line fix — the sensitivity default mismatch in One P1 finding (sensitivity default
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cron tick] --> B[digestFor: memoize by variant+lang+filter+window]
B --> C{cache hit?}
C -->|yes| E[return cached pool]
C -->|no| D[buildDigest story pool]
D --> E
E --> F[composeBriefFromDigestStories]
F --> G[filterTopStories with onDrop callback]
G --> H{story passes?}
H -->|yes| I[add to output]
H -->|no| J[onDrop fired: reason = severity / headline / url / shape]
H -->|cap reached| K[onDrop fired: reason = cap for each remaining]
I --> L{envelope produced?}
J --> L
K --> L
L -->|yes| M[log outcome=shipped with drop counts]
L -->|no, candidates exhausted| N{any non-empty pool attempted?}
N -->|yes| O[log outcome=wipeout]
N -->|no| P[silent return null]
Reviews (1): Last reviewed commit: "chore: ignore .context/ ce-code-review r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function composeBriefFromDigestStories(rule, digestStories, insightsNumbers, { nowMs = Date.now() } = {}) { | ||
| export function composeBriefFromDigestStories(rule, digestStories, insightsNumbers, { nowMs = Date.now(), onDrop } = {}) { | ||
| if (!Array.isArray(digestStories) || digestStories.length === 0) return null; | ||
| const sensitivity = rule.sensitivity ?? 'all'; |
There was a problem hiding this comment.
sensitivity default mismatch between compose and buildDigest
composeBriefFromDigestStories defaults to 'all' when rule.sensitivity is undefined, but buildDigest (line 392) and the new digestFor cache key both default to 'high'. For a user whose sensitivity field is not set, buildDigest produces a pool containing only critical+high stories, while this layer then calls filterTopStories with sensitivity='all'. The practical result is correct today (compose can't re-surface stories that buildDigest already dropped), but the intent is wrong and the cache-key comment in the new test explicitly notes that the 'high' default must match buildDigest — leaving this defaulting to 'all' means the three layers that must agree now only agree two-out-of-three.
| const sensitivity = rule.sensitivity ?? 'all'; | |
| const sensitivity = rule.sensitivity ?? 'high'; |
| } | ||
| console.log( | ||
| `[digest] brief filter drops user=${userId} ` + | ||
| `sensitivity=${allCandidateDrops[0].sensitivity} ` + |
There was a problem hiding this comment.
When multiple candidates all compose to null, allCandidateDrops[0].sensitivity reflects only the first attempt. If a user has candidates with, say, sensitivity: 'all' followed by sensitivity: 'high', an operator seeing outcome=wipeout sensitivity=all might investigate the wrong tier. The attempts=N field conveys that multiple candidates failed, but not their individual sensitivities. Consider logging a comma-separated list of distinct values so the wipeout signal accurately represents all failed attempts.
| { nowMs }, | ||
| { | ||
| nowMs, | ||
| onDrop: (ev) => { dropStats[ev.reason] = (dropStats[ev.reason] ?? 0) + 1; }, |
There was a problem hiding this comment.
?? 0 is dead code in the drop accumulator
dropStats is initialised with all five reason keys set to 0 on the line above, so dropStats[ev.reason] is never undefined for a known reason. The ?? 0 guard is unreachable and slightly misleading — it implies the key might be absent.
| onDrop: (ev) => { dropStats[ev.reason] = (dropStats[ev.reason] ?? 0) + 1; }, | |
| onDrop: (ev) => { dropStats[ev.reason] += 1; }, |
Addresses two PR #3387 review findings: - P2: Earlier candidates that composed to null (wiped out by post-group filtering) had their dropStats silently discarded when a later candidate shipped — exactly the signal Sol-0 was meant to surface. - P3: outcome=wipeout row was labeled with allCandidateDrops[0] .sensitivity, misleading when candidates within one user have different sensitivities. Fix: emit one structured row per attempted candidate, tagged with that candidate's own sensitivity and variant. Outcome is shipped|rejected. A wipeout is now detectable as "all rows for this user are rejected within the tick" — no aggregate-row ambiguity. Removes the allCandidateDrops accumulator entirely.
Review fixes — both findings addressed (commit
|
…to 'high' Addresses PR #3387 review (P2): composeBriefFromDigestStories defaulted to `?? 'all'` while buildDigest, the digestFor cache key, and the new per-attempt log line all default to `?? 'high'`. The mismatch is harmless in production (the live cron path pre-filters the pool) but: - A non-prefiltered caller with undefined sensitivity would silently ship medium/low stories. - Per-attempt telemetry labels the attempt as `sensitivity=high` while compose actually applied 'all' — operators are misled. Aligning compose to 'high' makes the four sites agree and the telemetry honest. Production output is byte-identical (input pool was already 'high'-filtered upstream). Adds 3 regression tests asserting the new default: critical/high admitted, medium/low dropped, and onDrop fires reason=severity for the dropped levels (locks in alignment with per-attempt telemetry).
P2 sensitivity-default alignment — fixed in
|
Addresses PR #3387 review (P2 + P3): three more sites still defaulted missing sensitivity to 'all' while compose/buildDigest/cache/log now treat it as 'high'. P2 — compareRules (scripts/lib/brief-compose.mjs:35-36): the rank function used to default to 'all', placing legacy undefined-sensitivity rules FIRST in the candidate order. Compose then applied a 'high' filter to them, shipping a narrow brief while an explicit 'all' rule for the same user was never tried. Aligned to 'high' so the rank matches what compose actually applies. P3 — enrichBriefEnvelopeWithLLM (scripts/lib/brief-llm.mjs:526): the digest prompt and cache key still used 'all' for legacy rules, misleading personalization ("Reader sensitivity level: all" while the brief contains only critical/high stories) and busting the cache for legacy vs explicit-'all' rows that should share entries. Also aligns the @deprecated composeBriefForRule (line 164) for consistency, since tests still import it. 3 new regression tests in tests/brief-composer-rule-dedup.test.mjs lock in the new ranking: explicit 'all' beats undefined-sensitivity, undefined-sensitivity ties with explicit 'high' (decided by updatedAt), and groupEligibleRulesByUser candidate order respects the rank. 6853/6853 tests pass (was 6850 → +3).
Remaining sensitivity-default sites aligned in
|
| Site | Before | After |
|---|---|---|
compareRules (brief-compose.mjs:35-36) |
?? 'all' ❌ |
?? 'high' ✓ |
composeBriefForRule (brief-compose.mjs:164, deprecated) |
?? 'all' ❌ |
?? 'high' ✓ |
enrichBriefEnvelopeWithLLM (brief-llm.mjs:526) |
?? 'all' ❌ |
?? 'high' ✓ |
P2 — compareRules: legacy undefined-sensitivity rule used to outrank explicit 'all' (rank 0 vs 0, broken by updatedAt), so a multi-variant user could ship a narrow 'high'-filtered brief from the legacy rule while never trying the explicit 'all'. Rank now matches what compose applies.
P3 — enrichBriefEnvelopeWithLLM: the digest prompt no longer says "Reader sensitivity level: all" while shipping a critical/high-only brief, and the cache key shares entries between undefined-sensitivity and explicit 'high' rules. One-time legacy cache invalidation on deploy (orphaned entries; new requests recompute).
Regression coverage in tests/brief-composer-rule-dedup.test.mjs:
- Explicit
'all'beats undefined-sensitivity rule of same variant + age - Undefined-sensitivity ties with explicit
'high'(decided by updatedAt) groupEligibleRulesByUsercandidate order respects the new ranking
npm run test:data: 6853/6853 (was 6850 → +3 new compareRules tests).
…nce kept it at 12) (#3389) * fix(brief): bump MAX_STORIES_PER_USER 12 → 16 Production telemetry from PR #3387 surfaced cap-truncation as the dominant filter loss: 73% of `sensitivity=all` users had `dropped_cap=18` per tick (30 qualified stories truncated to 12). Multi-member topics straddling the position-12 boundary lost members. Bumping the cap to 16 lets larger leading topics fit fully without affecting `sensitivity=critical` users (their pools cap at 7-10 stories — well below either threshold). Reduces dropped_cap from ~18 to ~14 per tick. Validation signal: watch the `[digest] brief filter drops` log line on Railway after deploy — `dropped_cap=` should drop by ~4 per tick. Side effect: this addresses the dominant production signal that Solution 3 (post-filter regroup, originally planned in docs/plans/2026-04-24-004-fix-brief-topic-adjacency-defects-plan.md) was supposed to handle. Production evidence killed Sol-3's premise (0 non-cap drops in 70 samples), so this is a simpler, evidence-backed alternative. * revise(brief): keep MAX_STORIES_PER_USER default at 12, add env-tunability Reviewer asked "why 16?" and the honest answer turned out to be: the data doesn't support it. After landing PR #3390's sweep harness with visible-window metrics, re-ran against 2026-04-24 production replay: threshold=0.45 cap=12 -> visible_quality 0.916 (best at this cap) threshold=0.45 cap=16 -> visible_quality 0.716 (cap bump HURTS) threshold=0.42 cap=12 -> visible_quality 0.845 threshold=0.42 cap=16 -> visible_quality 0.845 (neutral) At the current 0.45 threshold, positions 13-16 are mostly singletons or members of "should-separate" clusters — they dilute the brief without helping topic adjacency. Bumping the cap default to 16 was a wrong inference from the dropped_cap=18 signal alone. Revised approach: - Default MAX_STORIES_PER_USER stays at 12 (matches historical prod). - Constant becomes env-tunable via DIGEST_MAX_STORIES_PER_USER so any future sweep result can be acted on with a Railway env flip without a redeploy. The actual evidence-backed adjacency fix from the sweep is to lower DIGEST_DEDUP_TOPIC_THRESHOLD from 0.45 -> 0.42 (env flip; see PR #3390). * fix(brief-llm): tie buildDigestPrompt + hashDigestInput slice to MAX_STORIES_PER_USER Greptile P1 on PR #3389: with MAX_STORIES_PER_USER now env-tunable, hard-coded stories.slice(0, 12) in buildDigestPrompt and hashDigestInput would mean the LLM prose only references the first 12 stories when the brief carries more. Stories 13+ would appear as visible cards but be invisible to the AI summary — a quiet mismatch between reader narrative and brief content. Cache key MUST stay aligned with the prompt slice or it drifts from the prompt content; same constant fixes both sites. Exports MAX_STORIES_PER_USER from brief-compose.mjs (single source of truth) and imports it in brief-llm.mjs. No behaviour change at the default cap of 12.
1 participant
Summary
Ships PRs 1-2 (and review residuals) from
docs/plans/2026-04-24-004-fix-brief-topic-adjacency-defects-plan.md— the plan diagnoses why today's brief surfaced Iran-Iran-Iran split into a pair plus an outlier, with a Nigeria-Nigeria gap broken by a movie-junk story. Topic-grouping is running every tick (production logs confirmtopics=K,fallback=false); the regression is downstream.This PR lands the two pieces of work the plan classifies as "ship today" — both small, both independent, both correctness-or-instrumentation.
Sol-1: Include sensitivity in
digestForcache keybuildDigestfilters byrule.sensitivitybefore dedup, butdigestFormemoized only on(variant, lang, windowStart). Stricter-sensitivity users in a shared bucket inherited the looser populator's pool, producing the wrong story set and defeating downstream topic-grouping adjacency oncefilterTopStoriesre-applied sensitivity. One-line cache-key change + 3 static-shape tests (mirrors the existing pattern intests/digest-score-floor.test.mjs).Sol-0: Per-user
filterTopStoriesdrop instrumentationAdds an optional
onDropcallback tofilterTopStoriesand threads it throughcomposeBriefFromDigestStories. The seeder aggregates counts per composed brief and emits one structured log line per user per tick:The data this produces decides whether the conditional Solution 3 (post-filter regroup, gated at >5% drop rates) is warranted. No behaviour change for callers that omit
onDrop.Code-review fixes (P2 from
ce-code-reviewautofix run)The review caught two real diagnostic gaps in Sol-0's first cut:
filterTopStoriesbroke onout.length >= maxStoriesbefore theonDropemit sites, so up to 18 valid stories per user were invisible. Added'cap'to theDropMetricsFnreason union and emit one event per skipped story soin - out - sum(dropped_*) == 0reconciles. New test locks in the invariant.composeAndStoreBriefForUseronly logged drop stats for the WINNING candidate. When every candidate composed to null, the log line never fired — the exact case Sol-0 was meant to surface. Now tracks per-candidate drops and emits an aggregateoutcome=wipeoutline covering all attempts.What's deferred
Per the plan's own gating, Solutions 2-5 are explicit follow-ups, not skipped work:
text-embedding-3-largefor topic pass): conditional on residual gaps after 1-3.Two side observations the plan flags as out of scope for this PR:
_classifier.tsEXCLUSIONS — separate classifier-hardening plan.Commits
77a64c632— fix(digest): include sensitivity indigestForcache key83a3beb13— feat(digest): instrument per-userfilterTopStoriesdrops570d4b934— fix(digest): close two Sol-0 instrumentation gaps from code reviewee3f807c3— chore: ignore.context/ce-code-review run artifactsTest plan
npm run typecheckandnpm run typecheck:apipassscripts/*.cjsparse withnode -cseed-digest-notifications.mjs:main()is unchanged)npm run test:data: 6847/6847 (was 6844 → +3 new cap tests, +3 new sensitivity-key tests, +7 new onDrop tests)tests/edge-functions.test.mjs: 177/177npm run lint:mdcleannpm run version:checkcleanPost-Deploy Monitoring & Validation
Log queries (Railway
scripts-cron-digest-notifications):[digest] brief filter drops user=— one row per user per tickoutcome=shippedvsoutcome=wipeoutdistributiondropped_url=anddropped_severity=counts per tickExpected healthy signals:
outcome=shippedfor ~all users;outcome=wipeoutshould be rare (single-digit per day at most).dropped_*counters mostly zero. Non-zerodropped_urlwould point at upstream RSS link quality issues.in == out + dropped_severity + dropped_headline + dropped_url + dropped_shape + dropped_capshould hold for every line.Failure signals & rollback:
outcome=wipeoutfor known-good users → revert this PR or setDIGEST_DEDUP_TOPIC_GROUPING=0.dropped_severity> 0 onoutcome=shippedlines after the cache fix → indicatesfilterTopStories's sensitivity gate is still firing post-Sol-1; investigate cache populator order.Validation window: 24h after first cron tick post-deploy. Owner: @eliehabib.
Trigger for Sol-3 follow-up: once 3+ days of clean drop logs are in hand, run the offline sweep harness (Sol-2a) and decide on threshold + Sol-3 ship.