fix: align precondition env with step execution by yottahmd · Pull Request #2080 · dagucloud/dagu

yottahmd · 2026-05-02T11:48:37Z

Summary

move runtime step env setup ahead of precondition evaluation
reuse the same setup path for push-back env injection so preconditions see the same inputs as the step command
add regressions for runtime-managed step env and push-back env parity

Testing

go test ./internal/runtime -count=1

Summary by CodeRabbit

Bug Fixes
- Precondition checks now consistently observe the same runtime-managed environment variables and injected push-back inputs as the commands they guard, preventing mismatches and unexpected failures.
Refactor
- Consolidated environment setup to ensure consistent behavior across normal and rewound (push-back) executions.
Tests
- Added tests verifying preconditions access the correct environment during step and push-back runs.
- Increased artifact execution test timeouts for platform and persistence variability.

coderabbitai · 2026-05-02T11:48:52Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cbe1c48b-a692-47d3-9cf4-8e93daa32adc

📥 Commits

Reviewing files that changed from the base of the PR and between cd41ee1 and f816bfb.

📒 Files selected for processing (1)

internal/intg/distr/execution_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

internal/intg/distr/execution_test.go

📝 Walkthrough

Walkthrough

Extracts node environment setup into a new helper setupNodeExecutionEnv and invokes it before precondition checks so preconditions observe the same runtime-managed environment (including injected push-back inputs when ApprovalIteration > 0). Adds tests validating preconditions read the same env as step commands and increases artifact-status timeouts in integration tests.

Changes

Environment Setup Refactor & Precondition Visibility

Layer / File(s)	Summary
Data / Env shape `internal/runtime/.../runner.go`	Adds logic to filter allowed `state.PushBackInputs` by `step.Approval.Input`, marshal a payload for `exec.EnvKeyDAGPushBack`, and prepare per-input env vars when `node.State().ApprovalIteration != 0`.
Core implementation `internal/runtime/runner.go`	Adds `(Runner).setupNodeExecutionEnv(ctx context.Context, node Node) context.Context` which calls `node.SetupEnv(ctx)`, injects push-back env values, warns on unexpected inputs and marshal failures, and returns the updated context.
Callsite ordering / Control flow `internal/runtime/runner.go`	`runNodeExecution` now calls `setupNodeExecutionEnv` immediately after span creation and before `meetsPreconditions`, moving env initialization earlier so preconditions evaluate with the updated env.
Tests `internal/runtime/runner_test.go`	Adds `TestPreconditionUsesSameRuntimeManagedStepEnvAsCommand` and `TestPushBackPreconditionUsesSameEnvAsCommand` (both skip on Windows) to assert preconditions can read runtime-managed vars like `DAG_RUN_STEP_STDOUT_FILE` and injected `FEEDBACK`.

Integration Test Timeouts

Layer / File(s)	Summary
Test helper `internal/intg/distr/execution_test.go`	Introduces `artifactExecutionStatusTimeout()` with OS-/race-aware durations (longer on Windows and for shared-nothing persistence).
Test updates `internal/intg/distr/execution_test.go`	Replaces calls to `executionStatusTimeout()` with `artifactExecutionStatusTimeout()` in artifact-related tests to use the new, longer timeouts.

Sequence Diagram(s)

(Skipped)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

