-
Notifications
You must be signed in to change notification settings - Fork 530
Make targeted workflow compilation deterministic #57745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
7ed0639
05cf136
89dbde1
0df80ca
85e8e16
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -861,6 +861,12 @@ func runPostProcessing( | |
| // Note: Maintenance workflow generation requires parsing all workflows in the directory | ||
| // to check for expires fields, so we skip it when compiling specific files to avoid | ||
| // unnecessary parsing and warnings from unrelated workflows | ||
| if !config.NoEmit && config.WorkflowDir == "" { | ||
| if gitRoot, err := gitutil.FindGitRoot(); err == nil { | ||
| absWorkflowDir := getAbsoluteWorkflowDir(getWorkflowsDir(), gitRoot) | ||
| workflow.DisableDefaultActionFailureExpiryMarkersIfUnenforced(workflowDataList, absWorkflowDir) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 85e8e16: reconciliation now uses the actual emitted lock file path collected per compiled workflow (
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Reconcile the actual emitted lock path instead of reconstructing one from the workflow IDThe compile path already knows each generated lock filename (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 85e8e16: |
||
| } | ||
| } | ||
|
|
||
| // Prune stale gh-aw-actions entries before saving | ||
| pruneStaleActionCacheEntries(compiler, actionCache) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package workflow | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| ) | ||
|
|
||
| // DisableDefaultActionFailureExpiryMarkersIfUnenforced disables implicit | ||
| // action-failure expiry markers when no maintenance workflow exists to enforce them. | ||
| // The generated maintenance workflow always includes the global expiry sweeper. | ||
| func DisableDefaultActionFailureExpiryMarkersIfUnenforced(workflowDataList []*WorkflowData, workflowDir string) { | ||
| if _, err := os.Stat(filepath.Join(workflowDir, "agentics-maintenance.yml")); os.IsNotExist(err) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. File existence is the wrong enforcement check. 💡 Base the decision on whether maintenance would actually emit the expiry jobsUse the same repo-config logic that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 85e8e16: |
||
| disableDefaultActionFailureExpiryMarkers(workflowDataList, workflowDir) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 85e8e16:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 85e8e16:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This helper still rewrites every positive 💡 Only disable the implicit default marker, never an explicit configured expiryThe helper needs provenance, not just the rendered lock text. Thread the repo config or an explicit/implicit flag through post-processing, then rewrite only lock files whose expiration came from the implicit 168-hour default. Add a regression that compiles with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 85e8e16: |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/cli/compile_pipeline.go:864: yagni: one-call helper for a tiny branch. Inline the file-existence check in runPostProcessing and call disableDefaultActionFailureExpiryMarkers directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 85e8e16 the branch grew beyond a tiny file-existence check (it now also verifies
close-expired-entitiesisn't disabled and that maintenance isn't explicitly off), so it's kept as a named helper (isActionFailureExpiryEnforced) rather than inlined.