Skip to content

fix: llm:choose reliability + surface silently swallowed errors - #34

Merged
JNK234 merged 2 commits into
mainfrom
llm-choose-bug
Mar 14, 2026
Merged

fix: llm:choose reliability + surface silently swallowed errors#34
JNK234 merged 2 commits into
mainfrom
llm-choose-bug

Conversation

@JNK234

@JNK234 JNK234 commented Mar 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes #21, closes #24

  • llm:choose reliability: Exact matching, clean history management, and thinking model support for the llm:choose primitive
  • Surface silently swallowed errors: Replace silent error swallowing with warnings (System.err.println) and cause-chained exceptions across 6 files (18 locations) — users can now debug response parsing failures, connection issues, and invalid config values that were previously hidden

Error handling fixes by file:

  • GeminiProvider: Guard empty candidates with promptFeedback context, warn on unparseable parts/finishReason, chain exception causes
  • ClaudeProvider: Warn on malformed thinking/text blocks, chain exception causes
  • OllamaProvider: Warn on thinking field parse failures, log connection and model list errors, chain causes
  • OpenAIProvider: Chain exception cause in parse failures
  • LLMExtension: Warn on invalid timeout_seconds, log Ollama reachability failures, require template field in YAML templates
  • ReasoningModelDetector: Warn on invalid thinking_budget_tokens

Test plan

  • sbt compile passes
  • All 28 deterministic tests pass (sbt test)
  • Ollama integration tests pass locally (OLLAMA_TESTS=true sbt "testOnly org.nlogo.extensions.llm.OllamaIntegrationTests")
  • No case _: Exception => without warning remaining in modified code
  • All RuntimeException constructors chain the cause e
  • No new getOrElse("") without accompanying warning

JNK234 added 2 commits March 13, 2026 18:22
… model support

- Replace bidirectional contains matching with case-insensitive exact match
- Throw clear error on no match instead of silent random fallback
- Use chatWithFullResponse() with thinking field fallback for thinking models
- Store only original prompt in history, not internal constraint text
- Don't mutate history until API call succeeds (ChatReporter, ChatAsyncReporter, ChooseReporter)
- Centralize timeout lookup via getTimeoutSeconds helper
- Fix hardcoded 30s timeout in ChatReporter and ChatWithTemplateReporter
- Add 8 deterministic tests and 2 Ollama integration tests for choose behavior
- Remove unused Random import
Replace silent error swallowing with warnings (System.err.println) and
cause-chained exceptions across 6 files (18 locations). Users can now
debug response parsing failures, connection issues, and invalid config
values that were previously hidden.

- GeminiProvider: guard empty candidates with promptFeedback context,
  warn on unparseable parts/finishReason, chain exception causes
- ClaudeProvider: warn on malformed thinking/text blocks, chain causes
- OllamaProvider: warn on thinking field parse failures, log connection
  and model list errors, chain causes
- OpenAIProvider: chain exception cause in parse failures
- LLMExtension: warn on invalid timeout_seconds, log Ollama reachability
  failures, require template field in YAML templates
- ReasoningModelDetector: warn on invalid thinking_budget_tokens

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f95fbf7c71

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// Add response to history when completed

// Send with user message included, but don't mutate history yet
val responseFuture = provider.chat(history.toSeq :+ userMessage).map { responseMessage =>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve pending user turns when dispatching async chats

llm:chat-async now builds each request from history.toSeq :+ userMessage but defers writing userMessage into history until the future succeeds, so if an agent launches multiple async chats before awaiting results, later requests are sent without earlier pending turns in context. This can change model behavior and produce inconsistent conversation state for concurrent async workflows that rely on per-agent history continuity.

Useful? React with 👍 / 👎.

@JNK234
JNK234 merged commit 3d1283f into main Mar 14, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: llm:choose has silent random fallback and fragile parsing llm:choose does not reliably constrain LLM output to provided choices

1 participant