Forward precommit hook failures from "Create PR" to the agent#2968
Forward precommit hook failures from "Create PR" to the agent#2968salihudickson wants to merge 2 commits into
Conversation
|
Reviews (1): Last reviewed commit: "add functionality to forward pre-commit ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const PRE_COMMIT_COMMAND_REGEX = | ||
| /\blefthook\b[\s\S]*?\bpre-commit\b|\bpre-commit\b(?:[\s\S]*?\brun\b|\s+run\b|$)|\bhusky\b|\bovercommit\b|\blint-staged\b|[./]*\.(?:git\/hooks|husky)\/pre-commit\b/i; |
There was a problem hiding this comment.
\bhusky\b and \bovercommit\b match any command that contains those words — including setup/installation commands like husky install, npx husky add .husky/pre-commit, or overcommit --install. When one of those commands fails with a non-zero exit, showCommitFailureActions becomes true and the agent receives a prompt saying "A git commit command failed, most likely because a pre-commit hook rejected the commit" — which is factually wrong. The agent would be directed to diagnose a hook failure that never occurred, against the wrong command context entirely.
Consider narrowing these alternatives to require a hook-execution indicator, for example: \bhusky\b(?:\s+run\b|.*\bpre-commit\b) for husky and \bovercommit\b.*(?:--run\b|\bpre-commit\b) for overcommit. lint-staged is almost always a hook body rather than a setup command, so it is probably fine as-is.
| describe("ExecuteToolView", () => { | ||
| it("shows commit failure actions for a git commit failure with hook output", () => { | ||
| renderView( | ||
| makeToolCall({ | ||
| status: "failed", | ||
| rawInput: { command: 'git commit -m "test"' }, | ||
| content: textContent(`Exit code 1\n╭───╮\n│ hook: pre-commit │\n╰───╯`), | ||
| }), | ||
| ); | ||
|
|
||
| const actions = screen.getByTestId("commit-failure-actions"); | ||
| expect(actions).toHaveAttribute("data-command", 'git commit -m "test"'); | ||
| expect(actions).toHaveAttribute( | ||
| "data-output", | ||
| expect.stringContaining("hook: pre-commit"), | ||
| ); | ||
| }); | ||
|
|
||
| it("still shows commit failure actions when the tool status is completed but the output has a non-zero exit signal", () => { | ||
| renderView( | ||
| makeToolCall({ | ||
| status: "completed", | ||
| rawInput: { command: "pre-commit run --all-files" }, | ||
| content: textContent("exit status 1\npre-commit failed"), | ||
| }), | ||
| ); | ||
|
|
||
| expect(screen.getByTestId("commit-failure-actions")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("does not show commit failure actions for unrelated failed bash output", () => { | ||
| renderView( | ||
| makeToolCall({ | ||
| status: "failed", | ||
| rawInput: { command: "git push origin main" }, | ||
| content: textContent("exit status 1\nnetwork error"), | ||
| }), | ||
| ); | ||
|
|
||
| expect( | ||
| screen.queryByTestId("commit-failure-actions"), | ||
| ).not.toBeInTheDocument(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The three it blocks all exercise the same condition (whether CommitFailureActions is rendered) with different command/output/status combinations. Per project convention, tests like these should be collapsed into a single it.each table. The attribute assertions from the first test can live in a short standalone it if needed.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
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!
|
hey @charlesvien could you please take a look at this? |
1 participant
Problem
fixes #2736
Changes
errorPromt.tsfile, the gives the agent instructions on how to handle commit-hook failuresCommitFailureActions.tsx, which renderscopy errorandfix with agentbuttons on the relevant failed tool outputs.How did you test this?
Automatic notifications