fix: remove run_id from trending cache key and enforce via compiler validation#28387
fix: remove run_id from trending cache key and enforce via compiler validation#28387
Conversation
…restoration Agent-Logs-Url: https://github.com/github/gh-aw/sessions/13fe2e7f-eb46-47e1-b6c6-6571419e9d61 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
||
| tools: | ||
| cache-memory: | ||
| key: trending-data-${{ env.GH_AW_WORKFLOW_ID_SANITIZED }}-${{ github.run_id }} |
There was a problem hiding this comment.
@copilot the compiler should detect the github.run_id expression in the cache-memory key and raise a compilation error
There was a problem hiding this comment.
Done in 9baf3b1. parseCacheMemoryEntry now calls validateNoCacheKeyRunID which uses a word-boundary regex (github\.run_id(?:[^_\w]|$)) so github.run_identifier won't false-positive. The 4 other workflow files that had the same bug (cloclo.md, poem-bot.md, stale-repo-identifier.md, shared/charts-with-trending.md) are also fixed — all 201 workflows recompile cleanly.
…cted workflows Agent-Logs-Url: https://github.com/github/gh-aw/sessions/876b15a1-265e-4f0a-85a6-a0febf6476ca Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… key check Agent-Logs-Url: https://github.com/github/gh-aw/sessions/876b15a1-265e-4f0a-85a6-a0febf6476ca Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification DetailsView test classification table
Flagged Tests — Requires ReviewNo tests were flagged for behavioral issues. Minor note — Test Inflation (informational, not a blocker): The test file adds 126 lines against 32 lines of new production code (ratio ≈ 3.9:1, exceeding the 2:1 threshold). This is expected for table-driven tests and does not represent low-quality testing — the additional lines are valuable test-case rows, not redundant boilerplate. The 10-point inflation penalty in the score reflects the rubric's mechanical threshold rather than a real quality concern. Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §24921258553
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (158 new lines in AI has analyzed the PR diff and generated a draft ADR to help document this decision retroactively: 📄 Draft ADR:
📋 Draft ADR content — copy and save to
|
| Check | Result |
|---|---|
implementation label |
Not present |
New lines in pkg/ (business logic) |
158 lines (threshold: 100) |
| ADR linked in PR body | ❌ Not found |
Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. This PR introduces a new compiler-level invariant — a future contributor who sees validateNoCacheKeyRunID and wonders "why does this exist?" should find the answer in docs/adr/.
📋 Michael Nygard ADR Format Reference
An ADR must contain these four sections to be considered complete:
- Context — What is the problem? What forces are at play?
- Decision — What did you decide? Why?
- Alternatives Considered — What else could have been done?
- Consequences — What are the trade-offs (positive and negative)?
All ADRs are stored in docs/adr/ as Markdown files numbered by PR number (e.g., 28387-kebab-title.md for PR #28387).
References: §24921258566
🏗️ ADR gate enforced by Design Decision Gate 🏗️ · ● 194.4K · ◷
There was a problem hiding this comment.
Pull request overview
This PR fixes cache-memory key stability across workflow runs by removing user-specified ${{ github.run_id }} from cache-memory keys and enforcing that constraint at compile time, enabling cross-run cache restores.
Changes:
- Added compiler validation to reject user-supplied
tools.cache-memory.keyvalues that referencegithub.run_id, plus table-driven unit tests. - Updated cache-memory key parsing to invoke the new validation before accepting custom keys.
- Removed
-${{ github.run_id }}from affected workflow frontmatter cache-memory keys and updated documentation to log cache restore status.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/cache_validation.go | Adds validateNoCacheKeyRunID and regex to detect github.run_id usage in cache-memory keys. |
| pkg/workflow/cache_key_validation_test.go | Adds unit tests covering object/array notation and false-positive cases. |
| pkg/workflow/cache.go | Calls validateNoCacheKeyRunID when parsing a custom cache-memory key. |
| .github/workflows/shared/trending-charts-simple.md | Removes ${{ github.run_id }} from cache-memory key to make it stable across runs. |
| .github/workflows/shared/charts-with-trending.md | Removes ${{ github.run_id }} from cache-memory key to make it stable across runs. |
| .github/workflows/cloclo.md | Removes ${{ github.run_id }} from cache-memory key to make it stable across runs. |
| .github/workflows/poem-bot.md | Removes ${{ github.run_id }} from cache-memory key to make it stable across runs. |
| .github/workflows/stale-repo-identifier.md | Removes ${{ github.run_id }} from cache-memory key to make it stable across runs. |
| .github/workflows/api-consumption-report.md | Adds a bash snippet and template field to explicitly report whether cache was restored. |
| .github/workflows/stale-repo-identifier.lock.yml | Regenerated lock file corresponding to workflow frontmatter changes. |
| .github/workflows/python-data-charts.lock.yml | Regenerated lock file corresponding to shared workflow frontmatter changes. |
| .github/workflows/poem-bot.lock.yml | Regenerated lock file corresponding to workflow frontmatter changes. |
| .github/workflows/cloclo.lock.yml | Regenerated lock file corresponding to workflow frontmatter changes. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 1
| // # Validation Functions | ||
| // | ||
| // - validateNoDuplicateCacheIDs() - Ensures each cache entry has a unique ID | ||
| // - validateNoCacheKeyRunID() - Rejects cache keys that reference github.run_id | ||
| // |
There was a problem hiding this comment.
The top-of-file comment describes this file as validating sandbox cache-memory config, but this file now also contains validation used for tools.cache-memory keys (via parseCacheMemoryEntry). Consider updating the file-level documentation/section headings (or relocating the tool-specific validation) so the docs accurately reflect current usage.
See below for a potential fix:
// This file provides validation for cache-memory configuration.
//
// # Cache Memory Validation
//
// This file contains validation shared across cache-memory configuration,
// including:
// - sandbox.cache-memory entries, such as duplicate ID checks
// - tools.cache-memory keys, such as rejecting github.run_id references
//
// These validations prevent configuration mistakes that would otherwise cause
// runtime conflicts or ineffective cache behavior.
//
// # Validation Functions
//
// - validateNoDuplicateCacheIDs() - Ensures each sandbox cache entry has a unique ID
// - validateNoCacheKeyRunID() - Rejects tools.cache-memory keys that reference github.run_id
//
// # When to Add Validation Here
//
// Add validation to this file when:
// - Adding new sandbox.cache-memory or tools.cache-memory constraints
The
run_idsuffix in cache-memory keys was causing every run to write to a unique cache slot, making cross-run cache restoration impossible. The result:history.jsonlappeared to accumulate only one entry per run, with every run reporting "This is the first day of tracking".Changes
Compiler validation (
pkg/workflow/cache_validation.go,pkg/workflow/cache.go)validateNoCacheKeyRunID— returns a compilation error when a user-suppliedcache-memorykey contains the${{ github.run_id }}expressiongithub\.run_id(?:[^_\w]|$)) to avoid false positives on names likegithub.run_identifierparseCacheMemoryEntrybefore a custom key is acceptedcache_key_validation_test.gowith table-driven tests for both object and array notation, including false-positive casesWorkflow fixes — all affected workflows
Removed
-${{ github.run_id }}fromcache-memorykeys in every workflow that had the same bug:shared/trending-charts-simple.mdshared/charts-with-trending.mdcloclo.mdpoem-bot.mdstale-repo-identifier.mdThe compiler appends
run_idautomatically to the save key and generates a stablerestore-keysprefix — users must not add it themselves.api-consumption-report.mdCache restored from previous runfield to the Cache Memory Status section in the discussion template