chore: Moving envs and writers to venv#6092
Open
yhakbar wants to merge 2 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThis PR moves runtime env and stdout/stderr handling from options and exec-specific paths into ChangesRuntime venv refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
yhakbar
force-pushed
the
chore/moving-envs-to-venv
branch
from
4896ef3 to
4881e82
Compare
yhakbar
force-pushed
the
chore/moving-envs-to-venv
branch
from
4881e82 to
9e3b692
Compare
yhakbar
force-pushed
the
chore/expand-pure-testing-through-venv
branch
from
aa0c724 to
286d5b0
Compare
yhakbar
force-pushed
the
chore/expand-pure-testing-through-venv
branch
3 times, most recently
from
e9b5ad2 to
17510c1
Compare
yhakbar
force-pushed
the
chore/expand-pure-testing-through-venv
branch
from
17510c1 to
124f427
Compare
yhakbar
force-pushed
the
chore/moving-envs-to-venv
branch
from
337a979 to
0ff1991
Compare
yhakbar
force-pushed
the
chore/moving-envs-to-venv
branch
from
2833f7e to
6a3ac53
Compare
yhakbar
force-pushed
the
chore/moving-envs-to-venv
branch
from
6a3ac53 to
f89d0f7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/options/options.go (1)
79-300:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep a compatibility path for the exported options API.
pkg/optionsis importable API surface, and this change removes exportedEnv/Writers, removesNewTerragruntOptionsWithWriters, and changesTerraformDataDir/DataDircall signatures. That will break downstream builds despite the PR being marked backwards compatible; keep deprecated shims for one release or call this out as an intentional breaking change with the appropriate versioning/migration plan.Also applies to: 320-348, 539-559
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/options/options.go` around lines 79 - 300, The TerragruntOptions struct is removing exported API elements (Env, Writers fields, NewTerragruntOptionsWithWriters function, and TerraformDataDir/DataDir method signatures) that downstream users may depend on, which breaks backwards compatibility. Restore these removed exported fields and functions as deprecated shims that maintain the original signatures and delegate to the new internal implementations, and add deprecation warnings or comments to guide users toward the new API. This preserves the public API surface for at least one release while allowing the transition to the new structure.internal/runner/run/options.go (1)
231-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward the log flags into backend options.
Line 233 builds
backend.Optionsbut leavesLogShowAbsPathsandLogDisableErrorSummaryat their zero values, unlikeconfigbridge.BackendOptsFromOpts. Backend operations reached throughrun.Options.remoteStateOptswill ignore those CLI settings.Proposed fix
Options: backend.Options{ Writers: v.Writers, Env: v.Env, IAMRoleOptions: o.IAMRoleOptions, NonInteractive: o.NonInteractive, FailIfBucketCreationRequired: o.FailIfBucketCreationRequired, + LogShowAbsPaths: o.LogShowAbsPaths, + LogDisableErrorSummary: o.LogDisableErrorSummary, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/runner/run/options.go` around lines 231 - 239, The remoteStateOpts method is not forwarding the log configuration flags to the backend.Options struct being created. Add LogShowAbsPaths and LogDisableErrorSummary fields to the backend.Options initialization within the remoteStateOpts method, setting these fields from the corresponding values in the Options receiver (o), similar to how other options like IAMRoleOptions, NonInteractive, and FailIfBucketCreationRequired are already being forwarded.pkg/config/dependency.go (1)
1470-1550:⚠️ Potential issue | 🟡 MinorRemove redundant
WithWritercall at line 1532.Line 1480 sets
pctx.Venv = pctx.Venv.WithWriter(stdoutBufferWriter)after cloning, andpctx.Venvis not modified between that assignment and line 1532. SinceWithWriterreturns a new Venv (not mutating the receiver), line 1532'srunV := pctx.Venv.WithWriter(stdoutBufferWriter)appliesWithWriterto a venv that already hasstdoutBufferWriterset, creating a redundant duplicate writer assignment. Line 1532 should berunV := pctx.Venv.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/dependency.go` around lines 1470 - 1550, The WithWriter method is being applied redundantly to pctx.Venv. At the beginning of the runTerragruntOutputJSON function, pctx.Venv is already updated with stdoutBufferWriter via pctx.Venv = pctx.Venv.WithWriter(stdoutBufferWriter). Since WithWriter returns a new Venv without mutating the original, and pctx.Venv is not modified between that assignment and line 1532, the subsequent call runV := pctx.Venv.WithWriter(stdoutBufferWriter) applies the writer assignment twice. Remove the redundant WithWriter call on line 1532 and simply assign runV := pctx.Venv instead, since pctx.Venv already has the correct writer configured.
🧹 Nitpick comments (7)
internal/cli/commands/commands.go (1)
327-342: ⚡ Quick winConsider returning an error instead of panicking.
The function panics if
v.Envisnil, but since this is called fromRunAction(line 261) which already has error-handling infrastructure, returning an error would be more idiomatic Go and allow graceful degradation.-// Panics if v.Env is nil, as it mutates it. func setupAutoProviderCacheDir(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, v venv.Venv) error { if v.Env == nil { - panic("setupAutoProviderCacheDir: venv environment map is nil") + return errors.New("setupAutoProviderCacheDir: venv environment map is nil") }Panics in Go are typically reserved for truly unrecoverable programming errors. A nil environment map, while unexpected, can be handled gracefully by returning an error that the caller can log and handle appropriately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/commands/commands.go` around lines 327 - 342, Replace the panic statement in the setupAutoProviderCacheDir function with a proper error return. Instead of panicking when v.Env is nil, return a formatted error (using fmt.Errorf or similar) with a descriptive message about the nil environment map. This allows the calling code in RunAction to handle the error gracefully through the existing error-handling infrastructure rather than crashing the application.internal/cli/commands/hcl/validate/validate.go (1)
335-403: 💤 Low valueConsider refactoring to reduce cognitive complexity.
The function has multiple validation paths with nested conditionals (missing/unused vars, strict mode logic, error assembly). While the current structure is acceptable for a validation function, breaking it into smaller helpers could improve readability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/commands/hcl/validate/validate.go` around lines 335 - 403, The runValidateInputs function has multiple nested validation paths and conditional blocks that can be simplified. Extract the unused variable detection loop into a helper function, extract the missing variable detection loop into a separate helper function, create a helper function for logging the validation results (handling both missing and unused vars cases), and create a helper function for the error determination logic that checks strict mode. Then refactor runValidateInputs to call these helpers in sequence, reducing the overall cognitive complexity and improving readability while maintaining the same behavior.Source: Linters/SAST tools
pkg/config/config_helpers_test.go (1)
639-642: ⚡ Quick winConsider reordering venv initialization for clarity.
The current initialization sets
pctx.Venv.Envon line 639, then reassigns the entirepctx.Venvstruct on line 642 via theWithWriter(...).WithErrWriter(...)chain. While this works if those methods preserve fields via shallow copying, the ordering makes the code harder to reason about.♻️ Suggested reordering
- pctx.Venv.Env = map[string]string{} pctx.SourceMap = map[string]string{} pctx.TerraformCliArgs = iacargs.New() pctx.Venv = pctx.Venv.WithWriter(os.Stdout).WithErrWriter(os.Stderr) + pctx.Venv.Env = map[string]string{}Or initialize in a single struct literal:
- pctx.Venv.Env = map[string]string{} pctx.SourceMap = map[string]string{} pctx.TerraformCliArgs = iacargs.New() - pctx.Venv = pctx.Venv.WithWriter(os.Stdout).WithErrWriter(os.Stderr) + pctx.Venv = venv.Venv{ + Env: map[string]string{}, + Writers: writer.Writers{Writer: os.Stdout, ErrWriter: os.Stderr}, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/config_helpers_test.go` around lines 639 - 642, The pctx.Venv initialization sets the Env field first on the initial struct, then immediately reassigns the entire pctx.Venv via the WithWriter and WithErrWriter method chain, which could overwrite the previously set Env field if those methods don't preserve it. Reorder the initialization by first assigning pctx.Venv with the complete WithWriter(os.Stdout).WithErrWriter(os.Stderr) chain, then set pctx.Venv.Env on the resulting venv instance after the method chain completes. This makes the initialization order clearer and ensures the Env field is not overwritten.internal/discovery/phase_worktree.go (1)
65-172: 🏗️ Heavy liftSplit
Runinto smaller phase steps.Sonar flags
WorktreePhase.Runat Line 65 for cognitive complexity 43 > 15. Extracting pair expansion/discovery and final classification into helpers would keep the venv threading intact while satisfying the quality gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/discovery/phase_worktree.go` around lines 65 - 172, The WorktreePhase.Run method has excessive cognitive complexity (43 > 15) due to nested logic for pair discovery, stack changes discovery, and classification. Extract the discovery orchestration into a separate helper method that handles setting up the error group, looping through worktree pairs with discoverInWorktree calls, and invoking discoverChangesInWorktreeStacks, returning the discoveredComponents collection. Then extract the classification and result building logic into another helper method that iterates through the discovered components, invokes input.Classifier.Classify for each component, constructs the DiscoveryResult objects, and applies the switch statement to add results. This separation will reduce the Run method's complexity while preserving the venv threading behavior via the passed context and logger parameters.Source: Linters/SAST tools
internal/providercache/providercache.go (1)
505-580: ⚡ Quick winExtract the retry attempt body to satisfy the complexity gate.
Sonar flags
runTerraformCommandat Line 505 for cognitive complexity 18 > 15. Pulling the per-attempt command setup/error classification/flush handling into a small helper should preserve behavior and unblock analysis.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providercache/providercache.go` around lines 505 - 580, The runTerraformCommand function exceeds the cognitive complexity threshold (18 > 15) due to the complex error handling and retry logic within the closure passed to util.DoWithRetry. Extract the entire closure body that handles error writer setup, command execution, error classification (httpStatusCacheProviderReg matching, timeout detection), and flushing operations into a separate private helper function. This helper should take the necessary parameters (ctx, logger, venv, TFOptions, CLI args) and return the command output and error, then call this helper from within the util.DoWithRetry closure to maintain behavior while reducing cognitive complexity.Source: Linters/SAST tools
internal/discovery/phase_graph.go (1)
66-171: 🏗️ Heavy liftReduce
GraphPhase.Runcomplexity to clear the SonarCloud gate.SonarCloud reports cognitive complexity 29 vs the allowed 15 here. Extract candidate partitioning and graph-task scheduling/error aggregation so the new venv threading stays reviewable and the check turns green.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/discovery/phase_graph.go` around lines 66 - 171, The GraphPhase.Run method has excessive cognitive complexity (29 vs the allowed 15). Extract the candidate partitioning logic that separates input.Candidates into graphTargetCandidates and otherCandidates (the switch statement on candidate.Reason) into a separate helper method. Additionally, extract the graph-task scheduling and error aggregation logic that handles the errgroup setup, iterates over graphExprs and matchingCandidates, and collects errors (from the g.Go call through the final error handling) into another separate helper method. This will reduce the complexity of the main Run method while keeping the new threading logic reviewable and maintainable.Source: Linters/SAST tools
internal/discovery/phase_parse.go (1)
159-238: ⚡ Quick winSplit
ParsePhase.Runbefore merging to satisfy the complexity check.SonarCloud reports cognitive complexity 20 vs the allowed 15. Extracting parse-queue construction and result/error handling should be a localized fix while preserving the new venv propagation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/discovery/phase_parse.go` around lines 159 - 238, The ParsePhase.Run method has cognitive complexity exceeding the allowed threshold. Extract the logic that builds the componentsToParse queue (the section filtering candidates and checking discovery.readFiles, discovery.parseExclude, discovery.parseIncludes conditions) into a separate private helper method that returns the queue of components to parse. Additionally, extract the goroutine processing logic that handles the switch statement for result.Status and error accumulation into another separate private helper method that takes the parsed result and appends to results. This will simplify the main Run method by reducing nested conditionals and improving readability while preserving the venv propagation.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/runner/run/creds/providers/externalcmd/provider.go`:
- Around line 203-205: In the amazonsts.NewProvider call within the code block
around the provider initialization, replace the nil argument (third parameter)
with v.Env to ensure that the environment variables from the venv context are
properly passed to the STS provider for role assumption, allowing the external
auth command responses to use the correct runtime environment instead of
bypassing the venv-backed environment.
In `@internal/shell/run_cmd.go`:
- Around line 258-262: The code directly mutates the shared Env map in v (at
line 261 with v.Env[telemetry.TraceParentEnv] = traceParent) which causes race
conditions when parallel commands execute concurrently and also panics if Env is
nil. Create a per-command copy of the environment map before injecting the
TRACEPARENT value, ensuring to handle the nil case by initializing an empty map
when needed. Apply this same pattern to all three locations where TraceParent is
injected (around lines 258-262, 275-284, and 304-310), and use the copied env
map for both the engine and exec execution paths instead of modifying the
original v.Env directly.
In `@pkg/config/config_helpers.go`:
- Around line 448-456: The stderr replay logic in the replayOnce.Do block is
nested within the stdout writer condition, causing cached stderr to be lost when
stdout is discarded. Move the stderr replay logic (checking cachedEntry.Stderr
and pctx.Venv.Writers.ErrWriter) outside of the stdout writer condition so it
executes independently. Additionally, in the second location around lines
498-505, avoid writing stderr again to ErrWriter if it has already been streamed
by RunCommandWithOutput, as this causes duplicate output to the user.
In `@pkg/config/parsing_context.go`:
- Around line 39-45: The ParsingContext struct is exported from pkg/config and
the refactor removes the exported Env field while moving the source of truth to
Venv.Env, which breaks downstream code that constructs or reads
ParsingContext.Env. To maintain backward compatibility, retain the Env field on
the ParsingContext struct as a deprecated field and ensure it is synchronized
with Venv.Env by updating any methods that modify either field to keep them in
sync, or create getter and setter methods that bridge between ParsingContext.Env
and Venv.Env. Alternatively, if accepting this as a breaking API change,
document and communicate this clearly before merging.
---
Outside diff comments:
In `@internal/runner/run/options.go`:
- Around line 231-239: The remoteStateOpts method is not forwarding the log
configuration flags to the backend.Options struct being created. Add
LogShowAbsPaths and LogDisableErrorSummary fields to the backend.Options
initialization within the remoteStateOpts method, setting these fields from the
corresponding values in the Options receiver (o), similar to how other options
like IAMRoleOptions, NonInteractive, and FailIfBucketCreationRequired are
already being forwarded.
In `@pkg/config/dependency.go`:
- Around line 1470-1550: The WithWriter method is being applied redundantly to
pctx.Venv. At the beginning of the runTerragruntOutputJSON function, pctx.Venv
is already updated with stdoutBufferWriter via pctx.Venv =
pctx.Venv.WithWriter(stdoutBufferWriter). Since WithWriter returns a new Venv
without mutating the original, and pctx.Venv is not modified between that
assignment and line 1532, the subsequent call runV :=
pctx.Venv.WithWriter(stdoutBufferWriter) applies the writer assignment twice.
Remove the redundant WithWriter call on line 1532 and simply assign runV :=
pctx.Venv instead, since pctx.Venv already has the correct writer configured.
In `@pkg/options/options.go`:
- Around line 79-300: The TerragruntOptions struct is removing exported API
elements (Env, Writers fields, NewTerragruntOptionsWithWriters function, and
TerraformDataDir/DataDir method signatures) that downstream users may depend on,
which breaks backwards compatibility. Restore these removed exported fields and
functions as deprecated shims that maintain the original signatures and delegate
to the new internal implementations, and add deprecation warnings or comments to
guide users toward the new API. This preserves the public API surface for at
least one release while allowing the transition to the new structure.
---
Nitpick comments:
In `@internal/cli/commands/commands.go`:
- Around line 327-342: Replace the panic statement in the
setupAutoProviderCacheDir function with a proper error return. Instead of
panicking when v.Env is nil, return a formatted error (using fmt.Errorf or
similar) with a descriptive message about the nil environment map. This allows
the calling code in RunAction to handle the error gracefully through the
existing error-handling infrastructure rather than crashing the application.
In `@internal/cli/commands/hcl/validate/validate.go`:
- Around line 335-403: The runValidateInputs function has multiple nested
validation paths and conditional blocks that can be simplified. Extract the
unused variable detection loop into a helper function, extract the missing
variable detection loop into a separate helper function, create a helper
function for logging the validation results (handling both missing and unused
vars cases), and create a helper function for the error determination logic that
checks strict mode. Then refactor runValidateInputs to call these helpers in
sequence, reducing the overall cognitive complexity and improving readability
while maintaining the same behavior.
In `@internal/discovery/phase_graph.go`:
- Around line 66-171: The GraphPhase.Run method has excessive cognitive
complexity (29 vs the allowed 15). Extract the candidate partitioning logic that
separates input.Candidates into graphTargetCandidates and otherCandidates (the
switch statement on candidate.Reason) into a separate helper method.
Additionally, extract the graph-task scheduling and error aggregation logic that
handles the errgroup setup, iterates over graphExprs and matchingCandidates, and
collects errors (from the g.Go call through the final error handling) into
another separate helper method. This will reduce the complexity of the main Run
method while keeping the new threading logic reviewable and maintainable.
In `@internal/discovery/phase_parse.go`:
- Around line 159-238: The ParsePhase.Run method has cognitive complexity
exceeding the allowed threshold. Extract the logic that builds the
componentsToParse queue (the section filtering candidates and checking
discovery.readFiles, discovery.parseExclude, discovery.parseIncludes conditions)
into a separate private helper method that returns the queue of components to
parse. Additionally, extract the goroutine processing logic that handles the
switch statement for result.Status and error accumulation into another separate
private helper method that takes the parsed result and appends to results. This
will simplify the main Run method by reducing nested conditionals and improving
readability while preserving the venv propagation.
In `@internal/discovery/phase_worktree.go`:
- Around line 65-172: The WorktreePhase.Run method has excessive cognitive
complexity (43 > 15) due to nested logic for pair discovery, stack changes
discovery, and classification. Extract the discovery orchestration into a
separate helper method that handles setting up the error group, looping through
worktree pairs with discoverInWorktree calls, and invoking
discoverChangesInWorktreeStacks, returning the discoveredComponents collection.
Then extract the classification and result building logic into another helper
method that iterates through the discovered components, invokes
input.Classifier.Classify for each component, constructs the DiscoveryResult
objects, and applies the switch statement to add results. This separation will
reduce the Run method's complexity while preserving the venv threading behavior
via the passed context and logger parameters.
In `@internal/providercache/providercache.go`:
- Around line 505-580: The runTerraformCommand function exceeds the cognitive
complexity threshold (18 > 15) due to the complex error handling and retry logic
within the closure passed to util.DoWithRetry. Extract the entire closure body
that handles error writer setup, command execution, error classification
(httpStatusCacheProviderReg matching, timeout detection), and flushing
operations into a separate private helper function. This helper should take the
necessary parameters (ctx, logger, venv, TFOptions, CLI args) and return the
command output and error, then call this helper from within the util.DoWithRetry
closure to maintain behavior while reducing cognitive complexity.
In `@pkg/config/config_helpers_test.go`:
- Around line 639-642: The pctx.Venv initialization sets the Env field first on
the initial struct, then immediately reassigns the entire pctx.Venv via the
WithWriter and WithErrWriter method chain, which could overwrite the previously
set Env field if those methods don't preserve it. Reorder the initialization by
first assigning pctx.Venv with the complete
WithWriter(os.Stdout).WithErrWriter(os.Stderr) chain, then set pctx.Venv.Env on
the resulting venv instance after the method chain completes. This makes the
initialization order clearer and ensures the Env field is not overwritten.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99c263cb-2ece-49a6-bcf5-832ae18138bb
📒 Files selected for processing (139)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/commands/aws-provider-patch/aws-provider-patch.gointernal/cli/commands/backend/bootstrap/bootstrap.gointernal/cli/commands/backend/bootstrap/cli.gointernal/cli/commands/backend/cli.gointernal/cli/commands/backend/delete/cli.gointernal/cli/commands/backend/delete/delete.gointernal/cli/commands/backend/migrate/migrate.gointernal/cli/commands/catalog/catalog.gointernal/cli/commands/catalog/tui/scaffold.gointernal/cli/commands/catalog/tui/welcome.gointernal/cli/commands/commands.gointernal/cli/commands/dag/cli.gointernal/cli/commands/dag/graph/cli.gointernal/cli/commands/dag/graph/cli_test.gointernal/cli/commands/exec/exec.gointernal/cli/commands/find/cli.gointernal/cli/commands/find/find.gointernal/cli/commands/find/find_test.gointernal/cli/commands/hcl/cli.gointernal/cli/commands/hcl/format/cli.gointernal/cli/commands/hcl/format/format.gointernal/cli/commands/hcl/format/format_bench_test.gointernal/cli/commands/hcl/format/format_test.gointernal/cli/commands/hcl/validate/validate.gointernal/cli/commands/info/print/print.gointernal/cli/commands/list/cli.gointernal/cli/commands/list/list.gointernal/cli/commands/list/list_test.gointernal/cli/commands/render/render.gointernal/cli/commands/render/render_test.gointernal/cli/commands/run/cli.gointernal/cli/commands/run/help.gointernal/cli/commands/run/run.gointernal/cli/commands/scaffold/scaffold.gointernal/cli/commands/stack/cli.gointernal/cli/commands/stack/stack.gointernal/cli/flags/global/flags.gointernal/configbridge/bridge.gointernal/discovery/benchmark_test.gointernal/discovery/constructor.gointernal/discovery/discovery.gointernal/discovery/discovery_integration_test.gointernal/discovery/discovery_test.gointernal/discovery/filter_test.gointernal/discovery/graph_target_test.gointernal/discovery/helpers.gointernal/discovery/options.gointernal/discovery/phase_filesystem.gointernal/discovery/phase_graph.gointernal/discovery/phase_parse.gointernal/discovery/phase_relationship.gointernal/discovery/phase_test.gointernal/discovery/phase_worktree.gointernal/discovery/phase_worktree_integration_test.gointernal/discovery/types.gointernal/engine/engine.gointernal/git/git.gointernal/prepare/prepare.gointernal/providercache/providercache.gointernal/remotestate/backend/backend.gointernal/remotestate/remote_state.gointernal/runner/common/unit_runner.gointernal/runner/graph/graph.gointernal/runner/run/creds/getter.gointernal/runner/run/creds/providers/amazonsts/provider.gointernal/runner/run/creds/providers/externalcmd/provider.gointernal/runner/run/creds/providers/externalcmd/provider_mem_test.gointernal/runner/run/creds/providers/externalcmd/provider_test.gointernal/runner/run/creds/providers/provider.gointernal/runner/run/debug.gointernal/runner/run/download_source.gointernal/runner/run/download_source_test.gointernal/runner/run/hook.gointernal/runner/run/hook_lifecycle_test.gointernal/runner/run/hook_test.gointernal/runner/run/options.gointernal/runner/run/prepare_internal_test.gointernal/runner/run/run.gointernal/runner/run/run_e2e_test.gointernal/runner/run/run_test.gointernal/runner/run/venv.gointernal/runner/run/version_check.gointernal/runner/run/version_check_mem_test.gointernal/runner/run/version_check_test.gointernal/runner/runall/runall.gointernal/runner/runner.gointernal/runner/runnerpool/builder.gointernal/runner/runnerpool/builder_helpers.gointernal/runner/runnerpool/runner.gointernal/runner/runnerpool/writer.gointernal/shell/git.gointernal/shell/git_mem_test.gointernal/shell/git_windows_test.gointernal/shell/run_cmd.gointernal/shell/run_cmd_mem_test.gointernal/shell/run_cmd_output_test.gointernal/shell/run_cmd_test.gointernal/shell/run_cmd_unix_test.gointernal/shell/run_cmd_windows_test.gointernal/stacks/generate/generate.gointernal/stacks/output/output.gointernal/strict/view/plaintext/render.gointernal/strict/view/plaintext/render_internal_test.gointernal/telemetry/meter_test.gointernal/telemetry/telemeter_test.gointernal/telemetry/tracer_test.gointernal/tf/context.gointernal/tf/run_cmd.gointernal/tf/run_cmd_test.gointernal/tflint/tflint.gointernal/tflint/tflint_test.gointernal/tflint/venv.gointernal/util/env.gointernal/venv/venv.gointernal/writer/writer.gomain.gopkg/config/config.gopkg/config/config_helpers.gopkg/config/config_helpers_mem_test.gopkg/config/config_helpers_test.gopkg/config/dependency.gopkg/config/dependency_test.gopkg/config/early_stack_eval_test.gopkg/config/fuzz_test.gopkg/config/include.gopkg/config/locals.gopkg/config/parsing_context.gopkg/config/sops_race_test.gopkg/config/sops_test.gopkg/config/stack.gopkg/options/options.gopkg/options/options_test.gotest/benchmarks/helpers/helpers.gotest/helpers/package.gotest/integration_aws_test.gotest/integration_debug_test.gotest/integration_gcp_test.go
💤 Files with no reviewable changes (2)
- internal/util/env.go
- internal/discovery/options.go
Comment on lines
203
to
+205
| provider := amazonsts.NewProvider(l, iamRoleOpts, nil) | ||
|
|
||
| creds, err := provider.GetCredentials(ctx, l, exec) | ||
| creds, err := provider.GetCredentials(ctx, l, v) |
Contributor
There was a problem hiding this comment.
Pass v.Env into the nested STS provider.
amazonsts.Provider.GetCredentials ignores the venv.Venv argument and reads only the env map captured by amazonsts.NewProvider; constructing it with nil means awsRole responses from the external auth command can bypass the venv-backed environment. Use v.Env here so role assumption uses the same runtime env.
Proposed fix
- provider := amazonsts.NewProvider(l, iamRoleOpts, nil)
+ provider := amazonsts.NewProvider(l, iamRoleOpts, v.Env)
creds, err := provider.GetCredentials(ctx, l, v)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/runner/run/creds/providers/externalcmd/provider.go` around lines 203
- 205, In the amazonsts.NewProvider call within the code block around the
provider initialization, replace the nil argument (third parameter) with v.Env
to ensure that the environment variables from the venv context are properly
passed to the STS provider for role assumption, allowing the external auth
command responses to use the correct runtime environment instead of bypassing
the venv-backed environment.
Comment on lines
258
to
262
| // Pass the traceparent to the child process if it is available in the context. | ||
| if traceParent := telemetry.TraceParentFromContext(ctx, runOpts.Telemetry); traceParent != "" { | ||
| l.Debugf("Setting trace parent=%q for command %s", traceParent, fmt.Sprintf("%s %v", cmdOpts.Command, cmdOpts.Args)) | ||
| runOpts.Env[telemetry.TraceParentEnv] = traceParent | ||
| v.Env[telemetry.TraceParentEnv] = traceParent | ||
| } |
Contributor
There was a problem hiding this comment.
Copy the command env before injecting TRACEPARENT.
Line 261 mutates v.Env directly. Since Venv is copied by value but Env is a shared map, parallel commands can race/panic with concurrent map writes; a nil Env also panics when a trace parent is present. Build a per-command env map and pass that to both engine and exec paths.
🐛 Proposed fix
// Pass the traceparent to the child process if it is available in the context.
+ cmdEnv := v.Env
if traceParent := telemetry.TraceParentFromContext(ctx, runOpts.Telemetry); traceParent != "" {
l.Debugf("Setting trace parent=%q for command %s", traceParent, fmt.Sprintf("%s %v", cmdOpts.Command, cmdOpts.Args))
- v.Env[telemetry.TraceParentEnv] = traceParent
+ cmdEnv = make(map[string]string, len(v.Env)+1)
+ for key, value := range v.Env {
+ cmdEnv[key] = value
+ }
+ cmdEnv[telemetry.TraceParentEnv] = traceParent
}
@@
- Env: v.Env,
+ Env: cmdEnv,
@@
- exec.WithEnv(v.Env),
+ exec.WithEnv(cmdEnv),Also applies to: 275-284, 304-310
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/shell/run_cmd.go` around lines 258 - 262, The code directly mutates
the shared Env map in v (at line 261 with v.Env[telemetry.TraceParentEnv] =
traceParent) which causes race conditions when parallel commands execute
concurrently and also panics if Env is nil. Create a per-command copy of the
environment map before injecting the TRACEPARENT value, ensuring to handle the
nil case by initializing an empty map when needed. Apply this same pattern to
all three locations where TraceParent is injected (around lines 258-262,
275-284, and 304-310), and use the copied env map for both the engine and exec
execution paths instead of modifying the original v.Env directly.
Comment on lines
+448
to
456
| if w := pctx.Venv.Writers.Writer; w != nil && w != io.Discard { | ||
| cachedEntry.replayOnce.Do(func() { | ||
| if !suppressOutput && cachedEntry.Stdout != "" { | ||
| _, _ = pctx.Writers.Writer.Write([]byte(cachedEntry.Stdout)) | ||
| _, _ = w.Write([]byte(cachedEntry.Stdout)) | ||
| } | ||
|
|
||
| if cachedEntry.Stderr != "" { | ||
| _, _ = pctx.Writers.ErrWriter.Write([]byte(cachedEntry.Stderr)) | ||
| if cachedEntry.Stderr != "" && pctx.Venv.Writers.ErrWriter != nil { | ||
| _, _ = pctx.Venv.Writers.ErrWriter.Write([]byte(cachedEntry.Stderr)) | ||
| } |
Contributor
There was a problem hiding this comment.
Handle stderr replay independently from stdout.
Lines 448-456 only replay cached stderr when stdout has a real writer, so stderr is lost when stdout is discarded. Lines 498-505 also write fresh stderr again even though RunCommandWithOutput already streamed stderr to ErrWriter; this can duplicate user output.
🐛 Proposed fix
- if w := pctx.Venv.Writers.Writer; w != nil && w != io.Discard {
+ stdoutWriter := pctx.Venv.Writers.Writer
+ stderrWriter := pctx.Venv.Writers.ErrWriter
+ hasStdoutWriter := stdoutWriter != nil && stdoutWriter != io.Discard
+ hasStderrWriter := stderrWriter != nil && stderrWriter != io.Discard
+
+ if hasStdoutWriter || hasStderrWriter {
cachedEntry.replayOnce.Do(func() {
- if !suppressOutput && cachedEntry.Stdout != "" {
- _, _ = w.Write([]byte(cachedEntry.Stdout))
+ if hasStdoutWriter && !suppressOutput && cachedEntry.Stdout != "" {
+ _, _ = stdoutWriter.Write([]byte(cachedEntry.Stdout))
}
- if cachedEntry.Stderr != "" && pctx.Venv.Writers.ErrWriter != nil {
- _, _ = pctx.Venv.Writers.ErrWriter.Write([]byte(cachedEntry.Stderr))
+ if hasStderrWriter && cachedEntry.Stderr != "" {
+ _, _ = stderrWriter.Write([]byte(cachedEntry.Stderr))
}
})
}
@@
- if w := pctx.Venv.Writers.Writer; w != nil && w != io.Discard {
+ stdoutWriter := pctx.Venv.Writers.Writer
+ stderrWriter := pctx.Venv.Writers.ErrWriter
+ hasStdoutWriter := stdoutWriter != nil && stdoutWriter != io.Discard
+ hasStderrWriter := stderrWriter != nil && stderrWriter != io.Discard
+
+ if hasStdoutWriter || hasStderrWriter {
entry.replayOnce.Do(func() {
- if !suppressOutput && entry.Stdout != "" {
- _, _ = w.Write([]byte(entry.Stdout))
- }
-
- if entry.Stderr != "" && pctx.Venv.Writers.ErrWriter != nil {
- _, _ = pctx.Venv.Writers.ErrWriter.Write([]byte(entry.Stderr))
+ if hasStdoutWriter && !suppressOutput && entry.Stdout != "" {
+ _, _ = stdoutWriter.Write([]byte(entry.Stdout))
}
+ // Stderr was already streamed by RunCommandWithOutput on this fresh execution.
})
}Also applies to: 498-505
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/config/config_helpers.go` around lines 448 - 456, The stderr replay logic
in the replayOnce.Do block is nested within the stdout writer condition, causing
cached stderr to be lost when stdout is discarded. Move the stderr replay logic
(checking cachedEntry.Stderr and pctx.Venv.Writers.ErrWriter) outside of the
stdout writer condition so it executes independently. Additionally, in the
second location around lines 498-505, avoid writing stderr again to ErrWriter if
it has already been streamed by RunCommandWithOutput, as this causes duplicate
output to the user.
Comment on lines
39
to
45
| type ParsingContext struct { | ||
| Writers writer.Writers | ||
|
|
||
| // Venv is the virtualized environment used by HCL helper functions | ||
| // that shell out (e.g. get_repo_root) or evaluate dependency outputs. | ||
| // It also carries the shell environment and stdout/stderr writers. | ||
| // Defaults to the OS-backed environment when [NewParsingContext] is | ||
| // called; callers with a threaded root Venv set it before parsing. | ||
| Venv venv.Venv |
Contributor
There was a problem hiding this comment.
Keep a compatibility bridge for the removed ParsingContext.Env field.
ParsingContext is exported from pkg/config, and this refactor removes the exported Env map[string]string field while moving the source of truth to Venv.Env. That breaks downstream code constructing or reading ParsingContext.Env, despite the PR claiming backward compatibility. Please either retain a deprecated Env bridge that is normalized into Venv.Env, or mark this as a breaking API change before merge.
Also applies to: 134-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/config/parsing_context.go` around lines 39 - 45, The ParsingContext
struct is exported from pkg/config and the refactor removes the exported Env
field while moving the source of truth to Venv.Env, which breaks downstream code
that constructs or reads ParsingContext.Env. To maintain backward compatibility,
retain the Env field on the ParsingContext struct as a deprecated field and
ensure it is synchronized with Venv.Env by updating any methods that modify
either field to keep them in sync, or create getter and setter methods that
bridge between ParsingContext.Env and Venv.Env. Alternatively, if accepting this
as a breaking API change, document and communicate this clearly before merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Moves environment variable access and writes to stdout/stderr to venv.
TODOs
Read the Gruntwork contribution guidelines.
Summary by CodeRabbit
Bug Fixes
--log-show-abs-pathsand disabling error summaries—are applied consistently during execution.Refactor
Chores