Skip to content

Slice 5/5: feat(calm-hub): GitHub-backed read-only storage for CALM resources - #3066

Draft
jpgough-ms wants to merge 38 commits into
mainfrom
slice/3001-s5-github-storage-backend
Draft

jpgough-ms wants to merge 38 commits into
mainfrom
slice/3001-s5-github-storage-backend

Conversation

@jpgough-ms

@jpgough-ms jpgough-ms commented Sep 8, 2026

Copy link
Copy Markdown
Member

Description

Structural rework since the original split (Office Hours, 2026-09-10, #3052)

  • Removed "Building Block." New CALM concept, introduced without agreement — Office Hours: model it as Standard instead. building-blocks/ now aliases to STANDARD in both JSON and markdown classifiers; existing repo layouts and the VSCode plugin's glob keep working. Behaviour change: .md files under building-blocks/ were previously dropped entirely (no markdown classifier case existed) — now indexed as standards, merged into the same listing as standards/
  • Removed GUIDELINE — same problem as Building Block. Nothing served guidelines/ files, so no regression
  • Reverted CONTROL from the namespace-scoped front controller. Controls are domain-scoped (ControlResource is @Path("/api/calm/domains"), every ControlStore method takes domain first); the front controller is namespace-scoped and was passing a namespace into the domain parameter — broken in every storage mode:
    • GitHub mode returned HTTP 500: DomainNotFoundException wasn't in the front controller's catch list. Hidden by test fixtures using the same string for namespace and domain. No test exercised /calm/namespaces/{ns}/controls/... before this fix
    • Mongo/nitrite can't represent it: createMapping requires a namespace; a control is keyed by (domain, controlId), no namespace
    • Domain-scoped /api/calm/domains/{domain}/controls/... (fully implemented) is untouched. Proposing a tracking issue for namespace-addressable controls as its own design — cost: domain-keyed mapping rows, a schema migration in both backends, a backfill
  • Removed latest from the shared, backend-agnostic version contract. Reverses a prior "no concept of latest" decision. GET .../versions/latest now 400s in every backend. GitHub mode: an unresolvable/non-SHA version 404s via the resource's *VersionNotFoundException, never a silent HEAD read. Version lists contain only real SHAs, or the clone's current HEAD SHA when the API has nothing — assembled once in a shared base store, not duplicated per resource
  • Dissolved store/github/util entirely (Office Hours - 2026-09-10 #3052 — ~15 runtime CDI beans misfiled as "util," ~20 package-private fields/methods existing only as test seams). Moved into store.github.{config,sync,registry,api,access} by responsibility; every store now full constructor injection (final fields, no field pokes); house-standard JavaDoc added throughout. CalmResourceType renamed to RegistryResourceType, kept deliberately separate from domain.ResourceType (different questions: file classification vs. front-controller addressability; ResourceType is serialized/name-coupled elsewhere); UNKNOWN dropped for Optional<RegistryResourceType>
  • Extracted AbstractGitHubStore / AbstractReadOnlyGitHubStore (closes the structural half of calm-hub: extract a shared base for the read-only GitHub stores #3079) — verifyNamespace, id lookup, version listing, and the SHA-vs-local-file read block were duplicated across 9 stores and already drifting (GitHubPatternStore had inlined two error strings its siblings had hoisted into constants)
  • CUSTOM_ID_REGEX now allows uppercase (^[a-z]...^[a-zA-Z]...) — pre-existing change from the original split, flagged here as the public API change it is: custom IDs are case-sensitive in every storage mode
  • Resolved from the original split's open findings: store/github/util duplication and missing shared base class; the 10 near-identical GitHub stores with no shared base
  • Still open (tracked as issues below): no readiness gate during initial clone; uniqueId().hashCode() & 0x7FFFFFFF + .findFirst() has no collision detection; PatternLayoutStoreProducer has no GitHub branch; GitHubForkPRService.createPullRequest always throws; no docs/ADR for the storage backend

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🔧 Refactoring (no functional changes)

Affected Components

  • CALM Hub (calm-hub/)
  • CALM Hub UI (calm-hub-ui/)
  • Dependencies

Testing

  • I have tested my changes locally
  • I have added/updated unit tests
  • All existing tests pass — calm-hub: 3,380 unit tests + 554 Docker integration tests (5 new, covering this rework's behaviour changes end to end against a real local clone), 0 failures, JaCoCo 90%-per-class gate met; UI suite unchanged by this rework

Checklist

  • My commits follow the conventional commit format
  • I have added tests for my changes (if applicable)
  • My changes follow the project's coding standards

Split from #3001. Depends on #3065.

@github-actions github-actions Bot added calm-hub Affects `calm-hub` calm-hub-ui Affects `calm-hub-ui` labels Sep 8, 2026
@jpgough-ms jpgough-ms changed the title feat(calm-hub): GitHub-backed read-only storage for CALM resources Slice 5/5: feat(calm-hub): GitHub-backed read-only storage for CALM resources Sep 8, 2026
name, repo, branch, accessGroups);
}

executor.runAsync(this::cloneAndRebuild);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class javadoc says "health endpoints are available during clone," but there's no @Readiness/HealthCheck anywhere in calm-hub and no smallrye-health dependency in the pom — so that's describing an endpoint that doesn't exist. Meanwhile InMemoryRegistryService starts empty and only gets rebuilt once cloneAndRebuild finishes (line 76), so any request for a validly-configured namespace during the clone window hits verifyNamespace against an empty namespace list and gets a 404 "namespace does not exist" rather than a 503 "still starting up." Worth adding a real readiness gate here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the javadoc — it previously claimed a @Readiness/health-check endpoint existed here; there is no smallrye-health dependency in this module, so that was describing something that doesn't exist. It now documents the actual behavior honestly (404 during the clone window, GitHubCloneManager.getState() as the real poll signal) rather than papering over it.

The actual readiness gate (503 while cloning, ahead of verifyNamespace) isn't implemented yet — tracked in #3093, which also folds in a second ordering gap found while building this PR's end-to-end integration test: getState() flips to READY before the registry is actually rebuilt, so a gate on clone state alone has the same hole. Leaving this thread open pending that issue.

private RegistryEntry findEntryById(String namespace, int id) throws ArchitectureNotFoundException {
List<RegistryEntry> entries = registryService.listByType(namespace, CalmResourceType.ARCHITECTURE);
Optional<RegistryEntry> found = entries.stream()
.filter(e -> (e.uniqueId().hashCode() & 0x7FFFFFFF) == id)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uniqueId().hashCode() & 0x7FFFFFFF used as the public numeric ID, looked up here via .filter(...).findFirst(). This is the same pattern repeated across ~21 sites in the GitHub stores (Architecture, BuildingBlock, Control, Interface, Standard, Flow, Pattern, Search, ResourceMapping). A 31-bit hash collision between two resources in the same namespace would silently return the wrong document — findFirst() has no way to detect or report the collision. Worth a shared ID scheme with actual collision handling rather than each store rolling its own.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and not something to fix piecemeal at this one site — filed #3080 to cover all ~21 sites with a shared ID scheme that can actually detect a collision, rather than the current per-store .filter(...).findFirst(). Leaving this thread open pending that issue.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR now extracts the shared read-path base (AbstractReadOnlyGitHubStore), which makes findEntry single-site instead of duplicated per store — so this stays reachable as one small change rather than 9 separate ones. Scoping #3079 to cover the collision-safe ID work alone (reject vs. log is a behaviour decision, not a mechanical extraction) rather than closing it here. Leaving this thread open, pointed at #3079.

Comment thread calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubForkPRService.java Outdated
@jpgough-ms

Copy link
Copy Markdown
Member Author

One more finding that doesn't have a line to attach to, since the file isn't touched by this PR's diff:

calm-hub/src/main/java/org/finos/calm/store/producer/PatternLayoutStoreProducer.java has no github branch — only standalone (nitrite) and a mongo fallback (lines 20-40). In calm.database.mode=github, GET/PUT /patterns/{id}/layout resolves to MongoPatternLayoutStore with no MongoDB configured, which will fail at request time rather than returning a clean "not supported" response the way the other GitHub stores do. The architecture equivalent, LayoutStoreProducer (touched by this PR), does have a github branch — this one looks like it was missed when the GitHub branches were added across the producers.

Comment thread calm-hub/src/main/java/org/finos/calm/store/github/GitHubAdrStore.java Outdated
@jpgough-ms

Copy link
Copy Markdown
Member Author

No docs or ADR changes accompany a new storage backend and its config surface (calm.github.* in application-oidc.properties — clone directory, sync interval, service token, namespace registration format name|repo|branch[|accessGroups]). Worth an ADR before this merges, given the design decisions embedded here (in-memory registry rebuilt on a schedule, hash-based ids, read-only-for-now scope).

@jpgough-ms
jpgough-ms marked this pull request as draft September 8, 2026 10:26
@rocketstack-matt
rocketstack-matt force-pushed the slice/3001-s5-github-storage-backend branch from 58e4d0b to a4e60ba Compare September 8, 2026 16:48
@jpgough-ms
jpgough-ms force-pushed the slice/3001-s5-github-storage-backend branch from a4e60ba to 3394ba0 Compare September 9, 2026 08:27
@rocketstack-matt
rocketstack-matt force-pushed the slice/3001-s5-github-storage-backend branch from 3394ba0 to bdf9831 Compare September 9, 2026 08:57
@jpgough-ms
jpgough-ms force-pushed the slice/3001-s5-github-storage-backend branch from bdf9831 to e9d23d4 Compare September 9, 2026 10:57
@jpgough-ms
jpgough-ms force-pushed the slice/3001-s5-github-storage-backend branch from e9d23d4 to fa6ac8f Compare September 9, 2026 11:22
…l package

Fifth move of the store/github/util dissolution. GitHubApiResponseCache
was already well-shaped - two constructors, one @Inject with a config
value, one package-private for tests to drive expiry deterministically
with a fake Ticker. Only change beyond the move: the test constructor
becomes public, since package-private visibility stops being a usable
seam once the test lives in a different package from the production
class.

Moves to store.github.api. GitHubVersionService (not yet moved itself)
picks up the new import.

Full verify (3344 tests, 0 failures) and JaCoCo coverage checks pass.
… as GitHubFileHistoryClient

Sixth move of the store/github/util dissolution, and where the
`latest` sentinel's outbound leak actually gets fixed - see the
maintainer's decision on PR #3066 to remove it from the public
contract entirely.

- Rename to GitHubFileHistoryClient ("Service" said nothing about what
  it does; it's a REST client for one specific API). Constructor
  injection for all five collaborators (cache, storeConfig, three
  @ConfigProperty timeouts/limits), building the shared HttpClient in
  the constructor body instead of a separate @PostConstruct step - the
  only reason that split existed was that a field initializer would
  have read connectTimeoutSeconds before @ConfigProperty injection
  populated it; constructor injection has no such ordering gap.
- getFileVersions returns List.of() on failure (empty commits
  response, non-200, malformed JSON, connection failure) instead of
  List.of("latest"). This class has no namespace, so it cannot
  meaningfully fall back to a local clone's HEAD SHA - that assembly
  happens one layer up, in the shared store base still to come, which
  has both the namespace and the clone manager.
- extractShas/extractNextLink go from package-private test seams to
  private - every branch they have is reachable from the public
  getFileVersions/getFileAtVersion surface. The rewritten test hits a
  real local HTTP server throughout rather than reaching into
  internals, and adds two tests the old package-private-access version
  never had: that a blank service token sends no Authorization header,
  and that a configured one sends the expected Bearer value - both now
  verified against actual request headers instead of asserted only
  against the token field's getter.

The 7 stores field-injecting this class (Architecture, Control, Flow,
Interface, Pattern, Standard, Timeline) pick up the renamed type; the
store-level `List.of("latest")` fallback in each is unchanged here -
it's guarded by `cloneManager != null`/`versionService != null` checks
that constructor injection removes in the shared-base sweep, which is
where that fallback goes too.

Full verify (3343 tests, 0 failures) and JaCoCo coverage checks pass.
…ckage

Seventh move of the store/github/util dissolution. GitHubCloneManager
was already constructor-injected and clean; this move is mostly
mechanical, plus three fixes that belong with it:

- Corrects the class javadoc, which claimed "parallel cloning" - cloneAll
  and pullAll are plain sequential for loops. Points at the tracking
  issue for actually parallelising them instead of leaving the false
  claim in place.
- Adds headSha(String namespace), delegating to GitHubRepoSync.headSha
  on the namespace's own clone directory. This is what the shared store
  base (still to come) will use to give getFileVersions an honest
  fallback when the commits API has nothing - see GitHubFileHistoryClient's
  javadoc for why that fallback lives one layer up from the API client.
- Deletes the 3-arg registerNamespace(name, repo, branch) overload -
  production code (GitHubStartupInitializer) only ever called the 4-arg
  form with access groups; the 3-arg form existed only for tests, which
  now call the 4-arg form with Set.of() instead.

Moves to store.github.sync (alongside GitHubRepoSync, already there).
NamespaceAccessFilter, GitHubSyncScheduler, and GitHubStartupInitializer
(not yet moved themselves) pick up the new import; 9 stores field-injecting
this class pick up the renamed package.

Full verify (3344 tests, 0 failures) and JaCoCo coverage checks pass.
… package

Eighth move of the store/github/util dissolution. Constructor injection
for all five collaborators, replacing package-private @Inject fields
tests wrote to directly. Adds @LookupIfProperty(github) - it previously
instantiated in every storage mode despite only being consumed by
GitHubControlStore and GitHubDomainStore, both GitHub-only.

Verified against calm.database.mode=github specifically because this
bean now constructor-injects SecurityIdentity, a request-scoped bean,
into an @ApplicationScoped one - the full verify (unit + integration,
Docker) confirms CDI still resolves it correctly under the real
GitHub-mode profile, not just the mongo default.

Moves to store.github.access (alongside NamespaceFileReader, already
there). GitHubControlStore and GitHubDomainStore pick up the new
import.

Full verify (3344 unit + 549 with integration, 0 failures) and JaCoCo
coverage checks pass.
Ninth and tenth moves - the last two classes in store/github/util,
which no longer exists after this commit. Both get constructor
injection for every collaborator except the framework-mandated
@Scheduled/@observes callback parameters themselves (sync(),
onStart(@observes StartupEvent ev)) - CDI requires those exact method
shapes, so they're the one deliberate exception to "no package-private
members," per the checklist the rest of this dissolution has followed.

GitHubSyncScheduler's misleading SKIP comment is corrected: it said
concurrentExecution = SKIP protects a request thread reading a file
mid-`reset --hard`. It doesn't - SKIP only prevents this method's own
tick from overlapping its next tick. The corrected comment names both
open races (a request thread racing a reset, and this scheduler racing
GitHubStartupInitializer's own rebuild during the initial clone
window) and points at the tracking issue for GitHub clone lifecycle
coordination.

Both moved test files construct their subject through the constructor
now instead of building a no-arg instance and poking package-private
fields afterward.

Full verify with integration tests (Docker), confirming the real
calm.database.mode=github CDI boot end to end, not just the mongo
default: 3344 unit + 549 with integration, 0 failures, JaCoCo coverage
checks pass.
…tor injection

- Add AbstractGitHubStore (verifyNamespace, WRITE_UNSUPPORTED) and
  AbstractReadOnlyGitHubStore (findEntry, getVersions, readAtVersion) to remove
  the copy-pasted read block that existed across 9 GitHub stores (#3079)
- The shared read block only ever treats a version as a real, resolvable SHA:
  an unrecognised version now 404s via the store's *VersionNotFoundException
  instead of silently falling back to whatever the local clone's HEAD holds -
  closes the last "latest with the name removed" gap left after Phase 3/4
- Version lists fall back to the local clone's HEAD SHA only when the GitHub
  API returns nothing, assembled once in the base store rather than 8 separate
  copies
- Migrate all 14 GitHub*Store classes with any @Inject field to full
  constructor injection (Architecture, Flow, Interface, Pattern, Standard,
  Timeline, Adr, Decorator, Control, Domain, Search, UserAccess); delete the
  now-unreachable cloneManager/versionService/accessFilter null guards
- GitHubStandardStore keeps its markdown-sibling preference via a 4-arg
  readAtVersion overload, scoped so the override never changes what a pinned
  SHA fetch targets - only the local-HEAD optimisation
- GitHubControlStore documents why it doesn't extend the shared bases (it
  looks up by domain, not namespace+type+id) while still following the same
  SHA-or-404 read rule
- Add package-private no-arg constructors to both abstract base classes so
  Arc can generate CDI client proxies for the @ApplicationScoped subclasses;
  documented as proxy-only, never a real collaborator path
- Add TestAbstractReadOnlyGitHubStoreShould and rewrite every affected store
  test for constructor-injected mocks, split "latest" version-list assertions
  into empty/head-SHA-fallback cases, and add local-file-missing coverage

Verified: full unit suite (3380 tests) and Docker-based integration suite
(549 tests) pass; JaCoCo 90%-per-class gate met.
…ore classes

- Add class-level JavaDoc to the 11 GitHub*Store classes and 2 registry
  records left undocumented after Phase 4/5 (Adr, Architecture, Decorator,
  Flow, Interface, Namespace, Pattern, ResourceMapping, Search, Standard,
  Timeline, RegistryEntry, RegistrySnapshot)
- Each explains why the class exists and what it deliberately does not do,
  matching the store/util house standard (CanonicalVersion, VersionScheme):
  why Adr/Decorator have no registry representation, why
  GitHubResourceMappingStore is the one named place translating between
  ResourceType and RegistryResourceType, why Standard's markdown-sibling
  preference only applies to the local-HEAD read
- No behaviour change; confirmed via a full mvn verify (3380 tests, JaCoCo
  90%-per-class gate met)
…ate test seam

- Remove blanket @MockitoSettings(LENIENT) from the 7 GitHub store tests that
  don't actually have any unused stubbing (Architecture, Control, Flow,
  Interface, Pattern, Standard, Timeline) - verified by running each without
  it first. Keep it only on Search and UserAccess, which genuinely stub
  shared setup used by a subset of their tests
- Replace the CDI-proxy-constructor coverage test's four
  assertThat(...,nullValue()) assertions on package-private base-class
  fields with a single "constructs without throwing" check - reaching into
  those fields from a test was exactly the seam pattern Phase 4 removed
  everywhere else; the constructor executing is the whole contract this
  test needs to cover
…s five behaviours

- Extend GitHubFixtureResource's finos-repo with a building-blocks/ file
  alongside its existing control fixture
- Add GitHubReworkBehaviorIntegration covering, against a real local clone
  (no mocking): building-blocks/ classifies as a Standard; a version list
  never contains "latest" and is a real SHA; GET .../versions/latest 400s in
  GitHub mode same as every other backend; the namespace-scoped controls
  route 400s rather than 500ing
- Extend GitHubUserAccessDomainReadIntegration with a control-content read
  through the domain route, namespace deliberately distinct from domain

Verified: full Docker integration suite passes (554 tests, was 549).
}

@Test
void ignore_md_sibling_preference_when_fetching_a_pinned_sha_from_the_api(@TempDir Path tempDir) throws Exception {
// Package-private, taking the CDI-mandated StartupEvent parameter: this is a
// framework contract method, not a test seam - @Observes requires it callable by
// the container with exactly this signature.
void onStart(@Observes StartupEvent ev) {
NamespaceFileReader.existsContained called Files.exists(target) on an
unvalidated path before the containment check. isContained's own
toRealPath() resolution already fails for a path that doesn't exist, so
containment implies existence — drop the separate exists() call.
…ad-failure branches

CI's `clean verify` measured GitHubControlStore at 89% line coverage,
below the 90% gate — my local runs never did a full clean between test
runs this session, so stale jacoco.exec data masked the real number.

Add tests for the two uncovered branches: findControlEntry and
findNamespaceForControl disagreeing mid-registry-rebuild (via consecutive
Mockito stubbing), and the local-HEAD read failing with IOException when
the file is missing on disk. GitHubControlStore is now at 100% line
coverage.
@jpgough-ms

Copy link
Copy Markdown
Member Author

In the discussion on office hours we've agreed that building-blocks should be modelled with the existing calm type Standard

Hello @jpgough-ms , @markscott-ms & @rocketstack-matt ,

Upon further reflection we are leaning to using /packs to create a company specific pack that would contain our controls or any one else customized nodes as an extensional model for future enterprises. The node vs. standard caused us challenges because the standard is a schema. Would us shifting from building-blocks to packs will suffice?

Thank You.

@byrash,

I think that we need to take this into an issue and then into a discussion on office hours. To make it clear, any new introduction of core concepts, needs to be more than footnote in a PR.

For new ideas to be accepted into open source projects, you need to convince the community that it makes sense to add this, what the use case is, and why it is not supported by anything in CALM at this time. At this point packs, building blocks are new words without being tied into the CALM domain/terminology. The former was covered by standards.

The other concept in CALM Template to consider is bundles not supported by calm-hub at this time, but have been used in description/discussion for code and template generation for a while. I'd want to try compare the two things before introduction.

I'm going to continue to try get the PR in a mergeable shape, but there's a wide array of outstanding issues to get the core of this working.

cc/ @markscott-ms , @rocketstack-matt

@rocketstack-matt

Copy link
Copy Markdown
Member

@byrash perhaps you can explain why standards being a schema is a problem? After all the whole core of CALM is a schema.

To @jpgough-ms's point, we need to make sure that what you need to support your organization makes sense for the whole community and is done in a way that doesn't duplicate functionality that may already exist, where there is something similar, but not quite sufficient we should prefer to extend rather than replace.

Thanks.

…bControlStore

- Route domain derivation through ControlDomains.extractDomain instead of an
  inline substring match. The substring check matched "controls/" at any
  path depth; extractDomain only recognises it as the first segment. A
  control nested below root (foo/controls/security/x.json) resolved to
  domain "security" via the substring check but "default" via
  extractDomain - GitHubUserAccessStore (which already uses
  extractDomain) could grant or deny access based on a domain the store's
  own lookups disagreed with
- Fix getConfigurationsForControl/getConfigurationDetailsForControl/
  getConfigurationVersions/getConfigurationForVersion: these are GET-backed
  reads with no registry representation (same shape as GitHubAdrStore),
  but threw GitHubWriteNotSupportedException - a misleading 501 for a
  request that was never a write. They now validate the control exists via
  findControlEntry and answer "none exist" (empty list, or
  ControlConfigurationNotFoundException for a specific id), matching every
  other read-only GitHub store's pattern

Found by /code-review high.
…tion path

NamespaceAccessFilter re-derived readable namespaces from OIDC role/group
membership directly, independently of UserAccessValidator - the same
resolution SearchResource/DomainResource use. That second implementation
silently diverged on two counts: calm.auth.allow-public-read was never
consulted (public-read deployments still got narrowed to the caller's own
OIDC groups for domain/control visibility), and UserAccessValidator's
ancestor-chain AND rule wasn't applied.

Delegate to UserAccessValidator.getReadableNamespaces instead. It already
calls into UserAccessStore.getGrantsForUser, which in GitHub mode is
GitHubUserAccessStore - the OIDC role resolution still happens, just in one
place instead of two.

Found by /code-review high.
…h not the local one

- Neither cloneRepo nor pullRepo set a JGit transport timeout. Combined
  with GitHubSyncScheduler's concurrentExecution = SKIP, a hung/unresponsive
  remote blocked one sync tick indefinitely and then blocked every future
  tick from ever starting - SKIP only prevents tick-vs-tick overlap, it
  doesn't time out a stuck execution. Add calm.github.git.timeout-seconds
  (default 30s) and apply it to both commands
- pullRepo reset to origin/<whatever branch is currently checked out>
  instead of the configured branch. If an operator changes a namespace's
  branch without wiping its clone directory, cloneAll()/pullAll() still see
  a valid repo and resync via pullRepo instead of a fresh clone - resetting
  to the stale local branch left the working tree permanently pointed at
  the old branch while GitHubFileHistoryClient queried version history
  against the newly configured one. pullRepo now takes the configured
  branch explicitly and resets to that

Found by /code-review high.
…s to pull

sync() recorded a success metric whenever pullAll() didn't throw, but
pullAll() never throws - every per-repo git failure is caught inside
GitHubRepoSync and folded into GitHubCloneManager's state instead. An
unreachable GitHub, or an expired token, reported as a healthy sync for
every namespace, hiding a total outage from monitoring. Check the clone
state pullAll() actually left behind and record failure for FAILED or
DEGRADED.

Found by /code-review high.
Every sibling *StoreProducer this rework touched got a GitHub branch;
PatternLayoutStoreProducer was missed. In calm.database.mode=github, a
pattern-layout request fell through to MongoPatternLayoutStore, which is
itself @LookupIfProperty(stringValue = "mongo")-gated and so doesn't exist
as a bean in GitHub mode - every request resolved to an unsatisfied
injection and failed with a 500.

Add GitHubPatternLayoutStore, mirroring GitHubLayoutStore's empty/no-op
pattern (layouts are client-side state in GitHub mode), and wire it into
the producer alongside the DatabaseMode constant rename the rest of this
producer's siblings already had.

Found by /code-review high.
The qualified-id index is keyed only by namespace + uniqueId. A uniqueId
without an explicit "unique-id" field falls back to the filename minus
extension, so two files with the same basename in different subdirectories
(e.g. patterns/a/foo.json and patterns/b/foo.json) collide - rebuild()
silently overwrote one with the other, and since Files.walk's iteration
order is unspecified, which one survived could change between rebuilds of
otherwise-unchanged content.

listByType is unaffected (it reads the per-namespace list, not the
qualified-id index), but a direct findByUniqueId lookup - and anything
built on it, e.g. resource-mapping lookups - could only ever resolve to
one of the two. This doesn't resolve the underlying collision (that's the
scoped-out ID-collision-detection work tracked on #3079); it makes the
outcome deterministic (entries sorted by path before indexing) and logs
the collision so a colliding repo layout is diagnosable rather than a
silent "wrong document served" surprise.

Found by /code-review high.
The merged, all-types stream was capped at MAX_RESULTS_PER_TYPE * 7 before
being split by type - a namespace with many matching architectures could
exhaust that cap before entries of other types were ever reached in the
stream, so a real pattern/flow/standard match came back empty even though
it existed. filterByType already caps each type independently, so the
premature combined cap was redundant as well as harmful; the registry is
an in-memory index, not an external call, so there's no cost reason to cut
the merge short. Removed.

Found by /code-review high.
addNewVersion() had no explicit GitHubWriteNotSupportedException catch
before its broad catch(Exception), unlike its sibling create/update paths.
Adding a version to an existing resource in GitHub mode threw
GitHubWriteNotSupportedException, which the broad catch swallowed into a
misleading 400 "Failed to update resource" instead of the 501 every other
GitHub-mode write path correctly returns.

Found by /code-review high.
@byrash

byrash commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

@byrash perhaps you can explain why standards being a schema is a problem? After all the whole core of CALM is a schema.

To @jpgough-ms's point, we need to make sure that what you need to support your organization makes sense for the whole community and is done in a way that doesn't duplicate functionality that may already exist, where there is something similar, but not quite sufficient we should prefer to extend rather than replace.

Thanks.

Thanks @jpgough-ms and @rocketstack-matt — agreed this deserves a proper issue and office hours discussion. We'll open one. Providing our context here as well for continiuity,

To @rocketstack-matt's question — we've reviewed the Standards tutorial and understand that Standards extend core CALM definitions with organizational requirements (e.g., requiring costCenter, owner on all nodes via allOf composition). We also see that Pattern + Standard layered governance covers "what nodes must exist" + "what properties they must have."

Standards won't work in our scenario because we really need custom nodes that don't exist today — apologies we couldn't share more, though. Looking through the CALM Hub Interfaces (https://calm.finos.org/calm-hub/calm-hub-developer-guide/#the-store-interfaces), our main goal was to distribute a private, concrete set of nodes to all of our internal consumers via CALM Hub. This allows us to avoid continuous VS Code plugin updates with new private nodes.

After some review of the documentation,we feel the best way forward is to introduce a NodeStore alongside Controls, ADRs, etc. in CALM Hub. This will give us centralized management of custom nodes.

@byrash

byrash commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Hello @jpgough-ms @rocketstack-matt , Create this feature request for NodeStore, Happy to have a discussion in our next WG session for consensus.

#3102

Thank You.

@rocketstack-matt

rocketstack-matt commented Sep 11, 2026

Copy link
Copy Markdown
Member

@byrash re: "Standards won't work in our scenario because we really need custom nodes that don't exist today" — I don't think that's accurate, and I want to walk through why before we go further down the NodeStore route.

node-type is already free-form in the core schema, not a closed enum: core.json#L184-L203 defines it as anyOf: [enum(9 well-known values), string]. You can write "node-type": "feature-store" today — nothing in CALM needs to change for that.

What you're actually describing — a shared, enforced definition of a custom node type across your internal consumers — is what a Standard gives you. A Standard is a schema because a node type is a contract (required properties, shape), not because it replaces the node. The node instance still lives in the architecture document.

Two examples:

  • feature-store node: Standard = allOf [core node, {node-type: const "feature-store", retention-days: required, owning-team: required}]. Instance: a normal node with "node-type": "feature-store" plus those two properties.
  • kafka-topic node: same pattern, gating on partition-count / retention-policy.

A Pattern then $refs the Standard via items (not prefixItems), so calm validate enforces the shape wherever that node type appears, without dictating what else exists in the architecture. This is the same mechanism already used for decorator standards — see deployment.decorator.standard.json, which gates on "type": {"const": "deployment"}.

On distribution: CALM Hub already has this — StandardStore plus its resource/store implementations. Publishing a new custom node type is publishing a new Standard document to the Hub — no VS Code plugin redeploy, since it's a hosted schema, not code baked into the client. That's the exact distribution mechanism you asked for.

One gap: Standards define types and Patterns mandate their use — neither is a browsable catalogue of pre-built node snippets a user picks from. If what your team actually needs is that catalogue rather than a schema contract, that's closer to what template-bundles/ already does, and worth comparing before we introduce a new NodeStore concept. Happy to take that as its own issue if the schema-contract approach above doesn't cover your case.

@byrash

byrash commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

@byrash re: "Standards won't work in our scenario because we really need custom nodes that don't exist today" — I don't think that's accurate, and I want to walk through why before we go further down the NodeStore route.

node-type is already free-form in the core schema, not a closed enum: core.json#L184-L203 defines it as anyOf: [enum(9 well-known values), string]. You can write "node-type": "feature-store" today — nothing in CALM needs to change for that.

What you're actually describing — a shared, enforced definition of a custom node type across your internal consumers — is what a Standard gives you. A Standard is a schema because a node type is a contract (required properties, shape), not because it replaces the node. The node instance still lives in the architecture document.

Two examples:

  • feature-store node: Standard = allOf [core node, {node-type: const "feature-store", retention-days: required, owning-team: required}]. Instance: a normal node with "node-type": "feature-store" plus those two properties.
  • kafka-topic node: same pattern, gating on partition-count / retention-policy.

A Pattern then $refs the Standard via items (not prefixItems), so calm validate enforces the shape wherever that node type appears, without dictating what else exists in the architecture. This is the same mechanism already used for decorator standards — see deployment.decorator.standard.json, which gates on "type": {"const": "deployment"}.

On distribution: CALM Hub already has this — StandardStore plus its resource/store implementations. Publishing a new custom node type is publishing a new Standard document to the Hub — no VS Code plugin redeploy, since it's a hosted schema, not code baked into the client. That's the exact distribution mechanism you asked for.

One gap: Standards define types and Patterns mandate their use — neither is a browsable catalogue of pre-built node snippets a user picks from. If what your team actually needs is that catalogue rather than a schema contract, that's closer to what template-bundles/ already does, and worth comparing before we introduce a new NodeStore concept. Happy to take that as its own issue if the schema-contract approach above doesn't cover your case.

Thanks @rocketstack-matt for the feedback. The gap that you highlighted is what we wanted to address, we need a pre built catalog of node snippets which includes correct & curated controls with their requirements mapped for Architects to reuse and confirm to our governance process. Thank You.
PS: we can continue our research on template bundles and can talk about it in our next WG if that meets the needs & address the gaps.

@markscott-ms markscott-ms mentioned this pull request Sep 17, 2026
7 tasks
@markscott-ms

Copy link
Copy Markdown
Contributor

This PR was discussed in the OH. The terminology of Building Block and the general concept was discussed on the call with multiple organisations deeming it useful..

Some post-OH notes...

  • Architecture Building Blocks (ABBs) — abstract, capability/requirement-level definitions (closer to what CALM standard does: defines the constraints, like "integer dimensions, these colours").
  • Solution Building Blocks (SBBs) — concrete, selectable, potentially vendor/product-specific implementations that satisfy an ABB, but not yet placed into a specific architecture.

A catalogue of partially-defined nodes maps closely to SBBs — pieces you can pick off the shelf, concrete enough to be selected, but not yet instantiated into a specific design. So "Building Block Catalogue" (or "Solution Building Block Catalogue" if we want to lean on the TOGAF term precisely) reads as standard architecture vocabulary rather than an invented name.

So... Building Block is ok with me.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

calm-hub Affects `calm-hub` calm-hub-ui Affects `calm-hub-ui`

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants