Skip to content

fix(acp): persist model catalogs and scope discovery per agent - #6197

Open
Sun-GLiang wants to merge 11 commits into
apache:mainfrom
Sun-GLiang:codex/fix-acp-catalog-cache
Open

Sun-GLiang wants to merge 11 commits into
apache:mainfrom
Sun-GLiang:codex/fix-acp-catalog-cache

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Summary

Opening a new draft after more than 60 seconds or a Host/application restart previously launched another ACP discovery process. Successful neutral-discovery catalogs now persist across drafts and restarts.

  • Keep public catalogs separate from conversation-specific models, selections, and modes. Project A cannot overwrite project B's public choices; real sessions validate their own configuration.
  • Bind snapshots to configuration and executable/helper metadata. Version 2 rejects potentially session-contaminated version 1 snapshots. Every successful neutral probe saves current metadata, including when the model list is unchanged.
  • Scope discovery, loading, invalidation, and retry per agent. Transient refresh failures retain previous choices; cold failures have a 60-second retry cooldown. Explicit refresh and setup/login invalidation discover changes without a periodic timer.
  • Preserve serialized probes, invalidation fencing, writes, and cleanup ownership while removing redundant bookkeeping and persistence caches.
  • Synchronize the Astryx surface inventory for the model picker's Spinner.

Verification

  • After rebasing onto main (ff74d21c8), passed the protocol epoch guard, full build:test, and 235 focused catalog, executor, protocol, IPC, and model-picker regression tests.
  • Passed 98 catalog/store/executor tests and 5 selected real stdio/process fixture tests.
  • Passed ACP package build, ACP/Antigravity adapter typechecks, repository-wide lint, format, and ASF header checks.
  • Reproduced the CI failure from the stale Astryx inventory; regenerated it and passed the inventory check plus 23 related tests.
  • The earlier isolation fix also passed full workspace typecheck and 44 selected Desktop/runtime/picker tests; those were not rerun during ablation.
  • Full workspace suites and official-binary acceptance were not rerun. An earlier full ACP attempt stalled at a real stdio checkpoint test and was stopped. Historical official Antigravity inference returned a location-eligibility HTTP 403, so full question/answer execution remains unverified.

Compatibility

Runtime Host compatibility epoch increases from 226 to 227 for executor inventory/filter queries; update Desktop and Host together. Missing, corrupt, or incompatible snapshots fall back to neutral discovery.

AI use

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

OpenAI Codex implemented the changes, regression tests, documentation, and local verification. Follow-up commits include a Generated-by: Codex trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and affected tests pass within the verification scope above

Does this PR entail a change in behavior?

  • Yes — described above
  • No

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Oct 20, 2026
@Sun-GLiang
Sun-GLiang force-pushed the codex/fix-acp-catalog-cache branch from 6754a58 to e16eb1c Compare October 21, 2026 03:05
@Sun-GLiang
Sun-GLiang marked this pull request as ready for review October 21, 2026 05:58

@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.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

I reviewed head e16eb1c9c536088aa896e8ca38d97b3b103f5525. This change keeps ACP model discovery in a neutral session, persists the resulting catalog with executable/configuration fingerprints, and replaces repeated discovery with explicit per-agent refresh and process-free executor inventory.

P2 — Advance the compatibility epoch beyond current main. packages/runtime-host/src/protocol/index.ts:111 still uses epoch 226, which current main (85e942724c02e7149c4a56f44a0ad2f1350563be) already uses. The new executorId and listOnly catalog-query fields are observable protocol changes: the actual main decoder accepts the legacy query but rejects either new field, while this head accepts both. Both peers would advertise epoch 226. The protocol guard against current main also fails. Rebase, resolve the conflicts, and choose the next unused epoch (currently 227 or later); these strict input changes cannot be justified as wire-invisible.

The catalog behavior passed an independent probe using actual ACP stdio subprocesses and separate Node processes sharing a disk-backed cache. Two project sessions advertised different private models and sent later configuration updates; neither contaminated the public neutral catalog. An aged successful cache entry and a newly constructed executor reused the catalog without another agent process. Explicit refresh observed a changed upstream model, persisted it for the next process, and invalidation forced discovery again. Substituting the previous implementation caused the aged-cache process-count assertion to fail, as expected. This probe uses the real executor/agent transport with a file-backed storage seam, rather than the complete Host storage service.

Validation passed: a fresh dependency install and normal root build:test; 249 focused tests; five additional real-process discovery/cleanup tests; Desktop, ACP and Antigravity type checks; the normal renderer build; 168 architecture tests; surface inventory, license headers, locale hygiene, Windows test inventory and formatting checks. I also inspected persistence revision fencing, retained-session cleanup, Host query routing, and picker loading/retry state. I found no additional confirmed P0–P2 runtime defect in those checks.

Merge status: GitHub reports successful checks for this head, but the local protocol guard fails against current main. A trial merge also conflicts in packages/runtime-host/src/protocol/index.ts and apps/desktop/src/main/__tests__/runtime-host-session-catalog-ipc-main.test.ts; I have not built a resolved merge. I did not validate native Electron interaction, authenticated external ACP agents, or external account/model changes without an explicit refresh. This is a feature change requiring maintainer judgment after these integration issues are resolved.

// interoperability. Mismatches are rejected before domain commands are admitted.
// Re-check against main before merging; sibling PRs may use the same next epoch.
export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 225 as const;
export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 226 as const;

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.

[P2] Allocate a fresh compatibility epoch after rebasing onto main

Current main already advertises epoch 226. This PR adds executorId and listOnly to plugin.executor.query, but main's strict decoder rejects either field even though the compatibility epoch matches. I verified that the legacy query succeeds with both decoders and that only the new head accepts the two new fields. The protocol guard against current main fails as well. Rebase and use the next unused epoch (currently at least 227) so incompatible peers are rejected during admission.

@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.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Second, independent review of e16eb1c9, written before reading the review above. It agrees on the epoch P2 and the merge conflicts. Severity note: the stale-catalog-after-Maka-update finding below was raised only in this review and has not been reproduced. The stored record already carries a manual version field (record.version !== 2 in acp-catalog-store.ts) that can be bumped when the catalog derivation changes, and users can refresh manually, so treat it as P3 (a maintenance hazard worth addressing, e.g. by bumping version whenever adapter grouping/labels change or by folding a plugin build id into the fingerprint), not a merge blocker.


First review of head e16eb1c9c536088aa896e8ca38d97b3b103f5525 (merge-base b0ed5707, 28 files, +2235/-133). I read every non-test source change: the ACP catalog store and catalog lifecycle, the executor probe/dispose changes, the per-agent executorId/listOnly query from the protocol through the Host, Desktop IPC/preload and renderer hook, and the model picker. There are no prior findings to follow up. I did not run tests locally because memory is limited on the review machine. CI on this exact head is green (test, label).

What looks right:

  • Scoping. The snapshot key is acp-catalog/<adapter.id>/<sha256(entryId)>/snapshot inside the plugin's namespaced storage, so agents and entries cannot see each other's catalogs. invalidateCatalog(executorId) now only invalidates the named agent (Antigravity on setup success). Per-agent renderer loads use [key, attempt, id] operation keys and are guarded by the attempt revision, so a stale response is discarded and cannot land in another draft.
  • Invalidation and concurrency. The binding covers executor id, config, launch spec, platform/arch, and realpath/size/mtime/ctime of the executable and helper. That covers a changed executable path (reconcile installs a new entry, so the fingerprint mismatches) and the managed installer's versioned directory (setup onSucceeded invalidates as well). Hydration is fenced by revision. Writes are serialized, so an old save cannot bring back a deleted snapshot. Dropping the blanket invalidation in applyRuntimePolicyMutationEffects looks safe, because the only policy input to the agent is executable, which reconcile turns into a new entry.
  • Failure and offline behavior. authentication_required clears both the memory and the persisted catalog. Transient failures keep the last good list, mark it refreshFailed and apply a 60-second cooldown. If the agent is uninstalled, the realpath/X_OK check makes load() fail, so it falls back to fresh discovery. Version-1 snapshots are rejected.

Findings:

P2: Protocol epoch collision. The PR sets RUNTIME_HOST_COMPATIBILITY_EPOCH to 226, but main is already at 226 (#6176, assistant.allocate-workspace). After rebase the PR would ship a wire change (executorId/listOnly on the executor catalog query) under the same epoch as main, and the protocol guard would fail. Please bump to the next unused value (227 unless another PR lands first). Also add the // 227: ... history comment, which the PR currently omits.

P3 (downgraded, see note above): The persisted snapshot is not tied to the adapter/plugin code that produced it. createAcpCatalogStore stores the fully derived ExecutorCatalogEntry, which includes displayName and the output of adapter.describeModels (Antigravity modelGroups/providerType). The binding (acp-catalog-store.ts:43) only covers config, launch and binary metadata. The in-memory TTL is gone and hydration survives restarts, so after a Maka update that changes describeAntigravityModels, the display name, or catalog normalization, users keep seeing the old derived catalog indefinitely. It only clears if they press Refresh or the agent binary changes. Suggested fixes: persist the raw model/mode option lists and re-derive with #catalogEntry on load, or include a plugin/adapter build identity in the binding (for example, the package content digest or an explicit derivation version).

P3: The snapshot is orphaned when the agent is removed. Uninstalling the Antigravity package (clearing its executable) does not delete acp-catalog/.../snapshot, just like the existing continuity records. The binding makes it harmless on reuse, but it is never cleaned up.

P3: Neither the catalog nor the snapshot binding notices an external account change (sign out or account switch done outside Maka). The persisted list stays until the first real Session reports authentication failure or the user refreshes. This is acceptable as a documented trade-off, but worth a note in the README section the PR edits.

P3: In a draft, any change to executorId or key re-runs the effect. That re-fetches the roster and marks every agent as loadingIds, so each picker briefly shows "Loading models..." even when every catalog is already warm. This is cosmetic only.

Mergeability: the branch does not merge cleanly into current main (d2818638). git merge-tree reports content conflicts in packages/runtime-host/src/protocol/index.ts (the epoch line, see above) and in apps/desktop/src/main/__tests__/runtime-host-session-catalog-ipc-main.test.ts. The test conflict is trivial: both sides append tests, so keep both. GitHub reports CONFLICTING/DIRTY. A rebase is required.

CI: test and label pass on e16eb1c9. Nothing in CI is caused by this PR.

Verdict: needs a rebase with the epoch moved to 227, and the persisted catalog should be tied to the code that derives it. The rest of the persistence, scoping and failure-handling design looks correct.

// interoperability. Mismatches are rejected before domain commands are admitted.
// Re-check against main before merging; sibling PRs may use the same next epoch.
export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 225 as const;
export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 226 as const;

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.

P2: main is already at epoch 226 (#6176). This wire change needs the next unused epoch (227 unless another PR lands first) plus a // 227: ... history comment. Otherwise the protocol guard fails after rebase.

// Persist only a digest, never configuration values or credentials. Do not
// fingerprint the inherited process environment: it includes per-launch
// values and would make an otherwise unchanged restart miss the cache.
const configuration = JSON.stringify(

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: The binding covers config, launch and binary metadata but not the adapter/plugin code. entry stores derived data (displayName and the describeModels output: modelGroups/providerType). With no TTL and hydration across restarts, a Maka update that changes that derivation keeps serving the stale catalog until a manual refresh. Suggest persisting the raw model/mode options and re-deriving on load, or adding a plugin build/derivation version to the binding.

Reuse successful catalogs across drafts and Host restarts, update candidates from real sessions, and isolate invalidation, loading and retry state by agent. Keep stale choices on transient refresh failure and retain probe ownership until cleanup succeeds.

Generated-by: Codex
Merge persisted neutral defaults with early Session observations before saving, including during shutdown. Add ready-agent refresh controls and route inventory errors through inventory retry. Cover hydration races and picker recovery with regression tests, refresh documentation and Windows inventory, and record browser verification.

Generated-by: Codex
Keep ready executor selections visible during background polling and session updates while preserving loading feedback for initial reads and explicit retries. Cover silent polling, model updates, and failure recovery.

Generated-by: Codex
Hide draft refresh controls while a native model is selected and expose a single action for the selected external agent. Update picker coverage and catalog documentation.

Generated-by: Codex
Keep public model catalogs sourced exclusively from neutral discovery so project-specific Session candidates cannot hide models in other projects. Remove the Session-to-catalog write path and reject version 1 snapshots that may contain project-specific candidates.

Cover cold and warm cross-project discovery, Session updates, restart reuse, explicit refresh, and snapshot compatibility. Update documentation to describe the plugin-local boundary.

Generated-by: Codex
Remove redundant probe bookkeeping and persistence caches while retaining invalidation fences and serialized writes. Cover executable updates and shutdown races, and align documentation with neutral-only snapshots and measured ablation results.

Generated-by: Codex
Regenerate the Astryx inventory to include the model picker Spinner and restore the CI inventory check.

Generated-by: Codex
Keep current catalog behavior in the plugin README and architecture document; remove historical screenshots and duplicate experiment records.

Generated-by: Codex
@Sun-GLiang
Sun-GLiang force-pushed the codex/fix-acp-catalog-cache branch from e16eb1c to 1105d8d Compare October 21, 2026 08:16
Centralize the snapshot version and document when adapter presentation and catalog semantics require invalidation. Cover one-time rediscovery and restart reuse across a version change with unchanged config and executable metadata.

Generated-by: Codex
Show only the selected model or model family in the composer trigger. Keep agent names in the dropdown section headings and agent-specific actions.

Generated-by: Codex

@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.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review at 6baf1045b2c983cf1520316d64b76a994d5143cf, following e16eb1c9c536088aa896e8ca38d97b3b103f5525. The previous protocol P2 and merge conflicts are resolved. One new P2 remains: the model-label change breaks two required Storybook play assertions.

I compared the author commits across the rebase, checked the protocol and IPC conflict resolutions, and read the two additional commits separately from main's changes. Those commits make the catalog snapshot version's maintenance contract explicit and remove agent prefixes from the selected model label. The catalog still uses version 2; the new constant documents when maintainers must bump it rather than automatically detecting code changes.

P2 — update the Storybook assertions for the new model label. packages/ui/src/composer-model-picker.tsx:246 deliberately stops displaying the agent prefix, but packages/ui/stories/model-picker.stories.tsx:170 and :189 still expect it. I rebuilt Storybook and ran both unmodified stories in Chromium. product-model-picker--codex-and-acp fails expecting Codex · GPT-6 Astra while the actual trigger reads GPT-6 Astra; product-model-picker--unselected-unavailable-agent-narrow fails expecting Antigravity · Gemini 3.8 Flash while the actual trigger reads Gemini 3.8 Flash. An adjacent unchanged executor story passes. These are the same two failures in the exact-head CI run. The story file is byte-identical to current main, whose CI passed. Align both assertions with the intended label while retaining the agent-group and notice checks.

The protocol now advances from main's 226 to 227, and the epoch guard passes. Actual decoder checks confirm that this head accepts the new catalog fields that main rejects; peers now advertise different epochs. A static merge with main 9826db1b9dad32897be105b2c1c621becedb27ce is clean. I did not separately build the merged tree.

P3 — coordinate epoch 227 before merging. At this check, open PRs #6091 (efbe29e6), #6192 (07c7a8bb) and this PR all declare 227. The first merged protocol change consumes that epoch. The remaining PRs must choose the next unused epoch and be reviewed again; they must not all land unchanged under 227.

Validation otherwise passed: 278 focused tests, the normal clean root test build, all five Desktop TypeScript configurations, Storybook and production renderer builds, 168 architecture-checker tests plus the strict application check, Biome on 25 changed code files, and git diff --check.

Additional real-subprocess checks with disk-backed storage verified neutral catalog reuse across restarts, explicit refresh and invalidation, and isolation of project-specific models from the public snapshot. For the version contract, an actual earlier implementation wrote a v1 snapshot. The current implementation re-probed once and wrote v2 with the same executable/configuration binding; another restart reused it. Removing only the version check reproduced stale catalog reuse. This verifies the version mechanism, not automatic detection of every adapter code change.

The independent persistence probe uses a file-backed storage seam rather than a full Host database. Native Electron, Windows/macOS, authenticated external agents and the complete repository suite were not exercised. The Storybook CI failure above remains unresolved.

const selectedModel = executor?.selection?.configuration.model ?? selected?.currentModel;
const group = selected && executorModelGroup(selected, selectedModel);
const modelLabel = contributed ? contributedModel?.label ?? props.executorTarget?.model ?? copy.default : executor?.selection
const label = contributed ? contributedModel?.label ?? props.executorTarget?.model ?? copy.default : executor?.selection

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.

P2: removing the agent prefix here breaks the required Storybook smoke run because two existing play assertions still require that prefix. The unmodified product-model-picker--codex-and-acp story expects Codex · GPT-6 Astra but receives GPT-6 Astra; product-model-picker--unselected-unavailable-agent-narrow expects Antigravity · Gemini 3.8 Flash but receives Gemini 3.8 Flash. Both fail in a fresh local Chromium build and CI run 39569703472. Update packages/ui/stories/model-picker.stories.tsx:170,189 to the intentional label behavior while preserving its agent-group/notice coverage.

Match selected model labels exactly without agent prefixes while retaining agent grouping and unavailable-agent notice coverage.

Generated-by: Codex

@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.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review of 6baf1045..0bcb095a. The ten existing commits are unchanged (git range-diff), and one test-only commit was added (model-picker.stories.tsx, 2 lines). Our P2 is fixed; no P0–P2 remains.

Both Storybook play assertions that still expected the old agent-prefixed trigger labels now match the model-only label exactly: /^GPT-6 Astra$/ in CodexAndAcp, and /^Gemini 3\.8 Flash$/ in UnselectedUnavailableAgentNarrow. Anchoring the regex also proves the prefix is gone, so this is a stronger check than the old substring. Agent grouping and the unavailable-agent notice assertions are unchanged. CI test now passes on this head, and the PR is mergeable.

The P3 from our last round still stands. This PR, #6091 and #6192 all set RUNTIME_HOST_COMPATIBILITY_EPOCH = 227, while main is at 226. Whichever merges first keeps 227; the others need the next free epoch and a short re-review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants