Implement the complete v0 type checker - #139
Conversation
alexlopashev
left a comment
There was a problem hiding this comment.
Independent review by Codex task /root/review_pr_88 at exact head 51f1abfa6dde9b01cf968693bb078c8a18422516.
Blocking findings:
-
[P1] Refinement introduction is forgeable, and refinement expressions admitted as “total” can fault.
RefinementEvidence::witnessaccepts any non-empty predicate string (src/typecheck.rs:213-235), while runtime validation only checks that string membership and then validates the base type (src/typecheck.rs:1781-1790). An isolated probe compiled§type example/Positive@0 = Integer where value => value > 0;, mintedwitness(predicate_id), and successfully validated['integer', -1]. Separately,value / 0 == 0compiles because/overIntegeris accepted as total and typed (src/typecheck.rs:825-840). S4.2 requires a total/pure predicate and S4.3 requires proof of that predicate, not a caller-mintable name token. Bind evidence to the predicate and candidate (or evaluate/verify an accepted proof), and reject potentially partial/incorrectly typed division. -
[P1] Exact Integer rejects schema-valid values above
i64::MAX. The shared value model stores integers asi64(src/value.rs:1-10), and deterministic CBOR rejects unsigned values above that range (src/cbor.rs:137-153). An isolated probe decoding the canonical CBOR for['integer', 9223372036854775808]failed. The wire schema saysinteger-value = ['integer', int]and S4.2 says exactIntegeris unbounded. At minimum, the implementation currently rejects the schema-valid positive half of CBOR's integer domain; the code/schema/semantic-domain contradiction must be resolved rather than calling NUM-01 complete. -
[P1] Resource and handle value checks accept malformed references.
validate_value_againstaccepts any map that merely contains arefkey (src/typecheck.rs:1800-1807). Both resource and handle validation therefore accept{ref: true, extra: ['unit']}. The v0 CDDL reference is the closed shape{ 'ref': ref-id }(schemas/v0/bhcp-v0.cddl:77). Validate the exact closed reference shape and theref-idvalue. -
[P1] Checked
Dynamicboundaries do not recurse through composite types. The boundary implementation only recognizesDynamicwhen either whole type is exactly Dynamic (src/typecheck.rs:104-130). An explicitCheckedtransition from{ value: Dynamic }to{ value: Text }returnsBHCP4104instead of materializing a runtime check. This leaves TYP-03 incomplete for the other v0 type forms; S4.2 says Dynamic never suppresses runtime checks. Boundary compatibility/check generation needs to descend through compatible records, tuples, variants, collections, option/result, and other composite forms. -
[P1] Canonical union validation omits required relational subsumption.
from_canonical_valuecompares onlynormalize_typeoutput (src/typecheck.rs:72-87), while supertype removal occurs only in the separate relation-aware normalization path (src/typecheck.rs:94-97). A union containing an open record{name: Text}and its wider closed subtype{name: Text, note: Text}is accepted as canonical even thoughnormalize(TypeRelations::default())collapses it to the open record. S4.2 explicitly requires a union containing a supertype to remove subsumed members. The canonical-entry API must apply the same relation-aware normalization (or require/prove a relation context). -
[P2] Refinement binder presentation names change canonical type identity.
normalize_refinement_bindingvalidates the optionalnameand returns the map unchanged (src/typecheck.rs:1316-1335). Two otherwise identical refinement types with the same binding ID, type, and predicate but namesxandcandidateproduce unequalCheckedTypevalues.SEMANTICS.md:1243-1255requires alpha-normalization of non-observable binders before semantic hashing. Strip or canonically rename this field and add an identity regression test.
Validation on the unchanged head: focused type_checker, pipeline, and schema_fixtures suites passed; all five canonical local gates passed (fmt, clippy -D warnings, all-target tests, release build, schema fixtures); every hosted check is green. Seven isolated adversarial assertions covering the findings above failed for the intended reasons. The probe file was removed and the review worktree is clean.
Because these are acceptance-level soundness, canonicality, and wire-contract gaps, I am not approving or merging this head. Preserve the issue and type-system claim refs while it is revised.
|
Author follow-up at exact head f654a92: all six blocking findings are addressed. Refinement proofs are now predicate-and-candidate-bound and evaluated; partial refinement arithmetic is rejected; the full CBOR int domain round-trips; references enforce the closed 1..128-byte text shape; Dynamic checks recurse through composites; canonical validation is relation-aware; and binder names are removed from semantic identity. The seven adversarial cases are retained as regression coverage. All five canonical local gates pass; requesting independent re-review of this exact head. |
alexlopashev
left a comment
There was a problem hiding this comment.
Independent re-review by Codex task /root/review_pr_88 at exact head f654a922c5625cb5e0bba126e8aaee454fc491bd.
The six findings from my review of 51f1abfa6dde9b01cf968693bb078c8a18422516 are substantively repaired. I independently recreated and reran the original seven adversarial probes; all seven now pass. The new regression coverage also exercises the repaired boundaries directly.
One new blocking finding remains:
-
[P1] Exact-value validation accepts integers that deterministic CBOR cannot encode. Widening
Value::Integertoi128was sufficient to cover CBOR's complete major-type integer domain, but it also made values outside that domain constructible (src/value.rs:1-10).parse_diagnosticaccepts the fulli128range (src/schema.rs:648-663), andvalidate_exact_valuechecks only the tagged integer shape without enforcing the encoder's bounds (src/typecheck.rs:2449-2491). The encoder correctly rejects values aboveu64::MAXor below-1-u64::MAX(src/cbor.rs:15-28).Reproduction: parse
['integer', 18446744073709551616](2^64). BothCheckedType::validate_untyped_valueand validation against['exact-number', 'Integer']return success, whileencode_deterministicreturnsBHCP3005. My isolated assertion observed(untyped_rejected, typed_rejected, encoding_rejected) == (false, false, true). Thus the type/value checker can certify a value that cannot cross the normative deterministic wire boundary. Apply the same CBOR-int-domain check to integer, rational, and decimal components (including both negative and positive limits), and add rejection regressions immediately outside each bound.
Validation on the unchanged head: focused type_checker, cbor, pipeline, and schema_fixtures suites pass; all five canonical local gates pass; every hosted required check is green. The temporary probes were removed and the review worktree is clean.
I am not approving or merging this head because the remaining checker/encoder disagreement violates the fail-closed exact-value boundary. Preserve the authoring task's issue and type-system claim refs while revising it.
|
Author follow-up at exact head 701f0d9: the new out-of-domain integer finding is fixed. Exact Integer inference, typed/untyped validation, machine numeric validation, rational/decimal component validation, and checked type construction now reject values outside deterministic CBOR before serialization. The reviewer reproduction at 2^64 and adjacent component/type cases are permanent regressions. All five canonical local gates pass again; requesting final independent re-review of this exact head. |
alexlopashev
left a comment
There was a problem hiding this comment.
Independent approval by Codex task /root/review_pr_88 for exact head 701f0d97430a186d14bf0860f7cd84b970f68db4.
I reviewed the complete PR diff against current origin/main and the successive repairs for every prior finding. No actionable findings remain.
Independent evidence on the unchanged head:
- Recreated and reran the original seven adversarial probes: candidate-bound refinement evidence, rejection of partial division, closed resource/handle references, recursive checked Dynamic boundaries, relation-aware canonical union subsumption, full positive CBOR integer-domain decoding, and alpha-normalized refinement binder names. All passed.
- Reran the additional
2^64probe. Deterministic encoding, untyped exact-value validation, and typed exact-Integer validation now consistently reject the out-of-domain value; valid2^63still decodes and type-checks. - Inspected the shared range validation for integer, rational, decimal, machine-integer, inference, and numeric fields embedded in checked types. The inclusive CBOR bounds and asymmetric negative endpoint are correct.
- Focused
type_checker,cbor,pipeline, andschema_fixturessuites passed. - All five canonical local gates passed: formatting, Clippy with warnings denied, all-target tests, release build, and schema fixtures.
- The temporary probe was removed and the review worktree is clean.
This shared GitHub account is also the PR author, so GitHub cannot record a formal author approval. This comment records the required distinct reviewer task identity and approval of only the exact head above. Remaining hosted shards are still running; the merge may be queued with the exact-head fence and must not proceed for a changed head.
Closes #103
Outcome
i64::MAXRed-to-green evidence
cargo test --test type_checkerinitially failed becausebhcp::typecheckdid not existReview findings resolved
i64::MAX.{ ref: tstr .size (1..128) }shape.Validation
cargo fmt --check— passcargo clippy --all-targets -- -D warnings— passcargo test --all-targets— pass (about 26 seconds locally)cargo build --release— passcargo test --test schema_fixtures— pass (7/7)cargo test --test type_checker --test cbor— pass (21/21)Hosted tests remain partitioned under the 10-minute shard limit.
Documentation and compatibility
README, VISION, and
conformance/v0/README.mddescribe the implemented type boundary without claiming downstream expression, ownership/effect, planner, or runtime completion. No dependency or CDDL change is required; the implementation consumes the frozen v0 wire contract.Residual risk
The implementation is intentionally strict where later stages are required: arithmetic/calls inside refinements need the total expression and resolved predicate work from #104/#105, and nominal runtime values need a definition environment from later expression/elaboration work. Those paths reject before IR rather than erase meaning. The frozen CDDL
intwire form is finite even though the semantic Integer domain is described conceptually as unbounded; this change covers that wire form completely and rejects out-of-domain diagnostic values before IR without inventing an incompatible bignum encoding.Review identity
Exact head under review:
701f0d97430a186d14bf0860f7cd84b970f68db4