Set explicit Claude Sonnet fallback when workflow model is unset - #48618
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (11 additions detected). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
Great work! 🚀 This fix handles a critical issue with Claude model fallback behavior. When workflow model is unset, the old code emitted empty fallback expressions, allowing Claude CLI to implicitly select Opus (which was then rejected by the pricing gateway). Your changes now ensure an explicit Sonnet fallback across all three phases (agent, detection, evals), keeping the behavior consistent. What looks good:
This is ready for review!
|
There was a problem hiding this comment.
Pull request overview
Adds an explicit Claude Sonnet fallback when workflows omit a model.
Changes:
- Defaults Claude agent, detection, and evals execution to
claude-sonnet-5. - Aligns expression-model fallback behavior.
- Updates model environment tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/model_env_vars_test.go |
Updates Claude fallback expectations. |
pkg/workflow/evals_steps.go |
Adds the explicit Claude evals fallback. |
pkg/workflow/claude_engine.go |
Applies explicit fallbacks across Claude phases. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/workflow/claude_engine.go:518
- Unset Claude agent runs will now execute
claude-sonnet-5, but the generated run-info step still resolves Claude throughgetDefaultAgentModeltoAgentDefaultModel("agent"; seecompiler_yaml_lookups.go:110). This makes the recorded/audited model disagree with the model actually passed to Claude. Please align the run-info fallback and its test with the new Claude default.
env[constants.EnvVarModelAgentClaude] = compilerenv.BuildModelOverrideExpression(constants.EnvVarModelAgentClaude, compilerenv.DefaultModelClaude, constants.CopilotBYOKDefaultModel)
pkg/workflow/claude_engine.go:531
- This makes Claude's fallback depend on
CopilotBYOKDefaultModel, which is documented as Copilot BYOK-only inpkg/constants/engine_constants.go:333-338. A future Copilot default change would silently change Claude's expression fallback too. Introduce a Claude-specific (or deliberately shared, neutrally named) default constant and use it across the Claude fallback paths.
env[constants.EnvVarModelFallback] = compilerenv.BuildModelOverrideExpression(claudeModelVar, compilerenv.DefaultModelClaude, constants.CopilotBYOKDefaultModel)
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Medium
| return compilerenv.BuildModelOverrideExpression(constants.EnvVarModelEvalsCopilot, compilerenv.DefaultModelCopilot, constants.CopilotBYOKDefaultModel) | ||
| case string(constants.ClaudeEngine): | ||
| return compilerenv.BuildModelOverrideExpressionEmptyFallback(constants.EnvVarModelEvalsClaude, compilerenv.DefaultModelClaude) | ||
| return compilerenv.BuildModelOverrideExpression(constants.EnvVarModelEvalsClaude, compilerenv.DefaultModelClaude, constants.CopilotBYOKDefaultModel) |
| if workflowData.Model == "" { | ||
| if phase == runPhaseEvals { | ||
| env[constants.EnvVarModelEvalsClaude] = compilerenv.BuildModelOverrideExpressionEmptyFallback(constants.EnvVarModelEvalsClaude, compilerenv.DefaultModelClaude) | ||
| env[constants.EnvVarModelEvalsClaude] = compilerenv.BuildModelOverrideExpression(constants.EnvVarModelEvalsClaude, compilerenv.DefaultModelClaude, constants.CopilotBYOKDefaultModel) |
There was a problem hiding this comment.
Changes look correct and consistent. The explicit claude-sonnet-5 fallback is applied uniformly across all three Claude execution phases (agent, detection, evals) and for the expression-model path. Tests are updated to match. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20 AIC · ⌖ 5.18 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — one naming observation, otherwise the fix is clean and well-targeted.
📋 Key Themes & Highlights
Key Themes
- Constant reuse across engines:
CopilotBYOKDefaultModelis now the explicit fallback for both the Copilot and Claude engines, but its name signals Copilot-only intent. This is a minor but real navigation hazard.
Positive Highlights
- ✅ Root cause directly addressed — empty fallback expressions replaced throughout all three Claude execution phases (agent, detection, evals)
- ✅ Consistent change: all four call sites updated symmetrically
- ✅ Tests updated to assert the new non-empty fallback value
- ✅ PR description clearly documents the fallback chain
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 22.9 AIC · ⌖ 4.7 AIC · ⊞ 6.7K
Comment /matt to run again
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (4 modified table-driven rows)
Verdict
|
There was a problem hiding this comment.
Verdict: non-blocking naming concern only
The fallback logic change is correct, targeted, and well-tested (build + targeted unit tests pass). One maintainability nit flagged inline about reusing a Copilot-named constant for the Claude fallback default.
Themes
- Root-cause fix (empty fallback → explicit
claude-sonnet-5default) is sound and consistent across agent/detection/evals/expression paths. - Test updates correctly reflect the new fallback expressions.
- Only concern:
constants.CopilotBYOKDefaultModelis now reused for a semantically distinct purpose (Claude implicit-model bug fix vs. Copilot BYOK requirement), which could cause confusion or accidental coupling in future changes.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 74.2 AIC · ⌖ 4.38 AIC · ⊞ 7.4K
Comment /review to run again
| env[constants.EnvVarModelDetectionClaude] = compilerenv.BuildModelOverrideExpression(constants.EnvVarModelDetectionClaude, compilerenv.DefaultModelClaude, constants.CopilotBYOKDefaultModel) | ||
| } else { | ||
| env[constants.EnvVarModelAgentClaude] = compilerenv.BuildModelOverrideExpressionEmptyFallback(constants.EnvVarModelAgentClaude, compilerenv.DefaultModelClaude) | ||
| env[constants.EnvVarModelAgentClaude] = compilerenv.BuildModelOverrideExpression(constants.EnvVarModelAgentClaude, compilerenv.DefaultModelClaude, constants.CopilotBYOKDefaultModel) |
There was a problem hiding this comment.
Fallback default reuses a Copilot-named constant for the Claude/Detection/Evals path; naming is misleading for future maintainers.
💡 Details
The change correctly fixes the root-cause bug (Claude implicit model resolution defaulting to Opus). However, constants.CopilotBYOKDefaultModel is reused as the generic Claude fallback in three call sites here plus evals_steps.go, despite its name and doc comment ("explicit fallback model for Copilot BYOK mode") describing a Copilot-specific concept. A reader modifying Copilot BYOK behavior later could unknowingly change Claude's fallback too, or vice versa, since they are now coupled via one constant that happens to hold the same string value today.
Suggested fix: introduce a neutral constant, e.g. constants.SonnetDefaultModel = "claude-sonnet-5", and have both CopilotBYOKDefaultModel and the Claude fallback usages reference it (or just use it directly for Claude), so the two use cases remain independently adjustable.
Rationale: coupling two independently-evolving fallback behaviors (Copilot BYOK vs. Claude implicit-model bug fix) through a single misnamed constant is a latent maintenance hazard, even though today's value happens to match.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed a pr-finisher pass and pushed commit |
|
🎉 This pull request is included in a new release. Release: |
Scout failed because unset Claude model selection resolved to
claude-opus-5, which was rejected by the AI credits gateway pricing policy. This change makes the unset-model path resolve to a Sonnet-tier default instead of relying on Claude CLI implicit selection.Root cause addressed
... || ''), allowing runtime implicit model selection (Opus in this run).Model fallback behavior changes
GH_AW_MODEL_AGENT_CLAUDEGH_AW_MODEL_DETECTION_CLAUDEGH_AW_MODEL_EVALS_CLAUDEclaude-sonnet-5Expression-model path alignment
modelvalues, updatedGH_AW_MODEL_FALLBACKfor Claude to include the same explicit Sonnet fallback (instead of empty fallback).Evals parity
Test updates
claude-sonnet-5rather than empty fallback.