finos / finos/architecture-as-code

Validation: first-class cycle detection for recursive detailed-architecture traversal

Open
#2,821 0 comments 0 reactions 0 assignees View on GitHub
validator
Dominant language
TypeScript
Stars
399
Forks
138
Avg merge
2d 6h
Merged PRs (30d)
38

Description

## 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), 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 #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).

### Alternatives Considered:
- Do nothing (status quo from #2805): safe against hangs/OOM but never reports a cycle. Rejected
because a genuine cyclic reference is a real modelling error worth surfacing.
- Flat `seen` set only (pre-#2805): caused sibling errors to vanish and could not report cycles.
Already improved by #2805.

### Testing Strategy:
- 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.

Contributor guide

Open the contributing guide

Research direction

Read shared/src/resolver/caching-tracking-resolver.ts, shared/src/commands/validate/validate-node-details.ts, and ValidationTechnicalSpecification.md sections 6 and 8. Start by reviewing the existing resolver and node-details tests, then clarify the traversal-stack design and validation output. Done means A→B→A reports one cycle-detected finding with its path, while diamond and sibling references remain silently de-duplicated, with tests and documentation updated.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
tooling
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.