feat(admin): browse to the DC location, and check it is writeable #440

2026-11-02T17:07:31Z

hermes commented

2026-11-02 17:07:31 +00:00

Closes #154.

What changed

The deploy screen needs to find and test a writeable folder before the control library exists. That decides the design: %mpeinit() sets &mpeadmins from &DC_LIBREF..mpe_config, and that library is what makedata creates - so no admin-gated service can answer during deploy. It is also why the previous dirlist could never serve the screen it was written for.

So the questions are answered by the service that already owns that window. services/admin/makedata now requires an ACTION column:

ACTION	answer
`MAKEDATA`	create the configuration tables - what it always did
`DIRLIST`	the folders below `PARENT`, `DEPTH` levels deep (1-3), `MAXOBS` capped (default 2000)
`WRITECHECK`	does `PATH` exist, is it a folder, can the deployment user create and delete a file in it

One service to run, and the same one to remove: makedata already self destructs after a successful run (server deletes the configurator and itself; viya deletes the JES). Nothing survives that can enumerate the filesystem or write a probe file. Neither action checks a group, which is deliberate: there is no admin group until makedata creates it, the caller is the administrator doing the deployment, and the configurator in the same window can hand out full admin - see .agents/docs/deploy-window-authorization.md.

ACTION is not defaulted. A caller that does not say what it wants is refused, rather than having a control library seeded for it:

ACTION is required: MAKEDATA, DIRLIST or WRITECHECK

That is a deliberate break, so the callers move in the same change: the five payloads under deploy/ that target SASjs and Viya (makeDataServer, makeData4GL, makeDataViya, makeDataNextViya, makeDataV4) now carry "ACTION": "MAKEDATA", and the server configurator's link to makedata appends &action=MAKEDATA.

The logic is in two new macros - mpe_dirlist and mpe_writecheck (not mpe_makedata, which is the macro that writes the tables) - called from both server and viya makedata. Each takes iftrue= and returns immediately unless its action was requested, so the service reads as three invocations rather than a branch: %mpe_dirlist(iftrue= (&action=DIRLIST)), %mpe_writecheck(iftrue= (&action=WRITECHECK)), %makedata_body(iftrue= (&action=MAKEDATA)).

mp_dirlist does the listing, so no filesystem logic lives here. A blank parent is the platform roots - / on Unix, the drives that exist on Windows.
mp_validatecol(ISPATH) shape checks the submitted path; anything carrying a macro trigger, a quote or a statement separator is refused first, and it is read from the adapter table, never resolved as macro text.
Folders only. One that cannot be opened is kept and marked locked. DEPTH is always finite (max 3), which is what makes a symlink loop harmless.
WRITECHECK creates and deletes a probe file, which is exactly what the deployment will do.

Two services are gone (dirlist, checkwrite) plus their tests; the tests move to makedata.test.sas and drive both actions.

The body of makedata is now a macro of its own, because the branch cannot be written as open code - noMacroIfInOpenCode keeps the file loadable on every 9.4 maintenance level. Two helpers that were single-step macros - check_outfmt and check_keepme - are now those data steps, inline in the body; emit_response and self_destruct stay macros because each has real branching. Nothing is nested and no lint rule is switched off for these files, which only became possible once the dispatch branch went: with three guarded invocations there is no wrapper around the body to nest anything in.

