You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up to PR #2805 (validation engine redesign). During review @willosborne noted that, given the
size of that refactor, we should keep the cycle-safety story minimal for now: as long as a cycle does
not hang or OOM the process we are fine, and true cycle detection can be a follow-up. This issue
captures that follow-up.
Target Project:
shared/ (validation engine), primarily shared/src/resolver/caching-tracking-resolver.ts and shared/src/commands/validate/validate-node-details.ts.
Description of Feature:
Introduce first-class cycle detection for recursive detailed-architecture traversal that
distinguishes a genuine cycle (a reference that is an ancestor in the current traversal path) from
plain sibling de-duplication (a reference already resolved elsewhere in the tree).
Today both are collapsed into a single "already seen" check. PR #2805 made that check safe (a
successfully-resolved reference short-circuits recursion, and a failed reference is re-reported per
node) so the process cannot hang or OOM, but it still cannot tell the user why a reference was not
re-descended into, nor report an actual cycle as a validation finding.
User Stories:
As an architect, I want a real cyclic detailed-architecture reference to be reported as a clear
validation finding (with the cycle path) so that I can fix the loop, rather than it being silently
de-duplicated.
As a maintainer, I want cycle detection separated from sibling de-duplication so that the two
concerns can evolve independently and are individually testable.
Current Limitations:
CachingTrackingResolver tracks a flat seen set. validate-node-details skips any reference where has(url) && !hasFailed(url). This conflates:
a genuine cycle (A -> B -> A), and
an unrelated sibling node referencing the same already-resolved sub-architecture.
Both are treated as "skip". There is no notion of the current traversal path (ancestry), so a true
cycle cannot be surfaced as its own diagnostic; it is only implicitly prevented from recursing.
Proposed Implementation:
Track an explicit traversal stack (ancestry) in addition to the global seen/cache sets, either
inside a dedicated model walker or threaded alongside the existing CachingTrackingResolver.
On encountering a reference already present in the current ancestry, emit a dedicated cycle-detected validation output (severity to be decided) that includes the cycle path, rather
than silently returning an empty result.
Keep sibling de-duplication (reference resolved elsewhere but not an ancestor) as a silent skip, as
today.
Technical design considerations:
Reuse the existing Resolvable / CalmReferenceResolver seam; avoid a parallel bespoke visited
set in the caller (the original OOM on feat(shared): validate node details and controls #2778 came from a dropped visited set).
Decide whether cycle detection lives in the resolver, in a ModelWalker, or in the node-details
rule; align with the "one first-class cycle-safe walk" direction in ValidationTechnicalSpecification.md.
API changes: potentially a new ValidationOutput code (cycle-detected); possibly a helper on the
resolver/walker to expose the current ancestry.
Data model changes: none expected.
Dependencies on other components: node-details validation, controls validation (any recursive
resolution path).
Unit: A -> B -> A produces a single cycle-detected finding with the cycle path; A -> B -> C
(diamond, C referenced twice, no cycle) produces no cycle finding and validates C once.
Unit: sibling nodes referencing the same valid sub-architecture still de-duplicate silently.
Regression: existing node-details and resolver specs continue to pass.
Documentation Requirements:
Update shared/src/commands/validate/ValidationTechnicalSpecification.md §6 (reference resolution
/ cycle safety) and move this item out of §8 (open questions) once implemented.
Implementation Checklist:
Design reviewed and approved
Implementation completed
Tests written and passing
Documentation updated
Relevant workflows updated (if needed)
Performance impact assessed
Additional Context:
Split out of PR #2805 to keep that PR mergeable. See @willosborne's review comment on #2805 (point 2)
and ValidationTechnicalSpecification.md §8.
Feature Proposal
Follow-up to PR #2805 (validation engine redesign). During review @willosborne noted that, given the
size of that refactor, we should keep the cycle-safety story minimal for now: as long as a cycle does
not hang or OOM the process we are fine, and true cycle detection can be a follow-up. This issue
captures that follow-up.
Target Project:
shared/(validation engine), primarilyshared/src/resolver/caching-tracking-resolver.tsandshared/src/commands/validate/validate-node-details.ts.Description of Feature:
Introduce first-class cycle detection for recursive
detailed-architecturetraversal thatdistinguishes a genuine cycle (a reference that is an ancestor in the current traversal path) from
plain sibling de-duplication (a reference already resolved elsewhere in the tree).
Today both are collapsed into a single "already seen" check. PR #2805 made that check safe (a
successfully-resolved reference short-circuits recursion, and a failed reference is re-reported per
node) so the process cannot hang or OOM, but it still cannot tell the user why a reference was not
re-descended into, nor report an actual cycle as a validation finding.
User Stories:
detailed-architecturereference to be reported as a clearvalidation finding (with the cycle path) so that I can fix the loop, rather than it being silently
de-duplicated.
concerns can evolve independently and are individually testable.
Current Limitations:
CachingTrackingResolvertracks a flatseenset.validate-node-detailsskips any reference wherehas(url) && !hasFailed(url). This conflates:Both are treated as "skip". There is no notion of the current traversal path (ancestry), so a true
cycle cannot be surfaced as its own diagnostic; it is only implicitly prevented from recursing.
Proposed Implementation:
seen/cachesets, eitherinside a dedicated model walker or threaded alongside the existing
CachingTrackingResolver.cycle-detectedvalidation output (severity to be decided) that includes the cycle path, ratherthan silently returning an empty result.
today.
Resolvable/CalmReferenceResolverseam; avoid a parallel bespoke visitedset in the caller (the original OOM on feat(shared): validate node details and controls #2778 came from a dropped
visitedset).ModelWalker, or in the node-detailsrule; align with the "one first-class cycle-safe walk" direction in
ValidationTechnicalSpecification.md.ValidationOutputcode (cycle-detected); possibly a helper on theresolver/walker to expose the current ancestry.
resolution path).
Alternatives Considered:
because a genuine cyclic reference is a real modelling error worth surfacing.
seenset only (pre-feat(shared): Validation Engine Redesign with articulated assumptions #2805): caused sibling errors to vanish and could not report cycles.Already improved by feat(shared): Validation Engine Redesign with articulated assumptions #2805.
Testing Strategy:
cycle-detectedfinding with the cycle path; A -> B -> C(diamond, C referenced twice, no cycle) produces no cycle finding and validates C once.
Documentation Requirements:
shared/src/commands/validate/ValidationTechnicalSpecification.md§6 (reference resolution/ cycle safety) and move this item out of §8 (open questions) once implemented.
Implementation Checklist:
Additional Context:
Split out of PR #2805 to keep that PR mergeable. See @willosborne's review comment on #2805 (point 2)
and
ValidationTechnicalSpecification.md§8.