feat: add research input manifest validation - #276
Conversation
Co-Authored-By: Codex <noreply@openai.com>
![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: 1f1515f1e7
ℹ️ 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".
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| from jsonschema import Draft202012Validator |
There was a problem hiding this comment.
The new test module imports jsonschema, but the test job in .github/workflows/ci.yml installs only the editable package, NumPy, pandas, pytest, pytest-cov, and Ruff, while the project declares no runtime dependencies. On that clean CI environment, collection stops here with ModuleNotFoundError: No module named 'jsonschema' before any tests run; add it to the CI/test dependency set or avoid the external import.
Useful? React with 👍 / 👎.
| try: | ||
| ZoneInfo(timezone) | ||
| except ZoneInfoNotFoundError: |
There was a problem hiding this comment.
When this package runs on Windows or a minimal image without a system IANA timezone database, ZoneInfo("America/New_York") raises ZoneInfoNotFoundError; because the project declares neither tzdata nor another fallback, an otherwise valid manifest such as the test fixture is rejected solely according to the host installation. Bundle timezone data or avoid making contract validity depend on an undeclared system resource.
Useful? React with 👍 / 👎.
| if timezone in {"Factory", "posixrules"}: | ||
| _invalid() |
There was a problem hiding this comment.
On Linux layouts that expose /usr/share/zoneinfo/localtime, ZoneInfo("localtime") succeeds and this blocklist accepts it. That key follows the machine's /etc/localtime, so the same frozen manifest can denote UTC on one host and another timezone on a different host, undermining reproducibility; reject localtime along with the other non-canonical pseudo-zones.
Useful? React with 👍 / 👎.
| except (InvalidResearchInputEvidence, UnicodeDecodeError, TypeError, ValueError, json.JSONDecodeError): | ||
| raise InvalidResearchInputEvidence("invalid research-input manifest JSON") from None |
There was a problem hiding this comment.
For an invalid payload containing roughly a thousand nested arrays, json.loads raises RecursionError, which is not covered by this exception handler, so read_research_input_manifest_json leaks an implementation exception instead of the documented InvalidResearchInputEvidence. Callers that use that exception to fail closed can therefore crash on malformed input; reject excessive nesting or translate RecursionError consistently.
Useful? React with 👍 / 👎.
1 participant
Scope
Deterministic gates
F0 closeout only. No F1/P3/provider/live scope.