Repository navigation
Fix Claude reasoning overrides after #58 - #60
Merged
Merged
Conversation
Comment on lines
+242
to
+262
| it("clears default reasoning whenever an agent override switches agents", () => { | ||
| const agentOnly = resolveConfig({ env: {} }); | ||
| expect(agentOnly.ok).toBe(true); | ||
| if (!agentOnly.ok) return; | ||
| applyDefaultProfileOverrides(agentOnly.value, { agent: "ClaudeAgent" }); | ||
| expect(agentOnly.value.models.profiles.default).toMatchObject({ | ||
| agent: "ClaudeAgent" | ||
| }); | ||
| expect(agentOnly.value.models.profiles.default?.model ?? null).toBeNull(); | ||
| expect(agentOnly.value.models.profiles.default?.reasoning ?? null).toBeNull(); | ||
|
|
||
| const pinned = resolveConfig({ env: {} }); | ||
| expect(pinned.ok).toBe(true); | ||
| if (!pinned.ok) return; | ||
| applyDefaultProfileOverrides(pinned.value, { agent: "ClaudeAgent", model: "claude-sonnet-5" }); | ||
| expect(pinned.value.models.profiles.default).toMatchObject({ | ||
| agent: "ClaudeAgent", | ||
| model: "claude-sonnet-5" | ||
| }); | ||
| expect(pinned.value.models.profiles.default?.reasoning ?? null).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Missing pre-condition assertion weakens the regression test
The test verifies that reasoning is null after the override but never asserts that it was non-null beforehand. If the default profile in ultrafuzz.toml ever loses its reasoning = "xhigh" entry, the test would still pass while silently proving nothing — the clearing logic could be deleted and every expectation would continue to hold. Adding an assertion before the applyDefaultProfileOverrides call would make the pre-condition explicit and keep the test honest as an invariant check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/config/test/config.test.ts
Line: 242-262
Comment:
**Missing pre-condition assertion weakens the regression test**
The test verifies that `reasoning` is `null` after the override but never asserts that it was non-null beforehand. If the default profile in `ultrafuzz.toml` ever loses its `reasoning = "xhigh"` entry, the test would still pass while silently proving nothing — the clearing logic could be deleted and every expectation would continue to hold. Adding an assertion before the `applyDefaultProfileOverrides` call would make the pre-condition explicit and keep the test honest as an invariant check.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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
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.
Follow-up to #58.
Validation:
Greptile Summary
This follow-up to #58 fixes two related gaps in agent-override handling: backend-specific
reasoningis now always cleared whenever--agentswitches to a different agent (including when--modelis also provided), and Claude'sreasoningEffortis now forwarded to the Claude Code CLI via--effortrather than silently dropped.applyDefaultProfileOverridesrestructured sodelete profile.reasoningfires unconditionally on any cross-agent switch, decoupled from theoverrides.model === undefinedguard that controlsdelete profile.model.createClaudeAgentwiresoptions.reasoningEfforttoextraArgs: [\"--effort\", value]onSmithersClaudeCodeAgent; the template is excluded from the packagetsconfig.json(.tsxpattern), soextraArgsvalidity is only caught at generated-project compile time.config.test.tsand updated integration assertion inruntime.test.tscover both the agent-only and agent-plus-model override paths.Confidence Score: 4/5
Safe to merge; the logic change is small and well-scoped, and all stated validation steps (build, lint, tests, CI) reportedly pass.
The reasoning-clear fix in applyDefaultProfileOverrides is straightforward and the test coverage is good, but the new test in config.test.ts omits a pre-condition assertion — it checks that reasoning is null after the override without first confirming it was set, so a future regression that removes the clearing logic would go undetected. The extraArgs forwarding in the Claude template is also untested against real smithers-orchestrator types (templates are excluded from the tsconfig), leaving that integration unverified at the package level.
packages/config/test/config.test.ts — the new test case should assert the pre-condition; packages/runtime/src/templates/smithers/agents/claude.tsx — extraArgs validity is not type-checked in this repo.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["CLI: --agent / --model flags"] --> B{applyDefaultProfileOverrides} B --> C{agent override provided AND differs from profile.agent?} C -- No --> F[Profile unchanged, reasoning preserved] C -- Yes --> D{model override also provided?} D -- No --> E["delete profile.model\ndelete profile.reasoning"] D -- Yes --> G["delete profile.reasoning\nprofile.model = overrides.model"] E --> H[profile.agent = overrides.agent] G --> H F --> I{model override provided?} I -- Yes --> J[profile.model = overrides.model] I -- No --> K[Done] H --> K J --> K K --> L[Task spec: reasoningEffort = profile.reasoning ?? null] L --> M{reasoningEffort null?} M -- Yes --> N[Agent created without effort arg] M -- No --> O["ClaudeAgent: extraArgs: ['--effort', value]\nCodexAgent: config.model_reasoning_effort = value"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A["CLI: --agent / --model flags"] --> B{applyDefaultProfileOverrides} B --> C{agent override provided AND differs from profile.agent?} C -- No --> F[Profile unchanged, reasoning preserved] C -- Yes --> D{model override also provided?} D -- No --> E["delete profile.model\ndelete profile.reasoning"] D -- Yes --> G["delete profile.reasoning\nprofile.model = overrides.model"] E --> H[profile.agent = overrides.agent] G --> H F --> I{model override provided?} I -- Yes --> J[profile.model = overrides.model] I -- No --> K[Done] H --> K J --> K K --> L[Task spec: reasoningEffort = profile.reasoning ?? null] L --> M{reasoningEffort null?} M -- Yes --> N[Agent created without effort arg] M -- No --> O["ClaudeAgent: extraArgs: ['--effort', value]\nCodexAgent: config.model_reasoning_effort = value"]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix Claude reasoning overrides" | Re-trigger Greptile