compass(spec): explain refuses a partly present quantity and names every missing field - #268
Conversation
…ery missing field On a spec assembled from parts, explain answered a quantity or block from the fields it held and dropped the rest without saying so: kv_blocks with no device.memory.capacity_bytes came back as 12 rows. It now refuses with TOTALITY when any required field under the term is absent, and names all of them. The multi-field wording is a sibling in rules.py that the accessor's single-field refusal now calls, so the two cannot drift. An optional field the document left out is not missing, so a block holding one still explains. A field that is present but contributes no rows, such as an empty tokenizer table, is still printed as the empty value it holds; it stays out of the contributions. Closes #265 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| The required half of `refuse_absent_field`, which reads one path through | ||
| here, so a question over several fields and a question over one are | ||
| declined in the same words. Every missing field is named rather than the | ||
| first: each is a separate fragment to merge, and a refusal naming one of |
There was a problem hiding this comment.
Nit, not blocking (principle 8: a claim carries its measurement). "each is a separate fragment to merge" is stated as fact, and it is not always true. A fragment is a partial document (merge.py module docstring), and one fragment can carry several fields. For example, the tokenizer probe writes host.cpu.cores_physical and host.cpu.cores_logical alongside its table. The reason to name every field does not need that claim. Each missing field needs some fragment to supply it, and a refusal that names only one field sends the reader back once per field. Suggest: "each needs a fragment that measures it, and a refusal naming one of them sends the reader back once per field."
The plural remedy, "merge the fragments that measure these fields", reads fine even when one fragment covers two of the fields. Only the docstring overstates this.
| with pytest.raises(SpecRefusal) as accessed: | ||
| spec.value("device.memory.capacity_bytes") | ||
| assert refused.value.rule is Rule.TOTALITY | ||
| assert str(refused.value) == str(accessed.value) |
There was a problem hiding this comment.
Pin gap, not blocking (principle 8). This assertion compares explain with spec.value. Both now build their text in the same refuse_absent_fields, so the comparison cannot fail on wording. It proves the two sides share a site. It does not prove the singular text is what it was.
I measured this on node 18 with a line-count-preserving mutation of rules.py L166 (195 → 195 lines), run over test_spec_verbs.py and test_spec_schema.py:
("is", "it", "fragment that measures this field") → ("is", "it", "fragment that measures that field"): 289 passed.
So the singular remedy is pinned by nothing. The PR body names two schema tests as holding the singular text. Those tests assert the what, and a what mutation does redden test_a_declared_field_a_spec_lacks_is_never_called_undeclared. They do not assert the remedy.
The text is byte-identical today. I checked all 39 fields × {required, optional} with the old rules.py from 77d203b86 against the new one: 78/78 identical, including refuse_absent_fields((p,)). So nothing is wrong now. The gap only means a later edit could change the wording silently. One line here closes it:
assert refused.value.remedy.endswith("merge the fragment that measures this field before asking for it")|
Agent-authored review (Claude). Reviewer for #265, cycle 1. Review, cycle 1: APPROVE at head
|
| # | mutation | result | failing node ids (all tests/compass/) |
|---|---|---|---|
| N | if missing: → if len(missing): (null) |
289 passed | none |
| named | if missing: → if (): (tip behaviour) |
3 failed | test_spec_verbs.py::test_a_quantity_missing_one_field_is_refused_rather_than_explained_from_the_rest, test_spec_verbs.py::test_every_missing_field_under_a_quantity_is_named_and_not_only_the_first, test_spec_verbs.py::test_a_block_an_assembled_spec_holds_nothing_under_names_every_field |
| D1 | refuse_absent_fields(missing[:1]) |
2 failed | test_spec_verbs.py::test_every_missing_field_under_a_quantity_is_named_and_not_only_the_first, test_spec_verbs.py::test_a_block_an_assembled_spec_holds_nothing_under_names_every_field |
| D2 | …required or 1) (optional treated as required) |
3 failed | test_spec_verbs.py::test_an_optional_field_the_document_left_out_is_refused_as_absent, test_spec_verbs.py::test_a_block_explains_without_an_optional_field_the_document_left_out, test_spec_schema.py::test_explain_declines_an_unknown_term_and_an_absent_one_at_two_sites |
| D3 | self.contributions + () ] (empty line dropped) |
2 failed | test_spec_verbs.py::test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusing, test_spec_verbs.py::test_an_empty_tokenizer_table_is_printed_as_empty_under_a_quantity |
| R1 | missing reversed |
2 failed | same two as D1 |
| R2 | empty row pushed into contributions |
2 failed | same two as D3 |
| R3 | if not rows: → if 1: (empty line for every field) |
1 failed | test_spec_verbs.py::test_an_empty_tokenizer_table_is_printed_as_empty_under_a_quantity |
| R4 | if len(absent) == len(paths): → if 0: |
2 failed | test_spec_verbs.py::test_an_optional_field_the_document_left_out_is_refused_as_absent, test_spec_schema.py::test_explain_declines_an_unknown_term_and_an_absent_one_at_two_sites |
| R5 | one = len(paths) >= 1 (always singular) |
2 failed | same two as D1 |
| R6 | singular remedy this field → that field (rules.py L166) |
289 passed | none: the inline finding at L1096 |
| R7 | what text by this schema → by the schema |
2 failed | test_spec_verbs.py::test_every_missing_field_under_a_quantity_is_named_and_not_only_the_first, test_spec_schema.py::test_a_declared_field_a_spec_lacks_is_never_called_undeclared |
| R8 | empty row carries None instead of the held value |
2 failed | same two as D3 |
The developer's named result and the three mutants D1–D3 reproduce exactly, by name. R1–R8 are mine. All of them bite except R6.
Gate 1: the tree that will land
- The integration tip moved during review. It went from
77d203b86to308922c5c, which is compass(tests): make the capture census discriminate, and assert what it measured (#238) #261 and touches onlytests/compass/test_capture_real_model.py. So the developer's merged tree03cf301edis no longer the landing tree. git merge-tree --write-tree 308922c5c 04bc6e51dgives22bddc4c322578418239c42c3933f01db4128c94, rc 0.- Staging:
git archiveof that tree, with the.compass-commit/.compass-changedstamps, passed throughdocker exec -i … tar -xinto/tmp/r268gates/merged/ATOM. The md5 wasc19c1a37…on both ends. - Gate: the tree's own
scripts/compass/gate_cpu.sh, run undertimeout -k 10 1500. It printedatom: /tmp/r268gates/merged/ATOM/atom/__init__.pyandcommit: 04bc6e51d (stamp). - Result: 5119 passed, 149 skipped, 3 xfailed,
GATE_CPU_RC=0, in 163 s. - This is the developer's 5115 plus 4 from compass(tests): make the capture census discriminate, and assert what it measured (#238) #261:
test_capture_real_model.pycollects 18 tests at308922c5cand 14 at77d203b86. Notest_stream_marker_properties.pyflake fired. - Before the tip moved, I had gated the developer's tree
03cf301edonce. It gave 5115 passed, 149 skipped, 3 xfailed,GATE_CPU_RC=0, matching the developer's count exactly. - The tips were re-read before posting: head
04bc6e51d, base308922c5c, both unmoved. Staging was removed afterwards.
No other blocking issues.
Closes #265
What was wrong
Build a spec from parts with the public
MachineSpec(values, tokenizers), then askexplainfor a quantity or block that is only partly present.explainanswered from the fields it had. The rest were left out and nothing said so. The refusal added in #264 fired only when none of the fields was present.The case from the issue, driven on node 18 at both refs:
77d203b8604bc6e51dkv_blocks: 12 rows, no refusal(the four-width reserve and allocator rows, persistent buffer and three graph-pool rows. There is no capacity row and no sign that one is missing.)
refused TOTALITY: … `device.memory.capacity_bytes` is declared by this schema, and this spec carries no value for it. … merge the fragment that measures this field before asking for itadmissionmissing two fields:5 rows, no refusalrefused TOTALITY: … `host.admission_fixed_s`, `host.ipc.shm_broadcast_s` are declared by this schema, and this spec carries no value for them. … merge the fragments that measure these fields before asking for themThe empty-tokenizer case from the issue comment is
admissionon a spec read with an emptyhost.tokenizers:What was decided
MachineSpec.value.rules.py,refuse_absent_fields(paths).refuse_absent_field(path, required)now sends its required branch through it with a one-element tuple. That makes the required refusal a singleSpecRefusal(...)site with singular and plural forms, soexplainand the accessor cannot drift.rules.pyis in the diff because the alternative was a second copy of the text inexplain.py, and a second copy is exactly the drift the issue asks to rule out. The singular text is unchanged character for character.test_the_three_refusals_are_not_interchangeableandtest_a_declared_field_a_spec_lacks_is_never_called_undeclaredstill assert on it and stay green.provenance.fragmentsandprovenance.notesare optional. A spec read from a document that omits notes is whole by the schema, soexplain(spec, "provenance")still explains from the rest. No quantity inQUANTITIESnames an optional field.Basisgainsempty: tuple[Contribution, ...] = (). These are fields the spec holds that produced no rows.__str__prints them after the contributions, in the samepath = value [source]form, sohost.tokenizers = ()means "held, and empty". A field that was left out now refuses (point 1), so a printed basis has no third state to confuse with these two.contributionsis unchanged, so compass(spec): explain refuses an unknown term as ADDRESSING and an absent one as TOTALITY #264's "an empty table explains to zero rows" still holds exactly:basis.contributions == ().What surprised me
test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusingassertedstr(basis)was the header alone. That is the behaviour the issue comment asks to change. Itscontributions == ()assertion is kept, and itsstrassertion now expects thehost.tokenizers = ()line. This is the one landed test whose body changed.test_a_block_an_assembled_spec_holds_nothing_under_names_its_first_fieldasserted the first-only behaviour by name. It is renamed..._names_every_fieldand now expects bothhost.ipcfields. Its node id changes.test_spec_schema.pydid not move. The required-absentSpecRefusal(...)site moved fromrefuse_absent_fieldintorefuse_absent_fields, but it is still one TOTALITY site inrules.py. Explain and the accessor still build a single-field refusal at the same site, andtest_explain_declines_an_unknown_term_and_an_absent_one_at_two_sitesis green unmodified.test_spec_schema.pyis not in the diff.Left undone
admissionthe tokenizer table comes last anyway. For a block such ashostthe empty line comes after theipcandadmissionrows. Interleaving would mean carrying a per-field grouping throughBasis, which is more than this issue needs.provenance.notesunderprovenance) is still silent in the printed basis. It is not a dependency of any quantity, so I left it alone. If a reader needs to see it, the fix is the sameempty-style line with a "not stated" marker.Gates (node 18,
xiaobizh_n18_cpu,git archivestaging under/tmp/i265gates, removed afterwards)Gate 1: ATOM's suite, unmodified, via the tree's own
scripts/compass/gate_cpu.sh, with.compass-commit/.compass-changedstampsatom.__file__GATE_CPU_RC77d203b86(stamp)/tmp/i265gates/control/ATOM/atom/__init__.py04bc6e51d(stamp)/tmp/i265gates/branch/ATOM/atom/__init__.pyBoth runs printed
gpu: not required. The delta is +4 passed, which matches the node-id diff oftests/compasscollection (control 1155 ids, branch 1159):tests/compass/test_spec_verbs.py::test_a_quantity_missing_one_field_is_refused_rather_than_explained_from_the_resttests/compass/test_spec_verbs.py::test_every_missing_field_under_a_quantity_is_named_and_not_only_the_firsttests/compass/test_spec_verbs.py::test_an_empty_tokenizer_table_is_printed_as_empty_under_a_quantitytests/compass/test_spec_verbs.py::test_a_block_explains_without_an_optional_field_the_document_left_outtests/compass/test_spec_verbs.py::test_a_block_an_assembled_spec_holds_nothing_under_names_every_field(rename)tests/compass/test_spec_verbs.py::test_a_block_an_assembled_spec_holds_nothing_under_names_its_first_field(renamed, above)tests/compass/test_spec_verbs.py::test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusinggit merge-tree --write-tree 77d203b86 04bc6e51dgives03cf301edbafc144876fd493eedb970c32d4452dwith rc 0. That equals the head tree, because the branch sits directly on the tip.Gate 2: the new CPU-only tests listed above.
test_spec_verbs.pyandtest_spec_schema.pytogether: 289 passed.Gate 3: the named result, with line-count-preserving mutations of
explain.py(229 → 229 lines each) run overtest_spec_verbs.pyandtest_spec_schema.pyif missing:→if len(missing):if missing:→if ():test_a_quantity_missing_one_field_is_refused_rather_than_explained_from_the_rest,test_every_missing_field_under_a_quantity_is_named_and_not_only_the_first,test_a_block_an_assembled_spec_holds_nothing_under_names_every_fieldrefuse_absent_fields(missing[:1])..._named_and_not_only_the_first,..._names_every_field…required or 1)test_an_optional_field_the_document_left_out_is_refused_as_absent,test_a_block_explains_without_an_optional_field_the_document_left_out,test_spec_schema.py::test_explain_declines_an_unknown_term_and_an_absent_one_at_two_sitesself.contributions + ()test_an_empty_tokenizer_table_explains_to_no_rows_rather_than_refusing,test_an_empty_tokenizer_table_is_printed_as_empty_under_a_quantityThe named test asserts
str(refusal) == str(spec.value("device.memory.capacity_bytes") refusal). It pins both the refusal and the shared wording.Gate 4: the reviewer is dispatched by the coordinator.
Size
The estimate was about 15 production lines and 30 test lines. These are AST code lines: docstrings, comments and blank lines are excluded.
explain.py+16/−6,rules.py+17/−8)test_spec_verbs.py)Net production is 1.3x the estimate. Gross added is 2.2x, and 8 of those 33 lines are the existing required-field
SpecRefusal(...)moved fromrefuse_absent_fieldinto its sibling. Tests are 1.2x net. Rawgit diff --numstat:explain.py30/10,rules.py27/8,test_spec_verbs.py60/8.🤖 Generated with Claude Code