fix: critical defects and add comprehensive test coverage - #1
Merged
piotrlaczkowski merged 21 commits intoAug 27, 2026
Merged
Conversation
The execution engine had defects that made it unsafe for production use. Each was first reproduced with a failing test, then fixed at the root; the reproductions are kept as pkg/core/regression_test.go. Defects fixed: - Conditional edges were inert. AddConditionalEdges recorded routes into Graph.Metadata, but Execute's router only ever scanned static edges, so the primary LangGraph routing primitive silently did nothing. Routing now evaluates a node's conditional edge exactly once per visit and maps the result through its route table, with END supported as a destination. - A node returning a nil state dereferenced nil *while holding the graph read lock*, so the panic left the lock held and every later operation deadlocked. Node calls are now panic-guarded, no user code runs under a lock, and (nil, nil) means "no state update" as it does in LangGraph. - Clone panicked on any struct containing unexported fields, which includes time.Time. deepCopy is rewritten to be cycle-aware, depth-limited, and to share rather than crash on values reflection cannot rebuild. - Interrupt after Close panicked by sending on a closed channel, and a second Close panicked again. Close is now idempotent, interrupts are broadcast per in-flight run, and stream sends are guarded. - Execute stored run state on the Graph, so concurrent invocations overwrote each other's state and history. Run state is now per invocation; a Graph is safe to execute concurrently. - Context cancellation was flattened into a string, so callers could not use errors.Is. Execution now returns typed sentinels (ErrRecursionLimit, ErrInterrupted, ErrNodePanic, ErrNoRoute, ErrGraphInvalid) and preserves the context cause. - Map iteration made routing non-deterministic when several edges could match. Nodes and edges now keep insertion order. - FromJSON left nil maps behind, so the next Set panicked. Also adds the missing LangGraph state primitive: StateSchema channels with reducers (Append, AddMessages, SumInt, SumFloat, MergeMap), AddUpdateNode for partial-update nodes, and ExecuteParallelUpdates for deterministic reducer-based merging of parallel branches. Node errors are now recorded in execution history and serialised over JSON so clients can display them, and retries default to off since node bodies are frequently non-idempotent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
Checkpointing silently destroyed every state it was given. BaseState keeps
its data in unexported fields and had no MarshalJSON, so encoding/json
serialised it as "{}" — every checkpoint written by the file, Postgres and
Redis backends stored an empty state, and every API response and WebSocket
frame carrying a state sent nothing. The in-memory checkpointer clones
instead of serialising, which is why existing tests passed.
Separately, FileCheckpointer was a stub: ensureDir, writeFile, readFile,
listFiles and deleteFile were empty functions returning success, so Save
reported that it had persisted a checkpoint while writing nothing at all.
Fixed:
- BaseState now implements json.Marshaler/json.Unmarshaler, accepting both
the canonical envelope and a bare object, so state survives any transport.
- The file checkpointer performs real IO, writing through a temporary file
and renaming so a crash cannot leave a truncated checkpoint.
- Thread and checkpoint IDs become path components and arrive from API
clients, so they are validated against a strict pattern; "../escape" and
friends are rejected rather than writing outside the store.
- Panic stacks are attached to a typed *PanicError and logged, but kept out
of error strings so they cannot leak into HTTP responses.
Adds:
- test/conformance: 52 tests (68 with subtests) checking GoLangGraph against
LangGraph semantics — transitions, conditional edges, cycles, recursion
limits, reducers, streaming, checkpointing, thread isolation, durable
execution, interrupt/resume including human-in-the-loop edits, retries,
parallel branches, subgraphs, cancellation and error recovery.
- test/conformance/DEVIATIONS.md documenting the nine places GoLangGraph
intentionally differs from LangGraph, each with the reasoning and the test
that pins the GoLangGraph contract.
- pkg/core/subgraph.go: nested graphs as nodes, with input/output projection,
namespacing and build-time rejection of composition cycles.
- persistence.CheckpointSaver bridging Checkpointer to the engine's
StateSaver hook, making execution durable and resumable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
Security defects, each now covered by a test: - authMiddleware read X-API-Key and then ignored it. Every endpoint was unauthenticated with no way to turn authentication on. It now enforces configured keys with a constant-time comparison, exempting preflight and configured public paths so health probes keep working, and fails closed when auth is required but no keys are configured. - The WebSocket upgrader returned true from CheckOrigin unconditionally, so any page a user visited could open a socket to the server and drive it as that user. Upgrades now honour the same origin allowlist as the API. - CORS emitted a hardcoded "Access-Control-Allow-Origin: *" with no way to restrict it. Origins are now configurable, echoed with Vary: Origin, and credentials are only offered to a specific origin, never to "*". - Cross-origin preflight 404'd on every endpoint except /health, because routes declare concrete methods and no OPTIONS route existed. A browser would have blocked every cross-origin POST, PUT and DELETE from Studio. - No request body limit (memory exhaustion from one client), no panic recovery on the main server, and no security response headers. All three are now middleware; the recovery handler logs the stack but returns only a generic JSON error, so stacks cannot leak to clients. Concurrency and lifecycle defects: - WebSocket connections were tracked one-per-resource-ID, so a second client watching the same agent evicted the first, and the first's cleanup then removed the second's entry. Connections are now tracked per connection. - The streaming goroutine wrote to the connection while the read loop was still running. gorilla/websocket permits one concurrent writer, so frames could interleave and corrupt the stream. All writes now go through a serialising writer. - Streaming used context.Background(), so a client that disconnected left its run — and any provider calls it made — running unattended. Runs are now bound to the connection's context. - A single malformed frame terminated the client's session. Payload errors are now answered with an error frame and the session continues; only transport errors close the connection. - Stop() nil-dereferenced if called before Start, did not close hijacked WebSocket connections (so Shutdown could block), and Start reported the normal ErrServerClosed as a startup failure. Graph API implemented: list, get, topology, execute and interrupt were all placeholders returning empty arrays and "placeholder result", which is precisely what Studio's debugger reads. They now operate on a real GraphManager, return node/edge topology including conditional routes, execute graphs with per-step results, and report an interrupt as a resumable 200 rather than an error. Server coverage 31.4% -> 48.0%; 49.1% across the module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
Tool arguments are chosen by a language model that may be steered by untrusted input, so every tool reaching the filesystem, a shell or the network is attacker-controlled. The built-in tools had no meaningful boundary. Filesystem tools guarded only the file extension, with no directory restriction, so any .json/.yaml/.txt file the process could read was readable: kubeconfigs, container registry credentials, cloud credential files. Reads, writes and listings are now confined to configured roots (working directory and temp directory by default). Paths are resolved before use and symlinks are followed during validation, so a link planted inside an allowed root cannot be used to reach a file outside one. The shell tool's allowlist was security theatre. It permitted "find", whose -exec, -execdir and -ok flags run arbitrary programs, and "cat"/"grep", which read any file the process can reach — so the allowlist bounded nothing. Those commands are out of the default set, arguments containing program-executing flags or shell metacharacters are rejected, a command given as a path is rejected, and output is capped. The HTTP tool made requests to any URL a model produced, which is server-side request forgery: http://169.254.169.254/ returns cloud instance credentials, and internal admin endpoints were equally reachable. Requests to loopback, private, link-local, multicast and unspecified addresses are now refused unless explicitly permitted. The check runs at dial time against the resolved address, so a hostname that resolves to an internal address is caught even if it resolved elsewhere a moment earlier, and each redirect hop is re-validated so a public URL cannot redirect inward. Only standard HTTP methods are accepted and response bodies are capped. All limits are configurable through SecurityPolicy; the escape hatches are covered by tests that show the block is policy, not a broken client. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
ProviderConfig accepted RetryCount and RetryDelay and then never used them: no provider had any retry logic, so a single transient blip from an upstream failed the whole call while the configuration suggested otherwise. Provider failures are now classified into typed sentinels — ErrProviderUnavailable, ErrRateLimited, ErrProviderAuth, ErrProviderRequest — so callers can branch with errors.Is instead of matching message text, and so retry decisions are made on the class of failure rather than blindly. Transient failures (network errors, 5xx, 429) are retried with exponential backoff up to the configured budget; permanent failures (4xx, API-level errors) fail on the first attempt; cancellation abandons retries immediately; a Retry-After header from the provider takes precedence over the configured delay. Response bodies are now read through a size limit, so a provider that streams without ever terminating cannot exhaust memory, and error bodies are bounded before being embedded in an error. Adds provider tests covering status classification, retry exhaustion, permanent-error fast failure, Retry-After, cancellation during retries, refused connections, malformed and truncated payloads, unbounded responses, request timeouts, and concurrent use of the provider manager. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
DatabaseConnection is a public interface whose query methods return
interface{}, and the callers asserted the result to *sql.Row / *sql.Rows
without checking. Any implementation other than PostgresConnection — a
test double, an alternative backend, or a connection that was never
opened — crashed the process instead of returning an error. The
assertions are now checked, a SessionManager with no connection reports
that rather than dereferencing nil, and PostgresConnection's methods
refuse to operate on an unopened handle.
Adds persistence tests covering checkpoint JSON round-tripping, that the
file backend writes real bytes and survives a process restart, atomic
writes leaving no partial files, rejection of unsafe identifiers,
isolation of stored state from caller mutation, concurrent access across
backends, corrupt-file handling on both load and list, the durable
execution saver, and the non-SQL and nil connection paths.
Postgres and Redis integration tests are not included: no container
runtime is available in this environment to run a real instance against,
and a mocked SQL driver would assert the mock rather than the backend.
The shared Checkpointer contract that all backends implement is covered
by the conformance suite for the memory and file backends.
Package coverage 10.8% -> 36.5%; the untested remainder is almost
entirely the Postgres and Redis backends noted above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
Studio is treated as a first-class client: test/e2e/studio_compat_test.go
boots a real server on a real port and drives every request Studio's
api/client.ts makes, over real HTTP and a real WebSocket. Each assertion
names the Studio code that reads the field, so a server change that would
break the console fails here rather than in someone's browser.
Running it against the server found three genuine mismatches:
- /api/v1/agents returned a list of agent ID strings, but Studio renders
each agent's name, type, model and provider from that list, so every
field was undefined. It now returns full configurations.
- /api/v1/providers likewise returned bare names where Studio's
ProviderInfo expects an object. It now returns a description per
provider, with api_key filtered out so credentials are never served.
- AgentExecution.Error was a Go error with a json tag. A Go error marshals
to an empty object, so a failed execution reached the client as
"error":{} with no reason at all. The error is now carried in a string
field, and a test asserts the reason survives serialisation.
Studio also requests a topology using the agent's ID, so the graph
endpoints now fall back to an agent's own execution graph; without that,
the graph view was empty for every agent regardless of the fix to the
topology endpoint itself.
Adds a deterministic fake provider so the end-to-end tests exercise real
graph execution, real state handling and real transport rather than mocks
of the framework.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
Eleven fuzz targets covering the boundaries where data arrives from outside the process: checkpoint and API payloads, state values copied between nodes, routing decisions returned by user code, tool arguments chosen by a language model, and HTTP request bodies and path parameters. - core: state JSON round-tripping through both FromJSON and the json.Marshaler path, deep copying of arbitrary decoded JSON, routing with arbitrary condition results, and graph construction from arbitrary identifiers. Each execution target is watchdogged, so a hang fails rather than stalling the run. - tools: arguments fed to every built-in tool inside a sandbox, plus direct properties of the security policy — a path the policy accepts must resolve inside the allowed roots, a command it accepts must be a bare allowlisted name, and a URL it accepts must be http(s) with a host. - server: arbitrary request bodies against the write endpoints and arbitrary identifiers in path parameters, asserting no panic and no 500. Roughly 15 million executions across the targets, all clean, with the generated corpus committed as regression seeds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
…suites Unchecked error returns were hiding real failures: - Agent graph construction ignored SetStartNode and AddEndNode errors, so a malformed graph produced an agent that ran against it anyway. - appendToFile ignored the error from Close. A buffered write can fail there, so an append could be reported as successful without ever reaching the file. - The default tool registry and the quick builder discarded registration errors, leaving a registry silently missing tools the caller asked for. - The CLI's project scaffolding ignored every os.WriteFile error, telling the user a project had been generated when files had not been written. - Response encoders, response bodies, streams and SQL rows were closed or written without checking, and provider error strings were capitalised. CI changes: - Tests ran ./pkg/... only, which skipped the LangGraph conformance suite and the Studio end-to-end suite entirely; the run now covers ./... with the race detector and cross-package coverage. - The integration job passed -tags=integration to a package with no build tag, so it silently ran no tests at all. - Adds a fuzz smoke step exercising all eleven targets briefly on each run. - Lint had continue-on-error set, so failures never failed the build. The module is now clean under the repository's own golangci configuration (0 issues across pkg, cmd and examples), so the flag is removed and lint is enforced. Also removes code left dead by the engine rewrite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
The health command reported a healthy system regardless of reality. It printed "PostgreSQL: ✓ Reachable", "Redis: ✓ Reachable" and "Ollama: ✓ Reachable" without ever opening a connection — the source said "in a real implementation, you would test actual connectivity" — and reported disk space as sufficient and memory as available unconditionally. An operator relying on it got assurance that meant nothing. It also exited 1 when the optional OPENAI_API_KEY was unset. Combined with the container HEALTHCHECK, that marked every deployment without an OpenAI key permanently unhealthy, so an orchestrator would restart a perfectly working container forever. The check now: - opens real TCP connections to PostgreSQL, Redis and Ollama, and only to the ones actually configured, since defaulting to localhost reports a failure in every deployment that does not use that service; - measures free disk space with statfs and available memory from the kernel, failing on genuinely low resources; - treats missing optional provider credentials as warnings, with --strict for callers that want warnings to fail; - adds --server to probe a running server's HTTP endpoint, which is the right question for a container health check. Both Dockerfiles now use that server probe. Dockerfile.agent additionally could not be built at all: it copied configs/ and static/ from the build context, and neither directory exists in the repository, so the build failed on the first COPY. Those directories are now created in the image. Adds Docker tests that parse the Dockerfiles and check every context COPY source exists, that the images run as non-root, that the health check probes the server, and that the binary built is the binary run — plus a real docker build that runs when a container runtime is available and skips cleanly when it is not. Adds end-to-end resource tests: goroutine accounting across 100 executions and 40 WebSocket sessions, cleanup after cancelled runs, a 2001-step long-running graph, duplicate concurrent requests, repeated server restart cycles, and a slow provider not pinning a connection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
AgentExecution.ExecutionPath and StateChanges were declared to "track which nodes were executed" and "track state progression", but the code that would fill them was a placeholder assigning an empty slice. Every execution therefore reported that no nodes had run. GoLangGraph Studio highlights nodes from execution_path, so its debug view was blank for runs that had in fact executed — which is the one thing a graph debugger exists to show. The agent now streams its graph run and records the node sequence plus a before/after state snapshot per node, so a client can step through what actually happened. Adds conformance coverage for the LangGraph semantics that need a model: agent runs and their recorded output, that the user's input reaches the provider, that a ReAct loop whose model always asks for another action still terminates under its iteration bound, provider-failure reporting with a serialisable reason, cancellation, rejection of concurrent runs on one agent, history accumulation, conversation retention across turns, and tool execution including malformed arguments, unknown tools and the definitions handed to a model. Adds test/fakes: a scriptable llm.Provider shared by the conformance and end-to-end suites. It stands in for the model only — the graph engine, agent loop, tool execution, state handling and HTTP server in these tests are all the real implementations. Module coverage 56.7%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
Two examples could not be built at all. 07-tools-integration and 08-production-ready had incomplete go.sum files, so `go build` in either directory failed on a missing checksum entry for a transitive dependency. Both are tidied and now build. The repository also tracked eleven compiled example binaries totalling about 100 MB. Every clone downloaded them, they were built for one platform, and a committed binary can drift from the source it was built from with nothing to reveal the difference. They are removed and ignored; `go build ./...` inside an example produces them on demand. All twelve example modules now build from a clean checkout, and building them leaves no untracked artifacts behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
Adds docs/PRODUCTION.md covering the settings that need a deliberate choice before real traffic: API key authentication and the allowed-origin list (which also governs WebSocket upgrades), tool sandboxing for the filesystem, shell and HTTP tools, durable execution and resume, human-in-the-loop interrupts, which health probe belongs in a container, the typed error sentinels, retry policy, concurrency guarantees and observability. It ends with a table of the behaviour and payload shapes that changed, so an existing deployment knows what to check. The README now points at it, and at the conformance suite and the documented LangGraph deviations. Every code example in that document is executed by test/conformance/documented_api_test.go — the security config, the tool policy, the checkpoint-and-resume recipe, the human-in-the-loop flow, the retry policy and the full list of error sentinels — so the documentation cannot drift from the API it describes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
GeminiProvider never contacted Google. Complete matched the prompt against a few substrings and returned hardcoded text — "Hello! I'm Gemini, Google's AI assistant" for anything containing "hello", otherwise a note saying a real implementation would call the API. CompleteStream split that text on whitespace and emitted a word every 50ms to imitate streaming, and IsHealthy returned nil unconditionally. None of this was visible to a caller. NewGeminiProvider demanded an API key, the quick builder wired it up, and the CLI offered it, so configuring Gemini with a valid key produced canned text that looked like a model answering and reported itself healthy. The provider now speaks the Generative Language API: generateContent for completions and streamGenerateContent over server-sent events for streaming, with the API key as a query parameter. Message mapping follows Gemini's model rather than OpenAI's — the assistant role is sent as "model", and system prompts go in systemInstruction instead of the message list. Temperature, max output tokens and stop sequences are forwarded. Failures are classified through the shared provider sentinels, transient ones are retried, responses are size-limited, and IsHealthy queries the models endpoint. Three existing streaming tests passed only because they asserted against the mock's canned reply. They now run against a fake Generative Language API and exercise the real request building and SSE parsing. Adds 20 Gemini tests: that the API is actually called and the reply comes from it, model and key targeting, role and system-prompt mapping, generation settings, empty-request rejection, status classification, in-body errors, malformed responses, blocked prompts returning no candidates, retries, cancellation, streaming including callback errors and error statuses, health for both outcomes, and that the exposed configuration masks the key. llm coverage 39.7% -> 47.2%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
Three endpoints reported things that were not true. The checkpoints endpoint returned an empty list unconditionally, so a thread's history was invisible even when checkpoints existed — the one thing that endpoint is for. It now lists the thread's checkpoints from a configured checkpointer, ordered oldest first so a client can replay a thread, and reports a missing checkpointer instead of passing it off as an empty history. The metrics endpoint hardcoded requests_total to 0 and memory usage to "N/A", so an operator watching it saw nothing regardless of load. Two of the values it did compute were unsafe: the agent count dereferenced a nil manager, and the WebSocket count read the connection map without holding its mutex. Requests and failures are now counted as they are served, and the endpoint reports goroutines, allocated and system memory, GC cycles, registered graphs and uptime, with both unsafe reads fixed. The reload endpoint answered "Configuration reloaded successfully" without reloading anything, which would lead an operator to believe a change had taken effect. It and the logs endpoint, which returned a fabricated entry, now report that they are not implemented rather than faking success. Adds tests covering real history and its ordering, the missing-checkpointer and unsafe-thread-ID paths, that metrics move with traffic and are measured rather than constant, metrics with no agent manager, that the unimplemented endpoints do not claim success, and that concurrent traffic does not race the counters or the connection map. Server coverage 48.0% -> 51.2%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
The auto server's validation endpoint answered "valid": true for every payload without inspecting it, and every agent execution response carried "schema_valid": true beside a TODO. A client using either as a gate accepted anything, including payloads that plainly violated the schema the same server advertises. Both now validate against that schema. The validator covers the JSON Schema subset the server generates — type, properties, required, additionalProperties, minLength, maxLength, minimum, maximum, enum and items, including nested objects inside arrays. Keywords it does not implement, such as pattern and format, are ignored rather than treated as failures, so a richer schema is never reported invalid for a rule that was never checked. Errors are sorted, so the result does not vary with Go's map iteration order. The validation endpoint also rejects a type other than "input" or "output" rather than silently treating it as input. Adds 12 tests: conforming payloads, missing required fields, each wrong type, length and numeric bounds, enum membership, nested array items, unsupported keywords being ignored, empty schemas, a non-object at the root, additionalProperties, determinism across repeated runs, and that arbitrary values never panic the validator. Server coverage 51.2% -> 53.1%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
…uting Hardening the server left a second serving path untouched. AutoServer — what the `auto-serve` command runs — had no authentication of any kind, a hardcoded "Access-Control-Allow-Origin: *", a MaxRequestSize that was configured and never enforced, and panic recovery that was opt-in through a middleware name list, so omitting "recovery" from that list meant a panicking handler tore down the connection. It now takes the same SecurityConfig as Server, and recovery, the body limit and the security headers are unconditional. Cross-origin preflight is answered: most of its routes declare concrete methods, so OPTIONS fell through to 404 and a browser blocked every cross-origin write. A test written for the metrics middleware found a real data race: requestCount was a plain int64 incremented from every request goroutine. It is now atomic. The agent maps are guarded, and regenerating endpoints is refused once Start has run, since that would rewrite the route table and those maps while handlers read them. Two AutoServers also shared the process-wide agent registry, so one served the other's agents; NewAutoServerWithRegistry gives a server its own. Agents built the documented way had no identity. Every constructor in pkg/builder — and the README's own example — builds AgentConfig as a bare literal, leaving ID empty, and AgentManager keys its map by ID, so every such agent collided on "" and registering a second replaced the first. NewAgent now assigns an ID when none is given, which fixes the builder, the examples and any user code following the documentation. The ReAct graph had a routing hole. From the reason node, one edge fired only when the model requested a tool and the other only when the iteration limit was reached or the reply contained a completion marker. A model that simply answered — the ordinary outcome — matched neither, so the run failed with "no valid next node" instead of returning the answer. Both conditions now share one definition of what counts as an action, and no action requested means finish. Also: AgentSwarm.Execute returned results by ranging over a map, so Go's randomised iteration gave a caller a different agent's result each run; pipelines and swarms re-registered their agents on every call; the builder named a "mock" provider that does not exist when none was configured; and ServerConfig.LogLevel was declared and never read. Server middleware is also reordered so recovery really is outermost, which its comment claimed but the registration order contradicted. Adds tests for each defect, plus a suite that runs the README's Quick Start verbatim so the documented path cannot drift again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
…kends Three subsystems were audited in depth. Each defect below was reproduced with a failing test before being fixed. OpenAI provider --------------- Every o1/o3/o4 call failed without leaving the process: the SDK rejects max_tokens and a non-default temperature for reasoning models and the provider always sent both. Setting Stream on a request passed the flag through to a non-streaming call, which the SDK rejects locally. Timeout was configured and never applied, so an endpoint that accepted a connection and went silent hung the caller forever, and RetryCount/RetryDelay were ignored so a single 503 failed the whole call. Failures were never classified, so a 429 was indistinguishable from a 400. SetConfig could not change the endpoint, key or timeout because the SDK client was never rebuilt. SystemPrompt and configured Headers were accepted and dropped. GetModels handed out its cache by reference, and that cache was written without a lock. Streaming retries only the open: once a chunk has reached the callback, replaying would duplicate output. Multi-agent runtime ------------------- GET /config returned provider API keys, database, cache and SMTP passwords, the Slack webhook and both secret maps verbatim to any caller; the response is now redacted. Authentication accepted any non-empty API key. Rate limiting was fully configured and did nothing. GET /health answered 200 "healthy" unconditionally while listing agents reported as unhealthy beside it, so every liveness probe passed. Restart logged a message, answered "restart_initiated" and did nothing. Routing rule conditions were parsed, stored, serialised and never compared against a request, so a guarded rule matched everything. It also leaked a goroutine per agent that nothing could stop, panicked in four ordinary situations (a nil agent entry in YAML, absent routing config, an unset health-check period, absent CORS config), and had ten data races where handlers encoded live state after releasing the lock. Database backends ----------------- PostgreSQL could not be used as a Checkpointer at all: checkpoints has a foreign key to threads, nothing created the thread row, so the first save of any new thread failed on the constraint. The RAG path never worked either — SaveDocument and vector search passed a Go map and a []float64 straight to database/sql, which rejects both — and embeddings read back were discarded behind a comment. Redis leaked state across threads: keys joined the thread and checkpoint IDs with ":" and no escaping, so thread "x:a" + checkpoint "b:c1" collided with thread "x:a:b" + checkpoint "c1", and loading one thread could return another's checkpoint. Thread IDs are commonly user- or session-derived, so this is a cross-tenant leak. Redis expiry was also hardcoded to 24 hours, silently discarding every checkpoint after a day. Across both: rows.Err() was never checked, so a connection dropping mid-iteration returned a silently truncated list with a nil error — and Latest() is built on that list, so a resume could pick up from the wrong checkpoint. Delete reported success when it deleted nothing, and several error paths leaked connections. Testing ------- Real PostgreSQL 16 with pgvector and real Redis 7 were installed and run in this environment, so the database backends now have genuine end-to-end coverage rather than none; the tests skip with an explicit message when no server is reachable. The OpenAI and multi-agent suites run the real implementations against an httptest server and a scripted provider. Roughly 250 tests were added. Their teeth were checked by reverting fixes one at a time and confirming the corresponding test fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
The CLI told operators it had done things it had not. `validate` checked only that the file existed and then printed "Configuration validation completed successfully!" — unparseable YAML, missing required fields and unknown agent types all reported valid, and `--strict` was read and never used. It now parses the file, validates fields and ranges, resolves tool names, builds the agent graph and checks routing rules against defined agents. `deploy docker`, `multi-agent deploy` and `generate docker|k8s` printed a progress line and returned, generating nothing, with `--output`, `--multi-service` and `--namespace` never read. `docker build` printed the command it would have run and reported "build prepared". `visualize` ignored its file argument and rendered a hard-coded sample, and `--format json` wrote the literal string "JSON output not implemented yet" into the output file before reporting the file saved. `multi-agent validate --check-schemas` defaults to true and its two validators both returned nil under a comment saying to add logic. Each either does the work now or exits non-zero saying it does not. `migrate` never worked at all: its flags were declared on the command but read through viper, to which they were never bound, so every run died with "Unsupported database type: ". `dev --port 3000` served on 8080 for the same reason. `auto-serve` created three example agents whose registrations were all rejected for having no model, ignored the errors, and reported success while serving none of them, and it silently skipped source paths that did not exist. Three commands dereferenced optional config sections and panicked — one of them after printing "validation passed". `init` allowed path traversal and produced a project with no Go code and no go.mod while telling you to run it. In the server: AutoServer.Start ran ListenAndServe in a goroutine that only logged a bind failure, so Start blocked on ctx.Done() and the caller believed the server was up when the port was taken. It now binds before reporting success, surfaces a serve failure, and exposes the address actually bound. LoadAgentsFromDirectory scanned nothing — it listed whatever was already registered and logged that count as though it had loaded it — and now reads the directory, or says why it cannot. Adds 126 CLI tests plus server tests for the bind and directory paths. Generated Go is parsed to confirm it is valid, scaffolding is asserted file by file, docker is stubbed rather than executed, and tests bind only ephemeral ports and write only under t.TempDir(). Module coverage 58.3% -> 70.7%; 928 test cases, all passing under -race, with lint clean across pkg, cmd and examples. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
piotrlaczkowski
changed the title
…r work
main moved the module to github.com/UnicoLab/GoLangGraph, turned Agent into
an interface with BaseAgent behind it, added HITL resume hooks and stream
early-exit, and migrated the lint config to golangci-lint v2. This merges
that work with the production-readiness branch.
Conflicts resolved, and why:
- pkg/agent/agent.go routing: main's shouldAct/shouldContinueReasoning keyed
on structured `pending_tool_calls` supersede this branch's string-matching
heuristic. Both closed the same hole — a reasoning step that requested no
tool matched no edge and failed with ErrNoRoute instead of returning the
answer — and main's is the better design. Kept main's; carried over the
comment recording why the fallthrough must name a route.
- pkg/agent: AgentExecution now lives in types.go. Merged this branch's
ErrorMessage, StateChanges and JSON tags into it rather than keeping a
duplicate struct.
- ExecutionPath: this branch collects it from the graph's step stream, main
reads it from final state. Kept the stream as primary and main's read as a
fallback, so a graph that publishes its own route still reports one and
neither doubles the other.
- pkg/llm/openai.go: kept the thread-safe config snapshot and typed io.EOF
check, adopted main's early-exit propagation and its shared CollectStream
helper (which fixes the same empty-role bug this branch had fixed inline).
- pkg/server/server.go: kept the origin allowlist over main's wildcard CORS,
and the connection-scoped context plus serialized WebSocket writer. main
independently made the same /api/v1/agents fix; kept this branch's version,
which also orders the result deterministically.
- pkg/core/state.go: kept this branch's marshalling fix, which subsumes
main's reflect.Pointer rename.
Three defects the merge surfaced, fixed with regression tests:
- Resume was inferred from a non-empty conversation, which is also true of
every ordinary turn after the first. A caller's second question was dropped
instead of recorded and the agent answered the first one again; the
iteration counter never reset either, so a long-lived chat agent drifted
toward its limit. Resume is now set explicitly by SeedResumeState and
SeedConversation and consumed by the run it applies to.
- AgentConfig.EarlyExit and CompletionRequest.EarlyExit are func-typed with
only `json:"-"`. gopkg.in/yaml.v3 panics rather than skipping such a field,
so `golanggraph multi-agent init` crashed writing project YAML. Tagged
`yaml:"-"` here and on core.RetryPolicy.RetryIf and core.Node.Function.
- AgentExecution shipped untagged, serializing as Go PascalCase while the
rest of the API is snake_case, and its Error field rendered as {} so a
failed run reached clients with no reason. Tagged the struct and updated
the frontend contract test to pin the tagged names and reject the untagged
ones. Studio is updated in lockstep.
Also: migrated every new file to the UnicoLab module path, and cleared the
misspell and govet/shadow checks main enabled in the v2 lint config.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // probeTCP opens a TCP connection to verify a service is actually listening. | ||
| func probeTCP(name, address string, timeout time.Duration, optional bool) checkResult { | ||
| conn, err := net.DialTimeout("tcp", address, timeout) |
| if err := syscall.Statfs(wd, &stat); err != nil { | ||
| results = append(results, checkResult{Name: "Disk space", Warning: true, Detail: "unavailable: " + err.Error()}) | ||
| } else { | ||
| free := stat.Bavail * uint64(stat.Bsize) |
| } | ||
|
|
||
| content, err := os.ReadFile(data) | ||
| content, err := os.ReadFile(path) |
| if err := tmp.Close(); err != nil { | ||
| return err | ||
| } | ||
| if err := os.Chmod(tmpName, 0o640); err != nil { |
Both scans in the Pre-commit workflow have failed on every run on main, including at this PR's base commit, so neither has ever reported on this codebase. Neither failure is caused by this branch; both are fixed here because a security scan that cannot start is the same class of defect as the rest of this work — declared, but never actually done. Security Scan: aquasecurity/trivy-action moved from bare version tags to v-prefixed ones, so the pinned `@0.25.0` no longer resolves and the job died before checkout. Pinned to `@v0.36.0`, the current release. Dependency Check: govulncheck reported 27 vulnerabilities the framework's own code paths reach — the HTTP tool, the server's listener, and the Postgres and Redis checkpointers — all of them Go standard library, all "Found in: go1.23.12". CI pinned GO_VERSION 1.23; the fixes land across 1.24.8 through 1.25.13. Raised to 1.25 in every workflow, which resolves to a patch level carrying all 27 fixes. The Docker builder images were pinned to golang:1.23.10-alpine, which would have shipped the same standard library in the released binaries; raised to 1.25.13-alpine. go.mod keeps `go 1.23.0`: that is the minimum language version consumers need, and it is unaffected by which toolchain builds the artifacts. Verified under go1.25.13: build, vet, `go test -race ./...` across all 11 packages, and golangci-lint clean. govulncheck itself could not be re-run here — this environment's network policy denies vuln.go.dev — so the vulnerability count is CI's to confirm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
piotrlaczkowski
added a commit
that referenced
this pull request
Aug 27, 2026
piotrlaczkowski
added a commit
that referenced
this pull request
Aug 27, 2026
piotrlaczkowski
pushed a commit
that referenced
this pull request
Aug 27, 2026
PR #1 merged at 598b2ec while the gosec and pre-commit fixes were still in flight, so those land on top of the merged history rather than inside it. The only conflicts were license headers: main carries the three-line form, this commit the canonical five-line block the insert-license hook expects. Kept the canonical form in all 17 files — the three-line form is what made the hook prepend a duplicate header and fail Pre-commit Checks. Verified after the merge: gofmt clean, build, vet, `go test -race ./...` green across all 11 packages under go1.25.13, golangci-lint 0 issues, and gosec 0 issues under the workflow's exclusion set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc
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
3 participants
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
This PR addresses multiple critical defects in the GoLangGraph framework that caused data races, goroutine leaks, silent failures, and incorrect behavior. It also adds extensive test coverage for the fixed functionality and introduces production-ready documentation.
Key Changes
Critical Defects Fixed
Data races in health checking: Added mutex protection to
HealthCheckermutable fields and implemented proper lifecycle management with context cancellation for health checker goroutines, eliminating goroutine leaks that occurred when managers were created but never stopped.Concurrent state access: Implemented
Clone()methods forDeploymentStateandAgentStateto prevent data races when reading state while request handlers write to it concurrently.Silent command failures: Changed command handlers from
RuntoRunEto properly propagate errors instead of silently failing. Added explicit error handling for flag parsing that was previously ignored.Database configuration parsing: Fixed
MaxLifetimeduration parsing to fail loudly on invalid values instead of silently ignoring them, and addedCheckpointTTLconfiguration for Redis key expiration.Flag binding defect in migrate command: Fixed flags that were declared but never bound to viper, causing all database connection parameters to be ignored.
OpenAI provider thread safety: Added mutex protection to guard concurrent access to the client and model cache during
SetConfigcalls.Gemini provider implementation: Replaced hardcoded mock responses with actual API calls to Google's Generative Language API.
Request body limits: Added
MaxRequestBodyBytesconstant to prevent unbounded memory consumption from streaming request bodies.Nil pointer dereferences: Added nil checks in
BaseState.Get(),BaseState.Set(), andBaseState.GetAll()methods.New Features & Improvements
Error sentinels: Introduced sentinel errors (
ErrGraphInvalid,ErrRecursionLimit,ErrInterrupted,ErrNodePanic,ErrNoRoute,ErrGraphClosed) for proper error classification.Request rate limiting: Added
rateLimitertoMultiAgentManagerfor controlling request throughput.Routing configuration enhancements: Added match modes (
exact,prefix,suffix,contains,regex) and condition operators for more flexible routing rules.State schema support: New
pkg/core/schema.gowithReducerandChanneltypes for typed state management mirroring LangGraph's TypedDict.Subgraph support: New
pkg/core/subgraph.gofor composable graph execution.Resilience patterns: New
pkg/llm/resilience.gowith retry logic and error classification for LLM provider failures.Production documentation: Added
docs/PRODUCTION.mdcovering security, authentication, CORS, tool sandboxing, and health checking for production deployments.Test Coverage
pkg/agent/multi_agent_defects_test.go: Tests for concurrent access, state management, and request handlingpkg/agent/multi_agent_lifecycle_test.go: Tests for goroutine lifecycle and health checker cleanuppkg/llm/openai_test.go: Tests for OpenAI provider with mock API serverpkg/llm/gemini_test.go: Tests for Gemini provider implementationpkg/llm/resilience_test.go: Tests for retry logic and error handlingcmd/golanggraph/cli_test.go: CLI command testscmd/golanggraph/multi_agent_commands_test.go: Multi-agent command testscmd/golanggraph/auto_serve_command_test.go: Auto-serve command testscmd/golanggraph/health_test.go: Health check endpoint testspkg/core/fuzz_test.go: Fuzzing for state JSON round-tripspkg/core/regression_test.go: Regression tests for previously fixed bugstest/directoryCode Quality
https://claude.ai/code/session_017JM31riXDsAT7MEtDs9crc