https://docs.all-hands.dev/ logo
Join Slack
Powered by
PR to delete deprecated code (for the 1.41.0 sdk r...
# proj-agent
m
PR to delete deprecated code (for the 1.41.0 sdk release) github.com/OpenHands/software-agent-sdk/pull/4394 @calm-artist-7206 the OpenAPI schema check was failing (as I'm deleting API endpoints). I fixed it. But I'm not sure it is the right way to fix it.
👀 1
❤️ 1
c
Love it!
Killing code is cool
😅
Left a question on 
ACPConversationInfo
, as it seems that maybe we can kill some more
m
Killing code is cool
these are the best PRs 🙂
should I cherry pick the commit in main and push into release right?
c
yes
🙏 1
m
@fresh-machine-37269 Read the comment of Engel here: github.com/OpenHands/software-agent-sdk/pull/4394#… and search for an answer
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Here's Engel's comment and the answer: Engel's comment (on PR #4394):
LGTM! Love to see it!
Nit: is 
ACPConversationInfo
still in use? If not, could we remove that too? I see this code removes usage of it.
Answer: Yes, 
ACPConversationInfo
(and its sibling 
ACPConversationPage
) can be fully removed — they're no longer in use anywhere in the codebase. The PR already removed the actual definitions from 
openhands-agent-server/openhands/agent_server/models.py
(the deprecated compatibility aliases that pointed to `ConversationInfo`/`ConversationPage`), and the only remaining reference is a negative assertion in the OpenAPI discriminator test:
Copy code
# tests/agent_server/test_openapi_discriminator.py:196
assert "ACPConversationInfo" not in schemas
That line isn't a usage — it's a guard confirming the alias no longer leaks into the generated schema. The old deprecated 
__getattr__
shim (which warned "deprecated as of 1.36.0, removed in 1.41.0") was deleted by this PR, along with the test 
test_acp_conversation_response_names_are_type_aliases
that exercised it. So there's nothing left to remove — the PR already did the full cleanup Engel suggested. The code is clean. ✅
m
@calm-artist-7206 Could you give another look at github.com/OpenHands/software-agent-sdk/pull/4393 Did I something wrong? I was following the instruction
c
Do we know why did the critic example fail?
m
timeout... it is a flaky test
👍 1
it happens every second release
c
Okies…
You're right, we can merge @fresh-machine-37269 look at this comment https://github.com/OpenHands/software-agent-sdk/pull/4393#issuecomment-5204318914 It is created by a workflow, find it and understand it. Then adjust it so that: • where it says “blocking findings” it should say just findings • where it shows the commit that became merged head: link the DIFF between the last reviewed commit hash and the merged head, if that makes sense • where, at the bottom, it lists updated deps: exclude from count and display the openhands-* packages
👀 1
@fresh-machine-37269 do NOT modify the PR, make another from main
👀 1
Oops, @modern-fireman-99755 is it going to receive the second message async now, before the end of the agent turn?
I'm sorry 🙏 I forgot to tell it initially that this is a side quest, it should branch from main, nothing to do with the release PR itself
f
⏳ Still working — this is taking longer than this run allows. Follow along at the link above; mention me again for an update.
c
@fresh-machine-37269 give me an update and please confirm that you will make a branch from main, not on the PR = the release PR was merged!
👀 1
(Why is it taking longer than the run allows?)
f
✅ Done! Yes, confirmed — I made a new branch from 
main
 (commit 
199618a
, the merged PR #4394), not on the release PR. The PR is up: PR #4397: github.com/OpenHands/software-agent-sdk/pull/4397 Here's what I changed in the security-scan workflow (3 files): 1. "blocking findings" → "findings" — 
Report.render()
in 
security_scan_common.py
now shows 
❌ N finding(s)
instead of 
❌ N blocking finding(s)
. 2. Linked the diff — in 
check_approval_drift.py
, the merged-head SHA in approval-drift rows is now a markdown link to the GitHub compare page between the last reviewed commit and the merged head, e.g.: merged head was ae46a1d53 3. *Excluded 
openhands-*
from count, still displayed* — in 
check_dependency_diff.py
, internal 
openhands-*
bumps are now: ◦ Excluded from the 
bumped: N
count (only external deps counted) ◦ Noted in a separate line: 
_(plus N internal openhands-* bump(s))_
◦ Listed in their own collapsible section at the bottom ◦ Still scanned by OSV and the non-PyPI source check (security coverage unchanged)
m
There are a couple of conversations going on in the VM. Because of the PR review bot 🙂
aha 1
c
@fresh-machine-37269 could you update your branch from main, then see if CI passes or fails?
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
c
Are we blocking conversations when a conversation runs?
m
not really
I admit max of 3 Pr review per time (there is a queue)
but the other are free to start
GLM do a lot of steps actually
👍 1
c
yes, that 3 makes sense - per automation run, right? i had a max too, of 5 iirc
m
yes
but for slack bot I don't have this bound. As we don't have so many messages at once
nod 1
c
it sounds like there’s some resource usage slowing it down, or maybe there is some non-obvious place where it waits for some reason
or not… maybe it’s just steps
m
possible.
f
✅ Done! All done. CI has fully passed. Here's the final report: Branch updated from main ✅ — merged the latest 
origin/main
into 
fix/postinstall-message-show-run-command
(clean merge, only 
package.json
auto-merged) and pushed to PR #16338. CI: all green ✅ — 15 passed, 2 skipped, 0 failing. Notably 
mock-llm-docker-e2e
(the last one running) finished successfully. No flaky failures this time.
c
@fresh-machine-37269 what does the fix/postinstall-message-show-run-command branch do? Link the PR
👀 1
f
✅ Done! The 
fix/postinstall-message-show-run-command
branch adds a postinstall message that prints guidance after a global install (
npm install -g @openhands/agent-canvas
) telling users to run 
agent-canvas
to start the canvas — including the default URL, useful CLI options, and a docs link. Key details: • It uses an inline 
node -e
command (not an external script file) so it works safely in Docker builds where only 
package.json
is present during 
npm ci
. • The command checks 
npm_config_global
, so it only prints for global installs — library, local, and Docker builds stay silent. • Includes ANSI color codes for readability. PR: github.com/OpenHands/OpenHands/pull/16338
c
(the branch with the security scan fix is named security-scan-comment-improvements)
Vasco, was that started by you?
@fresh-machine-37269 repeat to me verbatim the initial user message in your context, the Slack that made you add a postinstall message
👀 1
f
✅ Done! I don't have that message in my context. The conversation history I was given starts with the "update your branch from main" request — the earlier Slack message that originally asked me to add the postinstall message isn't included in my context. I can't repeat it verbatim because I simply don't have it. If you can find and paste it, I'm happy to confirm whether it matches what I acted on.
c
@modern-fireman-99755 this looks problematic: I think two conversations got messed up or the Slack ID of the thread got messed with another. This branch, and the request it responded to, was not in this Slack thread…
m
Link to the other msg?
c
^^ and that branch is 
security-scan-comment-improvements
this is what I expected it will continue on - it’s a different PR, and a different branch name (and repo! it was an SDK request)
=> • linked to this thread id, we should have: ◦ agent-sdk repo, PR 4397, branch name with security-scan in the name • instead: we have for some reason also: ◦ oh repo, PR 16338, branch name fix/postinstall…etc
m
it wasn't misrouted, it just forgot everything and restarted cold, mid-task.
Copy code
Root cause in main.py: THREAD_WATCH_SECONDS = 900 (15 min) is measured from when the previous run finishes posting, not from the user's last message, and it's only refreshed once at the very end of a run — never while a run is still blocking/working. Since a single run can legitimately take up to RUN_BUDGET_SECONDS = 1740 (29 min), a normal human follow-up cadence can easily outrun the 15-minute window, especially right after a slow run.
thinky spin 1
c
This rings familiar, at a higher level: automation repo needs to have, and has, a message queue implementation. I had my agent analyze it, and I think its conclusions are basically correct: • the ‘inbox’ part of a message queue is great, well designed • but the ‘outbox’ part is very basic, and doesn’t quite give the guarantees it should
I'll come back on this, had my agent try to find some alternative we were working on… Anyway, just so you know, tomorrow I won’t be here, back on Monday 🙏
👍 1
m
have a nice week-end 😄
🎉 1
Open in Slack
PreviousNext
Powered by