test: make the read-honesty guard see the fields it was meant to protect - #107
Merged
Conversation
The default-masking check compared each seed against the schema default. Seeds are keyed by uci option name and schema_properties by wire name, so every field whose two names differ failed the lookup and was skipped without a word. It inspected 15 of 140 seeded keys. What that cost, measured rather than argued: deleting the `PasswordAuth` write from dropbear.instances.uc passed all 1126 tests. That deletion silently re-enables SSH password authentication for any client that reads a section and writes it back, which is precisely the class the read-honesty property exists to catch. Dropping `masq` from firewall.zones, which silently disables NAT the same way, also passed. The guard now compares the read-back value against the default instead. That needs no name mapping at all, because it asks the question the property actually depends on: if toUci dropped this field, would the re-read refill it and hide the loss? Any resource whose uci names differ from its wire names is covered for free, including the ones nobody has written yet. It reports per case rather than as one lump, so a failure names the resource instead of a 100-item string. That exposed 101 blind pairs across 34 of the 46 cases, all now seeded off their defaults: 99 keys added, 2 corrected where the fixture already set the field to its default further down the literal. `network.rules.action` is seeded `unreachable` rather than the first non-default enum member, since `goto` requires a companion field the round trip does not send. Both deletions above now fail the suite.
raspbeguy
force-pushed
the
fix/read-honesty-default-guard
branch
from
August 8, 2026 09:36
1956f34 to
f4133e9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finding 3 of the
v2.4.1..mainreview, and the one I would fix first if I could only fix one. Independent of #101 through #106.The read-honesty property has a guard whose whole job is to stop a field hiding behind its own default: if a seed sits at the default, dropping the field from
toUciis invisible, because the re-read synthesizes the default again and before matches after.That guard compared the seed against the default. Seeds are keyed by uci option name;
schema_propertiesis keyed by wire name. So every field whose two names differ failed the lookup and was skipped in silence. It inspected 15 of 140 seeded keys.What that cost, measured rather than argued. Deleting the
out.PasswordAuthline fromdropbear.instances.uc::toUcipassed all 1126 tests:That deletion silently re-enables SSH password authentication for any client that reads a section and writes it back, which is exactly the class this property exists to catch. Dropping
out.masqfromfirewall.zones, which silently disables NAT the same way, also passed.The fix is to compare the read-back value instead of the seed. That needs no name mapping, because it asks the question the property actually depends on: if
toUcidropped this field, would the re-read refill it and hide the loss? Every resource whose uci names differ from its wire names is covered for free, including ones nobody has written yet. It also reports per case rather than as one lump, so a failure names the resource instead of returning a 100-item string.101 blind pairs across 34 of the 46 cases, all now seeded off their defaults: 99 keys added, 2 corrected where the fixture already set the field to its default further down the same object literal, which silently overrode the value I had just inserted. Worst offenders were
prometheus_node_exporter_lua(18),openvpn.instances(10),dhcp.dnsmasq(9) andlldpd.config(7).Both deletions above now fail:
Does this change the wire surface? No. Tests and fixtures only.
Was it verified against a real device, or only by CI? Neither, and deliberately: this is a test-harness change with no runtime component. The verification that matters is the planted-failure pair above, run against the full suite.
tests/integration/47_read_honesty_test.shcovers the hardware half of the same property and is untouched.Was LuCI checked? Not applicable.
Two things worth flagging for review.
network.rules.actionis seededunreachablerather than the first non-default enum member, becausegotorequires a companion field a faithful round trip does not send. And no exemption list was needed: all 101 pairs were confirmed writable to a non-default value before I started, so a case that cannot be seeded off its default would be a genuine new finding rather than an expected exception.