Skip to content

fix: register staged Copilot skills and constrain optimizer recovery - #70359

Merged
pelikhan merged 5 commits into
mainfrom
pelikhan-animated-broccoli
Oct 19, 2026
Merged

pelikhan merged 5 commits into
mainfrom
pelikhan-animated-broccoli

Conversation

@pelikhan

Copy link
Copy Markdown
Collaborator

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

  • Explicitly register the staged workflow skill directory in Copilot SDK sessions, including bare mode, while keeping ambient configuration discovery disabled. Missing roots produce an empty list; file or symlink roots fail explicitly.
  • Read Copilot Opt's four phase instructions directly with view. Document absolute-path, allowlist-aware analysis and recovery instead of retrying denied cd, cut, or sed chains.
  • Add mocked regression coverage for actual inline skill staging, bare/normal session registration, ambient-skill exclusion, and invalid roots. Update the SDK specification, patch changeset, and compiled workflow hash.

Permissions, credentials, sandboxing, safe-output limits, and the three-denial guard are unchanged.

Validation

  • npm run typecheck in actions/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.
  • Read-only security review: PASS, no confirmed vulnerabilities.

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.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@pelikhan
pelikhan marked this pull request as ready for review October 19, 2026 20:19
Copilot AI max review requested due to automatic review settings October 19, 2026 20:19
@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

✅ 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: reported_no_action

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.

Generated from Design Decision Gate 🏗️ · opus55 · 10 AIC · ⌖ 31.8 AIC · ◇ 13.4 AIC · ⊞ 1.9K · ◷

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

✅ Ponytail Reviewer completed successfully!

Ponytail Reviewer

Outcome: reported_no_action

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.

Generated from Ponytail Reviewer · gpt56 · 6.15 AIC · ⌖ 5.32 AIC · ⊞ 14.7K · ◷

Generated by Ponytail Reviewer for #70359

@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel

Outcome: reported_no_action

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.

Generated from Test Quality Sentinel · haiku45 · 5.19 AIC · ⌖ 7.81 AIC · ◇ 2.53 AIC · ⊞ 9.2K · ◷

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: false are verified fields in the vendored @github/copilot-sdk type definitions (not invented), and the file/symlink guard uses lstatSync (not statSync) 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 skill indirection with direct view reads 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 throw for a non-directory skills root happens inside the existing try block (after client.start()), so it's caught by the existing catch and reported as session.error with exitCode: 1 — no resource leak, client.stop() still runs via finally.

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: false keeps 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

Comment thread actions/setup/js/copilot_sdk_session.cjs
Comment thread docs/src/content/docs/specs/copilot-sdk-driver-specification.md

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

Comment thread .github/workflows/copilot-opt.md
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
  2. Review (actions/setup/js/copilot_sdk_session.cjs:385): [/tdd] Nice guard — using lstatSync (not statSync) correctly fails fast on a symlinked skills root without following it, and the test suite covers both the file and symlink cases. One edge case not covered: a dangling symlink (target does not exist). lstatSync would still report isDirectory() === false for it, so the explicit-reject path is hit correctly — worth a quick confirming test since the current regression test only exercises a symlink pointing at a real directory. - fix: register staged Copilot skills and constrain optimizer recovery #70359 (comment)
  3. Review (docs/src/content/docs/specs/copilot-sdk-driver-specification.md:158): [/grill-with-docs] Good addition, but the spec doesn't mention the file/symlink rejection failure mode's observable effect: session creation fails entirely (exitCode 1, session.error event), not a soft warning. Worth one sentence clarifying that this is a hard failure so operators reading the spec don't assume a silent skip. - fix: register staged Copilot skills and constrain optimizer recovery #70359 (comment)
  4. Review (.github/workflows/copilot-opt.md:142): Optional: list the permitted analysis commands here rather than referring only to the configured allowlist. Runtime import strips this workflow's frontmatter, and the permission handler returns only a generic rejection (copilot_sdk_permissions.cjs:417). Avoiding cd, cut, and sed still leaves the agent guessing whether alternatives such as awk, xargs, or tr are permitted; those commands are also absent from the effective rules in copilot-opt.lock.yml:1050. Naming allowed alternatives, including python or grep for plain-text transcripts, would reduce avoidable denials without... - fix: register staged Copilot skills and constrain optimizer recovery #70359 (comment)

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
Sous-chef work: 2bae5e21dde1e44675ad05d4f6b48ab78c7e9355feb2c16961c4b1e805fde11a 6bf6fa6d51953ce7842ab1072c997e576512f2abc00667a164cb212ce9492d87 d6edf88e3dacccaf1bb6bee1c0ba27e35dd7d0e422de3d52ea905f929d6bc5f2
Sous-chef state: ac02585e4e518e15b3dc620f1d9531c181641bf9d32dac39bdaaf944673c079e

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · 11.4 AIC · ⌖ 6.82 AIC · ⊞ 1.1K · ◷
Comment /souschef to run again

Copilot AI and others added 2 commits October 19, 2026 21:13
…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>
@pelikhan

Copy link
Copy Markdown
Collaborator Author

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.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

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.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

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.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

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.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

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.

@pelikhan

Copy link
Copy Markdown
Collaborator Author

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>
@pelikhan
pelikhan merged commit 5676aa8 into main Oct 19, 2026
15 checks passed
@pelikhan
pelikhan deleted the pelikhan-animated-broccoli branch October 19, 2026 21:47
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.92.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants