Sampler: pass template names newline-separated so spaced account template names survive - #37
Merged
Conversation
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) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe sampler workflow now accepts newline-separated handles and account-template names. It preserves spaces in CLI arguments, rejects names beginning with ChangesSampler input handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/run_sampler.yml (1)
173-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an automated argv-boundary regression test.
Test a two-line value containing
Investment- and depreciation details. Assert that the constructed array contains two names after the-hor-atflag. This protects the primary fix from a future return to shell word splitting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/run_sampler.yml around lines 173 - 188, Add an automated regression test for the argument construction around HANDLE_ARGS and ACCOUNT_ARGS using a two-line input that includes “Investment- and depreciation details”; assert the resulting array preserves exactly two names after the relevant -h or -at flag, confirming newline-based parsing does not regress to shell word splitting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run_sampler.yml:
- Around line 375-380: Update the fmtNames formatter used by the HANDLES and
ACCOUNT_TEMPLATES PR comment entries to escape or safely encode backticks within
each trimmed name before wrapping it in Markdown code delimiters. Preserve the
existing newline splitting, whitespace filtering, and comma-separated rendering
behavior.
---
Nitpick comments:
In @.github/workflows/run_sampler.yml:
- Around line 173-188: Add an automated regression test for the argument
construction around HANDLE_ARGS and ACCOUNT_ARGS using a two-line input that
includes “Investment- and depreciation details”; assert the resulting array
preserves exactly two names after the relevant -h or -at flag, confirming
newline-based parsing does not regress to shell word splitting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a12af9a-2206-473b-bef9-23470718c1ab
📒 Files selected for processing (2)
.github/workflows/run_sampler.ymlREADME.md
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) <noreply@anthropic.com>
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 <names...>) 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/run_sampler.yml (1)
134-144: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftSecurity Misconfiguration (CWE-829): Inclusion of Functionality from Untrusted Control Sphere
Reachability: External
Verify transitive dependency pinning for the privileged install.
The workflow installs the GitHub package before the sampler runs. The commit pin fixes the top-level
silverfin-clisource, but the provided workflow does not show a lockfile or integrity check for its transitive dependencies. A changed dependency could execute duringnpm installwhile the job-levelSF_API_CLIENT_IDandSF_API_SECRETvariables are available.Verify the dependency tree and use a committed lockfile or a reviewed artifact with integrity verification if the dependency tree is not pinned.
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '--- repository lockfiles ---' find . -path './.git' -prune -o -type f \ \( -name package-lock.json -o -name npm-shrinkwrap.json -o -name yarn.lock -o -name pnpm-lock.yaml \) \ -print tmp="$(mktemp -d)" trap 'rm -rf "$tmp"' EXIT git clone --filter=blob:none --no-checkout \ https://github.com/silverfin/silverfin-cli.git "$tmp/cli" git -C "$tmp/cli" fetch --depth 1 origin 1ba5a0f git -C "$tmp/cli" checkout --detach 1ba5a0f printf '%s\n' '--- silverfin-cli lockfiles ---' find "$tmp/cli" -type f \ \( -name package-lock.json -o -name npm-shrinkwrap.json \) \ -print🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/run_sampler.yml around lines 134 - 144, Update the privileged silverfin-cli installation in the workflow to pin and integrity-verify its complete dependency tree, not only the Git commit. Use a committed lockfile or reviewed artifact with integrity metadata, and install with the corresponding lockfile-enforcing mechanism; verify the selected 1ba5a0f dependency tree before relying on it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run_sampler.yml:
- Around line 200-203: Update the validation error in the NAME loop to use
neutral terminology for values from both HANDLE_NAMES and ACCOUNT_NAMES; replace
“Template name” and “template directory” with wording that applies equally to
reconciliation handles and account names.
In `@README.md`:
- Around line 263-265: Document in the README input contract and the
corresponding `.github/workflows/run_sampler.yml` configuration comments that
`handles` and `account_templates` entries must not begin with `-`. Add this
restriction alongside the existing newline-separated format guidance, without
changing workflow validation behavior.
---
Nitpick comments:
In @.github/workflows/run_sampler.yml:
- Around line 134-144: Update the privileged silverfin-cli installation in the
workflow to pin and integrity-verify its complete dependency tree, not only the
Git commit. Use a committed lockfile or reviewed artifact with integrity
metadata, and install with the corresponding lockfile-enforcing mechanism;
verify the selected 1ba5a0f dependency tree before relying on it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc692bd5-7b80-48df-89dc-50ec2231a8d7
📒 Files selected for processing (2)
.github/workflows/run_sampler.ymlREADME.md
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Account template identifiers are directory names and routinely contain spaces (
account_templates/Investment- and depreciation details). Thehandles/account_templatesinputs were documented and encoded as space-separated strings, then deliberately word-split into CLI args — so-atreceivedInvestment-,and,depreciation,detailsand the run died:Hit live in lu_market: https://github.com/silverfin/lu_market/actions/runs/30612771053 (the PR under review changed exactly one file, inside that template).
Change
Both inputs are now one name per line, and the arg arrays are built with
mapfileso each line becomes exactly one argv entry. This is already the convention in this repo's ownrun_tests.yml(TEMPLATE_BUCKETS, with a comment saying why) — the sampler was the only place that regressed to space-joining.firm_idsstays space-separated; it's numeric, so theSC2086split there is still intentional.Two supporting bits in the same area:
%q-quoted, so a name containing spaces is visibly one argument.A caller still passing a space-joined list now fails loudly (
Config file for account template "..." not found) instead of silently sampling templates nobody asked for. I deliberately did not add a space-splitting fallback: for names containing spaces it cannot be made correct, and the loud failure is the honest outcome.Input descriptions and the README input list are updated to say one per line. The only callers today are the lu_market / nl_market wrappers, which currently run inlined forks of this job (pinned to silverfin-cli's unmerged
sampler-compact-diff-v2branch); their matching fixes are silverfin/lu_market#785 and silverfin/nl_market#914. When those forks are reverted touses:after silverfin-cli #265 merges, this fix means they don't regress — but the newline-emittingclassifyhalf lives in the market repos, so the two halves must land together.Testing
The name-splitting logic was verified in bash against the real lu_market names (
Investment- and depreciation details+Loan details→ 3 argv entries, spaces intact); YAML validated. Not yet exercised in a live sampler run — see the market PRs for that.🤖 Generated with Claude Code
Also in this PR
Three follow-ups that landed on the same branch:
Backtick-safe rendering of names in the PR comment (
ddcb0af) — flagged by CodeRabbit (CWE-116). Template names are directory names from the PR's own tree, so a name containing a backtick closed its code span and injected markdown into the bot's comment. Fenced per CommonMark (one more backtick than the longest run inside the name, space-padded when it starts/ends with one) rather than backslash-escaping, which does nothing inside a code span. Names without backticks render byte-identically.Reject names starting with
-(5b88bf3) — flagged by CodeRabbit (CWE-88). Commander stops consuming a variadic option (-h/-attake<names...>) at the first--prefixed token, and a--separator does not protect variadic values, so a directory named e.g.--from-zipwould be reinterpreted as an option. The step now fails with a rename-the-directory message before any CLI call. Only a leading-is rejected;Cut-offandInvestment- and depreciation detailsare unaffected.Re-pin silverfin-cli to the branch tip (
cb7d387) — pre-existing debt from #35, not introduced here. The pin5accd6bis not an ancestor ofsampler-compact-diff-v2: it's a pre-rebase copy of one of its commits, orphaned by the rebase. It still runs, so nothing failed loudly — it just froze this workflow five compact-diff fixes behind the branch it claims to track, including the one that stops--add-diffs-folderfillingdiffs/with byte-identical pairs. Now pinned to1ba5a0f, with a note that re-pinning is required whenever that branch moves (until silverfin-cli#265 merges and this returns to unpinnedmain).Validation
Beyond the bash-level checks above, the identical newline fix has now run in production: lu_market run 30920292551 sampled
Investment- and depreciation detailssuccessfully end-to-end (21 min, artifact uploaded, PR comment posted). This repo's own PRs don't exercise the sampler job, so that market run is the real signal.