thalladaandClaude Fable 5.1
d0560afa1a
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A1rCLQeKBgnBo3oTgHuTMe
119 lines
13 KiB
Markdown
119 lines
13 KiB
Markdown
# Plan Review: Personalized Ranking, Embeddings, Facets, and Feedback (v6)
|
|
|
|
**Reviewed:** 2026-08-19
|
|
**Plan:** `docs/plans/2026-08-17-personalized-ranking-and-facets.md` (revision 6)
|
|
**Verdict:** **Close, but not yet safe to implement verbatim.** Revision 6 correctly repairs the churn query, makes the event layer authoritative in every mode, snapshots final feed attribution, adds attempt-level budget accounting, and closes the migration/publication consistency gaps from the previous review. No architectural rewrite remains. Four High-severity contracts still need to be made implementable: protected-feed classification is not actually based on feed identity, the shadow sub-limit does not reserve production capacity, concurrent ledger reservations have no specified SQLite atomicity mechanism, and the legacy churn fallback cannot implement the new observation-time/latest-value semantics. A final normative consistency pass is also still required.
|
|
|
|
## Critical
|
|
|
|
No remaining Critical finding. The ranking, observation-history, manifest-lifecycle, and publication designs are now internally sound in their primary paths.
|
|
|
|
## High
|
|
|
|
### H1. The provider-policy type prevents bypasses only after classification; its host matcher can misclassify a private feed
|
|
|
|
Section 25.1 defines `no_external_ai_feeds` as feed IDs or host substrings “matched exactly like `always_include_feeds`” and then gives `externally_processable` only an `Article` and `CurationConfig` (§25.1, §30). That existing matcher does not inspect the feed URL. In `src/curate/prefilter.rs:136-165`, numeric values match any source feed ID, but string values are searched as case-insensitive substrings of `article.url` and `article.canonical_url`. `SourceRef` contains `feed_id`, title, category, and kind, but no feed URL (`src/types.rs:62-70`).
|
|
|
|
That is insufficient for the stated threat model. A private Miniflux feed may live at `reader.internal/private.xml` while its entries link to public sites. Configuring `reader.internal` appears valid but does not protect those articles; `externally_processable` constructs the wrapper and the strong type then faithfully sends the protected data. Substring matching also has the wrong security properties for a deny policy: it can match unrelated hosts and does not define exact-host/subdomain behavior.
|
|
|
|
This is especially easy to miss in a deduplicated cluster whose best article URL is public but one secondary `SourceRef.feed_id` is protected. Numeric IDs can classify that case correctly, but the advertised host form cannot.
|
|
|
|
**Required amendment:** make the privacy configuration identity-safe before relying on the type-level guarantee. The safest V1 is a typed `no_external_ai_feed_ids: Vec<FeedId>` and a startup error for nonnumeric entries; Miniflux feed IDs are already present on every source and survive deduplication. If host policies must remain, separate them into explicitly named fields and pass actual Miniflux feed URL/site metadata into the policy gate. Parse URLs and compare normalized hosts (with a documented exact-host/subdomain rule), never arbitrary substrings of article URLs.
|
|
|
|
Add tests where:
|
|
|
|
1. a protected feed URL host differs from the linked article host;
|
|
2. the best source is public but a secondary source feed is protected;
|
|
3. a lookalike hostname does not accidentally match an unrelated protected hostname.
|
|
|
|
The full-run recording mock should use one of these adversarial classifications rather than only a numeric best-feed match.
|
|
|
|
### H2. A shadow sub-limit inside a shared ceiling does not preserve a production slice
|
|
|
|
Section 7.6 says all classes share the provider's `max_daily_usd`, while `shadow_max_daily_usd` merely places an additional cap on shadow (§7.6 lines 672-687). Section 24 then claims shadow “can never consume the production slice” (§24 line 1734). Those statements are not equivalent.
|
|
|
|
With the documented Voyage defaults, the provider ceiling is $0.25 and the shadow sub-limit is $0.20. A shadow invocation that spends $0.20 first leaves only $0.05 for a later production run. Production-first ordering protects calls only within one invocation; it does nothing across runs or across the UTC day. The same issue arises if a standalone non-publication command runs before the timer. The ledger accurately records the depletion, but it does not reserve capacity for the newspaper.
|
|
|
|
**Required amendment:** choose and state one of these contracts:
|
|
|
|
- If publication capacity is guaranteed, add a per-provider `production_reserve_daily_usd` (or explicit class allocations) and admit shadow/backfill only when `provider_total + estimate <= provider_max - production_reserve`. Define which operations may consume the reserve and what happens after the publication run completes.
|
|
- If the ceiling is only an account-wide runaway guard, remove “production slice” and state plainly that shadow is bounded but may reduce capacity available to later production.
|
|
|
|
The Phase A promise that shadowing does not affect the paper favors the first option. Add an order-sensitive test: spend shadow first, then prove the configured production reserve is still dispatchable. A mixed-class test inside one run, which §31.11b currently requires, does not cover this defect.
|
|
|
|
### H3. “Atomic check-and-reserve” needs an explicit SQLite serialization mechanism
|
|
|
|
The plan correctly requires checking and reserving before each concurrently spawned request, but it never defines the transaction primitive that makes the read-sum-insert sequence atomic. `buffer_unordered` can run sibling reservations concurrently inside one process. The process-wide `flock` serializes commands, not async tasks or SQLite connections within that command.
|
|
|
|
A naive SQLx transaction is deferred in SQLite: two tasks can both read the same pre-reservation total, both decide they fit, and then contend when writing. Depending on timing, that either admits estimates beyond the ceiling or produces `SQLITE_BUSY` at a point the plan currently treats like ordinary provider degradation. A unique key on `(request_id, attempt)` does not serialize different requests, and the spend index does not enforce a sum constraint.
|
|
|
|
**Required amendment:** specify one implementation-grade reservation path. Viable choices are:
|
|
|
|
- a provider-scoped in-process async mutex around a short `BEGIN IMMEDIATE` transaction that re-sums and inserts before commit; or
|
|
- a single reservation actor/connection that serializes all check-and-insert operations.
|
|
|
|
Because every provider-spending command holds the OS lock, an in-process mutex plus `BEGIN IMMEDIATE` is sufficient for the declared single-host deployment. Specify busy-timeout/retry behavior for this short transaction and keep external HTTP work outside it.
|
|
|
|
Add a barrier-based concurrency test that releases many reservation tasks simultaneously near the ceiling and asserts that the sum of admitted estimates never exceeds either the provider cap or the applicable class cap. Also assert that refusal occurs before the corresponding mock HTTP dispatch.
|
|
|
|
### H4. The legacy `scores` fallback cannot satisfy the churn rule unless its precedence and lifetime are bounded
|
|
|
|
Section 7.8 now gives `candidate_rankings` a correct latest-observation query over `runs.started_at`, but then says pre-`0002` dates fall back to `scores` in live/recurate (line 795). `scores` is exactly the mutable, nominal-date-keyed projection the section rejected: it has no observation timestamp and may contain multiple rows for an article across nominal dates. The plan gives no query, merge precedence, or retirement point for that fallback.
|
|
|
|
A straightforward union of low IDs recreates the v5 defect: one legacy low row can suppress an article despite a newer high `candidate_rankings` observation. Using `scores.run_date` as recency recreates the wrong-time-axis defect. Because new runs continue writing the compatibility projection, “pre-migration score” also cannot be inferred merely from the row's existence.
|
|
|
|
**Required amendment:** either delete the fallback—the database is effectively at cold start, so this is the cleanest option—or define it as a strictly temporary compatibility bridge:
|
|
|
|
- capture a migration timestamp/marker;
|
|
- consider legacy `scores` only for articles with no `candidate_rankings` observation at all;
|
|
- document the nominal-date approximation explicitly;
|
|
- disable the fallback after one `recent_rejection_lookback_days` interval from migration, so an unverifiable projection cannot suppress forever.
|
|
|
|
Do not let a projection row compete with an observed candidate row. Add tests for legacy-low → new-high, legacy-high → new-low, two legacy nominal dates for one article, and fallback expiry. Fidelity should continue to exclude these unverifiable rows.
|
|
|
|
## Medium
|
|
|
|
### M1. The normative worklist still contains v5 authorities and one lifecycle contradiction
|
|
|
|
Revision 6's core sections are clear, but the later implementation instructions still tell an agent to build several superseded forms:
|
|
|
|
- §23 line 1720 and §30 `src/server.rs` line 2065 say to capture distinct direct-feed IDs. The authoritative event schema requires the completed, versioned `feed_credits_json` after fallback.
|
|
- §25.1 line 1810 says protected feed affinity and `W_global` derive from `ratings` and `sources_json`. They must derive from latest `rating_events` and stored feed credits; using current sources reintroduces the temporal bug §7.9 removes.
|
|
- §30 `src/db.rs` lines 2007-2008 says to load/derive from ratings joined to current sources/facets. The normative source is latest rating events, with article/facet joins only for compatible local signals.
|
|
- §30 `src/db.rs` lists `bootstrap_profile_history()` but omits `bootstrap_observation_history()`.
|
|
- §30 `src/publish.rs` line 2069 says publication events commit with `replace_issue_articles`; the actual contract is one transaction containing `upsert_issue`, `replace_issue_articles`, and events.
|
|
- §30 `src/main.rs` line 2079 still releases and reacquires the lock and runs only the profile bootstrap, contradicting §24.1's same-file-descriptor rule and the two-bootstrap contract.
|
|
- The eligibility table says `explain` accepts only `ranking_fixed`/`final`, while the failure semantics and §31.8 say `explain --run-id` accepts `provisional` too.
|
|
|
|
These are execution instructions, not historical review tables, and several directly reintroduce bugs v6 says are fixed.
|
|
|
|
**Required amendment:** update §23, §25.1, §30, and the eligibility table so each has one authority. Add `bootstrap_observation_history` to both the startup sequence and migration concurrency test. Search the normative text for `distinct direct-feed`, `ratings and sources_json`, `load ratings`, `transactionally with replace_issue_articles`, and `release → ... re-acquire`; none should remain except in explicitly labelled historical discussion.
|
|
|
|
## Low
|
|
|
|
No separate Low-severity finding. The remaining cleanup belongs in the normative consistency pass above.
|
|
|
|
## Alternatives
|
|
|
|
### Prefer typed feed IDs for provider opt-out
|
|
|
|
Feed IDs are the least ambiguous privacy boundary in this codebase: every clustered source carries one, and the Miniflux account owns the mapping. A separate article-domain rule can be added later for public-domain policy, but it should not masquerade as feed identity.
|
|
|
|
### Retire legacy churn state at migration
|
|
|
|
Given the near-empty production history, accepting at most one week without legacy churn suppression is lower risk than maintaining a second, semantically weaker query. If temporary continuity is still desired, snapshot the legacy low set once with an explicit expiry instead of continuing to consult the mutable `scores` projection.
|
|
|
|
### Serialize ledger admission, parallelize HTTP only
|
|
|
|
Reservation transactions are tiny. Serialize those locally, commit, then allow HTTP attempts to proceed concurrently. This preserves throughput where it matters while making the monetary invariant easy to state and test.
|
|
|
|
## Open Questions
|
|
|
|
1. Is `no_external_ai_feeds` intended to identify Miniflux feed subscriptions, article domains, or both? If both, what are the exact matching semantics for each namespace?
|
|
2. Must Phase A/shadow work be incapable of reducing a later production run's available budget, or is `shadow_max_daily_usd` only an additional runaway cap?
|
|
3. Is preserving pre-migration churn suppression worth a temporary second semantic path, given the production store has approximately one issue of history?
|
|
|
|
## Bottom line
|
|
|
|
The main ranking and temporal architecture is ready. Amend H1-H4 before implementation because each concerns a guarantee the current schema/API description cannot actually enforce: privacy classification, production budget availability, atomic budget admission, and latest-observation churn semantics. M1 should be fixed in the same edit so §30 becomes a reliable implementation checklist rather than a source of regressions.
|