Add workflow option to stats command#247
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR extends the ChangesPer-workflow statistics reporting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
Toby-Masters-SF
force-pushed
the
241-workflow-test-statistics
branch
3 times, most recently
from
0e6fed1 to
3fb5199
Compare
Toby-Masters-SF
force-pushed
the
241-workflow-test-statistics
branch
from
3fb5199 to
b134da4
Compare
Toby-Masters-SF
force-pushed
the
241-workflow-test-statistics
branch
2 times, most recently
from
b6c253d to
66ed189
Compare
Toby-Masters-SF
force-pushed
the
241-workflow-test-statistics
branch
from
66ed189 to
51fc334
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/cli/stats.js (2)
370-371: 💤 Low valueMisleading parameter name
templatesInWorkflow.The parameter is named
templatesInWorkflowbut it actually receives a full workflow object (which is then passed toyamlFilesActivity). The naming is inconsistent with the actual usage. Consider renaming toworkflowfor clarity.-async function getYamlSummary(sinceDate, templatesInWorkflow) { - const yamlActivity = await yamlFilesActivity(sinceDate, templatesInWorkflow); +async function getYamlSummary(sinceDate, workflow) { + const yamlActivity = await yamlFilesActivity(sinceDate, workflow);🤖 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 `@lib/cli/stats.js` around lines 370 - 371, The parameter name templatesInWorkflow in function getYamlSummary is misleading because it actually expects a full workflow object passed into yamlFilesActivity; rename the parameter to workflow (update the function signature of getYamlSummary and all internal references) and update any call sites to pass the same workflow variable name so usage is consistent with the actual object being passed to yamlFilesActivity.
319-344: 💤 Low valueNo defensive checks for missing workflow properties.
If a workflow JSON file is malformed or missing the
templatesproperty (or its sub-properties likereconciliations,exports,accounts), the code will throw with an unclear error. Consider adding defensive checks or clearer error messages.💡 Optional: Add defensive validation
// Fetch workflow // Assume no empty items if present within the workflow. + if (!workflow.templates) { + consola.error(`Workflow "${workflow.name || 'unknown'}" is missing 'templates' property`); + process.exit(1); + } summary.workflow_name = workflow.name; // Reconciliations - const reconciliationsInWorkflow = workflow.templates.reconciliations; + const reconciliationsInWorkflow = workflow.templates.reconciliations || []; // ... - const exportFilesInWorkflow = workflow.templates.exports; + const exportFilesInWorkflow = workflow.templates.exports || []; // ... - const accountTemplatesInWorkflow = workflow.templates.accounts; + const accountTemplatesInWorkflow = workflow.templates.accounts || [];🤖 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 `@lib/cli/stats.js` around lines 319 - 344, The code assumes workflow.templates and its sub-properties exist (e.g., workflow.templates.reconciliations, .exports, .accounts), which can throw on malformed workflow JSON; update the start of this block to defensively validate workflow and its template arrays (or normalize them) before using them: check that workflow is truthy, that workflow.templates is an object, and that workflow.templates.reconciliations, workflow.templates.exports, workflow.templates.accounts are arrays (fallback to [] if missing) or throw a clear, descriptive error; ensure subsequent uses of reconciliationsInWorkflow, exportFilesInWorkflow, accountTemplatesInWorkflow, and calls to listExternallyManagedTemplates/countYamlFiles use these validated/normalized variables so percentageRoundTwo and .length won't fail.
🤖 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 `@lib/cli/stats.js`:
- Around line 23-26: The code currently sets workflowsFolder and populates
workflowHandles using fs.readdirSync which will throw if the "workflows"
directory is missing; update the logic around the
workflowsFolder/workflowHandles assignment (the workflowsFolder const and
workflowHandles variable) to handle a missing folder by either checking
fs.existsSync(workflowsFolder) before calling fs.readdirSync or wrapping
readdirSync in a try/catch and falling back to an empty array, and ensure any
caught error is handled or logged rather than allowed to crash the process.
- Around line 55-58: The TEMPLATE_PATTERN is built from raw template names
(templatesInWorkflow) so regex metacharacters in names break the pattern; before
joining, escape each template string (e.g., implement or use an escapeRegExp
helper and map over templatesInWorkflow to escape chars like . * + ? ^ $ ( ) [ ]
{ } | \ /) then join the escaped names to build TEMPLATE_PATTERN and proceed to
construct YAML_EXPRESSION and fileTypeRegExp from that escaped pattern (refer to
templatesInWorkflow, TEMPLATE_PATTERN, YAML_EXPRESSION, fileTypeRegExp).
In `@lib/utils/fsUtils.js`:
- Around line 483-495: getWorkflow currently uses the raw CLI-provided
workflowHandle to build file paths allowing path traversal; update getWorkflow
to validate and sanitize workflowHandle (e.g., reject any path separators, "..",
or characters outside a safe whitelist like /^[A-Za-z0-9_-]+$/) and return a
clear error instead of accepting unsafe names, then apply the same sanitization
or safe-filename conversion in saveWorkflowOverviewToFile before constructing
CSV/output paths so both functions (getWorkflow and saveWorkflowOverviewToFile)
never join untrusted input into filesystem paths.
---
Nitpick comments:
In `@lib/cli/stats.js`:
- Around line 370-371: The parameter name templatesInWorkflow in function
getYamlSummary is misleading because it actually expects a full workflow object
passed into yamlFilesActivity; rename the parameter to workflow (update the
function signature of getYamlSummary and all internal references) and update any
call sites to pass the same workflow variable name so usage is consistent with
the actual object being passed to yamlFilesActivity.
- Around line 319-344: The code assumes workflow.templates and its
sub-properties exist (e.g., workflow.templates.reconciliations, .exports,
.accounts), which can throw on malformed workflow JSON; update the start of this
block to defensively validate workflow and its template arrays (or normalize
them) before using them: check that workflow is truthy, that workflow.templates
is an object, and that workflow.templates.reconciliations,
workflow.templates.exports, workflow.templates.accounts are arrays (fallback to
[] if missing) or throw a clear, descriptive error; ensure subsequent uses of
reconciliationsInWorkflow, exportFilesInWorkflow, accountTemplatesInWorkflow,
and calls to listExternallyManagedTemplates/countYamlFiles use these
validated/normalized variables so percentageRoundTwo and .length won't fail.
🪄 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: 25e76814-59ef-4417-9c64-b5134a0b6c9a
📒 Files selected for processing (3)
bin/cli.jslib/cli/stats.jslib/utils/fsUtils.js
| } else { | ||
| const workflowsFolder = path.join(process.cwd(), "workflows"); | ||
| workflowHandles = fs.readdirSync(workflowsFolder).map((file) => path.basename(file, ".json")); | ||
| } |
There was a problem hiding this comment.
Missing error handling when workflows folder doesn't exist.
If the workflows directory doesn't exist, fs.readdirSync will throw an uncaught ENOENT error. This could happen in repositories that don't have any workflows defined.
🛡️ Proposed fix to handle missing workflows folder
} else {
const workflowsFolder = path.join(process.cwd(), "workflows");
+ if (!fs.existsSync(workflowsFolder)) {
+ consola.error("No workflows folder found. Please create a 'workflows' directory with workflow JSON files.");
+ process.exit(1);
+ }
workflowHandles = fs.readdirSync(workflowsFolder).map((file) => path.basename(file, ".json"));
+ if (workflowHandles.length === 0) {
+ consola.info("No workflow files found in the workflows folder.");
+ 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.
| } else { | |
| const workflowsFolder = path.join(process.cwd(), "workflows"); | |
| workflowHandles = fs.readdirSync(workflowsFolder).map((file) => path.basename(file, ".json")); | |
| } | |
| } else { | |
| const workflowsFolder = path.join(process.cwd(), "workflows"); | |
| if (!fs.existsSync(workflowsFolder)) { | |
| consola.error("No workflows folder found. Please create a 'workflows' directory with workflow JSON files."); | |
| process.exit(1); | |
| } | |
| workflowHandles = fs.readdirSync(workflowsFolder).map((file) => path.basename(file, ".json")); | |
| if (workflowHandles.length === 0) { | |
| consola.info("No workflow files found in the workflows folder."); | |
| return; | |
| } | |
| } |
🤖 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 `@lib/cli/stats.js` around lines 23 - 26, The code currently sets
workflowsFolder and populates workflowHandles using fs.readdirSync which will
throw if the "workflows" directory is missing; update the logic around the
workflowsFolder/workflowHandles assignment (the workflowsFolder const and
workflowHandles variable) to handle a missing folder by either checking
fs.existsSync(workflowsFolder) before calling fs.readdirSync or wrapping
readdirSync in a try/catch and falling back to an empty array, and ensure any
caught error is handled or logged rather than allowed to crash the process.
There was a problem hiding this comment.
Additional context: the same readdirSync line also lacks a filter for non-JSON files. Every entry in the folder is mapped through path.basename(file, '.json') — so .DS_Store, README.md, a subdirectory, etc. produce invalid handles (e.g. .DS_Store unchanged). getWorkflow('.DS_Store') then looks for workflows/.DS_Store.json, doesn't find it, and calls process.exit(1), aborting the entire run before any remaining valid workflows are processed. The fix proposed above should also include a .filter(f => f.endsWith('.json')) before the .map(...).
There was a problem hiding this comment.
@michieldegezelle, excellent catch — that's a real footgun. Without the filter, even a stray .DS_Store or README.md silently breaks the whole run via process.exit(1) inside getWorkflow. Here's the combined fix:
} else {
const workflowsFolder = path.join(process.cwd(), "workflows");
if (!fs.existsSync(workflowsFolder)) {
consola.error("No workflows folder found. Please create a 'workflows' directory with workflow JSON files.");
process.exit(1);
}
workflowHandles = fs.readdirSync(workflowsFolder)
+ .filter((file) => file.endsWith(".json"))
.map((file) => path.basename(file, ".json"));
if (workflowHandles.length === 0) {
consola.info("No workflow files found in the workflows folder.");
return;
}
}The .filter(f => f.endsWith('.json')) ensures that directories, dotfiles (.DS_Store), and other non-JSON files are excluded before path.basename strips the extension — so only valid workflow handles ever reach getWorkflow.
✏️ Learnings added
Learnt from: michieldegezelle
Repo: silverfin/silverfin-cli PR: 247
File: lib/cli/stats.js:23-26
Timestamp: 2026-06-11T07:33:04.929Z
Learning: In `lib/cli/stats.js` (`generateWorkflowOverview`), when reading workflow handles from the `workflows/` folder via `fs.readdirSync`, the result must be filtered to `.json` files only (`.filter(f => f.endsWith('.json'))`) before mapping with `path.basename(file, '.json')`. Without this filter, non-JSON entries like `.DS_Store`, `README.md`, or subdirectories produce invalid handles that cause `getWorkflow()` to call `process.exit(1)`, aborting the entire run.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const templatesInWorkflow = workflow ? workflow.templates.reconciliations.concat(workflow.templates.accounts) : []; | ||
| const TEMPLATE_PATTERN = workflow ? `(${templatesInWorkflow.join("|")})` : `.*`; | ||
| const YAML_EXPRESSION = `.*/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`; | ||
| const fileTypeRegExp = RegExp(YAML_EXPRESSION, "g"); |
There was a problem hiding this comment.
Regex metacharacters in template names are not escaped.
If template names in the workflow contain regex special characters (e.g., ., +, *, (, )), the constructed regex pattern will behave unexpectedly. Template names are joined and used directly in a regex pattern without escaping.
🔧 Proposed fix to escape regex special characters
+function escapeRegex(string) {
+ return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
+
async function yamlFilesActivity(sinceDate, workflow) {
// ...
const templatesInWorkflow = workflow ? workflow.templates.reconciliations.concat(workflow.templates.accounts) : [];
- const TEMPLATE_PATTERN = workflow ? `(${templatesInWorkflow.join("|")})` : `.*`;
+ const TEMPLATE_PATTERN = workflow ? `(${templatesInWorkflow.map(escapeRegex).join("|")})` : `.*`;
const YAML_EXPRESSION = `.*/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`;🤖 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 `@lib/cli/stats.js` around lines 55 - 58, The TEMPLATE_PATTERN is built from
raw template names (templatesInWorkflow) so regex metacharacters in names break
the pattern; before joining, escape each template string (e.g., implement or use
an escapeRegExp helper and map over templatesInWorkflow to escape chars like . *
+ ? ^ $ ( ) [ ] { } | \ /) then join the escaped names to build TEMPLATE_PATTERN
and proceed to construct YAML_EXPRESSION and fileTypeRegExp from that escaped
pattern (refer to templatesInWorkflow, TEMPLATE_PATTERN, YAML_EXPRESSION,
fileTypeRegExp).
There was a problem hiding this comment.
The identical pattern exists in countYamlFiles (line 99), where TEMPLATE_PATTERN is built the same way from templatesInWorkflow. The escapeRegex helper proposed above needs to be applied there too, otherwise workflow-filtered YAML counts will have the same unescaped-metacharacter exposure as the activity scan.
There was a problem hiding this comment.
@michieldegezelle, you're absolutely right — the same unescaped-metacharacter exposure exists in countYamlFiles. Here's the combined fix that defines the helper once and applies it in both places:
+function escapeRegex(string) {
+ return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
+
async function yamlFilesActivity(sinceDate, workflow) {
// ...
const templatesInWorkflow = workflow
? workflow.templates.reconciliations.concat(workflow.templates.accounts)
: [];
- const TEMPLATE_PATTERN = workflow ? `(${templatesInWorkflow.join("|")})` : `.*`;
+ const TEMPLATE_PATTERN = workflow
+ ? `(${templatesInWorkflow.map(escapeRegex).join("|")})`
+ : `.*`;
const YAML_EXPRESSION = `.*/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`;
// ...
}
async function countYamlFiles(templateType, templatesInWorkflow) {
// ...
- const TEMPLATE_PATTERN = templatesInWorkflow ? `(${templatesInWorkflow.join("|")})` : `.*`;
+ const TEMPLATE_PATTERN = templatesInWorkflow
+ ? `(${templatesInWorkflow.map(escapeRegex).join("|")})`
+ : `.*`;
const YAML_EXPRESSION = `.*${FOLDER}/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`;
// ...
}Both the activity scan and the YAML file count share the same vulnerable pattern-building logic, so both need the same fix. Thanks for catching this!
| function getWorkflow(workflowHandle) { | ||
| try { | ||
| const workflowPath = path.join(process.cwd(), "workflows", `${workflowHandle}.json`); | ||
| if (!fs.existsSync(workflowPath)) { | ||
| throw new Error(`Workflow "${workflowHandle}" not found`); | ||
| } | ||
| return JSON.parse(fs.readFileSync(workflowPath).toString()); | ||
| } catch (error) { | ||
| consola.error(`An error occurred when trying to read the workflow "${workflowHandle}"`); | ||
| consola.error(error); | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsanitized workflowHandle from CLI enables path traversal in multiple locations.
Both getWorkflow in lib/utils/fsUtils.js and saveWorkflowOverviewToFile in lib/cli/stats.js use the CLI-provided workflowHandle directly in file paths. The root cause is that the --workflow CLI argument is not validated at the entry point. Consider adding validation in getWorkflow (which is called first) and ensuring safe filenames in the CSV output function.
🤖 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 `@lib/utils/fsUtils.js` around lines 483 - 495, getWorkflow currently uses the
raw CLI-provided workflowHandle to build file paths allowing path traversal;
update getWorkflow to validate and sanitize workflowHandle (e.g., reject any
path separators, "..", or characters outside a safe whitelist like
/^[A-Za-z0-9_-]+$/) and return a clear error instead of accepting unsafe names,
then apply the same sanitization or safe-filename conversion in
saveWorkflowOverviewToFile before constructing CSV/output paths so both
functions (getWorkflow and saveWorkflowOverviewToFile) never join untrusted
input into filesystem paths.
| .action((options) => { | ||
| stats.generateOverview(options.since); | ||
| if (options.workflow) { | ||
| const workflowHandle = typeof options.workflow === "string" ? options.workflow : undefined; |
There was a problem hiding this comment.
--workflow without a value silently runs all-workflows scan instead of erroring.
The option is declared with optional-argument syntax [handle], so running silverfin stats -s 2024-01-01 --workflow (flag present, no value) causes Commander to set options.workflow = true (boolean). The if (options.workflow) check on the line above is truthy, but this typeof === 'string' guard then sets workflowHandle = undefined. As a result, generateWorkflowOverview(sinceDate, undefined) is called and silently reads every file in the workflows/ folder — the user gets a full-repo scan with no indication their argument was dropped.
Consider using <handle> (required argument) instead of [handle], or explicitly check options.workflow === true and print an error.
| fs.mkdirSync("stats"); | ||
| } | ||
| if (!fs.existsSync(CSV_PATH)) { | ||
| fs.writeFileSync(CSV_PATH, ROW_HEADER, (err) => { |
There was a problem hiding this comment.
fs.writeFileSync silently ignores the callback passed as the third argument.
writeFileSync is synchronous and its third parameter is an options object (encoding, mode, flag) — not an error handler. The callback (err) => { consola.error(err); } is coerced to an options object and silently discarded. Any write failure (e.g. permission denied, disk full) throws an uncaught synchronous exception rather than being handled. The same issue exists at line 592 in saveOverviewToFile.
Remove the callback and either let the exception propagate naturally, or wrap the call in a try/catch.
| // Files to Search (YAML) | ||
| const YAML_EXPRESSION = `.*/.*/tests/.*_liquid_test.*.y(a)?ml`; | ||
| const templatesInWorkflow = workflow ? workflow.templates.reconciliations.concat(workflow.templates.accounts) : []; | ||
| const TEMPLATE_PATTERN = workflow ? `(${templatesInWorkflow.join("|")})` : `.*`; |
There was a problem hiding this comment.
Empty template list produces a broken regex pattern.
When a workflow has no reconciliations and no accounts, templatesInWorkflow is [], so TEMPLATE_PATTERN becomes () and YAML_EXPRESSION effectively matches all YAML test paths — the opposite of filtering. Consider returning early (or skipping YAML activity) when the combined list is empty.
| const FOLDER = fsUtils.FOLDERS[templateType]; | ||
| const YAML_EXPRESSION = `.*${FOLDER}/.*/tests/.*_liquid_test.*.y(a)?ml`; | ||
| const TEMPLATE_PATTERN = templatesInWorkflow ? `(${templatesInWorkflow.join("|")})` : `.*`; | ||
| // Issue with counting multiple yml files in the same tests folder? |
There was a problem hiding this comment.
Leftover open question in a comment (Issue with counting multiple yml files in the same tests folder?). Either confirm this is not a problem and remove the comment, or document/fix the edge case.
| }; | ||
|
|
||
| // Fetch workflow | ||
| // Assume no empty items if present within the workflow. |
There was a problem hiding this comment.
Global vs workflow stats count different populations.
generateOverview counts only non-empty templates (listNonEmptyTemplates), while workflow mode counts every handle listed in the workflow JSON. That is probably fine, but worth documenting in the CLI help or PR description so users do not compare the two totals directly.
| return summary; | ||
| } | ||
|
|
||
| async function getYamlSummary(sinceDate, templatesInWorkflow) { |
There was a problem hiding this comment.
Minor: the second parameter is named templatesInWorkflow but callers pass the full workflow object into yamlFilesActivity. Renaming to workflow would match actual usage.
2 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
Plan
getWorkflowTemplateSummary(workflowHandle)functionlistWorkflowTemplatesfunction which can take different classes?Testing notes
ymlfiles in the sametestsfolder so may need to update the Regex expression:.*${FOLDER}/${templatePattern}/tests/.*_liquid_test.*.y(a)?ml. Resolved by temporarily deleting "extra" tests in test branchQuick Notes
clifolder?getTemplatesSummary--> Can it be co-opted for the purposes of this workflow? Take the "Reconciliations" section as an example!