diff --git a/CHANGELOG.md b/CHANGELOG.md index b540971f7..f55b4c166 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Fixed +- **Review-agent compact diffs now use local git as the authoritative patch source.** `GetPRDiffContext` is built from the checked-out PR workspace (`origin/...HEAD`) instead of GitHub's potentially clipped `pulls.listFiles().patch` body, while GitHub remains the source of changed-file ordering and counts. Files without a locally verified patch are explicitly listed in `SKIPPED FILES`, skipped-file guidance now points to `cascade-tools scm get-pr-diff --prNumber --path `, and `PR context prepared` logs include patch-source counts, token counts, skip reasons, and bounded local-vs-GitHub hunk/size mismatches. See Linear issue [MNG-739](https://linear.app/issue/MNG-739). + - **Linear and JIRA inline checklist writes are now idempotent across provider/tool retries.** The shared markdown checklist engine now upserts by exact `### {Checklist Name}` heading and exact item text, merges duplicate inline sections into the first matching section, preserves non-checkbox prose from collapsed duplicate sections, preserves checked state when duplicate rows disagree, and keeps Trello on native checklist APIs. Linear also caches accepted description writes in-process so stale readback no longer overwrites or replays checklist appends into duplicate blocks. See Linear issue [MNG-741](https://linear.app/mongrel/issue/MNG-741/make-linear-checklist-mutations-idempotent-after-visibility-timeouts). - **`cascade-tools scm reply-to-review-comment` now accepts `--body-file`, and `create-pr-review --comment` handles one inline comment object ergonomically.** `ReplyToReviewComment` now exposes the same generated body file-input contract as the other SCM comment commands. Array-of-object CLI params still prefer JSON arrays, but one top-level JSON object is normalized to a one-item array, while `null` and primitive JSON values fail early with the structured `json-parse` envelope instead of reaching the GitHub client. See Linear issues [MNG-736](https://linear.app/mongrel/issue/MNG-736/fix-cascade-tools-scm-review-command-cli-drift), [MNG-731](https://linear.app/mongrel/issue/MNG-731/friction-tooling-low-cascade-tools-reply-to-review-comment-rejects), and [MNG-729](https://linear.app/mongrel/issue/MNG-729/friction-tooling-low-create-pr-review-comment-accepted-an-object-but). diff --git a/CLAUDE.md b/CLAUDE.md index f7eaf6134..08671a8c3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -146,9 +146,9 @@ The wedged-lock canary should never fire under normal operation. Its presence in Review agent receives a **compact per-file diff context**, not full file contents. Each changed file is a `### (, +N -M)` section with a unified diff hunk. Budget: `REVIEW_DIFF_CONTEXT_TOKEN_LIMIT` = 200k tokens, per-file cap 10%. -Files that can't fit (deleted, binary, oversized patch, or budget exhausted) are injected as `SKIPPED FILES` with instructions to fetch on demand via `gh pr diff`, `Read`, or `Grep`. +GitHub's changed-file API is used for file enumeration and change counts, but compact patch bodies come from the checked-out PR workspace via `git diff origin/...HEAD`. Files that can't fit or can't be locally verified (deleted, binary/no text patch, local diff failure/empty patch, oversized patch, or budget exhausted) are injected as `SKIPPED FILES` with instructions to fetch on demand via `cascade-tools scm get-pr-diff --prNumber --path `, `Read`, or `Grep`. -When review output misses something, check the `PR context prepared` log entry for `included` / `skipped` / `skipReasons` to confirm whether the file was visible to the agent. +When review output misses something, check the `PR context prepared` log entry for `included` / `skipped` / `skipReasons`, `patchSources`, `totalDiffTokens`, `perFileTokenCap`, and `localGitMismatches` to confirm whether the file was visible to the agent and whether GitHub's API patch differed from the local patch. Also check context offload logs if the diff context was written under `.cascade/context/`. ## Engines diff --git a/docs/architecture/03-trigger-system.md b/docs/architecture/03-trigger-system.md index 39e1a1d29..c8812535b 100644 --- a/docs/architecture/03-trigger-system.md +++ b/docs/architecture/03-trigger-system.md @@ -201,7 +201,7 @@ Each trigger in a YAML agent definition can declare a `contextPipeline` — an o | `contextFiles` | Read key project files (README, etc.) | | `workItem` | Fetch work item details from PM tool | | `prepopulateTodos` | Pre-populate todo list from work item checklists | -| `prContext` | Fetch PR details, compact per-file diffs, CI checks; emit a `SKIPPED FILES` injection when files are omitted (over budget, deleted, binary) | +| `prContext` | Fetch PR details, GitHub changed-file metadata, locally verified compact per-file diffs from `origin/...HEAD`, and CI checks; emit a `SKIPPED FILES` injection when files are omitted (over budget, deleted, binary/no patch, or local diff unavailable) | | `prConversation` | Fetch PR comments and review threads | | `pipelineSnapshot` | Fetch CI pipeline status | | `alertingIssue` | Fetch Sentry issue and event details | diff --git a/docs/architecture/04-agent-system.md b/docs/architecture/04-agent-system.md index 8cbb6144a..b2b82ac4f 100644 --- a/docs/architecture/04-agent-system.md +++ b/docs/architecture/04-agent-system.md @@ -212,6 +212,18 @@ Pipeline prompts receive separate PM identifiers for selection and creation: For Trello, BACKLOG is a list, so `backlogStatusId` and `workItemCreateContainerId` are both the backlog list ID. For JIRA, `backlogStatusId` is `jira.statuses.backlog` and creation uses `jira.projectKey`. For Linear, `backlogStatusId` is `linear.statuses.backlog` and creation uses `linear.teamId`; backlog-manager must not use the Linear team ID to discover candidate backlog issues. +### Alert task prompt context + +Alerting task prompts can reference scalar alert fields passed through `AgentInput`: + +| Variable | Purpose | +|----------|---------| +| `alertTitle` | Provider-normalized alert title, with empty and stringified `undefined`/`null` candidates discarded | +| `alertIssueUrl` | Human-facing Sentry issue or alert permalink when available | +| `alertIssueId` | Sentry issue ID for issue/event alerts | +| `alertOrgId` | Sentry organization slug used for alerting API reads | +| `alertMetricKey` | Stable metric-alert key (`orgSlug:title`) used by worker-side materialization | + ## Hooks ### Trailing hooks diff --git a/docs/architecture/07-gadgets.md b/docs/architecture/07-gadgets.md index f9167397f..db0479424 100644 --- a/docs/architecture/07-gadgets.md +++ b/docs/architecture/07-gadgets.md @@ -115,7 +115,7 @@ cascade-tools pm report-friction \ | `GetAlertingEventDetail` | `alerting:read` | Fetch Sentry issue-event details with stacktrace, tags, breadcrumbs, request data, and context | | `ListAlertingEvents` | `alerting:read` | List recent events for an issue | -`GetAlertingEventDetail` accepts Sentry's issue-event response shape, including REST aliases from the [Retrieve an Issue Event API](https://docs.sentry.io/api/events/retrieve-an-issue-event/). +`GetAlertingEventDetail` accepts Sentry's issue-event response shape, including REST aliases from the [Retrieve an Issue Event API](https://docs.sentry.io/api/events/retrieve-an-issue-event/). It fetches issue metadata best-effort and includes `Sentry issue: ` near the top when Sentry returns a permalink; if that metadata request fails, the event details still render. ## cascade-tools CLI diff --git a/package-lock.json b/package-lock.json index b2caac049..2df82b87b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3287,7 +3287,9 @@ "license": "BSD-3-Clause" }, "node_modules/@protobufjs/codegen": { - "version": "2.0.4", + "version": "2.0.5", + "resolved": "https://registry.npmjs.org/@protobufjs/codegen/-/codegen-2.0.5.tgz", + "integrity": "sha512-zgXFLzW3Ap33e6d0Wlj4MGIm6Ce8O89n/apUaGNB/jx+hw+ruWEp7EwGUshdLKVRCxZW12fp9r40E1mQrf/34g==", "license": "BSD-3-Clause" }, "node_modules/@protobufjs/eventemitter": { @@ -3307,7 +3309,9 @@ "license": "BSD-3-Clause" }, "node_modules/@protobufjs/inquire": { - "version": "1.1.0", + "version": "1.1.1", + "resolved": "https://registry.npmjs.org/@protobufjs/inquire/-/inquire-1.1.1.tgz", + "integrity": "sha512-mnzgDV26ueAvk7rsbt9L7bE0SuAoqyuys/sMMrmVcN5x9VsxpcG3rqAUSgDyLp0UZlmNfIbQ4fHfCtreVBk8Ew==", "license": "BSD-3-Clause" }, "node_modules/@protobufjs/path": { @@ -3319,7 +3323,9 @@ "license": "BSD-3-Clause" }, "node_modules/@protobufjs/utf8": { - "version": "1.1.0", + "version": "1.1.1", + "resolved": "https://registry.npmjs.org/@protobufjs/utf8/-/utf8-1.1.1.tgz", + "integrity": "sha512-oOAWABowe8EAbMyWKM0tYDKi8Yaox52D+HWZhAIJqQXbqe0xI/GV7FhLWqlEKreMkfDjshR5FKgi3mnle0h6Eg==", "license": "BSD-3-Clause" }, "node_modules/@rollup/rollup-android-arm-eabi": { @@ -9397,22 +9403,22 @@ "license": "MIT" }, "node_modules/protobufjs": { - "version": "7.5.5", - "resolved": "https://registry.npmjs.org/protobufjs/-/protobufjs-7.5.5.tgz", - "integrity": "sha512-3wY1AxV+VBNW8Yypfd1yQY9pXnqTAN+KwQxL8iYm3/BjKYMNg4i0owhEe26PWDOMaIrzeeF98Lqd5NGz4omiIg==", + "version": "7.5.8", + "resolved": "https://registry.npmjs.org/protobufjs/-/protobufjs-7.5.8.tgz", + "integrity": "sha512-dvpCIeLPbXZS/Ete7yLaO7RenOdken2NHKykBXbsaGxZT0UTltcarBciw+A78SRQs9iMAAVpsYA+l8b1hTePIA==", "hasInstallScript": true, "license": "BSD-3-Clause", "dependencies": { "@protobufjs/aspromise": "^1.1.2", "@protobufjs/base64": "^1.1.2", - "@protobufjs/codegen": "^2.0.4", + "@protobufjs/codegen": "^2.0.5", "@protobufjs/eventemitter": "^1.1.0", "@protobufjs/fetch": "^1.1.0", "@protobufjs/float": "^1.0.2", - "@protobufjs/inquire": "^1.1.0", + "@protobufjs/inquire": "^1.1.1", "@protobufjs/path": "^1.1.2", "@protobufjs/pool": "^1.1.0", - "@protobufjs/utf8": "^1.1.0", + "@protobufjs/utf8": "^1.1.1", "@types/node": ">=13.7.0", "long": "^5.0.0" }, diff --git a/package.json b/package.json index 888931e94..843cdbc4f 100644 --- a/package.json +++ b/package.json @@ -138,6 +138,7 @@ "lodash": "^4.18.1", "lodash-es": "^4.18.1", "brace-expansion": "^2.0.3", - "axios": "^1.15.0" + "axios": "^1.15.0", + "protobufjs": "^7.5.8" } } diff --git a/src/agents/definitions/alerting.yaml b/src/agents/definitions/alerting.yaml index 6d5fec23f..601be8271 100644 --- a/src/agents/definitions/alerting.yaml +++ b/src/agents/definitions/alerting.yaml @@ -54,7 +54,7 @@ strategies: {} prompts: taskPrompt: | - An alert has been triggered: "<%= it.alertTitle %>" + An alert has been triggered: "<%= it.alertTitle || it.workItemTitle || it.alertIssueId || 'Sentry alert' %>" <% if (it.alertIssueUrl) { %>Issue: <%= it.alertIssueUrl %><% } %> diff --git a/src/agents/definitions/contextSteps.ts b/src/agents/definitions/contextSteps.ts index 069e276d7..5f43beb8d 100644 --- a/src/agents/definitions/contextSteps.ts +++ b/src/agents/definitions/contextSteps.ts @@ -23,12 +23,12 @@ import { getSentryClient } from '../../sentry/client.js'; import type { AgentInput, ProjectConfig } from '../../types/index.js'; import { parseRepoFullName } from '../../utils/repo.js'; import type { ContextInjection, LogWriter } from '../contracts/index.js'; +import { sourceLocalPRDiffs } from '../shared/prDiffSource.js'; import { countSkipsByReason, extractPRDiffs, formatPRComments, formatPRDetails, - formatPRDiff, formatPRDiffContext, formatPRIssueComments, formatPRReviews, @@ -156,7 +156,6 @@ export async function fetchPRContextStep(params: FetchContextParams): Promise>((acc, file) => { + acc[file.patchSource] = (acc[file.patchSource] ?? 0) + 1; + return acc; + }, {}); params.logWriter('INFO', 'PR context prepared', { included: diffContext.included.length, skipped: diffContext.skipped.length, skipReasons, + patchSources, + totalDiffTokens: diffContext.totalDiffTokens, + perFileTokenCap: diffContext.perFileTokenCap, + localGitMismatches: localDiffSource.mismatches.slice(0, 20), }); injections.push({ @@ -564,7 +574,7 @@ export async function fetchPipelineSnapshotStep( export async function fetchAlertingIssueStep( params: FetchContextParams, ): Promise { - const { alertIssueId, alertOrgId } = params.input; + const { alertIssueId, alertOrgId, alertIssueUrl, alertTitle } = params.input; if (!alertIssueId || typeof alertIssueId !== 'string') return []; if (!alertOrgId || typeof alertOrgId !== 'string') return []; @@ -576,7 +586,15 @@ export async function fetchAlertingIssueStep( const client = getSentryClient(); const event = await client.getIssueEvent(alertOrgId, alertIssueId, 'latest'); - const result = formatSentryEvent(event); + const issue = + typeof alertIssueUrl === 'string' && alertIssueUrl.trim() + ? { + id: alertIssueId, + permalink: alertIssueUrl, + title: typeof alertTitle === 'string' ? alertTitle : undefined, + } + : await client.getIssue(alertOrgId, alertIssueId).catch(() => undefined); + const result = formatSentryEvent(event, issue); params.logWriter('INFO', 'fetchAlertingIssueStep: fetched alerting event successfully', { issueId: alertIssueId, diff --git a/src/agents/definitions/review.yaml b/src/agents/definitions/review.yaml index f042a860f..78fb306a9 100644 --- a/src/agents/definitions/review.yaml +++ b/src/agents/definitions/review.yaml @@ -70,12 +70,17 @@ prompts: diff hunk. Only the changed lines are in your context; unchanged parts of a file are not echoed. Use `Read ` if you need surrounding code. + GitHub is used to enumerate changed files, but compact patch bodies are + produced from the checked-out local PR workspace so the context is not + limited by GitHub's `pulls.listFiles().patch` truncation behavior. + If a **`SKIPPED FILES`** injection is present, those files were omitted from - the compact context (because they were deleted, binary, their patch was too - large, or the cumulative diff budget was reached). When any skipped file is - relevant to your review, fetch it on demand: + the compact context (because they were deleted, binary, could not be locally + diffed, their patch was too large, or the cumulative diff budget was + reached). When any skipped file is relevant to your review, fetch it on + demand: - - `gh pr diff <%= it.prNumber %> -- ` to read the file's patch + - `cascade-tools scm get-pr-diff --prNumber <%= it.prNumber %> --path ` to read the file's patch - `Read ` to read the post-PR file content - `Grep ` to search inside the file diff --git a/src/agents/prompts/index.ts b/src/agents/prompts/index.ts index 5cfeb953b..f28bdcca6 100644 --- a/src/agents/prompts/index.ts +++ b/src/agents/prompts/index.ts @@ -219,10 +219,16 @@ export interface TaskPromptInput { * Null handling: all optional fields remain undefined when not present (no 'unknown' defaults). */ export function buildTaskPromptContext(input: TaskPromptInput): TaskPromptContext { + const context: TaskPromptContext = {}; + for (const [key, value] of Object.entries(input)) { + if (key === 'project' || key === 'config') continue; + if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean') { + context[key] = value; + } + } + return { - workItemId: input.workItemId, - prNumber: input.prNumber, - prBranch: input.prBranch, + ...context, commentText: input.triggerCommentBody ?? input.triggerCommentText, commentAuthor: input.triggerCommentAuthor, commentBody: input.triggerCommentBody ?? input.triggerCommentText, @@ -386,6 +392,23 @@ export function getTaskTemplateVariables(): Array<{ { name: 'workItemId', group: 'Work Item', description: 'Work item ID (card or issue)' }, { name: 'commentText', group: 'Comment', description: 'Comment text content (PM comments)' }, { name: 'commentAuthor', group: 'Comment', description: 'Comment author username' }, + { + name: 'alertTitle', + group: 'Alerting', + description: 'Alert title from the alerting provider', + }, + { + name: 'alertIssueUrl', + group: 'Alerting', + description: 'Human-facing URL for the alerting issue or alert', + }, + { name: 'alertIssueId', group: 'Alerting', description: 'Alerting provider issue ID' }, + { name: 'alertOrgId', group: 'Alerting', description: 'Alerting provider organization ID' }, + { + name: 'alertMetricKey', + group: 'Alerting', + description: 'Stable metric alert key used for metric alert materialization', + }, { name: 'prNumber', group: 'PR', description: 'Pull request number' }, { name: 'prBranch', group: 'PR', description: 'Pull request branch name' }, { name: 'commentBody', group: 'PR Comment', description: 'PR comment body text' }, diff --git a/src/agents/prompts/templates/resolve-conflicts.eta b/src/agents/prompts/templates/resolve-conflicts.eta index c828a3f3b..cabac6809 100644 --- a/src/agents/prompts/templates/resolve-conflicts.eta +++ b/src/agents/prompts/templates/resolve-conflicts.eta @@ -10,7 +10,7 @@ You are an expert software engineer resolving merge conflicts on a pull request. 2. **Review PR details** - Pre-loaded for you: - PR details (GetPRDetails) — includes base branch and head branch - - PR diff (GetPRDiff) + - PR diff context (GetPRDiffContext) 3. **Identify conflicting files** - Run `git status` to see which files have conflict markers 4. **Read codebase guidelines** (CLAUDE.md, README.md, etc.) for conventions diff --git a/src/agents/prompts/templates/respond-to-ci.eta b/src/agents/prompts/templates/respond-to-ci.eta index 6c5ab4d28..3d80e108e 100644 --- a/src/agents/prompts/templates/respond-to-ci.eta +++ b/src/agents/prompts/templates/respond-to-ci.eta @@ -10,7 +10,7 @@ You are an expert software engineer fixing CI check failures on a pull request. 2. **Review CI failures** - Pre-loaded for you: - PR details (GetPRDetails) - - PR diff (GetPRDiff) + - PR diff context (GetPRDiffContext) - Check run status summary (GetPRChecks) 3. **Get failed check logs** - Use the `GetCIRunLogs` gadget with the PR's head SHA to fetch failed CI run logs 4. **Read codebase guidelines** (CLAUDE.md, README.md, etc.) for conventions diff --git a/src/agents/prompts/templates/respond-to-pr-comment.eta b/src/agents/prompts/templates/respond-to-pr-comment.eta index 44472d08d..fea27e1d8 100644 --- a/src/agents/prompts/templates/respond-to-pr-comment.eta +++ b/src/agents/prompts/templates/respond-to-pr-comment.eta @@ -15,7 +15,7 @@ A user @mentioned you in a PR comment. Read their request and do exactly what th - Line-specific review comments (GetPRComments) - Review submissions (GetPRReviews) - General PR comments (GetPRIssueComments) - - PR diff (GetPRDiff) + - PR diff context (GetPRDiffContext) 4. **Read codebase guidelines** (CLAUDE.md, README.md, etc.) to understand conventions ### Phase 3: Plan & Execute diff --git a/src/agents/prompts/templates/review.eta b/src/agents/prompts/templates/review.eta index 06325c7a0..55583b973 100644 --- a/src/agents/prompts/templates/review.eta +++ b/src/agents/prompts/templates/review.eta @@ -198,7 +198,7 @@ Use CreatePRReview with: - `body`: Summary of findings (or "LGTM" if no issues) - `comments`: Inline comments for specific issues -**CRITICAL**: Inline comments can ONLY be placed on files in the PR diff. If you comment on a file path not in the diff, or use line numbers outside the changed ranges, the entire review will fail. Only add inline comments for files you saw in GetPRDiff output. +**CRITICAL**: Inline comments can ONLY be placed on files in the PR diff. If you comment on a file path not in the diff, or use line numbers outside the changed ranges, the entire review will fail. Only add inline comments for files you saw in GetPRDiffContext output or files you verified by fetching a skipped file on demand via `cascade-tools scm get-pr-diff --path`. ### Review Body Format diff --git a/src/agents/shared/modelResolution.ts b/src/agents/shared/modelResolution.ts index 28d1f149c..0a89d7d5f 100644 --- a/src/agents/shared/modelResolution.ts +++ b/src/agents/shared/modelResolution.ts @@ -35,6 +35,21 @@ export interface ResolveModelConfigOptions { agentInput?: AgentInput; } +function buildResolveTaskContext( + agentInput: AgentInput | undefined, + promptContext: PromptContext | undefined, +): PromptContext { + return { + ...promptContext, + ...buildTaskPromptContext({ + ...(agentInput ?? {}), + workItemId: agentInput?.workItemId ?? promptContext?.workItemId, + prNumber: agentInput?.prNumber ?? (promptContext?.prNumber as number | undefined), + prBranch: agentInput?.prBranch ?? (promptContext?.prBranch as string | undefined), + }), + }; +} + export async function resolveModelConfig(options: ResolveModelConfigOptions): Promise { const { agentType, project, repoDir, modelOverride, promptContext, dbPartials } = options; const configKey = options.configKey ?? agentType; @@ -78,21 +93,7 @@ export async function resolveModelConfig(options: ResolveModelConfigOptions): Pr const maxIterations = project.maxIterations; // Build task context (shared between project and definition task prompt rendering) - const taskContext = { - // Forward all prompt context (PM list IDs, vocabulary, etc.) so task - // prompts can reference any system-level variable via Eta. - ...promptContext, - // Task-specific fields from agentInput override prompt context - ...buildTaskPromptContext({ - workItemId: options.agentInput?.workItemId ?? promptContext?.workItemId, - prNumber: options.agentInput?.prNumber ?? (promptContext?.prNumber as number | undefined), - prBranch: options.agentInput?.prBranch ?? (promptContext?.prBranch as string | undefined), - triggerCommentText: options.agentInput?.triggerCommentText, - triggerCommentAuthor: options.agentInput?.triggerCommentAuthor, - triggerCommentBody: options.agentInput?.triggerCommentBody, - triggerCommentPath: options.agentInput?.triggerCommentPath, - }), - }; + const taskContext = buildResolveTaskContext(options.agentInput, promptContext); // Resolve task prompt: project override → definition override → undefined (use .eta default) let taskPrompt: string | undefined; diff --git a/src/agents/shared/prDiffSource.ts b/src/agents/shared/prDiffSource.ts new file mode 100644 index 000000000..5c7e621a0 --- /dev/null +++ b/src/agents/shared/prDiffSource.ts @@ -0,0 +1,216 @@ +import type { PRDiffFile } from '../../github/client.js'; +import { runCommand } from '../../utils/repo.js'; +import type { LogWriter } from '../contracts/index.js'; + +export type PatchSourceStatus = 'local-git' | 'local-diff-empty' | 'local-diff-failed' | 'no-patch'; + +export interface EnrichedPRDiffFile extends PRDiffFile { + patchSource: PatchSourceStatus; + localPatchChars: number; + githubPatchChars: number; + localHunkCount: number; + githubHunkCount: number; + localDiffError?: string; +} + +export interface DiffSourceMismatch { + filename: string; + localPatchChars: number; + githubPatchChars: number; + localHunkCount: number; + githubHunkCount: number; +} + +export interface PRDiffSourceResult { + files: EnrichedPRDiffFile[]; + mismatches: DiffSourceMismatch[]; +} + +function countDiffHunks(patch: string | undefined): number { + if (!patch) return 0; + return patch.match(/^@@ /gm)?.length ?? 0; +} + +function hasMismatch(file: EnrichedPRDiffFile): boolean { + return ( + file.patchSource === 'local-git' && + (file.localPatchChars !== file.githubPatchChars || file.localHunkCount !== file.githubHunkCount) + ); +} + +function summarizeFailure(stderr: string, stdout: string): string { + const text = (stderr || stdout || 'git diff failed').trim(); + return text.length > 500 ? `${text.slice(0, 500)}...` : text; +} + +/** + * Build the fail-closed result when `git fetch origin ` fails. + * With a stale origin/, per-file diffs can succeed against the old ref + * and include base-branch-only commits — producing a misleading 'local-git' + * patch. Deleted files get 'no-patch'; all others get 'local-diff-failed' so + * the review agent can fetch verified patches on demand. + */ +function buildFetchFailedFiles(files: PRDiffFile[], fetchError: string): EnrichedPRDiffFile[] { + return files.map((file) => { + const githubPatchChars = file.patch?.length ?? 0; + const githubHunkCount = countDiffHunks(file.patch); + if (file.status === 'removed') { + return { + ...file, + patchSource: 'no-patch' as const, + localPatchChars: 0, + githubPatchChars, + localHunkCount: 0, + githubHunkCount, + patch: undefined, + }; + } + return { + ...file, + patch: undefined, + patchSource: 'local-diff-failed' as const, + localPatchChars: 0, + githubPatchChars, + localHunkCount: 0, + githubHunkCount, + localDiffError: `base-ref fetch failed: ${fetchError}`, + }; + }); +} + +export async function sourceLocalPRDiffs(params: { + files: PRDiffFile[]; + repoDir: string; + baseBranch: string; + logWriter: LogWriter; +}): Promise { + const enriched: EnrichedPRDiffFile[] = []; + const mismatches: DiffSourceMismatch[] = []; + + // Refresh origin/ so stale snapshot refs don't produce patches + // that include base-branch commits that aren't part of this PR. + // Snapshot-reuse PR setup only fetches refs/pull/N/head; origin/ can + // lag. If the fetch fails (no network, no remote) we fail closed — see the + // handler below. + const fetchResult = await runCommand( + 'git', + ['fetch', 'origin', params.baseBranch], + params.repoDir, + undefined, + { silent: true, label: `fetch-base-branch:${params.baseBranch}` }, + ); + if (fetchResult.exitCode !== 0) { + const fetchError = summarizeFailure(fetchResult.stderr, fetchResult.stdout); + params.logWriter('WARN', 'Failed to refresh base branch ref before local diff', { + baseBranch: params.baseBranch, + exitCode: fetchResult.exitCode, + reason: fetchResult.reason, + error: fetchError, + }); + return { files: buildFetchFailedFiles(params.files, fetchError), mismatches }; + } + + for (const file of params.files) { + const githubPatchChars = file.patch?.length ?? 0; + const githubHunkCount = countDiffHunks(file.patch); + if (file.status === 'removed') { + enriched.push({ + ...file, + patchSource: 'no-patch', + localPatchChars: 0, + githubPatchChars, + localHunkCount: 0, + githubHunkCount, + patch: undefined, + }); + continue; + } + + // For renamed files, supply both the old and new literal paths so git + // can emit proper 'rename from / rename to' metadata. With only the + // destination path, git emits 'new file mode' and marks every line as + // added — a misleading patch for a pure or near-pure rename. The old + // path is always listed first (before the new path) so git's rename + // detection algorithm pairs them correctly. + const pathspecs: string[] = [`:(literal)${file.filename}`]; + if (file.previousFilename) { + pathspecs.unshift(`:(literal)${file.previousFilename}`); + } + + const result = await runCommand( + 'git', + [ + 'diff', + '--no-color', + '--no-ext-diff', + '--find-renames', + '--find-copies', + `origin/${params.baseBranch}...HEAD`, + '--', + ...pathspecs, + ], + params.repoDir, + undefined, + { silent: true, label: `local-pr-diff:${file.filename}` }, + ); + + if (result.exitCode !== 0) { + const localDiffError = summarizeFailure(result.stderr, result.stdout); + params.logWriter('WARN', 'Local PR diff failed for changed file', { + filename: file.filename, + previousFilename: file.previousFilename, + baseBranch: params.baseBranch, + exitCode: result.exitCode, + reason: result.reason, + error: localDiffError, + }); + enriched.push({ + ...file, + patch: undefined, + patchSource: 'local-diff-failed', + localPatchChars: 0, + githubPatchChars, + localHunkCount: 0, + githubHunkCount, + localDiffError, + }); + continue; + } + + const rawLocalPatch = result.stdout.trimEnd(); + const localHunkCount = countDiffHunks(rawLocalPatch); + const localPatchChars = rawLocalPatch.length; + + // Binary files produce a non-empty metadata line ("Binary files ... differ") + // but contain no @@ hunks. Detect this via the 'Binary files' marker so the + // file is listed in SKIPPED FILES rather than injected into review context as + // a misleading 'local-git' patch containing binary metadata. Pure renames + // also have no @@ hunks but carry 'rename from'/'rename to' metadata that IS + // meaningful, so they intentionally pass this guard and remain 'local-git'. + const hasBinaryMarker = /^Binary files /m.test(rawLocalPatch); + const patch = rawLocalPatch.length > 0 && !hasBinaryMarker ? rawLocalPatch : undefined; + const patchSource: PatchSourceStatus = + patch !== undefined ? 'local-git' : file.patch ? 'local-diff-empty' : 'no-patch'; + const enrichedFile: EnrichedPRDiffFile = { + ...file, + patch, + patchSource, + localPatchChars, + githubPatchChars, + localHunkCount, + githubHunkCount, + }; + enriched.push(enrichedFile); + if (hasMismatch(enrichedFile)) { + mismatches.push({ + filename: file.filename, + localPatchChars, + githubPatchChars, + localHunkCount, + githubHunkCount, + }); + } + } + + return { files: enriched, mismatches }; +} diff --git a/src/agents/shared/prFormatting.ts b/src/agents/shared/prFormatting.ts index 237526613..f9d755670 100644 --- a/src/agents/shared/prFormatting.ts +++ b/src/agents/shared/prFormatting.ts @@ -1,5 +1,6 @@ import { estimateTokens, REVIEW_DIFF_CONTEXT_TOKEN_LIMIT } from '../../config/reviewConfig.js'; import type { githubClient } from '../../github/client.js'; +import type { EnrichedPRDiffFile, PatchSourceStatus } from './prDiffSource.js'; type PRDetails = Awaited>; type PRDiff = Awaited>; @@ -109,12 +110,19 @@ export function formatPRIssueComments(prIssueComments: PRIssueComments): string * Reason a PR file's diff was omitted from the compact context. * * - `deleted`: file was removed in the PR (no meaningful diff to read). - * - `no-patch`: GitHub didn't return a patch (typically binary blobs or files - * too large for the diff API). + * - `no-patch`: neither GitHub nor local git returned a text patch. + * - `local-diff-empty`: GitHub reported a patch, but local git produced none. + * - `local-diff-failed`: local git could not produce a verified patch. * - `patch-too-large`: the individual file's patch would exceed the per-file cap. * - `over-budget`: the cumulative context budget was reached before this file. */ -export type SkipReason = 'deleted' | 'no-patch' | 'patch-too-large' | 'over-budget'; +export type SkipReason = + | 'deleted' + | 'no-patch' + | 'local-diff-empty' + | 'local-diff-failed' + | 'patch-too-large' + | 'over-budget'; export interface SkippedFile { filename: string; @@ -122,8 +130,16 @@ export interface SkippedFile { } export interface PRDiffContext { - included: Array<{ path: string; status: PRDiff[number]['status']; diff: string }>; + included: Array<{ + path: string; + status: PRDiff[number]['status']; + diff: string; + patchSource?: PatchSourceStatus; + tokens: number; + }>; skipped: SkippedFile[]; + totalDiffTokens: number; + perFileTokenCap: number; } /** Per-file cap: any single file's patch over this is skipped as "patch-too-large". */ @@ -133,11 +149,18 @@ const PER_FILE_TOKEN_CAP = Math.floor(REVIEW_DIFF_CONTEXT_TOKEN_LIMIT / 10); * Format a single file's compact diff section with a consistent header. * Kept internal — callers consume `PRDiffContext.included[].diff` directly. */ -function formatCompactDiff(file: PRDiff[number]): string { +function formatCompactDiff(file: PRDiff[number] | EnrichedPRDiffFile): string { const header = `### ${file.filename} (${file.status}, +${file.additions} -${file.deletions})`; return `${header}\n\n\`\`\`diff\n${file.patch ?? ''}\n\`\`\``; } +function skipReasonForUnverified(file: PRDiff[number] | EnrichedPRDiffFile): SkipReason | null { + const patchSource = (file as EnrichedPRDiffFile).patchSource; + if (patchSource === 'local-diff-failed') return 'local-diff-failed'; + if (patchSource === 'local-diff-empty') return 'local-diff-empty'; + return null; +} + /** * Produce a compact-diff context for the review agent from the PR's changed-files * list. Scales with PR size (not repo size). Files that can't be included are @@ -164,6 +187,11 @@ export function extractPRDiffs(prDiff: PRDiff): PRDiffContext { skipped.push({ filename: file.filename, reason: 'deleted' }); continue; } + const unverifiedReason = skipReasonForUnverified(file); + if (unverifiedReason) { + skipped.push({ filename: file.filename, reason: unverifiedReason }); + continue; + } if (!file.patch) { skipped.push({ filename: file.filename, reason: 'no-patch' }); continue; @@ -178,11 +206,17 @@ export function extractPRDiffs(prDiff: PRDiff): PRDiffContext { skipped.push({ filename: file.filename, reason: 'over-budget' }); continue; } - included.push({ path: file.filename, status: file.status, diff }); + included.push({ + path: file.filename, + status: file.status, + diff, + patchSource: (file as EnrichedPRDiffFile).patchSource, + tokens, + }); totalTokens += tokens; } - return { included, skipped }; + return { included, skipped, totalDiffTokens: totalTokens, perFileTokenCap: PER_FILE_TOKEN_CAP }; } /** @@ -219,8 +253,8 @@ export function formatSkippedFilesInjection( '', 'If any of these files are relevant to your review, fetch them on demand:', prNumber !== undefined - ? ` • \`gh pr diff ${prNumber} -- \` to read the patch` - : ' • `gh pr diff -- ` to read the patch', + ? ` • \`cascade-tools scm get-pr-diff --prNumber ${prNumber} --path \` to read the patch` + : ' • `cascade-tools scm get-pr-diff --prNumber --path ` to read the patch', ' • `Read ` to read the post-PR file content', ' • `Grep ` to search inside the file', '', @@ -235,6 +269,8 @@ export function countSkipsByReason(skipped: SkippedFile[]): Record = { deleted: 0, 'no-patch': 0, + 'local-diff-empty': 0, + 'local-diff-failed': 0, 'patch-too-large': 0, 'over-budget': 0, }; diff --git a/src/cli/scm/get-pr-diff.ts b/src/cli/scm/get-pr-diff.ts index 0cdd6bd7a..7a1261e6d 100644 --- a/src/cli/scm/get-pr-diff.ts +++ b/src/cli/scm/get-pr-diff.ts @@ -3,5 +3,10 @@ import { getPRDiffDef } from '../../gadgets/github/definitions.js'; import { createCLICommand } from '../../gadgets/shared/cliCommandFactory.js'; export default createCLICommand(getPRDiffDef, async (params) => { - return getPRDiff(params.owner as string, params.repo as string, params.prNumber as number); + return getPRDiff( + params.owner as string, + params.repo as string, + params.prNumber as number, + params.path as string | undefined, + ); }); diff --git a/src/gadgets/github/GetPRDiff.ts b/src/gadgets/github/GetPRDiff.ts index 63b2268c1..2765a5660 100644 --- a/src/gadgets/github/GetPRDiff.ts +++ b/src/gadgets/github/GetPRDiff.ts @@ -3,5 +3,10 @@ import { getPRDiff } from './core/getPRDiff.js'; import { getPRDiffDef } from './definitions.js'; export const GetPRDiff = createGadgetClass(getPRDiffDef, async (params) => { - return getPRDiff(params.owner as string, params.repo as string, params.prNumber as number); + return getPRDiff( + params.owner as string, + params.repo as string, + params.prNumber as number, + params.path as string | undefined, + ); }); diff --git a/src/gadgets/github/core/getPRDiff.ts b/src/gadgets/github/core/getPRDiff.ts index 15e2d21cf..05a7bf08f 100644 --- a/src/gadgets/github/core/getPRDiff.ts +++ b/src/gadgets/github/core/getPRDiff.ts @@ -1,15 +1,26 @@ import { githubClient } from '../../../github/client.js'; -export async function getPRDiff(owner: string, repo: string, prNumber: number): Promise { +export async function getPRDiff( + owner: string, + repo: string, + prNumber: number, + path?: string, +): Promise { try { const files = await githubClient.getPRDiff(owner, repo, prNumber); + const filteredFiles = path + ? files.filter((f) => f.filename === path || f.previousFilename === path) + : files; - if (files.length === 0) { - return 'No files changed in this PR.'; + if (filteredFiles.length === 0) { + return path ? `No changed file matched path: ${path}` : 'No files changed in this PR.'; } - const formatted = files.map((f) => { + const formatted = filteredFiles.map((f) => { const lines = [`## ${f.filename}`, `Status: ${f.status} | +${f.additions} -${f.deletions}`]; + if (f.previousFilename) { + lines.push(`Previous filename: ${f.previousFilename}`); + } if (f.patch) { lines.push('```diff', f.patch, '```'); } else { @@ -18,7 +29,7 @@ export async function getPRDiff(owner: string, repo: string, prNumber: number): return lines.join('\n'); }); - return `${files.length} file(s) changed:\n\n${formatted.join('\n\n')}`; + return `${filteredFiles.length} file(s) changed:\n\n${formatted.join('\n\n')}`; } catch (error) { const message = error instanceof Error ? error.message : String(error); return `Error fetching PR diff: ${message}`; diff --git a/src/gadgets/github/definitions.ts b/src/gadgets/github/definitions.ts index f5963b76e..36b8ebc56 100644 --- a/src/gadgets/github/definitions.ts +++ b/src/gadgets/github/definitions.ts @@ -328,6 +328,12 @@ export const getPRDiffDef: ToolDefinition = { describe: 'The pull request number', required: true, }, + path: { + type: 'string', + describe: + 'Optional changed-file path to fetch. Matches either the current filename or previous filename for renames.', + optional: true, + }, }, examples: [ { diff --git a/src/gadgets/sentry/core/format.ts b/src/gadgets/sentry/core/format.ts index eb8c12bcb..7288d06fb 100644 --- a/src/gadgets/sentry/core/format.ts +++ b/src/gadgets/sentry/core/format.ts @@ -12,6 +12,10 @@ import type { SentryStackFrame, } from '../../../sentry/types.js'; +export type SentryIssueEventContext = Partial< + Pick +>; + // ============================================================================ // Issue formatting // ============================================================================ @@ -178,6 +182,17 @@ function appendEventMeta(lines: string[], event: SentryEvent): void { if (event.level) lines.push(`Level: ${event.level}`); } +function appendIssueContext(lines: string[], issue?: SentryIssueEventContext): void { + if (!issue) return; + const issueUrl = issue.web_url ?? issue.permalink; + if (!issueUrl && !issue.id && !issue.shortId && !issue.title) return; + + if (issueUrl) lines.push(`Sentry issue: ${issueUrl}`); + if (issue.id) lines.push(`Issue ID: ${issue.id}`); + if (issue.shortId) lines.push(`Short ID: ${issue.shortId}`); + if (issue.title) lines.push(`Issue title: ${issue.title}`); +} + function appendEventTags(lines: string[], event: SentryEvent): void { const tagPairs = normalizeTagPairs(event.tags).map(([key, value]) => `${key}=${value}`); if (tagPairs.length > 0) { @@ -341,13 +356,14 @@ function formatCompactValue(value: unknown): string { } } -export function formatSentryEvent(event: SentryEvent): string { +export function formatSentryEvent(event: SentryEvent, issue?: SentryIssueEventContext): string { const lines: string[] = []; const title = event.title ?? event.message ?? '(no title)'; lines.push(`# Alert Event: ${title}`); lines.push(''); + appendIssueContext(lines, issue); appendEventMeta(lines, event); appendEventTags(lines, event); appendEventRequest(lines, event); diff --git a/src/gadgets/sentry/core/getSentryEventDetail.ts b/src/gadgets/sentry/core/getSentryEventDetail.ts index 67cd7c3fd..dddab91ad 100644 --- a/src/gadgets/sentry/core/getSentryEventDetail.ts +++ b/src/gadgets/sentry/core/getSentryEventDetail.ts @@ -8,5 +8,6 @@ export async function getSentryEventDetail( ): Promise { const client = getSentryClient(); const event = await client.getIssueEvent(organizationId, issueId, eventId); - return formatSentryEvent(event); + const issue = await client.getIssue(organizationId, issueId).catch(() => undefined); + return formatSentryEvent(event, issue); } diff --git a/src/github/client.ts b/src/github/client.ts index 82ad05541..41dc3f217 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -94,6 +94,7 @@ export interface FailedWorkflowRuns { export interface PRDiffFile { filename: string; + previousFilename?: string; status: 'added' | 'removed' | 'modified' | 'renamed' | 'copied' | 'changed' | 'unchanged'; additions: number; deletions: number; @@ -384,6 +385,7 @@ export const githubClient = { }); return data.map((f) => ({ filename: f.filename, + previousFilename: f.previous_filename, status: f.status as PRDiffFile['status'], additions: f.additions, deletions: f.deletions, diff --git a/src/integrations/README.md b/src/integrations/README.md index 43413fb1a..14201403c 100644 --- a/src/integrations/README.md +++ b/src/integrations/README.md @@ -367,9 +367,9 @@ See [spec 016](../../docs/specs/016-pm-image-delivery-reliability.md.done) for t materializeAlertWorkItem(source, externalId, project, hints) → pmNativeWorkItemId ``` -Located at `src/integrations/alerting/_shared/materialize.ts`. Callable from any alerting trigger; today only Sentry's `SentryIssueAlertTrigger` uses it, but the signature is generic (`source: AlertSource`). +Located at `src/integrations/alerting/_shared/materialize.ts`. Callable from any alerting worker path; today Sentry materializes on the worker side after trigger resolution for event alerts, issue-lifecycle webhooks, and metric alerts. -- **`source`** — `'sentry' | 'pagerduty' | 'datadog' | 'github-alert'` (union grows as new sources are added). +- **`source`** — `'sentry' | 'sentry-issue' | 'sentry-metric' | 'pagerduty' | 'datadog' | 'github-alert'` (union grows as new sources are added). The Sentry literals distinguish alert-rule event alerts, issue-lifecycle webhooks, and metric alerts so unique external mappings do not collide. - **`externalId`** — the alert provider's stable issue/alert ID (e.g. Sentry issue ID `117972276`). - **`project`** — the full `ProjectConfig` for the target project. The materializer uses `project.pm` to determine which PM provider to call and which `alerts` slot to create the card in. - **`hints`** — `{ title: string; descriptionMarkdown: string }` — content to put in the card. Built by the per-source format helper (see below). @@ -387,7 +387,9 @@ CREATE UNIQUE INDEX uq_pr_work_items_project_external WHERE external_source IS NOT NULL; ``` -A second Sentry alert for the same issue on the same project hits the unique index and updates the existing row (lazy-heal: if the PM card was deleted, the UPDATE fetches the stored `work_item_id`, calls `getWorkItem`, and re-creates the card on 404). This makes the materializer fully idempotent — the same Sentry issue always produces the same `workItemId`. +A second Sentry alert for the same source/external ID on the same project hits the unique index and updates the existing row (lazy-heal: if the PM card was deleted, the UPDATE fetches the stored `work_item_id`, calls `getWorkItem`, and re-creates the card on 404). This makes the materializer fully idempotent — the same Sentry source event always produces the same `workItemId`. + +After worker-side materialization, the Sentry webhook worker mirrors display metadata back into the resolved `TriggerResult`: `workItemId`, `workItemTitle` from the formatted alert hints, `workItemUrl` from the active PM provider, and the same fields on `agentInput`. Shared run persistence, dashboard headers, environment setup, and progress comments consume those values without alert-specific call-site branches. ### Required `alerts` slot per provider @@ -407,7 +409,7 @@ A `cascade-alert` label (Trello: `labels['cascade-alert']`; JIRA: `labels.cascad ### Per-source format helpers -Each alert source has a format helper that maps the raw webhook payload to `AlertHints`. Today only Sentry is implemented (`src/integrations/alerting/_shared/format.ts` → `formatSentryCardBody`). Adding PagerDuty, Datadog, or GitHub Alerts follows this pattern: +Each alert source has a format helper that maps the raw webhook payload to `AlertHints`. Today Sentry has three helpers in `src/integrations/alerting/_shared/format.ts`: `formatSentryCardBody` (`sentry`), `formatSentryIssueLifecycleCardBody` (`sentry-issue`), and `formatSentryMetricCardBody` (`sentry-metric`). Adding PagerDuty, Datadog, or GitHub Alerts follows this pattern: 1. Add the source literal to `AlertSource` in `src/integrations/alerting/_shared/types.ts`. 2. Add a `formatXxxCardBody(payload) → AlertHints` function in `format.ts` (or a new per-source file). diff --git a/src/integrations/alerting/_shared/format.ts b/src/integrations/alerting/_shared/format.ts index c2cb922b2..fc4924306 100644 --- a/src/integrations/alerting/_shared/format.ts +++ b/src/integrations/alerting/_shared/format.ts @@ -15,18 +15,34 @@ import type { } from '../../../sentry/types.js'; import type { AlertHints } from './types.js'; +export function firstUsefulString(...candidates: unknown[]): string | undefined { + for (const candidate of candidates) { + if (typeof candidate !== 'string') continue; + const trimmed = candidate.trim(); + if (!trimmed) continue; + if (trimmed === 'undefined' || trimmed === 'null') continue; + return trimmed; + } + return undefined; +} + +export function firstUsefulUrl(...candidates: unknown[]): string | undefined { + return firstUsefulString(...candidates); +} + /** Build the PM card title and description body from a Sentry event_alert payload. */ export function formatSentryCardBody(augmented: SentryAugmentedPayload): AlertHints { const payload = augmented.payload as SentryIssueAlertPayload; const event = payload.data?.event; - const alertTitle = - payload.data?.issue_alert?.title ?? - payload.data?.triggered_rule ?? - event?.title ?? - 'Issue Alert'; + const alertTitle = firstUsefulString( + payload.data?.issue_alert?.title, + payload.data?.triggered_rule, + event?.title, + 'Issue Alert', + ); - const issueUrl = event?.web_url ?? event?.issue_url ?? ''; + const issueUrl = firstUsefulUrl(event?.web_url, event?.issue_url) ?? ''; const timestamp = event?.timestamp ?? ''; const topFrame = findTopInAppFrame(event?.exception?.values?.[0]?.stacktrace?.frames); @@ -35,7 +51,10 @@ export function formatSentryCardBody(augmented: SentryAugmentedPayload): AlertHi if (issueUrl) lines.push(`**Sentry issue:** ${issueUrl}`); if (timestamp) lines.push(`**First seen:** ${timestamp}`); - const ruleName = payload.data?.issue_alert?.title ?? payload.data?.triggered_rule; + const ruleName = firstUsefulString( + payload.data?.issue_alert?.title, + payload.data?.triggered_rule, + ); if (ruleName) lines.push(`**Alert rule:** ${ruleName}`); if (topFrame) { @@ -68,8 +87,8 @@ export function formatSentryIssueLifecycleCardBody(augmented: SentryAugmentedPay const payload = augmented.payload as SentryIssuePayload; const issue = payload.data?.issue; - const alertTitle = issue?.title?.trim() ? issue.title : 'Sentry Issue'; - const issueUrl = issue?.web_url ?? issue?.permalink ?? ''; + const alertTitle = firstUsefulString(issue?.title, 'Sentry Issue'); + const issueUrl = firstUsefulUrl(issue?.web_url, issue?.permalink, issue?.url) ?? ''; const lines: string[] = []; if (issueUrl) lines.push(`**Sentry issue:** ${issueUrl}`); @@ -94,12 +113,13 @@ export function formatSentryIssueLifecycleCardBody(augmented: SentryAugmentedPay export function formatSentryMetricCardBody(augmented: SentryAugmentedPayload): AlertHints { const payload = augmented.payload as SentryMetricAlertPayload; - const alertTitle = - payload.data?.description_title ?? - payload.data?.metric_alert?.alert_rule?.aggregate ?? - `Metric Alert (${payload.action})`; + const alertTitle = firstUsefulString( + payload.data?.description_title, + payload.data?.metric_alert?.alert_rule?.aggregate, + `Metric Alert (${payload.action})`, + ); - const webUrl = payload.data?.web_url ?? ''; + const webUrl = firstUsefulUrl(payload.data?.web_url) ?? ''; const action = payload.action; const query = payload.data?.metric_alert?.alert_rule?.query; const aggregate = payload.data?.metric_alert?.alert_rule?.aggregate; diff --git a/src/sentry/types.ts b/src/sentry/types.ts index dd370fc7f..16066335c 100644 --- a/src/sentry/types.ts +++ b/src/sentry/types.ts @@ -156,6 +156,7 @@ export interface SentryIssue { id: string; title: string; culprit?: string; + web_url?: string; permalink?: string; shortId?: string; status?: string; diff --git a/src/triggers/sentry/alerting-issue-lifecycle.ts b/src/triggers/sentry/alerting-issue-lifecycle.ts index 2e60dbf09..ffc06a6f8 100644 --- a/src/triggers/sentry/alerting-issue-lifecycle.ts +++ b/src/triggers/sentry/alerting-issue-lifecycle.ts @@ -24,6 +24,7 @@ * webhooks for the same Sentry issue ID. */ +import { firstUsefulString, firstUsefulUrl } from '../../integrations/alerting/_shared/format.js'; import { getAlertsContainerId } from '../../pm/config.js'; import { getSentryIntegrationConfig } from '../../sentry/integration.js'; import type { SentryAugmentedPayload, SentryIssuePayload } from '../../sentry/types.js'; @@ -73,8 +74,8 @@ export class SentryIssueLifecycleTrigger implements TriggerHandler { return null; } - const issueUrl = issue?.web_url ?? issue?.permalink ?? issue?.url; - const alertTitle = issue?.title?.trim() ? issue.title : 'Sentry Issue'; + const issueUrl = firstUsefulUrl(issue?.web_url, issue?.permalink, issue?.url); + const alertTitle = firstUsefulString(issue?.title, 'Sentry Issue'); // Look up org slug from integration config (mirrors SentryIssueAlertTrigger). const sentryConfig = await getSentryIntegrationConfig(ctx.project.id); diff --git a/src/triggers/sentry/alerting-issue.ts b/src/triggers/sentry/alerting-issue.ts index a7679ac5b..caafa235e 100644 --- a/src/triggers/sentry/alerting-issue.ts +++ b/src/triggers/sentry/alerting-issue.ts @@ -11,6 +11,7 @@ * which would return HTTP 200 to Sentry with no job ever enqueued. */ +import { firstUsefulString, firstUsefulUrl } from '../../integrations/alerting/_shared/format.js'; import { getAlertsContainerId } from '../../pm/config.js'; import { getSentryIntegrationConfig } from '../../sentry/integration.js'; import type { SentryAugmentedPayload, SentryIssueAlertPayload } from '../../sentry/types.js'; @@ -48,12 +49,13 @@ export class SentryIssueAlertTrigger implements TriggerHandler { // Extract issue/event info from the payload const event = innerPayload.data?.event; const issueId = event?.issue_id ?? event?.issue_url?.split('/').pop(); - const issueUrl = event?.web_url ?? event?.issue_url; - const alertTitle = - innerPayload.data?.issue_alert?.title ?? - innerPayload.data?.triggered_rule ?? - event?.title ?? - 'Issue Alert'; + const issueUrl = firstUsefulUrl(event?.web_url, event?.issue_url); + const alertTitle = firstUsefulString( + innerPayload.data?.issue_alert?.title, + innerPayload.data?.triggered_rule, + event?.title, + 'Issue Alert', + ); if (!issueId) { logger.warn('SentryIssueAlertTrigger: cannot determine issue ID from payload', { diff --git a/src/triggers/sentry/alerting-metric.ts b/src/triggers/sentry/alerting-metric.ts index cc6656850..9bd216413 100644 --- a/src/triggers/sentry/alerting-metric.ts +++ b/src/triggers/sentry/alerting-metric.ts @@ -13,6 +13,7 @@ * which would return HTTP 200 to Sentry with no job ever enqueued. */ +import { firstUsefulString, firstUsefulUrl } from '../../integrations/alerting/_shared/format.js'; import { getAlertsContainerId } from '../../pm/config.js'; import { getSentryIntegrationConfig } from '../../sentry/integration.js'; import type { SentryAugmentedPayload, SentryMetricAlertPayload } from '../../sentry/types.js'; @@ -70,10 +71,11 @@ export class SentryMetricAlertTrigger implements TriggerHandler { return null; } - const alertTitle = - innerPayload.data?.description_title ?? - innerPayload.data?.metric_alert?.alert_rule?.aggregate ?? - `Metric Alert (${action})`; + const alertTitle = firstUsefulString( + innerPayload.data?.description_title, + innerPayload.data?.metric_alert?.alert_rule?.aggregate, + `Metric Alert (${action})`, + ); logger.info('Alerting: metric alert triggered', { projectId: ctx.project.id, @@ -108,7 +110,7 @@ export class SentryMetricAlertTrigger implements TriggerHandler { alertMetricKey, alertOrgId: sentryConfig.organizationSlug, alertTitle, - alertIssueUrl: innerPayload.data?.web_url, + alertIssueUrl: firstUsefulUrl(innerPayload.data?.web_url), }, // workItemId omitted — worker sets it after materialisation. // lockKey provides router-level work-item concurrency protection while diff --git a/src/triggers/sentry/webhook-handler.ts b/src/triggers/sentry/webhook-handler.ts index 3fb1390be..31d32ad64 100644 --- a/src/triggers/sentry/webhook-handler.ts +++ b/src/triggers/sentry/webhook-handler.ts @@ -19,6 +19,7 @@ */ import { AlertSlotMissingError } from '../../integrations/alerting/_shared/types.js'; +import { pmRegistry } from '../../pm/registry.js'; import type { SentryAugmentedPayload } from '../../sentry/types.js'; import type { ProjectConfig, TriggerResult } from '../../types/index.js'; import { startWatchdog } from '../../utils/lifecycle.js'; @@ -65,39 +66,36 @@ async function materializeSentryAlertWorkItem( const augmented = payload as SentryAugmentedPayload; try { - let workItemId: string; + let source: Parameters[0]; + let externalId: string; + let hints: Parameters[3]; if (triggerEvent === TRIGGER_EVENTS.ALERTING.ISSUE_LIFECYCLE && alertIssueId) { // Sentry-Hook-Resource: issue (Internal Integration default surface). // Distinct AlertSource ('sentry-issue') from event_alert ('sentry') so the // partial-unique (project_id, external_source, external_id) index doesn't // collide if the same Sentry issue arrives via both surfaces. - workItemId = await materializeAlertWorkItem( - 'sentry-issue', - alertIssueId, - project, - formatSentryIssueLifecycleCardBody(augmented), - ); + source = 'sentry-issue'; + externalId = alertIssueId; + hints = formatSentryIssueLifecycleCardBody(augmented); } else if (alertIssueId) { // event_alert path (Sentry Alert Rule firings). - workItemId = await materializeAlertWorkItem( - 'sentry', - alertIssueId, - project, - formatSentryCardBody(augmented), - ); + source = 'sentry'; + externalId = alertIssueId; + hints = formatSentryCardBody(augmented); } else { // alertMetricKey is guaranteed non-null here (checked above). - workItemId = await materializeAlertWorkItem( - 'sentry-metric', - alertMetricKey as string, - project, - formatSentryMetricCardBody(augmented), - ); + source = 'sentry-metric'; + externalId = alertMetricKey as string; + hints = formatSentryMetricCardBody(augmented); } + const workItemId = await materializeAlertWorkItem(source, externalId, project, hints); + const workItemUrl = pmRegistry.createProvider(project).getWorkItemUrl(workItemId); return { ...result, workItemId, - agentInput: { ...result.agentInput, workItemId }, + workItemTitle: hints.title, + workItemUrl, + agentInput: { ...result.agentInput, workItemId, workItemTitle: hints.title, workItemUrl }, }; } catch (err) { if (err instanceof AlertSlotMissingError) { diff --git a/tests/unit/agents/definitions/contextSteps.test.ts b/tests/unit/agents/definitions/contextSteps.test.ts index f119c0628..9da2e8631 100644 --- a/tests/unit/agents/definitions/contextSteps.test.ts +++ b/tests/unit/agents/definitions/contextSteps.test.ts @@ -15,6 +15,16 @@ vi.mock('../../../../src/gadgets/todo/storage.js', () => ({ const mockTrelloDownload = vi.fn(); const mockJiraDownload = vi.fn(); const mockLinearDownload = vi.fn(); +const mockGetIssueEvent = vi.fn(); +const mockGetIssue = vi.fn(); +const mockSourceLocalPRDiffs = vi.hoisted(() => vi.fn()); + +vi.mock('../../../../src/sentry/client.js', () => ({ + getSentryClient: vi.fn(() => ({ + getIssueEvent: mockGetIssueEvent, + getIssue: mockGetIssue, + })), +})); vi.mock('../../../../src/trello/client.js', () => ({ trelloClient: { @@ -47,8 +57,13 @@ vi.mock('../../../../src/github/client.js', () => ({ }, })); +vi.mock('../../../../src/agents/shared/prDiffSource.js', () => ({ + sourceLocalPRDiffs: mockSourceLocalPRDiffs, +})); + import type { FetchContextParams } from '../../../../src/agents/definitions/contextSteps.js'; import { + fetchAlertingIssueStep, fetchPRContextStep, fetchWorkItemStep, prepopulateTodosStep, @@ -76,6 +91,18 @@ function makeParams(input: Partial): FetchContextParams { }; } +function makeLocalDiffSourceFile(file: Record) { + const patch = typeof file.patch === 'string' ? file.patch : undefined; + return { + ...file, + patchSource: patch ? 'local-git' : 'no-patch', + localPatchChars: patch?.length ?? 0, + githubPatchChars: patch?.length ?? 0, + localHunkCount: patch ? 1 : 0, + githubHunkCount: patch ? 1 : 0, + }; +} + describe('prepopulateTodosStep', () => { it('returns empty array when no workItemId', async () => { const result = await prepopulateTodosStep(makeParams({})); @@ -502,8 +529,79 @@ describe('fetchWorkItemStep', () => { }); }); +describe('fetchAlertingIssueStep', () => { + beforeEach(() => { + mockGetIssueEvent.mockReset(); + mockGetIssue.mockReset(); + }); + + it('uses alertIssueUrl from agent input in preloaded event context', async () => { + mockGetIssueEvent.mockResolvedValue({ + eventID: 'evt-1', + title: 'TypeError: object is not iterable', + }); + + const result = await fetchAlertingIssueStep( + makeParams({ + alertIssueId: '119054737', + alertOrgId: 'mongrel', + alertTitle: 'TypeError group', + alertIssueUrl: 'https://sentry.io/organizations/mongrel/issues/119054737/', + }), + ); + + expect(mockGetIssueEvent).toHaveBeenCalledWith('mongrel', '119054737', 'latest'); + expect(mockGetIssue).not.toHaveBeenCalled(); + expect(result[0].result).toContain( + 'Sentry issue: https://sentry.io/organizations/mongrel/issues/119054737/', + ); + expect(result[0].result).toContain('Issue ID: 119054737'); + expect(result[0].result).toContain('Issue title: TypeError group'); + }); + + it('falls back to Sentry issue metadata when alertIssueUrl is absent', async () => { + mockGetIssueEvent.mockResolvedValue({ + eventID: 'evt-1', + title: 'TypeError: object is not iterable', + }); + mockGetIssue.mockResolvedValue({ + id: '119054737', + title: 'TypeError group', + permalink: 'https://sentry.io/organizations/mongrel/issues/119054737/', + }); + + const result = await fetchAlertingIssueStep( + makeParams({ alertIssueId: '119054737', alertOrgId: 'mongrel' }), + ); + + expect(mockGetIssue).toHaveBeenCalledWith('mongrel', '119054737'); + expect(result[0].result).toContain( + 'Sentry issue: https://sentry.io/organizations/mongrel/issues/119054737/', + ); + }); + + it('returns event details when issue metadata fetch fails', async () => { + mockGetIssueEvent.mockResolvedValue({ + eventID: 'evt-1', + title: 'TypeError: object is not iterable', + }); + mockGetIssue.mockRejectedValue(new Error('metadata unavailable')); + + const result = await fetchAlertingIssueStep( + makeParams({ alertIssueId: '119054737', alertOrgId: 'mongrel' }), + ); + + expect(result[0].result).toContain('Event ID: evt-1'); + expect(result[0].result).not.toContain('Sentry issue:'); + }); +}); + describe('fetchPRContextStep — compact diffs + SKIPPED FILES contract', () => { beforeEach(() => { + mockSourceLocalPRDiffs.mockImplementation(async ({ files }) => ({ + files: files.map(makeLocalDiffSourceFile), + mismatches: [], + })); mockGetPR.mockResolvedValue({ number: 1092, title: 'Test PR', @@ -589,7 +687,7 @@ describe('fetchPRContextStep — compact diffs + SKIPPED FILES contract', () => expect(skipped).toBeDefined(); expect(skipped?.result as string).toContain('legacy.ts'); expect(skipped?.result as string).toContain('deleted'); - expect(skipped?.result as string).toContain('gh pr diff 1092'); + expect(skipped?.result as string).toContain('cascade-tools scm get-pr-diff --prNumber 1092'); expect(skipped?.result as string).toContain('Read'); }); @@ -649,7 +747,154 @@ describe('fetchPRContextStep — compact diffs + SKIPPED FILES contract', () => included: 1, skipped: 2, skipReasons: expect.objectContaining({ deleted: 1, 'no-patch': 1 }), + patchSources: expect.objectContaining({ 'local-git': 1, 'no-patch': 2 }), + totalDiffTokens: expect.any(Number), + perFileTokenCap: expect.any(Number), + localGitMismatches: [], + }), + ); + }); + + it('uses local-git patch bodies for compact diff context', async () => { + mockGetPRDiff.mockResolvedValue([ + { + filename: 'server/lib/calendar/eventContextService.ts', + status: 'modified', + additions: 3, + deletions: 1, + changes: 4, + patch: '@@ -1 +1 @@\n+api hunk', + }, + ]); + mockSourceLocalPRDiffs.mockResolvedValue({ + files: [ + { + filename: 'server/lib/calendar/eventContextService.ts', + status: 'modified', + additions: 3, + deletions: 1, + changes: 4, + patch: '@@ -1 +1 @@\n+api hunk\n@@ -50 +50 @@\n+local-only central hunk', + patchSource: 'local-git', + localPatchChars: 64, + githubPatchChars: 20, + localHunkCount: 2, + githubHunkCount: 1, + }, + ], + mismatches: [ + { + filename: 'server/lib/calendar/eventContextService.ts', + localPatchChars: 64, + githubPatchChars: 20, + localHunkCount: 2, + githubHunkCount: 1, + }, + ], + }); + + const params = makePRParams(); + const injections = await fetchPRContextStep(params); + + const diffInjection = injections.find((i) => i.description === 'Pre-fetched PR diff context'); + expect(diffInjection?.result as string).toContain('local-only central hunk'); + expect(injections.some((i) => i.toolName === 'GetPRDiff')).toBe(false); + expect(params.logWriter).toHaveBeenCalledWith( + 'INFO', + 'PR context prepared', + expect.objectContaining({ + localGitMismatches: [ + expect.objectContaining({ + filename: 'server/lib/calendar/eventContextService.ts', + localHunkCount: 2, + githubHunkCount: 1, + }), + ], }), ); }); + + it('skips local diff failures instead of including GitHub API patches', async () => { + mockGetPRDiff.mockResolvedValue([ + { + filename: 'src/a.ts', + status: 'modified', + additions: 1, + deletions: 0, + changes: 1, + patch: '@@ -1 +1 @@\n+api-only', + }, + ]); + mockSourceLocalPRDiffs.mockResolvedValue({ + files: [ + { + filename: 'src/a.ts', + status: 'modified', + additions: 1, + deletions: 0, + changes: 1, + patch: undefined, + patchSource: 'local-diff-failed', + localPatchChars: 0, + githubPatchChars: 18, + localHunkCount: 0, + githubHunkCount: 1, + }, + ], + mismatches: [], + }); + + const injections = await fetchPRContextStep(makePRParams()); + + const diffInjection = injections.find((i) => i.description === 'Pre-fetched PR diff context'); + expect(diffInjection?.result as string).not.toContain('api-only'); + const skipped = injections.find((i) => i.description === 'Skipped files'); + expect(skipped?.result as string).toContain('src/a.ts'); + expect(skipped?.result as string).toContain('local-diff-failed'); + }); + + it('passes prDetails.baseRef (not project baseBranch) to sourceLocalPRDiffs for stacked PRs', async () => { + // When a stacked PR targets a feature branch (e.g. 'parent-feature') instead of + // the project's default base branch (e.g. 'main'), the local diff should be + // computed against the PR's actual base ref so parent-branch commits are not + // included in the review context. + mockGetPR.mockResolvedValueOnce({ + number: 1092, + title: 'Stacked PR', + body: 'Stacked on parent-feature', + state: 'open', + htmlUrl: 'https://github.com/o/r/pull/1092', + headRef: 'stacked-feature', + headSha: 'abc123', + baseRef: 'parent-feature', + merged: false, + mergeable: true, + user: { login: 'dev' }, + }); + mockGetPRDiff.mockResolvedValue([ + { + filename: 'src/stacked.ts', + status: 'added', + additions: 3, + deletions: 0, + changes: 3, + patch: '@@ -0,0 +1,3 @@\n+const pr = 3', + }, + ]); + + const params = makeParams({ + prNumber: 1092, + repoFullName: 'o/r', + }); + // Provide a project with a different baseBranch to confirm it is ignored + (params as FetchContextParams & { project?: { baseBranch: string } }).project = { + baseBranch: 'main', + } as never; + + await fetchPRContextStep(params as FetchContextParams); + + expect(mockSourceLocalPRDiffs).toHaveBeenCalledWith( + expect.objectContaining({ baseBranch: 'parent-feature' }), + ); + }); }); diff --git a/tests/unit/agents/definitions/review.yaml.test.ts b/tests/unit/agents/definitions/review.yaml.test.ts index 345654f1c..55d3a94e2 100644 --- a/tests/unit/agents/definitions/review.yaml.test.ts +++ b/tests/unit/agents/definitions/review.yaml.test.ts @@ -19,11 +19,16 @@ describe('review.yaml prompt contract', () => { expect(yamlText).toMatch(/SKIPPED FILES/i); }); - it('tells the agent to fetch skipped files via `gh pr diff` or `Read`', () => { - expect(yamlText).toMatch(/gh pr diff/); + it('tells the agent to fetch skipped files via cascade-tools or Read', () => { + expect(yamlText).toMatch(/cascade-tools scm get-pr-diff/); + expect(yamlText).toMatch(/--path /); expect(yamlText).toMatch(/\bRead\b/); }); + it('does not send skipped-file fetches through raw gh', () => { + expect(yamlText).not.toMatch(/gh pr diff/); + }); + it('describes the compact-diff context shape', () => { // Agent should know it will see per-file diffs (not full file contents). expect(yamlText.toLowerCase()).toMatch(/compact|per-file diff|diff\s+context/); diff --git a/tests/unit/agents/prompts.test.ts b/tests/unit/agents/prompts.test.ts index 8650bec4c..9c2ccb07f 100644 --- a/tests/unit/agents/prompts.test.ts +++ b/tests/unit/agents/prompts.test.ts @@ -21,10 +21,12 @@ vi.mock('../../../src/agents/definitions/index.js', () => ({ })); import { + buildTaskPromptContext, getAvailablePartialNames, getRawPartial, getRawTemplate, getSystemPrompt, + getTaskTemplateVariables, getTemplateVariables, getValidAgentTypes, initPrompts, @@ -39,6 +41,51 @@ beforeAll(async () => { await initPrompts(); }); +describe('buildTaskPromptContext', () => { + it('preserves scalar alert fields and comment aliases', () => { + const context = buildTaskPromptContext({ + workItemId: 'MNG-740', + alertTitle: 'Error rate high', + alertIssueUrl: 'https://sentry.io/issues/123/', + alertIssueId: '123', + alertOrgId: 'mongrel', + alertMetricKey: 'mongrel:Error rate high', + triggerCommentBody: 'Please investigate', + triggerCommentText: 'legacy body', + triggerCommentAuthor: 'aaight42', + project: { id: 'not-exposed' }, + config: { projects: [] }, + }); + + expect(context).toMatchObject({ + workItemId: 'MNG-740', + alertTitle: 'Error rate high', + alertIssueUrl: 'https://sentry.io/issues/123/', + alertIssueId: '123', + alertOrgId: 'mongrel', + alertMetricKey: 'mongrel:Error rate high', + commentText: 'Please investigate', + commentBody: 'Please investigate', + commentAuthor: 'aaight42', + }); + expect(context.project).toBeUndefined(); + expect(context.config).toBeUndefined(); + }); + + it('documents alert task template variables', () => { + const names = getTaskTemplateVariables().map((variable) => variable.name); + expect(names).toEqual( + expect.arrayContaining([ + 'alertTitle', + 'alertIssueUrl', + 'alertIssueId', + 'alertOrgId', + 'alertMetricKey', + ]), + ); + }); +}); + describe('getSystemPrompt', () => { it('returns splitting prompt for splitting agent', () => { const prompt = getSystemPrompt('splitting'); diff --git a/tests/unit/agents/shared/modelResolution.test.ts b/tests/unit/agents/shared/modelResolution.test.ts index 4dce9605a..4153195d9 100644 --- a/tests/unit/agents/shared/modelResolution.test.ts +++ b/tests/unit/agents/shared/modelResolution.test.ts @@ -43,6 +43,7 @@ vi.mock('../../../../src/agents/definitions/loader.js', () => ({ 'respond-to-pr-comment', 'respond-to-planning-comment', 'debug', + 'alerting', ]), getBuiltinAgentTypes: vi.fn().mockReturnValue([]), })); @@ -62,6 +63,7 @@ vi.mock('../../../../src/agents/definitions/index.js', () => ({ 'respond-to-pr-comment', 'respond-to-planning-comment', 'debug', + 'alerting', ]), getBuiltinAgentTypes: vi.fn().mockReturnValue([]), })); @@ -542,6 +544,33 @@ describe('resolveModelConfig', () => { expect(result.taskPrompt).toContain('Fix this line'); }); + it('renders alert scalar variables from agentInput in task prompt override', async () => { + vi.mocked(resolveAgentDefinition).mockResolvedValue( + mockAgentDefinition({ + taskPrompt: 'Alert: <%= it.alertTitle %> | <%= it.alertIssueUrl %>', + }), + ); + + const result = await resolveModelConfig({ + agentType: 'alerting', + project: makeProject(), + config: makeConfig(), + repoDir: '/tmp/test', + agentInput: { + triggerEvent: 'alerting:issue-alert', + alertTitle: 'TypeError in worker', + alertIssueUrl: 'https://sentry.io/issues/119054737/', + alertIssueId: '119054737', + alertOrgId: 'mongrel', + }, + }); + + expect(result.taskPrompt).toBe( + 'Alert: TypeError in worker | https://sentry.io/issues/119054737/', + ); + expect(result.taskPrompt).not.toContain('undefined'); + }); + it('forwards promptContext fields to task prompt rendering', async () => { vi.mocked(resolveAgentDefinition).mockResolvedValue( mockAgentDefinition({ diff --git a/tests/unit/agents/shared/prDiffSource.test.ts b/tests/unit/agents/shared/prDiffSource.test.ts new file mode 100644 index 000000000..6d07d540c --- /dev/null +++ b/tests/unit/agents/shared/prDiffSource.test.ts @@ -0,0 +1,323 @@ +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { sourceLocalPRDiffs } from '../../../../src/agents/shared/prDiffSource.js'; +import type { PRDiffFile } from '../../../../src/github/client.js'; + +const tempDirs: string[] = []; + +function git(cwd: string, ...args: string[]): void { + execFileSync('git', args, { cwd, stdio: 'pipe' }); +} + +function makeRepo(): string { + // Create a bare remote so git fetch origin succeeds in the diff helper. + // Without a real remote, the fail-closed fetch check would mark all files as + // local-diff-failed even for a healthy repo. + const remoteDir = mkdtempSync(join(tmpdir(), 'cascade-remote-')); + tempDirs.push(remoteDir); + execFileSync('git', ['init', '--bare'], { cwd: remoteDir, stdio: 'pipe' }); + + const dir = mkdtempSync(join(tmpdir(), 'cascade-pr-diff-source-')); + tempDirs.push(dir); + git(dir, 'init'); + git(dir, 'config', 'user.email', 'test@example.com'); + git(dir, 'config', 'user.name', 'Test User'); + git(dir, 'remote', 'add', 'origin', remoteDir); + writeFileSync(join(dir, 'file.ts'), 'const a = 1;\nconst b = 2;\nconst c = 3;\n'); + git(dir, 'add', 'file.ts'); + git(dir, 'commit', '-m', 'base'); + git(dir, 'push', 'origin', 'HEAD:refs/heads/main'); + git(dir, 'checkout', '-b', 'feature'); + writeFileSync(join(dir, 'file.ts'), 'const a = 10;\nconst b = 2;\nconst c = 30;\n'); + git(dir, 'add', 'file.ts'); + git(dir, 'commit', '-m', 'feature'); + return dir; +} + +function makeFile(overrides: Partial = {}): PRDiffFile { + return { + filename: 'file.ts', + status: 'modified', + additions: 2, + deletions: 2, + changes: 4, + patch: '@@ -1 +1 @@\n-const a = 1;\n+const a = 10;', + ...overrides, + }; +} + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe('sourceLocalPRDiffs', () => { + it('uses local git patches and reports GitHub API clipping mismatches', async () => { + const repoDir = makeRepo(); + + const result = await sourceLocalPRDiffs({ + files: [makeFile()], + repoDir, + baseBranch: 'main', + logWriter: vi.fn(), + }); + + expect(result.files[0]).toEqual( + expect.objectContaining({ + filename: 'file.ts', + patchSource: 'local-git', + githubHunkCount: 1, + }), + ); + expect(result.files[0].patch).toContain('const c = 30'); + expect(result.files[0].localPatchChars).toBeGreaterThan(result.files[0].githubPatchChars); + expect(result.mismatches).toEqual([ + expect.objectContaining({ + filename: 'file.ts', + githubHunkCount: 1, + }), + ]); + }); + + it('marks local diff failures explicitly when base-ref fetch fails', async () => { + // When the repoDir does not exist, git fetch exits non-zero. The helper must + // fail closed: mark all non-deleted files as local-diff-failed rather than + // proceeding with a stale origin/ ref that could produce misleading patches. + const logWriter = vi.fn(); + const result = await sourceLocalPRDiffs({ + files: [makeFile()], + repoDir: '/path/that/does/not/exist', + baseBranch: 'main', + logWriter, + }); + + expect(result.files[0]).toEqual( + expect.objectContaining({ + filename: 'file.ts', + patch: undefined, + patchSource: 'local-diff-failed', + localPatchChars: 0, + }), + ); + // The WARN is now emitted at the fetch-failure point (fail-closed), not at + // the per-file diff step. + expect(logWriter).toHaveBeenCalledWith( + 'WARN', + 'Failed to refresh base branch ref before local diff', + expect.objectContaining({ baseBranch: 'main' }), + ); + }); + + it('uses :(literal) pathspec to avoid bracket-glob filename expansion', async () => { + // Regression for the pathspec metacharacter bug: without :(literal), + // git treats [id] as a character-class glob and includes i.ts / d.ts + // hunks under the [id].ts diff header. + const remoteDir = mkdtempSync(join(tmpdir(), 'cascade-remote-bracket-')); + tempDirs.push(remoteDir); + execFileSync('git', ['init', '--bare'], { cwd: remoteDir, stdio: 'pipe' }); + + const dir = mkdtempSync(join(tmpdir(), 'cascade-pr-diff-bracket-')); + tempDirs.push(dir); + git(dir, 'init'); + git(dir, 'config', 'user.email', 'test@example.com'); + git(dir, 'config', 'user.name', 'Test User'); + git(dir, 'remote', 'add', 'origin', remoteDir); + // [id].ts has bracket chars; i.ts would be matched by the [id] glob + writeFileSync(join(dir, '[id].ts'), 'const bracket = 1;\n'); + writeFileSync(join(dir, 'i.ts'), 'const normal = 1;\n'); + git(dir, 'add', '.'); + git(dir, 'commit', '-m', 'base'); + git(dir, 'push', 'origin', 'HEAD:refs/heads/main'); + git(dir, 'checkout', '-b', 'feature'); + writeFileSync(join(dir, '[id].ts'), 'const bracket = 2;\n'); + writeFileSync(join(dir, 'i.ts'), 'const normal = 2;\n'); + git(dir, 'add', '.'); + git(dir, 'commit', '-m', 'feature'); + + const result = await sourceLocalPRDiffs({ + files: [makeFile({ filename: '[id].ts' })], + repoDir: dir, + baseBranch: 'main', + logWriter: vi.fn(), + }); + + expect(result.files).toHaveLength(1); + expect(result.files[0].patchSource).toBe('local-git'); + // Only [id].ts content in patch — i.ts/normal must not bleed in + expect(result.files[0].patch).toContain('bracket'); + expect(result.files[0].patch).not.toContain('normal'); + expect(result.files[0].patch).not.toContain('i.ts'); + }); + + it('fetches origin base branch before diffing to avoid stale-ref patches', async () => { + // Regression for snapshot-reuse scenario: only refs/pull/N/head is + // fetched when reusing a snapshot, leaving origin/ stale. + // Without the fetch, git diff origin/main...HEAD includes base-branch + // commits that are NOT part of the PR (A...C instead of B...C). + + // Set up a bare "remote" repo + const remoteDir = mkdtempSync(join(tmpdir(), 'cascade-remote-')); + tempDirs.push(remoteDir); + execFileSync('git', ['init', '--bare'], { cwd: remoteDir, stdio: 'pipe' }); + + // Set up local working repo with origin pointing to remoteDir + const localDir = mkdtempSync(join(tmpdir(), 'cascade-local-')); + tempDirs.push(localDir); + git(localDir, 'init'); + git(localDir, 'config', 'user.email', 'test@example.com'); + git(localDir, 'config', 'user.name', 'Test User'); + git(localDir, 'remote', 'add', 'origin', remoteDir); + + // Commit A: initial state — push to remote (remote/main = A) + writeFileSync(join(localDir, 'file.ts'), 'const a = 1;\n'); + git(localDir, 'add', '.'); + git(localDir, 'commit', '-m', 'A'); + git(localDir, 'push', 'origin', 'HEAD:refs/heads/main'); + const shaA = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: localDir, + encoding: 'utf8', + }).trim(); + + // Commit B: base-branch advancement — push to remote (remote/main = B) + // This simulates main advancing after the snapshot was created. + writeFileSync(join(localDir, 'file.ts'), 'const a = 1;\nconst base = 2;\n'); + git(localDir, 'add', '.'); + git(localDir, 'commit', '-m', 'B'); + git(localDir, 'push', 'origin', 'HEAD:refs/heads/main'); + + // Commit C: the PR change on top of B (not pushed — only fetched via + // refs/pull/N/head in the real snapshot-reuse path) + writeFileSync(join(localDir, 'file.ts'), 'const a = 1;\nconst base = 2;\nconst pr = 3;\n'); + git(localDir, 'add', '.'); + git(localDir, 'commit', '-m', 'C'); + + // Simulate a stale snapshot: pin origin/main to A so the local ref lags + git(localDir, 'update-ref', 'refs/remotes/origin/main', shaA); + + // sourceLocalPRDiffs should fetch origin/main (updating it to B) before + // diffing, so the returned patch is B...C, not A...C. + const result = await sourceLocalPRDiffs({ + files: [makeFile()], + repoDir: localDir, + baseBranch: 'main', + logWriter: vi.fn(), + }); + + expect(result.files[0].patchSource).toBe('local-git'); + // Only C's change is an added line (+); B's `const base = 2` should be + // context (a space-prefixed line), not an addition (+line). + // Without the fetch fix, A...C would mark `const base = 2` as "+added". + expect(result.files[0].patch).toContain('+const pr = 3'); + expect(result.files[0].patch).not.toContain('+const base = 2'); + }); + + it('emits rename metadata when file.previousFilename is supplied', async () => { + // Regression for the renamed-file pathspec bug: with only the destination + // path (:(literal)new.ts), git diff emits 'new file mode' with every line + // added. Passing both :(literal)old.ts and :(literal)new.ts causes git to + // emit the proper 'rename from / rename to' metadata. + const remoteDir = mkdtempSync(join(tmpdir(), 'cascade-remote-rename-')); + tempDirs.push(remoteDir); + execFileSync('git', ['init', '--bare'], { cwd: remoteDir, stdio: 'pipe' }); + + const dir = mkdtempSync(join(tmpdir(), 'cascade-pr-diff-rename-')); + tempDirs.push(dir); + git(dir, 'init'); + git(dir, 'config', 'user.email', 'test@example.com'); + git(dir, 'config', 'user.name', 'Test User'); + git(dir, 'remote', 'add', 'origin', remoteDir); + + // Base commit: old.ts exists + writeFileSync(join(dir, 'old.ts'), 'const x = 1;\nconst y = 2;\n'); + git(dir, 'add', '.'); + git(dir, 'commit', '-m', 'base'); + git(dir, 'push', 'origin', 'HEAD:refs/heads/main'); + git(dir, 'checkout', '-b', 'feature'); + + // PR commit: rename old.ts -> new.ts (pure rename, no content change) + git(dir, 'mv', 'old.ts', 'new.ts'); + git(dir, 'commit', '-m', 'rename old.ts to new.ts'); + + const result = await sourceLocalPRDiffs({ + files: [ + makeFile({ + filename: 'new.ts', + previousFilename: 'old.ts', + status: 'renamed', + additions: 0, + deletions: 0, + changes: 0, + patch: undefined, + }), + ], + repoDir: dir, + baseBranch: 'main', + logWriter: vi.fn(), + }); + + expect(result.files).toHaveLength(1); + expect(result.files[0].patchSource).toBe('local-git'); + // Proper rename metadata — not 'new file mode' from a destination-only pathspec + expect(result.files[0].patch).toContain('rename from old.ts'); + expect(result.files[0].patch).toContain('rename to new.ts'); + expect(result.files[0].patch).not.toContain('new file mode'); + }); + + it('classifies binary files as no-patch instead of local-git', async () => { + // Regression for binary diff metadata: git diff emits + // "Binary files /dev/null and b/blob.bin differ" with no @@ hunks, but the + // output is non-empty. The helper must not classify this as 'local-git' — + // binary files should be listed in SKIPPED FILES rather than injecting + // misleading binary metadata into the review context. + const remoteDir = mkdtempSync(join(tmpdir(), 'cascade-remote-binary-')); + tempDirs.push(remoteDir); + execFileSync('git', ['init', '--bare'], { cwd: remoteDir, stdio: 'pipe' }); + + const dir = mkdtempSync(join(tmpdir(), 'cascade-pr-diff-binary-')); + tempDirs.push(dir); + git(dir, 'init'); + git(dir, 'config', 'user.email', 'test@example.com'); + git(dir, 'config', 'user.name', 'Test User'); + git(dir, 'remote', 'add', 'origin', remoteDir); + + // Base commit with only a text file + writeFileSync(join(dir, 'text.ts'), 'const a = 1;\n'); + git(dir, 'add', 'text.ts'); + git(dir, 'commit', '-m', 'base'); + git(dir, 'push', 'origin', 'HEAD:refs/heads/main'); + git(dir, 'checkout', '-b', 'feature'); + + // PR commit: add a binary file (null byte causes git to treat it as binary) + writeFileSync(join(dir, 'blob.bin'), Buffer.from([0x00, 0xff, 0x00, 0xff])); + git(dir, 'add', 'blob.bin'); + git(dir, 'commit', '-m', 'add binary blob'); + + const result = await sourceLocalPRDiffs({ + files: [ + makeFile({ + filename: 'blob.bin', + status: 'added', + additions: 0, + deletions: 0, + changes: 0, + patch: undefined, // GitHub returns no patch for binary files + }), + ], + repoDir: dir, + baseBranch: 'main', + logWriter: vi.fn(), + }); + + expect(result.files).toHaveLength(1); + // Binary diffs must NOT be classified as local-git (the regression guard) + expect(result.files[0].patchSource).not.toBe('local-git'); + // GitHub also has no patch — so this should be no-patch, not local-diff-empty + expect(result.files[0].patchSource).toBe('no-patch'); + // The patch field must be undefined — no binary metadata passed to the LLM + expect(result.files[0].patch).toBeUndefined(); + }); +}); diff --git a/tests/unit/agents/shared/prFormatting.test.ts b/tests/unit/agents/shared/prFormatting.test.ts index 7268b0882..65bcbb460 100644 --- a/tests/unit/agents/shared/prFormatting.test.ts +++ b/tests/unit/agents/shared/prFormatting.test.ts @@ -391,7 +391,12 @@ describe('extractPRDiffs', () => { it('handles empty PR diff input', () => { const result = extractPRDiffs([]); - expect(result).toEqual({ included: [], skipped: [] }); + expect(result).toEqual({ + included: [], + skipped: [], + totalDiffTokens: 0, + perFileTokenCap: expect.any(Number), + }); }); it('per-file diff contains a header with filename, status, and line-change counts', () => { @@ -418,4 +423,42 @@ describe('extractPRDiffs', () => { it('sanity: budget constant matches imported value (guards against drift)', () => { expect(REVIEW_DIFF_CONTEXT_TOKEN_LIMIT).toBeGreaterThan(0); }); + + it('marks local diff failures as skipped instead of trusting an API patch', () => { + const prDiff: PRDiff = [ + makePRFile({ + filename: 'api-clipped.ts', + patch: undefined, + patchSource: 'local-diff-failed', + } as Partial), + ]; + + const result = extractPRDiffs(prDiff); + + expect(result.included).toHaveLength(0); + expect(result.skipped).toEqual([ + { filename: 'api-clipped.ts', reason: 'local-diff-failed' }, + ]); + }); + + it('preserves local-git source metadata and token counts for included files', () => { + const prDiff: PRDiff = [ + makePRFile({ + filename: 'local.ts', + patch: '@@ -1 +1 @@\n+local', + patchSource: 'local-git', + } as Partial), + ]; + + const result = extractPRDiffs(prDiff); + + expect(result.included[0]).toEqual( + expect.objectContaining({ + path: 'local.ts', + patchSource: 'local-git', + tokens: expect.any(Number), + }), + ); + expect(result.totalDiffTokens).toBe(result.included[0].tokens); + }); }); diff --git a/tests/unit/backends/agent-profiles.test.ts b/tests/unit/backends/agent-profiles.test.ts index 468a0a449..535cd9750 100644 --- a/tests/unit/backends/agent-profiles.test.ts +++ b/tests/unit/backends/agent-profiles.test.ts @@ -164,7 +164,6 @@ import { import { formatPRComments, formatPRDetails, - formatPRDiff, formatPRIssueComments, formatPRReviews, } from '../../../src/agents/shared/prFormatting.js'; @@ -753,7 +752,7 @@ describe('fetchReviewContext', () => { mockGithub.getCheckSuiteStatus.mockResolvedValue({ checks: [] } as never); }); - it('includes PR injections (Details, Diff, Checks)', async () => { + it('includes PR injections (Details, compact Diff Context, Checks)', async () => { const profile = await getAgentProfile('review'); const params = makeContextParams({ triggerEvent: 'scm:check-suite-success', @@ -767,7 +766,8 @@ describe('fetchReviewContext', () => { const toolNames = injections.map((i) => i.toolName); expect(toolNames).toContain('GetPRDetails'); - expect(toolNames).toContain('GetPRDiff'); + expect(toolNames).toContain('GetPRDiffContext'); + expect(toolNames).not.toContain('GetPRDiff'); expect(toolNames).toContain('GetPRChecks'); }); @@ -832,7 +832,6 @@ describe('fetchReviewContext', () => { await profile.fetchContext(params as Parameters[0]); expect(vi.mocked(formatPRDetails)).toHaveBeenCalled(); - expect(vi.mocked(formatPRDiff)).toHaveBeenCalled(); }); }); @@ -860,7 +859,8 @@ describe('fetchCIContext', () => { const toolNames = injections.map((i) => i.toolName); expect(toolNames).toContain('GetPRDetails'); - expect(toolNames).toContain('GetPRDiff'); + expect(toolNames).toContain('GetPRDiffContext'); + expect(toolNames).not.toContain('GetPRDiff'); expect(toolNames).toContain('GetPRChecks'); expect(toolNames).toContain('ListDirectory'); expect(toolNames).toContain('ReadFile'); @@ -909,7 +909,8 @@ describe('fetchPRCommentResponseContext', () => { const toolNames = injections.map((i) => i.toolName); expect(toolNames).toContain('GetPRDetails'); - expect(toolNames).toContain('GetPRDiff'); + expect(toolNames).toContain('GetPRDiffContext'); + expect(toolNames).not.toContain('GetPRDiff'); expect(toolNames).toContain('GetPRChecks'); // 3 conversation injections (all tagged as GetPRComments) diff --git a/tests/unit/cli/scm/scm-commands.test.ts b/tests/unit/cli/scm/scm-commands.test.ts index 36636de95..19d7deaaf 100644 --- a/tests/unit/cli/scm/scm-commands.test.ts +++ b/tests/unit/cli/scm/scm-commands.test.ts @@ -141,7 +141,7 @@ describe('GetPRDiff command', () => { const cmd = new GetPRDiff(['--prNumber', '15'], makeMockConfig() as never); await cmd.run(); - expect(getPRDiff).toHaveBeenCalledWith('owner', 'repo', 15); + expect(getPRDiff).toHaveBeenCalledWith('owner', 'repo', 15, undefined); }); it('resolves owner/repo from env vars', async () => { @@ -150,7 +150,17 @@ describe('GetPRDiff command', () => { const cmd = new GetPRDiff(['--prNumber', '99'], makeMockConfig() as never); await cmd.run(); - expect(getPRDiff).toHaveBeenCalledWith('acme', 'webapp', 99); + expect(getPRDiff).toHaveBeenCalledWith('acme', 'webapp', 99, undefined); + }); + + it('passes optional path to getPRDiff', async () => { + const cmd = new GetPRDiff( + ['--prNumber', '15', '--path', 'src/old.ts'], + makeMockConfig() as never, + ); + await cmd.run(); + + expect(getPRDiff).toHaveBeenCalledWith('owner', 'repo', 15, 'src/old.ts'); }); it('outputs JSON success result', async () => { diff --git a/tests/unit/gadgets/github/core/getPRDiff.test.ts b/tests/unit/gadgets/github/core/getPRDiff.test.ts index b7384b716..97e02c6b3 100644 --- a/tests/unit/gadgets/github/core/getPRDiff.test.ts +++ b/tests/unit/gadgets/github/core/getPRDiff.test.ts @@ -64,6 +64,52 @@ describe('getPRDiff', () => { expect(result).toBe('No files changed in this PR.'); }); + it('filters by current or previous filename when path is supplied', async () => { + mockGithub.getPRDiff.mockResolvedValue([ + { + filename: 'src/new.ts', + previousFilename: 'src/old.ts', + status: 'renamed', + additions: 1, + deletions: 1, + changes: 2, + patch: '@@ -1 +1 @@', + }, + { + filename: 'src/other.ts', + status: 'modified', + additions: 1, + deletions: 0, + changes: 1, + patch: '@@ -2 +2 @@', + }, + ] as Awaited>); + + const result = await getPRDiff('owner', 'repo', 42, 'src/old.ts'); + + expect(result).toContain('1 file(s) changed:'); + expect(result).toContain('## src/new.ts'); + expect(result).toContain('Previous filename: src/old.ts'); + expect(result).not.toContain('src/other.ts'); + }); + + it('returns a clear message when path filter matches no changed file', async () => { + mockGithub.getPRDiff.mockResolvedValue([ + { + filename: 'src/other.ts', + status: 'modified', + additions: 1, + deletions: 0, + changes: 1, + patch: '@@ -2 +2 @@', + }, + ] as Awaited>); + + const result = await getPRDiff('owner', 'repo', 42, 'src/missing.ts'); + + expect(result).toBe('No changed file matched path: src/missing.ts'); + }); + it('returns error message string when githubClient throws', async () => { mockGithub.getPRDiff.mockRejectedValue(new Error('API rate limit exceeded')); diff --git a/tests/unit/gadgets/github/definitions.test.ts b/tests/unit/gadgets/github/definitions.test.ts index ccf8a2500..f5b1775f7 100644 --- a/tests/unit/gadgets/github/definitions.test.ts +++ b/tests/unit/gadgets/github/definitions.test.ts @@ -11,6 +11,7 @@ import { replyToReviewCommentDef, updatePRCommentDef, } from '../../../../src/gadgets/github/definitions.js'; +import { buildZodSchema } from '../../../../src/gadgets/shared/gadgetFactory.js'; import type { ToolDefinition } from '../../../../src/gadgets/shared/toolDefinition.js'; const ALL_SCM_DEFINITIONS: ToolDefinition[] = [ @@ -287,6 +288,43 @@ describe('GitHub SCM gadget definitions', () => { }); }); +// ─── GetPRDiff specific ─────────────────────────────────────────────────── +describe('getPRDiffDef', () => { + it('has required owner, repo, and prNumber parameters', () => { + expect(getPRDiffDef.parameters.owner?.required).toBe(true); + expect(getPRDiffDef.parameters.repo?.required).toBe(true); + expect(getPRDiffDef.parameters.prNumber?.required).toBe(true); + }); + + it('path parameter is optional (not required)', () => { + expect(getPRDiffDef.parameters.path?.optional).toBe(true); + expect(getPRDiffDef.parameters.path?.required).toBeUndefined(); + }); + + it('generated schema accepts a call without path (full-PR behavior unchanged)', () => { + const schema = buildZodSchema(getPRDiffDef.parameters); + const result = schema.safeParse({ + comment: 'Get full PR diff', + owner: 'acme', + repo: 'myapp', + prNumber: 42, + }); + expect(result.success).toBe(true); + }); + + it('generated schema accepts a call with path (single-file filtering)', () => { + const schema = buildZodSchema(getPRDiffDef.parameters); + const result = schema.safeParse({ + comment: 'Get diff for a specific file', + owner: 'acme', + repo: 'myapp', + prNumber: 42, + path: 'src/foo.ts', + }); + expect(result.success).toBe(true); + }); +}); + // --------------------------------------------------------------------------- // Spec 014 plan 2: createPRReviewDef declarative opt-in // --------------------------------------------------------------------------- diff --git a/tests/unit/gadgets/sentry/format.test.ts b/tests/unit/gadgets/sentry/format.test.ts index f0608e4d9..933ae0b39 100644 --- a/tests/unit/gadgets/sentry/format.test.ts +++ b/tests/unit/gadgets/sentry/format.test.ts @@ -111,6 +111,22 @@ describe('formatSentryEvent', () => { expect(result).toContain('os: {"name":"Linux"}'); }); + it('includes issue permalink metadata when provided', () => { + const result = formatSentryEvent(makeRestEvent(), { + id: '119054737', + title: 'TypeError group', + permalink: 'https://sentry.io/organizations/mongrel/issues/119054737/', + shortId: 'CASCADE-1', + }); + + expect(result).toContain( + 'Sentry issue: https://sentry.io/organizations/mongrel/issues/119054737/', + ); + expect(result).toContain('Issue ID: 119054737'); + expect(result).toContain('Short ID: CASCADE-1'); + expect(result).toContain('Issue title: TypeError group'); + }); + it('formats tuple-array tags', () => { const result = formatSentryEvent({ title: 'Tuple tags', diff --git a/tests/unit/gadgets/sentry/getSentryEventDetail.test.ts b/tests/unit/gadgets/sentry/getSentryEventDetail.test.ts index 573f230d2..2a0de4fac 100644 --- a/tests/unit/gadgets/sentry/getSentryEventDetail.test.ts +++ b/tests/unit/gadgets/sentry/getSentryEventDetail.test.ts @@ -2,12 +2,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { SentryEvent } from '../../../../src/sentry/types.js'; -const { mockGetIssueEvent } = vi.hoisted(() => ({ +const { mockGetIssueEvent, mockGetIssue } = vi.hoisted(() => ({ mockGetIssueEvent: vi.fn(), + mockGetIssue: vi.fn(), })); vi.mock('../../../../src/sentry/client.js', () => ({ getSentryClient: vi.fn(() => ({ + getIssue: mockGetIssue, getIssueEvent: mockGetIssueEvent, })), })); @@ -56,14 +58,27 @@ function makeRestEvent(): SentryEvent { describe('getSentryEventDetail', () => { beforeEach(() => { mockGetIssueEvent.mockReset(); + mockGetIssue.mockReset(); }); it('formats REST-shaped issue-event responses', async () => { mockGetIssueEvent.mockResolvedValueOnce(makeRestEvent()); + mockGetIssue.mockResolvedValueOnce({ + id: '119054737', + title: 'TypeError group', + permalink: 'https://sentry.io/organizations/mongrel/issues/119054737/', + shortId: 'CASCADE-1', + }); const result = await getSentryEventDetail('mongrel', '119054737', 'latest'); expect(mockGetIssueEvent).toHaveBeenCalledWith('mongrel', '119054737', 'latest'); + expect(mockGetIssue).toHaveBeenCalledWith('mongrel', '119054737'); + expect(result).toContain( + 'Sentry issue: https://sentry.io/organizations/mongrel/issues/119054737/', + ); + expect(result).toContain('Issue ID: 119054737'); + expect(result).toContain('Short ID: CASCADE-1'); expect(result).toContain('Event ID: abcdef1234567890'); expect(result).toContain('Tags: environment=production'); expect(result).toContain('## Exception'); @@ -73,9 +88,20 @@ describe('getSentryEventDetail', () => { it('defaults eventId to latest', async () => { mockGetIssueEvent.mockResolvedValueOnce(makeRestEvent()); + mockGetIssue.mockResolvedValueOnce({ id: '119054737', title: 'TypeError group' }); await getSentryEventDetail('mongrel', '119054737'); expect(mockGetIssueEvent).toHaveBeenCalledWith('mongrel', '119054737', 'latest'); }); + + it('returns event details when issue metadata fetch fails', async () => { + mockGetIssueEvent.mockResolvedValueOnce(makeRestEvent()); + mockGetIssue.mockRejectedValueOnce(new Error('Sentry issue metadata unavailable')); + + const result = await getSentryEventDetail('mongrel', '119054737', 'latest'); + + expect(result).toContain('Event ID: abcdef1234567890'); + expect(result).not.toContain('Sentry issue:'); + }); }); diff --git a/tests/unit/github/client.test.ts b/tests/unit/github/client.test.ts index c64e67494..48b3c82c5 100644 --- a/tests/unit/github/client.test.ts +++ b/tests/unit/github/client.test.ts @@ -666,6 +666,7 @@ describe('githubClient', () => { data: [ { filename: 'src/index.ts', + previous_filename: 'src/old-index.ts', status: 'modified', additions: 10, deletions: 5, @@ -682,6 +683,7 @@ describe('githubClient', () => { expect(result).toHaveLength(1); expect(result[0]).toEqual({ filename: 'src/index.ts', + previousFilename: 'src/old-index.ts', status: 'modified', additions: 10, deletions: 5, diff --git a/tests/unit/integrations/alerting/format-sentry.test.ts b/tests/unit/integrations/alerting/format-sentry.test.ts index 72175dc94..33fd7b4ee 100644 --- a/tests/unit/integrations/alerting/format-sentry.test.ts +++ b/tests/unit/integrations/alerting/format-sentry.test.ts @@ -73,4 +73,22 @@ describe('formatSentryCardBody', () => { const result = formatSentryCardBody(fixturePayload); expect(result.descriptionMarkdown).toContain('processWebhook'); }); + + it('does not use non-useful title candidates', () => { + const result = formatSentryCardBody({ + ...fixturePayload, + payload: { + ...fixturePayload.payload, + data: { + ...fixturePayload.payload.data, + issue_alert: { title: 'undefined' }, + triggered_rule: ' ', + event: { ...fixturePayload.payload.data.event, title: 'Real event title' }, + }, + }, + }); + + expect(result.title).toBe('[Sentry] Real event title'); + expect(result.title).not.toContain('undefined'); + }); }); diff --git a/tests/unit/triggers/sentry-alerting.test.ts b/tests/unit/triggers/sentry-alerting.test.ts index 39f0d66cd..7e81dffe7 100644 --- a/tests/unit/triggers/sentry-alerting.test.ts +++ b/tests/unit/triggers/sentry-alerting.test.ts @@ -230,6 +230,16 @@ describe('SentryIssueAlertTrigger', () => { expect(result?.agentInput?.alertTitle).toBe('Issue Alert'); }); + it('skips empty and stringified undefined title candidates', async () => { + const ctx = makeSentryIssueAlertCtx({ + issueAlertTitle: 'undefined', + triggeredRule: ' ', + eventOverrides: { title: 'Real event title' }, + }); + const result = await trigger.handle(ctx); + expect(result?.agentInput?.alertTitle).toBe('Real event title'); + }); + it('sets alertIssueUrl from event.web_url', async () => { const result = await trigger.handle(makeSentryIssueAlertCtx()); expect(result?.agentInput?.alertIssueUrl).toBe('https://sentry.io/issues/issue-42/'); @@ -397,6 +407,15 @@ describe('SentryMetricAlertTrigger', () => { expect(result?.agentInput?.alertTitle).toBe('Metric Alert (critical)'); }); + it('skips empty and stringified undefined metric title candidates', async () => { + const ctx = makeSentryMetricAlertCtx({ + descriptionTitle: 'undefined', + aggregate: ' p95(transaction.duration) ', + }); + const result = await trigger.handle(ctx); + expect(result?.agentInput?.alertTitle).toBe('p95(transaction.duration)'); + }); + it('sets alertIssueUrl from data.web_url', async () => { const ctx = makeSentryMetricAlertCtx({ webUrl: 'https://sentry.io/alerts/123/' }); const result = await trigger.handle(ctx); diff --git a/tests/unit/triggers/sentry-webhook-handler.test.ts b/tests/unit/triggers/sentry-webhook-handler.test.ts index 5c06119f7..d066598b1 100644 --- a/tests/unit/triggers/sentry-webhook-handler.test.ts +++ b/tests/unit/triggers/sentry-webhook-handler.test.ts @@ -28,6 +28,14 @@ vi.mock('../../../src/triggers/shared/trigger-resolution.js', () => ({ resolveTriggerResult: vi.fn(), })); +vi.mock('../../../src/pm/registry.js', () => ({ + pmRegistry: { + createProvider: vi.fn(() => ({ + getWorkItemUrl: (id: string) => `https://pm.example.test/${id}`, + })), + }, +})); + // Mock the dynamically-imported materialization helpers vi.mock('../../../src/integrations/alerting/_shared/materialize.js', () => ({ materializeAlertWorkItem: vi.fn(), @@ -224,7 +232,11 @@ describe('processSentryWebhook', () => { const payload = { resource: 'event_alert', cascadeProjectId: 'proj-sentry' }; const triggerResult = { agentType: 'alerting', - agentInput: { alertIssueId: 'sentry-issue-42' }, + agentInput: { + triggerEvent: 'alerting:issue-alert', + alertIssueId: 'sentry-issue-42', + alertIssueUrl: 'https://sentry.io/issues/sentry-issue-42/', + }, } as never; vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); vi.mocked(materializeAlertWorkItem).mockResolvedValue('card-new'); @@ -240,7 +252,14 @@ describe('processSentryWebhook', () => { expect(runAgentExecutionPipeline).toHaveBeenCalledWith( expect.objectContaining({ workItemId: 'card-new', - agentInput: expect.objectContaining({ workItemId: 'card-new' }), + workItemTitle: '[Sentry] Test', + workItemUrl: 'https://pm.example.test/card-new', + agentInput: expect.objectContaining({ + workItemId: 'card-new', + workItemTitle: '[Sentry] Test', + workItemUrl: 'https://pm.example.test/card-new', + alertIssueUrl: 'https://sentry.io/issues/sentry-issue-42/', + }), }), mockProject, expect.any(Object), @@ -325,7 +344,10 @@ describe('processSentryWebhook', () => { const payload = { resource: 'metric_alert', cascadeProjectId: 'proj-sentry' }; const triggerResult = { agentType: 'alerting', - agentInput: { alertMetricKey: 'my-org:Error Rate High' }, + agentInput: { + alertMetricKey: 'my-org:Error Rate High', + alertIssueUrl: 'https://sentry.io/alerts/123/', + }, lockKey: 'sentry-metric:my-org:Error Rate High', } as never; vi.mocked(resolveTriggerResult).mockResolvedValue(triggerResult); @@ -343,7 +365,14 @@ describe('processSentryWebhook', () => { expect(runAgentExecutionPipeline).toHaveBeenCalledWith( expect.objectContaining({ workItemId: 'metric-card-1', - agentInput: expect.objectContaining({ workItemId: 'metric-card-1' }), + workItemTitle: '[Sentry Metric] Error Rate High', + workItemUrl: 'https://pm.example.test/metric-card-1', + agentInput: expect.objectContaining({ + workItemId: 'metric-card-1', + workItemTitle: '[Sentry Metric] Error Rate High', + workItemUrl: 'https://pm.example.test/metric-card-1', + alertIssueUrl: 'https://sentry.io/alerts/123/', + }), }), mockProject, expect.any(Object), @@ -421,6 +450,7 @@ describe('processSentryWebhook', () => { agentInput: { triggerEvent: 'alerting:issue-lifecycle', alertIssueId: '118723355', + alertIssueUrl: 'https://mongrel.sentry.io/issues/118723355/', }, lockKey: 'sentry-issue:118723355', } as never; @@ -441,7 +471,14 @@ describe('processSentryWebhook', () => { expect(runAgentExecutionPipeline).toHaveBeenCalledWith( expect.objectContaining({ workItemId: 'issue-card-1', - agentInput: expect.objectContaining({ workItemId: 'issue-card-1' }), + workItemTitle: '[Sentry] wedged work-item lock', + workItemUrl: 'https://pm.example.test/issue-card-1', + agentInput: expect.objectContaining({ + workItemId: 'issue-card-1', + workItemTitle: '[Sentry] wedged work-item lock', + workItemUrl: 'https://pm.example.test/issue-card-1', + alertIssueUrl: 'https://mongrel.sentry.io/issues/118723355/', + }), }), mockProject, expect.any(Object), diff --git a/tests/unit/triggers/sentry/issue-lifecycle.test.ts b/tests/unit/triggers/sentry/issue-lifecycle.test.ts index f14a5d708..592c1475d 100644 --- a/tests/unit/triggers/sentry/issue-lifecycle.test.ts +++ b/tests/unit/triggers/sentry/issue-lifecycle.test.ts @@ -205,6 +205,13 @@ describe('SentryIssueLifecycleTrigger', () => { expect(result?.agentInput?.alertTitle).toBe('Sentry Issue'); }); + it('falls back to default alertTitle when title is stringified undefined', async () => { + const result = await trigger.handle( + makeIssueLifecycleCtx({ issueOverrides: { title: 'undefined' } }), + ); + expect(result?.agentInput?.alertTitle).toBe('Sentry Issue'); + }); + it('falls back to permalink for alertIssueUrl when web_url is missing', async () => { const result = await trigger.handle( makeIssueLifecycleCtx({