Repository navigation
New ai review instructions - #22
knutviljar wants to merge 6 commits into
Conversation
A code review of a medium sized change could require 5-10 steps just using the read tool for the modified files. Adding steps for git commands, reading test suites and more I believe we could easily reach the limit of 20.
I believe a common workflow is to work on some new code, commit changes you are happy with and then ask the AI for a review before pushing/creating PR. Therefore I think the reviewer subagent should look for a base branch to compare against instead of returning early.
The sub-agent would need these git commands in order to track which branch it should compare commited changes against. See 1256e28 for more context
|
Whether this branch should be merged or not is not that important, just wanted to trigger some discussions around the reviewer sub-agent. Will probably do something similar with AGENTS.md. Couldn't deduce any label best practice from earlier PRs, so did not use any labels on this one |
There was a problem hiding this comment.
I like the improvements. But we should discuss what the behaviour should be with and without arguments.
As it is now the behaviour on a clean branch is essentially the same when using the commands /review and /review main, but with different wording/prompt. I like the idea that using the command without arguments handles both the clean branch case and the unclean branch case. But then maybe we should change the /review ref case to be a pure diff between the reference/branch and the current branch? Instead of a diff from the common ancestor. What do you think?
There was a problem hiding this comment.
Agree, would love to discuss. I am going to test a bit how the agent responds to using the command vs. just asking for a review. I believe it will almost always invoke the review skill in the latter case as well and I believe this will be the more "normal" way of asking for review for the laymen (the command requires a certain know-how and is easy to forget exists)
The idea here is that the ssb-reviewer sub-agent should not have more context than necessary to do the review. The skill file describes what the reviewer subagent should do when reviewing
637c8dc to
b900c71
Compare
|
Made some changes to the reviewer sub-agent instructions that I think are meaningful. I guess this PR is mostly meant as a way to discuss what should be included in these files, let me know what you think!