feat: add top-level model field, deprecate engine.model#46570
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot remove Go Engine.Model to avoid duplication, apply fix to this repo |
There was a problem hiding this comment.
Pull request overview
Adds canonical top-level model configuration and deprecates engine.model.
Changes:
- Parses and prioritizes top-level
model. - Adds schema deprecation metadata and warnings.
- Adds a
gh aw fixmigration codemod and tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/frontmatter_types.go |
Adds the typed top-level model field. |
pkg/workflow/engine.go |
Applies model precedence and emits deprecation warnings. |
pkg/parser/schemas/main_workflow_schema.json |
Defines top-level model and deprecation metadata. |
pkg/cli/fix_codemods.go |
Registers the codemod. |
pkg/cli/fix_codemods_test.go |
Updates registry expectations. |
pkg/cli/codemod_engine_model.go |
Implements model migration. |
pkg/cli/codemod_engine_model_test.go |
Tests codemod behavior. |
.changeset/engine-model-to-top-level.md |
Documents the feature. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 5
- Review effort level: Medium
| if topLevelMaxRuns == 0 { | ||
| topLevelMaxRuns = parseMaxRunsValue(frontmatter["max-runs"]) | ||
| } | ||
| topLevelModel, _ := frontmatter["model"].(string) |
| Description: "Moves deprecated 'engine.model' to the top-level 'model' field. The top-level field is the canonical location for LLM model configuration and takes precedence over engine.model.", | ||
| IntroducedIn: "0.78.0", | ||
| Apply: func(content string, frontmatter map[string]any) (string, bool, error) { | ||
| return migrateEngineFieldToTopLevel( |
| "description": "Optional specific LLM model to use (e.g., 'claude-3-5-sonnet-20241022', 'gpt-4'). Has sensible defaults and can typically be omitted.", | ||
| "deprecated": true, | ||
| "x-deprecation-message": "'engine.model' is deprecated. Use top-level 'model' instead. Run 'gh aw fix' to automatically migrate." |
| if model, hasModel := engineObj["model"]; hasModel { | ||
| if modelStr, ok := model.(string); ok { | ||
| config.Model = modelStr | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage("'engine.model' is deprecated. Use top-level 'model' instead. Run 'gh aw fix' to automatically migrate.")) |
|
|
||
| - New top-level `model` field for specifying the LLM model used by the agentic engine. Takes precedence over `engine.model` when both are set. | ||
| - Deprecation warning emitted at compile time when `engine.model` is used. | ||
| - `gh aw fix` codemod (`engine-model-to-top-level`) automatically migrates `engine.model` to top-level `model`. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: add top-level model field, deprecate engine.model
Overall the implementation is clean and well-tested. Existing inline comments capture the blocking issues.
Key concerns (already flagged inline):
engine.goline 208 —topLevelModelis not propagated in the import-accumulator path; workflows using imported models would silently get no model.engine.goline 329 — Deprecation warning bypasses the compiler's warning counter, and the migration message overpromises for inline-map engine syntax.codemod_engine_model.goline 15 — Comment-bearingengine.modellines may leave a danglingengine:key after migration..changesetline 9 — Release note doesn't disclose the inline-map limitation.- Schema line 12260 — Existing examples in
engine_configstill show the deprecatedengine.model.
Items 1–3 are correctness/reliability issues worth addressing before merge. Items 4–5 are polish.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 14.9 AIC · ⌖ 5.03 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 81/100 — Excellent
📊 Metrics (7 tests)
Test Breakdown:
Verdict✅ Approved. 14% implementation tests (threshold: 30%). No guideline violations. Strengths:
Test inflation (6:1 ratio) is justified: Codemods are transformation engines requiring comprehensive YAML/frontmatter testing. Production code (30 lines) delegates to shared helper; test suite provides necessary behavioral specification. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on correctness and consistency issues.
📋 Key Themes & Highlights
Key Themes
- Silent model loss on import (
engine.go~line 208):topLevelModelis never read in the import-accumulator path, so imported workflows lose their top-levelmodel:setting. - Out-of-band deprecation warning (
engine.goline 329): usingfmt.Fprintln(os.Stderr)bypasses the compiler's warning pipeline — not counted, not testable. - Misleading changeset (
.changeset/engine-model-to-top-level.md): claimsgh aw fixalways migratesengine.model, but the codemod deliberately no-ops for inline-map syntax. - Comment-trailing model-only engine (
codemod_engine_model.goline 15): migratingengine:\n # comment\n model: xleaves a danglingengine:key with an orphaned comment. - Schema examples still show deprecated form (
main_workflow_schema.json~line 12168): deprecatedengine.modelexamples remain in theengine_configvariants.
Positive Highlights
- ✅ Comprehensive test suite in
codemod_engine_model_test.gocovering no-op, idempotent, migration, and inline-map cases - ✅ Clean precedence logic: top-level
modelconsistently wins overengine.modelacross all engine parse paths - ✅ Schema deprecation annotations added in both
engine_configvariants
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 22 AIC · ⌖ 4.61 AIC · ⊞ 6.7K
Comment /matt to run again
| if topLevelMaxRuns == 0 { | ||
| topLevelMaxRuns = parseMaxRunsValue(frontmatter["max-runs"]) | ||
| } | ||
| topLevelModel, _ := frontmatter["model"].(string) |
There was a problem hiding this comment.
[/tdd] topLevelModel is not propagated through the import-accumulator path (around line 208), so a workflow that sets top-level model: and is imported by another workflow will silently lose its model override.
💡 Suggested fix
After extracting the sub-workflow frontmatter in the import accumulator, read topLevelModel and assign it to config.Model — the same pattern used on line 302 for the object-engine path.
Add a test that imports a workflow with model: gpt-5.4 and asserts the compiled output carries the model.
@copilot please address this.
| if model, hasModel := engineObj["model"]; hasModel { | ||
| if modelStr, ok := model.(string); ok { | ||
| config.Model = modelStr | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage("'engine.model' is deprecated. Use top-level 'model' instead. Run 'gh aw fix' to automatically migrate.")) |
There was a problem hiding this comment.
[/codebase-design] The deprecation warning is emitted via fmt.Fprintln(os.Stderr, ...) while the rest of the compiler surfaces warnings through its structured warning pipeline. This bypasses the warning count and makes the message invisible in tests.
💡 Suggested fix
Use the existing compiler warning mechanism (e.g. c.Warnings = append(c.Warnings, ...) or equivalent) so the deprecation warning is counted, testable, and routed through the same output channel as all other compiler diagnostics.
@copilot please address this.
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (245 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
REQUEST_CHANGES
The feature direction is sound, but several correctness and test-coverage gaps must be addressed before merging.
Dual parsing path for model — FrontmatterConfig.Model is populated by JSON unmarshal in ParseFrontmatterConfig but ExtractEngineConfig re-reads the same field directly from the raw frontmatter map, creating two independent representations. Any normalization logic added to the struct path will silently not apply to the engine path.
No tests for the new primary code path — the topLevelModel-only branch (engine.go lines 607–615) is the canonical new UX (model: without an engine: key) and is entirely untested.
Pre-existing review comments also flag: deprecation warning not counted in compiler warning total; misleading codemod promises in the changeset; import-path model propagation gap; schema examples still showing deprecated form; codemod edge case with model-only engine syntax.
🔎 Code quality review by PR Code Quality Reviewer · 53 AIC · ⌖ 4.67 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/frontmatter_types.go:382
Dual parsing path for model field creates silent divergence risk: FrontmatterConfig.Model is populated correctly by JSON unmarshal, but engine.go ignores it and re-reads from the raw frontmatter map (frontmatter["model"].(string)) independently.
<details>
<summary>💡 Details</summary>
ParseFrontmatterConfig (frontmatter_parsing.go) already unmarshals all top-level fields into FrontmatterConfig, including the new Model string field. But ExtractEngineConfig (engine.go line 3…
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Introduces a canonical top-level
modelfield for LLM model selection, deprecatingengine.modelwhich required nesting a single scalar inside the engine object.Changes
Top-level
modelsupport (engine.go,frontmatter_types.go)model:field parsed from frontmatter; populatesEngineConfig.Modelacross all engine formats (string, object, inline, model-only)engine.modelwhen both are presentDeprecation warning (
engine.go)engine.modelis encountered:'engine.model' is deprecated. Use top-level 'model' instead. Run 'gh aw fix' to automatically migrate.Codemod (
codemod_engine_model.go)engine-model-to-top-levelautomatically migratesengine.model→ top-levelmodelviagh aw fixengine: { id: copilot, model: gpt-4 }); removesengine.modelif top-levelmodelalready presentSchema (
main_workflow_schema.json)modelpropertyengine.modelasdeprecatedwith migration message in both object-formengine_configvariantsExample