fix: deep review findings — security, engine, storage, tools - #195
Merged
Conversation
Pulls provider autodetection and model ID canonicalization fixes
- H5: drop RLock in Registry.Execute before tool execution so parallel tools within a turn are no longer serialized behind a read lock. - H6: preserve partial tool output before the error prefix so the LLM sees what a tool produced before it failed. - M1: let free/local models (price 0) win as the compaction model. - M14: widen isSmallTalkPrompt via conservative phrase + filler matching (guarded so a trailing request clause like "hi, inspect this" stays false). - C5: make SnowballDetector.IsSnowballing an honest growth-rate check; the progress-per-token clause was provably redundant with a constant signal. - H2: remove dead slashSel mutation inside by-value View() (clamped in Update).
…nel sends - H4: move the activity-nudge injection from persisted (AppendSystemContext, every-turn leak) to ephemeral opts.System so it never accumulates. - M2: cache the spec constitution.md read keyed on path + mtime, so unchanged files are not re-read from disk every turn. - H11: remove the dead timeout context in the auto-remember goroutine (Remember does not accept a context); document it as best-effort fire-and-forget. - H12: add an emit() helper that selects on ctx.Done() and use it for all post-stream bare channel sends, so an early-exit consumer can't leak the agentLoop goroutine.
Pull the per-turn ChatOptions assembly (system-prompt tiering, ephemeral injections for beliefs/memory nudge/smart skills/spec stage/work mode, and promoted tools) into a dedicated buildTurnOptions(turnContext) helper. Pure behavior-preserving extraction: agentLoop now hands a turnContext to the helper and uses the returned opts, shrinking the loop and giving the prompt-assembly logic a single, testable home.
…or fix, rename pkg
Fix the NetworkProxy request routing: ServeMux pattern '/' does not match authority-form CONNECT targets (e.g. 'CONNECT host:port'), so HTTPS clients fell through to 404. Implement ServeHTTP directly on the proxy so CONNECT tunneling and plain-HTTP forwarding both route correctly. Bind the egress proxy with tcp4 when host is 0.0.0.0 so containers reach it via IPv4 host.docker.internal (on macOS/Orbstack, 'tcp'+'0.0.0.0' yields an IPv6 [::] socket that drops IPv4 connections). Make docker-CLI discovery robust to test/daemon subprocess PATH differences by also checking /Users/$USER/.orbstack/bin/docker. Add an end-to-end egressproxy-tagged integration test that verifies an allowed domain tunnels through (real HTTPS to github.com) while a blocked domain is rejected (403). Bump go.mod and go.work to go1.26.6, which resolves the stdlib vulnerabilities (GO-2026-6218, GO-2026-6091, GO-2026-6090) flagged by govulncheck. EOF )
CI runs on Go 1.26.5 with GOTOOLCHAIN=local, so requiring 1.26.6 broke every check (vet, test, build). Keep all module files on 1.26.5; the stdlib vulns are pre-existing and not introduced by these changes. EOF )
Go 1.26.6 patches stdlib vulnerabilities (GO-2026-6218, GO-2026-6091, GO-2026-6090) that govulncheck flags as reachable from pre-existing code. Bump go.mod, go.work, and every CI workflow's go-version pin so setup-go installs 1.26.6 — this satisfies go.work, unblocks the build, and clears govulncheck. EOF )
The parent branch (feat/sync-eyrie-add-providers) moved the eyrie submodule to commit 7ec579abfaa9 but did not update go.mod's require version, which broke the submodule/module release parity check. Update the pseudo-version to resolve to the pinned commit. EOF )
go work sync updates go.sum to reflect the new eyrie pseudo-version. EOF )
The Dockerfile, Dockerfile.daemon, Makefile, and setup-deps action all generate a go.work with a hardcoded Go version. Bump each from 1.26.5 to 1.26.6 so they match go.mod/go.work and the Docker/CI build does not reject the version mismatch. EOF )
Align the Docker build image with go.mod/go.work (1.26.6) so the build does not reject the toolchain version inside the container (GOTOOLCHAIN=local prevents auto-download). Pinned by digest for supply-chain hardening. EOF )
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.
Summary
Fixes findings from the deep code review across security, engine, storage, tool, and CLI subsystems. All changes verified twice: build ✓, vet ✓, lint 0 ✓, full test suite ✓.
Security
NetworkProxy(default allowlist: npm/PyPI/GitHub/Go/crates.io/Docker Hub); container reaches proxy viahost.docker.internal. Opt out withHAWK_DISABLE_EGRESS_PROXY=1.allowedkey, skipping injected spoofed fragments.*without creds./bot<TOKEN>) redacted in request logs.git statusno longer auto-approvesgit status; rm -rf /.Engine
buildTurnOptions(turnContext)from the 877-lineagentLoop.emit()helper withctx.Done()guard on all post-stream channel sends.compactModelnow lets free/local models (price 0) win for summarization.SemanticCompressrecent-detection now uses each member's original index (was matching by content equality, wrong on repeated text).Storage
SQLiteStore+GainTrackerand their tests (2063 lines, zero production callers).safewritetemp+rename).internal/engine/session→internal/engine/compression(removed naming collision withinternal/session).BatchedWAL's durability window.Tools
Registry.Executereleases the read lock before tool execution (was serializing parallel tools).ToolSchema+SchemaProviderinterface;ValidateToolInputnow checks types/enums when provided.BashandFileReadconverted as reference implementations.Refactorerdoc comment (regex-based, not AST-based) with scope limitations noted.CLI
slashSelmutation inside by-valueView().Session
CoherenceTracker.GetStatenow deep-copies slices/pointers (was sharing internal arrays with callers).Lint
errorlint; fixed 19 production files (errors.Asfor type assertions,%wfor error wrapping).Co-Authored-By: Claude noreply@anthropic.com