Skip to content

fix(review-memory): enforce structured output#4090

Open
alex-alecu wants to merge 1 commit into
mainfrom
review-memory-json-output
Open

fix(review-memory): enforce structured output#4090
alex-alecu wants to merge 1 commit into
mainfrom
review-memory-json-output

Conversation

@alex-alecu

Copy link
Copy Markdown
Contributor

Summary

Review Memory now asks the model for schema-checked JSON instead of trying to recover JSON from plain text. Bad or incomplete replies fail before any GitHub branch or pull request is created, while safe request details make failures easier to trace without storing prompts or model output. Structured-output routing is applied only where the selected provider supports it.

Verification

  1. Approve a Review Memory proposal with a valid model reply and confirm the REVIEW.md pull request opens.
  2. Return malformed or cut-off model output and confirm the proposal becomes retryable without creating a GitHub branch.
  3. Use a direct provider and confirm OpenRouter-only routing settings are not sent upstream.

Visual Changes

N/A

Reviewer Notes

Provider-facing schemas remove unsupported rules, but the complete Zod schemas still validate every model reply inside the app. Failure reports contain only fixed messages, counts, and request IDs; they do not contain prompts, repository content, or raw model output.

return stackFrames ? sanitizeReviewMemoryDiagnosticMessage(stackFrames) : null;
}

function getSafeErrorClass(error: unknown): string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION]: getSafeErrorClass is duplicated from ./llm.ts

This helper is byte-for-byte identical to getSafeErrorClass in ./llm.ts (added in this same PR). Consider exporting it from llm.ts (or a shared review-memory util) and importing it here so the two implementations don't drift apart. The repo's coding style asks to report DRY violations rather than silently duplicate.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

@kilo-code-bot

kilo-code-bot Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

Structured-output enforcement is well built with careful sanitization; the only finding is a minor DRY duplication of getSafeErrorClass across two new files.

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
apps/web/src/lib/code-reviews/review-memory/change-request.ts 453 getSafeErrorClass is duplicated from ./llm.ts
Other Observations (not in diff)

No blocking issues. A few low-confidence notes for the author's consideration:

File Line Note
apps/web/src/lib/trpc/init.ts 21 REQUEST_CORRELATION_ID_PATTERN is very strict; if real x-vercel-id values don't match it, every request silently falls back to randomUUID() and won't correlate with Vercel request tracing. Functionally safe (random fallback), but worth confirming the pattern matches production Vercel IDs so the header is actually used.
apps/web/src/lib/trpc/route-handler callers - Several createTRPCContext() callers (trpc-route-handler.ts, code-indexing/* routes, cloud-agent-fork/review/[reviewId]/route.ts) pass no requestCorrelationId, so they always get a random UUID. Fine for correctness, but they won't share the Vercel trace ID if that's desired later.
apps/web/src/lib/code-reviews/review-memory/llm.ts - No memory leaks introduced: no module-scope caching of clients/promises, jsonSchema/onFinish closures are per-call, and defaultOperations holds only pure function references.
Files Reviewed (14 files)
  • apps/web/src/app/api/trpc/[trpc]/route.ts
  • apps/web/src/lib/ai-gateway/providers/apply-provider-specific-logic.test.ts
  • apps/web/src/lib/ai-gateway/providers/apply-provider-specific-logic.ts
  • apps/web/src/lib/ai-gateway/providers/openrouter/types.ts
  • apps/web/src/lib/code-reviews/review-memory/aggregation.test.ts
  • apps/web/src/lib/code-reviews/review-memory/aggregation.ts
  • apps/web/src/lib/code-reviews/review-memory/change-request.test.ts
  • apps/web/src/lib/code-reviews/review-memory/change-request.ts - 1 issue
  • apps/web/src/lib/code-reviews/review-memory/llm.test.ts
  • apps/web/src/lib/code-reviews/review-memory/llm.ts
  • apps/web/src/lib/code-reviews/review-memory/review-md-integration.test.ts
  • apps/web/src/lib/code-reviews/review-memory/review-md-integration.ts
  • apps/web/src/lib/trpc/init.ts
  • apps/web/src/routers/code-reviews/review-memory-router.ts
  • apps/web/src/routers/test-utils.ts

Fix these issues in Kilo Cloud


Reviewed by glm-5.2-20260616 · 761,644 tokens

Review guidance: REVIEW.md from base branch main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant