Centralize engine default domain sets - #55461
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Centralizes built-in engine and threat-detection domain sets while separating internal defaults from selectable ecosystems.
Changes:
- Adds a copied domain-set registry and compatibility variables.
- Moves threat-detection domains out of ecosystem configuration.
- Documents engine domain sets and adds isolation tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/domains.go |
Introduces the centralized registry. |
pkg/workflow/domains_test.go |
Tests registry contents and copy isolation. |
pkg/workflow/data/ecosystem_domains.json |
Removes the selectable threat-detection ecosystem. |
docs/src/content/docs/reference/network.md |
Documents automatic domain sets. |
.github/aw/network.md |
Adds domain-set guidance for agents. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Balanced
| "terraform": ["releases.hashicorp.com", "apt.releases.hashicorp.com", "yum.releases.hashicorp.com", "registry.terraform.io"], | ||
| "threat-detection": [ | ||
| "api.business.githubcopilot.com", | ||
| "api.enterprise.githubcopilot.com", | ||
| "api.github.com", | ||
| "api.githubcopilot.com", | ||
| "api.individual.githubcopilot.com", | ||
| "github.com", | ||
| "host.docker.internal", | ||
| "registry.npmjs.org", | ||
| "telemetry.enterprise.githubcopilot.com" | ||
| ], | ||
| "zig": ["ziglang.org", "pkg.machengine.org", "deps.files.ghostty.org"] |
There was a problem hiding this comment.
Addressed in 03a3bcb: restored the legacy threat-detection ecosystem alias in the embedded JSON so network.allowed: [threat-detection] remains accepted and expands as before.
|
|
||
| func TestThreatDetectionDomains(t *testing.T) { | ||
| detectionDomains := getEcosystemDomains("threat-detection") | ||
| detectionDomains := GetEngineDefaultDomainSets()["threat-detection"] |
There was a problem hiding this comment.
Addressed in 03a3bcb: after preserving compatibility, the test now asserts network.allowed: [threat-detection] expands to the centralized Copilot detection domain set and stays in sync with the engine-default registry.
| // Engine default domain lists intentionally exclude package registries (npm, PyPI, | ||
| // and similar). See the package-registry invariant above. |
There was a problem hiding this comment.
Addressed in 03a3bcb: scoped the package-registry invariant to runtime engine defaults and documented the Copilot threat-detection exception for registry.npmjs.org.
| Each engine automatically receives the domain set it requires in addition to | ||
| `network.allowed`. These named sets are maintained by the compiler for analysis | ||
| and reporting; they are not valid `network.allowed` identifiers. |
There was a problem hiding this comment.
Addressed in 03a3bcb: scoped the docs to say the threat-detection set is applied automatically only to Copilot threat-detection runs; external Claude, Codex, Gemini, and other detection runs use their own engine defaults.
| Each engine automatically receives the domain set it requires in addition to | ||
| `network.allowed`. These named sets are maintained by the compiler for analysis | ||
| and reporting; they are not valid `network.allowed` identifiers. |
There was a problem hiding this comment.
Addressed in 03a3bcb: updated .github/aw/network.md with the same Copilot-only automatic threat-detection scoping and clarified other detection engines use their own defaults.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 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
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This refactor quietly changes the public network.allowed surface and the new docs overstate how threat-detection behaves. Those are compatibility/documentation regressions, not just internal cleanup.
Blocking themes
- Removing
threat-detectionfrom the selectable ecosystem map turns an acceptednetwork.allowedvalue into a compile-time error. - The new network reference tables describe
threat-detectionas an automatic engine set even though the code path here only applies that set to the Copilot detection helper. - The registry-level package-registry invariant comment is now false because this same registry intentionally includes
registry.npmjs.org.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 11 AIC · ⌖ 10.7 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Clean, well-executed refactor. The centralized engineDefaultDomainSets map eliminates duplication while the copy-on-read semantics (via copyEngineDefaultDomainSet) keep the internal map immutable from callers. The new test TestGetEngineDefaultDomainSets verifies both directions of isolation. Documentation updates in both network.md files are accurate and clear. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 17.8 AIC · ⌖ 8.97 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — requesting changes on 2 issues (existing Copilot comments also warrant attention).
📋 Key Themes & Highlights
Key Themes
- Mutable internal slice exposure (
domains.go:802):GetThreatDetectionAllowedDomainsaccesses the registry slice directly;copyEngineDefaultDomainSetshould be used here too for consistency and safety. - Docs/code gap (
network.md):pi-baseis registered, exported, and tested but absent from both reference doc tables.
Pre-existing comments (Copilot, not re-raised here)
Five earlier comments cover the breaking removal of threat-detection from ecosystem JSON, the missing negative test, the incorrect package-registry invariant comment, and the misrepresentation of threat-detection scope in docs — those are the higher-priority items.
Positive Highlights
- ✅ Clean centralization: single source of truth for all engine allow-lists
- ✅ Defensive copies via
copyEngineDefaultDomainSetare applied consistently to the exported variables - ✅ Mutation-isolation test in
TestGetEngineDefaultDomainSetsis a solid regression guard - ✅ Documentation tables auto-generated from the registry — good principle
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 28.5 AIC · ⌖ 10 AIC · ⊞ 7.6K
Comment /matt to run again
| // excludes raw.githubusercontent.com (not needed when MCP servers are disabled and the CLI binary | ||
| // is pre-installed). npm registry access is read-only metadata lookup only — installs are not | ||
| // permitted during detection runs. | ||
| // Any additional user-specified network.allowed entries are merged in (typically empty for detection). |
There was a problem hiding this comment.
[/codebase-design] Direct map access hands the internal slice to mergeDomainsWithNetworkToolsAndRuntimes — if that function appends to the slice in a future grow-in-place scenario, it could silently corrupt the registry entry for subsequent calls.
💡 Suggested fix
Use the same defensive copy pattern used everywhere else in this PR:
detectionDomains := copyEngineDefaultDomainSet(engineDefaultDomainSets["threat-detection"])This is a one-liner and keeps the access pattern consistent with CopilotDefaultDomains, ClaudeDefaultDomains, etc.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 03a3bcb: GetThreatDetectionAllowedDomains now copies the threat-detection registry slice before merging additional network entries.
| | `copilot` | `api.github.com`, `api.githubcopilot.com`, `github.com`, `host.docker.internal`, `raw.githubusercontent.com` | | ||
| | `claude` | Anthropic APIs, GitHub transport, certificate/OCSP services, Ubuntu package metadata, Playwright downloads, and `host.docker.internal` | | ||
| | `codex` | `172.30.0.1`, `api.github.com`, `api.openai.com`, `chatgpt.com`, `github.com`, `host.docker.internal`, `openai.com` | | ||
| | `gemini` | `*.googleapis.com`, `generativelanguage.googleapis.com`, `github.com`, `host.docker.internal`, `raw.githubusercontent.com` | |
There was a problem hiding this comment.
[/grill-with-docs] The pi-base set exists in engineDefaultDomainSets and is exported via GetEngineDefaultDomainSets() / PiBaseDefaultDomains, but it is absent from both doc tables — leaving a gap between the code and the reference docs.
💡 Suggested addition
Add a row after the pi row in both docs/src/content/docs/reference/network.md and .github/aw/network.md:
| `pi-base` | `host.docker.internal`, `github.com`, `raw.githubusercontent.com`; applied as the provider-independent baseline before a provider prefix is resolved |@copilot please address this.
There was a problem hiding this comment.
Addressed in 03a3bcb: added the missing pi-base row to both network reference tables.
… sets Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
🏗️ ADR Required — Draft GeneratedStatus: A draft ADR has been committed to this branch at This PR adds 159 lines to What to do next
What the draft captured
Why ADRs matterArchitecture Decision Records capture why a decision was made — the problem, the alternatives considered, and the trade-offs accepted. They give future maintainers the context needed to evolve or reverse the decision safely.
|
|
@copilot refactor all domains information into a .json file that gets embedded in go. Synconce loaded. Remove hardcoded domains from .go files |
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32763356328
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in a099902: all workflow domain allow-lists, Pi provider targets, and sanitization defaults now live in the embedded JSON and are loaded once via |
|
/smoke-copilot |
|
/smoke-claude |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "clients2.google.com"
- "mtalk.google.com"See Network Configuration for more information.
|
|
Smoke Test: Copilot - 32766469873 PR: "Centralize engine default domain sets"
Overall: FAIL (1/16 failed) cc Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Me caveman review done. Smoke test only. Grunt approve.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comwww.google.comwww.gstatic.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · copilot · auto · 43.5 AIC · ⌖ 2.42 AIC · ⊞ 9.2K
Comment /smoke-copilot to run again
Add label smoke to run again
Comments that could not be inline-anchored
pkg/workflow/domains.go:1
Me caveman look here. Comment good place for smoke test.
pkg/workflow/domains_test.go:1
Me caveman check test file too. Grunt good.
🤖 Smoke Test Results — Run 32766957815Core Tests (#1–#12): PR Review Tests (#13–#19): Overall: PARTIAL (1 failed, 2 skipped) Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "clients2.google.com"
- "mtalk.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
clients2.google.commtalk.google.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "clients2.google.com"
- "mtalk.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · claude · sonnet46 · 78.1 AIC · ⌖ 8.81 AIC · ⊞ 7.1K
Comment /smoke-claude to run again
|
|
||
| var loadEcosystemDomains = sync.OnceValues(func() (map[string][]string, error) { | ||
| domainsLog.Print("Loading ecosystem domains from embedded JSON") | ||
| type domainSets struct { |
There was a problem hiding this comment.
Nice use of a typed struct domainSets to encapsulate all domain sets — this makes the JSON unmarshaling explicit and the fields self-documenting. Consider adding a validate() method to check for empty required fields at load time.
There was a problem hiding this comment.
No code change made for this optional follow-up: the current loader keeps JSON parsing centralized and the existing domain tests cover required registry contents/copy behavior. I kept this pass focused on the blocking maintainer-facing gaps.
|
|
||
| **Date**: 2026-08-24 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown |
There was a problem hiding this comment.
Status is Draft — please update to Accepted once the PR is approved and the implementation is verified to be complete and stable.
There was a problem hiding this comment.
Addressed in 5e44cf7: updated the ADR status to Accepted and aligned the decision/consequences text with the final embedded-JSON registry plus retained threat-detection compatibility alias.
|
Please do one focused cleanup pass:
I also attempted a branch refresh for this PR in this run. Run: https://github.com/github/gh-aw/actions/runs/32771472228
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed in |
|
🎉 This pull request is included in a new release. Release: |
Engine default allow-lists were distributed across static variables and the ecosystem JSON, making them difficult to inspect as a complete set and absent from network references.
Domain registry
engineDefaultDomainSets.GetEngineDefaultDomainSetsfor analysis/reporting.Threat detection
Documentation
✨ PR Review Safe Output Test - Run 32766957815
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
clients2.google.commtalk.google.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.