fix: make Honcho SDK timeout configurable (closes #25)#28
Open
offendingcommit wants to merge 1 commit into
Open
fix: make Honcho SDK timeout configurable (closes #25)#28offendingcommit wants to merge 1 commit into
offendingcommit wants to merge 1 commit into
Conversation
The SDK client was constructed with `timeout: 8000`, which aborts dialectic chat calls at high/max reasoning levels where server-side processing can take 10-30+ seconds (multi-iteration tool loops, local model backends, rich observation sets). Changes: - Add `sdkTimeout?: number` to both HonchoFileConfig (on-disk) and HonchoCLAUDEConfig (resolved runtime shape) with a doc comment pointing to plastic-labs#25. - New `parseTimeoutEnv()` helper reads `HONCHO_TIMEOUT` env var and coerces it to a positive integer. - `resolveConfig` prefers env var, then file config. - `loadConfigFromEnv` reads env var into the resolved config. - `getHonchoClientOptions` uses `config.sdkTimeout ?? 30000`; exported `DEFAULT_SDK_TIMEOUT_MS` replaces the magic number. Precedence: HONCHO_TIMEOUT env > config.sdkTimeout file field > 30000.
WalkthroughThe changes make the SDK client timeout configurable by adding an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugins/honcho/src/config.ts (2)
240-246: Minor: comment says "positive integer" but code accepts any positive finite number; also no validation forraw.sdkTimeoutfrom the config file.
Number("30.5")→30.5passesNumber.isFinite(n) && n > 0, so the function accepts non-integer values despite the doc comment. Either tighten the check (Number.isInteger(n)) or relax the comment.raw.sdkTimeoutread from~/.honcho/config.jsonon Lines 351/721 is used as-is with no validation, so a malformed file value (e.g.,0,-1, a string) can reach the SDK as the HTTP timeout. Consider running file values through the same sanitizer as the env var.- Silently discarding an invalid
HONCHO_TIMEOUTmakes misconfiguration hard to notice; a one-timeconsole.warnon invalid input would save debugging time.🔧 Suggested refactor
-/** Parse HONCHO_TIMEOUT env var into a positive integer, or undefined. */ -function parseTimeoutEnv(): number | undefined { - const raw = process.env.HONCHO_TIMEOUT; - if (!raw) return undefined; - const n = Number(raw); - return Number.isFinite(n) && n > 0 ? n : undefined; -} +/** Coerce a value to a positive finite number of milliseconds, or undefined. */ +function sanitizeTimeout(value: unknown): number | undefined { + if (value === undefined || value === null || value === "") return undefined; + const n = typeof value === "number" ? value : Number(value); + return Number.isFinite(n) && n > 0 ? n : undefined; +} + +/** Parse HONCHO_TIMEOUT env var, warning on invalid input. */ +function parseTimeoutEnv(): number | undefined { + const raw = process.env.HONCHO_TIMEOUT; + if (!raw) return undefined; + const parsed = sanitizeTimeout(raw); + if (parsed === undefined) { + console.warn(`[honcho] Ignoring invalid HONCHO_TIMEOUT="${raw}"; expected positive number of ms.`); + } + return parsed; +}And at Line 351:
- sdkTimeout: parseTimeoutEnv() ?? raw.sdkTimeout, + sdkTimeout: parseTimeoutEnv() ?? sanitizeTimeout(raw.sdkTimeout),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/src/config.ts` around lines 240 - 246, The docstring for parseTimeoutEnv() is inaccurate and file config values aren't validated: tighten parseTimeoutEnv() to accept only positive integers (use Number.isInteger check) or update the comment to allow non-integer positives; ensure the config loader that reads raw.sdkTimeout runs the same sanitizer as parseTimeoutEnv() (apply the same validation logic to raw.sdkTimeout before passing to the SDK), and emit a one-time console.warn when HONCHO_TIMEOUT (env) or raw.sdkTimeout (config file) is present but invalid so misconfigurations are visible.
712-724: LGTM — default raised to 30s and client option is now configurable.Clean replacement of the hardcoded
8000withconfig.sdkTimeout ?? DEFAULT_SDK_TIMEOUT_MS, and exporting the constant makes it easy for tests/other callers to reference. This directly addresses#25for the common dialecticchattimeout case.One forward-looking thought (out of scope for this PR, per your test plan): the issue also mentions per-tool timeouts so short data tools aren't forced to wait 30s. A follow-up could thread a per-call override through
getHonchoClientOptions()or expose a second constant (e.g.DEFAULT_CHAT_TIMEOUT_MS) for the long-running path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/honcho/src/config.ts` around lines 712 - 724, Change getHonchoClientOptions to accept an optional per-call timeout or add a separate constant for long-running chat timeouts: either add an optional parameter (e.g., timeoutMs?: number) to getHonchoClientOptions and use timeout: timeoutMs ?? config.sdkTimeout ?? DEFAULT_SDK_TIMEOUT_MS, or export a new DEFAULT_CHAT_TIMEOUT_MS and use that where chat-specific clients are created; update callers of getHonchoClientOptions (or the chat client creation site) to pass the chat-specific timeout when needed and keep DEFAULT_SDK_TIMEOUT_MS as the general default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/honcho/src/config.ts`:
- Around line 240-246: The docstring for parseTimeoutEnv() is inaccurate and
file config values aren't validated: tighten parseTimeoutEnv() to accept only
positive integers (use Number.isInteger check) or update the comment to allow
non-integer positives; ensure the config loader that reads raw.sdkTimeout runs
the same sanitizer as parseTimeoutEnv() (apply the same validation logic to
raw.sdkTimeout before passing to the SDK), and emit a one-time console.warn when
HONCHO_TIMEOUT (env) or raw.sdkTimeout (config file) is present but invalid so
misconfigurations are visible.
- Around line 712-724: Change getHonchoClientOptions to accept an optional
per-call timeout or add a separate constant for long-running chat timeouts:
either add an optional parameter (e.g., timeoutMs?: number) to
getHonchoClientOptions and use timeout: timeoutMs ?? config.sdkTimeout ??
DEFAULT_SDK_TIMEOUT_MS, or export a new DEFAULT_CHAT_TIMEOUT_MS and use that
where chat-specific clients are created; update callers of
getHonchoClientOptions (or the chat client creation site) to pass the
chat-specific timeout when needed and keep DEFAULT_SDK_TIMEOUT_MS as the general
default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98ecb57d-895b-4b17-ad04-264d79094dc6
📒 Files selected for processing (1)
plugins/honcho/src/config.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #25.
What this does
Replaces the hardcoded
timeout: 8000ingetHonchoClientOptions()with a configurable value.Precedence:
HONCHO_TIMEOUTenv var →config.sdkTimeout(on-disk) →DEFAULT_SDK_TIMEOUT_MS(30000).Why
As documented in #25, 8s aborts dialectic chat at
high/maxreasoning levels even with healthy backends, because multi-iteration tool loops on local models (LM Studio/Ollama) or rich observation sets regularly take 10-30s. 30s matches the issue's primary suggestion and leaves headroom without matching the SDK's 60s default (which felt too generous as a new default). Users who want 60s can setHONCHO_TIMEOUT=60000or add"sdkTimeout": 60000to their config.Changes
HonchoFileConfig(on-disk shape): addedsdkTimeout?: numberso the value survivessaveConfig().HonchoCLAUDEConfig(resolved runtime shape): addedsdkTimeout?: numberwith doc comment citing Hardcoded 8s SDK timeout breaks dialectic queries at high/max reasoning levels #25.parseTimeoutEnv()helper: readsHONCHO_TIMEOUT, coerces to a positive finite integer, returnsundefinedon invalid input so the next precedence level wins.resolveConfig(): env var first, thenraw.sdkTimeoutfrom the file.loadConfigFromEnv(): env var only.getHonchoClientOptions():config.sdkTimeout ?? DEFAULT_SDK_TIMEOUT_MS.DEFAULT_SDK_TIMEOUT_MS = 30000replaces the magic number.Test plan
bunx tsc --noEmitcleanHONCHO_TIMEOUT=45000→ 45000 (parsed as positive int)HONCHO_TIMEOUT=abc→ falls through to file/defaultsdkTimeout: 20000with no env var → 20000Not included
user-prompt.ts— that's a separate UX fallback (serves stale context on slowpeer.context()calls) and doesn't hard-fail, so it's out of scope for Hardcoded 8s SDK timeout breaks dialectic queries at high/max reasoning levels #25.Summary by CodeRabbit
New Features
HONCHO_TIMEOUTenvironment variablesdkTimeoutconfiguration setting for granular timeout control