feat(shared): Validation Engine Redesign with articulated assumptions

[LeighFinegold]

Description

Follow-up to @willosborne's PR #2778 (#2778). I believe this starts to
address the concerns raised on that PR and the assumptions we have been making about the validation
framework, by reworking recursive detailed-architecture and control validation into a cycle-safe,
resolver-driven design. Still some open questions but speaking to @willosborne he thinks likely can be tackled in subsequent PRs and this does handle additional requirements that PR #2778 was adding.

Summary of the design (see
shared/src/commands/validate/ValidationTechnicalSpecification.md
for detail):

• Validation phases are modelled as first-class ValidationRules run by a ValidationEngine, keeping
  the existing ValidationOutcome contract (output buckets, ordering, error/warning flags).
• A single cycle-safe, resolver-driven ModelWalker owns model traversal, dereferencing and cycle
  detection. This fixes the pattern-less dispatch branch dropping the visited set (the self-reference
  heap OOM flagged on feat(shared): validate node details and controls  #2778).
• Reference resolution is driven through the model's Resolvable / CalmReferenceResolver seam, with
  a CachingTrackingResolver giving fetch-at-most-once and validate-once/cycle-safety in one place.
• Node-details and control validation reuse that seam; error/message helpers are consolidated onto the
  shared getErrorMessage.
• ValidationTechnicalSpecification.md also records the open questions the framework does not yet
  settle: default recursive traversal vs a caller-selected resolution policy (same always-on concern
  raised on feat(shared): validate node details and controls  #2778), custom/organisation-specific rules per document, a typed CALM document input
  instead of four positional params, and how validation should differ between the CLI and CalmHub.

This is intended as a discussion PR: it restates the validation design and surfaces the remaining
unknowns rather than claiming to close them.

Design detail (from the Validation Technical Specification)

The full write-up lives in
shared/src/commands/validate/ValidationTechnicalSpecification.md.
The key parts are inlined below so the design is reviewable directly on the PR.

Reference resolution, caching and cycle safety

The CALM model is lazily dereferenced: a ResolvableAndAdaptable<CalmCoreSchema, CalmCore> only
calls resolve(ref) when a caller chooses to dereference it, and the resolve function returns the
raw CalmCoreSchema before it is adapted to CalmCore. Node-details validation needs that raw
document (for Spectral + JSON-Schema), so the resolve seam is the natural place to centralise:

• caching: a reference is fetched at most once; repeat requests return the cached raw document;
• tracking: every attempted reference is recorded, giving dedupe ("validate each
  detailed-architecture once") and cycle safety without each caller keeping its own Set.

CachingTrackingResolver is a decorator that implements CalmReferenceResolver, wrapping any
inner resolver. This is the single resolver interface used by both the model dereference path and
validation:

classDiagram
    class CalmReferenceResolver {
        <<interface>>
        +canResolve(ref) boolean
        +resolve(ref) Promise
    }
    class CachingTrackingResolver {
        -delegate: CalmReferenceResolver
        -cache: Map~string, unknown~
        -seen: Set~string~
        +canResolve(ref) boolean
        +resolve(ref) Promise
        +has(ref) boolean
        +get(ref) unknown
        +markSeen(ref) void
        +resolvedReferences: ReadonlySet~string~
    }
    class SchemaDirectoryReferenceResolver {
        -schemaDirectory: SchemaDirectory
        +canResolve(ref) boolean
        +resolve(ref) Promise
    }
    CalmReferenceResolver <|.. CachingTrackingResolver
    CalmReferenceResolver <|.. SchemaDirectoryReferenceResolver
    CachingTrackingResolver o-- CalmReferenceResolver : decorates

Loading

A reference is marked seen before the underlying load is awaited, so a failed load still counts as
seen (a sibling referencing the same failing URL is not retried), matching the visitedUrls.add
-before-load behaviour originally implemented in PR #2778
(#2778). Only successful loads populate the value cache. In validation the
decorator wraps a SchemaDirectoryReferenceResolver (which loads architecture documents via
schemaDirectory.loadDocument(ref, 'architecture')); in docify the caller (TemplateProcessor)
composes the decorator around its resolver (Mapped -> Composite) so repeated references are
fetched once. Both DereferencingVisitor and ModelWalker are agnostic: they dereference through
whatever CalmReferenceResolver they are given; caching/tracking is purely the caller's composition
choice.

Resolution policy (design intent)

Whether a given reference is resolvable is governed by a ReferenceResolutionPolicy, consulted in
canResolve. The policy is a constructor input to the resolver adapter; the CalmReferenceResolver
interface is unchanged. Two variants:

• shallow (default): canResolve returns false for detailed-architecture document
  references, so a shallow run never fetches or recurses into sub-architectures.
• deep (opt-in): canResolve allows those references, so the node-details rule resolves and
  validates each referenced sub-architecture.

Because only the node-details rule uses ValidationContext.references (schemas load via
SchemaDirectory.getSchema directly), the policy affects recursion alone. Child validation contexts
reuse the same resolver, so the policy is inherited at every level of recursion. This is design
intent; the policy type, its resolver wiring and the entry-point option are pending implementation.

Open questions / Out of Scope

These are recorded in the specification as design intent, not settled behaviour:

• Default traversal and resolution policies. By default, validation recurses into a node's
  detailed-architecture. This rewrite still suffers from the same always-on concern raised on
  feat(shared): validate node details and controls  #2778; making it a caller-selected resolution policy is worth considering.
• Custom rules versus the default traversal. Organisation-specific rules may apply only to
  specific documents or document types, which the generic traversal does not model. It is an open
  question how a caller such as CalmHub tells the calm-server which rules to run, and how differing
  rule sets across a large organisation are catered for (see
  Validate uploaded documents in CalmHub via CALM server sidecar #2716 and Questions on CALM Validations #2566).
• Typed CALM document input. validate() currently takes four loosely-typed positional inputs and
  infers a mode; a single typed CALM document from calm-models (Standardize CALM document type list in calm-models #2770)
  would be cleaner.
• CLI versus CalmHub. The CLI is user-driven and would default to recursive (deep) traversal;
  CalmHub validates on upload and would decide when a deep run is performed.

[rocketstack-matt]

Solid refactor — the phased ValidationRule/ValidationEngine design is clean, and the original self-reference heap-OOM (flagged on #2778) is genuinely fixed with a regression test I confirmed passes. But I found and reproduced a real correctness bug in the caching/cycle-tracking, not one of the open questions this PR calls out — see inline comments.

[rocketstack-matt]

resolve() only gates re-fetching via cache.has(), not seen. Since a failed delegate.resolve() never populates cache, a later resolve() call for the same reference retries the delegate — contradicting this class's own docstring ("a failed load still counts as seen ... not retried") and the PR description's claim of the same. The test titled "marks a reference as seen even when the load fails (no retry for siblings)" doesn't actually call resolve() a second time, so it never verifies that claim.

For a real (e.g. HTTP-backed) resolver, a broken reference reachable from many nodes gets fetched repeatedly instead of once. Suggest tracking failures explicitly (e.g. cache the rejection, or check seen before retrying) so a second resolve() on a known-bad reference short-circuits instead of re-attempting the delegate.

[LeighFinegold]

Reproduced. Added a failing test caching-tracking-resolver.spec.ts -> "does not retry the delegate for a reference whose first load failed": a second resolve() on a failed reference attempts the delegate again (asserted once, got twice). Left red for now; see my top-level note on the intended semantics before I fix it.

[rocketstack-matt]

This conflates "already resolved" with "cycle." I reproduced this: two sibling nodes referencing the same non-existent detailed-architecture URL produce a node-details-validation error for the first node only — the second node's identical (failed) reference is silently skipped as emptyNodeResult() (logged only at debug as "Cycle detected"), even though it isn't a cycle at all, just an unrelated node sharing a broken reference. A real, reportable validation error goes completely missing for every occurrence after the first.

This has the same root cause as the caching-tracking-resolver.ts comment — references.has() doesn't distinguish "previously succeeded" (safe to dedupe silently) from "previously failed" (should still surface an error at this node's path).

[LeighFinegold]

Reproduced. Added a failing test validate-node-details.spec.ts -> "reports a load error for every node sharing the same broken detailed-architecture reference": two sibling nodes pointing at the same broken URL yield a single error (asserted 2, got 1), the second being skipped as "Cycle detected". Left red for now; see my top-level note on the intended semantics before I fix it.

[LeighFinegold]

Thanks @rocketstack-matt. Both issues are real and I have reproduced each with a failing test on the branch (left red for now, before we agree the fix). Replies in-thread on both comments above.

As you noted, both share one root cause: seen/has() records that a reference was attempted but does not distinguish previously succeeded (safe to dedupe, or a genuine cycle) from previously failed (should still surface an error), and resolve() gates only on the success cache.

Before I implement, I would like to agree the intended semantics so the fix matches what we want rather than just what makes the tests green:

1. A detailed-architecture reference that fails to load and is referenced by multiple nodes: should we re-emit the load error at every referencing node's path (from a cached failure, so the delegate is not re-fetched), or report it once?
2. Should we now separate true cycle detection (an ancestor in the current traversal path) from sibling dedupe (a reference already resolved elsewhere), or is distinguishing success from failure sufficient for this PR, leaving the fuller "one first-class cycle-safe walk" as the follow-up this PR already calls out?
3. A sub-architecture that loads successfully but is itself invalid, referenced by two nodes: keep the current validate-once behaviour (its errors reported once), or report per referencing node?

Happy to go with your steer; the tests are in place to lock in whichever behaviour we choose.

[willosborne]

@LeighFinegold my take on these questions:

1. imo, if a user needs to fix something in their architecture, it's much better to report it in every place at once rather than making them fix>re-run>fix again>re-run>fix again>re-run, etc. Could be quite frustrating.
2. IMO, given this is already a big refactor, we should keep it simple and implement other gaps as follow-ups. cycle detection is nice, but as long as a cycle doesn't blow up/hang the process we are fine.
3. this could get very, very noisy, if the architecture has many faults and is referenced many times. IMO this should only be output once - but if this is extra work given my answer to 1) we should leave it outputting it each time and fix later, in order to get this PR merged sooner rather than later.

This is just my 2c from reading the comment. If any of these answers will result in a significant amount of work before we can even consider this mergeable, we should do them as a follow up. The main thing is that this PR doesn't introduce potential infinite loops/process hangs, etc.

[LeighFinegold]

Thanks @willosborne, that matches where I landed. Summary of the response:

Point 1 (report at every node): fixed in this PR. A detailed-architecture reference that fails
to load is now re-reported at every node that references it, instead of being silently dropped after
the first occurrence as a "cycle". This shared a root cause with the resolver comment @rocketstack-matt
raised: the tracking recorded that a reference had been attempted but not whether it succeeded or
failed. The resolver now caches the failure, so a repeat resolve() re-throws the cached error
without re-fetching, and node-details only skips references that previously succeeded.

Points 2 and 3: raised as separate issues so this PR stays mergeable:

• Validation: first-class cycle detection for recursive detailed-architecture traversal #2821: first-class cycle detection, separating a genuine cycle (an ancestor in the traversal path)
  from silent sibling de-duplication.
• Validation: report a loaded-but-invalid shared sub-architecture's errors once #2822: report a loaded-but-invalid shared sub-architecture's own errors once, while keeping the
  per-node reporting for failed loads (the asymmetry from points 1 and 3).

On the size of the change: yes, it is a large refactor, but it deliberately keeps the existing
contract the same (although even that could be reviewed). The ValidationOutcome shape (output buckets, ordering, error/warning flags) is
unchanged, so it is behaviour-preserving for consumers apart from the two bug fixes above. The only
requirement your point 2 flagged, that a cycle must not hang or OOM the process, holds: failed
references never recurse and successful references are cache-guarded.

Both previously-failing tests are now green as tweaked assertions

[LeighFinegold]

Heads up: after landing the point 1 fix and rebasing onto latest main, an additional thing broke. The one remaining red check is CLI ↔ CalmHub smoke (every other check is green). It is deterministic rather than flaky, and it is worth clarifying.

Two distinct causes, both in the new control-requirement validation (validate-controls.ts, which does not exist on main):

1. The redesign now loads a control's config-url and validates it against the requirement schema. That surfaces a genuine inconsistency in the FINOS getting-started examples that the smoke fixture (added in CALMHub/CLI smoke tests #2785) relies on: permitted-connection.requirement.json pins control-id to const: "security-002", but permitted-connection-jdbc.config.json declares security-003. Old main never validated this, so the smoke test passed. The new behaviour arguably catches a real defect, but it does break the fixture.

2. When a hub is passed (-c), external https://calm.finos.org/...requirement.json URLs are routed through the CalmHub document loader (which only serves that hub) and fail to load. Previously an external HTTPS URL resolved directly over HTTP even with a hub configured.

Proposal. Before changing anything, I would like to add a dedicated control + detailed-architecture e2e test to pin the intended behaviour, then decide per cause: for (1) whether we keep the stricter validation and correct the example data / smoke fixture, or make the control-id const mismatch non-fatal for now; for (2) restore direct HTTPS resolution so external requirement URLs are not forced through the hub loader. Happy to take a steer on both directions.

[LeighFinegold]

Follow-up to the above: that dedicated control + detailed-architecture test is now in.

As discussed with @willosborne, we're making control validation stricter now and will see what breaks internally.

The smoke coverage: a compliant control validates clean (it carries a hub hosted detailed architecture, so the node-details descent is exercised), and a control whose control-id breaks its requirement is reported. Both run against control docs pushed to CalmHub.

One caveat: the bad case only works because CalmHub stores control docs as is today. If we add a sidecar that validates on upload, the hub push of the bad config will be rejected and this test will break at the push step, so it will need a rethink then. Separate from the loader routing question in #2827.

Matt's suggestions have all been either applied or raised as issues for future implementation. He's not available today so I'm dismissing this so we can get this merged; can always fix as follow-up.

[willosborne]

I think all Matt's suggestions are now either implemented or covered in follow ups - #2821 #2822 - so this should be merged