Skip to content

Add guarded full-content hydration for new RSS items - #326

Merged
WaylandYang merged 11 commits into
deeplethe:devfrom
J-i-K:hermes/rss-full-content-pr
Sep 5, 2026
Merged

Add guarded full-content hydration for new RSS items#326
WaylandYang merged 11 commits into
deeplethe:devfrom
J-i-K:hermes/rss-full-content-pr

Conversation

@J-i-K

@J-i-K J-i-K commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Opt-in full article acquisition for newly observed RSS items; new sources default to feed, with no activation backfill.
  • Shared guarded Content-reach HTTP fetching and shared Readability/htmd HTML extraction; encoding-aware decoding and Chinese-content acceptance.
  • Accepted Markdown uses normal document storage. Internal observations retain baseline/discovery identity and pending inputs; jobs own execution and documents own accepted content. Source locks, capacity limits, and deletion fences protect retry behavior.
  • RSS retry policy lives in its own store module. Entries without GUID/Atom ID or a usable HTTP(S) article link are silently skipped in both modes.
  • Removed the raw entry-diagnostics endpoint, client, and UI link. No new metadata object or troubleshooting UI.
  • Migration 0032_rss_full_content.sql; ADR 0023.
  • Integrated upstream dev through ffd0d3012f2cb5b4730fa395bb698311a700f0af using normal merges. Published head afe3a2e72095003859fdcb2a7e71c19935796709.

Validation

  • Backend/migration tree: 435 workspace tests passed with database access required; one live-network test ignored by default and passed when explicitly executed.
  • Fresh disposable PostgreSQL/pgvector database: 32 successful migrations and migration replay/schema contract passed. Existing shared fixture databases were not migrated.
  • Locked workspace/all-target Clippy and check, formatting and whitespace checks passed.
  • Frozen frontend install/build passed; final UI-only upstream merge revalidated with style guard, TypeScript/build, and five browser tests.
  • Real Chromium tests execute source creation, verify default feed and explicit full_new_items POST payloads, and confirm absence of raw entry diagnostics. Mutation controls detect wrong defaults and disconnected payloads.
  • Live acceptance exercises production Content-reach DNS/pinning, HTTPS redirect, response bounds, extraction and acceptance.

Boundaries

Remote CI and maintainer acceptance are separate gates. No production deployment, production migration, or downstream model-processing completion is claimed. The purge fence uses database wall-clock timestamps, not a proven monotonic-clock guarantee. The ADR records the design discussion retrospectively.

Co-Authored-By: Jay

Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>
Co-Authored-By: Jay
Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>

Co-Authored-By: Jay
Co-Authored-By: Jay
Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>
Co-Authored-By: Jay
Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>
@WaylandYang

Copy link
Copy Markdown
Contributor

Careful work — the fetch hardening especially (public-address check, resolve_to_addrs pinning against rebinding, hand-walked redirects), and thanks for merging dev in. Two questions before we take it, plus one thing blocking CI.

1. Should your extractor replace ours, not join it?

We already have parsers.rs::htmlscraper, main/article container, skip-list for nav and footers. Yours is Readability + htmd, and it is probably the better of the two. What we'd rather not ship is both: the same page through a url source and through RSS full content would come out with different bodies. Would you point the existing HTML path at the new extractor here — or say where the two cases genuinely differ?

2. What do documents and jobs not already carry?

Two tables and seven states is a lot of surface. Documents already have external_key identity and keep the old body as a version when content is replaced (decision 0008); jobs already carry attempts and the last error. The one thing we can see your ledger holding that they can't is an item that never became a document — baselines and terminal failures. Is that the reason, or is there more?

3. The migration number

After the merge the branch carries both 0022_deleting_is_an_event.sql and 0022_rss_full_content.sql. sqlx::migrate! refuses duplicate versions and CI checks the prefixes, so nothing will go green until this is 0027. That is the only thing standing between here and a full CI run — we'll approve the workflow as soon as it moves.

