Sampler: pass template names newline-separated so spaced account template names survive - #37
Sampler: pass template names newline-separated so spaced account template names survive#37michieldegezelle wants to merge 3 commits into
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.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe sampler workflow now accepts newline-separated handles and account-template names. It preserves spaces in CLI arguments, shell-escapes attempt logs, and renders each value separately in PR comments. The README documents the new format and retains space-separated 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>
1 participant
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