Log first trigger reason for agentics-maintenance generation

[Copilot]

The compiler generated agentics-maintenance.yml without clearly stating which workflow condition required it. This change makes maintenance generation causality explicit in logs by recording and surfacing the first triggering condition.

• Trigger-cause visibility

  • Extended maintenance trigger scanning to return a third value: triggerReason.
  • Captures the first condition that flips maintenance generation on (e.g., create_issues.expires, create_discussions.expires, create_pull_requests.expires, or no-op issue reporting).

• Generation-time logging

  • Added a dedicated log line before generation:
    • Maintenance workflow generation triggered: <reason>
  • Preserves existing per-workflow expires diagnostics while adding a single high-signal cause line for quick triage.

• Behavior contract coverage

  • Added focused unit coverage for:
    • no-trigger case (triggerReason == "")
    • first-trigger-wins semantics (does not get overwritten by later triggers)

hasExpires, minExpires, triggerReason := scanWorkflowsForExpires(workflowDataList)
if hasExpires {
    maintenanceLog.Printf("Maintenance workflow generation triggered: %s", triggerReason)
}

[Copilot]

Pull request overview

This PR improves observability for agentics-maintenance.yml generation by capturing and surfacing the first workflow condition that triggered maintenance generation, making the causality explicit in logs for faster triage.

Changes:

• Extended scanWorkflowsForExpires to return a third value, triggerReason, representing the first trigger encountered.
• Added a generation-time log line in GenerateMaintenanceWorkflow to print the trigger reason when maintenance generation occurs.
• Added unit tests covering the no-trigger case and “first trigger wins” semantics.

Show a summary per file

File	Description
pkg/workflow/maintenance_workflow.go	Returns and logs a “first trigger reason” for maintenance workflow generation.
pkg/workflow/maintenance_workflow_test.go	Adds unit coverage for triggerReason behavior (empty vs. first-trigger-wins).

Copilot's findings

Tip

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

• Files reviewed: 2/2 changed files
• Comments generated: 1

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

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

No ADR enforcement needed: PR #38616 does not have the implementation label (has_implementation_label=false) and has 58 new lines of code in business logic directories, which is below the 100-line threshold (requires_adr_by_default_volume=false).

[github-actions]

🧪 Test Quality Sentinel completed test quality analysis.

[github-actions]

🧠 Matt Pocock Skills Reviewer was skipped during the skills-based review.

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 90/100 — Excellent

Analyzed 2 test scenario(s) across 1 test function: 2 design tests, 0 implementation tests, 0 hard guideline violations (1 soft warning: missing assertion messages).

📊 Metrics & Test Classification (2 test scenarios analyzed)

Metric	Value
New/modified tests analyzed	2 sub-tests (t.Run) in 1 function
✅ 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 — 40 test lines / 18 production lines = 2.22× (threshold: 2×)
🚨 Coding-guideline violations	0 (no mock libraries; build tag //go:build !integration present)

Test Classification Details

Test	File	Classification	Issues Detected
TestScanWorkflowsForExpires_TriggerReason/"no triggers"	pkg/workflow/maintenance_workflow_test.go	✅ Design	Missing assertion messages
TestScanWorkflowsForExpires_TriggerReason/"captures first trigger reason"	pkg/workflow/maintenance_workflow_test.go	✅ Design	Missing assertion messages

Language Support

Tests analyzed:

• 🐹 Go (*_test.go): 2 sub-tests — unit (//go:build !integration)

⚠️ Soft Warnings — Does Not Fail Check (1 issue)

⚠️ Missing assertion messages (pkg/workflow/maintenance_workflow_test.go)

Issue: All 8 require.* calls in TestScanWorkflowsForExpires_TriggerReason lack a descriptive message argument. The project guideline is that every testify assertion should carry a message explaining what failed.

Examples of bare assertions found:

require.False(t, hasExpires)                           // ← no message
require.Equal(t, 0, minExpires)                        // ← no message
require.Contains(t, triggerReason, "first-trigger")    // ← no message

Suggested fix:

require.False(t, hasExpires, "no safe-outputs config should not set hasExpires")
require.Equal(t, 0, minExpires, "no safe-outputs config should leave minExpires at zero")
require.Contains(t, triggerReason, "first-trigger", "triggerReason should identify the first workflow that triggered maintenance")

Note: This is a soft guideline warning, not a failure condition. The tests themselves are high-quality behavioral contracts.

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). No hard coding-guideline violations detected. Minor: test lines grew at 2.22× the production lines (above the 2× soft threshold) and 8 assertions are missing descriptive messages — neither is a failure condition.

📖 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: §27350375743

🧪 Test quality analysis by Test Quality Sentinel · 275.1 AIC · ⌖ 19.6 AIC · ◷

[github-actions]

✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both sub-tests are high-value behavioral contracts covering the 'first trigger wins' invariant and nil-input edge case. Minor: 8 assertions are missing descriptive messages (soft guideline warning, not a failure condition).

[github-actions]

Non-blocking observation

The refactoring correctly separates hasExpires tracking from triggerReason tracking, and the new test assertion (require.NotContains) properly validates the first-trigger-wins behavior.

One maintainability concern (see inline comment)

Moving hasExpires = true out of the closure and into 4 call sites creates a hidden contract: every future check block must remember to set hasExpires = true before calling setTriggerReason. If a new expiration type is added and the assignment is missed, scanWorkflowsForExpires silently returns hasExpires=false despite a qualifying workflow existing. Consolidating hasExpires = true back into the closure eliminates this risk with no behavioral change.

🔎 Code quality review by PR Code Quality Reviewer · ⌖ 13.2 AIC

[github-actions]

Decoupled invariant: setTriggerReason no longer owns hasExpires, creating an implicit call-site contract — any future check block that calls setTriggerReason without first setting hasExpires = true will silently return an inconsistent state (hasExpires=false with a non-empty triggerReason, or vice versa).

💡 Suggested fix: consolidate `hasExpires` back into the closure

The original closure atomically managed both variables. Keeping that invariant while still separating the two guards is straightforward:

setTriggerReason := func(reason string) {
    hasExpires = true
    if triggerReason == "" {
        triggerReason = reason
        maintenanceLog.Printf("Maintenance workflow became required: %s", reason)
    }
}

This restores a single source of truth for hasExpires and eliminates the 4 duplicated hasExpires = true assignments at the call sites. Every future check block automatically sets the flag by calling setTriggerReason, with no extra step to remember.