also looking at this
fix(review): give pull request reviews the analyzers and learnings they skipped
#1415
4 files
+316
-106
| @@ -1585,56 +1585,39 @@ cli | ||
| 1585 | 1585 | process.exit(1) |
| 1586 | 1586 | } |
| 1587 | 1587 | |
| 1588 | const { | |
| 1589 | composeInstructions, | |
| 1590 | loadGuidelines, | |
| 1591 | parseReviewState, | |
| 1592 | prepareReview, | |
| 1593 | reviewDiff, | |
| 1594 | } = await import('../src/review') | |
| 1588 | const { composeInstructions, reviewDiff } = await import('../src/review') | |
| 1595 | 1589 | const { collectLocalDiff, formatReview, shouldFail } = await import('../src/review/local') |
| 1596 | 1590 | const { runAnalyzers } = await import('../src/analysis') |
| 1597 | 1591 | |
| 1598 | let diff: string | |
| 1599 | let headSha = '' | |
| 1600 | let seenFingerprints: string[] = [] | |
| 1601 | let gitProvider: GitProvider | null = null | |
| 1602 | let prBody: string | null = null | |
| 1603 | let wasPaused = false | |
| 1604 | ||
| 1592 | // A pull request review goes through the shared path, which is also what | |
| 1593 | // `@buddy review` uses. It used to be reimplemented here and had drifted: | |
| 1594 | // this command skipped analyzers outright for a pull request and never | |
| 1595 | // read learnings at all, so the automatic review the generated workflow | |
| 1596 | // runs was the one path missing both. | |
| 1605 | 1597 | if (prNumber) { |
| 1606 | gitProvider = await providerFor(config, 'reviewing a pull request', log) | |
| 1607 | ||
| 1608 | const number = Number.parseInt(prNumber, 10) | |
| 1609 | ||
| 1610 | // Carry forward what earlier reviews already said, so a re-review | |
| 1611 | // reports only what is new rather than repeating itself. | |
| 1612 | const prs = await gitProvider.getPullRequests('open') | |
| 1613 | const pr = prs.find(candidate => candidate.number === number) | |
| 1614 | ||
| 1615 | // `--auto` is what the generated workflow passes. Without it this is | |
| 1616 | // someone at a terminal, who has already decided they want a review. | |
| 1617 | if (pr) { | |
| 1618 | const { reviewSkipReason } = await import('../src/review/filters') | |
| 1619 | const skip = reviewSkipReason(config, pr, options.auto ? 'automatic' : 'requested') | |
| 1620 | if (skip) { | |
| 1621 | log.info(`🔍 Skipping PR #${number}: ${skip}`) | |
| 1622 | return | |
| 1623 | } | |
| 1624 | } | |
| 1598 | const gitProvider = await providerFor(config, 'reviewing a pull request', log) | |
| 1599 | const { runReviewForPR } = await import('../src/review/run') | |
| 1625 | 1600 | |
| 1626 | diff = await gitProvider.getPullRequestDiff(number) | |
| 1627 | const state = parseReviewState(pr?.body) | |
| 1628 | seenFingerprints = state?.fingerprints ?? [] | |
| 1629 | prBody = pr?.body ?? null | |
| 1630 | wasPaused = Boolean(state?.paused) | |
| 1601 | const status = await runReviewForPR({ | |
| 1602 | config, | |
| 1603 | provider: gitProvider, | |
| 1604 | prNumber: Number.parseInt(prNumber, 10), | |
| 1605 | // `--auto` is what the generated workflow passes. Without it this is | |
| 1606 | // someone at a terminal, who has already decided they want a review. | |
| 1607 | trigger: options.auto ? 'automatic' : 'requested', | |
| 1608 | logger: log, | |
| 1609 | dryRun: Boolean(options.dryRun), | |
| 1610 | ...(options.summaryOnly ? { summaryOnly: true } : {}), | |
| 1611 | ...(options.profile ? { profile: options.profile as 'chill' | 'assertive' } : {}), | |
| 1612 | }) | |
| 1631 | 1613 | |
| 1632 | // The commit being reviewed now, not the one reviewed last time. | |
| 1633 | // Reading it from the stored state recorded a stale sha, so the next | |
| 1634 | // run could not tell whether this commit had been seen. | |
| 1635 | headSha = await gitProvider.getPullRequestHeadSha(number) | |
| 1614 | logger.success(`✅ ${status}`) | |
| 1615 | return | |
| 1636 | 1616 | } |
| 1637 | else { | |
| 1617 | ||
| 1618 | let diff: string | |
| 1619 | ||
| 1620 | { | |
| 1638 | 1621 | const base = options.base || config.repository?.baseBranch || 'main' |
| 1639 | 1622 | const mode = options.staged ? 'staged' : options.branch ? 'branch' : 'working' |
| 1640 | 1623 | log.info(`🔍 Reviewing ${mode} changes${mode === 'branch' ? ` against ${base}` : ''}...`) |
| @@ -1648,10 +1631,12 @@ cli | ||
| 1648 | 1631 | } |
| 1649 | 1632 | |
| 1650 | 1633 | // Analyzers run locally against the working tree, so a local review gets |
| 1651 | // the same secret scanning and linting a CI review does — with no token. | |
| 1634 | // the same secret scanning and linting a pull request review does — with | |
| 1635 | // no token. The `|| prNumber` that used to sit in this condition is why | |
| 1636 | // a pull request review got none of it. | |
| 1652 | 1637 | const { parseUnifiedDiff } = await import('../src/review/diff') |
| 1653 | 1638 | const changedFiles = parseUnifiedDiff(diff).files.map(file => file.path) |
| 1654 | const analysis = config.analysis?.enabled === false || prNumber | |
| 1639 | const analysis = config.analysis?.enabled === false | |
| 1655 | 1640 | ? { findings: [], ran: [], skipped: [] } |
| 1656 | 1641 | : await runAnalyzers({ |
| 1657 | 1642 | files: changedFiles, |
| @@ -1676,30 +1661,18 @@ cli | ||
| 1676 | 1661 | return |
| 1677 | 1662 | } |
| 1678 | 1663 | |
| 1679 | // Guidelines are inlined into the prompt, so they are always read from | |
| 1680 | // the base branch — reading them from the PR's own branch would let a | |
| 1681 | // contributor rewrite the rules their code is reviewed against. | |
| 1682 | const baseRef = config.repository?.baseBranch || options.base || 'main' | |
| 1683 | const guidelines = gitProvider | |
| 1684 | ? await loadGuidelines( | |
| 1685 | (path, ref) => gitProvider!.getFileContent(path, ref), | |
| 1686 | baseRef, | |
| 1687 | config.ai?.review?.guidelineFiles, | |
| 1688 | logger, | |
| 1689 | ) | |
| 1690 | : '' | |
| 1691 | ||
| 1664 | // A local review has no provider to read the base branch through, so it | |
| 1665 | // runs without repository guidelines. Reading them from the working tree | |
| 1666 | // would be a different thing: guidelines are trusted context, and the | |
| 1667 | // working tree is whatever the person running this has edited. | |
| 1692 | 1668 | const reviewed = await reviewDiff(ai!, { |
| 1693 | 1669 | diff, |
| 1694 | 1670 | profile: options.profile ?? config.ai?.review?.profile, |
| 1695 | 1671 | summaryOnly: options.summaryOnly ?? config.ai?.review?.summaryOnly, |
| 1696 | instructions: composeInstructions({ | |
| 1697 | global: config.ai?.review?.instructions, | |
| 1698 | guidelines, | |
| 1699 | }), | |
| 1672 | instructions: composeInstructions({ global: config.ai?.review?.instructions }), | |
| 1700 | 1673 | pathFilters: config.ai?.review?.pathFilters, |
| 1701 | 1674 | pathInstructions: config.ai?.review?.pathInstructions, |
| 1702 | seenFingerprints, | |
| 1675 | seenFingerprints: [], | |
| 1703 | 1676 | logger: log, |
| 1704 | 1677 | }) |
| 1705 | 1678 | |
| @@ -1711,50 +1684,15 @@ cli | ||
| 1711 | 1684 | omittedFiles: [...reviewed.omittedFiles, ...analysis.skipped.map(entry => `${entry.name} (${entry.reason})`)], |
| 1712 | 1685 | } |
| 1713 | 1686 | |
| 1714 | const prepared = prepareReview(result, { | |
| 1715 | headSha, | |
| 1716 | requestChangesOn: config.ai?.review?.requestChangesOn, | |
| 1717 | seenFingerprints, | |
| 1718 | }) | |
| 1719 | ||
| 1720 | if (!prNumber) { | |
| 1721 | process.stdout.write(formatReview(result, format)) | |
| 1722 | ||
| 1723 | if (options.fix) { | |
| 1724 | const applied = await applyFixes(result.findings, Boolean(options.yes), logger) | |
| 1725 | logger.success(`🔧 Applied ${applied} suggestion(s)`) | |
| 1726 | } | |
| 1727 | ||
| 1728 | if (options.failOn && shouldFail(result.findings, options.failOn as 'critical')) | |
| 1729 | process.exit(1) | |
| 1730 | return | |
| 1731 | } | |
| 1732 | ||
| 1733 | if (options.dryRun) { | |
| 1734 | logger.info(`\n${prepared.body}\n`) | |
| 1735 | for (const comment of prepared.comments) | |
| 1736 | logger.info(`${comment.path}:${comment.line}\n${comment.body}\n`) | |
| 1737 | logger.success(`✅ Review complete: ${result.findings.length} finding(s)`) | |
| 1738 | return | |
| 1739 | } | |
| 1687 | process.stdout.write(formatReview(result, format)) | |
| 1740 | 1688 | |
| 1741 | assertSupports(gitProvider!, 'inlineReviewComments', 'createReview', 'posting a review') | |
| 1742 | const reviewedNumber = Number.parseInt(prNumber, 10) | |
| 1743 | await gitProvider.createReview(reviewedNumber, prepared) | |
| 1744 | ||
| 1745 | // The review has landed; recording where it got to is bookkeeping, so a | |
| 1746 | // failure here must not present as a failed review. | |
| 1747 | try { | |
| 1748 | const { upsertReviewState } = await import('../src/review/marker') | |
| 1749 | await gitProvider.updatePullRequest(reviewedNumber, { | |
| 1750 | body: upsertReviewState(prBody, { ...prepared.state, ...(wasPaused ? { paused: true } : {}) }), | |
| 1751 | }) | |
| 1752 | } | |
| 1753 | catch (error) { | |
| 1754 | logger.warn(`Could not record review state on PR #${prNumber}: ${error instanceof Error ? error.message : String(error)}`) | |
| 1689 | if (options.fix) { | |
| 1690 | const applied = await applyFixes(result.findings, Boolean(options.yes), logger) | |
| 1691 | logger.success(`🔧 Applied ${applied} suggestion(s)`) | |
| 1755 | 1692 | } |
| 1756 | 1693 | |
| 1757 | logger.success(`✅ Reviewed PR #${prNumber}: ${result.findings.length} finding(s)`) | |
| 1694 | if (options.failOn && shouldFail(result.findings, options.failOn as 'critical')) | |
| 1695 | process.exit(1) | |
| 1758 | 1696 | } |
| 1759 | 1697 | catch (error) { |
| 1760 | 1698 | logger.error('Review failed:', error) |