chore: thread venv throughout CLI#6089
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (88)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (65)
📝 WalkthroughWalkthroughAdds a root virtual environment type and threads its FS/Exec through CLI constructors, command actions, runners, discovery, shell/git helpers, download/getter clients, credential providers, prepare/run internals, parsing context, and tests. ChangesEnd-to-end venv threading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
yhakbar
force-pushed
the
chore/thread-venv-through-cli
branch
from
d47a34d to
26a86cc
Compare
yhakbar
force-pushed
the
chore/thread-venv-through-cli
branch
from
26a86cc to
e547de5
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/cli/commands/run/run.go (2)
29-149: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRefactor
Runinto smaller stages to clear complexity gate.Line 29 currently mixes dispatching, setup, parsing, dependency confirmation, reporting lifecycle, and execution in one method, which is tripping the configured complexity rule.
Proposed extraction outline
func Run(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, v venv.Venv) error { - // large orchestration body + rv := run.FromRoot(v) + r := initReport(l, opts) + + if handled, err := dispatchRunModes(ctx, l, rv, opts); handled || err != nil { + return err + } + + if err := prepareRunEnvironment(ctx, l, rv, opts); err != nil { + return err + } + + return executeUnitRun(ctx, l, rv, opts, r) }🤖 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/run/run.go` around lines 29 - 149, The Run function is too large and should be split into clear stages: extract a setup stage (credential initialization using creds.NewGetter and ObtainAndUpdateEnvIfNecessary, and checkVersionConstraints into e.g., initEnvironment), a config/parsing stage (configbridge.NewParsingContext, config.ReadTerragruntConfig and building runCfg into parseConfig), a confirmation stage (confirmActionWithDependentUnits into confirmDependencies), and a run lifecycle stage (report.EnsureRun, the deferred EndRun logic, and the actual execution via run.Run into executeRun). Implement small helper functions with those names (or similar) and replace the inline blocks in Run with calls to them; keep references to rv, opts, l, ctx, pctx, credsGetter, r, runCfg and ensure errors are propagated unchanged.
192-250: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit
checkVersionConstraintsinto focused helpers.Line 192 combines config read, TF path mutation, TF version detection, constraint checks, and feature-flag hydration. Breaking these into 2–3 helpers should bring complexity under the threshold and make side effects easier to reason about.
Proposed extraction outline
func checkVersionConstraints(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, v venv.Venv) (log.Logger, error) { - // full flow inline + partialCfg, err := getTerragruntConfig(ctx, l, v, opts) + if err != nil { return l, err } + + applyTerraformPathDefault(opts, partialCfg) + + l, err = populateAndValidateTerraformVersion(ctx, l, v, opts, partialCfg) + if err != nil { return l, err } + + if err := applyFeatureFlagDefaults(opts, partialCfg); err != nil { + return l, err + } + return l, nil }🤖 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/run/run.go` around lines 192 - 250, checkVersionConstraints is doing too many things—split it into focused helpers: extract config and TFPath mutation into a helper (e.g., loadPartialConfigAndSetTFPath calling getTerragruntConfig and setting opts.TFPath when partial.TerraformBinary is present), extract TF version detection into detectAndSetTFVersion that calls run.PopulateTFVersion and updates opts.TerraformVersion and opts.TofuImplementation, extract constraint checks into validateVersionConstraints that calls run.CheckTerraformVersionMeetsConstraint and run.CheckTerragruntVersionMeetsConstraint, and extract feature-flag hydration into hydrateFeatureFlags that iterates partialTerragruntConfig.FeatureFlags and updates opts.FeatureFlags; wire these helpers from checkVersionConstraints preserving returned (log.Logger, error) semantics and keep behavior identical to the original functions (use configbridge.TFRunOptsFromOpts, DefaultTerraformVersionConstraint, and the same error handling).
🧹 Nitpick comments (4)
internal/cli/commands/catalog/tui/redesign/scaffold.go (1)
52-54: 🏗️ Heavy liftAvoid hardcoding
venv.OSVenv()in the scaffold leaf path.This path bypasses root-venv propagation and forces real OS side effects. Thread the injected venv through
scaffoldCmdand use that here.Patch sketch (this file)
type scaffoldCmd struct { component *Component opts *options.TerragruntOptions logger log.Logger + v venv.Venv plan *scaffold.Plan values map[string]string } -func newScaffoldCmd(l log.Logger, opts *options.TerragruntOptions, c *Component) *scaffoldCmd { - return &scaffoldCmd{component: c, opts: opts, logger: l} +func newScaffoldCmd(l log.Logger, opts *options.TerragruntOptions, v venv.Venv, c *Component) *scaffoldCmd { + return &scaffoldCmd{component: c, opts: opts, logger: l, v: v} } @@ - return scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts, c.component.TerraformSourcePath(), "") + return scaffold.Run(context.Background(), c.logger, c.v, c.opts, c.component.TerraformSourcePath(), "")🤖 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/catalog/tui/redesign/scaffold.go` around lines 52 - 54, The scaffold leaf is directly calling scaffold.Run(..., venv.OSVenv(), ...) which hardcodes the OS venv; instead thread the injected virtualenv through the scaffold command: add a venv field/parameter to scaffoldCmd (or its constructor) and have the caller populate it from the root CLI, then replace the hardcoded venv.OSVenv() argument in the scaffold.Run call with the injected venv field; ensure the injected venv's type matches the scaffold.Run parameter and update any usages of scaffoldCmd to pass the venv through.internal/cli/app_test.go (1)
857-865: ⚡ Quick winReuse a single root venv instance in
runAppTest.
runAppTestcurrently creates separatevenv.OSVenv()instances forcommands.New(...)andWrapWithTelemetry(...). Reusing one instance keeps test wiring aligned with production root-venv threading.Proposed patch
func runAppTest(l log.Logger, args []string, opts *options.TerragruntOptions) (*options.TerragruntOptions, error) { emptyAction := func(ctx context.Context, cliCtx *clihelper.Context) error { return nil } - terragruntCommands := commands.New(l, opts, venv.OSVenv()) + v := venv.OSVenv() + terragruntCommands := commands.New(l, opts, v) setCommandAction(emptyAction, terragruntCommands...) @@ - app.Commands = terragruntCommands.WrapAction(commands.WrapWithTelemetry(l, opts, venv.OSVenv())) + app.Commands = terragruntCommands.WrapAction(commands.WrapWithTelemetry(l, opts, 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/cli/app_test.go` around lines 857 - 865, The test creates two separate root venv instances by calling venv.OSVenv() twice; instead construct a single root venv variable (e.g., rootVenv := venv.OSVenv()) and pass that same variable into commands.New(...) and commands.WrapWithTelemetry(...) (and any other places in runAppTest that call venv.OSVenv()) so the test uses one shared root-venv instance matching production threading.internal/cli/commands/backend/migrate/migrate.go (1)
23-125: ⚡ Quick winSplit
Runinto a few helpers to clear the complexity gate.Line 23’s
Runis currently above the configured cognitive complexity limit (17 > 15). Extracting path resolution, unit lookup/build, and remote-state parsing into helper functions should preserve behavior while satisfying the check.🤖 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/backend/migrate/migrate.go` around lines 23 - 125, The Run function is too complex; extract logical sections into small helpers to reduce cognitive complexity: create functions like resolveCanonicalPaths(ctx, opts, srcPath, dstPath) that calls util.CanonicalPath and returns resolved src/dst (and log via l), buildUnits(rnr, srcPath, dstPath) that finds units via runner.NewStackRunner/GetStack/FindUnitByPath and calls runner.BuildUnitOpts for both source and destination returning srcUnit, dstUnit, srcOpts, dstOpts, and parseRemoteStates(ctx, l, v, srcOpts, dstOpts) that creates parsing contexts (configbridge.NewParsingContext), calls config.ParseRemoteState for each, validates non-nil, sets opts.WorkingDir from pctx.WorkingDir, and returns srcRemoteState and dstRemoteState; then have Run call these helpers and keep the final version-check and srcRemoteState.Migrate logic unchanged (refer to symbols Run, resolveCanonicalPaths, buildUnits, parseRemoteStates, runner.NewStackRunner, runner.BuildUnitOpts, config.ParseRemoteState, configbridge.NewParsingContext, srcRemoteState.Migrate).internal/runner/graph/graph.go (1)
29-29: ⚖️ Poor tradeoffPre-existing cognitive complexity flagged by SonarCloud.
The function has a cognitive complexity of 27 (threshold 15) according to SonarCloud. This is not introduced by the venv threading changes—the complexity stems from the existing report handling, deferred writes, and multiple conditional branches. Consider extracting report setup/teardown into helper functions in a follow-up refactor.
🤖 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/graph/graph.go` at line 29, The Run function's cognitive complexity is too high; extract the report initialization/teardown and the deferred write/cleanup logic into small helper functions to reduce branching inside Run. Specifically, move the code that constructs and configures the report (currently inside Run) into a helper like initReport(ctx, l, opts) that returns the report and any teardown func, and move the deferred write/flush/error-handling block into a separate finishReport(report, err) helper; replace the inlined conditional branches that manage report creation, closing, and error propagation with calls to these helpers so Run becomes a simple orchestrator invoking initReport(...) and defer finishReport(...) and focuses on the main flow. Ensure helper names reference Run’s existing symbols (report, finishWrite/deferred write, error propagation) so callers in Run are obvious and tests/logic remain unchanged.
🤖 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/getter/filecopy.go`:
- Around line 93-97: NewFileCopyGetter currently accepts any vfs.FS allowing
creation of FileCopyGetter instances that WithFS rejects and that later may fail
in Get; change NewFileCopyGetter to validate the FS the same way WithFS does
(reject non-OS filesystems) before returning &FileCopyGetter{FS: fs}, returning
a nil or panicking/handling error consistently with WithFS behavior; reference
NewFileCopyGetter, FileCopyGetter.FS, WithFS and Get to locate the logic to copy
the same validation checks.
In `@internal/getter/tfr.go`:
- Around line 56-61: NewRegistryGetter currently accepts any vfs.FS which is
inconsistent with WithFS; change NewRegistryGetter to return (*RegistryGetter,
error), validate that the provided fs is an OS filesystem (same check/logic used
by WithFS), and return a descriptive error if it is not; on success, construct
and return the RegistryGetter (with HTTPClient, Logger, FS, TofuImplementation
as before). Ensure callers are updated to handle the error and that the
validation logic mirrors WithFS to keep behavior consistent.
In `@internal/services/catalog/catalog.go`:
- Around line 283-285: The call to scaffold.Run currently forces OS venv by
calling venv.OSVenv(), breaking injected virtual environments; change the
catalog leaf to accept and propagate the caller's virtual environment instead of
using venv.OSVenv(): update the surrounding function/method signature to take a
venv (or obtain it from the caller/root), replace the scaffold.Run call to pass
that injected venv value, and ensure any upstream callers are updated to thread
the venv through the catalog service so the scaffold.Run invocation uses the
provided virtual environment rather than the hardcoded OS one.
---
Outside diff comments:
In `@internal/cli/commands/run/run.go`:
- Around line 29-149: The Run function is too large and should be split into
clear stages: extract a setup stage (credential initialization using
creds.NewGetter and ObtainAndUpdateEnvIfNecessary, and checkVersionConstraints
into e.g., initEnvironment), a config/parsing stage
(configbridge.NewParsingContext, config.ReadTerragruntConfig and building runCfg
into parseConfig), a confirmation stage (confirmActionWithDependentUnits into
confirmDependencies), and a run lifecycle stage (report.EnsureRun, the deferred
EndRun logic, and the actual execution via run.Run into executeRun). Implement
small helper functions with those names (or similar) and replace the inline
blocks in Run with calls to them; keep references to rv, opts, l, ctx, pctx,
credsGetter, r, runCfg and ensure errors are propagated unchanged.
- Around line 192-250: checkVersionConstraints is doing too many things—split it
into focused helpers: extract config and TFPath mutation into a helper (e.g.,
loadPartialConfigAndSetTFPath calling getTerragruntConfig and setting
opts.TFPath when partial.TerraformBinary is present), extract TF version
detection into detectAndSetTFVersion that calls run.PopulateTFVersion and
updates opts.TerraformVersion and opts.TofuImplementation, extract constraint
checks into validateVersionConstraints that calls
run.CheckTerraformVersionMeetsConstraint and
run.CheckTerragruntVersionMeetsConstraint, and extract feature-flag hydration
into hydrateFeatureFlags that iterates partialTerragruntConfig.FeatureFlags and
updates opts.FeatureFlags; wire these helpers from checkVersionConstraints
preserving returned (log.Logger, error) semantics and keep behavior identical to
the original functions (use configbridge.TFRunOptsFromOpts,
DefaultTerraformVersionConstraint, and the same error handling).
---
Nitpick comments:
In `@internal/cli/app_test.go`:
- Around line 857-865: The test creates two separate root venv instances by
calling venv.OSVenv() twice; instead construct a single root venv variable
(e.g., rootVenv := venv.OSVenv()) and pass that same variable into
commands.New(...) and commands.WrapWithTelemetry(...) (and any other places in
runAppTest that call venv.OSVenv()) so the test uses one shared root-venv
instance matching production threading.
In `@internal/cli/commands/backend/migrate/migrate.go`:
- Around line 23-125: The Run function is too complex; extract logical sections
into small helpers to reduce cognitive complexity: create functions like
resolveCanonicalPaths(ctx, opts, srcPath, dstPath) that calls util.CanonicalPath
and returns resolved src/dst (and log via l), buildUnits(rnr, srcPath, dstPath)
that finds units via runner.NewStackRunner/GetStack/FindUnitByPath and calls
runner.BuildUnitOpts for both source and destination returning srcUnit, dstUnit,
srcOpts, dstOpts, and parseRemoteStates(ctx, l, v, srcOpts, dstOpts) that
creates parsing contexts (configbridge.NewParsingContext), calls
config.ParseRemoteState for each, validates non-nil, sets opts.WorkingDir from
pctx.WorkingDir, and returns srcRemoteState and dstRemoteState; then have Run
call these helpers and keep the final version-check and srcRemoteState.Migrate
logic unchanged (refer to symbols Run, resolveCanonicalPaths, buildUnits,
parseRemoteStates, runner.NewStackRunner, runner.BuildUnitOpts,
config.ParseRemoteState, configbridge.NewParsingContext,
srcRemoteState.Migrate).
In `@internal/cli/commands/catalog/tui/redesign/scaffold.go`:
- Around line 52-54: The scaffold leaf is directly calling scaffold.Run(...,
venv.OSVenv(), ...) which hardcodes the OS venv; instead thread the injected
virtualenv through the scaffold command: add a venv field/parameter to
scaffoldCmd (or its constructor) and have the caller populate it from the root
CLI, then replace the hardcoded venv.OSVenv() argument in the scaffold.Run call
with the injected venv field; ensure the injected venv's type matches the
scaffold.Run parameter and update any usages of scaffoldCmd to pass the venv
through.
In `@internal/runner/graph/graph.go`:
- Line 29: The Run function's cognitive complexity is too high; extract the
report initialization/teardown and the deferred write/cleanup logic into small
helper functions to reduce branching inside Run. Specifically, move the code
that constructs and configures the report (currently inside Run) into a helper
like initReport(ctx, l, opts) that returns the report and any teardown func, and
move the deferred write/flush/error-handling block into a separate
finishReport(report, err) helper; replace the inlined conditional branches that
manage report creation, closing, and error propagation with calls to these
helpers so Run becomes a simple orchestrator invoking initReport(...) and defer
finishReport(...) and focuses on the main flow. Ensure helper names reference
Run’s existing symbols (report, finishWrite/deferred write, error propagation)
so callers in Run are obvious and tests/logic remain unchanged.
🪄 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: b491ad97-4ce5-41e1-a136-6a9a61b007fc
📒 Files selected for processing (80)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/commands/aws-provider-patch/aws-provider-patch.gointernal/cli/commands/aws-provider-patch/cli.gointernal/cli/commands/backend/cli.gointernal/cli/commands/backend/migrate/cli.gointernal/cli/commands/backend/migrate/migrate.gointernal/cli/commands/catalog/tui/redesign/scaffold.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/commands.gointernal/cli/commands/exec/cli.gointernal/cli/commands/exec/exec.gointernal/cli/commands/hcl/cli.gointernal/cli/commands/hcl/validate/cli.gointernal/cli/commands/hcl/validate/validate.gointernal/cli/commands/info/cli.gointernal/cli/commands/info/print/cli.gointernal/cli/commands/info/print/print.gointernal/cli/commands/render/cli.gointernal/cli/commands/render/render.gointernal/cli/commands/render/render_test.gointernal/cli/commands/run/cli.gointernal/cli/commands/run/run.gointernal/cli/commands/run_action_test.gointernal/cli/commands/scaffold/cli.gointernal/cli/commands/scaffold/scaffold.gointernal/cli/commands/shortcuts.gointernal/cli/commands/stack/cli.gointernal/cli/commands/stack/stack.gointernal/discovery/constructor.gointernal/discovery/discovery.gointernal/discovery/options.gointernal/discovery/phase_parse.gointernal/discovery/types.gointernal/getter/client_test.gointernal/getter/defaults_test.gointernal/getter/filecopy.gointernal/getter/options_test.gointernal/getter/tfr.gointernal/getter/tfr_test.gointernal/getter/types_test.gointernal/prepare/prepare.gointernal/remotestate/remote_state.gointernal/runner/common/runner.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/download_source.gointernal/runner/run/download_source_test.gointernal/runner/run/run.gointernal/runner/run/venv.gointernal/runner/run/version_check_mem_test.gointernal/runner/runall/runall.gointernal/runner/runall/runall_test.gointernal/runner/runner.gointernal/runner/runnerpool/builder.gointernal/runner/runnerpool/builder_helpers.gointernal/runner/runnerpool/graph_fallback_test.gointernal/runner/runnerpool/runner.gointernal/services/catalog/catalog.gointernal/shell/git.gointernal/shell/git_mem_test.gointernal/shell/git_windows_test.gointernal/shell/run_cmd_mem_test.gointernal/shell/run_cmd_test.gointernal/tflint/venv.gointernal/venv/venv.gomain.gopkg/config/config_helpers.gopkg/config/config_helpers_mem_test.gopkg/config/config_helpers_test.gopkg/config/dependency.gopkg/config/parsing_context.gotest/benchmarks/helpers/helpers.gotest/helpers/package.go
💤 Files with no reviewable changes (1)
- pkg/config/config_helpers_test.go
Comment on lines
+93
to
97
| // NewFileCopyGetter returns a FileCopyGetter backed by the supplied | ||
| // filesystem. Use the With* methods to customize other behavior. | ||
| func NewFileCopyGetter() *FileCopyGetter { | ||
| return &FileCopyGetter{FS: vfs.NewOSFS()} | ||
| func NewFileCopyGetter(fs vfs.FS) *FileCopyGetter { | ||
| return &FileCopyGetter{FS: fs} | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce the same FS invariant in the constructor
Line 95 allows creating FileCopyGetter with any vfs.FS, but WithFS explicitly rejects non-OS filesystems. This lets callers build invalid instances that fail later on Get. Validate in NewFileCopyGetter too.
Proposed fix
func NewFileCopyGetter(fs vfs.FS) *FileCopyGetter {
- return &FileCopyGetter{FS: fs}
+ if !vfs.IsOSFS(fs) {
+ panic("getter.FileCopyGetter.NewFileCopyGetter: requires an OS-backed filesystem")
+ }
+
+ return &FileCopyGetter{FS: fs}
}As per coding guidelines: Review the Go code for quality and correctness.
🤖 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/getter/filecopy.go` around lines 93 - 97, NewFileCopyGetter
currently accepts any vfs.FS allowing creation of FileCopyGetter instances that
WithFS rejects and that later may fail in Get; change NewFileCopyGetter to
validate the FS the same way WithFS does (reject non-OS filesystems) before
returning &FileCopyGetter{FS: fs}, returning a nil or panicking/handling error
consistently with WithFS behavior; reference NewFileCopyGetter,
FileCopyGetter.FS, WithFS and Get to locate the logic to copy the same
validation checks.
Comment on lines
+56
to
61
| func NewRegistryGetter(l log.Logger, fs vfs.FS) *RegistryGetter { | ||
| return &RegistryGetter{ | ||
| HTTPClient: cleanhttp.DefaultClient(), | ||
| Logger: l, | ||
| FS: vfs.NewOSFS(), | ||
| FS: fs, | ||
| TofuImplementation: tfimpl.OpenTofu, |
Contributor
There was a problem hiding this comment.
Constructor should reject non-OS filesystems
Line 56 accepts any vfs.FS, while WithFS rejects non-OS filesystems. This creates inconsistent behavior and allows invalid RegistryGetter instances to be built.
Proposed fix
func NewRegistryGetter(l log.Logger, fs vfs.FS) *RegistryGetter {
+ if !vfs.IsOSFS(fs) {
+ panic("getter.RegistryGetter.NewRegistryGetter: requires an OS-backed filesystem")
+ }
+
return &RegistryGetter{
HTTPClient: cleanhttp.DefaultClient(),
Logger: l,
FS: fs,
TofuImplementation: tfimpl.OpenTofu,
}
}As per coding guidelines: Review the Go code for quality and correctness.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func NewRegistryGetter(l log.Logger, fs vfs.FS) *RegistryGetter { | |
| return &RegistryGetter{ | |
| HTTPClient: cleanhttp.DefaultClient(), | |
| Logger: l, | |
| FS: vfs.NewOSFS(), | |
| FS: fs, | |
| TofuImplementation: tfimpl.OpenTofu, | |
| func NewRegistryGetter(l log.Logger, fs vfs.FS) *RegistryGetter { | |
| if !vfs.IsOSFS(fs) { | |
| panic("getter.RegistryGetter.NewRegistryGetter: requires an OS-backed filesystem") | |
| } | |
| return &RegistryGetter{ | |
| HTTPClient: cleanhttp.DefaultClient(), | |
| Logger: l, | |
| FS: fs, | |
| TofuImplementation: tfimpl.OpenTofu, | |
| } | |
| } |
🤖 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/getter/tfr.go` around lines 56 - 61, NewRegistryGetter currently
accepts any vfs.FS which is inconsistent with WithFS; change NewRegistryGetter
to return (*RegistryGetter, error), validate that the provided fs is an OS
filesystem (same check/logic used by WithFS), and return a descriptive error if
it is not; on success, construct and return the RegistryGetter (with HTTPClient,
Logger, FS, TofuImplementation as before). Ensure callers are updated to handle
the error and that the validation logic mirrors WithFS to keep behavior
consistent.
Comment on lines
+283
to
+285
| // TODO: thread venv from the CLI entrypoint through catalog service | ||
| // so this leaf participates in the root virtualized environment. | ||
| return scaffold.Run(ctx, l, venv.OSVenv(), opts, module.TerraformSourcePath(), "") |
Contributor
There was a problem hiding this comment.
Hardcoded venv.OSVenv() breaks end-to-end venv injection.
This path ignores the caller/root virtualized handles and always uses OS side effects, so the catalog scaffold leaf no longer participates in the same injected environment as the rest of the run.
💡 Suggested direction
type catalogServiceImpl struct {
opts *options.TerragruntOptions
+ venv venv.Venv
newRepo NewRepoFunc
repoURL string
repoURLs []string
modules module.Modules
mu sync.Mutex
}
-func NewCatalogService(opts *options.TerragruntOptions) *catalogServiceImpl {
+func NewCatalogService(opts *options.TerragruntOptions, v venv.Venv) *catalogServiceImpl {
return &catalogServiceImpl{
opts: opts,
+ venv: v,
newRepo: module.NewRepo,
}
}
func (s *catalogServiceImpl) Scaffold(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, module *module.Module) error {
l.Debugf("Scaffolding module: %q", module.TerraformSourcePath())
- return scaffold.Run(ctx, l, venv.OSVenv(), opts, module.TerraformSourcePath(), "")
+ return scaffold.Run(ctx, l, s.venv, opts, module.TerraformSourcePath(), "")
}🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 283-283: Complete the task associated to this TODO comment.
🤖 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/services/catalog/catalog.go` around lines 283 - 285, The call to
scaffold.Run currently forces OS venv by calling venv.OSVenv(), breaking
injected virtual environments; change the catalog leaf to accept and propagate
the caller's virtual environment instead of using venv.OSVenv(): update the
surrounding function/method signature to take a venv (or obtain it from the
caller/root), replace the scaffold.Run call to pass that injected venv value,
and ensure any upstream callers are updated to thread the venv through the
catalog service so the scaffold.Run invocation uses the provided virtual
environment rather than the hardcoded OS one.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/runner/run/download_source.go`:
- Around line 469-470: The check at if !vfs.IsOSFS(v.FS) returns
ErrNonOSFilesystem but the error's wording/doc only references run.Options.FS,
which misleads when run.Venv.FS is invalid; update the handling at that
validation point (the v.FS check in download_source.go) to return a
context-specific error (either create/use ErrNonOSFilesystemVenv or wrap
ErrNonOSFilesystem with fmt.Errorf to mention run.Venv.FS/v.FS) so the error
message clearly identifies the venv filesystem as the source of the problem;
keep ErrNonOSFilesystem for options.FS checks unchanged and ensure callers still
get the wrapped/contextualized error.
🪄 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: 8acb887d-e12e-4f30-8db2-ede6d684d816
📒 Files selected for processing (3)
internal/runner/run/download_source.gointernal/runner/run/download_source_test.gopkg/config/parsing_context.go
yhakbar
force-pushed
the
chore/thread-venv-through-cli
branch
2 times, most recently
from
73bcbb6 to
7b980aa
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cli/commands/catalog/tui/redesign/scaffold.go`:
- Around line 52-54: The call to scaffold.Run uses venv.OSVenv(), which bypasses
the caller's virtualized environment; replace that argument with the
propagated/owned venv instance instead (e.g., use the CLI-threaded venv field or
parameter rather than venv.OSVenv()). Locate the scaffold.Run invocation and
change the third parameter from venv.OSVenv() to the root/provided venv object
(for example c.venv or the venv param you thread from the CLI entrypoint), or
add a venv parameter to the enclosing function and pass it through so this leaf
executes inside the same virtualized environment.
In `@internal/cli/commands/catalog/tui/redesign/update.go`:
- Line 615: scaffold.Prepare is being called with the real OS environment via
venv.OSVenv(), so the interactive TUI still runs downloads/parsing in the real
filesystem/exec; change the call site in update.go to accept and forward the
virtualized environment handle that the TUI model/entrypoint provides instead of
venv.OSVenv(), and ensure that same handle is threaded into discoverFormCmd so
all scaffold.Prepare invocations use the injected virtual FS/exec rather than
the hard-coded OS environment.
🪄 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: dedc7f26-883d-4778-b55c-399bb50e89bf
📒 Files selected for processing (74)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/commands/aws-provider-patch/aws-provider-patch.gointernal/cli/commands/aws-provider-patch/cli.gointernal/cli/commands/backend/cli.gointernal/cli/commands/backend/migrate/cli.gointernal/cli/commands/backend/migrate/migrate.gointernal/cli/commands/catalog/tui/redesign/scaffold.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/commands.gointernal/cli/commands/exec/cli.gointernal/cli/commands/exec/exec.gointernal/cli/commands/hcl/cli.gointernal/cli/commands/hcl/validate/cli.gointernal/cli/commands/hcl/validate/validate.gointernal/cli/commands/info/cli.gointernal/cli/commands/info/print/cli.gointernal/cli/commands/info/print/print.gointernal/cli/commands/render/cli.gointernal/cli/commands/render/render.gointernal/cli/commands/render/render_test.gointernal/cli/commands/run/cli.gointernal/cli/commands/run/run.gointernal/cli/commands/run_action_test.gointernal/cli/commands/scaffold/cli.gointernal/cli/commands/scaffold/scaffold.gointernal/cli/commands/shortcuts.gointernal/cli/commands/stack/cli.gointernal/cli/commands/stack/stack.gointernal/discovery/constructor.gointernal/discovery/discovery.gointernal/discovery/options.gointernal/discovery/phase_parse.gointernal/discovery/types.gointernal/getter/client_test.gointernal/getter/defaults_test.gointernal/getter/filecopy.gointernal/getter/options_test.gointernal/getter/tfr.gointernal/getter/tfr_test.gointernal/getter/types_test.gointernal/prepare/prepare.gointernal/remotestate/remote_state.gointernal/runner/common/runner.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/download_source.gointernal/runner/run/download_source_test.gointernal/runner/run/run.gointernal/runner/run/venv.gointernal/runner/run/version_check_mem_test.gointernal/runner/runall/runall.gointernal/runner/runall/runall_test.gointernal/runner/runner.gointernal/runner/runnerpool/builder.gointernal/runner/runnerpool/builder_helpers.gointernal/runner/runnerpool/graph_fallback_test.gointernal/runner/runnerpool/runner.gointernal/services/catalog/catalog.gointernal/shell/git.gointernal/shell/git_mem_test.gointernal/shell/git_windows_test.gointernal/shell/run_cmd_mem_test.gointernal/shell/run_cmd_test.gointernal/tflint/venv.gointernal/venv/venv.gomain.gopkg/config/config.go
💤 Files with no reviewable changes (33)
- internal/shell/git_windows_test.go
- internal/shell/run_cmd_mem_test.go
- internal/tflint/venv.go
- internal/runner/common/runner.go
- internal/runner/run/creds/providers/externalcmd/provider_test.go
- internal/runner/run/creds/getter.go
- internal/runner/run/venv.go
- internal/runner/run/creds/providers/provider.go
- internal/runner/runnerpool/graph_fallback_test.go
- main.go
- internal/runner/run/creds/providers/amazonsts/provider.go
- internal/runner/runnerpool/builder.go
- internal/runner/common/unit_runner.go
- internal/runner/runall/runall.go
- internal/runner/runnerpool/runner.go
- internal/services/catalog/catalog.go
- internal/remotestate/remote_state.go
- pkg/config/config.go
- internal/runner/runner.go
- internal/runner/runall/runall_test.go
- internal/runner/run/creds/providers/externalcmd/provider.go
- internal/runner/runnerpool/builder_helpers.go
- internal/venv/venv.go
- internal/runner/run/version_check_mem_test.go
- internal/shell/git.go
- internal/runner/graph/graph.go
- internal/runner/run/creds/providers/externalcmd/provider_mem_test.go
- internal/prepare/prepare.go
- internal/runner/run/download_source_test.go
- internal/runner/run/download_source.go
- internal/runner/run/run.go
- internal/shell/run_cmd_test.go
- internal/shell/git_mem_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/getter/client_test.go
🚧 Files skipped from review as they are similar to previous changes (34)
- internal/getter/tfr_test.go
- internal/discovery/options.go
- internal/cli/commands/aws-provider-patch/cli.go
- internal/cli/app.go
- internal/cli/commands/exec/cli.go
- internal/discovery/discovery.go
- internal/cli/commands/render/render_test.go
- internal/cli/commands/hcl/cli.go
- internal/getter/filecopy.go
- internal/cli/commands/backend/cli.go
- internal/discovery/types.go
- internal/getter/types_test.go
- internal/cli/commands/render/cli.go
- internal/cli/commands/run_action_test.go
- internal/cli/commands/render/render.go
- internal/cli/app_test.go
- internal/discovery/constructor.go
- internal/getter/tfr.go
- internal/cli/commands/scaffold/cli.go
- internal/getter/options_test.go
- internal/discovery/phase_parse.go
- internal/cli/commands/stack/cli.go
- internal/cli/commands/exec/exec.go
- internal/cli/commands/stack/stack.go
- internal/cli/commands/scaffold/scaffold.go
- internal/cli/commands/run/cli.go
- internal/cli/commands/hcl/validate/cli.go
- internal/cli/commands/backend/migrate/cli.go
- internal/cli/commands/info/cli.go
- internal/cli/commands/shortcuts.go
- internal/cli/commands/aws-provider-patch/aws-provider-patch.go
- internal/cli/commands/commands.go
- internal/cli/commands/info/print/cli.go
- internal/cli/commands/info/print/print.go
Comment on lines
+52
to
+54
| // TODO: thread venv from the CLI entrypoint through the catalog TUI | ||
| // so this leaf participates in the root virtualized environment. | ||
| return scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts, c.component.TerraformSourcePath(), "") |
Contributor
There was a problem hiding this comment.
Line 54 still bypasses the caller's virtualized environment.
Using venv.OSVenv() here means the non-interactive catalog scaffold path still executes against the real OS-backed FS/exec instead of the root venv this PR is threading through the CLI. That leaves this leaf outside the new side-effect boundary.
Suggested direction
type scaffoldCmd struct {
component *Component
opts *options.TerragruntOptions
logger log.Logger
+ v venv.Venv
plan *scaffold.Plan
values map[string]string
}
-func newScaffoldCmd(l log.Logger, opts *options.TerragruntOptions, c *Component) *scaffoldCmd {
- return &scaffoldCmd{component: c, opts: opts, logger: l}
+func newScaffoldCmd(l log.Logger, v venv.Venv, opts *options.TerragruntOptions, c *Component) *scaffoldCmd {
+ return &scaffoldCmd{component: c, opts: opts, logger: l, v: v}
}
@@
- return scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts, c.component.TerraformSourcePath(), "")
+ return scaffold.Run(context.Background(), c.logger, c.v, c.opts, c.component.TerraformSourcePath(), "")🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 52-52: Complete the task associated to this TODO comment.
🤖 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/catalog/tui/redesign/scaffold.go` around lines 52 - 54,
The call to scaffold.Run uses venv.OSVenv(), which bypasses the caller's
virtualized environment; replace that argument with the propagated/owned venv
instance instead (e.g., use the CLI-threaded venv field or parameter rather than
venv.OSVenv()). Locate the scaffold.Run invocation and change the third
parameter from venv.OSVenv() to the root/provided venv object (for example
c.venv or the venv param you thread from the CLI entrypoint), or add a venv
parameter to the enclosing function and pass it through so this leaf executes
inside the same virtualized environment.
| quiet := l.WithOptions(log.WithOutput(io.Discard)) | ||
|
|
||
| plan, err := scaffold.Prepare(ctx, quiet, opts, c.TerraformSourcePath(), "") | ||
| plan, err := scaffold.Prepare(ctx, quiet, venv.OSVenv(), opts, c.TerraformSourcePath(), "") |
Contributor
There was a problem hiding this comment.
Line 615 still prepares modules in the real OS environment.
scaffold.Prepare performs download/parse work, so hard-coding venv.OSVenv() here means the interactive catalog flow still ignores the caller's virtualized FS/exec even though the rest of this PR is threading one through CLI side effects.
Suggested direction
-func discoverFormCmd(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, c *Component) tea.Cmd {
+func discoverFormCmd(ctx context.Context, l log.Logger, v venv.Venv, opts *options.TerragruntOptions, c *Component) tea.Cmd {
return func() tea.Msg {
if c.Kind.IsCopyable() {
return discoverValuesFields(c)
}
- return discoverModuleFields(ctx, l, opts, c)
+ return discoverModuleFields(ctx, l, v, opts, c)
}
}
@@
-func discoverModuleFields(ctx context.Context, l log.Logger, opts *options.TerragruntOptions, c *Component) tea.Msg {
+func discoverModuleFields(ctx context.Context, l log.Logger, v venv.Venv, opts *options.TerragruntOptions, c *Component) tea.Msg {
quiet := l.WithOptions(log.WithOutput(io.Discard))
- plan, err := scaffold.Prepare(ctx, quiet, venv.OSVenv(), opts, c.TerraformSourcePath(), "")
+ plan, err := scaffold.Prepare(ctx, quiet, v, opts, c.TerraformSourcePath(), "")Then pass the same handle from the TUI model/entrypoint into discoverFormCmd.
🤖 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/catalog/tui/redesign/update.go` at line 615,
scaffold.Prepare is being called with the real OS environment via venv.OSVenv(),
so the interactive TUI still runs downloads/parsing in the real filesystem/exec;
change the call site in update.go to accept and forward the virtualized
environment handle that the TUI model/entrypoint provides instead of
venv.OSVenv(), and ensure that same handle is threaded into discoverFormCmd so
all scaffold.Prepare invocations use the injected virtual FS/exec rather than
the hard-coded OS environment.
Adds a top level venv, and threads it through the CLI. As a consequence more pure alternatives for tests were made possible, as opposed to going through `os.` handles.
yhakbar
force-pushed
the
chore/thread-venv-through-cli
branch
from
7b980aa to
5d0e454
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/config/stack.go (1)
127-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJustify env-gated
experiment.StackDependenciesautoinclude parsing/merge code paths (signature 7)
pkg/config/stack.goadds multiple execution paths guarded bypctx.Experiments.Evaluate(experiment.StackDependencies)/parser.Experiments.Evaluate(experiment.StackDependencies)(127-177, 798-825, 1011-1035). Becauseexperiment-modecan be enabled viaTG_EXPERIMENT_MODE=trueand specific experiments viaTG_EXPERIMENT=stack-dependencies(or--experiment-mode/--experiment stack-dependencies), this matches UNKNOWN-UNKNOWNS signature (7) (env-var-gated/dormant behavior). Provide explicit intent and safety rationale for why this substantial autoinclude parse/override/prune/merge subsystem is included in this PR (or split it out), including the exact behavioral impact when the experiment is enabled.🤖 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/stack.go` around lines 127 - 177, Add an explicit justification and safety rationale for gating the autoinclude parsing/merge subsystem behind the experiment flag by updating the PR description and in-code comments near the two experiment checks (pctx.Experiments.Evaluate(experiment.StackDependencies) and parser.Experiments.Evaluate(experiment.StackDependencies)); state why this feature is introduced in this change, the exact behavioral differences when the experiment is enabled (e.g., autoinclude HCL-only parsing, reading stack bytes, rescoping eval context via createTerragruntEvalContext, running pruneOverriddenStackAutoIncludes and the autoinclude merge flow), and any safety/rollback mechanisms or intended users, or split the subsystem into its own PR if the justification is insufficient. Ensure the comment references the key functions/flows affected (parseStackFile call, pruneOverriddenStackAutoIncludes, and any merge routine) so reviewers can see the scope and runtime impact.internal/runner/graph/graph.go (1)
117-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
ErrUserCancelledfor the summary defer.After changing
Runto return a plainerror, the defer now inspects a stale localerr. Ifrunall.RunAllOnStackreturnsrunall.ErrUserCancelled, the summary is no longer suppressed because that final error never gets assigned toerr.💡 Minimal fix
- return runall.RunAllOnStack(ctx, l, v, graphOpts, rnr, r) + err = runall.RunAllOnStack(ctx, l, v, graphOpts, rnr, r) + return err🤖 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/graph/graph.go` around lines 117 - 135, The defer that skips writing the summary checks errors.Is(err, runall.ErrUserCancelled) but `err` is stale because Run returns a plain error; update Run so the defer sees the final error by either converting the function to use a named return variable (err error) or by capturing the result of runall.RunAllOnStack into a local (e.g. retErr) and assign that to err before returning; ensure you set err when runner.NewStackRunner fails and when calling runall.RunAllOnStack so r.WriteSummary (and the errors.Is(err, runall.ErrUserCancelled) check) behaves correctly; reference symbols: opts.SummaryDisable, r.WriteSummary, runall.ErrUserCancelled, runner.NewStackRunner, runall.RunAllOnStack, Run.
♻️ Duplicate comments (1)
internal/cli/commands/catalog/tui/redesign/scaffold.go (1)
52-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread the injected venv here instead of hard-coding OSVenv.
Line 54 still bypasses the CLI-threaded environment (
venv.OSVenv()), so this path remains outside the root side-effect boundary. The TODO on Line 52 is also a deferred correctness concern (signature (8)); please either wire the venv now or link a tracking issue in the TODO before merge.Suggested patch
type scaffoldCmd struct { component *Component opts *options.TerragruntOptions logger log.Logger + v venv.Venv plan *scaffold.Plan values map[string]string } -func newScaffoldCmd(l log.Logger, opts *options.TerragruntOptions, c *Component) *scaffoldCmd { - return &scaffoldCmd{component: c, opts: opts, logger: l} +func newScaffoldCmd(l log.Logger, v venv.Venv, opts *options.TerragruntOptions, c *Component) *scaffoldCmd { + return &scaffoldCmd{component: c, opts: opts, logger: l, v: v} } @@ - return scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts, c.component.TerraformSourcePath(), "") + return scaffold.Run(context.Background(), c.logger, c.v, c.opts, c.component.TerraformSourcePath(), "")As per coding guidelines: "New TODO / FIXME / HACK / XXX comment paired with a deferred safety or correctness concern" must be resolved in the PR or linked to a tracking issue.
🤖 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/catalog/tui/redesign/scaffold.go` around lines 52 - 54, The call to scaffold.Run is hard-coding venv.OSVenv(), bypassing the CLI-threaded virtual environment; update the call site that invokes scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts, c.component.TerraformSourcePath(), "") to accept and forward the injected CLI venv (e.g., a venv parameter on the surrounding command struct or method) instead of calling venv.OSVenv(), or if you cannot wire it now, replace the TODO with a TODO that includes a tracking issue ID and a short rationale; ensure you reference scaffold.Run and the venv argument so the leaf participates in the root virtualized environment.
🤖 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/cli/commands/run/run.go`:
- Around line 170-177: In getTFPathFromConfig replace the OS-only existence
check (util.FileExists(opts.TerragruntConfigPath)) with a check that uses the
injected virtual filesystem (v.FS) or simply attempt to call getTerragruntConfig
and treat a not-found error as the fast-path miss; specifically, update the
existence check in getTFPathFromConfig to query v.FS for
opts.TerragruntConfigPath (or remove the pre-check and let getTerragruntConfig
return an error you handle) so the --version fast-path no longer depends on the
OS filesystem.
In `@pkg/config/config_partial.go`:
- Around line 330-332: Add a short rationale in the PR description (or link RFC
`#5663`) explaining why the StackDependencies experiment and sibling autoinclude
merge (guarded by pctx.Experiments.Evaluate(experiment.StackDependencies) and
the recursion-prevention flag skipAutoIncludeMerge) are bundled into this
changeset; mention that autoIncludeCacheKeySuffix is used to fold sibling
autoinclude state into the partial-parse cache key and that the merge is
experiment-gated to avoid changing the experiment-off key, so reviewers can see
the design intent and link to the RFC for deeper context.
---
Outside diff comments:
In `@internal/runner/graph/graph.go`:
- Around line 117-135: The defer that skips writing the summary checks
errors.Is(err, runall.ErrUserCancelled) but `err` is stale because Run returns a
plain error; update Run so the defer sees the final error by either converting
the function to use a named return variable (err error) or by capturing the
result of runall.RunAllOnStack into a local (e.g. retErr) and assign that to err
before returning; ensure you set err when runner.NewStackRunner fails and when
calling runall.RunAllOnStack so r.WriteSummary (and the errors.Is(err,
runall.ErrUserCancelled) check) behaves correctly; reference symbols:
opts.SummaryDisable, r.WriteSummary, runall.ErrUserCancelled,
runner.NewStackRunner, runall.RunAllOnStack, Run.
In `@pkg/config/stack.go`:
- Around line 127-177: Add an explicit justification and safety rationale for
gating the autoinclude parsing/merge subsystem behind the experiment flag by
updating the PR description and in-code comments near the two experiment checks
(pctx.Experiments.Evaluate(experiment.StackDependencies) and
parser.Experiments.Evaluate(experiment.StackDependencies)); state why this
feature is introduced in this change, the exact behavioral differences when the
experiment is enabled (e.g., autoinclude HCL-only parsing, reading stack bytes,
rescoping eval context via createTerragruntEvalContext, running
pruneOverriddenStackAutoIncludes and the autoinclude merge flow), and any
safety/rollback mechanisms or intended users, or split the subsystem into its
own PR if the justification is insufficient. Ensure the comment references the
key functions/flows affected (parseStackFile call,
pruneOverriddenStackAutoIncludes, and any merge routine) so reviewers can see
the scope and runtime impact.
---
Duplicate comments:
In `@internal/cli/commands/catalog/tui/redesign/scaffold.go`:
- Around line 52-54: The call to scaffold.Run is hard-coding venv.OSVenv(),
bypassing the CLI-threaded virtual environment; update the call site that
invokes scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts,
c.component.TerraformSourcePath(), "") to accept and forward the injected CLI
venv (e.g., a venv parameter on the surrounding command struct or method)
instead of calling venv.OSVenv(), or if you cannot wire it now, replace the TODO
with a TODO that includes a tracking issue ID and a short rationale; ensure you
reference scaffold.Run and the venv argument so the leaf participates in the
root virtualized environment.
🪄 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: 6dda1b8b-5c5c-4f8a-9065-2b33f67d3791
📒 Files selected for processing (88)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/commands/aws-provider-patch/aws-provider-patch.gointernal/cli/commands/aws-provider-patch/cli.gointernal/cli/commands/backend/cli.gointernal/cli/commands/backend/migrate/cli.gointernal/cli/commands/backend/migrate/migrate.gointernal/cli/commands/catalog/tui/redesign/scaffold.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/commands.gointernal/cli/commands/exec/cli.gointernal/cli/commands/exec/exec.gointernal/cli/commands/hcl/cli.gointernal/cli/commands/hcl/validate/cli.gointernal/cli/commands/hcl/validate/validate.gointernal/cli/commands/info/cli.gointernal/cli/commands/info/print/cli.gointernal/cli/commands/info/print/print.gointernal/cli/commands/render/cli.gointernal/cli/commands/render/render.gointernal/cli/commands/render/render_test.gointernal/cli/commands/run/cli.gointernal/cli/commands/run/run.gointernal/cli/commands/run_action_test.gointernal/cli/commands/scaffold/cli.gointernal/cli/commands/scaffold/scaffold.gointernal/cli/commands/shortcuts.gointernal/cli/commands/stack/cli.gointernal/cli/commands/stack/stack.gointernal/discovery/constructor.gointernal/discovery/discovery.gointernal/discovery/options.gointernal/discovery/phase_parse.gointernal/discovery/types.gointernal/getter/casgetter_tfr_test.gointernal/getter/client_test.gointernal/getter/defaults.gointernal/getter/defaults_test.gointernal/getter/filecopy.gointernal/getter/options_test.gointernal/getter/tfr.gointernal/getter/tfr_test.gointernal/getter/types_test.gointernal/prepare/prepare.gointernal/remotestate/remote_state.gointernal/runner/common/runner.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/download_source.gointernal/runner/run/download_source_test.gointernal/runner/run/run.gointernal/runner/run/venv.gointernal/runner/run/version_check_mem_test.gointernal/runner/runall/runall.gointernal/runner/runall/runall_test.gointernal/runner/runner.gointernal/runner/runnerpool/builder.gointernal/runner/runnerpool/builder_helpers.gointernal/runner/runnerpool/graph_fallback_test.gointernal/runner/runnerpool/runner.gointernal/services/catalog/catalog.gointernal/shell/git.gointernal/shell/git_mem_test.gointernal/shell/git_windows_test.gointernal/shell/run_cmd_mem_test.gointernal/shell/run_cmd_test.gointernal/tflint/venv.gointernal/venv/venv.gomain.gopkg/config/config.gopkg/config/config_helpers.gopkg/config/config_helpers_mem_test.gopkg/config/config_helpers_test.gopkg/config/config_partial.gopkg/config/dependency.gopkg/config/early_stack_eval.gopkg/config/exclude.gopkg/config/fuzz_test.gopkg/config/locals.gopkg/config/parsing_context.gopkg/config/stack.gotest/benchmarks/helpers/helpers.go
✅ Files skipped from review due to trivial changes (3)
- internal/venv/venv.go
- internal/getter/client_test.go
- pkg/config/exclude.go
🚧 Files skipped from review as they are similar to previous changes (65)
- internal/runner/run/creds/providers/externalcmd/provider_test.go
- internal/runner/common/runner.go
- internal/runner/runall/runall_test.go
- internal/runner/run/creds/providers/amazonsts/provider.go
- internal/cli/commands/info/cli.go
- test/benchmarks/helpers/helpers.go
- internal/cli/commands/aws-provider-patch/cli.go
- internal/cli/commands/run_action_test.go
- internal/cli/commands/shortcuts.go
- pkg/config/parsing_context.go
- internal/discovery/discovery.go
- internal/discovery/types.go
- internal/getter/filecopy.go
- internal/getter/types_test.go
- internal/shell/git_windows_test.go
- internal/cli/commands/exec/cli.go
- internal/cli/commands/render/render_test.go
- internal/discovery/constructor.go
- internal/remotestate/remote_state.go
- internal/discovery/options.go
- internal/cli/commands/hcl/cli.go
- internal/shell/run_cmd_mem_test.go
- internal/runner/run/creds/providers/provider.go
- internal/cli/commands/backend/cli.go
- internal/shell/git.go
- internal/cli/app.go
- internal/runner/common/unit_runner.go
- internal/cli/commands/backend/migrate/cli.go
- internal/cli/commands/render/render.go
- internal/cli/commands/aws-provider-patch/aws-provider-patch.go
- main.go
- internal/runner/run/creds/providers/externalcmd/provider_mem_test.go
- internal/cli/commands/catalog/tui/redesign/update.go
- internal/cli/commands/scaffold/cli.go
- internal/runner/run/version_check_mem_test.go
- internal/runner/run/creds/providers/externalcmd/provider.go
- internal/getter/tfr.go
- internal/cli/commands/stack/stack.go
- internal/cli/commands/exec/exec.go
- internal/runner/run/venv.go
- internal/runner/runnerpool/graph_fallback_test.go
- internal/cli/commands/info/print/print.go
- internal/cli/commands/run/cli.go
- internal/runner/runnerpool/builder_helpers.go
- internal/cli/commands/info/print/cli.go
- internal/cli/commands/hcl/validate/cli.go
- internal/runner/run/download_source.go
- pkg/config/config.go
- pkg/config/config_helpers_test.go
- internal/cli/commands/render/cli.go
- internal/shell/run_cmd_test.go
- internal/runner/runner.go
- internal/getter/options_test.go
- internal/discovery/phase_parse.go
- internal/cli/commands/stack/cli.go
- pkg/config/dependency.go
- internal/runner/run/creds/getter.go
- internal/prepare/prepare.go
- internal/runner/run/download_source_test.go
- internal/shell/git_mem_test.go
- internal/cli/commands/scaffold/scaffold.go
- internal/cli/app_test.go
- pkg/config/config_helpers_mem_test.go
- internal/cli/commands/commands.go
- internal/runner/run/run.go
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/config/stack.go (1)
127-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winJustify env-gated
experiment.StackDependenciesautoinclude parsing/merge code paths (signature 7)
pkg/config/stack.goadds multiple execution paths guarded bypctx.Experiments.Evaluate(experiment.StackDependencies)/parser.Experiments.Evaluate(experiment.StackDependencies)(127-177, 798-825, 1011-1035). Becauseexperiment-modecan be enabled viaTG_EXPERIMENT_MODE=trueand specific experiments viaTG_EXPERIMENT=stack-dependencies(or--experiment-mode/--experiment stack-dependencies), this matches UNKNOWN-UNKNOWNS signature (7) (env-var-gated/dormant behavior). Provide explicit intent and safety rationale for why this substantial autoinclude parse/override/prune/merge subsystem is included in this PR (or split it out), including the exact behavioral impact when the experiment is enabled.🤖 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/stack.go` around lines 127 - 177, Add an explicit justification and safety rationale for gating the autoinclude parsing/merge subsystem behind the experiment flag by updating the PR description and in-code comments near the two experiment checks (pctx.Experiments.Evaluate(experiment.StackDependencies) and parser.Experiments.Evaluate(experiment.StackDependencies)); state why this feature is introduced in this change, the exact behavioral differences when the experiment is enabled (e.g., autoinclude HCL-only parsing, reading stack bytes, rescoping eval context via createTerragruntEvalContext, running pruneOverriddenStackAutoIncludes and the autoinclude merge flow), and any safety/rollback mechanisms or intended users, or split the subsystem into its own PR if the justification is insufficient. Ensure the comment references the key functions/flows affected (parseStackFile call, pruneOverriddenStackAutoIncludes, and any merge routine) so reviewers can see the scope and runtime impact.internal/runner/graph/graph.go (1)
117-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
ErrUserCancelledfor the summary defer.After changing
Runto return a plainerror, the defer now inspects a stale localerr. Ifrunall.RunAllOnStackreturnsrunall.ErrUserCancelled, the summary is no longer suppressed because that final error never gets assigned toerr.💡 Minimal fix
- return runall.RunAllOnStack(ctx, l, v, graphOpts, rnr, r) + err = runall.RunAllOnStack(ctx, l, v, graphOpts, rnr, r) + return err🤖 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/graph/graph.go` around lines 117 - 135, The defer that skips writing the summary checks errors.Is(err, runall.ErrUserCancelled) but `err` is stale because Run returns a plain error; update Run so the defer sees the final error by either converting the function to use a named return variable (err error) or by capturing the result of runall.RunAllOnStack into a local (e.g. retErr) and assign that to err before returning; ensure you set err when runner.NewStackRunner fails and when calling runall.RunAllOnStack so r.WriteSummary (and the errors.Is(err, runall.ErrUserCancelled) check) behaves correctly; reference symbols: opts.SummaryDisable, r.WriteSummary, runall.ErrUserCancelled, runner.NewStackRunner, runall.RunAllOnStack, Run.
♻️ Duplicate comments (1)
internal/cli/commands/catalog/tui/redesign/scaffold.go (1)
52-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread the injected venv here instead of hard-coding OSVenv.
Line 54 still bypasses the CLI-threaded environment (
venv.OSVenv()), so this path remains outside the root side-effect boundary. The TODO on Line 52 is also a deferred correctness concern (signature (8)); please either wire the venv now or link a tracking issue in the TODO before merge.Suggested patch
type scaffoldCmd struct { component *Component opts *options.TerragruntOptions logger log.Logger + v venv.Venv plan *scaffold.Plan values map[string]string } -func newScaffoldCmd(l log.Logger, opts *options.TerragruntOptions, c *Component) *scaffoldCmd { - return &scaffoldCmd{component: c, opts: opts, logger: l} +func newScaffoldCmd(l log.Logger, v venv.Venv, opts *options.TerragruntOptions, c *Component) *scaffoldCmd { + return &scaffoldCmd{component: c, opts: opts, logger: l, v: v} } @@ - return scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts, c.component.TerraformSourcePath(), "") + return scaffold.Run(context.Background(), c.logger, c.v, c.opts, c.component.TerraformSourcePath(), "")As per coding guidelines: "New TODO / FIXME / HACK / XXX comment paired with a deferred safety or correctness concern" must be resolved in the PR or linked to a tracking issue.
🤖 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/catalog/tui/redesign/scaffold.go` around lines 52 - 54, The call to scaffold.Run is hard-coding venv.OSVenv(), bypassing the CLI-threaded virtual environment; update the call site that invokes scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts, c.component.TerraformSourcePath(), "") to accept and forward the injected CLI venv (e.g., a venv parameter on the surrounding command struct or method) instead of calling venv.OSVenv(), or if you cannot wire it now, replace the TODO with a TODO that includes a tracking issue ID and a short rationale; ensure you reference scaffold.Run and the venv argument so the leaf participates in the root virtualized environment.
🤖 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/cli/commands/run/run.go`:
- Around line 170-177: In getTFPathFromConfig replace the OS-only existence
check (util.FileExists(opts.TerragruntConfigPath)) with a check that uses the
injected virtual filesystem (v.FS) or simply attempt to call getTerragruntConfig
and treat a not-found error as the fast-path miss; specifically, update the
existence check in getTFPathFromConfig to query v.FS for
opts.TerragruntConfigPath (or remove the pre-check and let getTerragruntConfig
return an error you handle) so the --version fast-path no longer depends on the
OS filesystem.
In `@pkg/config/config_partial.go`:
- Around line 330-332: Add a short rationale in the PR description (or link RFC
`#5663`) explaining why the StackDependencies experiment and sibling autoinclude
merge (guarded by pctx.Experiments.Evaluate(experiment.StackDependencies) and
the recursion-prevention flag skipAutoIncludeMerge) are bundled into this
changeset; mention that autoIncludeCacheKeySuffix is used to fold sibling
autoinclude state into the partial-parse cache key and that the merge is
experiment-gated to avoid changing the experiment-off key, so reviewers can see
the design intent and link to the RFC for deeper context.
---
Outside diff comments:
In `@internal/runner/graph/graph.go`:
- Around line 117-135: The defer that skips writing the summary checks
errors.Is(err, runall.ErrUserCancelled) but `err` is stale because Run returns a
plain error; update Run so the defer sees the final error by either converting
the function to use a named return variable (err error) or by capturing the
result of runall.RunAllOnStack into a local (e.g. retErr) and assign that to err
before returning; ensure you set err when runner.NewStackRunner fails and when
calling runall.RunAllOnStack so r.WriteSummary (and the errors.Is(err,
runall.ErrUserCancelled) check) behaves correctly; reference symbols:
opts.SummaryDisable, r.WriteSummary, runall.ErrUserCancelled,
runner.NewStackRunner, runall.RunAllOnStack, Run.
In `@pkg/config/stack.go`:
- Around line 127-177: Add an explicit justification and safety rationale for
gating the autoinclude parsing/merge subsystem behind the experiment flag by
updating the PR description and in-code comments near the two experiment checks
(pctx.Experiments.Evaluate(experiment.StackDependencies) and
parser.Experiments.Evaluate(experiment.StackDependencies)); state why this
feature is introduced in this change, the exact behavioral differences when the
experiment is enabled (e.g., autoinclude HCL-only parsing, reading stack bytes,
rescoping eval context via createTerragruntEvalContext, running
pruneOverriddenStackAutoIncludes and the autoinclude merge flow), and any
safety/rollback mechanisms or intended users, or split the subsystem into its
own PR if the justification is insufficient. Ensure the comment references the
key functions/flows affected (parseStackFile call,
pruneOverriddenStackAutoIncludes, and any merge routine) so reviewers can see
the scope and runtime impact.
---
Duplicate comments:
In `@internal/cli/commands/catalog/tui/redesign/scaffold.go`:
- Around line 52-54: The call to scaffold.Run is hard-coding venv.OSVenv(),
bypassing the CLI-threaded virtual environment; update the call site that
invokes scaffold.Run(context.Background(), c.logger, venv.OSVenv(), c.opts,
c.component.TerraformSourcePath(), "") to accept and forward the injected CLI
venv (e.g., a venv parameter on the surrounding command struct or method)
instead of calling venv.OSVenv(), or if you cannot wire it now, replace the TODO
with a TODO that includes a tracking issue ID and a short rationale; ensure you
reference scaffold.Run and the venv argument so the leaf participates in the
root virtualized environment.
🪄 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: 6dda1b8b-5c5c-4f8a-9065-2b33f67d3791
📒 Files selected for processing (88)
internal/cli/app.gointernal/cli/app_test.gointernal/cli/commands/aws-provider-patch/aws-provider-patch.gointernal/cli/commands/aws-provider-patch/cli.gointernal/cli/commands/backend/cli.gointernal/cli/commands/backend/migrate/cli.gointernal/cli/commands/backend/migrate/migrate.gointernal/cli/commands/catalog/tui/redesign/scaffold.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/commands.gointernal/cli/commands/exec/cli.gointernal/cli/commands/exec/exec.gointernal/cli/commands/hcl/cli.gointernal/cli/commands/hcl/validate/cli.gointernal/cli/commands/hcl/validate/validate.gointernal/cli/commands/info/cli.gointernal/cli/commands/info/print/cli.gointernal/cli/commands/info/print/print.gointernal/cli/commands/render/cli.gointernal/cli/commands/render/render.gointernal/cli/commands/render/render_test.gointernal/cli/commands/run/cli.gointernal/cli/commands/run/run.gointernal/cli/commands/run_action_test.gointernal/cli/commands/scaffold/cli.gointernal/cli/commands/scaffold/scaffold.gointernal/cli/commands/shortcuts.gointernal/cli/commands/stack/cli.gointernal/cli/commands/stack/stack.gointernal/discovery/constructor.gointernal/discovery/discovery.gointernal/discovery/options.gointernal/discovery/phase_parse.gointernal/discovery/types.gointernal/getter/casgetter_tfr_test.gointernal/getter/client_test.gointernal/getter/defaults.gointernal/getter/defaults_test.gointernal/getter/filecopy.gointernal/getter/options_test.gointernal/getter/tfr.gointernal/getter/tfr_test.gointernal/getter/types_test.gointernal/prepare/prepare.gointernal/remotestate/remote_state.gointernal/runner/common/runner.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/download_source.gointernal/runner/run/download_source_test.gointernal/runner/run/run.gointernal/runner/run/venv.gointernal/runner/run/version_check_mem_test.gointernal/runner/runall/runall.gointernal/runner/runall/runall_test.gointernal/runner/runner.gointernal/runner/runnerpool/builder.gointernal/runner/runnerpool/builder_helpers.gointernal/runner/runnerpool/graph_fallback_test.gointernal/runner/runnerpool/runner.gointernal/services/catalog/catalog.gointernal/shell/git.gointernal/shell/git_mem_test.gointernal/shell/git_windows_test.gointernal/shell/run_cmd_mem_test.gointernal/shell/run_cmd_test.gointernal/tflint/venv.gointernal/venv/venv.gomain.gopkg/config/config.gopkg/config/config_helpers.gopkg/config/config_helpers_mem_test.gopkg/config/config_helpers_test.gopkg/config/config_partial.gopkg/config/dependency.gopkg/config/early_stack_eval.gopkg/config/exclude.gopkg/config/fuzz_test.gopkg/config/locals.gopkg/config/parsing_context.gopkg/config/stack.gotest/benchmarks/helpers/helpers.go
✅ Files skipped from review due to trivial changes (3)
- internal/venv/venv.go
- internal/getter/client_test.go
- pkg/config/exclude.go
🚧 Files skipped from review as they are similar to previous changes (65)
- internal/runner/run/creds/providers/externalcmd/provider_test.go
- internal/runner/common/runner.go
- internal/runner/runall/runall_test.go
- internal/runner/run/creds/providers/amazonsts/provider.go
- internal/cli/commands/info/cli.go
- test/benchmarks/helpers/helpers.go
- internal/cli/commands/aws-provider-patch/cli.go
- internal/cli/commands/run_action_test.go
- internal/cli/commands/shortcuts.go
- pkg/config/parsing_context.go
- internal/discovery/discovery.go
- internal/discovery/types.go
- internal/getter/filecopy.go
- internal/getter/types_test.go
- internal/shell/git_windows_test.go
- internal/cli/commands/exec/cli.go
- internal/cli/commands/render/render_test.go
- internal/discovery/constructor.go
- internal/remotestate/remote_state.go
- internal/discovery/options.go
- internal/cli/commands/hcl/cli.go
- internal/shell/run_cmd_mem_test.go
- internal/runner/run/creds/providers/provider.go
- internal/cli/commands/backend/cli.go
- internal/shell/git.go
- internal/cli/app.go
- internal/runner/common/unit_runner.go
- internal/cli/commands/backend/migrate/cli.go
- internal/cli/commands/render/render.go
- internal/cli/commands/aws-provider-patch/aws-provider-patch.go
- main.go
- internal/runner/run/creds/providers/externalcmd/provider_mem_test.go
- internal/cli/commands/catalog/tui/redesign/update.go
- internal/cli/commands/scaffold/cli.go
- internal/runner/run/version_check_mem_test.go
- internal/runner/run/creds/providers/externalcmd/provider.go
- internal/getter/tfr.go
- internal/cli/commands/stack/stack.go
- internal/cli/commands/exec/exec.go
- internal/runner/run/venv.go
- internal/runner/runnerpool/graph_fallback_test.go
- internal/cli/commands/info/print/print.go
- internal/cli/commands/run/cli.go
- internal/runner/runnerpool/builder_helpers.go
- internal/cli/commands/info/print/cli.go
- internal/cli/commands/hcl/validate/cli.go
- internal/runner/run/download_source.go
- pkg/config/config.go
- pkg/config/config_helpers_test.go
- internal/cli/commands/render/cli.go
- internal/shell/run_cmd_test.go
- internal/runner/runner.go
- internal/getter/options_test.go
- internal/discovery/phase_parse.go
- internal/cli/commands/stack/cli.go
- pkg/config/dependency.go
- internal/runner/run/creds/getter.go
- internal/prepare/prepare.go
- internal/runner/run/download_source_test.go
- internal/shell/git_mem_test.go
- internal/cli/commands/scaffold/scaffold.go
- internal/cli/app_test.go
- pkg/config/config_helpers_mem_test.go
- internal/cli/commands/commands.go
- internal/runner/run/run.go
🛑 Comments failed to post (2)
internal/cli/commands/run/run.go (1)
170-177:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the injected filesystem in the version fast-path.
Line 171 still gates config loading with
util.FileExists, soterragrunt run -- versioncan silently skipopts.TerragruntConfigPathwhen the file only exists inv.FS. That reintroduces an OS-filesystem dependency into the one path this PR is trying to virtualize end-to-end.Use
v.FSfor the existence check as well, or just attempt the partial parse and treat not-found as the fast-path miss.🤖 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/run/run.go` around lines 170 - 177, In getTFPathFromConfig replace the OS-only existence check (util.FileExists(opts.TerragruntConfigPath)) with a check that uses the injected virtual filesystem (v.FS) or simply attempt to call getTerragruntConfig and treat a not-found error as the fast-path miss; specifically, update the existence check in getTFPathFromConfig to query v.FS for opts.TerragruntConfigPath (or remove the pre-check and let getTerragruntConfig return an error you handle) so the --version fast-path no longer depends on the OS filesystem.pkg/config/config_partial.go (1)
330-332:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="pkg/config/config_partial.go" # Show context around each referenced range for start in 300 420 620 700; do end=$((start+120)) echo "===== $FILE:$start-$end =====" nl -ba "$FILE" | sed -n "${start},${end}p" echo done # Targeted symbol searches within the file echo "===== rg in $FILE: experiment.StackDependencies =====" rg -n "experiment\.StackDependencies|StackDependencies" "$FILE" || true echo "===== rg in $FILE: skipAutoIncludeMerge|autoIncludeCacheKeySuffix|autoinclude" =====" rg -n "skipAutoIncludeMerge|autoIncludeCacheKeySuffix|autoInclude|autoinclude" "$FILE" || true # Trace definitions/usages elsewhere in repo echo "===== rg repo: experiment.StackDependencies =====" rg -n "experiment\.StackDependencies|StackDependencies" . | head -n 50 echo "===== rg repo: autoIncludeCacheKeySuffix =====" rg -n "autoIncludeCacheKeySuffix" . | head -n 50 echo "===== rg repo: skipAutoIncludeMerge =====" rg -n "skipAutoIncludeMerge" . | head -n 50Repository: gruntwork-io/terragrunt
Length of output: 178
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="pkg/config/config_partial.go" echo "== tool availability ==" command -v rg || true command -v sed || true command -v cat || true command -v awk || true echo "== context around key ranges ==" for start in 300 420 430 620 650 700 730; do end=$((start+80)) echo "===== $FILE:$start-$end =====" cat -n "$FILE" | sed -n "${start},${end}p" || true echo done echo "== symbol searches in file ==" rg -n "experiment\.StackDependencies|StackDependencies" "$FILE" || true rg -n "skipAutoIncludeMerge|autoIncludeCacheKeySuffix|autoinclude" "$FILE" || true echo "== usages in repo (limited output) ==" rg -n "experiment\.StackDependencies|StackDependencies" . | head -n 50 || true rg -n "autoIncludeCacheKeySuffix" . | head -n 50 || true rg -n "skipAutoIncludeMerge" . | head -n 50 || trueRepository: gruntwork-io/terragrunt
Length of output: 35056
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="pkg/config/config_partial.go" echo "== context around autoIncludeCacheKeySuffix (lines ~840-910) ==" cat -n "$FILE" | sed -n '820,910p' echo echo "== context around mergeAutoIncludePartialIfPresent and reconciliation (lines ~740-820) ==" cat -n "$FILE" | sed -n '720,840p' echo echo "== look up skipAutoIncludeMerge definition/usage in parsing_context.go and config.go ==" for f in pkg/config/parsing_context.go pkg/config/config.go pkg/config/dependency.go; do echo "----- $f: tail context -----" cat -n "$f" | sed -n '80,160p' || true echo done echo "----- pkg/config/config.go: around skipAutoIncludeMerge toggles -----" cat -n pkg/config/config.go | sed -n '2250,2340p' || true echo echo "----- pkg/config/dependency.go: around autoPctx.skipAutoIncludeMerge -----" cat -n pkg/config/dependency.go | sed -n '1640,1705p' || trueRepository: gruntwork-io/terragrunt
Length of output: 25267
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "type Experiments|Evaluate\\(|EnableExperiment\\(" pkg internal -S | head -n 50 || true rg -n "StackDependencies" internal/experiment pkg/config -S || true rg -n "env|ENV|os\\.Getenv|flag\\.|pflag|cobra|build tag|//go:build|debug" internal/experiment pkg -S | head -n 100 || true # Print internal/experiment/experiment.go top and relevant parts if [ -f internal/experiment/experiment.go ]; then echo "===== internal/experiment/experiment.go (head) =====" sed -n '1,220p' internal/experiment/experiment.go fi # Print parsing_context Experiments field type and construction if found if [ -f pkg/experiment/experiment.go ]; then sed -n '1,220p' pkg/experiment/experiment.go fiRepository: gruntwork-io/terragrunt
Length of output: 25300
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate Evaluate implementation rg -n "func \\(exps? .*\\) Evaluate\\(" internal/experiment pkg -S rg -n "Enabled" internal/experiment pkg | head -n 80 # Search for where experiments are enabled from flags/env rg -n "EnableExperiment\\(|ExperimentMode\\(|experiments" internal pkg -S | head -n 80 rg -n "EXPERIMENT|TG_.*EXPERIMENT|STACK_DEPENDENCIES|stack-dependencies" internal pkg -S || true # Inspect internal experiment option wiring if any fd -t f -a internal pkg | wc -l >/dev/null 2>&1 || trueRepository: gruntwork-io/terragrunt
Length of output: 12029
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for likely env vars/flags controlling experiments rg -n "TG_.*EXPERIMENT|EXPERIMENTS?|--experiment|experiment(s)?=" . | head -n 80 || true rg -n "type Options|Experiments" pkg/options internal pkg -S | head -n 80 || true # Search for documentation strings referencing experiments env vars rg -n "docs/.*experiments|reference/experiments|experiments.md" . | head -n 50 || true # Look for flags parsing in options / CLI rg -n "EnableExperiment\\(|NewExperiments\\(|ExperimentMode\\(" pkg internal -S | head -n 80 || true # Inspect pkg/options/options.go around where Experiments are configured if [ -f pkg/options/options.go ]; then echo "===== pkg/options/options.go around experiments config =====" rg -n "Experiments" pkg/options/options.go # show some surrounding context around matches awk 'NR>=1 && NR<=260 {print}' pkg/options/options.go >/dev/null 2>&1 || true sed -n '120,260p' pkg/options/options.go fiRepository: gruntwork-io/terragrunt
Length of output: 49606
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "TG_.*EXPERIMENT|EXPERIMENTS?|--experiment|experiment(s)?=" . | head -n 120 || true rg -n "EnableExperiment\\(|NewExperiments\\(|ExperimentMode\\(" pkg internal -S | head -n 120 || true rg -n "Experiments" pkg/options/options.go | head -n 80 || true if [ -f pkg/options/options.go ]; then echo "===== pkg/options/options.go: show likely experiments-related sections =====" # show broad slice where experiments are declared and wired sed -n '1,260p' pkg/options/options.go | rg -n "Experiments|experiment|EXPERIMENT" || true sed -n '260,620p' pkg/options/options.go | rg -n "Experiments|experiment|EXPERIMENT" || true fiRepository: gruntwork-io/terragrunt
Length of output: 32727
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "TG_.*EXPERIMENT|EXPERIMENTS?|--experiment|experiment(s)?=" . | head -n 120 || true rg -n "EnableExperiment\\(|NewExperiments\\(|ExperimentMode\\(" pkg internal -S | head -n 120 || true rg -n "Experiments" pkg/options/options.go | head -n 120 || true if [ -f pkg/options/options.go ]; then echo "===== pkg/options/options.go: experiments-related context =====" sed -n '1,260p' pkg/options/options.go | rg -n "Experiments|experiment|EXPERIMENT" || true sed -n '260,620p' pkg/options/options.go | rg -n "Experiments|experiment|EXPERIMENT" || true fiRepository: gruntwork-io/terragrunt
Length of output: 32714
Downgrade: autoinclude behavior is explicitly experiment-gated and explained in-code
The sibling autoinclude cache-key/merge logic is clearly documented and guarded by the
StackDependenciesexperiment (pctx.Experiments.Evaluate(experiment.StackDependencies)) plus a recursion-prevention flag (skipAutoIncludeMerge), so this isn’t an env/host/debug/date/build-tag time-bomb style gate. The comments already spell out why the partial-parse cache key is modified and why the merge is bounded.What’s still missing is tying this StackDependencies/autoinclude work to the PR’s stated goal: add a short rationale in the PR description (or link the relevant RFC, noted in the experiment docs as RFC
#5663) for why this is included in the same changeset.🤖 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_partial.go` around lines 330 - 332, Add a short rationale in the PR description (or link RFC `#5663`) explaining why the StackDependencies experiment and sibling autoinclude merge (guarded by pctx.Experiments.Evaluate(experiment.StackDependencies) and the recursion-prevention flag skipAutoIncludeMerge) are bundled into this changeset; mention that autoIncludeCacheKeySuffix is used to fold sibling autoinclude state into the partial-parse cache key and that the merge is experiment-gated to avoid changing the experiment-off key, so reviewers can see the design intent and link to the RFC for deeper context.
denis256
approved these changes
Jun 9, 2026
| // FromRoot projects the root [venv.Venv] threaded from the CLI entrypoint | ||
| // into the tflint package's local Venv. The two carry the same handles but | ||
| // are distinct types so the tflint package owns its own contract. | ||
| func FromRoot(v venv.Venv) Venv { |
Member
There was a problem hiding this comment.
Looks like the function is not called from any place
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
2 participants
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
Adds a top-level
venvpackage and threads through a handle to all call-sites that interact with side-effects so that things likevfsandvexeccan be bundled together when a package requires multiple handles.TODOs
Read the Gruntwork contribution guidelines.
Summary by CodeRabbit