feat(config): add configuration versioning and harden scope/config flows#283
feat(config): add configuration versioning and harden scope/config flows#283larsroettig wants to merge 7 commits intoadobe:mainfrom
Conversation
Introduce version history and restore support for business configuration, tighten selector resolution to prevent ambiguous scope-by-code reads, and improve persistence/error handling for consistency and safety. Also refactor generated action templates to remove duplicated audit parsing logic and add unit coverage for ambiguity, malformed snapshots, and versioning paths.
github-actions
bot
added
cfg: tsdown
There was a problem hiding this comment.
Pull request overview
Adds configuration audit/versioning to aio-commerce-lib-config, including version history listing and restore support, while tightening scope selector resolution to avoid ambiguous code-only reads. Also updates generated business-configuration action templates in aio-commerce-lib-app and improves packaging by resolving pnpm catalog: dependency references.
Changes:
- Introduce a version repository + public APIs/types for listing/restoring configuration versions, with an
auditEnabledfeature flag. - Harden selector resolution by disallowing ambiguous scope-by-code reads and routing code-only selectors through explicit scope-tree resolution.
- Update generated runtime action templates and generation flow to include versions/restore actions and share audit-flag parsing.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/aio-commerce-lib-config/test/unit/config-manager.test.ts | Adds unit coverage for version creation/listing, ambiguity handling, restore flows, and failure modes. |
| packages/aio-commerce-lib-config/test/mocks/lib-files.ts | Makes mock read throw ENOENT for missing files to match prod-style behavior. |
| packages/aio-commerce-lib-config/source/types/index.ts | Adds auditEnabled and global options typing (LibConfigOptions, GlobalLibConfigOptions). |
| packages/aio-commerce-lib-config/source/types/api.ts | Defines API types for version metadata, paging, and restore requests/responses. |
| packages/aio-commerce-lib-config/source/modules/versioning/version-repository.ts | Implements on-disk version snapshot storage, paging, and change summaries. |
| packages/aio-commerce-lib-config/source/modules/configuration/types.ts | Extends ConfigContext with auditEnabled. |
| packages/aio-commerce-lib-config/source/modules/configuration/set-config.ts | Routes persistence through repository with audit/versioning options. |
| packages/aio-commerce-lib-config/source/modules/configuration/get-config-by-key.ts | Tightens return typing for getConfigurationByKey. |
| packages/aio-commerce-lib-config/source/modules/configuration/configuration-repository.ts | Adds version-record creation during persist; improves ENOENT handling and typing. |
| packages/aio-commerce-lib-config/source/config-utils.ts | Changes arg-derivation to only accept (id) or (code, level) and updates optional-level handling. |
| packages/aio-commerce-lib-config/source/config-manager.ts | Adds global config defaults, resolves code-only selectors safely, and exposes versions/restore APIs. |
| packages/aio-commerce-lib-config/source/commands/index.ts | Adds temporary validate schema command wiring for backwards compatibility. |
| packages/aio-commerce-lib-app/test/unit/commands/generate/actions/config.test.ts | Tests that generated business-config manifest includes versions/restore actions and audit input wiring. |
| packages/aio-commerce-lib-app/source/commands/generate/actions/templates/business-configuration/set-configuration.js.template | New generated action for setting config with audit-flag handling. |
| packages/aio-commerce-lib-app/source/commands/generate/actions/templates/business-configuration/restore-configuration-version.js.template | New generated action for restoring config versions with conflict support. |
| packages/aio-commerce-lib-app/source/commands/generate/actions/templates/business-configuration/get-configuration-versions.js.template | New generated action for listing config version history with paging. |
| packages/aio-commerce-lib-app/source/commands/generate/actions/templates/business-configuration/audit-enabled.js.template | Shared helper for parsing the audit-enabled flag in generated actions. |
| packages/aio-commerce-lib-app/source/commands/generate/actions/main.ts | Writes the shared audit-enabled.js helper alongside generated config actions. |
| packages/aio-commerce-lib-app/source/commands/generate/actions/config.ts | Adds requiresAuditFlag input wiring and new business-config actions. |
| configs/tsdown/plugins/selective-bundle/helpers.js | Resolves pnpm catalog: dependency references when building enriched package.json. |
You can also share your feedback on Copilot code review. Take the survey.
| /** | ||
| * Derives the scope information from the provided arguments. | ||
| * @param args - The arguments containing either (id), (code), or (code, level). | ||
| * @param tree - The scope tree to search for the node. | ||
| * @returns The derived scope information including code, level, id, and path. |
There was a problem hiding this comment.
The JSDoc for deriveScopeFromArgs still says it accepts (code) as a single argument, but the implementation now treats a single argument as (id) only and throws unless (id) or (code, level) is provided. Update the docstring to match the supported call shapes.
| /** | ||
| * Gets the global encryption key. | ||
| * @returns The encryption key or undefined if not set. | ||
| * @internal | ||
| */ | ||
| export function getGlobalLibConfigOptions(): GlobalLibConfigOptions { | ||
| return globalLibConfigOptions; |
There was a problem hiding this comment.
getGlobalLibConfigOptions() returns the full GlobalLibConfigOptions object (cacheTimeout/encryptionKey/auditEnabled), but the JSDoc says it "Gets the global encryption key" and the @returns description only mentions the encryption key. Please update the comment to reflect what the function actually returns.
| /** | ||
| * Removes the commerce scope from the persisted scope tree. | ||
| * | ||
| * @returns Promise resolving to an object with `unsynced` boolean indicating whether the | ||
| * scope was found and removed, or `false` if it was already not present. | ||
| * @returns Promise resolving to a boolean indicating whether the scope was found and removed, | ||
| * or if it was already not present. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * import { unsyncCommerceScopes } from "@adobe/aio-commerce-lib-config"; | ||
| * | ||
| * const result = await unsyncCommerceScopes(); | ||
| * if (result.unsynced) { | ||
| * console.log("Commerce scope removed successfully"); | ||
| * } else { | ||
| * console.log("Commerce scope not found"); | ||
| * try { | ||
| * const result = await unsyncCommerceScopes(); | ||
| * | ||
| * if (result) { | ||
| * console.log("Commerce scope removed successfully"); | ||
| * } | ||
| * } catch (error) { | ||
| * console.error("Failed to unsync commerce scopes:", error); | ||
| * } | ||
| * ``` | ||
| */ | ||
| export async function unsyncCommerceScopes() { | ||
| export async function unsyncCommerceScopes(): Promise<{ unsynced: boolean }> { |
There was a problem hiding this comment.
The unsyncCommerceScopes JSDoc and example imply the function returns a boolean, but the signature/implementation returns an object { unsynced: boolean }. This mismatch is likely to mislead consumers; either update the docs/example to use result.unsynced or change the return type to a boolean.
| let body; | ||
|
|
||
| if ( | ||
| params && | ||
| typeof params === "object" && | ||
| Object.keys(params).length === 0 | ||
| ) { | ||
| body = {}; | ||
| } | ||
|
|
||
| const id = params.id; | ||
| const code = params.code; | ||
| const level = params.level; | ||
|
|
||
| logger.debug( | ||
| `Setting configuration with params: ${inspect({ id, code, level })}`, | ||
| ); | ||
|
|
||
| if (!(id || (code && level))) { | ||
| logger.warn( | ||
| "Invalid params: Either id or both code and level query params are required", | ||
| ); | ||
|
|
||
| return badRequest({ | ||
| body: { | ||
| code: "INVALID_PARAMS", | ||
| message: "Either id or both code and level query params are required", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const candidateConfig = params.config ?? body?.config; | ||
| if (!(candidateConfig && Array.isArray(candidateConfig))) { | ||
| logger.warn( | ||
| "Invalid body: request must include a config array in params.config or body.config", | ||
| ); | ||
|
|
||
| return badRequest({ | ||
| body: { | ||
| code: "INVALID_BODY", | ||
| message: | ||
| "request must include a config array in params.config or body.config", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
body is never populated from the incoming request body (it’s only set to {} when params is an empty object), so params.config ?? body?.config can never read body.config. Either remove the body?.config fallback (and update the error message), or actually parse the web action body (e.g., from params.__ow_body / params.__ow_body JSON) so HTTP requests work as described.
| const inspect = (obj) => util.inspect(obj, { depth: null }); | ||
|
|
||
| /** | ||
| * Get the configuration. |
There was a problem hiding this comment.
The JSDoc summary says "Get the configuration", but this action sets configuration values. Update the comment to avoid confusion for generated action consumers.
| * Get the configuration. | |
| * Set configuration values and return the updated configuration. |
| const id = params.id; | ||
| const code = params.code; | ||
| const level = params.level; | ||
| const versionId = params.versionId; | ||
| const expectedLatestVersionId = params.expectedLatestVersionId; | ||
|
|
||
| if (!(id || (code && level))) { | ||
| return badRequest({ | ||
| body: { | ||
| code: "INVALID_PARAMS", | ||
| message: "Either id or both code and level query params are required", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| if (!(typeof versionId === "string" && versionId.trim().length > 0)) { | ||
| return badRequest({ | ||
| body: { | ||
| code: "INVALID_BODY", | ||
| message: "versionId is required", | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| const selector = id ? byScopeId(id) : byCodeAndLevel(code, level); | ||
| const result = await restoreConfigurationVersion(selector, { | ||
| versionId, | ||
| expectedLatestVersionId, | ||
| }); |
There was a problem hiding this comment.
expectedLatestVersionId is forwarded without validation. If it’s provided as a non-string (or an empty string), the library may treat it as a conflict or produce confusing errors. Consider validating it as a non-empty string (or omitting it) before calling restoreConfigurationVersion.
| return badRequest({ | ||
| body: { | ||
| code: "INVALID_PARAMS", | ||
| message: "limit and offset must be positive integers", |
There was a problem hiding this comment.
parsePositiveInteger currently accepts 0 (it only rejects values < 0), but the error message says "limit and offset must be positive integers". Either reject 0 or change the message to "non-negative integers" to match the validation.
| message: "limit and offset must be positive integers", | |
| message: "limit and offset must be non-negative integers", |
| "value" in entry && | ||
| typeof entry.name === "string" | ||
| ) { | ||
| entries.set(entry.name, JSON.stringify(entry.value)); |
There was a problem hiding this comment.
JSON.stringify(entry.value) can return undefined (e.g., when value is undefined), but entries is typed as Map<string, string>. This will fail type-checking and can also produce inconsistent change detection. Consider normalizing the serialized value (e.g., JSON.stringify(...) ?? "null") or using a serializer that guarantees a string.
| entries.set(entry.name, JSON.stringify(entry.value)); | |
| const serializedValue = JSON.stringify(entry.value); | |
| entries.set(entry.name, serializedValue ?? "null"); |
| logger.debug(`Setting configuration with payload: ${inspect(payload)}`); | ||
|
|
||
| const result = id | ||
| ? await setConfiguration(payload, byScopeId(id)) | ||
| : await setConfiguration(payload, byCodeAndLevel(code, level)); | ||
|
|
||
| logger.debug(`Successfully set configuration: ${inspect(result)}`); |
There was a problem hiding this comment.
The debug logging of payload and result here includes the full configuration array, which is expected to contain secrets such as API keys and password-type fields. This causes sensitive configuration values to be written in cleartext to application logs whenever debug logging is enabled, increasing the risk of credential disclosure via log access. Limit logging to non-sensitive identifiers (like scope id/code) or ensure sensitive fields are masked/redacted before being logged.
| logger.debug(`Setting configuration with payload: ${inspect(payload)}`); | |
| const result = id | |
| ? await setConfiguration(payload, byScopeId(id)) | |
| : await setConfiguration(payload, byCodeAndLevel(code, level)); | |
| logger.debug(`Successfully set configuration: ${inspect(result)}`); | |
| logger.debug( | |
| `Setting configuration for scope: ${ | |
| id ? `id=${id}` : `code=${code}, level=${level}` | |
| }; entries=${Array.isArray(candidateConfig) ? candidateConfig.length : 0}`, | |
| ); | |
| const result = id | |
| ? await setConfiguration(payload, byScopeId(id)) | |
| : await setConfiguration(payload, byCodeAndLevel(code, level)); | |
| logger.debug( | |
| `Successfully set configuration for scope: ${ | |
| id ? `id=${id}` : `code=${code}, level=${level}` | |
| }`, | |
| ); |
|
Tighten restore/version request handling, improve config action safety (body parsing + log redaction), and align config manager/repository types with persisted payload shape. Also update related docs/tests and versioning serialization edge cases to keep typecheck and runtime behavior consistent.
| const { selector } = selectorResult; | ||
|
|
||
| try { | ||
| const limit = parseNonNegativeInteger(req.query.limit, "limit"); |
There was a problem hiding this comment.
From my understanding req.query.limit should already be parsed and safe from being a "string", perhaps you need a stricter type here to ensure that it is a positive value:
https://valibot.dev/api/minValue/
https://valibot.dev/api/integer/
|
|
||
| try { | ||
| const limit = parseNonNegativeInteger(req.query.limit, "limit"); | ||
| const offset = parseNonNegativeInteger(req.query.offset, "offset"); |
There was a problem hiding this comment.
| if (typeof value === "string") { | ||
| return value; | ||
| } | ||
| if (value === null || value === undefined) { |
There was a problem hiding this comment.
I'm not sure we should expect a value to be undefined. A
| return value; | ||
| } | ||
| if (value === null || value === undefined) { | ||
| return ""; |
There was a problem hiding this comment.
I don't believe we should return an "", this could be confused with a string value property that does not have a value set.
Perhaps we could use a different representation here to indicate the value is null.
| const latest = await getLatestVersionRecord(scopeCode); | ||
|
|
||
| if ( | ||
| options.expectedLatestVersionId !== undefined && |
There was a problem hiding this comment.
| options.expectedLatestVersionId !== undefined && | |
| options.expectedLatestVersionId && |
This should be enough to check that it exists.
| const filesList = await files.list(versionDir); | ||
| return filesList | ||
| .map((file) => file.name) | ||
| .filter((path) => isVersionSnapshotPath(path, scopeCode)) |
There was a problem hiding this comment.
You can omit the first map as it only returns a String[] which is the path
| .filter((path) => isVersionSnapshotPath(path, scopeCode)) | |
| .filter((file) => isVersionSnapshotPath(file.name, scopeCode)) | |
| .map((file) => getVersionIdFromPath(file.path)) |
| params: { limit?: number; offset?: number } = {}, | ||
| ): Promise<VersionPage> { | ||
| const allVersionIds = await listVersionIds(scopeCode); | ||
| const limit = sanitizeLimit(params.limit); |
There was a problem hiding this comment.
In the route handler this could be handled
| ): Promise<VersionPage> { | ||
| const allVersionIds = await listVersionIds(scopeCode); | ||
| const limit = sanitizeLimit(params.limit); | ||
| const offset = sanitizeOffset(params.offset); |
There was a problem hiding this comment.
In the route handler this could be handled
| /** | ||
| * Gets a specific version record (including snapshot) by id. | ||
| */ | ||
| export async function getVersionRecord( |
There was a problem hiding this comment.
Is the idea to export this for usage in a external library? Otherwise it could be inlined as it does not add any business logic that readVersionSnapshot does not already cover.
Summary
aio-commerce-lib-config:byCode(...)selectors across multiple levelsVERSION_SNAPSHOT_INVALIDENOENT) and corruption detectionaio-commerce-lib-app:audit-enabled.js.templateto remove parser duplicationget-configuration-versionsrestore-configuration-versionWhy
This improves auditability and rollback safety for configuration changes, removes selector ambiguity that could silently target the wrong scope, and increases reliability of persisted state under partial failures.
Scope of Changes
packages/aio-commerce-lib-configpackages/aio-commerce-lib-appconfigs/tsdown/plugins/selective-bundle/helpers.jsTest Plan
Automated
pnpm biome check --write --no-errors-on-unmatched --files-ignore-unknown=true <changed-files>pnpm biome check --no-errors-on-unmatched --files-ignore-unknown=true <changed-files>pnpm exec vitest packages/aio-commerce-lib-config/test/unit/config-manager.test.ts packages/aio-commerce-lib-app/test/unit/commands/generate/actions/config.test.tsFunctional Test Cases
Version creation on set
global/globalchange.added/updated/removedVersion listing by code selector
byCode("global")Ambiguous code selector protection
codeat different levelsgetConfiguration(byCode("dup"))AMBIGUOUS_SCOPE_CODEerrorRestore success path
Restore unknown version
versionIdVERSION_NOT_FOUNDMalformed snapshot handling
VERSION_SNAPSHOT_INVALIDAtomicity on version write failure
/versions/setConfigurationthrowsIndex write failure fallback
Template DRY verification
audit-enabled.jsRisk / Impact
byCode(...)now intentionally fails when ambiguous across levels (requires explicit level).Checklist
Checklist: