Skip to content

Add same-run weekly period-lock acquisition preflight - #30

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

Add same-run weekly period-lock acquisition preflight#30
Pigbibi wants to merge 2 commits into
mainfrom
codex/pert-weekly-period-lock-pr1a

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Scope

Isolated, test-only PR-1A for pert.weekly.period_lock_acquisition.v1. This does not modify the RSS/source production pipeline, producer fetch behavior, QAR, Pages, or any production credentials.

Contract

  • Attempt 1 builds fixed representative period-lock and input-snapshot values without wall-clock derivation, reads them back, and uploads one immutable artifact named pert-weekly-period-lock-<github.run_id>.
  • The bundle has exactly period_lock.json, input_snapshot.json, and bundle_manifest.json; the manifest binds run id, source attempt 1, artifact name, and member SHA-256 values.
  • Attempt 2 is limited to the same run rerun (github.run_attempt == 2), downloads by the same github.run_id, and verifies canonical bytes, exact member set, lock/snapshot identity, attempt/run binding, artifact name, and hashes. It never rebuilds or overwrites attempt-1 evidence.
  • Actions are pinned to immutable full SHAs. Permissions are contents:read, actions:read, and artifact-metadata:write; no contents write, secrets, or OIDC.

Validation

  • python3 -m pytest -q: 94 passed
  • uv sync --locked --extra test && uv run --locked pytest -q: 94 passed
  • python3 -m compileall -q src scripts tests: passed
  • actionlint .github/workflows/pert_weekly_period_lock_acquisition.yml: passed
  • git diff --check: passed
  • ruff: unavailable in the environment

Acceptance gate

After PR checks are green and the PR head remains 6227d65bcfaa75618b405506b21beb24549253b2, run this isolated workflow once naturally for attempt 1. Only if attempt 1 succeeds, manually rerun this same workflow exactly once to obtain attempt-2 artifact download/readback evidence. Do not rerun RSS, production, CI, review, or gate workflows.

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 .github/workflows/pert_weekly_period_lock_acquisition.yml, the workflow grants actions: read at workflow scope, checks out the dispatch-selected ref with actions/checkout and then executes python scripts/period_lock_acquisition_preflight.py ... from that checked-out tree. The new docs explicitly say a maintainer should select the PR head for the initial run, so PR-controlled repository code can run with an actions:read token, which preserves the trusted-code problem in a new form even though pull_request was removed. In scripts/period_lock_acquisition_preflight.py, verify_bundle() only requires period_lock.json to parse canonically and match source_run_id/source_attempt; it never compares the rest of the lock to _fixture_lock_payload(expected_run_id) and never checks that the parsed lock's snapshot/provenance fields agree with input_snapshot.json. The manifest check does not close that gap because expected_manifest_bytes is rebuilt from the actual lock and snapshot payload bytes, so a replaced-but-canonical lock plus a recomputed manifest still verifies. The current findings do not conflict with the prior blocking findings: the security finding still requires keeping actions:read on trusted code paths, and the logic finding adds exact lock verification without reversing the prior exact-manifest requirement.

🚫 Blocking Issues

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

1. 🟠 [HIGH] Security in .github/workflows/pert_weekly_period_lock_acquisition.yml

This job checks out and executes repository code from the manually selected ref while granting GITHUB_TOKEN actions:read. If a maintainer dispatches it against a PR branch, the Python invoked in later steps is PR-controlled code and can use that token to enumerate or download other workflow artifacts from the repository, which is the opposite of the stated goal of keeping actions:read away from untrusted PR code. (line 20)

Suggestion: Do not run the verification script from the PR ref. Check out a trusted ref for the workflow logic (or vendor the script into the workflow action), treat the candidate PR SHA only as data, and only grant actions:read to trusted code paths.

2. 🟠 [HIGH] Logic in scripts/period_lock_acquisition_preflight.py

