-
Notifications
You must be signed in to change notification settings - Fork 187
[superlog] Fix insights agent annotation creation: resolve domain→UUID and add manage:config scope #472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
[superlog] Fix insights agent annotation creation: resolve domain→UUID and add manage:config scope #472
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||||||||||||||||||||||||||
| import { tool } from "ai"; | ||||||||||||||||||||||||||||||
| import dayjs from "dayjs"; | ||||||||||||||||||||||||||||||
| import { z } from "zod"; | ||||||||||||||||||||||||||||||
| import type { AppContext } from "../config/context"; | ||||||||||||||||||||||||||||||
| import { callRPCProcedure, createToolLogger, getAppContext } from "./utils"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const logger = createToolLogger("Annotations Tools"); | ||||||||||||||||||||||||||||||
|
|
@@ -87,18 +88,45 @@ const deleteAnnotationInputSchema = z.object({ | |||||||||||||||||||||||||||||
| confirmed: z.boolean().describe("false=preview, true=delete"), | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * The insights agent receives the website domain (e.g. "example.com") as its | ||||||||||||||||||||||||||||||
| * primary identifier, so LLMs routinely pass a domain name where the backend | ||||||||||||||||||||||||||||||
| * expects a UUID. This helper maps a domain name back to the UUID that the RPC | ||||||||||||||||||||||||||||||
| * layer understands, using the app context that is injected at agent run time. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| function resolveWebsiteId(ctx: AppContext, inputId: string): string { | ||||||||||||||||||||||||||||||
| // Direct match against context UUID — already correct. | ||||||||||||||||||||||||||||||
| if (inputId === ctx.websiteId || inputId === ctx.defaultWebsiteId) { | ||||||||||||||||||||||||||||||
| return inputId; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| // Input is the context domain → return the context UUID. | ||||||||||||||||||||||||||||||
| if (ctx.websiteDomain && inputId === ctx.websiteDomain && ctx.websiteId) { | ||||||||||||||||||||||||||||||
| return ctx.websiteId; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| // Input matches an accessible website's domain → return its UUID. | ||||||||||||||||||||||||||||||
| const byDomain = (ctx.accessibleWebsites ?? []).find( | ||||||||||||||||||||||||||||||
| (w) => w.domain === inputId | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
|
Comment on lines
+103
to
+109
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||
| if (byDomain) { | ||||||||||||||||||||||||||||||
| return byDomain.id; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| // Pass through — might already be a valid UUID or will fail at the RPC layer. | ||||||||||||||||||||||||||||||
| return inputId; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export function createAnnotationTools() { | ||||||||||||||||||||||||||||||
| const listAnnotationsTool = tool({ | ||||||||||||||||||||||||||||||
| description: | ||||||||||||||||||||||||||||||
| "List annotations for a chart context (metadata, text, tags, timing).", | ||||||||||||||||||||||||||||||
| inputSchema: listAnnotationsInputSchema, | ||||||||||||||||||||||||||||||
| execute: async ({ websiteId, chartType, chartContext }, options) => { | ||||||||||||||||||||||||||||||
| const context = getAppContext(options); | ||||||||||||||||||||||||||||||
| const resolvedWebsiteId = resolveWebsiteId(context, websiteId); | ||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||
| const result = await callRPCProcedure( | ||||||||||||||||||||||||||||||
| "annotations", | ||||||||||||||||||||||||||||||
| "list", | ||||||||||||||||||||||||||||||
| { websiteId, chartType, chartContext }, | ||||||||||||||||||||||||||||||
| { websiteId: resolvedWebsiteId, chartType, chartContext }, | ||||||||||||||||||||||||||||||
| context | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||
|
|
@@ -107,7 +135,7 @@ export function createAnnotationTools() { | |||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||
| logger.error("Failed to list annotations", { | ||||||||||||||||||||||||||||||
| websiteId, | ||||||||||||||||||||||||||||||
| websiteId: resolvedWebsiteId, | ||||||||||||||||||||||||||||||
| chartType, | ||||||||||||||||||||||||||||||
| error, | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
|
|
@@ -140,6 +168,7 @@ export function createAnnotationTools() { | |||||||||||||||||||||||||||||
| options | ||||||||||||||||||||||||||||||
| ) => { | ||||||||||||||||||||||||||||||
| const context = getAppContext(options); | ||||||||||||||||||||||||||||||
| const resolvedWebsiteId = resolveWebsiteId(context, websiteId); | ||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||
| if (!confirmed) { | ||||||||||||||||||||||||||||||
| const dateRangePreview = `${chartContext.dateRange.start_date} to ${chartContext.dateRange.end_date} (${chartContext.dateRange.granularity})`; | ||||||||||||||||||||||||||||||
|
|
@@ -149,7 +178,7 @@ export function createAnnotationTools() { | |||||||||||||||||||||||||||||
| message: | ||||||||||||||||||||||||||||||
| "Please review the annotation details below and confirm if you want to create it:", | ||||||||||||||||||||||||||||||
| annotation: { | ||||||||||||||||||||||||||||||
| websiteId, | ||||||||||||||||||||||||||||||
| websiteId: resolvedWebsiteId, | ||||||||||||||||||||||||||||||
| chartType, | ||||||||||||||||||||||||||||||
| dateRange: dateRangePreview, | ||||||||||||||||||||||||||||||
| annotationType, | ||||||||||||||||||||||||||||||
|
|
@@ -170,7 +199,7 @@ export function createAnnotationTools() { | |||||||||||||||||||||||||||||
| "annotations", | ||||||||||||||||||||||||||||||
| "create", | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| websiteId, | ||||||||||||||||||||||||||||||
| websiteId: resolvedWebsiteId, | ||||||||||||||||||||||||||||||
| chartType, | ||||||||||||||||||||||||||||||
| chartContext, | ||||||||||||||||||||||||||||||
| annotationType, | ||||||||||||||||||||||||||||||
|
|
@@ -192,7 +221,7 @@ export function createAnnotationTools() { | |||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||||
| logger.error("Failed to create annotation", { | ||||||||||||||||||||||||||||||
| websiteId, | ||||||||||||||||||||||||||||||
| websiteId: resolvedWebsiteId, | ||||||||||||||||||||||||||||||
| chartType, | ||||||||||||||||||||||||||||||
| text, | ||||||||||||||||||||||||||||||
| error, | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultWebsiteId-only contextsBranch 2 returns
ctx.websiteId, but the codebase usesdefaultWebsiteId ?? websiteIdas the canonical primary identifier (seerequireWebsiteIdincontext.ts). Whenctx.websiteIdis falsy butctx.defaultWebsiteIdholds the correct UUID — a valid AppContext for non-insights chat agents — the guard&& ctx.websiteIdcauses a fallthrough to theaccessibleWebsitesscan. If that list is also empty (common in lightweight contexts), the raw domain string passes through to the RPC layer, reproducing the sameNOT_FOUNDerror this PR was written to fix. The fix is one line: returnctx.websiteId ?? ctx.defaultWebsiteIdinstead of justctx.websiteId.