Run multiple handles in parallel #239
Conversation
WalkthroughAdds variadic CLI options for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/cli.js (1)
466-477: Reject--handleand--account-templateused together (ambiguous template type).Both options can currently be passed simultaneously. When they are,
templateTypesilently prefers--handle(line 476), leaving--account-templateignored without warning. Add an explicit mutual-exclusion check to prevent this ambiguity.Proposed fix
.action(async (options) => { if (!options.handle && !options.accountTemplate) { consola.error("You need to specify either a reconciliation handle or an account template"); process.exit(1); } + if (options.handle && options.accountTemplate) { + consola.error('You cannot use both "--handle" and "--account-template" at the same time'); + process.exit(1); + }
🤖 Fix all issues with AI agents
In @bin/cli.js:
- Around line 495-499: The CLI ignores the return value of
liquidTestRunner.runTestsStatusOnly in the options.status branch causing tests
to always exit 0; capture the result of liquidTestRunner.runTestsStatusOnly
(which returns "PASSED" or "FAILED") and call process.exit(1) when the result
indicates failure (e.g., result !== "PASSED"), otherwise exit 0 or simply
return; update the block handling options.status (where runTestsStatusOnly is
invoked) to check the returned status and propagate a non-zero exit code for
failures so CI/CD can detect test failures.
In @CHANGELOG.md:
- Around line 5-8: The changelog uses a locale-ambiguous date format in the
release header "## [1.52.0] (12/01/2026)"; update that header to use an
unambiguous ISO-8601 date like "## [1.52.0] (2026-01-12)" so the date format is
consistent and clear across locales.
In @package.json:
- Line 3: The package.json "version" field was changed to "1.52.0"—confirm
whether this version bump is intentional per your release process and the PR
template checkbox "Skip bumping the CLI version"; if it was unintentional,
revert the change to the version field in package.json, otherwise add a note in
the PR description documenting that the bump is deliberate and aligns with the
release workflow.
🧹 Nitpick comments (1)
lib/liquidTestRunner.js (1)
518-522: Consider a concurrency limit for many handles (avoid API rate limiting / noisy logs).
Promise.allwill fan out requests for all handles at once; if users pass many handles, this can spike load and increase failures. Consider bounding concurrency (e.g.,p-limit).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
CHANGELOG.mdbin/cli.jslib/liquidTestRunner.jspackage.json
🧰 Additional context used
🧬 Code graph analysis (1)
bin/cli.js (3)
lib/liquidTestRunner.js (1)
options(16-16)lib/cli/utils.js (1)
options(89-89)lib/cli/devMode.js (1)
liquidTestRunner(4-4)
| if (options.status) { | ||
| liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern); | ||
| } else { | ||
| if (options.previewOnly && !options.htmlInput && !options.htmlPreview) { | ||
| consola.info(`When using "--preview-only" you need to specify at least one of the following options: "--html-input", "--html-preview"`); | ||
| process.exit(1); | ||
| } | ||
| liquidTestRunner.runTestsWithOutput(options.firm, templateType, templateName, options.test, options.previewOnly, options.htmlInput, options.htmlPreview, options.pattern); | ||
| // Status mode: allow multiple, pass array of template names | ||
| await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the structure of the repository
git ls-files | grep -E "(cli|test)" | head -20Repository: silverfin/silverfin-cli
Length of output: 659
🏁 Script executed:
# Find the CLI file and examine the status mode code
fd cli.jsRepository: silverfin/silverfin-cli
Length of output: 77
🏁 Script executed:
# Let's read the relevant section of bin/cli.js
wc -l bin/cli.jsRepository: silverfin/silverfin-cli
Length of output: 81
🏁 Script executed:
# Read around lines 495-499 with more context
sed -n '480,520p' bin/cli.jsRepository: silverfin/silverfin-cli
Length of output: 2096
🏁 Script executed:
# Find where runTestsStatusOnly is defined
rg "runTestsStatusOnly" --type js -B 2 -A 5Repository: silverfin/silverfin-cli
Length of output: 1298
🏁 Script executed:
# Check the return type/value of runTestsStatusOnly
ast-grep --pattern 'runTestsStatusOnly($$$) { $$$ }'Repository: silverfin/silverfin-cli
Length of output: 49
🏁 Script executed:
# Let's also check how other test modes handle exit codes
sed -n '450,550p' bin/cli.jsRepository: silverfin/silverfin-cli
Length of output: 4967
🏁 Script executed:
# Get the full implementation of runTestsStatusOnly
sed -n '1,100p' lib/liquidTestRunner.js | grep -A 50 "async function runTestsStatusOnly"Repository: silverfin/silverfin-cli
Length of output: 49
🏁 Script executed:
# Search for return statements in runTestsStatusOnly function
rg -A 100 "async function runTestsStatusOnly" lib/liquidTestRunner.js | head -120Repository: silverfin/silverfin-cli
Length of output: 1910
Status mode should propagate failure via exit code (required for CI/CD workflows).
runTestsStatusOnly() returns "PASSED" or "FAILED", but the CLI ignores this return value and always exits with code 0, breaking CI/CD integrations that depend on non-zero exit codes to detect test failures.
Proposed fix
if (options.status) {
// Status mode: allow multiple, pass array of template names
- await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern);
- return;
+ const overallStatus = await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern);
+ process.exitCode = overallStatus === "PASSED" ? 0 : 1;
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (options.status) { | |
| liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern); | |
| } else { | |
| if (options.previewOnly && !options.htmlInput && !options.htmlPreview) { | |
| consola.info(`When using "--preview-only" you need to specify at least one of the following options: "--html-input", "--html-preview"`); | |
| process.exit(1); | |
| } | |
| liquidTestRunner.runTestsWithOutput(options.firm, templateType, templateName, options.test, options.previewOnly, options.htmlInput, options.htmlPreview, options.pattern); | |
| // Status mode: allow multiple, pass array of template names | |
| await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern); | |
| return; | |
| } | |
| if (options.status) { | |
| // Status mode: allow multiple, pass array of template names | |
| const overallStatus = await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern); | |
| process.exitCode = overallStatus === "PASSED" ? 0 : 1; | |
| return; | |
| } |
🤖 Prompt for AI Agents
In @bin/cli.js around lines 495 - 499, The CLI ignores the return value of
liquidTestRunner.runTestsStatusOnly in the options.status branch causing tests
to always exit 0; capture the result of liquidTestRunner.runTestsStatusOnly
(which returns "PASSED" or "FAILED") and call process.exit(1) when the result
indicates failure (e.g., result !== "PASSED"), otherwise exit 0 or simply
return; update the block handling options.status (where runTestsStatusOnly is
invoked) to check the returned status and propagate a non-zero exit code for
failures so CI/CD can detect test failures.
| ## [1.52.0] (12/01/2026) | ||
| This update improves test execution performance when running tests with status checks across multiple template handles. | ||
| Tests are now run in parallel for multiple handles when using the `--status` flag, significantly reducing the overall execution time. Previously, tests with status checks for multiple handles would run sequentially, but now they leverage parallel processing for better efficiency. This change only affects the `silverfin run-test` command when both multiple handles and the status flag are used together. | ||
|
|
There was a problem hiding this comment.
Use an unambiguous date format in the changelog.
(12/01/2026) is locale-ambiguous; consider 2026-01-12 (ISO-8601) for consistency and clarity.
🤖 Prompt for AI Agents
In @CHANGELOG.md around lines 5 - 8, The changelog uses a locale-ambiguous date
format in the release header "## [1.52.0] (12/01/2026)"; update that header to
use an unambiguous ISO-8601 date like "## [1.52.0] (2026-01-12)" so the date
format is consistent and clear across locales.
| async function runTestsStatusOnly(firmId, templateType, handles, testName = "", pattern = "") { | ||
| if (templateType !== "reconciliationText" && templateType !== "accountTemplate") { | ||
| consola.error(`Template type is missing or invalid`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| let status = "FAILED"; | ||
| const testResult = await runTests(firmId, templateType, handle, testName, false, "none", pattern); | ||
| const runSingleHandle = async (singleHandle) => { | ||
| let status = "FAILED"; | ||
| const failedTestNames = []; | ||
| const testResult = await runTests(firmId, templateType, singleHandle, testName, false, "none", pattern); | ||
|
|
||
| if (!testResult) { | ||
| status = "PASSED"; | ||
| consola.success(status); | ||
| return status; | ||
| } | ||
| if (!testResult) { | ||
| status = "PASSED"; | ||
| } else { | ||
| const testRun = testResult?.testRun; | ||
|
|
||
| const testRun = testResult?.testRun; | ||
| if (testRun && testRun?.status === "completed") { | ||
| const errorsPresent = checkAllTestsErrorsPresent(testRun.tests); | ||
| if (errorsPresent === false) { | ||
| status = "PASSED"; | ||
| } else { | ||
| // Extract failed test names | ||
| const testNames = Object.keys(testRun.tests).sort(); | ||
| testNames.forEach((testName) => { | ||
| const testErrorsPresent = checkTestErrorsPresent(testName, testRun.tests); | ||
| if (testErrorsPresent) { | ||
| failedTestNames.push(testName); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (testRun && testRun?.status === "completed") { | ||
| const errorsPresent = checkAllTestsErrorsPresent(testRun.tests); | ||
| if (errorsPresent === false) { | ||
| status = "PASSED"; | ||
| consola.success(status); | ||
| return status; | ||
| if (status === "PASSED") { | ||
| consola.log(`${singleHandle}: ${status}`); | ||
| } else { | ||
| consola.log(`${singleHandle}: ${status}`); | ||
| // Display failed test names | ||
| failedTestNames.forEach((testName) => { | ||
| consola.log(` ${testName}: FAILED`); | ||
| }); | ||
| } | ||
| } | ||
| consola.error(status); | ||
| return status; | ||
|
|
||
| return { handle: singleHandle, status, failedTestNames }; | ||
| }; | ||
|
|
||
| const results = await Promise.all(handles.map(runSingleHandle)); | ||
|
|
||
| const overallStatus = results.every((result) => result.status === "PASSED") ? "PASSED" : "FAILED"; | ||
|
|
||
| return overallStatus; | ||
| } |
There was a problem hiding this comment.
Validate handles and don’t treat “no result” as PASS (can mask real failures).
Two issues:
handles.map(...)will throw ifhandlesisn’t an array (public API).if (!testResult) status = "PASSED";can incorrectly PASS whenrunTests(...)fails/returns nothing due to setup/API errors.
Proposed fix (within this function)
async function runTestsStatusOnly(firmId, templateType, handles, testName = "", pattern = "") {
if (templateType !== "reconciliationText" && templateType !== "accountTemplate") {
consola.error(`Template type is missing or invalid`);
process.exit(1);
}
+
+ if (!Array.isArray(handles) || handles.length === 0) {
+ consola.error(`You need to provide one or more handles`);
+ process.exit(1);
+ }
const runSingleHandle = async (singleHandle) => {
let status = "FAILED";
const failedTestNames = [];
const testResult = await runTests(firmId, templateType, singleHandle, testName, false, "none", pattern);
- if (!testResult) {
- status = "PASSED";
- } else {
+ if (!testResult) {
+ // If we couldn't start/read a run, treat as failure (otherwise CI can silently pass).
+ status = "FAILED";
+ } else {
const testRun = testResult?.testRun;
if (testRun && testRun?.status === "completed") {
const errorsPresent = checkAllTestsErrorsPresent(testRun.tests);
if (errorsPresent === false) {
status = "PASSED";
} else {
// Extract failed test names
const testNames = Object.keys(testRun.tests).sort();
testNames.forEach((testName) => {
const testErrorsPresent = checkTestErrorsPresent(testName, testRun.tests);
if (testErrorsPresent) {
failedTestNames.push(testName);
}
});
}
}
}| { | ||
| "name": "silverfin-cli", | ||
| "version": "1.51.0", | ||
| "version": "1.52.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the package.json file
fd package.json -type fRepository: silverfin/silverfin-cli
Length of output: 237
🏁 Script executed:
# Search for PR template files
fd -i pull_request_template -type fRepository: silverfin/silverfin-cli
Length of output: 237
🏁 Script executed:
# Also check for common template locations
ls -la .github/ 2>/dev/null || echo "No .github directory"Repository: silverfin/silverfin-cli
Length of output: 369
🏁 Script executed:
# Get git diff to see what actually changed in package.json
git diff HEAD -- package.json 2>/dev/null || echo "Git diff not available"Repository: silverfin/silverfin-cli
Length of output: 49
🏁 Script executed:
# Read package.json
cat package.json | head -20Repository: silverfin/silverfin-cli
Length of output: 585
🏁 Script executed:
# Read the PR template
cat .github/pull_request_template.mdRepository: silverfin/silverfin-cli
Length of output: 482
Confirm the version bump is intentional for this PR.
The PR template checklist includes "Skip bumping the CLI version", but package.json shows the version was updated to 1.52.0. Please clarify whether this version bump matches your release process.
🤖 Prompt for AI Agents
In @package.json at line 3, The package.json "version" field was changed to
"1.52.0"—confirm whether this version bump is intentional per your release
process and the PR template checkbox "Skip bumping the CLI version"; if it was
unintentional, revert the change to the version field in package.json, otherwise
add a note in the PR description documenting that the bump is deliberate and
aligns with the release workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @bin/cli.js:
- Around line 476-477: The code silently prefers options.handle over
options.accountTemplate when both are provided; add a mutual-exclusivity check
before computing templateType/templateName by calling
cliUtils.checkUniqueOption(["handle", "accountTemplate"], options) (or
equivalent) to validate that only one of these options is set, then keep the
existing logic that sets templateType and templateName based on options.handle;
ensure you reference options, templateType, and templateName in the same scope
so the check runs first.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/cli.js
🧰 Additional context used
🧬 Code graph analysis (1)
bin/cli.js (3)
lib/liquidTestRunner.js (1)
options(16-16)lib/cli/utils.js (1)
options(89-89)lib/cli/devMode.js (1)
liquidTestRunner(4-4)
🔇 Additional comments (4)
bin/cli.js (4)
456-457: LGTM - Variadic options for parallel execution.The option definitions correctly use commander's variadic syntax (
<handle...>,<name...>) to accept multiple values, which aligns with the PR objective of running multiple handles in parallel.
479-488: LGTM - Proper validation for multiple templates.The validation correctly:
- Ensures at least one handle/template is provided (lines 479-482)
- Blocks multiple handles/templates unless
--statusis specified (lines 484-488)This prevents users from accidentally providing multiple templates in non-status mode where only the first would be used.
490-494: LGTM - Status mode correctly passes array and awaits.The status mode correctly passes the array of template names to
runTestsStatusOnlyand properly awaits the result before returning. The early return ensures non-status mode logic is skipped.
496-513: LGTM - Non-status mode correctly extracts single template.The non-status execution path properly extracts the first template name from the array and passes all required parameters to
runTestsWithOutputin the correct order matching the documented signature.
| const templateType = options.handle ? "reconciliationText" : "accountTemplate"; | ||
| const templateName = options.handle ? options.handle : options.accountTemplate; |
There was a problem hiding this comment.
Missing mutual exclusivity check for handle and accountTemplate.
If both --handle and --account-template are provided, handle silently wins and accountTemplate is ignored. Other commands in this file use cliUtils.checkUniqueOption() to enforce mutual exclusivity (e.g., line 721 in development-mode).
🔧 Proposed fix
const templateType = options.handle ? "reconciliationText" : "accountTemplate";
const templateName = options.handle ? options.handle : options.accountTemplate;
+ if (options.handle && options.accountTemplate) {
+ consola.error("You cannot use both --handle and --account-template options at the same time");
+ process.exit(1);
+ }
+
if (!templateName || templateName.length === 0) {Alternatively, use the existing utility:
cliUtils.checkUniqueOption(["handle", "accountTemplate"], options);🤖 Prompt for AI Agents
In @bin/cli.js around lines 476 - 477, The code silently prefers options.handle
over options.accountTemplate when both are provided; add a mutual-exclusivity
check before computing templateType/templateName by calling
cliUtils.checkUniqueOption(["handle", "accountTemplate"], options) (or
equivalent) to validate that only one of these options is set, then keep the
existing logic that sets templateType and templateName based on options.handle;
ensure you reference options, templateType, and templateName in the same scope
so the check runs first.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/liquidTestRunner.js:
- Around line 472-476: runTestsStatusOnly is assuming handles is an array but
can receive a string (which makes .map iterate chars); add a defensive
validation at the top of runTestsStatusOnly: check if (!Array.isArray(handles))
then log an error via consola.error('handles must be an array') and exit
(process.exit(1)); alternatively, fix the caller that invokes runTestsStatusOnly
(the CLI invocation that passes templateName) to pass an array (e.g.,
[templateName]) so handles is always an array.
🧹 Nitpick comments (2)
lib/liquidTestRunner.js (2)
489-503: Variable shadowing:testNameshadows the outer parameter.The
testNamevariable in theforEachcallback at line 496 shadows the outer function parametertestNamefrom line 472. This can cause confusion during maintenance and may lead to subtle bugs if the outertestNameis needed later.♻️ Proposed fix
} else { // Extract failed test names const testNames = Object.keys(testRun.tests).sort(); - testNames.forEach((testName) => { - const testErrorsPresent = checkTestErrorsPresent(testName, testRun.tests); + testNames.forEach((name) => { + const testErrorsPresent = checkTestErrorsPresent(name, testRun.tests); if (testErrorsPresent) { - failedTestNames.push(testName); + failedTestNames.push(name); } }); }
506-514: Simplify duplicate logging statement.The
consola.logcall on lines 507 and 509 is identical. The conditional branches only differ in whether failed test names are printed.♻️ Proposed simplification
- if (status === "PASSED") { - consola.log(`${singleHandle}: ${status}`); - } else { - consola.log(`${singleHandle}: ${status}`); + consola.log(`${singleHandle}: ${status}`); + if (status !== "PASSED") { // Display failed test names failedTestNames.forEach((testName) => { consola.log(` ${testName}: FAILED`); }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/liquidTestRunner.js
🧰 Additional context used
🧬 Code graph analysis (1)
lib/liquidTestRunner.js (2)
bin/cli.js (2)
templateType(476-476)testName(525-525)lib/cli/spinner.js (1)
process(1-1)
🔇 Additional comments (1)
lib/liquidTestRunner.js (1)
519-523: LGTM on parallel execution and aggregation logic.The use of
Promise.allfor parallel execution is appropriate here and aligns with the PR objective. The aggregation using.every()correctly returns"PASSED"only when all handles pass.One edge case to be aware of: if
handlesis an empty array,[].every()returnstrue, so the function would return"PASSED". Verify this is the intended behavior for your use case (e.g., GitHub Actions).
| async function runTestsStatusOnly(firmId, templateType, handles, testName = "", pattern = "") { | ||
| if (templateType !== "reconciliationText" && templateType !== "accountTemplate") { | ||
| consola.error(`Template type is missing or invalid`); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the function implementation around lines 472-476
sed -n '472,530p' lib/liquidTestRunner.js | cat -nRepository: silverfin/silverfin-cli
Length of output: 2354
🏁 Script executed:
#!/bin/bash
# Search for all calls to runTestsStatusOnly in the codebase
rg -n 'runTestsStatusOnly\s*\(' --type js -A 2Repository: silverfin/silverfin-cli
Length of output: 579
Add input validation to guard against string being passed to handles parameter.
The function expects handles to be an array, but the only caller in bin/cli.js:492 passes templateName (a string). When a string is passed to .map(), it iterates over individual characters instead of array items, causing incorrect behavior.
Add defensive validation at the beginning of the function:
if (!Array.isArray(handles)) {
consola.error('handles must be an array');
process.exit(1);
}Alternatively, update the caller to wrap templateName in an array: [templateName] or ensure it's always passed as an array.
🤖 Prompt for AI Agents
In @lib/liquidTestRunner.js around lines 472 - 476, runTestsStatusOnly is
assuming handles is an array but can receive a string (which makes .map iterate
chars); add a defensive validation at the top of runTestsStatusOnly: check if
(!Array.isArray(handles)) then log an error via consola.error('handles must be
an array') and exit (process.exit(1)); alternatively, fix the caller that
invokes runTestsStatusOnly (the CLI invocation that passes templateName) to pass
an array (e.g., [templateName]) so handles is always an array.
Fixes # (link to the corresponding issue if applicable)
Description
Include a summary of the changes made
Testing Instructions
Steps:
Author Checklist
Reviewer Checklist