feat(storage): see what a module is holding, and drop what outlived it #46

Merged
julian merged 9 commits from feat/issue-39-storage-enumeration into main 2026-08-21 23:20:32 +02:00
Owner

Closes #39.

ctx.storage capped each value at 8 MB and bounded nothing else: no enumeration, no way to remove rows for a module that was deleted or renamed, and updated_at_ms written on every set and read by nothing.

Store

ModuleKv gains three methods:

method answers
list(module, limit?) { key, bytes, updatedAtMs }[] — never values. limit becomes a SQL LIMIT, so it bounds the read itself, not just the response — the query runs on DuckDB's one serialized queue
usage() per-module { keys, bytes, updatedAtMs }, every module holding rows
deleteAll(module) rows removed, counted through DELETE … RETURNING so the number and the delete are one statement

Sizes come from strlen, not length: DuckDB's length counts characters and the cap in set is in UTF-8 bytes, and two units for one number is how a 3 MB value reads as 1 MB. There is a test pinning that.

usage and deleteAll are unscoped by design and never appear on ctx. They are reached only through ModuleHost.moduleStorage, which is typed Pick<ModuleKv, "list" | "usage" | "deleteAll"> (OperatorModuleKv) — so "operator surfaces only" is structural, not a comment: get/set cannot leak along with them.

Module-facing: ctx.storage.list()

Its own keys, with sizes and write times, never the values — asking what you hold must not cost the whole footprint to answer. It passes no limit: a module's contract is every key; the bound exists for the operator route below.

It takes no module argument. Like get/set/delete, the facade in host.ts closes over the module's own name, so list can only ever answer for the caller. A test proves that end to end — a real module whose register(ctx) exposes ctx.storage.list() on a route, with a neighbour module's rows in the same table:

const { keys } = await res.json();
assertEquals(keys.map((k) => k.key), ["mine-1", "mine-2"]);
assertFalse(JSON.stringify(keys).includes("secret"));

Confirmed it has teeth: pointing the facade at the neighbour's name fails it.

The use it exists for is expiring your own entries — a cache keyed per image or per remote object can otherwise only delete the keys it still remembers, which are not the ones that accumulate.

Operator-facing: /system

