fix: close Phase E1 profile-binding soundness holes - #67
Conversation
…tity, geometry, calibration pinning)
|
@droid review |
|
Droid finished @txmed82's task —— View job Review complete. The PR is a careful, well-tested hardening of profile binding: the conflation fix, identity pinning, and deep calibration freeze all check out, and the security review found no new fail-open paths. One definite logic bug: the new Posted 1 inline comment (P1). |
|
Droid finished @txmed82's task —— View job Droid review complete. The conflation, identity-pinning, and calibration-freezing changes are sound and well tested, but the new valid_range geometry validator contains an inverted guard that rejects every fully finite bounded range (e.g. (0.0, 10.0)) as 'bounds neither side' — the most common case, and one no test exercises. That needs fixing before merge; the rest of the PR checks out. Posted 1 inline comment (1 P1). |
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
| if not (math.isfinite(low) and high == math.inf) and not ( | ||
| math.isfinite(high) and low == -math.inf | ||
| ): | ||
| raise TaskContractError( | ||
| f"stream {self.id} valid_range {self.valid_range!r} bounds neither side; " | ||
| "omit valid_range when the value is unconstrained" | ||
| ) |
There was a problem hiding this comment.
[P1] _geometry_is_sane rejects every fully finite valid_range
The "bounds neither side" guard only passes when the range matches exactly one of the two one-sided-infinite patterns (isfinite(low) and high == inf, or isfinite(high) and low == -inf). For an ordinary bounded range such as (0.0, 10.0) both clauses are False, so not A and not B is True and construction raises "bounds neither side" — the opposite of the stated intent (only ranges unbounded on both sides should be refused). Every task that declares a normal bounded measurement range now fails validation, and the new tests miss it because they only exercise NaN, reversed, both-infinite, and one-sided cases. The guard should reject only ranges with no finite bound at all, and a finite-range regression test should be added.
| if not (math.isfinite(low) and high == math.inf) and not ( | |
| math.isfinite(high) and low == -math.inf | |
| ): | |
| raise TaskContractError( | |
| f"stream {self.id} valid_range {self.valid_range!r} bounds neither side; " | |
| "omit valid_range when the value is unconstrained" | |
| ) | |
| if not math.isfinite(low) and not math.isfinite(high): | |
| raise TaskContractError( | |
| f"stream {self.id} valid_range {self.valid_range!r} bounds neither side; " | |
| "omit valid_range when the value is unconstrained" | |
| ) |
| if not (math.isfinite(low) and high == math.inf) and not ( | ||
| math.isfinite(high) and low == -math.inf | ||
| ): |
There was a problem hiding this comment.
[P1] valid_range validator rejects every fully finite range
The guard in _geometry_is_sane raises unless the range is exactly one-sided-infinite: for low=0.0, high=1.0, clause A (math.isfinite(low) and high == math.inf) is False and clause B (math.isfinite(high) and low == -math.inf) is False, so both not clauses hold and a TaskContractError claiming the range "bounds neither side" is raised. Verified by running the validator: StreamSpec(valid_range=(0.0, 1.0)) is rejected while (0.0, inf) and (-inf, 0.0) pass. The adjacent comment and the PR description say the intent is to refuse only ranges unbounded on both sides, and a bounded range is the most common declaration (a normalized [0, 1] score, a [0, 255] pixel range, a joint angle in [-pi, pi]), so every task declaring one now fails validation. The new tests only exercise one-sided ranges as the allowed cases, so this regression passes CI unnoticed. Raise only when neither endpoint is finite.
| if not (math.isfinite(low) and high == math.inf) and not ( | |
| math.isfinite(high) and low == -math.inf | |
| ): | |
| if not (math.isfinite(low) or math.isfinite(high)): |
|
@droid review |
|
Droid finished @txmed82's task —— View job Droid review complete. The head commit fixes the previously reported P1 (the inverted valid_range guard that rejected every fully finite range); verified by direct probing that finite, degenerate, and one-sided-infinite ranges now pass while NaN, reversed, and both-unbounded ranges are refused, with new regression tests pinning the finite happy path. The conflation fix (split by_id/by_schema index with ambiguity refusal), unconditional identity pinning, and deep calibration freeze (_FrozenDict/_freeze_json with a post-validation refreeze safety net) all check out under edge-case probing, and the full affected suite (104 tests) passes locally. No new high-confidence actionable findings; the previously reported issue is resolved and therefore not re-flagged. Posted 0 inline comments. |
2 participants
Summary
Corrective PR against merged #66 (Phase E1 semantic stream profiles). Ground-truth probes against the merged code confirmed four soundness gaps in
CapabilitySpec.satisfies()/StreamSpecvalidation; this PR closes all four and locks each with regression tests.1. Cross-channel conflation (
satisfies)The profile index merged
s.idands.schema_idkeys into one dict, and lookup fell backid -> schema_id. Because both namespaces are open slugs, an interface may legally declare channelcamwith schemaprobeand channelprobewith schemacam. In that layout, capability profiles for channel B vouched for channel A:only_b.satisfies(crossed)returnedTruewhile A had no declaration at all.Fix: two independent maps (
_profile_index->by_id,by_schema). Exact channel-id match wins; the schema route requires declared semantics on the interface side, exactly one candidate profile for that schema, and that the candidate not be another interface channel's declaration.2. Identity pinning
Once a profile is in play,
schema_id,adapter,adapter_digest, andsourceare now compared unconditionally on both sides (a$-vs-pointer source locator is a different channel even under one id).privilegedis gated before any semantic comparison and requires explicitaccepts_privileged.roleis enforced when the interface declares it.3. Geometry validation
shapedims are screened in amode="before"field validator soTruecannot be laundered to1by pydantic lax coercion, and zero/negative axes are refused (tuple[int, ...]accepted them pre-fix).valid_rangenow rejects NaN bounds, reversed bounds, and ranges unbounded on both sides; one-sided infinite ranges stay legal (real physics, e.g. depth with no ceiling).4. Calibration pinning
camera_calibrationwas a plain dict after validation:stream.camera_calibration["fx"] = ...succeeded post-construction, defeating digest-style pinning. Now frozen deep at validation into_FrozenDict(every mutator raises), with JSON-canonicalizing_freeze_json(non-finite floats, non-string keys, non-JSON leaves rejected as oneTaskContractErrorfrom a before-validator). Declaredness is key-count based so{"fx": 0.0}/skew=Falsecount as declared._calibration_equalre-canonicalizes both sides, so post-model_copytrees compare by content, and it never raises inside selection loops.Test plan
tests/test_multi_modality_contracts.py: conflation (11), legitimate schema route (11b), ambiguity refusal (11c), plus new tests:test_stream_rejects_impossible_geometry,test_camera_calibration_is_deeply_immutable,test_camera_calibration_rejects_non_json_values,test_zero_valued_calibration_counts_as_declared,test_calibration_equality_ignores_key_order_and_uses_content,test_stream_identity_adapter_digest_and_source_are_enforced.ruff check,ruff format --check,mypystrict clean;pytest tests/test_multi_modality_contracts.py tests/test_eval_run.py tests/test_eval_p3.py tests/test_eval_v03.py tests/test_eval_contracts.py-> 104 passed.