From 3331199c58d6f0f755213ed254ce410079a74d76 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 20 Aug 2026 22:42:22 -0700 Subject: [PATCH 1/2] Add @corbits/chat/display-name and id-leak-guard modules deriveDisplayName/humanizeSlug (CL-6413) lived only in @corbits/agent-directory, which already depends on @corbits/chat -- so the chat participant invite/greeting path could never import them back without a circular dependency. Move them here as the canonical implementation; agent-directory re-exports them in the next commit. id-leak-guard adds the systemic check CL-6471 calls for: no user-visible string may carry an internal id (run_/wfd_/tnt_/prn_/ast_/ gtk_), in raw or humanized ("Run 737a058d...") form. --- packages/chat/package.json | 4 +- packages/chat/src/display-name.test.ts | 55 +++++++++++++ packages/chat/src/display-name.ts | 102 ++++++++++++++++++++++++ packages/chat/src/id-leak-guard.test.ts | 60 ++++++++++++++ packages/chat/src/id-leak-guard.ts | 44 ++++++++++ 5 files changed, 264 insertions(+), 1 deletion(-) create mode 100644 packages/chat/src/display-name.test.ts create mode 100644 packages/chat/src/display-name.ts create mode 100644 packages/chat/src/id-leak-guard.test.ts create mode 100644 packages/chat/src/id-leak-guard.ts diff --git a/packages/chat/package.json b/packages/chat/package.json index 7dabda6d7..d4f2068dd 100644 --- a/packages/chat/package.json +++ b/packages/chat/package.json @@ -15,7 +15,9 @@ "./stream-events": "./src/stream-events.ts", "./blocks": "./src/blocks.ts", "./agent-address": "./src/agent-address.ts", - "./workbench-host-naming": "./src/workbench-host-naming.ts" + "./workbench-host-naming": "./src/workbench-host-naming.ts", + "./display-name": "./src/display-name.ts", + "./id-leak-guard": "./src/id-leak-guard.ts" }, "scripts": { "typecheck": "tsc --noEmit", diff --git a/packages/chat/src/display-name.test.ts b/packages/chat/src/display-name.test.ts new file mode 100644 index 000000000..b65cf3ffc --- /dev/null +++ b/packages/chat/src/display-name.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, test } from "bun:test"; +import { deriveDisplayName, humanizeSlug } from "./display-name"; + +describe("humanizeSlug", () => { + test("title-cases a hyphenated slug", () => { + expect(humanizeSlug("research-analyst")).toBe("Research Analyst"); + }); + + test("title-cases an underscored slug", () => { + expect(humanizeSlug("architecture_reviewer")).toBe("Architecture Reviewer"); + }); + + test("passes prose through with only case fixed up", () => { + expect(humanizeSlug("myra")).toBe("Myra"); + }); +}); + +describe("deriveDisplayName", () => { + test("prefers a non-blank description over the slug", () => { + expect( + deriveDisplayName({ + name: "architecture-reviewer", + description: "Architecture reviewer", + }), + ).toBe("Architecture reviewer"); + }); + + test("humanizes the slug when description is absent or blank", () => { + expect(deriveDisplayName({ name: "architecture-reviewer" })).toBe( + "Architecture Reviewer", + ); + expect( + deriveDisplayName({ name: "architecture-reviewer", description: " " }), + ).toBe("Architecture Reviewer"); + }); + + test("throws rather than humanizing an internal run id into a fake name (CL-6471)", () => { + expect(() => + deriveDisplayName({ name: "run_737a058d48006e2bde12559576f422e0" }), + ).toThrow(/internal identifier/); + }); + + test("throws when the description itself is an internal id", () => { + expect(() => + deriveDisplayName({ + name: "architecture-reviewer", + description: "run_737a058d48006e2bde12559576f422e0", + }), + ).toThrow(/internal identifier/); + }); + + test('throws on a malformed shape rather than rendering "undefined"', () => { + expect(() => deriveDisplayName({} as { name: string })).toThrow(); + }); +}); diff --git a/packages/chat/src/display-name.ts b/packages/chat/src/display-name.ts new file mode 100644 index 000000000..24eedf911 --- /dev/null +++ b/packages/chat/src/display-name.ts @@ -0,0 +1,102 @@ +// A workflow definition's person-facing display name, derived once here +// so every caller that decides "what does this agent look like to a +// person" reads it the same way — never a scattered `description ?? name` +// (or worse, a raw address/run id) reimplemented per call site. +// +// Lives in `@corbits/chat` rather than `@corbits/agent-directory` (which +// originated this logic, CL-6413) because `@corbits/agent-directory` +// itself depends on `@corbits/chat` (`workbench-host-naming`); a reverse +// dependency would be circular. `@corbits/agent-directory/client.ts` +// re-exports `deriveDisplayName`/`humanizeSlug` from here for its existing +// callers rather than keeping a second copy that could drift. +import { type } from "arktype"; +import { ID_LEAK_PATTERN } from "./id-leak-guard"; + +const DisplayNameSource = type({ + name: "string", + "description?": "string | null", +}); + +/** kebab-case identifier -> Title Case words: `"research-analyst"` -> + * `"Research Analyst"`. Words that aren't hyphen-separated (a name that + * already reads as prose) pass through with only their case fixed up, so + * this is safe to run over a definition's raw `name` unconditionally — + * PROVIDED that `name` is never an internal id in disguise; see + * `deriveDisplayName`'s own guard for why the raw run-id case never + * reaches this function at all. */ +export function humanizeSlug(slug: string): string { + return slug + .split(/[-_\s]+/) + .filter((word) => word.length > 0) + .map((word) => word.slice(0, 1).toUpperCase() + word.slice(1)) + .join(" "); +} + +/** + * The display name a definition should render as: its own description + * when one was set at creation, otherwise a humanized reading of its + * immutable slug. A whitespace-only description reads as absent — never + * a blank display name — since it carries nothing a person actually + * typed. Throws on a shape that isn't at least `{ name }` — this is a + * trust boundary, not a formatting helper, so a malformed record fails + * loudly rather than rendering "undefined". + * + * Also throws when `name` is itself an internal-id shape (`run_…`, + * `wfd_…`, …, see `./id-leak-guard`) — the product rule is that a person + * never sees an internal identifier, so a caller that reaches this + * function with a run id where a definition's slug belongs gets a loud + * failure instead of a Title-Cased leak like "Run 737a058d…" (CL-6471). + */ +export function deriveDisplayName(definition: { + readonly name: string; + readonly description?: string | null; +}): string { + const parsed = DisplayNameSource(definition); + if (parsed instanceof type.errors) { + throw new Error( + `deriveDisplayName: invalid agent definition: ${parsed.summary}`, + ); + } + const description = parsed.description?.trim(); + if (description !== undefined && description !== "") { + if (ID_LEAK_PATTERN.test(description)) { + throw new Error( + `deriveDisplayName: description "${description}" for definition ` + + `"${parsed.name}" carries an internal identifier; refusing to ` + + "render it as a display name", + ); + } + return description; + } + if (ID_LEAK_PATTERN.test(parsed.name)) { + throw new Error( + `deriveDisplayName: definition name "${parsed.name}" is an internal ` + + "identifier, not a slug; refusing to humanize it into a fake " + + "display name", + ); + } + return humanizeSlug(parsed.name); +} + +export type UserFacingAgentDefinition = { + readonly id: string; + readonly name: string; + readonly description?: string | null; +}; + +export type WithDisplayName = T & { readonly displayName: string }; + +/** Projects `deriveDisplayName` onto a definition, keeping every other + * field untouched — the read-boundary derivation done once here rather + * than as scattered `??` fallbacks in UI code. */ +export function withDisplayName( + definition: T, +): WithDisplayName { + return { ...definition, displayName: deriveDisplayName(definition) }; +} + +export function withDisplayNames( + definitions: readonly T[], +): readonly WithDisplayName[] { + return definitions.map(withDisplayName); +} diff --git a/packages/chat/src/id-leak-guard.test.ts b/packages/chat/src/id-leak-guard.test.ts new file mode 100644 index 000000000..82d35aa2c --- /dev/null +++ b/packages/chat/src/id-leak-guard.test.ts @@ -0,0 +1,60 @@ +// CL-6471's systemic guard: no user-visible string may carry an internal +// identifier, in raw or humanized ("Run 737a058d…") form. +import { describe, expect, test } from "bun:test"; +import { assertNoLeakedInternalId } from "./id-leak-guard"; + +describe("assertNoLeakedInternalId", () => { + test("passes real, human-authored text through untouched", () => { + expect(() => + assertNoLeakedInternalId("Architecture reviewer", "a display name"), + ).not.toThrow(); + expect(() => + assertNoLeakedInternalId( + "Hi Alice, I'm Myra — your teammate here.", + "a greeting", + ), + ).not.toThrow(); + }); + + test("catches a raw internal id for every named prefix", () => { + const ids = [ + "run_737a058d48006e2bde12559576f422e0", + "wfd_737a058d48006e2bde12559576f422e0", + "tnt_737a058d48006e2bde12559576f422e0", + "prn_737a058d48006e2bde12559576f422e0", + "ast_737a058d48006e2bde12559576f422e0", + "gtk_737a058d48006e2bde12559576f422e0", + ]; + for (const id of ids) { + expect(() => assertNoLeakedInternalId(id, "a name")).toThrow( + /internal identifier/, + ); + } + }); + + test("catches the humanized (Title Cased) form the same id renders as once split", () => { + // `humanizeSlug`'s exact transform: "run_737a058d..." -> "Run 737a058d..." + expect(() => + assertNoLeakedInternalId( + "Run 737a058d48006e2bde12559576f422e0", + "a participant name", + ), + ).toThrow(/internal identifier/); + }); + + test("catches an id leaked mid-sentence, as a greeting would carry it", () => { + expect(() => + assertNoLeakedInternalId( + "Hi Alice, I'm run_737a058d48006e2bde12559576f422e0. Three reviewers read every pull request.", + "a greeting", + ), + ).toThrow(/internal identifier/); + }); + + test("never flags a short, coincidental substring match", () => { + // "runner" starts with "run" but not the "run_" id prefix + hex tail. + expect(() => + assertNoLeakedInternalId("Runner McRunface", "a display name"), + ).not.toThrow(); + }); +}); diff --git a/packages/chat/src/id-leak-guard.ts b/packages/chat/src/id-leak-guard.ts new file mode 100644 index 000000000..554cf37af --- /dev/null +++ b/packages/chat/src/id-leak-guard.ts @@ -0,0 +1,44 @@ +// The systemic guard CL-6471 calls for: a person never sees an internal +// identifier — not as a participant's name, not in a join/system line, +// not in a greeting, not in a chat title. This has recurred repeatedly +// (a raw definitionId/runId in insights, a `writing_systems` id in a +// Myra reply, and CL-6471's own "Run 737a058d…" / "I'm run_737a…") each +// time as a one-off display-time patch; this module is the one place +// every id-generating prefix is named, so a new leak is a missed test +// run rather than a missed grep. +// +// Prefix words mirror `@intx/hub-common`'s `generateId` (vendored; +// upstream source: `packages/hub-common/src/ids.ts`'s `PREFIXES`) for +// the six kinds CL-6471 names explicitly — a workflow run, a workflow +// definition, a tenant, a principal, an asset, and a git token — each +// normally followed by `_` and 32 lowercase hex characters (`run_737a…`). +// +// Matched with either `_`, a space, or `-` as the separator, not just +// `_`: `humanizeSlug`'s Title Case reading of a leaked id (CL-6471's own +// "Run 737a058d48006e2bde12559576f422e0") replaces the underscore with a +// space, so the raw and the humanized-leak forms both need to trip this. +const ID_PREFIX_WORDS = ["run", "wfd", "tnt", "prn", "ast", "gtk"] as const; + +export const ID_LEAK_PATTERN = new RegExp( + `\\b(?:${ID_PREFIX_WORDS.join("|")})[_\\s-][0-9a-f]{16,}\\b`, + "i", +); + +/** + * Throws when `value` carries an internal identifier — a raw id, or a + * humanized reading of one (`"Run 737a058d…"`, produced by title-casing + * `run_737a058d…`'s underscore split). The humanized case is why this + * checks the ORIGINAL prefixes case-insensitively rather than requiring + * the literal lowercase prefix: `humanizeSlug` capitalizes the first + * letter of the leading word, so a leaked run id renders as "Run …", not + * "run …", by the time it would reach a person. + */ +export function assertNoLeakedInternalId(value: string, context: string): void { + if (ID_LEAK_PATTERN.test(value)) { + throw new Error( + `${context} carries an internal identifier ("${value}"); a person ` + + "must never see one — resolve the real name at the source instead " + + "of rendering the id (CL-6471)", + ); + } +} From 6b2aeadbad567a8bee84d95400e2b47c94efa4b5 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 20 Aug 2026 22:42:37 -0700 Subject: [PATCH 2/2] CL-6471: never leak a run id as a participant's name or greeting An invited agent's display name/mention handle came from the caller's pre-fetched listInvitableDefinitions() snapshot; when that snapshot missed the definition (a just-created or just-redeployed row it predates -- exactly the "fresh stack, instantiate a template" race the owner hit), launchAndJoinAgent silently fell back to the run's own address local part, and routes.ts's greeting resolution fell back to that same leaked handle. The result: a participant named "Run 737a058d48006e2bde12559576f422e0" and a greeting reading "I'm run_737a058d...". Both call sites now resolve through resolveInvitedDisplayName, which adds a live resolveDefinitionNameSource lookup as the platform-port fallback instead of ever degrading to a raw id, and derives the name via deriveDisplayName so the agent's own greeting states its real name. agent-directory/client.ts now re-exports deriveDisplayName/humanizeSlug from @corbits/chat/display-name rather than keeping its own copy. --- packages/agent-directory/src/client.ts | 92 ++++---------------- packages/chat/src/platform-adapter.ts | 10 +++ packages/chat/src/platform-port.ts | 18 ++++ packages/chat/src/routes.ts | 4 +- packages/chat/src/workbench-service.ts | 78 +++++++++++++---- packages/chat/test/routes.test.ts | 52 ++++++++++- packages/chat/test/test-support.ts | 11 +++ packages/chat/test/workbench-service.test.ts | 79 ++++++++++++++++- 8 files changed, 243 insertions(+), 101 deletions(-) diff --git a/packages/agent-directory/src/client.ts b/packages/agent-directory/src/client.ts index 54c206c4e..d90885691 100644 --- a/packages/agent-directory/src/client.ts +++ b/packages/agent-directory/src/client.ts @@ -7,8 +7,26 @@ // to identify that plumbing; a host injects only its raw definition/ // instance lists, already fetched from wherever it gets them. -import { type } from "arktype"; import { isWorkbenchHostDefinitionName } from "@corbits/chat/workbench-host-naming"; +import { + deriveDisplayName, + humanizeSlug, + withDisplayName, + withDisplayNames, + type WithDisplayName, +} from "@corbits/chat/display-name"; + +// `deriveDisplayName`/`humanizeSlug` (CL-6413) live in `@corbits/chat` +// itself, not here: this package already depends on `@corbits/chat` (for +// `isWorkbenchHostDefinitionName` below), so a copy defined here could +// never be imported back by `@corbits/chat`'s own call sites without a +// circular dependency — exactly the gap CL-6471 traces the "Run +// 737a058d…" leak to (the chat participant invite/greeting path never +// migrated onto this derivation because it couldn't). Re-exported here so +// every existing caller of `@corbits/agent-directory/client`'s +// `deriveDisplayName`/`humanizeSlug` keeps working unchanged. +export { deriveDisplayName, humanizeSlug, withDisplayName, withDisplayNames }; +export type { WithDisplayName }; export type UserFacingAgentDefinition = { readonly id: string; @@ -16,78 +34,6 @@ export type UserFacingAgentDefinition = { readonly description?: string | null; }; -/** - * A definition's kebab `name` is its immutable, URL-facing identifier - * (CL-6413) — the mail handle `createAgentDefinitionCore` binds it to - * (`@corbits/agent-directory/agent-workflow`'s `input.handle`). The - * person-facing display name it was created with lands one hop away, on - * `description`: that same handler seeds `workflowDefinition.description` - * from the asset's own `displayName` - * (`vendor/intx/hub-sessions/src/workflow-definition-ensure.ts`), so - * `deriveDisplayName` reads it from there. A definition created before - * that seeding existed, or with no description ever set, has no display - * name to read — `humanizeSlug` backfills one from the identifier itself - * rather than showing the raw slug as if it were a name. - */ -const DisplayNameSource = type({ - name: "string", - "description?": "string | null", -}); - -/** kebab-case identifier -> Title Case words: `"research-analyst"` -> - * `"Research Analyst"`. Words that aren't hyphen-separated (a name that - * already reads as prose) pass through with only their case fixed up, so - * this is safe to run over a definition's raw `name` unconditionally. */ -export function humanizeSlug(slug: string): string { - return slug - .split(/[-_\s]+/) - .filter((word) => word.length > 0) - .map((word) => word.slice(0, 1).toUpperCase() + word.slice(1)) - .join(" "); -} - -/** - * The display name a definition should render as: its own description - * when one was set at creation, otherwise a humanized reading of its - * immutable slug. A whitespace-only description reads as absent — never - * a blank display name — since it carries nothing a person actually - * typed. Throws on a shape that isn't at least `{ name }` — this is a - * trust boundary, not a formatting helper, so a malformed record fails - * loudly rather than rendering "undefined". - */ -export function deriveDisplayName(definition: { - readonly name: string; - readonly description?: string | null; -}): string { - const parsed = DisplayNameSource(definition); - if (parsed instanceof type.errors) { - throw new Error( - `deriveDisplayName: invalid agent definition: ${parsed.summary}`, - ); - } - const description = parsed.description?.trim(); - return description !== undefined && description !== "" - ? description - : humanizeSlug(parsed.name); -} - -export type WithDisplayName = T & { readonly displayName: string }; - -/** Projects `deriveDisplayName` onto a definition, keeping every other - * field untouched — the read-boundary derivation the ticket calls for, - * done once here rather than as scattered `??` fallbacks in UI code. */ -export function withDisplayName( - definition: T, -): WithDisplayName { - return { ...definition, displayName: deriveDisplayName(definition) }; -} - -export function withDisplayNames( - definitions: readonly T[], -): readonly WithDisplayName[] { - return definitions.map(withDisplayName); -} - export type UserFacingAgentInstance = { readonly id: string; readonly definitionId: string; diff --git a/packages/chat/src/platform-adapter.ts b/packages/chat/src/platform-adapter.ts index 70a1a8591..421304ea0 100644 --- a/packages/chat/src/platform-adapter.ts +++ b/packages/chat/src/platform-adapter.ts @@ -666,6 +666,16 @@ export function createHubChatPlatform( return row?.assetId ?? undefined; }, + async resolveDefinitionNameSource(definitionId) { + const row = await deps.db.query.workflowDefinition.findFirst({ + where: eq(workflowDefinition.id, definitionId), + }); + if (row === undefined) return undefined; + return typeof row.description === "string" && row.description !== "" + ? { name: row.name, description: row.description } + : { name: row.name }; + }, + async refreshAgentInstanceFromDefinition( tenantId, _workbenchId, diff --git a/packages/chat/src/platform-port.ts b/packages/chat/src/platform-port.ts index ff624eda4..ecf525a75 100644 --- a/packages/chat/src/platform-port.ts +++ b/packages/chat/src/platform-port.ts @@ -95,6 +95,24 @@ export interface WorkbenchLauncher { */ resolveDefinitionAssetId(definitionId: string): Promise; + /** + * Resolves a definition id directly to the name/description pair + * `@corbits/chat/display-name`'s `deriveDisplayName` reads — the + * authoritative source a caller falls back to when a definition it + * needs to name isn't present in an already-fetched + * `listInvitableDefinitions` snapshot (a just-created or just-redeployed + * definition the snapshot predates). Never used as the primary lookup — + * an `invitable` hit is cheaper and already in hand — only as the seam + * that keeps a stale-snapshot miss from ever degrading to a raw address + * or run id (CL-6471). Returns undefined only when the tenant truly has + * no such definition. + */ + resolveDefinitionNameSource( + definitionId: string, + ): Promise< + { readonly name: string; readonly description?: string } | undefined + >; + /** * Recomputes an already-invited instance's folded launch body from * its definition's CURRENT asset content, and persists it so the diff --git a/packages/chat/src/routes.ts b/packages/chat/src/routes.ts index 9dcab235d..99527db00 100644 --- a/packages/chat/src/routes.ts +++ b/packages/chat/src/routes.ts @@ -1283,9 +1283,7 @@ export function createChatRoutes(deps: CreateChatRoutesDeps): Hono { const agentAddress = joined.address; const joinEventDelivered = joined.joinEventDelivered; - const agentDisplayName = - invitable.find((definition) => definition.id === definitionId) - ?.description ?? joined.handle; + const agentDisplayName = joined.displayName; const templatePromise = body.templatePromise; const connectGithubRequiredFor = body.connectGithubRequiredFor; runPostMintDelivery(async () => { diff --git a/packages/chat/src/workbench-service.ts b/packages/chat/src/workbench-service.ts index 6a1019bb6..f6863f279 100644 --- a/packages/chat/src/workbench-service.ts +++ b/packages/chat/src/workbench-service.ts @@ -12,6 +12,8 @@ import { InferenceResolutionError } from "@corbits/folded-runs"; import { encodeParts } from "./codec"; import type { Part as PartType } from "./parts"; import { localPartOf } from "./agent-address"; +import { deriveDisplayName } from "./display-name"; +import { assertNoLeakedInternalId } from "./id-leak-guard"; import { isAgentAddress, mentionedParticipants } from "./mentions"; import { mergeContextIntoParts } from "./workbench-context"; import { @@ -163,6 +165,12 @@ export type LaunchAndJoinAgentResult = { readonly address: string; readonly definitionId: string; readonly handle: string; + /** The agent's real, person-facing name — see `resolveInvitedDisplayName`. + * Never the mention handle: a caller that needs "Architecture reviewer" + * rather than "architecture-reviewer" (the canned greeting, chiefly) + * reads this instead of re-deriving it from a possibly-stale + * `invitable` snapshot (CL-6471). */ + readonly displayName: string; readonly settings: Record; /** * Settles when the timeline's `workbench.agent-joined` event has been @@ -210,6 +218,41 @@ export async function findResidentAgentForDefinition( return undefined; } +/** + * Resolves the display name an invited definition should carry, the one + * source both the participant's mention handle and the canned greeting's + * "I'm ${agent}" read (CL-6471): the pre-fetched `invitable` snapshot + * when it has the definition, falling back to a live, authoritative + * lookup (`resolveDefinitionNameSource`) when it doesn't — a just-created + * or just-redeployed definition the snapshot predates. Never falls + * further than that: a definition this tenant genuinely has no row for + * is a loud error, never a raw address or run id standing in for a name. + */ +export async function resolveInvitedDisplayName( + platform: Pick, + invitable: readonly InvitableDefinition[], + definitionId: string, +): Promise { + const invitedDefinition = invitable.find( + (definition) => definition.id === definitionId, + ); + const nameSource = + invitedDefinition ?? + (await platform.resolveDefinitionNameSource(definitionId)); + if (nameSource === undefined) { + throw new Error( + `cannot resolve a display name for definition "${definitionId}": ` + + "this tenant carries no such definition", + ); + } + const displayName = deriveDisplayName(nameSource); + assertNoLeakedInternalId( + displayName, + `definition "${definitionId}"'s display name`, + ); + return displayName; +} + /** * The invite core: launches the definition's own instance, derives * its friendly mention handle, appends the participant record, posts @@ -236,25 +279,18 @@ export async function launchAndJoinAgent( definitionId: input.definitionId, }); - // The invited definition's human display name (`description`, e.g. - // "Myra" for the `assistant` asset) becomes the friendly mention - // handle, falling back to the asset name itself when the deploy - // carried no display name, and to the invited run's own unusable - // instance-id local part when the listing no longer carries the - // definition at all. The asset name (`.name`) is a wire identifier, - // never UI copy — it must never surface as a mention handle. Either - // way it is de-duplicated against every handle already in the - // workbench ("echo", "echo-2", ...). - const invitedDefinition = input.invitable.find( - (definition) => definition.id === input.definitionId, + // The invited definition's real display name becomes the friendly + // mention handle (see `resolveInvitedDisplayName`) — de-duplicated + // against every handle already in the workbench ("echo", "echo-2", + // ...). Never the asset name's raw slug, and never the run's own + // address/instance-id local part: a definition this snapshot missed + // is resolved live rather than degraded to an internal id (CL-6471). + const displayName = await resolveInvitedDisplayName( + deps.platform, + input.invitable, + input.definitionId, ); - const desiredHandle = - invitedDefinition !== undefined - ? handleFromName( - invitedDefinition.description ?? invitedDefinition.name, - launched.address, - ) - : localPartOf(launched.address); + const desiredHandle = handleFromName(displayName, launched.address); // The record is updated before the join event is posted, matching // the settings PATCH route's record-then-mail ordering: the @@ -314,6 +350,7 @@ export async function launchAndJoinAgent( address: launched.address, definitionId: input.definitionId, handle: desiredHandle, + displayName, settings: row.settings, joinEventDelivered, }; @@ -397,6 +434,11 @@ function templateGreeting(who: string, agent: string, promise: string): string { } export function cannedGreeting(input: CannedGreetingInput): string { + // The agent states its own name here, verbatim — the exact spot + // CL-6471's "I'm run_737a058d…" leaked from. Guarded at the source + // rather than trusted, since every caller ultimately reaches this + // through `agentName` alone. + assertNoLeakedInternalId(input.agentName, "a greeting's agent name"); const who = input.senderName !== undefined && input.senderName !== "" ? ` ${input.senderName}` diff --git a/packages/chat/test/routes.test.ts b/packages/chat/test/routes.test.ts index f82ed2d61..203786af3 100644 --- a/packages/chat/test/routes.test.ts +++ b/packages/chat/test/routes.test.ts @@ -243,7 +243,9 @@ describe("POST /workbenches", () => { expect(greeting?.workbenchId).toBe(body.id); expect(greeting?.runId).toBe("ins_invited1"); expect(greeting?.sender.address).toBe("ins_invited1@acme.example"); - expect(timelineTexts(timeline)[0]).toContain("echo"); + // The greeting names the agent by its real display name (CL-6471) — + // never its lowercase mention handle. + expect(timelineTexts(timeline)[0]).toContain("Echo"); expect(timelineTexts(timeline)[0]).toMatch(/\?$/); }); @@ -276,6 +278,42 @@ describe("POST /workbenches", () => { ); }); + // CL-6471: the owner's live repro — instantiating the code-review + // template on a fresh stack, the setup agent's own definition missed + // the pre-fetched `invitable` snapshot (a just-seeded/just-redeployed + // row the snapshot predates), and its greeting rendered "I'm + // run_737a058d…" instead of its real name. The greeting must resolve + // the real name through a live lookup instead, never the run's own + // address. + test("a definition missing from the invitable snapshot still greets with its real name, never its own run address (CL-6471)", async () => { + const deliveries: (() => Promise)[] = []; + const deps = buildDeps({ + platform: fakePlatform({ + invitable: [], // the stale/pre-fetched snapshot misses it + resolveDefinitionNameSource: async (definitionId) => + definitionId === "wfd_echo" + ? { name: "echo", description: "Myra" } + : undefined, + }), + runPostMintDelivery: (work) => { + deliveries.push(work); + }, + }); + const app = mountAs(createChatRoutes(deps), "prn_alice"); + + const { body } = await createWorkbench(app, { + kind: "chat", + definitionId: "wfd_echo", + templatePromise: + "Three reviewers read every pull request and post what they'd change.", + }); + await deliveries[0]?.(); + + const timeline = await timelineOf(deps, body.id); + expect(timelineTexts(timeline)[0]).toContain("I'm Myra"); + expect(timelineTexts(timeline)[0]).not.toContain("ins_invited1"); + }); + test("creating a chat with connectGithubRequiredFor posts a connect-github block after the greeting", async () => { const deliveries: (() => Promise)[] = []; const deps = buildDeps({ @@ -331,7 +369,7 @@ describe("POST /workbenches", () => { expect(response.status).toBe(201); const timeline = await timelineOf(deps, body.id); - expect(timelineTexts(timeline)[0]).toContain("echo"); + expect(timelineTexts(timeline)[0]).toContain("Echo"); }); test("creating a chat deploys nothing on the request path — the deploys ride the post-mint delivery", async () => { @@ -1060,6 +1098,7 @@ describe("DELETE /workbenches/:id/participants/:address", () => { test("removes an invited agent and releases its launched instance", async () => { const released: { address: string; reason: string }[] = []; const deps = buildDeps({ + platform: fakePlatform({ invitable: [{ id: "wfd_echo", name: "Echo" }] }), releaseAgentInstance: async (address, reason) => { released.push({ address, reason }); }, @@ -1096,7 +1135,9 @@ describe("DELETE /workbenches/:id/participants/:address", () => { }); test("still removes the participant when no releaseAgentInstance is wired", async () => { - const deps = buildDeps(); + const deps = buildDeps({ + platform: fakePlatform({ invitable: [{ id: "wfd_echo", name: "Echo" }] }), + }); const app = mountAs(createChatRoutes(deps), "prn_alice"); const { body: workbench } = await createWorkbench(app, { kind: "workbench", @@ -1220,6 +1261,7 @@ describe("GET /workbenches/:id/agents", () => { test("resolves the workbench's agent participant back to its definition id", async () => { const deps = buildDeps({ platform: fakePlatform({ + invitable: [{ id: "wfd_echo", name: "Echo" }], resolveDefinitionIdByAddress: async (address) => address === "ins_invited1@acme.example" ? "wfd_echo" : undefined, }), @@ -1249,6 +1291,10 @@ describe("GET /workbenches/:id/agents", () => { let invited = 0; const deps = buildDeps({ platform: fakePlatform({ + invitable: [ + { id: "wfd_echo", name: "Echo" }, + { id: "wfd_other", name: "Other" }, + ], launchInvite: async () => { invited += 1; return { diff --git a/packages/chat/test/test-support.ts b/packages/chat/test/test-support.ts index 8ff71f8cc..cf2b8c438 100644 --- a/packages/chat/test/test-support.ts +++ b/packages/chat/test/test-support.ts @@ -58,6 +58,11 @@ export function fakePlatform( resolveDefinitionIdByAddress?: ( address: string, ) => Promise; + resolveDefinitionNameSource?: ( + definitionId: string, + ) => Promise< + { readonly name: string; readonly description?: string } | undefined + >; refreshAgentInstanceFromDefinition?: ( tenantId: string, workbenchId: string, @@ -141,6 +146,12 @@ export function fakePlatform( } return undefined; }, + async resolveDefinitionNameSource(definitionId) { + if (opts.resolveDefinitionNameSource !== undefined) { + return opts.resolveDefinitionNameSource(definitionId); + } + return (opts.invitable ?? []).find((d) => d.id === definitionId); + }, async refreshAgentInstanceFromDefinition(tenantId, workbenchId, address) { refreshCalls.push({ tenantId, workbenchId, address }); if (opts.refreshAgentInstanceFromDefinition !== undefined) { diff --git a/packages/chat/test/workbench-service.test.ts b/packages/chat/test/workbench-service.test.ts index 06192d249..95b5b31b8 100644 --- a/packages/chat/test/workbench-service.test.ts +++ b/packages/chat/test/workbench-service.test.ts @@ -925,7 +925,9 @@ describe("message fan-out", () => { describe("POST /workbenches/:id/invite", () => { test("launches the definition, appends the participant, and posts a join event", async () => { - const deps = buildDeps(); + const deps = buildDeps({ + platform: fakePlatform({ invitable: [{ id: "wfd_echo", name: "Echo" }] }), + }); const app = mountAs(createChatRoutes(deps), "prn_alice"); const { body: workbench } = await createWorkbench(app, { kind: "workbench", @@ -960,8 +962,11 @@ describe("POST /workbenches/:id/invite", () => { TENANT.id, workbench.id, ); + // The handle is derived from the definition's own name ("Echo" -> + // "echo") — never the run's own address local part, which is what + // it fell back to before CL-6471's fix. expect(settingsRow?.settings["chat/participants"]).toEqual([ - { address: "ins_invited1@acme.example", handle: "ins_invited1" }, + { address: "ins_invited1@acme.example", handle: "echo" }, ]); // Joining is a fact about the room, so it is posted onto the room's @@ -1025,7 +1030,9 @@ describe("POST /workbenches/:id/invite", () => { }); test("appends onto an existing participant list rather than replacing it", async () => { - const deps = buildDeps(); + const deps = buildDeps({ + platform: fakePlatform({ invitable: [{ id: "wfd_echo", name: "Echo" }] }), + }); const app = mountAs(createChatRoutes(deps), "prn_alice"); const { body: workbench } = await createWorkbench(app, { kind: "workbench", @@ -1044,7 +1051,7 @@ describe("POST /workbenches/:id/invite", () => { ); expect(settingsRow?.settings["chat/participants"]).toEqual([ { address: "existing@acme.example", handle: "existing" }, - { address: "ins_invited1@acme.example", handle: "ins_invited1" }, + { address: "ins_invited1@acme.example", handle: "echo" }, ]); }); @@ -1104,6 +1111,70 @@ describe("POST /workbenches/:id/invite", () => { ]); }); + // CL-6471: a freshly created/redeployed definition can miss the + // `invitable` snapshot the caller pre-fetched (the exact "fresh stack, + // instantiate a template" race the owner hit) — this must resolve the + // real name live rather than degrading to the run's own address. + test("a definition missing from the invitable snapshot still resolves its real name via a live lookup (CL-6471)", async () => { + const deps = buildDeps({ + platform: fakePlatform({ + invitable: [], // the stale/pre-fetched snapshot misses it + resolveDefinitionNameSource: async (definitionId) => + definitionId === "wfd_reviewer" + ? { + name: "architecture-reviewer", + description: "Architecture reviewer", + } + : undefined, + }), + }); + const app = mountAs(createChatRoutes(deps), "prn_alice"); + const { body: workbench } = await createWorkbench(app, { + kind: "workbench", + }); + + const response = await app.request(`/workbenches/${workbench.id}/invite`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ definitionId: "wfd_reviewer" }), + }); + + expect(response.status).toBe(201); + const settingsRow = await deps.store.getWorkbenchSettings( + TENANT.id, + workbench.id, + ); + // Never "ins_invited1" (the raw address local part) and never + // "run_..."/"ins_..." in any form — the real, humanized name. + expect(settingsRow?.settings["chat/participants"]).toEqual([ + { address: "ins_invited1@acme.example", handle: "architecture-reviewer" }, + ]); + }); + + test("a definition unresolvable anywhere fails loud rather than leaking the run's own address as its name (CL-6471)", async () => { + const deps = buildDeps({ + platform: fakePlatform({ invitable: [] }), // no live lookup will find it either + }); + const app = mountAs(createChatRoutes(deps), "prn_alice"); + const { body: workbench } = await createWorkbench(app, { + kind: "workbench", + }); + + const response = await app.request(`/workbenches/${workbench.id}/invite`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ definitionId: "wfd_ghost" }), + }); + + // Never a 201 with a participant record carrying a leaked id. + expect(response.status).not.toBe(201); + const settingsRow = await deps.store.getWorkbenchSettings( + TENANT.id, + workbench.id, + ); + expect(settingsRow?.settings["chat/participants"]).toEqual([]); + }); + test("a malformed body is rejected with the structured error envelope", async () => { const deps = buildDeps(); const app = mountAs(createChatRoutes(deps), "prn_alice");