verify_bundle() only checks that period_lock.json is canonical and that its source_run_id/source_attempt match. It never asserts that the rest of the lock matches the deterministic attempt-1 fixture, nor that the lock's snapshot fields agree with input_snapshot.json. A same-run artifact with a replaced but still syntactically valid lock (different period boundaries, workflow/provenance fields, or snapshot identifiers) plus recomputed manifest digests will still verify successfully. (line 174)

Suggestion: Rebuild the exact expected lock for expected_run_id and require byte-for-byte equality with period_lock.json, or compare every lock field against the expected fixture and cross-check the parsed lock's snapshot fields against input_snapshot.json before accepting the bundle.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Reliability in .github/workflows/pert_weekly_period_lock_acquisition.yml

The acceptance gate depends on running attempt 1 only if the PR head is still 6227d65..., but the workflow has no input or assertion that binds the run to that reviewed commit. workflow_dispatch selects a branch tip, not an immutable commit, so if the branch advances after checks go green this workflow will silently validate a different revision and still produce a passing artifact. (line 4)

Suggestion: Add an expected_sha workflow input and fail unless it matches github.sha (or explicitly check out that SHA). Record the commit SHA in the manifest and verify it on rerun so the artifact is tied to the reviewed revision.


Review by Codex PR Review bot • PR

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

@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: 6227d65bcf

ℹ️ 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 +186 to +187
if serialize_period_lock(lock) != payloads["period_lock.json"] or lock.source_run_id != expected_run_id or lock.source_attempt != 1:
raise ValueError("period_lock_run_mismatch")

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 Verify the full period lock payload

In the attempt-2 verification path, this check only proves that period_lock.json is canonical and has the expected run id/attempt. A bundle can replace it with a different valid weekly period or different snapshot/provenance fields and update bundle_manifest.json with the new digest; the fixed snapshot is checked independently, so verify_bundle still accepts a non-original lock. Since this preflight is meant to perform an exact readback of the original lock, compare the parsed lock to the expected fixture payload (or bind the snapshot fields from the lock) before returning.

Useful? React with 👍 / 👎.

Comment on lines +205 to +207
files = manifest["files"]
if not isinstance(files, list) or {item.get("name") for item in files if isinstance(item, dict)} != {"input_snapshot.json", "period_lock.json"}:
raise ValueError("period_lock_manifest_files_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 duplicate manifest file entries

When files contains duplicate objects for an expected member name, this set comparison still passes as long as both expected names appear at least once, and the loop below accepts duplicate entries whose digests match. That means a tampered manifest with repeated input_snapshot.json or period_lock.json entries is treated as valid even though the preflight contract says duplicated bundle metadata must fail closed; require exactly two entries with unique names before checking their digests.

Useful? React with 👍 / 👎.

Comment on lines +50 to +53
name: ${{ env.ARTIFACT_NAME }}
path: ${{ runner.temp }}/pert-period-lock/downloaded
run-id: ${{ github.run_id }}
github-token: ${{ github.token }}

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 Fail when duplicate run artifacts exist

When a rerun leaves more than one artifact with this same run-scoped name, this name-only download-artifact lookup does not prove uniqueness: the v4 artifact client uses the public name-filtered run API for github-token/run-id downloads and returns the newest match when multiple artifacts share the name. That silently accepts the duplicate/replacement case the preflight doc says must fail closed; query the run artifacts and require exactly one matching artifact (or download by a previously recorded artifact id) before verifying the bundle.

Useful? React with 👍 / 👎.

@Pigbibi

Pigbibi commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

PERMANENT_FREEZE/HUMAN_REQUIRED after the single allowed closure. Review fingerprint 2e1c0a6672f5f3c21882 repeated. Preserving branch, commits, comments, checks, and workflow evidence; no further code action. Current blockers: actions:read executes checked-out ref, verification lacks exact full lock/snapshot binding, and dispatch ref is not bound to expected reviewed SHA.

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