fix: resolve runtime env references in DAG env#2131
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRuntime-managed envs are built centrally, DAG Env entries are runtime-evaluated sequentially with managed keys protected, user envs are merged, and managed envs are reapplied for final precedence. Runner now writes evaluated step/container env entries directly into the env scope. Tests validate sequential evaluation and precedence. ChangesEnvironment Variable Sequential Evaluation
Sequence Diagram(s)sequenceDiagram
participant OS as OS Env
participant NewCtx as NewContext
participant Runner as Runner.setupVariables
participant Scope as Env Scope
OS->>NewCtx: provide base env
NewCtx->>Scope: seed base + managed runtimeEnvs
NewCtx->>Scope: evaluate DAG Env (protected keys)
NewCtx->>Runner: pass env scope
Runner->>Scope: evaluate step/container env entries sequentially
Scope-->>Runner: provide vars for later entries
Runner->>Scope: write entries as evaluated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/runtime/runner_internal_test.go (1)
89-119: ⚡ Quick winAdd a container-env regression alongside this step-env case.
This only exercises
Step.Env, but the behavior change also coversstep.Container.Envand DAG container fallback. One small case that resolves a later container env entry from an earlier one would lock down the other new branch insetupVariables.🤖 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/runtime/runner_internal_test.go` around lines 89 - 119, Add a parallel regression that exercises container-level env resolution in setupVariables: extend TestSetupVariables_StepEnvEvaluatesSequentiallyWithRuntimeVars to also set step.Container.Env (e.g., "WORK_DIR=${DAG_RUN_ARTIFACTS_DIR}" and "CURRENT_IDEA_PATH=${WORK_DIR}/current_idea.md") and assert those container envs are resolved the same way as step.Env; additionally add a sub-case where step.Container is nil so setupVariables falls back to DAG.Container.Env and verify fallback resolution works (use NewPlan/plan.GetNodeByName, runner.setupVariables, and AllEnvsMap as in the existing test to locate and validate WORK_DIR and CURRENT_IDEA_PATH).
🤖 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/core/exec/context.go`:
- Around line 339-343: The DAG env evaluation currently allows user DAG entries
to write reserved runtime variables (runtimeEnvs) into the interim env map,
causing derived vars to resolve against incorrect values; fix by ensuring
runtimeEnvs are injected into envs before calling evaluateDAGEnvRuntime (or
alter evaluateDAGEnvRuntime to ignore/skip keys present in runtimeEnvs) so
reserved keys are immutable during evaluation; apply the same change to the
other evaluation site around the 390-402 block; reference evaluateDAGEnvRuntime,
runtimeEnvs, envs, and stringutil.KeyValuesToMap when making the adjustment.
---
Nitpick comments:
In `@internal/runtime/runner_internal_test.go`:
- Around line 89-119: Add a parallel regression that exercises container-level
env resolution in setupVariables: extend
TestSetupVariables_StepEnvEvaluatesSequentiallyWithRuntimeVars to also set
step.Container.Env (e.g., "WORK_DIR=${DAG_RUN_ARTIFACTS_DIR}" and
"CURRENT_IDEA_PATH=${WORK_DIR}/current_idea.md") and assert those container envs
are resolved the same way as step.Env; additionally add a sub-case where
step.Container is nil so setupVariables falls back to DAG.Container.Env and
verify fallback resolution works (use NewPlan/plan.GetNodeByName,
runner.setupVariables, and AllEnvsMap as in the existing test to locate and
validate WORK_DIR and CURRENT_IDEA_PATH).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02b536a3-ec8a-4bc5-a95c-c851fc306119
📒 Files selected for processing (4)
internal/core/exec/context.gointernal/core/exec/context_test.gointernal/runtime/runner.gointernal/runtime/runner_internal_test.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/exec/context_env_internal_test.go (1)
23-25: ⚡ Quick winUse
require.Failfinstead oft.Fatalfin this test.This keeps
_test.goassertions consistent with the repository’s testify rule.Proposed change
if _, ok := seen[env.key]; ok { - t.Fatalf("duplicate managed DAG-run env key: %s", env.key) + require.Failf(t, "duplicate managed DAG-run env key", "%s", env.key) }As per coding guidelines:
**/*_test.go: Use stretchr/testify assertions for testing in Go.🤖 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/core/exec/context_env_internal_test.go` around lines 23 - 25, Replace the direct t.Fatalf call with the testify require helper: import "github.com/stretchr/testify/require" and change the failing assertion in the duplicate check (the block that tests seen[env.key] and currently calls t.Fatalf("duplicate managed DAG-run env key: %s", env.key)) to use require.Failf(t, "<short message>", "%s", env.key) (or require.Failf(t, "duplicate managed DAG-run env key", "%s", env.key)) so the test follows the repository’s stretchr/testify convention.
🤖 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.
Nitpick comments:
In `@internal/core/exec/context_env_internal_test.go`:
- Around line 23-25: Replace the direct t.Fatalf call with the testify require
helper: import "github.com/stretchr/testify/require" and change the failing
assertion in the duplicate check (the block that tests seen[env.key] and
currently calls t.Fatalf("duplicate managed DAG-run env key: %s", env.key)) to
use require.Failf(t, "<short message>", "%s", env.key) (or require.Failf(t,
"duplicate managed DAG-run env key", "%s", env.key)) so the test follows the
repository’s stretchr/testify convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77d3062d-aa0d-4829-8413-86cf0801f909
📒 Files selected for processing (4)
internal/core/exec/context.gointernal/core/exec/context_env.gointernal/core/exec/context_env_internal_test.gointernal/core/exec/context_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/exec/context.go
- internal/core/exec/context_test.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
Testing
Summary by CodeRabbit
New Features
Tests