Repository navigation
fix: register staged Copilot skills and constrain optimizer recovery - #70359
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Verdict
No actionable correctness or security defects found in the changed lines. The staged-skill registration is covered for normal/bare sessions, absent roots, and invalid file/symlink roots; review was static and no code or tests were executed.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.27 AIC · ⌖ 0.542 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs (triage: bug_fix). The fix is well-scoped, traceable to the documented root cause, and covered by comprehensive mocked regression tests. Approving with two minor, non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause correctly addressed:
skillDirectories+enableConfigDiscovery: falseare verified fields in the vendored@github/copilot-sdktype definitions (not invented), and the file/symlink guard useslstatSync(notstatSync) to avoid following a symlink before validating it — correct choice for a trust boundary. - Regression coverage is strong: new tests cover bare/normal session registration, all four staged Opt phase skills, ambient-skill exclusion when nothing is staged, and both invalid-root cases (file, symlink). One small gap: a dangling symlink isn't separately tested (see inline comment), though the existing logic appears to already handle it correctly.
- Tool Discipline section is a good recovery-pattern fix: replacing
Invoke skillindirection with directviewreads of staged absolute paths removes the SDK-catalog dependency that caused the original denial-budget exhaustion, and the explicit "don't retry denied commands, report via safe-outputs instead" instruction directly targets the regression from the linked Copilot Opt run. - Error handling verified: the new
throwfor a non-directory skills root happens inside the existing try block (afterclient.start()), so it's caught by the existingcatchand reported assession.errorwithexitCode: 1— no resource leak,client.stop()still runs viafinally.
Positive Highlights
- ✅ Minimal, surgical diff (7 lines in the core driver) for what could have been a much larger change
- ✅ Spec, changeset, and compiled lock file all updated consistently with the code change
- ✅ Default-deny posture preserved:
enableConfigDiscovery: falsekeeps ambient repo/home skill roots out, only the explicitly staged directory is registered
Two inline suggestions posted (a missing dangling-symlink test case, and a spec clarification that invalid-root rejection is a hard session-creation failure, not a soft warning) — neither is blocking.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 126.4 AIC · ⌖ 14.9 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🔵 Needs a closer look
Registration affects every SDK session, while actual SDK skill discovery, isolation, and recovery remain unverified.
1 open issue
What changed in this PR
Registers staged workflow skills in Copilot SDK sessions and improves Copilot Opt’s recovery guidance without changing permission controls.
Changes:
- Registers staged skills in normal and bare sessions while disabling ambient discovery.
- Reads optimizer phase instructions directly and adds allowlist-aware recovery guidance.
- Adds mocked regression coverage, specification updates, and release metadata.
| File | Description |
|---|---|
docs/src/content/docs/specs/copilot-sdk-driver-specification.md |
Documents explicit skill registration and root validation. |
actions/setup/js/copilot_sdk_session.test.cjs |
Tests staging, registration, ambient exclusion, and invalid roots. |
actions/setup/js/copilot_sdk_session.cjs |
Validates and registers the staged skill directory. |
.github/workflows/copilot-opt.md |
Adds recovery guidance and direct phase-file reads. |
.github/workflows/copilot-opt.lock.yml |
Refreshes the compiled workflow body hash. |
.changeset/patch-copilot-opt-skill-registration.md |
Records the patch release changes. |
Review effort: Max
|
@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: 0e3a469
|
…ccoli Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
In response to #70359 (comment): No code change is needed for this informational design-gate result, which requests no ADR or design revision. Publication was separately verified at commit 35ae9e0; the report alone is not being treated as validation. |
|
In response to #70359 (comment): No code change is needed because this completion notice contains no actionable finding. The requested review fixes were independently verified, and the latest-main integration was validated locally and pushed. |
|
In response to #70359 (comment): No additional change is needed for this status-only report. The dangling-symlink regression is already present in a058b44, and the mocked session suite passed all 30 tests during this pass. |
|
In response to #70359 (comment): No code change is needed; the associated review found no actionable correctness or security defects. The final local progress gate passed after updating from main. |
|
In response to #70359 (comment): Accepted both associated suggestions, already implemented in a058b44: dangling-symlink coverage and explicit hard-failure documentation. Both were independently verified during this pass, so no duplicate edits or inline replies were added. |
|
In response to #70359 (comment): Accepted: make VERSION=1.0.98 merge-main completed without conflicts, and the validated merge was pushed as 35ae9e0. All three requested review fixes were already present in a058b44 and have been verified and their threads resolved; the final local progress gate passed, no CI was triggered, and only human review and current-head CI remain. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |

Why
Copilot Opt run 39296602400 exhausted its three-denial budget by retrying shell chains containing undeclared commands. Separately, its phase skill files were staged and readable but missing from the SDK skill catalog. Model resolution succeeded; this was not a missing-model or credential failure.
Approach
view. Document absolute-path, allowlist-aware analysis and recovery instead of retrying deniedcd,cut, orsedchains.Permissions, credentials, sandboxing, safe-output limits, and the three-denial guard are unchanged.
Validation
npm run typecheckinactions/setup/js.npm run test:js -- --no-file-parallelism copilot_sdk_session.test.cjs copilot_sdk_driver.test.cjs extract_inline_skills.test.cjs: 133 tests passed.make VERSION=1.0.98 recompile: all 344 workflows compiled../gh-aw compile copilot-opt --dry-run --no-check-update --schedule-seed github/gh-aw --json: source/model checks and ShellCheck passed.PATH="/opt/homebrew/bin:$PATH" make VERSION=1.0.98 BASE_REF=1215c14345db2252c84a86f62c314480af3d4404 agent-report-progress: passed formatting, lint, impacted tests, schema freshness, and workflow drift checks.No workflow was dispatched or rerun, and no live SDK, inference, or MCP execution was performed. Live recovery remains unverified; optional security scanners were not run.