Skip to content

Record compiler ref in lock metadata for --action-tag compiles #39687

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.

Sign up for GitHub

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

Merged
pelikhan merged 3 commits into from
Jun 16, 2026
Merged

Record compiler ref in lock metadata for --action-tag compiles #39687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adc1df2
Record compile ref for action-tag
Copilot Jun 16, 2026
1127713
Add CLI regression coverage for compiler version metadata
Copilot Jun 16, 2026
50872c3
Merge branch 'main' into copilot/update-compile-cli-command
pelikhan Jun 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions pkg/cli/compile_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2247,3 +2247,65 @@ func TestCompileWithActionsRepoDefaultFallback(t *testing.T) {

t.Logf("Default actions repo test passed - default repo baked into lock file: %s", lockFilePath)
}

func TestCompileWithActionRefOverrideIncludesCompilerVersionMetadata(t *testing.T) {
tests := []struct {
name string
args []string
}{
{
name: "action-tag",
args: []string{"--action-tag", "v9.9.9"},
},
{
name: "gh-aw-ref",
args: []string{"--gh-aw-ref", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
setup := setupIntegrationTest(t)
defer setup.cleanup()

srcPath := filepath.Join(projectRoot, "pkg/cli/workflows/test-actions-repo.md")
dstPath := filepath.Join(setup.workflowsDir, "test-actions-repo.md")

srcContent, err := os.ReadFile(srcPath)
if err != nil {
t.Fatalf("Failed to read source workflow file %s: %v", srcPath, err)
}
if err := os.WriteFile(dstPath, srcContent, 0o644); err != nil {
t.Fatalf("Failed to write workflow to test dir: %v", err)
}

cmdArgs := append([]string{"compile"}, tt.args...)
cmdArgs = append(cmdArgs, dstPath)
cmd := exec.Command(setup.binaryPath, cmdArgs...)
output, err := cmd.CombinedOutput()
if err != nil {
t.Fatalf("CLI compile command failed: %v\nOutput: %s", err, string(output))
}

lockFilePath := filepath.Join(setup.workflowsDir, "test-actions-repo.lock.yml")
lockContent, err := os.ReadFile(lockFilePath)
if err != nil {
t.Fatalf("Failed to read lock file: %v", err)
}

metadataLine := ""
for line := range strings.SplitSeq(string(lockContent), "\n") {
if strings.Contains(line, "gh-aw-metadata:") {
metadataLine = line
break
}
}
if metadataLine == "" {
t.Fatal("Could not find gh-aw-metadata in lock file")
}
if !strings.Contains(metadataLine, `"compiler_version":"`) || strings.Contains(metadataLine, `"compiler_version":""`) {

@github-actions github-actions Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Integration test asserts non-empty compiler_version but never checks the actual value. This passes equally whether compiler_version is "dev", a real SHA, or any other non-empty string — meaning a regression where the wrong ref is recorded would go undetected here.

💡 Details

For an integration test the binary version is whatever the test binary was built with, so you cannot always assert an exact string. But you could at least verify it is not the "dev" placeholder if you care about real provenance, or document via a comment that "dev" is the expected value for test builds.

Alternately, build the test binary with a known -X version flag (as already done in some other integration tests) so the assertion can check the exact value.

t.Fatalf("Expected non-empty compiler_version in metadata, got: %s", metadataLine)
}
})
}
}
3 changes: 3 additions & 0 deletions pkg/workflow/compiler_yaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ func (c *Compiler) generateWorkflowHeader(yaml *strings.Builder, data *WorkflowD
agentInfo.EngineVersions = collectEngineVersionsForMetadata(data)
agentInfo.AgentImageRunner = resolveAgentImageRunnerIdentifier(data.RawFrontmatter)
metadata := GenerateLockMetadata(LockHashInfo{FrontmatterHash: frontmatterHash, BodyHash: bodyHash}, data.StopTime, c.effectiveStrictMode(data.RawFrontmatter), agentInfo)
if metadata.CompilerVersion == "" && c.GetActionTag() != "" {

@pelikhan pelikhan Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot review if there is anything cli arg as well

Copilot AI Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 1127713. I added CLI integration coverage for the flag-driven paths as well: --action-tag and the --gh-aw-ref alias now both verify the generated lock metadata includes a non-empty compiler_version.

@github-actions github-actions Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnose] central_slash_command_workflow.go has a parallel IsRelease() gate for its own commandsHeaderMetadata.Compiler field (around line 462). If someone runs gh aw compile --action-tag and the repository uses a central slash command dispatch workflow, that path would still emit "dev" for its compiler identifier.

If this is intentional (different metadata structure, different downstream consumers), a brief comment here noting the scope of this fix would help future readers avoid applying it in the wrong place.

💡 Context

The affected code in central_slash_command_workflow.go:

metadataCompilerVersion := "dev"
if IsRelease() && strings.TrimSpace(GetVersion()) != "" {
    metadataCompilerVersion = GetVersion()
}

That path uses package-level functions, not the Compiler receiver, so it cannot access c.GetActionTag(). A follow-up that threads the compiler instance (or the resolved version) through generateCommandsHeaderMetadata would close the gap — or explicitly document why it is out of scope.

metadata.CompilerVersion = c.GetVersion()

@github-actions github-actions Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

c.GetVersion() returns "dev" for local/unversioned builds, silently writing useless provenance. For any developer running gh aw compile --action-tag v1.0.0 on a locally-built binary, this writes "compiler_version":"dev" — a placeholder that identifies nothing about which commit compiled the workflow.

💡 Details and suggested guard

The unit test avoids this case by always pre-seeding version: "401bd13" before construction:

SetVersion("401bd13")
compiler.SetActionTag("v9.9.9")

That means the test never exercises the path where c.GetVersion() is "dev". If the intent is "record the version only when it is meaningful (i.e. not the build-time placeholder)", add a guard:

if metadata.CompilerVersion == "" && c.GetActionTag() != "" && c.GetVersion() != "dev" {
    metadata.CompilerVersion = c.GetVersion()
}

If the intent is "always record whatever version the binary has, even if it is dev", that is defensible — but then the unit test should add an explicit case with version: "dev" to document and protect that behaviour.

}
metadataJSON, err := metadata.ToJSON()
if err != nil {
// Fallback to legacy format if JSON serialization fails
Expand Down
11 changes: 11 additions & 0 deletions pkg/workflow/compiler_yaml_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1399,18 +1399,28 @@ func TestLockMetadataVersionInReleaseBuilds(t *testing.T) {
name string
isRelease bool
version string
actionTag string
expectVersion bool
}{
{
name: "dev build should not include version",
isRelease: false,
version: "dev",
actionTag: "",
expectVersion: false,
},
{
name: "release build should include version",
isRelease: true,
version: "v0.1.2",
actionTag: "",
expectVersion: true,
},
{
name: "action-tag compile should include current ref",
isRelease: false,
version: "401bd13",
actionTag: "v9.9.9",
expectVersion: true,
},

@github-actions github-actions Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] Missing boundary test: isRelease=true + actionTag="v9.9.9" — the case where both paths are active simultaneously.

The new guard (metadata.CompilerVersion == "" && c.GetActionTag() != "") relies on the release-build path in GenerateLockMetadata having already populated CompilerVersion, leaving it non-empty so the guard is skipped. Without an explicit test for this ordering, a future refactor that swaps or merges the two assignment paths could silently cause the action-tag value to overwrite a correctly-set release version.

💡 Suggested test case to add 
{
    name:          "release build with action-tag should use release version (not guard)",
    isRelease:     true,
    version:       "v1.2.3",
    actionTag:     "v9.9.9",
    expectVersion: true,
    // compiler_version should be "v1.2.3" (release path), not "401bd13"
},

This documents the intended priority and will catch a regression if the guard order ever changes.

}
Expand All @@ -1437,6 +1447,7 @@ Test prompt.

// Compile the workflow
compiler := NewCompiler()
compiler.SetActionTag(tt.actionTag)

@github-actions github-actions Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SetVersion / SetIsRelease mutate package-level globals with no t.Cleanup to restore them. The new third test case changes the dirty post-test global state from {isRelease:true, version:"v0.1.2"} to {isRelease:false, version:"401bd13"}. Any test in the package that runs afterward and reads IsRelease() or GetVersion() without an explicit reset will observe these stale values.

💡 Suggested fix

Add a cleanup at the top of each sub-test (or once at the parent level):

origVersion := GetVersion()
origRelease := IsRelease()
t.Cleanup(func() {
    SetVersion(origVersion)
    SetIsRelease(origRelease)
})

This is a pre-existing pattern in the test, but the new case changes which dirty state is left behind — worth hardening now while the test is being touched.

err := compiler.CompileWorkflow(workflowPath)
if err != nil {
t.Fatalf("Failed to compile workflow: %v", err)
Expand Down
Loading