Separately, and ours to fix: the existing url source fetch has no SSRF guard at all. We'll open an issue to put all three fetch paths on one guarded client, built from yours.

Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>
Co-Authored-By: Jay
@J-i-K

J-i-K commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review. I agree with the direction: there should be one extraction path, and the ledger should retain only what the existing document and job models cannot represent.

  1. Shared extraction. Generic HTML and RSS article pages now use the same Readability + htmd extractor in utopia-ingest; the old scraper implementation is removed. Feed fragments still bypass Readability because they are already entry-scoped. The page path accepts a base URL where available, so relative-link resolution can differ without maintaining a second body extractor.

  2. One observation table. The two RSS tables and seven persisted states are reduced to one table with baseline, candidate, and no_source classifications. Activation lives on sources; jobs own execution, attempts, and errors; documents own accepted content and versions. The remaining inputs preserve acquisition eligibility after an item leaves the feed window. Accepted body inputs are cleared. Deletion history retains the identity released by purge so old backlog cannot recreate a purged document without a genuine later observation.

  3. Migration numbering. The migration is now 0028_rss_full_content.sql: 0027_entities_rewind_too.sql was already on dev when I synchronized. The fresh-database migration check runs twice and verifies the reduced schema.

The final review also caught two regressions, now covered by tests: ordinary source reappearance again restores soft-deleted documents for both unchanged and changed content, and poorer feed observations cannot erase retained candidate inputs or refresh their purge permission.

Local verification: 393 workspace tests passed, with database access required and no schema-test exclusion; formatting, workspace Clippy with warnings denied, and the frontend frozen install/build also passed. The HTTP-backed regression covers 201-item baselines, empty/failed feeds, stale responses, and feed-native hydration through processing-job creation. This is local evidence, not a claim that upstream CI or deployment has passed.

I added decision record 0022, explicitly noting that it follows the design discussion here rather than preceding implementation. I have retained my established GitHub signing identity and noreply address for privacy; please let me know if that prevents acceptance under the contribution policy.

@WaylandYang WaylandYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — all three points from last round are answered the way we hoped, and CI is green. Merging dev in surfaced one new problem, and a few things in the new code would hit Chinese-language pages first. Five before we can take it, then some we can argue about.

Before we take it

  1. Two SSRF guards. rss_full_content.rs still carries its own fetch_once / resolve_public_addresses / redirect_target / is_public_ip. Since #339, dev has http_fetch::get(url, Reach::Content, Limits { .. }), built from your first version. Fetch the article through it and keep only what is yours: content-type check, challenge-shell detection, quality check.
  2. The interstitial check rejects ordinary pages. html::looks_like_challenge_html runs on the raw page: any <form> plus the word "password", or one <input type="email">, fails the whole page. Newsletter boxes and login modals are in most pages' DOM, and this path now serves every HTML upload and url source. Run the check on what Readability extracted — your server-side looks_like_challenge_shell already does — and drop the raw-page one.
  3. Linked articles must be UTF-8. read_page_response uses String::from_utf8; a GBK page fails and retries to terminal. The upload path sniffs with chardetng (parsers::plain_text) — decode through the same helper.
  4. "Substantive" counts words by whitespace. A Chinese paragraph is one word to split_whitespace, so only the 1,200-character bound applies and a 600-character news item is terminal. Count characters, or count CJK characters as words.
  5. New RSS sources default to full mode (useState("full_new_items") in the create form). The PR says opt-in; default to feed.

Worth changing, open to argument

  • sources::list computes eight fields for every source through a CTE over ENTRY_SELECT, on every Library load, for bases with no RSS. One nullable rss_full_content object, computed only for rss sources, keeps the list query what it was.
  • jobs::requeue_failed now knows hydrate_rss_entry's tables. The requirement (same source lock, same 25-job capacity) is right; put it in a requeue_failed_hydrations in the RSS module, called from the same route.
  • Decision record 0022: the status line and the sentence about the issue-before-record sequence will be stale on merge — write Implemented in #326, same for the README row.
  • Items with neither a guid nor an http(s) link are now silently skipped, in feed mode too. Probably right; say so in the PR text.
  • The "Entry diagnostics" link opens raw JSON; fine as a placeholder. HTML now comes out as Markdown for every source, link destinations included; acceptable, naming it so it is a decision.

