Add inline engine.driver sources for Copilot SDK workflows - #47991
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
engine.driver sources for Copilot SDK workflows
|
@copilot add integration workflow test |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds inline Node, Python, Go, and Java sources for Copilot SDK engine drivers.
Changes:
- Parses and validates inline driver configuration.
- Materializes drivers and installs runtime-specific SDK dependencies.
- Adds runtime detection, tests, schema, editor metadata, and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/runtime_setup_integration_test.go |
Tests compiled Python driver setup. |
pkg/workflow/runtime_detection.go |
Detects inline driver runtimes. |
pkg/workflow/engine.go |
Parses inline driver objects. |
pkg/workflow/engine_validation.go |
Validates inline driver constraints. |
pkg/workflow/copilot_inline_driver.go |
Generates source files and launchers. |
pkg/workflow/copilot_inline_driver_test.go |
Tests extraction, setup, and execution. |
pkg/workflow/copilot_engine_installation.go |
Installs language-specific SDK dependencies. |
pkg/workflow/copilot_engine_execution.go |
Executes workspace-generated launchers. |
pkg/parser/schemas/main_workflow_schema.json |
Defines the new driver shape. |
pkg/parser/schema_test.go |
Tests inline-driver schema validation. |
docs/src/content/docs/reference/glossary.md |
Updates driver terminology. |
docs/src/content/docs/reference/frontmatter.md |
Documents inline syntax. |
docs/src/content/docs/reference/frontmatter-full.md |
Regenerates schema reference. |
docs/src/content/docs/reference/engines.md |
Documents runtime materialization. |
docs/public/editor/autocomplete-data.json |
Adds editor metadata. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/15 changed files
- Comments generated: 4
- Review effort level: Medium
| case "java": | ||
| spec.stepName = "Install GitHub Copilot SDK (Java)" | ||
| spec.command = fmt.Sprintf( | ||
| `cd "${GITHUB_WORKSPACE}/%[1]s" && mvn -q dependency:build-classpath -Dmdep.outputFile="%[2]s"`, |
There was a problem hiding this comment.
Fixed in 64d1d1f. The Java inline install step now checks for mvn first and downloads a pinned Maven 3.9.9 from repo.maven.apache.org when not found — no-op on GitHub-hosted runners, bootstrap fallback on self-hosted runners.
| # Format 2: Inline Copilot SDK driver source. Provide exactly one runtime key. | ||
| driver: | ||
| # Inline Node.js driver source written to a generated .cjs file. | ||
| # (optional) | ||
| node: "example-value" |
There was a problem hiding this comment.
Fixed in 64d1d1f. The frontmatter-full.md example now shows exactly one runtime key (node) with a comment noting the four supported keys, satisfying the maxProperties: 1 schema constraint.
| switch d.Runtime { | ||
| case "go": | ||
| return map[string]string{ | ||
| inlineCopilotSDKDriverGoModPath: fmt.Sprintf("module %s\n\ngo %s\n", inlineCopilotSDKDriverGoModuleName, constants.DefaultGoVersion), |
There was a problem hiding this comment.
Fixed in 64d1d1f. buildInlineCopilotSDKDriverWriteStep now reads ParsedFrontmatter.RuntimesTyped.Go.Version and passes it to additionalFiles, falling back to constants.DefaultGoVersion only when no explicit pin is set. Test TestInlineGoDriverWriteStep_UsesEffectiveGoVersion verifies this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 87/100 — Excellent
📊 Metrics (7 tests)
Verdict
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (595 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add inline engine.driver sources for Copilot SDK workflows
The overall design is solid — clean struct separation, good validation gating on copilot-only, runtime detection integration, and solid test coverage.
One blocking bug requires a fix before merge:
🔴 Java wrapper passes source file path instead of class name (blocking)
In copilot_inline_driver.go line 56, the Java wrapper script does:
exec java -cp "$CLASSPATH_CONTENT" "${GITHUB_WORKSPACE}/.../Main.java" "$@"This is incorrect — java -cp expects a class name as the last positional argument, not a path to a .java source file. The fix is to use the inlineCopilotSDKDriverJavaMainClass constant ("Main") already defined in the file.
🟡 Non-blocking issues
- Dead code in
getInlineCopilotSDKInstallSpec—typescript,rust,dotnetcases are unreachable given current validation. - Silent multi-key discard in
applyEngineDriverField— when multiple runtime keys are present, the first is silently used; a validation error would be more user-friendly.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 38.5 AIC · ⌖ 4.8 AIC · ⊞ 5K
Comments that could not be inline-anchored
pkg/workflow/copilot_inline_driver.go:56
Bug: Java wrapper passes source file path instead of class name
The exec java -cp "$CLASSPATH_CONTENT" "${GITHUB_WORKSPACE}/.../Main.java" command is incorrect. When using -cp, the last argument to java must be the main class name (e.g., Main), not a path to a .java source file. A .java file is source code — this command fails at runtime with a class-not-found or illegal argument error.
Suggested fix:
exec java -cp "$CLASSPATH_CONTENT" Main "$@"The `inlineCop…
pkg/workflow/copilot_engine_installation.go:250
Dead code: unreachable typescript, rust, dotnet cases
getInlineCopilotSDKInstallSpec has cases for "typescript", "rust", and "dotnet" but the validation in validateInlineEngineDriver and applyEngineDriverField only allows node, python, go, java. These branches can never be reached at runtime.
Either:
- Remove the dead cases to keep the code consistent with validation, or
- Add the missing runtimes to validation/schema if they are intentionally planned.
pkg/workflow/engine.go:151
Silent discard of multiple runtime keys
When a driver map contains more than one runtime key (e.g., both node: and python:), applyEngineDriverField silently uses the first one it finds in the fixed iteration order and discards the rest. This could be confusing for workflow authors who accidentally provide multiple keys expecting an error.
Consider returning an error when more than one runtime key is present, consistent with the schema comment "Provide exactly one runtime key".
@c…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on three correctness issues and one dead-code hygiene issue.
📋 Key Themes & Highlights
Issues
- Dead code (lines 287–302 of
copilot_engine_installation.go):typescript,rust,dotnetcases ingetInlineCopilotSDKInstallSpecare unreachable — the supported runtime set is enforced earlier and those arms silently fall back to the Node.js default. - Incorrect Java wrapper fallback (
copilot_inline_driver.goline 56): Theexec javafallback whenclasspath.txtis absent passes a.javasource path as the main class argument, which is not valid JVM invocation syntax. - Go
go runat execution time (copilot_inline_driver.goline 54): Recompiles on every invocation; consider compiling to a binary during install. - Silent multi-runtime selection (
engine.goline 476): A driver map with multiple non-empty runtime keys silently picks the first one; this should be validated and rejected with a clear error.
Positive Highlights
- ✅ Good test coverage across all four runtimes in both unit and integration tests
- ✅ Clean separation: parsing, validation, materialisation, and runtime detection are each in their own focused function
- ✅ Heredoc delimiter randomisation (
GenerateHeredocDelimiterFromContent) prevents content collisions - ✅ Validation correctly gates inline drivers to the Copilot engine only
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 63 AIC · ⌖ 5.14 AIC · ⊞ 6.7K
Comment /matt to run again
| spec.command = workspaceCommandPrefix + "cargo add github-copilot-sdk@" + version | ||
| case "dotnet": | ||
| spec.stepName = "Install GitHub Copilot SDK (.NET)" | ||
| spec.command = workspaceCommandPrefix + "dotnet add package GitHub.Copilot.SDK --version " + version |
There was a problem hiding this comment.
[/codebase-design] Dead code: typescript, rust, and dotnet cases in getInlineCopilotSDKInstallSpec are unreachable — applyEngineDriverField only accepts node, python, go, java, and validateInlineEngineDriver enforces the same set. These arms silently fall back to a Node.js install command instead of returning an error, which will confuse future contributors.
💡 Suggested fix
Remove the three unreachable cases, or annotate them with a TODO that names the tracking work item. Either way, add a default arm that returns an empty spec with a log line so future callers fail loudly.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 412cd47. Removed the typescript, rust, and dotnet cases from getInlineCopilotSDKInstallSpec — they were unreachable since applyEngineDriverField only populates InlineDriver for node, python, go, and java. No default arm was needed because the function already returns the Node.js spec as the default when no case matches.
| case "go": | ||
| return "#!/usr/bin/env bash\nset -euo pipefail\ncd \"${GITHUB_WORKSPACE}/" + inlineCopilotSDKDriverDir + "\"\nexec go run \"" + inlineCopilotSDKDriverGoPath[strings.LastIndex(inlineCopilotSDKDriverGoPath, "/")+1:] + "\" \"$@\"\n" | ||
| case "java": | ||
| return "#!/usr/bin/env bash\nset -euo pipefail\nif [ -f \"${GITHUB_WORKSPACE}/" + inlineCopilotSDKDriverJavaClassPath + "\" ]; then\n CLASSPATH_CONTENT=$(cat \"${GITHUB_WORKSPACE}/" + inlineCopilotSDKDriverJavaClassPath + "\")\n exec java -cp \"$CLASSPATH_CONTENT\" \"${GITHUB_WORKSPACE}/" + sourcePath + "\" \"$@\"\nfi\nexec java \"${GITHUB_WORKSPACE}/" + sourcePath + "\" \"$@\"\n" |
There was a problem hiding this comment.
[/codebase-design] The Java wrapper script falls back to exec java "${GITHUB_WORKSPACE}/..." when classpath.txt is missing. java does not accept a source-file path as its first argument in standard mode — the fallback would fail with a cryptic JVM error. Either omit the fallback entirely (require classpath) or clarify the intent with a comment.
💡 Suggested fix
If classpath.txt is always generated by the Maven step, treat its absence as a hard failure:
#!/usr/bin/env bash
set -euo pipefail
CLASSPATH_CONTENT=$(cat "${GITHUB_WORKSPACE}/.gh-aw/copilot-sdk/classpath.txt")
exec java -cp "$CLASSPATH_CONTENT" Main "$@"This makes the failure mode obvious rather than producing a confusing JVM usage error at runtime.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 412cd47. The Java wrapper now uses cat unconditionally to read classpath.txt, removing the if [ -f ... ] guard and the plain-java fallback. If classpath.txt is absent, cat exits with a clear "No such file or directory" error rather than proceeding to a cryptic JVM invocation. Test TestInlineJavaWrapperScript_RequiresClasspath verifies the new behavior.
| case "python": | ||
| return "#!/usr/bin/env bash\nset -euo pipefail\nexec python3 \"${GITHUB_WORKSPACE}/" + sourcePath + "\" \"$@\"\n" | ||
| case "go": | ||
| return "#!/usr/bin/env bash\nset -euo pipefail\ncd \"${GITHUB_WORKSPACE}/" + inlineCopilotSDKDriverDir + "\"\nexec go run \"" + inlineCopilotSDKDriverGoPath[strings.LastIndex(inlineCopilotSDKDriverGoPath, "/")+1:] + "\" \"$@\"\n" |
There was a problem hiding this comment.
[/codebase-design] The Go wrapper uses go run at execution time, which recompiles the driver on every agent invocation. This adds latency and requires the Go toolchain at runtime (it must already be installed). Consider compiling to a binary during the install step instead, similar to how the Java classpath is materialized.
💡 Suggested alternative
Add a go build -o inline-driver-bin ./inline-driver.go command to the install step and point the wrapper at the binary. This avoids compile-time overhead on every execution and matches what Node/Python/Java do (they run pre-installed artifacts, not source).
@copilot please address this.
There was a problem hiding this comment.
Fixed in 412cd47. The Go install step now appends && go build -o inline-driver-bin ./inline-driver.go after go get, compiling the driver to a binary at install time. The wrapper script execs inline-driver-bin directly instead of using go run, eliminating per-invocation recompilation. Tests TestInlineGoInstallStep_CompilesBinary and TestInlineGoWrapperScript_ExecsBinaryNotGoRun verify this.
| appendHeredocWrite := func(path, content string, chmod bool) { | ||
| delimiter := GenerateHeredocDelimiterFromContent("INLINE_COPILOT_SDK_DRIVER", content) | ||
| step = append(step, fmt.Sprintf(" cat > \"${GITHUB_WORKSPACE}/%s\" << '%s'", path, delimiter)) | ||
| for line := range strings.SplitSeq(content, "\n") { |
There was a problem hiding this comment.
[/tdd] The heredoc loop uses strings.SplitSeq which preserves a trailing empty string when content ends with (very common for source files). This would emit an extra blank line after the delimiter that breaks the heredoc. There is no unit test for this boundary condition.
💡 Suggested test
Add a test case in copilot_inline_driver_test.go that verifies a source string ending in produces valid shell output with no stray blank lines after the content block:
func TestBuildInlineCopilotSDKDriverWriteStep_TrailingNewline(t *testing.T) {
d := &InlineEngineDriver{Runtime: "python", Source: "print('hello')
"}
step := buildInlineCopilotSDKDriverWriteStep(...)
// assert no blank line appears between content and delimiter
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 412cd47. The appendHeredocWrite helper now calls strings.TrimSuffix(content, "\n") before splitting, so a source file ending with \n no longer emits an extra blank line between the last content line and the heredoc delimiter. Test TestHeredocWrite_TrailingNewlineDoesNotProduceBlankLine verifies this boundary condition.
| for _, runtime := range []string{"node", "python", "go", "java"} { | ||
| source, ok := driverMap[runtime].(string) | ||
| if !ok || source == "" { | ||
| continue |
There was a problem hiding this comment.
[/codebase-design] When a driver map contains multiple runtime keys (e.g., node: and python: both set), applyEngineDriverField silently picks whichever comes first in the ordered iteration slice. A user mistake produces no error — a confusing silent behaviour. Consider rejecting driver maps with more than one non-empty key.
💡 Suggested fix
Count non-empty keys before selecting:
var matched []string
for _, runtime := range []string{"node", "python", "go", "java"} {
if s, ok := driverMap[runtime].(string); ok && s != "" {
matched = append(matched, runtime)
}
}
if len(matched) > 1 {
// return or log an error: "engine.driver: multiple runtimes specified"
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 412cd47. applyEngineDriverField now collects all matched runtime keys first. When more than one is present it sets InlineDriver.MultipleRuntime = true; validateInlineEngineDriver then rejects the map with "exactly one runtime key is allowed" before any engine-ID or empty-source checks run. Tests TestExtractEngineConfig_MultipleRuntimeKeys_SetsMultipleRuntimeFlag and TestValidateEngineDriver_MultipleRuntimeKeysRejected verify this.
There was a problem hiding this comment.
Request Changes — Java driver is broken at runtime; two other medium issues
The inline driver feature is well-structured and the Node/Python/Go paths look solid, but the Java path has two bugs that will cause runtime failures, and there's a silent-misconfiguration hazard in the parser. The rest of the code (validation, runtime detection, SDK wiring, test coverage) is sound.
Blocking issues
HIGH — Java class name bug (copilot_inline_driver.go line 56): The generated wrapper passes the full filesystem path to java -cp $CLASSPATH_CONTENT, but Java expects a bare class name (Main). This will always throw ClassNotFoundException when the classpath.txt exists. Fix: use inlineCopilotSDKDriverJavaMainClass (= "Main") as the class name argument. Also note the two branches (source-launch vs. classpath-launch) are semantically inconsistent — decide on one execution model.
MEDIUM — Java install step missing mkdir -p (copilot_engine_installation.go line 1755): The cd into the SDK driver directory will fail if the directory doesn't exist. Go and Python both guard with mkdir -p; Java should too.
MEDIUM — Silent first-wins on multiple inline runtime keys (engine.go line 2151): If a user specifies more than one runtime key in the driver object, the extra keys are silently dropped. At minimum log a warning; ideally return a validation error.
Non-blocking
Dead code in getInlineCopilotSDKInstallSpec (copilot_engine_installation.go line 1736): typescript, rust, and dotnet switch cases are unreachable given the parser only accepts node/python/go/java. Remove or wire them up.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 94.5 AIC · ⌖ 5.17 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/copilot_inline_driver.go:56
Java wrapper passes full .java file path as the class name, causing a runtime crash: exec java -cp "$CLASSPATH_CONTENT" "${GITHUB_WORKSPACE}/...Main.java" passes a filesystem path where Java expects a bare class name (Main), so the JVM will throw ClassNotFoundException.
<details>
<summary>💡 Suggested fix</summary>
The argument after -cp <classpath> must be the bare class name, not a file path:
exec java -cp "$CLASSPATH_CONTENT" Main "$@"The fallback branch (no cl…
pkg/workflow/copilot_engine_installation.go:1755
Java SDK install step will fail if the driver write step hasn't run: the Java case uses cd "${GITHUB_WORKSPACE}/.gh-aw/copilot-sdk" && mvn ... without a mkdir -p guard, but every other runtime (Go, Python) creates the directory before cd-ing into it.
<details>
<summary>💡 Suggested fix</summary>
Add a mkdir -p guard, consistent with the Go case (line ~1743):
case "java":
spec.command = fmt.Sprintf(
`mkdir -p "${GITHUB_WORKSPACE}/%[1]s" && cd "${GITHUB_WORKSPACE}/%…
</details>
<details><summary>pkg/workflow/engine.go:2151</summary>
**Multiple runtime keys in an inline driver object are silently ignored — no error or warning is returned**: if a user writes both `node:` and `python:` keys, the first one in the hard-coded iteration order wins and the rest are dropped without any diagnostic.
<details>
<summary>💡 Suggested fix</summary>
Count the number of non-empty runtime keys before selecting one, and return an error if more than one is present:
```go
func applyEngineDriverField(config *EngineConfig, engineObj map[strin…
</details>
<details><summary>pkg/workflow/copilot_engine_installation.go:1736</summary>
**Dead code: `typescript`, `rust`, and `dotnet` cases in `getInlineCopilotSDKInstallSpec` are unreachable**: the calling code only sets `InlineDriver.Runtime` to one of `["node","python","go","java"]` (via `applyEngineDriverField`), so the three additional switch cases will never execute.
<details>
<summary>Details</summary>
This is a latent maintenance trap: a future developer may think those runtimes are supported for inline drivers and add the corresponding parsing without realizing the sw…
</details>
|
@copilot please run the
|
…trap, Go version pin, docs example
- engine.go: preserve runtime for empty inline source so validateInlineEngineDriver
rejects it (removes silent bypass when driver: {python: ""} is used on non-copilot
engine or with empty content)
- copilot_engine_installation.go: Java inline driver now bootstraps Maven when not
pre-installed; adds pinned mvnw-style download from repo.maven.apache.org (already
in Java ecosystem allowlist) so self-hosted runners without Maven work correctly
- copilot_inline_driver.go: go.mod uses effective runtimes.go.version pin instead of
always hardcoding constants.DefaultGoVersion
- frontmatter-full.md: inline driver example now shows exactly one runtime key (was
showing all four, contradicting the maxProperties:1 schema rule)
- Tests: add TestExtractEngineConfig_InlineDriverEmptySourcePreserved,
TestValidateEngineDriver_EmptyInlineSourceIsRejected,
TestInlineJavaInstallStep_BootstrapsMaven,
TestInlineGoDriverWriteStep_UsesEffectiveGoVersion
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. All four review issues are addressed in the latest commit:
Tests added for all four fixes. |
|
@copilot update instructions in .github/aw/*.md for using a custom driver / copilot-sdk |
Document the new inline object form for engine.driver in the copilot-sdk section: one runtime key (node/python/go/java), materialization under .gh-aw/copilot-sdk/, automatic SDK installation, and constraints. Include YAML examples for all four supported runtimes. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. Updated
|
|
@copilot please run the
|
…pile, heredoc newline, multi-runtime rejection - Remove unreachable typescript/rust/dotnet cases from getInlineCopilotSDKInstallSpec; these were dead code since applyEngineDriverField only accepts node/python/go/java - Java wrapper: drop conditional fallback (exec java without classpath); cat classpath.txt directly so missing file produces a clear error instead of a cryptic JVM error - Go driver: compile to binary during install (go build -o inline-driver-bin) so the wrapper execs the pre-built binary instead of go run on every invocation - Heredoc trailing newline: trim suffix '\n' before SplitSeq so source files ending with a newline don't emit an extra blank line before the heredoc delimiter - Multiple runtime keys: detect all matched keys before selecting; set MultipleRuntime flag so validateInlineEngineDriver can reject the map with a clear error - Tests: add TestValidateEngineDriver_MultipleRuntimeKeysRejected, TestExtractEngineConfig_MultipleRuntimeKeys_SetsMultipleRuntimeFlag, TestInlineGoInstallStep_CompilesBinary, TestInlineGoWrapperScript_ExecsBinaryNotGoRun, TestInlineJavaWrapperScript_RequiresClasspath, TestHeredocWrite_TrailingNewlineDoesNotProduceBlankLine Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…k-line check - copilot_inline_driver.go: use strings.TrimRight(content, \"\\n\") instead of TrimSuffix to strip all trailing newlines, not just one - copilot_inline_driver_test.go: fix TestHeredocWrite test to: - strip heredoc single-quotes from delimiter before searching for it - add delimiterFound flag to stop scanning after delimiter is located - assert heredocFound so test doesn't silently pass if step format changes Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. All unresolved review threads addressed across two commits (412cd47 and the follow-up): github-actions[bot] threads (5 new issues):
Previously-outdated copilot reviewer threads:
Mergeable state: PR is open, not draft, |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🎉 This pull request is included in a new release. Release: |
This extends
engine.driverso Copilot workflows can define the driver inline instead of committing a separate file. The compiler now materializes inline Node, Python, Go, or Java driver sources at runtime and wires the engine to execute them with the matching Copilot SDK setup.New frontmatter shape
engine.drivernow accepts either the existing string form or an inline object with exactly one runtime source:node:python:go:java:Compiler support
Runtime materialization
.gh-aw/copilot-sdk/.mcp-scripts.SDK/runtime wiring
go getCopilot execution path
run: https://github.com/github/gh-aw/actions/runs/30159569186
Run: https://github.com/github/gh-aw/actions/runs/30160879503