Handle tool-call streaming loops and strengthen model cache validation#23
Handle tool-call streaming loops and strengthen model cache validation#23christopherkarani wants to merge 3 commits intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Code ReviewThanks for these improvements. The overall direction is good — the streaming loop is significantly cleaner than the old one-shot approach and the disk-validation in Bugs / Correctness1.
let models = await cache.allCachedModels() // may include stale entries
for m in models {
await cache.isCached(m.identifier) // returns false ← mismatch
}
2. Double progress callback per event (ModelManager.swift)
Design / Code Quality3. Redundant continuation variables (ModelManager.swift, lines ~696-704) Three variables are declared for what is ultimately the same value: var speedContinuation: AsyncStream<DownloadProgress>.Continuation?
var speedContinuationRef: AsyncStream<DownloadProgress>.Continuation?
// ...
speedContinuationRef = speedContinuation // ← copy
let continuationSnapshot = speedContinuationRef // ← another copy
4. Redundant The 5. Changing a non-throwing The call sites in Minor6. Tool-round counting semantics (ChatSession.swift) The guard 7. No new tests The tool-call streaming loop is the most complex change here. The existing
These are all high-value cases that would also catch regressions in the Summary
The core logic in all three areas is sound. Addressing the |
|
@claude review and fix all identified issues |
|
Claude finished @christopherkarani's task in 9m 28s —— View job All Identified Issues FixedAll seven issues from the previous review have been addressed in commit Tasks
What was changed
The
Both call sites now use
The doc comment now explicitly states the inclusive semantics:
Four new
|
- ModelCache: filter allCachedModels() through validateCachedEntry so its results are consistent with isCached/info(for:)/localPath(for:) - ModelManager: collapse three redundant speedContinuation variables into one, remove no-op finish() inside speedTask, and fire the user progress callback once per tick (from the speed stream task) instead of twice - GenerationChunk: restore non-throwing public init with precondition guards to preserve backward compatibility; add public static PartialToolCall.validated(...) throwing factory for use in streaming code that receives potentially malformed server data - OpenAIProvider+Streaming: use PartialToolCall.validated at both call sites and upgrade log level from warning to error for invalid fragments - ChatSession: clarify maxToolCallRounds doc comment with inclusive/exclusive semantics (0 = no rounds, N = exactly N rounds) - ChatSessionTests: extend MockTextProvider with setQueuedStreamChunkSets and add four streaming tool-call loop tests covering: N rounds completing successfully, maxToolCallRounds overflow, nil toolExecutor, and maxToolCallRounds = 0 Co-authored-by: Christopher Karani <christopherkarani@users.noreply.github.com>
|
Claude finished @christopherkarani's task in 3m 9s —— View job Code Review & Fix PassTasks
Issues Found & Fixed1. Build-breaking: The // Before (broken — ToolCallRetryPolicy is undefined)
let capturedState: (..., toolCallRetryPolicy: ToolCallRetryPolicy, ...) = withLock { ... }
// After (correct)
let capturedState: (..., toolCallRetryPolicy: ToolExecutor.RetryPolicy, ...) = withLock { ... }2.
// After: matches send(_:) behaviour
return (messages, toolExecutor, toolCallRetryPolicy, max(0, maxToolCallRounds))3. These three accessors read Summary
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ate totalSize/count - ChatSession.stream(): fix `ToolCallRetryPolicy` (undefined) to the correct `ToolExecutor.RetryPolicy` type in the capturedState tuple annotation, which was a build-breaking error introduced in the previous commit. - ChatSession.stream(): clamp `maxToolCallRounds` to `max(0, ...)` before capturing into the loop, matching the behavior of `send(_:)` and producing a consistent error message for negative values. - ModelCache.totalSize(): filter through validateCachedEntry() before summing sizes, so stale (deleted from disk) entries are excluded and pruned, consistent with isCached/info/localPath/allCachedModels. - ModelCache.count / isEmpty: route through validateCachedEntry() so callers get an accurate count of entries that actually exist on disk, consistent with the rest of the validated accessor family. Co-authored-by: Christopher Karani <christopherkarani@users.noreply.github.com>
1 participant
Summary
ChatSessionthat handles tool calls, executor retries, and proper state updates while cancelling and error handling are improvedPartialToolCallconstruction with validation that logs and skips invalid fragments across OpenAI streaming helpersModelCachelookups to verify disk paths and prune stale entries plus enrich model download progress with speed/ETA trackingTesting