Skip to content

Commit 0653f44

Browse files
Simon Rosenbergclaude
andcommitted
chore: make dry-run LLM load total; drop -O-stripped assert (#3717)
Address PR review: - dry-run LLM load now catches any store exception (filelock.TimeoutError from lock contention, OSError, validation errors) and records it as a diagnostic ("Could not load ...", distinct from "not found") instead of propagating and crashing the editor preview (#3719). Adds a regression test with a load that raises TimeoutError. - replace `assert llm is not None` (stripped under python -O) with an explicit guard so a None LLM can never reach _build_openhands_settings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent dd58f47 commit 0653f44

2 files changed

Lines changed: 39 additions & 2 deletions

File tree

openhands-sdk/openhands/sdk/profiles/resolver.py

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -331,10 +331,19 @@ def resolve_agent_profile_dry_run(
331331
llm = llm_store.load(profile.llm_profile_ref, cipher=cipher)
332332
diagnostics.llm_profile_resolved = True
333333
diagnostics.llm_api_key_set = _api_key_set(llm)
334-
except (FileNotFoundError, ValueError):
334+
except FileNotFoundError:
335335
diagnostics.errors.append(
336336
f"LLM profile {profile.llm_profile_ref!r} not found"
337337
)
338+
except Exception as e:
339+
# Keep the dry-run total: the store can raise filelock.TimeoutError
340+
# (lock contention), OSError, or a validation error before its own
341+
# handler runs. Surface those as a diagnostic instead of crashing
342+
# the editor preview (#3719) — distinct from a definitively-missing
343+
# profile above.
344+
diagnostics.errors.append(
345+
f"Could not load LLM profile {profile.llm_profile_ref!r}: {e}"
346+
)
338347
else:
339348
(
340349
diagnostics.acp_api_key_secret_name,
@@ -350,7 +359,12 @@ def resolve_agent_profile_dry_run(
350359
# diagnostics rather than raising, matching the API contract.
351360
try:
352361
if isinstance(profile, OpenHandsAgentProfile):
353-
assert llm is not None
362+
# valid here implies the LLM load above succeeded; gate
363+
# explicitly rather than via assert (stripped under python -O).
364+
if llm is None:
365+
raise RuntimeError(
366+
"OpenHands profile marked valid without a resolved LLM"
367+
)
354368
settings = _build_openhands_settings(profile, llm, filtered_mcp, cipher)
355369
else:
356370
settings = _build_acp_settings(profile, filtered_mcp)

tests/sdk/profiles/test_resolver.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,29 @@ def test_dry_run_reports_dangling_llm_and_mcp(
345345
assert diag.resolved_settings is None
346346

347347

348+
def test_dry_run_total_on_llm_store_transient_error(
349+
llm_store: LLMProfileStore, mcp_config: MCPConfig
350+
) -> None:
351+
# The store can raise filelock.TimeoutError (lock contention) before its
352+
# own handler runs; the dry-run must surface that as a diagnostic, not
353+
# crash the editor preview (#3719).
354+
def _boom(*_args: object, **_kwargs: object) -> LLM:
355+
raise TimeoutError("profile store lock acquisition timed out")
356+
357+
llm_store.load = _boom # type: ignore[method-assign]
358+
profile = OpenHandsAgentProfile(
359+
name="oh", llm_profile_ref="default", mcp_server_refs=["fetch"]
360+
)
361+
diag = resolve_agent_profile_dry_run(
362+
profile, llm_store=llm_store, mcp_config=mcp_config, cipher=None
363+
)
364+
assert diag.valid is False
365+
assert diag.llm_profile_resolved is False
366+
# Reported as "could not load" (transient), distinct from "not found".
367+
assert any("Could not load LLM profile" in e for e in diag.errors)
368+
assert diag.resolved_settings is None
369+
370+
348371
def test_dry_run_verdict_matches_real_resolve(
349372
llm_store: LLMProfileStore, mcp_config: MCPConfig
350373
) -> None:

0 commit comments

Comments
 (0)