support PLANOUT

[motatoes]

• move execution below
• add tf json output
• fix args order

[greptile-apps]

Greptile Summary

This PR implements support for a PLANOUT feature by adding conditional JSON output support to the Terraform execution framework. The core change involves refactoring the Plan() method in execution.go to separate plan execution from post-processing operations, and adding a boolean parameter to all TerraformExecutor interface implementations' Show() methods to control JSON output formatting.

The key architectural change moves the terraform show command execution, plan summary generation, plan storage, and cleanup logic outside of the planning loop to execute after all plan steps complete. This ensures that plan analysis operations occur after the planning phase is fully finished, making the execution flow more robust.

The new boolean parameter added to Show() methods enables the system to retrieve both JSON-formatted output (for programmatic analysis) and human-readable output (for user display) from the same plan file. The execution logic now makes two separate Show() calls - one requesting JSON format for plan parsing and analysis, and another requesting human-readable format for cleanup and reporting purposes.

All executor implementations (Terraform, OpenTofu, Terragrunt, Pulumi) have been updated to maintain interface compatibility, though the implementation varies: Terraform conditionally adds the -json flag based on the parameter, while OpenTofu, Terragrunt, and Pulumi continue to always output JSON regardless of the parameter value. The mock implementation in tests has also been updated to match the new interface signature.

PR Description Notes:

• The PR description mentions "fix args order" but the changes don't show obvious argument reordering - this may refer to internal parameter ordering in the Show() method calls
• There's a typo in the Terraform implementation where the parameter is named retrunJson instead of returnJson

Confidence score: 2/5

• This PR has several implementation inconsistencies and a parameter naming typo that could cause confusion
• Score lowered due to inconsistent JSON parameter handling across executors and the unused parameter in some implementations
• Pay close attention to libs/execution/tf.go (typo in parameter name), libs/execution/opentofu.go, and libs/execution/terragrunt.go (unused boolean parameters)

_{6 files reviewed, 6 comments}

_{Edit Code Review Bot Settings | Greptile}

[greptile-apps]

logic: Logic error: 'err' variable is nil at this point (last assignment was from Show() call), but you're checking if err != nil. This will never execute the error logging.

[greptile-apps]

style: Parameter name 'b' is non-descriptive. Consider using 'returnJson' or 'asJson' to match the intent of the real implementation.

[greptile-apps]

syntax: Parameter name has typo: retrunJson should be returnJson

[greptile-apps]

logic: The boolean parameter b is added but not used. Unlike the Terraform implementation, this always includes -json regardless of the parameter value.

[greptile-apps]

style: The boolean parameter b is added but never used. Consider renaming it to returnJson for clarity and implementing the conditional logic like in the Terraform implementation.

[greptile-apps]

style: Unlike the Terraform implementation, the -json flag is hardcoded here. Consider making this conditional based on the boolean parameter for consistency across executors.

[bismuthdev]

Bug Summary Report

Total Bugs Found: 6

Summary of Bugs

1. Terragrunt Show Method Parameter Issue - The method always includes the -json flag regardless of the boolean parameter value, making it inconsistent with the Terraform implementation.

2. Pulumi Show Method Parameter Issue - Similar to the Terragrunt bug, the method always adds the --json flag regardless of the boolean parameter value.

3. Terraform Show Method Typo - The parameter name 'retrunJson' has a spelling error and should be 'returnJson'.

4. OpenTofu Show Method Parameter Issue - The method doesn't respect the boolean parameter and always includes the -json flag regardless of the parameter value.

5. OpenTofu Implementation Inconsistency - Duplicate bug report for the OpenTofu Show method, highlighting the inconsistency with the Terraform implementation.

6. Show Method Interface Lacks Documentation - The boolean parameter in the Show method interface is not descriptive and should be documented with a comment to indicate its purpose.

Most Critical Bugs

The most critical issues are the consistent implementation bugs across multiple IaC tools (Terragrunt, Pulumi, and OpenTofu) where the JSON output flag is always included regardless of the parameter value. This creates inconsistent behavior compared to the Terraform implementation and could lead to unexpected outputs when these methods are used.

[bismuthdev]

The parameter name 'retrunJson' in the Show method has a typo. It should be 'returnJson' instead. This is a simple spelling error that should be fixed for code clarity and consistency.

Suggested change

	func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
	func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {

[bismuthdev]

The parameter name retrunJson in the Terraform.Show method has a typo. It should be returnJson instead of retrunJson.

Suggested change

	func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
	func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {

[bismuthdev]

The boolean parameter in the Show method interface is not descriptive. It should be documented with a comment to indicate its purpose (returnJson) for better code readability and maintainability.

Suggested change

	Show([]string, map[string]string, string, bool) (string, string, error)
	Show([]string, map[string]string, string, bool /* returnJson */) (string, string, error)

[greptile-apps]

Greptile Summary

Note: This review covers only the changes made since the last review (commit a61cd78), not the entire PR.

This change updates the test expectations in digger_test.go to reflect a new execution order in the Terraform planning process. The test TestCorrectCommandExecutionWhenPlanning now expects a different sequence of commands: the Run command has been moved to execute earlier in the process (before the first Show operation), and an additional Show command has been added at the end of the sequence.

The new expected sequence is: Init, Plan, Run echo, Show, StorePlanFile plan, Show. This change appears to support the PLANOUT functionality mentioned in the PR title, where the system now performs two separate Show operations - likely one for internal plan processing and another for generating JSON output. Moving the Run command earlier ensures that any custom commands execute before plan output processing begins.

This test modification aligns with the broader changes to support PLANOUT functionality, where the execution flow has been restructured to accommodate JSON output generation and proper sequencing of operations.

Confidence score: 4/5

• This change is safe to merge with minimal risk as it only updates test expectations to match the new execution flow
• Score reflects that this is a straightforward test update that properly validates the new command execution sequence
• Pay attention to ensuring the actual implementation matches these test expectations and that the new execution order doesn't break existing functionality

_{1 file reviewed, no comments}

_{Edit Code Review Bot Settings | Greptile}

[bismuthdev]

Summary of Bugs Found in Codebase

A total of 5 bugs were identified in the codebase, primarily related to inconsistent implementation of the Show method across different IaC executor implementations.

Critical Bugs

1. Inconsistent JSON flag handling in Pulumi executor: The Pulumi implementation always adds the --json flag regardless of the boolean parameter value, unlike the Terraform implementation.

2. Inconsistent JSON flag handling in Terragrunt executor: Similar to Pulumi, the Terragrunt implementation always includes the -json flag regardless of the boolean parameter value.

3. Inconsistent JSON flag handling in OpenTofu executor: The OpenTofu implementation also always includes the -json flag regardless of the boolean parameter value.

4. Parameter naming typo in Terraform executor: The parameter is incorrectly named retrunJson instead of returnJson in the Terraform implementation.

5. Redundant Show method calls in DiggerExecutor: The Plan method calls Show twice - once with the boolean parameter true and once with false, but this is redundant for executors that always include the JSON flag.

These bugs create inconsistent behavior across different executor implementations and could lead to unexpected results when using the library.

[bismuthdev]

There is a typo in the parameter name 'retrunJson' in the Show method of the Terraform struct. The parameter should be named 'returnJson' instead. This typo could lead to confusion for developers maintaining the code, as it doesn't follow the standard naming convention and could be easily misread. Consistent parameter naming is important for code readability and maintainability.

Suggested change

	func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
	params = append(params, "-no-color")
	if retrunJson {
	params = append(params, "-json")
	}
	func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
	params = append(params, "-no-color")
	if returnJson {
	params = append(params, "-json")
	}

[bismuthdev]

The Plan method in DiggerExecutor calls Show twice - once with the boolean parameter true to get JSON output for parsing, and once with false to get human-readable output. However, the implementations of Show in Terragrunt, Pulumi, and OpenTofu always include the -json flag regardless of the boolean parameter value, making the second call redundant and potentially causing unexpected behavior.

The fix checks if the executor is specifically a Terraform executor (which respects the boolean parameter) and only makes the second call in that case. For other executors, it reuses the JSON output from the first call since they always return JSON format anyway.

[greptile-apps]

Greptile Summary

This review covers only the changes made since the last review (commit b90269b), not the entire PR.

The most recent changes focus on simplifying the cleanupTerraformPlan function signature by removing three unused parameters. In the test files, the function calls have been updated from cleanupTerraformPlan(true, nil, stdout, "") to cleanupTerraformPlan(stdout), indicating that the function now only requires the stdout string parameter. This refactoring eliminates the boolean flag, nil parameter, and empty string that were previously passed but apparently unused in the function implementation.

Additionally, there's a logic fix in the execution flow where an error check has been corrected. Previously, there was a bug where err was being checked for nil status after a Show() call, but the variable was already nil at that point, making the error logging unreachable. This has been addressed to ensure proper error handling.

These changes align with the PR's broader goal of supporting PLANOUT functionality and fixing argument order issues, making the API cleaner and more focused by removing redundant parameters.

Confidence score: 4/5

• This PR is safe to merge with minimal risk as it primarily simplifies function signatures and fixes a minor logic bug
• Score reflects straightforward refactoring changes that remove unused parameters and correct error handling logic
• Pay close attention to any integration points that might still be calling the old cleanupTerraformPlan signature with 4 parameters

_{2 files reviewed, no comments}

_{Edit Code Review Bot Settings | Greptile}

[bismuthdev]

Bug Summary Report

A total of 15 bugs were identified in the codebase, with several critical issues affecting the execution flow and error handling. Here's a summary of the bugs found:

Critical Bugs

1. Parameter Typo in Terraform.Show Method

   • Misspelled parameter name retrunJson instead of returnJson

2. JSON Flag Always Added Regardless of Parameter Value

   • In OpenTofu, Terragrunt, and Pulumi implementations, the JSON flag is always added regardless of the boolean parameter value
   • This causes these executors to always return JSON output even when non-JSON output is requested

3. Potential Nil Pointer Dereference

   • In DiggerExecutor.Plan method, d.PlanStage.FilterRegex is accessed without checking if d.PlanStage is nil
   • Could cause a panic if d.PlanStage is nil

4. Missing Plan JSON Output

   • The variable terraformPlanOutputJsonString is initialized but never populated before being returned
   • Results in empty string being returned instead of JSON output

5. No Check for Plan File Existence

   • The code doesn't verify if the plan file exists before trying to read it
   • Can lead to errors when attempting to read non-existent files

Other Issues

• Inconsistent error handling in postProcessPlan function
• Errors from the Show command are ignored in multiple places
• Unnecessary error check for already handled errors
• Post-processing attempted even when no plan step was executed
• Inconsistent parameter naming across implementations

These bugs primarily affect the execution flow, error handling, and output formatting of the infrastructure-as-code operations, potentially causing unexpected behavior or silent failures.

[bismuthdev]

There is a typo in the parameter name retrunJson in the Show method of the Terraform struct. The parameter should be named returnJson instead. This typo could cause confusion for developers who are trying to understand or use this method, as the parameter name doesn't clearly convey its purpose due to the misspelling.

Suggested change

	func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, retrunJson bool) (string, string, error) {
	params = append(params, "-no-color")
	if retrunJson {
	params = append(params, "-json")
	}
	func (tf Terraform) Show(params []string, envs map[string]string, planArtefactFilePath string, returnJson bool) (string, string, error) {
	params = append(params, "-no-color")
	if returnJson {
	params = append(params, "-json")
	}

[bismuthdev]

There's an inconsistent error handling pattern in the postProcessPlan function. At line 332-334, there's an if err != nil check that logs an error but doesn't return it. This is problematic because:

1. The err variable being checked was already handled earlier in the function (at lines 301-304) and would have been returned if it was non-nil.
2. By this point in the code, err should always be nil unless there's a logic error elsewhere.
3. The error message "error publishing comment" doesn't match any operation being performed in this section of code.

This appears to be a leftover error check that should be removed, as it's checking an error that has already been handled and is using an error message that doesn't match the current operation.

Suggested change

	// TODO: move this function to iacUtils interface and implement for pulumi
	cleanedUpPlan := cleanupTerraformPlan(stdout)
	if err != nil {
	slog.Error("error publishing comment", "error", err)
	}
	// TODO: move this function to iacUtils interface and implement for pulumi
	cleanedUpPlan := cleanupTerraformPlan(stdout)

[bismuthdev]

There is a potential null pointer dereference in the Plan method of DiggerExecutor. When d.PlanStage is nil, the code correctly creates default plan steps, but later when calling the Plan method on the TerraformExecutor, it directly uses d.PlanStage.FilterRegex without checking if d.PlanStage is nil. This could lead to a null pointer dereference.

The fix adds a check to safely handle the case when d.PlanStage is nil by creating a local filterRegex variable that is only set from d.PlanStage.FilterRegex when d.PlanStage is not nil.

Suggested change

	_, _, _, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), d.PlanStage.FilterRegex)
	var filterRegex *string
	if d.PlanStage != nil {
	filterRegex = d.PlanStage.FilterRegex
	}
	_, _, _, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), filterRegex)

