feat(ai): auto-evaluation - #156
sishir2001 wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a full auto-evaluation subsystem: configuration, environment validation, evaluation context/builders, prompt/score parsing, retry management, feedback integration, caching, telemetry, and a new Auto-Evaluation middleware wired into the middleware factory and public exports. Also updates types for GenerateResult metadata. Numerous documentation whitespace/formatting cleanups and a new architecture doc. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant MW as AutoEval Middleware
participant Prov as BaseProvider
participant RM as RetryManager
participant Ctx as ContextBuilder
participant Eval as AutoEvaluator
participant Cache as EvaluationCache
participant Tel as TelemetryCollector
participant Err as ErrorHandler
App->>MW: wrapGenerate(options)
MW->>Err: isEvaluationDisabled?
alt Circuit open
MW->>Prov: generate(options)
Prov-->>MW: result
MW-->>App: result (+autoEvaluationMetadata: performed=false, error=true)
else Enabled
MW->>Cache: get(options)
alt Cache hit
Cache-->>MW: cached result
MW-->>App: cached result
else Miss
MW->>RM: retryGeneration(providerWrapper, options)
RM->>Prov: generate(attempt N)
Prov-->>RM: content
RM->>Ctx: build(context)
Ctx-->>RM: EnhancedEvaluationContext
RM->>Eval: evaluate(context)
Eval-->>RM: EvaluationResult
alt Meets threshold
RM-->>MW: RetryResult(success)
MW->>Tel: collectEvaluationEvent(...)
MW->>Cache: set(options, result)
MW-->>App: result (+autoEvaluationMetadata)
else Below threshold
RM->>Prov: (next attempt with enhanced prompt)
Prov-->>RM: content'
RM->>Eval: evaluate(...)
Eval-->>RM: EvaluationResult'
RM-->>MW: final RetryResult(success/failure)
MW-->>App: final result (+autoEvaluationMetadata)
end
end
end
opt Error path
MW->>Err: handleError(e, "wrapGenerate")
MW->>Prov: fallback generate(options)
Prov-->>MW: result
MW-->>App: result (+autoEvaluationMetadata: error)
end
sequenceDiagram
autonumber
participant App as Application
participant MW as AutoEval Middleware
participant Prov as BaseProvider (stream)
participant Eval as AutoEvaluator
participant Err as ErrorHandler
App->>MW: wrapStream(options)
MW->>Prov: stream(options)
Prov-->>MW: stream
MW->>MW: collect streamed chunks
MW-->>App: transformed stream passthrough
MW->>Eval: evaluate(full content) (on flush)
Eval-->>MW: EvaluationResult
alt Below threshold
MW->>MW: log warning (no retry in-stream)
end
MW->>Err: handleError(e, "wrapStream") (if any)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 65
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/types/generateTypes.ts (1)
71-139: Add consumer logic for autoEvaluationMetadata
No code was found reading autoEvaluationMetadata after it’s set in src/lib/middleware/builtin/autoEvaluation.ts; add downstream consumers (e.g., in your telemetry or result-processing layers) to extract and utilize this metadata.src/lib/evaluation/errorHandler.ts (1)
1-86: Wire in env-based circuit breaker config
UseautoEvaluationConfigManagerinErrorHandler(src/lib/evaluation/errorHandler.ts) to initializecircuitBreakerThresholdandcircuitBreakerWindowfrom theNEUROLINK_AUTO_EVAL_CIRCUIT_BREAKER_THRESHOLDandNEUROLINK_AUTO_EVAL_CIRCUIT_BREAKER_RESETenv vars instead of the current hard-coded defaults.src/lib/evaluation/feedbackIntegrator.ts (1)
1-401: Align feedback integrations to useconversationMessagesinstead ofmessages.
TextGenerationOptions(src/lib/core/types.ts #250–251) defines onlyconversationMessages?: ChatMessage[], yet feedbackIntegrator.ts (and related enhancers) reads/writesoptions.messages, which won’t be picked up by the message builder. Replace alloptions.messagesusages in src/lib/evaluation/feedbackIntegrator.ts (and promptEnhancer.ts/aggressiveEnhancement, etc.) withoptions.conversationMessagesso injected system messages are correctly handled.
🧹 Nitpick comments (67)
src/lib/types/generateTypes.ts (1)
107-117: Flatten status fields in autoEvaluationMetadata to a single discriminator.Having both success and error booleans is ambiguous. Prefer a single status plus message; also clarify duration units.
Apply:
- // Auto-evaluation metadata - autoEvaluationMetadata?: { - performed: boolean; - finalScore?: number; - attempts?: number; - duration?: number; - improvement?: number; - success?: boolean; - error?: boolean; - errorMessage?: string; - }; + // Auto-evaluation metadata + autoEvaluationMetadata?: { + performed: boolean; + status?: "success" | "error" | "skipped"; + finalScore?: number; + attempts?: number; + durationMs?: number; + improvement?: number; + errorMessage?: string; + };src/lib/config/types.ts (1)
212-219: DEFAULT_CONFIG: include flags you expose in the interface.If keeping autoRetry/strictMode, set sane defaults here for predictability.
evaluation: { enabled: true, provider: "auto", model: "auto", temperature: 0.3, maxRetries: 3, timeout: 30000, + autoRetry: true, + strictMode: false, },src/lib/evaluation/errorHandler.ts (1)
66-84: Expose circuit state in stats (optional).Helps observability without extra calls.
getStats(): ErrorStats { - return { + return { totalErrors: this.errorCount, errorsByType: { ...this.errorsByType }, lastError: this.lastError, }; }(Add isCircuitOpen?: boolean to ErrorStats if desired.)
src/lib/evaluation/types.ts (2)
41-56: Constrain numeric fields to non-negative.Duration, metadata counters should be >= 0.
export const ExtractedToolExecutionSchema = z.object({ @@ - duration: z.number(), + duration: z.number().nonnegative(), success: z.boolean(), error: z.string().optional(), metadata: z.object({ - timestamp: z.number(), - sequenceNumber: z.number(), - retryCount: z.number(), + timestamp: z.number().int().nonnegative(), + sequenceNumber: z.number().int().nonnegative(), + retryCount: z.number().int().nonnegative(), }), });
72-113: Add a Zod schema for EnhancedEvaluationContext (and maybe EvaluationResult).You validate sub-parts but not the full context/result, leaving a runtime gap.
If helpful, I can draft zod schemas mirroring EnhancedEvaluationContext and EvaluationResult.
src/lib/config/autoEvaluationEnv.ts (4)
8-16: Type the env var registry for safety and editor help.Currently untyped; add a definition and use satisfies for structural checks.
-export const AUTO_EVALUATION_ENV_VARS = { +type EnvVarType = "boolean" | "string" | "number"; +type EnvVarDef = { + description: string; + type: EnvVarType; + default: string; // env values are strings + example: string; + options?: string[]; + range?: [number, number]; +}; + +export const AUTO_EVALUATION_ENV_VARS = { @@ -}; +} satisfies Record<string, EnvVarDef>;
206-213: Accept case-insensitive boolean values.Avoid surprising validation failures for "TRUE"/"False".
- case "boolean": - if (value !== "true" && value !== "false") { + case "boolean": { + const v = value.toLowerCase(); + if (v !== "true" && v !== "false") { errors.push(`${key} must be 'true' or 'false'`); } - break; + break; + }
213-225: Minor: prefer Number.parseFloat/Number.isNaN.Style/readability; consistent with other modules.
- const num = parseFloat(value); - if (isNaN(num)) { + const num = Number.parseFloat(value); + if (Number.isNaN(num)) { errors.push(`${key} must be a valid number`); } else if (config.range) {
149-162: Integer-only keys (optional).Batch/parallel/cache sizes should be integers; consider enforcing int in validation for these keys.
I can add a small allowlist of integer-only var names and check Number.isInteger for them.
src/lib/middleware/factory.ts (3)
40-47: DRY: De-duplicate built-in creators mapbuiltInMiddlewareCreators is defined twice (initialize and getCreator). Use a single shared constant or class field to avoid drift.
Example (top-level constant):
const BUILTIN_CREATORS: Record<string, (config?: Record<string, JsonValue>) => NeuroLinkMiddleware> = { analytics: createAnalyticsMiddleware, guardrails: createGuardrailsMiddleware, "auto-evaluation": createAutoEvaluationMiddleware, };Then use BUILTIN_CREATORS in both places.
Also applies to: 193-203
52-54: Default “always-on” auto-evaluation: confirm operational impactEnabling evaluation by default increases token usage and latency. Confirm SLOs, quotas, and cost budgets are acceptable, or provide an easy opt-out (env or preset). Consider conservative defaults (e.g., lower maxAttempts).
309-311: Use slice or randomUUID; substr is deprecatedMinor: replace substr with slice; optionally prefer crypto.randomUUID when available.
- requestId: `${provider}-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`, + requestId: `${provider}-${Date.now()}-${ + typeof globalThis.crypto?.randomUUID === "function" + ? globalThis.crypto.randomUUID() + : Math.random().toString(36).slice(2, 11) + }`,src/lib/evaluation/conversationEnhancer.ts (1)
33-47: Minor: use a single timestamp for the current turn pairKeeps ordering coherent when comparing user/assistant timestamps.
- // Add current interaction - enhancedTurns.push({ + // Add current interaction + const now = Date.now(); + enhancedTurns.push({ role: "user", content: currentQuery, - timestamp: Date.now(), + timestamp: now, metadata: { tokenCount: this.estimateTokenCount(currentQuery), }, }); @@ enhancedTurns.push({ role: "assistant", content: currentResponse, - timestamp: Date.now(), + timestamp: now, metadata: { tokenCount: metadata?.tokenUsage?.completion || this.estimateTokenCount(currentResponse),src/lib/evaluation/scoreParser.ts (5)
137-141: Prevent premature section termination on capitalized lines with colons.The current “next section” finder can treat a bullet like “- Note: ...” as a header. Anchor to line starts and full-line headers.
- const nextSectionMatch = afterHeader.match(/\n(#{1,3}|[A-Z][A-Za-z\s]+:)/); - const sectionEnd = nextSectionMatch - ? (nextSectionMatch.index ?? 0) - : afterHeader.length; + const nextSectionRegex = /^\s*(?:#{1,6}\s+[^\n]+|[A-Z][A-Za-z\s]+:)\s*$/m; + const nextSectionMatch = nextSectionRegex.exec(afterHeader); + const sectionEnd = nextSectionMatch ? nextSectionMatch.index : afterHeader.length;
200-204: Count atomic statements from analysis sections only.Current heuristic counts every bullet in the whole response, inflating counts. Sum bullets from known analysis sections.
- // Simple heuristic: count bullet points in analysis sections - const bulletMatches = response.match(/^[\s]*[-•*]\s*.+$/gm); - return bulletMatches ? bulletMatches.length : 1; + // Heuristic: bullets within analysis sections only + const sections = ["Strengths:", "Weaknesses:", "Missing Elements:", "Suggestions:"]; + const total = sections + .map((h) => this.extractListSection(response, h).length) + .reduce((a, b) => a + b, 0); + return Math.max(total, 1);
229-233: Make confidence reflect flag completeness, not just object presence.You always award 0.2 since flags object is non-empty. Check expected booleans individually.
- const flagKeys = Object.keys(flags); - confidence += flagKeys.length > 0 ? 0.2 : 0; + const expectedFlags = ["isOffTopic","hasHallucination","hasIncompleteAnswer","hasMisinformation"]; + const presentFlags = expectedFlags.filter((k) => typeof flags[k] === "boolean").length; + confidence += (presentFlags / expectedFlags.length) * 0.2;
64-65: Use central logger instead of console.error.Aligns with observability/telemetry and log levels; avoids leaking raw errors in production logs.
269-293: Dead code: validateScoreConsistency is unused.Either remove it or wire it into the main flow (see suggested hook above).
Would you like me to add unit tests covering inconsistent cases (off-topic with high relevance, hallucination with high accuracy) to guard this logic?
src/lib/evaluation/index.ts (1)
1-6: Consider exporting ScoreParser for advanced consumers.If external integrators need raw parsing (e.g., custom evaluators), re-export it here.
export { ContextBuilder } from "./contextBuilder.js"; export { AutoEvaluator } from "./autoEvaluator.js"; export { RetryManager } from "./retryManager.js"; export { createAutoEvaluationMiddleware } from "../middleware/builtin/autoEvaluation.js"; +export { ScoreParser } from "./scoreParser.js";src/lib/evaluation/autoEvaluationConfig.ts (2)
39-63: Freeze defaults to prevent accidental mutation at runtime.Protects global defaults from being modified by consumers.
-export const DEFAULT_AUTO_EVALUATION_CONFIG: Required<AutoEvaluationConfig> = { +export const DEFAULT_AUTO_EVALUATION_CONFIG: Readonly<Required<AutoEvaluationConfig>> = Object.freeze({ quality: { threshold: 7, strictMode: false, }, retry: { maxAttempts: 3, backoffMultiplier: 1.5, }, evaluationModel: { provider: "", // Will use same as generation provider model: "", // Will auto-select temperature: 0.3, }, performance: { timeout: 120000, cache: true, cacheTTL: 3600000, }, telemetry: { enabled: true, endpoint: "", }, -}; +} as const);
5-37: Add min/max guards (doc or schema) for numeric fields.Downstream config manager should validate threshold (1–10), temperature (0–2), timeout (>0), cacheTTL (>0), attempts (>=1). If not already present, I can add a zod schema.
src/lib/evaluation/retryManager.ts (3)
175-188: Let RetryStateManager compute improvement deltas.Passing zeroes risks masking real deltas if
addAttemptdoesn’t override them. Prefer omitting or computing them correctly.If the type requires the field, compute it:
+ const prev = this.stateManager.getState(stateId)?.attempts.at(-1)?.evaluation.overall ?? evaluation.overall; this.stateManager.addAttempt(stateId, { attemptNumber, timestamp: Date.now(), evaluation, feedback, promptModifications: modifications, responseContent: generateResult.content, - improvementDelta: { - relevance: 0, - accuracy: 0, - completeness: 0, - overall: 0, - }, + improvementDelta: { + relevance: 0, + accuracy: 0, + completeness: 0, + overall: evaluation.overall - prev, + }, });If
addAttemptalready overrides this field, ignore this change.
27-34: Unused config flag.
enableProgressiveFeedbackisn’t used. Remove it or implement behavior.
500-524: Retry statistics won’t accumulate.You clear state in
finally, sogetRetryStatistics()will usually see no completed states. Consider a separate, long-lived metrics accumulator.Also applies to: 131-134
src/lib/evaluation/evaluationTypes.ts (1)
83-137: Nit: rubric key typing.
Record<number, string>is fine, but object keys are strings at runtime. If you want stricter typing, define a union of allowed keys.src/lib/middleware/builtin/autoEvaluation.ts (3)
79-80: Minor: avoid deprecatedsubstr.Use
slicefor ID generation.- const requestId = `ae-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; + const requestId = `ae-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`;
193-253: Ensure TransformStream is available under Node.Add an explicit import from
stream/web(or polyfill) to avoid runtime/typing issues.+import { TransformStream } from "stream/web";
139-147: Telemetry and metadata: use RetryManager timing.Prefer
result.totalDurationfor consistency.Already covered in diff above for metadata. Consider also emitting per-attempt telemetry via
collectRetryEvent.Also applies to: 154-161
src/lib/evaluation/contextBuilder.ts (3)
70-71: Avoid using build-time as responseTime fallback.Falling back to Date.now() - startTime (context build time) misrepresents model latency. Prefer explicit value or 0 when absent.
-const responseTime = result.responseTime || Date.now() - startTime; +const responseTime = result.responseTime ?? 0;
154-161: Prefer explicit systemPrompt from options; fall back to first system message.This keeps generationParams faithful to caller intent.
return { temperature: options.temperature, maxTokens: options.maxTokens, - systemPrompt: this.extractSystemPrompt(options.conversationMessages), + systemPrompt: + options.systemPrompt ?? + this.extractSystemPrompt(options.conversationMessages), };Also applies to: 163-171
212-247: Avoid in-place mutation of context.context in enrichWithDomain.Return a new object to prevent side effects for callers holding references.
- if (domainEnrichments[domain]) { - context.context = { - ...context.context, - domainRequirements: domainEnrichments[domain], - }; - } - return context; + if (!domainEnrichments[domain]) return context; + return { + ...context, + context: { + ...(context.context ?? {}), + domainRequirements: domainEnrichments[domain], + }, + };src/lib/evaluation/feedbackIntegrator.ts (1)
318-353: Parameter tuning is fine; consider provider-aware caps.Max token cap 4000 may be too low/high depending on provider/model limits.
src/lib/evaluation/autoEvaluator.ts (1)
59-63: Make maxTokens configurable instead of hard-coded 2000.Honor config.maxTokens if present; fall back to a sane default.
- maxTokens: 2000, + maxTokens: (this.config as any).maxTokens ?? 2000,src/lib/evaluation/promptEnhancer.ts (4)
157-167: Prefer system/developer note over assistant “I’ll keep in mind” to avoid confusing conversation state.Assistant-role injections can pollute dialogue semantics. Use a system role note.
Apply:
- // Add as single note - this.addNote(enhanced, minimalFeedback); + // Add as single system note + this.addNote(enhanced, minimalFeedback); modifications.push({ type: "context", originalContent: "", modifiedContent: minimalFeedback, reason: "Minimal conservative enhancement", });And update addNote (see below) to push a system role.
343-353: Make addNote a system message to avoid changing the conversation’s assistant persona.This aligns with “instructions” semantics and avoids confusing downstream evaluators.
Apply:
- // Add as assistant message to make it conversational - options.messages.push({ - role: "assistant", - content: `I'll keep in mind: ${note}`, - }); + // Add as system message to set guidance + if (!options.conversationMessages) options.conversationMessages = []; + options.conversationMessages.push({ + role: "system", + content: `Note for this attempt: ${note}`, + });
169-188: Heuristic thresholds: consider bounding previousAttempts and guarding empty/NaN scores.If a.evaluation.overall is missing/NaN, average could be misleading. Clamp to [0,10] and ignore undefined.
206-227: Tone of “FINAL CHANCE” may be undesirable for production.If this middleware is “always-on,” consider configurable tone to avoid harsh language in user-visible traces.
src/lib/evaluation/promptBuilder.ts (2)
114-130: Weights printed as raw floating percentages; format for readability.Apply:
- criteriaText += `### ${criterion.name.charAt(0).toUpperCase() + criterion.name.slice(1)} (Weight: ${criterion.weight * 100}%) + const pct = Math.round(criterion.weight * 100); + criteriaText += `### ${criterion.name.charAt(0).toUpperCase() + criterion.name.slice(1)} (Weight: ${pct}%)
102-112: Protect code blocks from accidental fence termination.If aiResponse contains ``` it can break formatting. Replace triple backticks in content or use a different fence token.
Example:
-\`\`\` -${context.aiResponse} -\`\`\` +```text +${context.aiResponse.replace(/```/g, "``\\`")} +```src/lib/evaluation/queryIntentAnalyzer.ts (2)
296-347: Keep important two-letter acronyms (AI, ML, JS, Go) in keywords.The current length > 2 filter drops “ai”, “ml”, “go”, “js”. Add a whitelist.
Apply:
- .filter((word) => word.length > 2 && !stopWords.has(word)); + .filter((word) => { + const whitelisted = new Set(["ai", "ml", "js", "go", "ui", "ux"]); + return (word.length > 2 || whitelisted.has(word)) && !stopWords.has(word); + });
271-280: Tool requirement heuristic: include “latest version of”, “today”, “current time/date”.Covers common phrasings that imply web/time tools.
Apply:
- /\b(search|find|lookup|current|latest)\b/i, + /\b(search|find|lookup|current|latest|today|now)\b/i, + /\b(latest version|current version)\b/i,src/lib/evaluation/toolExecutionExtractor.ts (2)
82-100: Over-redaction:keymatches “monkey” etc.; use stricter patternsCurrent substring match over-redacts benign fields. Use regex patterns; optionally recurse for nested objects.
Apply:
- const sanitized = { ...input } as Record<string, unknown>; - const sensitiveKeys = ["password", "token", "secret", "key", "credential"]; + const sanitized = { ...(input as Record<string, unknown>) }; + const sensitivePatterns = [ + /password|passphrase/i, + /\b(api|access|private)[-_]?key\b/i, + /secret/i, + /token/i, + /credential/i, + /bearer/i, + ]; Object.keys(sanitized).forEach((key) => { - if ( - sensitiveKeys.some((sensitive) => key.toLowerCase().includes(sensitive)) - ) { + if (sensitivePatterns.some((re) => re.test(key))) { sanitized[key] = "[REDACTED]"; } });
132-144: Dedup key too narrow; prefer toolCallId then content-based fallbackDuplicates can slip through when toolCallId differs between sources. Use toolCallId if present, else include input signature.
Apply:
const seen = new Set<string>(); return executions.filter((execution) => { - const key = `${execution.toolName}-${execution.toolCallId}`; + const key = + execution.toolCallId || + `${execution.toolName}-${JSON.stringify(execution.input)}`; if (seen.has(key)) { return false; } seen.add(key); return true; });src/lib/evaluation/extendedAutoEvaluationConfig.ts (4)
9-16: Avoid config duplication/ambiguity vs base configTop-level
provider,model,temperature,maxRetries,timeoutoverlap with nested fields inAutoEvaluationConfig(evaluationModel,retry,performance). Define precedence or fold into nested blocks to prevent drift.Would you prefer nested-only config (single source of truth) and derive top-level aliases at load time?
19-26: Centralize retry strategy typeExport a shared type to prevent stringly-typed drift across modules.
Apply:
+export type RetryStrategy = + | "STANDARD" + | "AGGRESSIVE" + | "CONSERVATIVE" + | "ADAPTIVE"; ... retryConfig?: { enabled?: boolean; - strategy?: "STANDARD" | "AGGRESSIVE" | "CONSERVATIVE" | "ADAPTIVE"; + strategy?: RetryStrategy;
55-95: Make defaults immutable at type-level (and document units)Prevent accidental mutation of shared defaults and clarify ms units.
Apply:
-export const DEFAULT_EXTENDED_AUTO_EVALUATION_CONFIG: ExtendedAutoEvaluationConfig = - { +// Note: timeouts/ttls are milliseconds. +export const DEFAULT_EXTENDED_AUTO_EVALUATION_CONFIG = + { enabled: true, provider: "auto", model: "auto", temperature: 0.3, maxRetries: 3, timeout: 30000, @@ parallelLimit: 3, - }; + } as const satisfies ExtendedAutoEvaluationConfig;Optionally freeze at runtime where loaded if mutation is a concern.
58-66: Always-on default: add a kill switchGiven this middleware is enabled by default, consider honoring an env var (e.g., NEUROLINK_AUTO_EVAL_ENABLED=false) to quickly disable in prod incidents.
src/lib/evaluation/feedbackGenerator.ts (3)
213-240: Prioritization rarely triggers; base it on severity keywordsSorting by “accuracy/relevance” substrings misses most entries. Rank by broader patterns.
Apply:
- return unique.sort((a, b) => { - // Hallucination fixes are highest priority - if (a.includes("hallucin") && !b.includes("hallucin")) { - return -1; - } - if (!a.includes("hallucin") && b.includes("hallucin")) { - return 1; - } - - // Accuracy fixes are next priority - if (a.includes("accuracy") && !b.includes("accuracy")) { - return -1; - } - if (!a.includes("accuracy") && b.includes("accuracy")) { - return 1; - } - - // Then relevance - if (a.includes("relevance") && !b.includes("relevance")) { - return -1; - } - if (!a.includes("relevance") && b.includes("relevance")) { - return 1; - } - - return 0; - }); + const rank = (s: string) => { + if (/hallucin/i.test(s)) return 0; + if (/accuracy|fact|source/i.test(s)) return 1; + if (/relevance|off-?topic/i.test(s)) return 2; + if (/completeness|missing/i.test(s)) return 3; + return 9; + }; + return unique.sort((a, b) => rank(a) - rank(b));
110-135: Constraints should reflect final-attempt statusHardcoding “FINAL attempt” for attempts ≥3 may misalign with configurable max retries. Consider passing
isFinalAttempt(or maxAttempts) to tailor constraints.Would you like a follow-up PR to plumb this from config?
243-259: Skip empty sections in retry system promptAvoid noisy blank sections when lists are empty.
Apply:
- generateRetrySystemPrompt(feedback: RetryFeedback): string { - return `IMPORTANT: This is retry attempt ${feedback.attempt}. You MUST address these specific issues: - -${feedback.specificIssues.map((issue, i) => `${i + 1}. ${issue}`).join("\n")} - -Required Improvements: -${feedback.requiredImprovements.map((imp, i) => `${i + 1}. ${imp}`).join("\n")} - -Constraints for this attempt: -${feedback.constraints.map((constraint, i) => `${i + 1}. ${constraint}`).join("\n")} - -Focus especially on: -${feedback.focusAreas.map((area, i) => `${i + 1}. ${area}`).join("\n")} - -Your response MUST score 7+ on all evaluation criteria to be acceptable.`; - } + generateRetrySystemPrompt(feedback: RetryFeedback): string { + const parts: string[] = [ + `IMPORTANT: This is retry attempt ${feedback.attempt}.`, + ]; + if (feedback.specificIssues.length) { + parts.push( + "You MUST address these specific issues:", + feedback.specificIssues.map((issue, i) => `${i + 1}. ${issue}`).join("\n"), + ); + } + if (feedback.requiredImprovements.length) { + parts.push( + "Required Improvements:", + feedback.requiredImprovements.map((imp, i) => `${i + 1}. ${imp}`).join("\n"), + ); + } + if (feedback.constraints.length) { + parts.push( + "Constraints for this attempt:", + feedback.constraints.map((c, i) => `${i + 1}. ${c}`).join("\n"), + ); + } + if (feedback.focusAreas.length) { + parts.push( + "Focus especially on:", + feedback.focusAreas.map((area, i) => `${i + 1}. ${area}`).join("\n"), + ); + } + parts.push("Your response MUST score 7+ on all evaluation criteria to be acceptable."); + return parts.join("\n\n"); + }src/lib/evaluation/retryTypes.ts (3)
55-61: Consider promoting string unions to named enums.
modificationStrategy?: "aggressive" | "moderate" | "minimal";is fine, but aligning with a dedicated enum (similar toRetryStrategy) reduces drift and typos across modules.
75-83: MakepreviousFeedbackoptional insideretryMetadata.Some retry paths may not have prior feedback; making it optional avoids forcing callers to fabricate empty objects.
export interface EnhancedRetryOptions extends TextGenerationOptions { retryMetadata?: { attemptNumber: number; - previousFeedback: RetryFeedback; + previousFeedback?: RetryFeedback; focusAreas: string[]; mandatoryImprovements: string[]; }; }
85-98: Model failure-case flexibility inRetryResult.When
success === false,finalContentand/orfinalEvaluationmay be absent. Consider optional fields or clearly document sentinel values.src/lib/config/autoEvaluationConfigManager.ts (2)
201-206: Return a deep copy to avoid accidental external mutation.Shallow spread won’t protect nested objects.
- getConfig(): AutoEvaluationConfig { - return { ...this.config }; - } + getConfig(): AutoEvaluationConfig { + return JSON.parse(JSON.stringify(this.config)); + }
377-388: Useperformance.cacheand avoid non-existentretry.strategy.Adjust recommendations to the schema; drop strategy tip unless you add it under
retry.- if (!this.config.cache.enabled) { + if (!this.config.performance.cache) { recommendations.push( "Enable caching to improve performance for repeated evaluations", ); } - if (this.config.retryConfig.strategy === "STANDARD") { - recommendations.push( - "Consider using ADAPTIVE retry strategy for better performance", - ); - }src/lib/evaluation/telemetryCollector.ts (5)
21-25: Add an in-flight flush guard to prevent overlapping flushes.Avoid concurrent flushes from timer and size-triggered flush.
export class TelemetryCollector { private events: TelemetryEvent[] = []; private flushInterval?: NodeJS.Timeout; private config: Required<TelemetryConfig>; + private isFlushing = false;
99-106: Don’t keep the process alive solely for telemetry; unref the timer.This lets Node exit naturally.
private startBatchProcessing(): void { - this.flushInterval = setInterval(() => { + this.flushInterval = setInterval(() => { if (this.events.length > 0) { this.flush(); } - }, this.config.flushInterval); + }, this.config.flushInterval); + // Do not keep the event loop alive just for telemetry + this.flushInterval.unref?.(); }
107-123: Make flush re-entrant safe and avoid event loss on failure.Add a simple lock; keep existing requeue behavior.
private async flush(): Promise<void> { - if (this.events.length === 0 || !this.config.endpoint) { + if (this.isFlushing || this.events.length === 0 || !this.config.endpoint) { return; } - const eventsToFlush = [...this.events]; - this.events = []; + this.isFlushing = true; + const eventsToFlush = this.events.splice(0, this.events.length); try { await this.sendEvents(eventsToFlush); logger.debug(`[Telemetry] Flushed ${eventsToFlush.length} events`); } catch (error) { logger.error(`[Telemetry] Failed to flush events:`, error); // Re-add events for retry - this.events.unshift(...eventsToFlush); + this.events.unshift(...eventsToFlush); } + this.isFlushing = false; }
133-138: Await the final flush in destroy().Ensure events aren’t lost during shutdown.
- destroy(): void { + async destroy(): Promise<void> { if (this.flushInterval) { clearInterval(this.flushInterval); } - this.flush(); + await this.flush(); }
140-169: Metrics are computed from the in-memory queue only.After a flush,
getMetrics()returns zeros. Either maintain rolling counters or document this behavior. Optional to add a lightweight accumulator.src/lib/evaluation/retryStateManager.ts (2)
222-233: Make average improvement fallback neutral (0) instead of optimistic (1).This keeps ETA conservative when there’s insufficient data.
Apply:
private calculateAverageImprovement(scores: number[]): number { if (scores.length < 2) { - return 1; + return 0; }
243-260: Improve summary messaging to reflect status explicitly and avoid implying “In Progress” on failure.Current string shows “In Progress” when finalResult is absent, even if status is failed/timeout.
Apply:
return `Retry Summary: - Total Attempts: ${state.currentAttempt} - Duration: ${(duration / 1000).toFixed(1)}s - Status: ${state.status} - Score Progression: ${progress.scoreHistory.overall.map((s) => s.toFixed(1)).join(" → ")} - Trend: ${progress.improvementTrend} -- Final Result: ${state.finalResult ? `Success (${state.finalResult.evaluation.overall}/10)` : "In Progress"}`; +- Final Status: ${state.status}${state.finalResult ? ` (${state.finalResult.evaluation.overall}/10)` : ""}`;docs/auto-evaluation/AUTO-EVALUATION-ARCHITECTURE.md (5)
438-447: Align “always-on by default” with the code snippet.Doc says no feature flags; snippet gates on disableAutoEvaluation. Recommend default-on with opt-out, not opt-in.
Apply:
-// In BaseProvider constructor or factory -if (!options.disableAutoEvaluation) { - this.middleware.use( - createAutoEvaluationMiddleware({ - threshold: 7, - maxRetries: 3, - evaluationModel: process.env.NEUROLINK_EVALUATION_MODEL, - }), - ); -} +// In BaseProvider constructor or factory (enabled by default; allow explicit opt-out) +this.middleware.use( + createAutoEvaluationMiddleware({ + threshold: Number(process.env.NEUROLINK_EVALUATION_THRESHOLD ?? 7), + maxRetries: Number(process.env.NEUROLINK_MAX_EVAL_RETRIES ?? 3), + evaluationModel: process.env.NEUROLINK_EVALUATION_MODEL, + // If you still want an escape hatch: + disabled: process.env.NEUROLINK_AUTO_EVALUATION_ENABLED === "false" || options?.disableAutoEvaluation === true, + }), +);
452-456: Fix .env formatting (no spaces) and clarify boolean values.Current examples include spaces; some parsers will reject them.
Apply:
-NEUROLINK_AUTO_EVALUATION_ENABLED = true; -NEUROLINK_EVALUATION_THRESHOLD = 7; -NEUROLINK_MAX_EVAL_RETRIES = 3; +NEUROLINK_AUTO_EVALUATION_ENABLED=true +NEUROLINK_EVALUATION_THRESHOLD=7 +NEUROLINK_MAX_EVAL_RETRIES=3
228-231: Use the concrete error name used elsewhere (QualityAssuranceError).Consistency helps operators correlate logs/errors.
Apply:
-Max Retries → Quality Error → Detailed Error Information +Max Retries → QualityAssuranceError → Detailed Error Information
544-546: Consistent error naming in error-handling diagram.“QualityError” vs “QualityAssuranceError”.
Apply:
- B -->|Quality Failure| F[Throw QualityError] + B -->|Quality Failure| F[Throw QualityAssuranceError]
595-600: Nit: minor wording polish.Consider “LLM judge” or “evaluation model” consistently; “judge LLM” is used elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
CHANGELOG.md(0 hunks)docs/CONTEXT-SUMMARIZATION.md(0 hunks)docs/DYNAMIC-MODELS.md(0 hunks)docs/MCP-CONFIGURATION-LOCATIONS.md(0 hunks)docs/PERFORMANCE-OPTIMIZATION.md(0 hunks)docs/REAL-TIME-SPEECH-AGENTS.md(0 hunks)docs/TESTING.md(0 hunks)docs/TROUBLESHOOTING.md(0 hunks)docs/advanced/dynamic-models.md(0 hunks)docs/analysis/MASTER_NEUROLINK_COMPLETE_ANALYSIS.md(0 hunks)docs/analysis/VERIFICATION_RESULTS.md(0 hunks)docs/auto-evaluation/AUTO-EVALUATION-ARCHITECTURE.md(1 hunks)docs/demos/interactive.md(0 hunks)docs/demos/screenshots.md(0 hunks)docs/development/cli-factory-impact-assessment.md(0 hunks)docs/development/package-overrides.md(0 hunks)docs/development/testing.md(0 hunks)docs/getting-started/environment-variables.md(0 hunks)docs/reference/troubleshooting.md(0 hunks)docs/test-reports/phase-1-2-completion-report.md(0 hunks)docs/test-reports/visual-content-documentation-update-summary.md(0 hunks)docs/tracking/CLI_OPTIMIZATION_TRACKING.md(0 hunks)docs/tracking/IMMEDIATE_WORK_PLAN.md(0 hunks)docs/visual-content/phase-1-2-visual-content-achievement.md(0 hunks)docs/visual-content/phase-1-2-workflow-tools-plan.md(0 hunks)examples/sagemaker/README.md(0 hunks)src/lib/config/autoEvaluationConfigManager.ts(1 hunks)src/lib/config/autoEvaluationEnv.ts(1 hunks)src/lib/config/types.ts(3 hunks)src/lib/evaluation/autoEvaluationConfig.ts(1 hunks)src/lib/evaluation/autoEvaluator.ts(1 hunks)src/lib/evaluation/contextBuilder.ts(1 hunks)src/lib/evaluation/conversationEnhancer.ts(1 hunks)src/lib/evaluation/errorHandler.ts(1 hunks)src/lib/evaluation/evaluationCache.ts(1 hunks)src/lib/evaluation/evaluationTypes.ts(1 hunks)src/lib/evaluation/extendedAutoEvaluationConfig.ts(1 hunks)src/lib/evaluation/feedbackGenerator.ts(1 hunks)src/lib/evaluation/feedbackIntegrator.ts(1 hunks)src/lib/evaluation/index.ts(1 hunks)src/lib/evaluation/promptBuilder.ts(1 hunks)src/lib/evaluation/promptEnhancer.ts(1 hunks)src/lib/evaluation/queryIntentAnalyzer.ts(1 hunks)src/lib/evaluation/retryManager.ts(1 hunks)src/lib/evaluation/retryStateManager.ts(1 hunks)src/lib/evaluation/retryTypes.ts(1 hunks)src/lib/evaluation/scoreParser.ts(1 hunks)src/lib/evaluation/telemetryCollector.ts(1 hunks)src/lib/evaluation/toolExecutionExtractor.ts(1 hunks)src/lib/evaluation/types.ts(1 hunks)src/lib/index.ts(1 hunks)src/lib/middleware/builtin/autoEvaluation.ts(1 hunks)src/lib/middleware/builtin/index.ts(1 hunks)src/lib/middleware/factory.ts(3 hunks)src/lib/types/generateTypes.ts(1 hunks)
💤 Files with no reviewable changes (25)
- docs/tracking/CLI_OPTIMIZATION_TRACKING.md
- docs/TROUBLESHOOTING.md
- docs/demos/screenshots.md
- docs/development/package-overrides.md
- docs/development/cli-factory-impact-assessment.md
- docs/REAL-TIME-SPEECH-AGENTS.md
- examples/sagemaker/README.md
- docs/DYNAMIC-MODELS.md
- docs/visual-content/phase-1-2-workflow-tools-plan.md
- docs/reference/troubleshooting.md
- docs/MCP-CONFIGURATION-LOCATIONS.md
- docs/getting-started/environment-variables.md
- docs/analysis/VERIFICATION_RESULTS.md
- docs/analysis/MASTER_NEUROLINK_COMPLETE_ANALYSIS.md
- docs/advanced/dynamic-models.md
- docs/TESTING.md
- docs/development/testing.md
- docs/test-reports/visual-content-documentation-update-summary.md
- docs/tracking/IMMEDIATE_WORK_PLAN.md
- docs/visual-content/phase-1-2-visual-content-achievement.md
- CHANGELOG.md
- docs/CONTEXT-SUMMARIZATION.md
- docs/test-reports/phase-1-2-completion-report.md
- docs/demos/interactive.md
- docs/PERFORMANCE-OPTIMIZATION.md
🧰 Additional context used
🧬 Code graph analysis (25)
src/lib/evaluation/queryIntentAnalyzer.ts (2)
examples/quick-start/evaluation-demo.js (1)
query(17-17)src/lib/evaluation/types.ts (1)
QueryIntentAnalysis(4-13)
src/lib/middleware/builtin/autoEvaluation.ts (9)
src/lib/evaluation/autoEvaluationConfig.ts (2)
AutoEvaluationConfig(5-37)DEFAULT_AUTO_EVALUATION_CONFIG(40-63)src/lib/evaluation/contextBuilder.ts (2)
contextBuilder(251-251)ContextBuilder(9-248)src/lib/evaluation/autoEvaluator.ts (1)
AutoEvaluator(14-243)src/lib/evaluation/retryManager.ts (1)
RetryManager(20-525)src/lib/evaluation/evaluationCache.ts (1)
EvaluationCache(7-72)src/lib/evaluation/telemetryCollector.ts (1)
TelemetryCollector(21-171)src/lib/evaluation/errorHandler.ts (1)
ErrorHandler(13-85)src/lib/types/middlewareTypes.ts (1)
NeuroLinkMiddlewareMetadata(8-21)src/lib/utils/logger.ts (2)
logger(341-380)error(223-225)
src/lib/evaluation/feedbackIntegrator.ts (3)
src/lib/types/generateTypes.ts (1)
TextGenerationOptions(179-215)src/lib/evaluation/evaluationTypes.ts (1)
RetryFeedback(66-72)src/lib/evaluation/retryTypes.ts (2)
RetryAttempt(5-18)PromptModification(21-26)
src/lib/evaluation/retryTypes.ts (3)
src/lib/evaluation/types.ts (1)
EvaluationResult(116-134)src/lib/evaluation/evaluationTypes.ts (1)
RetryFeedback(66-72)src/lib/types/generateTypes.ts (1)
TextGenerationOptions(179-215)
src/lib/evaluation/feedbackGenerator.ts (2)
src/lib/evaluation/evaluationTypes.ts (2)
StructuredEvaluation(33-63)RetryFeedback(66-72)src/lib/evaluation/types.ts (1)
EnhancedEvaluationContext(73-113)
src/lib/evaluation/promptBuilder.ts (2)
src/lib/evaluation/types.ts (1)
EnhancedEvaluationContext(73-113)src/lib/evaluation/evaluationTypes.ts (4)
EvaluationCriteria(4-9)CORE_CRITERIA(84-137)RetryFeedback(66-72)EvaluationPromptSections(21-30)
src/lib/evaluation/promptEnhancer.ts (3)
src/lib/types/generateTypes.ts (1)
TextGenerationOptions(179-215)src/lib/evaluation/evaluationTypes.ts (1)
RetryFeedback(66-72)src/lib/evaluation/retryTypes.ts (2)
RetryAttempt(5-18)PromptModification(21-26)
src/lib/evaluation/autoEvaluator.ts (5)
src/lib/evaluation/promptBuilder.ts (1)
EvaluationPromptBuilder(9-217)src/lib/evaluation/scoreParser.ts (1)
ScoreParser(5-293)src/lib/evaluation/feedbackGenerator.ts (1)
FeedbackGenerator(4-260)src/lib/evaluation/evaluationTypes.ts (4)
RetryFeedback(66-72)CORE_CRITERIA(84-137)EvaluationCriteria(4-9)StructuredEvaluation(33-63)src/lib/evaluation/types.ts (2)
EnhancedEvaluationContext(73-113)EvaluationResult(116-134)
src/lib/evaluation/retryManager.ts (8)
src/lib/evaluation/retryStateManager.ts (1)
RetryStateManager(11-261)src/lib/evaluation/feedbackIntegrator.ts (1)
FeedbackIntegrator(9-400)src/lib/evaluation/promptEnhancer.ts (1)
PromptEnhancer(9-354)src/lib/evaluation/contextBuilder.ts (1)
ContextBuilder(9-248)src/lib/evaluation/autoEvaluator.ts (1)
AutoEvaluator(14-243)src/lib/evaluation/retryTypes.ts (6)
RetryConfiguration(29-36)RetryResult(86-98)PromptModification(21-26)RetryAttempt(5-18)RetryState(39-52)RetryDecision(55-60)src/lib/types/generateTypes.ts (1)
TextGenerationOptions(179-215)src/lib/evaluation/types.ts (1)
EvaluationResult(116-134)
src/lib/evaluation/evaluationTypes.ts (2)
src/lib/evaluation/index.ts (2)
StructuredEvaluation(17-17)RetryFeedback(17-17)src/lib/config/types.ts (1)
EvaluationConfig(106-115)
src/lib/evaluation/contextBuilder.ts (6)
src/lib/evaluation/queryIntentAnalyzer.ts (1)
QueryIntentAnalyzer(8-371)src/lib/evaluation/toolExecutionExtractor.ts (1)
ToolExecutionExtractor(5-182)src/lib/evaluation/conversationEnhancer.ts (1)
ConversationEnhancer(4-246)src/lib/types/generateTypes.ts (2)
GenerateResult(71-139)TextGenerationOptions(179-215)src/lib/evaluation/types.ts (2)
EvaluationResult(116-134)EnhancedEvaluationContext(73-113)src/lib/types/conversation.ts (1)
ChatMessage(81-87)
src/lib/config/autoEvaluationEnv.ts (1)
src/lib/core/modelConfiguration.ts (1)
parseFloat(616-622)
src/lib/evaluation/extendedAutoEvaluationConfig.ts (1)
src/lib/evaluation/autoEvaluationConfig.ts (1)
AutoEvaluationConfig(5-37)
src/lib/evaluation/conversationEnhancer.ts (2)
src/lib/types/conversation.ts (1)
ChatMessage(81-87)src/lib/evaluation/types.ts (1)
EnhancedConversationTurn(58-70)
src/lib/evaluation/errorHandler.ts (1)
src/lib/utils/logger.ts (2)
error(223-225)logger(341-380)
src/lib/evaluation/evaluationCache.ts (1)
src/lib/utils/logger.ts (1)
logger(341-380)
src/lib/config/types.ts (1)
src/lib/evaluation/evaluationTypes.ts (1)
EvaluationConfig(75-81)
src/lib/config/autoEvaluationConfigManager.ts (2)
src/lib/evaluation/autoEvaluationConfig.ts (2)
AutoEvaluationConfig(5-37)DEFAULT_AUTO_EVALUATION_CONFIG(40-63)src/lib/utils/logger.ts (1)
logger(341-380)
src/lib/middleware/factory.ts (3)
src/lib/evaluation/index.ts (1)
createAutoEvaluationMiddleware(5-5)src/lib/middleware/builtin/autoEvaluation.ts (1)
createAutoEvaluationMiddleware(21-260)src/lib/middleware/builtin/index.ts (1)
createAutoEvaluationMiddleware(4-4)
src/lib/evaluation/types.ts (1)
src/lib/evaluation/index.ts (5)
QueryIntentAnalysis(13-13)ExtractedToolExecution(14-14)EnhancedConversationTurn(15-15)EnhancedEvaluationContext(11-11)EvaluationResult(12-12)
src/lib/evaluation/toolExecutionExtractor.ts (2)
src/lib/types/generateTypes.ts (1)
GenerateResult(71-139)src/lib/evaluation/types.ts (1)
ExtractedToolExecution(42-55)
src/lib/evaluation/scoreParser.ts (1)
src/lib/evaluation/evaluationTypes.ts (2)
StructuredEvaluation(33-63)StructuredEvaluationSchema(140-166)
src/lib/evaluation/autoEvaluationConfig.ts (3)
src/lib/evaluation/index.ts (2)
AutoEvaluationConfig(8-8)DEFAULT_AUTO_EVALUATION_CONFIG(9-9)src/lib/index.ts (2)
AutoEvaluationConfig(71-71)DEFAULT_AUTO_EVALUATION_CONFIG(72-72)src/lib/middleware/builtin/index.ts (1)
AutoEvaluationConfig(7-7)
src/lib/evaluation/telemetryCollector.ts (2)
src/lib/evaluation/retryTypes.ts (1)
RetryResult(86-98)src/lib/utils/logger.ts (2)
logger(341-380)error(223-225)
src/lib/evaluation/retryStateManager.ts (3)
src/lib/evaluation/retryTypes.ts (5)
RetryState(39-52)RetryConfiguration(29-36)RetryAttempt(5-18)RetryDecision(55-60)RetryProgress(63-73)src/lib/types/generateTypes.ts (1)
TextGenerationOptions(179-215)src/lib/evaluation/types.ts (1)
EvaluationResult(116-134)
🔇 Additional comments (13)
src/lib/config/types.ts (1)
13-18: Addition of evaluation config entry in NeuroLinkConfig looks good.No issues with the optional field introduction.
src/lib/middleware/factory.ts (2)
52-54: Double-generation cost in Auto-Evaluation flow (cross-file)autoEvaluation.ts calls doGenerate within retry flow and again afterward to build the final result, doubling latency/cost. Make the final result reuse the last attempt instead of calling doGenerate again. Attaching a minimal fix for autoEvaluation.ts:
- // Get the actual generation result with enhanced metadata - const finalResult = await doGenerate(); - const enhancedResult = { - ...finalResult, - text: result.finalContent || finalResult.text, + // Reuse the last attempt result from retryManager/providerWrapper + // Capture lastRawResult in closure inside providerWrapper.generate + const enhancedResult = { + ...lastRawResult, + text: result.finalContent || lastRawResult?.text, autoEvaluationMetadata: { performed: true, finalScore: result.finalEvaluation.overall, attempts: result.attempts.length, duration: Date.now() - startTime, improvement: result.averageScoreImprovement, success: result.success, }, };If helpful, I can provide the full patch (adds a lastRawResult variable captured by providerWrapper.generate and used here).
Likely an incorrect or invalid review comment.
140-149: Clarify per-factory registry vs. per-call mutation
MiddlewareRegistry is instantiated once per factory (not a global singleton), so factory-to-factory bleed isn’t a concern. However, applyMiddleware mutates that shared registry on each call, risking cross-request config bleed if the same factory handles concurrent calls. Either stem the mutation—e.g. buildChain should apply config without calling register(…, {replace:true})—or isolate per-call state (clone the registry or use fresh instances) to guarantee safety under concurrent use.src/lib/index.ts (1)
69-73: LGTM: clean public exports for auto-evaluationExports are consistent with the new middleware and types.
src/lib/middleware/builtin/index.ts (1)
1-7: LGTM: coherent built-in middleware barrelCentralizes exports; matches root index additions.
src/lib/evaluation/index.ts (1)
7-22: Barrel exports look correct.Type-only exports are appropriately marked; no tree-shaking pitfalls spotted.
src/lib/evaluation/evaluationTypes.ts (1)
139-166: Schema looks solid.Runtime validation for
StructuredEvaluationis well specified.src/lib/evaluation/contextBuilder.ts (1)
173-210: Minimal-context helper looks solid.Good utility for tests and scaffolding. No issues spotted.
src/lib/evaluation/feedbackIntegrator.ts (1)
9-45: API surface looks good.Mode routing and return shape are clean.
src/lib/evaluation/autoEvaluator.ts (1)
135-157: Conversion mapping looks correct.Fields and metadata are mapped as expected.
src/lib/evaluation/promptBuilder.ts (1)
160-198: Great structure and explicit format block.Clear instructions and fixed output schema will help downstream parsers.
src/lib/evaluation/queryIntentAnalyzer.ts (1)
164-178: Sanity-check: analyzer class looks solid overall.Deterministic heuristics, precompiled regexes, and clear outputs. Nice.
src/lib/config/autoEvaluationConfigManager.ts (1)
96-130: Incorrect suggestion — minScoreThresholds exists on ExtendedAutoEvaluationConfig
minScoreThresholdsis declared in ExtendedAutoEvaluationConfig (see src/lib/evaluation/extendedAutoEvaluationConfig.ts), so these environment overrides align with the schema.Likely an incorrect or invalid review comment.
| /** | ||
| * Evaluation configuration | ||
| */ | ||
| export interface EvaluationConfig { | ||
| enabled?: boolean; | ||
| provider?: string; | ||
| model?: string; | ||
| temperature?: number; | ||
| maxRetries?: number; | ||
| timeout?: number; | ||
| autoRetry?: boolean; | ||
| strictMode?: boolean; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify EvaluationConfig type to avoid drift with evaluation/evaluationTypes.ts.
This duplicates a similar interface elsewhere and will diverge. Reuse the core type and extend locally only for extras.
-export interface EvaluationConfig {
- enabled?: boolean;
- provider?: string;
- model?: string;
- temperature?: number;
- maxRetries?: number;
- timeout?: number;
- autoRetry?: boolean;
- strictMode?: boolean;
-}
+import type {
+ EvaluationConfig as CoreEvaluationConfig,
+} from "../evaluation/evaluationTypes.js";
+
+export type EvaluationConfig = CoreEvaluationConfig & {
+ enabled?: boolean;
+ autoRetry?: boolean;
+ strictMode?: boolean;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Evaluation configuration | |
| */ | |
| export interface EvaluationConfig { | |
| enabled?: boolean; | |
| provider?: string; | |
| model?: string; | |
| temperature?: number; | |
| maxRetries?: number; | |
| timeout?: number; | |
| autoRetry?: boolean; | |
| strictMode?: boolean; | |
| } | |
| /** | |
| * Evaluation configuration | |
| */ | |
| import type { | |
| EvaluationConfig as CoreEvaluationConfig, | |
| } from "../evaluation/evaluationTypes.js"; | |
| export type EvaluationConfig = CoreEvaluationConfig & { | |
| enabled?: boolean; | |
| autoRetry?: boolean; | |
| strictMode?: boolean; | |
| }; |
🤖 Prompt for AI Agents
In src/lib/config/types.ts around lines 103 to 116, the EvaluationConfig
interface duplicates the core evaluation type and risks divergence; remove this
duplicate definition, import the canonical type from
evaluation/evaluationTypes.ts (e.g. import { EvaluationConfig as
CoreEvaluationConfig } from '.../evaluation/evaluationTypes'), and re-export or
extend it locally for any extra fields (e.g. export type EvaluationConfig =
CoreEvaluationConfig & { /* local extras */ } or export interface
EvaluationConfig extends CoreEvaluationConfig { /* extras */ }); ensure all
existing optional fields remain available via the imported type and update any
local references to use the unified type.
| import type { EnhancedEvaluationContext, EvaluationResult } from "./types.js"; | ||
| import type { | ||
| StructuredEvaluation, | ||
| EvaluationConfig, | ||
| RetryFeedback, | ||
| } from "./evaluationTypes.js"; | ||
| import { EvaluationPromptBuilder } from "./promptBuilder.js"; | ||
| import { ScoreParser } from "./scoreParser.js"; | ||
| import { FeedbackGenerator } from "./feedbackGenerator.js"; | ||
| import { AIProviderFactory } from "../core/factory.js"; | ||
| import type { BaseProvider } from "../core/baseProvider.js"; | ||
| import { CORE_CRITERIA } from "./evaluationTypes.js"; | ||
|
|
There was a problem hiding this comment.
Missing import for EvaluationCriteria used in buildPrompt signature.
This will fail type-checking.
-import type {
- StructuredEvaluation,
- EvaluationConfig,
- RetryFeedback,
-} from "./evaluationTypes.js";
+import type {
+ StructuredEvaluation,
+ EvaluationConfig,
+ RetryFeedback,
+ EvaluationCriteria,
+} from "./evaluationTypes.js";Also applies to: 122-133
🤖 Prompt for AI Agents
In src/lib/evaluation/autoEvaluator.ts around lines 1-13 and also where
referenced around 122-133, the type EvaluationCriteria is used in the
buildPrompt signature but not imported, causing type-check failures; add an
import for EvaluationCriteria from "./evaluationTypes.js" (alongside the
existing imports from that module) and ensure any usages reference the imported
type name so TypeScript can resolve the type.
| constructor(config: EvaluationConfig = {}) { | ||
| this.promptBuilder = new EvaluationPromptBuilder(); | ||
| this.scoreParser = new ScoreParser(); | ||
| this.feedbackGenerator = new FeedbackGenerator(); | ||
| this.config = { | ||
| provider: | ||
| config.provider || | ||
| process.env.NEUROLINK_EVALUATION_PROVIDER || | ||
| "google-ai", | ||
| model: | ||
| config.model || | ||
| process.env.NEUROLINK_EVALUATION_MODEL || | ||
| "gemini-2.0-flash", | ||
| temperature: config.temperature || 0.3, | ||
| maxRetries: config.maxRetries || 3, | ||
| timeout: config.timeout || 30000, | ||
| ...config, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate EvaluationConfig type presence and defaults.
Ensure EvaluationConfig includes provider, model, temperature, maxRetries, timeout; otherwise spread may mask missing fields.
🏁 Script executed:
#!/bin/bash
# Confirm EvaluationConfig declaration and fields
rg -nP 'interface\s+EvaluationConfig|type\s+EvaluationConfig' -C3 --type tsLength of output: 747
Add missing maxRetries and timeout to EvaluationConfig.
In src/lib/evaluation/evaluationTypes.ts, the EvaluationConfig interface only defines provider, model, and temperature; add
maxRetries?: number;
timeout?: number;so the constructor’s defaults are fully declared and type-checked.
🤖 Prompt for AI Agents
In src/lib/evaluation/evaluationTypes.ts around where EvaluationConfig is
defined (referenced by src/lib/evaluation/autoEvaluator.ts lines 21-39), the
interface is missing the optional maxRetries and timeout properties referenced
by the constructor defaults; update the EvaluationConfig interface to include
maxRetries?: number; and timeout?: number; so the constructor’s defaulting is
type-checked and the config object accepts those numeric options.
| import type { ChatMessage } from "../types/conversationTypes.js"; | ||
| import type { GenerateResult } from "../types/generateTypes.js"; | ||
| import type { TextGenerationOptions } from "../core/types.js"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix type import paths and unify TextGenerationOptions source.
- ChatMessage appears to live at src/lib/types/conversation.ts; current path "../types/conversationTypes.js" likely breaks.
- TextGenerationOptions is defined in src/lib/types/generateTypes.ts; importing from "../core/types.js" risks type drift.
Apply:
-import type { ChatMessage } from "../types/conversationTypes.js";
-import type { TextGenerationOptions } from "../core/types.js";
+import type { ChatMessage } from "../types/conversation.js";
+import type { TextGenerationOptions } from "../types/generateTypes.js";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { ChatMessage } from "../types/conversationTypes.js"; | |
| import type { GenerateResult } from "../types/generateTypes.js"; | |
| import type { TextGenerationOptions } from "../core/types.js"; | |
| import type { ChatMessage } from "../types/conversation.js"; | |
| import type { GenerateResult } from "../types/generateTypes.js"; | |
| import type { TextGenerationOptions } from "../types/generateTypes.js"; |
🤖 Prompt for AI Agents
In src/lib/evaluation/contextBuilder.ts lines 2-4, the type import paths are
incorrect: update the ChatMessage import to come from src/lib/types/conversation
(replace "../types/conversationTypes.js" with "../types/conversation") and unify
TextGenerationOptions to the same generateTypes module (import
TextGenerationOptions from "../types/generateTypes" and keep GenerateResult from
"../types/generateTypes" as well), removing the import from "../core/types.js"
so all related types come from src/lib/types.
| private extractUserQuery(options: TextGenerationOptions): string { | ||
| // If there's a direct prompt, use it | ||
| if (typeof options === "string") { | ||
| return options; | ||
| } | ||
|
|
||
| // If there's an input.text, use it | ||
| if (options.input?.text) { | ||
| return options.input.text; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove string-typed options handling; TextGenerationOptions is an object.
The typeof options === "string" checks contradict the declared type, leading to dead branches and confusion.
- if (typeof options === "string") {
- return options;
- }
+ // Prefer explicit fields on options; no string variant supported.
...
- if (typeof options === "string") {
- return {};
- }
+ // Always derive from options object.Also applies to: 150-153
🤖 Prompt for AI Agents
In src/lib/evaluation/contextBuilder.ts around lines 120 to 129 (and similarly
at 150-153), remove the dead branch that checks typeof options === "string"
because TextGenerationOptions is an object; instead treat options as the
declared object type and rely on options.input?.text (and other object
properties) for extracting the user query, and update any TypeScript signatures
or call sites if necessary to ensure callers never pass a raw string.
| private extractReasoning(response: string): string { | ||
| // Look for REASONING section | ||
| const reasoningMatch = response.match(/### REASONING\n(.+?)(?=\n###|$)/s); | ||
| if (reasoningMatch && reasoningMatch[1]) { | ||
| return reasoningMatch[1].trim(); | ||
| } | ||
|
|
||
| // Fallback: look for any paragraph after scores | ||
| const paragraphMatch = response.match(/Overall:\s*\d+\n\n(.+?)(?=\n\n|$)/s); | ||
| if (paragraphMatch && paragraphMatch[1]) { | ||
| return paragraphMatch[1].trim(); | ||
| } | ||
|
|
||
| return "Evaluation completed based on provided criteria."; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make reasoning extraction resilient (case-insensitive headings, CRLF, flexible levels).
Current regex only catches “### REASONING” (exact, uppercase) and Unix newlines. Broaden to any H1–H6 “Reasoning” and handle CRLF.
- const reasoningMatch = response.match(/### REASONING\n(.+?)(?=\n###|$)/s);
+ const reasoningMatch = response.match(/^\s*#{1,6}\s*reasoning\b[\s:]*\r?\n([\s\S]+?)(?=^\s*#{1,6}\s|\r?\n[A-Z][A-Za-z\s]+:|$)/im);
@@
- const paragraphMatch = response.match(/Overall:\s*\d+\n\n(.+?)(?=\n\n|$)/s);
+ const paragraphMatch = response.match(/Overall:\s*\d+(?:\.\d+)?(?:\s*\/\s*10)?[^\n]*\r?\n+([\s\S]+?)(?=\r?\n{2,}|\r?\n[A-Z][A-Za-z\s]+:|^\s*#{1,6}\s|$)/im);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private extractReasoning(response: string): string { | |
| // Look for REASONING section | |
| const reasoningMatch = response.match(/### REASONING\n(.+?)(?=\n###|$)/s); | |
| if (reasoningMatch && reasoningMatch[1]) { | |
| return reasoningMatch[1].trim(); | |
| } | |
| // Fallback: look for any paragraph after scores | |
| const paragraphMatch = response.match(/Overall:\s*\d+\n\n(.+?)(?=\n\n|$)/s); | |
| if (paragraphMatch && paragraphMatch[1]) { | |
| return paragraphMatch[1].trim(); | |
| } | |
| return "Evaluation completed based on provided criteria."; | |
| } | |
| private extractReasoning(response: string): string { | |
| // Look for REASONING section | |
| const reasoningMatch = response.match( | |
| /^\s*#{1,6}\s*reasoning\b[\s:]*\r?\n([\s\S]+?)(?=^\s*#{1,6}\s|\r?\n[A-Z][A-Za-z\s]+:|$)/im | |
| ); | |
| if (reasoningMatch && reasoningMatch[1]) { | |
| return reasoningMatch[1].trim(); | |
| } | |
| // Fallback: look for any paragraph after scores | |
| const paragraphMatch = response.match( | |
| /Overall:\s*\d+(?:\.\d+)?(?:\s*\/\s*10)?[^\n]*\r?\n+([\s\S]+?)(?=\r?\n{2,}|\r?\n[A-Z][A-Za-z\s]+:|^\s*#{1,6}\s|$)/im | |
| ); | |
| if (paragraphMatch && paragraphMatch[1]) { | |
| return paragraphMatch[1].trim(); | |
| } | |
| return "Evaluation completed based on provided criteria."; | |
| } |
🤖 Prompt for AI Agents
In src/lib/evaluation/scoreParser.ts around lines 174 to 188, the
extractReasoning function's regexes only match an exact "### REASONING" with
Unix newlines; update them to be case-insensitive, accept any heading level
H1–H6 (e.g., one to six leading # characters with optional space), and allow
CRLF or LF line endings and dot-all matching so multi-line reasoning is
captured; apply the same CRLF and case-insensitive/flexible heading logic to the
fallback paragraph match (e.g., match "Overall:" robustly across CRLF/LF), and
ensure trimming and the original fallback string remain unchanged.
| // Tool Execution Data | ||
| toolExecutions: ExtractedToolExecution[]; | ||
|
|
||
| // Conversation Context | ||
| conversationHistory: EnhancedConversationTurn[]; | ||
| conversationSummary?: string; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Naming drift vs GenerateResult.toolExecutions.
Here uses toolName/toolCallId; GenerateResult uses name without call id. Consider aligning or adding an adapter to avoid mapping bugs.
🏁 Script executed:
#!/bin/bash
# Find all toolExecutions shapes to assess alignment
rg -nP --type=ts -C2 '(toolExecutions\s*:\s*Array<|interface\s+ExtractedToolExecution)'Length of output: 575
Align toolExecutions shape across modules
The ExtractedToolExecution interface (src/lib/evaluation/types.ts:42) expects toolName and toolCallId, but baseProvider.ts (line 383) emits objects with name and no call ID. Introduce a mapping layer or rename fields so the shapes match and avoid mapping bugs.
🤖 Prompt for AI Agents
In src/lib/evaluation/types.ts around lines 90 to 96, the toolExecutions type
expects ExtractedToolExecution but other modules (e.g., baseProvider.ts) emit
objects with name and no toolCallId; update code so shapes match by either (A)
updating ExtractedToolExecution to accept `name?: string` and `toolCallId?:
string` (and add a small conversion when reading legacy objects), or (B) update
the emitter in baseProvider.ts to emit `toolName` and `toolCallId` (populate
call ID as empty string or undefined if unavailable); ensure a single mapping
function converts incoming tool execution objects to the canonical
ExtractedToolExecution shape before pushing into toolExecutions to avoid
downstream mapping bugs.
| // Create a provider wrapper for retry manager | ||
| const providerWrapper = { | ||
| generate: async (_options: unknown) => { | ||
| const result = await doGenerate(); | ||
| return { | ||
| content: result.text || "", | ||
| provider: params.model?.provider || "unknown", | ||
| model: params.model?.modelId || "unknown", | ||
| usage: result.usage | ||
| ? { | ||
| prompt: result.usage.promptTokens || 0, | ||
| completion: result.usage.completionTokens || 0, | ||
| total: result.usage.totalTokens || 0, | ||
| } | ||
| : undefined, | ||
| responseTime: Date.now() - startTime, | ||
| }; | ||
| }, | ||
| providerName: params.model?.provider || "unknown", | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Retries never use enhanced options (critical).
providerWrapper.generate ignores _options, so every attempt calls doGenerate() with the original params. Retries won’t improve.
- const providerWrapper = {
- generate: async (_options: unknown) => {
- const result = await doGenerate();
+ let lastRawResult: any | null = null;
+ const providerWrapper = {
+ generate: async (_options: any) => {
+ // Merge RetryManager-provided options over original params
+ const mergedParams = { ...params, ...(_options || {}) };
+ const result = await doGenerate(mergedParams as any);
+ lastRawResult = result;
return {
content: result.text || "",
provider: params.model?.provider || "unknown",
model: params.model?.modelId || "unknown",
usage: result.usage
? {
prompt: result.usage.promptTokens || 0,
completion: result.usage.completionTokens || 0,
total: result.usage.totalTokens || 0,
}
: undefined,
responseTime: Date.now() - startTime,
};
},
providerName: params.model?.provider || "unknown",
};If doGenerate doesn’t accept overrides, you’ll need to adapt the underlying provider call. Please verify its signature.
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'wrapGenerate\W*:\W*async\s*\(\{\s*doGenerate' srcLength of output: 1687
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/lib/middleware/builtin/autoEvaluation.tsLength of output: 7128
Merge _options into the doGenerate call in providerWrapper.generate
_options is currently ignored, so every retry uses the original params. Update as:
- generate: async (_options: unknown) => {
- const result = await doGenerate();
+ generate: async (_options: unknown) => {
+ const result = await doGenerate({ ...params, ...(_options as Record<string, unknown>) });Ensure doGenerate supports parameter overrides or adjust the underlying provider call accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create a provider wrapper for retry manager | |
| const providerWrapper = { | |
| generate: async (_options: unknown) => { | |
| const result = await doGenerate(); | |
| return { | |
| content: result.text || "", | |
| provider: params.model?.provider || "unknown", | |
| model: params.model?.modelId || "unknown", | |
| usage: result.usage | |
| ? { | |
| prompt: result.usage.promptTokens || 0, | |
| completion: result.usage.completionTokens || 0, | |
| total: result.usage.totalTokens || 0, | |
| } | |
| : undefined, | |
| responseTime: Date.now() - startTime, | |
| }; | |
| }, | |
| providerName: params.model?.provider || "unknown", | |
| }; | |
| // Create a provider wrapper for retry manager | |
| const providerWrapper = { | |
| generate: async (_options: unknown) => { | |
| const result = await doGenerate({ ...params, ...(_options as Record<string, unknown>) }); | |
| return { | |
| content: result.text || "", | |
| provider: params.model?.provider || "unknown", | |
| model: params.model?.modelId || "unknown", | |
| usage: result.usage | |
| ? { | |
| prompt: result.usage.promptTokens || 0, | |
| completion: result.usage.completionTokens || 0, | |
| total: result.usage.totalTokens || 0, | |
| } | |
| : undefined, | |
| responseTime: Date.now() - startTime, | |
| }; | |
| }, | |
| providerName: params.model?.provider || "unknown", | |
| }; |
🤖 Prompt for AI Agents
In src/lib/middleware/builtin/autoEvaluation.ts around lines 112 to 131,
providerWrapper.generate currently ignores the _options parameter so retries
reuse the original params; update the generate implementation to merge _options
into the call to doGenerate (e.g. create an overrides object by shallow-merging
_options into the original params or into the specific provider input, then pass
that merged options to doGenerate) and ensure doGenerate and any downstream
provider call accept and apply those overrides (adjust signatures or map fields
as needed) so retries use the merged parameters.
| // Get the actual generation result with enhanced metadata | ||
| const finalResult = await doGenerate(); | ||
| const enhancedResult = { | ||
| ...finalResult, | ||
| text: result.finalContent || finalResult.text, | ||
| autoEvaluationMetadata: { | ||
| performed: true, | ||
| finalScore: result.finalEvaluation.overall, | ||
| attempts: result.attempts.length, | ||
| duration: Date.now() - startTime, | ||
| improvement: result.averageScoreImprovement, | ||
| success: result.success, | ||
| }, | ||
| }; | ||
|
|
||
| // Cache successful result | ||
| if (cache && result.success) { | ||
| cache.set(params, enhancedResult); | ||
| } | ||
|
|
||
| logger.debug(`[AutoEvaluation] Completed request ${requestId}`, { | ||
| finalScore: result.finalEvaluation.overall, | ||
| attempts: result.attempts.length, | ||
| success: result.success, | ||
| }); | ||
|
|
||
| return enhancedResult; | ||
| } catch (error) { |
There was a problem hiding this comment.
Avoid extra generation; reuse the successful attempt.
You call doGenerate() again after retries, incurring extra cost and divergence. Use the content from RetryManager.
- // Get the actual generation result with enhanced metadata
- const finalResult = await doGenerate();
- const enhancedResult = {
- ...finalResult,
- text: result.finalContent || finalResult.text,
+ // Reuse the last attempt result (no extra generation)
+ const enhancedResult = {
+ ...(lastRawResult ?? {}),
+ text: result.finalContent || lastRawResult?.text || "",
autoEvaluationMetadata: {
performed: true,
finalScore: result.finalEvaluation.overall,
attempts: result.attempts.length,
- duration: Date.now() - startTime,
+ duration: result.totalDuration,
improvement: result.averageScoreImprovement,
success: result.success,
},
};Committable suggestion skipped: line range outside the PR's diff.
| autoEvaluation: createAutoEvaluationMiddleware, | ||
| }; |
There was a problem hiding this comment.
Critical: ID mismatch (‘autoEvaluation’ vs ‘auto-evaluation’) prevents enabling and re-registration
The middleware metadata id in autoEvaluation.ts is "auto-evaluation", but presets/maps here use "autoEvaluation". This breaks:
- Preset activation (keys won’t match config generated from registry IDs).
- Creator lookup in getCreator (returns undefined for "auto-evaluation").
Unify on "auto-evaluation" everywhere.
Apply this diff within this file:
@@
- autoEvaluation: createAutoEvaluationMiddleware,
+ "auto-evaluation": createAutoEvaluationMiddleware,
@@
- description: "Default preset with auto-evaluation for quality assurance.",
- config: { autoEvaluation: { enabled: true } },
+ description: "Default preset with auto-evaluation for quality assurance.",
+ config: { "auto-evaluation": { enabled: true } },
@@
- config: {
- analytics: { enabled: true },
- guardrails: { enabled: true },
- autoEvaluation: { enabled: true },
- },
+ config: {
+ analytics: { enabled: true },
+ guardrails: { enabled: true },
+ "auto-evaluation": { enabled: true },
+ },
@@
- name: "quality",
- description: "Ensures quality with auto-evaluation and guardrails.",
- config: {
- guardrails: { enabled: true },
- autoEvaluation: { enabled: true },
- },
+ name: "quality",
+ description: "Ensures quality with auto-evaluation and guardrails.",
+ config: {
+ guardrails: { enabled: true },
+ "auto-evaluation": { enabled: true },
+ },
@@
- autoEvaluation: createAutoEvaluationMiddleware,
+ "auto-evaluation": createAutoEvaluationMiddleware,Also applies to: 52-54, 58-63, 69-76, 193-203
🤖 Prompt for AI Agents
In src/lib/middleware/factory.ts around lines 46-47 (and also update occurrences
at 52-54, 58-63, 69-76, and 193-203), the middleware key/name uses
"autoEvaluation" but the middleware metadata id is "auto-evaluation"; change all
occurrences of the key/name from autoEvaluation to auto-evaluation so
presets/maps and creator lookup use the same ID; update any object keys, preset
entries, and getCreator references in those line ranges to "auto-evaluation"
consistently to ensure activation and registration work.
| import type { AutoEvaluationConfig } from "../evaluation/autoEvaluationConfig.js"; | ||
| import { DEFAULT_AUTO_EVALUATION_CONFIG } from "../evaluation/autoEvaluationConfig.js"; | ||
| import { logger } from "../utils/logger.js"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Config schema mismatch with AutoEvaluationConfig (won’t type-check).
This file treats config as flat keys (enabled, provider, model, timeout, retryConfig, cache, etc.), but AutoEvaluationConfig is nested: quality, retry, evaluationModel, performance, telemetry. Align all reads/writes to the nested structure or update the type. As-is, this will fail compilation and runtime merges.
Run to confirm current schema:
🏁 Script executed:
#!/bin/bash
# Inspect the canonical config type
rg -nC2 'interface\s+AutoEvaluationConfig' src | sed -n '1,200p'Length of output: 360
🏁 Script executed:
#!/bin/bash
# Show full AutoEvaluationConfig interface
sed -n '1,200p' src/lib/evaluation/autoEvaluationConfig.ts
# Show autoEvaluationConfigManager implementation
sed -n '1,200p' src/lib/config/autoEvaluationConfigManager.tsLength of output: 7385
Align AutoEvaluationConfigManager with nested AutoEvaluationConfig schema
• The manager reads/writes top-level properties (enabled, provider, model, temperature, maxRetries, timeout, retryConfig, circuitBreaker, minScoreThresholds, cache, batchSize, parallelLimit) that don’t exist on AutoEvaluationConfig (which defines quality, retry, evaluationModel, performance, telemetry)
• Update loadEnvironmentConfig and applyEnvironmentOverrides to set overrides under the correct nested keys (e.g. overrides.evaluationModel = { provider, model, temperature }, overrides.retry = { maxAttempts, backoffMultiplier }, overrides.performance = { timeout, cache, cacheTTL }, overrides.telemetry = { enabled, endpoint }) or extend the AutoEvaluationConfig type to include any truly global flags
• Remove or properly define the unsupported keys (circuitBreaker, minScoreThresholds, batchSize, parallelLimit) to restore type safety and ensure compilation passes
File: src/lib/config/autoEvaluationConfigManager.ts
🤖 Prompt for AI Agents
In src/lib/config/autoEvaluationConfigManager.ts around lines 6 to 8, the
manager currently reads/writes top-level properties that don't exist on the
AutoEvaluationConfig schema (which uses nested keys like quality, retry,
evaluationModel, performance, telemetry); update loadEnvironmentConfig and
applyEnvironmentOverrides to map environment variables into the correct nested
structure (e.g. set overrides.evaluationModel = { provider, model, temperature
}, overrides.retry = { maxAttempts, backoffMultiplier, timeout? },
overrides.performance = { timeout, cache, cacheTTL }, overrides.telemetry = {
enabled, endpoint }) and stop reading/writing unsupported top-level keys
(circuitBreaker, minScoreThresholds, batchSize, parallelLimit), or if those are
required, add them to the AutoEvaluationConfig type and
DEFAULT_AUTO_EVALUATION_CONFIG; ensure types align, remove or properly define
unsupported keys to restore type safety and update any tests/usages accordingly.
| // Basic settings | ||
| if (process.env.NEUROLINK_AUTO_EVAL_ENABLED !== undefined) { | ||
| overrides.enabled = process.env.NEUROLINK_AUTO_EVAL_ENABLED === "true"; | ||
| } | ||
|
|
||
| if (process.env.NEUROLINK_AUTO_EVAL_PROVIDER) { | ||
| overrides.provider = process.env.NEUROLINK_AUTO_EVAL_PROVIDER; | ||
| } | ||
|
|
||
| if (process.env.NEUROLINK_AUTO_EVAL_MODEL) { | ||
| overrides.model = process.env.NEUROLINK_AUTO_EVAL_MODEL; | ||
| } | ||
|
|
||
| if (process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE) { | ||
| overrides.temperature = parseFloat( | ||
| process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE, | ||
| ); | ||
| } | ||
|
|
||
| if (process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES) { | ||
| overrides.maxRetries = parseInt( | ||
| process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES, | ||
| 10, | ||
| ); | ||
| } | ||
|
|
||
| if (process.env.NEUROLINK_AUTO_EVAL_TIMEOUT) { | ||
| overrides.timeout = parseInt(process.env.NEUROLINK_AUTO_EVAL_TIMEOUT, 10); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Map env vars to the nested config shape.
Flattened assignments (enabled, provider, model, temperature, maxRetries, timeout) should target telemetry.enabled, evaluationModel.provider, evaluationModel.model, evaluationModel.temperature, retry.maxAttempts, and performance.timeout.
- // Basic settings
- if (process.env.NEUROLINK_AUTO_EVAL_ENABLED !== undefined) {
- overrides.enabled = process.env.NEUROLINK_AUTO_EVAL_ENABLED === "true";
- }
- if (process.env.NEUROLINK_AUTO_EVAL_PROVIDER) {
- overrides.provider = process.env.NEUROLINK_AUTO_EVAL_PROVIDER;
- }
- if (process.env.NEUROLINK_AUTO_EVAL_MODEL) {
- overrides.model = process.env.NEUROLINK_AUTO_EVAL_MODEL;
- }
- if (process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE) {
- overrides.temperature = parseFloat(
- process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE,
- );
- }
- if (process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES) {
- overrides.maxRetries = parseInt(
- process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES,
- 10,
- );
- }
- if (process.env.NEUROLINK_AUTO_EVAL_TIMEOUT) {
- overrides.timeout = parseInt(process.env.NEUROLINK_AUTO_EVAL_TIMEOUT, 10);
- }
+ // Basic settings (nested)
+ if (process.env.NEUROLINK_AUTO_EVAL_ENABLED !== undefined) {
+ overrides.telemetry = {
+ ...overrides.telemetry,
+ enabled: process.env.NEUROLINK_AUTO_EVAL_ENABLED === "true",
+ };
+ }
+ if (process.env.NEUROLINK_AUTO_EVAL_PROVIDER) {
+ overrides.evaluationModel = {
+ ...overrides.evaluationModel,
+ provider: process.env.NEUROLINK_AUTO_EVAL_PROVIDER,
+ };
+ }
+ if (process.env.NEUROLINK_AUTO_EVAL_MODEL) {
+ overrides.evaluationModel = {
+ ...overrides.evaluationModel,
+ model: process.env.NEUROLINK_AUTO_EVAL_MODEL,
+ };
+ }
+ if (process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE) {
+ overrides.evaluationModel = {
+ ...overrides.evaluationModel,
+ temperature: parseFloat(process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE),
+ };
+ }
+ if (process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES) {
+ overrides.retry = {
+ ...overrides.retry,
+ maxAttempts: parseInt(process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES, 10),
+ };
+ }
+ if (process.env.NEUROLINK_AUTO_EVAL_TIMEOUT) {
+ overrides.performance = {
+ ...overrides.performance,
+ timeout: parseInt(process.env.NEUROLINK_AUTO_EVAL_TIMEOUT, 10),
+ };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Basic settings | |
| if (process.env.NEUROLINK_AUTO_EVAL_ENABLED !== undefined) { | |
| overrides.enabled = process.env.NEUROLINK_AUTO_EVAL_ENABLED === "true"; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_PROVIDER) { | |
| overrides.provider = process.env.NEUROLINK_AUTO_EVAL_PROVIDER; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_MODEL) { | |
| overrides.model = process.env.NEUROLINK_AUTO_EVAL_MODEL; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE) { | |
| overrides.temperature = parseFloat( | |
| process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE, | |
| ); | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES) { | |
| overrides.maxRetries = parseInt( | |
| process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES, | |
| 10, | |
| ); | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_TIMEOUT) { | |
| overrides.timeout = parseInt(process.env.NEUROLINK_AUTO_EVAL_TIMEOUT, 10); | |
| } | |
| // Basic settings (nested) | |
| if (process.env.NEUROLINK_AUTO_EVAL_ENABLED !== undefined) { | |
| overrides.telemetry = { | |
| ...overrides.telemetry, | |
| enabled: process.env.NEUROLINK_AUTO_EVAL_ENABLED === "true", | |
| }; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_PROVIDER) { | |
| overrides.evaluationModel = { | |
| ...overrides.evaluationModel, | |
| provider: process.env.NEUROLINK_AUTO_EVAL_PROVIDER, | |
| }; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_MODEL) { | |
| overrides.evaluationModel = { | |
| ...overrides.evaluationModel, | |
| model: process.env.NEUROLINK_AUTO_EVAL_MODEL, | |
| }; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE) { | |
| overrides.evaluationModel = { | |
| ...overrides.evaluationModel, | |
| temperature: parseFloat(process.env.NEUROLINK_AUTO_EVAL_TEMPERATURE), | |
| }; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES) { | |
| overrides.retry = { | |
| ...overrides.retry, | |
| maxAttempts: parseInt(process.env.NEUROLINK_AUTO_EVAL_MAX_RETRIES, 10), | |
| }; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_TIMEOUT) { | |
| overrides.performance = { | |
| ...overrides.performance, | |
| timeout: parseInt(process.env.NEUROLINK_AUTO_EVAL_TIMEOUT, 10), | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/lib/config/autoEvaluationConfigManager.ts around lines 26 to 55, the
current env var overrides assign flat keys (enabled, provider, model,
temperature, maxRetries, timeout) but should set nested config fields instead;
update assignments so NEUROLINK_AUTO_EVAL_ENABLED maps to
overrides.telemetry.enabled (boolean), NEUROLINK_AUTO_EVAL_PROVIDER to
overrides.evaluationModel.provider (string), NEUROLINK_AUTO_EVAL_MODEL to
overrides.evaluationModel.model (string), NEUROLINK_AUTO_EVAL_TEMPERATURE to
overrides.evaluationModel.temperature (parseFloat),
NEUROLINK_AUTO_EVAL_MAX_RETRIES to overrides.retry.maxAttempts (parseInt base
10), and NEUROLINK_AUTO_EVAL_TIMEOUT to overrides.performance.timeout (parseInt
base 10), preserving the existing presence checks and parsing logic.
| // Retry configuration | ||
| if (process.env.NEUROLINK_AUTO_EVAL_RETRY_ENABLED !== undefined) { | ||
| overrides.retryConfig = { | ||
| ...overrides.retryConfig, | ||
| enabled: process.env.NEUROLINK_AUTO_EVAL_RETRY_ENABLED === "true", | ||
| }; | ||
| } | ||
|
|
||
| if (process.env.NEUROLINK_AUTO_EVAL_RETRY_STRATEGY) { | ||
| overrides.retryConfig = { | ||
| ...overrides.retryConfig, | ||
| strategy: process.env.NEUROLINK_AUTO_EVAL_RETRY_STRATEGY as | ||
| | "STANDARD" | ||
| | "AGGRESSIVE" | ||
| | "CONSERVATIVE" | ||
| | "ADAPTIVE", | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or re-home retry strategy and retryConfig keys.
retryConfig and strategy aren’t part of AutoEvaluationConfig. Either (a) extend the type to include retry.strategy: RetryStrategy, or (b) drop strategy env support here and handle strategy elsewhere. Keeping as-is breaks types.
- if (process.env.NEUROLINK_AUTO_EVAL_RETRY_ENABLED !== undefined) {
- overrides.retryConfig = {
- ...overrides.retryConfig,
- enabled: process.env.NEUROLINK_AUTO_EVAL_RETRY_ENABLED === "true",
- };
- }
- if (process.env.NEUROLINK_AUTO_EVAL_RETRY_STRATEGY) {
- overrides.retryConfig = {
- ...overrides.retryConfig,
- strategy: process.env.NEUROLINK_AUTO_EVAL_RETRY_STRATEGY as
- | "STANDARD"
- | "AGGRESSIVE"
- | "CONSERVATIVE"
- | "ADAPTIVE",
- };
- }
+ // NOTE: No retry strategy field in AutoEvaluationConfig. Consider adding `retry.strategy` to the type, or handle externally.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Retry configuration | |
| if (process.env.NEUROLINK_AUTO_EVAL_RETRY_ENABLED !== undefined) { | |
| overrides.retryConfig = { | |
| ...overrides.retryConfig, | |
| enabled: process.env.NEUROLINK_AUTO_EVAL_RETRY_ENABLED === "true", | |
| }; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_RETRY_STRATEGY) { | |
| overrides.retryConfig = { | |
| ...overrides.retryConfig, | |
| strategy: process.env.NEUROLINK_AUTO_EVAL_RETRY_STRATEGY as | |
| | "STANDARD" | |
| | "AGGRESSIVE" | |
| | "CONSERVATIVE" | |
| | "ADAPTIVE", | |
| }; | |
| } | |
| // Retry configuration | |
| // NOTE: No retry strategy field in AutoEvaluationConfig. Consider adding `retry.strategy` to the type, or handle externally. |
🤖 Prompt for AI Agents
In src/lib/config/autoEvaluationConfigManager.ts around lines 56 to 74, the code
is adding retryConfig and strategy keys that are not part of
AutoEvaluationConfig which breaks types; remove the environment-driven
retryConfig/strategy assignment from this file and instead either (1) add these
env vars to the module that owns retry behavior (move parsing and application of
NEUROLINK_AUTO_EVAL_RETRY_ENABLED / NEUROLINK_AUTO_EVAL_RETRY_STRATEGY to the
retry-management/config module) or (2) if you intend AutoEvaluationConfig to
include them, update the AutoEvaluationConfig type to include a retry: {
enabled: boolean; strategy: RetryStrategy } and adjust all usages — choose one
approach and implement consistently so types remain correct.
| // Circuit breaker configuration | ||
| if (process.env.NEUROLINK_AUTO_EVAL_CIRCUIT_BREAKER_THRESHOLD) { | ||
| overrides.circuitBreaker = { | ||
| ...overrides.circuitBreaker, | ||
| failureThreshold: parseInt( | ||
| process.env.NEUROLINK_AUTO_EVAL_CIRCUIT_BREAKER_THRESHOLD, | ||
| 10, | ||
| ), | ||
| }; | ||
| } | ||
|
|
||
| if (process.env.NEUROLINK_AUTO_EVAL_CIRCUIT_BREAKER_RESET) { | ||
| overrides.circuitBreaker = { | ||
| ...overrides.circuitBreaker, | ||
| resetTimeout: parseInt( | ||
| process.env.NEUROLINK_AUTO_EVAL_CIRCUIT_BREAKER_RESET, | ||
| 10, | ||
| ), | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Circuit breaker block references undefined config section.
circuitBreaker isn’t defined in AutoEvaluationConfig. Remove or add to the schema in autoEvaluationConfig.ts with proper defaults before using it.
🤖 Prompt for AI Agents
In src/lib/config/autoEvaluationConfigManager.ts around lines 75 to 95, the code
references overrides.circuitBreaker which is not defined on
AutoEvaluationConfig; update the schema in
src/lib/config/autoEvaluationConfig.ts to add a circuitBreaker property (typed
interface with failureThreshold:number and resetTimeout:number and sensible
defaults), and update the manager to either initialize overrides.circuitBreaker
= overrides.circuitBreaker || {} before spreading or rely on the new defaults so
parseInt values can be applied safely; ensure the default values are
exported/used by the config loader so existing consumers get the new field.
| // Performance optimization | ||
| if (process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED !== undefined) { | ||
| overrides.cache = { | ||
| ...overrides.cache, | ||
| enabled: process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED === "true", | ||
| }; | ||
| } | ||
|
|
||
| if (process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL) { | ||
| overrides.cache = { | ||
| ...overrides.cache, | ||
| ttl: parseInt(process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL, 10), | ||
| }; | ||
| } | ||
|
|
||
| if (process.env.NEUROLINK_AUTO_EVAL_CACHE_MAX_SIZE) { | ||
| overrides.cache = { | ||
| ...overrides.cache, | ||
| maxSize: parseInt(process.env.NEUROLINK_AUTO_EVAL_CACHE_MAX_SIZE, 10), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Cache section doesn’t match schema.
Use performance.cache (boolean) and performance.cacheTTL (ms). cache.maxSize is unknown to the type.
- if (process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED !== undefined) {
- overrides.cache = {
- ...overrides.cache,
- enabled: process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED === "true",
- };
- }
- if (process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL) {
- overrides.cache = {
- ...overrides.cache,
- ttl: parseInt(process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL, 10),
- };
- }
- if (process.env.NEUROLINK_AUTO_EVAL_CACHE_MAX_SIZE) {
- overrides.cache = {
- ...overrides.cache,
- maxSize: parseInt(process.env.NEUROLINK_AUTO_EVAL_CACHE_MAX_SIZE, 10),
- };
- }
+ if (process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED !== undefined) {
+ overrides.performance = {
+ ...overrides.performance,
+ cache: process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED === "true",
+ };
+ }
+ if (process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL) {
+ overrides.performance = {
+ ...overrides.performance,
+ cacheTTL: parseInt(process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL, 10),
+ };
+ }
+ // Remove unsupported CACHE_MAX_SIZE or add to schema first.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Performance optimization | |
| if (process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED !== undefined) { | |
| overrides.cache = { | |
| ...overrides.cache, | |
| enabled: process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED === "true", | |
| }; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL) { | |
| overrides.cache = { | |
| ...overrides.cache, | |
| ttl: parseInt(process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL, 10), | |
| }; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_CACHE_MAX_SIZE) { | |
| overrides.cache = { | |
| ...overrides.cache, | |
| maxSize: parseInt(process.env.NEUROLINK_AUTO_EVAL_CACHE_MAX_SIZE, 10), | |
| }; | |
| } | |
| // Performance optimization | |
| if (process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED !== undefined) { | |
| overrides.performance = { | |
| ...overrides.performance, | |
| cache: process.env.NEUROLINK_AUTO_EVAL_CACHE_ENABLED === "true", | |
| }; | |
| } | |
| if (process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL) { | |
| overrides.performance = { | |
| ...overrides.performance, | |
| cacheTTL: parseInt(process.env.NEUROLINK_AUTO_EVAL_CACHE_TTL, 10), | |
| }; | |
| } | |
| // Remove unsupported CACHE_MAX_SIZE or add to schema first. |
🤖 Prompt for AI Agents
In src/lib/config/autoEvaluationConfigManager.ts around lines 131 to 151, the
environment-driven cache overrides are using a non-existent cache shape; update
them to match the config schema by setting overrides.performance (boolean) and
overrides.performance.cacheTTL (number in ms) instead of overrides.cache fields,
remove/ignore maxSize (unknown to the type), and ensure proper parsing: convert
NEUROLINK_AUTO_EVAL_CACHE_ENABLED to a boolean, parse
NEUROLINK_AUTO_EVAL_CACHE_TTL as an integer (ms) and assign to
performance.cacheTTL, and do not set cache.maxSize.
| constructor(config: TelemetryConfig) { | ||
| this.config = { | ||
| enabled: config.enabled, | ||
| endpoint: config.endpoint || "", | ||
| batchSize: config.batchSize || 100, | ||
| flushInterval: config.flushInterval || 30000, // 30 seconds | ||
| }; | ||
|
|
||
| if (this.config.enabled && this.config.endpoint) { | ||
| this.startBatchProcessing(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Disable collector when enabled=true but endpoint is empty (prevents unbounded queue growth).
Currently events are enqueued but never flushed if endpoint is falsy. Flip enabled off in that case.
constructor(config: TelemetryConfig) {
this.config = {
enabled: config.enabled,
endpoint: config.endpoint || "",
batchSize: config.batchSize || 100,
flushInterval: config.flushInterval || 30000, // 30 seconds
};
- if (this.config.enabled && this.config.endpoint) {
- this.startBatchProcessing();
- }
+ if (this.config.enabled && this.config.endpoint) {
+ this.startBatchProcessing();
+ } else if (this.config.enabled && !this.config.endpoint) {
+ logger.warn("[Telemetry] Enabled without endpoint; disabling collector.");
+ this.config.enabled = false;
+ }
}🤖 Prompt for AI Agents
In src/lib/evaluation/telemetryCollector.ts around lines 26 to 37, the
constructor currently leaves the collector enabled when config.enabled is true
but config.endpoint is empty, allowing events to queue indefinitely; update the
initialization so that enabled is set to false if endpoint is falsy (e.g. set
enabled: config.enabled && !!config.endpoint), ensure any subsequent logic (like
calling startBatchProcessing) only runs when the final enabled flag is true, and
keep the existing defaults for batchSize and flushInterval.
| collectEvaluationEvent( | ||
| requestId: string, | ||
| provider: string, | ||
| model: string, | ||
| result: RetryResult, | ||
| ): void { | ||
| if (!this.config.enabled) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also guard collection when endpoint is missing.
Extra safety to avoid enqueuing when disabled or endpoint is absent.
- if (!this.config.enabled) {
+ if (!this.config.enabled || !this.config.endpoint) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| collectEvaluationEvent( | |
| requestId: string, | |
| provider: string, | |
| model: string, | |
| result: RetryResult, | |
| ): void { | |
| if (!this.config.enabled) { | |
| return; | |
| } | |
| collectEvaluationEvent( | |
| requestId: string, | |
| provider: string, | |
| model: string, | |
| result: RetryResult, | |
| ): void { | |
| if (!this.config.enabled || !this.config.endpoint) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/lib/evaluation/telemetryCollector.ts around lines 39 to 47, the method
collectEvaluationEvent currently only checks this.config.enabled before
enqueuing telemetry; add an additional guard to return early when the telemetry
endpoint is not configured (e.g., if (!this.config.endpoint) return), so no work
or enqueue occurs when the endpoint is absent; optionally emit a debug log when
skipping due to missing endpoint.
| collectRetryEvent( | ||
| requestId: string, | ||
| attemptNumber: number, | ||
| score: number, | ||
| issues: string[], | ||
| ): void { | ||
| if (!this.config.enabled) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same guard for retry events.
- if (!this.config.enabled) {
+ if (!this.config.enabled || !this.config.endpoint) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| collectRetryEvent( | |
| requestId: string, | |
| attemptNumber: number, | |
| score: number, | |
| issues: string[], | |
| ): void { | |
| if (!this.config.enabled) { | |
| return; | |
| } | |
| collectRetryEvent( | |
| requestId: string, | |
| attemptNumber: number, | |
| score: number, | |
| issues: string[], | |
| ): void { | |
| if (!this.config.enabled || !this.config.endpoint) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/lib/evaluation/telemetryCollector.ts around lines 72 to 81, the
collectRetryEvent method needs the same telemetry-enabled guard used by other
event collectors; add an early return that checks this.config.enabled (and any
other relevant config flag used by other collectors) at the top of the method so
the function exits immediately when telemetry is disabled, preventing any
further processing or emissions.
| // Also check toolCalls and toolResults for additional data | ||
| if (result.toolCalls && result.toolResults) { | ||
| const additionalExecutions = this.correlateCallsAndResults( | ||
| result.toolCalls as ToolCall[], | ||
| result.toolResults as ToolResult[], | ||
| ); | ||
| executions.push(...additionalExecutions); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correlation misses common shapes (toolCallId, alt duration/status fields); allow calls-only path
Results are keyed only by id/toolName, and correlation runs only when both calls and results exist. This loses matches from SDKs that emit toolCallId or alternative fields.
Apply:
- // Also check toolCalls and toolResults for additional data
- if (result.toolCalls && result.toolResults) {
+ // Also check toolCalls and toolResults for additional data
+ if (Array.isArray(result.toolCalls)) {
const additionalExecutions = this.correlateCallsAndResults(
- result.toolCalls as ToolCall[],
- result.toolResults as ToolResult[],
+ result.toolCalls as ToolCall[],
+ (result.toolResults as ToolResult[]) ?? [],
);
executions.push(...additionalExecutions);
} private correlateCallsAndResults(
toolCalls: ToolCall[],
toolResults: ToolResult[],
): ExtractedToolExecution[] {
const executions: ExtractedToolExecution[] = [];
- const resultMap = new Map(toolResults.map((r) => [r.id || r.toolName, r]));
+ const byToolCallId = new Map<string, ToolResult>();
+ const byId = new Map<string, ToolResult>();
+ const byName = new Map<string, ToolResult>();
+ for (const r of toolResults ?? []) {
+ const any = r as any;
+ const tcid = any.toolCallId ?? any.id;
+ if (tcid) byToolCallId.set(String(tcid), r);
+ if (any.id) byId.set(String(any.id), r);
+ if (any.toolName) byName.set(String(any.toolName), r);
+ }
toolCalls.forEach((call, index) => {
- const result = resultMap.get(call.toolCallId || call.id || call.toolName);
+ const lookupKey = call.toolCallId || call.id;
+ const result =
+ (lookupKey && (byToolCallId.get(lookupKey) ?? byId.get(lookupKey))) ||
+ byName.get(call.toolName);
executions.push({
toolName: call.toolName,
- toolCallId: call.toolCallId || call.id || `tool-${index}`,
- input: this.sanitizeInput(call.parameters || call.args),
- output: result ? this.sanitizeOutput(result.output) : null,
- duration: result?.executionTime || 0,
- success: result ? result.status === "success" : false,
- error: result?.error,
+ toolCallId: call.toolCallId || call.id || `tool-${index}`,
+ input: this.sanitizeInput(call.parameters ?? call.args ?? {}),
+ output: result
+ ? this.sanitizeOutput(
+ (result as any).output ??
+ (result as any).result ??
+ (result as any).data,
+ )
+ : null,
+ duration:
+ ((result as any)?.executionTime ??
+ (result as any)?.durationMs ??
+ (result as any)?.timeMs ??
+ 0) as number,
+ success: result
+ ? ((result as any).status === "success" ||
+ (result as any).status === "ok" ||
+ (result as any).ok === true)
+ : false,
+ error: (result as any)?.error as string | undefined,
metadata: {
timestamp: Date.now(),
sequenceNumber: index,
retryCount: 0,
},
});
});Also applies to: 53-80
🤖 Prompt for AI Agents
In src/lib/evaluation/toolExecutionExtractor.ts around lines 16-23 (also apply
same fix to lines 53-80), the current correlation only runs when both
result.toolCalls and result.toolResults exist and matches strictly on
result.id/toolName, which misses SDK shapes that use toolCallId or alternative
duration/status fields and loses calls when only calls are present; update the
logic to (1) run a "calls-only" path when toolCalls exist without toolResults
and map those ToolCall entries to executions using available fields (accept
toolCallId as an identifier, fallback to id/toolName pairing), (2) make
correlateCallsAndResults tolerant to alternative field names for linking (check
toolCallId, id, and toolName combinations) and to alternate duration/status
properties when building execution objects, and (3) ensure executions are
de-duplicated when both results and calls are present so the same call matched
via different keys is not duplicated.
| private parseToolExecution( | ||
| execution: unknown, | ||
| sequenceNumber: number, | ||
| ): ExtractedToolExecution { | ||
| const exec = execution as Record<string, unknown>; | ||
| const startTime = (exec.startTime as number) || Date.now(); | ||
| const endTime = (exec.endTime as number) || Date.now(); | ||
|
|
||
| return { | ||
| toolName: (exec.toolName as string) || "unknown", | ||
| toolCallId: | ||
| (exec.toolCallId as string) || `tool-${Date.now()}-${sequenceNumber}`, | ||
| input: this.sanitizeInput(exec.args || exec.input || {}), | ||
| output: this.sanitizeOutput(exec.result || exec.output), | ||
| duration: endTime - startTime, | ||
| success: exec.success !== false, | ||
| error: exec.error as string | undefined, | ||
| metadata: { | ||
| timestamp: startTime, | ||
| sequenceNumber, | ||
| retryCount: (exec.retryCount as number) || 0, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bug: toolName fallback ignores name; success default too optimistic; duration can go negative
Current parsing sets toolName to "unknown" when only name is present, defaults success to true when success is undefined, and doesn’t clamp duration. This skews metrics and dedup keys.
Apply:
private parseToolExecution(
execution: unknown,
sequenceNumber: number,
): ExtractedToolExecution {
const exec = execution as Record<string, unknown>;
- const startTime = (exec.startTime as number) || Date.now();
- const endTime = (exec.endTime as number) || Date.now();
+ const startTime = (exec.startTime as number) ?? Date.now();
+ const endTime = (exec.endTime as number) ?? Date.now();
+ const toolName =
+ (exec.toolName as string) || (exec.name as string) || "unknown";
+ const toolCallId =
+ (exec.toolCallId as string) ||
+ (exec.id as string) ||
+ `tool-${sequenceNumber}`;
return {
- toolName: (exec.toolName as string) || "unknown",
- toolCallId:
- (exec.toolCallId as string) || `tool-${Date.now()}-${sequenceNumber}`,
- input: this.sanitizeInput(exec.args || exec.input || {}),
- output: this.sanitizeOutput(exec.result || exec.output),
- duration: endTime - startTime,
- success: exec.success !== false,
+ toolName,
+ toolCallId,
+ input: this.sanitizeInput(exec.args ?? exec.input ?? {}),
+ output: this.sanitizeOutput((exec.result ?? exec.output) as unknown),
+ duration: Math.max(0, endTime - startTime),
+ success:
+ ((exec.success as boolean | undefined) === true) ||
+ (!exec.error && (exec.result ?? exec.output) !== undefined),
error: exec.error as string | undefined,
metadata: {
timestamp: startTime,
sequenceNumber,
- retryCount: (exec.retryCount as number) || 0,
+ retryCount: Number(exec.retryCount) || 0,
},
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private parseToolExecution( | |
| execution: unknown, | |
| sequenceNumber: number, | |
| ): ExtractedToolExecution { | |
| const exec = execution as Record<string, unknown>; | |
| const startTime = (exec.startTime as number) || Date.now(); | |
| const endTime = (exec.endTime as number) || Date.now(); | |
| return { | |
| toolName: (exec.toolName as string) || "unknown", | |
| toolCallId: | |
| (exec.toolCallId as string) || `tool-${Date.now()}-${sequenceNumber}`, | |
| input: this.sanitizeInput(exec.args || exec.input || {}), | |
| output: this.sanitizeOutput(exec.result || exec.output), | |
| duration: endTime - startTime, | |
| success: exec.success !== false, | |
| error: exec.error as string | undefined, | |
| metadata: { | |
| timestamp: startTime, | |
| sequenceNumber, | |
| retryCount: (exec.retryCount as number) || 0, | |
| }, | |
| }; | |
| } | |
| private parseToolExecution( | |
| execution: unknown, | |
| sequenceNumber: number, | |
| ): ExtractedToolExecution { | |
| const exec = execution as Record<string, unknown>; | |
| const startTime = (exec.startTime as number) ?? Date.now(); | |
| const endTime = (exec.endTime as number) ?? Date.now(); | |
| const toolName = | |
| (exec.toolName as string) || (exec.name as string) || "unknown"; | |
| const toolCallId = | |
| (exec.toolCallId as string) || | |
| (exec.id as string) || | |
| `tool-${sequenceNumber}`; | |
| return { | |
| toolName, | |
| toolCallId, | |
| input: this.sanitizeInput(exec.args ?? exec.input ?? {}), | |
| output: this.sanitizeOutput((exec.result ?? exec.output) as unknown), | |
| duration: Math.max(0, endTime - startTime), | |
| success: | |
| ((exec.success as boolean | undefined) === true) || | |
| (!exec.error && (exec.result ?? exec.output) !== undefined), | |
| error: exec.error as string | undefined, | |
| metadata: { | |
| timestamp: startTime, | |
| sequenceNumber, | |
| retryCount: Number(exec.retryCount) || 0, | |
| }, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/lib/evaluation/toolExecutionExtractor.ts around lines 28 to 51, update
parseToolExecution so toolName falls back to exec.name when exec.toolName is
missing, compute success strictly as exec.success === true (do not default
undefined to true), and clamp duration to a non-negative value using something
like Math.max(0, endTime - startTime); keep other fields the same and ensure
types/casts remain safe (e.g., retryCount numeric fallback).
|
@sishir2001 pending comments and build failures |
- Add always-on auto-evaluation middleware for quality assurance - Implement enhanced context builder to capture rich evaluation metadata - Create auto evaluator with RAGAS-inspired scoring methodology - Add retry manager with progressive feedback integration - Implement 4 retry strategies: STANDARD, AGGRESSIVE, CONSERVATIVE, ADAPTIVE - Add configuration management with environment variable support - Integrate auto-evaluation as default middleware (no feature flags) - Create evaluation cache for performance optimization - Add telemetry collector for monitoring evaluation metrics - Fix all linting errors in evaluation modules - Update middleware factory to include auto-evaluation by default - Export auto-evaluation components in main index
0b9f804 to
38bf692
Compare
|
@amreetkhuntia Can you check if the changes in this PR will help you to move things faster? |
|
Closing this as it has been picked up by @amreetkhuntia |
3 participants
Pull Request
Description
Type of Change
Related Issues
Changes Made
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
Screenshots/Demo
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Documentation