From d237646c660302fc187cda9837a49d03b900fe9d Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sat, 8 Aug 2026 11:41:14 +0200 Subject: [PATCH] fix(host,sdk,kit): library scanners sat in the nav, could not sync local art, and so never got their settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three symptoms on .21, two defects. Lutris and Heroic appeared in the console sidebar they explicitly opt out of; Lutris's settings were unreachable from the Library screen; and Lutris and Steam logged `sync (startup) failed: HostRequestError`. **The sidebar is a publish gap.** The console is correct — it keeps `category: "library"` plugins out of the nav (`uiPlugins`, app-shell.tsx) — but the host reports no category for them at all. `defineLibraryPlugin` sets it and `sdk/src/ui.ts` forwards it; what SHIPS does not. `@punktfunk/host` was bumped to 0.1.2 on 2026-07-20 and `category` landed 2026-08-05 without a bump, so the registry's 0.1.2 is the pre-category build and every installed scanner registers without one. Bumps the SDK to 0.1.3 — **inert until it is published**. Because the field rides the untyped `pf.request` seam so an older host ignores it rather than rejecting the registration, dropping it is silent by design. `serveUi` now reads its own directory entry back and warns once when a requested category did not land, the same way `defineLibraryPlugin` already warns when a store claim did not take. That is what turns the next occurrence into a log line instead of a bug report. **The missing settings and the failed sync are ONE defect: a write/read disagreement about `file://`.** `local_art_bytes` decodes a `file://` value before testing containment; `validate_art_paths` handed the raw value to `Path::new`, where `file:///home/u/c.jpg` is a RELATIVE path whose first component is `file:`. It canonicalized against the cwd, failed, and read as "outside every art root". So the host refused every cover the kit's own `fileUrl` helper emits — the documented way for a plugin to publish local art — while the read path would have served those same files. That the two symptoms share a cause is not obvious and is why this is one commit: the Library screen's settings control renders only for `origin: "plugin"`, and a source becomes `plugin` only once it holds a store CLAIM, which is taken during a successful reconcile. Lutris failed at entry 0 and Steam at entry 3, so neither ever claimed its store, both stayed `origin: "builtin"`, and neither got a settings button. Heroic reconciled (its art is http(s)) and has had its settings all along; rom-manager was never affected because zero entries meant it never applied. `art_path_is_servable` now decodes first, so both halves of the confinement judge the same string. Confinement itself is unchanged: an out-of-root path is still refused in `file://` clothing, which the test asserts alongside the accept case. Diagnosing this took the HOST's journal, because both surfaces that should have explained it lied. `HostRequestError` stringified to its bare tag, so the sync engine's `${e.cause}` logged `HostRequestError` and discarded the method, the path and the host's own message; it now renders all three, including an object-shaped cause that used to print `[object Object]`. And the host logged "payload carries a field this lane may not set" for BOTH refusals in `check_entry_fields`, so a 400 about an art path read as an auth problem — it now logs the real reason and the entry title. Verified on .21 (Linux): 463 host tests pass, clippy clean under `-D warnings`, `cargo fmt --all --check` clean. The new art test fails without the fix and passes with it. plugin-kit 71 and SDK 72 tests pass, both typecheck clean, biome clean. --- crates/punktfunk-host/src/library/art.rs | 100 +++++++++++++++++++++- crates/punktfunk-host/src/mgmt/library.rs | 50 +++++++---- plugin-kit/bun.lock | 2 +- plugin-kit/package.json | 4 +- plugin-kit/src/errors.ts | 38 +++++++- plugin-kit/src/ui-server.ts | 58 ++++++++++++- plugin-kit/test/errors.test.ts | 64 ++++++++++++++ sdk/package.json | 2 +- 8 files changed, 292 insertions(+), 26 deletions(-) create mode 100644 plugin-kit/test/errors.test.ts diff --git a/crates/punktfunk-host/src/library/art.rs b/crates/punktfunk-host/src/library/art.rs index f5324497..7d6fa0ce 100644 --- a/crates/punktfunk-host/src/library/art.rs +++ b/crates/punktfunk-host/src/library/art.rs @@ -350,8 +350,21 @@ fn sniff_image_type(bytes: &[u8]) -> Option<&'static str> { /// write-time half of the art confinement — [`validate_art_paths`] refuses to persist a value this /// rejects, so an out-of-root path never reaches the catalog in the first place, and /// [`local_art_bytes`] re-checks at read time so an entry written before this existed is still safe. +/// +/// A `file://` value is decoded to a plain path FIRST, exactly as [`local_art_bytes`] does. Both +/// halves of the confinement must judge the *same* string or they disagree: `Path::new` on a raw +/// `file:///home/u/c.jpg` yields a RELATIVE path whose first component is `file:`, which +/// canonicalizes against the cwd, fails, and reads as "outside every root". That is not a +/// conservative failure — it rejected every `file://` cover the plugin kit emits (`fileUrl`, the +/// documented way for a library plugin to publish local art), so the Lutris and Steam scanners +/// could not reconcile a single entry while the read path would have served those same files +/// happily. pub fn art_path_is_servable(value: &str) -> bool { - let p = Path::new(value); + // Idempotent for the already-decoded caller: the decoded form no longer carries the prefix, + // so `local_art_bytes` passing its own output back through here is a no-op, not a second + // percent-decode of a path that legitimately contains `%`. + let value = file_url_to_path(value); + let p = Path::new(&*value); let ext_ok = p .extension() .and_then(|e| e.to_str()) @@ -699,11 +712,23 @@ mod tests { const PNG: &[u8] = &[0x89, b'P', b'N', b'G', 0x0D, 0x0A, 0x1A, 0x0A, 0, 0, 0, 13]; + /// `PUNKTFUNK_LIBRARY_ART_ROOTS` is process-global while cargo runs tests as threads, so the + /// tests that repoint it must not overlap — one clearing the variable mid-flight makes the + /// other's temp root stop being a root, which fails as a confinement bug that isn't there. + /// Poisoning is recovered rather than propagated: a panic in one test should report ITS + /// failure, not cascade into an unrelated `PoisonError`. + static ART_ROOTS_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + fn lock_art_roots() -> std::sync::MutexGuard<'static, ()> { + ART_ROOTS_LOCK.lock().unwrap_or_else(|e| e.into_inner()) + } + /// The art proxy reads bytes in the HOST process (LocalSystem on Windows) from a path the /// plugin lane can write — so what it will and will not read IS the security boundary /// (2026-08-05 review H-2). Confinement, extension, and content are all load-bearing. #[test] fn local_art_bytes_is_confined_and_image_only() { + let _guard = lock_art_roots(); let dir = std::env::temp_dir().join(format!("pf-art-test-{}", std::process::id())); let outside = std::env::temp_dir().join(format!("pf-art-out-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); @@ -837,6 +862,79 @@ mod tests { ); } + /// The write gate and the read gate must judge the SAME string. + /// + /// Regression for 2026-08-08: `validate_art_paths` handed the raw value to `Path::new`, so a + /// `file:///…` cover became a *relative* path starting with a `file:` component, canonicalized + /// against the cwd, failed, and was refused as "outside every art root" — while + /// `local_art_bytes` decoded the very same value and served the file. Every Lutris and Steam + /// entry carrying local art was rejected with a 400 the plugin could only report as + /// `HostRequestError`, so neither scanner could sync a single game. Asserting servable and + /// readable together is the point: either alone passes with the bug present. + #[test] + fn file_url_art_is_accepted_at_write_time_exactly_as_at_read_time() { + let _guard = lock_art_roots(); + let dir = std::env::temp_dir().join(format!("pf-art-wr-{}", std::process::id())); + let outside = std::env::temp_dir().join(format!("pf-art-wr-out-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + std::env::set_var("PUNKTFUNK_LIBRARY_ART_ROOTS", &dir); + + let cover = dir.join("cover.png"); + std::fs::write(&cover, PNG).unwrap(); + + // What the kit's `fileUrl` actually emits for a Lutris/Steam cover. + let url = file_url(&cover); + assert!( + is_local_art_path(&url), + "a file:// value is local art, so the confinement applies to it" + ); + assert!( + art_path_is_servable(&url), + "write time must accept the file:// form of a servable cover" + ); + assert!( + validate_art_paths(&Artwork { + portrait: Some(url.clone()), + header: Some(url), + ..Default::default() + }) + .is_ok(), + "a real Lutris-shaped payload must reconcile" + ); + + // A percent-encoded name (the reason the decode exists at all) survives the round trip. + let spaced = dir.join("My Cover.png"); + std::fs::write(&spaced, PNG).unwrap(); + let spaced_url = file_url(&spaced).replace(' ', "%20"); + assert!( + art_path_is_servable(&spaced_url), + "percent-encoded names must decode before the containment test: {spaced_url}" + ); + assert!(local_art_bytes(&spaced_url).is_some(), "read time agrees"); + + // Loosening the write gate must not loosen the confinement: outside the root is still + // refused in file:// clothing, which is what the raw-string bug was accidentally doing. + let elsewhere = outside.join("cover.png"); + std::fs::write(&elsewhere, PNG).unwrap(); + assert!( + !art_path_is_servable(&file_url(&elsewhere)), + "file:// must not escape the art roots at write time either" + ); + assert!( + validate_art_paths(&Artwork { + portrait: Some(file_url(&elsewhere)), + ..Default::default() + }) + .is_err(), + "an out-of-root file:// cover is still refused" + ); + + std::env::remove_var("PUNKTFUNK_LIBRARY_ART_ROOTS"); + let _ = std::fs::remove_dir_all(&dir); + let _ = std::fs::remove_dir_all(&outside); + } + #[test] fn sniff_image_type_recognizes_containers_and_rejects_secrets() { assert_eq!(sniff_image_type(PNG), Some("image/png")); diff --git a/crates/punktfunk-host/src/mgmt/library.rs b/crates/punktfunk-host/src/mgmt/library.rs index 21c2e525..a4dfddfc 100644 --- a/crates/punktfunk-host/src/mgmt/library.rs +++ b/crates/punktfunk-host/src/mgmt/library.rs @@ -14,33 +14,42 @@ use axum::Extension; /// scanner plugin — while `prep` / `launch.kind = "command"` inside that payload are the operator's /// authority alone. Route reachability and field authority are separate questions. /// -/// `Some(response)` is the refusal to return; `None` means the payload may proceed. Deliberately -/// not `Result<(), Response>`: the "error" here IS the response the handler sends, so there is no -/// error value to propagate, and a 128-byte `Response` in an `Err` variant is what +/// `Some((reason, response))` is the refusal to return; `None` means the payload may proceed. +/// Deliberately not `Result<(), Response>`: the "error" here IS the response the handler sends, so +/// there is no error value to propagate, and a 128-byte `Response` in an `Err` variant is what /// `clippy::result_large_err` objects to. +/// +/// `reason` is the caller's log line. It exists because these are TWO different refusals — an +/// operator-privileged field (403) and an unservable art path (400) — and logging both as "carries +/// a field this lane may not set" sent the Lutris/Steam `file://` art rejection looking like an +/// auth problem. The plugin only ever sees `HostRequestError`, so this log line is the sole +/// diagnosis surface for whoever has to explain why a scanner syncs nothing. fn check_entry_fields( lane: AuthLane, art: &crate::library::Artwork, launch: Option<&crate::library::LaunchSpec>, prep: &[crate::hooks::PrepCmd], -) -> Option { +) -> Option<(String, Response)> { if !lane.may_set_privileged_fields() { if let Some(field) = crate::library::privileged_field(launch, prep) { - return Some(api_error( - StatusCode::FORBIDDEN, - &format!( - "`{field}` is executed as the host user and may only be set with the \ - operator's admin token — a plugin may publish entries with any host-resolved \ - launch kind (steam_appid, steam_ui, launcher_ui, epic, gog, aumid, xbox, lutris_id, \ - heroic, playnite) \ - instead" + return Some(( + format!("payload carries `{field}`, which this lane may not set"), + api_error( + StatusCode::FORBIDDEN, + &format!( + "`{field}` is executed as the host user and may only be set with the \ + operator's admin token — a plugin may publish entries with any host-resolved \ + launch kind (steam_appid, steam_ui, launcher_ui, epic, gog, aumid, xbox, lutris_id, \ + heroic, playnite) \ + instead" + ), ), )); } } crate::library::validate_art_paths(art) .err() - .map(|e| api_error(StatusCode::BAD_REQUEST, &e)) + .map(|e| (e.clone(), api_error(StatusCode::BAD_REQUEST, &e))) } #[derive(Deserialize)] @@ -205,7 +214,9 @@ pub(crate) async fn create_custom_game( if input.title.trim().is_empty() { return api_error(StatusCode::BAD_REQUEST, "title must not be empty"); } - if let Some(denied) = check_entry_fields(lane, &input.art, input.launch.as_ref(), &input.prep) { + if let Some((_, denied)) = + check_entry_fields(lane, &input.art, input.launch.as_ref(), &input.prep) + { return denied; } match crate::library::add_custom(input) { @@ -238,7 +249,9 @@ pub(crate) async fn update_custom_game( if input.title.trim().is_empty() { return api_error(StatusCode::BAD_REQUEST, "title must not be empty"); } - if let Some(denied) = check_entry_fields(lane, &input.art, input.launch.as_ref(), &input.prep) { + if let Some((_, denied)) = + check_entry_fields(lane, &input.art, input.launch.as_ref(), &input.prep) + { return denied; } use crate::library::MutateOutcome; @@ -364,11 +377,14 @@ pub(crate) async fn reconcile_provider_entries( // Every entry in the payload, not just the first — a reconcile replaces a whole entry set, so // one privileged field anywhere in it is one command execution. for (i, e) in inputs.iter().enumerate() { - if let Some(denied) = check_entry_fields(lane, &e.art, e.launch.as_ref(), &e.prep) { + if let Some((reason, denied)) = check_entry_fields(lane, &e.art, e.launch.as_ref(), &e.prep) + { tracing::warn!( provider, index = i, - "library reconcile refused: payload carries a field this lane may not set" + title = %e.title, + reason = %reason, + "library reconcile refused" ); return denied; } diff --git a/plugin-kit/bun.lock b/plugin-kit/bun.lock index 31675d59..42db0182 100644 --- a/plugin-kit/bun.lock +++ b/plugin-kit/bun.lock @@ -13,7 +13,7 @@ "typescript": "^5.9.3", }, "peerDependencies": { - "@punktfunk/host": "^0.1.2", + "@punktfunk/host": "^0.1.3", "effect": "^4.0.0-beta.98", "react": "^19.2.0", }, diff --git a/plugin-kit/package.json b/plugin-kit/package.json index d1058736..e07c4de0 100644 --- a/plugin-kit/package.json +++ b/plugin-kit/package.json @@ -1,6 +1,6 @@ { "name": "@punktfunk/plugin-kit", - "version": "0.3.2", + "version": "0.3.3", "description": "Effect-based framework for punktfunk plugins: lifecycle runtime, config/state, sync engine, UI serving, CLI scaffold, and browser helpers.", "type": "module", "license": "MIT OR Apache-2.0", @@ -56,7 +56,7 @@ }, "peerDependencies": { "effect": "^4.0.0-beta.98", - "@punktfunk/host": "^0.1.2", + "@punktfunk/host": "^0.1.3", "react": "^19.2.0" }, "peerDependenciesMeta": { diff --git a/plugin-kit/src/errors.ts b/plugin-kit/src/errors.ts index c9a34728..45707a9e 100644 --- a/plugin-kit/src/errors.ts +++ b/plugin-kit/src/errors.ts @@ -3,12 +3,46 @@ // Schema-based errors with status annotations. import { Data } from "effect"; -/** A management-API call through the pf facade failed. */ +/** + * A management-API call through the pf facade failed. + * + * The `message` getter is load-bearing, not decoration. `Data.TaggedError`'s default string form is + * the bare tag, and the sync engine logs `sync (${reason}) failed: ${e.cause}` — so a host that + * refused a reconcile with a perfectly clear 400 surfaced in the plugin log as exactly + * `sync (startup) failed: HostRequestError`, with the method, the path and the host's own + * explanation all discarded. Diagnosing the 2026-08-08 Lutris/Steam art rejection meant reading the + * HOST's journal instead, because the plugin's own log could not distinguish a validation refusal + * from the host being down. + */ export class HostRequestError extends Data.TaggedError("HostRequestError")<{ readonly method: string; readonly path: string; readonly cause: unknown; -}> {} +}> { + override get message(): string { + return `${this.method} ${this.path} failed: ${describeCause(this.cause)}`; + } +} + +/** + * Render whatever `pf.request` rejected with into one line. + * + * An `Error` stringifies usefully already; a plain object (the host's `{error: "…"}` body, which is + * what a rejected reconcile actually carries) stringifies to `[object Object]`, which is how the + * useful half of the message got lost. JSON is the fallback so a body-shaped cause survives, and a + * cycle or a BigInt degrades to `String(cause)` rather than throwing inside error formatting. + */ +const describeCause = (cause: unknown): string => { + if (cause instanceof Error) return cause.message; + if (typeof cause === "object" && cause !== null) { + try { + return JSON.stringify(cause); + } catch { + return String(cause); + } + } + return String(cause); +}; /** config.json exists but does not parse/decode. */ export class ConfigParseError extends Data.TaggedError("ConfigParseError")<{ diff --git a/plugin-kit/src/ui-server.ts b/plugin-kit/src/ui-server.ts index ddfb8723..d3bbd22e 100644 --- a/plugin-kit/src/ui-server.ts +++ b/plugin-kit/src/ui-server.ts @@ -7,7 +7,11 @@ import { Effect, FileSystem, Layer, Path, Schema, type Scope } from "effect"; import { Etag, HttpPlatform, HttpRouter } from "effect/unstable/http"; import type { ConfigService } from "./config.js"; import { UiServeError } from "./errors.js"; -import { HostClient, PluginInfo } from "./host-client.js"; +import { + HostClient, + type HostClientService, + PluginInfo, +} from "./host-client.js"; /** * Everything `HttpApiBuilder.layer` needs beyond the router, satisfied from effect core — @@ -192,7 +196,7 @@ export const serveUi = ( return handler(req); }; - return yield* Effect.acquireRelease( + const handle = yield* Effect.acquireRelease( Effect.tryPromise({ try: () => servePluginUi(host.facade, { @@ -212,4 +216,54 @@ export const serveUi = ( }), (handle) => Effect.promise(() => handle.close()).pipe(Effect.ignore), ); + + yield* verifyCategoryLanded(opts.category, info.name, host); + return handle; }); + +/** + * Read our own directory entry back and warn if the requested `category` is not on it. + * + * `category` travels through the UNTYPED `pf.request` seam precisely so an older host ignores it + * instead of rejecting the registration — which means dropping it is SILENT by design, at three + * different layers (an old host, an old runner-resolved SDK, a typo). On 2026-08-08 the middle one + * happened: `@punktfunk/host@0.1.2` was published before it forwarded the field, so every installed + * library scanner registered without a category. The visible result was Lutris and Heroic sitting in + * the console nav — which they explicitly opt out of — and their settings unreachable, because the + * Library section's Game sources surface lists exactly the plugins whose category IS `library`. + * Nothing logged anything. + * + * So this asks the host what it actually recorded. Same spirit as the store-claim degradation + * warning in `defineLibraryPlugin`: turn a silent no-op into one line that names the fix. Purely + * advisory — a failed read, or a host too old to report the field, must never keep a working plugin + * from starting. + */ +const verifyCategoryLanded = ( + category: string | undefined, + id: string, + host: { readonly request: HostClientService["request"] }, +): Effect.Effect => { + if (category === undefined) return Effect.void; + return host.request("GET", "/plugins").pipe( + Effect.flatMap((body) => { + const mine = (Array.isArray(body) ? body : []).find( + (p): p is { id: string; category?: string } => + typeof p === "object" && + p !== null && + (p as { id?: unknown }).id === id, + ); + // Not finding ourselves is not evidence of anything: the lease is registered + // best-effort, so a host that was momentarily away simply has not listed us yet. + if (!mine || mine.category === category) return Effect.void; + return Effect.logWarning( + `registered without category "${category}" (the host reports ` + + `${mine.category === undefined ? "none" : `"${mine.category}"`}). ` + + `This plugin will appear in the console's sidebar instead of its intended ` + + `section. The usual cause is an @punktfunk/host older than 0.1.3, which drops ` + + `the field before registering — update it, or the host, to resolve it.`, + ); + }), + // Advisory only: never let a diagnostic take down the plugin it is diagnosing. + Effect.ignore, + ); +}; diff --git a/plugin-kit/test/errors.test.ts b/plugin-kit/test/errors.test.ts new file mode 100644 index 00000000..731f66dd --- /dev/null +++ b/plugin-kit/test/errors.test.ts @@ -0,0 +1,64 @@ +// What a kit error says when something interpolates it — which is the whole diagnosis surface a +// plugin operator gets, because `sync-engine`'s failure path logs `${e.cause}` and nothing else. +import { describe, expect, test } from "bun:test"; +import { HostRequestError } from "../src/errors.js"; + +describe("HostRequestError", () => { + // Regression for 2026-08-08: this printed the bare tag, so `plugin:lutris sync (startup) + // failed: HostRequestError` was the ENTIRE record of a host that had answered with a precise + // 400. Interpolation is the assertion because interpolation is what the sync engine does. + test("names the call and carries the host's explanation", () => { + const err = new HostRequestError({ + method: "PUT", + path: "/library/provider/lutris?store=lutris", + cause: new Error("art.portrait: local art must be an image file"), + }); + + expect(`${err}`).toContain("PUT"); + expect(`${err}`).toContain("/library/provider/lutris?store=lutris"); + expect(`${err}`).toContain("art.portrait"); + expect(`${err}`).not.toBe("HostRequestError"); + }); + + // The host's rejection arrives as a parsed `{error: "…"}` body, not an Error. Left to default + // stringification that is `[object Object]` — the useful half lost a second way. + test("renders an object cause instead of [object Object]", () => { + const err = new HostRequestError({ + method: "PUT", + path: "/library/provider/steam", + cause: { error: "art.header: local art must be an image file" }, + }); + + expect(`${err}`).toContain("art.header"); + expect(`${err}`).not.toContain("[object Object]"); + }); + + // Error formatting must never itself throw: a cycle (or a BigInt) would make JSON.stringify + // blow up INSIDE the catch that is trying to report the original failure. + test("survives a cause that cannot be serialized", () => { + const cyclic: Record = {}; + cyclic.self = cyclic; + const err = new HostRequestError({ + method: "GET", + path: "/library", + cause: cyclic, + }); + + expect(() => `${err}`).not.toThrow(); + expect(`${err}`).toContain("/library"); + }); + + // The tag stays matchable — `Effect.catchTag`/`_tag` narrowing must not be traded away for a + // readable message. + test("keeps its tag and its fields", () => { + const err = new HostRequestError({ + method: "DELETE", + path: "/library/provider/heroic", + cause: "boom", + }); + + expect(err._tag).toBe("HostRequestError"); + expect(err.method).toBe("DELETE"); + expect(err.path).toBe("/library/provider/heroic"); + }); +}); diff --git a/sdk/package.json b/sdk/package.json index 0a30d853..787c7a91 100644 --- a/sdk/package.json +++ b/sdk/package.json @@ -1,6 +1,6 @@ { "name": "@punktfunk/host", - "version": "0.1.2", + "version": "0.1.3", "description": "TypeScript SDK for the punktfunk streaming host: typed management-API client + lifecycle event stream, built on Effect.", "type": "module", "license": "MIT OR Apache-2.0",