Skip to content

feat: add D3 daily preview artifact acceptance slice#51

Closed
Pigbibi wants to merge 2 commits into
mainfrom
codex/qar-vnext-d3-daily-action-artifact
Closed

feat: add D3 daily preview artifact acceptance slice#51
Pigbibi wants to merge 2 commits into
mainfrom
codex/qar-vnext-d3-daily-action-artifact

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Scope

D3-PR0 clean-slate, daily-only representative-fixture producer -> existing qar.preview_bundle.v1 -> isolated $RUNNER_TEMP directory -> GitHub Actions artifact upload -> independent download/readback evidence.

Source of truth: /Users/lisiyi/Documents/Codex/2026-07-14/ba-2/outputs/qar_vnext_d3_concrete_action_artifact_design_v1_2026-07-15.md

Contract

  • Representative repository fixture only; never labeled live producer or production-trusted.
  • Exact report.json, report.html, manifest.json; canonical JSON; manifest SHA/source pair; relative links; repeat-build byte equality.
  • Daily only, schema 5 / manifest contract model_recommendations.v5.
  • No weekly/monthly, Pages, publisher/archive/feed, legacy/compatibility/migration, identity/store, or production integration.
  • Issue vNext D2: harden preview bundle readback evidence #50 remains the production-trust hardening gate.

Validation

  • python3 -m pytest: 284 passed
  • python3 -m compileall -q src scripts tests: passed
  • actionlint .github/workflows/qar_vnext_d3_daily_preview_artifact.yml: passed
  • git diff --check: passed
  • Targeted/full ruff: unavailable in this environment (ruff executable not installed)

Real GitHub Actions artifact upload/download/readback evidence is required before merge; worker does not merge or poll after PR creation.

Base: origin/main@c5ba801a8696eec63c7ba348f3f125cb52cd06ff
Commit: c255a80

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

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

🚫 Merge blocked: 2 serious issue(s) found in high-risk files

⚖️ Codex Review Arbitration

🚫 block: Current finding 1 remains valid. The new workflow only triggers on six paths in .github/workflows/qar_vnext_d3_daily_preview_artifact.yml, but the build step directly consumes examples/political_events.example.csv and examples/political_watchlist.example.csv, src/quant_advisor_research/preview_bundle.py imports contracts.py and artifact_integrity.py, and the job runs python -m pip install -e ., so packaging inputs such as pyproject.toml can affect the executed code. A PR can therefore change artifact bytes or validation behavior without triggering this acceptance workflow. Current finding 2 also remains valid. In src/quant_advisor_research/preview_bundle.py, read_preview_bundle() enforces canonical bytes only for report.json via _canonical_json(snapshot) != report_bytes; for manifest.json it only parses JSON and checks manifest != expected_manifest, which would still pass if the manifest were re-encoded with different whitespace or key order. The new build/verify scripts nevertheless record canonical_json_readback as a completed check, so the acceptance evidence overstates what was actually proven. The prior blocking finding about binding download bytes to build evidence is fixed by the new --build-evidence-path flow and hash/base-sha/source comparisons in scripts/d3_verify_daily_preview_artifact.py, and the current findings do not contradict that behavior.

🚫 Blocking Issues

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

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

The pull_request.paths filter is too narrow for the contract this workflow is supposed to enforce. The uploaded preview bytes and readback rules also depend on the representative fixture CSVs and on transitive validators/helpers such as src/quant_advisor_research/contracts.py, src/quant_advisor_research/artifact_integrity.py, and packaging/install inputs. A PR can change those files, alter the produced artifact or relax validation, and this acceptance workflow will not run, so the repository can merge artifact-contract changes without the promised upload/download evidence. (line 5)

Suggestion: Remove the path filter, or expand it to cover every direct input and validator that can affect bundle bytes or acceptance, including the example CSV fixtures and the preview-bundle validation dependencies.

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

