Skip to content

feat(auth/gcp): add GCP credential provider - #1173

Open
wolo-lab wants to merge 12 commits into
mainfrom
wolo/auth-gcp-provider
Open

feat(auth/gcp): add GCP credential provider#1173
wolo-lab wants to merge 12 commits into
mainfrom
wolo/auth-gcp-provider

Conversation

@wolo-lab

@wolo-lab wolo-lab commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Problem

auth/gcp has the credentials-service REST client (#1149) but no auth.CredentialProvider that resolves the acting user's credential from the invocation context. Without it there is no way to wire per-user GCP credentials (Agent Identity / IAM Connector) into a tool call — adk-python has this via its GCP auth provider.

Resolving a credential per end user also needs a way to recover which user, from code that holds only a context.Context. A tool's outbound request reaches an http.RoundTripper several layers below the tool call, past jsonrpc2 and net/http wrapping, with the typed ADK context long since erased.

Summary

  • Add gcp.NewProvider(ctx, gcp.ProviderConfig), returning an auth.CredentialProvider that resolves a per-user credential through the REST client and maps it to an auth.Credential. The acting user is read from the ADK context at resolve time, so the provider is long-lived and shared, and never needs per-user configuration. ctx roots the default client that is built lazily when ProviderConfig.Client is nil — values only, cancellation is not honored, because that client outlives any one request.
  • Add agent.Identity and agent.IdentityFromContext(ctx) (Identity, bool). ADK contexts answer a private key held in internal/adkcontext, a module-internal leaf package shared by agent and internal/context to break the import cycle. Code outside the module cannot name that key, so it cannot mint an identity onto a context that has none. It can still read or rewrite the agent.Identity a wrapper passes through, so the identity is trusted only as far as every wrapper in the chain is, and the doc says so.
  • Three context types answer the key, and they agree. The two that own a session — internal/context.InvocationContext and agent.invocationContext — answer for themselves and never delegate: a nested invocation with no session of its own reports no identity rather than the enclosing invocation's user, whose credential would otherwise be minted for a call they never made. agent.commonContext owns no session, so it asks the invocation it wraps and takes that answer as final. That keeps a tool or callback context working, since theirs return a nil session by design and delegate the key downwards themselves.
  • The identity read recovers rather than nil-testing. session.Session is a public interface of six accessors, so a struct value implements it naturally, a typed-nil pointer survives an interface-nil check, and a session wrapping a nil session (the shape llmagent.newWrappedSession produces for a nil original) panics in the accessor. This runs inside http.RoundTripper, where net/http does not recover, so a session that cannot answer costs the identity and not the process, and the credential path fails closed.
  • The lazy default client bounds its waiters, not its attempt. google.FindDefaultCredentials reads the credentials file with os.ReadFile and probes with the context-free metadata.OnGCE(), so no context can interrupt it. Each caller waits on the earlier of its own context and 30s. A failed attempt is not cached and the next call retries. A hung one is kept rather than abandoned — abandoning it starts a fresh lookup every 30s, each parked in a syscall pinning an OS thread — and if it eventually lands, its client is published to everyone after it.
  • NewProvider validates at wiring time what would otherwise fail on every request from inside a transport: the resource name must be one of the two shapes Client routes on, and a Client that did not come from NewClient is rejected. The shape check is stricter than Client and than adk-python, both of which send any non-connector name to Agent Identity, and ProviderScheme.Name labels it as such.
  • New sentinels: gcp.ErrNoActingUser (no ADK identity on the context, or an invocation whose session carries no user — the two are distinguishable by message) and gcp.ErrClientUnavailable (the default client was not ready in time). Existing sentinels, *APIError and *auth.ConsentRequiredError all stay reachable with errors.Is/errors.As through the provider's wrap.
  • A provider error names the resource and nothing else. It becomes the tool's error, which is fed back to the model and persisted as a session event, and every identifier available at that point is caller-supplied — session.UserID is commonly an email, and the session id arrives unvalidated from the request path. This is a deliberate divergence from adk-python, which names the user.

Behavior change to existing code

auth.ConsentRequiredError.Error() no longer prints the authorization URI, which stays on the AuthURI field for consumers to read. The URI carries the state and the consent nonce that bind the credential, and this error reaches the model and the session store by the path above. apidiff reports the change as additive because no signature moved — the message text is not part of what it compares. Nothing in the tree matches on that message.

Requirements this puts on the transport

Neither can be enforced from this package, and both are stated on NewProvider.

  • Every authenticated request must descend from the invoking user's context. A transport that shares one connection across invocations does not qualify, mcptoolset included: its per-call POSTs are per-user, but the MCP session and the standalone server-to-client stream are opened on the context of whichever user connected first and stay bound to it.
  • The transport must not follow a cross-host redirect. net/http strips Authorization above the RoundTripper, so auth.Transport re-resolves and re-applies the end user's credential to the redirect target. The ADC-backed client NewClient builds for itself already refuses redirects for the same reason.

Both live in packages this PR does not touch and will get their own issues.

@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch 4 times, most recently from 5a28496 to 234d752 Compare July 20, 2026 11:29
@wolo-lab
wolo-lab marked this pull request as ready for review July 20, 2026 11:32
@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch from 234d752 to c65d414 Compare July 20, 2026 11:40
@wolo-lab
wolo-lab requested a review from baptmont July 20, 2026 11:49
@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch from c65d414 to 9186dc4 Compare July 20, 2026 11:49
@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch from 9186dc4 to edeb3da Compare July 22, 2026 21:21
@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch from edeb3da to a4d444d Compare July 22, 2026 22:27

@karolpiotrowicz karolpiotrowicz 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 shape of the change is good: the leaf package to break the agentinternal/context cycle is a clean way to solve it, the Value overrides delegate correctly for every other key, and the defensive copy of Scopes does what its comment claims. Two things need fixing before this lands, and there's a short list of smaller items below.

The default path doesn't work. resolveClient builds the ADC-backed client on a context it cancels one line later, and the token source keeps that context — so with ProviderConfig.Client == nil (the documented default) every credential retrieval fails with context canceled, permanently, because the broken client is then cached. Details inline on auth/gcp/provider.go. It reproduces on authorized_user (the gcloud auth application-default login path), external_account (workload identity federation) and impersonated_service_account; it happens to survive on a service-account JSON key and on GCE/GKE metadata, because those two token sources don't bind the request to the construction context. This is invisible today because resolveClient's lazy-init block has no test coverage.

Two tests don't test what they're named for. Deleting the identity guard in Credential entirely leaves all three provider tests passing, and adding a cross-user credential cache — which would break the provider's headline per-user isolation property — also leaves them all passing. Both are noted inline with the mutations that demonstrate it.

Cross-user credential scope. Worth thinking through before this gets wired up: with a per-user provider behind an auth.Transport, per-request calls are correctly scoped, but a transport that keeps a long-lived shared connection is not. Inline on the doc comment that recommends that wiring.

Smaller items, grouped so they don't clutter the diff:

  • auth/gcp/provider.go:88-91 — errors from resolveClient and RetrieveCredential are returned bare, so a process with several providers wired in gives an operator no way to tell which resource or which user failed. Wrapping with %w keeps errors.Is/errors.As working for the sentinels and the consent error.
  • auth/gcp/provider.go:58 — this is the only error-returning constructor among the providers in auth (StaticToken, APIKey, TokenSourceProvider, ADC, ServiceAccount all return a bare CredentialProvider), and it validates only an empty Name, while RetrieveCredential rejects a much wider class on every request. Either validate fully here or drop the error and match the siblings. Relatedly, returning the interface rather than a concrete *Provider freezes the callable surface — worth deciding before the API is public.
  • auth/gcp/provider.go:32Scheme is a slightly overloaded name in this package: mapCredential already uses scheme for the HTTP auth scheme, and the endpoint fields document "scheme+host". ProviderScheme would read unambiguously and tracks the reference implementation's GcpAuthProviderScheme.
  • auth/gcp/provider.go:46-50 — on the nil-Client path there's no way to point at a non-production endpoint, since :129 hardcodes NewClient(dctx, nil). Building a Client and passing it does work, so this is only about the convenience path.
  • auth/gcp/provider.go:82Identity.UserID is supplied by the embedding server and isn't authenticated anywhere in the framework. That was fine when it was a storage partition key; this change makes it decide whose credential gets minted. Worth stating that trust requirement explicitly in the doc.
  • auth/gcp/provider.go:91 — a failing retrieval's error becomes the tool's error and is fed back to the model and persisted in the session, which carries the consent URI and part of the service's response body along with it. Consider keeping the URI on the struct field and out of Error().

Two questions rather than assertions:

  • agent.Identity carries {UserID, AppName, SessionID} but not the current function-call id. The reference implementation uses that id for its consent anti-loop check (it asks whether consent for this tool call already completed before re-prompting). Adding the field later is a compatible change, so this isn't urgent — but is leaving it out now a deliberate call?
  • Is per-request credential retrieval the intended cost model? Since auth.Transport resolves on every outgoing request and nothing here caches, each request costs a full retrieval, and a pending response costs up to PollTimeout (10s by default) inside a RoundTripper. The service response carries no expiry, so caching may well be the wrong answer — but it seems worth a note either way.

Comment thread auth/gcp/provider.go Outdated
Comment thread auth/gcp/provider.go Outdated
Comment thread auth/gcp/provider.go Outdated
Comment thread auth/gcp/provider.go Outdated
Comment thread auth/gcp/provider.go Outdated
Comment thread auth/gcp/provider_test.go Outdated
Comment thread auth/gcp/provider_test.go
Comment thread internal/adkcontext/adkcontext.go Outdated
Comment thread agent/common_context.go Outdated
Comment thread auth/providers.go Outdated
// shared helper introduced together with the first provider that needs it —
// so identity rides on the one ADK context rather than an auth-specific key.
// provider, which keys on the user) recovers it from ctx via
// [agent.IdentityFromContext] — so identity rides on the one ADK context

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.

This doc link doesn't resolve — it renders as the literal text [agent.IdentityFromContext], brackets and all, because Go's doc tooling only resolves a [pkg.Sym] link for packages the file actually imports, and nothing in package auth imports .../agent. (The identical text in auth/gcp/provider.go:55 does render as a link, since that package imports agent.)

Adding the import just for a doc link would create a real layering dependency, so naming the package in prose is probably the cheaper fix.

@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch from a4d444d to bb3f6e3 Compare August 11, 2026 14:20
@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch from bb3f6e3 to 458290d Compare August 13, 2026 09:35

@karolpiotrowicz karolpiotrowicz 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 routing through the ADK context is the right shape, and the parts that carry the security weight hold up: the identity is re-read from the calling context on every request so a shared provider never serves one user's credential to another, Scheme.Scopes reaches the wire unmodified, the defensive copy is real, and no error path carries a token. Two areas need work before this lands.

Value can panic, and the guard added to prevent that is one of the causes

hasSession ends in return s != nil && !reflect.ValueOf(s).IsNil(). reflect.Value.IsNil is only defined for chan, func, interface, map, pointer, slice and unsafe pointer — for anything else it panics. session.Session is a public interface of six read-only accessors, so a struct value is a natural way to implement it, and s != nil is true for a struct, so the short-circuit does not help.

Calling hasSession directly with each receiver shape:

pointer receiver     -> true
typed-nil pointer    -> false
nil                  -> false
struct value         -> panic: reflect: call of reflect.Value.IsNil on struct Value

The guard does what it was written for and panics on a different legal input.

Three separate shapes reach a panic, and they need to be considered together because they do not share a fix:

  • A struct-value session panics in reflect.IsNil, as above.
  • A typed-nil session is correctly rejected by hasSession, and then Value falls through to c.Context.Value(key). For a Promoted context that embedded context is the parent *InvocationContext, whose Value guards with a plain c.params.Session != nil and then dereferences. The reflect guard is undone by its own delegation.
  • A non-nil pointer wrapping a nil inner session passes both checks — the pointer is genuinely non-nil — and both then call UserID().

That third shape is the constraint on any fix: because the accessors are arbitrary third-party code, no nil test can make Value panic-free. A kind-aware IsNil closes the first two and leaves the third. Snapshotting the Identity once at NewInvocationContext time would close all three and move the accessor calls off the per-request path, at the cost of not tracking a session mutated mid-invocation. A recover in the identity lookup also works.

Worth fixing even though no current deployment hits it. Every production session.Session is a pointer type, and the four value-typed implementations in the tree are all fakes — including TestSession, which is in a non-_test.go file but is imported only from tests. What makes it worth blocking on is where it lands rather than how often: auth.Transport resolves a credential per outbound request, so this executes inside http.RoundTripper on the caller's goroutine, where net/http does not recover. It is reachable by correct third-party code against a public interface, and both Value doc comments currently promise the opposite — agent/common_context.go says "Value never panics" and internal/context/invocation_context.go says "never panicking".

The two Value implementations should also agree. For a typed-nil session that does not dereference, one returns ok=false and the other returns ok=true with an empty Identity. One shared helper for "is there a usable session" would remove the divergence and the duplicated logic.

A hung ADC lookup wedges the provider for the process lifetime

resolveClient bounds each waiter but nothing bounds the init itself. context.WithoutCancel strips the deadline without adding one, and singleflight holds the "client" key until the function returns. If FindDefaultCredentials blocks — a stalled NFS or gcsfuse mount, an ADC path that is a FIFO or a device, a hung resolver inside OnGCE — the key is never released and p.client is never set, so every later call attaches to the dead flight and can only burn its own deadline.

With the ADC path a FIFO, then the environment repaired with a valid credentials file before the second caller arrives:

caller 1, 300ms budget                     -> context deadline exceeded after 301ms
caller 2, healthy environment, 2s budget   -> context deadline exceeded after 2.001s

The doc comment's "a failed init is not cached; the next call retries" holds for a failed init but not a hung one. auth/providers.go already solves this for the sibling lazy path — initTimeout = 30 * time.Second, with the comment "bounds a hung init so it fails and is retried, not wedged forever". Since FindDefaultCredentials ignores cancellation, the bound has to retire the attempt rather than just pass a bounded context, which is what lazyTokenSource does.

The shared client inherits whichever caller won the cold-start race

Same function: NewClient(context.WithoutCancel(ctx), nil). WithoutCancel strips cancellation but keeps values, so the process-lifetime client is parameterised by one arbitrary request. NewClient hands that context to oauth2.NewClient, which reads oauth2.HTTPClient off it, and to FindDefaultCredentials, whose token source retains it for every later refresh.

With Alice's context carrying an instrumented oauth2.HTTPClient and Bob's context plain, Alice's transport carried both the ADC token mint and Bob's credential retrieval. It also pins that first invocation's whole context graph — session, events, agent, artifacts — for the life of the process, and that retained context still answers the identity key with Alice's Identity.

Nothing about a shared client needs a per-request value. Rooting it at context.Background() removes both effects. If some ambient value really is needed, taking it explicitly in ProviderConfig would at least make it deterministic.

Smaller things

  • auth/gcp/doc.go now contradicts the package. Lines 27-29 say "This package holds only the transport-level client. The auth.CredentialProvider that resolves the acting user from the invocation context is a separate, higher layer." This PR puts that provider here, and that paragraph is the first thing go doc auth/gcp prints.
  • The fail-closed paths have no tests. Every error branch in resolveClient is uncovered, including ADC discovery failing, and so is the empty-UserID guard. They behave correctly today, but deleting the UserID guard leaves the suite green while changing the error callers see from ErrNoActingUser to a bare one from RetrieveCredential. The same is true of the reflect branch in hasSession — removing it keeps everything green, which is why the struct case shipped. Worth a table over {pointer, typed-nil, struct value, nil, wrapper-over-nil}, since that table would have caught all three panics.
  • NewProvider validates Name only for emptiness. resourceNameRE and the .. check live in RetrieveCredential, so a malformed resource name fails on every request inside a RoundTripper instead of once at wiring time. Both are in package gcp, so validating in the constructor costs nothing. A misspelled collection (authProvidrs) is the awkward case — it passes the charset filter and silently routes to Agent Identity.
  • Errors come back unwrapped. resolveClient returns r.Err and ctx.Err() bare, and Credential passes resolveClient's error straight through, so a DeadlineExceeded out of Credential gives no hint whether it came from client init or the credential fetch.
  • UserID is now an ambient assertion. Previously a caller passed it explicitly to RetrieveCredential. Now any request whose context descends from an invocation is signed as that session's user, with no call site naming them. Authorization is the service's job, but the docs on NewProvider and agent.Identity should say that binding session.UserID() to an authenticated principal is a precondition.
  • TestResolveClientHonorsCallerDeadline asserts elapsed > time.Second against a 50ms deadline. A regression to a 900ms floor would pass.
  • IdentityFromContext can return ok=true with an empty UserID, which the doc does not mention. The provider handles it, the next caller may not.

All line references above are pinned to 458290d41b04b4a140e683ad88390bd102a76265.

@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch from 458290d to 37a67ca Compare August 18, 2026 20:27
@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch from 37a67ca to d438f23 Compare August 19, 2026 21:44
Base automatically changed from wolo/auth-gcp-client to main August 19, 2026 21:54
@wolo-lab
wolo-lab force-pushed the wolo/auth-gcp-provider branch from d438f23 to 7e64124 Compare August 19, 2026 21:54
Add gcp.NewProvider, an auth.CredentialProvider that resolves per-user
credentials from the Agent Identity / IAM Connector services (via the REST
client) and maps them to an auth.Credential. It takes the acting user from the
ADK context at resolve time, so it runs inside an agent invocation and needs no
per-user configuration.

To recover the user from an http.RoundTripper that only sees a context.Context
(deep beneath a tool call, past jsonrpc2/net/http wrapping), this also adds
agent.FromContext(ctx) (ReadonlyContext, bool): ADK contexts register a
read-only view of themselves under a private key, and FromContext returns it.
Identity still lives on the typed context via Session(); nothing is stored under
a key.

Additive and non-breaking: the only new public API in agent is FromContext; the
Value overrides are on unexported/internal context types and change behavior
only for the new private key. No new module dependencies (the GCP client is
hand-rolled over net/http + ADC).
…ontext

FromContext/RequireContext returned a full agent.ReadonlyContext just so the
GCP credential provider could read UserID. That forced a readonlyView
laundering type (13 method forwarders) plus anti-widening + reflection tests,
all existing only to stop the recovered context from being widened back to a
mutable Context/InvocationContext.

Return a small immutable agent.Identity{UserID,AppName,SessionID} via
IdentityFromContext/RequireIdentity instead. Returning a value erases the
widening concern entirely: readonlyView and the widening tests are gone.
Rename the context key SelfKey -> IdentityKey to match what it now carries.

All of this API is new in this PR (absent from main), so nothing released
changes; ReadonlyContext/Context/InvocationContext are untouched.
- resolveClient waits on the caller's context (DoChan), so a slow cold
  start no longer outlives the request that triggered it; auth.Transport
  resolves a credential per outbound request, so one stalled init used to
  stall them all.
- Drop clientInitTimeout: it bounded nothing. FindDefaultCredentials
  reads the credentials file with os.ReadFile and probes with the
  context-free metadata.OnGCE(), so the bound has to live on the waiting
  side.
- Reject a ProviderConfig.Client that did not come from NewClient; a zero
  value used to nil-deref inside net/http on first use.
- Split the two identity failures and export ErrNoActingUser: "must run
  within an agent invocation" was misleading for an invocation whose
  session carries no user.
- commonContext.Value no longer panics on a nil embedded context or a
  typed-nil Session, which its own doc already promised.
- Say what the identity key does and does not guarantee: the key cannot
  be named outside the module, but the agent.Identity it addresses can be
  read or substituted by any in-process context wrapper.
- NewProvider's doc no longer points at remoteagent, which has no wiring
  for a CredentialProvider, and states the requirement it actually has:
  every authenticated request must descend from the invoking user.

Tests: two users through one shared provider (the property that matters
for a long-lived provider), scopes/continueUri on the wire, the scope
clone, the ADK-identity guard (previously green with the guard deleted),
the lazy ADC path end to end, and the caller-deadline bound.
…t retryable

Value's session guard used reflect.Value.IsNil, which panics on a struct
value — a shape a six-accessor interface like session.Session invites — and
the two Value implementations disagreed: one rejected a typed-nil session,
the other passed the interface-nil check and dereferenced. Both land inside
an http.RoundTripper, where net/http does not recover. One shared kind-aware
check (adkcontext.Usable) fixes both and keeps them in step. A session whose
own accessors panic is out of reach of any nil test, so the docs now say what
holds instead of promising more.

resolveClient bounded each waiter but nothing bounded the init: singleflight
holds the key until the function returns, and FindDefaultCredentials ignores
its context, so a hung ADC lookup wedged the provider for the process
lifetime. Replaced with the shape lazyTokenSource uses — one shared attempt,
retired after initTimeout so the next call retries.

NewClient also ran on the winning caller's context, so the process-lifetime
client inherited one request's oauth2.HTTPClient and pinned its whole context
graph. NewProvider now takes the context to root it with, matching
gcp.NewClient; context.Background() is not available to library code here.

Also: validate the resource name at wiring time rather than on every request,
name the user and resource on a failed retrieval (as adk-python does), rename
Scheme to ProviderScheme so it stops colliding with the HTTP auth scheme, and
correct doc.go, which still said this package holds only the client.

Tests: the four legal session shapes across both Value paths (mutation-checked
against the original guards), unknown-key delegation, both identity failures
against a server that fails if called, a failed init retried, a hung init
retired, and a tighter caller-deadline bound.
…okup per timeout

A /review_max pass over the previous commit found the identity path still
panics on shapes the nil check waves through, and inherits an identity it
should not.

Identity resolution is now fail-closed and panic-free. The kind-aware nil
check could not see a session wrapping a nil session — the shape
llmagent.newWrappedSession produces for a nil original — and it wrongly
rejected a typed-nil session whose accessors never dereference. It is
replaced by one shared read that recovers: a session that cannot answer
costs the identity, not the process, which matters because this runs inside
an http.RoundTripper where net/http does not recover. The session is read
once instead of four times, so the identity cannot tear and the tool-context
wrappers stop logging on every outbound request.

An invocation now answers the identity key itself even when its session
cannot be read, rather than delegating up the chain. A nested invocation
without a session used to report the enclosing one's user, so a credential
would be minted for a user who made no such call. agent.invocationContext
gained the same override; it was the third implementation of the invariant
the comment claimed was held.

runInit no longer abandons a hung lookup. Retiring it started a fresh one
every 30s, each parked in a syscall pinning an OS thread — measured at 20
leaked goroutines over 20 calls. The waiters are bounded instead, and a
lookup that eventually lands still publishes its client.

Also: fold Scheme into ProviderConfig, so there is one place for the next
knob rather than two; reject a resource name that is neither an authProvider
nor a connector, and one whose normalization would route elsewhere than the
name that was validated; keep the wiring context only when it will be used;
reject a (nil, nil) client instead of caching it; attribute failures to the
session rather than the user, whose id is commonly an email and whose error
text reaches the model; and keep the consent URI, which carries the state and
nonce, out of the error string that is persisted in the session.

Tests: concurrent callers share one client flight (a no-op mutex was green
before), concurrent two-user credential resolution, identity across
WithContext and a nil embedded parent, the nested-invocation guard, the
session shapes that panicked, and a (nil, nil) client rejected. The three
previously surviving mutants are killed.
… to the owner

A second /review_max pass caught two defects in the previous commit's own
fixes.

The recover did not cover the call it existed for. identityOf took a
session.Session, so Session() — itself a method on caller-supplied code —
was evaluated as the argument, outside the recover. The repo's own exported
agent.StrictContextMock panics there. It now takes a getter, called inside.

Fail-closed was applied one level too high. commonContext does not own a
session; it reads one off the invocation it wraps, and a tool or callback
context returns nil there by design. Refusing to delegate therefore broke
identity for every context derived from one of those — ErrNoActingUser on
each outbound request. The guard belongs on the two types that own a session
field, which answer for themselves and never inherit an enclosing
invocation's user; a wrapper defers to what it wraps.

runInit publishes from a defer, so a builder that panics or calls
runtime.Goexit releases the waiters and the in-flight slot instead of leaving
pending set with its goroutine dead. The panic value is reported as that
attempt's error rather than killing a process that an eagerly built client
would only have panicked inside.

The init timeout gets its own sentinel, ErrClientUnavailable: it previously
wrapped context.DeadlineExceeded, which is what the caller-deadline arm
returns, so the two were indistinguishable by anything but their message.

Error attribution names the resource and nothing else. The session id it
named before arrives unvalidated from the request path, so swapping the user
id out for it moved the injection vector rather than closing it; the text is
fed to the model and persisted.

Also: ReadIdentity became the generic Recovered, returning the value instead
of filling one in by closure; the redirect requirement no longer claims
NewClient covers a caller-supplied HTTPClient; and ProviderScheme.Name labels
the two-shape check as stricter than Client and than adk-python.

Tests: the panicking Session(), tool and callback contexts resolving through
a wrapper, the owner's guard on both implementations, a late-landing client
published rather than rebuilt, and an abrupt builder not wedging the
provider. Eight mutants covering every guard added in this and the previous
commit are killed.
…e package prefix

The resource charset omitted the colon, so a domain-scoped project id —
projects/example.com:my-project/... — was rejected outright. The name is
always appended after the endpoint and a /v1 segment, so it can never be read
as a scheme; re-fuzzed the URL-safety property over the widened class (1.95M
executions, no failures), and the empty-segment check still blocks the
authority form.

Every provider-internal wrap prefixed "gcp:" and then went through attribute,
which prefixes it again, so a client-init failure printed the package three
times. The prefix now lives only where the error leaves the package.

Also record what a recovered read costs: a bug inside a caller's session
accessor surfaces as a missing identity rather than a stack trace.
…nt context

A third /review_max pass found the fail-closed rule could be sidestepped
through the public API: a context promoted from a session-less invocation and
then reparented onto a plain context carrying another invocation reported
that other user, because the identity key fell through to the embedded
parent.

commonContext now asks the invocation it speaks for and takes that answer as
final. A tool or callback context keeps working — theirs return a nil session
by design and delegate the key to the context underneath — while a
session-less invocation's refusal stays authoritative, whatever the derived
context is later attached to. Asking the invocation first also stops the
tool-context wrappers logging "Session() is not supported" once per outbound
request, since their session is no longer read at all. An InvocationContext
from outside the module need not answer the key, so a recovered session read
remains as a fallback.

Passing an InvocationContext to WithContext still rebinds which invocation
the context speaks for. That is deliberate, and the test says so.

Two smaller things the same pass found: the empty-UserID error named the app
and session, which are caller-supplied and reach the model, and a client-init
failure was attributed to a resource it has nothing to do with — it is about
this process's own credentials, and attributing it also stacked the package
prefix on ErrClientUnavailable.

Tests: the reparenting bypass, the delegation exercised through a
session-less wrapper in every derivation (three of the previous cases read
the session directly and would have passed either way), the nil-invocation
guard, percent-escaped resource names, and the documented contract that the
wiring context's cancellation is not honored. Seven mutants killed; hoisting
the session method value out of the recover is an equivalent mutant — binding
one on a non-nil interface does not dereference.
… the error prefixes

Two regressions from the previous commit, found by a delta review of it.

The invocation's answer was accepted on `!= nil` alone. An InvocationContext
with a permissive Value — a decorator, or a test double that answers every key
— then handed back something that is not an Identity, which swallowed the
session read that would have resolved it. Fails closed, so no wrong user is
ever served, but the cost is ErrNoActingUser on every outbound request from
such a context. Type-asserting the answer restores the fallback.

Removing attribute from the client-init path left four of the eight Credential
error paths bare: the previous commit had stripped `gcp:` from five
constructions on the invariant that attribute re-added it. They carry their
own prefix again.

Also narrow two doc claims to what actually holds. An invocation implemented
outside this module cannot promise never to report an enclosing invocation's
user, because the key is unnameable there and its embedded parent answers
instead; and WithContext rebinds which invocation a context speaks for on the
two implementations that do so, not on all five.

The delta review measured the change this corrects across 12,525 three-deep
context chains: 566 differed from the previous commit, none went from
resolving to lost and none gained a foreign user, it closes a cross-user leak
on healthy invocations, and it repairs an identity loss on a callback context
reparented around a tracing span — a shape production builds every turn.
Resolution is ~35x faster (4322 -> 125 ns/op), since a recovered nil-session
panic per wrapper level is gone.
…nherited

A final review of the whole change, rather than of the last delta, found a
confused deputy the delta reviews could not see.

commonContext asked the wrapped invocation's Value first and took any Identity
it got back as final. An InvocationContext written outside this module embeds
the context it was derived from, to inherit cancellation, and cannot override a
key it cannot name — so its Value answers with the enclosing invocation's
identity even when it has a session of its own naming a different user. An
invocation whose session says bob was served alice's credential through
Promote, NewToolContext and NewCallbackContext alike, using only the public
surface. agent.StrictContextMock forwards Value to its parent the same way, so
the shape exists in this repo too.

Reading the session first is what makes an invocation report itself. Only an
invocation with no readable session of its own delegates now, which is what a
tool or callback context needs and all it needed. The earlier ordering was
chosen on the belief that this was inherent to a type that cannot name the key.
It is not: the type knows its own session, and that is the authoritative
answer.

Two more from the same review. A stuck credentials lookup cost every outbound
request the full 30s bound, because the attempt is deliberately kept running
and nothing recorded that its bound had already blown — measured five callers
each paying it in full. The first waiter to time out now latches it and later
ones fail fast until the attempt lands. And discovery failing outright, the
common case, carried no sentinel at all while the rarer timeout did, so a
caller behind a RoundTripper had to match on a message; both are
ErrClientUnavailable now.

Also: the consent error is wrapped by the time a caller sees it, so the
CredentialProvider doc no longer implies a type assertion will find it.

Tests: the decorated-invocation confused deputy, the latched bound, and the
sentinel on discovery failure. All three mutants die.
… reads

The two sentinels that name the resource themselves are always wrapped by
attribute, which names it too, so a consent rejection printed a 39-character
resource name twice into text that is fed to the model and persisted in the
session. Every caller of RetrieveCredential supplied the resource, so the
sentinel does not need to repeat it back.

The remaining doubled `gcp:` is ordinary wrapping within one package, and Client
keeps its own prefix because it is usable on its own.

Also two documentation slips from earlier edits: TestProviderErrorAttribution's
comment had come to sit above TestProviderCredentialConcurrent, still describing
the invocation id that attribute deliberately stopped naming, and a paragraph in
IdentityFromContext's doc was broken mid-sentence.
…iting one

Found by tabulating the identity decision procedure instead of reviewing it a
diff at a time. An invocation whose Session() panics fell through to asking what
it embeds, so it answered with the enclosing invocation's user — the same
fail-open as the decorated-invocation case, one row over. agent.StrictContextMock
is exactly this shape: Session() panics and Value forwards to its parent, so a
fake built as its own doc instructs, nested under a real invocation, served that
invocation's user.

An invocation that cannot answer is now distinguished from one that has nothing
to answer with. No session of its own still delegates, which is what a tool or
callback context needs. A session that panics, or one present but unreadable,
reports nothing: broken is not absent, and the context a decorator embeds belongs
to a different call.

The table is committed as TestIdentityDecisionMatrix — 12 session and invocation
shapes crossed with 9 derivation paths, 103 cells. It is what should have been
written when this procedure was first touched: reviewing it one diff at a time
found one failing shape per round across five rounds, each fix moving the failure
to a neighbouring shape, and every one of those shapes is a row here.

Crossing the two axes is the part that pays. An invocation that owns a session
fails closed on its own, so an unreadable session only reaches the delegation
through a decorator — walking each axis separately leaves that cell untested, and
removing the guard for it keeps a suite of ninety cells green.
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.

2 participants