diff --git a/AGENTS.md b/AGENTS.md index 5d180ed..8ec64b3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -169,7 +169,9 @@ EXISTING app is missing the item-11 handshake (20 is the reachability repair to `field` every `--json` consumer groups on; and item 24 covers the ONE transport-vs-filesystem predicate now shared by the CLI-wide exit-code classifier (which every command's published exit code funnels through) and -`pkg/civitai`'s read-GET retry loop. +`pkg/civitai`'s read-GET retry loop; and item 25 covers blockId derivation — +the one identity this CLI mints that can NEVER be renamed, and the residuals the +refusal knowingly ships with. The durable fix for the mirroring is a server-side `civitai app validate` endpoint that calls the real `BlockManifestValidator` — until that exists, vendoring is on purpose. @@ -1865,6 +1867,112 @@ neither one's. full type resolution because `golang.org/x/tools/go/packages` would be a new dependency, which is an "ask first" below. +25. **The blockId derivation REFUSES rather than transliterates, and the + exemption that makes refusing safe is "LOWERCASES INTO ASCII" — never a + character allowlist.** `scaffold.Slugify` used to lowercase the name and + replace every run of non-`[a-z0-9]` with a hyphen, which silently DROPPED + content: `"Café App"` minted the blockId `caf-app` and `"ÜberApp Ω"` minted + `berapp` (measured), at exit 0, for an identity that **cannot be renamed + afterwards** — it is the hostname the app is served at and the argument every + later command takes. It now refuses and names the offending characters, with + `--slug` as the escape hatch (#259). + - 🔴 **THE EXEMPTION IS THE LOAD-BEARING PART, AND IT IS A PREDICATE ON THE + LOWERED RUNE, NOT A LIST.** `isLossyBase` asks `unicode.ToLower(r) < + utf8.RuneSelf` and, above that, whether the lowered rune is a + space/punct/symbol. Because lowercasing is what derivation does first, the + exemption re-decides NOTHING: every ASCII derivation this CLI has ever + produced is byte-identical, including the two dead ends whose existing + messages are good (`"123 Numbers"`, `"!!!"`). **Do not "improve" it into an + allowlist of permitted characters.** An allowlist is a second, hand- + maintained mirror of the slug alphabet that re-opens every one of those + derivations to a typo, and it buys nothing the boundary does not already + give. The `<` was a surviving mutant (only U+0080 changes hands, a C1 + control nobody types) and is now pinned from both sides precisely because + an unpinned boundary is a claim nothing holds. + - 🔴 **IT NARROWS #259; IT DOES NOT CLOSE IT — and the first write-up said + otherwise.** An audit measured three classes of input still producing the + exact `#259` shape after the refusal shipped. State the residuals with the + claim, or the next reader inherits a closure that was never delivered: + - **(a) INVALID UTF-8 — now CLOSED.** `app create $'caf\xe9 app'` derived + `caf-app` at rc 0, because `for _, r := range` yields U+FFFD per bad byte + and U+FFFD is a Symbol, i.e. the SEPARATOR branch. The written + `block.manifest.json` also held the raw `0xE9` and was not valid UTF-8, + and `validate.ManifestOnly` accepted it. `Slugify` now gates on + `utf8.ValidString` BEFORE anything ranges over the string, and + `runAppScaffold` refuses an invalid-UTF-8 DISPLAY name too — that second + guard is not redundant, because `--slug` bypasses derivation entirely and + the name still reaches the manifest verbatim. `internal/validate` still + has no UTF-8 check of its own; that gap is real and is not this item's. + - **(b) THE TWO RUNES ABOVE ASCII THAT LOWER INTO ASCII — a deliberate, + ENUMERATED exception.** All of Unicode was walked: exactly two exist, + `İ` U+0130 → `i` and `K` U+212A KELVIN SIGN → `k`. So `"İstanbul App"` + derives `istanbul-app` at rc 0. That is the nicest transliteration + available and it costs nothing, so it STAYS — but it is why any absolute + claim that every non-ASCII letter is refused is false, and why the + exemption is spelled "lowers into ASCII" rather than "is ASCII". + `TestSlugifyLowersIntoAsciiIsADocumentedException` re-walks Unicode and + fails if a future Go table grows a third. + - **(c) SYMBOLS, EMOJI AND NON-ASCII PUNCTUATION fold to a hyphen**, so + `"Rocket 🚀 App"` derives `rocket-app` at rc 0. Census over printable + non-ASCII runes: **8,580** take the separator branch, **140,321** the + refuse branch. This is the asymmetry working as designed for `—` and `»`, + and arguably not what an author means by an emoji — but an emoji has no + lossless ASCII form either, so refusing would trade a silent drop for a + dead end. **The product decision is made and the behaviour stays: + civitai/cli#272.** Do not re-argue it in a doc comment; point at the + issue. + - 🔴 **A COMBINING MARK IS REPORTED WITH ITS BASE, because the same VISIBLE + name arrives as two different byte sequences.** macOS paths and some paste + routes deliver NFD, so `"Café App"` arrives as `e` + U+0301 and the first + message read `"́" cannot appear in a blockId` — an accent rendered over + nothing, while the NFC form of the same name said `"é"`. `LossyChars` + clusters base+marks, so both forms now report `"é"`; a mark with no base at + all is shown on a dotted circle (U+25CC). This changed only the RENDERING — + a mark is non-ASCII and is neither space, punct nor symbol, so it was + already refused, and the set of refused names is unchanged. + - 🔴 **CLASSIFY ON THE LOWERED RUNE, REPORT THE ORIGINAL. The first version + did both on the lowered one and quoted characters the user never typed.** + Measured: `"ẞE App"` reported `"ß"` — a rune ABSENT from the input, because + U+1E9E lowers to U+00DF — and `"ＡＢＣ"` reported `"ａ", "ｂ", "ｃ"`. Someone + searching their own name for the quoted character finds nothing, which is + worse than a generic message. The two axes are independent and the code + keeps them apart; `TestSlugifyReportsTheCharacterTheAuthorTyped` pins it + with rows whose lowered form is provably not in the input. + - 🔴 **`--slug` SUPPRESSES THE NAME FIELD, NOT THE PROMPT — and getting that + wrong is a silent capability loss, not a cosmetic one.** `--slug` was first + wired into the `stdinIsTTY` guard on the reasoning that it "supplies the + one thing the prompt exists to collect". `runScaffoldForm` collects a name + AND a TEMPLATE, so `civitai app create --slug my-app` on a TTY silently + took page-money with no template choice — a question the user was asked + before the flag existed. The mutant deleting `slugFlag == ""` from that + guard survived with **zero** failures, because nothing covered the + suppression at all. Whenever a flag is made to skip a prompt, enumerate + what ELSE that prompt collects. + - 🔴 **THE ECHOED URL IS FUTURE TENSE, and that is not style.** The scaffold + echoes the blockId always (it was a DEAD PARAMETER of + `printScaffoldResult` — no line of output named the app's permanent id, and + with `--dir` the only copy was inside `block.manifest.json`). But the first + version printed the bare `https://.civit.ai/` as the "permanent + public id" at scaffold time — a URL **guaranteed to 404 at that exact + moment**, since the subdomain is only programmed on approval + deploy (the + README says so, and `app status` already says "Not live yet — … only serves + after the app is approved and deployed"). That is the same false-promise + class as the "validates clean" claim two lines up in the SAME output block. + Keep the two surfaces on the same words. + - 🔴 **A `mustNotProduce` ROW THAT CANNOT RUN IS NOT COVERAGE, AND A GREEN + SUITE CANNOT TELL YOU.** The refusal test claimed each row "pins the exact + pre-fix output it must not produce (`berapp`, `caf-del-mar`) rather than + merely 'an error came back'". The block sat AFTER a `t.Fatalf` that had + already aborted the subtest whenever `err == nil`, so its `e == nil` + condition never held. Measured: deleting the whole block left + `internal/scaffold` green while a positive control reddened 1 — the harness + could go red; the assertion simply never executed. It now lives INSIDE the + `err == nil` branch, and each row's expected pre-fix string is verified + against `legacySlugify`, a copy of the pre-refusal derivation, so a row + cannot name an output the old code never emitted. **When a test's headline + claim is "it pins the exact old value", check that the line can be + reached.** + **When you change a validation rule, keep all four vendored mirrors in sync with the server — `schema/`, the ported Go checks in `internal/validate/` (including the slot registry), the Vite dotenv resolution behind the dev-tunnel parent-origin diff --git a/README.md b/README.md index dccfd6f..df31ef8 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,7 @@ contract, and **packages/submits** it for review. - [Quickstart: build an App Block](#quickstart-build-an-app-block) - [SDK packages](#sdk-packages) - [Command reference](#command-reference) + - [The blockId](#the-blockid) - [Templates](#templates) - [The host handshake (`BLOCK_READY`)](#the-host-handshake-block_ready) - [Local dev loop (harness: mock vs live)](#local-dev-loop-harness-mock-vs-live) @@ -273,7 +274,7 @@ README. For the end-to-end walkthrough, see | `civitai buzz [--json]` | Show your spendable Buzz balance (**blue / green / yellow**, plus a **total**). Needs the BuzzRead scope — a full-scope personal API key or `civitai login --scopes generate`; a **default** OAuth login token can't read it, and gets a clear message naming both fixes. `--json` emits `{blue,green,yellow,total}` (scriptable — handy for before/after diffing a `dev:live` spend). | | `civitai app list [--kind ] [--category ] [--sort ] [--limit ] [--cursor ] [--json]` | **Discover published Apps in the store** (`GET /api/v1/apps`) — filter-based discovery, not free-text search. **Needs a credential** (`civitai login` or `CIVITAI_TOKEN`): the endpoint keys the visible catalog off your identity, so this is *not* one of the anonymous reads. Cursor-paged. See [Browse the App store](#browse-the-app-store). | | `civitai app view [--json]` | **Show one published App's store detail** (`GET /api/v1/apps/{slug}`) — description, category, rating, gallery, live/external target. **Needs a credential**, same as `app list`. Reads the *public store catalog*, which is a different resource from your own deploy — a not-found here says nothing about `.civit.ai`. See [Browse the App store](#browse-the-app-store). | -| `civitai app create [name] [dir] [--template static\|page-vite\|page-money] [--dir ] [--name ]` | **The friendly happy path.** Scaffold a ready-to-build App, defaulting to the batteries-included `page-money` SDK template (default dir `./`). | +| `civitai app create [name] [dir] [--template static\|page-vite\|page-money] [--dir ] [--name ] [--slug ]` | **The friendly happy path.** Scaffold a ready-to-build App, defaulting to the batteries-included `page-money` SDK template (default dir `./`). `--slug` sets the **blockId** explicitly instead of deriving it from the name — needed when derivation refuses the name (see [The blockId](#the-blockid)). | | `civitai app init [name] [dir] [...]` | Same scaffolder as `create` with a no-build `static` default (back-compat alias). | | `civitai app dev-token [--env] [--spend] [--budget ]` | **Mint a short-lived (~4h) dev block token for `npm run dev:live`.** `--spend` must be asked for explicitly to request real-Buzz spend — without it the CLI filters `ai:write:budgeted` out of the mint request. `--env` prints a paste-ready `VITE_LIVE_BLOCK_TOKEN=`. See [Local dev loop](#local-dev-loop-harness-mock-vs-live). | | `civitai app dev-tunnel [blockId] [--port] [--tunnel-endpoint] [--idle-timeout]` | **(Pre-GA / invite-gated)** Preview your **local** dev server inside the **real** Civitai host at `civitai.com/apps/dev/` — a prod-fidelity inner-dev-loop. Pre-flights whether the host can actually **embed** your dev server and warns (never fatally) when it cannot. See [Preview in the real host](#preview-in-the-real-host-app-dev-tunnel). | @@ -294,6 +295,40 @@ README. For the end-to-end walkthrough, see Run `civitai help`, `civitai app --help`, or `civitai --help` for the full details and examples. +### The blockId + +The **blockId** is your app's permanent public identity: the hostname it will be +served at once approved (`https://.civit.ai/`) and the argument every +later command takes (`app status`, `app metrics`, `app listing`, `app dev-token`, +`app dev-tunnel`). **It cannot be renamed afterwards.** `app create` / `app init` +echo the one they chose, so it is on screen before you commit anything. + +By default it is derived from the name: `"My Cool Block"` → `my-cool-block`. +Pass **`--slug `** to choose it yourself — it bypasses derivation entirely, +so name, blockId and directory are three fully independent axes. + +> **Breaking change.** Derivation used to lowercase the name and replace every +> run of non-`[a-z0-9]` with a hyphen, which silently **dropped characters**: +> `civitai app create "Café App"` minted the blockId **`caf-app`**, and +> `"ÜberApp"` minted **`berapp`** — a different permanent public id than the +> author typed, with no warning and exit 0. Derivation now **refuses** and names +> the offending characters, exiting **2** and asking for `--slug`. **If you have +> a script passing a non-ASCII name, it must now pass `--slug `.** The old +> output was wrong, so the break is the point — but it is a break. + +What derivation refuses is **letters, digits and marks** above ASCII that the +slug alphabet cannot carry. Three things still derive rather than refuse, and +they are deliberate: + +| input | blockId | why | +| --- | --- | --- | +| `"Rocket 🚀 App"` | `rocket-app` | Symbols, emoji and non-ASCII punctuation are **separators** — that is what makes `"Widget — Pro"` → `widget-pro` right. An emoji has no lossless ASCII form either, so refusing would only trade a silent drop for a dead end. Tracked as [#272](https://github.com/civitai/cli/issues/272). | +| `"İstanbul App"` | `istanbul-app` | Exactly two runes above ASCII lowercase **into** ASCII — `İ` (U+0130) and `K` (U+212A). Lowercasing is what decides whether a character survives, so these transliterate for free. | +| `"My Cool___Block"` | `my-cool-block` | ASCII is exempt by construction — every derivation that worked before still produces the byte-identical blockId. | + +A name that is **not valid UTF-8** is refused outright (it used to lose the bad +bytes from the blockId *and* write them into `block.manifest.json`). + ### Templates - **`static`** — a no-build page app (`index.html` + a tiny `app.js`, diff --git a/internal/cmd/app_create.go b/internal/cmd/app_create.go index 6e5f98a..814342a 100644 --- a/internal/cmd/app_create.go +++ b/internal/cmd/app_create.go @@ -14,6 +14,7 @@ func newAppCreateCmd() *cobra.Command { var fromSlug string var dirFlag string var nameFlag string + var slugFlag string var noInput bool cmd := &cobra.Command{ @@ -25,8 +26,9 @@ This is the friendly happy path: a thin superset of "civitai app init" that defaults to the rich page-money template — a Vite + React + TypeScript full-page app wired to the published App SDK (estimate -> consent -> submit -> poll -> Buzz spend), with a mock-host dev harness and a unit test. The scaffold is -immediately runnable (npm install && npm run dev:harness), test-green, and -validates clean. +immediately runnable (npm install && npm run dev:harness) and test-green. +"civitai app validate" passes once you have run "npm install" — until then it +correctly reports the package-lock.json the platform build installs from. The default scaffold ships a runnable txt2img money path AND a Comfy on Civitai (customComfy) sample that runs a server-registered recipe (invite-only beta) — @@ -50,6 +52,14 @@ Templates (override with --template): The display name can be free-form ("My Cool Block"); it is slugified for the blockId. A slug-shaped name is used verbatim. +The blockId is your app's PERMANENT public identity — the hostname your app will +be served at once it is approved, and the argument every later command takes — so +derivation refuses rather than guesses when the name carries LETTERS a blockId +cannot hold ("Café Del Mar", "ÜberApp", any non-Latin name). Punctuation, symbols +and emoji still fold to a hyphen, as they always have ("Rocket 🚀 App" -> +rocket-app). Pass --slug to choose the blockId yourself; it bypasses +derivation entirely. + By default the project is created in ./. Override the output directory with a positional [dir] or --dir ; override the display name independently with --name (so name, slug, and directory can all differ). @@ -67,19 +77,23 @@ the AI Services scopes: ` + spendCredentialRoutes + `.`, civitai app create my-block --template static # Custom output directory (slug stays my-block; created in ./apps/foo). - civitai app create my-block --dir ./apps/foo`, + civitai app create my-block --dir ./apps/foo + + # A name derivation cannot slugify: choose the blockId yourself. + civitai app create "Café Del Mar" --slug cafe-del-mar`, Args: cobra.MaximumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - return runAppScaffold(cmd, args, templateFlag, fromSlug, dirFlag, nameFlag, noInput) + return runAppScaffold(cmd, args, templateFlag, fromSlug, dirFlag, nameFlag, slugFlag, noInput) }, } // create defaults to the batteries-included page-money template; every // other flag matches init exactly. cmd.Flags().StringVarP(&templateFlag, "template", "t", string(scaffold.PageMoney), "project template: static | page-vite | page-money") - cmd.Flags().StringVar(&fromSlug, "from", "", "fork from an existing published app slug (not yet wired)") + cmd.Flags().StringVar(&fromSlug, "from", "", fromFlagUsage) cmd.Flags().StringVar(&dirFlag, "dir", "", "output directory (default ./)") cmd.Flags().StringVar(&nameFlag, "name", "", "display name (default derived from the name argument)") + cmd.Flags().StringVar(&slugFlag, "slug", "", slugFlagUsage) cmd.Flags().BoolVarP(&noInput, "yes", "y", false, "non-interactive: never prompt (use flags/defaults; fail if a name is missing)") return cmd } diff --git a/internal/cmd/app_create_cmd_test.go b/internal/cmd/app_create_cmd_test.go index 43083d5..ce045bc 100644 --- a/internal/cmd/app_create_cmd_test.go +++ b/internal/cmd/app_create_cmd_test.go @@ -233,15 +233,18 @@ func TestAppCreateRefusesNonEmptyDir(t *testing.T) { } } +// TestAppCreateFromIsNotWired pins the `--from` refusal for `app create`; the +// message contract itself (no internal TODO, an actionable next command) is +// asserted for BOTH commands in TestScaffoldFromErrorShipsNoEngineeringNote. func TestAppCreateFromIsNotWired(t *testing.T) { tmp := t.TempDir() dest := filepath.Join(tmp, "out") _, errOut, err := run(t, "app", "create", "my-block", dest, "--from", "some-slug") if err == nil { - t.Fatal("expected --from to error (not yet wired)") + t.Fatal("expected --from to error (not available yet)") } - if !strings.Contains(err.Error()+errOut, "not yet wired") { - t.Errorf("--from should report it is not wired: err=%v stderr=%s", err, errOut) + if !strings.Contains(err.Error()+errOut, "--from is not available yet") { + t.Errorf("--from should report it is unavailable: err=%v stderr=%s", err, errOut) } } diff --git a/internal/cmd/app_init.go b/internal/cmd/app_init.go index 4a0bcbe..bd58aa1 100644 --- a/internal/cmd/app_init.go +++ b/internal/cmd/app_init.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "strings" + "unicode/utf8" "github.com/charmbracelet/huh" "github.com/civitai/cli/internal/scaffold" @@ -20,6 +21,7 @@ func newAppInitCmd() *cobra.Command { var fromSlug string var dirFlag string var nameFlag string + var slugFlag string var noInput bool cmd := &cobra.Command{ @@ -36,6 +38,14 @@ Templates: The display name can be free-form ("My Cool Block"); it is slugified for the blockId. A slug-shaped name is used verbatim. +The blockId is your app's PERMANENT public identity — the hostname your app will +be served at once it is approved, and the argument every later command takes — so +derivation refuses rather than guesses when the name carries LETTERS a blockId +cannot hold ("Café Del Mar", "ÜberApp", any non-Latin name). Punctuation, symbols +and emoji still fold to a hyphen, as they always have ("Rocket 🚀 App" -> +rocket-app). Pass --slug to choose the blockId yourself; it bypasses +derivation entirely. + By default the project is created in ./. Override the output directory with a positional [dir] or --dir ; override the display name independently with --name (so name, slug, and directory can all differ).`, @@ -48,38 +58,60 @@ a positional [dir] or --dir ; override the display name independently with # Custom output directory (slug stays my-block; created in ./apps/foo). civitai app init my-block --dir ./apps/foo + # A name derivation cannot slugify: choose the blockId yourself. + civitai app init "Café Del Mar" --slug cafe-del-mar + # Name, slug, and dir all independent. civitai app init my-block ./apps/foo --name "My Block"`, Args: cobra.MaximumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - return runAppScaffold(cmd, args, templateFlag, fromSlug, dirFlag, nameFlag, noInput) + return runAppScaffold(cmd, args, templateFlag, fromSlug, dirFlag, nameFlag, slugFlag, noInput) }, } cmd.Flags().StringVarP(&templateFlag, "template", "t", string(scaffold.Static), "project template: static | page-vite | page-money") - cmd.Flags().StringVar(&fromSlug, "from", "", "fork from an existing published app slug (not yet wired)") + cmd.Flags().StringVar(&fromSlug, "from", "", fromFlagUsage) cmd.Flags().StringVar(&dirFlag, "dir", "", "output directory (default ./)") cmd.Flags().StringVar(&nameFlag, "name", "", "display name (default derived from the name argument)") + cmd.Flags().StringVar(&slugFlag, "slug", "", slugFlagUsage) cmd.Flags().BoolVarP(&noInput, "yes", "y", false, "non-interactive: never prompt (use flags/defaults; fail if a name is missing)") return cmd } +// slugFlagUsage / fromFlagUsage are shared by `app init` and `app create` so the +// two commands cannot drift on the same flag. +const ( + slugFlagUsage = "explicit blockId (bypasses derivation from the name; 3-40 chars, starts with a letter, lowercase a-z/0-9/hyphens)" + // The flag stays discoverable — it is a real roadmap item and hiding it + // would only move the surprise to a user who read about it elsewhere — but + // the usage string says up front that it cannot work yet, so the failure is + // predictable BEFORE the command is run rather than after. + fromFlagUsage = "fork from an existing published app slug (NOT AVAILABLE YET — the CLI cannot fetch app source)" +) + // runAppScaffold is the shared scaffold body behind both `app init` and // `app create`. The two commands differ only in the default template; all of // the slug/display derivation, dir resolution, rendering, self-validation, and // next-steps output lives here so there is a single code path. -func runAppScaffold(cmd *cobra.Command, args []string, templateFlag, fromSlug, dirFlag, nameFlag string, noInput bool) error { +func runAppScaffold(cmd *cobra.Command, args []string, templateFlag, fromSlug, dirFlag, nameFlag, slugFlag string, noInput bool) error { out := cmd.OutOrStdout() + // TODO(server): expose a read endpoint that returns a published app's + // canonical source tree by slug, then --from can scaffold from it. Until + // then the flag has a 100% failure rate, so the message a user sees is a + // plain actionable CLI error — the engineering context stays HERE. An + // end-user error is not the place to ship an internal ticket. if fromSlug != "" { - return fmt.Errorf(`--from is not yet wired up. - -Forking an existing published app requires fetching its source from the -server, which this CLI cannot do yet (no programmatic app-source endpoint). + return fmt.Errorf("--from is not available yet: this CLI cannot fetch a published app's source. Scaffold a plain project with `%s ` and copy the upstream files in by hand", cmd.CommandPath()) + } -TODO(server): expose a read endpoint that returns a published app's canonical -source tree by slug, then --from can scaffold from it. For now, run a plain -init and copy the upstream files in manually`) + // An explicit --slug bypasses derivation entirely, so it is checked against + // the same server contract a derived slug is. A bad VALUE is a usage error + // (exit 2), same class as a bad --template. + if slugFlag != "" { + if err := scaffold.ValidateSlug(slugFlag); err != nil { + return asUsageError(err) + } } name := "" @@ -92,8 +124,18 @@ init and copy the upstream files in manually`) // stdin (not a TTY), or --yes all SKIP huh — so scripted invocations never // block on a prompt (huh requires a TTY). The prompt fills in the name + // template; everything downstream is identical to the flag-driven path. + // + // 🔴 --slug SUPPRESSES THE NAME FIELD, NOT THE PROMPT. An earlier version + // added `&& slugFlag == ""` to this condition, reasoning that --slug + // "supplies the one thing the prompt exists to collect". It does not: the + // form collects a name AND a TEMPLATE, so `civitai app create --slug my-app` + // on a TTY silently took page-money with no template choice offered — a + // question the user was asked before and is no longer asked. The identity is + // the only thing --slug settles, so it drops that one field and the template + // select still runs; the display name then falls back to the slug's title + // case. --yes remains the way to skip the prompt entirely. if name == "" && !noInput && stdinIsTTY() { - inputs, err := scaffoldPromptFn(cmd, templateFlag) + inputs, err := scaffoldPromptFn(cmd, templateFlag, slugFlag == "") if err != nil { return err } @@ -103,6 +145,19 @@ init and copy the upstream files in manually`) } } + // The display name is written VERBATIM into block.manifest.json. JSON is + // UTF-8 by definition, so an invalid-UTF-8 name produces a manifest no + // conforming parser can read — and the platform is the one that finds out. + // scaffold.Slugify refuses the same bytes for the blockId (see its (a) + // residual); this is the other half, and it is needed because --slug + // bypasses derivation entirely, so the name reaches the manifest unchecked. + // A bad flag/arg VALUE is a usage error (exit 2), same class as --template. + for _, v := range []struct{ what, val string }{{"the name argument", name}, {"--name", nameFlag}} { + if v.val != "" && !utf8.ValidString(v.val) { + return asUsageError(fmt.Errorf("%s is not valid UTF-8: %q — the display name is written into block.manifest.json verbatim, and JSON must be UTF-8", v.what, v.val)) + } + } + // A bad --template VALUE is a usage error (exit 2), exactly like a bad flag // NAME. scaffold cannot tag it itself — ErrUsage lives here, and internal/cmd // already imports internal/scaffold — so the tag is attached at the call site. @@ -112,7 +167,7 @@ init and copy the upstream files in manually`) return asUsageError(err) } - if name == "" { + if name == "" && slugFlag == "" { return asUsageError(fmt.Errorf("provide a project name: %s ", cmd.CommandPath())) } @@ -130,18 +185,35 @@ init and copy the upstream files in manually`) targetDir = posDir } - // Derive slug + display name. If the name is already a valid slug - // use it verbatim; otherwise slugify and title-case for display. + // Derive slug + display name. An explicit --slug wins outright; otherwise a + // name that is already a valid slug is used verbatim, and anything else is + // slugified (which REFUSES rather than dropping characters — see + // scaffold.LossyChars, and the residuals in scaffold.Slugify's header: + // derivation still folds symbols/emoji to a hyphen and still lowers the two + // runes above ASCII that lower INTO ASCII). + nameIsSlug := name != "" && scaffold.ValidateSlug(name) == nil var slug, display string - if err := scaffold.ValidateSlug(name); err == nil { + switch { + case slugFlag != "": + slug = slugFlag + case nameIsSlug: slug = name - display = scaffold.TitleFromSlug(name) - } else { + default: // An unusable argument is a bad VALUE, same class as --template. slug, err = scaffold.Slugify(name) if err != nil { return asUsageError(err) } + } + + // Display name: title-cased from whichever identifier we have when the user + // typed one that reads as an id, verbatim when they typed prose. + switch { + case name == "": + display = scaffold.TitleFromSlug(slug) + case nameIsSlug: + display = scaffold.TitleFromSlug(name) + default: display = name } @@ -190,12 +262,49 @@ init and copy the upstream files in manually`) return nil } +// installStepFmt is next-step 1 for the templates that install. It is shared by +// both npm branches so they cannot drift. +// +// 🔴 THE SECOND LINE IS THE FIX, NOT DECORATION. `civitai app validate` FAILS on +// a freshly-scaffolded page-money / page-vite project until `npm install` has +// written package-lock.json — the platform build installs strictly from the +// committed lockfile, so the check is right and its message is good. What was +// wrong was the PROMISE: `app create --help` said the scaffold "validates +// clean", and validating before installing ~103 MB of node_modules is the +// natural first instinct. Saying it here (the last screen before the author +// starts) and in the help text is what closes that minute-3 surprise. `static` +// ships no package.json, installs nothing, and does validate clean — which is +// why this line lives on the install branches only (issue #260). +const installStepFmt = " 1. cd %s && npm install # writes package-lock.json — COMMIT it\n" + + " (`civitai app validate` fails until you do — it needs that lockfile)\n" + // printScaffoldResult prints a one-line summary (with a file count, not the // full tree) followed by a numbered, template-tailored "Next steps" sequence. func printScaffoldResult(out io.Writer, display, slug string, tmpl scaffold.Template, destDir, abs string, written []string) { // One scannable line: name, template, where, and how many files — no tree. fmt.Fprintln(out, ui.Success(fmt.Sprintf("Created App %q (%s) · %s/ · %d files", display, tmpl, destDir, len(written)))) + // 🔴 ECHO THE blockId ALWAYS, not only when it was derived or only when --dir + // was passed. It is the app's PERMANENT public identity — the hostname it is + // served at, and the argument `app status` / `app metrics` / `app listing` + // all take — and `slug` was a DEAD PARAMETER here: no line of this output + // named it. With the default directory you can infer it from the directory + // name; with --dir it is invisible unless you open block.manifest.json. So + // the case where derivation is most likely to surprise you was the case + // where it was least visible (issue #259). + // + // 🔴 THE URL IS A FUTURE TENSE, AND THAT IS NOT A STYLE CHOICE. The first + // version printed the bare `https://.civit.ai/` as the app's + // "permanent public id" at scaffold time — a URL that is guaranteed to 404 + // at that exact moment, because the subdomain is only programmed on + // approval + deploy (README: "Before approval, https://.civit.ai/ + // 404s"). That is the same false-promise class as the "validates clean" + // claim two lines up in this very output block, and `app status` already + // gets it right ("Not live yet — … only serves after the app is approved and + // deployed"). Keep the two surfaces saying the same thing. + fmt.Fprintln(out, ui.Dim(fmt.Sprintf(" blockId: %s — your app's permanent public id (it cannot be renamed later)", slug))) + fmt.Fprintln(out, ui.Dim(fmt.Sprintf(" Will be served at https://%s.civit.ai/ — only after the app is approved and deployed · `civitai app status %s`", slug, slug))) + // page-money ships a runnable txt2img money path and a Comfy on Civitai // (customComfy) sample, with body builders for BOTH customComfy arms — the // server-registered recipe, and an inline graph the app ships itself. Surface @@ -230,7 +339,7 @@ func printScaffoldResult(out io.Writer, display, slug string, tmpl scaffold.Temp // The two beta surfaces are still listed submit-first, but that is a // PRESENTATIONAL grouping (publish path, then preview path) — not a // dependency. Do not re-derive an ordering requirement from it. - fmt.Fprintf(out, " 1. cd %s && npm install # writes package-lock.json — COMMIT it\n", destDir) + fmt.Fprintf(out, installStepFmt, destDir) fmt.Fprintln(out, " 2. npm run dev:harness # mock host, no Buzz — works today") fmt.Fprintln(out, " 3. edit src/App.tsx and iterate") fmt.Fprintln(out) @@ -239,7 +348,7 @@ func printScaffoldResult(out io.Writer, display, slug string, tmpl scaffold.Temp fmt.Fprintln(out, " npm run dev:tunnel # in another terminal: serve your app for the tunnel") fmt.Fprintln(out, " civitai app dev-tunnel # your LOCAL app INSIDE the real host, prod-fidelity — no submit needed") case tmpl == scaffold.PageVite: - fmt.Fprintf(out, " 1. cd %s && npm install # writes package-lock.json — COMMIT it\n", destDir) + fmt.Fprintf(out, installStepFmt, destDir) fmt.Fprintln(out, " 2. npm run dev # preview locally (your UI only — see below)") fmt.Fprintln(out, " 3. civitai app submit # validate + submit for review") default: @@ -304,29 +413,35 @@ var scaffoldPromptFn = runScaffoldForm // runScaffoldForm runs the huh form that collects a missing app name + template. // defaultTemplate pre-selects the template (the command's default). The form // renders to stderr (status stream) and reads the command's stdin. -func runScaffoldForm(cmd *cobra.Command, defaultTemplate string) (scaffoldInputs, error) { +// +// askName is false when --slug already settled the identity: the TEMPLATE select +// still runs, because that is a second, independent question the prompt exists +// to ask and --slug says nothing about it. +func runScaffoldForm(cmd *cobra.Command, defaultTemplate string, askName bool) (scaffoldInputs, error) { in := scaffoldInputs{template: defaultTemplate} + var fields []huh.Field + if askName { + fields = append(fields, huh.NewInput(). + Title("App name"). + Description(`Free-form ("My Cool Block") — slugified for the blockId.`). + Value(&in.name). + Validate(func(s string) error { + if strings.TrimSpace(s) == "" { + return fmt.Errorf("a name is required") + } + return nil + })) + } + fields = append(fields, huh.NewSelect[string](). + Title("Template"). + Options( + huh.NewOption("static — no-build page app (index.html + a tiny JS)", string(scaffold.Static)), + huh.NewOption("page-vite — Vite + React page app", string(scaffold.PageVite)), + huh.NewOption("page-money — Vite + React + TS SDK money-path app", string(scaffold.PageMoney)), + ). + Value(&in.template)) form := huh.NewForm( - huh.NewGroup( - huh.NewInput(). - Title("App name"). - Description(`Free-form ("My Cool Block") — slugified for the blockId.`). - Value(&in.name). - Validate(func(s string) error { - if strings.TrimSpace(s) == "" { - return fmt.Errorf("a name is required") - } - return nil - }), - huh.NewSelect[string](). - Title("Template"). - Options( - huh.NewOption("static — no-build page app (index.html + a tiny JS)", string(scaffold.Static)), - huh.NewOption("page-vite — Vite + React page app", string(scaffold.PageVite)), - huh.NewOption("page-money — Vite + React + TS SDK money-path app", string(scaffold.PageMoney)), - ). - Value(&in.template), - ), + huh.NewGroup(fields...), ).WithInput(cmd.InOrStdin()).WithOutput(cmd.ErrOrStderr()) if err := form.Run(); err != nil { return in, err diff --git a/internal/cmd/app_init_echo_test.go b/internal/cmd/app_init_echo_test.go new file mode 100644 index 0000000..977c973 --- /dev/null +++ b/internal/cmd/app_init_echo_test.go @@ -0,0 +1,248 @@ +package cmd + +import ( + "errors" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + "unicode/utf8" + + "github.com/spf13/cobra" +) + +// serveURLRe finds the `https://.civit.ai/` the scaffold echoes. The +// capture group is the whole thing this file exists to assert on. +var serveURLRe = regexp.MustCompile(`https://([^\s]+)\.civit\.ai/`) + +// TestScaffoldServingURLIsBuiltFromTheSlugNotTheDisplayName pins the OPERAND of +// the echoed URL, which nothing did before. +// +// 🔴 SWAPPING `slug` FOR `display` IN THAT `Fprintf` SURVIVED THE WHOLE SUITE. +// Every existing assertion was a `strings.Contains(stdout, slug)`, and the FIRST +// `%s` on that line still carried the slug — so the mutant printed +// `https://Widget Machine.civit.ai/` as the app's "permanent public id" and no +// test could see it. The fixture keeps slug, display name and directory pairwise +// distinct AND substring-disjoint, so no assertion here can be satisfied by the +// wrong value. +func TestScaffoldServingURLIsBuiltFromTheSlugNotTheDisplayName(t *testing.T) { + tmp := t.TempDir() + dest := filepath.Join(tmp, "zzz-output-dir") + + stdout, _, err := run(t, "app", "create", "ignored-positional", + "--name", "Widget Machine", + "--slug", "qqq-block-id", + "--dir", dest, + "--template", "static") + if err != nil { + t.Fatalf("app create: %v\n%s", err, stdout) + } + + m := serveURLRe.FindAllStringSubmatch(stdout, -1) + if len(m) != 1 { + t.Fatalf("expected exactly one https://.civit.ai/ in the output, got %d:\n%s", len(m), stdout) + } + if got := m[0][1]; got != "qqq-block-id" { + t.Errorf("the serving URL must be built from the blockId, got host %q (want qqq-block-id):\n%s", got, stdout) + } + // The display name must not appear inside the URL at all — the mutation this + // test exists for produces `https://Widget Machine.civit.ai/`. + if strings.Contains(stdout, "https://Widget") { + t.Errorf("the display name leaked into the serving URL:\n%s", stdout) + } +} + +// TestScaffoldDoesNotPromiseTheURLServesYet — issue #260, same class as the +// "validates clean" promise two lines up in the SAME output block. +// +// 🔴 THE URL IS GUARANTEED TO 404 AT THE MOMENT IT IS PRINTED. The subdomain is +// only programmed on approval + deploy (README: "Before approval, +// https://.civit.ai/ 404s"), and `app status` already says so ("Not +// live yet — … only serves after the app is approved and deployed"). Printing it +// bare as the app's "permanent public id" walks a first-time author straight +// into a 404 they were told to expect to work. The qualifier must sit on the +// SAME LINE as the URL, or a later edit can move the URL out from under it. +func TestScaffoldDoesNotPromiseTheURLServesYet(t *testing.T) { + tmp := t.TempDir() + stdout, _, err := run(t, "app", "create", "my-block", "--dir", filepath.Join(tmp, "out"), "--template", "static") + if err != nil { + t.Fatalf("app create: %v\n%s", err, stdout) + } + + var urlLine string + for _, line := range strings.Split(stdout, "\n") { + if strings.Contains(line, ".civit.ai/") { + urlLine = line + break + } + } + if urlLine == "" { + t.Fatalf("expected the scaffold to echo a serving URL:\n%s", stdout) + } + // The wording deliberately matches `app status`'s "approved and deployed", + // so the two surfaces tell the author the same thing (app_status.go). + if !strings.Contains(urlLine, "approved and deployed") { + t.Errorf("the serving URL must be qualified on its own line — it 404s until approval:\n%s", urlLine) + } + // And the blockId is still labelled, on a line of its own. + if !strings.Contains(stdout, "blockId: my-block") { + t.Errorf("output must still LABEL the blockId:\n%s", stdout) + } +} + +// promptRecorder captures what the scaffold asked the interactive form for. +type promptRecorder struct { + calls int + askedName bool + defaultTemplate string +} + +func stubPrompt(t *testing.T, rec *promptRecorder, reply scaffoldInputs) { + t.Helper() + origTTY, origPrompt := stdinIsTTY, scaffoldPromptFn + t.Cleanup(func() { stdinIsTTY = origTTY; scaffoldPromptFn = origPrompt }) + stdinIsTTY = func() bool { return true } + scaffoldPromptFn = func(_ *cobra.Command, defaultTemplate string, askName bool) (scaffoldInputs, error) { + rec.calls++ + rec.askedName = askName + rec.defaultTemplate = defaultTemplate + return reply, nil + } +} + +// TestSlugFlagDropsTheNameFieldButKeepsTheTemplatePrompt — the regression this +// PR introduced and this test closes. +// +// 🔴 `--slug` USED TO SUPPRESS THE WHOLE PROMPT, on the reasoning that it +// "supplies the one thing the prompt exists to collect". `runScaffoldForm` +// collects a name AND a TEMPLATE, so a TTY user running +// `civitai app create --slug my-app` silently got page-money with no template +// choice — a question they were asked before the flag existed. The mutant +// deleting `slugFlag == ""` from that guard survived with ZERO failures because +// nothing covered the suppression at all. +// +// The assertions are structural, not "some prompt happened": the prompt must be +// CALLED, it must be told NOT to ask for a name, and the template it returns +// must be the one the project is built from — which is only observable because +// the reply (page-vite) differs from `app create`'s default (page-money). +func TestSlugFlagDropsTheNameFieldButKeepsTheTemplatePrompt(t *testing.T) { + var rec promptRecorder + stubPrompt(t, &rec, scaffoldInputs{name: "", template: "page-vite"}) + + tmp := t.TempDir() + chdir(t, tmp) + stdout, _, err := run(t, "app", "create", "--slug", "solo-block") + if err != nil { + t.Fatalf("app create --slug on a TTY: %v\n%s", err, stdout) + } + + if rec.calls != 1 { + t.Fatalf("--slug must NOT suppress the prompt: it was called %d time(s), want 1", rec.calls) + } + if rec.askedName { + t.Error("--slug settles the identity, so the NAME field must be dropped from the form") + } + if rec.defaultTemplate != "page-money" { + t.Errorf("the form should be pre-selected with the command's default template, got %q", rec.defaultTemplate) + } + // The prompted template is what got built — the whole point of still asking. + if !strings.Contains(stdout, "(page-vite)") { + t.Errorf("the template chosen at the prompt must be the one scaffolded:\n%s", stdout) + } + if got := readBlockID(t, filepath.Join(tmp, "solo-block")); got != "solo-block" { + t.Errorf("blockId = %q, want solo-block", got) + } +} + +// TestNoSlugFlagStillAsksForTheName is the other direction, and it is what stops +// the test above from being satisfied by a form that never asks for a name at +// all. Without --slug there is no identity yet, so both fields must be collected. +func TestNoSlugFlagStillAsksForTheName(t *testing.T) { + var rec promptRecorder + stubPrompt(t, &rec, scaffoldInputs{name: "prompted-block", template: "static"}) + + tmp := t.TempDir() + chdir(t, tmp) + if stdout, _, err := run(t, "app", "create"); err != nil { + t.Fatalf("app create on a TTY: %v\n%s", err, stdout) + } + if rec.calls != 1 { + t.Fatalf("the prompt must run when no name and no --slug are supplied (calls=%d)", rec.calls) + } + if !rec.askedName { + t.Error("with no --slug the form still has to collect the name") + } +} + +// TestScaffoldRefusesAnInvalidUTF8Name — the manifest half of the silent-loss +// class #259 is about. `civitai app create $'caf\xe9 app'` used to derive the +// blockId `caf-app` with rc 0 AND write a block.manifest.json holding the raw +// 0xE9, which is not valid UTF-8 and which `validate.ManifestOnly` accepted. +// scaffold.Slugify now refuses the derivation; this pins the other route in, +// because --slug bypasses derivation entirely and the name still lands in the +// manifest verbatim. +// +// Classification is asserted with errors.Is, never message text (AGENTS item 7). +func TestScaffoldRefusesAnInvalidUTF8Name(t *testing.T) { + const badName = "caf\xe9 app" + if utf8.ValidString(badName) { + t.Fatal("fixture is broken: badName must NOT be valid UTF-8") + } + + t.Run("as the positional name, with --slug", func(t *testing.T) { + tmp := t.TempDir() + dest := filepath.Join(tmp, "out") + _, _, err := run(t, "app", "create", badName, "--slug", "cafe-app", "--dir", dest, "--template", "static") + if err == nil { + t.Fatal("an invalid-UTF-8 display name must be refused") + } + if !errors.Is(err, ErrUsage) { + t.Errorf("a bad name VALUE is a usage error: %v", err) + } + if _, statErr := os.Stat(dest); statErr == nil { + t.Error("refused, but a project was still written") + } + }) + + t.Run("as --name", func(t *testing.T) { + tmp := t.TempDir() + dest := filepath.Join(tmp, "out") + _, _, err := run(t, "app", "create", "my-block", "--name", badName, "--dir", dest, "--template", "static") + if err == nil { + t.Fatal("an invalid-UTF-8 --name must be refused") + } + if !errors.Is(err, ErrUsage) { + t.Errorf("a bad --name VALUE is a usage error: %v", err) + } + }) + + t.Run("derivation refuses it too, with no --slug", func(t *testing.T) { + tmp := t.TempDir() + _, _, err := run(t, "app", "create", badName, "--dir", filepath.Join(tmp, "out"), "--template", "static") + if err == nil { + t.Fatal("invalid UTF-8 must not derive a slug") + } + if !errors.Is(err, ErrUsage) { + t.Errorf("a bad name VALUE is a usage error: %v", err) + } + }) + + // POSITIVE CONTROL: the same shape with VALID UTF-8 succeeds and writes a + // manifest that is valid UTF-8 — so the rows above are evidence about the + // bytes, not about a build that refuses everything. + t.Run("a valid non-ASCII name still scaffolds with --slug", func(t *testing.T) { + tmp := t.TempDir() + dest := filepath.Join(tmp, "out") + if _, _, err := run(t, "app", "create", "Café App", "--slug", "cafe-app", "--dir", dest, "--template", "static"); err != nil { + t.Fatalf("a valid UTF-8 name must still work: %v", err) + } + b, readErr := os.ReadFile(filepath.Join(dest, "block.manifest.json")) + if readErr != nil { + t.Fatalf("read manifest: %v", readErr) + } + if !utf8.Valid(b) { + t.Error("the written manifest must be valid UTF-8") + } + }) +} diff --git a/internal/cmd/app_init_identity_test.go b/internal/cmd/app_init_identity_test.go new file mode 100644 index 0000000..b240542 --- /dev/null +++ b/internal/cmd/app_init_identity_test.go @@ -0,0 +1,332 @@ +package cmd + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// manifestBlockID / manifestName read what the scaffold actually WROTE. The +// manifest is the authority on the app's identity; the printed line is a claim +// ABOUT it, so the tests below assert both and never infer one from the other. +// (readBlockID already exists in app_dev_token_rename_test.go — reused here.) +func manifestBlockID(t *testing.T, dir string) string { + t.Helper() + got := readBlockID(t, dir) + if got == "" { + t.Fatalf("manifest in %s has no blockId", dir) + } + return got +} + +func manifestName(t *testing.T, dir string) string { + t.Helper() + b, err := os.ReadFile(filepath.Join(dir, "block.manifest.json")) + if err != nil { + t.Fatalf("read manifest: %v", err) + } + var m struct { + Name string `json:"name"` + } + if err := json.Unmarshal(b, &m); err != nil { + t.Fatalf("decode manifest: %v", err) + } + return m.Name +} + +// TestScaffoldEchoesTheDerivedSlugWithADir is the half of issue #259 that makes +// the derivation bug INVISIBLE. `printScaffoldResult` took `slug` and never used +// it — a dead parameter — so no line of output named the blockId. With the +// default directory you can infer it from the directory name; with --dir the +// only copy is inside block.manifest.json. +// +// The assertion is deliberately not "the output contains the slug somewhere": +// the slug here (`ueber-app`) shares no substring with the display name or the +// directory, so a green cannot be earned by an unrelated line. +func TestScaffoldEchoesTheDerivedSlugWithADir(t *testing.T) { + tmp := t.TempDir() + dest := filepath.Join(tmp, "somewhere-else") + + stdout, _, err := run(t, "app", "create", "Uber App", "--slug", "ueber-app", "--dir", dest, "--template", "static") + if err != nil { + t.Fatalf("app create: %v\n%s", err, stdout) + } + if got := manifestBlockID(t, dest); got != "ueber-app" { + t.Fatalf("manifest blockId = %q, want ueber-app", got) + } + if !strings.Contains(stdout, "ueber-app") { + t.Errorf("output must echo the blockId (the app's permanent public id):\n%s", stdout) + } + if !strings.Contains(stdout, "blockId") { + t.Errorf("output must LABEL the id, not just happen to contain the string:\n%s", stdout) + } + // The directory name is the one place the slug is normally readable; here it + // is not, which is what makes this the load-bearing case. + if strings.Contains(filepath.Base(dest), "ueber-app") { + t.Fatal("fixture is broken: the --dir name must NOT contain the slug") + } +} + +// TestScaffoldEchoesTheSlugWithTheDefaultDir is the same claim for the default +// directory. It is a weaker observation on its own (the directory name is the +// slug), so it asserts the LABEL too. +func TestScaffoldEchoesTheSlugWithTheDefaultDir(t *testing.T) { + tmp := t.TempDir() + chdir(t, tmp) + + stdout, _, err := run(t, "app", "create", "My Cool Block", "--template", "static") + if err != nil { + t.Fatalf("app create: %v\n%s", err, stdout) + } + if got := manifestBlockID(t, filepath.Join(tmp, "my-cool-block")); got != "my-cool-block" { + t.Fatalf("manifest blockId = %q, want my-cool-block", got) + } + if !strings.Contains(stdout, "blockId: my-cool-block") { + t.Errorf("output must echo the blockId even with the default dir:\n%s", stdout) + } +} + +// TestSlugFlagIsTheEscapeHatchForARefusedName is the reason the refusal in +// scaffold.Slugify is shippable at all: "Café App" no longer derives (it used to +// silently mint `caf-app`), and --slug is how the author says what they want +// instead. +func TestSlugFlagIsTheEscapeHatchForARefusedName(t *testing.T) { + tmp := t.TempDir() + dest := filepath.Join(tmp, "out") + + // Without --slug the name is refused, as a usage error (exit 2). + if _, _, err := run(t, "app", "create", "Café App", "--dir", filepath.Join(tmp, "refused"), "--template", "static"); err == nil { + t.Fatal("expected a non-slugifiable name to be refused") + } else if !errors.Is(err, ErrUsage) { + t.Errorf("a refused name is a usage error: errors.Is(err, ErrUsage) = false (%v)", err) + } + + // With --slug it succeeds and the manifest carries the explicit slug. + stdout, _, err := run(t, "app", "create", "Café App", "--slug", "cafe-app", "--dir", dest, "--template", "static") + if err != nil { + t.Fatalf("app create with --slug: %v\n%s", err, stdout) + } + if got := manifestBlockID(t, dest); got != "cafe-app" { + t.Errorf("manifest blockId = %q, want cafe-app", got) + } + if got := manifestName(t, dest); got != "Café App" { + t.Errorf("display name = %q, want the name the author typed (Café App)", got) + } + if !strings.Contains(stdout, "cafe-app") { + t.Errorf("output must echo the explicit blockId too:\n%s", stdout) + } +} + +// TestSlugFlagRejectsAnInvalidValue: --slug bypasses derivation, so it is the +// only thing standing between a typo and an invalid manifest. It goes through +// the same ValidateSlug the server contract is written from, and a bad VALUE is +// a usage error — pinned with errors.Is, never message text (AGENTS item 7). +func TestSlugFlagRejectsAnInvalidValue(t *testing.T) { + for _, bad := range []string{ + "Cafe-App", // uppercase + "1st-app", // must start with a letter + "ab", // under the 3-char floor + "cafe app", // space + "cafe-", // must end alphanumeric + strings.Repeat("a", 41), // over the 40-char cap + } { + t.Run(bad, func(t *testing.T) { + tmp := t.TempDir() + _, _, err := run(t, "app", "create", "my-block", "--slug", bad, "--dir", filepath.Join(tmp, "out"), "--template", "static") + if err == nil { + t.Fatalf("--slug %q should be refused", bad) + } + if !errors.Is(err, ErrUsage) { + t.Errorf("--slug %q must be a usage error: %v", bad, err) + } + if _, statErr := os.Stat(filepath.Join(tmp, "out")); statErr == nil { + t.Errorf("--slug %q was refused but a project was still written", bad) + } + }) + } +} + +// TestNameSlugAndDirAreFullyIndependent. The help text has promised name/dir +// independence for a while; --slug makes the third axis real. All three values +// are pairwise distinct AND share no substring, so no assertion here can be +// satisfied by the wrong one. +func TestNameSlugAndDirAreFullyIndependent(t *testing.T) { + tmp := t.TempDir() + dest := filepath.Join(tmp, "zzz-output-dir") + + stdout, _, err := run(t, "app", "create", "ignored-positional", + "--name", "Widget Machine", + "--slug", "qqq-block-id", + "--dir", dest, + "--template", "static") + if err != nil { + t.Fatalf("app create: %v\n%s", err, stdout) + } + if got := manifestBlockID(t, dest); got != "qqq-block-id" { + t.Errorf("blockId = %q, want qqq-block-id", got) + } + if got := manifestName(t, dest); got != "Widget Machine" { + t.Errorf("display name = %q, want Widget Machine", got) + } + if _, err := os.Stat(filepath.Join(dest, "block.manifest.json")); err != nil { + t.Errorf("project should be written to --dir: %v", err) + } + if _, err := os.Stat(filepath.Join(tmp, "qqq-block-id")); err == nil { + t.Error("--dir was given, so nothing may be written to ./") + } + for _, want := range []string{"qqq-block-id", "Widget Machine", "zzz-output-dir"} { + if !strings.Contains(stdout, want) { + t.Errorf("output should name all three of slug/display/dir; missing %q:\n%s", want, stdout) + } + } +} + +// TestSlugFlagAloneNeedsNoName — the axes really are independent: --slug on its +// own is enough to scaffold, with the display name title-cased from it. +func TestSlugFlagAloneNeedsNoName(t *testing.T) { + tmp := t.TempDir() + chdir(t, tmp) + + stdout, _, err := run(t, "app", "create", "--slug", "solo-block", "--template", "static") + if err != nil { + t.Fatalf("app create --slug (no name): %v\n%s", err, stdout) + } + if got := manifestBlockID(t, filepath.Join(tmp, "solo-block")); got != "solo-block" { + t.Errorf("blockId = %q, want solo-block", got) + } + if got := manifestName(t, filepath.Join(tmp, "solo-block")); got != "Solo Block" { + t.Errorf("display name = %q, want Solo Block (title-cased from the slug)", got) + } +} + +// TestAsciiDerivationIsUnchanged is the CONTROL for the refusal: every name that +// derived a slug before must derive the byte-identical slug now, through the +// real command. These rows are expected to have been green before the change — +// they are an invariant guard, not regression coverage, and their job is to fail +// if the refusal predicate ever widens to swallow ordinary punctuation. +func TestAsciiDerivationIsUnchanged(t *testing.T) { + cases := map[string]string{ + "My Cool Block": "my-cool-block", + "my-block": "my-block", + " Spaced Out ": "spaced-out", + "Foo/Bar_Baz.Qux!": "foo-bar-baz-qux", + "Widget (v2) — Pro": "widget-v2-pro", // an em dash is a separator, not content + "a & b + c = d": "a-b-c-d", + "Trailing punctuation.": "trailing-punctuation", + } + for name, want := range cases { + t.Run(name, func(t *testing.T) { + tmp := t.TempDir() + dest := filepath.Join(tmp, "out") + stdout, _, err := run(t, "app", "create", name, "--dir", dest, "--template", "static") + if err != nil { + t.Fatalf("app create %q: %v\n%s", name, err, stdout) + } + if got := manifestBlockID(t, dest); got != want { + t.Errorf("blockId for %q = %q, want %q", name, got, want) + } + }) + } +} + +// TestScaffoldHelpDoesNotPromiseAValidateCleanScaffold — issue #260 item 1. +// `app create --help` said the scaffold "validates clean"; a fresh page-money / +// page-vite project FAILS `civitai app validate` until `npm install` writes the +// lockfile the platform build installs from. The check is right; the promise was +// wrong. +func TestScaffoldHelpDoesNotPromiseAValidateCleanScaffold(t *testing.T) { + help, _, err := run(t, "app", "create", "--help") + if err != nil { + t.Fatalf("app create --help: %v", err) + } + if strings.Contains(help, "validates clean") { + t.Errorf("help must not claim the scaffold `validates clean` — it does not until `npm install` runs:\n%s", help) + } + // And it must say what the real precondition is, rather than going silent. + for _, want := range []string{"civitai app validate", "npm install"} { + if !strings.Contains(help, want) { + t.Errorf("help should name the validate/install relationship; missing %q:\n%s", want, help) + } + } +} + +// TestNextStepsSayValidateNeedsInstall — the same fact at the other surface the +// author actually reads: the numbered next-steps block printed right after the +// scaffold is written. The `static` template installs nothing and DOES validate +// clean, so it must NOT carry the caveat; that pair is what stops the assertion +// from passing on a line printed unconditionally. +func TestNextStepsSayValidateNeedsInstall(t *testing.T) { + t.Run("page-money warns", func(t *testing.T) { + tmp := t.TempDir() + stdout, _, err := run(t, "app", "create", "my-block", "--dir", filepath.Join(tmp, "out")) + if err != nil { + t.Fatalf("app create: %v\n%s", err, stdout) + } + if !strings.Contains(stdout, "npm install") { + t.Fatalf("expected an npm install step:\n%s", stdout) + } + if !strings.Contains(stdout, "civitai app validate` fails until you do") { + t.Errorf("next steps must say validate fails until npm install has run:\n%s", stdout) + } + }) + t.Run("page-vite warns", func(t *testing.T) { + tmp := t.TempDir() + stdout, _, err := run(t, "app", "create", "my-block", "--dir", filepath.Join(tmp, "out"), "--template", "page-vite") + if err != nil { + t.Fatalf("app create: %v\n%s", err, stdout) + } + if !strings.Contains(stdout, "civitai app validate` fails until you do") { + t.Errorf("next steps must say validate fails until npm install has run:\n%s", stdout) + } + }) + t.Run("static does not (it really does validate clean)", func(t *testing.T) { + tmp := t.TempDir() + stdout, _, err := run(t, "app", "create", "my-block", "--dir", filepath.Join(tmp, "out"), "--template", "static") + if err != nil { + t.Fatalf("app create: %v\n%s", err, stdout) + } + if strings.Contains(stdout, "fails until you do") { + t.Errorf("static installs nothing and validates clean — it must not carry the caveat:\n%s", stdout) + } + }) +} + +// TestScaffoldFromErrorShipsNoEngineeringNote — issue #260 item 6. `--from` used +// to answer with a multi-line internal note ("TODO(server): expose a read +// endpoint …") that buried the one useful sentence. The context belongs in a +// source comment; the user gets an ordinary actionable CLI error. +func TestScaffoldFromErrorShipsNoEngineeringNote(t *testing.T) { + for _, sub := range []string{"create", "init"} { + t.Run(sub, func(t *testing.T) { + tmp := t.TempDir() + _, errOut, err := run(t, "app", sub, "forked-app", "--from", "some-published-app", "--dir", filepath.Join(tmp, "out")) + if err == nil { + t.Fatal("expected --from to fail") + } + blob := err.Error() + errOut + for _, leaked := range []string{"TODO(", "TODO", "endpoint that returns"} { + if strings.Contains(blob, leaked) { + t.Errorf("user-facing error leaks an engineering note (%q):\n%s", leaked, blob) + } + } + if !strings.Contains(blob, "--from is not available yet") { + t.Errorf("error should say plainly that the flag is unavailable:\n%s", blob) + } + // Actionable: it names what to do instead, with the real command path. + if !strings.Contains(blob, "civitai app "+sub+" ") { + t.Errorf("error should name the command to run instead:\n%s", blob) + } + if strings.Count(blob, "\n") > 1 { + t.Errorf("error should be a single line, not a multi-line note:\n%s", blob) + } + // Unchanged classification: an unavailable feature is not a + // malformed invocation, so it stays generic (exit 1), not exit 2. + if errors.Is(err, ErrUsage) { + t.Error("--from is a real flag with a real value: it must not be tagged ErrUsage") + } + }) + } +} diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 5b56ddf..4180bfe 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -268,6 +268,9 @@ func TestAppInitRequiresName(t *testing.T) { } } +// TestAppInitFromIsNotWired pins the `--from` refusal for `app init`; the +// message contract itself (no internal TODO, an actionable next command) is +// asserted for BOTH commands in TestScaffoldFromErrorShipsNoEngineeringNote. func TestAppInitFromIsNotWired(t *testing.T) { tmp := t.TempDir() chdir(t, tmp) @@ -275,8 +278,8 @@ func TestAppInitFromIsNotWired(t *testing.T) { if err == nil { t.Fatal("expected --from to be reported as not wired") } - if !strings.Contains(err.Error(), "not yet wired") { - t.Errorf("error should say not yet wired: %v", err) + if !strings.Contains(err.Error(), "--from is not available yet") { + t.Errorf("error should say --from is not available yet: %v", err) } } diff --git a/internal/cmd/scaffold_prompt_test.go b/internal/cmd/scaffold_prompt_test.go index e51f64b..701a795 100644 --- a/internal/cmd/scaffold_prompt_test.go +++ b/internal/cmd/scaffold_prompt_test.go @@ -24,7 +24,7 @@ func TestMain(m *testing.M) { func TestScaffoldNonInteractiveNoPrompt(t *testing.T) { orig := scaffoldPromptFn t.Cleanup(func() { scaffoldPromptFn = orig }) - scaffoldPromptFn = func(_ *cobra.Command, _ string) (scaffoldInputs, error) { + scaffoldPromptFn = func(_ *cobra.Command, _ string, _ bool) (scaffoldInputs, error) { t.Fatal("huh prompt must NOT run on a non-TTY stdin") return scaffoldInputs{}, nil } @@ -44,7 +44,7 @@ func TestScaffoldYesSkipsPromptEvenOnTTY(t *testing.T) { t.Cleanup(func() { stdinIsTTY = origTTY; scaffoldPromptFn = origPrompt }) stdinIsTTY = func() bool { return true } - scaffoldPromptFn = func(_ *cobra.Command, _ string) (scaffoldInputs, error) { + scaffoldPromptFn = func(_ *cobra.Command, _ string, _ bool) (scaffoldInputs, error) { t.Fatal("huh prompt must NOT run when --yes is set") return scaffoldInputs{}, nil } @@ -65,7 +65,7 @@ func TestScaffoldInteractiveUsesPromptValues(t *testing.T) { t.Cleanup(func() { stdinIsTTY = origTTY; scaffoldPromptFn = origPrompt }) stdinIsTTY = func() bool { return true } - scaffoldPromptFn = func(_ *cobra.Command, _ string) (scaffoldInputs, error) { + scaffoldPromptFn = func(_ *cobra.Command, _ string, _ bool) (scaffoldInputs, error) { return scaffoldInputs{name: "prompted-block", template: "static"}, nil } @@ -91,7 +91,7 @@ func TestScaffoldNameArgSkipsPrompt(t *testing.T) { t.Cleanup(func() { stdinIsTTY = origTTY; scaffoldPromptFn = origPrompt }) stdinIsTTY = func() bool { return true } - scaffoldPromptFn = func(_ *cobra.Command, _ string) (scaffoldInputs, error) { + scaffoldPromptFn = func(_ *cobra.Command, _ string, _ bool) (scaffoldInputs, error) { t.Fatal("huh prompt must NOT run when a name is supplied") return scaffoldInputs{}, nil } diff --git a/internal/scaffold/slug.go b/internal/scaffold/slug.go index 458e80e..fa8fdb2 100644 --- a/internal/scaffold/slug.go +++ b/internal/scaffold/slug.go @@ -5,6 +5,8 @@ import ( "fmt" "regexp" "strings" + "unicode" + "unicode/utf8" ) // slugSuffixAlphabet is lowercase-alphanumeric only (no hyphens) so a generated @@ -66,9 +68,177 @@ var slugPattern = regexp.MustCompile(`^[a-z][a-z0-9-]*[a-z0-9]$`) var nonSlugChars = regexp.MustCompile(`[^a-z0-9]+`) +// isCombiningMark reports whether r is a Unicode Mark (Mn/Mc/Me) — a character +// that has no standalone appearance and renders on top of the one before it. +func isCombiningMark(r rune) bool { return unicode.Is(unicode.M, r) } + +// isLossyBase classifies ONE base rune: true when derivation would DROP it +// rather than fold it into a hyphen. +// +// 🔴 THE CLASSIFICATION IS ON THE LOWERED RUNE AND THE REPORT IS ON THE ORIGINAL +// — the two must not be collapsed. Derivation lowercases first, so lowering is +// what decides whether a rune survives; but a message that quotes the LOWERED +// rune names a character the user never typed. Measured on the first version of +// this code, which classified and reported off `strings.ToLower(name)`: +// `"ẞE App"` reported `"ß"`, a rune ABSENT from the input, and `"ＡＢＣ"` +// reported `"ａ", "ｂ", "ｃ"`. Someone searching their own name for the quoted +// character finds nothing. +// +// 🔴 The rule itself is the ASYMMETRY between the two things a non-`[a-z0-9]` +// rune can be. A SEPARATOR (space, `_`, `.`, `/`, `-`, `!`, `&`, an em dash …) +// carries no information of its own: turning it into a hyphen is what the author +// meant, and that is why `"My Cool Block"` → `my-cool-block` is correct. A +// LETTER, DIGIT or MARK the ASCII slug alphabet cannot carry is the opposite: it +// is content the author typed, and there is no lossless place to put it. +// Dropping it silently mints a DIFFERENT permanent public identity — +// `"ÜberApp Ω"` → `berapp` (the leading `Ü` becomes a hyphen and is then +// trimmed), `"Café Del Mar"` → `caf-del-mar` (a spurious word boundary +// mid-name). Neither is a truncation the author can recognise, and a blockId is +// not renameable, so the caller refuses and asks for an explicit slug instead. +// +// 🔴 EVERY RUNE THAT LOWERS INTO ASCII IS EXEMPT BY CONSTRUCTION, and that is +// load-bearing rather than lazy: it is what makes the ASCII derivations that +// work today byte-identical, including the two dead ends whose existing messages +// are good (`"123 Numbers"` → starts with a digit; `"!!!"` → nothing left). It +// is also the source of the (b) exception in the Slugify header: exactly two +// runes above ASCII lower INTO ASCII. Do not "improve" the exemption into a +// character allowlist — an allowlist re-decides every ASCII derivation this CLI +// has ever produced, and the whole point of the boundary is that it decides +// none of them. +func isLossyBase(r rune) bool { + lower := unicode.ToLower(r) + if lower < utf8.RuneSelf { + // Lowers into ASCII: either a slug character or a separator. + // Byte-identical to the derivation this CLI has always done. + return false + } + // A separator with no content of its own — becomes a hyphen. + return !unicode.IsSpace(lower) && !unicode.IsPunct(lower) && !unicode.IsSymbol(lower) +} + +// dottedCircle is the Unicode convention for displaying a combining mark that +// has no base character to sit on (U+25CC DOTTED CIRCLE). +const dottedCircle = "◌" + +// LossyChars returns, in first-appearance order and deduplicated, the characters +// in name that slug derivation would DROP rather than fold into a hyphen — each +// as the printable string the AUTHOR typed, never a lowered or decomposed +// substitute. +// +// 🔴 A COMBINING MARK IS REPORTED WITH ITS BASE, because on its own it renders +// as an accent floating over nothing. macOS paths and some paste routes deliver +// NFD, so `"Café App"` arrives as `e` + U+0301 and the first version of this +// message read `"́" cannot appear in a blockId` — the same VISIBLE name yielding +// two different messages, one of them pointing at nothing the user can see. The +// cluster is what gets quoted, so NFC and NFD both report `"é"`. This changes +// only the RENDERING: a mark is non-ASCII and is neither space, punct nor +// symbol, so it was already refused, and the set of refused names is unchanged. +// A mark with no base at all (a name STARTING with one) is shown on a dotted +// circle rather than bare. +// +// Transliteration was considered and rejected: `Ω → o` vs `omega` is a +// locale-dependent judgement, a general transliterator means a new +// `golang.org/x/text` dependency, and it produces nothing usable for CJK, +// Cyrillic or Arabic — which is most of the population this refusal serves. +// +// name MUST be valid UTF-8; Slugify gates that before calling here. Ranging over +// invalid bytes yields U+FFFD, which is a Symbol and would be waved through as a +// separator. +func LossyChars(name string) []string { + runes := []rune(strings.TrimSpace(name)) + var lossy []string + seen := map[string]bool{} + add := func(s string) { + if seen[s] { + return + } + seen[s] = true + lossy = append(lossy, s) + } + for i := 0; i < len(runes); { + base := runes[i] + j := i + 1 + for j < len(runes) && isCombiningMark(runes[j]) { + j++ + } + switch { + case isCombiningMark(base): + // Only reachable at the very start of the name: every other mark is + // consumed by the base before it. + add(dottedCircle + string(runes[i:j])) + case j > i+1 || isLossyBase(base): + // Either the cluster carries a mark (content, dropped by + // derivation) or the base itself is lossy. Report the cluster. + add(string(runes[i:j])) + } + i = j + } + return lossy +} + +// maxNamedChars bounds how many offending characters the refusal spells out. A +// fully non-Latin name can carry dozens of distinct characters and an error that +// re-prints the whole name back as a quoted list stops being readable; the first +// few are enough to make the refusal recognisable, and the name itself is quoted +// in the same sentence. +const maxNamedChars = 6 + +// quoteChars renders characters as a comma-separated list of quoted characters, +// for an error that has to NAME what it is refusing. +func quoteChars(cs []string) string { + more := "" + if len(cs) > maxNamedChars { + more = fmt.Sprintf(" (and %d more)", len(cs)-maxNamedChars) + cs = cs[:maxNamedChars] + } + parts := make([]string, len(cs)) + for i, c := range cs { + parts[i] = fmt.Sprintf("%q", c) + } + return strings.Join(parts, ", ") + more +} + // Slugify converts an arbitrary name into a valid block slug, or returns an -// error if it cannot produce one within the length bounds. +// error if it cannot produce one within the length bounds — or if deriving one +// would silently DISCARD characters the author typed (see LossyChars). +// +// 🔴 THIS NARROWS ISSUE #259, IT DOES NOT CLOSE IT. Three classes of input still +// derive a slug that quietly differs from the name, and all three are stated +// here rather than implied away — a residual nobody writes down is +// indistinguishable from a bug nobody noticed: +// +// - (a) INVALID UTF-8 — CLOSED. `caf\xe9 app` used to derive `caf-app` with +// rc 0, because `for _, r := range` yields U+FFFD per bad byte and U+FFFD is +// a Symbol, i.e. the separator branch. Worse, the manifest the scaffold then +// wrote held the raw `0xE9` and was not valid UTF-8 — and `validate` accepted +// it. The `utf8.ValidString` guard below refuses it. (The manifest half is +// closed at the call site, which also refuses an invalid-UTF-8 DISPLAY name; +// `internal/validate` still has no UTF-8 check of its own.) +// - (b) THE TWO RUNES ABOVE ASCII THAT LOWER INTO ASCII — a deliberate, +// ENUMERATED exception, not an oversight. All of Unicode was walked: exactly +// two exist, `İ` U+0130 → `i` and `K` U+212A KELVIN SIGN → `k`. So +// `"İstanbul App"` derives `istanbul-app` with rc 0. That is arguably the +// nicest transliteration available and it costs nothing, so it stays — but +// it means the exemption below is "lowers into ASCII", never "is ASCII", +// and any absolute claim that every non-ASCII letter is refused is false. +// - (c) SYMBOLS, EMOJI AND NON-ASCII PUNCTUATION fold to a hyphen, so +// `"Rocket 🚀 App"` derives `rocket-app` with rc 0. Census over printable +// non-ASCII runes: 8,580 take the separator branch, 140,321 the refuse +// branch. This is the asymmetry working as designed for `—` and `»`, and +// arguably NOT what an author means by an emoji — but an emoji has no +// lossless ASCII form either, so refusing would only trade a silent drop for +// a dead end. Left as-is deliberately, tracked separately. func Slugify(name string) (string, error) { + // 🔴 BEFORE anything ranges over the string. `for _, r := range` silently + // substitutes U+FFFD for each invalid byte, and U+FFFD classifies as a + // Symbol — the separator branch — so a mojibake name derived a slug that + // dropped the bad bytes AND wrote them raw into block.manifest.json. + if !utf8.ValidString(name) { + return "", fmt.Errorf("cannot derive a slug from %q: the name is not valid UTF-8, so the characters it holds cannot be read (a blockId is lowercase a-z, 0-9 and hyphens only), and deriving one anyway would give your app a different permanent public id than you typed — choose one yourself with --slug ", name) + } + if lossy := LossyChars(name); len(lossy) > 0 { + return "", fmt.Errorf("cannot derive a slug from %q: %s cannot appear in a blockId (lowercase a-z, 0-9 and hyphens only), and dropping them would give your app a different permanent public id than you typed — choose one yourself with --slug ", name, quoteChars(lossy)) + } s := strings.ToLower(strings.TrimSpace(name)) s = nonSlugChars.ReplaceAllString(s, "-") s = strings.Trim(s, "-") diff --git a/internal/scaffold/slug_lossy_test.go b/internal/scaffold/slug_lossy_test.go new file mode 100644 index 0000000..e62b93c --- /dev/null +++ b/internal/scaffold/slug_lossy_test.go @@ -0,0 +1,494 @@ +package scaffold + +import ( + "fmt" + "regexp" + "strings" + "testing" + "unicode" + "unicode/utf8" +) + +// runesThatLowerIntoASCII walks ALL of Unicode for runes above ASCII whose +// unicode.ToLower lands back inside ASCII. It is a walk rather than a literal +// pair so the "exactly two" claim in Slugify's header tracks the Go unicode +// tables instead of somebody's memory of them. +func runesThatLowerIntoASCII() []rune { + var out []rune + for r := rune(utf8.RuneSelf); r <= unicode.MaxRune; r++ { + if !utf8.ValidRune(r) { + continue + } + if unicode.ToLower(r) < utf8.RuneSelf { + out = append(out, r) + } + } + return out +} + +// legacySlugify is the derivation EXACTLY as it stood before the refusal landed +// (`e800129`-era `Slugify`, minus the two new guards). It exists so a +// `mustNotProduce` row is a MEASUREMENT of the old behaviour rather than a +// remembered string: a row naming an output the old algorithm never produced +// would be evidence about nothing, and nothing else in the suite could tell. +func legacySlugify(name string) (string, error) { + s := strings.ToLower(strings.TrimSpace(name)) + s = regexp.MustCompile(`[^a-z0-9]+`).ReplaceAllString(s, "-") + s = strings.Trim(s, "-") + for strings.Contains(s, "--") { + s = strings.ReplaceAll(s, "--", "-") + } + if len(s) < 3 { + return "", fmt.Errorf("cannot derive a valid slug from %q (need ≥3 chars)", name) + } + if len(s) > 40 { + s = strings.Trim(s[:40], "-") + } + if !slugPattern.MatchString(s) { + return "", fmt.Errorf("derived slug %q is invalid", s) + } + return s, nil +} + +// TestSlugifyRefusesRatherThanDroppingCharacters is the headline of issue #259. +// +// 🔴 EACH ROW PINS THE OLD OUTPUT IT MUST NOT PRODUCE, not merely "an error". +// "an error came back" is satisfied by a refusal for the wrong reason and by a +// refusal that happens to fire everywhere; naming `berapp` / `caf-del-mar` is +// what makes the row evidence about THIS defect. +// +// 🔴 AND THE ASSERTION HAS TO EXECUTE. The first version put that check behind a +// `t.Fatalf("want a refusal")` that had already aborted the subtest whenever +// `err == nil`, so the `mustNotProduce` block was UNREACHABLE — measured: +// deleting it left the package green while a positive control reddened 1, i.e. +// the harness could go red and this assertion simply never ran. The PR body's +// headline claim rested on it. It now lives INSIDE the `err == nil` branch, +// where it is the only thing that can distinguish "derivation came back" from +// "derivation came back with the exact #259 mangling". +func TestSlugifyRefusesRatherThanDroppingCharacters(t *testing.T) { + cases := []struct { + name string + // mustNotProduce is what Slugify returned BEFORE the fix — a silently + // different permanent public identity. "" means the pre-fix derivation + // bottomed out in one of the two ASCII dead ends instead. + mustNotProduce string + // chars are the characters the refusal has to point at, AS TYPED. + chars []string + }{ + { + // The leading Ü becomes a hyphen and is then trimmed, so the app's + // public id loses its first letter outright. + name: "ÜberApp Ω", + mustNotProduce: "berapp", + chars: []string{"Ü", "Ω"}, + }, + { + // Worse than dropping: mid-string it INSERTS a word boundary that + // the author never typed. + name: "Café Del Mar", + mustNotProduce: "caf-del-mar", + chars: []string{"é"}, + }, + { + // 🔴 THE ROW THAT PINS "REPORT THE ORIGINAL, NOT THE LOWERED RUNE". + // U+1E9E LATIN CAPITAL LETTER SHARP S lowers to U+00DF ß — a + // DIFFERENT character, and one absent from this input. The first + // version classified and reported off strings.ToLower(name) and so + // quoted "ß" at a user who never typed it. + name: "\u1e9eE App", + mustNotProduce: "e-app", + chars: []string{"\u1e9e"}, + }, + { + // Same class, fullwidth: Ａ lowers to ａ, so the pre-fix message + // named "ａ", "ｂ", "ｃ" instead of what was typed. + name: "\uff21\uff22\uff23 Widget", + mustNotProduce: "widget", + chars: []string{"\uff21", "\uff22", "\uff23"}, + }, + { + name: "日本語アプリ", + mustNotProduce: "", + chars: []string{"日", "本", "語"}, + }, + { + name: "Приложение", + mustNotProduce: "", + chars: []string{"П", "р"}, + }, + { + name: "تطبيق", + mustNotProduce: "", + chars: []string{"ت", "ط"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Positive control on the FIXTURE: mustNotProduce must be what the + // pre-fix derivation really produced, or the row below is asserting + // against a string nothing ever emitted. + legacy, legacyErr := legacySlugify(tc.name) + if tc.mustNotProduce == "" { + if legacyErr == nil { + t.Fatalf("fixture is broken: the pre-fix derivation produced %q for %q, so mustNotProduce must name it", legacy, tc.name) + } + } else if legacyErr != nil || legacy != tc.mustNotProduce { + t.Fatalf("fixture is broken: the pre-fix derivation gave (%q, %v) for %q, want %q", legacy, legacyErr, tc.name, tc.mustNotProduce) + } + + got, err := Slugify(tc.name) + if err == nil { + // 🔴 This is the reachable form of "the pre-fix output must be + // unreachable". A build that reintroduces the mangling fails + // HERE, naming it, rather than in the generic branch below. + if tc.mustNotProduce != "" && got == tc.mustNotProduce { + t.Fatalf("Slugify(%q) = %q — that is the exact pre-fix mangling issue #259 is about, a different permanent public id than the author typed", tc.name, got) + } + t.Fatalf("Slugify(%q) = %q, want a refusal", tc.name, got) + } + if got != "" { + t.Errorf("a refused Slugify must return no slug, got %q", got) + } + msg := err.Error() + for _, c := range tc.chars { + if !strings.Contains(msg, c) { + t.Errorf("refusal must NAME the offending character %q AS TYPED: %s", c, msg) + } + } + // 🔴 The refusal quotes the INPUT. Nothing asserted this, so a + // mutant replacing the interpolated name with a fixed literal + // survived — an error naming somebody else's name is worse than one + // naming none. + if want := fmt.Sprintf("%q", tc.name); !strings.Contains(msg, want) { + t.Errorf("refusal must quote the name the author typed (%s): %s", want, msg) + } + // Actionable: it points at the escape hatch, and states the rule. + if !strings.Contains(msg, "--slug") { + t.Errorf("refusal must point at --slug: %s", msg) + } + if !strings.Contains(msg, "a-z") || !strings.Contains(msg, "hyphens") { + t.Errorf("refusal must state the blockId constraint: %s", msg) + } + }) + } +} + +// TestSlugifyReportsTheCharacterTheAuthorTyped is the same claim as the "ẞE App" +// row above, stated directly against LossyChars so it cannot be satisfied by any +// other part of the refusal sentence. Each row's lowered form is a character +// that is NOT in the input — that is what makes the row discriminating. +func TestSlugifyReportsTheCharacterTheAuthorTyped(t *testing.T) { + cases := []struct { + in string + want []string + mustNotReport []string + }{ + {in: "\u1e9eE App", want: []string{"\u1e9e"}, mustNotReport: []string{"\u00df"}}, + {in: "\uff21\uff22\uff23", want: []string{"\uff21", "\uff22", "\uff23"}, mustNotReport: []string{"\uff41", "\uff42", "\uff43"}}, + {in: "ÜberApp Ω", want: []string{"Ü", "Ω"}, mustNotReport: []string{"ü", "ω"}}, + } + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + got := strings.Join(LossyChars(tc.in), "") + for _, w := range tc.want { + if !strings.Contains(got, w) { + t.Errorf("LossyChars(%q) = %q, must report the typed %q", tc.in, got, w) + } + } + for _, bad := range tc.mustNotReport { + if strings.Contains(got, bad) { + t.Errorf("LossyChars(%q) = %q — %q is the LOWERED form and does not appear in the input", tc.in, got, bad) + } + } + // And the same through the real message, so the fix cannot be + // correct in the predicate and lost at the printer. + _, err := Slugify(tc.in) + if err == nil { + t.Fatalf("Slugify(%q) should refuse", tc.in) + } + for _, bad := range tc.mustNotReport { + if strings.Contains(err.Error(), bad) { + t.Errorf("refusal names the lowered %q, absent from the input: %v", bad, err) + } + } + }) + } +} + +// TestSlugifyRefusesInvalidUTF8 — residual (a) of the Slugify header, now +// closed. `for _, r := range` yields U+FFFD per invalid byte and U+FFFD is a +// Symbol, i.e. the SEPARATOR branch, so mojibake derived a slug with rc 0 AND +// the scaffold wrote the raw bytes into block.manifest.json. +func TestSlugifyRefusesInvalidUTF8(t *testing.T) { + // Measured pre-fix outputs, i.e. exactly the #259 shape. + cases := map[string]string{ + "caf\xe9 app": "caf-app", + "\xff\xfeWidget": "widget", + "Sm\xc3rt Widget": "sm-rt-widget", + } + for in, legacy := range cases { + t.Run(fmt.Sprintf("%q", in), func(t *testing.T) { + // Positive control on the fixture: the bytes really are invalid and + // the pre-fix derivation really produced the named slug. + if got, err := legacySlugify(in); err != nil || got != legacy { + t.Fatalf("fixture is broken: pre-fix derivation gave (%q, %v), want %q", got, err, legacy) + } + got, err := Slugify(in) + if err == nil { + t.Fatalf("Slugify(%q) = %q, want a refusal — invalid UTF-8 silently loses bytes", in, got) + } + if got != "" { + t.Errorf("a refused Slugify must return no slug, got %q", got) + } + if !strings.Contains(err.Error(), "UTF-8") { + t.Errorf("the refusal should say what is wrong with the name: %v", err) + } + if !strings.Contains(err.Error(), "--slug") { + t.Errorf("refusal must point at the escape hatch: %v", err) + } + }) + } +} + +// TestLossyCharsReportsACombiningMarkWithItsBase — F5. macOS paths and some +// paste routes deliver NFD, so the SAME VISIBLE NAME arrives as two different +// byte sequences. Before this, NFC reported "é" and NFD reported a bare +// combining acute rendered over nothing. The refusal SET is identical either +// way (a mark was always lossy); only the rendering differed. +func TestLossyCharsReportsACombiningMarkWithItsBase(t *testing.T) { + const ( + nfc = "Caf\u00e9 App" // e-acute as ONE rune (U+00E9) + nfd = "Cafe\u0301 App" // 'e' + U+0301 COMBINING ACUTE ACCENT + bare = "\u0301Widget name" // a combining mark with no base at all + ) + // Fixture control. A character that RENDERS identically can be a different + // code point, so assert the bytes rather than trusting the source text. + if nfc == nfd { + t.Fatal("fixture is broken: the NFC and NFD forms must be different byte sequences") + } + if !strings.ContainsRune(nfd, '\u0301') || strings.ContainsRune(nfc, '\u0301') { + t.Fatalf("fixture is broken: only the NFD form may carry U+0301 (nfc=% x nfd=% x)", nfc, nfd) + } + + for _, tc := range []struct{ in, want string }{ + {nfc, "\u00e9"}, + {nfd, "e\u0301"}, + } { + got := LossyChars(tc.in) + if len(got) != 1 || got[0] != tc.want { + t.Fatalf("LossyChars(% x) = %q, want the single cluster [%q] the author actually sees", tc.in, got, tc.want) + } + // READABILITY is the whole point: the reported token must lead with a + // base character, never a lone combining mark floating over nothing. + if []rune(got[0])[0] == '\u0301' { + t.Errorf("LossyChars(% x) reported a bare combining mark %q", tc.in, got[0]) + } + if _, err := Slugify(tc.in); err == nil { + t.Fatalf("Slugify(% x) should refuse", tc.in) + } + } + + // A leading mark has no base to attach to, so it is shown on a dotted + // circle (the Unicode convention) rather than bare. + got := LossyChars(bare) + if len(got) == 0 { + t.Fatal("a leading combining mark is still content derivation would drop") + } + if !strings.HasPrefix(got[0], dottedCircle) { + t.Errorf("a base-less combining mark must be shown on a dotted circle, got %q", got[0]) + } +} + +// TestSlugifyAsciiDerivationIsByteIdentical is the CONTROL. These rows were +// green before the refusal existed and must stay green: the predicate exempts +// every rune that LOWERS INTO ASCII by construction, so ordinary punctuation and +// multiple spaces still fold to a single hyphen. An invariant guard, not +// regression coverage — its job is to fail loudly if the refusal ever widens. +func TestSlugifyAsciiDerivationIsByteIdentical(t *testing.T) { + cases := map[string]string{ + "My Cool Block": "my-cool-block", + "my-block": "my-block", + " Spaced Out ": "spaced-out", + "Foo/Bar_Baz.Qux!": "foo-bar-baz-qux", + "a & b + c = d": "a-b-c-d", + "Trailing punctuation.": "trailing-punctuation", + "UPPER case NAME": "upper-case-name", + "app2 v3": "app2-v3", + "a---b": "a-b", + } + for in, want := range cases { + got, err := Slugify(in) + if err != nil { + t.Errorf("Slugify(%q) errored, want %q: %v", in, want, err) + continue + } + if got != want { + t.Errorf("Slugify(%q) = %q, want %q", in, got, want) + } + } +} + +// TestSlugifyAsciiExemptionBoundary pins the `< utf8.RuneSelf` comparison from +// BOTH sides. Widening it to `<=` was a surviving mutant: U+0080 is a C1 control +// nobody types, so no behavioural row saw the change — but the doc comment calls +// that boundary load-bearing, and an unpinned boundary is a claim nothing holds. +func TestSlugifyAsciiExemptionBoundary(t *testing.T) { + // U+007F DEL is the last ASCII rune: exempt, folded to a hyphen like any + // other non-slug ASCII character. + if got, err := Slugify("del\x7fimiter app"); err != nil || got != "del-imiter-app" { + t.Errorf("U+007F is ASCII and must stay exempt: Slugify = (%q, %v), want del-imiter-app", got, err) + } + // U+0080 is the first rune ABOVE ASCII. It is a control (Cc) — not space, + // punct or symbol — so it is content with nowhere to go, and refusing it is + // what the boundary says. + got, err := Slugify("ctrl\u0080name app") + if err == nil { + t.Errorf("U+0080 is above ASCII and must be refused, got %q", got) + } + if len(LossyChars("ctrl\u0080name")) != 1 { + t.Errorf("LossyChars must report U+0080: %q", LossyChars("ctrl\u0080name")) + } +} + +// TestSlugifyLowersIntoAsciiIsADocumentedException — residual (b). Exactly two +// runes above ASCII lower INTO ASCII, and both derive rather than refuse. This +// is deliberate (it is the nicest transliteration available and costs nothing), +// so it is pinned as BEHAVIOUR rather than left as an accident of the boundary: +// a future "tighten the exemption to `r < utf8.RuneSelf` on the ORIGINAL rune" +// would silently start refusing İstanbul. +func TestSlugifyLowersIntoAsciiIsADocumentedException(t *testing.T) { + cases := map[string]string{ + "\u0130stanbul App": "istanbul-app", // U+0130 LATIN CAPITAL LETTER I WITH DOT ABOVE -> i + "Temp Kelvin": "temp-kelvin", // U+212A KELVIN SIGN -> k + } + for in, want := range cases { + got, err := Slugify(in) + if err != nil { + t.Errorf("Slugify(%q) errored, want the documented %q: %v", in, want, err) + continue + } + if got != want { + t.Errorf("Slugify(%q) = %q, want %q", in, got, want) + } + } + // The claim is that these are the ONLY two. If a future Go unicode table + // grows a third, the header's "exactly two" stops being true and this fails. + if n := len(runesThatLowerIntoASCII()); n != 2 { + t.Errorf("the header claims exactly 2 runes above ASCII lower into ASCII; found %d", n) + } +} + +// TestSlugifySymbolsAndEmojiStillFoldToAHyphen — residual (c), pinned as the +// deliberate behaviour it is rather than left undescribed. If someone decides to +// close it, this test is what says the change was intentional. +func TestSlugifySymbolsAndEmojiStillFoldToAHyphen(t *testing.T) { + cases := map[string]string{ + "Rocket \U0001F680 App": "rocket-app", + "Widget \u2764 Pro": "widget-pro", + } + for in, want := range cases { + got, err := Slugify(in) + if err != nil { + t.Errorf("Slugify(%q) errored, want the documented %q: %v", in, want, err) + continue + } + if got != want { + t.Errorf("Slugify(%q) = %q, want %q — symbols are a documented exception", in, got, want) + } + } +} + +// TestSlugifyNonAsciiSeparatorsStaySeparators: above ASCII the classification is +// by Unicode category, so a punctuation or symbol rune is still a separator with +// no content of its own. Getting this wrong would refuse names that derive +// perfectly well — the false-refusal direction. +func TestSlugifyNonAsciiSeparatorsStaySeparators(t *testing.T) { + cases := map[string]string{ + "Widget — Pro": "widget-pro", // em dash (Pd) + "Widget « Pro »": "widget-pro", // guillemets (Pi/Pf) + "Widget © Pro": "widget-pro", // copyright sign (So) + "Widget Pro": "widget-pro", // no-break space (Zs) + } + for in, want := range cases { + got, err := Slugify(in) + if err != nil { + t.Errorf("Slugify(%q) errored, want %q: %v", in, want, err) + continue + } + if got != want { + t.Errorf("Slugify(%q) = %q, want %q", in, got, want) + } + } +} + +// TestSlugifyExistingDeadEndsKeepTheirMessages. Both of these are ASCII, so the +// new refusal must not intercept them — the issue itself praises their messages, +// and stealing them would ALSO make the "names the offending characters" claim +// false (there are none to name). +func TestSlugifyExistingDeadEndsKeepTheirMessages(t *testing.T) { + cases := map[string]string{ + "123 Numbers": "must start with a letter", + "!!!": "need ≥3 chars", + } + for in, want := range cases { + _, err := Slugify(in) + if err == nil { + t.Errorf("Slugify(%q) should still fail", in) + continue + } + if !strings.Contains(err.Error(), want) { + t.Errorf("Slugify(%q) message changed: got %q, want it to contain %q", in, err, want) + } + if strings.Contains(err.Error(), "--slug ") { + t.Errorf("Slugify(%q) was intercepted by the lossy-rune refusal: %v", in, err) + } + } +} + +// TestLossyCharsIsDeduplicatedAndOrdered — the message lists characters, so a +// name repeating one must not repeat it in the error. +func TestLossyCharsIsDeduplicatedAndOrdered(t *testing.T) { + got := LossyChars("ÜÜber Ω Über") + want := []string{"Ü", "Ω"} + if len(got) != len(want) { + t.Fatalf("LossyChars = %q, want %q", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("LossyChars = %q, want %q (first-appearance order)", got, want) + } + } + if len(LossyChars("My Cool Block")) != 0 { + t.Error("an ASCII name has no lossy characters") + } +} + +// TestSlugifyRefusalBoundsTheCharacterList — a fully non-Latin name can carry +// dozens of distinct characters; the refusal names the first few and counts the +// rest rather than echoing the whole name back as a quoted list. +func TestSlugifyRefusalBoundsTheCharacterList(t *testing.T) { + long := "日本語アプリケーションのテスト" + lossy := LossyChars(long) + if len(lossy) <= maxNamedChars { + t.Fatalf("fixture is broken: %q has %d distinct lossy characters, need > %d", long, len(lossy), maxNamedChars) + } + _, err := Slugify(long) + if err == nil { + t.Fatal("expected a refusal") + } + msg := err.Error() + // The bounded prefix is named … + for _, c := range lossy[:maxNamedChars] { + if !strings.Contains(msg, c) { + t.Errorf("refusal should name %q: %s", c, msg) + } + } + // … and the remainder is COUNTED, not silently dropped. + if !strings.Contains(msg, fmt.Sprintf("(and %d more)", len(lossy)-maxNamedChars)) { + t.Errorf("refusal should count the characters it did not spell out: %s", msg) + } + // The quoted-list is bounded: exactly maxNamedChars quoted characters. + if got := strings.Count(msg, `", "`); got != maxNamedChars-1 { + t.Errorf("expected %d separators in the quoted list, got %d: %s", maxNamedChars-1, got, msg) + } +}