The download/readback harness treats read_preview_bundle() as proof of canonical_json_readback, but that helper only enforces canonical bytes for report.json. manifest.json is accepted after semantic JSON comparison, so a future change that re-encodes the manifest with different whitespace or key order would still pass this acceptance slice even though the PR contract requires exact canonical manifest.json bytes. (line 26)

Suggestion: After parsing the bundle, recompute the canonical JSON bytes for manifest.json with the same sort_keys=True, separators=(',', ':'), and allow_nan=False settings used by the writer, and compare them byte-for-byte during both build-time and download-time verification before emitting success.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Security in .github/workflows/qar_vnext_d3_daily_preview_artifact.yml

The workflow executes remote actions (actions/checkout, actions/setup-python, actions/upload-artifact, actions/download-artifact) by mutable major tags instead of immutable commit SHAs. A compromised or retagged upstream release would run attacker-controlled code in the integrity-attestation job with this workflow's token and artifact access, which undermines the trustworthiness of the evidence this PR is adding. (line 25)

Suggestion: Pin each uses: reference to a reviewed full commit SHA (or an internally vetted wrapper action) and update those SHAs deliberately when you want to upgrade.


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

ℹ️ 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 +86 to +89
build_preview_bundle(report, repeat_output)
repeat_equal = all(
(output / name).read_bytes() == (repeat_output / name).read_bytes()
for name in ("manifest.json", "report.html", "report.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.

P2 Badge Rebuild the report for repeat-byte evidence

When the workflow is re-run for the same --as-of at different seconds, build_advisory_report stamps a new generated_at, so report.json and the manifest hashes differ; this check still records repeat_build_bytes=true because it only serializes the same in-memory report a second time. The acceptance evidence can therefore certify byte equality while two actual fixture builds are not byte-identical.

Useful? React with 👍 / 👎.

Comment on lines +7 to +11
- "scripts/d3_build_daily_preview_artifact.py"
- "scripts/d3_verify_daily_preview_artifact.py"
- "src/quant_advisor_research/preview_bundle.py"
- "src/quant_advisor_research/advisory_report.py"
- "tests/test_d3_daily_preview_artifact.py"

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 Include fixture CSVs in the PR path filter

When a PR changes examples/political_events.example.csv or examples/political_watchlist.example.csv, this workflow is skipped because the paths filter only lists workflow/script/src/test files, even though the build step consumes those two fixture files. That lets changes to the representative artifact inputs bypass the artifact upload/download/readback acceptance check.

Useful? React with 👍 / 👎.

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

Pigbibi commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Concentrated closure completed at 7bbf5c7 (full head to be verified by GitHub).

Addressed only the three D3 evidence findings:

  1. The harness now independently calls build_advisory_report twice and separately builds both bundles under a harness-only frozen generated_at (2026-07-15T00:00:00Z); evidence explicitly records deterministic_clock.mode=frozen_harness and does not claim live wall-clock determinism.
  2. Download verification now requires build evidence and compares expected full base SHA, source metadata, exact file set, and all three report/html/manifest SHA-256 values before producing download evidence.
  3. Build and verify both enforce schema 5, manifest model_recommendations.v5, cadence daily, and bundle qar.preview_bundle.v1.

Validation: focused D3+preview tests 20 passed; full pytest 285 passed; compileall, actionlint, and diff-check passed. Ruff was unavailable because the executable is not installed. No weekly/monthly/Pages/publisher/archive/feed/legacy/identity/store changes. This is closure 1/1, absolute final; any new critical/high requires permanent freeze/close-reslice.

@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: 7bbf5c7772

ℹ️ 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 +9 to +10
- "src/quant_advisor_research/preview_bundle.py"
- "src/quant_advisor_research/advisory_report.py"

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 Add transitive modules to the path filter

In the pull_request workflow path filter I checked, changes to transitive validation code such as src/quant_advisor_research/contracts.py do not trigger this job, even though preview_bundle.py imports validate_advisory_report from that module for build_preview_bundle()/read_preview_bundle() validation. A PR that only changes the contract/schema validation can therefore bypass the upload/download artifact acceptance; please include the transitive modules or broaden the src/quant_advisor_research/** filter.

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