fix: gate push_repo_memory on agent not skipped instead of success#25960
Merged
fix: gate push_repo_memory on agent not skipped instead of success#25960
Conversation
Change the push_repo_memory job condition from checking needs.agent.result == 'success' to needs.agent.result != 'skipped'. This prevents push_repo_memory from running on no-op workflow invocations (e.g. bot comments where pre_activation is skipped and the skip cascades through activation → agent → detection). The != 'skipped' check also allows repo-memory to be pushed when the agent fails, preserving partial memory data. Fixes #1556 Agent-Logs-Url: https://github.com/github/gh-aw/sessions/053233b6-7afc-4caf-bf6e-74bbd9f26562 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix push_repo_memory job activation logic
fix: gate push_repo_memory on agent not skipped instead of success
Apr 13, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the generated GitHub Actions workflow logic so push_repo_memory does not run when the agent job was skipped (e.g., no-op bot-triggered runs), while still allowing repo-memory pushes when the agent ran but failed.
Changes:
- Update
push_repo_memoryjob condition to gate onneeds.agent.result != 'skipped'(instead of== 'success'). - Add a unit test ensuring the job condition uses the new gating logic with and without threat detection.
- Regenerate compiled
.lock.ymlworkflows to reflect the updated condition.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/repo_memory.go | Adjusts push_repo_memory job condition generation to gate on agent not being skipped. |
| pkg/workflow/repo_memory_test.go | Adds test coverage for the updated job condition logic. |
| .github/workflows/workflow-health-manager.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/weekly-blog-post-writer.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/technical-doc-writer.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/security-compliance.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/pr-triage-agent.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/metrics-collector.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/glossary-maintainer.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/firewall-escape.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/discussion-task-miner.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/developer-docs-consolidator.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/delight.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/deep-report.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/daily-testify-uber-super-expert.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/daily-news.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/daily-community-attribution.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/daily-code-metrics.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/daily-cli-performance.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/copilot-token-optimizer.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/copilot-token-audit.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/copilot-session-insights.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/copilot-pr-prompt-analysis.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/copilot-cli-deep-research.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/copilot-agent-analysis.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/code-scanning-fixer.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/audit-workflows.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
| .github/workflows/agent-performance-analyzer.lock.yml | Regenerated lock workflow reflecting updated push_repo_memory condition. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 29/29 changed files
- Comments generated: 3
pkg/workflow/repo_memory.go
Outdated
Comment on lines
747
to
759
| 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)) | ||
| } |
There was a problem hiding this comment.
The new job condition gates on needs.agent.result != 'skipped', which will also evaluate true when the agent job is cancelled. Previously (when checking == 'success') this job would not run on cancellations; with always() it can now attempt to push repo memory for cancelled runs, risking spurious commits/runner usage. Consider adding !cancelled() to the job-level condition (similar to pkg/workflow/compiler_safe_outputs_job.go:451-457) so push_repo_memory does not run after workflow cancellation.
See below for a potential fix:
// Also require !cancelled() so the job does not run after workflow cancellation.
agentRan := BuildNotEquals(
BuildPropertyAccess(fmt.Sprintf("needs.%s.result", constants.AgentJobName)),
BuildStringLiteral("skipped"),
)
notCancelled := BuildNot(BuildFunctionCall("cancelled"))
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(BuildAnd(BuildFunctionCall("always"), notCancelled), buildDetectionPassedCondition()), agentRan))
jobNeeds = append(jobNeeds, string(constants.DetectionJobName))
} else {
jobCondition = RenderCondition(BuildAnd(BuildAnd(BuildFunctionCall("always"), notCancelled), agentRan))
pkg/workflow/repo_memory_test.go
Outdated
Comment on lines
+1304
to
+1324
| 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.
The new test asserts on substrings only; if you add a cancellation guard (e.g. !cancelled()), it would be good for this test to assert it explicitly so cancellations don’t regress. Also consider asserting the full expected pushJob.If for both branches to make the condition semantics unambiguous.
pkg/workflow/repo_memory.go
Outdated
Comment on lines
739
to
750
| // 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"), | ||
| ) |
There was a problem hiding this comment.
Minor consistency: this variable represents needs.agent.result != 'skipped' (matching the common name agentNotSkipped used elsewhere, e.g. pkg/workflow/threat_detection.go:746 and pkg/workflow/compiler_safe_outputs_job.go:451). Renaming agentRan to agentNotSkipped would align terminology and reduce ambiguity (especially if you later add !cancelled() and the job is no longer strictly “agent ran”).
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification Details
Test Analysis✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The added test TestPushRepoMemoryJobConditionGatesOnAgentNotSkipped is a strong behavioral contract test that directly pins the fix against regression.
Collaborator
|
@copilot review all comments |
Address review feedback: 1. Add !cancelled() to prevent push_repo_memory from running after workflow cancellation (consistent with compiler_safe_outputs_job.go) 2. Rename agentRan to agentNotSkipped for consistency with the naming convention used elsewhere in the codebase 3. Update test to assert full expected condition string and verify !cancelled() presence Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b01603df-bac9-4013-8fe3-6f9d9e8a2d41 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Addressed all three review comments in
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
push_repo_memoryruns on every bot-triggered no-op workflow invocation becausealways()bypasses skip propagation and the condition only checkedneeds.agent.result == 'success'. Whenpre_activationskips (no slash command found), the entire job graph cascades to skipped—butpush_repo_memorystill fires, wasting runner time and risking spurious commits to the memory branch.Change
pkg/workflow/repo_memory.go: Replaceneeds.agent.result == 'success'withneeds.agent.result != 'skipped'and add a!cancelled()guard to prevent the job from running after workflow cancellation (consistent withcompiler_safe_outputs_job.go).This correctly distinguishes "agent never ran" (skipped → don't push) from "agent ran but failed" (failure → still push partial memory data), while also preventing spurious runs after workflow cancellation. The detection gate for threat detection workflows is preserved unchanged.
pkg/workflow/repo_memory_test.go: AddedTestPushRepoMemoryJobConditionGatesOnAgentNotSkippedcovering both threat-detection-enabled and disabled paths, asserting the full expected condition string including!cancelled()..lock.ymlfiles: Recompiled.