**COLLECTING BRANCH** Per-user data ownership: subscriptions, aggregates, OPML #66
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!66
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-65-data-ownership"
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?
Implements Effort 2 (data ownership) of the Phase 6 multi-user effort.
Refs #65
Refs #25 (non-closing — only the eventual final PR into
maincarriesCloses)Base branch:
feat/multiuser(collecting branch — this PR stacks on it; it is NOT based onmain).What this does
feedstable plusfeed_subscriptions(user_id, feed_id): duplicate subscriptions fetch/store once; the last subscriber out triggers orphan-feed GC (FK cascades remove articles, media, tag links, read state).aggregates.user_id: fully independent per-user topic clusters; cross-user aggregate ids 404 on view and mark routes.add-feed/list-feedsgain--user(defaultdefault);docs/cli.mdanddocs/reader.mdupdated in the same PR.20261003_data_ownership: seedsdefaultwhen absent (worker-only installs), backfills all feeds and aggregates to user 1, rebuildsaggregateswithuser_id NOT NULL; downgrade reverses.Verification
uv run nox9/9 sessions (zaojun under--compat-ok).01551c3atomicity,821d00adocs/annotation hygiene).Note for review: the nox
pytest-3.12/13/14sessions collect 0 tests viatryke test(pre-existingnoxfile.pybehaviour, unchanged here) — real cross-interpreter evidence is the direct pytest runs.WuMing
Found 5 issue(s). See inline comments below.
@ -23,3 +23,2 @@deepseek_key: ${{ secrets.wuming_deepseek_key }}agents: code:deepseek-v4-pro,config:deepseek-v4-flash,docs:deepseek-v4-flash,shell:deepseek-v4-flash,security:deepseek-v4-pro,tests:deepseek-v4-flashmax_diff_lines: "2000"agents: code:deepseek-flash,config:deepseek-flash,docs:deepseek-flash,shell:deepseek-flash,security:deepseek-flash,tests:deepseek-flashconfig [MEDIUM]
All review agents, including
securityandcode, were collapsed todeepseek-flash. Previouslysecurityandcodeuseddeepseek-v4-pro, the stronger model for those roles; downgrading the security/code reviewers to the cheaper flash model weakens exactly the checks this pipeline exists to perform. Also confirmdeepseek-flashis a valid model id in the zaojun registry (the other entries previously carried av4-prefix, e.g.deepseek-v4-flash), otherwise the review job will fail or silently fall back. If the downgrade is intentional for cost reasons, keepsecurity:deepseek-v4-proand document the change.@ -24,2 +24,2 @@agents: code:deepseek-v4-pro,config:deepseek-v4-flash,docs:deepseek-v4-flash,shell:deepseek-v4-flash,security:deepseek-v4-pro,tests:deepseek-v4-flashmax_diff_lines: "2000"agents: code:deepseek-flash,config:deepseek-flash,docs:deepseek-flash,shell:deepseek-flash,security:deepseek-flash,tests:deepseek-flashmax_diff_lines: "5000"config [LOW]
max_diff_linesis raised from 2000 to 5000. This is the size of the largest diff handed to the review agents; tripling it increases the chance the reviewer runs out of context and produces shallow or skipped findings, and it increases token spend. Unless the previous limit was rejecting legitimate diffs, keep 2000 (or split large PRs); if 5000 is required, note that reviewers on large diffs will be less thorough.@ -64,10 +67,28 @@ yunjin add-feed URL --title "Feed Title" [--description "..."] [--interval MINUT| `--title` | *required* | Display name for the feed. |docs [MEDIUM]
The preceding text (lines 56–59) now states that feed rows are shared between accounts and that subscribing a second account to the same URL reuses the existing feed, but the option table still describes
--titlesimply as "Display name for the feed." With a shared feed row it is unclear what this flag does on reuse: does it rename the feed for every other account already subscribed to it, or is it ignored when the feed already exists? The same ambiguity applies to--intervalon line 69 (a per-feed refresh interval cannot be changed by a later subscriber without affecting everyone). Add an explicit note, e.g. "When the feed already exists,--title/--intervalapply only if supplied by the first subscriber; later subscriptions reuse the existing feed metadata," or state whatever the actual behaviour is.@ -96,0 +96,4 @@[[tool.zaojun.license-ignore]]package = "waitress"reason = "Uses Zope Public License (ZPL) Version 2.1 which is close-ish to MIT, so ok"config [LOW]
A license exception is added to bypass
check-licensesforwaitress. The stated justification is inaccurate: Zope Public License 2.1 is not 'close-ish to MIT' — it is a copyleft-adjacent, GPL-compatible license with notice/redistribution obligations, distinct from MIT/BSD. Since waitress is a runtime dependency, verify the exception is actually acceptable for the project's distribution terms (and record the real rationale/approval) rather than relying on an incorrect description of the license.@ -211,3 +214,4 @@aggregate = agg_db.create_aggregate(db_conn, user_id=_test_user_id(db_conn), summary="")response = auth_client.get(f"/aggregate/{aggregate.id}")assert response.status_code == 200tests [MEDIUM]
test_unsubscribe_feedonly asserts the redirect chain resolves to 200; it never verifies the effect of the route. A no-op handler that still redirects to /feeds would pass this test. Add a post-condition assertion (e.g.subscriptions_db.user_subscribes(db_conn, _test_user_id(db_conn), feed.id) is False, or that the feed row is gone for the last subscriber) so the unsubscribe behavior itself is exercised.WuMing
Found 3 issue(s). See inline comments below.
@ -54,3 +54,3 @@### `add-feed`Add a new RSS/Atom feed.Subscribe an account to a new RSS/Atom feed, creating the feed row when itdocs [LOW]
"Subscribe an account to a new RSS/Atom feed" is misleading, since the very next sentence describes subscribing to a feed that already exists (the row is reused). Suggest dropping "new": "Subscribe an account to an RSS/Atom feed, creating the feed row when it does not exist yet."
@ -67,0 +72,4 @@| `--user` | `default` | Account that owns the subscription. |An unknown `--user` name logs an error and exits with code 1 without creatinga feed. If the account already subscribes to the URL, the command logs andocs [MEDIUM]
This sentence contradicts itself and the surrounding semantics: a duplicate subscription "logs an error but exits successfully" (i.e. exit code 0), while the preceding sentence says an unknown
--user"logs an error and exits with code 1". Readers writing scripts or CI checks cannot tell from the docs whether a duplicate is a failure. Please state the exact exit code explicitly (e.g. "...prints a notice that the account already subscribes and exits 0") and reword "logs an error" to "logs a notice/warning" so the two cases are clearly distinguishable. Confirm the wording matches the actual implementation.@ -541,3 +596,4 @@agg_id = self._resolve_topic_aggregate_id(topic, user_id)# Link articles to this aggregatefor article_id in topic.article_ids:security [MEDIUM]
A03/A01: These article ids come straight from the untrusted LLM response (
topic.article_ids) and are linked with no check that each id belongs to an article in one of the user's subscribed feeds. Article titles/content from feeds are attacker-influenced data fed into the prompt, so a prompt-injection payload in a malicious feed could name another user's (sequential, guessable) article id and have it linked into this user's aggregate, which the reader then renders for the attacker. Aggregate ids are ownership-checked in_resolve_topic_aggregate_id, but article ids are not; validate everyarticle_idagainstarticlesjoined to the user'sfeed_subscriptions(or scope the resulting list to the user's feeds) before callingadd_article_to_aggregate..forgejo/workflows/ci.yml🔴 Declined — this is the repository owner's own deliberate change in this PR (commit
4a2457d): the cap was raised precisely so that large legitimate diffs (including this one) get reviewed instead of being skipped. The thoroughness-vs-coverage trade-off at 5000 lines is accepted by the operator..forgejo/workflows/ci.yml🔴 Declined — same commit (
4a2457d), the operator's deliberate cost choice, made with knowledge of the previous v4-pro roles. On the factual question: thepr-reviewjob completed successfully on this PR and produced the very review wave this reply is part of, which operationally confirms thedeepseek-flashids resolve in this pipeline.pyproject.toml🔴 Declined — the exception and its reason string are the repository owner's own commit (
a78fe97); license acceptance for this project is the operator's decision. The characterization wording is theirs to adjust if they wish; no code change accompanies this comment.docs/cli.md✅ Fixed in commit
819cb98—docs/cli.mdnow states that the first subscriber's--title/--description/--intervaldefine the shared feed row and a later subscriber's values are ignored on reuse (matchingsubscribe_or_create's actual behaviour).tests/test_web.py✅ Fixed in commit
819cb98—test_unsubscribe_feednow asserts the post-conditions:user_subscribes(...) is Falsefor the session user AND the feed row is gone (sole-subscriber GC removed it).docs/cli.md✅ Fixed in commit
c851e8a— the duplicate-subscription path now logs a warning (logger.warning) anddocs/cli.mdstates it explicitly: "logs a warning and exits with code 0" (vs the unknown-user error at exit code 1). The wording now matches the implementation, and a new test (test_add_feed_already_subscribed_warns_and_exits_0) pins the warning level, message, exit-less return, and that no second feed row is created.docs/cli.md✅ Fixed in commit
c851e8a— the sentence now reads "Subscribe an account to an RSS/Atom feed, creating the feed row when it does not exist yet."src/yunjin/services/llm_aggregator.py✅ Fixed in commit
c851e8a—_apply_topicsnow takesallowed_article_ids(the set of article ids actually sent in this user's prompt: new articles foraggregate(), all subscribed-feed articles forrefresh()). Any topic article id outside the set — foreign-owned or hallucinated — is skipped with a warning and never linked, and the per-aggregate linked count reports only articles actually linked. Pinned bytest_apply_topics_ignores_article_ids_outside_users_feeds(a topic naming user 2's article cannot link it into user 1's aggregate).WuMing
Found 1 issue(s). See inline comments below.
@ -0,0 +20,4 @@def test_feed_subscriptions_table_shape(db_conn):columns = {row[1] for row in db_conn.execute("PRAGMA table_info(feed_subscriptions)")}assert {"id", "user_id", "feed_id", "created_at"} <= columnsunique_index_columns = [row[2] for row in db_conn.execute("PRAGMA index_list(feed_subscriptions)") if row[3]]tests [MEDIUM]
This assertion does not verify what its message claims.
PRAGMA index_list(feed_subscriptions)returns (seq, name, unique, origin, partial), sorow[2]is the booleanuniqueflag androw[3]isorigin(truthy for every index, 'u' for a UNIQUE constraint and 'c' for a plain CREATE INDEX). The list therefore just collects unique flags for any index, andassert unique_index_columnsonly proves that at least one index exists on the table — it would still pass if theUNIQUE(user_id, feed_id)constraint were dropped and onlyidx_feed_subscriptions_feed_idremained, giving false confidence in the constraint that makes subscribe() idempotent. Inspect the indexed columns instead, e.g. find the index whoseuniqueflag is 1 and assert{r[2] for r in PRAGMA index_info(<that name>)}covers{user_id, feed_id}, or queryPRAGMA index_listfor a unique index and then check its columns viaPRAGMA index_info.tests/test_migration_data_ownership.py✅ Fixed in commit
819d-series — the assertion now collects indexes whose unique flag is set and verifies viaPRAGMA index_infothat one of them indexes exactly (user_id, feed_id). Verified discriminating: it fails on a degraded schema with only the plain feed_id index and passes on the real one (check script output recorded in the PR timeline).Correction to my reply above: the fix commit is
5439ead(:white_check_mark: Assert the subscription UNIQUE constraint by indexed columns) — the hash written in the previous comment was wrong; this one is authoritative.WuMing
Found 1 issue(s). See inline comments below.
@ -94,2 +94,4 @@min-age = 7check-licenses = true[[tool.zaojun.license-ignore]]config [LOW]
A new
license-ignoreentry suppresses thecheck-licenses = truegate forwaitress(line 98), but the stated reason on line 99 ("close-ish to MIT, so ok") is informal and not a verifiable compatibility argument. Record waitress's actual SPDX identifier (ZPL-2.1) and the concrete basis for accepting it (it is a permissive, OSI-approved licence, not MIT), or resolve/allow the dependency through the normal licence policy rather than bypassing the check. Also note the version actually pinned in this file iswaitress~=3.0.2, so the ignore should be reviewed whenever it is next bumped.Wave 4 on
bcbf9e8— one re-raised finding, the rest already answered🔴 Duplicate — waitress licence-ignore rationale (
pyproject.toml)Same substance as
the wave-1 finding, already declined there: the exception and its reason string are the repository owner's own commit (a78fe97); the licence-acceptance decision and any rewording of that reason are theirs to make, not this branch's. (For what it's worth, the wave-4 wording — ZPL-2.1, permissive, OSI-approved — is a more accurate description than the original "close-ish to MIT"; the owner may want to lift it into the reason string.)Already answered in waves 1–3, no further action: diff cap 5000 · deepseek-flash agents · --title/--interval reuse · duplicate exit code · "new" wording · article-id scoping · UNIQUE assertion · unsubscribe post-condition.
WuMing
Found 4 issue(s). See inline comments below.
@ -0,0 +25,4 @@NOT NULL user_id (SQLite cannot ADD COLUMN ... NOT NULL), so everypre-ownership aggregate becomes owned by user 1."""op.execute("INSERT INTO users (username) SELECT 'default' WHERE NOT EXISTS (SELECT 1 FROM users WHERE id = 1)")security [LOW]
A05/A07: The migration seeds a guessable 'default' account with no password/credentials and no explicit disabled state. If any authentication path treats a NULL password_hash as acceptable, or if the setup flow that runs while a passwordless user exists is reachable unauthenticated, this becomes a default-credential entry point. Seed the account disabled or with a random unusable hash, and require explicit operator provisioning of credentials.
@ -64,10 +69,28 @@ yunjin add-feed URL --title "Feed Title" [--description "..."] [--interval MINUT| `--title` | *required* | Display name for the feed. |docs [LOW]
The prose at lines 59–61 states that for an existing feed a later subscriber's
--titleis ignored on reuse, yet this table still marks--titleas required. Readers subscribing to an already-known URL will be unsure whether they must supply (a useless) title. Consider softening the requirement note, e.g. "--title| required | Display name for the feed. Ignored when the feed already exists (the first subscriber's title is kept)."@ -52,2 +52,2 @@"ruff~=0.16.8","ty~=0.0.83","ruff~=0.16.10","ty~=0.0.84",config [MEDIUM]
tyis bumped to~=0.0.84here andv0.0.84in.pre-commit-config.yaml, butpylock.tomlhas no corresponding hunk forty(every other tool bumped in this PR — ruff 0.16.10, uv 0.12.22, prek 0.5.4 — is updated in all three places). The lock file is therefore stale and does not satisfy the new constraint; regenerate it (uv lock/ theuv-lock+uv-exportpre-commit hooks) and commit the updatedtyentry so the lock, pre-commit rev and project requirement agree.@ -95,1 +95,4 @@check-licenses = true[[tool.zaojun.license-ignore]]package = "waitress"config [LOW]
A new
[[tool.zaojun.license-ignore]]entry suppresses thecheck-licenses = truegate for a runtime dependency (waitress~=3.0.2) that this PR does not otherwise touch. Excluding a package from licence compliance checking is a policy decision, not a version-bump side effect — confirm the licence really is ZPL-2.1 and that the upstream metadata gap cannot be fixed, and consider record-keeping elsewhere (e.g. an allow-list reviewed by the operator) so the ignore is not silently inherited by futurewaitressbumps.docs/cli.md✅ Fixed in commit
f74f9f2— the table rows for--title,--description, and--intervalnow each carry "Ignored on reuse.", matching the prose above the table.pyproject.toml🔴 Not correct —
pylock.tomldoes pinty0.0.84:name = "ty"at pylock.toml:1453 with the 0.0.84 sdist and wheel set at :1456-1460, matchingpyproject.toml'sty~=0.0.84and.pre-commit-config.yaml'sv0.0.84. The lock, pre-commit rev and requirement already agree; no regeneration needed.pyproject.toml🔴 The rationale was reworded to exactly this standard in commit
a49c786(which this review ran on): it now records the SPDX identifier (ZPL-2.1), the licence character (permissive, OSI-approved, BSD-style notice obligations), the upstream metadata gap as the reason zaojun reports UNKNOWN, and explicit operator acceptance. Whether the upstream metadata gap gets fixed and any further record-keeping are the repository owner's policy decisions.alembic/versions/20261003_feed_subscriptions_and_aggregate_owner.py🔴 Declined — this is the documented bootstrap design, not a default-credential hole. Authentication never treats a NULL
password_hashas acceptable (login explicitly rejects passwordless users), and while any user lacks a credential the whole app redirects to/setup, whose handler re-verifies the gate condition against the database on every request; once any user is credentialed,/setupis permanently disabled. Seeding a random unusable hash would actively break this: the setup gate opens only while no user has a password, so it would lock operators out of first-run bootstrap on upgraded deployments. Operator provisioning exists viayunjin set-password(and the setup page itself). Account lifecycle hardening is deliberately Effort 3.WuMing
Found 4 issue(s). See inline comments below.
@ -47,0 +46,4 @@- Add a feed by URL or by site address (Yunjin discovers candidate feeds).Subscribing to a feed another account already has reuses the same feed —each account keeps its own subscription and aggregates.- **Unsubscribe** from a feed. Unsubscribing removes the feed and itsdocs [LOW]
"Unsubscribing removes the feed and its aggregates from your account" reads as though the shared feed row itself is deleted, which contradicts the shared-feed model stated two lines above (feeds are shared across accounts; only the subscription and aggregates are per account). Reword to say that unsubscribing removes your subscription and your aggregates, and that the shared feed row is deleted only when the last subscriber leaves.
@ -47,0 +48,4 @@each account keeps its own subscription and aggregates.- **Unsubscribe** from a feed. Unsubscribing removes the feed and itsaggregates from your account; when you are the last subscriber, the feedand its articles are removed for everyone.docs [LOW]
"the feed and its articles are removed for everyone" is misleading: being "the last subscriber" means no other account is subscribed, so "for everyone" implies an impact on other users that cannot exist. Say instead that the shared feed and its stored articles are deleted (or "removed entirely").
@ -267,6 +267,36 @@ def get_all_articles(conn: sqlite3.Connection, limit: int | None = None, offset:return [_article_from_row(row) for row in rows]def get_articles_for_feeds(conn: sqlite3.Connection, feed_ids: list[int]) -> list[Article]:tests [LOW]
New function
get_articles_for_feedshas no direct test: no test in the diff references it, it is only exercised indirectly through the aggregator. In particular the empty-list guard on line 281 is unreachable from its only caller (llm_aggregator._new_articles_for_user returns early when there are no subscriptions), so that branch is never covered. Add focused tests for the emptyfeed_idscase and the multi-feed IN-clause filtering.@ -109,2 +112,3 @@user = users.get_user_by_username(db_conn, "test_user")assert user is not Noneif user is None:user = users.create_user(db_conn, "test_user")tests [MEDIUM]
This helper used to validate fixture state with
assert user is not None; it now silently createstest_userin the shareddb_connfixture when missing. That both weakens the guard (a misconfigured fixture no longer fails) and mutates shared fixture state from a helper called by many tests without any teardown/restore. Restore the assertion (the sibling_other_user_idhelper at line 2125 still does exactly this), or create the user in a dedicated fixture so setup failures surface.docs/reader.md✅ Fixed in commit from this wave — the sentence now reads: "Unsubscribing removes your subscription and your aggregates that contained that feed's articles. The shared feed row itself is deleted only when the last subscriber leaves — along with its stored articles."
docs/reader.md✅ Fixed in the same commit — "removed for everyone" is gone; it now says the shared feed row is deleted only when the last subscriber leaves, along with its stored articles.
src/yunjin/db/articles.py✅ Fixed —
get_articles_for_feedsnow has direct tests: emptyfeed_idsreturns[](covering the previously unreachable guard), and multi-feed IN-clause filtering plus newest-first ordering are asserted. Seetests/test_db.py::TestArticles::test_get_articles_for_feeds_*.tests/test_web.py🔴 Declined — the get-or-create is deliberate and fixes a real bug from this PR's own Task 1:
test_build_aggregate_rowconsumessample_aggregatewithoutauth_client, sotest_userlegitimately did not exist yet and the hard assert failed on fixture ordering. Restoring the assert would re-break that path; the docstring documents the fallback. The alternative (a dedicatedtest_userfixture consumed by every caller) is a wider refactor we're happy to consider post-merge, but it is not a correctness gap now.