Sign up
View stargazersSign up

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

open8
main
GitHub fork: t3code/acp-per-model-options
opened16d agobyWWout Stiens
10 files changed
View on GitHubTwo-way sync enabledPR last imported 3d ago

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.

Checks

22 successful · 7 skipped
All checks passed
4036991

Added 1 commit

  • fix(acp): registry models keep their own option listsafea60c
MmacroscopeappBot16d ago
Synced from GitHub

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.

CcoderabbitaiBot16d ago
Synced from GitHub

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 bf8ffe81a63c82d3de5fbaa9991fd5f30034a452 and afea60c0aadf901d2e84f5856a6c08e0fec12c11.

📒 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)

mermaid
1sequenceDiagram
2 participant ACPRegistryRuntimeCoordinator
3 participant recordAcpRegistryModelOptions
4 participant Watcher
5 participant applyAcpRegistryLiveConfiguration
6 ACPRegistryRuntimeCoordinator->>recordAcpRegistryModelOptions: Record options for the current model
7 recordAcpRegistryModelOptions-->>ACPRegistryRuntimeCoordinator: Return accumulated model options
8 ACPRegistryRuntimeCoordinator->>Watcher: Notify provider instance
9 Watcher->>ACPRegistryRuntimeCoordinator: Read current stored state
10 ACPRegistryRuntimeCoordinator-->>Watcher: Return latest configuration and model options
11 Watcher->>applyAcpRegistryLiveConfiguration: Apply live state to provider models

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 nameStatusExplanation
Linked Issues checkPassedThe 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 checkPassedThe 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 checkPassedThe title clearly and concisely describes the primary change: keeping ACP Registry model option lists separate.
Description checkPassedThe 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.

Added 1 commit

  • Merge origin/main into t3code/acp-per-model-options4fa4787
JJulius Marminge11d ago

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.

Added 6 commits

  • test(acp): focus per-model option regression coveragee1c87df
  • fix(acp): refresh model options without losing observationse428219
  • fix(acp): retain model options across snapshot refreshes538801f
  • fix(acp): preserve probe cache on ordinary refresh64d724c
  • fix(acp): retain pending probes during cache refreshc305685
  • fix(acp): hydrate cached models with empty option descriptors4036991