Fix activity_report cache persistence when report step fails

[Copilot]

Summary

Ensure the activity_report maintenance job always persists cache data, even if report generation fails.

Changes

• Switched activity_report cache handling from actions/cache to explicit actions/cache/restore + actions/cache/save.
• Added id: activity_report_logs_cache to the restore step.
• Added a save step with if: ${{ always() }} so cache is saved even on failure paths.
• Save step uses steps.activity_report_logs_cache.outputs.cache-primary-key for key consistency.
• Applied the same behavior in both maintenance workflow generators and the checked-in workflow file.
• Updated workflow tests to assert restore/save steps and always-run cache save behavior.

Validation

• Targeted tests passed:
  • go test -v -run "TestGenerateMaintenanceWorkflow_RunOperationCLICodegen|TestGenerateSideRepoMaintenanceWorkflow" ./pkg/workflow/
• Full validation command run:
  • make agent-finish (fails due to pre-existing unrelated tests: TestCopilotDetectionDefaultModel, TestWasmGolden_CompileFixtures)
• Parallel validation:
  • Code Review: passed with no comments
  • CodeQL: timed out (tool reported exhausted time budget)

[Copilot]

Pull request overview

Ensures the activity_report maintenance job persists its activity-report logs cache even when the report-generation step fails by splitting caching into explicit restore/save steps.

Changes:

• Replaced actions/cache with actions/cache/restore + actions/cache/save for activity_report logs caching.
• Added an always()-gated cache save step using the restore step’s primary key output.
• Updated workflow-generation tests (and the side-repo integration test) to assert the new restore/save behavior.

Show a summary per file

File	Description
pkg/workflow/side_repo_maintenance_integration_test.go	Updates integration assertions to require restore/save cache steps and always-run save behavior.
pkg/workflow/side_repo_maintenance.go	Generates restore/save cache steps for side-repo activity_report job.
pkg/workflow/maintenance_workflow_yaml.go	Generates restore/save cache steps for main maintenance activity_report job.
pkg/workflow/maintenance_workflow_test.go	Updates unit tests to assert restore/save steps and always-run save semantics.
.github/workflows/agentics-maintenance.yml	Applies the same restore/save caching pattern to the checked-in workflow.

Copilot's findings

Tip

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

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

[Copilot]

The cache save step runs with always(), so it will execute even if earlier steps failed and the restore step was skipped. In that case steps.activity_report_logs_cache.outputs.cache-primary-key will be empty/undefined, and actions/cache/save will fail due to a missing key, potentially masking the original failure. Consider gating the save step on the restore step having run and having a non-empty primary key output (or otherwise ensure a fallback key is provided).

[Copilot]

Because this cache save step uses if: ${{ always() }}, it will run even when earlier steps fail and the restore step was skipped. If activity_report_logs_cache didn’t run, steps.activity_report_logs_cache.outputs.cache-primary-key will be empty and actions/cache/save will error due to a missing key, which can obscure the real failure. Add a condition to only run the save when the restore step produced a primary key (or provide a safe fallback key).

[Copilot]

With if: ${{ always() }}, this save step will run even if the build or cache restore step failed/was skipped. In that scenario the referenced output steps.activity_report_logs_cache.outputs.cache-primary-key will be empty, and actions/cache/save will fail because key is required—potentially replacing the original error. Gate this step on the restore step having run and emitting a non-empty primary key output (or provide a fallback key).

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 90/100

✅ Excellent test quality

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	No
🚨 Coding-guideline violations	None

---

Test Classification Details

Test	File	Classification	Issues Detected
TestGenerateMaintenanceWorkflow_OperationJobConditions	pkg/workflow/maintenance_workflow_test.go	✅ Design	Behavioral assertions weakened — see flagged section
TestSideRepoMaintenanceWorkflowGenerated_EndToEnd	pkg/workflow/side_repo_maintenance_integration_test.go:54	✅ Design	Behavioral assertions weakened — see flagged section

---

Flagged Tests — Requires Review

⚠️ Both modified tests — lost if: $\{\{ always() }} behavioral invariant

Classification: Design tests (behavioral contracts), but a key fault-tolerance assertion was removed

Issue: Both tests previously asserted four specific YAML properties of the cache implementation:

1. A "Restore activity report logs cache" step exists
2. A "Save activity report logs cache" step exists
3. if: $\{\{ always() }} is present — ensuring cache saves even when the report step fails
4. steps.activity_report_logs_cache.outputs.cache-primary-key is used for the save key

The fix replaces this with a single actions/cache (combined restore+save) step, and the tests are updated to match. However, the replacement assertions only check for the step name ("Cache activity report logs") and do not verify the new mechanism still saves the cache when the upstream report step fails.

What design invariant does this test enforce? The tests verify the YAML structure of the activity_report job — a behavioral contract. But the original if: $\{\{ always() }} check was the direct assertion of the PR's stated fix ("cache persistence when report step fails"). That invariant is no longer verified.

What would break if deleted? A regression that removes the cache step entirely would be caught. But a regression that removes the actions/cache post-step save behavior (e.g., reverting to a bare restore-only step) would not be caught by the new tests.

Suggested improvement: Add an assertion that the generated YAML does not contain a separate actions/cache/restore-only step (since that was the broken pattern), and/or verify that the actions/cache combined action is used — which intrinsically saves on job completion including failure. For example:

assert.Contains(t, contentStr, getActionPin("actions/cache"),
    "should use combined actions/cache (not restore-only) so cache saves even on failure")
assert.NotContains(t, contentStr, "actions/cache/restore",
    "should not use restore-only cache action which would skip save on failure")

---

Language Support

Tests analyzed:

• 🐹 Go (*_test.go): 2 tests — 1 unit (//go:build !integration), 1 integration (//go:build integration)
• 🟨 JavaScript (*.test.cjs, *.test.js): 0 tests

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). No coding-guideline violations detected. Both modified tests are behavioral contract tests with proper build tags and assertion messages. However, the key fault-tolerance invariant (cache saves even on step failure) is no longer explicitly verified — see the flagged section above for a suggested improvement.

---

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

🧪 Test quality analysis by Test Quality Sentinel · ● 775.2K · ◷

[github-actions]

✅ Test Quality Sentinel: 90/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). Both modified tests are behavioral contract tests with proper build tags. See the comment above for a suggestion to explicitly verify the fault-tolerance invariant (cache saves even on failure) using the new actions/cache combined action.