Repository navigation
Fix Copilot URL approvals from finalized runtime firewall policy - #70750
Conversation
Translate finalized AWF domain rules into scoped CLI URL approvals and denials, including caller additions, without changing shell, SDK, or firewall permissions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ 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.
|
|
✅ 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Request changes
The new permission injection has a runtime configuration-path regression and silently changes behavior for global allow rules. The first issue can make the harness read the wrong firewall config or fail before execution; the second leaves valid policies without approvals and no diagnostic. Please resolve both before merging.
Findings
- Use
GH_AW_AWF_CONFIG_PATH(with the existing default) rather than the hard-coded module constant when reading finalized network policy. - Reject or explicitly diagnose
*inallowDomains, instead of silently dropping it.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.23 AIC · ⌖ 0.625 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd to the new copilot_url_permissions.cjs module (triaged as a bug fix; triage ran successfully). No blocking issues — 2 minor, non-blocking suggestions posted inline.
📋 Key Themes & Highlights
Key Themes
- Error diagnostics:
addCopilotURLPermissionsswallows the real read/parse error in itscatch {}, unlike the siblingloadAwfConfigData()in the same file which distinguishesENOENTfrom other failures and logs detail. Minor, but will make a future runtime config bug harder to triage. - Hostname validation scope: single-label hostnames (no TLD) pass the DNS-label regex with no explicit test documenting whether that's intended — worth a quick confirmation + test so the supported surface is explicit rather than implicit.
Positive Highlights
- ✅ Explicit fail-over-silent-drop design for global wildcard domain blocks — directly closes the gap described in the PR body and avoids a dangerous silent no-op.
- ✅ Strong test coverage in
copilot_url_permissions.test.cjs: parameterized malformed-rule rejection, protocol/port/IP preservation, SDK/unsandboxed bypass, and a real subprocess integration test against the harness. - ✅ Docs update (
docs/src/content/docs/engines/copilot.mdline 60) accurately reflects the actual code behavior (apex+subdomain, both protocols unless restricted, no shell/firewall bypass). - ✅ Insertion before the
--CLI delimiter is correct and covered by a dedicated test.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 152.5 AIC · ⌖ 15 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
🔵 Needs a closer look
Native CLI permission matching needs human verification, and confirmed startup regressions remain unresolved.
2 open issues
What changed in this PR
Derives sandboxed Copilot CLI URL approvals from finalized AWF policy, including runtime caller additions, without changing shell permissions.
Changes:
- Adds scoped URL allow/deny translation and validation.
- Integrates approvals into harness startup with regression coverage.
- Documents behavior and adds a patch changeset.
| File | Description |
|---|---|
docs/src/content/docs/engines/copilot.md |
Documents sandboxed URL approvals. |
actions/setup/js/copilot_url_permissions.test.cjs |
Tests translation, runtime additions and subprocess arguments. |
actions/setup/js/copilot_url_permissions.cjs |
Translates finalized firewall rules into URL permissions. |
actions/setup/js/copilot_harness.test.cjs |
Adds config fixtures for startup tests. |
actions/setup/js/copilot_harness.cjs |
Applies URL permissions before CLI startup. |
.changeset/copilot-runtime-url-approvals.md |
Records the patch release change. |
Review effort: Max
|
@copilot fix ci failures |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed the CJS failures in |
|
@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: 145cda6
|
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>
Require published dry-run source and lock revisions, existing review and execution gates, and stop on authorization failures. Keep non-dry-run dispatch prohibited. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/smoke-copilot |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
|
Smoke partial. Me run few test. PRs ✅, curl ✅, file ✅, build ✅, summarizer ✅. Serena, Playwright, others not run ❌. Overall: FAIL.
|
|
🎉 This pull request is included in a new release. Release: |

Summary
Fix the URL-approval portion of #70743 without changing shell permissions or addressing the separate browser ABI failure. This replaces only the URL work from closed, unmerged #70697; merged ledger fixes are untouched.
network_allowedexpansion, rather than copying compile-time domain strings into flags.Evidence
The raw session of run 39529502581 contains three URL approval requests for
https://github.com, all denied by the unattended fallback. Localdateandcatsucceeded. The invocation had--allow-tool shellbut no URL approvals.The independent browser-library ABI failure is tracked in github/gh-aw-firewall#10385, not fixed here.
Validation
make fmt-cjsandmake lint-cjspassed (repository-wide lint reports existing warnings, zero errors).PATH=/opt/homebrew/bin:$PATH make agent-report-progresspassed, including impacted tests. The first attempt used macOS Bash 3 and failed before validation becausemapfilewas unavailable; reran successfully with installed Bash 5..cjshelpers.help permissionsconfirms separate protocol approvals and wildcard URL patterns. End-to-end hosted CLI behavior, especially omitted-port/nested-subdomain matching, remains unverified; no workflows were dispatched.Branch renaming was blocked by an existing shared
packed-refs.lock; that lock was not removed. The original isolated session branch was committed and pushed successfully.