Skip to content

H1 reslice: quarantine-safe primitive feed status - #42

Closed
Pigbibi wants to merge 2 commits into
mainfrom
feat/pert-upstream-feed-primitive-status-h1-reslice
Closed

H1 reslice: quarantine-safe primitive feed status#42
Pigbibi wants to merge 2 commits into
mainfrom
feat/pert-upstream-feed-primitive-status-h1-reslice

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Scope

Fresh reslice from origin/main@294e30b8eed36f070299b1dc01d34955fb262918; PR #41 is closed evidence and was not reused.

  • Add producer-owned PrimitiveRow and canonical pert.feed_primitives.v1 status.
  • Quarantined feeds are structurally required to carry zero accepted rows and do not enter per-feed/aggregate accepted-row digests.
  • Recognized zero-entry feeds produce a canonical quarantined status instead of a hard total exception; complete/eligible remain false.
  • Accepted/failed/quarantined rows, counts, digests, and errors are rebuilt from one snapshot.
  • Canonical feed ordering and JSON-safe integer bounds are strict.
  • Existing RSS/Atom CSV row fields and H0 defusedxml raw boundary remain in place.

Non-scope

No workflow, publish, QAR, weekly artifact, permissions, or automation changes. No legacy fallback.

Verification

  • uv sync --locked --extra test passed
  • uv run pytest -q — 128 passed
  • python3 -m compileall -q src scripts tests passed
  • git diff --check passed
  • ruff unavailable

W0-B remains deferred until this fresh H1 reslice merges and post-main CI succeeds.

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

🤖 Codex PR Review

🚫 Merge blocked: 1 serious issue(s) found in high-risk files

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in src/political_event_tracking_research/rss_source_fetch.py

parse_feed_items() now recognizes RSS only when root.tag == "rss". Any feed that still exposes ./channel/item entries but does not have that exact literal root tag will now fall through to the Atom branch, return zero rows, and be recorded as quarantined zero_entries instead of producing items. This is a silent data-loss regression because the previous parser keyed off the RSS item structure rather than the exact root tag. (line 128)

Suggestion: Keep the structural RSS detection (./channel/item) or explicitly validate both RSS and Atom roots before classifying a feed. If the XML is neither recognized RSS nor recognized Atom, raise FeedXmlError instead of quarantining it as an empty feed.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Reliability in src/political_event_tracking_research/rss_source_fetch.py

When status_output is requested, fetch_rss_sources() now unconditionally serializes the primitive status at the end of the run, but build_status([]) rejects an empty feed set with configured_feed_empty. That means an empty feed config now crashes only when status_output is enabled, after the CSV has already been written, which is a behavior change and leaves partial outputs behind. (line 225)

Suggestion: Handle the zero-feed case consistently before writing any artifacts: either reject an empty configuration up front regardless of status_output, or allow build_status()/write_fetch_status() to emit a canonical zero-feed status so the optional status file does not change success vs. failure.


Review by Codex PR Review bot • PR

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 771f7d13d4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

item_count=0,
error=f"{type(exc).__name__}: {exc}",
)
error_code = exc.code if isinstance(exc, (FeedXmlError, PrimitiveStatusError)) else "fetch_failed"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Avoid reading .code from FeedXmlError

When a feed returns malformed or oversized XML, parse_feed_items raises FeedXmlError, but that class does not define a code attribute. This line therefore raises AttributeError before the failed feed record is appended or --continue-on-feed-error can take effect, so the RSS Source Pipeline's fetch step can still abort on expected XML producer-boundary failures instead of writing the sanitized status.

Useful? React with 👍 / 👎.

)
return rows

return ("atom", rows) if include_kind else rows

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve RSS kind for empty feeds

For an RSS document with a channel but zero <item> entries, the new quarantine path reaches this Atom return because rss_items is empty. That records kind: "atom" for quarantined RSS feeds in source_fetch_status.json, so the producer status misclassifies the feed type exactly in the zero-entry scenario this change adds.

Useful? React with 👍 / 👎.

Comment on lines +222 to +223
if any(item["state"] == "failed" for item in feed_records):
raise RuntimeError("all configured RSS/Atom feeds failed")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Don't fail mixed quarantine status as all failed

When --continue-on-feed-error is used by the RSS Source Pipeline and all non-failing feeds are merely quarantined with zero entries, this branch still raises if any other feed failed because there are no accepted feeds. That is not the same as all configured feeds failing, and it aborts before writing the empty CSV even though the all-zero quarantine case just above is intended to complete with status.

Useful? React with 👍 / 👎.

Co-Authored-By: Codex <noreply@openai.com>
@Pigbibi Pigbibi closed this Jul 15, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 94786ebd11

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

)
return rows

return ("atom", rows) if include_kind else rows

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Classify unsupported XML as unknown or failed

When a feed URL returns well-formed XML that is not an Atom feed (for example an XHTML/XML error page or another unsupported root), atom_entries is empty and this unconditional return labels it as kind: "atom" with zero rows. fetch_rss_sources then records a quarantined zero_entries feed instead of an unknown/failed feed, which can mask a bad source and let an all-empty run complete without tripping the failed-feed guard.

Useful? React with 👍 / 👎.

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.

1 participant