Repository navigation
fix(api): reject modality inputs that would be silently discarded - #334
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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: 2e126ca2ff
ℹ️ 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".
|
|
||
| /// Octad create/update request | ||
| #[derive(Debug, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] |
There was a problem hiding this comment.
With deny_unknown_fields, schema-conforming POST/PUT payloads containing top-level document, vector, graph, or semantic objects now fail with 422 because those names are absent from OctadRequest. I checked the v1 OpenAPI reference to connectors/shared/json-schema/octad-input.json, the Rust client's nested OctadInput, and the repository smoke scripts, all of which send this nested shape; moreover, the Rust client serializes its unsupported optional fields even when they are None, so its create/update calls will be rejected universally. Accept the published nested fields here, or version and update the schema, clients, documentation, and scripts atomically rather than breaking the existing v1 contract.
Useful? React with 👍 / 👎.
The current strict `clippy (all-targets)` job fails on `rust-core/verisim-octad/src/ram_promotion.rs` even for unrelated PRs (#333 and #334). Two diagnostics: - `HashSet` is imported but never used. - `PromotedState.modality` is never read; the containing `PromotionManager.promoted` map already keys entries by `Modality`. Remove the unused import and redundant private field/initializer. No storage format, behaviour, or API change. Await CI. This is independent of the WAL and API integrity changes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Tests** * Updated the capability-matching test setup; its matching logic and expected result remain unchanged. * **Refactor** * Simplified internal promotion-state handling and removed an unused import. No user-facing behaviour changes are reported. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
…tions and v1 consumer contract (#337) Independent code-grounded reconnaissance as of main `5876bc67e8962e3b276fb0161664e0b3a420a64c`. Adds three review documents: - `docs/reviews/2026-10-29-verisimdb-as-built-recon.adoc` — source-derived architecture, conceptual ER, eight-modality/persistence matrix, actual lifecycle, P0/P1/P2 discrepancy register, tests and formal proof obligations, independent Mathesis/QuandleDB/JanusKey/Marches reuse, prioritized engineering programme. - `docs/consumer-interface-contract-v1-observed.adoc` — actual flatten-vs-nested HTTP ABI, operations, HTTP statuses, feature/configuration guarantees and explicit non-guarantees. - `docs/reviews/2026-10-29-hypatia-verified-handover.adoc` — only verified downstream integration facts. No rewrite of the Octad schema; accepted generic term **modality profile** distinguished from **modality set** and **consonance subject**. References separate code fixes #333, #334, #335, #336. Makes no soundness claims beyond observed source/test status. No data mutation.
P0/P1 contract-integrity finding
The published nested
OctadInputJSON schema does not match the flattenedOctadRequestaccepted by HTTP POST/PUT. Because serde ignored unknown keys, schema-conforming clients could create a temporal-only entity despite submitting nested document, vector, or other fields. A body-only document request was also silently dropped byto_octad_input.Fix
#[serde(deny_unknown_fields)].bodyfield withouttitlerather than discarding document content.Compatibility and follow-up
This is a deliberate fail-closed validation change; previously silent unknown keys become errors. No data is deleted, and no database migration occurs. OpenAPI and client libraries must be reconciled in a separate versioned contract PR; do not treat their nested schema as an implemented API. Other field-level validation and empty-title policy require review.
Verification
Source and regression tests committed; awaiting CI compilation/test evidence. Keep draft until checks pass and compatibility is reviewed.