What is right

One extractor; one observation table with state projected rather than stored twice, and a test that pins it; clear fences on observation time, generation and the source lock; eleven database tests on the cases that matter. Thank you for the care.

On the name

Yes. The DCO asks for someone who answers for the submission, and a GitHub identity with history and three rounds of discussion under it is that. The "real name" line in CONTRIBUTING.md is old kernel wording; we will change it to what we mean — a consistent identity you answer to, no anonymous or throwaway contributions. That change is ours, separately.

Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>
Co-Authored-By: Jay
Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>
Co-Authored-By: Jay
Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>
Co-Authored-By: Jay
@J-i-K

J-i-K commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the detailed review. Updated in cf7ad2447558d44f48e76dd586a558fa0491df84, including upstream dev through ae219478346affe85294d116844a9b742293d914 via normal merges.

The five blockers are addressed:

  • RSS article fetching now uses the shared guarded HTTP client with Reach::Content, retaining RSS request/response limits and address restrictions.
  • Interstitial detection evaluates extracted content rather than rejecting ordinary newsletter/login chrome. Regression coverage includes quoted challenge phrases and the same article split into normal short paragraphs, while retaining shell negatives.
  • Linked articles use the existing encoding-aware ingest decoder, including actual GBK-byte coverage.
  • Substantive-content checks handle Chinese text without relying on whitespace-separated English words.
  • New RSS sources default to feed; full-content mode remains explicitly opt-in. Chromium tests execute the actual form and verify both submitted payloads.

RSS-specific failed-requeue policy is now in the RSS store module, with existing API authorization, filters, counts, source locking, capacity limits, and purge protections preserved. The ADR status is simplified, and silent skipping of entries without a GUID/Atom ID or usable HTTP(S) article link is documented for both modes.

We removed the raw entry-diagnostics link and endpoint rather than exposing JSON as a troubleshooting UI. Accepted article content still uses normal document storage; the internal observation ledger is not a second article-content field. The proposed source-list metadata object/query restructuring is intentionally not included in this update.

Upstream's new migrations and ADRs are retained: RSS is now migration 0032 and ADR 0023.

Validation on the integrated tree: 435 workspace tests passed with database access required; the one default-ignored live-network test passed when run explicitly. Fresh-database migration replay, locked workspace Clippy/check, formatting, frozen frontend install/build, and five browser tests passed. The live test exercised the production Content-reach path, public DNS/pinning, HTTPS redirect, response bounds, extraction, and acceptance. These are local integration results, not a claim of remote CI or maintainer approval. No production deployment was performed.

Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>
Co-Authored-By: Jay
@J-i-K

J-i-K commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: upstream advanced once more during publication with the input-styling change (#398). That UI-only commit is now integrated through ffd0d30 in final head afe3a2e72095003859fdcb2a7e71c19935796709. The final frontend style/build gates and all five browser tests passed again; backend and migrations are unchanged from the validated integration. Both PR descriptions now reflect the current migration, ADR, scope, and evidence.

J-i-K and others added 2 commits September 5, 2026 20:34
Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com>
Co-Authored-By: Jay

@WaylandYang WaylandYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Third round checked against the five: one guard (http_fetch::get with Reach::Content, nothing of its own left), the interstitial check on the extracted Markdown with the raw-page check only as a sub-200-character fallback, decode_text with the GBK case, CJK counted as words, feed as the default. Requeue lives in the RSS module; the record and the README row say #326. CI is green on the merged head. Taking it.

Two small things we will fix on our side rather than ask for another round: text-caption is not one of the five sizes (it should be text-fine), and the two mode hints read as documentation rather than UI copy. The list-query cost stays a follow-up, as you said.

Thank you for the three rounds.

@WaylandYang
WaylandYang merged commit 28db351 into deeplethe:dev Sep 5, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants