fix: find access.log in sandbox/firewall/logs/squid-logs/ subdirectory - #42513
Conversation
…subdirectory
AWF writes Squid access.log to {proxy-logs-dir}/squid-logs/access.log (a squid-logs/
subdirectory within --proxy-logs-dir), not directly at the top level. The analysis
and parsing code used a non-recursive *.log glob at sandbox/firewall/logs/ which
missed the subdirectory.
- analyzeFirewallLogs: check sandbox/firewall/logs/squid-logs/ first, then fall
back to sandbox/firewall/logs/ for older AWF layout
- parseFirewallLogs: add sandbox/firewall/logs/squid-logs/ as the primary path,
keeping legacy squid-logs/ and workflow-logs/squid-logs/ as fallbacks
- Add tests: TestAnalyzeFirewallLogsSandboxSquidSubdir and
TestAnalyzeFirewallLogsSandboxFallbackToTopLevel
Closes #42507
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. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42513 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (95 additions, threshold 100). |
There was a problem hiding this comment.
Pull request overview
This PR fixes firewall log discovery for firewall-enabled run bundles where Squid access.log is written under sandbox/firewall/logs/squid-logs/ (a subdirectory of the proxy logs dir), ensuring the analysis/parsing code can find and process it while retaining compatibility with older artifact layouts.
Changes:
- Update firewall log analysis to look in
sandbox/firewall/logs/squid-logs/before falling back tosandbox/firewall/logs/*.log. - Update firewall log parsing to prefer
sandbox/firewall/logs/squid-logs/ahead of other legacy locations. - Add tests covering the new subdirectory layout and the top-level sandbox fallback.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_parsing_firewall.go | Adds sandbox/firewall/logs/squid-logs/ as the primary lookup path for the JS-based firewall log parser. |
| pkg/cli/firewall_log.go | Updates analyzeFirewallLogs to check sandbox/firewall/logs/squid-logs/ before analyzing sandbox/firewall/logs/ directly. |
| pkg/cli/firewall_log_test.go | Adds tests for the new squid-logs/ subdirectory layout and the sandbox top-level fallback behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
| squidSubDir := filepath.Join(sandboxFirewallLogsDir, "squid-logs") | ||
| if fileutil.DirExists(squidSubDir) { | ||
| firewallLogLog.Printf("Found firewall logs directory: sandbox/firewall/logs/squid-logs") | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Found firewall logs directory: sandbox/firewall/logs/squid-logs")) | ||
| } | ||
| return analyzeMultipleFirewallLogs(squidSubDir, verbose) | ||
| } |
| if fileutil.DirExists(sandboxSquidLogsDir) { | ||
| logsDir = sandboxSquidLogsDir | ||
| logsParsingFirewallLog.Printf("Found firewall logs in sandbox/firewall/logs/squid-logs directory") | ||
| } else if fileutil.DirExists(squidLogsDir) { | ||
| logsDir = squidLogsDir |
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (2 tests analyzed)
Go: 2 ( Score breakdown: design ratio 40 + edge coverage 15 + no duplicates 20 + inflation penalty −10 = 75
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix for squid-logs/ subdirectory lookup
The fix is correct and well-targeted. AWF writes Squid access logs to {proxy-logs-dir}/squid-logs/access.log, and the old filepath.Glob("*.log") at the parent level never descended into that subdirectory. Both analyzeFirewallLogs and parseFirewallLogs now check sandbox/firewall/logs/squid-logs/ as the primary path, with proper fallbacks for older artifact layouts.
Correctness ✅
- Priority ordering is right: new subdirectory → legacy top-level → glob fallback.
- Both functions updated consistently.
Tests ✅
TestAnalyzeFirewallLogsSandboxSquidSubdir— exercises the new primary path end-to-end with 3 log entries (2 allowed, 1 blocked).TestAnalyzeFirewallLogsSandboxFallbackToTopLevel— confirms the top-level fallback still fires whensquid-logs/is absent.
Minor gap (non-blocking): logs_parsing_firewall.go has no test file at all; the new sandboxSquidLogsDir branch is untested in isolation. Pre-existing gap, not introduced here.
Edge case (non-blocking): If squid-logs/ exists but is empty, both functions return empty/nil rather than falling back to parent-level files — this is expected behavior.
Approved — the bug is fixed, logic is sound, and tests cover the important scenarios.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 36.3 AIC · ⌖ 9.31 AIC · ⊞ 4.9K
Skills Review Summary — PR #42513Applied 🔍 Findings (4 inline comments)
The most important item is the missing @copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on test coverage and fallback-chain consistency.
📋 Key Themes & Highlights
Key Themes
- Test gap in
parseFirewallLogs: The new primary lookup path inlogs_parsing_firewall.gohas no regression test, even though the parallel functionanalyzeFirewallLogswas fully tested. A bug could regress here invisibly. - Divergent fallback chains:
analyzeFirewallLogsandparseFirewallLogsnow follow different fallback orders after the primary path — this asymmetry is a latent source of confusion and inconsistency. - Path string duplication:
sandbox/firewall/logs/squid-logsis hardcoded in two separate files with no shared constant, increasing the risk of future drift. - Empty-directory edge case: No test documents what
analyzeFirewallLogsshould do whensquid-logs/exists but contains no log files.
Positive Highlights
- ✅ Root cause correctly identified and addressed: non-recursive glob replaced by an explicit subdirectory probe.
- ✅ Backward-compatible: older AWF artifact layouts still work via the fallback path.
- ✅ Two solid regression tests added for
analyzeFirewallLogscovering both the new and the legacy path. - ✅ Clear, informative comments throughout the changed code.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 53.4 AIC · ⌖ 7.92 AIC · ⊞ 6.5K
Comment /matt to run again
| var logsDir string | ||
| if fileutil.DirExists(squidLogsDir) { | ||
| if fileutil.DirExists(sandboxSquidLogsDir) { | ||
| logsDir = sandboxSquidLogsDir |
There was a problem hiding this comment.
[/tdd] The new sandboxSquidLogsDir primary path is not covered by any regression test — only analyzeFirewallLogs received new tests in firewall_log_test.go, leaving this code path unverified.
💡 Suggested test sketch
Add a TestParseFirewallLogsSandboxSquidSubdir analogous to TestAnalyzeFirewallLogsSandboxSquidSubdir, creating {tmpDir}/sandbox/firewall/logs/squid-logs/access.log and asserting that parseFirewallLogs completes without error. Without this, a regression here would be invisible.
@copilot please address this.
| if fileutil.DirExists(squidLogsDir) { | ||
| if fileutil.DirExists(sandboxSquidLogsDir) { | ||
| logsDir = sandboxSquidLogsDir | ||
| logsParsingFirewallLog.Printf("Found firewall logs in sandbox/firewall/logs/squid-logs directory") |
There was a problem hiding this comment.
[/diagnosing-bugs] analyzeFirewallLogs and parseFirewallLogs now have divergent fallback chains: analyzeFirewallLogs falls back to sandbox/firewall/logs/ (the parent dir, for *.log files), while parseFirewallLogs falls back to the bare squid-logs/ and workflow-logs/squid-logs/ paths. A run with an old-format artifact layout could succeed in one function and silently fail in the other.
💡 Suggested alignment
Consider adding sandbox/firewall/logs/ as a fallback in parseFirewallLogs to match analyzeFirewallLogs, and/or extract the ordered list of candidate paths into a shared helper so both functions stay in sync automatically:
func firewallLogCandidateDirs(runDir string) []string {
return []string{
filepath.Join(runDir, "sandbox", "firewall", "logs", "squid-logs"),
filepath.Join(runDir, "squid-logs"),
filepath.Join(runDir, "workflow-logs", "squid-logs"),
}
}@copilot please address this.
| sandboxFirewallLogsDir := filepath.Join(runDir, "sandbox", "firewall", "logs") | ||
| if fileutil.DirExists(sandboxFirewallLogsDir) { | ||
| // AWF writes the Squid access.log to a squid-logs/ subdirectory within --proxy-logs-dir. | ||
| // Check for that subdirectory first before falling back to the parent directory. |
There was a problem hiding this comment.
[/codebase-design] The string path sandbox/firewall/logs/squid-logs is now duplicated verbatim across firewall_log.go (here) and logs_parsing_firewall.go (line 44). If the artifact layout ever changes again, both sites need updating and it's easy to miss one.
💡 Suggested approach
Define a package-level constant or constructor to centralise the path:
// squidLogsSubDir is the subdirectory within the sandbox firewall logs dir
// where AWF writes the Squid access.log.
const squidLogsSubDir = "squid-logs"Or extract a helper function that returns the ordered candidate dirs (see also the comment in logs_parsing_firewall.go about divergent fallback chains).
@copilot please address this.
| firewallLogLog.Printf("Found firewall logs directory: sandbox/firewall/logs/squid-logs") | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Found firewall logs directory: sandbox/firewall/logs/squid-logs")) | ||
| } |
There was a problem hiding this comment.
[/tdd] No test covers the edge case where sandbox/firewall/logs/squid-logs/ exists but is empty (no *.log files inside). Depending on analyzeMultipleFirewallLogs's behaviour, the result may be nil, nil, causing silent success with no analysis — which could mask a real problem at runtime.
💡 Suggested test
Add TestAnalyzeFirewallLogsSandboxSquidSubdirEmpty that creates the directory without any log files and asserts either a non-nil error or a specific "no logs found" outcome, so the contract is explicit:
func TestAnalyzeFirewallLogsSandboxSquidSubdirEmpty(t *testing.T) {
tmpDir := testutil.TempDir(t, "test-squid-empty-*")
squidDir := filepath.Join(tmpDir, "sandbox", "firewall", "logs", "squid-logs")
os.MkdirAll(squidDir, 0755)
// no files written
analysis, err := analyzeFirewallLogs(tmpDir, false)
// assert expected behaviour: err != nil OR analysis.TotalRequests == 0
}@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — the primary fix (adding sandbox/firewall/logs/squid-logs/ as the primary Squid log path) is correct and well-motivated, but there is a correctness gap and missing test coverage that need to be addressed before merge.
Blocking issues
Behavioral divergence between analyzeFirewallLogs and parseFirewallLogs
analyzeFirewallLogs has a two-level probe for the sandbox case: it checks sandbox/firewall/logs/squid-logs/ first, then falls back to sandbox/firewall/logs/ directly when the subdirectory is absent (backward compat, proven by the new TestAnalyzeFirewallLogsSandboxFallbackToTopLevel test). parseFirewallLogs adds only the first level — sandbox/firewall/logs/squid-logs/ — and then jumps to squid-logs/ at the run-directory root. For the documented older-AWF layout the two functions now behave inconsistently: one processes the logs, the other silently skips them.
New parseFirewallLogs path has zero test coverage
The only test for parseFirewallLogs is TestParseFirewallLogsNoLogs. Neither the new primary path (sandbox/firewall/logs/squid-logs/) nor the if/else priority logic is exercised. A regression in path construction or priority order would be invisible.
🔎 Code quality review by PR Code Quality Reviewer · 80.7 AIC · ⌖ 7.36 AIC · ⊞ 1.6K
Comment /review to run again
| if fileutil.DirExists(sandboxSquidLogsDir) { | ||
| logsDir = sandboxSquidLogsDir | ||
| logsParsingFirewallLog.Printf("Found firewall logs in sandbox/firewall/logs/squid-logs directory") | ||
| } else if fileutil.DirExists(squidLogsDir) { |
There was a problem hiding this comment.
Missing sandbox/firewall/logs/ fallback causes silent data loss: when sandbox/firewall/logs/ exists but has no squid-logs/ subdir, this function skips straight to squid-logs/ at the run-directory root, silently dropping those logs.
Details and suggested fix
analyzeFirewallLogs in firewall_log.go (line 394) handles this case explicitly — it falls back to analyzeMultipleFirewallLogs(sandboxFirewallLogsDir, verbose) — and the new TestAnalyzeFirewallLogsSandboxFallbackToTopLevel test documents it as a supported backward-compat scenario.
But parseFirewallLogs has no equivalent fallback. Its if/else chain jumps from sandboxSquidLogsDir directly to squidLogsDir (at the run-directory root), skipping sandbox/firewall/logs/ entirely. On an older AWF layout where Squid logs land at sandbox/firewall/logs/access.log directly (no squid-logs/ subdir), analyzeFirewallLogs processes them correctly while parseFirewallLogs silently produces no output. The test TestAnalyzeFirewallLogsSandboxFallbackToTopLevel gives the impression this backward-compat path is fully covered, but it only covers analyzeFirewallLogs.
Suggested fix — add an intermediate fallback:
if fileutil.DirExists(sandboxSquidLogsDir) {
logsDir = sandboxSquidLogsDir
logsParsingFirewallLog.Printf("Found firewall logs in sandbox/firewall/logs/squid-logs directory")
} else if sandboxFirewallLogsDir := filepath.Join(runDir, "sandbox", "firewall", "logs"); fileutil.DirExists(sandboxFirewallLogsDir) {
// Older AWF layout: Squid logs written directly to --proxy-logs-dir (no squid-logs/ subdir)
logsDir = sandboxFirewallLogsDir
logsParsingFirewallLog.Printf("Found firewall logs in sandbox/firewall/logs directory")
} else if fileutil.DirExists(squidLogsDir) {| // Determine which directory to use | ||
| var logsDir string | ||
| if fileutil.DirExists(squidLogsDir) { | ||
| if fileutil.DirExists(sandboxSquidLogsDir) { |
There was a problem hiding this comment.
New primary path in parseFirewallLogs has zero test coverage: the sandboxSquidLogsDir branch added here is completely untested, leaving a regression risk in the exact path the PR is meant to fix.
Details
The two new tests (TestAnalyzeFirewallLogsSandboxSquidSubdir, TestAnalyzeFirewallLogsSandboxFallbackToTopLevel) only exercise analyzeFirewallLogs. The only existing test for parseFirewallLogs is TestParseFirewallLogsNoLogs (empty directory, no logs). Neither the new primary sandbox/firewall/logs/squid-logs/ path nor the directory-selection logic in this if/else chain is verified by any test.
A test covering directory resolution can be added without running the full Node.js parser — for example, by asserting that when sandbox/firewall/logs/squid-logs/ exists and contains a .log file, the function returns without error and produces a firewall.md output file (which the JS parser creates). This would catch regressions in path priority or filepath.Join construction.
| // Check for that subdirectory first before falling back to the parent directory. | ||
| squidSubDir := filepath.Join(sandboxFirewallLogsDir, "squid-logs") | ||
| if fileutil.DirExists(squidSubDir) { | ||
| firewallLogLog.Printf("Found firewall logs directory: sandbox/firewall/logs/squid-logs") |
There was a problem hiding this comment.
Hardcoded path literal in log message will mislead debugging: the message emits 'sandbox/firewall/logs/squid-logs' as a literal string rather than the resolved squidSubDir path, so if the run directory is not under CWD the logged location won't match where the file was actually found.
Suggested fix
Every other log site in this function uses the actual resolved path variable. Use squidSubDir here and on the existing fallback line below:
firewallLogLog.Printf("Found firewall logs directory: %s", squidSubDir)
if verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Found firewall logs directory: "+squidSubDir))
}The existing fallback at line 390-393 has the same issue with sandboxFirewallLogsDir.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 22/50 · Urgency 15/30 · Quality 12/20 Rationale: Fixes missing firewall log analysis — Squid writes to
|
|
🎉 This pull request is included in a new release. Release: |
Firewall-enabled run bundles were missing
access.logfrom analysis — AWF writes Squid logs to{proxy-logs-dir}/squid-logs/access.log(a subdirectory), but the analysis code used a non-recursivefilepath.Glob("*.log")at thesandbox/firewall/logs/level, which never descends intosquid-logs/.Changes
firewall_log.go—analyzeFirewallLogs: probesandbox/firewall/logs/squid-logs/before falling back tosandbox/firewall/logs/*.log(backward compat for older AWF builds that wrote logs at the top level)logs_parsing_firewall.go—parseFirewallLogs: addsandbox/firewall/logs/squid-logs/as the primary lookup path; existingsquid-logs/andworkflow-logs/squid-logs/remain as fallbacksfirewall_log_test.go: two new tests — one covering thesquid-logs/subdirectory path, one confirming the top-level fallback still works