Mark copilot-sdk mode as stable - #48978
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Promotes Copilot SDK mode to stable across compiler behavior and primary documentation.
Changes:
- Removes the experimental compiler warning.
- Updates warning regression tests.
- Documents SDK mode as a first-class Copilot variant.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_validators.go |
Removes the SDK experimental warning. |
pkg/workflow/copilot_sdk_experimental_warning_test.go |
Updates warning expectations. |
docs/src/content/docs/reference/frontmatter-full.md |
Removes the experimental label. |
docs/src/content/docs/reference/engines.md |
Adds SDK mode to the engine table. |
.changeset/minor-copilot-sdk-non-experimental.md |
Records the minor release change. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Medium
| tool-timeout: "example-value" | ||
|
|
||
| # Enables the experimental GitHub Copilot SDK integration (copilot engine only). | ||
| # Enables the GitHub Copilot SDK integration (copilot engine only). |
There was a problem hiding this comment.
Fixed in commit 34aa776. Updated both remaining references:
.github/aw/syntax-agentic.md:303: removedexperimental,from(experimental, copilot only)→(copilot only)pkg/workflow/README.md:150: removed**(Experimental)**from theCopilotSDKfield description
| # Test Workflow | ||
| `, | ||
| expectWarning: true, | ||
| expectWarning: false, |
There was a problem hiding this comment.
Fixed in commit 34aa776. Renamed the test to TestCopilotSDKNoExperimentalWarning, removed the expectWarning bool field and the dead positive-warning branch entirely, matching the pattern used in TestExperimentsNoExperimentalWarning and TestGuardPolicyNoExperimentalWarning.
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has only 2 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Great work! 🎉 This PR looks ready for review. The change cleanly removes the "experimental" designation from ✅ All checklist items pass:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are clean and well-scoped: removing copilot-sdk from the experimental warnings list, updating the test expectation, and surfacing SDK mode in docs all consistently reflect the graduation to stable. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14 AIC · ⌖ 5.9 AIC · ⊞ 5.3K
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — Expectation Update
📊 Analysis Details
Test Quality (Existing Test):
Verdict
|
There was a problem hiding this comment.
REQUEST_CHANGES
Doc lifecycle wording is inconsistent across surfaces, and the updated test has dead/misleading assertions.
💡 Themes
- Inconsistent stability claims:
frontmatter-full.mdnow callscopilot-sdkstable, but.github/aw/syntax-agentic.md:303andpkg/workflow/README.md:150-151still label it experimental. This should be reconciled before merge. - Dead test code:
copilot_sdk_experimental_warning_test.gonow setsexpectWarning: falsefor every case, making theexpectWarning==truebranch unreachable and theexpectedMessagestring stale. Not a functional bug, but reduces test clarity/maintainability.
Core compiler/changeset changes themselves are correct and minimal.
🔎 Code quality review by PR Code Quality Reviewer · aut00 · 30.5 AIC · ⌖ 7.23 AIC · ⊞ 7.8K
Comment /review to run again
| tool-timeout: "example-value" | ||
|
|
||
| # Enables the experimental GitHub Copilot SDK integration (copilot engine only). | ||
| # Enables the GitHub Copilot SDK integration (copilot engine only). |
There was a problem hiding this comment.
Marking this stable here contradicts two other docs that still call copilot-sdk experimental, leaving readers with conflicting lifecycle guidance.
💡 Details
.github/aw/syntax-agentic.md:303 still says **copilot-sdk/engine.driver** (experimental, copilot only) and pkg/workflow/README.md:150-151 still labels CopilotSDK/Driver as **(Experimental)**. If this PR's intent is to declare the feature stable, all doc surfaces referencing its maturity should be updated together; otherwise a reader following the frontmatter reference will get a different lifecycle status than the syntax guide or generated field docs.
Fix: update the three references consistently (either all say stable, or clarify why frontmatter differs).
There was a problem hiding this comment.
Fixed in commit 34aa776. Both references are now consistent:
.github/aw/syntax-agentic.md:303:(experimental, copilot only)→(copilot only)pkg/workflow/README.md:150:**(Experimental)** Enables GitHub Copilot SDK integration→Enables GitHub Copilot SDK integration
| # Test Workflow | ||
| `, | ||
| expectWarning: true, | ||
| expectWarning: false, |
There was a problem hiding this comment.
With every case now setting expectWarning: false, the if tt.expectWarning branch (checking for the warning message and GetWarningCount() > 0) is unreachable dead code.
💡 Details
Since the compiler no longer emits "Using experimental feature: engine.copilot-sdk" at all, this test can never exercise the positive-warning branch (lines ~97-105), and expectedMessage now refers to a warning string that doesn't exist anywhere in the compiler. The test still passes, but it's misleading: a future reader might think the warning path is still tested. Consider simplifying the test to just assert no warning is ever produced (drop the now-dead expectWarning/branch logic), matching the pattern used in experiments_experimental_warning_test.go for other stabilized features.
There was a problem hiding this comment.
Fixed in commit 34aa776. Removed the expectWarning bool struct field and the unreachable positive-warning code path. The test now directly asserts no warning is produced, matching TestExperimentsNoExperimentalWarning and TestGuardPolicyNoExperimentalWarning.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — the change is clean and well-scoped. The two existing review comments (stale experimental references in other docs, and dead positive-branch test code) are the highest-impact items; no new blocking issues found.
📋 Highlights
Positive Highlights
- ✅ Compiler change is surgical — removes exactly one entry from the experimental-feature list
- ✅ Test name and assertion updated in sync with the behaviour change
- ✅ Docs table row uses correct engine value (
copilot+copilot-sdk: true) - ✅ Changeset entry is accurate and bumped at the right level (minor)
Existing Comments to Address
- Stale
experimentalreferences — other docs files still describecopilot-sdkas experimental; those should be updated for consistency. - Dead test branch — after flipping
expectWarningtofalsefor all cases, the positive-warning branch in the test helper becomes unreachable; either add a new test case that exercises it or remove the dead branch.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 19.3 AIC · ⌖ 6.63 AIC · ⊞ 7K
Comment /matt to run again
|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/30492442799
|
…ocs and clean up test Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all four threads in commit
|
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
.github/aw/syntax-agentic.md:303
- This removes the experimental label from the combined
copilot-sdk/engine.driverentry, but the PR only stabilizes SDK mode andpkg/workflow/README.md:151still explicitly marksengine.driverexperimental. The agent guidance would therefore present custom drivers as stable. Keep the experimental qualifier onengine.driverwhile promoting onlycopilot-sdk.
- **`copilot-sdk` / `engine.driver`** (copilot only): set `copilot-sdk: true` to start a headless Copilot CLI SDK sidecar. Set `driver: <path-or-command>` on the copilot engine to supply a custom SDK driver (`.js`/`.cjs`/`.mjs`/`.py`/`.ts`/`.mts`/`.rb`, or a bare PATH command); this also enables `copilot-sdk: true` automatically. Tune the repeated-tool-denial safeguard with the top-level `max-tool-denials:` field (default `5`).
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Medium
|
@copilot please run the Reviewer feedback to address:
Run: https://github.com/github/gh-aw/actions/runs/30497974805
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All review feedback is addressed. The latest commit ( Summary of all changes across this PR:
All unit tests pass. |
|
@copilot please run the
|
Triage: Mark copilot-sdk mode as stable
|
All reviewer feedback was addressed in previous commits — no new unresolved threads since the latest push (
The |
Triage Summary
Doc/compiler flag change; PR is in blocked (merge-conflict) state — needs rebase. Low urgency.
|
engine.copilot-sdkhas been shipping and tested long enough to drop the experimental label. This removes the compiler warning and surfaces SDK mode as a first-class Copilot variant in the docs.Changes
compiler_validators.go): Removeengine.copilot-sdkfrom the experimental feature warnings list — workflows usingcopilot-sdk: trueno longer emit a warning at compile time.copilot_sdk_experimental_warning_test.go): Update test expectations to assert no warning is produced forcopilot-sdk: true.engines.md): Add "GitHub Copilot (SDK mode)" as a distinct row in the Available Coding Agents table:frontmatter-full.md): Remove the word "experimental" from thecopilot-sdkfield comment.Run: https://github.com/github/gh-aw/actions/runs/30503727864