fix: align command condition expansion with shell commands #2076: Related — also touches precondition/command evaluation and runtime-managed environment/expansion semantics.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main change: moving environment setup before precondition evaluation to align precondition environment with step execution.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/precondition-env-parity

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/runtime/runner_test.go`:
- Around line 835-845: The test
PreconditionUsesSameRuntimeManagedStepEnvAsCommand uses POSIX shell commands
(`printf` in withCommand and `test -n` in withPrecondition) so make it skip on
Windows: at the start of the test body (inside the t.Run for
PreconditionUsesSameRuntimeManagedStepEnvAsCommand) add a Windows guard such as
`if runtime.GOOS == "windows" { t.Skip("skipping POSIX shell test on Windows")
}`, import the runtime package if not already imported, and leave the rest of
the newPlan/newStep/withCommand/withPrecondition setup 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e218ab66-d679-4ccc-ab6d-4e8f5d6805a0

📥 Commits

Reviewing files that changed from the base of the PR and between 4570208 and 95df748.

📒 Files selected for processing (2)

internal/runtime/runner.go
internal/runtime/runner_test.go

yottahmd · 2026-05-02T11:56:20Z

@coderabbitai review

coderabbitai · 2026-05-02T11:56:25Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

yottahmd · 2026-05-02T13:38:10Z

@coderabbitai review

coderabbitai · 2026-05-02T13:38:15Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/intg/distr/execution_test.go (1)
256-340: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Complete the artifact-timeout migration in the remaining artifact-persistence test.

artifactExecutionStatusTimeout() is now used in most artifact paths, but Line 229 still uses executionStatusTimeout() inside sharedNothingUploadsArtifactsToCoordinatorFilesystem. That keeps the shorter Windows timeout in the same mode this helper targets, so flakes can still occur.
Suggested patch
-		status := f.waitForStatus(core.Succeeded, executionStatusTimeout())
+		status := f.waitForStatus(core.Succeeded, artifactExecutionStatusTimeout())
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/intg/distr/execution_test.go` around lines 256 - 340, In the test
sharedNothingUploadsArtifactsToCoordinatorFilesystem replace the shorter timeout
helper call executionStatusTimeout() with artifactExecutionStatusTimeout() so
the test uses the artifact-specific (longer) timeout; update the waitForStatus
call (and any other uses in that test) to call artifactExecutionStatusTimeout()
instead of executionStatusTimeout() to complete the artifact-timeout migration.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@internal/intg/distr/execution_test.go`:
- Around line 256-340: In the test
sharedNothingUploadsArtifactsToCoordinatorFilesystem replace the shorter timeout
helper call executionStatusTimeout() with artifactExecutionStatusTimeout() so
the test uses the artifact-specific (longer) timeout; update the waitForStatus
call (and any other uses in that test) to call artifactExecutionStatusTimeout()
instead of executionStatusTimeout() to complete the artifact-timeout migration.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 01d8aba5-190c-4566-b066-ccef8bf84b61

📥 Commits

Reviewing files that changed from the base of the PR and between 51c1aa4 and cd41ee1.

📒 Files selected for processing (1)

internal/intg/distr/execution_test.go

yottahmd · 2026-05-02T13:53:35Z

@coderabbitai review

coderabbitai · 2026-05-02T13:53:40Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

fix: align precondition env with step execution

95df748

coderabbitai Bot reviewed May 2, 2026

View reviewed changes

Comment thread internal/runtime/runner_test.go

test: guard runtime env parity regression on Windows

51c1aa4

yottahmd closed this May 2, 2026

yottahmd reopened this May 2, 2026

test: relax distributed artifact timeout on Windows

cd41ee1

coderabbitai Bot reviewed May 2, 2026

View reviewed changes

test: finish artifact timeout migration

f816bfb

yottahmd merged commit 366758f into main May 2, 2026
10 checks passed

yottahmd deleted the fix/precondition-env-parity branch May 2, 2026 14:35

coderabbitai Bot mentioned this pull request May 6, 2026

feat: carry push-back context into AI steps #2109

Merged

Uh oh!

Conversation

yottahmd commented May 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yottahmd commented May 2, 2026

Uh oh!

coderabbitai Bot commented May 2, 2026

Uh oh!

yottahmd commented May 2, 2026

Uh oh!

coderabbitai Bot commented May 2, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

yottahmd commented May 2, 2026

Uh oh!

coderabbitai Bot commented May 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

yottahmd commented May 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 2, 2026 •

edited

Loading