https://docs.all-hands.dev/ logo
Join Slack
Powered by
Hi all! <@U09C4UKV8GP> <@U06PYTVAES1> <@U06QFDJ1QQ...
# proj-extensions
l
Hi all! @curved-angle-80449 @calm-artist-7206 @wooden-umbrella-84098 and I were having a discussion on a private channel about how our code review bot might be approving PRs that it shouldn’t be. I know @modern-fireman-99755 also pointed this out at one point. From my POV: I want our code review bot to be good enough that every time it approves we can push to main without human review. If we’re not there now, I’d love to get there eventually, so I view any time the code review bot approves something that causes a problem later to be a bug in the code review bot. I know that @red-crayon-20970, @important-motherboard-94590, and @jolly-orange-24543 are all looking at improving the code review skill as part of their research projects on skill improvement and code review improvement. So it’d be really helpful to have negative examples — what are examples of PRs that were auto-approved, merged, and caused problems later. If we can get a big list of these (maybe semi-automatically) we could use them later to test future iterations of the code review bot.
👍 1
💯 3
c
One technique I have used to my own agent-enabled code reviews that regularly produces better results than the code review skills is that I prime the conversation by asking to clone the repo or repos that would be connected to the workflow and have summarize itself with how the feature or code path works today before asking it to review the PR. Or for a new feature I will ask it to explore how a similar or adjacent feature works before the code review. This does consume more tokens for sure but it tends to produce a much higher signal review.
> to be good enough that every time it approves we can push to main without human review There are a few angles on this that I think are worth exploring. 1. Human expert knowledge of the whole codebase is going to catch approaches to larger units of work than our agents see today. I often use an agent to draw up a proposal for a significant new capability and then discuss with a human familiar with that section of the code. In almost every case the human interview has a lot of value add even if agent got some things right. 2. When fixing bugs, having the person that introduced the bug participate in the code review is important signal and learning back to the human. 3. When a human works on an issue and merges it without any other human in the loop knowledge of that issue registers only with that one human. While the code is there in the repo, the fewer instances we have where only one person has ever seen that code is likely to be good for building institutional knowldege. It may seem like I am arguing that humans should always be in the loop on merges, but that isn't my position. I have some private repos where the agent writes, reviews, merges and deploys all without any human and I have safeguards built in to the approach that make me comfortable with that. But I do think it's worth thinking about how do we keep the humans connected to what is happening in the code -- how do we build institutional knowledge and memory when so much work is done by agents? I have too much evidence in day-to-day interactions to believe that the institutional knowledge is irrelevant. There may be other ways to achieve than review--but we ought to think carefully about what those are and how they'd work in practice.
l
I don’t disagree! My point is that “when the agent approves it should be safe to merge”, so when it approves something that’s too big to be safe to merge, the agent should not be approving.
c
Just for clarity. I feel this is a different topic than where we started, so I want to outline that first. For months now, the way we worked in agent-sdk (mainly) was that the PR author uses their own judgement when they need another human review; basically we know that easy stuff is fine, and that if it affected core mechanics or agent mechanics or stuff like that, it’s good to pass it by another human. Up until approx ~May, I would say that worked pretty well. I know because I’m a bit of an obsessive reviewer so I actually looked at a lot of PRs, regardless if asked or not, regardless if before or after merge. We had a case when we needed to revert, no biggie, reverted and rebuilt and refined our understanding what needed human review. I’m not exactly sure right now, but I’m a bit blocked by the fact that I wasn’t much here in May so I’m not sure. Please correct me if wrong, I hear in your words that maybe that changed? When do you think humans should ask for human review?
c
I think I was aware that in agent-sdk some people were merging some things without review. But I still saw quite a lot of human review going on. What I hadn't seen before was that this was happening in OpenHands/OpenHands. I came across a bunch of PRs being merged by without another human reviewer and it surprised me, hence my question. I'm not sure it would have surprised me if the contributor had been someone who was regularly working in that area of the code base but it was contributed without visible evidence of consulting with people most intimate with that part of the code. I don't have any doubts or concerns about the contributor's work or capability. It just surprised me. I contribute to all of the core OpenHands repos but as I am not really a core member of any of those teams, I have not been comfortable merging PRs without eyes of others... Perhaps I need to adapt. Not sure.
nod 1
l
Could you share a few of these examples? If they’re mine feel free to call me out 🙂
I’m still trying to come up with a few examples so we can tune the reviewer agent so it will not approve in these cases.
c
That is another detail here, and IMHO it matters. If I’m working in an area I know well, I might consider it safe enough to go with only bot approval; if on the other hand, someone else is basically responsible for the area, I should be inclined to ask them for review. This isn’t always clear; but sometimes it is.
I’m still trying to come up with a few examples so we can tune the reviewer agent so it will not approve in these cases.
Sorry to be dense, I still didn’t understand a little detail, and I would appreciate it if you or someone working on skills tuning could tell me I’m wrong: • I thought the human merging a PR is a signal for the approving reviewer agent trajectory, that is was doing a good job • but we seem to talk here about humans merging “automatically” when the agent approved • without a human reviewer In such case, it seems to me that such tuning, if that’s what is happening, would be self-reinforcing, making it easier and easier to approve PRs, and “weakening” the non-deterministic conditions we put in place in the reviewer agent skill. I'm sure I’m wrong, but could you please tell why I’m wrong?
l
Certainly!
I thought the human merging a PR is a signal for the approving reviewer agent trajectory, that is was doing a good job
It is not necessarily the case that a human merging the PR is a 0-noise signal that the review was good. Obviously, given @curved-angle-80449’s concerns, it is not. So what I am trying to do here is find explicit counter-examples where a PR was merged without human review when it shouldn’t have been. We can then work backwards from those and see why that happened, and come up with some better signals than simply “the PR was merged”
c
I don't think I have a ready concrete example of a case where a human merged their own PR with known bad outcome. I can probably find examples of: a. automated code review that should have caught things it didn;t b. PRs that were merged by the author with no human review and I was surprised for reasons above. c. PRs that were merged and had serious quality problems not discovered until after release My initial question was a policy one primarily intended to help me understand what my next step should be re: (b) and what my own behavior should be.
nod 1
c
Thank you. 🤔 Well, one problem I see with your question is that for people to see those cases, they’d have to review PRs • without being asked for review • after they were merged Funnily enough, there could be such cases - I’ve done that, as noted above; but it doesn’t scale, sorry. And there could be accidental cases, like John Mason happened to be interested in a thing that broke, so looked at stuff retroactively. However, there are many more cases that are not as simple as those. Most importantly, it seems to me, the agent messing up architecture / code design. When that happens, one of my concerns is that the agent will be more likely to do (and accept!) similar stuff again; so it could take time until we figure it out, and it might not necessarily be traceable to a “clear example”. > It is not necessarily the case that a human merging the PR is a 0-noise signal that the review is good. What if we exclude the easy cases? That is, when the PR is low-risk, and the PR author did not ask for human review, then it shouldn’t count. Why would it count?
l
My personal opinion about what the policy should be is: • we treat the agent auto-approving a PR that it shouldn’t as a false positive for the review bot • we treat the agent not auto-approving a PR that gets a simple LGTM from a human reviewer as a false negative for the review bot and then we try to tune the review bot to reduce both false positives and false negatives. And that’s the reason why I suggested it we start collecting examples of the former to try to figure out if we can identify them more systematically in the future. I also would like to think about how we can identify the latter, but that seems relatively easy.
c
From what we see in the other thread, • the LLM used by the cloud reviewer was Minimax • when it approved Vasco’s PR it shouldn’t have, it needed eval It seems Minimax didn’t read the reference skill file. I think maybe it wasn’t intended? I recall GPT-5.5 was also active at some point. But usually Sonnet 4.5 afaik. It seems prone to confounding this skill improvement loop, to have different LLMs that react differently to prompts. Minimax seems to have weaker instruction following 🤔
l
Yeah, great point! Posting the PR here for archiving purposes: https://github.com/OpenHands/software-agent-sdk/pull/3745 I actually just talked to Rohit and he’s working on setting up automations so we can A/B test different models and measure the difference in code review performance between them
c
Totally trivial example, maybe not good… I just happened to notice it today. OpenRouter URL update I reflexively said yes when my agent looked at the OpenRouter URL and wanted to update it from 
docs.all-hands.dev
to `docs.openhands.dev`: • the agent did it • the reviewer agent approved it • the QA agent verified the exact goal of the PR. Sure, why wouldn’t a trivial change work… Before pressing the merge button, I went back to the agent and asked it if this could lose OpenRouter’s history/rankings. The question came in the back of my mind when I was reading the diff. And apparently, yes, it discovered in OpenRouter docs that it probably does - the URL is the app identifier. I thought that is not good! So I changed the PR back, and documented this in a comment, for future agents to see, if any agent is attempting this change again.
Another little example that you may know already, this one seems a bit more risky: fix(agent_server): honor OH_PERSISTENCE_DIR in profile stores • my agent, OpenHands-GPT-5.5, posted a comment review about storing credentials in plaintext, and the PR changes their location • the cloud reviewer agent ran after this comment • but it happily approved ◦ (curious, is it back to Sonnet-4.5 or was it Minimax?) • @strong-ice-84145 noticed and fixed it • the reviewer agent happily approved again, admitting that was a good fix. Which it didn’t notice the first time.
In this case, my concern was that users using local agent-server could have ended up with credentials stored in plaintext in random locations on the machine. We only store them in 
~/.openhands/
, because user home is industry practice. It was intended behavior to only do it there; Tim has implemented encryption for any other location, e.g. in docker workspace, cloud sandboxes, etc.
I think this is a fascinating issue, with many aspects. Re: the second example. Initially, the reviewer agent used to not get automatically in context the top-level comments. I think since it runs in the cloud, it still doesn’t? So it didn't see there was another review. I also wonder if the QA agent does. If they don't see existing concerns, they're probably more limited than maybe they can be. That all said, I do think... that you are right... We are heading towards self-driving codebases, and the agent building itself is just the start. That's incredible, and it's, a bit, well, scary. But we have to... I will come back on this. I don't think Sonnet-4.5 can truly help us get there, though. I admit it doesn't seem to me really useful to optimize prompts for Sonnet-4.5, when it's much more limited than others? It's easy for it to miss things, compared to GPT-5.5. And, it's fairly easy to "convince", even! 😇 IMHO, the simple way to look at it is that the reviewer and QA agents are tools in the human's toolbox. They’re extraordinary tools, and yes, we can use them - and our agents - to do cool stuff. I’m just thinking that responsibility must be with the human.
l
Thanks a lot for the additional examples! This is very helpful
And yeah, it’s great to think about the process as well, we might want to spell it out more explicitly
c
Quick note: my agent tried to analyze 2 
review-this
failed runs. TL;DR: • in the same PR, one got GPT-5.5, the other got Minimax • it reports that the 
AUTOMATION_EVENT_PAYLOAD
was empty, even though the prompt told it it's an envvar • GPT-5.5 used 
invoke_skill
and got the info so started to follow it anyway • Minimax didn’t use 
invoke_skill
, got the json error from payload, and apparently got “lost” doing stuffs In other words, there seems to be a bug or two (I think you made a PR for one, my agent made a PR for another), but of course there is also different behavior for these LLMs when faced with it.
A little third example: approval for a PR to extensions/ repo I could be wrong, and I think the PR is very low-risk, we could do something like this; on the other hand, maybe we can think how to structure this kind of eval it proposes (prompt injection for pr-review, fun!), and, it's very unusual to use LLM providers' clients, without proxy, since they wouldn't work here typically. I can think of an old exception in some V0 benchmark, but typically we don't have this kind of code. If this wasn't 
extensions
repo, I'd think the reviewer was wrong That said, it's 
extensions
repo, and I don't think we told the agent here yet 😅
🙏 1
This seems to me like a better example perhaps, inline with what John Mason noted about the reviewer agent missing things. I admit I like to throw my agent, with a SOTA model, GPT-5.x usually since ~September ish 2025, at somewhat higher risk PRs. Regardless whether the automated reviewer ran or not; because it catches things that the rocket-emoji caffeinated Sonnet 4.5 that was running for a long time in it, was happily missing. 😅 Credentials in workspace .git review
l
Here’s another example of a review that should not have been approved: github.com/OpenHands/software-agent-sdk/pull/4341 feel free to add more to this thread, and give them a thumbs-down when you encounter them.
🙏 1
c
An example: https://openhands-ai.slack.com/archives/C06R25BT5B2/p1787505657140499?thread_ts=1787497906.592339&amp;channel=C06R25BT5B2&amp;message_ts=1787505657.140499 TL;DR: the reviewer agent didn’t read the previous section of AGENTS.md, which already had the information - so it approved redundant, duplicate sections.
Another example, I think: github.com/OpenHands/OpenHands/pull/17344#… • I asked a cloud agent to review • it posted a review pointing out that it would "hold the PR" and mentioned an incorrect removal of MCP stuff • then, the 
all-hands-bot
started review • my agent committed a fix • the bot didn't see any of that, and posted approval recommendation (as a comment, as I think it's told now) In other words, it missed issues, and despite that another review already pointed it out. I think the reviewer should read all other reviews to date. 🤔
Reviews of Ai's mutation testing PRs: • bot-approved, and re-reviewed with Astra • with a prompt asking for a classification of minor/major miss/defensible/correct - <github.com/OpenHands/OpenHands/pull/17334#…: cover draft persistence> - <github.com/OpenHands/OpenHands/pull/17338#…: strengthen LLM profile configuration> - <github.com/OpenHands/OpenHands/pull/17341#…: strengthen shared utility behavior> - <github.com/OpenHands/OpenHands/pull/17344#…: cover MCP config utilities>
🐾 1
b
OpenHands-Astra reviews, posted as @enyst. These PRs had already been reviewed by the review bot; the Astra pass also reviews the bot's review, and in each PR the agent points out what the bot missed. • ✅ Automation #458 — Approved • 🛠️ SDK #5054 — Changes requested: test assertions • 🛠️ OpenHands #17341 — Changes requested: missing test coverage • 🛠️ SDK #4999 — Changes requested: archive/recovery defects
c
To clarify, these are tests of an Automation that has criteria: • the bot fumbled approval/dismiss/comment • authors with write access Requirements: • to review -> act • to score the fumbled bot review • to keep it super-concise 😅 4 more, from the actual OpenHands Automation trial: All four were requested changes: • SDK #5003⁠ — incompatible public adapter types. • SDK #4864⁠ — installation and environment handling. • Automation #438⁠ — validation and state-transition defects. • SDK #4862⁠ — cleanup can leave descendant processes running. In other words, they’re 
request changes
where the default review bot previously 
approved
.
👀 1
l
Hey @calm-artist-7206, thanks for noting all of these, I’ll try to incorporate all of this into improvements to the automation in the next day or two
👀 1
OK, here are some PRs to attempt to make major improvements to our review bot. These include both general improvements to our review automation: • extensions #612 (github.com/OpenHands/extensions/pull/612): shared code-review automation improvements. And improvements to repo-specific review criteria: • OpenHands #17523 (github.com/OpenHands/OpenHands/pull/17523): Canvas review checkpoints plus matching AGENTS.md implementation • software-agent-sdk #5176 (github.com/OpenHands/software-agent-sdk/pull/5176): concise SDK review checkpoints plus matching AGENTS.md guidance for compatibility, propagation, lifecycle, secrets, and packaged runtimes. • automation #495 (github.com/OpenHands/automation/pull/495): lifecycle, concurrency, capacity, authorization, database, and integration checkpoints in both the review guide and AGENTS.md. • extensions #611 (github.com/OpenHands/extensions/pull/611): extension-specific guidance for executable instructions, runtime parity, trigger authorization, generated artifacts, and dependency provenance. They are based on an analysis of all the bot reviews on these 4 repos, as well as the information from this above thread. Hopefully we can get this in and make our bot reviews more solid, predictable, and consistent!
👀 1
m
I was looking over the skills, and they look good to me. The only point I’d like to raise is that we currently have three different files where we gather information: • General code-review skill • In-repo code-review skill • AGENTS.md It seems to me that there is some overlap between them.
👀 1
l
Thanks! I don’t think the reviewer necessarily gets its info from AGENT.md? The idea of having a general code-review skill and in-repo code-review skill is intentional, because there are general good practices about doing code review, and then also things that are specific to a particular repo. I do think we can reduce overlap if the in-repo skill is repeating stuff in the general skill though
👀 1
m
First of all, this is just a minor nitpick, so I don't want to block anything over it. I’ve approved the PRs, and we can always optimize later if needed. I agree with you regarding the general and in-repo code-review skills and their respective responsibilities. Here is how I see it (which overlaps with what you said): • General code-review skill: Code review practices for the organization and/or general best practices • In-repo code-review skill: Specific instructions for this repository • AGENTS.md: Architecture of the repository My comment was simply because 
AGENTS.md
was repeating rules that were already covered to some extent in the other two skills. But as I said: this is minor... Excited to see how it works 🙂
👀 1
I also opened a small PR: docs(review): point typed client access at the SDK because in the OpenHands repo we were still pointing to the 
OpenHands/typescript-client
instead of the new location in the sdk
👀 1
🙏 1
c
I think the reviewer agent, when running on a repo, does get AGENTS.md in context
👍 1
🐾 1
b
🐾 As the resident example: yes — when I run against a repo, AGENTS.md is in my context; OpenHands loads repo instructions by default. The distinction worth keeping is that the review paths in these PRs also explicitly read the branch-local 
.agents/skills/custom-codereview-guide.md
, so a reviewer can get repo instructions from both files — which is exactly why the overlap point mattered. Whether the review automation gets AGENTS.md automatically still depends on how it assembles its context; I can't inspect that from here.
2 Views
Open in Slack
PreviousNext
Powered by