Repository navigation
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Treat the explicit Copilot dynamic-workflow advisory as informational in development mode while preserving other warning and scanner gates. Remove the LSP smoke sample and update regression coverage and documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Actions run 37713903842 loaded the packaged extension but stopped after an unattended URL approval denial for https://github.com. Pre-approve only the existing GitHub and local Playwright test destinations, and keep independent checks running after failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add a minimal permission reproducer and keep dynamic workflows opt-in and warning-gated in dry-run compilation until hosted execution is established. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #66747 does not have the 'implementation' label (has_implementation_label=false) and has only 82 new lines in default business logic directories, which is at or below the 100-line threshold (requires_adr_by_default_volume=false). No custom .design-gate.yml config present.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
I found two actionable concerns: the new reproducer is pinned to an older Copilot CLI than the workflow it validates, and this change removes the only Copilot smoke coverage for hosted LSP/document-symbol wiring.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 69.3 AIC · ⌖ 5.29 AIC · ⊞ 19.8K
Comment /review to run again
There was a problem hiding this comment.
🟡 Changes recommended
The reproducer can still create automatic failure issues despite its logging-only, no-resource contract.
1 open finding
What changed in this PR
Adds smoke coverage to distinguish Copilot CLI URL approval failures from dynamic-workflow execution failures.
Changes:
- Runs packaged dynamic workflows first in the main Copilot smoke.
- Adds a minimal permission reproducer and generated workflow.
- Preserves dry-run warning behavior with tests and documentation.
| File | Description |
|---|---|
pkg/workflow/copilot_dynamic_workflows_experimental_warning_test.go |
Tests dry-run and diagnostic failure behavior. |
pkg/workflow/compiler_validators.go |
Handles warning-output failures. |
docs/src/content/docs/reference/compilation-process.md |
Documents dry-run warning failures. |
docs/src/content/docs/engines/copilot.md |
Documents smoke coverage and limitations. |
.github/workflows/smoke-copilot.md |
Adds dynamic-workflow smoke test 16. |
.github/workflows/smoke-copilot.lock.yml |
Regenerates the main smoke workflow. |
.github/workflows/smoke-copilot-dynamic-workflows.md |
Adds the minimal permission reproducer. |
.github/workflows/smoke-copilot-dynamic-workflows.lock.yml |
Adds its generated Actions workflow. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
There was a problem hiding this comment.
Reviewed the diff (compiler_validators.go error-handling change, new experimental-warning tests, smoke-copilot.md/smoke-copilot-dynamic-workflows.md changes, and docs). No blocking issues found.
writeExperimentalFeatureDiagnosticcorrectly counts the write failure as an additional warning and still surfaces the original message via stderr; covered byTestCopilotDynamicWorkflowsDryRunDiagnosticWriteFailure.- LSP removal from
smoke-copilot.mdand replacement of test 16 with the dynamic-workflow check are consistent (no leftoverlsp:frontmatter in the committed version). - New minimal reproducer workflow (
smoke-copilot-dynamic-workflows.md) is scoped, staged, and creates no GitHub resources, matching its stated intent. go build,go vet,gofmt -l, and the targeted test suite (TestCopilotDynamicWorkflows*,TestEnforceDevelopmentDiagnostics,TestDevelopmentDiagnostics) all pass locally.
No actionable Impeccable-mode findings (no UI/UX surface changed; this PR is compiler + CI workflow config + docs).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 59.7 AIC · ⌖ 13 AIC · ⊞ 8.1K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs (via pr-triage) — requesting changes on test-coverage depth and a naming collision.
📋 Key Themes & Highlights
Key Themes
- Test coverage is count-based, not behavior-based: the new diagnostic-write-failure test asserts
GetWarningCount()increments but never drives the failure through an actual dry-run CLI path to confirm user-visible compilation failure. - Implicit convention between compiler and CLI:
emitExperimentalFeatureWarningsTorelies on every caller independently checkingGetWarningCount() > 0to enforce the dry-run block; nothing makes this contract explicit or hard to miss at new call sites. - Near-duplicate workflow filenames:
smoke-copilot-dynamic-workflows.md(new) vs. the existingsmoke-copilot-dynamic-workflow.mddiffer by a single letter, inviting confusion given the PR already frames the new file as a distinct "permission reproducer."
Positive Highlights
- ✅ Honest, well-documented investigation limitations in the PR description — the author is explicit that hosted dynamic-workflow support remains unverified and that Actions green status doesn't prove success.
- ✅ Thorough matrix-style regression tests (
TestCopilotDynamicWorkflowsDryRunWarning,...DryRunPreservesOtherWarnings) covering batch/SDK/enabled permutations. - ✅ The minimal reproducer workflow (
smoke-copilot-dynamic-workflows.md) is appropriately scoped: staged, logging-only, creates no GitHub resources, with clear instructions not to retry denials or expand permissions.
Note: skill selection used the automated pr-triage agent output (/tdd, /grill-with-docs); no fallback heuristic was needed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 98.2 AIC · ⌖ 15.8 AIC · ⊞ 10.1K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/copilot_dynamic_workflows_experimental_warning_test.go:138
[/tdd] writeExperimentalFeatureDiagnostic silently continues after a write failure instead of surfacing it to the caller, and the new test only asserts the warning count increments — it never verifies compilation actually fails or that a caller-visible error occurs.
<details>
<summary>💡 Why this matters</summary>
If fmt.Fprintln(writer, ...) fails (e.g. a broken pipe to stdout in --dry-run mode), the only observable effect is one extra warning count. There's no test exercising the …
.github/workflows/smoke-copilot-dynamic-workflows.md:5
[/grill-with-docs] smoke-copilot-dynamic-workflows.md (new) and the pre-existing smoke-copilot-dynamic-workflow.md differ only by a trailing "s" — this is a high risk of confusion for future readers/maintainers distinguishing "the packaged extension smoke test" from "the permission reproducer".
<details>
<summary>💡 Suggested naming</summary>
The PR body and docs already describe this new file as a "permission reproducer" / "minimal reproducer" — consider naming the file to match, e.g…
Disable automatic conclusion issue reporters, document the intentional CLI pin, and verify experimental warning enforcement through the dry-run CLI gate and failed diagnostic output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: ee8c9f5
|
…flow-permissions Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Merged the latest |
Motivation
Investigate unattended Copilot CLI permission failures without confusing native URL approval, firewall policy, extension discovery, and actual dynamic-workflow execution. Hosted dynamic workflows are not yet verified, so they remain explicitly opt-in and experimental.
Changes
smoke-copilot, run it first, and verify its run ID, completion status, and exact result. Continue independent checks after failures.smoke-copilot-dynamic-workflows.md, a minimal staged, logging-only reproducer that independently attempts an intentionally unapproved GitHub URL and the packaged dynamic workflow. It creates no GitHub resources.Investigation results and limitations
Local packaged dynamic-workflow execution passed. The hosted smoke run discovered the extension but encountered native CLI URL approval denial and stopped before attempting the dynamic workflow. Its green Actions status did not establish smoke success. The scoped URL-approval fix and minimal reproducer have not been rerun in Actions.
Dynamic-workflow warnings intentionally block
--dry-run; normal compilation still succeeds with the warning. This PR does not claim hosted dynamic-workflow support is functional.Validation
make buildandmake fmtgo test ./pkg/workflow ./pkg/cli -run 'TestCopilotDynamicWorkflows|TestEnforceDevelopmentDiagnostics|TestDevelopmentDiagnostics' -count=1make recompilemake agent-report-progress