Expand run-sampler --compact: results register, vanished-output grouping, scope tier, visual-only tier - #265
Expand run-sampler --compact: results register, vanished-output grouping, scope tier, visual-only tier#265michieldegezelle wants to merge 11 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
|
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 compact sampler diff now includes ChangesCompact sampler diff
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 2
🤖 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 `@bin/cli.js`:
- Around line 554-556: Update the CLI flow around options.fromZip and the
-p/--partner validation so the from-zip offline path does not require a partner
id. Preserve partner validation for all non-zip execution paths, and continue
passing options.partner to LiquidSamplerRunner when available.
In `@lib/liquidSamplerCompact.js`:
- Around line 734-736: The headline construction using summary.templatesChanged
and summary.entriesChanged misrepresents runs whose findings exist only in
collapsed, scope, or visual tiers. Update the summary-count logic and the
headline generation around summary.templatesChanged, summary.entriesChanged, and
the nothingChanged guard so non-data findings are reflected in the counts, or
explicitly label the headline as data-diff-specific and suppress the
contradictory zero-count line when other tiers contain findings.
🪄 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: 51b15e33-e97d-4eea-8a7b-b93f47d281b7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.mdbin/cli.jslib/liquidSamplerCompact.jslib/liquidSamplerRunner.jspackage.jsontests/lib/liquidSamplerCompact.test.jstests/lib/liquidSamplerRunner.test.js
|
Addressed all outstanding review comments in 3aa9484:
Added/updated tests for all of the above; |
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/liquidSamplerCompact.js`:
- Around line 700-712: The exampleRef function currently validates only the URL
scheme, allowing Markdown link syntax injection through destinations containing
delimiters. Parse the URL, require an http(s) protocol, and reject or escape
Markdown-sensitive characters before interpolation; apply the same sanitizer to
any visual-only link rendering.
🪄 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: 05e1bb82-e9ba-4c30-a8c8-426dbbd6142f
📒 Files selected for processing (4)
bin/cli.jslib/liquidSamplerCompact.jstests/bin/cli.test.jstests/lib/liquidSamplerCompact.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/cli.js
- tests/lib/liquidSamplerCompact.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/lib/liquidSamplerRunner.test.js (1)
326-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover non-data diff tiers in this regression test.
This fixture only flags entry
1throughnamed_results. Add scope, vanished-output, and visual-only fixtures/assertions so a regression indiffEntryKeyscannot silently exclude those entries fromdiffs/.🤖 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/liquidSamplerRunner.test.js` around lines 326 - 351, The regression test around “adds view.html before/after…” currently covers only a named_results diff; extend its ZIP fixture with separate entries representing scope, vanished-output, and visual-only differences, then assert each flagged entry receives both before and after view.html files under diffs/. Keep an unchanged control entry and retain the assertion that original ZIP contents remain intact, updating the expected success count to include all covered entries.
🤖 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.
Nitpick comments:
In `@tests/lib/liquidSamplerRunner.test.js`:
- Around line 326-351: The regression test around “adds view.html before/after…”
currently covers only a named_results diff; extend its ZIP fixture with separate
entries representing scope, vanished-output, and visual-only differences, then
assert each flagged entry receives both before and after view.html files under
diffs/. Keep an unchanged control entry and retain the assertion that original
ZIP contents remain intact, updating the expected success count to include all
covered entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b02cfc3-f2f1-48ac-83ab-3dc25e40449d
📒 Files selected for processing (6)
bin/cli.jslib/liquidSamplerCompact.jslib/liquidSamplerRunner.jstests/bin/cli.test.jstests/lib/liquidSamplerCompact.test.jstests/lib/liquidSamplerRunner.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- bin/cli.js
- lib/liquidSamplerCompact.js
- tests/lib/liquidSamplerCompact.test.js
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
🟡 Minor — CHANGELOG.md [1.58.0] documents the --compact expansions and --from-zip, but the new --add-diffs-folder option from 07534f9 is missing. Worth a sentence on the 1.58.0 entry so the release notes match the user-facing surface (hidden command, but this repo still changelogs those).
5accd6b to
0bf8f15
Compare
BenjaminLangenakenSF
left a comment
BenjaminLangenakenSF
left a comment
There was a problem hiding this comment.
I found some issues with Claude, and asked for pre-made prompts that you can easily verify with your AI tool of choice 😉
1. Flag-array reorders render as an identical no-op line — and dedupe into each other
In lib/liquidSamplerCompact.js, formatResultsValue (line 203) reduces a flag-shaped results vector to a triggered count. So a genuine change where the same number of indicators fire but different ones do — ["1.0","0.0"] → ["0.0","1.0"] — renders as:
- `results`: `1/2 triggered` → `1/2 triggered`
diffResultsRegister correctly detects it as a change (it compares the raw JSON), but the rendered line tells a reviewer nothing, and reads like a bug in the diff tool itself.
It's worse than cosmetic: that rendered string doubles as the cross-entry dedup key in changeDedupKey (line 565). Two entries of the same template with opposite reorders collapse into one [2×] results: 1/2 triggered → 1/2 triggered line — two different real changes reported as one repeated change. Verified both by running extractCompact/formatCompact over a two-entry fixture with mirrored flag vectors.
This is exactly the failure mode the #<hash> fingerprint in renderValue was added to prevent for truncated long values (see that function's own comment about dedup keys) — the same guard is just missing on the flag-array path. Either name the flipped indices when the counts match but the vectors differ, or append a short hash of the vector the way renderValue does.
2. The --add-diffs-folder success message overstates what landed in the zip
In lib/liquidSamplerRunner.js:426, the success line is built from data.diffEntryKeys.length — every entry the compact diff flagged — not from the entries that actually got a view.html written. filesAdded is tracked correctly right above it (lines 405-412) but only used for the count of files.
On a run where 9 entries were flagged but only 2 of them had a view.html on disk, it printed:
✔ Added diffs/ folder to results.zip: 4 view.html file(s) across 9 entries.
diffs/ contained 2 entries. That's the normal case, not an edge case — the data and scope tiers flag plenty of entries whose view.html was never rendered, and a reviewer who opens diffs/ expecting 9 finds 2 and assumes the folder is broken. Count the distinct entries that contributed a file instead (track a Set alongside filesAdded), and keep the entry/entries pluralisation keyed off that number.
3. --add-diffs-folder is missing from the CHANGELOG
The 1.58.0 entry in CHANGELOG.md documents run-sampler --from-zip but says nothing about --add-diffs-folder, which arrived in the three commits after that entry was written (a78b80a, 2f2f2f4, 0bf8f15). The CHANGELOG is surfaced to users by the CLI itself (lib/cli/changelogReader.js), so a new user-facing flag that isn't in it is effectively undocumented. Add a sentence covering it — worth mentioning that it only adds before/after pairs for the entries the compact diff flagged, since that's the whole point versus extracting the full archive.
4. Two small ones
Two non-blocking notes on the new code:
-
extractNamedFieldsinlib/liquidSamplerCompact.jskeys fields bydata-nameinto aMap, so a repeateddata-name(a radio group: several<input>s sharing one name) is last-wins, and thecheckedattribute isn't read at all — onlyvalue. A change in which radio is selected therefore produces no field-level note and falls through to the generic "layout/markup changed with no anchored field explaining it" line. That fallback is honest, so this isn't a correctness bug, but if radio groups are common in the sampled templates it's a blind spot worth either fixing (readchecked, key byname + value) or naming in the function's doc comment alongside the existing note about fields without adata-name. -
--from-zip --add-diffs-folderextracts the zip and runsextractCompactover it twice — once inprintCompactDiffFromZip(line 357) and again inaddDiffsFolderToZip(line 384), each with its ownmkdtemp/rmSynccycle. Correct, just wasteful;diffEntryKeysfrom the first pass could be handed to the second.
…, radio groups - Distinguish same-triggered-count flag-array reorders by naming the flipped indices, so distinct results reorders no longer render (and cross-entry dedupe) as an identical no-op line. - Count only entries that actually got a view.html written when reporting --add-diffs-folder success, instead of every entry the compact diff flagged. - Track the checked option in a radio group by its value so a changed selection produces a field-level visual-diff note instead of falling through to the generic layout-changed fallback. - Document --add-diffs-folder in the 1.58.0 CHANGELOG entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the thorough pass, Benji — fixed 1, 2, and 3 in b148ffa, and took your suggestion for 4.1 (the radio-group blind spot). Left 4.2 (double
4.1. Radio-group blind spot — fixed. 4.2. Double extraction — skipping for now, per above. |
There was a problem hiding this comment.
Small pushback:
1.. The mirrored-reorder collapse survives. flipped is the set of differing indices, which is the same computed from either direction, so both sides render 1/2 triggered (indices 0,1 flipped) — identical strings, and changeDedupKey derives from them. Two entries with opposite reorders still dedupe into one [2×] line. The new test asserts the different-index-set case, which is why it passes. Naming which indices went 0→1 vs 1→0 fixes the rendered line and the key together.
Also: the suffix is uncapped, and the flag-array branch never reaches renderValue, so neither MAX_VALUE_CHARS nor the # guard applies. A long vector with many flips renders a 200+ char line.
Human readable 😅:
The fix that works: say which way each switch went — "0 turned off, 1 turned on" — so the two directions print differently and stop being mistaken for each other.
4.1. /\bchecked\b/ matches aria-checked and data-checked — \b matches after a hyphen. Once every radio in a group looks checked, last-wins returns and the selection change is invisible again. Anchor on preceding whitespace. Same weakness in getAttr(m[1], "type") vs data-type="radio". I couldn't tell from the repo whether the sampled markup emits aria-checked, so this may be latent rather than live.
Human readable 😅:
If every button in a group carries aria-checked, they all look selected, the code falls back to just keeping the last one, and a changed selection goes unnoticed — back to the original bug the fix was meant to solve.
The fix is to only accept checked when there's a space in front of it, not a hyphen.
Everything else ok 😉
|
Good pushback, both landed — fixed in 4598087.
664/664 tests pass, lint clean. |
BenjaminLangenakenSF
left a comment
BenjaminLangenakenSF
left a comment
There was a problem hiding this comment.
Approved!
Two non-blocking nitpicks:
- The first has a suggestion you can apply directly
- Test file, expect(diffA.before.length).toBeLessThan(100): Only bites if you apply the suggestion above: once before drops the suffix this just measures "10/20 triggered" and stops testing the cap. diffA.after keeps it meaningful.
|
Applied both — fixed in 8b05852. Dropped the suffix from |
…ing, scope tier, visual-only tier - Diff the `results` register alongside `named_results` (0/1 flag arrays render as a triggered-indicator count, other shapes fall back to a plain value diff). - Group entries whose named_results/results collapsed entirely (>=3 keys lost at once) into a single "output vanished" finding per template, instead of one line per lost key. - Add a scope/dependency tier (`dependencies`/`rollforward_params`/ `required_keys_missing`), separate from the data diff, with `dependencies` rendered as one sub-line per category rather than a semicolon-packed line. - Add a visual-only tier: entries where `view.html` changed but the data diff found nothing, described field-by-field via Silverfin's `data-name` anchor where possible, with an honest fallback note otherwise. - Truncate long values and cap per-template/per-entry change lists, always disclosing what was elided. - Every finding now links to a concrete follow-up: the entry's live app URL when known, else its path inside the results directory. - Add `run-sampler --from-zip <path>` to build the compact diff from an already-downloaded results.zip, with no sampler run or network call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Make -p/--partner optional for --from-zip, which never touches the partner/sampler API; still required (with a clear error) for every other run-sampler path. - Fix the compact-diff headline misreporting '0 template(s) changed' when findings exist only in the collapsed/scope/visual tiers - it's now worded as data-diff (named_results/results) specific and no longer contradicts the sections printed below it. - Guard dependencyLedgers/diffStringSet against malformed (non-array) dependencies.ledgers/account_ranges register values so one bad entry can't throw and abort the whole run's diff. - Validate exampleRef's url as http(s) before interpolating it into Markdown link syntax, since with --from-zip that url no longer necessarily comes from Silverfin's own sampler backend. - Decode HTML entities in <input> field values, consistent with the <textarea>/<select> branches. - Distinguish an explicit results: null from a genuinely absent register in formatResultsValue/diffResultsRegister, so a legitimate null doesn't get miscounted into the 'output vanished' heuristic. - Give truncated long values a short content hash alongside the length, so two different values that share the same truncated prefix and total length no longer collide under the same cross-entry dedup key. 🤖 Generated with Claude Code Co-authored-by: Cursor <cursoragent@cursor.com>
CodeRabbit follow-up: an http(s) scheme check alone isn't enough - a value like 'https://trusted.example/a) [injected](https://attacker)' still passes it but closes the generated (...) link early and injects arbitrary Markdown. Add a shared sanitizeUrl() that requires a parseable http(s) URL and rejects parentheses/whitespace/angle brackets/control characters outright, and use it for both exampleRef and the visual-only section's 'open in app' link (which previously interpolated entry.url with no validation at all). 🤖 Generated with Claude Code Co-authored-by: Cursor <cursoragent@cursor.com>
Reviewers currently have to dig through the full results.zip (often ~150 MB)
to find the handful of entries the compact diff actually flagged. extractCompact
now returns diffEntryKeys - every entry mentioned across the data/scope/
vanished-output/visual-only tiers - and run-sampler --from-zip --add-diffs-folder
uses it to add a diffs/<kind>/<entryId>/{before,after}/view.html folder to an
existing results.zip, in place, with no extra network call.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zip.writeZip(zipPath) truncated and rewrote the input zip in place with no temp file or rename, so a crash mid-write (OOM, timeout, kill) could leave a corrupted results.zip that CI would still upload as the reviewer-facing artifact. Flagged independently across nl_market#906, lu_market#774, and bso_github_actions#35's reviews of the CI workflows that call this path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
addDiffsFolderToZip still rewrote the zip and reported success even when every flagged entry lacked a view.html (filesAdded stays 0), matching the existing empty-diffEntryKeys early return instead. Also trims the CHANGELOG entry for 1.58.0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Large sampler runs (many templates x many firms) can legitimately take longer than an hour on the backend; the CLI's client-side timeout was giving up and reporting failure while the run was still in progress. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…, radio groups - Distinguish same-triggered-count flag-array reorders by naming the flipped indices, so distinct results reorders no longer render (and cross-entry dedupe) as an identical no-op line. - Count only entries that actually got a view.html written when reporting --add-diffs-folder success, instead of every entry the compact diff flagged. - Track the checked option in a radio group by its value so a changed selection produces a field-level visual-diff note instead of falling through to the generic layout-changed fallback. - Document --add-diffs-folder in the 1.58.0 CHANGELOG entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Name the direction of each flag-array flip (on/off), not just its index. Two entries with opposite-direction reorders touching the same indices previously rendered identically and still collapsed into one deduped change - naming direction fixes the rendered line and the dedup key together. Cap the detail list and fingerprint the elided flips (like renderValue's truncation) so two long vectors can't collapse either. - Anchor getAttr and the radio checked-attribute check on a preceding whitespace instead of \b, which also matches after a hyphen - "type" was matching inside data-type="radio", and "checked" was matching inside aria-checked/data-checked, making every radio in a group look checked and silently reverting to last-wins. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Putting the direction suffix on both before/after still rendered the
line as an identical no-op string on either side, despite naming the
flip direction in the text. Keeping it on after only ("N/M triggered"
-> "N/M triggered (0 off, 1 on)") makes the change visible while the
mirrored dedup keys stay distinct.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
--add-diffs-folder copied before/after view.html for every entry the compact diff flagged, including entries flagged for something the RENDER doesn't show (a named_results/results value, a dependencies/scope change). Those pairs are byte-identical, so the reviewer opens diffs/ and finds two files with nothing to compare. Seen live on lu_market run 30920292551: all 3 flagged entries were timestamp-only named_results changes, so the entire diffs/ folder was 6 identical files — the one thing the folder exists to avoid. Only add a pair when the two renders actually differ, and say how many entries were skipped for being identical (in the success line, or as the reason nothing was added) so an absent diffs/ folder is never ambiguous between "nothing rendered differently" and "no render was captured". Verified against that run's real results.zip: before, 6 files across 3 entries; after, no diffs/ folder and an explicit message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1ba5a0f to
813d64f
Compare
2 participants
Summary
DO NOT MERGE UNTIL NL AND LU PILOT PHASE HAS FINISHED
Follow-up to #236/#261's
--compactmode, driven by real feedback while reviewing PR #894's sampler run against nl_market: thenamed_results-only diff missed real signal and got unreadable fast (78KB/~19.7K tokens on a 202-entry NL run). This PR:resultsregister alongsidenamed_results- 0/1 flag arrays render as a triggered-indicator count (e.g.1/1 triggered), other shapes (raw numeric values) fall back to a plain value diff.named_results/resultscollapsed entirely (≥3 keys lost at once, all toundefined) into a single "output vanished" finding per template - previously this showed as dozens of individual "value → undefined" lines and could dominate the whole summary.dependencies/rollforward_params/required_keys_missing), kept separate from the data diff so a dependency change is never mistaken for a data regression.dependenciesrenders as one sub-line per category (ledgers/handles/account ranges/company.attributes) instead of a semicolon-packed single line.view.htmlchanged but the data diff found nothing to explain it - a rendering-only regression the old diff couldn't see at all. Described field-by-field via Silverfin'sdata-nameattribute where one anchors the change, with an honest "compare the files directly" fallback where it doesn't.run-sampler --from-zip <path>: build the compact diff from an already-downloadedresults.zip, with no sampler run or network call - lets a reviewer re-analyze a real result (or iterate on the compact-diff format itself) without a 30-60 min re-run against the shared staging backend.Net effect measured on the real PR #894 zip (202 entries, 6 templates changed + 21 collapsed + 9 visual-only): output went from 78KB/~19.7K tokens (old format) to ~31.7KB while covering strictly more signal.
Why keeping this open for now
This is currently pointed at from nl_market's and lu_market's
run_sampler.yml(temporarily, on this branch) so we can gather real feedback across their approved PRs before merging - see the follow-up in each of those repos.Test plan
npm test- 640/640 passingnpm run lint- cleanValidated against a real downloaded
results.zipfrom nl_market PR #894 via--from-zip, output reviewed in fullReal end-to-end validation via a live download URL (only the local-zip path has been exercised against real data so far; the live-download path is covered by mocked tests)
Feedback gathered from nl_market/lu_market usage before merge
Skip bumping the CLI version
🤖 Generated with Claude Code