Skip to content

fix: apply config.temperature to LLM client and expose it via opencli config#252

Open
zjshen14 wants to merge 1 commit into
mainfrom
fix/issue-251
Open

fix: apply config.temperature to LLM client and expose it via opencli config#252
zjshen14 wants to merge 1 commit into
mainfrom
fix/issue-251

Conversation

@zjshen14

Copy link
Copy Markdown
Owner

Summary

  • Core fix: createAgent() now uses temperature ?? config.temperature instead of only the explicit CLI flag, so the value in ~/.opencli/config.json is applied to every LLM call when no --temperature flag is given
  • Config command: Adds --temperature <float> to opencli config so users can set the default temperature without manually editing the JSON file

Why this was broken

Config declares temperature: number (default 0.7) and loadConfig() reads it correctly, but createAgent() passed the raw temperature parameter to createClient() — which is undefined when neither startChat nor runSingle received an explicit value. The effect: config.temperature was dead config that had no effect on any LLM call, while config.maxTokens, config.historySize, and config.autoCompact were all applied correctly.

Related: opencli config had no --temperature option, making the field inaccessible via the CLI.

Test plan

  • npm run typecheck && npm run lint && npm run format:check && npm test — all pass (710 tests)
  • opencli config --temperature 0.2 saves the value to ~/.opencli/config.json
  • opencli config (no args) still prints current config
  • opencli run --temperature 0.5 "…" still takes precedence over config value (?? operator)
  • No change to provider implementations, agent core, or tools

Closes #251

🤖 Generated with Claude Code


Generated by Claude Code

… config

config.temperature was loaded from ~/.opencli/config.json but silently
ignored — createAgent() passed the explicit --temperature flag through
(which is undefined when absent) rather than falling back to the config
value. Consistent with how config.maxTokens and config.historySize are
handled.

This also adds --temperature to `opencli config` so the field can be set
from the CLI rather than requiring manual JSON edits.

Closes #251
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@zjshen14 zjshen14 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review: fix: apply config.temperature to LLM client and expose it via opencli config

What's good: The root cause diagnosis is correct and the fix is minimal. temperature ?? config.temperature in createAgent() gives the right precedence — explicit --temperature flag wins, config file is the fallback — matching how maxTokens, historySize, and autoCompact already work. The opencli config --temperature surface wires cleanly to saveConfig(), and the existing "print current config when nothing was provided" guard was correctly extended to cover the new option.

Only src/cli/index.ts changed (8 additions, 2 deletions). All three CI checks pass. No layering concerns — this stays in the CLI layer as expected.


Not merging: This PR is authored by the same account the review bot runs as, so self-merge is not permitted. This is a clear-cut fix that can be merged directly by the maintainer.

Recommendation: Merge.


Generated by Claude Code

@zjshen14

Copy link
Copy Markdown
Owner Author

Review: fix: apply config.temperature to LLM client and expose it via opencli config

What's good: Root cause correctly identified and the fix is precise. temperature ?? config.temperature in createAgent() ensures the persisted config value is used when no CLI flag is passed. The --temperature option is correctly added to the config command, the save path is properly guarded (if (opts.temperature !== undefined)), and the no-op print-current-config condition is correctly extended to cover the new option. The ?? operator is the right choice over || — it passes through an explicit 0 correctly (zero temperature is valid and meaningful), whereas || would silently substitute the config default for a falsy CLI value. CI is all green.


Notes

1. No test for the temperature ?? config.temperature fallback (src/cli/index.ts:342)

The exact bug — config.temperature being silently ignored when no --temperature flag is given — is straightforward to regress if someone refactors createAgent(). A test that stubs loadConfig() returning { temperature: 0.3 } and asserts createClient is called with temperature: 0.3 when the flag is absent would lock this in as a durable artifact. Not a blocker, but worth adding as a follow-up or in this PR before landing.

2. mergeable_state is "unknown" — bot is not auto-merging

GitHub's mergeability check is lazily computed; "unknown" means it hasn't been evaluated yet, not that a conflict exists. CI is green across all three checks. If the branch is clean when you look, this is safe to merge.


Recommendation: Merge. Consider adding a test for the temperature ?? config.temperature path as a follow-up.


Generated by Claude Code

@zjshen14

Copy link
Copy Markdown
Owner Author

Heads up before merging: dependency on PR #230 (o1 temperature fix)

After this PR lands, createAgent() will pass config.temperature (default: 0.7) to every LLM call. That's the intended behaviour — but it creates a regression for o1/o1-mini/o1-preview users.

The current code at openai.ts:77 guards with:

...(this.temperature !== undefined ? { temperature: this.temperature } : {}),

Today, startChat() passes no temperature → undefined → guard fires → o1 models receive no temperature parameter → ✅ OK.

After this PR: temperature ?? config.temperature = 0.7 → guard no longer fires → temperature is sent → o1 API returns 400 Invalid parameter (o1 temperature is fixed at 1 server-side).

PR #230 already has the correct fix: a lacksTemperatureSupport() helper that extends the guard to also skip temperature for o1 models. Both src/providers/openai.ts and src/providers/openai.test.ts are untouched since #230's base commit, so it applies cleanly to current main.

Recommended merge order: #230 first, then this PR. Merging this PR alone will silently break anyone who has model: "o1-mini" (or similar) in ~/.opencli/config.json.


Generated by Claude Code

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.

bug: config.temperature is loaded but never applied to the LLM client

2 participants