fix: push_repo_memory should not run when agent job is skipped or failed#24363
fix: push_repo_memory should not run when agent job is skipped or failed#24363
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/111dcd6f-85f7-4979-a14a-c0a71ff548da Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the generated GitHub Actions workflow logic so push_repo_memory only runs when the agent job succeeds, preventing wasted runner usage and avoiding writing stale/incomplete repo memory.
Changes:
- Tightened
push_repo_memoryjob-levelif:to additionally requireneeds.agent.result == 'success'(and still gate on detection pass/skip when threat detection is enabled). - Regenerated compiled workflow lock files to reflect the updated condition across workflows.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/repo_memory.go | Updates push_repo_memory job condition to require agent success (and detection pass/skip when enabled). |
| .github/workflows/workflow-health-manager.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/weekly-blog-post-writer.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/technical-doc-writer.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/security-compliance.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/pr-triage-agent.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/metrics-collector.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/glossary-maintainer.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/firewall-escape.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/discussion-task-miner.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/developer-docs-consolidator.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/delight.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/deep-report.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/daily-testify-uber-super-expert.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/daily-news.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/daily-copilot-token-report.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/daily-community-attribution.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/daily-code-metrics.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/daily-cli-performance.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/copilot-session-insights.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/copilot-pr-prompt-analysis.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/copilot-cli-deep-research.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/copilot-agent-analysis.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/code-scanning-fixer.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/audit-workflows.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
| .github/workflows/agent-performance-analyzer.lock.yml | Regenerated lock to include needs.agent.result == 'success' in push_repo_memory condition. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 27/27 changed files
- Comments generated: 2
| // 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). |
There was a problem hiding this comment.
The function-level doc comment for buildPushRepoMemoryJob earlier in this file still states the job runs even if the agent job fails, but the updated jobCondition now requires needs.agent.result == 'success'. Please update that doc comment to match the new behavior so future readers aren’t misled.
| // 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). | |
| // failed or was skipped). Using always() so the condition is still evaluated consistently | |
| // when other upstream jobs are skipped (e.g. detection is skipped when agent produces no | |
| // outputs), but the job still requires the agent job result to be "success". |
| agentSucceeded := BuildEquals( | ||
| BuildPropertyAccess(fmt.Sprintf("needs.%s.result", constants.AgentJobName)), | ||
| BuildStringLiteral("success"), | ||
| ) | ||
| jobNeeds := []string{string(constants.AgentJobName)} | ||
| var jobCondition string | ||
| if threatDetectionEnabled { | ||
| // When threat detection is enabled, run only if detection succeeded (no threats found) | ||
| // or was skipped (agent produced no outputs or patch — nothing to detect against). | ||
| jobCondition = RenderCondition(BuildAnd(BuildFunctionCall("always"), buildDetectionPassedCondition())) | ||
| // When threat detection is enabled, also require detection passed (succeeded or skipped). | ||
| jobCondition = RenderCondition(BuildAnd(BuildAnd(BuildFunctionCall("always"), buildDetectionPassedCondition()), agentSucceeded)) | ||
| jobNeeds = append(jobNeeds, string(constants.DetectionJobName)) | ||
| } else { | ||
| jobCondition = RenderCondition(BuildAnd(BuildFunctionCall("always"), agentSucceeded)) | ||
| } |
There was a problem hiding this comment.
This change updates the generated job-level if: for push_repo_memory, but there doesn’t appear to be a test asserting the new condition (agent success required, plus detection passed/skipped when enabled). Adding a unit/integration test that validates pushRepoMemoryJob.If contains the expected clauses would help prevent regressions (similar to the existing update_cache_memory condition coverage).
push_repo_memorywas unconditionally guarded byalways(), so it ran even when theagentjob failed or was skipped — pointlessly consuming a runner and risking writing stale/incomplete memory.Changes
pkg/workflow/repo_memory.go: Addedneeds.agent.result == 'success'to thepush_repo_memoryjob condition, matching the identical guard already present onupdate_cache_memoryBefore / After (with threat detection):
Without threat detection: