-
Notifications
You must be signed in to change notification settings - Fork 563
support filtered regex #1978
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
support filtered regex #1978
Conversation
WalkthroughThis update introduces optional regex-based filtering to workflow stages and steps, enabling output redaction during plan execution. It adds Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config Loader
participant Validator as Config Validator
participant Executor as DiggerExecutor
participant TF as TerraformExecutor
participant Writer as FilteringWriter
Config->>Validator: Load and validate config (with FilterRegex)
Validator->>Validator: Compile FilterRegex (if present)
Validator-->>Config: Return error if invalid regex
Executor->>TF: Plan(params, envs, planFile, filterRegex)
TF->>Writer: Create FilteringWriter if filterRegex provided
TF->>TF: Run plan command, output goes through FilteringWriter
Writer-->>TF: Filtered output (redacted by regex)
TF-->>Executor: Plan results
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
PR Summary
Implements regex-based filtering capabilities across multiple components to allow selective output filtering in Terraform, OpenTofu, and Pulumi operations.
- Added new
FilteringWriter
inlibs/execution/io_writer_filtered.go
for pattern-based content redaction in execution outputs - Incomplete implementation in
libs/execution/pulumi.go
andlibs/execution/opentofu.go
where new filterRegex parameter is added but unused - Missing validation for regex patterns in
libs/digger_config/yaml.go
could lead to runtime errors - Inconsistent error handling in FilteringWriter ignores underlying Write call errors
- ToCoreStage() function in
libs/digger_config/yaml.go
needs updating to properly handle the new FilterRegex field
15 files reviewed, 6 comments
Edit PR Review Bot Settings | Greptile
libs/digger_config/config.go
Outdated
Value string | ||
ExtraArgs []string | ||
Shell string | ||
FilterRegex string |
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.
style: Consider making FilterRegex pointer type (*string) like in Stage for consistency in handling empty/nil cases
libs/execution/terragrunt.go
Outdated
@@ -49,7 +49,7 @@ func (terragrunt Terragrunt) Destroy(params []string, envs map[string]string) (s | |||
return stdout, stderr, err | |||
} | |||
|
|||
func (terragrunt Terragrunt) Plan(params []string, envs map[string]string, planArtefactFilePath string) (bool, string, string, error) { | |||
func (terragrunt Terragrunt) Plan(params []string, envs map[string]string, planArtefactFilePath string, s *string) (bool, string, string, error) { |
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.
logic: Parameter 's' is added but never used in the function implementation. This needs to be properly implemented if it's meant for regex filtering like other IaC tools in the PR.
libs/execution/io_writer_filtered.go
Outdated
if fw.writer != nil { | ||
fw.writer.Write(filtered) | ||
} |
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.
logic: Error from Write() call is ignored - this could mask important write failures. Consider returning error: if _, err := fw.writer.Write(filtered); err != nil { return len(p), err }
params = append(params, []string{"--save-plan", planArtefactFilePath}...) | ||
stdout, stderr, statusCode, err := pl.runPululmiCommand("preview", true, envs, params...) |
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.
style: Consider updating logs with 'planArtefactFilePath' to improve debug traceability
if workflow.Plan != nil && workflow.Plan.FilterRegex != nil { | ||
_, err := regexp.Compile(*workflow.Plan.FilterRegex) | ||
if err != nil { | ||
slog.Error("invalid regex for plan filter", | ||
"regex", *workflow.Plan.FilterRegex, | ||
"error", err) | ||
return fmt.Errorf("regex for plan filter is invalid: %v", err) | ||
} | ||
} | ||
if workflow.Apply != nil && workflow.Apply.FilterRegex != nil { | ||
_, err := regexp.Compile(*workflow.Apply.FilterRegex) | ||
if err != nil { | ||
slog.Error("invalid regex for apply filter", | ||
"regex", *workflow.Apply.FilterRegex, | ||
"error", err) | ||
return fmt.Errorf("regex for apply filter is invalid: %v", err) | ||
} | ||
} |
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.
style: identical validation code blocks for plan and apply. Consider extracting to helper function validateRegexPattern(pattern string, stage string) error to reduce duplication.
libs/execution/opentofu.go
Outdated
@@ -41,7 +41,7 @@ func (tf OpenTofu) Apply(params []string, plan *string, envs map[string]string) | |||
return stdout, stderr, err | |||
} | |||
|
|||
func (tf OpenTofu) Plan(params []string, envs map[string]string, planArtefactFilePath string) (bool, string, string, error) { | |||
func (tf OpenTofu) Plan(params []string, envs map[string]string, planArtefactFilePath string, s *string) (bool, string, string, error) { |
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.
logic: The new 's' parameter lacks documentation and isn't used in the method body. Either remove or properly implement.
func (tf OpenTofu) Plan(params []string, envs map[string]string, planArtefactFilePath string, s *string) (bool, string, string, error) { | |
func (tf OpenTofu) Plan(params []string, envs map[string]string, planArtefactFilePath string) (bool, string, string, error) { |
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.
Actionable comments posted: 4
🔭 Outside diff range comments (4)
libs/digger_config/yaml.go (1)
72-83
: 🛠️ Refactor suggestion
FilterRegex
lost during YAML→core conversion
StageYaml
now carriesFilterRegex
(l. 81) yetToCoreStage()
(l. 72-78) still returns
Stage{Steps: steps}
– the field is silently dropped. Down-stream code (e.g. scheduler & execution) expects it to be populated, so workflows defined only in YAML will never receive the regex.func (s *StageYaml) ToCoreStage() Stage { var steps []Step for _, step := range s.Steps { steps = append(steps, step.ToCoreStep()) } - return Stage{Steps: steps} + return Stage{ + Steps: steps, + FilterRegex: s.FilterRegex, + } }libs/execution/execution.go (1)
232-239
:⚠️ Potential issueNil-pointer dereference when default workflow is used
d.PlanStage
can benil
(see l. 210-221).
Dereferencing it here will crash:_, stdout, stderr, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), d.PlanStage.FilterRegex)Patch:
-_, stdout, stderr, err := d.TerraformExecutor.Plan( - planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), d.PlanStage.FilterRegex) +var filter *string +if d.PlanStage != nil { + filter = d.PlanStage.FilterRegex +} +_, stdout, stderr, err := d.TerraformExecutor.Plan( + planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), filter)libs/scheduler/json_models.go (1)
131-147
:⚠️ Potential issue
jsonToStage
dropsFilterRegex
whensteps
is emptyEarly-returning when no steps are present means a
StageJson
that only specifies afilter_regex
is silently ignored.
Example: a workflow can rely on default steps but just provide a redaction regex – that information is lost here.- if len(stageJson.Steps) == 0 { - return nil - } +if len(stageJson.Steps) == 0 && stageJson.FilterRegex == nil { + // nothing to convert + return nil +} // keep going – allow empty-step stage carrying a regexWithout this guard, config-to-scheduler conversion becomes asymmetric and plan filtering will never happen for such inputs.
🧰 Tools
🪛 golangci-lint (1.64.8)
137-137: S1016: should convert step (type StepJson) to Step instead of using struct literal
(gosimple)
libs/execution/terragrunt.go (1)
52-63
:⚠️ Potential issueTerragrunt
Plan
always returnstrue
regardless of exit codeMethod ignores
exitCode
and unconditionally returnstrue
forhasChanges
,
so callers cannot distinguish “no diff” from “changes present”.Either follow the Terraform/OpenTofu behaviour (
exitCode == 2
) or map
Terragrunt exit codes accordingly.- return true, stdout, stderr, err + return exitCode == 2, stdout, stderr, err
🧹 Nitpick comments (10)
libs/digger_config/config.go (2)
90-95
: UnusedStep.FilterRegex
field
Step
now embedsFilterRegex
(string) but:
StepYaml
has no corresponding field.- Conversion functions (
extract
,ToCoreStep
,copyStage
) never set it.- Execution layer does not consume it.
This bloats the API and misleads future contributors. Delete the field or finish the feature by wiring it end-to-end.
98-100
: Field order: favour logical grouping
Stage
listsSteps
beforeFilterRegex
; the YAML struct listsFilterRegex
first.
Not a blocker, but mirroring the ordering improves readability and diff hygiene across layers.libs/scheduler/json_models.go (1)
17-19
: JSON tag naming – confirm consistencyThis file uses
filter_regex
, while the YAML structs use the same snake_case field. Just double-check that any client code or API expectingfilterRegex
(camelCase) isn’t broken.
If the public contract is camelCase, align the tag now to avoid a breaking change later.libs/execution/tf_test.go (1)
23-24
: Add a positive test for the new filtering behaviourThe updated signature is exercised, but we never verify that the regex actually redacts output.
Consider writing a small plan that emits a known string and assert that it’s removed when passing a suitable regex.Also applies to: 40-41, 58-60
cli/pkg/digger/digger_test.go (1)
66-68
: Unused parameter – make intent explicit
Plan
’s new*string
argument isn’t used in the mock; naming it_
avoids the “what is this for?” moment.-func (m *MockTerraformExecutor) Plan(params []string, envs map[string]string, planJsonFilePath string, s *string) ... +func (m *MockTerraformExecutor) Plan(params []string, envs map[string]string, planJsonFilePath string, _ *string) ...libs/scheduler/jobs.go (1)
49-51
: Field order mismatch – use keyed literals
Stage
listsFilterRegex
beforeSteps
, but the composite literal inToConfigStage
setsSteps
first.
Keyed literals avoid such positional slip-ups and self-document intent:-return &Stage{ - Steps: steps, - FilterRegex: configStage.FilterRegex, -} +return &Stage{ + FilterRegex: configStage.FilterRegex, + Steps: steps, +}libs/execution/io_writer_filtered.go (1)
16-22
: Name the constructor result
NewFilteringWriter
returns a pointer but the struct has no exported fields,
so the caller cannot mutate it. Returning the value (not*
) would avoid
an unnecessary heap allocation.libs/execution/tf.go (2)
98-113
: Regex compiled for every Terraform command – cache to improve performance
regexp.Compile
is called on every invocation ofrunTerraformCommand
.
For workflows that execute many Terraform sub-commands this becomes wasteful.
Pre-compile once per plan (or reuse a*regexp.Regexp
passed in) to
avoid repeated compilation cost.
114-122
: Filtering writers created even when no regex suppliedWhen
filterRegex == nil
the code still allocates twoFilteringWriter
wrappers that do no filtering.
Replacing them with the direct writers whenregEx == nil
removes two
allocations per command and a small amount of indirection.- if printOutputToStdout { - mwout = NewFilteringWriter(os.Stdout, &stdout, regEx) - mwerr = NewFilteringWriter(os.Stderr, &stderr, regEx) - } else { - mwout = NewFilteringWriter(nil, &stdout, regEx) - mwerr = NewFilteringWriter(nil, &stderr, regEx) - } + if regEx == nil { + // no filtering needed + if printOutputToStdout { + mwout = io.MultiWriter(os.Stdout, &stdout) + mwerr = io.MultiWriter(os.Stderr, &stderr) + } else { + mwout = &stdout + mwerr = &stderr + } + } else { + if printOutputToStdout { + mwout = NewFilteringWriter(os.Stdout, &stdout, regEx) + mwerr = NewFilteringWriter(os.Stderr, &stderr, regEx) + } else { + mwout = NewFilteringWriter(nil, &stdout, regEx) + mwerr = NewFilteringWriter(nil, &stderr, regEx) + } + }libs/execution/pulumi.go (1)
38-46
: Unused*string
parameter clutters the interface
Plan(..., s *string)
accepts a regex pointer but never uses it. While
it is kept for interface uniformity, leaving the identifier ass
hides
the intent and triggersunused
linter ignores in callers.Rename to
_
orfilterRegex *string
for clarity, or document a TODO to
implement filtering in the Pulumi executor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
cli/pkg/digger/digger_test.go
(1 hunks)libs/digger_config/config.go
(1 hunks)libs/digger_config/converters.go
(1 hunks)libs/digger_config/digger_config.go
(2 hunks)libs/digger_config/yaml.go
(1 hunks)libs/execution/execution.go
(1 hunks)libs/execution/io_writer_filtered.go
(1 hunks)libs/execution/opentofu.go
(1 hunks)libs/execution/opentofu_test.go
(3 hunks)libs/execution/pulumi.go
(1 hunks)libs/execution/terragrunt.go
(1 hunks)libs/execution/tf.go
(6 hunks)libs/execution/tf_test.go
(3 hunks)libs/scheduler/jobs.go
(3 hunks)libs/scheduler/json_models.go
(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
libs/digger_config/converters.go (2)
libs/scheduler/jobs.go (1)
Stage
(48-51)libs/digger_config/config.go (1)
Stage
(97-100)
libs/digger_config/config.go (1)
libs/scheduler/jobs.go (1)
Step
(41-46)
libs/scheduler/jobs.go (1)
libs/digger_config/config.go (1)
Step
(89-95)
libs/execution/tf.go (1)
libs/execution/io_writer_filtered.go (1)
NewFilteringWriter
(16-22)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (4)
libs/digger_config/converters.go (1)
123-125
: Missing propagation of step-level filterOnly
Stage.FilterRegex
is copied.
Step
gained aFilterRegex
field (see config.go) but is never populated here, making the new struct member dead code. Either remove it or copy it from the YAML representation (if intended).libs/execution/opentofu_test.go (1)
23-24
: LGTM – signature update reflected in testsTests now pass
nil
for the newfilterRegex
parameter, matching the updated API.libs/execution/tf.go (1)
168-179
: Missing workspace handling inPlan
Unlike
Init
,Apply
,Destroy
, etc., thePlan
method does not switch to
tf.Workspace != "default"
. If users rely on non-default workspaces the plan
will run in the wrong state dir.Confirm and, if needed, insert the same
switchToWorkspace
logic.libs/execution/opentofu.go (1)
44-64
: Filtering parameter ignoredSame as Pulumi – the added
*string
parameter is unused. Please either:
- Implement filtering via
FilteringWriter
(preferred), or- Rename parameter to
_
with comment explaining deliberate no-op.Consistent behaviour across executors matters.
func copyStage(stage *StageYaml) *Stage { | ||
result := Stage{} | ||
result := Stage{ | ||
FilterRegex: stage.FilterRegex, | ||
} | ||
result.Steps = make([]Step, len(stage.Steps)) |
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.
🛠️ Refactor suggestion
Nil-pointer risk when stage
is nil
copyStage
is called unconditionally from copyWorkflows
; if a workflow omits the
plan
/apply
block, stage
will be nil
→ panic. Guard first:
func copyStage(stage *StageYaml) *Stage {
- result := Stage{
- FilterRegex: stage.FilterRegex,
- }
+ if stage == nil {
+ return nil
+ }
+ result := Stage{
+ FilterRegex: stage.FilterRegex,
+ }
🤖 Prompt for AI Agents
In libs/digger_config/converters.go around lines 122 to 126, the function
copyStage does not check if the input stage pointer is nil, which can cause a
panic if called with nil. Add a guard at the start of copyStage to return nil
immediately if stage is nil, preventing any dereference of a nil pointer.
libs/digger_config/digger_config.go
Outdated
if configYaml.Workflows != nil { | ||
for _, workflow := range configYaml.Workflows { | ||
if workflow.Plan != nil && workflow.Plan.FilterRegex != nil { | ||
_, err := regexp.Compile(*workflow.Plan.FilterRegex) | ||
if err != nil { | ||
slog.Error("invalid regex for plan filter", | ||
"regex", *workflow.Plan.FilterRegex, | ||
"error", err) | ||
return fmt.Errorf("regex for plan filter is invalid: %v", err) | ||
} | ||
} | ||
if workflow.Apply != nil && workflow.Apply.FilterRegex != nil { | ||
_, err := regexp.Compile(*workflow.Apply.FilterRegex) | ||
if err != nil { | ||
slog.Error("invalid regex for apply filter", | ||
"regex", *workflow.Apply.FilterRegex, | ||
"error", err) | ||
return fmt.Errorf("regex for apply filter is invalid: %v", err) | ||
} | ||
} | ||
} | ||
} |
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.
🛠️ Refactor suggestion
Validation loop: add more context & reuse compiled regex
- Error messages omit the workflow name, making debugging harder in large configs.
- Compiled regex is immediately discarded – we’ll need to compile again at execution time. Storing it would avoid double work.
- _, err := regexp.Compile(*workflow.Plan.FilterRegex)
+ if _, err := regexp.Compile(*workflow.Plan.FilterRegex); err != nil {
+ return fmt.Errorf("workflow %q: invalid plan filter regex %q: %w", name, *workflow.Plan.FilterRegex, err)
+ }
Apply the same pattern for the apply stage and optionally persist the compiled regex in the struct for later reuse.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libs/digger_config/digger_config.go around lines 550 to 571, the validation
loop for regex filters lacks context in error messages by not including the
workflow name, and it discards the compiled regex immediately, causing redundant
compilation later. Modify the error logs to include the workflow name for better
debugging, and store the compiled regex in the workflow struct fields for both
plan and apply filters to reuse them later during execution, avoiding
recompilation.
var filtered []byte | ||
if fw.pattern == nil { | ||
filtered = p | ||
} else { | ||
filtered = fw.pattern.ReplaceAll(p, []byte("<REDACTED>")) | ||
} | ||
|
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.
🛠️ Refactor suggestion
Pattern matching may fail across chunk boundaries
ReplaceAll
works only on the current byte slice; if the sensitive string
is split between two consecutive writes it will escape filtering.
Consider buffering until newline or using a streaming regex library to avoid
data-leak edge-cases.
🤖 Prompt for AI Agents
In libs/execution/io_writer_filtered.go around lines 27 to 33, the current use
of ReplaceAll on each byte slice independently can miss sensitive patterns split
across chunks. To fix this, implement buffering of incoming data until a newline
or another delimiter before applying the pattern replacement, or switch to a
streaming regex approach that can handle matches spanning multiple writes,
ensuring no sensitive data leaks between chunks.
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/execution/utils.go (2)
10-10
: Use idiomatic lower-case variable name for the compiled pattern
regEx
mixes camel-case with an upper-caseE
. Go’s style guide recommends simple, lower-case names likeregex
orre
.- var regEx *regexp.Regexp + var regex *regexp.Regexp
9-26
: Missing unit tests for the new helperThis helper is security-sensitive (malformed patterns, surprise “match-all” cases). Add focused tests covering:
•
nil
input →nil, nil
• valid pattern → compiled regexp, no error
• invalid pattern →nil, error
• empty string behaviour (per previous comment)Happy to supply a minimal test file if useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/execution/opentofu.go
(7 hunks)libs/execution/terragrunt.go
(5 hunks)libs/execution/tf.go
(7 hunks)libs/execution/utils.go
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/execution/terragrunt.go
- libs/execution/opentofu.go
- libs/execution/tf.go
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (1)
libs/execution/utils.go (1)
11-18
: Empty string is treated as “match-everything” – is that intentional?
regexp.Compile("")
succeeds and yields a pattern that matches every position, effectively disabling redaction.
If the intent is “no filter” when the string is empty, guard against it:if input != nil && *input != "" { slog.Debug("using regex for filter", "regex", *input) - var err error - regex, err = regexp.Compile(*input) + var err error + regex, err = regexp.Compile(*input) ... } else { slog.Debug("no regex for filter") regex = nil }
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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/execution/io_writer_filtered.go (1)
27-33
: Regex still applied per-chunk → pattern may leak across write boundaries
The earlier review already pointed this out.ReplaceAll
operates only on the
current slice, so a sensitive token split across two consecutiveWrite
calls
won’t be redacted. The mitigation (buffering until a delimiter or using a
streaming-regex lib) is still necessary.
🧹 Nitpick comments (1)
libs/execution/io_writer_filtered.go (1)
24-49
: Not safe for concurrent writes
FilteringWriter
maintains no internal synchronisation, yet stdout/stderr
streamers are often written from multiple goroutines. Document this limitation
or add a mutex to avoid interleaved/garbled output.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/digger_config/config.go
(1 hunks)libs/execution/io_writer_filtered.go
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/digger_config/config.go
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
libs/execution/io_writer_filtered.go
Outdated
if fw.writer != nil { | ||
_, err = fw.writer.Write(filtered) | ||
if err != nil { | ||
return 0, err | ||
} | ||
} | ||
if fw.buffer != nil { | ||
_, err = fw.buffer.Write(filtered) | ||
if err != nil { | ||
return 0, err | ||
} | ||
} |
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.
🛠️ Refactor suggestion
Partial-write handling is incorrect; may break io.Writer contract
You ignore the byte counts returned by the wrapped writers and always report
len(p)
on success. If the underlying writer performs a short write (n <
len(filtered)) with no error, the caller will believe all data was accepted,
potentially causing data loss or infinite loops. Capture n
and propagate the
lowest successful count.
- if fw.writer != nil {
- _, err = fw.writer.Write(filtered)
+ var wrote int
+ if fw.writer != nil {
+ n, err := fw.writer.Write(filtered)
+ if err != nil {
+ return n, err
+ }
+ wrote = n
}
- if fw.buffer != nil {
- _, err = fw.buffer.Write(filtered)
+ if fw.buffer != nil {
+ n, err := fw.buffer.Write(filtered)
+ if err != nil {
+ return min(wrote, n), err
+ }
+ if n < wrote || wrote == 0 {
+ wrote = n
+ }
}
- return len(p), err
+ return wrote, nil
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In libs/execution/io_writer_filtered.go around lines 34 to 45, the code ignores
the byte counts returned by the underlying writers and always returns len(p),
which violates the io.Writer contract. Modify the code to capture the number of
bytes written (n) from each writer call, track the lowest successful count among
them, and return that count along with any error. This ensures correct handling
of partial writes and prevents data loss or infinite loops.
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/digger_config/digger_config_test.go (2)
516-521
: Consider adding a negative-path test for invalid regex + tidy YAML styleGreat to see the happy-path for
filter_regex
covered. Two minor follow-ups you may want to add:
- A test case that supplies an obviously-bad pattern (e.g.
"*["
) and asserts thatLoadDiggerConfig
returns a validation error. That will protect the new validation logic you added inValidateDiggerConfigYaml
.- The list items
- init:
and- plan:
end with a colon (and a space at 520) while elsewhere in this file you use- init
(no colon) when no mapping follows. Keeping one style avoids churn in diffs and appeases YAML linters.
532-533
: Guard against nil pointer before dereferencingFilterRegex
assert.Equal(t, "myregex", *r)
will panic ifr
is unexpectedlynil
, masking the real failure reason. Add an explicit nil check first for clearer feedback:+ assert.NotNil(t, r, "filter_regex should have been parsed") r := dg.Workflows["my_custom_workflow"].Plan.FilterRegex assert.Equal(t, "myregex", *r)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/digger_config/digger_config.go
(2 hunks)libs/digger_config/digger_config_test.go
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/digger_config/digger_config.go
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
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.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/execution/io_writer_filtered.go (1)
35-51
:⚠️ Potential issuePartial-write handling still violates
io.Writer
contract
(Repeated from earlier review)You ignore the
n
returned by the underlying writers and always respond withlen(p)
.
If the wrapped writer performs a short write without error the caller will believe all bytes were accepted, risking data loss or infinite loops.Please propagate the minimum successful byte count and return immediately on error.
🧹 Nitpick comments (3)
libs/execution/io_writer_filtered_test.go (2)
3-8
: Prefer a single assertion styleThe test mixes
t.Errorf
withtestify/assert
. Pick one style for consistency (ideally the richerassert
helpers) to avoid cognitive overhead.
11-14
: Buffer initialisation can be simplified
var buf bytes.Buffer
is sufficient – the explicit= bytes.Buffer{}
is redundant.-var buf bytes.Buffer = bytes.Buffer{} +var buf bytes.Bufferlibs/execution/io_writer_filtered.go (1)
35-44
: Debug prints leak filtered content and hurt performance
fmt.Printf("filtered: %v\n", filtered)
:
- Re-exposes the (partly) sensitive data you just redacted.
- Spams stdout in production libraries.
- Adds an extra allocation per write.
Remove or gate behind a debug logger.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/execution/io_writer_filtered.go
(1 hunks)libs/execution/io_writer_filtered_test.go
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
libs/execution/io_writer_filtered_test.go (1)
libs/execution/io_writer_filtered.go (1)
NewFilteringWriter
(17-23)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
writer.Write([]byte("sensitive")) | ||
assert.Equal(t, "<REDACTED>", buf.String()) | ||
|
||
writer.Write([]byte("output")) | ||
assert.Equal(t, "<REDACTED>output", buf.String()) |
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.
🛠️ Refactor suggestion
Handle and assert Write
errors
Write
returns (n, err)
but the error is silently ignored.
Capture the returned values and assert err == nil
to ensure the writer behaves as expected.
- writer.Write([]byte("sensitive"))
+ n, err := writer.Write([]byte("sensitive"))
+ assert.NoError(t, err)
+ assert.Equal(t, 9, n) // len("sensitive")
Repeat for the second write.
🤖 Prompt for AI Agents
In libs/execution/io_writer_filtered_test.go around lines 22 to 26, the Write
method calls ignore the returned error values. Modify the code to capture both
the number of bytes written and the error for each Write call, then add
assertions to check that the error is nil to verify the writes succeed without
errors.
func (fw *FilteringWriter) Write(p []byte) (n int, err error) { | ||
// Filter the content | ||
|
||
var filtered []byte | ||
if fw.pattern == nil { | ||
filtered = p | ||
} else { | ||
filtered = fw.pattern.ReplaceAll(p, []byte("<REDACTED>")) | ||
} |
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.
🛠️ Refactor suggestion
Regex applied per-chunk may leak data across boundaries
ReplaceAll
is run only on the current byte slice. If the sensitive token spans two consecutive Write
calls it will bypass redaction (e.g., "sens"
then "itive"
).
Consider buffering until a delimiter (newline) or using a streaming/rolling matcher to guarantee full coverage.
🤖 Prompt for AI Agents
In libs/execution/io_writer_filtered.go around lines 25 to 33, the current Write
method applies regex replacement only on the current byte slice, which can miss
sensitive tokens spanning multiple Write calls. To fix this, implement buffering
of incoming data until a delimiter like a newline is encountered, then apply the
regex replacement on the buffered data before writing it out. This ensures that
tokens split across chunks are fully matched and redacted.
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/ce/howto/masking-sensitive-values.mdx (3)
5-5
: Fix typo in description
The word “senstivie” is misspelled.- You can use a regular expression to ensure that senstivie outputs are masked from plan outputs and comments by digger. + You can use a regular expression to ensure that sensitive outputs are masked from plan outputs and comments by digger.
18-19
: Improve grammar and punctuation
Combine sentences for clarity, add missing commas, and correct “occurence”:- Currently this is only supported at the plan step. The filter_regex is a regular expression and it will mask any occurence of - that expression in the logs and the comment, replacing it with `<REDACTED>`. For example the following terraform: + Currently, this is only supported at the plan step, and the `filter_regex` is a regular expression that masks any occurrence of that expression in the logs and comments, replacing it with `<REDACTED>`. For example, the following Terraform:🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...The filter_regex is a regular expression and it will mask any occurence of that expr...(COMMA_COMPOUND_SENTENCE)
[typographical] ~19-~19: After the expression ‘for example’ a comma is usually used.
Context: ...nt, replacing it with<REDACTED>
. For example the following terraform: ``` output "f...(COMMA_FOR_EXAMPLE)
27-27
: Add alt text to image markdown
Provide meaningful alt text for accessibility:-  + 
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/images/configuration/masking-sensitive-values.png
is excluded by!**/*.png
📒 Files selected for processing (2)
docs/ce/howto/masking-sensitive-values.mdx
(1 hunks)docs/mint.json
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/mint.json
🧰 Additional context used
🪛 LanguageTool
docs/ce/howto/masking-sensitive-values.mdx
[uncategorized] ~17-~17: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ...eps: - init - plan ``` Currently this is only supported at the plan step...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~18-~18: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...The filter_regex is a regular expression and it will mask any occurence of that expr...
(COMMA_COMPOUND_SENTENCE)
[typographical] ~19-~19: After the expression ‘for example’ a comma is usually used.
Context: ...nt, replacing it with <REDACTED>
. For example the following terraform: ``` output "f...
(COMMA_FOR_EXAMPLE)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
Summary by CodeRabbit