Address whole-project review (todo §2); wire plugin hooks (onBoot/onRequest/onResponse), document template trust boundary, tidy discovery

docs/plugin-contract.md

@@ -134,6 +134,21 @@ export async function listShifts(ctx: RequestContext) {
   (see the README's *Stateless* section). The partials only need rows.
 - `default` status: `200` for `view`/`html`/`json`, `303` for `redirect`.
 
+### Escaping & the trust boundary
+
+The host does not sandbox plugin output (crash-isolation is a non-goal), so a handler **owns the
+safety of the data it renders**:
+
+- **Raw HTML is raw.** An `{ html }` result and the `*.html` partial fields (`cell.html`,
+  `error.html`, a menu `trigger.html`) are emitted **unescaped** — that's their purpose (slot
+  composition). Escape any untrusted content yourself before putting it there.
+- **Text is auto-escaped; URLs are not scheme-checked.** Partials escape text fields (labels,
+  names), so those are injection-safe. But a URL field — nav `href`, a table cell link, a menu
+  item, a breadcrumb, `brand.logo` — is emitted as-is inside the attribute: a `javascript:` or
+  `data:` URL from upstream/user data becomes live XSS. When a URL comes from data you don't
+  control, restrict it to a relative (`/`, `?`, `#`) or `http(s):` URL before handing it to a
+  partial. (A shared `safeUrl()` helper lands with the data-driven plugins in §4.)
+
 ## RequestContext
 
 Every handler receives one argument, the `RequestContext` (`src/context.ts`), built once per
@@ -223,9 +238,14 @@ Optional, for reacting to system actions. A plugin's `hooks` may implement:
 | `onRequest(ctx)` | before route matching | inspect, or **short-circuit** by returning a `RouteResult` |
 | `onResponse(ctx, result)` | after the handler | observe/log; cannot change the response |
 
-Hooks run with no sandbox — a throwing hook fails loud (boot for `onBoot`, the request for the
-others). Keep them cheap; `onRequest` is on the hot path. This surface is intentionally small and
-may grow additively within the major version.
+Hooks run in **discovery order** (plugins sorted by id). `onRequest` fires on every request that
+reaches routing (static assets bypass it); the **first** hook to return a `RouteResult` wins and
+short-circuits — later `onRequest` hooks and the route handler are skipped, and that result renders
+against its own plugin's views. `onResponse` runs for a matched route after its handler, with the
+handler's result; its return value is ignored. Hooks run with no sandbox — a throwing hook fails
+loud (boot for `onBoot`, the request for the others). Keep them cheap; `onRequest` is on the hot
+path (the host skips the pipeline entirely when no plugin declares a hook). This surface is
+intentionally small and may grow additively within the major version.
 
 ## Local dev & test story
 

src/app.test.ts

@@ -176,6 +176,29 @@ test("mounts plugin routes: params, html/json/redirect/view results, and the per
   assert.equal((await fetch(url + "/demo/nope")).status, 404);
 });
 
+test("plugin hooks: onRequest can short-circuit a request and onResponse observes the handler result", async (t) => {
+  const seen: string[] = [];
+  const hooked: Plugin = {
+    apiVersion: "1.0.0",
+    hooks: {
+      onRequest: (c) => (c.url.pathname === "/hooked/blocked" ? { html: "blocked by hook", status: 403 } : undefined),
+      onResponse: (c, r) => void seen.push(`${c.url.pathname}:${r && "html" in r ? r.html : "?"}`),
+    },
+    id: "hooked",
+    routes: [{ handler: () => ({ html: "handler ran" }), method: "GET", path: "/ok" }],
+  };
+  const url = await startApp(t, [hooked]);
+
+  // onRequest short-circuits before routing — handler never runs.
+  const blocked = await fetch(url + "/hooked/blocked");
+  assert.equal(blocked.status, 403);
+  assert.match(await blocked.text(), /blocked by hook/);
+
+  // A normal route runs the handler; onResponse observed its result.
+  assert.match(await (await fetch(url + "/hooked/ok")).text(), /handler ran/);
+  assert.ok(seen.includes("/hooked/ok:handler ran"));
+});
+
 test("rejects unsafe static request paths (encoded traversal, NUL) with 403", async () => {
   assert.equal((await fetch(base + "/public/..%2f..%2fapp.ts")).status, 403);
   assert.equal((await fetch(base + "/public/%00")).status, 403);

src/app.ts

@@ -5,6 +5,7 @@ import * as ejs from "ejs";
 import { buildContext } from "./context.ts";
 import { buildDashboardModel } from "./dashboard.ts";
 import { PLUGINS_DIR } from "./discovery.ts";
+import { runRequestHooks, runResponseHooks } from "./hooks.ts";
 import { DEFAULT_MENU, type MenuConfig } from "./menu-config.ts";
 import type { Plugin, RouteResult } from "./plugin.ts";
 import { allowedMethods, isAuthorized, matchRoute } from "./router.ts";
@@ -29,6 +30,9 @@ export function createApp(options: AppOptions = {}): Server {
   const menu = options.menu ?? DEFAULT_MENU;
   const plugins = options.plugins ?? [];
   const pluginIds = new Set(plugins.map((p) => p.id));
+  // Skip the hook pipeline entirely unless a plugin declares the hook (keeps the hot path free).
+  const anyRequestHooks = plugins.some((p) => p.hooks?.onRequest);
+  const anyResponseHooks = plugins.some((p) => p.hooks?.onResponse);
   const pluginsDir = options.pluginsDir ?? PLUGINS_DIR;
   const publicDir = options.publicDir ?? join(rootDir, "public");
   const viewsDir = options.viewsDir ?? join(rootDir, "views");
@@ -48,8 +52,8 @@ export function createApp(options: AppOptions = {}): Server {
   return createServer(async (req, res) => {
     try {
       const method = req.method ?? "GET";
-      const { url } = buildContext(req, res);
-      const pathname = url.pathname;
+      const ctx = buildContext(req, res); // base context (no route params yet); reused for onRequest
+      const pathname = ctx.url.pathname;
 
       if (pathname.startsWith("/public/") && (method === "GET" || method === "HEAD")) {
         // /public/<id>/… serves a plugin's public/; everything else the core public/.
@@ -58,22 +62,32 @@ export function createApp(options: AppOptions = {}): Server {
         return;
       }
 
+      // Plugin onRequest hooks run before routing and may short-circuit the request.
+      if (anyRequestHooks) {
+        const short = await runRequestHooks(plugins, ctx);
+        if (short) {
+          await sendResult(res, short.result, (view, data) => renderView(short.plugin.id, view, data));
+          return;
+        }
+      }
+
       // Plugin routes (any method): gate on the route's permission, then run the handler.
       const match = matchRoute(plugins, method, pathname);
       if (match) {
-        const ctx = buildContext(req, res, { params: match.params });
-        if (!isAuthorized(match.route, ctx.roles)) {
+        const routeCtx = buildContext(req, res, { params: match.params });
+        if (!isAuthorized(match.route, routeCtx.roles)) {
           sendHtml(res, 403, await render("403", { title: "Forbidden" }));
           return;
         }
-        const result = await match.route.handler(ctx);
-        await sendResult(res, result ?? null, (view, data) => renderView(match.plugin.id, view, data));
+        const result = (await match.route.handler(routeCtx)) ?? null;
+        if (anyResponseHooks) await runResponseHooks(plugins, routeCtx, result); // observers; a throw → 500
+        await sendResult(res, result, (view, data) => renderView(match.plugin.id, view, data));
         return;
       }
 
       if (pathname === "/" && (method === "GET" || method === "HEAD")) {
         // Mock data + no roles until auth (§4) lands; branding/override come from config/menu.ts.
-        sendHtml(res, 200, await render("index", { model: buildDashboardModel(url, [], menu) }));
+        sendHtml(res, 200, await render("index", { model: buildDashboardModel(ctx.url, [], menu) }));
         return;
       }
 

src/discovery.ts

@@ -29,7 +29,7 @@ export async function discoverPlugins(options: DiscoverOptions = {}): Promise<Pl
   const plugins: Plugin[] = [];
 
   for (const id of pluginFolders(dir)) {
-    const fail = (msg: string): number => errors.push(`plugins/${id}: ${msg}`);
+    const fail = (msg: string): void => void errors.push(`plugins/${id}: ${msg}`);
 
     if (!isValidPluginId(id)) {
       errors.push(`"${id}" is not a valid plugin folder name (lowercase a–z, digits, dashes)`);

src/hooks.test.ts

@@ -0,0 +1,50 @@
+import assert from "node:assert/strict";
+import { test } from "node:test";
+import type { RequestContext } from "./context.ts";
+import type { Plugin, PluginHooks } from "./plugin.ts";
+import { runBootHooks, runRequestHooks, runResponseHooks } from "./hooks.ts";
+
+const ctx = {} as RequestContext; // the hooks only thread ctx through; they never read it
+
+function plugin(id: string, hooks: PluginHooks): Plugin {
+  return { apiVersion: "1.0.0", hooks, id };
+}
+
+test("runBootHooks runs each onBoot in order, skips plugins without one, and a throw aborts", async () => {
+  const calls: string[] = [];
+  await runBootHooks([
+    plugin("a", { onBoot: () => void calls.push("a") }),
+    plugin("b", {}), // no onBoot → skipped
+    plugin("c", { onBoot: async () => void calls.push("c") }),
+  ]);
+  assert.deepEqual(calls, ["a", "c"]);
+
+  await assert.rejects(runBootHooks([plugin("x", { onBoot: () => { throw new Error("boom"); } })]), /boom/);
+});
+
+test("runRequestHooks short-circuits on the first RouteResult (with its plugin); later hooks skipped", async () => {
+  const calls: string[] = [];
+  const short = await runRequestHooks([
+    plugin("a", { onRequest: () => void calls.push("a") }), // returns void → continue
+    plugin("b", { onRequest: () => { calls.push("b"); return { html: "stop" }; } }),
+    plugin("c", { onRequest: () => void calls.push("c") }), // never reached
+  ], ctx);
+
+  assert.deepEqual(short?.result, { html: "stop" });
+  assert.equal(short?.plugin.id, "b"); // the owning plugin (so a `view` result resolves correctly)
+  assert.deepEqual(calls, ["a", "b"]);
+
+  // No hook short-circuits → null (proceed with normal routing).
+  assert.equal(await runRequestHooks([plugin("a", { onRequest: () => {} })], ctx), null);
+});
+
+test("runResponseHooks runs every onResponse as an observer with the result; a throw fails", async () => {
+  const seen: unknown[] = [];
+  await runResponseHooks([
+    plugin("a", { onResponse: (_c, r) => void seen.push(r) }),
+    plugin("b", {}), // no onResponse → skipped
+  ], ctx, { html: "ok" });
+  assert.deepEqual(seen, [{ html: "ok" }]);
+
+  await assert.rejects(runResponseHooks([plugin("x", { onResponse: () => { throw new Error("boom"); } })], ctx, null), /boom/);
+});

src/hooks.ts

@@ -0,0 +1,29 @@
+// Plugin lifecycle hooks (todo §2): the host invokes the optional PluginHooks a plugin may declare
+// (docs/plugin-contract.md → Hooks). No sandbox — a throwing hook fails loud (boot for onBoot, the
+// request for the others). Hooks run in discovery order (plugins sorted by id). app.ts skips these
+// entirely when no plugin declares the hook, so the no-hooks hot path stays free.
+
+import type { RequestContext } from "./context.ts";
+import type { Plugin, RouteResult } from "./plugin.ts";
+
+// After discovery, before the server listens. A throw aborts boot.
+export async function runBootHooks(plugins: Plugin[]): Promise<void> {
+  for (const plugin of plugins) await plugin.hooks?.onBoot?.();
+}
+
+// Before route matching. The first hook to return a RouteResult short-circuits the request — its
+// result becomes the response and later hooks + the route handler are skipped. Returns that result
+// with its owning plugin (so a `view` result resolves against that plugin's views), or null to proceed.
+export async function runRequestHooks(plugins: Plugin[], ctx: RequestContext): Promise<{ plugin: Plugin; result: RouteResult } | null> {
+  for (const plugin of plugins) {
+    const result = await plugin.hooks?.onRequest?.(ctx);
+    if (result != null) return { plugin, result };
+  }
+  return null;
+}
+
+// After a route handler produces its result. Observers only — the return value is ignored, so a
+// hook cannot change the response; a throw fails the request.
+export async function runResponseHooks(plugins: Plugin[], ctx: RequestContext, result: RouteResult | null): Promise<void> {
+  for (const plugin of plugins) await plugin.hooks?.onResponse?.(ctx, result);
+}

src/server.ts

@@ -1,6 +1,7 @@
 import { createApp } from "./app.ts";
 import { loadConfig } from "./config.ts";
 import { discoverPlugins } from "./discovery.ts";
+import { runBootHooks } from "./hooks.ts";
 import { loadMenuConfig } from "./menu-config.ts";
 
 const config = loadConfig(); // validates the env (incl. enforced secrets) — fails loud at boot
@@ -8,6 +9,7 @@ const menu = await loadMenuConfig(); // config/menu.ts override + branding — f
 
 const plugins = await discoverPlugins(); // scans plugins/, validates — fails loud on a bad plugin
 console.log(`Discovered ${plugins.length} plugin(s)${plugins.length ? `: ${plugins.map((p) => p.id).join(", ")}` : ""}`);
+await runBootHooks(plugins); // plugin onBoot — after discovery, before listen; a throw aborts boot
 
 const server = createApp({ cache: config.cacheTemplates, menu, plugins }).listen(config.port, () => {
   console.log(`Listening on http://localhost:${config.port}`);

todo.md

@@ -52,7 +52,7 @@ everything via Docker.
 - [x] Per-plugin static serving: `plugins/<id>/public/` → `/public/<id>/`. → `routePublic` (pure, in `src/static.ts`), wired into `app.ts`'s existing `/public/` branch. A request `/public/<rest>` whose leading segment names a discovered plugin serves from `plugins/<id>/public/<rest>`; anything else (e.g. `css/styles.css`) stays on the core `public/`. Disambiguates by the discovered plugin-id set, so only mounted plugins expose assets and core paths are unaffected; plugin ids are URL-safe so the raw segment compares directly (no decode needed). Reuses `serveStatic` unchanged, so the sub-path keeps its decode + traversal/control-char guard (encoded `..` ⇒ 403) and HEAD support; a missing `public/` or file ⇒ 404. Tests-first: a `routePublic` unit (plugin/core split, nested asset, bare `/public/<id>`) + the `app.test.ts` plugin integration now serves a real `demo/public/app.css` (200 + `text/css`) and still 403s a traversal; typecheck + 103 units green. `config/menu.ts` central override is the next §2 item.
 - [x] `config/menu.ts` central override: reorder/rename/hide/group + branding (app name, logo, default theme). → `src/menu-config.ts` (`MenuConfig`/`Branding`/`MenuConfigInput`, `defineMenu()` identity helper, `DEFAULT_MENU`, `loadMenuConfig()`) + the operator file `config/menu.ts`. The override is `composeNav`'s existing `NavOverride` (reorder/rename/group/hide by node id, applied before the per-user filter); branding = `{ name, logo?, sub?, theme? }`. `loadMenuConfig` (imperative shell) dynamically imports `config/menu.ts` if present, validates the authored shape fail-loud (branding field types + `theme` enum, override `hide`/`order` string-arrays / `groups` array / `rename` object), merges branding over defaults; **absent file ⇒ `DEFAULT_MENU`** (clean clone). Wired: `server.ts` loads it at boot → `createApp({ menu })` → `buildDashboardModel(url, roles, menu)` feeds `menu.override` into `composeNav` and `menu.branding` (name/sub) into the shell brand. `config/menu.ts` ships defaults matching prior behaviour (name "Plainpages"/sub "Console", empty override), so a clean clone is unchanged. Added `config` to tsconfig `include` so the authored file is type-checked (Dockerfile `COPY . .` already bakes it). Tests-first: `menu-config.test.ts` (absent⇒defaults / read+merge / malformed⇒throws) + a `dashboard.test.ts` case asserting rename+hide+branding take effect; typecheck (incl. `config/`) + 107 units green; smoke-loaded the real file at boot. **Rendering branding (logo, default theme) into the app shell is the next §2 item.**
 - [x] Wire branding into the app shell. → Completes the §2 branding chain (name/sub already flowed). `shell.ejs` now renders `brand.logo` as `<img class="brand-logo" alt="">` when set, else the default `#i-box` brand-mark; the `theme` local (already forwarded to the theme-switch) is now supplied. `buildDashboardModel` puts `menu.branding.logo` into `shell.brand` and `menu.branding.theme` into `shell.theme` (both omitted when unset, so a clean clone is unchanged → brand-mark + auto theme); `views/index.ejs` forwards `theme` to the shell. Added a `.brand-logo` CSS rule (22px, matches `.brand-mark` sizing). Tests-first: `shell.test.ts` (logo replaces the mark + default theme checked; no-logo ⇒ mark + auto) + extended `dashboard.test.ts` (logo→brand, theme→shell.theme) + an `app.test.ts` integration rendering `createApp({ menu })` end-to-end (logo `<img>` + `theme-dark` checked on `/`). Default-app shell rendering is byte-equivalent, so the visual E2E is unaffected; typecheck + 109 units green. The §2 plugin host is feature-complete (remaining §2 items are the project-wide review + comment/test cleanup).
-- [ ] Run the architecture _and_ the stability reviewer agents on the _whole_ project, not just the latest changes, and address their issues.
+- [x] Run the architecture _and_ the stability reviewer agents on the _whole_ project, not just the latest changes, and address their issues. → Ran both on all of `src/`, `views/`, `config/`, Docker/tsconfig. Verdict: architecture sound + disciplined, no crash/security defect in the current path (fail-loud, traversal guards, JWT/cookie defenses all confirmed). **Fixed now:** (1) HIGH — `PluginHooks` was typed+documented but never invoked; wired it (`src/hooks.ts`: `runBootHooks`/`runRequestHooks`/`runResponseHooks`) — `server.ts` runs `onBoot` after discovery before listen, `app.ts` runs `onRequest` (before routing, first non-void short-circuits, renders against its plugin) + `onResponse` (after handler, observer, throw→500); skipped entirely when no plugin declares a hook (hot path free); `hooks.test.ts` + an `app.test.ts` integration. (2) `discovery.ts` `fail` helper retyped `: void`. (3) Documented the template trust boundary in `docs/plugin-contract.md` (raw `html`/`*.html` fields; URL sinks escaped but not scheme-checked) + tightened the Hooks prose to the wired semantics. **Deferred (reviewer-scoped, not §2):** extract a shared `buildShellContext` out of `dashboard.ts` and route the built-in screens through `matchRoute`/`isAuthorized` → §5 (premature at one call site); a `safeUrl()` helper for href sinks → §4 (no untrusted URLs until upstream data flows); doc/type-duplication + non-local `§N` refs → the §2 comment-cleanup item; HEAD-render cost + dev empty-secret fallback → negligible. typecheck + 113 units green; boot smoke-tested.
 - [ ] Go over all comments in the code and the README and try to make it shorter and more information dense. Remove not strictly needed stuff.
 - [ ] Go over all tests and combine/unify ones that cover the same stuff or are very related and could be combined in a good way. Remove tests that aren't helping, we only want tests that are actually helpful to us.