[bismuthdev]

The variable terraformPlanOutputJsonString is initialized but never populated with any value before being returned from the Plan() function. This is a bug because the function signature indicates it should return the JSON output from the Show command as its fifth return value.

The function already calls Show() in the postProcessPlan() method, but it doesn't store the result in the terraformPlanOutputJsonString variable. This means that any code expecting to receive the JSON output from the Plan function will receive an empty string instead.

The fix adds a call to the Show command to populate the variable with the JSON output before returning it.

Suggested change

	terraformPlanOutputJsonString := ""
	terraformPlanOutput, _, _ := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
	terraformPlanOutputJsonString := terraformPlanOutput

[bismuthdev]

There's a bug in the postProcessPlan method where there's an error check at line 332-334 that doesn't make sense. The err variable being checked is not defined in the local scope at this point - the last err variable was defined and handled at line 323-327 for the StorePlanFile operation.

This error check is meaningless and should be removed as it's checking a variable that's already been handled earlier in the function. The code will never log an error here because if there was an error with StorePlanFile, the function would have already returned with an error.

Suggested change

	// TODO: move this function to iacUtils interface and implement for pulumi
	cleanedUpPlan := cleanupTerraformPlan(stdout)
	if err != nil {
	slog.Error("error publishing comment", "error", err)
	}
	// TODO: move this function to iacUtils interface and implement for pulumi
	cleanedUpPlan := cleanupTerraformPlan(stdout)

[bismuthdev]

In the postProcessPlan method, there's no check to ensure that the plan file exists before trying to read it. This can lead to errors if the plan file doesn't exist, as the Show method will attempt to read a non-existent file.

The fix adds a check using os.Stat to verify the file exists before proceeding with the Show command. If the file doesn't exist, it returns an appropriate error message that includes the path that was checked, making debugging easier.

Suggested change

	showArgs := make([]string, 0)
	terraformPlanOutput, _, _ := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
	showArgs := make([]string, 0)
	// Check if plan file exists before trying to read it
	if _, err := os.Stat(d.PlanPathProvider.LocalPlanFilePath()); os.IsNotExist(err) {
	return "", nil, false, fmt.Errorf("plan file does not exist at %s: %v", d.PlanPathProvider.LocalPlanFilePath(), err)
	}
	terraformPlanOutput, _, _ := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)

[bismuthdev]

When there's no explicit plan step (hasPlanStep is false), the code still tries to post-process the plan by calling d.postProcessPlan(plan). However, the postProcessPlan function attempts to read and process a plan file at d.PlanPathProvider.LocalPlanFilePath() which wouldn't exist if no plan step was executed.

This will lead to errors when trying to read a non-existent file in the postProcessPlan function, specifically at line 317 where it calls os.ReadFile(d.PlanPathProvider.LocalPlanFilePath()). The error is properly caught and returned, but it's an unnecessary error condition that could be avoided by simply not attempting to post-process when there's no plan step.

Suggested change

	if !hasPlanStep {
	var err error
	plan, planSummary, isEmptyPlan, err = d.postProcessPlan(plan)
	if err != nil {
	slog.Debug("error post processing plan",
	"error", err,
	"plan", plan,
	"planSummary", planSummary,
	"isEmptyPlan", isEmptyPlan,
	)
	return nil, false, false, "", "", fmt.Errorf("error post processing plan: %v", err) //nolint:wrapcheck // err
	}
	}
	if !hasPlanStep {
	// Skip post-processing if there was no plan step, as there won't be a plan file to process
	slog.Debug("No plan step found, skipping post-processing")
	}

[bismuthdev]

There is a potential nil pointer dereference in the Plan method of DiggerExecutor. On line 241, d.PlanStage.FilterRegex is accessed without first checking if d.PlanStage is nil.

While there is a check at the beginning of the method (lines 209-220) to initialize planSteps if d.PlanStage is nil, there's no similar check when accessing d.PlanStage.FilterRegex directly in the call to d.TerraformExecutor.Plan().

If d.PlanStage is nil at this point, accessing d.PlanStage.FilterRegex would cause a nil pointer dereference and result in a panic.

The fix adds a check to safely handle the case where d.PlanStage might be nil by creating a local variable filterRegex that is only set from d.PlanStage.FilterRegex if d.PlanStage is not nil.

Suggested change

	_, _, _, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), d.PlanStage.FilterRegex)
	var filterRegex *string
	if d.PlanStage != nil {
	filterRegex = d.PlanStage.FilterRegex
	}
	_, _, _, err := d.TerraformExecutor.Plan(planArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), filterRegex)

