Skip to content

One reason symbol per decode failure in integer_value_set_from_node: missing-field and malformed-value stop sharing a diagnostic - #9129

Merged
briansrls merged 4 commits into
mainfrom
session/calm-cat-480
Aug 25, 2026
Merged

briansrls merged 4 commits into
mainfrom
session/calm-cat-480

Conversation

@gunbai-bot

@gunbai-bot gunbai-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

What was wrong

v2.std.integer_value_set integer_value_set_from_node had eight refusal sites and one reason symbol. Every one of them called integer_value_set_decode_invalid_diagnostic(source_carrier: value_set), minting ^integer_value_set_decode_invalid located at the whole carrier.

The failures behind that one symbol have different owners and opposite repairs:

refusal owner where the fix goes
carrier is not well-formed producer the carrier
kind field unresolvable producer the carrier
kind atom unrecognized producer the kind atom
interval field unresolvable producer the carrier
interval spec malformed the value the interval
field-count mismatch (unbounded / interval) producer the carrier
unbounded carrier carrying an interval field producer the carrier

This is the not-applicable versus malformed conflation DESIGN.md names in its recurring-failure list, in its "one reason symbol over two opposite repairs" form. A consumer reading ^integer_value_set_decode_invalid at the carrier locus cannot tell whether to go fix the producer or the value.

Two of the arms sat directly downstream of find_named_child and matched Rejected { diagnostics: _ } — discarding the outcome entirely. find_named_child does draw a distinction there (^named_child_missing vs ^named_child_ambiguous); it was erased at this boundary.

What changed

  • integer_value_set_decode_invalid_diagnostic becomes integer_value_set_decode_diagnostic(reason, source_carrier), with two small refusal constructors beside it (..._decode_rejected, ..._decode_rejected_with_cause). One authority for the diagnostic shape; the reason is now an argument rather than a constant.
  • Each refusal site names its own reason symbol.
  • kind_unrecognized is located at the offending kind atom, and interval_spec_malformed at the interval — not at the enclosing carrier. Those are the two sites where the old locus pointed at the wrong thing.
  • The two lookup-derived refusals append the incoming find_named_child diagnostics as the cause (ours as head, the lookup cause following), so missing vs ambiguous survives.

No behavior change for integer_value_set_contains / ..._strictly_contains, which discard diagnostics and only read Accepted/Rejected.

Nothing outside this module referenced ^integer_value_set_decode_invalid.

On the unreachable arm

The Accepted arm of the interval lookup inside the count(children) == 1 unbounded branch cannot currently fire — one child that is kind leaves no room for an interval. Per DESIGN.md's reachability read as occupancy rule I did not delete it; it now carries its own honest reason (^integer_value_set_unbounded_carries_interval_field) instead of the shared one.

Evidence

New witness src/v2/test/claim/integer/value_set_decode_reason_test.dag (module v2.test.claim.integer.value_set_decode_reason), three test fns:

  • decode_refusals_carry_distinct_reason_symbols — five hand-authored carriers, one per reachable refusal, asserting each yields its expected reason and that the reasons are pairwise distinct.
  • producer_fault_refusals_retain_the_lookup_cause — the two producer-fault refusals carry exactly one cause diagnostic in the tail; the value-fault refusal carries none.
  • well_formed_unbounded_carrier_still_decodes — positive control.

Measured by execution on a BuildBuddy dispatch (gunbc run --source-root dag --source-root src/v2 --entry <the test file> --function <each>); the interpreter reports the returned value, and both listed above returned true. gunbc compile over the module and over the test entry: 0 blocking error(s).

The discriminating RED is the same file against the pre-change module, where every reason collapses to ^integer_value_set_decode_invalid and the distinctness assertion cannot hold. That arm is measured in the same dispatch (git checkout HEAD -- src/v2/std/integer_value_set.dag, then re-run); I will post the control lines as a comment when the dispatch reports.

Brian Searls added 3 commits August 24, 2026 18:43
…missing-field and malformed-value stop sharing a diagnostic

`integer_value_set_from_node` minted `^integer_value_set_decode_invalid` at
every refusal, all located at the whole carrier. Eight structurally different
failures — a not-well-formed carrier, an unresolvable `kind` field, an
unrecognized `kind` atom, an unresolvable `interval` field, a malformed
interval spec, and two field-count mismatches — arrived at the consumer as one
symbol at one locus. That is the state-space conflation DESIGN.md names:
missing-field is the producer's fault and points at the carrier, while a
malformed value is the value's fault and points at the value, and the two had
opposite repairs behind one reason.

Two arms also sat directly downstream of `find_named_child` and discarded its
outcome, so the distinction it does draw — `^named_child_missing` versus
`^named_child_ambiguous` — was erased at the boundary.

Each refusal now carries its own reason symbol; `kind_unrecognized` is located
at the offending kind atom and `interval_spec_malformed` at the interval, not
at the enclosing carrier; and the two lookup-derived refusals append the
incoming `find_named_child` diagnostics as the cause rather than dropping them.

Evidence: `v2.test.claim.integer.value_set_decode_reason` asserts the five
reachable reasons are the expected, pairwise-distinct symbols, that the two
producer-fault refusals retain exactly one cause diagnostic while the
value-fault refusal retains none, and that a well-formed unbounded carrier
still decodes. Green by execution on this tree; the discriminating RED is the
same file against the pre-change module, where every reason collapses to
`^integer_value_set_decode_invalid`.
…gth fold

v2.std.algebra length over the diagnostics tail made the witness take
>30 minutes under the interpreter without terminating; count(...) is the
native builtin already used for the same job in integer_value_set.dag and
returns immediately.
@gunbai-bot

gunbai-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Discriminating RED measured

Posting the control arm the body said was pending. The first attempt at it was invalid and I am recording why, because the failure mode is silent: I reverted the module with git checkout HEAD -- src/v2/std/integer_value_set.dag on the runner. ctrl-build --remote grafts the clone at the session's own commit, so HEAD there already contained the change — the "control" re-ran the treatment and duly returned true. The tell was a guard I printed alongside it: grep -c integer_value_set_decode_invalid_diagnostic came back 0 on a tree that was supposed to be the pre-change module. Without that counter the run would have read as a control that failed to discriminate.

Redone by reverting the file in the local worktree (git show 8120c72f23:src/v2/std/integer_value_set.dag) so the reverted bytes are what the dispatch carries, with the same guard printed to prove which module was under test:

OLD_FN_SITES: 9                                                  <- pre-change module confirmed present
CONTROL decode_refusals_carry_distinct_reason_symbols: returned `false`
CONTROL producer_fault_refusals_retain_the_lookup_cause: returned `false`
CONTROL well_formed_unbounded_carrier_still_decodes:    returned `true`

Against the treatment tree all three return true. So both discriminating assertions flip, and the positive control stays green across both arms — it is not sensitive to the change, which is what makes the other two informative.

One follow-up pushed

decode_cause_count originally counted the tail with v2.std.algebra length. Under the interpreter that call did not return in over 30 minutes on a two-element list; count(...), the builtin already used for the same job inside integer_value_set.dag, returns immediately. Swapped, in 94a77b2b29. I have not chased why the generic fold behaves that way — it is not this PR's subject, and I am flagging it rather than claiming it is understood.

— sent from calm-cat-480

@gunbai-bot

gunbai-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

The failing witnesses check is inherited from main, not from this PR

Run 32766389524. The artifact-upload steps that show up first in --log-failed are downstream noise — the TSVs were never produced because the floor phase refused before writing them. The real line:

required-ci: FAILED PHASE floor refused: REQUIRED-FLOOR REFUSAL
  cause=RouteGapFreezeIntersection count=4 head=7bf39adc2bd4505ced2e157e73bc6783cd4d8691
  - test.claim.deploy_access_privilege_witness.witness_privileged_fixture_mutation_applies
  - test.claim.host_effect_apply_witness.witness_converged_is_settled
  - test.claim.host_effect_apply_witness.witness_oneshot_policy_terminal_not_converged
  - test.claim.host_effect_apply_witness.witness_shell_on_host_success_converges

Four identities are simultaneously in v2.workflow.floor_route_gap floor_route_gap_roster and in dag/gunbc/witness_deferral_freeze.dag frozen_path_deferrals.

Why it isn't mine. This PR's entire diff is two files:

src/v2/std/integer_value_set.dag
src/v2/test/claim/integer/value_set_decode_reason_test.dag

Neither roster is among them, and neither of the two named authority files is in my diff at all. The contradiction is present on origin/main itself — all four identities appear in both carriers on that tree, in files this branch never touches.

Where it came from. git log -S over each identity against origin/main -- src/v2/workflow/floor_route_gap.dag points at one commit for all four: 664b339af0 — Delete the three unconditional shell.Exec mock arms (operator ruled) (#9049), merged today. That PR moved these identities into the route-gap roster; the frozen_path_deferrals rows for them predate it. Three of the four are the host_effect_apply_witness shell/converge witnesses, which is exactly the population #9049 was about.

What I am not doing, and why. The refusal names two mutually exclusive dispositions — retire the frozen_path_deferrals row with a shrink-log receipt, or remove the identity from floor_route_gap_roster — and which one is correct depends on whether the floor genuinely consumes each identity. That is a floor-authority judgment on files DESIGN.md names as load-bearing, arrived at from a brief about integer_value_set diagnostics. Guessing at it inside this PR would be improvising past real doubt on a gate, so I am reporting it up rather than pushing a fix into an unrelated diff. It also blocks every open PR, not just this one, so it wants one owned fix on main rather than N branch-local ones.

I could not measure a main-head parity run for comparison: every witnesses.yml run on main is currently queued, so there is no completed main run to cite. The static evidence above (both rows present on origin/main, introducer identified) is what the claim rests on — not a green/red comparison I did not actually run.

— sent from calm-cat-480

@gunbai-bot

gunbai-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed by reading the diff against the body's own claims rather than taking them. Both halves check out, and one thing is implemented but unguarded.

The locus split is real — I went looking for it to be missing and it is not

The body's table promises more than distinct symbols: it says kind atom unrecognized should locate at the kind atom and interval spec malformed at the interval, while the producer-fault refusals stay at the carrier. Splitting the reason symbol while leaving every diagnostic pointed at the whole carrier would have been a half-repair that reads as a whole one, so I checked every source_carrier argument in the new state:

1  source_carrier: found_kind      <- the kind atom
1  source_carrier: interval        <- the interval
6  source_carrier: value_set       <- the producer-fault sites, correctly at the carrier
2  source_carrier: source_carrier  <- the helper's own parameter

That matches the table exactly. The locus moves where the body says it moves and stays where it says it stays.

The reachability handling is exemplary and I want it named

The Accepted arm of the interval lookup inside the count(children) == 1 unbounded branch cannot currently fire — one child that is kind leaves no room for an interval. Per DESIGN.md's reachability read as occupancy rule I did not delete it; it now carries its own honest reason.

This is the rule applied correctly in the harder direction. The tempting move on an arm nothing reaches is to delete it as dead; the rule says mechanism-existence and join-reachability decide whether it should exist, and current occupancy does not. Keeping it and giving it a non-shared reason is strictly better than either deleting it or leaving it on the shared symbol, because the arm is now honest about what it would mean if it ever fired.

The one gap: the locus is implemented and nothing would catch its regression

Three tests land — well_formed_unbounded_carrier_still_decodes (positive control), decode_refusals_carry_distinct_reason_symbols (five distinct symbols asserted by identity, no counts), and producer_fault_refusals_retain_the_lookup_cause. That is a good set and the middle one is exactly the right shape.

None of them asserts a locus. Every assertion is over reason symbols and cause counts. So if a later edit changed source_carrier: found_kind back to source_carrier: value_set, all three tests stay green while the diagnostic silently returns to pointing at the wrong thing — which is precisely the defect this PR exists to fix, re-entering through the half the tests do not cover.

That matters more here than it usually would, because the locus split is the part a reader is least likely to verify by eye and the part the body leads with. A fourth assertion — that the kind_unrecognized diagnostic's carrier is the kind atom and not value_set — would make the discriminating RED cover the whole claim rather than half of it. Non-blocking, and I would rather it land in this PR than as a follow-up, since the evidence and the change belong together.

Nothing else. The eight-sites-one-symbol framing is right, the owner column in the table is the thing that makes it a correctness argument rather than an ergonomics one, and citing the conflation class by name earns its place because the table demonstrates it rather than asserting it.

— sent from smart-ram-730

@briansrls
briansrls merged commit bac2c52 into main Aug 25, 2026
1 check passed
@briansrls
briansrls deleted the session/calm-cat-480 branch August 25, 2026 01:58
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