Add held-out chemistry validation boundary - #11
erinepshovel-code wants to merge 9 commits into
Conversation
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: 1cc98feea0
ℹ️ 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".
|
|
||
| The chemistry app is a **discovery/reference surface**, not an EPAC dependency and not the sole authority. Facts used for scoring carry an authority plus locator. Missing prediction, missing provenance, unsupported comparison, or incomparable data is `UNRESOLVED`; it is never silently counted as success. | ||
|
|
||
| The checked-in `data/heldout_chemistry_oracle.json` is intentionally empty. Populate it only with facts selected independently of EPAC outputs. This prevents choosing test cases after seeing what EPAC predicts. |
There was a problem hiding this comment.
Because the checked-in oracle is empty while the prediction commitment exposes the predictions in plaintext, an oracle curator can see the outputs and then choose cases, comparison kinds, or numeric tolerances that produce SURVIVED; the receipt only hashes that post-hoc oracle and cannot establish the claimed independent selection or preregistered comparator. Freeze or externally bind the case inventory and comparison rules before predictions are revealed rather than claiming that an empty corpus prevents selection bias.
AGENTS.md reference: AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| "predicted": predictions[case_id], | ||
| "expected": case.get("expected"), | ||
| "provenance": provenance, |
There was a problem hiding this comment.
When predictions, expected values, or provenance contain mutable objects, these fields are inserted into the receipt by reference. Mutating the original commitment or oracle after comparison therefore changes the returned receipt while leaving its status, counts, and receipt_sha256 unchanged—for example, appending to an expected list silently rewrites preserved evidence. Deep-copy or canonicalize the payload when constructing the receipt.
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| if case_id not in predictions: | ||
| results.append({"id": case_id, "status": "UNRESOLVED", "reason": "no frozen prediction"}) | ||
| continue | ||
| status = _compare(case.get("expected"), predictions[case_id], case.get("comparison", {"kind": "exact"})) |
There was a problem hiding this comment.
For an oracle case that omits expected, case.get("expected") supplies None; if the frozen prediction is also None, the default exact comparator records SURVIVED. This silently converts missing evidence into a successful validation despite the documented missing-data policy, so key presence must be checked before comparison and the case classified as UNRESOLVED.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
| 1. Run EPAC construction and produce predictions keyed by stable case IDs. | ||
| 2. Call `freeze_predictions(..., source_identity=<exact EPAC head/receipt>)`. | ||
| 3. Persist the returned commitment. Its SHA-256 binds the predictions. | ||
| 4. Only then expose/load an oracle corpus. | ||
| 5. Call `compare_after_freeze(commitment, oracle)`. |
There was a problem hiding this comment.
The usage section contains only calls with ellipses and undefined objects; it does not show imports, a valid prediction/oracle payload, persistence, packaged-oracle loading, or receipt output. Consequently, a user cannot copy and run the documented workflow from either a checkout or the installed distribution. Add a complete executable example covering freeze, persistence/reload, oracle load, comparison, and receipt preservation.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
| if kind == "exact": | ||
| return "SURVIVED" if predicted == expected else "FALSIFIED" |
There was a problem hiding this comment.
Python equality treats booleans as integers, so an exact chemistry case with expected value 1 records a boolean prediction of True as SURVIVED (and False likewise matches 0). A predictor returning a classification flag instead of a numeric valence can therefore produce false validation evidence; exact comparison should require compatible JSON value types in addition to equality.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| for case in cases: | ||
| case_id = case.get("id") |
There was a problem hiding this comment.
If the oracle contains the same stable ID more than once, the loop scores every occurrence independently, so one prediction can be counted multiple times or even appear as both SURVIVED and FALSIFIED under conflicting duplicate entries. This makes aggregate counts dependent on duplicated rows rather than unique cases and permits accidental or deliberate weighting; validate case-ID uniqueness before producing any standings.
AGENTS.md reference: AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| @@ -0,0 +1,116 @@ | |||
| """Two-phase held-out chemistry validation for EPAC. | |||
There was a problem hiding this comment.
This new public experiment module has no MODULE_BUILD declaration, even though the applicable .agents/skills/meta-module-build/SKILL.md contract requires new modules to colocate their public/internal surfaces, boundaries, tests, rollout, rollback, and unresolved status. Without that declaration, the repository's metadata tooling cannot expose the module's ownership or risk and documentation coverage, leaving the implementation incomplete under the required workflow.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| tolerance = rule.get("absolute_tolerance") | ||
| if not isinstance(tolerance, (int, float)) or isinstance(tolerance, bool) or tolerance < 0: | ||
| return "UNRESOLVED" | ||
| try: | ||
| return "SURVIVED" if abs(float(predicted) - float(expected)) <= tolerance else "FALSIFIED" |
There was a problem hiding this comment.
The numeric comparator accepts non-finite tolerances and operands: an infinite tolerance makes every finite difference SURVIVED, while a NaN tolerance or operand makes the comparison decisively FALSIFIED. Python can produce these values from programmatic inputs or JSON numbers outside float range, and they represent incomparable rather than measured chemistry data; require math.isfinite for the tolerance and both converted operands and otherwise return UNRESOLVED.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
| provenance = case.get("provenance", {}) | ||
| authoritative = provenance.get("authority") | ||
| locator = provenance.get("locator") |
There was a problem hiding this comment.
An oracle naturally representing unavailable provenance as "provenance": null aborts the entire comparison with AttributeError because the code immediately calls .get on None. The documented contract says missing provenance yields UNRESOLVED, so validate that provenance is a mapping and emit the unresolved result instead of preventing every case from receiving a receipt.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
| if commitment["schema"] != "epac.heldout-prediction-commitment" or commitment["version"] != "v1": | ||
| raise ValueError("unsupported prediction commitment") | ||
| unsigned = {k: commitment[k] for k in ("schema", "version", "source_identity", "predictions")} | ||
| if _digest(unsigned) != commitment["commitment_sha256"]: |
There was a problem hiding this comment.
freeze_predictions rejects an empty source identity, but verify_commitment does not enforce that invariant, so a deserialized or externally created envelope with source_identity: "" and a matching digest passes comparison and produces standings with no identifiable prediction source. Because compare_after_freeze accepts arbitrary mappings rather than only constructor outputs, the verifier must require a nonempty string identity before accepting the commitment.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
|
@codex review Please review the current exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1281ddd8e3
ℹ️ 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".
| normalized.append({ | ||
| "id": case_id, | ||
| "domain": deepcopy(case.get("domain")), | ||
| "comparison": _normalized_comparison(case), |
There was a problem hiding this comment.
When a caller supplies a plan comparator such as {"kind":"exact","expected":7}, the top-level extra-field check passes and this line copies the expected label into the persisted validation plan, which is then exposed to the prediction side. The resulting receipt can therefore claim held-out validation even though oracle data crossed the construction boundary; whitelist the allowed keys for each comparator kind before hashing the plan.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| left = _finite_number(predicted) | ||
| right = _finite_number(expected) | ||
| if tolerance is None or tolerance < 0 or left is None or right is None: | ||
| return "UNRESOLVED" | ||
| return "SURVIVED" if abs(left - right) <= tolerance else "FALSIFIED" |
There was a problem hiding this comment.
For JSON integer operands beyond the exact binary64 range, _finite_number rounds both values before comparison; for example, predicted 9007199254740993 and expected 9007199254740992 with tolerance 0 are both converted to the same float and recorded as SURVIVED. Preserve integer arithmetic when possible or reject conversions that are not lossless so a genuine disagreement is not reclassified as success.
AGENTS.md reference: AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| provenance = case.get("provenance") | ||
| if not isinstance(provenance, Mapping) or not provenance.get("authority") or not provenance.get("locator"): |
There was a problem hiding this comment.
The new mapping/truthiness guard still accepts whitespace-only provenance, so a case with {"authority":" ","locator":"\t"} can be recorded as SURVIVED even though it has no identifiable evidence source. Validate both fields as nonempty strings after stripping whitespace, as is already done for other identities, so unknown authority remains unresolved.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
| def load_oracle(path: str | Path) -> dict[str, Any]: | ||
| """Comparison-side helper. Construction modules must not call this.""" | ||
| return json.loads(Path(path).read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When an external oracle file repeats an object key, such as "expected": 1 followed by "expected": 2, json.loads silently retains only the last value. The comparison and oracle_sha256 then bind that collapsed interpretation rather than detecting ambiguous source evidence, while another custodian or parser may interpret the same file differently; load with duplicate-key detection and reject the oracle instead.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
| # public_surface: freeze_validation_plan, freeze_predictions, verify_commitment, compare_after_freeze, load_oracle, load_packaged_oracle | ||
| # internal_surface: _canonical, _digest, _verify_validation_plan, _json_exact_equal, _json_token, _finite_number, _compare | ||
| # auth_boundary: none | ||
| # storage_boundary: none |
There was a problem hiding this comment.
The newly added MODULE_BUILD declaration now exists, but it advertises storage_boundary: none even though the public load_oracle and load_packaged_oracle surfaces read persistent files and package resources. The applicable .agents/skills/meta-module-build/SKILL.md schema classifies this effect as read; leaving it as none makes metadata consumers report an incorrect permission boundary.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| try: | ||
| resource = files("epac_data").joinpath("heldout_chemistry_oracle.json") | ||
| return json.loads(resource.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When the current source checkout is run in an interpreter that also has an older interdependency-epac installed, files("epac_data") resolves that installed package before the checkout fallback is considered. The documented source workflow can therefore load a stale oracle, or raise FileNotFoundError when the installed package predates this resource, instead of reading this checkout's data/heldout_chemistry_oracle.json; prefer the sibling source resource when present or verify that the module and data package share a distribution identity.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
|
@codex review Please review the current exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c82325a7e5
ℹ️ 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".
| case_id = _required_nonempty_string(case_id, "prediction case id") | ||
| if case_id not in allowed_ids: | ||
| raise ValueError(f"prediction case is absent from frozen validation plan: {case_id}") | ||
| normalized[case_id] = deepcopy(value) |
There was a problem hiding this comment.
When a predictor returns a tuple or another value that json.dumps coerces, this line preserves the Python representation even though the commitment digest represents its JSON form. For example, a tuple prediction (1,) compared directly with expected [1] is FALSIFIED, but after the documented JSON persistence/reload it becomes a list and is SURVIVED; both commitments have the same commitment_sha256 and pass verification. Reject non-JSON values or normalize them before storing and hashing so one commitment cannot produce contradictory standings.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| seen.add(case_id) | ||
| normalized.append({ | ||
| "id": case_id, | ||
| "domain": deepcopy(case.get("domain")), |
There was a problem hiding this comment.
Fresh evidence in this head is that the new comparator-field whitelist still copies domain as arbitrary JSON into the prediction-side plan. A case with domain={"expected": 7} is accepted, exposes the held-out label before prediction, and can later produce SURVIVED when the oracle repeats that domain and expected value, defeating the documented no-expected-values boundary through another allowed field. Restrict and revalidate domains as identifier-shaped primitives rather than accepting arbitrary objects.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5d7dd5a4d
ℹ️ 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".
| # rollback: remove this module, its package and boundary-inventory entries, tests, oracle fixture, and documentation without changing EPAC construction modules or chemistry derivation behavior | ||
| # since: 2026-09-29 | ||
| # unresolved: external oracle authority selection and custody remain outside EPAC; this module binds supplied evidence but does not establish independent custody by itself | ||
| # === END MODULE_BUILD === |
There was a problem hiding this comment.
The new public experiment ends after MODULE_BUILD, while tests/test_heldout_validation.py likewise contains tests without a CHECKS block. The applicable test-build contract requires source-owned CONTRACTS and linked test-owned CHECKS; without them, the repository's evidence graph cannot discover or reconcile this module's validation obligations even though the implementation relies on those behaviors for evidence integrity.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| if set(oracle_by_id) != set(plan_by_id): | ||
| raise ValueError("oracle case inventory does not match frozen validation plan") | ||
|
|
||
| predictions = commitment_snapshot["predictions"] |
There was a problem hiding this comment.
When a caller supplies a deserialized or externally constructed commitment with a correctly recomputed digest, verify_commitment validates prediction key shape but not membership in the plan, and this function then iterates only plan_by_id. A commitment containing extra, non-preregistered prediction IDs is therefore accepted and sealed into the receipt digest while those entries are silently ignored, violating the documented frozen-inventory boundary; compare the prediction-key set with the plan before scoring.
Useful? React with 👍 / 👎.
| if not math.isfinite(float(left)) or not math.isfinite(float(right)): | ||
| return None | ||
| except (OverflowError, TypeError, ValueError): | ||
| return None |
There was a problem hiding this comment.
For exact cases containing a valid JSON integer too large for binary64, converting the integer with float(...) raises OverflowError, so even two identical integers such as 10**400 are classified UNRESOLVED. Integers are inherently finite in Python and can be compared directly; apply the finiteness conversion only to float operands so exact large-integer evidence is not discarded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d8620ad9b
ℹ️ 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".
| } | ||
| ]) | ||
| (root / "plan.json").write_text(json.dumps(plan), encoding="utf-8") | ||
| plan = json.loads((root / "plan.json").read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When a persisted validation plan or prediction commitment comes from external custody or is modified before reload, these raw json.loads calls silently retain the last occurrence of duplicate fields. A document can therefore contain two cases, predictions, or digest interpretations while the last interpretation passes verification, making the supposedly frozen evidence parser-dependent. Fresh evidence in this head is that duplicate-key rejection was added only to load_oracle; provide equivalent loaders for plans and commitments and use them in the documented workflow.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
1 participant
Purpose
Add a held-out validation architecture for EPAC without making chemistry references an EPAC construction dependency.
Flow:
frozen validation plan → EPAC derivation → frozen prediction → held-out oracle → comparator → evidence receiptThe pre-plan binds case IDs, domains, and comparator rules before the prediction commitment exists. Expected chemistry values and provenance remain held out until comparison.
Boundary properties
freeze_validation_planSHA-256-binds the case inventory and comparator rules before predictions are frozensource_identitySURVIVED / FALSIFIED / UNRESOLVEDUNRESOLVEDnullor canonical nonempty strings so structured oracle data cannot cross through the planMODULE_BUILDcontract with areadstorage boundary and is included in the distributable packageepac_datapackageCONTRACTSare linked to 23 zero-argument test-ownedCHECKSwitnessesinternal/non-boundaryFile plan
epac_heldout_validation.pytests/test_heldout_validation.pydocs/heldout-chemistry-validation.mddata/heldout_chemistry_oracle.jsonepac_boundary_probe_completeness.pytests/test_boundary_probe_completeness.pypyproject.tomlRepair identity
1cc98feea010eb2a862c05ce72e37daf33a6b9091281ddd8e3d8c8b5744537fb25944b7343264c41c82325a7e5691fe63a19e6cb7efaf1bd0b166f11a5d7dd5a4da8d0a10a2233c589a18665de5e2eed7d8620ad9bf455507da0e9b07a4436b0ca430a041e5c999286f12221eff9870d1372203ac7935f2aVerification
Initial isolated regression coverage and the documented end-to-end example passed before the first repair push. The exact-head follow-up adds regressions for all six findings from the
1281ddd8…review. Hosted full gates for7d8620ad…must be green before merge; this section does not claim hosted acceptance in advance.The existing sealed molecular-geometry experiment remains untouched. No chemistry, physics, theorem, proof, measurement, or empirical standing transfers through this validation harness.
hmmm
The module enforces and binds the validation-plan-before-prediction-commitment order. Stronger proof that an external curator could not see predictions before creating the plan requires independently controlled/timestamped custody outside EPAC.