feat(digest-dedup): replayable per-story input log (opt-in, no behaviour change)#3330
Conversation
…our change)
Ship the measurement layer before picking any recall-lift strategy.
Why: the current dedup path embeds titles only, so brief-wire headlines
that share a real event but drop the geographic anchor (e.g. "Alleged
Coup: defendant arrives in court" vs "Trial opens after Nigeria charges
six over 2025 coup plot") can slip past the 0.60 cosine threshold. To
tune recall without regressing precision we need a replayable per-tick
dataset — one record per story with the exact fields any downstream
candidate (title+slug, LLM-canonicalise, text-embedding-3-large, cross-
encoder re-rank, etc.) would need to score.
This PR ships ONLY the log. Zero behaviour change:
- Opt-in via DIGEST_DEDUP_REPLAY_LOG=1 (default OFF).
- Writer is best-effort: all errors swallowed + warned, never affects
digest delivery. No throw path.
- Records include hash, originalIndex, isRep, clusterId, raw +
normalised title, link, severity/score/mentions/phase/sources,
embeddingCacheKey, hasEmbedding sidecar flag, and the tick's config
snapshot (mode, clustering, cosineThreshold, topicThreshold, veto).
- clusterId derives from rep.mergedHashes (already set by
materializeCluster) so the orchestrator is untouched.
- Storage: Upstash list keyed by {variant}:{lang}:{sensitivity}:{date}
with 30-day EXPIRE. Date suffix caps per-key growth; retention
covers the labelling cadence + cross-candidate comparison window.
- Env flag is '1'-only (fail-closed on typos, same pattern as
DIGEST_DEDUP_MODE).
Activation path (post-merge): flip DIGEST_DEDUP_REPLAY_LOG=1 on the
seed-digest-notifications Railway service. Watch one cron tick for the
RPUSH + EXPIRE pair (or a single warn line if creds/upstream flake),
then leave running for at least one week to accumulate calibration data.
Tests: 21 unit tests covering flag parsing, key shape + sanitisation,
record field correctness (isRep, clusterId, embeddingCacheKey,
hasEmbedding, tickConfig), pipeline null/throw handling, malformed
input. Existing 77 dedup tests unchanged and still green.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThis PR adds an opt-in, append-only per-story replay log ( Confidence Score: 5/5Safe to merge; all findings are minor style suggestions with no runtime impact. All three P2 findings are non-blocking style points: a shared object reference only relevant to in-memory consumers, a post-dedup timestamp acceptable for calibration, and a cosmetic Redis key edge case. No logic bugs, data-loss risk, or security concerns. The failure-isolation contract (best-effort, never throws, env-gated) is correctly implemented and well-tested. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Cron as seed-digest-notifications
participant Dedup as deduplicateStories
participant Log as writeReplayLog
participant Redis as Upstash (replay-log key)
Cron->>Dedup: stories[]
Dedup-->>Cron: { reps, embeddingByHash, logSummary }
Cron->>Log: void writeReplayLog({ stories, reps, embeddingByHash, cfg, tickContext })
Note over Cron,Log: fire-and-forget — void discards the promise
alt DIGEST_DEDUP_REPLAY_LOG != '1'
Log-->>Cron: { wrote:0, skipped:'disabled' }
else stories empty
Log-->>Cron: { wrote:0, skipped:'empty' }
else happy path
Log->>Log: buildReplayRecords() → N JSON records
Log->>Log: buildReplayLogKey(ruleId, tsMs) → digest:replay-log:v1:{rule}:{date}
Log->>Redis: RPUSH key record0 … recordN
Log->>Redis: EXPIRE key 2592000
Redis-->>Log: [rpush_result, 1]
Log-->>Cron: { wrote:N, key, skipped:null }
else pipeline null / throws
Log->>Log: warn(...)
Log-->>Cron: { wrote:0, key, skipped:null }
end
Reviews (1): Last reviewed commit: "feat(digest-dedup): replayable per-story..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // purpose: dedup input is shared across users of the same (variant, | ||
| // lang, sensitivity), and we don't want user identity in log keys. | ||
| // See docs/brainstorms/2026-04-23-001-brief-dedup-recall-gap.md §5 Phase 1. | ||
| const tsMs = Date.now(); |
There was a problem hiding this comment.
tsMs captured post-dedup, not at tick start
const tsMs = Date.now() is sampled after await deduplicateStories(stories) returns, so it reflects the moment dedup finished rather than when the tick began processing. For a calibration log whose primary key is a date suffix this is harmless, but the briefTickId (${ruleKey}:${tsMs}) is documented as a "full tick id" and a downstream reader might expect it to anchor to tick start. Consider capturing tsMs before the deduplicateStories call, or naming the field loggedAtMs to set accurate expectations.
| const tsMs = Date.now(); | |
| const tsMs = Date.now(); | |
| const cfg = readOrchestratorConfig(process.env); | |
| const { reps: dedupedAll, embeddingByHash, logSummary } = | |
| await deduplicateStories(stories); | |
| // Replay log (opt-in via DIGEST_DEDUP_REPLAY_LOG=1). Best-effort — any | |
| // failure is swallowed by writeReplayLog. Runs AFTER dedup so the log | |
| // captures the real rep + cluster assignments. RuleId omits userId on | |
| // purpose: dedup input is shared across users of the same (variant, | |
| // lang, sensitivity), and we don't want user identity in log keys. | |
| // See docs/brainstorms/2026-04-23-001-brief-dedup-recall-gap.md §5 Phase 1. |
| export function buildReplayLogKey(ruleId, tsMs) { | ||
| // Allow ':' so `variant:lang:sensitivity` composite ruleIds stay | ||
| // readable as Redis key segments. Strip anything else to '_'; then | ||
| // if the whole string collapsed to nothing meaningful (all '_' or |
There was a problem hiding this comment.
safeRuleId fallback check ignores colons and hyphens
The emptiness check strips only underscores: raw.replace(/_/g, '') === ''. A ruleId consisting entirely of colons (e.g., ':::') sanitizes to ':::', passes the check, and produces a key like digest:replay-log:v1::::2026-04-23 with four consecutive colons. That's a valid Redis key but will confuse redis-cli KEYS/SCAN queries that use : as a namespace separator. Extending the strip to also remove : and - makes the guard tighter.
| export function buildReplayLogKey(ruleId, tsMs) { | |
| // Allow ':' so `variant:lang:sensitivity` composite ruleIds stay | |
| // readable as Redis key segments. Strip anything else to '_'; then | |
| // if the whole string collapsed to nothing meaningful (all '_' or | |
| const safeRuleId = raw.replace(/[_:\-]/g, '') === '' ? 'unknown' : raw; |
| if (typeof h === 'string' && !clusterByHash.has(h)) { | ||
| clusterByHash.set(h, clusterId); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // `repHashes` is a Set of the winning story's hash per cluster. A | ||
| // story is the rep iff its hash === the rep.hash at its clusterId. | ||
| const repHashes = new Set(); | ||
| if (Array.isArray(reps)) { |
There was a problem hiding this comment.
tickConfig object shared across all record entries
tickConfig is built once outside the forEach and the same object reference is pushed into every record. JSON.stringify in writeReplayLog serializes independently so there's no storage bug, but any in-memory consumer of the returned records array (future tests, replay harness) could mutate one record's tickConfig and silently affect all others. A shallow spread on push — tickConfig: { ...tickConfig } — costs nothing and removes the footgun.
Review catch (PR #3330): the tickConfig snapshot omitted topicGroupingEnabled even though readOrchestratorConfig returns it and the digest's post-dedup topic ordering gates on it. A tick run with DIGEST_DEDUP_TOPIC_GROUPING=0 serialised identically to a default tick, making those runs non-replayable for the calibration work this log is meant to enable. Add topicGroupingEnabled to the recorded tickConfig. One-line schema fix + regression test asserting topic-grouping-off ticks serialise distinctly from default. 22/22 tests pass.
|
Fixed in be8449a. You're right — The fix is a one-field addition to the |
….exit Review catch (PR #3330): the fire-and-forget `void writeReplayLog(...)` call could be dropped on the explicit-exit paths — the brief-compose failure gate at line 1539 and main().catch at line 1545 both call process.exit(1). Unlike natural exit, process.exit does not drain in-flight promises, so the last N ticks' replay records could be silently lost on runs where measurement fidelity matters most. Fix: await the writeReplayLog call. Safe because: - writeReplayLog returns synchronously when the flag is off (replayLogEnabled check is the first thing it does) - It has a top-level try/catch that always returns a result object - The Upstash pipeline call has a 10s timeout ceiling - buildDigest already awaits many Upstash calls (dedup, compose, render) so one more is not a hot-path concern Comment block added above the call explains why the await is deliberate — so a future refactor doesn't revert it to void thinking it's a leftover. No test change: existing writeReplayLog unit tests already cover the disabled / empty / success / error paths. The fix is a single-keyword change in a caller that was already guaranteed-safe by the callee's contract.
… log Three non-blocking polish items from the automated review, bundled because they all touch the same new module and none change behaviour. 1. tsMs captured BEFORE deduplicateStories (seed-digest-notifications.mjs). Previously sampled after dedup returned, so briefTickId reflected dedup-completion time rather than tick-start. For downstream readers the natural reading of "briefTickId" is when the tick began processing; moved the Date.now() call to match that expectation. Drift is maybe 100ms-2s on cold-cache embed calls — small, but moving it is free. 2. buildReplayLogKey emptiness check now strips ':' and '-' in addition to '_'. A pathological ruleId of ':::' previously passed through verbatim, producing keys like `digest:replay-log:v1::::2026-04-23` that confuse redis-cli's namespace tooling (SCAN / KEYS / tab completion). The new guard falls back to "unknown" on any input that's all separators. Added a regression test covering the ':::' / '---' / '___' / mixed cases. 3. tickConfig is now a per-record shallow copy instead of a shared reference. Storage is unaffected (writeReplayLog serialises each record via JSON.stringify independently) but an in-memory consumer that mutated one record's tickConfig for experimentation would have silently affected all other records in the same batch. Added a regression test asserting mutation doesn't leak across records. Tests: 24/24 pass (22 prior + 2 new regression). Typecheck + lint clean.
|
Addressed all three P2 comments in commit 2afaa05:
Tests: 24/24 pass (22 prior + 2 new regression). Typecheck + lint clean. |
|
Fixed in 0dac05a. You're right — Changed to
Added a comment block above the call explaining why the |
1 participant
Why this PR?
The current brief-dedup path embeds titles only, so wire headlines that share a real event but drop the geographic anchor can slip past the 0.60 cosine threshold. Concrete miss from the 2026-04-23 08:02 brief:
Both are the same Nigerian coup-plot trial. They should have been one rep with
mentionCount=2.To tune recall without regressing precision we need a replayable per-tick dataset — one record per story with every field any downstream candidate (title+slug, LLM-canonicalise,
text-embedding-3-large, cross-encoder re-rank, etc.) would need to re-score the full pair matrix offline. A baseline-band pair log alone is not enough: options that shift the score distribution surface misses that are currently below 0.45 or already above 0.60, and a band-scoped log is blind to those.This PR ships only the measurement layer. No recall-lift behaviour change, no model change, no threshold change. One env flip away from collecting data, a second flip away to turn off.
What this ships
scripts/lib/brief-dedup-replay-log.mjs— new pure module:replayLogEnabled(env)— gate, readsDIGEST_DEDUP_REPLAY_LOGat call time.buildReplayLogKey(ruleId, tsMs)— keyer,digest:replay-log:v1:{ruleId}:{YYYY-MM-DD}.buildReplayRecords(...)— pure record builder;clusterIdderives fromrep.mergedHashes(already set bymaterializeCluster) so the orchestrator stays untouched.writeReplayLog(...)— best-effort Upstash write. All failures caught + warned; never throws.scripts/seed-digest-notifications.mjs— one call site afterdeduplicateStories. Usesvoidbecause the writer never rejects; node drains outstanding promises before natural exit.tests/brief-dedup-replay-log.test.mjs— 21 tests.Record shape (one per input story)
{ "v": 1, "briefTickId": "full:en:high:1713859320000", "ruleId": "full:en:high", "tsMs": 1713859320000, "storyHash": "h1", "originalIndex": 0, "isRep": true, "clusterId": 0, "title": "Alleged Coup: One defendant arrives in court...", "normalizedTitle": "alleged coup: one defendant arrives in court...", "link": "https://premiumtimesng.com/...", "severity": "high", "currentScore": 12, "mentionCount": 1, "phase": "emerging", "sources": ["Premium Times"], "embeddingCacheKey": "brief:emb:v1:text-3-small-512:sha256...", "hasEmbedding": true, "tickConfig": { "mode": "embed", "clustering": "single", "cosineThreshold": 0.6, "topicThreshold": 0.45, "entityVetoEnabled": true } }Zero-behaviour-change guarantees
DIGEST_DEDUP_REPLAY_LOGdefaults OFF; requires literal"1"to enable (same fail-closed-on-typo pattern asDIGEST_DEDUP_MODE).brief-dedup.mjs).story:track:v1Redis schema.digest:replay-log:v1:*with 30-dayEXPIRE. Date suffix caps per-key growth.Activation path (post-merge)
DIGEST_DEDUP_REPLAY_LOG=1on theseed-digest-notificationsRailway service. Takes effect on next cron tick (no redeploy needed, same orchestrator pattern as otherDIGEST_DEDUP_*flags).Rollback
DIGEST_DEDUP_REPLAY_LOG=0or unset. Next cron tick stops writing.Tests
node --test tests/brief-dedup-replay-log.test.mjs→ 21/21 passnode --test tests/brief-dedup-embedding.test.mjs tests/brief-dedup-jaccard.test.mjs→ 77/77 pass (pre-existing, unchanged)npm run typecheck→ cleannpx biome linton changed files → cleanTest coverage
'1')[A-Za-z0-9:_-], collapses others to_, falls back to"unknown"if the result is all-underscoreisRep,clusterIdfrommergedHashes,embeddingCacheKey,hasEmbedding,tickConfigsnapshot,originalIndexpreservationwrote=0, pipeline throws → caught + warn +wrote=0, malformed args → no throwmergedHashesfalls back torep.hashalone (defensive)Related
Driven by internal brainstorm memo (gitignored under
docs/brainstorms/) that laid out the recall-gap diagnosis + 19 options. The doc's §5 Phase 1 specifies this log's record shape; its §7 pins this as the first ship, prerequisite for testing any of the recall-lift candidates (B URL-slug, H rare-token overlap, J verb-class gate, P adaptive threshold).Test plan
node --test tests/brief-dedup-replay-log.test.mjsnode --test tests/brief-dedup-embedding.test.mjs tests/brief-dedup-jaccard.test.mjsnpm run typechecknpx biome linton changed filesDIGEST_DEDUP_REPLAY_LOG=1on Railway, observe one tick in Upstash