ci: enable downstream client smoke (pad/cli/desktop) — Phase 2

[JohnMcLear]

PR Summary by Qodo

CI: enable live downstream client smoke gate for pad/cli/desktop
🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Walkthroughs

User Description

Builds on #7923 (Phase 1). Turns on the live downstream-smoke gate now that each client has its Phase 2 vector + smoke tests.

What this adds

• src/tests/downstream/run-clients.sh — per-kind orchestration: for each enabled manifest entry, clone the client @ its pinned ref, set up its toolchain, point it at core's freshly generated fixture via ETHERPAD_WIRE_VECTORS, then run the client's vectorTest + smokeCmd against the booted server (ETHERPAD_SMOKE_URL + ETHERPAD_SMOKE_APIKEY). Clients test against current core's serialization, not a vendored snapshot.
• clients.json — all three flipped to enabled:true, pinned to the commits carrying their Phase 2 tests.
• downstream-smoke.yml — adds the Rust toolchain and calls the runner with the smoke env.

The three client PRs (each adds vendored fixture + vectorTest + live smokeCmd)

• test: downstream wire-compat (vectors + smoke) pad#1 (Rust crate: changeset decode vectors + PadSession round-trip)
• test: downstream wire-compat (vectors + smoke) etherpad-cli-client#136 (repo's first test runner; reuses its own Changeset/AttributePool)
• test: downstream wire-compat (vectors + smoke) etherpad-desktop#78 (fixture-integrity guard + headless-light HTTP roundtrip)

Verification

Ran the full orchestration locally against a real core — clone @ pinned SHA → inject fixture → vectors + live smoke — all green:

• etherpad-pad: vectors ✓, smoke ✓
• etherpad-cli-client: vectors 5/5 ✓, live USER_CHANGES round-trip ✓
• etherpad-desktop: vectors 22/22 ✓, smoke ✓

Follow-up

Once the three client PRs squash-merge, bump their refs in clients.json to the merge commits (the pinned pre-merge SHAs stay fetchable, so the gate keeps working until then).

🤖 Generated with Claude Code

AI Description

• Enable Phase 2 downstream-smoke gate to run real client vector + smoke tests.
• Add a per-client orchestrator that clones pinned refs, injects core vectors, and runs tests.
• Turn on pad/cli/desktop clients and install Rust toolchain in the workflow.

Diagram

graph TD
  wf["downstream-smoke.yml"] --> srv(["Booted core server"]) --> vec["Generate vectors"] --> fx["wire-vectors.json"] --> runner["run-clients.sh"] --> clients[["Downstream clients"]]
  manifest["clients.json"] --> runner
  clients --> srv

  subgraph Legend
    direction LR
    _cfg["Config/File"] ~~~ _svc(["Service"]) ~~~ _ext[["External component"]]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. GitHub Actions matrix (one job per client kind/name)

• ➕ Runs clients in parallel, reducing wall-clock time
• ➕ Clearer isolation/logging per client, easier to retry a single client
• ➕ Can tailor toolchain/caching per client kind (cargo/pnpm)
• ➖ More workflow complexity (matrix generation, passing vectors/artifacts)
• ➖ Needs careful artifact handling for wire-vectors.json and server URL/apikey

2. Package runner as a composite action / reusable workflow

• ➕ Reuses orchestration across repos/branches with a stable interface
• ➕ Simplifies the main workflow YAML and centralizes maintenance
• ➖ Adds indirection and versioning overhead
• ➖ Still needs the same cloning/toolchain logic internally

3. Avoid full clones (shallow fetch pinned commit)

• ➕ Less network/disk usage in CI; faster checkout of large clients
• ➖ More git edge cases around fetching non-tip SHAs; needs careful commands

Recommendation: The current approach (single runner script + manifest) is a good Phase 2 activation step: it’s straightforward, keeps client config data-driven, and makes local reproduction easy. If CI duration becomes an issue, consider moving to a matrix workflow for parallelism and better per-client isolation; also consider shallow-fetching pinned SHAs to reduce checkout cost.

File Changes

Other (3)

downstream-smoke.yml Install Rust toolchain and invoke Phase 2 downstream client runner +9/-12

Install Rust toolchain and invoke Phase 2 downstream client runner

• Adds a Rust toolchain setup step for the rust-kind downstream client and replaces the previous placeholder logic with a call to the new run-clients.sh orchestrator. Passes SMOKE_URL and SMOKE_APIKEY env vars into the runner so downstream clients can execute live smoke tests against the booted server.

.github/workflows/downstream-smoke.yml

---

clients.json Enable pad/cli/desktop downstream clients and update pinned refs +6/-6

Enable pad/cli/desktop downstream clients and update pinned refs

• Flips etherpad-pad, etherpad-cli-client, and etherpad-desktop to enabled=true and updates each entry to a pinned commit ref that contains their Phase 2 vector + smoke tests. Keeps per-client kind and command configuration in the manifest for the orchestrator.

src/tests/downstream/clients.json

---

run-clients.sh Add per-kind downstream client orchestrator (clone, inject vectors, run tests) +74/-0

Add per-kind downstream client orchestrator (clone, inject vectors, run tests)

• Introduces a bash runner that reads enabled clients from clients.json, clones each repo at its pinned ref into a temp workspace, exports ETHERPAD_WIRE_VECTORS/ETHERPAD_SMOKE_URL/ETHERPAD_SMOKE_APIKEY, and runs the configured vector + smoke commands. Handles kind-specific setup (pnpm install for node/desktop; relies on Rust toolchain for rust) and aggregates failures while continuing to run subsequent clients.

src/tests/downstream/run-clients.sh

---

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

✅ Tickets: 🎫 Mobile support and some low hanging CSS fruits [low priority probably]

1. Clone failure aborts loop 🐞 Bug ☼ Reliability

Description

run-clients.sh runs git clone/fetch/checkout under set -e before the per-client `|| { ...
fail=1; }` handler, so a single network/ref failure exits the whole script and skips remaining
clients. The git fetch ... || true also suppresses the root cause, making the subsequent
checkout failure harder to diagnose.

Code

src/tests/downstream/run-clients.sh[R16-53]

+set -euo pipefail
+
+REPO_ROOT="$(cd "$(dirname "$0")/../../.." && pwd)"
+MANIFEST="${MANIFEST:-$REPO_ROOT/src/tests/downstream/clients.json}"
+WIRE_VECTORS="${WIRE_VECTORS:-$REPO_ROOT/src/tests/fixtures/wire-vectors.json}"
+SMOKE_URL="${SMOKE_URL:-http://localhost:9003}"
+SMOKE_APIKEY="${SMOKE_APIKEY:-}"
+
+[ -f "$WIRE_VECTORS" ] || { echo "::error::fixture not found: $WIRE_VECTORS"; exit 1; }
+
+WORK="$(mktemp -d)"
+trap 'rm -rf "$WORK"' EXIT
+
+MANIFEST="$MANIFEST" node -e '
+  const c = require(process.env.MANIFEST).filter((x) => x.enabled);
+  for (const x of c) {
+    process.stdout.write([x.name, x.repo, x.ref, x.kind, x.vectorTest, x.smokeCmd].join("\t") + "\n");
+  }
+' > "$WORK/clients.tsv"
+
+if [ ! -s "$WORK/clients.tsv" ]; then
+  echo "No downstream clients enabled. Nothing to run."
+  exit 0
+fi
+
+export ETHERPAD_WIRE_VECTORS="$WIRE_VECTORS"
+export ETHERPAD_SMOKE_URL="$SMOKE_URL"
+export ETHERPAD_SMOKE_APIKEY="$SMOKE_APIKEY"
+
+fail=0
+while IFS=$'\t' read -r name repo ref kind vectorTest smokeCmd; do
+  echo "::group::$name ($kind) @ ${ref:0:12}"
+  dir="$WORK/$name"
+  git clone --quiet "$repo" "$dir"
+  # Fetch the exact pinned commit (works even when it is not a branch tip).
+  git -C "$dir" fetch --quiet origin "$ref" 2>/dev/null || true
+  git -C "$dir" checkout --quiet "$ref"
+

Evidence

The script enables set -euo pipefail, performs git operations before the only per-client failure
handler, and explicitly ignores fetch errors, so failures in clone/checkout will terminate the
script rather than being converted into fail=1 and continuing to the next client.

src/tests/downstream/run-clients.sh[16-53]
src/tests/downstream/run-clients.sh[54-72]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/tests/downstream/run-clients.sh` intends to continue running other clients even if one fails (it uses `fail=1`), but `git clone`/`fetch`/`checkout` happen outside the guarded block and are affected by `set -euo pipefail`. Any failure there aborts the entire run, skipping later clients. Additionally, fetch errors are currently suppressed (`|| true` + `2>/dev/null`), which can hide the real failure and produce a confusing checkout error.

## Issue Context
This runner is invoked by `.github/workflows/downstream-smoke.yml` and is meant to provide a stable multi-client gate. For robustness and debuggability, each client's checkout should either succeed or be reported as a per-client failure while allowing the loop to continue.

## Fix Focus Areas
- src/tests/downstream/run-clients.sh[16-53]
- src/tests/downstream/run-clients.sh[45-72]

## Suggested fix outline
- Move `git clone`/`fetch`/`checkout` into the same per-client guarded section as the tests, or add explicit `if ! ...; then` handling for each step that:
 - Emits a clear `::error::...` with repo/ref details.
 - Sets `fail=1` and `continue`s to the next client.
- Don’t suppress fetch errors; if you want a “best effort” fetch, still check whether the target commit exists locally before attempting checkout (e.g., `git cat-file -e "$ref^{commit}"`).
- Consider checking out via `FETCH_HEAD` when fetching by SHA succeeds to avoid ambiguity.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Eval makes commands brittle 🐞 Bug ⚙ Maintainability

Description

The runner executes vectorTest and smokeCmd via eval, which double-parses shell syntax and
makes quoting/escaping in clients.json error-prone (and allows the command string to alter the
current subshell environment). This increases maintenance risk as manifest commands evolve.

Code

src/tests/downstream/run-clients.sh[R54-65]

+  (
+    cd "$dir"
+    case "$kind" in
+      rust)
+        eval "$vectorTest"
+        eval "$smokeCmd"
+        ;;
+      node|desktop)
+        pnpm install
+        eval "$vectorTest"
+        eval "$smokeCmd"
+        ;;

Evidence

The runner explicitly uses eval to execute string fields, and those fields are stored as free-form
strings in clients.json, which makes correct escaping dependent on an extra shell-parse step.

src/tests/downstream/run-clients.sh[56-65]
src/tests/downstream/clients.json[1-28]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`run-clients.sh` uses `eval "$vectorTest"` and `eval "$smokeCmd"` to run manifest-provided command strings. `eval` performs an extra round of shell parsing/expansion, which is notoriously brittle (e.g., nested quotes, backslashes) and can cause surprising behavior as commands get more complex.

## Issue Context
The commands come from `src/tests/downstream/clients.json` and are intended to be simple. However, as clients add flags/args, `eval`-related escaping issues tend to crop up.

## Fix Focus Areas
- src/tests/downstream/run-clients.sh[54-65]
- src/tests/downstream/clients.json[1-28]

## Suggested fix outline
Choose one of:
1) Minimal change: replace `eval "$cmd"` with `bash -lc "$cmd"` to avoid `eval` and keep execution contained to a child shell.
2) More robust: change the manifest schema to store commands as argv arrays (e.g., `"vectorTest": ["cargo","test","--test","vectors"]`) and execute them without a shell (`"${vectorTest[@]}"`). This removes quoting hazards entirely.
3) If keeping strings: at least document supported syntax and add validation (no newlines/tabs) when generating `clients.tsv`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[JohnMcLear]

Thanks @qodo — both fixed in 7abf3ad:

1. Clone failure aborts loop — good catch, and it ran deeper than it looked: moving the git ops into the ( … ) || { fail=1; } subshell actually suspends set -e (the subshell is an || operand), so failures were being silently swallowed and the loop continued from the wrong cwd. Fixed by guarding every step (clone/fetch/checkout/cd/install/test) with an explicit || exit 1 so one client's failure becomes a per-client fail=1, the loop continues, and the run exits non-zero. Fetch errors are no longer suppressed — fetch runs only when the pinned commit isn't already reachable, and surfaces its real error.
2. eval brittleness — manifest commands now run via bash -c instead of eval.

Verified: two bogus clients → both reported, loop continues, exit 1; happy path (cli, no server) → vectors pass, smoke skips, exit 0.

[JohnMcLear]

Note: #7924 merged before this review was actioned, so the two fixes above landed as follow-up #7927 (cherry-picked onto develop) rather than on this branch.