Hermes Agent Code Review (new commits 45226e1, 003550b)
Critical: _debug=131 is still hard-enabled. The comment says it will be turned off before final release, but there is no guard (env flag, build conditional, or linked TODO) to ensure that happens. Please revert to ' ' (debug off) or gate behind environment.production ? ' ' : '&_debug=131' so debug output only appears in dev builds. This is the only blocking item.
Carried over (non-blocking): const anyDetails: any = details bypasses the ComputeContextDetails type to probe runAsUserId / environment.runAsUserId. If these fields appear on Viya responses, extend the model so the fallbacks are type-checked.
Carried from first review: symget('_contextname') is concatenated directly into the PRX replacement string. A context name containing $ or \ would be interpreted as a backreference. Practical risk is low (context names are plain identifiers), but confirm context names can never contain $/\, or escape the value before embedding.
Review feedback has been addressed in PR #307 (pushed from a fork since the hermes token does not have write access to dc/dc directly).
Changes:
- Debug mode comments improved to explain the…
Hermes Agent Code Review (new commit df2027d)
Suggestion: anyDetails: any = details bypasses ComputeContextDetails to probe runAsUserId / environment.runAsUserId. If these fields are part of the Viya response, model them on Attributes/ComputeContextDetails (as was done for runServerAs/reuseServerProcesses) so the fallbacks are type-checked rather than any-probed.
Suggestion: getFilteredComputeContexts() is called in the @for expression, so it re-runs (filter + new array) on every change-detection cycle. Consider caching the result in a property updated by onBatchIdChange / onShowAllContextsChange / getComputeContextDetails and binding the @for to that property instead.
Warning: font-size: 0.55rem (≈8.8px) for .deploy-field-description is unusually small for the help text beneath each deploy field, and combined with opacity: 0.75 it will be hard to read. This looks like a typo — 0.85rem or 0.75rem would be more in line with Clarity's body text. Please confirm the intended size.
Critical: debug is hard-enabled here ('&_debug=131') with the off-variant commented out. The previous reviewed commit had _debug = ' ' (off). This appears to be a debug leftover — every makedata run from this tab will now stream the full debug log. Revert to ' ' unless this is intentionally toggled elsewhere.
Test Coverage Report
Ran npx @sasjs/cli c -t server-ci from sas/sasjs/ against this PR branch (commit 6527c10).
Result: SAS compile succeeded.
ANCHOR TEST: regex metachar concern at line 249
Suggestion: the new DC.html context-rewrite logic (lines 214-267) has no test. testsetup.sas runs makedata but doesn't assert that contextname="..." in DC.html was updated to &_contextname. Consider reading DC.html back after makedata and asserting the contextname= value equals &defaultcontext.