-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(session): preserve source titles when branching #9764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
fa1630e
7f24b47
8aac1c3
84aae48
22a7109
65ce1f1
b0651d5
3090125
1359f61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,6 +78,7 @@ import { | |
| subagentGenerator, | ||
| redactUrlCredentials, | ||
| computeUniqueBranchTitle, | ||
| normalizeDerivedBranchTitle, | ||
| BranchPointInvalidError, | ||
| parseGoalSnapshotV2, | ||
| parseGoalStateCause, | ||
|
|
@@ -1141,19 +1142,10 @@ function getLoadReplayPageSize(params: LoadSessionRequest): number | undefined { | |
| return value as number; | ||
| } | ||
|
|
||
| function deriveForkBaseName( | ||
| name: unknown, | ||
| recording: { getCurrentCustomTitle(): string | undefined } | undefined, | ||
| sessionId: string, | ||
| ): string { | ||
| if (typeof name === 'string' && name.trim().length > 0) { | ||
| return name.trim(); | ||
| } | ||
| const existingTitle = recording?.getCurrentCustomTitle(); | ||
| const stripped = existingTitle | ||
| ?.replace(/\s*\(Branch(?:\s+\d+)?\)\s*$/, '') | ||
| .trim(); | ||
| return stripped && stripped.length > 0 ? stripped : sessionId.slice(0, 8); | ||
| function normalizeRequestedBranchName(value: unknown): string | undefined { | ||
| if (typeof value !== 'string') return undefined; | ||
| const normalized = value.trim(); | ||
| return normalized || undefined; | ||
| } | ||
| function createHiddenWorkspaceMemoryConfig(config: Config): Config { | ||
| return new Proxy(config, { | ||
|
|
@@ -11078,11 +11070,31 @@ class QwenAgent implements Agent { | |
| const recording = sourceConfig.getChatRecordingService(); | ||
| const sessionService = sourceConfig.getSessionService(); | ||
|
|
||
| const baseName = deriveForkBaseName( | ||
| name, | ||
| recording, | ||
| sessionId, | ||
| ); | ||
| const requestedName = normalizeRequestedBranchName(name); | ||
| const sourceCustomTitle = | ||
| requestedName === undefined | ||
| ? recording?.getCurrentCustomTitle() | ||
| : undefined; | ||
| const persistedDisplayName = | ||
| requestedName === undefined && | ||
| sourceCustomTitle === undefined | ||
| ? await sessionService.getSessionDisplayName(sessionId) | ||
| : undefined; | ||
| const sourceDisplayName = | ||
| sourceCustomTitle ?? persistedDisplayName; | ||
| const derivedBaseName = sourceCustomTitle | ||
| ? normalizeDerivedBranchTitle(sourceCustomTitle) | ||
| : sourceDisplayName; | ||
|
Comment on lines
+11083
to
+11087
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] When the source session's current custom title is exactly the empty string, this derivation chain leaks it all the way to Witness (scratch-tree probe against the built helpers at this commit): Normalize the derived base the way the requested name is already normalized (and apply the same treatment to const baseName =
requestedName ??
(derivedBaseName?.trim() || undefined) ??
sessionId.slice(0, 8);中文说明当源会话当前的自定义标题恰好为空字符串时,这条派生链会把它一路泄漏到 见证(在本提交的临时树中对已构建助手执行的探针):空自定义标题 → 建议按请求名称已有的归一化方式同样处理派生基础名(并对 — qwen3.8-max via Qwen Code /review (v0.22.0) |
||
| // A base that is empty, whitespace-only, or exactly a | ||
| // legacy `(Branch)`/`(Branch N)` token falls back to the | ||
| // session-id prefix here, while CLI /branch falls back to | ||
| // the first prompt. Deliberate: no picker name survives to | ||
| // anchor the family to, and one shared fallback would need | ||
| // a prompt-only display-name read on this route. | ||
| const baseName = | ||
| requestedName ?? | ||
| (derivedBaseName?.trim() || undefined) ?? | ||
| sessionId.slice(0, 8); | ||
|
|
||
| const title = await computeUniqueBranchTitle( | ||
| baseName, | ||
|
|
@@ -11120,7 +11132,15 @@ class QwenAgent implements Agent { | |
| const recording = sourceConfig.getChatRecordingService(); | ||
| if (recording) await recording.flush(); | ||
| const sessionService = sourceConfig.getSessionService(); | ||
| const title = deriveForkBaseName(name, recording, sessionId); | ||
| const requestedName = normalizeRequestedBranchName(name); | ||
| let title = requestedName; | ||
| if (title === undefined) { | ||
| const sourceCustomTitle = recording?.getCurrentCustomTitle(); | ||
| title = sourceCustomTitle | ||
| ? (normalizeDerivedBranchTitle(sourceCustomTitle) ?? | ||
| sessionId.slice(0, 8)) | ||
|
Comment on lines
+11139
to
+11141
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The side-task path's derived-title branch — no explicit Witness (scratch-tree mutant probe at this commit): Add one case alongside the existing ones: no 中文说明side-task 路径中“推导出的标题”分支——未提供显式 见证(在本提交的临时树中执行的变异探针):变异体(PR 前的 建议在现有用例旁补充一个:不提供 — qwen3.8-max via Qwen Code /review (v0.22.0) |
||
| : sessionId.slice(0, 8); | ||
|
Comment on lines
+11138
to
+11142
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This diff deletes the shared 中文说明本 diff 删除了共享的 — qwen3.8-max via Qwen Code /review (v0.22.0)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for raising this. I kept the side-task persisted-display fallback intentionally unchanged in
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decline upheld in this round's triage. The two derivations stay separate by design: side tasks keep 中文说明本轮分诊维持拒绝(Decline)。两份推导有意保持分离:side task 保留 |
||
| } | ||
|
Comment on lines
+11137
to
+11143
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] Same empty-normalization gap as the branch path above, with a worse outcome here: when the source's custom title is exactly 中文说明与上方分支路径相同的空归一化缺口,此处后果更重:当源会话的自定义标题恰好是 — qwen3.8-max via Qwen Code /review (v0.22.0)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in |
||
| const newSessionId = randomUUID(); | ||
| const fork = () => | ||
| sessionService.forkSession(sessionId, newSessionId, { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] This mock copies the real
normalizeDerivedBranchTitleregex body verbatim instead of reusing the implementation — the samevi.mockfactory already passes other pure helpers throughimportOriginal(e.g.parseInvocationContext,emptyGoalSnapshot). The adjacentcomputeUniqueBranchTitlestub deliberately simplifies to${baseName}(1)for determinism, but this mock simplifies nothing — it duplicates. This PR just rewrote the normalization scheme once ((Branch N)→(N)); the next such change will updatesessionService.tswithout this mock, and the ACP route tests will silently keep exercising the stale regex: expectations like'Source session(2)'→'Source session(1)'would validate the mock's old behaviour and stay green while the shipped function behaves differently — the integration tests would endorse a normalization that no longer exists.中文说明
这个 mock 逐字复制了真实
normalizeDerivedBranchTitle的正则实现,而不是复用真实实现——同一个vi.mock工厂已经通过importOriginal引入了其他纯函数(如parseInvocationContext、emptyGoalSnapshot)。旁边的computeUniqueBranchTitle桩是为了确定性而刻意简化为${baseName}(1),但这个 mock 没有任何简化——只是重复实现。本 PR 刚刚重写过一次归一化方案((Branch N)→(N));下次此类变更只会更新sessionService.ts而不会更新这个 mock,ACP 路由测试会继续悄悄验证过期的正则:像'Source session(2)'→'Source session(1)'这样的断言仍会通过,但验证的是 mock 的旧行为,而真实函数的行为已经不同——集成测试将为一个已不存在的归一化方案背书。— qwen3.8-max via Qwen Code /review (v0.22.0)