Summaries describe content only, never classify the article #77
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Reference
marvin8/yunjin!77
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/issue-70-summary-prompt"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Aggregate summaries carried classification and commentary instead of summary text — across the 7 Oct prod copy, roughly a third of aggregates contained meta-subject phrasing (34% contained "standalone" alone), e.g. "The piece is a standalone music and culture feature rather than coverage of a news event." That text tells the reader nothing they would not learn within seconds of reading the article.
Root cause: the system prompt demanded an overview of 2–3 paragraphs for every group, but ~80% of aggregates hold exactly one article. With nothing to condense "together", the model filled its paragraph quota with genre classification and grouping rationale — the two failure modes in #70.
What this does
The overview instruction in
_SYSTEM_PROMPT(services/llm_aggregator.py) now:Prompt-construction tests assert the new rules (TDD, red → green). Recorded-fixture response tests are unaffected — the prompt text is not part of the recorded responses, so no live LLM calls were needed.
Closes #70
Roughly a third of prod aggregates carried classification or commentary instead of summary: genre labels ("standalone music and culture feature rather than coverage of a news event") and grouping rationale mirroring the prompt's own clustering vocabulary. With ~80% of groups holding a single article, the fixed 2-3-paragraph quota was a padding machine — nothing to condense, so the model filled the paragraphs with classification. The overview instruction now: states the summary covers what happened, key facts, context and differing angles only; explicitly bans type/genre/format/newsworthiness classification and grouping rationale; and makes length adaptive — one paragraph for a single-article group, two or three only when genuinely condensing multiple sources. Recorded-fixture response tests are unaffected (prompt text is not part of the recorded responses). Closes #70WuMing
Found 4 issue(s). See inline comments below.
@ -90,1 +90,4 @@across sources.- Summaries must describe content only. NEVER describe or classify the article(s)themselves — no statements about their type, genre, format, or newsworthiness(e.g. "opinion piece", "live blog", "roundup", "standalone story", "news event",code [LOW]
The ban is enforced only by enumerating the exact phrases to avoid. Negative instructions that quote the target tokens verbatim ('standalone story', 'live blog', 'news event') can prime the model toward emitting them, and the issue reports 34% of aggregates already contained 'standalone'. Consider describing the prohibited category abstractly (e.g. 'do not comment on the article's type, genre, format or newsworthiness') and pair the prompt change with a lightweight post-processing check that flags/rejects overviews containing these phrases, so compliance is not left solely to the model.
@ -995,3 +995,1 @@def test_prompt_requires_multi_paragraph_overview(self):"""Test the prompt asks for a multi-paragraph condensed overview."""assert "2-3 paragraphs" in _SYSTEM_PROMPTdef test_prompt_bans_article_classification_in_summaries(self):code [MEDIUM]
The new TestSystemPrompt tests only assert that literal phrases exist in _SYSTEM_PROMPT (a tautology: they check substring presence, not behaviour, and would still pass if the negation were inverted or the rule were contradicted elsewhere in the prompt). More importantly, nothing verifies the actual generated overviews. The PR states the recorded-fixture response tests are unaffected because prompt text is not part of the recordings, which means those fixtures were produced under the OLD prompt and can still contain the classification text described in #70 while CI stays green. Add an output-level regression test (e.g. a fixture recorded with the new prompt, or a deterministic post-generation check) that asserts overviews for single-article groups contain no classification phrasing such as 'standalone', 'live blog', 'news event', or grouping rationale.
@ -998,0 +997,4 @@lowered = _SYSTEM_PROMPT.lower()assert "never describe or classify" in loweredassert "standalone" in loweredtests [LOW]
These assertions check only that the tokens "standalone" and "live blog" appear somewhere in the prompt, not that they are prohibited. A prompt that said, e.g., "mention whether the story is a standalone story" would still satisfy them, so the test cannot catch the regression it is meant to guard against. Assert on the ban sentence instead (e.g.
assert "never describe or classify the article" in loweredplus a check that the banned examples appear within that prohibition), or compare the prompt against the expected banned-phrase list.@ -998,0 +1006,4 @@assert "never explain or justify the grouping" in lowereddef test_prompt_overview_length_is_adaptive(self):tests [LOW]
This test verifies the new adaptive wording is present but never verifies the old fixed quota was removed, which is the actual root cause fixed in #70. Since the prompt still contains the words "two or three paragraphs", a partial revert that reintroduces "overview of 2-3 paragraphs" would pass unnoticed. Add a negative assertion such as
assert "2-3 paragraphs" not in lowered.Review wave addressed — 3 findings fixed, 2 partials with reasoning
(4657) · (4659) · (4660) · (4661)
✅ Fixed in
cf28daf— output-level regression guard (4657, 4659 second half): a post-parse regex check now logs any overview containing the known classification phrasing (same pattern list as the #70 prevalence count), on both the aggregate() and refresh() paths, with a unit test proving the detection fires and stays quiet on clean summaries.✅ Fixed in
cf28daf— tautological assertions (4660, 4661): the tests now assert the ban sentence itself and that the banned examples sit inside the prohibition (ordering check), and that the old fixed quota ("2-3 paragraphs") stays absent so a partial revert cannot pass.✅ Partially / 🔴 declined in part (4659): the monitoring added above is deliberately log-only, never rejecting — a style check must not discard an otherwise good grouping (that trade-off is recorded in the #70 investigation comment). The concrete example phrases are kept: they follow the abstract prohibition sentence and bound the category more tightly than an abstract description alone; whether they prime the model is now empirically observable via the new monitoring rather than assumed. If recurrence shows up in the logs, trimming the examples is a one-line follow-up with evidence behind it.
✅ Partially (4657 first half): re-recording fixtures under the new prompt needs a live LLM call — the recorded fixtures are response snapshots (data, not code), so asserting on their prose would test fixture data; project convention keeps live LLM calls out of the suite. The detection test added above is the enforceable output-level guard available in CI.
WuMing
Found 3 issue(s). See inline comments below.
@ -97,0 +106,4 @@# for trend-watching only, never enforced — a style check must not lose a# good grouping._META_COMMENTARY_PATTERN = re.compile(r"\bstandalone\b|\blive blog\b|\broundup\b|\bopinion piece\b|\bnews event\b|rather than coverage",code [LOW]
The monitoring pattern is broader (and narrower) than the prompt it is meant to watch.
\bstandalone\bmatches the bare word, but the prompt bans the phrase "standalone story" (line 94); a legitimate summary sentence containing "standalone" (e.g. describing a standalone product launch) will be logged as a violation and erode trust in the signal. Conversely, "advertising feature" is banned by the prompt but absent from the regex, so that failure mode is never observed. Align the pattern with the exact phrases the prompt lists (or use the same list to build both the prompt and the regex) so the metric measures what it claims to measure.@ -999,0 +1060,4 @@with caplog.at_level(logging.WARNING, logger="yunjin.services.llm_aggregator"):_log_meta_commentary([topic], user_id=3)assert not [record for record in caplog.records if record.levelno == logging.WARNING]code [MEDIUM]
This negative assertion can pass vacuously. Unlike its sibling test (line 1043), it never re-enables the module logger, and the comment on line 1039-1041 in that test states that Alembic's fileConfig (run by init_db earlier in this file) disables existing loggers. With
yunjin.services.llm_aggregatordisabled, no records are emitted at all, socaplog.recordswould be empty and the test would succeed even if_log_meta_commentarywarned on clean summaries. Add the samemonkeypatch.setattr(logging.getLogger("yunjin.services.llm_aggregator"), "disabled", False)(and takemonkeypatchas a fixture) here. Also filter byrecord.nameso an unrelated WARNING emitted by another library during the call cannot make this test flaky.tests [MEDIUM]
This negative test cannot fail. The comment in the sibling test (line 1039-1041) documents that Alembic's fileConfig, run by init_db earlier in this file, disables the 'yunjin.services.llm_aggregator' logger, which is why that test has to monkeypatch
disabled = False. monkeypatch restores the original value after that test, so by the time this test runs the logger is disabled again and everylogger.warning()call is silently dropped before a record is created —caplogwill always be empty regardless of whether_log_meta_commentarydecides a clean summary is offending. Apply the samemonkeypatch.setattr(module_logger, "disabled", False)workaround here (or better, add a session/module-level autouse fixture that re-enables the logger) so the assertion actually exercises the code path.Review wave addressed — all 3 findings fixed in
1eae416(4669) · (4670) · (4671)
✅ Fixed — vacuous negative test (4669, 4671): the clean-summary test now re-enables the module logger (same alembic workaround as its sibling) and filters records by logger name, so an unrelated library warning cannot make it flaky and a regression cannot hide behind a disabled logger.
✅ Fixed — pattern aligned with the prompt (4670): the prompt's example list and the monitoring regex are now generated from one shared
_BANNED_COMMENTARY_PHRASEStuple ("standalone story" not bare "standalone"; "advertising feature" now watched). New tests pin both sides: a standalone-product summary is not flagged, an advertising-feature summary is.The autouse-fixture idea for the alembic logger workaround was noted but deliberately not taken here — changing shared test infrastructure ripples into other files' caplog assertions and deserves its own change if the workaround keeps multiplying.
WuMing
Found 5 issue(s). See inline comments below.
@ -97,0 +121,4 @@# for trend-watching only, never enforced — a style check must not lose a# good grouping. Built from _BANNED_COMMENTARY_PHRASES so it stays in lockstep# with the prompt; "rather than coverage" watches the grouping-rationale ban._META_COMMENTARY_PATTERN = re.compile(code [LOW]
Monitor coverage is narrower than the prompt's ban. The prompt (lines 108-111) bans both grouping-rationale forms — "separate from other coverage of..." and "rather than a news event" — but the regex only watches the literal "rather than coverage". More importantly, the dominant failure mode reported in #70 (34% of aggregates containing bare "standalone") is deliberately not matched, so a production regression of exactly the kind this PR fixes would not be detected. If bare "standalone" truly must stay out, at least add a targeted alternative such as "standalone (story|piece|feature|article)" and "separate from other coverage" so the monitor tracks the prompts's stated prohibitions.
@ -97,0 +122,4 @@# good grouping. Built from _BANNED_COMMENTARY_PHRASES so it stays in lockstep# with the prompt; "rather than coverage" watches the grouping-rationale ban._META_COMMENTARY_PATTERN = re.compile(r"\b(?:rather than coverage|" + "|".join(re.escape(phrase) for phrase in _BANNED_COMMENTARY_PHRASES) + r")\b",code [LOW]
The monitoring regex matches the bare words "roundup" and "news event" anywhere in a summary, but those words are legitimate content vocabulary (e.g. "a roundup of the weekend's results", "the news event drew thousands"). Because the module comment claims the monitor "measures exactly what the prompt prohibits", these false positives will inflate the very trend metric this PR introduces to track the #70 fix, and each one emits a warning-level log line. Consider requiring classification context (e.g. "is a roundup", "an? roundup (piece|story|article)", or an article-noun after the phrase) rather than matching the noun alone, or log these at INFO so trend queries can distinguish them.
@ -478,6 +535,8 @@ class LlmAggregationService:)continue_log_meta_commentary(topics, user.id)code [LOW]
The new wiring is untested:
_log_meta_commentaryis unit-tested in isolation, but neither this call site nor the one in the refresh path (line 604) has a test asserting the warning is actually emitted duringrun()/refresh with a classification-style summary, nor that a match never alters the stored grouping. Since the PR's whole claim is "monitoring, never enforcement", add a test that runs an aggregation with a meta-commentary summary and asserts (a) the warning fires and (b) the resulting aggregate summary/topics are unchanged.@ -998,0 +1002,4 @@# The banned examples must sit inside the prohibition sentence, not# float free where they could read as an instruction to emit them.ban_position = lowered.index("never describe or classify")assert lowered.index("standalone", ban_position) > ban_positioncode [LOW]
This assertion does not test what its comment claims.
str.index(sub, start)only reports where the substring occurs at/afterstart, so it passes as long as "standalone" appears anywhere after the prohibition sentence — it cannot detect the banned example "floating free" elsewhere in the prompt (e.g. in a later section). To actually pin the examples inside the ban sentence, slice the sentence first, e.g.ban_sentence = lowered[ban_position:lowered.index(")", ban_position)]and assert both "standalone" and "live blog" are inside that slice.@ -999,0 +1032,4 @@"""Test overviews that classify the article are flagged for monitoring (#70)."""topic = _LlmTopic(name="Racing milestone",summary="The win is a standalone story, separate from other coverage of the race.",tests [LOW]
This test uses a summary that contains both a banned classification phrase ("standalone story") and a grouping-rationale phrase ("separate from other coverage of the race"), but _META_COMMENTARY_PATTERN only matches the former. The warning is therefore triggered solely by "standalone story", so the grouping-rationale half of the pattern (the "rather than coverage" alternative, which the prompt's ban on "separate from other coverage of..."/"rather than a news event" is supposed to watch) is never exercised anywhere in the suite and could regress silently. Add a case whose only violation is the grouping-rationale wording, and assert on the logged match text so the test proves which alternative fired.
Review wave addressed — all 5 findings fixed in
676fc17(4677) · (4679) · (4682) · (4681) · (4683)
✅ INFO severity (4677): matches now log at INFO, the alternative you offered — the watched vocabulary ("roundup", "news event") is ambiguous in legitimate prose, so records are trend data, not alerts; the phrases stay watched because the #70 investigation showed exactly this vocabulary in the offending lines.
✅ Wider coverage (4679): the pattern now also watches "separate from other coverage" and standalone + article-noun variants ("standalone piece/feature/article"), tested in both directions (variant flagged; bare "standalone product" still not).
✅ Wire-level tests (4682): aggregate() and refresh() each get a test proving the monitor fires during a real run with a classification-style summary and that the stored grouping content is untouched by it.
✅ Ban-sentence slice (4681): the test now slices between the two NEVER sentences and requires both examples inside, so floating examples elsewhere in the prompt cannot pass.
✅ Rationale-only case (4683): a test whose only violation is the grouping-rationale wording now asserts the logged match text.