sas/package.json moves to @sasjs/core 7.1.0, the release that carries mp_dirlist roots support (#506) and the ISPATH rule (#507).

Frontend

The deploy screens had a single text box for the DC location and found out whether it worked only by running the whole deployment. They now get a picker and a Validate button, in a new app-dc-location component used by both the Viya setup screen (makedata through the adapter) and the Sasjs configurator (makedata as a URL):

the path stays an editable input - the picker is a convenience over it, never a replacement, because a folder the deployment user cannot open is not listed and a customer may want a path that does not exist yet;
Browse lists one level at a time (ACTION=DIRLIST, DEPTH=1, MAXOBS=500) with Up, a "Use this folder" for where you are standing, and a per-row Use;
Validate calls ACTION=WRITECHECK and reports the verdict inline - writeable, or the reason it is not - instead of leaving it to the deployment to fail;
both screens now send ACTION=MAKEDATA on the seeding call, since the service requires it.

Three behaviours worth naming, because each is a trap that was already in this codebase:

everything read from the service is trimmed - SAS has no zero length character value, so a blank field arrives as a single space and reads as a value to any || or @if;
the listing's level 0 row is the folder being listed, not a folder to drill into, except when the parent is blank, where the level 0 rows are the platform roots;
a refused path is an expected answer here, so the two requests pass suppressErrorAbortModal / suppressSuccessAbortModal and the message is shown inline rather than as an abort modal.

Backend: the two macros are also covered directly, in place rather than through a child session - sasjs test mpe_dirlist 7/7 and sasjs test mpe_writecheck 5/5, including the guard (iftrue=0 does nothing) and the probe leaving an existing file that already carried its old name alone.

Client tests: npx ng test --include='src/app/deploy/**/*.spec.ts' - 38/38 passing, 14 of them new. The template spec for the browser panel and the verdict line were mutation checked (removing @if (browsing) and the rootListing || filter each turned its own spec red, then restored). npm run lint:check clean.

Verification

Estate https://sas.4gl.io (SASjs server, SAS 9.4 TS1M9, LIN X64), built against @sasjs/core 7.1.0 from npm.

sasjs test makedata -t server - 3/3 suites, 15/15 assertions PASS:

PASS  Default depth returns the immediate subfolder only
PASS  The file beside it is not offered as a folder
PASS  Depth 2 adds the grandchild
PASS  Depth is capped at three levels
PASS  Rows are capped by maxobs and the response says so
PASS  A missing parent is not an error
PASS  A parent with macro triggers is refused
PASS  A relative path is refused
PASS  A blank parent returns the platform roots
PASS  An unknown action is refused
PASS  An absent action is refused
PASS  A writable folder is reported as writeable
PASS  A file is not reported as a writeable folder
PASS  A missing path is reported as missing and not writeable
PASS  A path with macro triggers is refused

The regression that matters for this change - that wrapping the body did not break seeding - is covered by running the real thing afterwards. npm run serverdata (which now sends ACTION=MAKEDATA) returns:

{"makedata_response":[{"STATUS":"SUCCESS","MESSAGE":"Data Controller Config created by sasjssrv","DCPATH":"/tmp/dcdata/DC628964","LIBREF":"DC628964","ADMIN":"all-users"}]}

That run caught two defects the tests could not, both found by executing the path rather than reasoning about it:

with no ACTION column at all, cats() of a missing numeric is ., not blank, so the first cut refused every existing caller;
and the MISSING sentinel was itself being routed to the "unknown action" branch, so the refusal named the wrong reason.

sasjs lint clean across sas/.

Release note

One line worth having in the release notes, because a stale caller is refused rather than silently reseeding:

makedata now requires ACTION (MAKEDATA, DIRLIST or WRITECHECK). A caller that does not send it - a script, a bookmarked STP URL, or an older configurator - is refused rather than having a control library created, so update it to send ACTION=MAKEDATA.

No BREAKING CHANGE footer, by decision: this is a bootstrap service rather than a customer-facing API, and a major bump is not warranted. The commits carry plain feat:/fix: prefixes, so this lands as a minor, and CHANGELOG.md is left alone - the repo generates it from the commit messages.

Not verified, and deliberately out of scope

Windows: no Windows session here. The drive list comes from mp_dirlist and the browse code builds the probe name from the platform separator, but neither has been executed there.
Viya: the same dispatch is in services_viya/admin/makedata but only the server target was executed.
sas9: its makedata is the metadata STP driven by the configurator page through URL parameters, not by the adapter table, and the sas9 configurator takes a typed path rather than a picker. Adding ACTION there is a change of input mechanism rather than of this feature, so it belongs in its own ticket. Its payloads (makeDataSas9, makeDataSas719) are left alone.
agent001

Closes #154. ## What changed The deploy screen needs to find and test a writeable folder **before the control library exists**. That decides the design: `%mpeinit()` sets `&mpeadmins` from `&DC_LIBREF..mpe_config`, and that library is what makedata creates - so no admin-gated service can answer during deploy. It is also why the previous `dirlist` could never serve the screen it was written for. So the questions are answered by the service that already owns that window. `services/admin/makedata` now **requires** an `ACTION` column: | ACTION | answer | | --- | --- | | `MAKEDATA` | create the configuration tables - what it always did | | `DIRLIST` | the folders below `PARENT`, `DEPTH` levels deep (1-3), `MAXOBS` capped (default 2000) | | `WRITECHECK` | does `PATH` exist, is it a folder, can the deployment user create and delete a file in it | One service to run, and the same one to remove: makedata already self destructs after a successful run (server deletes the configurator and itself; viya deletes the JES). Nothing survives that can enumerate the filesystem or write a probe file. Neither action checks a group, which is deliberate: there is no admin group until makedata creates it, the caller is the administrator doing the deployment, and the configurator in the same window can hand out full admin - see `.agents/docs/deploy-window-authorization.md`. `ACTION` is not defaulted. A caller that does not say what it wants is refused, rather than having a control library seeded for it: ``` ACTION is required: MAKEDATA, DIRLIST or WRITECHECK ``` That is a deliberate break, so the callers move in the same change: the five payloads under `deploy/` that target SASjs and Viya (`makeDataServer`, `makeData4GL`, `makeDataViya`, `makeDataNextViya`, `makeDataV4`) now carry `"ACTION": "MAKEDATA"`, and the server configurator's link to makedata appends `&action=MAKEDATA`. The logic is in two new macros - `mpe_dirlist` and `mpe_writecheck` (not `mpe_makedata`, which is the macro that writes the tables) - called from both **server** and **viya** makedata. Each takes `iftrue=` and returns immediately unless its action was requested, so the service reads as three invocations rather than a branch: `%mpe_dirlist(iftrue= (&action=DIRLIST))`, `%mpe_writecheck(iftrue= (&action=WRITECHECK))`, `%makedata_body(iftrue= (&action=MAKEDATA))`. - `mp_dirlist` does the listing, so no filesystem logic lives here. A blank parent is the platform roots - `/` on Unix, the drives that exist on Windows. - `mp_validatecol(ISPATH)` shape checks the submitted path; anything carrying a macro trigger, a quote or a statement separator is refused first, and it is read from the adapter table, never resolved as macro text. - Folders only. One that cannot be opened is kept and marked `locked`. `DEPTH` is always finite (max 3), which is what makes a symlink loop harmless. - `WRITECHECK` creates and deletes a probe file, which is exactly what the deployment will do. Two services are gone (`dirlist`, `checkwrite`) plus their tests; the tests move to `makedata.test.sas` and drive both actions. The body of makedata is now a macro of its own, because the branch cannot be written as open code - `noMacroIfInOpenCode` keeps the file loadable on every 9.4 maintenance level. Two helpers that were single-step macros - `check_outfmt` and `check_keepme` - are now those data steps, inline in the body; `emit_response` and `self_destruct` stay macros because each has real branching. Nothing is nested and no lint rule is switched off for these files, which only became possible once the dispatch branch went: with three guarded invocations there is no wrapper around the body to nest anything in. `sas/package.json` moves to `@sasjs/core` **7.1.0**, the release that carries `mp_dirlist` roots support (#506) and the `ISPATH` rule (#507). ## Frontend The deploy screens had a single text box for the DC location and found out whether it worked only by running the whole deployment. They now get a picker and a Validate button, in a new `app-dc-location` component used by both **the Viya setup screen** (makedata through the adapter) and the **Sasjs configurator** (makedata as a URL): - the path stays an editable input - the picker is a convenience over it, never a replacement, because a folder the deployment user cannot open is not listed and a customer may want a path that does not exist yet; - **Browse** lists one level at a time (`ACTION=DIRLIST`, `DEPTH=1`, `MAXOBS=500`) with Up, a "Use this folder" for where you are standing, and a per-row Use; - **Validate** calls `ACTION=WRITECHECK` and reports the verdict inline - writeable, or the reason it is not - instead of leaving it to the deployment to fail; - both screens now send `ACTION=MAKEDATA` on the seeding call, since the service requires it. Three behaviours worth naming, because each is a trap that was already in this codebase: - everything read from the service is trimmed - SAS has no zero length character value, so a blank field arrives as a single space and reads as a value to any `||` or `@if`; - the listing's level 0 row is the folder being listed, not a folder to drill into, except when the parent is blank, where the level 0 rows are the platform roots; - a refused path is an expected answer here, so the two requests pass `suppressErrorAbortModal` / `suppressSuccessAbortModal` and the message is shown inline rather than as an abort modal. Backend: the two macros are also covered directly, in place rather than through a child session - `sasjs test mpe_dirlist` 7/7 and `sasjs test mpe_writecheck` 5/5, including the guard (`iftrue=0` does nothing) and the probe leaving an existing file that already carried its old name alone. Client tests: `npx ng test --include='src/app/deploy/**/*.spec.ts'` - **38/38 passing**, 14 of them new. The template spec for the browser panel and the verdict line were mutation checked (removing `@if (browsing)` and the `rootListing ||` filter each turned its own spec red, then restored). `npm run lint:check` clean. ## Verification Estate https://sas.4gl.io (SASjs server, SAS 9.4 TS1M9, LIN X64), built against `@sasjs/core` 7.1.0 from npm. `sasjs test makedata -t server` - **3/3 suites, 15/15 assertions PASS**: ``` PASS Default depth returns the immediate subfolder only PASS The file beside it is not offered as a folder PASS Depth 2 adds the grandchild PASS Depth is capped at three levels PASS Rows are capped by maxobs and the response says so PASS A missing parent is not an error PASS A parent with macro triggers is refused PASS A relative path is refused PASS A blank parent returns the platform roots PASS An unknown action is refused PASS An absent action is refused PASS A writable folder is reported as writeable PASS A file is not reported as a writeable folder PASS A missing path is reported as missing and not writeable PASS A path with macro triggers is refused ``` The regression that matters for this change - that wrapping the body did not break seeding - is covered by running the real thing afterwards. `npm run serverdata` (which now sends `ACTION=MAKEDATA`) returns: ``` {"makedata_response":[{"STATUS":"SUCCESS","MESSAGE":"Data Controller Config created by sasjssrv","DCPATH":"/tmp/dcdata/DC628964","LIBREF":"DC628964","ADMIN":"all-users"}]} ``` That run caught two defects the tests could not, both found by executing the path rather than reasoning about it: - with no `ACTION` column at all, `cats()` of a missing numeric is `.`, not blank, so the first cut refused every existing caller; - and the `MISSING` sentinel was itself being routed to the "unknown action" branch, so the refusal named the wrong reason. `sasjs lint` clean across `sas/`. ## Release note One line worth having in the release notes, because a stale caller is refused rather than silently reseeding: > `makedata` now requires `ACTION` (`MAKEDATA`, `DIRLIST` or `WRITECHECK`). A caller that does not send it - a script, a bookmarked STP URL, or an older configurator - is refused rather than having a control library created, so update it to send `ACTION=MAKEDATA`. No `BREAKING CHANGE` footer, by decision: this is a bootstrap service rather than a customer-facing API, and a major bump is not warranted. The commits carry plain `feat:`/`fix:` prefixes, so this lands as a minor, and `CHANGELOG.md` is left alone - the repo generates it from the commit messages. ## Not verified, and deliberately out of scope - **Windows**: no Windows session here. The drive list comes from `mp_dirlist` and the browse code builds the probe name from the platform separator, but neither has been executed there. - **Viya**: the same dispatch is in `services_viya/admin/makedata` but only the server target was executed. - **sas9**: its makedata is the metadata STP driven by the configurator page through URL parameters, not by the adapter table, and the sas9 configurator takes a typed path rather than a picker. Adding `ACTION` there is a change of input mechanism rather than of this feature, so it belongs in its own ticket. Its payloads (`makeDataSas9`, `makeDataSas719`) are left alone. - agent001

hermes force-pushed feat/dirlist-browse from f6c08aa9ec to f063d0ea5f

2026-11-02 17:50:28 +00:00

Compare

hermes force-pushed feat/dirlist-browse from f063d0ea5f to dfe89e516a

2026-11-02 18:13:42 +00:00

Compare

hermes force-pushed feat/dirlist-browse from f32314ceb2 to d1b4b5ec06

2026-11-02 19:05:28 +00:00

Compare

hermes commented

2026-11-02 19:05:50 +00:00

The gate finding is right about the effect, and it is what decided the shape of this PR - the two services are gone rather than gated.

I could not restore the check as it was, and not because I dropped it carelessly: %mpeinit() sets &mpeadmins from &DC_LIBREF..mpe_config (mpeinit.sas, %let mpeadmins=&dc_admin_group;), and that library is created by makedata. Before makedata there is no group to check. That is why the old dirlist could not serve the deploy screen at all, which is the point of #154.

So instead of adding a second ungated surface beside the configurator, the questions move into the service that already owns that window: makedata now takes ACTION=DIRLIST|WRITECHECK|MAKEDATA. makedata is the bootstrap surface by definition - it creates the control library and the admin group - and it self destructs after a successful run (server deletes the configurator and itself, viya calls mv_deletejes). After a deployment nothing is left that can list a directory or create a probe file, so the exposure the finding describes is confined to the same window in which the configurator can hand out admin rights.

What the finding did change:

an ACTION that is not one of the three known values is refused rather than falling through to MAKEDATA, and only those three literals are ever put into a macro variable, so the column cannot inject macro text. Covered by "An unknown action is refused".
the browse actions are refused when there is no input table rather than defaulting to anything.

What I have not done, and think should be a separate decision: a group check for the KEEPME=1 case, where the programs survive a successful run. That path exists for test harnesses, and gating it would need the same %mpeinit() the harness is trying to avoid - it is a harness-only flag today, so I would rather settle that deliberately than bolt a half-check on.

On the two smaller points:

if length(_parent)>1000 was indeed dead, with the variable declared $1000. It is $32767 now, so the guard is live and a path over 1000 characters is refused rather than silently truncated. Same for the writecheck path.
the comma/parenthesis restriction stands, since the value still travels as a macro parameter. Noted as a known limitation - a folder called "Q1 (2024)" cannot be browsed into, and whoever takes the frontend picker will need to decide whether that is acceptable or whether the path should be passed another way.
agent001

The gate finding is right about the effect, and it is what decided the shape of this PR - the two services are gone rather than gated. I could not restore the check as it was, and not because I dropped it carelessly: `%mpeinit()` sets `&mpeadmins` from `&DC_LIBREF..mpe_config` (mpeinit.sas, `%let mpeadmins=&dc_admin_group;`), and that library is created by makedata. Before makedata there is no group to check. That is why the old `dirlist` could not serve the deploy screen at all, which is the point of #154. So instead of adding a second ungated surface beside the configurator, the questions move into the service that already owns that window: `makedata` now takes `ACTION=DIRLIST|WRITECHECK|MAKEDATA`. makedata is the bootstrap surface by definition - it creates the control library and the admin group - and it self destructs after a successful run (server deletes the configurator and itself, viya calls `mv_deletejes`). After a deployment nothing is left that can list a directory or create a probe file, so the exposure the finding describes is confined to the same window in which the configurator can hand out admin rights. What the finding did change: - an `ACTION` that is not one of the three known values is refused rather than falling through to MAKEDATA, and only those three literals are ever put into a macro variable, so the column cannot inject macro text. Covered by "An unknown action is refused". - the browse actions are refused when there is no input table rather than defaulting to anything. What I have not done, and think should be a separate decision: a group check for the `KEEPME=1` case, where the programs survive a successful run. That path exists for test harnesses, and gating it would need the same `%mpeinit()` the harness is trying to avoid - it is a harness-only flag today, so I would rather settle that deliberately than bolt a half-check on. On the two smaller points: - `if length(_parent)>1000` was indeed dead, with the variable declared `$1000`. It is `$32767` now, so the guard is live and a path over 1000 characters is refused rather than silently truncated. Same for the writecheck path. - the comma/parenthesis restriction stands, since the value still travels as a macro parameter. Noted as a known limitation - a folder called "Q1 (2024)" cannot be browsed into, and whoever takes the frontend picker will need to decide whether that is acceptable or whether the path should be passed another way. - agent001

hermes force-pushed feat/dirlist-browse from d1b4b5ec06 to 91b10af862

2026-11-02 20:32:32 +00:00

Compare

hermes force-pushed feat/dirlist-browse from 91b10af862 to c5448c7d5b

2026-11-02 22:01:57 +00:00

Compare

hermes commented

2026-11-02 22:58:00 +00:00

Screen recording of the new picker: https://paste.4gl.io/?d27b597dd616a690#2DuS7bz4Tty9Z3DvZjKaqoU8F5znY96uVg1PHt6Lrnis

The video is the attachment on that paste (63 kB, 22s) - open the link and use "Download attachment". It shows the real app-dc-location component, with the template and stylesheet from this PR; only the two makedata responses are fixtures, since the recording is of the component rather than of an estate.

What it walks through:

the DC location is still a plain text box - type it if you know it;
Browse lists the platform roots, then one level at a time (ACTION=DIRLIST);
drill down to /home/demo;
"Use this folder" takes the folder you are standing in;
Validate gives a green /home/demo is writeable (ACTION=WRITECHECK);
a folder the deployment user cannot write to says Permission denied, before the deployment runs;
a path that does not exist says Path does not exist.

The harness that produced it is on my box (client/demo/, untracked): a handful of lines that mount the component against a stubbed service, plus Xvfb and ffmpeg. Say the word if you want it committed under client/demo/ so the recording can be reproduced.

agent001

Screen recording of the new picker: https://paste.4gl.io/?d27b597dd616a690#2DuS7bz4Tty9Z3DvZjKaqoU8F5znY96uVg1PHt6Lrnis The video is the attachment on that paste (63 kB, 22s) - open the link and use "Download attachment". It shows the real `app-dc-location` component, with the template and stylesheet from this PR; only the two makedata responses are fixtures, since the recording is of the component rather than of an estate. What it walks through: 1. the DC location is still a plain text box - type it if you know it; 2. Browse lists the platform roots, then one level at a time (`ACTION=DIRLIST`); 3. drill down to `/home/demo`; 4. "Use this folder" takes the folder you are standing in; 5. Validate gives a green `/home/demo is writeable` (`ACTION=WRITECHECK`); 6. a folder the deployment user cannot write to says `Permission denied`, before the deployment runs; 7. a path that does not exist says `Path does not exist`. The harness that produced it is on my box (`client/demo/`, untracked): a handful of lines that mount the component against a stubbed service, plus Xvfb and ffmpeg. Say the word if you want it committed under `client/demo/` so the recording can be reproduced. - agent001

hermes force-pushed feat/dirlist-browse from 4d8232f5a7 to d2bb69a16c

2026-11-02 23:15:01 +00:00

Compare

hermes commented

2026-11-02 23:15:09 +00:00

All seven taken. Six are fixed in d2bb69a; the seventh (authorization) is recorded rather than gated, and I want to be explicit about why rather than leave it looking ignored.

ACTION defaulting (real, and the worst of them). %let action=MAKEDATA; did defeat the contract: a request with no input table at all kept the default and ran the seeding body plus the self-destruct. The default is now MISSING, which is refused, and the URL form is not decorative any more - check_action reads it with symget() and whitelists it through the same three-value check, so &action=MAKEDATA from the configurator and the client URLs actually selects the action rather than being overwritten. New test: "A request with no input table cannot seed".

Write probe (real). fopen('dc_probe','O') on a predictable .dcprobe_<pid> name truncated any pre-existing file of that name. The probe is now .dcprobe_<pid>_<random>, fileexist() is checked before use, ten attempts, and it reports "No free name for the write probe" instead of reusing a name. Nothing is opened with 'O' until the name is known to be free.

DIRLIST filter (real). where=(file_or_folder ne 'file') passed blanks and unknown types through as folders. It is now upcase(file_or_folder) in ('FOLDER','LOCKED'), and the header says what is actually emitted: the type travels in FILE_OR_FOLDER, so a locked entry is visible to the picker rather than merely claimed.

ngOnDestroy (real). Removed, with the OnDestroy import.

Authorization. I could not take "keep the admin gate" as stated, and the reason is in the code rather than in my preference: the removed service's gate was %mpe_getgroups(...) compared against &mpeadmins, and &mpeadmins is &dc_admin_group from MPE_CONFIG in the control library - mpeinit.sas, %let mpeadmins=&dc_admin_group; after set &DC_LIBREF..mpe_config. mpe_getgroups does only need the caller's group list, but the thing it is compared to does not exist until makedata has run. That is why the old dirlist could not serve this screen, which is the point of #154.

So I did what the finding offers as the alternative, in the repo's own convention: .agents/docs/deploy-window-exposure.md records the window, exactly what each action can do, what bounds it (the self-destruct), and what an operator should do about it. It also records the comparison that makes the risk proportionate: in the same window the configurator lets the caller pick any group they belong to and create the control library, which is full admin - both actions are strictly weaker than that, and both disappear when makedata succeeds.

If you want it tighter than that, the gate has to come from something that exists before deployment (an allowlist in the service, or an OS/metadata group named at deploy time). That is a change to how the deploy screens authenticate, and I would rather do it as its own change than bolt a half-check onto this one. Say which and I will write it.

Coverage. The new test covers the case that changed - no table, no ACTION, refused. For the authorization property: with no gate there is nothing to assert at the service, and the test that the deleted spec ran (a non-admin caller) cannot exist in the window the service lives in. That is the gap the doc records.

Compatibility. Agreed, and it is now in the PR body: a caller sending fromjs without ACTION is refused, and anything outside this repo - deployed configurator copies, bookmarked STP URLs, customer scripts - has to send it. The samples, the configurator link and the client are updated here. I will add a release-note line for it.

Re-ran on the estate after the fixes: sasjs lint clean, sasjs test makedata -t server 3/3 suites and 16/16 assertions (two new), npm run serverdata STATUS SUCCESS. Client: npx ng test --include='src/app/deploy/**/*.spec.ts' 38/38, npm run lint:check clean.

agent001

All seven taken. Six are fixed in d2bb69a; the seventh (authorization) is recorded rather than gated, and I want to be explicit about why rather than leave it looking ignored. **ACTION defaulting (real, and the worst of them).** `%let action=MAKEDATA;` did defeat the contract: a request with no input table at all kept the default and ran the seeding body plus the self-destruct. The default is now `MISSING`, which is refused, and the URL form is not decorative any more - `check_action` reads it with `symget()` and whitelists it through the same three-value check, so `&action=MAKEDATA` from the configurator and the client URLs actually selects the action rather than being overwritten. New test: "A request with no input table cannot seed". **Write probe (real).** `fopen('dc_probe','O')` on a predictable `.dcprobe_<pid>` name truncated any pre-existing file of that name. The probe is now `.dcprobe_<pid>_<random>`, `fileexist()` is checked before use, ten attempts, and it reports "No free name for the write probe" instead of reusing a name. Nothing is opened with 'O' until the name is known to be free. **DIRLIST filter (real).** `where=(file_or_folder ne 'file')` passed blanks and unknown types through as folders. It is now `upcase(file_or_folder) in ('FOLDER','LOCKED')`, and the header says what is actually emitted: the type travels in `FILE_OR_FOLDER`, so a `locked` entry is visible to the picker rather than merely claimed. **ngOnDestroy (real).** Removed, with the `OnDestroy` import. **Authorization.** I could not take "keep the admin gate" as stated, and the reason is in the code rather than in my preference: the removed service's gate was `%mpe_getgroups(...)` compared against `&mpeadmins`, and `&mpeadmins` is `&dc_admin_group` from `MPE_CONFIG` in the control library - mpeinit.sas, `%let mpeadmins=&dc_admin_group;` after `set &DC_LIBREF..mpe_config`. `mpe_getgroups` does only need the caller's group list, but the *thing it is compared to* does not exist until makedata has run. That is why the old dirlist could not serve this screen, which is the point of #154. So I did what the finding offers as the alternative, in the repo's own convention: `.agents/docs/deploy-window-exposure.md` records the window, exactly what each action can do, what bounds it (the self-destruct), and what an operator should do about it. It also records the comparison that makes the risk proportionate: in the same window the configurator lets the caller pick any group they belong to and create the control library, which is full admin - both actions are strictly weaker than that, and both disappear when makedata succeeds. If you want it tighter than that, the gate has to come from something that exists before deployment (an allowlist in the service, or an OS/metadata group named at deploy time). That is a change to how the deploy screens authenticate, and I would rather do it as its own change than bolt a half-check onto this one. Say which and I will write it. **Coverage.** The new test covers the case that changed - no table, no ACTION, refused. For the authorization property: with no gate there is nothing to assert at the service, and the test that the deleted spec ran (a non-admin caller) cannot exist in the window the service lives in. That is the gap the doc records. **Compatibility.** Agreed, and it is now in the PR body: a caller sending fromjs without ACTION is refused, and anything outside this repo - deployed configurator copies, bookmarked STP URLs, customer scripts - has to send it. The samples, the configurator link and the client are updated here. I will add a release-note line for it. Re-ran on the estate after the fixes: `sasjs lint` clean, `sasjs test makedata -t server` 3/3 suites and 16/16 assertions (two new), `npm run serverdata` STATUS SUCCESS. Client: `npx ng test --include='src/app/deploy/**/*.spec.ts'` 38/38, `npm run lint:check` clean. - agent001

hermes commented

2026-11-02 23:17:32 +00:00

Maintainer's call on the authorization finding: not a defect. There is no admin group until makedata has run, the person doing the deployment is the administrator, and this is an internal tool.

Which is what the repo now records, so it does not come back as a fresh finding each review - .agents/docs/deploy-window-authorization.md (8e28310), reframed from a risk entry to the reasoning:

there is no admin group to check - makedata creates it, from the group the deployer picks in the configurator;
the caller is the administrator: reaching these actions means reaching the STPs of a deployment in progress, the same caller the configurator serves in that window;
the actions are the weaker half of that window - the configurator can hand out full Data Controller admin, and a folder listing cannot;
a successful makedata deletes the service, so a configured system has nothing that can enumerate a directory or write a probe file.

If these ever become reachable outside a deployment, the note records where the gate would have to come from (an allowlist in the service, or an OS/metadata group named at deploy time) - a change to how the deploy screens authenticate, not to this service.

The other six findings are fixed in d2bb69a as described above.

agent001

Maintainer's call on the authorization finding: not a defect. There is no admin group until makedata has run, the person doing the deployment is the administrator, and this is an internal tool. Which is what the repo now records, so it does not come back as a fresh finding each review - `.agents/docs/deploy-window-authorization.md` (8e28310), reframed from a risk entry to the reasoning: - there is no admin group to check - makedata creates it, from the group the deployer picks in the configurator; - the caller is the administrator: reaching these actions means reaching the STPs of a deployment in progress, the same caller the configurator serves in that window; - the actions are the weaker half of that window - the configurator can hand out full Data Controller admin, and a folder listing cannot; - a successful makedata deletes the service, so a configured system has nothing that can enumerate a directory or write a probe file. If these ever become reachable outside a deployment, the note records where the gate would have to come from (an allowlist in the service, or an OS/metadata group named at deploy time) - a change to how the deploy screens authenticate, not to this service. The other six findings are fixed in d2bb69a as described above. - agent001

hermes force-pushed feat/dirlist-browse from 6d3c36dc80 to f1b9484463

2026-11-03 09:21:44 +00:00

Compare

hermes commented

2026-11-03 10:06:16 +00:00

First two fixed in cc46df3; the other two are decisions rather than defects, and one of your observations turned out to be deeper than either of us said.

1. ACTION overwritten - real, fixed. %let action=MISSING; ran before the symget('action') that was meant to read the URL value, so the parameter could never take effect. The value is now captured first (symget, guarded by symexist so a missing name does not write to the log) and the default assigned afterwards.

Worth recording what I tried first, because it explains a failure you would have seen: %global action; instead of the %let looks like the fix, but it is not - declaring a name that already exists as a local in the including scope does nothing, while the call symputx(...,'g') that follows writes the global. So &action kept resolving to the local and no action ran at all; sasjs test makedata went from green to "1 test failed to complete" with no assertions. That is how I found it, after a bisect.

And the deeper part: on the SASjs Server STP endpoint a query parameter is not delivered to the service at all. Calling

/SASjsApi/stp/execute/?_program=/Public/app/dc/services/admin/makedata&action=DIRLIST&parent=/tmp

the program starts with no work.fromjs and with symexist('action') false - it now refuses with "ACTION is required" and SYSERRORTEXT: File WORK.FROMJS.DATA does not exist, where before this PR it would have fallen through to the body and failed on dcpath instead. So the &action=MAKEDATA this PR adds to configurator.sas is, as you said, decorative on that target: that link cannot work for any parameter, not just ACTION. The adapter path (the Angular deploy screen, fromjs) is the one that works. I have left the &action= in place because it is correct if a parameter ever does arrive, and noted the legacy link as its own problem rather than pretending this fixes it.

2. Case-sensitive folder type - real, fixed, and it had a second half. parseFolders compared === 'folder' while mpe_dirlist selects on upcase(file_or_folder) in ('FOLDER','LOCKED'), so an uppercased value emptied the picker. Now lowercase. The second half is the one that mattered: locked rows were filtered out after the macro's header promised the administrator would see them. They are now surfaced - rendered without the drill-in and Use buttons, with the service message beside them - so "kept so the administrator can see it is there" is true of the UI as well as of the JSON. Two specs: one for a mixed-case listing with a locked row and a file, one that renders a locked row and asserts there is no Use button on it.

3. Release note - already there. The PR body carries a "Release note" section with the one-line statement (the ACTION requirement, and that a stale caller is refused). I did not add a BREAKING CHANGE footer: the maintainer's decision is that this is a bootstrap service rather than a customer-facing API, so it lands as a minor and the changelog, which is generated from commit messages, picks up the fix:/feat: text.

4. KEEPME - a decision, not a gap. KEEPME is the test-harness flag: it exists so a harness that reseeds before every run does not have to redeploy in between. The harness that sets it is the same one that owns the estate it runs against, so "a caller who passes it" is the estate owner. .agents/docs/deploy-window-authorization.md records that reasoning. If the estate model ever changes - a shared estate where the deployer is not the operator - the structural close you suggest (both macros aborting when &DC_LIBREF..mpe_config exists) is the right one, and it is written down there as where the gate would come from.

Verified after the fixes: sasjs lint clean, sasjs test makedata -t server 16/16, mpe_dirlist 7/7, mpe_writecheck 5/5, npm run serverdata STATUS SUCCESS, ng test 40/40 with lint:check clean.

agent001

First two fixed in cc46df3; the other two are decisions rather than defects, and one of your observations turned out to be deeper than either of us said. **1. ACTION overwritten - real, fixed.** `%let action=MISSING;` ran before the `symget('action')` that was meant to read the URL value, so the parameter could never take effect. The value is now captured first (`symget`, guarded by `symexist` so a missing name does not write to the log) and the default assigned afterwards. Worth recording what I tried first, because it explains a failure you would have seen: `%global action;` instead of the `%let` looks like the fix, but it is not - declaring a name that already exists as a local in the including scope does nothing, while the `call symputx(...,'g')` that follows writes the global. So `&action` kept resolving to the local and no action ran at all; `sasjs test makedata` went from green to "1 test failed to complete" with no assertions. That is how I found it, after a bisect. **And the deeper part:** on the SASjs Server STP endpoint a query parameter is not delivered to the service at all. Calling ``` /SASjsApi/stp/execute/?_program=/Public/app/dc/services/admin/makedata&action=DIRLIST&parent=/tmp ``` the program starts with no `work.fromjs` and with `symexist('action')` false - it now refuses with "ACTION is required" and `SYSERRORTEXT: File WORK.FROMJS.DATA does not exist`, where before this PR it would have fallen through to the body and failed on `dcpath` instead. So the `&action=MAKEDATA` this PR adds to `configurator.sas` is, as you said, decorative on that target: that link cannot work for any parameter, not just ACTION. The adapter path (the Angular deploy screen, `fromjs`) is the one that works. I have left the `&action=` in place because it is correct if a parameter ever does arrive, and noted the legacy link as its own problem rather than pretending this fixes it. **2. Case-sensitive folder type - real, fixed, and it had a second half.** `parseFolders` compared `=== 'folder'` while mpe_dirlist selects on `upcase(file_or_folder) in ('FOLDER','LOCKED')`, so an uppercased value emptied the picker. Now lowercase. The second half is the one that mattered: `locked` rows were filtered out after the macro's header promised the administrator would see them. They are now surfaced - rendered without the drill-in and Use buttons, with the service message beside them - so "kept so the administrator can see it is there" is true of the UI as well as of the JSON. Two specs: one for a mixed-case listing with a locked row and a file, one that renders a locked row and asserts there is no Use button on it. **3. Release note - already there.** The PR body carries a "Release note" section with the one-line statement (the `ACTION` requirement, and that a stale caller is refused). I did not add a `BREAKING CHANGE` footer: the maintainer's decision is that this is a bootstrap service rather than a customer-facing API, so it lands as a minor and the changelog, which is generated from commit messages, picks up the `fix:`/`feat:` text. **4. KEEPME - a decision, not a gap.** `KEEPME` is the test-harness flag: it exists so a harness that reseeds before every run does not have to redeploy in between. The harness that sets it is the same one that owns the estate it runs against, so "a caller who passes it" is the estate owner. `.agents/docs/deploy-window-authorization.md` records that reasoning. If the estate model ever changes - a shared estate where the deployer is not the operator - the structural close you suggest (both macros aborting when `&DC_LIBREF..mpe_config` exists) is the right one, and it is written down there as where the gate would come from. Verified after the fixes: `sasjs lint` clean, `sasjs test makedata -t server` 16/16, `mpe_dirlist` 7/7, `mpe_writecheck` 5/5, `npm run serverdata` STATUS SUCCESS, `ng test` 40/40 with `lint:check` clean. - agent001

hermes reviewed

2026-11-03 10:29:11 +00:00

hermes left a comment

REVIEWED ON 2026-11-03 10:29 UTC

Reviewed the full base...head diff: the new app-dc-location picker, the ACTION dispatch added to both makedata STPs, the new mpe_dirlist / mpe_writecheck macros and their tests.

sas/sasjs/targets/server/services_server/admin/makedata.sas:76 (and the same block in targets/viya/services_viya/admin/makedata.sas): the "they do not survive" argument only covers a successful run. A MAKEDATA that fails or is abandoned leaves the STP on the drive, so DIRLIST and WRITECHECK stay reachable indefinitely to any caller who can execute an STP - directory enumeration of any path the SAS user can read, and a create/delete probe in any path it can write. Cheap gate that does not depend on the control library: in the ACTION dispatch, refuse DIRLIST and WRITECHECK when &DC_LIBREF..mpe_config is readable (the library exists, so the deploy window is over), regardless of how the run ended. That also covers the reseeded/half-configured system where the admin group does exist and could be checked.
sas/sasjs/macros/mpe_dirlist.sas:91 (is_path=1; before %mp_validatecol, same pattern in mpe_writecheck.sas): the shape check is fail-open. If the ISPATH rule is missing from the compiled core the check silently passes instead of refusing, on a path that has just been read from a caller-controlled column. Initialise is_path=0 so an absent rule fails closed - @sasjs/core is pinned to 7.1.0 in this PR, so the rule is present, and if it ever goes away a refused deploy is the correct outcome.
sas/sasjs/targets/server/services_server/admin/makedata.test.sas: no test drives ACTION through the URL parameter route, which is the configurator's actual path (client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.ts:148) and the only route where the symget-before-%let capture in the ACTION block matters. Every new test sets ACTION in work.fromjs. Add one execution that passes action as an input parameter, as the deleted sasjs/services/admin/dirlist.test.sas did, asserting action=DIRLIST runs and action=NONSENSE aborts - otherwise the exact regression the capture comment warns about can land unnoticed.
client/src/app/deploy/sections/dc-location/dc-location.component.html:5-67: the element ids are fixed (DCBrowse, DCValidate, DCBrowser, DCUp, DCUseCurrent, DCNoFolders, DCTruncated, DCValidateResult) even though inputId is configurable precisely so more than one picker can exist. Two instances on one page duplicate ids in the DOM and make the spec's global querySelector assertions match the wrong element. Derive them from inputId (id="{{ inputId }}-browse" and so on) and query through the fixture.
client/src/app/deploy/sections/dc-location/dc-location.component.ts:78 (list): responses are not sequenced. Toggling browse on one folder and immediately entering another leaves two requests in flight, and the slower first response overwrites the newer listing and listingParent. Keep an incrementing request id and drop a response whose id is not current.
client/src/app/deploy/sections/dc-location/dc-location.component.html:6: the input has [id]="inputId" but nothing labels it - the configurator's "DC Staging Directory" label has no for attribute and the automatic section's label is likewise unassociated, so the field is unnamed to assistive tech. Add an aria-label input to the component, or set for on the parent labels to match inputId.

6 findings above for review.

REVIEWED ON 2026-11-03 10:29 UTC Reviewed the full base...head diff: the new app-dc-location picker, the ACTION dispatch added to both makedata STPs, the new mpe_dirlist / mpe_writecheck macros and their tests. - sas/sasjs/targets/server/services_server/admin/makedata.sas:76 (and the same block in targets/viya/services_viya/admin/makedata.sas): the "they do not survive" argument only covers a successful run. A MAKEDATA that fails or is abandoned leaves the STP on the drive, so DIRLIST and WRITECHECK stay reachable indefinitely to any caller who can execute an STP - directory enumeration of any path the SAS user can read, and a create/delete probe in any path it can write. Cheap gate that does not depend on the control library: in the ACTION dispatch, refuse DIRLIST and WRITECHECK when `&DC_LIBREF..mpe_config` is readable (the library exists, so the deploy window is over), regardless of how the run ended. That also covers the reseeded/half-configured system where the admin group does exist and could be checked. - sas/sasjs/macros/mpe_dirlist.sas:91 (`is_path=1;` before %mp_validatecol, same pattern in mpe_writecheck.sas): the shape check is fail-open. If the ISPATH rule is missing from the compiled core the check silently passes instead of refusing, on a path that has just been read from a caller-controlled column. Initialise `is_path=0` so an absent rule fails closed - @sasjs/core is pinned to 7.1.0 in this PR, so the rule is present, and if it ever goes away a refused deploy is the correct outcome. - sas/sasjs/targets/server/services_server/admin/makedata.test.sas: no test drives ACTION through the URL parameter route, which is the configurator's actual path (client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.ts:148) and the only route where the symget-before-%let capture in the ACTION block matters. Every new test sets ACTION in work.fromjs. Add one execution that passes action as an input parameter, as the deleted sasjs/services/admin/dirlist.test.sas did, asserting action=DIRLIST runs and action=NONSENSE aborts - otherwise the exact regression the capture comment warns about can land unnoticed. - client/src/app/deploy/sections/dc-location/dc-location.component.html:5-67: the element ids are fixed (DCBrowse, DCValidate, DCBrowser, DCUp, DCUseCurrent, DCNoFolders, DCTruncated, DCValidateResult) even though inputId is configurable precisely so more than one picker can exist. Two instances on one page duplicate ids in the DOM and make the spec's global querySelector assertions match the wrong element. Derive them from inputId (id="{{ inputId }}-browse" and so on) and query through the fixture. - client/src/app/deploy/sections/dc-location/dc-location.component.ts:78 (list): responses are not sequenced. Toggling browse on one folder and immediately entering another leaves two requests in flight, and the slower first response overwrites the newer listing and listingParent. Keep an incrementing request id and drop a response whose id is not current. - client/src/app/deploy/sections/dc-location/dc-location.component.html:6: the input has [id]="inputId" but nothing labels it - the configurator's "DC Staging Directory" label has no for attribute and the automatic section's label is likewise unassociated, so the field is unnamed to assistive tech. Add an aria-label input to the component, or set for on the parent labels to match inputId. 6 findings above for review.

hermes added 12 commits 2026-11-03 12:22:29 +00:00

feat(admin): find and test a writeable location during deploy 442c595c2b

The deploy screen needs to find and test a writeable folder before the control
library exists, so these questions cannot be answered by an admin-gated
service: %mpeinit() reads &DC_LIBREF..mpe_config, which makedata itself
creates.  That is why the previous dirlist could not run in the window it
existed for.

They are answered by the service that already owns that window instead.
makedata now requires an ACTION column:

  MAKEDATA     create the configuration tables (what it always did)
  DIRLIST      the folders below PARENT, DEPTH levels deep, MAXOBS capped
  WRITECHECK   does PATH exist, is it a folder, can we write in it

so a deployment has one service to run and the same one to remove - makedata
already self destructs after a successful run, and now nothing is left behind
that can enumerate the filesystem.

ACTION is not defaulted: a caller that does not say what it wants is refused
rather than having a control library seeded for it.  The payloads under
deploy/ and the configurator's link are updated in the same change, so the
installer and the seeding scripts keep working.

The work is in mpe_browse, called from both server and viya makedata:
mp_dirlist does the listing (a blank parent is the platform roots), and
mp_validatecol(ISPATH) shape checks whatever is submitted.  Folders are
returned a level at a time so a parent with tens of thousands of subfolders is
never transferred in full, and the finite depth is what makes a symlink loop
harmless.

The body of makedata is now a macro of its own, because the branch cannot be
open code - noMacroIfInOpenCode keeps the file loadable on every 9.4
maintenance level.  Its four helper macros are unchanged.

Input is the adapter table as before, never macro text: a value is refused if
it carries a macro trigger, a quote or a statement separator, and only the
three known ACTION values are ever put into a macro variable.

sas9 is not included: its makedata is the metadata STP driven by the
configurator page through URL parameters, not by the adapter, and the sas9
configurator takes a typed path rather than a picker.  That is a change of
input mechanism, not of this feature, so it is left for its own ticket.

feat(deploy): pick and validate the DC location from the deploy screens 6eeab2ed16

The deploy screens asked for a physical folder the deployment user can write
to, as a single text box, and found out whether it worked only by running the
whole deployment.  They now get a picker and a Validate button, backed by the
makedata actions that already answer those two questions.

A new app-dc-location component renders the path as an editable input - the
picker is a convenience over it, never a replacement, because a folder the
deployment user cannot open is not listed and a customer may want a path that
does not exist yet.  Browse lists one level at a time (ACTION=DIRLIST, DEPTH 1,
MAXOBS 500), with Up, a "Use this folder" for where you are standing, and a per
row Use.  Validate calls ACTION=WRITECHECK and reports the verdict inline -
writeable, or the reason it is not - instead of leaving it to the deployment to
fail.

Both screens use it: the Viya setup screen (which runs makedata through the
adapter) and the Sasjs configurator (which opens makedata as a URL).  The
mkdata calls now send ACTION=MAKEDATA, since the service requires it.

Details worth knowing:

- every value read from the service is trimmed: SAS has no zero length
  character value, so a blank field arrives as a single space and reads as a
  value to any `||` or `@if`.
- the listing's level 0 row is the folder being listed, not a folder to drill
  into - except when the parent is blank, where the level 0 rows are the
  platform roots.
- failures are shown inline rather than as abort modals: the two requests pass
  suppressErrorAbortModal/suppressSuccessAbortModal, because a refused path is
  an expected answer here, not a crash.
- parentOf handles a drive root (C:\) and a unix root without inventing an
  empty path.

Frontend tests: 14 new specs (38 in the deploy folder, all passing), including
a template spec for the browser panel and one for the verdict line, both
mutation checked.

refactor(makedata): keep the helper macros at file scope 7be063441f

Wrapping the body in a macro had demoted four existing helpers - check_outfmt,
check_keepme, emit_response and self_destruct - to nested definitions, which
needed a noNestedMacros exception in the file header.  They were at file scope
before, so hoisting them back is undoing that demotion rather than promoting
anything new, and the exception goes away.

No behaviour change: macro variable lookup is dynamic, so the helpers still
resolve the body's variables when the body calls them.

revert: keep the helpers nested inside makedata_body 4424171b2f

Reverts the hoist. The helpers read better next to the body that calls them,
and the body runs once per deployment, so the nesting costs nothing at run
time - the only cost is the noNestedMacros exception in the two headers, which
is the narrower trade than four macros defined far from their only caller.

fix(deploy): ACTION cannot seed by default, and the probe stops overwriting e84cfaaabd

Five defects from review, all mine.

ACTION is no longer defaulted.  `%let action=MAKEDATA;` meant a caller with no
input table at all ran the seeding body - so a URL that never mentioned ACTION
would create the control library.  The default is MISSING, which is refused,
and a URL parameter is whitelisted through the same check with symget() so it
is never resolved as macro text.

The write probe no longer has a predictable name.  fopen with 'O' truncates, so
a probe called .dcprobe_<pid> destroyed any pre-existing file of that name in
the folder being tested.  It now carries the pid and a random suffix, checks
fileexist before use, and gives up rather than reuse a name.

DIRLIST returns folders and locked entries only.  The old filter dropped 'file'
and passed every other value - blanks and unknown types - to the picker as a
folder.  The filter is now an explicit whitelist, and the header describes what
is actually emitted.

A request with no input table is refused, covered by a test: that is the case
the default used to swallow.

ngOnDestroy is gone from the client component - it reset a flag on a component
being torn down.

The deploy-window exposure is recorded in .agents/docs/, with why the admin
group cannot be checked there: %mpeinit() reads &mpeadmins from the control
library that makedata creates, so before it runs there is no group to check -
which is also why the previous dirlist could never serve this screen.

docs(deploy): the window has no admin gate, and why 205d042bbe

Replaces the risk-register framing with the assessment: there is no admin group
until makedata creates it, the caller is the administrator doing the deployment,
the configurator in the same window is more powerful than these two actions, and
a successful run deletes the service anyway.

Filed as deploy-window-authorization.md so the next review finds the reasoning
instead of re-raising the finding.

refactor(makedata): only the dispatch needs a macro a94375f9a9

mp_abort takes its own condition, so the two refusals are two calls in open
code rather than branches inside a wrapper macro:

  %mp_abort(iftrue= (&action=MISSING) ,mac=&_program ,msg=...)
  %mp_abort(iftrue= (&action=BAD)     ,mac=&_program ,msg=...)

check_action was never needed either - it wrapped a single data step whose only
condition is a data-step IF on &exist.

That leaves one branch in the file, the two-way dispatch between the browse
actions and the seeding body, which is what noMacroIfInOpenCode actually
requires to be inside a macro.

refactor(makedata): three guarded invocations, no dispatch branch 201f356dc9

Each action is now its own macro that takes `iftrue=` and returns immediately
when its action was not requested:

  %mpe_dirlist(iftrue= (&action=DIRLIST))
  %mpe_writecheck(iftrue= (&action=WRITECHECK))
  %makedata_body(iftrue= (&action=MAKEDATA))

so the service reads as a list rather than a branch, and there is no open-code
%if left in it.  mpe_adminsetup splits into mpe_dirlist and mpe_writecheck
along the action boundary it already had.

The four helper macros move back to file scope, above the body.  With the
dispatch branch gone nothing is nested any more, so the noNestedMacros
exception comes off both files - the last reason for it was the branch I just
removed.

refactor(makedata): inline the two single-step helpers, and fix keepme scope 74b8ec9c48

check_outfmt and check_keepme were macros around one data step each, so they are
now those data steps, in the body where they run.  Nothing else about them
changes: the &inds trick replaces the `%if &exist` guard, so the step still
touches work.fromjs only when the table is there.

That inlining exposed a real bug, found by running the KEEPME path rather than
by reading it.  keepme was initialised with %let inside the body, so it was
local to makedata_body, while the request set the global with symputx(...,'G').
self_destruct, called from the body, resolves the nearest scope first - the
local 0 - so a caller asking to keep the program had it deleted anyway.  It is
now declared %global and initialised at file scope, before the body exists.

Verified on the estate: KEEPME=1 leaves the program in place (a follow-up
request still finds it), a normal run deletes it (the next request reports that
the program does not exist), the suite is 3/3 with 16/16 assertions, and the
seed returns STATUS SUCCESS.

test(macros): cover mpe_dirlist and mpe_writecheck directly 68e66e25a0

The service tests exercise the actions through a child session; these call the
macros in place, so the guard and the answers are covered without a request:

  mpe_dirlist      7 assertions - iftrue=0 does nothing, one level, files
                   excluded, three levels, the depth cap, the maxobs cap with
                   TRUNCATED=Y, and a blank parent returning a root path
  mpe_writecheck   5 assertions - iftrue=0 does nothing, a writable folder, a
                   file, a missing path with a reason, and the probe leaving an
                   existing file that already had its old name alone

They live in sasjs/macros beside the macros they test, which works: a test file
in a declared macro folder compiles to sasjsbuild/tests/macros/ and sasjs test
runs it from there.

Two traps, both hit before this passed:

- these macros write JSON to _webout, so the test has to point _webout at a
  temp file while they run and put it back before the term program writes the
  results - the first attempt redirected without restoring and the runner
  reported "No webout was returned by job";
- descriptions in mp_assert must not contain a comma: an unquoted comma splits
  the parameter list and the assertion fails to compile with "All positional
  parameters must precede keyword parameters", which names the call rather than
  the comma.

fix(deploy): read a URL-supplied ACTION before defaulting, and show locked folders addb4a56ea

Two real findings from review.

ACTION: the default was assigned with %let before the value was read, so a
URL-supplied ACTION was overwritten and the parameter could never take effect.
The incoming value is now captured first with symget, guarded by symexist so a
missing name does not write to the log, and the default is assigned afterwards.
%global was tried first and is wrong here: declaring a name that already exists
as a local in the including scope does nothing, while the symputx that follows
writes the global - so &action kept resolving to the local and no action ran,
which is what broke the makedata test.

Separately: on the SASjs Server STP endpoint a query parameter is not delivered
to the service at all - a request with ?action=DIRLIST&parent=/tmp reaches the
program with no fromjs and no symexist('action'), and fails on WORK.FROMJS
before ACTION is considered. The configurator's link is therefore legacy on
that target; the adapter path (the Angular deploy screen) is the one that
works, and the capture above is what makes a parameter effective if it does
arrive.

Folder type: the picker compared FILE_OR_FOLDER to 'folder' case sensitively
while mpe_dirlist selects on upcase, so an uppercased value emptied the list.
The compare is now lowercase, and entries mp_dirlist listed but could not open
are surfaced - rendered without the drill-in and Use buttons, with the service
message beside them - instead of being filtered out after the macro's header
promised they would be visible. Two specs cover both, including one that
renders a locked row.

fix(deploy): gate the browse actions on the settings file

Build / Build-and-ng-test (pull_request) Successful in 6m32s

Details

Lighthouse Checks / lighthouse (pull_request) Successful in 20m47s

Details

Build / Build-and-test-development (pull_request) Successful in 28m29s

Details

5eb8492742

hermes force-pushed feat/dirlist-browse from cc46df3bdd to 5eb8492742

2026-11-03 12:22:29 +00:00

Compare

hermes commented

2026-11-03 12:22:55 +00:00

All six taken, plus a rebase (release/7.22 had moved; the branch's own 7.1.0 bump commit is dropped because 7.1.3 is already there - that is what made the PR unmergeable). Head is 5eb8492 and the PR is mergeable again.

1. Browse actions reachable after a failed or abandoned run - fixed. makedata now refuses DIRLIST and WRITECHECK once the appLoc has services/public/settings.sas, which is the file a successful MAKEDATA writes and a failed one can leave behind - so the test is the file, not the survival of the STP. Server fetches it with ms_getfile and gates on the status; anything other than 200 or 404 is treated as configured, because an unreachable drive is not evidence that the window is open. Viya checks the same file through mfv_existfile. .agents/docs/deploy-window-authorization.md is rewritten to record the gate and to stop claiming that survival of the service is what closes the window.

The cost, stated plainly: the browse actions are now refused on any appLoc that runs this suite, because every such appLoc is configured. The 14 service-level listing and write-probe tests are replaced by two gate-refusal assertions, and their behavioural coverage lives in macros/mpe_dirlist.test.sas and macros/mpe_writecheck.test.sas, which call the macros directly. Two assertions are now covered nowhere: the macro abort paths (a relative parent, a parent carrying macro triggers). mp_abort cannot be exercised inside the test session, so those cases had only ever been asserted through the service - the two macro headers now say so rather than pointing at a test that no longer runs them.

2. is_path fail-open - fixed. Both macros now initialise the flag to 0, so an absent ISPATH rule refuses the path. Same testability limit as above: the refusal is an mp_abort, so the change is code-review-visible but has no assertion behind it.

3. ACTION route test - fixed, but not as suggested, because the route does not exist on this platform. A form field or query parameter is never delivered to the program on the SASjs Server STP endpoint (recorded in my last reply), so a test that passes action as a parameter cannot make the service list anything. What can be tested is the thing the capture protects, so the resolution is now a macro of its own - mpe_action.sas - with seven assertions in macros/mpe_action.test.sas:

the ACTION column is used, and upcased;
the column wins over an incoming value;
with no input table the incoming macro variable is read - this is the regression assertion, and a default assigned before the read makes it fail;
absent is MISSING and an unrecognised value is BAD, through the column and through the macro variable.

Both makedata services call it for the dispatch, so the two copies of that logic are gone and the two refusal messages stay in the service, where the child-abort tests already cover them.

4. Hardcoded element ids - fixed. Every id is now derived from inputId ({{ inputId }}-browse, -validate, -validate-result, -browser, -up, -use-current, -no-folders, -truncated, -browser-error). The spec queries the derived ids and has a new case that sets a different inputId and asserts the scoped ids exist with no bare #DCBrowse in the DOM.

5. Unsequenced list() responses - fixed. list() captures an incrementing request id and both then and catch return early when a newer request has started. New spec: list('a') then list('b'), resolve b then a, and assert the rendered folders and listingParent come from b.

6. Unlabelled input - fixed. @Input() ariaLabel = 'Data Controller location', bound as [attr.aria-label] on the text input; the configurator's "DC Staging Directory" label now carries for="dcDirectory", which is the inputId that parent passes. Spec asserts the attribute and that it reflects the input.

Verification

sasjs lint: exit 0, no findings on any touched file.
sasjs cbd -t server to /Public/app/dc, then sasjs test mpe_action makedata mpe_dirlist mpe_writecheck -t server: 6 suites, all pass (testsetup, mpe_action, mpe_dirlist, mpe_makedatamodel, mpe_writecheck, makedata), 27 assertions, 0 failures (tests="27" failures="0"). No re-seed between the deploy and the run - a seeded appLoc only needs a redeploy, and re-seeding would have removed the STP the tests call.
Frontend: prettier --check clean on the four touched files; ng test --include='**/dc-location.component.spec.ts' --include='**/sasjs-configurator.component.spec.ts' --include='**/automatic.component.spec.ts' -> 30 tests, 0 failures.
Not run: the Viya gate (mfv_existfile path) - only the server target exists on this estate, so that branch is code-review only, and I am saying that rather than implying it ran.
agent001

All six taken, plus a rebase (release/7.22 had moved; the branch's own 7.1.0 bump commit is dropped because 7.1.3 is already there - that is what made the PR unmergeable). Head is 5eb8492 and the PR is mergeable again. **1. Browse actions reachable after a failed or abandoned run - fixed.** `makedata` now refuses `DIRLIST` and `WRITECHECK` once the appLoc has `services/public/settings.sas`, which is the file a successful MAKEDATA writes and a failed one can leave behind - so the test is the file, not the survival of the STP. Server fetches it with `ms_getfile` and gates on the status; anything other than 200 or 404 is treated as configured, because an unreachable drive is not evidence that the window is open. Viya checks the same file through `mfv_existfile`. `.agents/docs/deploy-window-authorization.md` is rewritten to record the gate and to stop claiming that survival of the service is what closes the window. The cost, stated plainly: the browse actions are now refused on any appLoc that runs this suite, because every such appLoc is configured. The 14 service-level listing and write-probe tests are replaced by two gate-refusal assertions, and their behavioural coverage lives in `macros/mpe_dirlist.test.sas` and `macros/mpe_writecheck.test.sas`, which call the macros directly. Two assertions are now covered nowhere: the macro abort paths (a relative parent, a parent carrying macro triggers). `mp_abort` cannot be exercised inside the test session, so those cases had only ever been asserted through the service - the two macro headers now say so rather than pointing at a test that no longer runs them. **2. `is_path` fail-open - fixed.** Both macros now initialise the flag to 0, so an absent ISPATH rule refuses the path. Same testability limit as above: the refusal is an `mp_abort`, so the change is code-review-visible but has no assertion behind it. **3. ACTION route test - fixed, but not as suggested, because the route does not exist on this platform.** A form field or query parameter is never delivered to the program on the SASjs Server STP endpoint (recorded in my last reply), so a test that passes `action` as a parameter cannot make the service list anything. What can be tested is the thing the capture protects, so the resolution is now a macro of its own - `mpe_action.sas` - with seven assertions in `macros/mpe_action.test.sas`: - the ACTION column is used, and upcased; - the column wins over an incoming value; - with no input table the *incoming macro variable is read* - this is the regression assertion, and a default assigned before the read makes it fail; - absent is MISSING and an unrecognised value is BAD, through the column and through the macro variable. Both `makedata` services call it for the dispatch, so the two copies of that logic are gone and the two refusal messages stay in the service, where the child-abort tests already cover them. **4. Hardcoded element ids - fixed.** Every id is now derived from `inputId` (`{{ inputId }}-browse`, `-validate`, `-validate-result`, `-browser`, `-up`, `-use-current`, `-no-folders`, `-truncated`, `-browser-error`). The spec queries the derived ids and has a new case that sets a different `inputId` and asserts the scoped ids exist with no bare `#DCBrowse` in the DOM. **5. Unsequenced list() responses - fixed.** `list()` captures an incrementing request id and both `then` and `catch` return early when a newer request has started. New spec: `list('a')` then `list('b')`, resolve `b` then `a`, and assert the rendered folders and `listingParent` come from `b`. **6. Unlabelled input - fixed.** `@Input() ariaLabel = 'Data Controller location'`, bound as `[attr.aria-label]` on the text input; the configurator's "DC Staging Directory" label now carries `for="dcDirectory"`, which is the `inputId` that parent passes. Spec asserts the attribute and that it reflects the input. **Verification** - `sasjs lint`: exit 0, no findings on any touched file. - `sasjs cbd -t server` to `/Public/app/dc`, then `sasjs test mpe_action makedata mpe_dirlist mpe_writecheck -t server`: 6 suites, all pass (testsetup, mpe_action, mpe_dirlist, mpe_makedatamodel, mpe_writecheck, makedata), 27 assertions, 0 failures (`tests="27" failures="0"`). No re-seed between the deploy and the run - a seeded appLoc only needs a redeploy, and re-seeding would have removed the STP the tests call. - Frontend: `prettier --check` clean on the four touched files; `ng test --include='**/dc-location.component.spec.ts' --include='**/sasjs-configurator.component.spec.ts' --include='**/automatic.component.spec.ts'` -> 30 tests, 0 failures. - Not run: the Viya gate (`mfv_existfile` path) - only the server target exists on this estate, so that branch is code-review only, and I am saying that rather than implying it ran. - agent001

hermes commented

2026-11-03 12:24:04 +00:00

One addition on finding 5, since a spec that never fails is worth nothing: the stale-response case was mutation-checked on the pushed head. With the .then guard removed, ng test --include='**/dc-location.component.spec.ts' gives TOTAL: 1 FAILED, 19 SUCCESS; with the guard restored it is TOTAL: 20 SUCCESS. So the assertion is the guard, not the fixture.

The same check on the earlier full run stands as reported: 30 tests across the dc-location, configurator and automatic specs, 0 failures.

agent001

One addition on finding 5, since a spec that never fails is worth nothing: the stale-response case was mutation-checked on the pushed head. With the `.then` guard removed, `ng test --include='**/dc-location.component.spec.ts'` gives `TOTAL: 1 FAILED, 19 SUCCESS`; with the guard restored it is `TOTAL: 20 SUCCESS`. So the assertion is the guard, not the fixture. The same check on the earlier full run stands as reported: 30 tests across the dc-location, configurator and automatic specs, 0 failures. - agent001

allan merged commit 1d9108109d into release/7.22

2026-11-03 12:39:30 +00:00

allan deleted branch feat/dirlist-browse

2026-11-03 12:39:30 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dc/dc#440