Expand run-sampler --compact: results register, vanished-output grouping, scope tier, visual-only tier#265
Open
michieldegezelle wants to merge 3 commits into
Open
Expand run-sampler --compact: results register, vanished-output grouping, scope tier, visual-only tier#265michieldegezelle wants to merge 3 commits into
michieldegezelle wants to merge 3 commits into
Conversation
…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>
![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.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe compact sampler diff now includes ChangesCompact sampler diff
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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
- 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>
Contributor
Author
|
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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