Add focused unit coverage for pre-activation job compiler helpers

[Copilot]

compiler_pre_activation_job.go was the only large compiler module without a dedicated test file, leaving core pre-activation assembly logic under-covered. This PR adds focused unit tests for the highest-value helper paths used to build job structure, guards, and permissions.

• New dedicated test file

  • Added pkg/workflow/compiler_pre_activation_job_test.go to isolate pre-activation helper coverage from broader integration-heavy compiler tests.

• buildPreActivationPermissions coverage

  • Added scenarios for:
    • empty permissions in release mode when no optional permission sources apply
    • merged/inferred permissions in script mode (contents: read, actions: read, pull-requests: read) plus explicit on.permissions merge behavior.

• applyPreActivationIfConditionGuards coverage

  • Added assertions for guard composition behavior:
    • label guard + comment author_association guard when eligible
    • suppression of static comment guard when bot entries contain expressions
    • inclusion of skip-author-associations guard clauses.

• buildPreActivationJob coverage

  • Added end-to-end helper-level assertions for:
    • composed if condition (base if + label guard + author guard)
    • deduplicated on.needs
    • output wiring from on.steps (<id>_result)
    • always-declared matched_command output defaulting to empty for non-command workflows.

job, err := c.buildPreActivationJob(data, true)
require.NoError(t, err)

assert.Contains(t, job.If, "github.event.label == null")
assert.Contains(t, job.If, "author_association")
assert.Equal(t, []string{"prepare"}, job.Needs)
assert.Contains(t, job.Outputs, "gate_result")

• Fixes [Code Quality] Add test file for compiler_pre_activation_job.go (40 functions, 956 lines, zero tests) #48381

[github-actions]

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

[github-actions]

❌ Design Decision Gate 🏗️ failed during design decision gate check.

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[Copilot]

Pull request overview

Adds focused unit tests for pre-activation job assembly and guard behavior.

Changes:

• Tests permissions, condition guards, needs deduplication, and outputs.
• Updates generated model-cost metadata in an unrelated workflow lock file.

Show a summary per file

File	Description
pkg/workflow/compiler_pre_activation_job_test.go	Adds focused pre-activation compiler tests.
.github/workflows/smoke-copilot-auto.lock.yml	Adds unrelated model-cost metadata.

Review details

Tip

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

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 85/100 — Excellent

Analyzed 6 test(s): 6 design, 0 implementation, 0 violation(s).

📊 Metrics (6 tests)

Metric	Value
Analyzed	6 (Go: 6, JS: 0)
✅ Design	6 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	3 (50%)
Duplicate clusters	0
Inflation	NO
🚨 Violations	0

Test	File	Classification	Issues
TestBuildPreActivationPermissions/release mode without optional checks	compiler_pre_activation_job_test.go:13	design_test	None
TestBuildPreActivationPermissions/script mode merges inferred and explicit permissions	compiler_pre_activation_job_test.go:23	design_test	None
TestApplyPreActivationIfConditionGuards/combines label and comment-author guards	compiler_pre_activation_job_test.go:53	design_test	None
TestApplyPreActivationIfConditionGuards/does not add comment-author guard for expression-based bot list	compiler_pre_activation_job_test.go:67	design_test, edge case	None
TestApplyPreActivationIfConditionGuards/adds skip-author-associations guard	compiler_pre_activation_job_test.go:79	design_test	None
TestBuildPreActivationJob	compiler_pre_activation_job_test.go:90	design_test, error coverage	None

Advisory (not a violation)

Several assert.Contains / assert.NotContains calls lack descriptive failure context. Adding a message argument (e.g., assert.Contains(t, result, "author_association", "expected author_association guard in if-condition")) would improve debuggability. This is advisory only and does not affect the score.

Verdict

✅ passed. 0% implementation tests (threshold: 30%). All 6 tests verify behavioral contracts: permissions merging, if-condition guard composition, deduplication of needs, and full job structure. Build tag //go:build !integration present. No mock library usage detected.

🧪 Test quality analysis by Test Quality Sentinel · sonnet46 · 31.3 AIC · ⌖ 7.23 AIC · ⊞ 8.1K · ◷
_{Comment /review to run again}

[github-actions]

The new test file is well-structured and targets high-value paths in compiler_pre_activation_job.go. All assertions are accurate against the production code. No issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.7 AIC · ⌖ 5.16 AIC · ⊞ 5K

[github-actions]

✅ Test Quality Sentinel: 85/100. 0% implementation tests (threshold: 30%).

[github-actions]

Skills-Based Review 🧠

Applied /tdd — two minor suggestions, no blocking issues.

📋 Key Themes & Highlights

Key Themes

• TestBuildPreActivationJob is monolithic: multiple independent behaviours are asserted in one flat function. When any assertion fails, the test output doesn't pinpoint which specification broke. Splitting into t.Run blocks is the standard Go idiom and aligns with red-green-refactor.
• Dev-mode permissions gap: buildPreActivationPermissions has explicit release and script mode sub-tests but the IsDev() branch of needsContentsRead is never directly asserted. It's exercised indirectly by TestBuildPreActivationJob but without a permissions assertion, so a regression there would go undetected.

