-
Notifications
You must be signed in to change notification settings - Fork 424
Encode branch-aware cache-miss semantics for daily AIC fallback #39266
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 3 commits
a4d6d04
a09e2c1
56612f7
89027a8
2772076
a6859b8
379fd10
33914b9
f88cb48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -345,6 +345,22 @@ func (c *Compiler) buildActivationDailyAICGuardrailStep(data *WorkflowData) []st | |
| steps = append(steps, fmt.Sprintf(" key: %s${{ github.run_id }}\n", cacheKeyPrefix)) | ||
| steps = append(steps, fmt.Sprintf(" restore-keys: %s\n", cacheKeyPrefix)) | ||
| steps = append(steps, " path: /tmp/gh-aw/agentic-workflow-usage-cache.jsonl\n") | ||
| // Detect true cache misses while accounting for branch-scoped action caches. | ||
| // The primary key includes run_id, so exact hits are not expected across runs. | ||
| // We treat a restore as a hit when cache-matched-key is present, and as a miss | ||
| // when no matched key is available. | ||
| steps = append(steps, " - name: Detect daily AIC usage cache miss\n") | ||
| steps = append(steps, " id: detect-daily-aic-cache-miss\n") | ||
| steps = append(steps, fmt.Sprintf(" if: %s\n", maxDailyAICreditsConfiguredIfExpr)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing 💡 Failure mechanism and fixThe if: ${{ env.GH_AW_MAX_DAILY_AI_CREDITS != '' }}This contains no The shell script is simple enough that failures are unlikely on GitHub-hosted runners, but Fix — add steps = append(steps, " id: detect-daily-aic-cache-miss\n")
steps = append(steps, " continue-on-error: true\n") // add this
steps = append(steps, fmt.Sprintf(" if: %s\n", maxDailyAICreditsConfiguredIfExpr)) |
||
| steps = append(steps, " env:\n") | ||
| steps = append(steps, " GH_AW_RESTORE_DAILY_AIC_CACHE_HIT: ${{ steps.restore-daily-aic-cache.outputs.cache-hit }}\n") | ||
| steps = append(steps, " GH_AW_RESTORE_DAILY_AIC_CACHE_MATCHED_KEY: ${{ steps.restore-daily-aic-cache.outputs.cache-matched-key }}\n") | ||
| steps = append(steps, " run: |\n") | ||
| steps = append(steps, " if [ -z \"$GH_AW_RESTORE_DAILY_AIC_CACHE_HIT\" ] || { [ \"$GH_AW_RESTORE_DAILY_AIC_CACHE_HIT\" = \"false\" ] && [ -z \"$GH_AW_RESTORE_DAILY_AIC_CACHE_MATCHED_KEY\" ]; }; then\n") | ||
| steps = append(steps, " echo \"cache_miss=true\" >> \"$GITHUB_OUTPUT\"\n") | ||
| steps = append(steps, " else\n") | ||
| steps = append(steps, " echo \"cache_miss=false\" >> \"$GITHUB_OUTPUT\"\n") | ||
| steps = append(steps, " fi\n") | ||
| // Artifact-based fallback for cross-branch cache misses. | ||
| // GitHub Actions actions/cache is branch-scoped: caches written by the conclusion job | ||
| // on one PR branch are invisible to the activation job running on a different PR branch. | ||
|
|
@@ -353,7 +369,7 @@ func (c *Compiler) buildActivationDailyAICGuardrailStep(data *WorkflowData) []st | |
| // The fallback script is a no-op when the cache file already exists. | ||
| steps = append(steps, " - name: Restore daily AIC usage cache (artifact fallback)\n") | ||
| steps = append(steps, " id: restore-daily-aic-cache-fallback\n") | ||
| steps = append(steps, fmt.Sprintf(" if: %s\n", maxDailyAICreditsConfiguredIfExpr)) | ||
| steps = append(steps, fmt.Sprintf(" if: %s && steps.detect-daily-aic-cache-miss.outputs.cache_miss == 'true'\n", maxDailyAICreditsConfiguredIfExpr)) | ||
|
Copilot marked this conversation as resolved.
Outdated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/zoom-out] The fallback 💡 SuggestionConsider consolidating both conditions inside a single |
||
| steps = append(steps, " continue-on-error: true\n") | ||
| steps = append(steps, fmt.Sprintf(" uses: %s\n", getCachedActionPin("actions/github-script", data))) | ||
| steps = append(steps, " with:\n") | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -166,6 +166,21 @@ Guardrail test workflow` | |||||||||||||||||||||
| if !strings.Contains(activationSection, "id: restore-daily-aic-cache-fallback") { | ||||||||||||||||||||||
| t.Fatal("expected activation job to include the artifact-based AIC cache fallback step") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if !strings.Contains(activationSection, "id: detect-daily-aic-cache-miss") { | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] 💡 Suggested guarddetectIdx := strings.Index(activationSection, "id: detect-daily-aic-cache-miss")
fallbackIdx := strings.Index(activationSection, "id: restore-daily-aic-cache-fallback")
if detectIdx == -1 || fallbackIdx == -1 || detectIdx >= fallbackIdx {
t.Fatal("expected detect-daily-aic-cache-miss to appear before restore-daily-aic-cache-fallback")
}The golden tests already verify ordering indirectly, but a direct assertion here makes the invariant explicit and fails fast with a descriptive message. |
||||||||||||||||||||||
| t.Fatal("expected activation job to include a cache-miss detection step before fallback") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if !strings.Contains(activationSection, "GH_AW_RESTORE_DAILY_AIC_CACHE_HIT: ${{ steps.restore-daily-aic-cache.outputs.cache-hit }}") { | ||||||||||||||||||||||
| t.Fatal("expected cache-miss detection to pass cache-hit output via env for template-injection safety") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if !strings.Contains(activationSection, "GH_AW_RESTORE_DAILY_AIC_CACHE_MATCHED_KEY: ${{ steps.restore-daily-aic-cache.outputs.cache-matched-key }}") { | ||||||||||||||||||||||
| t.Fatal("expected cache-miss detection to pass cache-matched-key output via env") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if !strings.Contains(activationSection, `if [ -z "$GH_AW_RESTORE_DAILY_AIC_CACHE_HIT" ] || { [ "$GH_AW_RESTORE_DAILY_AIC_CACHE_HIT" = "false" ] && [ -z "$GH_AW_RESTORE_DAILY_AIC_CACHE_MATCHED_KEY" ]; }; then`) { | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test verifies that the shell predicate string is present, but doesn't exercise its four distinct cases. The restore-key-hit case ( 💡 Missing cases to cover
A table-driven shell test (or even a small |
||||||||||||||||||||||
| t.Fatal("expected cache-miss detection to treat missing matched key as a true miss") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if !strings.Contains(activationSection, "steps.detect-daily-aic-cache-miss.outputs.cache_miss == 'true'") { | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Substring-only assertion does not validate the expression form: this test passes even when the 💡 Why it matters and a stronger alternativeThe current check confirms the fragment That line contains the substring, so this test passes — even though the generated YAML is invalid. A stronger assertion verifies the complete, well-formed // Check the entire if: line on the fallback step is a properly formed bare expression
wantFallbackIf := "if: env.GH_AW_MAX_DAILY_AI_CREDITS != '' && steps.detect-daily-aic-cache-miss.outputs.cache_miss == 'true'"
if !strings.Contains(activationSection, wantFallbackIf) {
t.Fatalf("expected fallback step if: to use a well-formed bare expression, got activation section:\n%s", activationSection)
}Without this, the |
||||||||||||||||||||||
| t.Fatal("expected artifact fallback step to run only when cache-miss detection reports a miss") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if !strings.Contains(lockStr, "id: upload-daily-aic-cache") { | ||||||||||||||||||||||
| t.Fatal("expected conclusion job to include the AIC usage cache artifact upload step") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,9 +96,21 @@ jobs: | |
| key: agentic-workflow-usage-workflow-${{ github.run_id }} | ||
| restore-keys: agentic-workflow-usage-workflow- | ||
| path: /tmp/gh-aw/agentic-workflow-usage-cache.jsonl | ||
| - name: Detect daily AIC usage cache miss | ||
| id: detect-daily-aic-cache-miss | ||
| if: ${{ env.GH_AW_MAX_DAILY_AI_CREDITS != '' }} | ||
| env: | ||
| GH_AW_RESTORE_DAILY_AIC_CACHE_HIT: ${{ steps.restore-daily-aic-cache.outputs.cache-hit }} | ||
| GH_AW_RESTORE_DAILY_AIC_CACHE_MATCHED_KEY: ${{ steps.restore-daily-aic-cache.outputs.cache-matched-key }} | ||
| run: | | ||
| if [ -z "$GH_AW_RESTORE_DAILY_AIC_CACHE_HIT" ] || { [ "$GH_AW_RESTORE_DAILY_AIC_CACHE_HIT" = "false" ] && [ -z "$GH_AW_RESTORE_DAILY_AIC_CACHE_MATCHED_KEY" ]; }; then | ||
| echo "cache_miss=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "cache_miss=false" >> "$GITHUB_OUTPUT" | ||
| fi | ||
| - name: Restore daily AIC usage cache (artifact fallback) | ||
| id: restore-daily-aic-cache-fallback | ||
| if: ${{ env.GH_AW_MAX_DAILY_AI_CREDITS != '' }} | ||
| if: ${{ env.GH_AW_MAX_DAILY_AI_CREDITS != '' }} && steps.detect-daily-aic-cache-miss.outputs.cache_miss == 'true' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Broken 💡 Impact and required fixEvery updated golden file now contains: if: ${{ env.GH_AW_MAX_DAILY_AI_CREDITS != '' }} && steps.detect-daily-aic-cache-miss.outputs.cache_miss == 'true'GitHub Actions resolves # Option A – single ${{ }} wrapping the entire expression
if: ${{ env.GH_AW_MAX_DAILY_AI_CREDITS != '' && steps.detect-daily-aic-cache-miss.outputs.cache_miss == 'true' }}
# Option B – fully bare (no ${{ }} wrapper at all)
if: env.GH_AW_MAX_DAILY_AI_CREDITS != '' && steps.detect-daily-aic-cache-miss.outputs.cache_miss == 'true'The golden files need to be regenerated after the Go-source fix ( |
||
| continue-on-error: true | ||
| uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 | ||
| with: | ||
|
|
||
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.
[/diagnose] The detect step has no
continue-on-error: true, unlike the surrounding restore and fallback steps. If writing to$GITHUB_OUTPUTever fails (unwritable path, runner misconfiguration), the job dies here —cache_missoutput is never set, the fallback silently does not run, and the cache stays unpopulated on a confirmed miss.💡 Suggested fix
Add
continue-on-error: trueto stay consistent with the restore step and keep the guardrail flow robust:If the step still fails despite
continue-on-error,cache_missremains unset and the fallback conditioncache_miss == 'true'evaluates to false — no worse than today's pre-PR behaviour.