feat: liquid sampler command#236
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Fix all issues with AI agents
In @lib/liquidSamplerRunner.js:
- Around line 36-43: The code assumes SF.createSamplerRun returned an object
with a data property and immediately accesses samplerResponse.data.id; add a
null-safety guard to handle falsy or malformed responses by first validating
samplerResponse and samplerResponse.data (e.g., using optional chaining or
explicit checks) before reading .id, set samplerId from samplerResponse.data?.id
?? samplerResponse.data, and if no id is found log the full samplerResponse (or
relevant error info) via consola.error and exit; update references in this block
around SF.createSamplerRun, samplerResponse, and samplerId accordingly.
- Around line 57-67: fetchSamplerStatus currently assumes SF.readSamplerRun
returns an object with a data property; add a null-safety check after awaiting
SF.readSamplerRun to verify response and response.data exist before calling
handleSamplerResponse, and if missing call errorUtils.errorHandler or log/throw
a descriptive error (including samplerId and partnerId) so you don't pass
undefined into handleSamplerResponse; update the function surrounding
SF.readSamplerRun, fetchSamplerStatus, handleSamplerResponse and
errorUtils.errorHandler accordingly to handle the absent-data branch.
🧹 Nitpick comments (2)
lib/liquidSamplerRunner.js (2)
192-192: Remove commented-out code.The commented-out exponential backoff logic is dead code. Either implement the backoff strategy or remove the comment to keep the codebase clean.
♻️ Proposed fix
waitingTime += pollingDelay; - // pollingDelay *= 1.05; if (waitingTime >= waitingLimit) {
225-231: Address TODO or track it in an issue.There's a TODO comment indicating the browser-opening logic should follow a pattern similar to the liquid test runner. Consider implementing this consistency or creating an issue to track it.
Would you like me to help open an issue to track this refactor, or would you like me to suggest an implementation that aligns with the liquid test runner pattern?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/cli.jslib/api/sfApi.jslib/liquidSamplerRunner.js
🧰 Additional context used
🧬 Code graph analysis (2)
lib/api/sfApi.js (1)
lib/liquidSamplerRunner.js (1)
samplerId(38-38)
bin/cli.js (1)
lib/liquidSamplerRunner.js (2)
templateHandles(25-25)templateHandles(80-80)
🔇 Additional comments (5)
lib/api/sfApi.js (1)
644-666: LGTM!The new
createSamplerRunandreadSamplerRunfunctions follow the established patterns in this file, with consistent error handling usingresponseSuccessHandlerandresponseErrorHandler. The partner-scoped Axios instance is correctly used for these partner API endpoints.bin/cli.js (1)
483-516: LGTM!The
run-samplercommand implementation follows existing CLI patterns. The validation logic correctly ensures at least one template type is specified when not fetching by ID, and thetemplateHandlesobject construction properly maps CLI options to the expected format used byliquidSamplerRunner.lib/liquidSamplerRunner.js (3)
1-11: LGTM!The imports are well-organized and include all necessary dependencies for the sampler functionality.
243-246: LGTM!The module exports correctly expose
runSamplerandfetchSamplerStatusfor use by the CLI and other consumers.
101-101: No action needed.The inconsistent
awaitusage is correct.SharedPart.read()is an async function (declared withstatic async read()), whileReconciliationText.read()andAccountTemplate.read()are synchronous functions (declared withoutasync). Usingawaitonly for the async method and omitting it for synchronous methods is the correct pattern.Likely an incorrect or invalid review comment.
AgustinSilverfin
force-pushed
the
agustin-bso-sampler
branch
from
56b49ae to
990bf5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/cli/cliUpdater.js (3)
64-66: Incorrect HTTP status check logic.Line 64 returns early if
response.status !== 200, but this prevents the function from returning the version when the status is 200. The logic should check for success (=== 200) and proceed, or check for failure and return early.🐛 Proposed fix
- if (response.status !== 200) { + if (response.status === 200 && response.data.version) { + return response.data.version; + } else { return; } - if (response.data.version) { - return response.data.version; - }Or simply remove the status check since axios throws on non-2xx by default:
- if (response.status !== 200) { - return; - } if (response.data.version) { return response.data.version; }
76-85: Version comparison doesn't handle equal versions or edge cases.The function has two issues:
- It doesn't return anything when versions are equal (line 84 is unreachable)
- It assumes both version strings have the same number of parts
For example, comparing "1.2.3" with "1.2" would only check the first two segments and return undefined.
♻️ Proposed fix
static #compareVersions(latestVersion, currentVersion) { const latestVersionParts = latestVersion.split(".").map(Number); const currentVersionParts = currentVersion.split(".").map(Number); + const maxLength = Math.max(latestVersionParts.length, currentVersionParts.length); - for (let i = 0; i < latestVersionParts.length; ++i) { + for (let i = 0; i < maxLength; ++i) { + const latest = latestVersionParts[i] || 0; + const current = currentVersionParts[i] || 0; - if (latestVersionParts[i] !== currentVersionParts[i]) { - return latestVersionParts[i] > currentVersionParts[i]; + if (latest !== current) { + return latest > current; } } + return false; // versions are equal }
1-7: Upgrade axios from 1.6.2 due to multiple critical security vulnerabilities.axios 1.6.2 is affected by several known CVEs:
- Prototype pollution via formDataToJSON (affects ≥0.28.0 and <1.6.4)
- Regular-expression DoS (affects ≥1.0.0 and <1.6.3)
- SSRF vulnerability in path/protocol-relative URL handling (affects ≥1.3.2 and <1.7.4)
- Data: URI DoS (affects <1.12.0)
Upgrade to at minimum 1.6.5+ for the first two issues, 1.7.4+ to address SSRF, or preferably the latest 1.12.x/1.13.x series to cover all known vulnerabilities.
chalk 4.1.2 and consola 3.2.3 are secure with no known vulnerabilities.
🤖 Fix all issues with AI agents
In @lib/cli/cliUpdater.js:
- Line 10: The UPDATE_COMMAND currently embeds a hardcoded sudo which breaks on
Windows; update static #UPDATE_COMMAND in cliUpdater.js to be cross-platform by
removing the leading "sudo" (or conditionally prepending it only on non-Windows
platforms), so the command becomes just "npm install -g ${pkg.repository.url}"
(or build it with a platform check using process.platform !== 'win32' to add
sudo on UNIX-like systems).
In @lib/exportFileInstanceGenerator.js:
- Around line 42-81: The method #generateInstance mixes returning false with
exception handling; make error signaling consistent by throwing errors instead
of returning false: replace each `return false` in #generateInstance with `throw
new Error(...)` containing context (use this.#details(exportFileInstanceId)
where available), and update the catch to call `errorUtils.errorHandler(error)`
then rethrow the error so callers like generateAndOpenFile can handle it; ensure
success paths still return the response as before.
- Around line 31-33: The early return in generateInstance() silently exits when
response is falsy; update the function (generateInstance in
exportFileInstanceGenerator.js) to log a clear warning or error before returning
(e.g., use the module's logger or console to state that generateInstance
returned no response and include any available context such as input parameters
or status) so callers/users can see why the process stopped; then return or
throw as appropriate.
In @lib/liquidSamplerRunner.js:
- Around line 233-234: The condition currently checks the wrong property
(response.result_url) before calling new
UrlHandler(response.content_url).openFile(); update the check to test
response.content_url instead so the guard matches the value actually accessed;
locate the block around the UrlHandler usage in liquidSamplerRunner.js and
replace the condition to ensure response.content_url is defined before
constructing UrlHandler.
- Line 44: The fallback assigning samplerId using "samplerResponse.data.id ||
samplerResponse.data" is unsafe; update the logic that sets samplerId (the
variable assigned from samplerResponse) to explicitly handle the two supported
shapes: if samplerResponse.data is an object with a defined non-null id (check
existence via !== undefined and !== null and ensure type is string or number)
use that id, else if samplerResponse.data is a primitive string/number use it
directly (check typeof), otherwise throw or return a clear validation error so
later API calls receive a valid samplerId.
In @lib/utils/urlHandler.js:
- Around line 31-44: The private async method #downloadFile swallows exceptions
by calling errorUtils.errorHandler(error) without rethrowing or returning,
causing callers like openFile to get undefined filePath; update #downloadFile so
after calling errorUtils.errorHandler(error) it either rethrows the error (throw
error) or returns a rejected promise (return Promise.reject(error)) so the error
propagates to openFile and upstream callers; make this change inside the catch
block of #downloadFile and keep the existing errorUtils.errorHandler call for
logging/handling.
- Around line 37-38: The code in urlHandler.js is using blocking fs.mkdirSync
and fs.writeFileSync inside an async flow; replace them with non-blocking async
calls (e.g., use fs.promises.mkdir or import from 'fs/promises' and await
mkdir(path.dirname(tempFilePath), { recursive: true }) and await
writeFile(tempFilePath, response.data)) and propagate/handle errors
appropriately so tempFilePath is created and written without blocking the event
loop; update any surrounding try/catch in the function that fetches
response.data to await these async ops.
- Line 33: The axios call currently reads the whole payload into memory using
responseType: "arraybuffer" which can OOM on large exports; change the
axios.get(this.url, { responseType: "arraybuffer" }) call to use streaming
(responseType: "stream"), then consume the returned response.stream by piping it
to a temporary file or processing it via streaming transforms (e.g.,
response.data.pipe(fs.createWriteStream(tempPath)) or using a stream parser),
ensure you handle backpressure/errors and cleanup the temp file, and update any
downstream code that expects a Buffer to instead read from the streamed file or
stream APIs.
🧹 Nitpick comments (7)
lib/utils/urlHandler.js (2)
36-36: Potential filename collision withDate.now().Using
Date.now()alone for the filename could cause collisions if multiple files are downloaded in rapid succession (within the same millisecond).♻️ Proposed improvement
Add a random component to reduce collision probability:
- const tempFilePath = path.resolve(require("os").tmpdir(), "silverfin", `${Date.now()}.${fileExtension}`); + const tempFilePath = path.resolve(require("os").tmpdir(), "silverfin", `${Date.now()}-${Math.random().toString(36).substr(2, 9)}.${fileExtension}`);
22-29: No cleanup mechanism for temporary files.Downloaded files accumulate in the temp directory without cleanup. Consider implementing automatic cleanup or documenting the cleanup strategy.
Potential approaches:
- Clean up the file after opening (though the OS might still be using it)
- Implement periodic cleanup of old files in the silverfin temp directory
- Use OS-level temp file APIs that handle cleanup automatically
- Document that users should manually clean
os.tmpdir()/silverfinperiodicallylib/exportFileInstanceGenerator.js (1)
56-67: Polling timeout and user experience considerations.Current polling configuration:
- MAX_ATTEMPTS: 25
- Initial wait: 1s, incrementing by 1s each iteration, capped at 5s
- Total max time: ~92 seconds (1+2+3+4+5×21)
Consider:
- Displaying progress to users during long waits
- Making timeout configurable for different scenarios
- Documenting expected generation times for various export types
Adding periodic progress updates would improve UX:
if (attempts % 5 === 0) { consola.info(`Still waiting for export file generation... (${attempts}/${this.#MAX_ATTEMPTS})`); }lib/liquidSamplerRunner.js (2)
93-176: Consider extracting duplicate validation logic.The config existence check and partner_id validation are duplicated across reconciliation texts, account templates, and shared parts sections. Extracting this into a helper method would improve maintainability.
Suggested refactor
+ #validateAndReadTemplate(templateType, identifier, TemplateReader) { + const configPresent = fsUtils.configExists(templateType, identifier); + + if (!configPresent) { + consola.error(`Config file for ${templateType} "${identifier}" not found`); + process.exit(1); + } + + const config = fsUtils.readConfig(templateType, identifier); + + if (!config.partner_id || !config.partner_id[this.partnerId]) { + consola.error(`Template '${identifier}' has no partner_id entry for partner ${this.partnerId}. Import the template to this partner first.`); + process.exit(1); + } + + return { + id: config.partner_id[this.partnerId], + content: TemplateReader.read(identifier) + }; + }Then simplify each loop:
for (const handle of reconciliationTexts) { const { id, content } = this.#validateAndReadTemplate('reconciliationText', handle, ReconciliationText); templates.push({ type: "Global::Partner::ReconciliationText", id: String(id), text: content.text, text_parts: content.text_parts, }); }
202-202: Remove commented-out code.The commented-out exponential backoff logic should either be implemented or removed to keep the codebase clean.
bin/cli.js (2)
512-515: Duplicate validation with LiquidSamplerRunner.This validation is also performed in
LiquidSamplerRunner.run()(lines 32-35 of liquidSamplerRunner.js). While this defensive programming provides early feedback to users, it creates maintenance overhead if validation logic needs to change.
517-521: Consider using shorthand property syntax.For consistency and conciseness, you can use ES6 shorthand property syntax.
Suggested refactor
const templateHandles = { reconciliationTexts: handles, - accountTemplates: accountTemplates, - sharedParts: sharedParts, + accountTemplates, + sharedParts, };
📜 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 (8)
CHANGELOG.mdbin/cli.jslib/api/sfApi.jslib/cli/cliUpdater.jslib/exportFileInstanceGenerator.jslib/liquidSamplerRunner.jslib/utils/urlHandler.jspackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
lib/liquidSamplerRunner.js (1)
lib/cli/spinner.js (1)
spinner(76-76)
🔇 Additional comments (7)
CHANGELOG.md (1)
5-7: LGTM! Clear and informative changelog entry.The new version entry provides a comprehensive description of the
silverfin generate-export-filecommand with helpful usage guidance.lib/api/sfApi.js (2)
644-666: LGTM! Export file instance endpoints follow established patterns.The implementation is consistent with other API methods in this file, properly using AxiosFactory, error handling, and response utilities.
668-690: LGTM! Sampler run endpoints are well-structured.Both
createSamplerRunandreadSamplerRunfollow the same robust pattern used throughout this API module, with proper error handling and response management.lib/exportFileInstanceGenerator.js (1)
14-23: LGTM! Parameter validation in constructor.Proper validation ensures the class cannot be instantiated with missing required parameters, catching configuration errors early.
bin/cli.js (2)
6-7: LGTM!The imports for the new modules are correctly structured.
754-766: LGTM!The generate-export-file command is well-structured with proper option handling and clear action flow.
lib/liquidSamplerRunner.js (1)
111-111: No changes needed—the code is correct as written.The read methods have different signatures:
ReconciliationText.read()andAccountTemplate.read()are synchronous methods, so they should not be awaited.SharedPart.read()is async, so the await on line 169 is correct. The implementation in liquidSamplerRunner.js properly matches each method's return type.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/liquidSamplerRunner.js`:
- Around line 200-201: The polling code calls SF.readSamplerRun(this.partnerId,
samplerId) and assigns response.data to samplerRun without null-safety; add a
defensive check after the await to verify response and response.data exist
(e.g., if (!response || !response.data) { handle error/retry/continue }) before
using samplerRun, and ensure any subsequent logic that expects samplerRun (the
variable assigned from response.data) handles the missing case gracefully;
update the block around SF.readSamplerRun, response, and samplerRun to use this
guard.
🧹 Nitpick comments (2)
lib/liquidSamplerRunner.js (1)
204-204: Remove commented-out code.The commented exponential backoff
// pollingDelay *= 1.05;should be removed if not needed, or implemented if intended. Leaving dead code creates maintenance burden.♻️ Proposed fix
waitingTime += pollingDelay; - // pollingDelay *= 1.05; if (waitingTime >= waitingLimit) {bin/cli.js (1)
508-516: Consider removing duplicate validation.The template validation (lines 513-516) duplicates the validation already present in
LiquidSamplerRunner.run()(lines 33-36). While defensive, maintaining validation in two places risks divergence. Consider letting the runner handle this validation.♻️ Option: Remove CLI-side validation
- // Validate: at least one template specified - const handles = options.handle || []; - const accountTemplates = options.accountTemplate || []; - const sharedParts = options.sharedPart || []; - - if (handles.length === 0 && accountTemplates.length === 0 && sharedParts.length === 0) { - consola.error("You need to specify at least one template using -h, -at, or -s"); - process.exit(1); - } const templateHandles = { - reconciliationTexts: handles, - accountTemplates: accountTemplates, - sharedParts: sharedParts, + reconciliationTexts: options.handle || [], + accountTemplates: options.accountTemplate || [], + sharedParts: options.sharedPart || [], };
AgustinSilverfin
force-pushed
the
agustin-bso-sampler
branch
from
b171592 to
d3c099d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/liquidSamplerRunner.js`:
- Around line 188-193: The JSDoc for fetchAndWaitSamplerResult is stale: it
lists a partnerId parameter that the function does not accept; update the
docblock above fetchAndWaitSamplerResult in lib/liquidSamplerRunner.js to remove
the incorrect `@param` {number} partnerId entry and ensure the `@param` entries
match the actual signature (e.g., `@param` {string} samplerId and any other real
params) and keep the `@returns` description accurate so the comment reflects the
real function signature.
d3c099d to
346af6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bin/cli.js`:
- Around line 491-527: options.firmIds are strings from Commander but
LiquidSamplerRunner.run expects Array<number>; before constructing firmIds pass
options.firmIds through a numeric normalization step (map to Number or parseInt)
and filter/validate out non-numeric entries, failing with a clear consola.error
and process.exit(1) if any provided ID is not a valid number; update the firmIds
assignment in the run-sampler handler (referencing options.firmIds and the
firmIds variable) so you call new
LiquidSamplerRunner(options.partner).run(templateHandles, firmIds) with an
Array<number>.
In `@lib/liquidSamplerRunner.js`:
- Around line 117-154: ReconciliationText.read and AccountTemplate.read are
being used synchronously and can return Promises or falsy values; change the
calls in the account and reconciliation template loops to await
ReconciliationText.read(handle) and await AccountTemplate.read(name), and after
awaiting validate the result (e.g., if (!templateContent ||
!templateContent.text) { consola.error(...); process.exit(1); }) before
accessing templateContent.text or templateContent.text_parts; use the same
defensive pattern as SharedPart.read to avoid Promise deref and runtime crashes.
346af6b to
57a4508
Compare
AgustinSilverfin
force-pushed
the
agustin-bso-sampler
branch
from
57a4508 to
43f855b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lib/liquidSamplerRunner.js (2)
118-124:⚠️ Potential issue | 🟠 MajorGuard template reader failures before dereferencing
.text.
lib/templates/reconciliationText.js:94-115,lib/templates/accountTemplate.js:83-103, andlib/templates/sharedPart.js:50-60all returnfalseon validation failure. Lines 123-124, 152-153, and 181 currently dereference that result, so a bad handle turns into aTypeErrorinstead of a clear CLI error.Suggested patch
const templateId = config.partner_id[this.partnerId]; const templateContent = ReconciliationText.read(handle); + if (!templateContent || typeof templateContent !== "object") { + consola.error(`Reconciliation text "${handle}" couldn't be read`); + process.exit(1); + } @@ const templateId = config.partner_id[this.partnerId]; const templateContent = AccountTemplate.read(name); + if (!templateContent || typeof templateContent !== "object") { + consola.error(`Account template "${name}" couldn't be read`); + process.exit(1); + } @@ const templateId = config.partner_id[this.partnerId]; const templateContent = await SharedPart.read(name); + if (!templateContent || typeof templateContent !== "object") { + consola.error(`Shared part "${name}" couldn't be read`); + process.exit(1); + }Also applies to: 147-153, 176-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/liquidSamplerRunner.js` around lines 118 - 124, The code dereferences templateContent.text after calling ReconciliationText.read (and similarly AccountTemplate.read and SharedPart.read) without checking for failure; modify the places where you push into templates to first check the reader return value (e.g., templateContent) and if it is falsy/false, throw or return a clear CLI error (with context like templateId and handle) instead of proceeding to access .text/.text_parts; update the push-site for ReconciliationText.read, AccountTemplate.read, and SharedPart.read to guard the read result and surface a user-friendly error when validation fails.
44-49:⚠️ Potential issue | 🟠 MajorValidate SF responses before reading
.dataor treating it as an ID.Line 45 and Line 206 still assume the wrapper always returns
{ data: ... }. IfcreateSamplerRun/readSamplerRunbubble up a different error payload, this throws before the CLI can emit a useful message, andsamplerResponse.data.id || samplerResponse.datacan still pass a whole object through assamplerId.Suggested patch
// Start sampler run const samplerResponse = await SF.createSamplerRun(this.partnerId, samplerParams); - const samplerId = samplerResponse.data.id || samplerResponse.data; + const samplerData = samplerResponse?.data; + if (samplerData === undefined || samplerData === null) { + consola.error("Failed to start sampler run - invalid response from API"); + process.exit(1); + } + const samplerId = + samplerData && typeof samplerData === "object" + ? samplerData.id + : typeof samplerData === "string" || typeof samplerData === "number" + ? samplerData + : undefined; - if (!samplerId) { + if (samplerId === undefined || samplerId === null || samplerId === "") { consola.error("Failed to start sampler run - no ID returned"); process.exit(1); } @@ const response = await SF.readSamplerRun(this.partnerId, samplerId); + if (!response || response.data === undefined || response.data === null) { + spinner.stop(); + consola.error("Failed to fetch sampler status during polling - invalid response from API"); + process.exit(1); + } samplerRun = response.data;#!/bin/bash set -euo pipefail # Inspect the SF API wrappers and any shared response handlers. sf_api="$(fd 'sfApi\.js$' | head -n1)" error_utils="$(fd 'errorUtils\.js$' | head -n1)" echo "== SF API wrapper definitions ==" rg -n -C3 'createSamplerRun|readSamplerRun|responseErrorHandler' "$sf_api" echo echo "== Error utility helpers ==" rg -n -C3 'responseErrorHandler|errorHandler' "$error_utils" echo echo "== Current LiquidSamplerRunner call sites ==" rg -n -C2 'createSamplerRun\(|readSamplerRun\(' lib/liquidSamplerRunner.jsAlso applies to: 205-206
🧹 Nitpick comments (1)
lib/liquidSamplerRunner.js (1)
199-219: Always stop the spinner in afinally.Any exception between Line 199 and Line 218 skips the normal cleanup path, so polling failures can leave the terminal spinner hanging.
Suggested patch
spinner.spin("Running sampler..."); let waitingTime = 0; - while (samplerRun.status === "pending" || samplerRun.status === "running") { - await new Promise((resolve) => setTimeout(resolve, pollingDelay)); - - const response = await SF.readSamplerRun(this.partnerId, samplerId); - samplerRun = response.data; - - waitingTime += pollingDelay; - // pollingDelay *= 1.05; - - if (waitingTime >= waitingLimit) { - spinner.stop(); - consola.error("Timeout. Try to fetch the status by using the --id flag, if not run your sampler again"); - process.exit(1); - } - } - - spinner.stop(); - return samplerRun; + try { + while (samplerRun.status === "pending" || samplerRun.status === "running") { + await new Promise((resolve) => setTimeout(resolve, pollingDelay)); + + const response = await SF.readSamplerRun(this.partnerId, samplerId); + samplerRun = response.data; + + waitingTime += pollingDelay; + // pollingDelay *= 1.05; + + if (waitingTime >= waitingLimit) { + spinner.stop(); + consola.error("Timeout. Try to fetch the status by using the --id flag, if not run your sampler again"); + process.exit(1); + } + } + + return samplerRun; + } finally { + spinner.stop(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/liquidSamplerRunner.js` around lines 199 - 219, The spinner can be left running if an exception occurs during polling; wrap the polling logic that begins with spinner.spin(...) and the while loop that calls SF.readSamplerRun(...) in a try...finally block so that spinner.stop() is always called in the finally branch; keep spinner.spin(...) before the try, perform the await polling and updates to samplerRun, waitingTime, pollingDelay and timeout checks inside the try, and then call spinner.stop() in finally (rethrow or return samplerRun after the try/finally as appropriate) to ensure cleanup even on errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/liquidSamplerRunner.js`:
- Around line 233-235: The "failed" case currently only logs the error and
breaks, letting run() and checkStatus() exit with code 0; update the case in the
switch (the "failed" branch where consola.error is called) to set a non-zero
exit (e.g., call process.exit(1) or assign process.exitCode = 1 and then
process.exit()) after logging so the process exits with failure; apply this
change in the function containing the switch (checkStatus / run flow) so a
failed sampler run returns a non-zero exit code.
---
Duplicate comments:
In `@lib/liquidSamplerRunner.js`:
- Around line 118-124: The code dereferences templateContent.text after calling
ReconciliationText.read (and similarly AccountTemplate.read and SharedPart.read)
without checking for failure; modify the places where you push into templates to
first check the reader return value (e.g., templateContent) and if it is
falsy/false, throw or return a clear CLI error (with context like templateId and
handle) instead of proceeding to access .text/.text_parts; update the push-site
for ReconciliationText.read, AccountTemplate.read, and SharedPart.read to guard
the read result and surface a user-friendly error when validation fails.
---
Nitpick comments:
In `@lib/liquidSamplerRunner.js`:
- Around line 199-219: The spinner can be left running if an exception occurs
during polling; wrap the polling logic that begins with spinner.spin(...) and
the while loop that calls SF.readSamplerRun(...) in a try...finally block so
that spinner.stop() is always called in the finally branch; keep
spinner.spin(...) before the try, perform the await polling and updates to
samplerRun, waitingTime, pollingDelay and timeout checks inside the try, and
then call spinner.stop() in finally (rethrow or return samplerRun after the
try/finally as appropriate) to ensure cleanup even on errors.
🪄 Autofix (Beta)
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: 15ca1da7-2758-4c5e-b1a7-bfba88734f78
📒 Files selected for processing (3)
bin/cli.jslib/api/sfApi.jslib/liquidSamplerRunner.js
✅ Files skipped from review due to trivial changes (1)
- bin/cli.js
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/api/sfApi.js
AgustinSilverfin
force-pushed
the
agustin-bso-sampler
branch
from
43f855b to
530f325
Compare
michieldegezelle
force-pushed
the
agustin-bso-sampler
branch
from
530f325 to
f8fec0e
Compare
feat: firm_ids parameter
Toby-Masters-SF
force-pushed
the
agustin-bso-sampler
branch
2 times, most recently
from
a22a601 to
ab30d65
Compare
Toby-Masters-SF
force-pushed
the
agustin-bso-sampler
branch
from
ab30d65 to
dffd8df
Compare
Toby-Masters-SF
force-pushed
the
agustin-bso-sampler
branch
from
58e0993 to
cba14f8
Compare
Toby-Masters-SF
force-pushed
the
agustin-bso-sampler
branch
from
f030866 to
47ca5e4
Compare
Toby-Masters-SF
left a comment
Toby-Masters-SF
left a comment
There was a problem hiding this comment.
Reviewed with changes made
Toby-Masters-SF
force-pushed
the
agustin-bso-sampler
branch
from
e585999 to
7649bc3
Compare
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
📋 Comprehensive Review Summary
Status: 🛑 REQUEST CHANGES before merge
Critical Issues (5 blockers)
✅ Posted inline comments above for:
- Path traversal via template handle validation
- firmIds type mismatch (strings vs numbers)
- Credentials exposure in error logs
- partner_id type coercion bug
- API response pattern inconsistency
Test Coverage Gap
LiquidSamplerRunner class or run-sampler CLI command. Other lib modules have corresponding .test.js files. Recommend:
- Add
tests/lib/liquidSamplerRunner.test.js - Add
tests/bin/cli/run-sampler.test.js
Additional Findings
🟡 Medium: Empty object validation (line 175), unvalidated URL from API
🟢 Low: Integer overflow check, status pre-validation, code style (spacing + trailing whitespace)
What's Good ✅
- All CodeRabbit findings properly addressed
- Error handling correctly structured
- CLI options and imports verified correct
- CI passing (565 tests)
- Version bump synchronized
Estimated Fix Time
- High-severity fixes: 2-3 hours
- Test coverage: 1-2 hours
- Total: ~3-4 hours before re-review
Please address blockers and request re-review.
3 participants
Fixes # (link to the corresponding issue if applicable)
Description
Include a summary of the changes made
Testing Instructions
Steps:
Author Checklist
Reviewer Checklist
Notes
GH Action
GH action currently calls run-sampler as follows:
run-sampler -p ${SAMPLER_PARTNER} -h ${SAMPLER_HANDLES} ${FIRM_ARGS} --compact--compactadded as part of 2nd PR atop this PRFIRM_ARGSis simply-foption followed by a list of firms