-
Notifications
You must be signed in to change notification settings - Fork 351
fix: gate push_repo_memory on agent not skipped instead of success #25960
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 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -736,21 +736,26 @@ func (c *Compiler) buildPushRepoMemoryJob(data *WorkflowData, threatDetectionEna | |
| steps = append(steps, c.generateRestoreActionsSetupStep()) | ||
| } | ||
|
|
||
| // Job condition: only run if the agent job succeeded (do not push repo memory when agent | ||
| // failed or was skipped). Using always() so the job still runs even when upstream jobs | ||
| // are skipped (e.g. detection is skipped when agent produces no outputs). | ||
| agentSucceeded := BuildEquals( | ||
| // Job condition: only run when the agent actually executed (not skipped). | ||
| // Using always() so the job still runs even when upstream jobs are skipped | ||
| // (e.g. detection is skipped when agent produces no outputs). | ||
| // We check != 'skipped' rather than == 'success' so that repo-memory is | ||
| // pushed even when the agent fails — partial memory data is still valuable. | ||
| // Crucially, this prevents the job from running on no-op workflow invocations | ||
| // (e.g. bot comments) where pre_activation is skipped and the skip cascades | ||
| // through activation → agent → detection. | ||
| agentRan := BuildNotEquals( | ||
| BuildPropertyAccess(fmt.Sprintf("needs.%s.result", constants.AgentJobName)), | ||
| BuildStringLiteral("success"), | ||
| BuildStringLiteral("skipped"), | ||
| ) | ||
| jobNeeds := []string{string(constants.AgentJobName), string(constants.ActivationJobName)} | ||
| var jobCondition string | ||
| if threatDetectionEnabled { | ||
| // When threat detection is enabled, also require detection passed (succeeded or skipped). | ||
| jobCondition = RenderCondition(BuildAnd(BuildAnd(BuildFunctionCall("always"), buildDetectionPassedCondition()), agentSucceeded)) | ||
| jobCondition = RenderCondition(BuildAnd(BuildAnd(BuildFunctionCall("always"), buildDetectionPassedCondition()), agentRan)) | ||
| jobNeeds = append(jobNeeds, string(constants.DetectionJobName)) | ||
| } else { | ||
| jobCondition = RenderCondition(BuildAnd(BuildFunctionCall("always"), agentSucceeded)) | ||
| jobCondition = RenderCondition(BuildAnd(BuildFunctionCall("always"), agentRan)) | ||
| } | ||
|
||
|
|
||
| // Build outputs map for validation failures from all memory steps | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1281,3 +1281,46 @@ func TestPushRepoMemoryJobConcurrencyKey(t *testing.T) { | |
| assert.NotContains(t, pushJob.Concurrency, "push-repo-memory-${{ github.repository }}\"", | ||
| "Concurrency key should not be the old repo-wide-only key format") | ||
| } | ||
|
|
||
| // TestPushRepoMemoryJobConditionGatesOnAgentNotSkipped verifies that the push_repo_memory | ||
| // job condition uses needs.agent.result != 'skipped' so the job does not run on no-op | ||
| // workflow invocations (e.g. bot comments where pre_activation is skipped). | ||
| func TestPushRepoMemoryJobConditionGatesOnAgentNotSkipped(t *testing.T) { | ||
| data := &WorkflowData{ | ||
| RepoMemoryConfig: &RepoMemoryConfig{ | ||
| Memories: []RepoMemoryEntry{ | ||
| {ID: "default", BranchName: "memory/my-workflow"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| compiler := NewCompiler() | ||
|
|
||
| t.Run("without threat detection", func(t *testing.T) { | ||
| pushJob, err := compiler.buildPushRepoMemoryJob(data, false) | ||
| require.NoError(t, err, "Should build push job without error") | ||
| require.NotNil(t, pushJob, "Should produce a push job") | ||
|
|
||
| assert.Contains(t, pushJob.If, "always()", | ||
| "Condition should contain always() to bypass normal skip propagation") | ||
| assert.Contains(t, pushJob.If, "!= 'skipped'", | ||
| "Condition should check agent result != 'skipped' to gate on agent having run") | ||
| assert.NotContains(t, pushJob.If, "== 'success'", | ||
| "Condition should NOT use == 'success' — agent failures should still push memory") | ||
| }) | ||
|
|
||
| t.Run("with threat detection", func(t *testing.T) { | ||
| pushJob, err := compiler.buildPushRepoMemoryJob(data, true) | ||
| require.NoError(t, err, "Should build push job without error") | ||
| require.NotNil(t, pushJob, "Should produce a push job") | ||
|
|
||
| assert.Contains(t, pushJob.If, "always()", | ||
| "Condition should contain always()") | ||
| assert.Contains(t, pushJob.If, "!= 'skipped'", | ||
| "Condition should check agent result != 'skipped'") | ||
| assert.Contains(t, pushJob.If, "needs.detection.result", | ||
| "Condition should still check detection result when threat detection is enabled") | ||
| assert.NotContains(t, pushJob.If, "needs.agent.result == 'success'", | ||
| "Condition should NOT use == 'success' for agent check") | ||
|
||
| }) | ||
| } | ||
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.
Minor consistency: this variable represents
needs.agent.result != 'skipped'(matching the common nameagentNotSkippedused elsewhere, e.g.pkg/workflow/threat_detection.go:746andpkg/workflow/compiler_safe_outputs_job.go:451). RenamingagentRantoagentNotSkippedwould align terminology and reduce ambiguity (especially if you later add!cancelled()and the job is no longer strictly “agent ran”).