Review: human-authored PRs opened in past 48 hours (24 PRs)

[Copilot]

Reviewed 24 human-authored PRs opened in the past 48 hours. GitHub REST API and web UI are blocked by network proxy in this environment, so findings are documented here.

Critical Issues Found

PR #23220 — HA BGP down / config reload tests (aronovic)

• breakpoint() left in both new test files — will hang CI indefinitely
• Fixture typo: setup_dash_ha_from_jason (should be setup_dash_ha_from_json) → FixtureLookupError at runtime
• Import name mismatch: test imports ha_bgp_shutdown/ha_bgp_start; utils defines ha_shutdown_bgp/ha_start_bgp → ImportError
• Inverted BGP logic in _ha_bgp_oper: start=True runs config bgp shutdown, start=False runs config bgp start — callers get opposite behavior

PR #23221 — rsyslog fixture (lipxu)

• run_logrotate catches subprocess.CalledProcessError but duthost.shell() never raises it — dead code; subprocess import is unused

---

Notable Issues

PR	Issue
#23225	# noqa F811 (missing colon) on several lines; flake8 requires # noqa: F811
#23209	wait_for_rx_quiescence has no max timeout — infinite loop if traffic never drains
#23210	Route convergence wait can hit 11 min max; monit poll failure exits silently with no warning
#23204	type(...) is IPv4Network → use isinstance; verify "last IP" semantics are intentional
#23200	Loganalyzer regex has a leading space before ERR; swss\d* matches non-numeric suffixes — use swss[0-9]*
#23199	is_bmc() calls get_running_config_facts() every invocation — cache it; get_bmc_host() bypasses normal DUT initialization
#23214	Conflicts with #23219 (both touch test_liquid_cooling_leakage.py) — coordinate merge order
#23211	duthost.loganalyzer is non-standard; usual pattern is the loganalyzer fixture dict
#23207	vxlan_security="false" in test_dash_smartswitch_vnet.py vs "true" in conftest.py — add comment explaining the intentional difference
#23208	After config interface startup, no check that interface came back up before continuing

---

Minor / Suggestions

PR	Note
#23228	Add inline comment explaining why '' is preferred over None downstream
#23224	PR template checkboxes unpopulated; verbose 5-line comment could be condensed
#23202	BGP_ANNOUNCE_TIME = 3 fixed sleep — prefer wait_until with route-presence check; verify requests is available in test env
#23217	dut_basic_facts() called inline every time; cache if expensive
#23193	dict.fromkeys() dedup idiom — add a comment for clarity
#23195	SKU=bmc in devices CSV is a non-standard placeholder; console format 0.115200.0 is undocumented
#23201	Add comment explaining why enum_supervisor_dut_hostname was wrong here
#23219	pytest.fail() vs pytest.skip() when leak_sensors key missing from platform.json

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• https://api.github.com/graphql
  • Triggering command: /usr/bin/gh gh auth status (http block)
• https://api.github.com/user
  • Triggering command: /usr/bin/curl curl -s -H Authorization: token ****** REDACTED (http block)
  • Triggering command: /usr/bin/curl curl -v -H Authorization: token ****** REDACTED (http block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

---

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.