what a host-account module needs from the schema — role-hidden modules, a multi-line input, a status that says why #33

thisilike · 2026-08-11T22:05:22+02:00

thisilike commented

2026-08-11 22:05:22 +02:00

Rebased onto main, and current main merged in since (aa90aaf..7108fb3) — the head under review is now the tree CI and the merge commit will run. What main landed in the meantime changed what is left here, so the description is rewritten rather than patched — see What main absorbed at the end.

Everything below is implemented, tested, and verified in a browser (the caveat on the last two rounds is in Verification). The module it exists for is OpsDeck/module_users, written against exactly this contract.

1. A module can be hidden from the sessions that may not use it

Mounting a module's router under /api/mod/<name> gives it authentication, not authorization. A module that guards every route with requireRole("admin") was still listed by /api/core/modules for everyone: a viewer got a nav entry, a palette entry, and a page that answered 403 to every request it made. The module was locked, not hidden — and for the modules where this matters most (host files, host accounts) the list of what exists is itself worth hiding. The filebrowser hit this too.

Two declarations decide whether the module is visible at all, resolved together in modules/visibility.ts:

the manifest's nav.role, which also covers a module with a custom web frontend — the shell imports and renders that bundle itself, so there are no schema pages to inspect;
a schema page's role, which already existed in the validator and was enforced nowhere. A module whose every page is admin-only is admin-only whether or not its manifest remembered to say so.

/api/core/modules drops what the session may not see, and the schema route serves it projected. A schema whose root page did not survive is not served at all — every client resolves a module by its root page, so half a schema is a first navigation into a 404. That route answers 404 rather than 403 below the role, since a route map is what hiding a module hides.

The projection hides the whole route map, not only the pages (the third review round's finding 2 and 3):

a page above the session's role is dropped, and so is every way in to it — a page: action, a list item's link, a page's parent, a form's successPage. Left in, they render a control whose destination has no route, and the shell answers that with "Page not found" — strictly worse than the access-denied screen the viewer used to get, and no client can repair it, because the page's role left with the page.
an action, a form's submit and a terminal block each carry a role that until now only ever hid client-side, so the paths inside them were served to exactly the session the projection was hiding the route map from. They are projected server-side now; the renderer keeps its own check, which is what a module gets from an older server.
a page the projection emptied is dropped too. Same judgement one level up — a page whose every block was admin-only is an admin page whether or not it said so — and it keeps the response valid, since blocks is min(1): an emptied page (or actions block) is not a smaller schema, it is one a validating client rejects whole.

Dropping a page can empty another one, so the projection runs to a fixpoint. Link targets are resolved against every page and then checked for survival, with the router's own static-beats-param precedence: a target whose page is gone must not fall through to a laxer pattern that happens to still be registered, which is a link that silently navigates somewhere else. A target matching no page at all is left alone — that was broken before any projection existed, and swallowing it hides an authoring bug rather than a page.

Filtering server-side is what makes one change hide the module in the web shell and the Android app at once.

All of it hides; it does not authorize — routes keep their own guards, and the docs say so twice, because a module that sets nav.role and guards nothing has an API open to anyone who types the URL.

2. `textarea` — the multi-line input kind

text is one line. Pasting an SSH key, a config file, or anything with newlines meant a module either mangled it or wrote a custom Svelte page — and a custom frontend is all-or-nothing per module, so one multi-line field cost every page on the phone. Renders monospace with white-space: pre; a base64 blob wrapped mid-token is unreadable.

The same commit put input[type="password"] into the schema form's styled-control list, where it had never been: password was already a rendered input kind, so a secret field sat in a browser-default box next to styled text ones. Named here because it rode in unannounced (third round, finding 5); a comment in FormBlock.svelte now says the same thing.

3. A form can carry a confirmation, and both kinds interpolate

An action gained confirmPhrase on main while this branch was open (see below). A form's submit had neither it nor an interpolated message, so the same operation was guarded on a button and unguarded on a form:

submit: { label: "Delete", method: "POST", path: "/account/{name}/delete",
          confirm: "Deletes {name} and its home directory. There is no undo.",
          confirmPhrase: "{name}" }

confirm now interpolates route params in both blocks — it rendered {name} literally before — and one shared schema covers the phrase on an action and on a form, including the rule that a phrase needs a confirm to say why it is locked.

A {param} the page does not declare is now a load-time error. Everything else that interpolates fails loudly — a bad param in a path is a 404 somebody sees — but a message interpolates to a hole ("Deletes and its home directory") and an unresolved phrase degrades to asking the operator to type {name} literally, on exactly the actions chosen as too dangerous to answer by reflex. A phrase of nothing but whitespace is refused for the same reason: it renders a box that an empty answer satisfies — and that rule holds for a resolved phrase too, not only an authored one.

4. A status can say why

UiStatus.title — a dot-path to the sentence behind the verdict. An account is LOCKED and the reason (no password set, expired 3 days ago) is computed server-side, so no client can re-derive it; a colour whose reason nobody can interrogate is a colour people learn to ignore.
Tooltip (new in @opsdeck/ui) and HealthDot's explain prop — because that sentence cannot go in title="". The native tooltip is unstyled, waits about a second, truncates differently per platform, never appears on a phone, and is drawn outside the page, so it cannot be screenshotted or tested. Positioned fixed from the trigger's rect so it escapes a card's overflow: hidden, flipped by available room, dismissed on Escape, shown on keyboard focus as well as hover. Its window listeners — scroll, resize and now Escape — live only while it is open (third round, finding 4): a table of 200 rows with explain set was 200 permanent keydown listeners.
Icon: users and key — a module managing accounts and SSH keys had no honest nav glyph.
CodeListBlock — the file name moved out of Card's title (small, muted, uppercase: right for "KEYS", wrong for the one string somebody scans the list for) into the card body, and a note now renders alongside content as a caption. It used to appear only when content was missing, so a module could describe a file it could not show and say nothing about one it could — which is the block authorized_keys is rendered with.

Verification

deno fmt --check, deno lint, deno task check, deno task check:svelte (0 errors), 546 server/UI tests + 3 builder tests, all green on the pushed head — including the three module_listing_test.ts cases that failed the leak sanitizer at the old base, which main's 1763b56 fixed and this merge picks up.
New tests: visibility_test.ts — the page projection, plus the link-into-a-dropped-page cases (list link, page: action, parent, successPage), router precedence (static beats param), a link matching no page at all, role-gated action / form / terminal, a page the projection emptied, a root page the projection emptied. module_listing_test.ts — the whole app with a real session store (auth-disabled mode cannot express this, its dev user holding both roles), including one case that asserts on the served BYTES: no /wipe, no account/{name} in what a viewer is handed.
Browser (headless, earlier round, against a throwaway schema-only module with a detail page): the textarea prefills from source, renders monospace with no soft wrap, and a three-line value round-trips through the form verbatim. The delete dialog opens with the message and the phrase both interpolated (Deletes alice and its home directory., asks for alice), focus in the phrase box, Confirm locked; alic stays locked, alice unlocks (both sides are trimmed), and confirming runs the action and closes the dialog.
Not re-run in a browser since: no browser binary on this machine. The projection is the one claim a browser is the weaker instrument for anyway — module_listing_test.ts asserts on the response a real session receives from the real app, which is what every client renders from. The tooltip change moves an existing handler between two registration sites and svelte-check reads the template.

What main absorbed

confirmPhrase on an action landed on main independently while this was open — the declaration, the validator rule, resolveConfirmPhrase, ConfirmDialog's phrase field and its three-stop keyboard trap. The conflicts resolved toward main's version in sdk/ui.ts, ui-schema.ts, ActionsBlock.svelte and ConfirmDialog.svelte, and this branch's duplicate of all of it is gone: what remains of the typed confirmation is section 3. Two review findings dissolved with it — main's phrase resolution already fails closed on an unresolved placeholder, and its ceiling was already 120 rather than 64 (the form's now matches it). The later merge brought the rate-limit sweep off its timer (1763b56), which is what makes the new whole-app tests pass the leak sanitizer.

Review findings, addressed

First round:

Trim — the dialog trims both sides now. The reviewer caught it trimming only the typed text, so a phrase carrying its own space could never match; the other direction is a mobile keyboard appending one and locking a correctly typed word.
Unmatched {param} — a validator cross-check, per the second of the two suggestions, since this is only ever wrong at authoring time.
roleAllows twice — one dependency-free packages/sdk/roles.ts. The server imports it as @opsdeck/sdk; the browser side reaches the source file by relative path, because @opsdeck/sdk is not one of the shared bare specifiers and a bundled import of it would not resolve.
Phrase length 64 — moot; the shared ceiling is main's 120.
Math.random() tooltip id — a module-scope counter: unique by construction rather than unlikely to collide, which is the property aria-describedby needs.

Second round:

The tooltip id comment overclaimed (bbba10f) — ++seq runs at instance init, so a remount takes the next number exactly as a random suffix would. The claim now says only what the counter guarantees.
A resolved phrase could still be whitespace-only (aa90aaf) — resolveConfirmPhrase returns the phrase trimmed, and the dialog derives the box, the focus target, the Tab stops and the match from one trimmed value, so "there is a box" and "the box can be satisfied" stop being separate questions inside the component.

Third round:

The head failed its own tests under the leak sanitizer — current main merged in (e5c9143). 546 + 3 green on 7108fb3.
Links into stripped pages (d000828) — projected, per the suggestion: the server knows exactly which paths it removed. Covers all four ways in, runs to a fixpoint, and resolves targets with the router's own precedence. Section 1 has the reasoning.
docs/modules.md claimed more than the projection did (d000828) — the claim is now true rather than reworded: actions, form submits and terminal blocks are projected server-side too. The doc lists what is dropped, including the emptied-page rule and the deliberate exception (a link matching no page).
<svelte:window on:keydown> (7e913b1) — gone; Escape registers in the existing open effect with scroll and resize, so a closed tooltip holds no window listener.
input[type="password"] drive-by (7108fb3) — named in section 2 and commented in the file. Kept: it is the same list of styled controls the textarea joined, and password was already a rendered kind sitting there unstyled.

Follow-up, not in here

The Android app renders confirm and ignores confirmPhrase and textarea — a separate PR in OpsDeck/mobile. Falling back to the plain confirmation is the intended degradation, and the route is the real guard. It gains the projected schema for free, since the filtering is server-side.
deno task test passes --ignore=packages/module-builder, and a CLI --ignore replaces deno.json's exclude rather than adding to it — so the .claude and data exclusions that every other task honours are silently off for the test task, and locally it walks other branches' worktrees and cloned external modules. CI never sees it. Not touched here; it wants its own one-line PR.

Rebased onto main, and current main merged in since (`aa90aaf..7108fb3`) — the head under review is now the tree CI and the merge commit will run. What main landed in the meantime changed what is left here, so the description is rewritten rather than patched — see **What main absorbed** at the end. Everything below is implemented, tested, and verified in a browser (the caveat on the last two rounds is in **Verification**). The module it exists for is `OpsDeck/module_users`, written against exactly this contract. ## 1. A module can be hidden from the sessions that may not use it Mounting a module's router under `/api/mod/<name>` gives it authentication, not authorization. A module that guards every route with `requireRole("admin")` was still listed by `/api/core/modules` for everyone: a viewer got a nav entry, a palette entry, and a page that answered 403 to every request it made. The module was locked, not hidden — and for the modules where this matters most (host files, host accounts) the list of what exists is itself worth hiding. The filebrowser hit this too. Two declarations decide whether the module is visible at all, resolved together in `modules/visibility.ts`: - the manifest's **`nav.role`**, which also covers a module with a custom web frontend — the shell imports and renders that bundle itself, so there are no schema pages to inspect; - a schema **page's `role`**, which already existed in the validator and was enforced nowhere. A module whose every page is admin-only is admin-only whether or not its manifest remembered to say so. `/api/core/modules` drops what the session may not see, and the schema route serves it projected. A schema whose **root** page did not survive is not served at all — every client resolves a module by its root page, so half a schema is a first navigation into a 404. That route answers 404 rather than 403 below the role, since a route map is what hiding a module hides. **The projection hides the whole route map, not only the pages** (the third review round's finding 2 and 3): - a page above the session's role is dropped, and so is **every way in to it** — a `page:` action, a list item's `link`, a page's `parent`, a form's `successPage`. Left in, they render a control whose destination has no route, and the shell answers that with "Page not found" — strictly worse than the access-denied screen the viewer used to get, and no client can repair it, because the page's `role` left with the page. - an **action**, a form's **`submit`** and a **`terminal`** block each carry a `role` that until now only ever hid client-side, so the paths inside them were served to exactly the session the projection was hiding the route map from. They are projected server-side now; the renderer keeps its own check, which is what a module gets from an older server. - a page the projection **emptied** is dropped too. Same judgement one level up — a page whose every block was admin-only is an admin page whether or not it said so — and it keeps the response *valid*, since `blocks` is `min(1)`: an emptied page (or actions block) is not a smaller schema, it is one a validating client rejects whole. Dropping a page can empty another one, so the projection runs to a fixpoint. Link targets are resolved against every page and then checked for survival, with the router's own static-beats-param precedence: a target whose page is gone must not fall through to a laxer pattern that happens to still be registered, which is a link that silently navigates somewhere else. A target matching no page at all is left alone — that was broken before any projection existed, and swallowing it hides an authoring bug rather than a page. Filtering server-side is what makes one change hide the module in the web shell and the Android app at once. All of it **hides; it does not authorize** — routes keep their own guards, and the docs say so twice, because a module that sets `nav.role` and guards nothing has an API open to anyone who types the URL. ## 2. `textarea` — the multi-line input kind `text` is one line. Pasting an SSH key, a config file, or anything with newlines meant a module either mangled it or wrote a custom Svelte page — and a custom frontend is all-or-nothing per module, so one multi-line field cost every page on the phone. Renders monospace with `white-space: pre`; a base64 blob wrapped mid-token is unreadable. The same commit put `input[type="password"]` into the schema form's styled-control list, where it had never been: `password` was already a rendered input kind, so a secret field sat in a browser-default box next to styled text ones. Named here because it rode in unannounced (third round, finding 5); a comment in `FormBlock.svelte` now says the same thing. ## 3. A form can carry a confirmation, and both kinds interpolate An action gained `confirmPhrase` on main while this branch was open (see below). A form's `submit` had neither it nor an interpolated message, so the same operation was guarded on a button and unguarded on a form: ```ts submit: { label: "Delete", method: "POST", path: "/account/{name}/delete", confirm: "Deletes {name} and its home directory. There is no undo.", confirmPhrase: "{name}" } ``` `confirm` now interpolates route params in both blocks — it rendered `{name}` literally before — and one shared schema covers the phrase on an action and on a form, including the rule that a phrase needs a `confirm` to say why it is locked. **A `{param}` the page does not declare is now a load-time error.** Everything else that interpolates fails loudly — a bad param in a path is a 404 somebody sees — but a message interpolates to a hole ("Deletes and its home directory") and an unresolved phrase degrades to asking the operator to type `{name}` literally, on exactly the actions chosen as too dangerous to answer by reflex. A phrase of nothing but whitespace is refused for the same reason: it renders a box that an empty answer satisfies — and that rule holds for a **resolved** phrase too, not only an authored one. ## 4. A status can say why - **`UiStatus.title`** — a dot-path to the sentence behind the verdict. An account is `LOCKED` and the reason (`no password set`, `expired 3 days ago`) is computed server-side, so no client can re-derive it; a colour whose reason nobody can interrogate is a colour people learn to ignore. - **`Tooltip`** (new in `@opsdeck/ui`) and **`HealthDot`'s `explain` prop** — because that sentence cannot go in `title=""`. The native tooltip is unstyled, waits about a second, truncates differently per platform, never appears on a phone, and is drawn outside the page, so it cannot be screenshotted or tested. Positioned `fixed` from the trigger's rect so it escapes a card's `overflow: hidden`, flipped by available room, dismissed on Escape, shown on keyboard focus as well as hover. Its window listeners — scroll, resize and now Escape — live only while it is open (third round, finding 4): a table of 200 rows with `explain` set was 200 permanent keydown listeners. - **`Icon`: `users` and `key`** — a module managing accounts and SSH keys had no honest nav glyph. - **`CodeListBlock`** — the file name moved out of `Card`'s title (small, muted, uppercase: right for "KEYS", wrong for the one string somebody scans the list for) into the card body, and a `note` now renders *alongside* content as a caption. It used to appear only when content was missing, so a module could describe a file it could not show and say nothing about one it could — which is the block `authorized_keys` is rendered with. ## Verification - `deno fmt --check`, `deno lint`, `deno task check`, `deno task check:svelte` (0 errors), **546 server/UI tests + 3 builder tests, all green** on the pushed head — including the three `module_listing_test.ts` cases that failed the leak sanitizer at the old base, which main's `1763b56` fixed and this merge picks up. - New tests: `visibility_test.ts` — the page projection, plus the link-into-a-dropped-page cases (list `link`, `page:` action, `parent`, `successPage`), router precedence (static beats param), a link matching no page at all, role-gated action / form / terminal, a page the projection emptied, a root page the projection emptied. `module_listing_test.ts` — the whole app with a real session store (auth-disabled mode cannot express this, its dev user holding both roles), including one case that asserts on the served BYTES: no `/wipe`, no `account/{name}` in what a viewer is handed. - Browser (headless, earlier round, against a throwaway schema-only module with a detail page): the textarea prefills from `source`, renders monospace with no soft wrap, and a three-line value round-trips through the form verbatim. The delete dialog opens with the message and the phrase both interpolated (`Deletes alice and its home directory.`, asks for `alice`), focus in the phrase box, Confirm locked; `alic` stays locked, `alice ` unlocks (both sides are trimmed), and confirming runs the action and closes the dialog. - Not re-run in a browser since: no browser binary on this machine. The projection is the one claim a browser is the weaker instrument for anyway — `module_listing_test.ts` asserts on the response a real session receives from the real app, which is what every client renders from. The tooltip change moves an existing handler between two registration sites and svelte-check reads the template. ## What main absorbed `confirmPhrase` on an **action** landed on main independently while this was open — the declaration, the validator rule, `resolveConfirmPhrase`, `ConfirmDialog`'s phrase field and its three-stop keyboard trap. The conflicts resolved toward main's version in `sdk/ui.ts`, `ui-schema.ts`, `ActionsBlock.svelte` and `ConfirmDialog.svelte`, and this branch's duplicate of all of it is gone: what remains of the typed confirmation is section 3. Two review findings dissolved with it — main's phrase resolution already fails closed on an unresolved placeholder, and its ceiling was already 120 rather than 64 (the form's now matches it). The later merge brought the rate-limit sweep off its timer (`1763b56`), which is what makes the new whole-app tests pass the leak sanitizer. ## Review findings, addressed First round: - **Trim** — the dialog trims both sides now. The reviewer caught it trimming only the typed text, so a phrase carrying its own space could never match; the other direction is a mobile keyboard appending one and locking a correctly typed word. - **Unmatched `{param}`** — a validator cross-check, per the second of the two suggestions, since this is only ever wrong at authoring time. - **`roleAllows` twice** — one dependency-free `packages/sdk/roles.ts`. The server imports it as `@opsdeck/sdk`; the browser side reaches the source file by relative path, because `@opsdeck/sdk` is not one of the shared bare specifiers and a bundled import of it would not resolve. - **Phrase length 64** — moot; the shared ceiling is main's 120. - **`Math.random()` tooltip id** — a module-scope counter: unique by construction rather than unlikely to collide, which is the property `aria-describedby` needs. Second round: - **The tooltip id comment overclaimed** (`bbba10f`) — `++seq` runs at instance init, so a remount takes the next number exactly as a random suffix would. The claim now says only what the counter guarantees. - **A resolved phrase could still be whitespace-only** (`aa90aaf`) — `resolveConfirmPhrase` returns the phrase trimmed, and the dialog derives the box, the focus target, the Tab stops and the match from one trimmed value, so "there is a box" and "the box can be satisfied" stop being separate questions inside the component. Third round: - **The head failed its own tests under the leak sanitizer** — current main merged in (`e5c9143`). 546 + 3 green on `7108fb3`. - **Links into stripped pages** (`d000828`) — projected, per the suggestion: the server knows exactly which paths it removed. Covers all four ways in, runs to a fixpoint, and resolves targets with the router's own precedence. Section 1 has the reasoning. - **`docs/modules.md` claimed more than the projection did** (`d000828`) — the claim is now true rather than reworded: actions, form submits and terminal blocks are projected server-side too. The doc lists what is dropped, including the emptied-page rule and the deliberate exception (a link matching no page). - **`<svelte:window on:keydown>`** (`7e913b1`) — gone; Escape registers in the existing `open` effect with scroll and resize, so a closed tooltip holds no window listener. - **`input[type="password"]` drive-by** (`7108fb3`) — named in section 2 and commented in the file. Kept: it is the same list of styled controls the textarea joined, and `password` was already a rendered kind sitting there unstyled. ## Follow-up, not in here - The Android app renders `confirm` and ignores `confirmPhrase` and `textarea` — a separate PR in `OpsDeck/mobile`. Falling back to the plain confirmation is the intended degradation, and the route is the real guard. It gains the projected schema for free, since the filtering is server-side. - `deno task test` passes `--ignore=packages/module-builder`, and a CLI `--ignore` **replaces** `deno.json`'s `exclude` rather than adding to it — so the `.claude` and `data` exclusions that every other task honours are silently off for the test task, and locally it walks other branches' worktrees and cloned external modules. CI never sees it. Not touched here; it wants its own one-line PR.

thisilike added 2 commits

2026-08-11 22:05:22 +02:00

feat(modules): hide a module from the sessions that may not use it 9d82ae6ced

Mounting a module's router under /api/mod/<name> gives it authentication,
not authorization. A module that guards every route with requireRole("admin")
was still listed by /api/core/modules for everyone, so a viewer got a nav
entry, a palette entry and a page that answered 403 to every request it made.
The module was locked, not hidden — and for the modules where this matters
most (host files, host accounts) the list of what exists is itself worth
hiding.

Two declarations now decide it, resolved together in one place
(modules/visibility.ts) so no route answers from a different rule:

- the manifest's `nav.role`, which covers a custom web frontend too — the
  shell imports and renders that bundle itself, so there are no schema pages
  to inspect;
- a schema page's `role`, which already existed and was enforced nowhere. A
  module whose every page is admin-only is admin-only whether or not its
  manifest remembered to say so.

/api/core/modules drops what the session may not see, and the schema route
serves it projected: pages above the requester's role are stripped, and a
schema whose ROOT page did not survive is not served at all — every client
resolves a module by its root page, so half a schema is not a smaller module,
it is a first navigation into a 404.

That route answers 404 rather than 403 below the role, since a route map is
what hiding a module hides. Filtering server-side is also what makes this
one change hide the module in the web shell and the Android app at once.

Both are HIDING, never authorization: routes keep their own guards, and the
docs say so twice, because a module that sets nav.role and guards nothing has
an API open to anyone who types the URL.

feat(schema): a confirmation you have to type, and a multi-line input

Build and Deploy / verify (pull_request) Successful in 1m13s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m16s

Details

0617b859ca

Two additions to the portable UI schema, both found by writing a module that
manages host accounts and running out of vocabulary.

`confirmPhrase` on an action or a form's submit makes the operator TYPE that
text before the button unlocks. `confirm` is a yes/no dialog, which is the
right weight for restarting a container and the wrong weight for `userdel -r`
or revoking a key across every account. It requires `confirm`: a box demanding
a word with no sentence saying why is a puzzle, not a guard.

Both it and `confirm` now interpolate route params. `confirm: "Delete
{name}?"` rendered that literally before, and a typed confirmation whose
phrase is a placeholder would be worse than none — the phrase you want is
almost always the subject of the page.

`textarea` is the multi-line input kind. `text` is one line, so pasting an
SSH key, a config file or anything with newlines meant a module either
mangled it or built a custom Svelte page — and a custom frontend is
all-or-nothing per module, so one multi-line field cost every page on the
phone. It renders monospace with `white-space: pre`, because a base64 blob
wrapped mid-token is unreadable.

ConfirmDialog grows `requirePhrase`, and its keyboard trap generalises from a
swap between two buttons to a cycle over two or three focusables (honouring
Shift+Tab, which two elements hid). Focus lands in the box when there is one;
Enter there confirms only while the phrase matches, and a locked dialog
cannot be confirmed from anywhere. The typed text is cleared when the dialog
closes, so an answer to one prompt cannot unlock the next.

The demo module exercises both. Drive-by: password inputs in a schema form
were unstyled, having never been in the selector list.

Verified in a browser against a schema-only module: prefill, a three-line
value round-tripping through the form, the dialog refusing a near-miss
("wip"), unlocking on "wipe", and the action then running.

thisilike added 1 commit

2026-08-11 23:19:58 +02:00

feat(ui): a users icon and a key icon for the nav

Build and Deploy / verify (pull_request) Successful in 1m14s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m10s

Details

180bca07c5

The users module needs a nav glyph, and every module before it added its own
rather than borrowing one that means something else — the IPMI module's
history is a commit undoing exactly that. `users` is two people (host
accounts, not dashboard logins); `key` is for anything about authorized_keys.
Aliases: accounts/people -> users, ssh -> key.

thisilike added 1 commit

2026-08-11 23:48:16 +02:00

feat(schema): a status can say why

Build and Deploy / verify (pull_request) Successful in 1m16s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m10s

Details

ea31cbffcc

A status dot is a claim, and a claim the reader cannot interrogate is one they
learn to ignore — "CHECK" on an account tells an operator that something is
wrong and nothing about what. The verdict is computed server-side (that is the
point of keeping the schema dumb), so the client cannot re-derive the reason:
it has to travel with it.

`UiStatus` gains `title`, a dot-path to the sentence behind the verdict. The
list, table and tree renderers put it on the HealthDot, and the list also puts
it on the badge, which is what the eye lands on first.

Nothing changes for a status without it: HealthDot already fell back to the
bare state name.

thisilike added 1 commit

2026-08-12 00:04:10 +02:00

feat(ui): an in-page tooltip, because title="" is the browser's UI

Build and Deploy / verify (pull_request) Successful in 1m15s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m10s

Details

22ba7c9257

The status reason shipped as a native `title` attribute, and that was the
wrong surface: it cannot be styled, waits about a second, truncates
differently per platform, never appears on touch, and is drawn outside the
page — so it is also invisible to a screenshot and to any test. An
explanation nobody can see is not an explanation.

`Tooltip` is a small primitive: positioned `fixed` from the trigger's rect, so
it escapes the `overflow: hidden` of the card it sits in, flipped below when
there is no room above, shown on hover AND on keyboard focus, dismissed with
Escape. `HealthDot` gains `explain`, which renders through it and suppresses
the native attribute so the two cannot both appear.

The schema renderers now pass `status.title` to `explain`. A dot without one
keeps the old behaviour, native title and all.

thisilike added 1 commit

2026-08-12 00:18:27 +02:00

fix: what the review found in the schema guards

Build and Deploy / verify (pull_request) Successful in 1m14s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m11s

Details

0d481b1493

Eight findings, all from a review of this branch. Two of them mattered.

**The typed confirmation could vanish silently.** `interpolate` substitutes ""
for a param the route does not have — a typo in the placeholder, or an actions
block on a page without that param — and ConfirmDialog treats an empty phrase
as "no phrase": no box, Confirm enabled. The guard added for `userdel -r`
degraded to a single click, invisibly. Both call sites now fall back to the
literal, so a broken placeholder asks the operator to type "{name}" rather
than waving them through. Fail closed.

**Forward Tab dead-ended in a locked dialog.** `focusables()` included the
disabled Confirm button, and since the trap had already called
preventDefault, Tab from Cancel simply did nothing — with the text box only
reachable by Shift+Tab. Disabled elements are skipped.

**An overview-only frontend was judged by having a frontend.** A module with a
dashboard card, no custom pages and an admin-only schema was listed for a
viewer with `hasUi: false`, which the shell cannot render — it throws into a
console nobody reads, leaving neither nav entry nor card. A schema, when there
is one, decides; a frontend with no schema is judged by the manifest. Test
added.

Tooltip: unique id per instance (two open bubbles shared `id="od-tip"`, so
`aria-describedby` resolved to whichever came first in the document); the
horizontal clamp applies to the box rather than to its centre, so a
left-column tooltip is no longer half off-screen; a capturing scroll listener,
because `scroll` does not bubble and a dot inside DataTable's own overflow
container left the fixed bubble pinned to a stale rect; and the trigger is no
longer `role="button"`, which announced a button that does nothing on Enter.

The new `key` icon's keyhole sat on the ring rather than inside it.

thisilike added 1 commit

2026-08-12 07:38:13 +02:00

fix(schema): a codeList note is a caption, not a fallback

Build and Deploy / verify (pull_request) Successful in 1m16s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m10s

Details

8d01fb5b7b

`note` rendered only when `content` was null, so a module could explain a file
it could NOT show and say nothing about one it could. That is backwards: the
caption is what says where this text came from, what state it is in, or which
of several similar blocks you are looking at.

It now renders under the code as well. Nothing changes for the docker module,
whose only notes describe a file it declined to read (`content: null`).

Long single tokens in a note — a fingerprint, an absolute path — wrap instead
of widening the card.

thisilike added 1 commit

2026-08-12 07:59:22 +02:00

feat(schema): a codeList name is the identifier, not a section label

Build and Deploy / verify (pull_request) Successful in 1m13s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m10s

Details

f3426be840

Card's title is small, muted and uppercase — right for "KEYS", wrong for the
one string somebody scans a list to find: the comment on a key, the path of a
file. The name now renders as a heading inside the card, monospace and in the
ink colour, and wraps rather than truncating, because half a fingerprint or
half a path identifies nothing.

thisilike changed title from ~~WIP: what a host-account module needs from the schema — role-hidden modules, a typed confirmation, a multi-line input~~ to what a host-account module needs from the schema — role-hidden modules, a typed confirmation, a multi-line input

2026-08-15 20:55:25 +02:00

thisilike requested review from julian

2026-08-15 20:55:31 +02:00

julian was assigned by thisilike

2026-08-15 20:55:32 +02:00

thisilike force-pushed feat/schema-guards from f3426be840

Build and Deploy / verify (pull_request) Successful in 1m13s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m10s

Details

to 4eec13c8e1

Build and Deploy / verify (pull_request) Successful in 1m18s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m5s

Details

2026-08-15 20:56:18 +02:00

Compare

julian requested changes

2026-08-15 23:00:04 +02:00

Dismissed

julian left a comment

Reviewed the full diff. The design is sound and the tests cover the claims (viewer vs admin listing, root-page-stripped 404, confirmPhrase-requires-confirm), but two things need action before merge.

1. Merge conflicts — please rebase onto main

The branch is not mergeable. #31 (feat/ui-access-denied-and-rate) landed after your fork point and touches the same files. Conflicts in exactly four:

packages/sdk/ui.ts
packages/server/src/modules/ui-schema.ts
packages/shell/src/schema/blocks/ActionsBlock.svelte
packages/ui/src/components/ConfirmDialog.svelte

Please rebase onto current main (or merge main in) and re-run deno task ci — ConfirmDialog and ui-schema changed on both sides, so the resolution is not mechanical.

2. Scope beyond the description

The diff also adds Tooltip.svelte (new, 144 lines) and reworks HealthDot.svelte, Icon.svelte, and CodeListBlock.svelte (file name moved into the card body, note shown alongside content). None of this is mentioned in the PR description, and it isn't covered by #31 on main either. If module_users needs these, fine — but say so in the description so the record matches the diff, or split them out.

Minor findings (fix or follow-up, your call)

packages/ui/src/components/ConfirmDialog.svelte: the phrase check trims the typed input but not requirePhrase (typed.trim() !== requirePhrase). A phrase with leading/trailing whitespace can never match. Trim both sides.
packages/shell/src/schema/blocks/FormBlock.svelte + ActionsBlock.svelte: if a {param} in confirm/confirmPhrase has no matching route param, the literal {param} is silently kept — the operator would be asked to type {name}. Consider a console warning, or have the validator cross-check placeholders against the path params.
packages/server/src/modules/visibility.ts / packages/shell/src/schema/resolve.ts: roleAllows is implemented twice with slightly different signatures (null vs undefined, readonly vs not). Works, but two codepaths for the same rule will drift — worth a shared helper.
packages/server/src/modules/ui-schema.ts: confirmPhrase max length 64 may be tight once interpolation is in play (long resource names). Consider 128.
packages/ui/src/components/Tooltip.svelte: id from Math.random().toString(36) — a counter or crypto.randomUUID() is collision-free.

No security gaps found: visibility filtering is applied to both the listing and the schema route, the 404-below-role behavior is consistent, and the docs correctly hammer that hiding is not authorization.

Reviewed the full diff. The design is sound and the tests cover the claims (viewer vs admin listing, root-page-stripped 404, confirmPhrase-requires-confirm), but two things need action before merge. ## 1. Merge conflicts — please rebase onto main The branch is not mergeable. #31 (`feat/ui-access-denied-and-rate`) landed after your fork point and touches the same files. Conflicts in exactly four: - `packages/sdk/ui.ts` - `packages/server/src/modules/ui-schema.ts` - `packages/shell/src/schema/blocks/ActionsBlock.svelte` - `packages/ui/src/components/ConfirmDialog.svelte` Please rebase onto current main (or merge main in) and re-run `deno task ci` — ConfirmDialog and ui-schema changed on both sides, so the resolution is not mechanical. ## 2. Scope beyond the description The diff also adds `Tooltip.svelte` (new, 144 lines) and reworks `HealthDot.svelte`, `Icon.svelte`, and `CodeListBlock.svelte` (file name moved into the card body, note shown alongside content). None of this is mentioned in the PR description, and it isn't covered by #31 on main either. If module_users needs these, fine — but say so in the description so the record matches the diff, or split them out. ## Minor findings (fix or follow-up, your call) - `packages/ui/src/components/ConfirmDialog.svelte`: the phrase check trims the typed input but not `requirePhrase` (`typed.trim() !== requirePhrase`). A phrase with leading/trailing whitespace can never match. Trim both sides. - `packages/shell/src/schema/blocks/FormBlock.svelte` + `ActionsBlock.svelte`: if a `{param}` in `confirm`/`confirmPhrase` has no matching route param, the literal `{param}` is silently kept — the operator would be asked to type `{name}`. Consider a console warning, or have the validator cross-check placeholders against the path params. - `packages/server/src/modules/visibility.ts` / `packages/shell/src/schema/resolve.ts`: `roleAllows` is implemented twice with slightly different signatures (null vs undefined, readonly vs not). Works, but two codepaths for the same rule will drift — worth a shared helper. - `packages/server/src/modules/ui-schema.ts`: `confirmPhrase` max length 64 may be tight once interpolation is in play (long resource names). Consider 128. - `packages/ui/src/components/Tooltip.svelte`: id from `Math.random().toString(36)` — a counter or `crypto.randomUUID()` is collision-free. No security gaps found: visibility filtering is applied to both the listing and the schema route, the 404-below-role behavior is consistent, and the docs correctly hammer that hiding is not authorization.

thisilike force-pushed feat/schema-guards from 4eec13c8e1

Build and Deploy / verify (pull_request) Successful in 1m18s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 1m5s

Details

to be7f58a31e

Build and Deploy / verify (pull_request) Successful in 1m33s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 55s

Details

2026-08-21 20:46:02 +02:00

Compare

thisilike changed title from ~~what a host-account module needs from the schema — role-hidden modules, a typed confirmation, a multi-line input~~ to what a host-account module needs from the schema — role-hidden modules, a multi-line input, a status that says why

2026-08-21 20:47:00 +02:00

thisilike commented

2026-08-21 20:47:24 +02:00

Rebased and re-pushed (force, 4eec13c → be7f58a); mergeable again. Description rewritten rather than patched, because the rebase changed what the PR is.

Both blocking items:

Rebase — done, onto cc65b8c. The four conflicts resolved toward main, and that turned out to matter more than a resolution: confirmPhrase on an action had landed on main independently (declaration, validator rule, resolveConfirmPhrase, the dialog's phrase field and its three-stop trap). This branch's duplicate of all of it is gone. What is left of that gap is the form side — submit had neither the phrase nor an interpolated message — plus confirm interpolating route params, which it did not before. Two of your minor findings dissolved with it: main's resolution already fails closed on an unresolved placeholder, and its ceiling was already 120, not 64 (the form's now matches).
Scope — the Tooltip / HealthDot / Icon / CodeListBlock work is section 4 of the description now, with what each is for. It is one feature: UiStatus.title, a dot-path to the sentence behind a verdict, which cannot live in title="" — unstyled, second-long delay, absent on touch, invisible to a screenshot. Kept rather than split, since the schema field and the thing that renders it are the same change.

Minor findings:

Trim — both sides now. Your direction (a phrase carrying its own space could never match) plus the other one (a mobile keyboard appending a space, locking a correctly typed word). A whitespace-only phrase would then be satisfied by an empty box, so the schema refuses one.
Unmatched {param} — took the validator option, not the console warning: this is only ever wrong at authoring time. A browser pass earned its keep here — the first version read only {name} placeholders out of a page path, and real pages spell params account/:name, so it would have rejected every module that uses the feature. Both spellings count now, with a test for each.
roleAllows twice — one packages/sdk/roles.ts, dependency-free on purpose: the server imports it as @opsdeck/sdk, the browser side reaches the source file by relative path, since @opsdeck/sdk is not a shared bare specifier and a bundled import of it would not resolve.
Tooltip id — module-scope counter, unique by construction and stable across mounts.

Verified: fmt / lint / check / svelte-check clean, 500 + 3 tests green, and a headless browser pass over a throwaway schema-only module — textarea prefill and a three-line round trip, then the delete dialog with message and phrase both interpolated, alic locked, alice accepted, action run.

One unrelated thing the run surfaced, left alone here: deno task test's --ignore=packages/module-builder replaces deno.json's exclude instead of adding to it, so the .claude and data exclusions are silently off for that one task — locally it walks other branches' worktrees and cloned external modules and fails on their code. CI never sees it. Its own one-line PR, if you want it.

Rebased and re-pushed (force, `4eec13c` → `be7f58a`); mergeable again. Description rewritten rather than patched, because the rebase changed what the PR is. **Both blocking items:** 1. **Rebase** — done, onto `cc65b8c`. The four conflicts resolved toward main, and that turned out to matter more than a resolution: `confirmPhrase` on an **action** had landed on main independently (declaration, validator rule, `resolveConfirmPhrase`, the dialog's phrase field and its three-stop trap). This branch's duplicate of all of it is gone. What is left of that gap is the form side — `submit` had neither the phrase nor an interpolated message — plus `confirm` interpolating route params, which it did not before. Two of your minor findings dissolved with it: main's resolution already fails closed on an unresolved placeholder, and its ceiling was already 120, not 64 (the form's now matches). 2. **Scope** — the `Tooltip` / `HealthDot` / `Icon` / `CodeListBlock` work is section 4 of the description now, with what each is for. It is one feature: `UiStatus.title`, a dot-path to the sentence behind a verdict, which cannot live in `title=""` — unstyled, second-long delay, absent on touch, invisible to a screenshot. Kept rather than split, since the schema field and the thing that renders it are the same change. **Minor findings:** - **Trim** — both sides now. Your direction (a phrase carrying its own space could never match) plus the other one (a mobile keyboard appending a space, locking a correctly typed word). A whitespace-only phrase would then be satisfied by an empty box, so the schema refuses one. - **Unmatched `{param}`** — took the validator option, not the console warning: this is only ever wrong at authoring time. A browser pass earned its keep here — the first version read only `{name}` placeholders out of a page path, and real pages spell params `account/:name`, so it would have rejected every module that uses the feature. Both spellings count now, with a test for each. - **`roleAllows` twice** — one `packages/sdk/roles.ts`, dependency-free on purpose: the server imports it as `@opsdeck/sdk`, the browser side reaches the source file by relative path, since `@opsdeck/sdk` is not a shared bare specifier and a bundled import of it would not resolve. - **Tooltip id** — module-scope counter, unique by construction and stable across mounts. Verified: fmt / lint / check / svelte-check clean, 500 + 3 tests green, and a headless browser pass over a throwaway schema-only module — textarea prefill and a three-line round trip, then the delete dialog with message and phrase both interpolated, `alic` locked, `alice ` accepted, action run. One unrelated thing the run surfaced, left alone here: `deno task test`'s `--ignore=packages/module-builder` **replaces** `deno.json`'s `exclude` instead of adding to it, so the `.claude` and `data` exclusions are silently off for that one task — locally it walks other branches' worktrees and cloned external modules and fails on their code. CI never sees it. Its own one-line PR, if you want it.

thisilike requested review from julian

2026-08-21 20:47:33 +02:00

julian requested changes

2026-08-21 23:00:00 +02:00

Dismissed

julian left a comment

Substance verified — visibility projection, validator cross-check, shared roleAllows, both-side trim, tooltip counter id all check out against the claims. Two small items before merge:

Tooltip id is not stable across remounts. ++seq runs at instance init, so a remounted component gets a new id — the module-script comment and the PR description both claim mount-stability the code does not have. Either make the claim match the code (it is "unique by construction", which is the part that matters) or derive the id from something mount-stable.
resolveConfirmPhrase can still return a whitespace-only phrase. The validator refuses an authored one, but the resolved value is what the dialog compares, and ConfirmDialog's locked gate checks confirmPhrase !== "" untrimmed — a whitespace-only resolved phrase auto-unlocks. Unreachable today (URL segments are non-empty, unresolved keeps the template), but one trim() at the return closes the class instead of relying on those two facts staying true.

Substance verified — visibility projection, validator cross-check, shared roleAllows, both-side trim, tooltip counter id all check out against the claims. Two small items before merge: 1. **Tooltip id is not stable across remounts.** `++seq` runs at instance init, so a remounted component gets a new id — the module-script comment and the PR description both claim mount-stability the code does not have. Either make the claim match the code (it is "unique by construction", which is the part that matters) or derive the id from something mount-stable. 2. **`resolveConfirmPhrase` can still return a whitespace-only phrase.** The validator refuses an authored one, but the resolved value is what the dialog compares, and `ConfirmDialog`'s `locked` gate checks `confirmPhrase !== ""` untrimmed — a whitespace-only resolved phrase auto-unlocks. Unreachable today (URL segments are non-empty, unresolved keeps the template), but one `trim()` at the return closes the class instead of relying on those two facts staying true.

packages/shell/src/schema/resolve.ts Outdated

Defense-in-depth: return the phrase trimmed (or have the caller treat a whitespace-only phrase as absent). ConfirmDialog's locked gate is confirmPhrase !== "" untrimmed, so a whitespace-only resolved phrase would satisfy the gate with an empty box. Unreachable via route params today, but cheap to close permanently.

Defense-in-depth: return the phrase trimmed (or have the caller treat a whitespace-only phrase as absent). ConfirmDialog's `locked` gate is `confirmPhrase !== ""` untrimmed, so a whitespace-only resolved phrase would satisfy the gate with an empty box. Unreachable via route params today, but cheap to close permanently.

packages/ui/src/components/Tooltip.svelte

					
				@ -0,0 +2,4 @@

				  // Shared by every instance, so an id is unique BY CONSTRUCTION. A random

				  // suffix is only unlikely to collide, and it changes on every mount, which

				  // makes a DOM snapshot in a test or a screenshot diff noisy for no reason.

				  let seq = 0;

Comment overclaims: ++seq increments on every instance init, so a remount gets a new id. "Unique by construction" is true; "stable across mounts" is not. Fix the comment (and PR description), or derive the id mount-stably.

Comment overclaims: `++seq` increments on every instance init, so a remount gets a new id. "Unique by construction" is true; "stable across mounts" is not. Fix the comment (and PR description), or derive the id mount-stably.

thisilike added 2 commits

2026-08-21 23:18:52 +02:00

docs(ui): say what the tooltip id counter guarantees, and what it does not bbba10fd8b

The counter makes an id unique BY CONSTRUCTION, which is the property
`aria-describedby` needs — two triggers must never point at one bubble. It
does not make an id stable across mounts: `++seq` runs at instance init, so a
remount takes the next number exactly as a random suffix would. The comment
claimed both.

Nothing depends on the claim, which is the point: a comment promising
stability invites the test that asserts on the literal id, and that test would
pass until something above it remounted.

fix(ui): a phrase of nothing but spaces is no phrase, not an open lock

Build and Deploy / verify (pull_request) Successful in 1m36s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 56s

Details

aa90aaf7b6

`ConfirmDialog` drew the phrase box whenever `confirmPhrase !== ""` and
compared the two sides trimmed, so a phrase that was only whitespace rendered
a box an empty answer satisfied — strictly worse than no box, since it reads
as a guard while gating nothing.

Two halves of one rule, both closed:

- `resolveConfirmPhrase` returns the resolved phrase trimmed, so a template
  that only became whitespace when a route param filled it comes out as "" —
  which is how the dialog spells "no phrase required". The validator already
  refused an authored whitespace-only phrase; this covers the one it cannot
  see.
- the dialog derives one trimmed value and decides the box, the focus target,
  the Tab stops and the match from it, so "there is a box" and "the box can be
  satisfied" stop being separate questions inside the component. A caller that
  never went through the schema gets the same rule.

Unreachable through route params today (a URL segment is non-empty, and an
unresolved placeholder keeps its template) — which is exactly the pair of
facts a `trim()` means nobody has to keep true.

thisilike commented

2026-08-21 23:19:59 +02:00

Both items done, be7f58a..aa90aaf.

1. Tooltip id (bbba10f) — took the "make the claim match the code" branch. ++seq runs at instance init, so a remount takes the next number exactly as a random suffix would; there is nothing mount-stable to derive from that is also unique per instance (the text is not — two triggers can carry the same sentence). The comment now claims only uniqueness by construction, names aria-describedby as the reason that is the property that matters, and says explicitly that nothing may assert on the literal id. PR description corrected the same way. No behaviour change.

2. Whitespace-only phrase (aa90aaf) — closed on both halves rather than one, since the two are the same rule read from either end:

resolveConfirmPhrase returns the resolved phrase trim()ed, so a template that only became whitespace when a param filled it comes out "" — which is how the dialog already spells "no phrase required". The validator covers the authored case; this covers the one it cannot see.
ConfirmDialog derives one trimmed value and decides the box, the initial focus target, the Tab stops and the match from it. Previously the box was gated on confirmPhrase !== "" and the match on its trim, which is exactly the split that lets a lock exist that an empty answer opens. Now "there is a box" and "the box can be satisfied" are one question inside the component, so a caller that never went through the schema (DependentsConfirm, RunPage) gets the rule too.

Your unreachability analysis holds — a URL segment is non-empty and an unresolved placeholder keeps its template — which is why this is worth a trim(): it stops depending on those two facts staying true.

New test a resolved phrase that is only whitespace is no phrase in resolve_test.ts covers a param that fills to spaces, a whitespace-only template, surrounding whitespace on a real phrase, and an unresolved template keeping its gate regardless.

deno fmt --check, deno lint, deno task check, deno task check:svelte (0 errors), 501 + 3 tests green. Not re-run in a browser: no browser binary on this machine, and the dialog's behaviour for any phrase that was already valid is unchanged (a trim of an already-trimmed phrase is identity). The earlier headless run's assertions — alic locked, alice unlocking — are exactly the paths the shared derived value now serves.

Both items done, `be7f58a..aa90aaf`. **1. Tooltip id (`bbba10f`)** — took the "make the claim match the code" branch. `++seq` runs at instance init, so a remount takes the next number exactly as a random suffix would; there is nothing mount-stable to derive from that is also unique per instance (the text is not — two triggers can carry the same sentence). The comment now claims only uniqueness by construction, names `aria-describedby` as the reason that is the property that matters, and says explicitly that nothing may assert on the literal id. PR description corrected the same way. No behaviour change. **2. Whitespace-only phrase (`aa90aaf`)** — closed on both halves rather than one, since the two are the same rule read from either end: - `resolveConfirmPhrase` returns the resolved phrase `trim()`ed, so a template that only became whitespace when a param filled it comes out `""` — which is how the dialog already spells "no phrase required". The validator covers the authored case; this covers the one it cannot see. - `ConfirmDialog` derives one trimmed value and decides the box, the initial focus target, the Tab stops and the match from it. Previously the box was gated on `confirmPhrase !== ""` and the match on its trim, which is exactly the split that lets a lock exist that an empty answer opens. Now "there is a box" and "the box can be satisfied" are one question inside the component, so a caller that never went through the schema (`DependentsConfirm`, `RunPage`) gets the rule too. Your unreachability analysis holds — a URL segment is non-empty and an unresolved placeholder keeps its template — which is why this is worth a `trim()`: it stops depending on those two facts staying true. New test `a resolved phrase that is only whitespace is no phrase` in `resolve_test.ts` covers a param that fills to spaces, a whitespace-only template, surrounding whitespace on a real phrase, and an unresolved template keeping its gate regardless. `deno fmt --check`, `deno lint`, `deno task check`, `deno task check:svelte` (0 errors), 501 + 3 tests green. Not re-run in a browser: no browser binary on this machine, and the dialog's behaviour for any phrase that was already valid is unchanged (a trim of an already-trimmed phrase is identity). The earlier headless run's assertions — `alic` locked, `alice ` unlocking — are exactly the paths the shared derived value now serves.

thisilike requested review from julian

2026-08-21 23:20:03 +02:00

julian requested changes

2026-08-22 12:16:59 +02:00

Dismissed

julian left a comment

Round three, against aa90aaf. Both previous items check out: the tooltip id comment now claims only what ++seq guarantees, and resolveConfirmPhrase trims at the return with ConfirmDialog deriving box / focus / Tab stops / match from the one trimmed value — resolve_test.ts covers the whitespace cases as described.

Gates on my side: deno fmt --check, deno lint, deno task check, deno task check:svelte all clean (the one svelte-check warning is pre-existing and not in a file this PR touches). git merge-tree against current main is conflict-free.

Blocking

1. The branch as pushed fails its own new tests under Deno's leak sanitizer.

All three tests in module_listing_test.ts fail on aa90aaf:

error: Leaks detected:
  - An interval was started in this test, but never completed.
    at rateLimit (packages/server/src/util/ratelimit.ts:16:3)
    at createApp (packages/server/src/app.ts:174:22)
    at withApp (packages/server/tests/module_listing_test.ts:121:15)

At your base (cc65b8c) rateLimit starts a setInterval that createApp never clears, so any test that builds the whole app leaks two intervals. Main has since moved that sweep onto the request path (1763b56), and with origin/main merged in the same files pass 44/44 — so the fix is simply to merge or rebase onto current main, so the head under review is the thing CI and the merge commit will actually run. "501 + 3 green" is not reproducible on the head as pushed, which is the claim I would otherwise be merging on.

Non-blocking (please address or say why not)

2. visibility.ts:39 — pages are projected, links into them are not. A list item's link.page or an action's page: that targets a stripped page still renders for the viewer. The shell's router then has no route (router.match is null) and App.svelte:20 shows "Page not found" — which is worse than what the viewer got before this PR (the page rendered and the data request hit the access-denied screen from #31). sdk/ui.ts:39 says "clients hide links to pages the user lacks"; nothing in the shell implements that, and after projection it cannot, because the page's role left with the page. Suggest stripping page: actions and item.links whose target path was dropped in the same projection (the server knows exactly which paths it removed), or at minimum documenting that anything linking into a role-gated page must carry a matching role itself.

3. docs/modules.md:562 — "blocks and actions carry their own role for the same purpose." The projection drops pages only. An action or form submit with role: "admin" is still served to a viewer, path included, so part of the route map the description says is hidden is not. Either project role-gated actions/submits server-side as well, or reword the doc to say those are client-hidden only (and the rest of the sentence about "the same purpose" goes).

4. Tooltip.svelte:87 — <svelte:window on:keydown> is the legacy directive in a runes component (CommandPalette uses onkeydown), and it registers one window listener per instance for the instance's lifetime — a table of 200 rows with explain set is 200 keydown listeners. The open effect already exists; register the Escape handler there.

5. FormBlock.svelte:475 — input[type="password"] styling is a drive-by the description does not mention. Name it or drop it.

Verification notes

Full suite could not run in a fresh worktree on this box (LoadLibraryExW failed — DuckDB DLL under the scratch path, environment, not you), so the server and schema tests ran in the main checkout. external_modules_test.ts failed once in the full run on your head and passed in isolation and on the merged tree — treating that as a flake.

Round three, against `aa90aaf`. Both previous items check out: the tooltip id comment now claims only what `++seq` guarantees, and `resolveConfirmPhrase` trims at the return with `ConfirmDialog` deriving box / focus / Tab stops / match from the one trimmed value — `resolve_test.ts` covers the whitespace cases as described. Gates on my side: `deno fmt --check`, `deno lint`, `deno task check`, `deno task check:svelte` all clean (the one svelte-check warning is pre-existing and not in a file this PR touches). `git merge-tree` against current main is conflict-free. ## Blocking **1. The branch as pushed fails its own new tests under Deno's leak sanitizer.** All three tests in `module_listing_test.ts` fail on `aa90aaf`: ``` error: Leaks detected: - An interval was started in this test, but never completed. at rateLimit (packages/server/src/util/ratelimit.ts:16:3) at createApp (packages/server/src/app.ts:174:22) at withApp (packages/server/tests/module_listing_test.ts:121:15) ``` At your base (`cc65b8c`) `rateLimit` starts a `setInterval` that `createApp` never clears, so any test that builds the whole app leaks two intervals. Main has since moved that sweep onto the request path (`1763b56`), and with `origin/main` merged in the same files pass 44/44 — so the fix is simply to merge or rebase onto current main, so the head under review is the thing CI and the merge commit will actually run. "501 + 3 green" is not reproducible on the head as pushed, which is the claim I would otherwise be merging on. ## Non-blocking (please address or say why not) **2. `visibility.ts:39` — pages are projected, links into them are not.** A list item's `link.page` or an action's `page:` that targets a stripped page still renders for the viewer. The shell's router then has no route (`router.match` is null) and `App.svelte:20` shows "Page not found" — which is worse than what the viewer got before this PR (the page rendered and the data request hit the access-denied screen from #31). `sdk/ui.ts:39` says "clients hide links to pages the user lacks"; nothing in the shell implements that, and after projection it cannot, because the page's `role` left with the page. Suggest stripping `page:` actions and `item.link`s whose target path was dropped in the same projection (the server knows exactly which paths it removed), or at minimum documenting that anything linking into a role-gated page must carry a matching `role` itself. **3. `docs/modules.md:562` — "blocks and actions carry their own `role` for the same purpose."** The projection drops pages only. An action or form submit with `role: "admin"` is still served to a viewer, path included, so part of the route map the description says is hidden is not. Either project role-gated actions/submits server-side as well, or reword the doc to say those are client-hidden only (and the rest of the sentence about "the same purpose" goes). **4. `Tooltip.svelte:87` — `<svelte:window on:keydown>`** is the legacy directive in a runes component (`CommandPalette` uses `onkeydown`), and it registers one window listener per instance for the instance's lifetime — a table of 200 rows with `explain` set is 200 keydown listeners. The `open` effect already exists; register the Escape handler there. **5. `FormBlock.svelte:475` — `input[type="password"]` styling** is a drive-by the description does not mention. Name it or drop it. ## Verification notes Full suite could not run in a fresh worktree on this box (`LoadLibraryExW failed` — DuckDB DLL under the scratch path, environment, not you), so the server and schema tests ran in the main checkout. `external_modules_test.ts` failed once in the full run on your head and passed in isolation and on the merged tree — treating that as a flake.

thisilike added 4 commits

2026-08-23 20:55:29 +02:00

Merge remote-tracking branch 'origin/main' into feat/schema-guards e5c914375b

fix(schema): the projection hides the route map, not only the pages d000828579

A page's `role` dropped the page and left every way in to it: a `page:`
action, a list item's `link`, a `parent`, a `successPage`. The shell then
had no route to match, so the viewer got "Page not found" — worse than
the access-denied screen a guarded route gave them before, and no client
could fix it, because the page's `role` left with the page.

The same hole one level down: an action, a form's `submit` and a
`terminal` block each carry a `role` that only ever hid client-side, so
part of the route map the projection exists to hide was served anyway.
They are projected here now, and the renderer keeps its own check — that
is what a module gets from an older server.

A page left with no blocks goes too. Same judgement as the role check
above it (a page whose every block was admin-only is an admin page
whether or not it said so), and it keeps the response VALID, since
`blocks` is `min(1)`: an emptied page or actions block is not a smaller
schema, it is one a validating client rejects whole.

Dropping a page can empty another, so this runs to a fixpoint. Link
targets resolve against every page and are then checked for survival,
with the router's own static-beats-param precedence: a target whose page
is gone must not fall through to a laxer pattern that happens to still be
registered, which is a link that silently navigates somewhere else. A
target matching no page at all is left alone — that was broken before any
projection, and swallowing it hides an authoring bug, not a page.

fix(ui): a tooltip's window listeners live only while it is open 7e913b18e9

`<svelte:window on:keydown>` is the legacy directive in a runes component,
and it registered one window listener per instance for the instance's
lifetime: a table of 200 rows with `explain` set is 200 listeners asking
whether a key was Escape on behalf of a tooltip nobody is looking at. The
scroll/resize effect already had the right shape, so Escape moves into it.

style(schema): say why a password input joined the styled controls

Build and Deploy / verify (pull_request) Successful in 13m6s

Details

Build and Deploy / build (pull_request) Has been skipped

Details

Dependency Check / dependencies (pull_request) Successful in 59s

Details

7108fb3c8f

`password` was already a rendered input kind and had never been in the
selector list, so a secret field sat unstyled next to styled text ones.
It rode in with the textarea work unannounced; the comment names it.

thisilike commented

2026-08-23 20:57:09 +02:00

All five done, aa90aaf..7108fb3.

1. The head failing its own tests (blocking) — current main merged in (e5c9143). Your diagnosis was exact: rateLimit's setInterval at my base, gone on main in 1763b56. On the pushed head the three module_listing_test.ts cases pass, and the full run is 546 + 3 green with deno fmt --check, deno lint, deno task check and deno task check:svelte (0 errors) clean. Merged rather than rebased so the four earlier conflict resolutions stay reviewable as the commits you already read.

2. Links into stripped pages (d000828) — took the projection option. The server does know exactly which paths it removed, and it turned out to be the only place that can know: after projection a page's role has left with the page, so no client can reconstruct the rule.

All four ways in are covered, not just the two you named: a page: action, a list item's link, a page's parent, a form's successPage. Two things fell out of it that were not obvious until it was written:

It needs a fixpoint. Which pages survive decides which links survive, and dropping a link can empty a block, which can empty a page, which drops more links. So: project, drop the pages left with no blocks, repeat. Each round removes at least one page, so it terminates; the test a page the projection emptied is dropped, and takes its links with it is exactly the two-round case.
A target has to be resolved the way the router will resolve it. Static segments beat params, so a link to account/new must be stripped when that page is admin-only even though account/:name also matches it and survives. Resolving against the surviving pages instead of all of them turns a hidden page into a link that silently navigates somewhere the operator did not ask for. Pinned in a link is resolved the way the router will resolve it.

One deliberate exception: a target matching no page at all is left alone. That was broken before any projection existed, and swallowing it would hide an authoring bug rather than a page.

3. docs/modules.md overclaiming (d000828) — made the claim true rather than rewording it, since the sentence described the behaviour I actually wanted. An action's role, a form submit's and a terminal block's are projected server-side now. The reasoning is yours from finding 2, one level down: the path inside a role-gated action is part of the route map, and half a hidden route map is not hidden. A form goes whole when its submit is above the session — a read-only rendering of a form is a control that cannot be used and a path that did not need serving.

Two things that constrain how this is allowed to work, both now in the doc:

The renderer keeps its own roleAllows check. It is not redundant: it is what a module gets from a server older than this commit.
An emptied block or page cannot simply be left empty. actions and blocks are both min(1), so an emptied one is not a smaller schema — it is one a validating client rejects whole, taking the module with it. Hence: an emptied actions block is dropped, and a page left with no blocks is dropped, which is the same judgement as a page's own role one level up.

4. <svelte:window on:keydown> (7e913b1) — gone. Escape registers inside the existing open effect alongside scroll and resize, so a closed tooltip holds no window listener at all and your 200-row table holds none until one opens.

5. input[type="password"] (7108fb3) — named, kept. It is not a restyle: password was already a rendered input kind that had never been in FormBlock's selector list, so a secret field sat in a browser-default box beside styled text ones. It rode in with the textarea because that is the same list. Section 2 of the description says so, and a comment in the file says it where the next reader will be.

Verification. Gates as above. New tests in visibility_test.ts: a list link and a page: action into a dropped page, router precedence, a link matching no page, a role-gated action / form / terminal (asserting on the serialized bytes — no /wipe, no /pty), a page the projection emptied, a root page the projection emptied, parent and successPage. In module_listing_test.ts, the same claim through the whole app with a real session store, asserting on the response body a viewer is actually handed.

Not re-run in a browser: still no browser binary here. For this round I would argue that is not the weaker evidence — the claim is about what a session is served, and module_listing_test.ts asserts on the bytes leaving the real app, which is what the shell and the phone both render from. The tooltip change moves one handler between two registration sites, and svelte-check reads the template.

All five done, `aa90aaf..7108fb3`. **1. The head failing its own tests (blocking)** — current main merged in (`e5c9143`). Your diagnosis was exact: `rateLimit`'s `setInterval` at my base, gone on main in `1763b56`. On the pushed head the three `module_listing_test.ts` cases pass, and the full run is **546 + 3 green** with `deno fmt --check`, `deno lint`, `deno task check` and `deno task check:svelte` (0 errors) clean. Merged rather than rebased so the four earlier conflict resolutions stay reviewable as the commits you already read. **2. Links into stripped pages** (`d000828`) — took the projection option. The server does know exactly which paths it removed, and it turned out to be the only place that can know: after projection a page's `role` has left with the page, so no client can reconstruct the rule. All four ways in are covered, not just the two you named: a `page:` action, a list item's `link`, a page's `parent`, a form's `successPage`. Two things fell out of it that were not obvious until it was written: - *It needs a fixpoint.* Which pages survive decides which links survive, and dropping a link can empty a block, which can empty a page, which drops more links. So: project, drop the pages left with no blocks, repeat. Each round removes at least one page, so it terminates; the test `a page the projection emptied is dropped, and takes its links with it` is exactly the two-round case. - *A target has to be resolved the way the router will resolve it.* Static segments beat params, so a link to `account/new` must be stripped when that page is admin-only **even though `account/:name` also matches it and survives**. Resolving against the surviving pages instead of all of them turns a hidden page into a link that silently navigates somewhere the operator did not ask for. Pinned in `a link is resolved the way the router will resolve it`. One deliberate exception: a target matching no page at all is left alone. That was broken before any projection existed, and swallowing it would hide an authoring bug rather than a page. **3. `docs/modules.md` overclaiming** (`d000828`) — made the claim true rather than rewording it, since the sentence described the behaviour I actually wanted. An action's `role`, a form `submit`'s and a `terminal` block's are projected server-side now. The reasoning is yours from finding 2, one level down: the path inside a role-gated action is part of the route map, and half a hidden route map is not hidden. A form goes whole when its `submit` is above the session — a read-only rendering of a form is a control that cannot be used and a path that did not need serving. Two things that constrain how this is allowed to work, both now in the doc: - The renderer keeps its own `roleAllows` check. It is not redundant: it is what a module gets from a server older than this commit. - An emptied block or page cannot simply be left empty. `actions` and `blocks` are both `min(1)`, so an emptied one is not a smaller schema — it is one a validating client rejects whole, taking the module with it. Hence: an emptied actions block is dropped, and a page left with no blocks is dropped, which is the same judgement as a page's own `role` one level up. **4. `<svelte:window on:keydown>`** (`7e913b1`) — gone. Escape registers inside the existing `open` effect alongside scroll and resize, so a closed tooltip holds no window listener at all and your 200-row table holds none until one opens. **5. `input[type="password"]`** (`7108fb3`) — named, kept. It is not a restyle: `password` was already a rendered input kind that had never been in `FormBlock`'s selector list, so a secret field sat in a browser-default box beside styled text ones. It rode in with the textarea because that is the same list. Section 2 of the description says so, and a comment in the file says it where the next reader will be. **Verification.** Gates as above. New tests in `visibility_test.ts`: a list `link` and a `page:` action into a dropped page, router precedence, a link matching no page, a role-gated action / form / terminal (asserting on the serialized bytes — no `/wipe`, no `/pty`), a page the projection emptied, a root page the projection emptied, `parent` and `successPage`. In `module_listing_test.ts`, the same claim through the whole app with a real session store, asserting on the response body a viewer is actually handed. Not re-run in a browser: still no browser binary here. For this round I would argue that is not the weaker evidence — the claim is about what a session is *served*, and `module_listing_test.ts` asserts on the bytes leaving the real app, which is what the shell and the phone both render from. The tooltip change moves one handler between two registration sites, and svelte-check reads the template.

thisilike requested review from julian

2026-08-23 20:57:14 +02:00

julian approved these changes

2026-08-23 22:30:36 +02:00

julian left a comment

LGTM — round 4, no findings.

Full diff reviewed (26 files, head 7108fb3), plus an independent spot-check of packages/server/src/modules/visibility.ts:

Fixpoint terminates: each non-breaking round shrinks kept by at least one page; an empty kept breaks on the first pass.
No cache mutation: projection copies via spread only on change and returns the same object when untouched; the original ui is never modified.
reachable is sound: targets resolve against all pages with static-beats-param precedence before the survival check, so a dropped page's link cannot fall through to a laxer pattern; a target matching no page is deliberately left alone.
Root check (path === "") is correct — an empty target splits to zero segments and matches only the root pattern.
untouched identity comparison holds: any drop changes array length, any projection changes the object reference.

Interpolation edge cases, Tooltip listener lifecycle (listeners only while open, Escape registered in the open effect), textarea round-tripping, and the new test assertions (including the served-bytes leak check in module_listing_test.ts) all verified clean. All round-3 fixes are present in the diff: projected links into dropped pages (all four ways in), server-side projection of action/submit/terminal roles, the emptied-page rule, input[type="password"] styling named and commented.

LGTM — round 4, no findings. Full diff reviewed (26 files, head `7108fb3`), plus an independent spot-check of `packages/server/src/modules/visibility.ts`: - **Fixpoint terminates**: each non-breaking round shrinks `kept` by at least one page; an empty `kept` breaks on the first pass. - **No cache mutation**: projection copies via spread only on change and returns the same object when untouched; the original `ui` is never modified. - **`reachable` is sound**: targets resolve against *all* pages with static-beats-param precedence before the survival check, so a dropped page's link cannot fall through to a laxer pattern; a target matching no page is deliberately left alone. - **Root check** (`path === ""`) is correct — an empty target splits to zero segments and matches only the root pattern. - **`untouched` identity comparison** holds: any drop changes array length, any projection changes the object reference. Interpolation edge cases, Tooltip listener lifecycle (listeners only while open, Escape registered in the `open` effect), textarea round-tripping, and the new test assertions (including the served-bytes leak check in `module_listing_test.ts`) all verified clean. All round-3 fixes are present in the diff: projected links into dropped pages (all four ways in), server-side projection of action/submit/terminal roles, the emptied-page rule, `input[type="password"]` styling named and commented.

thisilike merged commit 4e8defb52d into main

2026-08-24 14:16:28 +02:00

thisilike deleted branch feat/schema-guards

2026-08-24 14:16:28 +02:00

thisilike referenced this pull request from a commit

2026-08-24 14:16:28 +02:00

Merge pull request 'what a host-account module needs from the schema — role-hidden modules, a multi-line input, a status that says why' (#33) from feat/schema-guards into main

Sign in to join this conversation.

No reviewers

No labels

No milestone

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

OpsDeck/core!33

Rows
Columns

what a host-account module needs from the schema — role-hidden modules, a multi-line input, a status that says why #33

1. A module can be hidden from the sessions that may not use it

2. textarea — the multi-line input kind

3. A form can carry a confirmation, and both kinds interpolate

4. A status can say why

Verification

What main absorbed

Review findings, addressed

Follow-up, not in here

1. Merge conflicts — please rebase onto main

2. Scope beyond the description

Minor findings (fix or follow-up, your call)

Blocking

Non-blocking (please address or say why not)

Verification notes

2. `textarea` — the multi-line input kind