fix: enforce 48h data retention on AIC usage cache entries#39084
Conversation
- Add `timestamp` field to entries written by write_daily_aic_usage_cache.cjs - Prune entries older than 48h when writing the cache file - Filter entries older than 48h when loading the cache in check_daily_aic_workflow_guardrail.cjs - Both write and load preserve entries without a timestamp (backward compat) - Add CACHE_RETENTION_MS = 48h constant to both files - Export mainWithPaths() for testability - Add write_daily_aic_usage_cache.test.cjs with 5 tests - Extend check_daily_aic_workflow_guardrail.test.cjs with 3 new timestamp tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR enforces a 48-hour retention policy for the daily AI Credits (AIC) usage cache by adding timestamps to cache entries, pruning stale entries on write, and skipping stale entries on read, with unit tests covering the new behavior.
Changes:
- Add
timestampto new AIC cache entries and prune timestamped entries older than 48 hours when rewriting the cache. - Update cache loading to ignore timestamped entries older than 48 hours and include
skippedStalein diagnostics. - Add/extend Vitest coverage for timestamp stamping, pruning, and backward compatibility (no timestamp).
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/write_daily_aic_usage_cache.cjs | Adds timestamp stamping and 48h pruning on cache rewrite; introduces mainWithPaths for testability. |
| actions/setup/js/check_daily_aic_workflow_guardrail.cjs | Skips cache entries older than 48h when loading and logs skippedStale. |
| actions/setup/js/write_daily_aic_usage_cache.test.cjs | New tests validating timestamp presence and 48h pruning/compat behavior. |
| actions/setup/js/check_daily_aic_workflow_guardrail.test.cjs | Extends tests to cover recent/stale/missing timestamp handling. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
| if (typeof entry?.timestamp === "string") { | ||
| const ts = Date.parse(entry.timestamp); | ||
| if (Number.isFinite(ts) && ts < cutoff) { | ||
| skippedStale++; | ||
| continue; | ||
| } | ||
| } |
| // Patch the module-level path constants by calling main() on the already-imported | ||
| // module; to make the paths configurable we call the exported helper that accepts | ||
| // explicit path arguments (defined below), falling back to swapping env vars. |
| it("writes a new entry with run_id, aic, and a timestamp when no cache file exists", async () => { | ||
| writeUsageFile(7.5); | ||
| await runMain(); | ||
|
|
||
| const content = fs.readFileSync(cacheFile, "utf8").trim(); | ||
| const entry = JSON.parse(content); | ||
| expect(entry.run_id).toBe(12345); | ||
| expect(entry.aic).toBe(7.5); | ||
| expect(typeof entry.timestamp).toBe("string"); | ||
| // Timestamp should be a valid ISO 8601 date within the last minute. | ||
| const ts = Date.parse(entry.timestamp); | ||
| expect(Number.isFinite(ts)).toBe(true); | ||
| expect(ts).toBeLessThanOrEqual(Date.now()); | ||
| expect(ts).toBeGreaterThan(Date.now() - 60_000); | ||
| }); |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39084 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). Neither ADR gate condition is met. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out — approving with minor suggestions.
The core fix is correct: timestamps are stamped atomically on write, pruned on both the read and write paths, and the backward-compat contract for no-timestamp entries is explicit and consistent across both files. The mainWithPaths extraction is a clean testability refactor with no production behaviour change.
📋 Key Themes & Highlights
Key Themes
- DRY risk:
CACHE_RETENTION_MSis defined independently in two files. Worth extracting to a shared constants module before the value ever needs to change. - Test coverage gaps: Missing boundary test (
ts === cutoff→ kept) and missing invalid-timestamp test. Both behaviours are correct but untested, which means a future refactor could silently break them. - Backward-compat convergence: No-timestamp entries are preserved indefinitely (by design), but the comment does not explain when full pruning enforcement is expected to kick in — helpful context for future maintainers.
Positive Highlights
- Symmetric pruning enforcement in both the write and read paths
Number.isFiniteguard onDate.parseresult handles corrupt timestamps gracefully- Diagnostic log (
skippedStale,pruned,total) makes retention behaviour observable in CI logs - 8 new test cases with meaningful margins (1 h, 2 h, 49 h, 50 h) — no brittle sub-second timing
mainWithPathsexport is a minimal, non-breaking API surface for testing
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 510.4 AIC · ⌖ 47.9 AIC · ⊞ 27.9K
| const PRIMARY_GUARDRAIL_ARTIFACT_NAMES = ["usage"]; | ||
| const DAILY_WORKFLOW_WINDOW_MS = 24 * 60 * 60 * 1000; | ||
| /** Cache entries older than this threshold (in ms) are skipped when loading. */ | ||
| const CACHE_RETENTION_MS = 48 * 60 * 60 * 1000; |
There was a problem hiding this comment.
[/diagnose] CACHE_RETENTION_MS is defined identically in both check_daily_aic_workflow_guardrail.cjs and write_daily_aic_usage_cache.cjs. If the retention window changes, both files must be updated atomically — easy to miss.
💡 Suggestion
Extract the constant into a shared module (e.g. daily_aic_constants.cjs) and require it in both files:
// daily_aic_constants.cjs
module.exports = {
CACHE_RETENTION_MS: 48 * 60 * 60 * 1000,
};This also makes it clear that both the write and read paths are intentionally coupled to the same window.
| // Read existing cache content (restored from the previous run's cache snapshot, if any). | ||
| let existingLines = ""; | ||
| // Entries with a `timestamp` older than CACHE_RETENTION_MS are pruned to keep the file | ||
| // bounded. Entries without a `timestamp` (written by an older version of this script) |
There was a problem hiding this comment.
[/zoom-out] No-timestamp entries are preserved forever — they are never pruned by the write path, and never skipped by the read path. This is intentional for migration safety, but it means caches that existed before this PR deployed will carry unbounded legacy entries until GitHub Actions expires the cache key naturally (typically 7 days).
💡 Suggestion
Consider documenting the expected convergence timeline in the comment, e.g.:
// Entries without a `timestamp` (written by an older version of this script before YYYY-MM-DD)
// are preserved indefinitely until the GitHub Actions cache key expires (~7 days).
// After all active cache files have cycled through at least one write with the new code,
// every entry will have a timestamp and pruning will be fully enforced.This helps future maintainers understand when the backward-compat clause can safely be removed.
| async function runMain() { | ||
| // Patch the module-level path constants by calling main() on the already-imported | ||
| // module; to make the paths configurable we call the exported helper that accepts | ||
| // explicit path arguments (defined below), falling back to swapping env vars. |
There was a problem hiding this comment.
[/tdd] The comment "falling back to swapping env vars" describes a strategy that does not exist in this implementation. runMain() always calls mainWithPaths directly with explicit paths — there is no env-var fallback.
Suggest simplifying the comment:
async function runMain() {
await exports.mainWithPaths(cacheFile, usageDir);
}| delete process.env.GITHUB_RUN_ID; | ||
| writeUsageFile(5.0); | ||
| await runMain(); | ||
| expect(fs.existsSync(cacheFile)).toBe(false); |
There was a problem hiding this comment.
[/tdd] The test verifies the cache file is not created, but does not assert that core.warning was called. When GITHUB_RUN_ID is unset, the module calls core.warning("[daily-aic-cache] GITHUB_RUN_ID not set; skipping cache write.") — worth asserting to confirm the skip is explicitly signalled and not silently swallowed.
💡 Suggested addition
it("skips writing when GITHUB_RUN_ID is not set", async () => {
delete process.env.GITHUB_RUN_ID;
writeUsageFile(5.0);
await runMain();
expect(fs.existsSync(cacheFile)).toBe(false);
expect(global.core.warning).toHaveBeenCalledWith(
expect.stringContaining("GITHUB_RUN_ID not set")
);
});| writeUsageFile(5.0); | ||
| await runMain(); | ||
| expect(fs.existsSync(cacheFile)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
[/tdd] Two edge cases are missing from the test suite:
-
Invalid timestamp string — e.g.
{ run_id: 9001, aic: 1.0, timestamp: "not-a-date" }. The code handles this correctly (Date.parsereturnsNaN,Number.isFinite(NaN)isfalse, entry is kept), but without a test this silent-keep behaviour could regress unnoticed. -
Exact boundary — an entry timestamped exactly
Date.now() - CACHE_RETENTION_MS. The check ists < cutoff(strict), so boundary entries are kept — a test pinning this would prevent an accidental<=change.
💡 Suggested tests
it("keeps entries with an unparseable timestamp (treats as no-timestamp)", async () => {
fs.writeFileSync(cacheFile, JSON.stringify({ run_id: 9001, aic: 1.0, timestamp: "not-a-date" }) + "\n", "utf8");
writeUsageFile(0);
await runMain();
const lines = fs.readFileSync(cacheFile, "utf8").trim().split("\n");
expect(lines.map(l => JSON.parse(l).run_id)).toContain(9001);
});
it("keeps an entry at exactly the 48 h cutoff boundary", async () => {
const boundaryTs = new Date(Date.now() - 48 * 60 * 60 * 1000).toISOString();
fs.writeFileSync(cacheFile, JSON.stringify({ run_id: 9002, aic: 2.0, timestamp: boundaryTs }) + "\n", "utf8");
writeUsageFile(0);
await runMain();
const lines = fs.readFileSync(cacheFile, "utf8").trim().split("\n");
expect(lines.map(l => JSON.parse(l).run_id)).toContain(9002);
});
|
@copilot run pr-finisher skill |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 83/100 — Excellent
📊 Metrics & Test Classification (8 tests analyzed)
Test Classification Details
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: §27469570702
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 83/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 8 new tests assert observable behavioral contracts covering the 48h data-retention feature, backward compatibility, and error paths.
Cache entries were stored as
{run_id, aic}with no timestamp, making age-based pruning impossible and allowing stale data to accumulate indefinitely. The cache miss seen in the referenced CI run is expected on first activation (activation always runs before conclusion, so only prior runs' caches are eligible), but retention still needed enforcing.Changes
write_daily_aic_usage_cache.cjstimestamp: new Date().toISOString()timestampare preserved for backward compatibilitymainWithPaths(cachePath, usageDir)to enable unit testingcheck_daily_aic_workflow_guardrail.cjs—loadAICUsageCachetimestampolder than 48 h; no-timestamp entries are still loadedskippedStaleto the diagnostic log lineTests
write_daily_aic_usage_cache.test.cjs: timestamp presence, 48 h pruning, preserve-recent, preserve-no-timestamp, skip-when-no-run-idcheck_daily_aic_workflow_guardrail.test.cjs: recent timestamp kept, stale timestamp skipped, missing timestamp kept