From b8ddd44219eaf48e07588fd602b2201a0f8ba853 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Tue, 4 Aug 2026 15:50:40 +0200 Subject: [PATCH 1/5] Pass sampler template names newline-separated so spaced names survive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Account template identifiers are directory names and routinely contain spaces ("Investment- and depreciation details"). The sampler encoded the handles/account_templates inputs as space-separated strings and then word-split them into CLI args, so -at received "Investment-", "and", "depreciation", "details" and the run died with: [error] Config file for account template "Investment-" not found Switch both inputs to one-name-per-line and build the arg arrays with mapfile, so each line becomes exactly one argv entry. This is already the convention in run_tests.yml (TEMPLATE_BUCKETS) — the sampler was the only place that regressed to space-joining. firm_ids stays space-separated; it's numeric. Also render the names in the PR comment as individually code-quoted, comma-separated entries (a multi-line value inside one backtick pair renders as garbage) and %q-quote the attempt log line so a spaced name is visibly one argument. Callers must now pass these inputs newline-separated — README and the input descriptions updated accordingly. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/run_sampler.yml | 37 ++++++++++++++++++++++--------- README.md | 5 +++-- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index a7e29dc..8356ab9 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -33,12 +33,12 @@ on: required: true type: string handles: - description: "Reconciliation text handles to sample, space-separated (directory names under reconciliation_texts/). Optional if account_templates is set." + description: "Reconciliation text handles to sample, ONE PER LINE (directory names under reconciliation_texts/). Newline-separated, not space-separated — names may contain spaces. Optional if account_templates is set." required: false type: string default: "" account_templates: - description: "Account template names to sample, space-separated (directory names under account_templates/). Optional if handles is set." + description: "Account template names to sample, ONE PER LINE (directory names under account_templates/). Newline-separated, not space-separated — these names routinely contain spaces. Optional if handles is set." required: false type: string default: "" @@ -170,14 +170,22 @@ jobs: SAMPLER_ACCOUNT_TEMPLATES: ${{ inputs.account_templates }} SAMPLER_FIRM_IDS: ${{ inputs.firm_ids }} run: | - # Intentional word-splitting: handles/account templates arrive as a single - # space-separated string and must become separate CLI args (shellcheck SC2206). + # One name per line, and each line becomes exactly ONE CLI arg. Newline-separated (not + # space-separated) keeps identifiers that contain spaces intact — account template + # names do (e.g. "Investment- and depreciation details"), and word-splitting them + # handed the CLI "Investment-" as a template name: + # [error] Config file for account template "Investment-" not found + # Same convention as run_tests.yml's TEMPLATE_BUCKETS. HANDLE_ARGS=() - # shellcheck disable=SC2206 - [[ -n "${SAMPLER_HANDLES}" ]] && HANDLE_ARGS=(-h ${SAMPLER_HANDLES}) + if [[ -n "${SAMPLER_HANDLES//[[:space:]]/}" ]]; then + mapfile -t HANDLE_NAMES < <(printf '%s\n' "${SAMPLER_HANDLES}" | sed '/^[[:space:]]*$/d') + HANDLE_ARGS=(-h "${HANDLE_NAMES[@]}") + fi ACCOUNT_ARGS=() - # shellcheck disable=SC2206 - [[ -n "${SAMPLER_ACCOUNT_TEMPLATES}" ]] && ACCOUNT_ARGS=(-at ${SAMPLER_ACCOUNT_TEMPLATES}) + if [[ -n "${SAMPLER_ACCOUNT_TEMPLATES//[[:space:]]/}" ]]; then + mapfile -t ACCOUNT_NAMES < <(printf '%s\n' "${SAMPLER_ACCOUNT_TEMPLATES}" | sed '/^[[:space:]]*$/d') + ACCOUNT_ARGS=(-at "${ACCOUNT_NAMES[@]}") + fi # GitHub Actions concurrency (group + queue: max) serializes runs within THIS repo, but # concurrency groups do not span repositories — two market repos sharing this partner id @@ -187,7 +195,10 @@ jobs: DEADLINE=$(( $(date +%s) + 90*60 )) ATTEMPT=1 while true; do - echo "[$(date -u +%H:%M:%S)] run-sampler attempt ${ATTEMPT}: -p ${SAMPLER_PARTNER} ${HANDLE_ARGS[*]} ${ACCOUNT_ARGS[*]} --firm-ids ${SAMPLER_FIRM_IDS} --compact" + # %q-quoted so a name containing spaces is visibly one argument in the log. + printf '[%s] run-sampler attempt %s: -p %s' "$(date -u +%H:%M:%S)" "${ATTEMPT}" "${SAMPLER_PARTNER}" + printf ' %q' "${HANDLE_ARGS[@]}" "${ACCOUNT_ARGS[@]}" + printf ' --firm-ids %s --compact\n' "${SAMPLER_FIRM_IDS}" set +e # shellcheck disable=SC2086 # SAMPLER_FIRM_IDS is intentionally word-split (space-separated ids -> separate args) OUTPUT=$(node ./node_modules/silverfin-cli/bin/cli.js run-sampler -p "${SAMPLER_PARTNER}" "${HANDLE_ARGS[@]}" "${ACCOUNT_ARGS[@]}" --firm-ids ${SAMPLER_FIRM_IDS} --compact 2>&1 | tr -d '\r') @@ -361,8 +372,12 @@ jobs: lines.push(`⚠️ The sampler run did not complete cleanly — see the [workflow run](${runUrl}) for details.`); } lines.push(""); - if (process.env.HANDLES) lines.push(`- Reconciliation handles: \`${process.env.HANDLES}\``); - if (process.env.ACCOUNT_TEMPLATES) lines.push(`- Account templates: \`${process.env.ACCOUNT_TEMPLATES}\``); + // Newline-separated on the way in (names can contain spaces) — render one code-quoted + // name per entry, comma-separated. A multi-line value inside one backtick pair + // renders as garbage in a PR comment. + const fmtNames = v => (v || "").split("\n").map(s => s.trim()).filter(Boolean).map(s => `\`${s}\``).join(", "); + if (process.env.HANDLES) lines.push(`- Reconciliation handles: ${fmtNames(process.env.HANDLES)}`); + if (process.env.ACCOUNT_TEMPLATES) lines.push(`- Account templates: ${fmtNames(process.env.ACCOUNT_TEMPLATES)}`); lines.push(`- Firm(s): \`${process.env.FIRM_IDS}\``); if (artifactUrl) { lines.push(`- **[📊 Open full sampler report](${artifactUrl})** (GitHub sign-in required; downloads \`results.zip\`, kept 7 days)`); diff --git a/README.md b/README.md index 8f292fd..10a3475 100644 --- a/README.md +++ b/README.md @@ -260,8 +260,9 @@ _Trigger:_ _Inputs:_ * `partner` (required) — partner environment id (must be authorized — see `PARTNER_CONFIG_JSON` secret). -* `handles` (optional) — reconciliation text handles to sample, space-separated (directory names under `reconciliation_texts/`). Optional if `account_templates` is set. -* `account_templates` (optional) — account template names to sample, space-separated (directory names under `account_templates/`). Optional if `handles` is set. +* `handles` (optional) — reconciliation text handles to sample, **one per line** (directory names under `reconciliation_texts/`). Optional if `account_templates` is set. +* `account_templates` (optional) — account template names to sample, **one per line** (directory names under `account_templates/`). Optional if `handles` is set. + * Both lists are newline-separated, **not** space-separated: account template directory names routinely contain spaces (e.g. `Investment- and depreciation details`), so a space-joined list is ambiguous and gets word-split into template names that don't exist (`Config file for account template "Investment-" not found`). Same convention as [`run_tests.yml`](#run-liquid-tests-run_testsyml). `firm_ids` is the exception — numeric, so it stays space-separated. * `firm_ids` (required) — firm id(s) to sample against, space-separated. The backend 422s if empty. * `ref` (required) — git ref (commit SHA) to check out — the PR head, so sampled template content matches the PR under review. * `pull_request_number` (optional) — PR number to post the result comment on. If empty, no comment is posted (results still upload as an artifact). From ddcb0af5ff0630c4c3b2802608163581f9aafefe Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Tue, 4 Aug 2026 16:29:28 +0200 Subject: [PATCH 2/5] Escape backticks when rendering template names in the PR comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Template names are directory names taken from the PR's own tree, so they are untrusted text. Wrapping them in a single backtick pair let a name containing a backtick close its code span and inject markdown into the sampler's PR comment (CWE-116; spoofed links/formatting, not code execution) — flagged by CodeRabbit on bso_github_actions#37. Fence each name per CommonMark instead: one more backtick than the longest run inside the name, plus a space pad when it starts or ends with one. Names without backticks render exactly as before. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/run_sampler.yml | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 8356ab9..7214ad2 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -375,7 +375,18 @@ jobs: // Newline-separated on the way in (names can contain spaces) — render one code-quoted // name per entry, comma-separated. A multi-line value inside one backtick pair // renders as garbage in a PR comment. - const fmtNames = v => (v || "").split("\n").map(s => s.trim()).filter(Boolean).map(s => `\`${s}\``).join(", "); + // + // These names are directory names from the PR's own tree, so treat them as untrusted + // text: a name containing a backtick would otherwise close its code span and inject + // markdown into this comment. Per CommonMark, fence with one more backtick than the + // longest run inside the name, and pad when it starts/ends with a backtick. + const codeSpan = (s) => { + const longest = Math.max(0, ...[...s.matchAll(/`+/g)].map((m) => m[0].length)); + const fence = "`".repeat(longest + 1); + const pad = s.startsWith("`") || s.endsWith("`") ? " " : ""; + return `${fence}${pad}${s}${pad}${fence}`; + }; + const fmtNames = v => (v || "").split("\n").map(s => s.trim()).filter(Boolean).map(codeSpan).join(", "); if (process.env.HANDLES) lines.push(`- Reconciliation handles: ${fmtNames(process.env.HANDLES)}`); if (process.env.ACCOUNT_TEMPLATES) lines.push(`- Account templates: ${fmtNames(process.env.ACCOUNT_TEMPLATES)}`); lines.push(`- Firm(s): \`${process.env.FIRM_IDS}\``); From 5b88bf37b986627e7603776f05f83015c7c9319a Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Tue, 4 Aug 2026 16:31:36 +0200 Subject: [PATCH 3/5] Reject template names starting with "-" before invoking the CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Template names are directory names from the PR's own tree. A name starting with "-" is read by the CLI's option parser as a FLAG rather than a value: commander stops consuming a variadic option (-h/-at take ) at the first "-"-prefixed token, so a directory named e.g. "--from-zip" would be reinterpreted as an option instead of a template to sample. A leading "--" separator does not protect variadic values, so the fix is an explicit guard: fail the step with an actionable message (rename the directory) before any CLI call. Flagged by CodeRabbit as CWE-88 on lu_market#785. Names with internal or trailing hyphens ("Cut-off", "Investment- and depreciation details") are unaffected — only a leading "-" is rejected. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/run_sampler.yml | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 7214ad2..77770ad 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -176,14 +176,33 @@ jobs: # handed the CLI "Investment-" as a template name: # [error] Config file for account template "Investment-" not found # Same convention as run_tests.yml's TEMPLATE_BUCKETS. - HANDLE_ARGS=() + HANDLE_NAMES=() + ACCOUNT_NAMES=() if [[ -n "${SAMPLER_HANDLES//[[:space:]]/}" ]]; then mapfile -t HANDLE_NAMES < <(printf '%s\n' "${SAMPLER_HANDLES}" | sed '/^[[:space:]]*$/d') - HANDLE_ARGS=(-h "${HANDLE_NAMES[@]}") fi - ACCOUNT_ARGS=() if [[ -n "${SAMPLER_ACCOUNT_TEMPLATES//[[:space:]]/}" ]]; then mapfile -t ACCOUNT_NAMES < <(printf '%s\n' "${SAMPLER_ACCOUNT_TEMPLATES}" | sed '/^[[:space:]]*$/d') + fi + + # A name starting with "-" would be read by the CLI's option parser as a FLAG rather + # than a value: commander stops consuming a variadic option (-h/-at take ) at + # the first "-"-prefixed token, and a leading "--" separator does not protect variadic + # values either. Reject such a name up front instead of letting the CLI silently + # reinterpret it as an option — the actionable fix is to rename the template directory. + for NAME in "${HANDLE_NAMES[@]}" "${ACCOUNT_NAMES[@]}"; do + if [[ "${NAME}" == -* ]]; then + echo "::error::Template name '${NAME}' starts with '-', which silverfin-cli would parse as an option instead of a template name. Rename the template directory." + exit 1 + fi + done + + HANDLE_ARGS=() + if [[ ${#HANDLE_NAMES[@]} -gt 0 ]]; then + HANDLE_ARGS=(-h "${HANDLE_NAMES[@]}") + fi + ACCOUNT_ARGS=() + if [[ ${#ACCOUNT_NAMES[@]} -gt 0 ]]; then ACCOUNT_ARGS=(-at "${ACCOUNT_NAMES[@]}") fi From cb7d387eb78cf825f63c6a03011e66662d8b9214 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 5 Aug 2026 09:18:40 +0200 Subject: [PATCH 4/5] Re-pin silverfin-cli to the current sampler branch tip (1ba5a0f) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pin added in #35 (5accd6b) is not an ancestor of silverfin-cli's sampler-compact-diff-v2 branch: it's a pre-rebase copy of "Make --add-diffs-folder's zip write atomic" that the rebase orphaned. It still runs, so nothing failed loudly — it just silently froze this workflow five fixes behind the branch it claims to track: 0bf8f15 Skip the diffs/ zip rewrite when no view.html is found b148ffa Flag-array dedup, diffs-folder count, radio groups 4598087 Flag-array flip direction + radio-checked attribute boundary 8b05852 Flag-flip suffix only on the after side of a results diff 1ba5a0f Keep byte-identical render pairs out of the diffs/ folder That last one matters most for adopters of this workflow: without it, --add-diffs-folder copies before/after view.html for entries flagged on a data-only change, so diffs/ fills with byte-identical pairs (lu_market run 30920292551 produced 3 such pairs and nothing else). The market wrappers don't have this problem because they install #sampler-compact-diff-v2, the branch, and so track fixes automatically. Documented that re-pinning is required whenever that branch moves, until silverfin-cli#265 merges and this goes back to unpinned main. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/run_sampler.yml | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 77770ad..2e02ae4 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -131,10 +131,17 @@ jobs: - name: Install silverfin-cli run: | # Pinned to a commit on silverfin-cli's sampler-compact-diff-v2 branch, since - # --add-diffs-folder (used below) is not yet on main. Switch back to installing - # unpinned main, matching every other production workflow's convention, once that - # branch's PR merges. - npm install https://github.com/silverfin/silverfin-cli.git#5accd6b + # --compact/--from-zip/--add-diffs-folder (used below) are not yet on main. Switch back + # to installing unpinned main, matching every other production workflow's convention, + # once that branch's PR (silverfin-cli#265) merges. + # + # Re-pin to the branch tip whenever that branch gains fixes — a commit pin silently goes + # stale, and a rebase can orphan it entirely. The previous pin (5accd6b) was exactly + # that: a pre-rebase copy of "Make --add-diffs-folder's zip write atomic", no longer an + # ancestor of the branch, and missing five later fixes to the compact diff (flag-array + # flip direction, radio-checked attribute boundary, flag-flip suffix side, the + # no-view.html rewrite skip, and the identical-render diffs/ skip). + npm install https://github.com/silverfin/silverfin-cli.git#1ba5a0f VERSION=$(node ./node_modules/silverfin-cli/bin/cli.js -V) echo "CLI version: ${VERSION}" From f7397bc47dc58200aa91ab5003c67421c930966f Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 5 Aug 2026 09:32:32 +0200 Subject: [PATCH 5/5] Name the value kind in the leading-hyphen rejection, and document it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review points from CodeRabbit on #37: 1. The guard checks handles AND account templates, but the error called every value a "Template name" and told the user to rename "the template directory" — wrong words for a reconciliation handle. Rather than a neutral label, check each list separately so the message names the kind ("Reconciliation handle" / "Account template name") and the directory to rename (reconciliation_texts/ / account_templates/). 2. The restriction wasn't part of the documented input contract. Added it to both `handles`/`account_templates` input descriptions and the README input list, including why (variadic -h/-at stop at the first "-"-prefixed token; "--" doesn't protect variadic values) and that only a LEADING hyphen is affected — "Cut-off" and "Investment- and depreciation details" are fine. Verified in bash: empty list, both lists populated, hostile handle, and hostile account template each behave correctly (the empty-array call passes only the two label args, so the loop is a no-op). Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/run_sampler.yml | 26 +++++++++++++++++--------- README.md | 1 + 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/.github/workflows/run_sampler.yml b/.github/workflows/run_sampler.yml index 2e02ae4..3ec8aaf 100644 --- a/.github/workflows/run_sampler.yml +++ b/.github/workflows/run_sampler.yml @@ -33,12 +33,12 @@ on: required: true type: string handles: - description: "Reconciliation text handles to sample, ONE PER LINE (directory names under reconciliation_texts/). Newline-separated, not space-separated — names may contain spaces. Optional if account_templates is set." + description: "Reconciliation text handles to sample, ONE PER LINE (directory names under reconciliation_texts/). Newline-separated, not space-separated — names may contain spaces. A name starting with '-' is rejected (the CLI would parse it as an option). Optional if account_templates is set." required: false type: string default: "" account_templates: - description: "Account template names to sample, ONE PER LINE (directory names under account_templates/). Newline-separated, not space-separated — these names routinely contain spaces. Optional if handles is set." + description: "Account template names to sample, ONE PER LINE (directory names under account_templates/). Newline-separated, not space-separated — these names routinely contain spaces. A name starting with '-' is rejected (the CLI would parse it as an option). Optional if handles is set." required: false type: string default: "" @@ -196,13 +196,21 @@ jobs: # than a value: commander stops consuming a variadic option (-h/-at take ) at # the first "-"-prefixed token, and a leading "--" separator does not protect variadic # values either. Reject such a name up front instead of letting the CLI silently - # reinterpret it as an option — the actionable fix is to rename the template directory. - for NAME in "${HANDLE_NAMES[@]}" "${ACCOUNT_NAMES[@]}"; do - if [[ "${NAME}" == -* ]]; then - echo "::error::Template name '${NAME}' starts with '-', which silverfin-cli would parse as an option instead of a template name. Rename the template directory." - exit 1 - fi - done + # reinterpret it as an option. Both lists are checked, so the message names which kind + # of value and which directory to rename. + reject_option_like_names() { + local kind="$1" dir="$2" + shift 2 + local name + for name in "$@"; do + if [[ "${name}" == -* ]]; then + echo "::error::${kind} '${name}' starts with '-', which silverfin-cli would parse as an option instead of a value. Rename the ${dir} directory." + exit 1 + fi + done + } + reject_option_like_names "Reconciliation handle" "reconciliation_texts/" "${HANDLE_NAMES[@]}" + reject_option_like_names "Account template name" "account_templates/" "${ACCOUNT_NAMES[@]}" HANDLE_ARGS=() if [[ ${#HANDLE_NAMES[@]} -gt 0 ]]; then diff --git a/README.md b/README.md index 10a3475..3015df7 100644 --- a/README.md +++ b/README.md @@ -263,6 +263,7 @@ _Inputs:_ * `handles` (optional) — reconciliation text handles to sample, **one per line** (directory names under `reconciliation_texts/`). Optional if `account_templates` is set. * `account_templates` (optional) — account template names to sample, **one per line** (directory names under `account_templates/`). Optional if `handles` is set. * Both lists are newline-separated, **not** space-separated: account template directory names routinely contain spaces (e.g. `Investment- and depreciation details`), so a space-joined list is ambiguous and gets word-split into template names that don't exist (`Config file for account template "Investment-" not found`). Same convention as [`run_tests.yml`](#run-liquid-tests-run_testsyml). `firm_ids` is the exception — numeric, so it stays space-separated. + * A name that **starts with `-`** is rejected before the CLI is called, and the job fails with the directory to rename. `silverfin-cli`'s `-h`/`-at` are variadic options, so commander stops consuming values at the first `-`-prefixed token and would read such a name as a flag; a `--` separator does not protect variadic values. Only a leading `-` is affected — internal and trailing hyphens (`Cut-off`, `Investment- and depreciation details`) are fine. * `firm_ids` (required) — firm id(s) to sample against, space-separated. The backend 422s if empty. * `ref` (required) — git ref (commit SHA) to check out — the PR head, so sampled template content matches the PR under review. * `pull_request_number` (optional) — PR number to post the result comment on. If empty, no comment is posted (results still upload as an artifact).