Skip to content

feat: add Phase 1 V2 clean-cutover snapshot contract - #202

Closed
Pigbibi wants to merge 4 commits into
mainfrom
codex/qsl-clean-cutover-phase1-v2-20260726
Closed

feat: add Phase 1 V2 clean-cutover snapshot contract#202
Pigbibi wants to merge 4 commits into
mainfrom
codex/qsl-clean-cutover-phase1-v2-20260726

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Scope

  • Add the frozen clean-cutover schema, offline materializer, strict readback, and synthetic bounded tests.

Validation

  • PYTHONPATH=src python3 -m pytest tests/test_soxl_tqqq_clean_cutover_snapshot.py -q
  • python3 -m ruff check src/us_equity_snapshot_pipelines/soxl_tqqq_clean_cutover_snapshot.py tests/test_soxl_tqqq_clean_cutover_snapshot.py
  • python3 -m compileall -q src/us_equity_snapshot_pipelines/soxl_tqqq_clean_cutover_snapshot.py
  • Schema JSON parse and exact three-file scope check.

No providers, credentials, real data, calendar generation, strategy, plugin activation, or live actions.

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

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

Merge allowed: 2 finding(s) reported but none are blocking

ℹ️ Other Findings

1. 🟡 [MEDIUM] Security in src/us_equity_snapshot_pipelines/soxl_tqqq_clean_cutover_snapshot.py

_reserve() only protects the final path component with O_NOFOLLOW. os.lstat(parent) and os.open(parent, ...) still traverse symlinks in ancestor components, so a caller can pass a destination such as trusted/alias/snapshot where alias is a symlink and make the materializer publish outside the caller-bound tree. That defeats the new filesystem-state safety contract and can place snapshots in an unintended directory without tripping validation. (line 290)

Suggestion: Resolve the destination one component at a time with openat(..., O_NOFOLLOW) plus inode checks (or openat2(..., RESOLVE_NO_SYMLINKS|RESOLVE_BENEATH) on Linux) instead of validating only the final component.

2. 🟡 [MEDIUM] Reliability in src/us_equity_snapshot_pipelines/soxl_tqqq_clean_cutover_snapshot.py

_read_member() does a stat(..., follow_symlinks=False) and then a blocking os.open() on the same name. If another process replaces manifest.json, payload.json, or publication.json with a FIFO or device after the stat check, strict_readback_clean_cutover_snapshot() can block indefinitely before the post-open fstat() runs. This makes the new public readback path vulnerable to a local DoS on any mutable snapshot directory. (line 397)

Suggestion: Open publication members with _NONBLOCK, validate os.fstat(fd) on the opened descriptor before reading, and avoid relying on the pre-open stat result for safety-critical checks.


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: 2266a334b0

ℹ️ 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 thread src/us_equity_snapshot_pipelines/soxl_tqqq_clean_cutover_snapshot.py Outdated
Comment thread src/us_equity_snapshot_pipelines/soxl_tqqq_clean_cutover_snapshot.py Outdated
Comment thread src/us_equity_snapshot_pipelines/soxl_tqqq_clean_cutover_snapshot.py Outdated
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: 92a89b19c0

ℹ️ 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 thread src/us_equity_snapshot_pipelines/soxl_tqqq_clean_cutover_snapshot.py Outdated
@Pigbibi

Pigbibi commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Fail-closed: the sole authorized consolidated remediation budget is exhausted and the guarded merge remained BLOCKED after exact-head checks. Closing unmerged and parking per authority.

@Pigbibi Pigbibi closed this Jul 26, 2026
@Pigbibi Pigbibi reopened this Jul 26, 2026
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: ae01237735

ℹ️ 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 thread src/us_equity_snapshot_pipelines/soxl_tqqq_clean_cutover_snapshot.py Outdated
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: d09aea1560

ℹ️ 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 +199 to +200
normalized_row = {"session": session, "symbol": symbol, "adjusted_close": adjusted_close}
serialized_rows_size += len(_canonical(normalized_row))

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 invalid symbols before JSON serialization

When a caller supplies a row whose symbol is a very large string, _canonical(normalized_row) duplicates and UTF-8-encodes the entire value before the size bound is checked, even though the symbol cannot match either fixed symbol for the pair. A sufficiently large value can therefore cause excessive allocation or MemoryError despite MAX_MEMBER_BYTES; validate membership in PAIR_SYMBOLS[pair_id] or conservatively bound the string before serialization.

Useful? React with 👍 / 👎.

Comment on lines +435 to +436
_require_string(manifest["source_identity"], "source_identity")
_require_string(manifest["producer_identity"], "producer_identity")

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 Apply identity bounds during strict readback

When a self-consistent publication contains a source_identity or producer_identity longer than MAX_MEMBER_BYTES // 16 but still fits within the one-megabyte manifest limit, strict readback accepts it after the hashes are recomputed, while _build_members rejects the same identity through _require_identity. Use that validator here too so strict readback does not admit evidence the materializer cannot produce.

Useful? React with 👍 / 👎.

@Pigbibi

Pigbibi commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Closing unmerged per final remediation authority hard-stop: complete review produced new unresolved findings after the final authorized batch; no further remediation is authorized.

@Pigbibi Pigbibi closed this Jul 26, 2026
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