[bismuthdev]

The RetrievePlanJson method in libs/execution/execution.go ignores errors from the Show command. If the Show command fails, the error is silently ignored and the method returns potentially empty or incomplete output. This could lead to downstream issues where the plan JSON is expected to be valid but is actually empty or malformed.

The fix adds proper error handling to check if the Show command fails and returns an appropriate error message that includes both the error and stderr output for better debugging.

Suggested change

	showArgs := make([]string, 0)
	terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath)
	terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
	return terraformPlanOutput, nil
	showArgs := make([]string, 0)
	terraformPlanOutput, stderr, err := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
	if err != nil {
	return "", fmt.Errorf("failed to show plan: %v, stderr: %s", err, stderr)
	}
	return terraformPlanOutput, nil

[bismuthdev]

In the postProcessPlan method, errors from the Show command are also ignored. If the Show command fails, the error is silently ignored and the method continues processing with potentially empty or invalid plan output. This could lead to incorrect plan summaries or other downstream issues.

The fix adds proper error handling to check if the Show command fails and returns an appropriate error message that includes both the error and stderr output for better debugging.

Suggested change

	showArgs := make([]string, 0)
	terraformPlanOutput, _, _ := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
	showArgs := make([]string, 0)
	terraformPlanOutput, stderr, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
	if err != nil {
	return "", nil, false, fmt.Errorf("error showing plan: %v, stderr: %s", err, stderr)
	}

[greptile-apps]

Greptile Summary

This review covers only the changes made since the last review (commit e4c90db), not the entire PR.

This change updates the unit test TestCorrectCommandExecutionWhenPlanning to reflect a modification in the command execution order during the planning phase. The test assertion has been updated to expect the Run command (with echo) to execute after the StorePlanFile operation rather than between the Plan and Show operations.

Specifically, the expected command sequence has changed from ["Init ", "Plan ", "Run echo", "Show ", "StorePlanFile plan", "Show "] to ["Init ", "Plan ", "Show ", "StorePlanFile plan", "Run echo"]. This indicates that custom run commands now execute after the plan has been generated, shown, and stored, ensuring that all core Terraform operations complete before any custom commands run. Additionally, a duplicate Show command has been removed from the expected sequence, cleaning up redundant operations.

This change aligns with the PR's goal of supporting PLANOUT functionality, where the reordering ensures that custom commands have access to completed plan artifacts and that the planning workflow is more logically structured.

Confidence score: 4/5

• This PR is safe to merge with minimal risk as it only updates a test assertion to match the new execution order
• Score reflects that this is a straightforward test update that correctly reflects an architectural change in command execution ordering
• Pay close attention to ensure the actual implementation matches this test expectation and that downstream systems expecting the old execution order are updated accordingly

_{1 file reviewed, no comments}

_{Edit Code Review Bot Settings | Greptile}

[bismuthdev]

Summary of Bugs Found in Codebase

A total of 12 bugs were identified in the codebase, primarily related to inconsistencies in the implementation of the Show method across different executor implementations. Here's a summary of the most critical issues:

Critical Bugs

1. Parameter Name Typo: The Show method in the Terraform struct has a parameter named retrunJson instead of returnJson, making the code less readable and potentially confusing for developers.

2. Inconsistent JSON Flag Handling:

   • In the Terraform implementation, the -json flag is conditionally added based on the returnJson parameter
   • In the OpenTofu, Terragrunt, and Pulumi implementations, the -json flag is always added regardless of the parameter value
   • This inconsistency can cause bugs when the parameter is set to false, as different executors will produce different output formats

3. Non-descriptive Parameter Names: In some implementations like Terragrunt, the boolean parameter is named b, which is not descriptive and makes the code harder to maintain.

4. Missing Documentation: The TerraformExecutor interface lacks documentation explaining the purpose of the boolean parameter in the Show method, making it difficult for developers to understand its intended use.

5. Missing Tests: There are no tests for the Show method's boolean parameter functionality in either tf_test.go or opentofu_test.go, meaning bugs in this functionality could go undetected.

These issues affect code readability, maintainability, and could lead to inconsistent behavior across different executor implementations.

[bismuthdev]

In the postProcessPlan method, a file is created with os.Create when the plan is not empty, and the file handle is closed using defer file.Close(). However, if any of the subsequent operations in the function fail (like reading the plan file or storing it), the function will return early with an error, but the deferred close will only execute when the function completes normally.

This pattern can lead to file handle leaks if there are multiple error paths after the file is created. The fix is to close the file immediately after creation since we don't actually write anything to it - we're just creating an empty marker file. This ensures the file handle is properly closed regardless of any subsequent errors.

Suggested change

	if !isEmptyPlan {
	nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	defer file.Close()
	}
	if !isEmptyPlan {
	nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	// Close file immediately after creation since we don't write to it
	// This ensures the file handle is closed even if subsequent operations fail
	file.Close()
	}

[greptile-apps]

Greptile Summary

This review covers only the changes made since the last review (commit 20aff88), not the entire PR.

The most recent change in this PR is a simple typo fix in the Terraform.Show method in libs/execution/tf.go. The parameter name retrunJson has been corrected to returnJson on line 170. This change improves code readability and maintains consistency with standard naming conventions. The functionality of the method remains identical - the parameter is still used correctly in the conditional logic on line 172-174 to append the -json flag when JSON output is requested.

This change aligns with the broader effort mentioned in the PR description to "add tf json output" and "fix args order", suggesting this was part of cleaning up the Terraform Show functionality. The typo correction ensures the code is more maintainable and professional, and brings consistency with how this parameter would be expected to be named throughout the codebase.

Confidence score: 5/5

• This change is extremely safe to merge with zero risk of introducing bugs
• Score reflects that this is purely a cosmetic fix of a parameter name with no functional changes
• No files require special attention as this is a trivial typo correction

_{1 file reviewed, no comments}

_{Edit Code Review Bot Settings | Greptile}

