feat: tip for known external engines (opencode, crush) missing their import - #51088
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (87 additions detected). |
|
|
|
|
|
|
|
@copilot ensure all engines in this repository are in the list. |
There was a problem hiding this comment.
Pull request overview
Adds actionable import guidance when resolving known external engines that lack their shared definitions.
Changes:
- Maps external engine IDs to shared definition imports.
- Appends import instructions to invalid-engine errors.
- Tests known and unknown engine error behavior.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/engine_definition.go |
Adds known-engine import tips. |
pkg/workflow/engine_definition_test.go |
Tests tip generation and omission. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (2)
pkg/workflow/engine_definition.go:446
- Lowercasing only the tip lookup makes the guidance ineffective for inputs such as
OpenCode: catalog registration and prefix resolution above remain case-sensitive, so importing the suggested lower-case definition still leavesOpenCodeinvalid and repeats this same tip. Match the known ID exactly here, or explicitly instruct the user to change the engine ID to its lower-case form.
if importPath, ok := knownEngineImports[strings.ToLower(id)]; ok {
pkg/workflow/engine_definition.go:350
- Kiro is also a non-built-in engine with a shared definition (
.github/workflows/shared/kiro.md, documented alongside these samples indocs/src/content/docs/reference/engines.md:27-35), but it is omitted here. As a result,engine: kirostill receives the generic error while the other shared engines receive the new guidance. Add it to this catalog and to the table-driven tip test.
"goose": "github/gh-aw/.github/workflows/shared/goose.md@main",
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| // To add a new well-known engine, append an entry here with the engine ID as the | ||
| // key and the fully-qualified path to its shared engine definition file as the value. | ||
| var knownEngineImports = map[string]string{ | ||
| "opencode": "github/gh-aw/.github/workflows/shared/opencode.md@main", |
There was a problem hiding this comment.
The implementation is clean and correct. knownEngineImports map is well-documented, the strings.ToLower lookup handles case-insensitive engine IDs properly, the tip is appended only after the existing error (not replacing it), and all 5 engines have corresponding tests. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.2 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two minor observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
@mainhardcoding — all five import paths pin to@main. If users are on a tagged release, the tip could direct them to a mismatched file. A shared constant for the ref would future-proof this.- Missing case-insensitive test — the production code normalises the engine ID with
strings.ToLower, but no test exercises mixed-case input (e.g."OpenCode").
Positive Highlights
- ✅ Clean, additive design: new engines need only one line in
knownEngineImports - ✅ Solid test coverage: five known-engine cases + a negative case for truly unknown engines
- ✅ Error message is actionable — shows the exact
imports:block the user needs to add - ✅ Good comment on the exported map documenting the expected key/value format
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 21.8 AIC · ⊞ 7.1K
Comment /matt to run again
| // To add a new well-known engine, append an entry here with the engine ID as the | ||
| // key and the fully-qualified path to its shared engine definition file as the value. | ||
| var knownEngineImports = map[string]string{ | ||
| "opencode": "github/gh-aw/.github/workflows/shared/opencode.md@main", |
There was a problem hiding this comment.
[/diagnosing-bugs] Import paths are hardcoded to @main, so users on a pinned release may follow a tip that points at a schema mismatched to their binary version.
💡 Suggestion
Consider exposing the ref as a constant (e.g. derived from the binary version) so all five entries can be updated in one place:
const knownEngineImportRef = "main" // or Version
var knownEngineImports = map[string]string{
"opencode": fmt.Sprintf("github/gh-aw/.github/workflows/shared/opencode.md@%s", knownEngineImportRef),
...
}@copilot please address this.
| registry := NewEngineRegistry() | ||
| catalog := NewEngineCatalog(registry) | ||
|
|
||
| _, err := catalog.Resolve("totally-unknown-engine", nil) |
There was a problem hiding this comment.
[/tdd] No test covers case-insensitive lookup. The production code calls strings.ToLower(id), but there is no test that passes e.g. "OpenCode" or "CRUSH" and asserts the tip still appears.
💡 Suggested test case to add to TestEngineCatalog_Resolve_KnownImportTip
{
name: "opencode case-insensitive",
engineID: "OpenCode",
wantImportPath: "github/gh-aw/.github/workflows/shared/opencode.md@main",
},Without this, a future change to the strings.ToLower call would break the feature silently.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (8 tests)
Verdict
|
There was a problem hiding this comment.
Verdict: Approve with non-blocking suggestions
Small, well-tested addition (import-tip map + Resolve error enrichment). No correctness or security bugs found in the changed lines; flagged concerns are maintainability hardening, not blockers.
Themes
@main-pinned import suggestions in the tip message are a drift/reproducibility risk (low severity today since nothing auto-fetches them).knownEngineImportsis a second source of truth versus the actual files in.github/workflows/shared/*.md; nothing currently validates the two stay in sync.- No collision guard between
knownEngineImportskeys and future built-in engine IDs. - Test coverage for the five new entries and the negative (unknown-engine, no-tip) case is solid.
🔎 Code quality review by PR Code Quality Reviewer · auto · 40.7 AIC · ⊞ 7.8K
Comment /review to run again
| "crush": "github/gh-aw/.github/workflows/shared/crush.md@main", | ||
| "cursor": "github/gh-aw/.github/workflows/shared/cursor.md@main", | ||
| "aider": "github/gh-aw/.github/workflows/shared/aider.md@main", | ||
| "goose": "github/gh-aw/.github/workflows/shared/goose.md@main", |
There was a problem hiding this comment.
Import tip pins engines to the mutable @main branch rather than a tagged release or SHA.
💡 Why this matters
All five suggested import paths (opencode, crush, cursor, aider, goose) use @main. Since main is a moving target, the shared definition file content a user imports today can differ from what they get next week if main is force-pushed, rebased, or the shared engine file changes shape. docs/reference/imports.md explicitly documents that refs can be a semantic tag, branch, or commit SHA — a pinned tag/SHA would give reproducible, auditable imports instead of drifting silently.
var knownEngineImports = map[string]string{
"opencode": "github/gh-aw/.github/workflows/shared/opencode.md@v1.2.3", // or a pinned commit SHA
}This is only a suggested fix in an error message today (nothing auto-fetches it), so the risk is low-severity now, but it normalizes an anti-pattern that could get copy-pasted into places that do auto-resolve imports.
| Runtime CodingAgentEngine // resolved adapter from the EngineRegistry | ||
| } | ||
|
|
||
| // knownEngineImports maps engine IDs that are not built into the binary to the |
There was a problem hiding this comment.
knownEngineImports duplicates knowledge that already lives in .github/workflows/shared/*.md, and nothing keeps the two in sync.
💡 Why this matters
The map is a second source of truth for which engine IDs have a shared definition file and where it lives. If a shared file under .github/workflows/shared/ is ever renamed, moved, or deleted, this map will keep emitting a tip pointing at a broken import path, and none of the new tests (TestEngineCatalog_Resolve_KnownImportTip) would catch the drift because they only assert against the map's own values, not against the actual filesystem.
A cheap guard would be a test that globs .github/workflows/shared/*.md and asserts every knownEngineImports value resolves to an existing file (and optionally that every existing shared engine file has a corresponding map entry), so the two never silently diverge.
| // | ||
| // To add a new well-known engine, append an entry here with the engine ID as the | ||
| // key and the fully-qualified path to its shared engine definition file as the value. | ||
| var knownEngineImports = map[string]string{ |
There was a problem hiding this comment.
No safeguard stops a future built-in engine ID from colliding with an entry already present in knownEngineImports.
💡 Why this matters
Resolve only consults knownEngineImports after both the exact catalog lookup and the runtime-ID prefix fallback fail, so today there's no functional bug. But if a new built-in engine is ever added under one of these five names (e.g. a native cursor engine ships later), the map entry becomes stale dead code that nobody is prompted to remove, and — more importantly — there's no test or init-time check that would flag the naming collision to a reviewer. A simple init()-time assertion (or a unit test) that knownEngineImports keys don't overlap with registry.GetSupportedEngines() would catch this automatically instead of relying on someone remembering to grep the map.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please address the latest maintainer feedback, refresh the branch if GitHub can update it cleanly, and run the Open review items to address (newest first):
Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31190216100
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot run pr-finisher skill |
| const ( | ||
| knownEngineImportsOwner = "github" | ||
| knownEngineImportsRepo = "gh-aw" | ||
| knownEngineImportsPath = ".github/aw/engines.json" |
There was a problem hiding this comment.
@copilot must be laoded from github through a raw user path
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please address the remaining maintainer/review feedback, refresh the branch if GitHub can update it cleanly, and run the Open items to address (newest first):
Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31195256639
|
| "engines": [ | ||
| { | ||
| "id": "aider", | ||
| "import": "github/gh-aw/.github/workflows/shared/aider.md@e3caca541fa7311d84947e25d3a7bfe4be3b431f" |
There was a problem hiding this comment.
@copilot let the cli add the sha/release version that embedded in the cli.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request . take claude golden from main |
…er-agent-imports # Conflicts: # pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
When
engine: opencodeorengine: crushis used without importing the engine definition file, the compiler currently emits a generic "invalid engine" error with no guidance. These engines exist inshared/opencode.md/shared/crush.mdbut are not built into the binary.Changes
pkg/workflow/engine_definition.go— addsknownEngineImportsmap (engine ID → import path) foropencodeandcrush;EngineCatalog.Resolveappends an import tip to the error message when the unknown engine ID is in the mappkg/workflow/engine_definition_test.go— addsTestEngineCatalog_Resolve_KnownImportTip(opencode, crush) andTestEngineCatalog_Resolve_UnknownNoTipError message before / after
New engines can be added by appending to
knownEngineImports— no other code changes required.Run context: https://github.com/github/gh-aw/actions/runs/31190216100> Generated by 👨🍳 PR Sous Chef · gpt54 · 21.1 AIC · ⊞ 8.3K · ◷