Enforce immediate prior weekly provenance - #37
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review🚫 Merge blocked: 3 serious issue(s) found in high-risk files ⚖️ Codex Review Arbitration🚫 block: 至少两条当前 blocking finding 仍被累计 diff 直接证实：1) 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🟠 [HIGH] Logic in
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c551e647d
ℹ️ 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".
| if not isinstance(value, Mapping) or not required.issubset(value) or set(value) - required - optional: | ||
| raise _fail("feed_status_invalid") |
There was a problem hiding this comment.
When the weekly artifact step consumes data/output/rss_source_pipeline/source_fetch_status.json, this check rejects the actual payload from write_fetch_status(), which always includes extra top-level keys such as generated_at, item_count, and feeds. As a result, even a successful fetch run raises feed_status_invalid before uploading political-event-weekly-v1, so the new weekly artifact cannot be produced from the workflow's own inputs.
Useful? React with 👍 / 👎.
| ref: ${{ github.sha }} | ||
| persist-credentials: false |
There was a problem hiding this comment.
When COMMIT_OUTPUTS=true and generated files change, the later publish step runs git commit followed by plain git push; checking out the raw ${{ github.sha }} leaves the job on a detached commit, and persist-credentials: false opts out of the checkout token that actions/checkout documents as enabling later authenticated git commands. Scheduled runs therefore fail instead of publishing live CSV updates whenever there is a change to commit.
Useful? React with 👍 / 👎.
| "source_watchlist_length": len(watchlist), "source_watchlist_sha256": _sha(watchlist), | ||
| "source_inputs": [{"name": "source_events.csv", "length": len(source_events), "sha256": _sha(source_events)}, {"name": WATCHLIST, "length": len(watchlist), "sha256": _sha(watchlist)}], | ||
| } | ||
| files: dict[str, bytes] = {LOCK: _json(lock, "period_lock_invalid"), EVENTS: filtered, WATCHLIST: watchlist, WEEKLY: _json(weekly, "weekly_contract_invalid")} |
There was a problem hiding this comment.
When a consumer validates the uploaded JSON files with the repository's existing contract parsers, both fail: political_event_weekly.json advertises political_event_weekly.v1 but lacks the required source_artifacts, and period_lock.json advertises pert.weekly.period_lock.v1 but lacks required lock fields such as source_snapshot_id/source_artifacts. Since this line serializes those ad-hoc dicts into the artifact, the new artifact is incompatible with the repo's own validators (contract_shape_invalid / period_lock_shape_invalid).
Useful? React with 👍 / 👎.
| run: | | ||
| set -euo pipefail | ||
| mkdir -p data/output/political-event-weekly-v1 | ||
| python scripts/build_weekly_artifact.py --period-start "${{ needs.validate-run-boundary.outputs.period_start }}" --as-of "${{ needs.validate-run-boundary.outputs.as_of }}" --generated-at "$(date -u +%Y-%m-%dT%H:%M:%S.000000Z)" --workflow-ref "${GITHUB_WORKFLOW_REF}" --source-run-id "${GITHUB_RUN_ID}" --producer-ref "${{ needs.validate-run-boundary.outputs.producer_ref }}" --source-events data/output/rss_source_pipeline/source_events.csv --watchlist "${WATCHLIST_PATH}" --feed-status data/output/rss_source_pipeline/source_fetch_status.json --output-dir data/output/political-event-weekly-v1 --run-mode "${RUN_MODE}" |
There was a problem hiding this comment.
When a manual dispatch overrides feeds_path, aliases_path, watchlist_path, or max_items_per_feed, the boundary guard still succeeds because it checks only run identity and period, and this command publishes the resulting CSV/watchlist as the official weekly artifact without recording those overrides. A test run using a reduced feed set or example watchlist can therefore upload political-event-weekly-v1 that looks like canonical weekly provenance while silently omitting or changing the source universe.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
|
PERMANENT_FREEZE/CLOSE_RESLICE: terminal review failure after absolute-final closure. Preserve branch, commits, reviews, checks, and tests; no further code action. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aee50fd8fb
ℹ️ 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".
| if lock_object.source_snapshot_digest != _snapshot_digest(files[EVENTS], files[WATCHLIST]): | ||
| raise _fail("source_snapshot_mismatch") |
There was a problem hiding this comment.
When a bad producer or regenerated manifest changes source_artifacts in either JSON contract, this still accepts the artifact because only the combined snapshot digest is checked; the per-file hashes and row counts declared in political_event_weekly.json/period_lock.json are never compared with the included CSV bytes. That can bless an artifact whose advertised provenance points at different source files than the ones consumers actually receive.
Useful? React with 👍 / 👎.
| if not isinstance(files, Mapping) or tuple(files) != ARTIFACT_FILES or any(type(files[name]) is not bytes for name in ARTIFACT_FILES): | ||
| raise _fail("artifact_file_set_invalid") |
There was a problem hiding this comment.
When a consumer loads a valid downloaded artifact from disk, the insertion order of the mapping can differ from ARTIFACT_FILES (for example when iterating a directory), but the artifact still contains exactly the required five names. Comparing tuple(files) makes parse_weekly_artifact reject those valid artifacts solely because of mapping order, which is not preserved by the artifact contract.
Useful? React with 👍 / 👎.
| if lock_object.period_start != start or lock_object.as_of != as_of or lock_object.workflow_ref != WORKFLOW_REF or lock_object.producer_ref != contract.producer_ref: | ||
| raise _fail("period_lock_mismatch") |
There was a problem hiding this comment.
When either JSON contract is regenerated with a different valid source_provenance, the artifact still parses because this consistency check only compares period/workflow/producer fields. The manifest hard-codes the official provenance, so a single accepted artifact can contain contradictory provenance claims that downstream consumers may trust differently.
Useful? React with 👍 / 👎.
| if counts["feed_count"] <= 0 or counts["successful_feed_count"] != counts["feed_count"] or any(counts[key] for key in ("failed_feed_count", "stale_feed_count", "missing_feed_count")) or not complete: | ||
| raise _fail("feed_status_incomplete") |
There was a problem hiding this comment.
When the status JSON records a failed feed in feeds but the summary counts still say complete (for example after a writer bug or hand-edited status), _status accepts it because this check trusts only the top-level counters. Since the weekly producer relies on this validation to fail closed on failed feeds, inconsistent feed evidence can still produce an official weekly artifact.
Useful? React with 👍 / 👎.
| if type(self.source_attempt) is not int or not 1 <= self.source_attempt <= MAX_SAFE_JSON_INTEGER: | ||
| raise _invalid("source_attempt_invalid") |
There was a problem hiding this comment.
When a GitHub rerun produces run_attempt 2+, the workflow passes that value into the weekly lock and fetches live RSS again; accepting attempts above 1 here mints a distinct official lock instead of comparing against attempt 1's immutable source snapshot as documented in docs/weekly_period_lock_contract.md. If feeds have rolled over, a rerun can publish a different or empty artifact for the same weekly period.
Useful? React with 👍 / 👎.
1 participant
Summary
Contract
run.created_atfrom the trusted GitHub REST workflow-run payload is the period authority. Manualperiod_start/as_ofare required and must exactly match the derived previous week. No wall-clock/latest-RSS fallback or history backfill is supported. Historical backfill requires a separate immutable source snapshot contract.The dedicated artifact remains
political-event-weekly-v1, retention 30 days, with exactly:period_lock.json,political_events.csv,political_watchlist.csv,political_event_weekly.json,weekly_manifest.json.Source events are strictly date-parsed and filtered to the locked period; zero-event weeks are valid; incomplete/failed/stale/missing feed status fails closed. Main-only workflow identity and
actions:readare retained.Source design:
/Users/lisiyi/Documents/Codex/2026-07-16/pert_weekly_history_backfill_provenance_reslice_design_v1_2026-07-16.mdValidation
python3 -m pytest -q— 124 passeduv sync --locked --extra test && uv run pytest -q— 124 passedpython3 -m compileall -q src scripts tests— passedactionlint .github/workflows/rss_source_pipeline.yml— passedgit diff --check— passedruffunavailable in environmentScope exclusions
No history backfill, QAR consumer, Pages/publisher, monthly support, permission expansion, legacy migration, or production trading logic.