feat(checks): Improve GitHub output and check summary

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<void> {
           ? 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,

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<Severity, number>;
   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<void> {
   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<Severity, number>): 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<Severity, number>
-): 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<Severity, Finding[]>();
+    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('<details>');
+        lines.push(`<summary><strong>${finding.title}</strong>${location}</summary>`, '');
+        lines.push(finding.description, '');
+        lines.push('</details>', '');
+      }
+    }
   }
 
   // Add stats footer if available
-  const statsLine = formatStatsCompact(report.durationMs, report.usage);
-  if (statsLine) {
-    lines.push('', '---', `<sub>${statsLine}</sub>`);
+  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<Severity, Finding[]>();
+    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('<details>');
+        lines.push(`<summary><strong>${finding.title}</strong>${location}</summary>`, '');
+        lines.push(finding.description, '');
+        lines.push('</details>', '');
+      }
     }
-    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('<details>');
+  lines.push(`<summary>${data.totalSkills} skill${skillPlural} analyzed</summary>`, '');
 
   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('', '</details>', '');
+
+  // 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');

src/sdk/runner.ts

@@ -450,6 +450,7 @@ export async function runSkill(
   options: SkillRunnerOptions = {}
 ): Promise<SkillReport> {
   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,
   };
 }