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

[Copilot]

gh aw compile --action-tag ... was switching action resolution to a pinned ref without recording the compiler ref in lock metadata unless the build itself was a release build. This change makes compiler_version show up for that code path as well, so lock files preserve the ref used for compilation.

• Metadata behavior

  • Populate gh-aw-metadata.compiler_version when an explicit --action-tag is in effect, even for non-release builds.
  • Leave existing release-build behavior unchanged.

• Compile path update

  • Apply the metadata override at lock header generation time, using the compiler’s current version/ref only when:
    • the normal release-only metadata path would omit it, and
    • an action tag override is present.

• Regression coverage

  • Extend the metadata test matrix to cover non-release compiles with --action-tag.
  • Assert that the generated lock header includes compiler_version for that path.
  • Add CLI integration coverage to verify the lock metadata includes compiler_version when the override is provided via --action-tag or the --gh-aw-ref alias.

# Before (non-release + --action-tag)
# gh-aw-metadata: {"schema_version":"v4", ...}

# After
# gh-aw-metadata: {"schema_version":"v4","compiler_version":"401bd13", ...}

[pelikhan]

@copilot review if there is anything cli arg as well

[Copilot]

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.

[Copilot]

Pull request overview

This PR ensures lock files produced by gh aw compile --action-tag ... (and the --gh-aw-ref alias path) record the compiler ref/version in gh-aw-metadata.compiler_version even when the binary is not a release build, improving reproducibility and traceability of compiled outputs.

Changes:

• Add a compile-time metadata override to populate compiler_version when --action-tag is in effect and the default (release-only) path would otherwise omit it.
• Extend unit test coverage to include the non-release + --action-tag case.
• Add CLI integration coverage verifying compiler_version is present for both --action-tag and --gh-aw-ref.

Show a summary per file

File	Description
pkg/workflow/compiler_yaml.go	Populate compiler_version in lock metadata when an action tag override is set and the default release-only metadata path leaves it empty.
pkg/workflow/compiler_yaml_test.go	Extend the lock-metadata version matrix to cover non-release compiles with an action tag override.
pkg/cli/compile_integration_test.go	Add CLI integration test asserting lock metadata includes a non-empty compiler_version for --action-tag and --gh-aw-ref.

Copilot's findings

Tip

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

• Files reviewed: 3/3 changed files
• Comments generated: 0

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #39687 does not have the 'implementation' label (has_implementation_label=false) and has only 76 new lines of code in business logic directories (requires_adr_by_default_volume=false, threshold=100).

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

Skills-Based Review

Applied /tdd and /diagnose — approving with two minor suggestions.

Key Themes & Highlights

Positive Highlights

• Minimal, targeted fix (3 lines) with zero risk to existing release-build or dev-build paths — the CompilerVersion == "" guard makes it strictly additive
• Both --action-tag and its --gh-aw-ref alias are exercised in the integration test, closing the CLI surface completely
• Unit test matrix extended with a realistic commit SHA (401bd13) and action tag — the new case mirrors the exact scenario described in the bug
• Clean PR description with before/after YAML makes the change reviewable without running code

Suggestions (non-blocking)

1. [/tdd] Add isRelease=true + actionTag set boundary test to the unit matrix — documents that the release path wins the priority ordering (see inline comment)
2. [/diagnose] central_slash_command_workflow.go has a parallel IsRelease() gate that this fix does not reach — worth clarifying scope or tracking as a follow-up (see inline comment)

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

[github-actions]

[/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.

[github-actions]

[/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.

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 90/100 — Excellent

Analyzed 2 test function(s) (3 table scenarios): 2 design, 0 implementation, 0 guideline violation(s).

📊 Metrics & Test Classification (2 tests analyzed)

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	Yes (compiler_yaml_test.go: 11 lines added vs 3 production lines ≈ 3.7:1)
🚨 Coding-guideline violations	0

Test	File	Classification	Issues Detected
TestCompileWithActionRefOverrideIncludesCompilerVersionMetadata	pkg/cli/compile_integration_test.go:2250	✅ Design	—
TestLockMetadataVersionInReleaseBuilds (new row)	pkg/workflow/compiler_yaml_test.go:1399	✅ Design	Minor inflation: 11 test lines vs 3 production lines

Go: 2 (*_test.go); JavaScript: 0. No other languages detected.

Verdict

✅ Check passed. 0% implementation tests (threshold: 30%). Both tests enforce behavioral contracts — they verify that the lock file gh-aw-metadata contains a non-empty compiler_version when compiling with --action-tag or --gh-aw-ref, which is directly observable output. The integration test's 62-line addition is justified by its end-to-end setup boilerplate covering two flag variants. Minor inflation in the unit test (3.7:1) costs 10 points but does not affect the pass verdict.

🧪 Test quality analysis by Test Quality Sentinel · ◷

[github-actions]

✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both tests enforce behavioral contracts on observable lock file output.

[github-actions]

The production fix is correct and the unit test covers the exact value ("compiler_version":"401bd13"). Three non-blocking observations flagged inline.

📋 Observations summary

compiler_yaml.go:113 — "dev" written as compiler_version for local builds

When a locally-built binary (no release linker flags) runs --action-tag, c.GetVersion() returns "dev" — a placeholder, not a ref. The unit test specifically avoids this path by pre-seeding a real SHA. Either add a guard (&& c.GetVersion() != "dev") or add a test case to explicitly document and protect the "dev" behaviour.

compiler_yaml_test.go:1450 — Global state not restored between tests

SetVersion / SetIsRelease mutate package-level globals without t.Cleanup. The new test case changes the dirty post-test state from {isRelease:true} to {isRelease:false}. A cleanup guard would make this safe against future test additions.

compile_integration_test.go:2306 — Integration assertion is non-empty only

The test accepts any non-empty compiler_version, including "dev". The unit test already checks exact values so this is not a critical gap, but pinning the test binary version with -X would make the integration check equally precise.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

🔎 Code quality review by PR Code Quality Reviewer

[github-actions]

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.

[github-actions]

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.

[github-actions]

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.