Conversation
|
|
||
| export type ResourceChangeType = 'MAJOR' | 'MINOR' | 'PATCH'; | ||
|
|
||
| export const NARRATIVE_DOCUMENT_TYPES = ['knowledge', 'sad'] as const; |
There was a problem hiding this comment.
These are structurally identical to CALM_DOCUMENT_TYPES_LIST / CalmDocumentType / isValidCalmDocumentType in calm-models/types. Co-locating them there would give a single source of truth and clean up the union in bundle.ts and the import in workspace-document-loader.ts. Hub API DTOs should stay here.
There was a problem hiding this comment.
The narrative document list, type, and guard now live in calm-models beside the existing CALM document types. Hub request and response DTOs remain in shared
| throw new Error('Narrative document Hub identity is incomplete. Re-add the document to repair it.'); | ||
| } | ||
| const identity = { | ||
| namespace: entry.namespace ?? '', |
There was a problem hiding this comment.
The ?? '' is misleading — validateNarrativeIdentity rejects empty strings anyway. Same at bump.ts.
There was a problem hiding this comment.
Push and bump now validate the manifest namespace directly. Missing or invalid namespaces produce a clear validation error rather than being replaced with an empty fallback value.
| const location = await client.createNarrativeDocumentVersion( | ||
| identity.namespace, identity.type, identity.calmHubDocumentId!, version, narrative.request | ||
| ); | ||
| parseNarrativeDocumentLocation(location, identity); |
There was a problem hiding this comment.
Return value discarded — pure validation. A // validate Location comment would prevent someone removing this.
There was a problem hiding this comment.
Location checks now use the named validateNarrativeDocumentLocation wrapper. This makes it clear that the call validates the Hub response even when its parsed value is not otherwise required.
|
|
||
| vi.mock('@finos/calm-shared/src/hub/calm-hub-client', () => ({ | ||
| CalmHubClient: mocks.CalmHubClient, | ||
| NARRATIVE_DOCUMENT_TYPES: ['knowledge', 'sad'], |
There was a problem hiding this comment.
Will drift if a type is added to the canonical constant. Consider vi.importActual.
There was a problem hiding this comment.
commands.spec.ts imports CALM_NARRATIVE_DOCUMENT_TYPES_LIST directly from calm-models, which is not mocked in that test. The command-choice assertion therefore always uses the real canonical constant without needing vi.importActual. Happy to change to use vi.importActual if this is the preferred method or importing the document types from calm-models is not appropriate
| copy: options.copy, | ||
| type, | ||
| namespace: options.namespace.trim(), | ||
| version: '1.0.0', |
There was a problem hiding this comment.
Always 1.0.0, no --version override. Re-adding after manifest removal will reset regardless of Hub state.
There was a problem hiding this comment.
Recovery requires --calm-hub-document-id and --ver, validates the identity, loads configured Hub authentication, fetches the exact remote version, and compares the complete Markdown before writing the manifest entry. This prevents recovery from resetting a published document to 1.0.0.
| for (const [id, entry] of Object.entries(manifest)) { | ||
| const filePath = resolveFilePath(bundlePath, entry.path); | ||
| if (!existsSync(filePath)) { | ||
| if (isNarrativeDocumentType(entry.type)) throw new Error(`Narrative document '${id}' file not found: ${filePath}`); |
There was a problem hiding this comment.
Throws immediately here, while push.ts accumulates. Defensible, but a one-line rationale comment would help.
There was a problem hiding this comment.
Added a rationale comment in the narrative validation path: bump fails fast because it must validate the workspace before updating local manifest versions. Push can collect independent failures because it does not make that coordinated local change.
| import { existsSync, readFileSync } from 'fs'; | ||
| import { SchemaDirectory } from '../schema-directory'; | ||
| import path from 'path'; | ||
| import { NARRATIVE_DOCUMENT_TYPES, type NarrativeDocumentType } from '../hub/calm-hub-client'; |
There was a problem hiding this comment.
If types move to calm-models (see comment on calm-hub-client.ts), this can import from @finos/calm-models/types — matching the existing CalmDocumentType import.
There was a problem hiding this comment.
WorkspaceDocumentLoader now imports isNarrativeDocumentType from calm-models and skips narrative Markdown entries because it only handles CALM JSON documents and $ref resolution.
|
@101Steeps — POC is coming along nicely 👍. Just the CalmHub visuals left now. |
| urlToLocalPathMapping?: Map<string, string>; | ||
| } | ||
|
|
||
| const YAML_FRONTMATTER_PATTERN = /^---\r?\n([\s\S]*?)\r?\n---(?=\r?\n|$)/; |
There was a problem hiding this comment.
As we build this out, I want to see what it takes to converge this with docify's parseFrontMatterFromContent just below. For example, docify expects the closing --- (with optional trailing whitespace via .trim()), but this regex is stricter. Part of the POC goal is to figure out where the two can share code. I'll pick that up when I loop back to flow.
|
@101Steeps can you take a look at the feedback from @LeighFinegold and resolve the conflicts and failing builds on this PR please. |
…ment-publish-2983 # Conflicts: # cli/src/command-helpers/workspace/bump.spec.ts # cli/src/command-helpers/workspace/bump.ts # cli/src/command-helpers/workspace/commands.ts # shared/src/hub/calm-hub-client.ts
Merge conflicts resolved and feedback comments addressed. The smoke suite uses the Hub document API from PR #3002 , I think once PR #3002 is merged the smoke suite will pass for PR #3015 as the endpoints required will be present. I've run a combined local Hub image containing #3002 and #3015 and the smoke suite passed. For future, should I use stacked PRs where one PR has a dependency on another, or are separate PRs with combined local verification and a dependency note sufficient? |
|
|
||
| const manifest = await loadManifest(bundlePath); | ||
| manifest[id] = { path: rel, type: opts?.type ?? 'unknown', ...(opts?.namespace ? { namespace: opts.namespace } : {}) }; | ||
| manifest[id] = { |
There was a problem hiding this comment.
Re-adding a tracked narrative document silently drops its Hub identity and creates a duplicate on Hub.
addFileToBundle replaces manifest[id] wholesale. If a narrative document was already published (has calmHubDocumentId/calmHubId/version) and a user runs workspace add again for the same id — without the explicit --calm-hub-document-id/--ver recovery flags — the fresh-add code path in commands.ts calls addFileToBundle with only version: '1.0.0', overwriting the entry and losing calmHubDocumentId/calmHubId.
On the next workspace push, entry.calmHubDocumentId === undefined triggers client.createNarrativeDocument, creating a second, duplicate document on CalmHub instead of updating the existing one. There is no guard in addFileToBundle or the add command against overwriting an existing manifest entry's Hub identity.
There was a problem hiding this comment.
Fixed in 42dd30b. Re-adding an already-published narrative now preserves its existing calmHubDocumentId, calmHubId, and version. Conflicting re-registration is rejected, so an ordinary re-add can no longer drop the Hub identity and cause the next push to create a duplicate.
| throw new Error('A narrative document without calmHubDocumentId must use version 1.0.0.'); | ||
| } | ||
| const location = await client.createNarrativeDocument(identity.namespace, identity.type, narrative.request); | ||
| const documentId = parseNarrativeDocumentLocation(location, identity); |
There was a problem hiding this comment.
If createNarrativeDocument succeeds but parseNarrativeDocumentLocation(location, identity) then throws (e.g. an unexpected Location header shape), the document has already been created server-side, but manifest[id] is never updated with the new calmHubDocumentId/calmHubId — the exception propagates to the outer catch and only narrativeFailures is recorded.
The next workspace push will still see entry.calmHubDocumentId === undefined and call createNarrativeDocument again, creating another duplicate document on Hub. Every retry after a malformed-Location response compounds the problem.
There was a problem hiding this comment.
Fixed across 80332af, 0eff450, and finalised in 474c07e. A pending recovery state is now persisted before the create POST. If the create outcome is ambiguous, including a missing or malformed Location, that pending state is retained and a subsequent push fails before issuing another create. The document must be explicitly reconciled before pushing again.
| namespace: options.namespace.trim(), | ||
| type: options.type, | ||
| version: options.ver, | ||
| calmHubDocumentId: Number(options.calmHubDocumentId), |
There was a problem hiding this comment.
calmHubDocumentId: Number(options.calmHubDocumentId) accepts anything Number() parses leniently — e.g. --calm-hub-document-id 1e2 becomes 100, or a value with surrounding whitespace is trimmed. validateNarrativeIdentity only checks Number.isSafeInteger(...) && > 0, so 1e2 passes as a valid positive integer.
A typo in this flag (scientific notation, stray whitespace) silently recovers/matches against the wrong Hub document id instead of failing validation. Consider requiring the raw option string to match /^[1-9]\d*$/ before calling Number().
There was a problem hiding this comment.
Fixed in 78b0ef8. The raw --calm-hub-document-id value must now match /^[1-9]\d*$/ before conversion, and the converted value must also be a safe integer, so inputs such as 1e2 or values with surrounding whitespace are rejected
| continue; | ||
| } | ||
|
|
||
| if (isNarrativeDocumentType(entry.type)) { |
There was a problem hiding this comment.
Altitude: narrative-vs-mapping dispatch is done as a repeated if (isNarrativeDocumentType(entry.type)) { ...; continue/return; } early-branch, duplicated at every call site that iterates the manifest — here, pushWorkspaceToHub, the add action in commands.ts, and WorkspaceDocumentLoader.
Adding a third document kind means touching all of these functions again, and it's easy to miss one (the ref-rewrite cascade a few lines below, for example, deals with this by filtering narrative entries out of the manifest entirely rather than making syncReferences/buildRefRulesFromDiskIds type-aware). A small per-type handler/strategy interface implemented once by 'mapping' and 'narrative' document types would keep future document kinds from requiring a sweep across four+ files.
There was a problem hiding this comment.
Addressed in 7c5860e and 20e3668. Document-kind classification and dispatch are now centralised behind the workspace document handler/dispatch layer, with the shared loader using the same canonical classification policy. Mapping, narrative, legacy unknown, and unsupported types are handled explicitly rather than each consumer independently deciding narrative-vs-mapping behavior.
| } | ||
|
|
||
| if (conflicts.length > 0) { | ||
| if (conflicts.length > 0 || narrativeFailures.length > 0) { |
There was a problem hiding this comment.
Asymmetric failure handling between the two document kinds this function now pushes: a JSON mapping document's createMappedResourceVersion failure (further up, in the unchanged catch around client.createMappedResourceVersion) is only logged — the loop continues and pushWorkspaceToHub still resolves normally — while the equivalent narrative-document failure is pushed into narrativeFailures and fails the whole call via this check.
A CI pipeline that runs workspace push and checks the exit code gets a false success when a mapping document fails to publish (e.g. a transient network error), but a hard failure when a narrative document hits the same kind of error. Worth aligning the two paths now that push failures are being surfaced as errors.
There was a problem hiding this comment.
Fixed in 615d99d. Mapping Hub failures are now accumulated in mappingFailures and included in the final push error, so a failed mapping publish no longer logs the error and lets workspace push resolve successfully while the equivalent narrative failure fails.
| export type WorkspaceDocumentType = CalmDocumentType | 'unknown'; | ||
| export type WorkspaceDocumentType = CalmDocumentType | NarrativeDocumentType | 'unknown'; | ||
|
|
||
| export type WorkspaceManifestEntry = { |
There was a problem hiding this comment.
WorkspaceManifestEntry now overloads one flat shape for two unrelated identity models: for a mapping document, version/identity come from the embedded JSON $id and version/calmHubDocumentId are meaningless; for a narrative document, there is no $id at all and version/calmHubDocumentId/calmHubId are the only source of truth. calmHubId itself means 'the last-pushed mapping URL' for one type and 'the canonical Hub document Location' for the other.
Nothing in the type system stops a mapping entry from picking up a stray calmHubDocumentId, or a narrative entry from having calmHubId without calmHubDocumentId slipping through anywhere that doesn't call the addFileToBundle runtime check. A discriminated union on type (mapping vs narrative fields) would let the compiler catch this instead of surfacing it only as a runtime 'incomplete Hub identity' error deep in bump/push.
There was a problem hiding this comment.
Fixed in 552333e. WorkspaceManifestEntry is now a discriminated union separating mapping entries from unpublished, recovery-pending, and published narrative entries. Mapping entries cannot carry narrative identity fields, and published narrative identity requires the corresponding document id and Hub location together.
| try { | ||
| const version = entry.version; | ||
| if (!version) throw new Error('Narrative document manifest entry has no version. Re-add the document to repair it.'); | ||
| if ((entry.calmHubId === undefined) !== (entry.calmHubDocumentId === undefined)) { |
There was a problem hiding this comment.
This narrative-entry validation preamble (version presence, calmHubId/calmHubDocumentId XOR check, validateNarrativeNamespace, identity construction, parseNarrativeDocument) is duplicated almost verbatim in bump.ts's detectChangedResources (lines ~117-126). A fix to one — e.g. tightening the namespace check, or the incomplete-identity message — is easy to forget in the other, letting bump and push silently disagree on what counts as a valid narrative manifest entry.
Consider extracting a shared resolveNarrativeEntry(id, entry, raw) in narrative-document.ts that both callers use.
There was a problem hiding this comment.
Fixed in d19843d. The common validation and parsing path is now resolveNarrativeEntry, and both push and bump use it for version, namespace, Hub-identity consistency, identity validation, and Markdown parsing
| const docIncrement = options.perDocIncrements?.get(c.id) ?? options.increment; | ||
| const toVersion = computeSemVerBump(c.latestHubVersion, docIncrement); | ||
| if (c.kind === 'narrative') { | ||
| const manifest = await loadManifest(bundlePath); |
There was a problem hiding this comment.
bumpWorkspace calls loadManifest/saveManifest (full file read + write) once per narrative document inside the per-changed-doc loop, instead of loading the manifest once before the loop and saving once after. Bumping N narrative documents in a single workspace bump invocation does N redundant full manifest reads and N full rewrites of the whole workspace-manifest.json, where the mapping-document path in the very same loop does zero manifest I/O per document.
There was a problem hiding this comment.
Fixed in aba7582. Narrative changes now load the manifest once before applying the batch, update all narrative versions in memory, and save it once after the narrative update phase rather than performing a full read/write for every changed narrative.
| } | ||
| validateNarrativeIdentity(identity, true, id); | ||
| validateNarrativeDocumentLocation(entry.calmHubId, identity, false); | ||
| const versions = await client.getNarrativeDocumentVersions(identity.namespace, identity.type, identity.calmHubDocumentId!); |
There was a problem hiding this comment.
getNarrativeDocumentVersions and getNarrativeDocumentVersion (and their mapping-document equivalents just below) are issued strictly sequentially inside the for loop over all manifest entries. A workspace with many tracked documents makes one Hub round trip per document per document, one at a time, so workspace check/bump latency grows linearly with document count instead of being bounded by the slowest single request. Most of these calls are independent across documents and could run concurrently (Promise.all or a small concurrency pool).
There was a problem hiding this comment.
Fixed in e4ee2fb. Independent per-document Hub checks are prepared first and then executed concurrently with Promise.allSettled, with results still inspected in manifest order for deterministic handling.
| const hasVersion = options.ver !== undefined; | ||
| const hasHubUrl = options.calmHubUrl !== undefined; | ||
| const recoveryRequested = hasDocumentId || hasVersion || hasHubUrl; | ||
| if (recoveryRequested) { |
There was a problem hiding this comment.
The recovery branch here and the fresh-add narrative branch below (around line 126) both read the file, call parseNarrativeDocument, and call addFileToBundle with an overlapping options shape, differing only in where the identity comes from (CLI flags + Hub verification vs. a hardcoded version: '1.0.0') and the log-message ternary vs if/else. A future change to narrative registration (a new frontmatter field, a new addFileToBundle option) has to be made in both places; extracting a shared registerNarrativeDocument(bundlePath, srcPath, file, opts) helper that both branches call with a pre-resolved identity would keep them from drifting.
There was a problem hiding this comment.
Fixed in 42dd30b. Fresh narrative add and explicit recovery now both go through registerNarrativeDocument; identity resolution and remote verification happen before that shared registration path, so parsing and addFileToBundle registration logic are no longer duplicated.
Description
Implements #2983: publish versioned Markdown narrative documents from a CALM workspace to CALM Hub.
sadandknowledgeMarkdown documents.workspace check,bump, andpushfor narrative documents.Depends on the narrative-document API in #3002 / #2982. The combined local POC branch was used only for integration testing; this PR remains focused on the CLI and shared client changes.
Type of Change
Affected Components
cli/)calm/)calm-ai/)calm-hub/)calm-hub-ui/)calm-server/)calm-widgets/)docs/)shared/)calm-plugins/vscode/)Commit Message Format ✅
All commits follow Conventional Commits.
Testing
I have tested my changes locally
I have added/updated unit tests
All existing tests pass
npm testpassed on Linux: all 17 workspace summaries passed (including shared: 90 files / 1,084 tests; CLI: 28 files / 682 tests).npm run lintcompleted with 0 errors; 11 existing warnings remain outside this change.npm run build:clipassed.Combined local POC: First-class Documents in CALM Hub #2981 POC, with Add a first-class Document endpoint to CALM Hub #2982 merged locally:
cd calm-hub && ../mvnw verify— 2,772 tests passed; JaCoCo checks passed.cd calm-hub && ../mvnw -P integration verify— 537 tests passed; JaCoCo checks passed.bash scripts/build-hub-smoke-image.sh— passed and builtcalm-hub:smoke.npm run test:smoke --workspace cli— 7 test files / 28 tests passed.The smoke test exercises the full narrative-document workflow against a disposable Docker Calm Hub instance: create a workspace, publish architecture and Markdown SAD
1.0.0, retrieve it, detect an edited-but-unbumped document, bump to1.1.0, republish it, and retrieve the updated Markdown.A standalone manual POC repeated the same flow against local Calm Hub. The final
workspace checkreported the workspace up to date and all documents valid.Swagger Evidence

Endpoint list showing the five narrative document routes
Version API used by

workspace pushRetrieval endpoint used to verify the stored markdown

Checklist