https://docs.all-hands.dev/ logo
Join Slack
Powered by
<@U0BME18CF7S> please review 582 on openhands/auto...
# automations-debug
c
@fresh-machine-37269 please review 582 on openhands/automation and post your review as a gh review with event approve or request changes or comment as you think fit
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Done — review posted on OpenHands/automation#582 as an approve. I dug into the diff and ran the checks locally: the 57 skill tests pass, `ruff format`/`ruff check`/`pyright` are clean, and 
map check --no-routes
is ok on the committed map. The core change is sound — a known bug now only reads as 
xfail
when the marked command fails and its 
# bug: PREDICATE
matches, so an unrelated 401/404/503 can no longer stand in for a reproduction. I also confirmed the sibling ports (software-agent-sdk#5728, OpenHands#18348) are merged. Three non-blocking nits in the review body: 1. 
assert --json FILE
overloads a flag that means "JSON body string" on `api`/`exec`, where the file form is 
--json-file
. 2. 
NEGATIVE_BUG_RE
is searched over the whole command line, so a quoted JSON body or 
--check
value containing 
; if
, 
&& !
, 
test !
or 
[ !
trips the "negative bug line" rejection. 3. 
# bug: exit eq 1
parses as a payload path (the payload really has an 
exit
key) rather than the documented 
exit N
form. None of these block the merge — approval stands.
c
can you @fresh-machine-37269 also review 18463 from openhands, and post your approve if you don’t see issues
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Done — I reviewed OpenHands/OpenHands #18463 and posted an APPROVE. It's a clean, low-risk fix: the two error texts on the automation setup panel (
automation-setup-draft-validity
and 
automation-setup-draft-runs-error
) swap raw 
text-red-300
for the theme token 
text-feedback-error
, which resolves to 
#C0282D
on Light+ and 
#BB2A26
on Solarized Light via 
--oh-feedback-error
, so the pill goes from 1.49:1 to ~4.6:1 while dark themes keep their danger color. The earlier 
Validate PR description
blocker is cleared on head 
ad6db62
(the linked issue now carries 
ready-for-dev
), all other checks pass, and the SHA-pinned before/after Light+ captures cover the functional-evidence requirement. Mergeable state is now clean, with the approval in place.
c
same for 18464 please , post your review @fresh-machine-37269
👀 1
f
✅ Done! Reviewed OpenHands/OpenHands #18464 and posted an APPROVE too. 🎉 It fixes a real bug: switching Frequency from a preset to Custom was dropping the configured schedule and showing the constant 
0 9 * * *
. The fix seeds 
customSchedule
from the preset it replaces via the same 
toCron(...)
used on save, with the guard 
value === "custom" && frequency !== "custom"
so re-selecting Custom still preserves a typed cron, and loaded (editing) automations aren't clobbered. I verified beyond the diff: on head 
a6f324d3
, 
npm run make-i18n && npx vitest run __tests__/components/automations/automation-setup-panel.test.tsx
passes 49/49, and removing only the new seeding block makes the added test fail (so the regression test is real, not vacuous). All acceptance criteria are met and the SHA-pinned before/after captures are present. Both earlier blockers are now clear — 
Validate PR description
(issue #18461 gained 
ready-for-dev
) and `test-and-build (ubuntu)`/`(windows)` are all green — so both #18463 and #18464 are approved, and their mergeable state is now clean. Note: per the repo's review rules I can only submit APPROVE or COMMENT, not REQUEST_CHANGES — if a human maintainer wants to block on anything, that call is theirs.
Open in Slack
PreviousNext
Powered by