From aa5232098842766f82a3aad1d302cc9b3b10bd43 Mon Sep 17 00:00:00 2001 From: Andrej Koelewijn Date: Sun, 13 Sep 2026 07:34:05 +0000 Subject: [PATCH 01/11] test(run-local): count settle polls instead of timing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestSettleSourceReturnsPromptlyForOneChange failed CI on PR #449 with "a quiet source took 196.975373ms to settle, want under 100ms" — on a docs-only diff that cannot reach a debounce timer. The same tree passed build-and-test in the push run and failed in the pull_request run minutes apart, which is direct evidence of nondeterminism rather than a regression. The test bounded elapsed wall-clock as a multiple of the poll interval: poll * (sourceSettleWindow + 3), 100ms against a nominal 40ms. settleSource waits on time.After(poll), which guarantees AT LEAST the duration and says nothing about the upper bound, so a loaded runner blows the budget with nothing wrong. It is also the anti-pattern the PR checklist names — tests must not rely on sleeps for synchronisation. What the test is really guarding is a POLL COUNT: a quiet source costs sourceSettleWindow polls, not an unbounded wait. So the poll timer is now injected (settleSourceWith) and the test counts polls. Widening the budget would only have moved the flake threshold. settleSource keeps its signature and is still exercised with a real timer by the multi-file and interrupt tests, so the wrapper is not going untested. The seam also makes a guarantee expressible that the timing test could not reach: the window is sourceSettleWindow CONSECUTIVE quiet polls, so a write part-way through restarts the count. Deleting `quiet = 0` from the change branch was green against every pre-existing test in this file. Controls, each isolating one guarantee: - loop costs one poll more than the window it declares -> both count tests fail - `quiet = 0` removed from the change branch -> only the consecutive test fails - tick returning one shared channel instead of re-arming -> the multi-file test hangs, so the re-arm is load-bearing rather than a style choice - widening sourceSettleWindow itself -> both still pass, deliberately: the assertions track the declared window rather than pinning its value 30 consecutive runs of the four tests are green, and the package passes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017s476QkXr9CFMvKspVzcvu --- .../skills/fix-issue/findings/cmd-mxcli.jsonl | 1 + cmd/mxcli/docker/runlocal.go | 15 +++- cmd/mxcli/docker/runlocal_settle_test.go | 75 +++++++++++++++++-- 3 files changed, 84 insertions(+), 7 deletions(-) diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index f05ae9bbf7..6f928cc32a 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -105,3 +105,4 @@ {"area": "cmd/mxcli", "date": "2026-09-11", "symptom": "Two opposite `brain plan` failures with one root cause. A requirement anchored at a bare MODULE (`@Maintenance`) reports BUILT the moment the module exists, with none of its work done. A requirement anchored at a MODULE ROLE (`@Maintenance.Coordinator`) reports PLANNED forever, even once the roles exist \u2014 `describe` refuses it ('no describable document named \u2026') and `mxcli refs` finds nothing", "cause": "`catalogResolver.Resolve` in `cmd/mxcli/cmd_brain.go`. A module has a row in the catalog's `objects` view, so it resolves immediately \u2014 fine for a decision (anchors point backward) and useless for a requirement (anchors point forward, so resolution IS the progress signal). A module role is in NEITHER the objects view NOR `FindDocumentUnit`, because it is not a document \u2014 so both lookups miss and it falls through to NotFound, which for a requirement means 'not built yet' permanently", "file": "`cmd/mxcli/cmd_brain.go` (`catalogResolver.Resolve`, `moduleRoleExists` via `GetModuleSecurity`), `cmd/mxcli/brain/entry.go` (`requirementAnchorsArePlannable`)", "insight": "**A resolver's vocabulary has to cover what people actually anchor at, and the two failure directions need opposite fixes.** The module-role gap is a LOOKUP gap \u2014 fixed by consulting `GetModuleSecurity`, case-insensitively because Mendix treats role names that way and an anchor is hand-written. The module-anchor gap is SEMANTIC and cannot be fixed by lookup: the anchor resolves correctly and is still useless, so it is refused at capture time with the alternative named (an author told only 'no' deletes the anchor, which loses the signal entirely rather than fixing it). Keep the control in BOTH directions: a module anchor stays legal on a decision and on a question, or the fix degrades to 'refuse every module anchor' and the negative test still passes. Verified end to end: `@MyFirstModule.User` \u2192 1 anchor 1 resolved; `@MyFirstModule.NoSuchRole` \u2192 NOT FOUND, exit 1, so the resolver did not simply become permissive. Reported by ako/ChipCoV1", "refs": ["ako/ChipCoV1 FINDINGS.md"]} {"area": "cmd/mxcli", "date": "2026-09-11", "symptom": "`.claude/bootstrap-mxcli.sh` (the SessionStart hook) re-downloads ~85 MB of mxcli on EVERY fresh session in an environment where mxcli is already installed on PATH", "cause": "The script gated only on `[ ! -x ./mxcli ]` and never consulted PATH, while the bootstrap skill tells you to `rm -f /mxcli` after moving the project to the repo root \u2014 so in a session image with mxcli pre-installed the guard could never be satisfied by the binary already present, forever", "file": "`cmd/mxcli/init_hook.go` (`bootstrapScriptTemplate`)", "insight": "**A guard that asks 'is the artifact HERE' rather than 'is it AVAILABLE' pays full cost for something already on the machine.** Hardlink a PATH copy in first (what `mxcli new` itself does), falling back to symlink across filesystems then to a copy, and only download when none of those work \u2014 every path leaves ./mxcli working, which the rest of the script and the generated project CLAUDE.md both assume. Keep BOTH controls when testing: nothing on PATH must still download (not silently no-op), and an existing ./mxcli must be left untouched. Reported by ako/ChipCoV1", "refs": ["ako/ChipCoV1 FINDINGS.md"]} {"area": "cmd/mxcli", "date": "2026-09-11", "symptom": "`mxcli theme create --from ` seeds the palette and nothing else: the scaffolded brand theme still described itself in `theme list` as 'Cool slate, one teal signal colour' with Signal's six swatches, and vendored ~500 KB of IBM Plex woff2 the seeded --mxt-font never names, plus a SIL OFL licence for fonts it does not use. Separately, the primary button was never the brand colour: Atlas derives --btn-primary-bg from --brand-primary-600 = color-mix(brand, contrast 20%), so a brand blue #10069F rendered rgb(21,13,140)", "cause": "`manifest()` copied the base theme's Summary and Colorway verbatim and the walk copied every file unconditionally. The Atlas map pinned `--btn-primary-color` to `--mxt-brand-ink` \u2014 an ink each theme picks to sit on `--mxt-brand` (console pairs near-black #04211d with bright teal #2dd4bf) \u2014 while leaving the background to Atlas's derivative, so the pairing the theme designed for was never the pairing that rendered. The map's own comment already called it 'a brand-filled button'", "file": "`cmd/mxcli/theme/create_seeded.go`, `create.go` (`manifest`, the scaffold walk), `assets/*/files/theme/web/_mxcli-atlas-map.scss`", "insight": "**Inheriting a statement ABOUT the base theme into a theme whose palette is no longer the base's is a confident lie; derive it or drop it.** Two traps in the font half. (1) The decision must be made BEFORE the walk: it is taken by reading the partial and applied to files elsewhere in the tree, so doing it inline depended on WalkDir's lexical order putting `_mxcli-.scss` before `mxcli-fonts/` \u2014 true only because of the leading underscore. (2) It touches two halves \u2014 the @font-face rules and the woff2 files \u2014 and getting either alone wrong is silent: a surviving rule for a deleted file 404s, a surviving file nothing loads is the dead weight being removed. Unit tests on each half cannot catch a mismatch; the guard is an integration assertion that a scaffolded theme ships exactly the fonts it loads (control: stubbing the file half fails it with 'X is shipped but no @font-face loads it'). Reported by ako/ChipCoV1", "refs": ["ako/ChipCoV1 FINDINGS.md"]} +{"area": "cmd/mxcli", "date": "2026-09-13", "symptom": "`build-and-test` fails in CI on `TestSettleSourceReturnsPromptlyForOneChange` \u2014 \"a quiet source took 196.975373ms to settle, want under 100ms\" \u2014 while the SAME tree passes in another run of the same workflow minutes earlier", "cause": "The test bounded elapsed wall-clock time as a multiple of the poll interval (`poll * (sourceSettleWindow + 3)`, 100ms against a nominal 40ms). settleSource waits on `time.After(poll)`, which guarantees AT LEAST the duration and nothing about the upper bound, so a loaded runner blows the budget with no defect present.", "file": "cmd/mxcli/docker/runlocal.go (settleSourceWith, the injected tick), cmd/mxcli/docker/runlocal_settle_test.go", "insight": "The property being guarded was a POLL COUNT, not a duration \u2014 'a quiet source costs one extra poll' \u2014 so the fix is to make polls countable (inject the timer) rather than to widen the budget, which only moves the flake threshold. Diagnosis shortcut worth reusing: the same workflow ran twice on the same tree, once from the push event and once from the pull_request merge commit, and disagreed \u2014 two runs of one tree is direct evidence of nondeterminism and cheaper than reading the test. Two things the controls settled that reasoning did not: (1) the assertions are written in terms of `sourceSettleWindow`, so WIDENING that constant leaves both tests green \u2014 they assert the loop honours whatever window is declared, never the number itself, and the real control is a loop that costs one poll MORE than it declares (both fail). (2) Each tick call must return a freshly-armed channel; returning one shared channel makes the multi-file test HANG rather than miscount, so the re-arm is load-bearing and not a style choice. The seam also made a previously untestable guarantee expressible: the window must be sourceSettleWindow CONSECUTIVE quiet polls, and dropping `quiet = 0` from the change branch was green against every pre-existing test in the file.", "refs": ["ako/mxcli#449"]} diff --git a/cmd/mxcli/docker/runlocal.go b/cmd/mxcli/docker/runlocal.go index e63726df43..8eec31d004 100644 --- a/cmd/mxcli/docker/runlocal.go +++ b/cmd/mxcli/docker/runlocal.go @@ -1172,12 +1172,25 @@ const sourceSettleWindow = 2 // returns as soon as the source has been quiet for sourceSettleWindow polls, so // an ordinary single-file save costs one extra poll. func settleSource(projectPath string, seen time.Time, poll time.Duration, sigCh <-chan os.Signal) time.Time { + return settleSourceWith(projectPath, seen, sigCh, func() <-chan time.Time { return time.After(poll) }) +} + +// settleSourceWith is settleSource with the poll timer injected, so a test can +// assert how many polls a quiet source costs rather than how long it took. +// +// The distinction is not cosmetic. `tick` is a LOWER bound — time.After +// guarantees at least the duration and says nothing about the upper one — so a +// test that bounds elapsed wall-clock as a multiple of it fails on a loaded CI +// runner with no defect present, which is what this seam exists to stop. +// Each call must return a freshly-armed channel: the window is "quiet for N +// consecutive polls", so reusing one channel would collapse the wait. +func settleSourceWith(projectPath string, seen time.Time, sigCh <-chan os.Signal, tick func() <-chan time.Time) time.Time { quiet := 0 for { select { case <-sigCh: return time.Time{} - case <-time.After(poll): + case <-tick(): } now := sourceMTime(projectPath) if now.After(seen) { diff --git a/cmd/mxcli/docker/runlocal_settle_test.go b/cmd/mxcli/docker/runlocal_settle_test.go index 257db5034c..be41f7181a 100644 --- a/cmd/mxcli/docker/runlocal_settle_test.go +++ b/cmd/mxcli/docker/runlocal_settle_test.go @@ -73,8 +73,26 @@ func TestSettleSourceWaitsForAMultiFileWrite(t *testing.T) { } } +// countingTick stands in for the poll timer: every call fires immediately and +// records that a poll happened, so a test can count polls instead of timing them. +func countingTick(polls *int) func() <-chan time.Time { + return func() <-chan time.Time { + *polls++ + ch := make(chan time.Time, 1) + ch <- time.Now() + return ch + } +} + // TestSettleSourceReturnsPromptlyForOneChange guards the other direction: a // single editor save must not pay a long wait. +// +// "Promptly" is a POLL COUNT, not a duration. This test used to assert +// `elapsed <= poll * (sourceSettleWindow + 3)` — 100ms against a nominal 40ms — +// and failed on a loaded CI runner at 196ms with nothing wrong: time.After +// guarantees at least its duration and nothing about the upper bound, so +// bounding wall-clock as a multiple of it is not a property the scheduler +// offers. Counting polls asserts the same guarantee and cannot flake. func TestSettleSourceReturnsPromptlyForOneChange(t *testing.T) { dir := t.TempDir() mpr := filepath.Join(dir, "App.mpr") @@ -82,16 +100,61 @@ func TestSettleSourceReturnsPromptlyForOneChange(t *testing.T) { t.Fatal(err) } - const poll = 20 * time.Millisecond - start := time.Now() - settled := settleSource(mpr, sourceMTime(mpr), poll, nil) - elapsed := time.Since(start) + polls := 0 + settled := settleSourceWith(mpr, sourceMTime(mpr), nil, countingTick(&polls)) if settled.IsZero() { t.Fatal("settleSource reported an interrupt that never happened") } - if max := poll * (sourceSettleWindow + 3); elapsed > max { - t.Errorf("a quiet source took %v to settle, want under %v", elapsed, max) + if polls != sourceSettleWindow { + t.Errorf("a quiet source cost %d polls, want exactly %d", polls, sourceSettleWindow) + } +} + +// TestSettleSourceNeedsConsecutiveQuietPolls pins the half of the window the +// timing test could never see: the source must be quiet for sourceSettleWindow +// polls IN A ROW, so a write part-way through the window restarts the count +// rather than being tolerated. Without it, `quiet = 0` could be dropped from the +// change branch and every test here would still pass. +// +// Only expressible now that polls are countable — with a real timer this needed +// a write timed against a wall clock, which is the flake this file just removed. +func TestSettleSourceNeedsConsecutiveQuietPolls(t *testing.T) { + dir := t.TempDir() + mpr := filepath.Join(dir, "App.mpr") + if err := os.WriteFile(mpr, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + start := sourceMTime(mpr) + + // Touch the source once, on the poll right before the window would close. + polls, touched := 0, false + tick := func() <-chan time.Time { + polls++ + if polls == sourceSettleWindow && !touched { + touched = true + if err := os.Chtimes(mpr, start.Add(time.Second), start.Add(time.Second)); err != nil { + t.Error(err) + } + } + ch := make(chan time.Time, 1) + ch <- time.Now() + return ch + } + + settled := settleSourceWith(mpr, start, nil, tick) + + if !touched { + t.Fatal("the write never happened — the test proved nothing") + } + // The touch lands on the last poll of the first window; that poll observes + // the change and resets instead of closing, so a full window runs again. + if want := sourceSettleWindow * 2; polls != want { + t.Errorf("a source touched mid-window cost %d polls, want %d "+ + "(the quiet counter did not restart)", polls, want) + } + if settled.Before(start.Add(time.Second)) { + t.Errorf("settled at %v, before the last write — the build would miss it", settled) } } From 40b2ecd56b10ddcd4276dbc2d912e67f50abfe27 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 07:42:26 +0000 Subject: [PATCH 02/11] fix: provision PostgreSQL as a non-root user (closes mendixlabs/mxcli#984) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mxcli run --local --ensure-db` could not provision PostgreSQL in a non-root devcontainer. That is the environment mxcli itself generates: `mxcli init` writes a devcontainer from mcr.microsoft.com/devcontainers/base:bookworm which installs the postgresql server and runs as `vscode`, and wires `run --local --setup --ensure-db` into the Claude Code SessionStart hook — so it fired on every fresh session in an initialized project, not only when someone typed the flag. Fixing it here rather than in the template also repairs projects that have already been scaffolded. Three independent defects sat on one path, each sufficient to block it: - The service start was never elevated. Debian's /etc/init.d/postgresql runs under `set -e` and calls create_socket_directory first, which chmods /var/run/postgresql — refused for a non-root user, so the script aborts before it looks at a single cluster. Now attempted with `sudo -n` first and unprivileged second. - The #823 user-owned-cluster fallback was inert on Debian and Ubuntu, which wrap only the client tools into /usr/bin while initdb and pg_ctl live in /usr/lib/postgresql//bin. The safety net for a failed service start could never deploy on the platform that needs it most. Both tools are now resolved from the same versioned directory, ranked numerically — a lexical sort puts "9" above "16", and a data directory made by one major cannot be started by another. - The superuser was unreachable: `sudo -u postgres` is refused by the devcontainer's own sudoers rule, `vscode ALL=(root) NOPASSWD:ALL`, which permits root as the only target. Falls back to `sudo -n -- sudo -n -u postgres`, since root may in turn target any account. Everything stays non-interactive, so an unattended run cannot block on a password prompt. Separately, the readiness timeout discarded the service manager's own output, which this package had already collected — the permission denial was in hand and reported as a generic 20s timeout. A guard that names the wrong cause costs the reader more than no guard at all, so the diagnostics now travel with the error. Measured on Ubuntu 24.04 / PostgreSQL 16 with a real non-root user and the real sudoers rule, against a harness calling EnsureDatabase directly: the shipped build fails with `exec: "initdb": executable file not found in $PATH` and, once the cluster is up, `no local PostgreSQL superuser available`; after the fix the sudo path provisions in 2.6s and the no-sudo user-cluster path in 7.2s. Each of the four fixes was reverted individually and its test re-run against the reported symptom. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Dp1syGhH8yr7Hve2wwjzqg --- .../skills/fix-issue/findings/cmd-mxcli.jsonl | 1 + .claude/skills/mendix/run-local/SKILL.md | 10 +- CHANGELOG.md | 10 + cmd/mxcli/docker/ensuredb.go | 186 +++++++++++--- cmd/mxcli/docker/ensuredb_test.go | 230 +++++++++++++++++- 5 files changed, 393 insertions(+), 44 deletions(-) diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index f05ae9bbf7..e28ee25824 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -105,3 +105,4 @@ {"area": "cmd/mxcli", "date": "2026-09-11", "symptom": "Two opposite `brain plan` failures with one root cause. A requirement anchored at a bare MODULE (`@Maintenance`) reports BUILT the moment the module exists, with none of its work done. A requirement anchored at a MODULE ROLE (`@Maintenance.Coordinator`) reports PLANNED forever, even once the roles exist \u2014 `describe` refuses it ('no describable document named \u2026') and `mxcli refs` finds nothing", "cause": "`catalogResolver.Resolve` in `cmd/mxcli/cmd_brain.go`. A module has a row in the catalog's `objects` view, so it resolves immediately \u2014 fine for a decision (anchors point backward) and useless for a requirement (anchors point forward, so resolution IS the progress signal). A module role is in NEITHER the objects view NOR `FindDocumentUnit`, because it is not a document \u2014 so both lookups miss and it falls through to NotFound, which for a requirement means 'not built yet' permanently", "file": "`cmd/mxcli/cmd_brain.go` (`catalogResolver.Resolve`, `moduleRoleExists` via `GetModuleSecurity`), `cmd/mxcli/brain/entry.go` (`requirementAnchorsArePlannable`)", "insight": "**A resolver's vocabulary has to cover what people actually anchor at, and the two failure directions need opposite fixes.** The module-role gap is a LOOKUP gap \u2014 fixed by consulting `GetModuleSecurity`, case-insensitively because Mendix treats role names that way and an anchor is hand-written. The module-anchor gap is SEMANTIC and cannot be fixed by lookup: the anchor resolves correctly and is still useless, so it is refused at capture time with the alternative named (an author told only 'no' deletes the anchor, which loses the signal entirely rather than fixing it). Keep the control in BOTH directions: a module anchor stays legal on a decision and on a question, or the fix degrades to 'refuse every module anchor' and the negative test still passes. Verified end to end: `@MyFirstModule.User` \u2192 1 anchor 1 resolved; `@MyFirstModule.NoSuchRole` \u2192 NOT FOUND, exit 1, so the resolver did not simply become permissive. Reported by ako/ChipCoV1", "refs": ["ako/ChipCoV1 FINDINGS.md"]} {"area": "cmd/mxcli", "date": "2026-09-11", "symptom": "`.claude/bootstrap-mxcli.sh` (the SessionStart hook) re-downloads ~85 MB of mxcli on EVERY fresh session in an environment where mxcli is already installed on PATH", "cause": "The script gated only on `[ ! -x ./mxcli ]` and never consulted PATH, while the bootstrap skill tells you to `rm -f /mxcli` after moving the project to the repo root \u2014 so in a session image with mxcli pre-installed the guard could never be satisfied by the binary already present, forever", "file": "`cmd/mxcli/init_hook.go` (`bootstrapScriptTemplate`)", "insight": "**A guard that asks 'is the artifact HERE' rather than 'is it AVAILABLE' pays full cost for something already on the machine.** Hardlink a PATH copy in first (what `mxcli new` itself does), falling back to symlink across filesystems then to a copy, and only download when none of those work \u2014 every path leaves ./mxcli working, which the rest of the script and the generated project CLAUDE.md both assume. Keep BOTH controls when testing: nothing on PATH must still download (not silently no-op), and an existing ./mxcli must be left untouched. Reported by ako/ChipCoV1", "refs": ["ako/ChipCoV1 FINDINGS.md"]} {"area": "cmd/mxcli", "date": "2026-09-11", "symptom": "`mxcli theme create --from ` seeds the palette and nothing else: the scaffolded brand theme still described itself in `theme list` as 'Cool slate, one teal signal colour' with Signal's six swatches, and vendored ~500 KB of IBM Plex woff2 the seeded --mxt-font never names, plus a SIL OFL licence for fonts it does not use. Separately, the primary button was never the brand colour: Atlas derives --btn-primary-bg from --brand-primary-600 = color-mix(brand, contrast 20%), so a brand blue #10069F rendered rgb(21,13,140)", "cause": "`manifest()` copied the base theme's Summary and Colorway verbatim and the walk copied every file unconditionally. The Atlas map pinned `--btn-primary-color` to `--mxt-brand-ink` \u2014 an ink each theme picks to sit on `--mxt-brand` (console pairs near-black #04211d with bright teal #2dd4bf) \u2014 while leaving the background to Atlas's derivative, so the pairing the theme designed for was never the pairing that rendered. The map's own comment already called it 'a brand-filled button'", "file": "`cmd/mxcli/theme/create_seeded.go`, `create.go` (`manifest`, the scaffold walk), `assets/*/files/theme/web/_mxcli-atlas-map.scss`", "insight": "**Inheriting a statement ABOUT the base theme into a theme whose palette is no longer the base's is a confident lie; derive it or drop it.** Two traps in the font half. (1) The decision must be made BEFORE the walk: it is taken by reading the partial and applied to files elsewhere in the tree, so doing it inline depended on WalkDir's lexical order putting `_mxcli-.scss` before `mxcli-fonts/` \u2014 true only because of the leading underscore. (2) It touches two halves \u2014 the @font-face rules and the woff2 files \u2014 and getting either alone wrong is silent: a surviving rule for a deleted file 404s, a surviving file nothing loads is the dead weight being removed. Unit tests on each half cannot catch a mismatch; the guard is an integration assertion that a scaffolded theme ships exactly the fonts it loads (control: stubbing the file half fails it with 'X is shipped but no @font-face loads it'). Reported by ako/ChipCoV1", "refs": ["ako/ChipCoV1 FINDINGS.md"]} +{"area": "cmd/mxcli", "date": "2026-09-13", "symptom": "`mxcli run --local --ensure-db` cannot provision PostgreSQL in a non-root devcontainer (Debian/Ubuntu base, remoteUser vscode): reported as a bare \"PostgreSQL did not become ready at 127.0.0.1:5432 within 20s\", or as `exec: \"initdb\": executable file not found in $PATH`. Fires on every fresh Claude Code session in an initialized project, since `mxcli init` wires `run --local --setup --ensure-db` into the SessionStart hook", "cause": "THREE independent defects on one path, each sufficient to block it. (1) `service postgresql start` ran unelevated: Debian's /etc/init.d/postgresql runs under `set -e` and calls create_socket_directory FIRST, which chmods /var/run/postgresql — refused for a non-root user, so the script aborts before it looks at a single cluster. (2) The #823 user-owned-cluster fallback was INERT on Debian/Ubuntu: postgresql-common wraps only the CLIENT tools (psql, pg_isready, pg_ctlcluster) into /usr/bin, while initdb/pg_ctl live in /usr/lib/postgresql//bin — so the safety net for a failed service start could never deploy on the platform that needs it most. (3) `resolveSuperuser` used `sudo -n -u postgres psql`, but mcr.microsoft.com/devcontainers/base grants its user sudo to root ONLY (`vscode ALL=(root) NOPASSWD:ALL`, confirmed in devcontainers/features main.sh) — so the target is refused even though the user is effectively an administrator. Plus a diagnostic defect: the 20s readiness timeout discarded the service-manager output the package had already collected", "file": "`cmd/mxcli/docker/ensuredb.go` (`serviceStartAttempts`, `postgresServerBinDir`/`postgresTool`, `superuser.viaRoot`, `withServiceDiag`)", "insight": "mxcli GENERATES the broken environment — `generateDockerfile` emits that exact base image, installs postgresql, and runs as vscode — so this was not user misconfiguration, and fixing it in code (not the template) also repairs projects already scaffolded. Root may target any account, so `sudo -n -- sudo -n -u postgres` reaches postgres under a root-only sudoers policy; try the direct form first and nest only on refusal. Resolve initdb and pg_ctl from the SAME bin directory and rank majors NUMERICALLY — a lexical sort puts \"9\" above \"16\", and a data directory made by one major cannot be started by another. **Measurement trap that cost the most time**: reasoning about which error the user would see is unreliable here — four plausible code paths produce four different messages, and the reported wording was reproducible by none of them on Ubuntu 24.04/PG16. What settled it was building a harness that calls `EnsureDatabase` directly and running it as a real non-root user with the real sudoers rule, then isolating each defect with a one-variable control (widen sudoers to `(ALL)` and nothing else changes → provisioning succeeds; prepend /usr/lib/postgresql/16/bin → the fallback completes). Each of the four fixes was reverted individually and its test re-run: two controls initially failed to COMPILE rather than reproducing the symptom, which proves nothing — they were redone faithfully before being believed", "refs": ["mendixlabs/mxcli#984", "#823"]} diff --git a/.claude/skills/mendix/run-local/SKILL.md b/.claude/skills/mendix/run-local/SKILL.md index 9e05786c05..91cfba3d28 100644 --- a/.claude/skills/mendix/run-local/SKILL.md +++ b/.claude/skills/mendix/run-local/SKILL.md @@ -57,8 +57,14 @@ association catalog only at startup; behavioural changes are hot-reloaded. - **`--ensure-db`** provisions it for a fresh session: starts local Postgres if the port is down and creates the role + database if missing. It uses a service manager, or a user-owned `initdb`/`pg_ctl` cluster under `~/.mxcli/postgres` - when no service becomes ready (e.g. Arch) — needing no `postgres` OS account or `sudo`. - Remote hosts are only checked, not provisioned. + when no service becomes ready (e.g. Arch) — the latter needing no `postgres` + OS account or `sudo`. Remote hosts are only checked, not provisioned. + In a **non-root devcontainer** the service start is elevated with `sudo -n` + (Debian's init script aborts on a permission denial before it reaches any + cluster), and the superuser is reached through root where the sudoers policy + permits only that target — the devcontainer default. Both are non-interactive, + so a run never blocks on a password prompt; where sudo is unavailable the + user-owned cluster still carries it. The user-owned cluster persists across sessions; its server log is `~/.mxcli/postgres/server.log`. Stop it with `pg_ctl -D "$HOME/.mxcli/postgres/data" stop`. To remove it, stop it first and diff --git a/CHANGELOG.md b/CHANGELOG.md index 91bf30580d..5eb0880b89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`--ensure-db` could not provision PostgreSQL in a non-root devcontainer** (mendixlabs/mxcli#984) — reported as a bare `PostgreSQL did not become ready at 127.0.0.1:5432 within 20s`. This is the environment **mxcli itself generates**: `mxcli init` writes a devcontainer from `mcr.microsoft.com/devcontainers/base:bookworm` that installs the postgresql server and runs as `vscode`, and wires `run --local --setup --ensure-db` into the Claude Code SessionStart hook — so it fired on every fresh session in an initialized project, not only when someone typed the flag. + + Three independent defects sat on that one path, each sufficient to block it. **The service start was never elevated**: Debian's `/etc/init.d/postgresql` runs under `set -e` and calls `create_socket_directory` *first*, which chmods `/var/run/postgresql` — refused for a non-root user, so the script aborts before it looks at a single cluster. **The #823 user-owned-cluster fallback was inert on Debian and Ubuntu**: `postgresql-common` wraps only the *client* tools (`psql`, `pg_isready`, `pg_ctlcluster`) into `/usr/bin` while `initdb` and `pg_ctl` live in `/usr/lib/postgresql//bin`, so the safety net for a failed service start could never deploy on the platform that needs it most. **The superuser was unreachable**: `sudo -u postgres` is refused by the devcontainer's own sudoers rule, `vscode ALL=(root) NOPASSWD:ALL`, which permits root as the only target even though the user is effectively an administrator. + + The start is now attempted with `sudo -n` first and unprivileged second; `initdb` and `pg_ctl` are resolved from the same versioned bin directory, ranked **numerically** (a lexical sort puts `9` above `16`, and a data directory made by one major cannot be started by another); and the superuser falls back to `sudo -n -- sudo -n -u postgres`, since root may target any account. Everything stays non-interactive, so an unattended run cannot block on a password prompt. + + Separately, the readiness timeout **discarded the service manager's own output**, which this package had already collected — the permission denial was in hand and reported as a generic timeout. A guard that names the wrong cause costs the reader more than no guard at all, so the diagnostics now travel with the error. + + Measured on Ubuntu 24.04 / PostgreSQL 16 with a real non-root user and the real sudoers rule, against a harness calling `EnsureDatabase` directly: the shipped build fails with `exec: "initdb": executable file not found in $PATH` and, once the cluster is up, `no local PostgreSQL superuser available`; after the fix both the sudo path (2.6s) and the no-sudo user-cluster path (7.2s) provision cleanly. Note the trap — **reasoning about which error a user will see is unreliable here**: four plausible code paths produce four different messages, and the reported wording reproduced under none of them on this stack. Each fix was also reverted individually and its test re-run; two controls first failed to *compile* rather than reproducing the symptom, which proves nothing, and were redone faithfully. + - **`ALTER PAGE` bound the replacement widget against the wrong data context** (mendixlabs/mxcli#1076) — `replace txtPreviewPeriod with { … }` inside a data view bound `datasource: selection lvVersions` re-scoped the binding to the **outer** data view's entity (`[CE1613] "The selected attribute 'Bug.Dashboard.PeriodLabel' no longer exists."`), and the same statement inside a Gallery or DataGrid 2 sourced by a **microflow/nanoflow** dropped the binding entirely (`[CE0402] "No value specified."`, `describe` renders `ContentParams: [{1} = ]`). `mxcli check --references` and `exec` both reported success; `CREATE PAGE` binds the same widget in the same position correctly, so the defect was in the ALTER walk alone. A widget's scope was resolved by **two separate walks that each knew a different subset** of Mendix's ten `Forms$*Source` kinds, which is the third time that split has produced a wrong binding (association and flow sources in FINDINGS #55, pluggable widgets in ako/mxcli#935). `Forms$ListenTargetSource` carries no `EntityRef` at all — only the listen target's *name* — so the entity walk saw no source and left the context at the enclosing data view; and the flow walk read only a widget's **top-level** `DataSource` key, so a pluggable list, whose source sits in `Object.Properties[datasource]`, was invisible to it. diff --git a/cmd/mxcli/docker/ensuredb.go b/cmd/mxcli/docker/ensuredb.go index e4230f40ff..35eb21fd49 100644 --- a/cmd/mxcli/docker/ensuredb.go +++ b/cmd/mxcli/docker/ensuredb.go @@ -36,6 +36,120 @@ var pgIdent = regexp.MustCompile(`^[a-z_][a-z0-9_]*$`) // so tests can shrink it further. var serviceReadyTimeout = 3 * time.Second +// readyTimeout is that single authoritative wait. A variable so tests need not +// sit out the full deadline. +var readyTimeout = 20 * time.Second + +// currentEUID reports the effective user id. A variable so a test can exercise +// the unprivileged path on a root CI runner, and the root path on a laptop. +var currentEUID = os.Geteuid + +// postgresServerBinGlobs are the directories a distribution may keep the +// PostgreSQL *server* tools in when they are not on PATH. Debian and Ubuntu ship +// initdb, pg_ctl and postgres under /usr/lib/postgresql//bin and wrap only +// the client tools (psql, pg_isready, pg_ctlcluster) into /usr/bin. The +// user-owned cluster below needs initdb and pg_ctl, so without this the whole +// fallback is inert on the commonest devcontainer base — the safety net for a +// failed service start could never deploy on the platform that needs it most +// (#984). A variable so a stubbed PATH also gets a hermetic tool lookup. +var postgresServerBinGlobs = []string{"/usr/lib/postgresql/*/bin"} + +// onPath reports whether a command is resolvable through PATH. +func onPath(name string) bool { + _, err := exec.LookPath(name) + return err == nil +} + +// isExecutableFile reports whether path is a regular file with an execute bit. +func isExecutableFile(path string) bool { + info, err := os.Stat(path) + return err == nil && !info.IsDir() && info.Mode().Perm()&0o111 != 0 +} + +// postgresServerBinDir returns the directory to take initdb and pg_ctl from, or +// "" to resolve them through PATH. Both must come from the SAME installation: a +// data directory initialized by one major version cannot be started by another, +// so a directory is only a candidate when it carries both tools. +func postgresServerBinDir() string { + if onPath("initdb") && onPath("pg_ctl") { + return "" + } + best, bestMajor := "", -1 + for _, glob := range postgresServerBinGlobs { + matches, err := filepath.Glob(glob) + if err != nil { + continue + } + for _, dir := range matches { + if !isExecutableFile(filepath.Join(dir, "initdb")) || + !isExecutableFile(filepath.Join(dir, "pg_ctl")) { + continue + } + // Prefer the newest major. Comparing the paths as strings would rank + // "9" above "16", so read the version component as a number. + major, err := strconv.Atoi(filepath.Base(filepath.Dir(dir))) + if err != nil { + major = -1 + } + if best == "" || major > bestMajor { + best, bestMajor = dir, major + } + } + } + return best +} + +// postgresTool resolves a PostgreSQL server binary by name. It returns the bare +// name when nothing is found, so a genuinely missing tool still fails with the +// familiar "executable file not found in $PATH". +func postgresTool(name string) string { + if dir := postgresServerBinDir(); dir != "" { + return filepath.Join(dir, name) + } + return name +} + +// serviceAttempt is one service-manager invocation, with a label for diagnostics +// (the elevated and unprivileged forms of the same command must not report +// themselves identically). +type serviceAttempt struct { + label string + argv []string +} + +// serviceStartAttempts lists the service-manager invocations to try, in order. +// +// Debian's /etc/init.d/postgresql runs under `set -e` and calls +// create_socket_directory first, which chmods (or chowns) /var/run/postgresql — +// so as a non-root user the script aborts on a permission denial before it looks +// at a single cluster (#984). Every devcontainer mxcli itself generates is in +// exactly that position: the image runs as "vscode" with passwordless sudo. Try +// the elevated form first; `sudo -n` never prompts, so where sudo is absent or +// unauthorised it fails in milliseconds and the unprivileged attempt still runs. +func serviceStartAttempts() []serviceAttempt { + plain := []string{"service", "postgresql", "start"} + unprivileged := serviceAttempt{label: "service", argv: plain} + if currentEUID() == 0 || !onPath("sudo") { + return []serviceAttempt{unprivileged} + } + return []serviceAttempt{ + {label: "sudo service", argv: append([]string{"sudo", "-n", "--"}, plain...)}, + unprivileged, + } +} + +// withServiceDiag appends what the service manager said to a later failure. +// Without it the caller sees a bare readiness timeout for a permission denial +// this package already has in hand — a guard naming the wrong cause, which costs +// the reader more than no guard at all. +func withServiceDiag(err error, diag []string) error { + if len(diag) == 0 { + return err + } + return fmt.Errorf("%w\n(a service manager ran first but Postgres did not "+ + "become ready:\n%s)", err, strings.Join(diag, "\n")) +} + // splitHostPort splits a PostgreSQL endpoint into host and port, defaulting the // port to 5432 when absent. net.SplitHostPort handles the canonical bracketed // IPv6 form; the compatibility branch keeps accepting the historical @@ -145,11 +259,12 @@ func EnsureDatabase(db *DBConfig, w io.Writer) error { // Port down: start the local Postgres service (best-effort). if err := pingTCP(db.Host, 2*time.Second); err != nil { fmt.Fprintln(w, " Starting local PostgreSQL...") - if err := startLocalPostgres(host, port, w); err != nil { + serviceDiag, err := startLocalPostgres(host, port, w) + if err != nil { return fmt.Errorf("starting local PostgreSQL: %w", err) } - if err := waitPGReady(host, port, 20*time.Second); err != nil { - return err + if err := waitPGReady(host, port, readyTimeout); err != nil { + return withServiceDiag(err, serviceDiag) } } @@ -186,34 +301,32 @@ func canConnectDB(db DBConfig) bool { // service managers in turn. When none is present — e.g. on Arch — or they do // not produce a ready server, it falls back to a user-owned cluster started with // the portable initdb/pg_ctl tools (#823). -func startLocalPostgres(host, port string, w io.Writer) error { - var err error - port, err = normalizePostgresPort(port) +// It returns whatever the service managers reported, so a caller whose own +// readiness wait then fails can say what this package already saw. +func startLocalPostgres(host, port string, w io.Writer) ([]string, error) { + port, err := normalizePostgresPort(port) if err != nil { - return err + return nil, err } // Only real, portable service managers belong here. The old // {"pg_ctlcluster", "--", "start"} entry was a placeholder whose args could // never start a cluster, so it only ever burned a readiness timeout before // the fallback — dropped (#823 review). - attempts := [][]string{ - {"service", "postgresql", "start"}, - } var serviceDiag []string - for _, a := range attempts { - if _, err := exec.LookPath(a[0]); err != nil { + for _, a := range serviceStartAttempts() { + if _, err := exec.LookPath(a.argv[0]); err != nil { continue } // The command may exit non-zero yet still bring Postgres up, so a short // readiness probe decides — not the exit code. The probe is intentionally - // short: the single authoritative 20s wait is in EnsureDatabase, so a + // short: the single authoritative wait is in EnsureDatabase, so a // slow-but-working manager is honoured there rather than paid for here. - out, _ := exec.Command(a[0], a[1:]...).CombinedOutput() + out, _ := exec.Command(a.argv[0], a.argv[1:]...).CombinedOutput() if waitPGReady(host, port, serviceReadyTimeout) == nil { - return nil + return serviceDiag, nil } if d := strings.TrimSpace(string(out)); d != "" { - serviceDiag = append(serviceDiag, a[0]+": "+d) + serviceDiag = append(serviceDiag, a.label+": "+d) } } @@ -223,21 +336,17 @@ func startLocalPostgres(host, port string, w io.Writer) error { // This guard also covers a process that won the port between the caller's // initial reachability check and this fallback. if pingTCP(net.JoinHostPort(host, port), time.Second) == nil { - return nil + return serviceDiag, nil } // No service manager made PostgreSQL ready: start a user-owned cluster with // the portable tools. This needs neither a `postgres` OS account nor sudo. if err := startUserCluster(host, port, w); err != nil { - if len(serviceDiag) > 0 { - // Surface what the service manager said; otherwise the user sees only - // an initdb/pg_ctl error from two steps later. - return fmt.Errorf("%w\n(a service manager ran first but Postgres did not "+ - "become ready:\n%s)", err, strings.Join(serviceDiag, "\n")) - } - return err + // Surface what the service manager said; otherwise the user sees only + // an initdb/pg_ctl error from two steps later. + return serviceDiag, withServiceDiag(err, serviceDiag) } - return nil + return serviceDiag, nil } // userClusterDirs returns the state, data, and socket directories for the @@ -375,7 +484,7 @@ func rejectLegacyHostTrust(dataDir string) error { // readable, its TCP port and Unix-socket directory from postmaster.pid. // `pg_ctl status` alone only proves that some server runs from this data directory. func clusterStatus(dataDir string) (running bool, port, sockDir string) { - if exec.Command("pg_ctl", "-D", dataDir, "status").Run() != nil { + if exec.Command(postgresTool("pg_ctl"), "-D", dataDir, "status").Run() != nil { return false, "", "" } data, err := os.ReadFile(filepath.Join(dataDir, "postmaster.pid")) @@ -426,7 +535,7 @@ func startUserCluster(host, port string, w io.Writer) error { // our own provisioning password-free, but loopback TCP is scram-sha-256: // binding 127.0.0.1 is not an access control on a multi-user host, so trust // there would let any local account act as the postgres superuser. - init := exec.Command("initdb", "-D", dataDir, "-U", "postgres", + init := exec.Command(postgresTool("initdb"), "-D", dataDir, "-U", "postgres", "--auth-local=trust", "--auth-host=scram-sha-256", "--encoding=UTF8") if out, err := init.CombinedOutput(); err != nil { return fmt.Errorf("initializing PostgreSQL cluster in %s: %w\n%s", @@ -474,7 +583,7 @@ func startUserCluster(host, port string, w io.Writer) error { fmt.Fprintln(w, " Starting user-owned PostgreSQL cluster...") logPath := filepath.Join(stateDir, "server.log") - start := exec.Command("pg_ctl", "-D", dataDir, "-w", + start := exec.Command(postgresTool("pg_ctl"), "-D", dataDir, "-w", "-t", "30", "-l", logPath, "start") if out, err := start.CombinedOutput(); err != nil { return fmt.Errorf("starting PostgreSQL cluster in %s: %w\n%s\n (see the server "+ @@ -520,6 +629,12 @@ type superuser struct { host, port string sock string // Unix-socket dir for the user-owned cluster; preferred over TCP sudo bool + // viaRoot reaches the postgres account through root instead of directly. + // Devcontainers built from mcr.microsoft.com/devcontainers/base grant their + // non-root user sudo to root ONLY (`vscode ALL=(root) NOPASSWD:ALL`), so + // `sudo -u postgres` is refused there even though the user is effectively an + // administrator — while root may in turn target any account (#984). + viaRoot bool } // withoutPostgresTargetEnv prevents inherited libpq settings from overriding a @@ -566,6 +681,9 @@ func (s superuser) psql(args ...string) *exec.Cmd { // system cluster's peer-authenticated Unix socket, but pass the requested // port so a non-default cluster cannot fall through to port 5432. sudoBase := []string{"-n", "-u", "postgres", "--", "psql"} + if s.viaRoot { + sudoBase = append([]string{"-n", "--", "sudo"}, sudoBase...) + } cmd := exec.Command("sudo", append(sudoBase, append(base, args...)...)...) cmd.Env = withoutPostgresTargetEnv(os.Environ()) return cmd @@ -593,14 +711,18 @@ func resolveSuperuser(host, port string) (superuser, error) { } } if _, err := exec.LookPath("sudo"); err == nil { - sudo := superuser{host: host, port: port, sudo: true} - if sudo.psql("-tAc", "select 1").Run() == nil { - return sudo, nil + // Direct first; through root only if the sudoers policy refuses to target + // the postgres account, which is the devcontainer default. + for _, viaRoot := range []bool{false, true} { + sudo := superuser{host: host, port: port, sudo: true, viaRoot: viaRoot} + if sudo.psql("-tAc", "select 1").Run() == nil { + return sudo, nil + } } } return superuser{}, fmt.Errorf("no local PostgreSQL superuser available to create the " + "role/database (tried a direct 'psql -U postgres' connection over the cluster socket " + - "and TCP, and non-interactive 'sudo -u postgres')") + "and TCP, and non-interactive 'sudo -u postgres' both directly and via root)") } // ensureRole creates the app login role if it does not already exist. diff --git a/cmd/mxcli/docker/ensuredb_test.go b/cmd/mxcli/docker/ensuredb_test.go index 6a5a9efe4c..cbcf0ee39b 100644 --- a/cmd/mxcli/docker/ensuredb_test.go +++ b/cmd/mxcli/docker/ensuredb_test.go @@ -160,9 +160,32 @@ func newStubPATH(t *testing.T) (dir, logPath string) { dir = t.TempDir() t.Setenv("PATH", dir) t.Setenv("HOME", t.TempDir()) + // A stubbed PATH is only hermetic if the server-binary lookup is stubbed + // too: this host really does have /usr/lib/postgresql/*/bin, so a test that + // asserts "the tools are missing" would otherwise run a real initdb. + withServerBinGlobs(t) + // Pin the privilege level as well, so the same attempts are made whether the + // suite runs as root in a container or as a user on a laptop. + withEUID(t, 0) return dir, filepath.Join(dir, "calls") } +// withServerBinGlobs points the server-binary search at dirs (none by default). +func withServerBinGlobs(t *testing.T, dirs ...string) { + t.Helper() + old := postgresServerBinGlobs + postgresServerBinGlobs = dirs + t.Cleanup(func() { postgresServerBinGlobs = old }) +} + +// withEUID forces the effective user id the start path branches on. +func withEUID(t *testing.T, uid int) { + t.Helper() + old := currentEUID + currentEUID = func() int { return uid } + t.Cleanup(func() { currentEUID = old }) +} + func writeStub(t *testing.T, dir, name, body string) { t.Helper() if err := os.WriteFile(filepath.Join(dir, name), []byte("#!/bin/sh\n"+body+"\n"), 0o755); err != nil { @@ -228,7 +251,7 @@ func TestStartLocalPostgres_ServicePaths(t *testing.T) { writeStub(t, dir, "initdb", initdbStub(logPath)) writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) - if err := startLocalPostgres("127.0.0.1", "5432", io.Discard); err != nil { + if _, err := startLocalPostgres("127.0.0.1", "5432", io.Discard); err != nil { t.Fatal(err) } calls := readCalls(t, logPath) @@ -248,7 +271,7 @@ func TestStartLocalPostgres_ServicePaths(t *testing.T) { writeStub(t, dir, "initdb", initdbStub(logPath)) writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) - if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + if _, err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { t.Fatal(err) } calls := readCalls(t, logPath) @@ -281,7 +304,7 @@ func TestStartLocalPostgres_OccupiedPortSkipsFallback(t *testing.T) { writeStub(t, dir, "initdb", initdbStub(logPath)) writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) - if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + if _, err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { t.Fatal(err) } calls := readCalls(t, logPath) @@ -322,7 +345,7 @@ func TestStartLocalPostgres_Fallback(t *testing.T) { writeStub(t, dir, "pg_ctl", pgctlStub(logPath, tt.statusCode, tt.startCode)) } - err := startLocalPostgres("127.0.0.1", port, io.Discard) + _, err := startLocalPostgres("127.0.0.1", port, io.Discard) if (err != nil) != tt.wantErr { t.Fatalf("error = %v, wantErr %v", err, tt.wantErr) } @@ -505,7 +528,7 @@ func TestStartLocalPostgres_NeverRunsPgCtlCluster(t *testing.T) { writeStub(t, dir, "initdb", initdbStub(logPath)) writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) - if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + if _, err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { t.Fatal(err) } calls := readCalls(t, logPath) @@ -529,7 +552,7 @@ func TestStartLocalPostgres_SurfacesServiceOutput(t *testing.T) { writeStub(t, dir, "pg_isready", "exit 1") // No initdb/pg_ctl on PATH, so the fallback fails — the error should still // carry the service manager's message. - err := startLocalPostgres("127.0.0.1", port, io.Discard) + _, err := startLocalPostgres("127.0.0.1", port, io.Discard) if err == nil { t.Fatal("expected an error when neither a service nor the portable tools work") } @@ -547,7 +570,7 @@ func TestStartUserCluster_InitdbAuthArgs(t *testing.T) { writeStub(t, dir, "initdb", `echo "$@" >> "`+argsPath+`"; `+initdbStub(logPath)) writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) - if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + if _, err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { t.Fatal(err) } args := readCalls(t, argsPath) @@ -713,7 +736,7 @@ func TestStartUserCluster_RunningPortGuard(t *testing.T) { writePostmasterPID(t, dataDir, port) writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // status=0 => running - if err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + if _, err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { t.Fatal(err) } if calls := readCalls(t, logPath); strings.Contains(calls, "pg_ctl_start") { @@ -738,7 +761,7 @@ func TestStartUserCluster_RunningPortGuard(t *testing.T) { } writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 0, 0)) // status=0 => running - err = startLocalPostgres("127.0.0.1", port, io.Discard) + _, err = startLocalPostgres("127.0.0.1", port, io.Discard) if err == nil { t.Fatal("expected an error when a cluster runs on a different port") } @@ -795,7 +818,7 @@ func TestStartUserCluster_StartErrorNamesLog(t *testing.T) { initClusterDir(t) writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 1)) // not running, start fails - err := startLocalPostgres("127.0.0.1", port, io.Discard) + _, err := startLocalPostgres("127.0.0.1", port, io.Discard) if err == nil { t.Fatal("expected a start failure") } @@ -803,3 +826,190 @@ func TestStartUserCluster_StartErrorNamesLog(t *testing.T) { t.Fatalf("start error should name the server log: %v", err) } } + +// --- #984: --ensure-db in a non-root devcontainer --- + +// withTimeouts shrinks both readiness waits so a test need not sit them out. +func withTimeouts(t *testing.T, d time.Duration) { + t.Helper() + oldService, oldReady := serviceReadyTimeout, readyTimeout + serviceReadyTimeout, readyTimeout = d, d + t.Cleanup(func() { serviceReadyTimeout, readyTimeout = oldService, oldReady }) +} + +// Debian's /etc/init.d/postgresql runs under `set -e` and calls +// create_socket_directory before it looks at any cluster; that chmod of +// /var/run/postgresql is refused for a non-root user, so the script aborts +// having started nothing. The elevated attempt must therefore come first — and +// must never be able to block on a password prompt. +func TestStartLocalPostgres_ElevatesServiceStartWhenNotRoot(t *testing.T) { + dir, logPath := newStubPATH(t) + withEUID(t, 1000) + withTimeouts(t, 20*time.Millisecond) + port := unusedTCPPort(t) + writeStub(t, dir, "sudo", `echo "sudo $@" >> "`+logPath+`"`) + writeStub(t, dir, "service", `echo "service $@" >> "`+logPath+`"`) + writeStub(t, dir, "pg_isready", "exit 1") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if _, err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + first := strings.SplitN(strings.TrimSpace(calls), "\n", 2)[0] + if !strings.HasPrefix(first, "sudo ") { + t.Fatalf("the elevated start must be attempted FIRST, calls:\n%s", calls) + } + if !strings.Contains(first, "-n") { + t.Fatalf("sudo must be non-interactive, or an unattended run hangs on a "+ + "password prompt: %s", first) + } + for _, want := range []string{"service", "postgresql", "start"} { + if !strings.Contains(first, want) { + t.Fatalf("elevated attempt missing %q: %s", want, first) + } + } + // The unprivileged form still runs, so nothing that worked before regresses. + if !strings.Contains(calls, "\nservice postgresql start") { + t.Fatalf("the unprivileged attempt must still run:\n%s", calls) + } +} + +// Running as root, there is nothing to elevate and sudo must not be involved. +func TestStartLocalPostgres_RootStartsServiceDirectly(t *testing.T) { + dir, logPath := newStubPATH(t) // pins euid 0 + withTimeouts(t, 20*time.Millisecond) + port := unusedTCPPort(t) + writeStub(t, dir, "sudo", `echo "sudo $@" >> "`+logPath+`"`) + writeStub(t, dir, "service", `echo "service $@" >> "`+logPath+`"`) + writeStub(t, dir, "pg_isready", "exit 1") + writeStub(t, dir, "initdb", initdbStub(logPath)) + writeStub(t, dir, "pg_ctl", pgctlStub(logPath, 3, 0)) + + if _, err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + if calls := readCalls(t, logPath); strings.Contains(calls, "sudo") { + t.Fatalf("root must not shell out to sudo:\n%s", calls) + } +} + +// Debian and Ubuntu wrap only the CLIENT tools into PATH; initdb and pg_ctl live +// in /usr/lib/postgresql//bin. Without looking there the user-owned +// cluster fallback cannot run at all on the commonest devcontainer base. +func TestStartUserCluster_FindsServerBinariesOutsidePATH(t *testing.T) { + dir, logPath := newStubPATH(t) + withTimeouts(t, 20*time.Millisecond) + port := unusedTCPPort(t) + writeStub(t, dir, "pg_isready", "exit 1") // a client tool, on PATH + + // Two majors installed side by side, as a long-lived box really has. + libDir := t.TempDir() + for _, major := range []string{"9", "16"} { + binDir := filepath.Join(libDir, major, "bin") + if err := os.MkdirAll(binDir, 0o755); err != nil { + t.Fatal(err) + } + writeStub(t, binDir, "initdb", `echo "initdb-major-`+major+`" >> "`+logPath+`" +`+initdbStub(logPath)) + writeStub(t, binDir, "pg_ctl", `echo "pg_ctl-major-`+major+`" >> "`+logPath+`" +`+pgctlStub(logPath, 3, 0)) + } + withServerBinGlobs(t, filepath.Join(libDir, "*", "bin")) + + if _, err := startLocalPostgres("127.0.0.1", port, io.Discard); err != nil { + t.Fatal(err) + } + calls := readCalls(t, logPath) + for _, want := range []string{"initdb", "pg_ctl_start"} { + if !strings.Contains(calls, want) { + t.Fatalf("%s was not run from the versioned bin directory:\n%s", want, calls) + } + } + // Newest major wins. A lexical sort would rank "9" above "16" and pick the + // wrong installation — and initdb and pg_ctl must agree, since a data + // directory made by one major cannot be started by another. + if !strings.Contains(calls, "initdb-major-16") || !strings.Contains(calls, "pg_ctl_start") { + t.Fatalf("expected the newest major (16) to be used:\n%s", calls) + } + if strings.Contains(calls, "-major-9") { + t.Fatalf("major 9 must not be preferred over 16:\n%s", calls) + } +} + +// mcr.microsoft.com/devcontainers/base grants its non-root user sudo to root +// ONLY (`vscode ALL=(root) NOPASSWD:ALL`), so `sudo -u postgres` is refused +// there. Root may target any account, so the nested form still reaches postgres. +func TestResolveSuperuser_FallsBackToSudoViaRoot(t *testing.T) { + dir, logPath := newStubPATH(t) + writeStub(t, dir, "psql", "exit 1") // no direct superuser connection + writeStub(t, dir, "sudo", `echo "sudo $@" >> "`+logPath+`" +case " $* " in + *" -n -- sudo "*) exit 0 ;; + *" -n -u postgres "*) echo "sudo: a password is required" >&2; exit 1 ;; +esac +exit 1`) + + su, err := resolveSuperuser("127.0.0.1", "5432") + if err != nil { + t.Fatalf("a root-only sudoers policy must still reach postgres: %v", err) + } + if !su.sudo || !su.viaRoot { + t.Fatalf("expected the via-root superuser, got %+v", su) + } + calls := readCalls(t, logPath) + directAt := strings.Index(calls, "sudo -n -u postgres") + nestedAt := strings.Index(calls, "sudo -n -- sudo") + if directAt < 0 || nestedAt < 0 || directAt > nestedAt { + t.Fatalf("the direct form must be tried before nesting through root:\n%s", calls) + } +} + +func TestSuperuserPSQL_ViaRootNestsAndNeverPrompts(t *testing.T) { + cmd := (superuser{host: "127.0.0.1", port: "5544", sudo: true, viaRoot: true}). + psql("-tAc", "select 1") + want := []string{ + "sudo", "-n", "--", "sudo", "-n", "-u", "postgres", "--", "psql", + "-X", "-v", "ON_ERROR_STOP=1", "-w", + "-p", "5544", "-U", "postgres", "-d", "postgres", + "-tAc", "select 1", + } + if !reflect.DeepEqual(cmd.Args, want) { + t.Fatalf("via-root psql args = %#v, want %#v", cmd.Args, want) + } +} + +// The reported symptom: a permission denial this package already has in hand is +// reported to the user as a bare readiness timeout. A guard that names the wrong +// cause costs the reader more than no guard at all. +func TestEnsureDatabase_ReadinessTimeoutCarriesServiceDiagnostics(t *testing.T) { + dir, _ := newStubPATH(t) + withEUID(t, 1000) + withTimeouts(t, 50*time.Millisecond) + port := unusedTCPPort(t) + writeStub(t, dir, "psql", "exit 1") + writeStub(t, dir, "sudo", `echo "sudo: a password is required" >&2; exit 1`) + writeStub(t, dir, "service", + `echo "chmod: changing permissions of '/var/run/postgresql': Operation not permitted" >&2 +exit 1`) + writeStub(t, dir, "pg_isready", "exit 1") // never becomes ready + writeStub(t, dir, "initdb", initdbStub(filepath.Join(dir, "calls"))) + writeStub(t, dir, "pg_ctl", pgctlStub(filepath.Join(dir, "calls"), 3, 0)) + + db := DBConfig{ + Type: "PostgreSQL", Host: net.JoinHostPort("127.0.0.1", port), + Name: "app", User: "mendix", Password: "secret", + } + err := EnsureDatabase(&db, io.Discard) + if err == nil { + t.Fatal("expected a readiness failure") + } + if !strings.Contains(err.Error(), "did not become ready") { + t.Fatalf("expected the readiness timeout, got: %v", err) + } + if !strings.Contains(err.Error(), "Operation not permitted") { + t.Fatalf("the readiness timeout must carry what the service manager "+ + "actually said, got:\n%v", err) + } +} From af97445da14cc92bc98135690fa1e528c532691c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 07:53:01 +0000 Subject: [PATCH 03/11] docs(record-narrated-demo): conform the capture overlay to the video design system MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The demo capture is most of a product-demonstration film's runtime, and its narration overlay was the one piece of furniture drawn to nothing in particular. Build it to video-system/DESIGN-LANGUAGE.md in ako/mxcli-intro-video, which that repo makes normative over this skill. Fixes a silent placement bug found while doing it. Under `html{zoom:z}` — which take.js uses to reach a fixed-width Mendix layout — Chromium reports getBoundingClientRect() in zoom-adjusted (video) pixels but getComputedStyle() and style.* in CSS pixels, and narrate.js had it wrong in both directions: point() read a rect and assigned it straight to style.left, so the highlight landed at position x zoom (measured at 1.6842: a target at (168,202) ringed at (274,330)), while the 96px caption plate reached the file at 162 video px against a 184px band. Every overlay dimension is now stated in video pixels and divided by a zoom passed to configure(); checkOverlay() measures the installed plate against the band and throws at record time. Its control is one line — omit the zoom and it reports a 310px plate starting at y770. Design conformance: one accent and it belongs to the pointer; flat plane (no shadow, no radius, an opacity pulse rather than an expanding glow); the plate fills the caption band at the x96 gutter; the plate inherits the app's own computed body font instead of asking for 'Segoe UI', which falls back to DejaVu Sans on a clean build machine with nothing saying so; say() refuses tofu glyphs. The compositor keep-alive is a sweeping hairline rather than a spinning ring, since a system with no rounded corners can only spin a square — still continuous, not per caption. Narration is pinned to the catalogue: Kokoro bm_george, two-pass loudnorm to I=-18.6:TP=-1.5 (was -16, which would have made the demo the loudest film in the catalogue), a -30 LUFS bed, and per-line verification because TTS has failed both 0-of-12 and 10-of-12 while passing a total-duration check. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L45JFiJ6y58ftg2zWVEq7h --- .claude/skills/fix-issue/findings/other.jsonl | 1 + .../mendix/record-narrated-demo/SKILL.md | 222 ++++++++++--- .../mendix/record-narrated-demo/narrate.js | 310 ++++++++++++++---- .../mendix/record-narrated-demo/take.js | 4 + CHANGELOG.md | 14 + 5 files changed, 452 insertions(+), 99 deletions(-) diff --git a/.claude/skills/fix-issue/findings/other.jsonl b/.claude/skills/fix-issue/findings/other.jsonl index 70a928b841..3fa07fd258 100644 --- a/.claude/skills/fix-issue/findings/other.jsonl +++ b/.claude/skills/fix-issue/findings/other.jsonl @@ -14,3 +14,4 @@ {"area": "generated/metamodel", "date": "2026-08-19", "raw": "| A REST call's `body mapping Mod.EMM from $var` is written with the variable under a key Mendix does not read, an empty `ContentType`, and an action-level `RequestHandlingType` of `Custom` that contradicts its own `Microflows$MappingRequestHandling` sub-element. A form-data body is dropped entirely by DESCRIBE, so describe → edit → exec silently produces a call that posts nothing | `generated/metamodel` is decisive: `MicroflowsMappingRequestHandling` owns exactly three properties — `contentType` (enum Json\\|Xml), `mappingId`, `mappingVariableName`. mxcli wrote `ParameterVariable`, which the type does not own, and omitted `MappingVariableName`; its own READER had known the right key since #843 and the writer was never corrected. `RequestHandlingType` was hardcoded `\"Custom\"` in both engines regardless of handler. `FormDataRequestHandling` / `AdvancedRequestHandling` can be parsed and not written, so a rewrite dropped them | `mdl/backend/modelsdk/microflow_write.go` (`requestHandlingTypeOf`, the Mapping case) and `sdk/mpr/writer_microflow_actions.go` (`restRequestHandlingTypeOf`, same case); guard `mdl/executor/validate_rest_request_handling.go` wired from `cmd_microflows_create.go`; tests `mdl/backend/modelsdk/microflow_restbody_test.go`, `mdl/executor/validate_rest_request_handling_test.go` | **A reader that compensates for a writer hides the writer's bug** — the `MappingVariableName` fallback made DESCRIBE round-trip correctly while every document mxcli wrote carried the wrong key. When a reader has a \"the real key is X\" comment, check that the writer agrees. **An unknown property is worse than a wrong value**: mxbuild tolerates it and Studio Pro refuses to open the document, so a green build proves nothing (same rule as the overlay section in CLAUDE.md). **Ask for one Studio Pro example per variant** — four microflows covering Custom/Mapping/FormData/Binary settled the discriminator question that a single example had left as \"unverified, so leave it alone\"; guessing the other five enum values from one measured case would have been a coin flip. **Parse-but-cannot-write is a data-loss path, not a gap**: DESCRIBE omits what the writer cannot express, so the omission looks like faithful output — refuse the rewrite (ADR-0005), and make the guard's allow-list the writable set so it stops refusing the moment a type becomes expressible. Not fixed: the legacy engine encodes `MappingId` as binary where Studio Pro stores a qualified-name string |", "refs": ["#843"]} {"area": "model", "date": "2026-08-25", "raw": "| A multi-segment member in an **inline REST** mapping (`\"Title\" = \"fields/Title\"` inside `Response:`/`Body: MAPPING`) passes every gate — `mxcli check`, `exec`, `mx check`, `describe` — and the column is **empty at runtime** | The inline REST serializer is a SEPARATE code path from the mapping documents, and it appended the member text verbatim: `jsonPath + \"\\|\" + m.ExposedName` stores `(Object)\\|fields/Title`, one member whose NAME contains a slash, where Mendix stores `(Object)\\|fields\\|Title`. Nothing converted `/` to `\\|` on this path. Confirmed against four Studio Pro-authored inline response mappings in the demo apps, which all store full pipe paths (`(Object)\\|results\\|bindings\\|(Object)\\|caseId\\|value`) | `model/mapping_paths.go` (`InlineMappingPath`, `InlineMappingExposedName`), `sdk/mpr/writer_rest.go` (`serializeInlineMappingElement`), `mdl/backend/modelsdk/consumed_rest_write.go` (`restInlineMappingElementToGen`), `mdl/executor/cmd_rest_clients.go` (`inlineMemberName`) | **The document fixes do not reach this.** The mapping census (`PROPOSAL_mapping_coverage.md`) classifies mapping DOCUMENTS only and does not mention the inline form, so none of #248/#262–#268 improved it as a side effect — a reader who sees multi-segment paths documented as working will reasonably try one here. Store the pipe path and put the **last segment** in `ExposedName` (Studio Pro's own derivation uniquifies against siblings — `caseId\\|value` becomes `CaseId_Value` but `graphData\\|value` in the same document becomes plain `Value` — so it is not reproducible from the path and does not need to be; ExposedName is a label, JsonPath is what binds). **Fixing the path breaks DESCRIBE in the same motion**: the printer emitted `ExposedName`, which now holds only the last segment, so it produced `\"Title\" = \"Title\"` — output that parses, re-executes and binds the wrong member. Derive the member from the stored JsonPath relative to the enclosing element and drop the generated `(Object)`/`(Array)`/`(Wrapper)` markers. Both engines have their own copy of this serializer; patch both or they drift. Repro `mdl-examples/bug-tests/rest-inline-mapping-paths.mdl`, tests `sdk/mpr/writer_rest_inline_mapping_test.go`. mxcli-rest FINDINGS #36 |", "refs": ["#248", "#262", "#268", "#36"]} {"area": "web/dist", "date": "2026-08-30", "raw": "| After `mxcli test … --local`, an app another `mxcli run --local` is serving goes blank while still answering HTTP 200 (~1.7 KB, the Mendix SPA shell); the runtime log shows `Connector: 404 - file not found for file: dist%2Findex.js` and `deployment/web/dist` is gone | `cmd/mxcli/testrunner/localapp_options.go`, `cmd/mxcli/testrunner/runner_local.go` (`localTestDeployDir`), `cmd/mxcli/testrunner/runner.go` (`checkScratchDeploymentExists`) | A local test run already used its own ports and its own `_test` database — the code comment says why, verbatim — but shared the **deployment directory**, which is the one the *browser* reads. A headless test boot does not bundle the web client, so its build left the running app serving the shell over a 404: tests pass, run keeps running, app is blank, nothing reported at either end. **Detection was not the fix**: the two processes use different ports by design, so no port check can see it, and a lock file would only turn a silent blanking into a refusal. The test boot now builds into `/.mxcli/deployment-test/` — gitignored, already where the test runtime log lives — which makes the collision impossible. Note booting a runtime against the shared directory damages it even **without** a rebuild (the packaging step removes the bundle — FINDINGS §35, `ReportLostWebClientBundle`), so \"reuse the dev loop's tree read-only\" is not an alternative. Consequence to wire: `--skip-build` used to mean \"reuse deployment/\" and now has nothing until tests have run once, so it is refused with the reason rather than failing inside the runtime boot against a path the user never chose. Reported as mxcli-formula1 FINDINGS §62 |"} +{"area": ".claude/skills/mendix/record-narrated-demo", "date": "2026-09-13", "symptom": "In a narrated demo recorded with CSS `zoom` (take.js's fix for a fixed-width Mendix page), narrate.js's `point()` highlight ring is drawn around the wrong control or off the edge of the frame, and the caption plate is the wrong height and sits outside the film's caption band. Nothing in the take, the beat assertions or the contact sheet reports anything.", "cause": "Under `html{zoom:z}` Chromium reports `getBoundingClientRect()` in ZOOM-ADJUSTED (video) pixels but `getComputedStyle()` and `style.*` in CSS pixels. `point()` read a rect and assigned it straight to `style.left/top/width/height`, so the ring landed at position x z (measured at z=1.6842: a target at (168,202) ringed at (274,330)). The plate had the mirror-image problem: its geometry was declared in CSS pixels, so a 96px bar reached the file as 96 x z = 162 video px against a 184px caption band.", "file": ".claude/skills/mendix/record-narrated-demo/narrate.js (`point`, `css`, `checkOverlay`)", "insight": "The overlay lives in the page's coordinate space and the film is specified in the frame's, and `zoom` is the only conversion between them - so every overlay number is now stated in VIDEO pixels and divided by a zoom passed to `configure()`. The trap is that the conversion runs in opposite directions depending on which API you read it back with, which is why the fix came with `checkOverlay()`: it measures the installed plate against the band and refuses the take, with the control being one line (build the overlay without telling it the zoom -> 'caption plate is 310 video px tall, the band is 184'). A design rule that can be measured in the page should be a check that throws at record time, not a note in a skill - the same argument PRODUCTION.md sec 12 makes for compositions.", "refs": ["ako/mxcli-intro-video video-system/DESIGN-LANGUAGE.md"]} diff --git a/.claude/skills/mendix/record-narrated-demo/SKILL.md b/.claude/skills/mendix/record-narrated-demo/SKILL.md index 7c56a13d38..b9b74a290f 100644 --- a/.claude/skills/mendix/record-narrated-demo/SKILL.md +++ b/.claude/skills/mendix/record-narrated-demo/SKILL.md @@ -69,8 +69,8 @@ part of recording, not a nicety before it. ## Recording mechanics -Four things that decide whether the video is watchable. Each has a reason; none -is a style preference. +The things that decide whether the video is watchable, and whether it looks like +the rest of the catalogue. Each has a reason; none is a style preference. ### Record at a human pace, not the harness's @@ -84,18 +84,33 @@ Two numbers, both from films that were re-cut for being too fast: Caption read time is roughly `words ÷ 3.5` seconds — which is what `narrate.js` computes. Screen read time is how long it takes a viewer to *find the thing that changed*, and it is always longer than it feels while authoring. `narrate.js` - knows only the caption; when the screen is the slower of the two, pass `holdMs`. + knows only the caption; when the screen is the slower of the two — or when a + narration line is longer than both — pass the measured duration as `holdMs`. - **About two events per ten seconds.** A click, then its result. Not a click, a scroll, a filter and a result — that is four things a viewer is asked to track in the time they can follow one. +And three the finished film is measured against, from the video system's +`TONE-AND-SPEED.md` — a product demonstration is **60–120s**, **55–65% voice +density**, **~135 wpm**. Density is the one worth checking early: it is the tail +budget stated as a ratio, and a walk that fills 80% of its runtime with talking +is not a slow film that needs trimming, it is a fast one wearing a slow pace. + ### Give the compositor something to draw during pauses Playwright's video captures only frames the compositor actually produces. A genuinely idle screen during a reading pause can collapse to almost no video, so a 5-second pause plays back in a blink and the narration desyncs. Keep something -continuously animating — a small pulsing indicator is enough — so idle time is -recorded *as* idle time. +continuously animating so idle time is recorded *as* idle time. + +`narrate.js` does this with a hairline segment sweeping the caption plate's top +edge, on a loop, for the whole take. It used to be a spinning ring, and the ring +had to go for a reason worth keeping in mind whenever this element is redesigned: +the design language has **no rounded corners**, so a ring can only be a spinning +square. The keep-alive has to be expressible in the system's own vocabulary — +here, 1px structure travelling — rather than bolted on beside it. What it must +not become is per-caption: a hold *between* captions still has to produce frames, +so the animation runs continuously and is never keyed to a reveal. ### Record the same walk at desktop and at a real mobile profile @@ -166,20 +181,80 @@ project: | | | |---|---| -| `say(page, text, step)` | caption, held for `max(2200, words * 280)` ms — a fixed hold rushes long lines and stalls on short ones | -| `point` / `unpoint` | pulsing outline around an element's rect, **drawn** — a real click ring would move the cursor and the page under it | +| `configure({ zoom, accent, ground, font })` | tokens and the **zoom take.js is using** — see below; call it before anything else | +| `say(page, text, step)` | caption, held for `max(2500, words * 280)` ms — a fixed hold rushes long lines and stalls on short ones. Refuses a caption containing a tofu glyph | +| `point` / `unpoint` | pulsing outline around an element's rect, **drawn** — a real click ring would move the cursor and the page under it. The film's one accent event | | `clickSlowly` | scroll in, mark, beat, click: a cursor that arrives and clicks in one frame reads as a glitch | | `typeSlowly` | per-key typing, then commit | | `bringIntoView` | includes **horizontal** scroll (`inline: 'center'`), for a grid whose action sits past a phone's right edge | +| `checkOverlay(page)` | the design rules that are measurable in the page, as a check that **throws**. Run by `install()`, so it costs nothing to remember | -The spinning ring in the caption bar is the compositor fix described above, not -decoration and not a loading indicator — it is what keeps a reading pause from -collapsing to no frames. Removing it silently breaks the pause *and* any audio -timed against it. +The sweeping hairline is the compositor fix described above, not decoration and +not a loading indicator — it is what keeps a reading pause from collapsing to no +frames. Removing it silently breaks the pause *and* any audio timed against it. What stays per-project is the walk itself: the persona, the steps and the selectors (`narrated-walkthrough.js`). Only the library is shared. +### The overlay is the film's furniture, so it is on the design system + +A capture is most of a Type A film's runtime, and the caption plate is the only +thing the film draws over it. Furniture that disagrees with the frames it cuts +against reads as two designs — so the overlay is built to +`video-system/DESIGN-LANGUAGE.md`, not to whatever looked reasonable in a +browser. What that changed, and why each one is a defect rather than a taste: + +- **One accent, and it is the pointer.** The plate carries none. Two accented + things competing is the fastest way a frame stops working, and the accent's + job here is *where to look*. +- **The plane is flat** — no shadow under the plate, no corner radius on the + plate or the pointer, no glow. Depth is the hairline at the plate's top edge + and the value step to the app's ground. The pointer pulses on **opacity**, + because the old expanding `box-shadow` was a glow. +- **No system font stack.** `'Segoe UI'` and friends do not exist on a clean + build machine and fall back to DejaVu Sans with nothing saying so, so the + plate now inherits the **app's own computed body font**. That is also the + seam-free choice: furniture set in a different face from the UI it wraps is + the one thing a viewer notices without being able to name it. +- **Ligatures off**, for the reason the whole system has them off: a code + ligature composes `!=` into one glyph, and on a caption quoting the app that is + a claim rather than a typographic choice. `say()` refuses `≠ → ✓ ✗` and the + rest of the tofu set outright — derive the real set from the font's cmap when + you know the file (`fontTools` recipe in `PRODUCTION.md` §3). +- **The plate fills the caption band**, exactly — the bottom 17% of 1080, + `y896–1080` — and the safe-area gutter puts its text at `x96`. That band is + reserved across the whole catalogue, kept clear even in films that carry no + captions, so a plate that is 96px tall or floated somewhere else is not a + smaller caption: it is the one film whose bottom edge does not line up. + +**Geometry is stated in video pixels and converted with the zoom, and the two +coordinate spaces do not agree.** This is the part worth reading twice, because +it is invisible until the capture is cut against a composed frame. `take.js` +reaches a fixed-width layout with CSS `zoom` on `html`, so a plate declared +`96px` tall lands at `96 × zoom` in the *file*. Measured at zoom 1.6842: the old +96px plate is **162 video px**, not the 184 the band wants — and it is worse than +a wrong number, because everything about it looks right in the browser. So pass +the zoom to `configure()` and let the overlay do the division. + +Then measuring it back has the opposite trap, also measured on Chromium 1194: + +| under `html{zoom:1.6842}` | reports | +|---|---| +| `getBoundingClientRect()` | **video px** — a 109.25px-tall plate measures 183.98 | +| `getComputedStyle()` | **CSS px** — the same plate reports `109.25px` | + +`checkOverlay()` compares the rect raw and multiplies the computed padding, and +it exists because getting that backwards produces a perfectly styled plate in the +wrong place. Its control is one line: build the overlay without telling it the +zoom, and it reports a 310px plate starting at y770. + +The same mismatch had already broken `point()`, silently, for as long as anything +has used `zoom`: it read a rect (video px) and assigned it straight to +`style.left` (CSS px, zoomed on the way out), so the highlight landed at +*position × zoom*. Measured at 1.6842, a target at (168, 202) was ringed at +(274, 330) — a ring around the wrong control, or off the screen, in a take that +otherwise looks fine. Anything that reads a rect and writes a style has to divide. + ### Spoken narration, if you add it `recordVideo` writes a **silent** track — voice is not a setting, it is a second @@ -187,34 +262,58 @@ pipeline you build and mux in. It has been produced ad hoc in a session before, which is the problem: re-improvised each time, it lands on a different voice, a different pace and different levels, so the demo's sound quality is luck. Pin it. -Neither dependency is guaranteed present — both were **absent** from a fresh web -container, and `apt-get install ffmpeg` failed there against a stale package index -(404s on superseded `libva`/`mesa` versions) until `apt-get update` ran first. -Check for them before promising audio; `pip install piper-tts` plus one voice -`.onnx` + `.onnx.json` is the rest. +**The voice is Kokoro `bm_george`, and it is not a per-film decision.** Every +film in the catalogue uses it; a demo that arrives in a different voice breaks +the family harder than any visual difference, because the viewer hears the change +before they can look for it. Pace varies by type, the voice does not. -Three things decide whether the result sounds professional. All are measured, not +```python +from kokoro import KPipeline +pipe = KPipeline(lang_code='b') # British English +audio = np.concatenate([c.audio.numpy() for c in pipe(line, voice='bm_george', speed=1.0)]) +sf.write(f"assets/voice/{n:02d}.wav", audio, 24000) # Kokoro emits 24 kHz mono +``` + +Where mxcli is named at all — in a Type A film that is the closing credit only — +**write it phonetically in the script**, `em ex see ell eye`, so the TTS spells +the five letters out instead of trying to pronounce it. On screen it stays +`mxcli`, lowercase. A copy pass will "correct" this back into a word if nobody +says why it is there. + +`ffmpeg` and `ffprobe` are not guaranteed present — both were **absent** from a +fresh web container, and `apt-get install ffmpeg` failed there against a stale +package index (404s on superseded `libva`/`mesa` versions) until `apt-get update` +ran first. Check for them before promising audio. + +Four things decide whether the result sounds professional. All are measured, not matters of taste: -1. **Normalize, or it clips.** Raw Piper output measured **-17.5 LUFS with a - +0.0 dBTP true peak** — at full scale, so it crunches audibly the moment it is - encoded to AAC for the video. Two-pass `loudnorm` (measure with - `print_format=json`, then feed the measured values back) to `I=-16:TP=-1.5` - brought the same clip to -16.2 LUFS / **-4.5 dBTP**. Resample to 48 kHz stereo - at the same time: Piper emits 22.05 kHz mono, which is not what a video - container wants. -2. **Set the pace explicitly and then verify it.** `--length-scale` controls - speaking rate, but only scales cleanly with `--sentence-silence 0` - (measured 0.5 → 2.26s, 1.0 → 3.39s, 2.0 → 5.41s on one sentence; an earlier - run varying the flag alone moved the duration by 6% across the same range). - Read each clip's real duration back with `ffprobe` rather than trusting the - flag. -3. **Time the video from the audio, not the reverse.** Synthesize first, measure - each clip, and hold the step for that long. This is also why the pulsing - indicator above is load-bearing rather than cosmetic: if an idle pause - collapses to almost no frames, a pre-rendered voice track drifts against the - picture no matter how good the synthesis is. Confirm the recorded file's - duration matches the script's wall-clock before adding audio at all. +1. **Normalize to the catalogue's level, or it clips *and* it stands out.** Raw + TTS output measured **-17.5 LUFS with a +0.0 dBTP true peak** — at full scale, + so it crunches audibly the moment it is encoded to AAC for the video. Two-pass + `loudnorm` (measure with `print_format=json`, then feed the measured values + back) to **`I=-18.6:TP=-1.5`**, which is where the rest of the films sit; an + earlier pass here targeted -16 and would have made the demo the loudest thing + in the catalogue by two and a half LU. Resample to 48 kHz stereo at the same + time — 24 kHz mono is not what a video container wants. +2. **A bed, not a track.** A product demonstration takes a warm sustained pad, + low and continuous, no percussion, mastered to **-30 LUFS** (`loudnorm=I=-30: + TP=-3:LRA=7`) so it sits well under the voice. Duck it to near-silence under + the beat that pays off. +3. **Verify per line, never by total duration.** TTS has failed two ways that + both pass a total check: 0 of 12 lines generated (a silent film, exit 0), and + 10 of 12 (a film with two silent beats). Assert each file exists, is longer + than 0.5s, and matches the duration recorded for it — and that the count + matches the script's line count. Read each clip's real duration back with + `ffprobe` rather than trusting any synthesis flag. +4. **Time the video from the audio, not the reverse.** Synthesize first, measure + each clip, and hold the step for `voice duration + tail` — not the bare voice + length, which is what every sync tool defaults to and which leaves zero + reading time. This is also why the keep-alive above is load-bearing rather + than cosmetic: if an idle pause collapses to almost no frames, a pre-rendered + voice track drifts against the picture no matter how good the synthesis is. + Confirm the recorded file's duration matches the script's wall-clock before + adding audio at all. ## The take has to be true, not only watchable @@ -319,20 +418,59 @@ present tense instead. This skill owns the **capture**. How a capture is framed, cut and scored into a finished film is the video system's — `video-system/` in `ako/mxcli-intro-video`, -which defines the product-demonstration type this skill feeds. +which defines the product-demonstration type this skill feeds. Read it before a +film, in its own order, and **treat it as normative over this page**: the numbers +here are copied from it and go stale when it moves. -Two boundaries worth keeping: the recording is **full-bleed** (no browser chrome, -no window frame, no laptop mockup — the capture *is* the frame), and the -narration plate stays `narrate.js`'s. **One caption system per film**; a second -one layered on in the edit reads as two designs. +| | | +|---|---| +| `DESIGN-LANGUAGE.md` | palette, type, grid, plane, motion, the closing lockup, the honesty rules — invariant across all three types | +| `TONE-AND-SPEED.md` | everything that varies, as numbers. The comparison table is the working document | +| `types/product-demonstration.md` | the type this skill feeds | +| `PRODUCTION.md` | the hazards, several of which are the ones on this page | + +Four boundaries worth keeping: + +- The recording is **full-bleed** — no browser chrome, no window frame, no laptop + mockup. The capture *is* the frame. +- The narration plate stays `narrate.js`'s. **One caption system per film**; a + HyperFrames caption layered on in the edit reads as two designs. A film may + also run the capture with no captions at all and carry the narration in voice + alone (`videos/sudoku-demo` does) — but then the caption band stays *clear*, + which is the same rule, not an exemption from it. +- **Nothing is animated on top of a capture in the edit** — no drifting scale, no + Ken Burns, no highlight rings added in post. The app's own transitions carry + those beats, and `point()` is allowed precisely because it is *in* the capture, + decided at record time with the app's state in front of you. +- The **closing lockup** is the film's, not the capture's, and it is identical in + every film in the catalogue. Do not build one into the walk. + +### The clock map is a consequence of one long take + +`take.js` records a whole walk in one browser context, which is why `cut-clips.js` +has to fit an offset *and* a ~1.065 clock scale before it can trust a mark. The +alternative — **one context per beat** — removes that problem entirely rather than +modelling it: each recording starts at its own zero, and there is nothing to map. +It costs the session, so each beat has to re-enter the app (carry the login as +Playwright `storageState`) and it cannot film a continuous interaction across a +cut. Take it when the walk is genuinely a set of independent scenes; keep the +single take when the continuity is the point. ## Checklist - [ ] `journeys/.journey.json` exists and the journey run is `PASS` - [ ] The positive-control run has shown every rung can go red — no `UNPROVEN` - [ ] The demo is a **separate script**; no PASS/FAIL verdict lives in it -- [ ] Pace is human, with real reading pauses +- [ ] Pace is human, with real reading pauses; the finished film is 60–120s at + 55–65% voice density - [ ] Something animates during pauses, so idle time survives into the video +- [ ] `configure()` was told the same `zoom` as `openTake`, and `checkOverlay` + passed — the plate fills the caption band, flat, accent-free, in the app's + own font +- [ ] Captions carry no glyph outside the shipped fonts (`say()` refuses the + known set; derive the rest from the font's cmap) +- [ ] Voice is Kokoro `bm_george`, mastered two-pass to `I=-18.6:TP=-1.5`, and + every line was verified individually — not by total duration - [ ] Recorded at **both** a desktop viewport and a real mobile device profile - [ ] The mobile pass runs the same steps, with nothing simplified - [ ] Narration mentions no database proof and no past bugs diff --git a/.claude/skills/mendix/record-narrated-demo/narrate.js b/.claude/skills/mendix/record-narrated-demo/narrate.js index f04cfdf842..20f5f34b55 100644 --- a/.claude/skills/mendix/record-narrated-demo/narrate.js +++ b/.claude/skills/mendix/record-narrated-demo/narrate.js @@ -1,20 +1,89 @@ // // The narration overlay. Injected into the page under test; asserts nothing. // -// Two jobs, and the second one is not decoration: +// Three jobs, and only the first is obvious: // // 1. Show a caption a viewer can read while the app does something. // 2. Keep something MOVING for the whole recording. +// 3. Look like the rest of the catalogue. // -// Playwright's video captures frames the compositor actually produces. A screen -// that is genuinely still during a reading pause can collapse to almost no -// video - the pause the viewer needed disappears, and the demo cuts from one +// (2) Playwright's video captures frames the compositor actually produces. A +// screen that is genuinely still during a reading pause can collapse to almost +// no video - the pause the viewer needed disappears, and the demo cuts from one // action straight into the next. A small continuously animating element means -// idle time is recorded as idle time. That is what the progress ring is for; it -// is not a spinner and it is not pretending anything is loading. +// idle time is recorded as idle time. That is what the sweeping hairline is +// for; it is not a spinner and it is not pretending anything is loading. // +// (3) The capture is most of a Type A film's runtime, so this overlay is the +// film's furniture, not a debug HUD. It is built to +// `video-system/DESIGN-LANGUAGE.md` in ako/mxcli-intro-video - the palette, the +// flat plane, the grid and the caption band - because furniture that disagrees +// with the frames it cuts against reads as two designs. `checkOverlay()` below +// enforces the parts of that which are measurable in the page. +// + +// The system palette (DESIGN-LANGUAGE.md §1). ONE accent, and it belongs to +// #demo-spot alone: the thing the viewer should be looking at. The caption +// plate is deliberately accent-free - two teal things competing is the single +// most common way a frame stops working. +// +// A product demonstration may swap `accent` (and `ground`) for the demonstrated +// app's own - see types/product-demonstration.md, "Type and accent". That is +// the one documented allowance; pass it through `configure()`, do not edit it +// here. +const SYSTEM = { + ground: '#0e1116', // full-bleed ground + line: '#262828', // 1px hairline - carries all structure + ink: '#e6edf3', // primary text, 16.0:1 + faint: '#6b7787', // chrome labels, 4.15:1 - floor for 24px+ mono only + accent: '#3fbdb8', // the only colour +}; + +// Geometry is stated in VIDEO pixels (the 1920x1080 frame), never in CSS +// pixels, and converted with `zoom`. This is not pedantry: take.js reaches a +// fixed-width layout with CSS `zoom`, so a plate declared as `96px` tall lands +// at 96*zoom in the file - at the zoom sudoku-demo used, 162 device px, which +// is neither the caption band nor anything else in the system. Tell the overlay +// the zoom and every number below is the number that reaches the video. +const FRAME = { width: 1920, height: 1080 }; +const BAND = 184; // bottom 17% of 1080 - the caption band (DESIGN-LANGUAGE.md §3) +const GUTTER = 96; // safe-area x - the caption's left edge aligns with the grid +const CAPTION_PX = 34; +const LABEL_PX = 26; // >= 24px, the floor for `faint` on mono + +let cfg = { + ...SYSTEM, + zoom: 1, + // Set to a family that is actually loaded in the page. Left null, the plate + // inherits the app's own computed body font, which is both the seam-free + // choice for a Type A film and the only one that cannot fall back: a plate + // asking for 'Segoe UI' renders in DejaVu Sans on a clean build machine and + // nothing says so. + font: null, + // checkOverlay() throws rather than warns. The pipeline's default failure + // mode is to keep going and hand you a plausible-looking film. + strict: true, +}; + +/** Override the tokens, the zoom, or the caption font. Call before install(). */ +function configure(opts = {}) { + cfg = { ...cfg, ...opts }; + return cfg; +} -const OVERLAY_CSS = ` +// Marks that are not in the shipped fonts and render as tofu. The system +// derives this set from the film font's cmap (PRODUCTION.md §3) - `·`, `—` and +// `–` ARE present in Recursive and are not banned; `≠`, `→`, `✓` and `✗` are +// not present anywhere in the catalogue's fonts. A caption is set in the APP's +// font, whose coverage is its own business, so this list is the floor rather +// than the whole check: when you know the file, derive the set from it. +const BANNED_GLYPHS = /[≠→←↑↓⇒⇐➔➡✓✗✔✘≤≥]/; + +const css = () => { + const z = cfg.zoom || 1; + const px = (v) => `${(v / z).toFixed(3)}px`; + const viewportW = FRAME.width / z; + return ` /* pointer-events: none for the same reason #demo-spot has it, and the reason is easy to miss here: the plate is a full-width bar pinned to the BOTTOM of the viewport, which is exactly where Mendix puts a page footer's buttons. @@ -26,67 +95,171 @@ const OVERLAY_CSS = ` so it gives up pointer events for free. */ #demo-narration { position: fixed; left: 0; right: 0; bottom: 0; z-index: 2147483647; - pointer-events: none; - display: flex; align-items: center; gap: 14px; - padding: 16px 22px; - background: rgba(17, 24, 39, .94); - color: #fff; - font: 500 17px/1.45 system-ui, -apple-system, 'Segoe UI', Roboto, sans-serif; - box-shadow: 0 -8px 24px rgba(0,0,0,.22); - transform: translateY(110%); + box-sizing: border-box; height: ${px(BAND)}; + pointer-events: none; overflow: hidden; + display: flex; align-items: center; gap: ${px(28)}; + padding: 0 ${px(GUTTER)}; + background: ${cfg.ground}; color: ${cfg.ink}; + /* Depth is a hairline and the value step to the app's own ground. NO + box-shadow: the flat plane is not a preference, it is what stops the + film reading as a template (DESIGN-LANGUAGE.md §4). */ + border-top: ${px(1)} solid ${cfg.line}; + font-size: ${px(CAPTION_PX)}; line-height: 1.35; font-weight: 500; + /* A code ligature composes "!=" into a single glyph: the bytes stay "!=" + and the picture shows a different character. On a caption quoting what + the app said, that is a claim rather than a typographic choice. */ + font-variant-ligatures: none; + font-feature-settings: "liga" 0, "clig" 0, "calt" 0, "dlig" 0; + transform: translateY(100%); transition: transform .45s cubic-bezier(.16,.84,.44,1); } #demo-narration.up { transform: translateY(0); } - #demo-narration .ring { - flex: 0 0 auto; width: 22px; height: 22px; border-radius: 50%; - border: 2.5px solid rgba(255,255,255,.28); border-top-color: #fff; - animation: demo-spin 1.15s linear infinite; + + /* The compositor keep-alive, and the reason it is a hairline rather than the + spinning ring this used to be: the system has no rounded corners, so a ring + could only be a spinning square. A segment travelling the plate's top edge + is the same guarantee in the system's own vocabulary — 1px structure, in + the "faint" role, never the accent — and it doubles as a read-progress cue. + Continuous, not per-caption: a hold between captions still has to produce + frames. */ + #demo-narration .sweep { + position: absolute; top: 0; left: 0; + width: ${px(180)}; height: ${px(2)}; + background: ${cfg.faint}; + animation: demo-sweep 2.6s linear infinite; } + @keyframes demo-sweep { + from { transform: translateX(${px(-180)}); } + to { transform: translateX(${viewportW.toFixed(2)}px); } + } + #demo-narration .text { flex: 1 1 auto; } #demo-narration .step { - flex: 0 0 auto; font-size: 12.5px; letter-spacing: .06em; - text-transform: uppercase; color: rgba(255,255,255,.55); + flex: 0 0 auto; font-family: ui-monospace, monospace; + font-size: ${px(LABEL_PX)}; letter-spacing: .14em; + text-transform: uppercase; color: ${cfg.faint}; } - @keyframes demo-spin { to { transform: rotate(360deg); } } - /* Where the viewer should be looking. Drawn, not clicked - a real click ring - would move the cursor and the page under it. */ + /* Where the viewer should be looking, and the film's one accent event. + Drawn, not clicked — a real click ring would move the cursor and the page + under it. Square, because the plane is flat; the pulse is opacity, because + a soft expanding glow is banned for the same reason the shadow is. */ #demo-spot { position: fixed; z-index: 2147483646; pointer-events: none; - border-radius: 10px; border: 2.5px solid #2563eb; - box-shadow: 0 0 0 4px rgba(37,99,235,.22); - transition: all .4s cubic-bezier(.16,.84,.44,1); + border: ${px(3)} solid ${cfg.accent}; + transition: left .4s cubic-bezier(.16,.84,.44,1), + top .4s cubic-bezier(.16,.84,.44,1), + opacity .4s linear; opacity: 0; } #demo-spot.on { opacity: 1; animation: demo-pulse 1.6s ease-in-out infinite; } + @keyframes demo-pulse { 0%, 100% { opacity: 1; } 50% { opacity: .42; } } /* Reserve the plate's own height at the foot of the page. pointer-events above makes a covered control CLICKABLE; this makes it VISIBLE, and the film needs both — a click that lands under an opaque caption is a beat the viewer cannot see happen, which is the same dead beat by another route. - Applied once at install, before the take starts, so nothing shifts mid-shot. - --demo-plate-height is overridable for an unusually long caption. */ - :root { --demo-plate-height: 96px; } + Applied once at install, before the take starts, so nothing shifts mid-shot. */ + :root { --demo-plate-height: ${px(BAND)}; } body { padding-bottom: var(--demo-plate-height) !important; } - @keyframes demo-pulse { - 0%, 100% { box-shadow: 0 0 0 4px rgba(37,99,235,.22); } - 50% { box-shadow: 0 0 0 9px rgba(37,99,235,.10); } - } `; +}; /** Put the overlay on the page. Safe to call again after a navigation. */ -async function install(page) { - await page.addStyleTag({ content: OVERLAY_CSS }).catch(() => {}); - await page.evaluate(() => { - if (document.getElementById('demo-narration')) return; - const bar = document.createElement('div'); - bar.id = 'demo-narration'; - bar.innerHTML = '
'; - document.body.appendChild(bar); - const spot = document.createElement('div'); - spot.id = 'demo-spot'; - document.body.appendChild(spot); - }).catch(() => {}); +async function install(page, opts) { + if (opts) configure(opts); + await page.addStyleTag({ content: css() }).catch(() => {}); + await page.evaluate((font) => { + let bar = document.getElementById('demo-narration'); + if (!bar) { + bar = document.createElement('div'); + bar.id = 'demo-narration'; + bar.innerHTML = '
'; + document.body.appendChild(bar); + const spot = document.createElement('div'); + spot.id = 'demo-spot'; + document.body.appendChild(spot); + } + // Inherit the app's own typeface unless told otherwise. Furniture set in + // a different face from the UI it wraps reads as a seam, and a family + // this file names is a family that can be missing. + bar.style.fontFamily = font || getComputedStyle(document.body).fontFamily; + }, cfg.font).catch(() => {}); + return checkOverlay(page); +} + +/** + * The design rules that are measurable in the page, as a check that fails the + * take rather than the render. Everything here is a rule from + * DESIGN-LANGUAGE.md; the geometry ones exist because a wrong `zoom` puts a + * perfectly styled plate in the wrong place and nothing looks wrong until the + * capture is cut against a composed frame. + * + * Returns { ok, problems }. Throws in strict mode (the default). + */ +async function checkOverlay(page) { + const rgb = (hex) => { + const h = hex.replace('#', ''); + return `rgb(${parseInt(h.slice(0, 2), 16)}, ${parseInt(h.slice(2, 4), 16)}, ${parseInt(h.slice(4, 6), 16)})`; + }; + const problems = await page.evaluate(([z, want, band, frameH]) => { + const out = []; + const bar = document.getElementById('demo-narration'); + const spot = document.getElementById('demo-spot'); + if (!bar || !spot) return ['overlay is not installed']; + const s = getComputedStyle(bar); + + if (s.boxShadow !== 'none') out.push(`caption plate has a box-shadow (${s.boxShadow}) — the plane is flat`); + for (const c of ['borderTopLeftRadius', 'borderTopRightRadius']) { + if (parseFloat(s[c]) > 0.5) out.push(`caption plate has a corner radius (${s[c]}) — the plane is flat`); + } + if (parseFloat(getComputedStyle(spot).borderTopLeftRadius) > 0.5) { + out.push('the pointer has a corner radius — the plane is flat'); + } + if (s.backgroundColor !== want.ground) { + out.push(`caption plate ground is ${s.backgroundColor}, expected ${want.ground}`); + } + if (/system-ui|Segoe UI|-apple-system|BlinkMacSystem/.test(s.fontFamily)) { + out.push(`caption font resolves to a system stack (${s.fontFamily}) — it will not exist on a clean build machine`); + } + // Geometry, in VIDEO pixels. The two coordinate spaces differ and it + // is measured, not assumed: under `html{zoom}` Chromium reports + // getBoundingClientRect() ALREADY zoom-adjusted (a 109.25px-tall plate + // at zoom 1.6842 measures 183.98) while getComputedStyle() still + // reports CSS px (109.25). So the rect is compared raw and the computed + // padding is multiplied. Getting this backwards is how a perfectly + // styled plate ends up 310px tall in the file. + // Measure where the plate SITS, not where it happens to be sliding + // through: install() runs both before the first raise (parked a full + // height below the fold) and again mid-take, where the 0.45s entrance + // transition may still be in flight — which read as a 5px band error + // once. Neutralising the transform and restoring it inside one + // synchronous block never reaches a paint, so nothing flashes. + const t0 = bar.style.transform, n0 = bar.style.transition; + bar.style.transition = 'none'; + bar.style.transform = 'translateY(0)'; + const r = bar.getBoundingClientRect(); + bar.style.transform = t0; + bar.style.transition = n0; + const height = Math.round(r.height); + const top = Math.round(r.top); + if (Math.abs(height - band) > 3) out.push(`caption plate is ${height} video px tall, the band is ${band} — check \`zoom\``); + if (Math.abs(top - (frameH - band)) > 3) out.push(`caption plate top is y${top}, the band starts at y${frameH - band}`); + const pad = parseFloat(getComputedStyle(document.body).paddingBottom) || 0; + if (Math.round(pad * z) < band - 3) out.push(`body reserves only ${Math.round(pad * z)} video px for a ${band}px plate — the plate will cover a control`); + // One accent event: it belongs to the pointer, and to nothing else. + if (JSON.stringify([s.color, s.backgroundColor, s.borderTopColor]).includes(want.accent)) { + out.push('the caption plate carries the accent — one accent event per frame, and it is the pointer'); + } + return out; + }, [cfg.zoom || 1, { ground: rgb(cfg.ground), accent: rgb(cfg.accent) }, BAND, FRAME.height]).catch((e) => [`overlay check failed: ${e.message}`]); + + if (problems.length) { + const msg = `narrate.js: overlay does not conform:\n - ${problems.join('\n - ')}`; + if (cfg.strict) throw new Error(msg); + console.warn(msg); + } + return { ok: problems.length === 0, problems }; } /** @@ -94,9 +267,18 @@ async function install(page) { * * The hold is derived from the length of the sentence, not from a fixed number: * a demo that gives every caption the same 2 seconds either rushes the long ones - * or stalls on the short ones. + * or stalls on the short ones. The 2.5s floor is the product-demonstration + * type's, and the hold is a FLOOR in the other direction too — when the screen + * takes longer to read than the caption, or when a narration line is longer + * than both, pass the measured duration as `holdMs`. */ async function say(page, text, stepLabel, opts = {}) { + const bad = text.match(BANNED_GLYPHS); + if (bad) { + throw new Error( + `narrate.js: caption contains ${JSON.stringify(bad[0])}, which renders as tofu ` + + `in the catalogue's fonts. Use ASCII ("->", "!=") or an inline SVG mark.\n ${text}`); + } await install(page); await page.evaluate(([t, s]) => { const bar = document.getElementById('demo-narration'); @@ -107,7 +289,7 @@ async function say(page, text, stepLabel, opts = {}) { }, [text, stepLabel]); const words = text.split(/\s+/).length; - const readMs = opts.holdMs || Math.max(2200, Math.round(words * 280)); + const readMs = opts.holdMs || Math.max(2500, Math.round(words * 280)); await page.waitForTimeout(readMs); } @@ -130,21 +312,31 @@ async function bringIntoView(page, selector) { await page.waitForTimeout(900); } -/** Draw attention to an element without touching it. */ +/** + * Draw attention to an element without touching it. + * + * The two coordinate spaces meet here and they do NOT agree: under + * `html{zoom}` Chromium reports getBoundingClientRect() in VIDEO pixels, while + * a style value is read back as CSS pixels and zoomed on the way out. Assigning + * the rect straight across therefore multiplies the position by the zoom — at + * 1.6842 a target at (168,202) was ringed at (274,330), which is a highlight + * sitting on the wrong control, or off the screen entirely, with nothing in the + * take saying so. Divide, then pad in video pixels like every other number. + */ async function point(page, selector) { await install(page); - await page.evaluate((sel) => { + await page.evaluate(([sel, z]) => { const spot = document.getElementById('demo-spot'); const el = document.querySelector(sel); if (!spot || !el) return; const r = el.getBoundingClientRect(); - const pad = 6; - spot.style.left = (r.left - pad) + 'px'; - spot.style.top = (r.top - pad) + 'px'; - spot.style.width = (r.width + pad * 2) + 'px'; - spot.style.height = (r.height + pad * 2) + 'px'; + const pad = 10; // video px of air around the target + spot.style.left = ((r.left - pad) / z) + 'px'; + spot.style.top = ((r.top - pad) / z) + 'px'; + spot.style.width = ((r.width + pad * 2) / z) + 'px'; + spot.style.height = ((r.height + pad * 2) / z) + 'px'; spot.classList.add('on'); - }, selector).catch(() => {}); + }, [selector, cfg.zoom || 1]).catch(() => {}); await page.waitForTimeout(700); } @@ -183,4 +375,8 @@ async function typeSlowly(page, selector, value, perKeyMs = 140) { await unpoint(page); } -module.exports = { install, say, point, unpoint, bringIntoView, clickSlowly, typeSlowly }; +module.exports = { + configure, install, checkOverlay, say, point, unpoint, + bringIntoView, clickSlowly, typeSlowly, + SYSTEM, FRAME, BAND, BANNED_GLYPHS, +}; diff --git a/.claude/skills/mendix/record-narrated-demo/take.js b/.claude/skills/mendix/record-narrated-demo/take.js index a21f0edd2a..fb68d24a30 100644 --- a/.claude/skills/mendix/record-narrated-demo/take.js +++ b/.claude/skills/mendix/record-narrated-demo/take.js @@ -11,7 +11,11 @@ // Usage, from the per-project walkthrough script: // // const { openTake } = require('./take.js'); +// const narrate = require('./narrate.js'); // const take = await openTake(browser, { url: 'http://127.0.0.1:8080/', zoom: 1.68 }); +// narrate.configure({ zoom: 1.68 }); // THE SAME ZOOM — see narrate.js: +// // every overlay number is in video +// // pixels and divided by this one. // await take.goto(); // navigates, settles, starts the clock // take.mark('home'); // await take.click('.sd-key >> nth=0'); // paced, dialog-guarded diff --git a/CHANGELOG.md b/CHANGELOG.md index 91bf30580d..abc689ccc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **A narrated demo's caption plate and highlight ring were placed in the wrong coordinate space whenever the take used `zoom`** — `record-narrated-demo`'s `take.js` reaches a fixed-width Mendix layout with CSS `zoom` on `html`, and `narrate.js` was written as though that did not exist. Under `html{zoom:z}` Chromium reports `getBoundingClientRect()` in **zoom-adjusted (video) pixels** but `getComputedStyle()` and `style.*` in **CSS pixels**, and the overlay got it wrong in both directions at once: `point()` read a rect and assigned it straight to `style.left`, so the highlight landed at *position × zoom* — measured at 1.6842, a target at (168, 202) ringed at (274, 330), which is a ring around the wrong control or off the frame entirely — while the caption plate, declared 96 CSS px tall, reached the file at 162 video px against a 184 px caption band. + + Every overlay dimension is now stated in **video** pixels and divided by a zoom passed to `narrate.configure({ zoom })` — the same one handed to `openTake`. The reason this needed more than an arithmetic fix is that nothing reports it: the beat assertions pass, the contact sheet looks plausible, and the ring is simply around the wrong thing. So `checkOverlay()` measures the installed plate against the band and **throws** at record time rather than at edit time; its control is one line — build the overlay without telling it the zoom and it reports a 310 px plate starting at y770. + - **`ALTER PAGE` bound the replacement widget against the wrong data context** (mendixlabs/mxcli#1076) — `replace txtPreviewPeriod with { … }` inside a data view bound `datasource: selection lvVersions` re-scoped the binding to the **outer** data view's entity (`[CE1613] "The selected attribute 'Bug.Dashboard.PeriodLabel' no longer exists."`), and the same statement inside a Gallery or DataGrid 2 sourced by a **microflow/nanoflow** dropped the binding entirely (`[CE0402] "No value specified."`, `describe` renders `ContentParams: [{1} = ]`). `mxcli check --references` and `exec` both reported success; `CREATE PAGE` binds the same widget in the same position correctly, so the defect was in the ALTER walk alone. A widget's scope was resolved by **two separate walks that each knew a different subset** of Mendix's ten `Forms$*Source` kinds, which is the third time that split has produced a wrong binding (association and flow sources in FINDINGS #55, pluggable widgets in ako/mxcli#935). `Forms$ListenTargetSource` carries no `EntityRef` at all — only the listen target's *name* — so the entity walk saw no source and left the context at the enclosing data view; and the flow walk read only a widget's **top-level** `DataSource` key, so a pluggable list, whose source sits in `Object.Properties[datasource]`, was invisible to it. @@ -141,6 +145,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Measured end to end on 11.12.1, both engines: the reporter's repro now checks clean, and a project broken by the **pre-fix** binary is repaired by the fixed one — `Reconciled 1 access rule(s) in module ProbeSecond`, `mx check` 1 → 0. That also closes a gap an earlier fix recorded explicitly: `update security` had never been shown repairing a real CE0066, because once the write path reconciles, no MDL script can produce one. +### Changed + +- **`record-narrated-demo` is on the shared video design language** — the skill produces the capture that is most of a product-demonstration film's runtime, and its overlay was the one piece of furniture drawn to nothing in particular. It is now built to `video-system/DESIGN-LANGUAGE.md` in `ako/mxcli-intro-video`, which that repo makes normative: one accent and it belongs to the pointer, a flat plane (no shadow under the plate, no corner radius, the pointer pulsing on opacity rather than an expanding glow), and the plate filling the caption band — `y896–1080`, text at the `x96` safe-area gutter — which every other film in the catalogue keeps clear. + + Two of those are correctness rather than taste. The plate asked for `'Segoe UI'`, which does not exist on a clean build machine and falls back to DejaVu Sans with nothing saying so; it now inherits the **app's own computed body font**, which is also the seam-free choice for furniture wrapping that app's UI. And captions had no glyph guard: `say()` now refuses `≠ → ✓ ✗` and the rest of the tofu set, for the same reason ligatures are off everywhere in the system — a caption that quotes what the app said and silently rewrites a character is a claim about the tool. + + The keep-alive that stops a reading pause collapsing to no frames is now a hairline segment sweeping the plate's top edge, not a spinning ring: with no rounded corners in the system a ring can only be a spinning square, so the guarantee had to be re-expressed in the system's own vocabulary. It still runs continuously rather than per caption, because a hold *between* captions has to produce frames too. + + Narration is pinned to the catalogue's voice and levels — Kokoro `bm_george` (a demo arriving in a different voice breaks the family harder than any visual difference), two-pass `loudnorm` to `I=-18.6:TP=-1.5` where the skill previously said -16 and would have made the demo the loudest thing in the catalogue, a `-30 LUFS` bed, and per-line verification because TTS has failed both 0-of-12 and 10-of-12 while passing a total-duration check. + ### Added - **`mxcli lint` reports a navigation screen that cannot be linked to (CONV019)** — a Mendix page is reachable at `/p/` only if it has been given a URL; without one it exists solely at the end of a click path and cannot be bookmarked, shared, reopened after a refresh, or captured with `--screenshot-url` (ako/CapTrackV4 FINDINGS 014). From 990e9419593a4205acd1bda082b2ae2bcc68d6a6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 08:33:48 +0000 Subject: [PATCH 04/11] docs: record the error-handler population MDL-FLOW01 cannot see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit structure.go's successors() drops every IsErrorHandler edge before building the graph, so an error path that rejoins the normal one scores zero in the prevalence table — not because it is rare, but because the detector is blind to it by construction. That is a third uncounted population beside the two already noted, and it needs the same Mode 2 merge/join syntax by a different route. Measured across three whole projects (235 microflows, user-written modules included, unlike the marketplace table): 7 carry a true error-handler flow, 1 rejoins the normal path, in 3 of 3 projects, always at an ExclusiveMerge. That one microflow is what DESCRIBE flattens to an empty `on error { }` and what produced the CE0709 over-connected end event fixed in #450. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ --- ...OPOSAL_structured_microflow_description.md | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/docs/11-proposals/PROPOSAL_structured_microflow_description.md b/docs/11-proposals/PROPOSAL_structured_microflow_description.md index 80eaffeb1f..14513984e2 100644 --- a/docs/11-proposals/PROPOSAL_structured_microflow_description.md +++ b/docs/11-proposals/PROPOSAL_structured_microflow_description.md @@ -325,7 +325,7 @@ render as ordinary nested `if`s. So **Mode 3 earns its cost**, and Mode 2 is the honest fallback for the ~20 % that stay crossed. Scheduling is still a separate call; what is settled is that Mode 3 is not speculative work. -Two caveats that matter more than the percentages: +Three caveats that matter more than the percentages: - **The shipped lint rule never sees this corpus.** `LintContext.Microflows()` filters through `notPlatformModule`, so `mxcli lint` deliberately skips @@ -339,6 +339,25 @@ Two caveats that matter more than the percentages: `RULE`-typed flows (and the 39 nanoflows) in this project, so the rule skips them silently. A rule is "a special kind of microflow" and can branch, so this is a real gap in the detector's reach, not just in this measurement. +- **Error-handler rejoins are excluded by construction, and are a separate + population needing the same syntax.** `successors()` in + `mdl/microflowgraph/structure.go` skips every flow with `IsErrorHandler` before + the graph is built, so an error path that rejoins the normal one contributes + **zero** to the 5.8 % — not because it is rare, but because the detector cannot + see it. Measured across three whole projects (ako/TestApp, CapTrack, RestLab — + 235 microflows, so **user-written modules included**, unlike the table above): + 7 microflows carry a true error-handler flow, and **1 of them rejoins the + normal path** — `FeedbackModule.SUB_Feedback_SendToServer`, in 3 of 3 projects, + always landing on a `Microflows$ExclusiveMerge`. That single microflow is what + DESCRIBE flattens into an empty `on error … { }` block, and what produced the + CE0709 over-connected end event fixed in #450. The control that the scan + discriminates rather than flagging every handler: the other 6 — including one + in an app module — terminate on their own end event and are not flagged, and + the same scan over the *mxcli-rewritten* copy of one project reports the rejoin + landing on an `EndEvent` instead of a merge, which is the #450 defect showing + up in the measurement. So the fix here is not "detect more": the syntax an + error rejoin needs is Mode 2's `merge`/`join` labels, reached by a different + route. Method and control, since a rule that never runs and a rule that finds nothing look identical: the scan was instrumented to report what it actually examined — From c5faf94f1386b719c299bca7125037462b49e20e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 08:38:36 +0000 Subject: [PATCH 05/11] docs: plan for error-handler rejoins (Phase E) Measured the failure the caveat implies rather than asserting it: repointing SUB_Feedback_SendToServer's error edge from the tail merge to the merge before the AppId split gives two behaviourally different graphs that DESCRIBE renders as the same MDL, differing only in a layout annotation and carrying no warning. Executing that MDL reproduces the first graph in both cases, so the upstream rejoin is silently rewritten while check, open and mxbuild all stay green. E0 (detect and warn) is independently shippable and worth doing regardless of Mode 2; E1 (join out of an on-error block) is a scoping rule on Mode 2's Phase 1 rather than a feature of its own; E2 pins the round trip using the mutated graph as the control. Also records what not to do: folding error edges into successors() would move the prevalence figure by reclassifying, since post-dominance there treats a successor as a branch of a condition. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ --- ...OPOSAL_structured_microflow_description.md | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/docs/11-proposals/PROPOSAL_structured_microflow_description.md b/docs/11-proposals/PROPOSAL_structured_microflow_description.md index 14513984e2..938eb20454 100644 --- a/docs/11-proposals/PROPOSAL_structured_microflow_description.md +++ b/docs/11-proposals/PROPOSAL_structured_microflow_description.md @@ -402,6 +402,60 @@ Two deviations from the plan above, both deliberate: best-practices grade; the model here is valid and builds cleanly, so docking a project's score for an mxcli limitation would be wrong. +### Phase E — error-handler rejoins + +The third caveat above is a separate population with its own phasing, because the +detector cannot reach it and the failure mode is worse than the one Phase 0 +addresses. Where an irreducible split describes to MDL that is *unfaithful but +visible* (the reader can see the structure was flattened), an error rejoin +describes to MDL that is **wrong and indistinguishable from correct**. + +**The measurement.** Take `FeedbackModule.SUB_Feedback_SendToServer` and repoint +its error edge from the tail merge to the merge just before the `AppId` split — +i.e. from "on error, return empty" to "on error, re-enter the split". Two graphs, +different behaviour. `DESCRIBE` emits **the same MDL for both**, differing only in +a `@merge(230, 160)` layout annotation, with no warning; and executing that MDL +produces the first graph in both cases. So the upstream rejoin is silently +rewritten into a tail return, `mxcli check` is clean, the project opens and +mxbuild is green. (The unmutated microflow round-trips correctly — error → merge → +the same end event as the `else` branch — but by luck: the empty handler falls +through to the enclosing branch's continuation, which happens to be that end +event. That is also why #450's post-pass was enough to make it *buildable*.) + +- **E0 — detect and warn (independently shippable).** Ask, of each custom error + handler, whether the node it reaches is reachable from the start over normal + edges only. If it is, the handler cannot be spelled: emit the `-- WARNING:` line + beside the #923 one, and a lint finding. The reachability query is on a graph + the describer already has in hand — `collectErrorHandlerStatements` computes + `firstReachableErrorHandlerMerge` today and then silently returns an empty + block. This turns a silent rewrite into a named refusal, the same move Phase 0 + made for irreducible splits, and it is worth doing whether or not Mode 2 ships. +- **E1 — spell it, as part of Mode 2's Phase 1.** `join