Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions integration-tests/cli/qwen-serve-routes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ describe('qwen serve — capabilities envelope', () => {
'session_create',
'session_scope_override',
'session_load',
'session_resume',
'unstable_session_resume',
'session_list',
'session_prompt',
Expand Down
6 changes: 6 additions & 0 deletions packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6763,6 +6763,12 @@ class QwenAgent implements Agent {
// <available_skills> at cold start.
buildDisabledSkillNamesProvider(this.settings),
);
// ACP sessions run with piped stdio (non-TTY), so the default
// interactive-based gating disables file checkpointing. Enable it
// explicitly so /rewind works across daemon session resume.
if (typeof config.enableFileCheckpointing === 'function') {
config.enableFileCheckpointing();
}
// Inject the workspace-shared MCP transport pool BEFORE
// `config.initialize()` so the ToolRegistry picks it up.
if (
Expand Down
14 changes: 11 additions & 3 deletions packages/cli/src/acp-integration/session/Session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,12 @@ describe('Session', () => {
.fn()
.mockReturnValue(mockBackgroundShellRegistry),
getMonitorRegistry: vi.fn().mockReturnValue(mockMonitorRegistry),
getFileHistoryService: vi.fn().mockReturnValue({
Comment thread
doudouOUC marked this conversation as resolved.
Comment thread
doudouOUC marked this conversation as resolved.
makeSnapshot: vi.fn().mockResolvedValue(undefined),
getSnapshots: vi.fn().mockReturnValue([]),
restoreFromSnapshots: vi.fn(),
rewind: vi.fn(),
}),
Comment thread
doudouOUC marked this conversation as resolved.
Comment thread
doudouOUC marked this conversation as resolved.
} as unknown as Config;

Comment thread
doudouOUC marked this conversation as resolved.
mockClient = {
Expand Down Expand Up @@ -446,9 +452,11 @@ describe('Session', () => {
expect(result).toEqual({ targetTurnIndex: 1, apiTruncateIndex: 2 });
expect(mockChat.truncateHistory).toHaveBeenCalledWith(2);
expect(mockChat.stripThoughtsFromHistory).toHaveBeenCalled();
expect(mockChatRecordingService.rewindRecording).toHaveBeenCalledWith(1, {
truncatedCount: 2,
});
expect(mockChatRecordingService.rewindRecording).toHaveBeenCalledWith(
Comment thread
doudouOUC marked this conversation as resolved.
1,
{ truncatedCount: 2 },
[],
);
});

it('preserves startup context when rewinding to the first user turn', () => {
Expand Down
38 changes: 35 additions & 3 deletions packages/cli/src/acp-integration/session/Session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -614,9 +614,20 @@ export class Session implements SessionContext {
chat.truncateHistory(apiTruncateIndex);
chat.stripThoughtsFromHistory();

this.config.getChatRecordingService()?.rewindRecording(targetTurnIndex, {
truncatedCount: Math.max(0, apiHistory.length - apiTruncateIndex),
});
const fileHistoryService = this.config.getFileHistoryService();
const survivingSnapshots = fileHistoryService
.getSnapshots()
.slice(0, targetTurnIndex + 1);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] restoreFromSnapshots(survivingSnapshots) replaces in-memory state but never calls cleanupOrphanedBackups for the removed snapshots' backup files. The TUI path uses fileHistoryService.rewind() which calls cleanupOrphanedBackups(removed) explicitly. Over repeated ACP rewinds in long-lived daemon sessions, orphaned backup files accumulate on disk.

Consider exposing cleanupOrphanedBackups and calling it here after restoreFromSnapshots, or having restoreFromSnapshots internally detect and clean up orphans:

const allSnapshots = fileHistoryService.getSnapshots();
const survivingSnapshots = allSnapshots.slice(0, targetTurnIndex + 1);
const removed = allSnapshots.slice(targetTurnIndex + 1);
fileHistoryService.restoreFromSnapshots(survivingSnapshots);
fileHistoryService.cleanupOrphanedBackups(removed);

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed — orphaned backup cleanup deferred in wenshao review. Bounded by MAX_SNAPSHOTS, cleaned up by subsequent makeSnapshot calls.

fileHistoryService.restoreFromSnapshots(survivingSnapshots);
Comment thread
doudouOUC marked this conversation as resolved.
Comment thread
doudouOUC marked this conversation as resolved.
Comment thread
doudouOUC marked this conversation as resolved.
Comment thread
doudouOUC marked this conversation as resolved.

this.config
.getChatRecordingService()
?.rewindRecording(
targetTurnIndex,
{ truncatedCount: Math.max(0, apiHistory.length - apiTruncateIndex) },
survivingSnapshots,
);

return { targetTurnIndex, apiTruncateIndex };
}
Expand Down Expand Up @@ -1054,6 +1065,27 @@ export class Session implements SessionContext {
}
}

// Snapshot file state before this turn (mirrors the makeSnapshot
Comment thread
doudouOUC marked this conversation as resolved.
// block in GeminiClient.sendMessageStream). Placed after
// slash-command and hook early-returns so locally handled commands
// don't create phantom snapshots that desync the snapshot index.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This makeSnapshot + recordFileHistorySnapshot block (lines 1012–1028) is nearly identical to the one in packages/core/src/core/client.ts:1624-1643 — same 15-line structure with identical nested try/catch and identical error messages. The comment above even acknowledges it "mirrors the makeSnapshot block in GeminiClient.sendMessageStream." The two sites already differ slightly (this one caches the service in a local, client.ts calls the getter twice), and they will silently diverge further over time.

Consider extracting a shared helper:

async function snapshotAndRecordFileHistory(
  config: Config,
  promptId: string,
): Promise<void> {
  try {
    const fhs = config.getFileHistoryService();
    await fhs.makeSnapshot(promptId);
    try {
      const latest = fhs.getSnapshots().at(-1);
      if (latest) {
        config.getChatRecordingService()?.recordFileHistorySnapshot(latest);
      }
    } catch (e) {
      debugLogger.error(`FileHistory: recordSnapshot failed: ${e}`);
    }
  } catch (e) {
    debugLogger.error(`FileHistory: makeSnapshot failed: ${e}`);
  }
}

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed — two call sites with different contexts don't justify extraction. Addressed in multiple prior review rounds.

try {
const fileHistoryService = this.config.getFileHistoryService();
await fileHistoryService.makeSnapshot(promptId);
try {
const latestSnapshot = fileHistoryService.getSnapshots().at(-1);
if (latestSnapshot) {
this.config
.getChatRecordingService()
?.recordFileHistorySnapshot(latestSnapshot);
}
} catch (e) {
debugLogger.error(`FileHistory: recordSnapshot failed: ${e}`);
}
} catch (e) {
debugLogger.error(`FileHistory: makeSnapshot failed: ${e}`);
}

// Prepend session-level system reminders (plan mode / subagent /
// arena) so the model sees them, matching the behaviour of
// `GeminiClient.sendMessageStream` in the CLI/TUI path. Without this,
Expand Down
6 changes: 3 additions & 3 deletions packages/cli/src/serve/capabilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ export const SERVE_CAPABILITY_REGISTRY = {
session_create: { since: 'v1' },
session_scope_override: { since: 'v1' },
session_load: { since: 'v1' },
// ACP backs this with `connection.unstable_resumeSession`. Surface
// the unstable prefix so clients don't pin against a `v1` shape that
// the underlying ACP method may still change.
session_resume: { since: 'v1' },
Comment thread
doudouOUC marked this conversation as resolved.
// Deprecated alias — kept until @agentclientprotocol/sdk graduates
// the underlying ACP method from unstable_resumeSession to resumeSession.
unstable_session_resume: { since: 'v1' },
session_list: { since: 'v1' },
session_prompt: { since: 'v1' },
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/serve/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ const EXPECTED_STAGE1_FEATURES = [
'session_create',
'session_scope_override',
'session_load',
'session_resume',
'unstable_session_resume',
'session_list',
'session_prompt',
Expand Down
28 changes: 24 additions & 4 deletions packages/cli/src/ui/AppContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,11 @@ export const AppContainer = (props: AppContainerProps) => {
);

// Additional hooks moved from App.tsx
const { stats: sessionStats, startNewSession } = useSessionStats();
const {
stats: sessionStats,
startNewSession,
seedPromptCount,
} = useSessionStats();
const logger = useLogger(config.storage, sessionStats.sessionId);
const branchName = useGitBranchName(config.getTargetDir());
const worktreeSession = useWorktreeSession(config);
Expand Down Expand Up @@ -549,6 +553,15 @@ export const AppContainer = (props: AppContainerProps) => {
);
historyManager.loadHistory(historyItems);

// Seed the prompt counter from the resumed conversation so new
// promptIds don't collide with restored file history snapshots.
const userTurnCount = resumedSessionData.conversation.messages.filter(
Comment thread
doudouOUC marked this conversation as resolved.
(m) => m.type === 'user' && m.subtype !== 'mid_turn_user_message',
).length;
if (userTurnCount > 0) {
seedPromptCount(userTurnCount);
Comment thread
doudouOUC marked this conversation as resolved.
}

// Re-arm any `/goal` that was active when the prior session ended.
Comment thread
doudouOUC marked this conversation as resolved.
try {
restoreGoalFromHistory(historyItems, config, historyManager.addItem);
Expand Down Expand Up @@ -2622,9 +2635,16 @@ export const AppContainer = (props: AppContainerProps) => {
Date.now(),
);

config.getChatRecordingService()?.rewindRecording(targetTurnIndex, {
truncatedCount: originalLength - truncatedUi.length,
});
config.getChatRecordingService()?.rewindRecording(
targetTurnIndex,
{ truncatedCount: originalLength - truncatedUi.length },
!hasRestoreFailure
? config
.getFileHistoryService()
.getSnapshots()
.slice(0, targetTurnIndex + 1)
: undefined,
);
Comment thread
doudouOUC marked this conversation as resolved.
}

Comment thread
doudouOUC marked this conversation as resolved.
// Show file restore result after conversation truncation so the
Expand Down
11 changes: 10 additions & 1 deletion packages/cli/src/ui/contexts/SessionContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ interface SessionStatsContextValue {
startNewSession: (sessionId: string) => void;
startNewPrompt: () => void;
getPromptCount: () => number;
seedPromptCount: (count: number) => void;
}

// --- Context Definition ---
Expand Down Expand Up @@ -271,14 +272,22 @@ export const SessionStatsProvider: React.FC<{
[stats.promptCount],
);

const seedPromptCount = useCallback((count: number) => {
setStats((prevState) => ({
...prevState,
promptCount: Math.max(prevState.promptCount, count),
}));
}, []);

const value = useMemo(
() => ({
stats,
startNewSession,
startNewPrompt,
getPromptCount,
seedPromptCount,
}),
[stats, startNewSession, startNewPrompt, getPromptCount],
[stats, startNewSession, startNewPrompt, getPromptCount, seedPromptCount],
);

return (
Expand Down
16 changes: 15 additions & 1 deletion packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1151,7 +1151,7 @@ export class Config {
private fileDiscoveryService: FileDiscoveryService | null = null;
private sessionService: SessionService | undefined = undefined;
private chatRecordingService: ChatRecordingService | undefined = undefined;
private readonly fileCheckpointingEnabled: boolean;
private fileCheckpointingEnabled: boolean;
private fileHistoryService: FileHistoryService | undefined;
private readonly proxy: string | undefined;
private readonly cwd: string;
Expand Down Expand Up @@ -3604,13 +3604,27 @@ export class Config {
return this.fileCheckpointingEnabled;
}

enableFileCheckpointing(): void {
Comment thread
doudouOUC marked this conversation as resolved.
this.fileCheckpointingEnabled = true;
this.fileHistoryService = undefined;
}

getFileHistoryService(): FileHistoryService {
if (!this.fileHistoryService) {
this.fileHistoryService = new FileHistoryService(
this.sessionId,
this.fileCheckpointingEnabled,
this.cwd,
);
const snapshots = this.sessionData?.fileHistorySnapshots;
Comment thread
doudouOUC marked this conversation as resolved.
if (snapshots?.length && this.fileHistoryService.isEnabled()) {
this.fileHistoryService.restoreFromSnapshots(snapshots);
void this.fileHistoryService.validateRestoredSnapshots().catch((e) => {
this.debugLogger.error(
`FileHistory: validateRestoredSnapshots failed: ${e}`,
);
});
}
Comment thread
doudouOUC marked this conversation as resolved.
}
Comment thread
doudouOUC marked this conversation as resolved.
return this.fileHistoryService;
}
Expand Down
13 changes: 13 additions & 0 deletions packages/core/src/core/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1661,6 +1661,19 @@ export class GeminiClient {
if (messageType === SendMessageType.UserQuery) {
try {
await this.config.getFileHistoryService().makeSnapshot(prompt_id);
try {
Comment thread
doudouOUC marked this conversation as resolved.
const latestSnapshot = this.config
.getFileHistoryService()
.getSnapshots()
.at(-1);
if (latestSnapshot) {
Comment thread
doudouOUC marked this conversation as resolved.
this.config
.getChatRecordingService()
?.recordFileHistorySnapshot(latestSnapshot);
}
} catch (e) {
debugLogger.error(`FileHistory: recordSnapshot failed: ${e}`);
}
} catch (e) {
debugLogger.error(`FileHistory: makeSnapshot failed: ${e}`);
}
Expand Down
50 changes: 47 additions & 3 deletions packages/core/src/services/chatRecordingService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ import type {
import type { Status } from '../core/coreToolScheduler.js';
import type { AgentResultDisplay, FileDiff } from '../tools/tools.js';
import type { UiEvent } from '../telemetry/uiTelemetry.js';
import type {
FileHistorySnapshot,
SerializedFileHistorySnapshot,
} from './fileHistoryService.js';
import { serializeSnapshot } from './fileHistoryService.js';

const debugLogger = createDebugLogger('CHAT_RECORDING');

Expand Down Expand Up @@ -235,7 +240,8 @@ export interface ChatRecord {
| 'custom_title'
| 'rewind'
| 'agent_bootstrap'
| 'agent_launch_prompt';
| 'agent_launch_prompt'
| 'file_history_snapshot';
/** Working directory at time of message */
cwd: string;
/** CLI version for compatibility tracking */
Expand Down Expand Up @@ -281,7 +287,8 @@ export interface ChatRecord {
| CustomTitleRecordPayload
| NotificationRecordPayload
| RewindRecordPayload
| AgentBootstrapRecordPayload;
| AgentBootstrapRecordPayload
| FileHistorySnapshotRecordPayload;

/** Background subagent that produced this record (e.g. "explore-7f3c"). */
agentId?: string;
Expand Down Expand Up @@ -428,6 +435,14 @@ export interface RewindRecordPayload {
truncatedCount: number;
}

/**
* Stored payload for file history snapshot persistence.
* Each entry records one or more snapshots for session resume.
*/
export interface FileHistorySnapshotRecordPayload {
snapshots: SerializedFileHistorySnapshot[];
}

/**
* Service for recording the current chat session to disk.
*
Expand Down Expand Up @@ -1140,7 +1155,11 @@ export class ChatRecordingService {
* nothing before it), 1 means keep the first user turn, etc.
* @param payload Additional metadata to persist with the rewind record.
*/
rewindRecording(targetTurnIndex: number, payload: RewindRecordPayload): void {
rewindRecording(
targetTurnIndex: number,
payload: RewindRecordPayload,
survivingFileHistorySnapshots?: FileHistorySnapshot[],
): void {
try {
// Re-root: point back to the record just before the target user turn.
this.lastRecordUuid = this.turnParentUuids[targetTurnIndex] ?? null;
Expand All @@ -1161,6 +1180,12 @@ export class ChatRecordingService {
};

this.appendRecord(record);

// Re-record surviving file history snapshots on the active branch so
// they are visible to reconstructHistory on resume.
if (survivingFileHistorySnapshots?.length) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The new 3-arg branch of rewindRecording (re-recording surviving snapshots on the active branch after rewind) has no test coverage. The existing test at line 669 uses the old 2-arg call, and Session.test.ts passes [] as the third arg — neither exercises the if (survivingFileHistorySnapshots?.length) branch.

If this branch breaks, resumed sessions lose file history state for surviving turns after rewind.

Suggested test: call rewindRecording(targetTurnIndex, payload, [mockSnapshot]) and verify the batch record is written alongside the rewind record.

— qwen3.7-max via Qwen Code /review

this.recordFileHistorySnapshotBatch(survivingFileHistorySnapshots);
}
} catch (error) {
debugLogger.error('Error saving rewind record:', error);
}
Expand Down Expand Up @@ -1346,4 +1371,23 @@ export class ChatRecordingService {
debugLogger.error('Error saving attribution snapshot:', error);
}
}

Comment thread
doudouOUC marked this conversation as resolved.
Comment thread
doudouOUC marked this conversation as resolved.
recordFileHistorySnapshot(snapshot: FileHistorySnapshot): void {
Comment thread
doudouOUC marked this conversation as resolved.
Comment thread
doudouOUC marked this conversation as resolved.
Comment thread
doudouOUC marked this conversation as resolved.
this.recordFileHistorySnapshotBatch([snapshot]);
}

recordFileHistorySnapshotBatch(snapshots: FileHistorySnapshot[]): void {
if (snapshots.length === 0) return;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] recordFileHistorySnapshot() and recordFileHistorySnapshotBatch() — the sole mechanism for persisting file history snapshots to JSONL — have no test coverage. A regression in record structure (wrong subtype, missing systemPayload, bad serialization) would silently break /rewind across sessions.

Suggested tests:

  1. recordFileHistorySnapshot writes a single-entry batch record with correct subtype: 'file_history_snapshot' and serialized systemPayload
  2. recordFileHistorySnapshotBatch([]) is a no-op
  3. Write errors are caught without throwing

— qwen3.7-max via Qwen Code /review

try {
const record: ChatRecord = {
...this.createBaseRecord('system'),
type: 'system',
subtype: 'file_history_snapshot',
systemPayload: { snapshots: snapshots.map(serializeSnapshot) },
};
this.appendRecord(record);
} catch (error) {
debugLogger.error('Error saving file history snapshot batch:', error);
}
}
}
Loading
Loading