Dropping a whole module's rows is deliberately not on ctx: the rows most in need of dropping belong to a module that no longer loads and therefore has no ctx at all.

  • /system payload gains moduleStorage: { modules, totalBytes, unidentified }, each row carrying configuredthree-valued. true when the deployment still asks for the module; false when nothing does — the orphan case the issue is about; null when the answer is not knowable this boot. The last state exists because a failure can record only the repo slug while the rows live under the manifest name: a slug can never match them, so while any external module failed before its manifest was readable, an unmatched row must not be called orphaned. ModuleFailure carries nameFromManifest to make that decidable — and the recovery keys on the manifest being READABLE, not acceptable: readManifestName parses the schema without the SDK gate, so a module requiring a newer SDK major — the one failure mode that never resolves itself — still names its failure, from whatever checkout is on disk (the previous boot's for a clone failure, the fresh one for a manifest the gate refused). null is therefore reserved for a failure with no readable manifest anywhere. unidentified lists those failures by name (still slugs, redacted), so the page can say which module is unaccounted for instead of a deployment-wide flag.
  • A read of usage() that throws is a thrown /system, not an empty section: an empty store answers [], so a catch there could only dress a database failure up as "Nothing stored" on the page whose job is finding rows.
  • GET|DELETE /api/core/system/module-storage/:module, both adminOnly, both under the app-wide CSRF guard, with the delete logged (module, removed, by). The GET caps at 500 keys in the SQL and reports the true total from usage()'s row count; both routes have a non-admin 403 test run against real sessions rather than the auth-disabled static admin.
  • A "Module storage" section on the system page: totals, an expandable key list fetched per module (a module may hold thousands; the page should not pay for them to show a list nobody expanded — and says "showing the first N of M keys" when the cap cut it; key names are module-chosen, image refs with digests included, so the pane wraps them instead of blowing the label column open), and a two-click Clear. Its request failure lands in a visible message rather than a silently disarmed button; its success reports the server's own count — "Cleared docker — removed 41 keys." — because the armed label comes from the stale /system snapshot and the RETURNING count is the actual confirmation. Arming Clear on a still-configured module says the module may be using these rows right now; the configured: false row says clearing is the only cleanup; the configured: null row says the opposite — leave the rows alone unless the module is known gone for good — and names the unidentified module.

Never automatic on a load failure, per the review: a repo that could not clone this boot is absent for a reason that resolves itself, and deleting its state would turn a network blip into permanent data loss.

Not included

The optional per-module total bound. It wants a number, and the enumeration this adds is what will supply one from a real deployment instead of a guess. Deferring it also keeps this change from making a previously-succeeding set() start throwing.

Verification

Beyond the suite: built the app and ran it against a data dir seeded with rows for a loaded module (docker) and an unmatched module, with an unreachable external repo configured so the unidentified path is live, then drove the page in headless Edge:

before: docker + old-renamed-module (unverified) | 2 modules · 2.32 KiB
unverified copy names the failure: "…identified this boot (127.0.0.1-1-nope)…"
armed on docker: "This module is still configured and may be using these
  rows right now — clearing loses whatever state it has derived." → Cancel
armed + confirmed on old-renamed-module
notice: "Cleared old-renamed-module — removed 3 keys."
after: 1 module · 35.0 B
=== server-side truth === [["docker",2]] total 35

The two-click clear removes exactly one module's rows, the notice carries the server's count, and the totals refresh with the database agreeing. A delete that lands while the follow-up /system read fails surfaces a section-level message naming what happened, removed count included.

The branch now carries a merge of main (#45's stage: "retry" rescue and #49's git-test isolation landed there since the last round). Two merge consequences owned here: the rescue record gains nameFromManifest, carried from the failed attempt — whether the name is the manifest's is a fact about the name, and the rescue does not change the name — and seedRepo spawns git through TEST_GIT_SPAWN / tools/test-gitconfig like every other git-shelling test, which supersedes the inline gpgsign override this branch had (and with it, a failing call reports the git verb again, since no flags precede it).

Gates, on the merged tree: deno task check, deno lint (203 files), deno fmt --check (321 files), deno task check:svelte (0 errors), deno task build, and packages/server/tests/131 passed: the 129 the third review measured, plus a regression pinning that an SDK-incompatible manifest still names its failure (sdk: "^999", clone succeeds, stage manifest, nameFromManifest: true) — the round-3 blocker: the recovery called readManifest, which throws on the SDK gate after parsing, so the failure that lasts forever was exactly the one that recorded the slug — plus the retry-rescue test that arrived with the merge.

The rate limiter's on-request sweep is throttled to once a minute above the threshold: the map key is client-supplied (leftmost x-forwarded-for behind an appending proxy), so the map's size — and with it a per-request scan's cost — is a number an attacker rotating addresses chooses; the comment now says so, and no longer claims the old interval was unref'd (it was not — and the leak's discriminator is the CI-pinned Deno image, not "some machines").

Docs: docs/modules.md gains list() and what it is for; CLAUDE.md gains the split between what is on ctx and what is on /system, and why.

Closes #39. `ctx.storage` capped each value at 8 MB and bounded nothing else: no enumeration, no way to remove rows for a module that was deleted or renamed, and `updated_at_ms` written on every `set` and read by nothing. ## Store `ModuleKv` gains three methods: | method | answers | | --- | --- | | `list(module, limit?)` | `{ key, bytes, updatedAtMs }[]` — never values. `limit` becomes a SQL `LIMIT`, so it bounds the read itself, not just the response — the query runs on DuckDB's one serialized queue | | `usage()` | per-module `{ keys, bytes, updatedAtMs }`, every module holding rows | | `deleteAll(module)` | rows removed, counted through `DELETE … RETURNING` so the number and the delete are one statement | Sizes come from `strlen`, not `length`: DuckDB's `length` counts characters and the cap in `set` is in UTF-8 bytes, and two units for one number is how a 3 MB value reads as 1 MB. There is a test pinning that. `usage` and `deleteAll` are unscoped by design and never appear on `ctx`. They are reached only through `ModuleHost.moduleStorage`, which is typed `Pick<ModuleKv, "list" | "usage" | "deleteAll">` (`OperatorModuleKv`) — so "operator surfaces only" is structural, not a comment: `get`/`set` cannot leak along with them. ## Module-facing: `ctx.storage.list()` Its own keys, with sizes and write times, never the values — asking what you hold must not cost the whole footprint to answer. It passes no `limit`: a module's contract is every key; the bound exists for the operator route below. **It takes no module argument.** Like `get`/`set`/`delete`, the facade in `host.ts` closes over the module's own name, so `list` can only ever answer for the caller. A test proves that end to end — a real module whose `register(ctx)` exposes `ctx.storage.list()` on a route, with a neighbour module's rows in the same table: ```ts const { keys } = await res.json(); assertEquals(keys.map((k) => k.key), ["mine-1", "mine-2"]); assertFalse(JSON.stringify(keys).includes("secret")); ``` Confirmed it has teeth: pointing the facade at the neighbour's name fails it. The use it exists for is **expiring your own entries** — a cache keyed per image or per remote object can otherwise only delete the keys it still remembers, which are not the ones that accumulate. ## Operator-facing: `/system` Dropping a whole module's rows is deliberately **not** on `ctx`: the rows most in need of dropping belong to a module that no longer loads and therefore has no `ctx` at all. - `/system` payload gains `moduleStorage: { modules, totalBytes, unidentified }`, each row carrying `configured` — **three-valued**. `true` when the deployment still asks for the module; `false` when nothing does — the orphan case the issue is about; `null` when the answer is not knowable this boot. The last state exists because a failure can record only the repo slug while the rows live under the manifest name: a slug can never match them, so while any external module failed before its manifest was readable, an unmatched row must not be called orphaned. `ModuleFailure` carries `nameFromManifest` to make that decidable — and the recovery keys on the manifest being READABLE, not acceptable: `readManifestName` parses the schema without the SDK gate, so a module requiring a newer SDK major — the one failure mode that never resolves itself — still names its failure, from whatever checkout is on disk (the previous boot's for a clone failure, the fresh one for a manifest the gate refused). `null` is therefore reserved for a failure with no readable manifest anywhere. `unidentified` lists those failures by name (still slugs, redacted), so the page can say which module is unaccounted for instead of a deployment-wide flag. - A read of `usage()` that throws is a thrown `/system`, not an empty section: an empty store answers `[]`, so a catch there could only dress a database failure up as "Nothing stored" on the page whose job is finding rows. - `GET|DELETE /api/core/system/module-storage/:module`, both `adminOnly`, both under the app-wide CSRF guard, with the delete logged (`module`, `removed`, `by`). The GET caps at 500 keys **in the SQL** and reports the true total from `usage()`'s row count; both routes have a non-admin 403 test run against real sessions rather than the auth-disabled static admin. - A "Module storage" section on the system page: totals, an expandable key list fetched per module (a module may hold thousands; the page should not pay for them to show a list nobody expanded — and says "showing the first N of M keys" when the cap cut it; key names are module-chosen, image refs with digests included, so the pane wraps them instead of blowing the label column open), and a two-click Clear. Its request failure lands in a visible message rather than a silently disarmed button; its success reports the server's own count — "Cleared docker — removed 41 keys." — because the armed label comes from the stale `/system` snapshot and the `RETURNING` count is the actual confirmation. Arming Clear on a still-configured module says the module may be using these rows right now; the `configured: false` row says clearing is the only cleanup; the `configured: null` row says the opposite — leave the rows alone unless the module is known gone for good — and names the unidentified module. **Never automatic on a load failure**, per the review: a repo that could not clone this boot is absent for a reason that resolves itself, and deleting its state would turn a network blip into permanent data loss. ## Not included The optional per-module total bound. It wants a number, and the enumeration this adds is what will supply one from a real deployment instead of a guess. Deferring it also keeps this change from making a previously-succeeding `set()` start throwing. ## Verification Beyond the suite: built the app and ran it against a data dir seeded with rows for a loaded module (`docker`) and an unmatched module, with an unreachable external repo configured so the unidentified path is live, then drove the page in headless Edge: ``` before: docker + old-renamed-module (unverified) | 2 modules · 2.32 KiB unverified copy names the failure: "…identified this boot (127.0.0.1-1-nope)…" armed on docker: "This module is still configured and may be using these rows right now — clearing loses whatever state it has derived." → Cancel armed + confirmed on old-renamed-module notice: "Cleared old-renamed-module — removed 3 keys." after: 1 module · 35.0 B === server-side truth === [["docker",2]] total 35 ``` The two-click clear removes exactly one module's rows, the notice carries the server's count, and the totals refresh with the database agreeing. A delete that lands while the follow-up `/system` read fails surfaces a section-level message naming what happened, removed count included. The branch now carries a merge of `main` (#45's `stage: "retry"` rescue and #49's git-test isolation landed there since the last round). Two merge consequences owned here: the rescue record gains `nameFromManifest`, carried from the failed attempt — whether the name is the manifest's is a fact about the name, and the rescue does not change the name — and `seedRepo` spawns git through `TEST_GIT_SPAWN` / `tools/test-gitconfig` like every other git-shelling test, which supersedes the inline gpgsign override this branch had (and with it, a failing call reports the git verb again, since no flags precede it). Gates, on the merged tree: `deno task check`, `deno lint` (203 files), `deno fmt --check` (321 files), `deno task check:svelte` (0 errors), `deno task build`, and `packages/server/tests/` — **131 passed**: the 129 the third review measured, plus a regression pinning that an SDK-incompatible manifest still names its failure (`sdk: "^999"`, clone succeeds, stage `manifest`, `nameFromManifest: true`) — the round-3 blocker: the recovery called `readManifest`, which throws on the SDK gate after parsing, so the failure that lasts forever was exactly the one that recorded the slug — plus the retry-rescue test that arrived with the merge. The rate limiter's on-request sweep is throttled to once a minute above the threshold: the map key is client-supplied (leftmost `x-forwarded-for` behind an appending proxy), so the map's size — and with it a per-request scan's cost — is a number an attacker rotating addresses chooses; the comment now says so, and no longer claims the old interval was unref'd (it was not — and the leak's discriminator is the CI-pinned Deno image, not "some machines"). Docs: `docs/modules.md` gains `list()` and what it is for; `CLAUDE.md` gains the split between what is on `ctx` and what is on `/system`, and why.
feat(storage): see what a module is holding, and drop what outlived it
Some checks failed
Dependency Check / dependencies (pull_request) Successful in 1m10s
Build and Deploy / verify (pull_request) Failing after 1m10s
Build and Deploy / build (pull_request) Has been skipped
cb79a5da19
Follow-up from the #38 review. `ctx.storage` capped each value at 8 MB and
bounded nothing else: no way to enumerate keys, no way to remove rows for a
module that was deleted or renamed, and `updated_at_ms` written on every set
and read by nothing.

`ModuleKv` gains `list`, `usage` and `deleteAll`. Sizes are `strlen`, not
`length` — DuckDB's `length` counts characters and the cap in `set` is in
UTF-8 bytes, and two units for one number is how a 3 MB value reads as 1 MB.

`ctx.storage.list()` is the module-facing half: its own keys, with sizes and
write times, never values — asking what you hold must not cost the whole
footprint to answer. It takes no module argument, like every other method on
the facade, which closes over the module's own name. A module still cannot
name another module's rows, and a test now proves that through a real
`register(ctx)` rather than through the store underneath it.

Dropping a whole module's rows is deliberately NOT on `ctx`. The rows most in
need of dropping belong to a module that no longer loads and therefore has no
`ctx` at all, so it is an operator action: `/system` gains totals per module,
a per-module key list, and a two-click clear behind `adminOnly` and CSRF, with
the delete logged. `configured: false` marks a module the deployment no longer
asks for — the case the whole issue is about, invisible on every other surface.

Never automatic on a load failure, per the review: a repo that could not clone
this boot is absent for a reason that resolves itself, and deleting its state
would turn a network blip into permanent data loss.

The optional per-module total bound is deliberately not here. It wants a
number, and the enumeration this adds is what will supply one from a real
deployment instead of a guess.

Verified against a running instance with seeded rows: the section renders,
the key drill-down expands, and the two-click clear removes one module's rows
and refreshes the totals with the database agreeing.

Closes #39

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test(system): one stack per test, so the worker survives its own teardown
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m22s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
2d712429d7
The two tests added for module storage each built their own database, module
host, task service and app. Two extra native stacks in this file's worker was
enough to segfault the run at worker teardown: every test passed, then the
process died with exit 139 and no failure to point at.

Bisected rather than guessed. Under a constrained CPU set, where the effect
reproduces locally:

  both new tests present        3 crashes / 3 runs
  only the HTTP test            0 / 4
  only the facade test          0 / 4
  neither                       1 / 4   (the same as main)

Either alone is fine; the pair is not. So `withStack` now hands out the app
over the stack it already built, and the two tests are one test using it.
Same assertions, same coverage: the routes read and drop one module's rows,
a mutation without the CSRF header does not reach the store, and the
`ctx.storage.list` facade is still proved through a real `register(ctx)` with
a neighbour module's rows in the same table.
thisilike left a comment

Re-reviewed the whole diff against the PR head (2d71242) in a clean worktree, and re-ran every gate rather than taking the description's word for them.

Gates, independently reproduced

gate result
deno task check pass
deno lint pass, 201 files
deno fmt --check pass, 319 files
deno task check:svelte 0 errors (shell 209 files, modules 4×)
packages/server/tests/ 126 passed, 0 failed

Two notes on the suite. The description says 127; I get 126 on the same commit — not a real problem, but the number in the body is not the number the tree produces. And external_modules_test.ts fails outright on any machine with commit.gpgsign=true, because seedRepo shells out to git commit without -c commit.gpgsign=false (gpg: signing failed, then fatal: failed to write commit object). That is pre-existing, not yours, but it is why a clean run here needed GIT_CONFIG_KEY_0=commit.gpgsign.

Claims I checked rather than accepted

  • strlen vs length. Correct, and worth the comment: SELECT length('☃☃☃'), strlen('☃☃☃') returns 3, 9. The unit really does match set's TextEncoder cap.
  • "Never automatic on a load failure." Holds. deleteAll has exactly one caller, the DELETE route.
  • Module scoping of list(). Holds structurally — the facade closes over name, there is no argument to pass, and the end-to-end test through a real register(ctx) is the right shape for proving it.
  • "never the values". Both new endpoints return sizes and times only; I confirmed that against the SQL, not just the assertions.

So the core of this is sound. What follows is what did not survive.

Blocking

1. configured: false is wrong for the exact case this PR says it protects — and the UI turns that into an invitation to delete.

known is built from config.modules + loaded names + failed names. But ModuleFailure.name is documented, at host.ts:130, as "manifest name once known, otherwise the configured name/slug", and prepareExternalModule only assigns name = manifest.name after syncRepo succeeds (external.ts:139-141). A clone-stage failure therefore records the slug — git.imhof.cloud-OpsDeck-mod-teamspeak — while the rows are under the manifest name, teamspeak.

I reproduced it end to end against buildSystemInfo, with one external module configured, one recordFailure({ stage: "clone", name: <slug> }), and one row stored under the manifest name:

rows = [{"module":"teamspeak","keys":1,"bytes":7,"updatedAtMs":…,"configured":false}]

The page then renders not configured plus "Nothing configured under this name — rows left by a module that was removed or renamed. Clearing is the only thing that removes them." — for a module the deployment is still asking for, which will come back on the next boot. That is precisely the "network blip becomes permanent data loss" the storage doc comment, the CLAUDE.md paragraph and the PR body all say this design avoids; it is just moved from an automatic deletion to a UI that tells the operator the deletion is safe. The strongest wording is in system.ts itself: "A repo that could not clone this boot has not stopped being ours, and its rows must not read as orphaned." The code does not deliver that.

Two ways out, either acceptable:

  • Make the name knowable. A clone failure after a previous success still has srcDir on disk with a readable opsdeck.module.json, so prepareExternalModule's catch can fall back to readManifest(srcDir) before settling for the slug. That fixes the failure record for /system's module list too, not just this section.
  • Or make configured honest about not knowing: when any configured external module failed before its manifest was readable, the answer for an unmatched row is unknown, not orphaned — and the copy has to stop saying "removed or renamed" and stop implying clearing is the correct next step.

A regression test belongs with it: slug ≠ manifest name, stage: "clone", rows under the manifest name. Everything currently tested about configured uses a built-in whose name matches, which is the case that cannot fail.

Should fix before merge

2. deps.kv.usage().catch(() => []) (system.ts:131) makes a database error indistinguishable from an empty store. The section renders EmptyState — "Nothing stored", "modules keep derived state here through ctx.storage" — and the operator who came here to find orphans concludes there are none. The getMeta calls above it catch for a real reason (the meta row may be absent); usage() has no such case — I confirmed an empty table returns [], not a throw. So the catch only ever hides a genuine failure. Given you added a section-level message for "the delete landed but the refresh did not", swallowing the read is inconsistent with the standard this PR sets for itself.

3. ModuleHost.moduleStorage hands out the whole unscoped ModuleKv (host.ts:253), get and set included. storage.ts:13 states the principle: "Scoping is the host's job, not the caller's promise." This getter converts it back into a promise — the comment says "for operator surfaces only", and nothing enforces it. Pick<ModuleKv, "list" | "usage" | "deleteAll"> costs nothing and keeps the guarantee structural, which is the whole reason the facade exists.

4. clearStorage does not guard the DELETE against rejection (SystemPage.svelte:185). toggleKeys, fourteen lines above, does .catch(() => null). Here a dropped connection rejects inside the try, finally clears busy and confirming, and the operator sees the armed button revert with no message at all — the one outcome worse than a wrong total is a destructive action whose failure is invisible. The clearError path you added for the refresh case is the right pattern; extend it to the request itself.

Smaller

5. GET /system/module-storage/:module is unbounded (app.ts:347). The comment justifying the per-module fetch says a module "may hold thousands" — but once expanded there is no cap, no pagination, and no truncation notice. For a change whose subject is unbounded storage, a limit with a "showing N of M" line is cheap.

6. deleteAll's count is not atomic with its delete (storage.ts:181). The comment calls the number "the whole confirmation the operator gets", and it can be wrong: Promise.all([kv.deleteAll("m"), kv.set("m", "c", "z")]) with two rows present reported removed = 2 while three rows were actually removed and the module's set resolved successfully with its row already gone. Rare, and arguably inherent to "clear everything", but the comment overstates what the number guarantees. DELETE … RETURNING counted in one statement, if DuckDB obliges, removes the gap; otherwise soften the claim.

7. Misplaced JSDoc (host.ts:243-253). The new getter was inserted between /** Record a module that never reached load() at all (clone/build failure). */ and recordFailure, so that doc now sits on moduleStorage and recordFailure has none.

8. Dead nullable. ModuleStorageUsage.updatedAtMs: number | null is documented "null for a module holding nothing", but GROUP BY module cannot produce a group with no rows and updated_at_ms is BIGINT NOT NULL (schema.ts:210). Confirmed: an empty table yields []. The type and the branch at storage.ts:163 describe a state that cannot exist, and it propagates into the SDK-facing shape and the Svelte interface.

9. storage.ts:13 now lies. "every method takes the module name" — usage() is the first that does not. It is a load-bearing comment; it should say which methods are unscoped and who is allowed to call them.

10. SystemDeps.kv duplicates SystemDeps.host. buildSystemInfo already has the host and could read host.moduleStorage; instead every caller must remember to pass kv: host.moduleStorage, and one that passes a ModuleKv over a different Db gets totals describing a database nobody is serving.

11. Test gap: neither new route has a non-admin case. withStack runs OPSDECK_AUTH=disabled, which injects a static admin, so both HTTP assertions pass on a session that is admin by construction. The CSRF negative is tested and good; the role gate the PR body advertises twice is not exercised at all.

12. class="keys" on the dl has no rule in the style block — the bare dl selector is what styles it. Harmless, but the attribute reads as if it does something.

Happy to re-review quickly once 1-4 are addressed; the rest can ride along.

Re-reviewed the whole diff against the PR head (`2d71242`) in a clean worktree, and re-ran every gate rather than taking the description's word for them. ## Gates, independently reproduced | gate | result | | --- | --- | | `deno task check` | pass | | `deno lint` | pass, 201 files | | `deno fmt --check` | pass, 319 files | | `deno task check:svelte` | 0 errors (shell 209 files, modules 4×) | | `packages/server/tests/` | **126** passed, 0 failed | Two notes on the suite. The description says 127; I get 126 on the same commit — not a real problem, but the number in the body is not the number the tree produces. And `external_modules_test.ts` fails outright on any machine with `commit.gpgsign=true`, because `seedRepo` shells out to `git commit` without `-c commit.gpgsign=false` (`gpg: signing failed`, then `fatal: failed to write commit object`). That is pre-existing, not yours, but it is why a clean run here needed `GIT_CONFIG_KEY_0=commit.gpgsign`. ## Claims I checked rather than accepted - **`strlen` vs `length`.** Correct, and worth the comment: `SELECT length('☃☃☃'), strlen('☃☃☃')` returns `3, 9`. The unit really does match `set`'s `TextEncoder` cap. - **"Never automatic on a load failure."** Holds. `deleteAll` has exactly one caller, the DELETE route. - **Module scoping of `list()`.** Holds structurally — the facade closes over `name`, there is no argument to pass, and the end-to-end test through a real `register(ctx)` is the right shape for proving it. - **"never the values".** Both new endpoints return sizes and times only; I confirmed that against the SQL, not just the assertions. So the core of this is sound. What follows is what did not survive. ## Blocking **1. `configured: false` is wrong for the exact case this PR says it protects — and the UI turns that into an invitation to delete.** `known` is built from `config.modules` + loaded names + failed names. But `ModuleFailure.name` is documented, at `host.ts:130`, as *"manifest name once known, otherwise the configured name/slug"*, and `prepareExternalModule` only assigns `name = manifest.name` **after** `syncRepo` succeeds (`external.ts:139-141`). A clone-stage failure therefore records the slug — `git.imhof.cloud-OpsDeck-mod-teamspeak` — while the rows are under the manifest name, `teamspeak`. I reproduced it end to end against `buildSystemInfo`, with one external module configured, one `recordFailure({ stage: "clone", name: <slug> })`, and one row stored under the manifest name: ``` rows = [{"module":"teamspeak","keys":1,"bytes":7,"updatedAtMs":…,"configured":false}] ``` The page then renders `not configured` plus *"Nothing configured under this name — rows left by a module that was removed or renamed. Clearing is the only thing that removes them."* — for a module the deployment is still asking for, which will come back on the next boot. That is precisely the "network blip becomes permanent data loss" the storage doc comment, the CLAUDE.md paragraph and the PR body all say this design avoids; it is just moved from an automatic deletion to a UI that tells the operator the deletion is safe. The strongest wording is in `system.ts` itself: *"A repo that could not clone this boot has not stopped being ours, and its rows must not read as orphaned."* The code does not deliver that. Two ways out, either acceptable: - Make the name knowable. A clone failure after a previous success still has `srcDir` on disk with a readable `opsdeck.module.json`, so `prepareExternalModule`'s catch can fall back to `readManifest(srcDir)` before settling for the slug. That fixes the failure record for `/system`'s module list too, not just this section. - Or make `configured` honest about not knowing: when any configured external module failed before its manifest was readable, the answer for an unmatched row is *unknown*, not *orphaned* — and the copy has to stop saying "removed or renamed" and stop implying clearing is the correct next step. A regression test belongs with it: slug ≠ manifest name, `stage: "clone"`, rows under the manifest name. Everything currently tested about `configured` uses a built-in whose name matches, which is the case that cannot fail. ## Should fix before merge **2. `deps.kv.usage().catch(() => [])` (`system.ts:131`) makes a database error indistinguishable from an empty store.** The section renders `EmptyState` — "Nothing stored", "modules keep derived state here through `ctx.storage`" — and the operator who came here to find orphans concludes there are none. The `getMeta` calls above it catch for a real reason (the meta row may be absent); `usage()` has no such case — I confirmed an empty table returns `[]`, not a throw. So the catch only ever hides a genuine failure. Given you added a section-level message for "the delete landed but the refresh did not", swallowing the read is inconsistent with the standard this PR sets for itself. **3. `ModuleHost.moduleStorage` hands out the whole unscoped `ModuleKv`** (`host.ts:253`), `get` and `set` included. `storage.ts:13` states the principle: *"Scoping is the host's job, not the caller's promise."* This getter converts it back into a promise — the comment says "for operator surfaces only", and nothing enforces it. `Pick<ModuleKv, "list" | "usage" | "deleteAll">` costs nothing and keeps the guarantee structural, which is the whole reason the facade exists. **4. `clearStorage` does not guard the DELETE against rejection** (`SystemPage.svelte:185`). `toggleKeys`, fourteen lines above, does `.catch(() => null)`. Here a dropped connection rejects inside the `try`, `finally` clears `busy` and `confirming`, and the operator sees the armed button revert with no message at all — the one outcome worse than a wrong total is a destructive action whose failure is invisible. The `clearError` path you added for the refresh case is the right pattern; extend it to the request itself. ## Smaller **5. `GET /system/module-storage/:module` is unbounded** (`app.ts:347`). The comment justifying the per-module fetch says a module "may hold thousands" — but once expanded there is no cap, no pagination, and no truncation notice. For a change whose subject is unbounded storage, a limit with a "showing N of M" line is cheap. **6. `deleteAll`'s count is not atomic with its delete** (`storage.ts:181`). The comment calls the number "the whole confirmation the operator gets", and it can be wrong: `Promise.all([kv.deleteAll("m"), kv.set("m", "c", "z")])` with two rows present reported `removed = 2` while three rows were actually removed and the module's `set` resolved successfully with its row already gone. Rare, and arguably inherent to "clear everything", but the comment overstates what the number guarantees. `DELETE … RETURNING` counted in one statement, if DuckDB obliges, removes the gap; otherwise soften the claim. **7. Misplaced JSDoc** (`host.ts:243-253`). The new getter was inserted between `/** Record a module that never reached load() at all (clone/build failure). */` and `recordFailure`, so that doc now sits on `moduleStorage` and `recordFailure` has none. **8. Dead nullable.** `ModuleStorageUsage.updatedAtMs: number | null` is documented "null for a module holding nothing", but `GROUP BY module` cannot produce a group with no rows and `updated_at_ms` is `BIGINT NOT NULL` (`schema.ts:210`). Confirmed: an empty table yields `[]`. The type and the branch at `storage.ts:163` describe a state that cannot exist, and it propagates into the SDK-facing shape and the Svelte interface. **9. `storage.ts:13` now lies.** "every method takes the module name" — `usage()` is the first that does not. It is a load-bearing comment; it should say which methods are unscoped and who is allowed to call them. **10. `SystemDeps.kv` duplicates `SystemDeps.host`.** `buildSystemInfo` already has the host and could read `host.moduleStorage`; instead every caller must remember to pass `kv: host.moduleStorage`, and one that passes a `ModuleKv` over a different `Db` gets totals describing a database nobody is serving. **11. Test gap: neither new route has a non-admin case.** `withStack` runs `OPSDECK_AUTH=disabled`, which injects a static admin, so both HTTP assertions pass on a session that is admin by construction. The CSRF negative is tested and good; the role gate the PR body advertises twice is not exercised at all. **12.** `class="keys"` on the `dl` has no rule in the style block — the bare `dl` selector is what styles it. Harmless, but the attribute reads as if it does something. Happy to re-review quickly once 1-4 are addressed; the rest can ride along.
@ -342,0 +344,4 @@
// never the values: this is a diagnostics page, and a module's stored state
// can be anything it derived, including data its own UI gates on a role.
api.get(
"/system/module-storage/:module",
Owner

Unbounded. The comment on the frontend explains the per-module fetch by "a module may hold thousands" — but the expanded list has no cap, no pagination and no truncation notice, so the thousands arrive in one response and one array. A limit plus a "showing N of M" line matches what the rest of this change is arguing for.

Also: neither this route nor the DELETE has a non-admin test. withStack runs OPSDECK_AUTH=disabled, so the HTTP tests are all admin by construction — the CSRF negative is covered, the role gate is not.

Unbounded. The comment on the frontend explains the per-module fetch by "a module may hold thousands" — but the expanded list has no cap, no pagination and no truncation notice, so the thousands arrive in one response and one array. A limit plus a "showing N of M" line matches what the rest of this change is arguing for. Also: neither this route nor the DELETE has a non-admin test. `withStack` runs `OPSDECK_AUTH=disabled`, so the HTTP tests are all admin by construction — the CSRF negative is covered, the role gate is not.
@ -244,0 +250,4 @@
* module's own name (see `contextFor`), and that closure is the whole
* mechanism keeping one module out of another's rows.
*/
get moduleStorage(): ModuleKv {
Owner

Two things.

The getter returns the full unscoped ModuleKv, get/set included, so "for operator surfaces only" is enforced by the comment alone — while storage.ts:13 insists scoping is "the host's job, not the caller's promise". Pick<ModuleKv, "list" | "usage" | "deleteAll"> keeps that structural at zero cost.

And it landed between recordFailure's doc comment and recordFailure, so /** Record a module that never reached load() at all … */ now documents this getter.

Two things. The getter returns the full unscoped `ModuleKv`, `get`/`set` included, so "for operator surfaces only" is enforced by the comment alone — while `storage.ts:13` insists scoping is "the host's job, not the caller's promise". `Pick<ModuleKv, "list" | "usage" | "deleteAll">` keeps that structural at zero cost. And it landed between `recordFailure`'s doc comment and `recordFailure`, so `/** Record a module that never reached load() at all … */` now documents this getter.
@ -40,0 +51,4 @@
keys: number;
bytes: number;
/** most recent write, or null for a module holding nothing */
updatedAtMs: number | null;
Owner

Unreachable. GROUP BY module yields no group without rows, and updated_at_ms is BIGINT NOT NULL (schema.ts:210) — an empty table gives [], not a row with a null. The nullability propagates into the SDK shape and the Svelte interface and the branch at line 163 can never be taken.

Unreachable. `GROUP BY module` yields no group without rows, and `updated_at_ms` is `BIGINT NOT NULL` (`schema.ts:210`) — an empty table gives `[]`, not a row with a null. The nullability propagates into the SDK shape and the Svelte interface and the branch at line 163 can never be taken.
@ -95,0 +178,4 @@
// counted first: DuckDB's DELETE reports nothing back through `run`, and
// "removed 0 rows" versus "removed 40" is the whole confirmation the
// operator gets
const [row] = await this.db.query<{ keys: number }>(
Owner

The count and the delete are separate trips through the serialized queue, so the number the comment calls "the whole confirmation the operator gets" can be wrong. Promise.all([kv.deleteAll("m"), kv.set("m", "c", "z")]) over two existing rows reported removed = 2 while three rows went, and the module's set resolved fine with its row already deleted. DELETE … RETURNING counted in one statement if DuckDB allows it; otherwise weaken the comment.

The count and the delete are separate trips through the serialized queue, so the number the comment calls "the whole confirmation the operator gets" can be wrong. `Promise.all([kv.deleteAll("m"), kv.set("m", "c", "z")])` over two existing rows reported `removed = 2` while three rows went, and the module's `set` resolved fine with its row already deleted. `DELETE … RETURNING` counted in one statement if DuckDB allows it; otherwise weaken the comment.
@ -105,0 +123,4 @@
// URL rather than by module name, so the name it stores under is only
// knowable from a load or a failure. A repo that could not clone this boot
// has not stopped being ours, and its rows must not read as orphaned.
const known = new Set([
Owner

Blocking. host.failed.map(f => f.name) cannot supply the manifest name for a clone-stage failure: ModuleFailure.name is "manifest name once known, otherwise the configured name/slug" (host.ts:130), and prepareExternalModule sets name = manifest.name only after syncRepo returns (external.ts:139-141).

So for https://git.imhof.cloud/OpsDeck/mod-teamspeak.git whose manifest name is teamspeak, known gets the slug git.imhof.cloud-OpsDeck-mod-teamspeak and the rows under teamspeak come out configured: false. Reproduced against this exact function:

rows = [{"module":"teamspeak","keys":1,"bytes":7,"updatedAtMs":…,"configured":false}]

The comment right here says "its rows must not read as orphaned" — this is the code that makes them read that way, and the UI copy then tells the operator clearing is the only fix. Either resolve the manifest name on a clone failure (the previous boot's srcDir still has a readable opsdeck.module.json), or make the unmatched case unknown rather than orphaned.

Blocking. `host.failed.map(f => f.name)` cannot supply the manifest name for a clone-stage failure: `ModuleFailure.name` is "manifest name once known, otherwise the configured name/slug" (`host.ts:130`), and `prepareExternalModule` sets `name = manifest.name` only after `syncRepo` returns (`external.ts:139-141`). So for `https://git.imhof.cloud/OpsDeck/mod-teamspeak.git` whose manifest name is `teamspeak`, `known` gets the slug `git.imhof.cloud-OpsDeck-mod-teamspeak` and the rows under `teamspeak` come out `configured: false`. Reproduced against this exact function: ``` rows = [{"module":"teamspeak","keys":1,"bytes":7,"updatedAtMs":…,"configured":false}] ``` The comment right here says "its rows must not read as orphaned" — this is the code that makes them read that way, and the UI copy then tells the operator clearing is the only fix. Either resolve the manifest name on a clone failure (the previous boot's `srcDir` still has a readable `opsdeck.module.json`), or make the unmatched case *unknown* rather than *orphaned*.
@ -105,0 +128,4 @@
...host.loaded.map((m) => m.name),
...host.failed.map((f) => f.name),
]);
const usage = await deps.kv.usage().catch(() => []);
Owner

.catch(() => []) renders a database failure as "Nothing stored" — on the one surface whose purpose is finding rows nobody else can see. The getMeta catches above have a real absent-row case; usage() does not (an empty table returns [], I checked), so this only ever hides a genuine error. Let it throw, or carry the failure into the payload the way clearError carries the refresh failure.

`.catch(() => [])` renders a database failure as "Nothing stored" — on the one surface whose purpose is finding rows nobody else can see. The `getMeta` catches above have a real absent-row case; `usage()` does not (an empty table returns `[]`, I checked), so this only ever hides a genuine error. Let it throw, or carry the failure into the payload the way `clearError` carries the refresh failure.
@ -128,0 +182,4 @@
async function clearStorage(module: string) {
busy = true;
try {
const res = await coreFetch(
Owner

Unguarded, unlike toggleKeys fourteen lines up which does .catch(() => null). A dropped connection rejects here, finally resets busy and confirming, and the operator watches the armed button revert with no message — a destructive action whose failure is silent. Route it into clearError like the refresh failure below.

Unguarded, unlike `toggleKeys` fourteen lines up which does `.catch(() => null)`. A dropped connection rejects here, `finally` resets `busy` and `confirming`, and the operator watches the armed button revert with no message — a destructive action whose failure is silent. Route it into `clearError` like the refresh failure below.
@ -325,0 +475,4 @@
</div>
{#if !row.configured}
<div class="source">
Nothing configured under this name — rows left by a module that
Owner

This copy is the sharp edge on the configured bug. For a still-configured external module whose clone failed this boot, the row says "removed or renamed" and "clearing is the only thing that removes them" — so the page actively recommends destroying state that is coming back. Until configured can tell orphaned from could not be identified this boot, the wording must not assert the first.

This copy is the sharp edge on the `configured` bug. For a still-configured external module whose clone failed this boot, the row says "removed or renamed" and "clearing is the only thing that removes them" — so the page actively recommends destroying state that is coming back. Until `configured` can tell *orphaned* from *could not be identified this boot*, the wording must not assert the first.
The sweep interval was never cleared, so every createApp left two timers
ticking in an otherwise-idle process — which is also why any test that
builds an app failed the sanitizer on machines whose Deno counts an
unref'd timer. Sweeping when the bucket map crosses a size threshold
keeps it bounded with no background work at all.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix(storage): a failure that hides its name stops calling rows orphaned
Some checks failed
Build and Deploy / verify (pull_request) Failing after 1m15s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m31s
8f39595f27
Review rework for #46, the blocking finding first: a clone-stage failure
records the repo slug, the rows live under the manifest name, and the
slug can never match them — so the page called still-configured state
"not configured" and recommended the one action that destroys it.
Two changes close that from both ends:

- prepareExternalModule recovers the manifest name from the previous
  boot's checkout when the sync fails before reading it, so the failure
  record (and the /system module list) names the module whenever a
  checkout exists to ask.
- ModuleFailure carries nameFromManifest, and /system's `configured`
  becomes three-valued: while any external failure has no manifest name,
  an unmatched row is null — "not knowable this boot" — never false,
  and the page's copy says to leave it alone instead of inviting the
  delete. A regression test pins the slug-vs-manifest case end to end.

The rest of the review, in order of its numbering:

- buildSystemInfo no longer catches usage() into []: an empty store
  answers [], so the catch could only dress a database failure up as
  "Nothing stored" on the page whose job is finding rows.
- ModuleHost.moduleStorage returns OperatorModuleKv — list/usage/
  deleteAll, never get/set — so "operator surfaces only" is structural,
  and recordFailure gets its doc comment back.
- clearStorage guards the DELETE itself: a dropped connection lands in
  clearError instead of silently disarming the button.
- The key list route caps at 500 and reports the true total; the page
  says "showing the first N of M keys" when it was cut.
- deleteAll counts through DELETE ... RETURNING, one statement, so the
  number can no longer disagree with what a concurrent set() saw.
- ModuleStorageUsage.updatedAtMs drops its unreachable null.
- The storage.ts scoping comment names the unscoped methods and who may
  hold them; SystemDeps loses `kv` (buildSystemInfo reads the host it
  already has); the dead class="keys" is gone.
- Both /system/module-storage routes now have a non-admin case, run
  against real sessions rather than the auth-disabled static admin.
- seedRepo commits with -c commit.gpgsign=false so a signing dev
  machine can run the suite.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

Rework for the review is up as 8f39595 (plus 7847fca for the sanitizer leak your gpgsign note led me to). All 12 items addressed, by your numbering:

1 (blocking) — both ways out, taken together. prepareExternalModule's catch now falls back to readManifest(srcDir) when the manifest was never read, so a clone failure after any previous success records the manifest name — fixing the /system module list as well as this section. For the case with nothing on disk to read, ModuleFailure gains a required nameFromManifest, and buildSystemInfo makes configured three-valued: while any external failure has no manifest name, an unmatched row is null — "not knowable this boot" — never false. The regression test you asked for is there (slug ≠ manifest name, stage: "clone", rows under the manifest name → null; same failure with the manifest name → true), plus an external_modules_test proving the on-disk recovery through a real seeded checkout whose fetch fails.

2 — the catch(() => []) is gone; a usage() throw is now a failed /system request, with a comment saying why no catch belongs there.

3moduleStorage returns OperatorModuleKv = Pick<ModuleKv, "list" | "usage" | "deleteAll">. Tests that seed rows construct a raw ModuleKv over the same Db, which is itself a nice proof the surface lost set.

4 — the DELETE is guarded like toggleKeys; a rejection lands in clearError ("the request never reached the server") instead of a silently disarmed button.

5 — the GET caps at 500 and returns { keys, total }; the pane shows "showing the first N of M keys" when cut.

6DELETE … RETURNING key — DuckDB obliges, so the count and the delete are one statement and the comment's claim is now true.

7recordFailure has its doc back; the getter has its own.

8updatedAtMs is number end to end (store, /system payload, Svelte interface); the unreachable branch is gone.

9 — the scoping comment now names get/set/delete/list as the facade-scoped set and usage/deleteAll as operator-only via moduleStorage.

10SystemDeps.kv removed; buildSystemInfo reads host.moduleStorage off the host it already has.

11 — both routes have a non-admin case: the stack runs in real OIDC mode with sessions seeded through SessionStore, viewer gets 403 on GET and on DELETE (rows untouched), admin passes the same gate.

12 — the dead class="keys" is gone.

Also from your gate notes: seedRepo commits with -c commit.gpgsign=false, so a signing machine runs the suite clean. And chasing why the suite failed here turned up the cause worth keeping: rateLimit() started a sweep interval nothing ever cleared — two leaked timers per createApp, which some Deno setups' sanitizer counts even unref'd. The sweep now runs on the request path when the bucket map crosses a size threshold, so there is no background timer at all (7847fca).

Gates re-run on the new head: deno task check, deno lint (201), deno fmt --check (319), check:svelte 0 errors, deno task build, and the suite — 129 passed (your 126, plus the three new tests). The body's stale 127 is corrected too.

Rework for the review is up as `8f39595` (plus `7847fca` for the sanitizer leak your gpgsign note led me to). All 12 items addressed, by your numbering: **1 (blocking) — both ways out, taken together.** `prepareExternalModule`'s catch now falls back to `readManifest(srcDir)` when the manifest was never read, so a clone failure after any previous success records the manifest name — fixing the `/system` module list as well as this section. For the case with nothing on disk to read, `ModuleFailure` gains a required `nameFromManifest`, and `buildSystemInfo` makes `configured` three-valued: while any external failure has no manifest name, an unmatched row is `null` — "not knowable this boot" — never `false`. The regression test you asked for is there (slug ≠ manifest name, `stage: "clone"`, rows under the manifest name → `null`; same failure with the manifest name → `true`), plus an `external_modules_test` proving the on-disk recovery through a real seeded checkout whose fetch fails. **2** — the `catch(() => [])` is gone; a `usage()` throw is now a failed `/system` request, with a comment saying why no catch belongs there. **3** — `moduleStorage` returns `OperatorModuleKv = Pick<ModuleKv, "list" | "usage" | "deleteAll">`. Tests that seed rows construct a raw `ModuleKv` over the same `Db`, which is itself a nice proof the surface lost `set`. **4** — the DELETE is guarded like `toggleKeys`; a rejection lands in `clearError` ("the request never reached the server") instead of a silently disarmed button. **5** — the GET caps at 500 and returns `{ keys, total }`; the pane shows "showing the first N of M keys" when cut. **6** — `DELETE … RETURNING key` — DuckDB obliges, so the count and the delete are one statement and the comment's claim is now true. **7** — `recordFailure` has its doc back; the getter has its own. **8** — `updatedAtMs` is `number` end to end (store, `/system` payload, Svelte interface); the unreachable branch is gone. **9** — the scoping comment now names `get`/`set`/`delete`/`list` as the facade-scoped set and `usage`/`deleteAll` as operator-only via `moduleStorage`. **10** — `SystemDeps.kv` removed; `buildSystemInfo` reads `host.moduleStorage` off the host it already has. **11** — both routes have a non-admin case: the stack runs in real OIDC mode with sessions seeded through `SessionStore`, viewer gets 403 on GET and on DELETE (rows untouched), admin passes the same gate. **12** — the dead `class="keys"` is gone. Also from your gate notes: `seedRepo` commits with `-c commit.gpgsign=false`, so a signing machine runs the suite clean. And chasing why the suite failed *here* turned up the cause worth keeping: `rateLimit()` started a sweep interval nothing ever cleared — two leaked timers per `createApp`, which some Deno setups' sanitizer counts even unref'd. The sweep now runs on the request path when the bucket map crosses a size threshold, so there is no background timer at all (`7847fca`). Gates re-run on the new head: `deno task check`, `deno lint` (201), `deno fmt --check` (319), `check:svelte` 0 errors, `deno task build`, and the suite — **129 passed** (your 126, plus the three new tests). The body's stale 127 is corrected too.
test(system): the role-gate test gets its own worker
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m25s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m10s
54224875b5
CI segfaulted (exit 139) at worker teardown with every test green — the
shape system_test's withStack comment documents: the new non-admin test
was a second app-building stack in that file's worker. It moves to its
own file, where it is also the only stack; it could never share one
anyway, since the auth-disabled stacks inject a static admin and a role
gate probed by an admin-by-construction session is not probed at all.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

CI on 8f39595 failed with exit 139 at worker teardown — every test green, then the segfault withStack's comment documents: the new non-admin test was a second app-building stack in system_test's worker. 5422487 moves it to its own file (system_roles_test.ts), where it is also the only stack; it could never share one anyway, since the auth-disabled stacks inject a static admin. Both workflows are green on the new head; still 129 tests.

CI on `8f39595` failed with exit 139 at worker teardown — every test green, then the segfault `withStack`'s comment documents: the new non-admin test was a second app-building stack in system_test's worker. `5422487` moves it to its own file (`system_roles_test.ts`), where it is also the only stack; it could never share one anyway, since the auth-disabled stacks inject a static admin. Both workflows are green on the new head; still 129 tests.
thisilike left a comment

Third pass, whole diff against 54224875 (merge base db57f4d) in a clean detached worktree. Every gate re-run here, and every claim in the comment probed rather than read.

Gates, independently reproduced

gate result here
deno task check pass
deno lint pass, 202 files
deno fmt --check pass, 320 files
deno task check:svelte 0 errors, no warning in SystemPage.svelte
deno task build pass
packages/server/tests/ 129 passed, 0 failed

129 matches. The file counts do not: the body says 201/319, the tree gives 202/320 — the new system_roles_test.ts. Same off-by-one class as the 127-vs-126 last round; the numbers are re-run output, so they should be the tree's.

What I checked instead of accepting

  • 1–12 from last round: all present. OperatorModuleKv is a real Pick and ModuleHost.kv is private, so get/set are genuinely unreachable from /system. usage()'s catch is gone. deleteAll is DELETE … RETURNING key and the count is one statement (the concurrent-set race I reproduced last time no longer applies). SystemDeps.kv is gone. Docs, JSDoc placement, the dead nullable and the dead class="keys" — all done.

  • The non-admin gate: real. system_roles_test.ts runs an OIDC-mode stack with sessions through SessionStore; viewer gets 403 on GET and DELETE with the rows untouched, admin gets 200 through the same gate. That is the test the last two rounds did not have.

  • gpgsign: works. Proved it rather than assumed — a repo with local commit.gpgsign=true and gpg.program=/bin/false fails git commit with gpg failed to sign the data and succeeds under -c commit.gpgsign=false.

  • The sanitizer story is real and stronger than the comment says. With the old ratelimit.ts restored, the full suite passes here, 129/129, on Deno 2.9.5 — so "some machines" reads like flakiness. It is not. Under the Dockerfile/CI-pinned image:

    $ docker run --rm denoland/deno:2.5.6 deno test --no-check rl_test.ts
    error: Leaks detected:
      - 2 intervals were started in this test, but never completed.
    

    deno task ci runs the suite inside exactly that image, so the old code fails every app-building test in CI, not on somebody's laptop. Two corrections for the comment: the interval was never unrefTimer'd, so "even unref'd" is wrong, and the discriminator is the pinned toolchain version — which is the thing CLAUDE.md already says only shows up in the image.

  • configured three-valued: the regression test is the right shape and passes. But see below — the fix does not cover the case that matters most.

Blocking

1. The manifest-name recovery is gated behind the SDK compat check, so the failure that lasts forever is exactly the one it cannot name.

readManifest parses the manifest and then throws on sdkCompatible. Both new recovery paths call it: external.ts:176 in the catch, and host.ts:374's stage !== "manifest". So a module whose manifest is perfectly readable — name right there in the error text — records the slug and nameFromManifest: false.

Reproduced with a healthy local checkout, a reachable origin and nothing wrong but the SDK major:

PROBE2: {"stage":"manifest","name":"git.example.com-opsdeck-mod-teamspeak",
         "nameFromManifest":false,
         "error":"module \"teamspeak\" requires SDK ^2, host provides 1"}

And the consequence through buildSystemInfo, with one genuinely orphaned row present:

PROBE rows: [{"module":"long-deleted-module","keys":1,"bytes":7,"configured":null}]

unidentified is a single deployment-wide flag, so one such module pins every unmatched row to null — "unverified", "Do not clear them unless you know the module is gone for good" — on every boot, permanently, for a condition that does not resolve itself the way a clone failure does. The orphan signal this PR exists to provide is off, and the page tells the operator not to act. It also puts the slug back in /system's module list, which the comment says the recovery fixed ("fixing the /system module list as well as this section").

Not data loss like last round's inversion — but the headline feature is inert in the most persistent failure mode there is, and the copy is actively wrong about why.

Fix is small: get the name from the parsed manifest before the compat gate — a readManifestName(dir) that only runs ManifestSchema.parse, or have readManifest throw a typed error carrying manifest.name. Then both recovery sites become "the manifest was readable" instead of "the manifest was acceptable". Worth a test at the same shape as the clone-recovery one: incompatible sdk, manifest on disk, expect nameFromManifest: true.

While you are in there: nameFromManifest: stage !== "manifest" (host.ts:374) encodes the same assumption and deserves a comment saying it means "the manifest was accepted", not "the name is unknowable".

Should fix before merge

2. removed is computed atomically and then thrown away. storage.ts earns the RETURNING with "'removed 0 rows' versus 'removed 40' is the whole confirmation the operator gets" — and clearStorage never reads the response body (SystemPage.svelte:205-215). The operator gets a refreshed total and nothing else; the count exists only in a server log line they cannot see. It also matters more than usual here, because the armed button is labelled from the stale /system snapshot ("Delete 3 keys") and a running module can have written since. Either show what came back or stop claiming the number is the confirmation.

3. The rate-limiter sweep is now O(map) per request on the unauthenticated login path. if (buckets.size > SWEEP_ABOVE) sweep(now) (ratelimit.ts:33), and the key is the leftmost x-forwarded-for — client-supplied when the proxy appends. Above 1024 distinct fresh IPs every request walks the whole map, and fresh buckets are never removed, so the map keeps growing and the scan keeps getting longer: rotate headers and the per-request cost rises with the number you have sent. The old code's scan was once per five minutes. Keep the timer-free design, just do not sweep per request — let sweptAtMs = 0; if (buckets.size > SWEEP_ABOVE && now - sweptAtMs > 60_000) { sweep(now); sweptAtMs = now; } — or cap the map. Also worth saying out loud: the comment's "bounded by the number of distinct client IPs seen in the last ten minutes" is a bound an attacker chooses.

Smaller

4. The 500 cap is on the response, not on the read (app.ts:355). list() fetches every key and .slice() afterwards, so the comment's "a module may hold thousands" is still paid in full — and it is paid on the single serialized DuckDB queue, i.e. queue time shared with metric flushes. Measured for honesty: 50 000 keys × 2 KB values took 26 ms unbounded vs 8 ms with LIMIT 500. Nit-level in cost, but LIMIT 501 plus usage()'s existing keys count for total is strictly less code than the comment explaining the cap.

5. The key pane can overflow. dt (SystemPage.svelte:545) has no word-break, while every other long-string surface on that page sets one (.wrap, pre, .source). Keys are module-chosen and the documented use is caching per image or per remote object — an image ref with a digest will blow the minmax(110px, auto) column.

6. seedRepo's error message regressed. run("-c", "commit.gpgsign=false", "commit", …) still reports git ${args[0]}, so a commit failure now prints git -c: … (external_modules_test.ts:235). Prepend the flags inside run instead, or index past them.

7. "Unverified" does not say which module could not be identified. The operator has to scroll to the module list and correlate a slug themselves. The section already knows — naming it in the copy closes the loop, and it becomes the obvious place to see the effect of item 1.

8. Clear looks identical on a loaded module's row. docker keeps live update verdicts in ctx.storage; the two-click confirm says nothing about that, while the orphan and unverified rows both get explanatory copy. One clause for the configured === true case ("this module is running and will lose derived state") would match the care the other two states got.

9. Builtin failures lean on directory name == manifest name. unidentified filters origin === "external", so a builtin that fails at the manifest stage is assumed matchable — true only because readManifest never compares the two and all five in-repo manifests happen to agree with their directories. Harmless today; a sentence in the known comment would keep it that way.

Item 1 is the one I would not merge without. 2 and 3 are small and mechanical. The rest can ride along.

Third pass, whole diff against `54224875` (merge base `db57f4d`) in a clean detached worktree. Every gate re-run here, and every claim in the comment probed rather than read. ## Gates, independently reproduced | gate | result here | | --- | --- | | `deno task check` | pass | | `deno lint` | pass, **202** files | | `deno fmt --check` | pass, **320** files | | `deno task check:svelte` | 0 errors, no warning in `SystemPage.svelte` | | `deno task build` | pass | | `packages/server/tests/` | **129** passed, 0 failed | 129 matches. The file counts do not: the body says 201/319, the tree gives 202/320 — the new `system_roles_test.ts`. Same off-by-one class as the 127-vs-126 last round; the numbers are re-run output, so they should be the tree's. ## What I checked instead of accepting - **1–12 from last round**: all present. `OperatorModuleKv` is a real `Pick` and `ModuleHost.kv` is `private`, so `get`/`set` are genuinely unreachable from `/system`. `usage()`'s catch is gone. `deleteAll` is `DELETE … RETURNING key` and the count is one statement (the concurrent-`set` race I reproduced last time no longer applies). `SystemDeps.kv` is gone. Docs, JSDoc placement, the dead nullable and the dead `class="keys"` — all done. - **The non-admin gate**: real. `system_roles_test.ts` runs an OIDC-mode stack with sessions through `SessionStore`; viewer gets 403 on GET and DELETE with the rows untouched, admin gets 200 through the same gate. That is the test the last two rounds did not have. - **`gpgsign`**: works. Proved it rather than assumed — a repo with local `commit.gpgsign=true` and `gpg.program=/bin/false` fails `git commit` with `gpg failed to sign the data` and succeeds under `-c commit.gpgsign=false`. - **The sanitizer story is real and stronger than the comment says.** With the old `ratelimit.ts` restored, the full suite passes here, 129/129, on Deno 2.9.5 — so "some machines" reads like flakiness. It is not. Under the Dockerfile/CI-pinned image: ``` $ docker run --rm denoland/deno:2.5.6 deno test --no-check rl_test.ts error: Leaks detected: - 2 intervals were started in this test, but never completed. ``` `deno task ci` runs the suite inside exactly that image, so the old code fails every app-building test in CI, not on somebody's laptop. Two corrections for the comment: the interval was never `unrefTimer`'d, so "even unref'd" is wrong, and the discriminator is the pinned toolchain version — which is the thing `CLAUDE.md` already says only shows up in the image. - **`configured` three-valued**: the regression test is the right shape and passes. But see below — the fix does not cover the case that matters most. ## Blocking **1. The manifest-name recovery is gated behind the SDK compat check, so the failure that lasts forever is exactly the one it cannot name.** `readManifest` parses the manifest and *then* throws on `sdkCompatible`. Both new recovery paths call it: `external.ts:176` in the catch, and `host.ts:374`'s `stage !== "manifest"`. So a module whose manifest is perfectly readable — name right there in the error text — records the slug and `nameFromManifest: false`. Reproduced with a healthy local checkout, a reachable origin and nothing wrong but the SDK major: ``` PROBE2: {"stage":"manifest","name":"git.example.com-opsdeck-mod-teamspeak", "nameFromManifest":false, "error":"module \"teamspeak\" requires SDK ^2, host provides 1"} ``` And the consequence through `buildSystemInfo`, with one genuinely orphaned row present: ``` PROBE rows: [{"module":"long-deleted-module","keys":1,"bytes":7,"configured":null}] ``` `unidentified` is a single deployment-wide flag, so one such module pins **every** unmatched row to `null` — "unverified", *"Do not clear them unless you know the module is gone for good"* — on every boot, permanently, for a condition that does not resolve itself the way a clone failure does. The orphan signal this PR exists to provide is off, and the page tells the operator not to act. It also puts the slug back in `/system`'s module list, which the comment says the recovery fixed ("fixing the `/system` module list as well as this section"). Not data loss like last round's inversion — but the headline feature is inert in the most persistent failure mode there is, and the copy is actively wrong about why. Fix is small: get the name from the parsed manifest *before* the compat gate — a `readManifestName(dir)` that only runs `ManifestSchema.parse`, or have `readManifest` throw a typed error carrying `manifest.name`. Then both recovery sites become "the manifest was readable" instead of "the manifest was acceptable". Worth a test at the same shape as the clone-recovery one: incompatible `sdk`, manifest on disk, expect `nameFromManifest: true`. While you are in there: `nameFromManifest: stage !== "manifest"` (`host.ts:374`) encodes the same assumption and deserves a comment saying it means "the manifest was accepted", not "the name is unknowable". ## Should fix before merge **2. `removed` is computed atomically and then thrown away.** `storage.ts` earns the `RETURNING` with *"'removed 0 rows' versus 'removed 40' is the whole confirmation the operator gets"* — and `clearStorage` never reads the response body (`SystemPage.svelte:205-215`). The operator gets a refreshed total and nothing else; the count exists only in a server log line they cannot see. It also matters more than usual here, because the armed button is labelled from the stale `/system` snapshot ("Delete 3 keys") and a running module can have written since. Either show what came back or stop claiming the number is the confirmation. **3. The rate-limiter sweep is now O(map) per request on the unauthenticated login path.** `if (buckets.size > SWEEP_ABOVE) sweep(now)` (`ratelimit.ts:33`), and the key is the leftmost `x-forwarded-for` — client-supplied when the proxy appends. Above 1024 distinct fresh IPs every request walks the whole map, and fresh buckets are never removed, so the map keeps growing and the scan keeps getting longer: rotate headers and the per-request cost rises with the number you have sent. The old code's scan was once per five minutes. Keep the timer-free design, just do not sweep per request — `let sweptAtMs = 0; if (buckets.size > SWEEP_ABOVE && now - sweptAtMs > 60_000) { sweep(now); sweptAtMs = now; }` — or cap the map. Also worth saying out loud: the comment's "bounded by the number of distinct client IPs seen in the last ten minutes" is a bound an attacker chooses. ## Smaller **4. The 500 cap is on the response, not on the read** (`app.ts:355`). `list()` fetches every key and `.slice()` afterwards, so the comment's "a module may hold thousands" is still paid in full — and it is paid on the single serialized DuckDB queue, i.e. queue time shared with metric flushes. Measured for honesty: 50 000 keys × 2 KB values took 26 ms unbounded vs 8 ms with `LIMIT 500`. Nit-level in cost, but `LIMIT 501` plus `usage()`'s existing `keys` count for `total` is strictly less code than the comment explaining the cap. **5. The key pane can overflow.** `dt` (`SystemPage.svelte:545`) has no `word-break`, while every other long-string surface on that page sets one (`.wrap`, `pre`, `.source`). Keys are module-chosen and the documented use is caching per image or per remote object — an image ref with a digest will blow the `minmax(110px, auto)` column. **6. `seedRepo`'s error message regressed.** `run("-c", "commit.gpgsign=false", "commit", …)` still reports `git ${args[0]}`, so a commit failure now prints `git -c: …` (`external_modules_test.ts:235`). Prepend the flags inside `run` instead, or index past them. **7. "Unverified" does not say which module could not be identified.** The operator has to scroll to the module list and correlate a slug themselves. The section already knows — naming it in the copy closes the loop, and it becomes the obvious place to see the effect of item 1. **8. Clear looks identical on a loaded module's row.** `docker` keeps live update verdicts in `ctx.storage`; the two-click confirm says nothing about that, while the orphan and unverified rows both get explanatory copy. One clause for the `configured === true` case ("this module is running and will lose derived state") would match the care the other two states got. **9. Builtin failures lean on directory name == manifest name.** `unidentified` filters `origin === "external"`, so a builtin that fails at the manifest stage is assumed matchable — true only because `readManifest` never compares the two and all five in-repo manifests happen to agree with their directories. Harmless today; a sentence in the `known` comment would keep it that way. Item 1 is the one I would not merge without. 2 and 3 are small and mechanical. The rest can ride along.
@ -342,0 +352,4 @@
async (c) => {
const keys = await host.moduleStorage.list(c.req.param("module"));
return c.json({
keys: keys.slice(0, KEY_LIST_LIMIT),
Owner

The cap is on the response; the read is not. list() selects every key and this slices afterwards, so "a module may hold thousands" is still paid in full — on the one serialized DuckDB queue, i.e. in time shared with metric flushes. Measured: 50 000 keys × 2 KB values, 26 ms unbounded vs 8 ms with LIMIT 500. Small, but LIMIT 501 plus usage()'s existing keys count for total is less code than the comment explaining the cap.

The cap is on the response; the read is not. `list()` selects every key and this slices afterwards, so "a module may hold thousands" is still paid in full — on the one serialized DuckDB queue, i.e. in time shared with metric flushes. Measured: 50 000 keys × 2 KB values, 26 ms unbounded vs 8 ms with `LIMIT 500`. Small, but `LIMIT 501` plus `usage()`'s existing `keys` count for `total` is less code than the comment explaining the cap.
@ -167,0 +173,4 @@
// orphaned. First-ever failures have no checkout; the slug stands.
if (!nameFromManifest) {
try {
name = (await readManifest(srcDir)).name;
Owner

This is the recovery, and it cannot recover the case that lasts: readManifest parses the manifest and then throws on sdkCompatible, so a checkout whose manifest is fine but whose SDK major is not keeps the slug — with the name sitting in the error string.

Reproduced with a reachable origin and nothing else wrong:

PROBE2: {"stage":"manifest","name":"git.example.com-opsdeck-mod-teamspeak",
         "nameFromManifest":false,
         "error":"module \"teamspeak\" requires SDK ^2, host provides 1"}

One such module pins every unmatched storage row to configured: null on every boot, so the orphan signal is permanently off and the page says "do not clear". Parse the name without the compat gate (a readManifestName, or a typed error carrying manifest.name) and this site becomes "the manifest was readable" rather than "the manifest was acceptable".

This is the recovery, and it cannot recover the case that lasts: `readManifest` parses the manifest and *then* throws on `sdkCompatible`, so a checkout whose manifest is fine but whose SDK major is not keeps the slug — with the name sitting in the error string. Reproduced with a reachable origin and nothing else wrong: ``` PROBE2: {"stage":"manifest","name":"git.example.com-opsdeck-mod-teamspeak", "nameFromManifest":false, "error":"module \"teamspeak\" requires SDK ^2, host provides 1"} ``` One such module pins every unmatched storage row to `configured: null` on every boot, so the orphan signal is permanently off and the page says "do not clear". Parse the name without the compat gate (a `readManifestName`, or a typed error carrying `manifest.name`) and this site becomes "the manifest was readable" rather than "the manifest was acceptable".
@ -350,1 +371,4 @@
name,
// the manifest read is the first step, so any later stage means the
// name above came from it
nameFromManifest: stage !== "manifest",
Owner

Same assumption as external.ts, and the comment states it as a fact it is not: a manifest-stage failure is not "the name is unknowable", it is "the manifest was rejected" — which for an SDK mismatch happens after the name has been parsed. Once the name is recoverable before the compat gate, this should read from that, and the comment should say "the manifest was accepted".

Same assumption as `external.ts`, and the comment states it as a fact it is not: a `manifest`-stage failure is not "the name is unknowable", it is "the manifest was rejected" — which for an SDK mismatch happens after the name has been parsed. Once the name is recoverable before the compat gate, this should read from that, and the comment should say "the manifest was accepted".
@ -24,3 +30,4 @@
const ip = c.req.header("x-forwarded-for")?.split(",")[0].trim() ??
"local";
const now = Date.now();
if (buckets.size > SWEEP_ABOVE) sweep(now);
Owner

This makes the sweep O(map) per request on the pre-auth login path, keyed by the leftmost x-forwarded-for — client-supplied when the proxy appends. Past 1024 distinct fresh IPs every request walks the whole map, and fresh buckets are never dropped, so the map grows and the scan grows with it: the per-request cost rises with the number of headers already sent. The old code scanned once per five minutes.

Keep the timer-free design, just throttle it: let sweptAtMs = 0; if (buckets.size > SWEEP_ABOVE && now - sweptAtMs > 60_000) { sweep(now); sweptAtMs = now; } — or cap the map outright.

(The motivation checks out and is stronger than the comment: under the Dockerfile-pinned denoland/deno:2.5.6, which is what deno task ci runs, the old interval fails with 2 intervals were started in this test, but never completed. On 2.9.5 the whole suite passes with the old file, so it is the pinned version, not "some machines" — and it was never unrefTimer'd, so "even unref'd" is wrong.)

This makes the sweep O(map) *per request* on the pre-auth login path, keyed by the leftmost `x-forwarded-for` — client-supplied when the proxy appends. Past 1024 distinct fresh IPs every request walks the whole map, and fresh buckets are never dropped, so the map grows and the scan grows with it: the per-request cost rises with the number of headers already sent. The old code scanned once per five minutes. Keep the timer-free design, just throttle it: `let sweptAtMs = 0; if (buckets.size > SWEEP_ABOVE && now - sweptAtMs > 60_000) { sweep(now); sweptAtMs = now; }` — or cap the map outright. (The motivation checks out and is stronger than the comment: under the Dockerfile-pinned `denoland/deno:2.5.6`, which is what `deno task ci` runs, the old interval fails with `2 intervals were started in this test, but never completed`. On 2.9.5 the whole suite passes with the old file, so it is the pinned version, not "some machines" — and it was never `unrefTimer`'d, so "even unref'd" is wrong.)
@ -128,0 +209,4 @@
const fresh = await coreFetch("/api/core/system").catch(() => null);
if (fresh?.ok) {
info = await fresh.json();
clearError = null;
Owner

The response body is never read, here or on the success path above — so removed, which storage.ts went to DELETE … RETURNING lengths to make exact and calls "the whole confirmation the operator gets", reaches nobody but the server log. It matters more than usual because the armed button is labelled from the stale /system snapshot ("Delete 3 keys") and a running module can have written since. Show the count, or drop the claim from the comment.

The response body is never read, here or on the success path above — so `removed`, which `storage.ts` went to `DELETE … RETURNING` lengths to make exact and calls "the whole confirmation the operator gets", reaches nobody but the server log. It matters more than usual because the armed button is labelled from the stale `/system` snapshot ("Delete 3 keys") and a running module can have written since. Show the count, or drop the claim from the comment.
Review rework for #46, round 3, the blocking finding: the manifest-name
recovery called readManifest, which parses the file and THEN throws on
the SDK gate — so a module requiring a newer SDK major recorded the
slug with the name sitting right there in a perfectly readable file.
That failure mode never resolves itself, and one such module pinned
every unmatched storage row to "unverified" on every boot, permanently,
with copy telling the operator not to act. The orphan signal the PR
exists for was inert exactly where it is needed longest.

readManifestName(dir) parses the schema and skips the gate; the
recovery in prepareExternalModule's catch uses it, so the condition for
a usable name is "the manifest was readable", not "acceptable".
Loading keeps going through readManifest — the gate still refuses the
module, it just no longer eats the name on the way down. A regression
test pins it: sdk "^999", clone succeeds, stage "manifest", and the
failure carries the manifest name with nameFromManifest true.

host.ts's `nameFromManifest: stage !== "manifest"` gets the comment the
review asked for — it means the manifest was ACCEPTED, and why that is
sufficient on this path.

Also from the review: seedRepo moves the gpgsign override inside run()
so a commit failure reports "git commit:" again instead of "git -c:".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review rework for #46, round 3, items 2 and 4–9:

- The delete's `removed` count was computed atomically and then thrown
  away — clearStorage never read the response body, so the one number
  the route calls "the whole confirmation the operator gets" existed
  only in a server log. The page now shows it in a section-level notice
  ("Cleared docker — removed 41 keys."), and folds it into the
  reload-failed message too. It matters more than usual because the
  armed button's label comes from the stale /system snapshot.
- `unidentified` becomes the list of failure names (still slugs,
  redacted) instead of a deployment-wide boolean, and the "unverified"
  copy names the module the operator would otherwise have to correlate
  against the module list themselves.
- The 500-key cap moves into the SQL: ModuleKv.list takes an optional
  LIMIT, so the read itself is bounded — an unbounded fetch is paid on
  DuckDB's serialized queue, not just on the wire. `total` comes from
  usage()'s row count. ctx.storage.list passes no limit; a module's
  contract is every key.
- Arming Clear on a still-configured module now says what the orphan
  and unverified rows already said for their cases: the module may be
  using these rows right now, clearing loses its derived state.
- Key names are module-chosen (the documented use is caching per image
  or remote object), so the key pane's dt gets word-break: break-all —
  the class="keys" that was dead last round now carries the rule.
- The `known` comment says out loud that builtin failures are matchable
  only because directory name == manifest name, which nothing verifies.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix(server): sweep the rate-limit map once a minute, not once a request
All checks were successful
Build and Deploy / verify (pull_request) Successful in 1m50s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 1m5s
1763b563f2
Review rework for #46, round 3, item 3: above the threshold every
request walked the whole bucket map, and the map is keyed by the
leftmost x-forwarded-for — client-supplied behind an appending proxy —
so both the map's size and the per-request scan were numbers an
attacker rotating addresses chooses, on the unauthenticated login path.
The sweep keeps its timer-free shape and gains a once-per-minute
throttle; the comment now says the bound is attacker-chosen, and drops
the wrong "even unref'd" claim — the old interval was never unref'd,
and the discriminator was the CI-pinned Deno image, where the leak
failed every app-building test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
Owner

Round 3 addressed, head is now 1763b56 (three commits on top of 5422487).

1 (blocking) — the recovery keys on READABLE, not acceptable. Took your first way out: readManifestName(dir) in manifest.ts runs ManifestSchema.parse and skips the SDK gate; the recovery in prepareExternalModule's catch calls it instead of readManifest. Loading still goes through readManifest, so the gate refuses the module exactly as before — it just no longer eats the name on the way down. Your probe shape is now a test: seedRepo with sdk: "^999", clone succeeds, and the failure comes back stage: "manifest", name: "futurist", nameFromManifest: true. host.ts's nameFromManifest: stage !== "manifest" got the comment you asked for — it means the manifest was ACCEPTED, plus why that is sufficient on that path (externals gate through prepareExternalModule first; builtins' hint is the directory name, see item 9).

2 — the count is shown. clearStorage reads the response body and puts the server's number in a section-level notice — "Cleared old-renamed-module — removed 3 keys." in the drill below — and folds it into the reload-failed message too ("Cleared X (removed N keys), but the page could not reload…"). Agreed on why it matters: the armed label is the stale snapshot, the RETURNING count is the confirmation.

3 — sweep throttled. Kept the timer-free shape, added sweptAtMs with a 60 s floor, exactly your sketch. The comment now says the bound is attacker-chosen (leftmost x-forwarded-for, appended by the proxy), drops the wrong "even unref'd" claim, and names the pinned image as the discriminator rather than "some machines".

4 — the cap is in the SQL. ModuleKv.list takes an optional limit that becomes LIMIT ?; the route passes 500 and takes total from usage()'s row count. ctx.storage.list passes none — a module's contract is every key.

5 — keys wrap. class="keys" is back and this time carries a rule: .keys dt { word-break: break-all }. The drill seeded a ghcr.io/...@sha256:... key to make sure the case is real.

6 — seedRepo names the verb again. The gpgsign override moved inside run(), prepended on every call (harmless off the commit), so args[0] is the verb and a failure reports git commit: ….

7 — "unverified" names the module. unidentified is now the list of failure names (slugs, redacted) instead of a deployment-wide boolean; /system carries it and the row copy reads "…failed before it could be identified this boot (127.0.0.1-1-nope)…". The system_test asserts the array both ways: populated while the slug stands, empty once the failure names itself.

8 — clearing a configured module says so. Arming Clear on a configured: true row shows "This module is still configured and may be using these rows right now — clearing loses whatever state it has derived." — shown exactly while armed, so the row list stays quiet otherwise.

9 — said out loud. The known comment now states that builtin failures are matchable only because directory name == manifest name, that nothing verifies it, and that this is why unidentified filters external failures alone.

Verification, this round: deno task check, deno lint (202), deno fmt --check (320), check:svelte (0 errors, none in SystemPage), deno task build, tests 130/130 (your 129 + the SDK-incompat regression). Re-drove the page in headless Edge against a seeded data dir with an unreachable external repo configured, so the unidentified path was live end to end: unverified copy names the slug, armed-on-configured warning shows, two-click clear returns "removed 3 keys" in the notice, totals and the database agree afterwards. Full transcript in the updated PR description, which also owns the list(module, limit?) signature, the unidentified payload field, and the corrected rate-limiter story.

Round 3 addressed, head is now `1763b56` (three commits on top of `5422487`). **1 (blocking) — the recovery keys on READABLE, not acceptable.** Took your first way out: `readManifestName(dir)` in `manifest.ts` runs `ManifestSchema.parse` and skips the SDK gate; the recovery in `prepareExternalModule`'s catch calls it instead of `readManifest`. Loading still goes through `readManifest`, so the gate refuses the module exactly as before — it just no longer eats the name on the way down. Your probe shape is now a test: `seedRepo` with `sdk: "^999"`, clone succeeds, and the failure comes back `stage: "manifest"`, `name: "futurist"`, `nameFromManifest: true`. `host.ts`'s `nameFromManifest: stage !== "manifest"` got the comment you asked for — it means the manifest was ACCEPTED, plus why that is sufficient on that path (externals gate through `prepareExternalModule` first; builtins' hint is the directory name, see item 9). **2 — the count is shown.** `clearStorage` reads the response body and puts the server's number in a section-level notice — "Cleared old-renamed-module — removed 3 keys." in the drill below — and folds it into the reload-failed message too ("Cleared X (removed N keys), but the page could not reload…"). Agreed on why it matters: the armed label is the stale snapshot, the `RETURNING` count is the confirmation. **3 — sweep throttled.** Kept the timer-free shape, added `sweptAtMs` with a 60 s floor, exactly your sketch. The comment now says the bound is attacker-chosen (leftmost `x-forwarded-for`, appended by the proxy), drops the wrong "even unref'd" claim, and names the pinned image as the discriminator rather than "some machines". **4 — the cap is in the SQL.** `ModuleKv.list` takes an optional `limit` that becomes `LIMIT ?`; the route passes 500 and takes `total` from `usage()`'s row count. `ctx.storage.list` passes none — a module's contract is every key. **5 — keys wrap.** `class="keys"` is back and this time carries a rule: `.keys dt { word-break: break-all }`. The drill seeded a `ghcr.io/...@sha256:...` key to make sure the case is real. **6 — seedRepo names the verb again.** The gpgsign override moved inside `run()`, prepended on every call (harmless off the commit), so `args[0]` is the verb and a failure reports `git commit: …`. **7 — "unverified" names the module.** `unidentified` is now the list of failure names (slugs, redacted) instead of a deployment-wide boolean; `/system` carries it and the row copy reads "…failed before it could be identified this boot (127.0.0.1-1-nope)…". The system_test asserts the array both ways: populated while the slug stands, empty once the failure names itself. **8 — clearing a configured module says so.** Arming Clear on a `configured: true` row shows "This module is still configured and may be using these rows right now — clearing loses whatever state it has derived." — shown exactly while armed, so the row list stays quiet otherwise. **9 — said out loud.** The `known` comment now states that builtin failures are matchable only because directory name == manifest name, that nothing verifies it, and that this is why `unidentified` filters external failures alone. Verification, this round: `deno task check`, `deno lint` (202), `deno fmt --check` (320), `check:svelte` (0 errors, none in SystemPage), `deno task build`, tests **130/130** (your 129 + the SDK-incompat regression). Re-drove the page in headless Edge against a seeded data dir with an unreachable external repo configured, so the unidentified path was live end to end: unverified copy names the slug, armed-on-configured warning shows, two-click clear returns "removed 3 keys" in the notice, totals and the database agree afterwards. Full transcript in the updated PR description, which also owns the `list(module, limit?)` signature, the `unidentified` payload field, and the corrected rate-limiter story.
Merge remote-tracking branch 'origin/main' into feat/issue-39-storage-enumeration
All checks were successful
Build and Deploy / verify (pull_request) Successful in 2m59s
Build and Deploy / build (pull_request) Has been skipped
Dependency Check / dependencies (pull_request) Successful in 36s
c0b86ed554
# Conflicts:
#	packages/server/tests/external_modules_test.ts
Author
Owner

Addendum: main moved under this while the round-3 rework was in flight (#45's stage: "retry" rescue, #49's git-test isolation), so the head is now the merge commit c0b86ed, not 1763b56. Two things worth knowing beyond the mechanical resolution:

  • Item 6 is superseded, not just fixed. #49's TEST_GIT_SPAWN + tools/test-gitconfig is the right mechanism, so seedRepo now spawns git through it like every other git-shelling test — the inline -c commit.gpgsign=false from 1763b56 is gone again, and with no flags in the argument list the error message names the verb for free.
  • One semantic conflict git could not see: #45's rescue builds a ModuleFailure, which on this branch requires nameFromManifest. The rescue carries it over from the failed attempt — whether the name is the manifest's is a fact about the name, and the rescue does not change the name. deno task check caught it; the fix is in the merge commit.

Gates re-run on the merged tree: check / lint (203) / fmt (321) / check:svelte (0 errors) / build all pass, tests 131/131 — the 130 from the comment above plus the retry-rescue test that came with the merge. PR description updated to the merged tree's numbers.

Addendum: `main` moved under this while the round-3 rework was in flight (#45's `stage: "retry"` rescue, #49's git-test isolation), so the head is now the merge commit `c0b86ed`, not `1763b56`. Two things worth knowing beyond the mechanical resolution: - **Item 6 is superseded, not just fixed.** #49's `TEST_GIT_SPAWN` + `tools/test-gitconfig` is the right mechanism, so `seedRepo` now spawns git through it like every other git-shelling test — the inline `-c commit.gpgsign=false` from `1763b56` is gone again, and with no flags in the argument list the error message names the verb for free. - **One semantic conflict git could not see:** #45's rescue builds a `ModuleFailure`, which on this branch requires `nameFromManifest`. The rescue carries it over from the failed attempt — whether the name is the manifest's is a fact about the name, and the rescue does not change the name. `deno task check` caught it; the fix is in the merge commit. Gates re-run on the merged tree: check / lint (203) / fmt (321) / check:svelte (0 errors) / build all pass, tests **131/131** — the 130 from the comment above plus the retry-rescue test that came with the merge. PR description updated to the merged tree's numbers.
thisilike approved these changes 2026-08-21 23:09:27 +02:00
thisilike left a comment

Fourth pass, whole diff against the merged head c0b86ed (merge base cc65b8c) in a clean detached worktree. Gates re-run, every round-3 item re-probed rather than read, and the merge itself checked.

Gates

gate result here
deno task check pass
deno lint pass, 203 files
deno fmt --check pass, 321 files
deno task check:svelte 0 errors, none in SystemPage.svelte
deno task build pass
packages/server/tests/ 131 passed, 0 failed

Every number in the description matches the tree this time, including the file counts.

Round 3, verified

1 (was blocking) — fixed, and the payoff is real. readManifestName splits readable from acceptable, and prepareExternalModule's catch uses it. Re-ran my probe end to end — an SDK-refused external module (sdk: "^999", clone succeeds) plus one genuinely orphaned row:

PROBE failure: {"stage":"manifest","name":"futurist","nameFromManifest":true}
PROBE rows: [["futurist",true],["long-deleted-module",false]] unidentified: []

Round 3 gave configured: null for both, forever. Now the refused module keeps its own rows marked as state and the orphan is called an orphan — which is the whole feature. Your regression test is the same shape and asserts the failure record; the /system consequence above is what it buys.

2clearStorage reads removed and the notice carries it, including folded into the reload-failed message. Good call keeping it section-level.

3 — sweep throttled to 60 s above the threshold, and the comment now says the map size is attacker-chosen and names the pinned image as the discriminator instead of "some machines". Accurate on both counts now.

4 — cap is in the SQL. Measured, since it was my claim to substantiate — 50 000 keys in one module plus four 5 000-key neighbours:

list(big) unbounded [round 3]:      29.6 ms
list(big, 500) [round 4]:            3.0 ms
usage() [round 4 total]:             2.6 ms
count(*) where module=? [alt]:       0.8 ms

5.6 ms against 29.6 ms. The full-table aggregate for total is cheaper than I expected (DuckDB is columnar and usage() is already the section's other query) so I would leave it; a per-module count(*) is the marginal option if you ever want it, not a correction.

5–9.keys dt { word-break: break-all }, unidentified as a named list that the row copy interpolates, the armed-on-configured warning, the known comment stating the directory-name assumption out loud. All present.

6, via the merge — agreed that TEST_GIT_SPAWN + tools/test-gitconfig supersedes the inline override; read both, and the isolation is the stronger mechanism (the environment outranks the file, and it strips GIT_* wholesale). With no flags before the verb, the error message names it again for free. Also checked the merge did not swallow anything: git diff origin/main..HEAD is exactly this branch's 16 files, and external-retry.ts's only branch-side change is the nameFromManifest carry-over, which is the right resolution — the rescue does not change the name.

Credentials: since unidentified is a new payload field carrying failure names into page copy, I checked whether a slug can hold a token. It cannot — slugify strips ^[^/]*@ after the scheme, so the credential is gone before redactSecrets is even asked. No leak.

Follow-ups (none blocking)

a. Nothing pins the cap. I dropped KEY_LIST_LIMIT from the route call — list(module), unbounded, exactly the round-3 shape — and the suite still passes 131/131. storage_test.ts has no limit case either, so the new SQL bound and the total that reports what it cut are the one part of this change with no test. kv.list("m", 2) over five rows, and a route assertion where total > keys.length, would close it; total is currently only asserted in the uncapped case ("nothing is capped here").

b. const { removed } = await res.json() can throw (SystemPage.svelte:215). The delete has already landed at that point, finally resets busy/confirming, and the operator sees no notice and no error — the invisible-outcome case the surrounding code exists to prevent, on a truncated response rather than a dropped connection. A .catch(() => null) with a "cleared, count unavailable" fallback covers it.

c. clearNotice never expires — it sits until the next clear. Fine on a diagnostics page; noting it in case you want it tied to the row.

Aside, not this PR

A probe file of mine that opened a plain Db (no app, no host) in the same run segfaulted the suite at teardown — exit 139, every test green first, and it went away when I deleted the file. So the hazard withStack's comment documents is broader than two app stacks: one more DuckDB handle in that worker is enough. Worth an issue of its own, since it makes any future test that opens a database a coin flip.

Approving. Item 1's fix is confirmed at the level that matters, the merge is clean, and (a) is a test I would like to see but not a reason to hold the branch.

Fourth pass, whole diff against the merged head `c0b86ed` (merge base `cc65b8c`) in a clean detached worktree. Gates re-run, every round-3 item re-probed rather than read, and the merge itself checked. ## Gates | gate | result here | | --- | --- | | `deno task check` | pass | | `deno lint` | pass, 203 files | | `deno fmt --check` | pass, 321 files | | `deno task check:svelte` | 0 errors, none in `SystemPage.svelte` | | `deno task build` | pass | | `packages/server/tests/` | **131** passed, 0 failed | Every number in the description matches the tree this time, including the file counts. ## Round 3, verified **1 (was blocking) — fixed, and the payoff is real.** `readManifestName` splits readable from acceptable, and `prepareExternalModule`'s catch uses it. Re-ran my probe end to end — an SDK-refused external module (`sdk: "^999"`, clone succeeds) plus one genuinely orphaned row: ``` PROBE failure: {"stage":"manifest","name":"futurist","nameFromManifest":true} PROBE rows: [["futurist",true],["long-deleted-module",false]] unidentified: [] ``` Round 3 gave `configured: null` for both, forever. Now the refused module keeps its own rows marked as state and the orphan is called an orphan — which is the whole feature. Your regression test is the same shape and asserts the failure record; the `/system` consequence above is what it buys. **2** — `clearStorage` reads `removed` and the notice carries it, including folded into the reload-failed message. Good call keeping it section-level. **3** — sweep throttled to 60 s above the threshold, and the comment now says the map size is attacker-chosen and names the pinned image as the discriminator instead of "some machines". Accurate on both counts now. **4** — cap is in the SQL. Measured, since it was my claim to substantiate — 50 000 keys in one module plus four 5 000-key neighbours: ``` list(big) unbounded [round 3]: 29.6 ms list(big, 500) [round 4]: 3.0 ms usage() [round 4 total]: 2.6 ms count(*) where module=? [alt]: 0.8 ms ``` 5.6 ms against 29.6 ms. The full-table aggregate for `total` is cheaper than I expected (DuckDB is columnar and `usage()` is already the section's other query) so I would leave it; a per-module `count(*)` is the marginal option if you ever want it, not a correction. **5–9** — `.keys dt { word-break: break-all }`, `unidentified` as a named list that the row copy interpolates, the armed-on-configured warning, the `known` comment stating the directory-name assumption out loud. All present. **6, via the merge** — agreed that `TEST_GIT_SPAWN` + `tools/test-gitconfig` supersedes the inline override; read both, and the isolation is the stronger mechanism (the environment outranks the file, and it strips `GIT_*` wholesale). With no flags before the verb, the error message names it again for free. Also checked the merge did not swallow anything: `git diff origin/main..HEAD` is exactly this branch's 16 files, and `external-retry.ts`'s only branch-side change is the `nameFromManifest` carry-over, which is the right resolution — the rescue does not change the name. Credentials: since `unidentified` is a new payload field carrying failure names into page copy, I checked whether a slug can hold a token. It cannot — `slugify` strips `^[^/]*@` after the scheme, so the credential is gone before `redactSecrets` is even asked. No leak. ## Follow-ups (none blocking) **a. Nothing pins the cap.** I dropped `KEY_LIST_LIMIT` from the route call — `list(module)`, unbounded, exactly the round-3 shape — and the suite still passes 131/131. `storage_test.ts` has no `limit` case either, so the new SQL bound and the `total` that reports what it cut are the one part of this change with no test. `kv.list("m", 2)` over five rows, and a route assertion where `total > keys.length`, would close it; `total` is currently only asserted in the uncapped case ("nothing is capped here"). **b. `const { removed } = await res.json()` can throw** (`SystemPage.svelte:215`). The delete has already landed at that point, `finally` resets `busy`/`confirming`, and the operator sees no notice and no error — the invisible-outcome case the surrounding code exists to prevent, on a truncated response rather than a dropped connection. A `.catch(() => null)` with a "cleared, count unavailable" fallback covers it. **c. `clearNotice` never expires** — it sits until the next clear. Fine on a diagnostics page; noting it in case you want it tied to the row. ## Aside, not this PR A probe file of mine that opened a plain `Db` (no app, no host) in the same run segfaulted the suite at teardown — `exit 139`, every test green first, and it went away when I deleted the file. So the hazard `withStack`'s comment documents is broader than two app stacks: one more DuckDB handle in that worker is enough. Worth an issue of its own, since it makes any future test that opens a database a coin flip. Approving. Item 1's fix is confirmed at the level that matters, the merge is clean, and (a) is a test I would like to see but not a reason to hold the branch.
@ -342,0 +351,4 @@
adminOnly,
async (c) => {
const module = c.req.param("module");
const keys = await host.moduleStorage.list(module, KEY_LIST_LIMIT);
Owner

Nothing pins this. I replaced it with host.moduleStorage.list(module) — unbounded, the round-3 shape — and the suite still passed 131/131; storage_test.ts has no limit case either, so both the SQL bound and the total that reports what it cut are untested. kv.list("m", 2) over five rows, plus a route case where total > keys.length, is the whole gap (today total is only asserted where "nothing is capped here").

Nothing pins this. I replaced it with `host.moduleStorage.list(module)` — unbounded, the round-3 shape — and the suite still passed 131/131; `storage_test.ts` has no `limit` case either, so both the SQL bound and the `total` that reports what it cut are untested. `kv.list("m", 2)` over five rows, plus a route case where `total > keys.length`, is the whole gap (today `total` is only asserted where "nothing is capped here").
@ -128,0 +212,4 @@
// came from the /system snapshot, and a running module may have written
// since — "removed 41" when the button said 40 is exactly the kind of
// thing an operator should get to see
const { removed } = await res.json();
Owner

This can throw, and the delete has already landed when it does: finally clears busy/confirming, no notice, no error — the invisible-outcome case the rest of this function exists to prevent, reached by a truncated response rather than a dropped connection. await res.json().catch(() => null) with a "cleared, count unavailable" fallback keeps the guarantee whole.

This can throw, and the delete has already landed when it does: `finally` clears `busy`/`confirming`, no notice, no error — the invisible-outcome case the rest of this function exists to prevent, reached by a truncated response rather than a dropped connection. `await res.json().catch(() => null)` with a "cleared, count unavailable" fallback keeps the guarantee whole.
julian merged commit 754647d9ef into main 2026-08-21 23:20:32 +02:00
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!46
No description provided.