Repository navigation
refactor: stage DAG spec compiler - #2186
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors DAG and step build flows into ordered staged pipelines. Transformers, action builders, and validators are grouped into ordered stages. dag.build becomes a dagBuildState workflow with discrete state methods. Loader now applies base/history/location data directly to the built DAG. ChangesStaged Build Pipeline Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 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
🤖 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/spec/dag.go`:
- Around line 700-710: composeInheritedContext currently computes merged via
composeBuildDAGContext and stores it in s.effective but subsequent build steps
keep mutating and returning s.result, so inherited DAG fields never make it into
the final DAG; fix by applying the merged context back into the result used by
the rest of the pipeline (e.g. assign merged into s.result or merge merged
fields into s.result) so that subsequent code paths that read/modify s.result
(and functions that later return s.result) include inherited fields; update
composeInheritedContext and the analogous places noted (the other compose/merge
points) to ensure the merged context from composeBuildDAGContext is propagated
into the final s.result before further mutations and returns.
🪄 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: 6204fa34-8b37-4853-b772-3f7180aa0580
📒 Files selected for processing (2)
internal/core/spec/dag.gointernal/core/spec/step.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 397a577fb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/spec/dag.go (1)
2950-2956: ⚡ Quick winConsider deep cloning handler steps in
cloneStepPointerto avoid unpredictable mutations during merge.
cloneStepPointercreates a shallow copy (cloned := *step), which shares references to all slice and map fields (Depends, Env, Commands, Preconditions, etc.) with the original Step. Whenmergesubsequently callsmergo.Merge, mergo appends to destination slices, and depending on slice capacity, this may modify the shared underlying arrays. While the current code works, this pattern is fragile and violates the intent of cloning—to create an independent copy before merging. A deep clone would eliminate the ambiguity and ensure handler steps can be safely composed without unexpected mutations.🤖 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/spec/dag.go` around lines 2950 - 2956, cloneStepPointer currently does a shallow copy which leaves slice and map fields shared and allows mergo.Merge to mutate original data; change cloneStepPointer to perform a deep clone by copying all slice and map fields of core.Step (e.g., Depends, Env, Commands, Preconditions and any other [] or map fields on core.Step) into newly allocated slices/maps and also deep-copy any nested *core.Step pointers or handler-related structs before returning, so the returned *core.Step is independent and safe to merge with mergo.Merge.
🤖 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/spec/dag.go`:
- Around line 2950-2956: cloneStepPointer currently does a shallow copy which
leaves slice and map fields shared and allows mergo.Merge to mutate original
data; change cloneStepPointer to perform a deep clone by copying all slice and
map fields of core.Step (e.g., Depends, Env, Commands, Preconditions and any
other [] or map fields on core.Step) into newly allocated slices/maps and also
deep-copy any nested *core.Step pointers or handler-related structs before
returning, so the returned *core.Step is independent and safe to merge with
mergo.Merge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bf0c8e5-dc2a-476c-96df-c774cf88395d
📒 Files selected for processing (2)
internal/core/spec/dag.gointernal/core/spec/loader_test.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1a79bbf20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Refactor the DAG spec compiler internals into explicit stages while preserving the public
spec.Loadinterface and existing base-config inheritance behavior.Changes
Related Issues
N/A
Checklist
Testing
go test ./internal/core/specgo test ./internal/core/exec -run 'TestAbortQueuedDAGRun' -count=20go test ./internal/intg -run 'Test(BaseDAGSpecialEnvVarsInHandler|SkipBaseHandlers)' -count=1go test ./internal/core/...go test ./internal/cmd ./internal/service/scheduler ./internal/service/frontend/api/v1make lintgit diff --check -- internal/core/spec/dag.go internal/core/spec/loader_test.go internal/core/exec/queue_abort_test.goSummary by CodeRabbit
Refactor
Tests