Add guarded full-content hydration for new RSS items - #326
Conversation
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>
|
Careful work — the fetch hardening especially (public-address check, 1. Should your extractor replace ours, not join it? We already have 2. What do Two tables and seven states is a lot of surface. Documents already have 3. The migration number After the merge the branch carries both Separately, and ours to fix: the existing |
Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com> Co-Authored-By: Jay
|
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.
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
left a comment
There was a problem hiding this comment.
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
- Two SSRF guards.
rss_full_content.rsstill carries its ownfetch_once/resolve_public_addresses/redirect_target/is_public_ip. Since #339,devhashttp_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. - The interstitial check rejects ordinary pages.
html::looks_like_challenge_htmlruns 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 andurlsource. Run the check on what Readability extracted — your server-sidelooks_like_challenge_shellalready does — and drop the raw-page one. - Linked articles must be UTF-8.
read_page_responseusesString::from_utf8; a GBK page fails and retries to terminal. The upload path sniffs withchardetng(parsers::plain_text) — decode through the same helper. - "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. - New RSS sources default to full mode (
useState("full_new_items")in the create form). The PR says opt-in; default tofeed.
Worth changing, open to argument
sources::listcomputes eight fields for every source through a CTE overENTRY_SELECT, on every Library load, for bases with no RSS. One nullablerss_full_contentobject, computed only forrsssources, keeps the list query what it was.jobs::requeue_failednow knowshydrate_rss_entry's tables. The requirement (same source lock, same 25-job capacity) is right; put it in arequeue_failed_hydrationsin 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
feedmode 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
|
Thank you for the detailed review. Updated in The five blockers are addressed:
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 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
|
Follow-up: upstream advanced once more during publication with the input-styling change (#398). That UI-only commit is now integrated through |
Signed-off-by: J-i-K <5702515+J-i-K@users.noreply.github.com> Co-Authored-By: Jay
WaylandYang
left a comment
There was a problem hiding this comment.
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.
Summary
feed, with no activation backfill.0032_rss_full_content.sql; ADR 0023.devthroughffd0d3012f2cb5b4730fa395bb698311a700f0afusing normal merges. Published headafe3a2e72095003859fdcb2a7e71c19935796709.Validation
feedand explicitfull_new_itemsPOST payloads, and confirm absence of raw entry diagnostics. Mutation controls detect wrong defaults and disconnected payloads.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