Skip to content

feat: publish weekly producer contract artifact - #28

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/pert-weekly-producer-artifact-pr2
Closed

feat: publish weekly producer contract artifact#28
Pigbibi wants to merge 2 commits into
mainfrom
codex/pert-weekly-producer-artifact-pr2

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Scope

Producer/artifact boundary only, based on the merged political_event_weekly.v1 and weekly manifest contracts.

  • Explicit local inputs -> validated canonical weekly_manifest.json.
  • Full producer SHA, input SHA-256/CSV row counts, typed complete feed status, provenance, UTC ISO-week/as_of/generated_at.
  • Fail-closed on partial/stale/missing/mismatched/unsafe inputs and non-empty output destinations; local readback before return.
  • RSS workflow uses Monday UTC completed-week scheduling, requires explicit manual period_start/as_of, pins actions to immutable SHAs, and uploads exactly one weekly manifest with 30-day retention.
  • No QAR consumer, Pages, publisher, monthly, new data source, fetch algorithm, or credential-permission changes.

Validation

  • python3 -m pytest -q: 78 passed
  • focused weekly producer/contract/manifest/workflow tests: 48 passed
  • python3 -m compileall -q src tests scripts: passed
  • actionlint .github/workflows/*.yml: passed
  • git diff --check: passed
  • ruff: unavailable in local environment

Source-of-truth: merged political_event_weekly.v1 contract and docs/weekly_producer_artifact_contract.md.

Review mode: preflight then one concentrated review, no merge in this task.

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

⚖️ Codex Review Arbitration

🚫 block: In .github/workflows/rss_source_pipeline.yml, the Build weekly contract artifact step still takes the scheduled path whenever GITHUB_EVENT_NAME != 'workflow_dispatch', sets RUN_DATE="$(date -u +%F)", and derives PERIOD_START/AS_OF from that wall-clock date before calling write_weekly_artifact.py. No schedule-only input, artifact metadata, test, or documentation persists the original scheduled week for reuse on rerun; docs/weekly_producer_artifact_contract.md explicitly says scheduled runs 'derive that week using the documented producer rule,' which is this runtime date calculation. Therefore a rerun executed after a later Monday would compute a different completed week than the original scheduled run, so the current finding remains valid. The prior blocking finding concerned duplicate artifact upload/retention boundaries, which this diff addresses separately, so there is no contract conflict.

🚫 Blocking Issues

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

1. 🟠 [HIGH] Logic in .github/workflows/rss_source_pipeline.yml

The scheduled path derives period_start/as_of from date -u at execution time. If a scheduled run is rerun after a later Monday, it will silently generate a manifest for the newer completed week instead of the original failed run's week, producing a wrong weekly artifact under the same workflow run history. (line 98)

Suggestion: Do not derive the reporting week from the current wall clock on reruns. Persist explicit period_start/as_of values from the original run, or block schedule reruns and require backfills through workflow_dispatch with explicit dates.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Security in src/political_event_tracking_research/weekly_producer.py

The code validates each input path with lstat() and then reopens the same pathname multiple times in _sha256, _row_count, and _feed_status. A concurrent rename/symlink swap between validation and open can make the manifest hash, row count, or feed-status parse come from a different file than the one that passed the non-symlink check, breaking the advertised 'exact file' and 'non-symlink' guarantees. (line 183)

Suggestion: Open validated files once via a file descriptor with no-follow semantics, verify the opened inode matches the validated entry, and compute hash/row-count/status from that descriptor instead of reopening by pathname.

2. 🟡 [MEDIUM] Reliability in src/political_event_tracking_research/weekly_producer.py

Cleanup only runs when output_dir was created by this function. If the caller supplies an existing empty directory and the write/readback path fails after weekly_manifest.json is created, the function raises but leaves a partial artifact behind. That stale file makes the directory non-empty on retry and can be mistaken for a valid artifact by external callers. (line 281)

Suggestion: On any post-write failure, remove the newly created weekly_manifest.json (and restore the directory to its original empty state) even when output_dir already existed.


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: 38376f5c69

ℹ️ 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 +134 to +135
if not isinstance(item, Mapping) or type(item.get("ok")) is not bool:
raise _invalid("feed_status_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 malformed feed entries before marking status complete

When source_fetch_status.json is malformed or truncated but its counters still match, this loop accepts each feed entry as long as it has an ok boolean; for example feeds: [{"ok": true}] is treated as complete=True even though the RSS fetcher writes feed identity, URL, item count, and error fields for every feed. Since this producer is the fail-closed validation boundary for the weekly artifact, accepting status entries without the expected per-feed shape can publish a manifest from a corrupt status file instead of failing closed.

Useful? React with 👍 / 👎.

--watchlist "${WATCHLIST_PATH}" \
--events data/output/rss_source_pipeline/source_events.csv \
--output data/output/rss_source_pipeline/source_tracker.csv
- name: Build weekly contract artifact

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 Keep partial RSS runs from blocking live output publication

In this workflow the fetch step still uses --continue-on-feed-error, so a single RSS outage produces usable CSVs plus a nonzero failed_feed_count; however this new weekly artifact step runs before the existing live-output commit/upload path, and the producer exits on incomplete feed status. In scheduled runs where one feed fails, the job now stops here and skips publishing the ordinary RSS outputs that were previously refreshed from the healthy feeds, so the weekly artifact failure should be isolated or moved after the existing publication path if partial RSS runs should remain available.

Useful? React with 👍 / 👎.

Comment on lines +123 to +126
data/output/rss_source_pipeline/source_items.csv \
data/output/rss_source_pipeline/source_events.csv \
data/output/rss_source_pipeline/source_tracker.csv \
data/output/rss_source_pipeline/source_fetch_status.json

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 Filter weekly artifacts to the declared period

The manifest records a weekly period_start/as_of, but the files passed here are the unfiltered RSS pipeline outputs; the preceding fetch/extract/build commands do not take period bounds, so source_items.csv, source_events.csv, and source_tracker.csv can include anything in the latest N feed items, or current items when manually backfilling an older week. That makes the uploaded political-event-weekly-v1 artifact claim a completed week while hashing rows outside that week, which can corrupt downstream weekly experiments.

Useful? React with 👍 / 👎.

Comment on lines +188 to +192
artifacts.append(WeeklySourceArtifact(key, _sha256(path), _row_count(path)))
status_key, status_file = _relative_input(feed_status_path, base)
if status_key not in seen or status_file is None:
raise _invalid("feed_status_missing")
status = _feed_status(status_file)

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 Compare fetched item count with source_items rows

If source_items.csv is truncated or paired with a source_fetch_status.json from a different run, the fetcher’s top-level item_count is the available cross-check, but after computing row counts this code only validates feed success counters and still marks the status complete. The artifact can therefore contain the SHA and row count of a partial items CSV while advertising complete feed status, so the producer should fail when source_items.csv row_count disagrees with the status item_count.

Useful? React with 👍 / 👎.

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

Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

PERMANENT_FREEZE/CLOSE_RESLICE per control-plane stop rule. Evidence preserved; no further code action.

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