https://docs.all-hands.dev/ logo
Join Slack
Powered by
Could I get :eyes: on 2 PRs for managed Codex cred...
# proj-agent
s
Could I get 👀 on 2 PRs for managed Codex credential synchronization? T - OpenHands #15287 — canonical storage, scoped bindings, and safe pause/resume: github.com/OpenHands/OpenHands/pull/15287 - SDK #4198 — callback readiness and mandatory reactivation before cold load: github.com/OpenHands/software-agent-sdk/pull/4198 The SDK PR builds on that merged one: github.com/OpenHands/software-agent-sdk/pull/4124 The OpenHands PR will be merged after sdk release + openhands pinning @limited-student-10747 @calm-artist-7206
👀 1
I tried to keep the scope narrow and code small but rounds of bug fixes keep adding complexity. Added a table in the description of the OpenHands PR explaining why the features introduced are necessary.
l
thanks a lot! I’m going to be out for most of the day, so probably good to have someone else take a look, maybe @modern-fireman-99755 or @calm-artist-7206 if they’re available?
m
I reviewed the sdk PR. Left some comments
c
Looking into the other. Sorry, my agents got messed up since yesterday I activated ACP for Claude Code and changed a bunch of LLMs to LiteLLM, I bumped into multiple bugs 😹
Now this is a nit, just IMHO a nice nit to see flagged in a review. 😅 Funnily enough, it reads kinda okay to me… if we put some one-line comments on each clause, but Claude is not wrong…
Better might be to make a single file dedicated to tiny self-explainable guard methods
(I'd put that in a follow up though, hopefully not forgotten lolsob)
(many are isinstance, so they are actually readable / skippable)
s
It's so annoying. The cycle 
codereview + fix bugs + repeat
simply doesn't converge. I'm on the 5th or 6th iteration. The PR is huge. Every codereview finds new bugs. I tried narrowing the scope and asked to bring a minimal fix. But there are always bugs. I am using 
CC codereview max with opus 5
+ 
codex codereview with sol 5.6 ultra
then I aggregate errors using codex sol 5.6. But yeah it never stops finding bugs 😕 I guess it's my fault to blindly tell sol 5.6 to fix all bugs found in the code reviews. Those bugs should be filtered
👀 1
@calm-artist-7206 I am starting a new round of "Narrow the scope. Think of a way to fix the issue in a smaller narrow scoped openhands PR"
but... I already did this 5 commits ago 😛
c
This sucks, sorry. I think it's because for a complex enough component or problem, we really need to think over the behavior and the code structure/code design. And we may need to keep a firm 👀 on that. Otherwise, I noticed this too, LLMs make a bunch of small wrong choices, and then those add up, and result in chaos. What if you have it make us a page to show us what it proposes or plans to do, the Before and After?
in smolpaws/smolpaws repo, there's a skill called ‘show-me’
that instructs it to tell humans what and why
so we can decide
👍 1
l
Hey @strong-ice-84145 please tell me if you need any help iterating and getting this in, I could take a look!
s
Thanks! I will resubmit a simpler PR for review tomorrow morning!
👍 1
I think it's because for a complex enough component or problem, we really need to think over the behavior and the code structure/code design
what's annoying is that the code structure/design is already fixed (it's in the merged SDK PR), those reviews/fixes cycles are not changing the core design but they are revealing weird edge cases that require changing the code in many files to fix... But perhaps those bugs are edge cases that are barely reachable in prod... I will try to undertand better this failure mode by reading all the comments left on the PR!
nod 1
l
More broadly, I have found that me interacting with a reviewing agent seems to provide superior results. There can be a lot of churn in fully AI-generated reviews and AI reviewers can suggest lots of added complexity. Our linus review skill was actually pretty good at cutting this down, but I’m not sure if we’re using it consistently now, and even then it wasn’t perfect
nod 1
s
I think this a great argument for having a personalized review-bot where inputs/outputs are logged so we can adapt the prompt so failure modes like this are avoided. But yes I am generally very happy with 
CC /code-review max
, except when sometimes the loop diverges into a monster PR 😅
m
But can you not control that by adding something like: the changes should be minimal ? I always add this sentences everywhere and in general you can control the number of lines
s
so that's for the agent that fixes the bug? Yes I added that but it's still "minimal" changes in 83 different files so still hard to review/merge
@calm-artist-7206 here's a simplified solution 🙂 Description github.com/OpenHands/OpenHands/issues/15393 PR github.com/OpenHands/OpenHands/pull/15394
m
Ah ok I see... I was speaking about CC. In general I ask another independent agent with fresh context to review the changes and search for code that we could reuse or for dead-code. You are sure that your number of lines will go down. But I understand that this is painfull 😞
s
In general I ask another independent agent with fresh context to review the changes and search for code that we could reuse or for dead-code
In think that's pretty good. Especially with CC code-review which usually returns lots of minor bugs/improvements. I found that Codex returns less of them
m
Interesting. I'm always comapring CC vs Codex vs Pi and I will say that CC returns a lot but also wrong ones, i.e., are not bugs or improvements while Codex returns less but in general you are sure it is something. Pi is in the middle
s
First rounds of Codex/CC reviews are in. 5 Codex and 15 CC for real "bugs", not improvements 🫠 Let me see if those are fixable without enlarging the scope
m
nice 🙂
c
I think we merged the Linus review into the other codereview, and IMHO we can maybe try to bring it back
s
@calm-artist-7206 if you have time to give a look at github.com/OpenHands/OpenHands/issues/15393 🙂 (and the associated simpler PR...!)
👀 1
@limited-student-10747 so this code is migrated to 
enterprise
which is private? so Engel won't be able to review such code in the future?
l
The OpenHands cloud code? Yes, we’re moving that code to enterprise
s
@calm-artist-7206 well, in that case, no need to review that PR anymore 😅 since it's been migrated to a private repo
acknowledged 1
c
OK! I’m a bit confused (I was reading the issue) if the 
auth.json
switch works everywhere - since we need it to work everywhere; but I’m good if the PR concerns enterprise-only!
Clearly, there’s more headaches with it in the enterprise codebase, sorry ❤️
@strong-ice-84145 @limited-student-10747 just a quick note, I think we need https://github.com/OpenHands/OpenHands/issues/15298 to stay in the open
I was just talking to a contributor interested because they're working on something similar. Of course, this issue also spans the entire OpenHands surface, so it’s best imho to think it through
l
agreed i’ll move it back
gratitude thank you 1
s
@calm-artist-7206 recreated here github.com/OpenHands/software-agent-sdk/issues/4288 couldn't move it (private -> public not allowed) so I recreated it. Openhands got called again 😕 will cleanup
m
the auto-labeler via automations is working 😄 sorry for the noise...
🙌 3
c
@calm-artist-7206 recreated here github.com/OpenHands/software-agent-sdk/issues/4288
couldn't move it (private -> public not allowed) so I recreated it.
Oh. lolsob That means we lost the discussion and clarifications though. Any chance we could convince the agent to just post the replies, e.g. with an intro sentence “Restored from previous discussion.” or if that’s too annoying it’s okay… I'll notify Lucio
👍 1
s
posting a comment that contains all discussion
❤️ 1
thanks on a stick 1
c
Thank you so much! It’s useful context 🎉
2 Views
Open in Slack
PreviousNext
Powered by