-
Notifications
You must be signed in to change notification settings - Fork 448
fix: broaden engine.driver schema description to match runtime multi-language support #44355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2320,11 +2320,14 @@ engine: | |
| # (optional) | ||
| copilot-sdk: true | ||
|
|
||
| # Custom inner driver script for engines that support driver mode (e.g. pi). | ||
| # Accepts a workspace-relative path ending with .js, .cjs, or .mjs (e.g. | ||
| # `.github/drivers/pi_agent_core_driver_sample_node.cjs`), or a bare filename | ||
| # without a path separator resolved from the gh-aw setup-action directory (e.g. | ||
| # `pi_agent_core_driver.cjs`). | ||
| # Custom driver script or command for the engine. Accepts a workspace-relative | ||
| # path or bare name. Supported extensions depend on the engine: the copilot | ||
| # engine accepts .js, .cjs, .mjs (Node.js), .py (Python), .ts or .mts | ||
| # (TypeScript), or .rb (Ruby); other engines (e.g. pi) accept .js, .cjs, or | ||
| # .mjs only. A bare name without an extension is also valid for any engine | ||
| # (treated as a built-in driver or executable in PATH). Examples: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Factual error: bare names for the 💡 Details and suggested fixWhat the runtime actually does
// no "/" → resolve from setup-action directory
driverPath = fmt.Sprintf(`"%s/%s"`, SetupActionDestinationShell, driverName)So The old wording was correct: "resolved from the gh-aw setup-action directory." The copilot engine does treat bare names (no extension) as arbitrary executables in PATH — but the updated text accidentally generalises this to "any engine", which is wrong for pi. Suggested wording |
||
| # `.github/drivers/my_driver.cjs`, `.github/drivers/my_driver.py`, | ||
| # `my-copilot-driver`. | ||
|
Comment on lines
+2327
to
+2330
|
||
| # (optional) | ||
| driver: "example-value" | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12415,7 +12415,7 @@ | |
| "driver": { | ||
| "type": "string", | ||
| "pattern": "^(?:(?:\\.[A-Za-z0-9_]|[A-Za-z0-9_])[A-Za-z0-9._-]*)(?:/(?:(?:\\.[A-Za-z0-9_]|[A-Za-z0-9_])[A-Za-z0-9._-]*))*$", | ||
| "description": "Custom inner driver script for engines that support driver mode (e.g. pi). Accepts a workspace-relative path ending with .js, .cjs, or .mjs (e.g. `.github/drivers/pi_agent_core_driver_sample_node.cjs`), or a bare filename without a path separator resolved from the gh-aw setup-action directory (e.g. `pi_agent_core_driver.cjs`)." | ||
| "description": "Custom driver script or command for the engine. Accepts a workspace-relative path or bare name. Supported extensions depend on the engine: the copilot engine accepts .js, .cjs, .mjs (Node.js), .py (Python), .ts or .mts (TypeScript), or .rb (Ruby); other engines (e.g. pi) accept .js, .cjs, or .mjs only. A bare name without an extension is also valid for any engine (treated as a built-in driver or executable in PATH). Examples: `.github/drivers/my_driver.cjs`, `.github/drivers/my_driver.py`, `my-copilot-driver`." | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/grill-with-docs] Same bare-name accuracy issue: the schema description says bare names are treated as "executable in PATH" for any engine, but for the pi engine they are resolved from the setup-action directory. 💡 Suggested wordingCurrent:
Suggested:
This aligns the JSON schema description with the runtime behaviour in @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Schema description does not match docs: the 💡 DetailsThe JSON Schema description on line 34 of the diff says:
This is accurate for the copilot engine, but for the pi engine driverPath = fmt.Sprintf(`"%s/%s"`, SetupActionDestinationShell, driverName)i.e., Suggested replacement for the bare-name clause: |
||
| }, | ||
| "extensions": { | ||
| "type": "array", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] The phrase "executable in PATH" is inaccurate for the pi engine — it drops a correct detail from the original docs and contradicts the internal implementation.
💡 Details and suggested fix
Per
buildPiDriverCommand()inpkg/workflow/pi_engine.go(line 562–572) and theDriverfield comment inpkg/workflow/engine.go(line 61), a bare name for the pi engine is resolved from the setup-action directory (${RUNNER_TEMP}/gh-aw/actions/<name>), not from PATH. Only the copilot engine treats bare names as arbitrary PATH executables.The original docs correctly said:
Suggested replacement for the last comment line before the
(optional)marker:This preserves the accurate per-engine distinction that was lost in the rewrite.
@copilot please address this.