diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 500e04943..e635e5291 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -26,6 +26,7 @@ - Fixed advisor context handling to automatically maintain its token budget by promoting the advisor model or compacting/restarting advisor context when needed, preventing advice from degrading on long sessions - Fixed `startup.quiet` leaving MCP and LSP startup status events visible during launch ([#2639](https://github.com/can1357/oh-my-pi/issues/2639)). - Registered the `Advisor` group in the `model` settings tab so advisor settings render correctly in the settings panel. +- Fixed soft-expired `issue://` and `pr://` reads to refresh live before returning stale state, with an explicit stale warning when the live refresh fails ([#2684](https://github.com/can1357/oh-my-pi/issues/2684)). ## [15.13.3] - 2026-06-15 diff --git a/packages/coding-agent/src/internal-urls/issue-pr-protocol.ts b/packages/coding-agent/src/internal-urls/issue-pr-protocol.ts index a65e8a797..d8a825361 100644 --- a/packages/coding-agent/src/internal-urls/issue-pr-protocol.ts +++ b/packages/coding-agent/src/internal-urls/issue-pr-protocol.ts @@ -28,7 +28,7 @@ import { parsePositiveDecimalInt, resolveDefaultRepoMemoized, } from "../tools/gh"; -import { formatFreshnessNote } from "../tools/github-cache"; +import { type CacheStatus, formatFreshnessNote } from "../tools/github-cache"; import * as git from "../utils/git"; import type { InternalResource, InternalUrl, ProtocolHandler, ResolveContext } from "./types"; @@ -355,7 +355,7 @@ interface BuildSingleArgs { scheme: Scheme; parsed: ParsedSingle; rendered: string; - status: "miss" | "fresh" | "stale" | "disabled"; + status: CacheStatus; fetchedAt: number; /** Resolved repo (post short-form expansion) — used for the PR-only diff hint. */ repo?: string; @@ -377,11 +377,15 @@ function buildSingleResource({ const diffUrl = repoSegment ? `pr://${repoSegment}/${parsed.number}/diff` : `pr://${parsed.number}/diff`; notes.push(`Diff: ${diffUrl}`); } + const content = + status === "stale" + ? `> WARNING: Live GitHub refresh failed; this ${scheme} content is cached and may be stale.\n\n${rendered}` + : rendered; return { url: url.href, - content: rendered, + content, contentType: "text/markdown", - size: Buffer.byteLength(rendered, "utf-8"), + size: Buffer.byteLength(content, "utf-8"), notes, }; } diff --git a/packages/coding-agent/src/tools/github-cache.ts b/packages/coding-agent/src/tools/github-cache.ts index 664369a37..62a41bb0f 100644 --- a/packages/coding-agent/src/tools/github-cache.ts +++ b/packages/coding-agent/src/tools/github-cache.ts @@ -8,10 +8,11 @@ * helpers swallow open/IO failures and degrade to "no cache" so a corrupt or * unreadable DB never blocks a `gh` call. * - * TTL: * Soft TTL → return cached row directly. - * Past soft TTL but within hard TTL → return cached row AND schedule a - * background refresh (errors logged, never thrown). + * Stateful issue/PR rows past soft TTL but within hard TTL → refresh + * synchronously, falling back to the cached row if the live fetch fails. + * Expensive PR diff rows past soft TTL but within hard TTL → return cached + * row AND schedule a background refresh (errors logged, never thrown). * Past hard TTL → treat as miss and fetch fresh. */ @@ -449,7 +450,7 @@ export interface CacheLookupOptions { now?: number; } -export type CacheStatus = "miss" | "fresh" | "stale" | "disabled"; +export type CacheStatus = "miss" | "fresh" | "refreshed" | "stale" | "disabled"; export interface CacheLookupResult { rendered: string; @@ -595,7 +596,7 @@ export async function getOrFetchView(options: CacheLookupOptions): Promise status: "fresh", fetchedAt: cached.fetchedAt, }; - } else { + } else if (options.kind === "pr-diff") { scheduleBackgroundRefresh( authKey, options.repo, @@ -611,6 +612,27 @@ export async function getOrFetchView(options: CacheLookupOptions): Promise status: "stale", fetchedAt: cached.fetchedAt, }; + } else { + try { + const fresh = await options.fetchFresh(); + const fetchedAt = Date.now(); + storeResult(authKey, options.repo, options.kind, options.number, options.includeComments, fresh, fetchedAt); + return { ...fresh, status: "refreshed", fetchedAt }; + } catch (err) { + logger.debug("github cache: synchronous refresh failed; returning stale view", { + err: String(err), + repo: options.repo, + kind: options.kind, + number: options.number, + }); + return { + rendered: cached.rendered, + sourceUrl: cached.sourceUrl, + payload: cached.payload, + status: "stale", + fetchedAt: cached.fetchedAt, + }; + } } } @@ -624,7 +646,7 @@ export async function getOrFetchView(options: CacheLookupOptions): Promise * Human-friendly freshness note for protocol-handler `notes[]` rendering. */ export function formatFreshnessNote(status: CacheStatus, fetchedAtMs: number, now: number = Date.now()): string { - if (status === "miss") return "Fetched live"; + if (status === "miss" || status === "refreshed") return "Fetched live"; if (status === "disabled") return "Cache disabled; fetched live"; const ageSec = Math.max(0, Math.round((now - fetchedAtMs) / 1000)); const human = @@ -633,6 +655,7 @@ export function formatFreshnessNote(status: CacheStatus, fetchedAtMs: number, no : ageSec < 3600 ? `${Math.round(ageSec / 60)}m ago` : `${Math.round(ageSec / 3600)}h ago`; - if (status === "stale") return `Cached: ${human} (refreshing in background)`; + if (status === "stale") + return `WARNING: showing cached content from ${human}; live refresh failed or is still running`; return `Cached: ${human}`; } diff --git a/packages/coding-agent/test/internal-urls/issue-pr-protocol.test.ts b/packages/coding-agent/test/internal-urls/issue-pr-protocol.test.ts index d4c36a5f3..359796938 100644 --- a/packages/coding-agent/test/internal-urls/issue-pr-protocol.test.ts +++ b/packages/coding-agent/test/internal-urls/issue-pr-protocol.test.ts @@ -9,6 +9,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { InternalUrlRouter } from "@oh-my-pi/pi-coding-agent/internal-urls"; import { resetForTests as resetCacheForTests } from "@oh-my-pi/pi-coding-agent/tools/github-cache"; import * as git from "@oh-my-pi/pi-coding-agent/utils/git"; @@ -173,6 +174,27 @@ describe("issue:// protocol handler", () => { expect(spy).toHaveBeenCalledTimes(1); }); + it("marks soft-expired issue fallback content as stale when live refresh fails", async () => { + const spy = vi.spyOn(git.github, "json").mockResolvedValue(issuePayload(43, "cached body") as never); + const settings = Settings.isolated({ + "github.cache.softTtlSec": 0, + "github.cache.hardTtlSec": 86400, + }); + + const router = InternalUrlRouter.instance(); + await router.resolve("issue://owner/example/43"); + await Bun.sleep(1); + spy.mockImplementation(async () => { + throw new Error("offline"); + }); + + const resource = await router.resolve("issue://owner/example/43", { settings }); + expect(resource.content.startsWith("> WARNING: Live GitHub refresh failed")).toBe(true); + expect(resource.notes?.[0]).toMatch(/^WARNING: showing cached content/); + expect(resource.content).toContain("cached body"); + expect(spy).toHaveBeenCalledTimes(2); + }); + it("retries issue://owner/repo/ without stateReason when gh does not support it", async () => { const spy = vi.spyOn(git.github, "json").mockImplementation(async (_cwd, args) => { if (requestedJsonFields(args).has("stateReason")) { diff --git a/packages/coding-agent/test/tools/github-cache.test.ts b/packages/coding-agent/test/tools/github-cache.test.ts index cb4631a50..e6a904a82 100644 --- a/packages/coding-agent/test/tools/github-cache.test.ts +++ b/packages/coding-agent/test/tools/github-cache.test.ts @@ -299,7 +299,7 @@ describe("getOrFetchView (TTL semantics)", () => { expect(fetchFresh).not.toHaveBeenCalled(); }); - it("returns cached row AND schedules a background refresh past soft TTL", async () => { + it("refreshes issue rows synchronously past soft TTL", async () => { const settings = Settings.isolated({ "github.cache.softTtlSec": 60, "github.cache.hardTtlSec": 86400, @@ -326,21 +326,94 @@ describe("getOrFetchView (TTL semantics)", () => { fetchFresh, settings, }); - expect(result.status).toBe("stale"); - expect(result.rendered).toBe("old"); - - // Background refresh runs on a microtask; flush + give the write a tick - // to land before asserting. - await Promise.resolve(); - // Wait for the chained .then() (writes) to settle. - await new Promise(resolve => setTimeout(resolve, 5)); - + expect(result.status).toBe("refreshed"); + expect(result.rendered).toBe("refreshed"); expect(fetchFresh).toHaveBeenCalledTimes(1); + const updated = getCached<{ refreshed: boolean }>(TEST_REPO, "issue", 50, true); expect(updated?.rendered).toBe("refreshed"); expect(updated?.payload.refreshed).toBe(true); }); + it("falls back to stale issue rows when a soft-expired synchronous refresh fails", async () => { + const settings = Settings.isolated({ + "github.cache.softTtlSec": 60, + "github.cache.hardTtlSec": 86400, + }); + const fetchFresh = vi.fn( + async (): Promise<{ + rendered: string; + sourceUrl: undefined; + payload: { number: number; refreshed: boolean }; + }> => { + throw new Error("offline"); + }, + ); + putCached({ + repo: TEST_REPO, + kind: "issue", + number: 51, + includeComments: true, + payload: { number: 51, refreshed: false }, + rendered: "old", + fetchedAt: Date.now() - 5 * 60_000, + }); + + const result = await getOrFetchView({ + repo: TEST_REPO, + kind: "issue", + number: 51, + includeComments: true, + fetchFresh, + settings, + }); + + expect(result.status).toBe("stale"); + expect(result.rendered).toBe("old"); + expect(fetchFresh).toHaveBeenCalledTimes(1); + }); + + it("keeps PR diff rows stale-first past soft TTL", async () => { + const settings = Settings.isolated({ + "github.cache.softTtlSec": 60, + "github.cache.hardTtlSec": 86400, + }); + const fetchFresh = vi.fn(async () => ({ + rendered: "refreshed-diff", + sourceUrl: undefined, + payload: { files: [], refreshed: true }, + })); + putCached({ + repo: TEST_REPO, + kind: "pr-diff", + number: 52, + includeComments: false, + payload: { files: [], refreshed: false }, + rendered: "old-diff", + fetchedAt: Date.now() - 5 * 60_000, + }); + + const result = await getOrFetchView({ + repo: TEST_REPO, + kind: "pr-diff", + number: 52, + includeComments: false, + fetchFresh, + settings, + }); + + expect(result.status).toBe("stale"); + expect(result.rendered).toBe("old-diff"); + + await Promise.resolve(); + await Bun.sleep(5); + + expect(fetchFresh).toHaveBeenCalledTimes(1); + const updated = getCached<{ refreshed: boolean }>(TEST_REPO, "pr-diff", 52, false); + expect(updated?.rendered).toBe("refreshed-diff"); + expect(updated?.payload.refreshed).toBe(true); + }); + it("treats past-hard-TTL rows as cache misses (fetcher runs)", async () => { const settings = Settings.isolated({ "github.cache.softTtlSec": 60,