fix(core): restore file history snapshots on resume#4253
Conversation
| expect(snapshots[0]?.timestamp).toBeInstanceOf(Date); | ||
| expect( | ||
| snapshots[0]?.trackedFileBackups['/tmp/src/app.ts']?.backupTime, | ||
| ).toBeInstanceOf(Date); |
There was a problem hiding this comment.
[Critical] Test failure: backupTime assertion expects a Date instance but receives undefined.
The test "restores persisted file-history snapshots when resuming a session" fails because snapshots[0]?.trackedFileBackups['/tmp/src/app.ts']?.backupTime is undefined instead of a Date. This indicates that the snapshot restoration path through FileHistoryService.restoreFromSnapshots() is not properly hydrating the backupTime field when the input uses a string-typed-but-cast-as-Date value (as happens when snapshots are deserialized from JSONL).
This is the core feature of this PR — if the restoration test itself fails, the feature is not working as intended.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ); | ||
| } | ||
| if ( | ||
| !this.fileHistoryRestoredFromSessionData && |
There was a problem hiding this comment.
[Critical] restoreFromSnapshots() called without try/catch — a single malformed snapshot makes file history permanently broken for the session.
In getFileHistoryService(), the restoreFromSnapshots() call is not wrapped in try/catch. If it throws (e.g., corrupted trackedFileBackups from a malformed JSONL record), fileHistoryRestoredFromSessionData stays false while this.fileHistoryService has already been assigned. Every subsequent call to getFileHistoryService() will retry the restore and throw again — an unrecoverable persistent failure. This propagates to TUI render paths (DialogManager.tsx passes config.getFileHistoryService() as a prop) and tool execution, potentially crashing the RewindSelector dialog or blocking file operations.
| !this.fileHistoryRestoredFromSessionData && | |
| if ( | |
| !this.fileHistoryRestoredFromSessionData && | |
| this.sessionData?.fileHistorySnapshots | |
| ) { | |
| try { | |
| this.fileHistoryService.restoreFromSnapshots( | |
| this.sessionData.fileHistorySnapshots, | |
| ); | |
| } catch (e) { | |
| debugLogger.error( | |
| `FileHistory: Failed to restore snapshots from session data: ${e}`, | |
| ); | |
| } | |
| this.fileHistoryRestoredFromSessionData = true; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| @@ -315,9 +315,22 @@ export class FileHistoryService { | |||
| for (const [p, backup] of Object.entries(snapshot.trackedFileBackups)) { | |||
There was a problem hiding this comment.
[Critical] restoreFromSnapshots() lacks structural guards against malformed snapshot data.
The method iterates Object.entries(snapshot.trackedFileBackups) and spreads ...backup without checking whether snapshot, snapshot.trackedFileBackups, or backup are actually the expected types. A corrupted JSONL record (e.g., trackedFileBackups: null or a backup entry that is null instead of an object) would throw TypeErrors with no diagnostic context about which snapshot or file path caused the failure. Combined with the missing try/catch in the caller (config.ts), this becomes a persistent unrecoverable error.
| for (const [p, backup] of Object.entries(snapshot.trackedFileBackups)) { | |
| for (const snapshot of snapshots) { | |
| if (!snapshot?.trackedFileBackups || !snapshot?.promptId) continue; | |
| const trackedFileBackups: Record<string, FileHistoryBackup> = {}; | |
| for (const [p, backup] of Object.entries(snapshot.trackedFileBackups)) { | |
| if (!backup || typeof backup !== 'object') continue; | |
| const trackingPath = this.maybeShortenFilePath(p); | |
| trackedFiles.add(trackingPath); | |
| trackedFileBackups[trackingPath] = { | |
| ...backup, | |
| backupTime: | |
| backup.backupTime instanceof Date | |
| ? backup.backupTime | |
| : new Date(backup.backupTime), | |
| }; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| @@ -1241,7 +1241,14 @@ export class GeminiClient { | |||
|
|
|||
| if (messageType === SendMessageType.UserQuery) { | |||
| try { | |||
There was a problem hiding this comment.
[Suggestion] The catch block's error message is misleading: it always logs "makeSnapshot failed" but the try block now also calls getFileHistoryService() (which can throw from restoreFromSnapshots), getSnapshots(), and recordFileHistorySnapshot(). Any exception from those operations will be misattributed to makeSnapshot, sending anyone debugging the issue on a wild goose chase.
| try { | |
| } catch (e) { | |
| debugLogger.error( | |
| `FileHistory: snapshot/record cycle failed for ${prompt_id}: ${e}`, | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| function extractFileHistorySnapshots( |
There was a problem hiding this comment.
[Suggestion] extractFileHistorySnapshots() validates payload?.version !== 1 || !snapshot but never checks that snapshot has the required fields (promptId, trackedFileBackups, timestamp) or that trackedFileBackups is an object. A record with { version: 1, snapshot: { promptId: "x" } } (missing trackedFileBackups) would pass through, reach restoreFromSnapshots(), and throw a TypeError on Object.entries(undefined). Validation closer to the ingestion point improves debuggability.
| function extractFileHistorySnapshots( | |
| const snapshot = payload?.snapshot; | |
| if ( | |
| payload?.version !== 1 || | |
| !snapshot || | |
| !snapshot.promptId || | |
| !snapshot.trackedFileBackups || | |
| typeof snapshot.trackedFileBackups !== 'object' | |
| ) { | |
| continue; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| try { | ||
| const fileHistoryService = this.config.getFileHistoryService(); | ||
| await fileHistoryService.makeSnapshot(prompt_id); | ||
| const latestSnapshot = fileHistoryService.getSnapshots().at(-1); |
There was a problem hiding this comment.
[Suggestion] The new snapshot recording integration (lines 1244-1251) has zero test coverage in client.test.ts. This is the central hook where makeSnapshot output is persisted to the session JSONL log — the success path (promptId matches), the mismatch path (snapshot was a no-op), and the error path are all untested. Consider adding at least a unit test that verifies recordFileHistorySnapshot is called with the correct snapshot after makeSnapshot.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| const trackingPath = this.maybeShortenFilePath(p); | ||
| trackedFiles.add(trackingPath); |
There was a problem hiding this comment.
[Suggestion] Path traversal risk from session JSONL data. The new data flow lets file paths from session JSONL records enter restoreFromSnapshots() → maybeShortenFilePath() and ultimately into trackedFiles set. Paths containing ../ sequences that survive maybeShortenFilePath (which passes non-absolute paths through unchanged) would later be expanded via maybeExpandFilePath (join(this.cwd, filePath)) and used in filesystem ops (stat, readFile, copyFile). While the JSONL files are user-owned, consider adding a resolved.startsWith(this.cwd) check in restoreFromSnapshots as defense-in-depth to prevent restored paths from escaping the project root.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
收回 01:32 的 APPROVED——本地实测后发现 PR 不是看着的 +12631/-12279，而是 ~406 行真实改动 + ~12k 行 EOL 污染。完整对账：
1. 行尾污染（阻塞）
4 个核心文件被整文件从 LF 转成 CRLF：
| 文件 | raw diff | 真实语义改动 | CRLF 占比 |
|---|---|---|---|
client.ts |
3415 行 | 15 行 | 1860/1860 全部 CRLF |
config.ts |
6429 行 | 85 行 | 大量 |
sessionService.ts |
2384 行 | 47 行 | 1356/1356 全部 CRLF |
fileHistoryService.ts |
1292 行 | 50 行 | 760/760 全部 CRLF |
核实命令：grep -c $'\r$' packages/core/src/core/client.ts 返回 1860（=该文件总行数）。
后果：
- git blame 全部断在这条 commit
- 此后跟 main 上每一条改这些文件的 PR 都冲突（已经实际触发
config.ts/config.test.ts/client.ts/sessionService.test.ts4 个冲突） - 历史导航污染 ~12k 行
看起来是作者本地编辑器在 Windows 上把行尾改了。修法：
git config core.autocrlf input # 或 false
dos2unix packages/core/src/{core/client.ts,config/config.ts,services/sessionService.ts,services/fileHistoryService.ts}
git add -A && git commit --amend --no-edit
git rebase origin/main顺便加一个 .gitattributes 杜绝重发：
* text=auto eol=lf
*.{ts,tsx,js,mjs,cjs,json,md,yml,yaml} text eol=lf
2. CI 没跑
HEAD fd5c653af 上 check-runs 总数 0，status pending。需要 maintainer trigger 一下 workflow（first-time 贡献者 workflow gate）。
3. 内容本身
本地实测 256/256 vitest passed，tsc clean。restoreFromSnapshots 加的结构守卫（toValidDate + isFileHistoryBackupRecord + 字段校验）正确收掉了 17:24 那条 [Critical]。
但 post-approve 我留的两条 [Suggestion] 都还开着，见下面 inline。
优先级：阻塞项 = EOL 污染；EOL 修完之后 CI 重跑绿 + 两条 [Suggestion] 收掉就可以重新 approve。
| COMPRESSION_PRESERVE_THRESHOLD, | ||
| COMPRESSION_TOKEN_THRESHOLD, | ||
| }; | ||
| /** |
There was a problem hiding this comment.
[Blocker] 整个文件 1860/1860 行都是 CRLF（grep -c $'\\r$' 实测）。和 main 比真实语义改动只有 15 行，但 raw diff 是 3415 行。这条 commit 落地后：
- git blame 完全断（每一行都看似在 fix(core): restore file history snapshots on resume #4253 被改过）
- 此后改这个文件的每条 PR 都会跟 main 冲突
- 历史导航不可用
建议在本地 PR 分支上：
dos2unix packages/core/src/core/client.ts
git add packages/core/src/core/client.ts && git commit --amend --no-edit对 config.ts / sessionService.ts / fileHistoryService.ts 三个文件做同样的操作。仓库根加 .gitattributes * text=auto eol=lf 防止后续再发生。
| try { | ||
| const fileHistoryService = this.config.getFileHistoryService(); | ||
| await fileHistoryService.makeSnapshot(prompt_id); | ||
| const latestSnapshot = fileHistoryService.getSnapshots().at(-1); |
There was a problem hiding this comment.
[Suggestion, post-approve carry-over] 新加的 snapshot 录入 hook（L1244-1252）在 client.test.ts 里零覆盖。这是 makeSnapshot → JSONL 持久化的中心 hook，断了的话整个 resume 路径都不能恢复 snapshot 状态。
至少补一个 client.test.ts 用例：
- 设
messageType = UserQuery，stubfileHistoryService.makeSnapshot返回一个带promptId的 snapshot - 断言
chatRecordingService.recordFileHistorySnapshot被以该 snapshot 调用 - 再加一个 negative case：
makeSnapshot抛错时不传播到调用方（被 try/catch 兜住），但debugLogger.error记录了
EOL 修完之后顺手补，不必单独再发一轮。
| } | ||
|
|
||
| const trackingPath = this.maybeShortenFilePath(p); | ||
| trackedFiles.add(trackingPath); |
There was a problem hiding this comment.
[Suggestion, post-approve carry-over] 路径遍历 defense-in-depth：session JSONL 数据 → restoreFromSnapshots() → maybeShortenFilePath() → trackedFiles Set → maybeExpandFilePath()（join(this.cwd, filePath)）→ filesystem ops (stat/readFile/copyFile)。
含 ../ 的相对路径会被 maybeShortenFilePath 原样穿过，后面 join 出来的路径可以逃出 cwd。JSONL 是用户自有，attack surface 窄，但加一行 defense-in-depth 防 future regression 很便宜：
const trackingPath = this.maybeShortenFilePath(p);
const resolved = path.resolve(this.cwd, trackingPath);
if (!resolved.startsWith(this.cwd + path.sep) && resolved !== this.cwd) {
debugLogger.warn(`Skipping snapshot path outside cwd: ${p}`);
continue;
}
trackedFiles.add(trackingPath);EOL 修完之后顺手补即可。
Alexxigang
force-pushed
the
fix/resume-file-history-snapshots
branch
from
fd5c653 to
ef246a2
Compare
|
Rebased this PR onto the current main branch, resolved the merge conflicts, and force-pushed the updated branch. Re-ran the targeted core validation on the rebased branch: |
| import { CronScheduler } from '../services/cronScheduler.js'; | ||
|
|
||
| // Tools — only lightweight imports; tool classes are lazy-loaded via dynamic import | ||
| // Tools �?only lightweight imports; tool classes are lazy-loaded via dynamic import |
There was a problem hiding this comment.
[Critical] Systematic UTF-8 encoding corruption across ~40 lines in 3 files (config.ts, fileHistoryService.test.ts, sessionService.test.ts). The third byte of multi-byte UTF-8 sequences is replaced with 0x3F (ASCII ?): em-dash — (U+2014, e2 80 94) → e2 80 3f, arrow → (U+2192) → e2 86 92 3f, ≥ (U+2265) → e2 89 65 3f. Verified at byte level via xxd. This is not a rendering artifact — the files are actually corrupted on disk, likely caused by a tool/editor that processed files through a non-UTF-8-aware pipeline.
Impact: comments become mojibake, CI encoding checks would fail, and readability is permanently degraded.
Fix: revert all encoding-only changes and re-apply the semantic changes on top of unmodified source text. On the PR branch:
git checkout origin/main -- packages/core/src/config/config.ts packages/core/src/services/fileHistoryService.test.ts packages/core/src/services/sessionService.test.ts
# Then re-apply only the semantic changes— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| `FileHistory: Failed to restore snapshots from session data: ${e}`, | ||
| ); | ||
| } | ||
| this.fileHistoryRestoredFromSessionData = true; |
There was a problem hiding this comment.
[Suggestion] fileHistoryRestoredFromSessionData = true is set unconditionally after the try/catch, so if restoreFromSnapshots throws (transient I/O error, corrupted data), the flag prevents any retry on subsequent getFileHistoryService() calls — permanently disabling file-history restoration for the session with only a debug-level log.
| this.fileHistoryRestoredFromSessionData = true; | |
| this.fileHistoryService.restoreFromSnapshots( | |
| this.sessionData.fileHistorySnapshots, | |
| ); | |
| this.fileHistoryRestoredFromSessionData = true; | |
| } catch (e) { | |
| this.debugLogger.error( | |
| `FileHistory: Failed to restore snapshots from session data: ${e}`, | |
| ); | |
| } |
Move the flag assignment inside the try block so it only flips on success, allowing retry on the next access.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| return Number.isNaN(normalized.getTime()) ? null : normalized; | ||
| } | ||
|
|
||
| function isFileHistoryBackupRecord(value: unknown): value is FileHistoryBackup { |
There was a problem hiding this comment.
[Suggestion] The type guard isFileHistoryBackupRecord accepts any non-null object (!!value && typeof value === 'object'), including arrays and objects without any FileHistoryBackup fields. While downstream toValidDate(backup.backupTime) catches entries missing backupTime, an object with a valid backupTime but missing backupFileName would pass through and produce undefined in path resolution.
| function isFileHistoryBackupRecord(value: unknown): value is FileHistoryBackup { | |
| function isFileHistoryBackupRecord(value: unknown): value is FileHistoryBackup { | |
| return ( | |
| !!value && | |
| typeof value === 'object' && | |
| !Array.isArray(value) && | |
| 'version' in value && | |
| typeof (value as FileHistoryBackup).version === 'number' | |
| ); | |
| } |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| trackedFileBackups, | ||
| }); | ||
| } | ||
| this.state = { |
There was a problem hiding this comment.
[Suggestion] restoreFromSnapshots sets this.state.snapshots = migrated without enforcing the MAX_SNAPSHOTS = 100 cap. Eviction only runs inside makeSnapshot. If the JSONL contains more than 100 snapshot records (long session, data corruption), all are loaded into memory and the orphaned-backup cleanup that eviction triggers is never reached for the extras.
Consider capping before assignment:
if (migrated.length > MAX_SNAPSHOTS) {
migrated = migrated.slice(migrated.length - MAX_SNAPSHOTS);
}— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| }); | ||
|
|
||
| it('refreshes the telemetry session context with the new session ID', () => { | ||
| it('refreshes the telemetry session context with the new session ID', () => { |
There was a problem hiding this comment.
[Nice to have] Indentation regression: this it(...) line was re-indented from 4 spaces to 2 spaces, breaking alignment with sibling test cases in the same describe block. Likely an accidental edit during rebase.
| it('refreshes the telemetry session context with the new session ID', () => { | |
| it('refreshes the telemetry session context with the new session ID', () => { |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
2 participants
Summary
FileHistoryServicealready supports snapshot export/import, but the resume path never hydrated those snapshots back into the service, so resumed sessions lost file-history state./resumeand whether recording one snapshot per user turn matches the intended persistence model.Validation
cd packages/core npm run typecheck npm run build npx vitest run src/services/chatRecordingService.test.ts src/services/sessionService.test.ts src/config/config.test.ts src/core/client.test.ts npx eslint src/services/chatRecordingService.ts src/services/fileHistoryService.ts src/services/sessionService.ts src/config/config.ts src/core/client.ts src/services/chatRecordingService.test.ts src/services/sessionService.test.ts src/config/config.test.tsfile_history_snapshotsystem records; runtime snapshot recording path throughGeminiClientuser-query handling.Datefields restored, and adjacent core tests keep passing.npm run typecheckpassed,npm run buildpassed, 4 targeted test files passed (357 tests), and ESLint passed on all touched files.SessionService.loadSession()plusConfig.getFileHistoryService()restore flow.TYPECHECK_EXIT=0,BUILD_EXIT=0,TEST_EXIT=0,LINT_EXIT=0with 357 passing tests.Scope / Risk
file_history_snapshotsystem record per user-turn snapshot, which increases session-record size in exchange for correct resume behavior.Testing Matrix
Testing matrix notes:
packages/coreonly.Linked Issues / Bugs