From f30ad7daa6cd78b8cf41dc0a77990a2dfc4aa89a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 2 Aug 2026 14:45:23 +0000 Subject: [PATCH 1/4] Initial plan From 61b26aba3a28628aafb4548f5b4fee2ade59f98a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 2 Aug 2026 14:59:46 +0000 Subject: [PATCH 2/4] test: add activation step unit coverage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../compiler_activation_steps_test.go | 508 ++++++++++++++++++ 1 file changed, 508 insertions(+) create mode 100644 pkg/workflow/compiler_activation_steps_test.go diff --git a/pkg/workflow/compiler_activation_steps_test.go b/pkg/workflow/compiler_activation_steps_test.go new file mode 100644 index 00000000000..76b531aeec1 --- /dev/null +++ b/pkg/workflow/compiler_activation_steps_test.go @@ -0,0 +1,508 @@ +//go:build !integration + +package workflow + +import ( + "strings" + "testing" + + "github.com/github/gh-aw/pkg/constants" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newActivationStepsTestCompiler(version string) *Compiler { + if version == "" { + version = "dev" + } + compiler := NewCompiler(WithVersion(version)) + compiler.SetActionMode(ActionModeDev) + return compiler +} + +func newActivationStepsTestContext(data *WorkflowData) *activationJobBuildContext { + if data == nil { + data = &WorkflowData{} + } + return &activationJobBuildContext{ + data: data, + lockFilename: "test.lock.yml", + outputs: map[string]string{}, + } +} + +func TestActivationStepsAddReactionStep(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("adds reaction step", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + AIReaction: "eyes", + }) + ctx.hasReaction = true + ctx.reactionIssues = true + + compiler.addActivationReactionStep(ctx) + + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Add eyes reaction for immediate feedback") + assert.Contains(t, steps, "id: react") + assert.Contains(t, steps, "uses: actions/github-script") + assert.Contains(t, steps, "GH_AW_REACTION: \"eyes\"") + assert.Contains(t, steps, "github-token: ${{ secrets.GITHUB_TOKEN }}") + assert.Contains(t, steps, "add_reaction.cjs") + }) + + t.Run("skips when reaction is disabled", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{AIReaction: "eyes"}) + + compiler.addActivationReactionStep(ctx) + + assert.Empty(t, ctx.steps) + }) +} + +func TestActivationStepsAddSecretValidationStep(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + engine, err := compiler.getAgenticEngine("copilot") + require.NoError(t, err) + + ctx := newActivationStepsTestContext(&WorkflowData{AI: "copilot"}) + ctx.engine = engine + + compiler.addActivationSecretValidationStep(ctx) + + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "id: validate-secret") + assert.Contains(t, steps, "validate_multi_secret.sh") + assert.Equal(t, "${{ steps.validate-secret.outputs.verification_result }}", ctx.outputs["secret_verification_result"]) +} + +func TestActivationStepsAddOAuthTokenCheckStep(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("uses default copilot token secret", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{}) + + compiler.addActivationOAuthTokenCheckStep(ctx) + + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Check for OAuth tokens") + assert.Contains(t, steps, "id: check-oauth-tokens") + assert.Contains(t, steps, "check_oauth_tokens.sh") + assert.Contains(t, steps, constants.CopilotGitHubToken+": ${{ secrets."+constants.CopilotGitHubToken+" }}") + assert.Contains(t, steps, constants.EnvVarGitHubToken+": ${{ secrets."+constants.EnvVarGitHubToken+" }}") + assert.Contains(t, steps, constants.EnvVarGitHubMCPServerToken+": ${{ secrets."+constants.EnvVarGitHubMCPServerToken+" }}") + }) + + t.Run("uses engine env override for copilot token", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + EngineConfig: &EngineConfig{ + Env: map[string]string{ + constants.CopilotGitHubToken: "${{ secrets.CUSTOM_COPILOT_TOKEN }}", + }, + }, + }) + + compiler.addActivationOAuthTokenCheckStep(ctx) + + assert.Contains(t, strings.Join(ctx.steps, ""), constants.CopilotGitHubToken+": ${{ secrets.CUSTOM_COPILOT_TOKEN }}") + }) +} + +func TestActivationStepsAddCrossRepoGuidanceStep(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("adds workflow_call guidance", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + On: "\"on\":\n workflow_call:\n", + }) + + compiler.addActivationCrossRepoGuidanceStep(ctx) + + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Print cross-repo setup guidance") + assert.Contains(t, steps, "resolve-host-repo.outputs.target_repo != github.repository") + assert.Contains(t, steps, "cross-repo workflow_call") + }) + + t.Run("skips for inlined imports", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + On: "\"on\":\n workflow_call:\n", + InlinedImports: true, + }) + + compiler.addActivationCrossRepoGuidanceStep(ctx) + + assert.Empty(t, ctx.steps) + }) +} + +func TestActivationStepsAddRepositoryAndOutputSteps(t *testing.T) { + t.Run("adds repository and output steps", func(t *testing.T) { + originalIsRelease := isReleaseBuild + isReleaseBuild = true + t.Cleanup(func() { isReleaseBuild = originalIsRelease }) + + compiler := newActivationStepsTestCompiler("v1.2.3") + ctx := newActivationBuildContext(&WorkflowData{}, false, "", "test.lock.yml") + + err := compiler.addActivationRepositoryAndOutputSteps(ctx) + + require.NoError(t, err) + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Checkout .github and .agents folders") + assert.Contains(t, steps, "Check workflow lock file") + assert.Contains(t, steps, "Check compile-agentic version") + assert.Equal(t, `""`, ctx.outputs["comment_id"]) + assert.Equal(t, `""`, ctx.outputs["comment_repo"]) + }) + + t.Run("returns text output errors", func(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + ctx := newActivationBuildContext(&WorkflowData{ + NeedsTextOutput: true, + Model: "/bad-provider", + EngineConfig: &EngineConfig{ + ID: "opencode", + }, + }, false, "", "test.lock.yml") + + err := compiler.addActivationRepositoryAndOutputSteps(ctx) + + require.Error(t, err) + }) +} + +func TestActivationStepsAddCheckoutAndBaseRestoreStep(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("adds checkout and save-base steps", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{}) + + compiler.addActivationCheckoutAndBaseRestoreStep(ctx) + + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Checkout .github and .agents folders") + assert.Contains(t, steps, "Save agent config folders for base branch restoration") + assert.Contains(t, steps, "save_base_github_folders.sh") + }) + + t.Run("skips when checkout is disabled by action tag", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + Features: map[string]any{ + "action-tag": "v1.2.3", + }, + }) + + compiler.addActivationCheckoutAndBaseRestoreStep(ctx) + + assert.Empty(t, ctx.steps) + }) +} + +func TestActivationStepsAddLockFileStep(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("adds stale lock step", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + ActivationGitHubToken: "${{ secrets.CUSTOM_TOKEN }}", + StaleCheckFull: true, + }) + + compiler.addActivationLockFileStep(ctx) + + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Check workflow lock file") + assert.Contains(t, steps, "id: check-lock-file") + assert.Contains(t, steps, "GH_AW_WORKFLOW_FILE: \"test.lock.yml\"") + assert.Contains(t, steps, "GH_AW_STALE_CHECK_FULL: \"true\"") + assert.Contains(t, steps, "github-token: ${{ secrets.CUSTOM_TOKEN }}") + assert.Contains(t, steps, "check_workflow_timestamp_api.cjs") + }) + + t.Run("skips when stale check is disabled", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{StaleCheckDisabled: true}) + + compiler.addActivationLockFileStep(ctx) + + assert.Empty(t, ctx.steps) + }) +} + +func TestActivationStepsAddVersionCheckStep(t *testing.T) { + t.Run("adds version check for release builds", func(t *testing.T) { + originalIsRelease := isReleaseBuild + isReleaseBuild = true + t.Cleanup(func() { isReleaseBuild = originalIsRelease }) + + compiler := newActivationStepsTestCompiler("v1.2.3") + ctx := newActivationStepsTestContext(&WorkflowData{}) + + compiler.addActivationVersionCheckStep(ctx) + + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Check compile-agentic version") + assert.Contains(t, steps, "GH_AW_COMPILED_VERSION: \"v1.2.3\"") + assert.Contains(t, steps, "check_version_updates.cjs") + }) + + t.Run("skips version check for dev builds", func(t *testing.T) { + compiler := newActivationStepsTestCompiler("dev") + ctx := newActivationStepsTestContext(&WorkflowData{}) + + compiler.addActivationVersionCheckStep(ctx) + + assert.Empty(t, ctx.steps) + }) +} + +func TestActivationStepsAddSkillInstallSteps(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("adds skill install steps", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + EngineConfig: &EngineConfig{ID: "claude"}, + Skills: []string{ + "githubnext/skills@deadbeef", + }, + }) + + err := compiler.addActivationSkillInstallSteps(ctx) + + require.NoError(t, err) + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Upgrade gh CLI for frontmatter skills") + assert.Contains(t, steps, "Install frontmatter skill 1") + assert.Contains(t, steps, "GH_AW_INFO_ENGINE_ID: \"claude\"") + assert.Contains(t, steps, "GH_AW_GH_SKILL_AGENT_NAME: \"claude-code\"") + assert.Contains(t, steps, "GH_AW_FRONTMATTER_SKILLS: \"githubnext/skills@deadbeef\"") + assert.Contains(t, steps, "collect_skill_install_failures.cjs") + assert.Equal(t, "${{ steps.collect-skill-install-failures.outputs.failure_count || '0' }}", ctx.outputs["skill_install_failure_count"]) + assert.Equal(t, "${{ steps.collect-skill-install-failures.outputs.errors || '' }}", ctx.outputs["skill_install_errors"]) + }) + + t.Run("skips when no skills are configured", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{}) + + err := compiler.addActivationSkillInstallSteps(ctx) + + require.NoError(t, err) + assert.Empty(t, ctx.steps) + assert.Empty(t, ctx.outputs) + }) +} + +func TestActivationStepsAddTextOutputStep(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("adds text output step", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + NeedsTextOutput: true, + Bots: []string{"dependabot[bot]"}, + EngineConfig: &EngineConfig{ID: "copilot"}, + SafeOutputs: &SafeOutputsConfig{ + AllowedDomains: []string{"docs.example.com"}, + }, + }) + + err := compiler.addActivationTextOutputStep(ctx) + + require.NoError(t, err) + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Compute current body text") + assert.Contains(t, steps, "id: sanitized") + assert.Contains(t, steps, "GH_AW_ALLOWED_BOTS: \"dependabot[bot]\"") + assert.Contains(t, steps, "GH_AW_ALLOWED_DOMAINS:") + assert.Contains(t, steps, "docs.example.com") + assert.Contains(t, steps, "github.com") + assert.Contains(t, steps, "localhost") + assert.Contains(t, steps, "compute_text.cjs") + assert.Equal(t, "${{ steps.sanitized.outputs.text }}", ctx.outputs["text"]) + assert.Equal(t, "${{ steps.sanitized.outputs.title }}", ctx.outputs["title"]) + assert.Equal(t, "${{ steps.sanitized.outputs.body }}", ctx.outputs["body"]) + }) + + t.Run("skips when text output is not needed", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{}) + + err := compiler.addActivationTextOutputStep(ctx) + + require.NoError(t, err) + assert.Empty(t, ctx.steps) + }) + + t.Run("returns sanitization errors", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + NeedsTextOutput: true, + Model: "/bad-provider", + EngineConfig: &EngineConfig{ + ID: "opencode", + }, + }) + + err := compiler.addActivationTextOutputStep(ctx) + + require.Error(t, err) + }) +} + +func TestActivationStepsComputeActivationSanitizationDomains(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("uses expanded allowed domains when configured", func(t *testing.T) { + domains, err := compiler.computeActivationSanitizationDomains(&WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot"}, + SafeOutputs: &SafeOutputsConfig{ + AllowedDomains: []string{"docs.example.com"}, + }, + }) + + require.NoError(t, err) + assert.Contains(t, domains, "docs.example.com") + assert.Contains(t, domains, "github.com") + assert.Contains(t, domains, "localhost") + }) + + t.Run("uses base allowed domains when safe outputs are absent", func(t *testing.T) { + domains, err := compiler.computeActivationSanitizationDomains(&WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot"}, + }) + + require.NoError(t, err) + assert.Contains(t, domains, "api.github.com") + }) + + t.Run("returns errors for malformed models", func(t *testing.T) { + _, err := compiler.computeActivationSanitizationDomains(&WorkflowData{ + Model: "/bad-provider", + EngineConfig: &EngineConfig{ + ID: "opencode", + }, + }) + + require.Error(t, err) + }) +} + +func TestActivationStepsAddStatusCommentStep(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("adds status comment step", func(t *testing.T) { + statusComment := true + ctx := newActivationStepsTestContext(&WorkflowData{ + Name: "test-workflow", + StatusComment: &statusComment, + TrackerID: "tracker-1234", + LockForAgent: true, + SafeOutputs: &SafeOutputsConfig{ + Messages: &SafeOutputMessagesConfig{ + RunStarted: "started", + }, + }, + }) + ctx.statusCommentIssues = true + + err := compiler.addActivationStatusCommentStep(ctx) + + require.NoError(t, err) + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Add comment with workflow run link") + assert.Contains(t, steps, "id: add-comment") + assert.Contains(t, steps, "GH_AW_WORKFLOW_NAME: \"test-workflow\"") + assert.Contains(t, steps, "GH_AW_TRACKER_ID: \"tracker-1234\"") + assert.Contains(t, steps, "GH_AW_LOCK_FOR_AGENT: \"true\"") + assert.Contains(t, steps, "GH_AW_SAFE_OUTPUT_MESSAGES:") + assert.Contains(t, steps, "add_workflow_run_comment.cjs") + assert.NotContains(t, steps, "github-token:") + assert.Equal(t, "${{ steps.add-comment.outputs.comment-id }}", ctx.outputs["comment_id"]) + assert.Equal(t, "${{ steps.add-comment.outputs.comment-url }}", ctx.outputs["comment_url"]) + assert.Equal(t, "${{ steps.add-comment.outputs.comment-repo }}", ctx.outputs["comment_repo"]) + }) + + t.Run("skips when status comments are disabled", func(t *testing.T) { + statusComment := false + ctx := newActivationStepsTestContext(&WorkflowData{ + StatusComment: &statusComment, + }) + + err := compiler.addActivationStatusCommentStep(ctx) + + require.NoError(t, err) + assert.Empty(t, ctx.steps) + }) +} + +func TestActivationStepsAddSafeOutputMessagesEnv(t *testing.T) { + t.Run("adds serialized messages env", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + SafeOutputs: &SafeOutputsConfig{ + Messages: &SafeOutputMessagesConfig{ + RunFailure: "failed", + }, + }, + }) + + err := addActivationSafeOutputMessagesEnv(ctx) + + require.NoError(t, err) + assert.Contains(t, strings.Join(ctx.steps, ""), "GH_AW_SAFE_OUTPUT_MESSAGES:") + }) + + t.Run("skips when messages are absent", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{}) + + err := addActivationSafeOutputMessagesEnv(ctx) + + require.NoError(t, err) + assert.Empty(t, ctx.steps) + }) +} + +func TestActivationStepsAddIssueLockStep(t *testing.T) { + compiler := newActivationStepsTestCompiler("") + + t.Run("adds issue lock step", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{ + LockForAgent: true, + AIReaction: "eyes", + }) + + compiler.addActivationIssueLockStep(ctx) + + steps := strings.Join(ctx.steps, "") + assert.Contains(t, steps, "Lock issue for agentic workflow") + assert.Contains(t, steps, "id: lock-issue") + assert.Contains(t, steps, "lock-issue.cjs") + assert.Equal(t, "${{ steps.lock-issue.outputs.locked }}", ctx.outputs["issue_locked"]) + }) + + t.Run("skips when lock-for-agent is disabled", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{}) + + compiler.addActivationIssueLockStep(ctx) + + assert.Empty(t, ctx.steps) + }) +} + +func TestActivationStepsEnsureActivationCommentOutputs(t *testing.T) { + t.Run("adds missing outputs", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{}) + + ensureActivationCommentOutputs(ctx) + + assert.Equal(t, `""`, ctx.outputs["comment_id"]) + assert.Equal(t, `""`, ctx.outputs["comment_repo"]) + }) + + t.Run("preserves existing outputs", func(t *testing.T) { + ctx := newActivationStepsTestContext(&WorkflowData{}) + ctx.outputs["comment_id"] = "existing-id" + ctx.outputs["comment_repo"] = "existing-repo" + + ensureActivationCommentOutputs(ctx) + + assert.Equal(t, "existing-id", ctx.outputs["comment_id"]) + assert.Equal(t, "existing-repo", ctx.outputs["comment_repo"]) + }) +} From 90380f131966c36d80faf1532ec3a3c7954136f7 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 2 Aug 2026 15:20:11 +0000 Subject: [PATCH 3/4] docs(adr): add draft ADR-49800 for dedicated activation step unit tests Co-Authored-By: Claude Sonnet 4.6 --- ...-unit-tests-for-activation-step-helpers.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 docs/adr/49800-add-dedicated-unit-tests-for-activation-step-helpers.md diff --git a/docs/adr/49800-add-dedicated-unit-tests-for-activation-step-helpers.md b/docs/adr/49800-add-dedicated-unit-tests-for-activation-step-helpers.md new file mode 100644 index 00000000000..0214f4ad579 --- /dev/null +++ b/docs/adr/49800-add-dedicated-unit-tests-for-activation-step-helpers.md @@ -0,0 +1,44 @@ +# ADR-49800: Add Dedicated Unit Test File for Activation Step Helpers + +**Date**: 2026-08-02 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +`pkg/workflow/compiler_activation_steps.go` contains a dense set of activation-job helper methods — including reaction steps, OAuth token checks, lock-file checks, version checks, skill installs, text-output setup, and status comment wiring — many of which return errors. Despite this complexity, none of these helpers had focused unit tests; their behavior was exercised only incidentally through broader compiler-level integration tests. This made it difficult to pinpoint regressions in individual helpers and left error-returning paths (such as sanitization domain computation failures) without direct coverage. + +### Decision + +We will add a dedicated unit test file (`pkg/workflow/compiler_activation_steps_test.go`) that tests each activation step helper in isolation, using lightweight `activationJobBuildContext` instances constructed directly rather than going through full workflow compilation. Each helper's success path and primary error/skip path will be covered by a focused table-driven or sub-test function. + +### Alternatives Considered + +#### Alternative 1: Rely solely on existing broad compiler tests + +Keep the status quo and cover activation step behavior only through higher-level compiler tests that exercise the full compilation pipeline. This avoids adding a new test file but provides poor regression signal: a failure in any activation helper surfaces as a broad compiler test failure with no indication of which helper broke. Error-returning paths deep in individual helpers are impractical to trigger through the compilation surface. + +#### Alternative 2: Add integration-style tests that compile full workflows + +Write tests that call `Compiler.Compile(...)` end-to-end with fixture workflow data, then assert on the generated YAML. This would exercise the real pipeline but requires constructing valid full-workflow inputs for every helper scenario, making tests verbose and slow. It also makes it harder to isolate a specific helper's behavior when a test fails. + +### Consequences + +#### Positive +- Focused tests that directly exercise each helper make regressions in individual methods easy to identify without reading through a full compilation diff. +- Error-returning paths (e.g., malformed model causing sanitization failure) are now explicitly covered, reducing the risk of silent breakage. +- Tests run without a full compilation pass, keeping the unit-test suite fast. + +#### Negative +- Adds 508 lines of test code that must be maintained alongside `compiler_activation_steps.go`; renaming helpers or changing their signatures requires updating both files. +- The test helpers (`newActivationStepsTestCompiler`, `newActivationStepsTestContext`) partially duplicate the construction logic of production build contexts and may drift if the context struct evolves. + +#### Neutral +- Tests are tagged `//go:build !integration` so they are excluded from integration test runs, consistent with the existing test build tag convention in this package. +- The new file lives in the same `workflow` package (not `workflow_test`), giving it access to unexported types such as `activationJobBuildContext`. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 920a48cbd26c1bd0fbbf94ba9453c2b5ded5ccb8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 2 Aug 2026 16:09:55 +0000 Subject: [PATCH 4/4] fix: address review feedback on activation step helpers and unit tests - Fix CI failure: update model from 'small' to 'claude-haiku-4.5' in pr-code-quality-reviewer.md (TestPRCodeQualityReviewerWorkflowSubAgentModelContract) - Remove unreachable error return from addActivationSkillInstallSteps (always returned nil) - Remove unreachable error return from addActivationSafeOutputMessagesEnv (json.Marshal cannot fail on SafeOutputMessagesConfig which contains only strings and bools) - Update addActivationStatusCommentStep caller to not error-check removed return - Update compiler_activation_job.go to call addActivationSkillInstallSteps without error check - Strengthen test assertions: check serialized message value (not just key) in TestActivationStepsAddSafeOutputMessagesEnv and TestActivationStepsAddStatusCommentStep - Update tests to match new non-error function signatures for skill install and messages env - Complete ADR-49800: change status from Draft to Accepted, update consequences to reflect the unreachable error returns that were removed during focused test authoring Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../pr-code-quality-reviewer.lock.yml | 2 +- .github/workflows/pr-code-quality-reviewer.md | 2 +- ...-unit-tests-for-activation-step-helpers.md | 9 ++++---- pkg/workflow/compiler_activation_job.go | 4 +--- pkg/workflow/compiler_activation_steps.go | 22 +++++++------------ .../compiler_activation_steps_test.go | 19 ++++++++-------- 6 files changed, 25 insertions(+), 33 deletions(-) diff --git a/.github/workflows/pr-code-quality-reviewer.lock.yml b/.github/workflows/pr-code-quality-reviewer.lock.yml index a4094776a21..91b17b89fa8 100644 --- a/.github/workflows/pr-code-quality-reviewer.lock.yml +++ b/.github/workflows/pr-code-quality-reviewer.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"1915423b7590fd2377cb225f67fe7d9de87e69bc461aca7e160c1470fa845af1","body_hash":"d66144ce2443a9def8e7c4286113d8fb5c2fbac95fe62f367ef8cb668b9527c9","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.77","copilot-sdk":"1.0.8"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"1915423b7590fd2377cb225f67fe7d9de87e69bc461aca7e160c1470fa845af1","body_hash":"23fad5f911121efe38aabc755ccc0804905411b87387d1565ffd987fb1837fcd","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.77","copilot-sdk":"1.0.8"}} # gh-aw-manifest: {"version":1,"secrets":["COPILOT_GITHUB_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"3d3c42e5aac5ba805825da76410c181273ba90b1","version":"v7.0.1"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"820762786026740c76f36085b0efc47a31fe5020","version":"v7.0.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.43","digest":"sha256:04e2d1987a565000a8f114b89d806ae7a3864dd4f944be65275b28c93d8690e6","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.43@sha256:04e2d1987a565000a8f114b89d806ae7a3864dd4f944be65275b28c93d8690e6"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.43","digest":"sha256:d85f57975af5ea23af4996e41ed73fbc8f5b4a47402472bfe82e508f352cb0c1","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.43@sha256:d85f57975af5ea23af4996e41ed73fbc8f5b4a47402472bfe82e508f352cb0c1"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.43","digest":"sha256:65c45ea2967984d0024f3df61bc71335658a77ede96c8d9665da7a5f33a795ab","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.43@sha256:65c45ea2967984d0024f3df61bc71335658a77ede96c8d9665da7a5f33a795ab"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.43","digest":"sha256:26be5e0b8c8f4c41c8a59126b29bb5d80b07253597472ded2a16bdd75abcbf9d","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.43@sha256:26be5e0b8c8f4c41c8a59126b29bb5d80b07253597472ded2a16bdd75abcbf9d"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.7","digest":"sha256:7545220a9aca134b71e51193ee0eaf4c50756ebf8fbd25a63ae7556e62815c00","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.7@sha256:7545220a9aca134b71e51193ee0eaf4c50756ebf8fbd25a63ae7556e62815c00"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748","pinned_image":"ghcr.io/github/gh-aw-node@sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748"},{"image":"ghcr.io/github/github-mcp-server:v1.8.0","digest":"sha256:d5a18c04b92714c309eb46a2305087e91a4dbd80420f6e462656699f95093520","pinned_image":"ghcr.io/github/github-mcp-server:v1.8.0@sha256:d5a18c04b92714c309eb46a2305087e91a4dbd80420f6e462656699f95093520"}],"has_pull_request":true} # This file was automatically generated by gh-aw. DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # diff --git a/.github/workflows/pr-code-quality-reviewer.md b/.github/workflows/pr-code-quality-reviewer.md index 29349af0913..d3597db76cb 100644 --- a/.github/workflows/pr-code-quality-reviewer.md +++ b/.github/workflows/pr-code-quality-reviewer.md @@ -181,7 +181,7 @@ Use `COMMENT` when all findings are non-blocking. Keep the overall review body c ## agent: `grumpy-coder` --- description: Hyper-critical senior reviewer that aggressively finds merge-blocking issues in changed lines -model: small +model: claude-haiku-4.5 --- You are a grumpy senior engineer doing a hostile first-pass code review. diff --git a/docs/adr/49800-add-dedicated-unit-tests-for-activation-step-helpers.md b/docs/adr/49800-add-dedicated-unit-tests-for-activation-step-helpers.md index 0214f4ad579..f70d3fb9c2e 100644 --- a/docs/adr/49800-add-dedicated-unit-tests-for-activation-step-helpers.md +++ b/docs/adr/49800-add-dedicated-unit-tests-for-activation-step-helpers.md @@ -1,8 +1,8 @@ # ADR-49800: Add Dedicated Unit Test File for Activation Step Helpers **Date**: 2026-08-02 -**Status**: Draft -**Deciders**: Unknown +**Status**: Accepted +**Deciders**: Copilot --- @@ -30,9 +30,10 @@ Write tests that call `Compiler.Compile(...)` end-to-end with fixture workflow d - Focused tests that directly exercise each helper make regressions in individual methods easy to identify without reading through a full compilation diff. - Error-returning paths (e.g., malformed model causing sanitization failure) are now explicitly covered, reducing the risk of silent breakage. - Tests run without a full compilation pass, keeping the unit-test suite fast. +- Writing focused tests surfaced two helpers (`addActivationSkillInstallSteps`, `addActivationSafeOutputMessagesEnv`) whose `error` return types were unreachable; both were removed, simplifying callers and eliminating misleading dead-code paths. #### Negative -- Adds 508 lines of test code that must be maintained alongside `compiler_activation_steps.go`; renaming helpers or changing their signatures requires updating both files. +- Adds test code that must be maintained alongside `compiler_activation_steps.go`; renaming helpers or changing their signatures requires updating both files. - The test helpers (`newActivationStepsTestCompiler`, `newActivationStepsTestContext`) partially duplicate the construction logic of production build contexts and may drift if the context struct evolves. #### Neutral @@ -41,4 +42,4 @@ Write tests that call `Compiler.Compile(...)` end-to-end with fixture workflow d --- -*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* +*ADR reviewed and accepted as part of PR #49800.* diff --git a/pkg/workflow/compiler_activation_job.go b/pkg/workflow/compiler_activation_job.go index fefe91d6558..504b9a95cbd 100644 --- a/pkg/workflow/compiler_activation_job.go +++ b/pkg/workflow/compiler_activation_job.go @@ -49,9 +49,7 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate if err := c.addActivationRepositoryAndOutputSteps(ctx); err != nil { return nil, fmt.Errorf("failed to add activation repository and output steps: %w", err) } - if err := c.addActivationSkillInstallSteps(ctx); err != nil { - return nil, fmt.Errorf("failed to add skill install steps: %w", err) - } + c.addActivationSkillInstallSteps(ctx) if err := c.addActivationCommandAndLabelOutputs(ctx); err != nil { return nil, fmt.Errorf("failed to add activation command and label outputs: %w", err) } diff --git a/pkg/workflow/compiler_activation_steps.go b/pkg/workflow/compiler_activation_steps.go index 97ccd9f9d89..18032b918ec 100644 --- a/pkg/workflow/compiler_activation_steps.go +++ b/pkg/workflow/compiler_activation_steps.go @@ -170,7 +170,7 @@ func (c *Compiler) addActivationVersionCheckStep(ctx *activationJobBuildContext) ctx.steps = append(ctx.steps, generateGitHubScriptWithRequire("check_version_updates.cjs")) } -func (c *Compiler) addActivationSkillInstallSteps(ctx *activationJobBuildContext) error { +func (c *Compiler) addActivationSkillInstallSteps(ctx *activationJobBuildContext) { skillRefs := append([]SkillReference(nil), ctx.data.SkillReferences...) if len(skillRefs) == 0 && len(ctx.data.Skills) > 0 { skillRefs = make([]SkillReference, 0, len(ctx.data.Skills)) @@ -182,7 +182,7 @@ func (c *Compiler) addActivationSkillInstallSteps(ctx *activationJobBuildContext } } if len(skillRefs) == 0 { - return nil + return } engineID := resolveActivationEngineID(ctx.data) @@ -243,8 +243,6 @@ func (c *Compiler) addActivationSkillInstallSteps(ctx *activationJobBuildContext ctx.outputs["skill_install_failure_count"] = "${{ steps.collect-skill-install-failures.outputs.failure_count || '0' }}" ctx.outputs["skill_install_errors"] = "${{ steps.collect-skill-install-failures.outputs.errors || '' }}" - - return nil } func (c *Compiler) addActivationTextOutputStep(ctx *activationJobBuildContext) error { @@ -312,9 +310,7 @@ func (c *Compiler) addActivationStatusCommentStep(ctx *activationJobBuildContext if ctx.data.LockForAgent { ctx.steps = append(ctx.steps, " GH_AW_LOCK_FOR_AGENT: \"true\"\n") } - if err := addActivationSafeOutputMessagesEnv(ctx); err != nil { - return err - } + addActivationSafeOutputMessagesEnv(ctx) ctx.steps = append(ctx.steps, " with:\n") commentToken := c.resolveActivationToken(ctx.data) if commentToken != "${{ secrets.GITHUB_TOKEN }}" { @@ -328,18 +324,16 @@ func (c *Compiler) addActivationStatusCommentStep(ctx *activationJobBuildContext return nil } -func addActivationSafeOutputMessagesEnv(ctx *activationJobBuildContext) error { +func addActivationSafeOutputMessagesEnv(ctx *activationJobBuildContext) { if ctx.data.SafeOutputs == nil || ctx.data.SafeOutputs.Messages == nil { - return nil - } - messagesJSON, err := serializeMessagesConfig(ctx.data.SafeOutputs.Messages) - if err != nil { - return fmt.Errorf("failed to serialize messages config for activation job: %w", err) + return } + // serializeMessagesConfig uses json.Marshal on a struct containing only strings and bools, + // so it cannot fail in practice; the error is intentionally ignored here. + messagesJSON, _ := serializeMessagesConfig(ctx.data.SafeOutputs.Messages) if messagesJSON != "" { ctx.steps = append(ctx.steps, fmt.Sprintf(" GH_AW_SAFE_OUTPUT_MESSAGES: %q\n", messagesJSON)) } - return nil } func (c *Compiler) addActivationIssueLockStep(ctx *activationJobBuildContext) { diff --git a/pkg/workflow/compiler_activation_steps_test.go b/pkg/workflow/compiler_activation_steps_test.go index 76b531aeec1..ac9ce9fef85 100644 --- a/pkg/workflow/compiler_activation_steps_test.go +++ b/pkg/workflow/compiler_activation_steps_test.go @@ -267,9 +267,8 @@ func TestActivationStepsAddSkillInstallSteps(t *testing.T) { }, }) - err := compiler.addActivationSkillInstallSteps(ctx) + compiler.addActivationSkillInstallSteps(ctx) - require.NoError(t, err) steps := strings.Join(ctx.steps, "") assert.Contains(t, steps, "Upgrade gh CLI for frontmatter skills") assert.Contains(t, steps, "Install frontmatter skill 1") @@ -284,9 +283,8 @@ func TestActivationStepsAddSkillInstallSteps(t *testing.T) { t.Run("skips when no skills are configured", func(t *testing.T) { ctx := newActivationStepsTestContext(&WorkflowData{}) - err := compiler.addActivationSkillInstallSteps(ctx) + compiler.addActivationSkillInstallSteps(ctx) - require.NoError(t, err) assert.Empty(t, ctx.steps) assert.Empty(t, ctx.outputs) }) @@ -412,6 +410,7 @@ func TestActivationStepsAddStatusCommentStep(t *testing.T) { assert.Contains(t, steps, "GH_AW_TRACKER_ID: \"tracker-1234\"") assert.Contains(t, steps, "GH_AW_LOCK_FOR_AGENT: \"true\"") assert.Contains(t, steps, "GH_AW_SAFE_OUTPUT_MESSAGES:") + assert.Contains(t, steps, "started") assert.Contains(t, steps, "add_workflow_run_comment.cjs") assert.NotContains(t, steps, "github-token:") assert.Equal(t, "${{ steps.add-comment.outputs.comment-id }}", ctx.outputs["comment_id"]) @@ -433,7 +432,7 @@ func TestActivationStepsAddStatusCommentStep(t *testing.T) { } func TestActivationStepsAddSafeOutputMessagesEnv(t *testing.T) { - t.Run("adds serialized messages env", func(t *testing.T) { + t.Run("adds serialized messages env with message value", func(t *testing.T) { ctx := newActivationStepsTestContext(&WorkflowData{ SafeOutputs: &SafeOutputsConfig{ Messages: &SafeOutputMessagesConfig{ @@ -442,18 +441,18 @@ func TestActivationStepsAddSafeOutputMessagesEnv(t *testing.T) { }, }) - err := addActivationSafeOutputMessagesEnv(ctx) + addActivationSafeOutputMessagesEnv(ctx) - require.NoError(t, err) - assert.Contains(t, strings.Join(ctx.steps, ""), "GH_AW_SAFE_OUTPUT_MESSAGES:") + combined := strings.Join(ctx.steps, "") + assert.Contains(t, combined, "GH_AW_SAFE_OUTPUT_MESSAGES:") + assert.Contains(t, combined, "failed") }) t.Run("skips when messages are absent", func(t *testing.T) { ctx := newActivationStepsTestContext(&WorkflowData{}) - err := addActivationSafeOutputMessagesEnv(ctx) + addActivationSafeOutputMessagesEnv(ctx) - require.NoError(t, err) assert.Empty(t, ctx.steps) }) }