fix: address code-review findings (SQL, UTF-8 truncation, server hardening, dedup) - #81
Merged
Merged
Conversation
ProviderUpdate and WebProviderUpdate built their SET clause only from fields the caller actually set, then always ran the query. When every field was empty (e.g. an update command invoked with no flags), opts stayed empty and the code produced `UPDATE providers SET WHERE id = ?;` — invalid SQL that surfaced as a confusing driver error instead of a clean no-op. Return early when opts has nothing to set, before building or running the query. Adds a regression test in each package confirming an update call with no fields set succeeds without error and leaves the existing row unchanged.
NewDatabase opened the sqlite connection, then returned the bare migration error without closing db on failure, leaking the *sql.DB and its underlying file handle on every failed migration run. Close db before returning in that branch; the earlier sql.Open error branch was already fine since db is nil there. Adds a test that forces a migration failure against a read-only database file and confirms NewDatabase surfaces an error and does not leave the connection locked for a subsequent open of the same path.
spawnSessionName, the diff truncation in modules/file, and the file_read truncation all sliced a string with s[:n] where n is a byte count, even though the surrounding names and constants (Chars) imply character count. When a cut point landed inside a multi-byte UTF-8 rune (Korean text, for example), the result held a broken trailing sequence. Add util.TruncateRunes, a shared helper that slices by rune count, and use it at all three sites so truncation can never split a rune. The numeric limits are unchanged, so ASCII-only behavior is byte-for-byte identical; only multi-byte input is affected.
readLine() accumulated typed input a byte at a time and removed exactly one byte on Backspace. Deleting a multi-byte UTF-8 character (a Korean syllable, for example) took several presses to clear and left buf holding a broken trailing sequence in between, which later got JSON-marshaled as the ask_user_question answer and corrupted it. Extract dropLastRune, a pure helper that uses utf8.DecodeLastRune to find the width of the trailing rune and drops that many bytes instead of one; it falls back to dropping a single byte on already-invalid trailing data. readLine() still emits one "\b \b" per Backspace press, since removing a whole rune collapses to one erased glyph on screen.
http.Server in server/app.go had no timeouts at all, and nothing in cli/serve.go ever called Shutdown, so the process just died abruptly on SIGINT/SIGTERM without draining in-flight requests. ReadTimeout and WriteTimeout are unsafe to add here: /ws upgrades to a WebSocket that can sit idle for up to pongWait (30 minutes) after hijacking the connection, and /api/v1/chat/completions streams a long response over plain HTTP, so either timeout could kill a legitimate long-lived connection. Add only ReadHeaderTimeout and IdleTimeout, which bound header-read time and keep-alive idle time without touching an established connection's lifetime. serveExecute now derives a context from signal.NotifyContext for os.Interrupt and syscall.SIGTERM, and a background goroutine calls WebServer.Shutdown with a bounded timeout once that context is done. ListenAndServe's return is now treated as a clean exit when it is http.ErrServerClosed, since that is the expected result of a Shutdown-triggered close rather than a real failure.
executeTool only ran the approval gate when both the tool was Dangerous and approve was non-nil, so a nil approve callback let a Dangerous tool (bash_exec, file writes/edits, browser_*) execute unguarded. No production caller passes nil for a real turn today, but the gate was fail-open, which is the wrong default for a security control: a future caller that forgets to wire approve would silently lose the gate instead of erroring loudly. Make a missing approve callback deny Dangerous tools instead of skipping the check, so the failure mode is loud rather than silent.
SendChatMessage duplicated the four-step system-message assembly (summary, memory index, skill catalog, agent soul) once for the initial union and again after context compaction recomputes tail/union. Extracted the shared prepend logic into prependSystemContext, placed above SendChatMessage per the dependency-order convention, and had each call site pass its own memoryIndex/skillCatalog rather than recomputing them inside the helper. No behavior change: prepend order and conditions are identical.
TestUpdateCheckStartWritesTheCacheInTheBackground busy-polled util.UpdateCacheRead for up to 2 seconds waiting for updateCheckStart's background goroutine to write the cache, a timing-dependent approach that risked flaking under a slow CI runner or -race overhead. Added an optional updateCheckDone channel that updateCheckStart's goroutine closes on exit when set; production callers leave it nil so behavior is unchanged, and the test sets it and blocks on it with select instead of sleeping in a loop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Four unrelated bugs/gaps found during a code review pass, fixed as four independent commits (one per theme):
core/provider.go/core/web_provider.go:ProviderUpdate/WebProviderUpdatebuiltUPDATE ... SET WHERE id = ?;(empty SET clause, a SQL syntax error) when called with no fields set — e.g.mininaru provider set <id>/mininaru webprovider set <id>with no flags. Now a no-op.util/database.go'sNewDatabasealso leaked the*sql.DBwhenmigrations()failed — now closed before returning the error.util/text.go(new): four spots sliced strings by byte index while treating the limit as a character count —core/agent_spawn.go'sspawnSessionName, and two truncation points inmodules/file/file.go— corrupting multi-byte UTF-8 (e.g. Korean) when the cut landed mid-character. Added a sharedutil.TruncateRunesand used it at all three.modules/client/render.go'sreadLine()had the same class of bug on Backspace (removed one byte instead of one rune, corrupting an in-progressask_user_questionanswer) — fixed with an extracted, unit-testeddropLastRunehelper.server/app.go/cli/serve.go: the HTTP server had no timeouts and nothing handledSIGINT/SIGTERMfor a graceful shutdown. AddedReadHeaderTimeout/IdleTimeoutonly —ReadTimeout/WriteTimeoutwere deliberately left out since they'd cap the/wsWebSocket and the/api/v1/chat/completionsSSE stream.core/tool_loop.go'sexecuteToolalso ran a Dangerous tool unguarded wheneverapprove == nil(fail-open); now it fails closed instead.core/chat.go:SendChatMessageduplicated its system-message-assembly block (summary/memory/skill/soul prepends) once before and once after context compaction — extracted intoprependSystemContext, pure refactor.cli/update_test.go's background-cache test busy-polled for up to 2s instead of waiting on a real completion signal — now blocks on a completion channel (verified 5 consecutive passes).How it was verified
make test-racepassesEach fix carries its own regression test (see individual commits):
TestWebProviderUpdateNoFieldsIsNoop/TestProviderUpdateNoFieldsIsNoop,TestNewDatabaseClosesConnectionOnMigrationFailure,util/text_test.go,TestSpawnSessionNameTruncatesMultiByteRunesSafely, a multi-byte case inmodules/file's existing truncation test,TestDropLastRune*inmodules/client, and the existing chat/context test suites re-run to confirm thechat.gorefactor is behavior-preserving.Checklist
:=, onevarblock per function in first-use order witherrlast, callees before callers, andmainunconditionally last.gofiles carry the two-line SPDX headerGPL-3.0-only, matching the project