fix(agent): only attach PR URLs from gh pr create#1964
Open
Conversation
Today any GitHub PR URL appearing in a bash tool's output gets attached to TaskRun.output.pr_url, which over-catches: gh pr view, gh search prs, gh pr list, and similar lookups all latch unrelated PRs onto the run and trigger downstream effects (Slack notifications, UI PR-status rendering). Gate the existing detection on the originating bash command containing gh pr create. The bash command is threaded onto _meta.claudeCode in sdk-to-acp.ts so consumers don't have to correlate tool_call to tool_call_update messages. The detection regex/text extraction is now shared between the desktop service and the cloud agent-server via a new pr-url-detector helper, replacing two byte-identical implementations. Generated-By: PostHog Code Task-Id: 302a50dd-3e6e-4f4c-abbf-4ca0e0f9250d
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/agent/src/pr-url-detector.test.ts:7-137
**Prefer parameterised tests**
All 13 test cases follow the exact same shape — vary `toolName`, `bashCommand`, `toolResponse`/`content`, and expect a URL or `null`. Per the project's simplicity rules, this should be a single `it.each()` table rather than 13 separate `it()` blocks.
```typescript
it.each([
["gh pr create (string response)", "Bash", 'gh pr create --title "x"', `${PR_URL}\n`, undefined, PR_URL],
["gh pr create (object stdout)", "Bash", "gh pr create --fill", { stdout: `${PR_URL}\n` }, undefined, PR_URL],
["gh pr view ignored", "Bash", `gh pr view ${PR_URL}`, { stdout: PR_URL }, undefined, null],
// … remaining rows
])(
"%s",
(_desc, toolName, bashCommand, toolResponse, content, expected) => {
expect(extractCreatedPrUrl({ toolName, bashCommand, toolResponse, content }))
.toBe(expected);
},
);
```
This also makes it trivial to add new cases without boilerplate.
Reviews (1): Last reviewed commit: "fix(agent): only attach PR URLs from gh ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+7
to
+137
| it("returns the URL when gh pr create produced it (string toolResponse)", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: 'gh pr create --title "x" --body "y"', | ||
| toolResponse: `${PR_URL}\n`, | ||
| }), | ||
| ).toBe(PR_URL); | ||
| }); | ||
|
|
||
| it("returns the URL when gh pr create produced it (object toolResponse)", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: "gh pr create --fill", | ||
| toolResponse: { stdout: `${PR_URL}\n`, stderr: "" }, | ||
| }), | ||
| ).toBe(PR_URL); | ||
| }); | ||
|
|
||
| it("ignores PR URLs from gh pr view", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: `gh pr view ${PR_URL}`, | ||
| toolResponse: { stdout: PR_URL }, | ||
| }), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("ignores PR URLs from gh search prs", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: 'gh search prs "fix login"', | ||
| toolResponse: PR_URL, | ||
| }), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("ignores PR URLs from gh pr list", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: "gh pr list --json url", | ||
| toolResponse: PR_URL, | ||
| }), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns null when bashCommand is missing", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: undefined, | ||
| toolResponse: PR_URL, | ||
| }), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns null for non-Bash tools", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Edit", | ||
| bashCommand: "gh pr create", | ||
| toolResponse: PR_URL, | ||
| }), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("accepts the lowercase 'bash' tool variant", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "bash", | ||
| bashCommand: "gh pr create", | ||
| toolResponse: PR_URL, | ||
| }), | ||
| ).toBe(PR_URL); | ||
| }); | ||
|
|
||
| it("returns null when output has no PR URL", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: "gh pr create", | ||
| toolResponse: "no pr was created", | ||
| }), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it("finds the URL in the content array when toolResponse is empty", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: "gh pr create --fill", | ||
| toolResponse: undefined, | ||
| content: [{ type: "text", text: `Created: ${PR_URL}` }], | ||
| }), | ||
| ).toBe(PR_URL); | ||
| }); | ||
|
|
||
| it("handles output field on object toolResponse", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: "gh pr create", | ||
| toolResponse: { output: PR_URL }, | ||
| }), | ||
| ).toBe(PR_URL); | ||
| }); | ||
|
|
||
| it("matches gh pr create even with a chained command", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: "git push -u origin feat/x && gh pr create --fill", | ||
| toolResponse: { stdout: PR_URL }, | ||
| }), | ||
| ).toBe(PR_URL); | ||
| }); | ||
|
|
||
| it("does not match a fake command containing 'pr create' as text", () => { | ||
| expect( | ||
| extractCreatedPrUrl({ | ||
| toolName: "Bash", | ||
| bashCommand: "echo 'i should pr create later'", | ||
| toolResponse: PR_URL, | ||
| }), | ||
| ).toBeNull(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
All 13 test cases follow the exact same shape — vary toolName, bashCommand, toolResponse/content, and expect a URL or null. Per the project's simplicity rules, this should be a single it.each() table rather than 13 separate it() blocks.
it.each([
["gh pr create (string response)", "Bash", 'gh pr create --title "x"', `${PR_URL}\n`, undefined, PR_URL],
["gh pr create (object stdout)", "Bash", "gh pr create --fill", { stdout: `${PR_URL}\n` }, undefined, PR_URL],
["gh pr view ignored", "Bash", `gh pr view ${PR_URL}`, { stdout: PR_URL }, undefined, null],
// … remaining rows
])(
"%s",
(_desc, toolName, bashCommand, toolResponse, content, expected) => {
expect(extractCreatedPrUrl({ toolName, bashCommand, toolResponse, content }))
.toBe(expected);
},
);This also makes it trivial to add new cases without boilerplate.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/pr-url-detector.test.ts
Line: 7-137
Comment:
**Prefer parameterised tests**
All 13 test cases follow the exact same shape — vary `toolName`, `bashCommand`, `toolResponse`/`content`, and expect a URL or `null`. Per the project's simplicity rules, this should be a single `it.each()` table rather than 13 separate `it()` blocks.
```typescript
it.each([
["gh pr create (string response)", "Bash", 'gh pr create --title "x"', `${PR_URL}\n`, undefined, PR_URL],
["gh pr create (object stdout)", "Bash", "gh pr create --fill", { stdout: `${PR_URL}\n` }, undefined, PR_URL],
["gh pr view ignored", "Bash", `gh pr view ${PR_URL}`, { stdout: PR_URL }, undefined, null],
// … remaining rows
])(
"%s",
(_desc, toolName, bashCommand, toolResponse, content, expected) => {
expect(extractCreatedPrUrl({ toolName, bashCommand, toolResponse, content }))
.toBe(expected);
},
);
```
This also makes it trivial to add new cases without boilerplate.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Author
|
will merge after a bit more testing 🫡 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gh pr create, eliminating false positives fromgh pr view,gh search prs,gh pr list, etc.pr-url-detectorhelper, replacing two byte-identical implementations in the desktop service and the cloud agent-server._meta.claudeCode.bashCommandin the SDK→ACP converter so consumers don't have to correlatetool_call→tool_call_update.Why
Today any GitHub PR URL printed by a bash tool gets latched onto
TaskRun.output.pr_url. That fires downstream Slack notifications and PR-status UI off URLs the agent merely referenced (e.g. when looking up a historical PR). A real instance of this misfire produced four "Pull request opened" Slack messages from a singlegh pr viewcall.The most robust signal that a URL is a PR the agent just created is: the command was
gh pr create.git pushonly ever printspull/new/<branch>, which doesn't match the URL regex;gh pr edit/merge/view/list/searchoperate on existing PRs.Test plan
Generated-By: PostHog Code
Task-Id: 302a50dd-3e6e-4f4c-abbf-4ca0e0f9250d