fix(hir,cli): an unresolvable --root must fail, not fabricate a model (#417) - #418
Merged
Merged
Conversation
…#417) `spar instance --root Totally::Bogus.Impl --format json` printed a fabricated node — the requested package/type/impl echoed back, empty children, empty diagnostics — and exited 0. A typo'd root was indistinguishable from an empty system, and every downstream analysis then found nothing wrong with nothing. TWO INDEPENDENT CAUSES. `SystemInstance::instantiate` returns `Self` and cannot fail. For an unresolved classifier the builder fabricates a `System` root and records "unresolved implementation: …" in `builder.diagnostics` (instance.rs:1479-1485) — which nothing downstream reads, because `to_serializable` hardcodes an empty list (lib.rs:642). And `Database::instantiate` returned `Some` unconditionally, contradicting its own doc comment: "Returns `None` if … the implementation is not found." THE DEFECT WAS PINNED BY A TEST. `instantiate_not_found` asserted `is_some()`, with a comment explaining that "the function always creates a root" — a test whose name says not-found, asserting found, against a doc comment saying None. The documentation and the test disagreed and the test won. Same shape as check_lean_sorries.py:178 before #385: a case that affirms the hole as intended rather than finding it. FIX. Resolve the root classifier before instantiating and return `None` when it is not a ComponentImpl. Deliberately at the root only: the builder legitimately fabricates placeholders for unresolved SUBcomponents so one bad reference does not discard the rest of the tree; it is only the root whose absence makes the whole instance meaningless. The CLI now exits 2 and lists what IS available, because a typo'd root is the common case and a candidate list makes it self-correcting: error: cannot instantiate root 'Totally::Bogus.Impl' no component implementation with that qualified name was found available implementations (5): ConnectionInstantiationExample::proc.one ... FIXING ONE PATH WAS NOT ENOUGH, and this is the part worth keeping. Guarding the JSON path left the human-readable path building its own GlobalScope and instantiating directly, so it still printed "Instance model: Totally::Bogus. / 1 component instances" and exited 0. It warned on stderr — better than silence — but a caller checking the exit code still saw success. The guard is now hoisted above both paths. CORROBORATION. All three spar-wasm call sites already do `.ok_or_else(|| RenderError::NoRoot(...))?`, and one carries the comment "If the root could not be resolved, the instance will contain diagnostics about unresolved implementations. Treat that as a NoRoot error." The renderer was written against the DOCUMENTED contract and had to add a diagnostics-inspection workaround because the implementation did not honour it. This makes their `ok_or_else` fire. EVIDENCE. 4 CLI tests (both paths reject; the error lists candidates; a real root still succeeds) plus the spar-hir pair. Mutation-tested: removing the guard fails 3 of 4 CLI tests and `instantiate_not_found`, while both discriminating partners keep passing. Whole workspace compiles (`--all-targets --no-run`); spar-hir 52, spar-wasm 7, fuzz_pipeline 35 green. Found while building the Tier-B OSATE conformance oracle (#246): this bug would have made that gate pass on a fabricated root — an empty spar projection "agreeing" with a missing reference. Closes #417. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rivet verification gate✅ 20/20 passed
Filter: Failed artifacts(none) Updated automatically by |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Closes #417. Found while building the Tier-B OSATE conformance oracle (#246) —
the setup work found the bug before the oracle existed.
The defect
A typo'd root produced a plausible empty model at exit 0. Every downstream
analysis then reported no problems — because there was nothing to analyse.
Two independent causes
SystemInstance::instantiatereturnsSelfand cannot fail. For anunresolved classifier the builder fabricates a
Systemroot, records"unresolved implementation: …"inbuilder.diagnostics(
instance.rs:1479-1485), and carries on — and nothing reads that, becauseto_serializablehardcodesdiagnostics: vec.Database::instantiatereturnedSomeunconditionally, contradicting its owndoc comment: "Returns
Noneif … the implementation is not found."The defect was pinned by a test
A test named not_found asserting found, against a doc comment saying
None. The documentation and the test disagreed, and the test won. Same shapeas
check_lean_sorries.py:178before #385 — a case that affirms the hole asintended behaviour rather than finding it.
Fix
Resolve the root classifier before instantiating; return
Nonewhen it is not aComponentImpl. Deliberately at the root only — the builder legitimatelyfabricates placeholders for unresolved subcomponents so that one bad reference
does not discard the rest of the tree. It is only the root whose absence makes
the whole instance meaningless.
The CLI exits 2 and lists what is available, because a typo'd root is the common
case and the candidate list makes it self-correcting:
Fixing one path was not enough
Guarding the JSON path left the human-readable path building its own
GlobalScopeand instantiating directly:It warned on stderr — better than the JSON path's silence — but a caller
checking the exit code still saw success. The guard is now hoisted above both
paths. Fixing the instance and leaving the class is the trap this repo keeps
finding (#383, #384, #402).
Corroboration from an unrelated crate
All three
spar-wasmcall sites already do.ok_or_else(|| RenderError::NoRoot(...))?, and one carries the comment:The renderer was written against the documented contract and had to add a
diagnostics-inspection workaround because the implementation did not honour it.
This change makes their
ok_or_elseactually fire.Evidence
instantiate_not_found(now assertsNone) +instantiate_found_still_worksinstantiate_not_found; both discriminating partners keep passingcargo test --workspace --all-targets --no-runclean; spar-hir 52, spar-wasm 7, fuzz_pipeline 35 green;cargo fmt --all --checkcleanThe CLI-level tests exist because the property users depend on is the exit
code, and a library-level test cannot see that the human path was still
fabricating.
Why this blocked #246
A Tier-B gate comparing spar to OSATE would have passed on a fabricated root
— an empty spar projection "agreeing" with a missing reference. The empty-passes
trap, one layer up.
🤖 Generated with Claude Code