feat(agent-platform): add version-tolerant kagent client - #1998
Conversation
kagent sessions live in kagent's Postgres and are served over the controller's HTTP API, so unlike agents and model configs they are not Kubernetes resources and the Kubernetes proxy the rest of the plugin uses cannot reach them. This adds the transport the upcoming Sessions list needs. A backend hop is unavoidable: the browser cannot call kagent.<baseDomain> cross-origin, and the user's per-installation Dex ID token has to become the Authorization header toward kagent (inbound, that header carries the Backstage identity). Same approach as muster-backend, via a separate backstage-kagent-authorization header. The base URL is derived per installation as https://kagent.<baseDomain>/api -- the oauth2-proxy-fronted host whose nginx sidecar proxies /api/ to kagent-controller:8083 -- and is overridable per installation, which also acts as an allowlist since kagent is only deployed on some installations. Responses pass through verbatim: the {error, data, message} envelope is not unwrapped and unknown fields are not stripped, so kagent schema drift is absorbed by the frontend client instead of needing a backend release. The split is backend = transport, frontend = schema. All config keeps the default backend visibility; apiBaseUrl embeds baseDomain, which deanonymizes customers, so /kagent/installations returns names only.
…-read failures Addresses review feedback on #1997. Both findings verified against the kagent source and the connectivity chart. Remove the /version probe entirely. kagent serves /version at its server root, and neither door we support routes the root to the controller: the derived door's nginx sidecar proxies only `location /api/` to kagent-controller:8083 while `location /` goes to the kagent UI (which answers HTML, so our own non-JSON guard reported a sign-in page on healthy installations), and the agentgateway override matches only the /kagent prefix so a root-relative /version never matches its HTTPRoute. Nothing under /api exposes the controller version either -- the Version fields in /api/substrate/status are per-actor, not the controller's -- so there is nothing reachable to probe. Version tolerance is unaffected: it lives in the frontend's permissive parsing, which is where it does the real work. A test now asserts every request stays under /api, and another that the route is gone. Map failures while reading the response body. The try/catch only covered fetch(), but the abort signal stays armed after the headers arrive, so a mid-stream abort, a connection reset, or truncated/invalid JSON escaped untyped and surfaced as a generic 500 instead of the 503 the frontend keys off for "kagent unreachable". Reject a non-absolute apiBaseUrl at startup with one clear message, rather than letting new URL()/fetch fail opaquely on every request.
The data layer behind the upcoming Sessions list. No visible change yet. kagent ships no OpenAPI spec, GS pins v0.9.9 while upstream is on v0.10.0-beta9, and every installation is an independent deployment -- so the fleet can run several kagent versions at once. Everything here is keyed per installation and defensive by default. Wire/domain boundary: responses are parsed with permissive zod schemas and normalized into a stable KagentSession, so a schema change is absorbed in one function rather than rippling through components. Rows are validated one at a time, so a single malformed entry is skipped instead of costing the whole list. Envelope drift is tolerated (data absent per Go's omitempty, data: null, or a bare array), and Go zero time -- which browsers render as "Dec 31, 0000" -- is dropped along with anything unparseable. Capability negotiation: a cached /version probe per installation yields named flags, so components never compare versions inline. The supported window is explicit; below the floor still renders, above the ceiling proceeds optimistically and logs once, and a failed probe degrades to the oldest supported version without ever blocking the session query. A /me probe adds isUserScoped, because kagent's unsecure auth mode ignores the forwarded token and resolves every caller to a shared user -- worth detecting rather than mislabelling the list as the user's own. The client mints each installation's Dex token lazily per request so a mint failure degrades one installation rather than the page, and never caches tokens: the plugin's query cache is persisted to localStorage, which is no place for a credential. The token-minting sequence moves out of useDeployAgent so the deploy flow and this client mint identically. Version-matrix fixtures -- including a captured live v0.9.9 response -- assert that v0.9.9 and v0.10 normalize to identical output and that a synthetic future version with unknown fields changes nothing. Those fixtures immediately earned their keep by catching a zod subtlety: wrapping z.unknown() in a transform makes the field required again, which had been failing every row that omits a key -- which is most real rows.
marians
left a comment
marians
left a comment
There was a problem hiding this comment.
Careful pass over the whole diff. The wire→domain layer is solid — I replicated the zod schemas against zod 4.4.3 and confirmed every documented tolerance actually holds (missing keys, null rows, arrays-as-rows, data absent/null/non-array, bare top-level array, and the unknown-in-transform-needs-.optional() subtlety). I also verified the external facts the design rests on against kagent itself: /version returns the bare VersionResponse (no envelope) and /api/me returns claims-or-{sub} in both v0.9.9 and v0.10.0-beta9, APIPathMe exists in v0.9.9, and admin@kagent.dev is genuinely UnsecureAuthenticator's fallback user ID. No issues found in kagentSchema.ts's parsing, normalizeSessionList's drift logic, normalizeTimestamp, capabilitiesForVersion's semver gating, installationOidcToken.ts, or the plugin.tsx wiring.
7 findings inline, none blocking. The two I'd actually act on before the UI lands are the retry: 1 override (it silently undoes the fail-fast contract that handleResponse's error names exist to provide, and 404/503 is the common case across the fleet) and the unmapped 400 (the backend throws InputError for an installation outside its kagent allowlist, which the sessions provider would then surface as an error rather than "not deployed here"). The rest are smaller: the unconditional token mint making the backend's optional-token probe path unreachable, isError/message parsed then discarded, an array identity in the memo deps that defeats the signature it sits next to, and isUserScoped collapsing "unknown" into false.
One thing outside this PR's diff, but it decides whether the capability negotiation works at all: KagentClient.getVersion resolves new URL('/version', apiBaseUrl), i.e. https://kagent.<baseDomain>/version at the host root. If the agentic-platform-connectivity nginx sidecar only proxies /api/ to kagent-controller:8083, that request never reaches kagent and every installation permanently degrades to FALLBACK_KAGENT_CAPABILITIES — silently, since the fallback is by design indistinguishable from a healthy 0.9.9. Worth confirming against a live installation before the Sessions UI starts gating on these flags.
| queryKey: kagentVersionQueryKey(installation), | ||
| queryFn: () => kagentApi.getVersion(installation), | ||
| staleTime: VERSION_STALE_TIME_MS, | ||
| retry: 1, |
There was a problem hiding this comment.
retry: 1 on both probe queries (here and L64) replaces the QueryClientProvider retry predicate rather than tightening it, which defeats the short-circuit KagentApiClient.handleResponse exists for.
The predicate in components/QueryClientProvider.tsx returns false for NotFoundError / ServiceUnavailableError precisely so that "kagent isn't deployed on this installation" fails fast. Since kagent is only on a couple of installations, 404/503 is the normal outcome for most of the fleet — and with retry: 1 each of those now costs a second request after a retryDelay of 1s, per probe, per installation. On a 20-installation fleet with kagent on 2 that's ~36 extra doomed round trips per page load, each one also re-running getInstallationOidcToken (a broker token exchange).
Dropping retry here inherits the predicate; if you want a lower ceiling than the default failureCount <= 2, pass a predicate that keeps the name check.
| (errorData as { error?: { message?: string } })?.error?.message ?? | ||
| `kagent request failed with status ${response.status}`; | ||
| const error = new Error(message); | ||
| if (response.status === 401) { |
There was a problem hiding this comment.
400 is unmapped, and the backend produces it on a path this client can actually hit.
agent-platform-backend's resolveInstallation throws Backstage InputError (→ 400) twice: when ?installation= is missing, and — the interesting one — when the name is not in the configured kagent allowlist (Unknown kagent installation '<name>'). The allowlist is gs.installations entries with a baseDomain, or the explicit agentPlatform.kagent.installations block, which is not the same set as "installations the frontend considers reachable".
So if useKagentCapabilitiesMap (or the upcoming sessions provider) is handed anything wider than listInstallations() — e.g. useReachableInstallations() output — those installations return 400, which becomes a plain Error. Two consequences: the retry predicate retries it 3× with exponential backoff instead of failing fast, and the doc comment right above says everything that isn't 404/503 is "we couldn't read it, surfaced" — so a misconfigured/unlisted installation will render as a user-visible error rather than "kagent isn't deployed here".
Mapping 400 to a distinct name (or at least to a non-retryable one) would make this classifiable.
| const headers: Record<string, string> = {}; | ||
| if (installation) { | ||
| url.searchParams.set('installation', installation); | ||
| headers[KAGENT_AUTH_HEADER] = await getInstallationOidcToken( |
There was a problem hiding this comment.
The token is minted unconditionally for every installation-scoped call, which makes the backend's deliberate "token optional for /version and /me" path unreachable.
router.ts in #1997 reads the token with { required: false } for both probes, with the comment: "kagent's /version is unauthenticated in-process … and a token-mint failure never turns the probe into a hard 401". But get() mints before fetching whenever installation is set, so a mint failure (expired main Dex session, broker_unreachable/session-expired from GSAuthProviders.createClusterTokenProvider, or an installation whose oidcTokenProvider isn't wired) rejects the probe without any request reaching the backend.
Scenario: the broker exchange fails for one installation. /version would have answered fine unauthenticated, but instead capabilitiesForVersion never runs, that installation silently degrades to FALLBACK_KAGENT_CAPABILITIES, and isUserScoped stays undefined — i.e. the exact diagnostics this PR adds go dark for the installation that most needs them.
Suggest a per-call requireToken flag (true for /sessions, false for /version and /me, where a failed mint just omits the header).
| .transform(envelope => ({ | ||
| rows: envelope.data ?? [], | ||
| hadDataArray: envelope.data !== undefined && envelope.data !== null, | ||
| isError: envelope.error === true, |
There was a problem hiding this comment.
isError and message are extracted here but never read — normalizeSessionList only ever touches parsed.data.rows and hadDataArray.
That turns an in-band error into a silent empty list: a 200 response carrying { "error": true, "message": "…", "data": null } normalizes to { sessions: [] } with no drift at all, so the UI shows "no sessions" and nothing is logged. The backend can't cover this for you — KagentClient.request only classifies on HTTP status and returns any 2xx body verbatim, exactly so the envelope stays the frontend's problem.
Either fold isError into the drift result (drift: message ?? 'kagent reported an error in the envelope') or drop the two fields from the transform so it's clear nothing checks them.
|
|
||
| return result; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [installations, signature]); |
There was a problem hiding this comment.
The comment at L68-70 says the memo "keys off a stable signature of the outcomes rather than the array identities", but installations — an array identity — is still in the dep list, so the signature buys nothing when the caller derives the array inline.
useReachableInstallations happens to return a memoized array, but the natural call site is something like useKagentCapabilitiesMap(installations.filter(…)) or an intersection with listInstallations(), which is fresh every render. Then byInstallation is a new Map each render, useCallback hands back a new capabilitiesFor identity each render, and any consumer memo/useEffect keyed on it re-runs — including, potentially, a sessions useQueries built from it.
The established pattern in this plugin is useReachableInstallations, which keys on allInstallations.join(',') and explicitly excludes the array identity. Same fix here: [signature] alone already encodes the installation names.
| */ | ||
| const UNSCOPED_SUBJECTS = ['admin@kagent.dev']; | ||
|
|
||
| function isUserScopedSubject(sub: string | undefined): boolean { |
There was a problem hiding this comment.
Returning false for a missing sub conflates "we couldn't tell" with "confirmed shared built-in user", and the caller has already spent undefined on "still loading" — so there is no value left to mean "unknown".
This is reachable without kagent being in unsecure mode. HandleGetCurrentUser returns principal.Claims verbatim when claims exist (only falling back to {sub: principal.User.ID} when they don't), so /api/me under trusted-proxy returns whatever the IdP put in the token. And getIdentity maps any unparseable body to { sub: undefined } and resolves — so status === 'success', isUserScoped === false.
Net effect: an odd-but-working deployment gets whatever "these are not your sessions" affordance the next PR renders, which is the mislabelling this probe was added to prevent. Consider a tri-state (true / false / undefined for "no subject reported") so the UI can stay silent when it doesn't know.
There was a problem hiding this comment.
Fixed in 7424430 — isUserScoped is now genuinely tri-state.
I'd reasoned that false was the safer default ("wrongly claiming scoping is the more misleading error") but you've identified the flaw: undefined was already spent on "loading", so there was no value left for "can't tell", and the two collapsed into the alarming one.
I checked the reachability claim in kagent's source and it holds — HandleGetCurrentUser responds with principal.Claims verbatim whenever claims exist, only falling back to {sub: principal.User.ID} when they don't. So under trusted-proxy the body is whatever the IdP emitted, and a deployment configured with a different userIdClaim, or an IdP that doesn't emit sub, legitimately yields no subject. Combined with getIdentity resolving on an unparseable body, that meant status === 'success' and isUserScoped === false on a perfectly healthy install — the exact mislabelling the probe was added to prevent.
isUserScopedSubject now returns boolean | undefined, the type documents all three states, and the contract for PR 3 is explicit: stay silent on undefined, warn only on an explicit false.
| ) { | ||
| warnedAboveCeiling.add(installation); | ||
| // eslint-disable-next-line no-console | ||
| console.warn( |
There was a problem hiding this comment.
Minor: this is a side effect inside useMemo, guarded by module-level mutable state.
useMemo callbacks aren't guaranteed to run once per logical update — React may discard and recompute them — so the "once" property here rests entirely on warnedAboveCeiling, which is module-global and never cleared. Two consequences worth a thought: the warning is suppressed for the whole page session even across full app remounts (an installation upgraded past the ceiling mid-session warns once and never again, which is fine, but so does a genuinely new occurrence), and reportedDrift in KagentApiClient.ts has the same unbounded-growth shape with a key that embeds a varying row count (skipped 2 unreadable session rows), so it dedupes less than it looks like it does.
An useEffect keyed on signature would put the logging where effects belong and let React own the once-per-commit guarantee.
There was a problem hiding this comment.
The specific code here is gone — the above-ceiling warning went with the version flags when /version turned out to be unreachable through either door (see #1997), so there's no longer a side effect inside this useMemo.
The second half of your point outlived it though, and I've fixed that: reportedDrift in KagentApiClient.ts had the same unbounded-growth shape with a key embedding a varying row count. It now keys on a closed set of drift kinds rather than the formatted message, so the set is bounded by installations × kinds and recurring drift warns once. Details in that thread.
Taking the general note too: no useMemo in this hook performs side effects any more, so React is free to discard and recompute them without observable consequences.
Follows the removal of the unreachable /version probe in the backend (review feedback on #1997). kagent's /version sits at the server root, which neither supported door proxies to the controller, and nothing under /api reports the controller version -- so there is no version to negotiate on. Removes getVersion from the client, the version wire schema, the semver dependency, and the three version-derived flags (hasSessionShares, canRenameSessionViaPatch, hasSessionReadOnly) along with the MIN_SUPPORTED / TESTED_UP_TO window. All three flags were unused -- nothing in the Sessions list consumes them -- so this removes speculative code rather than a feature. Capabilities keep the one thing we can actually observe: isUserScoped, from the /me probe, which matters because kagent's unsecure auth mode would otherwise present a shared session list as the user's own. Version tolerance is untouched. It lives in the permissive parsing layer and its version-matrix fixtures, which is where it does the real work; version detection was the speculative part. If gating is ever needed, probe by behaviour (call a version-specific endpoint and treat 404 as absent) rather than by version string.
f8a1e5c to
e66cf73
Compare
|
Rebased onto the updated #1997 and dropped the version-derived capability flags, following the review finding there that kagent's Removed: Kept: Version tolerance is unaffected, which is worth being explicit about given it was the headline of this PR: it lives in the permissive parsing layer and its version-matrix fixtures — v0.9.9 and v0.10 payloads still normalize to identical output, unknown fields and unknown 177 tests pass, |
marians
left a comment
marians
left a comment
There was a problem hiding this comment.
Re-reviewed after e66cf73 (drop version-derived capability flags).
The version probe removal is the right fix, and it's clean: no dangling references to capabilitiesForVersion, getVersion, kagent_version, isAboveTestedCeiling, the three flags, or the MIN_SUPPORTED/TESTED_UP_TO window anywhere in either plugin (the remaining semver hits are the unrelated Flux OCIRepository range in composeManifests.ts). Changeset rewritten coherently, dep dropped from package.json + yarn.lock, and router.test.ts now pins that /kagent/version 404s. Keeping isUserScoped as the one observable capability, plus the "probe by behaviour, treat 404 as absent" note for any future gating, is the durable version of this.
Two earlier comments are resolved by the removal:
- The
console.warnside effect insideuseMemoguarded by the never-clearedwarnedAboveCeilingmodule-global — gone with the version code. isUserScopedSubject(undefined) === false— now an explicit, documented, and tested decision rather than an implicit one. Fair call; withdrawn.
Six from the first pass still stand, re-anchored below to the current lines. All non-blocking.
One nit outside this diff, in #1997's territory: KagentClient.ts:39 still documents the token as "Optional because /version and /me are useful even when no token could be minted" — /version no longer exists, so only /me is left to justify it.
| queryKey: kagentIdentityQueryKey(installation), | ||
| queryFn: () => kagentApi.getIdentity(installation), | ||
| staleTime: IDENTITY_STALE_TIME_MS, | ||
| retry: 1, |
There was a problem hiding this comment.
Still stands after the version query's removal, halved in cost but unchanged in kind: retry: 1 replaces the QueryClientProvider retry predicate rather than tightening it, so NotFoundError/ServiceUnavailableError no longer short-circuit.
kagent is deployed on only a couple of installations, so 404/503 is the normal outcome for most of the fleet — and each probe now pays a second doomed request after a 1s delay, plus a re-run of the broker token exchange in getInstallationOidcToken.
retry: (failureCount, error) =>
failureCount < 1 && !['NotFoundError', 'ServiceUnavailableError'].includes(error.name),or drop retry entirely and let the provider's predicate apply.
There was a problem hiding this comment.
Fixed in 7424430 — dropped the retry override entirely so the provider predicate applies.
I'd not appreciated that a per-query retry replaces the default rather than composing with it, which is exactly backwards from what I wanted: the predicate in QueryClientProvider.tsx exists to make NotFoundError/ServiceUnavailableError fail fast, and overriding it re-enabled retries for precisely the case it was written to short-circuit. Your fleet arithmetic is the part that makes it matter — 404/503 is the normal outcome on most installations, so the cost scales with the fleet rather than with anything going wrong, and each doomed retry re-runs the broker token exchange.
I went with dropping retry rather than the composed predicate you offered, since failureCount <= 2 is a reasonable ceiling for genuinely unknown errors and one fewer place to keep the name list in sync. There's a comment at the call site explaining why there's deliberately no override, so it doesn't get "helpfully" re-added.
| if (response.status === 401) { | ||
| error.name = 'UnauthorizedError'; | ||
| } | ||
| if (response.status === 403) { | ||
| error.name = 'ForbiddenError'; | ||
| } | ||
| if (response.status === 404) { | ||
| error.name = 'NotFoundError'; | ||
| } | ||
| if (response.status === 503) { | ||
| error.name = 'ServiceUnavailableError'; | ||
| } |
There was a problem hiding this comment.
400 is still unmapped, and the backend does throw it: resolveInstallation raises InputError (→400) for an installation it has no client for (router.ts:123). That set is gs.installations entries with a baseDomain, which is not the same as the frontend's "reachable" set — so this is reachable in normal operation, not just on a coding error.
When it happens the failure becomes a plain Error: retried 3× with backoff by the provider predicate, and — per the doc comment right above this block — classified as "we couldn't read it" (surfaced to the user) rather than "kagent isn't deployed here" (silent). Mapping 400 to NotFoundError, or adding a distinct name the sessions provider treats as not-deployed, would put it on the silent path where it belongs.
There was a problem hiding this comment.
Fixed in 7424430 — 400 now maps to NotFoundError.
The distinction you drew is the one I'd missed: the backend's allowlist (gs.installations entries with a baseDomain, or the explicit block) is not the frontend's reachable set, so an unlisted installation is ordinary operation rather than a coding error. Left unmapped it got the worst of both — retried three times with backoff, then surfaced to the user as a read failure, when the honest reading is "kagent isn't available here".
I chose NotFoundError over a new distinct name because it puts the case on the existing silent path with no further wiring, and it matches the backend's own 404 wording. The trade-off I'm accepting: a genuinely missing ?installation= would also be 400 and would now be swallowed silently. That's tolerable because the client always sets the parameter whenever an installation is passed, so the unknown-installation branch is the only reachable 400 — but it's noted in a comment so the assumption is visible if that ever changes.
| if (installation) { | ||
| url.searchParams.set('installation', installation); | ||
| headers[KAGENT_AUTH_HEADER] = await getInstallationOidcToken( | ||
| this.kubernetesApi, | ||
| this.kubernetesAuthProvidersApi, | ||
| installation, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The token is minted unconditionally whenever installation is set. The backend reads it for /kagent/me with readUserToken(req, { required: false }) (router.ts:173) — and now that the version route is gone, /me is the only route with that relaxation, so this client makes the deliberate optional-token path entirely dead code.
The cost is where it hurts: a broker or session failure rejects the capability probe before any request is sent, so the installation silently degrades to FALLBACK_KAGENT_CAPABILITIES with isUserScoped undefined. That's the one case where the answer matters most — an unsecure-mode deployment showing a shared session list as "your sessions" — and the diagnostics go dark exactly there.
A per-call tokenRequired flag (or .catch(() => undefined) on the mint for the probe paths only) would let /me reach the backend token-less and still report a subject.
There was a problem hiding this comment.
Fixed in 7424430 — added a per-call token requirement, false for /kagent/me.
You're right that this made the backend's relaxation dead code, and the consequence you point out is the sharp bit: the probe would fail before any request was sent, so the installation degraded to unknown capabilities exactly when the answer mattered most — an unsecure deployment presenting a shared session list as the user's own. Losing that diagnostic to a broker hiccup is precisely the wrong failure mode.
getIdentity now mints best-effort and omits the header when the mint fails, so the request still reaches the backend token-less. /sessions keeps a required mint, since without a token kagent has no subject to scope on. A test asserts the token-less request goes out and still returns a subject.
| isError: envelope.error === true, | ||
| message: | ||
| typeof envelope.message === 'string' ? envelope.message : undefined, |
There was a problem hiding this comment.
isError and message are extracted here and in the bare-array branch, but nothing reads them — normalizeSessionList only uses rows and hadDataArray.
So a 200 carrying {"error": true, "message": "...", "data": null} normalizes to an empty session list with no drift note and no log: indistinguishable from "this user has no sessions". Either feed them into the drift path (drift: message ?? 'error envelope') or drop the two fields so the schema doesn't imply a tolerance it doesn't have.
There was a problem hiding this comment.
Fixed in 7424430 — isError now feeds the drift path.
This was the most consequential of the batch: I'd extracted the two fields and then never read them, so a 200 carrying {"error": true, "data": null} normalized to {sessions: []} with no drift and no log — an empty table that looks exactly like "you have no sessions". And you're right that the backend can't cover it, by its own design: it classifies on HTTP status and returns 2xx bodies verbatim precisely so the envelope stays the frontend's problem.
normalizeSessionList now returns drift: {kind: 'error-envelope', message}, falling back to a generic message when the envelope carries none. Two tests cover it, one from a fixture and one for the no-message case.
| /** Drift is worth knowing about, but only once per message per page-session. */ | ||
| const reportedDrift = new Set<string>(); | ||
|
|
||
| function reportDrift(installation: string, drift: string) { | ||
| const key = `${installation}:${drift}`; | ||
| if (reportedDrift.has(key)) { | ||
| return; | ||
| } | ||
| reportedDrift.add(key); | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `kagent sessions response drift on installation '${installation}': ${drift}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
reportedDrift is module-global and never cleared, so "once per message per page-session" is really once per process — a Backstage tab stays open for days, and a genuinely new drift after a kagent upgrade is reported once and then never again.
The key also undercuts the bound in the other direction: kagentSessions.ts builds drift as skipped ${skippedRows} unreadable session…, so a varying row count yields a fresh key and a fresh warn each time.
Since the values are already surfaced as drift on the return, letting the caller decide (or keying on a stable drift kind rather than the formatted message) avoids both edges.
There was a problem hiding this comment.
Fixed in 7424430 — dedupe now keys on a stable drift kind rather than the formatted message.
Both edges you describe were real and they pulled in opposite directions, which is what made the original wrong in a way that looked fine: skipped ${n} unreadable session rows yields a fresh key per row count, so the same recurring problem re-warned indefinitely, while the set itself grew without bound in a long-lived tab.
normalizeSessionList now returns drift: {kind, message} with kind from a closed set (unparseable-body, error-envelope, data-not-array, skipped-rows), and the client dedupes on installation:kind. That bounds the set at installations × kinds, and keeps recurring drift to one warning. The message is still what gets logged, so the row count is not lost — it just no longer participates in the key.
| // useQueries returns fresh arrays every render, so the memo below keys off a | ||
| // stable signature of the outcomes rather than the array identities. Computed | ||
| // plainly (not memoized) because it is the memo's own dependency and is cheap. | ||
| const signature = installations | ||
| .map((installation, index) => { | ||
| const identity = identityQueries[index]; | ||
| return `${installation}:${identity?.status}:${identity?.data?.sub ?? ''}`; | ||
| }) | ||
| .join('|'); |
There was a problem hiding this comment.
The comment says the memo keys off a stable signature "rather than the array identities", but installations is still in the dep array below (line 73), so the array identity is load-bearing after all.
Any caller deriving the array inline — useKagentCapabilitiesMap(reachable.map(i => i.name)) — rebuilds the Map and the returned callback identity on every render, invalidating downstream memos. useReachableInstallations in this same plugin does it the other way, keying on allInstallations.join(',') and keeping the array out of the deps. Either match that, or reword the comment to say the array identity is intentionally part of the key.
There was a problem hiding this comment.
Fixed in 7424430 — installations is out of the dep array; the memo keys on [signature] alone.
Fair hit: the comment asserted a property the code didn't have. The signature already encodes the installation names, so it was doing the work while the array identity quietly invalidated everything anyway.
Your point about the natural call site is what makes this more than cosmetic — the upcoming sessions provider intersects the reachable set with listInstallations(), which is a fresh array every render, so capabilitiesFor would have had a new identity on each render and any useQueries derived from it would have churned. Now matching useReachableInstallations, which keys on allInstallations.join(',') and keeps the array out of the deps.
Added a test that renders with an inline array literal — my first attempt reused one array instance across rerenders and so proved nothing, which is worth flagging since it's an easy way to write a test that passes for the wrong reason.
Seven findings from review on #1998, all verified against the code and the kagent source before changing anything. Inherit the retry predicate. A per-query `retry: 1` *replaced* the QueryClientProvider predicate rather than tightening it, so NotFoundError and ServiceUnavailableError no longer short-circuited. Since kagent runs on only a couple of installations, 404/503 is the normal outcome for most of the fleet, and each one was paying a second doomed request plus another broker token exchange. Map 400. The backend raises InputError (400) for an installation outside its kagent allowlist, and that allowlist is not the same set as the frontend's reachable installations -- so this happens in ordinary operation. Unmapped it was retried with backoff and classified as "we couldn't read it", i.e. surfaced to the user, when it means "kagent isn't available here". Make the token optional where the backend says it is. /kagent/me is read with `{required: false}`, but the client minted unconditionally, so a broker or Dex-session failure rejected the probe before any request was sent. That silently cost the isUserScoped diagnostic on exactly the installation where it matters most. The mint is now best-effort for that path. Surface in-band errors. `isError`/`message` were extracted from the envelope and never read, so `{error: true, data: null}` on a 200 normalized to an empty list with nothing logged -- indistinguishable from "this user has no sessions". The backend passes 2xx bodies through verbatim by design, so this is ours to check. Give drift a stable kind. Dedupe keyed on the formatted message, but the skipped-rows message embeds a varying count, so it deduped less than it appeared to while also growing unbounded. Drift now carries `{kind, message}` and dedupe keys on the kind. Drop the array identity from the memo deps. The comment claimed the memo keyed off an outcome signature rather than array identities, but `installations` was still in the dep list -- so any caller deriving the array inline rebuilt the Map and the returned callback every render, invalidating downstream memos. The signature already encodes the names, matching useReachableInstallations. Make isUserScoped tri-state. Returning false for a missing subject conflated "we can't tell" with "confirmed shared user", and /api/me returns the token's claims verbatim -- so an IdP that doesn't emit `sub` would have been shown the very "these aren't your sessions" warning the probe exists to prevent. undefined now means unknown, and callers must stay silent on it.
|
All seven findings addressed in 7424430. Each was valid; I verified the claims against the code and kagent's source before changing anything, and two of them were catching real user-visible bugs rather than style.
The two I'd single out as more than hygiene:
The one comment I didn't act on as written is the 184 tests pass (up from 177), |
JS Dependency Audit0 added · 0 removed · 213 total (0 vs base) Projects audited
No change in vulnerabilities compared to the base branch. Full current vulnerability list (213)
|
1 participant
What does this PR do?
Adds the data layer behind the upcoming Agent Platform Sessions list: a
version-tolerant client for the kagent REST API. No visible change yet — this
lands the contract before any UI depends on it.
The constraint that shapes all of it: kagent ships no OpenAPI spec, GS pins
v0.9.9 while upstream is already on v0.10.0-beta9, and every installation is an
independent deployment. So the fleet can run several kagent versions at once, and
everything here is keyed per installation and defensive by default.
Wire → domain boundary (
lib/kagentSchema.ts,lib/kagentSessions.ts).Permissive zod schemas (unknown fields pass through, every field may be absent or
retyped) normalize into a stable
KagentSession, so a kagent schema change isabsorbed in one function instead of rippling through components. Specifics worth
reviewing:
than costing the whole list.
dataabsent (Go'somitemptyon an emptyslice — this is what "no sessions" actually looks like),
data: null, and abare top-level array.
created_at/updated_atare non-pointertime.Time, so unset arrives as0001-01-01T00:00:00Z— which browsers renderas "Dec 31, 0000".
Per-installation capability negotiation (
lib/kagentCapabilities.ts,hooks/useKagentCapabilities.ts). A cached/versionprobe per installationyields named flags, so components never compare versions inline. The supported
window is explicit (
MIN_SUPPORTED0.9.9,TESTED_UP_TO0.10.0): below the flooris flagged but still renders, above the ceiling proceeds optimistically and logs
once. A failed or unparseable probe degrades to the oldest supported version, and
never blocks the sessions query.
An
isUserScopedflag from a/meprobe. kagent'sunsecureauth modeignores the forwarded token and resolves every caller to a shared built-in user —
so the list would silently not be "your sessions". Better detected than
mislabelled.
Auth, per installation. There is no fleet-wide token: each installation has
its own Dex and
oidcTokenProvider. The client mints lazily per request, so amint failure degrades one installation (you may be signed in to some and not
others) rather than the page. Tokens are never cached — the plugin's query
cache is persisted to
localStorage, which is no place for a credential. Theminting sequence moves out of
useDeployAgentintolib/installationOidcToken.tsso the deploy flow and this client mintidentically.
What is the effect of this change to users?
None yet — nothing renders this. It unblocks the Sessions tab.
How does it look like?
Version-matrix fixtures are the contract, since kagent offers no spec. They
include a captured live v0.9.9 response, on the grounds that a payload from
production beats a hand-written one — it already exercises absent
source, absentdeleted_at, both real id shapes (64-char hex and UUID), and kagent's20-character truncated titles.
The core assertions: v0.9.9 and v0.10 payloads normalize to identical output,
and a synthetic future version carrying unknown fields plus an unknown
sourcevalue changes nothing.
Those fixtures immediately earned their keep by catching a zod subtlety I had
wrong: wrapping
z.unknown()in a.transform()makes the field requiredagain, so every row that merely omits a key was being dropped — which is most
real rows. Without the live fixture this would have shipped as a mysteriously
short list.
177 tests pass in
plugins/agent-platform(32 of them pre-existing);tscandlint are clean. The 8 remaining
no-mixed-plugin-importslint warnings arepre-existing files importing from
gs— none from this PR.Any background context you can provide?
Re-implementation of the Sessions view from the
agent-platform-ui prototype.
The prototype's
Sessionhas ~20 fields against kagent's 7, so the eventual UI isa deliberately reduced, read-only version.
Two notes for the next PR, both discovered from the live payload:
sourceis absent on every real row, so the planned "hide A2A subagentsessions" filter is a no-op against current data. Kept as forward-compatibility,
but it hides nothing today.
"What issues are assi...") andthe full first message is unrecoverable. The Session column will therefore be
weak, and the Agent column ends up carrying most of a row's meaning — worth
bearing in mind when reviewing the table.
Do the docs need to be updated?
The
docs/agent-platform.mdSessions section lands with the UI PR, where thebehaviour is user-visible. Design rationale lives in doc comments here.
Should this change be mentioned in the release notes?