fix: add actions: write to update_cache_memory and conclusion jobs to unblock cache saves#47046
Conversation
…r cache saves
Cache saves (actions/cache/save) require a token with at least one writable scope.
The update_cache_memory job had permissions: {} (empty), and the conclusion job
relied only on safe-outputs permissions (issues/pull-requests: write), neither of
which satisfies GitHub's cache-reservation backend requirement.
Both jobs now emit actions: write:
- buildUpdateCacheMemoryJob (pkg/workflow/cache.go): replaces NewPermissionsEmpty()
with NewPermissionsActionsWrite(), additionally setting contents: read in dev mode.
- buildConclusionJob (pkg/workflow/notify_comment.go): sets PermissionActions: write
when hasMaxDailyAICGuardrail && WorkflowID != "" (i.e. when the daily-AIC cache
save steps are included).
Recompiled all 260 lock files to reflect the new permissions.
Fixes: cache write denied: token has no writable scopes
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
actions: write to update_cache_memory and conclusion jobs to unblock cache saves
|
@copilot validate this fix against GitHub actions docs |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #47046 only modifies .lock.yml workflow files to add actions: write permissions. |
There was a problem hiding this comment.
Pull request overview
Adds the required actions: write permission to compiler-generated cache-save jobs, fixing silent cache persistence failures.
Changes:
- Grants cache-write permissions to
update_cache_memoryand applicableconclusionjobs. - Adds unit and integration coverage.
- Regenerates affected workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/cache.go |
Grants cache-update jobs actions: write. |
pkg/workflow/notify_comment.go |
Grants daily-AIC cache saves actions: write. |
pkg/workflow/compiler_jobs_test.go |
Tests cache-memory permissions. |
pkg/workflow/notify_comment_test.go |
Tests conclusion-job permissions. |
pkg/workflow/cache_memory_threat_detection_test.go |
Adds compiled-workflow assertion. |
.github/workflows/*.lock.yml (260 files) |
Regenerates affected conclusion and cache-update permissions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 264/264 changed files
- Comments generated: 2
- Review effort level: Medium
| "- name: Save cache-memory to cache (default)", | ||
| "uses: actions/cache/save@", | ||
| // update_cache_memory job must have actions: write for cache reservation | ||
| "actions: write", |
| t.Run("no actions: write when WorkflowID is empty", func(t *testing.T) { | ||
| workflowData := &WorkflowData{ | ||
| Name: "Test Workflow", | ||
| WorkflowID: "", // no WorkflowID → no daily-AIC cache steps |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-scoped.
cache.go:update_cache_memorynow starts withNewPermissionsActionsWrite()(alwaysactions: write) and conditionally addscontents: readin dev mode — previously it started with empty permissions, breaking cache saves.notify_comment.go:conclusionaddsactions: writeonly whenhasMaxDailyAICGuardrail && WorkflowID != "", precisely targeting the daily-AIC cache save path.
All referenced permission helpers exist and are correctly used. The 260 recompiled lock files reflect the change consistently.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 23.8 AIC · ⌖ 5.23 AIC · ⊞ 4.9K
Validated against the GitHub Actions workflow syntax docs and the June 2026 changelog on read-only cache for untrusted triggers. The fix is correct. Here's why: What the docs sayThe
The cache service is part of the Actions platform, so Why
|
…save jobs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (110 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one minor test coverage gap noted; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Root cause properly fixed:
NewPermissionsEmpty()→NewPermissionsActionsWrite()correctly addresses the cause, not just the symptom. The dev-mode branch now usesperms.Set(...)instead of reassigning the whole block, so both permissions coexist. - Conclusion job fix is precisely guarded:
hasMaxDailyAICGuardrail(data) && data.WorkflowID != ""matches exactly the condition under whichbuildDailyAICUsageCacheStepsinjects theactions/cache/savestep — tight and correct. - Tests land with the fix:
TestUpdateCacheMemoryJobHasActionsWritePermission,TestConclusionJobActionsWritePermissionForDailyAICCache, and the integration assertion incache_memory_threat_detection_test.goall ship in this PR.
Test Coverage Gap
- The dev-mode branch (
setupActionRef != "") is not exercised by the new tests. This is the exact branch the original bug inhabited (permissions were fully replaced, droppingactions: write). A dedicated test that asserts bothactions: writeandcontents: readappear whensetupActionRef != ""would lock in the regression.
Positive Highlights
- ✅ Clean, minimal diff in the Go source — no unrelated changes.
- ✅ Comment explaining why
actions: writeis needed is clear and actionable for future readers. - ✅ 264 lock files regenerated consistently — confirms
make recompilewas run. - ✅
COMMENTevent rather than requesting changes: the gap is low risk since the default path (no dev mode) is covered, and the existing integration test incache_memory_threat_detection_test.goexercises the full compiled output.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 52.1 AIC · ⌖ 5.31 AIC · ⊞ 6.6K
Comment /matt to run again
| t.Fatal("Expected update_cache_memory job to be created") | ||
| } | ||
|
|
||
| // Must have actions: write so cache saves are not rejected with "token has no writable scopes". |
There was a problem hiding this comment.
[/tdd] The dev mode branch (setupActionRef != "") is not covered — this is the exact code path where the original bug lived (the old code replaced the entire permissions block, losing actions: write). No regression test verifies that dev mode still emits both actions: write and contents: read.
💡 Suggested addition
Add a test (or sub-test) that exercises the dev-mode path and asserts both permissions are present:
func TestUpdateCacheMemoryJobHasActionsWritePermissionInDevMode(t *testing.T) {
// ... set up compiler with setupActionRef != "" so dev-mode branch executes ...
if !strings.Contains(job.Permissions, "actions: write") {
t.Errorf("dev mode must preserve actions: write, got: %q", job.Permissions)
}
if !strings.Contains(job.Permissions, "contents: read") {
t.Errorf("dev mode must have contents: read, got: %q", job.Permissions)
}
}@copilot please address this.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Fix is correct; two non-blocking maintainability issues flagged
The core change is sound: replacing NewPermissionsEmpty() with NewPermissionsActionsWrite() in buildUpdateCacheMemoryJob correctly unblocks cache saves, and the dev-mode contents-read path is preserved via perms.Set(...) on the same object (fixing a latent bug where the old code created a new perms object, discarding any prior settings). The conclusion job permission guard mirrors the step-injection condition precisely.
🔍 Findings (non-blocking)
1. Duplicated guard condition — drift risk (notify_comment.go:86)
The hasMaxDailyAICGuardrail(data) && data.WorkflowID != "" expression is copy-pasted in both buildConclusionJob (permissions) and buildConclusionSetupSteps (step injection). If either site drifts, the regression is invisible because cache steps run with continue-on-error: true. A shared needsDailyAICCachePermission(data) predicate would prevent this. See inline comment.
2. Weak integration-test assertion (cache_memory_threat_detection_test.go:65)
The "actions: write" string is matched against the entire compiled lock file, not scoped to the update_cache_memory: block. The dedicated unit test in compiler_jobs_test.go is the real guard; the integration-test assertion provides false confidence. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 113.4 AIC · ⌖ 4.77 AIC · ⊞ 5.6K
Comment /review to run again
| } | ||
| // actions: write is required for GitHub's cache-reservation backend to allow cache saves. | ||
| // The daily-AIC usage cache save step uses actions/cache/save which requires this permission. | ||
| if hasMaxDailyAICGuardrail(data) && data.WorkflowID != "" { |
There was a problem hiding this comment.
Permission and step-inclusion guards are duplicated across two files — future drift will silently break cache saves.
💡 Details and suggested fix
The actions: write permission is gated on hasMaxDailyAICGuardrail(data) && data.WorkflowID != "" here (line 86), and the identical expression appears in notify_comment_conclusion_helpers.go:50 to guard step injection. They are in sync today, but as two independent copy-pasted guards there is no enforcement.
If the helper ever relaxes or changes its guard, the permission block here will silently not be set, producing the original cache write denied: token has no writable scopes regression — invisible because cache steps run with continue-on-error: true.
Extract a single named predicate:
// needsDailyAICCachePermission reports whether the conclusion job
// must have actions: write for daily-AIC cache saves.
func needsDailyAICCachePermission(data *WorkflowData) bool {
return hasMaxDailyAICGuardrail(data) && data.WorkflowID != ""
}Both buildConclusionJob and buildConclusionSetupSteps call needsDailyAICCachePermission(data), making them impossible to drift apart.
| "- name: Save cache-memory to cache (default)", | ||
| "uses: actions/cache/save@", | ||
| // update_cache_memory job must have actions: write for cache reservation | ||
| "actions: write", |
There was a problem hiding this comment.
Assertion "actions: write" is a whole-file substring search — it passes even if update_cache_memory specifically is missing the permission.
💡 Details and suggested fix
The integration test checks expectedInLock by scanning the entire compiled lock file for the string "actions: write". If any other job in the output (e.g. conclusion) carries actions: write, this assertion passes even if update_cache_memory silently lost the permission.
The dedicated unit test TestUpdateCacheMemoryJobHasActionsWritePermission in compiler_jobs_test.go does inspect updateCacheMemoryJob.Permissions directly and is the stronger regression guard. But this integration-test assertion provides false confidence — consider scoping it to the update_cache_memory: block, or removing it here and relying solely on the unit test.
Example scoped check:
// Find the update_cache_memory job block and assert actions: write within it
idx := strings.Index(lock, "update_cache_memory:")
if idx == -1 {
t.Fatal("update_cache_memory job not found in lock")
}
if !strings.Contains(lock[idx:idx+500], "actions: write") {
t.Error("update_cache_memory job missing actions: write")
}
|
@copilot run pr-finisher skill |
|
@copilot update compiler to only add writeable scope when needed (lazy) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot recompile |
Ran |
| needs.activation.outputs.secret_verification_result == 'failed' || needs.activation.outputs.daily_ai_credits_exceeded == 'true') | ||
| runs-on: ubuntu-slim | ||
| permissions: | ||
| actions: write |
There was a problem hiding this comment.
@copilot there is already issues:write in this scope, why is actions:write needed?
There was a problem hiding this comment.
Addressed in 269b95a. issues: write already makes the GITHUB_TOKEN writable for the daily-AIC cache-save path, so actions: write was redundant in that scope. The compiler now adds actions: write only when the conclusion job would otherwise be fully read-only.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage
Rationale: Fixes silent cache-save failures (
|
|
Great work on this fix! 🚀 The cache persistence issue you are addressing is critical — workflows silently fail to persist cache lineage while staying green, which breaks downstream cache-memory and daily-AIC tracking. What is solid here:
Ready for merge — this is high-quality agentic output that follows the project development process. The
|
|
@copilot run pr-finisher skill |
GitHub's cache-reservation backend requires a token with at least one writable scope. Both
update_cache_memoryandconclusion(daily-AIC) cache-save jobs lacked this, causing silent failures:cache write denied: token has no writable scopes. Because cache steps arecontinue-on-error: true, runs stayed green whilecache-memoryand daily-AIC lineage were never persisted.Changes
pkg/workflow/cache.go—buildUpdateCacheMemoryJobNewPermissionsEmpty()(renderspermissions: {}) withNewPermissionsActionsWrite()contents: readpkg/workflow/notify_comment.go—buildConclusionJobactions: writetoconclusionPermswhenhasMaxDailyAICGuardrail && WorkflowID != ""— i.e. exactly whenbuildDailyAICUsageCacheStepsinjects theactions/cache/savestepTests
TestUpdateCacheMemoryJobHasActionsWritePermission— unit test on the job structTestConclusionJobActionsWritePermissionForDailyAICCache— table test covering both with/withoutWorkflowIDcache_memory_threat_detection_test.goLock files
All 260 compiled lock files regenerated; each affected
update_cache_memoryandconclusionjob now carriesactions: write.