Skip to content

fix: include workflow ID in experiment artifact cache keys and artifact name#29747

Merged
pelikhan merged 2 commits intomainfrom
copilot/review-artifact-key-generation
May 2, 2026
Merged

fix: include workflow ID in experiment artifact cache keys and artifact name#29747
pelikhan merged 2 commits intomainfrom
copilot/review-artifact-key-generation

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 2, 2026

Summary

Fixes the experiment artifact key generation to include the workflow ID.

Root Cause

The experiment cache steps in the activation job used ${{ env.GH_AW_WORKFLOW_ID_SANITIZED }} in their cache keys:

key: experiments-${{ env.GH_AW_WORKFLOW_ID_SANITIZED }}-${{ github.run_id }}

However, GH_AW_WORKFLOW_ID_SANITIZED is only set in the agent job environment — not in the activation job. At runtime this env var evaluates to an empty string, resulting in cache keys like experiments--12345 instead of experiments-smokecopilot-12345. This means:

  • All workflows share the same experiment cache namespace (empty ID)
  • Cross-workflow experiment state contamination is possible

Similarly, the experiment artifact was uploaded as just "experiment" without any workflow ID scoping.

Fix

  1. Cache keys: Replace ${{ env.GH_AW_WORKFLOW_ID_SANITIZED }} with the literal sanitized workflow ID baked in at compile time (since data.WorkflowID is known when the workflow is compiled):

    # Before:
    key: experiments-${{ env.GH_AW_WORKFLOW_ID_SANITIZED }}-${{ github.run_id }}
    # After:
    key: experiments-smokecopilot-${{ github.run_id }}
  2. Artifact name: Include the sanitized workflow ID as prefix for non-workflow_call workflows:

    # Before:
    name: experiment
    # After:
    name: smokecopilot-experiment
  3. buildExperimentArtifactDownloadSteps: Updated to accept *WorkflowData instead of a prefix string, computing the correct artifact name for both workflow_call and regular workflows. Upload and download names are always consistent.

Files Changed

  • pkg/workflow/compiler_experiments.go — core fix + new helpers
  • pkg/workflow/threat_detection.go — updated call site
  • pkg/workflow/compiler_experiments_test.go — updated tests
  • Recompiled lock files + updated golden test fixtures

Reference: https://github.com/github/gh-aw/actions/runs/25249937943/job/74040362646#step:14:1

Copilot AI and others added 2 commits May 2, 2026 10:53
…ct name

- Replace `${{ env.GH_AW_WORKFLOW_ID_SANITIZED }}` in experiment cache
  keys with the literal sanitized workflow ID baked in at compile time.
  The activation job does not set this env var, so the cache key was
  resolving to an empty string (e.g. `experiments--<run_id>`).
- Include the sanitized workflow ID as prefix in the experiment artifact
  name for non-workflow_call workflows (e.g. `smokecopilot-experiment`).
- Update `buildExperimentArtifactDownloadSteps` to accept `*WorkflowData`
  and compute the correct artifact name for both workflow_call and regular
  workflows.
- Recompile affected lock files and update golden test fixtures.

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0b4d51cc-ad50-45c8-add4-48e3e0fe09b3

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…mpty ID fallback

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0b4d51cc-ad50-45c8-add4-48e3e0fe09b3

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI requested a review from pelikhan May 2, 2026 10:59
@pelikhan pelikhan marked this pull request as ready for review May 2, 2026 11:00
Copilot AI review requested due to automatic review settings May 2, 2026 11:00
@pelikhan pelikhan merged commit 5289cc4 into main May 2, 2026
18 of 19 checks passed
@pelikhan pelikhan deleted the copilot/review-artifact-key-generation branch May 2, 2026 11:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes experiment cache key + artifact naming so experiment state is scoped per workflow (avoids cross-workflow cache/artifact collisions), including in activation jobs where GH_AW_WORKFLOW_ID_SANITIZED is not set.

Changes:

  • Embed a compile-time sanitized workflow ID into experiment cache keys and (non-workflow_call) artifact names.
  • Refactor experiment artifact download step generation to derive the correct artifact name from WorkflowData (consistent for workflow_call vs regular workflows).
  • Update tests, lockfiles, and golden fixtures to reflect the new naming.
Show a summary per file
File Description
pkg/workflow/compiler_experiments.go Computes cache keys using compile-time sanitized workflow ID; updates upload/download artifact naming helpers.
pkg/workflow/compiler_experiments_test.go Adjusts tests to assert workflow-scoped cache keys/artifact names and updated helper signature.
pkg/workflow/threat_detection.go Updates call site to new buildExperimentArtifactDownloadSteps(*WorkflowData) signature.
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden Golden output updates from recompilation (pinned action SHAs / formatting).
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden Golden output updates from recompilation (pinned action SHAs / formatting).
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden Golden output updates from recompilation (pinned action SHAs / formatting).
.github/workflows/smoke-copilot.lock.yml Updates experiment cache keys and artifact name to include sanitized workflow ID.
.github/workflows/daily-community-attribution.lock.yml Updates experiment cache keys and artifact name to include sanitized workflow ID.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (2)

