diff --git a/config/scripts/benchmark-artifact-comparison.test.mjs b/config/scripts/benchmark-artifact-comparison.test.mjs new file mode 100644 index 000000000..d7700deda --- /dev/null +++ b/config/scripts/benchmark-artifact-comparison.test.mjs @@ -0,0 +1,440 @@ +import { spawnSync } from 'node:child_process' +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { + compareBenchmarkArtifacts, + formatBenchmarkComparisonMarkdown, + parseBenchmarkComparisonArgs +} from './compare-benchmark-artifacts.mjs' + +const scriptPath = 'config/scripts/compare-benchmark-artifacts.mjs' +const tempDirs = [] + +function makeTempDir() { + const dir = mkdtempSync(join(tmpdir(), 'orca-benchmark-comparison-')) + tempDirs.push(dir) + return dir +} + +function writeArtifact(dir, name, artifact) { + const artifactPath = join(dir, name) + writeFileSync(artifactPath, JSON.stringify(artifact)) + return artifactPath +} + +function comparePaths(baselinePath, candidatePath, extra = {}) { + return compareBenchmarkArtifacts({ + baselinePath, + candidatePath, + now: () => new Date('2026-06-21T12:00:00.000Z'), + title: 'Test Compare', + ...extra + }) +} + +afterEach(() => { + while (tempDirs.length > 0) { + rmSync(tempDirs.pop(), { force: true, recursive: true }) + } +}) + +describe('benchmark artifact comparison', () => { + it('parses required CLI flags and rejects missing paths', () => { + expect(() => parseBenchmarkComparisonArgs(['--baseline'])).toThrow('--baseline requires a path') + expect(() => parseBenchmarkComparisonArgs(['--candidate'])).toThrow( + '--candidate requires a path' + ) + expect(() => parseBenchmarkComparisonArgs(['--candidate', 'candidate.json'])).toThrow( + 'Usage: node config/scripts/compare-benchmark-artifacts.mjs --baseline --candidate [--title ] [--output <path>] [--json-output <path>] [--higher-is-better <metric-key> ...]' + ) + }) + + it('compares startup-style summary median metrics and skips null candidates', () => { + const dir = makeTempDir() + const baselinePath = writeArtifact(dir, 'baseline.json', { + label: 'baseline', + summaryMedianMs: { + missingLater: 5, + spawnToAppReady: 25, + totalToDidFinishLoad: 100 + } + }) + const candidatePath = writeArtifact(dir, 'candidate.json', { + label: 'candidate', + summaryMedianMs: { + missingLater: null, + spawnToAppReady: 30, + totalToDidFinishLoad: 40 + } + }) + + const comparison = comparePaths(baselinePath, candidatePath) + + expect(comparison.schemaVersion).toBe(1) + expect(comparison.createdAt).toBe('2026-06-21T12:00:00.000Z') + expect(comparison.baseline.label).toBe('baseline') + expect(comparison.candidate.label).toBe('candidate') + expect( + comparison.metrics.find((metric) => metric.key === 'totalToDidFinishLoad') + ).toMatchObject({ + absoluteDelta: -60, + baseline: 100, + candidate: 40, + percentDelta: -60, + status: 'improved', + unit: 'ms' + }) + expect(comparison.metrics.find((metric) => metric.key === 'spawnToAppReady')).toMatchObject({ + status: 'regressed' + }) + expect(comparison.skippedMetrics).toContainEqual({ + key: 'missingLater', + reason: 'missing candidate metric' + }) + }) + + it('compares numeric Playwright annotation metrics and omits metadata fields', () => { + const dir = makeTempDir() + const baselinePath = writeArtifact(dir, 'baseline-playwright.json', { + suites: [ + { + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'opencode-scale-same-workspace-50', + description: + 'panes=50 frames=60 median=80.0ms worst=120.0ms rendererQueuedChars=1000 samples=1,2' + } + ] + } + ] + } + ] + } + ] + } + ] + }) + const candidatePath = writeArtifact(dir, 'candidate-playwright.json', { + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'opencode-scale-same-workspace-50', + description: + 'panes=50 frames=60 median=60.0ms worst=100.0ms rendererQueuedChars=800 samples=1,2' + } + ] + } + ] + } + ] + } + ] + }) + + const comparison = comparePaths(baselinePath, candidatePath) + const metricKeys = comparison.metrics.map((metric) => metric.key) + + expect(metricKeys).toContain('opencode-scale-same-workspace-50.median') + expect(metricKeys).toContain('opencode-scale-same-workspace-50.rendererQueuedChars') + expect(metricKeys).not.toContain('opencode-scale-same-workspace-50.panes') + expect(metricKeys).not.toContain('opencode-scale-same-workspace-50.frames') + expect(metricKeys).not.toContain('opencode-scale-same-workspace-50.samples') + expect( + comparison.metrics.find((metric) => metric.key === 'opencode-scale-same-workspace-50.median') + ).toMatchObject({ status: 'improved', unit: 'ms' }) + }) + + it('aggregates duplicate Playwright scenario metrics before comparison', () => { + const dir = makeTempDir() + const baselinePath = writeArtifact(dir, 'baseline-playwright-duplicates.json', { + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'opencode-duplicate', + description: 'median=80.0ms rendererQueuedChars=1000' + }, + { + type: 'opencode-duplicate', + description: 'median=100.0ms rendererQueuedChars=1400' + } + ] + } + ] + } + ] + } + ] + }) + const candidatePath = writeArtifact(dir, 'candidate-playwright-duplicates.json', { + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'opencode-duplicate', + description: 'median=60.0ms rendererQueuedChars=800' + }, + { + type: 'opencode-duplicate', + description: 'median=70.0ms rendererQueuedChars=1000' + } + ] + } + ] + } + ] + } + ] + }) + + const comparison = comparePaths(baselinePath, candidatePath) + const duplicateMedianMetrics = comparison.metrics.filter( + (metric) => metric.key === 'opencode-duplicate.median' + ) + + expect(duplicateMedianMetrics).toHaveLength(1) + expect(duplicateMedianMetrics[0]).toMatchObject({ + absoluteDelta: -25, + baseline: 90, + candidate: 65, + percentDelta: -27.8, + status: 'improved', + unit: 'ms' + }) + expect( + comparison.metrics.filter((metric) => metric.key === 'opencode-duplicate.rendererQueuedChars') + ).toHaveLength(1) + }) + + it('skips unit mismatches instead of comparing incompatible metrics', () => { + const dir = makeTempDir() + const baselinePath = writeArtifact(dir, 'baseline-playwright-units.json', { + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'opencode-units', + description: 'median=80.0ms rendererQueuedChars=1000' + } + ] + } + ] + } + ] + } + ] + }) + const candidatePath = writeArtifact(dir, 'candidate-playwright-units.json', { + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'opencode-units', + description: 'median=60 rendererQueuedChars=800' + } + ] + } + ] + } + ] + } + ] + }) + + const comparison = comparePaths(baselinePath, candidatePath) + + expect(comparison.metrics.map((metric) => metric.key)).toEqual([ + 'opencode-units.rendererQueuedChars' + ]) + expect(comparison.skippedMetrics).toContainEqual({ + key: 'opencode-units.median', + reason: 'unit mismatch (ms vs count)' + }) + }) + + it('supports higher-is-better metrics for generic summary artifacts', () => { + const dir = makeTempDir() + const baselinePath = writeArtifact(dir, 'generic-baseline.json', { + summary: { + totalBytes: 2048, + totalCpuPercent: { mean: 80 }, + throughput: 10 + } + }) + const candidatePath = writeArtifact(dir, 'generic-candidate.json', { + summary: { + totalBytes: 1024, + totalCpuPercent: { mean: 70 }, + throughput: 12 + } + }) + + const comparison = comparePaths(baselinePath, candidatePath, { + higherIsBetter: new Set(['summary.throughput']) + }) + + expect(comparison.metrics.find((metric) => metric.key === 'summary.throughput')).toMatchObject({ + direction: 'higher-is-better', + status: 'improved' + }) + expect(comparison.metrics.find((metric) => metric.key === 'summary.totalBytes')).toMatchObject({ + unit: 'bytes' + }) + expect( + comparison.metrics.find((metric) => metric.key === 'summary.totalCpuPercent.mean') + ).toMatchObject({ + unit: '%' + }) + }) + + it('writes Markdown and JSON reports from the CLI while printing Markdown', () => { + const dir = makeTempDir() + const baselinePath = writeArtifact(dir, 'baseline.json', { + label: 'baseline', + summaryMedianMs: { totalToDidFinishLoad: 100 } + }) + const candidatePath = writeArtifact(dir, 'candidate.json', { + label: 'candidate', + summaryMedianMs: { totalToDidFinishLoad: 40 } + }) + const markdownPath = join(dir, 'nested', 'comparison.md') + const jsonPath = join(dir, 'nested', 'comparison.json') + + const result = spawnSync( + process.execPath, + [ + scriptPath, + '--baseline', + baselinePath, + '--candidate', + candidatePath, + '--title', + 'Test Compare', + '--output', + markdownPath, + '--json-output', + jsonPath + ], + { cwd: process.cwd(), encoding: 'utf8' } + ) + + expect(result.status).toBe(0) + expect(result.stdout).toContain('| Metric | Baseline | Candidate | Delta | Delta % | Result |') + expect(result.stdout).toContain( + '| totalToDidFinishLoad | 100.0ms | 40.0ms | -60.0ms | -60.0% | improved |' + ) + expect(existsSync(markdownPath)).toBe(true) + expect(existsSync(jsonPath)).toBe(true) + expect(JSON.parse(readFileSync(jsonPath, 'utf8')).schemaVersion).toBe(1) + }) + + it('redacts absolute input paths from generated reports', () => { + const dir = makeTempDir() + const baselinePath = writeArtifact(dir, 'absolute-baseline.json', { + label: 'baseline', + summaryMedianMs: { totalToDidFinishLoad: 100 } + }) + const candidatePath = writeArtifact(dir, 'absolute-candidate.json', { + label: 'candidate', + summaryMedianMs: { totalToDidFinishLoad: 40 } + }) + const markdownPath = join(dir, 'comparison.md') + const jsonPath = join(dir, 'comparison.json') + + const result = spawnSync( + process.execPath, + [ + scriptPath, + '--baseline', + baselinePath, + '--candidate', + candidatePath, + '--output', + markdownPath, + '--json-output', + jsonPath + ], + { cwd: process.cwd(), encoding: 'utf8' } + ) + const json = JSON.parse(readFileSync(jsonPath, 'utf8')) + const markdown = readFileSync(markdownPath, 'utf8') + + expect(result.status).toBe(0) + expect(json.baseline.path).toBe('absolute-baseline.json') + expect(json.candidate.path).toBe('absolute-candidate.json') + expect(markdown).not.toContain(dir) + expect(result.stdout).not.toContain(dir) + }) + + it('escapes artifact-controlled Markdown fields in reports', () => { + const markdown = formatBenchmarkComparisonMarkdown({ + title: 'Compare\n[Injected](https://example.test)', + createdAt: '2026-06-21T12:00:00.000Z', + baseline: { label: 'base\n<label>', path: 'base|path', kind: 'summary' }, + candidate: { label: 'candidate', path: 'candidate.md', kind: 'summary' }, + metrics: [ + { + key: 'summary.value|with-pipe', + unit: '', + baseline: 1, + candidate: 2, + absoluteDelta: 1, + percentDelta: 100, + status: 'regressed' + } + ], + skippedMetrics: [{ key: 'missing\nmetric', reason: 'missing | candidate' }] + }) + + expect(markdown).toContain('# Compare \\[Injected\\]\\(https://example\\.test\\)') + expect(markdown).toContain('Baseline: base \\<label\\> (base|path)') + expect(markdown).toContain( + '| summary\\.value\\|with\\-pipe | 1.0 | 2.0 | 1.0 | 100.0% | regressed |' + ) + expect(markdown).toContain('- missing metric: missing | candidate') + }) + + it('fails unsupported artifacts in the CLI', () => { + const dir = makeTempDir() + const baselinePath = writeArtifact(dir, 'baseline.json', { hello: 'world' }) + const candidatePath = writeArtifact(dir, 'candidate.json', { hello: 'world' }) + + const result = spawnSync( + process.execPath, + [scriptPath, '--baseline', baselinePath, '--candidate', candidatePath], + { cwd: process.cwd(), encoding: 'utf8' } + ) + + expect(result.status).not.toBe(0) + expect(result.stderr).toContain('unsupported benchmark artifact') + }) +}) diff --git a/config/scripts/check-terminal-perf-report-budgets.mjs b/config/scripts/check-terminal-perf-report-budgets.mjs index 6e579e02d..693822373 100644 --- a/config/scripts/check-terminal-perf-report-budgets.mjs +++ b/config/scripts/check-terminal-perf-report-budgets.mjs @@ -1,5 +1,5 @@ -import { readFileSync } from 'node:fs' import { basename } from 'node:path' +import { collectTerminalPerfRows, readJsonReport } from './terminal-perf-report-annotations.mjs' const reportPaths = process.argv.slice(2) if (reportPaths[0] === '--') { @@ -26,55 +26,6 @@ const BUDGETS = { maxRendererDroppedBacklogs: 0 } -function readJsonReport(path) { - const raw = readFileSync(path, 'utf8') - const start = raw.indexOf('{') - const end = raw.lastIndexOf('}') - if (start === -1 || end <= start) { - throw new Error(`${path}: no JSON object found`) - } - return JSON.parse(raw.slice(start, end + 1)) -} - -function parseAnnotationDescription(description) { - const values = {} - for (const part of description.split(/\s+/)) { - const index = part.indexOf('=') - if (index === -1) { - continue - } - values[part.slice(0, index)] = part.slice(index + 1) - } - return values -} - -function collectTerminalPerfRows(report, source) { - const rows = [] - const visitSuite = (suite) => { - for (const spec of suite.specs ?? []) { - for (const test of spec.tests ?? []) { - for (const annotation of test.annotations ?? []) { - if (!annotation.type.startsWith('opencode-')) { - continue - } - rows.push({ - source, - scenario: annotation.type, - ...parseAnnotationDescription(annotation.description ?? '') - }) - } - } - } - for (const child of suite.suites ?? []) { - visitSuite(child) - } - } - for (const suite of report.suites ?? []) { - visitSuite(suite) - } - return rows -} - function parseMs(value, fieldName, row, failures) { if (value == null || value === '') { return null diff --git a/config/scripts/compare-benchmark-artifacts.mjs b/config/scripts/compare-benchmark-artifacts.mjs new file mode 100644 index 000000000..3dc293556 --- /dev/null +++ b/config/scripts/compare-benchmark-artifacts.mjs @@ -0,0 +1,394 @@ +import { mkdirSync, readFileSync, writeFileSync } from 'node:fs' +import { basename, dirname, isAbsolute, relative } from 'node:path' +import { pathToFileURL } from 'node:url' +import { collectTerminalPerfRows } from './terminal-perf-report-annotations.mjs' + +const USAGE = + 'Usage: node config/scripts/compare-benchmark-artifacts.mjs --baseline <path> --candidate <path> [--title <title>] [--output <path>] [--json-output <path>] [--higher-is-better <metric-key> ...]' + +const PLAYWRIGHT_METADATA_FIELDS = new Set(['source', 'scenario', 'panes', 'frames', 'samples']) + +export function parseBenchmarkComparisonArgs(argv = process.argv.slice(2)) { + const args = argv[0] === '--' ? argv.slice(1) : [...argv] + const parsed = { + higherIsBetter: new Set(), + title: 'Benchmark comparison' + } + + for (let index = 0; index < args.length; index += 1) { + const flag = args[index] + if (flag === '--baseline') { + parsed.baselinePath = readRequiredValue(args, ++index, '--baseline requires a path') + continue + } + if (flag === '--candidate') { + parsed.candidatePath = readRequiredValue(args, ++index, '--candidate requires a path') + continue + } + if (flag === '--title') { + parsed.title = readRequiredValue(args, ++index, '--title requires a value') + continue + } + if (flag === '--output') { + parsed.outputPath = readRequiredValue(args, ++index, '--output requires a path') + continue + } + if (flag === '--json-output') { + parsed.jsonOutputPath = readRequiredValue(args, ++index, '--json-output requires a path') + continue + } + if (flag === '--higher-is-better') { + let consumed = 0 + while (args[index + 1] != null && !args[index + 1].startsWith('--')) { + parsed.higherIsBetter.add(args[index + 1]) + index += 1 + consumed += 1 + } + if (consumed === 0) { + throw new Error('--higher-is-better requires a metric key') + } + continue + } + throw new Error(USAGE) + } + + if (!parsed.baselinePath) { + throw new Error(USAGE) + } + if (!parsed.candidatePath) { + throw new Error(USAGE) + } + return parsed +} + +function readRequiredValue(args, index, message) { + const value = args[index] + if (value == null || value.startsWith('--')) { + throw new Error(message) + } + return value +} + +export function readBenchmarkArtifact(path) { + return JSON.parse(readFileSync(path, 'utf8')) +} + +export function normalizeBenchmarkArtifact(path, artifact = readBenchmarkArtifact(path)) { + if (artifact?.summaryMedianMs != null) { + return normalizeNumericObject(path, artifact, 'startup', artifact.summaryMedianMs, () => 'ms') + } + if (artifact?.summaryMedian != null) { + return normalizeNumericObject(path, artifact, 'daemon', artifact.summaryMedian, (key) => + key.endsWith('Count') || key.endsWith('After') ? 'count' : 'ms' + ) + } + if (artifact?.suites != null) { + return normalizePlaywrightArtifact(path, artifact) + } + if (artifact?.summary != null) { + return normalizeSummaryArtifact(path, artifact) + } + throw new Error( + `${path}: unsupported benchmark artifact; expected summaryMedianMs, summaryMedian, Playwright suites, or top-level summary` + ) +} + +function artifactLabel(path, artifact) { + return typeof artifact?.label === 'string' && artifact.label.length > 0 + ? artifact.label + : basename(path) +} + +function normalizeNumericObject(path, artifact, kind, values, unitForKey) { + return { + kind, + label: artifactLabel(path, artifact), + metrics: Object.entries(values ?? {}) + .filter(([, value]) => Number.isFinite(value)) + .map(([key, value]) => ({ + direction: 'lower-is-better', + key, + unit: unitForKey(key), + value + })) + } +} + +function normalizePlaywrightArtifact(path, artifact) { + const rows = collectTerminalPerfRows(artifact, basename(path), { typePrefix: 'opencode-' }) + const groupedMetrics = new Map() + for (const row of rows) { + for (const [field, rawValue] of Object.entries(row)) { + if (PLAYWRIGHT_METADATA_FIELDS.has(field)) { + continue + } + const parsed = parseMetricValue(rawValue) + if (parsed == null) { + continue + } + const key = `${row.scenario}.${field}` + const metricGroup = groupedMetrics.get(key) ?? { + unit: parsed.unit, + values: [] + } + metricGroup.values.push(parsed.value) + groupedMetrics.set(key, metricGroup) + } + } + const metrics = [...groupedMetrics.entries()].map(([key, metricGroup]) => ({ + direction: 'lower-is-better', + key, + unit: metricGroup.unit, + value: mean(metricGroup.values) + })) + return { + kind: 'playwright', + label: artifactLabel(path, artifact), + metrics + } +} + +function mean(values) { + return values.reduce((sum, value) => sum + value, 0) / values.length +} + +function parseMetricValue(rawValue) { + if (typeof rawValue === 'string') { + const msMatch = rawValue.match(/^(-?\d+(?:\.\d+)?)ms$/) + if (msMatch) { + return { unit: 'ms', value: Number(msMatch[1]) } + } + } + const numericValue = Number(rawValue) + if (!Number.isFinite(numericValue)) { + return null + } + return { unit: 'count', value: numericValue } +} + +function normalizeSummaryArtifact(path, artifact) { + const metrics = [] + flattenSummary(metrics, ['summary'], artifact.summary) + return { + kind: 'summary', + label: artifactLabel(path, artifact), + metrics + } +} + +function flattenSummary(metrics, pathParts, value) { + if (Number.isFinite(value)) { + const key = pathParts.join('.') + metrics.push({ + direction: 'lower-is-better', + key, + unit: unitForSummaryKey(pathParts), + value + }) + return + } + if (value == null || typeof value !== 'object' || Array.isArray(value)) { + return + } + for (const [childKey, childValue] of Object.entries(value)) { + flattenSummary(metrics, [...pathParts, childKey], childValue) + } +} + +function unitForSummaryKey(pathParts) { + if (pathParts.some((part) => part.endsWith('CpuPercent'))) { + return '%' + } + if (pathParts.some((part) => part.endsWith('Bytes'))) { + return 'bytes' + } + return '' +} + +export function compareBenchmarkArtifacts({ + baselinePath, + candidatePath, + title = 'Benchmark comparison', + higherIsBetter = new Set(), + now = () => new Date() +}) { + const baseline = normalizeBenchmarkArtifact(baselinePath) + const candidate = normalizeBenchmarkArtifact(candidatePath) + const candidateMetrics = new Map(candidate.metrics.map((metric) => [metric.key, metric])) + const baselineMetrics = new Map(baseline.metrics.map((metric) => [metric.key, metric])) + const metrics = [] + const skippedMetrics = [] + + for (const baselineMetric of baseline.metrics) { + const candidateMetric = candidateMetrics.get(baselineMetric.key) + if (!isComparableMetric(baselineMetric)) { + skippedMetrics.push({ key: baselineMetric.key, reason: 'missing baseline metric' }) + continue + } + if (!isComparableMetric(candidateMetric)) { + skippedMetrics.push({ key: baselineMetric.key, reason: 'missing candidate metric' }) + continue + } + if (baselineMetric.unit !== candidateMetric.unit) { + skippedMetrics.push({ + key: baselineMetric.key, + reason: `unit mismatch (${formatUnitLabel(baselineMetric.unit)} vs ${formatUnitLabel(candidateMetric.unit)})` + }) + continue + } + const direction = higherIsBetter.has(baselineMetric.key) + ? 'higher-is-better' + : baselineMetric.direction + metrics.push(compareMetric(baselineMetric, candidateMetric, direction)) + } + + for (const candidateMetric of candidate.metrics) { + if (!baselineMetrics.has(candidateMetric.key)) { + skippedMetrics.push({ key: candidateMetric.key, reason: 'missing baseline metric' }) + } + } + + if (metrics.length === 0) { + throw new Error('No comparable benchmark metrics found.') + } + + return { + schemaVersion: 1, + createdAt: now().toISOString(), + title, + baseline: { + path: benchmarkDisplayPath(baselinePath), + label: baseline.label, + kind: baseline.kind + }, + candidate: { + path: benchmarkDisplayPath(candidatePath), + label: candidate.label, + kind: candidate.kind + }, + metrics, + skippedMetrics + } +} + +function benchmarkDisplayPath(path, cwd = process.cwd()) { + if (!isAbsolute(path)) { + return path + } + const relativePath = relative(cwd, path) + if (relativePath && !relativePath.startsWith('..') && !isAbsolute(relativePath)) { + return relativePath + } + return basename(path) +} + +function isComparableMetric(metric) { + return metric != null && Number.isFinite(metric.value) +} + +function compareMetric(baselineMetric, candidateMetric, direction) { + const rawDelta = candidateMetric.value - baselineMetric.value + const absoluteDelta = roundOneDecimal(rawDelta) + const percentDelta = + baselineMetric.value === 0 + ? null + : roundOneDecimal((rawDelta / Math.abs(baselineMetric.value)) * 100) + return { + key: baselineMetric.key, + unit: baselineMetric.unit, + direction, + baseline: baselineMetric.value, + candidate: candidateMetric.value, + absoluteDelta, + percentDelta, + status: metricStatus(absoluteDelta, direction) + } +} + +function formatUnitLabel(unit) { + return unit === '' ? 'none' : unit +} + +function roundOneDecimal(value) { + return Math.round(value * 10) / 10 +} + +function metricStatus(absoluteDelta, direction) { + if (absoluteDelta === 0) { + return 'unchanged' + } + if (direction === 'higher-is-better') { + return absoluteDelta > 0 ? 'improved' : 'regressed' + } + return absoluteDelta < 0 ? 'improved' : 'regressed' +} + +export function formatBenchmarkComparisonMarkdown(comparison) { + const lines = [ + `# ${markdownText(comparison.title)}`, + '', + `Baseline: ${markdownText(comparison.baseline.label)} (${markdownText(comparison.baseline.path)})`, + `Candidate: ${markdownText(comparison.candidate.label)} (${markdownText(comparison.candidate.path)})`, + `Generated: ${comparison.createdAt}`, + '', + '| Metric | Baseline | Candidate | Delta | Delta % | Result |', + '|---|---:|---:|---:|---:|---|' + ] + for (const metric of comparison.metrics) { + lines.push( + `| ${markdownTableCell(metric.key)} | ${formatMetricValue(metric.baseline, metric.unit)} | ${formatMetricValue(metric.candidate, metric.unit)} | ${formatMetricValue(metric.absoluteDelta, metric.unit)} | ${formatPercent(metric.percentDelta)} | ${metric.status} |` + ) + } + if (comparison.skippedMetrics.length > 0) { + lines.push('', '## Skipped metrics') + for (const skippedMetric of comparison.skippedMetrics) { + lines.push(`- ${markdownText(skippedMetric.key)}: ${markdownText(skippedMetric.reason)}`) + } + } + return `${lines.join('\n')}\n` +} + +function markdownText(value) { + return String(value ?? '') + .replace(/\s*\r?\n\s*/g, ' ') + .replace(/[\\`*_{}<>()#+.!-]|\[|\]/g, '\\$&') +} + +function markdownTableCell(value) { + return markdownText(value).replaceAll('|', '\\|') +} + +function formatMetricValue(value, unit) { + return `${Number(value).toFixed(1)}${unit}` +} + +function formatPercent(value) { + return value == null ? '' : `${value.toFixed(1)}%` +} + +export function runBenchmarkComparisonCli(args = {}) { + const argv = args.argv ?? process.argv.slice(2) + const parsed = parseBenchmarkComparisonArgs(argv) + const comparison = compareBenchmarkArtifacts(parsed) + const markdown = formatBenchmarkComparisonMarkdown(comparison) + if (parsed.outputPath) { + mkdirSync(dirname(parsed.outputPath), { recursive: true }) + writeFileSync(parsed.outputPath, markdown) + } + if (parsed.jsonOutputPath) { + mkdirSync(dirname(parsed.jsonOutputPath), { recursive: true }) + writeFileSync(parsed.jsonOutputPath, `${JSON.stringify(comparison, null, 2)}\n`) + } + const stdout = args.stdout ?? process.stdout + stdout.write(markdown) + return comparison +} + +if (process.argv[1] != null && import.meta.url === pathToFileURL(process.argv[1]).href) { + try { + runBenchmarkComparisonCli() + } catch (error) { + console.error(error.message) + process.exit(1) + } +} diff --git a/config/scripts/summarize-terminal-perf-report.mjs b/config/scripts/summarize-terminal-perf-report.mjs index 04a80d81d..6d2adea54 100644 --- a/config/scripts/summarize-terminal-perf-report.mjs +++ b/config/scripts/summarize-terminal-perf-report.mjs @@ -1,5 +1,5 @@ -import { readFileSync } from 'node:fs' import { basename } from 'node:path' +import { collectTerminalPerfRows, readJsonReport } from './terminal-perf-report-annotations.mjs' const reportPaths = process.argv.slice(2) if (reportPaths[0] === '--') { @@ -13,55 +13,6 @@ if (reportPaths.length === 0) { process.exit(1) } -function readJsonReport(path) { - const raw = readFileSync(path, 'utf8') - const start = raw.indexOf('{') - const end = raw.lastIndexOf('}') - if (start === -1 || end <= start) { - throw new Error(`${path}: no JSON object found`) - } - return JSON.parse(raw.slice(start, end + 1)) -} - -function parseAnnotationDescription(description) { - const values = {} - for (const part of description.split(/\s+/)) { - const index = part.indexOf('=') - if (index === -1) { - continue - } - values[part.slice(0, index)] = part.slice(index + 1) - } - return values -} - -function collectTerminalPerfRows(report, source) { - const rows = [] - const visitSuite = (suite) => { - for (const spec of suite.specs ?? []) { - for (const test of spec.tests ?? []) { - for (const annotation of test.annotations ?? []) { - if (!annotation.type.startsWith('opencode-')) { - continue - } - rows.push({ - source, - scenario: annotation.type, - ...parseAnnotationDescription(annotation.description ?? '') - }) - } - } - } - for (const child of suite.suites ?? []) { - visitSuite(child) - } - } - for (const suite of report.suites ?? []) { - visitSuite(suite) - } - return rows -} - function markdownCell(value) { return String(value ?? '').replaceAll('|', '\\|') } diff --git a/config/scripts/summarize-terminal-perf-report.test.mjs b/config/scripts/summarize-terminal-perf-report.test.mjs new file mode 100644 index 000000000..2a1f16ec1 --- /dev/null +++ b/config/scripts/summarize-terminal-perf-report.test.mjs @@ -0,0 +1,63 @@ +import { execFileSync } from 'node:child_process' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' + +const scriptPath = 'config/scripts/summarize-terminal-perf-report.mjs' +const tempDirs = [] + +function writeReport() { + const dir = mkdtempSync(join(tmpdir(), 'orca-terminal-perf-summary-')) + tempDirs.push(dir) + const reportPath = join(dir, 'report.json') + writeFileSync( + reportPath, + JSON.stringify({ + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'opencode-scale', + description: 'panes=50 frames=60 median=12.3ms rendererQueuedChars=1000' + }, + { + type: 'browser-unrelated', + description: 'panes=1 median=999.0ms' + } + ] + } + ] + } + ] + } + ] + }) + ) + return reportPath +} + +afterEach(() => { + while (tempDirs.length > 0) { + rmSync(tempDirs.pop(), { force: true, recursive: true }) + } +}) + +describe('summarize-terminal-perf-report', () => { + it('prints only OpenCode terminal perf annotation rows', () => { + const output = execFileSync(process.execPath, [scriptPath, writeReport()], { + cwd: process.cwd(), + encoding: 'utf8' + }) + + expect(output).toContain('| Source | Scenario | Panes | Frames | Median |') + expect(output).toContain('| report.json | opencode-scale | 50 | 60 | 12.3ms |') + expect(output).toContain('1000') + expect(output).not.toContain('browser-unrelated') + expect(output).not.toContain('999.0ms') + }) +}) diff --git a/config/scripts/terminal-perf-report-annotations.mjs b/config/scripts/terminal-perf-report-annotations.mjs new file mode 100644 index 000000000..aca4eca60 --- /dev/null +++ b/config/scripts/terminal-perf-report-annotations.mjs @@ -0,0 +1,53 @@ +import { readFileSync } from 'node:fs' + +export function readJsonReport(path) { + const raw = readFileSync(path, 'utf8') + const start = raw.indexOf('{') + const end = raw.lastIndexOf('}') + if (start === -1 || end <= start) { + throw new Error(`${path}: no JSON object found`) + } + return JSON.parse(raw.slice(start, end + 1)) +} + +export function parseAnnotationDescription(description) { + const values = {} + for (const part of description.split(/\s+/)) { + const index = part.indexOf('=') + if (index === -1) { + continue + } + values[part.slice(0, index)] = part.slice(index + 1) + } + return values +} + +export function collectTerminalPerfRows(report, source, options = {}) { + const { typePrefix = 'opencode-' } = options + const rows = [] + const visitSuite = (suite) => { + for (const spec of suite.specs ?? []) { + for (const test of spec.tests ?? []) { + for (const annotation of test.annotations ?? []) { + if (!annotation.type.startsWith(typePrefix)) { + continue + } + rows.push({ + ...parseAnnotationDescription(annotation.description ?? ''), + // Why: annotation descriptions are artifact-controlled; keep the + // trusted report source and annotation type from being relabeled. + source, + scenario: annotation.type + }) + } + } + } + for (const child of suite.suites ?? []) { + visitSuite(child) + } + } + for (const suite of report.suites ?? []) { + visitSuite(suite) + } + return rows +} diff --git a/config/scripts/terminal-perf-report-annotations.test.mjs b/config/scripts/terminal-perf-report-annotations.test.mjs new file mode 100644 index 000000000..82f4be5ea --- /dev/null +++ b/config/scripts/terminal-perf-report-annotations.test.mjs @@ -0,0 +1,144 @@ +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { + collectTerminalPerfRows, + parseAnnotationDescription, + readJsonReport +} from './terminal-perf-report-annotations.mjs' + +const tempDirs = [] + +function makeReportPath(content) { + const dir = mkdtempSync(join(tmpdir(), 'orca-terminal-perf-annotations-')) + tempDirs.push(dir) + const reportPath = join(dir, 'report.json') + writeFileSync(reportPath, content) + return reportPath +} + +function makeNestedReport() { + return { + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'browser-unrelated', + description: 'median=999.0ms' + } + ] + } + ] + } + ], + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'opencode-scale', + description: 'panes=50 median=12.3ms ignored-token rendererQueuedChars=1000' + }, + { + type: 'terminal-scale', + description: 'panes=25 median=7.0ms' + } + ] + } + ] + } + ] + } + ] + } + ] + } +} + +afterEach(() => { + while (tempDirs.length > 0) { + rmSync(tempDirs.pop(), { force: true, recursive: true }) + } +}) + +describe('terminal perf report annotations', () => { + it('parses key-value annotation description segments only', () => { + expect(parseAnnotationDescription('panes=50 no-equals median=12.3ms name=a=b')).toEqual({ + median: '12.3ms', + name: 'a=b', + panes: '50' + }) + }) + + it('reads JSON reports surrounded by noisy process output', () => { + const reportPath = makeReportPath( + `noise before\n${JSON.stringify({ suites: [] })}\nnoise after` + ) + + expect(readJsonReport(reportPath)).toEqual({ suites: [] }) + }) + + it('collects nested OpenCode annotations by default', () => { + expect(collectTerminalPerfRows(makeNestedReport(), 'report.json')).toEqual([ + { + median: '12.3ms', + panes: '50', + rendererQueuedChars: '1000', + scenario: 'opencode-scale', + source: 'report.json' + } + ]) + }) + + it('keeps trusted source and scenario fields when descriptions contain matching keys', () => { + const report = { + suites: [ + { + specs: [ + { + tests: [ + { + annotations: [ + { + type: 'opencode-scale', + description: 'source=spoofed.json scenario=spoofed median=12.3ms' + } + ] + } + ] + } + ] + } + ] + } + + expect(collectTerminalPerfRows(report, 'report.json')).toEqual([ + { + median: '12.3ms', + scenario: 'opencode-scale', + source: 'report.json' + } + ]) + }) + + it('supports a custom annotation type prefix', () => { + expect( + collectTerminalPerfRows(makeNestedReport(), 'report.json', { typePrefix: 'terminal-' }) + ).toEqual([ + { + median: '7.0ms', + panes: '25', + scenario: 'terminal-scale', + source: 'report.json' + } + ]) + }) +}) diff --git a/package.json b/package.json index 9ab4f4fef..818222852 100644 --- a/package.json +++ b/package.json @@ -82,7 +82,10 @@ "test:e2e:ssh-codex-artifacts-repro": "node config/scripts/run-ssh-codex-artifacts-repro-e2e.mjs", "test:e2e:headful": "pnpm run ensure:electron-runtime && npx playwright test --config tests/playwright.config.ts --project electron-headful", "test:e2e:computer": "vitest run --config tests/e2e/vitest.config.ts", - "bench:idle-cpu": "pnpm run ensure:electron-runtime && node config/scripts/run-idle-cpu-benchmark.mjs" + "bench:idle-cpu": "pnpm run ensure:electron-runtime && node config/scripts/run-idle-cpu-benchmark.mjs", + "bench:startup": "pnpm run ensure:electron-runtime && node tools/benchmarks/startup-time-bench.mjs", + "bench:daemon-coldstart": "pnpm run ensure:electron-runtime && node tools/benchmarks/daemon-coldstart-bench.mjs", + "bench:compare": "node config/scripts/compare-benchmark-artifacts.mjs" }, "dependencies": { "@electron-toolkit/preload": "^3.0.2", diff --git a/src/main/observability/instrumentation.test.ts b/src/main/observability/instrumentation.test.ts new file mode 100644 index 000000000..2f7d1da05 --- /dev/null +++ b/src/main/observability/instrumentation.test.ts @@ -0,0 +1,169 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { _resetTracerForTests, setActiveSink, type TracerSink } from './tracer' +import { + _gitSpanSamplingBucketCountForTests, + _resetGitSpanSamplingForTests, + withGitSpan +} from './instrumentation' + +type SpanRecord = { + readonly name: string + readonly durationMs: number + readonly attributes: Record<string, unknown> + readonly exit: { readonly _tag: string; readonly cause?: string } +} + +type CapturedSink = TracerSink & { + readonly records: SpanRecord[] +} + +function isSpanRecord(record: unknown): record is SpanRecord { + return ( + record !== null && + typeof record === 'object' && + 'name' in record && + typeof record.name === 'string' && + 'durationMs' in record && + typeof record.durationMs === 'number' && + 'attributes' in record && + record.attributes !== null && + typeof record.attributes === 'object' && + 'exit' in record && + record.exit !== null && + typeof record.exit === 'object' && + '_tag' in record.exit && + typeof record.exit._tag === 'string' + ) +} + +function makeCapturingSink(): CapturedSink { + const records: SpanRecord[] = [] + return { + records, + push(record) { + if (!isSpanRecord(record)) { + throw new Error('expected span record') + } + records.push(record) + }, + flush() { + /* no-op */ + }, + close() { + /* no-op */ + } + } +} + +let sink: CapturedSink +let nowMs = 1_700_000_000_000 + +async function runGitSpan( + meta: { args: readonly string[]; cwd?: string }, + durationMs: number, + fail = false +) { + vi.setSystemTime(nowMs) + const promise = withGitSpan(meta, async () => { + vi.setSystemTime(nowMs + durationMs) + if (fail) { + throw new Error('git failed') + } + return 'ok' + }) + nowMs += durationMs + 1 + return await promise +} + +beforeEach(() => { + vi.useFakeTimers() + nowMs = 1_700_000_000_000 + _resetGitSpanSamplingForTests() + sink = makeCapturingSink() + setActiveSink(sink) +}) + +afterEach(() => { + vi.useRealTimers() + _resetGitSpanSamplingForTests() + _resetTracerForTests() +}) + +describe('withGitSpan sampling', () => { + it('bounds fast successful repeated git spans by subcommand and cwd while preserving important spans', async () => { + for (let i = 0; i < 10_000; i++) { + await runGitSpan({ args: ['status', '--short'], cwd: '/repo' }, 5) + } + + const repeatedFastSuccesses = sink.records.filter( + (record) => + record.name === 'git.exec' && + record.exit._tag === 'Success' && + record.attributes['git.subcommand'] === 'status' && + record.attributes.cwd === '/repo' && + record.durationMs < 250 + ) + expect(repeatedFastSuccesses.length).toBeGreaterThan(0) + expect(repeatedFastSuccesses.length).toBeLessThan(200) + + await expect(runGitSpan({ args: ['status'], cwd: '/repo' }, 5, true)).rejects.toThrow( + 'git failed' + ) + await runGitSpan({ args: ['status'], cwd: '/repo' }, 275) + await runGitSpan({ args: ['branch'], cwd: '/repo' }, 5) + await runGitSpan({ args: ['status'], cwd: '/other-repo' }, 5) + + expect( + sink.records.some( + (record) => + record.exit._tag === 'Failure' && + record.attributes['git.subcommand'] === 'status' && + record.attributes.cwd === '/repo' + ) + ).toBe(true) + expect( + sink.records.some( + (record) => + record.exit._tag === 'Success' && + record.durationMs >= 250 && + record.attributes['git.subcommand'] === 'status' && + record.attributes.cwd === '/repo' + ) + ).toBe(true) + expect( + sink.records.some( + (record) => + record.exit._tag === 'Success' && + record.attributes['git.subcommand'] === 'branch' && + record.attributes.cwd === '/repo' + ) + ).toBe(true) + expect( + sink.records.some( + (record) => + record.exit._tag === 'Success' && + record.attributes['git.subcommand'] === 'status' && + record.attributes.cwd === '/other-repo' + ) + ).toBe(true) + }) + it('parses git subcommands after global options without changing arg count', async () => { + await runGitSpan({ args: ['-c', 'core.quotePath=false', 'status', '--short'], cwd: '/repo' }, 5) + + expect(sink.records[0]?.attributes['git.subcommand']).toBe('status') + expect(sink.records[0]?.attributes['git.arg_count']).toBe(4) + }) + + it('prunes stale git sampling buckets and caps unique cwd buckets', async () => { + for (let i = 0; i < 700; i++) { + await runGitSpan({ args: ['status'], cwd: `/repo-${i}` }, 5) + } + + expect(_gitSpanSamplingBucketCountForTests()).toBeLessThanOrEqual(512) + + nowMs += 60_000 + await runGitSpan({ args: ['status'], cwd: '/fresh-repo' }, 5) + + expect(_gitSpanSamplingBucketCountForTests()).toBe(1) + }) +}) diff --git a/src/main/observability/instrumentation.ts b/src/main/observability/instrumentation.ts index 3651a355e..6030c8c77 100644 --- a/src/main/observability/instrumentation.ts +++ b/src/main/observability/instrumentation.ts @@ -23,29 +23,164 @@ import { withSpan, type ActiveSpan } from './tracer' +const GIT_FAST_SUCCESS_THRESHOLD_MS = 250 +const GIT_FAST_SUCCESS_WINDOW_MS = 60_000 +const GIT_FAST_SUCCESS_BUDGET_PER_WINDOW = 60 +const GIT_SAMPLING_MAX_BUCKETS = 512 + +// Why: trace captures showed `git status --short` bursts dominating payloads. +// Keep enough fast successes for timing shape while bounding memory and volume. +const GIT_GLOBAL_OPTIONS_WITH_OPERAND = new Set([ + '-c', + '-C', + '--git-dir', + '--work-tree', + '--config-env', + '--namespace', + '--exec-path', + '--super-prefix', + '--pathspec-from-file' +]) +const GIT_GLOBAL_FLAGS = new Set([ + '--bare', + '--no-pager', + '--paginate', + '--literal-pathspecs', + '--glob-pathspecs', + '--noglob-pathspecs', + '--icase-pathspecs', + '--no-optional-locks', + '--pathspec-file-nul' +]) + +type GitSamplingBucket = { + windowStartMs: number + emitted: number +} + +const gitSamplingBuckets = new Map<string, GitSamplingBucket>() + +function gitSubcommandFromArgs(args: readonly string[]): string { + for (let index = 0; index < args.length; index++) { + const arg = args[index] + if (!arg) { + continue + } + if (arg === '--') { + return '<none>' + } + if (GIT_GLOBAL_OPTIONS_WITH_OPERAND.has(arg)) { + index += 1 + continue + } + if ( + arg.startsWith('--git-dir=') || + arg.startsWith('--work-tree=') || + arg.startsWith('--config-env=') || + arg.startsWith('--namespace=') || + arg.startsWith('--exec-path=') || + arg.startsWith('--super-prefix=') || + arg.startsWith('--pathspec-from-file=') || + (arg.startsWith('-c') && arg.length > 2) || + (arg.startsWith('-C') && arg.length > 2) + ) { + continue + } + if (GIT_GLOBAL_FLAGS.has(arg)) { + continue + } + if (arg.startsWith('-')) { + continue + } + return arg + } + return '<none>' +} + +function pruneGitSamplingBuckets(nowMs: number): void { + for (const [key, bucket] of gitSamplingBuckets) { + if (nowMs - bucket.windowStartMs >= GIT_FAST_SUCCESS_WINDOW_MS) { + gitSamplingBuckets.delete(key) + } + } + while (gitSamplingBuckets.size > GIT_SAMPLING_MAX_BUCKETS) { + let oldestKey: string | undefined + let oldestWindowStartMs = Number.POSITIVE_INFINITY + for (const [key, bucket] of gitSamplingBuckets) { + if (bucket.windowStartMs < oldestWindowStartMs) { + oldestKey = key + oldestWindowStartMs = bucket.windowStartMs + } + } + if (oldestKey === undefined) { + return + } + gitSamplingBuckets.delete(oldestKey) + } +} + +function gitSamplingKey(meta: GitSpanArgs): string { + return `${gitSubcommandFromArgs(meta.args)}\u0000${meta.cwd ?? '<none>'}` +} + +function shouldRecordGitSpan( + meta: GitSpanArgs, + record: { durationMs: number; startTimeUnixNano: string; exit: { _tag: string } } +): boolean { + if (record.exit._tag !== 'Success' || record.durationMs >= GIT_FAST_SUCCESS_THRESHOLD_MS) { + return true + } + + const nowMs = Number(BigInt(record.startTimeUnixNano) / 1_000_000n) + pruneGitSamplingBuckets(nowMs) + const key = gitSamplingKey(meta) + const bucket = gitSamplingBuckets.get(key) + if (!bucket) { + gitSamplingBuckets.set(key, { windowStartMs: nowMs, emitted: 1 }) + pruneGitSamplingBuckets(nowMs) + return true + } + if (bucket.emitted < GIT_FAST_SUCCESS_BUDGET_PER_WINDOW) { + bucket.emitted += 1 + return true + } + return false +} + +function addGitAttributes(span: ActiveSpan, meta: GitSpanArgs): void { + span.setAttribute('git.subcommand', gitSubcommandFromArgs(meta.args)) + // Why: git args can contain commit messages, branch names, remotes, or + // paths. Keep cardinality without copying user-authored content. + span.setAttribute('git.arg_count', meta.args.length) + if (meta.cwd) { + span.setAttribute('cwd', meta.cwd) + } +} + +export function _resetGitSpanSamplingForTests(): void { + gitSamplingBuckets.clear() +} + +export function _gitSpanSamplingBucketCountForTests(): number { + return gitSamplingBuckets.size +} + export type GitSpanArgs = { readonly args: readonly string[] readonly cwd?: string } -/** Wrap a git execution in a `git.exec` span. The first argument typically - * is the subcommand (`status`, `clone`, `pull`); promoting it to its own - * attribute makes it grep-friendly without pulling the full args array - * into a single comma-joined string in dashboards. */ +/** Wrap a git execution in a `git.exec` span. Git accepts global options before + * the subcommand; promoting the parsed command to its own attribute makes it + * grep-friendly without copying the full args array into dashboards. */ export async function withGitSpan<T>(meta: GitSpanArgs, fn: () => Promise<T>): Promise<T> { return withSpan( 'git.exec', async (span) => { - span.setAttribute('git.subcommand', meta.args[0] ?? '<none>') - // Why: git args can contain commit messages, branch names, remotes, or - // paths. Keep cardinality without copying user-authored content. - span.setAttribute('git.arg_count', meta.args.length) - if (meta.cwd) { - span.setAttribute('cwd', meta.cwd) - } + addGitAttributes(span, meta) return await fn() }, - { attributes: { kind: 'git' } } + { attributes: { kind: 'git' }, shouldRecord: (record) => shouldRecordGitSpan(meta, record) } ) } diff --git a/src/main/observability/tracer.ts b/src/main/observability/tracer.ts index bb1739acf..3ef93449c 100644 --- a/src/main/observability/tracer.ts +++ b/src/main/observability/tracer.ts @@ -128,10 +128,16 @@ export function getActiveSpanContext(): SpanContext | undefined { * This is the function 90% of call sites should reach for: it keeps span * lifetime scoped to the async work it measures. */ +type SpanRecordDecision = (record: RedactableSpan) => boolean + export async function withSpan<T>( name: string, fn: (span: ActiveSpan) => Promise<T> | T, - options?: { kind?: string; attributes?: Record<string, unknown> } + options?: { + kind?: string + attributes?: Record<string, unknown> + shouldRecord?: SpanRecordDecision + } ): Promise<T> { const span = startSpan(name, options) try { @@ -157,7 +163,11 @@ export async function withSpan<T>( */ export function startSpan( name: string, - options?: { kind?: string; attributes?: Record<string, unknown> } + options?: { + kind?: string + attributes?: Record<string, unknown> + shouldRecord?: SpanRecordDecision + } ): ActiveSpan { if (!activeSink) { return noopSpan @@ -203,6 +213,10 @@ export function startSpan( exit } + if (options?.shouldRecord && !options.shouldRecord(record)) { + return + } + const redacted = redactSpan(record, 'client') // Wrap in a `type: 'effect-span'` envelope so the NDJSON file is // compatible with Effect-style span output. Effect-oriented consumers diff --git a/src/renderer/src/components/right-sidebar/SourceControl.compare-summary.test.ts b/src/renderer/src/components/right-sidebar/SourceControl.compare-summary.test.ts index 72d8bac88..69a1f3646 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.compare-summary.test.ts +++ b/src/renderer/src/components/right-sidebar/SourceControl.compare-summary.test.ts @@ -1,9 +1,12 @@ import { describe, expect, it, vi } from 'vitest' import { + BRANCH_REFRESH_INTERVAL_MS, CompareSummary, CompareSummaryToolbarButton, + refreshSourceControlAfterRemoteAction, resolveSourceControlBaseRef, resolveSourceControlPickerBaseRef, + shouldRefreshBranchCompareForStatusHead, shouldShowCompareSummary } from './SourceControl' import type { GitBranchCompareSummary } from '../../../../shared/types' @@ -272,4 +275,63 @@ describe('SourceControl compare summary', () => { expect(collectCompareSummaryToolbarLabels(node)).toEqual(['Change base ref', 'Retry']) }) + + it('keeps a 30 second branch compare fallback refresh', () => { + expect(BRANCH_REFRESH_INTERVAL_MS).toBe(30_000) + }) + + it('refreshes branch compare when git status observes a new head for the same base', () => { + expect( + shouldRefreshBranchCompareForStatusHead( + { baseRef: 'origin/main', statusHead: 'old-head', worktreeId: 'wt-1' }, + { baseRef: 'origin/main', statusHead: 'new-head', worktreeId: 'wt-1' } + ) + ).toBe(true) + }) + + it('does not refresh branch compare for initial, unknown, or unrelated status heads', () => { + expect( + shouldRefreshBranchCompareForStatusHead(null, { + baseRef: 'origin/main', + statusHead: 'head', + worktreeId: 'wt-1' + }) + ).toBe(false) + expect( + shouldRefreshBranchCompareForStatusHead( + { baseRef: 'origin/main', statusHead: 'old-head', worktreeId: 'wt-1' }, + { baseRef: 'origin/main', statusHead: null, worktreeId: 'wt-1' } + ) + ).toBe(false) + expect( + shouldRefreshBranchCompareForStatusHead( + { baseRef: 'origin/main', statusHead: 'old-head', worktreeId: 'wt-1' }, + { baseRef: 'origin/main', statusHead: 'new-head', worktreeId: 'wt-2' } + ) + ).toBe(false) + expect( + shouldRefreshBranchCompareForStatusHead( + { baseRef: 'origin/main', statusHead: 'old-head', worktreeId: 'wt-1' }, + { baseRef: 'origin/release', statusHead: 'new-head', worktreeId: 'wt-1' } + ) + ).toBe(false) + }) + + it('keeps immediate refresh paths for remote actions', () => { + const refreshGitStatus = vi.fn(async () => {}) + const refreshBranchCompare = vi.fn(async () => {}) + const refreshGitHistory = vi.fn(async () => {}) + + refreshSourceControlAfterRemoteAction({ + refreshGitStatus, + refreshBranchCompare, + refreshGitHistory + }) + + expect(refreshGitStatus).toHaveBeenCalledTimes(1) + expect(refreshBranchCompare).toHaveBeenCalledTimes(1) + expect(refreshGitHistory).toHaveBeenCalledTimes(1) + // Direct commit, manual, retry, and base-ref refresh paths remain component-level + // behavior covered by the existing UI wiring; keep this test on the pure helper. + }) }) diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 30a56cc55..245555f2d 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -466,7 +466,9 @@ const CONFLICTS_SECTION_LABEL = { fallback: 'Conflicts' } -const BRANCH_REFRESH_INTERVAL_MS = 5000 +// Why: 5s branch compare polling churned git subprocesses in large repos. +// Explicit commit, remote, manual, and base-ref refresh paths still run immediately. +export const BRANCH_REFRESH_INTERVAL_MS = 30_000 // Why: row action buttons host Radix Tooltip triggers. Keeping the overlay // measurable prevents transient top-left tooltip placement during hover. const SOURCE_CONTROL_ROW_ACTION_OVERLAY_CLASS = @@ -742,6 +744,9 @@ function SourceControlInner(): React.JSX.Element { ? (s.gitStatusByWorktree[activeWorktreeId] ?? EMPTY_GIT_STATUS_ENTRIES) : EMPTY_GIT_STATUS_ENTRIES ) + const activeGitStatusHead = useAppStore((s) => + activeWorktreeId ? (s.gitStatusHeadByWorktree?.[activeWorktreeId] ?? null) : null + ) const repositoryHuge = useAppStore((s) => activeWorktreeId ? s.gitStatusHugeByWorktree?.[activeWorktreeId] : undefined ) @@ -1907,7 +1912,7 @@ function SourceControlInner(): React.JSX.Element { // // Then fire-and-forget refreshBranchCompare so the "Committed on // Branch" section repopulates as soon as the IPC returns instead of - // waiting up to 5 seconds for the next poll. Unawaited on purpose: + // waiting for the next poll. Unawaited on purpose: // compound flows (runCompoundCommitAction) need handleCommit to // resolve immediately so the push step starts without delay. Errors // here are best-effort — the polling tick will retry. @@ -4429,6 +4434,7 @@ function SourceControlInner(): React.JSX.Element { const branchCompareRerunRef = useRef(false) const branchCompareRunPromiseRef = useRef<Promise<void> | null>(null) const refreshBranchCompareRef = useRef<() => Promise<void>>(async () => {}) + const branchCompareStatusHeadRef = useRef<BranchCompareStatusHeadSnapshot | null>(null) const runBranchCompare = useCallback(async () => { if (!activeWorktreeId || !worktreePath || !effectiveBaseRef || isFolder) { @@ -4444,8 +4450,8 @@ function SourceControlInner(): React.JSX.Element { // getBaseRefDefault corrected a stale cross-repo value). Polling retries // — whether the previous result was 'ready' *or* an error — keep the // current UI visible until the new IPC result arrives. Resetting to - // 'loading' on every 5-second poll when the compare is in an error state - // caused a visible loading→error→loading→error flicker. + // 'loading' on every poll when the compare is in an error state caused a + // visible loading→error→loading→error flicker. const baseRefChanged = existingSummary && existingSummary.baseRef !== effectiveBaseRef const shouldResetToLoading = !existingSummary || baseRefChanged if (shouldResetToLoading) { @@ -4503,8 +4509,8 @@ function SourceControlInner(): React.JSX.Element { branchCompareInFlightRef.current = true const runPromise = (async (): Promise<void> => { - // Why: branch compare shells out to git on a timer and can exceed the - // 5s poll interval on large repos. Keep one compare chain in flight and + // Why: branch compare shells out to git from both event-driven refreshes + // and the fallback timer. Keep one compare chain in flight and // collapse skipped ticks into one trailing refresh instead of stacking // subprocesses while preserving the await contract for direct callers. try { @@ -4602,12 +4608,36 @@ function SourceControlInner(): React.JSX.Element { useEffect(() => { if (!activeWorktreeId || !worktreePath || !isBranchVisible || !effectiveBaseRef || isFolder) { + branchCompareStatusHeadRef.current = null return } - // Why: branch compare shells out to git every tick. The panel only needs - // background freshness while Orca is visible; hidden-window time should not - // burn subprocess work or timer wakeups. + const current = { + baseRef: effectiveBaseRef, + statusHead: activeGitStatusHead, + worktreeId: activeWorktreeId + } + const previous = branchCompareStatusHeadRef.current + branchCompareStatusHeadRef.current = current + if (shouldRefreshBranchCompareForStatusHead(previous, current)) { + void refreshBranchCompareRef.current() + } + }, [ + activeGitStatusHead, + activeWorktreeId, + effectiveBaseRef, + isBranchVisible, + isFolder, + worktreePath + ]) + + useEffect(() => { + if (!activeWorktreeId || !worktreePath || !isBranchVisible || !effectiveBaseRef || isFolder) { + return + } + + // Why: git-status HEAD changes refresh branch compare immediately. Keep a + // visible-window fallback for base refs or remote updates that do not move HEAD. return installWindowVisibilityInterval({ run: () => void refreshBranchCompareRef.current(), intervalMs: BRANCH_REFRESH_INTERVAL_MS @@ -6805,6 +6835,25 @@ export function CommitArea({ ) } +type BranchCompareStatusHeadSnapshot = { + baseRef: string + statusHead: string | null + worktreeId: string +} + +export function shouldRefreshBranchCompareForStatusHead( + previous: BranchCompareStatusHeadSnapshot | null, + current: BranchCompareStatusHeadSnapshot +): boolean { + return ( + current.statusHead !== null && + previous !== null && + previous.worktreeId === current.worktreeId && + previous.baseRef === current.baseRef && + previous.statusHead !== current.statusHead + ) +} + export function shouldShowCompareSummary(summary: GitBranchCompareSummary | null): boolean { if (!summary || summary.status === 'loading') { return true