Repository navigation
Fix Goose (and other harness engines) crashing when engine.version is unset - #50849
Conversation
… unset Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes #50731 by applying imported engine-definition versions when workflows omit engine.version.
Changes:
- Adds default version metadata to
EngineDefinition. - Applies catalog version defaults while preserving explicit versions.
- Adds regression coverage and recompiles affected workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/engine_definition.go |
Adds engine default version metadata. |
pkg/workflow/compiler_orchestrator_engine.go |
Applies catalog version fallback. |
pkg/workflow/compiler_orchestrator_engine_test.go |
Tests imported version defaults. |
.github/workflows/smoke-opencode.lock.yml |
Pins OpenCode version. |
.github/workflows/smoke-goose.lock.yml |
Pins Goose version. |
.github/workflows/smoke-crush.lock.yml |
Pins Crush version. |
.github/workflows/smoke-aider.lock.yml |
Pins Aider version. |
.github/workflows/daily-token-consumption-report.lock.yml |
Applies Goose version. |
.github/workflows/daily-team-evolution-insights.lock.yml |
Applies Goose version. |
.github/workflows/daily-security-red-team.lock.yml |
Applies OpenCode version. |
.github/workflows/daily-reliability-review.lock.yml |
Applies OpenCode version. |
.github/workflows/daily-code-metrics.lock.yml |
Applies Crush version. |
.github/workflows/daily-arxiv-researcher.lock.yml |
Applies Crush version. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 0
- Review effort level: Balanced
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (66 additions detected). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix is surgical and correct. applyEngineImportDefaults now falls back to the engine catalog version only when engineConfig.Version is empty and an ID is set — preserving any explicitly declared version. The new test validates the exact runtime crash scenario. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.2 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor suggestion.
📋 Key Themes & Highlights
Summary
The fix is precise and correct. The root cause (import-path version being silently dropped when engineConfig was already non-nil) is properly addressed in applyEngineImportDefaults, and the new Version field on EngineDefinition is cleanly placed with a clear doc comment.
Positive Highlights
- ✅ Fix is placed after all import-merge logic, so an explicit
engine.versionin the main workflow still takes precedence — correct ordering. - ✅ Regression test
TestSetupEngineAndImports_ImportedEngineVersionDefaultis clear and mirrors the real production scenario (shared goose.md pattern). - ✅ Lock files for all affected harness engines (Aider, Crush, OpenCode, Goose) are recompiled — good blast-radius awareness.
- ✅ The debug log line is consistent with the existing pattern in
applyEngineImportDefaults.
One Suggestion
- A counterpart test asserting that an explicit
engine.versionin the main workflow is not overridden would close the spec gap and prevent accidental removal of theif engineConfig.Version == ""guard (see inline comment).
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 34.2 AIC · ⊞ 7.1K
Comment /matt to run again
| require.NotNil(t, result) | ||
| require.NotNil(t, result.engineConfig) | ||
| assert.Equal(t, "harness-engine", result.engineSetting) | ||
| assert.Equal(t, "1.45.0", result.engineConfig.Version) |
There was a problem hiding this comment.
[/tdd] Missing counterpart test: explicit version in the main workflow should NOT be overridden by the catalog default.
💡 Suggested test skeleton
func TestSetupEngineAndImports_ExplicitVersionNotOverriddenByDefault(t *testing.T) {
// arrange: main workflow sets engine.version: "2.0.0"
// shared/engine.md also declares version: "1.45.0" for the same engine ID
// act: compile
// assert: result.engineConfig.Version == "2.0.0" // main workflow wins
}Without this test the guard if engineConfig.Version == "" could be accidentally removed and regressions would go undetected.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 3bfbb66 by adding TestSetupEngineAndImports_ExplicitVersionNotOverriddenByDefault, which verifies an explicit workflow engine.version is preserved over the imported default.
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report
📊 Metrics (24 tests)
|
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
REQUEST_CHANGES — targeted fix is reasonable but has a shared-state leakage bug and a YAML quoting inconsistency that should be fixed before merge.
💡 Themes
- Correctness (blocking):
EngineCatalogis a mutable, per-Compilermap that survives across all files in a batchcompilerun. The newGet(engineConfig.ID)lookup inapplyEngineImportDefaultscan silently apply a version registered by an earlier workflow's import to a later, unrelated workflow that never imported that shared definition. - Consistency (should fix): Generated
GH_AW_ENGINE_VERSIONvalues are unquoted in job-levelenv:blocks but quoted (%q) elsewhere. This PR is the first time default version strings flow broadly into these files, making the YAML-float-parsing risk (e.g."1.10"→1.1) newly relevant. - Test coverage (should fix): Only the "version omitted" happy path is tested; no regression test for explicit-version-wins-over-default precedence or for cross-file catalog leakage.
The core mechanism (adding EngineDefinition.Version and applying it as a fallback in applyEngineImportDefaults) is a sound, minimal fix for the reported crash, and the regenerated lock files look consistent with the intent.
🔎 Code quality review by PR Code Quality Reviewer · auto · 138.6 AIC · ⊞ 7.8K
Comment /review to run again
| orchestratorEngineLog.Printf("Applied model preference from import: %s", model) | ||
| } | ||
| if engineConfig.Version == "" && engineConfig.ID != "" { | ||
| if def := c.engineCatalog.Get(engineConfig.ID); def != nil && def.Version != "" { |
There was a problem hiding this comment.
Version defaults can leak across unrelated workflows within a single batch compile run: silently applying a wrong version to a workflow that never declared one.
💡 Details
engineCatalog lives on the Compiler struct and is reused for every markdown file processed in compileSpecificFiles/compileAllFilesInDirectory (a single compiler instance is created once via createAndConfigureCompiler and passed through the whole batch loop). Register(def) (engine_definition.go:343) simply does c.definitions[def.ID] = def — it never clears or scopes to the current file.
Given this new lookup:
if def := c.engineCatalog.Get(engineConfig.ID); def != nil && def.Version != "" {
engineConfig.Version = def.Version
}Once workflow A imports shared/goose.md (which registers goose with version: "1.45.0"), that definition stays in the catalog for the rest of the batch. If workflow B in the same batch also uses engine: { id: goose } but does not import the shared definition (or imports a different shared file that doesn't set a version), workflow B will incorrectly inherit 1.45.0 from workflow A's registration, because Get("goose") returns the last-registered definition regardless of which file is currently compiling.
Fix: either give EngineCatalog a per-file reset/scratch overlay for imported definitions, or make registerNamedEngineDefinitionFromJSON register on a fresh catalog view per compilation unit instead of mutating shared state.
There was a problem hiding this comment.
Addressed in 3bfbb66 by defaulting from the current workflow's imported engine definitions instead of the compiler-wide catalog, plus regression coverage for no cross-workflow leakage with a reused compiler.
| GH_AW_ASSETS_ALLOWED_EXTS: "" | ||
| GH_AW_ASSETS_BRANCH: "" | ||
| GH_AW_ASSETS_MAX_SIZE_KB: 0 | ||
| GH_AW_ENGINE_VERSION: 0.88.0 |
There was a problem hiding this comment.
GH_AW_ENGINE_VERSION is emitted unquoted in some lock-file locations (0.88.0) and quoted in others ("0.88.0"), which is a YAML footgun: unquoted version strings like 1.10 would be parsed as a float and truncate/reformat.
💡 Details
Compare the generated lock diffs:
GH_AW_ENGINE_VERSION: 0.88.0 # unquoted, job-level env (compiler_orchestrator_engine.go path)
...
GH_AW_ENGINE_VERSION: "0.88.0" # quoted, step-level env (compiler_safe_outputs_job.go / safe_outputs_env.go path)safe_outputs_env.go:197 and compiler_safe_outputs_job.go:837 both format the value with %q, but whatever path produces the job-level env: block for the main execution job renders it unquoted. Since this PR is the first time a default (non-user-specified) version string like 1.45.0 or 0.88.0 flows into that env block for engines that previously never had one, this inconsistency is now visible across many workflows. A version such as "1.10" would be silently reinterpreted as 1.1 by strict YAML parsers reading these lock files (e.g. some linters, or manual reconciliation tooling), and 0.10.0 could become 0.1 if ever parsed numerically — a latent correctness bug that this PR's new default-version injection makes far more likely to trigger since defaults are attacker/maintainer-controlled version strings from shared/*.md, not just user frontmatter.
Fix: locate the env template that renders the unquoted GH_AW_ENGINE_VERSION (likely in the job-level env: YAML emission path, distinct from applyEngineVersionEnv) and always quote it, matching the two already-quoted call sites.
There was a problem hiding this comment.
Addressed in 3bfbb66 by quoting job-level GH_AW_ENGINE_VERSION values and regenerating the affected lock files and wasm golden.
| require.NotNil(t, result) | ||
| require.NotNil(t, result.engineConfig) | ||
| assert.Equal(t, "harness-engine", result.engineSetting) | ||
| assert.Equal(t, "1.45.0", result.engineConfig.Version) |
There was a problem hiding this comment.
No test asserts that an explicit engine.version in the main workflow still wins over the shared definition's default — the only new test covers just the "version omitted" happy path.
💡 Details
applyEngineImportDefaults guards the new logic with if engineConfig.Version == "", so precedence should already be correct, but there is no regression test proving it (e.g. main workflow sets engine: { id: goose, version: "1.0.0" } while shared/goose.md declares version: "1.45.0" — expect 1.0.0 to be preserved). Given this PR's stated purpose is fixing a version-defaulting regression, a precedence test guards against a future edit accidentally flipping the condition (e.g. to unconditionally overwrite) and silently breaking users who do pin an explicit version.
Also worth covering: a second file in the same compiler instance/batch that references the same engine ID but without importing the shared definition (see the engineCatalog leakage comment on compiler_orchestrator_engine.go:433) — currently nothing in the test suite exercises multi-workflow compilation with a shared Compiler instance for this code path.
There was a problem hiding this comment.
Addressed in 3bfbb66 with both requested tests: explicit version precedence and no imported-version leakage across workflow compilations using one compiler instance.
|
@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>
Ran the pr-finisher pass and pushed |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot The PR appears updated after prior review. Please make one final pass on any remaining unresolved review feedback, confirm the prior fixes address the dismissed/stale blocking review concerns, refresh the branch if needed, and then run the pr-finisher skill. Run URL: https://github.com/github/gh-aw/actions/runs/31109158981
|
|
🎉 This pull request is included in a new release. Release: |
Workflows setting
engine: { id: goose }without an explicitversion:crashed at runtime withError: GH_AW_ENGINE_VERSION is required, since the shared engine definition's default version (declared in.github/workflows/shared/goose.md) was never merged into the compiled config.Root cause
applyEngineImportDefaultsonly pulled engine config from an imported/shared definition when the main workflow declared noengine:block at all (engineConfig == nil).engine: { id: goose }directly already had a non-nilengineConfig, so the shared definition'sversion: "1.45.0"was silently dropped — leavingGH_AW_ENGINE_VERSIONunset and the Goose harness script throwing before the agent could start.Changes
EngineDefinition(pkg/workflow/engine_definition.go): added aVersionfield so shared/imported engine definitions can declare a default version.applyEngineImportDefaults(pkg/workflow/compiler_orchestrator_engine.go): now falls back to the resolved engine definition's defaultVersion(looked up via the engine catalog) wheneverEngineConfig.Versionis still empty after import processing — mirroring the existing merge pattern forMaxTurns,MCPToolTimeout, etc.TestSetupEngineAndImports_ImportedEngineVersionDefaultverifies a shared engine definition's version is applied when the main workflow only specifiesengine: { id: ... }.now correctly produces
GH_AW_ENGINE_VERSION: "1.45.0"in the compiled lock file instead of leaving it empty.Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.