feat(acp): restore Antigravity sessions after restart - #5670
Conversation
Generated-by: OpenAI Codex
Persist external Session identity and prompt checkpoints in Plugin storage. Resume committed turns through ACP, hold uncertain load replay outside canonical history, and expose explicit restore and history-gap states in Desktop. Refs apache#5103 Generated-by: OpenAI Codex
Acknowledge consumed terminal results and commit ACP checkpoints only after the agent has definitively settled. Preserve pending checkpoints for timeouts, crashes, and detached consumers; cover restart behavior with real stdio regression tests. Generated-by: OpenAI Codex
jackwener
left a comment
There was a problem hiding this comment.
[kabi-grok-reviewer]
I reviewed 7eb6741ee884691b681aa655712b2912309b3dfc.
Design. #5103 is grounded: Maka as ACP client must continue the same external Antigravity Session after the ACP process or Host restarts. Persisting the Agent Session ID plus a versioned prompt checkpoint in Plugin-private storage, then calling negotiated session/resume, is that slice. Tasks without a saved ID stay history_only.
Storage. AcpContinuityRecord.version is the literal 2. decodeContinuity maps version === 1 to 'legacy' and anything else invalid to 'invalid'; both inspect as history_only. That fails closed on unknown future versions and is evolvable (a v3 can be added later). binding is sha256: of adapter + executable/helper bytes and is not sent to the Host.
Epoch. Code is RUNTIME_HOST_COMPATIBILITY_EPOCH = 190 with comment "Executor readiness exposes explicit restore, restore-failed and history-gap states." Live main is 189. The bump is required: ExecutorReadiness adds restorable | restoring | restore_failed | history_gap. The PR description's "183 → 184" is stale text, not the code.
Desktop UI. Catalog and picker copy include all four states. Actions: Restore on restorable/restore_failed; New Task on history_only/history_gap; restoring has no extra button. That matches the states. I did not click them in Electron.
docs/archive. antigravity-acp-pr3-acceptance.md is a dated probe log (including failed 403 gates), same shape as the PR 2 archive file. Evidence belongs in archive, not as the living contract.
P0–P2: none.
Checked this round: issue #5103 title/problem; protocol index.ts 107–108 vs origin/main 189; AcpContinuityRecord 113–127 and decodeContinuity 1027–1066; inspectConversation 236–275; executor-catalog.ts 35–41, 90–99; picker copy 68–95 and actions 313–318; archive doc 1–80; git merge-tree --write-tree origin/main 7eb6741ee exit 0, tree 700767fda.
Not checked: commit-after-terminal crash window, whether replay can leak into canonical transcript, restore retry, tests, Desktop restart, Antigravity process.
简体中文
我审查了 7eb6741ee884691b681aa655712b2912309b3dfc。#5103 站得住。存储 version: 2,未知版本失败为只读历史。epoch 代码是 190(PR 正文 183→184 是过时文字)。四个 Desktop 状态和按钮对得上。archive 文档当验收记录可以。没有 P0–P2。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
jackwener
left a comment
There was a problem hiding this comment.
[kabi-sol] Restore state-machine review of 7eb6741ee884691b681aa655712b2912309b3dfc: one P2; I do not recommend merging this revision.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
The inline finding concerns late external-session output escaping restoration quarantine while the history-gap checkpoint is being persisted. A controlled real-stdio Agent, the production SDK, AcpExecutor, PluginExecutorService, and PluginExecutorBackend reproduce it: the backend emits old text with the new, unsent Turn ID before returning the history-gap error. An isolated build changing only the notification guard to include session.historyGap eliminates that text event under the same schedule.
On this head, fresh dependencies and workspace dependencies/ACP plugin builds completed, and 83 existing targeted tests passed across the ACP executor/process and Runtime plugin backend/service suites. They exercise same-ID restoration after acknowledgement, uncertain restore replay, failed restore retry, corrupt/legacy record rejection, and terminal-consumption acknowledgement for completed/cancelled/failed results. These passing cases do not cover the reproduced late-notification window.
Evidence boundaries: the added stdio probe uses a controlled Agent and a delayed in-memory implementation of the Plugin state-store port. It checks the production backend's SessionEvent stream, not a full Host database/UI write. Existing real-process tests and backend acknowledgement tests were rerun; I did not kill the actual Host between its database commit and Plugin acknowledgement, run the official signed-in Antigravity Agent, a signed-in Desktop, Windows, or the full repository suite. CI test was still in progress at the checked snapshot. No approval, merge, or tracked production edits.
zhiiw
left a comment
There was a problem hiding this comment.
Independent review (blind — no existing comments read). Conclusions bind to 7eb6741ee884691b681aa655712b2912309b3dfc (test was still running at review time — no CI verdict claimed). The PR body's epoch note says "183 to 184"; the diff moves 189 → 190 with the right ledger comment — the body text is stale, the hunk is correct.
Verified locally (real Windows 11, Node 24.18.1 — the version CI pins), against the dispatch's three asks:
- Plugin suites with real stdio + force-killed processes:
acp-executor-plugin+acp-process— 49/52 pass, 2 skipped; all 3 failures are the test setup callingsymlink(2)and getting EPERM (this machine has no developer mode) — a pre-existing environment class that fails identically on the base, not this PR. The real-stdio cases that matter here — killed process after durable ack, fresh process continuing the same Session, uncertain load replay — all pass. - Checkpoint write on Windows: the record writes go through
pluginStateStore→PluginStorageService.set→HostPluginDataRuntime'satomicWrite(plugin-data-runtime.ts:297) — tmp file withwx, then rename. Atomic on Windows (same-dir rename replaces; the destination is never held open). No fsync on the file or the directory, so a power loss between rename and flush can drop the newest checkpoint — the failure direction is safe (a lost checkpoint restores as history-only / shows the explicit gap; nothing ever claims more than what was durably written). Not a finding; noting the bound so it is on record. - Ablation of the "committed only after the canonical terminal event is consumed" gate: the gate has two halves. Plugin-side (
acknowledgeExecution's awaitingAck/phase/pendingTurnId check inacp-executor-plugin/src/index.ts:674) is a second belt — removing it alone changes nothing the tests observe. The load-bearing half is the caller gate inplugin-executor-backend.ts(returnedResultonly exists once the queue's terminal event was accepted). MakingacknowledgeExecutiona no-op (scratch edit, reverted) → 10 tests fail, including the real-stdiorestores the same Session in a new process after durable acknowledgementand the cancelled/end_turn/max_tokens/refusal checkpoint cases. After restore: green again. The commit path is pinned by real tests.
Read and agree with: the phase machine (reserved → established → prompt_pending → committed / history_gap) fails closed at every step — a failed checkpoint write flips to history_gap and loses the session rather than claiming continuity; restore never falls back to session/new; decodeContinuity rejects malformed records field-by-field (so a torn or old record degrades to history-only); the binding digest ties the saved Session to exact executable+helper bytes. The retry rule (only when no session/new was attempted and continuity was never reserved) prevents a lost response from double-creating an Agent Session.
Not verified: the official Antigravity ACP 1.1.1 resume (author's macOS run, cited in docs/archive/antigravity-acp-pr3-acceptance.md — not rechecked); the Desktop UI restart surface (the PR lists it as remaining draft verification); the full serial workspace sweep.
No P0–P3 findings.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
Finding
[P2] I independently corroborate the existing current-head inline finding about late restored-session output escaping the history-gap quarantine. execute() installs the new Turn's active context before initialization (packages/acp-executor-plugin/src/index.ts:357-368). On restoring a saved prompt_pending record, initialization clears session.restoring before its awaited history_gap write (packages/acp-executor-plugin/src/index.ts:583-603). During that await, #acceptUpdate gates only on session.restoring, then emits an Agent text chunk or tool event through the new context (packages/acp-executor-plugin/src/index.ts:818-843). The backend assigns that output the new Turn ID (packages/runtime/src/plugin-executor-backend.ts:392-400), even though the new prompt has not been sent. This can contaminate the canonical transcript before the history-gap error. The existing inline comment includes a real-stdio reproduction, so I am not duplicating it inline. Keep replay notifications quarantined until the gap path fully closes and add a regression for a notification arriving during the checkpoint write.
Scope and readiness
The 18-file PR adds a versioned Plugin-private ACP continuity record, executable binding, same-session resume/load and terminal-consumption acknowledgement, plus explicit Desktop readiness and restore controls. I inspected the Plugin restore/checkpoint state machine, Runtime backend acknowledgement boundary, Desktop restore/configuration path, catalog/epoch contract, and adjacent tests. The configuration patch intentionally invokes the provider even when the selected model is unchanged (packages/runtime-host/src/server/session-catalog-coordinator.ts:823-853), so the Restore action can reach initialization. I found no other substantiated P0–P3 issue in that scope.
Current-head test and label checks pass; the diff check and synthetic merge onto fetched main are clean. The P2 remains a blocker despite those checks. I did not run local tests (Node 18/no dependencies), the signed-in Antigravity Agent, a real Host restart, or Desktop UI interaction. No database schema/migration files changed. This is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Keep history-gap and lost sessions from projecting late Agent output or permissions into a newly requested turn. Cover notifications arriving while the gap checkpoint write is pending. Generated-by: OpenAI Codex
|
Automated follow-up (OpenAI Codex): A correction to the Windows checkpoint note in @zhiiw's review: the absence of |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed current head e0eccd0c7d650b6481c22cfea4653078c7b51244, focusing on the new ACP restore-quarantine increment and its call path. I found no substantiated remaining P0–P3 issue in that inspected path. The previous P2 is addressed on this head: the prompt_pending restore sets historyGap before awaiting its durable checkpoint (packages/acp-executor-plugin/src/index.ts:586-607), and late updates and permission requests now stop on that flag or a lost session (packages/acp-executor-plugin/src/index.ts:804-845). The added regression blocks the checkpoint write and injects late text, tool activity, and permission, asserting no new-Turn events escape (packages/acp-executor-plugin/src/__tests__/acp-executor-plugin.test.ts:182-246). I read the existing current-head follow-up and did not repeat its inline comment.
The full PR spans ACP execution/restoration plus related runtime, Desktop, UI, tests, and docs; this pass concentrated on the new two-file increment and its adjacent lifecycle. No schema/migration files change. Current-head test succeeds and PR diff-check is clean, but the branch conflicts with freshly fetched main in packages/runtime-host/src/protocol/index.ts; GitHub reports it unmergeable. I did not run local tests, a signed-in external Agent, Host restart, or Desktop UI validation. Resolve the conflict and recheck the resulting head before human merge consideration. This is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Resolve the compatibility epoch collision at 193 and re-pin compatible refactor declarations. Preserve restore readiness alongside the updated model label. Generated-by: OpenAI Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head 38b3a37c and its 20-file effective PR diff after the merge from main. I found no new substantiated P0–P3 issue in the inspected restore and integration paths.
The earlier pending-prompt quarantine issue remains fixed: session/load replay is excluded while restoring is true, historyGap is set synchronously before the checkpoint write, and later updates and permission requests are rejected (packages/acp-executor-plugin/src/index.ts:564-607,804-827). The merge leaves the ACP provider, Runtime acknowledgement path, Desktop selection controller, and their focused tests unchanged from the previously reviewed e0eccd0c head. Its substantive conflict resolution advances the Host compatibility epoch to 193 after main's 190–192 entries (packages/runtime-host/src/protocol/index.ts:103-111), and the model-picker merge retains the restore states while adopting the new trigger label component (packages/ui/src/executor-model-picker.tsx:311-355). This does not project the old review onto the new head; these are the current-tree checks.
The current-head test check passed, git diff --check passed, and a merge-tree against freshly fetched main is clean. There is no database-schema migration in this diff. I did not rerun local suites (Node 18/no dependencies), a signed-in Antigravity Agent, packaged Host, or full Desktop restart/keyboard smoke. CI and static inspection do not prove those runtime paths. This is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 38b3a37c166724f7677bd2bed0b2970114b78fd2 (6 commits, +1477/−73 across 20 files; ~700 production lines, the rest tests). This adds restart recovery for Antigravity ACP sessions: a durable continuity record decides whether a session may be resumed, restored with a replay, or must be reported as having a history gap.
No P0–P3 findings. Four areas, in the order the dispatch asked.
Merge state (please read before merging)
This revision is mergeable=false / mergeable_state=dirty. The conflict is the compatibility epoch: here it moves to 193 (packages/runtime-host/src/protocol/index.ts:106), while main is already at 195 — both edit the same constant and its comment block. After rebasing, the bump needs to take the next free slot (currently 196, following main), and the two refreshed ledgers have to move with it: packages/runtime-host/protocol-compatible-changes/executor-catalog-browser-safe.json and issue-4032-composition-identity.json currently change "epoch": 189 → 193 and their reason strings to "through epoch 193"; both must be renumbered to match. Everything below binds this head.
1. Restore vs. replay isolation — isolation is the design, not a side effect
- The durable record has explicit phases (
packages/acp-executor-plugin/src/index.ts,phase: 'reserved' | 'established' | 'prompt_pending' | 'committed' | 'history_gap'). A durably acknowledged session resumes the same external session ID with no replay and nosession/new; an unacknowledgedprompt_pendingrecord goes throughsession.loadreplay, and that replay is quarantined: the record is flipped tohistory_gapwithpendingTurnId: undefined, so the in-flight turn is never re-issued and no second prompt is sent. - Late updates arriving from the restored agent after the gap is persisted stay quarantined rather than entering the transcript (covered by the new case "late restored-session updates stay quarantined while the history gap is persisted").
- A failed restore permits an explicit retry and never creates a replacement session; a missing restore capability, a corrupted record, or a changed ACP installation fail closed (
acp_restore_identity_changed,acp_restore_unsupported) — the installation check is what stops a replay into a different agent. - The pending marker is cleared only on proof of settlement, at both layers. The backend returns true only after it published a terminal result, and then calls the new optional
acknowledgeExecution(sessionId, turnId)(packages/runtime/src/plugin-executor-backend.ts:91-118,:142-215); the plugin re-checks that its record is stillprompt_pendingand that the turn ID matches. A failed or uncertain acknowledgement deliberately leaves the marker, so uncertainty resolves to a visible gap on the next start rather than to a duplicate execution — which is the right direction. The acknowledgement is scope-guarded in the service (conversationKey !== sessionIdrejects, plus abort and retirement checks) atpackages/runtime/src/plugin-executor-service.ts:449-464.
2. historyGap and permission requests
- A history gap is a first-class, visible state: new readiness values
restorable,restoring,restore_failed,history_gap(packages/core/src/executor-catalog.ts:35-42, accepted by the validator at:87-100), reported asacp_history_gapwith the message "External Session was restored, but conversation history may have a gap". - The important authority property is pinned by a test rather than assumed: in the gap scenario a permission request from the restored agent resolves to
{ outcome: { outcome: 'cancelled' } }— restoration does not inherit or silently grant approval. - The user-facing copy matches the mechanism:
history_gapsays the agent may be ahead of saved history and recommends starting a new task "to avoid an incomplete transcript".
3. Host compatibility epoch
The 192 → 193 bump is justified: the new readiness values are part of the executor catalog that crosses the Client–Host boundary, and the shared normalizer rejects unknown readiness values — so an epoch-192 peer would refuse a 193 host's catalog. The new comment line follows the file's existing convention of naming the reason per epoch.
4. Desktop recovery controller
restore() in apps/desktop/src/renderer/features/conversation/controller/use-executor-selection.ts:188-206 requires a session, an executor and a model (the recorded executorConfig.model, or the inspected current model), flips the row to restoring optimistically but scoped to the current key, then reuses select with that model. The optimistic value cannot get stuck: select refreshes on failure and stores an error snapshot before rethrowing, so the real backend readiness (restore_failed, history_gap, …) replaces it. onRestore is threaded through executorComposerProps (executor-composer.ts:46) into the picker, which gains the four readiness strings plus a Restore Session label in every locale (packages/ui/src/executor-model-picker.tsx:66-73, :90-120).
Checks on this revision: the test check is green.
What I could not judge
- No live Antigravity CLI/agent: the "real stdio" cases exercise a real child process with a scripted protocol peer, not a real agent installation.
- No native desktop run of the picker (component tests only), no evaluation of the acceptance documents' claims beyond what the code shows, and I did not attempt the conflicted merge locally.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Third independent review lineage at 38b3a37c (supplements the two earlier reviews on this head).
The core replay isolation is sound:
- updates are dropped while
restoring historyGapis set synchronously before the checkpoint await- permission requests are cancelled
- a pending turn is never re-prompted
committedis only written after the Runtime has consumed the terminal event
The continuity record decoding is strict and fails closed. The epoch bump is required, because older peers reject the new readiness values.
I did find several P2 state-machine gaps that I'd want fixed before merging:
- A missed ack wedges the conversation while it reports
ready(inline,index.ts:386).awaitingAckis cleared only by a successfulacknowledgeExecution. If the kernel throws while persisting the terminal event (disk full, store conflict), or the service wrapper rejects as retired/aborted, the ack never arrives. Every later send then returnsacp_busyfor the rest of the Host lifetime, andinspectConversationstill reportsready, so the UI shows no gap and no guidance. Suggest having the backend signal abandonment in afinally(markhistoryGap+ lose), or reportinghistory_gapfrom inspect whenawaitingAckis set with no active prompt. - Workspace fs reads and writes aren't gated during restore.
fs/read_text_fileandfs/write_text_file(#configureClient, ~L788-798) only call#assertSession.acpSessionIdis set to the stored id before launch, andwriteTextFile: trueis advertised. So duringsession/loadorsession/resume, and in the await window betweenhistoryGap = trueand#lose, the agent can still write the workspace. The pending-branch comment itself worries that the agent may have moved past Maka's last durable event, and permission requests are refused in exactly these states. Suggest rejecting fs requests whenrestoring || historyGap || lost. - Concurrent retry after
restore_failedcan orphan an agent process (inline,index.ts:466). Two callers (e.g. Desktop Restore plus a send, or a WorkHub dispatch) can bothawait existing.loss. Both then install their owncreatedsession, and the second overwrites the first in#sessions. The first process is then unreachable bydispose/disposeConversation, and both processes resume the same ACP session. Suggest re-checkingthis.#sessions.get(key)after the await, or serialising lost→new per conversation. configureConversationon a fresh conversation can permanently mark ithistory_only(inline,index.ts:291). Configure now spawns and initializes. On failure it only calls#lose, and unlikeexecuteit doesn't delete a never-reserved session from the map. An unauthenticated CLI, a spawn failure, or the 30s init timeout before the first message therefore leaves the task reporting "external process was lost" for the rest of the Host lifetime.- Compat epoch (inline). This needs a rebase to the next free epoch (currently 196, since main is at 195). Also, the edits to
executor-catalog-browser-safe.jsonandissue-4032-composition-identity.json(189→193) have no effect on the guard, which only reads newly added declarations, and they rewrite historical records. Suggest reverting those two files and changing onlyRUNTIME_HOST_COMPATIBILITY_EPOCHwith its comment.
P3s:
- A pending record that can't be loaded (missing
loadSession, binding changed) surfaces asrestore_failed("retry") rather thanhistory_gap, even though retrying can never succeed. It also spawns a process just to collect gap evidence. - The launch binding hashes the full executable and helper on every non-probe init, not only on restore, and any Antigravity auto-update turns all existing sessions into
restore_failed. - The Desktop optimistic
restoringstate can show without any action ifselectis already in flight.
Tests: the new tests would fail on old code. Nothing covers restore end to end at the Host/composition level with a real state store, nor non-conforming agents (replay on resume, fs writes during load), nor any of the P2 scenarios above.
Not verified: tests were not run locally; I did not run the real Antigravity binary.
This review was produced with automated assistance (AI review agents) and checked by a maintainer-side reviewer before posting.
|
Correction to my earlier review (pullrequestreview-5331195535). My instruction about the two
Where this conflicts with my earlier review, the revert wins.
|
…estore # Conflicts: # packages/runtime-host/src/protocol/index.ts
Make unconsumed terminal results explicitly create a history gap, serialize restore replacement, keep fresh configuration retryable, and block workspace access while continuity is untrusted. Also guard optimistic Desktop restore state. Generated-by: OpenAI Codex
These declarations landed on main at epoch 189. Keep that historical record intact instead of rewriting it during branch synchronization. Generated-by: OpenAI Codex
|
Review follow-up on current head Fixed in
The protocol conflict was resolved by merging current main, moving the live epoch from 197 to 198, and restoring the two historical compatible-change declarations to epoch 189 ( I am not changing the two remaining binding/restore points:
Validation on the pushed head: full build passed; Runtime, ACP, Runtime Host, and Desktop renderer typechecks passed; Biome passed; 206 targeted tests passed; and the merge-result protocol guard passed for 197 → 198. CI is currently running. |
jackwener
left a comment
There was a problem hiding this comment.
Approving at 9ac0387034ac9c54c95de96216bcd9005be76375. Re-reviewed against the P2s in review 5331202734. All five are addressed, and no P0–P2 remain.
- Missed ack wedge:
PluginExecutorBackendnow records whetheracknowledgeExecutionsucceeded. If the producer returned a result but the ack never landed (kernel threw, consumer detached, or ack rejected), thefinallycalls the newabandonExecution. The provider then moves a matchingprompt_pendingrecord tohistory_gap, clearsawaitingAck, persists, and#loses the session, so later sends surface a gap instead ofacp_busybehind areadyinspect. The service wrapper keeps the same Session-scope and retirement checks as acknowledgement. - Workspace fs during restore:
fs/read_text_fileandfs/write_text_filenow go through#assertWorkspaceAccess, which rejects whilerestoring,historyGaporlost. - Concurrent restore: after
await existing.loss, a caller that finds a replacement session re-enters#sessioninstead of installing a second one. - Fresh configure failure: a session with no ACP session id, no reservation and no record is removed from
#sessions, so it no longer reportsacp_history_onlyfor the Host lifetime. - Epoch ledgers: both historical
protocol-compatible-changesfiles are restored to epoch 189. The live epoch is 198 after main.
The Desktop optimistic-restore P3 is also handled: restore refuses while the same selection is in flight. Regression tests were added in the plugin, backend and service suites.
Remaining P3 (not blocking): if the executor is retired between the missed ack and the abandonment, the wrapper rejects abandonExecution too. Retirement disposes the session anyway, so there is no lasting wedge.
CI test is green on this head. The first attempt failed only in opencli-chrome.test.js:81 (Windows store-page browser choice), which this PR does not touch; the re-run passed.
Not run locally: suites or a real Antigravity binary.
Astro-Han
left a comment
There was a problem hiding this comment.
Re-review at 9ac03870. Verdict: all five prior P2s are fixed; no new P0–P2; 3 new P3s (two of them are tests that do not exercise their fix).
Fixed:
- Missed ack wedging the session as busy while showing
ready. An unconsumed terminal event or a failed ack now callsabandonExecution(plugin-executor-backend.ts:96-128, service:470-480, pluginindex.ts:704-725). This recordshistory_gapand closes the process. Tests fail if this is reverted. - Fresh configure failure leaving
history_only(index.ts:294-296). - Workspace fs gated during restore (
index.ts:816,822,875-883). With absolute paths, reverting the gate lets the write land; with the gate it is rejected. - Concurrent restore retries leaking a process (
index.ts:467-469). The resume count goes to 2 if reverted. - Epoch: the historical declaration files are back to 189, and the live epoch is 197→198.
Verified in a scratch copy: ACP plugin 61/61 and runtime backend/service 27/27 pass, with a per-fix revert check. The Desktop tests were not run.
New P3s:
- The workspace-access test does not test the gate (inline). It uses relative paths, which the fs layer rejects as "must be absolute" regardless of the gate, so reverting the gate still passes 38/38. Use absolute paths and assert the rejection code.
- The "fresh configure initialization failure remains retryable" test (
:409-444) fails the storage write after the record is already set, so it never reaches the new cleanup branch; reverting that branch still passes. Failing earlier (e.g. atstate.read) distinguishes old from new code. - Epoch 198 is also claimed by other open PRs (#4751, #5709, #5753), and #5394 uses 200. Whichever merges later must renumber. The epoch check blocks a bad merge, so this is merge-order coordination only.
Still open from the last round (P3):
- An unloadable pending record shows "restore failed, retry", although retry always ends in a history gap.
- Executable and helper hashing runs on every initialization.
- The Host still briefly reports
readybefore restore starts (index.ts:255-256vs:530).
Note, not counted as a finding: abandonment can also fire when the terminal event was persisted but downstream delivery failed. That produces a false history gap, which is the safe direction and was a permanent busy state before.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
| }); | ||
| assert.deepEqual(await protocol.requestPermission(), { outcome: { outcome: 'cancelled' } }); | ||
| const workspaceAccess = await Promise.allSettled([ | ||
| protocol.readTextFile('package.json'), |
There was a problem hiding this comment.
P3. 'package.json' is a relative path. The fs layer rejects it as "must be absolute" whether or not the restore gate exists, so removing the gate at index.ts:875-883 still passes this test. Use an absolute workspace path, and assert the specific gate rejection.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 9ac0387034ac9c54c95de96216bcd9005be76375. I found no substantiated P0–P3 in the changed paths; this is a COMMENTED review, not a merge approval.
The follow-up closes the previously reported restore gaps: PluginExecutorBackend abandons a settled turn when its terminal event is not consumed or its acknowledgement fails (packages/runtime/src/plugin-executor-backend.ts:96-127); the ACP provider moves a matching pending turn to history_gap, clears the in-memory ack gate, and loses the external session (packages/acp-executor-plugin/src/index.ts:703-725). Workspace read/write callbacks now reject access during restore, gap, or loss (index.ts:813-824,875-883). Concurrent restore retries re-check the session map after awaiting loss (index.ts:449-479), and a fresh pre-reservation configure failure removes its retryable session (index.ts:278-301). The Desktop restore action now rejects while selection is already in flight. The two historical compatibility ledgers remain at epoch 189; the live protocol epoch is 198, immediately after current main's 197 (packages/runtime-host/src/protocol/index.ts:103-110).
Validation: Node 24 build:test passed, and seven focused ACP/runtime/core/UI/Desktop test files passed (118 tests). The current-head hosted test check is green. Fresh main 5735554b6fa99ab2029054c142f746c0a8fa0e71 merges without conflict and git diff --check is clean. I did not run the full local suite, a real Antigravity binary, or packaged Desktop/Host crash recovery. A storage outage while writing abandonment can still leave durable prompt_pending, which intentionally fails closed as a gap on restart; the real crash boundary remains untested.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Use absolute workspace paths and assert the history-gap error for blocked filesystem callbacks. Fail state reading before continuity reservation so the fresh configure retry test covers session cleanup. Generated-by: OpenAI Codex
Resolve the epoch collision in protocol/index.ts: main's epoch 198 (Executor readiness restore states, apache#5670) is kept as-is, and this branch's callKinds usage-query allocation is bumped to epoch 199.
Summary
Maka could read an Antigravity task after its ACP process or Host restarted, but could not continue the same external Session. This PR persists the external Session ID and a versioned prompt checkpoint in Plugin-private storage, then restores a committed task through the Agent's negotiated
session/resumemethod. The original Session and saved model are retained; no earlier prompt is resent and restoration never falls back tosession/new.A completed turn becomes committed only after Maka has consumed its canonical terminal event. If a prompt may have progressed beyond saved history,
session/loadreplay is kept outside the canonical conversation and the task shows an explicit history gap. Desktop now shows restorable, restoring, restore-failed, and history-gap states with Restore, retry, or New Task actions as appropriate. Tasks created before this change remain history-only because they have no saved external Session ID.Refs #5103
Verification
npm run build,npm run typecheck,npm run lint, andnpm run format:checkpassed.npm run check:renderer-architecture,npm run check:locale-hygiene, and the protocol compatibility epoch guard passed. The final epoch advances from 197 to 198.node scripts/run-workspace-tests-parallel.mjs --concurrency=1onbd8661f3a. The branch then rebased tob62ca805e; the final build, typecheck, lint, format, epoch guard, and 104 affected tests passed after that rebase.restorable→readyand recalled prior context in a fresh process. Sanitized evidence and the replay decision are indocs/archive/antigravity-acp-pr3-acceptance.md.An earlier serial test run hit one intermittent Runtime Host Goal handoff assertion unrelated to ACP. The complete Host workspace passed independently, that test passed in isolation, and the final serial run passed.
Review focus
The official Agent replay did not expose stable canonical event identities for every observed turn. This PR deliberately reports a history gap for an uncertain prompt instead of guessing which replay notifications to append. The Plugin-private record binds the saved Session to the exact executable and helper bytes, workspace, and adapter identity. Process/Host restart is covered after a successful Plugin storage write; power-loss durability without fsync is not established.
Post-merge verification
aff4bb076) passed thetestcheck.AI use
Tool(s) and scope: OpenAI Codex implemented the ACP restoration path, tests, Desktop state, and acceptance documentation. Affected commits include
Generated-by: OpenAI Codextrailers.Checklist
Does this PR entail a change in behavior?