fix: remove "minimal" thinking level (LLM-2595) - #914
Conversation
Kimchi Code Review
Summary📊 Review Score: 80/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Existing tests were updated to replace 📝 Found 3 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
|
📚 No documentation changes were needed for this PR. |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 80/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 1/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Existing tests were updated to replace minimal with low, and a new test was added to verify orchestration instructions no longer mention minimal. Coverage is appropriate for the scope but does not verify runtime rejection of the removed value.
📝 Found 3 issue(s). See inline comments for details.
The "minimal" thinking level causes errors with vLLM/sglang providers. The proxy already maps minimal → low; this stops the harness from offering a broken level. Changes: - Remove "minimal" from ThinkingLevel type union - Remove "minimal" from agent tool schema descriptions - Remove "minimal" from agent wizard thinking-level select - Remove "minimal" from CLI --thinking help text - Update docs/agents.md frontmatter reference - Update invocation-config test to use "low" instead of "minimal" - Add assertion that orchestration instructions don't contain "minimal"
0ff890b to
870a713
Compare
Address kimchi-review bot comments on PR getkimchi#914: 1. Replace Type.String() with Type.Union of Type.Literal for the Agent tool's thinking parameter — runtime enum validation now rejects invalid values. 2. Add coerceThinkingLevel() in custom-agents.ts that maps the removed 'minimal' level to 'low' for backward compatibility with existing agent profile YAML files. Applied in both custom-agents.ts (YAML loading) and invocation-config.ts (runtime param resolution). 3. Strengthen the orchestration-instructions test to also assert that the valid level string 'off, low, medium, high, xhigh' is present in the rendered output, not just that 'minimal' is absent. Co-Authored-By: Kimchi <noreply@kimchi.dev>
|
This PR has been marked as stale because it has not had recent activity. It will be closed tomorrow if no further activity occurs. |
1 participant
Linked issue
Closes #915
What does this PR do?
Removes the
minimalthinking level from the harness. This level causes errors with vLLM/sglang providers (LLM-2595). The proxy already mapsminimaltolow; this stops the harness from offering a broken level.Changes:
minimalfromThinkingLeveltype union (personas/types.ts)minimalfromTHINKING_LEVEL_ORDERarray; changeDELEGATION_THINKING_BASE.explore.simplefromminimaltolow; updaterenderDelegationThinkingLevelTable()Explore row (thinking-level-policy.ts)minimalfrom Agent tool schema description and agent config help text (agents/index.ts)minimalfrom agent wizard thinking-level select menu (agents/index.ts)minimalfromset_phasetool thinking parameter enum (tags.ts)minimalfrom CLI--thinkinghelp text (commands/help.ts)THINKING_LEVELSprose in orchestration instructions (orchestration-instructions.ts)docs/agents.mdfrontmatter referenceinvocation-config.test.tsto uselowinstead ofminimalthinking-level-policy.test.tsassertions and add regression test for absence ofminimalorchestration-instructions.test.tsthat rendered instructions do not containminimalChecklist
pnpm run test)pnpm run check)