diff --git a/src/action/main.ts b/src/action/main.ts index 58d41c0..b30bfe0 100644 --- a/src/action/main.ts +++ b/src/action/main.ts @@ -179,39 +179,36 @@ async function postReviewToGitHub( return; } + // Only post PR reviews with inline comments - skip standalone summary comments + // as they add noise without providing actionable inline feedback + if (!result.review) { + return; + } + const { owner, name: repo } = context.repository; const pullNumber = context.pullRequest.number; const commitId = context.pullRequest.headSha; - if (result.review) { - const reviewComments = result.review.comments - .filter((c): c is typeof c & { path: string; line: number } => Boolean(c.path && c.line)) - .map((c) => ({ - path: c.path, - line: c.line, - side: c.side ?? ('RIGHT' as const), - body: c.body, - start_line: c.start_line, - start_side: c.start_line ? c.start_side ?? ('RIGHT' as const) : undefined, - })); - - await octokit.pulls.createReview({ - owner, - repo, - pull_number: pullNumber, - commit_id: commitId, - event: result.review.event, - body: result.review.body, - comments: reviewComments, - }); - } else { - await octokit.issues.createComment({ - owner, - repo, - issue_number: pullNumber, - body: result.summaryComment, - }); - } + const reviewComments = result.review.comments + .filter((c): c is typeof c & { path: string; line: number } => Boolean(c.path && c.line)) + .map((c) => ({ + path: c.path, + line: c.line, + side: c.side ?? ('RIGHT' as const), + body: c.body, + start_line: c.start_line, + start_side: c.start_line ? c.start_side ?? ('RIGHT' as const) : undefined, + })); + + await octokit.pulls.createReview({ + owner, + repo, + pull_number: pullNumber, + commit_id: commitId, + event: result.review.event, + body: result.review.body, + comments: reviewComments, + }); } /** @@ -621,6 +618,7 @@ async function run(): Promise { ? reports.reduce((sum, r) => sum + (r.durationMs ?? 0), 0) : undefined, totalUsage: aggregateUsage(reports), + findings: reports.flatMap((r) => r.findings), skillResults: results.map((r) => ({ name: r.triggerName, findingCount: r.report?.findings.length ?? 0, diff --git a/src/output/github-checks.ts b/src/output/github-checks.ts index efd848c..39b034c 100644 --- a/src/output/github-checks.ts +++ b/src/output/github-checks.ts @@ -1,7 +1,7 @@ import type { Octokit } from '@octokit/rest'; import { SEVERITY_ORDER, filterFindingsBySeverity } from '../types/index.js'; import type { Severity, SeverityThreshold, Finding, SkillReport, UsageStats } from '../types/index.js'; -import { formatStatsCompact, formatDuration, formatCost, formatTokens, countBySeverity } from '../cli/output/formatters.js'; +import { formatDuration, formatCost, formatTokens } from '../cli/output/formatters.js'; /** * GitHub Check annotation for inline code comments. @@ -47,6 +47,8 @@ export interface CoreCheckSummaryData { findingsBySeverity: Record; totalDurationMs?: number; totalUsage?: UsageStats; + /** All findings from all skills */ + findings: Finding[]; skillResults: { name: string; findingCount: number; @@ -187,8 +189,11 @@ export async function updateSkillCheck( // Annotations are filtered by commentOn threshold const annotations = findingsToAnnotations(report.findings, options.commentOn); - const findingCounts = countBySeverity(report.findings); - const summary = buildSkillSummary(report, findingCounts); + const summary = buildSkillSummary(report); + + const title = report.findings.length === 0 + ? 'No issues' + : `${report.findings.length} issue${report.findings.length === 1 ? '' : 's'}`; await octokit.checks.update({ owner: options.owner, @@ -198,7 +203,7 @@ export async function updateSkillCheck( conclusion, completed_at: new Date().toISOString(), output: { - title: `${report.findings.length} finding${report.findings.length === 1 ? '' : 's'}`, + title, summary, annotations, }, @@ -265,6 +270,10 @@ export async function updateCoreCheck( ): Promise { const summary = buildCoreSummary(summaryData); + const title = summaryData.totalFindings === 0 + ? 'No issues' + : `${summaryData.totalFindings} issue${summaryData.totalFindings === 1 ? '' : 's'}`; + await octokit.checks.update({ owner: options.owner, repo: options.repo, @@ -273,131 +282,170 @@ export async function updateCoreCheck( conclusion, completed_at: new Date().toISOString(), output: { - title: `${summaryData.totalFindings} finding${summaryData.totalFindings === 1 ? '' : 's'} across ${summaryData.totalSkills} skill${summaryData.totalSkills === 1 ? '' : 's'}`, + title, summary, }, }); } -/** - * Render a markdown severity table from counts. - */ -function renderSeverityTable(counts: Record): string[] { - const severities: Severity[] = ['critical', 'high', 'medium', 'low', 'info']; - const lines: string[] = [ - '### Findings by Severity', - '', - '| Severity | Count |', - '|----------|-------|', - ]; - - for (const severity of severities) { - if (counts[severity] > 0) { - lines.push(`| ${severity} | ${counts[severity]} |`); - } - } - - return lines; -} - /** * Build the summary markdown for a skill check. */ -function buildSkillSummary( - report: SkillReport, - findingCounts: Record -): string { +function buildSkillSummary(report: SkillReport): string { const lines: string[] = [report.summary, '']; if (report.findings.length === 0) { - lines.push('No findings.'); + lines.push('No issues found.'); } else { - lines.push(...renderSeverityTable(findingCounts)); + // Sort findings by severity + const sortedFindings = [...report.findings].sort( + (a, b) => SEVERITY_ORDER[a.severity] - SEVERITY_ORDER[b.severity] + ); + + // Group findings by severity + const findingsBySeverity = new Map(); + for (const finding of sortedFindings) { + const existing = findingsBySeverity.get(finding.severity) ?? []; + existing.push(finding); + findingsBySeverity.set(finding.severity, existing); + } + + const severityOrder: Severity[] = ['critical', 'high', 'medium', 'low', 'info']; + for (const severity of severityOrder) { + const findings = findingsBySeverity.get(severity); + if (!findings?.length) continue; + + const label = severity.charAt(0).toUpperCase() + severity.slice(1); + lines.push(`### ${label}`, ''); + + for (const finding of findings) { + const location = finding.location ? ` - ${formatLocation(finding.location)}` : ''; + lines.push('
'); + lines.push(`${finding.title}${location}`, ''); + lines.push(finding.description, ''); + lines.push('
', ''); + } + } } // Add stats footer if available - const statsLine = formatStatsCompact(report.durationMs, report.usage); - if (statsLine) { - lines.push('', '---', `${statsLine}`); + if (report.durationMs !== undefined || report.usage) { + const statsParts: string[] = []; + if (report.durationMs !== undefined) { + statsParts.push(`**Duration:** ${formatDuration(report.durationMs)}`); + } + if (report.usage) { + const totalInput = report.usage.inputTokens + (report.usage.cacheReadInputTokens ?? 0); + statsParts.push(`**Tokens:** ${formatTokens(totalInput)} in / ${formatTokens(report.usage.outputTokens)} out`); + statsParts.push(`**Cost:** ${formatCost(report.usage.costUSD)}`); + } + lines.push('---', statsParts.join(' · ')); } return lines.join('\n'); } /** - * Map check conclusion to display icon. + * Format a file location as a markdown code span. */ -function conclusionIcon(conclusion: CheckConclusion): string { - switch (conclusion) { - case 'success': - return ':white_check_mark:'; - case 'failure': - return ':x:'; - case 'neutral': - case 'cancelled': - return ':warning:'; - } +function formatLocation(location: { path: string; startLine: number; endLine?: number }): string { + const { path, startLine, endLine } = location; + const lineRange = endLine && endLine !== startLine ? `${startLine}-${endLine}` : `${startLine}`; + return `\`${path}:${lineRange}\``; } +/** Maximum findings to show in the summary */ +const MAX_SUMMARY_FINDINGS = 10; + /** * Build the summary markdown for the core warden check. */ function buildCoreSummary(data: CoreCheckSummaryData): string { - const skillPlural = data.totalSkills === 1 ? '' : 's'; - const findingPlural = data.totalFindings === 1 ? '' : 's'; - const lines: string[] = [ - `Analyzed ${data.totalSkills} skill${skillPlural}, found ${data.totalFindings} total finding${findingPlural}.`, - '', - ]; + const lines: string[] = []; - // Add aggregate stats line if available - const hasStats = data.totalDurationMs !== undefined || data.totalUsage; - if (hasStats) { - const statsParts: string[] = []; - if (data.totalDurationMs !== undefined) { - statsParts.push(`⏱ **${formatDuration(data.totalDurationMs)}**`); + // Sort findings by severity and take top N + const sortedFindings = [...data.findings].sort( + (a, b) => SEVERITY_ORDER[a.severity] - SEVERITY_ORDER[b.severity] + ); + const topFindings = sortedFindings.slice(0, MAX_SUMMARY_FINDINGS); + + // Show findings grouped by severity, each in a collapsible details + if (topFindings.length > 0) { + const findingsBySeverity = new Map(); + for (const finding of topFindings) { + const existing = findingsBySeverity.get(finding.severity) ?? []; + existing.push(finding); + findingsBySeverity.set(finding.severity, existing); } - if (data.totalUsage) { - const totalInput = data.totalUsage.inputTokens + (data.totalUsage.cacheReadInputTokens ?? 0); - statsParts.push(`${formatTokens(totalInput)} in / ${formatTokens(data.totalUsage.outputTokens)} out`); - statsParts.push(`**${formatCost(data.totalUsage.costUSD)}**`); + + const severityOrder: Severity[] = ['critical', 'high', 'medium', 'low', 'info']; + for (const severity of severityOrder) { + const findings = findingsBySeverity.get(severity); + if (!findings?.length) continue; + + const label = severity.charAt(0).toUpperCase() + severity.slice(1); + lines.push(`### ${label}`, ''); + + for (const finding of findings) { + const location = finding.location ? ` - ${formatLocation(finding.location)}` : ''; + lines.push('
'); + lines.push(`${finding.title}${location}`, ''); + lines.push(finding.description, ''); + lines.push('
', ''); + } } - lines.push(statsParts.join(' · '), ''); - } - if (data.totalFindings > 0) { - lines.push(...renderSeverityTable(data.findingsBySeverity), ''); + // Note if there are more findings not shown + if (data.totalFindings > topFindings.length) { + const remaining = data.totalFindings - topFindings.length; + lines.push(`*...and ${remaining} more*`, ''); + } + } else { + lines.push('No issues found.', ''); } - // Check if any skill has timing/cost data + // Skills table in collapsible section const hasSkillStats = data.skillResults.some((s) => s.durationMs !== undefined || s.usage); + const skillPlural = data.totalSkills === 1 ? '' : 's'; + + lines.push('
'); + lines.push(`${data.totalSkills} skill${skillPlural} analyzed`, ''); if (hasSkillStats) { lines.push( - '### Skills', - '', - '| Skill | Findings | Duration | Cost | Result |', - '|-------|----------|----------|------|--------|' + '| Skill | Findings | Duration | Cost |', + '|-------|----------|----------|------|' ); - for (const skill of data.skillResults) { - const icon = conclusionIcon(skill.conclusion); const duration = skill.durationMs !== undefined ? formatDuration(skill.durationMs) : '-'; const cost = skill.usage ? formatCost(skill.usage.costUSD) : '-'; - lines.push(`| ${skill.name} | ${skill.findingCount} | ${duration} | ${cost} | ${icon} ${skill.conclusion} |`); + lines.push(`| ${skill.name} | ${skill.findingCount} | ${duration} | ${cost} |`); } } else { lines.push( - '### Skills', - '', - '| Skill | Findings | Result |', - '|-------|----------|--------|' + '| Skill | Findings |', + '|-------|----------|' ); - for (const skill of data.skillResults) { - const icon = conclusionIcon(skill.conclusion); - lines.push(`| ${skill.name} | ${skill.findingCount} | ${icon} ${skill.conclusion} |`); + lines.push(`| ${skill.name} | ${skill.findingCount} |`); + } + } + + lines.push('', '
', ''); + + // Stats footer with labeled inline format + const hasStats = data.totalDurationMs !== undefined || data.totalUsage; + if (hasStats) { + const statsParts: string[] = []; + if (data.totalDurationMs !== undefined) { + statsParts.push(`**Duration:** ${formatDuration(data.totalDurationMs)}`); + } + if (data.totalUsage) { + const totalInput = data.totalUsage.inputTokens + (data.totalUsage.cacheReadInputTokens ?? 0); + statsParts.push(`**Tokens:** ${formatTokens(totalInput)} in / ${formatTokens(data.totalUsage.outputTokens)} out`); + statsParts.push(`**Cost:** ${formatCost(data.totalUsage.costUSD)}`); } + lines.push('---', statsParts.join(' · ')); } return lines.join('\n'); diff --git a/src/sdk/runner.ts b/src/sdk/runner.ts index d2f342f..af6d9ec 100644 --- a/src/sdk/runner.ts +++ b/src/sdk/runner.ts @@ -450,6 +450,7 @@ export async function runSkill( options: SkillRunnerOptions = {} ): Promise { const { parallel = true, callbacks, abortController } = options; + const startTime = Date.now(); if (!context.pullRequest) { throw new SkillRunnerError('Pull request context required for skill execution'); @@ -464,6 +465,7 @@ export async function runSkill( summary: 'No code changes to analyze', findings: [], usage: emptyUsage(), + durationMs: Date.now() - startTime, }; } @@ -556,6 +558,7 @@ export async function runSkill( summary, findings: uniqueFindings, usage: totalUsage, + durationMs: Date.now() - startTime, }; }