Skip to content

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

Merged
merged 14 commits into from
Jun 14, 2025
Merged

support filtered regex #1978

merged 14 commits into from
Jun 14, 2025

Conversation

motatoes
Copy link
Contributor

@motatoes motatoes commented Jun 14, 2025

Summary by CodeRabbit

  • New Features
    • Introduced optional regex-based filtering for plan steps to redact sensitive content in command outputs and comments.
    • Added validation to ensure regex filters are syntactically correct before execution.
  • Bug Fixes
    • Improved formatting and consistency in configuration and job definitions.
  • Refactor
    • Updated method signatures and internal logic to support regex filtering across Terraform, Pulumi, Terragrunt, and OpenTofu integrations.
    • Added a filtering writer to transparently redact output based on regex patterns.
  • Tests
    • Adjusted tests to accommodate new method signatures and filtering functionality.
    • Added tests verifying filtering writer behavior and regex filter parsing in configurations.
  • Documentation
    • Added new documentation explaining how to mask sensitive values using regex filters in workflows.
    • Enhanced configuration tests to verify new regex filter fields in workflows.

Copy link
Contributor

coderabbitai bot commented Jun 14, 2025

Walkthrough

This update introduces optional regex-based filtering to workflow stages and steps, enabling output redaction during plan execution. It adds FilterRegex fields to relevant structs, validates regex syntax in configuration, and updates executor interfaces and implementations to support an optional regex parameter. Associated tests and conversion logic are also updated accordingly.

Changes

File(s) Change Summary
cli/pkg/digger/digger_test.go Updated MockTerraformExecutor.Plan method signature to include a *string parameter.
libs/digger_config/config.go
libs/digger_config/yaml.go
libs/scheduler/jobs.go
libs/scheduler/json_models.go
Added FilterRegex field to StageYaml, Stage, and StageJson structs and updated related conversions.
libs/digger_config/converters.go
libs/scheduler/jobs.go
libs/scheduler/json_models.go
Updated stage conversion logic to handle new FilterRegex field.
libs/digger_config/digger_config.go Added validation for regex syntax in workflow plan/apply filters.
libs/execution/tf.go Updated TerraformExecutor interface and Terraform struct's Plan and runTerraformCommand methods to accept optional filterRegex and apply output filtering.
libs/execution/io_writer_filtered.go Introduced FilteringWriter for regex-based output redaction.
libs/execution/execution.go Passed PlanStage.FilterRegex to executor's Plan method.
libs/execution/opentofu.go
libs/execution/pulumi.go
libs/execution/terragrunt.go
Updated Plan method signatures to include a *string filterRegex parameter (unused in implementation except opentofu and terragrunt use it for filtering).
libs/execution/tf_test.go
libs/execution/opentofu_test.go
Updated test calls to Plan to provide the new *string argument.
libs/execution/utils.go Added stringToRegex utility function to compile optional regex strings with logging and error handling.
libs/execution/io_writer_filtered_test.go Added tests for FilteringWriter verifying redaction behavior.
libs/digger_config/digger_config_test.go Updated workflow YAML test to include filter_regex field and assert its parsing.
docs/ce/howto/masking-sensitive-values.mdx
docs/mint.json
Added documentation describing usage of filter_regex for masking sensitive output in plan steps.

Sequence Diagram(s)

Loading
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

  • UtpalJayNadiger

Poem

🐰
Regexes hop in, with a filtering grin,
Redacting the secrets as outputs begin.
Writers now clever, with patterns in tow,
Hide what they must, let safe info show.
Stages and steps with filters anew—
The garden of configs just grew and grew!

✨ Finishing Touches
🧪 Generate Unit Tests
  • Create PR with Unit Tests
  • Commit Unit Tests in branch feat/filtered-regex
  • Post Copyable Unit Tests in Comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai auto-generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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 in libs/execution/io_writer_filtered.go for pattern-based content redaction in execution outputs
  • Incomplete implementation in libs/execution/pulumi.go and libs/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

Value string
ExtraArgs []string
Shell string
FilterRegex string
Copy link
Contributor

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

@@ -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) {
Copy link
Contributor

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.

Comment on lines 34 to 36
if fw.writer != nil {
fw.writer.Write(filtered)
}
Copy link
Contributor

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 }

Comment on lines 40 to 41
params = append(params, []string{"--save-plan", planArtefactFilePath}...)
stdout, stderr, statusCode, err := pl.runPululmiCommand("preview", true, envs, params...)
Copy link
Contributor

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

Comment on lines +552 to +569
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)
}
}
Copy link
Contributor

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.

