Skip to content

feat(daemon): add model gateway credentials - #974

Open
spacedragon wants to merge 4 commits into
mainfrom
dev/yulong/leatherjacket
Open

feat(daemon): add model gateway credentials#974
spacedragon wants to merge 4 commits into
mainfrom
dev/yulong/leatherjacket

Conversation

@spacedragon

Copy link
Copy Markdown
Contributor

Summary

  • translate cloud-daemon MODEL_BASE_URL and MODEL_TOKEN into the native Claude, Codex, and OpenCode configuration surfaces
  • add the HTTPS key-server client defined by feat(protocol): declare the agentconnect.key-server/v1 credential contract #963, including per-request bearer-token file reads and IssueKey/RevokeKey handling
  • scope dynamic model credentials to logical sessions, refresh them on activation, revoke them during teardown, and preserve static credential fallback when no key server is configured
  • document the runtime mappings and daemon/key-server lifecycle

Runtime mapping

Runtime Base URL API key
Claude ANTHROPIC_BASE_URL ANTHROPIC_AUTH_TOKEN
Codex CODEX_CONFIG provider base_url OPENAI_API_KEY via provider env_key
OpenCode OPENCODE_CONFIG_CONTENT provider options.baseURL provider options.apiKey via {env:MODEL_TOKEN}

Validation

  • pnpm --filter @agentconnect.md/daemon typecheck
  • pnpm exec eslint packages/daemon/src/index.ts packages/daemon/src/cli/run-foreground.ts packages/daemon/src/daemon.ts packages/daemon/src/runtimes/model-provider-config.ts packages/daemon/src/key-server/client.ts packages/daemon/test/model-provider-config.test.ts packages/daemon/test/key-server-client.test.ts packages/daemon/test/model-key-session.test.ts
  • pnpm --filter @agentconnect.md/daemon exec vitest run test/model-provider-config.test.ts test/key-server-client.test.ts test/model-key-session.test.ts
  • pnpm --filter @agentconnect.md/daemon exec vitest run test/daemon-k8s-mode.test.ts test/run-foreground.test.ts
  • pnpm --filter @agentconnect.md/daemon exec vitest run test/daemon-lifecycle.test.ts

@agentconnect-md-test agentconnect-md-test Bot 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.

I found three blocking correctness issues in the new session-scoped credential lifecycle: OpenCode credential selection does not follow effective per-session model changes, refresh can terminate live SDK background work, and TTL cleanup can revoke or stop a host during an already-admitted initialization. These are regressions introduced by this PR, not compatibility or migration concerns.

Verification performed on the trusted revision: the checkout is a synthetic merge whose parents are exactly base 2a1a76448f46c605d6e8216ee13df445e8fdb073 and head 5c4660f204341df01bc4164f96d694728265ed14; every changed file matched the head revision. The 14 focused credential tests and 81 daemon lifecycle/Kubernetes/foreground tests passed. Daemon typecheck and changed-file ESLint passed, and the patch whitespace/conflict-marker check was clean.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

this.log.info(`session ${key} model override → "${model}"`)
const acpSessionId = rec.acpSessionId
const host = this.hosts.get(rec.agentId)
const host = acpSessionId ? this.hostForStoredSession(rec.agentId, acpSessionId) : undefined

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.

[P1] Rotate OpenCode credentials before applying a cross-provider model. This new lookup makes setModelByKey apply live to a credential-scoped host, but that host's KeyProvider, API key, base URL, and configured provider were selected once from agent.runtimeOverrides.model. OpenCode model IDs are provider-prefixed and this setter can switch, for example, openai/... to anthropic/...; setSessionModel then moves the runtime onto a provider whose credential was never issued or injected, and later activations recompute the target from the unchanged agent default so they never repair it. A first-turn webchat model override has the same mismatch because the host is selected before that override is staged. Resolve the target from the effective session model and replace/reload the host when the provider prefix changes, or prevent cross-provider choices on such a host.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 538b567. The binding is now derived once, from the host that is actually running, and honoured at every apply site instead of only inside setModelByKey:

  • boundModelTarget(sessionKey, agentId) — the credential entry’\s target under a key server, the agent-default target on the static path, undefined when this daemon injects no credential at all (the runtime carries its own auth, so any model is fine).
  • modelCrossesHostProvider(...) compares the requested model’\s resolved target against that.

setModelByKey no longer rejects under a key server; it records the sticky override and returns true, and the per-turn re-apply in dispatch skips setSessionModel while the host is bound elsewhere. ensureModelSessionHost already resolves its target from store.getModelOverride(sessionKey), so the next start after the work settles issues for the new provider and rebinds — the repair path you noted was missing.

First-turn webchat selection is fenced too: it is passed into ensureModelSessionHost as effectiveModel, so under a key server the host is started for that model rather than selected before it.

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.

Confirmed—538b567e addresses the dynamic key-server binding concern. The running host retains its provider, cross-provider choices remain sticky without being applied live, and the next quiescent activation rebinds using that override. The first-turn key-server path also selects the credential target from the requested model.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

Comment thread packages/daemon/src/daemon.ts Outdated
throw error
}
if (entry) {
await this.stopModelSessionRuntime(entry, entry.host)

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.

[P1] Do not rotate a host while its session still has live SDK work. A logical session can be between top-level turns while Claude background tasks or a self-follow-up cycle remain active; elsewhere the idle close/reclaim paths consult sessionSdkQuiescent specifically because host.stop() kills those jobs. Once refreshAtMs passes, the next activation unconditionally stops that host here, terminating the background work even though the old grant has not expired. Defer a non-expired rotation until the session lease is quiescent (or migrate without killing it), and handle the expiry boundary explicitly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 538b567, with a deliberate change of policy: a started host is now authoritative for its whole working life. ensureModelSessionHost collapses the three rotation triggers — provider change, refresh hint, expiry — into one check and returns the existing host untouched whenever modelSessionSdkQuiescent is false. Nothing stops a host that still has SDK work behind it.

That extends to the expiry boundary you asked to handle explicitly: it no longer throws. A lapsed grant now fails upstream on the next request instead of the daemon killing background work to enforce a boundary the issuer already owns. Recorded in key-server.md §5 so the tradeoff is not silent. Covered by model-key-session.test.ts — "keeps a working host on its start-time credential past expiry".

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.

Confirmed—538b567e addresses this concern. Deferring provider changes and rotation while SDK work remains live is consistent with the documented host-authoritative policy, including the explicit expiry tradeoff. My current requested change is unrelated to this thread.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

// drain below (and the CP READY replay) delivers it eventually.
void this.drainWebchatMcpRevocations()
for (const row of closed) {
void this.releaseModelSessionHost(row.key).catch((error) =>

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.

[P1] Exempt already-admitted session keys from TTL teardown. closeIdleSessions only sees the durable row state and SDK lease, but an admitted turn can spend substantial time before SessionManager.handle() changes an old row from idle to prompting; the surrounding host-reclaim and retention code already uses the serial gate/active-dispatch fences for this window. In key-server mode this new release can therefore delete and revoke an entry while ensureModelSessionHost is still starting it, or stop the exact host just returned before session/new|load, leaving the turn with an untracked/revoked or closed runtime. Include gate-owned/active-dispatch sessions in the exemption before closing and releasing them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was already closed by d585fa1, before this round: closeIdleSessions's exemption predicate moved from !sessionSdkQuiescent to sessionRetentionActive, which covers exactly the two fences you named — inflight (the claimed serial gate) and activeDispatchDoneByKey (active dispatch) — alongside live Pending turns and pending durable inbox rows. Rows returned in closed therefore never include an admitted-but-not-yet-prompting session, so the releaseModelSessionHost call in that loop cannot race ensureModelSessionHost. Covered by "the idle sweep deletes expired sessions but spares live turns and gate-owned keys" in daemon-lifecycle.test.ts. Leaving the code as is.

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.

Confirmed. The sessionRetentionActive predicate now covers admitted initialization and active-dispatch ownership, so TTL cleanup cannot release that session during the pre-prompt window. This concern is resolved.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

@agentconnect-md-test agentconnect-md-test Bot 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.

Two blocking lifecycle/configuration issues remain in the synchronized revision. The prior live-refresh, cross-provider dynamic-host, and retention fixes now look sound in their covered paths, but a credential runtime can still escape teardown during startup, and static OpenCode credentials are still not bound to model-provider changes.

Verified at exact head d585fa164daeb3fb1ac76f3fdb7bbb6c43a5815b. The focused credential tests passed (17/17), the broader daemon lifecycle/K8s/foreground tests passed (82/82), and daemon typecheck plus ESLint passed. I also reproduced the startup/release race directly: the selected host returned with zero tracked entries and zero stop calls.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

Comment thread packages/daemon/src/daemon.ts Outdated
this.modelSessionHosts.set(sessionKey, entry)
}

if (!entry.host) entry.host = await this.startModelSessionRuntime(agent, entry)

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.

[P1] Fence this startup against concurrent release. The entry is published before this await, so agent stop/detach can delete it, see no entry.host to stop, and revoke the grant while startModelSessionRuntime is still pending. When the await completes, it assigns a live host to the detached object; the teardown's final pass only calls stopHost, so this credential host is never tracked or stopped. I reproduced that sequence with tracked: 0 and stops: 0. Track the in-progress start so release can join/cancel it, or verify map ownership after the await and immediately stop a superseded host.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 538b567. The start is published on the entry before it is awaited, and the host assignment happens inside the promise chain rather than after it:

const starting: Promise<AcpHost> = (owner.starting ??= this.startModelSessionRuntime(agent, owner)
  .then((started) => { owner.host = started; return started })
  .finally(() => { if (owner.starting === starting) owner.starting = undefined }))

releaseModelSessionHost sets released, then joins entry.starting before stopping — so by the time it looks at entry.host the host exists and gets stopped. The ensure side re-checks ownership after the await (owner.released || modelSessionHosts.get(sessionKey) !== owner), stops the superseded host and fails the turn rather than handing back a process nobody tracks. Both orderings are safe because the assignment precedes the promise settling.

Test: "stops a host started after its entry was released" — the start is held pending, release runs, then the start settles; stop is called once and key-1 is revoked.

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.

Confirmed—538b567e closes the start-versus-release race in both orderings, and the regression test covers the original reproduction. My current requested change concerns a different path: host.stop() rejecting after an already-running host has been selected for refresh.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

Comment thread packages/daemon/src/daemon.ts Outdated
private setModelByKey(key: string, model: string): boolean {
const rec = this.chatRuntimeSession(key)
if (!rec) return false
const credentialHost = this.modelSessionHosts.get(key)

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.

[P1] Apply the provider binding to the static MODEL_TOKEN path as well. modelSessionHosts is populated only when a key server exists; without one, buildAcpHost configures the static OpenCode credential only for the agent's initial provider, while this path still accepts a cross-provider model override. First-turn webchat model selection also enters ensureModelSessionHost only under this.keyServer. OpenCode can therefore switch to a provider whose options never received the configured key/base URL, causing the next prompt to fail. Reject/constrain static cross-provider selections or configure the host for the effective provider before use.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 538b567, and this one applies to the static path as well now — see the reply on 8066.

Under a key server the override is recorded and the next start honours it. On the shared static host there is no per-session start to rebind at, so setModelByKey refuses the selection outright rather than storing one that could never be honoured.

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.

Confirmed. 538b567e rejects cross-provider changes through setModelByKey on the shared static host, and 9624393 closes the separate first-turn staging bypass. This concern is resolved at the current head.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

@spacedragon
spacedragon marked this pull request as ready for review August 15, 2026 07:01
A credential-scoped host was rotated out from under whatever was still using
it, and its provider binding drifted from the model actually selected.

- ensureModelSessionHost returns the existing host untouched whenever its
  session still has live SDK work, whether the trigger is a provider change, a
  refresh hint, or expiry. A lapsed grant fails upstream instead of the daemon
  killing background work to enforce it.
- Publish an in-progress start on the entry so release joins it and stops the
  host it produces; a start that finds itself superseded stops its own host and
  fails rather than leaking an untracked process.
- Derive the host's provider binding once and honour it at both apply sites: a
  cross-provider model is recorded as the sticky override and picked up by the
  next start, never pushed to a running process whose options never received
  that provider's key. The shared static host has no per-session start to
  rebind at, so it refuses such a selection outright.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@agentconnect-md-test agentconnect-md-test Bot 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.

The synchronized revision fixes the previously reported in-progress-start teardown leak and the normal in-session static-provider switch, but two blocking credential lifecycle/configuration paths remain: first-turn webchat selection bypasses the static provider fence, and a replacement grant is leaked when refresh cannot stop the old host.

Reviewed against exact remote head 538b567ec866b18eac8ff49d24fd80f6c6ab9577; all 13 changed local files matched that commit's raw contents. Validation passed: focused credential tests 19/19, daemon lifecycle/K8s/foreground tests 82/82, daemon typecheck, ESLint, and added-line whitespace inspection.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

let remoteMcpServer: import('@agentclientprotocol/sdk').McpServer | undefined
try {
const reviewWorkspace = await this.prepareGithubReviewWorkspace(entry, key, agent)
if (this.keyServer) {

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.

[P1] Fence the static credential path for first-turn model selection too. When there is no key server, this branch is skipped; after sessions.handle, line 14685 persists webchat.runtime.model directly instead of going through setModelByKey. A first turn selecting anthropic/... on an OpenCode host statically bound to openai is therefore accepted and stored. The later modelCrossesHostProvider check only defers applying it, and the shared static host never rebinds, so the UI/session keeps an override that no prompt can honor. Validate/reject the requested first-turn provider before persisting it, or configure the static host for that effective provider.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — fixed in 9624393. The staged first-turn persist now goes through the same binding check as setModelByKey:

if (this.modelCrossesHostProvider(key, agentId, stagedRuntime.model)) {
  this.log.warn(`session ${key}: first-turn model "..." refused — the static host is bound`)
} else {
  this.store.setModelOverride(key, stagedRuntime.model)
}

Under a key server this never crosses — the host was started for that very model via the effectiveModel argument — so the branch only bites on the static path, which is the case with no rebind opportunity. Rejecting rather than configuring the static host for the requested provider is deliberate: that host is shared across every session of the agent, so rebinding it for one session would change the provider under the others.

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.

Confirmed. The binding check now prevents persisting an impossible first-turn override on the shared static host, while the dynamic key-server path remains bound to the requested effective model. Keeping the shared static host on its configured provider is the correct tradeoff here.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

Comment thread packages/daemon/src/daemon.ts Outdated
throw error
}
if (entry) {
await this.stopModelSessionRuntime(entry, entry.host)

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.

[P1] Revoke the freshly issued grant if replacing the old host fails. grant has already been obtained above; if this stop rejects, control exits before the new grant is stored or revoked. Because the protocol explicitly permits long-lived grants without an expiry, that credential can remain valid indefinitely with no remaining key ID owner. I reproduced this with a refresh where key-2 was issued, the stop rejected, the map still held key-1 with no host, and revoke was never called. Catch this failure and revoke the unused grant (and preserve/fence cleanup ownership of the old host) before propagating it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 9624393. The stop is wrapped, and a failure revokes the grant that was just issued before propagating:

const staleKeyId = entry.grant.keyId
try {
  await this.stopModelSessionRuntime(entry, entry.host)
} catch (error) {
  void keyServer.revoke(grant.keyId).catch(...)
  throw error
}

Cleanup ownership of the old host is preserved rather than fenced: the map still holds the old entry, so key-1 is still given back by the eventual releaseModelSessionHost. Only the new grant was ownerless, and since the protocol permits grants with no expiry that was the leak. Test: "revokes the fresh grant when replacing the superseded host fails" asserts key-2 is revoked, key-1 is not, and key-1 is still revoked at release.

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.

The fresh-grant leak is fixed, but cleanup ownership of the old host is not preserved. stopModelSessionRuntime sets entry.host = undefined before awaiting host.stop(). When that stop rejects, this catch leaves the map holding key-1 but no host reference; eventual release revokes key-1 yet cannot retry the stop.

My reproduction showed trackedHost: false after the failure and stopCalls: 1 even after release. That is the remaining finding on the current revision.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

…t-turn model

Two holes the pinning change left open:

- A refresh that issues key-2 and then fails to stop the superseded host exited
  with key-2 owned by nobody. The protocol permits grants that never expire, so
  that one could stay live forever. Revoke it on that path; the map still holds
  the old entry, so its own key is still given back at release.
- The staged first-turn webchat model bypassed setModelByKey and persisted
  straight to the store. On the shared static host that stored a provider the
  host can never be credentialed for, leaving the session showing an override no
  prompt could honour. It now goes through the same binding check.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@agentconnect-md-test agentconnect-md-test Bot 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.

The synchronized revision fixes the orphaned fresh grant and prevents persistence of an impossible first-turn static provider override. One blocking lifecycle issue remains: when replacement cannot stop the old credential host, the host reference is cleared before the failure propagates, so later teardown can revoke its key but cannot retry stopping the runtime.

Reviewed against exact remote head 9624393943c6daaffbbe852128ec039b66dd4316; all 13 PR files matched that commit's raw contents. Validation passed: focused credential tests 20/20, daemon lifecycle/K8s/foreground tests 82/82, daemon typecheck, and ESLint. A direct failure-path reproduction confirmed that after release both grants were revoked, but the old host remained untracked and its stop call count stayed at one.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

if (entry) {
const staleKeyId = entry.grant.keyId
try {
await this.stopModelSessionRuntime(entry, entry.host)

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.

[P1] Preserve cleanup ownership of the old host when this stop rejects. stopModelSessionRuntime sets entry.host = undefined before awaiting host.stop(), so this catch leaves the map owning only the old grant, not the possibly still-running process. releaseModelSessionHost later sees no host and cannot retry the stop; a subsequent refresh can also replace the entry entirely. I reproduced the full path: after refresh failure trackedHost was false, and releasing the session revoked both keys but left the host's stop call count at one. Restore/retain the host reference on failure, or keep a separate fenced cleanup owner that teardown can join or retry.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant