Repository navigation
refactor: decouple DAGStore from the loader and derive the rebuild restore set - #2527
Conversation
DAGStore declared two methods taking spec.LoadOption, so the port interface was parameterised by its implementation's option type. Every consumer of internal/core/exec therefore compiled against the YAML loader, whether or not it touched a DAG store at all. Define the options the port actually needs and let the file-backed store translate them. Across the whole codebase these two methods were called with three of the eleven available loader options, and one of those three was redundant: both implementations already append WithoutEval unconditionally, so a caller passing it changed nothing. What remains is whether build errors are tolerated, plus the name LoadSpec builds under, which every caller supplied. This takes internal/core/spec, internal/core/spec/types, internal/llm and internal/cmn/templatefuncs out of the transitive closure of everything importing exec, cutting the packages that compile against the loader from 85 to 27. Four test doubles stop importing spec as well.
|
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR replaces variadic ChangesDAG loading and field restoration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant FrontendAPI
participant DAGStore
participant FileStorage
participant YAMLLoader
FrontendAPI->>DAGStore: LoadSpec with name and DAGLoadOptions
DAGStore->>FileStorage: Translate AllowBuildErrors
FileStorage->>YAMLLoader: Load YAML with name and loader options
YAMLLoader-->>FileStorage: DAG or partial DAG with build errors
FileStorage-->>DAGStore: Return loaded DAG
DAGStore-->>FrontendAPI: Return DAG result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/persis/file/dag/store_test.go (1)
364-387: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTest
GetDetailswithAllowBuildErrors.The test only uses
exec.DAGLoadOptions{}. Add a DAG with a build error. Assert thatGetDetailsreturns the partially built DAG whenAllowBuildErrorsis true. Assert the default path keeps the existing failure behavior.As per coding guidelines, “Add or update tests appropriate to the changed code.”
🤖 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/persis/file/dag/store_test.go` around lines 364 - 387, Extend the GetDetails test to create a DAG with a build error, then verify the default exec.DAGLoadOptions{} path preserves the existing error behavior. Also call GetDetails with AllowBuildErrors enabled and assert it returns the partially built DAG without failing.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/cmn/telemetry/collector_test.go (1)
98-99: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep the explicit
LoadSpecname observable in both mocks.Both mocks accept the new
nameparameter but omit it fromm.Called. Tests cannot detect an incorrect DAG name.
internal/cmn/telemetry/collector_test.go#L98-L99: passnametom.Called.internal/runtime/agent/dbclient_test.go#L93-L94: passnametom.Called.As per coding guidelines, add or update tests appropriate to changed Go code.
Proposed fix
-func (m *mockDAGStore) LoadSpec(ctx context.Context, source []byte, _ string, opts exec.DAGLoadOptions) (*core.DAG, error) { - args := m.Called(ctx, source, opts) +func (m *mockDAGStore) LoadSpec(ctx context.Context, source []byte, name string, opts exec.DAGLoadOptions) (*core.DAG, error) { + args := m.Called(ctx, source, name, opts)🤖 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/cmn/telemetry/collector_test.go` around lines 98 - 99, Update mockDAGStore.LoadSpec in internal/cmn/telemetry/collector_test.go at lines 98-99 to include the explicit name argument in m.Called, and make the same change in internal/runtime/agent/dbclient_test.go at lines 93-94. Update or add relevant tests so incorrect DAG names are detectable by both mocks.Source: Coding guidelines
🤖 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/persis/file/dag/store_test.go`:
- Around line 1046-1053: Extend the LoadSpec test to pass a non-empty explicit
name different from the YAML name and assert dag.Name uses the explicit value.
Add a separate LoadSpec case using exec.DAGLoadOptions with AllowBuildErrors
enabled and assert the expected build-error behavior.
---
Outside diff comments:
In `@internal/persis/file/dag/store_test.go`:
- Around line 364-387: Extend the GetDetails test to create a DAG with a build
error, then verify the default exec.DAGLoadOptions{} path preserves the existing
error behavior. Also call GetDetails with AllowBuildErrors enabled and assert it
returns the partially built DAG without failing.
---
Nitpick comments:
In `@internal/cmn/telemetry/collector_test.go`:
- Around line 98-99: Update mockDAGStore.LoadSpec in
internal/cmn/telemetry/collector_test.go at lines 98-99 to include the explicit
name argument in m.Called, and make the same change in
internal/runtime/agent/dbclient_test.go at lines 93-94. Update or add relevant
tests so incorrect DAG names are detectable by both mocks.
🪄 Autofix
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 Plus
Run ID: 4c863095-3194-4c9b-9948-991e1629e96e
📒 Files selected for processing (15)
internal/cmn/telemetry/collector_test.gointernal/core/exec/dag.gointernal/intg/one_off_schedule_test.gointernal/intg/sched_test.gointernal/persis/file/dag/store.gointernal/persis/file/dag/store_test.gointernal/runtime/agent/dbclient.gointernal/runtime/agent/dbclient_test.gointernal/service/frontend/api/v1/dagruns_test.gointernal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/dags_test.gointernal/service/frontend/api/v1/notifications.gointernal/service/frontend/api/v1/webhooks.gointernal/service/notification/service.gointernal/service/notification/service_test.go
Rebuilding a DAG from its source restored the JSON-excluded fields by copying each one by name. The list had to track the json:"-" tags on DAG by hand, and when it did not, a field was restored on one code path and dropped on another: that is how S3, Redis, Kubernetes and the presolved build env came to be lost on API-initiated retries. Derive the set instead. Every omitted field is restored, and the three that record the outcome of a build rather than configuration are named as exclusions. Adding a new omitted field to DAG now joins the restore automatically, so the default is to keep a value rather than silently drop it. The derived set is the same twelve fields the list named, so behaviour is unchanged.
yohamta0
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. |
LoadSpec now takes the DAG name as a parameter and a struct selecting whether build errors are tolerated, but nothing exercised either: the existing case passed an empty name and zero-value options. Assert that an explicit name overrides the one the spec declares, and that a spec which builds with errors fails unless the caller tolerates them, in which case the partially built DAG comes back carrying them.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cmn/telemetry/collector_test.go (1)
98-99: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRecord
namein bothLoadSpecmock calls.The revised
LoadSpeccontract includes the DAG name, but both Testify mocks omit it fromm.Called. Tests can therefore pass an incorrect name without failing.
internal/cmn/telemetry/collector_test.go#L98-L99: passnametom.Calledand updateOn("LoadSpec", ...)expectations.internal/runtime/agent/dbclient_test.go#L93-L94: passnametom.Calledand updateOn("LoadSpec", ...)expectations.Proposed mock fix
-func (m *mockDAGStore) LoadSpec(ctx context.Context, source []byte, _ string, opts exec.DAGLoadOptions) (*core.DAG, error) { - args := m.Called(ctx, source, opts) +func (m *mockDAGStore) LoadSpec(ctx context.Context, source []byte, name string, opts exec.DAGLoadOptions) (*core.DAG, error) { + args := m.Called(ctx, source, name, opts)As per coding guidelines, update tests for the changed Go contract.
🤖 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/cmn/telemetry/collector_test.go` around lines 98 - 99, Update mockDAGStore.LoadSpec in internal/cmn/telemetry/collector_test.go at lines 98-99 to pass name to m.Called, and update every corresponding On("LoadSpec", ...) expectation in that file. Apply the same change to the LoadSpec mock at internal/runtime/agent/dbclient_test.go lines 93-94 and its expectations so both test mocks validate the DAG name argument.Source: Coding guidelines
internal/core/exec/dag.go (1)
53-54: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for explicit
LoadSpecnames.
LoadSpecnow makesnamepart of the DAG identity contract. The supplied tests call it with an empty name, so they do not detect an adapter that drops or misapplies the explicit name. Add a case withname: yaml-namein the source andrequested-nameas the argument. Assert the intended returnedcore.DAG.Name.The storage adapter in
internal/persis/file/dag/store.go:339-344translates this argument throughspec.WithName(name). As per coding guidelines, add or update tests appropriate to the changed code.🤖 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/dag.go` around lines 53 - 54, Extend the LoadSpec coverage to pass a non-empty requested name, such as “requested-name”, while the YAML also contains “yaml-name”, then assert that the returned core.DAG.Name uses the explicit argument. Add or update the storage-adapter test around spec.WithName(name) to verify this name translation and preserve the explicit-name precedence contract.Source: Coding guidelines
🤖 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/cmn/telemetry/collector_test.go`:
- Around line 98-99: Update mockDAGStore.LoadSpec in
internal/cmn/telemetry/collector_test.go at lines 98-99 to pass name to
m.Called, and update every corresponding On("LoadSpec", ...) expectation in that
file. Apply the same change to the LoadSpec mock at
internal/runtime/agent/dbclient_test.go lines 93-94 and its expectations so both
test mocks validate the DAG name argument.
In `@internal/core/exec/dag.go`:
- Around line 53-54: Extend the LoadSpec coverage to pass a non-empty requested
name, such as “requested-name”, while the YAML also contains “yaml-name”, then
assert that the returned core.DAG.Name uses the explicit argument. Add or update
the storage-adapter test around spec.WithName(name) to verify this name
translation and preserve the explicit-name precedence contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec9d5d8d-ba57-44ff-8d95-192d2cd31b6c
📒 Files selected for processing (17)
internal/cmn/telemetry/collector_test.gointernal/core/exec/dag.gointernal/core/restore.gointernal/core/spec/rebuild.gointernal/intg/one_off_schedule_test.gointernal/intg/sched_test.gointernal/persis/file/dag/store.gointernal/persis/file/dag/store_test.gointernal/runtime/agent/dbclient.gointernal/runtime/agent/dbclient_test.gointernal/service/frontend/api/v1/dagruns_test.gointernal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/dags_test.gointernal/service/frontend/api/v1/notifications.gointernal/service/frontend/api/v1/webhooks.gointernal/service/notification/service.gointernal/service/notification/service_test.go
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai both nitpicks declined — one is already done, the other rests on a premise that does not hold. Evidence for each. ❌ "Add coverage for explicit
|
|
Tip For best results, initiate chat on the files or code changes.
I withdraw both nitpicks.
The ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/persis/file/dag/store_test.go (1)
364-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
GetDetailsbuild-error path.The test covers
AllowBuildErrorsonly throughLoadSpec. Add a DAG file with a build error and assert thatGetDetails(ctx, name, exec.DAGLoadOptions{AllowBuildErrors: true})returns the partial DAG withBuildErrors. This verifies option translation in both storage methods.As per coding guidelines, “Add or update tests appropriate to the changed code.”
🤖 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/persis/file/dag/store_test.go` around lines 364 - 372, Extend the GetDetails test around the existing detailed-dag assertions by creating a DAG file with a build error, then call GetDetails with exec.DAGLoadOptions{AllowBuildErrors: true}. Assert that it returns the partial DAG and exposes the expected BuildErrors, covering option translation independently of the existing LoadSpec test.Source: Coding guidelines
🤖 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/restore.go`:
- Around line 27-45: Add regression tests covering RestoreUnpersistedFrom:
verify JSON-excluded configuration fields copy from src, while EnvEvaluated,
BuildErrors, and BuildWarnings remain unchanged; also test the separate Env
merge behavior through RebuildFromYAML. Use the existing DAG test conventions
and run the Go tests with race detection.
---
Nitpick comments:
In `@internal/persis/file/dag/store_test.go`:
- Around line 364-372: Extend the GetDetails test around the existing
detailed-dag assertions by creating a DAG file with a build error, then call
GetDetails with exec.DAGLoadOptions{AllowBuildErrors: true}. Assert that it
returns the partial DAG and exposes the expected BuildErrors, covering option
translation independently of the existing LoadSpec test.
🪄 Autofix
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 Plus
Run ID: 504c685a-9418-4b52-90c5-30d097d584ce
📒 Files selected for processing (17)
internal/cmn/telemetry/collector_test.gointernal/core/exec/dag.gointernal/core/restore.gointernal/core/spec/rebuild.gointernal/intg/one_off_schedule_test.gointernal/intg/sched_test.gointernal/persis/file/dag/store.gointernal/persis/file/dag/store_test.gointernal/runtime/agent/dbclient.gointernal/runtime/agent/dbclient_test.gointernal/service/frontend/api/v1/dagruns_test.gointernal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/dags_test.gointernal/service/frontend/api/v1/notifications.gointernal/service/frontend/api/v1/webhooks.gointernal/service/notification/service.gointernal/service/notification/service_test.go
RestoreUnpersistedFrom decides both what a rebuild carries over and what it must leave behind, and only the first half was exercised, indirectly, by the rebuild tests. Assert both: the fields JSON omits arrive from the rebuilt DAG, the fields it keeps survive untouched, and the build outcome stays with the DAG being restored rather than being replaced by the outcome of rebuilding it. Also exercise AllowBuildErrors through GetDetails. Both store methods translate the option through the same helper but assemble their loader options separately, so a mistake in one would not surface in the other's test.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
The previous test asserted the restored values field by field. It caught a misclassified field but not a newly added one, which is the oversight that will actually happen: a new JSON-excluded field is restored by default, so it slips through silently even when it describes the build and must not be. Most of what it did assert was already covered by the rebuild tests. Guard the classification instead. Every JSON-excluded field must be named as restored or as build outcome, so adding one fails until the decision is made, and removing the exclusion from a listed field fails as well. RestoreUnpersistedFrom treats every field through one uniform loop, so the loop is proven by any rebuild test that round-trips a field; asserting the same twelve fields individually added lines without adding coverage.
The classification guard proves every JSON-excluded field is assigned a side, but not that either side is honoured: deleting the exclusion check in RestoreUnpersistedFrom left it green while EnvEvaluated, BuildErrors and BuildWarnings were silently overwritten by the rebuild's own outcome. Cover the contract directly. One configuration field must cross, and the three outcome fields must keep the values of the DAG being restored.
1 participant
Two independent simplifications, both behaviour-preserving.
1. Give
DAGStoreits own load-option vocabularyDAGStoreis a port interface, but two of its methods were parameterised by the implementation's option type:That single import at
internal/core/exec/dag.goput the 46k-line YAML loader into the transitive closure of everything importinginternal/core/exec— everyruntime/builtin/*executor,internal/runtime,runtime/executor,runtime/transform, most ofpersis/file. None of them touch a DAG store; they importexecto name aDAGRunStatusor aNode.Give the port the vocabulary it needs and let the adapter translate:
The surface is smaller than it looks. Across the whole codebase these methods were called with three of the eleven loader options, and one was already redundant: both implementations append
spec.WithoutEval()unconditionally and noWithEval()exists to undo it, so the single caller passing it changed nothing. That leaves one real option, plus the name — which everyLoadSpeccaller supplied and which is now an explicit parameter rather than a field meaningful to only one of the two methods.Result
Out of exec's closure entirely:
core/spec(15,734 LOC),core/spec/types(1,648),llm(1,199),cmn/templatefuncs(157). No code is deleted — those still ship in the binary. What changes is how much of the tree recompiles when the loader changes, and that a port no longer depends on its adapter. Four test doubles also stop importingspec.WithName("")sets the zero value, so applying it unconditionally is equivalent to omitting it.2. Derive the rebuild restore set from the DAG's own tags
A DAG decoded from
dag.jsonis incomplete by design: the fields taggedjson:"-"are deliberately never written to disk. Retry and restart reload the stored YAML and copy those fields back — and that copy was a hand-written list of names that had to track the struct tags manually.When it didn't, a field was restored on one path and dropped on another. That is exactly how
S3,Redis,KubernetesandWorkingDirExplicitcame to be lost on API-initiated retries (#2521), and howPresolvedBuildEnvwas discarded (#2522).RestoreUnpersistedFromderives the set instead. Every omitted field is restored; the three recording the outcome of a build rather than configuration are named as exclusions:The failure mode inverts. Adding a new omitted field to
DAGnow joins the restore automatically, so forgetting means the value is kept, not silently dropped.Verified the derived set is exactly the twelve names the list held:
Envis the one field merged rather than replaced, so it is resolved after the wholesale copy.Verification
go build ./...andgo vet ./...clean-racegreen:core,core/exec,core/spec,persis/file/dag,cmn/telemetry,runtime/agent,service/notification,cmd,service/frontend/api/v1golangci-lint run ./...— 0 issues, and 0 underGOOS=windowsgofmtcleanSummary by CodeRabbit
New Features
Bug Fixes