diff --git a/apps/memos-local-plugin/core/session/episode-manager.ts b/apps/memos-local-plugin/core/session/episode-manager.ts index c38ae082e..3553f12c7 100644 --- a/apps/memos-local-plugin/core/session/episode-manager.ts +++ b/apps/memos-local-plugin/core/session/episode-manager.ts @@ -93,6 +93,32 @@ export function createEpisodeManager(deps: EpisodeManagerDeps): EpisodeManager { return snap; } + /** + * Mark `meta.rewardDirty` for removal on a terminal transition + * (issue #2370). + * + * `reopen()` re-sets the marker, so a close that doesn't happen to run + * the reward write (e.g. the episode resumes with `rTask != null`, so + * the reward fallback is not run) would leave a stale marker on a + * fully-resolved episode — indistinguishable from live dirt to + * `episodeRewardIsDirty()` and to any consistency check built on + * `json_type(meta_json,'$.rewardDirty') IS NOT NULL`. + * + * `undefined` (rather than `null`, or an omitted key) is deliberate on + * both counts: + * - `toJsonText` runs `JSON.stringify`, which omits undefined-valued + * keys, so the key is *absent* from `meta_json` afterwards. `null` + * would leave `json_type(...)` reporting `'null'`, which is still + * `IS NOT NULL` and keeps matching such a check. + * - `episodesRepo.close()` / `updateMeta()` *merge* the patch into + * the existing `meta_json`, so simply omitting the key would leave + * the previous value in place. It has to be overwritten. + * + * The reward / rescan paths may legitimately re-arm the marker later; + * clearing here only drops dirt that predates this transition. + */ + const CLEAR_REWARD_DIRTY = { rewardDirty: undefined } as const; + return { start(input: EpisodeStartInput, intent: IntentDecision): EpisodeSnapshot { if (!input.initialTurn || !input.initialTurn.content) { @@ -243,7 +269,12 @@ export function createEpisodeManager(deps: EpisodeManagerDeps): EpisodeManager { snap.endedAt = endedAt; if (input?.rTask !== undefined) snap.rTask = input.rTask; if (input?.patchMeta) snap.meta = { ...snap.meta, ...input.patchMeta }; - snap.meta = { ...snap.meta, topicState: "ended", closeReason: "finalized" }; + snap.meta = { + ...snap.meta, + ...CLEAR_REWARD_DIRTY, + topicState: "ended", + closeReason: "finalized", + }; deps.episodesRepo.close(id, endedAt, snap.rTask ?? undefined, snap.meta); log.info("episode.finalized", { episodeId: id, @@ -271,6 +302,7 @@ export function createEpisodeManager(deps: EpisodeManagerDeps): EpisodeManager { snap.endedAt = endedAt; snap.meta = { ...snap.meta, + ...CLEAR_REWARD_DIRTY, topicState: "ended", closeReason: "abandoned", abandonReason: reason, @@ -340,6 +372,22 @@ export function createEpisodeManager(deps: EpisodeManagerDeps): EpisodeManager { at: now(), ...previousReward, }, + // If the episode was previously terminal-skipped, clear the skip + // flag so episodeRewardIsDirty() can reach hasRewardDirtyMarker. + // rewardWasSkipped() short-circuits that check, so a reopened + // episode that previously had reward.skipped=true is invisible to + // every rescore scan and the rewardDirty marker never clears + // (#2370). + ...(snap.meta.reward && + typeof snap.meta.reward === "object" && + (snap.meta.reward as { skipped?: unknown }).skipped === true + ? { + reward: { + ...(snap.meta.reward as Record), + skipped: undefined, + }, + } + : {}), } : {}), }; diff --git a/apps/memos-local-plugin/tests/unit/session/episode-manager.test.ts b/apps/memos-local-plugin/tests/unit/session/episode-manager.test.ts index 2167bdcd7..70fcefeb2 100644 --- a/apps/memos-local-plugin/tests/unit/session/episode-manager.test.ts +++ b/apps/memos-local-plugin/tests/unit/session/episode-manager.test.ts @@ -1,6 +1,7 @@ import { beforeAll, beforeEach, describe, expect, it } from "vitest"; import { ERROR_CODES, MemosError } from "../../../agent-contract/errors.js"; +import type { EpisodeId } from "../../../agent-contract/dto.js"; import { createEpisodeManager, createSessionEventBus, @@ -54,6 +55,18 @@ describe("session/episode-manager", () => { return { epm, bus }; } + /** + * What actually lands in `meta_json` in production: the repo persists via + * `toJsonText` → `JSON.stringify`, which omits undefined-valued keys, and + * the row is read back through `JSON.parse`. The fake keeps the raw + * (unserialised) object, so round-trip it here to assert on the shape a + * consistency check such as `json_type(meta_json,'$.rewardDirty')` sees. + */ + function persistedMeta(id: EpisodeId): Record { + const raw = episodesFake.rows.get(id)!.meta; + return JSON.parse(JSON.stringify(raw)) as Record; + } + it("start inserts row and emits episode.started", () => { const { epm, bus } = makeEpm(); const events: string[] = []; @@ -200,4 +213,115 @@ describe("session/episode-manager", () => { const all = epm.listForSession("se_a"); expect(all.map((e) => e.id).sort()).toEqual([s1.id, s2.id].sort()); }); + + // Regression test for #2370: reopen() must clear reward.skipped when setting + // rewardDirty so that episodeRewardIsDirty() is not short-circuited by + // rewardWasSkipped() and the marker is actually cleared by the rescore path. + it("reopen clears reward.skipped when setting rewardDirty (#2370)", () => { + const { epm } = makeEpm(); + const snap = epm.start( + { sessionId: "se_a", initialTurn: { role: "user", content: "x" } }, + intent("task"), + ); + nowTick = 2_000; + // Simulate terminal-skip scoring: close the episode and stamp reward.skipped=true. + epm.finalize(snap.id); + epm.patchMeta(snap.id, { + reward: { + source: "heuristic", + reason: "too_short", + scoredAt: 2_000, + skipped: true, + }, + closeReason: "abandoned", + }); + + // Verify the skip is present before reopen. + const beforeReopen = episodesFake.rows.get(snap.id)!; + expect((beforeReopen.meta.reward as Record).skipped).toBe(true); + + nowTick = 91_000; + // Reopen the episode (e.g. follow_up path). + const reopened = epm.reopen(snap.id, "follow_up"); + + // rewardDirty must be set so rescore scans pick this episode up. + expect(reopened.meta.rewardDirty).toBeTruthy(); + expect((reopened.meta.rewardDirty as Record).reason).toBe( + "episode_reopened", + ); + + // reward.skipped must be cleared (undefined) so rewardWasSkipped() returns + // false and episodeRewardIsDirty() reaches the hasRewardDirtyMarker check. + const reward = reopened.meta.reward as Record | undefined; + expect(reward?.skipped).toBeUndefined(); + + // The DB row must also reflect the cleared flag. + const dbRow = episodesFake.rows.get(snap.id)!; + expect((dbRow.meta.reward as Record | undefined)?.skipped).toBeUndefined(); + expect(dbRow.meta.rewardDirty).toBeTruthy(); + }); + + it("reopen does not set rewardDirty when episode was never scored", () => { + const { epm } = makeEpm(); + const snap = epm.start( + { sessionId: "se_a", initialTurn: { role: "user", content: "x" } }, + intent(), + ); + epm.finalize(snap.id); + // No reward meta at all — rTask is null, no meta.reward set. + const reopened = epm.reopen(snap.id, "follow_up"); + expect(reopened.meta.rewardDirty).toBeUndefined(); + }); + + // Regression test for #2370: reopen() re-sets meta.rewardDirty, so a later + // terminal transition must drop it again. Before this, an episode that + // reached a terminal state through a path that did not happen to write a + // reward (e.g. it resumed with rTask != null, so the reward fallback was + // skipped) kept the marker forever and was reported dirty on every + // consistency run while being fully resolved. + it("reaching a terminal state clears the rewardDirty marker set by reopen (#2370)", () => { + const { epm } = makeEpm(); + const snap = epm.start( + { sessionId: "se_a", initialTurn: { role: "user", content: "x" } }, + intent(), + ); + nowTick = 2_000; + epm.finalize(snap.id, { rTask: 0.0 }); + + nowTick = 3_000; + const reopened = epm.reopen(snap.id, "follow_up"); + expect(reopened.status).toBe("open"); + expect(reopened.rTask).toBe(0.0); + expect(persistedMeta(snap.id).rewardDirty).toBeDefined(); + + nowTick = 4_000; + const closed = epm.finalize(snap.id, { rTask: 0.0 }); + expect(closed.status).toBe("closed"); + const db = episodesFake.rows.get(snap.id)!; + expect(db.status).toBe("closed"); + expect(db.meta.rewardDirty).toBeUndefined(); + // Key *absence*, not just falsiness: a check written as + // `json_type(meta_json,'$.rewardDirty') IS NOT NULL` — which is how the + // reporter's watchdog is built — must not match either. + expect("rewardDirty" in persistedMeta(snap.id)).toBe(false); + }); + + it("abandon is terminal too and clears the rewardDirty marker (#2370)", () => { + const { epm } = makeEpm(); + const snap = epm.start( + { sessionId: "se_a", initialTurn: { role: "user", content: "x" } }, + intent(), + ); + nowTick = 2_000; + epm.finalize(snap.id, { rTask: 0.5 }); + nowTick = 3_000; + epm.reopen(snap.id, "follow_up"); + expect(persistedMeta(snap.id).rewardDirty).toBeDefined(); + + nowTick = 4_000; + epm.abandon(snap.id, "host_crashed"); + const db = episodesFake.rows.get(snap.id)!; + expect(db.status).toBe("closed"); + expect("rewardDirty" in persistedMeta(snap.id)).toBe(false); + }); });