Skip to content

fix: bump the @sasjs deps, save minimal targets, and require node >= 22 - #1482

Merged
allanbowe merged 18 commits into
mainfrom
fix/roll-deps-config-node-lts
Oct 14, 2026
Merged

allanbowe merged 18 commits into
mainfrom
fix/roll-deps-config-node-lts

Conversation

@sasjs-dev

@sasjs-dev sasjs-dev Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

What

Rolls #1478, #1479 and #1480 into one PR, addresses every open review finding on all three, and bumps @sasjs/adapter, @sasjs/core and @sasjs/lint (plus @sasjs/utils) to their current releases.

Three strands:

1. Dependency bump (was #1478, extended)

@sasjs/adapter   4.19.2 -> 4.19.4
@sasjs/core      5.3.0  -> 5.3.2
@sasjs/lint      4.1.2  -> 4.1.3
@sasjs/utils     3.7.1  -> 3.7.2

2. Node floor, manifest hygiene and release tooling (was #1479)

engines.node ^14.18.0 || >=16.10.0 -> >=22, the oldest line still receiving security fixes and what CI already runs; the old range was already false, since the prod tree requires >=22. The dead checkNodeVersion / nodeVersionMessage scripts and the caller-less checkNodeVersion() in src/utils/utils.ts are gone - engines is the enforcement. axios is now declared (it was imported from two files but resolved only through adapter's hoist). jwt-decode, lodash.groupby, @types/lodash.groupby and tslib are dropped: nothing in the tree references them. The release toolchain (semantic-release, @semantic-release/exec) moved out of npx -p into release/, its own locked tree installed with npm ci --prefix release, audited separately by release/audit.mjs; the release checkout gets fetch-depth: 0.

3. Minimal target saving (was #1480)

saveToLocalConfig / saveToGlobalConfig defaulted saveWithDefaultValues to true, so the token-refresh path - the one caller that omits the argument - rewrote the saved target with a full set of defaults on every refresh. Both now default to false, with JSDoc stating the contract.

That flip exposed a user-facing regression once the pin moved to the fixed utils: the Target constructor no longer injects { deployServicePack: true, deployScripts: [] }, so a target declaring no deployConfig had none at all and deploy() refused. Fixed in the CLI - an absent deployConfig means the historical default, deploy the service pack, and an explicit deployServicePack: false is still honoured.

Review findings addressed

#1478 (5 findings at 3b1e91d) - all addressed, four of them by the #1479 strand which this PR folds in:

finding where it lands
stale engines range and the matching checkNodeVersion script >=22 in the manifest, scripts dropped, dead function and spec removed
ghost axios import declared and pinned to the resolved 1.20.0
stale jwt-decode removed
stale lodash.groupby + @types/lodash.groupby removed
unreferenced tslib removed

#1479 - final review at ef17b9a reads "No blocking issues"; all five earlier rounds of findings are in the branch as-is.

#1480 (2 open findings at 604c423) - both addressed:

finding where it lands
engines.node still the old range while utils 3.7.1 requires >= 22 >=22 here, with the lockfile root regenerated
the absent-deployConfig default had no spec new case in deploy.spec.ts, beside the existing SAS Viya service-pack case: a target that declares no deployConfig, asserting the service-pack path runs

The second one matters: the rest of the suite declares a deployConfig everywhere, which is why the runtime regression reached CI before any spec caught it.

Also in this PR

handlebars 4.7.9 -> 4.7.10 in release/package-lock.json. Three advisories were published against 4.7.9 (GHSA-xw65-4hp5-5hc7, GHSA-8r5x-fm3f-whwj, GHSA-p8wg-vrv2-v86f), all of which 4.7.10 is the first patched version for; the release audit goes back to exit 0 with its 13 documented exemptions. Nothing else moved.

Verification

build                        exit 0, 0 TS errors
jest (mocked)                87 suites / 667 tests, 2 suites failing on the
                             environment alone - docs (needs doxygen) and
                             version (needs a global @sasjs/cli install);
                             21 tests, all in those two
coverage                     74.52 / 62.78 / 75.21 / 75.1, above the
                             required 73 / 60.55 / 73.54 / 74
npm audit --omit=dev         0 vulnerabilities
node release/audit.mjs       13 exempt, exit 0
npm@10 ci --dry-run          exit 0, root tree and release/ tree

Provenance

The three commits of #1480 and the eleven of #1479 are carried here with git cherry-pick -x; the only conflict was the @sasjs/utils pin, resolved to 3.7.2. #1478's own two commits are not carried because the bump they perform is superseded by the one above - the outcome is the same packages, at newer versions.

#1478, #1479 and #1480 are closed as superseded, each with a comment recording what landed here.

Merge with squash or rebase

semantic-release reads only the first-parent chain, so a merge commit's subject is invisible to it.

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Coverage report

Total coverage

Status Category Percentage Covered / Total
🟡 Statements 75.72% 3570/4715
🟡 Branches 63.43% 1464/2308
🟡 Functions 75.53% 716/948
🟢 Lines 84.28% 9089/10784

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Show files with reduced coverage 🔻

Reduced coverage

Status Filename Statements Branches Functions Lines
🟢 src/utils/utils.ts 87.22% 77.14% 80.43% 92.12%
🟢 src/commands/compile/compileSingleFile.ts 95.83% (-2.08% 🔻) 84% (-4% 🔻) 100% 100%

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Report generated by 🧪jest coverage report action from 8ecc679

@sasjs-dev sasjs-dev Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewed the full base...head diff as one unit: the dependency bumps, the node floor change, the manifest hygiene removals, the release toolchain extraction, the minimal-target-save default flip and the deploy fallback. Verified against live data: both CI jobs pass at the head, depcheck reports no confirmed-unused and no ghost packages, the release toolchain audit runs clean (13 documented exemptions, exit 0), npm audit --omit=dev --audit-level=low on the head reports 0 vulnerabilities, and the @sasjs/utils 3.6.5 vs 3.7.2 Target constructor and toJson contracts were checked against upstream source.

  • src/utils/config.ts:319 and src/utils/config.ts:439 - the saveWithDefaultValues default flip to false is user-visible: any external caller of the exported saveToGlobalConfig/saveToLocalConfig that relied on the old true default now gets a minimal target. All in-repo callers were audited and either pass the argument explicitly (saveConfig.ts:14) or are the token-refresh path whose minimal save is the intent (config.ts:830), so this is in-repo sound, but the default change is a public API contract change worth a release note beyond the commit bodies; the existing tests in src/utils/spec/config.spec.ts (saveToGlobalConfig/saveToLocalConfig describes) only assert target presence, not the saved shape, so the minimal-vs-defaults distinction itself has no coverage - consider one assertion each on the persisted JSON (deployConfig absent for a minimal save, present for saveWithDefaultValues=true).

  • The node floor raise to ">=22" (package.json:5) is an install-time breaking change for anyone on node 16-21, but the base tree was already constrained to >=18 in practice (jsdom 26.0.0 engines >=18 at the base lockfile), so the practical break is 18/20 users. The commit is typed plain fix(package): with no BREAKING CHANGE footer, and this repo releases from commit subjects via semantic-release, so it cuts a patch. Advisory: either add the footer (major) or accept the patch consciously; the PR body's argument that the old range was already false is fair for 14/16 but not for 18/20.

  • release/audit.mjs:126-129 - stale exemptions are reported to stdout but never fail the build, so an exemption that outlives its advisory rots silently in CI output. Consider failing (or warning to stderr) when stale.length > 0, matching the script's own stated rule "an exemption that is no longer needed should be deleted, not left to rot".

  • .github/workflows/npmpublish.yml installs and runs the toolchain from the locked tree, and release/package-lock.json is committed and matches release/package.json (semantic-release 25.0.9, @semantic-release/exec 7.1.0 both exact-pinned). The pattern is already field-proven in sasjs/adapter. No issue - noting the verification because the npx-to-locked-tree swap is the riskiest change here.

  • src/commands/deploy/deploy.ts:34 - the ?? true fallback was verified against the old utils 3.6.5 constructor (which injected deployServicePack: true) and the 3.7.2 removal: the fallback restores the historical behaviour exactly, including for a partial deployConfig whose validateDeployConfig coerces an explicit false to real false (false ?? true stays false). The new spec covers the no-deployConfig case. No issue.

  • Housekeeping verifications: axios was indeed an undeclared import in src/utils/utils.ts and src/commands/run/run.ts (now declared at package.json exact pin 1.20.0, lockfile updated); checkNodeVersion, jwt-decode, lodash.groupby and tslib have zero remaining references in the tree; .npmrc carries ignore-scripts=true and save-exact=true; the gitleaks pre-commit hook is present and pinned to v8.30.1; both lockfiles are committed and the release tree installs with --ignore-scripts. No findings from these.

4 findings above for review.

@sasjs-dev

sasjs-dev Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Both actionable findings addressed in 8b5aeb6.

1. The minimal-vs-defaults coverage. You were right that the flip was asserted nowhere. There are now two new cases, one each in saveToGlobalConfig and saveToLocalConfig, asserting on the persisted JSON rather than target presence:

  • a minimal save (saveWithDefaultValues omitted) of a target that declares no deployConfig - the shape the token-refresh path constructs - persists neither deployConfig nor buildConfig;
  • a saveWithDefaultValues: true save persists the injected deployConfig.

One subtlety the first assertion forced, which is worth the record: generateTestTarget declares deployConfig: { deployServicePack: true } explicitly, so a minimal save of a target built that way correctly keeps its declared deployConfig - toJson(false) only drops defaults the caller never declared. The minimal case therefore constructs the target inline, the way the token-refresh path does. Config spec: 43 passing.

2. Stale exemptions now fail the build. release/audit.mjs reports them on stderr and exits 1, matching the script's own "deleted, not left to rot" rule. Both branches exercised: the clean tree exits 0 (13 exempt), and a stubbed bogus exemption id exits 1 and names it. The whole file had drifted past the prettier gate, so it is reformatted too.

On the node floor (finding 2): accepted as breaking. bdfa2d5 fix(package): require the oldest supported LTS (node >= 22) now carries a BREAKING CHANGE: engines.node is now >= 22; node 18 and 20 users must stay on the previous release footer, so semantic-release cuts a major rather than a patch. Confirmed locally that the analyzer reads it as major.

On the release-note point (finding 1): the footer lands it in the changelog, which is the release note this repo generates. There is nothing to add on top.

sasjs-dev Bot added 16 commits October 14, 2026 12:29
The declared node floor permits end-of-life releases - node 14 and 16 reached
EOL in 2023, 18 on 2025-04-30, and 20 on 2026-04-30. The oldest line still
receiving security fixes is 22, which is also the version CI runs.

Raise engines.node to >=22 so the declaration matches the runtimes this
package is tested and supported on.

Verification: the only package.json change is the engines.node value, and CI
runs on node 22.

(cherry picked from commit c6e342e)
The declared node floor permits end-of-life releases - node 14 and 16 reached
EOL in 2023, 18 on 2025-04-30, and 20 on 2026-04-30. The oldest line still
receiving security fixes is 22, which is also the version CI runs.

Raise engines.node to >=22 so the declaration matches the runtimes this
package is tested and supported on.

Verification: the only package.json change is the engines.node value, and CI
runs on node 22.

BREAKING CHANGE: engines.node is now >= 22; node 18 and 20 users must stay on
the previous release.

(cherry picked from commit c6e342e)
…se tooling

Three findings from review, plus one of the same staleness class:

- axios is imported by src/utils/utils.ts and src/commands/run/run.ts but was
  declared nowhere - it rode on the hoist from @sasjs/adapter's dependency.
  Declared directly, pinned to 1.20.0 (the version already resolved, so the
  tree does not move).
- the `checkNodeVersion` and `nodeVersionMessage` npm scripts are referenced by
  no workflow and no doc, and the check embedded "^14.18.0 || >=16.10.0" with
  `m > 16` logic, so it would accept node 18/20 that engines now rejects.
  Dropped both; engines does the job.
- src/utils/utils.ts's checkNodeVersion() refused only below node 12, and said
  so to the user. Raised to 22, matching engines, with its spec updated.
- .github/workflows/npmpublish.yml ran `npx -p ... semantic-release` unpinned,
  resolving latest on every release with publish credentials. Pinned to the
  versions the other sasjs release workflows use.

Verification:
- npm ci: 698 packages
- npx jest src/utils/spec/utils.spec.ts: 46 tests pass
- npm run lint: clean
- package-lock.json root now records axios and engines >=22

Note: `npm run test:mocked` bails on src/commands/version/spec/version.spec.ts
in a sandbox, because printVersion() shells out to `npm list -g @sasjs/cli` and
nothing is installed globally. Reproduced identically on the base commit with
these changes stashed, so it is environmental, not this diff.

(cherry picked from commit 71a639d)
The release toolchain now lives in `release/` with its own package.json and
lockfile, installed by the publish workflow with `npm ci --prefix release`,
rather than being resolved from npx at release time. Locked, so the resolved
versions and their integrity hashes are pinned - the gap raised in review, where
npx pinned only the top levels and fetched the transitive tree fresh.

The `files` allowlist already keeps `release/` out of the tarball.

Verification: every plugin in the release config resolves from
release/node_modules.

(cherry picked from commit 5242e0a)
The toolchain in release/ carries twelve advisories that cannot be fixed from
this repo - the ranges cover the newest published version - and that code runs
with publish credentials. Moving it out of the package's dependencies kept it
out of the package's `npm audit`, which is right; it also made it invisible,
which is not.

release/audit.mjs reports it separately and by name: the twelve are exempt with
their reasons, anything else fails the build, and an exemption that stops
appearing is reported so it can be deleted rather than left to rot. It uses
--package-lock-only, so it needs no install and runs on every PR as well as in
the release.

Verification: node release/audit.mjs reports 12 exempt and exits 0.
(cherry picked from commit f1c4506)
sas-language runs `prettier --check .`, which covers release/ - it is the only
one of the five repos that checks that directory - and it rejected the new
script. Formatted with that config (printWidth 100).

The file is kept byte-identical in all five repos, so the same content goes to
the four that do not check it. Otherwise the copies drift and a later fix lands
in some of them only.

Verification: `prettier --check .` clean, `node release/audit.mjs` still reports
12 exempt and exits 0, in all five.

(cherry picked from commit ce73dd9)
The publish workflow's checkout is shallow (depth 1), and semantic-release needs
the full history and tags to compute the version. It fetches --unshallow itself,
which works on a public repo but is a wasted round-trip on every release and
fails outright if the fetch is blocked. The sibling release workflows (adapter,
utils, sas-language) all set fetch-depth: 0 for this reason.

(cherry picked from commit 331c4fd)
`checkNodeVersion` in src/utils/utils.ts has no caller: src/index.ts does not
call it, it is not re-exported from cli.ts or index.ts, and nothing else in the
repo references it. It was already unreachable before this PR; this PR edited it
(the floor and the message), which is the moment to settle it either way.

Dropped rather than wired into the entrypoint, because as written it would fail
silently there: it reports through `process.logger?.error`, and the entrypoint
has not set up a logger yet, so the optional chain short-circuits and the
process exits 1 with no message at all.

`engines: ">=22"` is the enforcement, and npm surfaces it on install. The spec
block covering the function goes with it.

Verification: tsc -p tsconfig.json clean, prettier clean, src/utils/spec 44
tests pass (was 46).

(cherry picked from commit b82f4fd)
…udit

The audit caught a new advisory, which is what it is for: GHSA-rj75-hqrm-r3gf,
moderate, quadratic complexity in flat selector parsing.

It cannot be fixed from here. The vulnerable copy is npm's own bundled
dependency - release/node_modules/npm/node_modules/postcss-selector-parser -
so no resolver change reaches it, and 11.21.0 is the last npm 11.x while
@semantic-release/npm requires ^11. The eventual fix is that plugin moving to
npm 12. It is also unreachable in this usage: the parser reads this repo's own
release config, not the commits being matched.

Named with that reasoning rather than silenced, so the audit still fails on
anything new. 13 exempt, exit 0.

(cherry picked from commit 3e02d7a)
…udit

The audit caught a new advisory, which is what it is for: GHSA-rj75-hqrm-r3gf,
moderate, quadratic complexity in flat selector parsing.

It cannot be fixed from here. The vulnerable copy is npm's own bundled
dependency - release/node_modules/npm/node_modules/postcss-selector-parser -
so no resolver change reaches it, and 11.21.0 is the last npm 11.x while
@semantic-release/npm requires ^11. The eventual fix is that plugin moving to
npm 12. It is also unreachable in this usage: the parser reads this repo's own
release config, not the commits being matched.

Named with that reasoning rather than silenced, so the audit still fails on
anything new. 13 exempt, exit 0.

(cherry picked from commit 70c4ace)
Surfaced by the depcheck pass and carried through several reviews. Verified by
full-tree grep before removing:

- jwt-decode 3.1.2 and lodash.groupby 4.6.0 were runtime dependencies, but
  nothing in src, docs or scripts imports jwtDecode or groupBy. lodash.uniqby
  beside them is used; these two were not, so every @sasjs/cli consumer
  installed them for nothing.
- tslib 2.8.1 was a devDependency with no reference anywhere, and tsconfig sets
  no importHelpers to justify it.
- @types/lodash.groupby 4.6.9 loses its reason with lodash.groupby gone.

Verified after removal: `tsc --noEmit` clean, and the utils and config specs run
83 tests, all passing.

(cherry picked from commit ef17b9a)
`saveToLocalConfig` and `saveToGlobalConfig` defaulted `saveWithDefaultValues`
to true, so any caller that omitted the argument wrote every non-mandatory
attribute into the saved sasjsconfig. The token-refresh handler does exactly
that - it passes only the target and isDefault - so every token refresh rewrote
the global target with a full set of defaults, including a `deployConfig` the
user never wrote.

That is the other half of #221. The constructor and `toJson` halves are in
sasjs/utils#277; this repo passes explicit values on the `sasjs add` path, so
that path is unchanged, but the refresh path was writing full configs on its
own.

The specs that relied on the default are unaffected: they assert that the target
is present and whether it is the default, not the JSON shape.

Verification:
- npx jest src/utils/spec/config.spec.ts: 39 pass
- npx jest src/commands/add: 3 suites, 34 tests pass
- npx tsc -p tsconfig.json --noEmit: clean
- npm run lint: clean

Takes full effect once this repo's `@sasjs/utils` pin moves to the release
carrying the constructor fix (utils 3.7.1, from sasjs/utils#277) - until then
the constructor still inserts `deployConfig` before `toJson` is reached.

(cherry picked from commit 7b5725f)
Two review findings.

- Both flipped functions now carry a JSDoc stating the new contract: omitting
  `saveWithDefaultValues` persists a minimal target - the mandatory attributes
  only - and passing true writes the full set of injected defaults, including a
  deployConfig the caller never declared. The behaviour inverted, so the
  signature should say so rather than leave the next caller to infer it.

- The pin moves to @sasjs/utils 3.7.1, the release carrying the constructor fix
  from sasjs/utils#277. Without it this change is half-effective: 3.6.5's Target
  constructor inserts a default deployConfig for any target that declares none,
  so toJson(false) still emits it after the flip. sasjs/adapter#919 has already
  moved to 3.7.1; this follows it.

Verified: `tsc --noEmit` clean, config.spec 39 tests passing, and package.json
and the lockfile root both at 3.7.1.

(cherry picked from commit c8560b7)
The @sasjs/utils 3.7.1 bump in this PR exposed a downstream dependency. The
Target constructor used to inject
`{ deployServicePack: true, deployScripts: [] }` into any target that declared
no deployConfig. sasjs/utils#277 removed that injection, so after the bump a
target without a deployConfig has none at all - and deploy() then refuses:

    Deployment failed.
    Please either enable the 'deployServicePack' option or add deployment
    script paths to 'deployScripts' in your target's 'deployConfig'.

That is a user-facing regression: any existing sasjsconfig that omits
deployConfig deployed before this and would stop deploying once the pin moves.

Fixed where it belongs - in the CLI. An absent deployConfig now means the
historical default, deploy the service pack. An explicit
`deployServicePack: false` is still honoured, and the guard that throws uses the
same resolved value rather than re-reading the raw config.

The utils change was about not writing defaults into the user's saved config. It
was never meant to change runtime behaviour, and this restores that.

Pinned by a new case in deploy.spec.ts, beside the existing SAS Viya
service-pack case: a target that declares no deployConfig, asserting the
service-pack deploy path runs. The rest of the suite declares a deployConfig
everywhere, which is why the runtime regression reached CI before any spec
caught it.

Verified: tsc clean; deploy specs 29 tests passing; config and add specs 73
passing. The job that caught it - the SAS9 mocks deploy against a real
sasjs/server - is the end-to-end proof and will re-run on this head.
    @sasjs/adapter   4.19.2 -> 4.19.4
    @sasjs/core      5.3.0  -> 5.3.2
    @sasjs/lint      4.1.2  -> 4.1.3
    @sasjs/utils     3.7.1  -> 3.7.2

adapter 4.19.4 carries the Socket alert cleanup (#915 + #917): tough-cookie
5.1.2 drops psl, punycode, url-parse, querystringify and requires-port from the
prod tree, and the bundled @sasjs/utils drops cli-table, colors, lodash.isequal
and lodash.isfunction. It also declares engines.node >= 18, the floor its own
chain requires.

core 5.3.2 is the mf_getvalue edge-case fix (#459), on top of 5.3.1's
ms_runstp / ms_triggerstp / ms_testservice debug= parameter fix (#458).

lint 4.1.3 requires node >= 22 and syncs its own lockfile engines, so it agrees
with the engines floor this PR declares for the CLI. It still pins
@sasjs/utils 3.6.4, so a nested utils copy stays in the tree under lint - the
same exact-pin chain as adapter, and it converges when lint bumps its own pin.

utils 3.7.2 is a CI-only change (the release-marker filter check, #279); no
runtime difference from 3.7.1.

handlebars 4.7.9 -> 4.7.10 in the release toolchain lockfile. Three advisories
were published against 4.7.9 (GHSA-xw65-4hp5-5hc7, GHSA-8r5x-fm3f-whwj,
GHSA-p8wg-vrv2-v86f), all of which 4.7.10 is the first patched version for. The
release audit goes back to exit 0 with its 13 documented exemptions. The bump
lands in release/package-lock.json only, which is where the toolchain resolves.

Verified on this tree:

    build                              exit 0, 0 TS errors
    jest (mocked)                      87 suites / 667 tests, 2 suites failing
                                       on the environment alone - docs (needs
                                       doxygen) and version (needs a global
                                       @sasjs/cli install); 21 tests, all in
                                       those two
    coverage                           74.52 / 62.78 / 75.21 / 75.1, above the
                                       required 73 / 60.55 / 73.54 / 74
    npm audit --omit=dev               0 vulnerabilities
    node release/audit.mjs             13 exempt, exit 0
Adds the coverage the minimal-save flip was missing: one assertion each on
the persisted JSON for saveToGlobalConfig and saveToLocalConfig, so the
minimal-vs-defaults distinction has a spec pinning it.

The minimal case constructs a target that declares no deployConfig, like the
token-refresh path whose behaviour this is for, and asserts both deployConfig
and buildConfig are absent from the saved JSON. The defaults case asserts the
injected deployConfig is present when saveWithDefaultValues is true.

Also makes a stale release-toolchain exemption fail the build, matching the
script's own stated rule that an exemption which outlives its advisory should
be deleted, not left to rot.

Verified: config spec 43 passing; node release/audit.mjs exit 0; tsc clean;
prettier clean.
@sasjs-dev
sasjs-dev Bot force-pushed the fix/roll-deps-config-node-lts branch from 8b5aeb6 to c27e2de Compare October 14, 2026 12:35

@sasjs-dev sasjs-dev Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-review at the new head c27e2de (previous review was at 70f388f): full base...head diff re-checked at the current head, plus the hardening battery - depcheck triage (no confirmed-unused, no ghosts), lockfile sync via npm ci dry-run on both trees, npm audit --omit=dev --audit-level=low (0 vulnerabilities), release/audit.mjs (exit 0, 13 documented exemptions), and CI status (both workflows, Linux and Windows, green at this head). The previous review's findings that this push addressed - the persisted-shape specs for the minimal save, and the BREAKING CHANGE footer on the node-floor commit - are not re-raised; the footer reaches the squash commit body since this repo squashes with COMMIT_MESSAGES.

  • src/utils/spec/config.spec.ts:589 - the test name contradicts the call it makes. "should persist the target with injected defaults when saveWithDefaultValues is omitted" calls await saveToGlobalConfig(target, true, true) at line 596 - the argument is passed explicitly, not omitted, and with the new default of false an omitted call would persist a minimal target, the opposite of what the name claims. The assertions at :603 (deployConfig truthy) pass only because true is passed. Rename to match the saveToLocalConfig twin at :725 ("when saveWithDefaultValues is true").

  • release/audit.mjs:151-157 - carried over from the previous review, unchanged at this head: stale exemptions (EXEMPT entries whose advisory no longer appears in the audit report) are printed to stdout but never fail the build, so an exemption that outlives its advisory rots silently in passing CI output. The script's own comment states the rule - "An exemption that is no longer needed should be deleted, not left to rot" - enforce it: exit non-zero (or at minimum write the block to stderr and surface a warning) when stale.length > 0.

2 findings above for review.

…in exemptions

Renames the saveToGlobalConfig defaults case to match its saveToLocalConfig
twin: the test passes saveWithDefaultValues explicitly, so "is true" is the
accurate description, not "is omitted" - which under the new default of false
describes the opposite save.

Makes a stale exemption (an EXEMPT entry whose advisory no longer appears in
the audit report) exit 1 on stderr, matching the script's own stated rule
that an exemption which outlives its advisory should be deleted, not left to
rot. Both branches exercised: the clean tree exits 0 (13 exempt), a stubbed
bogus exemption id exits 1 and names it.
@sasjs-dev

sasjs-dev Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Both findings addressed in 146e8d9.

1. The test name. Renamed to "should persist the target with injected defaults when saveWithDefaultValues is true", matching its saveToLocalConfig twin. You were right - the case passes the argument explicitly, so "omitted" described the opposite save under the new default. Config spec still 43 passing.

2. The stale exemptions. Fixed for real this time, and the previous attempt deserves the explanation: my earlier "fix" was verified against a stubbed tree and then discarded by the cleanup - I restored the file with git checkout release/audit.mjs, which reverted it to HEAD and silently dropped the change, leaving only the prettier reformatting in the pushed commit. The review caught it; the same exit-1 change is now in, with both branches exercised: the clean tree exits 0 (13 exempt), and a stubbed bogus exemption id exits 1 and names it on stderr.

@sasjs-dev sasjs-dev Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-review at the new head 146e8d9 (prior reviews at 70f388f and c27e2de are superseded): the delta since c27e2de is one commit touching the two files the previous review flagged, both verified by reading the head tree; the rest of the base...head diff was re-checked and carries no new findings. Both test jobs are green at this head.

Both previous findings are addressed in this push and not re-raised: src/utils/spec/config.spec.ts:589 now reads "should persist the target with injected defaults when saveWithDefaultValues is true" and matches its call and the saveToLocalConfig twin at :725, and release/audit.mjs:151-157 now reports stale exemptions on stderr and fails the build via process.exit(1).

  • release/audit.mjs:151-157 - the new stale-exemption gate fires before the failing gate, so when a stale EXEMPT entry and a real unexempted advisory coexist the run exits with the stale message only and the failing advisory is never reported. Move the stale block after the failing.length block (or aggregate both into one non-zero exit) so an unexempted advisory is never masked by the stale report.

1 finding above for review.

The stale-exemption gate fired before the failing gate and exited, so when
both existed the run reported only the stale exemption and never mentioned
the unexempted advisory. Both blocks now report, and a single exit at the
end fails the build if either did. Order preserved: the unexempted advisory,
which is the reason the toolchain is unsafe to publish, prints before the
housekeeping note about deletable exemptions.

All four branches exercised: clean tree exits 0 (13 exempt); a stale
exemption alone exits 1 and names it; an unexempted advisory alone exits 1
and names it; both together exit 1 and report both.
@sasjs-dev

sasjs-dev Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in 8ecc679, and the aggregation option was the right one: the stale-exemption gate no longer exits early, so an unexempted advisory is never masked. Both blocks report, and a single process.exit(1) at the end fails the build if either fired. Order preserved - the unexempted advisory, which is the reason the toolchain is unsafe to publish, prints before the housekeeping note about deletable exemptions.

All four branches exercised: clean tree exit 0 (13 exempt); stale exemption alone exit 1, named; unexempted advisory alone exit 1, named; both together exit 1, both reported.

@sasjs-dev sasjs-dev Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-review at the new head 8ecc679 (prior reviews at 70f388f, c27e2de and 146e8d9 are superseded): the delta since 146e8d9 is one commit touching only release/audit.mjs, the file the previous finding was about; the rest of the base...head diff was re-checked and carries no new findings. Both test jobs are green at this head.

The previous finding is addressed in this push and not re-raised: release/audit.mjs now reports the failing-advisory block and the stale-exemption block to stderr unconditionally (no early exit between them), with a single aggregated exit at release/audit.mjs:172 - an unexempted advisory is no longer masked by a stale-exemption report, and vice versa.

Reviewed the diff - no blocking issues found.

@allanbowe
allanbowe merged commit 5f154f4 into main Oct 14, 2026
2 checks passed
@allanbowe
allanbowe deleted the fix/roll-deps-config-node-lts branch October 14, 2026 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant