ci(downstream): robust per-client error handling in run-clients.sh

[JohnMcLear]

Follow-up to #7924, which merged before its Qodo review was addressed. Lands the two Qodo findings (both real):

1. Clone failure aborted the whole run (silently). git clone/fetch/checkout ran outside the per-client failure guard. Worse: moving them into the ( … ) || { fail=1; } subshell suspends set -e (the subshell is an || operand), so a failed clone was silently swallowed and the loop continued from the wrong cwd — a broken/unreachable client would have passed the gate. Fixed by guarding every step (clone/fetch/checkout/cd/install/test) with 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 only when the pinned commit isn't already reachable).
2. eval brittleness — manifest commands now run via bash -c (trusted in-repo allowlist; no double-parse, no leak into this script's shell).

Verification

• Two bogus clients → both reported (::error::downstream client …), loop continues, exit 1.
• Happy path (cli, no server) → vectors pass, smoke skips cleanly, exit 0.

🤖 Generated with Claude Code

[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 (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. Child bash not strict ✓ Resolved 🐞 Bug ☼ Reliability

Description

run-clients.sh sets set -euo pipefail, but the manifest commands now run under a fresh bash -c
without -e/-u/-o pipefail, so masked failures (especially pipeline stage failures) or
unset-variable mistakes inside vectorTest/smokeCmd may not surface as non-zero exits.

Code

src/tests/downstream/run-clients.sh[R69-75]

+        bash -c "$vectorTest" || exit 1
+        bash -c "$smokeCmd" || exit 1
      ;;
    node|desktop)
-        pnpm install
-        eval "$vectorTest"
-        eval "$smokeCmd"
+        pnpm install || exit 1
+        bash -c "$vectorTest" || exit 1
+        bash -c "$smokeCmd" || exit 1

Evidence

The script enables strict mode at the top, but the new bash -c calls run in a separate shell with
default options unless explicitly enabled; the manifest strings are sourced from clients.json
(vectorTest/smokeCmd).

src/tests/downstream/run-clients.sh[16-16]
src/tests/downstream/run-clients.sh[67-76]
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` is in strict mode (`set -euo pipefail`), but the newly introduced `bash -c "$vectorTest"` / `bash -c "$smokeCmd"` executes those manifest commands in a new Bash process that does **not** automatically inherit `-e`, `-u`, or `pipefail`. This can allow certain failures inside the command string (notably pipeline stage failures without `pipefail`) to be missed.
### Issue Context
The manifest commands come from `clients.json` and are intended to be robustly checked. The outer script only sees the exit status of the child `bash -c` process.
### Fix Focus Areas
- src/tests/downstream/run-clients.sh[68-76]
### Suggested change
Update the command execution to enable strict mode in the child bash, for example:
- `bash -eu -o pipefail -c "$vectorTest" || exit 1`
- `bash -eu -o pipefail -c "$smokeCmd" || exit 1`
(Apply similarly for both `rust` and `node|desktop` cases.)

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

---

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

PR Summary by Qodo

Fix downstream runner to isolate per-client failures and avoid eval in run-clients.sh
🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

Walkthroughs

User Description

Follow-up to #7924, which merged before its Qodo review was addressed. Lands the two Qodo findings (both real):

1. Clone failure aborted the whole run (silently). git clone/fetch/checkout ran outside the per-client failure guard. Worse: moving them into the ( … ) || { fail=1; } subshell suspends set -e (the subshell is an || operand), so a failed clone was silently swallowed and the loop continued from the wrong cwd — a broken/unreachable client would have passed the gate. Fixed by guarding every step (clone/fetch/checkout/cd/install/test) with 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 only when the pinned commit isn't already reachable).
2. eval brittleness — manifest commands now run via bash -c (trusted in-repo allowlist; no double-parse, no leak into this script's shell).

Verification

• Two bogus clients → both reported (::error::downstream client …), loop continues, exit 1.
• Happy path (cli, no server) → vectors pass, smoke skips cleanly, exit 0.

🤖 Generated with Claude Code

AI Description

• Prevent one downstream client clone/checkout failure from silently passing the gate.
• Make each client run fail independently while continuing the loop and exiting non-zero.
• Replace manifest command execution via eval with bash -c to avoid double-parsing.

Diagram

graph TD
  A(["CI job"]) --> B["run-clients.sh"] --> C["clients.json"] --> D{"Per-client loop"} --> E["Clone/fetch/checkout"] --> F["Install + tests"] --> H(["Exit (fail)"])
  E --> G["::error:: + fail=1"] --> D
  F --> G
  subgraph Legend
    direction LR
    _start(["Start/End"]) ~~~ _proc["Process"] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Function-per-client with explicit return codes (no subshell-as-`||` operand)

• ➕ Avoids the surprising set -e suppression that happens in certain conditional contexts
• ➕ Can centralize error handling/logging and reduce repetitive || exit 1 guards
• ➖ Still requires careful Bash semantics (e.g., if/! also affect set -e behavior)
• ➖ Slightly more structural churn for marginal gain over the current explicit guards

2. Run each client as a separate job/step (matrix)

• ➕ Hard isolation between clients; clearer per-client logs and statuses
• ➕ Native CI retry/timeout controls per client
• ➖ Higher CI orchestration complexity and longer overall wall time due to reduced sharing
• ➖ Requires pipeline changes beyond this script and may complicate local reproduction

Recommendation: The PR’s approach is appropriate for a single-script runner: explicit || exit 1 guards reliably enforce per-client failure semantics despite set -e corner cases, and bash -c removes eval double-parsing risk while keeping trusted manifest commands. Alternatives mostly add orchestration/complexity without materially improving correctness for this use case.

File Changes

Bug fix (1)

run-clients.sh Isolate clone/checkout/test failures per client and replace eval with bash -c +23/-12

Isolate clone/checkout/test failures per client and replace eval with bash -c

• Moves git clone/fetch/checkout into the per-client guarded subshell and explicitly guards each step with '|| exit 1' to avoid 'set -e' being suspended under '(...) || ...'. Fetch is now conditional (only when the pinned ref is not reachable) and no longer suppresses errors. Manifest-provided commands are executed via 'bash -c' instead of 'eval', improving robustness and preventing leakage into the runner shell.

src/tests/downstream/run-clients.sh

---

[JohnMcLear]

Updated: now that the three client PRs have merged, this PR also bumps the manifest refs to their squash-merge commits (pad→91620c6, cli→ebc516e, desktop→ab83da6). This is needed because the previous pinned refs were the pre-merge PR-branch tips, which were deleted on merge and are no longer fetchable. The downstream-smoke run on this PR now clones the clients at their real merged main commits.

[JohnMcLear]

Fixed in the latest commit, thanks @qodo — manifest commands now run under bash -euo pipefail -c so pipeline-stage failures (and unset-var mistakes) inside a client's vectorTest/smokeCmd surface as a non-zero exit instead of being masked. Verified bash -euo pipefail -c 'false | cat' now exits non-zero, and the per-client failure aggregation still works (two bogus clients → 2 reported errors, exit 1).