feat(machine-integration): Implement Milestone 1 - Foundation layer#21
feat(machine-integration): Implement Milestone 1 - Foundation layer#21
Conversation
- Fix property name mismatches in hook parameters (_currentProject, _loadIssues) - Update drag event type compatibility for @dnd-kit/core - Add proper DragStartEvent and DragEndEvent type imports - Add validation, rate limiting, and JSON parsing middleware - Add unit tests for beads service and utilities 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add comprehensive provider infrastructure enabling multi-engine AI support with unified telemetry, capability probing, and agent monitoring. **Provider Registry (M1-T1)** - Singleton EngineRegistry with Map-based O(1) provider lookup - Auto-discovery via onRegister() lifecycle hooks - Fallback chain management for provider failover - Per-provider metadata tracking with priority-based routing **Capability Probing (M1-T2)** - Runtime detection of provider capabilities (vision, tools, streaming) - Authentication method detection (api-key, oauth, cli-auth) - Rate limit probing with 5-minute TTL caching - ProviderCapability and ProviderLimits interfaces **Claude Provider Enhancement (M1-T3)** - Add isAuthenticated() method for auth status checking - Add getCapabilities() returning provider capability summary - Integrate telemetryParser for usage parsing - Export claude-telemetry parser for SDK output **Cursor Provider (M1-T4)** - New CursorProvider extending BaseProvider - CLI-based query execution with streaming support - Authentication detection via cursor --version - Model listing for Cursor-hosted Claude models **Auth Cache (M1-T5)** - AuthCache class with Map-based O(1) lookups - Configurable TTL (default 5 minutes) - Cache statistics (hits, misses, hit rate) - getOrCompute() for lazy auth checking **Agent Monitor Service (M1-T6, M1-T7)** - SQLite-based agent execution tracking with PID monitoring - Parent-child relationship tracking for sub-agents - Automatic dead process detection and cleanup - Telemetry aggregation per agent - Agent tree queries and statistics **Unified Telemetry (M1-T8, M1-T9, M1-T10, M1-T11)** - ParsedTelemetry interface across all providers - Model-specific pricing (Opus: $15/M in, $75/M out) - Claude SDK telemetry parser - Cursor CLI telemetry parser - TelemetryCapture factory with parser routing **Provider API Routes (M1-T12)** - GET /api/providers/list - List all registered providers - POST /api/providers/test/:id - Test provider auth - POST /api/providers/probe/:id - Probe capabilities - GET /api/providers/status - Active provider + fallbacks **CI/CD** - Add provider-check.yml workflow for provider infrastructure validation - Test better-sqlite3 compatibility for agent monitor - Validate provider exports and telemetry parsers 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR introduces a comprehensive multi-provider infrastructure system, including a provider registry with capability probing, TTL-based authentication caching, and provider-specific telemetry parsers for Claude and Cursor. It adds SQLite-backed agent monitoring with PID tracking and telemetry aggregation, along with new API routes for provider management. Minor UI restructuring updates logo formatting. Changes
Sequence DiagramssequenceDiagram
participant Client
participant API as /api/providers
participant Registry as EngineRegistry
participant Probe as CapabilityProbe
participant Provider as BaseProvider
participant Cache as Cache (TTL)
Client->>API: POST /providers/probe/{providerId}
API->>Registry: get(providerId)
Registry-->>API: ProviderMetadata
API->>Probe: probeWithResult(providerId, provider)
rect rgb(200, 230, 255)
Note over Probe,Cache: Check cache for existing probe result
Probe->>Cache: lookup(providerId)
alt Cache hit and not expired
Cache-->>Probe: ProviderCapability (cached)
else Cache miss or expired
Probe->>Provider: detectAuth()
Provider-->>Probe: AuthMethod[]
Probe->>Provider: testCapability(capability)
Provider-->>Probe: boolean
Probe->>Provider: probeLimits()
Provider-->>Probe: RateLimit
Probe->>Cache: store(providerId, capability, timestamp)
end
end
Probe-->>API: ProbeResult {capability, limits, cacheHit, timestamp}
API-->>Client: JSON response with capabilities
sequenceDiagram
participant Agent as AI Agent
participant Monitor as AgentMonitorService
participant DB as SQLite DB
participant TelemetryFactory as TelemetryFactory
participant Parser as ProviderParser
Agent->>Monitor: register(agentRecord)
Monitor->>DB: INSERT agent
DB-->>Monitor: agentId
Agent->>Monitor: start(id, pid)
Monitor->>DB: UPDATE status=running, pid
Monitor->>Monitor: startPIDMonitoring()
Agent->>Agent: execute query
Agent->>TelemetryFactory: createTelemetryCapture(engine, model, output)
TelemetryFactory->>Parser: parse(output)
Parser-->>TelemetryFactory: ParsedTelemetry
TelemetryFactory-->>Agent: CapturedTelemetry
Agent->>Monitor: complete(id, telemetry)
Monitor->>DB: UPDATE status=completed, telemetry JSON
rect rgb(230, 200, 255)
Note over Monitor,DB: Periodic PID monitoring
Monitor->>Monitor: checkDeadProcesses()
Monitor->>Monitor: isProcessAlive(pid)
alt Process dead
Monitor->>Monitor: cleanupOrphanedPIDs()
Monitor->>DB: UPDATE orphaned agents
end
end
Agent->>Monitor: getStats()
Monitor->>DB: SELECT aggregated data
DB-->>Monitor: stats (tokens, cost, duration by status/engine)
Monitor-->>Agent: AgentStats
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
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 |
Summary of ChangesHello @0xtsotsi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the groundwork for a flexible and scalable multi-AI provider system. It establishes core components for managing different AI engines, dynamically assessing their features, efficiently handling authentication, and comprehensively monitoring agent activities. This foundational layer is critical for future expansions, ensuring consistent operational insights and seamless integration of diverse AI capabilities. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces a significant amount of foundational code for multi-provider support, including telemetry, capability probing, and agent monitoring. The overall structure is well-thought-out, but there are several critical areas that need attention regarding correctness, performance, and design before this can be considered production-ready. Key issues include potential race conditions in telemetry aggregation, inefficient database queries, hardcoded provider-specific logic in generic components, and incomplete or fragile implementations in areas like CLI streaming and authentication checks. Addressing these points will greatly improve the robustness and extensibility of this new foundation.
|
|
||
| // Check authentication by trying a simple command | ||
| // (Cursor may not have an explicit auth check command) | ||
| const authenticated = true; // Assume authenticated if CLI is available |
There was a problem hiding this comment.
The authentication check for the Cursor provider assumes it's authenticated if the CLI is available (const authenticated = true;). This is a fragile assumption that could lead to runtime errors if the CLI is installed but not logged in. A more robust check should be implemented, for example by running a command that requires authentication and checking its output.
| addTelemetry(id: string, telemetry: ParsedTelemetry): boolean { | ||
| const existing = this.getAgent(id); | ||
|
|
||
| if (!existing) { | ||
| return false; | ||
| } | ||
|
|
||
| // Aggregate with existing telemetry | ||
| const aggregated = existing.telemetry | ||
| ? { | ||
| tokensIn: existing.telemetry.tokensIn + telemetry.tokensIn, | ||
| tokensOut: existing.telemetry.tokensOut + telemetry.tokensOut, | ||
| cached: existing.telemetry.cached + telemetry.cached, | ||
| cost: existing.telemetry.cost + telemetry.cost, | ||
| duration: existing.telemetry.duration + telemetry.duration, | ||
| } | ||
| : telemetry; | ||
|
|
||
| const stmt = this.db.prepare(` | ||
| UPDATE agents | ||
| SET telemetry_json = ? | ||
| WHERE id = ? | ||
| `); | ||
|
|
||
| const result = stmt.run(JSON.stringify(aggregated), id); | ||
| return result.changes > 0; | ||
| } |
There was a problem hiding this comment.
The addTelemetry method performs a read-modify-write operation that is not atomic. If this method is called concurrently for the same agent ID, it could lead to a race condition where telemetry updates are lost. To ensure data integrity, this operation should be made atomic, for example by using a database transaction or a more advanced SQL update statement that performs the aggregation on the database side.
| export function generateTelemetryId(): string { | ||
| return `tel_${Date.now()}_${Math.random().toString(36).substring(2, 11)}`; | ||
| } |
There was a problem hiding this comment.
The current implementation of generateTelemetryId using Date.now() and Math.random() does not guarantee uniqueness, especially under high load, which could lead to data collision and corruption in your telemetry system. It's highly recommended to use a more robust method for generating unique IDs, such as crypto.randomUUID() which is available in Node.js. You'll also need to add import { randomUUID } from 'crypto'; at the top of the file.
| export function generateTelemetryId(): string { | |
| return `tel_${Date.now()}_${Math.random().toString(36).substring(2, 11)}`; | |
| } | |
| export function generateTelemetryId(): string { | |
| return `tel_${randomUUID()}`; | |
| } |
| async detectAuth(provider: BaseProvider): Promise<AuthMethod[]> { | ||
| const methods: AuthMethod[] = []; | ||
| const name = provider.getName(); | ||
|
|
||
| // Check for API key authentication | ||
| if (name === 'claude' && process.env.ANTHROPIC_API_KEY) { | ||
| methods.push('api-key'); | ||
| } | ||
|
|
||
| // Check for CLI-based authentication | ||
| const installStatus = await provider.detectInstallation(); | ||
| if (installStatus.authenticated) { | ||
| if (installStatus.method === 'cli') { | ||
| methods.push('cli-auth'); | ||
| } | ||
| } | ||
|
|
||
| // Default to 'none' if no auth found but provider is installed | ||
| if (methods.length === 0 && installStatus.installed) { | ||
| methods.push('none'); | ||
| } | ||
|
|
||
| return methods; | ||
| } |
There was a problem hiding this comment.
The detectAuth method contains hardcoded logic specific to claude and cursor providers. This violates the separation of concerns and makes the capability probe difficult to extend. This logic should be moved into the respective provider classes. Consider adding a method like getAuthenticationMethods(): Promise<AuthMethod[]> to the BaseProvider class and calling it here.
| async probeLimits(provider: BaseProvider): Promise<ProviderCapability['rateLimit']> { | ||
| const name = provider.getName(); | ||
|
|
||
| // Known rate limits for popular providers | ||
| switch (name) { | ||
| case 'claude': | ||
| return { | ||
| requestsPerMinute: 50, // Default for most tiers | ||
| concurrent: 5, | ||
| }; | ||
| case 'cursor': | ||
| return { | ||
| requestsPerMinute: 60, | ||
| concurrent: 3, | ||
| }; | ||
| default: | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to detectAuth, the probeLimits method has hardcoded rate limit information for providers. This logic should be encapsulated within each provider. I suggest adding a getRateLimits(): Promise<ProviderCapability['rateLimit']> method to the BaseProvider class to make this component more extensible.
| if ( | ||
| 'getCapabilities' in metadata.provider && | ||
| typeof metadata.provider.getCapabilities === 'function' | ||
| ) { |
There was a problem hiding this comment.
Checking for the existence of getCapabilities using the in operator is a form of duck typing that can be brittle. For better type safety and cleaner code, consider adding an optional getCapabilities method to the BaseProvider abstract class. This would make the check a simple if (metadata.provider.getCapabilities).
| console.warn( | ||
| `[TelemetryFactory] No parser found for provider "${engine}", returning empty telemetry` | ||
| ); |
There was a problem hiding this comment.
Using console.warn for logging in a server application is not ideal as it lacks structure and can't be easily configured for different environments. Consider using a dedicated logger instance for better observability and control over log levels.
| getCapabilities(): { | ||
| supportsPlanning: boolean; | ||
| supportsVision: boolean; | ||
| supportsTools: boolean; | ||
| supportsStreaming: boolean; | ||
| supportsSystemPrompt: boolean; | ||
| supportsConversationHistory: boolean; | ||
| supportsMCP: boolean; | ||
| supportsThinking: boolean; | ||
| maxContextWindow: number; | ||
| maxOutputTokens: number; | ||
| } { | ||
| return { | ||
| supportsPlanning: true, | ||
| supportsVision: true, | ||
| supportsTools: true, | ||
| supportsStreaming: true, | ||
| supportsSystemPrompt: true, | ||
| supportsConversationHistory: true, | ||
| supportsMCP: true, | ||
| supportsThinking: true, | ||
| maxContextWindow: 200000, | ||
| maxOutputTokens: 16000, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The getCapabilities method returns hardcoded values for maxContextWindow and maxOutputTokens. These values are already defined in the models returned by getAvailableModels(). To avoid duplication and ensure consistency, these capability values should be derived dynamically from the available models. This makes the provider more maintainable as you only need to update model definitions in one place.
getCapabilities() {
const models = this.getAvailableModels();
const maxContextWindow = Math.max(...models.map((m) => m.contextWindow ?? 0));
const maxOutputTokens = Math.max(...models.map((m) => m.maxOutputTokens ?? 0));
return {
supportsPlanning: true,
supportsVision: true,
supportsTools: true,
supportsStreaming: true,
supportsSystemPrompt: true,
supportsConversationHistory: true,
supportsMCP: true,
supportsThinking: true,
maxContextWindow,
maxOutputTokens,
};
}| // Ambiguous, assume input | ||
| tokensIn = parseInt(match[1], 10); |
There was a problem hiding this comment.
The logic for parsing text-based telemetry is ambiguous. When a single token count is found without an 'in' or 'out' keyword, it's assumed to be input tokens. This could lead to incorrect telemetry data. It would be safer to log a warning about the ambiguous format or require a more explicit output format from the CLI.
| export function getAgentMonitor(dataDir: string): AgentMonitorService { | ||
| if (!agentMonitorInstance || agentMonitorInstance['dbPath'] !== path.join(dataDir, 'agents.db')) { | ||
| agentMonitorInstance = new AgentMonitorService(dataDir); | ||
| } | ||
| return agentMonitorInstance; | ||
| } |
There was a problem hiding this comment.
The singleton factory getAgentMonitor accesses the private property dbPath of the AgentMonitorService instance using bracket notation (agentMonitorInstance['dbPath']). This breaks encapsulation and is a code smell. A cleaner approach would be to manage the singleton instance without relying on its internal state, or to expose a public method for this check if necessary.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
DevFlow/apps/server/src/lib/beads-validation.ts
Lines 62 to 66 in ed1f76a
Preserve parentIssueId in create issue schema
The create-issue route validates with createBeadsIssueSchema, which no longer includes parentIssueId. Zod strips unknown fields, so parentIssueId supplied by clients is discarded and beads-service never adds --parent, breaking subtask creation via the API. Add parentIssueId back to the schema or explicitly allow it in the route.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function createTelemetryCapture( | ||
| engine: string, | ||
| model: string, | ||
| output: string, | ||
| workingDir: string, |
There was a problem hiding this comment.
This file imports createTelemetryCapture from telemetry.js and then later declares another export function createTelemetryCapture(...) with the same identifier. In ESM/TypeScript that’s a duplicate binding and the module won’t compile (the provider-check workflow runs a server build). Rename the local factory or alias the import to avoid the redeclaration.
Useful? React with 👍 / 👎.
| private runCursorCommand(args: string[]): Promise<string> { | ||
| return new Promise((resolve, reject) => { | ||
| const child = spawn(this.cliPath, args); | ||
| let output = ''; |
There was a problem hiding this comment.
The Cursor provider receives cwd in executeQuery, but runCursorCommand spawns the CLI without a working directory. Requests that expect project-specific context will run from the server’s cwd instead, so Cursor may read the wrong workspace or fail to access project files. Thread cwd into spawn (or pass it into runCursorCommand) to ensure correct context.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (19)
apps/server/src/providers/auth-cache.ts (2)
137-142: Inconsistency betweenclear()andinvalidateAll()behavior.
invalidateAll()clears entries but preserves hit/miss stats, whileclear()resets both. This inconsistency could confuse callers. Consider documenting this difference or aligning the behavior.🔎 Option: Document the behavioral difference
/** * Invalidate all cache entries + * + * Note: This preserves hit/miss statistics. Use clear() to also reset stats. */ invalidateAll(): void { this.cache.clear(); }
114-125: Consider whether to cache failures ingetOrCompute.Currently, if
fn()throws, the error propagates without caching. This is likely intentional to allow retries on transient failures, but you may want to document this behavior or optionally cache negative results for a shorter TTL to prevent thundering herd on repeated failures.apps/server/src/providers/capability-probe.ts (3)
370-378:getCacheStats()returns placeholder values instead of actual statistics.The method always returns
{ hits: 0, misses: 0, size: ... }. TheAuthCacheclass in the same codebase tracks these metrics properly. Consider implementing actual hit/miss tracking for consistency and observability.🔎 Proposed implementation with hit/miss tracking
Add private counters to the class:
export class ProviderCapabilityProbe { private cache = new Map<string, CachedCapability>(); private cacheTTL: number; + private hits = 0; + private misses = 0;Update the
probemethod to track cache hits/misses:if (cached && now - cached.timestamp < this.cacheTTL) { console.log(`[CapabilityProbe] Cache hit for provider "${providerId}"`); + this.hits++; return cached.capability; } + this.misses++;Update
getCacheStats:getCacheStats(): { hits: number; misses: number; size: number } { - // For simplicity, we're not tracking hits/misses in this implementation - // A production version would add counters for better observability return { - hits: 0, - misses: 0, + hits: this.hits, + misses: this.misses, size: this.cache.size, }; }
144-167: Auth detection is tightly coupled to specific provider names.The
detectAuthmethod hardcodes the "claude" provider check forANTHROPIC_API_KEY. Consider abstracting this to make the probe more extensible as new providers are added.
262-266: Consider using structured logging instead ofconsole.log.The provider routes use
createLoggerfrom@automaker/utilsfor consistent logging. Usingconsole.loghere creates inconsistency in log format and makes it harder to filter/aggregate logs.apps/server/src/providers/cursor-telemetry.ts (1)
102-142: Pattern matching logic may not behave as expected.The text format parser iterates through all patterns, but the logic for determining input vs. output tokens based on the pattern source string is fragile. The
pattern.source.includes('in')check on line 125 will match patterns containing 'in' anywhere (e.g., 'input' contains 'in'), but patterns liketokens?\s*(?:out|output)?:?\s*(\d+)/iat line 109 also contain 'in' due to the negative pattern. Additionally, bothtokenPatternsandoutputPatternsmay match the same text.Consider restructuring to associate each pattern with its token type explicitly:
Suggested approach
const patterns = [ { regex: /cursor\s+request[:\s]*(\d+)\s*in,\s*(\d+)\s*out/i, type: 'both' }, { regex: /tokens?\s*(?:in|input):?\s*(\d+)/i, type: 'in' }, { regex: /tokens?\s*(?:out|output):?\s*(\d+)/i, type: 'out' }, // ... ] as const;apps/server/src/providers/claude-telemetry.ts (2)
166-189: Duration parsing heuristic may misclassify values.Line 180 uses
value > 100to distinguish milliseconds from seconds, but this heuristic fails for durations like 150 seconds (2.5 minutes) which would be incorrectly treated as milliseconds. Consider making the pattern matching more explicit about units.Suggested improvement
function extractDuration(output: string): number { const durationPatterns = [ - /duration[:\s]+(\d+(?:\.\d+)?)\s*s/i, - /took[:\s]+(\d+(?:\.\d+)?)\s*s/i, - /(\d+(?:\.\d+)?)\s*seconds?/i, - /(\d+)\s*ms/i, + { regex: /duration[:\s]+(\d+(?:\.\d+)?)\s*s(?:ec|econds?)?/i, unit: 's' }, + { regex: /took[:\s]+(\d+(?:\.\d+)?)\s*s(?:ec|econds?)?/i, unit: 's' }, + { regex: /(\d+(?:\.\d+)?)\s*seconds?/i, unit: 's' }, + { regex: /(\d+)\s*ms/i, unit: 'ms' }, + { regex: /(\d+)\s*milliseconds?/i, unit: 'ms' }, ]; - for (const pattern of durationPatterns) { - const match = output.match(pattern); + for (const { regex, unit } of durationPatterns) { + const match = output.match(regex); if (match) { const value = parseFloat(match[1]); - if (pattern.source.includes('ms') || value > 100) { + if (unit === 'ms') { return Math.round(value); } return Math.round(value * 1000); } } return 0; }
244-248: Parser recreation on each call may be inefficient.
parseClaudeTelemetrycreates a new parser instance on every call viacreateClaudeTelemetryParser(model). For high-frequency telemetry parsing, consider caching parsers by model.apps/server/src/lib/telemetry-factory.ts (1)
30-34: Module-level mutable state may cause issues in testing and concurrent usage.
PROVIDER_PARSERSis a mutable object at module scope. WhileclearTelemetryParsers()exists for testing, concurrent test runs or dynamic registration could lead to race conditions.Consider documenting this limitation or using a more robust pattern for provider parser management if concurrent access is expected.
apps/server/src/lib/telemetry.ts (2)
255-274: Duration aggregation may not be semantically meaningful.
aggregateTelemetrysums durations across captures (line 270), but if captures represent sequential operations, the total duration is the sum. If they represent parallel operations, the total should be the maximum. Consider clarifying the expected use case or documenting this behavior.
95-120: Consider extracting pricing from configuration rather than hardcoding.The prices are currently accurate (as of December 2025), but hardcoding model-specific rates means they'll need code updates whenever Anthropic adjusts pricing. Loading from configuration would reduce maintenance burden and allow pricing updates without code changes.
apps/server/src/providers/cursor-provider.ts (2)
187-214: Authentication is assumed without actual verification.Line 197 sets
authenticated = trueunconditionally when the CLI is available. The comment acknowledges this limitation, but it may lead to runtime failures if the user isn't actually authenticated with Cursor.Consider attempting a lightweight authenticated operation to verify, or document this limitation more prominently.
285-311: CLI spawn lacks working directory and timeout handling.The
spawncall on line 287 doesn't specify acwdoption, so it inherits the process's current directory. Additionally, there's no timeout—a hanging CLI process will block indefinitely.Consider adding:
- A
cwdoption fromExecuteOptions- An
AbortControlleror timeout mechanismSuggested improvements
- private runCursorCommand(args: string[]): Promise<string> { + private runCursorCommand(args: string[], options?: { cwd?: string; timeoutMs?: number }): Promise<string> { return new Promise((resolve, reject) => { - const child = spawn(this.cliPath, args); + const child = spawn(this.cliPath, args, { cwd: options?.cwd }); let output = ''; let errorOutput = ''; + + const timeout = options?.timeoutMs ? setTimeout(() => { + child.kill(); + reject(new Error('Cursor CLI command timed out')); + }, options.timeoutMs) : undefined; // ... existing handlers ... child.on('close', (code) => { + if (timeout) clearTimeout(timeout); if (code === 0) { resolve(output); } else { reject(new Error(`Cursor CLI command failed: ${errorOutput || output}`)); } });apps/server/src/providers/registry.ts (3)
96-123: Async registration callbacks are fire-and-forget.
this.onRegister(metadata)at line 120 calls an async method withoutawait. Registration callbacks (which may bePromise-returning) will execute in the background, and any errors or completion won't be reflected in theregister()return flow.If callback completion is important before returning metadata, consider awaiting it. If fire-and-forget is intentional, add
.catch()to avoid unhandled rejections bubbling up.🔎 Option A: Await the callbacks
- register(id: string, provider: BaseProvider, config: ProviderConfig = {}): ProviderMetadata { + async register(id: string, provider: BaseProvider, config: ProviderConfig = {}): Promise<ProviderMetadata> { // ... existing code ... - this.onRegister(metadata); + await this.onRegister(metadata); return metadata; }🔎 Option B: Explicitly handle rejections if fire-and-forget is intentional
- this.onRegister(metadata); + this.onRegister(metadata).catch((err) => { + console.error(`[EngineRegistry] Unhandled error in registration callbacks for "${id}":`, err); + });
256-265:update()allows overwriting identity fields.
Object.assign(metadata, updates)permits callers to overwrite critical fields likeidorprovider, which could break registry invariants (e.g., Map key mismatch withid).🔎 Proposed fix: Exclude identity fields from updates
update(id: string, updates: Partial<ProviderMetadata>): ProviderMetadata | undefined { const metadata = this.providers.get(id); if (!metadata) { return undefined; } - Object.assign(metadata, updates); + const { id: _id, provider: _provider, ...safeUpdates } = updates; + Object.assign(metadata, safeUpdates); return metadata; }
141-154: Model lookup is O(n×m); consider indexing if model count grows.Both
getProviderForModel()andgetMetadataForModel()iterate over all providers and their models. For a small number of providers/models this is fine, but if usage scales, consider building a model→provider index on registration.Also applies to: 162-174
apps/server/src/services/agent-monitor.ts (3)
783-788: Accessing private property via bracket notation is a code smell.
agentMonitorInstance['dbPath']bypasses TypeScript's private access check. Consider exposing a getter or using a different comparison approach.🔎 Proposed fix: Add a public getter for path comparison
In the class:
+ getDbPath(): string { + return this.dbPath; + }Then in
getAgentMonitor:- if (!agentMonitorInstance || agentMonitorInstance['dbPath'] !== path.join(dataDir, 'agents.db')) { + if (!agentMonitorInstance || agentMonitorInstance.getDbPath() !== path.join(dataDir, 'agents.db')) { agentMonitorInstance = new AgentMonitorService(dataDir); }
475-485: Unbounded recursion inbuildTreeNodecould cause stack overflow.If the agent hierarchy has cycles (due to data corruption) or extreme depth,
buildTreeNodewill overflow the stack. Consider adding a depth limit.🔎 Proposed fix: Add maximum depth guard
- private buildTreeNode(agent: AgentRecord, depth: number): AgentTreeNode { + private static readonly MAX_TREE_DEPTH = 100; + + private buildTreeNode(agent: AgentRecord, depth: number): AgentTreeNode { + if (depth >= AgentMonitorService.MAX_TREE_DEPTH) { + console.warn(`[AgentMonitor] Max tree depth reached for agent ${agent.id}`); + return { ...agent, children: [], depth }; + } + const children = this.getAgents({ parentId: agent.id }).map((child) => this.buildTreeNode(child, depth + 1) );
683-685:getActiveAgents()makes two separate queries; consider combining.Two separate
getAgents()calls with spreading could be combined into a single SQL query for efficiency.🔎 Proposed fix: Single query with OR condition
getActiveAgents(): AgentRecord[] { - return [...this.getAgents({ status: 'running' }), ...this.getAgents({ status: 'starting' })]; + const stmt = this.db.prepare(` + SELECT * FROM agents WHERE status IN ('running', 'starting') ORDER BY created_at DESC + `); + const rows = stmt.all() as unknown[]; + return rows.map((r) => this.rowToAgent(r)).filter((r): r is AgentRecord => r !== null); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.github/workflows/provider-check.ymlapps/server/src/lib/beads-validation.tsapps/server/src/lib/telemetry-factory.tsapps/server/src/lib/telemetry.tsapps/server/src/providers/auth-cache.tsapps/server/src/providers/capability-probe.tsapps/server/src/providers/claude-provider.tsapps/server/src/providers/claude-telemetry.tsapps/server/src/providers/cursor-provider.tsapps/server/src/providers/cursor-telemetry.tsapps/server/src/providers/index.tsapps/server/src/providers/registry.tsapps/server/src/routes/providers/common.tsapps/server/src/routes/providers/index.tsapps/server/src/routes/providers/routes/list.tsapps/server/src/routes/providers/routes/probe.tsapps/server/src/routes/providers/routes/status.tsapps/server/src/routes/providers/routes/test.tsapps/server/src/services/agent-monitor.ts
💤 Files with no reviewable changes (1)
- apps/server/src/lib/beads-validation.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/server/src/routes/**
📄 CodeRabbit inference engine (CLAUDE.md)
API routes should be placed in
apps/server/src/routes/, with one file per route/resource
Files:
apps/server/src/routes/providers/index.tsapps/server/src/routes/providers/common.tsapps/server/src/routes/providers/routes/list.tsapps/server/src/routes/providers/routes/probe.tsapps/server/src/routes/providers/routes/status.tsapps/server/src/routes/providers/routes/test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Run type checking withnpm run typecheckbefore syncing the Beads database as part of quality gates
Run linting withnpm run lintbefore syncing the Beads database as part of quality gates
Files:
apps/server/src/routes/providers/index.tsapps/server/src/routes/providers/common.tsapps/server/src/routes/providers/routes/list.tsapps/server/src/routes/providers/routes/probe.tsapps/server/src/providers/claude-provider.tsapps/server/src/routes/providers/routes/status.tsapps/server/src/lib/telemetry.tsapps/server/src/providers/cursor-telemetry.tsapps/server/src/providers/index.tsapps/server/src/providers/cursor-provider.tsapps/server/src/services/agent-monitor.tsapps/server/src/routes/providers/routes/test.tsapps/server/src/providers/claude-telemetry.tsapps/server/src/lib/telemetry-factory.tsapps/server/src/providers/registry.tsapps/server/src/providers/auth-cache.tsapps/server/src/providers/capability-probe.ts
apps/server/src/services/**
📄 CodeRabbit inference engine (CLAUDE.md)
Services should be placed in
apps/server/src/services/, with one service per file
Files:
apps/server/src/services/agent-monitor.ts
🧠 Learnings (1)
📚 Learning: 2025-12-24T19:31:56.698Z
Learnt from: CR
Repo: 0xtsotsi/DevFlow PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T19:31:56.698Z
Learning: Run linting and tests on server changes with `npm run lint --workspace=apps/server` and `npm run test:server`
Applied to files:
.github/workflows/provider-check.yml
🧬 Code graph analysis (11)
apps/server/src/routes/providers/index.ts (4)
apps/server/src/routes/providers/routes/list.ts (1)
createListProvidersHandler(25-64)apps/server/src/routes/providers/routes/status.ts (1)
createStatusHandler(9-53)apps/server/src/routes/providers/routes/test.ts (1)
createTestProviderHandler(10-54)apps/server/src/routes/providers/routes/probe.ts (2)
createProbeProviderHandler(10-43)createProbeAllProvidersHandler(48-74)
apps/server/src/routes/providers/common.ts (1)
apps/server/src/routes/common.ts (1)
createLogError(34-38)
apps/server/src/routes/providers/routes/list.ts (3)
apps/server/src/providers/index.ts (1)
EngineRegistry(18-18)apps/server/src/providers/registry.ts (1)
EngineRegistry(362-362)apps/server/src/routes/providers/common.ts (1)
logError(12-12)
apps/server/src/routes/providers/routes/probe.ts (3)
apps/server/src/providers/registry.ts (1)
EngineRegistry(362-362)apps/server/src/providers/capability-probe.ts (1)
capabilityProbe(403-403)apps/server/src/routes/providers/common.ts (1)
logError(12-12)
apps/server/src/providers/claude-provider.ts (3)
apps/server/src/lib/telemetry.ts (1)
TelemetryParser(43-43)apps/server/src/providers/registry.ts (1)
TelemetryParser(23-25)apps/server/src/providers/claude-telemetry.ts (1)
claudeTelemetryParser(236-236)
apps/server/src/routes/providers/routes/status.ts (3)
apps/server/src/providers/index.ts (1)
EngineRegistry(18-18)apps/server/src/providers/registry.ts (1)
EngineRegistry(362-362)apps/server/src/routes/providers/common.ts (1)
logError(12-12)
apps/server/src/providers/cursor-provider.ts (3)
apps/server/src/lib/telemetry.ts (1)
TelemetryParser(43-43)apps/server/src/providers/registry.ts (1)
TelemetryParser(23-25)apps/server/src/providers/cursor-telemetry.ts (1)
cursorTelemetryParser(286-286)
apps/server/src/providers/claude-telemetry.ts (3)
apps/server/src/lib/telemetry.ts (3)
ParsedTelemetry(22-35)calculateCost(167-180)TelemetryParser(43-43)apps/server/src/providers/index.ts (5)
ParsedTelemetry(22-22)createClaudeTelemetryParser(42-42)TelemetryParser(21-21)claudeTelemetryParser(41-41)parseClaudeTelemetry(43-43)apps/server/src/providers/registry.ts (2)
ParsedTelemetry(30-43)TelemetryParser(23-25)
apps/server/src/lib/telemetry-factory.ts (5)
apps/server/src/lib/telemetry.ts (5)
TelemetryParser(43-43)createTelemetryCapture(189-201)CapturedTelemetry(51-64)TelemetryCaptureOptions(69-78)createEmptyTelemetry(211-222)apps/server/src/providers/index.ts (3)
TelemetryParser(21-21)claudeTelemetryParser(41-41)cursorTelemetryParser(46-46)apps/server/src/providers/registry.ts (1)
TelemetryParser(23-25)apps/server/src/providers/claude-telemetry.ts (1)
claudeTelemetryParser(236-236)apps/server/src/providers/cursor-telemetry.ts (1)
cursorTelemetryParser(286-286)
apps/server/src/providers/auth-cache.ts (1)
apps/server/src/providers/index.ts (3)
AuthCacheStats(37-37)AuthCache(36-36)authCache(36-36)
apps/server/src/providers/capability-probe.ts (1)
apps/server/src/providers/index.ts (7)
ProviderCapability(29-29)ProviderLimits(31-31)ProbeResult(30-30)ProviderCapabilityProbe(27-27)BaseProvider(8-8)ModelDefinition(57-57)capabilityProbe(27-27)
🪛 Biome (2.1.2)
apps/server/src/lib/telemetry-factory.ts
[error] 69-69: Shouldn't redeclare 'createTelemetryCapture'. Consider to delete it or rename it.
'createTelemetryCapture' is defined here:
(lint/suspicious/noRedeclare)
apps/server/src/providers/registry.ts
[error] 362-362: Shouldn't redeclare 'EngineRegistry'. Consider to delete it or rename it.
'EngineRegistry' is defined here:
(lint/suspicious/noRedeclare)
[error] 362-362: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (21)
.github/workflows/provider-check.yml (1)
1-72: LGTM!The workflow structure, trigger paths, and validation steps for provider exports and telemetry parsers are well-designed. The path filters correctly target the provider infrastructure files.
apps/server/src/providers/auth-cache.ts (1)
1-60: LGTM!Well-structured cache implementation with comprehensive documentation, proper TTL handling, and useful utility methods. The Map-based storage provides the promised O(1) lookups.
apps/server/src/providers/capability-probe.ts (1)
26-57: LGTM!Well-defined interfaces for
ProviderCapability,ProviderLimits, andProbeResult. The type definitions are comprehensive and will provide good type safety for capability probing across the codebase.Also applies to: 70-79, 84-90
apps/server/src/routes/providers/index.ts (1)
1-24: LGTM!Clean router factory pattern with proper imports and documentation. The factory approach enables easy testing and composition.
apps/server/src/routes/providers/common.ts (1)
1-12: LGTM!Clean utility module that properly scopes a logger for provider routes and re-exports shared error handling utilities.
apps/server/src/routes/providers/routes/test.ts (1)
10-53: LGTM!Good implementation of the test handler with proper validation, cache-aside pattern via
authCache.getOrCompute, and registry update. The duck-typing check forisAuthenticatedprovides flexibility for different provider implementations.apps/server/src/routes/providers/routes/status.ts (1)
9-52: LGTM!Clean implementation providing comprehensive status information including primary provider, fallback chain, and aggregated counts. The response structure is well-organized for client consumption.
apps/server/src/routes/providers/routes/list.ts (1)
25-63: LGTM!Well-structured handler that builds provider information including optional capabilities. The duck-typing pattern for
getCapabilitiesis consistent with other handlers in this module.apps/server/src/routes/providers/routes/probe.ts (2)
10-43: LGTM! Handler follows established patterns.The single-provider probe handler validates input, handles not-found cases, and returns consistent JSON responses. Error handling with
logErrorandgetErrorMessagealigns with sibling route files.
48-74: LGTM! Bulk probe handler is well-structured.The handler correctly aggregates all provider probes and returns a count alongside results. The Map-based provider lookup aligns with the capability probing system design.
apps/server/src/providers/cursor-telemetry.ts (2)
40-90: LGTM! JSON parsing handles multi-format output well.The parser correctly aggregates tokens across JSON lines and handles Cursor-specific, Claude-standard, and direct token count formats. Silent catch for non-JSON lines is appropriate here.
240-281: LGTM! Factory pattern provides clean API.The parser factory correctly prioritizes JSON parsing over text fallback and enriches results with duration and metadata when available.
apps/server/src/providers/claude-provider.ts (1)
22-23: LGTM! Authentication and telemetry integration are clean.The
telemetryParserproperty andisAuthenticated()method follow the same pattern as the Cursor provider, enabling consistent capability probing across providers.Also applies to: 29-37
apps/server/src/providers/index.ts (1)
39-49: LGTM! Telemetry parser exports are well-organized.Both Claude and Cursor telemetry parsers are properly exposed with their factory functions and convenience wrappers.
apps/server/src/providers/claude-telemetry.ts (1)
44-88: LGTM! JSON parsing is robust.The parser handles both message-wrapped and direct usage objects, correctly aggregating tokens across multiple JSON lines.
apps/server/src/lib/telemetry-factory.ts (1)
197-270: LGTM! TelemetryCapture class provides good lifecycle management.The class encapsulates parsing state and allows parser overrides, which is useful for testing and custom integrations.
apps/server/src/lib/telemetry.ts (1)
22-35: LGTM! Core telemetry types are well-designed.The
ParsedTelemetry,TelemetryParser,CapturedTelemetry, andTelemetryCaptureOptionsinterfaces provide a clean abstraction for unified telemetry handling across providers.Also applies to: 43-43, 51-64, 69-78
apps/server/src/providers/cursor-provider.ts (1)
34-84: LGTM! Model definitions are comprehensive.The
CURSOR_MODELSarray provides well-structured model definitions with appropriate metadata for the Cursor-hosted Claude models.apps/server/src/providers/registry.ts (1)
1-17: LGTM on overall structure.The registry design with Map-based O(1) lookups, lifecycle hooks, and convenience exports is well-structured. The interface definitions are clear and the singleton pattern is appropriate for this use case.
Also applies to: 370-390
apps/server/src/services/agent-monitor.ts (2)
143-172: LGTM on schema design.Good use of indexes on frequently queried columns (
status,parent_id,feature_id,beads_id,pid,created_at). WAL mode is appropriate for concurrent read/write scenarios.
22-106: LGTM on type definitions.The
AgentStatus,AgentRecord,AgentTreeNode, andAgentStatsinterfaces are well-defined with clear documentation and appropriate use of optional fields.
| - name: Test provider registry operations | ||
| shell: bash | ||
| run: | | ||
| node --run << 'EOF' | ||
| import { EngineRegistry } from './apps/server/src/providers/registry.js'; | ||
| import { ClaudeProvider } from './apps/server/src/providers/claude-provider.js'; | ||
|
|
||
| // Test registration | ||
| const claude = new ClaudeProvider(); | ||
| const metadata = EngineRegistry.register('test-claude', claude, {}); | ||
|
|
||
| console.log('✓ Provider registered:', metadata.id); | ||
|
|
||
| // Test retrieval | ||
| const retrieved = EngineRegistry.get('test-claude'); | ||
| if (!retrieved || retrieved.id !== 'test-claude') { | ||
| throw new Error('Provider retrieval failed'); | ||
| } | ||
| console.log('✓ Provider retrieved successfully'); | ||
|
|
||
| // Test getAll | ||
| const all = EngineRegistry.getAll(); | ||
| console.log('✓ Total providers registered:', all.length); | ||
|
|
||
| // Cleanup | ||
| EngineRegistry.unregister('test-claude'); | ||
| console.log('✓ Provider registry integration test passed'); | ||
| EOF |
There was a problem hiding this comment.
Invalid node --run syntax will cause CI failure.
The node --run flag is used to execute scripts defined in package.json, not to run inline JavaScript code. This integration test step will fail.
🔎 Proposed fix using proper ESM execution
- name: Test provider registry operations
shell: bash
run: |
- node --run << 'EOF'
- import { EngineRegistry } from './apps/server/src/providers/registry.js';
- import { ClaudeProvider } from './apps/server/src/providers/claude-provider.js';
-
- // Test registration
- const claude = new ClaudeProvider();
- const metadata = EngineRegistry.register('test-claude', claude, {});
-
- console.log('✓ Provider registered:', metadata.id);
-
- // Test retrieval
- const retrieved = EngineRegistry.get('test-claude');
- if (!retrieved || retrieved.id !== 'test-claude') {
- throw new Error('Provider retrieval failed');
- }
- console.log('✓ Provider retrieved successfully');
-
- // Test getAll
- const all = EngineRegistry.getAll();
- console.log('✓ Total providers registered:', all.length);
-
- // Cleanup
- EngineRegistry.unregister('test-claude');
- console.log('✓ Provider registry integration test passed');
- EOF
+ node --input-type=module -e "
+ import { EngineRegistry } from './apps/server/src/providers/registry.js';
+ import { ClaudeProvider } from './apps/server/src/providers/claude-provider.js';
+
+ // Test registration
+ const claude = new ClaudeProvider();
+ const metadata = EngineRegistry.register('test-claude', claude, {});
+
+ console.log('✓ Provider registered:', metadata.id);
+
+ // Test retrieval
+ const retrieved = EngineRegistry.get('test-claude');
+ if (!retrieved || retrieved.id !== 'test-claude') {
+ throw new Error('Provider retrieval failed');
+ }
+ console.log('✓ Provider retrieved successfully');
+
+ // Test getAll
+ const all = EngineRegistry.getAll();
+ console.log('✓ Total providers registered:', all.length);
+
+ // Cleanup
+ EngineRegistry.unregister('test-claude');
+ console.log('✓ Provider registry integration test passed');
+ "📝 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.
| - name: Test provider registry operations | |
| shell: bash | |
| run: | | |
| node --run << 'EOF' | |
| import { EngineRegistry } from './apps/server/src/providers/registry.js'; | |
| import { ClaudeProvider } from './apps/server/src/providers/claude-provider.js'; | |
| // Test registration | |
| const claude = new ClaudeProvider(); | |
| const metadata = EngineRegistry.register('test-claude', claude, {}); | |
| console.log('✓ Provider registered:', metadata.id); | |
| // Test retrieval | |
| const retrieved = EngineRegistry.get('test-claude'); | |
| if (!retrieved || retrieved.id !== 'test-claude') { | |
| throw new Error('Provider retrieval failed'); | |
| } | |
| console.log('✓ Provider retrieved successfully'); | |
| // Test getAll | |
| const all = EngineRegistry.getAll(); | |
| console.log('✓ Total providers registered:', all.length); | |
| // Cleanup | |
| EngineRegistry.unregister('test-claude'); | |
| console.log('✓ Provider registry integration test passed'); | |
| EOF | |
| - name: Test provider registry operations | |
| shell: bash | |
| run: | | |
| node --input-type=module -e " | |
| import { EngineRegistry } from './apps/server/src/providers/registry.js'; | |
| import { ClaudeProvider } from './apps/server/src/providers/claude-provider.js'; | |
| // Test registration | |
| const claude = new ClaudeProvider(); | |
| const metadata = EngineRegistry.register('test-claude', claude, {}); | |
| console.log('✓ Provider registered:', metadata.id); | |
| // Test retrieval | |
| const retrieved = EngineRegistry.get('test-claude'); | |
| if (!retrieved || retrieved.id !== 'test-claude') { | |
| throw new Error('Provider retrieval failed'); | |
| } | |
| console.log('✓ Provider retrieved successfully'); | |
| // Test getAll | |
| const all = EngineRegistry.getAll(); | |
| console.log('✓ Total providers registered:', all.length); | |
| // Cleanup | |
| EngineRegistry.unregister('test-claude'); | |
| console.log('✓ Provider registry integration test passed'); | |
| " |
🤖 Prompt for AI Agents
.github/workflows/provider-check.yml around lines 137 to 164: the step uses the
invalid `node --run` flag which will fail; replace `node --run << 'EOF'` with
`node --input-type=module << 'EOF'` so the heredoc is executed as ESM (allowing
top-level import), keep the existing inline script and EOF markers unchanged,
and ensure the imported paths are valid ESM module paths.
| export function calculateCost( | ||
| tokensIn: number, | ||
| tokensOut: number, | ||
| cached: number, | ||
| model: string | ||
| ): number { | ||
| const pricing = getPricingForModel(model); | ||
|
|
||
| const inputCost = (tokensIn - cached) * (pricing.inputPrice / 1_000_000); | ||
| const outputCost = tokensOut * (pricing.outputPrice / 1_000_000); | ||
| const cachedCost = cached * ((pricing.cachedPrice ?? pricing.inputPrice * 0.1) / 1_000_000); | ||
|
|
||
| return inputCost + outputCost + cachedCost; | ||
| } |
There was a problem hiding this comment.
Potential negative cost if cached tokens exceed input tokens.
Line 175 computes inputCost = (tokensIn - cached) * pricing.inputPrice / 1_000_000. If cached > tokensIn (which shouldn't happen normally but could occur with malformed data), this produces a negative cost.
Consider adding a guard:
Suggested fix
- const inputCost = (tokensIn - cached) * (pricing.inputPrice / 1_000_000);
+ const inputCost = Math.max(0, tokensIn - cached) * (pricing.inputPrice / 1_000_000);📝 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.
| export function calculateCost( | |
| tokensIn: number, | |
| tokensOut: number, | |
| cached: number, | |
| model: string | |
| ): number { | |
| const pricing = getPricingForModel(model); | |
| const inputCost = (tokensIn - cached) * (pricing.inputPrice / 1_000_000); | |
| const outputCost = tokensOut * (pricing.outputPrice / 1_000_000); | |
| const cachedCost = cached * ((pricing.cachedPrice ?? pricing.inputPrice * 0.1) / 1_000_000); | |
| return inputCost + outputCost + cachedCost; | |
| } | |
| export function calculateCost( | |
| tokensIn: number, | |
| tokensOut: number, | |
| cached: number, | |
| model: string | |
| ): number { | |
| const pricing = getPricingForModel(model); | |
| const inputCost = Math.max(0, tokensIn - cached) * (pricing.inputPrice / 1_000_000); | |
| const outputCost = tokensOut * (pricing.outputPrice / 1_000_000); | |
| const cachedCost = cached * ((pricing.cachedPrice ?? pricing.inputPrice * 0.1) / 1_000_000); | |
| return inputCost + outputCost + cachedCost; | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/lib/telemetry.ts around lines 167 to 180, the input cost
calculation can go negative if cached > tokensIn; clamp the cached value used in
cost math to at most tokensIn (e.g., effectiveCached = Math.min(cached,
tokensIn)) and use that effectiveCached when computing inputCost and cachedCost
so inputCost remains non-negative and cachedCost does not double-count beyond
tokensIn.
| async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> { | ||
| const { prompt, model, cwd, systemPrompt } = options; | ||
|
|
||
| // Build Cursor CLI arguments | ||
| const args = this.buildCursorArgs(model, systemPrompt); | ||
|
|
||
| // Add prompt as argument | ||
| if (typeof prompt === 'string') { | ||
| args.push(prompt); | ||
| } | ||
|
|
||
| try { | ||
| // Run the command and get output | ||
| const output = await this.runCursorCommand(args); | ||
|
|
||
| // Yield the output as an assistant message | ||
| yield { | ||
| type: 'assistant', | ||
| message: { | ||
| role: 'assistant', | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: output, | ||
| }, | ||
| ], | ||
| }, | ||
| }; | ||
|
|
||
| // Yield final result | ||
| yield { | ||
| type: 'result', | ||
| subtype: 'success', | ||
| result: output, | ||
| }; | ||
| } catch (error) { | ||
| yield { | ||
| type: 'error', | ||
| error: (error as Error).message, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Array prompts (multi-part with images) are silently ignored.
The executeQuery method only handles string prompts (lines 124-126). If prompt is an array (multi-part with images per the ExecuteOptions type), it's not added to the CLI args, resulting in a command executed without a prompt.
Consider throwing an error or logging a warning for unsupported prompt types:
Suggested fix
// Add prompt as argument
if (typeof prompt === 'string') {
args.push(prompt);
+ } else if (Array.isArray(prompt)) {
+ // Cursor CLI doesn't support multi-part prompts
+ throw new Error('Cursor CLI does not support multi-part prompts with images');
}📝 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.
| async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> { | |
| const { prompt, model, cwd, systemPrompt } = options; | |
| // Build Cursor CLI arguments | |
| const args = this.buildCursorArgs(model, systemPrompt); | |
| // Add prompt as argument | |
| if (typeof prompt === 'string') { | |
| args.push(prompt); | |
| } | |
| try { | |
| // Run the command and get output | |
| const output = await this.runCursorCommand(args); | |
| // Yield the output as an assistant message | |
| yield { | |
| type: 'assistant', | |
| message: { | |
| role: 'assistant', | |
| content: [ | |
| { | |
| type: 'text', | |
| text: output, | |
| }, | |
| ], | |
| }, | |
| }; | |
| // Yield final result | |
| yield { | |
| type: 'result', | |
| subtype: 'success', | |
| result: output, | |
| }; | |
| } catch (error) { | |
| yield { | |
| type: 'error', | |
| error: (error as Error).message, | |
| }; | |
| } | |
| } | |
| async *executeQuery(options: ExecuteOptions): AsyncGenerator<ProviderMessage> { | |
| const { prompt, model, cwd, systemPrompt } = options; | |
| // Build Cursor CLI arguments | |
| const args = this.buildCursorArgs(model, systemPrompt); | |
| // Add prompt as argument | |
| if (typeof prompt === 'string') { | |
| args.push(prompt); | |
| } else if (Array.isArray(prompt)) { | |
| // Cursor CLI doesn't support multi-part prompts | |
| throw new Error('Cursor CLI does not support multi-part prompts with images'); | |
| } | |
| try { | |
| // Run the command and get output | |
| const output = await this.runCursorCommand(args); | |
| // Yield the output as an assistant message | |
| yield { | |
| type: 'assistant', | |
| message: { | |
| role: 'assistant', | |
| content: [ | |
| { | |
| type: 'text', | |
| text: output, | |
| }, | |
| ], | |
| }, | |
| }; | |
| // Yield final result | |
| yield { | |
| type: 'result', | |
| subtype: 'success', | |
| result: output, | |
| }; | |
| } catch (error) { | |
| yield { | |
| type: 'error', | |
| error: (error as Error).message, | |
| }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/server/src/providers/cursor-provider.ts around lines 117 to 158, the
executeQuery currently only handles string prompts and silently ignores array
prompts; update the method to detect when prompt is an array and handle it
explicitly: if the array contains any non-text parts (e.g., images) throw or
yield an error informing the caller that multi-part/image prompts are
unsupported by the Cursor CLI, otherwise join all text segments (preserving
order and spacing) into a single string and push that into args; also add a
debug warning log when converting an array of text segments so the behavior is
visible.
| // Probe a specific provider's capabilities | ||
| router.post('/probe/:providerId', createProbeProviderHandler()); | ||
|
|
||
| // Probe all providers | ||
| router.post('/probe/all', createProbeAllProvidersHandler()); |
There was a problem hiding this comment.
Route ordering bug: /probe/all will never be matched.
Express matches routes in registration order. Since /probe/:providerId is registered before /probe/all, a request to /probe/all will be caught by the parameterized route with providerId = "all".
🔎 Proposed fix: Register specific route before parameterized route
// Probe a specific provider's capabilities
- router.post('/probe/:providerId', createProbeProviderHandler());
-
- // Probe all providers
router.post('/probe/all', createProbeAllProvidersHandler());
+
+ // Probe a specific provider's capabilities
+ router.post('/probe/:providerId', createProbeProviderHandler());
return router;📝 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.
| // Probe a specific provider's capabilities | |
| router.post('/probe/:providerId', createProbeProviderHandler()); | |
| // Probe all providers | |
| router.post('/probe/all', createProbeAllProvidersHandler()); | |
| // Probe all providers | |
| router.post('/probe/all', createProbeAllProvidersHandler()); | |
| // Probe a specific provider's capabilities | |
| router.post('/probe/:providerId', createProbeProviderHandler()); |
🤖 Prompt for AI Agents
In apps/server/src/routes/providers/index.ts around lines 29-33, the
parameterized route router.post('/probe/:providerId', ...) is registered before
the specific router.post('/probe/all', ...), causing '/probe/all' to be captured
as providerId="all"; fix by registering the specific '/probe/all' route before
the '/probe/:providerId' route (i.e., move the router.post('/probe/all',
createProbeAllProvidersHandler()) line above the
router.post('/probe/:providerId', createProbeProviderHandler()) line) so the
literal route is matched first.
| export class AgentMonitorService { | ||
| private db!: Database.Database; | ||
| private dbPath: string; | ||
| private pidCheckInterval?: NodeJS.Timeout; | ||
| private pidCheckIntervalMs = 30000; // 30 seconds | ||
|
|
||
| constructor(dataDir: string) { | ||
| this.dbPath = path.join(dataDir, 'agents.db'); | ||
| } |
There was a problem hiding this comment.
Database access before initialize() will throw.
The db property uses definite assignment assertion (!), but if any method (e.g., register(), getAgent()) is called before initialize(), it will throw an undefined access error. Consider adding a guard or lazy initialization.
🔎 Proposed fix: Add initialization guard
+ private initialized = false;
+
+ private ensureInitialized(): void {
+ if (!this.initialized) {
+ throw new Error('AgentMonitorService not initialized. Call initialize() first.');
+ }
+ }
+
async initialize(): Promise<void> {
await ensureDir(path.dirname(this.dbPath));
this.db = new Database(this.dbPath);
this.db.pragma('journal_mode = WAL');
this.createSchema();
+ this.initialized = true;
// Start PID monitoring
this.startPIDMonitoring();
}Then call this.ensureInitialized() at the start of methods that use this.db.
Also applies to: 128-138
🤖 Prompt for AI Agents
In apps/server/src/services/agent-monitor.ts around lines 113-121 (and similarly
128-138), the class uses a definite-assigned this.db which will be undefined if
methods are called before initialize(); add a private async ensureInitialized()
that initializes/open the database if not already set (or throws a clear error),
then call await this.ensureInitialized() at the start of all public methods that
access this.db (e.g., register(), getAgent(), etc.); alternatively change to
lazy-initialize the DB in ensureInitialized and remove the definite-assignment
operator to prevent undefined access.
| private rowToAgent(row: unknown): AgentRecord | null { | ||
| if (!row || typeof row !== 'object') { | ||
| return null; | ||
| } | ||
|
|
||
| const r = row as Record<string, unknown>; | ||
|
|
||
| return { | ||
| id: String(r.id ?? ''), | ||
| parentId: r.parent_id ? String(r.parent_id) : null, | ||
| status: (r.status ?? 'pending') as AgentStatus, | ||
| engine: String(r.engine ?? ''), | ||
| model: String(r.model ?? ''), | ||
| pid: r.pid !== null ? Number(r.pid) : null, | ||
| workingDir: String(r.working_dir ?? ''), | ||
| featureId: r.feature_id ? String(r.feature_id) : null, | ||
| beadsId: r.beads_id ? String(r.beads_id) : null, | ||
| prompt: String(r.prompt ?? ''), | ||
| createdAt: Number(r.created_at ?? 0), | ||
| startedAt: r.started_at !== null ? Number(r.started_at) : null, | ||
| completedAt: r.completed_at !== null ? Number(r.completed_at) : null, | ||
| error: r.error !== null ? String(r.error) : null, | ||
| telemetry: r.telemetry_json | ||
| ? (JSON.parse(String(r.telemetry_json)) as ParsedTelemetry) | ||
| : null, | ||
| sessionId: r.session_id ? String(r.session_id) : null, | ||
| metadata: r.metadata_json | ||
| ? (JSON.parse(String(r.metadata_json)) as Record<string, unknown>) | ||
| : null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
JSON.parse can throw on malformed data—missing error handling.
If telemetry_json or metadata_json columns contain invalid JSON (e.g., due to data corruption or schema migration issues), JSON.parse will throw and crash the entire query operation.
🔎 Proposed fix: Wrap JSON parsing in try-catch
+ private safeJsonParse<T>(json: string | null | undefined): T | null {
+ if (!json) return null;
+ try {
+ return JSON.parse(json) as T;
+ } catch {
+ console.warn('[AgentMonitor] Failed to parse JSON:', json.substring(0, 100));
+ return null;
+ }
+ }
+
private rowToAgent(row: unknown): AgentRecord | null {
// ... existing validation ...
return {
// ... other fields ...
- telemetry: r.telemetry_json
- ? (JSON.parse(String(r.telemetry_json)) as ParsedTelemetry)
- : null,
+ telemetry: this.safeJsonParse<ParsedTelemetry>(r.telemetry_json as string),
sessionId: r.session_id ? String(r.session_id) : null,
- metadata: r.metadata_json
- ? (JSON.parse(String(r.metadata_json)) as Record<string, unknown>)
- : null,
+ metadata: this.safeJsonParse<Record<string, unknown>>(r.metadata_json as string),
};
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/src/services/agent-monitor.ts around lines 638 to 668, the
JSON.parse calls for telemetry_json and metadata_json can throw on malformed
data; update rowToAgent to guard parsing by wrapping each JSON.parse in a
try-catch (separately for telemetry_json and metadata_json), return null for
that field if parsing fails (or optionally log the parsing error using the
service logger), and ensure no exceptions propagate out of rowToAgent so a
single bad row cannot crash the query operation.
Address high-priority and medium-priority issues from PR review: **High Priority Fixes:** - Fix P1: telemetry-factory.ts import collision - alias imported createTelemetryCapture as createTelemetryCaptureUtil - Fix P2: Pass cwd to Cursor CLI spawn - add cwd parameter to runCursorCommand and pass to spawn - Fix: Cursor auth check fragility - add --help command test to verify CLI functionality - Fix: Race condition in addTelemetry - use atomic SQL UPDATE with json_set for aggregation - Fix: generateTelemetryId uniqueness - use crypto.randomUUID() instead of Date.now() + Math.random() - Fix: Hardcoded logic in capability probe - move auth/rate limit logic to provider classes via getAuthenticationMethods() and getRateLimits() - Fix: Hardcoded rate limits in probe - delegate to provider.getRateLimits() - Fix: Incomplete cleanupOrphanedPIDs - implement full PID checking loop **Medium Priority Fixes:** - Fix: Remove unused capabilityProbe import from list.ts route - Fix: Add getCapabilities? optional method to BaseProvider for better type safety - Fix: Derive maxContextWindow/maxOutputTokens dynamically from available models **Architecture Improvements:** - Add AuthMethod and RateLimit types to BaseProvider - Add getAuthenticationMethods() and getRateLimits() abstract methods to BaseProvider - Implement provider-specific methods in ClaudeProvider and CursorProvider Addresses comments: gemini-code-assist[bot] #2649131671-2649131689 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/providers/claude-provider.ts (1)
191-241: Update Claude Sonnet 4maxOutputTokensfrom 16,000 to 64,000.Per Anthropic documentation, Claude Sonnet 4 supports up to 64,000 output tokens, not the 16,000 specified in the code. Update the value to match current specifications.
♻️ Duplicate comments (6)
apps/server/src/lib/telemetry-factory.ts (1)
91-93: Consider structured logging instead ofconsole.warn.Using
console.warnin a server application lacks structure and environment-specific configurability. Consider using a dedicated logger for better observability.Also applies to: 139-140
apps/server/src/providers/capability-probe.ts (1)
336-344:getCacheStats()returns placeholder values.The method returns
hits: 0, misses: 0despite the comment acknowledging this limitation. For production observability, consider implementing actual hit/miss counters or removing this method to avoid misleading callers.apps/server/src/services/agent-monitor.ts (3)
668-698:JSON.parsecan throw on malformed data.If
telemetry_jsonormetadata_jsoncontain invalid JSON (due to corruption or migration issues),JSON.parsewill throw and crash the entire query. Wrap parsing in try-catch to handle gracefully.
113-121: Database access beforeinitialize()will throw.The
dbproperty uses definite assignment (!), but methods likeregister()orgetAgent()called beforeinitialize()will cause undefined access errors. Consider adding an initialization guard.
813-818: Accessing private property via bracket notation breaks encapsulation.
agentMonitorInstance['dbPath']accesses a private property, which is fragile and bypasses TypeScript's access control. Consider exposing a public getter or managing the singleton differently.apps/server/src/lib/telemetry.ts (1)
169-182: Potential negative cost ifcached > tokensIn.Line 177 computes
(tokensIn - cached)which produces a negative value ifcachedexceedstokensIn(e.g., due to malformed data). This would result in incorrect cost calculation.🔎 Proposed fix
- const inputCost = (tokensIn - cached) * (pricing.inputPrice / 1_000_000); + const inputCost = Math.max(0, tokensIn - cached) * (pricing.inputPrice / 1_000_000);
🧹 Nitpick comments (4)
apps/server/src/providers/claude-provider.ts (1)
89-94: Rate limits are hardcoded and tier-dependent.The values
requestsPerMinute: 50andconcurrent: 5are fixed, but Anthropic API rate limits vary by tier (Tier 1 through Tier 4). Consider documenting that these represent a conservative baseline or exposing a way to configure them.apps/server/src/lib/telemetry.ts (1)
24-45: Type duplication with registry.ts.
ParsedTelemetryandTelemetryParserare defined identically here and inapps/server/src/providers/registry.ts(per relevant snippets). Consider consolidating to a single source of truth to avoid drift.apps/server/src/lib/telemetry-factory.ts (1)
30-34: Mutable module-level state could cause issues.
PROVIDER_PARSERSis module-level mutable state. WhileclearTelemetryParsers()is documented for testing, if called inadvertently in production, it would remove all parsers including the built-in ones (claude,anthropic,cursor). Consider reinitializing with defaults rather than clearing entirely.🔎 Proposed fix
+const DEFAULT_PARSERS: Record<string, TelemetryParser> = { + claude: claudeTelemetryParser, + anthropic: claudeTelemetryParser, + cursor: cursorTelemetryParser, +}; + const PROVIDER_PARSERS: Record<string, TelemetryParser> = { - claude: claudeTelemetryParser, - anthropic: claudeTelemetryParser, - cursor: cursorTelemetryParser, + ...DEFAULT_PARSERS, }; export function clearTelemetryParsers(): void { for (const key of Object.keys(PROVIDER_PARSERS)) { delete PROVIDER_PARSERS[key]; } + // Optionally restore defaults: + // Object.assign(PROVIDER_PARSERS, DEFAULT_PARSERS); }Also applies to: 185-189
apps/server/src/providers/capability-probe.ts (1)
300-313:probeAllsilently swallows probe failures.When a probe fails, the error is logged but the provider is simply omitted from results. Callers have no way to know which providers failed. Consider including failure information in the result map.
🔎 Proposed enhancement
+export interface ProbeResult { + providerId: string; + capability: ProviderCapability; + limits: ProviderLimits; + timestamp: number; + cacheHit: boolean; + error?: string; // Add error field +} async probeAll(providers: Map<string, BaseProvider>): Promise<Map<string, ProbeResult>> { const results = new Map<string, ProbeResult>(); for (const [id, provider] of providers.entries()) { try { const result = await this.probeWithResult(id, provider); results.set(id, result); } catch (error) { console.error(`[CapabilityProbe] Failed to probe provider "${id}":`, error); + results.set(id, { + providerId: id, + capability: DEFAULT_CAPABILITY, + limits: DEFAULT_LIMITS, + timestamp: Date.now(), + cacheHit: false, + error: error instanceof Error ? error.message : String(error), + }); } } return results; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/server/src/lib/telemetry-factory.tsapps/server/src/lib/telemetry.tsapps/server/src/providers/base-provider.tsapps/server/src/providers/capability-probe.tsapps/server/src/providers/claude-provider.tsapps/server/src/providers/cursor-provider.tsapps/server/src/routes/providers/routes/list.tsapps/server/src/services/agent-monitor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/providers/cursor-provider.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Run type checking withnpm run typecheckbefore syncing the Beads database as part of quality gates
Run linting withnpm run lintbefore syncing the Beads database as part of quality gates
Files:
apps/server/src/providers/base-provider.tsapps/server/src/providers/claude-provider.tsapps/server/src/routes/providers/routes/list.tsapps/server/src/providers/capability-probe.tsapps/server/src/lib/telemetry-factory.tsapps/server/src/services/agent-monitor.tsapps/server/src/lib/telemetry.ts
apps/server/src/routes/**
📄 CodeRabbit inference engine (CLAUDE.md)
API routes should be placed in
apps/server/src/routes/, with one file per route/resource
Files:
apps/server/src/routes/providers/routes/list.ts
apps/server/src/services/**
📄 CodeRabbit inference engine (CLAUDE.md)
Services should be placed in
apps/server/src/services/, with one service per file
Files:
apps/server/src/services/agent-monitor.ts
🧬 Code graph analysis (3)
apps/server/src/providers/base-provider.ts (1)
apps/server/src/providers/capability-probe.ts (1)
AuthMethod(21-21)
apps/server/src/routes/providers/routes/list.ts (3)
apps/server/src/providers/index.ts (1)
EngineRegistry(18-18)apps/server/src/providers/registry.ts (1)
EngineRegistry(362-362)apps/server/src/routes/providers/common.ts (1)
logError(12-12)
apps/server/src/lib/telemetry.ts (3)
apps/server/src/providers/index.ts (2)
ParsedTelemetry(22-22)TelemetryParser(21-21)apps/server/src/providers/registry.ts (2)
ParsedTelemetry(30-43)TelemetryParser(23-25)apps/server/src/lib/telemetry-factory.ts (1)
createTelemetryCapture(69-99)
🔇 Additional comments (9)
apps/server/src/providers/base-provider.ts (2)
14-26: Well-designed type definitions for authentication and rate limiting.The
AuthMethodtype andRateLimitinterface provide a clean, extensible contract for providers. The optional fields inRateLimitallow providers to report only the limits they know about.
64-100: LGTM — sensible default implementations for provider capabilities.The base class provides safe defaults (
['none']for auth,undefinedfor rate limits and capabilities) that subclasses can override. MakinggetCapabilitiesoptional with?is appropriate since not all providers may implement it.apps/server/src/providers/claude-provider.ts (1)
58-74: Good fix: capabilities now derived dynamically from model definitions.The
getCapabilities()method correctly derivesmaxContextWindowandmaxOutputTokensfromgetAvailableModels(), addressing the previous review feedback about hardcoded values.apps/server/src/lib/telemetry.ts (1)
233-235: Good: Usescrypto.randomUUID()for guaranteed uniqueness.This addresses the previous review concern about using
Date.now()andMath.random()for ID generation.apps/server/src/lib/telemetry-factory.ts (1)
17-21: Good fix: Import alias resolves name collision.The import
createTelemetryCapture as createTelemetryCaptureUtilproperly avoids the previously flagged collision with the localcreateTelemetryCapturefunction.apps/server/src/providers/capability-probe.ts (1)
146-149: Good fix: Capability detection now delegates to providers.Both
detectAuth()andprobeLimits()now properly delegate to the provider's own methods (getAuthenticationMethods()andgetRateLimits()), addressing the previous review feedback about hardcoded provider-specific logic.Also applies to: 171-174
apps/server/src/routes/providers/routes/list.ts (2)
40-45: Type assertion for optional method is acceptable but could be cleaner.The check
if (metadata.provider.getCapabilities)followed by the type assertion works correctly sincegetCapabilitiesis optional onBaseProvider. The assertion is needed because TypeScript can't narrow the type through the property access check.
24-59: Route handler is well-structured.The handler correctly uses the factory pattern, iterates providers, and handles errors appropriately. The
capabilityProbeunused import issue from previous reviews has been resolved.apps/server/src/services/agent-monitor.ts (1)
642-660: Good:cleanupOrphanedPIDsis now fully implemented.The method now fetches running agents, checks process liveness, and updates dead agents to 'failed' status. This addresses the previous review feedback about the incomplete implementation.
| addTelemetry(id: string, telemetry: ParsedTelemetry): boolean { | ||
| // Use an atomic UPDATE with JSON extraction/aggregation | ||
| // This avoids the read-modify-write race condition | ||
| const stmt = this.db.prepare(` | ||
| UPDATE agents | ||
| SET telemetry_json = json_set( | ||
| COALESCE(telemetry_json, '{"tokensIn":0,"tokensOut":0,"cached":0,"cost":0,"duration":0}'), | ||
| '$.tokensIn', | ||
| COALESCE(json_extract(telemetry_json, '$.tokensIn'), 0) + ? | ||
| ), | ||
| telemetry_json = json_set( | ||
| telemetry_json, | ||
| '$.tokensOut', | ||
| json_extract(telemetry_json, '$.tokensOut') + ? | ||
| ), | ||
| telemetry_json = json_set( | ||
| telemetry_json, | ||
| '$.cached', | ||
| json_extract(telemetry_json, '$.cached') + ? | ||
| ), | ||
| telemetry_json = json_set( | ||
| telemetry_json, | ||
| '$.cost', | ||
| json_extract(telemetry_json, '$.cost') + ? | ||
| ), | ||
| telemetry_json = json_set( | ||
| telemetry_json, | ||
| '$.duration', | ||
| json_extract(telemetry_json, '$.duration') + ? | ||
| ) | ||
| WHERE id = ? | ||
| `); | ||
|
|
||
| const result = stmt.run( | ||
| telemetry.tokensIn, | ||
| telemetry.tokensOut, | ||
| telemetry.cached, | ||
| telemetry.cost, | ||
| telemetry.duration, | ||
| id | ||
| ); | ||
| return result.changes > 0; | ||
| } |
There was a problem hiding this comment.
Invalid SQL: Multiple assignments to the same column in a single UPDATE.
The SQL statement attempts to set telemetry_json five times in sequence. SQLite will only apply the last assignment, not aggregate all values. Each subsequent json_set uses telemetry_json as input, but the column isn't updated until the statement completes, so intermediate results are lost.
🔎 Proposed fix: Nest json_set calls
addTelemetry(id: string, telemetry: ParsedTelemetry): boolean {
const stmt = this.db.prepare(`
UPDATE agents
- SET telemetry_json = json_set(
- COALESCE(telemetry_json, '{"tokensIn":0,"tokensOut":0,"cached":0,"cost":0,"duration":0}'),
- '$.tokensIn',
- COALESCE(json_extract(telemetry_json, '$.tokensIn'), 0) + ?
- ),
- telemetry_json = json_set(
- telemetry_json,
- '$.tokensOut',
- json_extract(telemetry_json, '$.tokensOut') + ?
- ),
- telemetry_json = json_set(
- telemetry_json,
- '$.cached',
- json_extract(telemetry_json, '$.cached') + ?
- ),
- telemetry_json = json_set(
- telemetry_json,
- '$.cost',
- json_extract(telemetry_json, '$.cost') + ?
- ),
- telemetry_json = json_set(
- telemetry_json,
- '$.duration',
- json_extract(telemetry_json, '$.duration') + ?
- )
+ SET telemetry_json = json_set(
+ json_set(
+ json_set(
+ json_set(
+ json_set(
+ COALESCE(telemetry_json, '{"tokensIn":0,"tokensOut":0,"cached":0,"cost":0,"duration":0}'),
+ '$.tokensIn',
+ COALESCE(json_extract(telemetry_json, '$.tokensIn'), 0) + ?
+ ),
+ '$.tokensOut',
+ COALESCE(json_extract(telemetry_json, '$.tokensOut'), 0) + ?
+ ),
+ '$.cached',
+ COALESCE(json_extract(telemetry_json, '$.cached'), 0) + ?
+ ),
+ '$.cost',
+ COALESCE(json_extract(telemetry_json, '$.cost'), 0) + ?
+ ),
+ '$.duration',
+ COALESCE(json_extract(telemetry_json, '$.duration'), 0) + ?
+ )
WHERE id = ?
`);
1 participant
Machine Integration - Milestone 1: Foundation
This PR implements the foundational infrastructure for multi-engine AI provider support with unified telemetry, capability probing, and agent monitoring.
Summary
Implements Milestone 1: Foundation (12 tasks) from the Machine Integration specification:
Changes
Provider Registry (
apps/server/src/providers/registry.ts)EngineRegistrywith Map-based O(1) provider lookuponRegister()lifecycle hooksCapability Probing (
apps/server/src/providers/capability-probe.ts)ProviderCapabilityandProviderLimitsinterfacesClaude Provider Enhancement (
apps/server/src/providers/claude-provider.ts)isAuthenticated()method for auth status checkinggetCapabilities()returning provider capability summarytelemetryParserfor usage parsingCursor Provider (
apps/server/src/providers/cursor-provider.ts)CursorProviderextendingBaseProvidercursor --versionAuth Cache (
apps/server/src/providers/auth-cache.ts)AuthCacheclass with Map-based O(1) lookupsAgent Monitor Service (
apps/server/src/services/agent-monitor.ts)Unified Telemetry (
apps/server/src/lib/telemetry*.ts)ParsedTelemetryinterface across all providersTelemetryCapturefactory with parser routingProvider API Routes (
apps/server/src/routes/providers/)GET /api/providers/list- List all registered providersPOST /api/providers/test/:id- Test provider authPOST /api/providers/probe/:id- Probe capabilitiesGET /api/providers/status- Active provider + fallbacksCI/CD (
.github/workflows/provider-check.yml)better-sqlite3compatibility testingTest Plan
npm run build:packagessuccessfullyBreaking Changes
None - this is new functionality.
Related Issues
Covers tasks M1-T1 through M1-T12 from the Machine Integration specification.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.