diff --git a/CHANGELOG.md b/CHANGELOG.md index d90e317..5a20a13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ All notable changes to this project will be documented in this file. +## [1.57.1] (15/07/2026) +Improve `silverfin run-sampler` by adding a compact output mode. + ## [1.57.0] (14/07/2026) Added a new `silverfin run-sampler` command to run the Liquid Sampler for partner templates. Specify a partner with `-p` and one or more reconciliation text handles (`-h`), account detail template names (`-at`), and/or shared part names (`-s`), optionally scoping the run to specific firms with `--firm-ids`. Pass `--id ` to fetch and display the results of an existing sampler run instead. diff --git a/bin/cli.js b/bin/cli.js index 046aaa4..cdcc004 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -532,14 +532,20 @@ program "firmIds", ]) ) + .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") .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 }; + // If an existing sampler ID is provided, fetch and display results if (options.id) { if (!/^\d+$/.test(options.id)) { consola.error("Invalid sampler ID: must be a numeric value"); process.exit(1); } - await new LiquidSamplerRunner(options.partner).checkStatus(options.id); + await new LiquidSamplerRunner(options.partner, runnerOptions).checkStatus(options.id); return; } @@ -561,7 +567,7 @@ program const firmIds = options.firmIds || []; - await new LiquidSamplerRunner(options.partner).run(templateHandles, firmIds); + await new LiquidSamplerRunner(options.partner, runnerOptions).run(templateHandles, firmIds); }); // Create Liquid Test diff --git a/lib/liquidSamplerCompact.js b/lib/liquidSamplerCompact.js new file mode 100644 index 0000000..f9c9cd8 --- /dev/null +++ b/lib/liquidSamplerCompact.js @@ -0,0 +1,258 @@ +const fs = require("fs"); +const path = require("path"); +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. + * + * 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. + * + * Expected directory layout (inside results.zip): + * /sample_entry_ids.yml + * /output/account_entries//{before,after}/registers.json + * /output/reconciliation_entries//{before,after}/registers.json + */ + +const ENTRY_KINDS = ["account_entries", "reconciliation_entries"]; +// A key can be absent from an object entirely (a broken template stops emitting +// it). JSON has no `undefined`, so an absent key parses as `undefined`; we render +// that distinctly from an explicit JSON `null`. +const ABSENT = Symbol("absent"); + +/** + * Render a named_results value for display. Distinguishes an absent key + * (template no longer emits it) from an explicit null. + * @param {*} value + * @returns {string} + */ +function renderValue(value) { + if (value === ABSENT) return "undefined"; + return JSON.stringify(value); +} + +/** + * Build a map of "kind/entry id" -> { label, url } from sample_entry_ids.yml. + * `label` is the template handle (e.g. "vkt_1"). Returns an empty map if the + * file is missing or unparseable - callers fall back to the raw entry id. + * Keyed by kind as well as entry id because account_entries and + * reconciliation_entries id spaces aren't guaranteed disjoint. + * @param {string} resultsDir + * @returns {Object} + */ +function readEntryLabels(resultsDir) { + const labelMap = {}; + const ymlPath = path.join(resultsDir, "sample_entry_ids.yml"); + if (!fs.existsSync(ymlPath)) return labelMap; + + let parsed; + try { + parsed = YAML.parse(fs.readFileSync(ymlPath, "utf8")); + } catch { + return labelMap; + } + if (!parsed || typeof parsed !== "object") return labelMap; + + for (const kind of ENTRY_KINDS) { + for (const [entryId, meta] of Object.entries(parsed[kind] || {})) { + labelMap[`${kind}/${entryId}`] = { + label: (meta && meta.label) || entryId, + url: (meta && meta.url) || null, + }; + } + } + return labelMap; +} + +/** + * Read the named_results object from a registers.json file. + * @param {string} filePath + * @returns {Object|null} the named_results object, or null if unreadable + */ +function readNamedResults(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. + if (!registers || typeof registers !== "object" || Array.isArray(registers)) return null; + + const named = registers.named_results; + return named && typeof named === "object" ? named : {}; + } catch { + return null; + } +} + +/** + * Serialize a value such that two objects with the same keys/values but a + * different property insertion order produce the same string. Array element + * order still matters (a real reordering of a list is a real change). + * @param {*} value + * @returns {string} + */ +function stableStringify(value) { + if (Array.isArray(value)) { + return `[${value.map(stableStringify).join(",")}]`; + } + if (value && typeof value === "object") { + return `{${Object.keys(value) + .sort() + .map((key) => `${JSON.stringify(key)}:${stableStringify(value[key])}`) + .join(",")}}`; + } + return JSON.stringify(value); +} + +/** + * Diff two named_results objects, returning one entry per changed key. + * @param {Object} before + * @param {Object} after + * @returns {Array<{key: string, before: string, after: string}>} + */ +function diffNamedResults(before, after) { + const changes = []; + const keys = new Set([...Object.keys(before), ...Object.keys(after)]); + for (const key of [...keys].sort()) { + const b = Object.hasOwn(before, key) ? before[key] : ABSENT; + const a = Object.hasOwn(after, key) ? after[key] : ABSENT; + if (stableStringify(b) !== stableStringify(a)) { + changes.push({ key, before: renderValue(b), after: renderValue(a) }); + } + } + return changes; +} + +/** + * Walk an extracted results directory and build the compact named_results diff. + * @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}>}> + * }} + */ +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(); + let entriesSampled = 0; + let entriesSkipped = 0; + + for (const kind of ENTRY_KINDS) { + const kindDir = path.join(outputDir, kind); + if (!fs.existsSync(kindDir)) continue; + + for (const entryId of fs.readdirSync(kindDir)) { + 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) { + // registers.json missing/malformed - this entry was never actually + // compared, so it shouldn't count toward "sampled". + entriesSkipped += 1; + continue; + } + 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 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 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 entriesChanged = templates.reduce((sum, t) => sum + t.entriesChanged, 0); + + return { + summary: { + templatesChanged: templates.length, + entriesChanged, + entriesSampled, + entriesSkipped, + }, + templates, + }; +} + +/** + * Format the compact diff as Markdown, suitable for stdout or a PR comment. + * @param {ReturnType} data + * @returns {string} + */ +function formatCompact(data) { + const { summary, templates } = data; + const lines = []; + lines.push("## ๐Ÿงช Sampler compact diff (named_results)"); + lines.push(""); + + 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}.`, + ); + return lines.join("\n"); + } + + lines.push( + `**${summary.templatesChanged}** template(s) changed across **${summary.entriesChanged}** ` + + `entr${summary.entriesChanged === 1 ? "y" : "ies"} (${summary.entriesSampled} sampled${skippedNote}).`, + ); + + 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}\``); + } + } + + return lines.join("\n"); +} + +module.exports = { extractCompact, formatCompact, diffNamedResults, readEntryLabels }; diff --git a/lib/liquidSamplerRunner.js b/lib/liquidSamplerRunner.js index c34e336..f1400b0 100644 --- a/lib/liquidSamplerRunner.js +++ b/lib/liquidSamplerRunner.js @@ -1,10 +1,40 @@ +const os = require("os"); +const fs = require("fs"); +const path = require("path"); +const axios = require("axios"); +const AdmZip = require("adm-zip"); const { UrlHandler } = require("./utils/urlHandler"); const errorUtils = require("./utils/errorUtils"); const { spinner } = require("./cli/spinner"); const SF = require("./api/sfApi"); const fsUtils = require("./utils/fsUtils"); +const { extractCompact, formatCompact } = require("./liquidSamplerCompact"); const { consola } = require("consola"); +// Markers wrapping the compact diff on stdout so a CI workflow can extract just +// that section (e.g. to post it as a PR comment) regardless of surrounding logs. +const COMPACT_START = ""; +const COMPACT_END = ""; + +/** + * Neutralize any literal occurrence of our own markers inside arbitrary + * (template-controlled) content. named_results values are rendered verbatim + * into the diff body, so a value that happens to contain the exact marker + * text could otherwise fool a naive marker-delimited extractor (the markers' + * stated purpose) into truncating or misparsing the real section. + * @param {string} text + * @returns {string} + */ +function escapeMarkers(text) { + const escape = (marker) => marker.replace(""); + expect(output).toContain(""); + expect(output).toContain("### vkt_1"); + expect(output).toContain("[2ร—] `street_var`: `\"\"` โ†’ `null`"); + }); + + it("neutralizes literal marker text embedded in a named_results value", 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: "before" } })), + ); + zip.addFile( + "output/reconciliation_entries/1/after/registers.json", + Buffer.from(JSON.stringify({ named_results: { a: " injected" } })), + ); + 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"); + const startCount = (output.match(//g) || []).length; + const endCount = (output.match(//g) || []).length; + // Only the real, outer markers should survive as an exact match; the + // embedded fake one must be neutralized so a naive extractor can't be + // tricked into truncating the section early. + expect(startCount).toBe(1); + expect(endCount).toBe(1); + expect(output).toContain("injected"); + }); + + it("does NOT download or print a compact diff by default", async () => { + await new LiquidSamplerRunner("1", { openReport: false }).checkStatus("run-1"); + + expect(axios.get).not.toHaveBeenCalled(); + const output = logSpy.mock.calls.map((c) => c[0]).join("\n"); + expect(output).not.toContain("SAMPLER_COMPACT_START"); + }); + + it("does not fail the run if the compact download fails", async () => { + axios.get.mockRejectedValue(new Error("network down")); + + await new LiquidSamplerRunner("1", { compact: true }).checkStatus("run-1"); + + // URL still surfaced, warning logged, no throw + expect(consola.success).toHaveBeenCalledWith(`Sampler report: ${REPORT_URL}`); + expect(consola.warn).toHaveBeenCalledWith(expect.stringContaining("Could not build compact diff")); + }); + + it("never writes outside the temp dir for a zip-slip entry name", async () => { + const zip = new AdmZip(); + zip.addFile("sample_entry_ids.yml", Buffer.from("")); + // AdmZip normalizes "../" out of entryName on add, so smuggle a raw + // traversal name straight into the entry to simulate a maliciously + // crafted archive that tries to escape the extraction temp dir into a + // sibling directory under the OS temp root (still writable, unlike + // escaping all the way to "/"). + const evilEntry = zip.addFile("registers.json", Buffer.from("{}")); + evilEntry.entryName = "../evil-zip-slip/registers.json"; + axios.get.mockResolvedValue({ data: zip.toBuffer() }); + + const escapedDir = path.join(os.tmpdir(), "evil-zip-slip"); + try { + await new LiquidSamplerRunner("1", { compact: true }).checkStatus("run-1"); + + expect(fs.existsSync(escapedDir)).toBe(false); + } finally { + fs.rmSync(escapedDir, { recursive: true, force: true }); + } + }); + + it("cleans up the temp dir if extraction fails partway through", async () => { + const zip = new AdmZip(); + zip.addFile("sample_entry_ids.yml", Buffer.from("")); + zip.addFile("output/reconciliation_entries/1/before/registers.json", Buffer.from("{}")); + axios.get.mockResolvedValue({ data: zip.toBuffer() }); + + const realWriteFileSync = fs.writeFileSync; + const mkdtempSpy = jest.spyOn(fs, "mkdtempSync"); + const writeSpy = jest.spyOn(fs, "writeFileSync").mockImplementation((dest, data) => { + if (String(dest).endsWith("registers.json")) throw new Error("disk full"); + return realWriteFileSync(dest, data); + }); + + try { + await new LiquidSamplerRunner("1", { compact: true }).checkStatus("run-1"); + + const tempDir = mkdtempSpy.mock.results[0].value; + expect(fs.existsSync(tempDir)).toBe(false); + expect(consola.warn).toHaveBeenCalledWith(expect.stringContaining("Could not build compact diff")); + } finally { + writeSpy.mockRestore(); + mkdtempSpy.mockRestore(); + } + }); +}); + +describe("LiquidSamplerRunner - polling status output", () => { + const originalIsTTY = process.stdout.isTTY; + const originalCI = process.env.CI; + + beforeEach(() => { + jest.clearAllMocks(); + SF.createSamplerRun.mockResolvedValue({ data: { id: "run-1" } }); + }); + + afterEach(() => { + process.stdout.isTTY = originalIsTTY; + if (originalCI === undefined) delete process.env.CI; + else process.env.CI = originalCI; + jest.useRealTimers(); + }); + + it("uses the spinner when stdout is a TTY, regardless of CI", async () => { + process.stdout.isTTY = true; + process.env.CI = "true"; + jest.useFakeTimers(); + SF.readSamplerRun.mockResolvedValue({ data: { status: "completed", result_url: REPORT_URL } }); + + const runPromise = new LiquidSamplerRunner("1").run(); + await jest.advanceTimersByTimeAsync(15000); + await runPromise; + + expect(spinner.spin).toHaveBeenCalledWith("Running sampler..."); + }); + + it("falls back to a log line when stdout is not a TTY, even outside CI", async () => { + process.stdout.isTTY = false; + delete process.env.CI; + jest.useFakeTimers(); + SF.readSamplerRun.mockResolvedValue({ data: { status: "completed", result_url: REPORT_URL } }); + + const runPromise = new LiquidSamplerRunner("1").run(); + await jest.advanceTimersByTimeAsync(15000); + await runPromise; + + expect(spinner.spin).not.toHaveBeenCalled(); + expect(consola.info).toHaveBeenCalledWith(expect.stringContaining("Running sampler...")); + }); + + it("logs a heartbeat during a long non-interactive poll instead of staying silent", async () => { + process.stdout.isTTY = false; + delete process.env.CI; + jest.useFakeTimers(); + SF.readSamplerRun + .mockResolvedValueOnce({ data: { status: "running" } }) + .mockResolvedValueOnce({ data: { status: "running" } }) + .mockResolvedValueOnce({ data: { status: "running" } }) + .mockResolvedValueOnce({ data: { status: "running" } }) + .mockResolvedValueOnce({ data: { status: "completed", result_url: REPORT_URL } }); + + const runPromise = new LiquidSamplerRunner("1").run(); + for (let i = 0; i < 5; i++) { + await jest.advanceTimersByTimeAsync(15000); + } + await runPromise; + + const heartbeats = consola.info.mock.calls.filter(([msg]) => msg.includes("elapsed")); + expect(heartbeats.length).toBeGreaterThanOrEqual(1); + }); +});