Skip to content

Refuse a malformed NZ cell-support contract with a named error (#1202 follow-up) - #1219

Merged
MaxGhenis merged 2 commits into
mainfrom
nzhub/g8a-named-errors
Oct 13, 2026
Merged

MaxGhenis merged 2 commits into
mainfrom
nzhub/g8a-named-errors

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #1202. Its round-3 review found two older paths in _cell_contract (transport/entitlement.py) that fail closed on a malformed NZ spec, but with bare errors that don't say what's wrong:

  • a cell-support contract missing a target_* threshold raised KeyError while the thresholds were read;
  • matching lists of mappings on both the contract and the tracked-target gate passed the drift guard and then raised TypeError at a set() comparison.

The change

_require_cell_contract_shape runs before the contract is read. It requires every key the contract uses:

  • the five thresholds;
  • required_cells;
  • required_scenarios;
  • aggregation_column.

required_cells and required_scenarios must be non-empty lists of non-empty strings. A violation raises a ValueError naming the gate and the offending key or value. A well-formed spec behaves exactly as before.

Tests

test_transport_entitlement_graph.py gains 21 cases (10 omitted keys, 10 malformed lists, 1 matching mapping):

  • each of the eight omitted keys;
  • mapping, mixed, empty, string and blank values for both lists;
  • the reviewer's matching-mapping case.
  • (d41040b) omitting required_weight_kind or required_weight_entity.

All 21 fail when the check is removed (an in-memory mutant; r2 confirmed 24/24 direct mutation checks, including the changed existing case's 3 parameters). One #1202 case, a string contract list, is now refused by this earlier check; its expected message changes accordingly.

Runs on this tree:

  • test_transport_entitlement_graph.py: 67 tests at the final head d41040b, all passing in r2's full run (67 passed). The earlier run, at 05290df, had 65 tests: 64 passed and 1 failed, the case adjusted above, which then passed on re-run.
  • test_nz_spec_package.py: 72 passed.
  • test_transport_graph.py: 15 passed.
  • test_transport_gate_bindings.py: 114 passed.
  • ruff check, ruff format --check, tools/ci_test_plan.py verify and git diff --check pass.

Invariant, scoped: a contract that omits a required key, or declares a malformed required_cells or required_scenarios list, is refused with a named ValueError before the threshold reads and the list set comparisons. Valid contracts behave as on main; r2 checked five valid declarations for identical graph JSON. The check does not type-check every value. For example, aggregation_column=[] still fails later with a bare TypeError, and a missing parameters object fails with a generic TypeError. Both paths predate this PR and still fail closed.

Review

  • r1 (20261012-083448, GPT-6.1 Sol) at 05290df: REQUEST_CHANGES (P2). _CELL_CONTRACT_KEYS omitted required_weight_kind and required_weight_entity, which _cell_contract's caller reads (entitlement.py ~374/380). Its executed deletion probes passed the shape check and then raised a bare KeyError. The reviewer's test run was cut short by the disk floor; ruff, the CI plan (658 files), the burndown (41 green) and diff-check passed.
  • Fixed in d41040b:
    • Both keys are now in _CELL_CONTRACT_KEYS. Those are the only params[...], parameters[...] and contract_params[...] reads of the contract in entitlement.py.
    • Both are added to the omission test's explicit key list.
    • The omission tests give 10 passed with the fix and 2 failed with the source reverted.
    • The full entitlement file runs in CI and in the delta review.
  • r2 (20261012-184519, GPT-6.1 Sol) at d41040b: APPROVE_WITH_NITS, Needs Max no. 268 tests passed (67 entitlement); 24/24 direct mutation checks failed as expected. Its two nits are applied here: the scoped invariant and the final counts.

🤖 Generated with Claude Code

The round-3 review of #1202 found two older paths in _cell_contract that
failed closed with bare errors on a malformed spec: a contract missing a
target_* threshold raised KeyError while the thresholds were read, and
matching lists of mappings on both gates reached a set() comparison and
raised TypeError.

_require_cell_contract_shape now runs first. It requires every key the
contract is read for, and requires required_cells and required_scenarios
to be non-empty lists of non-empty strings, naming the gate and the
offending key or value.

Tests: each of the eight omitted keys; mapping, mixed, empty, string and
blank values for both lists; and the reviewer's matching-mapping case.
All 19 fail with the check removed. One #1202 case, a string contract
list, is now refused by this earlier check, and its expected message
says so.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_cell_contract reads required_weight_kind and required_weight_entity, but
the shape check did not list them, so a contract missing either passed the
check and then raised a bare KeyError. Both are now required, with an
omission test each (red without the fix: 2 failed).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaxGhenis
MaxGhenis marked this pull request as ready for review October 13, 2026 01:11
@MaxGhenis
MaxGhenis merged commit c62a2c2 into main Oct 13, 2026
10 checks passed
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Merged by the NZ hub (session local_e407a527) at d41040b with --squash. Gates: gh pr checks exit 0; MERGEABLE; not draft; no CHANGES_REQUESTED review. Reviewed head: d41040b. Reviews (GPT-6.1 Sol): r1 20261012-083448 REQUEST_CHANGES (P2: two contract weight keys missing from _CELL_CONTRACT_KEYS), fixed in d41040b; r2 20261012-184519 APPROVE_WITH_NITS at d41040b (268 tests incl. 67 entitlement; 24/24 direct mutation checks), its description nits applied. Needs Max: no. CI all green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant