fix: suppress internal AWF sidecar hostnames from blocked-domain warnings - #48591
Conversation
…ings The MCP Gateway sidecar (awmg-mcpg) and CLI proxy sidecar (awmg-cli-proxy) are framework-managed containers attached to awf-net via network.topologyAttach. Their hostnames appeared in firewall logs as "blocked" because they are not in network.allowDomains, causing a spurious warning on every issue/PR created by safe-outputs. Filter these known internal sidecar hostnames from: - getBlockedDomains() in firewall_blocked_domains.cjs (the PR/issue warning) - analyzeFirewallLogLines() in parse_firewall_logs.cjs (the step summary) Closes #48038 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ 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.
Pull request overview
Suppresses framework-managed AWF sidecars from firewall warnings and summaries.
Changes:
- Adds internal-sidecar filtering to firewall log processing.
- Adds coverage for both known sidecar hostnames.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/parse_firewall_logs.cjs |
Filters sidecars from blocked statistics. |
actions/setup/js/parse_firewall_logs.test.cjs |
Tests sidecar detection and counting. |
actions/setup/js/firewall_blocked_domains.cjs |
Suppresses sanitized sidecar domains. |
actions/setup/js/firewall_blocked_domains.test.cjs |
Tests warning suppression. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
| if (!isInternalSidecarHost(domainKey)) { | ||
| blockedRequests++; | ||
| blockedDomains.add(domainKey); | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business logic directories: src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: suppress internal AWF sidecar hostnames
The overall approach is correct and well-tested. One blocking issue found.
Blocking: requestsByDomain tracking in parse_firewall_logs.cjs is not gated on isInternalSidecarHost, so sidecar hosts (awmg-mcpg:8080, awmg-cli-proxy:3128) still appear in the generateFirewallSummary step-summary table with non-zero blocked counts. The issue/PR body warning is suppressed by firewall_blocked_domains.cjs, but the Actions step summary will still show them. See inline comment for the fix.
Everything else looks good: the constant lists are consistent across both files, the sanitized-form pre-computation is correct, isInternalSidecarHost handles port stripping correctly, and the test coverage is thorough.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.8 AIC · ⌖ 4.71 AIC · ⊞ 5K
Comments that could not be inline-anchored
actions/setup/js/parse_firewall_logs.cjs:221
Bug: sidecar hosts still appear in the step-summary table.
requestsByDomain is populated unconditionally for every entry (lines 220-229), including the sidecar hostnames excluded from blockedRequests. So generateFirewallSummary will still list awmg-mcpg:8080 and awmg-cli-proxy:3128 in the domain table with their blocked counts — the fix is incomplete for the step summary.
Suggested fix — guard the requestsByDomain tracking block with the same sidecar check:
// Track req…
</details>
🧪 Test Quality Sentinel Report
📊 Metrics (11 tests)
📋 Test Breakdownfirewall_blocked_domains.test.cjs (3 tests, +64 lines):
parse_firewall_logs.test.cjs (8 tests, +48 lines):
|
| File | Test Lines Added | Production Lines | Ratio |
|---|---|---|---|
| firewall_blocked_domains.cjs | 64 | 13 | 4.92:1 |
| parse_firewall_logs.cjs | 48 | 26 | 1.85:1 |
| Combined | 112 | 39 | 2.87:1 |
Justification: The production changes are small but require comprehensive filtering coverage. All 11 tests verify design invariants (no implementation tests). Edge cases for safety-critical firewall filtering are justified.
Verdict
✅ Passed. 0% implementation tests (threshold: 30%). All tests verify design contracts — no violations. Test inflation ratio 2.87:1 exceeds 2:1 guideline but is justified by comprehensive safety-critical filtering coverage and zero implementation tests.
🧪 Test quality analysis by Test Quality Sentinel · haiku45 · 21 AIC · ⌖ 8.91 AIC · ⊞ 8.1K · ◷
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two correctness issues in parse_firewall_logs.cjs.
📋 Key Themes & Highlights
Key Issues
-
Incomplete suppression in step summary (
parse_firewall_logs.cjsL214, L220): Sidecar requests still incrementtotalRequestsand populaterequestsByDomain, so the collapsed domain table in the step summary will show the sidecar with a non-zeroblockedcount. The fix is only half-applied. -
totalRequestsimbalance (parse_firewall_logs.cjsL214): After the fix,totalRequests ≠ allowedRequests + blockedRequestsfor sidecar traffic. The test suite does not catch this because it never asserts the balance invariant. -
Duplicated constant (
firewall_blocked_domains.cjsL21):AWF_INTERNAL_SIDECAR_HOSTSis defined in two files. A shared module would prevent drift.
Positive Highlights
- ✅ Correct fix in
firewall_blocked_domains.cjs— the issue/PR warning path is cleanly addressed - ✅ Good use of pre-computed
Setfor O(1) lookups - ✅
isInternalSidecarHost()is a well-named, well-tested helper - ✅ Regression tests cover both sidecars and the all-sidecar edge case
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.2 AIC · ⌖ 5.08 AIC · ⊞ 6.7K
Comment /matt to run again
| // Skip internal sidecar hostnames (awmg-mcpg, awmg-cli-proxy) from the | ||
| // blocked domain set. These are framework-managed topology-attach containers | ||
| // and are not user-actionable external blocked domains. | ||
| if (!isInternalSidecarHost(domainKey)) { |
There was a problem hiding this comment.
[/diagnosing-bugs] Sidecar requests increment totalRequests but are silently dropped from blockedRequests — the step-summary header will read "X requests | Y allowed | Z blocked" where X ≠ Y+Z for sidecar traffic, creating a confusing discrepancy.
💡 Suggested fix
Either also exclude sidecar requests from totalRequests (skip the totalRequests++ at line 197 when isInternalSidecarHost), or add a dedicated internalRequests counter and note them in the summary. The current approach silently "loses" requests in the totals.
// Option A: exclude sidecar entries entirely from request counting
if (isInternalSidecarHost(domainKey)) {
continue; // skip totalRequests++ too
}
totalRequests++;@copilot please address this.
| } | ||
| } | ||
|
|
||
| // Track request count per domain |
There was a problem hiding this comment.
[/diagnosing-bugs] Sidecar entries are also tracked in requestsByDomain, so they will appear in the per-domain table of the step summary with non-zero blocked counts — directly contradicting the goal of hiding them from users.
💡 Suggested fix
Skip requestsByDomain tracking for internal sidecars, or filter them out in generateFirewallSummary alongside error: entries:
const validDomains = Array.from(requestsByDomain.keys())
.filter(domain => domain !== "-" && !domain.startsWith("error:") && !isInternalSidecarHost(domain))
.sort();Without this, a user could still notice the sidecar in the collapsed domain table.
@copilot please address this.
| // gh-aw itself (e.g. the MCP Gateway and the CLI proxy). These are | ||
| // framework-managed, not user-controllable external domains, and must never | ||
| // surface in the "blocked domains" warning shown on issues/PRs. | ||
| const AWF_INTERNAL_SIDECAR_HOSTS = ["awmg-mcpg", "awmg-cli-proxy"]; |
There was a problem hiding this comment.
[/codebase-design] AWF_INTERNAL_SIDECAR_HOSTS is duplicated across firewall_blocked_domains.cjs and parse_firewall_logs.cjs — a future sidecar addition requires updating two files. Since both modules already require from the same directory, this list could live in a shared constant module (e.g. awf_internal_hosts.cjs) and be imported by both.
💡 Example
// awf_internal_hosts.cjs
const AWF_INTERNAL_SIDECAR_HOSTS = ["awmg-mcpg", "awmg-cli-proxy"];
module.exports = { AWF_INTERNAL_SIDECAR_HOSTS };This is a minor maintainability concern, not blocking, but worth addressing before the list grows.
@copilot please address this.
| const result = analyzeFirewallLogLines(lines); | ||
| (expect(result.totalRequests).toBe(2), expect(result.blockedRequests).toBe(0), expect(result.allowedRequests).toBe(1), expect(result.blockedDomains.size).toBe(0)); | ||
| })); | ||
| })); |
There was a problem hiding this comment.
[/tdd] The analyzeFirewallLogLines sidecar tests do not verify that totalRequests is still counted correctly when sidecars are present. The current test only asserts blockedRequests === 0 and blockedDomains.size === 0, but leaves the totalRequests vs allowedRequests + blockedRequests imbalance untested — so the discrepancy noted at line 214 of the implementation is invisible in the test suite.
💡 Suggested assertion
// In the "should report zero blocked requests" test:
expect(result.totalRequests).toBe(2); // already present
expect(result.allowedRequests + result.blockedRequests).toBe(result.totalRequests); // this will FAIL todayAdding this assertion turns the coverage gap into a red test that forces the fix.
@copilot please address this.
There was a problem hiding this comment.
Verdict: Request changes
Sidecar-host filtering is incomplete — the rendered step-summary table (generateFirewallSummary) still surfaces awmg-mcpg/awmg-cli-proxy rows, so the noise this PR aims to eliminate persists in one of the two user-facing surfaces. A shared-constant duplication issue also increases future drift risk.
Themes
- Correctness gap:
isInternalSidecarHostonly guards theblockedRequests/blockedDomainscounters, notrequestsByDomain, so the per-domain markdown table in the Actions step summary still lists sidecar hosts — the PR only half-fixes the reported spam (issue-body warning is fixed viafirewall_blocked_domains.cjs, but the step-summary table inparse_firewall_logs.cjsis not). - Maintainability: the sidecar hostname list is copy-pasted across two
.cjsfiles (and must additionally track a third source of truth in the Go compiler), with no mechanism to catch drift if names ever change. - Test coverage is decent for the counting logic but does not assert on the actual rendered summary output, which is how the table-row gap went undetected.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 36.3 AIC · ⌖ 4.55 AIC · ⊞ 7.4K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/parse_firewall_logs.cjs:245
Internal sidecar hosts still appear as rows in the rendered step-summary table, defeating the stated purpose of this PR.
<details>
<summary>💡 Fix and rationale</summary>
isInternalSidecarHost is applied only inside the blockedRequests/blockedDomains accumulation branch (lines ~210-218), but requestsByDomain is populated unconditionally right after (line 221-229) and generateFirewallSummary builds its markdown table straight from requestsByDomain (line 245+) without any sidecar fi…
actions/setup/js/firewall_blocked_domains.cjs:21
The internal sidecar hostname list is duplicated verbatim across two files with no shared source of truth.
<details>
<summary>💡 Why this matters</summary>
AWF_INTERNAL_SIDECAR_HOSTS = ["awmg-mcpg", "awmg-cli-proxy"] here and the equivalent Set(["awmg-mcpg", "awmg-cli-proxy"]) in parse_firewall_logs.cjs (and the PR description explicitly says they must match buildAWFTopologyAttachList in the Go compiler) is now a triple-maintained list across two languages/files. If the Go compiler ev…
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot run pr-finisher skill |
|
@copilot run pr-finisher skill |
The review feedback has been addressed: Tests were updated to assert
|
|
@copilot run pr-finisher skill |
The requestsByDomain map was updated unconditionally for all entries, including blocked internal sidecar hosts (awmg-mcpg, awmg-cli-proxy). generateFirewallSummary() derives its domain table and blocked counts from that map, so sidecars were still appearing in the Actions step summary despite being excluded from blockedRequests/blockedDomains. Guard the requestsByDomain update so blocked internal sidecar entries are fully skipped, matching the same condition already applied to the blockedRequests/blockedDomains counters. Update tests to assert requestsByDomain exclusion for both known sidecar hostnames. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — |
|
@copilot avoid duplicated AWF_INTERNAL_SIDECAR_HOSTS |
|
Thanks for the fix @pelikhan !!! |
|
🎉 This pull request is included in a new release. Release: |
awmg-mcpg(MCP Gateway) andawmg-cli-proxyare framework-managed containers attached toawf-netvianetwork.topologyAttach. Because they are not innetwork.allowDomains, Squid logs them as blocked — producing a spurious⚠️ Firewall blocked 1 domain(s): awmgmcpgwarning on every issue/PR safe-outputs creates. The hostname is a bare label that the compiler rejects, so users have no way to suppress it.Changes
firewall_blocked_domains.cjs—getBlockedDomains()filters sanitized forms of known internal sidecar names (awmgmcpg,awmgcliproxy) before adding to the blocked set, eliminating the warning from issue/PR bodies.parse_firewall_logs.cjs—analyzeFirewallLogLines()skips internal sidecar hostnames when counting blocked requests in the Actions step summary. AddsisInternalSidecarHost(domainKey)helper (strips port, checks against the known set).Both files share the same constant list of internal sidecar names (
awmg-mcpg,awmg-cli-proxy), matching whatbuildAWFTopologyAttachListunconditionally emits in the Go compiler.