feat(acp): emit config option categories and filter model catalog to active providers - #3922
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d47dc8e7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ff4fd4b to
884e269
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 884e26985c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46723c82ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES
Blocker 1 — CHANGELOG regression: this PR silently deletes the unrelated #3859 fix entry under ## [Unreleased]. The removed line documented that "Always-apply and rulebook rules are injected on the default system prompt path again. Discovery still loaded .gjc/rules/, ~/.gjc/agent/rules/, and sticky RULES.md, but only custom-system-prompt.md rendered them, so normal sessions silently dropped the content while AGENTS.md in the same directory continued to work (#3859)." That fix is still live in the codebase (see packages/coding-agent/src/discovery/builtin.ts, packages/coding-agent/src/sdk/session.ts, packages/coding-agent/src/system-prompt.ts), so its changelog entry must be restored. Removing it breaks attribution and release-note coverage for an already-shipped fix.
Non-blocker / cleanup: packages/coding-agent/src/modes/acp/acp-agent.ts around line 1915 comment says "Resolve usable providers in parallel", but collectActiveProviderIds is awaited after the Promise.all([config, modelCatalog]), so it actually runs sequentially. Correct the comment to match the code (or parallelize the call if safe).
Verified:
- Head
31d356521a6d7519890f92a95f1cceb330b1def3matches expected hold-lift SHA. bun test packages/coding-agent/test/acp-startup-options.test.ts packages/coding-agent/test/acp-initialize-conformance.test.ts packages/coding-agent/test/acp-builtins.test.ts packages/coding-agent/test/acp-client-bridge.test.ts packages/coding-agent/test/acp/acp-fallback-cancel-completion.test.ts packages/coding-agent/test/sdk-acp-prompt-terminal.test.ts→ 119 pass, 0 fail.bun --cwd=packages/coding-agent run check→ biome + tsc clean.bun run generate-docs-index→ no diff; generated index in sync.- Codex review comments are placeholder-only (no actionable suggestions) and not blockers.
- Category emission, active-provider filtering, pagination, fail-open scope, and fixture updates all review clean.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Mode, Model, and Thinking session config options now carry the ACP spec-defined `category` field (`mode`, `model`, `thought_level`). Standards-compliant ACP clients such as Paseo rely on this field to discover models, modes, and thinking levels without provider-specific metadata; without it the model picker comes back empty.
…t/active The ACP session model catalog (models.list/current) returns every known model, including providers without usable credentials. Session state now queries providers.list/active (Q29, GJC >= 0.12.8) in parallel and filters the catalog to providers with credential or credentialless connection kinds, mirroring the TUI model picker. An older session host that rejects the query falls back to the full catalog so model discovery never regresses. Documents the Paseo generic-ACP provider setup in external-control-readiness.md.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
MERGE_READY
Re-reviewed at new head 98ccd35b1a1dfb10335816141a5e2a92f1c405e8.
Previous blocker cleared: the #3859 CHANGELOG entry under ## [Unreleased] is restored and no longer deleted.
No new blockers or regressions found:
- ACP config option categories are correctly emitted for
mode,model, andthought_level. - Model catalog filtering via
providers.list/activecorrectly keeps credential/credentialless providers, falls back to the full catalog on first-pageoperation_not_session_owned/invalid_request(pre-Q29 hosts), and fails closed on operational errors/timeouts. - Pagination collectors handle
completeflags and continuation cursors with page budgets, and the model catalog is fully collected before filtering so later-page models are not dropped. - Current model is retained even if its provider is not in the active set.
- No provider credentials or private ids leak into the ACP session-state surface; only provider/id/name tuples are exposed as model options.
- Tests cover category emission, active-provider filtering, pagination, and fail-open behavior.
- Docs/index surfaces are consistent with the Paseo readiness notes.
Local verification on this exact head already completed in the review lane (focused ACP suite + coding-agent check + docs-index sync).
CI check-runs on this exact head are complete/success (or intentional skips). mergeable_state=clean.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
Post-merge contract verification — PASSED Terminal verdict: MERGE_READY (verified post-merge; no outstanding REQUEST_CHANGES items). Verified against the exact merged head
No mutation performed: exact-head verification only; no merges, reverts, or branch changes. — |
…-path test The 0.12.13 release CI failed: collectModelCatalog and collectActiveProviderIds (added in #3922) now require every query page to carry either complete:true or a continuationCursor string, but the production-path test's generic query_request mock returned bare {page:{items}} with neither. #sessionState's paginated resumeSession path threw AcpSdkAdapterError("models.list/current page is incomplete without a continuation cursor.") instead of exercising the real single-page contract. Mark the mock's single-page responses complete:true, matching how a real session host answers a query whose full snapshot fits in one page. Lore-id: relfix0.12.14 Constraint: v0.12.13 tag is already pushed and immutable; this fix ships in 0.12.14 Rejected: weakening collectModelCatalog's completeness check | would silently truncate real large model catalogs again Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test packages/coding-agent/test/sdk-acp-production-path.test.ts (2 pass); full ACP suite (208 pass)
…-path test The 0.12.13 release CI failed: collectModelCatalog and collectActiveProviderIds (added in #3922) now require every query page to carry either complete:true or a continuationCursor string, but the production-path test's generic query_request mock returned bare {page:{items}} with neither. #sessionState's paginated resumeSession path threw AcpSdkAdapterError("models.list/current page is incomplete without a continuation cursor.") instead of exercising the real single-page contract. Mark the mock's single-page responses complete:true, matching how a real session host answers a query whose full snapshot fits in one page. Lore-id: relfix0.12.14 Constraint: v0.12.13 tag is already pushed and immutable; this fix ships in 0.12.14 Rejected: weakening collectModelCatalog's completeness check | would silently truncate real large model catalogs again Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test packages/coding-agent/test/sdk-acp-production-path.test.ts (2 pass); full ACP suite (208 pass)
2 participants
What
Two coupled changes that make GJC's ACP session model catalog usable by standards-compliant ACP clients (e.g. Paseo):
categoryfield (mode,model,thought_level). Clients rely on this field to discover models, modes, and thinking levels; without it the model picker comes back empty.models.list/current, which returns every known model) is now filtered to active providers viaproviders.list/active(Q29), mirroring the TUI model picker'smodelRegistry.getAvailable(). All Q29 pages are consumed before the provider set is built.Why
categoryfield, generic ACP providers (Paseo registers GJC viaextends: "acp") discover zero models even though GJC returns the full catalog.Related: #3694 (Q29
providers.list/active) is the merged SDK query this PR consumes. No existing PR/issue covers the ACP side; this is the consumer layer that was missing.Review feedback (addressed)
() => undefinedrejection handler now falls back to the full catalog only foroperation_not_session_owned(older host without Q29); timeouts, resolver failures, and malformed pages fail closed.collectActiveProviderIdsnow followscontinuationCursoruntilpage.complete, instead of reading only the first page.collectModelCatalognow follows the Q10continuationCursorto completion before the active-provider filter is applied, so models on later catalog pages are never dropped.invalid_request, notoperation_not_session_owned; both now take the full-catalog fallback on the first page, while later-page and operational failures fail closed.acp-fallback-cancel-completionandsdk-acp-prompt-terminalmocks now answer completed Q10 and Q29 pages.packages/coding-agent/CHANGELOG.mdUnreleased/Fixed (feat(acp): emit config option categories and filter model catalog to active providers #3922).Scope
What this PR enables through a standards-compliant ACP client (e.g. Paseo), and what intentionally stays on GJC's own surfaces.
Available (once this change ships in a released GJC):
extends: "acp"provider entry in~/.paseo/config.json; no client-side code changes.providers.list/active) and fully paginated (Q10 + Q29), with a full-catalog compatibility fallback for session hosts that predate Q29.Not covered (GJC-specific surfaces, intentionally outside the ACP wire):
workflow.gate_answer/ Coordinator MCP), so a plain ACP client withoutelicitation.formleaves the gate waiting.turn.steer,model.set, thinking / queue / retry / compaction, context / transcript queries): these live on the SDK WebSocket surface, not ACP.gjc/gjc --tmux) sessions: ACPsession/listlists broker-managed sessions only; interactive sessions host their own SDK endpoint and are not broker-registered.Testing
Fresh upstream
devcheckout,bun install+ natives build, macOS darwin-arm64:bun test packages/coding-agent/test/acp-startup-options.test.ts packages/coding-agent/test/acp-initialize-conformance.test.ts packages/coding-agent/test/acp-builtins.test.ts packages/coding-agent/test/acp-client-bridge.test.ts→ 134 pass, 0 fail (includes new pagination and fail-open-scope tests)tsc -p tsconfig.json --noEmit(packages/coding-agent) → cleanbiome checkon changed files → cleangit diff --check→ cleanbun scripts/check-public-version-sync.ts→ docs/site/version surfaces in sync (docs-index.generated.ts regenerated)gjcregistered asextends: "acp"provider; model list drops from 1125 to the active provider's models (opencode-go/deepseek-v4-flash+ thinking options);paseo run --provider gjc/opencode-go/deepseek-v4-flash "Reply with exactly: OK"completed with the expected answer.GJC verdict
The codex connector (
chatgpt-codex-connector) re-approved the exact current head98ccd35with THUMBS_UP, all 6 review threads resolved, and no CI failures. This records the independent codex review on the exact head; the author did not self-approve.Checklist
devbun check(check:tools + check:types on changed scope) passes