|
| 1 | +# D272 loader review, round 1: draft PR #123 (feat/d272-obligations 7958d081), measured over fix/translation-absent-forms 71e575c6 merged in. |
| 2 | + |
| 3 | +[review] |
| 4 | +schema = "sqlquack.recorder-change-review.v1" |
| 5 | +decision = "D272" |
| 6 | +branch = "feat/d272-obligations" |
| 7 | +commit = "7958d081d8bc1bd5de1ea45a37993e7d293ec725" |
| 8 | +measured_with = "71e575c6c merged into 7958d081 locally (not pushed)" |
| 9 | +reviewed_at = "2026-11-06T14:56:29Z" |
| 10 | +round = 1 |
| 11 | +verdict = "changes_requested" |
| 12 | +checked = "A debug build of the merge ran verify: 11,787 cases, 0 failed, 220/220 verified. syntax-coverage results then gives 61 matching and 27 unmet_obligations, with 3,312 obligations (979 met, 2,333 unmet), 0 load errors and 0 uncited cases. The one form short of B's 62 is data-types-logical. It is refused here only because this orb's build refuses the 25 Python project-function cases (fn:<udf>, uncoded). That is an environment artifact, not the PR, so the figure is 62. Against 71e575c6's baseline, the form regressions to UnmetObligations are exactly the PR body's 27 ids, nothing more and nothing missing, which meets D272c. For each of the 27, I checked the unmet obligations' cited outcomes. 12 are refused by the BINARY cast (SQ0002 node:cast). 7 IS_* are refused by that cast plus json_fidelity at PARSE_JSON. LN and SQRT are refused SQ0003 node:cast (DECFLOAT). CURRENT_DATE x2 are refused SQ0003 node:cast. ARRAY_COMPACT, ARRAY_FLATTEN, ARRAY_MAX and ARRAY_MIN are json_fidelity at ARRAY_CONSTRUCT. Obligations are met only when every cited case matches. The 'one of two cited cases' table row fails if this is weakened to 'any'. Held is never met and blocks matching. Unmeasured is never met and does not block. Load-shape errors (held with cases, blank held or reason, cases outside the family folder, id not form.t<digits>, absent_from_source form, retired id reused, replaced_by not live) are rejected in obligation_problems and retired_problems. On the real binary, three holes stay open (findings below). Cross-checks on 71e575c6: 0 cited cases whose case file construct differs from the obligation's form, and 1 non-error_parity obligation met through a matching_error case (a boundary overflow, legitimate)." |
| 13 | + |
| 14 | +[[review.finding]] |
| 15 | +kind = "fail_open" |
| 16 | +blocking = true |
| 17 | +detail = "A cited case may belong to another form. The loader checks only the family folder prefix and never compares the case file's construct with obligation.form. Probe: sf.functions.array-max.t005 retargeted to cite array-min.toml#t001_numbers loads with no error, and ARRAY_MAX goes unmet_obligations -> matching. Smallest fix: read construct from each cited cases/translation/<family>/<file>.toml and make construct != obligation.form a load error. 0 obligations on 71e575c6 violate this, so the figure does not change." |
| 18 | + |
| 19 | +[[review.finding]] |
| 20 | +kind = "fail_open" |
| 21 | +blocking = true |
| 22 | +detail = "A required obligation can be deleted without a tombstone. When an unmet obligation disappears, the --against diff records Unmet -> None and Matching gains as 'other changes', not regressions. Probe: deleting sf.functions.array-max.t005 (no retired entry) moves ARRAY_MAX to matching and gives '0 regressions, 2 other changes' against a baseline recorded from the head. Re-recording the baseline then makes the gate clean. Smallest fix: in the --against diff, an obligation id in the base and absent from the head is a regression unless retired-obligations.toml lists it." |
| 23 | + |
| 24 | +[[review.finding]] |
| 25 | +kind = "fail_open" |
| 26 | +blocking = true |
| 27 | +detail = "A counted form with no obligations matches vacuously. form_results only collects unmet required obligations, so a form with none reaches matching on any linked statement. 13 of the 62 matching forms declare no obligation at all: data-types-datetime.time, data-types-datetime.timestamp, data-types-logical, data-types-numeric.double, data-types-numeric.int, data-types-semistructured.array, array-index-access, colon-subscript-dot, object, object-key-access, data-types-text.dollar-quoted-string-constant, functions.extract.extract-from and functions.nullif. That leaves about a fifth of the published figure unbound to D151 obligations. Smallest fix: a form with no required obligation is unmet_obligations with a synthetic 'no obligation declared' entry, or a load error. Either drops the figure to 49 until C declares obligations for the 13. Which way is the coordinator's call, but a form without obligations must not count as matching under D272." |
| 28 | + |
| 29 | +[[review.finding]] |
| 30 | +kind = "fail_open" |
| 31 | +blocking = false |
| 32 | +detail = "A missing coverage/translation/obligations directory loads as zero obligations with no error (probe: 0 declared, 88 matching). Once the target baseline holds obligation states, --against catches this through Met -> None regressions. It should still be a load error, as the README describes obligations as mandatory." |
| 33 | + |
| 34 | +[[review.finding]] |
| 35 | +kind = "doc" |
| 36 | +blocking = false |
| 37 | +detail = "The README and PR body say uncited_cases 'can make no form match'. Not so: any linked statement, including non-translation cases (aggregate/, relational/, project-functions/) and cases cited only by another form's obligations, still makes a form match when it has no obligations or all are met. The third blocking fix makes the sentence true. Otherwise reword it." |
| 38 | + |
| 39 | +[[review.finding]] |
| 40 | +kind = "doc" |
| 41 | +blocking = false |
| 42 | +detail = "PR body table: ARRAY_MAX and ARRAY_MIN are unmet through declared divergence json_fidelity at ARRAY_CONSTRUCT (array-max/array-min t005_null_array), not array_sort_order at ARRAY_MAX. Ranked item 2 then covers 11 forms, and item 5 does not apply." |
| 43 | + |
| 44 | +[review.files_sha256] |
| 45 | +"7958d081:crates/sqlquack-syntax-coverage/src/results/_helpers/obligations.rs" = "9e2d6469108b6f4e555a649f53280d430b3e6f391bc5bb76b9661e109166647b" |
| 46 | +"7958d081:crates/sqlquack-syntax-coverage/src/results/_helpers/report.rs" = "dda25cb001fc43d3e14ed3de8bd113469b5410b35735f82c89718676b6cb101f" |
| 47 | +"7958d081:crates/sqlquack-syntax-coverage/src/results/_helpers/forms.rs" = "a46c0580420c38237f6652675fa988c88548e1fe1679ba3074e958787f12391e" |
| 48 | +"7958d081:crates/sqlquack-syntax-coverage/src/results/_helpers/diff.rs" = "ea274487ccff03c41b01519c49974a2e238d3e9e1ab0491a88408d98e7b916c8" |
| 49 | +"7958d081:coverage/translation/README.md" = "ee91414992fcff4d1486d2b4b52c4130d2470654e371929e4a35816f4eaf600e" |
0 commit comments