Skip to content

fix(acp): registry models keep their own option lists - #19575

Open
StiensWout wants to merge 8 commits into
pingdotgg:mainfrom
StiensWout:t3code/acp-per-model-options
Open

StiensWout wants to merge 8 commits into
pingdotgg:mainfrom
StiensWout:t3code/acp-per-model-options

Conversation

@StiensWout

@StiensWout StiensWout commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

ACP registry agents report options for the session’s current model. T3 copied those choices onto every advertised model and could restore them from saved snapshots after refresh.

Keep probe and live observations by model ID, with live observations taking precedence, including explicit empty lists. Unobserved/custom models use agent defaults. Sanitize saved ACP descriptors to an empty list so MCP rejects explicit options until the model is observed. Preserve current observations through partial snapshots and failed probes; ordinary status refreshes reuse the probe cache, while explicit model refreshes reprobe without discarding a pending same-identity probe.

Built on @StiensWout’s work in #19575, preserving the contributor commits. Fixes #15177. This stays focused on per-model options: #19567 handles selecting the first model-category field; #19754 depends on this behavior.

Verification:

  • 99/99 tests pass in four existing suites, with scoped lint, format and ACP registry/server typechecks. The original three suites went from 93 tests to 92 after trimming redundant coverage, then 93 with the missing lifecycle regression; the existing cache suite adds six. Red/green checks prove per-model isolation, retained observations, cache reuse, pending-probe refresh and safe saved-cache hydration.
  • Five dual full-diff review rounds; the latest round found no actionable issues at 403699172b. Confirmed refresh, capability-retention and MCP hydration findings were fixed. Account teardown concerns were compared with main and are pre-existing.
  • Live isolated Linux server/web client with a deterministic ACP subprocess: complete turns, switch models, explicitly refresh status, observe a new discovery process, and switch back. Wide retains five thinking choices and narrow two, including while a discovery probe is pending. This verifies protocol and client paths with a fixture; vendor accounts were not tested.

Before the retention fix (e428219a14): pending refresh removes the wide model’s picker.

Before: pending refresh removes the wide model option picker

After (c30568509d), same thread and viewport: all five choices remain.

After: pending refresh preserves five wide model choices

Narrow model offers only low and high

The final cache-hydration correction (403699172b) changes saved descriptors from null to empty and has separate red/green coverage; it does not change the captured client rendering. Captures use the real client with a font fallback for headless Linux. macOS, Windows, iOS, Android and Electron desktop were not verified.

Implementation: Codex gpt-6.1-sol (high); tests: Codex gpt-6-astra (high); adversarial reviews: Claude claude-opus-5-5 and Codex gpt-6-sol, through the T3 Code harness.

ACP session config options describe only the session's current model, but
the registry snapshot copied one session's options onto every model. The
newest live session then overwrote all models until the instance restarted.

Each probe and live session now records its options against its current
model. The driver keeps probe observations with its probe cache, the runtime
coordinator keeps live observations per instance, and both are cleared on
auth loss, logout, and disposal. The provider registry no longer copies an
older snapshot's options onto ACP Registry models, so polluted caches heal.

Fixes pingdotgg#15177

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 14, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 4036991

Macroscope's review found this PR approvable — This PR narrowly fixes ACP Registry model-option propagation so each model retains only options observed for that model, while sanitizing stale cached snapshots and covering probe/live/cache paths with tests. The changes do not introduce a new capability, schema change, deployment change, or analysis override.

You can add or adjust custom eligibility rules. Learn more.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 14, 2026
@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3bb94bee-0c8c-48bd-9223-e60a8e3281ba
📥 Commits

Reviewing files that changed from the base of the PR and between bf8ffe8 and afea60c.

📒 Files selected for processing: 8
  • apps/server/src/provider/ProviderRegistry.test.ts
  • apps/server/src/provider/ProviderRegistry.ts
  • docs/user/providers-acp.md
  • packages/provider-acp-registry/src/server/AcpRegistryRuntimeCoordinator.test.ts
  • packages/provider-acp-registry/src/server/AcpRegistryRuntimeCoordinator.ts
  • packages/provider-acp-registry/src/server/driver.test.ts
  • packages/provider-acp-registry/src/server/driver.ts
  • packages/provider-acp-registry/src/server/probe.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

📝 Walkthrough

ACP Registry model options are now recorded for the model observed in discovery or a live session. Provider snapshots apply the accumulated options to matching models. ACP Registry refreshes no longer restore cached capabilities when a refreshed model has empty options.

Changes: ACP Registry model-specific options