pkg/workflow/compiler_experiments.go:258

  • sanitizedID is used unconditionally in the cache key/restore key format strings. If data.WorkflowID is empty (or sanitizes to an empty string), this will still produce keys like experiments--${{ github.run_id }} and reintroduce cross-workflow cache collisions. Consider explicitly validating data.WorkflowID/sanitizedID when experiments are enabled, or falling back to a non-empty, workflow-scoped identifier.
	// Use the literal sanitized workflow ID in the cache key so it is correct in the
	// activation job, which does not have GH_AW_WORKFLOW_ID_SANITIZED in its environment.
	sanitizedID := SanitizeWorkflowIDForCacheKey(data.WorkflowID)
	cacheKey := fmt.Sprintf("experiments-%s-${{ github.run_id }}", sanitizedID)
	restoreKey := fmt.Sprintf("experiments-%s-", sanitizedID)

pkg/workflow/compiler_experiments.go:412

  • experimentArtifactUploadName takes sanitizedID as a separate parameter even though it can be derived from data.WorkflowID. This makes it easier for callers to accidentally pass a mismatched value (leading to upload/download name divergence). Since sanitization is cheap, consider computing the sanitized ID inside the helper (or accepting the raw workflow ID) to keep the API harder to misuse.
func experimentArtifactUploadName(data *WorkflowData, sanitizedID string) string {
	if hasWorkflowCallTrigger(data.On) {
		return artifactPrefixExprForActivationJob(data) + constants.ExperimentArtifactName
	}
	if sanitizedID == "" {
		return constants.ExperimentArtifactName
	}
	return sanitizedID + "-" + constants.ExperimentArtifactName
}
  • Files reviewed: 8/8 changed files
  • Comments generated: 1

// writes a Markdown step summary); outputs: one per experiment (e.g. "caveman=yes") + "experiments" JSON blob
// 3. Save experiment cache – actions/cache/save keyed by workflow ID
// 4. Upload experiment artifact – actions/upload-artifact named "experiment"
// 4. Upload experiment artifact – actions/upload-artifact named "{workflowID}-experiment"
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 2, 2026

🧪 Test Quality Sentinel Report

Test Quality Score: 97/100

Excellent test quality

Metric Value
New/modified tests analyzed 2
✅ Design tests (behavioral contracts) 2 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 2 (100%)
Duplicate test clusters 0
Test inflation detected No (ratio: 0.48×)
🚨 Coding-guideline violations None

Test Classification Details

Test File Classification Issues Detected
TestGenerateExperimentSteps_Generated (modified) pkg/workflow/compiler_experiments_test.go:226 ✅ Design None — new assertions verify workflow ID appears in cache key and artifact name
TestBuildExperimentArtifactDownloadStep_NoPrefix (new) pkg/workflow/compiler_experiments_test.go:326 ✅ Design None — tests non-workflow_call branch for correct sanitized ID as artifact prefix

Analysis Notes

TestGenerateExperimentSteps_Generated — Three assertions were added directly addressing the PR fix: assert.Contains(joined, "myworkflow-experiment", ...), assert.Contains(joined, "experiments-myworkflow-", ...), and critically assert.NotContains(joined, "GH_AW_WORKFLOW_ID_SANITIZED", ...). The NotContains check on the env-var string is a particularly high-value guard: it would catch any future regression where the cache key accidentally embeds an unresolved variable reference instead of the literal sanitized ID.

TestBuildExperimentArtifactDownloadStep_NoPrefix — New test covers the non-workflow_call code path, asserting that the sanitized workflow ID (smokecopilot-experiment) is used as the artifact name prefix. This is the complementary branch to the existing TestBuildExperimentArtifactDownloadStep_Generated test (which covers workflow_call with a runtime prefix expression), giving full branch coverage of the artifact-naming logic.

No issues detected: build tag present (//go:build !integration), no mock libraries, all assertions include descriptive messages, test growth proportional to production code changes (+23 test lines vs +48 production lines, 0.48× ratio well below the 2:1 threshold).


Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 2 tests — unit (//go:build !integration)
  • 🟨 JavaScript (*.test.cjs, *.test.js): 0 tests

Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). Both new/modified tests enforce concrete behavioral contracts directly tied to the PR fix — verifying that workflow IDs appear correctly in cache keys and artifact names across both workflow_call and non-workflow_call triggers.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

References: §25250407795

🧪 Test quality analysis by Test Quality Sentinel · ● 1M ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 97/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both new/modified tests enforce behavioral contracts directly tied to the PR fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants