https://docs.all-hands.dev/ logo
Join Slack
Powered by
<@U0BME18CF7S> please review the agent-sdk PR 5703...
# automations-debug
c
@fresh-machine-37269 please review the agent-sdk PR 5703 and its related docs PR, and feel free to approve, request changes, or comment, as you think appropriate.
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Done — reviewed both and left comments on each: SDK PR #5703 — "fix(llm): keep verified OpenRouter models discoverable" The fix is sound: injecting the curated 
openrouter/<id>
routes independently of `litellm.model_list`/`model_cost` restores discovery without depending on a catalog LiteLLM can drop, and keeping the prefix preserves provider derivation + context-window resolution via the endpoints API. No correctness blockers from me. Three non-blocking notes: 1. Discovery still indirectly relies on 
openrouter
being in LiteLLM's 
provider_list
(that's what 
_extract_model_and_provider
uses); if that name ever goes away, 
?provider=openrouter
silently returns 
[]
again — a guard/test would future-proof it. 2. The new reasoning-effort test derives its expectation from the same 
get_supported_openai_params
the code consumes, so it checks forwarding/normalization rather than pinning behavior. 3. The 
CustomAliasedAction
module-scope move is a legit fix for the subclass-cache leak but orthogonal to the OpenRouter change. One merge gate: the 
Validate PR description
check is red on the head commit — the 
## Issue Number
section needs a 
Fixes #NNNN
-style link to an issue (and that issue must predate the rollout or carry 
ready-for-dev
). Referencing PR #5700 doesn't satisfy it. Once that's green I'm happy to approve. Docs PR #924 — "docs(sdk): explain OpenRouter model discovery" Content is accurate and matches the SDK change. Flagged that it's still a draft (can't merge as-is) and that it's worth holding until #5703 is ready so the docs don't land ahead of the behavior. One tiny optional wording nit about "returned identifiers … can be passed directly to `LLM`". Didn't approve either yet, since the SDK PR has that red required check — say the word and I'll re-approve once it clears.
c
@fresh-machine-37269 it was fixed now, i think. please re-approve if it cleared
👀 1
f
✅ Done! It cleared — just re-approved SDK PR #5703. ✅ The 
Validate PR description
gate is now green on the head commit (they linked issue #5704, which carries the 
ready-for-dev
label), and the rest of the head's check suite is green too — sdk-tests, agent-server, cross, binary builds, pre-commit, etc. My earlier notes were non-blocking, so this is a clean approve. One thing still outstanding: the docs PR #924 is still marked draft, so it can't merge yet — worth un-drafting it now that the SDK side is approved.
c
@fresh-machine-37269 then take a deep breath and review openhands pr 18264 ; feel free to approve it if you don’t see major issues
👀 1
f
✅ Done! Reviewed OpenHands PR #18264 and approved it. ✅ It's a docs-only feature-map maintenance run (29 files: 
.agents/skills/verify-openhands/references/feature-map/*
plus retained 
.pr/
evidence), so runtime risk is essentially nil. I didn't just skim it — I cloned the head and re-ran the map's own gates: • 
map check
→ ok (27 files / 745 IDs, no problems) • 
map coverage
→ ok (31 routes, 29 feature dirs, with the documented context-menu exclusion) • 
map testids
→ ok (1,026 cited IDs, none unresolved) • 
map affected --base … --target …
→ ok (172 paths across all 27 families) I also cross-checked the cited PRs/issues (all 12 merged PRs closed and matching; #18263/#17948/#17959/#18061 open as claimed), confirmed the 
defaults.json
automation pin really is 
1.19.3
(so the F24 key-only fix is consistent), sanity-checked the export readbacks, and scanned the added text for leaked credentials (nothing). CI is green on the head. One non-blocking note I flagged: 
map baseline
prints 
ancestorOfHead: false
because the skill checks out its own branch head — BASE is still an ancestor of the frozen target, so it's expected, not a failure. Worth a sentence in the recipe so it isn't misread. That wraps the three reviews from this thread: SDK #5703 (approved), docs #924 (accurate, but still draft), and OpenHands #18264 (approved).
c
@fresh-machine-37269 i marked the docs pr ready; please approve it if it’s accurate
👀 1
f
✅ Done! Approved docs PR #924. ✅ It's now out of draft, and the single-file change (
sdk/arch/llm.mdx
) is accurate and matches the SDK fix that has since merged: • The description of 
/api/llm/models?provider=openrouter
keeping curated routes discoverable when LiteLLM's catalog omits them, the retained 
openrouter/
prefix, and the endpoints-API context limits all line up with the merged #5703. • The example 
openrouter/deepseek/deepseek-chat
is a real entry in 
VERIFIED_OPENROUTER_MODELS
, and 
LLM
already knows the 
openrouter/
provider, so the prefixed id is directly usable. • CI is green on the head (Mintlify internal links passed; external links skipped). Only a tiny optional wording nit, not blocking: "so they can be passed directly to `LLM`" is a touch loose — the returned value is the model id; provider/key still come from 
LLM
config. That closes out the thread: SDK #5703 merged, docs #924 approved, OpenHands #18264 approved.
c
@fresh-machine-37269 thanks! also review 18274 and 18275 from openhands; post your review with approve if you see no major issues
👀 1
f
🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…
✅ Done! Done! Reviewed and approved both: • #18274 — extends the 
no-restyle
contract to 
#/ui/combobox-caret
, keeping layout/text-color public while reserving appearance and state rotation (
isOpen
instead of 
rotate-*
) to the primitive. Probe coverage is solid. ✅ • #18275 — flips 
shadcn/no-inline-styles
to 
warn
with a single justified exception for 
agent-server-ui-root.tsx
. Staging as warnings with an owner inventory is the right low-risk step. ✅ Both are non-functional lint-policy changes, CI is green, and neither touches product source, CSS, or APIs, so I approved each with an LGTM review. 🎉
Open in Slack
PreviousNext
Powered by