Aggregation consolidation: stable names, membership eligibility, nightly re-cluster #79
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!79
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-71-aggregation-consolidation"
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?
Closes #71
Ongoing stories stop spawning a new aggregate every day. Four linked changes, in dependency order:
aggregatesgains anamecolumn (alembica88dd3534780). The migration backfillsnamefrom each summary's first line (trimmed) and rewritessummaryto the text after it, so pre-existing rows stop printing the title twice now that the reader titles from the column. The aggregator writes name and overview separately; updates rewrite only the overview, so a group keeps its name. The reader titles groups from the column (Untitled groupfor blank legacy names).max(updated_at)watermark is gone. Eligible = subscribed-feed article with no aggregate membership andcreated_atwithinYUNJIN_ARTICLE_ELIGIBILITY_DAYS(default 30, inclusive). Stranded articles are retried every run, and TTL expiry releases articles for free because the cascade delete drops the membership.YUNJIN_CONSOLIDATION_INTERVAL_HOURS, default 24, tracked per user inlast_consolidation_at) the trailingYUNJIN_CONSOLIDATION_WINDOW_HOURS(default 72) plus all eligible articles are re-clustered against the aggregates they touch, instead of the incremental call. The LLM call runs first: API or parse failure mutates nothing and does not stamp the timestamp, so the next due run retries. On success the pass unlinks only the current user's memberships (a shared article may sit in another user's group), applies the topics, deletes emptied aggregates, and writes the timestamp.Documentation: the three new
YUNJIN_*variables are indocs/config.md, and the worker-pipeline description notes the periodic consolidation stage.Verification: full
uv run noxgreen (ty, ruff, complexipy, pysentry, pytest on 3.12/3.13/3.14, deptry, zaojun); 679 tests pass. One fresh-context review pass; all findings addressed.WuMing
Found 8 issue(s). See inline comments below.
@ -13,6 +13,9 @@ Aggregation needs access to an **OpenAI-compatible chat completions API**| `YUNJIN_LLM_API_KEY` | *none* — **required** for aggregation | API key for the chat completions endpoint. Aggregation fails fast with a clear error when it is unset. || `YUNJIN_LLM_BASE_URL` | `https://openrouter.ai/api/v1` | Base URL of the OpenAI-compatible API. || `YUNJIN_LLM_MODEL` | `deepseek/deepseek-v4.1-flash` | Model used to group articles and write aggregate titles/overviews. || `YUNJIN_ARTICLE_ELIGIBILITY_DAYS` | `30` | Age floor (days) for aggregation eligibility: an article is only sent to the LLM if it was created within this window and belongs to none of the user's aggregates. Invalid or below-1 values fall back to the default. |docs [MEDIUM]
"Age floor" is inaccurate and will mislead readers: a floor implies a minimum age, but the variable is a maximum age/recency window — articles whose
created_atis older thanYUNJIN_ARTICLE_ELIGIBILITY_DAYSare excluded from the LLM call. Reword to something like "Maximum age (days) for aggregation eligibility" so the direction of the bound is unambiguous.docs [LOW]
The eligibility conditions are incomplete. Per the implementation, an article is eligible only if it comes from a subscribed feed, has no aggregate membership, and its
created_atfalls within the window — and the boundary is inclusive. The sentence omits the subscribed-feed requirement and the inclusive boundary, so operators cannot tell whether a borderline 30-day-old article from an unsubscribed feed is sent. Add both conditions, e.g. "…only sent to the LLM if it belongs to a subscribed feed, is not already in one of the user's aggregates, and was created within this many days (inclusive)."@ -298,2 +309,2 @@agg = agg_db.create_aggregate(db_conn, user_id=1, summary="Existing Topic\n\nAlready grouped.")for article in [a1, a2]:"""When every article is already a member of an aggregate, no LLM call is made."""self._mark_consolidation_done(db_conn)tests [LOW]
self._mark_consolidation_done(db_conn)was inserted as the first statement, so the following string literal is no longer the function's docstring (it becomes a dead expression statement). Move the call below the docstring to preserve the test's documentation.tests [LOW]
self._mark_consolidation_done(db_conn)now precedes the docstring, turning the docstring into a no-op string literal rather than the function's__doc__. Place the call after the docstring.tests [LOW]
self._mark_consolidation_done(db_conn)was inserted before the docstring, so the docstring is no longer attached to the test function (it is just a discarded expression). Reorder so the docstring remains the first statement.tests [LOW]
self._mark_consolidation_done(db_conn)precedes the docstring here, demoting the docstring to an unused string literal. Move the helper call below the docstring.tests [LOW]
self._mark_consolidation_done(db_conn)is placed before the docstring, so the docstring is no longer the function's documentation. Put the call after the docstring.@ -225,3 +227,3 @@sample_articles: list[Article],) -> None:"""Test the title is stripped of padding and CRLF remnants (issue #42).""""""Test a padded name renders verbatim without crashing (issue #42 legacy)."""tests [MEDIUM]
The test's title assertion was dropped when the summary-splitting logic was replaced, but nothing was added in its place: the test now only checks the status code and that the overview text is present, so it no longer verifies the 'renders verbatim' behavior its new docstring claims. Add an assertion on the rendered heading (e.g. the verbatim padded name ' Padded Title ') so this test still exercises its stated purpose.
Review wave addressed — all 8 findings fixed in
cd51c48✅ Eligibility wording (4752 · 4753)
The row now reads "Maximum age (days) for aggregation eligibility" and names all three conditions — subscribed feed, no aggregate membership, created within this many days (inclusive). Both points were right: the old wording inverted the bound and omitted the subscribed-feed requirement.
✅ Verbatim title pin (4754)
Fair catch — that assertion was a ledgered deviation from the plan and it left the test claiming behaviour it no longer checked. The test now asserts the rendered heading
<h2> Padded Title </h2>, pinning that names render exactly as stored (the old reader-side whitespace stripping was blob-format scaffolding, not spec'd behaviour).✅ Docstrings restored (4755 · 4756 · 4757 · 4758 · 4759)
Correct, and my bug: the helper insertion for the consolidation pin landed above five docstrings, demoting each to a discarded string literal (which also dodges the project's pydocstyle rules). All five now have the docstring as the first statement. I checked every function in the file with an AST pass — those five were the only affected sites.
WuMing
Found 3 issue(s). See inline comments below.
@ -13,6 +13,9 @@ Aggregation needs access to an **OpenAI-compatible chat completions API**| `YUNJIN_LLM_API_KEY` | *none* — **required** for aggregation | API key for the chat completions endpoint. Aggregation fails fast with a clear error when it is unset. || `YUNJIN_LLM_BASE_URL` | `https://openrouter.ai/api/v1` | Base URL of the OpenAI-compatible API. || `YUNJIN_LLM_MODEL` | `deepseek/deepseek-v4.1-flash` | Model used to group articles and write aggregate titles/overviews. |docs [LOW]
This entry still says the model writes "aggregate titles", but this PR replaces the summary-derived title with a stable
namecolumn that the aggregator writes separately (later passes rewrite only the overview, and the reader titles groups from the column). Update the wording to "write aggregate names and overviews" so the entry matches the new data model.@ -14,2 +14,4 @@| `YUNJIN_LLM_BASE_URL` | `https://openrouter.ai/api/v1` | Base URL of the OpenAI-compatible API. || `YUNJIN_LLM_MODEL` | `deepseek/deepseek-v4.1-flash` | Model used to group articles and write aggregate titles/overviews. || `YUNJIN_ARTICLE_ELIGIBILITY_DAYS` | `30` | Maximum age (days) for aggregation eligibility: an article is sent to the LLM only if it belongs to a subscribed feed, is not already in one of the user's aggregates, and was created within this many days (inclusive). Invalid or below-1 values fall back to the default. || `YUNJIN_CONSOLIDATION_INTERVAL_HOURS` | `24` | Minimum hours between a user's consolidation passes, which re-cluster the trailing window in place. Invalid or below-1 values fall back to the default. |docs [LOW]
"which re-cluster the trailing window in place" is vague: "in place" is undefined, and it omits that eligible articles are re-clustered too (described only on line 18). Consider: "Minimum hours between a user's consolidation passes; a pass re-clusters the trailing
YUNJIN_CONSOLIDATION_WINDOW_HOURSwindow together with any eligible articles."@ -60,3 +63,3 @@|----------|---------|---------|| `PORT` | `8080` | Port the web entry point listens on (integer 1–65535). || `YUNJIN_WORKER_INTERVAL` | `3600` | Seconds between worker loop iterations (fetch → aggregate → extract-tags → extract-media). Positive integer, at most 9 digits. || `YUNJIN_WORKER_INTERVAL` | `3600` | Seconds between worker loop iterations (fetch → aggregate (with periodic consolidation) → extract-tags → extract-media). Positive integer, at most 9 digits. |docs [LOW]
The nested parentheses make this pipeline description harder to read, and burying the new consolidation stage inside them makes it easy to miss. Consider flattening it, e.g. "fetch → aggregate → periodic consolidation → extract-tags → extract-media".
New commits pushed, approval review dismissed automatically according to repository settings
WuMing
Found 2 issue(s). See inline comments below.
@ -14,2 +14,3 @@| `YUNJIN_LLM_BASE_URL` | `https://openrouter.ai/api/v1` | Base URL of the OpenAI-compatible API. || `YUNJIN_LLM_MODEL` | `deepseek/deepseek-v4.1-flash` | Model used to group articles and write aggregate titles/overviews. || `YUNJIN_LLM_MODEL` | `deepseek/deepseek-v4.1-flash` | Model used to group articles and write aggregate names and overviews. || `YUNJIN_ARTICLE_ELIGIBILITY_DAYS` | `30` | Maximum age (days) for aggregation eligibility: an article is sent to the LLM only if it belongs to a subscribed feed, is not already in one of the user's aggregates, and was created within this many days (inclusive). Invalid or below-1 values fall back to the default. |docs [MEDIUM]
This row states the condition absolutely — an article is sent to the LLM "only if" it is not already in one of the user's aggregates — but the consolidation rows below (lines 17–18) say a pass re-clusters the trailing window together with any eligible articles. Articles already in the user's aggregates are therefore re-sent to the LLM whenever they fall inside the consolidation window, so the "only if" wording is inaccurate and contradicts the rows immediately following it. Fix by scoping the claim to the incremental path (e.g. "in the incremental pass, an article is sent to the LLM only if ...") or by adding a caveat such as "(during a consolidation pass the trailing window is also re-clustered, including articles already in the user's aggregates)".
@ -463,0 +467,4 @@db_conn,user_id=_test_user_id(db_conn),name="Group Name",summary="Group Name\n\nA long overview paragraph.",tests [LOW]
This test exists to verify that reader cards take the group title from the new
namecolumn rather than from the summary text, but the setup still embeds the title in the summary ("Group Name\n\nA long overview paragraph."). Since the summary's first line is also 'Group Name', the assertion cannot distinguish the new behavior from a regression back to summary-derived titles, so it passes either way. Update the summary to only the overview (e.g."A long overview paragraph.") as the sibling tests at lines 194, 233, and 397 already do, so the assertion actually exercises thenamecolumn.docs/config.mdline 16✅ Fixed in commit
1ebec46— the "only if" claim is now scoped to the incremental pass, with a follow-on sentence noting that a consolidation pass also sends the articles in its trailing window, including ones already in aggregates (verified against_run_consolidation).tests/test_web.pyline 470✅ Fixed in commit
1ebec46— the summary is now the overview only ("A long overview paragraph."), matching the sibling tests. RED-verified: with a simulated regression to summary-derived titles the strengthened test fails where the old one passed vacuously.