https://docs.all-hands.dev/ logo
Join Slack
Powered by
<@U0BME18CF7S> please review <https://github.com/O...
# automations-debug
c
@fresh-machine-37269 please review https://github.com/OpenHands/OpenHands/pull/18191 and approve or request changes or comment as you think appropriate.
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Done — I reviewed PR #18191 and left a comment review rather than an approval. Summary: Why not approve: the 
Validate PR description
check fails on the head (
7657187
) — it declares a functional change but has no screenshot/video in the description (the image embeds came out broken through the outbound proxy). Easy fix: drag two of the 
.pr/
PNGs into 
## Video/Screenshots
, or tick Docs / chore since no product code changed. Content checks out: the counts are internally consistent (736 → 740 sub-features for the four new rows; ledgers add to 31 checks, 29 pass, 2 fail), and both failures are correctly attributed outside Canvas — the image drop is SDK-side and already fixed by software-agent-sdk#5460/#5467 (merged, unreleased), and the onboarding repeat-endpoint profile loss has an open fix in #17889. Nothing new is being masked. Non-blocking: a couple of evidence-path/ID mismatches in the ledger (
.pr/
gets removed on approval anyway), and #17884 is a PR not an issue. I also flagged — without treating it as a defect — the new 
SKILL.md
safety note, since the run shows a real model reading outside its workspace (run dir, 
env
with ephemeral session keys, another session's transcript). Eval risk: none, this is verification recipes and a feature map only. Happy to re-review once the description check is green.
c
@fresh-machine-37269 fixed, please re-review. I think the safety note is a bit exagerated, in openhands agent-sdk this is normal and frankly it’s fine. Feel free to approve if you see no major issues. When done, please take a deep breath and do the same for https://github.com/OpenHands/OpenHands/pull/18174
👀 1
f
✅ Done! Both done. Here's the summary: #18191 — approved ✅ • Re-reviewed at the unchanged head (
7657187
); the diff is byte-identical to what I reviewed before, so no new code to check. • The 
Validate PR description
gate I flagged is now green, and 
test-and-build (ubuntu)
passes. Checklist against the PR's claims all holds (four new rows, 736 → 740 counts, ledger totals 31/29/2, both failures correctly attributed outside Canvas to SDK #5460/#5467 and fix #17889). • On the safety note: fair point, and I deferred to your call — it's expected harness behavior for agent-sdk, so I didn't hold the PR on it. #18174 — commented (code looks good; one evidence gap) • The fix itself is correct and minimal: 
useCloseOnEscape
is wired into the overflow menu exactly like the 
+
tools menu, clears the submenu, and returns focus to the trigger. The new unit test is a real regression test (it drives the real hook through the component, so it fails without the fix). • The only thing I flagged is the evidence: this is a temporal change (a menu that disappears), and the repo's guide requires a video showing trigger → transition → final state, since a still of an empty composer can't prove Escape caused it. #18095 and #18044 both shipped before/after videos for the same class of fix, so I asked for a short 320px recording. A maintainer can waive it, but I couldn't call it fully met. • Gates are otherwise green (description check, ubuntu build, HUMAN section filled), and there's no eval risk.
c
@fresh-machine-37269 ok thank you, fixed 18174, so you can reconsider
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Done — I've re-reviewed PR #18174 and approved it. Quick summary of what I checked at the current head (
3e9c3e4
): • Code: the 
useCloseOnEscape
registration matches the existing 
+
tools-menu pattern; Escape closes the overflow menu and any open Model submenu and returns focus to the trigger. Nothing else in the click-outside/toggle paths changed. • Evidence: the new 320 px before/after recordings are genuine animations (124/139 GIF frames, 259/289 MP4 frames, with the composer region actually changing), on real local stacks with the backend/model named — the previous still-image concern is resolved. • Gates: 
Validate PR description
passes locally against the current body, and 
test-and-build (ubuntu)
finished green (it was still running when I started). • Tests: suite passes 21/21; reverting the hook makes the new Escape test fail, so it's a real regression test. One heads-up: the recordings live under 
.pr/
, which gets removed on approval, so the inline images will disappear from the PR once the cleanup runs — the MP4s are SHA-pinned, so they'll still resolve for a human maintainer.
Open in Slack
PreviousNext
Powered by