Repository navigation
fix(acp): resolve session before config initialization and avoid same-minute filename collisions - #29463
Conversation
…-minute filename collisions
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a race condition in ACP mode where session initialization could overwrite existing session files if triggered within the same UTC minute. By reordering the session resolution logic and implementing a collision-avoidance mechanism for session filenames, the changes ensure that active session states are preserved during startup. Additionally, improved cleanup routines were added to remove temporary startup files, ensuring a cleaner session management lifecycle. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/M
|
There was a problem hiding this comment.
Code Review
This pull request refactors the session loading and initialization sequence in the ACP session manager to resolve the session before initializing the configuration. It also introduces collision handling for session filenames generated within the same UTC minute to prevent appending to or poisoning existing session files, and adds a best-effort cleanup of abandoned startup-only session files in the Gemini client. Corresponding unit tests have been added to verify these behaviors. There are no review comments, and I have no feedback to provide.
## Motivation
- The attachment proof accepted a PDF page marker and a rasterized
image of the page. Neither shows that the agent gave the model the
PDF.
- Native agent CLIs update themselves. An update can replace the
executable or the install of the developer, because an isolated
`HOME` does not move them.
- A native shell tool can lose the output of a command that exits
right after it writes. MiMo Code 0.1.15 does so.
- The settings wait looked for a `model` group. A running Fast Agent
or Amp agent has no such group, so the wait never ended.
- A reopened agent draws copied transcript rows while it still starts.
Those rows cannot prove a resume.
- The mock model server sent the tool calls of a Chat Completions
stream before the text. It picked the ambient scenario for the Goose
`/compact` request, which quotes the prompts in its system text.
- The controlled output command watched its release files with
`fs.watch`, which fails with EMFILE in the Codex sandbox.
- `exerciseSessionResume` returns a `NativeResumeResult` in place of
the request record. A caller that read the request must read the
fields of the result.
- Gemini CLI 0.62.0 cannot load a session in the UTC minute that the
file name of its archive states. A resume scenario must reopen the
stored session in a later minute.
- `exerciseNativeGoalPauseAndResume` requires the timing of the native
pause. Qwen Code pauses at once, and Grok Build pauses after the
running round. Each caller states its own timing.
## Modifications
### Attachment proof
- Require the exact source PDF in a typed PDF part of the current user
turn (`expectNativePdfPart`). Require the source image in a typed
image part (`expectNativeImagePart`).
- Read the typed parts of a request by its protocol and route:
- Anthropic Messages: PDF and image.
- Chat Completions: PDF and image.
- Kiro: PDF and image.
- Google: PDF and image.
- OpenAI Responses: image only.
- Cursor: image only.
- Amp: image only.
- Define the current turn as the user rows at the end of the history.
Skip these rows:
- Instruction rows.
- A user row that directly follows a tool row.
- A row that holds a tool result.
- Require canonical base64 and the same bytes as the source. A failure
states both lengths and both SHA-256 values.
- Accept a transcoded image only when the caller sets
`transcodedImageType`. The part must declare that type, hold bytes
of that format, and decode to the four quadrant colors.
- Add a `protocol` option to `exerciseAttachmentDelivery`. The helper
checks the request of the step that it queued.
- Let `expectNativeAttachmentProof` take an `ImageHandoff` and the
index of the step that holds the attachment turn.
- Export `scriptedRequest` and `PDF_PAGE_MARKER`.
- Add unit cases and probe cases that reject these forms:
- A page marker in text.
- A rasterized image of the page.
- Base64 in plain text.
- A wrong declared type.
- Non-canonical base64.
- A part in an earlier turn.
- A part in a system row.
- A part in a tool result.
### Mock model server and script
- Add the `midStream` flag to `MockModelError`. A Chat Completions
stream then sends one partial delta and an error payload. The route
refuses the flag for another protocol and for a request with no
stream.
- Read the system text for the scenario marker when no user text holds
one. The newest marker wins.
- Send the tool calls of a Chat Completions stream in a delta after the
text.
- Move `rateLimitHeaders` into `mockRateLimitHeaders.ts`. The Google
route sends the quota headers too.
- Add the Cursor request context query:
- A scripted call of the tool `CURSOR_REQUEST_CONTEXT_TOOL` sends
the query to the CLI.
- `cursorRequestContextExec` encodes the query.
- `cursorRequestContextResponseOf` decodes the answer.
- The rules that the CLI states reach `contextRules` of the Run
request witness.
- Change the Kiro `thinking.type` values from `enabled` and `disabled`
to `adaptive` and `disabled`.
### Tool calls
- Add `cursorRequestContextToolCall` and `JUNIE_ANSWER_TOOL` to
`providerToolCalls.ts`.
- Make the Amp `bash` call state `timeout_ms` with
`AMP_SHELL_WAIT_LIMIT_MS`. A held command then stays in its call.
- Add `printfMarkerCommand` to `shellArguments.ts`. It replaces
`$((...))`, which Gemini CLI refuses. Use it in
`nativeBypassPermissions.ts` and `nativeToolExecution.ts`.
- Correct the comments about Dirac `execute_command` and the Fast
Agent `__human_input` tool.
### Agent environment
- Turn off the update of these CLIs:
- OpenCode and Kilo, with `OPENCODE_DISABLE_AUTOUPDATE` and
`KILO_DISABLE_AUTOUPDATE`.
- Copilot CLI, with `COPILOT_AUTO_UPDATE=false`.
- Dirac, with `DIRAC_NO_AUTO_UPDATE=1`.
- Letta Code, with `DISABLE_AUTOUPDATER=1`.
- Qoder, with `QODERSEC_SKIP_ASYNC_UPDATE=1` and
`QODER_SECURITY_SCAN_SETTINGS_JSON`, which turns off the four
security checks.
- Gemini CLI, with `enableAutoUpdate` and
`enableAutoUpdateNotification` set to false in place of the
deprecated keys.
- Junie, with `JUNIE_SKIP_UPDATE_CHECK=1`, a private data directory
that links the installed versions and holds no staged update, and
a private copy of the launcher script.
- Remove the inline `autoupdate` key of the MiMo Code configuration.
MiMo reads that key only from its global files, and
`MIMOCODE_DISABLE_AUTOUPDATE` carries the switch.
- Correct the comments of the update switches of these CLIs:
- Qwen Code.
- Cline.
- Kimi Code.
- CodeBuddy.
- Codex.
- Write the Reasonix `.env` file with the mock key. Reasonix reads
`api_key_env` only from that file.
- Write the DeepSeek Harness preset row only when the caller selects a
preset. Export `DEEPSEEK_HARNESS_CONTEXT_WINDOW`.
### Native settings and composer helpers
- Add `waitForNativeSettingsHydrated`. It waits for each group with an
option in the live Worker catalog and reads that catalog again in
each attempt. These files call it:
- `nativeBypassPermissions.ts`.
- `nativeSettings.ts`.
- `unsupportedConfiguration.ts`.
- `unsupportedPlanMode.ts`.
- Add `waitForNativeOptionApplied` to `nativeSettings.ts`. It waits
until the Worker reports the applied value on an active agent.
- Read the hub URL from the page in `nativeSettingsAgent`. State the
observed Worker status when the agent is not active.
- Extract `openPermissionShortcut` from `applyPermissionPreset`.
`permissionPresetHydration.spec.ts` drives it on a static page.
- Add `MessageEntry` and `enterMessageText`. `sendMessage` can insert
each line at once instead of typing it.
- Add the options `entry` and `toolCalls` to `sendNativeAnswer`
(`NativeAnswerOptions`). The helper reads the request record after
the turn.
### Native resume
- Add `nativeResume.ts`:
- `nativeResumeTexts` builds four texts with one marker.
- `reopenedNativeAgentVerdict` and `expectReopenedNativeAgent`
require the Worker to confirm the stored provider and session.
- `countOriginalAnswerRows` counts the Worker rows that hold the
original answer before the original agent closes.
- `expectResumedConversation` requires a separate resumed answer,
on the page and in the Worker rows. It requires one original
prompt bubble. It requires the same number of original answer
bubbles and Worker rows as before the close.
- `expectNativeResumeContext` requires these turns in the native
model request, in this order:
- The original prompt in a user turn.
- The original answer in an assistant turn.
- The resumed prompt in a user turn.
- Add `nativeResumePicker.ts` with `resumePickerScenario`, the shared
picker flow of one provider.
- Make `exerciseSessionResume` use these helpers and return
`NativeResumeResult`. The result holds the four texts, the marker,
and the `request` that consumed the resumed prompt.
- Add these readers:
- `nativeMessagesHoldingText`.
- `jsonStringValues`.
- `nativeModelConversationTurns`.
- `nativeToolArgumentText`.
- `selectedAgentTab`.
- Read the result in the callers of `exerciseSessionResume`:
- `command-code/model-context-on-resume.spec.ts` and
`deepseek-harness/model-context-on-resume.spec.ts` read
`resumed.request`. After their own marker checks, each one calls
`expectNativeResumeContext` with
`nativeModelConversationTurns(resumed.request)`.
- `dirac/model-context-on-resume.spec.ts` reads the model context
from `resumed.request`. It requires that the prompt and the answer
from the saved rows equal `RESUMEPROMPT` with `resumed.marker` and
`resumed.originalAnswer`.
### Gemini CLI resume
- Gemini CLI 0.62.0 starts a new chat for a session ID before
`session/load` looks up the session. In the minute of the archive of
the session, that chat appends to the archive and replaces the
stored history. The lookup then fails with -32603 "No previous
sessions found for this project". Upstream fixed this in
google-gemini/gemini-cli#29463. The first release with the fix is
v0.63.0-preview.0.
- Add these items to `gemini-cli/nativeStore.ts`:
- `MinuteClock`, the clock that a minute wait reads and sleeps on.
A unit test supplies its own clock.
- `geminiSessionArchiveMinutes` reads the UTC minute of each root
archive of one session. An archive name has the form
`session-<minute>-<first 8 characters of the session ID>` with the
suffix `.jsonl` or `.json`. A name of another form gives no
minute. A session ID with a short or malformed prefix fails.
- `waitForMinuteAfter` sleeps until the UTC minute after an archive
minute begins. It sleeps again when a sleep ends early. It fails
for a text that is not an archive minute.
- `waitForGeminiArchiveMinuteToPass` reads the `chats` directory of
the Gemini project of the agent. It waits for the minute after the
newest archive of the session. It fails when the session has no
archive.
- Add `gemini-cli/nativeStore.test.ts`:
- `geminiSessionArchiveMinutes` reads the minute of each root
archive of the session and of no other file. It reads no minute
from an empty directory. It refuses a short or malformed session
ID.
- `waitForMinuteAfter` sleeps until the next minute begins. It
sleeps a whole minute at the first instant of the minute. It
sleeps again when a sleep ends early. It does not sleep after the
minute. It refuses a text that is not a UTC minute.
- Change `gemini-cli/resumeEvidence.ts`:
- `exerciseGeminiResumeWithEvidence` returns the
`NativeResumeResult`.
- The evidence capture finds the selected tab with
`selectedAgentTab`.
- The wrapper takes an optional `MinuteClock`. At the `stored`
phase, it calls `waitForGeminiArchiveMinuteToPass` until the UTC
minute of the archive ends. The scenario then reopens the session
in a later minute.
- Change `gemini-cli/resumeEvidence.test.ts`:
- The wrapper returns the unchanged resume result.
- The wrapper waits until the minute of the newest archive of the
session ends. An archive of another session in a later minute
changes nothing.
- The wrapper reopens at once when the clock shows a time after
that minute.
- The wrapper fails, and captures the failure, when the stored
session has no archive.
- Change `gemini-cli/model-context-on-resume.spec.ts` to read
`resumed.request`. It keeps its checks of the protocol and of the
two markers. Then it calls `expectNativeResumeContext` with
`nativeModelConversationTurns(resumed.request)`. Gemini states the
history as `contents`.
### Interrupt, error, and startup helpers
- Export `heldToolScript`. Its path words stay within 255 bytes,
which Dirac requires.
- Add `holdModelTurn` and `heldModelTurnEnd` to
`exerciseInterruptTurn`. They hold a turn after the first part of
its streamed text, and they accept a runtime that ends the turn
after the held answer arrives.
- Add `attempts` and `answerFailure` to `exerciseModelError`. The
helper returns the requests of the failed turn (`failedTurnRequests`).
- Add `passThroughWhen` to the startup wrapper. A launch with one of
these words runs at once.
- Add `projectConfigurationWorker` to `nativeWorkspaceTrustLimit.ts`.
- Add `answerToolNames` to the scenario context. A call of such a tool
counts as no tool activity in `assertNativeSoundActivity`.
- Add `currentIdleReceipt` to `turnEndSound.ts`.
### Permission, control, and MCP helpers
- Add `outputGate.ts` with `createOutputGate` and `runWithGatedOutput`.
An `OutputGate` holds a command until the browser shows its output.
- `exerciseNativePermissionDecision` and
`NativePermissionOperationPlan` take a `GatedOutput` as
`outputGate`. A denial cannot use it.
- `exerciseShellToolExecution` takes the flag `outputGate` and
creates the gate itself.
- Add `expectDeclinedToolRow` and `exerciseNativePermissionRefusal` to
`nativePermission.ts`.
- Add `nativeStoredControlDecision.ts` with `onlyObservedNativeControl`
and `readNativeStoredControlDecision`.
- Add MCP proofs for a client that cannot show a form:
- `nativeMcpCancellation` and `expectCancelledMcpInput`.
- `nativeMcpUnansweredInput` and `expectUnansweredMcpInput`.
### Goal lifecycle
- Add `pauseTiming` to `exerciseNativeGoalPauseAndResume`:
- `at-once` proves that the pause cancels the running round.
- `after-the-round` proves that the round finishes first.
- Add `supportRules` and `roundSystem`. The second option keeps a
housekeeping request, such as a session title, from taking the held
model answer of the goal round.
- Require that the goal command reaches the agent at once and never
waits in the input queue.
- Pass `pauseTiming` in the two callers:
- `qwen-code/session-goal-pause-and-resume.spec.ts` passes
`at-once`, because Qwen Code pauses through its goal control and
cancels the running round. The paused proof requires that the
Worker reports the status PAUSED and an empty `statusDetail`,
because the pause of the reader states no reason.
- `grok-build/session-goal-pause-and-resume.spec.ts` passes
`after-the-round`:
- It passes `roundSystem` with the opening words of the system
prompt of the agent turn of Grok. A session title request quotes
the goal but lacks those words.
- It passes `supportRules` (`grokGoalMachineryRules`) for the
model calls of the goal machinery of Grok:
- The Goal Plan Writer writes `GOAL_PLAN` to the file that its
prompt states.
- The Goal Plan Writer then answers `Done`.
- The hidden completion evaluator answers with text that is no
verdict, so Grok pauses the goal after the resumed round.
- The paused proof requires this answer on the page:
"Goal paused. Use /goal resume to continue."
- The paused proof also requires that the Worker reports the
status PAUSED and an empty `statusDetail`.
### Tool output, child agents, and context usage
- Poll the release files in `toolOutputControl.ts` instead of a file
watch. Add these options and values:
- `holdFirstOutput` and `releaseStartOutput`.
- A live tail for each output segment.
- Add `waitForToolStart` and `OutputBoundary` to
`generationProgress.ts`.
- Make `exerciseSteerAfterTool` wait for the first live tail instead
of the first marker.
- Add `lineMarker` to `computedNativeToolOutput`.
- Make `nativeOutputPathsPrecedePreview` fail when a marked element
wraps the path block or sits inside it. Wait for each marker inside
marked output.
- Add `expandResult` to the tool proof of `liveChildTranscript.ts`.
Script the held child answer after an optional Read. Add
`nativeResultBubble`.
- Add these exports to `subagentRegistry.ts`:
- `HELD_CHILD_TITLE`.
- `HELD_CHILD_NAME`.
- `HELD_CHILD_REPORT`.
- `heldAnswer` of `HeldChildCase`.
- `heldChildAnswer`.
- Add `parseContextRow` and `readContextRow` to `contextUsage.ts`.
- Add `expectRelatedTodoSurvivesReload` to `relatedTodoProof.ts`.
### Specs and unit tests
- Wait for the shell with `waitForTerminalReady` in
`069-terminal-ime.spec.ts`. A command that the test types before
the shell is ready loses its first characters.
- Set a timeout of 60 seconds on the `startSuiteServer` tests. The
call runs the real setup commands of the installed agent CLIs.
- Add unit tests for these new helpers:
- `outputGate.test.ts`.
- `nativeResume.test.ts`.
- `nativeStoredControlDecision.test.ts`.
- `nativeGoalLifecycle.test.ts`.
- `nativeModelError.test.ts`.
- `mockRateLimitHeaders.test.ts`.
- `jsonStringValues.test.ts`.
- Extend the unit tests of the existing helpers that these changes
touch.
Fixes an issue where invoking
session/loadin ACP mode within the same UTC minute assession/newcould overwrite the active session's checkpoint state before resolving the conversation file, causing session lookup to fail withNo previous sessions found for this project.Details
packages/cli/src/acp/acpSessionManager.ts): UpdatedAcpSessionManager.loadSessionto resolve the target session viaSessionSelector.resolveSession()before runningconfig.initialize(), and removed the redundantgeminiClient.initialize()call prior togeminiClient.resumeChat(). Also updatednewSession()to reuse the chat instance initialized duringconfig.initialize()when available.packages/core/src/services/chatRecordingService.ts): UpdatedChatRecordingService.initialize()'s fresh-session creation path to disambiguate the.jsonlfilename (session-<timestamp>-<counter>-<id8>.jsonl) if a conversation file for the same UTC minute and short session ID already exists on disk, ensuring existing conversation files are never appended to by fresh session initialization.packages/core/src/core/client.ts): UpdatedGeminiClient.resumeChat()to clean up any non-resumable startup-only session file created prior to switching toresumedSessionData.filePath.packages/core/src/services/chatRecordingService.test.tsandpackages/cli/src/acp/acpResume.test.ts.Related Issues
Fixes #28693
GH-28693
How to Validate
npm run lint:ci && npm run typecheckPre-Merge Checklist