diff --git a/.github/workflows/preset-library.yml b/.github/workflows/preset-library.yml new file mode 100644 index 00000000..322c6b5e --- /dev/null +++ b/.github/workflows/preset-library.yml @@ -0,0 +1,81 @@ +# SPDX-FileCopyrightText: 2026 Uberware Inc. +# SPDX-License-Identifier: AGPL-3.0-or-later + +name: Preset library + +# Validates the PUBLISHED preset library against the validator on this branch. +# +# Why this exists: a validator change can silently invalidate content that is +# already published. Commit 2cdef4f tightened parameter-control validation to +# match the base spec and corrected every preset in this repo in the same +# change -- but the copy at uberware.github.io/sqi-presets is only regenerated +# by the release workflow, so from that commit until the next release every +# published preset failed to load. Nothing reported it: the list page renders +# from the index, which needs no validation, and only the detail page parses. +# The first signal was a user clicking a preset and getting an error. +# +# Deliberately NOT part of ci.yml. It reaches the network, so putting it on +# every pull request would make unrelated work hostage to an outage. Instead it +# runs where it is actually informative: +# +# - on a schedule, so drift is found within a day rather than by a user; +# - when the presets or the validator change, which is exactly when +# already-published content can become invalid; +# - on demand. +on: + schedule: + # 07:00 UTC daily. Drift here is never urgent-to-the-minute; it just must + # not wait for someone to click a preset. + - cron: "0 7 * * *" + push: + branches: [main] + paths: + - "presets/**" + - "internal/openjd/**" + - "internal/product/**" + - "internal/presetlib/**" + - "test/presetlib/**" + - ".github/workflows/preset-library.yml" + pull_request: + paths: + - "presets/**" + - "internal/openjd/**" + - "internal/product/**" + - "internal/presetlib/**" + - "test/presetlib/**" + - ".github/workflows/preset-library.yml" + workflow_dispatch: + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + published-presets: + name: Published presets validate against this tree + runs-on: ubuntu-latest + + steps: + - name: Checkout + uses: actions/checkout@v7 + + - name: Set up Go + uses: actions/setup-go@v7 + with: + go-version-file: go.mod + + - name: Validate the published preset library + shell: bash + run: make test-preset-library 2>&1 | tee /tmp/preset-library.log + + # The target exits 0 when the library is unreachable, because an offline + # runner is not evidence that the content is bad. That means a green step + # alone proves nothing -- assert the test actually ran and passed. A SKIP + # fails here on purpose: it means this check verified nothing, and the + # cause (network, or a repointed URL) needs a human. + - name: Assert the check actually ran + run: | + grep -q -- '--- PASS: TestPublishedPresets_ValidateAgainstThisTree' /tmp/preset-library.log diff --git a/.golangci.yml b/.golangci.yml index 61185316..f8630676 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -21,6 +21,7 @@ run: - integration - conformance - oracle + - presetlib # ── Linters ─────────────────────────────────────────────────────────────────── linters: diff --git a/Makefile b/Makefile index fd6138b7..8d5495d8 100644 --- a/Makefile +++ b/Makefile @@ -239,6 +239,23 @@ test-expr-oracle: ## Differential-test the EXPR evaluator against the OpenJD ref fi go test $(TEST_FLAGS) -tags oracle -run 'TestExprOracle' -v -timeout 5m ./test/oracle/ +# Validates the PUBLISHED preset library against the validator in this working +# tree. It exists because a validator change can silently invalidate content +# already published: 2cdef4f tightened parameter-control validation and fixed +# every preset in this repo, but the copy at uberware.github.io/sqi-presets is +# only refreshed on release, so every preset there failed to load in between -- +# with no signal until a user clicked one. +# +# Needs the network. SKIPS when the library is unreachable and FAILS when it is +# reachable but invalid, so an offline runner never masks a real breakage. A +# SKIP VERIFIES NOTHING -- look for the "--- PASS: TestPublishedPresets" line. +# CI asserts it by name for that reason. +# +# SQI_TEST_PRESET_LIBRARY_URL points it at a staging index instead. +.PHONY: test-preset-library +test-preset-library: ## Validate the published preset library against this tree (needs network) + go test $(TEST_FLAGS) -tags presetlib -run 'TestPublishedPresets' -v -timeout 5m ./test/presetlib/ + .PHONY: test-ldap test-ldap: ## Run the LDAP tests against a real directory in a container (needs Docker) go test $(TEST_FLAGS) -tags integration -run 'TestLDAP_' -v -timeout 15m ./test/integration/ diff --git a/clients/python/README.md b/clients/python/README.md index 080ed9e2..e6610e96 100644 --- a/clients/python/README.md +++ b/clients/python/README.md @@ -134,6 +134,7 @@ with SqiClient("http://localhost:8080") as sqi: template="specificationVersion: jobtemplate-2023-09\nname: My Renderer\nsteps: []\n", format="yaml", description="Render a frame range.", + readme="# My Renderer\n\nLonger usage notes in Markdown go here.\n", category="Rendering", version="1.0.0", ) @@ -166,6 +167,17 @@ with SqiClient("http://localhost:8080") as sqi: print("submitted job", job.id) ``` +A product has two independent text fields: `description` is a short +(max 500 character) plain-text blurb -- not Markdown, since it reaches +consumers that cannot render markup, such as the Blender addon's tooltip -- +and it is the field product search matches on. `readme` is long-form Markdown +(max 8000 characters), rendered only on the product's detail page in the web +UI; it is never searched. Both caps are enforced server-side (HTTP 400 on +create/update) and are not validated client-side. + +`update_product` is a full PUT replacement: any field omitted from the call, +including `readme`, is cleared on the server rather than left unchanged. + `get_product_parameters` raises `NotFoundError` when the product does not exist and `ValidationError` when the stored template cannot be parsed (HTTP 422). `submit_product_job` uses the keyword argument `job_name=` (not `name=`) to diff --git a/clients/python/src/sqi_client/client.py b/clients/python/src/sqi_client/client.py index a7e03e5c..64eee1d2 100644 --- a/clients/python/src/sqi_client/client.py +++ b/clients/python/src/sqi_client/client.py @@ -1490,6 +1490,7 @@ def create_product( format: str, title: str | None = None, description: str | None = None, + readme: str | None = None, category: str | None = None, version: str | None = None, ) -> Product: @@ -1499,10 +1500,26 @@ def create_product( name: Stable product name (slug). template: Raw OpenJD template text. format: ``yaml`` or ``json``. - title, description, category, version: Optional metadata. + title, category, version: Optional metadata. + description: Short plain-text catalog blurb (max 500 characters). + Plain text, not Markdown -- it reaches consumers that cannot + render markup, such as the Blender addon's tooltip. It is also + the field product search matches on. + readme: Long-form Markdown documentation (max 8000 characters), + rendered on the product's detail page in the web UI. It is NOT + searched. """ return self._products.create( - _product_body(name, title, description, category, version, template, format) + _product_body( + name, + template, + format, + title=title, + description=description, + readme=readme, + category=category, + version=version, + ) ) def update_product( @@ -1513,12 +1530,27 @@ def update_product( format: str, title: str | None = None, description: str | None = None, + readme: str | None = None, category: str | None = None, version: str | None = None, ) -> Product: - """Replace a custom product's fields (PUT, full replacement) and return it.""" + """Replace a custom product's fields (PUT, full replacement) and return it. + + Full replace: an omitted ``readme`` is CLEARED, not preserved, exactly + as ``description`` already behaves. + """ return self._products.update( - name, _product_body(name, title, description, category, version, template, format) + name, + _product_body( + name, + template, + format, + title=title, + description=description, + readme=readme, + category=category, + version=version, + ), ) def delete_product(self, name: str) -> None: @@ -1932,18 +1964,26 @@ def _compute_location_body( def _product_body( name: str, - title: str | None, - description: str | None, - category: str | None, - version: str | None, template: str, template_format: str, + *, + title: str | None = None, + description: str | None = None, + readme: str | None = None, + category: str | None = None, + version: str | None = None, ) -> dict[str, Any]: + # description and readme are both str | None: with positional arguments a + # transposition of the two would pass mypy silently and ship a bug, so + # every optional field beyond the first three positional ones is + # keyword-only. body: dict[str, Any] = {"name": name, "template": template, "format": template_format} if title is not None: body["title"] = title if description is not None: body["description"] = description + if readme is not None: + body["readme"] = readme if category is not None: body["category"] = category if version is not None: diff --git a/clients/python/src/sqi_client/models.py b/clients/python/src/sqi_client/models.py index 7e6b1ec6..08d506a4 100644 --- a/clients/python/src/sqi_client/models.py +++ b/clients/python/src/sqi_client/models.py @@ -1326,6 +1326,12 @@ class Product: name: str title: str = "" description: str = "" + """Short plain-text catalog blurb (max 500 characters). Not Markdown — it + reaches consumers that cannot render markup, such as the Blender addon's + tooltip. It is also the field product search matches on.""" + readme: str = "" + """Long-form Markdown documentation (max 8000 characters), rendered on the + product's detail page in the web UI. It is NOT searched.""" category: str = "" version: str = "" source: str = "" @@ -1344,6 +1350,7 @@ def from_dict(cls, data: Mapping[str, Any]) -> Product: name=_as_str(data.get("name")), title=_as_str(data.get("title")), description=_as_str(data.get("description")), + readme=_as_str(data.get("readme")), category=_as_str(data.get("category")), version=_as_str(data.get("version")), source=_as_str(data.get("source")), diff --git a/clients/python/tests/test_models.py b/clients/python/tests/test_models.py index 76368e82..45bdc1c1 100644 --- a/clients/python/tests/test_models.py +++ b/clients/python/tests/test_models.py @@ -658,6 +658,27 @@ def test_product_from_dict() -> None: assert p.format == "yaml" +def test_product_parses_readme() -> None: + from sqi_client.models import Product + + p = Product.from_dict({"name": "probe", "readme": "# Docs\n\nBody.\n"}) + assert p.readme == "# Docs\n\nBody.\n" + + +def test_product_readme_defaults_empty() -> None: + from sqi_client.models import Product + + assert Product.from_dict({"name": "probe"}).readme == "" + + +def test_product_readme_tolerates_mistyped_value() -> None: + # Matches the module's tolerant-parsing contract: a mistyped field falls + # back to a type-appropriate default rather than raising. + from sqi_client.models import Product + + assert Product.from_dict({"name": "probe", "readme": 17}).readme == "" + + def test_product_parameter_from_dict_with_user_interface() -> None: from sqi_client.models import ProductParameter diff --git a/clients/python/tests/test_products.py b/clients/python/tests/test_products.py index 74647ecd..4ed4f0e8 100644 --- a/clients/python/tests/test_products.py +++ b/clients/python/tests/test_products.py @@ -46,6 +46,28 @@ def test_create_product(make_client: ClientFactory) -> None: assert sent["template"] == "tmpl" +@respx.mock +def test_create_product_sends_readme(make_client: ClientFactory) -> None: + route = respx.post(f"{API}/products").mock( + return_value=httpx.Response(201, json={"name": "custom", "source": "custom"}) + ) + client = make_client() + client.create_product(name="custom", template="t", format="yaml", readme="# Docs") + sent = json.loads(route.calls.last.request.content) + assert sent["readme"] == "# Docs" + + +@respx.mock +def test_create_product_omits_readme_when_unset(make_client: ClientFactory) -> None: + route = respx.post(f"{API}/products").mock( + return_value=httpx.Response(201, json={"name": "custom", "source": "custom"}) + ) + client = make_client() + client.create_product(name="custom", template="t", format="yaml") + sent = json.loads(route.calls.last.request.content) + assert "readme" not in sent + + @respx.mock def test_update_product(make_client: ClientFactory) -> None: respx.put(f"{API}/products/custom").mock( diff --git a/docs/development.md b/docs/development.md index 38cade52..c6e908bc 100644 --- a/docs/development.md +++ b/docs/development.md @@ -69,6 +69,7 @@ Run `make` (no arguments) to see all available targets with descriptions. | `make test-oidc` | Run the SSO tests against a real Keycloak in a container (needs Docker; **skips** without it) | | `make test-isolation` | Run run-as-user task-isolation tests as real root against real OS accounts in a container (needs Docker; **skips** without it) | | `make test-expr-oracle` | Differential-test the EXPR evaluator against the OpenJD reference implementation (needs `python3`; **skips** without it) | +| `make test-preset-library` | Validate the **published** preset library against the validator in your tree (needs network; **skips** when the library is unreachable, **fails** when it is reachable but invalid) | | `make expr-oracle-venv` | Create `.venv-oracle/` with the pinned reference implementation (`make test-expr-oracle` does this on demand) | | `make smoke` | End-to-end smoke test against the real binaries (REST + WebSocket) | | `make bench` | Run benchmarks | diff --git a/docs/preset-library.md b/docs/preset-library.md index 8c3c2d17..73e115dc 100644 --- a/docs/preset-library.md +++ b/docs/preset-library.md @@ -53,6 +53,11 @@ promised) and **update detection** (if the hash in the index changes, the instal product is shown as having an update available). It is not a cryptographic authorship signature — the trust boundary is the configured index URL itself. +The index carries `description` but **not** `readme`. `description` is there +because the preset list page searches it; `readme` is not searched, so shipping +it in the index would grow every client's cached index for nothing. A preset's +readme arrives with its definition when the detail page is opened. + --- ## Configuration diff --git a/docs/products.md b/docs/products.md index cda0cd91..efcb4c7c 100644 --- a/docs/products.md +++ b/docs/products.md @@ -86,7 +86,8 @@ template: |---|---|---| | `name` | yes | Stable slug identity. Lowercase letters, digits, `_` and `-`, with at most one `/` namespace separator (e.g. `studio/maya-render`). | | `title` | yes | Human-readable display name. | -| `description` | no | Short summary shown in the catalog. | +| `description` | no | Short plain-text catalog blurb, max 500 runes (a Unicode character count, not a byte count). Shown in cards and in DCC submitter dropdowns, and matched by product search. **Plain text, not Markdown** — it reaches consumers that cannot render markup, such as the Blender addon's tooltip. | +| `readme` | no | Long-form Markdown documentation, max 8000 runes. Rendered on the product's detail page in the web UI. **Not searched**, and not carried in the preset library index. | | `category` | no | Free-form group label (e.g. `General`, `Rendering`). | | `version` | no | Semver string used for future update-detection. | | `template` | yes | Inline OpenJD job template (`specificationVersion: jobtemplate-2023-09`). | @@ -96,6 +97,54 @@ The inline template is re-serialized and fully validated (via `openjd.Parse` + `openjd.ValidateWithOptions`) when the definition is parsed — a malformed template is rejected at load time. +### Writing a `readme` + +`readme` must use a YAML **literal** block (`|`), not the folded block (`>-`) +that `description` uses. A folded scalar collapses newlines, which silently +destroys paragraph breaks and list structure — the result is one run-on +paragraph, with no error to tell you: + +```yaml +description: >- + Splits a video into segments and transcodes them in parallel. +readme: | + # FFmpeg Segment Transcode + + Splits the source into fixed-length segments, transcodes each on its own + worker, then concatenates the results. + + ## When to use it + + - Long sources where a single-worker transcode would take hours. + - Codecs that tolerate segment boundaries. + + Set `SegmentSeconds` to trade parallelism against concat overhead. +``` + +`readme` is inline content, not a path — `readme: ./README.md` renders the +literal text `./README.md`. + +`description` is plain text and is what product search matches. `readme` is +Markdown, rendered only on the detail page, and is **not** searched. + +**Supported Markdown:** paragraphs, unordered and ordered lists, fenced code +blocks, ATX headings (`#` renders as `

`, nested under the page's existing +heading structure, clamped at `

`), `**bold**`, `*italic*`, `` `code` `` +and `[links](https://example.com)` using `http:`, `https:` or `mailto:` only. + +**Lists are single-level only — nesting is not supported.** The renderer +(`web/src/components/Markdown.tsx`) matches list items with a regular +expression anchored at column 0. An item indented under another list item +does not become a nested list; it silently renders as an ordinary paragraph +with a stray leading `-` or `1.`, with no error or warning. This has already +caught one author on this branch — write every list flat, with no indented +sub-items. + +**Not supported,** and rendered as literal text: images, tables, blockquotes, +reference links, raw HTML, and nesting of any kind. Images are excluded +deliberately — a remote image in a preset readme is an IP beacon firing for +every viewer. + ### `userInterface` parameter hints The `userInterface` block on each parameter is base-spec OpenJD, not a product diff --git a/docs/web-accessibility.md b/docs/web-accessibility.md index 76d6c09e..4c908441 100644 --- a/docs/web-accessibility.md +++ b/docs/web-accessibility.md @@ -95,3 +95,17 @@ These are not regressions to file against; they are deliberately deferred: See [`web-development.md`](web-development.md) for the development workflow and the `CONTRIBUTING.md` "Web UI contributions" section for the component and testing conventions these checks fit into. + +--- + +## Headings in rendered Markdown + +Product and preset readmes render through `src/components/Markdown.tsx`, which +offsets heading levels rather than emitting them verbatim: `#` becomes `

`, +`##` becomes `

`, and so on, clamped at `

`. Both detail pages already +nest `

` (the product or preset title) under `

` (PageHeader), so an +un-offset `#` would put a second `

` mid-document and break the outline. + +Real headings are used rather than visually-styled paragraphs: a bold paragraph +looks like a heading to sighted users and is invisible as structure to a screen +reader. diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 45e6e614..f4f7fa1e 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1267,6 +1267,7 @@ components: type: string description: type: string + maxLength: 500 category: type: string version: @@ -1280,6 +1281,15 @@ components: - $ref: "#/components/schemas/Preset" - type: object properties: + readme: + type: string + maxLength: 8000 + description: >- + Long-form documentation in a restricted Markdown subset + (paragraphs, lists, fenced code, ATX headings, bold, italic, + inline code, links). Rendered on detail pages only. Unlike + `description` it is NOT searched, and it is not carried in the + preset library index. template: type: string format: @@ -1297,6 +1307,21 @@ components: type: string description: type: string + maxLength: 500 + description: >- + Short plain-text catalog blurb, shown in cards and in DCC + submitter dropdowns. Plain text, not Markdown -- it reaches + consumers that cannot render markup, such as the Blender addon's + tooltip. + readme: + type: string + maxLength: 8000 + description: >- + Long-form documentation in a restricted Markdown subset + (paragraphs, lists, fenced code, ATX headings, bold, italic, + inline code, links). Rendered on detail pages only. Unlike + `description` it is NOT searched, and it is not carried in the + preset library index. category: type: string version: @@ -1324,6 +1349,21 @@ components: type: string description: type: string + maxLength: 500 + description: >- + Short plain-text catalog blurb, shown in cards and in DCC + submitter dropdowns. Plain text, not Markdown -- it reaches + consumers that cannot render markup, such as the Blender addon's + tooltip. + readme: + type: string + maxLength: 8000 + description: >- + Long-form documentation in a restricted Markdown subset + (paragraphs, lists, fenced code, ATX headings, bold, italic, + inline code, links). Rendered on detail pages only. Unlike + `description` it is NOT searched, and it is not carried in the + preset library index. category: type: string version: @@ -1344,6 +1384,21 @@ components: type: string description: type: string + maxLength: 500 + description: >- + Short plain-text catalog blurb, shown in cards and in DCC + submitter dropdowns. Plain text, not Markdown -- it reaches + consumers that cannot render markup, such as the Blender addon's + tooltip. + readme: + type: string + maxLength: 8000 + description: >- + Long-form documentation in a restricted Markdown subset + (paragraphs, lists, fenced code, ATX headings, bold, italic, + inline code, links). Rendered on detail pages only. Unlike + `description` it is NOT searched, and it is not carried in the + preset library index. category: type: string version: diff --git a/internal/api/presets.go b/internal/api/presets.go index b0ac4341..bf8fc97c 100644 --- a/internal/api/presets.go +++ b/internal/api/presets.go @@ -109,6 +109,7 @@ type presetResponse struct { type presetDetailResponse struct { presetResponse + Readme string `json:"readme"` Template string `json:"template"` Format string `json:"format"` } @@ -217,6 +218,7 @@ func (h *presetHandler) getPreset(w http.ResponseWriter, r *http.Request) { Name: entry.Name, Title: def.Title, Description: def.Description, Category: def.Category, Version: def.Version, Status: installStatus(entry, byRef), }, + Readme: def.Readme, Template: def.Template, Format: string(def.Format), }) diff --git a/internal/api/products.go b/internal/api/products.go index 02bf3d57..414193c1 100644 --- a/internal/api/products.go +++ b/internal/api/products.go @@ -131,6 +131,7 @@ type productResponse struct { Name string `json:"name"` Title string `json:"title"` Description string `json:"description"` + Readme string `json:"readme"` Category string `json:"category"` Version string `json:"version"` Source string `json:"source"` @@ -142,6 +143,7 @@ type createProductRequest struct { Name string `json:"name"` Title string `json:"title"` Description string `json:"description"` + Readme string `json:"readme"` Category string `json:"category"` Version string `json:"version"` Template string `json:"template"` @@ -150,7 +152,7 @@ type createProductRequest struct { func toProductResponse(p store.Product) productResponse { return productResponse{ - Name: p.Name, Title: p.Title, Description: p.Description, + Name: p.Name, Title: p.Title, Description: p.Description, Readme: p.Readme, Category: p.Category, Version: p.Version, Source: string(p.Source), Template: p.Template, Format: string(p.Format), } @@ -494,13 +496,31 @@ func (h *productHandler) decodeProductBody(w http.ResponseWriter, r *http.Reques writeProblem(w, r, http.StatusBadRequest, "template is required") return store.Product{}, false } - if err := product.ValidateTemplate(req.Template, format, h.templateValidateOptions()); err != nil { - h.writeTemplateProblem(w, r, err) - return store.Product{}, false - } - return store.Product{ - Name: name, Title: req.Title, Description: req.Description, + p := store.Product{ + Name: name, Title: req.Title, Description: req.Description, Readme: req.Readme, Category: req.Category, Version: req.Version, Template: req.Template, Format: format, - }, true + } + if !h.validateProductBody(w, r, p, format) { + return store.Product{}, false + } + return p, true +} + +// validateProductBody enforces the metadata length caps (checked first, since +// it is cheap) and then the OpenJD template itself (the expensive check) on a +// product built from a decoded request body. It writes the problem response +// and returns false on either failure. +func (h *productHandler) validateProductBody( + w http.ResponseWriter, r *http.Request, p store.Product, format store.TemplateFormat, +) bool { + if err := product.ValidateMetadata(p); err != nil { + writeProblem(w, r, http.StatusBadRequest, err.Error()) + return false + } + if err := product.ValidateTemplate(p.Template, format, h.templateValidateOptions()); err != nil { + h.writeTemplateProblem(w, r, err) + return false + } + return true } diff --git a/internal/api/products_test.go b/internal/api/products_test.go index 0e1c71d7..2ee38fdd 100644 --- a/internal/api/products_test.go +++ b/internal/api/products_test.go @@ -575,6 +575,76 @@ func TestProducts_SubmitWithDependsOn_MissingUpstreamIs422(t *testing.T) { } } +// TestCreateProduct_ReadmeRoundTrips verifies readme is accepted on create, +// distinct from description, and comes back on both the create response and +// a subsequent GET. +func TestCreateProduct_ReadmeRoundTrips(t *testing.T) { + srv := newProductRouter(fake.New()) + req := newReq(t, http.MethodPost, "/api/v1/products", jsonBody(t, map[string]any{ + "name": "readme-probe", "title": "Readme Probe", "description": "blurb", + "readme": "# Docs\n\nBody.\n", + "template": validTemplate, "format": "yaml", + })) + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, req) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201: %s", rec.Code, rec.Body.String()) + } + var got struct { + Readme string `json:"readme"` + Description string `json:"description"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if got.Readme != "# Docs\n\nBody.\n" { + t.Errorf("readme = %q, want the markdown body", got.Readme) + } + if got.Description != "blurb" { + t.Errorf("description = %q, want %q", got.Description, "blurb") + } + + rec = httptest.NewRecorder() + srv.ServeHTTP(rec, newReq(t, http.MethodGet, "/api/v1/products/readme-probe", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("get status = %d, want 200", rec.Code) + } + if !strings.Contains(rec.Body.String(), `"readme"`) { + t.Error("GET response has no readme field") + } +} + +// TestCreateProduct_RejectsOverlongMetadata verifies decodeProductBody enforces +// product.ValidateMetadata's length caps with a 400, naming the offending field. +func TestCreateProduct_RejectsOverlongMetadata(t *testing.T) { + tests := []struct { + field string + size int + }{ + {"description", product.MaxDescriptionLen + 1}, + {"readme", product.MaxReadmeLen + 1}, + {"title", product.MaxTitleLen + 1}, + } + for _, tt := range tests { + t.Run(tt.field, func(t *testing.T) { + srv := newProductRouter(fake.New()) + payload := map[string]string{ + "name": "cap-probe", "title": "Cap Probe", + "template": validTemplate, "format": "yaml", + } + payload[tt.field] = strings.Repeat("a", tt.size) + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, newReq(t, http.MethodPost, "/api/v1/products", jsonBody(t, payload))) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400: %s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), tt.field) { + t.Errorf("problem body %q does not name the field %q", rec.Body.String(), tt.field) + } + }) + } +} + // TestProducts_SubmitRejectsInvalidRetryOverrides asserts the product submit // endpoint applies the same retry-override bounds as direct job submission. func TestProducts_SubmitRejectsInvalidRetryOverrides(t *testing.T) { diff --git a/internal/presetlib/presetlib_test.go b/internal/presetlib/presetlib_test.go index 9874b169..868f6eda 100644 --- a/internal/presetlib/presetlib_test.go +++ b/internal/presetlib/presetlib_test.go @@ -9,6 +9,7 @@ import ( "errors" "net/http" "net/http/httptest" + "reflect" "sync/atomic" "testing" "time" @@ -163,3 +164,19 @@ func TestFetchIndex_StaleCache_OnFailedRefresh(t *testing.T) { t.Fatalf("stale cache entry mismatch: want %q, got %q", entries[0].Name, got[0].Name) } } + +// IndexEntry deliberately carries NO readme. The readme reaches PresetDetail +// from the fetched DEFINITION, not from the index -- presetDetailResponse is +// built from def, and only entry.Name and the install status come from the +// index. Description is in the index solely because the preset LIST page +// searches it; readme is not searched, so putting it in the index would grow +// every client's cached index for nothing and change the remote index format. +// +// This test exists so that adding it later is a deliberate decision rather than +// drift while implementing "make readme searchable". +func TestIndexEntry_HasNoReadmeField(t *testing.T) { + t.Parallel() + if _, ok := reflect.TypeFor[presetlib.IndexEntry]().FieldByName("Readme"); ok { + t.Fatal("IndexEntry gained a Readme field; read this test's comment before removing it") + } +} diff --git a/internal/product/builtins/container.yaml b/internal/product/builtins/container.yaml index c694c7c1..dd912b48 100644 --- a/internal/product/builtins/container.yaml +++ b/internal/product/builtins/container.yaml @@ -2,6 +2,35 @@ name: container title: Run a Docker Image description: Run a command inside a Docker image. +readme: | + # Run a Docker Image + + Runs a container image to completion on a worker, with `docker run --rm`. Use it + when the work already ships as an image and you want the farm to schedule it + rather than reproduce its dependencies on every host. + + ## Parameters + + - **Image** — the image reference to run, for example `alpine:3.20` or + `registry.example.com/team/tool:1.4.2`. + + ## Example + + ```sh + docker run --rm registry.example.com/team/nuke:15.1 + ``` + + ## Notes + + The step declares `attr.worker.tag.docker`, so it will only ever be scheduled + onto workers tagged as having Docker. A worker without that tag is not a + candidate, and a job whose queue has no such worker stays unscheduled rather + than failing — check the queue's unschedulable reason if a job sits idle. + + The image is run with **no arguments and no volumes**. Anything the container + needs from the host has to be baked into the image or fetched by its own + entrypoint. Copy this product to a custom one if you need to mount storage or + pass a command. category: General version: 1.0.0 template: diff --git a/internal/product/builtins/python.yaml b/internal/product/builtins/python.yaml index eec44b4e..5db30362 100644 --- a/internal/product/builtins/python.yaml +++ b/internal/product/builtins/python.yaml @@ -2,6 +2,35 @@ name: python title: Run a Python Script description: Run a Python script with a chosen interpreter. +readme: | + # Run a Python Script + + Runs a Python script on a worker. The script is written to a file next to the + task and executed with the interpreter you name, so it needs no quoting + gymnastics and can be as long as you like. + + ## Parameters + + - **Interpreter** — the Python to run, `python3` by default. Give an absolute + path to pin a specific install, for example `/opt/rez/python/3.11/bin/python`. + - **Python Script** — the script body itself. + + ## Example + + ```python + import sys + print("running on", sys.platform) + ``` + + ## Notes + + The script is delivered as an OpenJD embedded file, so it reaches the worker as + a real `script.py` on disk rather than as a command-line argument. That is why + multi-line scripts, quotes and backslashes all survive intact. + + Nothing installs the interpreter for you. If a worker has no `python3` on its + `PATH` the task fails at launch, so pin the path or gate the step with a + `hostRequirements` tag when your fleet is mixed. category: General version: 1.0.0 template: diff --git a/internal/product/builtins/script.yaml b/internal/product/builtins/script.yaml index efa8f488..0764614c 100644 --- a/internal/product/builtins/script.yaml +++ b/internal/product/builtins/script.yaml @@ -2,6 +2,34 @@ name: script title: Run a Shell Command description: Run an arbitrary shell command on a worker. +readme: | + # Run a Shell Command + + Runs one shell command on a worker, as a single task. The smallest useful + product in sqi, and the quickest way to check that a queue and its workers are + actually working. + + ## Parameters + + - **Command** — the shell text to run. It is passed to `/bin/sh -c`, so pipes, + redirection and `&&` all work. + + ## Example + + ```sh + ffmpeg -i input.mov -c:v libx264 output.mp4 + ``` + + ## Notes + + The command runs on whichever worker picks up the task, as the account that + worker runs under, in a session directory the worker creates. Nothing is + staged for you: any path in the command must already be reachable from the + worker. + + There are no host requirements, so this product will run *anywhere*. Add a + `hostRequirements` block to the template if the command needs a particular + platform or tool — see the [OpenJD specification](https://github.com/OpenJobDescription/openjd-specifications). category: General version: 1.0.0 template: diff --git a/internal/product/builtins_test.go b/internal/product/builtins_test.go index 0562b56a..e015742f 100644 --- a/internal/product/builtins_test.go +++ b/internal/product/builtins_test.go @@ -43,3 +43,72 @@ func TestBuiltins_ContainerDeclaresDockerRequirement(t *testing.T) { } t.Fatal("container built-in not found") } + +// Every built-in ships a readme. These three are the first products a new +// operator opens, so they double as the worked example of what the readme +// field is for and which Markdown the renderer actually supports. +func TestBuiltins_AllHaveAReadme(t *testing.T) { + builtins := product.Builtins() + if len(builtins) == 0 { + t.Fatal("no builtins loaded") + } + for _, p := range builtins { + if strings.TrimSpace(p.Readme) == "" { + t.Errorf("builtin %q has no readme", p.Name) + } + if err := product.ValidateMetadata(p); err != nil { + t.Errorf("builtin %q: %v", p.Name, err) + } + } +} + +// The built-in readmes are the reference for authors, so between them they +// must exercise the whole supported subset -- and nothing outside it. A +// construct the renderer does not support would render as literal text in the +// very examples people copy. +func TestBuiltins_ReadmesExerciseTheSupportedSubset(t *testing.T) { + builtins := product.Builtins() + var all strings.Builder + for _, p := range builtins { + all.WriteString(p.Readme) + all.WriteString("\n") + } + corpus := all.String() + + supported := map[string]string{ + "ATX heading": "\n# ", + "bullet list": "\n- ", + "fenced code": "```", + "bold": "**", + "inline code": "`", + "a link": "](http", + } + for name, marker := range supported { + if !strings.Contains(corpus, marker) { + t.Errorf("no builtin readme demonstrates %s (looked for %q)", name, marker) + } + } + + // Unsupported constructs render as literal text; none may appear. + for _, p := range builtins { + for _, bad := range []struct{ name, marker string }{ + {"an image", "!["}, + {"a blockquote", "\n> "}, + {"a table", "\n|"}, + {"raw HTML", "<"}, + } { + if strings.Contains(p.Readme, bad.marker) { + t.Errorf("builtin %q readme contains %s (%q), which the renderer does not support", + p.Name, bad.name, bad.marker) + } + } + // Nested list items silently lose their structure -- the renderer's + // list regexes are anchored at column 0. + for line := range strings.SplitSeq(p.Readme, "\n") { + trimmed := strings.TrimLeft(line, " ") + if len(line) > len(trimmed) && (strings.HasPrefix(trimmed, "- ") || strings.HasPrefix(trimmed, "* ")) { + t.Errorf("builtin %q readme has an indented list item %q; nesting is not supported", p.Name, line) + } + } + } +} diff --git a/internal/product/definition.go b/internal/product/definition.go index 938f8902..c0d7a1ca 100644 --- a/internal/product/definition.go +++ b/internal/product/definition.go @@ -26,6 +26,9 @@ func validateName(name string) error { if name == "" { return errors.New("product: name is required") } + if err := checkLen("name", name, MaxNameLen); err != nil { + return err + } if !slugPattern.MatchString(name) { return fmt.Errorf("product: name %q is not a valid slug (lowercase, digits, '-', '_', one optional '/')", name) } @@ -152,6 +155,7 @@ type definitionFile struct { Name string `yaml:"name"` Title string `yaml:"title"` Description string `yaml:"description"` + Readme string `yaml:"readme"` Category string `yaml:"category"` Version string `yaml:"version"` Template yaml.Node `yaml:"template"` @@ -192,16 +196,21 @@ func ParseDefinition(data []byte, opts ValidateOptions) (store.Product, error) { if err != nil { return store.Product{}, fmt.Errorf("product: re-serialize template: %w", err) } - if err := ValidateTemplate(string(rawTemplate), store.TemplateFormatYAML, opts); err != nil { - return store.Product{}, err - } - return store.Product{ + p := store.Product{ Name: df.Name, Title: df.Title, Description: df.Description, + Readme: df.Readme, Category: df.Category, Version: df.Version, - Template: string(rawTemplate), Format: store.TemplateFormatYAML, - }, nil + } + if err := ValidateMetadata(p); err != nil { + return store.Product{}, err + } + if err := ValidateTemplate(string(rawTemplate), store.TemplateFormatYAML, opts); err != nil { + return store.Product{}, err + } + p.Template = string(rawTemplate) + return p, nil } diff --git a/internal/product/definition_test.go b/internal/product/definition_test.go index 35863a57..c1d8fcac 100644 --- a/internal/product/definition_test.go +++ b/internal/product/definition_test.go @@ -80,3 +80,65 @@ func TestParseDefinition_Errors(t *testing.T) { }) } } + +func TestParseDefinition_Readme(t *testing.T) { + t.Parallel() + const def = ` +name: readme-probe +title: Readme Probe +description: A short blurb. +readme: | + # Readme Probe + + Does a thing. +category: General +version: 1.0.0 +template: + specificationVersion: jobtemplate-2023-09 + name: Readme Probe + steps: + - name: Step + script: + actions: + onRun: + command: echo + args: ["hi"] +` + p, err := product.ParseDefinition([]byte(def), product.ValidateOptions{EnforceLimits: true}) + if err != nil { + t.Fatalf("ParseDefinition: %v", err) + } + want := "# Readme Probe\n\nDoes a thing.\n" + if p.Readme != want { + t.Errorf("Readme = %q, want %q", p.Readme, want) + } + if p.Description != "A short blurb." { + t.Errorf("Description = %q, want the blurb unchanged", p.Description) + } +} + +// A definition with no readme yields the empty string, not an error. +func TestParseDefinition_ReadmeOptional(t *testing.T) { + t.Parallel() + const def = ` +name: no-readme +title: No Readme +template: + specificationVersion: jobtemplate-2023-09 + name: No Readme + steps: + - name: Step + script: + actions: + onRun: + command: echo + args: ["hi"] +` + p, err := product.ParseDefinition([]byte(def), product.ValidateOptions{EnforceLimits: true}) + if err != nil { + t.Fatalf("ParseDefinition: %v", err) + } + if p.Readme != "" { + t.Errorf("Readme = %q, want empty", p.Readme) + } +} diff --git a/internal/product/limits.go b/internal/product/limits.go new file mode 100644 index 00000000..59fe443e --- /dev/null +++ b/internal/product/limits.go @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package product + +import ( + "fmt" + "unicode/utf8" + + "github.com/uberware/sqi/internal/store" +) + +// Metadata length limits, in RUNES rather than bytes: 500 bytes of CJK is ~166 +// characters, and a Japanese-language description must not be silently +// third-class. +// +// The two interesting caps differ in kind. MaxDescriptionLen is a design +// constraint -- description is rendered into an unclamped picker card and a +// native Blender EnumProperty tooltip, and 500 is 1.5x the longest description +// that survived commit a1e529e's hand-trim (329). It would have REJECTED the +// 940-, 629- and 617-rune descriptions that forced that commit, which is the +// test a cap should pass; a 1000 cap would have permitted all three. +// MaxReadmeLen is only an abuse guard -- readme is detail-page-only, so nothing +// downstream breaks; it simply should not be a novel. +// +// The rest have far more headroom than the shipped presets need (their maxima +// are name 35, title 37, category 11). They exist because capping description +// while leaving its neighbors unbounded would be incoherent once the helper +// exists. +const ( + MaxNameLen = 128 + MaxTitleLen = 200 + MaxDescriptionLen = 500 + MaxReadmeLen = 8000 + MaxCategoryLen = 64 + MaxVersionLen = 32 +) + +// checkLen returns an error naming the field, the actual rune count and the cap +// when value is longer than maxRunes. +func checkLen(field, value string, maxRunes int) error { + if n := utf8.RuneCountInString(value); n > maxRunes { + return fmt.Errorf("product: %s is %d characters, limit is %d", field, n, maxRunes) + } + return nil +} + +// ValidateMetadata enforces the length limits on a product's metadata fields. +// +// It is exported and deliberately called from BOTH doors into product data -- +// ParseDefinition and the REST create/update handler. Those are separate entry +// points to the same data, and the comment on ValidateOptions records this exact +// trap biting once already: the preset routes silently kept validating on +// DefaultExprLimits() after the create/update route was fixed. +// +// It checks LENGTH only. The slug pattern stays in validateName on the +// definition path, because applying it to the REST route would tighten +// acceptance and strand products already stored under a pattern-invalid name. +func ValidateMetadata(p store.Product) error { + checks := []struct { + field string + value string + max int + }{ + {"name", p.Name, MaxNameLen}, + {"title", p.Title, MaxTitleLen}, + {"description", p.Description, MaxDescriptionLen}, + {"readme", p.Readme, MaxReadmeLen}, + {"category", p.Category, MaxCategoryLen}, + {"version", p.Version, MaxVersionLen}, + } + for _, c := range checks { + if err := checkLen(c.field, c.value, c.max); err != nil { + return err + } + } + return nil +} diff --git a/internal/product/limits_test.go b/internal/product/limits_test.go new file mode 100644 index 00000000..7bdc4e99 --- /dev/null +++ b/internal/product/limits_test.go @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package product + +import ( + "strings" + "testing" + + "github.com/uberware/sqi/internal/store" +) + +func TestValidateMetadata_Boundaries(t *testing.T) { + t.Parallel() + tests := []struct { + name string + build func(s string) store.Product + max int + field string + }{ + {"title", func(s string) store.Product { return store.Product{Title: s} }, MaxTitleLen, "title"}, + {"description", func(s string) store.Product { return store.Product{Description: s} }, MaxDescriptionLen, "description"}, + {"readme", func(s string) store.Product { return store.Product{Readme: s} }, MaxReadmeLen, "readme"}, + {"category", func(s string) store.Product { return store.Product{Category: s} }, MaxCategoryLen, "category"}, + {"version", func(s string) store.Product { return store.Product{Version: s} }, MaxVersionLen, "version"}, + {"name", func(s string) store.Product { return store.Product{Name: s} }, MaxNameLen, "name"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if err := ValidateMetadata(tt.build(strings.Repeat("a", tt.max))); err != nil { + t.Errorf("at the cap: unexpected error %v", err) + } + err := ValidateMetadata(tt.build(strings.Repeat("a", tt.max+1))) + if err == nil { + t.Fatal("at cap+1: want an error, got nil") + } + if !strings.Contains(err.Error(), tt.field) { + t.Errorf("error %q does not name the field %q", err, tt.field) + } + }) + } +} + +// The caps count runes, not bytes. A CJK description of exactly +// MaxDescriptionLen characters is ~3x that many bytes and must still be +// accepted -- otherwise a Japanese-language description is silently +// third-class. +func TestValidateMetadata_CountsRunesNotBytes(t *testing.T) { + t.Parallel() + desc := strings.Repeat("日", MaxDescriptionLen) + if len(desc) <= MaxDescriptionLen { + t.Fatalf("test is not exercising multi-byte input: %d bytes for %d runes", len(desc), MaxDescriptionLen) + } + if err := ValidateMetadata(store.Product{Description: desc}); err != nil { + t.Errorf("CJK description at the rune cap: unexpected error %v", err) + } + if err := ValidateMetadata(store.Product{Description: desc + "日"}); err == nil { + t.Error("CJK description at cap+1: want an error, got nil") + } +} + +// The message carries the actual length as well as the cap, so an author can +// see how much to cut without counting by hand. +func TestValidateMetadata_ErrorNamesCapAndActual(t *testing.T) { + t.Parallel() + err := ValidateMetadata(store.Product{Description: strings.Repeat("a", MaxDescriptionLen+7)}) + if err == nil { + t.Fatal("want an error, got nil") + } + msg := err.Error() + for _, want := range []string{"description", "507", "500"} { + if !strings.Contains(msg, want) { + t.Errorf("error %q is missing %q", msg, want) + } + } +} + +// The length cap applies to name, but the slug PATTERN stays on the definition +// path only -- see the spec's post-approval finding. A pattern-invalid name is +// therefore not ValidateMetadata's business. +func TestValidateMetadata_IgnoresSlugPattern(t *testing.T) { + t.Parallel() + if err := ValidateMetadata(store.Product{Name: "Not A Slug!!"}); err != nil { + t.Errorf("unexpected error %v", err) + } +} + +func TestValidateName_RejectsOverlongSlug(t *testing.T) { + t.Parallel() + if err := validateName(strings.Repeat("a", MaxNameLen+1)); err == nil { + t.Fatal("want an error for a pattern-valid but over-long slug, got nil") + } + if err := validateName(strings.Repeat("a", MaxNameLen)); err != nil { + t.Errorf("at the cap: unexpected error %v", err) + } +} diff --git a/internal/store/fake/product_test.go b/internal/store/fake/product_test.go index a0af25b2..473763d4 100644 --- a/internal/store/fake/product_test.go +++ b/internal/store/fake/product_test.go @@ -83,3 +83,23 @@ func TestFakeProduct_OriginRoundTrip(t *testing.T) { t.Fatalf("fake dropped origin: %+v", got) } } + +// The fake stores the whole store.Product value, so Readme needs no explicit +// handling. This test exists so a future refactor to field-by-field copying +// cannot silently drop it. +func TestFakeProduct_ReadmeRoundTrips(t *testing.T) { + st := fake.New() + ctx := context.Background() + if _, err := st.CreateProduct(ctx, store.Product{ + ID: "p1", Name: "readme-probe", Title: "Readme Probe", Readme: "# Docs\n", + }); err != nil { + t.Fatalf("create: %v", err) + } + got, err := st.GetProductByName(ctx, "readme-probe") + if err != nil { + t.Fatalf("get: %v", err) + } + if got.Readme != "# Docs\n" { + t.Errorf("readme = %q, want %q", got.Readme, "# Docs\n") + } +} diff --git a/internal/store/migrations/00029_products_readme.sql b/internal/store/migrations/00029_products_readme.sql new file mode 100644 index 00000000..db72e2bb --- /dev/null +++ b/internal/store/migrations/00029_products_readme.sql @@ -0,0 +1,29 @@ +-- SPDX-License-Identifier: AGPL-3.0-or-later +-- Long-form markdown documentation for a product. +-- +-- Separate from `description` because the two serve incompatible jobs. +-- `description` must fit a picker card and a native Blender EnumProperty +-- tooltip, which is why commit a1e529e shortened every shipped preset's +-- description by hand -- the longest, 940 characters on +-- ffmpeg-segment-transcode-expr, was documentation wearing a blurb's clothes. +-- Markdown in `description` would not have helped: it adds formatting, not +-- length budget. So the blurb stays short, plain and searchable, and the +-- documentation moves here. +-- +-- `readme` is deliberately NOT searched. That is what keeps the change small: +-- with no search over it, no markdown stripper is needed in either TypeScript +-- or Python, and presetlib.IndexEntry needs no readme field, so the remote +-- preset-index format is unchanged. +-- +-- NOT NULL DEFAULT '' matches `description` and every other late-added string +-- column in this schema (see 00028's note): scanProduct reads it into a plain +-- string, so a NULL would be a scan error rather than an empty value. +-- +-- The Down migration's ALTER TABLE ... DROP COLUMN requires SQLite >= 3.35.0, +-- the same note 00002, 00008, 00013, 00026 and 00028 carry. + +-- +goose Up +ALTER TABLE products ADD COLUMN readme TEXT NOT NULL DEFAULT ''; + +-- +goose Down +ALTER TABLE products DROP COLUMN readme; diff --git a/internal/store/product.go b/internal/store/product.go index 1fab0d7c..1e70f408 100644 --- a/internal/store/product.go +++ b/internal/store/product.go @@ -27,11 +27,15 @@ type Product struct { Name string // stable identity, e.g. "script", "studio/maya-render" Title string Description string - Category string - Version string - Source Source - Template string // verbatim OpenJD template - Format TemplateFormat + // Readme is long-form markdown documentation, rendered on detail pages + // only. Unlike Description it is never searched and never reaches a + // plain-text consumer; see the field table in docs/products.md. + Readme string + Category string + Version string + Source Source + Template string // verbatim OpenJD template + Format TemplateFormat // OriginRef is the preset-library index entry name this product was // installed from; empty for builtin/custom products. OriginRef string diff --git a/internal/store/sqlite/product.go b/internal/store/sqlite/product.go index efdc64d2..c3eaa467 100644 --- a/internal/store/sqlite/product.go +++ b/internal/store/sqlite/product.go @@ -11,23 +11,23 @@ import ( const ( sqlInsertProduct = ` -INSERT INTO products (id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at) -VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) -RETURNING id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at` +INSERT INTO products (id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at) +VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) +RETURNING id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at` sqlGetProductByName = ` -SELECT id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at +SELECT id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at FROM products WHERE name = ?` sqlListProducts = ` -SELECT id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at +SELECT id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at FROM products ORDER BY name` sqlUpdateProduct = ` UPDATE products -SET title = ?, description = ?, category = ?, version = ?, template = ?, format = ?, origin_ref = ?, origin_fingerprint = ?, updated_at = ? +SET title = ?, description = ?, readme = ?, category = ?, version = ?, template = ?, format = ?, origin_ref = ?, origin_fingerprint = ?, updated_at = ? WHERE name = ? -RETURNING id, name, title, description, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at` +RETURNING id, name, title, description, readme, category, version, source, template, format, origin_ref, origin_fingerprint, created_at, updated_at` sqlDeleteProduct = `DELETE FROM products WHERE name = ?` ) @@ -36,7 +36,7 @@ func scanProduct(row scanner) (store.Product, error) { var p store.Product var source, format, createdAt, updatedAt string if err := row.Scan( - &p.ID, &p.Name, &p.Title, &p.Description, &p.Category, &p.Version, + &p.ID, &p.Name, &p.Title, &p.Description, &p.Readme, &p.Category, &p.Version, &source, &p.Template, &format, &p.OriginRef, &p.OriginFingerprint, &createdAt, &updatedAt, ); err != nil { return store.Product{}, err @@ -52,7 +52,7 @@ func scanProduct(row scanner) (store.Product, error) { func (s *Store) CreateProduct(ctx context.Context, p store.Product) (store.Product, error) { now := timeToText(time.Now().UTC()) row := s.stmtInsertProduct.QueryRowContext(ctx, - p.ID, p.Name, p.Title, p.Description, p.Category, p.Version, + p.ID, p.Name, p.Title, p.Description, p.Readme, p.Category, p.Version, string(p.Source), p.Template, string(p.Format), p.OriginRef, p.OriginFingerprint, now, now) out, err := scanProduct(row) return out, mapErr(err) @@ -87,7 +87,7 @@ func (s *Store) ListProducts(ctx context.Context) ([]store.Product, error) { func (s *Store) UpdateProduct(ctx context.Context, p store.Product) (store.Product, error) { now := timeToText(time.Now().UTC()) row := s.stmtUpdateProduct.QueryRowContext(ctx, - p.Title, p.Description, p.Category, p.Version, + p.Title, p.Description, p.Readme, p.Category, p.Version, p.Template, string(p.Format), p.OriginRef, p.OriginFingerprint, now, p.Name) out, err := scanProduct(row) return out, mapErr(err) diff --git a/internal/store/sqlite/product_test.go b/internal/store/sqlite/product_test.go index 8bc823cf..4756a02b 100644 --- a/internal/store/sqlite/product_test.go +++ b/internal/store/sqlite/product_test.go @@ -148,3 +148,70 @@ func TestProduct_UpdateOriginRoundTrip(t *testing.T) { t.Fatalf("persisted origin wrong: ref=%q fp=%q", fetched.OriginRef, fetched.OriginFingerprint) } } + +func TestProduct_ReadmeRoundTrips(t *testing.T) { + st := newProductStore(t) + ctx := context.Background() + + const readme = "# Heading\n\nBody with code.\n" + created, err := st.CreateProduct(ctx, store.Product{ + ID: "p1", Name: "readme-probe", Title: "Readme Probe", + Description: "short blurb", Readme: readme, + Source: store.SourceCustom, Template: "specificationVersion: jobtemplate-2023-09\nname: X\nsteps: []", Format: store.TemplateFormatYAML, + }) + if err != nil { + t.Fatalf("create: %v", err) + } + if created.Readme != readme { + t.Errorf("create returned readme %q, want %q", created.Readme, readme) + } + + got, err := st.GetProductByName(ctx, "readme-probe") + if err != nil { + t.Fatalf("get: %v", err) + } + if got.Readme != readme { + t.Errorf("get returned readme %q, want %q", got.Readme, readme) + } + + list, err := st.ListProducts(ctx) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(list) != 1 || list[0].Readme != readme { + t.Errorf("list returned %+v, want one row carrying the readme", list) + } + + updated, err := st.UpdateProduct(ctx, store.Product{ + Name: "readme-probe", Title: "Readme Probe", Description: "short blurb", + Readme: "replaced", Template: "specificationVersion: jobtemplate-2023-09\nname: X\nsteps: []", Format: store.TemplateFormatYAML, + }) + if err != nil { + t.Fatalf("update: %v", err) + } + if updated.Readme != "replaced" { + t.Errorf("update returned readme %q, want %q", updated.Readme, "replaced") + } +} + +// A product created without a readme reads back as "" rather than failing the +// scan. That is what the migration's empty-string default buys for +// pre-existing rows. +func TestProduct_ReadmeDefaultsEmpty(t *testing.T) { + st := newProductStore(t) + ctx := context.Background() + + if _, err := st.CreateProduct(ctx, store.Product{ + ID: "p2", Name: "no-readme", Title: "No Readme", + Source: store.SourceCustom, Template: "specificationVersion: jobtemplate-2023-09\nname: Y\nsteps: []", Format: store.TemplateFormatYAML, + }); err != nil { + t.Fatalf("create: %v", err) + } + got, err := st.GetProductByName(ctx, "no-readme") + if err != nil { + t.Fatalf("get: %v", err) + } + if got.Readme != "" { + t.Errorf("readme = %q, want empty", got.Readme) + } +} diff --git a/presets/sqi/ffmpeg-segment-transcode-bash.yaml b/presets/sqi/ffmpeg-segment-transcode-bash.yaml index a8a41df9..2bda3173 100644 --- a/presets/sqi/ffmpeg-segment-transcode-bash.yaml +++ b/presets/sqi/ffmpeg-segment-transcode-bash.yaml @@ -2,15 +2,60 @@ name: ffmpeg-segment-transcode-bash title: FFmpeg Segment Transcode (Bash) description: >- - Splits a video into fixed-length slices, converts each on a different worker, - then joins them back into one file. Enter the source's length in seconds - - nothing can measure it before the job is submitted; a value that is too - small silently drops the tail, and a value that is too large wastes a task - transcoding an empty tail slice. The join step runs a bash script, so this - variant needs Linux or macOS workers; use the Portable variant on Windows or - a mixed farm. Slice files are written beside the output - and removed once the join succeeds. Requires ffmpeg on PATH. A starting - point - duplicate to customize for your pipeline. + Splits a video into fixed-length slices, transcodes each on a different + worker, then joins them with a bash script, so it needs Linux or macOS + workers. +readme: | + # FFmpeg Segment Transcode (Bash) + + Splits one source video into fixed-length slices, transcodes each slice on + a different worker, then joins the finished slices back into a single + output file. The join step is a bash script, so this variant needs Linux + or macOS workers - use the Portable variant on Windows or a mixed farm. + + ## Steps + + 1. **Transcode** - fans out into one task per slice. Each task runs: + `ffmpeg -ss -t -i SourceFile ...` and writes + `_seg_<00001>.` beside `OutputFile`. + The number of tasks is `ceil(DurationSeconds / SegmentSeconds)`. + 2. **Join** - depends on every Transcode task, and runs an embedded bash + script on the worker. The script globs `/_seg_*.` next + to `OutputFile` - zero-padded slice numbers make lexical glob order the + same as numeric order - builds a concat file list from the matches, + then runs `ffmpeg -f concat -safe 0` to copy the streams into + `OutputFile` with no re-encode. On success it deletes the slice files + it just joined. + + ## Parameters + + - **SourceFile** - the video to split. + - **OutputFile** - the joined result. Its parent directory and stem also + name the intermediate slice files the join step globs for. + - **DurationSeconds** - the source's length in seconds. Type it in: nothing + in the job template can measure a source file before the job is + submitted. Too small silently drops the tail of the source - the last + slice(s) are never scheduled. Too large schedules an extra task past the + real end of the source: `ffmpeg` transcodes silence or a frozen frame for + that slice, wasting a task, but the join step still globs it up, joins + it in, and deletes it same as every other slice. + - **SegmentSeconds** - the length of each slice, and so the number of + tasks. + - **VideoCodec**, **Quality** (CRF, lower is better), **AudioCodec** - + passed straight to `ffmpeg` for every slice. + + ## Cleanup + + The join script removes every slice file it globbed once the concat + succeeds. If the join fails, the slice files are left on disk for you to + inspect or remove by hand. + + ## Requirements + + `ffmpeg` and `bash` must be on `PATH` for every worker that runs this job, + and workers must be tagged `attr.worker.tag.ffmpeg = true` with + `attr.worker.os.family` in `linux` or `macos`. This is a starting point - + duplicate it to customize for your own pipeline. category: Transcoding version: 1.0.0 template: diff --git a/presets/sqi/ffmpeg-segment-transcode-expr.yaml b/presets/sqi/ffmpeg-segment-transcode-expr.yaml index 088ff06b..e1d40352 100644 --- a/presets/sqi/ffmpeg-segment-transcode-expr.yaml +++ b/presets/sqi/ffmpeg-segment-transcode-expr.yaml @@ -2,19 +2,69 @@ name: ffmpeg-segment-transcode-expr title: FFmpeg Segment Transcode (Portable) description: >- - Splits a video into fixed-length slices, converts each on a different worker, - then joins them back into one file. Enter the source's length in seconds - - nothing can measure it before the job is submitted; a value that is too - small silently drops the tail, and a value that is too large wastes a task - and leaves an extra empty slice file beside the output for you to clean up, - same as the rest. Needs no shell at all: the join step's file list is - written by the template itself, so this variant runs on Linux, macOS - and Windows workers alike. The cost of writing that list is charged at - submission and grows with the slice count, so it suits jobs of up to 400 - slices - a longer source needs a longer slice to stay under that. Past 400, - use the Bash or PowerShell variant, whose cost does not grow. Slice files are - left beside the output for you to remove. Requires ffmpeg on PATH. A starting - point - duplicate to customize for your pipeline. + Splits a video into fixed-length slices, transcodes each on a different + worker, then joins them with a template-generated file list so it runs on + Linux, macOS and Windows workers alike. +readme: | + # FFmpeg Segment Transcode (Portable) + + Splits one source video into fixed-length slices, transcodes each slice on a + different worker, then joins the finished slices back into a single output + file. This variant needs no shell on the worker: the file list the join step + reads is generated by the job template itself using OpenJD's `EXPR` + extension, so the same template runs unchanged on Linux, macOS and Windows + workers. + + ## Steps + + 1. **Transcode** - fans out into one task per slice. Each task runs: + `ffmpeg -ss -t -i SourceFile ...` and writes + `_seg_<00001>.` beside `OutputFile`. + The number of tasks is `ceil(DurationSeconds / SegmentSeconds)`. + 2. **Join** - depends on every Transcode task. An embedded file lists every + slice path as `file ''`, one per line, built with an `EXPR` + `join()` expression over a `range()` comprehension - no script, no loop + on the worker. `ffmpeg -f concat -safe 0` reads that list and copies the + streams into `OutputFile` with no re-encode. + + ## Parameters + + - **SourceFile** - the video to split. + - **OutputFile** - the joined result. Its parent directory and stem also + name the intermediate slice files. + - **DurationSeconds** - the source's length in seconds. Type it in: nothing + in the job template can measure a source file before the job is + submitted. Too small silently drops the tail of the source - the last + slice(s) are never scheduled. Too large schedules an extra task past the + real end of the source: `ffmpeg` transcodes silence or a frozen frame for + that slice, wasting a task, and its slice file is still concatenated + into the join, then left on disk afterward (see Cleanup). + - **SegmentSeconds** - the length of each slice, and so the number of + tasks. + - **VideoCodec**, **Quality** (CRF, lower is better), **AudioCodec** - + passed straight to `ffmpeg` for every slice. + + ## Scaling + + Building the join step's file list is charged against this template's + `EXPR` evaluation budget at submission time, and that cost grows with the + number of slices. It comfortably fits jobs of up to about 400 slices; a + longer source needs a longer `SegmentSeconds` to stay under that, or use + the Bash or PowerShell variant instead, whose join step costs the same + regardless of slice count (it lists slices with a directory glob at run + time, not at submission time). + + ## Cleanup + + Unlike the Bash and PowerShell variants, this template does not remove the + per-slice files after a successful join - there is no shell step to do it + in. They are left beside `OutputFile` for you to delete. + + ## Requirements + + `ffmpeg` must be on `PATH` for every worker that runs this job, and workers + must be tagged `attr.worker.tag.ffmpeg = true`. This is a starting point - + duplicate it to customize for your own pipeline. category: Transcoding version: 1.0.0 template: diff --git a/presets/sqi/ffmpeg-segment-transcode-powershell.yaml b/presets/sqi/ffmpeg-segment-transcode-powershell.yaml index bfc5bd6a..da37f30f 100644 --- a/presets/sqi/ffmpeg-segment-transcode-powershell.yaml +++ b/presets/sqi/ffmpeg-segment-transcode-powershell.yaml @@ -2,15 +2,61 @@ name: ffmpeg-segment-transcode-powershell title: FFmpeg Segment Transcode (PowerShell) description: >- - Splits a video into fixed-length slices, converts each on a different worker, - then joins them back into one file. Enter the source's length in seconds - - nothing can measure it before the job is submitted; a value that is too - small silently drops the tail, and a value that is too large wastes a task - transcoding an empty tail slice. The join step runs a PowerShell script, so - this variant needs Windows workers; use the Portable variant on a mixed farm. - Slice files are written beside the output and removed once the join succeeds. - Requires ffmpeg on PATH. A starting point - duplicate to customize for your - pipeline. + Splits a video into fixed-length slices, transcodes each on a different + worker, then joins them with a PowerShell script, so it needs Windows + workers. +readme: | + # FFmpeg Segment Transcode (PowerShell) + + Splits one source video into fixed-length slices, transcodes each slice on + a different worker, then joins the finished slices back into a single + output file. The join step is a PowerShell script, so this variant needs + Windows workers - use the Portable variant on a mixed farm. + + ## Steps + + 1. **Transcode** - fans out into one task per slice. Each task runs: + `ffmpeg -ss -t -i SourceFile ...` and writes + `_seg_<00001>.` beside `OutputFile`. + The number of tasks is `ceil(DurationSeconds / SegmentSeconds)`. + 2. **Join** - depends on every Transcode task, and runs an embedded + PowerShell script on the worker. The script lists + `\_seg_*` next to `OutputFile` with `Get-ChildItem`, + sorted by name - zero-padded slice numbers make that the same as + numeric order - writes the matches to a BOM-less UTF-8 concat file list + (a BOM breaks the concat demuxer's parse of the first line), then runs + `ffmpeg -f concat -safe 0` to copy the streams into `OutputFile` with no + re-encode. On success it deletes the slice files it just joined. + + ## Parameters + + - **SourceFile** - the video to split. + - **OutputFile** - the joined result. Its parent directory and stem also + name the intermediate slice files the join step lists. + - **DurationSeconds** - the source's length in seconds. Type it in: nothing + in the job template can measure a source file before the job is + submitted. Too small silently drops the tail of the source - the last + slice(s) are never scheduled. Too large schedules an extra task past the + real end of the source: `ffmpeg` transcodes silence or a frozen frame for + that slice, wasting a task, but the join step still lists it, joins it + in, and deletes it same as every other slice. + - **SegmentSeconds** - the length of each slice, and so the number of + tasks. + - **VideoCodec**, **Quality** (CRF, lower is better), **AudioCodec** - + passed straight to `ffmpeg` for every slice. + + ## Cleanup + + The join script removes every slice file it listed once the concat + succeeds. If the join fails, the slice files are left on disk for you to + inspect or remove by hand. + + ## Requirements + + `ffmpeg` and `powershell` must be on `PATH` for every worker that runs this + job, and workers must be tagged `attr.worker.tag.ffmpeg = true` with + `attr.worker.os.family` set to `windows`. This is a starting point - + duplicate it to customize for your own pipeline. category: Transcoding version: 1.0.0 template: diff --git a/test/presetlib/published_test.go b/test/presetlib/published_test.go new file mode 100644 index 00000000..eb82dfdb --- /dev/null +++ b/test/presetlib/published_test.go @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +//go:build presetlib + +// Package presetlib_test validates the PUBLISHED preset library against the +// validator in this working tree. +// +// It exists because of a real, silent breakage. Commit 2cdef4f ("improved +// openJD conformance") tightened parameter-control validation to match the base +// spec -- a PATH parameter may not use LINE_EDIT -- and corrected every preset +// in this repo in the same change. What it could not correct was the copy +// already published at uberware.github.io/sqi-presets, which is only refreshed +// on release. From that commit until the next release, every preset in the +// library failed to load, and nothing anywhere reported it: the list page +// renders from the index (which needs no validation) and only the detail page +// parses, so the first signal was a user clicking a preset and getting an +// error. +// +// This test closes that gap by running the published bytes through the same +// path the server uses -- presetlib.FetchDefinition, which pins the sha256 and +// then calls product.ParseDefinition -- so a validator change that invalidates +// published content fails here rather than in someone's browser. +// +// Build tag `presetlib` keeps it out of `make ci`: it needs the network, and a +// unit-test suite that reaches the internet is a flake generator. Run it with +// `make test-preset-library`. +package presetlib_test + +import ( + "context" + "errors" + "net" + "os" + "testing" + "time" + + "github.com/uberware/sqi/internal/presetlib" + "github.com/uberware/sqi/internal/product" +) + +// defaultIndexURL mirrors config.Defaults()' preset_library.url. Duplicated +// rather than imported so this test validates what operators actually get by +// default, and fails loudly if that default is ever repointed without thought. +const defaultIndexURL = "https://uberware.github.io/sqi-presets/index.json" + +// indexURL is the library under test. SQI_TEST_PRESET_LIBRARY_URL points it at +// a staging index or a local file server. +func indexURL() string { + if u := os.Getenv("SQI_TEST_PRESET_LIBRARY_URL"); u != "" { + return u + } + return defaultIndexURL +} + +// unreachable reports whether err is a transport failure rather than a verdict +// about the content. An offline runner must SKIP; invalid content must FAIL. +// Collapsing the two would let a real breakage hide behind a network blip. +func unreachable(err error) bool { + var dnsErr *net.DNSError + var opErr *net.OpError + return errors.As(err, &dnsErr) || errors.As(err, &opErr) || errors.Is(err, context.DeadlineExceeded) +} + +// validateOptions mirrors what the preset routes pass in production: limits +// enforced, EXPR budget left at its defaults (this test has no operator +// configuration to offer), and a generous deadline since we are validating a +// whole library rather than serving one request. +func validateOptions() product.ValidateOptions { + return product.ValidateOptions{EnforceLimits: true} +} + +func TestPublishedPresets_ValidateAgainstThisTree(t *testing.T) { + url := indexURL() + svc := presetlib.New(url, time.Minute) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + entries, err := svc.FetchIndex(ctx, true) + if err != nil { + if unreachable(err) { + t.Skipf("preset library %s unreachable, skipping: %v", url, err) + } + t.Fatalf("fetch index %s: %v", url, err) + } + if len(entries) == 0 { + t.Fatalf("preset library %s lists no presets", url) + } + t.Logf("validating %d published presets from %s", len(entries), url) + + for _, entry := range entries { + t.Run(entry.Name, func(t *testing.T) { + // FetchDefinition is the production path: it verifies the index's + // sha256 before parsing, so this also catches a definition that + // drifted from the fingerprint the index vouches for. + if _, err := svc.FetchDefinition(ctx, entry, validateOptions()); err != nil { + if unreachable(err) { + t.Skipf("definition %s unreachable: %v", entry.Definition, err) + } + t.Errorf("published preset %q no longer validates against this tree.\n"+ + " definition: %s\n"+ + " error: %v\n"+ + "This means the published library is stale relative to the validator in\n"+ + "this working tree. Publish the corrected presets (the release workflow\n"+ + "regenerates the library from presets/), or, if the validator change was\n"+ + "unintended, revert it.", entry.Name, entry.Definition, err) + } + }) + } +} diff --git a/web/src/api/mutations.test.ts b/web/src/api/mutations.test.ts index 1e9451d3..9b67a55c 100644 --- a/web/src/api/mutations.test.ts +++ b/web/src/api/mutations.test.ts @@ -63,6 +63,7 @@ describe('product mutations', () => { name: 'my-render', title: 'My Render', description: '', + readme: '', category: '', version: '', template: 'name: x', diff --git a/web/src/api/mutations.ts b/web/src/api/mutations.ts index d5cff752..95b43c7d 100644 --- a/web/src/api/mutations.ts +++ b/web/src/api/mutations.ts @@ -595,6 +595,7 @@ export interface ProductInput { name: string title: string description: string + readme: string category: string version: string template: string diff --git a/web/src/api/types.ts b/web/src/api/types.ts index 1284e359..f11c0efc 100644 --- a/web/src/api/types.ts +++ b/web/src/api/types.ts @@ -413,6 +413,7 @@ export interface PresetListItem { /** Wire shape returned by GET /api/v1/presets/{name} (detail). */ export interface PresetDetail extends PresetListItem { + readme: string template: string format: string } @@ -424,6 +425,7 @@ export interface Product { name: string title: string description: string + readme: string category: string version: string source: ProductSource diff --git a/web/src/components/Markdown.module.css b/web/src/components/Markdown.module.css new file mode 100644 index 00000000..ee3c7300 --- /dev/null +++ b/web/src/components/Markdown.module.css @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: AGPL-3.0-or-later */ +.markdown { + color: var(--color-text-secondary); + font-size: var(--font-size-sm); +} + +.markdown p, +.markdown ul, +.markdown ol, +.markdown pre { + margin: 0 0 var(--space-3) 0; +} + +.markdown h3, +.markdown h4, +.markdown h5, +.markdown h6 { + color: var(--color-text-primary); + margin: var(--space-4) 0 var(--space-2) 0; +} + +.markdown pre { + overflow-x: auto; + padding: var(--space-3); + border-radius: var(--radius-sm); +} diff --git a/web/src/components/Markdown.test.tsx b/web/src/components/Markdown.test.tsx new file mode 100644 index 00000000..0a74db0a --- /dev/null +++ b/web/src/components/Markdown.test.tsx @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +import { render, screen } from '@testing-library/react' +import { describe, expect, it } from 'vitest' +import Markdown from './Markdown' + +describe('Markdown', () => { + it('renders paragraphs split on blank lines', () => { + render() + expect(screen.getByText('First para.')).toBeInTheDocument() + expect(screen.getByText('Second para.')).toBeInTheDocument() + }) + + it('renders inline bold, italic and code', () => { + render() + expect(screen.getByText('bold').tagName).toBe('STRONG') + expect(screen.getByText('italic').tagName).toBe('EM') + expect(screen.getByText('code').tagName).toBe('CODE') + }) + + it('renders unordered lists', () => { + render() + expect(screen.getAllByRole('listitem')).toHaveLength(2) + }) + + it('renders ordered lists', () => { + const { container } = render() + expect(container.querySelector('ol')).not.toBeNull() + expect(screen.getAllByRole('listitem')).toHaveLength(2) + }) + + // CommonMark lazy continuation: a plain text line right after a list item, + // with no blank line between them, belongs to that item -- not a new + // paragraph. This is the shape every shipped preset readme uses (bullets + // wrapped across source lines). + it('folds a wrapped bullet continuation into the same list item', () => { + render() + const items = screen.getAllByRole('listitem') + expect(items).toHaveLength(1) + expect(items[0]).toHaveTextContent('one continued text') + }) + + it('keeps a wrapped ordered list as one
    with correctly numbered items', () => { + const { container } = render() + const lists = container.querySelectorAll('ol') + expect(lists).toHaveLength(1) + const items = screen.getAllByRole('listitem') + expect(items).toHaveLength(2) + expect(items[0]).toHaveTextContent('first continued') + expect(items[1]).toHaveTextContent('second') + }) + + it('still ends a list on a blank line, not folding the following text', () => { + render() + const items = screen.getAllByRole('listitem') + expect(items).toHaveLength(1) + expect(items[0]).toHaveTextContent('a') + expect(screen.getByText('plain').tagName).toBe('P') + }) + + it('renders fenced code blocks verbatim, without inline parsing', () => { + render() + expect(screen.getByText(/not \*\*bold\*\* here/)).toBeInTheDocument() + expect(screen.queryByText('bold')).not.toBeInTheDocument() + }) + + // Both detail pages nest h1 (PageHeader) -> h2 (product/preset title) -> + // readme, so readme headings start at h3 to keep the document outline valid. + it('offsets headings so the outline stays correct', () => { + render() + expect(screen.getByText('One').tagName).toBe('H3') + expect(screen.getByText('Two').tagName).toBe('H4') + expect(screen.getByText('Three').tagName).toBe('H5') + expect(screen.getByText('Four').tagName).toBe('H6') + expect(screen.getByText('Five').tagName).toBe('H6') + }) + + it('renders http and mailto links', () => { + render() + expect(screen.getByRole('link', { name: 'docs' })).toHaveAttribute( + 'href', + 'https://example.com/x', + ) + expect(screen.getByRole('link', { name: 'mail' })).toHaveAttribute('href', 'mailto:a@b.c') + }) + + it('gives external links rel="noopener noreferrer"', () => { + render() + expect(screen.getByRole('link', { name: 'docs' })).toHaveAttribute('rel', 'noopener noreferrer') + }) + + // Regression guard for a real defect: safeHref must render the exact + // string it validated, not a case-folded copy used only for the scheme + // check. A `href={cleaned}` implementation lowercases the whole href and + // would turn this into a dead link ("mydoc.pdf" 404s where "MyDoc.pdf" + // exists), so this must fail against that implementation. + it('preserves path case in a safe href exactly as checked', () => { + render() + expect(screen.getByRole('link', { name: 'docs' })).toHaveAttribute( + 'href', + 'https://example.com/MyDoc.pdf', + ) + }) + + it('degrades unsupported syntax to literal text', () => { + render() + expect(screen.getByText(/\| a \| b \|/)).toBeInTheDocument() + expect(screen.queryByRole('table')).not.toBeInTheDocument() + }) + + describe('security', () => { + it.each([ + ['javascript:alert(1)'], + ['JaVaScRiPt:alert(1)'], + [' javascript:alert(1)'], + ['java\tscript:alert(1)'], + ['data:text/html,'], + ['vbscript:msgbox(1)'], + ])('renders %s as text, not a link', (href) => { + render() + expect(screen.queryByRole('link')).not.toBeInTheDocument() + expect(screen.getByText(/click/)).toBeInTheDocument() + }) + + // \s-class whitespace (space, tab, newline, CR, FF, vertical tab) never + // reaches safeHref at all: the INLINE token regex's own href group + // (`[^)\s]*`) excludes it, so a leading space or an embedded tab breaks + // the link match structurally and the whole "[label](href)" construct + // degrades to literal text before safeHref ever runs. These two pin that + // -- the safety net two layers deep, not just at safeHref. + it('degrades a leading-whitespace href to literal text, never a link', () => { + const { container } = render() + expect(screen.queryByRole('link')).not.toBeInTheDocument() + // getByText normalizes whitespace, which would hide the very thing + // being pinned here, so assert on textContent directly. + expect(container.textContent).toBe('[x]( https://example.com)') + }) + + it('degrades a tab-embedded href to literal text, never a link', () => { + render() + expect(screen.queryByRole('link')).not.toBeInTheDocument() + }) + + // A C0 control byte outside the \s class (U+0001 here) is NOT excluded + // by INLINE's href group, so unlike the two cases above it DOES reach + // safeHref. This is the reachable version of the "checked string must + // equal rendered string" property: safeHref strips it from the value + // that reaches the DOM, and -- per the case-preservation guard above -- + // does so without touching the case of the rest of the path. + it('strips a reachable control byte from a safe href while preserving case', () => { + render() + expect(screen.getByRole('link', { name: 'x' })).toHaveAttribute( + 'href', + 'https://example.com/MyPath', + ) + }) + + it('renders raw HTML as visible text', () => { + const { container } = render( + alert(1) and '} />, + ) + expect(container.querySelector('script')).toBeNull() + expect(container.querySelector('img')).toBeNull() + expect(screen.getByText(/