Skip to content

feat(context): include diffHunk in PR review comment context - #1584

Open
rishavnaskar wants to merge 2 commits into
anthropics:mainfrom
rishavnaskar:feat/review-comment-diff-hunk
Open

feat(context): include diffHunk in PR review comment context#1584
rishavnaskar wants to merge 2 commits into
anthropics:mainfrom
rishavnaskar:feat/review-comment-diff-hunk

Conversation

@rishavnaskar

Copy link
Copy Markdown
Contributor

Fixes #855.

Review comments were passed to Claude with only path and line, so the code the comment was actually written against never reached the prompt. diffHunk is available on PullRequestReviewComment in the GraphQL API but wasn't being selected.

Before

  [Comment on src/index.ts:42]: This can overflow

After

  [Comment on src/index.ts:42]: This can overflow
  Diff context:
  ```diff
  @@ -40,3 +40,3 @@
  -const a = 1;
  +const a = 2;

Three changes: select `diffHunk` in the reviews query, add it to `GitHubReviewComment` as optional, and render it under the comment.

Two notes on the implementation:

- The hunk is PR-authored content reaching the prompt, so it goes through `sanitizeContent` the same way the comment body does.
- It's optional and the render is guarded, so comments without a hunk produce byte-identical output to before. All existing `formatReviewComments` tests pass unchanged.

Verified `diffHunk` against the live schema rather than assuming:

gh api graphql -f query='query { __type(name: "PullRequestReviewComment") { fields { name } } }'
→ diffHunk, line, path


### Tests

Two cases added to `test/data-formatter.test.ts`: a comment with a hunk, and one without. On `main` the first fails (`Expected to contain: "Diff context:"`); the second passes both before and after, which is the control showing existing output is unaffected.

bun test 806 pass, 0 fail
bun run typecheck clean
bun run format:check clean

Review comments arrived with only path and line, so the code they were
written against was missing from the prompt. Fetch diffHunk in the PR
GraphQL query and render it under the comment as a diff block.

The hunk is PR-authored content, so it goes through sanitizeContent like
the comment body. Comments without a hunk are unchanged.

Fixes anthropics#855
GitHub returns diffHunk: "" (not null) for comments whose line no longer
exists in the diff, so the render guard has to reject empty strings too.
Found running the real query against anthropics#1025.
@rishavnaskar

Copy link
Copy Markdown
Contributor Author

Verified end to end rather than only in unit tests — ran the modified PR_QUERY against real public PRs and piped the live response through formatReviewComments:

PR review comments populated diffHunk on main on this branch
cli/cli#13988 1 0 1
denoland/deno#36111 3 0 3
anthropics/claude-code-action#1025 1 0 0 (see below)

Real output for cli/cli#13988:

  [Comment on api/client_test.go:193]: Interesting that we didn't have a test for this! 🤔
  Diff context:
```diff
@@ -138,6 +139,100 @@ func TestRESTError(t *testing.T) {
 	}
 }

+func TestRESTWithNextError(t *testing.T) {

That third row turned up an edge case worth pinning: #1025's comment is outdated, and GitHub returns line: null with diffHunk: "" — an empty string, not null. The guard already skips it because "" is falsy, but a later change to diffHunk != null would emit an empty ```diff block. Added a regression test for it in 8021217.

bun test              807 pass, 0 fail
bun run typecheck     clean
bun run format:check  clean

@rishavnaskar

Copy link
Copy Markdown
Contributor Author

On the failing checks — they're the credential-gated integration suites, not this change.

They abort before running any code:

Secret source: None
##[error]Environment variable validation failed:
  - Either ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN, or workload identity federation
    (ANTHROPIC_FEDERATION_RULE_ID and ANTHROPIC_ORGANIZATION_ID) is required

Fork PRs don't get repository secrets, so these can't pass from a fork. Same pattern on unrelated fork PRs (e.g. #1576): six passing, thirty failing.

All six secret-free checks are green here — test, typecheck, prettier (and their ci / duplicates), so the full unit suite passes in CI as well as locally.

Happy to rebase or adjust if anything in the diff needs it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include diffHunk in PR review comment context

1 participant