From fad591d8c2cef759af5c82a74eb2a32f743fa410 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Wed, 22 Jul 2026 16:28:30 +0200 Subject: [PATCH 1/3] Expand run-sampler --compact: results register, vanished-output grouping, 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 ` to build the compact diff from an already-downloaded results.zip, with no sampler run or network call. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 3 + bin/cli.js | 19 +- lib/liquidSamplerCompact.js | 691 ++++++++++++++++++++++--- lib/liquidSamplerRunner.js | 77 ++- package-lock.json | 4 +- package.json | 2 +- tests/lib/liquidSamplerCompact.test.js | 503 +++++++++++++++++- tests/lib/liquidSamplerRunner.test.js | 63 +++ 8 files changed, 1252 insertions(+), 110 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a20a13..a399887 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ All notable changes to this project will be documented in this file. +## [1.58.0] (22/07/2026) +Expand `silverfin run-sampler --compact`: diff the `results` register alongside `named_results` (shown as a triggered-indicator count for 0/1 flag arrays), group entries whose rendered output vanished entirely into a single "output vanished" finding per template instead of dozens of individual lines, add a `dependencies`/`rollforward_params`/`required_keys_missing` scope-change tier (rendered as one sub-line per category, not a semicolon-packed dump), and add a visual-only tier that flags `view.html` changes invisible to the data diff - described field-by-field where a `data-name` anchors the change, with a plain "compare the files" fallback otherwise. Long values are now truncated and per-template/per-entry change lists are capped, both with an explicit "+N more" disclosure. Also adds `run-sampler --from-zip ` to build the compact diff from an already-downloaded `results.zip` with no sampler run or network call. + ## [1.57.1] (15/07/2026) Improve `silverfin run-sampler` by adding a compact output mode. diff --git a/bin/cli.js b/bin/cli.js index cdcc004..c76b733 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -530,15 +530,32 @@ program "accountTemplate", "sharedPart", "firmIds", + "fromZip", + ]) + ) + .addOption( + new Option("--from-zip ", "Build the compact diff from an already-downloaded results.zip - no sampler run, no network call").conflicts([ + "handle", + "accountTemplate", + "sharedPart", + "firmIds", + "id", ]) ) .option("--no-open", "Do not download/open the report locally; only print its URL (default in CI)") - .option("--compact", "Download the result and print a compact named_results diff (grouped by template) to stdout - review-friendly and safe in CI") + .option("--compact", "Download the result and print a compact diff (named_results/results, dependencies, vanished renders, visual-only changes) grouped by template - review-friendly and safe in CI") .action(async (options) => { // Commander sets options.open = false when --no-open is passed. // In CI, never open regardless of the flag. const runnerOptions = { openReport: options.open && !process.env.CI, compact: options.compact || false }; + // A local zip needs no partner API access at all - it's pure offline + // re-analysis of a result someone already has on disk. + if (options.fromZip) { + await new LiquidSamplerRunner(options.partner, runnerOptions).printCompactDiffFromZip(options.fromZip); + return; + } + // If an existing sampler ID is provided, fetch and display results if (options.id) { if (!/^\d+$/.test(options.id)) { diff --git a/lib/liquidSamplerCompact.js b/lib/liquidSamplerCompact.js index f9c9cd8..50e621c 100644 --- a/lib/liquidSamplerCompact.js +++ b/lib/liquidSamplerCompact.js @@ -5,20 +5,28 @@ const YAML = require("yaml"); /** * Extract a compact, LLM/review-friendly view of a Liquid Sampler result. * - * A sampler `results.zip` is huge (the rendered HTML report alone is ~470K tokens - * at scale, and the raw per-entry output is ~150 MB). The only signal a reviewer - * usually needs first is the DATA diff: how each template's `named_results` - * changed. That lives in `registers.json` for every sampled entry, before/after. + * A sampler `results.zip` is huge (the rendered HTML report alone can be tens of + * MB, and the raw per-entry output is ~150 MB at scale). This module builds + * several small, targeted signals instead of a raw diff: * - * This module reads an already-EXTRACTED results directory (the layout inside - * results.zip) and returns just the `named_results` changes, grouped by template - * and deduped by identical change, so the whole thing is ~900 tokens instead of - * hundreds of thousands. It never touches the rendered HTML. + * - `templates` - the `named_results`/`results` DATA diff, deduped by + * identical change and capped so one broken/verbose + * template can't blow up the whole summary. + * - `collapsedEntries` - entries whose rendered output vanished entirely + * between phases (a strong broken-template signal that + * would otherwise show up as dozens of individual + * "value -> undefined" lines). + * - `scopeTemplates` - `dependencies` / `rollforward_params` / + * `required_keys_missing` changes: what the template + * depends on, not what it renders. + * - `visualOnlyEntries` - entries where `view.html` changed but the DATA + * (named_results/results) didn't - a rendering-only + * regression the data diff can't see. * * Expected directory layout (inside results.zip): * /sample_entry_ids.yml - * /output/account_entries//{before,after}/registers.json - * /output/reconciliation_entries//{before,after}/registers.json + * /output/account_entries//{before,after}/{registers.json,view.html} + * /output/reconciliation_entries//{before,after}/{registers.json,view.html} */ const ENTRY_KINDS = ["account_entries", "reconciliation_entries"]; @@ -27,15 +35,35 @@ const ENTRY_KINDS = ["account_entries", "reconciliation_entries"]; // that distinctly from an explicit JSON `null`. const ABSENT = Symbol("absent"); +// Individual before/after values longer than this are elided - long free-text +// fields (accounting-policy paragraphs, notes) are the single biggest driver of +// oversized summaries, and the full text rarely helps a reviewer decide +// "is this intended?" faster than a preview does. +const MAX_VALUE_CHARS = 100; +// Detail lines shown per template before collapsing the rest into "+N more". +const MAX_CHANGES_SHOWN = 8; +// Named-results/results keys that flip from a value to `undefined` in a single +// entry, at or above this count, are treated as "the render broke" rather than +// as that many independent findings. +const COLLAPSE_MIN_LOST_KEYS = 3; +// Items shown before eliding a set-diff (dependencies/rollforward params/etc.) +const MAX_SET_ITEMS_SHOWN = 3; +// Field-level notes shown per visual-only entry before eliding the rest. +const MAX_VISUAL_CHANGES_SHOWN = 6; + /** * Render a named_results value for display. Distinguishes an absent key - * (template no longer emits it) from an explicit null. + * (template no longer emits it) from an explicit null, and elides long values. * @param {*} value * @returns {string} */ function renderValue(value) { if (value === ABSENT) return "undefined"; - return JSON.stringify(value); + const rendered = JSON.stringify(value); + if (rendered.length > MAX_VALUE_CHARS) { + return `${rendered.slice(0, MAX_VALUE_CHARS)}…(${rendered.length} chars)`; + } + return rendered; } /** @@ -72,25 +100,32 @@ function readEntryLabels(resultsDir) { } /** - * Read the named_results object from a registers.json file. + * Read and validate a registers.json file. * @param {string} filePath - * @returns {Object|null} the named_results object, or null if unreadable + * @returns {Object|null} the parsed registers object, or null if unreadable */ -function readNamedResults(filePath) { +function readRegisters(filePath) { try { const registers = JSON.parse(fs.readFileSync(filePath, "utf8")); // A valid registers.json is always an object. Anything else (a bare // string/number, an array, or a truncated-to-null file) means the file - // is unreadable, not that named_results happens to be absent. + // is unreadable, not that its contents happen to be absent. if (!registers || typeof registers !== "object" || Array.isArray(registers)) return null; - - const named = registers.named_results; - return named && typeof named === "object" ? named : {}; + return registers; } catch { return null; } } +/** + * @param {Object} registers - a parsed registers.json + * @returns {Object} the named_results object, defaulting to {} when absent + */ +function namedResultsOf(registers) { + const named = registers.named_results; + return named && typeof named === "object" ? named : {}; +} + /** * Serialize a value such that two objects with the same keys/values but a * different property insertion order produce the same string. Array element @@ -131,22 +166,410 @@ function diffNamedResults(before, after) { } /** - * Walk an extracted results directory and build the compact named_results diff. + * Whether a `results` array is a plain 0/1 flag vector (the common shape for + * reconciliation-check indicators) rather than raw numeric values. + * @param {Array} arr + * @returns {boolean} + */ +function isFlagArray(arr) { + return Array.isArray(arr) && arr.length > 0 && arr.every((v) => v === "0.0" || v === "1.0" || v === "0" || v === "1"); +} + +/** + * Format a `results` register value for display. Flag-shaped arrays (every + * element 0/1) render as a triggered-count, since that's the reviewable + * signal (e.g. "how many unreconciled indicators fired"); anything else + * (raw numeric values from other template kinds) falls back to the plain + * rendered value. + * @param {*} value + * @returns {string} + */ +function formatResultsValue(value) { + if (value == null) return "undefined"; + if (isFlagArray(value)) { + const triggered = value.filter((v) => v === "1.0" || v === "1").length; + return `${triggered}/${value.length} triggered`; + } + return renderValue(value); +} + +/** + * Diff the `results` register (Liquid's unnamed results array) between + * phases. Unlike named_results this is a single un-keyed value per entry, so + * it's either unchanged or it's one change - never one per array element. + * @param {*} before + * @param {*} after + * @returns {{before: string, after: string}|null} + */ +function diffResultsRegister(before, after) { + const b = before == null ? null : before; + const a = after == null ? null : after; + if (JSON.stringify(b) === JSON.stringify(a)) return null; + return { before: formatResultsValue(before), after: formatResultsValue(after) }; +} + +/** + * Set-diff two arrays of strings. + * @param {Array} beforeArr + * @param {Array} afterArr + * @returns {{added: Array, removed: Array}|null} null if identical + */ +function diffStringSet(beforeArr, afterArr) { + const before = new Set(beforeArr || []); + const after = new Set(afterArr || []); + const added = [...after].filter((x) => !before.has(x)).sort(); + const removed = [...before].filter((x) => !after.has(x)).sort(); + if (added.length === 0 && removed.length === 0) return null; + return { added, removed }; +} + +/** + * @param {Array} items + * @returns {string} up to MAX_SET_ITEMS_SHOWN items, then "+N more" + */ +function previewList(items, max = MAX_SET_ITEMS_SHOWN) { + if (items.length <= max) return items.join(", "); + return `${items.slice(0, max).join(", ")} +${items.length - max} more`; +} + +/** + * @param {number} n + * @param {string} unit - singular form, e.g. "handle" + * @returns {string} + */ +function pluralize(n, unit) { + return `${unit}${n === 1 ? "" : "s"}`; +} + +/** + * Render an added/removed set-diff as a compact one-line summary, e.g. + * "−1 ledger (10028311), +2 handles (overview_notes, office_info)". + * @param {Array} added + * @param {Array} removed + * @param {string} unit + * @returns {string} + */ +function formatSetSummary(added, removed, unit) { + const parts = []; + if (removed.length) parts.push(`−${removed.length} ${pluralize(removed.length, unit)} (${previewList(removed)})`); + if (added.length) parts.push(`+${added.length} ${pluralize(added.length, unit)} (${previewList(added)})`); + return parts.join(", "); +} + +/** + * @param {Object} deps - a `dependencies` register value + * @returns {Object>} handle names keyed by ledger id + */ +function dependencyHandlesPerLedger(deps) { + return (deps && deps.reconciliations && deps.reconciliations.handles_per_ledger) || {}; +} + +/** + * @param {Object} deps - a `dependencies` register value + * @returns {Array} every ledger id the template touches, from either + * the flat `ledgers` list or the handles-per-ledger map + */ +function dependencyLedgers(deps) { + if (!deps) return []; + const handlesPerLedger = dependencyHandlesPerLedger(deps); + return [...new Set([...(deps.ledgers || []).map(String), ...Object.keys(handlesPerLedger)])]; +} + +/** + * @param {Object} deps - a `dependencies` register value + * @returns {Array} every distinct handle name depended on, across all ledgers + */ +function dependencyHandles(deps) { + if (!deps) return []; + return [...new Set(Object.values(dependencyHandlesPerLedger(deps)).flat())]; +} + +/** + * Diff a template's `dependencies` register (which ledgers/handles/account + * ranges/company attributes it reads) - its SCOPE, not its rendered data. + * Returned as one line per category (rather than one joined string) since a + * template can touch all four at once and a semicolon-packed single line + * gets unreadable fast. + * @param {Object} before + * @param {Object} after + * @returns {Array|null} one summary line per changed category, or null if unchanged + */ +function diffDependencies(before, after) { + const parts = []; + + const ledgerDiff = diffStringSet(dependencyLedgers(before), dependencyLedgers(after)); + if (ledgerDiff) parts.push(formatSetSummary(ledgerDiff.added, ledgerDiff.removed, "ledger")); + + const handleDiff = diffStringSet(dependencyHandles(before), dependencyHandles(after)); + if (handleDiff) parts.push(formatSetSummary(handleDiff.added, handleDiff.removed, "handle")); + + const rangeDiff = diffStringSet((before && before.account_ranges) || [], (after && after.account_ranges) || []); + if (rangeDiff) parts.push(formatSetSummary(rangeDiff.added, rangeDiff.removed, "account range")); + + const beforeAttr = !!(before && before.company && before.company.attributes); + const afterAttr = !!(after && after.company && after.company.attributes); + if (beforeAttr !== afterAttr) parts.push(`company.attributes: ${beforeAttr} → ${afterAttr}`); + + return parts.length ? parts : null; +} + +/** + * Diff a template's `rollforward_params` register by declared param name + * (the `value` is usually null/a placeholder, so the name is the signal). + * @param {Array} before + * @param {Array} after + * @returns {string|null} + */ +function diffRollforwardParams(before, after) { + const names = (list) => (Array.isArray(list) ? list : []).map((p) => p && p.name).filter(Boolean); + const diff = diffStringSet(names(before), names(after)); + if (!diff) return null; + return formatSetSummary(diff.added, diff.removed, "param"); +} + +/** + * Diff a template's `required_keys_missing` register. + * @param {Array} before + * @param {Array} after + * @returns {string|null} + */ +function diffRequiredKeysMissing(before, after) { + const diff = diffStringSet(before, after); + if (!diff) return null; + return formatSetSummary(diff.added, diff.removed, "key"); +} + +/** + * Diff the "scope" registers (what the template depends on / requires), as + * distinct from the "data" registers (what it renders). Kept as its own tier + * so a dependency change never gets mistaken for a data regression. + * @param {Object} before - a parsed registers.json + * @param {Object} after - a parsed registers.json + * @returns {Array<{key: string, summary?: string, subLines?: Array}>} + */ +function diffScope(before, after) { + const changes = []; + const dependencies = diffDependencies(before.dependencies, after.dependencies); + if (dependencies) changes.push({ key: "dependencies", subLines: dependencies }); + + const rollforwardParams = diffRollforwardParams(before.rollforward_params, after.rollforward_params); + if (rollforwardParams) changes.push({ key: "rollforward_params", summary: rollforwardParams }); + + const requiredKeysMissing = diffRequiredKeysMissing(before.required_keys_missing, after.required_keys_missing); + if (requiredKeysMissing) changes.push({ key: "required_keys_missing", summary: requiredKeysMissing }); + + return changes; +} + +/** + * @param {string} resultsDir + * @param {string} kind + * @param {string} entryId + * @param {"before"|"after"} phase + * @returns {string} path to that entry's view.html for the given phase + */ +function viewHtmlPath(resultsDir, kind, entryId, phase) { + return path.join(resultsDir, "output", kind, entryId, phase, "view.html"); +} + +const HTML_ENTITIES = { " ": " ", "&": "&", "<": "<", ">": ">", """: '"', "'": "'" }; + +/** + * Minimal entity decoding - just the handful that show up in rendered form markup. + * @param {string} text + * @returns {string} + */ +function decodeEntities(text) { + return text.replace(/ |&|<|>|"|'/g, (m) => HTML_ENTITIES[m]); +} + +/** + * @param {string} attrs - a tag's raw attribute string + * @param {string} name + * @returns {string|null} the attribute's value, or null if absent + */ +function getAttr(attrs, name) { + const match = attrs.match(new RegExp(`\\b${name}="([^"]*)"`)); + return match ? match[1] : null; +} + +/** + * Extract every form field in a rendered view.html that's anchored by + * `data-name` (Silverfin's stable per-field identifier), keyed by that name. + * Fields without a `data-name` (static markup, wrapper divs) aren't + * individually addressable, so they fall outside this map entirely - a + * change confined to those falls back to the generic note in + * describeVisualChange. + * @param {string} html + * @returns {Map} + */ +function extractNamedFields(html) { + const fields = new Map(); + + for (const m of html.matchAll(/]*)>([\s\S]*?)<\/textarea>/g)) { + const name = getAttr(m[1], "data-name"); + if (!name) continue; + fields.set(name, { tag: "textarea", value: decodeEntities(m[2].trim()), placeholder: getAttr(m[1], "placeholder") }); + } + for (const m of html.matchAll(/]*?)\/?>/g)) { + const name = getAttr(m[1], "data-name"); + if (!name) continue; + fields.set(name, { tag: "input", value: getAttr(m[1], "value"), placeholder: getAttr(m[1], "placeholder") }); + } + for (const m of html.matchAll(/]*)>([\s\S]*?)<\/select>/g)) { + const name = getAttr(m[1], "data-name"); + if (!name) continue; + const selected = m[2].match(/]*\bselected\b[^>]*>([^<]*)} + */ +function describeVisualChange(beforeHtml, afterHtml) { + const before = extractNamedFields(beforeHtml); + const after = extractNamedFields(afterHtml); + const notes = []; + + for (const name of [...new Set([...before.keys(), ...after.keys()])].sort()) { + const b = before.get(name); + const a = after.get(name); + if (b && !a) { + notes.push(`field \`${name}\` removed`); + continue; + } + if (!b && a) { + notes.push(`field \`${name}\` added`); + continue; + } + if (b.value !== a.value) { + notes.push(`field \`${name}\` value: ${renderValue(b.value)} → ${renderValue(a.value)}`); + } + if ((b.placeholder || "") !== (a.placeholder || "")) { + notes.push(`field \`${name}\` placeholder: ${renderValue(b.placeholder || "")} → ${renderValue(a.placeholder || "")}`); + } + } + + if (notes.length === 0) { + notes.push("layout/markup changed with no anchored field explaining it - compare the two view.html files directly"); + } + return notes; +} + +/** + * Compare an entry's rendered view.html between phases and describe what + * changed. Returns null when either file is missing (view.html wasn't + * extracted, or this run predates it) or when they're identical, so callers + * can skip the tier entirely rather than false-flagging. + * @param {string} resultsDir + * @param {string} kind + * @param {string} entryId + * @returns {{changes: Array}|null} + */ +function describeViewHtmlDiff(resultsDir, kind, entryId) { + const beforePath = viewHtmlPath(resultsDir, kind, entryId, "before"); + const afterPath = viewHtmlPath(resultsDir, kind, entryId, "after"); + if (!fs.existsSync(beforePath) || !fs.existsSync(afterPath)) return null; + const beforeHtml = fs.readFileSync(beforePath, "utf8"); + const afterHtml = fs.readFileSync(afterPath, "utf8"); + if (beforeHtml === afterHtml) return null; + return { changes: describeVisualChange(beforeHtml, afterHtml) }; +} + +/** + * Add a set of changes to a per-template bucket, deduping identical changes + * across entries (with a count) and tracking one example entry to link back + * to for a reviewer. + * @param {Map} byTemplate + * @param {string} kind + * @param {string} label + * @param {string} entryKey - "kind/entryId" + * @param {Array<{key: string, before?: string, after?: string, summary?: string, subLines?: Array}>} changes + * @param {string|null} url + */ +function addChangesToTemplate(byTemplate, kind, label, entryKey, changes, url) { + const templateKey = JSON.stringify([kind, label]); + if (!byTemplate.has(templateKey)) { + byTemplate.set(templateKey, { + label, + entryIds: new Set(), + changeCounts: new Map(), + exampleUrl: url, + exampleEntryKey: entryKey, + }); + } + const bucket = byTemplate.get(templateKey); + bucket.entryIds.add(entryKey); + for (const change of changes) { + const dedupKey = changeDedupKey(change); + const existing = bucket.changeCounts.get(dedupKey); + if (existing) { + existing.count += 1; + } else { + bucket.changeCounts.set(dedupKey, { ...change, count: 1 }); + } + } +} + +/** + * A stable string key identifying a change's shape, for cross-entry dedup. + * @param {{key: string, before?: string, after?: string, summary?: string, subLines?: Array}} change + * @returns {string} + */ +function changeDedupKey(change) { + if (change.subLines !== undefined) return `${change.key}\x00${change.subLines.join("\x01")}`; + if (change.summary !== undefined) return `${change.key}\x00${change.summary}`; + return `${change.key}\x00${change.before}\x00${change.after}`; +} + +/** + * Convert a byTemplate bucket map into a sorted, plain-object array. + * @param {Map} byTemplate + * @returns {Array<{label: string, entriesChanged: number, exampleUrl: string|null, exampleEntryKey: string, changes: Array}>} + */ +function toTemplatesArray(byTemplate) { + const templates = [...byTemplate.entries()] + .map(([, bucket]) => ({ + label: bucket.label, + entriesChanged: bucket.entryIds.size, + exampleUrl: bucket.exampleUrl, + exampleEntryKey: bucket.exampleEntryKey, + // Most-repeated change first, then alphabetically by key for stability. + changes: [...bucket.changeCounts.values()].sort((x, y) => y.count - x.count || x.key.localeCompare(y.key)), + })) + .sort((x, y) => y.entriesChanged - x.entriesChanged || x.label.localeCompare(y.label)); + + return templates; +} + +/** + * Walk an extracted results directory and build every compact-diff tier. * @param {string} resultsDir - Path to the extracted results directory * @returns {{ - * summary: {templatesChanged: number, entriesChanged: number, entriesSampled: number, entriesSkipped: number}, - * templates: Array<{label: string, entriesChanged: number, changes: Array<{key: string, before: string, after: string, count: number}>}> + * summary: {templatesChanged: number, entriesChanged: number, entriesSampled: number, entriesSkipped: number, collapsedCount: number, visualOnlyCount: number}, + * templates: Array, + * scopeTemplates: Array, + * collapsedTemplates: Array, + * visualOnlyEntries: Array<{kind: string, entryId: string, label: string, url: string|null, changes: Array}> * }} */ function extractCompact(resultsDir) { const labelMap = readEntryLabels(resultsDir); const outputDir = path.join(resultsDir, "output"); - // JSON.stringify([kind, label]) -> { label, entryIds: Set, changeCounts: Map<"key\0before\0after", {key,before,after,count}> } - // Keyed by kind as well as label because the same label string could - // (however unlikely) apply to both an account and a reconciliation - // template - merging those would combine unrelated entries/changes. const byTemplate = new Map(); + const scopeByTemplate = new Map(); + const collapsedByTemplate = new Map(); + const visualOnlyEntries = []; let entriesSampled = 0; let entriesSkipped = 0; @@ -158,9 +581,9 @@ function extractCompact(resultsDir) { const entryDir = path.join(kindDir, entryId); if (!fs.statSync(entryDir).isDirectory()) continue; - const before = readNamedResults(path.join(entryDir, "before", "registers.json")); - const after = readNamedResults(path.join(entryDir, "after", "registers.json")); - if (before === null || after === null) { + const beforeRegisters = readRegisters(path.join(entryDir, "before", "registers.json")); + const afterRegisters = readRegisters(path.join(entryDir, "after", "registers.json")); + if (beforeRegisters === null || afterRegisters === null) { // registers.json missing/malformed - this entry was never actually // compared, so it shouldn't count toward "sampled". entriesSkipped += 1; @@ -168,41 +591,54 @@ function extractCompact(resultsDir) { } entriesSampled += 1; - const changes = diffNamedResults(before, after); - if (changes.length === 0) continue; - const entryKey = `${kind}/${entryId}`; const label = (labelMap[entryKey] && labelMap[entryKey].label) || entryId; - const templateKey = JSON.stringify([kind, label]); - if (!byTemplate.has(templateKey)) { - byTemplate.set(templateKey, { label, entryIds: new Set(), changeCounts: new Map() }); + const url = (labelMap[entryKey] && labelMap[entryKey].url) || null; + + const beforeNamed = namedResultsOf(beforeRegisters); + const afterNamed = namedResultsOf(afterRegisters); + const namedChanges = diffNamedResults(beforeNamed, afterNamed); + + const resultsChange = diffResultsRegister(beforeRegisters.results, afterRegisters.results); + const dataChanges = resultsChange ? [...namedChanges, { key: "results", ...resultsChange }] : namedChanges; + + // A template that broke mid-render loses many named_results/results + // keys at once - that's ONE finding ("output vanished"), not N. + const vanishedCount = dataChanges.filter((c) => c.after === "undefined").length; + const hadContentBefore = Object.keys(beforeNamed).length > 0 || beforeRegisters.results != null; + const isCollapse = hadContentBefore && vanishedCount === dataChanges.length && vanishedCount >= COLLAPSE_MIN_LOST_KEYS; + + if (isCollapse) { + // Grouped and deduped the same way as data/scope changes, so a + // template that broke across dozens of sampled companies collapses + // into one section entry instead of one line per entry. + addChangesToTemplate(collapsedByTemplate, kind, label, entryKey, [{ key: "output vanished", summary: `${vanishedCount} value(s) lost` }], url); + } else if (dataChanges.length > 0) { + addChangesToTemplate(byTemplate, kind, label, entryKey, dataChanges, url); } - const bucket = byTemplate.get(templateKey); - bucket.entryIds.add(entryKey); - for (const change of changes) { - const dedupKey = `${change.key}\x00${change.before}\x00${change.after}`; - const existing = bucket.changeCounts.get(dedupKey); - if (existing) { - existing.count += 1; - } else { - bucket.changeCounts.set(dedupKey, { ...change, count: 1 }); + + const scopeChanges = diffScope(beforeRegisters, afterRegisters); + if (scopeChanges.length > 0) { + addChangesToTemplate(scopeByTemplate, kind, label, entryKey, scopeChanges, url); + } + + // The visual-only tier only adds information when the data tier found + // nothing to explain a view.html change - otherwise it's just a noisier + // restatement of a finding already surfaced above. + if (!isCollapse && dataChanges.length === 0) { + const visualDiff = describeViewHtmlDiff(resultsDir, kind, entryId); + if (visualDiff) { + visualOnlyEntries.push({ kind, entryId, label, url, changes: visualDiff.changes }); } } } } - const templates = [...byTemplate.entries()] - .map(([, bucket]) => ({ - label: bucket.label, - entriesChanged: bucket.entryIds.size, - // Most-repeated change first, then alphabetically by key for stability. - changes: [...bucket.changeCounts.values()].sort( - (x, y) => y.count - x.count || x.key.localeCompare(y.key), - ), - })) - .sort((x, y) => y.entriesChanged - x.entriesChanged || x.label.localeCompare(y.label)); - + const templates = toTemplatesArray(byTemplate); + const scopeTemplates = toTemplatesArray(scopeByTemplate); + const collapsedTemplates = toTemplatesArray(collapsedByTemplate); const entriesChanged = templates.reduce((sum, t) => sum + t.entriesChanged, 0); + const collapsedCount = collapsedTemplates.reduce((sum, t) => sum + t.entriesChanged, 0); return { summary: { @@ -210,49 +646,154 @@ function extractCompact(resultsDir) { entriesChanged, entriesSampled, entriesSkipped, + collapsedCount, + visualOnlyCount: visualOnlyEntries.length, }, templates, + scopeTemplates, + collapsedTemplates, + visualOnlyEntries, }; } /** - * Format the compact diff as Markdown, suitable for stdout or a PR comment. + * A markdown link to an example entry: the live app URL when known, else a + * relative path into the results directory so a reviewer with the zip open + * locally still has somewhere to look. + * @param {string|null} url + * @param {string} entryKey - "kind/entryId" + * @returns {string} + */ +function exampleRef(url, entryKey) { + return url ? `([example](${url}))` : `(\`output/${entryKey}/\`)`; +} + +/** + * @param {number} n + * @param {string} word - singular form, e.g. "entry" + * @returns {string} "entry" or "entries"/"words" as appropriate + */ +function pluralizeWord(n, word) { + if (n === 1) return word; + return word.endsWith("y") ? `${word.slice(0, -1)}ies` : `${word}s`; +} + +/** + * Render a capped list of `{key, before, after}`, `{key, summary}`, or + * `{key, subLines}` change lines, disclosing how many were elided rather + * than silently dropping them. `subLines` (e.g. a `dependencies` change with + * one line per category: ledgers/handles/account ranges) renders as a nested + * list under the change's key instead of one semicolon-packed line. + * @param {Array} changes + * @returns {Array} + */ +function formatChangeLines(changes) { + const lines = []; + for (const change of changes.slice(0, MAX_CHANGES_SHOWN)) { + const times = change.count > 1 ? `[${change.count}×] ` : ""; + if (change.subLines !== undefined) { + lines.push(`- ${times}\`${change.key}\`:`); + for (const sub of change.subLines) lines.push(` - ${sub}`); + continue; + } + const body = change.summary !== undefined ? change.summary : `\`${change.before}\` → \`${change.after}\``; + lines.push(`- ${times}\`${change.key}\`: ${body}`); + } + if (changes.length > MAX_CHANGES_SHOWN) { + lines.push(`- … +${changes.length - MAX_CHANGES_SHOWN} more change${changes.length - MAX_CHANGES_SHOWN === 1 ? "" : "s"}`); + } + return lines; +} + +/** + * Format the full compact diff as Markdown, suitable for stdout or a PR + * comment. Sections only appear when they have content, and every finding + * points at a concrete file or URL to follow up on. * @param {ReturnType} data * @returns {string} */ function formatCompact(data) { - const { summary, templates } = data; + const summary = data.summary || {}; + const templates = data.templates || []; + const scopeTemplates = data.scopeTemplates || []; + const collapsedTemplates = data.collapsedTemplates || []; + const visualOnlyEntries = data.visualOnlyEntries || []; const lines = []; - lines.push("## 🧪 Sampler compact diff (named_results)"); + + lines.push("## 🧪 Sampler compact diff"); lines.push(""); - const skippedNote = - summary.entriesSkipped > 0 - ? `, ${summary.entriesSkipped} skipped (unreadable registers.json)` - : ""; + const nothingChanged = templates.length === 0 && scopeTemplates.length === 0 && collapsedTemplates.length === 0 && visualOnlyEntries.length === 0; + const skippedNote = summary.entriesSkipped > 0 ? `, ${summary.entriesSkipped} skipped (unreadable registers.json)` : ""; - if (templates.length === 0) { - lines.push( - `No \`named_results\` changes across ${summary.entriesSampled} sampled entr${summary.entriesSampled === 1 ? "y" : "ies"}${skippedNote}.`, - ); + if (nothingChanged) { + lines.push(`No changes detected across ${summary.entriesSampled} sampled ${pluralizeWord(summary.entriesSampled, "entry")}${skippedNote}.`); return lines.join("\n"); } - lines.push( - `**${summary.templatesChanged}** template(s) changed across **${summary.entriesChanged}** ` + - `entr${summary.entriesChanged === 1 ? "y" : "ies"} (${summary.entriesSampled} sampled${skippedNote}).`, - ); + const headline = [`**${summary.templatesChanged}** template(s) changed across **${summary.entriesChanged}** ${pluralizeWord(summary.entriesChanged, "entry")}`]; + headline.push(`(${summary.entriesSampled} sampled${skippedNote})`); + lines.push(headline.join(" ")); + + if (collapsedTemplates.length > 0) { + lines.push(""); + lines.push(`### ⚠️ Output vanished (${summary.collapsedCount} ${pluralizeWord(summary.collapsedCount, "entry")} across ${collapsedTemplates.length} template(s))`); + lines.push("Rendered output that existed before is completely gone after - check for a broken include/tag, not a data change."); + for (const template of collapsedTemplates) { + lines.push(""); + const ref = exampleRef(template.exampleUrl, template.exampleEntryKey); + lines.push(`**${template.label}** — ${template.entriesChanged} ${pluralizeWord(template.entriesChanged, "entry")} collapsed ${ref}`); + lines.push(...formatChangeLines(template.changes)); + } + } for (const template of templates) { lines.push(""); - lines.push(`### ${template.label} — ${template.entriesChanged} entr${template.entriesChanged === 1 ? "y" : "ies"} changed`); - for (const change of template.changes) { - const times = change.count > 1 ? `[${change.count}×] ` : ""; - lines.push(`- ${times}\`${change.key}\`: \`${change.before}\` → \`${change.after}\``); + const ref = exampleRef(template.exampleUrl, template.exampleEntryKey); + lines.push(`### ${template.label} — ${template.entriesChanged} ${pluralizeWord(template.entriesChanged, "entry")} changed ${ref}`); + lines.push(...formatChangeLines(template.changes)); + } + + if (scopeTemplates.length > 0) { + lines.push(""); + lines.push("### 🔧 Scope/dependency changes"); + lines.push("What each template depends on/requires changed - not its rendered data."); + for (const template of scopeTemplates) { + lines.push(""); + const ref = exampleRef(template.exampleUrl, template.exampleEntryKey); + lines.push(`**${template.label}** — ${template.entriesChanged} ${pluralizeWord(template.entriesChanged, "entry")} ${ref}`); + lines.push(...formatChangeLines(template.changes)); + } + } + + if (visualOnlyEntries.length > 0) { + lines.push(""); + lines.push(`### 👁️ Visual-only changes (${visualOnlyEntries.length} ${pluralizeWord(visualOnlyEntries.length, "entry")}, data unchanged)`); + lines.push("`view.html` differs even though named_results/results didn't - a markup/layout change the data diff can't see."); + for (const entry of visualOnlyEntries) { + lines.push(""); + const entryKey = `${entry.kind}/${entry.entryId}`; + const ref = entry.url ? `[open in app](${entry.url}) · ` : ""; + lines.push(`**${entry.label}** — ${ref}\`output/${entryKey}/{before,after}/view.html\``); + for (const note of entry.changes.slice(0, MAX_VISUAL_CHANGES_SHOWN)) { + lines.push(`- ${note}`); + } + if (entry.changes.length > MAX_VISUAL_CHANGES_SHOWN) { + const hidden = entry.changes.length - MAX_VISUAL_CHANGES_SHOWN; + lines.push(`- … +${hidden} more change${hidden === 1 ? "" : "s"}`); + } } } return lines.join("\n"); } -module.exports = { extractCompact, formatCompact, diffNamedResults, readEntryLabels }; +module.exports = { + extractCompact, + formatCompact, + diffNamedResults, + diffResultsRegister, + diffScope, + describeVisualChange, + readEntryLabels, +}; diff --git a/lib/liquidSamplerRunner.js b/lib/liquidSamplerRunner.js index f1400b0..d46a043 100644 --- a/lib/liquidSamplerRunner.js +++ b/lib/liquidSamplerRunner.js @@ -327,22 +327,63 @@ class LiquidSamplerRunner { } /** - * Download the result zip, extract the named_results diff, and print a compact - * review-friendly summary to stdout (wrapped in markers for CI extraction). - * A failure here never fails the run - the run itself already succeeded and the - * report URL was logged; the compact diff is a best-effort convenience. + * Download the result zip and print the compact diff. A failure here never + * fails the run - the run itself already succeeded and the report URL was + * logged; the compact diff is a best-effort convenience. * @param {string} resultUrl - Presigned URL to the results.zip */ async #printCompactDiff(resultUrl) { + try { + const response = await axios.get(resultUrl, { + responseType: "arraybuffer", + timeout: RESULTS_DOWNLOAD_TIMEOUT_MS, + }); + await this.#printCompactFromBuffer(Buffer.from(response.data)); + } catch (error) { + consola.warn(`Could not build compact diff (report is still available at the URL above): ${error.message}`); + } + } + + /** + * Build the compact diff from an already-downloaded local results.zip - no + * network call, no partner/sampler API involved. Used by `--from-zip` to + * re-analyze a zip a reviewer already has on disk without re-triggering the + * backend run (which otherwise means another ~30-60 min wait per re-check). + * Unlike the live-run path, a failure here IS the whole point of the + * command, so it's a hard error rather than a warning. + * @param {string} zipPath - Path to a local results.zip + * @returns {Promise} + */ + async printCompactDiffFromZip(zipPath) { + let buffer; + try { + buffer = fs.readFileSync(zipPath); + } catch (error) { + consola.error(`Could not read zip at ${zipPath}: ${error.message}`); + process.exit(1); + } + try { + await this.#printCompactFromBuffer(buffer); + } catch (error) { + consola.error(`Could not build compact diff: ${error.message}`); + process.exit(1); + } + } + + /** + * Shared extract-diff-print-cleanup path for both the live download and + * the local-zip entry point. Lets errors propagate - callers decide whether + * a failure here is a soft warning or a hard exit. + * @param {Buffer} zipBuffer + */ + async #printCompactFromBuffer(zipBuffer) { let resultsDir; try { - resultsDir = await this.#downloadAndExtractResults(resultUrl); + resultsDir = this.#extractResults(zipBuffer); const data = extractCompact(resultsDir); const body = escapeMarkers(formatCompact(data)); // Plain stdout (not consola) so the markdown is captured verbatim in CI. console.log(`\n${COMPACT_START}\n${body}\n${COMPACT_END}`); - } catch (error) { - consola.warn(`Could not build compact diff (report is still available at the URL above): ${error.message}`); } finally { if (resultsDir) { fs.rmSync(resultsDir, { recursive: true, force: true }); @@ -351,25 +392,23 @@ class LiquidSamplerRunner { } /** - * Download the results.zip and selectively extract only the files needed for - * the named_results diff (sample_entry_ids.yml + every registers.json). The - * full archive is ~150 MB; extracting selectively keeps disk/IO minimal. - * @param {string} resultUrl - Presigned URL to the results.zip - * @returns {Promise} Path to a temp directory holding the extracted files + * Selectively extract only the files the compact diff needs + * (sample_entry_ids.yml + every registers.json + every view.html) from a + * results.zip buffer. The full archive can run to ~150 MB (rendered_text.md + * + source_text.liquid dominate); extracting selectively keeps disk/IO minimal. + * @param {Buffer} zipBuffer + * @returns {string} Path to a temp directory holding the extracted files */ - async #downloadAndExtractResults(resultUrl) { - const response = await axios.get(resultUrl, { - responseType: "arraybuffer", - timeout: RESULTS_DOWNLOAD_TIMEOUT_MS, - }); - const zip = new AdmZip(Buffer.from(response.data)); + #extractResults(zipBuffer) { + const zip = new AdmZip(zipBuffer); const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "silverfin-sampler-")); 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 isNeeded = name === "sample_entry_ids.yml" || name.endsWith("/registers.json") || name.endsWith("/view.html"); + if (!isNeeded) continue; const dest = path.resolve(tempDir, name); // Reject entries whose name (e.g. "../../etc/passwd") resolves outside diff --git a/package-lock.json b/package-lock.json index d6e27f8..e566d65 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "silverfin-cli", - "version": "1.57.1", + "version": "1.58.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "silverfin-cli", - "version": "1.57.1", + "version": "1.58.0", "license": "MIT", "dependencies": { "adm-zip": "^0.5.18", diff --git a/package.json b/package.json index 4d216bb..eb59e2f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "silverfin-cli", - "version": "1.57.1", + "version": "1.58.0", "description": "Command line tool for Silverfin template development", "main": "index.js", "license": "MIT", diff --git a/tests/lib/liquidSamplerCompact.test.js b/tests/lib/liquidSamplerCompact.test.js index ce75ad1..b0d9f68 100644 --- a/tests/lib/liquidSamplerCompact.test.js +++ b/tests/lib/liquidSamplerCompact.test.js @@ -1,14 +1,29 @@ const fs = require("fs"); const os = require("os"); const path = require("path"); -const { extractCompact, formatCompact, diffNamedResults, readEntryLabels } = require("../../lib/liquidSamplerCompact"); +const { + extractCompact, + formatCompact, + diffNamedResults, + diffResultsRegister, + diffScope, + describeVisualChange, + readEntryLabels, +} = require("../../lib/liquidSamplerCompact"); const FIXTURE_DIR = path.join(__dirname, "..", "fixtures", "sampler-results"); /** - * Build a minimal results directory (sample_entry_ids.yml + output/) under a - * fresh temp dir, from a plain description of entries per kind. - * @param {Object>} byKind + * Build a results directory (sample_entry_ids.yml + output/) under a fresh + * temp dir, from a plain description of entries per kind. Each entry accepts + * `before`/`after` (named_results), plus optional `registers` overrides + * (results/dependencies/rollforward_params/required_keys_missing, merged in + * per phase) and `viewHtml` (raw content per phase). + * @param {Object>} byKind * @returns {string} path to the built results directory */ function buildResultsDir(byKind) { @@ -17,11 +32,15 @@ function buildResultsDir(byKind) { for (const [kind, entries] of Object.entries(byKind)) { yml[kind] = {}; for (const entry of entries) { - yml[kind][entry.id] = { label: entry.label, url: null }; + yml[kind][entry.id] = { label: entry.label, url: entry.url ?? null }; for (const phase of ["before", "after"]) { const entryDir = path.join(dir, "output", kind, entry.id, phase); fs.mkdirSync(entryDir, { recursive: true }); - fs.writeFileSync(path.join(entryDir, "registers.json"), JSON.stringify({ named_results: entry[phase] })); + const registers = { named_results: entry[phase], ...(entry.registers && entry.registers[phase]) }; + fs.writeFileSync(path.join(entryDir, "registers.json"), JSON.stringify(registers)); + if (entry.viewHtml && entry.viewHtml[phase] !== undefined) { + fs.writeFileSync(path.join(entryDir, "view.html"), entry.viewHtml[phase]); + } } } } @@ -64,6 +83,134 @@ describe("liquidSamplerCompact - diffNamedResults", () => { const changes = diffNamedResults({ a: [1, 2] }, { a: [2, 1] }); expect(changes.map((c) => c.key)).toEqual(["a"]); }); + + it("truncates long values instead of printing them in full", () => { + const longText = "x".repeat(500); + const changes = diffNamedResults({ a: longText }, { a: "short" }); + expect(changes[0].before).toMatch(/^"x+…\(502 chars\)$/); + expect(changes[0].before.length).toBeLessThan(120); + }); +}); + +describe("liquidSamplerCompact - diffResultsRegister", () => { + it("returns null when unchanged", () => { + expect(diffResultsRegister(["1.0"], ["1.0"])).toBeNull(); + expect(diffResultsRegister(null, null)).toBeNull(); + expect(diffResultsRegister(undefined, null)).toBeNull(); + }); + + it("formats an all-0/1 array as a triggered count", () => { + expect(diffResultsRegister(["1.0"], ["0.0"])).toEqual({ before: "1/1 triggered", after: "0/1 triggered" }); + expect(diffResultsRegister(["0.0", "0.0"], ["1.0", "0.0"])).toEqual({ before: "0/2 triggered", after: "1/2 triggered" }); + }); + + it("falls back to the raw value for non-flag (raw numeric) results arrays", () => { + const diff = diffResultsRegister(["996.08", "0.0"], ["996.08", "27171.4"]); + expect(diff.before).toBe('["996.08","0.0"]'); + expect(diff.after).toBe('["996.08","27171.4"]'); + }); + + it("treats a vanished results register as undefined", () => { + expect(diffResultsRegister(["1.0"], null)).toEqual({ before: "1/1 triggered", after: "undefined" }); + }); +}); + +describe("liquidSamplerCompact - diffScope", () => { + it("returns nothing when dependencies/rollforward_params/required_keys_missing are unchanged", () => { + const registers = { + dependencies: { ledgers: [1], reconciliations: { handles_per_ledger: { 1: ["general_settings"] } } }, + rollforward_params: [{ name: "a" }], + required_keys_missing: ["x"], + }; + expect(diffScope(registers, registers)).toEqual([]); + }); + + it("summarizes dependency changes as one sub-line per category (ledgers/handles/...), not a semicolon-packed dump", () => { + const before = { dependencies: { ledgers: [1, 2], reconciliations: { handles_per_ledger: { 1: ["a", "b"], 2: ["c"] } } } }; + const after = { dependencies: { reconciliations: { handles_per_ledger: { 1: ["a"] } } } }; + const [change] = diffScope(before, after); + expect(change.key).toBe("dependencies"); + expect(change.subLines).toHaveLength(2); + expect(change.subLines[0]).toBe("−1 ledger (2)"); + expect(change.subLines[1]).toContain("handle"); + expect(change.subLines[1]).toContain("b"); + expect(change.subLines[1]).toContain("c"); + }); + + it("summarizes rollforward_params by name", () => { + const before = { rollforward_params: [{ name: "selected.size_company" }, { name: "deposit_layout.dropdown" }] }; + const after = { rollforward_params: [{ name: "selected.size_company" }, { name: "show_prev_year_balance.presentation" }] }; + const [change] = diffScope(before, after); + expect(change.key).toBe("rollforward_params"); + expect(change.summary).toBe("−1 param (deposit_layout.dropdown), +1 param (show_prev_year_balance.presentation)"); + }); + + it("summarizes required_keys_missing as added/removed keys", () => { + const before = { required_keys_missing: ["letter_of_representation.date"] }; + const after = { required_keys_missing: ["letter_of_representation.date", "report.period", "statements.signed"] }; + const [change] = diffScope(before, after); + expect(change.key).toBe("required_keys_missing"); + expect(change.summary).toBe("+2 keys (report.period, statements.signed)"); + }); + + it("caps preview lists and discloses the remainder", () => { + const before = { required_keys_missing: [] }; + const after = { required_keys_missing: ["a", "b", "c", "d", "e"] }; + const [change] = diffScope(before, after); + expect(change.summary).toBe("+5 keys (a, b, c +2 more)"); + }); +}); + +describe("liquidSamplerCompact - describeVisualChange", () => { + const field = (name, attrs, value) => + ``; + + it("reports a removed field when a data-name'd tag disappears entirely", () => { + const before = field("salutation.address", 'placeholder="Begroeting"', "Aan het bestuur van:"); + const after = ` `; + const notes = describeVisualChange(before, after); + expect(notes).toEqual(["field `salutation.address` removed"]); + }); + + it("reports an added field the same way, symmetrically", () => { + const before = ``; + const after = field("salutation.address", 'placeholder="Begroeting"', "Aan het bestuur van:"); + const notes = describeVisualChange(before, after); + expect(notes).toEqual(["field `salutation.address` added"]); + }); + + it("reports a placeholder change on an otherwise-unchanged field (the real ac_policies_BS case)", () => { + const before = field("salutation.header", 'placeholder=""', "Geacht bestuur,"); + const after = field("salutation.header", 'placeholder="Geacht bestuur,"', "Geacht bestuur,"); + const notes = describeVisualChange(before, after); + expect(notes).toEqual(['field `salutation.header` placeholder: "" → "Geacht bestuur,"']); + }); + + it("reports a value change on a field whose placeholder didn't change", () => { + const before = field("company_city", 'placeholder=""', "Amsterdam"); + const after = field("company_city", 'placeholder=""', "Rotterdam"); + const notes = describeVisualChange(before, after); + expect(notes).toEqual(['field `company_city` value: "Amsterdam" → "Rotterdam"']); + }); + + it("reports both a value and a placeholder change as two separate notes", () => { + const before = field("x", 'placeholder="old hint"', "old value"); + const after = field("x", 'placeholder="new hint"', "new value"); + const notes = describeVisualChange(before, after); + expect(notes).toEqual(['field `x` value: "old value" → "new value"', 'field `x` placeholder: "old hint" → "new hint"']); + }); + + it("falls back to a generic note when no anchored field explains the diff", () => { + const notes = describeVisualChange("
old layout
", "
new layout
"); + expect(notes).toEqual(["layout/markup changed with no anchored field explaining it - compare the two view.html files directly"]); + }); + + it("says nothing about fields that are identical in both", () => { + const before = field("unchanged", 'placeholder="p"', "v") + field("changed", "", "old"); + const after = field("unchanged", 'placeholder="p"', "v") + field("changed", "", "new"); + const notes = describeVisualChange(before, after); + expect(notes).toEqual(['field `changed` value: "old" → "new"']); + }); }); describe("liquidSamplerCompact - readEntryLabels", () => { @@ -126,11 +273,18 @@ describe("liquidSamplerCompact - extractCompact", () => { expect(streetChange).toEqual({ key: "street_var", before: '""', after: "null", count: 2 }); }); - it("surfaces a broken template (value -> removed) as undefined", () => { + it("surfaces a broken template (value -> removed) as undefined when below the collapse threshold", () => { const liq = data.templates.find((t) => t.label === "liquidation_reserve"); const change = liq.changes.find((c) => c.key === "distributable_at_5"); expect(change.before).toBe('"20615.89"'); expect(change.after).toBe("undefined"); + // Only 1 key lost - not enough to call it a collapse. + expect(data.collapsedTemplates).toEqual([]); + }); + + it("attaches an example URL to each template for follow-up", () => { + const vkt = data.templates.find((t) => t.label === "vkt_1"); + expect(vkt.exampleUrl).toMatch(/^https:\/\/example\.staging\.getsilverfin\.com/); }); it("falls back to raw entry id when labels are missing", () => { @@ -218,27 +372,352 @@ describe("liquidSamplerCompact - extractCompact", () => { fs.rmSync(dir, { recursive: true, force: true }); } }); + + it("groups >= 3 keys vanishing at once into a single collapsed-template finding", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { + id: "7000", + label: "ac_policies_BS", + before: { a: "long policy text a", b: "long policy text b", c: "long policy text c" }, + after: {}, + }, + ], + }); + try { + const data = extractCompact(dir); + expect(data.templates).toEqual([]); // not folded into the normal data-diff tier + expect(data.collapsedTemplates).toHaveLength(1); + expect(data.collapsedTemplates[0]).toMatchObject({ label: "ac_policies_BS", entriesChanged: 1 }); + expect(data.collapsedTemplates[0].changes[0]).toMatchObject({ key: "output vanished", summary: "3 value(s) lost" }); + expect(data.summary.collapsedCount).toBe(1); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("groups multiple collapsed entries of the same template into one finding", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { id: "7010", label: "note_BEivAgi", before: { a: "1", b: "2", c: "3" }, after: {} }, + { id: "7011", label: "note_BEivAgi", before: { a: "1", b: "2", c: "3" }, after: {} }, + { id: "7012", label: "note_BEivAgi", before: { a: "1", b: "2", c: "3", d: "4" }, after: {} }, + ], + }); + try { + const data = extractCompact(dir); + expect(data.collapsedTemplates).toHaveLength(1); + expect(data.collapsedTemplates[0].entriesChanged).toBe(3); + // Two entries lost 3 keys (deduped with a count), one lost 4 (separate line). + expect(data.collapsedTemplates[0].changes).toEqual( + expect.arrayContaining([ + { key: "output vanished", summary: "3 value(s) lost", count: 2 }, + { key: "output vanished", summary: "4 value(s) lost", count: 1 }, + ]), + ); + expect(data.summary.collapsedCount).toBe(3); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("does not treat 1-2 lost keys as a collapse (stays in the normal data-diff tier)", () => { + const dir = buildResultsDir({ + reconciliation_entries: [{ id: "7001", label: "small_break", before: { a: "1", b: "2" }, after: {} }], + }); + try { + const data = extractCompact(dir); + expect(data.collapsedTemplates).toEqual([]); + expect(data.templates.map((t) => t.label)).toEqual(["small_break"]); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("does not treat a template gaining named_results from nothing as a collapse", () => { + const dir = buildResultsDir({ + reconciliation_entries: [{ id: "7002", label: "newly_populated", before: {}, after: { a: "1", b: "2", c: "3" } }], + }); + try { + const data = extractCompact(dir); + expect(data.collapsedTemplates).toEqual([]); + expect(data.templates.map((t) => t.label)).toEqual(["newly_populated"]); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("reports a `results` register change alongside named_results changes", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { + id: "8000", + label: "general_settings", + before: {}, + after: {}, + registers: { before: { results: ["1.0"] }, after: { results: ["0.0"] } }, + }, + ], + }); + try { + const data = extractCompact(dir); + const template = data.templates.find((t) => t.label === "general_settings"); + const resultsChange = template.changes.find((c) => c.key === "results"); + expect(resultsChange).toMatchObject({ before: "1/1 triggered", after: "0/1 triggered" }); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("reports scope (dependencies/rollforward_params/required_keys_missing) changes separately from data changes", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { + id: "9000", + label: "general_settings", + before: {}, + after: {}, + registers: { + before: { dependencies: { ledgers: [1, 2] } }, + after: { dependencies: { ledgers: [1] } }, + }, + }, + ], + }); + try { + const data = extractCompact(dir); + // No data (named_results/results) change - must not appear in the main tier. + expect(data.templates).toEqual([]); + expect(data.scopeTemplates).toHaveLength(1); + expect(data.scopeTemplates[0].label).toBe("general_settings"); + expect(data.scopeTemplates[0].changes[0]).toMatchObject({ key: "dependencies" }); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("flags a visual-only change (view.html differs, named_results/results identical) and describes it field-by-field", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { + id: "10000", + label: "general_settings", + before: { a: "1" }, + after: { a: "1" }, + viewHtml: { + before: '', + after: '', + }, + }, + ], + }); + try { + const data = extractCompact(dir); + expect(data.templates).toEqual([]); + expect(data.visualOnlyEntries).toHaveLength(1); + expect(data.visualOnlyEntries[0]).toMatchObject({ label: "general_settings", entryId: "10000" }); + expect(data.visualOnlyEntries[0].changes).toEqual(['field `salutation.header` placeholder: "" → "Geacht bestuur,"']); + expect(data.summary.visualOnlyCount).toBe(1); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("does NOT flag a visual-only change when the data already changed (avoids redundant noise)", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { + id: "10001", + label: "general_settings", + before: { a: "1" }, + after: { a: "2" }, + viewHtml: { before: "
old
", after: "
new
" }, + }, + ], + }); + try { + const data = extractCompact(dir); + expect(data.visualOnlyEntries).toEqual([]); + expect(data.templates).toHaveLength(1); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("does not flag anything when view.html wasn't extracted at all", () => { + // No viewHtml given -> files simply don't exist, same as the old selective extraction. + const dir = buildResultsDir({ + reconciliation_entries: [{ id: "10002", label: "general_settings", before: { a: "1" }, after: { a: "1" } }], + }); + try { + const data = extractCompact(dir); + expect(data.visualOnlyEntries).toEqual([]); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); }); describe("liquidSamplerCompact - formatCompact", () => { - it("renders a markdown summary with per-template sections", () => { + it("renders a markdown summary with per-template sections and an example link", () => { const md = formatCompact(extractCompact(FIXTURE_DIR)); - expect(md).toContain("Sampler compact diff (named_results)"); + expect(md).toContain("## 🧪 Sampler compact diff"); expect(md).toContain("### vkt_1"); expect(md).toContain("[2×] `street_var`: `\"\"` → `null`"); expect(md).toContain("### liquidation_reserve"); + expect(md).toContain("[example](https://example.staging.getsilverfin.com"); }); - it("renders a clear message when nothing changed", () => { - const md = formatCompact({ summary: { templatesChanged: 0, entriesChanged: 0, entriesSampled: 5 }, templates: [] }); - expect(md).toContain("No `named_results` changes across 5 sampled entries"); + it("renders a clear message when nothing changed at all, across every tier", () => { + const md = formatCompact({ summary: { entriesSampled: 5 }, templates: [], scopeTemplates: [], collapsedTemplates: [], visualOnlyEntries: [] }); + expect(md).toContain("No changes detected across 5 sampled entries"); }); it("discloses skipped entries when some registers.json were unreadable", () => { const md = formatCompact({ summary: { templatesChanged: 0, entriesChanged: 0, entriesSampled: 5, entriesSkipped: 2 }, templates: [], + scopeTemplates: [], + collapsedTemplates: [], + visualOnlyEntries: [], }); expect(md).toContain("2 skipped (unreadable registers.json)"); }); + + it("caps the number of change lines per template and discloses the remainder", () => { + const before = {}; + const after = {}; + for (let i = 0; i < 12; i++) after[`key_${i}`] = `value_${i}`; + const data = extractCompact( + buildResultsDir({ reconciliation_entries: [{ id: "1", label: "many_changes", before, after }] }), + ); + const md = formatCompact(data); + const changeLines = md.split("\n").filter((l) => l.startsWith("- `key_")); + expect(changeLines).toHaveLength(8); + expect(md).toContain("+4 more changes"); + }); + + it("renders the collapsed-output section with a file/url pointer, ahead of the normal template sections", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { id: "1", label: "collapsed_tpl", before: { a: "1", b: "2", c: "3" }, after: {}, url: "https://app.example.com/entry/1" }, + ], + }); + try { + const md = formatCompact(extractCompact(dir)); + expect(md).toContain("⚠️ Output vanished"); + expect(md).toContain("collapsed_tpl"); + expect(md).toContain("3 value(s) lost"); + expect(md).toContain("[example](https://app.example.com/entry/1)"); + expect(md.indexOf("⚠️ Output vanished")).toBeLessThan(md.indexOf("🔧 Scope") === -1 ? Infinity : md.indexOf("🔧 Scope")); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("renders the scope section with a file/url pointer and without arrows (it's a delta, not a before/after pair)", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { + id: "1", + label: "scoped_tpl", + before: {}, + after: {}, + registers: { before: { required_keys_missing: [] }, after: { required_keys_missing: ["report.period"] } }, + url: "https://app.example.com/entry/1", + }, + ], + }); + try { + const md = formatCompact(extractCompact(dir)); + expect(md).toContain("🔧 Scope/dependency changes"); + expect(md).toContain("required_keys_missing"); + expect(md).toContain("report.period"); + expect(md).toContain("[example](https://app.example.com/entry/1)"); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("renders the visual-only section pointing at the view.html files, with the field-level change explained", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { + id: "1", + label: "visual_tpl", + before: { a: "1" }, + after: { a: "1" }, + viewHtml: { + before: '', + after: '', + }, + url: "https://app.example.com/entry/1", + }, + ], + }); + try { + const md = formatCompact(extractCompact(dir)); + expect(md).toContain("👁️ Visual-only changes"); + expect(md).toContain("visual_tpl"); + expect(md).toContain("[open in app](https://app.example.com/entry/1)"); + expect(md).toContain("output/reconciliation_entries/1/{before,after}/view.html"); + expect(md).toContain('- field `salutation.header` placeholder: "" → "Geacht bestuur,"'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("caps visual-only change notes per entry and discloses the remainder", () => { + const fields = Array.from({ length: 8 }, (_, i) => i); + const html = (val) => fields.map((i) => ``).join(""); + const dir = buildResultsDir({ + reconciliation_entries: [ + { + id: "1", + label: "many_visual_changes", + before: { a: "1" }, + after: { a: "1" }, + viewHtml: { before: html("old"), after: html("new") }, + }, + ], + }); + try { + const md = formatCompact(extractCompact(dir)); + const noteLines = md.split("\n").filter((l) => l.startsWith("- field `f")); + expect(noteLines).toHaveLength(6); + expect(md).toContain("+2 more change"); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("renders a dependencies change as an indented sub-list, one category per line", () => { + const dir = buildResultsDir({ + reconciliation_entries: [ + { + id: "1", + label: "deps_tpl", + before: {}, + after: {}, + registers: { + before: { dependencies: { ledgers: [1, 2], account_ranges: ["A%"] } }, + after: { dependencies: { ledgers: [1] } }, + }, + }, + ], + }); + try { + const md = formatCompact(extractCompact(dir)); + const lines = md.split("\n"); + const keyLineIndex = lines.findIndex((l) => l.includes("`dependencies`:")); + expect(keyLineIndex).toBeGreaterThan(-1); + // No colon-separated summary crammed onto the key line itself. + expect(lines[keyLineIndex].trim()).toMatch(/`dependencies`:$/); + // Each category is its own indented sub-bullet underneath. + expect(lines[keyLineIndex + 1]).toMatch(/^ {2}- −1 ledger \(2\)$/); + expect(lines[keyLineIndex + 2]).toMatch(/^ {2}- −1 account range \(A%\)$/); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); }); diff --git a/tests/lib/liquidSamplerRunner.test.js b/tests/lib/liquidSamplerRunner.test.js index 95542ea..d2029c0 100644 --- a/tests/lib/liquidSamplerRunner.test.js +++ b/tests/lib/liquidSamplerRunner.test.js @@ -159,6 +159,22 @@ describe("LiquidSamplerRunner - compact diff", () => { expect(output).toContain("injected"); }); + it("extracts view.html too, so a visual-only (data-unchanged) change surfaces", async () => { + const zip = new AdmZip(); + zip.addFile("sample_entry_ids.yml", Buffer.from(JSON.stringify({ reconciliation_entries: { 1: { label: "vkt_1", url: null } } }))); + zip.addFile("output/reconciliation_entries/1/before/registers.json", Buffer.from(JSON.stringify({ named_results: { a: "1" } }))); + zip.addFile("output/reconciliation_entries/1/after/registers.json", Buffer.from(JSON.stringify({ named_results: { a: "1" } }))); + zip.addFile("output/reconciliation_entries/1/before/view.html", Buffer.from("
old
")); + zip.addFile("output/reconciliation_entries/1/after/view.html", Buffer.from("
new
")); + axios.get.mockResolvedValue({ data: zip.toBuffer() }); + + await new LiquidSamplerRunner("1", { compact: true }).checkStatus("run-1"); + + const output = logSpy.mock.calls.map((c) => c[0]).join("\n"); + expect(output).toContain("👁️ Visual-only changes"); + expect(output).toContain("output/reconciliation_entries/1/{before,after}/view.html"); + }); + it("does NOT download or print a compact diff by default", async () => { await new LiquidSamplerRunner("1", { openReport: false }).checkStatus("run-1"); @@ -225,6 +241,53 @@ describe("LiquidSamplerRunner - compact diff", () => { }); }); +describe("LiquidSamplerRunner - compact diff from a local zip (--from-zip)", () => { + let logSpy; + let originalExit; + let zipPath; + + beforeEach(() => { + jest.clearAllMocks(); + originalExit = process.exit; + process.exit = jest.fn(); + logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); + zipPath = path.join(os.tmpdir(), `sampler-from-zip-test-${process.pid}.zip`); + fs.writeFileSync(zipPath, fixtureZipBuffer()); + }); + + afterEach(() => { + logSpy.mockRestore(); + process.exit = originalExit; + fs.rmSync(zipPath, { force: true }); + }); + + it("prints the compact diff without any network call", async () => { + await new LiquidSamplerRunner("1").printCompactDiffFromZip(zipPath); + + expect(axios.get).not.toHaveBeenCalled(); + expect(SF.readSamplerRun).not.toHaveBeenCalled(); + const output = logSpy.mock.calls.map((c) => c[0]).join("\n"); + expect(output).toContain(""); + expect(output).toContain("### vkt_1"); + }); + + it("exits non-zero with a clear error when the path doesn't exist", async () => { + await new LiquidSamplerRunner("1").printCompactDiffFromZip("/no/such/results.zip"); + + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("Could not read zip")); + expect(process.exit).toHaveBeenCalledWith(1); + }); + + it("exits non-zero (rather than silently warning) when the zip is unreadable", async () => { + fs.writeFileSync(zipPath, "not a zip file"); + + await new LiquidSamplerRunner("1").printCompactDiffFromZip(zipPath); + + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("Could not build compact diff")); + expect(process.exit).toHaveBeenCalledWith(1); + }); +}); + describe("LiquidSamplerRunner - polling status output", () => { const originalIsTTY = process.stdout.isTTY; const originalCI = process.env.CI; From 3aa9484804b2f14328542c238bc53d592926e366 Mon Sep 17 00:00:00 2001 From: Michiel Degezelle Date: Thu, 23 Jul 2026 13:47:20 +0200 Subject: [PATCH 2/3] Address review comments on run-sampler compact diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 field values, consistent with the