Skip to content

H1 reslice: preserve explicit RSS/Atom feed kind - #43

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

H1 reslice: preserve explicit RSS/Atom feed kind#43
Pigbibi wants to merge 2 commits into
mainfrom
feat/pert-upstream-feed-primitive-status-h1-r2

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Scope

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

This PR fixes the producer-owned feed parsing/status boundary only:

  • parse_feed_snapshot() returns explicit feed_kind (rss2/atom) with the same immutable entries snapshot; empty feeds preserve their real kind.
  • Closed RSS2/Atom root/container grammar rejects near matches.
  • Primitive status is canonical, deterministic, JSON-safe, digest-bound, and uses accepted rows sorted by (published_at, item_id) to match emitted CSV order.
  • Empty feeds become quarantined with zero accepted rows/digest contribution; failed or quarantined feeds cannot be publication-complete.
  • Status output is canonical bytes without a trailing newline.

No workflow, publish, QAR, weekly artifact, permission, or automation changes are included.

Validation

  • uv run pytest -q: 134 passed
  • python3 -m compileall -q src scripts tests: passed
  • git diff --check: passed
  • ruff: unavailable in environment

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: 2 serious issue(s) found in high-risk files

⚖️ Codex Review Arbitration

🚫 block: Both current findings remain valid against the cumulative diff. In src/political_event_tracking_research/rss_source_fetch.py, the RSS path in parse_feed_snapshot() checks only root.tag == "rss", version == "2.0", and len(root.findall("./channel")) == 1, then validates only the children of that single <channel>. There is no validation of the full <rss> child set, so payloads like <rss version="2.0"><channel/><item/></rss> or <rss version="2.0"><channel/><channelx/></rss> are accepted instead of raising feed_schema_invalid. In src/political_event_tracking_research/feed_primitives.py, _validate_feed_record() explicitly rejects kind == "unknown" for accepted feeds and for quarantined feeds, but _validate_wire() never enforces that invariant: an accepted feed passes as long as it has positive accepted_row_count and no error_code, and a quarantined feed passes as long as it has zero accepted_row_count and a non-null error_code, even when kind is "unknown". That means serialize_status() / parse_status_bytes() accept wire states the builder would never emit. There is no contract conflict with the prior blocking finding: the prior requirement was to allow foreign-namespaced metadata under RSS <channel> / Atom <feed>, and the current findings do not require reversing that behavior.

🚫 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_snapshot() only checks that there is exactly one ./channel child under <rss>, but it never rejects additional non-foreign root children. Inputs like <rss version="2.0"><channel/><channelx>...</channelx></rss> or <rss version="2.0"><channel/><item>...</item></rss> are therefore accepted as valid RSS2 and can be misclassified as empty/quarantined, silently dropping data that should have failed schema validation. (line 194)

Suggestion: After locating the single <channel>, validate the full set of <rss> children and reject any child that is not that channel (or an explicitly allowed foreign-namespaced extension, if desired). This should make the RSS root grammar fail closed the same way the Atom root logic already does.

2. 🟠 [HIGH] Bug in src/political_event_tracking_research/feed_primitives.py

_validate_wire() accepts canonical status payloads where a feed has state: "accepted" or state: "quarantined" together with kind: "unknown". That violates the builder-side contract in _validate_feed_record(), which correctly requires explicit rss2/atom for non-failed feeds. As written, serialize_status() / parse_status_bytes() will treat impossible producer states as valid wire data, undermining the PR's explicit-kind guarantee. (line 217)

Suggestion: Mirror the same state/kind invariants from _validate_feed_record() inside _validate_wire(): reject kind: "unknown" for accepted feeds and for quarantined feeds, so the wire parser cannot accept states that the builder would never emit.


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: c018a04e9c

ℹ️ 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".

Comment on lines +215 to +216
if any(child.tag != "item" and child.tag not in rss_metadata for child in channel):
raise FeedXmlError("feed_schema_invalid")

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 Allow RSS extension metadata under channel

When a valid RSS 2.0 channel includes module metadata such as atom:link or sy:updatePeriod under <channel>, this new whitelist treats the feed as feed_schema_invalid before parsing any <item> rows. I checked .github/workflows/rss_source_pipeline.yml, and the scheduled pipeline runs this parser against config/free_rss_feeds.csv with --continue-on-feed-error, so common extensible feeds can now be marked failed and lose all rows even though their item structure is valid; keep the root/channel checks closed without rejecting namespaced channel metadata.

Useful? React with 👍 / 👎.

error=f"{type(exc).__name__}: {exc}",
)
parsed = parse_feed_snapshot(fetcher(feed.feed_url), feed, max_items=max_items_per_feed)
feed_rows = [PrimitiveRow.from_mapping(row).to_mapping() for row in parsed.entries]

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 Allow feeds with blank authors

When a feed config omits the optional author value or leaves it blank, load_feed_config() still constructs FeedConfig(..., author=""), and downstream source-item loading also treats author as optional, but this new PrimitiveRow.from_mapping() conversion rejects the empty author via _exact_str. A non-empty feed that previously wrote rows with a blank author is now recorded as failed or raises before writing any of its items.

Useful? React with 👍 / 👎.

Comment on lines +302 to +304
code = getattr(exc, "code", "fetch_failed")
if type(code) is not str or not re.fullmatch(r"[a-z][a-z0-9_]{0,63}", code):
code = "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.

P2 Badge Preserve parser error codes in feed status

When parse_feed_snapshot() rejects a feed, such as feed_schema_invalid or feed_xml_oversize, the scheduled RSS workflow continues on feed errors and records this status, but FeedXmlError only stores the code as its exception message and has no .code attribute. This fallback therefore reports every parser/schema/size failure as fetch_failed, losing the sanitized producer-boundary reason that the new status format is trying to expose.

Useful? React with 👍 / 👎.

Comment on lines +250 to +252
if item["state"] == "accepted":
if item["accepted_row_count"] == 0 or item["error_code"] is not None:
_fail("feed_state_invalid")

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 Reject accepted wire feeds with unknown kind

When validating externally supplied status bytes with parse_status_bytes() or serialize_status(), a feed can be state: "accepted" while still carrying kind: "unknown"; the counters can then make publication_complete true even though accepted producer records explicitly reject unknown feed kinds. This weakens the status boundary for consumers that trust parsed status flags without rebuilding from feed rows.

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: b452cf5b75

ℹ️ 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".

Comment on lines +259 to +260
complete = bool(feeds) and failed == 0 and quarantined == 0 and accepted == len(feeds)
if data["publication_complete"] != complete or data["eligible_for_live_publication"] != complete:

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 Require zero rejected rows before marking complete

When validating externally supplied status bytes via parse_status_bytes() or serialize_status(), a status with every feed in accepted state but rejected_row_count > 0 still recomputes publication_complete and eligible_for_live_publication as true here. That lets consumers that gate live publication on these flags accept a partial/rejected producer snapshot; include rejected_rows == 0 or reject nonzero feed rejections in the completeness check.

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