Audit cleanup: concurrency, error handling, DRY pass#407
Merged
Conversation
Cross-package code-review pass addressing high- and medium-severity
findings across the SDK, CLI, MCP server, VS extension, and mrt-utilities.
Concurrency / correctness:
- SDK: atomic write for the stateful auth-session file (temp + rename).
- SDK: single-flight refresh in OAuthStrategy.getAccessToken so concurrent
callers coalesce instead of stampeding the AM token endpoint.
- SDK: clean up debug-session keepalive/poll timers when connect()
fails after starting them.
- SDK: try/finally around progress timers in code download/deploy so
aborted requests can no longer leak intervals.
- CLI: long-running commands (code:watch, logs:tail, mrt:tail-logs)
deregister their SIGINT/SIGTERM handlers when finished.
- VS ext: guard webview postMessage against panel disposal in the
Swagger API Browser; drain pending uploads before tearing down code-
sync watchers; prevent sandbox-tree polling stop-checks from stacking
when the polling interval is shorter than the stabilization window.
- mrt-utilities: pipeToDestination now destroys the destination on
pipeline error so consumers fail fast instead of hanging.
- MCP: registerToolsets() throws clearly if invoked twice for the
same server; telemetry send failures are logged at debug.
Error handling:
- Replaced silent catch {} blocks in stateful-store unlinkSync,
job:run afterOperation hooks, logs:tail signal handler, and MCP
telemetry with debug-level logging.
- CIP commands now stream output through ux.stdout instead of
process.stdout, restoring --json and test capture; CIP tests use
runSilent.
DRY / dead code:
- New @salesforce/b2c-tooling-sdk/ux export hosts the canonical
confirm() prompt; CLI's prompts.ts is now a thin re-export.
- New auth/jwt-utils consolidates JWT exp/scope decoding previously
duplicated across three auth strategies.
- AM list commands share an amPageSizeFlag definition.
- formatApiError SLAS/SCAPI variants now alias the SDK's
getApiErrorMessage; ECDN gets a single requireScapiCoordinates
helper.
- Removed deprecated LocalSourceResult re-export.
Lint, typecheck, and the full test suite (4,075 tests) pass.
clavery
requested review from
patricksullivansf,
wei-liu-sf and
yhsieh1
as code owners
Companion to the audit cleanup. Addresses every High and the highest-
leverage Medium items from TEST_AUDIT_PLAN.md so the test suite catches
real regressions instead of just confirming functions ran.
CLI:
- expectError(fn, match?) helper added to test/helpers/test-setup.ts;
replaces the verbose try { await x; expect.fail(...) } catch {}
pattern that could swallow the wrong error type.
- code/deploy: tighten .calledOnce checks to verify args (instance,
cartridges, code version) and afterOperation hook payload.
- code/activate: assert PATCH path/body and reload toggle order
(active → alternate → active) instead of just call counts.
- auth/token: assert returned JSON shape and that ux.stdout was called
with exactly the access token, not just .calledOnce.
- mrt/env/var/push: positively assert that listEnvVars ran when
asserting setBatchStub was *not* called.
- cip/query: drop echo tautologies (result.sql === input); assert that
the resolved SQL was passed to mockClient.query.
- Promote sinon.stub(odsClient) and makeCommandThrowOnError out of 12
duplicated copies in sandbox tests into the canonical helpers.
- Switch .to.equal(true|false) → .to.be.true|false where present.
SDK:
- logger.test.ts: drop 18 redundant `expect(logger).to.exist` lines
(the followup `.to.be.a('function')` was the real check).
MCP:
- registry: new smoke test invokes a registered handler
(sfnext_get_guidelines) end-to-end; previously only tool names were
asserted.
- theming-store: 25 weak `expect(guidance).to.exist` calls upgraded to
`expect(guidance, '...').to.not.be.undefined` so failures point at
the missing key.
- figma generate-component: replace 7 `array.some(d => …).to.equal(true)`
chains with `array.find(...)` + concrete property assertions so
failures show what actually went wrong.
mrt-utilities:
- create-lambda-adapter-compression.test.ts: replace 39 fixed
`setTimeout(50ms)` waits with the existing event-driven
`stream.waitForEnd()`. Suite runtime drops from ~3s to ~1s.
All 4,076 tests pass (+1 from the new MCP smoke test). Lint and
typecheck clean.
clavery
added a commit
that referenced
this pull request
May 7, 2026
Acts on findings from a post-merge audit covering both this branch and recently-merged main work: SDK: - Export ACCESS_KEY_SCOPES + AccessKeyScope from operations/bm-users so the 4 access-key CLI commands no longer redeclare the same tuple. - Refresh stale doc comment on getBmUserAccessKey (referenced removed example values 'WEBDAV', 'OCAPI', 'SCAPI'). - printFieldsBlock now accepts null in addition to undefined and skips both, matching the common shape of optional OpenAPI fields. New DetailValue type alias exported. CLI — apply our helpers to MRT commands main introduced in PR #407: - mrt/org/cert/list and mrt/org/member/list now use TableRenderer + columnFlagsFor + selectColumns instead of inline createTable. - mrt/org/cert/get and mrt/org/member/get now use printFieldsBlock instead of inline cliui label/value rendering. CLI — finish the @inquirer/prompts -> SDK ux migration begun in main: - setup, setup/instance/remove, sandbox/reset, sandbox/alias/delete, and mrt/env/var/push now use confirm() from @salesforce/b2c-tooling-sdk/ux. The two remaining @inquirer/prompts importers (setup/skills, setup/instance/create) need more than just confirm so they stay as-is for now.
clavery
added a commit
that referenced
this pull request
May 7, 2026
…408) * Add bm users/whoami/access-key commands + table flag consistency Adds full Business Manager Data API user administration to the CLI: - bm users list/get/search/update/delete (OCAPI /users, /user_search) - bm whoami (OCAPI /users/this — defaults to user-auth) - bm access-key get/create/set/delete (OCAPI /users/{login}/access_key/{scope}; optional [LOGIN] defaults to whoami; --scope is an enum with WEBDAV_AND_STUDIO as the default) A new SDK module @salesforce/b2c-tooling-sdk/operations/bm-users wraps the underlying endpoints. Endpoints whose OCAPI documentation states "a valid user is required" (whoami + access-key) extend a shared BmUserAuthCommand base which defaults the auth-method priority to ['implicit'] so a fresh shell triggers browser login rather than failing the API call with UserNotAvailableException. Also reworks tabular output across the CLI for consistency: - New SDK helpers columnFlagsFor() / selectColumns() replace 22 copies of an identical getSelectedColumns() helper. printFieldsBlock() does the same for *Get-style label/value detail blocks (5 commands). - Adds --columns / --extended to ~30 list and search commands that previously had no column-customization (bm roles list, webdav ls, cap list, code list, content list, docs search, job search, logs list, sites list, slas client list, every mrt/* list command, plus several setup and scaffold commands). webdav ls --extended now exposes the previously-hidden modified and contentType columns. - Renames --confirm to --force on the new bm/users delete commands to match the dominant codebase convention. Skills + docs: new b2c-cli:b2c-bm-users-roles skill and a rewritten docs/cli/bm.md page cover the four bm command groups and the user-auth defaulting. The b2c-am skill now defers to the new BM skill. * Migrate bm delete commands to canonical ux/confirm helper Switches bm/users/delete and bm/access-key/delete from @inquirer/prompts to the SDK's @salesforce/b2c-tooling-sdk/ux confirm() introduced in main. Drops the @inquirer/prompts dependency from these two files. * Audit pass: hoist scope enum, extend helpers to MRT, migrate confirms Acts on findings from a post-merge audit covering both this branch and recently-merged main work: SDK: - Export ACCESS_KEY_SCOPES + AccessKeyScope from operations/bm-users so the 4 access-key CLI commands no longer redeclare the same tuple. - Refresh stale doc comment on getBmUserAccessKey (referenced removed example values 'WEBDAV', 'OCAPI', 'SCAPI'). - printFieldsBlock now accepts null in addition to undefined and skips both, matching the common shape of optional OpenAPI fields. New DetailValue type alias exported. CLI — apply our helpers to MRT commands main introduced in PR #407: - mrt/org/cert/list and mrt/org/member/list now use TableRenderer + columnFlagsFor + selectColumns instead of inline createTable. - mrt/org/cert/get and mrt/org/member/get now use printFieldsBlock instead of inline cliui label/value rendering. CLI — finish the @inquirer/prompts -> SDK ux migration begun in main: - setup, setup/instance/remove, sandbox/reset, sandbox/alias/delete, and mrt/env/var/push now use confirm() from @salesforce/b2c-tooling-sdk/ux. The two remaining @inquirer/prompts importers (setup/skills, setup/instance/create) need more than just confirm so they stay as-is for now. * Add tests for bm whoami, bm users, and bm access-key commands 10 new test files following the bm/roles test patterns. Each covers: JSON-mode return shape, non-JSON output (where applicable), flag/arg behavior, and OCAPI error paths via the expectError helper. - whoami.test.ts (3 cases) - users/{list,get,delete}.test.ts (3 cases each) - users/search.test.ts (5 cases — covers convenience flags, raw --query passthrough, and invalid JSON rejection) - users/update.test.ts (4 cases — covers the field→snake_case mapping and "no fields" guard) - access-key/{get,delete}.test.ts cover both the explicit-login and whoami-fallback branches via two-call OCAPI stubs - access-key/{create,set}.test.ts cover scope flag and PATCH body shape CLI tests now: 1218 passing (was 1184). SDK tests unchanged at 1722. * Strengthen authentication coverage in bm CLI docs bm.md: - Lead the Authentication section with the two flows (client-credentials vs user-auth) and explicit setup before the "defaults" table. - Document --user-auth, --auth-methods, and SFCC_AUTH_METHODS overrides with concrete examples. - Annotate the OCAPI permissions table with which command uses each resource so readers know what to grant. - Add a dedicated subsection on the Manage_Users_Access_Keys BM functional permission required for access-key writes. - Add Configuration Examples block. authentication.md: - Add "BM administration" entry under "Minimal Configuration by Feature" with the importable JSON snippet covering /roles, /users, /users/this, /users/*/access_key/*, and /user_search. - Add a tip box explaining the user-identity requirement on whoami / access-key endpoints and cross-link back to /cli/bm#authentication. Both pages now properly cross-link to each other.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two-part audit cleanup on this branch:
Part 1 — Source audit (commit
04fa47dc)Cross-package code review of
@salesforce/b2c-cli,@salesforce/b2c-tooling-sdk,@salesforce/b2c-dx-mcp,b2c-vs-extension, and@salesforce/mrt-utilities. Addresses every High and Medium item that wasn't a false positive — 21 fixes total.Concurrency & correctness:
OAuthStrategy.getAccessTokento coalesce concurrent token requests; cleanup of debug-session keepalive/poll timers whenconnect()fails after starting them; try/finally around progress timers in codedownload/deploy.code:watch,logs:tail,mrt:tail-logs) now deregister their SIGINT/SIGTERM handlers when finished.postMessageagainst panel disposal in the Swagger API Browser; drain pending uploads before tearing down code-sync watchers; prevent sandbox-tree polling stop-checks from stacking.pipeToDestinationdestroys the destination on pipeline error so consumers fail fast instead of hanging.registerToolsets()throws clearly if invoked twice for the same server.Error handling: replaced silent
catch {}blocks in stateful-store,job:run,logs:tail, MCP telemetry, code-sync dispose. CIP commands stream output throughux.stdoutinstead ofprocess.stdout, restoring--jsonand test capture.DRY / dead code: new
@salesforce/b2c-tooling-sdk/uxexport hosts canonicalconfirm(); newauth/jwt-utilsconsolidates JWT decoding from three sites; AM list commands shareamPageSizeFlag; ECDN gets a singlerequireScapiCoordinateshelper; SLAS/SCAPIformatApiErroraliases SDK'sgetApiErrorMessage; removed deprecatedLocalSourceResult.Part 2 — Test audit (commit
03fbda6f)Companion sweep addressing every High and the highest-leverage Medium item from a parallel test-quality audit. Goal: make the test suite catch real regressions instead of just confirming functions ran.
CLI tests:
expectError(fn, match?)helper replaces the verbosetry { await x; expect.fail(...) } catch {}pattern that could absorb the wrong error type.code/deploy,code/activate,auth/token,mrt/env/var/push,cip/query— tightened.calledOnce/.calledassertions to verify call args (instance, cartridges, paths, payloads) instead of just invocation counts.expect(result.sql).to.equal(input)); assertions now confirm the substituted SQL was passed to the client.stubOdsClientandmakeCommandThrowOnErrorout of 12 sandbox test files into the canonical helpers.SDK tests: dropped 18 redundant
expect(logger).to.existlines inlogger.test.ts.MCP tests: added a smoke test that actually invokes a registered tool handler (previously only tool names were asserted); 25 weak
expect(x).to.existcalls intheming-store.test.tsupgraded to named.to.not.be.undefined; replaced 7.some(d => …).to.equal(true)chains in figmagenerate-componentwith.find(...)+ concrete property assertions so failures show what actually went wrong.mrt-utilities tests: replaced 39 fixed
setTimeout(50ms)waits increate-lambda-adapter-compression.test.tswith the existing event-drivenstream.waitForEnd(). Suite runtime drops from ~3s to ~1s.Verified false positives
Source: H3 (prophet
dw.jsonfallback runs in user env, can't import SDK), H14 (Lambda response mock is fully populated synchronously), M2/M4/M5/M8/M9/M10/M11/M13/M15/M16/M17/M20/M21 (already-correct patterns or larger refactors).Tests: the cross-cutting agent's "406 weak
expect.fail" was overstated (verified: 167 typed catches + 200 with proper post-assertions + 0 truly empty); the SDK agent's "30+ middleware tests silently skip" claim was wrong (theif (!x) throwis TypeScript narrowing —throwinit()fails loud).Test plan
pnpm run lint:agentcleanpnpm run typecheck:agentcleanpnpm run test:agent— 4,076 tests passing (651 mcp + 1,705 sdk + 516 mrt + 1,204 cli)b2c cip query --jsonand--format json/csvproduce expected outputb2c code watchexits cleanly on Ctrl+C with no leaked listenersb2c auth clientinvocations don't corruptauth-session.json