Skip to content

feat(desktop): complete Antigravity ACP plugin execution - #5224

Merged
Astro-Han merged 35 commits into
apache:mainfrom
Sun-GLiang:feat/antigravity-acp-pr2
Sep 23, 2026
Merged

Astro-Han merged 35 commits into
apache:mainfrom
Sun-GLiang:feat/antigravity-acp-pr2

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the Antigravity ACP PR 2 execution slice from #5103: authenticated catalog discovery, executor/model selection, retained sessions, hosted permissions and questions, cancellation, and readable history after process loss.

  • One searchable executor/model panel supports browsing without changing configuration. Maka retains main's provider icons, single-line names, connection groups and exact model identity. Selecting a native model clears the external executor. Existing sessions retain their executor identity.
  • Antigravity's verified High/Medium/Low variants are grouped into base models. Choosing a base model automatically selects its highest supported intensity. The composer then shows only that external model and the shared thinking control; there is no extra Maka model or pending/cancel step.
  • All thinking selection stays in the composer footer. Native models use native thinking parameters; Antigravity selects real catalog variant IDs. For example, Gemini 3.1 Pro High is gemini-pro-agent, not an inferred suffix. The Antigravity adapter owns label recognition and exposes structured capabilities to generic UI.
  • Only actual supported levels are offered. No synthetic Agent default or off option; no Antigravity plug icon; Gemini icons remain. Unknown, singleton or ambiguous families retain their original rows and have no invented strength control.
  • Existing-session changes update the UI after confirmation. Failures restore and confirm the previous real ID before permitting retry. Busy/read-only/session locks remain enforced, and catalog refreshes and session switches use the confirmed model.
  • ACP execution preserves tool progress, authoritative diffs, permission/question identities and cancellation stop reasons. Review fixes bound/sanitize tool metadata, retain lifecycle events on projection failure, permit retry before a session exists, and keep filesystem/process ownership checks.
  • Built-in setup placeholders are appended at the Host catalog boundary only when no registered Plugin supplies that executor ID. The shared picker displays a generic loading status while discovery runs; Desktop no longer inserts an Antigravity-specific loading row. A live Plugin catalog takes precedence without another UI special case.

Refs #5103.

CI fixes

The earlier protocol declaration mismatch and Astryx raw-control inventory failures are fixed. The four failing checks on def615305 shared a later cause: GitHub's synthetic merge with main removed modelChoiceDescription, leaving references in the PR's model panel. The final integration removes that obsolete dependency while retaining searchable catalog metadata and main's native formatting.

Current main b004473ed is incorporated, including Host-owned WorkHub result turns and the running-state UI update. The compatibility epoch is 182, and the merge-result protocol guard passes against main at epoch 178. The Astryx surface inventory was regenerated for the combined file set. Earlier Storybook smoke passed 392 stories / 424 theme renders after its loading-state timing fix. Fresh GitHub checks for the new merged head are pending.

Verification

  • After the b004473ed merge: full workspace typecheck and format passed; Astryx inventory coverage passed for 295 files, its 19 generator tests passed, and 18 affected UI tests passed. GitHub reports the PR as mergeable; fresh CI is running.

  • After the bc0786ee6 merge: full workspace build:test and typecheck passed; 162 focused ACP process, Runtime executor, Host composition/protocol, and UI picker tests passed. The protocol epoch guard passed against current main. Earlier Storybook, lint/format, Host catalog, and renderer architecture checks belong to the previous integration checkpoint.

  • After integration with main 0052f1cfd: all 13 workspace suites passed after rechecks, 12,995 Node tests passed / 38 skipped. The initial run had two intermittent failures; focused checks and complete Host/Storage suite reruns passed without code changes.

  • Clean build, production renderer, all workspace type checks, lint/format, ASF/locale, architecture, Desktop/UI Knip, Astryx/Windows inventories, shell hooks and protocol guards passed. Release checks passed 203 tests serially; an earlier concurrent run hit test-worker deserialization and shutdown-timing failures.

  • Independent linkage checkpoint: 9,916 tests passed, 26 skipped, across Core, Runtime, Runtime Host, Desktop, UI, ACP executor and Antigravity adapter. The UI suite contains 633 tests.

  • Regression cases cover full/partial/unsorted levels, opaque IDs, unknown/ambiguous models, automatic highest selection, failed confirmation and rollback/retry, same-model confirmation, scope races, catalog notifications, execution locks, native reselection and executor switching.

  • Official ACP 1.1.1 returned 11 real variants forming four base models. Actual configuration responses confirmed Flash Medium, Pro High (gemini-pro-agent) and Pro Low. IDs are retained verbatim.

  • Real Electron verification: native rows/search, automatic High when choosing an external model, footer-only thinking, Flash Low/Medium/High, Pro Low/High, and consistent selection on reopening. A real Pro High task returned THINKING_UI_OK; an idle footer switch was acknowledged as gemini-3.1-pro-low, then displayed Low. Running tasks lock model changes.

  • Full-window Electron screenshots below show the official ACP 1.1.1 catalog in Maka. The isolated native connection is a fixture. The generic loading state is covered by Storybook smoke and picker tests; it is not pictured below.

Detailed acceptance report

Full-window UI evidence

Full Maka application-window captures from the real Electron Desktop on 2026-09-22, using the official Antigravity ACP 1.1.1 catalog and an isolated native-connection fixture. Each image includes the whole 1162 × 768 Maka window rather than a cropped picker. Images are GitHub attachments; no screenshot binaries are committed. The generic discovery loading state is verified by Storybook smoke but not shown in these captures.

Catalog ready: Antigravity base models

Full Maka window showing Antigravity base models in the model picker

Composer: Flash thinking control

Full Maka window showing the Flash thinking control in the composer

The acceptance report also shows Pro thinking and Maka's native model list.

Scope and limits

Verified against official ACP 1.1.1 on macOS arm64. New-task choices are validated against the ready catalog; the real ACP session confirms the ID on its first prompt. Unrecognized future label shapes remain raw options until verified. If rollback cannot be confirmed or an established process is lost, the session stays history-readable and requires a new task. PR 3 owns cross-process restoration; PR 4 owns modes and expanded catalog lifecycle.

After its first prompt, each conversation retains one ACP process even while idle, as required by #5103's PR 2 continuity rule. Closing the Desktop UI does not retire the task. PR 2 has no idle timeout or process-count cap, so a long-running Host can accumulate processes. Task retirement and Plugin/Host teardown clean up owned process trees. This keeps the same Agent Session available for follow-up turns while PR 2 cannot restore it after process loss. The bounded-retention decision is tracked separately in #5620; PR 3 does not itself commit to implementing that policy. The first successful model catalog is cached for the executor Entry lifetime; server-side changes without an Entry replacement require a Plugin reload until PR 4 adds invalidation and refresh reconciliation.

session/new currently passes mcpServers: [], so Maka MCP servers are not forwarded to Antigravity. The PR 2 execution run reused an existing Google login; fresh interactive login was verified in PR 1, and recovery from expired authentication during execution has not been established here. Public CI and independent human review remain merge gates.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex contributed implementation, tests, review fixes, verification, documentation, screenshots, and PR preparation. The new implementation and merge commits, plus the two previously missing AI-authored commits, carry a Generated-by: OpenAI Codex trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/XXL Over 2500 readable lines label Sep 12, 2026
@Sun-GLiang
Sun-GLiang force-pushed the feat/antigravity-acp-pr2 branch from 6a6b3be to 84bbed6 Compare September 12, 2026 07:29
@Sun-GLiang
Sun-GLiang marked this pull request as ready for review September 14, 2026 03:22
@Sun-GLiang
Sun-GLiang force-pushed the feat/antigravity-acp-pr2 branch from 9767546 to b278ff0 Compare September 14, 2026 14:09
@Sun-GLiang Sun-GLiang changed the title feat(runtime-host): add ACP connection ownership and sign-in state feat(runtime): rebuild ACP execution as plugin adapters Sep 14, 2026
@github-actions github-actions Bot added effort/XL Under 2500 readable lines and removed effort/XXL Over 2500 readable lines labels Sep 15, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review — comment only

Targeted review of the ACP rebuild. I did not read all ~2.5k lines; I focused on packages/acp-executor-plugin/src/index.ts, packages/antigravity-acp-plugin/src/index.ts, packages/runtime-host/src/server/builtin-external-agent-plugins.ts, the generic boundary changes in packages/runtime/src/plugin-executor-{service,backend}.ts, the host wiring, and the tests.

The boundary design reads well and I think it's the right shape: ACP mechanics stay in one bundle, adapters own only launch policy, and routing/lifecycle stay with #5283's generic executor path. The items below are about robustness and one class of silent failure, not about the architecture.

Issues

1. AcpExecutor marks a conversation permanently "history-only" for every execution failure, not just process losspackages/acp-executor-plugin/src/index.ts:212 and :215

execute()'s catch calls await this.#lose(session) unconditionally, and #lose (:508) sets session.lost = true, which makes #session() throw acp_history_only for that conversationKey for the rest of the Entry generation. That's correct for genuine process loss (owner.failed, cancel timeout at :499), but it also fires for:

  • the 30s INITIALIZE_TIMEOUT_MS (:59, :283) — a slow first spawn bricks the conversation;
  • acp_config_unavailable / acp_config_invalid from #applyInitialConfig — a wrong model value in the Entry config permanently disables the conversation even after the config is corrected;
  • a transient checkedExecutable ENOENT while the agent app is being reinstalled;
  • a caller abort during #ensureInitialized#initialize uses AbortSignal.any([signal, timeout]) (:283), so startupSignal.throwIfAborted() on a user stop during the first prompt also lands here.

The follow-up error is also misleading: it reports "history-only because its external process was lost" when no process was ever started. Suggest only calling #lose() when the connection actually failed, and for other errors clearing session.initialization/session.owner so the next prompt can retry.

2. Agent-authored tool metadata and >64 diffs throw inside context.emit, and the ACP SDK swallows the errorpackages/acp-executor-plugin/src/index.ts:421-470, packages/runtime/src/plugin-executor-service.ts:507

PluginExecutorService's emit wrapper calls normalizeOutputEvent outside its try/catch (plugin-executor-service.ts:296-306), so a validation failure throws back into #acceptTool, which runs inside the ACP session/update notification handler. The SDK's dispatch catches notification-handler errors and only console.error("Error handling notification", …) — the connection stays up, so the failure is silent in the product. Two realistic triggers:

  • #acceptTool passes displayName: snapshot.title and name: snapshot.name ?? … straight from the agent. normalizeOutputEvent bounds displayName at 8192 chars and name at 256 chars with no \r\n (plugin-executor-service.ts:423-426). A long agent-authored title makes tool_start throw, and because toolUseIds is only populated on a successful tool_start (plugin-executor-backend.ts #publishOutputEvent), the later tool_result is dropped too and #closeOptionalOutput never backstops it — the tool vanishes from the transcript entirely.
  • projectToolResult (:781) bounds the combined diff at MAX_TOOL_RESULT_DIFF but not the path count, while isPluginToolResultContent rejects paths.length > 64 (plugin-executor-service.ts:507). One multi-file tool call (a rename across >64 files) makes the tool_result emit throw; snapshot.terminal = true is set before the emit (:457), so no later tool_call_update retries it. The backend then backstops with the synthetic "External executor ended before reporting a tool result" for a tool that actually succeeded.

Suggest clamping displayName/name/paths before crossing the boundary (or degrading >64 paths to the text summary), setting terminal only after a successful emit, and wrapping #acceptUpdate in a try/catch that logs.

3. Retained ACP processes are unboundedpackages/acp-executor-plugin/src/index.ts:149, :255, :226

#sessions only ever grows (set in #session, cleared only in dispose), and every entry keeps a live child process plus its tree. One retained process per Maka conversation key, for the lifetime of the Entry generation, with no idle eviction, cap, or LRU. A long-running Host will accumulate one Antigravity process per conversation ever started. Either bound it (evict idle sessions — the continuity marker already encodes the "history-only" consequence) or state the limit explicitly in the README so operators know.

4. No authentication handling on the execution pathpackages/acp-executor-plugin/src/index.ts:579

child.stderr is drained to nowhere and initializeResponse.authMethods is ignored (the SDK exposes it at dist/schema/zod.gen.js:1134, along with an authenticate method). The setup path merged in #5164 does parse the auth line out of stderr (packages/runtime-host/src/server/acp/antigravity.ts:144-160). If the saved Antigravity login expires, the first prompt fails as acp_execution_failed / "ACP execution failed" with no re-auth affordance and nothing logged. At minimum, surface authMethods as a distinct code so the Desktop can route back into the existing setup flow.

5. A plugin-projection failure requests a Host drainpackages/runtime-host/src/server/execution-composition.ts:1962-1967

