Cap incremental EXISTING TOPICS to the most recently updated aggregates #80

agent-pi · 2026-10-11T01:18:25Z

agent-pi commented

2026-10-11 01:18:25 +00:00

Incremental aggregation runs sent the user's entire aggregate set to the LLM as EXISTING TOPICS — ~900 entries and growing, the dominant token cost, and a suspected cause of poor match rates.

Closes #74

What

New YUNJIN_MAX_PROMPT_TOPICS env var (default 100; unset/invalid/below-1 fall back to the default, same convention as the other aggregator vars).
The incremental path now lists at most 100 aggregates in EXISTING TOPICS — the most recently updated ones (the DB query already orders updated_at DESC, so the cap is a slice).
Consolidation passes are untouched: their EXISTING TOPICS are already bounded by input articles.

Why this is safe

#71's 7-day TTL already bounds the aggregate universe (590 of 898 in the 7 Oct prod snapshot); the cap bounds the prompt regardless of burstiness.
A re-reported older story is healed by the nightly consolidation pass whenever any of its articles is within the 30-day eligibility floor — its original aggregate holds an eligible article and so becomes a re-cluster candidate.
Stories fully past the eligibility horizon behave as before #71: fresh singleton, TTL clears it within a week.

Impact

At the default cap the incremental prompt drops from ~30–47k to ~5–8k tokens per run (5–8× vs post-TTL steady state) and the matching haystack shrinks accordingly.

Docs

docs/config.md gains the YUNJIN_MAX_PROMPT_TOPICS row in the LLM aggregation table.

Incremental aggregation runs sent the user's entire aggregate set to the LLM as EXISTING TOPICS — ~900 entries and growing, the dominant token cost, and a suspected cause of poor match rates. Closes #74 ## What - New `YUNJIN_MAX_PROMPT_TOPICS` env var (default **100**; unset/invalid/below-1 fall back to the default, same convention as the other aggregator vars). - The incremental path now lists at most 100 aggregates in EXISTING TOPICS — the **most recently updated** ones (the DB query already orders `updated_at DESC`, so the cap is a slice). - Consolidation passes are untouched: their EXISTING TOPICS are already bounded by input articles. ## Why this is safe - #71's 7-day TTL already bounds the aggregate universe (590 of 898 in the 7 Oct prod snapshot); the cap bounds the prompt regardless of burstiness. - A re-reported older story is healed by the nightly consolidation pass whenever any of its articles is within the 30-day eligibility floor — its original aggregate holds an eligible article and so becomes a re-cluster candidate. - Stories fully past the eligibility horizon behave as before #71: fresh singleton, TTL clears it within a week. ## Impact At the default cap the incremental prompt drops from ~30–47k to ~5–8k tokens per run (5–8× vs post-TTL steady state) and the matching haystack shrinks accordingly. ## Docs `docs/config.md` gains the `YUNJIN_MAX_PROMPT_TOPICS` row in the LLM aggregation table.

agent-pi added 2 commits

2026-10-11 01:18:25 +00:00

✨ Cap incremental EXISTING TOPICS to the most recently updated 6c1cf49725

📝 Document YUNJIN_MAX_PROMPT_TOPICS

/ gitleaks (pull_request) Successful in 17s

Details

/ pr-review (pull_request) Successful in 1m13s

Details

/ checks (pull_request) Successful in 30m41s

Details

/ publish-container (pull_request) Has been skipped

Details

/ deploy-docs (pull_request) Has been skipped

Details

/ container (pull_request) Successful in 1m40s

Details

e1f649b77b

forgejo-actions reviewed

2026-10-11 01:19:32 +00:00

forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## WuMing Found **3** issue(s). See inline comments below.

src/yunjin/services/llm_aggregator.py

					
				@ -591,0 +601,4 @@

				        # already ordered most-recently-updated first, so slicing keeps the

				        # freshest candidates. Consolidation passes are bounded separately

				        # by their input articles and are not affected.

				        prompt_aggregates = user_aggregates[: _max_prompt_topics()]

code [MEDIUM]

The truncation is completely silent: there is no log line or metric when len(user_aggregates) > _max_prompt_topics(), so nothing in the logs ever indicates that the matching haystack was cut. Because capped-out aggregates still exist in the DB, articles about those stories will be turned into brand-new (duplicate) topics by the incremental pass, and an operator lowering YUNJIN_MAX_PROMPT_TOPICS (e.g. to 10) to fix token cost would see match-rate regressions with no diagnostic. Add an explicit signal, e.g. if len(prompt_aggregates) < len(user_aggregates): logger.debug(f"User {user.id}: capped EXISTING TOPICS {len(user_aggregates)} -> {len(prompt_aggregates)}") (or a counter metric), so the effect of the cap is observable.

**code** [MEDIUM] The truncation is completely silent: there is no log line or metric when `len(user_aggregates) > _max_prompt_topics()`, so nothing in the logs ever indicates that the matching haystack was cut. Because capped-out aggregates still exist in the DB, articles about those stories will be turned into brand-new (duplicate) topics by the incremental pass, and an operator lowering `YUNJIN_MAX_PROMPT_TOPICS` (e.g. to 10) to fix token cost would see match-rate regressions with no diagnostic. Add an explicit signal, e.g. `if len(prompt_aggregates) < len(user_aggregates): logger.debug(f"User {user.id}: capped EXISTING TOPICS {len(user_aggregates)} -> {len(prompt_aggregates)}")` (or a counter metric), so the effect of the cap is observable.

marvin8 marked this conversation as resolved

tests/test_aggregator.py

					
				@ -1470,6 +1471,81 @@ class TestEligibility:

				        assert _eligibility_days() == 30

				class TestExistingTopicsCap:

code [LOW]

Coverage gaps for the PR's central claims: (1) nothing guards the statement that consolidation passes are untouched — the only prompt-affecting call site that changed is the incremental one, but since the same user_aggregates list is fetched in this method, a future refactor could easily cap the consolidation prompt too. Add a test asserting a consolidation run still sees all (uncapped) aggregates, e.g. by forcing last_consolidation_at past the interval and asserting an aggregate outside the cap is present in the prompt. (2) There is no boundary test: exactly N aggregates (no truncation) and the default cap of 100 with >100 aggregates are untested.

**code** [LOW] Coverage gaps for the PR's central claims: (1) nothing guards the statement that consolidation passes are untouched — the only prompt-affecting call site that changed is the incremental one, but since the same `user_aggregates` list is fetched in this method, a future refactor could easily cap the consolidation prompt too. Add a test asserting a consolidation run still sees all (uncapped) aggregates, e.g. by forcing `last_consolidation_at` past the interval and asserting an aggregate outside the cap is present in the prompt. (2) There is no boundary test: exactly `N` aggregates (no truncation) and the default cap of 100 with >100 aggregates are untested.

tests/test_aggregator.py Outdated

					
				@ -1473,0 +1508,4 @@

				    def test_incremental_run_caps_existing_topics_to_most_recent(self, db_conn, monkeypatch):

				        """Test an over-cap prompt keeps only the most recently updated aggregates."""

				        for index in range(5):

				            self._make_aggregate(db_conn, f"Oldest {index}", updated_minutes_ago=100 - index)

code [LOW]

This test does not actually prove the requirement 'most recently updated' survives the cap. The aggregates are both created and back-dated in the same order (Oldest 4 is created last and has the newest updated_at), so the assertions pass for any ordering that is effectively id/created_at DESC as well as updated_at DESC — i.e. the test would not catch a regression where get_all_aggregates stops ordering by updated_at. Make the creation order conflict with the update order (e.g. create the aggregate with updated_minutes_ago=96 first and the one with updated_minutes_ago=100 last, or use a shuffled ordering) so only a genuine updated_at DESC slice yields the asserted names.