Layer / File(s) Summary
Record discovery options by model
packages/provider-acp-registry/src/server/probe.ts, packages/provider-acp-registry/src/server/driver.ts, packages/provider-acp-registry/src/server/driver.test.ts
The probe records configuration descriptors by model slug. Discovery assigns options only to the observed model; tests cover listed, unlisted, and custom models.
Accumulate and apply live-session options
packages/provider-acp-registry/src/server/AcpRegistryRuntimeCoordinator.ts, packages/provider-acp-registry/src/server/AcpRegistryRuntimeCoordinator.test.ts, packages/provider-acp-registry/src/server/driver.ts, packages/provider-acp-registry/src/server/driver.test.ts
The coordinator stores the latest configuration with accumulated per-model options. The driver applies this state to the latest model list, and tests cover retention across sessions and provider instances.
Apply probe options to refreshed snapshots
packages/provider-acp-registry/src/server/driver.ts, packages/provider-acp-registry/src/server/driver.test.ts, apps/server/src/provider/ProviderRegistry.ts, apps/server/src/provider/ProviderRegistry.test.ts, docs/user/providers-acp.md
Successful probes record options in the generation-scoped cache, which the driver applies to provider snapshots. ACP Registry merges do not copy prior model capabilities onto refreshed models. Tests and documentation describe the per-model options behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Low

Sequence Diagram(s)

sequenceDiagram
  participant ACPRegistryRuntimeCoordinator
  participant recordAcpRegistryModelOptions
  participant Watcher
  participant applyAcpRegistryLiveConfiguration
  ACPRegistryRuntimeCoordinator->>recordAcpRegistryModelOptions: Record options for the current model
  recordAcpRegistryModelOptions-->>ACPRegistryRuntimeCoordinator: Return accumulated model options
  ACPRegistryRuntimeCoordinator->>Watcher: Notify provider instance
  Watcher->>ACPRegistryRuntimeCoordinator: Read current stored state
  ACPRegistryRuntimeCoordinator-->>Watcher: Return latest configuration and model options
  Watcher->>applyAcpRegistryLiveConfiguration: Apply live state to provider models
Loading

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to afea6

No concrete issue has been established that should block merging; normal validation remains appropriate.

Security architecture risk: 🔵 Low · up to afea6

The change limits option sharing to the model that reported each option list and preserves existing checks on explicit selections. No introduced security defect was identified in the inspected paths. Concurrent refresh and live-session behavior was assessed statically, not verified against live providers.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Within the inspected observation path, ACP peer-supplied descriptors influence option reporting and selection for the peer's provider instance, including later sessions on previously observed models. Instance-keyed storage and matching-slug application bound this propagation; no cross-instance application was identified in this path.

Trust Boundaries and Controls

  • observed — For listed, unobserved ACP Registry models, descriptors are an empty array rather than absent. The existing MCP explicit-option validator therefore rejects requested option IDs instead of taking its undefined-descriptor skip branch.
  • observed — Downstream configuration still checks the live session after model selection: unsupported option IDs and unavailable select values are skipped, while the runtime validates advertised types and values before setting options. These controls predate this PR and remain unchanged; cached descriptors are not the sole check at application time.

Resilience and Maintainability Implications

  • observed — Live publication atomically updates latest configuration and accumulated options; watchers subscribe before reading and re-read stored state on notifications. Driver snapshot writes use a semaphore, while generation checks prevent invalidated probes from updating the observation cache or publishing enrichment.
  • observed — Observation cleanup is tied to provider-instance lifecycle and authentication changes, not individual session completion. Driver creation and finalization clear live state; authentication invalidation and successful logout clear live observations and invalidate probe observations.
Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed The changes satisfy #15177. modelsFromDiscovery starts listed models with empty capabilities and records discovery options only for the observed model. recordAcpRegistryModelOptions stores observa…
Out of Scope Changes check Passed The changed files support #15177. The coordinator state changes hold per-model observations. The probe and driver changes record and apply those observations. The ProviderRegistry change prevents stal…
Title check Passed The title clearly and concisely describes the primary change: keeping ACP Registry model option lists separate.
Description check Passed The description explains the problem, implementation, scope, related issue, and detailed verification results. It omits the template headings and does not state explicit maintainer approval, but it is…
Full details: Linked Issues check

Explanation

The changes satisfy #15177. modelsFromDiscovery starts listed models with empty capabilities and records discovery options only for the observed model. recordAcpRegistryModelOptions stores observations by model. AcpRegistryRuntimeCoordinator accumulates live observations per provider instance. applyAcpRegistryLiveConfiguration applies the latest observation for each model and does not copy options to unobserved models. ProviderRegistry preserves prior capabilities only for non-ACP Registry providers. Tests cover discovery, live sessions, refreshes, provider isolation, and cached-capability merging.

Full details: Out of Scope Changes check

Explanation

The changed files support #15177. The coordinator state changes hold per-model observations. The probe and driver changes record and apply those observations. The ProviderRegistry change prevents stale ACP Registry capabilities from crossing refreshes. The tests and documentation describe or verify the corrected model-specific behavior. No unrelated change is established.

Full details: Description check

Explanation

The description explains the problem, implementation, scope, related issue, and detailed verification results. It omits the template headings and does not state explicit maintainer approval, but it is mostly complete and focused.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one. I'll rebuild it to current main conventions, trim the tests to what proves the behavior, and push to this branch.

@macroscopeapp
macroscopeapp Bot dismissed their stale review October 19, 2026 03:08

Dismissing prior approval to re-evaluate 4036991

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

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: ACP registry shows the current model's options on every model

2 participants