A major new capability: HARDFORMULA / SOFTFORMULA DQ rule types, backed by Handsontable's formulas plugin (HyperFormula).
Formula rules substitute column names with row-relative cell references and resolve DC.USER_NAME / DC.ORIG_VALUE to literal values; HARDFORMULA reuses the existing READONLY mechanism.
Live DC.ROW_STATUS via a hidden EDIT_STATUS column, so formulas can react to a row's edit state; the row-header +/-/~ indicator translates visual→physical row index so it stays correct when the grid is sorted.
Inserting rows uses hot.alter() so formula values realign instead of going stale.
UserService is now providedIn: 'root', fixing DC.USER_NAME (and other lazy-module readers) always seeing an unset user.
The column-info dropdown shows the applied formula.
Formula-overwritten cells are flagged and can be reverted; saveTable() submits the live computed value rather than the raw =... string.
Guards against EDIT_STATUS / dc.row_status colliding with a real column of the same name, and stops the client-only EDIT_STATUS column from being submitted to the backend during dynamic cell validation.
Cell revert
Revert one cells (or range of cells) to their original (current) value
Formatted / Unformatted toggle on the staging page
Mirrors the review page's toggle. getstagetable.sas now also returns SAS-formatted data (fmt_stagetable), and the UI picks between formatted and raw, falling back to raw when formatted data is absent. Added e2e coverage.
Viewbox resize on all edges
Viewboxes can now be dragged to resize from all four edges and corners (previously only the bottom-right corner). Resize handles sit above Handsontable's frozen header clones so clicks reach the handle rather than the table underneath.
Licence-key protocol mismatch warning
A licence key generated for one connection type (http/https) always fails to decrypt on the other. The mismatch is now detected client-side from the raw key text before submission, with a warning and the apply button disabled, instead of surfacing only as a generic "invalid key" error after a backend round trip.
Bug fixes
stage: ensure formats arrive from the backend in getstagetable.sas.
validations: bump core for the mp_validate fix.
CAS: support the REPLACE load type (plus docs).
startup: show the real startup-service response text when Viya returns a plain-text "Job error" body instead of JSON, instead of reporting every expected field as "not present".
editor: the column-info dropdown text can now be selected and copied.
editor: table-header buttons and title size to their content rather than fixed grid thirds, so the dataset name/row-count no longer wraps unnecessarily.
editor:dc.row_status / EDIT_STATUS is no longer submitted to the backend.
Dependencies / security
Resolved npm audit vulnerabilities in production dependencies (Angular HttpTransferCache cache-key ambiguity and i18n XSS advisories).
Aligned all @angular/* packages to the same lockstep version (20.3.27) so CI's strict npm ci passes.
Scoped the brace-expansion override to its actual vulnerable chain (fixes license-checker breakage).
Pinned babel-loader to resolve an unresolvable peer conflict; regenerated/patched the lockfile to fix npm ci sync errors.
Overrode nanoid to 3.3.18 in sas/ directly (rather than relying on @sasjs/cli's override, which doesn't propagate to consumers) so it survives future npm install runs.
## Highlights
### Formula-based data quality rules
A major new capability: `HARDFORMULA` / `SOFTFORMULA` DQ rule types, backed by Handsontable's formulas plugin (HyperFormula).
- Formula rules substitute column names with row-relative cell references and resolve `DC.USER_NAME` / `DC.ORIG_VALUE` to literal values; `HARDFORMULA` reuses the existing READONLY mechanism.
- Live `DC.ROW_STATUS` via a hidden `EDIT_STATUS` column, so formulas can react to a row's edit state; the row-header +/-/~ indicator translates visual→physical row index so it stays correct when the grid is sorted.
- Inserting rows uses `hot.alter()` so formula values realign instead of going stale.
- `UserService` is now `providedIn: 'root'`, fixing `DC.USER_NAME` (and other lazy-module readers) always seeing an unset user.
- The column-info dropdown shows the applied formula.
- Formula-overwritten cells are flagged and can be reverted; `saveTable()` submits the live computed value rather than the raw `=...` string.
- Guards against `EDIT_STATUS` / `dc.row_status` colliding with a real column of the same name, and stops the client-only `EDIT_STATUS` column from being submitted to the backend during dynamic cell validation.
### Cell revert
Revert one cells (or range of cells) to their original (current) value
### Formatted / Unformatted toggle on the staging page
Mirrors the review page's toggle. `getstagetable.sas` now also returns SAS-formatted data (`fmt_stagetable`), and the UI picks between formatted and raw, falling back to raw when formatted data is absent. Added e2e coverage.
### Viewbox resize on all edges
Viewboxes can now be dragged to resize from all four edges and corners (previously only the bottom-right corner). Resize handles sit above Handsontable's frozen header clones so clicks reach the handle rather than the table underneath.
### Licence-key protocol mismatch warning
A licence key generated for one connection type (http/https) always fails to decrypt on the other. The mismatch is now detected client-side from the raw key text before submission, with a warning and the apply button disabled, instead of surfacing only as a generic "invalid key" error after a backend round trip.
## Bug fixes
- **stage:** ensure formats arrive from the backend in `getstagetable.sas`.
- **validations:** bump core for the `mp_validate` fix.
- **CAS:** support the REPLACE load type (plus docs).
- **startup:** show the real startup-service response text when Viya returns a plain-text "Job error" body instead of JSON, instead of reporting every expected field as "not present".
- **editor:** the column-info dropdown text can now be selected and copied.
- **editor:** table-header buttons and title size to their content rather than fixed grid thirds, so the dataset name/row-count no longer wraps unnecessarily.
- **editor:** `dc.row_status` / `EDIT_STATUS` is no longer submitted to the backend.
## Dependencies / security
- Resolved npm audit vulnerabilities in production dependencies (Angular HttpTransferCache cache-key ambiguity and i18n XSS advisories).
- Aligned all `@angular/*` packages to the same lockstep version (20.3.27) so CI's strict `npm ci` passes.
- Scoped the `brace-expansion` override to its actual vulnerable chain (fixes license-checker breakage).
- Pinned `babel-loader` to resolve an unresolvable peer conflict; regenerated/patched the lockfile to fix `npm ci` sync errors.
- Overrode `nanoid` to 3.3.18 in `sas/` directly (rather than relying on `@sasjs/cli`'s override, which doesn't propagate to consumers) so it survives future `npm install` runs.
Adds HARDFORMULA/SOFTFORMULA DQ rule types, backed by Handsontable's
formulas plugin (gated per-table via hasFormulaRules). parseFormulaRule
substitutes column names with row-relative cell references (quote-aware,
blank-boundary matching per spec) and resolves DC.USER_NAME/DC.ORIG_VALUE
to literal values; applyFormulaRules injects the computed formula per row,
reusing the existing READONLY mechanism for HARDFORMULA.
DC.ROW_STATUS/EDIT_STATUS deliberately deferred - would require prepending
COLHEADERS, which headerColumns has undocumented positional coupling to
elsewhere in editor.component.ts.
- addRow()/insertRowAtPosition() use hot.alter() so HARDFORMULA/SOFTFORMULA
values realign on insert instead of going stale
- New hidden EDIT_STATUS column gives DC.ROW_STATUS a real, live cell
reference; row-header +/-/~ indicator now translates visual->physical
row index so it stays correct when the grid is sorted
- UserService is now providedIn: 'root' instead of module-scoped, fixing
DC.USER_NAME (and any other lazy-module reader) always seeing an unset user
- Column-info dropdown shows the applied formula ("√x=<formula>")
- Formulas' HyperFormula sync resolves a dotted `data` key differently
than getDataAtRowProp/datamap.get() - dataDotNotation: false is needed
for the renamed dc.row_status column to work as a live cell reference
- saveTable() now submits the live computed value for a formula cell, not
the raw '=...' string still sitting in dataSource
- Angular packages resolve within their existing ^20.3.26 range up to
20.3.27, which patches two advisories (HttpTransferCache cache-key
ambiguity, i18n XSS via event-handler attributes)
- brace-expansion (via exceljs -> archiver -> readdir-glob -> minimatch)
needed an override to ^5.0.9 - npm's resolver won't proactively bump a
nested transitive dep that already satisfies its declared semver range,
even when a patched version exists inside that same range
npm audit fix only bumped @angular/core/common/compiler to 20.3.27 (the
packages with a direct advisory), leaving animations/forms/platform-browser*/
router at 20.3.26 - each peer-depends on the others' exact version, so the
mismatch passed a lenient `npm install` locally but failed CI's strict
`npm ci`. All seven now resolve to 20.3.27.
- Angular packages (core/common/compiler/animations/forms/
platform-browser*/router/compiler-cli) all peer-depend on each other's
exact version - npm audit fix only bumped the three with a direct
advisory, leaving the rest mismatched; a plain `npm install` tolerated
it but CI's strict `npm ci` didn't. All now resolve to 20.3.27.
- undici/fast-uri updated via npm audit fix; brace-expansion pinned to
^5.0.9 via overrides (npm's resolver wasn't proactively bumping a
nested transitive dep that already satisfied its declared range)
Incremental `npm install <pkg>@version` patches left the lockfile
internally inconsistent enough that CI's `npm ci` rejected it as out of
sync with package.json (a long list of unrelated nested deps reported
missing). Regenerated from scratch instead of patching further; verified
with a clean `npm ci`, matching the resolved versions from the prior fix.
Fixed by patching only the 12 needed entries directly (9 @angular/*
packages to 20.3.27, brace-expansion to 5.0.9, fast-uri to 3.1.5, undici to 6.28.0) with exact registry resolved/integrity metadata, leaving the other 1925 packages untouched.
An unscoped "brace-expansion": "^5.0.9" override applies to every
occurrence in the tree, not just the vulnerable one - it was silently
replacing the 1.1.16 that glob's bundled minimatch@3.x actually needs,
breaking license-checker with "TypeError: expand is not a function".
Scoped it to the readdir-glob chain (exceljs -> archiver -> readdir-glob
-> minimatch@10.x), the same nested-override pattern already used here
for exceljs's own archiver/unzipper pins.
- Mirrors the review page's toggle: getstagetable.sas now also webouts
fmt_stagetable (SAS-formatted), selectFormattedRows() picks between it
and the raw stagetable, falling back to raw if fmt data is absent
- Adds stage.cy.ts e2e coverage and wires it into both build.yaml and
release.yaml's cypress --spec lists (previously out of sync with
each other on viewbox.cy.ts too)
The Filter/Edit/Upload buttons used btn-block (width: 100%), stretching
them wider than the Cancel/Add Row/Submit buttons shown during edit.
The back/viewboxes, title, and action-button columns were also locked to
equal 12-col grid thirds regardless of actual content, so the dataset
name/row-count wrapped even with visible free space on either side.
Switched the outer columns to size to their content (clr-col-*-auto) and
let the title column flex-grow into whatever space is left.
Handsontable's Menu widget closes on any mouseup inside an item and
unconditionally preventDefault()s contextmenu, even for the info item,
which has no callback and exists purely to show read-only column details.
That made its text impossible to select or right-click-copy. Marked the
item isCommand: false and stop mousedown/mouseup/contextmenu/selectstart
from bubbling past its rendered content to the menu's own listeners.
A misconfigured Viya computeTasks deployment makes the Compute service
return a plain-text "Job error" body instead of JSON. @sasjs/adapter can't
parse it and resolves with that raw text as adapterResponse, so both
startup and manual deploy validation reported every expected field as
"not present" without ever showing the actual cause. Added
getMalformedAdapterResponseMessage to detect a non-object adapterResponse
and surface its real text instead.
Adds right, left, bottom, and bottom-left-corner resize handles
alongside the existing bottom-right corner, and raises resize handle
z-index above Handsontable's frozen header clones so clicks reach the
handle instead of the table underneath it.
@sasjs/cli's own nanoid override doesn't propagate to consumers - npm
only reads overrides from the root project being installed. Adding it
here directly, rather than a raw lockfile patch, so it survives future
npm install runs instead of silently reverting again.
dynamicCellValidation() built its editors/getdynamiccolvals payload
from a raw dataSource row clone, which always carries the client-only
EDIT_STATUS column - unlike saveTable()'s submit path, nothing
stripped it before the request went out. Extracted the strip logic
(previously private to classifyRow) into a shared withoutEditStatus
util and applied it here too.
A key generated for one connection type (http/https) always fails to
decrypt on the other, surfacing only as a generic "invalid key" error
after a round trip to the backend. Detects the mismatch client-side
from the raw key text before the user submits, and disables the apply
button while it's showing.
Verdict: Comment — no blocking issues found; a few warnings and suggestions below.
This is a large, well-structured release PR (94 files, +7189/-3009). The new formula-based DQ rules feature is the core change, with supporting cell revert, formatted/unformatted staging toggle, viewbox resize on all edges, and licence-key protocol mismatch warning. The extraction of logic into small, well-tested utility functions is good practice.
Critical
None.
Warnings
W1 — quoteLiteral does not escape embedded double quotes (parseFormulaRule.ts:38-39)
When DC.USER_NAME or DC.ORIG_VALUE contains a double quote character (e.g. a username like john"doe or a cell value with "), the resulting formula string will have an unescaped quote inside a quoted literal, producing a malformed HyperFormula expression. This could cause the formula to silently fail to evaluate, or in a worst case, alter the formula's semantics. The values come from SAS data / user session, so while admin-controlled in most deployments, a user with a quote in their name is plausible.
If a numeric column's original value was stored in the comment as a non-numeric string (unlikely but possible if the comment was somehow corrupted or the raw SAS value was a special missing like .S), Number(rawValueText) returns NaN, which would then be written into the cell. There's no guard against this. Consider falling back to the raw text or validating before conversion.
W3 — Missing mp_abort between dc_assignlib and mp_getcols in getstagetable.sas (lines 48-52)
If mpe_submit has no row matching &table_id (e.g. stale table_id, race condition), &base_lib and &base_ds remain empty. dc_assignlib(READ,) and mp_getcols(..,.) will fail, but the mp_abort that catches this only runs aftermp_applyformats — several steps later. While &syscc will be set and eventually caught, adding an explicit abort immediately after the data _null_ step (or after dc_assignlib) would provide a clearer error message and prevent confusing intermediate errors.
W4 — addRow() with empty dataSource passes index - 1 to hot.alter() (editor.component.ts:1444-1452)
constnewIndex=this.dataSource.length// 0 if empty
hot.alter('insert_row_below',newIndex-1,1)// insert_row_below at -1
this.dataSource[newIndex].noLinkOption=true// dataSource[0]
When dataSource.length === 0, newIndex - 1 is -1. Handsontable's alter() does treat -1 as "insert at the end" which works, but this.dataSource[newIndex] immediately after assumes alter() synchronously spliced into dataSource. If the formulas plugin's beforeCreateRow hook interferes or the data binding is async, this could throw. The beforeCreateRow hook does check addingNewRow (set to true before), so it should be fine in practice — but this is fragile and worth a defensive guard.
Suggestions
S1 — buildColInfoHtml does not HTML-escape formulaValue (col-info-html.ts:35)
html+=`<br>√x=${formula}`
formulaValue comes from RULE_VALUE in MPE_VALIDATIONS, which is admin-controlled. However, the same function already interpolates colName, hardRegexValue, etc. without escaping, so this is consistent with existing behavior. Still, if any of these values contain < or >, they would be interpreted as HTML. Consider escaping all interpolated values in this function in a future cleanup.
S2 — getstagetable.sas — no mp_abort if base_lib/base_ds are empty after the DATA step
Related to W3. A simple guard would be:
%mp_abort(iftrue= (&base_lib= or&base_ds=)
,mac=&_program..sas
,msg=%str(No mpe_submit record found for table_id=&table_id)
)
S3 — Migration script 20260807_v7.13_release.sas — max(selectbox_rk) on empty table
If mpe_selectbox were ever empty, &maxrk resolves to missing (.), and &maxrk+1 is also missing. The INSERT would then set selectbox_rk=.. This is unlikely in a running DC instance (the table is always populated), but mpe_makedata.sas uses %mf_increment(rk) which handles this more robustly. Not a real-world issue, just a consistency note.
S4 — detectLicenceKeyProtocolMismatch — heuristic-based detection is fragile
The protocol mismatch detection relies on the assumption that HTTPS-format keys always have different licence/activation values and HTTP-format keys always have identical values. This is a structural assumption about the key generation process. If the key generation logic ever changes (e.g. HTTP keys also get distinct values), this detection would silently produce false positives. Worth a comment in the key generation code pointing to this dependency, or a more robust detection method.
S5 — mpe_targetloader.sas CAS branch — dcsession is referenced without being established in this macro
data &libds (append=yes) / sessref=dcsession;
set casuser.&tmpds;
run;
The dcsession CAS session is presumably established upstream (in the calling service). If it's not, this will fail with a confusing error. This is pre-existing convention, but a comment noting the prerequisite would help maintainers.
Looks Good
Utility extraction: classifyRow, expandCellRanges, findOverwrittenCells, getRevertableCols, getStableFormulaBaseCols, parseFormulaRule, etc. are all well-separated pure functions with thorough unit tests. The test coverage is excellent — each utility has spec files covering edge cases (composite keys, reverse drag selection, null values, loosely-equal comparisons).
EDIT_STATUS column design: Using dc.row_status (with a period) as a client-only column name that can never collide with a real SAS variable is a clever and safe approach. The dataDotNotation: false setting and the withoutEditStatus stripping before backend submission are correctly handled.
Cell revert feature: The comment-based "original value" tracking is well-thought-out. PK-matched comparison via findOverwrittenCells correctly handles row reordering. The live afterChange sync keeps comments and EDIT_STATUS up to date.
SAS changes: The mp_abort(mode=INCLUDE) additions in restore.sas, stagedata.sas, postdata.sas, getdata.sas, etc. correctly replace the deprecated mf_abort.error file-check pattern with the modern @sasjs/core approach. The CAS REPLACE branch in mpe_targetloader.sas with varchar casting and deleteRows truncation is well-structured with appropriate mp_abort guards before destructive operations.
Viewbox resize: The migration from CDK cdkDrag to native pointer events for resize handles is a solid improvement. The endActiveResize cleanup in ngOnDestroy prevents listener leaks. The resizeFromLeftEdge read-back of actual width (to respect CSS min-width) is a nice touch.
Licence protocol mismatch: Clean implementation with good unit test coverage. The use of window.crypto.subtle availability (rather than location.protocol) correctly handles localhost.
Cypress test coverage: 30+ new editor tests covering HARDFORMULA/SOFTFORMULA, row insert with formulas, DC.* variables, revert scenarios (single cell, range, column, mixed), row header symbols, sorting alignment, and submission. Stage and licensing tests also added.
Reviewed by Hermes Agent
## Hermes Agent Code Review
**Verdict: Comment** — no blocking issues found; a few warnings and suggestions below.
This is a large, well-structured release PR (94 files, +7189/-3009). The new formula-based DQ rules feature is the core change, with supporting cell revert, formatted/unformatted staging toggle, viewbox resize on all edges, and licence-key protocol mismatch warning. The extraction of logic into small, well-tested utility functions is good practice.
---
### Critical
None.
---
### Warnings
**W1 — `quoteLiteral` does not escape embedded double quotes (parseFormulaRule.ts:38-39)**
```ts
const quoteLiteral = (value: string | number | undefined): string =>
`"${value ?? ''}"`
```
When `DC.USER_NAME` or `DC.ORIG_VALUE` contains a double quote character (e.g. a username like `john"doe` or a cell value with `"`), the resulting formula string will have an unescaped quote inside a quoted literal, producing a malformed HyperFormula expression. This could cause the formula to silently fail to evaluate, or in a worst case, alter the formula's semantics. The values come from SAS data / user session, so while admin-controlled in most deployments, a user with a quote in their name is plausible.
**Suggested fix:**
```ts
const quoteLiteral = (value: string | number | undefined): string =>
`"${String(value ?? '').replace(/"/g, '""')}"`
```
**W2 — `Number(rawValueText)` can produce `NaN` in revert callback (editor.component.ts:96-100)**
```ts
hot.setDataAtRowProp(
row,
prop,
isNumericCol ? Number(rawValueText) : rawValueText
)
```
If a numeric column's original value was stored in the comment as a non-numeric string (unlikely but possible if the comment was somehow corrupted or the raw SAS value was a special missing like `.S`), `Number(rawValueText)` returns `NaN`, which would then be written into the cell. There's no guard against this. Consider falling back to the raw text or validating before conversion.
**W3 — Missing `mp_abort` between `dc_assignlib` and `mp_getcols` in getstagetable.sas (lines 48-52)**
```sas
%dc_assignlib(READ,&base_lib)
%mp_getcols(&base_lib..&base_ds,outds=work.basecols)
```
If `mpe_submit` has no row matching `&table_id` (e.g. stale table_id, race condition), `&base_lib` and `&base_ds` remain empty. `dc_assignlib(READ,)` and `mp_getcols(..,.)` will fail, but the `mp_abort` that catches this only runs *after* `mp_applyformats` — several steps later. While `&syscc` will be set and eventually caught, adding an explicit abort immediately after the `data _null_` step (or after `dc_assignlib`) would provide a clearer error message and prevent confusing intermediate errors.
**W4 — `addRow()` with empty `dataSource` passes `index - 1` to `hot.alter()` (editor.component.ts:1444-1452)**
```ts
const newIndex = this.dataSource.length // 0 if empty
hot.alter('insert_row_below', newIndex - 1, 1) // insert_row_below at -1
this.dataSource[newIndex].noLinkOption = true // dataSource[0]
```
When `dataSource.length === 0`, `newIndex - 1` is `-1`. Handsontable's `alter()` does treat `-1` as "insert at the end" which works, but `this.dataSource[newIndex]` immediately after assumes `alter()` synchronously spliced into `dataSource`. If the formulas plugin's `beforeCreateRow` hook interferes or the data binding is async, this could throw. The `beforeCreateRow` hook does check `addingNewRow` (set to `true` before), so it should be fine in practice — but this is fragile and worth a defensive guard.
---
### Suggestions
**S1 — `buildColInfoHtml` does not HTML-escape `formulaValue` (col-info-html.ts:35)**
```ts
html += `<br>√x=${formula}`
```
`formulaValue` comes from `RULE_VALUE` in `MPE_VALIDATIONS`, which is admin-controlled. However, the same function already interpolates `colName`, `hardRegexValue`, etc. without escaping, so this is consistent with existing behavior. Still, if any of these values contain `<` or `>`, they would be interpreted as HTML. Consider escaping all interpolated values in this function in a future cleanup.
**S2 — `getstagetable.sas` — no `mp_abort` if `base_lib`/`base_ds` are empty after the DATA step**
Related to W3. A simple guard would be:
```sas
%mp_abort(iftrue= (&base_lib= or &base_ds=)
,mac=&_program..sas
,msg=%str(No mpe_submit record found for table_id=&table_id)
)
```
**S3 — Migration script `20260807_v7.13_release.sas` — `max(selectbox_rk)` on empty table**
```sas
select max(selectbox_rk) into: maxrk
from &dclib..mpe_selectbox;
```
If `mpe_selectbox` were ever empty, `&maxrk` resolves to missing (`.`), and `&maxrk+1` is also missing. The INSERT would then set `selectbox_rk=.`. This is unlikely in a running DC instance (the table is always populated), but `mpe_makedata.sas` uses `%mf_increment(rk)` which handles this more robustly. Not a real-world issue, just a consistency note.
**S4 — `detectLicenceKeyProtocolMismatch` — heuristic-based detection is fragile**
The protocol mismatch detection relies on the assumption that HTTPS-format keys always have different licence/activation values and HTTP-format keys always have identical values. This is a structural assumption about the key generation process. If the key generation logic ever changes (e.g. HTTP keys also get distinct values), this detection would silently produce false positives. Worth a comment in the key generation code pointing to this dependency, or a more robust detection method.
**S5 — `mpe_targetloader.sas` CAS branch — `dcsession` is referenced without being established in this macro**
```sas
data &libds (append=yes) / sessref=dcsession;
set casuser.&tmpds;
run;
```
The `dcsession` CAS session is presumably established upstream (in the calling service). If it's not, this will fail with a confusing error. This is pre-existing convention, but a comment noting the prerequisite would help maintainers.
---
### Looks Good
- **Utility extraction**: classifyRow, expandCellRanges, findOverwrittenCells, getRevertableCols, getStableFormulaBaseCols, parseFormulaRule, etc. are all well-separated pure functions with thorough unit tests. The test coverage is excellent — each utility has spec files covering edge cases (composite keys, reverse drag selection, null values, loosely-equal comparisons).
- **EDIT_STATUS column design**: Using `dc.row_status` (with a period) as a client-only column name that can never collide with a real SAS variable is a clever and safe approach. The `dataDotNotation: false` setting and the `withoutEditStatus` stripping before backend submission are correctly handled.
- **Cell revert feature**: The comment-based "original value" tracking is well-thought-out. PK-matched comparison via `findOverwrittenCells` correctly handles row reordering. The live `afterChange` sync keeps comments and EDIT_STATUS up to date.
- **SAS changes**: The `mp_abort(mode=INCLUDE)` additions in restore.sas, stagedata.sas, postdata.sas, getdata.sas, etc. correctly replace the deprecated `mf_abort.error` file-check pattern with the modern `@sasjs/core` approach. The CAS REPLACE branch in `mpe_targetloader.sas` with varchar casting and `deleteRows` truncation is well-structured with appropriate `mp_abort` guards before destructive operations.
- **Viewbox resize**: The migration from CDK `cdkDrag` to native pointer events for resize handles is a solid improvement. The `endActiveResize` cleanup in `ngOnDestroy` prevents listener leaks. The `resizeFromLeftEdge` read-back of actual width (to respect CSS `min-width`) is a nice touch.
- **Licence protocol mismatch**: Clean implementation with good unit test coverage. The use of `window.crypto.subtle` availability (rather than `location.protocol`) correctly handles localhost.
- **Cypress test coverage**: 30+ new editor tests covering HARDFORMULA/SOFTFORMULA, row insert with formulas, DC.* variables, revert scenarios (single cell, range, column, mixed), row header symbols, sorting alignment, and submission. Stage and licensing tests also added.
---
Reviewed by Hermes Agent
If the comment text after the prefix is not a valid number (e.g. a SAS special missing like .S), Number() returns NaN which gets written into the cell. Consider a guard:
**W2 — `Number(rawValueText)` can produce `NaN`**
If the comment text after the prefix is not a valid number (e.g. a SAS special missing like `.S`), `Number()` returns `NaN` which gets written into the cell. Consider a guard:
```ts
const numericValue = Number(rawValueText)
hot.setDataAtRowProp(
row, prop,
isNaN(numericValue) ? rawValueText : numericValue
)
```
When dataSource.length === 0, newIndex - 1 is -1. Handsontable treats this as "insert at end" so it works, but this.dataSource[newIndex] immediately after assumes alter() synchronously spliced the row. A defensive guard (if (this.dataSource[newIndex])) would prevent a crash if the data binding ever becomes async.
**W4 — `addRow()` with empty `dataSource`**
When `dataSource.length === 0`, `newIndex - 1` is `-1`. Handsontable treats this as "insert at end" so it works, but `this.dataSource[newIndex]` immediately after assumes `alter()` synchronously spliced the row. A defensive guard (`if (this.dataSource[newIndex])`) would prevent a crash if the data binding ever becomes async.
W1 — quoteLiteral does not escape embedded double quotes
If DC.USER_NAME or DC.ORIG_VALUE contains a " character, the resulting formula string will have an unescaped quote inside a quoted literal, producing a malformed HyperFormula expression.
**W1 — `quoteLiteral` does not escape embedded double quotes**
If `DC.USER_NAME` or `DC.ORIG_VALUE` contains a `"` character, the resulting formula string will have an unescaped quote inside a quoted literal, producing a malformed HyperFormula expression.
**Suggested fix:**
```ts
const quoteLiteral = (value: string | number | undefined): string =>
`"${String(value ?? '').replace(/"/g, '""')}"`
```
W3 — Missing mp_abort between dc_assignlib and mp_getcols
If mpe_submit has no row matching &table_id, &base_lib/&base_ds remain empty. The abort that catches this only runs after mp_applyformats — several steps later. Consider adding an explicit guard here:
%mp_abort(iftrue= (&base_lib= or&base_ds=)
,mac=&_program..sas
,msg=%str(No mpe_submit record found for table_id=&table_id)
)
**W3 — Missing `mp_abort` between `dc_assignlib` and `mp_getcols`**
If `mpe_submit` has no row matching `&table_id`, `&base_lib`/`&base_ds` remain empty. The abort that catches this only runs after `mp_applyformats` — several steps later. Consider adding an explicit guard here:
```sas
%mp_abort(iftrue= (&base_lib= or &base_ds=)
,mac=&_program..sas
,msg=%str(No mpe_submit record found for table_id=&table_id)
)
```
This commit (067c087 - "chore: adding additional mp_abort per per review") addresses prior review feedback: it adds a null-check guard to getstagetable.sas for the case where no mpe_submit record is found for the given table_id.
Critical
None.
Warnings
None.
Suggestions
getstagetable.sas lines 45-48 - guard placement and syscc interaction
The data step that reads mpe_submit (lines 39-44) could set &syscc to a non-zero value if it encounters an error (e.g. mpe_submit locked or missing), in which case the existing mp_abort at line 29 (checking &syscc ne 0 after the stagetable load) would already have fired. However, if mpe_submit exists but simply has no matching TABLE_ID row, &syscc stays 0 and base_lib/base_ds remain empty - which is exactly the scenario this new guard catches. The guard is correctly placed after the data step and beforedc_assignlib(READ,&base_lib) (line 50), which would otherwise fail or behave unpredictably with an empty libref. Good defensive addition.
One minor consideration: the error message No mpe_submit record found for table_id=&table_id will surface to the end user via mp_abort. Confirm this is the desired UX (exposing the internal table_id value) vs. a more generic message. This is a stylistic choice, not a defect.
Looks Good
The guard prevents dc_assignlib from being called with an empty &base_lib, which would produce a confusing low-level SAS error rather than a clear abort message.
mp_abort is already in the file's documented @li macro list (line 10), so no doc header update is needed.
The mac=&_program..sas parameter correctly identifies the service in the abort message for debugging.
Reviewed by Hermes Agent
## Hermes Agent Code Review
**Verdict: Approve**
This commit (`067c087` - "chore: adding additional mp_abort per per review") addresses prior review feedback: it adds a null-check guard to `getstagetable.sas` for the case where no `mpe_submit` record is found for the given `table_id`.
### Critical
None.
### Warnings
None.
### Suggestions
**getstagetable.sas lines 45-48 - guard placement and syscc interaction**
The data step that reads `mpe_submit` (lines 39-44) could set `&syscc` to a non-zero value if it encounters an error (e.g. `mpe_submit` locked or missing), in which case the existing `mp_abort` at line 29 (checking `&syscc ne 0` after the `stagetable` load) would already have fired. However, if `mpe_submit` exists but simply has no matching `TABLE_ID` row, `&syscc` stays 0 and `base_lib`/`base_ds` remain empty - which is exactly the scenario this new guard catches. The guard is correctly placed *after* the data step and *before* `dc_assignlib(READ,&base_lib)` (line 50), which would otherwise fail or behave unpredictably with an empty libref. Good defensive addition.
One minor consideration: the error message `No mpe_submit record found for table_id=&table_id` will surface to the end user via `mp_abort`. Confirm this is the desired UX (exposing the internal `table_id` value) vs. a more generic message. This is a stylistic choice, not a defect.
### Looks Good
- The guard prevents `dc_assignlib` from being called with an empty `&base_lib`, which would produce a confusing low-level SAS error rather than a clear abort message.
- `mp_abort` is already in the file's documented `@li` macro list (line 10), so no doc header update is needed.
- The `mac=&_program..sas` parameter correctly identifies the service in the abort message for debugging.
Reviewed by Hermes Agent
The error message exposes the internal table_id value to the end user via mp_abort. Consider whether a more generic message (e.g. "staged table could not be located") is preferable for end-user UX, or if exposing table_id is intentional for support/debugging purposes. Stylistic choice, not a defect.
The error message exposes the internal `table_id` value to the end user via `mp_abort`. Consider whether a more generic message (e.g. "staged table could not be located") is preferable for end-user UX, or if exposing `table_id` is intentional for support/debugging purposes. Stylistic choice, not a defect.
Coverage measured via npx @sasjs/cli c -t server-ci against the server-ci target.
Category
Covered
Total
Percentage
Services
20
60
33%
Macros
12
42
29%
Overall
32
102
31%
The compile step completed successfully (server-ci target). A non-fatal warning was emitted about webSourcePath: client/dist not existing - expected in this SAS-only checkout context and does not affect coverage measurement.
Note: This PR is primarily a client-side (Angular) release. The SAS-side change in this commit is a single defensive mp_abort guard added to getstagetable.sas. The coverage figures above reflect the SAS test suite only; client-side Karma/Cypress coverage is run separately in CI.
Generated by Hermes Agent
## Test Coverage Report
Coverage measured via `npx @sasjs/cli c -t server-ci` against the `server-ci` target.
| Category | Covered | Total | Percentage |
|----------|---------|-------|------------|
| Services | 20 | 60 | 33% |
| Macros | 12 | 42 | 29% |
| **Overall** | **32** | **102** | **31%** |
The compile step completed successfully (`server-ci` target). A non-fatal warning was emitted about `webSourcePath: client/dist` not existing - expected in this SAS-only checkout context and does not affect coverage measurement.
Note: This PR is primarily a client-side (Angular) release. The SAS-side change in this commit is a single defensive `mp_abort` guard added to `getstagetable.sas`. The coverage figures above reflect the SAS test suite only; client-side Karma/Cypress coverage is run separately in CI.
Generated by Hermes Agent
Coverage measured via npx @sasjs/cli c -t server-ci against the server-ci target.
Category
Covered
Total
Percentage
Services
22
60
37%
Macros
12
42
29%
Overall
34
102
33%
The compile completed successfully. The streamWeb web-app compile step errored (client/dist does not exist in this review environment — the Angular client was not built), but this does not affect the SAS service/macro coverage figures above.
Notes
Coverage increased slightly from the previous review (Services 33%→37%, Overall 31%→33%), reflecting the two new test files added in the core v5 bump: exportdb.test.sas and refreshcatalog.test.sas.
The core v5 migration renamed mp_testservice→mp_execute and mp_getddl→mp_ds2ddl; all test files were updated accordingly.
Generated by Hermes Agent
## Test Coverage Report
Coverage measured via `npx @sasjs/cli c -t server-ci` against the `server-ci` target.
| Category | Covered | Total | Percentage |
|----------|---------|-------|------------|
| Services | 22 | 60 | 37% |
| Macros | 12 | 42 | 29% |
| **Overall** | **34** | **102** | **33%** |
The compile completed successfully. The `streamWeb` web-app compile step errored (`client/dist` does not exist in this review environment — the Angular client was not built), but this does not affect the SAS service/macro coverage figures above.
### Notes
- Coverage increased slightly from the previous review (Services 33%→37%, Overall 31%→33%), reflecting the two new test files added in the core v5 bump: `exportdb.test.sas` and `refreshcatalog.test.sas`.
- The core v5 migration renamed `mp_testservice`→`mp_execute` and `mp_getddl`→`mp_ds2ddl`; all test files were updated accordingly.
Generated by Hermes Agent
This review covers the new commits since the last review (067c087→9b2e0df): the @sasjs/core v4→v5 bump (a breaking-change migration) and two new SAS test files.
Summary of new changes
@sasjs/core 4.68.3 → 5.0.0: Updated package.json and package-lock.json.
mp_testservice → mp_execute: All 14 test files migrated. mp_execute is a thin wrapper around mx_testservice (verified in core v5 source) with an identical parameter signature — the calls work as drop-in replacements.
mp_getddl → mp_ds2ddl: Two service files (exportdb.sas, getddl.sas) updated, with the new showlog=NO parameter added to suppress log output in web-service context.
mp_coretable → mddl_dc_*: testsetup.sas now uses mddl_dc_locktable/mddl_dc_difftable (verified to exist in core v5 with the libds= parameter). Cleaned up testinit.sas (removed stale comments).
New test files: exportdb.test.sas (74 lines, tests PGSQL + SAS flavours) and refreshcatalog.test.sas (58 lines, tests catalog refresh + verification).
Checks performed
✅ No stale mp_testservice, mp_getddl, or mp_coretable references remain anywhere in sas/sasjs/.
✅ All replacement macros (mp_execute, mp_ds2ddl, mddl_dc_locktable, mddl_dc_difftable) exist in @sasjs/core@5.0.0.
✅mp_execute parameter signature matches the old mp_testservice calls — inputfiles, inputdatasets, outlib, outref, viyacontext, viyaresult, mdebug all carried over correctly.
✅mp_ds2ddl calls preserved the fref=, flavour=, schema=, applydttm= parameters and added showlog=NO.
✅ Compile (npx @sasjs/cli c -t server-ci) succeeds; coverage increased from 31%→33% overall thanks to the two new test files.
✅ The new test files follow the existing pattern (%mx_testservice + %mp_assertdsobs), are well-structured, and test both positive and edge cases.
No issues found
The migration is mechanical, complete, and correct. No security, correctness, or style concerns in the new commits.
Reviewed by Hermes Agent
## Hermes Agent Code Review
**Verdict: Approve**
This review covers the new commits since the last review (`067c087`→`9b2e0df`): the `@sasjs/core` v4→v5 bump (a breaking-change migration) and two new SAS test files.
### Summary of new changes
- **`@sasjs/core` 4.68.3 → 5.0.0**: Updated `package.json` and `package-lock.json`.
- **`mp_testservice` → `mp_execute`**: All 14 test files migrated. `mp_execute` is a thin wrapper around `mx_testservice` (verified in core v5 source) with an identical parameter signature — the calls work as drop-in replacements.
- **`mp_getddl` → `mp_ds2ddl`**: Two service files (`exportdb.sas`, `getddl.sas`) updated, with the new `showlog=NO` parameter added to suppress log output in web-service context.
- **`mp_coretable` → `mddl_dc_*`**: `testsetup.sas` now uses `mddl_dc_locktable`/`mddl_dc_difftable` (verified to exist in core v5 with the `libds=` parameter). Cleaned up `testinit.sas` (removed stale comments).
- **New test files**: `exportdb.test.sas` (74 lines, tests PGSQL + SAS flavours) and `refreshcatalog.test.sas` (58 lines, tests catalog refresh + verification).
### Checks performed
- ✅ No stale `mp_testservice`, `mp_getddl`, or `mp_coretable` references remain anywhere in `sas/sasjs/`.
- ✅ All replacement macros (`mp_execute`, `mp_ds2ddl`, `mddl_dc_locktable`, `mddl_dc_difftable`) exist in `@sasjs/core@5.0.0`.
- ✅ `mp_execute` parameter signature matches the old `mp_testservice` calls — `inputfiles`, `inputdatasets`, `outlib`, `outref`, `viyacontext`, `viyaresult`, `mdebug` all carried over correctly.
- ✅ `mp_ds2ddl` calls preserved the `fref=`, `flavour=`, `schema=`, `applydttm=` parameters and added `showlog=NO`.
- ✅ Compile (`npx @sasjs/cli c -t server-ci`) succeeds; coverage increased from 31%→33% overall thanks to the two new test files.
- ✅ The new test files follow the existing pattern (`%mx_testservice` + `%mp_assertdsobs`), are well-structured, and test both positive and edge cases.
### No issues found
The migration is mechanical, complete, and correct. No security, correctness, or style concerns in the new commits.
Reviewed by Hermes Agent
- Escape embedded double quotes in DC.USER_NAME/DC.ORIG_VALUE formula
literals, preventing malformed HyperFormula expressions
- Extract cell-revert numeric parsing into resolveRevertedCellValue,
falling back to the raw text instead of writing NaN
- Guard addRow() against an empty dataSource before indexing into it
- Document the cross-repo assumption behind licence protocol-mismatch
detection
This review covers the new commits since the last review (9b2e0df→bf54a58): commit 36963aa "fix: address hermes review feedback (formula quoting, cell revert, addRow guard)" and its merge.
Summary of new changes
The incremental commits address three pieces of prior review feedback:
Formula literal quoting fix (parseFormulaRule.ts): quoteLiteral now escapes embedded double-quotes (" → "") so a DC.USER_NAME/DC.ORIG_VALUE containing a " produces a valid spreadsheet literal instead of a broken formula. Two new spec cases cover john"doe and a "quoted" value. Correct and well-tested.
Cell revert special-missing handling (resolveRevertedCellValue.ts, new): Reverting a numeric column now falls back to the raw text when Number(rawValueText) is NaN (e.g. a SAS special missing like .S), instead of silently writing NaN into the cell. The editor's revert callback now calls this helper instead of the inline isNumericCol ? Number(rawValueText) : rawValueText. Three new spec cases cover non-numeric, valid numeric, and special-missing. Clean extraction.
addRow guard (editor.component.ts): After hot.alter('insert_row_below', ...) the this.dataSource[newIndex].noLinkOption = true assignment is now guarded with if (this.dataSource[newIndex]), defending against the data binding ever becoming async. Defensive and correct.
Doc note (detectLicenceKeyProtocolMismatch.ts): Added a comment documenting that the HTTP/HTTPS detection is a structural assumption about the (separate) key-generation repo. Good documentation of a cross-repo coupling.
Checks performed
✅quoteLiteral escaping is correct: String(value ?? '').replace(/"/g,'""') produces valid ""-escaped literals, matching the two new specs.
✅resolveRevertedCellValue uses Number.isNaN() (not loose isNaN) — correctly distinguishes NaN from numeric coercion quirks.
✅ The revert path in editor.component.ts passes the same rawValueText/isNumericCol the inline code did — no behavioural regression.
✅ The addRow guard preserves the existing seedFormulaValuesForRow/updateEditStatusForRow calls that follow it.
✅ SAS compile (npx @sasjs/cli c -t server-ci) succeeds; overall coverage holds at 33% (34/102). See the separate coverage comment.
Observations (non-blocking)
One low-probability regex edge case in parseFormulaRule's quoted-span splitter (see inline comment) — only relevant if a rule author hand-types "" escapes in RULE_VALUE.
The PK-matching pattern (find inside forEach) in applyFormulaRules, findOverwrittenCells, and classifyRow is O(rows²) per call. Fine for typical editor row counts; worth a Map-based lookup if very large staging tables ever become a supported use case.
No blocking issues
The feedback-fix commits are correct, tested, and don't introduce regressions. Approving.
Reviewed by Hermes Agent
## Hermes Agent Code Review
**Verdict: Approve**
This review covers the new commits since the last review (`9b2e0df`→`bf54a58`): commit `36963aa` "fix: address hermes review feedback (formula quoting, cell revert, addRow guard)" and its merge.
### Summary of new changes
The incremental commits address three pieces of prior review feedback:
1. **Formula literal quoting fix** (`parseFormulaRule.ts`): `quoteLiteral` now escapes embedded double-quotes (`"` → `""`) so a `DC.USER_NAME`/`DC.ORIG_VALUE` containing a `"` produces a valid spreadsheet literal instead of a broken formula. Two new spec cases cover `john"doe` and `a "quoted" value`. Correct and well-tested.
2. **Cell revert special-missing handling** (`resolveRevertedCellValue.ts`, new): Reverting a numeric column now falls back to the raw text when `Number(rawValueText)` is `NaN` (e.g. a SAS special missing like `.S`), instead of silently writing `NaN` into the cell. The editor's revert callback now calls this helper instead of the inline `isNumericCol ? Number(rawValueText) : rawValueText`. Three new spec cases cover non-numeric, valid numeric, and special-missing. Clean extraction.
3. **addRow guard** (`editor.component.ts`): After `hot.alter('insert_row_below', ...)` the `this.dataSource[newIndex].noLinkOption = true` assignment is now guarded with `if (this.dataSource[newIndex])`, defending against the data binding ever becoming async. Defensive and correct.
4. **Doc note** (`detectLicenceKeyProtocolMismatch.ts`): Added a comment documenting that the HTTP/HTTPS detection is a structural assumption about the (separate) key-generation repo. Good documentation of a cross-repo coupling.
### Checks performed
- ✅ `quoteLiteral` escaping is correct: `String(value ?? '').replace(/"/g,'""')` produces valid `""`-escaped literals, matching the two new specs.
- ✅ `resolveRevertedCellValue` uses `Number.isNaN()` (not loose `isNaN`) — correctly distinguishes `NaN` from numeric coercion quirks.
- ✅ The revert path in `editor.component.ts` passes the same `rawValueText`/`isNumericCol` the inline code did — no behavioural regression.
- ✅ The addRow guard preserves the existing `seedFormulaValuesForRow`/`updateEditStatusForRow` calls that follow it.
- ✅ SAS compile (`npx @sasjs/cli c -t server-ci`) succeeds; overall coverage holds at 33% (34/102). See the separate coverage comment.
### Observations (non-blocking)
- One low-probability regex edge case in `parseFormulaRule`'s quoted-span splitter (see inline comment) — only relevant if a rule author hand-types `""` escapes in `RULE_VALUE`.
- The PK-matching pattern (`find` inside `forEach`) in `applyFormulaRules`, `findOverwrittenCells`, and `classifyRow` is O(rows²) per call. Fine for typical editor row counts; worth a Map-based lookup if very large staging tables ever become a supported use case.
### No blocking issues
The feedback-fix commits are correct, tested, and don't introduce regressions. Approving.
Reviewed by Hermes Agent
Performance note (non-blocking): the dataSourceUnchanged.find(...) inside the dataSource.forEach + for (const formulaRule ...) loop is O(rows × formulaRules × unchangedRows). The same PK-matching find-inside-forEach pattern appears in findOverwrittenCells.ts and classifyRow.ts. Fine for typical editor row counts; if very large staging tables become a supported use case, a one-time Map keyed by the joined PK values would drop this to O(rows × formulaRules).
Performance note (non-blocking): the `dataSourceUnchanged.find(...)` inside the `dataSource.forEach` + `for (const formulaRule ...)` loop is O(rows × formulaRules × unchangedRows). The same PK-matching `find`-inside-`forEach` pattern appears in `findOverwrittenCells.ts` and `classifyRow.ts`. Fine for typical editor row counts; if very large staging tables become a supported use case, a one-time `Map` keyed by the joined PK values would drop this to O(rows × formulaRules).
Low-priority edge case: the quotedSpanPattern regex "[^"]*"|'[^']*' does not handle escaped double-quotes ("") inside a double-quoted span — it stops at the first inner ", so a literal like ="john""doe" is split into two matches ("john" and "doe") rather than one span.
This is low-impact because the splitter runs on the ORIGINAL rule text (before quoteLiteral produces "" escapes), so it only bites if a rule author literally types a "" escape in RULE_VALUE alongside a DC.* token. If it ever matters, /(?:"(?:""|[^"])*"|'[^']*')/g would handle the escape.
Low-priority edge case: the `quotedSpanPattern` regex `"[^"]*"|'[^']*'` does not handle escaped double-quotes (`""`) inside a double-quoted span — it stops at the first inner `"`, so a literal like `="john""doe"` is split into two matches (`"john"` and `"doe"`) rather than one span.
This is low-impact because the splitter runs on the ORIGINAL rule text (before `quoteLiteral` produces `""` escapes), so it only bites if a rule author literally types a `""` escape in `RULE_VALUE` alongside a `DC.*` token. If it ever matters, `/(?:"(?:""|[^"])*"|'[^']*')/g` would handle the escape.
This review covers the new commits since the last review (9b2e0df→bf54a58): commit 36963aa "fix: address hermes review feedback (formula quoting, cell revert, addRow guard)" and its merge.
Summary of new changes
The incremental commits address three pieces of prior review feedback:
Formula literal quoting fix (parseFormulaRule.ts): quoteLiteral now escapes embedded double-quotes (" → "") so a DC.USER_NAME/DC.ORIG_VALUE containing a " produces a valid spreadsheet literal instead of a broken formula. Two new spec cases cover john"doe and a "quoted" value. Correct and well-tested.
Cell revert special-missing handling (resolveRevertedCellValue.ts, new): Reverting a numeric column now falls back to the raw text when Number(rawValueText) is NaN (e.g. a SAS special missing like .S), instead of silently writing NaN into the cell. The editor's revert callback now calls this helper instead of the inline isNumericCol ? Number(rawValueText) : rawValueText. Three new spec cases cover non-numeric, valid numeric, and special-missing. Clean extraction.
addRow guard (editor.component.ts): After hot.alter('insert_row_below', ...) the this.dataSource[newIndex].noLinkOption = true assignment is now guarded with if (this.dataSource[newIndex]), defending against the data binding ever becoming async. Defensive and correct.
Doc note (detectLicenceKeyProtocolMismatch.ts): Added a comment documenting that the HTTP/HTTPS detection is a structural assumption about the (separate) key-generation repo. Good documentation of a cross-repo coupling.
Checks performed
✅quoteLiteral escaping is correct: String(value ?? '').replace(/"/g,'""') produces valid ""-escaped literals, matching the two new specs.
✅resolveRevertedCellValue uses Number.isNaN() (not loose isNaN) — correctly distinguishes NaN from numeric coercion quirks.
✅ The revert path in editor.component.ts passes the same rawValueText/isNumericCol the inline code did — no behavioural regression.
✅ The addRow guard preserves the existing seedFormulaValuesForRow/updateEditStatusForRow calls that follow it.
✅ SAS compile (npx @sasjs/cli c -t server-ci) succeeds; overall coverage holds at 33% (34/102). See the separate coverage comment.
Observations (non-blocking)
One low-probability regex edge case in parseFormulaRule's quoted-span splitter (see inline comment) — only relevant if a rule author hand-types "" escapes in RULE_VALUE.
The PK-matching pattern (find inside forEach) in applyFormulaRules, findOverwrittenCells, and classifyRow is O(rows²) per call. Fine for typical editor row counts; worth a Map-based lookup if very large staging tables ever become a supported use case.
No blocking issues
The feedback-fix commits are correct, tested, and don't introduce regressions. Approving.
Reviewed by Hermes Agent
## Hermes Agent Code Review
**Verdict: Approve**
This review covers the new commits since the last review (`9b2e0df`→`bf54a58`): commit `36963aa` "fix: address hermes review feedback (formula quoting, cell revert, addRow guard)" and its merge.
### Summary of new changes
The incremental commits address three pieces of prior review feedback:
1. **Formula literal quoting fix** (`parseFormulaRule.ts`): `quoteLiteral` now escapes embedded double-quotes (`"` → `""`) so a `DC.USER_NAME`/`DC.ORIG_VALUE` containing a `"` produces a valid spreadsheet literal instead of a broken formula. Two new spec cases cover `john"doe` and `a "quoted" value`. Correct and well-tested.
2. **Cell revert special-missing handling** (`resolveRevertedCellValue.ts`, new): Reverting a numeric column now falls back to the raw text when `Number(rawValueText)` is `NaN` (e.g. a SAS special missing like `.S`), instead of silently writing `NaN` into the cell. The editor's revert callback now calls this helper instead of the inline `isNumericCol ? Number(rawValueText) : rawValueText`. Three new spec cases cover non-numeric, valid numeric, and special-missing. Clean extraction.
3. **addRow guard** (`editor.component.ts`): After `hot.alter('insert_row_below', ...)` the `this.dataSource[newIndex].noLinkOption = true` assignment is now guarded with `if (this.dataSource[newIndex])`, defending against the data binding ever becoming async. Defensive and correct.
4. **Doc note** (`detectLicenceKeyProtocolMismatch.ts`): Added a comment documenting that the HTTP/HTTPS detection is a structural assumption about the (separate) key-generation repo. Good documentation of a cross-repo coupling.
### Checks performed
- ✅ `quoteLiteral` escaping is correct: `String(value ?? '').replace(/"/g,'""')` produces valid `""`-escaped literals, matching the two new specs.
- ✅ `resolveRevertedCellValue` uses `Number.isNaN()` (not loose `isNaN`) — correctly distinguishes `NaN` from numeric coercion quirks.
- ✅ The revert path in `editor.component.ts` passes the same `rawValueText`/`isNumericCol` the inline code did — no behavioural regression.
- ✅ The addRow guard preserves the existing `seedFormulaValuesForRow`/`updateEditStatusForRow` calls that follow it.
- ✅ SAS compile (`npx @sasjs/cli c -t server-ci`) succeeds; overall coverage holds at 33% (34/102). See the separate coverage comment.
### Observations (non-blocking)
- One low-probability regex edge case in `parseFormulaRule`'s quoted-span splitter (see inline comment) — only relevant if a rule author hand-types `""` escapes in `RULE_VALUE`.
- The PK-matching pattern (`find` inside `forEach`) in `applyFormulaRules`, `findOverwrittenCells`, and `classifyRow` is O(rows²) per call. Fine for typical editor row counts; worth a Map-based lookup if very large staging tables ever become a supported use case.
### No blocking issues
The feedback-fix commits are correct, tested, and don't introduce regressions. Approving.
Reviewed by Hermes Agent
Performance note (non-blocking): the dataSourceUnchanged.find(...) inside the dataSource.forEach + for (const formulaRule ...) loop is O(rows × formulaRules × unchangedRows). The same PK-matching find-inside-forEach pattern appears in findOverwrittenCells.ts and classifyRow.ts. Fine for typical editor row counts; if very large staging tables become a supported use case, a one-time Map keyed by the joined PK values would drop this to O(rows × formulaRules).
Performance note (non-blocking): the `dataSourceUnchanged.find(...)` inside the `dataSource.forEach` + `for (const formulaRule ...)` loop is O(rows × formulaRules × unchangedRows). The same PK-matching `find`-inside-`forEach` pattern appears in `findOverwrittenCells.ts` and `classifyRow.ts`. Fine for typical editor row counts; if very large staging tables become a supported use case, a one-time `Map` keyed by the joined PK values would drop this to O(rows × formulaRules).
Low-priority edge case: the quotedSpanPattern regex "[^"]*"|'[^']*' does not handle escaped double-quotes ("") inside a double-quoted span — it stops at the first inner ", so a literal like ="john""doe" is split into two matches ("john" and "doe") rather than one span.
This is low-impact because the splitter runs on the ORIGINAL rule text (before quoteLiteral produces "" escapes), so it only bites if a rule author literally types a "" escape in RULE_VALUE alongside a DC.* token. If it ever matters, /(?:"(?:""|[^"])*"|'[^']*')/g would handle the escape.
Low-priority edge case: the `quotedSpanPattern` regex `"[^"]*"|'[^']*'` does not handle escaped double-quotes (`""`) inside a double-quoted span — it stops at the first inner `"`, so a literal like `="john""doe"` is split into two matches (`"john"` and `"doe"`) rather than one span.
This is low-impact because the splitter runs on the ORIGINAL rule text (before `quoteLiteral` produces `""` escapes), so it only bites if a rule author literally types a `""` escape in `RULE_VALUE` alongside a `DC.*` token. If it ever matters, `/(?:"(?:""|[^"])*"|'[^']*')/g` would handle the escape.
SASjs compile/coverage run via npx @sasjs/cli c -t server-ci (local compile only — no server required).
Scope
Coverage
Overall
34 / 102 (33%)
Services
22 / 60 (37%)
Macros
12 / 42 (29%)
Notes
✔ The project compiled successfully for SASJS using target server-ci (output in sas/sasjsbuild).
The compile reported one unrelated ERROR about streamConfig.webSourcePath: '.../client/dist' not existing — this is expected, since the Angular client was not built in this review environment, and does not affect the coverage figures above.
Overall coverage holds at 33% (34/102), consistent with the previous review. The two new test files added earlier in this PR (exportdb.test.sas, refreshcatalog.test.sas) are reflected in the 22 covered services.
Newly added mpe_targetloader.test.sas (REPLACE loadtype) is registered as a test macro; the CAS-specific branch of mpe_targetloader.sas is explicitly noted in that test as not exercised (BASE engine only).
Generated by Hermes Agent
## Test Coverage Report
SASjs compile/coverage run via `npx @sasjs/cli c -t server-ci` (local compile only — no server required).
| Scope | Coverage |
|---|---|
| **Overall** | **34 / 102 (33%)** |
| Services | 22 / 60 (37%) |
| Macros | 12 / 42 (29%) |
### Notes
- ✔ The project compiled successfully for SASJS using target `server-ci` (output in `sas/sasjsbuild`).
- The compile reported one unrelated `ERROR` about `streamConfig.webSourcePath: '.../client/dist'` not existing — this is expected, since the Angular client was not built in this review environment, and does not affect the coverage figures above.
- Overall coverage holds at 33% (34/102), consistent with the previous review. The two new test files added earlier in this PR (`exportdb.test.sas`, `refreshcatalog.test.sas`) are reflected in the 22 covered services.
- Newly added `mpe_targetloader.test.sas` (REPLACE loadtype) is registered as a test macro; the CAS-specific branch of `mpe_targetloader.sas` is explicitly noted in that test as not exercised (BASE engine only).
Generated by Hermes Agent
- sasjs-configurator: rewrite _debug=131 comment to make explicit that
debug is ALWAYS enabled for the makedata service (runs once during
deployment, debug output always wanted for diagnostics). Remove the
commented-out debug-off line that caused reviewer confusion.
- viya-compute-context-details.model: extend Attributes with
allowXCMD, sessionInactiveTimeout (number), and make all attribute
fields optional. Add Environment interface with autoExecLines string
array. Add environment? to ComputeContextDetails (optional, not all
contexts expose it).
- automatic.component: rewrite extractRunAs to use
attributes.runServerAs directly per the actual Viya API response
shape. Remove all any casts and the non-existent runAsUserId /
environment.runAsUserId field probes.
symget('_contextname') is concatenated directly into the PRX replacement
string in makedata.sas. A context name containing \ or $ would be
interpreted as a backreference or variable interpolation in the Perl
regex replacement. Escape both characters with a leading backslash in a
data step before embedding the value in the prxchange call.
This review covers the 8 new commits since the last review (bf54a58→2dfeb02): the Viya deploy configurator rewrite (df2027d), the regex-metacharacter escaping fix in makedata.sas (9023eb2), the @sasjs/core major bump with mx_testservice/mp_execute→mx_execute test renames (6527c10 + merge), debug-comment clarification and model extension (72484ae), and the packages/deploy chore (2dfeb02).
Critical
None.
Warnings / Suggestions
prxchange delimiter not escaped — makedata.sas:236. The \ and $ escaping is correct and closes the backreference/interpolation gap, but the substitution uses | as its delimiter and a context name containing a literal | would terminate the replacement early. Low probability (Viya context names rarely contain |), but see the inline comment for a one-line fix.
Method call in @for — automatic.component.html:218. getFilteredComputeContexts() is invoked on every change-detection cycle. Binding to a cached filteredComputeContexts array (updated in the existing handler methods) avoids a fresh filter allocation each tick. See inline comment.
Internal host in config — sasjsconfig.json:146. The new nextviya target hardcodes https://nextviya.emea.sas.com and /Users/viyademo18/dc. Other targets already hardcode sas.4gl.io, so this follows the existing pattern, but it does expose an internal SAS demo host. Confirm this target is intended to ship in the public repo (it's referenced by a nextviya npm script in package.json).
Looks Good
Function-replacement for $ (automatic.component.ts:643): String.replace now uses a replacer function () => \contextname="${computeContextName}"`soin the context name is treated literally. Correct fix for the prior string-replacement bug where&/$1/$`patterns would mangle names containing$`.
getComputeContextDetails parallel fetch (automatic.component.ts:167): fetches each context's full details via Promise.all, with a graceful error branch that resolves runAs: null, reusable: false instead of rejecting. Good resilience — one unfetchable context won't block the whole dropdown.
getMemberAdminGroups fallback (automatic.component.ts:351): on memberships-fetch failure, falls back to showing all groups. Sensible degraded behaviour.
onComputeContextChange ordering note (automatic.component.ts:405): correctly synchronises selectedComputeContext before deriving runningAsUser, with a clear comment explaining the ngModelChange timing. The runningAsUser will now match the dropdown on the same change, not lag by one.
ComputeContextDetails model (viya-compute-context-details.model.ts): attributes fields made optional with ? and allowXCMD/sessionInactiveTimeout added, plus a new Environment.autoExecLines. Aligns with the real Viya API which doesn't always populate every attribute.
getCurrentUserGroupMemberships (sas-viya.service.ts:174): clean addition following the existing getAdminGroups pattern; limit is a number default, not user input, so no injection surface.
Test macro renames (~30 .test.sas files + testsetup.sas + buildtermviya.sas): mechanical mx_testservice/mp_execute→mx_execute rename driven by the @sasjs/core 5.0→5.1 bump. All renames are consistent.
SAS compile (npx @sasjs/cli c -t server-ci) succeeds; overall coverage holds at 33% (34/102). See the separate coverage comment.
Reviewed by Hermes Agent
## Hermes Agent Code Review
**Verdict: Approve**
This review covers the 8 new commits since the last review (`bf54a58`→`2dfeb02`): the Viya deploy configurator rewrite (`df2027d`), the regex-metacharacter escaping fix in `makedata.sas` (`9023eb2`), the `@sasjs/core` major bump with `mx_testservice`/`mp_execute`→`mx_execute` test renames (`6527c10` + merge), debug-comment clarification and model extension (`72484ae`), and the packages/deploy chore (`2dfeb02`).
### Critical
None.
### Warnings / Suggestions
- **`prxchange` delimiter not escaped** — `makedata.sas:236`. The `\` and `$` escaping is correct and closes the backreference/interpolation gap, but the substitution uses `|` as its delimiter and a context name containing a literal `|` would terminate the replacement early. Low probability (Viya context names rarely contain `|`), but see the inline comment for a one-line fix.
- **Method call in `@for`** — `automatic.component.html:218`. `getFilteredComputeContexts()` is invoked on every change-detection cycle. Binding to a cached `filteredComputeContexts` array (updated in the existing handler methods) avoids a fresh `filter` allocation each tick. See inline comment.
- **Internal host in config** — `sasjsconfig.json:146`. The new `nextviya` target hardcodes `https://nextviya.emea.sas.com` and `/Users/viyademo18/dc`. Other targets already hardcode `sas.4gl.io`, so this follows the existing pattern, but it does expose an internal SAS demo host. Confirm this target is intended to ship in the public repo (it's referenced by a `nextviya` npm script in `package.json`).
### Looks Good
- **Function-replacement for `$`** (`automatic.component.ts:643`): `String.replace` now uses a replacer function `() => \`contextname="${computeContextName}"\`` so `$` in the context name is treated literally. Correct fix for the prior string-replacement bug where `$&`/`$1`/`$\`` patterns would mangle names containing `$`.
- **`getComputeContextDetails` parallel fetch** (`automatic.component.ts:167`): fetches each context's full details via `Promise.all`, with a graceful error branch that resolves `runAs: null, reusable: false` instead of rejecting. Good resilience — one unfetchable context won't block the whole dropdown.
- **`getMemberAdminGroups` fallback** (`automatic.component.ts:351`): on memberships-fetch failure, falls back to showing all groups. Sensible degraded behaviour.
- **`onComputeContextChange` ordering note** (`automatic.component.ts:405`): correctly synchronises `selectedComputeContext` before deriving `runningAsUser`, with a clear comment explaining the `ngModelChange` timing. The `runningAsUser` will now match the dropdown on the same change, not lag by one.
- **`ComputeContextDetails` model** (`viya-compute-context-details.model.ts`): `attributes` fields made optional with `?` and `allowXCMD`/`sessionInactiveTimeout` added, plus a new `Environment.autoExecLines`. Aligns with the real Viya API which doesn't always populate every attribute.
- **`getCurrentUserGroupMemberships`** (`sas-viya.service.ts:174`): clean addition following the existing `getAdminGroups` pattern; `limit` is a number default, not user input, so no injection surface.
- **Test macro renames** (~30 `.test.sas` files + `testsetup.sas` + `buildtermviya.sas`): mechanical `mx_testservice`/`mp_execute`→`mx_execute` rename driven by the `@sasjs/core` 5.0→5.1 bump. All renames are consistent.
- **SAS compile** (`npx @sasjs/cli c -t server-ci`) succeeds; overall coverage holds at 33% (34/102). See the separate coverage comment.
Reviewed by Hermes Agent
Calling getFilteredComputeContexts() directly in the @for expression means Angular re-invokes the method on every change-detection cycle (it can't know the return value is stable). For a small list this is harmless, but the idiomatic and cheaper approach is to bind to a cached array property (e.g. update this.filteredComputeContexts inside ensureSelectedComputeContext() / onShowAllContextsChange() and iterate over that). This avoids a fresh filter allocation on every CD tick.
Calling `getFilteredComputeContexts()` directly in the `@for` expression means Angular re-invokes the method on every change-detection cycle (it can't know the return value is stable). For a small list this is harmless, but the idiomatic and cheaper approach is to bind to a cached array property (e.g. update `this.filteredComputeContexts` inside `ensureSelectedComputeContext()` / `onShowAllContextsChange()` and iterate over that). This avoids a fresh `filter` allocation on every CD tick.
This target hardcodes an internal SAS demo host (nextviya.emea.sas.com) and a demo user path (/Users/viyademo18/dc). It's referenced by the nextviya npm script in sas/package.json. Other targets already hardcode sas.4gl.io, so this follows the existing pattern, but confirm this internal host is intended to ship in the public repo.
This target hardcodes an internal SAS demo host (`nextviya.emea.sas.com`) and a demo user path (`/Users/viyademo18/dc`). It's referenced by the `nextviya` npm script in `sas/package.json`. Other targets already hardcode `sas.4gl.io`, so this follows the existing pattern, but confirm this internal host is intended to ship in the public repo.
The escaping covers \ (backreference) and $ (interpolation), but the prxchange substitution uses | as the delimiter:
s|contextname="[^"]*"|contextname="<escaped>"|
A context name containing a literal | would prematurely terminate the replacement string and shift the remainder into the flags section, either erroring or producing a malformed DC.html. Viya context names rarely contain |, so this is low-risk, but for completeness the delimiter itself should also be quoted in _ctxesc (e.g. add a tranwrd(...,'|','\\|')), or switch the delimiter to a character unlikely to appear in a name.
The escaping covers `\` (backreference) and `$` (interpolation), but the `prxchange` substitution uses `|` as the delimiter:
```
s|contextname="[^"]*"|contextname="<escaped>"|
```
A context name containing a literal `|` would prematurely terminate the replacement string and shift the remainder into the flags section, either erroring or producing a malformed `DC.html`. Viya context names rarely contain `|`, so this is low-risk, but for completeness the delimiter itself should also be quoted in `_ctxesc` (e.g. add a `tranwrd(...,'|','\\|')`), or switch the delimiter to a character unlikely to appear in a name.
- Paste a formula using column names (e.g. "=A_COL * B_COL") into any
cell and have it translated to that row's cell references
("=B4 * C4") so HyperFormula can evaluate it - covers both a
grid-level paste and pasting directly into an open cell editor
- Only applies on tables that already have formulas enabled (an
existing HARDFORMULA/SOFTFORMULA column); otherwise the pasted text
is left untouched rather than becoming inert translated text
- New substituteColumnReferences() reuses parseFormulaRule's
boundary-matching helpers but deliberately skips DC.* variable
substitution, which only makes sense for admin-defined rule values
Only backend-sourced `=`-led values are auto-escaped now; anything the
user types or pastes into a character column evaluates as a real
formula, and "Apply as formula" lets a user promote an auto-escaped
cell explicitly. Formulas are enabled on every table instead of only
ones with formula rules.
Uploaded file content is bulk external data, not user-typed input, so
a `=`-led value in a character column must be escaped the same way a
fresh backend load is - otherwise it silently evaluates as a live
formula now that formulas are enabled for every table.
dataSourceRaw is now escaped the same way dataSource is, so Revert
restores the escaped text a user actually saw instead of the raw
backend string (which would otherwise evaluate live again).
beforeChange re-marks a cell revert_cells restores to its escaped
form, distinguishing that from a user typing a literal leading `'`
themselves via the write's source tag.
- Add StartupCheckService to show deploy check progress on loading screen
with step-by-step status (appLoc check, Viya deploy, startup service)
- Add console logging throughout checkViyaDeploy and viyaMakedataSuccessfull
to diagnose deploy flow issues
- Fix missing return after resolve(false) in viyaMakedataSuccessfull when
folderId is undefined, which caused fall-through to getFolderMembers
with an undefined ID
- Fix Viya Folders API pagination: getFolderMembers now requests limit=500
so all members are returned (admin folder has 32 members, default page
size was hiding makedata)
- Fix error handler in viyaMakedataSuccessfull to resolve(false) instead of
reject() so the app falls back to setup screen on API errors
- Fix licence key whitespace: SAS makedata initialises keys as a single
space which is truthy in JS; trim keys before checking so empty keys
correctly fall back to free tier instead of triggering decryption errors
- Always set _debug=131 on makedata URLs (automatic and manual deploy)
so full debug output is available for the one-time setup service
- Add makedata completion polling after runMakedataInNewWindow so the app
auto-reloads when the makedata job self-deletes, instead of hanging
- Fix loading screen slider position from absolute to relative so it does
not obscure the startup check steps
- Add chunk_deploy.py to sasjs/utils for chunked Viya deploys, splitting
viya.sas by service and web file boundaries with correct %let path=
tracking per chunk
Closes#303, #200, #125
1. getstagetable.sas (lines 24, 26, 41) — &table_id used in libname and set statements without validation
table_id comes from mf_getvalue(work.iwant,table_id) (user-supplied via the web request). It's used directly in a libname path, a set statement, and a SQL where clause — all classic SAS macro injection vectors. If table_id contains spaces, quotes, or %-led macro triggers, this is exploitable. This pattern pre-exists in the file but the new code (lines 37-48) adds another unvalidated use of &table_id in a where clause and passes &base_lib/&base_ds (also from mpe_submit, user-influenced) into %dc_assignlib and %mp_getcols. Recommend validating table_id against a whitelist (e.g. exists in mpe_submit) before any use, or at minimum wrapping with appropriate sanitisation.
2. makedata.sas (lines 254-257) — prxchange regex substitution of _contextname into DC.html
The escaping on line 236 handles \ and $ but not | — the delimiter used in the prxchanges|...|...| pattern. A context name containing | would break the regex or inject a replacement. Low likelihood (context names are admin-controlled) but worth noting.
Correctness
3. sas.service.ts (lines 442, 460-465, 477-483, 526-529) — ~10 console.log statements added to production code
Multiple verbose console.log calls were added to checkViyaDeploy and viyaMakedataSuccessfull. These will spam the browser console in production. Some log internal config objects (sasjsConfig). Should be removed or gated behind a debug flag / the existing LoggerService.
4. sas.service.ts (line ~560) — error handler now silently resolves false instead of rejecting
The previous code called reject() on folder-members fetch error. Now it resolves false (redirects to /deploy). The comment says "assume setup is needed" but this masks real API failures (auth, network, permissions) as "needs setup" — the user gets sent to a deploy screen for an unrelated error.
5. editor.component.ts — dataSourceRaw uses array-index alignment with dataSource
overlayFormulaRawValuesOnUnchanged and the escape loop in initSetup both access this.dataSourceRaw[rowIndex] by index. But dataSource can be reordered by sort/insert/delete after load. The PK-matched lookups in findOverwrittenCells and syncOverwrittenCommentForCell are correct, but these index-aligned accesses will drift if any reordering happens between dataSourceRaw capture and their use. The escape loop runs immediately after capture so is likely safe, but overlayFormulaRawValuesOnUnchanged is called from editTable() which can run after sorts.
6. chunk_deploy.py (line 21) — header = lines[:HEADER_END] with no None check
If HEADER_END stays None (no %let path= or %let service= line found), lines[:None] returns the entire file as "header", and i = HEADER_END on line 29 sets i = None which throws TypeError in the while loop. Should add a guard: if HEADER_END is None: sys.exit("No header marker found").
7. chunk_deploy.py (line 63) — web_sections computed but never used
web_sections is computed on line 63 but the actual filtering on lines 67-73 recomputes web_file_sections and images_sections from scratch. web_sections is dead code.
Tests
8. No test for chunk_deploy.py — the Python deploy script has no tests and has at least two bugs (above). Given it's a deploy-time utility, at minimum a smoke test that it handles a missing header gracefully would help.
Observations
The mp_abort(mode=INCLUDE) pattern replacing the old mf_abort.error file check across 6 services is a clean simplification.
The EDIT_STATUS column approach (dc.row_status with a period to prevent SAS name collision) is well-reasoned, as is the dataDotNotation: false setting to make it work with HyperFormula.
The migration script correctly uses max(selectbox_rk)+1 rather than %mf_increment, appropriate for a standalone migration.
## PR #294 Review — Version 7.13 Release
### Security
**1. `getstagetable.sas` (lines 24, 26, 41) — `&table_id` used in `libname` and `set` statements without validation**
`table_id` comes from `mf_getvalue(work.iwant,table_id)` (user-supplied via the web request). It's used directly in a `libname` path, a `set` statement, and a SQL `where` clause — all classic SAS macro injection vectors. If `table_id` contains spaces, quotes, or `%`-led macro triggers, this is exploitable. This pattern pre-exists in the file but the new code (lines 37-48) adds another unvalidated use of `&table_id` in a `where` clause and passes `&base_lib`/`&base_ds` (also from `mpe_submit`, user-influenced) into `%dc_assignlib` and `%mp_getcols`. Recommend validating `table_id` against a whitelist (e.g. exists in `mpe_submit`) before any use, or at minimum wrapping with appropriate sanitisation.
**2. `makedata.sas` (lines 254-257) — `prxchange` regex substitution of `_contextname` into DC.html**
The escaping on line 236 handles `\` and `$` but not `|` — the delimiter used in the `prxchange` `s|...|...|` pattern. A context name containing `|` would break the regex or inject a replacement. Low likelihood (context names are admin-controlled) but worth noting.
### Correctness
**3. `sas.service.ts` (lines 442, 460-465, 477-483, 526-529) — ~10 `console.log` statements added to production code**
Multiple verbose `console.log` calls were added to `checkViyaDeploy` and `viyaMakedataSuccessfull`. These will spam the browser console in production. Some log internal config objects (`sasjsConfig`). Should be removed or gated behind a debug flag / the existing `LoggerService`.
**4. `sas.service.ts` (line ~560) — error handler now silently resolves `false` instead of rejecting**
The previous code called `reject()` on folder-members fetch error. Now it resolves `false` (redirects to /deploy). The comment says "assume setup is needed" but this masks real API failures (auth, network, permissions) as "needs setup" — the user gets sent to a deploy screen for an unrelated error.
**5. `editor.component.ts` — `dataSourceRaw` uses array-index alignment with `dataSource`**
`overlayFormulaRawValuesOnUnchanged` and the escape loop in `initSetup` both access `this.dataSourceRaw[rowIndex]` by index. But `dataSource` can be reordered by sort/insert/delete after load. The PK-matched lookups in `findOverwrittenCells` and `syncOverwrittenCommentForCell` are correct, but these index-aligned accesses will drift if any reordering happens between `dataSourceRaw` capture and their use. The escape loop runs immediately after capture so is likely safe, but `overlayFormulaRawValuesOnUnchanged` is called from `editTable()` which can run after sorts.
**6. `chunk_deploy.py` (line 21) — `header = lines[:HEADER_END]` with no None check**
If `HEADER_END` stays `None` (no `%let path=` or `%let service=` line found), `lines[:None]` returns the entire file as "header", and `i = HEADER_END` on line 29 sets `i = None` which throws `TypeError` in the `while` loop. Should add a guard: `if HEADER_END is None: sys.exit("No header marker found")`.
**7. `chunk_deploy.py` (line 63) — `web_sections` computed but never used**
`web_sections` is computed on line 63 but the actual filtering on lines 67-73 recomputes `web_file_sections` and `images_sections` from scratch. `web_sections` is dead code.
### Tests
**8. No test for `chunk_deploy.py`** — the Python deploy script has no tests and has at least two bugs (above). Given it's a deploy-time utility, at minimum a smoke test that it handles a missing header gracefully would help.
### Observations
- The `mp_abort(mode=INCLUDE)` pattern replacing the old `mf_abort.error` file check across 6 services is a clean simplification.
- The `EDIT_STATUS` column approach (`dc.row_status` with a period to prevent SAS name collision) is well-reasoned, as is the `dataDotNotation: false` setting to make it work with HyperFormula.
- The migration script correctly uses `max(selectbox_rk)+1` rather than `%mf_increment`, appropriate for a standalone migration.
### Coverage
```
Services: 22/60 (37%) | Macros: 12/42 (29%) | Overall: 34/102 (33%)
SHA: a7bb13c3 | 2026-08-21T19:35Z
```
Reviewed the incremental commit 1f4aa6f (hardening following PR review feedback). The getstagetable.sas input validation is well implemented — table_id is validated with mp_validatecol(…,ISNAME,…) and base_libds with LIBDS before any use in libname/set/where statements. The @sasjs/core bump to 5.2.0 adds the ISNAME rule and the lockfile is correctly updated. The chunk_deploy.pyHEADER_END is None guard and dead web_sections removal are clean. The new getstagetable.test.sas covers the end-to-end stage→fetch flow.
Reviewed by Hermes Agent
## Code Review
**Verdict: Approve** — no changes needed.
Reviewed the incremental commit `1f4aa6f` (hardening following PR review feedback). The `getstagetable.sas` input validation is well implemented — `table_id` is validated with `mp_validatecol(…,ISNAME,…)` and `base_libds` with `LIBDS` before any use in `libname`/`set`/`where` statements. The `@sasjs/core` bump to 5.2.0 adds the `ISNAME` rule and the lockfile is correctly updated. The `chunk_deploy.py` `HEADER_END is None` guard and dead `web_sections` removal are clean. The new `getstagetable.test.sas` covers the end-to-end stage→fetch flow.
---
*Reviewed by Hermes Agent*
saveTable() only resolved computed values for HARDFORMULA/SOFTFORMULA
columns - a formula typed, pasted, or "Apply as formula"'d into any
other character column still had its raw `=...` text submitted,
since HyperFormula recomputes the display without ever mutating the
underlying dataSource string.
saveTable() only resolved a live formula's computed value for character columns - coerceNumericRow (beforePaste/beforeAutofill) deliberately leaves a non-numeric, formula-looking value alone rather than forcing it to NaN, so a numeric column can hold a live formula too. Its raw `=...` text was reaching the backend, which a numeric SAS field can't parse.
Writing back an unescaped cell's value unconditionally materialized a column the backend never populated as an explicit `undefined`, which the SASjs adapter rejects at submit time. dynamicCellValidation's column-name lookup (Object.keys(row)[column]) only worked by coincidence when every row had a full key set - fixed to use hot.colToProp instead.
- Accept a single pasted/uploaded combined licence key (base64+gzip of
licence+activation key), auto-detected in either paste mode, alongside
the legacy two-field format
- Warn and block applying a key generated for the wrong protocol before
it ever reaches the backend
- Decode both the legacy positional features string and a new named
features object, so already-issued keys keep working unchanged
- Show a live "Key Details" preview (validity, users, site IDs, enabled
features) as a key is typed, pasted, or uploaded
- Fix cypress test setup broken by the paste-format default change, and
an unreliable FileReader-based base64 conversion in the test helpers
- Tests 7-10 only acted when already on the licensing page, silently
no-oping otherwise; now they navigate to licensing/update explicitly
when needed, chained via .then() to avoid Cypress conflating a raw
async function's promise with the preceding cy.visit()/cy.wait()
- Replace Response(readable).arrayBuffer() with a direct
ReadableStream reader loop in generateCombinedKey - the Response-based
read never settled under Cypress, likely due to its fetch/Response
patching for network interception
- licensing.cy.ts: call proceed() unconditionally via cy.then() instead of conditionally chaining .then() after cy.wait(), so the command queue stays open across the async gap and enqueued commands always run
- combinedLicenceKey.ts: replace new Response(stream).arrayBuffer() with an explicit streamToArrayBuffer helper for broader browser support
- remove debug-license.cy.ts, an untracked throwaway repro for the promise-handling question
- editor.cy.ts: assert DC.USER_NAME is non-empty instead of hardcoded
'sasdemo' (CI runs as a different OS user)
- editor.cy.ts: use =SOME_BESTNUM * 0 + 1936 for a deterministic formula
result regardless of the column's actual value
- excel.cy.ts: expect computed value '2' instead of formula text '=1+1'
(no XL_RULE=FORMULA configured on that column)
- excel-multi-load.cy.ts: use DC996664 libref to match the actual fixture
sheet names
- makedata.js: add labels to DC_JSLIB.MPE_X_TEST schema (SOME_CHAR,
SOME_DATE, SOME_DATETIME, SOME_TIME) for the viewer-labels tests
- makedata.js: register DC996664 libref with MPE_X_TEST/MPE_TABLES/
MPE_VALIDATIONS/MPE_ALERTS and copy table files for multi-load fixtures
- makedata.js: change SOME_SHORTNUM HARDREGEX from [1-5] to [0-2]
- loadfile.js: exclude VAR_TXFROM/VAR_TXTO/VAR_PROCESSED columns from the
missing-cols check for TXTEMPORAL tables (computed at load time)
- InfoModal.ts: add _PROGRAM to AbortDetails
- info-modal.component.html: display _PROGRAM in the abort modal
- 4 call sites: pass abortRes._PROGRAM when building AbortDetails
The multi-load fixture sheets for DC996664.MPE_VALIDATIONS had data
misaligned with the schema, corrupting the shared submit state. The
tests only need MPE_X_TEST and MPE_TABLES, so only register those two.
rejection.js defined its own local saveTableData(tableName, tableData)
with 2 args, but dcMockUtils.js (loaded via eval) also defines
saveTableData(dataDir, tableName, tableData) with 3 args. The
dcMockUtils version was shadowing the local one, causing
saveTableData('MPE_SUBMIT', submitData) to pass the table name as
dataDir and the data object as tableName, crashing with
'TypeError: tableName.toLowerCase is not a function'.
Removed the local saveTableData and updated calls to use the
dcMockUtils 3-arg signature: saveTableData(dataDir, tableName, data).
Same shadowing issue as rejection.js: postdata.js defined its own
saveTableData(tableName, tableData, tableLibref) which was shadowed
by dcMockUtils.saveTableData(dataDir, tableName, tableData) from the
eval. Renamed to saveTableToLib to eliminate the collision.
The Excel fixture files (7th_tab, extra_column, duplicate_column,
duplicate_row, regular_xls, regular_with_delete, password, leading
whitespace) contain SOME_SHORTNUM values of 0, 1, and 2. The
HARDREGEX /^(??$).*/ blocks those values, aborting
the upload before it reaches the submit/approve flow. Removing the
rule allows all 23 excel.cy.ts tests and both liveness.cy.ts tests
to pass.
startupservice.js only returned LIBREF in saslibs, but the dc-tree
component renders LIBRARYNAME — without it the nav tree node text was
empty, so libraryToOpenIncludes could not match. Added LIBRARYNAME,
LIBRARYID, and ENGINE to match the viewlibs service format.
Changed libraryToOpenIncludes_SASJS from 'dc' to 'dc_jslib' to avoid
matching multiple libraries (DC_JSLIB and DC996664 both contain 'dc').
viewdata.js: removed alphabetical .sort() on cols (SAS preserves
physical column order via mp_getcols); added empty row with correct
SAS missing values ('.' for numeric, '' for character) when a table
has no data, so the viewbox grid can derive column headers.
viewdata.js hardcoded PK_FIELDS as empty, so the viewbox grid treated
all columns as non-PK. removeAllColumns() could delete every column
including the ones the tests expect to be protected.
Now reads MPE_TABLES buskey (mirrors mp_getpk) and falls back to
LOAD_REF LIBREF DSN KEY_HASH TGTVAR_NM for MPE_AUDIT (which has a
known index-based PK in real SAS but isn't registered in MPE_TABLES).
All 12 viewbox.cy.ts tests now pass.
stage.cy.ts navigates directly to /stage/DC20221007T122326121_612316_7259
and expects a hot table with MPE_X_TEST data. The mock had no fixture
staged.json or MPE_SUBMIT row for this table ID, so getstagetable
returned 0 rows and getchangeinfo returned empty jsparams.
Added a fixture staged.json with MPE_X_TEST rows (SOME_DATE=42) and
a matching MPE_SUBMIT row (SUBMITTED status) in makedata.js.
getchangeinfo.js: use SAS datetime19. format (DDMMMYYYY:HH:MM:SS)
for SUBMITTED_ON_DTTM and REVIEWED_ON_DTTM, matching the real SAS
output. Use SUBMIT_STATUS_CD field name (not REVIEW_STATUS_ID)
to match the MPE_SUBMIT column name and getchangeinfo.sas output.
stage.cy.ts: add login step in beforeEach (the beforeAll logs out
after uploading the license, so each test needs to re-login).
Use loginAndUpdateValidKey(true) to force license upload.
The stage.cy.ts fixture had 7 rows of MPE_X_TEST data with wide string
values, causing Handsontable to auto-size columns wider so only 4 of
10 fit in the 1280px viewport. The test checks .ht_clone_top headers
which only render visible columns.
Limited to 1 row (matching the original sas9 mock) and moved
_____DELETE__THIS__RECORD_____ to the end of the column order.
Rewrite stage.cy.ts to follow the same pattern as excel.cy.ts:
open MPE_X_TEST, attach regular_excel.xlsx, submit to create staged
data, then test the formatted/unformatted toggle on the stage page.
The date assertion is now dynamic: reads the raw SOME_DATE value
from the unformatted view and computes the expected SAS date9.
formatted value, rather than hardcoding 12FEB1960.
Remove the fixture stageSubmission code from makedata.js — staged
data is created at runtime by stagedata.js, not by makedata.
Summary: Large v7.13 release PR (236 files) covering licence key refactoring, editor formula rules, viewbox resize handles, CAS REPLACE load type, and getstagetable input validation. The security hardening on getstagetable.sas and the CAS-aware REPLACE path in mpe_targetloader.sas are well done. A few issues below.
Issues:
[sas.service.ts:449] — Multiple console.log statements left in production code (checkViyaDeploy: checking appLoc, appLoc exists?, makedata job already run?, etc.). These should use loggerService or be removed before release — they'll spam the browser console for every Viya deployment check.
[sas.service.ts:593] — The getFolderMembers error handler changed from reject() to resolve(false). While "assume setup is needed on error" is reasonable, this silently swallows API errors — if the Viya folder API is down, the user is redirected to /deploy with no indication of the real problem. Consider at least logging the error via loggerService.error() before resolving.
[combinedLicenceKey.ts:54] — DecompressionStream is a relatively new Web API (Chrome 80+, not available in older browsers or Node < 18). If this code runs in any non-modern-browser context, gzip decompression will throw. Consider a feature check or polyfill fallback.
[mpe_targetloader.sas — CAS REPLACE path] — The temp table CASUSER.&tmpds is created and loaded before the syscc>0 check, but if the data step that populates it fails mid-way, the proc sql; drop table CASUSER.&tmpds;quit; in the error path runs — which is good. However, the non-CAS REPLACE path's error check (syscc>0) only unlocks the table; it doesn't clean up any WORK. temp datasets created earlier in the macro. Verify that WORK.&STAGING_DS and related temps are cleaned up by SAS session termination or explicit drops elsewhere.
Suggestions:
[decodeLicenceFeatures.ts:35-46] — The decodeFeaturesObject uses ?? to default numeric fields to Infinity. This is correct, but the boolean fields (vb, fu, er, ar) are passed through without coercion — if the object has undefined for these, the licence state would have undefined booleans rather than false. Consider !!features.vb for safety.
[automatic.component.ts:167] — getComputeContextDetails() fires N parallel API calls (one per compute context). For servers with many contexts, this could be slow or hit rate limits. Consider whether the batch identity is needed for all contexts or just the selected one.
[getstagetable.sas] — The table_id validation with mp_validatecol(ISNAME) is excellent and prevents code injection. Consider also validating base_lib and base_ds output from mpe_submit with the same macro before using them in %dc_assignlib and %mp_getcols (the LIBDS check is used, which is good, but double-check it covers all injection vectors for librefs/dsnames used in macro variable resolution).
Looks good:
getstagetable.sas input validation is a genuine security improvement — table_id was previously used unsanitized in libname and set statements.
The CAS REPLACE path in mpe_targetloader.sas correctly uses table.deleteRows (CAS doesn't support SQL deletes) and places the error check before the destructive truncate.
The licence feature decoding refactoring cleanly separates the two format variants (positional string vs named object) with clear documentation of the append-only contract.
Extensive test coverage added across dc-validator/utils/, licensing/utils/, and viewboxes.component.spec.ts.
## Code Review — 4gl-reviewer
**Summary:** Large v7.13 release PR (236 files) covering licence key refactoring, editor formula rules, viewbox resize handles, CAS REPLACE load type, and `getstagetable` input validation. The security hardening on `getstagetable.sas` and the CAS-aware REPLACE path in `mpe_targetloader.sas` are well done. A few issues below.
**Issues:**
- [`sas.service.ts:449`] — Multiple `console.log` statements left in production code (`checkViyaDeploy: checking appLoc`, `appLoc exists?`, `makedata job already run?`, etc.). These should use `loggerService` or be removed before release — they'll spam the browser console for every Viya deployment check.
- [`sas.service.ts:593`] — The `getFolderMembers` error handler changed from `reject()` to `resolve(false)`. While "assume setup is needed on error" is reasonable, this silently swallows API errors — if the Viya folder API is down, the user is redirected to `/deploy` with no indication of the real problem. Consider at least logging the error via `loggerService.error()` before resolving.
- [`combinedLicenceKey.ts:54`] — `DecompressionStream` is a relatively new Web API (Chrome 80+, not available in older browsers or Node < 18). If this code runs in any non-modern-browser context, gzip decompression will throw. Consider a feature check or polyfill fallback.
- [`mpe_targetloader.sas` — CAS REPLACE path] — The temp table `CASUSER.&tmpds` is created and loaded before the `syscc>0` check, but if the `data` step that populates it fails mid-way, the `proc sql; drop table CASUSER.&tmpds;quit;` in the error path runs — which is good. However, the non-CAS REPLACE path's error check (`syscc>0`) only unlocks the table; it doesn't clean up any `WORK.` temp datasets created earlier in the macro. Verify that `WORK.&STAGING_DS` and related temps are cleaned up by SAS session termination or explicit drops elsewhere.
**Suggestions:**
- [`decodeLicenceFeatures.ts:35-46`] — The `decodeFeaturesObject` uses `??` to default numeric fields to `Infinity`. This is correct, but the boolean fields (`vb`, `fu`, `er`, `ar`) are passed through without coercion — if the object has `undefined` for these, the licence state would have `undefined` booleans rather than `false`. Consider `!!features.vb` for safety.
- [`automatic.component.ts:167`] — `getComputeContextDetails()` fires N parallel API calls (one per compute context). For servers with many contexts, this could be slow or hit rate limits. Consider whether the batch identity is needed for all contexts or just the selected one.
- [`getstagetable.sas`] — The `table_id` validation with `mp_validatecol(ISNAME)` is excellent and prevents code injection. Consider also validating `base_lib` and `base_ds` output from `mpe_submit` with the same macro before using them in `%dc_assignlib` and `%mp_getcols` (the `LIBDS` check is used, which is good, but double-check it covers all injection vectors for librefs/dsnames used in macro variable resolution).
**Looks good:**
- `getstagetable.sas` input validation is a genuine security improvement — `table_id` was previously used unsanitized in `libname` and `set` statements.
- The CAS REPLACE path in `mpe_targetloader.sas` correctly uses `table.deleteRows` (CAS doesn't support SQL deletes) and places the error check before the destructive truncate.
- The licence feature decoding refactoring cleanly separates the two format variants (positional string vs named object) with clear documentation of the append-only contract.
- Extensive test coverage added across `dc-validator/utils/`, `licensing/utils/`, and `viewboxes.component.spec.ts`.
String.replace interpreted $&/$`/$'/$<digit> specially in DC.ORIG_VALUE/DC.USER_NAME substitutions, corrupting formulas whose cell data happened to contain a literal $. Separately, afterChange indexed dataSource with Handsontable's visual row instead of translating to physical (as beforeChange already does), desyncing EDIT_STATUS/overwritten-comment writes on a sorted grid. Investigating that surfaced a third, unrelated Handsontable bug: updateSettings() corrupts formula cell references whenever a sort is active, regardless of what it changes - fixed by clearing the sort around every updateSettings() call.
cancelEdit()'s getFormulaCellsToPreserveOnCancel callbacks passed a
physical rowIndex straight into visual-row-expecting Handsontable APIs
(getCommentAtCell, getDataAtRowProp) while the grid could still be
sorted, since clearSort() only ran later, right before updateSettings().
On a sorted grid this read the wrong row's comment/value, corrupting
which formula cells got preserved across cancel. Move clearSort() (via
getCurrentSortConfigs()) ahead of those callbacks instead.
Summary: Large v7.13 release PR adding formula column support (HARDFORMULA/SOFTFORMULA), combined licence keys, CAS engine REPLACE load type, editor row edit-status tracking, and substantial refactoring with well-tested extractions. Overall quality is high with strong test coverage, but a few items should be addressed before merge.
Issues:
[client/src/app/services/sas.service.ts:449-520] — Extensive console.log statements were left in checkViyaDeploy() and viyaMakedataSuccessfull(). These are debug-level logs (checkViyaDeploy: checking appLoc, viyaMakedataSuccessfull: checking folder, etc.) that will spam the browser console in production. Remove them or gate behind a debug flag.
[sas/sasjs/utils/chunk_deploy.py:152] — subprocess.run(..., timeout=timeout) can raise subprocess.TimeoutExpired which is not caught. An uncaught exception will crash the deploy script with a traceback instead of a clean failure message. Wrap in try/except to print a meaningful timeout error.
[sas/sasjs/macros/mpe_targetloader.sas:175-182] — In the CAS REPLACE path, the &syscc>0 check drops CASUSER.&tmpds between the UNLOCK and the %mp_abort call. If the drop fails (e.g. session state issues), &syscc could escalate, but %mp_abort is already conditioned on the prior &syscc value so this is low-risk. Consider moving the cleanup to after the abort guard for clarity.
Suggestions:
[client/src/app/licensing/licensing.component.ts:84-101] — bypassSecurityTrustHtml is used for all error messages. The comment correctly notes the content is hardcoded (no user input), so this is safe today, but consider keeping a code comment or lint rule to prevent future developers from interpolating user-supplied strings into these messages.
[client/src/app/editor/utils/classifyRow.ts:33] — JSON.stringify comparison for change detection is fragile (key ordering, undefined vs missing properties). It matches the existing inline logic it was extracted from, but consider a deep-equals utility for future iterations.
[client/src/app/editor/utils/normalizeSortConfig.ts:12] — The any[] return type is acknowledged in the doc comment (Handsontable's SortConfig isn't publicly typed). Acceptable pragmatic choice; add a // TODO: type when HT exports SortConfig for tracking.
Looks good:
Excellent test coverage on new extractions — classifyRow, expandCellRanges, combinedLicenceKey, decodeLicenceFeatures, syncOverwrittenCellComment, and the integration spec for sorted-grid row sync are all well-tested with edge cases.
The CAS REPLACE implementation in mpe_targetloader.sas mirrors the established pattern from bitemporal_dataloader.sas, including the pre-destructive abort check and varchar casting.
The mp_abort(mode=INCLUDE) additions across postdata.sas, getdata.sas, getdynamiccolvals.sas, loadfile.sas, and restore.sas are a clean fix for hook-script abort propagation.
The decodeLicenceFeatures refactor cleanly supports both legacy positional strings and new named objects without a version flag.
## Code Review — 4gl-reviewer
**Summary:** Large v7.13 release PR adding formula column support (HARDFORMULA/SOFTFORMULA), combined licence keys, CAS engine REPLACE load type, editor row edit-status tracking, and substantial refactoring with well-tested extractions. Overall quality is high with strong test coverage, but a few items should be addressed before merge.
**Issues:**
- [client/src/app/services/sas.service.ts:449-520] — Extensive `console.log` statements were left in `checkViyaDeploy()` and `viyaMakedataSuccessfull()`. These are debug-level logs (`checkViyaDeploy: checking appLoc`, `viyaMakedataSuccessfull: checking folder`, etc.) that will spam the browser console in production. Remove them or gate behind a debug flag.
- [sas/sasjs/utils/chunk_deploy.py:152] — `subprocess.run(..., timeout=timeout)` can raise `subprocess.TimeoutExpired` which is not caught. An uncaught exception will crash the deploy script with a traceback instead of a clean failure message. Wrap in try/except to print a meaningful timeout error.
- [sas/sasjs/macros/mpe_targetloader.sas:175-182] — In the CAS REPLACE path, the `&syscc>0` check drops `CASUSER.&tmpds` between the UNLOCK and the `%mp_abort` call. If the drop fails (e.g. session state issues), `&syscc` could escalate, but `%mp_abort` is already conditioned on the prior `&syscc` value so this is low-risk. Consider moving the cleanup to after the abort guard for clarity.
**Suggestions:**
- [client/src/app/licensing/licensing.component.ts:84-101] — `bypassSecurityTrustHtml` is used for all error messages. The comment correctly notes the content is hardcoded (no user input), so this is safe today, but consider keeping a code comment or lint rule to prevent future developers from interpolating user-supplied strings into these messages.
- [client/src/app/editor/utils/classifyRow.ts:33] — `JSON.stringify` comparison for change detection is fragile (key ordering, `undefined` vs missing properties). It matches the existing inline logic it was extracted from, but consider a deep-equals utility for future iterations.
- [client/src/app/editor/utils/normalizeSortConfig.ts:12] — The `any[]` return type is acknowledged in the doc comment (Handsontable's SortConfig isn't publicly typed). Acceptable pragmatic choice; add a `// TODO: type when HT exports SortConfig` for tracking.
**Looks good:**
- Excellent test coverage on new extractions — `classifyRow`, `expandCellRanges`, `combinedLicenceKey`, `decodeLicenceFeatures`, `syncOverwrittenCellComment`, and the integration spec for sorted-grid row sync are all well-tested with edge cases.
- The CAS REPLACE implementation in `mpe_targetloader.sas` mirrors the established pattern from `bitemporal_dataloader.sas`, including the pre-destructive abort check and varchar casting.
- The `mp_abort(mode=INCLUDE)` additions across `postdata.sas`, `getdata.sas`, `getdynamiccolvals.sas`, `loadfile.sas`, and `restore.sas` are a clean fix for hook-script abort propagation.
- The `decodeLicenceFeatures` refactor cleanly supports both legacy positional strings and new named objects without a version flag.
multiColumnSorting's sort() replaces the entire sort state on every call rather than accumulating, so the restore loop after updateSettings()/editTable()/cancelEdit() silently dropped every sorted column but the last. Pass the captured configs as a single array instead. Adds a Karma integration test proving the loop
loses columns, and a Cypress test verifying cancelEdit() keeps a real shift-click multi-column sort intact end-to-end.
Summary: Large v7.13 release introducing formula validation rules (HARDFORMULA/SOFTFORMULA) with HyperFormula integration, combined licence key support, CAS table handling in mpe_targetloader, security hardening in getstagetable.sas, and extensive utility extraction with good test coverage. Overall well-structured with thoughtful documentation.
Issues:
[sas/sasjs/macros/mpe_targetloader.sas:~180] — CAS branch does table.deleteRows where="1=1" (full truncate) before the data ... append=yes step. If the append fails, the target table is left empty with no rollback. Consider wrapping in a transaction or staging a backup that can be restored on failure.
[client/src/app/services/sas.service.ts:449-525] — Many verbose console.log statements (e.g. 'checkViyaDeploy: checking appLoc', 'viyaMakedataSuccessfull: folder members:') appear to be debug logging left in production code. These should use loggerService or be removed/gated behind a debug flag.
[client/src/app/shared/dc-validator/utils/applyFormulaRules.ts:30] and [findOverwrittenCells.ts:31] — Both do dataSourceUnchanged.find(...) inside a forEach loop, giving O(n×m) complexity for PK matching. On large tables this could be slow. A Map keyed by PK values would make each lookup O(1).
[client/src/app/licensing/licensing.component.ts:70-90] — bypassSecurityTrustHtml is used for error messages. The comment correctly notes the content is hardcoded, but this pattern bypasses Angular's XSS protection — if any of these strings ever incorporate user-derived data in the future, it becomes an injection vector. Worth a follow-up to render <clr-icon> via Angular templates instead.
[sas/sasjs/db/migrations/20260807_v7.13_release.sas:17] — select max(selectbox_rk) into: maxrk then &maxrk+1 / &maxrk+2 will produce .+1 (a SAS error) if mpe_selectbox is empty. The makedata equivalent uses %mf_increment(rk) which is safer. Consider adding a %let maxrk=0; default before the select.
Suggestions:
[client/src/app/services/utils/decodeLicenceFeatures.ts:64] — parseInt(codeBit) is called without a radix argument. While moved unchanged from the original, parseInt(codeBit, 10) would be more defensive against leading-zero strings.
[sas/sasjs/utils/chunk_deploy.py:140] — sys.exit(1) on the first chunk failure means a partial deployment with no rollback. Consider logging which chunks succeeded so the operator knows where to resume.
[client/src/app/deploy/sections/automatic/automatic.component.ts:165] — getComputeContextDetails fires one API call per context in parallel. Environments with many contexts could hit rate limits; consider batching or at least logging a count of contexts being fetched.
Looks good:
Excellent test coverage — nearly every new utility has a corresponding .spec.ts file with meaningful test cases.
getstagetable.sas security hardening (table_id validation via mp_validatecol + whitelist check against mpe_submit) is a solid injection-prevention pattern.
parseFormulaRule.ts correctly skips quoted string spans during variable substitution and uses function replacements to avoid $ interpretation — well thought out.
classifyRow.ts correctly excludes EDIT_STATUS from the diff comparison to avoid self-perpetuating 'M' state.
resizeFromLeftEdge in viewboxes.component.ts has an excellent explanation of why raw pointer coordinates are used instead of getBoundingClientRect.
Combined licence key format detection (DCKEY1: prefix, unambiguous with base64 alphabet) is clean and well-documented.
## Code Review — 4gl-reviewer
**Summary:** Large v7.13 release introducing formula validation rules (HARDFORMULA/SOFTFORMULA) with HyperFormula integration, combined licence key support, CAS table handling in mpe_targetloader, security hardening in getstagetable.sas, and extensive utility extraction with good test coverage. Overall well-structured with thoughtful documentation.
**Issues:**
- [`sas/sasjs/macros/mpe_targetloader.sas:~180`] — CAS branch does `table.deleteRows where="1=1"` (full truncate) before the `data ... append=yes` step. If the append fails, the target table is left empty with no rollback. Consider wrapping in a transaction or staging a backup that can be restored on failure.
- [`client/src/app/services/sas.service.ts:449-525`] — Many verbose `console.log` statements (e.g. `'checkViyaDeploy: checking appLoc'`, `'viyaMakedataSuccessfull: folder members:'`) appear to be debug logging left in production code. These should use `loggerService` or be removed/gated behind a debug flag.
- [`client/src/app/shared/dc-validator/utils/applyFormulaRules.ts:30`] and [`findOverwrittenCells.ts:31`] — Both do `dataSourceUnchanged.find(...)` inside a `forEach` loop, giving O(n×m) complexity for PK matching. On large tables this could be slow. A `Map` keyed by PK values would make each lookup O(1).
- [`client/src/app/licensing/licensing.component.ts:70-90`] — `bypassSecurityTrustHtml` is used for error messages. The comment correctly notes the content is hardcoded, but this pattern bypasses Angular's XSS protection — if any of these strings ever incorporate user-derived data in the future, it becomes an injection vector. Worth a follow-up to render `<clr-icon>` via Angular templates instead.
- [`sas/sasjs/db/migrations/20260807_v7.13_release.sas:17`] — `select max(selectbox_rk) into: maxrk` then `&maxrk+1` / `&maxrk+2` will produce `.+1` (a SAS error) if `mpe_selectbox` is empty. The makedata equivalent uses `%mf_increment(rk)` which is safer. Consider adding a `%let maxrk=0;` default before the select.
**Suggestions:**
- [`client/src/app/services/utils/decodeLicenceFeatures.ts:64`] — `parseInt(codeBit)` is called without a radix argument. While moved unchanged from the original, `parseInt(codeBit, 10)` would be more defensive against leading-zero strings.
- [`sas/sasjs/utils/chunk_deploy.py:140`] — `sys.exit(1)` on the first chunk failure means a partial deployment with no rollback. Consider logging which chunks succeeded so the operator knows where to resume.
- [`client/src/app/deploy/sections/automatic/automatic.component.ts:165`] — `getComputeContextDetails` fires one API call per context in parallel. Environments with many contexts could hit rate limits; consider batching or at least logging a count of contexts being fetched.
**Looks good:**
- Excellent test coverage — nearly every new utility has a corresponding `.spec.ts` file with meaningful test cases.
- `getstagetable.sas` security hardening (table_id validation via `mp_validatecol` + whitelist check against `mpe_submit`) is a solid injection-prevention pattern.
- `parseFormulaRule.ts` correctly skips quoted string spans during variable substitution and uses function replacements to avoid `$` interpretation — well thought out.
- `classifyRow.ts` correctly excludes `EDIT_STATUS` from the diff comparison to avoid self-perpetuating 'M' state.
- `resizeFromLeftEdge` in viewboxes.component.ts has an excellent explanation of why raw pointer coordinates are used instead of getBoundingClientRect.
- Combined licence key format detection (`DCKEY1:` prefix, unambiguous with base64 alphabet) is clean and well-documented.
allan
merged commit 34ff2ac8d0 into main2026-09-03 14:16:03 +00:00
allan
deleted branch version7-132026-09-03 14:16:03 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
hermes
Highlights
Formula-based data quality rules
A major new capability:
HARDFORMULA/SOFTFORMULADQ rule types, backed by Handsontable's formulas plugin (HyperFormula).DC.USER_NAME/DC.ORIG_VALUEto literal values;HARDFORMULAreuses the existing READONLY mechanism.DC.ROW_STATUSvia a hiddenEDIT_STATUScolumn, so formulas can react to a row's edit state; the row-header +/-/~ indicator translates visual→physical row index so it stays correct when the grid is sorted.hot.alter()so formula values realign instead of going stale.UserServiceis nowprovidedIn: 'root', fixingDC.USER_NAME(and other lazy-module readers) always seeing an unset user.saveTable()submits the live computed value rather than the raw=...string.EDIT_STATUS/dc.row_statuscolliding with a real column of the same name, and stops the client-onlyEDIT_STATUScolumn from being submitted to the backend during dynamic cell validation.Cell revert
Revert one cells (or range of cells) to their original (current) value
Formatted / Unformatted toggle on the staging page
Mirrors the review page's toggle.
getstagetable.sasnow also returns SAS-formatted data (fmt_stagetable), and the UI picks between formatted and raw, falling back to raw when formatted data is absent. Added e2e coverage.Viewbox resize on all edges
Viewboxes can now be dragged to resize from all four edges and corners (previously only the bottom-right corner). Resize handles sit above Handsontable's frozen header clones so clicks reach the handle rather than the table underneath.
Licence-key protocol mismatch warning
A licence key generated for one connection type (http/https) always fails to decrypt on the other. The mismatch is now detected client-side from the raw key text before submission, with a warning and the apply button disabled, instead of surfacing only as a generic "invalid key" error after a backend round trip.
Bug fixes
getstagetable.sas.mp_validatefix.dc.row_status/EDIT_STATUSis no longer submitted to the backend.Dependencies / security
@angular/*packages to the same lockstep version (20.3.27) so CI's strictnpm cipasses.brace-expansionoverride to its actual vulnerable chain (fixes license-checker breakage).babel-loaderto resolve an unresolvable peer conflict; regenerated/patched the lockfile to fixnpm cisync errors.nanoidto 3.3.18 insas/directly (rather than relying on@sasjs/cli's override, which doesn't propagate to consumers) so it survives futurenpm installruns.- addRow()/insertRowAtPosition() use hot.alter() so HARDFORMULA/SOFTFORMULA values realign on insert instead of going stale - New hidden EDIT_STATUS column gives DC.ROW_STATUS a real, live cell reference; row-header +/-/~ indicator now translates visual->physical row index so it stays correct when the grid is sorted - UserService is now providedIn: 'root' instead of module-scoped, fixing DC.USER_NAME (and any other lazy-module reader) always seeing an unset user - Column-info dropdown shows the applied formula ("√x=<formula>")Hermes Agent Code Review
Verdict: Comment — no blocking issues found; a few warnings and suggestions below.
This is a large, well-structured release PR (94 files, +7189/-3009). The new formula-based DQ rules feature is the core change, with supporting cell revert, formatted/unformatted staging toggle, viewbox resize on all edges, and licence-key protocol mismatch warning. The extraction of logic into small, well-tested utility functions is good practice.
Critical
None.
Warnings
W1 —
quoteLiteraldoes not escape embedded double quotes (parseFormulaRule.ts:38-39)When
DC.USER_NAMEorDC.ORIG_VALUEcontains a double quote character (e.g. a username likejohn"doeor a cell value with"), the resulting formula string will have an unescaped quote inside a quoted literal, producing a malformed HyperFormula expression. This could cause the formula to silently fail to evaluate, or in a worst case, alter the formula's semantics. The values come from SAS data / user session, so while admin-controlled in most deployments, a user with a quote in their name is plausible.Suggested fix:
W2 —
Number(rawValueText)can produceNaNin revert callback (editor.component.ts:96-100)If a numeric column's original value was stored in the comment as a non-numeric string (unlikely but possible if the comment was somehow corrupted or the raw SAS value was a special missing like
.S),Number(rawValueText)returnsNaN, which would then be written into the cell. There's no guard against this. Consider falling back to the raw text or validating before conversion.W3 — Missing
mp_abortbetweendc_assignlibandmp_getcolsin getstagetable.sas (lines 48-52)If
mpe_submithas no row matching&table_id(e.g. stale table_id, race condition),&base_liband&base_dsremain empty.dc_assignlib(READ,)andmp_getcols(..,.)will fail, but themp_abortthat catches this only runs aftermp_applyformats— several steps later. While&sysccwill be set and eventually caught, adding an explicit abort immediately after thedata _null_step (or afterdc_assignlib) would provide a clearer error message and prevent confusing intermediate errors.W4 —
addRow()with emptydataSourcepassesindex - 1tohot.alter()(editor.component.ts:1444-1452)When
dataSource.length === 0,newIndex - 1is-1. Handsontable'salter()does treat-1as "insert at the end" which works, butthis.dataSource[newIndex]immediately after assumesalter()synchronously spliced intodataSource. If the formulas plugin'sbeforeCreateRowhook interferes or the data binding is async, this could throw. ThebeforeCreateRowhook does checkaddingNewRow(set totruebefore), so it should be fine in practice — but this is fragile and worth a defensive guard.Suggestions
S1 —
buildColInfoHtmldoes not HTML-escapeformulaValue(col-info-html.ts:35)formulaValuecomes fromRULE_VALUEinMPE_VALIDATIONS, which is admin-controlled. However, the same function already interpolatescolName,hardRegexValue, etc. without escaping, so this is consistent with existing behavior. Still, if any of these values contain<or>, they would be interpreted as HTML. Consider escaping all interpolated values in this function in a future cleanup.S2 —
getstagetable.sas— nomp_abortifbase_lib/base_dsare empty after the DATA stepRelated to W3. A simple guard would be:
S3 — Migration script
20260807_v7.13_release.sas—max(selectbox_rk)on empty tableIf
mpe_selectboxwere ever empty,&maxrkresolves to missing (.), and&maxrk+1is also missing. The INSERT would then setselectbox_rk=.. This is unlikely in a running DC instance (the table is always populated), butmpe_makedata.sasuses%mf_increment(rk)which handles this more robustly. Not a real-world issue, just a consistency note.S4 —
detectLicenceKeyProtocolMismatch— heuristic-based detection is fragileThe protocol mismatch detection relies on the assumption that HTTPS-format keys always have different licence/activation values and HTTP-format keys always have identical values. This is a structural assumption about the key generation process. If the key generation logic ever changes (e.g. HTTP keys also get distinct values), this detection would silently produce false positives. Worth a comment in the key generation code pointing to this dependency, or a more robust detection method.
S5 —
mpe_targetloader.sasCAS branch —dcsessionis referenced without being established in this macroThe
dcsessionCAS session is presumably established upstream (in the calling service). If it's not, this will fail with a confusing error. This is pre-existing convention, but a comment noting the prerequisite would help maintainers.Looks Good
dc.row_status(with a period) as a client-only column name that can never collide with a real SAS variable is a clever and safe approach. ThedataDotNotation: falsesetting and thewithoutEditStatusstripping before backend submission are correctly handled.findOverwrittenCellscorrectly handles row reordering. The liveafterChangesync keeps comments and EDIT_STATUS up to date.mp_abort(mode=INCLUDE)additions in restore.sas, stagedata.sas, postdata.sas, getdata.sas, etc. correctly replace the deprecatedmf_abort.errorfile-check pattern with the modern@sasjs/coreapproach. The CAS REPLACE branch inmpe_targetloader.saswith varchar casting anddeleteRowstruncation is well-structured with appropriatemp_abortguards before destructive operations.cdkDragto native pointer events for resize handles is a solid improvement. TheendActiveResizecleanup inngOnDestroyprevents listener leaks. TheresizeFromLeftEdgeread-back of actual width (to respect CSSmin-width) is a nice touch.window.crypto.subtleavailability (rather thanlocation.protocol) correctly handles localhost.Reviewed by Hermes Agent
W2 —
Number(rawValueText)can produceNaNIf the comment text after the prefix is not a valid number (e.g. a SAS special missing like
.S),Number()returnsNaNwhich gets written into the cell. Consider a guard:W4 —
addRow()with emptydataSourceWhen
dataSource.length === 0,newIndex - 1is-1. Handsontable treats this as "insert at end" so it works, butthis.dataSource[newIndex]immediately after assumesalter()synchronously spliced the row. A defensive guard (if (this.dataSource[newIndex])) would prevent a crash if the data binding ever becomes async.W1 —
quoteLiteraldoes not escape embedded double quotesIf
DC.USER_NAMEorDC.ORIG_VALUEcontains a"character, the resulting formula string will have an unescaped quote inside a quoted literal, producing a malformed HyperFormula expression.Suggested fix:
W3 — Missing
mp_abortbetweendc_assignlibandmp_getcolsIf
mpe_submithas no row matching&table_id,&base_lib/&base_dsremain empty. The abort that catches this only runs aftermp_applyformats— several steps later. Consider adding an explicit guard here:Hermes Agent Code Review
Verdict: Approve
This commit (
067c087- "chore: adding additional mp_abort per per review") addresses prior review feedback: it adds a null-check guard togetstagetable.sasfor the case where nompe_submitrecord is found for the giventable_id.Critical
None.
Warnings
None.
Suggestions
getstagetable.sas lines 45-48 - guard placement and syscc interaction
The data step that reads
mpe_submit(lines 39-44) could set&sysccto a non-zero value if it encounters an error (e.g.mpe_submitlocked or missing), in which case the existingmp_abortat line 29 (checking&syscc ne 0after thestagetableload) would already have fired. However, ifmpe_submitexists but simply has no matchingTABLE_IDrow,&sysccstays 0 andbase_lib/base_dsremain empty - which is exactly the scenario this new guard catches. The guard is correctly placed after the data step and beforedc_assignlib(READ,&base_lib)(line 50), which would otherwise fail or behave unpredictably with an empty libref. Good defensive addition.One minor consideration: the error message
No mpe_submit record found for table_id=&table_idwill surface to the end user viamp_abort. Confirm this is the desired UX (exposing the internaltable_idvalue) vs. a more generic message. This is a stylistic choice, not a defect.Looks Good
dc_assignlibfrom being called with an empty&base_lib, which would produce a confusing low-level SAS error rather than a clear abort message.mp_abortis already in the file's documented@limacro list (line 10), so no doc header update is needed.mac=&_program..sasparameter correctly identifies the service in the abort message for debugging.Reviewed by Hermes Agent
The error message exposes the internal
table_idvalue to the end user viamp_abort. Consider whether a more generic message (e.g. "staged table could not be located") is preferable for end-user UX, or if exposingtable_idis intentional for support/debugging purposes. Stylistic choice, not a defect.Test Coverage Report
Coverage measured via
npx @sasjs/cli c -t server-ciagainst theserver-citarget.The compile step completed successfully (
server-citarget). A non-fatal warning was emitted aboutwebSourcePath: client/distnot existing - expected in this SAS-only checkout context and does not affect coverage measurement.Note: This PR is primarily a client-side (Angular) release. The SAS-side change in this commit is a single defensive
mp_abortguard added togetstagetable.sas. The coverage figures above reflect the SAS test suite only; client-side Karma/Cypress coverage is run separately in CI.Generated by Hermes Agent
Test Coverage Report
Coverage measured via
npx @sasjs/cli c -t server-ciagainst theserver-citarget.The compile completed successfully. The
streamWebweb-app compile step errored (client/distdoes not exist in this review environment — the Angular client was not built), but this does not affect the SAS service/macro coverage figures above.Notes
exportdb.test.sasandrefreshcatalog.test.sas.mp_testservice→mp_executeandmp_getddl→mp_ds2ddl; all test files were updated accordingly.Generated by Hermes Agent
Hermes Agent Code Review
Verdict: Approve
This review covers the new commits since the last review (
067c087→9b2e0df): the@sasjs/corev4→v5 bump (a breaking-change migration) and two new SAS test files.Summary of new changes
@sasjs/core4.68.3 → 5.0.0: Updatedpackage.jsonandpackage-lock.json.mp_testservice→mp_execute: All 14 test files migrated.mp_executeis a thin wrapper aroundmx_testservice(verified in core v5 source) with an identical parameter signature — the calls work as drop-in replacements.mp_getddl→mp_ds2ddl: Two service files (exportdb.sas,getddl.sas) updated, with the newshowlog=NOparameter added to suppress log output in web-service context.mp_coretable→mddl_dc_*:testsetup.sasnow usesmddl_dc_locktable/mddl_dc_difftable(verified to exist in core v5 with thelibds=parameter). Cleaned uptestinit.sas(removed stale comments).exportdb.test.sas(74 lines, tests PGSQL + SAS flavours) andrefreshcatalog.test.sas(58 lines, tests catalog refresh + verification).Checks performed
mp_testservice,mp_getddl, ormp_coretablereferences remain anywhere insas/sasjs/.mp_execute,mp_ds2ddl,mddl_dc_locktable,mddl_dc_difftable) exist in@sasjs/core@5.0.0.mp_executeparameter signature matches the oldmp_testservicecalls —inputfiles,inputdatasets,outlib,outref,viyacontext,viyaresult,mdebugall carried over correctly.mp_ds2ddlcalls preserved thefref=,flavour=,schema=,applydttm=parameters and addedshowlog=NO.npx @sasjs/cli c -t server-ci) succeeds; coverage increased from 31%→33% overall thanks to the two new test files.%mx_testservice+%mp_assertdsobs), are well-structured, and test both positive and edge cases.No issues found
The migration is mechanical, complete, and correct. No security, correctness, or style concerns in the new commits.
Reviewed by Hermes Agent
Hermes Agent Code Review
Verdict: Approve
This review covers the new commits since the last review (
9b2e0df→bf54a58): commit36963aa"fix: address hermes review feedback (formula quoting, cell revert, addRow guard)" and its merge.Summary of new changes
The incremental commits address three pieces of prior review feedback:
Formula literal quoting fix (
parseFormulaRule.ts):quoteLiteralnow escapes embedded double-quotes ("→"") so aDC.USER_NAME/DC.ORIG_VALUEcontaining a"produces a valid spreadsheet literal instead of a broken formula. Two new spec cases coverjohn"doeanda "quoted" value. Correct and well-tested.Cell revert special-missing handling (
resolveRevertedCellValue.ts, new): Reverting a numeric column now falls back to the raw text whenNumber(rawValueText)isNaN(e.g. a SAS special missing like.S), instead of silently writingNaNinto the cell. The editor's revert callback now calls this helper instead of the inlineisNumericCol ? Number(rawValueText) : rawValueText. Three new spec cases cover non-numeric, valid numeric, and special-missing. Clean extraction.addRow guard (
editor.component.ts): Afterhot.alter('insert_row_below', ...)thethis.dataSource[newIndex].noLinkOption = trueassignment is now guarded withif (this.dataSource[newIndex]), defending against the data binding ever becoming async. Defensive and correct.Doc note (
detectLicenceKeyProtocolMismatch.ts): Added a comment documenting that the HTTP/HTTPS detection is a structural assumption about the (separate) key-generation repo. Good documentation of a cross-repo coupling.Checks performed
quoteLiteralescaping is correct:String(value ?? '').replace(/"/g,'""')produces valid""-escaped literals, matching the two new specs.resolveRevertedCellValueusesNumber.isNaN()(not looseisNaN) — correctly distinguishesNaNfrom numeric coercion quirks.editor.component.tspasses the samerawValueText/isNumericColthe inline code did — no behavioural regression.seedFormulaValuesForRow/updateEditStatusForRowcalls that follow it.npx @sasjs/cli c -t server-ci) succeeds; overall coverage holds at 33% (34/102). See the separate coverage comment.Observations (non-blocking)
parseFormulaRule's quoted-span splitter (see inline comment) — only relevant if a rule author hand-types""escapes inRULE_VALUE.findinsideforEach) inapplyFormulaRules,findOverwrittenCells, andclassifyRowis O(rows²) per call. Fine for typical editor row counts; worth a Map-based lookup if very large staging tables ever become a supported use case.No blocking issues
The feedback-fix commits are correct, tested, and don't introduce regressions. Approving.
Reviewed by Hermes Agent
Performance note (non-blocking): the
dataSourceUnchanged.find(...)inside thedataSource.forEach+for (const formulaRule ...)loop is O(rows × formulaRules × unchangedRows). The same PK-matchingfind-inside-forEachpattern appears infindOverwrittenCells.tsandclassifyRow.ts. Fine for typical editor row counts; if very large staging tables become a supported use case, a one-timeMapkeyed by the joined PK values would drop this to O(rows × formulaRules).Low-priority edge case: the
quotedSpanPatternregex"[^"]*"|'[^']*'does not handle escaped double-quotes ("") inside a double-quoted span — it stops at the first inner", so a literal like="john""doe"is split into two matches ("john"and"doe") rather than one span.This is low-impact because the splitter runs on the ORIGINAL rule text (before
quoteLiteralproduces""escapes), so it only bites if a rule author literally types a""escape inRULE_VALUEalongside aDC.*token. If it ever matters,/(?:"(?:""|[^"])*"|'[^']*')/gwould handle the escape.Hermes Agent Code Review
Verdict: Approve
This review covers the new commits since the last review (
9b2e0df→bf54a58): commit36963aa"fix: address hermes review feedback (formula quoting, cell revert, addRow guard)" and its merge.Summary of new changes
The incremental commits address three pieces of prior review feedback:
Formula literal quoting fix (
parseFormulaRule.ts):quoteLiteralnow escapes embedded double-quotes ("→"") so aDC.USER_NAME/DC.ORIG_VALUEcontaining a"produces a valid spreadsheet literal instead of a broken formula. Two new spec cases coverjohn"doeanda "quoted" value. Correct and well-tested.Cell revert special-missing handling (
resolveRevertedCellValue.ts, new): Reverting a numeric column now falls back to the raw text whenNumber(rawValueText)isNaN(e.g. a SAS special missing like.S), instead of silently writingNaNinto the cell. The editor's revert callback now calls this helper instead of the inlineisNumericCol ? Number(rawValueText) : rawValueText. Three new spec cases cover non-numeric, valid numeric, and special-missing. Clean extraction.addRow guard (
editor.component.ts): Afterhot.alter('insert_row_below', ...)thethis.dataSource[newIndex].noLinkOption = trueassignment is now guarded withif (this.dataSource[newIndex]), defending against the data binding ever becoming async. Defensive and correct.Doc note (
detectLicenceKeyProtocolMismatch.ts): Added a comment documenting that the HTTP/HTTPS detection is a structural assumption about the (separate) key-generation repo. Good documentation of a cross-repo coupling.Checks performed
quoteLiteralescaping is correct:String(value ?? '').replace(/"/g,'""')produces valid""-escaped literals, matching the two new specs.resolveRevertedCellValueusesNumber.isNaN()(not looseisNaN) — correctly distinguishesNaNfrom numeric coercion quirks.editor.component.tspasses the samerawValueText/isNumericColthe inline code did — no behavioural regression.seedFormulaValuesForRow/updateEditStatusForRowcalls that follow it.npx @sasjs/cli c -t server-ci) succeeds; overall coverage holds at 33% (34/102). See the separate coverage comment.Observations (non-blocking)
parseFormulaRule's quoted-span splitter (see inline comment) — only relevant if a rule author hand-types""escapes inRULE_VALUE.findinsideforEach) inapplyFormulaRules,findOverwrittenCells, andclassifyRowis O(rows²) per call. Fine for typical editor row counts; worth a Map-based lookup if very large staging tables ever become a supported use case.No blocking issues
The feedback-fix commits are correct, tested, and don't introduce regressions. Approving.
Reviewed by Hermes Agent
Performance note (non-blocking): the
dataSourceUnchanged.find(...)inside thedataSource.forEach+for (const formulaRule ...)loop is O(rows × formulaRules × unchangedRows). The same PK-matchingfind-inside-forEachpattern appears infindOverwrittenCells.tsandclassifyRow.ts. Fine for typical editor row counts; if very large staging tables become a supported use case, a one-timeMapkeyed by the joined PK values would drop this to O(rows × formulaRules).Low-priority edge case: the
quotedSpanPatternregex"[^"]*"|'[^']*'does not handle escaped double-quotes ("") inside a double-quoted span — it stops at the first inner", so a literal like="john""doe"is split into two matches ("john"and"doe") rather than one span.This is low-impact because the splitter runs on the ORIGINAL rule text (before
quoteLiteralproduces""escapes), so it only bites if a rule author literally types a""escape inRULE_VALUEalongside aDC.*token. If it ever matters,/(?:"(?:""|[^"])*"|'[^']*')/gwould handle the escape.Test Coverage Report
SASjs compile/coverage run via
npx @sasjs/cli c -t server-ci(local compile only — no server required).Notes
server-ci(output insas/sasjsbuild).ERRORaboutstreamConfig.webSourcePath: '.../client/dist'not existing — this is expected, since the Angular client was not built in this review environment, and does not affect the coverage figures above.exportdb.test.sas,refreshcatalog.test.sas) are reflected in the 22 covered services.mpe_targetloader.test.sas(REPLACE loadtype) is registered as a test macro; the CAS-specific branch ofmpe_targetloader.sasis explicitly noted in that test as not exercised (BASE engine only).Generated by Hermes Agent
symget('_contextname') is concatenated directly into the PRX replacement string in makedata.sas. A context name containing \ or $ would be interpreted as a backreference or variable interpolation in the Perl regex replacement. Escape both characters with a leading backslash in a data step before embedding the value in the prxchange call.Hermes Agent Code Review
Verdict: Approve
This review covers the 8 new commits since the last review (
bf54a58→2dfeb02): the Viya deploy configurator rewrite (df2027d), the regex-metacharacter escaping fix inmakedata.sas(9023eb2), the@sasjs/coremajor bump withmx_testservice/mp_execute→mx_executetest renames (6527c10+ merge), debug-comment clarification and model extension (72484ae), and the packages/deploy chore (2dfeb02).Critical
None.
Warnings / Suggestions
prxchangedelimiter not escaped —makedata.sas:236. The\and$escaping is correct and closes the backreference/interpolation gap, but the substitution uses|as its delimiter and a context name containing a literal|would terminate the replacement early. Low probability (Viya context names rarely contain|), but see the inline comment for a one-line fix.@for—automatic.component.html:218.getFilteredComputeContexts()is invoked on every change-detection cycle. Binding to a cachedfilteredComputeContextsarray (updated in the existing handler methods) avoids a freshfilterallocation each tick. See inline comment.sasjsconfig.json:146. The newnextviyatarget hardcodeshttps://nextviya.emea.sas.comand/Users/viyademo18/dc. Other targets already hardcodesas.4gl.io, so this follows the existing pattern, but it does expose an internal SAS demo host. Confirm this target is intended to ship in the public repo (it's referenced by anextviyanpm script inpackage.json).Looks Good
$(automatic.component.ts:643):String.replacenow uses a replacer function() => \contextname="${computeContextName}"`soin the context name is treated literally. Correct fix for the prior string-replacement bug where&/$1/$`patterns would mangle names containing$`.getComputeContextDetailsparallel fetch (automatic.component.ts:167): fetches each context's full details viaPromise.all, with a graceful error branch that resolvesrunAs: null, reusable: falseinstead of rejecting. Good resilience — one unfetchable context won't block the whole dropdown.getMemberAdminGroupsfallback (automatic.component.ts:351): on memberships-fetch failure, falls back to showing all groups. Sensible degraded behaviour.onComputeContextChangeordering note (automatic.component.ts:405): correctly synchronisesselectedComputeContextbefore derivingrunningAsUser, with a clear comment explaining thengModelChangetiming. TherunningAsUserwill now match the dropdown on the same change, not lag by one.ComputeContextDetailsmodel (viya-compute-context-details.model.ts):attributesfields made optional with?andallowXCMD/sessionInactiveTimeoutadded, plus a newEnvironment.autoExecLines. Aligns with the real Viya API which doesn't always populate every attribute.getCurrentUserGroupMemberships(sas-viya.service.ts:174): clean addition following the existinggetAdminGroupspattern;limitis a number default, not user input, so no injection surface..test.sasfiles +testsetup.sas+buildtermviya.sas): mechanicalmx_testservice/mp_execute→mx_executerename driven by the@sasjs/core5.0→5.1 bump. All renames are consistent.npx @sasjs/cli c -t server-ci) succeeds; overall coverage holds at 33% (34/102). See the separate coverage comment.Reviewed by Hermes Agent
Calling
getFilteredComputeContexts()directly in the@forexpression means Angular re-invokes the method on every change-detection cycle (it can't know the return value is stable). For a small list this is harmless, but the idiomatic and cheaper approach is to bind to a cached array property (e.g. updatethis.filteredComputeContextsinsideensureSelectedComputeContext()/onShowAllContextsChange()and iterate over that). This avoids a freshfilterallocation on every CD tick.This target hardcodes an internal SAS demo host (
nextviya.emea.sas.com) and a demo user path (/Users/viyademo18/dc). It's referenced by thenextviyanpm script insas/package.json. Other targets already hardcodesas.4gl.io, so this follows the existing pattern, but confirm this internal host is intended to ship in the public repo.The escaping covers
\(backreference) and$(interpolation), but theprxchangesubstitution uses|as the delimiter:A context name containing a literal
|would prematurely terminate the replacement string and shift the remainder into the flags section, either erroring or producing a malformedDC.html. Viya context names rarely contain|, so this is low-risk, but for completeness the delimiter itself should also be quoted in_ctxesc(e.g. add atranwrd(...,'|','\\|')), or switch the delimiter to a character unlikely to appear in a name.Test Coverage
Services: 22/60 (37%) | Macros: 12/42 (29%) | Overall: 34/102 (33%) — run
2026-08-19T10:14Zon2dfeb02fafPR diff
Generated by Hermes Agent
- Paste a formula using column names (e.g. "=A_COL * B_COL") into any cell and have it translated to that row's cell references ("=B4 * C4") so HyperFormula can evaluate it - covers both a grid-level paste and pasting directly into an open cell editor - Only applies on tables that already have formulas enabled (an existing HARDFORMULA/SOFTFORMULA column); otherwise the pasted text is left untouched rather than becoming inert translated text - New substituteColumnReferences() reuses parseFormulaRule's boundary-matching helpers but deliberately skips DC.* variable substitution, which only makes sense for admin-defined rule valuesPR #294 Review — Version 7.13 Release
Security
1.
getstagetable.sas(lines 24, 26, 41) —&table_idused inlibnameandsetstatements without validationtable_idcomes frommf_getvalue(work.iwant,table_id)(user-supplied via the web request). It's used directly in alibnamepath, asetstatement, and a SQLwhereclause — all classic SAS macro injection vectors. Iftable_idcontains spaces, quotes, or%-led macro triggers, this is exploitable. This pattern pre-exists in the file but the new code (lines 37-48) adds another unvalidated use of&table_idin awhereclause and passes&base_lib/&base_ds(also frommpe_submit, user-influenced) into%dc_assignliband%mp_getcols. Recommend validatingtable_idagainst a whitelist (e.g. exists inmpe_submit) before any use, or at minimum wrapping with appropriate sanitisation.2.
makedata.sas(lines 254-257) —prxchangeregex substitution of_contextnameinto DC.htmlThe escaping on line 236 handles
\and$but not|— the delimiter used in theprxchanges|...|...|pattern. A context name containing|would break the regex or inject a replacement. Low likelihood (context names are admin-controlled) but worth noting.Correctness
3.
sas.service.ts(lines 442, 460-465, 477-483, 526-529) — ~10console.logstatements added to production codeMultiple verbose
console.logcalls were added tocheckViyaDeployandviyaMakedataSuccessfull. These will spam the browser console in production. Some log internal config objects (sasjsConfig). Should be removed or gated behind a debug flag / the existingLoggerService.4.
sas.service.ts(line ~560) — error handler now silently resolvesfalseinstead of rejectingThe previous code called
reject()on folder-members fetch error. Now it resolvesfalse(redirects to /deploy). The comment says "assume setup is needed" but this masks real API failures (auth, network, permissions) as "needs setup" — the user gets sent to a deploy screen for an unrelated error.5.
editor.component.ts—dataSourceRawuses array-index alignment withdataSourceoverlayFormulaRawValuesOnUnchangedand the escape loop ininitSetupboth accessthis.dataSourceRaw[rowIndex]by index. ButdataSourcecan be reordered by sort/insert/delete after load. The PK-matched lookups infindOverwrittenCellsandsyncOverwrittenCommentForCellare correct, but these index-aligned accesses will drift if any reordering happens betweendataSourceRawcapture and their use. The escape loop runs immediately after capture so is likely safe, butoverlayFormulaRawValuesOnUnchangedis called fromeditTable()which can run after sorts.6.
chunk_deploy.py(line 21) —header = lines[:HEADER_END]with no None checkIf
HEADER_ENDstaysNone(no%let path=or%let service=line found),lines[:None]returns the entire file as "header", andi = HEADER_ENDon line 29 setsi = Nonewhich throwsTypeErrorin thewhileloop. Should add a guard:if HEADER_END is None: sys.exit("No header marker found").7.
chunk_deploy.py(line 63) —web_sectionscomputed but never usedweb_sectionsis computed on line 63 but the actual filtering on lines 67-73 recomputesweb_file_sectionsandimages_sectionsfrom scratch.web_sectionsis dead code.Tests
8. No test for
chunk_deploy.py— the Python deploy script has no tests and has at least two bugs (above). Given it's a deploy-time utility, at minimum a smoke test that it handles a missing header gracefully would help.Observations
mp_abort(mode=INCLUDE)pattern replacing the oldmf_abort.errorfile check across 6 services is a clean simplification.EDIT_STATUScolumn approach (dc.row_statuswith a period to prevent SAS name collision) is well-reasoned, as is thedataDotNotation: falsesetting to make it work with HyperFormula.max(selectbox_rk)+1rather than%mf_increment, appropriate for a standalone migration.Coverage
See review comment above.
Code Review
Verdict: Approve — no changes needed.
Reviewed the incremental commit
1f4aa6f(hardening following PR review feedback). Thegetstagetable.sasinput validation is well implemented —table_idis validated withmp_validatecol(…,ISNAME,…)andbase_libdswithLIBDSbefore any use inlibname/set/wherestatements. The@sasjs/corebump to 5.2.0 adds theISNAMErule and the lockfile is correctly updated. Thechunk_deploy.pyHEADER_END is Noneguard and deadweb_sectionsremoval are clean. The newgetstagetable.test.sascovers the end-to-end stage→fetch flow.Reviewed by Hermes Agent
Test Coverage
Services: 23/60 (38%) | Macros: 12/42 (29%) | Overall: 35/102 (34%) —
2026-08-25T15:38:18Zon1f4aa6fcePR diff
Generated by Hermes Agent
rejection.js defined its own local saveTableData(tableName, tableData) with 2 args, but dcMockUtils.js (loaded via eval) also defines saveTableData(dataDir, tableName, tableData) with 3 args. The dcMockUtils version was shadowing the local one, causing saveTableData('MPE_SUBMIT', submitData) to pass the table name as dataDir and the data object as tableName, crashing with 'TypeError: tableName.toLowerCase is not a function'. Removed the local saveTableData and updated calls to use the dcMockUtils 3-arg signature: saveTableData(dataDir, tableName, data).startupservice.js only returned LIBREF in saslibs, but the dc-tree component renders LIBRARYNAME — without it the nav tree node text was empty, so libraryToOpenIncludes could not match. Added LIBRARYNAME, LIBRARYID, and ENGINE to match the viewlibs service format. Changed libraryToOpenIncludes_SASJS from 'dc' to 'dc_jslib' to avoid matching multiple libraries (DC_JSLIB and DC996664 both contain 'dc'). viewdata.js: removed alphabetical .sort() on cols (SAS preserves physical column order via mp_getcols); added empty row with correct SAS missing values ('.' for numeric, '' for character) when a table has no data, so the viewbox grid can derive column headers.Code Review — 4gl-reviewer
Summary: Large v7.13 release PR (236 files) covering licence key refactoring, editor formula rules, viewbox resize handles, CAS REPLACE load type, and
getstagetableinput validation. The security hardening ongetstagetable.sasand the CAS-aware REPLACE path inmpe_targetloader.sasare well done. A few issues below.Issues:
sas.service.ts:449] — Multipleconsole.logstatements left in production code (checkViyaDeploy: checking appLoc,appLoc exists?,makedata job already run?, etc.). These should useloggerServiceor be removed before release — they'll spam the browser console for every Viya deployment check.sas.service.ts:593] — ThegetFolderMemberserror handler changed fromreject()toresolve(false). While "assume setup is needed on error" is reasonable, this silently swallows API errors — if the Viya folder API is down, the user is redirected to/deploywith no indication of the real problem. Consider at least logging the error vialoggerService.error()before resolving.combinedLicenceKey.ts:54] —DecompressionStreamis a relatively new Web API (Chrome 80+, not available in older browsers or Node < 18). If this code runs in any non-modern-browser context, gzip decompression will throw. Consider a feature check or polyfill fallback.mpe_targetloader.sas— CAS REPLACE path] — The temp tableCASUSER.&tmpdsis created and loaded before thesyscc>0check, but if thedatastep that populates it fails mid-way, theproc sql; drop table CASUSER.&tmpds;quit;in the error path runs — which is good. However, the non-CAS REPLACE path's error check (syscc>0) only unlocks the table; it doesn't clean up anyWORK.temp datasets created earlier in the macro. Verify thatWORK.&STAGING_DSand related temps are cleaned up by SAS session termination or explicit drops elsewhere.Suggestions:
decodeLicenceFeatures.ts:35-46] — ThedecodeFeaturesObjectuses??to default numeric fields toInfinity. This is correct, but the boolean fields (vb,fu,er,ar) are passed through without coercion — if the object hasundefinedfor these, the licence state would haveundefinedbooleans rather thanfalse. Consider!!features.vbfor safety.automatic.component.ts:167] —getComputeContextDetails()fires N parallel API calls (one per compute context). For servers with many contexts, this could be slow or hit rate limits. Consider whether the batch identity is needed for all contexts or just the selected one.getstagetable.sas] — Thetable_idvalidation withmp_validatecol(ISNAME)is excellent and prevents code injection. Consider also validatingbase_libandbase_dsoutput frommpe_submitwith the same macro before using them in%dc_assignliband%mp_getcols(theLIBDScheck is used, which is good, but double-check it covers all injection vectors for librefs/dsnames used in macro variable resolution).Looks good:
getstagetable.sasinput validation is a genuine security improvement —table_idwas previously used unsanitized inlibnameandsetstatements.mpe_targetloader.sascorrectly usestable.deleteRows(CAS doesn't support SQL deletes) and places the error check before the destructive truncate.dc-validator/utils/,licensing/utils/, andviewboxes.component.spec.ts.Code Review — 4gl-reviewer
Summary: Large v7.13 release PR adding formula column support (HARDFORMULA/SOFTFORMULA), combined licence keys, CAS engine REPLACE load type, editor row edit-status tracking, and substantial refactoring with well-tested extractions. Overall quality is high with strong test coverage, but a few items should be addressed before merge.
Issues:
console.logstatements were left incheckViyaDeploy()andviyaMakedataSuccessfull(). These are debug-level logs (checkViyaDeploy: checking appLoc,viyaMakedataSuccessfull: checking folder, etc.) that will spam the browser console in production. Remove them or gate behind a debug flag.subprocess.run(..., timeout=timeout)can raisesubprocess.TimeoutExpiredwhich is not caught. An uncaught exception will crash the deploy script with a traceback instead of a clean failure message. Wrap in try/except to print a meaningful timeout error.&syscc>0check dropsCASUSER.&tmpdsbetween the UNLOCK and the%mp_abortcall. If the drop fails (e.g. session state issues),&syscccould escalate, but%mp_abortis already conditioned on the prior&sysccvalue so this is low-risk. Consider moving the cleanup to after the abort guard for clarity.Suggestions:
bypassSecurityTrustHtmlis used for all error messages. The comment correctly notes the content is hardcoded (no user input), so this is safe today, but consider keeping a code comment or lint rule to prevent future developers from interpolating user-supplied strings into these messages.JSON.stringifycomparison for change detection is fragile (key ordering,undefinedvs missing properties). It matches the existing inline logic it was extracted from, but consider a deep-equals utility for future iterations.any[]return type is acknowledged in the doc comment (Handsontable's SortConfig isn't publicly typed). Acceptable pragmatic choice; add a// TODO: type when HT exports SortConfigfor tracking.Looks good:
classifyRow,expandCellRanges,combinedLicenceKey,decodeLicenceFeatures,syncOverwrittenCellComment, and the integration spec for sorted-grid row sync are all well-tested with edge cases.mpe_targetloader.sasmirrors the established pattern frombitemporal_dataloader.sas, including the pre-destructive abort check and varchar casting.mp_abort(mode=INCLUDE)additions acrosspostdata.sas,getdata.sas,getdynamiccolvals.sas,loadfile.sas, andrestore.sasare a clean fix for hook-script abort propagation.decodeLicenceFeaturesrefactor cleanly supports both legacy positional strings and new named objects without a version flag.Code Review — 4gl-reviewer
Summary: Large v7.13 release introducing formula validation rules (HARDFORMULA/SOFTFORMULA) with HyperFormula integration, combined licence key support, CAS table handling in mpe_targetloader, security hardening in getstagetable.sas, and extensive utility extraction with good test coverage. Overall well-structured with thoughtful documentation.
Issues:
sas/sasjs/macros/mpe_targetloader.sas:~180] — CAS branch doestable.deleteRows where="1=1"(full truncate) before thedata ... append=yesstep. If the append fails, the target table is left empty with no rollback. Consider wrapping in a transaction or staging a backup that can be restored on failure.client/src/app/services/sas.service.ts:449-525] — Many verboseconsole.logstatements (e.g.'checkViyaDeploy: checking appLoc','viyaMakedataSuccessfull: folder members:') appear to be debug logging left in production code. These should useloggerServiceor be removed/gated behind a debug flag.client/src/app/shared/dc-validator/utils/applyFormulaRules.ts:30] and [findOverwrittenCells.ts:31] — Both dodataSourceUnchanged.find(...)inside aforEachloop, giving O(n×m) complexity for PK matching. On large tables this could be slow. AMapkeyed by PK values would make each lookup O(1).client/src/app/licensing/licensing.component.ts:70-90] —bypassSecurityTrustHtmlis used for error messages. The comment correctly notes the content is hardcoded, but this pattern bypasses Angular's XSS protection — if any of these strings ever incorporate user-derived data in the future, it becomes an injection vector. Worth a follow-up to render<clr-icon>via Angular templates instead.sas/sasjs/db/migrations/20260807_v7.13_release.sas:17] —select max(selectbox_rk) into: maxrkthen&maxrk+1/&maxrk+2will produce.+1(a SAS error) ifmpe_selectboxis empty. The makedata equivalent uses%mf_increment(rk)which is safer. Consider adding a%let maxrk=0;default before the select.Suggestions:
client/src/app/services/utils/decodeLicenceFeatures.ts:64] —parseInt(codeBit)is called without a radix argument. While moved unchanged from the original,parseInt(codeBit, 10)would be more defensive against leading-zero strings.sas/sasjs/utils/chunk_deploy.py:140] —sys.exit(1)on the first chunk failure means a partial deployment with no rollback. Consider logging which chunks succeeded so the operator knows where to resume.client/src/app/deploy/sections/automatic/automatic.component.ts:165] —getComputeContextDetailsfires one API call per context in parallel. Environments with many contexts could hit rate limits; consider batching or at least logging a count of contexts being fetched.Looks good:
.spec.tsfile with meaningful test cases.getstagetable.sassecurity hardening (table_id validation viamp_validatecol+ whitelist check againstmpe_submit) is a solid injection-prevention pattern.parseFormulaRule.tscorrectly skips quoted string spans during variable substitution and uses function replacements to avoid$interpretation — well thought out.classifyRow.tscorrectly excludesEDIT_STATUSfrom the diff comparison to avoid self-perpetuating 'M' state.resizeFromLeftEdgein viewboxes.component.ts has an excellent explanation of why raw pointer coordinates are used instead of getBoundingClientRect.DCKEY1:prefix, unambiguous with base64 alphabet) is clean and well-documented.