Skip to content

fix: preserve restored step config on approval resume#1959

Merged
yottahmd merged 1 commit intomainfrom
fix/1958-bugfix
Apr 3, 2026
Merged

fix: preserve restored step config on approval resume#1959
yottahmd merged 1 commit intomainfrom
fix/1958-bugfix

Conversation

@yottahmd
Copy link
Copy Markdown
Collaborator

@yottahmd yottahmd commented Apr 3, 2026 •
edited by coderabbitai Bot
Loading

Summary

  • rebind retry nodes to the restored DAG steps before retry planning so resumed execution uses the rebuilt step definitions instead of lossy persisted step snapshots
  • cover the retry planner with regressions for restored step env and step container env on both full DAG retry and step retry paths
  • add restore-path coverage for registry_auths rebuilt from DAG YAML and base.yaml content

Testing

  • go test ./internal/runtime ./internal/cmd ./internal/service/frontend/api/v1 -run 'Test(RetryPlan|StepRetryPlan|RestoreDAGFromStatus|ApproveDAGRunStep)' -count=1
  • go test ./internal/intg -run 'TestStepLevelContainer' -count=1

Closes #1958

Summary by CodeRabbit

  • Improvements

    • Retry operations now properly restore step configuration and environment variables from DAG definitions

  • Tests

    • Added tests verifying registry authentication restoration from DAG configuration
    • Added tests validating step configuration restoration during retry plan execution

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 3, 2026 •
edited
Loading

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: 6f2de2fe-2f77-49fb-a50f-05f65c5bc710

📥 Commits

Reviewing files that changed from the base of the PR and between 3b167ec and 1cc9267.

📒 Files selected for processing (3)
  • internal/cmd/helper_test.go
  • internal/runtime/plan.go
  • internal/runtime/plan_test.go
📝 Walkthrough

Walkthrough

This PR fixes the issue where registry_auths and step env variables are lost when resuming a DAG after an approval step. The solution adds tests for DAG restoration logic and implements a step rebinding mechanism that ensures step definitions from the restored DAG are properly transferred to retry plan nodes before execution.

Changes

Cohort / File(s) Summary
DAG Restoration Tests
internal/cmd/helper_test.go		 Added unit tests for restoreDAGFromStatus verifying that RegistryAuths are correctly reconstructed from YamlData or BaseConfigData with placeholder values preserved.
Retry Plan Rebinding
internal/runtime/plan.go		 Added rebindRetryNodesToSteps helper function that rebinds step definitions from the restored DAG onto retry nodes, called before plan initialization in both CreateRetryPlan and CreateStepRetryPlan.
Retry Plan Rebinding Tests
internal/runtime/plan_test.go		 Added test cases verifying that retry plans properly rebind step definitions (including Env and Container.Env) from the restored DAG onto existing nodes while maintaining expected node status transitions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.00% 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 'fix: preserve restored step config on approval resume' directly reflects the main change: rebinding retry nodes to restored DAG steps to preserve step configuration during approval resume.
Linked Issues check ✅ Passed The PR fully addresses issue #1958 by rebinding retry nodes to restored DAG steps, preserving registry_auths and step env during approval resume, with comprehensive regression tests for both DAG retry and single-step retry paths.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the issue: rebinding logic in plan.go, corresponding tests in plan_test.go and helper_test.go for registry_auths restoration, with no unrelated modifications.

✏️ 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/1958-bugfix

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

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

@yottahmd yottahmd merged commit c4e6ce9 into main Apr 3, 2026
5 checks passed
@yottahmd yottahmd deleted the fix/1958-bugfix branch April 3, 2026 02:52
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 3, 2026

Codecov Report

❌ Patch coverage is 38.46154% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.04%. Comparing base (3b167ec) to head (1cc9267).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
internal/runtime/plan.go 38.46% 4 Missing and 4 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1959      +/-   ##
==========================================
- Coverage   68.05%   68.04%   -0.01%     
==========================================
  Files         475      475              
  Lines       61310    61323      +13     
==========================================
+ Hits        41724    41727       +3     
- Misses      15610    15622      +12     
+ Partials     3976     3974       -2
Files with missing lines Coverage Δ
internal/runtime/plan.go 89.92% <38.46%> (-2.63%) ⬇️

... and 18 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b167ec...1cc9267. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pengu-fr pengu-fr mentioned this pull request Apr 3, 2026
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Apr 3, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

registry_auths and step env lost after approval step resume

1 participant

@yottahmd