Surface sampler report URL and quiet CI output#261
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:
WalkthroughThe sampler CLI adds CI-aware report behavior and compact named-results diff output. The runner downloads and extracts selected result files, formats Markdown changes, and exposes report-opening controls. Tests and fixtures cover diff semantics, aggregation, formatting, and runner integration. ChangesCompact sampler reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
🤖 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/liquidSamplerCompact.js`:
- Around line 95-100: Replace the JSON.stringify comparison in the compact diff
loop with an order-insensitive deep comparison for JSON values, so object
property insertion order does not produce changes while array element order
remains significant. Reuse an existing deep-comparison helper if available,
otherwise update the comparison logic near the changes loop without altering
renderValue or key traversal behavior.
- Around line 61-66: Update the ENTRY_KINDS iteration in the label-map
construction and the corresponding entryIds handling to use `${kind}/${entryId}`
as the identity key. Ensure labelMap entries and changed-entry tracking both
preserve separate account and reconciliation records when their raw IDs match,
while retaining the existing label and URL metadata.
- Around line 129-133: Update the sampling flow around entriesSampled and the
before/after readNamedResults calls so entries with missing or malformed
registers.json files are not counted as successfully sampled. Either abort
compact generation when before or after is null, or track skipped entries and
disclose their count in the resulting output; preserve accurate reporting for
entries that are actually analyzed.
In `@lib/liquidSamplerRunner.js`:
- Around line 330-332: Update `#downloadAndExtractResults` to pass a configurable
timeout option to axios.get when downloading resultUrl. Reuse the existing
timeout configuration mechanism, and preserve the current arraybuffer response
handling.
- Around line 334-344: Harden the ZIP extraction flow around the tempDir
creation and entry loop: resolve each destination path and reject any entry that
escapes tempDir, including traversal via ../, before writing files. Ensure
tempDir is removed in a finally block whenever extraction fails before
returning, while preserving the existing selected-entry extraction behavior on
success.
In `@tests/lib/liquidSamplerCompact.test.js`:
- Around line 77-82: Update the test named “falls back to raw entry id when
labels are missing” to create a temporary fixture containing an output entry
with registers.json but no sample_entry_ids.yml. Run extractCompact against that
fixture and assert the resulting template label equals the entry ID, ensuring
the raw-ID fallback is exercised with an actual entry rather than an empty
directory.
🪄 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: 4eba78f0-4868-442f-80f1-691d507191f3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
bin/cli.jslib/liquidSamplerCompact.jslib/liquidSamplerRunner.jspackage.jsontests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/after/registers.jsontests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/before/registers.jsontests/fixtures/sampler-results/sample_entry_ids.ymltests/lib/liquidSamplerCompact.test.jstests/lib/liquidSamplerRunner.test.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 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/liquidSamplerCompact.js`:
- Around line 95-100: Replace the JSON.stringify comparison in the compact diff
loop with an order-insensitive deep comparison for JSON values, so object
property insertion order does not produce changes while array element order
remains significant. Reuse an existing deep-comparison helper if available,
otherwise update the comparison logic near the changes loop without altering
renderValue or key traversal behavior.
- Around line 61-66: Update the ENTRY_KINDS iteration in the label-map
construction and the corresponding entryIds handling to use `${kind}/${entryId}`
as the identity key. Ensure labelMap entries and changed-entry tracking both
preserve separate account and reconciliation records when their raw IDs match,
while retaining the existing label and URL metadata.
- Around line 129-133: Update the sampling flow around entriesSampled and the
before/after readNamedResults calls so entries with missing or malformed
registers.json files are not counted as successfully sampled. Either abort
compact generation when before or after is null, or track skipped entries and
disclose their count in the resulting output; preserve accurate reporting for
entries that are actually analyzed.
In `@lib/liquidSamplerRunner.js`:
- Around line 330-332: Update `#downloadAndExtractResults` to pass a configurable
timeout option to axios.get when downloading resultUrl. Reuse the existing
timeout configuration mechanism, and preserve the current arraybuffer response
handling.
- Around line 334-344: Harden the ZIP extraction flow around the tempDir
creation and entry loop: resolve each destination path and reject any entry that
escapes tempDir, including traversal via ../, before writing files. Ensure
tempDir is removed in a finally block whenever extraction fails before
returning, while preserving the existing selected-entry extraction behavior on
success.
In `@tests/lib/liquidSamplerCompact.test.js`:
- Around line 77-82: Update the test named “falls back to raw entry id when
labels are missing” to create a temporary fixture containing an output entry
with registers.json but no sample_entry_ids.yml. Run extractCompact against that
fixture and assert the resulting template label equals the entry ID, ensuring
the raw-ID fallback is exercised with an actual entry rather than an empty
directory.
🪄 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: 4eba78f0-4868-442f-80f1-691d507191f3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
bin/cli.jslib/liquidSamplerCompact.jslib/liquidSamplerRunner.jspackage.jsontests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/after/registers.jsontests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/before/registers.jsontests/fixtures/sampler-results/sample_entry_ids.ymltests/lib/liquidSamplerCompact.test.jstests/lib/liquidSamplerRunner.test.js
🛑 Comments failed to post (6)
lib/liquidSamplerCompact.js (3)
61-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include the entry kind in entry identity.
Account and reconciliation entries can share the same raw ID. The label map will overwrite one label, and
entryIdscan then undercount changed entries. Key both structures by${kind}/${entryId}.Proposed fix
- labelMap[entryId] = { + labelMap[`${kind}/${entryId}`] = { ... - const label = (labelMap[entryId] && labelMap[entryId].label) || entryId; + const entryKey = `${kind}/${entryId}`; + const label = (labelMap[entryKey] && labelMap[entryKey].label) || entryId; ... - bucket.entryIds.add(entryId); + bucket.entryIds.add(entryKey);Also applies to: 138-143
🤖 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/liquidSamplerCompact.js` around lines 61 - 66, Update the ENTRY_KINDS iteration in the label-map construction and the corresponding entryIds handling to use `${kind}/${entryId}` as the identity key. Ensure labelMap entries and changed-entry tracking both preserve separate account and reconciliation records when their raw IDs match, while retaining the existing label and URL metadata.
95-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare JSON values structurally, not by serialization order.
Two objects containing identical properties in different insertion orders produce different
JSON.stringifyoutput, creating a false compact diff and defeating deduplication. Use an order-insensitive deep comparison while preserving array order.🤖 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/liquidSamplerCompact.js` around lines 95 - 100, Replace the JSON.stringify comparison in the compact diff loop with an order-insensitive deep comparison for JSON values, so object property insertion order does not produce changes while array element order remains significant. Reuse an existing deep-comparison helper if available, otherwise update the comparison logic near the changes loop without altering renderValue or key traversal behavior.
129-133: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not report unreadable entries as successfully sampled.
An entry with a missing or malformed
registers.jsonincrementsentriesSampledand is silently skipped. This can produce “No changes across N sampled entries” even though some entries were never analyzed. Either fail compact generation or track and disclose skipped entries.🤖 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/liquidSamplerCompact.js` around lines 129 - 133, Update the sampling flow around entriesSampled and the before/after readNamedResults calls so entries with missing or malformed registers.json files are not counted as successfully sampled. Either abort compact generation when before or after is null, or track skipped entries and disclose their count in the resulting output; preserve accurate reporting for entries that are actually analyzed.lib/liquidSamplerRunner.js (2)
330-332: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C3 'axios\.get\s*\(' lib/liquidSamplerRunner.jsRepository: silverfin/silverfin-cli
Length of output: 549
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "## locate usages" rg -n -C3 'downloadAndExtractResults|axios\.get\s*\(|timeout\s*:' lib . --glob '!**/node_modules/**' || true echo echo "## inspect relevant file section" sed -n '1,260p' lib/liquidSamplerRunner.js echo echo "## search for axios setup / defaults" rg -n -C2 'axios\.defaults|create\(|timeout\s*:' lib . --glob '!**/node_modules/**' || trueRepository: silverfin/silverfin-cli
Length of output: 18137
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "## repo-wide timeout/default search" rg -n -C2 'axios\.defaults|timeout\s*:|timeout\s*\)|timeout\b' . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' || true echo echo "## direct axios imports and gets" rg -n -C2 'require\("axios"\)|from "axios"|from '\''axios'\''|axios\.get\s*\(' lib tests . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' || true echo echo "## inspect axiosFactory timeout-relevant sections" sed -n '1,220p' lib/api/axiosFactory.jsRepository: silverfin/silverfin-cli
Length of output: 16558
Bound the compact-result download.
axios.get()has no timeout here, so a stalled results URL can hang the CLI/CI job indefinitely. Add a configurable timeout.🤖 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/liquidSamplerRunner.js` around lines 330 - 332, Update `#downloadAndExtractResults` to pass a configurable timeout option to axios.get when downloading resultUrl. Reuse the existing timeout configuration mechanism, and preserve the current arraybuffer response handling.
334-344: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash python - <<'PY' import os root = os.path.abspath("/tmp/silverfin-sampler-test") for name in ("sample_entry_ids.yml", "../escaped/registers.json", "/tmp/registers.json"): destination = os.path.abspath(os.path.join(root, name)) print(name, "inside:", destination.startswith(root + os.sep)) PYRepository: silverfin/silverfin-cli
Length of output: 269
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the relevant function and nearby context ast-grep outline lib/liquidSamplerRunner.js --view expanded || true printf '\n--- lines around 300-380 ---\n' sed -n '300,380p' lib/liquidSamplerRunner.js printf '\n--- call sites / cleanup usage ---\n' rg -n "sample_entry_ids\.yml|registers\.json|mkdtempSync|rmSync\\(|tempDir" lib/liquidSamplerRunner.jsRepository: silverfin/silverfin-cli
Length of output: 3572
Keep ZIP extraction confined to
tempDir.path.join(tempDir, name)still lets../entries escape the temp dir, and a failure before return leaves partial extraction behind; reject paths outside the temp root and clean up infinally.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 340-340: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(dest, entry.getData())
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').(detect-non-literal-fs-filename)
🤖 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/liquidSamplerRunner.js` around lines 334 - 344, Harden the ZIP extraction flow around the tempDir creation and entry loop: resolve each destination path and reject any entry that escapes tempDir, including traversal via ../, before writing files. Ensure tempDir is removed in a finally block whenever extraction fails before returning, while preserving the existing selected-entry extraction behavior on success.Source: Linters/SAST tools
tests/lib/liquidSamplerCompact.test.js (1)
77-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the raw-ID fallback with an actual entry.
This test uses a directory containing neither YAML nor output entries, so it only verifies empty-directory behavior. Create a temporary fixture with
output/.../registers.jsonbut nosample_entry_ids.yml, then assert that the resulting template label equals its entry ID.🤖 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 `@tests/lib/liquidSamplerCompact.test.js` around lines 77 - 82, Update the test named “falls back to raw entry id when labels are missing” to create a temporary fixture containing an output entry with registers.json but no sample_entry_ids.yml. Run extractCompact against that fixture and assert the resulting template label equals the entry ID, ensuring the raw-ID fallback is exercised with an actual entry rather than an empty directory.
Toby-Masters-SF
force-pushed
the
agustin-bso-sampler
branch
2 times, most recently
from
ab30d65 to
dffd8df
Compare
michieldegezelle
force-pushed
the
agustin-bso-sampler-easy-results
branch
from
8d8b8b4 to
ccc4bd5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/liquidSamplerCompact.js (1)
93-104: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValue diff is key-order sensitive.
JSON.stringify(b) !== JSON.stringify(a)compares object/arraynamed_resultsvalues by their serialized string. If a value is a nested object whose key order differs between "before" and "after" snapshots (e.g. due to a hash/map rebuild) but the content is unchanged, this reports a false-positive change. Consider a canonical/stable stringify (sort object keys recursively) or a proper deep-equal for the comparison, at least for non-primitive values.♻️ Example fix using a stable stringify
+function stableStringify(value) { + if (Array.isArray(value)) return `[${value.map(stableStringify).join(",")}]`; + if (value && typeof value === "object") { + return `{${Object.keys(value) + .sort() + .map((k) => `${JSON.stringify(k)}:${stableStringify(value[k])}`) + .join(",")}}`; + } + return JSON.stringify(value); +} + function diffNamedResults(before, after) { const changes = []; const keys = new Set([...Object.keys(before), ...Object.keys(after)]); for (const key of [...keys].sort()) { const b = Object.hasOwn(before, key) ? before[key] : ABSENT; const a = Object.hasOwn(after, key) ? after[key] : ABSENT; - if (JSON.stringify(b) !== JSON.stringify(a)) { + if (stableStringify(b) !== stableStringify(a)) { changes.push({ key, before: renderValue(b), after: renderValue(a) }); } } return changes; }🤖 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/liquidSamplerCompact.js` around lines 93 - 104, Update diffNamedResults to compare non-primitive before and after values using a key-order-independent deep comparison, such as recursively canonicalizing object keys before serialization. Preserve the existing ABSENT handling and renderValue output, while ensuring equivalent nested objects are not reported as changes.
🤖 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/liquidSamplerRunner.js`:
- Around line 330-344: Validate each selected zip entry’s resolved destination
stays inside the temporary directory before creating directories or writing
files. In the extraction loop around zip.getEntries and the tempDir path,
resolve the destination and reject entries that escape tempDir, including
traversal names such as ../../registers.json, while preserving the existing
filename filters for valid entries.
- Around line 329-331: Add a finite Axios request timeout to the download
performed by `#downloadAndExtractResults`, using the existing request options or
an appropriate shared Axios default. Ensure stalled presigned-URL requests fail
promptly so the best-effort `#printCompactDiff` path does not hang indefinitely.
---
Nitpick comments:
In `@lib/liquidSamplerCompact.js`:
- Around line 93-104: Update diffNamedResults to compare non-primitive before
and after values using a key-order-independent deep comparison, such as
recursively canonicalizing object keys before serialization. Preserve the
existing ABSENT handling and renderValue output, while ensuring equivalent
nested objects are not reported as changes.
🪄 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: 7e0589a4-e776-4f21-b6b7-98350c558c26
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
bin/cli.jslib/liquidSamplerCompact.jslib/liquidSamplerRunner.jspackage.jsontests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/after/registers.jsontests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/before/registers.jsontests/fixtures/sampler-results/sample_entry_ids.ymltests/lib/liquidSamplerCompact.test.jstests/lib/liquidSamplerRunner.test.js
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/before/registers.json
- tests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/before/registers.json
- tests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/after/registers.json
- package.json
- tests/fixtures/sampler-results/sample_entry_ids.yml
- tests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/after/registers.json
- tests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/before/registers.json
- tests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/before/registers.json
- tests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/after/registers.json
- tests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/after/registers.json
- tests/lib/liquidSamplerRunner.test.js
- tests/lib/liquidSamplerCompact.test.js
- bin/cli.js
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
🟠 Major — lib/liquidSamplerCompact.js:145 (can't be posted as an inline comment because GitHub treats this new file as binary and has no diff positions for it — see below):
const dedupKey = `${change.key}\\x00${change.before}\\x00${change.after}`;This line contains a literal raw NUL byte (0x00) between the template-literal segments, not the two-character escape sequence \\x00. Confirmed with file lib/liquidSamplerCompact.js → data (not "ASCII text"), and gh pr diff 261 renders this entire new 213-line file as Binary files /dev/null and b/lib/liquidSamplerCompact.js differ — it cannot be reviewed line-by-line on GitHub at all, and any future diff touching this file will likely render as binary too.
Functionally the code still works (a literal NUL is a valid, if unusual, dedup separator in a JS string), but this should be the actual two-character escape sequence in source so the file stays reviewable text going forward.
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
🟠 Major — lib/liquidSamplerCompact.js:61-66 (can't be posted as an inline comment — this file has a literal NUL byte at line 145 that makes GitHub treat the whole file as binary, so it has no diff positions; see the other top-level comment on this PR about that. This finding was originally raised by CodeRabbit's first review round on this file, but that round's inline comments all failed to post due to a GitHub API error, and it was never re-surfaced in the later round either — still present and unfixed):
for (const kind of ENTRY_KINDS) {
for (const [entryId, meta] of Object.entries(parsed[kind] || {})) {
labelMap[entryId] = {
label: (meta && meta.label) || entryId,
url: (meta && meta.url) || null,
};
}
}labelMap is keyed only by the raw entryId, shared across both account_entries and reconciliation_entries. Their id spaces aren't guaranteed disjoint, so if an account entry and a reconciliation entry happen to share a numeric id, the second kind processed silently overwrites the first's label/url — and extractCompact's lookup (labelMap[entryId], ~line 138) can then attribute a changed entry to the wrong template. Key by ${kind}/${entryId} in both this map and the corresponding lookup/grouping in extractCompact.
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
🟠 Major — lib/liquidSamplerCompact.js:126-133 (same situation as the previous comment: raised by CodeRabbit's first review round, never actually posted due to that round's inline-comment failure, never re-surfaced afterward, still present and unfixed):
for (const entryId of fs.readdirSync(kindDir)) {
const entryDir = path.join(kindDir, entryId);
if (!fs.statSync(entryDir).isDirectory()) continue;
entriesSampled += 1;
const before = readNamedResults(path.join(entryDir, "before", "registers.json"));
const after = readNamedResults(path.join(entryDir, "after", "registers.json"));
if (before === null || after === null) continue;entriesSampled increments before checking whether before/after actually parsed. An entry whose registers.json is missing or malformed still counts as "sampled" even though it's silently skipped from the diff — so formatCompact's "No named_results changes across N sampled entries" message can overstate how many entries were genuinely analyzed, masking entries that were never compared at all. Either count only entries where both files parsed successfully, or track and surface the skipped count separately.
Always log the hosted result_url on completion so it can be captured in CI (e.g. echoed into $GITHUB_STEP_SUMMARY) without downloading anything. Only download+open the report locally when not in CI; add --no-open. Use a static poll message instead of the animated spinner in CI to avoid flooding the captured log with thousands of frames. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The sampler result zip is huge (rendered HTML alone is ~470K tokens; raw per-entry output is ~150 MB), which makes it unusable as a review artifact pasted into a chat/PR. --compact downloads the result, selectively extracts only sample_entry_ids.yml + every registers.json, and prints a compact named_results diff grouped by template handle and deduped by identical change, wrapped in <!-- SAMPLER_COMPACT_START/END --> markers for CI extraction. On a 228-entry run this is ~7 KB instead of ~150 MB, and a broken template surfaces plainly as `value -> undefined`. - lib/liquidSamplerCompact.js: pure extractCompact()/formatCompact() - liquidSamplerRunner: compact option; download+extract is best-effort and never fails the run; temp dir cleaned up after - bin/cli.js: --compact flag (works with --id, independent of CI) - adm-zip dependency for selective zip extraction - unit + E2E tests against an in-memory zip of a committed fixture Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
michieldegezelle
force-pushed
the
agustin-bso-sampler-easy-results
branch
from
ccc4bd5 to
a6149cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/liquidSamplerRunner.js`:
- Around line 332-342: Update the extraction flow around the temporary directory
created by mkdtempSync to clean it up locally when mkdirSync, entry.getData, or
writeFileSync fails. Wrap the iteration and file extraction in error handling
that removes tempDir before rethrowing, while preserving the successful return
of tempDir.
🪄 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: 174736f9-f02b-4dbf-b923-fca2f6904bfa
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
bin/cli.jslib/liquidSamplerCompact.jslib/liquidSamplerRunner.jspackage.jsontests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/after/registers.jsontests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/before/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/after/registers.jsontests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/before/registers.jsontests/fixtures/sampler-results/sample_entry_ids.ymltests/lib/liquidSamplerCompact.test.jstests/lib/liquidSamplerRunner.test.js
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/after/registers.json
- tests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/after/registers.json
- tests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/before/registers.json
- tests/fixtures/sampler-results/sample_entry_ids.yml
- tests/fixtures/sampler-results/output/account_entries/1_103_1003_490000.000/after/registers.json
- tests/lib/liquidSamplerRunner.test.js
- tests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/before/registers.json
- tests/fixtures/sampler-results/output/reconciliation_entries/1_102_1002_6000/before/registers.json
- tests/fixtures/sampler-results/output/reconciliation_entries/1_100_1000_5000/before/registers.json
- tests/fixtures/sampler-results/output/reconciliation_entries/1_101_1001_5000/after/registers.json
- tests/lib/liquidSamplerCompact.test.js
- bin/cli.js
The rebase in 58e7747 accidentally reverted this branch from consola.error + process.exit(1) (introduced in cba14f8) back to a bare warn. A completed-but-urlless run now exits 0 and produces no "Sampler report:" line, indistinguishable from a quiet success in CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
An entry name like "../../evil/registers.json" still ends with "/registers.json" and passes the filename filter, but path.join(tempDir, name) resolves outside tempDir. Resolve each destination and skip any entry whose resolved path isn't inside tempDir before writing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
axios has no default timeout, so a stalled connection could leave the best-effort compact-diff path (#printCompactDiff) hanging indefinitely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
lib/liquidSamplerCompact.js:145 embedded a literal raw 0x00 byte between template-literal segments instead of the two-character escape sequence. Functionally harmless (a valid dedup separator), but it makes git/GitHub treat the entire file as binary - `file` reports "data" and `gh pr diff` renders the whole 213-line file as a binary diff, so it can't be reviewed line-by-line and any future change to it would render the same way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ollisions
readEntryLabels keyed its map by raw entry id alone, shared across both
account_entries and reconciliation_entries. Their id spaces aren't
guaranteed disjoint, so a shared numeric id let the second kind
processed silently overwrite the first's label/url, misattributing a
changed entry to the wrong template. Key by "${kind}/${entryId}" in
both the map and extractCompact's lookup/grouping.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
entriesSampled incremented before checking whether before/after actually parsed, so an entry whose registers.json was missing or malformed still counted as "sampled" despite being silently skipped from the diff - overstating how many entries formatCompact's summary line claims were analyzed. Only count entries that were actually compared, and track/ disclose the skipped count separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
diffNamedResults compared values via JSON.stringify, which is sensitive to object property insertion order - a before/after pair that's semantically identical but was rebuilt through a different code path (e.g. a hash/map rebuild) could report a false-positive change. Compare via a stable stringify that sorts object keys recursively while still treating array element order as significant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/liquidSamplerRunner.js (1)
334-356: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTemp dir still leaks if extraction fails mid-loop (unresolved from a prior review).
tempDiris created at line 341, but ifmkdirSync/entry.getData()/writeFileSyncthrows partway through the loop, the exception propagates without returningtempDir. The caller (#printCompactDiff) only assignsresultsDirfrom the return value, so on this failure pathresultsDirstaysundefinedand thefinallyblock's cleanup (fs.rmSync) never runs — the directory is orphaned under the OS temp dir. This was flagged previously and the suggested wrap-in-try/catch fix was not applied to this version.Proposed fix
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "silverfin-sampler-")); - for (const entry of zip.getEntries()) { - if (entry.isDirectory) continue; - const name = entry.entryName; - if (name !== "sample_entry_ids.yml" && !name.endsWith("/registers.json")) continue; - - const dest = path.resolve(tempDir, name); - if (dest !== tempDir && !dest.startsWith(tempDir + path.sep)) continue; - - fs.mkdirSync(path.dirname(dest), { recursive: true }); - fs.writeFileSync(dest, entry.getData()); + try { + for (const entry of zip.getEntries()) { + if (entry.isDirectory) continue; + const name = entry.entryName; + if (name !== "sample_entry_ids.yml" && !name.endsWith("/registers.json")) continue; + + const dest = path.resolve(tempDir, name); + if (dest !== tempDir && !dest.startsWith(tempDir + path.sep)) continue; + + fs.mkdirSync(path.dirname(dest), { recursive: true }); + fs.writeFileSync(dest, entry.getData()); + } + return tempDir; + } catch (error) { + fs.rmSync(tempDir, { recursive: true, force: true }); + throw error; } - return tempDir; }The zip-slip guard itself (
path.resolve+startsWithcheck) correctly fixes the previously flagged path-traversal issue.🤖 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/liquidSamplerRunner.js` around lines 334 - 356, Update `#downloadAndExtractResults` so any extraction failure after tempDir creation still exposes the directory to the caller for cleanup, while preserving the existing error propagation and zip-slip filtering. Ensure `#printCompactDiff`’s finally cleanup can remove tempDir when mkdirSync, entry.getData, or writeFileSync fails mid-loop.
🤖 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/liquidSamplerCompact.js`:
- Around line 166-194: The byTemplate aggregation currently merges buckets that
share a label across different entry kinds. Update the grouping logic around
entryKey, label, and byTemplate to key buckets by a kind-qualified identity
while retaining the original label for display and sorting; ensure entries with
missing label metadata are also separated by kind. Add coverage for matching
labels and raw IDs across kinds, verifying templatesChanged and combined changes
remain distinct.
In `@tests/lib/liquidSamplerCompact.test.js`:
- Around line 76-81: The test around readEntryLabels must create metadata
containing the same raw ID for both account and reconciliation kinds, then
assert that both kind-specific mapped labels are preserved. Replace the
keys-include-slash assertion in the “keeps account and reconciliation entries
separate when their raw ids match” test with explicit checks for both labels,
using the existing metadata and label-mapping conventions.
---
Duplicate comments:
In `@lib/liquidSamplerRunner.js`:
- Around line 334-356: Update `#downloadAndExtractResults` so any extraction
failure after tempDir creation still exposes the directory to the caller for
cleanup, while preserving the existing error propagation and zip-slip filtering.
Ensure `#printCompactDiff`’s finally cleanup can remove tempDir when mkdirSync,
entry.getData, or writeFileSync fails mid-loop.
🪄 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: a586fbeb-23ec-43a9-a5f7-1ad9a218d460
📒 Files selected for processing (4)
lib/liquidSamplerCompact.jslib/liquidSamplerRunner.jstests/lib/liquidSamplerCompact.test.jstests/lib/liquidSamplerRunner.test.js
tempDir was only ever removed via the caller's finally block, which reads the resultsDir return value - a failure partway through the extraction loop (mkdirSync/getData/writeFileSync) propagates the exception without returning, so the directory is orphaned under the OS temp dir. Wrap the loop and clean up locally before rethrowing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
byTemplate was keyed only by label, so an account template and a reconciliation template with the same label (whether via matching metadata or both falling back to the same raw id when labels are missing) were merged into one bucket, producing an incorrect templatesChanged count and combined changes. Key by kind + label while keeping the label for display, and replace the weak "every key includes a slash" assertion with a real cross-kind id collision test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
🟠 Major: check-cli-version is currently failing on this PR (verified via gh pr checks 261 + the job log). origin/main's package.json is now at 1.57.0 (merged separately via #236's own version bump), and this branch's package.json/package-lock.json (unchanged by this PR's diff) are also still at 1.57.0 — so the gate's "current branch version must be greater than main" check trips. CHANGELOG.md's top entry (## [1.57.0]) only describes the original run-sampler command from #236, with no entry for what this PR actually adds (report-URL surfacing, --no-open, --compact, plus the zip-slip/timeout/collision/exit-code fixes). Needs a version bump (e.g. 1.57.1) in both package.json and package-lock.json, plus a new ## [1.57.1] CHANGELOG.md entry — or check "Skip bumping the CLI version" in the PR description if that's intentional.
…sult
readNamedResults only returned null (counted as skipped) when JSON.parse
threw or the parsed value was null/undefined. A registers.json that
parsed fine but wasn't an object (a truncated-mid-write bare string,
number, or array) fell through to {} instead, silently counting as
sampled with an empty before/after - so real keys on the other side of
the pair were reported as spurious ABSENT<->value changes rather than
the entry being flagged as unreadable.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
named_results values are template-controlled and get rendered verbatim between the fixed SAMPLER_COMPACT_START/END markers. A value whose rendered form happens to contain the literal marker text could fool a downstream naive marker-delimited extractor (the markers' stated purpose, e.g. a CI job posting this section as a PR comment) into truncating or misparsing the real section. Escape any literal occurrence of the markers within the diff body before wrapping it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
useSpinner was based on !process.env.CI, diverging from spinner.js's own !stdout.isTTY guard (spin() silently no-ops when stdout isn't a TTY). In a non-CI, non-TTY invocation (piped output, cron, nohup) this left zero polling feedback: the spinner call no-ops and the consola.info fallback never fires since useSpinner was true. Key off stdout.isTTY instead, matching spinner.js, and log a heartbeat at most once a minute during non-interactive polling so a run can't go silent for up to the full hour-long waitingLimit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Swept the remaining review findings that couldn't be threaded as inline comments (this file's earlier diffs briefly rendered as binary to GitHub, so a few landed as top-level review comments instead of resolvable threads). Verified each against current
All 9 inline review threads already show resolved. Ran the full sampler test suites locally as a final check: |
2 participants
Always log the hosted result_url on completion so it can be captured in CI (e.g. echoed into $GITHUB_STEP_SUMMARY) without downloading anything. Only download+open the report locally when not in CI; add --no-open. Use a static poll message instead of the animated spinner in CI to avoid flooding the captured log with thousands of frames.
Fixes # (link to the corresponding issue if applicable)
Description
Include a summary of the changes made
Testing Instructions
Steps:
Author Checklist
Reviewer Checklist