Account for actions/cache branch scoping in cache-miss misconfiguration detection#39386
Account for actions/cache branch scoping in cache-miss misconfiguration detection#39386Copilot wants to merge 8 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…on detection Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces false-positive “cache_memory_miss misconfiguration” warnings by accounting for actions/cache branch scoping and first-run behavior, gating the warning on whether a cache restore actually matched an existing entry.
Changes:
- Adds an agent-job output (
cache_memory_restored) derived from restore step outputs (cache-hit/cache-matched-key) via a new aggregation step. - Threads
cache_memory_restoredinto failure handling (GH_AW_CACHE_MEMORY_RESTORED) and updateshandle_agent_failurelogic to only classifycache_memory_missas misconfiguration when restore matched. - Updates operator-facing messaging and extends JS unit tests for the new behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/cache.go | Assigns stable restore step IDs and emits a “restore availability” aggregation step producing cache_memory_restored. |
| pkg/workflow/compiler_main_job.go | Exposes cache_memory_restored as an agent job output for downstream jobs. |
| pkg/workflow/notify_comment.go | Passes GH_AW_CACHE_MEMORY_RESTORED into the failure handler environment. |
| actions/setup/js/handle_agent_failure.cjs | Gates cache-miss misconfiguration detection and cache warning rendering on cacheMemoryRestored. |
| actions/setup/js/handle_agent_failure.test.cjs | Updates tests for the new buildMissingDataContext signature and adds coverage for the “not restored” path. |
| actions/setup/md/cache_memory_miss.md | Clarifies that the warning only appears after a restore match and that first-run/branch-scoped misses are expected. |
| .github/workflows/design-decision-gate.lock.yml | Regenerated lock workflow metadata/hashes reflecting the updated compiled content. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
| @@ -1018,10 +1018,11 @@ function loadMissingDataMessages(items) { | |||
| * When cache-memory is enabled and a cache_miss is detected, appends a | |||
| @@ -432,6 +432,7 @@ func (c *Compiler) buildConclusionJob(data *WorkflowData, mainJobName string, sa | |||
| // indicates the prompt is referencing an incorrect file path within the cache directory. | |||
| @@ -2678,19 +2682,21 @@ async function main() { | |||
| // Check for items regardless of agentOutputResult.success so that cache-miss signals | |||
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39386 does not have the 'implementation' label (has_implementation_label=false) and has only 32 new lines in business logic directories, below the 100-line threshold (requires_adr_by_default_volume=false). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
@copilot do not add a new step, integrate in JavaScript that checks misconfigured cache |
|
@copilot safe outputs tool parameters already support synonyms/aliases. Reuse those instead of the anyOf introduced in the spec. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — the logic is correct, but three test coverage gaps in the Go code generation layer warrant attention.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap in Go code generation: The new
cache_memory_restore_statusaggregation step, thecache_memory_restoredjob output, and theGH_AW_CACHE_MEMORY_RESTOREDenv var all lack integration-test assertions. Each is a node in the propagation chain; a silent break at any node reverts to the pre-fix false-positive behavior with no test failure. - JS layer is well-tested: The new
buildMissingDataContextsignature, theshouldShowCacheWarningpredicate, and the regression test for the no-restore path are all solid. - Minor JS test gap:
buildMissingDataContext(true, true)(enabled + restored, no items) is missing from the "returns empty string" test suite.
Positive Highlights
- ✅ Clean tri-state predicate
shouldShowCacheWarning = cacheMemoryEnabled && cacheMemoryRestored && hasCacheMissreplaces an ambiguous two-condition check. - ✅ Index-based step IDs (
restore_cache_memory_N) prevent collisions from normalized cache-ID forms. - ✅ Belt-and-suspenders restore detection (
cache-matched-key || cache-hit) handles both partial and exact restores acrossactions/cacheversions. - ✅ The
else if (!cacheMemoryRestored)log branch is exactly the right operator signal for the suppressed-detection case. - ✅
cache_memory_miss.mdwording update is precise and actionable.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 544.7 AIC · ⌖ 13.9 AIC · ⊞ 27.9K
| builder.WriteString(" restored=true\n") | ||
| builder.WriteString(" fi\n") | ||
| } | ||
| builder.WriteString(" echo \"cache_memory_restored=${restored}\" >> \"$GITHUB_OUTPUT\"\n") |
There was a problem hiding this comment.
[/tdd] The new cache_memory_restore_status aggregation step (lines 607–624) has no test coverage in any Go test file — if the generated YAML regresses (wrong env var name, missing id:, bad shell logic), no test catches it.
💡 Suggested additions to `cache_memory_integration_test.go`
The existing expectedInLock string-assertion pattern in cache_memory_integration_test.go makes this easy. Adding these to the relevant test cases would prevent silent regressions:
// single-cache case
expectedInLock: []string{
"id: restore_cache_memory_0",
"- name: Detect cache-memory restore availability",
"id: cache_memory_restore_status",
"GH_AW_RESTORE_CACHE_MEMORY_0_MATCHED_KEY",
"cache_memory_restored=${restored}",
},// multi-cache case should also assert index 1:
expectedInLock: []string{
"id: restore_cache_memory_0",
"id: restore_cache_memory_1",
"GH_AW_RESTORE_CACHE_MEMORY_1_MATCHED_KEY",
},Without these, a rename of cache_memory_restore_status or a typo in the env var prefix would silently make cacheMemoryRestored always false in all live runs.
| // This lets downstream failure handling distinguish true prompt misconfiguration from | ||
| // expected cache misses (for example, branch-scoped misses on first runs). | ||
| if data.CacheMemoryConfig != nil && len(data.CacheMemoryConfig.Caches) > 0 { | ||
| outputs["cache_memory_restored"] = "${{ steps.cache_memory_restore_status.outputs.cache_memory_restored || 'false' }}" |
There was a problem hiding this comment.
[/diagnose] No test verifies that cache_memory_restored is emitted as a job output when cache-memory is configured. This output is the critical link in the data-flow chain: a stale step reference (e.g., if cache_memory_restore_status is renamed) makes the expression silently evaluate to false, suppressing all cache-miss detections without any test failure.
💡 Suggested test addition
The compiler_cache_test.go / cache_memory_integration_test.go integration-test pattern works here. For a workflow with tools: cache-memory: true, assert:
expectedInLock: []string{
"cache_memory_restored: ${{ steps.cache_memory_restore_status.outputs.cache_memory_restored",
},This ensures the step reference stays in sync with the generated step ID.
| // indicates the prompt is referencing an incorrect file path within the cache directory. | ||
| if data.CacheMemoryConfig != nil && len(data.CacheMemoryConfig.Caches) > 0 { | ||
| agentFailureEnvVars = append(agentFailureEnvVars, " GH_AW_CACHE_MEMORY_ENABLED: \"true\"\n") | ||
| agentFailureEnvVars = append(agentFailureEnvVars, fmt.Sprintf(" GH_AW_CACHE_MEMORY_RESTORED: ${{ needs.%s.outputs.cache_memory_restored || 'false' }}\n", mainJobName)) |
There was a problem hiding this comment.
[/diagnose] No test verifies that GH_AW_CACHE_MEMORY_RESTORED appears in the generated env block of the conclusion job failure-handler step. If this env var is missing or misspelled, the JS handler always sees cacheMemoryRestored=false — silently reverting to pre-fix behavior for all workflows.
💡 Suggested test
The compiler_shared_cache_test.go pattern (compiling a workflow and asserting strings in the lock YAML) works here:
expectedInLock: []string{
"GH_AW_CACHE_MEMORY_RESTORED: ${{ needs.",
".outputs.cache_memory_restored || 'false' }}",
},This directly tests the propagation chain: compiler_main_job.go → notify_comment.go → JS env var.
| @@ -2578,16 +2578,16 @@ describe("handle_agent_failure", () => { | |||
| }); | |||
|
|
|||
| it("returns empty string when agent output file does not exist", () => { | |||
There was a problem hiding this comment.
[/tdd] The "returns empty string" test covers (false, false) and (true, false) but omits (true, true) — cache enabled AND restored, but no missing-data items. Adding this case explicitly documents and protects the early-return path for the most operationally-relevant argument combination.
💡 One-line addition
expect(buildMissingDataContext(true, true)).toBe("");This is the case that occurs every normal run where cache-memory is healthy and the agent produced no missing-data signals.
| hasCacheMissMisconfiguration = true; | ||
| core.info(`Cache-miss misconfiguration detected: ${cacheMissItems.length} missing_data item(s) with reason "cache_memory_miss" despite cache-memory being available`); | ||
| core.info(`Cache-miss misconfiguration detected: ${cacheMissItems.length} missing_data item(s) with reason "cache_memory_miss" after cache restore matched an existing key`); | ||
| } |
There was a problem hiding this comment.
[/diagnose] ✅ Positive highlight: the new else if branch correctly logs why misconfiguration detection was skipped. This is exactly the right operator-facing signal — without it, a skipped detection would be invisible in the run logs.
One small follow-up worth considering: the log message references "actions/cache branch scoping and first-run behavior" — consistent with the PR description. Consider also logging the specific number of cache_memory_miss items (matching the style of the hasCacheMissMisconfiguration true-path log above) so operators can see the suppressed count if they need to audit:
�� Optional log improvement
const suppressedCount = agentOutputResult.items
? agentOutputResult.items.filter(i => i.type === "missing_data" && i.reason === "cache_memory_miss").length
: 0;
core.info(`Cache-memory is configured but no cache restore match was found; ${suppressedCount} cache_memory_miss item(s) treated as expected cache miss (actions/cache branch scoping and first-run behavior)`);
}
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (9 tests analyzed)
Go: 0 ( Verdict
|
There was a problem hiding this comment.
The PR correctly fixes false-positive misconfiguration signals on first-run and branch-scoped scenarios — the core logic is sound. Three non-blocking issues flagged in inline comments.
Review themes
if: always() missing on aggregation step (pkg/workflow/cache.go:608)
If generateCacheMemoryGitSetupStep fails for any cache, the cache_memory_restore_status step is skipped (no if: always() guard). The || 'false' fallback silently returns false, causing a legitimate post-restore misconfiguration signal to be swallowed. Adding if: always() to this single step is a one-liner fix.
Multi-cache OR-aggregation false positive (pkg/workflow/cache.go:617)
The aggregation uses OR across all caches, so restored=true whenever any cache matched. In multi-cache setups where Cache A restored but Cache B did not (expected for branch-scoped first runs), a miss on Cache B's files is still flagged as misconfiguration. Per-cache disambiguation or an explicit caveat in the operator message would address this.
Test coverage gap (handle_agent_failure.test.cjs:2582)
The two-arg conversion left the (true, true) variant untested for the no-file early-return path. Cheap one-liner addition.
Note: the sub-agent (grumpy-coder) did not complete within the review timeout; findings above are from an independent second pass only.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 527.4 AIC · ⌖ 13.7 AIC · ⊞ 17.3K
| } | ||
|
|
||
| builder.WriteString(" - name: Detect cache-memory restore availability\n") | ||
| builder.WriteString(" id: cache_memory_restore_status\n") |
There was a problem hiding this comment.
cache_memory_restore_status step missing if: always() — can silently drop the restored signal if any preceding step fails.
💡 Explanation and suggested fix
generateCacheMemoryGitSetupStep does not emit if: always(), so if the git-setup step for any cache fails (corrupted object store, script error, etc.), every subsequent step without its own if: always() — including this aggregation step — is skipped. The || 'false' fallback in the job output expression then returns false, which means a run where a cache was successfully restored gets misclassified as "expected miss", suppressing a real misconfiguration signal silently.
Fix: add if: always() so the aggregation step always runs regardless of upstream failures:
builder.WriteString(" - name: Detect cache-memory restore availability\n")
builder.WriteString(" id: cache_memory_restore_status\n")
builder.WriteString(" if: always()\n") // add thisThe restore steps themselves always succeed (cache miss returns exit 0), so the env-var expressions for cache-matched-key / cache-hit will always have valid values when the restore step actually ran.
| } | ||
| builder.WriteString(" shell: bash\n") | ||
| builder.WriteString(" run: |\n") | ||
| builder.WriteString(" restored=false\n") |
There was a problem hiding this comment.
OR-aggregation across caches creates false-positive misconfiguration warnings in multi-cache setups.
💡 Explanation
The loop sets restored=true if any cache matched — but the cache_memory_miss signal does not identify which cache the agent missed. Consider a two-cache workflow:
- Cache A (main branch, long-lived): restored ✓
- Cache B (feature branch, first run): not restored (expected — branch scoping)
- Agent reports
cache_memory_missfor Cache B's files
Because restored=true from Cache A, this gets classified as a misconfiguration and the operator sees a warning. The miss is actually expected.
The simplest correct fix is to also track per-cache restore state and only flag a miss as misconfiguration when the specific cache that the agent was configured to use was restored. Alternatively, document that the flag is coarse and that multi-cache setups with mixed restore outcomes may see false positives.
If per-cache disambiguation is out of scope for this PR, at minimum the cache_memory_miss.md message should note that the warning can be a false positive when multiple caches are configured and not all restored.
| expect(buildMissingDataContext(false)).toBe(""); | ||
| expect(buildMissingDataContext(true)).toBe(""); | ||
| expect(buildMissingDataContext(false, false)).toBe(""); | ||
| expect(buildMissingDataContext(true, false)).toBe(""); |
There was a problem hiding this comment.
Test now only covers (true, false) for the "no output file" case — (true, true) is untested.
💡 Details
The test was updated from buildMissingDataContext(true) to buildMissingDataContext(true, false), but the corresponding (true, true) variant (cache enabled AND restored, file absent) was not added. The early-return on empty missingDataMessages is not exercised when cacheMemoryRestored=true, so a regression that conditionally skips the early return in that branch would go undetected.
Suggested addition:
expect(buildMissingDataContext(true, true)).toBe("");This is low-risk but cheap to add alongside the existing cases.
| // signal in the generic "Missing Data" section. Keep the specialised cache warning below. | ||
| const displayableMissingData = cacheMemoryEnabled && hasCacheMiss ? missingDataMessages.filter(m => m.reason !== "cache_memory_miss") : missingDataMessages; | ||
| const shouldShowCacheWarning = cacheMemoryEnabled && cacheMemoryRestored && hasCacheMiss; | ||
| const displayableMissingData = shouldShowCacheWarning ? missingDataMessages.filter(m => m.reason !== "cache_memory_miss") : missingDataMessages; |
There was a problem hiding this comment.
cache_memory_miss items now flow into the generic missing-data section when cache was not restored — can trigger missingDataReportAsFailure failure verdicts on expected first-run misses.
💡 Explanation
In the old code, cache_memory_miss was always stripped from displayableMissingData whenever cacheMemoryEnabled && hasCacheMiss, regardless of restore status. The specialist template was then unconditionally shown.
In the new code, when cacheMemoryRestored=false, shouldShowCacheWarning is false, so displayableMissingData includes the cache_memory_miss item unfiltered. It flows into the "Missing Data Reported" block at line 1046 and is visible to the downstream missingDataReportAsFailure guard.
This means that if an operator has missingDataReportAsFailure=true, a completely normal first-run or branch-scoped cache miss (expected behavior) now causes a failure-level verdict in handle_agent_failure, which would have been a no-op before this PR.
The PR description notes this is intentional ("kept in generic missing-data context"), but the downstream failure guard was not updated to exclude cache_memory_miss from missingDataReportAsFailure evaluation when !cacheMemoryRestored. A targeted fix:
// Filter out expected cache misses from failure accounting when cache was not restored
const failureAccountableMissingData = (cacheMemoryEnabled && !cacheMemoryRestored)
? missingDataMessages.filter(m => m.reason !== "cache_memory_miss")
: missingDataMessages;
// use failureAccountableMissingData for hasMissingData / failure-level guards
// use displayableMissingData (current) for the formatted context stringCo-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 5812765. I removed the added |
I've reviewed the PR changes but cannot locate where Could you clarify:
This will help me implement the change correctly. |
|
@copilot review all comments and address the unresolved cache restore review feedback.
|
|
@copilot refresh the branch and rerun checks after addressing the review feedback.
|
|
@copilot review all comments and address unresolved review feedback.
|
|
pr-sous-chef: please refresh the branch and rerun checks after the review feedback is addressed.
|
🤖 PR Triage Report
Score Breakdown
|
|
@copilot review all comments and address unresolved review feedback. Then refresh the branch and summarize any remaining blockers.
|
|
@copilot review all comments and address the unresolved review feedback.
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR is ready for merge pending CI re-trigger. All local validation passed:
Workflows recompiled after merge with main in dae0142. |
cache_memory_misswas previously treated as a prompt misconfiguration whenever cache-memory was enabled, which produced false positives on first-run and cross-branch scenarios due toactions/cachebranch scoping. This change gates misconfiguration reporting on actual restore success so only actionable cache-path errors are flagged.Restore-aware misconfiguration signal
cache-matched-key/cache-hit) as agent job outputs.handle_agent_failurevia indexed env vars:GH_AW_CACHE_MEMORY_RESTORE_<index>_MATCHED_KEYGH_AW_CACHE_MEMORY_RESTORE_<index>_CACHE_HITcache_memory_restored=true|falsein JavaScript by checking those restore outputs.Failure handler semantics update
cache_memory_missclassification:enabled && restored && cache_memory_miss→ misconfiguration warning pathenabled && !restored && cache_memory_miss→ expected miss path (kept in generic missing-data context)Clearer operator-facing message
cache_memory_miss.mdwording to explicitly describe:Implementation hardening
pr-sous-chef: updated the branch from run https://github.com/github/gh-aw/actions/runs/27562390161
pr-sous-chef: updated branch and will follow up on review feedback. Run: https://github.com/github/gh-aw/actions/runs/27568109303
pr-sous-chef: refreshed branch for run https://github.com/github/gh-aw/actions/runs/27604242308