diff --git a/worker/src/review.ts b/worker/src/review.ts index 466ee65..e1a0843 100644 --- a/worker/src/review.ts +++ b/worker/src/review.ts @@ -24,6 +24,7 @@ import { getPullRequestDiff, listCommitComments, } from "./github"; +import { identity, inSpan } from "./tracing"; /** Workers AI models known to do function calling and structured output. */ export const REVIEW_MODELS = { @@ -439,6 +440,12 @@ export interface ReviewOptions { trigger?: string; } +/** + * The opaque id a review's spans are grouped under: the repository plus the + * pull request number or the commit. Public identifiers, never content. + */ +type Conversation = string; + /** * Review a diff. The part that does not care where the diff came from. */ @@ -447,6 +454,7 @@ async function reviewDiff( repo: string, diff: string, opts: ReviewOptions, + conversation: Conversation, ): Promise { const model = opts.model || env.REVIEW_MODEL || REVIEW_MODELS[DEFAULT_MODEL]; const maxDiffBytes = @@ -469,26 +477,52 @@ async function reviewDiff( }; } - const out = (await env.AI!.run( - model, + // The model call is the agent's one `chat` span. Metadata only: the model + // id, the diff's size and whether it was cut — never the diff, the prompt or + // the response (see tracing.ts). + const out = await inSpan( + "chat", { - messages: [ - { role: "system", content: SYSTEM_PROMPT }, + ...identity("chat", conversation), + "gen_ai.request.model": model, + "review.diff_bytes": sent.length, + "review.truncated": truncated, + }, + async (span) => { + const res = (await env.AI!.run( + model, { - role: "user", - content: `Repository: ${repo}\n\nAnnotated diff. Lines this change ADDS start with "+" and carry their new-file line number before the "|"; only those are valid finding targets.\n\n${sent}`, + messages: [ + { role: "system", content: SYSTEM_PROMPT }, + { + role: "user", + content: `Repository: ${repo}\n\nAnnotated diff. Lines this change ADDS start with "+" and carry their new-file line number before the "|"; only those are valid finding targets.\n\n${sent}`, + }, + ], + response_format: responseFormat(model), }, - ], - response_format: { type: "json_schema", json_schema: FINDINGS_SCHEMA }, + // The gateway is the THIRD argument, not a field of the input object — + // putting it in the input silently does nothing and the call still + // succeeds, so the mistake shows up as a gateway with no traffic rather + // than as an error. + gatewayOptions(env, repo, opts), + )) as ModelResponse; + const usage = res?.usage ?? {}; + span?.setAttributes({ + "gen_ai.usage.input_tokens": + typeof usage.prompt_tokens === "number" + ? usage.prompt_tokens + : undefined, + "gen_ai.usage.output_tokens": + typeof usage.completion_tokens === "number" + ? usage.completion_tokens + : undefined, + }); + return res; }, - // The gateway is the THIRD argument, not a field of the input object — - // putting it in the input silently does nothing and the call still - // succeeds, so the mistake shows up as a gateway with no traffic rather - // than as an error. - gatewayOptions(env, repo, opts), - )) as { response?: unknown }; - - const { summary, findings } = parseFindings(out?.response); + ); + + const { summary, findings } = parseFindings(modelOutput(out)); const { anchored, unanchored } = anchor(findings, addedLines); return { @@ -514,7 +548,7 @@ async function reviewDiff( * reviewed twice is a person asking for a second opinion, and handing back * the first one verbatim answers a question nobody asked. */ -function gatewayOptions( +export function gatewayOptions( env: Env, repo: string, opts: ReviewOptions, @@ -524,11 +558,50 @@ function gatewayOptions( gateway: { id: env.AI_GATEWAY_ID, skipCache: true, - metadata: { repo, trigger: opts.trigger ?? "manual" }, + // Attribution only, and at most five scalars (the gateway's limit). The + // trigger is reduced to its KIND: "mention:" names a person, and + // a GitHub login has no business in a cost log. + metadata: { + worker: "shamwari-github-mcp", + job: "pr_review", + repo, + trigger: (opts.trigger ?? "manual").split(":")[0], + }, }, }; } +/** + * A gateway dynamic route (`dynamic/`) speaks the OpenAI chat + * completions shape only; a Workers AI model id speaks the native one. The + * difference is the structured-output request and where the answer comes + * back, so REVIEW_MODEL can name either without a code change. + */ +export function isDynamicRoute(model: string): boolean { + return model.startsWith("dynamic/"); +} + +export function responseFormat(model: string): Record { + return isDynamicRoute(model) + ? { + type: "json_schema", + json_schema: { name: "review_findings", schema: FINDINGS_SCHEMA }, + } + : { type: "json_schema", json_schema: FINDINGS_SCHEMA }; +} + +interface ModelResponse { + response?: unknown; + choices?: Array<{ message?: { content?: unknown } }>; + usage?: Record; +} + +/** The model's answer, from either response shape. */ +export function modelOutput(out: ModelResponse | undefined): unknown { + if (out?.response !== undefined) return out.response; + return out?.choices?.[0]?.message?.content; +} + /** The guards that must pass before anything is fetched or spent. */ function assertRunnable(env: Env): void { if (env.REVIEW_ENABLED === "false") { @@ -591,9 +664,26 @@ export async function reviewPush( opts: ReviewOptions = {}, ): Promise { assertRunnable(env); - const diff = await compareCommits(env, repo, before, after); - const result = await reviewDiff(env, repo, diff, opts); - return post(env, repo, after, result, opts); + const conversation = `${repo}@${after.slice(0, 12)}`; + return inSpan( + "invoke_agent", + { + ...identity("invoke_agent", conversation), + "review.trigger": triggerKind(opts), + }, + async (span) => { + const skipped = await skipIfReviewed(env, repo, after, opts); + if (skipped) { + span?.setAttributes({ "review.skipped": "already reviewed" }); + return skipped; + } + const diff = await compareCommits(env, repo, before, after); + const result = await reviewDiff(env, repo, diff, opts, conversation); + const posted = await post(env, repo, after, result, opts); + span?.setAttributes(outcome(posted)); + return posted; + }, + ); } /** @@ -615,15 +705,75 @@ export async function reviewPullRequest( headSha?: string, ): Promise { assertRunnable(env); - const diff = await getPullRequestDiff(env, repo, number); - const result = await reviewDiff(env, repo, diff, opts); - if (!opts.post) return result; - if (!headSha) { + if (opts.post && !headSha) { throw new GitHubError( "cannot post a review without the head commit sha", 400, null, ); } - return post(env, repo, headSha, result, opts); + const conversation = `${repo}#${number}`; + return inSpan( + "invoke_agent", + { + ...identity("invoke_agent", conversation), + "review.trigger": triggerKind(opts), + }, + async (span) => { + if (headSha) { + const skipped = await skipIfReviewed(env, repo, headSha, opts); + if (skipped) { + span?.setAttributes({ "review.skipped": "already reviewed" }); + return skipped; + } + } + const diff = await getPullRequestDiff(env, repo, number); + const result = await reviewDiff(env, repo, diff, opts, conversation); + if (!opts.post) return result; + const posted = await post(env, repo, headSha!, result, opts); + span?.setAttributes(outcome(posted)); + return posted; + }, + ); +} + +function triggerKind(opts: ReviewOptions): string { + return (opts.trigger ?? "manual").split(":")[0]; +} + +function outcome(r: ReviewResult): Record { + return { + "review.posted": r.posted, + "review.findings": r.findings.length, + "review.unanchored": r.unanchored.length, + }; +} + +/** + * Answer "already reviewed" BEFORE the model is called. + * + * post() has always refused to write a second review on a commit, but it + * asked only after the diff had been fetched and the model had been paid to + * read it — so a webhook redelivery or a re-request bought a full review and + * then threw it away. Asking first costs one GitHub read and saves the model + * call. post() still checks again, which covers two deliveries racing. + */ +async function skipIfReviewed( + env: Env, + repo: string, + sha: string, + opts: ReviewOptions, +): Promise { + if (!opts.post || opts.force) return null; + if (!(await alreadyReviewed(env, repo, sha))) return null; + return { + model: opts.model || env.REVIEW_MODEL || REVIEW_MODELS[DEFAULT_MODEL], + summary: "", + findings: [], + unanchored: [], + truncated: false, + posted: false, + sha, + skipped: "already reviewed", + }; } diff --git a/worker/src/tracing.ts b/worker/src/tracing.ts new file mode 100644 index 0000000..eb36efc --- /dev/null +++ b/worker/src/tracing.ts @@ -0,0 +1,66 @@ +// Agent tracing — metadata only. +// +// The review agent owns its own turn (one model call, no AI SDK, no Think or +// Flue), so it is a "custom harness" in Cloudflare's agent-tracing terms: one +// `invoke_agent` span per review with a nested `chat` span for the model call. +// Both carry the three identity attributes the Agents dashboard keys on. +// +// METADATA ONLY, by owner decision. No `gen_ai.input.messages`, +// `gen_ai.output.messages` or `gen_ai.system_instructions` is ever set: the +// prompt is a diff of private work and the response is model prose about it. +// Repository and pull request number are already in every GitHub URL this +// worker touches, and neither identifies a person. +// +// `cloudflare:workers` only exists inside workerd. It is imported lazily and +// a failed import means "no tracing" rather than an error, so the unit tests +// (which run under Node) exercise the same code paths without a tracer. + +type Attributes = Record; + +interface SpanLike { + setAttributes(attributes: Attributes): unknown; +} + +interface TracingLike { + enterSpan(name: string, callback: (span: SpanLike) => T): T; +} + +export const AGENT_NAME = "shamwari-github-mcp"; +/** A single deployed reviewer, so the agent id is as stable as its name. */ +export const AGENT_ID = "shamwari-github-mcp/review"; + +let tracer: Promise | undefined; + +function getTracer(): Promise { + tracer ??= import("cloudflare:workers") + .then((m) => (m as { tracing?: TracingLike }).tracing ?? null) + .catch(() => null); + return tracer; +} + +/** The identity every agent span carries. `conversation` is opaque. */ +export function identity(operation: string, conversation: string): Attributes { + return { + "gen_ai.operation.name": operation, + "gen_ai.agent.name": AGENT_NAME, + "gen_ai.agent.id": AGENT_ID, + "gen_ai.conversation.id": conversation, + }; +} + +/** + * Run `fn` inside a span, or plainly when tracing is unavailable. Only the + * scalar attributes passed here are recorded — never a payload. + */ +export async function inSpan( + name: string, + attributes: Attributes, + fn: (span: SpanLike | null) => Promise, +): Promise { + const t = await getTracer(); + if (!t) return fn(null); + return t.enterSpan(name, (span) => { + span.setAttributes(attributes); + return fn(span); + }); +} diff --git a/worker/test/review.test.ts b/worker/test/review.test.ts index bcd8a69..c565b54 100644 --- a/worker/test/review.test.ts +++ b/worker/test/review.test.ts @@ -11,6 +11,9 @@ import type { Env } from "../src/env"; import { anchor, annotateDiff, + gatewayOptions, + modelOutput, + responseFormat, parseFindings, rank, renderBody, @@ -302,3 +305,66 @@ test("a missing AI binding says what to add rather than throwing on undefined", /wrangler\.toml/, ); }); + +// --- the gateway call ----------------------------------------------------- + +test("gateway metadata attributes cost without naming a person", () => { + const opts = gatewayOptions( + { AI_GATEWAY_ID: "fundi" } as unknown as Env, + "nyuchi/web-services", + { trigger: "mention:someone" }, + ) as { gateway: { id: string; skipCache: boolean; metadata: object } }; + assert.equal(opts.gateway.id, "fundi"); + assert.equal(opts.gateway.skipCache, true); + assert.deepEqual(opts.gateway.metadata, { + worker: "shamwari-github-mcp", + job: "pr_review", + repo: "nyuchi/web-services", + trigger: "mention", + }); + // The gateway accepts at most five metadata entries. + assert.ok(Object.keys(opts.gateway.metadata).length <= 5); +}); + +test("no gateway configured means calling Workers AI directly", () => { + assert.equal(gatewayOptions({} as Env, "nyuchi/web-services", {}), undefined); +}); + +test("asking to post without a head sha fails before the model is paid", async () => { + let calls = 0; + await assert.rejects( + reviewPullRequest( + { + REVIEW_ENABLED: "true", + AI: { + run: async () => { + calls++; + return {}; + }, + }, + } as unknown as Env, + "nyuchi/web-services", + 1, + { post: true }, + ), + /head commit sha/, + ); + assert.equal(calls, 0); +}); + +test("a dynamic route gets the OpenAI response_format and answer shape", () => { + assert.deepEqual( + Object.keys(responseFormat("dynamic/review").json_schema as object), + ["name", "schema"], + ); + assert.equal( + (responseFormat("@cf/zai-org/glm-5.3").json_schema as { type: string }) + .type, + "object", + ); + assert.equal( + modelOutput({ choices: [{ message: { content: "{}" } }] }), + "{}", + ); + assert.deepEqual(modelOutput({ response: { summary: "" } }), { summary: "" }); +}); diff --git a/worker/wrangler.toml b/worker/wrangler.toml index 44219dd..a8f6917 100644 --- a/worker/wrangler.toml +++ b/worker/wrangler.toml @@ -78,6 +78,11 @@ WORKOS_REQUIRED_PERMISSIONS = "mongodb:access" # and $0.003 on the flash variant. At this repository's volume the difference # is a few dollars a year: choose on whether the review is worth reading, not # on price. GLM 5.3 requires a Workers Paid plan. +# +# It may also name a dynamic route on the gateway, e.g. "dynamic/review" on +# fundi, so the model choice lives in gateway configuration. The code +# speaks both request shapes. Switch only once that route exists on the +# gateway: a route that is not there fails every review. REVIEW_MODEL = "@cf/zai-org/glm-5.3" # The handle that summons a review in a pull request comment. A var, not a @@ -91,7 +96,13 @@ REVIEW_MENTION = "@shamwari" # the association in the webhook payload, so it costs no extra call. REVIEW_TRIGGER_ASSOCIATIONS = "OWNER,MEMBER,COLLABORATOR" -# AI Gateway every review's inference is routed through. Each call is logged +# AI Gateway every review's inference is routed through: `fundi`, the +# internal gateway for Nyuchi's own agents and pipelines (owner, 2026-10-04: +# "all agents carry the fundi name"; authenticated, no cache, no guardrails). +# A review is an agent run fired by a GitHub event or a team member's +# mention, not a consumer using an app, so it does not ride the consumer +# `shamwari` gateway, and it is not staff chat (`nyuchi-ai`). The +# binding is pre-authenticated in-account, so no gateway token is stored. Each call is logged # with { repo, trigger } metadata attached, so "what did this repository cost # this month" is a query rather than an estimate — and caching, retries and # rate limits become gateway configuration instead of code in this worker. @@ -101,7 +112,7 @@ REVIEW_TRIGGER_ASSOCIATIONS = "OWNER,MEMBER,COLLABORATOR" # It lives here and not in the dashboard because `wrangler deploy` REPLACES # the whole [vars] block: a value set only in the dashboard is silently # dropped by the next deploy, exactly as the GITHUB_APP_ID note above warns. -AI_GATEWAY_ID = "shamwari" +AI_GATEWAY_ID = "fundi" # KILL SWITCH. Set to exactly "false" to make every review call fail closed # without a code deploy. @@ -155,7 +166,9 @@ head_sampling_rate = 1 invocation_logs = true persist = true +# Traces on (owner decision 2026-10-04): every review is an `invoke_agent` +# span with a nested `chat` span, METADATA ONLY — see src/tracing.ts. [observability.traces] -enabled = false +enabled = true head_sampling_rate = 1 persist = true