feat: add flags to store WAF log lines for failed tests - #626
Conversation
Fixed in a76dae6. The |
a76dae6 to
cf4861e
Compare
|
@copilot maybe adding a new flag instead, called |
Done in 28666e9. Added a new |
|
@copilot rebase |
|
@copilot fix failing tests |
|
@coderabbitai full_review |
Fixed in d445f16. The CI failure was caused by missing
The branch was already rebased/merged with main (commit |
|
@coderabbitai full_review |
|
✅ Actions performedFull review triggered. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a failure WAF logs feature with comprehensive error handling refactoring. It updates WAF log API methods to return errors instead of fatal-ing, refactors log assertion logic to propagate those errors, and implements conditional persistence of marked logs from failed test stages to a separate file via new CLI flags. ChangesFailure WAF Logs Implementation
Sequence Diagram(s)sequenceDiagram
participant FTWLogLines
participant AssertLogs as FTWCheck.AssertLogs
participant assertLogContains
participant Runner
Runner->>AssertLogs: call with error handling
AssertLogs->>assertLogContains: check log expectations
assertLogContains->>FTWLogLines: TriggeredRules()
alt Error from log operations
FTWLogLines-->>assertLogContains: (nil, error)
assertLogContains-->>AssertLogs: (false, error)
AssertLogs-->>Runner: (false, error)
else Success
FTWLogLines-->>assertLogContains: (ruleIds, nil)
assertLogContains-->>AssertLogs: (bool, nil)
AssertLogs-->>Runner: (bool, nil)
end
sequenceDiagram
participant CLI
participant buildRunner as buildRunnerConfig
participant Validator as Path Validator
participant Config as RunnerConfig
CLI->>buildRunner: --store-failure-waf-logs flags
buildRunner->>Validator: validate logs directory
alt Directory provided and exists
Validator-->>Config: use provided directory
else Directory not provided
Validator-->>Config: default to log file directory
else Directory does not exist
Validator-->>buildRunner: error
end
buildRunner->>Config: set StoreFailureLogs and FailureWafLogsFilePath
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@waflog/waflog.go`:
- Around line 84-87: Update the in-line comment above the os.OpenFile call that
currently references the wrong CLI flag; change the text mentioning
"--show-failures-only" to the correct flag name "--log-failures-only" so the
comment near ll.logFile, ll.logFilePath and the os.OpenFile call accurately
documents the behavior when the log file is truncated after each test.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65035d17-6cc9-4002-99be-079e437da444
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
cmd/run/run.goconfig/runner_config.gorunner/run.gorunner/run_test.gorunner/testdata/TestLogFailuresOnly.yamlrunner/types.gowaflog/waflog.gowaflog/waflog_test.go
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
|
Note: I'm rewriting this PR to use two separate flags. |
7852736 to
872b13e
Compare
|
@coderabbitai full_review |
|
✅ Actions performedFull review triggered. ✅ Actions performedFull review triggered. |
theseion
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
waflog/read.go (1)
175-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate I/O failures from log scanning instead of silently returning partial results.
GetMarkedLines()now has an error return, butbasicGetMarkedLines()still swallowsStat()/ scanner errors. This can turn operational failures into false-negative assertions.Suggested fix
-func (ll *FTWLogLines) basicGetMarkedLines() [][]byte { +func (ll *FTWLogLines) basicGetMarkedLines() ([][]byte, error) { fileInfo, err := ll.logFile.Stat() if err != nil { - log.Error().Caller().Msg("cannot read file's size") - return ll.markedLines + return nil, fmt.Errorf("cannot read log file size: %w", err) } ... for { line, _, err := scanner.LineBytes() if err != nil { if err != io.EOF { - log.Trace().Err(err) + return nil, fmt.Errorf("failed while scanning log lines: %w", err) } break } ... } slices.Reverse(ll.markedLines) - return ll.markedLines + return ll.markedLines, nil }- return ll.basicGetMarkedLines(), nil + lines, err := ll.basicGetMarkedLines() + if err != nil { + return nil, err + } + ll.markedLines = lines + ll.markedLinesInitialized = true + return ll.markedLines, nilAlso applies to: 195-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@waflog/read.go` around lines 175 - 183, basicGetMarkedLines currently swallows I/O errors (e.g., from ll.logFile.Stat and scanner errors) even though GetMarkedLines now returns an error; modify basicGetMarkedLines (FTWLogLines.basicGetMarkedLines) to return ([][]byte, error) and return any Stat()/scanner errors instead of logging and returning partial ll.markedLines, then update GetMarkedLines to call the new signature and propagate the error up; apply the same change to the similar block referenced at lines 195-201 so all scanning/Stat failures are returned rather than silently ignored.cmd/run/run.go (1)
220-277:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce
--failure-waf-logs-fileas a file name (not a path).This flag is documented as a file name, but path fragments are currently accepted. That can unintentionally bypass the selected failure-log directory.
Suggested patch
storeFailureWafLogsFileName, err := cmd.Flags().GetString(failureWafLogsFileNameFlag) if err != nil { return nil, err } + if filepath.Base(storeFailureWafLogsFileName) != storeFailureWafLogsFileName { + return nil, fmt.Errorf("argument to %s must be a file name, got: '%s'", failureWafLogsFileNameFlag, storeFailureWafLogsFileName) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/run/run.go` around lines 220 - 277, The flag value in storeFailureWafLogsFileName must be enforced as a file name (no path fragments): detect when the provided value contains path separators or differs from filepath.Base and return an error, or alternatively sanitize by using filepath.Base and reject empty names; update the logic where storeFailureWafLogsFileName is read (the block that sets runnerConfig.FailureWafLogsFilePath and checks failureWafLogsDirPath) to validate that storeFailureWafLogsFileName is non-empty and contains no directory components (compare to filepath.Base or check for os.PathSeparator/'/'), returning a clear error mentioning the --failure-waf-logs-file flag if validation fails before joining it with failureWafLogsDirPath to set runnerConfig.FailureWafLogsFilePath.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/run/run_test.go`:
- Around line 491-497: The test TestFailureWafLogsDirFlag_DirectoryDoesNotExist
is asserting on platform-specific error text via s.ErrorContains(err, "no such
file or directory"); change this to a platform-agnostic existence check by
asserting the error indicates a missing file/directory (e.g., use
os.IsNotExist(err) or errors.Is(err, fs.ErrNotExist)) after calling
buildRunnerConfig, replacing the s.ErrorContains call so the test passes on
Windows and Unix.
In `@cmd/run/run.go`:
- Around line 271-273: The error message formatting for the non-directory check
is malformed: update the fmt.Errorf call that uses failureWafLogsDirFlag and
failureWafLogsDirPath so the quoted path is properly closed and the format
string is correct (e.g., ensure the '%s' placeholder is terminated and the
trailing single quote is present). Locate the return statement that currently
reads fmt.Errorf("argument to %s is not a directory: '%s",
failureWafLogsDirFlag, failureWafLogsDirPath) and fix the format string to
include the missing closing quote and matching verbs.
In `@runner/run.go`:
- Around line 215-221: The current block checks notRunningInCloudMode(ftwCheck)
&& runContext.StoreFailureWafLogs and appends failed-stage WAF log lines via
appendFailureWafLogs when testResult == Failed, but never truncates the WAF log
after stage completion so non-failing-stage entries persist; update the logic to
truncate/clear the WAF log after each stage when StoreFailureWafLogs mode is
active — either add a call to the existing truncation helper (e.g.
truncateWafLog or clearWafLog) immediately after the stage finishes (both on
success and after handling failure) or modify appendFailureWafLogs to append the
failure lines and then truncate the live WAF log; ensure you reference
notRunningInCloudMode(ftwCheck), runContext.StoreFailureWafLogs,
testResult/Failed and appendFailureWafLogs when making the change.
In `@waflog/read.go`:
- Around line 38-47: The initialization flag triggeredRulesInitialized is being
set before the read/validation completes in FTWLogLines.TriggeredRules (and in
the analogous block flagged elsewhere), which can cache a failed/empty result;
change the code to only set ll.triggeredRulesInitialized = true after
GetMarkedLines() returns successfully and after you populate/validate
ll.triggeredRules, so that on error the flag remains false and subsequent calls
retry; apply the same change to the other method that currently sets its
initialization flag before completing its read/validation.
---
Outside diff comments:
In `@cmd/run/run.go`:
- Around line 220-277: The flag value in storeFailureWafLogsFileName must be
enforced as a file name (no path fragments): detect when the provided value
contains path separators or differs from filepath.Base and return an error, or
alternatively sanitize by using filepath.Base and reject empty names; update the
logic where storeFailureWafLogsFileName is read (the block that sets
runnerConfig.FailureWafLogsFilePath and checks failureWafLogsDirPath) to
validate that storeFailureWafLogsFileName is non-empty and contains no directory
components (compare to filepath.Base or check for os.PathSeparator/'/'),
returning a clear error mentioning the --failure-waf-logs-file flag if
validation fails before joining it with failureWafLogsDirPath to set
runnerConfig.FailureWafLogsFilePath.
In `@waflog/read.go`:
- Around line 175-183: basicGetMarkedLines currently swallows I/O errors (e.g.,
from ll.logFile.Stat and scanner errors) even though GetMarkedLines now returns
an error; modify basicGetMarkedLines (FTWLogLines.basicGetMarkedLines) to return
([][]byte, error) and return any Stat()/scanner errors instead of logging and
returning partial ll.markedLines, then update GetMarkedLines to call the new
signature and propagate the error up; apply the same change to the similar block
referenced at lines 195-201 so all scanning/Stat failures are returned rather
than silently ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b67b621-c1a9-4311-a6aa-eacd90a404a1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
cmd/run/run.gocmd/run/run_test.goconfig/runner_config.gorunner/check_base.gorunner/check_base_test.gorunner/check_error_test.gorunner/check_logs.gorunner/check_logs_test.gorunner/check_response_test.gorunner/run.gorunner/run_test.gorunner/testdata/TestStoreFailureWafLogs.yamlrunner/testdata/TestStoreFailureWafLogs_Disabled.yamlrunner/types.gowaflog/read.gowaflog/read_test.gowaflog/waflog_test.go
872b13e to
32f3ebf
Compare
|
@fzipi ready for review |
fzipi
left a comment
fzipi
left a comment
There was a problem hiding this comment.
Just a nitpick, feel free to merge.
- adds new flags: - `--store-failure-waf-logs` - `--failure-waf-logs-file` - `--failure-waf-logs-dir` These flags allow users to store the log lines from the WAF log in a dedicated file to help with debugging, e.g., in CI/CD.
32f3ebf to
f052368
Compare
--store-failure-waf-logs--failure-waf-logs-file--failure-waf-logs-dirThese flags allow users to store the log lines from the WAF log in a
dedicated file to help with debugging, e.g., in CI/CD.
Summary by CodeRabbit
Release Notes
New Features
--store-failure-waf-logsflag to capture WAF logs separately for failed tests--failure-waf-logs-fileand--failure-waf-logs-dirflags to configure failure log output locationBug Fixes