feat: enforce TQQQ calendar endpoint trust - #194
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review🚫 Merge blocked: 2 serious issue(s) found in high-risk files 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🔴 [CRITICAL] Security 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: 148e1fce74
ℹ️ 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 sums != {"prices.csv": member_hashes["prices.csv"], "trust.json": member_hashes["trust.json"]}: | ||
| _invalid("STRICT_READBACK_FAILED") |
There was a problem hiding this comment.
When a persisted snapshot is modified together with its metadata, this check provides no tamper protection because both sha256sums.json and trust.json are stored in the same untrusted directory. For example, changing an adjusted-close value, updating trust.json's prices_sha256, and regenerating sha256sums.json passes verification with the original three authority digests, since none of those external digests commits to the snapshot contents. Require the caller to supply a trusted digest of trust.json (as the existing verifier does for its manifest) or otherwise authenticate the output.
Useful? React with 👍 / 👎.
| normalized[PRICE_FIELD] = pd.to_numeric(normalized[PRICE_FIELD], errors="coerce") | ||
| if normalized[PRICE_FIELD].isna().any() or not normalized[PRICE_FIELD].map(math.isfinite).all(): | ||
| _invalid("EXACT_SESSION_SET_MISMATCH") |
There was a problem hiding this comment.
With otherwise valid calendar and session inputs, zero and negative adjusted-close values pass this validation because they are finite; native boolean values also pass because pd.to_numeric retains them and math.isfinite(True) is true. The materializer then persists and successfully verifies these invalid market prices, which can corrupt downstream return calculations. Apply the same boolean/type and strictly-positive checks used by _normalized_prices before writing the trusted snapshot.
Useful? React with 👍 / 👎.
| expected_endpoint_packet_sha256=expected_endpoint_packet_sha256, | ||
| expected_runtime_source_identity_sha256=expected_runtime_source_identity_sha256, | ||
| ) | ||
| os.replace(temporary, destination) |
There was a problem hiding this comment.
If another process creates the destination as an empty directory after the earlier existence check but before this call, POSIX os.replace removes that directory and installs the snapshot in its place. This violates the advertised immutable-create behavior and bypasses IMMUTABLE_CREATE_FAILED under a creation race; use an atomic no-replace installation strategy rather than a check followed by replacing rename.
Useful? React with 👍 / 👎.
| endpoint_observed = _parse_utc(endpoint["endpoint_observed_at_utc"], "CALENDAR_SCHEMA_INVALID") | ||
| last_close = _parse_utc(endpoint["required_last_completed_close_utc"], "CALENDAR_SCHEMA_INVALID") | ||
| next_close = _parse_utc(endpoint["next_session_close_utc"], "CALENDAR_SCHEMA_INVALID") | ||
| if endpoint_observed > observation_time or last_close > observation_time or observation_time >= next_close: |
There was a problem hiding this comment.
When a correctly hashed endpoint packet contains inconsistent next_session and next_session_close_utc values, the declared session is never parsed or compared with the close timestamp or last completed session. A packet that declares the next session as 2026-07-27 but supplies a close in 2099 therefore keeps passing the freshness check until 2099, allowing a stale calendar and missing price history to be materialized. Validate that next_session is canonical, follows the last completed session, and corresponds to next_session_close_utc before using that close as the freshness boundary.
Useful? React with 👍 / 👎.
| for symbol, expected in expected_sessions.items(): | ||
| actual = normalized.loc[normalized["symbol"].eq(symbol), "session"].dt.strftime("%Y-%m-%d").tolist() | ||
| if actual != expected: | ||
| _invalid("EXACT_SESSION_SET_MISMATCH") |
There was a problem hiding this comment.
When callers provide the exact required rows in a different order, this list comparison rejects the input even though the function subsequently sorts the normalized frame before writing it. DataFrame row order is not part of the session set, and ordinary concatenation or sampling can reorder otherwise valid QQQ/TQQQ data, so compare sets or sort each symbol's sessions before checking coverage.
Useful? React with 👍 / 👎.
| try: | ||
| names = tuple(sorted(member.name for member in output.iterdir())) | ||
| members = {name: (output / name).read_bytes() for name in _TRUSTED_OUTPUT_FILENAMES} | ||
| except OSError as exc: | ||
| raise SnapshotValidationError("STRICT_READBACK_FAILED") from exc | ||
| if names != _TRUSTED_OUTPUT_FILENAMES or any((output / name).is_symlink() for name in _TRUSTED_OUTPUT_FILENAMES): |
There was a problem hiding this comment.
When an untrusted or damaged snapshot contains a FIFO or device node under one of the expected filenames, read_bytes() runs before any regular-file check and can block indefinitely or consume an unbounded stream instead of failing closed. The later symlink-only test does not protect against these file types; check that every member is a regular, non-symlink file before opening any of them, as the existing R1 verifier does.
Useful? React with 👍 / 👎.
| for line in lines: | ||
| entry = _parse_json_object(line.encode("utf-8"), "calendar") | ||
| if set(entry) != {"schema", "session", "open_utc", "close_utc"} or entry["schema"] != _CALENDAR_SCHEMA: | ||
| _invalid("CALENDAR_SCHEMA_INVALID") |
There was a problem hiding this comment.
When a digest-authorized calendar contains malformed NDJSON, this parser raises SnapshotValidationError("invalid calendar") rather than the trust protocol's CALENDAR_SCHEMA_INVALID status. Callers that classify the documented red statuses therefore cannot recognize this calendar-schema failure even though equivalent structural defects take the expected path; catch the parser error here and re-raise it with CALENDAR_SCHEMA_INVALID.
Useful? React with 👍 / 👎.
| destination.parent.mkdir(parents=True, exist_ok=True) | ||
| temporary = Path(tempfile.mkdtemp(prefix=f".{destination.name}.", dir=destination.parent)) | ||
| try: |
There was a problem hiding this comment.
When the destination parent is unwritable, absent on a read-only filesystem, or out of space, mkdir or mkdtemp raises a raw OSError before the cleanup block rather than SnapshotValidationError("IMMUTABLE_CREATE_FAILED"). This bypasses the fail-closed error metadata and status handling added for the trusted consumer in common deployment failures; normalize destination setup and installation I/O errors to the immutable-create status.
Useful? React with 👍 / 👎.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e2e8de71d
ℹ️ 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".
| endpoint = _parse_json_object(endpoint_raw, "endpoint packet") | ||
| runtime = _parse_json_object(runtime_raw, "runtime source identity") |
There was a problem hiding this comment.
When a digest-authorized endpoint packet or runtime anchor contains malformed JSON, duplicate keys, or a non-object root, these calls propagate invalid endpoint packet or invalid runtime source identity from _parse_json_object rather than the corresponding CALENDAR_SCHEMA_INVALID or RUNTIME_SOURCE_IDENTITY_MISMATCH status used by the subsequent structural checks. Consumers that classify the trust protocol's red statuses therefore cannot recognize these authority failures; catch and remap the parser errors here.
Useful? React with 👍 / 👎.
| sums = _parse_json_object(members["sha256sums.json"], "trusted sha256sums") | ||
| trust = _parse_json_object(members["trust.json"], "trusted snapshot") |
There was a problem hiding this comment.
When the supplied external output digest matches a snapshot whose sha256sums.json or trust.json is malformed, these calls propagate generic invalid trusted ... errors from _parse_json_object instead of STRICT_READBACK_FAILED. This makes an authenticated but structurally invalid persisted snapshot unclassifiable by consumers handling the verifier's strict-readback status; remap metadata parsing failures at this boundary.
Useful? React with 👍 / 👎.
| if opened >= closed or closed.date().isoformat() != session: | ||
| _invalid("CALENDAR_SCHEMA_INVALID") |
There was a problem hiding this comment.
When a digest-authorized XNYS calendar row has an open_utc on a different date from its declared session but still before close_utc, this condition accepts it because only the close date is bound to the session. Such an internally inconsistent calendar can therefore drive the exact-session snapshot as trusted input; require the normalized open date to equal session as well.
Useful? React with 👍 / 👎.
|
Closing this PR unmerged under CEO authority Exact evidence at closure: base E1 remediation budget is exhausted (1/1). This is a close-and-park decision, not a review override; no review threads were replied to or resolved, and no merge or additional remediation is authorized. |
1 participant
Summary
Verification
python3 -m ruff check src/us_equity_snapshot_pipelines/tqqq_r1_snapshot.py tests/test_tqqq_r1_snapshot.pyPYTHONPATH=src python3 -m compileall -q src/us_equity_snapshot_pipelines/tqqq_r1_snapshot.pygit diff --check