feat(sdk): expose model profiles as synthetic gajae-code/<profile> models - #3988
Conversation
04f07e3 to
0dcd3e9
Compare
![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: 04f07e3141
ℹ️ 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".
0dcd3e9 to
2ce26fd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ce26fdb2b
ℹ️ 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".
2ce26fd to
4c870c8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c870c84de
ℹ️ 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".
2f2e5df to
db70f31
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Checked out preset-to-model at 2f2e5df70 and ran it locally. The Q10 projection, the reserved-namespace fail-closed guard, and the current-row uniqueness invariant all look well built, and 795 lines of tests for ~1100 lines of production code is the right ratio. Two blockers below, then a lint fix.
1. Blocking — selecting a model must not persist a global default
setDefaultModelProfileForControl defaults to persistDefault: true (agent-session.ts:10860), and the model.set synthetic branch passes persistDefault: true explicitly (session-runtime.ts:907, bus/index.ts:2495). So one pick in an ACP Model dropdown writes durable global config.
This inverts the contract the sibling control path states outright:
/**
* Activate a complete model profile through nonvisual session control.
* Session-scoped only: does not persist `modelProfile.default`.
*/
async activateModelProfileForControl(profileName: string)The TUI is equally explicit — selector-controller.ts:1570 prompts "Scope: 'session' (this session only) or 'default' (persist)" and only persists when the user picks default. Both existing surfaces treat persistence as an opt-in the user states. The new SDK path makes it the unavoidable default, and the wire has no way to ask for session-only.
The blast radius is wider than modelProfile.default. persistDefault also runs (model-profile-activation.ts:524-533):
prepared.settings.set("modelRoles", {});
prepared.settings.set("task.agentModelOverrides", {});Your own test locks this in (sdk-default-model-selection-e2e.test.ts:461): a user with modelRoles: { default: "runtime-provider/initial-model" } has it silently reset to {} by a model-picker click. That config survives the session and the process. A user who set per-role models by hand loses them with no prompt, no confirmation, and no undo — from an action whose UI affordance says "pick a model."
--mpreset X --default is a fair analogy for the mechanism, but not for the consent: there the user typed --default. In the picker they did not.
Suggested shape — persistence stays opt-in and explicit:
- Default
model.seton a synthetic id to session-scoped (persistDefault: false), matchingactivateModelProfileForControl. - If persisting remains desirable, gate it behind an explicit wire field (e.g.
scope: "session" | "default", defaulting tosession) so the client states intent, mirroring the TUI prompt. - Either way, do not clear
modelRoles/task.agentModelOverridesfrom a picker-driven path without explicit consent.
Happy to be overruled if this is a deliberate product call — but then it needs to be stated in docs/sdk.md as destructive, and I would still want the role-clearing carved out.
2. Blocking — check fails on this branch
bun --cwd=packages/coding-agent run check exits 1:
src/sdk/model-profile-model.ts:1:52 lint/style/noRestrictedImports
x Import from @gajae-code/ai/core instead.
import { UNK_CONTEXT_WINDOW, UNK_MAX_TOKENS } from "@gajae-code/ai";models.ts in this same PR already imports from @gajae-code/ai/core, so this is a one-line fix for consistency.
Not blocking
Tests pass — 145 pass / 0 fail across sdk-q10-models, sdk-model-profile-model, sdk-default-model-selection-e2e, sdk-host-wiring, sdk-control-dispatch.
The config.patch admission serialization is the subtlest part of the change and the comment explaining why the patch must not be wrapped in the shadow refresh ("that would delete the entry it just wrote on the second patch") is exactly the kind of note that keeps a future reader from reintroducing the bug. Good.
Once (1) is resolved the rest of this is in good shape.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db70f317b6
ℹ️ 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".
db70f31 to
4f39015
Compare
|
Addressed review 4880878386 in 4f39015. Synthetic |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f390152a4
ℹ️ 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".
1b29dac to
d11aefa
Compare
|
REQUEST_CHANGES Reviewed exact head Blocking finding — reserved-namespace collision detection is not registry-complete. CI: the requested SHA is not green. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d11aefa993
ℹ️ 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".
109fa8b to
7b38026
Compare
|
Fixed the three profile-scope findings in 7b38026: materialization now requires the persisted and active profile identities to match; activation keeps effective rollback snapshots separate from durable construction bases; and the changelog now states synthetic selection is session-scoped. Verified with typecheck and 48 focused tests. |
7b38026 to
aeda94c
Compare
|
Re: the REQUEST_CHANGES collision finding (comment 5214389822) — fixed in 109fa8b (present in current head aeda94c). |
aeda94c to
ebb3808
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebb380887e
ℹ️ 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".
ebb3808 to
40bd86a
Compare
|
Fixed in 40bd86a. Durable profiles (persisted modelProfile.default matching the active marker) now survive session transitions — their marker and runtime role overrides are preserved for the successor since the startup policy reapplies them on the next launch — while session-only markers are dropped together with their role overrides. Concrete selection after a session-only profile clears the override layers without materializing them globally, in both the typed control path and the extension setModel seam. Added regression coverage for durable /new survival, session-only /new reset, and concrete-selection override cleanup. Verified: package check and 193 focused tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40bd86ac60
ℹ️ 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".
40bd86a to
962b35c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 962b35cf0d
ℹ️ 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".
e4de5d3 to
3cd7353
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cd7353c58
ℹ️ 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".
3cd7353 to
8491f2c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8491f2cbf7
ℹ️ 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".
8491f2c to
1f83d2d
Compare
…dels External ACP/SDK clients (notably the paseo TUI) could only use presets via the session-scoped startup Preset select, never as ordinary model choices, and could not persist a preset as the global default. Selecting a preset now behaves exactly like picking a model: Q10 models.list/current lists every availability-filtered profile as a logical gajae-code/<profile> row, and model.set on that namespace activates the profile live AND persists it to global modelProfile.default (mirror of `gjc --mpreset <name> --default`), with the same credential preflight, role clearing, flush, and rollback as the CLI path. The facade lives only in the Q10 projection and a shared resolver: no fake Model registry entries, no TUI / --list-models / /v1-models / coordinator / Q29 surface changes. Logical current derives exclusively from the in-session active-profile marker (never the persisted default), cleared only on successful concrete user/startup-override materialization (including the scoped, unscoped, and role cycle paths and the interactive setModel bindings); config.patch and activation serialize through the shared session admission boundary, with a structurally-compared shadow that invalidates profile-owned keys after activation while preserving unrelated patches; availability uses the Q27 authenticated-provider derivation with pattern-aware, managed-fallback-eligible default resolution, and degrades fail-closed on registry, join, or per-profile resolution errors. gajae-code is a reserved namespace with deterministic collision handling. Planned via ralplan (run 20260806-201255; Architect CLEAR pass 3, Critic OKAY pass 3) and executed under ultragoal with boundary-cohort gate and terminal critic OKAY. Verified: 247/247 affected tests, package typecheck, ACP core-v1 conformance 21/21, the four repo gates, and a live real-process ACP smoke (paseo-shaped) proving selection, global persistence, fresh-launch reapply, prompt execution on the profile default model, and non-leakage into --list-models. Lore-id: preset-to-model-sdk-facade Directive: keep gajae-code reserved; document any future namespace change Tested: Q10 synthetic projection, marker lifecycle, concurrency races, config.patch shadow freshness, registry-error fail-closed, ACP corpus, live ACP smoke Not-tested: manual paseo picker render on a real device
1f83d2d to
5a7aed9
Compare
|
Re: "Restore the configured model when clearing session profiles" — fixed in 5a7aed9. /new now restores the configured global default model when a session-only profile is dropped, so the successor no longer inherits the profile concrete model. Verified: package check and 263 focused tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a7aed9934
ℹ️ 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".
| // Switching to another session file must not carry the predecessor's | ||
| // profile marker or role overrides into the successor; the successor's | ||
| // own configured model is restored above. | ||
| if (switchingToDifferentSession) this.#resetSessionScopedModelProfileState(); |
There was a problem hiding this comment.
When a session-scoped synthetic profile is active and session.resumeModelBehavior is useCurrentDefault, switching or resuming a different session reads settings.getModelRole("default") at line 17155 while the predecessor profile's runtime override is still installed, resolves that profile model, and only clears the profile state here afterward. Because the reset does not restore this.model, the supposedly session-scoped model leaks into the successor session; clear the transient profile before deriving settingsDefaultEntries.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-reviewed at head 5a7aed9934a3b009ec4279d0b8f66b01f1d0020a in a clean worktree off upstream/dev (fd585f2e2).
My previous blocker (#4880878386 — synthetic selection writing the global default) is fixed and verified: both hosts now pass persistDefault: false (sdk/host/session-runtime.ts:880-883, sdk/bus/index.ts:2467-2470), the docs/CHANGELOG were rewritten to the session-scoped contract, and the biome noRestrictedImports failure is gone. Q10 projection work is genuinely good — see the "verified correct" list at the bottom.
One blocker remains, and it is the exact invariant the PR advertises.
1. BLOCKING — /new still leaks a session-only preset's model into the successor
#initializeNewSessionState guards the leak by restoring resolveConfiguredDefaultModel():
// agent-session.ts:10586-10591
if (droppingSessionOnlyProfile) {
const configuredDefault = this.resolveConfiguredDefaultModel();
if (configuredDefault && (!this.model || !modelsAreEqual(this.model, configuredDefault))) {
this.#setModelAuthoritatively(configuredDefault, "restore");
}
}resolveConfiguredDefaultModel() resolves through #resolveRoleModelFull("default", available, undefined) — note currentModel = undefined (agent-session.ts:11115-11118), so when modelRoles.default is unset it returns undefined (agent-session.ts:14208-14214) and the guard is a no-op. this.model stays on the profile's model and is recorded as the successor's model three lines later (agent-session.ts:10603).
This is not a corner case for this PR's own audience: an ACP-only user (paseo) who has never picked a concrete model has no modelRoles.default. Worse, #resetSessionScopedModelProfileState() unconditionally deletes the default key it installed (agent-session.ts:10842), so the reset itself is what makes the lookup fail.
Reproduced against this head — profile custom-eco maps default -> runtime-provider/profile-model, no modelRoles configured anywhere:
expect(settings.getGlobal("modelRoles")).toBeUndefined(); // pass
expect(session.model?.id).toBe("initial-model"); // pass
await session.setDefaultModelProfileForControl("custom-eco", { persistDefault: false });
expect(session.getActiveModelProfile()).toBe("custom-eco"); // pass
expect(session.model?.id).toBe("profile-model"); // pass
expect(await session.newSession()).toBe(true); // pass
expect(session.getActiveModelProfile()).toBeUndefined(); // pass — marker cleared
expect(session.model?.id).toBe("initial-model"); // FAIL: "profile-model"The existing coverage at sdk-default-model-selection-e2e.test.ts:662-677 passes only because an earlier model.set in the same test already wrote a durable modelRoles.default. The assertion is real but the fixture dodges the failing branch.
Fix: keep the pre-profile concrete model in the session-only lifecycle state at activation time and restore it when resolveConfiguredDefaultModel() yields nothing. Please add the no-configured-default case as a regression test rather than extending the existing fixture.
2. MAJOR — failed activation can promote a merged read into the durable layer
model-profile-activation.ts:497 snapshots with the merged read and :567 writes it back durably:
const previousPersistedDefault = prepared.settings.get("modelProfile.default"); // :497
// ...
prepared.settings.set("modelProfile.default", previousPersistedDefault); // :567 (rollback)The sibling helper 60 lines down gets this right — prepared.settings.getGlobal("modelProfile.default") at :631. As written, a rollback after a failed flush can copy a project-scoped (or override-layer) profile default into global config.yml. Snapshot durable state with getGlobal(), keep the merged value only for the runtime override() restore.
3. MAJOR — durable materialization gates on the merged read
// agent-session.ts:10934-10936
const persistedProfile = this.settings.get("modelProfile.default");
if (persistedProfile === undefined || persistedProfile !== this.getActiveModelProfile()) return false;With modelProfile.default set in a project .gjc/config.yml, a session-scoped gajae-code/<profile> pick followed by any concrete pick satisfies this check and runs materializeActiveModelProfileAssignment, which does settings.set("modelRoles", …), settings.set("task.agentModelOverrides", …) and settings.unset("modelProfile.default") (model-profile-activation.ts:223-226). That is a durable global write plus a project-config deletion out of an action documented as session-scoped — the same consent problem as the original blocker, just reachable through a narrower door. Gate on getGlobal("modelProfile.default").
4. MAJOR — the new config.patch validator has no element validation for arrays
validArraySettingValue only checks elements when the schema declares items.enum (config/settings-schema.ts:3823-3825). Verified against this head:
{"cycleOrder":[1]} -> [] (accepted, persisted)
{"cycleOrder":[{"evil":true}]} -> []
{"cycleOrder":[null]} -> []
{"cycleOrder":1} -> [{"path":"cycleOrder","detail":"Expected array."}]
cycleOrder is string[] (settings-schema.ts:546, :4182) and is iterated by the role-cycling paths. To be fair: config.patch existed on dev with no validation at all, so this PR is a strict improvement and I am not blocking on it — but the validator is now yours, so please close the element hole (or at minimum annotate the string-array settings SDK clients can reach) here rather than in a follow-up nobody opens.
5. MAJOR (tests) — two claims are asserted by tests that cannot fail
sdk-default-model-selection-e2e.test.ts:465-482:Promise.all([config.patch, model.set])plus a final-state assertion. The asserted state is equally consistent with fully parallel execution; delete the admission boundary and this test still passes. If FIFO/mutual exclusion is a stated guarantee, hold one mutation behind a barrier and assert the second cannot settle before release.- No wire-level negative for two advertised behaviors:
model.setrejection while the reserved-namespace collision is active (:541-625only checks Q10 row omission), andauthentication_failedfor a credential-less profile through syntheticmodel.set.
6. MINOR
session-runtime.ts:878-889treats wrapper-function presence as capability presence, where the bus host gates onsdkBindings(). Mirror the bus host so a missing binding produces the same error on both surfaces.docs/sdk.md:332-333,docs/external-control-readiness.md:93say "authentication-required error"; name the wire codeauthentication_failed.sdk-q10-models.test.ts:220-227asserts the unknown-model fallbacks are positive numbers instead of the actual shared constants.
Verified correct (not re-litigating these)
persistDefault: falseat both hosts; result envelopes{ changed: true }/{ provider, modelId, thinkingLevel }; non-offthinking rejected asinvalid_inputbefore admission; credential failure mapped toauthentication_failed.- Docs + CHANGELOG match the implemented session-scoped contract; entry sits under
## [Unreleased], released sections untouched. - Q10 projection:
gajae-code/<profile>round-trips losslessly for any legal profile key (slashes, unicode, whitespace); reserved-namespace collision fails closed at all six call sites including selection; availability derivation is shared with Q27 (AND across strict requirements and groups, OR within each alternative group); registry failure degrades to the concrete catalog instead of failing Q10; the current-row uniqueness invariant holds. - Three new lifecycle seams are classified in
LOCKED_EXCLUSIONS;generate-sdk-operation-inventory.ts --checkis clean (pending=0).
Gates I ran at this head
bun --cwd=packages/coding-agent run check— clean (the one biome warning istest/smithery-env-trust.test.ts, untouched by this PR and present ondev).- Focused suites (
sdk-model-profile-model,sdk-q10-models,sdk-control-dispatch,sdk-host-wiring,sdk-default-model-selection-e2e,agent-session-model-change-cause) — 148/148. rebrand-inventory.ts --strict,check-visible-definitions.ts,generate-sdk-operation-inventory.ts --check— pass.verify-g002-gates.tsfails identically onupstream/dev, byte-for-byte. Pre-existing, not this PR.- Environment note for whoever runs this next:
sdk-default-model-selection-e2e.test.tsfails withFixture broker runtime already registered: session:<uuid>whenGJC_SESSION_IDis inherited from a parent gjc session, because bothcreateAgentSessioncalls then share a session id. Also reproduces ondev, so it is pre-existing fixture fragility, but it makes the file unrunnable from inside a gjc shell — worth a separate fix.
Blocking on #1; #2 and #3 belong in the same PR since they are the same "session-scoped action performs a durable write" class as the original blocker. #4-#6 I will not hold merge over once #1-#3 land.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES
Exact head reviewed: 5a7aed9934a3b009ec4279d0b8f66b01f1d0020a (single squashed commit, preset-to-model → dev). Reviewed read-only: exact-head diff, affected contracts, prior reviews, tests, and the CI run bound to this SHA.
The exact head is not terminal-green, so this is not MERGE_READY. Two independent grounds:
1. Exact-head CI is red
Run 31167503406 ("Dev CI", event pull_request, head_sha = 5a7aed9934…) is a failure on this exact head:
Affected path validation / plan— FAILURE:Exact-head CI requires this PR head to contain base fd585f2e2dc01e0fb577b8690156e01558a39a17; rebase onto current dev.Verified locally: merge-base withdevisb9cb17124;fd585f2e2(the currentdevtip) is not an ancestor of the head. The head needs a rebase onto currentdev.Affected path validation / evidence producer— FAILURE:Unable to download artifact(s): Artifact not found for name: dev-affected-plan-31167503406(downstream of the plan failure).Affected path validation(umbrella) — FAILURE.
GitHub state confirms: mergeStateStatus: UNSTABLE, reviewDecision: CHANGES_REQUESTED.
2. Prior blocker at this exact head is still unresolved
probepark's CHANGES_REQUESTED (2026-08-07T10:10:07Z) was submitted against this exact head and no commit has landed since (head is one squashed commit). I verified the blocker claim in the code:
- BLOCKING —
/newleaks a session-only preset's model into the successor.#initializeNewSessionState(agent-session.ts:10579-10591) only restores a configured default whenresolveConfiguredDefaultModel()yields one, but that resolves through#resolveRoleModelFull("default", availableModels, undefined)withcurrentModel = undefined(:11115-11118); with no durablemodelRoles.defaultthe role-default branch returnsundefined(:14209-14211), the guard is a no-op, andthis.modelstays on the profile's model, which is then recorded as the successor's model (:10603-10604).#resetSessionScopedModelProfileState()unconditionally deletes thedefaultkey it installed (:10842), which is exactly what makes the lookup fail. Reproduced by the reviewer; the existing coverage (sdk-default-model-selection-e2e.test.ts:662-677) passes only because an earliermodel.setin the same test already wrote a durablemodelRoles.default(asserted at:655). - MAJOR — failed activation can promote a merged read into the durable layer.
model-profile-activation.ts:497snapshots with the mergedget("modelProfile.default")and:567writes it back durably; the sibling helper 60 lines down correctly usesgetGlobal()(:631). Rollback after a failed flush can copy a project/override-layer default into globalconfig.yml. - MAJOR — durable materialization gates on the merged read.
agent-session.ts:10933-10936usessettings.get("modelProfile.default")to decide durable materialization; should gate ongetGlobal(...). - MAJOR (tests) —
sdk-default-model-selection-e2e.test.ts:465-482asserts a final state equally consistent with fully parallel execution (the admission-boundary guarantee is not actually tested), and there is no wire-level negative for reserved-namespacemodel.setrejection orauthentication_failedfor a credential-less profile. Not held as blockers by the prior reviewer; flagged as required coverage for the advertised guarantees.
Confirmed fixed at this head (not re-litigating): persistDefault: false at both hosts (session-runtime.ts:881, bus/index.ts:2468); result envelopes; non-off thinking rejected as invalid_input before admission; Q10 projection with lossless round-trip and fail-closed reserved namespace; docs/CHANGELOG session-scoped contract; operation-inventory generated output in sync.
Required before merge
- Rebase head onto current
devso exact-head CI (Affected path validation / plan,/ evidence producer) is green on the new head. - Fix the
/newleak: keep the pre-profile concrete model in the session-only lifecycle state and restore it whenresolveConfiguredDefaultModel()yields nothing; add the no-configured-default case as a regression test (do not extend the existing fixture). - Use
getGlobal("modelProfile.default")for the durable snapshot/rollback and materialization gate (#2 and #3 above) in the same PR.
One review, one verdict: REQUEST_CHANGES.
…ssion Review of Yeachan-Heo#3988 found the advertised session-scoped guarantee did not hold when no durable modelRoles.default exists -- the default state for the ACP-only clients this feature targets. resolveConfiguredDefaultModel() resolves with currentModel=undefined, so it returns undefined and the restore guard in #initializeNewSessionState was a no-op, leaving /new to record the profile's model as the successor's. Two sibling gates read the merged settings layer where they authorize durable global writes. Lore-id: 3f9c21ab Constraint: the pre-profile model must be snapshotted by the caller -- activation replaces the runtime model before noteProfileInstalledOverrides runs Constraint: first activation wins so chained session-only profiles restore the original selection, not the previous profile's Rejected: read this.model inside noteProfileInstalledOverrides | setModelTemporary already overwrote it, so it captures the profile's own model Rejected: change resolveConfiguredDefaultModel to accept a current model | the TUI resume flow depends on its current semantics Confidence: high Scope-risk: narrow Reversibility: easy Tested: /new after a session-only profile with no durable default restores the pre-profile model and writes no modelRoles Not-tested: rollback promotion under a concurrent project-config write
|
Pushed What changed#1 BLOCKING — The subtlety worth flagging: the obvious fix — capture The correct source is const restoredDefault = this.resolveConfiguredDefaultModel() ?? preProfileModel;
#2 MAJOR — rollback promoted a merged read into the durable layer. #3 MAJOR — durable materialization gated on a merged read. Regression test in With the fix, 3/3. That is the branch the existing coverage at Correcting my own review — #4 was wrongI called the
I had implemented the element check, then found it broke that test on What I did keep is one strictly-additive improvement to this PR's own new surface: Still open, non-blocking
Gates at
|
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Clearing my CHANGES_REQUESTED. Both grounds that blocked this PR are resolved at 0a11359ff.
#1-#3 fixed in bcc0b27f5, which I pushed to this branch. Each was verified by control run, not by inspection: removing the ?? preProfileModel fallback makes the new regression test fail with Expected: "initial-model" / Received: "profile-model" — the exact signature from my original report — and restoring it returns 3/3.
CI is green. 0a11359ff merges current dev into the branch rather than rebasing, so snowykr's 5a7aed993 keeps its SHA while satisfying the Verify PR head contains exact base gate that @Yeachan-Heo correctly identified. 29 checks pass, 6 skipped, mergeStateStatus: CLEAN.
I withdraw my MAJOR #4. validArraySettingValue is pre-existing and untouched by this PR; the container-only contract was a deliberate decision in #3603 and is locked on dev by settings-array-enum-validation.test.ts. I implemented the element check, watched it break that test on dev too, and reverted it. It was not this PR's to close and I should not have filed it against this PR.
Scope of this approval: I approve snowykr's feature work — the Q10 synthetic-model projection, the session-scoped selection contract, reserved-namespace collision handling, and the docs/CHANGELOG. I am not an independent reviewer of bcc0b27f5, because I wrote it.
So I am not merging this. @Yeachan-Heo's CHANGES_REQUESTED from 4881984744 is still open and should be resolved by someone who did not author code in this branch — that review independently reproduced all three defects, which makes it exactly the second pair of eyes bcc0b27f5 needs.
Non-blocking follow-ups remain as listed in my previous comment: the Promise.all FIFO test at sdk-default-model-selection-e2e.test.ts:465-482 still passes with the admission boundary deleted, the two hosts gate model.set differently (session-runtime.ts:878 vs bus/index.ts:2467), and the docs still say "authentication-required error" where the wire code is authentication_failed.
3 participants
Problem
External ACP/SDK clients (notably the Paseo TUI, registered as a generic ACP provider) can discover GJC's model catalog through the ACP
Modelselect, but model presets were only reachable through the session-scoped startup--mpreset/Q27Presetselect — never as ordinary model choices — and there was no way for a client to persist a preset as the global default. Users of paseo-style clients had to fall back to the TUI for preset configuration.Change
The SDK
models.list/current(Q10) catalog now lists model profiles as logical synthetic models under the reserved namespacegajae-code/<profile>(e.g.gajae-code/codex-eco, displayed as "Codex Eco"). Selecting one throughmodel.set(or the ACPModelpicker) activates the profile in the live session and persists it to the globalmodelProfile.default, mirroringgjc --mpreset <name> --default— with the same credential preflight, role clearing, settings flush, and rollback as the CLI path.Key semantics:
sdk/model-profile-model.ts). No fakeModelregistry entries; no TUI/model,--list-models, auth-gateway/v1/models, coordinator MCP, or Q29 provider-row changes. Verified non-leakage:gjc --list-modelsshows nogajae-coderows.current/config.list/getmodelderive exclusively from the in-session active-profile marker (session.getActiveModelProfile()), never the persisted default; the marker is cleared only on successful concrete user/startup-override materialization (incl.setModelTemporaryForControl) and never on internal fallback/restore/rollback (activation's ownprofile-activationcause never self-clears). A persisted-but-inactive default never creates a synthetic current row.model.setwithgajae-code/*validates/canonicalizes the suffix (lossless after the first slash, legacy alias support), rejects non-offthinking levels asinvalid_inputbefore admission, and routes throughactivateModelProfile({ persistDefault: true })inside the shared session admission queue.config.patchis serialized through the same admission boundary so a patch racing an activation is never lost or clobbered.models.ymlprovider namedgajae-codefails closed (rows omitted, selection rejected) instead of being silently shadowed.invalid_input(ACPinvalidParamsvia the existing mapping); missing profile credentials →authentication_failed(authRequired); the no-thinking result envelope is exactly{ changed: true }, the typed result is{ provider: "gajae-code", modelId, thinkingLevel }.User-visible behavior
config.list/getreport the synthetic id as current (gajae-code/<profile>,currentThinkingLevel: "inherit"); the existing ACP startup--mpreset/Q27Presetselect stays session-scoped and unchanged.docs/sdk.md,docs/external-control-readiness.md(Paseo section),docs/sdk-app-guide.md,docs/models.md;CHANGELOG.mdentry added.Validation
sdk-model-profile-model.test.ts; extendedsdk-q10-models,sdk-control-dispatch,sdk-host-wiring,sdk-default-model-selection-e2e), including concurrency races (two concurrent selections FIFO,config.patchracing a selection) and registry-error fail-closed.bun --cwd=packages/coding-agent run check:typesclean; packagecheck(biome + tsc) clean.verify-g002-gates.ts,rebrand-inventory.ts --strict,check-visible-definitions.ts,default-gjc-definitions.test.tsall pass;check:schemasclean; docs-index regenerated in sync.acp-core-v1corpus (acpx 0.13.0) 21/21.gjc acpprocess with an ACP client (same protocol paseo uses): the Model config options containgajae-code/custom-eco;session/set_config_optionswitches the session; a prompt turn runs on the profile's default model;modelProfile.defaultis persisted in the realconfig.yml; a freshgjc acplaunch reapplies it;--list-modelsshows nogajae-coderows.Related work
No existing open PR targets this surface (checked
gh pr list --head preset-to-model). This is additive to the ACP preset-catalog work (#3922) — it extends the same Q10/ACP pipeline from "filter the model catalog" to "offer presets as selectable models that persist globally". No duplicate; complements the existing startupPresetmode rather than replacing it.Checklist
dev