Protect firewall aggregate data in usage artifacts#58257
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Trusted team contribution — Thanks for protecting the firewall aggregate contract! This PR comes from the Copilot coding agent, a trusted core team agent, so it follows the agentic development process outlined in CONTRIBUTING.md. The changes align with the project's focus on safe usage data collection and downstream reporting. The PR is focused, well-scoped, and includes the necessary changeset metadata. Ready for core team review and merge. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
🔵 Needs a closer look
The test does not exercise blocked-domain deduplication despite explicitly protecting that contract.
Pull request overview
Protects firewall aggregate fields in usage artifacts for downstream reporting.
Changes:
- Documents and tests domain sets and per-domain counts.
- Refreshes generated workflows to AWF v0.28.13.
- Adds patch release metadata.
File summaries
| File | Description |
|---|---|
docs/src/content/docs/reference/artifacts.md |
Documents the complete firewall payload. |
actions/setup/js/generate_usage_activity_summary.test.cjs |
Expands aggregate contract assertions. |
.github/workflows/smoke-crush.lock.yml |
Refreshes AWF pins. |
.github/workflows/smoke-ci.lock.yml |
Refreshes AWF pins. |
.github/workflows/notion-issue-summary.lock.yml |
Refreshes AWF pins. |
.github/workflows/hippo-embed.lock.yml |
Refreshes AWF pins. |
.github/workflows/firewall.lock.yml |
Refreshes AWF pins. |
.github/workflows/example-permissions-warning.lock.yml |
Refreshes AWF pins. |
.github/workflows/daily-arxiv-researcher.lock.yml |
Refreshes AWF pins and enables egress verification. |
.github/workflows/codex-github-remote-mcp-test.lock.yml |
Refreshes AWF pins. |
.changeset/patch-firewall-usage-aggregate-contract.md |
Records the patch release change. |
Review details
- Files reviewed: 20/301 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"
- "github.com"See Network Configuration for more information.
|
|
✅ 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 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
❌ 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. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch PR file list
|
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.
|
There was a problem hiding this comment.
Request changes
The firewall aggregate contract itself looks fine, but this branch also drops the executable bit from multiple shipped helper scripts under .github/skills/ and .github/graders/. That is a runtime regression for any workflow or skill launcher that executes those entrypoints directly.
Blocking themes
- Restoring the firewall domain-set and per-domain count data is good, but it is buried in a branch that also breaks existing script entrypoints.
- The permission regressions affect shared tooling paths, so the blast radius is much larger than the focused artifact-schema change in this PR.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 38.1 AIC · ⌖ 7.39 AIC · ⊞ 23.5K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — this PR only strengthens test coverage and documentation; no production logic in generate_usage_activity_summary.cjs changed (verified byte-identical against main).
📋 Analysis
What changed
actions/setup/js/generate_usage_activity_summary.test.cjs: extends the existingparseFirewallLogs"counts valid Squid access log entries correctly" test with a secondapi.github.com:443entry and switches assertions from loosetoContain/count checks to exacttoEqualchecks onallowed_domains,blocked_domains, and the newrequests_by_domainper-domain arity map. This is a good TDD move: it locks in domain uniqueness (repeated domain doesn't duplicate the array) and per-domain allowed/blocked counts as an explicit contract, closing a gap where only substring/containment was previously verified.docs/src/content/docs/reference/artifacts.md: documents theallowed_domains,blocked_domains, andrequests_by_domainfields in the examplefirewallpayload, matching the actual implementation ingenerate_usage_activity_summary.cjs(parseFirewallLogs) exactly.- Changeset correctly scoped as a
patchdescribing the contract protection.
Verification
- Diffed
actions/setup/js/generate_usage_activity_summary.cjsagainstmain(6807d79) — byte-identical. This is a pure test+docs PR, not a behavior change, so there's no regression risk in the aggregation logic itself. - The new assertions accurately reflect current behavior in
parseFirewallLogs(dedup viaSet, per-domain counters inrequests_by_domain), so the test won't be a false positive.
Nothing actionable found
No gaps in scope: the PR does exactly what its title/changeset promise (protect the existing firewall aggregate contract with explicit tests + docs) without overreaching into unrelated code. The bulk of the diff (.lock.yml files) is expected regeneration noise from gh aw compile / make recompile and out of scope for review.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
api.github.comgithub.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.github.com"
- "github.com"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 115.2 AIC · ⌖ 13.7 AIC · ⊞ 10.3K
Comment /matt to run again
The usage artifact must retain unique firewall domains, per-domain request arity, and allowed/blocked classifications for downstream reporting.
Contract coverage
Documentation
firewallpayload:{ "allowed_domains": ["api.github.com:443"], "blocked_domains": ["blocked.example.com:443"], "requests_by_domain": { "api.github.com:443": { "allowed": 10, "blocked": 0 }, "blocked.example.com:443": { "allowed": 0, "blocked": 2 } } }