Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix: correct Squid access.log path and add runtime presence detection #49557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
fix: correct Squid access.log path and add runtime presence detection #49557
Changes from all commits
7b8a46db0e97423fc686dFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/diagnosing-bugs] The hardcoded
/tmp/gh-awbase path will silently returnfalseon ARC/DinD runners where the Go compiler writes logs underRUNNER_TEMP/gh-aw/—squidAccessLogPresentwill 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.shalready derives the path: the compiler exportsAWF_LOGS_DIR; the JS side can useRUNNER_TEMPto construct the same base, or a newGH_AW_BASE_DIRenv var:Add a unit-test case that sets
RUNNER_TEMPto confirm the path is derived dynamically.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded
/tmp/gh-aw/...paths makesquidAccessLogPresentalways 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/logson ARC/DinD topologies (seepkg/workflow/engine_firewall_support.goisArcDindTopologybranches usingconstants.AWFProxyLogsDirExpr), because/tmp/gh-awis not daemon-visible in that mode.print_firewall_logs.shcorrectly receives this via theAWF_LOGS_DIRenv var and checks the right location. This new.cjsscript instead hardcodes/tmp/gh-aw/sandbox/firewall/logs/...with no environment override, so on ARC/DinD it will always reportsquidAccessLogPresent: falseand 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.The generator step would need to pass the same resolved directory as an env var, mirroring what
generateFirewallLogParsingStepalready does for the shell script.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new step-summary field never fires for firewall-enabled workflows that lack OTLP, since
generateObservabilitySummarybails out early unlessisOTLPEnabled(data).💡 Details
In
pkg/workflow/compiler_yaml_ai_execution.go,generateObservabilitySummaryreturns 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 thesquidAccessLogPresentline 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.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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.shhas no accompanying test file in this diff, unlikegenerate_observability_summary.cjswhich 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. ifAWFProxyLogsDirchanges in Go but only the.cjsarray 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning is written only to
stderr(>&2) and is not appended to$GITHUB_STEP_SUMMARY. Unlike the AWF summary block above (which usestee -a "${GITHUB_STEP_SUMMARY:-/dev/null}"), this warning will be invisible in the step-summary panel — easy to miss during triage.Suggested fix:
@copilot please address this.