@@ -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) {
Copy link
Contributor

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.

Suggested change
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) {

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 carries FilterRegex (l. 81) yet ToCoreStage() (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 issue

Nil-pointer dereference when default workflow is used

d.PlanStage can be nil (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 drops FilterRegex when steps is empty

Early-returning when no steps are present means a StageJson that only specifies a filter_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 regex

Without 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 issue

Terragrunt Plan always returns true regardless of exit code

Method ignores exitCode and unconditionally returns true for hasChanges,
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: Unused Step.FilterRegex field

Step now embeds FilterRegex (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 lists Steps before FilterRegex; the YAML struct lists FilterRegex 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 consistency

This file uses filter_regex, while the YAML structs use the same snake_case field. Just double-check that any client code or API expecting filterRegex (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 behaviour

The 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 lists FilterRegex before Steps, but the composite literal in ToConfigStage sets Steps 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 of runTerraformCommand.
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 supplied

When filterRegex == nil the code still allocates two FilteringWriter
wrappers that do no filtering.
Replacing them with the direct writers when regEx == 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 as s hides
the intent and triggers unused linter ignores in callers.

Rename to _ or filterRegex *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

📥 Commits

Reviewing files that changed from the base of the PR and between 3540796 and b40b1bc.

📒 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 filter

Only Stage.FilterRegex is copied.
Step gained a FilterRegex 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 tests

Tests now pass nil for the new filterRegex parameter, matching the updated API.

libs/execution/tf.go (1)

168-179: Missing workspace handling in Plan

Unlike Init, Apply, Destroy, etc., the Plan 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 ignored

Same as Pulumi – the added *string parameter is unused. Please either:

  1. Implement filtering via FilteringWriter (preferred), or
  2. Rename parameter to _ with comment explaining deliberate no-op.

Consistent behaviour across executors matters.

Comment on lines 122 to 126
func copyStage(stage *StageYaml) *Stage {
result := Stage{}
result := Stage{
FilterRegex: stage.FilterRegex,
}
result.Steps = make([]Step, len(stage.Steps))
Copy link
Contributor

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.

Comment on lines 550 to 571
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)
}
}
}
}
Copy link
Contributor

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

  1. Error messages omit the workflow name, making debugging harder in large configs.
  2. 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.

Comment on lines +27 to +33
var filtered []byte
if fw.pattern == nil {
filtered = p
} else {
filtered = fw.pattern.ReplaceAll(p, []byte("<REDACTED>"))
}

Copy link
Contributor

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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-case E. Go’s style guide recommends simple, lower-case names like regex or re.

-	var regEx *regexp.Regexp
+	var regex *regexp.Regexp

9-26: Missing unit tests for the new helper

This 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

📥 Commits

Reviewing files that changed from the base of the PR and between b40b1bc and d999a91.

📒 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
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 consecutive Write 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

📥 Commits

Reviewing files that changed from the base of the PR and between 6dec7f5 and 465165e.

📒 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

Comment on lines 34 to 45
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
}
}
Copy link
Contributor

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 style

Great to see the happy-path for filter_regex covered. Two minor follow-ups you may want to add:

  1. A test case that supplies an obviously-bad pattern (e.g. "*[") and asserts that LoadDiggerConfig returns a validation error. That will protect the new validation logic you added in ValidateDiggerConfigYaml.
  2. 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 dereferencing FilterRegex

assert.Equal(t, "myregex", *r) will panic if r is unexpectedly nil, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 465165e and 3e2756c.

📒 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 issue

Partial-write handling still violates io.Writer contract
(Repeated from earlier review)

You ignore the n returned by the underlying writers and always respond with len(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 style

The test mixes t.Errorf with testify/assert. Pick one style for consistency (ideally the richer assert 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.Buffer
libs/execution/io_writer_filtered.go (1)

35-44: Debug prints leak filtered content and hurt performance

fmt.Printf("filtered: %v\n", filtered):

  1. Re-exposes the (partly) sensitive data you just redacted.
  2. Spams stdout in production libraries.
  3. Adds an extra allocation per write.

Remove or gate behind a debug logger.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e2756c and ac20c2b.

📒 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

Comment on lines +22 to +26
writer.Write([]byte("sensitive"))
assert.Equal(t, "<REDACTED>", buf.String())

writer.Write([]byte("output"))
assert.Equal(t, "<REDACTED>output", buf.String())
Copy link
Contributor

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.

Comment on lines +25 to +33
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>"))
}
Copy link
Contributor

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:

- ![](/images/configuration/masking-sensitive-values.png)
+ ![Masking sensitive values in plan output](/images/configuration/masking-sensitive-values.png)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac20c2b and bedc786.

⛔ 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

@motatoes motatoes merged commit d73a9e9 into develop Jun 14, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants