fix: correct Squid access.log path and add runtime presence detection - #49557
Conversation
- daily-observability-report.md: fix canonical path from sandbox/firewall/logs/access.log to sandbox/firewall/logs/squid-logs/access.log (current AWF layout), keep legacy path as fallback, and replace the native Squid format description with the AWF custom 10-field format - generate_observability_summary.cjs: add squidAccessLogPresent field (checks both current squid-logs/ and legacy path) and emit a warning line in the step summary when the access.log is missing on a firewall-enabled run - generate_observability_summary.test.cjs: add 4 tests covering present (squid-logs/ path), present (legacy path), missing, and firewall- disabled cases - print_firewall_logs.sh: after chown/chmod, probe both access.log paths and emit a stderr WARNING when neither is found so operators can spot the gap directly in the job log Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR TriageCategory: bug · Risk: low · Priority: medium · Score: 47/100 (impact 20, urgency 15, quality 12) Recommended action: Fixes a false-CRITICAL observability alert (wrong Squid log path). Draft, no CI/reviews yet — will fast-track once undrafted given real signal-quality impact.
|
PR Triage
OTel/observability-report path bug causing false CRITICAL alerts; draft, needs undraft. OTEL-FLAGGED.
|
|
cc
|
There was a problem hiding this comment.
Pull request overview
Corrects Squid access-log discovery and adds runtime visibility for missing firewall logs.
Changes:
- Updates current and legacy log paths and AWF format guidance.
- Adds step-summary presence reporting and tests.
- Emits job-log warnings when access logs are absent.
Show a summary per file
| File | Description |
|---|---|
actions/setup/sh/print_firewall_logs.sh |
Adds missing-log warning. |
actions/setup/js/generate_observability_summary.cjs |
Reports access-log presence. |
actions/setup/js/generate_observability_summary.test.cjs |
Tests summary behavior. |
.github/workflows/daily-observability-report.md |
Corrects path and format guidance. |
.github/workflows/daily-observability-report.lock.yml |
Recompiles workflow metadata. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
| for candidate in \ | ||
| "${AWF_LOGS_DIR}/squid-logs/access.log" \ | ||
| "${AWF_LOGS_DIR}/access.log"; do | ||
| if [[ -f "${candidate}" ]]; then |
| const OTLP_EXPORT_ERROR_DETAILS_PATH = "/tmp/gh-aw/otlp-export-errors.jsonl"; | ||
| const gatewayEventPaths = ["/tmp/gh-aw/mcp-logs/gateway.jsonl", "/tmp/gh-aw/mcp-logs/rpc-messages.jsonl"]; | ||
| // Squid access log paths: current AWF layout (squid-logs/ subdirectory) and legacy layout (directly under logs/). | ||
| const squidAccessLogPaths = ["/tmp/gh-aw/sandbox/firewall/logs/squid-logs/access.log", "/tmp/gh-aw/sandbox/firewall/logs/access.log"]; |
| staged: awInfo.staged === true, | ||
| firewallEnabled: awInfo.firewall_enabled === true, | ||
| firewallEnabled, | ||
| squidAccessLogPresent: firewallEnabled ? checkSquidAccessLogPresent() : null, |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-tested. One important correctness issue remains: the hardcoded /tmp/gh-aw base path in the JS summary script won't work on ARC/DinD runners.
📋 Key Themes & Highlights
Key Themes
- Hardcoded
/tmp/gh-awbase path (generate_observability_summary.cjsline 12):squidAccessLogPresentwill silently returnfalseon ARC runners where logs live underRUNNER_TEMP/gh-aw/. This is the highest-risk issue — mirrors comments already posted by a prior review pass. - Shell test coverage gap (
print_firewall_logs.sh): the new warning branch has no shell test assertions (also called out in existing comments). - Early-return coverage (
generate_observability_summary.cjsline 138):squidAccessLogPresentis only emitted when OTLP is enabled, so non-OTLP firewall runs won't surface the warning (also flagged in existing comments).
Positive Highlights
- ✅ Good dual-path fallback logic (current + legacy) in both JS and shell
- ✅ Test suite covers all four key scenarios: current path, legacy path, missing, firewall-disabled
- ✅ Warning written to stderr in the shell script — operators see it without grepping the step summary
- ✅
squidAccessLogPresent: nullwhen firewall is disabled — clean sentinel avoids false positives
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.5 AIC · ⌖ 8.05 AIC · ⊞ 7.1K
Comment /matt to run again
| const OTLP_EXPORT_ERROR_DETAILS_PATH = "/tmp/gh-aw/otlp-export-errors.jsonl"; | ||
| const gatewayEventPaths = ["/tmp/gh-aw/mcp-logs/gateway.jsonl", "/tmp/gh-aw/mcp-logs/rpc-messages.jsonl"]; | ||
| // Squid access log paths: current AWF layout (squid-logs/ subdirectory) and legacy layout (directly under logs/). | ||
| const squidAccessLogPaths = ["/tmp/gh-aw/sandbox/firewall/logs/squid-logs/access.log", "/tmp/gh-aw/sandbox/firewall/logs/access.log"]; |
There was a problem hiding this comment.
[/diagnosing-bugs] The hardcoded /tmp/gh-aw base path will silently return false on ARC/DinD runners where the Go compiler writes logs under RUNNER_TEMP/gh-aw/ — squidAccessLogPresent will always be wrong there even when the log file exists. This is the same root cause as the existing comment at this line but the fix direction is worth making explicit.
💡 Suggested approach
Mirror how print_firewall_logs.sh already derives the path: the compiler exports AWF_LOGS_DIR; the JS side can use RUNNER_TEMP to construct the same base, or a new GH_AW_BASE_DIR env var:
const GH_AW_BASE = (process.env.RUNNER_TEMP && process.env.RUNNER_TEMP !== '')
? path.join(process.env.RUNNER_TEMP, 'gh-aw')
: '/tmp/gh-aw';
const squidAccessLogPaths = [
GH_AW_BASE + '/sandbox/firewall/logs/squid-logs/access.log',
GH_AW_BASE + '/sandbox/firewall/logs/access.log',
];Add a unit-test case that sets RUNNER_TEMP to confirm the path is derived dynamically.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (8 tests)
Verdict
|
There was a problem hiding this comment.
Review summary
The changes correctly fix the canonical Squid access.log path (squid-logs/ subdirectory) and add dual-path fallback detection in both the JS summary generator and the shell script. The test coverage added is solid.
Three existing comments (by Copilot) and one new comment (line 64 of print_firewall_logs.sh) flag non-blocking but actionable issues:
print_firewall_logs.sh— warning not surfaced in step summary (new, line 64): the>&2-only message is invisible in the GitHub step-summary panel. Should also write to$GITHUB_STEP_SUMMARY.generate_observability_summary.cjs— hardcoded/tmp/gh-awpath (existing, line 12): ARC/DinD runners use$RUNNER_TEMP/gh-aw/...instead.generate_observability_summary.cjs— field never emitted without OTLP (existing, line 138): the step-summary block is gated onisOTLPEnabled, sosquidAccessLogPresentmay not appear for non-OTLP runs.print_firewall_logs.sh— no shell test assertions (existing, line 60): new warning code path has no test coverage.
None of these are blocking the correctness of the core fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.1 AIC · ⌖ 16.8 AIC · ⊞ 5.4K
| ACCESS_LOG_FOUND=true | ||
| break | ||
| fi | ||
| done |
There was a problem hiding this comment.
The warning is written only to stderr (>&2) and is not appended to $GITHUB_STEP_SUMMARY. Unlike the AWF summary block above (which uses tee -a "${GITHUB_STEP_SUMMARY:-/dev/null}"), this warning will be invisible in the step-summary panel — easy to miss during triage.
Suggested fix:
if [[ "${ACCESS_LOG_FOUND}" == "false" ]]; then
msg="WARNING: Squid access.log not found under ${AWF_LOGS_DIR}; egress traffic for this run cannot be audited."
echo "${msg}" >&2
echo "${msg}" >> "${GITHUB_STEP_SUMMARY:-/dev/null}"
fi@copilot please address this.
There was a problem hiding this comment.
Verdict: Request changes — the new detection logic reintroduces the same class of false-positive bug it's meant to fix.
💡 Themes
- Path resolution bug:
generate_observability_summary.cjshardcodes/tmp/gh-aw/...for the squid log check, but the Go compiler redirects firewall logs to${{ runner.temp }}/gh-aw/...on ARC/DinD topologies. This means the new diagnostic will falsely report the access log missing on ARC/DinD even when it's present — the exact failure mode this PR set out to eliminate. - Coverage gap: The new step-summary field is gated behind
isOTLPEnabled(data)in the Go compiler, so firewall-only workflows without OTLP configured never get this diagnostic at all, silently limiting the fix's real-world impact. - Test asymmetry: The shell script (
print_firewall_logs.sh) got no test coverage for its new warning logic, unlike the well-tested.cjscounterpart, risking silent drift between the two parallel implementations.
The core .md path fix and format-spec correction look solid and well-reasoned. The remaining issues are in the two auxiliary detection scripts added alongside it.
🔎 Code quality review by PR Code Quality Reviewer · auto · 28.2 AIC · ⌖ 9.8 AIC · ⊞ 7.8K
Comment /review to run again
| const OTLP_EXPORT_ERROR_DETAILS_PATH = "/tmp/gh-aw/otlp-export-errors.jsonl"; | ||
| const gatewayEventPaths = ["/tmp/gh-aw/mcp-logs/gateway.jsonl", "/tmp/gh-aw/mcp-logs/rpc-messages.jsonl"]; | ||
| // Squid access log paths: current AWF layout (squid-logs/ subdirectory) and legacy layout (directly under logs/). | ||
| const squidAccessLogPaths = ["/tmp/gh-aw/sandbox/firewall/logs/squid-logs/access.log", "/tmp/gh-aw/sandbox/firewall/logs/access.log"]; |
There was a problem hiding this comment.
Hardcoded /tmp/gh-aw/... paths make squidAccessLogPresent always false on ARC/DinD runners, producing a false "log missing" warning on every such run.
💡 Details
The Go compiler deliberately redirects firewall logs to ${{ runner.temp }}/gh-aw/sandbox/firewall/logs on ARC/DinD topologies (see pkg/workflow/engine_firewall_support.go isArcDindTopology branches using constants.AWFProxyLogsDirExpr), because /tmp/gh-aw is not daemon-visible in that mode. print_firewall_logs.sh correctly receives this via the AWF_LOGS_DIR env var and checks the right location. This new .cjs script instead hardcodes /tmp/gh-aw/sandbox/firewall/logs/... with no environment override, so on ARC/DinD it will always report squidAccessLogPresent: false and emit the "cannot be audited" warning even when the log is present — exactly the false-positive class of bug this PR set out to fix.
const squidAccessLogPaths = [
path.join(process.env.GH_AW_FIREWALL_LOGS_DIR || "/tmp/gh-aw/sandbox/firewall/logs", "squid-logs", "access.log"),
path.join(process.env.GH_AW_FIREWALL_LOGS_DIR || "/tmp/gh-aw/sandbox/firewall/logs", "access.log"),
];The generator step would need to pass the same resolved directory as an env var, mirroring what generateFirewallLogParsingStep already does for the shell script.
| staged: awInfo.staged === true, | ||
| firewallEnabled: awInfo.firewall_enabled === true, | ||
| firewallEnabled, | ||
| squidAccessLogPresent: firewallEnabled ? checkSquidAccessLogPresent() : null, |
There was a problem hiding this comment.
This new step-summary field never fires for firewall-enabled workflows that lack OTLP, since generateObservabilitySummary bails out early unless isOTLPEnabled(data).
💡 Details
In pkg/workflow/compiler_yaml_ai_execution.go, generateObservabilitySummary returns immediately when !isOTLPEnabled(data), before this script is even wired into the workflow. Firewall can be enabled independently of OTLP tracing, so the majority of firewall-only workflows (no telemetry export configured) will never emit the squidAccessLogPresent line or the missing-log warning — silently defeating the stated goal of "surfaces a warning line when missing so operators see the gap without waiting for the daily report" for that common configuration.
// pkg/workflow/compiler_yaml_ai_execution.go
func (c *Compiler) generateObservabilitySummary(yaml *strings.Builder, data *WorkflowData) {
if !isOTLPEnabled(data) {
return // <- firewall-only workflows never reach the squid-log check below
}
...Either decouple the squid-log presence check into its own step gated only on isFirewallEnabled(data), or relax the early return so firewall-only runs still get this diagnostic.
| @@ -50,3 +50,18 @@ if command -v awf &> /dev/null; then | |||
| else | |||
| echo 'AWF binary not installed, skipping firewall log summary' | |||
There was a problem hiding this comment.
No test suite covers this new warning path, so a shell-syntax regression (e.g. quoting/array issues) would silently ship undetected.
💡 Details
print_firewall_logs.sh has no accompanying test file in this diff, unlike generate_observability_summary.cjs which got four new Jest cases for the equivalent logic. The two implementations of the same "check current path, then legacy path" logic can now drift silently — e.g. if AWFProxyLogsDir changes in Go but only the .cjs array is updated (which is already stale per the ARC/DinD hardcoded-path issue noted elsewhere), this shell script would keep checking the old paths without any test catching the mismatch.
Consider adding a bats/shellspec test (or a minimal inline smoke test invoked from CI) that stubs AWF_LOGS_DIR, creates files at the current path, the legacy path, and neither, and asserts the WARNING line is/isn't emitted on stderr — mirroring the JS test matrix already added.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 3d65be5. The CI failure was |
|
🎉 This pull request is included in a new release. Release: |
The daily observability report was checking
sandbox/firewall/logs/access.logbut current AWF layout writes tosandbox/firewall/logs/squid-logs/access.log— causing every recent run to be flagged CRITICAL for a "missing" file that was actually present. The format description also referenced native Squid format instead of the AWF custom format.Changes
daily-observability-report.md: Fix canonical path tosquid-logs/access.log; retain the legacy direct path as fallback (explains the 3/8 runs that previously passed); replace format spec with actual AWF 10-field format:generate_observability_summary.cjs: AddsquidAccessLogPresentfield (probes both currentsquid-logs/and legacy paths) emitted in the step summary when firewall is enabled; surfaces a warning line when missing so operators see the gap without waiting for the daily report.print_firewall_logs.sh: After the chown/chmod pass, probe both paths and emit a stderrWARNINGif neither exists — makes the failure visible directly in the job log.