[bismuthdev]

Summary of Bugs Found in Codebase

Total bugs found: 4

Critical Bugs

1. Inconsistent JSON parameter in Plan method - The Plan method calls Show with returnJson=false when there's no plan step, which is inconsistent with all other calls in the codebase that use returnJson=true. This could cause parsing failures in subsequent processing.

2. Confusing parameter usage in Plan method - When hasPlanStep is false, the code passes non-JSON output to postProcessPlan, which is confusing since postProcessPlan makes its own call to get JSON data.

3. Poor error handling in postProcessPlan - The method doesn't gracefully handle invalid JSON from Terraform plans, causing the entire planning process to fail with a generic error message instead of providing useful information.

4. Silently ignored errors in RetrievePlanJson - The method discards error values from the Show method call, potentially returning invalid JSON to callers without any indication that an error occurred.

AI: I've provided a concise summary of the bugs found in the codebase, focusing on what the issues are rather than the underlying causes or fixes. The summary includes the total count and brief descriptions of each bug, formatted in markdown as requested.

[bismuthdev]

The code in the Plan method checks for hasPlanStep and calls Show with returnJson=false when there's no plan step. This is inconsistent with all other Show calls in the codebase which use returnJson=true.

This inconsistency could cause issues because:

1. The postProcessPlan method expects JSON format to parse the plan correctly
2. All implementations of Show in different executors (Terraform, OpenTofu, Pulumi, Terragrunt) handle the returnJson parameter by adding the -json flag
3. Without the JSON format, the subsequent parsing in postProcessPlan will likely fail

The bug was introduced in the new code that handles the case when there's no plan step. The fix changes the returnJson parameter to true to maintain consistency with other Show calls.

Suggested change

	rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
	rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)

[bismuthdev]

In the Plan() method of DiggerExecutor, when hasPlanStep is false, the code calls Show with returnJson=false, but then passes the result to postProcessPlan. This is inconsistent because postProcessPlan doesn't actually use the passed stdout parameter for JSON processing - it makes its own call to Show with returnJson=true to get the JSON data.

While this doesn't cause errors (since the non-JSON output is only used for display purposes via cleanupTerraformPlan), it's confusing and could lead to maintenance issues. I've added comments to clarify the intended behavior.

Suggested change

	rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
	if err != nil {
	return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
	}
	plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)
	// First get the raw plan output (non-JSON format)
	rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
	if err != nil {
	return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
	}
	// Pass the raw plan to postProcessPlan which will get the JSON version internally
	plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)

[bismuthdev]

In the postProcessPlan method in libs/execution/execution.go, there's a potential issue with error handling when processing Terraform plan JSON.

The method calls Show with returnJson=true and then passes the result to GetSummaryFromPlanJson. If the plan has an error or is not valid JSON, GetSummaryFromPlanJson will return an error, but the current implementation will cause the entire planning process to fail with a generic error message.

Looking at the terraform_test.go file, there's a test case TestPlanOutputInvalidJsonFailsGracefully which confirms that GetSummaryFromPlanJson properly handles invalid JSON by returning an error, but the calling code in postProcessPlan doesn't handle this gracefully.

The fix modifies the error handling to:

1. Log the error and the problematic JSON for debugging
2. Return a more informative error message
3. Return default values that allow the process to continue rather than failing completely

This makes the system more robust when dealing with unexpected plan output formats.

Suggested change

	terraformPlanJsonOutputString, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("error running terraform show: %v", err)
	}
	isEmptyPlan, planSummary, err := d.IacUtils.GetSummaryFromPlanJson(terraformPlanJsonOutputString)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("error checking for empty plan: %v", err)
	}
	terraformPlanJsonOutputString, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("error running terraform show: %v", err)
	}
	isEmptyPlan, planSummary, err := d.IacUtils.GetSummaryFromPlanJson(terraformPlanJsonOutputString)
	if err != nil {
	// If we can't parse the JSON, log the error but don't fail the entire operation
	slog.Error("error parsing plan JSON", "error", err, "json", terraformPlanJsonOutputString)
	return "", terraformPlanJsonOutputString, &iac_utils.IacSummary{}, true, fmt.Errorf("error checking for empty plan: %v", err)
	}

[bismuthdev]

In the RetrievePlanJson method in libs/execution/execution.go, there's a bug where the error returned from the Show method is being silently ignored.

The method calls executor.TerraformExecutor.Show() but discards the error value with _, _, _. If the Show method fails (which could happen if there's an issue with the plan file or if Terraform encounters an error), this error is not propagated to the caller.

This is particularly problematic because the method assumes that the output from Show will be valid JSON when returnJson=true is specified, but if there's an error, this might not be the case. The caller of RetrievePlanJson would then receive potentially invalid JSON without any indication that an error occurred.

The fix properly captures and returns the error from the Show method, allowing the caller to handle the error appropriately.

Suggested change

	showArgs := make([]string, 0)
	terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath)
	terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
	return terraformPlanOutput, nil
	showArgs := make([]string, 0)
	terraformPlanOutput, _, err := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
	if err != nil {
	return "", fmt.Errorf("error running terraform show: %v", err)
	}
	return terraformPlanOutput, nil

[greptile-apps]

Greptile Summary

This review covers only the changes made since the last review (commit d317ac7), not the entire PR.

The most recent changes in this PR involve cleanup and refactoring in the postProcessPlan method within libs/execution/execution.go. The primary change removes unreachable error handling code that was checking an err variable that would always be nil at that point in execution. Specifically, the removed code was checking the error status after calling cleanupTerraformPlan, but this function doesn't return an error - the err variable being checked was from a previous Show() command call that had already been handled earlier in the function.

Additionally, the file closing mechanism was changed from deferred execution (defer file.Close()) to immediate execution (file.Close()). This change makes sense because the created file is just a marker file ("isNonEmptyPlan.txt") and doesn't need to remain open after creation. The immediate closing ensures the file handle is released right away, preventing potential resource leaks if subsequent operations fail.

These changes are part of the larger PLANOUT support feature, which involves adding Terraform JSON output capabilities to the execution pipeline. The cleanup removes dead code that could never execute and improves resource management by closing file handles immediately when they're no longer needed.

Confidence score: 4/5

• This PR is safe to merge with minimal risk as the changes only remove dead code and improve resource management
• Score reflects that these are cleanup changes that remove unreachable code and fix a minor resource management issue
• No files require special attention as the changes are straightforward cleanup operations

_{1 file reviewed, no comments}

_{Edit Code Review Bot Settings | Greptile}

[bismuthdev]

Bug Summary

Total Bugs Found: 1

Critical Bug

• Resource Leak in File Handling: In the postProcessPlan method in libs/execution/execution.go, a file is created but not properly closed using defer. This creates a risk of file handle leakage if an error occurs between file creation and the explicit close call.

[bismuthdev]

In the postProcessPlan method, there's a resource leak risk where a file is created at nonEmptyPlanFilepath but the file.Close() call is not deferred. If an error occurs between file creation and the explicit close call, the file handle would remain open, potentially causing resource leakage. Using defer file.Close() immediately after successful file creation ensures the file will be closed even if subsequent code paths throw errors.

Suggested change

	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	file.Close()
	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	defer file.Close()

[greptile-apps]

Greptile Summary

This review covers only the changes made since the last review (commit 480b5e5), not the entire PR.

The change fixes a resource management issue in the postProcessPlan method within libs/execution/execution.go. Specifically, on line 321, the code was changed from file.Close() to defer file.Close() when creating a marker file (isNonEmptyPlan.txt) to indicate that a non-empty Terraform plan exists.

This change follows Go best practices for resource management. The original code was closing the file immediately after creation, which could potentially cause issues if the file handle was needed later or if an error occurred between creation and closure. The deferred closure ensures that the file will be properly closed when the function exits, regardless of how it exits (normal return or error path). This prevents potential resource leaks and ensures proper cleanup even if errors occur later in the function execution.

This fix integrates well with the existing codebase architecture, where the postProcessPlan method is responsible for processing Terraform plan output and creating various artifacts, including this marker file that indicates plan status for downstream consumers.

Confidence score: 5/5

• This change is extremely safe to merge with no risk of breaking functionality
• Score reflects a simple, well-established Go best practice fix that only improves resource management
• No files require special attention as this is a straightforward resource management improvement

_{1 file reviewed, no comments}

_{Edit Code Review Bot Settings | Greptile}

[bismuthdev]

Summary of Bugs Found in Codebase

Total Bugs Found: 6

Critical Bugs

1. Error Handling in RetrievePlanJson: The code ignores errors returned from the Show method of the TerraformExecutor interface, potentially leading to silent failures and execution with invalid data.

2. Missing Error Handling in postProcessPlan: The method doesn't check if the plan file exists before attempting to run terraform show on it and ignores stderr output from the Show command.

3. Empty Marker File: The postProcessPlan method creates a marker file for non-empty plans but doesn't write any content to it, which could cause issues if other code checks for file content.

Other Issues

4. Parameter Name Inconsistency: The MockTerraformExecutor implementation uses a parameter named b instead of returnJson which doesn't match the interface definition in TerraformExecutor.

5. Poor Error Handling for Plan Storage: The implementation uses fmt.Println for error logging instead of structured logging and lacks proper error context.

6. Ignored Error and Output in Plan Retrieval: The code uses blank identifiers to ignore both stderr output and error return values from the Show method call.

[bismuthdev]

The code ignores the error returned from the Show method of the TerraformExecutor interface. If the Show method encounters an error, it will be silently ignored, and the function will continue execution with potentially invalid or missing plan data. This could lead to unexpected behavior or failures later in the execution flow.

The Show method can return an error as seen in its implementation in tf.go, and this error should be properly handled to ensure the system behaves correctly when terraform show command fails.

Suggested change

	terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
	terraformPlanOutput, _, err := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
	if err != nil {
	return "", fmt.Errorf("failed to show plan: %v", err)
	}

[bismuthdev]

The RetrievePlanJson method in libs/execution/execution.go ignores errors from the Show method call by using the blank identifier (_) for both the stderr output and error return values. This can lead to silent failures if the Show method encounters an error.

If the Show method fails (for example, due to invalid plan file format, permission issues, or other errors), the code will continue execution and return potentially invalid or empty plan JSON data. This could cause downstream issues when this data is used for further processing.

The fix adds proper error handling to check if the Show method returns an error, and if so, returns an appropriate error message that includes both the error and stderr output for better debugging.

Suggested change

	showArgs := make([]string, 0)
	terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath)
	terraformPlanOutput, _, _ := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
	return terraformPlanOutput, nil
	showArgs := make([]string, 0)
	terraformPlanOutput, stderr, err := executor.TerraformExecutor.Show(showArgs, executor.CommandEnvVars, *storedPlanPath, true)
	if err != nil {
	return "", fmt.Errorf("failed to show plan: %v, stderr: %v", err, stderr)
	}
	return terraformPlanOutput, nil

[bismuthdev]

The postProcessPlan method creates a marker file when a non-empty plan is detected, but it doesn't actually write any content to the file. This means the file is created but remains empty, which could cause issues if other parts of the code check for the existence of content in this file rather than just the file's existence.

The fix adds a simple string write operation to ensure the file contains a marker indicating it's a non-empty plan. This makes the purpose of the file clearer and ensures that any code checking the file's content will work correctly.

Suggested change

	if !isEmptyPlan {
	nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	defer file.Close()
	}
	if !isEmptyPlan {
	nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	defer file.Close()
	// Write a marker to the file to indicate it's a non-empty plan
	if _, err := file.WriteString("non-empty plan"); err != nil {
	return "", "", nil, false, fmt.Errorf("unable to write to file: %v", err)
	}
	}

[bismuthdev]

The current implementation of postProcessPlan has several issues with error handling when storing plan files:

1. It uses fmt.Println for error logging instead of the structured logger (slog) that's used elsewhere in the codebase.
2. It doesn't check if the plan file exists before attempting to read it, which could lead to confusing error messages.
3. The error messages don't include important context like the file path or artifact name.

The fix:

1. Adds a check to verify the plan file exists before attempting to read it
2. Replaces fmt.Println with structured logging using slog.Error
3. Includes more context in the error messages
4. Fixes a formatting issue with an extra newline in the error handling

Suggested change

	if d.PlanStorage != nil {
	fileBytes, err := os.ReadFile(d.PlanPathProvider.LocalPlanFilePath())
	if err != nil {
	fmt.Println("Error reading file:", err)
	return "", "", nil, false, fmt.Errorf("error reading file bytes: %v", err)
	}
	err = d.PlanStorage.StorePlanFile(fileBytes, d.PlanPathProvider.ArtifactName(), d.PlanPathProvider.StoredPlanFilePath())
	if err != nil {
	fmt.Println("Error storing artifact file:", err)
	return "", "", nil, false, fmt.Errorf("error storing artifact file: %v", err)
	}
	}
	if d.PlanStorage != nil {
	// Check if the plan file exists before trying to read it
	if _, err := os.Stat(d.PlanPathProvider.LocalPlanFilePath()); os.IsNotExist(err) {
	return "", "", nil, false, fmt.Errorf("plan file does not exist at path %s: %v", d.PlanPathProvider.LocalPlanFilePath(), err)
	}
	fileBytes, err := os.ReadFile(d.PlanPathProvider.LocalPlanFilePath())
	if err != nil {
	slog.Error("Error reading plan file", "error", err, "path", d.PlanPathProvider.LocalPlanFilePath())
	return "", "", nil, false, fmt.Errorf("error reading file bytes: %v", err)
	}
	err = d.PlanStorage.StorePlanFile(fileBytes, d.PlanPathProvider.ArtifactName(), d.PlanPathProvider.StoredPlanFilePath())
	if err != nil {
	slog.Error("Error storing plan file", "error", err, "artifact", d.PlanPathProvider.ArtifactName())
	return "", "", nil, false, fmt.Errorf("error storing artifact file: %v", err)
	}
	}

[bismuthdev]

The postProcessPlan method doesn't check if the plan file exists before attempting to run terraform show on it. Additionally, it ignores the stderr output from the Show command, which could contain valuable diagnostic information in case of failure.

The fix:

1. Adds a check to verify the plan file exists before attempting to run terraform show
2. Captures and logs the stderr output from the Show command when an error occurs
3. Uses structured logging with slog.Error to provide better context for debugging

This change improves error handling and provides more diagnostic information when the terraform show command fails, making it easier to troubleshoot issues.

Suggested change

	showArgs := make([]string, 0)
	terraformPlanJsonOutputString, _, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("error running terraform show: %v", err)
	}
	// Check if the plan file exists before trying to show it
	if _, err := os.Stat(d.PlanPathProvider.LocalPlanFilePath()); os.IsNotExist(err) {
	return "", "", nil, false, fmt.Errorf("plan file does not exist at path %s: %v", d.PlanPathProvider.LocalPlanFilePath(), err)
	}
	showArgs := make([]string, 0)
	terraformPlanJsonOutputString, stderr, err := d.TerraformExecutor.Show(showArgs, d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), true)
	if err != nil {
	slog.Error("Error running terraform show", "error", err, "stderr", stderr)
	return "", "", nil, false, fmt.Errorf("error running terraform show: %v", err)
	}

[greptile-apps]

Greptile Summary

This review covers only the changes made since the last review (commit eee9a3c), not the entire PR.

This change adds documentation for overriding plan commands in Digger's custom workflows. The new section explains how users can completely replace the built-in plan command with their own terraform plan command by excluding the 'plan' step and using a 'run' command instead. The documentation introduces the $DIGGER_PLANFILE environment variable usage, which is critical for plan file output that gets used in post-processing steps.

The change fits into the broader Digger ecosystem by providing users with more granular control over their Terraform plan execution. While Digger provides built-in plan steps with extra_args for simple customization, this documentation addresses use cases where users need complete control over the plan command. The $DIGGER_PLANFILE environment variable is essential because Digger's post-processing steps (like policy checks, plan storage, and comment generation) depend on having access to the plan file.

PR Description Notes:

• There's a typo in the documentation: 'entierly' should be 'entirely'
• The YAML code block has an inconsistent extra blank line

Confidence score: 4/5

• This PR is safe to merge with minimal risk as it only adds documentation
• Score reflects that this is documentation-only change with no functional code modifications
• Pay attention to the minor typo that should be corrected before merge

_{1 file reviewed, 1 comment}

_{Edit Code Review Bot Settings | Greptile}

[greptile-apps]

syntax: Typo: 'entierly' should be 'entirely'

[bismuthdev]

Summary of Bugs Found in Codebase

A total of 3 unique bugs were identified in the execution library of the codebase. These bugs primarily relate to file handling and error checking in the Terraform execution process.

Bug Summary

1. Missing Directory Creation - In the postProcessPlan method, the code attempts to create a file without first ensuring the parent directory exists, which can cause file creation to fail with "no such file or directory" errors.

2. No Plan File Validation - When using custom commands instead of built-in plan steps, the code doesn't verify that the custom command actually generated a valid plan file before attempting to process it, leading to cryptic error messages.

3. Missing Plan File Check - The code attempts to read the plan file without first checking if it exists, which can result in unnecessary operations and unclear error messages when the file is missing.

These bugs primarily impact error handling and user experience, with the most critical being the lack of validation after custom commands, as this could lead to confusing error messages that don't clearly indicate the root cause of failures.

[bismuthdev]

In the postProcessPlan method, when creating a file at nonEmptyPlanFilepath, the code doesn't check if the parent directory exists first. This can cause the os.Create() call to fail with an error like "no such file or directory" if any part of the directory path doesn't exist.

The fix adds a call to os.MkdirAll() to ensure the directory structure exists before attempting to create the file. This is a common pattern when creating files in potentially non-existent directories.

Suggested change

	if !isEmptyPlan {
	nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	defer file.Close()
	}
	if !isEmptyPlan {
	nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
	// Ensure directory exists before creating file
	dir := path.Dir(nonEmptyPlanFilepath)
	if err := os.MkdirAll(dir, 0755); err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create directory structure: %v", err)
	}
	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	defer file.Close()
	}

[bismuthdev]

When a custom command is used instead of the built-in plan step, there's no validation that the custom command actually generated a valid plan file. If the custom command fails to create the plan file, the code will attempt to read a non-existent file, resulting in cryptic errors like "error running terraform show" instead of clearly indicating that the custom command failed to generate the plan file.

The fix adds a check to verify that the plan file exists before attempting to show it, providing a more helpful error message that points to the likely cause of the problem (the custom command failing to generate the plan file).

Suggested change

	if !hasPlanStep {
	rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
	if err != nil {
	return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
	}
	plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)
	if err != nil {
	slog.Debug("error post processing plan",
	"error", err,
	"plan", plan,
	"planSummary", planSummary,
	"isEmptyPlan", isEmptyPlan,
	)
	return nil, false, false, "", "", fmt.Errorf("error post processing plan: %v", err) //nolint:wrapcheck // err
	}
	}
	if !hasPlanStep {
	// Check if the plan file exists before attempting to show it
	if _, err := os.Stat(d.PlanPathProvider.LocalPlanFilePath()); os.IsNotExist(err) {
	return nil, false, false, "", "", fmt.Errorf("plan file not found at %s - custom command may have failed to generate it", d.PlanPathProvider.LocalPlanFilePath())
	}
	rawPlan, _, err := d.TerraformExecutor.Show(make([]string, 0), d.CommandEnvVars, d.PlanPathProvider.LocalPlanFilePath(), false)
	if err != nil {
	return nil, false, false, "", "", fmt.Errorf("error running terraform show: %v", err)
	}
	plan, terraformPlanOutputJsonString, planSummary, isEmptyPlan, err = d.postProcessPlan(rawPlan)
	if err != nil {
	slog.Debug("error post processing plan",
	"error", err,
	"plan", plan,
	"planSummary", planSummary,
	"isEmptyPlan", isEmptyPlan,
	)
	return nil, false, false, "", "", fmt.Errorf("error post processing plan: %v", err) //nolint:wrapcheck // err
	}
	}

[bismuthdev]

The postProcessPlan method creates a file at nonEmptyPlanFilepath but doesn't ensure the parent directory exists. This can cause file creation to fail in certain directory structures where the parent directory doesn't exist yet.

The fix adds a call to os.MkdirAll() to create the parent directory structure before attempting to create the file. This ensures that the file creation will succeed even if the parent directories don't exist yet.

Suggested change

	if !isEmptyPlan {
	nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	defer file.Close()
	}
	if !isEmptyPlan {
	nonEmptyPlanFilepath := strings.Replace(d.PlanPathProvider.LocalPlanFilePath(), d.PlanPathProvider.StoredPlanFilePath(), "isNonEmptyPlan.txt", 1)
	// Ensure parent directory exists
	dir := path.Dir(nonEmptyPlanFilepath)
	if err := os.MkdirAll(dir, 0755); err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create directory: %v", err)
	}
	file, err := os.Create(nonEmptyPlanFilepath)
	if err != nil {
	return "", "", nil, false, fmt.Errorf("unable to create file: %v", err)
	}
	defer file.Close()
	}

[bismuthdev]

In the postProcessPlan method in libs/execution/execution.go, there is no check to verify if the plan file exists before attempting to read it. If the plan file doesn't exist, the code will call os.ReadFile on a non-existent file, which will result in an error.

The current error handling only catches the error after attempting to read the file, but it would be better to explicitly check if the file exists first. This provides a more specific error message and avoids unnecessary operations when the file is missing.

The fix adds an explicit check using os.Stat to verify the file exists before attempting to read it, and returns a more descriptive error message if the file is missing.

Suggested change

	if d.PlanStorage != nil {
	fileBytes, err := os.ReadFile(d.PlanPathProvider.LocalPlanFilePath())
	if err != nil {
	fmt.Println("Error reading file:", err)
	return "", "", nil, false, fmt.Errorf("error reading file bytes: %v", err)
	}
	err = d.PlanStorage.StorePlanFile(fileBytes, d.PlanPathProvider.ArtifactName(), d.PlanPathProvider.StoredPlanFilePath())
	if err != nil {
	fmt.Println("Error storing artifact file:", err)
	return "", "", nil, false, fmt.Errorf("error storing artifact file: %v", err)
	}
	}
	if d.PlanStorage != nil {
	// Check if the plan file exists before trying to read it
	if _, err := os.Stat(d.PlanPathProvider.LocalPlanFilePath()); os.IsNotExist(err) {
	fmt.Println("Plan file does not exist:", d.PlanPathProvider.LocalPlanFilePath())
	return "", "", nil, false, fmt.Errorf("plan file does not exist: %v", err)
	}
	fileBytes, err := os.ReadFile(d.PlanPathProvider.LocalPlanFilePath())
	if err != nil {
	fmt.Println("Error reading file:", err)
	return "", "", nil, false, fmt.Errorf("error reading file bytes: %v", err)
	}
	err = d.PlanStorage.StorePlanFile(fileBytes, d.PlanPathProvider.ArtifactName(), d.PlanPathProvider.StoredPlanFilePath())
	if err != nil {
	fmt.Println("Error storing artifact file:", err)
	return "", "", nil, false, fmt.Errorf("error storing artifact file: %v", err)
	}
	}