ACP executor modes and scoped catalog lifecycle - #5826
Conversation
Generated-by: Codex
Generated-by: Codex
jackwener
left a comment
There was a problem hiding this comment.
[kabi-sol]
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.
No P0–P2 found in the assigned behavior scope at 5ccbfdb355ba5e44a91dc97822326deb273cbc57.
Mounted the production PluginExecutorService with AcpExecutor and a controlled ACP connection. Model-only, mode-only and combined changes remain pending, with the old durable configuration intact, until the Agent response is released. A second-option failure restores both Agent values and the persisted configuration. Directory-specific candidates stay separate; probes verified the 16-entry cache bound, 60-second expiry, explicit invalidation and rejection of a stale in-flight probe after invalidation.
All 65 ACP-plugin/service/catalog tests passed, including retained external-Session restoration and late-notification cases. As a control, the current tests against the parent commit's plugin implementation fail on caller cancellation, clearing a mode removed by a model change, and an unsolicited idle configuration notification. The last commit therefore fixes observable failures, rather than only changing assertions. Its Host-side complete-snapshot replacement was inspected, not separately exercised here.
Core/storage/MCP/runtime/ACP-plugin builds passed; current-head CI test is successful. No authenticated official Agent, real process-restart persistence, Desktop UI, full Host integration, or full repository suite was run in this review. The connection and state store in the behavioral probes are controlled test doubles.
jackwener
left a comment
There was a problem hiding this comment.
[kabi-opus-dev] Review of the protocol (epoch, plugin-platform.ts compatibility, executor-catalog.ts decoding), Desktop IPC/preload/bridge signatures, and the architecture gate. Automated review (Claude). Same owner as the coordinating seat, so this is not independent corroboration.
Bound to 5ccbfdb355ba5e44a91dc97822326deb273cbc57; head re-checked before publishing. CI test is green on this head. That matters here: the P1 below ships through a green CI because no test sends a catalog query through the real decoder without refresh.
Conclusion: 1×P1, 1×P3 (both inline). Everything else in my scope holds.
P1 — every non-refresh executor catalog query is rejected
plugin-platform.ts:345 lists refresh in requireExactRecord, but assertExactKeys (codec.ts:53-68) requires every listed key to be present. The Desktop IPC handler (runtime-host-session-catalog-ipc-main.ts:122) sends refresh only when it is true, and the renderer's ordinary load (use-executor-selection.ts:81, force false/undefined) is exactly that case. client.request runs spec.decodeInput(input) before sending (client/connection.ts:510, client/reconnecting-connection.ts:295), so the request is rejected on the client with Invalid Executor catalog fields, and the new-task executor picker lands in its error state with an empty catalog. Details, reproduction and a verified one-line fix are inline.
① Protocol
- Epoch: 198 → 199 with a changelog line. The merge-base and current
origin/main(0f98c2a48, 2 commits ahead) are both at 198, andgit merge-tree --write-tree HEAD origin/mainexits 0. No epoch conflict. plugin-platform.tsand older peers: mixed-epoch peers are rejected at the handshake, so a 198 peer never seesrefresh,modes,currentModeorsupportsModeChange. The compatibility story is the epoch, and it is bumped. Within epoch 199 the new optional field is not actually optional: that is the P1.executor-catalog.tsstrictness: themodeconfiguration validation mirrorsmodel(non-empty, ≤1024, no NUL/CR/LF), and the protocol test adds'','bad\nvalue'and4.modesis capped at 64, ids are deduped, names are checked byisCatalogText, and the output is re-frozen with only{id, name}. One gap: a mode entry with noidis accepted (P3 inline).currentModeis not required to be a member ofmodes; that matches the existingcurrentModel/modelsprecedent, so I am not grading it.
② Desktop IPC / preload / bridge
The signature grows by an optional refresh?: boolean consistently in bridge-contract.d.ts:1022, preload.ts:1928, conversation/ports.ts:91, and the IPC handler, which also rejects a non-boolean refresh. The only caller is use-executor-selection.ts:81. Typecheck of desktop tsconfig.{preload,main,renderer,storybook} (run individually) plus @maka/core, @maka/runtime-host, @maka/runtime, @maka/ui and @maka/acp-executor-plugin: all exit 0, so every implementer and caller is in sync at the type level. The P1 sits below the type system: the TS type says optional, and the runtime decoder says required.
③ Architecture gate
--base ae71ab319 --strict-base and --base 0f98c2a48 --strict-base (current main): both passed, fixtures 112/112. The PR does not touch renderer-architecture.json, and regenerating it with --write at head gives a byte-identical file. Knip --workspace apps/desktop and --workspace packages/ui both exit 0.
Tests run
All 8 touched test files: core 9/9, runtime-host 109/109, runtime 12/12, ui 22/22, acp-executor-plugin 44/44, desktop 6/6. They all pass alongside the P1, which is the coverage gap.
Not verified
I did not test mode selection behaviour end to end against a real ACP agent, the scoped catalog lifecycle or refresh semantics in the Host coordinator, UI rendering, E2E, or the full suite.
zhiiw
left a comment
There was a problem hiding this comment.
Bound to 5ccbfdb355ba5e44a91dc97822326deb273cbc57 (re-checked against GitHub immediately before posting, unmoved). Not draft; CI label + test green on this head. Real Windows machine, Node 24.18.1.
No P0–P2, no P3, no inline comments. Both named ablations land on exactly their pins; the path-casing question has a direct answer.
Suites on this head (Windows) and attribution
acp-executor-plugin44/44 · runtimeplugin-executor-service12/12 · uiexecutor-model-picker22/22 · coreexecutor-catalog9/9 — all green.- runtime-host
session-catalog-coordinator+session-catalog-protocol+external-agent-setup-coordinator: 107/109 — both failures are the known no-Developer-Mode environment class:EPERM: operation not permitted, symlinkwhile creating their fixtures (creation persists a canonical cwd…andHost-path relocation canonicalizes once…). Both are symlink fixtures for the canonical-cwd path; neither can create its fixture on this machine. Zero assertion failures.
Path casing / drive letters on the catalog key (asked specifically)
The cache key is await realpath(resolve(input.cwd)) (index.ts:212). Direct probe on this machine: C:/Users/wzy, c:/users/wzy, C:\Users\wzy all resolve to the identical on-disk casing C:\Users\wzy — so case variants and slash styles of one directory share one cache entry on Windows, and a symlinked spelling lands on its target's key. The two EPERM-skipped fixtures are exactly the symlink-arm of this; the casing arm is verified above without them.
Ablations (each restored, rebuilt, re-verified — full plugin file 44/44 green after)
- Combined-change rollback removed (the restore-both-fields-and-verify loop in the
catch) → exactlya failed second configuration option restores both model and modegoes red. - Catalog scope made global (the cache key forced to a constant instead of the canonicalized cwd) → exactly
catalog reuse is scoped to cwd and refresh only replaces that scopegoes red.
Read, no finding
- The provider-confirmation-as-complete-snapshot semantics (an omitted option is cleared) is consistent across the plugin (
withConfirmedConfiguration), the coordinator's confirmation check (#mergeConfigurationPatch+ the model/mode mismatch throw), and the fingerprint (mode included). The idle notification path refuses to adopt a notification that disagrees with the local model/mode during a mutation — the race is closed from both directions. - The busy gate for executor configuration now also covers
isTurnBusyand non-activeheaders — a stricter gate, matching the confirmation contract.
Not checked
The two symlink-fixture tests (environment, above), e2e, full repo suite.
UTC 2026-09-29 11:05.
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed the ACP executor catalog/mode path, the Desktop picker-to-Host request path, and the protocol boundary at commit 5ccbfdb355ba5e44a91dc97822326deb273cbc57. This is not ready to merge.
The P1 already reported on this head is reproducible: plugin-platform.ts:345 passes ['kind', 'cwd', 'refresh'] to requireExactRecord, whose assertExactKeys requires every listed key (codec.ts:63-66). The normal Desktop IPC request omits refresh unless it is true (runtime-host-session-catalog-ipc-main.ts:119-122), and the client decodes before sending (connection.ts:510). A direct call to the built decoder with {kind:'catalog',cwd:'/tmp'} throws Invalid Executor catalog fields, while refresh:true succeeds. Thus opening the executor picker without a forced refresh cannot obtain its catalog. I am not duplicating the existing inline finding. I also confirmed the separately reported P3 validation gap in executor-catalog.ts:130-133: isExecutorConfiguration({mode: undefined}) does not establish that each mode entry has a string id.
The current-head hosted test and label checks pass, and a fresh-main merge-tree and diff check are clean. Locally, core/storage/runtime/runtime-host builds and 44 Plugin Platform tests passed. I did not exercise a real ACP agent, packaged Desktop, or the complete repository suite. The protocol P1 remains a merge blocker despite those green checks.
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.
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 5ccbfdb3 and re-checked at exact head 9754140008d0eaba09d82b2efd55a7007ae898df. The new commit fixes the earlier refresh codec P1 and the mode.id P3. It doesn't touch session-catalog-coordinator.ts or acp-executor-plugin/src/index.ts, so the findings below, and their line numbers, still apply to this head.
P1: a model or mode change is now rejected unless the Session is active (session-catalog-coordinator.ts:857). On main, only waiting_for_user was blocked. A failed turn leaves the Session blocked, and a user stop leaves it aborted, so the model can no longer be switched after a model or quota failure. restore() for a restorable Session whose last turn failed is rejected too. Sending is already blocked while readiness isn't ready, so that Session has no way forward. Reproduced: with the compiled coordinator fixture, blocked/aborted return operation_conflict and configureExecutor is never called, while active succeeds.
P1: agent-originated config_option_update is dropped when model or mode differs (acp-executor-plugin/src/index.ts:978-985). This leaves configOptions stale, so the next prompt skips setConfigOption and runs on the agent's own configuration, while Maka reports and persists the old one. Reproduced: I changed only the fake agent so that its notification really changes its state. The existing test then shows a prompt that requested default running on fast. Modes can matter for permissions (plan vs. edit).
P2:
- On restore, a model or mode mismatch fails before any attempt to re-apply the saved configuration, so every retry fails the same way (
index.ts:664-673). - Catalog discovery now opens an agent session in the user's real workspace every 60s per directory, where it used to use a disposable temp directory (
index.ts:212). This needs a trust/design sign-off.
P3: refreshing aborts the shared in-flight probe, so callers already waiting on it get unavailable. A mode-only change with no live session also resets the model to the agent default (both reproduced).
Other notes:
- This head sets
RUNTIME_HOST_COMPATIBILITY_EPOCHto 199 (packages/runtime-host/src/protocol/index.ts:107). Several other open PRs also claim 199, so whichever merges later needs to renumber. - Older builds will reject session headers that carry
executorConfig.mode, so downgrading is not safe.
Checks and tests:
git diff --check,check:asf-headers,check:app-shell-hooksandcheck:renderer-architectureall pass.- After targeted builds, all 196 tests in the 7 affected test files pass.
- Not run: the desktop suite, the full workspace tests, and a real ACP agent.
Automated review notice: This comment was posted by an automated review agent (Claude) operating on behalf of @Astro-Han. It is not an independent human review and does not replace one.
Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the two commits since 5ccbfdb3 at exact head 1d5525cb3fe85eb0b43c89d5ce47b98ec375aad1, focusing on the prior protocol/configuration findings, ACP restore and notification flow, Host configuration changes, and added regressions. One new P2 is inline; I would not merge this head yet.
The ordinary catalog request now decodes without refresh, and a mode entry missing id is rejected. The Host permits idle configuration changes after blocked/aborted turns, restore re-applies saved model/mode, and a mode-only edit passes the merged configuration to the provider. The awaited probe is redirected after a refresh. The prior concern about opening an ACP agent session in the real workspace during discovery remains a trust/design question; I did not verify a real agent's workspace hooks or external history behavior. This PR's protocol epoch is 199 and must be reconciled with whichever concurrent epoch-199 change merges first.
Core, ACP plugin, and runtime-host builds passed on Node 24. All 220 focused tests across the four affected test modules passed. The current-head hosted test check passed; fresh main ec324da4 merged cleanly and git diff --check passed. I did not run a real ACP agent, packaged Desktop, or the full workspace suite.
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.
Re-reviewed exact head 1d5525cb3fe85eb0b43c89d5ce47b98ec375aad1 against my earlier findings on 97541400.
Fixed
- Config changes on
blocked/abortedSessions: onlyrunning/waiting_for_userare rejected now, and there are tests for all four states. - Divergent agent notifications: they now update the cache without being persisted as confirmed. The adopted test uses a fake agent that really switches.
- Restore: it re-applies the saved model/mode and fails only if the agent still disagrees.
- The partial-patch reset: the merged config is now sent.
- Refresh no longer strands callers waiting on the in-flight probe.
invalidateCatalog()on auth failure still resolves them asunavailable, which is minor.
Still open
- The catalog probe still opens an agent session in the user's real workspace (every 60s per directory) instead of a disposable directory. This needs an explicit trust/design decision.
- The separate P2 from the other review on this head still applies: drift that arrives during an active prompt is persisted as confirmed at ack time.
New P3s (both reproduced with temporary tests)
index.ts:1034(inline): a second identical drifted notification is persisted as confirmed.session-catalog-coordinator.ts:872: sending the merged config means a model-only change also re-applies a saved mode. That fails withacp_config_unavailable: modeif the new model has no mode option. This is a narrow case.
Epoch
The epoch is still 199 (packages/runtime-host/src/protocol/index.ts:107), so the collision with other open PRs remains.
Checks run
git diff --checkis clean.- The ASF header, renderer architecture and app-shell hooks checks pass.
- Targeted tests pass: acp-executor-plugin 46/46, coordinator 76/76, session-catalog-protocol 24/24, protocol 89/89, executor-catalog 9/9, plugin-executor-service 12/12.
- Not run: anything against a real agent, and the full suite.
Automated review notice: This comment was posted by an automated review agent (Claude) operating on behalf of @Astro-Han. It is not an independent human review and does not replace one.
Apply merged configuration once when opening a fresh Session, so a model change may remove an obsolete mode. Generated-by: Codex
|
Follow-up on the latest review: 2c98f49 fixes the fresh-Session model-only case where the selected model removes the prior mode. Initialization now applies the merged selection once; a regression test confirms that the provider returns { model: fast } and clears the saved mode. The two notification-drift findings are also fixed and tested in the same commit. The workspace-probe trust assumption is documented and discussed in its inline thread. The protocol epoch is still 199; apache/main currently remains at 198, so an epoch collision depends on which concurrent PR merges first and should be reconciled against main before merge. |
Generated-by: Codex
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Re-checked at 2c98f49b and re-bound to exact head 5b786f8d90a250a4b43a4f4bb0ce5a0bd165fd3e. The two newest commits only touch use-executor-selection.ts, its test, and the acceptance doc. acp-executor-plugin/src/index.ts and session-catalog-coordinator.ts are unchanged, so the findings below apply to this head.
Fixed since my last review: drift during an active prompt is no longer persisted as confirmed at ack time. A repeated drift notification no longer overwrites the saved selection either, since agent notifications no longer write durable state. The PR's new tests and my reproductions both keep the saved model/mode.
Still open:
- Model-only change re-applying the saved mode (
session-catalog-coordinator.ts:872, P3) is only partly fixed. It still fails withacp_config_unavailable: modefor an existing Session whose mode drifted while idle. It also fails for a fresh Session whose saved mode isn't the agent default. - The catalog probe in the real workspace is now documented in the acceptance doc (use discovery only with an agent trusted for that workspace, probes across directories are unbounded), but the code is unchanged. This still needs a maintainer design decision.
invalidateCatalog()waiters still resolveunavailable(minor).
New P3, a regression from 2c98f49b (inline at index.ts:360): on a fresh Session, configureConversation now applies the launch config first (for Antigravity, the configured default model) and then validates the requested config against that model's options. If the launch model has no mode option, {model:'default', mode:'auto'} fails with acp_config_unavailable: mode before anything is applied, and the fresh Session is lost. I reproduced it: the same scenario succeeds on 1d5525cb and fails on 2c98f49b. The prompt path is unaffected.
Epoch: still 199 (packages/runtime-host/src/protocol/index.ts:107) vs 198 on main.
Checks (at 2c98f49b):
git diff --checkclean,check:asf-headerspasses.- Tests: plugin 48/48, coordinator 76/76, catalog protocol 24/24, core catalog 9/9, antigravity 9/9.
- Not run: the full suite, lint, and anything against a real agent.
Automated review notice: This comment was posted by an automated review agent (Claude) operating on behalf of @Astro-Han. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 5b786f8d90a250a4b43a4f4bb0ce5a0bd165fd3e, focusing on the three commits since 1d5525cb: ACP confirmation/persistence, the Desktop executor-selection hook and its regressions, and the acceptance-document update. The earlier P2 is fixed on this head: an Agent-originated configuration update changes the observed options, but neither acknowledgement nor the notification persists it as the user's confirmed selection (acp-executor-plugin/src/index.ts:813-834,1019-1027). The Desktop hook now gives saved or locally confirmed configuration precedence over observed Agent values (use-executor-selection.ts:154-174,195-215). I found no additional substantiated P0–P3 issue in these inspected paths.
This is not a merge endorsement. The separately reported fresh-Session mode availability P3 remains in the configuration path (index.ts:354-361,786-793), and discovery still opens an Agent session in the selected workspace. The new documentation describes that trust assumption, but a maintainer still needs to accept the design. The acceptance document reports an authenticated official-Agent follow-up; I did not independently run that Agent or verify its claims.
On Node 24, the ACP plugin build and 48 focused ACP tests passed. The emitted current-head Desktop executor-selection test passed all 7 cases. A full UI/Desktop TypeScript build did not pass locally because SideNavItemProps.trailingAction is missing in an unchanged UI path, so I do not count it as a build pass. Fresh main 53f566b4 merges cleanly and git diff --check passes. The current-head hosted test check was pending when this review was posted. I did not run a packaged Desktop or the full repository suite.
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.
Record authenticated Desktop acceptance and post-restart continuation. Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head e9a1b18ff2e829d699947f1250cf10491f69cff5. This commit changes the macOS “choose existing program” picker from a file to the extracted directory and appends agy_acp_server.par; other platforms retain file selection (apps/desktop/src/main/runtime-host-boot.ts:1627-1634). The selected path is saved by the settings page, and connection setup verifies the server file and adjacent localharness_external helper before launching (packages/runtime-host/src/server/acp/antigravity.ts:62-65,128-139). Locale copy and the acceptance document were updated. I found no new substantiated P0–P3 issue in this increment.
The acceptance document reports a successful macOS development-build flow, but I did not independently reproduce the signed-in Agent or packaged Desktop path. A previously reported fresh-Session mode-availability P3 remains outside this commit. The current-head test check is red in the unchanged Runtime Host gitoxide-helper-invocation-internal process-identity wait (1 failure among 2,197 tests); it must be rerun or resolved before merge. The PR merges cleanly with fetched main and passes git diff --check. Protocol epoch 199 and the real-workspace catalog-probe trust assumption still require maintainer resolution; this is not 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.
|
Follow-up to @hqhq1025's review at e9a1b18: I merged current Validation: The fresh-Session mode-availability P3 remains open, and I have left its review thread unresolved. The workspace-probe trust assumption remains documented for maintainer review. Already-fixed inline findings have individual replies, so I have not duplicated them.Follow-up to @hqhq1025's review at e9a1b18 I merged current apache/main through c61cf9b in ff70ec9. Storage usage remains protocol epoch 199 Agent Graph remains 200 and this PR's ACP mode/catalog change advances the epoch to 201. GitHub now reports the branch as mergeable. The merge-result protocol epoch guard passes 200 to 201 all 17 protocol epoch guard tests passed during conflict resolution and git diff --check is clean. The previous head's failed test run was rerun successfully CI for ff70ec9 is still running. The fresh-Session mode-availability P3 remains open and its thread remains unresolved. The workspace-probe trust assumption remains documented for maintainer review. Already-fixed inline findings have individual replies so I have not duplicated them. |
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head, including the model-before-mode configuration repair and the subsequent acceptance-document updates. packages/acp-executor-plugin/src/index.ts:877-939 now selects the model first, validates the dependent mode against the Agent’s returned options, and rolls back an invalid post-model selection. The added regression cases cover a fresh Session, idle drift, and rollback. The merge advances the ACP wire change to epoch 201 after main’s epoch 200 (packages/runtime-host/src/protocol/index.ts:107-112). I found no substantiated new P0–P3 issue in the inspected increment.
Node 24 ACP plugin build and all 51 plugin tests pass; the protocol epoch guard, fresh-main merge-tree, and diff check pass. The current-head hosted test is still running, so the gate is not yet green. I did not independently reproduce the documented authenticated official-Agent/Desktop runs, cross-project UI switching, or a signed distributable. Catalog probing invokes the configured Agent in the chosen workspace, so the trust decision described in the acceptance record remains for maintainers.
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.
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Incremental re-review at 82d6b12b695a2967f2e907915afb817ff5087f9b. My previous review was at 6c4cd2d3. The branch was fast-forwarded with three commits: cc9c01224 (isolate draft catalog probes), 8ef010563 (validate program selection and consolidate protocol history) and 82d6b12b6 (Windows test inventory). All three carry Generated-by: Codex. Merge-base is still 1e80e3b8. Current main (c7fa6bb6) is at compatibility epoch 202, so this PR's 204 passes the guard.
Earlier findings:
- P2, catalog probe opened
session/newin the user's workspace with unbounded per-directory concurrency: fixed in code.AcpCatalognow probes an emptymkdtempdirectory and never uses the project path. It keeps one 60s cache per executor instance, runs one probe at a time per executor (a refresh waits for the superseded probe to finish cleanup), and shares anAdmissionLimiter(2)across adapters. The permit is held until the connection and temp directory have been disposed. Retained task Sessions still validate model and then mode in the real workspace before any prompt. The remaining caveat is documented: the Agent may keep an empty native Session for each probe. - P3, two epoch-history entries: fixed. There is now a single
// 204:entry. - P3, macOS picker blindly appended
agy_acp_server.par: fixed. The picker allows a file or a directory, appends the server name only when a directory was picked, and validates the executable and its siblinglocalharness_externalwith the same checker the Host uses. Failures are reported as the localized executable or helper error. - P3 (carried), catalog waiters receive
unavailableon invalidation: still present (inline). Low impact. - Design note: mode copy still does not say that modes such as
yoloare Agent-side auto-approval that bypasses Maka's permission prompts. Not blocking.
New:
- P2 (verification gate): the neutral-probe path has not been run against the signed-in official Agent or Desktop. The acceptance doc says so explicitly (inline).
- P3: each probe uses a fresh random temp path, so the Agent's native history gains an entry under a new workspace path on every TTL expiry or refresh (inline, optional).
Verification (local, Node 22.23): acp-executor-plugin 84/84; runtime-host antigravity, external-agent-setup and protocol suites passed 100 with 7 pre-existing platform skips; antigravity-acp-plugin 9/9; runtime admission-limiter and plugin-executor-service 20/20; desktop external-agent-executable-selection 11/11. Hosted CI: everything passes except test, which was still pending when I checked. GitHub reports the PR as MERGEABLE, with no conflicts.
Conclusion: the earlier blocking probe-design concern is addressed in code. Before merge, please attach real-Agent acceptance of the neutral probe. The P3s are optional.
This is an automated review by Claude (Anthropic), run on behalf of the maintainer. It does not replace human review.
| } | ||
|
|
||
| async #probeCatalog(signal: AbortSignal): Promise<ExecutorCatalogEntry> { | ||
| const directory = await mkdtemp(join(tmpdir(), 'maka-acp-catalog-')); |
There was a problem hiding this comment.
P2 (verification gate): the neutral-directory probe has not been checked against the official Agent.
The acceptance doc says that the two-directory official-Agent and Desktop evidence belongs to the earlier workspace-scoped implementation, and that no official-Agent run is claimed for this follow-up. An empty, unrecognised temp directory is exactly where an Agent might behave differently. It could apply workspace-trust gating, fail session/new (the picker then shows only unavailable for every draft), or return a different mode list. The stdio fixture cannot show any of this.
Fix: before merge, rerun acceptance steps 1-4 on this head with the signed-in official Agent. Record the session/new cwd, the models and modes it returns, and that the temp directory is removed afterwards. Then post the evidence on the PR.
There was a problem hiding this comment.
Acceptance steps 1–4 passed on exact head 82d6b12b695a2967f2e907915afb817ff5087f9b, using the signed-in official Agent 1.2.1 on macOS arm64 (Node 24.19.0), with a freshly built development Desktop and one dedicated profile.
Official-Agent neutral probe: the production AcpExecutor used the real server/helper. A transparent observer delegated every request to the production connection and recorded only initialization version and session/new cwd. Both initialization responses reported Agent 1.2.1 / protocol 1. The two actual probe requests used empty directories ending in /T/maka-acp-catalog-BqUc4Q and /T/maka-acp-catalog-TirUh5, never either selected project. Both directories were removed before their discovery calls returned; both empty projects remained unchanged.
Initial discovery and refresh both returned ready, with mode IDs default, auto_edit, yolo and these 11 model IDs:
gemini-3.8-flash-high
gemini-3.8-flash-medium
gemini-3.8-flash-low
gemini-3.7-flash-high
gemini-3.7-flash-medium
gemini-3.7-flash-low
gemini-3.6-flash-high
gemini-3.6-flash-medium
gemini-3.6-flash-low
gemini-pro-agent
gemini-3.1-pro-low
The probe default was gemini-3.8-flash-high / default. A/B returned the same initial catalog object. Refreshing A replaced that object, and the following B query returned the replacement. Exactly two probe Sessions were created in this sequence.
Real Desktop, same profile:
- The native macOS picker accepted both the official executable and its extracted directory. Incomplete directories produced the localized missing-server/helper errors, and the saved runtime-policy path remained the verified official executable. Connection and Google sign-in verification succeeded afterwards.
- Both empty projects were added through the native folder picker. B showed the four Gemini model families and Default/Auto Edit/YOLO; Gemini 3.7 Flash, High, Default was selectable.
- Switched B → A, pressed Retry, observed loading and completion, and selected Gemini 3.8 Flash, High, Auto Edit.
- Returned A → B without pressing Retry in B. Its model and mode choices remained available. Cache identity and neutral cwd are established separately by the production-executor check above.
Official server/helper SHA-256 values match the recorded 1.2.1 distribution: c93c86c0f505fcdf8b13c695bed26d306141ef5446189d591397074d324db34e / 1b8a2b712ca312c9769e425b800bfbcceec4770f19736404474d1e8e50d65456.
Fresh build, typecheck, lint, format, renderer architecture, locale hygiene, ASF headers and diff checks passed. ACP/Antigravity tests passed 93/93; Desktop picker/settings passed 28/28, with no skips or failures. The exact-head hosted test also passed. Observer script, actual-cwd log, JSON results, Desktop profile and gate logs are retained in pr5826-neutral-acceptance-2026-10-02.
This closes the requested neutral-probe verification gap. Prompt/restart steps 5–9 were not repeated in this run; their earlier-build evidence remains historical. The optional native-history/stable-directory concern remains separate.
| } | ||
|
|
||
| async #probeCatalog(signal: AbortSignal): Promise<ExecutorCatalogEntry> { | ||
| const directory = await mkdtemp(join(tmpdir(), 'maka-acp-catalog-')); |
There was a problem hiding this comment.
P3 (optional): a random path for every probe spreads Agent-native history across many "workspaces".
The README accepts that the Agent may keep an empty Session for each probe. With mkdtemp, each of those Sessions is filed under a different, now-deleted path, so every TTL expiry or refresh adds one more workspace to the Agent's history.
Suggestion: use a stable neutral directory for each adapter, for example <maka data>/acp-catalog/<adapter-id>. Empty it after each probe as you do now. Probes are already serialized per executor, so a stable directory cannot be shared by two concurrent probes, and the leftover history collapses to a single workspace key.
There was a problem hiding this comment.
Fixed in 4fefdc1.
Registered adapters now obtain a stable neutral scratch directory from Plugin storage. Its identity includes the Host data root, Plugin namespace, adapter ID and composition Entry. Refresh, TTL expiry and Host/runtime replacement reuse that path. The Host reuses its existing native file leases to serialize independent runtime instances and processes; the lease covers probe-process disposal and directory cleanup, and the next owner clears residue after a crashed owner releases its OS lease. No native dependency is added to the self-contained ACP Plugin bundle.
Official Agent 1.2.1 verification recorded three actual session/new requests (initial query, refresh, executor/runtime recreation), all with the same empty neutral cwd. The actual installed production Plugin bundles also passed A/B shared-cache and forced-refresh checks with one stable path. Both toy projects stayed unchanged. Controlled regressions cover namespace separation, competing instances/processes, cancellation and owner termination.
This reduces cwd churn; Agent-owned empty Sessions are still outside Maka's cleanup. Direct AcpExecutor embeddings without Host storage reuse a path within the instance lifetime and can supply withCatalogDirectory for durable ownership. Evidence and exact tested commit are recorded in the acceptance record.
| if (result.readiness !== 'unavailable' || this.#disposed) return result; | ||
| if (this.#cached && this.#cached.expires > Date.now()) return this.#cached.entry; | ||
| const replacement = this.#pending?.promise; | ||
| if (!replacement || replacement === pending) return result; |
There was a problem hiding this comment.
P3 (carried from the earlier review): waiters still get unavailable when the catalog is invalidated without a refresh.
invalidate() (setup or login via invalidateCatalog) aborts the pending probe and clears #pending. The waiter loop then finds no replacement here and returns unavailable, so an open picker briefly shows the executor as unavailable. Impact is low, because setup success fires onCatalogChanged and the renderer queries again.
Fix (optional): when result came from a superseded revision and !this.#disposed, start a new probe for the waiter rather than returning unavailable. For example, loop back through get(signal).
There was a problem hiding this comment.
Fixed in 4fefdc1.
Each pending probe now records its catalog revision. After a superseded probe drains, existing waiters loop back to the current revision and start or join its shared replacement, even when invalidateCatalog() was called without a refresh query. Explicit refresh invalidates only once, and the existing tail barrier still prevents replacement startup before old-process cleanup completes.
Retry is based on revision mismatch, not on unavailable: genuine failures, caller cancellation and disposal remain terminal. Regressions cover multiple original waiters without a new query, repeated invalidations, rejection of stale cache writes, cancellation/disposal, and genuine unavailable/authentication results. Reverting the revision change makes the invalidation regression fail.
The 97 ACP/Antigravity tests and 189 affected Runtime/Host tests passed with no skips or failures; full build, typecheck, lint, format and the affected repository gates also passed. Acceptance record.
Provide stable namespaced scratch directories through Plugin storage and reuse existing native file leases across runtime instances. Keep leases through probe cleanup and recover residue after owner exit. Redirect catalog waiters by revision without retrying genuine failures. Generated-by: Codex
Record official Agent checks at 4fefdc1, production Plugin bundle integration, 286 affected tests and negative regressions. Keep Desktop 1-4 and historical 5-9 evidence scoped to their tested implementations. Generated-by: Codex
jackwener
left a comment
There was a problem hiding this comment.
Reviewed exact head d5f43b2b8a9b86e9350cfdd0cf5b8ac773c1e381 against base 1e80e3b885d963b799f7e2e9a70083ceda99943d and current main at 887924e14aa3fe78b746fd88a43548d7335b9573.
Finding
P2 — add the required Generated-by: Codex trailer to the latest test commit
The PR states that Codex implemented the tests, but d5f43b2b8a9b86e9350cfdd0cf5b8ac773c1e381 adds 316 lines across four affected regression-test files without a Generated-by: trailer. It is the only missing trailer among the 26 non-merge commits in the current PR range. Please amend that commit with Generated-by: Codex and preserve the trailer through the final history.
Previous catalog-probe finding
The earlier P2 is closed. Draft discovery now ignores the selected project path and uses one stable, empty Plugin scratch directory scoped to the Host data root, Plugin namespace, adapter, and composition Entry. Native lifetime leases serialize that path across runtime instances and processes, and the lease remains held until both Agent-process disposal and directory cleanup finish. Catalog probes across adapters also share a two-permit admission limit.
I verified the production path with a mutation: changing discovery to initialize directly in input.cwd made the project-specific-startup regression fail with model fast instead of default; restoring the current neutral-catalog path made it pass. The recorded official-Agent 1.2.1 acceptance at implementation commit 4fefdc1e7 also remains applicable because the later commits through this head change only documentation and tests.
Verification
On Node 24.18.1, the exact head completed a full workspace build and passed 402/402 affected ACP, Antigravity, Runtime, Runtime Host, Desktop, and UI tests. Biome format/lint, renderer architecture 121/121, locale hygiene, ASF headers, the Windows skip inventory, diff checks, and the protocol epoch guard (202 → 204) passed. The exact-head hosted test check is successful.
The PR also merges cleanly with current main as tree 638aa80ccdbf59e91e5e2834623ed359e6cd4534. The merge result passed the full workspace build, the same 402/402 affected tests, and the static gates. I did not independently rerun the signed-in official Agent or the packaged Desktop acceptance in this pass.
Conclusion: BLOCKED — the technical catalog-probe concern is closed, but the latest test commit still needs its required attribution trailer.
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.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed d5f43b2b8a9b86e9350cfdd0cf5b8ac773c1e381 (50 files, +3897/−300, 29 commits). Scope stated first: this is a large change, so I verified the head, the gate, the protocol bookkeeping and the status of the findings already on this PR rather than reading all 50 files.
No new P0–P3 findings from what I checked.
Earlier findings cannot be carried forward, and that is worth saying explicitly. My line's most recent review of this PR is bound to 5b786f8d, which is not an ancestor of this head — the branch has been rebased, so old line numbers and hunks no longer refer to anything here. Worse for reuse, the two files those findings sat on, packages/acp-executor-plugin/src/index.ts and packages/runtime-host/src/server/session-catalog-coordinator.ts, have both changed since. So the earlier conclusions need re-verification against this revision rather than being assumed fixed or assumed open, and I did not perform that re-verification here.
Protocol bookkeeping is clean, which is the part I could check cheaply and completely. The epoch is 204 against main's 202. The gate that guards this (scripts/protocol-epoch-check.mjs) fails only when the epoch goes backward (headEpoch < baseEpoch), so a two-step advance passes; the only observation is that a minimal bump would be one step (203) had the author wanted the tighter number. More importantly, the two unrelated compatibility ledgers (mechanical-candidate-sweep.json, turn-snapshot-optional-fields.json) are untouched — "epoch": 200 on both main and this head — so this PR does not do the thing I have flagged on other PRs in this repository, where an unrelated argument's ledger gets re-dated along with a bump.
Gate on this head: test is green (alongside the installed-CLI validations in the same job). All 26 commits carry Generated-by: trailers naming Codex, and there is no Grok involvement.
What I did not judge
- The substance of the change: optional ACP modes across catalogs, Host and Desktop; the draft-discovery scratch scoping and its 60-second cache; the native file leases; the advisory-versus-confirmed split; and the macOS executable selection. That is the deep pass this PR deserves and it is not what I delivered here.
- Whether the currently-open earlier findings are now fixed, for the rebase reasons above.
- No Electron run.
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.
Incremental review of d5f43b2b against our last review at 82d6b12b (fast-forward, three commits: stable leased probe paths with restarted waiters, the acceptance record, and boundary tests).
Both open items from the last round are addressed.
- P2 (temp-directory probe not run against the real signed-in Agent): resolved. The PR body and
docs/archive/antigravity-acp-pr4-acceptance.mdnow record Official Agent 1.2.1 rechecked at4fefdc1e7: three realsession/newrequests (initial discovery, refresh, executor/runtime recreation) reused the same empty neutral cwd and completed cleanup, with both toy projects unchanged. The earlier full Desktop acceptance (model/mode and cwd confirmed, idle configuration changes, restarts) is recorded againstccd3c195e. - P3 (a new random temp path per probe): fixed. Probes now reuse a leased, stable Plugin scratch directory (
plugin-scratch-directory.ts,plugin-data-runtime.ts), so the Agent's history no longer gains an entry under a new path on every refresh. - Carried P3 (catalog waiters receiving
unavailableon invalidation): fixed.AcpCatalog.getnow loops: each pending probe carries its revision, and a waiter whose probe was superseded byinvalidate()starts or joins the replacement instead of returning the staleunavailable. Genuine probe failures at the current revision still return to the caller, so a real failure is not retried forever. The newacp-catalog.test.tscases cover the restart.
Gates: every check on this head passes and GitHub reports it mergeable. The protocol epoch is 204 against main's 202; the two unrelated compatibility ledgers are untouched.
Not re-verified here: the acceptance runs themselves (taken from the author's record), and the rest of the 50-file change, which earlier rounds covered.
This is an automated review by Claude (Anthropic), run on behalf of the maintainer. It is not an independent human review.
Generated-by: Codex
d5f43b2 to
4b7fb5e
Compare
jackwener
left a comment
There was a problem hiding this comment.
Approved at exact head 4b7fb5e88e3075417f2a2dac89d6cb6e492a6049. I found no unresolved P0–P2 issues.
The earlier catalog-probe concern is closed. Draft discovery ignores the selected project path and uses a stable, empty Plugin scratch directory. Native lifetime leases serialize that path across runtime instances and processes through Agent disposal and cleanup; probes across adapters share a two-permit limit; superseded probes drain before replacement; and invalidated waiters follow the replacement revision. Changing discovery back to the project cwd makes the production-path regression fail, while the current implementation passes.
The final attribution issue is also closed. This head has the same source tree as the reviewed d5f43b2b8a9b86e9350cfdd0cf5b8ac773c1e381; the rewritten test commit now carries Generated-by: Codex, and all 26 non-merge commits in the PR range have the required trailer.
On Node 24.18.1, the reviewed source tree completed the full workspace build, 402/402 affected ACP, Antigravity, Runtime, Runtime Host, Desktop, and UI tests, and the static gates. The exact-head hosted test check is successful. The PR is open, non-draft, and mergeable. It also merges cleanly with current main ab5996bdbe125f85ec86a0b3d54a6738c16e7af8 as tree 95a5bab78b24783368380937f496012f484fdde1; every PR-affected path is byte-identical to the fully tested merge result from the previous main revision.
Review notice: This review was prepared by an automated review agent operated by jackwener and is published at the direction of sun-os, who has read these findings and is the human accountable for them.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 4b7fb5e88e3075417f2a2dac89d6cb6e492a6049. The dispatch said last round was clean and to look at the new commits; the useful fact is that this head's tree is byte-identical to the one I reviewed (git diff between d5f43b2b and this head reports zero files), so the revision moved without changing content.
That makes my previous pass's conclusions carry over unchanged, and they were:
- The protocol bookkeeping is clean. The epoch is
204againstmain's202, and the gate that guards it (scripts/protocol-epoch-check.mjs) fails only on a backward move (headEpoch < baseEpoch), so a two-step advance passes — a one-step bump would be the tighter number if the author wants minimality. The two unrelated compatibility ledgers (mechanical-candidate-sweep.json,turn-snapshot-optional-fields.json) are untouched ("epoch": 200on bothmainand this head), so this PR does not re-date an unrelated argument's ledger. - My line's earlier findings still cannot be carried forward as fixed or open. They sit on
5b786f8d, which is not an ancestor of this head, and on two files that changed since (packages/acp-executor-plugin/src/index.ts,packages/runtime-host/src/server/session-catalog-coordinator.ts). Re-verifying them against this revision remains open work, and the identical tree means nothing changed on that front either. - No new P0–P3 from what I checked.
Gate on this head: test is green, alongside the installed-CLI validations in the same job; mergeable is true. Every commit carries a Generated-by: trailer with no Grok involvement.
What I did not judge
- The substance of the change — optional ACP modes across catalogs, Host and Desktop; the draft-discovery scratch scoping and its cache; the native file leases; the advisory-versus-confirmed split; the macOS executable selection. That deep pass is still not delivered, and this card is a repeat of my previous bounded answer because the tree did not change.
- I did not re-run the suites; the identity of the trees is verified with
git diff, and the epoch/ledger facts by reading both revisions.
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.
Show Retry only when the executor catalog is unavailable or has an error. Carry the selected catalog into locally pending sessions, defer Host inspection until admission, and preserve the selected model and mode in the pending summary. Generated-by: Codex
Generated-by: Codex
Move first-send Session activation into the conversation feature and regenerate the merged renderer debt ledger. Validate the public test import, activation flow, and strict base architecture checks. Generated-by: Codex
Require confirmation of every explicit and saved configuration value after model selection. Preserve model-only mode side effects by keeping the inherited mode out of the Host provider request. Cover retained-session drift, explicit-mode rollback, and Host patch intent. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed b51fd2e73f9571cffbd7dffa316ee69a0440a5ff — the increment only, since that is what was asked.
The rebase is faithful. A git range-diff against the previously reviewed revision marks all of this PR's original commits as = with zero changed hunks, so the existing work was re-applied byte-identically onto the new base.
The +359 increment is three new commits, each self-explanatory and on-theme:
fix(desktop): preserve executor controls during first sendfix(test): import executor composer through conversation API— a test reaching the composer through the feature's API rather than a deep path, which is the boundary discipline this series is enforcingfix(acp): reject unavailable explicit modes before prompting— which is the behaviour the description already claims ("rejects unavailable explicit values before prompting"), so the change closes the gap between the stated contract and the code
The protocol question you asked: yes, the epoch is still 204 (main is at 202). That is two steps ahead rather than one; the gate that guards it (scripts/protocol-epoch-check.mjs) fails only on a backward move, so this is legal — a one-step bump would merely be tighter. The two unrelated compatibility ledgers remain untouched, as I verified in my previous pass.
Gate on this head: test is success, alongside the installed-CLI validations in the same job; mergeable is true.
What I could not judge
- Scope, stated plainly: I verified the rebase's fidelity, the identity of the three new commits and the gate. I did not audit their ~359 lines line by line, and the deep pass over this PR's ACP mode work remains the one nobody has delivered — this is a re-review of the increment only.
- No live ACP session, so "reject unavailable explicit modes before prompting" is verified as the stated intent of the new commit, not by exercising it.
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.
Automated review (Claude lineage). This was not written by a human, and it does not approve the PR. Please verify before acting.
Incremental review of b51fd2e7 against our last review at 4b7fb5e8. It covers three commits and the merge of main 8ad836ce1 (91df4acf, a103d2fa, 2d016ea3, b51fd2e7).
Verdict: no P0–P2. There are three P3s, plus one nit outside the diff.
Merge 2d016ea3: clean. git show --remerge-diff shows no conflict markers. The only edits beyond the automatic merge are three:
- the first-send activator moved into
createExecutorSessionActivator, keeping the same adopt → commit → navigate → activate order; - the app-shell token count in
renderer-architecture.json; - the test that drives the activator.
Five files changed on both sides: bridge-contract.d.ts, preload.ts, app-shell.tsx, conversation/ports.ts and external-agent-settings/page.tsx. In each, every line from main (including #5927 composer Resume) and every line from the PR is still present. check-renderer-architecture.mjs --base 8ad836ce1 --strict-base passes. Resume interacts correctly with the executor sendBlocked: resumeShown already requires !sendBlocked && !sendPending.
91df4acf (first-send handoff): this looks correct. The handoff is keyed by the new Session id. A pending local Session skips Host inspection and polling until it is admitted. The picker and Send stay locked while the send is pending.
b51fd2e7: removing the waiver lines up with how the existing Session picker sends patches. A model choice sends {model}, a thinking level sends {model}, and a mode choice sends {mode}. With the coordinator now forwarding only {model} for a model-only patch, a mode the Agent changed as a side effect still persists.
Nit, not in the diff: the comment at composer-submit.ts:167-170 still says "the picker stays live throughout". executorComposerProps now disables the picker while sendPending is set.
Local tests:
acp-executor-plugin: 61/61session-catalog-coordinator: 86/86- desktop
executor-selection: 8/8 session-local: 30/30- ui
executor-model-picker: 29/29
The desktop and ui suites were bundled with esbuild. CI is green on this head.
| const option = acpOption(session.configOptions, key); | ||
| // Every supplied value is explicit, including saved task configuration. | ||
| // A model change cannot waive confirmation of a requested mode. | ||
| validate(session.configOptions, key, value); |
There was a problem hiding this comment.
P3 (latent; recovery gap). A requested mode can no longer be waived when a model change removes it, and nothing in the task lets the user get out of that state.
How to reach it:
- The draft picker carries the chosen mode across a model change (
executor-model-picker.tsx:335-336). select()checks that mode only against the probe's mode list, which covers just the probe's current model (use-executor-selection.ts:219-222).
So a {model, mode} pair the new model doesn't offer can still be submitted.
What happens next:
- The first prompt fails here with
acp_config_unavailable. executeloses the Session and setsrestoreFailed(lines 478-481).- Restore sends the same saved
{model, mode}again (use-executor-selection.ts:283,297) and fails the same way. - The model and mode controls require readiness
ready(executor-model-picker.tsx:209,ExecutorModeSelector), so the user can't drop the mode from inside the task.
Drift after an Agent change ends in the same state. The new test an unavailable saved mode rejects a followup after Agent model drift checks that the prompt is rejected, but nothing tests recovery.
The previous head covered the draft case. That test was renamed to ...removes the default mode, and its input changed to {model} (test line 1475/1492).
This can't happen with Antigravity 1.2.1: per the acceptance doc, all 11 models expose default/auto_edit/yolo. That is why this is P3. Any ACP Agent whose modes depend on the model would hit it.
Possible fixes: drop or revalidate the carried draft mode when the model changes, or let Restore fall back to model-only after acp_config_unavailable. Either way, add a test that covers recovery.
There was a problem hiding this comment.
Fixed in 3779a65.
Draft model changes now drop the carried mode, including thinking-level changes that select a different exact model ID. For an already failed Session, Restore submits model-only when inspection confirms that the saved mode is unavailable on the selected model. ACP restoration can then omit that unavailable saved mode and persist the Agent-confirmed configuration while retaining the same external Session.
Normal prompts and explicit mode requests still require exact confirmation; valid saved modes are still reasserted during restoration. Added regression coverage for a failed first prompt, removed/replaced modes after Agent drift, successful follow-up after recovery, and preserving the saved selection when capabilities are unknown. The final targeted run passed all 111 tests; the broader regression run passed all 922 tests.
| confirmed.previous?.mode === input.session?.executorConfig?.mode | ||
| ? confirmed.configuration | ||
| : { | ||
| ...(executorId && input.session?.model && input.session.model !== executorId |
There was a problem hiding this comment.
P3. When executorConfig is absent, the saved configuration now falls back to session.model. That's correct for the pending local summary, but it also applies to Host Sessions.
This affects Plugin executors that have no discovery or inspection:
- They get the fallback catalog entry with
models: [](plugin-executor-service.ts:380-387). - A Session created with
executorModel(for example from WorkHub defaults) storesmodel = executorModeland noexecutorConfig(session-catalog-coordinator.ts:1518-1524).
For those Sessions, selection.configuration.model is now set. executorSubmissionError (executor-submission.ts:77-79) treats an empty models array as truthy and returns invalid. canSubmitExecutor (app-shell-chat-actions.ts:286) then blocks every follow-up with that toast, and the picker shows the invalid notice. Before this commit the configuration stayed empty and the send went through.
ACP isn't affected, because it always persists executorConfig.
Suggested fix: limit the fallback to localState === 'pending', or apply it only when the inspected entry actually has models.
There was a problem hiding this comment.
Fixed in 3779a65. The session.model fallback is now limited to sessionPending, so it only supplies the model for a locally pending Session. A Host Session without executorConfig no longer gets an inferred model constraint from that field.
Added a regression test covering the pending-to-Host transition for a plugin with models: []: the pending summary retains its model, and follow-up submission succeeds once the Host Session is available. The test also verifies that an explicitly saved, unavailable model is still rejected.
| className="maka-executor-picker-entry maka-executor-picker-manage" | ||
| onClick={openSetup} | ||
| /> | ||
| {(props.error || browsed?.readiness === 'unavailable') && ( |
There was a problem hiding this comment.
P3. Retry now appears only when there's an error or the browsed entry is unavailable. A ready catalog (60 s TTL) or an authentication_required catalog can no longer be force-refreshed from the picker. Two cases where that matters: the user signed in outside Maka, or the Agent added models.
Step 3 of the acceptance runbook (docs/archive/antigravity-acp-pr4-acceptance.md:239) says to refresh a ready catalog with the UI refresh/retry control. That's no longer possible from the picker. Please either keep Retry for those states or update the runbook.
There was a problem hiding this comment.
Fixed in 3779a65. Retry is available in the picker for ready and authentication_required catalogs again, as well as unavailable/error states. It uses the existing force-refresh callback and stays disabled while loading, so the acceptance runbook's refresh step remains supported.
Added picker tests for all three readiness states, verifying that Retry invokes the refresh callback without changing the selection and is disabled during loading.
…-side apache#5826 locked the executor picker and send while a send's Host admission is pending by narrowing the shell's executor props in the Composer region. On this branch the region computes those props itself, so it passes `sendPending: submission.newTaskSendPending` to `executorComposerProps` (which apache#5826 taught to honor it) and the shell cannot pass it. apache#5826's owner tests now select an executor through the region's gate inputs. `createExecutorSessionActivator` joins the explicit Conversation exports (AppShell uses it); `executorSubmissionError` and `executorComposerProps` are test-only and come from `testing.ts`. The `useShellChatModel` row lists the first-send activation among its consumers. The ledger is main's copy with this branch's retired legacy paths and ownership entry removed, regenerated. Generated-by: Claude Opus 5.5
Summary
Add optional opaque ACP modes across executor catalogs, Host and Desktop configuration, and retained Session continuity. Draft discovery uses a stable empty Plugin-storage scratch directory scoped to the Host data root, Plugin namespace, adapter and composition Entry, with one 60-second candidate cache per configured ACP executor shared across projects. Native file leases serialize directory owners across runtime instances/processes and cover process and directory cleanup. Refresh and setup invalidate the instance cache; pending waiters follow the current revision even without a replacement query, while genuine failures remain terminal. The ACP runtime shares a two-probe admission budget across adapters.
Draft candidates are advisory: the real task Session initializes in its project, selects the model before validating dependent modes, and rejects unavailable explicit values before prompting. Confirmed task configuration, rollback and restart continuity remain separate from observed Agent updates and the draft cache. The configured Agent remains trusted and may retain empty native Session history.
On macOS, existing-program selection accepts the executable or its extracted directory. Selection and Host check/login share validation of the executable and sibling helper before saving. Missing or non-executable files produce localized errors without replacing the saved path. Protocol history describes the mode/refresh wire change once at epoch 204.
Refs #5103
Verification, 2026-10-02
P3 fixes:
4fefdc1e7; acceptance record:4c12e1935. The 97 ACP/Antigravity and 189 affected Runtime/Host tests passed (286 total, no skips or failures). They cover stable paths across runtime replacement, public Plugin storage namespace propagation, scope/key separation, cleanup after failure, native leases across instances/processes and owner termination, invalidation without refresh, multiple waiters, repeated invalidations and cancellation/disposal. Removing either fix makes its corresponding regression fail. Full workspace build, typecheck, lint, format, renderer architecture, locales, ASF headers, Windows skip inventory and diff checks passed locally; full workspace unit/e2e tests were not repeated locally for this follow-up. New-head CI was still running when this record was updated.Official Agent 1.2.1 was rechecked at implementation commit
4fefdc1e7: three actual session/new requests (initial discovery, refresh, executor/runtime recreation) reused the same empty neutral cwd and completed cleanup, with both toy projects unchanged. The actual production Plugin bundles also passed A/B shared-cache and forced-refresh checks with one stable leased path. These were real Agent runs without protocol fixtures and sent no task prompt. They do not inspect/delete Agent-private history or claim fresh Desktop steps 5–9.P2 isolation/lifecycle fix:
cc9c01224. The ACP suite passed 84 tests, the Antigravity adapter passed 9, and the affected Runtime, Host, Desktop and UI suites passed 225 without skips or failures. Regressions verify that session/new startup writes cannot pollute the selected project, refresh waiters receive the replacement result, and process cleanup retains admission, including after a crash. The project-mutation and overlapping-cleanup cases fail when the respective fixes are removed.P3 follow-up:
8ef010563. Filesystem selection, settings/IPC, setup/install and protocol suites passed 147 tests without skips or failures. They cover file/directory selection, incomplete distributions, executable permissions, cancellation, and actionable errors preserving settings. These suites overlap the preceding affected-suite set; counts are not additive.CI follow-up
82d6b12b6synchronizes the generated Windows test skip inventory for the new POSIX permission cases. The initial follow-up run stopped at the stale-inventory gate; the updated inventory gate and the entire rerun now pass. Missing-file and file-type cases continue to run on Windows.The final full workspace build, typecheck, lint, format, Desktop/UI Knip, renderer architecture, locale hygiene, ASF headers and diff checks passed locally. The protocol guard passes against fetched main
ab6db58c9(202 → 204). Hosted CI passed on preceding head82d6b12b6, including workspace and Runtime Host tests, Desktop e2e, Storybook smoke, transcript geometry and installed CLI release-candidate validation. The full workspace test run recorded on the previous head remains historical evidence; only the affected suites above were rerun for these follow-ups.Official Agent 1.2.1 and real macOS Desktop acceptance completed on the earlier build
ccd3c195e: both toy project prompts returned ACK with confirmed model/mode and cwd, idle configuration changes succeeded, and two normal restarts/restorations retained the same external Session and recalled synthetic context. This remains evidence for that build. Its workspace-scoped catalog cache checks do not verify the new neutral probe or native file/directory picker. The real macOS picker and Desktop discovery/refresh steps 1–4 were subsequently verified on82d6b12b6, including incomplete distributions preserving the saved path. The stable-directory official-Agent checks above cover4fefdc1e7; same-profile Desktop execution/recovery steps 5–9 remain historical evidence.Acceptance procedure
The acceptance record and repeatable procedure distinguish controlled tests from official-Agent/Desktop runs. Current draft acceptance requires one shared candidate snapshot across A/B, instance-wide refresh, and session/new containing only stable neutral scratch paths. Native selection acceptance includes incomplete distributions preserving the prior saved path.
Acceptance
4fefdc1e7; native picker and Desktop steps 1–4 passed on preceding head82d6b12b6.AI use
Tool(s) and scope: Codex implemented code, tests and acceptance records. Substantive commits include
Generated-by: Codex; human review and merge remain required.Checklist
82d6b12b6; new-head CI is pendingDoes this PR entail a change in behavior?