feat: add strict vNext result/store contract - #254
Conversation
Co-Authored-By: Codex <noreply@openai.com>
🤖 Codex PR Review🚫 Merge blocked: 1 serious issue(s) found in high-risk files ⚖️ Codex Review Arbitration🚫 block: 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🟠 [HIGH] Logic 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: 7e8624d07d
ℹ️ 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 isinstance(value, float) and math.isfinite(value): | ||
| return value | ||
| if isinstance(value, tuple): | ||
| return [_param_value(item) for item in value] |
There was a problem hiding this comment.
When any parameter is array-valued by passing a tuple, this branch serializes it into a JSON list in to_wire(), but decode_vnext_wire() feeds that list back through _param_value(), which rejects lists. As a result, the contract can emit wire data that its own decoder rejects; the stored list is also reachable via contract.params and can be mutated after construction, changing the digest/key of a supposedly frozen result contract.
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: e63ead352f
ℹ️ 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".
| def _param_value(value: Any) -> Any: | ||
| if value is None: | ||
| raise VNextContractError() | ||
| if isinstance(value, (str, bool, int)): |
There was a problem hiding this comment.
When a parameter value is a Python string containing a lone surrogate, this branch accepts it even though _canonical_json(..., ensure_ascii=False).encode() later raises UnicodeEncodeError while computing key or to_wire(). That lets callers construct a supposedly strict JSON-safe contract that cannot generate its deterministic wire/key and fails with an unsanitized low-level exception; validate parameter strings for UTF-8/JSON encodability before accepting them.
Useful? React with 👍 / 👎.
| object.__setattr__(self, "execution_timing", ExecutionTiming(self.execution_timing)) | ||
| object.__setattr__(self, "persist_mode", PersistMode(self.persist_mode)) | ||
| except Exception as exc: | ||
| raise VNextContractError() from exc |
There was a problem hiding this comment.
When an invalid execution_timing or persist_mode comes from untrusted wire data, this from exc preserves the underlying ValueError as __cause__; any traceback or structured error log will still include the raw rejected value even though the contract documents sanitized validation errors. Treat these enum coercion failures as expected validation failures and raise without chaining (or from None) so secrets embedded in malformed payloads are not echoed.
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: c1a76ae279
ℹ️ 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".
| def _text(value: Any) -> str: | ||
| if not isinstance(value, str) or not value or len(value) > _MAX_SEGMENT: | ||
| raise VNextContractError() | ||
| if any(ord(char) < 32 or char in "/\\" for char in value): |
There was a problem hiding this comment.
When untrusted wire data contains an escaped lone surrogate in computed_at, this check accepts it because it only rejects control characters and slashes. Since computed_at is excluded from _identity_payload(), the digest path never exercises _canonical_json(...).encode(), so decode_vnext_wire() can return a contract whose to_wire() payload cannot be UTF-8 JSON-encoded despite the strict JSON-safe wire contract; validate/sanitize these metadata strings before accepting them.
Useful? React with 👍 / 👎.
| if isinstance(value, float) and math.isfinite(value): | ||
| return value |
There was a problem hiding this comment.
When a parameter value is -0.0, this branch accepts it unchanged. Python mapping/dataclass equality treats a contract with -0.0 params as equal to the same contract with 0.0, but _canonical_json emits different tokens (-0.0 vs 0.0), so equal contracts produce different identity_digest/key values and can create duplicate result-store entries for the same logical parameters; normalize or reject negative zero before hashing.
Useful? React with 👍 / 👎.
| if isinstance(value, (str, bool)): | ||
| return value | ||
| if isinstance(value, int): | ||
| if abs(value) > MAX_SAFE_JSON_INTEGER: | ||
| raise VNextContractError() | ||
| return value |
There was a problem hiding this comment.
When the same parameter is provided as True versus 1 (or False versus 0), these branches preserve different JSON wire types, but Python mapping/dataclass equality still considers the resulting contracts equal. That leaves equal contracts with different identity_digest/key values, so any caller comparing decoded contracts, de-duplicating contract lists, or using equality in tests can treat two distinct result-store identities as interchangeable; make equality type-aware or reject/normalize these ambiguous numeric cases.
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: 92e7cbf3e4
ℹ️ 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 not isinstance(self.result_identity_version, int) or isinstance(self.result_identity_version, bool) or self.result_identity_version < 1: | ||
| raise VNextContractError() | ||
| if not isinstance(self.param_version, int) or isinstance(self.param_version, bool) or self.param_version < 1: | ||
| raise VNextContractError() |
There was a problem hiding this comment.
When untrusted wire data or direct construction supplies result_identity_version or param_version above MAX_SAFE_JSON_INTEGER, these checks still accept it even though oversized parameter ints are rejected for JSON safety. The resulting wire payload and identity digest then contain JSON numbers that double-based clients can round before recomputing or verifying the contract, so apply the same safe-integer upper bound to both version fields.
Useful? React with 👍 / 👎.
|
|
||
| @property | ||
| def key(self) -> str: | ||
| return f"{VNEXT_NAMESPACE}/{self.domain}/{self.canonical_profile}/{self.strategy_id}/{self.run_id}/{self.execution_timing.value}/i{self.result_identity_version}/p{self.param_version}/{_digest(self._identity_payload())}.json" |
There was a problem hiding this comment.
When callers pass a str enum or other str subclass with a custom __str__ for domain, canonical_profile, strategy_id, etc., _segment() accepts it and __post_init__ stores it unchanged. This interpolation then uses the subclass string form for the key (for example Profile.SOXL), while to_wire() and identity_digest JSON-encode the underlying string value (SOXL), so serializing and decoding the same contract changes key and can orphan stored results. Reject non-exact strings or coerce to plain strings before building the key.
Useful? React with 👍 / 👎.
|
PERMANENT_FREEZE: remaining vNext contract redesign evidence requires safe-range validation for all identity-bearing integers and one canonical UTC timestamp serialization. No further code, rerun, reslice, merge, bypass, or thread resolution. Preserve this PR as frozen evidence. |
1 participant
Scope
QPK-N1 clean-slate vNext strict result/store contract foundation only.
qpk-vnext/result/v1; no legacy key reuse or fallback.Source of truth:
/Users/lisiyi/Documents/Codex/2026-07-14/ba-2/outputs/quant_system_clean_slate_vnext_decision_2026-07-15.mdCommit:
7e8624d07df00d9385293a50cf5dfca36f310782Non-goals
No legacy reads/migration/index, PerformanceStore integration, caller/orchestrator/runner, real artifacts, broker/live/account, or new dependencies.
Validation
6 focused tests; full 613 passed, 1 skipped; ruff, compileall, diff-check passed.