**code** [LOW] This test does not actually prove the requirement 'most recently updated' survives the cap. The aggregates are both created and back-dated in the same order (`Oldest 4` is created last *and* has the newest `updated_at`), so the assertions pass for any ordering that is effectively id/created_at DESC as well as updated_at DESC — i.e. the test would not catch a regression where `get_all_aggregates` stops ordering by `updated_at`. Make the creation order conflict with the update order (e.g. create the aggregate with `updated_minutes_ago=96` first and the one with `updated_minutes_ago=100` last, or use a shuffled ordering) so only a genuine `updated_at DESC` slice yields the asserted names.

agent-pi commented

2026-10-11 01:55:36 +00:00

src/yunjin/services/llm_aggregator.py

The truncation is completely silent: there is no log line or metric when…

✅ Fixed in commit f416c0c — the incremental path now logs User {id}: capped EXISTING TOPICS {n} -> {m} at DEBUG when truncation occurs, pinned by a caplog test asserting the 5 -> 3 record.

[`src/yunjin/services/llm_aggregator.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/80#issuecomment-4817) > The truncation is completely silent: there is no log line or metric when… ✅ Fixed in commit `f416c0c` — the incremental path now logs `User {id}: capped EXISTING TOPICS {n} -> {m}` at DEBUG when truncation occurs, pinned by a caplog test asserting the `5 -> 3` record.

agent-pi commented

2026-10-11 01:55:41 +00:00

tests/test_aggregator.py

This test does not actually prove the requirement 'most recently updated' survives the cap…

✅ Fixed in commit f416c0c — creation order now conflicts with update recency (ids ascend as Second, Fourth, Newest, Oldest, Third with cap 3), so an id- or created_at-ordered slice fails the assertions. RED-verified against a simulated wrong-end slice; the correct implementation passes.

[`tests/test_aggregator.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/80#issuecomment-4818) > This test does not actually prove the requirement 'most recently updated' survives the cap… ✅ Fixed in commit `f416c0c` — creation order now conflicts with update recency (ids ascend as Second, Fourth, Newest, Oldest, Third with cap 3), so an id- or created_at-ordered slice fails the assertions. RED-verified against a simulated wrong-end slice; the correct implementation passes.

agent-pi commented

2026-10-11 01:55:47 +00:00

tests/test_aggregator.py

Coverage gaps for the PR's central claims: (1) nothing guards the statement that consolidation passes are untouched…

✅ Both added in commit f416c0c. (1) A guard test proves a due consolidation pass lists an aggregate the cap would exclude (an old aggregate holding the window article appears as a candidate with cap=3 and 5 newer aggregates present). One precision on the suggested shape: consolidation never sent all aggregates — its EXISTING TOPICS are the candidates holding input articles — so the test pins the real invariant (candidate selection bypasses the cap). (2) Boundary coverage: exactly-at-cap (3/3, nothing cut) and the default 100 with 101 aggregates (newest 100 kept, oldest cut) are both tested end-to-end.

[`tests/test_aggregator.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/80#issuecomment-4819) > Coverage gaps for the PR's central claims: (1) nothing guards the statement that consolidation passes are untouched… ✅ Both added in commit `f416c0c`. (1) A guard test proves a due consolidation pass lists an aggregate the cap would exclude (an old aggregate holding the window article appears as a candidate with cap=3 and 5 newer aggregates present). One precision on the suggested shape: consolidation never sent *all* aggregates — its EXISTING TOPICS are the candidates holding input articles — so the test pins the real invariant (candidate selection bypasses the cap). (2) Boundary coverage: exactly-at-cap (3/3, nothing cut) and the default 100 with 101 aggregates (newest 100 kept, oldest cut) are both tested end-to-end.

agent-pi added 1 commit

2026-10-11 02:18:47 +00:00

✅ Address WuMing review: cap log signal, discriminating tests

/ gitleaks (pull_request) Successful in 31s

Details

/ pr-review (pull_request) Successful in 1m23s

Details

/ checks (pull_request) Successful in 29m50s

Details

/ publish-container (pull_request) Has been skipped

Details

/ deploy-docs (pull_request) Has been skipped