Positive Highlights

• ✅ Excellent coverage of the three helpers that were previously completely untested.
• ✅ Guard-composition tests (label, expression-bot, skip-author-associations) cleanly exercise the branching logic in applyPreActivationIfConditionGuards.
• ✅ needs deduplication and matched_command default-output assertions are exactly the kind of assembly seam that prevents silent regressions.
• ✅ Good use of require.NoError / require.NotNil to fail fast before downstream assertions.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 33.2 AIC · ⌖ 4.87 AIC · ⊞ 6.7K
_{Comment /matt to run again}

[github-actions]

[/tdd] TestBuildPreActivationJob is a single flat function testing multiple independent behaviours — when one assertion fails it's unclear which behaviour broke.

💡 Split into focused sub-tests

Each t.Run block names exactly one behaviour and can go red independently:

func TestBuildPreActivationJob(t *testing.T) {
	t.Run("composes if from base-if + label guard + author guard", func(t *testing.T) { ... })
	t.Run("deduplicates on.needs", func(t *testing.T) { ... })
	t.Run("wires on.steps outputs as <id>_result", func(t *testing.T) { ... })
	t.Run("always declares matched_command output", func(t *testing.T) { ... })
}

This follows the red-green-refactor loop: each sub-test is a specification.

@copilot please address this.

[github-actions]

[/tdd] Missing boundary case: buildPreActivationPermissions in dev mode is never tested directly — the TestBuildPreActivationJob uses ActionModeDev but doesn't assert permission content, so the needsContentsRead branch for dev mode has no explicit coverage.

💡 Add a dev-mode sub-test

t.Run("dev mode infers contents:read", func(t *testing.T) {
    c := NewCompiler()
    c.SetActionMode(ActionModeDev)
    _, permissions := c.buildPreActivationPermissions(&WorkflowData{Name: "wf"}, "./actions/setup")
    assert.Contains(t, permissions, "contents: read")
})

Without this, the needsContentsRead branch for IsDev() is only exercised by TestBuildPreActivationJob which doesn't check permissions at all.

@copilot please address this.

[github-actions]

Verdict: COMMENT

Solid, well-targeted test coverage for previously untested pre-activation compiler helpers -- no blocking correctness or security issues found, but a few test-quality nits should be tightened.

💡 Themes

• Duplicate assertion in TestBuildPreActivationJob: assert.Equal(t, []string{"prepare"}, job.Needs) appears twice back-to-back -- likely copy-paste, adds no coverage.
• Weak substring assertions on permissions: both permission tests use assert.Contains against rendered YAML rather than validating the exact permission set. Since these helpers directly control the workflow's GitHub token scopes, tests should pin the full expected permission set (exact match) rather than allow silent scope creep to pass unnoticed.
• Unrelated lock file drift: .github/workflows/smoke-copilot-auto.lock.yml picks up a GH_AW_INFO_MODEL_COSTS line that traces to unrelated model-pricing work in a separate recent commit (#48482), not to this PR's test-only scope. Not a functional problem, but worth separating out for cleaner history.
• Test coverage itself is well-scoped (release vs script mode permissions, guard composition, expression-based bot list edge case, full job assembly) and follows existing repo test conventions.

🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 67.8 AIC · ⌖ 4.58 AIC · ⊞ 7.4K
_{Comment /review to run again}

Comments that could not be inline-anchored

pkg/workflow/compiler_pre_activation_job_test.go:145

Duplicate assertion: assert.Equal(t, []string{"prepare"}, job.Needs) is asserted twice back-to-back.

<details><summary>💡 Details</summary>

Lines 144-145 both assert job.Needs equals []string{"prepare"}. The second call is dead weight -- likely a copy-paste leftover -- and provides zero additional coverage. If the intent was to also verify dedup behavior more thoroughly (e.g. ordering when duplicates appear at different positions), that intent was lost; as written it just repeats the sa…

pkg/workflow/compiler_pre_activation_job_test.go:39

Permission assertions use loose substring Contains on rendered YAML instead of validating the exact permission set, which is risky since job permissions are a security-relevant surface.

<details><summary>💡 Details</summary>

The test only checks that the rendered permissions string contains "actions: read", "contents: read", "issues: write", "pull-requests: read" via assert.Contains. This does not assert there are exactly these four scopes and no more -- a regression that accident…

.github/workflows/smoke-copilot-auto.lock.yml:152

This lock file change is unrelated recompile drift from a prior commit and doesn't belong in a test-only PR.

<details><summary>💡 Details</summary>

The added GH_AW_INFO_MODEL_COSTS line traces back to model-pricing logic (compiler_model_pricing.go / compiler_yaml_step_lifecycle.go) unrelated to pre-activation job test coverage, and matches drift from a separate recent commit ("Fix imported engine.model silently dropped when max-ai-credits is set", #48482). Bundling incidental `make reco…

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Done — reverted the unrelated GH_AW_INFO_MODEL_COSTS drift from smoke-copilot-auto.lock.yml in the latest commit. The PR now contains only the pre-activation test file and the ADR.