feat(pkg/go): report the relations in a cycle the graph cannot resolve - #668
Draft
SoulPancake wants to merge 1 commit into
Draft
feat(pkg/go): report the relations in a cycle the graph cannot resolve#668SoulPancake wants to merge 1 commit into
SoulPancake wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SoulPancake
force-pushed
the
feat/graph-validation-api
branch
from
August 21, 2026 13:31
6f98202 to
1e1f520
Compare
SoulPancake
force-pushed
the
experiment/graph-refusal-identities
branch
from
August 21, 2026 13:31
d42d5b8 to
d65f0ff
Compare
Build refuses a model whose relations sit in a cycle that cannot be resolved and returns no graph alongside the refusal, so there is nothing left to read for a position. Two of those shapes leave every relation in them satisfiable, which means the rewrite-tree walk finds an entry point for all of them and reports nothing: a cycle where no step reads a tuple, so going round it never gets closer to an answer, and a cycle that does read a tuple but runs through an operand of an intersection or an exclusion, which the resolver cannot subtract while it is still computing it. Of the 38 shared corpus models Build refuses, 18 come back with a message naming no relation, and those 18 are these two shapes. cycle_shape.go finds both over the model and raises one finding per relation, so a caller gets a line rather than a sentence about the whole model. The graph stays the authority on whether a model resolves, and TestCycleShapesAgreeWithTheBuilder holds this to it by allowing findings only on models Build refuses: it reports on 18 of the 38 refused and on 0 of the 51 accepted. Enumerating simple cycles is exponential in the worst case, so a relation's search is capped, and one that hits the cap goes unreported rather than reported from a lower bound that would give one relation of a cycle a finding and the next one nothing. The worst corpus relation walks 7 steps of the 200,000 allowed. Cycle findings run only where the walk found nothing, because the relations the walk names are the same relations a cycle runs through and one finding per relation is enough. Where neither says anything the refusal is reported as itself, positionless, which no corpus case reaches. That last wording comes from the sentinel the refusal carries rather than from the builder's message. AssignWeights chooses which problem to report by ranging over a map, so a model with more than one gets a message naming whichever came out first, and a different one on the next run. One corpus case does this, alternating between page#member and ticket#member over repeated builds. Validation reports both of those as missing-definition either way, so nothing is lost by wording the finding from the sentinel, and the finding is then the same every run, which is what lets a consumer cache or diff it. The builder's error is chained under ErrModelNotBuildable so a caller that wants the original text can still reach it, and TestEveryGraphSentinelHasAWording keeps the table exhaustive. Only accepted graphs are read anywhere in this path. Fingerprinting all 51 accepted corpus graphs over 40 builds each, covering weights, node types, tuple-cycle flags, recursive labels and wildcards, gives one distinct graph per model and no model that flips between accepted and refused, so a built graph is stable to read. A refused one is not available to read at all. pkg/go/graph is untouched: it is byte-identical to develop in this tree. CyclicRelation joins the emitted error types with its own sentinel, ErrRelationInUnresolvableCycle. Its documentation page and graph-model-unbuildable's are rewritten against the messages the code now produces, both having documented examples that report something else. The five new shared corpus cases put an unsatisfiable relation among satisfiable ones, where walk order can change the answer; every pre-existing case breaks every relation in its model, so order cannot show up in any of them. Their expected values were taken from the Go validator, and pkg/js reports the same. One of the five expects two relations of one type, which no case did before, and that is order-sensitive in Java. ModelValidator raises entrypoint findings while iterating the relations HashMap's keySet, so their order is by hash bucket: brokenA and brokenB differ by one in hashCode and fall either side of a bucket boundary. It now iterates the names sorted, which is what pkg/go does in cycle_detection.go, so all three languages report them in the same order. No test outside the shared corpus asserts an entrypoint message in Java.
SoulPancake
force-pushed
the
experiment/graph-refusal-identities
branch
from
August 21, 2026 13:43
d65f0ff to
ccee2e7
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
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.
Build refuses a model whose relations sit in a cycle that cannot be resolved and returns no graph alongside the refusal, so there is nothing left to read for a position. Two of those shapes leave every relation in them satisfiable, which means the rewrite-tree walk finds an entry point for all of them and reports nothing: a cycle where no step reads a tuple, so going round it never gets closer to an answer, and a cycle that does read a tuple but runs through an operand of an intersection or an exclusion, which the resolver cannot subtract while it is still computing it. Of the 38 shared corpus models Build refuses, 18 come back with a message naming no relation, and those 18 are these two shapes.
cycle_shape.go finds both over the model and raises one finding per relation, so a caller gets a line rather than a sentence about the whole model. The graph stays the authority on whether a model resolves, and TestCycleShapesAgreeWithTheBuilder holds this to it by allowing findings only on models Build refuses: it reports on 18 of the 38 refused and on 0 of the 51 accepted. Enumerating simple cycles is exponential in the worst case, so a relation's search is capped, and one that hits the cap goes unreported rather than reported from a lower bound that would give one relation of a cycle a finding and the next one nothing. The worst corpus relation walks 7 steps of the 200,000 allowed.
Cycle findings run only where the walk found nothing, because the relations the walk names are the same relations a cycle runs through and one finding per relation is enough. Where neither says anything the refusal is reported as itself, positionless, which no corpus case reaches.
That last wording comes from the sentinel the refusal carries rather than from the builder's message. AssignWeights chooses which problem to report by ranging over a map, so a model with more than one gets a message naming whichever came out first, and a different one on the next run. One corpus case does this, alternating between page#member and ticket#member over repeated builds. Validation reports both of those as missing-definition either way, so nothing is lost by wording the finding from the sentinel, and the finding is then the same every run, which is what lets a consumer cache or diff it. The builder's error is chained under ErrModelNotBuildable so a caller that wants the original text can still reach it, and TestEveryGraphSentinelHasAWording keeps the table exhaustive.
Only accepted graphs are read anywhere in this path. Fingerprinting all 51 accepted corpus graphs over 40 builds each, covering weights, node types, tuple-cycle flags, recursive labels and wildcards, gives one distinct graph per model and no model that flips between accepted and refused, so a built graph is stable to read. A refused one is not available to read at all.
pkg/go/graph is untouched: it is byte-identical to develop in this tree.
CyclicRelation joins the emitted error types with its own sentinel, ErrRelationInUnresolvableCycle. Its documentation page and graph-model-unbuildable's are rewritten against the messages the code now produces, both having documented examples that report something else. The five new shared corpus cases put an unsatisfiable relation among satisfiable ones, where walk order can change the answer; every pre-existing case breaks every relation in its model, so order cannot show up in any of them. Their expected values were taken from the Go validator. pkg/js and pkg/java have not been run against them.
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
main