Skip to content

Add pure weekly period bundle binding contract - #31

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/pert-weekly-trusted-period-bundle-pr1a
Closed

Add pure weekly period bundle binding contract#31
Pigbibi wants to merge 2 commits into
mainfrom
codex/pert-weekly-trusted-period-bundle-pr1a

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Scope

PR-1A pure foundation from pert_weekly_trusted_workflow_period_lock_redesign_d0_2026-07-15.md. No workflow, GitHub API, filesystem, upload/download, producer, QAR, Pages, legacy, migration, or privileged permission changes. This does not reuse PR #30.

Contract

  • BundleContext fixes original run_id, source_attempt=1, expected merged period lock, exact reviewed workflow SHA, producer SHA, and external artifact evidence (name/id/digest/retention).
  • BundleWire is canonical bytes for exactly period lock, input snapshot, and bundle manifest, with SHA-256 properties.
  • Snapshot and manifest use strict exact key sets, canonical JSON, duplicate-key rejection, safe integer/type checks, and sanitized errors.
  • verify_period_bundle() reconstructs the complete expected lock, snapshot, and manifest from the full context and requires byte-for-byte equality; a replacement payload plus recomputed manifest cannot pass.
  • RerunContext accepts only current attempt 2 and reuses the original context. Collection verification requires exactly one bundle.
  • Unexpected programming/system exceptions are not broad-caught.

Validation

  • python3 -m pytest -q: 107 passed
  • uv sync --locked --extra test && uv run --locked pytest -q: 107 passed
  • python3 -m compileall -q src tests scripts: passed
  • git diff --check: passed
  • ruff: unavailable in environment

Explicit non-scope

No actions:read, artifact-metadata:write, workflow changes, arbitrary ref checkout, or PR privileged execution. PR-1B is blocked until this PR is merged and post-merge main CI succeeds; only then may a trusted default-branch harness be created and the one controlled rerun performed.

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: src/political_event_tracking_research/weekly_period_bundle.py never binds BundleContext.repository or reviewed workflow identity to period_lock.workflow_ref. BundleContext.__post_init__() checks run_id, repository, workflow_sha, producer_sha, artifact fields, period_lock.source_run_id, source_attempt, and producer_ref, but it never inspects period_lock.workflow_ref. The underlying lock type in weekly_period_lock.py also only validates workflow_ref syntactically via _workflow_ref(). In the bundle contract, _manifest_bytes() records repository and workflow_sha, while _expected_snapshot() uses workflow_sha; neither includes or cross-checks workflow_ref. verify_period_bundle() then accepts any bundle whose parsed lock exactly matches expected.period_lock, so a caller can supply a lock from another repository/workflow together with a BundleContext claiming QuantStrategyLab/PoliticalEventTrackingResearch and verification still succeeds. That leaves the current finding valid. The prior finding was about rerun identity binding (current_run_id / rerun provenance), so the current finding does not reverse or contradict it.

🚫 Blocking Issues

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

1. 🟠 [HIGH] Security in src/political_event_tracking_research/weekly_period_bundle.py

BundleContext.__post_init__() validates repository and workflow_sha syntactically, but never checks that period_lock.workflow_ref actually belongs to that repository or expected workflow path/ref. As written, a caller can create a bundle whose lock was produced by a different repository or workflow while the manifest still claims QuantStrategyLab/PoliticalEventTrackingResearch, and verify_period_bundle() will accept it as long as the caller supplies the same inconsistent BundleContext. (line 245)

Suggestion: Parse period_lock.workflow_ref during BundleContext validation and require its owner/name (and, if this contract fixes a specific workflow file/ref, that path/ref as well) to match repository and the reviewed workflow identity before building or verifying a bundle.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Logic in src/political_event_tracking_research/weekly_period_bundle.py

Manifest structural validation is effectively bypassed for any non-exact payload because verify_period_bundle() compares bundle.manifest_bytes to the reconstructed expected bytes before calling _parse_manifest(). That means malformed JSON, duplicate keys, missing/unknown keys, or noncanonical ordering all raise bundle_manifest_mismatch instead of the sanitized manifest-specific errors promised by the public contract. (line 365)

Suggestion: Parse and structurally validate bundle.manifest_bytes first, then compare the parsed/canonical manifest to the reconstructed expected manifest so malformed manifests raise bundle_manifest_invalid/bundle_manifest_duplicate_key and only genuine content differences raise bundle_manifest_mismatch.


Review by Codex PR Review bot • PR

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

Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Concentrated closure applied at 119149b.

  • RerunContext now requires exact current_run_id, repository, reviewed workflow SHA, and attempt 2; verifier rechecks the full context.
  • Added bounded lock/snapshot/manifest bytes, snapshot depth/object/list/string/source-artifact limits, with sanitized fail-closed errors and no broad system-exception catch.
  • Added regressions for unrelated runs, repository/workflow mismatches, resource bounds, valid same-run attempt 2, and RuntimeError/SystemExit/MemoryError propagation.

Pure PR-1A boundary is unchanged: no workflow, permissions, Actions, producer, or privileged integration changes.

@Pigbibi

Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

PERMANENT_FREEZE/CLOSE_RESLICE per control-plane stop rule. Preserving branch, worktree, commits, review threads, CI/review evidence; no further code action.

@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: 119149b036

ℹ️ 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 period_lock != context.period_lock:
raise _error("bundle_lock_mismatch")
try:
lock_bytes = serialize_period_lock(period_lock)

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 oversized generated bundle members

When period_lock is valid but serializes above the verifier caps, build_period_bundle() still returns a BundleWire that verify_period_bundle() rejects immediately (for example, 17 source artifacts with 4096-character paths are under the snapshot limits but produce lock_bytes > MAX_LOCK_BYTES, causing bundle_lock_oversized). Please enforce the same member-size limits while building so a locally built bundle cannot be unverifiable by this contract.

Useful? React with 👍 / 👎.

Comment on lines +258 to +260
if self.period_lock.source_run_id != self.run_id or self.period_lock.source_attempt != 1:
raise _error("bundle_lock_context_mismatch")
if self.period_lock.producer_ref != self.producer_sha:

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 locks from a different workflow ref

When the supplied period_lock.workflow_ref names a different owner/repo than repository, this context still validates because only the run id/attempt and producer SHA are compared. build_period_bundle() and verify_period_bundle() then accept a bundle whose manifest is bound to the expected repository while the lock source of truth points elsewhere, so inconsistent origin evidence does not fail closed. Please bind the lock's workflow ref to the bundle repository before accepting the context.

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