Skip to content

Commit 255d72a

Browse files
author
daewoongoh
committed
fix(task): keep manual titles across stores and align fallback with spec
Check the on-disk title inside the per-file lock so a rename made through another provider is never overwritten by a generated title. Use the first 42 characters of the message as the fallback, and add tests for pending generation and unusable model responses.
1 parent 160487a commit 255d72a

6 files changed

Lines changed: 135 additions & 40 deletions

File tree

‎src/core/task-persistence/TaskHistoryStore.ts‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -996,6 +996,51 @@ export class TaskHistoryStore {
996996

997997
// ────────────────────────────── Atomic read-modify-write ──────────────────────────────
998998

999+
/**
1000+
* Set `title` only while the item has none, checking the title on disk inside the
1001+
* per-file lock. The cache can be stale when another store (for example a second
1002+
* provider on the same storage path) saved a manual title, and a delta merge would
1003+
* otherwise overwrite it.
1004+
*
1005+
* @returns "conflict" when a title already exists (the cache is refreshed from disk).
1006+
* @throws If the task ID is not present in the cache.
1007+
*/
1008+
public setTitleIfUnset(taskId: string, title: string): Promise<"applied" | "conflict"> {
1009+
return this.withLock(async () => {
1010+
const current = this.cache.get(taskId)
1011+
if (!current) {
1012+
throw new Error(`[TaskHistoryStore] setTitleIfUnset: task ${taskId} not found in cache`)
1013+
}
1014+
if (current.title) {
1015+
return "conflict"
1016+
}
1017+
1018+
const filePath = await this.getTaskFilePath(taskId)
1019+
let written: HistoryItem = { ...current, title }
1020+
let conflict = false
1021+
await safeWriteJson(filePath, written, {
1022+
merge: (existing, incoming) => {
1023+
if (existing && typeof existing === "object" && (existing as HistoryItem).title) {
1024+
conflict = true
1025+
written = existing as HistoryItem
1026+
return existing
1027+
}
1028+
written = mergeHistoryDelta(existing, incoming as HistoryItem, { title })
1029+
return written
1030+
},
1031+
})
1032+
1033+
this.cache.set(taskId, written)
1034+
if (conflict) {
1035+
return "conflict"
1036+
}
1037+
if (this.onWrite) {
1038+
await this.onWrite(this.getAll())
1039+
}
1040+
return "applied"
1041+
})
1042+
}
1043+
9991044
/**
10001045
* Read a HistoryItem from the in-memory cache and write back an updated version,
10011046
* all within a single lock acquisition so no concurrent writer can interleave

‎src/core/task-persistence/__tests__/TaskHistoryStore.crossInstance.spec.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,35 @@ describe("TaskHistoryStore cross-instance safety", () => {
7373
await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => {})
7474
})
7575

76+
describe("setTitleIfUnset", () => {
77+
it("keeps a manual title that another instance wrote after this cache was loaded", async () => {
78+
await storeA.initialize()
79+
await storeB.initialize()
80+
await storeA.upsert(makeHistoryItem({ id: "title-task", task: "Original prompt" }))
81+
await storeB.reconcile()
82+
expect(storeB.get("title-task")?.title).toBeUndefined()
83+
84+
// Instance A still caches no title when B renames the task on disk.
85+
await storeB.atomicReadAndUpdate("title-task", (item) => ({ ...item, title: "Manual title" }))
86+
expect(storeA.get("title-task")?.title).toBeUndefined()
87+
88+
await expect(storeA.setTitleIfUnset("title-task", "Auto title")).resolves.toBe("conflict")
89+
90+
expect(storeA.get("title-task")?.title).toBe("Manual title")
91+
await storeB.reconcile()
92+
expect(storeB.get("title-task")?.title).toBe("Manual title")
93+
})
94+
95+
it("applies the title when no instance has set one", async () => {
96+
await storeA.initialize()
97+
await storeA.upsert(makeHistoryItem({ id: "title-free", task: "Original prompt" }))
98+
99+
await expect(storeA.setTitleIfUnset("title-free", "Auto title")).resolves.toBe("applied")
100+
101+
expect(storeA.get("title-free")).toMatchObject({ title: "Auto title", task: "Original prompt" })
102+
})
103+
})
104+
76105
it("two instances can write different tasks without conflict", async () => {
77106
await storeA.initialize()
78107
await storeB.initialize()

‎src/core/task/__tests__/Task.session-title.spec.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,27 @@ describe("Task#startTask session title trigger", () => {
173173
expect(resolved).toBe(true)
174174
})
175175

176+
it("does not wait for a pending title before starting the task loop", async () => {
177+
const provider = createMockProvider()
178+
const task = createTestTask(provider)
179+
stubStartup(task)
180+
181+
let releaseTitle: () => void = () => {}
182+
vi.mocked(generateSessionTitle).mockImplementation(
183+
() =>
184+
new Promise<void>((resolve) => {
185+
releaseTitle = resolve
186+
}),
187+
)
188+
189+
// startTask resolving while the title promise is still pending proves it is not awaited.
190+
await getStartTaskAccess(task).startTask("slow title message")
191+
192+
expect(getStartTaskAccess(task).initiateTaskLoop).toHaveBeenCalled()
193+
expect(lastTitleCall()?.userMessage).toBe("slow title message")
194+
releaseTitle()
195+
})
196+
176197
it.each([
177198
["Production", 1, true],
178199
["Development", 2, true],

‎src/core/task/__tests__/session-title.spec.ts‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,11 @@ describe("extractLlmTitle", () => {
100100
})
101101

102102
describe("fallbackTitle", () => {
103-
it("uses the first non-empty line", () => {
104-
expect(fallbackTitle("\n\nsecond\nthird")).toBe("second")
103+
it.each([
104+
["\n\nsecond\nthird", "second third"],
105+
["Fix\nlogin bug", "Fix login bug"],
106+
])("collapses whitespace in %j", (input, expected) => {
107+
expect(fallbackTitle(input)).toBe(expected)
105108
})
106109

107110
it("caps the fallback at the character limit", () => {
@@ -143,7 +146,7 @@ describe("generateSessionTitle", () => {
143146
expect(applied).toEqual([{ taskId: "task-1", title: "Fix Login Bug" }])
144147
})
145148

146-
it("falls back to the first message line when the LLM fails", async () => {
149+
it("falls back to the start of the message when the LLM fails", async () => {
147150
const recorder = createCompletePromptRecorder(async () => {
148151
throw new Error("api down")
149152
})
@@ -163,9 +166,33 @@ describe("generateSessionTitle", () => {
163166
completePrompt: recorder.fn,
164167
})
165168

166-
expect(applied).toEqual([{ taskId: "task-2", title: "first line of the request" }])
169+
expect(applied).toEqual([{ taskId: "task-2", title: "first line of the request second line" }])
167170
})
168171

172+
it.each([[""], [" "], ['""'], ["Title:"]])(
173+
"falls back to the message start when the LLM returns the unusable response %j",
174+
async (response) => {
175+
const recorder = createCompletePromptRecorder(async () => response)
176+
const applied: ApplyTitleCall[] = []
177+
const provider = createTitleProvider({
178+
applySessionTitle: async (taskId, title) => {
179+
applied.push({ taskId, title })
180+
return "applied"
181+
},
182+
})
183+
184+
await generateSessionTitle({
185+
provider,
186+
taskId: "task-unusable",
187+
userMessage: "Short first line\nwith more text after it that goes on and on",
188+
apiConfiguration: testApiConfiguration,
189+
completePrompt: recorder.fn,
190+
})
191+
192+
expect(applied).toEqual([{ taskId: "task-unusable", title: "Short first line with more text after it t" }])
193+
},
194+
)
195+
169196
it("falls back when the LLM times out", async () => {
170197
vi.useFakeTimers()
171198
try {

‎src/core/task/session-title.ts‎

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,6 @@ Output only the title.
1515
1616
`
1717

18-
/**
19-
* Thrown inside `atomicReadAndUpdate` when the item already carries a title,
20-
* so the auto-title path can detect the conflict without performing a
21-
* pointless disk write of an unchanged record.
22-
*/
23-
export class SessionTitleConflictError extends Error {
24-
constructor(taskId: string) {
25-
super(`[session-title] task ${taskId} already has a title; keeping it`)
26-
this.name = "SessionTitleConflictError"
27-
}
28-
}
29-
3018
/** Outcome of `ClineProvider.applySessionTitle`. */
3119
export type SessionTitleApplyResult = "applied" | "conflict"
3220

@@ -63,16 +51,10 @@ export function extractLlmTitle(raw: string): string | undefined {
6351
return title.length > 0 ? title : undefined
6452
}
6553

66-
/** First non-empty line of the sanitized message, capped at 42 characters. */
54+
/** Start of the sanitized message with whitespace collapsed, capped at 42 characters. */
6755
export function fallbackTitle(sanitized: string): string | undefined {
68-
const firstLine = sanitized
69-
.split("\n")
70-
.find((line) => line.trim().length > 0)
71-
?.trim()
72-
if (!firstLine) {
73-
return undefined
74-
}
75-
return firstLine.length > FALLBACK_TITLE_MAX_CHARS ? firstLine.slice(0, FALLBACK_TITLE_MAX_CHARS) : firstLine
56+
const collapsed = sanitized.replace(/\s+/g, " ").trim()
57+
return collapsed ? collapsed.slice(0, FALLBACK_TITLE_MAX_CHARS).trim() : undefined
7658
}
7759

7860
type CompletePromptFn = typeof singleCompletionHandler

‎src/core/webview/ClineProvider.ts‎

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ import {
7676
providerIdentifiers,
7777
} from "@roo-code/types"
7878
import { RateLimitClock, createRateLimitClock } from "../task/RateLimitClock"
79-
import { SessionTitleConflictError, type SessionTitleApplyResult } from "../task/session-title"
79+
import type { SessionTitleApplyResult } from "../task/session-title"
8080
import { TaskRegistry } from "../task/TaskRegistry"
8181
import { TaskScheduler } from "../task/TaskScheduler"
8282
import {
@@ -2835,7 +2835,7 @@ export class ClineProvider
28352835
/*
28362836
* Applies an auto-generated session title, but only while the item still
28372837
* carries its default name. The title check runs inside the store's
2838-
* read-modify-write lock, so a manual rename that lands while a title is
2838+
* lock and re-reads the title on disk, so a manual rename that lands while a title is
28392839
* generating always wins and is never overwritten. Returns "conflict"
28402840
* when nothing was applied (an existing title won, or the title was
28412841
* blank); throws on transient write failures so the caller can retry.
@@ -2846,18 +2846,9 @@ export class ClineProvider
28462846
return "conflict"
28472847
}
28482848

2849-
try {
2850-
await this.taskHistoryStore.atomicReadAndUpdate(id, (historyItem) => {
2851-
if (historyItem.title) {
2852-
throw new SessionTitleConflictError(id)
2853-
}
2854-
return { ...historyItem, title: trimmed }
2855-
})
2856-
} catch (error) {
2857-
if (error instanceof SessionTitleConflictError) {
2858-
return "conflict"
2859-
}
2860-
throw error
2849+
const result = await this.taskHistoryStore.setTitleIfUnset(id, trimmed)
2850+
if (result === "conflict") {
2851+
return "conflict"
28612852
}
28622853

28632854
this.recentTasksCache = undefined
@@ -3679,7 +3670,7 @@ export class ClineProvider
36793670
* Now delegates to TaskHistoryStore for per-task file persistence.
36803671
*
36813672
* This writer never owns the `title` field: `title` is written only by the title-owning paths
3682-
* (`renameTaskWithId` and `applySessionTitle`, both via `taskHistoryStore.atomicReadAndUpdate`).
3673+
* (`renameTaskWithId` via `atomicReadAndUpdate` and `applySessionTitle` via `setTitleIfUnset`).
36833674
* Callers here build items from snapshots read before taking the store lock, so any title they
36843675
* carry can be stale.
36853676
*

0 commit comments

Comments
 (0)