The case took the id from the stage route and looked for a download button
carrying it. For an upload the route does not hold a table id at all - it holds
a load reference, the CI failure naming DC20261031-201657.470_601010_4449 - so
the button it looked for could never exist, however long it waited or however
many times the screen was reloaded. That is why the row "never appeared" while
the approval was in fact there.
The row is now found the way hook-programs finds its own: by the table it names.
The id the download is named after is read from the button, which is the only
place that knows it.
Verified against the mock estate locally, with the licence the spec now plants:
7 passing, exit 0.
The estate ships no licence key, so it runs on the free tier, and the free tier
disables file upload - which is exactly what the audit case needs. Whether it
could run depended on another spec having planted a key earlier in the run: the
key persists in the estate config, so the case passed or failed on spec order.
loginAndUpdateValidKey(true) plants one here. csv-limited, which asserts the
free-tier modal, runs first in cypress-specs.sh and is unaffected; download runs
last, so nothing downstream inherits the key.
Found by running the spec against the mock estate locally: the upload input
never rendered, the modal read "Locked Feature (File Upload) ... site id (123)",
and the run went 7/7 green once the key was planted.
mpe_abortmessage existed to assemble the message mpe_execute aborts with - the
program, the state and the child message - inside a data step, so that a percent
sign, an ampersand or a quote in the child message was not macro resolved on the
way into a symbol. mpe_execute already runs a data step over the status row for
state and errmsg, so the text is built there now and the helper, plus the %let
fallback that duplicated it, is gone.
What the helper did is kept: the values come from the status row through
symget/symputx, never through %let, and a message already at the 32767 limit is
cut to the room the prefix leaves.
The comma and percent cases its tests covered now live in core's mx_execute
test, where the message is captured from a real child; what stays here is the
assembly DC owns - the prefix, the state and the child message from the status
row.
The audit-file case uploads a fixture and then looks for the approval it should
create. An upload identical to what the table already holds produces no
changes, and a changeset with no changes leaves nothing waiting for approval -
so the case finds no row. A retry lands in exactly that state: the first
attempt's rows are already in the table, so the second upload is a no-op and the
failure looks like a missing approval rather than a duplicate upload.
The fixture is now copied per attempt with its primary keys renumbered from a
random base, so the upload is always an insert. The copy is removed in
afterEach - deleteFolder cannot be used there because the source fixture lives
in the same directory.
Core 7.0.3 fills outstatus.errmsg on both platforms - the SASJS branch since
7.0.2, the Viya branch from the job result as of 7.0.3 - so a canceled call now
always carries the reason it was canceled.
That removes the last platform difference from the assertion: while Viya wrote a
blank errmsg, an expected message could only be checked on one platform and the
state alone had to stand in on the other. A comparison against a message that
was never recorded now fails instead of passing silently, which is the point of
asking for it.
The macro decided an aborted call by scanning the child's response text for a
sasjsAbort object and then recovering the message from that text - anchors for
the separator, gating on whether the payload was escaped, cuts for the log
extract, and a capture width that had to be widened more than once. Every one
of those steps existed to undo the escaping in the response.
Core now publishes the abort reason in outstatus.errmsg for both the SASJS and
Viya branches, with the whole message and no truncation, so the macro reads the
row instead: STATE says whether the child was canceled, ERRMSG is the reason.
The cut at the appended log extract is the only thing left, because that text is
part of the message the child raised; it is stripped as characters, as an
escaped pair or as a doubled pair, whichever the producer wrote.
The macro therefore takes outstatus= rather than outref=, and the call sites in
the service tests now pass the status dataset they already asked %mpe_execute
for. stagedata.test.4's two aborting calls had no status row at all and now
have one.
A platform that reports the state but no message - SAS Viya until core fills
errmsg from the job result - is judged on the state alone, so the same tests run
on both platforms.
@mpe_assert_abort.test covers the twelve cases directly against status rows
instead of building response files, including the doubled separator the estate
actually writes, a message past the old 256 character width, quotes and
backslashes.
Requires @sasjs/core 7.0.2, which carries the SASJS half of this.
The upload and submit now work; the failure moved to the approve grid, which is
loaded when the screen opens and does not yet contain the row just submitted.
The screen is reloaded up to five times, two seconds apart, until the row with
this test's id is present.
The mock getddl aborts when ds is missing, as production does (getddl.sas:43-45),
instead of quietly returning a library-level DDL - a client that dropped ds would
otherwise stay green against the mocks.
The comment above the case no longer claims a leftover row cannot matter: the
grid paginates, so a row pushed past the first page would not be found.
The upload does parse now - CI showed the info modal appearing - but the helper
treated it as an abort because the info modal and the abort modal are the same
component (.clr-abort-modal). The title is what separates them: a good upload
raises "Table Found", so that is asserted and anything else fails with its own
text instead of being clicked through.
After Submit, the stage route is already current, so waiting on the URL proved
nothing; the editor leaving preview mode is the signal that the submit was
accepted. Also trims the comment that narrated the old route.
The first case failed in CI with "Expected to find element:
input[type=file]#file-upload", because the editor renders that input inside its
upload modal - the Upload button in the button bar opens it. Clicking the button
by its label rather than by position, as excel.cy.ts does.
The viewer download cases (2-7) passed, so the rest of the spec is sound.
Follow-ups from the review of #414.
The credential guard now checks each staged .npmrc rather than only the client
one: the root and sas/ files are tracked policy as well, so a token pasted into
either was equally one `git add -A` away. Verified by staging a token in
sas/.npmrc - the hook exits 1.
$RUNNER_TEMP/npmrc is removed at the end of each job that writes it, with
`if: always()` so a failed job cleans up too.
The root `install` script is gone: the root .npmrc sets ignore-scripts, so npm
never ran it. CONTRIBUTING already spells out the per-project commands, and is
now the single source.
The release workflows build the published client from the same tree, so they run
`check:archiver` like build.yaml does.
readdir-glob reaches minimatch twice: the directory walk, and the pattern/ignore
matching behind archiver's glob(). The second is where a semantic change shows
up as silently skipped files rather than an error, so the check archives a tree
by pattern with an ignore and asserts what it kept: a.txt in, sub/** out.
client/.npmrc is tracked now, so a token left in it from the old pattern is one
`git add -A` from being committed. Prose in CONTRIBUTING is not a control: the
pre-commit hook fails when the staged file holds _authToken, _auth or _password.
`npm run check:archiver` runs the directory-walk guard, so it is reproducible
before a push instead of only in CI, and the CI step calls the same script.
CONTRIBUTING now spells out the per-project installs, since the root .npmrc has
ignore-scripts=true and the composite install script therefore never runs.
unzipper was only there to read the test archive back. archiver emits an 'entry'
event per file it adds, and those names are exactly what the walk produced, so
the guard asserts on them and needs no reader. archiver stays: it is the package
whose directory() walk reaches readdir-glob, and the import has to be pinned to
the version the override installs.
Four write steps interpolated ${{ secrets.NPMRC}} straight into the shell source,
so a quote or $( in the value would break the step or run. They use the env form
the same workflows already use elsewhere.
CONTRIBUTING.md now says what the tracked client/.npmrc means for a developer:
run `npm run postinstall` after installing, `npx cypress install` for the suite,
and keep registry credentials in the user config rather than in the tracked file.
The release workflow runs cypress against the built app, and its install steps
now inherit ignore-scripts from the tracked client/.npmrc - so the binary has to
be installed explicitly there too, as build.yaml does.
unzipper is pinned to 0.12.5 so the exceljs override block no longer mixes exact
pins with a range.
client/.npmrc is tracked now, and every workflow appended the NPMRC secret to it
inside the workspace - one `git add -A` from committing a live token, and
semantic-release runs in jobs that have already written it. The secret goes to
$RUNNER_TEMP/npmrc and NPM_CONFIG_USERCONFIG points the installs at it, so the
project file stays policy-only.
The archiver guard used a fixed path in the shared tmpdir and had no handler on
the write stream: two jobs on one runner collided on the path, and a write
failure surfaced as an uncaught throw. Each run now gets its own directory, both
streams report, and the tree is removed afterwards.
The exceljs override pins archiver to 8.0.0 rather than ^8.0.0, so the version
the guard was validated against cannot drift.
The committed client/.npmrc turns dependency scripts off everywhere, not just in
build.yaml - and every one of these workflows builds the frontend, which imports
src/environments/version.ts. The lighthouse job failed exactly there:
Error: Cannot find module '../../environments/version'
The generator now runs after each client install: build (with cypress install),
lighthouse, sites, docs, release-assets and the three release installs.
Three things the last run exposed.
Every workflow that writes the NPMRC secret used `>`, which now overwrites a
tracked policy file - so the release, lighthouse and sites jobs would install
with hooks enabled and ranges unpinned, the opposite of what this PR adds. They
append.
Cypress downloads its binary in a dependency install hook, so ignore-scripts
leaves it out: "The cypress npm package is installed, but the Cypress binary is
missing". Both client installs now run `npx cypress install` after the
generator.
The archiver guard imports archiver and unzipper, which were only present as
overrides under exceljs; they are declared devDependencies at their resolved
versions so the script owns what it imports. Both remain production packages
because exceljs still requires them.
Dependency install hooks now stay off in client/, as they already are at the root
and in sas/: client/.npmrc is committed with ignore-scripts and save-exact, the
root install script runs `npm run postinstall` after installing, and both CI
installs pass --ignore-scripts and then run the generator by hand. The client's
own postinstall is what creates src/environments/version.ts and the hot icons, so
it has to run - it is third-party hooks that do not.
CI writes client/.npmrc from the NPMRC secret; the second job now appends rather
than overwrites so the tracked policy survives (the first already appended).
The archiver guard reads the archive back instead of checking its size: a matcher
that takes the top-level file and misses the nested directory clears any size
threshold, and that is exactly the failure it exists to catch. It now asserts
a.txt and sub/b.txt are both present.
archiver 8 is ESM-only, so require('archiver') resolves through require(esm) and
that exists from Node 22.12 - a supported Node 18/20 would fail the check for a
reason that has nothing to do with the minimatch override. The script is .mjs and
imports ZipArchive.
The cut already tells the two payload shapes apart - the doubled separator means
mp_abort escaped the message, the single form means it did not - so the unescape
is gated on the same decision. Unescaping a payload that was never escaped
collapsed a genuine pair of backslashes, which is a UNC path or a regex in a hook
message, and the comparison then ran against text the hook never raised.
The over-long message cut clamps the room to zero first: a prefix long enough to
fill the buffer gave SUBSTR a negative third argument, which errors the step and
cancels the program the helper exists to explain.
programvar=/statevar=/msgvar= names that do not resolve now say so with a
WARNING, instead of reaching the abort text as a blank.
Two cases: an unescaped payload keeping its double backslashes, and a message at
the 32767 limit being cut rather than overflowing. The trailing-backslash case
now uses the fully escaped form, which is what the producer emits.
The client's postinstall generates src/environments/version.ts (and the hot
icons), which the app imports - with --ignore-scripts the build fails with
"Can't resolve '../../environments/version'". My local check missed it because
version.ts was already on disk from an earlier install.
So the client keeps its install scripts; the archiver guard stays, which is what
that commit was for.
The minimatch override forces a major above readdir-glob's own range, and
archiver's directory() is the only runtime caller of that path. scripts/
check-archiver.cjs archives a small tree and fails if the result is too small to
hold it, so the override is guarded by a run rather than by the install
succeeding.
client/.npmrc is gitignored - it is where a developer's registry credentials
live - so the repo-wide ignore-scripts policy is passed on the client install
steps instead. Verified locally: npm ci --ignore-scripts, the archiver check and
ng build all succeed.
Cutting on the marker plus a newline still truncated a message that ends a line
with "Log Extract:", and an abort whose message merely prefixed the expected one
could pass. The appended extract always emits '\n\nLog Extract:\n', so the anchor
is that whole string (doubled form first, single as fallback): a message cannot
reproduce it by accident, and nothing is cut when neither form is present, so an
unexpected layout fails loudly instead of shortening the text.
The tail strip goes with it - the leading newlines are part of the anchor - and
the test for the doubled separator now writes the fully escaped form the estate
actually emits.
mpe_abortmessage cuts an over-long message to the room left after the prefix
instead of losing its tail to the 32767 limit, and mpe_execute's fallback skips
the trailing blank when the child's message is blank, matching the helper.
The %let could never be used: it ran immediately before an unconditional call
to the helper, which overwrote the global, and on a build without the helper the
unresolved macro call does not fall through to it - the parameter text lands in
open code and the step breaks before the abort fires.
%sysmacexist now chooses between them: the helper when it is on the autocall
path, the %let when it is not. The %global stays, so both branches write the
symbol that %superq reads.
Test datasets and filerefs are numbered to match their test, so a failing
assertion can be traced back by name.
Same reasoning as the minimatch pin: two ranges in one override object, one of
them caret, means the block can still float. Exact 5.0.12, which is what the
lockfile already resolves.
An override exists to stop a later install moving the package it pins, so a
caret range defeats it: the lockfile records 11.0.3 and the range would allow
11.0.4 to arrive without review. Exact version, and the lockfile is unchanged -
it already resolved 11.0.3.
Three of the four review points; the fourth is a producer problem, explained in
the reply.
The marker is only a separator when a newline follows it, so the cut anchors on
marker+newline (doubled form first, single as fallback for a response assembled
without the escape) and takes the FIRST match. A message that mentions the
marker is not followed by a newline and survives; a log line that contains the
marker can no longer move the cut past the separator and leave extract residue.
New case: an abort whose log mentions the marker.
mpe_abortmessage reads program and state into their own variables before
building the prefix: symget() nested in cat() is typed $200 by the data step, so
a long endpoint name was truncated before the assignment.
The header no longer claims byte-for-byte retention: symputx strips leading and
trailing blanks.
Not changed: decoding \n in the message. mp_abort escapes the append as
cats(msg,'\n\nLog Extract:\n',logmsg) and then doubles every backslash, so the
newline arrives as \\n\\n - un-doubling leaves the two characters \ and n, and
the only way to turn those into a newline is also the way a literal backslash-n
in the message becomes one. That is a producer-side decision, not a consumer one.
With follow-redirects moved, the client audit still failed on minimatch
<11.0.1 (globparts amplification), reached through
exceljs -> archiver -> readdir-glob, which asks for ^10.2.2.
readdir-glob already carries an override block here for brace-expansion, so the
fix follows that shape rather than a breaking exceljs change: minimatch
^11.0.1 nested under readdir-glob. package-lock gains 23 lines, the override
line is the only package.json change.
Verified: npm audit --omit=dev exits 0 in client; ng build succeeds; an
exceljs xlsx write succeeds; and archiver ZipArchive.directory() - the caller
that walks the tree through readdir-glob - produces a valid zip.
The audit runs in root, sas and client, and sas carries its own lockfile - the
same 1.16.1 was resolving there, which is why the first attempt left CI red.
Same one-package update, no pin change.
`npm audit --omit=dev` in sas is now clean.
The audit gate fails on any runtime advisory, and this one arrived upstream:
follow-redirects 1.16.0-1.16.1 discards cross-realm sensitiveHeaders arrays and
lets custom credentials follow redirects. 1.16.2 is outside that range and inside
the ranges the tree already asks for, so this is a lockfile-only update - no pin
in package.json changes.
Regenerated with `npm update follow-redirects --package-lock-only`, which moves
that one package rather than re-resolving every floating range (an
`npm audit fix` run also bumped the whole @angular-devkit family, which is not
part of this fix).
`npm audit --omit=dev` in client: the high severity entry is gone. Four moderate
advisories remain, all through exceljs -> archiver -> readdir-glob -> minimatch,
and they need a breaking exceljs change; they are pre-existing and not touched
here.
Two of the three review points.
The fallback %let now carries %superq(msg) as well, so a build without the helper
on the autocall path still reports the child's own message rather than only the
program and state. The whole value is masked and consumed through %superq at the
abort, so the comma hazard does not come back.
A case with the doubled separator - the form the estate actually emits, six
characters once the backslashes are doubled - now covers the branch that all the
other cases, written single-escaped, could not reach.
The quote case is not fixed; see the reply.
Two comment blocks sat back to back above the same call - the click handler's
and the anchored label's. They describe one selector, so they read as one note.
Three review points.
The fallback %let now follows %global _mpe_abortmsg. Without it the %let created
a local of that name in mpe_execute while the helper wrote the global, so on the
first failed call in a session the fallback shadowed the child's message - the
symptom this branch set out to remove.
mpe_abortmessage takes names for all three inputs (programvar/statevar/msgvar),
so a comma in an endpoint path cannot split the parameter list either. The test
that passes a path containing a comma now covers that, alongside the message.
mpe_assert_abort unescapes outside the marker block: a response that carries no
appended extract is still escaped, and leaving the unescape inside the block made
the comparison fail on text that was correct.
Full suite 64/64, mpe_execute and mpe_assert_abort PASS, sasjs cbd first.
Three review points.
The table id was sliced out of the stage URL without trimming a trailing slash,
so a route rendered as /stage/<id>/ produced an id that never matched the button.
Trailing slashes are removed before use.
The options menu entry is matched on the whole label, /^\s*Download\s*$/, because
an entry like "Download web query" would make cy.contains ambiguous and it fails
outright rather than picking one.
The getddl guard covers both hosts: libref is read behind a typeof too, so a call
with either variable out of scope falls back instead of throwing.
Two review points.
The strip looped over trailing \n and \r escapes, so a message that itself ended
in an escaped newline lost that escape along with the separator and the exact
comparison failed. It now removes one fixed tail: the six characters the
separator becomes once mp_abort doubles the backslashes, or four if the extract
is not doubled. Anything the message itself ended with is left for the unescape.
mpe_execute sets _mpe_abortmsg before calling mpe_abortmessage, so a build where
the helper is not on the autocall path still aborts with the program and state
rather than an empty message.
Full suite 64/64, mpe_assert_abort PASS, sasjs cbd before the run.
Three review points.
attachExcelFile read the body immediately after attachFile, so a slow upload made
abortModal false and skipped the abort branch - the failure then arrived later as
the timeout the branch exists to avoid. It now waits for either the confirmation
button or the abort modal title, with longerCommandTimeout, and branches on which
one matched; the confirm click uses the same timeout.
getddl guards ds with typeof: the truthiness fallback only covers a ds that is
bound and empty, while a missing ds in scope throws before the fallback runs.
The submitted row test 1 leaves behind is now documented in both places. The CI job
deletes the appLoc and re-runs makedata before the specs, so it does not accumulate
between runs; a hand-run against a reused estate leaves one pending row per run, and
the selector is anchored on the id the test created, so a stale row changes nothing.
Three things, one of them a bug the review question uncovered.
- msgvar is passed explicitly at the call site, so renaming the message variable
fails loudly instead of quietly producing an abort text with no child message.
- program and state are read with symget rather than interpolated into the data
step, so a % or & in an endpoint name can no longer be macro-resolved, and a
quote cannot break the literal.
- _prefix is trimmed before the message is appended. Declared as a character
variable it arrives padded to length, so the child's message was landing past
the buffer end and disappearing - the wrapper text was right and the message
was simply not there. That showed up as the comma and blank cases failing while
the "names the program state" case passed.
The abort branch asserted the modal title did not include 'Abort Message', so
any other title let the run continue with the abort modal open and the next step
died on a timeout that never mentioned the upload. It now throws with the title,
which points at the upload as the cause.
Three review points.
attachExcelFile read '.clr-abort-modal .modal-title' unconditionally, so an
upload that raised no abort modal failed on the lookup, and the abort branch
then left the modal open for the next command to click through. It asks the body
first: no abort modal means confirm the upload; an abort modal means assert its
title is not an Abort Message and stop there.
openDownloadModal clicked the deepest element holding the text. The click
handler sits on the div[clrDropdownItem], so the item is selected by that
attribute and its text.
The spec list comment claimed download leaves nothing for later specs to trip
over. It seeds the approval it needs and that approval stays, which the comment
now says.
mp_abort appends the extract as cats(msg,'\\n\\nLog Extract:\\n',logmsg), which is
now stated where the strip happens. The strip removes whole \\n or \\r pairs, so a
separator that gains a carriage return does not leave a fragment glued to the
message; a message that ends with the marker and no appended extract is a known
ambiguity and says so in the comment.
stagedata.test.4: the comment above exp8 now records why the expected text has
no space after either colon - the child builds it as &mac.:&VAR_NM.:&reason -
so the spacing is not a formatting choice a later reader has to re-derive. The
change itself came with ebfd395 on this branch and is unchanged here.
Suite 64/64, mpe_assert_abort PASS, sasjs cbd before the run.
Two reasons the last CI run still had four failures.
test 1 timed out on label[for="file-upload"]. The label is a click target for a
hidden input; attachFile can drive the input itself, so the helper no longer
clicks anything and selects by the input's id, asserting there is exactly one.
That also answers the earlier review point about positional selectors without
reintroducing a click on a control that is not always rendered.
tests 5-7 timed out waiting for the DDL file. The mock names a DDL download
after the libref alone, while services/public/getddl.sas names it
<libref>_<ds>.ddl - which is what the viewer expects and what the spec looks
for. The mock now follows the service.
fixturePath carried a trailing slash that the template also supplies, so
attachFile was given /excels//regular_excel.xlsx. Also drops the stale duplicate
doc comment above attachExcelFile - the helper targets the editor's import label,
which is what the remaining line says.
Addresses the review on e2e9103.
- The marker is located from the end. A hook message may legitimately contain
the text "Log Extract:" and only the final occurrence is the one mp_abort
appended; taking the first cut such a message mid-sentence.
- The captured text is unescaped (\" -> " and \\ -> \) before the comparison, so
it is compared as the text the hook raised rather than as its JSON form. That
is also what makes a message ending in a backslash comparable at all: mp_abort
escapes it as two, and the strip now leaves the pair alone for the unescape to
collapse - it only removes \n pairs and a lone stray backslash that is the
start of an escape the marker cut in half. Requiring the preceding character
not to be a backslash is what keeps a real trailing backslash out of that
branch.
- mpe_abortmessage owns the name it writes, _mpe_abortmsg, instead of taking one
as a parameter, so a call can no longer overwrite a caller's global of the
same name. Each call replaces the previous value; the doc says so.
Two cases added to mpe_assert_abort.test: a message ending in a backslash, and a
message containing the marker itself. Both fail against the previous logic
(first occurrence, no unescape) and pass now. Full suite 64/64 on sas.4gl.io,
sasjs cbd before the run.
Second CI run put the spec at 2 passing / 5 failing, and the five failures were
one more stale assumption: cy.select() matches an option's text against the whole
label, and the labels carry a suffix - the option for the Excel writer is
"Excel (.xlsx)", not "Excel". Every format is now chosen by the option's value
(EXCEL, SAS, SAS_DDL, TSQL_DDL, PGSQL_DDL), which the template sets and which does
not change when a label gains or loses a qualifier.
Also the review points on this PR:
- attachExcelFile starts from label[for="file-upload"] instead of
.buttonBar button:last-child, and submitExcel no longer takes a callback that
nothing passed.
- test 1 asserts on the download button whose id is the table it submitted: the
stage URL carries the id, so the assertion names its own row and asserts one
match, instead of indexing into every match on the screen.
- the comment in cypress-specs.sh no longer claims the spec depends on approvals
left by earlier specs - it seeds its own - and says why it is appended.
Addresses the review on 5d32de9.
The text mpe_execute aborts with is now built by mpe_abortmessage, which takes
the child message by variable name and writes the result to the global symbol
table. Two reasons: a value argument would be split at the first comma before
the macro ran, which is the defect this PR is about, and having the assembly in
a macro makes the comma case testable - a test calls it directly and asserts the
text, where going through mpe_execute needs a second service aborting to observe
the call the caller itself is aborted by.
- mpe_assert_abort: a lone trailing backslash is no longer stripped. Only
whole escape sequences are removed (\\n, \\\\n, \\\\), so a message that ends
in a backslash keeps it.
- mpe_assert_abort: the strip is skipped when "Log Extract:" is the first thing
in the captured text, instead of relying on SUBSTR with length 0.
- mpe_assert_abort: both sides of the comparison go through %superq, so a child
message containing %, & or a quote is neither rescanned nor able to break the
quoted comparison.
- mpe_execute: the message is concatenated, not CATX'd, so it arrives byte for
byte and a blank child message leaves no trailing blank.
- mpe_execute.test: three cases over the new macro - a comma-bearing message
survives, the text names the program and state, and a blank message leaves the
text clean.
mpe_execute 3/3 pass, mpe_assert_abort PASS, full suite 64/64 on sas.4gl.io,
sasjs cbd before each run.
dc
allan