From b4a38d6aa7be144d172b5ce877aca0dfcfb4bb48 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 11 Jul 2026 18:02:11 -0400 Subject: [PATCH 01/12] feat: V1 orchestration pipeline, workflows, and README MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire the full review pipeline: orchestrate.ts composes the I/O clients (github, openrouter, context) with the pure review modules (diff, phases, prompt, selection, comment mapping) into a 15-step pipeline. Skip paths after PR identification post body-only reviews (§5 amendment). createPromptedGenerateFindings implements the V1 default — one prompted combined-phase call with per-run nonce-wrapped delimiters. main.ts replaces the scaffold stub with the full wiring: client construction, orchestrate call, output setting, and cost summary write. self_review.yml dogfoods the action on its own PRs and @umm review comments. release.yml builds and pushes the GHCR image, digest-flips action.yml, force-moves semver + floating major tags, and creates a GitHub Release. README documents all 12 inputs, 4 outputs, usage example with both triggers, and the review pipeline. 207 tests (41 new for orchestrate); 3 mutation spot checks verified. Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- .github/workflows/release.yml | 94 +++++ .github/workflows/self_review.yml | 43 +++ AGENTS.md | 4 +- README.md | 97 ++++- src/__tests__/orchestrate.test.ts | 585 ++++++++++++++++++++++++++++++ src/main.ts | 68 +++- src/orchestrate.ts | 282 ++++++++++++++ 7 files changed, 1151 insertions(+), 22 deletions(-) create mode 100644 .github/workflows/release.yml create mode 100644 .github/workflows/self_review.yml create mode 100644 src/__tests__/orchestrate.test.ts create mode 100644 src/orchestrate.ts diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..c12fe3a --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,94 @@ +name: Release + +on: + push: + tags: ["v*"] + +permissions: + contents: read + +jobs: + build-and-push: + runs-on: ubuntu-latest + permissions: + contents: read + packages: write + outputs: + digest: ${{ steps.build.outputs.digest }} + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + + - uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3 + + - uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3 + with: + registry: ghcr.io + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - uses: docker/metadata-action@c299e40c65443455700f0fdfc63efafe5b349051 # v5 + id: meta + with: + images: ghcr.io/${{ github.repository }} + tags: | + type=semver,pattern={{version}} + type=raw,value=latest + + - uses: docker/build-push-action@10e90e3645eae34f1e60eeb005ba3a3d33f178e8 # v6 + id: build + with: + context: . + push: true + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} + + release: + needs: build-and-push + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - uses: actions/create-github-app-token@fee1f7d63c2ff003460e3d139729b119787bc349 # v2 + id: app-token + with: + app-id: ${{ secrets.RELEASE_APP_ID }} + private-key: ${{ secrets.RELEASE_PRIVATE_KEY }} + + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + token: ${{ steps.app-token.outputs.token }} + ref: ${{ github.ref }} + + - name: Flip action.yml image to digest-pinned GHCR reference + env: + DIGEST: ${{ needs.build-and-push.outputs.digest }} + IMAGE: ghcr.io/${{ github.repository }} + run: | + sed -i "s|image: Dockerfile|image: docker://${IMAGE}@${DIGEST}|" action.yml + git config user.name "umm-actually[bot]" + git config user.email "umm-actually[bot]@users.noreply.github.com" + git add action.yml + git commit -m "chore(release): pin image to ${DIGEST}" + + - name: Force-move semver tag to the digest-flipped commit + env: + TAG: ${{ github.ref_name }} + run: | + git tag -f "$TAG" + git push origin "$TAG" --force + + - name: Force-move floating major tag + env: + TAG: ${{ github.ref_name }} + run: | + MAJOR="${TAG%%.*}" + git tag -f "$MAJOR" + git push origin "$MAJOR" --force + + - name: Create GitHub Release + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + TAG: ${{ github.ref_name }} + run: gh release create "$TAG" --generate-notes diff --git a/.github/workflows/self_review.yml b/.github/workflows/self_review.yml new file mode 100644 index 0000000..863f61c --- /dev/null +++ b/.github/workflows/self_review.yml @@ -0,0 +1,43 @@ +name: Self Review + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + issue_comment: + types: [created] + +permissions: + contents: read + +concurrency: + group: self-review-${{ github.event.pull_request.number || github.event.issue.number }} + cancel-in-progress: true + +jobs: + review: + runs-on: ubuntu-latest + if: >- + github.event_name == 'pull_request' || + ( + github.event_name == 'issue_comment' && + github.event.issue.pull_request && + contains(github.event.comment.body, '@umm review') + ) + permissions: + contents: read + pull-requests: write + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + + - uses: actions/create-github-app-token@fee1f7d63c2ff003460e3d139729b119787bc349 # v2 + id: app-token + with: + app-id: ${{ secrets.UMM_APP_ID }} + private-key: ${{ secrets.UMM_PRIVATE_KEY }} + + - uses: ./ + with: + github_token: ${{ steps.app-token.outputs.token }} + openrouter_api_key: ${{ secrets.OPENROUTER_KEY }} diff --git a/AGENTS.md b/AGENTS.md index 1240026..fbb447a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,7 +16,7 @@ action.yml # action metadata — inputs/outputs, runs.using: doc Dockerfile # multi-stage: build (tsc) → slim runtime fixtures/ # test fixtures (event payloads, sample diff, LLM responses) src/ - main.ts # entrypoint — collects/validates inputs; pipeline + outputs land next PR + main.ts # entrypoint — collects/validates inputs, wires clients into orchestrate, sets outputs config.ts # action inputs → validated ActionConfig logger.ts # structured JSON logger — levels, child contexts, lazy props github/ # GitHub I/O: event payload → PrContext, octokit wrappers (diff fetch, review posting) @@ -24,7 +24,7 @@ src/ diff/ # pure transforms over parse-diff output context/ # workspace I/O: conventions file, changed files, related-files reverse-import scan review/ # pure review logic: finding schema, phases, prompt, selection, comment mapping - orchestrate.ts # (planned — next PR) the pipeline — fully testable with stub clients + orchestrate.ts # pipeline + createPromptedGenerateFindings — fully testable with stub clients ``` ## Module layering diff --git a/README.md b/README.md index b04c652..e2cef88 100644 --- a/README.md +++ b/README.md @@ -2,8 +2,6 @@ LLM-powered pull request review as a GitHub Action. One consolidated review per PR with inline findings, powered by any model on [OpenRouter](https://openrouter.ai). -> Full documentation, inputs reference, and setup guide land with the first release. This repo is under active initial development. - ## What it does - Reviews the PR diff **and traces changed code into its callers** — regressions and pre-existing bugs in affected code are findings, not noise @@ -11,6 +9,101 @@ LLM-powered pull request review as a GitHub Action. One consolidated review per - Posts exactly **one** PR review with inline comments anchored to diff lines — no duplicate comments, no unrequested-reviewer badges - Structured output end to end: every finding carries a category, severity, confidence, and a concrete failure scenario - Model-agnostic via OpenRouter — pick your model, see your per-call costs +- Findings that can't be anchored to the diff (e.g. callers outside the changed files) render in the review body under "Findings beyond the diff" +- PRs with oversized diffs are skipped gracefully with a body-only review stating the reason + +## Setup + +umm-actually runs as a Docker-based action. It needs a GitHub token (for fetching the diff and posting the review) and an OpenRouter API key. + +For the best experience, use a [GitHub App](https://docs.github.com/en/apps/creating-github-apps) installation token so reviews are attributed to a bot identity rather than a personal account. + +## Usage + +```yaml +name: Review + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + issue_comment: + types: [created] + +permissions: + contents: read + +concurrency: + group: review-${{ github.event.pull_request.number || github.event.issue.number }} + cancel-in-progress: true + +jobs: + review: + runs-on: ubuntu-latest + if: >- + github.event_name == 'pull_request' || + ( + github.event_name == 'issue_comment' && + github.event.issue.pull_request && + contains(github.event.comment.body, '@umm review') + ) + permissions: + contents: read + pull-requests: write + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + + - uses: actions/create-github-app-token@v2 + id: app-token + with: + app-id: ${{ secrets.UMM_APP_ID }} + private-key: ${{ secrets.UMM_PRIVATE_KEY }} + + - uses: aliasunder/umm-actually@v0 + with: + github_token: ${{ steps.app-token.outputs.token }} + openrouter_api_key: ${{ secrets.OPENROUTER_KEY }} +``` + +The `@umm review` comment trigger lets you re-request a review on any PR by commenting. The `issue_comment` event fires for PR comments — the `if` condition filters to PRs only. + +## Inputs + +| Input | Default | Description | +| ----------------------- | ----------------------------- | ----------------------------------------------------------------------------------------------------------- | +| `github_token` | _(required)_ | Token for fetching the diff and posting the review. A GitHub App installation token keeps the bot identity. | +| `openrouter_api_key` | _(required)_ | OpenRouter API key | +| `model` | `anthropic/claude-sonnet-4-6` | OpenRouter model slug exactly as listed on openrouter.ai/models | +| `fallback_model` | `""` | Model to retry with if the primary model fails the structured-output ladder | +| `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | +| `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | +| `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt | +| `phases` | `combined` | Review phases to run. V1 supports: `combined` | +| `context_budget_tokens` | `80000` | Approximate token budget for prompt context (conventions + file contents + diff) | +| `trace_related_files` | `true` | Include files that reference changed files in the prompt so the model can trace regressions into callers | +| `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | +| `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | + +## Outputs + +| Output | Description | +| ---------------- | ------------------------------------------------------------ | +| `findings_count` | Number of findings posted (after threshold and cap) | +| `review_url` | URL of the submitted review; empty when no review was posted | +| `model_used` | Model that produced the accepted response | +| `skipped_reason` | Non-empty when the review was skipped (e.g. diff too large) | + +## How it works + +1. Resolves the PR from the triggering event (supports `pull_request`, `pull_request_target`, and `issue_comment` events) +2. Fetches the unified diff via the GitHub API — PRs that exceed the API's diff size limit are skipped +3. Reads the conventions file and changed source files (token-budgeted), then traces imports to find related files that reference the changes +4. Builds a structured prompt with randomized delimiter nonces (prompt injection defense) and sends it to OpenRouter +5. Validates the response against a strict Zod schema, retrying with a fallback model if the primary fails +6. Filters findings by severity threshold, deduplicates overlapping findings, and caps if configured +7. Maps findings to inline PR review comments anchored to diff lines, with a snap-to-nearest-hunk fallback +8. Posts one consolidated review — findings that can't be inlined render in the review body ## License diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts new file mode 100644 index 0000000..4e9438b --- /dev/null +++ b/src/__tests__/orchestrate.test.ts @@ -0,0 +1,585 @@ +import { readFileSync } from "node:fs" +import parseDiff from "parse-diff" +import { describe, expect, it } from "vitest" +import type { ActionConfig } from "../config.js" +import type { GithubClient } from "../github/client.js" +import type { PrContext } from "../github/event.js" +import type { ContextReader } from "../context/workspace.js" +import type { + ModelAttempt, + OpenRouterClient, + StructuredReviewResult, +} from "../openrouter/client.js" +import type { PromptFile } from "../review/prompt.js" +import type { ReviewResponse } from "../review/finding.js" +import { + orchestrate, + createPromptedGenerateFindings, + type OrchestrateDeps, + type GenerateFindings, +} from "../orchestrate.js" +import { createTestLogger } from "./test-logger.js" + +const sampleDiff = readFileSync( + new URL("../../fixtures/sample.diff", import.meta.url), + "utf8", +) + +const pullRequestPayload: Record = JSON.parse( + readFileSync( + new URL("../../fixtures/pull_request.opened.json", import.meta.url), + "utf8", + ), +) + +const fixtureReviewResponse: ReviewResponse = JSON.parse( + readFileSync( + new URL("../../fixtures/openrouter.response.json", import.meta.url), + "utf8", + ), +) + +const fixturePrContext: PrContext = { + prNumber: 7, + title: "feat: trim names before greeting", + body: "Trims whitespace from names and validates registry keys.", + headSha: "abc123def456abc123def456abc123def456abc1", + headRef: "feat/trim-names", + baseRef: "main", +} + +const fixtureAttempt: ModelAttempt = { + model: "test/model", + outcome: "accepted", + promptTokens: 1000, + completionTokens: 500, + costUsd: 0.01, + errorSummary: null, +} + +const fixtureChangedFile: PromptFile = { + path: "src/greeter.ts", + content: "export const greet = (name: string) => name.trim()", + includedAs: "full", +} + +const baseConfig: ActionConfig = { + githubToken: "ghp_test", + openrouterApiKey: "sk-test", + model: "test/model", + fallbackModel: "", + maxFindings: undefined, + severityThreshold: "low", + conventionsFile: "AGENTS.md", + phases: "combined", + contextBudgetTokens: 80_000, + traceRelatedFiles: true, + costSummary: true, + prNumberOverride: undefined, +} + +type RecordingStubs = { + deps: OrchestrateDeps + fetchPullRequestCalls: Record[] + fetchDiffCalls: Record[] + submitReviewCalls: Record[] + readConventionsCalls: Record[] + readChangedFilesCalls: Record[] + findRelatedFilesCalls: Record[] + generateFindingsCalls: unknown[] +} + +const makeOrchestrateDeps = ( + overrides: { + config?: Partial + eventName?: string + payload?: unknown + githubClient?: Partial + contextReader?: Partial + generateFindings?: GenerateFindings + fixtureResult?: Partial + } = {}, +): RecordingStubs => { + const fetchPullRequestCalls: Record[] = [] + const fetchDiffCalls: Record[] = [] + const submitReviewCalls: Record[] = [] + const readConventionsCalls: Record[] = [] + const readChangedFilesCalls: Record[] = [] + const findRelatedFilesCalls: Record[] = [] + const generateFindingsCalls: unknown[] = [] + + const structuredResult: StructuredReviewResult = { + review: fixtureReviewResponse, + modelUsed: "test/model", + attempts: [fixtureAttempt], + ...overrides.fixtureResult, + } + + const githubClient: GithubClient = { + fetchPullRequest: async (params) => { + fetchPullRequestCalls.push(params) + return fixturePrContext + }, + fetchDiff: async (params) => { + fetchDiffCalls.push(params) + return { kind: "ok" as const, diff: sampleDiff } + }, + submitReview: async (params) => { + submitReviewCalls.push(params) + return { + url: "https://github.com/test/review/1", + usedFallbackBody: false, + } + }, + ...overrides.githubClient, + } + + const contextReader: ContextReader = { + readConventions: async (params) => { + readConventionsCalls.push(params) + return "# Test conventions" + }, + readChangedFiles: async (params) => { + readChangedFilesCalls.push(params) + return { files: [fixtureChangedFile], remainingTokens: 40_000 } + }, + findRelatedFiles: async (params) => { + findRelatedFilesCalls.push(params) + return [] + }, + ...overrides.contextReader, + } + + const defaultGenerateFindings: GenerateFindings = async (reviewContext) => { + generateFindingsCalls.push(reviewContext) + return structuredResult + } + + const deps: OrchestrateDeps = { + config: { ...baseConfig, ...overrides.config }, + eventName: overrides.eventName ?? "pull_request", + payload: overrides.payload ?? pullRequestPayload, + githubClient, + contextReader, + generateFindings: overrides.generateFindings ?? defaultGenerateFindings, + } + + return { + deps, + fetchPullRequestCalls, + fetchDiffCalls, + submitReviewCalls, + readConventionsCalls, + readChangedFilesCalls, + findRelatedFilesCalls, + generateFindingsCalls, + } +} + +describe("orchestrate", () => { + describe("startup validation", () => { + it("throws on invalid severity threshold before any network call", async () => { + const stubs = makeOrchestrateDeps({ + config: { severityThreshold: "invalid" }, + }) + const logger = createTestLogger() + + await expect(orchestrate(stubs.deps, logger)).rejects.toThrow("severity") + expect(stubs.fetchDiffCalls).toHaveLength(0) + expect(stubs.generateFindingsCalls).toHaveLength(0) + expect(stubs.submitReviewCalls).toHaveLength(0) + }) + + it("throws on invalid phases input before any network call", async () => { + const stubs = makeOrchestrateDeps({ + config: { phases: "invalid" }, + }) + const logger = createTestLogger() + + await expect(orchestrate(stubs.deps, logger)).rejects.toThrow("phases") + expect(stubs.fetchDiffCalls).toHaveLength(0) + expect(stubs.generateFindingsCalls).toHaveLength(0) + }) + }) + + describe("event resolution", () => { + it("returns skipped result for non-PR events without calling any stubs", async () => { + const stubs = makeOrchestrateDeps({ + eventName: "push", + payload: {}, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.skippedReason).toContain("unsupported event") + expect(result.reviewUrl).toBe("") + expect(result.findingsCount).toBe(0) + expect(result.modelUsed).toBe("") + expect(result.costSummaryMarkdown).toBeNull() + expect(stubs.generateFindingsCalls).toHaveLength(0) + expect(stubs.submitReviewCalls).toHaveLength(0) + }) + + it("fetches PR context when event needs fetch", async () => { + const stubs = makeOrchestrateDeps({ + config: { prNumberOverride: 42 }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.fetchPullRequestCalls).toHaveLength(1) + expect(stubs.fetchPullRequestCalls[0]).toEqual({ prNumber: 42 }) + }) + }) + + describe("skip paths — post body-only review", () => { + it("posts skip review when diff is too large", async () => { + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "too_large" as const }), + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.skippedReason).toContain("diff exceeds") + expect(result.reviewUrl).toBe("https://github.com/test/review/1") + expect(result.findingsCount).toBe(0) + expect(result.modelUsed).toBe("") + expect(stubs.generateFindingsCalls).toHaveLength(0) + expect(stubs.submitReviewCalls).toHaveLength(1) + const reviewCall = stubs.submitReviewCalls[0] as Record + expect(reviewCall["comments"]).toEqual([]) + expect(reviewCall["body"]).toContain("diff exceeds") + }) + + it("posts skip review when diff parses to zero files", async () => { + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "ok" as const, diff: "" }), + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.skippedReason).toBe("empty diff") + expect(result.reviewUrl).toBe("https://github.com/test/review/1") + expect(stubs.generateFindingsCalls).toHaveLength(0) + expect(stubs.submitReviewCalls).toHaveLength(1) + const reviewCall = stubs.submitReviewCalls[0] as Record + expect(reviewCall["body"]).toContain("empty diff") + }) + + it("posts skip review when annotated diff exceeds budget", async () => { + const stubs = makeOrchestrateDeps({ + config: { contextBudgetTokens: 10 }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.skippedReason).toContain("diff too large") + expect(result.skippedReason).toContain("tokens") + expect(result.skippedReason).toContain("budget") + expect(result.reviewUrl).toBe("https://github.com/test/review/1") + expect(stubs.generateFindingsCalls).toHaveLength(0) + expect(stubs.submitReviewCalls).toHaveLength(1) + }) + }) + + describe("happy path", () => { + it("posts review with correct params and returns expected result", async () => { + const stubs = makeOrchestrateDeps() + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(fixtureReviewResponse.findings.length) + expect(result.reviewUrl).toBe("https://github.com/test/review/1") + expect(result.modelUsed).toBe("test/model") + expect(result.skippedReason).toBe("") + expect(result.costSummaryMarkdown).toEqual(expect.any(String)) + + expect(stubs.submitReviewCalls).toHaveLength(1) + const reviewCall = stubs.submitReviewCalls[0] as Record + expect(reviewCall["prNumber"]).toBe(7) + expect(reviewCall["commitId"]).toBe(fixturePrContext.headSha) + }) + + it("passes changed files and conventions to generateFindings", async () => { + const stubs = makeOrchestrateDeps() + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.generateFindingsCalls).toHaveLength(1) + const reviewContext = stubs.generateFindingsCalls[0] as Record< + string, + unknown + > + expect(reviewContext["conventions"]).toBe("# Test conventions") + expect(reviewContext["changedFiles"]).toEqual([fixtureChangedFile]) + expect(reviewContext["prContext"]).toEqual(fixturePrContext) + }) + + it("includes annotated diff in review context", async () => { + const stubs = makeOrchestrateDeps() + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + const reviewContext = stubs.generateFindingsCalls[0] as Record< + string, + unknown + > + const annotatedDiff = reviewContext["annotatedDiff"] as string + expect(annotatedDiff).toContain("src/greeter.ts") + }) + }) + + describe("conditional behaviors", () => { + it("does not call findRelatedFiles when traceRelatedFiles is false", async () => { + const stubs = makeOrchestrateDeps({ + config: { traceRelatedFiles: false }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.findRelatedFilesCalls).toHaveLength(0) + const reviewContext = stubs.generateFindingsCalls[0] as Record< + string, + unknown + > + expect(reviewContext["relatedFiles"]).toEqual([]) + }) + + it("calls findRelatedFiles when traceRelatedFiles is true", async () => { + const stubs = makeOrchestrateDeps({ + config: { traceRelatedFiles: true }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.findRelatedFilesCalls).toHaveLength(1) + }) + + it("posts zero-findings body when all findings are below threshold", async () => { + const stubs = makeOrchestrateDeps({ + config: { severityThreshold: "critical" }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(0) + expect(result.reviewUrl).toBe("https://github.com/test/review/1") + expect(result.skippedReason).toBe("") + expect(stubs.submitReviewCalls).toHaveLength(1) + const reviewCall = stubs.submitReviewCalls[0] as Record + expect(reviewCall["body"]).toContain("no findings above threshold") + expect(reviewCall["comments"]).toEqual([]) + }) + + it("respects maxFindings cap and includes dropped findings in body", async () => { + const stubs = makeOrchestrateDeps({ + config: { maxFindings: 1 }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(1) + const reviewCall = stubs.submitReviewCalls[0] as Record + const body = reviewCall["body"] as string + expect(body).toContain("omitted") + }) + + it("returns costSummaryMarkdown when LLM call happened", async () => { + const stubs = makeOrchestrateDeps() + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.costSummaryMarkdown).not.toBeNull() + expect(result.costSummaryMarkdown).toContain("test/model") + }) + + it("returns null costSummaryMarkdown when skipped before LLM call", async () => { + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "too_large" as const }), + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.costSummaryMarkdown).toBeNull() + }) + + it("builds fallback body with all selected findings", async () => { + const stubs = makeOrchestrateDeps() + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + const reviewCall = stubs.submitReviewCalls[0] as Record + const fallbackBody = reviewCall["fallbackBody"] as string + expect(fallbackBody).toContain("test/model") + for (const finding of fixtureReviewResponse.findings) { + expect(fallbackBody).toContain(finding.title) + } + }) + + it("uses complete PrContext from pull_request event without fetching", async () => { + const stubs = makeOrchestrateDeps() + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.fetchPullRequestCalls).toHaveLength(0) + }) + }) +}) + +describe("createPromptedGenerateFindings", () => { + it("passes model and fallbackModel to openrouterClient.requestReview", async () => { + const requestReviewCalls: Record[] = [] + const stubClient: OpenRouterClient = { + requestReview: async (params) => { + requestReviewCalls.push(params) + return { + review: fixtureReviewResponse, + modelUsed: "test/model", + attempts: [fixtureAttempt], + } + }, + } + + const generate = createPromptedGenerateFindings( + { + openrouterClient: stubClient, + model: "test/primary", + fallbackModel: "test/fallback", + }, + createTestLogger(), + ) + + const files = parseDiff(sampleDiff) + const { annotateDiff } = await import("../diff/annotate-diff.js") + const { resolvePhases } = await import("../review/phases.js") + const phases = resolvePhases("combined") + + await generate({ + prContext: fixturePrContext, + phase: phases[0]!, + conventions: "test conventions", + changedFiles: [fixtureChangedFile], + relatedFiles: [], + annotatedDiff: annotateDiff(files), + priorFindings: [], + }) + + expect(requestReviewCalls).toHaveLength(1) + expect(requestReviewCalls[0]).toEqual( + expect.objectContaining({ + model: "test/primary", + fallbackModel: "test/fallback", + }), + ) + }) + + it("includes annotated diff in the user prompt", async () => { + const requestReviewCalls: Record[] = [] + const stubClient: OpenRouterClient = { + requestReview: async (params) => { + requestReviewCalls.push(params) + return { + review: fixtureReviewResponse, + modelUsed: "test/model", + attempts: [fixtureAttempt], + } + }, + } + + const generate = createPromptedGenerateFindings( + { openrouterClient: stubClient, model: "m", fallbackModel: null }, + createTestLogger(), + ) + + const files = parseDiff(sampleDiff) + const { annotateDiff } = await import("../diff/annotate-diff.js") + const { resolvePhases } = await import("../review/phases.js") + const phases = resolvePhases("combined") + const annotated = annotateDiff(files) + + await generate({ + prContext: fixturePrContext, + phase: phases[0]!, + conventions: null, + changedFiles: [], + relatedFiles: [], + annotatedDiff: annotated, + priorFindings: [], + }) + + const call = requestReviewCalls[0] as Record + const userPrompt = call["userPrompt"] as string + expect(userPrompt).toContain("src/greeter.ts") + }) + + it("generates a unique nonce per call", async () => { + const userPrompts: string[] = [] + const stubClient: OpenRouterClient = { + requestReview: async (params) => { + userPrompts.push( + (params as Record)["userPrompt"] as string, + ) + return { + review: { analysis: "", findings: [] }, + modelUsed: "m", + attempts: [], + } + }, + } + + const generate = createPromptedGenerateFindings( + { openrouterClient: stubClient, model: "m", fallbackModel: null }, + createTestLogger(), + ) + + const files = parseDiff(sampleDiff) + const { annotateDiff } = await import("../diff/annotate-diff.js") + const { resolvePhases } = await import("../review/phases.js") + const phases = resolvePhases("combined") + const annotated = annotateDiff(files) + + const context = { + prContext: fixturePrContext, + phase: phases[0]!, + conventions: null, + changedFiles: [], + relatedFiles: [], + annotatedDiff: annotated, + priorFindings: [], + } + + await generate(context) + await generate(context) + + expect(userPrompts).toHaveLength(2) + // Nonce-suffixed tags should differ between calls + const noncePattern = / ({ githubToken: core.getInput("github_token", { required: true }), openrouterApiKey: core.getInput("openrouter_api_key", { required: true }), @@ -22,21 +26,49 @@ const collectRawInputs = (): RawInputs => ({ prNumberOverride: core.getInput("pr_number"), }) -// Input collection and config validation are wired now so bad inputs fail -// loudly today; orchestrate.ts lands in the next PR and replaces the -// setFailed stub below. try { const config = parseConfig(collectRawInputs()) - // Register both credentials with the runner's masker before anything can - // log — our JSON logger writes raw to stdout, bypassing the masking the - // runner applies only to values passed through ::add-mask::. core.setSecret(config.githubToken) core.setSecret(config.openrouterApiKey) - core.setFailed( - "umm-actually: review pipeline not yet implemented (scaffold only)", - ) -} catch (configError) { - core.setFailed( - configError instanceof Error ? configError.message : String(configError), + + const workspaceRoot = envVar + .from(process.env) + .get("GITHUB_WORKSPACE") + .required() + .asString() + const octokit = getOctokit(config.githubToken) + const { owner, repo } = context.repo + + const result = await orchestrate( + { + config, + eventName: context.eventName, + payload: context.payload, + githubClient: createGithubClient({ octokit, owner, repo }, logger), + contextReader: createContextReader({ workspaceRoot }, logger), + generateFindings: createPromptedGenerateFindings( + { + openrouterClient: createOpenRouterClient( + { sdk: new OpenRouter({ apiKey: config.openrouterApiKey }) }, + logger, + ), + model: config.model, + fallbackModel: + config.fallbackModel === "" ? null : config.fallbackModel, + }, + logger, + ), + }, + logger, ) + + core.setOutput("findings_count", result.findingsCount) + core.setOutput("review_url", result.reviewUrl) + core.setOutput("model_used", result.modelUsed) + core.setOutput("skipped_reason", result.skippedReason) + if (config.costSummary && result.costSummaryMarkdown !== null) { + await core.summary.addRaw(result.costSummaryMarkdown).write() + } +} catch (error) { + core.setFailed(error instanceof Error ? error.message : String(error)) } diff --git a/src/orchestrate.ts b/src/orchestrate.ts new file mode 100644 index 0000000..020cfab --- /dev/null +++ b/src/orchestrate.ts @@ -0,0 +1,282 @@ +import parseDiff from "parse-diff" +import type { ActionConfig } from "./config.js" +import { + computeCommentableLines, + newFilePath, +} from "./diff/commentable-lines.js" +import { annotateDiff } from "./diff/annotate-diff.js" +import type { Logger } from "./logger.js" +import type { GithubClient } from "./github/client.js" +import { resolvePullRequestEvent, type PrContext } from "./github/event.js" +import type { + OpenRouterClient, + StructuredReviewResult, +} from "./openrouter/client.js" +import { renderCostSummary } from "./openrouter/cost-summary.js" +import type { ContextReader } from "./context/workspace.js" +import { + buildReviewBody, + buildZeroFindingsBody, + mapFindingsToReview, +} from "./review/comment-mapping.js" +import { resolveSeverityThreshold, type Finding } from "./review/finding.js" +import { resolvePhases, type ReviewPhase } from "./review/phases.js" +import { + buildSystemPrompt, + buildUserPrompt, + estimateTokens, + generateDelimiterNonce, + type PromptFile, +} from "./review/prompt.js" +import { selectFindings } from "./review/select-findings.js" + +export type ReviewContext = { + prContext: PrContext + phase: ReviewPhase + conventions: string | null + changedFiles: PromptFile[] + relatedFiles: PromptFile[] + annotatedDiff: string + priorFindings: Finding[] +} + +export type GenerateFindings = ( + reviewContext: ReviewContext, +) => Promise + +export type OrchestrateResult = { + findingsCount: number + reviewUrl: string + modelUsed: string + skippedReason: string + costSummaryMarkdown: string | null +} + +export type OrchestrateDeps = { + config: ActionConfig + eventName: string + payload: unknown + githubClient: GithubClient + contextReader: ContextReader + generateFindings: GenerateFindings +} + +const buildSkipBody = (reason: string): string => + `**umm-actually** — review skipped\n\n${reason}\n\n---\n*umm-actually*` + +const SKIPPED_RESULT_BASE: Omit< + OrchestrateResult, + "reviewUrl" | "skippedReason" +> = { + findingsCount: 0, + modelUsed: "", + costSummaryMarkdown: null, +} + +export const orchestrate = async ( + deps: OrchestrateDeps, + logger: Logger, +): Promise => { + const { config, githubClient, contextReader, generateFindings } = deps + + // Step 1: fail-fast validation — throws before any network call + const severityThreshold = resolveSeverityThreshold(config.severityThreshold) + const phases = resolvePhases(config.phases) + + // Step 2: event resolution + const resolvedEvent = resolvePullRequestEvent( + { + eventName: deps.eventName, + payload: deps.payload, + prNumberOverride: config.prNumberOverride, + }, + logger, + ) + + if (resolvedEvent.kind === "not_a_pr") { + logger.info("not a PR event — skipping", { reason: resolvedEvent.reason }) + return { + ...SKIPPED_RESULT_BASE, + reviewUrl: "", + skippedReason: resolvedEvent.reason, + } + } + + // Step 3: PR context + const prContext: PrContext = + resolvedEvent.kind === "complete" + ? resolvedEvent.context + : await githubClient.fetchPullRequest({ + prNumber: resolvedEvent.prNumber, + }) + + const postSkipReview = async (reason: string): Promise => { + const body = buildSkipBody(reason) + const { url } = await githubClient.submitReview({ + prNumber: prContext.prNumber, + commitId: prContext.headSha, + body, + comments: [], + fallbackBody: body, + }) + logger.info("posted skip review", { reason, reviewUrl: url }) + return { ...SKIPPED_RESULT_BASE, reviewUrl: url, skippedReason: reason } + } + + // Step 4: diff fetch + const diffResult = await githubClient.fetchDiff({ + prNumber: prContext.prNumber, + }) + if (diffResult.kind === "too_large") { + return postSkipReview("diff exceeds GitHub's diff API limits") + } + + // Step 5: parse diff + const files = parseDiff(diffResult.diff) + if (files.length === 0) { + return postSkipReview("empty diff") + } + + // Step 6: annotate + token check + const annotatedDiff = annotateDiff(files) + const diffTokens = estimateTokens(annotatedDiff) + const budgetHalf = Math.floor(config.contextBudgetTokens / 2) + if (diffTokens > budgetHalf) { + return postSkipReview( + `diff too large for context budget (${diffTokens} tokens, budget ${config.contextBudgetTokens})`, + ) + } + + // Step 7: commentable lines + const commentableByPath = computeCommentableLines(files) + + // Step 8: extract changed paths + const changedPaths = files + .map((file) => newFilePath(file)) + .filter((path): path is string => path !== null) + + // Step 9: context reads + const conventions = await contextReader.readConventions({ + conventionsFile: config.conventionsFile, + }) + + const { files: changedFiles, remainingTokens } = + await contextReader.readChangedFiles({ + changedPaths, + budgetTokens: config.contextBudgetTokens, + }) + + const relatedFiles = config.traceRelatedFiles + ? await contextReader.findRelatedFiles({ + changedPaths, + budgetTokens: remainingTokens, + }) + : [] + + // Step 10–11: generate findings (V1: single combined phase) + const phase = phases[0] + if (phase === undefined) { + throw new Error("resolvePhases returned no phases") + } + const structuredResult = await generateFindings({ + prContext, + phase, + conventions, + changedFiles, + relatedFiles, + annotatedDiff, + priorFindings: [], + }) + + const { modelUsed, attempts } = structuredResult + + // Step 12: select findings + const { selected, droppedByCap } = selectFindings({ + findings: structuredResult.review.findings, + severityThreshold, + maxFindings: config.maxFindings, + }) + + // Step 13: post review + const hasFindings = selected.length > 0 + + const reviewBody = hasFindings + ? (() => { + const { comments, bodyFindings } = mapFindingsToReview({ + findings: selected, + commentableByPath, + }) + const body = buildReviewBody({ + bodyFindings, + droppedByCap, + model: modelUsed, + }) + const fallbackBody = buildReviewBody({ + bodyFindings: selected, + droppedByCap, + model: modelUsed, + }) + return { body, comments, fallbackBody } + })() + : { + body: buildZeroFindingsBody({ model: modelUsed }), + comments: [], + fallbackBody: buildZeroFindingsBody({ model: modelUsed }), + } + + const { url: reviewUrl } = await githubClient.submitReview({ + prNumber: prContext.prNumber, + commitId: prContext.headSha, + ...reviewBody, + }) + + logger.info("review posted", { + reviewUrl, + findingsCount: selected.length, + modelUsed, + }) + + // Step 14: cost summary + const costSummaryMarkdown = renderCostSummary({ attempts, modelUsed }) + + return { + findingsCount: selected.length, + reviewUrl, + modelUsed, + skippedReason: "", + costSummaryMarkdown, + } +} + +export const createPromptedGenerateFindings = ( + { + openrouterClient, + model, + fallbackModel, + }: { + openrouterClient: OpenRouterClient + model: string + fallbackModel: string | null + }, + logger: Logger, +): GenerateFindings => { + const log = logger.child({ module: "generateFindings" }) + + return async (reviewContext) => { + const delimiterNonce = generateDelimiterNonce() + const systemPrompt = buildSystemPrompt({ phase: reviewContext.phase }) + const userPrompt = buildUserPrompt({ + ...reviewContext, + delimiterNonce, + }) + + log.info("requesting review", { model, fallbackModel }) + + return openrouterClient.requestReview({ + systemPrompt, + userPrompt, + model, + fallbackModel, + }) + } +} From c8ce304a4edd1c988601d62fd3f8e2ab0c95335d Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 11 Jul 2026 18:10:58 -0400 Subject: [PATCH 02/12] fix(review): budget accounting, workflow permissions, release concurrency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Subtract diffTokens from the file budget in orchestrate.ts so total prompt context respects the stated contextBudgetTokens cap (was passing the full budget to readChangedFiles, allowing ~1.5x overshoot) - release.yml: downgrade GITHUB_TOKEN to contents:read on the release job — only the app token does writes (git push, gh release) - release.yml: add concurrency block to serialize releases and prevent floating major-tag races on simultaneous tag pushes - self_review.yml: remove unused pull-requests:write on GITHUB_TOKEN — the app token handles review posting Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- .github/workflows/release.yml | 6 +++++- .github/workflows/self_review.yml | 1 - src/orchestrate.ts | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c12fe3a..1e36bcc 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -7,6 +7,10 @@ on: permissions: contents: read +concurrency: + group: release + cancel-in-progress: true + jobs: build-and-push: runs-on: ubuntu-latest @@ -48,7 +52,7 @@ jobs: needs: build-and-push runs-on: ubuntu-latest permissions: - contents: write + contents: read steps: - uses: actions/create-github-app-token@fee1f7d63c2ff003460e3d139729b119787bc349 # v2 id: app-token diff --git a/.github/workflows/self_review.yml b/.github/workflows/self_review.yml index 863f61c..3e9f3fd 100644 --- a/.github/workflows/self_review.yml +++ b/.github/workflows/self_review.yml @@ -25,7 +25,6 @@ jobs: ) permissions: contents: read - pull-requests: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 020cfab..32a9def 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -160,10 +160,11 @@ export const orchestrate = async ( conventionsFile: config.conventionsFile, }) + const fileBudgetTokens = config.contextBudgetTokens - diffTokens const { files: changedFiles, remainingTokens } = await contextReader.readChangedFiles({ changedPaths, - budgetTokens: config.contextBudgetTokens, + budgetTokens: fileBudgetTokens, }) const relatedFiles = config.traceRelatedFiles From 755eb4c3c611e8bc2eb7cccb409f480a4df5dff1 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 11 Jul 2026 18:17:40 -0400 Subject: [PATCH 03/12] style: extract IIFE, rename reviewBody, add JSDoc to exports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract IIFE in orchestrate step 13 into named buildReviewPayload function (callback-decomposition trigger: 3 named intermediates) - Rename reviewBody → reviewPayload (value is { body, comments, fallbackBody }, not just a body string) - Add JSDoc to orchestrate and createPromptedGenerateFindings exports Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- src/orchestrate.ts | 71 +++++++++++++++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 19 deletions(-) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 32a9def..424ae75 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -3,6 +3,7 @@ import type { ActionConfig } from "./config.js" import { computeCommentableLines, newFilePath, + type CommentableFile, } from "./diff/commentable-lines.js" import { annotateDiff } from "./diff/annotate-diff.js" import type { Logger } from "./logger.js" @@ -18,6 +19,7 @@ import { buildReviewBody, buildZeroFindingsBody, mapFindingsToReview, + type ReviewComment, } from "./review/comment-mapping.js" import { resolveSeverityThreshold, type Finding } from "./review/finding.js" import { resolvePhases, type ReviewPhase } from "./review/phases.js" @@ -64,6 +66,43 @@ export type OrchestrateDeps = { const buildSkipBody = (reason: string): string => `**umm-actually** — review skipped\n\n${reason}\n\n---\n*umm-actually*` +type ReviewPayload = { + body: string + comments: ReviewComment[] + fallbackBody: string +} + +/** Separates findings into inline comments and body-only findings, then + * builds a fallback body that includes all findings in case GitHub rejects + * the inline anchors. */ +const buildReviewPayload = ({ + findings, + commentableByPath, + droppedByCap, + modelUsed, +}: { + findings: Finding[] + commentableByPath: Map + droppedByCap: Finding[] + modelUsed: string +}): ReviewPayload => { + const { comments, bodyFindings } = mapFindingsToReview({ + findings, + commentableByPath, + }) + const body = buildReviewBody({ + bodyFindings, + droppedByCap, + model: modelUsed, + }) + const fallbackBody = buildReviewBody({ + bodyFindings: findings, + droppedByCap, + model: modelUsed, + }) + return { body, comments, fallbackBody } +} + const SKIPPED_RESULT_BASE: Omit< OrchestrateResult, "reviewUrl" | "skippedReason" @@ -73,6 +112,8 @@ const SKIPPED_RESULT_BASE: Omit< costSummaryMarkdown: null, } +/** Runs the full review pipeline — event resolution through review posting — + * with all I/O injected through deps so the pipeline is fully testable. */ export const orchestrate = async ( deps: OrchestrateDeps, logger: Logger, @@ -201,24 +242,13 @@ export const orchestrate = async ( // Step 13: post review const hasFindings = selected.length > 0 - const reviewBody = hasFindings - ? (() => { - const { comments, bodyFindings } = mapFindingsToReview({ - findings: selected, - commentableByPath, - }) - const body = buildReviewBody({ - bodyFindings, - droppedByCap, - model: modelUsed, - }) - const fallbackBody = buildReviewBody({ - bodyFindings: selected, - droppedByCap, - model: modelUsed, - }) - return { body, comments, fallbackBody } - })() + const reviewPayload: ReviewPayload = hasFindings + ? buildReviewPayload({ + findings: selected, + commentableByPath, + droppedByCap, + modelUsed, + }) : { body: buildZeroFindingsBody({ model: modelUsed }), comments: [], @@ -228,7 +258,7 @@ export const orchestrate = async ( const { url: reviewUrl } = await githubClient.submitReview({ prNumber: prContext.prNumber, commitId: prContext.headSha, - ...reviewBody, + ...reviewPayload, }) logger.info("review posted", { @@ -249,6 +279,9 @@ export const orchestrate = async ( } } +/** V1 one-shot strategy — builds a prompt from the review context and sends + * it to OpenRouter. V1.5/V2 will swap in different strategies behind the + * same GenerateFindings interface. */ export const createPromptedGenerateFindings = ( { openrouterClient, From 05738f1b1b31a19ab7de7333f3bfa9fead430fff Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 11 Jul 2026 18:28:21 -0400 Subject: [PATCH 04/12] test: tighten assertions and fill coverage gaps in orchestrate tests Fix assertion quality (exact match on hardcoded skip reason, deterministic cost summary values over expect.any(String), annotation-specific format check). Replace banned `!` non-null assertions with guards. Add 4 tests: skip review identity fields, budget-minus-diff wiring, remainingTokens passthrough, and changedPaths extraction from diff. 207 -> 211 tests. Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- src/__tests__/orchestrate.test.ts | 105 ++++++++++++++++++++++++++++-- 1 file changed, 99 insertions(+), 6 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 4e9438b..c6c2557 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -245,7 +245,7 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) - expect(result.skippedReason).toContain("diff exceeds") + expect(result.skippedReason).toBe("diff exceeds GitHub's diff API limits") expect(result.reviewUrl).toBe("https://github.com/test/review/1") expect(result.findingsCount).toBe(0) expect(result.modelUsed).toBe("") @@ -289,6 +289,21 @@ describe("orchestrate", () => { expect(stubs.generateFindingsCalls).toHaveLength(0) expect(stubs.submitReviewCalls).toHaveLength(1) }) + + it("passes correct prNumber and commitId in skip reviews", async () => { + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchDiff: async () => ({ kind: "too_large" as const }), + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + const reviewCall = stubs.submitReviewCalls[0] as Record + expect(reviewCall["prNumber"]).toBe(fixturePrContext.prNumber) + expect(reviewCall["commitId"]).toBe(fixturePrContext.headSha) + }) }) describe("happy path", () => { @@ -302,7 +317,8 @@ describe("orchestrate", () => { expect(result.reviewUrl).toBe("https://github.com/test/review/1") expect(result.modelUsed).toBe("test/model") expect(result.skippedReason).toBe("") - expect(result.costSummaryMarkdown).toEqual(expect.any(String)) + expect(result.costSummaryMarkdown).toContain("test/model") + expect(result.costSummaryMarkdown).toContain("$0.010000") expect(stubs.submitReviewCalls).toHaveLength(1) const reviewCall = stubs.submitReviewCalls[0] as Record @@ -337,7 +353,78 @@ describe("orchestrate", () => { unknown > const annotatedDiff = reviewContext["annotatedDiff"] as string - expect(annotatedDiff).toContain("src/greeter.ts") + expect(annotatedDiff).toContain("=== src/greeter.ts ===") + }) + }) + + describe("context wiring", () => { + it("passes budget minus diff tokens to readChangedFiles", async () => { + const readChangedFilesCalls: Record[] = [] + const stubs = makeOrchestrateDeps({ + contextReader: { + readChangedFiles: async (params) => { + readChangedFilesCalls.push(params) + return { files: [fixtureChangedFile], remainingTokens: 10_000 } + }, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(readChangedFilesCalls).toHaveLength(1) + const call = readChangedFilesCalls[0] as Record + const budgetTokens = call["budgetTokens"] as number + // Budget for files = contextBudgetTokens (80k) - diffTokens; diffTokens + // varies with fixture size but must be less than the full budget and the + // resulting file budget must be positive and less than the total. + expect(budgetTokens).toBeGreaterThan(0) + expect(budgetTokens).toBeLessThan(baseConfig.contextBudgetTokens) + }) + + it("passes remainingTokens from readChangedFiles to findRelatedFiles", async () => { + const expectedRemainingTokens = 12_345 + const findRelatedFilesCalls: Record[] = [] + const stubs = makeOrchestrateDeps({ + config: { traceRelatedFiles: true }, + contextReader: { + readChangedFiles: async () => ({ + files: [fixtureChangedFile], + remainingTokens: expectedRemainingTokens, + }), + findRelatedFiles: async (params) => { + findRelatedFilesCalls.push(params) + return [] + }, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(findRelatedFilesCalls).toHaveLength(1) + const call = findRelatedFilesCalls[0] as Record + expect(call["budgetTokens"]).toBe(expectedRemainingTokens) + }) + + it("passes paths extracted from the diff to readChangedFiles", async () => { + const stubs = makeOrchestrateDeps() + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.readChangedFilesCalls).toHaveLength(1) + const changedPaths = ( + stubs.readChangedFilesCalls[0] as Record + )["changedPaths"] as string[] + // The fixture diff modifies src/greeter.ts, adds src/added-file.ts, + // renames to src/new-name.ts, modifies src/no-trailing-newline.ts, and + // deletes src/removed-file.ts (null newFilePath) + binary logo.png + expect(changedPaths).toContain("src/greeter.ts") + expect(changedPaths).toContain("src/added-file.ts") + expect(changedPaths).toContain("src/new-name.ts") + // Deleted file has no newFilePath — should NOT appear + expect(changedPaths).not.toContain("src/removed-file.ts") }) }) @@ -475,10 +562,12 @@ describe("createPromptedGenerateFindings", () => { const { annotateDiff } = await import("../diff/annotate-diff.js") const { resolvePhases } = await import("../review/phases.js") const phases = resolvePhases("combined") + const phase = phases[0] + if (phase === undefined) throw new Error("expected a phase") await generate({ prContext: fixturePrContext, - phase: phases[0]!, + phase, conventions: "test conventions", changedFiles: [fixtureChangedFile], relatedFiles: [], @@ -517,11 +606,13 @@ describe("createPromptedGenerateFindings", () => { const { annotateDiff } = await import("../diff/annotate-diff.js") const { resolvePhases } = await import("../review/phases.js") const phases = resolvePhases("combined") + const phase = phases[0] + if (phase === undefined) throw new Error("expected a phase") const annotated = annotateDiff(files) await generate({ prContext: fixturePrContext, - phase: phases[0]!, + phase, conventions: null, changedFiles: [], relatedFiles: [], @@ -558,11 +649,13 @@ describe("createPromptedGenerateFindings", () => { const { annotateDiff } = await import("../diff/annotate-diff.js") const { resolvePhases } = await import("../review/phases.js") const phases = resolvePhases("combined") + const phase = phases[0] + if (phase === undefined) throw new Error("expected a phase") const annotated = annotateDiff(files) const context = { prContext: fixturePrContext, - phase: phases[0]!, + phase, conventions: null, changedFiles: [], relatedFiles: [], From 826f9fc4b076ac7746ac5b5eea2eb850a22c457e Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 11 Jul 2026 18:31:03 -0400 Subject: [PATCH 05/12] fix: restore collectRawInputs JSDoc, pass model var in self_review Restore the collectRawInputs JSDoc that documents the SDK boundary semantics (getBooleanInput YAML 1.2 enforcement, runner-materialized defaults). Add model input to self_review.yml from vars.OPENROUTER_MODEL so the repo variable takes effect instead of falling through to the action.yml default. Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- .github/workflows/self_review.yml | 1 + src/main.ts | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/.github/workflows/self_review.yml b/.github/workflows/self_review.yml index 3e9f3fd..2d39234 100644 --- a/.github/workflows/self_review.yml +++ b/.github/workflows/self_review.yml @@ -40,3 +40,4 @@ jobs: with: github_token: ${{ steps.app-token.outputs.token }} openrouter_api_key: ${{ secrets.OPENROUTER_KEY }} + model: ${{ vars.OPENROUTER_MODEL || 'anthropic/claude-sonnet-4-6' }} diff --git a/src/main.ts b/src/main.ts index ca7241c..b6cc590 100644 --- a/src/main.ts +++ b/src/main.ts @@ -11,6 +11,12 @@ import { createPromptedGenerateFindings, orchestrate } from "./orchestrate.js" const logger = createLogger("umm-actually") +/** + * Collects raw inputs at the SDK boundary. Strings come from getInput; + * booleans come pre-parsed from getBooleanInput, which enforces the strict + * YAML 1.2 core-schema list (true|True|TRUE / false|False|FALSE) and throws + * on anything else. Defaults from action.yml are materialized by the runner. + */ const collectRawInputs = (): RawInputs => ({ githubToken: core.getInput("github_token", { required: true }), openrouterApiKey: core.getInput("openrouter_api_key", { required: true }), From 10dc24bdb6f994cf959af57326576c3533eca4fb Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 11 Jul 2026 18:38:47 -0400 Subject: [PATCH 06/12] =?UTF-8?q?fix:=20correct=20context=5Fbudget=5Ftoken?= =?UTF-8?q?s=20description=20=E2=80=94=20conventions=20have=20a=20separate?= =?UTF-8?q?=20cap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The description claimed the budget covers "conventions + file contents + diff", but conventions are truncated independently at an 8k-token cap (CONVENTIONS_TOKEN_CAP in prompt.ts) and are not counted against the context_budget_tokens budget. A user relying on this description would underestimate their actual prompt size by up to 8k tokens. Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- README.md | 2 +- action.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e2cef88..e13004e 100644 --- a/README.md +++ b/README.md @@ -80,7 +80,7 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm | `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | | `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt | | `phases` | `combined` | Review phases to run. V1 supports: `combined` | -| `context_budget_tokens` | `80000` | Approximate token budget for prompt context (conventions + file contents + diff) | +| `context_budget_tokens` | `80000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | | `trace_related_files` | `true` | Include files that reference changed files in the prompt so the model can trace regressions into callers | | `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | | `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | diff --git a/action.yml b/action.yml index 4c9c45e..7fc1c02 100644 --- a/action.yml +++ b/action.yml @@ -37,7 +37,7 @@ inputs: required: false default: combined context_budget_tokens: - description: Approximate token budget for prompt context (conventions + file contents + diff) + description: Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) required: false default: "80000" trace_related_files: From 95ae1cccfd215ba9c0ef0e8f9adc24f902324ade Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:27:09 -0400 Subject: [PATCH 07/12] fix: fallback body heading, request bot as reviewer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildReviewBody now accepts optional heading/description params so the 422 fallback body says "Findings — inline comments were unavailable" instead of "Findings beyond the diff" (which mischaracterizes why findings are in the body when GitHub rejects inline comments). self_review.yml now requests umm-actually[bot] as a reviewer before running the action so the bot appears in the Reviewers sidebar rather than under "+1 more reviewer." Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- .github/workflows/self_review.yml | 8 ++++++++ src/orchestrate.ts | 3 +++ src/review/comment-mapping.ts | 8 ++++++-- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/workflows/self_review.yml b/.github/workflows/self_review.yml index 2d39234..5dcc62a 100644 --- a/.github/workflows/self_review.yml +++ b/.github/workflows/self_review.yml @@ -36,6 +36,14 @@ jobs: app-id: ${{ secrets.UMM_APP_ID }} private-key: ${{ secrets.UMM_PRIVATE_KEY }} + - name: Request review from umm-actually bot + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} + PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }} + run: | + gh api "repos/${{ github.repository }}/pulls/${PR_NUMBER}/requested_reviewers" \ + --method POST -f 'reviewers[]=umm-actually[bot]' || true + - uses: ./ with: github_token: ${{ steps.app-token.outputs.token }} diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 424ae75..c8bda9a 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -99,6 +99,9 @@ const buildReviewPayload = ({ bodyFindings: findings, droppedByCap, model: modelUsed, + bodyFindingsHeading: "Findings", + bodyFindingsDescription: + "Inline comments were unavailable; all findings are listed here:", }) return { body, comments, fallbackBody } } diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index bc6e658..f88a4ca 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -185,20 +185,24 @@ const renderBodyFinding = (finding: Finding): string => ${finding.description} **Failure scenario:** ${finding.failure_scenario}${suggestionBlock(finding)}` -/** The review's top-level body: beyond-diff findings, cap note, attribution. */ +/** The review's top-level body: body findings section, cap note, attribution. */ export const buildReviewBody = ({ bodyFindings, droppedByCap, model, + bodyFindingsHeading = "Findings beyond the diff", + bodyFindingsDescription = "These are in code the changes touch or depend on, outside the diff's line ranges:", }: { bodyFindings: Finding[] droppedByCap: Finding[] model: string + bodyFindingsHeading?: string + bodyFindingsDescription?: string }): string => { const beyondDiffSection = bodyFindings.length === 0 ? "" - : `### Findings beyond the diff\n\nThese are in code the changes touch or depend on, outside the diff's line ranges:\n\n${bodyFindings.map(renderBodyFinding).join("\n\n")}` + : `### ${bodyFindingsHeading}\n\n${bodyFindingsDescription}\n\n${bodyFindings.map(renderBodyFinding).join("\n\n")}` const capNote = droppedByCap.length === 0 From 92c3945d8909e34ae477a5b297cf18feaa91d6a9 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:34:55 -0400 Subject: [PATCH 08/12] =?UTF-8?q?fix:=20address=20bot=20review=20findings?= =?UTF-8?q?=20=E2=80=94=20permissions,=20types,=20review=20body?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Workflow permissions: - release.yml: restore contents: write, add permission-contents: write to app-token, add explicit persist-credentials: true on push checkout - self_review.yml: restore pull-requests: write, add permission-contents and permission-pull-requests to app-token for least-privilege scoping Review body: - buildReviewBody now shows "N finding(s) posted as inline comments" when all findings are inlined (was attribution-only, looked empty) - Skip message references effective half-budget limit instead of full Test types: - Recording arrays typed with actual param shapes, eliminating all 19 lint warnings (as casts, ! assertions) — 0 warnings across the suite - first() helper replaces unsafe array access patterns Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- .github/workflows/release.yml | 4 +- .github/workflows/self_review.yml | 3 + src/__tests__/orchestrate.test.ts | 167 ++++++++++++++++-------------- src/orchestrate.ts | 3 +- src/review/comment-mapping.ts | 11 +- 5 files changed, 108 insertions(+), 80 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1e36bcc..2c0a3ae 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -52,18 +52,20 @@ jobs: needs: build-and-push runs-on: ubuntu-latest permissions: - contents: read + contents: write steps: - uses: actions/create-github-app-token@fee1f7d63c2ff003460e3d139729b119787bc349 # v2 id: app-token with: app-id: ${{ secrets.RELEASE_APP_ID }} private-key: ${{ secrets.RELEASE_PRIVATE_KEY }} + permission-contents: write - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: token: ${{ steps.app-token.outputs.token }} ref: ${{ github.ref }} + persist-credentials: true - name: Flip action.yml image to digest-pinned GHCR reference env: diff --git a/.github/workflows/self_review.yml b/.github/workflows/self_review.yml index 5dcc62a..5285a78 100644 --- a/.github/workflows/self_review.yml +++ b/.github/workflows/self_review.yml @@ -25,6 +25,7 @@ jobs: ) permissions: contents: read + pull-requests: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -35,6 +36,8 @@ jobs: with: app-id: ${{ secrets.UMM_APP_ID }} private-key: ${{ secrets.UMM_PRIVATE_KEY }} + permission-contents: read + permission-pull-requests: write - name: Request review from umm-actually bot env: diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index c6c2557..548c285 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -12,11 +12,13 @@ import type { } from "../openrouter/client.js" import type { PromptFile } from "../review/prompt.js" import type { ReviewResponse } from "../review/finding.js" +import type { ReviewComment } from "../review/comment-mapping.js" import { orchestrate, createPromptedGenerateFindings, type OrchestrateDeps, type GenerateFindings, + type ReviewContext, } from "../orchestrate.js" import { createTestLogger } from "./test-logger.js" @@ -78,15 +80,46 @@ const baseConfig: ActionConfig = { prNumberOverride: undefined, } +type SubmitReviewParams = { + prNumber: number + commitId: string + body: string + comments: ReviewComment[] + fallbackBody: string +} + +type ReadChangedFilesParams = { + changedPaths: string[] + budgetTokens: number +} + +type FindRelatedFilesParams = { + changedPaths: string[] + budgetTokens: number +} + +type RequestReviewParams = { + systemPrompt: string + userPrompt: string + model: string + fallbackModel: string | null +} + +const first = (array: T[]): T => { + const item = array[0] + if (item === undefined) throw new Error("expected at least one element") + return item +} + type RecordingStubs = { deps: OrchestrateDeps - fetchPullRequestCalls: Record[] - fetchDiffCalls: Record[] - submitReviewCalls: Record[] - readConventionsCalls: Record[] - readChangedFilesCalls: Record[] - findRelatedFilesCalls: Record[] - generateFindingsCalls: unknown[] + fetchPullRequestCalls: { prNumber: number }[] + fetchDiffCalls: { prNumber: number }[] + submitReviewCalls: SubmitReviewParams[] + readConventionsCalls: { conventionsFile: string }[] + readChangedFilesCalls: ReadChangedFilesParams[] + findRelatedFilesCalls: FindRelatedFilesParams[] + generateFindingsCalls: ReviewContext[] } const makeOrchestrateDeps = ( @@ -100,13 +133,13 @@ const makeOrchestrateDeps = ( fixtureResult?: Partial } = {}, ): RecordingStubs => { - const fetchPullRequestCalls: Record[] = [] - const fetchDiffCalls: Record[] = [] - const submitReviewCalls: Record[] = [] - const readConventionsCalls: Record[] = [] - const readChangedFilesCalls: Record[] = [] - const findRelatedFilesCalls: Record[] = [] - const generateFindingsCalls: unknown[] = [] + const fetchPullRequestCalls: { prNumber: number }[] = [] + const fetchDiffCalls: { prNumber: number }[] = [] + const submitReviewCalls: SubmitReviewParams[] = [] + const readConventionsCalls: { conventionsFile: string }[] = [] + const readChangedFilesCalls: ReadChangedFilesParams[] = [] + const findRelatedFilesCalls: FindRelatedFilesParams[] = [] + const generateFindingsCalls: ReviewContext[] = [] const structuredResult: StructuredReviewResult = { review: fixtureReviewResponse, @@ -251,9 +284,9 @@ describe("orchestrate", () => { expect(result.modelUsed).toBe("") expect(stubs.generateFindingsCalls).toHaveLength(0) expect(stubs.submitReviewCalls).toHaveLength(1) - const reviewCall = stubs.submitReviewCalls[0] as Record - expect(reviewCall["comments"]).toEqual([]) - expect(reviewCall["body"]).toContain("diff exceeds") + const reviewCall = first(stubs.submitReviewCalls) + expect(reviewCall.comments).toEqual([]) + expect(reviewCall.body).toContain("diff exceeds") }) it("posts skip review when diff parses to zero files", async () => { @@ -270,8 +303,8 @@ describe("orchestrate", () => { expect(result.reviewUrl).toBe("https://github.com/test/review/1") expect(stubs.generateFindingsCalls).toHaveLength(0) expect(stubs.submitReviewCalls).toHaveLength(1) - const reviewCall = stubs.submitReviewCalls[0] as Record - expect(reviewCall["body"]).toContain("empty diff") + const reviewCall = first(stubs.submitReviewCalls) + expect(reviewCall.body).toContain("empty diff") }) it("posts skip review when annotated diff exceeds budget", async () => { @@ -300,9 +333,9 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) - const reviewCall = stubs.submitReviewCalls[0] as Record - expect(reviewCall["prNumber"]).toBe(fixturePrContext.prNumber) - expect(reviewCall["commitId"]).toBe(fixturePrContext.headSha) + const reviewCall = first(stubs.submitReviewCalls) + expect(reviewCall.prNumber).toBe(fixturePrContext.prNumber) + expect(reviewCall.commitId).toBe(fixturePrContext.headSha) }) }) @@ -321,9 +354,9 @@ describe("orchestrate", () => { expect(result.costSummaryMarkdown).toContain("$0.010000") expect(stubs.submitReviewCalls).toHaveLength(1) - const reviewCall = stubs.submitReviewCalls[0] as Record - expect(reviewCall["prNumber"]).toBe(7) - expect(reviewCall["commitId"]).toBe(fixturePrContext.headSha) + const reviewCall = first(stubs.submitReviewCalls) + expect(reviewCall.prNumber).toBe(7) + expect(reviewCall.commitId).toBe(fixturePrContext.headSha) }) it("passes changed files and conventions to generateFindings", async () => { @@ -333,13 +366,10 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) expect(stubs.generateFindingsCalls).toHaveLength(1) - const reviewContext = stubs.generateFindingsCalls[0] as Record< - string, - unknown - > - expect(reviewContext["conventions"]).toBe("# Test conventions") - expect(reviewContext["changedFiles"]).toEqual([fixtureChangedFile]) - expect(reviewContext["prContext"]).toEqual(fixturePrContext) + const reviewContext = first(stubs.generateFindingsCalls) + expect(reviewContext.conventions).toBe("# Test conventions") + expect(reviewContext.changedFiles).toEqual([fixtureChangedFile]) + expect(reviewContext.prContext).toEqual(fixturePrContext) }) it("includes annotated diff in review context", async () => { @@ -348,22 +378,18 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) - const reviewContext = stubs.generateFindingsCalls[0] as Record< - string, - unknown - > - const annotatedDiff = reviewContext["annotatedDiff"] as string - expect(annotatedDiff).toContain("=== src/greeter.ts ===") + const reviewContext = first(stubs.generateFindingsCalls) + expect(reviewContext.annotatedDiff).toContain("=== src/greeter.ts ===") }) }) describe("context wiring", () => { it("passes budget minus diff tokens to readChangedFiles", async () => { - const readChangedFilesCalls: Record[] = [] + const localReadChangedFilesCalls: ReadChangedFilesParams[] = [] const stubs = makeOrchestrateDeps({ contextReader: { readChangedFiles: async (params) => { - readChangedFilesCalls.push(params) + localReadChangedFilesCalls.push(params) return { files: [fixtureChangedFile], remainingTokens: 10_000 } }, }, @@ -372,19 +398,18 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) - expect(readChangedFilesCalls).toHaveLength(1) - const call = readChangedFilesCalls[0] as Record - const budgetTokens = call["budgetTokens"] as number + expect(localReadChangedFilesCalls).toHaveLength(1) + const call = first(localReadChangedFilesCalls) // Budget for files = contextBudgetTokens (80k) - diffTokens; diffTokens // varies with fixture size but must be less than the full budget and the // resulting file budget must be positive and less than the total. - expect(budgetTokens).toBeGreaterThan(0) - expect(budgetTokens).toBeLessThan(baseConfig.contextBudgetTokens) + expect(call.budgetTokens).toBeGreaterThan(0) + expect(call.budgetTokens).toBeLessThan(baseConfig.contextBudgetTokens) }) it("passes remainingTokens from readChangedFiles to findRelatedFiles", async () => { const expectedRemainingTokens = 12_345 - const findRelatedFilesCalls: Record[] = [] + const localFindRelatedFilesCalls: FindRelatedFilesParams[] = [] const stubs = makeOrchestrateDeps({ config: { traceRelatedFiles: true }, contextReader: { @@ -393,7 +418,7 @@ describe("orchestrate", () => { remainingTokens: expectedRemainingTokens, }), findRelatedFiles: async (params) => { - findRelatedFilesCalls.push(params) + localFindRelatedFilesCalls.push(params) return [] }, }, @@ -402,9 +427,9 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) - expect(findRelatedFilesCalls).toHaveLength(1) - const call = findRelatedFilesCalls[0] as Record - expect(call["budgetTokens"]).toBe(expectedRemainingTokens) + expect(localFindRelatedFilesCalls).toHaveLength(1) + const call = first(localFindRelatedFilesCalls) + expect(call.budgetTokens).toBe(expectedRemainingTokens) }) it("passes paths extracted from the diff to readChangedFiles", async () => { @@ -414,9 +439,7 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) expect(stubs.readChangedFilesCalls).toHaveLength(1) - const changedPaths = ( - stubs.readChangedFilesCalls[0] as Record - )["changedPaths"] as string[] + const changedPaths = first(stubs.readChangedFilesCalls).changedPaths // The fixture diff modifies src/greeter.ts, adds src/added-file.ts, // renames to src/new-name.ts, modifies src/no-trailing-newline.ts, and // deletes src/removed-file.ts (null newFilePath) + binary logo.png @@ -438,11 +461,8 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) expect(stubs.findRelatedFilesCalls).toHaveLength(0) - const reviewContext = stubs.generateFindingsCalls[0] as Record< - string, - unknown - > - expect(reviewContext["relatedFiles"]).toEqual([]) + const reviewContext = first(stubs.generateFindingsCalls) + expect(reviewContext.relatedFiles).toEqual([]) }) it("calls findRelatedFiles when traceRelatedFiles is true", async () => { @@ -468,9 +488,9 @@ describe("orchestrate", () => { expect(result.reviewUrl).toBe("https://github.com/test/review/1") expect(result.skippedReason).toBe("") expect(stubs.submitReviewCalls).toHaveLength(1) - const reviewCall = stubs.submitReviewCalls[0] as Record - expect(reviewCall["body"]).toContain("no findings above threshold") - expect(reviewCall["comments"]).toEqual([]) + const reviewCall = first(stubs.submitReviewCalls) + expect(reviewCall.body).toContain("no findings above threshold") + expect(reviewCall.comments).toEqual([]) }) it("respects maxFindings cap and includes dropped findings in body", async () => { @@ -482,9 +502,8 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) expect(result.findingsCount).toBe(1) - const reviewCall = stubs.submitReviewCalls[0] as Record - const body = reviewCall["body"] as string - expect(body).toContain("omitted") + const reviewCall = first(stubs.submitReviewCalls) + expect(reviewCall.body).toContain("omitted") }) it("returns costSummaryMarkdown when LLM call happened", async () => { @@ -516,11 +535,10 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) - const reviewCall = stubs.submitReviewCalls[0] as Record - const fallbackBody = reviewCall["fallbackBody"] as string - expect(fallbackBody).toContain("test/model") + const reviewCall = first(stubs.submitReviewCalls) + expect(reviewCall.fallbackBody).toContain("test/model") for (const finding of fixtureReviewResponse.findings) { - expect(fallbackBody).toContain(finding.title) + expect(reviewCall.fallbackBody).toContain(finding.title) } }) @@ -537,7 +555,7 @@ describe("orchestrate", () => { describe("createPromptedGenerateFindings", () => { it("passes model and fallbackModel to openrouterClient.requestReview", async () => { - const requestReviewCalls: Record[] = [] + const requestReviewCalls: RequestReviewParams[] = [] const stubClient: OpenRouterClient = { requestReview: async (params) => { requestReviewCalls.push(params) @@ -585,7 +603,7 @@ describe("createPromptedGenerateFindings", () => { }) it("includes annotated diff in the user prompt", async () => { - const requestReviewCalls: Record[] = [] + const requestReviewCalls: RequestReviewParams[] = [] const stubClient: OpenRouterClient = { requestReview: async (params) => { requestReviewCalls.push(params) @@ -620,18 +638,15 @@ describe("createPromptedGenerateFindings", () => { priorFindings: [], }) - const call = requestReviewCalls[0] as Record - const userPrompt = call["userPrompt"] as string - expect(userPrompt).toContain("src/greeter.ts") + const call = first(requestReviewCalls) + expect(call.userPrompt).toContain("src/greeter.ts") }) it("generates a unique nonce per call", async () => { const userPrompts: string[] = [] const stubClient: OpenRouterClient = { requestReview: async (params) => { - userPrompts.push( - (params as Record)["userPrompt"] as string, - ) + userPrompts.push(params.userPrompt) return { review: { analysis: "", findings: [] }, modelUsed: "m", diff --git a/src/orchestrate.ts b/src/orchestrate.ts index c8bda9a..37063fe 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -94,6 +94,7 @@ const buildReviewPayload = ({ bodyFindings, droppedByCap, model: modelUsed, + inlineCommentCount: comments.length, }) const fallbackBody = buildReviewBody({ bodyFindings: findings, @@ -187,7 +188,7 @@ export const orchestrate = async ( const budgetHalf = Math.floor(config.contextBudgetTokens / 2) if (diffTokens > budgetHalf) { return postSkipReview( - `diff too large for context budget (${diffTokens} tokens, budget ${config.contextBudgetTokens})`, + `diff too large for context budget (${diffTokens} tokens, limit ${budgetHalf} of ${config.contextBudgetTokens})`, ) } diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index f88a4ca..1e43acf 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -185,20 +185,27 @@ const renderBodyFinding = (finding: Finding): string => ${finding.description} **Failure scenario:** ${finding.failure_scenario}${suggestionBlock(finding)}` -/** The review's top-level body: body findings section, cap note, attribution. */ +/** The review's top-level body: summary, body findings section, cap note, attribution. */ export const buildReviewBody = ({ bodyFindings, droppedByCap, model, + inlineCommentCount = 0, bodyFindingsHeading = "Findings beyond the diff", bodyFindingsDescription = "These are in code the changes touch or depend on, outside the diff's line ranges:", }: { bodyFindings: Finding[] droppedByCap: Finding[] model: string + inlineCommentCount?: number bodyFindingsHeading?: string bodyFindingsDescription?: string }): string => { + const summaryLine = + inlineCommentCount > 0 && bodyFindings.length === 0 + ? `Reviewed — ${inlineCommentCount} finding(s) posted as inline comments.` + : "" + const beyondDiffSection = bodyFindings.length === 0 ? "" @@ -211,7 +218,7 @@ export const buildReviewBody = ({ const attribution = `---\n*umm-actually · ${model}*` - return [beyondDiffSection, capNote, attribution] + return [summaryLine, beyondDiffSection, capNote, attribution] .filter((section) => section !== "") .join("\n\n") } From 936817b966d047a02f1e534d2e4a7fa320f07268 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:45:09 -0400 Subject: [PATCH 09/12] fix: tighten budget test assertion, include rename old paths Budget test now asserts the exact computed value (contextBudgetTokens - sampleDiffTokens) instead of loose range checks. changedPaths now includes the old path for renames (file.from when both paths are real files) so findRelatedFiles catches callers that still reference the pre-rename path. Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- src/__tests__/orchestrate.test.ts | 13 +++++++------ src/orchestrate.ts | 13 +++++++++++-- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 548c285..a0f06ae 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -10,7 +10,8 @@ import type { OpenRouterClient, StructuredReviewResult, } from "../openrouter/client.js" -import type { PromptFile } from "../review/prompt.js" +import { estimateTokens, type PromptFile } from "../review/prompt.js" +import { annotateDiff } from "../diff/annotate-diff.js" import type { ReviewResponse } from "../review/finding.js" import type { ReviewComment } from "../review/comment-mapping.js" import { @@ -27,6 +28,8 @@ const sampleDiff = readFileSync( "utf8", ) +const sampleDiffTokens = estimateTokens(annotateDiff(parseDiff(sampleDiff))) + const pullRequestPayload: Record = JSON.parse( readFileSync( new URL("../../fixtures/pull_request.opened.json", import.meta.url), @@ -400,11 +403,9 @@ describe("orchestrate", () => { expect(localReadChangedFilesCalls).toHaveLength(1) const call = first(localReadChangedFilesCalls) - // Budget for files = contextBudgetTokens (80k) - diffTokens; diffTokens - // varies with fixture size but must be less than the full budget and the - // resulting file budget must be positive and less than the total. - expect(call.budgetTokens).toBeGreaterThan(0) - expect(call.budgetTokens).toBeLessThan(baseConfig.contextBudgetTokens) + expect(call.budgetTokens).toBe( + baseConfig.contextBudgetTokens - sampleDiffTokens, + ) }) it("passes remainingTokens from readChangedFiles to findRelatedFiles", async () => { diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 37063fe..f2b25e7 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -195,9 +195,18 @@ export const orchestrate = async ( // Step 7: commentable lines const commentableByPath = computeCommentableLines(files) - // Step 8: extract changed paths + // Step 8: extract changed paths (includes old path for renames so the + // import scanner finds callers that still reference the pre-rename path) const changedPaths = files - .map((file) => newFilePath(file)) + .flatMap((file) => { + const toPath = newFilePath(file) + const isRename = + toPath !== null && + file.from !== undefined && + file.from !== "/dev/null" && + file.from !== file.to + return isRename ? [toPath, file.from] : [toPath] + }) .filter((path): path is string => path !== null) // Step 9: context reads From 3a95d4938b032be02ab838bc9a4a2fceaebb29b2 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 00:32:44 -0400 Subject: [PATCH 10/12] test: tighten orchestrate assertions to exact values, codify convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md test conventions: when fixtures and stubs produce deterministic results, assert the entire return value or call params — fragments are the cheap option. orchestrate.test.ts: all happy path, skip path, zero findings, cap, and fallback body assertions now use toEqual on the full result object and full submitReview params (body, comments, fallbackBody) instead of individual field checks or toContain fragments. Expected values are precomputed from the same deterministic fixtures using the actual pipeline functions. Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- AGENTS.md | 6 +- src/__tests__/orchestrate.test.ts | 181 +++++++++++++++++++++++------- 2 files changed, 146 insertions(+), 41 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fbb447a..41a2a55 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,7 +90,11 @@ files. Prefer SDK-provided types over redefining shapes. - `const` per test via factory helpers; `beforeEach` only when per-test creation is genuinely impractical. - Exact assertions over loose matchers; assert whole values over substrings - when output is deterministic. + when output is deterministic. When fixtures and stubs produce deterministic + results, assert the entire return value or call params — not just individual + fields. Asserting fragments is the cheap option; asserting the whole value + catches drift in formatting, structure, and attribution that field-level + checks miss. - Two-bar rule: a test must (1) fail when the behavior breaks and (2) pass only because the intended behavior occurred. Guard against silent no-op, wrong-error, and early-return passes. diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index a0f06ae..4a93268 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -12,8 +12,16 @@ import type { } from "../openrouter/client.js" import { estimateTokens, type PromptFile } from "../review/prompt.js" import { annotateDiff } from "../diff/annotate-diff.js" +import { computeCommentableLines } from "../diff/commentable-lines.js" import type { ReviewResponse } from "../review/finding.js" -import type { ReviewComment } from "../review/comment-mapping.js" +import { + buildReviewBody, + buildZeroFindingsBody, + mapFindingsToReview, + type ReviewComment, +} from "../review/comment-mapping.js" +import { selectFindings } from "../review/select-findings.js" +import { renderCostSummary } from "../openrouter/cost-summary.js" import { orchestrate, createPromptedGenerateFindings, @@ -68,6 +76,60 @@ const fixtureChangedFile: PromptFile = { includedAs: "full", } +// Precomputed expected values from deterministic fixtures — used for exact +// assertions on the full result and submitReview params +const fixtureFiles = parseDiff(sampleDiff) +const fixtureCommentableByPath = computeCommentableLines(fixtureFiles) + +const expectedSelection = selectFindings({ + findings: fixtureReviewResponse.findings, + severityThreshold: "low", + maxFindings: undefined, +}) +const expectedMapped = mapFindingsToReview({ + findings: expectedSelection.selected, + commentableByPath: fixtureCommentableByPath, +}) +const expectedBody = buildReviewBody({ + bodyFindings: expectedMapped.bodyFindings, + droppedByCap: expectedSelection.droppedByCap, + model: "test/model", + inlineCommentCount: expectedMapped.comments.length, +}) +const expectedFallbackBody = buildReviewBody({ + bodyFindings: expectedSelection.selected, + droppedByCap: expectedSelection.droppedByCap, + model: "test/model", + bodyFindingsHeading: "Findings", + bodyFindingsDescription: + "Inline comments were unavailable; all findings are listed here:", +}) +const expectedCostSummary = renderCostSummary({ + attempts: [fixtureAttempt], + modelUsed: "test/model", +}) + +const expectedCappedSelection = selectFindings({ + findings: fixtureReviewResponse.findings, + severityThreshold: "low", + maxFindings: 1, +}) +const expectedCappedMapped = mapFindingsToReview({ + findings: expectedCappedSelection.selected, + commentableByPath: fixtureCommentableByPath, +}) +const expectedCappedBody = buildReviewBody({ + bodyFindings: expectedCappedMapped.bodyFindings, + droppedByCap: expectedCappedSelection.droppedByCap, + model: "test/model", + inlineCommentCount: expectedCappedMapped.comments.length, +}) + +const expectedZeroBody = buildZeroFindingsBody({ model: "test/model" }) + +const buildSkipBody = (reason: string): string => + `**umm-actually** — review skipped\n\n${reason}\n\n---\n*umm-actually*` + const baseConfig: ActionConfig = { githubToken: "ghp_test", openrouterApiKey: "sk-test", @@ -248,11 +310,13 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) - expect(result.skippedReason).toContain("unsupported event") - expect(result.reviewUrl).toBe("") - expect(result.findingsCount).toBe(0) - expect(result.modelUsed).toBe("") - expect(result.costSummaryMarkdown).toBeNull() + expect(result).toEqual({ + findingsCount: 0, + reviewUrl: "", + modelUsed: "", + skippedReason: "unsupported event: push", + costSummaryMarkdown: null, + }) expect(stubs.generateFindingsCalls).toHaveLength(0) expect(stubs.submitReviewCalls).toHaveLength(0) }) @@ -281,15 +345,23 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) - expect(result.skippedReason).toBe("diff exceeds GitHub's diff API limits") - expect(result.reviewUrl).toBe("https://github.com/test/review/1") - expect(result.findingsCount).toBe(0) - expect(result.modelUsed).toBe("") + const skipReason = "diff exceeds GitHub's diff API limits" + expect(result).toEqual({ + findingsCount: 0, + reviewUrl: "https://github.com/test/review/1", + modelUsed: "", + skippedReason: skipReason, + costSummaryMarkdown: null, + }) expect(stubs.generateFindingsCalls).toHaveLength(0) expect(stubs.submitReviewCalls).toHaveLength(1) - const reviewCall = first(stubs.submitReviewCalls) - expect(reviewCall.comments).toEqual([]) - expect(reviewCall.body).toContain("diff exceeds") + expect(first(stubs.submitReviewCalls)).toEqual({ + prNumber: fixturePrContext.prNumber, + commitId: fixturePrContext.headSha, + body: buildSkipBody(skipReason), + comments: [], + fallbackBody: buildSkipBody(skipReason), + }) }) it("posts skip review when diff parses to zero files", async () => { @@ -302,12 +374,23 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) - expect(result.skippedReason).toBe("empty diff") - expect(result.reviewUrl).toBe("https://github.com/test/review/1") + const skipReason = "empty diff" + expect(result).toEqual({ + findingsCount: 0, + reviewUrl: "https://github.com/test/review/1", + modelUsed: "", + skippedReason: skipReason, + costSummaryMarkdown: null, + }) expect(stubs.generateFindingsCalls).toHaveLength(0) expect(stubs.submitReviewCalls).toHaveLength(1) - const reviewCall = first(stubs.submitReviewCalls) - expect(reviewCall.body).toContain("empty diff") + expect(first(stubs.submitReviewCalls)).toEqual({ + prNumber: fixturePrContext.prNumber, + commitId: fixturePrContext.headSha, + body: buildSkipBody(skipReason), + comments: [], + fallbackBody: buildSkipBody(skipReason), + }) }) it("posts skip review when annotated diff exceeds budget", async () => { @@ -318,12 +401,24 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) - expect(result.skippedReason).toContain("diff too large") - expect(result.skippedReason).toContain("tokens") - expect(result.skippedReason).toContain("budget") - expect(result.reviewUrl).toBe("https://github.com/test/review/1") + const budgetHalf = Math.floor(10 / 2) + const skipReason = `diff too large for context budget (${sampleDiffTokens} tokens, limit ${budgetHalf} of 10)` + expect(result).toEqual({ + findingsCount: 0, + reviewUrl: "https://github.com/test/review/1", + modelUsed: "", + skippedReason: skipReason, + costSummaryMarkdown: null, + }) expect(stubs.generateFindingsCalls).toHaveLength(0) expect(stubs.submitReviewCalls).toHaveLength(1) + expect(first(stubs.submitReviewCalls)).toEqual({ + prNumber: fixturePrContext.prNumber, + commitId: fixturePrContext.headSha, + body: buildSkipBody(skipReason), + comments: [], + fallbackBody: buildSkipBody(skipReason), + }) }) it("passes correct prNumber and commitId in skip reviews", async () => { @@ -349,17 +444,22 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) - expect(result.findingsCount).toBe(fixtureReviewResponse.findings.length) - expect(result.reviewUrl).toBe("https://github.com/test/review/1") - expect(result.modelUsed).toBe("test/model") - expect(result.skippedReason).toBe("") - expect(result.costSummaryMarkdown).toContain("test/model") - expect(result.costSummaryMarkdown).toContain("$0.010000") + expect(result).toEqual({ + findingsCount: expectedSelection.selected.length, + reviewUrl: "https://github.com/test/review/1", + modelUsed: "test/model", + skippedReason: "", + costSummaryMarkdown: expectedCostSummary, + }) expect(stubs.submitReviewCalls).toHaveLength(1) - const reviewCall = first(stubs.submitReviewCalls) - expect(reviewCall.prNumber).toBe(7) - expect(reviewCall.commitId).toBe(fixturePrContext.headSha) + expect(first(stubs.submitReviewCalls)).toEqual({ + prNumber: fixturePrContext.prNumber, + commitId: fixturePrContext.headSha, + body: expectedBody, + comments: expectedMapped.comments, + fallbackBody: expectedFallbackBody, + }) }) it("passes changed files and conventions to generateFindings", async () => { @@ -489,9 +589,13 @@ describe("orchestrate", () => { expect(result.reviewUrl).toBe("https://github.com/test/review/1") expect(result.skippedReason).toBe("") expect(stubs.submitReviewCalls).toHaveLength(1) - const reviewCall = first(stubs.submitReviewCalls) - expect(reviewCall.body).toContain("no findings above threshold") - expect(reviewCall.comments).toEqual([]) + expect(first(stubs.submitReviewCalls)).toEqual({ + prNumber: fixturePrContext.prNumber, + commitId: fixturePrContext.headSha, + body: expectedZeroBody, + comments: [], + fallbackBody: expectedZeroBody, + }) }) it("respects maxFindings cap and includes dropped findings in body", async () => { @@ -503,8 +607,9 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) expect(result.findingsCount).toBe(1) + expect(stubs.submitReviewCalls).toHaveLength(1) const reviewCall = first(stubs.submitReviewCalls) - expect(reviewCall.body).toContain("omitted") + expect(reviewCall.body).toBe(expectedCappedBody) }) it("returns costSummaryMarkdown when LLM call happened", async () => { @@ -513,8 +618,7 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) - expect(result.costSummaryMarkdown).not.toBeNull() - expect(result.costSummaryMarkdown).toContain("test/model") + expect(result.costSummaryMarkdown).toBe(expectedCostSummary) }) it("returns null costSummaryMarkdown when skipped before LLM call", async () => { @@ -537,10 +641,7 @@ describe("orchestrate", () => { await orchestrate(stubs.deps, logger) const reviewCall = first(stubs.submitReviewCalls) - expect(reviewCall.fallbackBody).toContain("test/model") - for (const finding of fixtureReviewResponse.findings) { - expect(reviewCall.fallbackBody).toContain(finding.title) - } + expect(reviewCall.fallbackBody).toBe(expectedFallbackBody) }) it("uses complete PrContext from pull_request event without fetching", async () => { From e497e3dd179244b4041cf6d78a59c8cce9bf8324 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 00:47:05 -0400 Subject: [PATCH 11/12] =?UTF-8?q?docs:=20add=20Status=20section=20to=20REA?= =?UTF-8?q?DME=20=E2=80=94=20shipped,=20in=20progress,=20planned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surfaces what's working, what's being built (review dedup, doc-staleness, branded checks, vault-cortex swap), and what's on the roadmap (V1.5 verification tool, V2 agentic exploration). Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- README.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/README.md b/README.md index e13004e..d4b831a 100644 --- a/README.md +++ b/README.md @@ -105,6 +105,33 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm 7. Maps findings to inline PR review comments anchored to diff lines, with a snap-to-nearest-hunk fallback 8. Posts one consolidated review — findings that can't be inlined render in the review body +## Status + +umm-actually is in early development — the core review pipeline works but there's more to build. Here's what's shipped and what's in progress: + +**Shipped (V1)** + +- Single-pass review with inline findings anchored to diff lines +- Structured output with retry ladder and fallback model +- Import-tracing: changed code is traced into callers via reverse-import scan +- Token-budgeted context (changed files + related files + conventions) +- Prompt injection defense (randomized delimiter nonces) +- Skip-path handling with posted reasons (oversized diff, empty diff, API limits) +- Cost transparency (per-run model/token/USD report in workflow summary) +- `@umm review` comment trigger for on-demand re-reviews + +**In progress** + +- **Review dedup on re-runs** — currently each push posts a new review; working on deduplicating findings across runs and updating a single summary comment instead of creating new ones +- **Doc-staleness detection** — extending the workspace scan to doc files (`.md`, `.json`) so unchanged docs that describe changed code reach the prompt and staleness becomes a finding +- **Branded check run** — using the Checks API so the CI check shows the umm-actually avatar instead of the generic GitHub Actions logo +- **Review update on vault-cortex** — replacing PR-Agent with umm-actually as the review engine on the [vault-cortex](https://github.com/aliasunder/vault-cortex) project + +**Planned** + +- V1.5: `read_file` verification tool — the model can read additional files before finalizing findings +- V2: bounded agentic exploration — multi-step investigation with tool use behind a `generateFindings` seam + ## License [MIT](./LICENSE) From c2fe7bad86a15f78189104a9d32003a63337f679 Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sun, 12 Jul 2026 00:47:46 -0400 Subject: [PATCH 12/12] docs: remove internal vault-cortex reference from README Public OSS README should only contain items relevant to users of the action, not internal project details. Co-Authored-By: Claude Opus 4.6 (1M context) Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index d4b831a..784d01a 100644 --- a/README.md +++ b/README.md @@ -125,7 +125,6 @@ umm-actually is in early development — the core review pipeline works but ther - **Review dedup on re-runs** — currently each push posts a new review; working on deduplicating findings across runs and updating a single summary comment instead of creating new ones - **Doc-staleness detection** — extending the workspace scan to doc files (`.md`, `.json`) so unchanged docs that describe changed code reach the prompt and staleness becomes a finding - **Branded check run** — using the Checks API so the CI check shows the umm-actually avatar instead of the generic GitHub Actions logo -- **Review update on vault-cortex** — replacing PR-Agent with umm-actually as the review engine on the [vault-cortex](https://github.com/aliasunder/vault-cortex) project **Planned**