applyRuntimePolicyMutationEffects now runs builtinExternalAgentPlugins.reconcile() inside the existing context.requestDrain(); throw error; path. The setting is already committed by then, and HostPluginPlatform already records the failure and schedules its own reconcile (#recordPackageFailure / #scheduleReconcile), so draining the whole Host because a derived, replaceable package layer failed to install seems heavier than needed. Worth confirming this is intended.

Nits

  • Dropped SessionUpdate kinds#acceptUpdate (:405) handles 4 of the 14 kinds in the SDK union. config_option_update is dropped, so the in-memory session.configOptions goes stale once the agent changes a value mid-session (relevant to Set B); plan/plan_update are dropped as well. A debug log for dropped/unknown kinds would make agent behavior diagnosable.
  • Diagnostics are discardederrorCode (:879) collapses every non-AcpRuntimeError to acp_execution_failed and safeErrorMessage (:886) to 'ACP execution failed', with no cause and no log line. This makes issues 1 and 4 very hard to diagnose in the field.
  • String-sniffed error channelactive.text.trimStart().startsWith('Agent execution error:') (:200) treats model-authored transcript text as a failure signal. A response that legitimately begins with that phrase fails the turn. Prefer an explicit stop reason / _meta signal if Antigravity exposes one.
  • supportsAttachments is static — the initialize response already reports agentCapabilities.promptCapabilities (image/audio/embeddedContext). Deriving support from the negotiated capabilities would avoid failing a whole turn with acp_attachments_unsupported for agents that do accept images.
  • Prompt flatteningpromptText folds instructions, quotes, and directory references into prose prefixes inside one text block. ACP ContentBlock supports resource_link/resource/image, which would carry that structure instead of text the agent may act on. Relatedly, session/new sends mcpServers: [], so ACP agents get none of Maka's MCP servers — worth stating that explicitly.
  • dispose() throwing breaks teardownterminate() throws 'ACP process cleanup failed' if the child is still alive at the deadline, and dispose() aggregates that, so a stuck child makes the fiber's effect cleanup fail during plugin uninstall/reload. Consider logging and continuing. (terminate also has no final close/exit await, so a child exiting just past the last poll is a false positive.)
  • createWholeFileDiff (:812) emits a single whole-file hunk with no \ No newline at end of file marker, so a diff whose oldText/newText lacks a trailing newline is technically malformed for strict patch consumers.
  • Bundled host runtime codepackage.json lists @maka/runtime as a devDependency, but index.ts value-imports terminateChildProcessTree from @maka/runtime/process-tree-terminator, so that implementation is esbuild-bundled into dist/plugin.mjs. That's consistent with the deliberate "no cross-bundle instanceof" isolation, but it means the shipped bundle carries its own copy that won't track @maka/runtime. Worth a line in the README.
  • Docs self-referencedocs/antigravity-acp-plugin-rebuild.md opens with "Why PR #5224 cannot be carried forward unchanged" / "PR #5224 predates #5283", but this is PR #5224, so the doc reads as arguing against itself. Naming it "the pre-#5283 revision of this PR" would fix it.
  • Non-hosted permission denialPluginExecutorBackend.#requestPermission (plugin-executor-backend.ts:186) returns { outcome: 'cancelled' } whenever input.hostedInteraction is absent, so every ACP permission request is denied for CLI/API/scheduled execution. Safe default, but worth documenting since it silently narrows what external agents can do headlessly.

Verified / no action needed

  • Recovery ordering is correct. builtin-external-agent-plugins is registered after plugin-platform in the module array (execution-composition.ts:2504, platform module ~:2494), and recoverRuntimeHostDomainModules iterates in array order, so recover()'s packageProjections()/installPackage() hit a platform where #assertReadable() passes. Good — I checked this specifically because the coordinator depends on platform recovery.
  • Digest idempotency holds. extensionPackageDirectoryContentDigest uses the same sorted-path + content hash as PluginPackageStore.decodePackage, and prepareInstall copies into its own transaction directory before the staging dir is disposed, so the "no generation churn on restart" claim is sound (and builtin-external-agent-plugins.test.ts asserts a stable authorityEpoch).
  • Packaging. dependencies is in WORKSPACE_RELEASE_MANIFEST_FIELDS, so @maka/acp-executor-plugin / @maka/antigravity-acp-plugin reach the packaged app through runtime-host's production closure (files: ["dist"] includes plugin.mjs).
  • Adapter isolation. isolate: { acp: true } gives the acp-runtime Entry its own acp label that children inherit; ctx.provide('acp', …) is on the runtime Entry's Context and the adapter passes its own Context explicitly, so executors.register scopes to the consumer Entry (PluginExecutorService.register reads this.ctx, which Service._bind rebinds per consumer).
  • Cancellation and teardown. #awaitPrompt sends session/cancel, waits for settlement, and force-terminates only on the 15s timeout; PluginExecutorService's retirement path aborts active executions and awaits settlement, and the effect cleanup disposes the provider (process trees included).
  • Antigravity launch policy matches the live setup path in packages/runtime-host/src/server/acp/antigravity.ts (BROWSER=/usr/bin/true, PYTHONUNBUFFERED=1, ANTIGRAVITY_HARNESS_PATH, cwd = dirname(executable), localharness_external helper precheck).
  • Generic boundary changes are additive and validated symmetricallyPluginExecutorToolResultContent is a bounded discriminated union, file_diff already exists in the canonical ToolResultEvent shape, and normalizePermissionRequest/normalizePermissionResult validate both directions with the form withdrawn on abort.
  • Concurrency. One prompt per conversation is enforced by session.active (acp_busy), and session.initialization de-dupes concurrent initialization.

Test coverage vs. the PR checklist

The checklist says tests cover "lifecycle, continuity, cancellation, permission bridging, diff projection, and adapter registration". acp-executor-plugin.test.ts has 3 tests (retention, history-only, cancellation); permission bridging and file_diff are asserted only incidentally inside the first. The "Behavior and safety" claims with no test: workspace containment including symlink escape for fs/readTextFile / fs/writeTextFile, the 8 MiB file cap, the diff-size degrade path, setConfigOption validation, adapter/config validation, process-tree termination on dispose, and the durable pluginStateStore (only a fake store is exercised). Containment and the diff-degrade path are the two I'd add first, since they're the security/robustness claims.

Coordination

#5283 is the merged base and #5222/#5385/#5386 are CLI-side ACP work that doesn't touch these packages, so I don't see a conflict. The one seam worth aligning is mcpServers: [] above: #5386 adds session-scoped ACP MCP on the CLI side, and the runtime plugin currently opts out entirely. Similarly, this PR's "Set C" (agent questions, unsupported-input presentation) overlaps #5385's interaction mapping — worth a quick sync so the two don't land incompatible contracts.

@Sun-GLiang
Sun-GLiang marked this pull request as draft September 17, 2026 12:04
Sun-GLiang added a commit to Sun-GLiang/maka-agent that referenced this pull request Sep 22, 2026
The two existing PR commits were already incorporated on the newer main baseline. Preserve their ancestry while retaining the fully tested PR2 tree.
@Sun-GLiang
Sun-GLiang force-pushed the feat/antigravity-acp-pr2 branch from e53fc4e to 421ca51 Compare September 22, 2026 00:34
@github-actions github-actions Bot added effort/XXL Over 2500 readable lines and removed effort/XL Under 2500 readable lines labels Sep 22, 2026
@Sun-GLiang
Sun-GLiang force-pushed the feat/antigravity-acp-pr2 branch from 8691bc9 to 7d7f4f5 Compare September 22, 2026 07:21
@Sun-GLiang Sun-GLiang changed the title feat(runtime): rebuild ACP execution as plugin adapters feat(desktop): complete Antigravity ACP plugin execution Sep 22, 2026
@Sun-GLiang
Sun-GLiang force-pushed the feat/antigravity-acp-pr2 branch from 7d7f4f5 to f555e4b Compare September 22, 2026 07:40
@Sun-GLiang
Sun-GLiang marked this pull request as ready for review September 22, 2026 07:41
@Sun-GLiang
Sun-GLiang force-pushed the feat/antigravity-acp-pr2 branch from 717c84b to 82b5696 Compare September 22, 2026 08:50
Connect provider catalogs and exact model configuration to Desktop, preserve question option identities and drafts, and project process loss as history-only. Add controlled-process coverage and official Antigravity Desktop acceptance evidence.

Generated-by: OpenAI Codex
Separate executor selection, share native model rows, keep model panels the same height, and show full external model names without plugin icons.

Generated-by: OpenAI Codex
Include both ACP packages in the incremental build graph and build the exact Host plugin entrypoints before launch. Exercise replacement of stale bundles and clean adapter output.

Generated-by: OpenAI Codex
Preserve executor-specific models, reasoning effort and client model slots from main. Keep confirmed ACP configuration distinct, reject conflicting model targets, and align relocated selection state and protocol compatibility.

Generated-by: OpenAI Codex
Canonicalize catalog-managed executor model inputs across main and PR2 entrypoints. Retain the provider stop reason through cancellation and the durable runtime ledger, with regression coverage for settlement races.

Generated-by: OpenAI Codex
Render Maka models directly in the shared searchable panel and keep native thinking beside the composer model trigger. Preserve selection and retry behavior, cover thinking placement, and refresh attachment-only UI evidence.

Generated-by: OpenAI Codex
Match main native model formatting, reuse Gemini provider icons, and remove the synthetic Agent default option.

Generated-by: OpenAI Codex
Keep the panel helper import separate from the wheel helper removed on main, preventing a clean textual merge from leaving an unresolved reference.

Generated-by: OpenAI Codex
Expose verified Antigravity model families and opaque variant IDs as structured catalog capabilities. Select the highest supported intensity when choosing a base model, keep all intensity selection in the composer, and display only confirmed external configuration. Preserve native model presentation and protect rollback, retry, scope changes and execution locks with regression tests.

Generated-by: OpenAI Codex
Preserve main's external-session workspace import contract and native model formatting. Remove the obsolete model description helper dependency exposed by PR merge CI, and advance the combined protocol and compatible declaration to epoch 181. Record clean builds, all workspace suite results and serial release validation.

Generated-by: OpenAI Codex
Reconcile the renderer architecture and Astryx inventories with current main, and keep the Antigravity entry visible while ACP discovers live models.

Generated-by: OpenAI Codex
@Sun-GLiang
Sun-GLiang force-pushed the feat/antigravity-acp-pr2 branch from a6c35a9 to 48eb1c2 Compare September 23, 2026 02:03
Record process continuity as soon as ACP creates a session, before model configuration can fail. Emit correct whole-file hunk counts and newline markers, with regression coverage.

Generated-by: OpenAI Codex
Reconcile confirmed executor models after failed Host commits, retain
terminal tool text beside diffs, and reserve continuity before ACP
session creation.

Generated-by: OpenAI Codex
Keep built-in setup placeholders in the Host catalog and let registered Plugin entries replace them. Show a generic loading state in the shared picker, with regression coverage and current UI stories.

Generated-by: OpenAI Codex
@Sun-GLiang
Sun-GLiang force-pushed the feat/antigravity-acp-pr2 branch from a5fe2ef to bb05a3f Compare September 23, 2026 05:37
Resolve the renderer architecture ledger against the merged source and retain the generic executor catalog loading boundary.

Generated-by: OpenAI Codex
@likun666661

Copy link
Copy Markdown
Member

I reviewed this from the problem/mechanism/scope perspective at head 7fc76eb (not as a full line-by-line approval).

The problem and execution boundary are well chosen: Maka needs to act as an ACP client for an agent that owns its own session and tools, rather than presenting Antigravity as an ordinary LLM connection or duplicating the conversation UI. Reusing the generic Plugin executor path from #5283, keeping ACP transport/lifecycle in the shared runtime, and limiting the Antigravity adapter to launch policy and verified catalog interpretation is a coherent solution. The real-agent acceptance evidence for model confirmation, tool/diff projection, interactions, follow-up and cancellation supports that mechanism. I would retain this architecture rather than replace it with an Antigravity-specific Host path.

From an Occam’s razor perspective, the remaining question is whether every retained state and shared contract is necessary for this vertical slice. My main actionable concern is process ownership: AcpExecutor.#sessions retains a live ACP child per conversation and has no idle bound. A long-running Host can accumulate processes as users open tasks. Could this PR define and test a bounded retirement policy (or explicitly document the operational limit and why it is acceptable for PR 2)? Idle eviction would make the evicted task history-only until PR 3, so that UX tradeoff should be deliberate. This was raised in the earlier review and still appears present at this head.

I would also keep the acceptance language precise: PR 2 provides readable history, not cross-process continuation; session/new currently passes mcpServers: []; and fresh login/expired-auth recovery on the execution path was not established by the cited PR 2 run. These may be valid scope limits rather than reasons to grow this PR, but they should be explicit in user-facing claims and follow-up ownership. For the broad Core/Runtime/Host/Desktop/UI contract changes, please ensure each added capability has a concrete consumer in this slice and leave speculative modes/catalog lifecycle work to PR 4.

I am not repeating the earlier projection/metadata/diff findings as current blockers: the head now bounds tool metadata, degrades oversized diff projection and sets terminal state after emission. My conclusion is that the boundary design is convergent, while process retention and scope claims need one more pass before calling the implementation fully converged.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On the merge-vs-split question: merge this as one PR. I traced the dependency edges across all 126 files — the chain is strictly linear (core/runtime contracts → host + protocol + storage + IPC → plugin packages + UI), and every candidate cut produces a worse intermediate state:

  • Plugins first doesn't compile (@maka/core/executor-catalog and the new PluginSessionExecutor surface don't exist on main).
  • Infra first either crashes Host boot — execution-composition.ts:421 unconditionally constructs the builtin coordinator whose constructor import.meta.resolves the two package specifiers — or ships a plugin.executor.query + 3 IPC channels with no producer or consumer.
  • UI first renders a picker whose every request fails against a nonexistent op.
  • Protocol/storage first is dead schema.

This is already the minimal end-to-end slice: the executor seam landed earlier (#5283, #5164), and this PR is its first real consumer — which is exactly what #5103 requires ("every production contract must ship with its real producer and consumer in the same PR"). The one technically-landable two-PR cut (infra, then packages+UI) would require making the coordinator's package resolution lazy first and ships a placeholder-only catalog in between — dead surface bought only for review size. Recommend against it.

Correctness: verified clean on the risky paths. I followed selection → IPC → coordinator → executor service → ACP process → events back, through failure/retry/cancellation/recovery. The invariants all close: external changes commit before durable state, uncertain outcomes → history_only, process loss → readable history with explicit "start a new task" gating, permission/question identity binding through admitFormRequest, bounded cancellation drain preserving real stop reasons, filesystem callbacks with realpath/O_NOFOLLOW/dev-ino containment, epoch 181 covering the new wire surface. The prior-round fixes (bounded tool metadata, retained lifecycle events on projection failure, retry before session exists, fs/process ownership checks) all verified.

Three P3 findings inline — none blocking. Also worth a cleanup pass at some point (not merge gates):

  • PluginExecutorService.execute()/list() are test-only public methods that bypass bind()'s scope check; ExecutorModelPickerProps.scopeKey is a dead prop (the key={scopeKey} on the JSX does the remount); ExecutorModelPickerBoundary is a pure forwarding wrapper.
  • docs/antigravity-acp-pr2-acceptance.md reads as a run log — docs/README.md keeps those on the PR thread or in docs/archive/.
  • plugin.platform.query's 'executors' view and the executorConfig nested channel on session create appear to have no production consumer yet — fine if they're PR3/PR4 groundwork, worth a comment saying so.
  • The ACP-runtime vs Antigravity-adapter package split is a real boundary with a documented second-adapter plan — keep it.

Nice work — this is a large diff but the layering is disciplined, the staged plan is honest about what PR3/PR4 still owe, and the verification story (real ACP 1.1.1, real Electron) is solid.

中文

关于拆分:建议一次合入。 126 个文件的依赖链严格线性,每个候选切点都会产生更差的中间态:先落插件不编译、先落 infra 要么 Host 启动即抛要么落一堆无消费者的查询面、先落 UI 会渲染一个永远报错的 picker、先落协议是纯死 schema。main 上的 executor seam(#5283/#5164)已经在了,本 PR 是它的第一个真实消费者——这正是 #5103 明文要求的"契约与生产消费者同 PR"。唯一可行的二切也得先把 coordinator 的包解析改 lazy,买到的只是评审体量,不解锁任何阻塞。

正确性:沿真实生产路径(选择 → IPC → coordinator → executor service → ACP 进程 → 事件回流)走完失败/重试/取消/恢复全链,核心不变量全部闭合——进程丢失留可读历史、取消有界且保留真实 stopReason、权限身份精确绑定、fs 回调容器校验完整、epoch 181 覆盖新 wire 面。三条 P3 在行内,均不阻塞。

可删小件(不挡合并):execute()/list() 仅测试调用、scopeKey 死 prop、纯转发 wrapper、acceptance 文档按治理应进 archive 或 PR 正文、executors 视图与 executorConfig 通道疑似暂无生产者(若是 PR3/PR4 地基请注明)。ACP 双包拆分建议保留。


Review generated with AI assistance (Devin); verified against head 7fc76eb.


async discover(input: { cwd: string; signal: AbortSignal }): Promise<ExecutorCatalogEntry> {
if (this.#disposed) return this.#catalogEntry('unavailable');
if (this.#catalog) return this.#catalog;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3 — trigger ② (failure/staleness path): the catalog caches permanently after the first successful discover(). #catalog is never invalidated on the success path — if the agent's model catalog changes without an executable-path change (Antigravity's models are server-side, so this is the expected case), the picker won't see new models for the life of the Host process, and assertExecutorAvailable (execution-composition.ts) will reject a valid new model as unavailable. Failure direction is safe (unavailable/auth entries aren't cached and can retry). A TTL, or invalidating on acp_config_invalid/policy change, would close it.

中文

discover() 成功后 #catalog 在 executor 生命周期内不再刷新——agent 服务端下发新模型时(Antigravity 正是这类)picker 长期看不到,assertExecutorAvailable 还会把有效新模型误判 unavailable,直到 Host 重启。失败路径不缓存可重试,方向安全;加个 TTL 或在配置失效时清缓存即可。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that the cache can become stale when Antigravity changes its server-side models. I am keeping the successful catalog cached for PR 2 because #5103 assigns account/catalog invalidation and refresh reconciliation to PR 4. Adding a TTL here would introduce refresh timing and selected-model reconciliation behavior before that slice. I have made the current Entry-lifetime cache and Plugin-reload workaround explicit in the README and PR scope, so the limitation is visible rather than presented as live refresh. I would leave the lifecycle change to PR 4.


function normalizeResult(result: PluginExecutorResult): PluginExecutorResult {
if (!result || typeof result !== 'object') throw new TypeError('Executor result is invalid');
if (result.status === 'completed' && typeof result.text === 'string') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3 — trigger ③ (trust boundary): completed.text passes through unbounded. Every other event text goes through isSafeEventText (≤8192), but a completion's accumulated text is emitted as-is — a runaway or misbehaving ACP agent can write an arbitrarily large single durable event, which may hit a store/transport per-event limit and end the turn in error. Same exposure class as existing streaming aggregation, but the single-event shape adds a real boundary. A cap in normalizeResult (or chunking text_complete in the backend) covers it.

中文

completed.text 无界:其他事件文本都过 ≤8192 的 isSafeEventText,唯独 completion 的累积全文原样落 durable event——失控 agent 可写任意大单条事件。在 normalizeResult 加上限或后端分片即可。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9483a78. normalizeResult now rejects completed text above 256 KiB in UTF-8 bytes, so the backend emits a bounded failure instead of a single oversized text_complete durable event. The focused backend test verifies that a streamed delta remains followed by error/complete, with no large completion event. The test also passed after the latest main merge.

const info = await file.stat();
if (info.size > MAX_TEXT_FILE_BYTES) throw new Error('ACP text file is too large');
const text = await file.readFile('utf8');
const start = line ? line - 1 : 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3 — trigger ③ (trust boundary): negative line returns the file's tail instead of an error. start = line - 1 with line: -3 slices from the end. Only affects files the agent could already read in full — no boundary escape — but it's accidental semantics. Reject line < 1 or limit < 0; also fine to leave.

中文

line 为负时 slice 返回文件尾部而非报错。agent 对本就有权整读的文件,无边界突破;顺手加个 line < 1 校验即可,不改也能接受。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9483a78. File reads now reject a non-positive or non-integer line and a negative or non-integer limit before opening the file. limit: 0 now returns an empty range. A focused filesystem test covers the invalid values and a normal slice, and passed after the latest main merge.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Thanks for the focused review. I agree that retaining one process per conversation creates an unbounded resource cost over a long-running Host.

For PR 2, I would take the documentation option you suggested. The #5103 checklist explicitly treats a session waiting for its next turn as active and requires its process to remain available while idle; closing the UI does not retire it. Evicting that process now would make an otherwise usable task history-only until PR 3 provides restoration. I will make the current limit and tradeoff explicit in the PR description and README: there is no idle timeout or process-count cap, and owned processes are cleaned up on task retirement and Plugin/Host teardown. I will track a bounded-retention policy separately, informed by PR 3's restoration behavior; I am not assuming PR 3 itself will implement that policy.

I will also tighten the scope statements: PR 2 keeps history readable after process loss but cannot continue the same external session; session/new passes mcpServers: []; and the PR 2 execution run reused an existing login, so it does not establish expired-auth recovery during execution. I will make a final pass over the added shared contracts to ensure they have concrete consumers in this PR.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Follow-up on my reply above: commit 0ab97618d updates the Antigravity Plugin README and the PR's Scope and limits. They now explicitly state that PR 2 retains one process per conversation while idle, has no idle timeout or process-count cap, cleans up on task retirement and Plugin/Host teardown, and leaves a lost Session history-only. The scope text also calls out mcpServers: [] and the limits of the cached-login execution evidence.

I opened #5620 to decide bounded process retention separately after the PR 3 restoration behavior is established; PR 3 itself is not being assigned that policy. In the shared-contract pass, I removed the unused optional ExecutorCatalogEntry.message field, which had no PR 2 producer or consumer.

Full workspace typecheck and the focused Core executor-catalog tests pass locally. Would the documented PR 2 tradeoff address your process-ownership concern?

Document catalog cache scope and archive PR2 acceptance evidence.

Generated-by: OpenAI Codex
Resolve the Host compatibility epoch collision at 182.

Generated-by: OpenAI Codex
@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

Thanks for the approval and the dependency review. I kept this as one vertical PR, addressed the three P3 comments inline, and merged current main (bc0786ee6) with the Runtime Host compatibility epoch advanced to 182.

For the optional cleanup notes: I removed the unused picker scopeKey prop and moved the PR 2 run log to docs/archive/, updating its links. PluginExecutorService.execute()/list() and the plugin.platform.query executors view are already present on the main baseline rather than introduced here. The nested executorConfig channel does have a same-PR consumer: Desktop sends the chosen model through Session creation/configuration, and Host admission applies it to the ACP Session before the first prompt. I kept ExecutorModelPickerBoundary because it owns the optional native fallback and the keyed remount when the task scope changes; inlining it would duplicate the large native picker subtree without changing behavior.

After the main merge, the full workspace build and typecheck, the merge-result protocol epoch guard, and 162 focused tests passed locally. Fresh GitHub checks are pending.

Regenerate the Astryx surface inventory for the combined UI file set.

Generated-by: OpenAI Codex
@Astro-Han
Astro-Han merged commit 0cb4fc3 into apache:main Sep 23, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants