Repository navigation
fix(trading): harden R3 verification before freeze - #911
Conversation
seathatflowsinourveins
added
the
lane:trading
e13d1a3 to
58ea680
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 58ea680098
ℹ️ 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".
| boundaries.append({"file": file, "pointer": pointer, "reference": reference, | ||
| "scope": "external, historical or separately pinned source; not read by this verifier"}) | ||
| try: | ||
| local = not Path(reference).is_absolute() and not reference.startswith("attempt-") and (root / reference).is_file() |
There was a problem hiding this comment.
When an optional reference contains an overlong path component, CPython 3.14's Path.is_file() returns False instead of raising ENAMETOOLONG, so this branch classifies the malformed reference as an external boundary and the verifier exits 0 with PASS. Reproducing tests.test_r3_verify with the installed Python 3.14.4 fails test_cli_long_optional_reference_returns_json_failure, while 3.12.13 and 3.13.13 pass; use a metadata probe that preserves the OSError (such as stat) or validate the reference explicitly so the promised JSON failure remains version-independent.
AGENTS.md reference: AGENTS.md:L5-L7
Useful? React with 👍 / 👎.
| if path.is_symlink(): | ||
| errors.append(f"file is a symlink: {name}") | ||
| return None | ||
| if not path.is_file(): | ||
| errors.append(f"missing or not a regular file: {name}") | ||
| return None | ||
| return path.read_bytes() |
There was a problem hiding this comment.
When a manifest member is below a symlinked directory (for example, link/extra.json where link points outside the packet), path.is_symlink() is false for the final regular file, so read_bytes() opens and hashes the external file. A resealed packet containing such a member returns PASS, contrary to the verifier's symlink rejection and no-external-source boundary; inspect every path component or otherwise enforce that the opened member remains inside the packet without traversing symlinks.
Useful? React with 👍 / 👎.
| location = (file, pointer) | ||
| if location in seen: | ||
| return |
There was a problem hiding this comment.
Because seen is keyed by the unescaped pointer string, a JSON key containing / can collide with a genuinely nested location and cause a binding to be skipped. For example, adding a root key named R2_bound_contracts/construction whose value is {"path":"malicious-local.json","sha256":"NOT-A-DIGEST"} collides with the mandatory nested construction pointer; after resealing, the verifier returns PASS and reports neither the invalid digest nor the reference. Deduplicate using the tuple of actual traversal components, or RFC 6901-escape keys before constructing pointer identities.
Useful? React with 👍 / 👎.
| if key in obj: | ||
| if (file, pointer) in UNFILLED_INPUTS and obj[key] is None and obj["sha256"] is None: | ||
| break |
There was a problem hiding this comment.
When either exempt PIT-04 object keeps path and sha256 null but also adds another binding field such as artifact, this break skips the whole binding check. Updating the enclosing contract digests and carriers then yields PASS even with artifact: "R2-CONSTRUCTION.json" and no digest, so a partial binding can still hide in the placeholders despite the new regression's stated invariant. Apply the exemption only when no other path/artifact/pdf/receipt field is present, or validate the exact placeholder shape.
Useful? React with 👍 / 👎.
| try: | ||
| root = args.root.resolve() if args.root is not None else Path(__file__).resolve().parent | ||
| except OSError as error: |
There was a problem hiding this comment.
When --root names a symlink loop on the repository's Python 3.12 runtime, Path.resolve() raises RuntimeError, not OSError, so this exception handler is bypassed and the CLI emits a traceback with no JSON diagnostic. This is reproducible with two mutually linked paths on Python 3.12.13; catch the version-specific RuntimeError as well so inaccessible root resolution consistently honors the documented JSON-failure contract.
AGENTS.md reference: AGENTS.md:L5-L7
Useful? React with 👍 / 👎.
1 participant
Scope
The R3 verifier must return a JSON diagnostic when file access or declared packet integrity fails. This freeze-preparation PR requires valid digests/current-packet joins/carriers, corrects repeated binding counts, and handles missing, malformed or inaccessible inputs. The successor additionally protects filesystem metadata probes and CLI root resolution from
OSErrortracebacks.06aca3df6c9ba0edea13719e4b7fb8169bba9838, the corrected main target for this rebase. docs(trading): revise R3 census to 2018-2023 #900 landed at36e36ea498e23ffd5bfc103035a2da883a6a08e4. The reviewed predecessor of this micro-fix is0a8589350f1a7b18823b231ef3ea81e865aaad79; it includes the earliera3b8e60083c6e9f728bac9f2905b99c5304684dbcorrections.lane:trading.tests/test_r3_verify.py; and their evidence registrations.SHA256SUMS. Nonregular inputs are rejected before opening. File reads, symlink/regular-file checks, optional-target probes and CLI root/default resolution catchOSErrorand return a named JSON error. Verification exits 0 on PASS, 1 on FAIL.The subsequent micro-fix adds direct tests for member bytes changed without resealing, a well-formed wrong ROW contract digest, and a join retargeted to another present member with its correct digest. JSON decoding now reports RecursionError and ValueError as named failures, and recursive traversal reports its own nesting-limit failure. Tests cover native deep JSON/large integers and deterministic decoder/traversal limits. The docstring and ROW source identify the actual
json.loads/hashlib.sha256primitives, retain the CC-R3-1 lineage and local-only scope, and the unuseddigest()function is removed.Rebase head
58ea680098d0833b5e75fe8829c5167113726112preserves the seven PR file bytes from reviewed heade13d1a3c1aa7676b79240fa03cd4afb892d9c4a6and rebuildsmanifests/evidence.jsonfrom corrected main plus the seven owned registrations. The intermediate unpublished rebase onto2ecce6ecwas replaced before the one push. The rebase requires both designated micro reads on its delta, green CI on this head and the CC's cue.Two further SFH disclosures:
assert_cli_failuregains optionalinterpreter_args=(). Its subprocess argv is[sys.executable, *interpreter_args, SCRIPT, ...]; the empty default preserves the prior invocation. The huge-integer regression passes-X int_max_str_digits=4300to make the child interpreter's limit explicit.verifier.sourcereplace the former CPython v3.12.3 attribution (json.load/hashlib.file_digest) with v3.13.16 (json.loads/hashlib.sha256). This is a source-attribution correction; no interpreter installation, runtime-lock change or historical receipt rewrite is performed.Explicit behavior and output changes
sha256, without a path/artifact/pdf/receipt sibling, must be a 64-character lowercase hexadecimal string. It is validated without becoming an internal join..jsoncarrier member must have an object root, including optional members; array/scalar roots fail.internal_binding_countandbinding_count_scope.internal_bindingsis deduplicated by source file and digest-pointer location, giving 17 locations rather than 27 repeated checks for the current packet.manifest_sha256can be null when the canonical carrier/root cannot be read;semantic_checkscan be partial or empty after an early failure. Those conditions accompany FAIL diagnostics and exit 1.SOTA sources
python/cpython v3.13.16 json and decoder: installed parser/JSONDecodeError and the native interpreter used for returned failure behavior.
CPython v3.13.16 pathlib, hashlib, re and unittest: filesystem predicates/reads, exact byte digests, whole-string validation and the native regression harness. The installed pathlib predicates may propagate PermissionError/ENAMETOOLONG; those operations now share protected diagnostic paths.
Published verifier at36e36ea4, protocol and ROW14: reference integration and normative schema. Existing
scripts/host_receipts.py:register_filerefreshes the dependent hashes. No new dependency or runner is added.Designated GPT verdict SHA256 406d3bee5bf70ad9617d6749c5206bcae30c7625c8ab683fae6f9b1e36ab53e2 and api-actions J8 silent-failure report SHA256 3e63cf3b7621e1ae39edd756f12c592c329fa338d37dda08d78160b37d90d7f7 identify the reviewed failure paths and behavior/test gaps. This successor implements the released items; the PTA result and both micro verdicts remain separate.
CC micro-read SHA256 192c02a0ff10b34dc4cb18b78a5833b9217fe24965639f73aa26b24c03459084 and J8 test-analyzer rerun SHA256 53459435782bb7a5428b1da4a85e437d76d6cb0bfb58c3d43f89a3dccf763271 identify the released F1/F2/F10/F6/P3 micro items. The prior 40-case verification remains predecessor evidence; the new head requires its delta read, J8 micro, CI and the CC cue.
J8 micro report SHA256 626fbe4ca7fe9071e803cf2313837cc0ae518e0da284af171262fb5b7533e2af, section3: the two additional disclosures above. The report was hash-verified before use.
Evidence-class table
VERIFIER.verify(PACKET)without resealing and executes the native CLI on that same packet, requiring PASS/exit0 and17uniquejoins.scripts/validate.py; actual returned output retained.Fresh checks on the rebased committed head:
tests.test_r3_verify47/47,scripts/validate.pyrc0, native verifier PASS with 17 bindings and nine semantic checks, and each of the three carriers 12/12OK. The classifier41/pre-push16/evidence-manifest5 results below remain the retained predecessor runs; they were not rerun for this rebase. The PR packet/test bytes are unchanged, and CI acceptance remains pending on the new head.Local commands run
Final verifier module:47cases; classification41, pre-push16 and evidence-manifest5. All returned rc0 on the implementation predecessor; the fresh rebase results are specified above. Validatorrc0; current verifierPASS,12members/17distinctbindings/9semanticchecks; nativecarrier12/12OK. Locally authored mutation/injection tests and exact-packet integration checks do not supply upstream scientific acceptance, either micro verdict or CI acceptance.
Decision record
The packet README records the verifier error/count contract, numeric exits and evidence limits. The chosen approach extends the existing stdlib integration, preserving its scientific inputs. Fresh GPT/CC micro reads, CI, any J8 micro required by the PTA result and the CC's cue remain pending. #867 is closed with a pointer to landed #900.
No census count, fit, acquisition, vendor contact, purchase or broker action was performed. Sanitized returned command evidence remains with the custodian; private locators and protected study inputs are excluded from this PR.