finos / finos/architecture-as-code

Validation: report a loaded-but-invalid shared sub-architecture's errors once

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

Description

## Feature Proposal

Follow-up to PR #2805 (validation engine redesign). During review @willosborne noted that a
sub-architecture with faults that is referenced by many nodes could get "very, very noisy" if its
errors are reported once per referencing node, and that ideally such errors should be reported once,
but that this can be deferred to keep #2805 mergeable. This issue captures that follow-up.

### Target Project:
`shared/` (validation engine), primarily
`shared/src/commands/validate/validate-node-details.ts`.

### Description of Feature:
De-duplicate validation findings for a `detailed-architecture` sub-architecture that *loads
successfully but is itself invalid* and is referenced by multiple nodes, so its intrinsic errors are
reported once rather than repeated at every referencing node's path.

Note the deliberate asymmetry agreed on #2805:
- A *failed-to-load* reference is reported at every referencing node (so the user can fix all call
sites at once), implemented in #2805.
- A *loaded-but-invalid* sub-architecture's own errors should be reported once (this issue), because
repeating potentially many intrinsic errors across many referencing nodes is pure noise.

### User Stories:
- As an architect, I want the intrinsic validation errors of a shared, invalid sub-architecture to be
reported once so that a widely-referenced faulty sub-architecture does not flood my output with
duplicate findings.
- As an architect, I still want a sub-architecture that *fails to load* to be reported at every
referencing node (unchanged from #2805), so I can find and fix all the broken references.

### Current Limitations:
`validate-node-details` de-duplicates *traversal* (each sub-architecture is validated once via the
`CachingTrackingResolver` `seen`/`cache` tracking), so in the common case a successfully-loaded
sub-architecture is only validated (and therefore only reported) once. This issue exists to:
- confirm and lock that behaviour with explicit tests, and
- define the intended semantics if/when a future change (e.g. per-node contextual validation, or the
internal "validate against a specified `required-pattern`" requirement) causes the same
sub-architecture to be validated in more than one context, which could reintroduce duplicate
intrinsic findings.

### Proposed Implementation:
- Add explicit tests asserting that a loaded-but-invalid sub-architecture referenced by N nodes yields
its intrinsic errors once (attributed to a single, well-defined path), while a failed-to-load
reference still yields one finding per referencing node.
- If duplication is reintroduced by later work, de-duplicate intrinsic sub-architecture findings by
(sub-architecture URL + finding identity), keeping the failed-load per-node behaviour separate.
- Technical design considerations:
- Decide the canonical path attribution for once-only intrinsic findings (first referencing node,
the sub-architecture URL itself, or a synthesised path).
- Ensure de-duplication keys on stable finding identity (code + path + message) to avoid dropping
genuinely distinct findings.
- API changes: none expected (internal to node-details result aggregation).
- Data model changes: none.
- Dependencies on other components: interacts with Issue (cycle detection) follow-up and any future
required-pattern contextual validation.

### Alternatives Considered:
- Report intrinsic errors per referencing node: rejected as too noisy for widely-shared invalid
sub-architectures (@willosborne, #2805 point 3).
- Always report once for everything (including load failures): rejected because a failed reference is
a per-call-site problem the user needs to see at every node (#2805 point 1).

### Testing Strategy:
- Unit: two nodes referencing the same loadable-but-invalid sub-architecture -> its intrinsic errors
appear once.
- Unit: two nodes referencing the same *unloadable* sub-architecture -> one load error per node
(regression guard for the #2805 behaviour).
- Regression: existing node-details specs continue to pass.

### Documentation Requirements:
- Update `shared/src/commands/validate/ValidationTechnicalSpecification.md` §6/§8 to record the
agreed asymmetry (failed-load per node vs invalid-content once).

### 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 3)
and `ValidationTechnicalSpecification.md` §8.

Contributor guide

Open the contributing guide

Research direction

Start with shared/src/commands/validate/validate-node-details.ts and read ValidationTechnicalSpecification.md sections 6 and 8, along with the validation changes from PR #2805. Add tests for loaded-but-invalid shared sub-architectures and for unloadable references, preserving their different reporting behavior. Done means the duplicate intrinsic findings are reported once, load failures remain per referencing node, existing node-details specs pass, and the specification is updated.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend, testing-qa
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.