feat(core): add core/event and core/provider packages#11
Merged
Conversation
Implements phase 3 of the v0.1 bootstrap (#1). Lands the closed sum type of streaming events and the single Provider implementation v1 ships, with table-driven tests, two fuzz targets, and an in-memory fake for downstream tests. core/event/ - Sealed Event interface (unexported isEvent on each variant), with TextDelta, ToolCallRequest, ToolCallResult, Finish, Error. - Supporting types: FinishReason, Usage, ToolContent. - Each event marshals with a leading "type" discriminator; Decode dispatches on it and returns ErrUnknownType for unknown variants. - Error round-trips through JSON as a string; documented as a deliberate trade-off for session replay. - Two fuzz targets (FuzzDecode, FuzzDecodeNeverPanics). core/provider/ - Provider interface (Stream, Name) with Request, Message, ToolCall, Tool, Role. - OpenAICompatible adapter against any OpenAI-compatible runtime (Ollama, mlx-lm, llama.cpp's server, vLLM). Default http.Client has dial/header/idle timeouts but no overall timeout (streams may run for many minutes). - SSE parsing buffers tool-call argument deltas across frames and emits a single ToolCallRequest per call only when the runtime signals finish_reason: tool_calls (spec §8.4). Accumulated args are validated as JSON before emission; invalid args produce an Error event rather than a partial tool call. - Tool calls are emitted in stable index order even when the server interleaves indices in delta frames. - SSE comments (lines starting with ":") and missing/late [DONE] / finish_reason are tolerated; the provider synthesizes a Finish if the stream ends without one. - Stream → runStream → handleLine → handleChoice / flushPending keeps cognitive complexity under the lint threshold. - FakeProvider replays scripted FakeScript entries in order and captures Request values for assertion. ErrFakeExhausted signals end of script. Local quality gate (matches CI): - go test -race -shuffle=on -count=1 ok - coverage on core/... 90.6% (gate: 80%) - gofmt -l . clean - go vet ./... clean - go mod tidy -diff clean - golangci-lint run ./... 0 issues - govulncheck ./core/... no vulnerabilities - FuzzDecode 15s 6.3M execs, 0 panics - FuzzDecodeNeverPanics 10s 3.7M execs, 0 panics Closes #4.
|
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. |
This commit fixes the proximate CI failures on PR #11 and, more importantly, closes the process gap that let those failures hit CI in the first place: there was no single command a developer (human or agent) could run that exercised everything CI exercises. Process: `make verify` and a pre-push hook - `make verify` is now the canonical "ready to push" gate. It runs every check CI runs that can run locally: * gofmt-check (fail on any unformatted file) * go mod verify, go mod tidy -diff * go vet * golangci-lint config verify, golangci-lint run * go test -race -shuffle=on -count=1 -covermode=atomic -coverprofile=coverage.out * cross-compile build matrix across darwin/{amd64,arm64}, linux/{amd64,arm64}, windows/amd64 — same matrix as ci.yml * gosec, govulncheck * semgrep IN THE SAME DOCKER IMAGE CI USES (pinned manifest digest); skipped with a loud warning if Docker is not running * 5s fuzz pass per Fuzz* target Output ends with a clear PASS/FAIL banner. - .githooks/pre-push runs `make verify` automatically before every push. Enable once per clone: git config core.hooksPath .githooks Bypass for emergencies with SKIP_VERIFY=1; document the bypass reason in the PR. - CONTRIBUTING.md and docs/ci.md updated to make `make verify` the documented prerequisite for pushing. Process side-effect: the gate caught two of its own bugs - SHELL was defaulting to /bin/sh; the fuzz-quick target uses bash process substitution. Now SHELL := /bin/bash with -eu -o pipefail. - The `vuln` target's "is there Go source yet" detection used `ls **/*.go` which doesn't expand without globstar. Replaced with the same `find` invocation the workflows use. CI failures fixed - .github/workflows/fuzz.yml: the `Fuzz` step interpolated ${{ inputs.duration }} and ${{ matrix.target }} directly into a shell `run:` block. Semgrep flagged this as yaml.github-actions.security.run-shell-injection — correctly: matrix and inputs values are user-controllable in some flows and should never reach a shell via direct interpolation. Remediation per the rule's docs: pass via `env:` and reference the env vars from inside the shell block. - .github/workflows/ci.yml: Codecov upload had fail_ci_if_error: true while the repository is not yet registered with Codecov, producing a hard "Repository not found" failure on every PR. Set fail_ci_if_error: false until registration is complete; docs/ci.md "Codecov registration" explains the one-time setup and notes the flag should be flipped back to true afterward. Verified locally - make verify PASS (8 lint checks, tests with race+coverage, 5-arch build matrix, gosec, govulncheck, semgrep clean, FuzzDecode + FuzzDecodeNeverPanics 5s each clean)
Six should-fix items, one nit, and one design clarification from the PR #11 critical review. Tests added for each behavioral change. core/provider/openai_compatible.go - SSE multi-line `data:` frames are now correctly coalesced (S1). `streamState.dataBuf` accumulates `data:` lines until the blank-line boundary; a new `dispatchData` helper parses the joined payload. Single-line frames still work; multi-line now works too. Spec-compliant. - `[DONE]` and EOF synthesis no longer drop the tool_calls signal (S3). When pending tool calls are flushed at end-of-stream without a prior `finish_reason`, the synthesized Finish now carries Reason=tool_calls so the agent loop dispatches the call rather than treating the turn as complete. - Tool arguments are validated to be JSON objects, not just valid JSON (S4). A model that emits `null`/`[]`/scalar/etc. now surfaces an `event.Error` here rather than letting an MCP server reject the call later with a less-actionable message. - `cfg.Headers` is deep-copied at construction (S6). Stored on a new `OpenAICompatible.headers` field. Caller-side mutations after `NewOpenAICompatible` no longer affect later requests. - Comment on `chatToolFunctionSpec(t)` clarifies that struct conversion is *safer* than a named-field literal (a field rename or reorder on either type becomes a compile error, not a silent wire-format drift) (S5 follow-up). core/event/event.go - Removed the unused `FinishReasonError` constant (S8). The streaming code emits `event.Error` for failures rather than a Finish with an error reason; the constant was dead. Doc note added so this stays explicit. - Doc note on `Error` describing the load-bearing Marshal/Unmarshal asymmetry (N1). core/event/event_test.go - Renamed `TestSealedInterface` to `TestEventInterfaceMembership` with an honest doc (D2). Go's interface satisfaction is structural; the test confirms membership, not closure. .githooks/pre-push - Reads stdin to detect tag pushes and branch deletes (N8). Skips `make verify` for those cases — verifying source state on a tag push is redundant and on a delete is meaningless. Makefile - Comment on `make verify` now lists CI-only checks (CodeQL, Scorecard, dependency-review, Trivy SARIF upload, Codecov upload) so the "every check CI runs that can run locally" claim is unambiguous (D6). Tests added (5) - TestOpenAICompatible_DoneWithPendingToolCallsKeepsToolCallsReason locks in S3. - TestOpenAICompatible_MultiLineDataFrame locks in S1. - TestOpenAICompatible_ContentFilterFinishReason exercises the previously-untested FinishReasonContentFilter wire value. - TestOpenAICompatible_OversizedLineSurfacesAsError covers a 2 MiB SSE line and asserts it surfaces as event.Error (no panic, no silent truncation). - TestOpenAICompatible_NonObjectToolArgsEmitsError locks in S4. - TestOpenAICompatible_HeadersDeepCopiedAtConstruction locks in S6. Tracking issue - #12 opened to flip codecov fail_ci_if_error back to true once the repo is registered with Codecov (D5). Verified locally - make verify PASS end-to-end - coverage on core/... 91.5% (was 90.6%) - semgrep 0 findings - all 5 new tests green under -race -shuffle=on
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
2 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.
Closes #4. Part of #1 (project bootstrap, v0.1).
This is phase 3 of 7 in the v0.1 bootstrap. It lands the first Go source in the repository: the closed sum of streaming events, the [Provider] interface, the single OpenAI-compatible adapter that v1 ships, and an in-memory fake for downstream tests.
Because this PR introduces source for the first time, the empty-tree gates added in phase 2 (#3) flip to active. CI will run lint, test (ubuntu + macOS), the 5-arch build matrix,
gosec,semgrep,govulncheck, andcodeqlagainst real Go code on this PR.Summary
core/event/— sealed Event interface with five concrete variants. JSON discriminator-based marshaling drives session persistence and replay; Decode dispatches ontypeand returns ErrUnknownType for unknown variants.core/provider/— Provider interface, request/message types, and a single concrete implementation: OpenAICompatible targeting any OpenAI Chat Completions-compatible runtime (Ollama,mlx-lm,llama.cpp's server, vLLM).core/provider/testing.go— FakeProvider for downstream tests in phases 4 (Phase 4: core/mcp + core/session #5) through 7 (Phase 7: examples/acme-revenue + ADR + architecture doc #8).core/event/. ~10 million combined executions locally before this PR; 0 panics, 0 round-trip mismatches.core/...— thecore/Codecov gate (80%) lands well in the green.Phase mapping
feat/bootstrap-skeletonfeat/bootstrap-cicore/event+core/provider(this PR)feat/bootstrap-event-providercore/mcp+core/sessionfeat/bootstrap-mcp-sessioncore/loop+core/router+core/approvalfeat/bootstrap-loop-router-approvalcmd/askCLIfeat/bootstrap-askexamples/acme-revenue+ ADR + architecture docfeat/bootstrap-acme-exampleWhat lands
core/event/event.go(236 lines, 90% coverage)Eventinterface sealed by an unexportedisEvent()method on each variant. Outside packages cannot extend the type set, so future exhaustive switches stay meaningful.TextDelta,ToolCallRequest,ToolCallResult,Finish,Error.FinishReason(5 enum values),Usage(token accounting),ToolContent(text / image / resource).MarshalJSONwrites a flat object with a leading"type"discriminator (text_delta,tool_call_request,tool_call_result,finish,error).Decodereads the discriminator and dispatches to the right concrete unmarshal.Errorround-trips through JSON as a string-onlyerrors.New(msg)— documented in the type's doc comment as an explicit trade-off for replay.ErrUnknownTypesentinel forerrors.Ischecks on decode failures.core/event/event_test.go(167 lines)TestRoundTripcovering all 5 variants plus three notable cases:tool_call_requestwithServerpopulated,Finishwith zeroUsage,Error{}(nil err).TestDecodeErrorscovering 7 malformed-input shapes.TestErrUnknownTypeconfirming the sentinel is properly chained forerrors.Is.TestSealedInterfaceasserting at compile time that each variant satisfiesEvent.core/event/fuzz_test.go(74 lines)FuzzDecode— round-trip equivalence: a successfully-decoded event must remarshal and re-decode to the same JSON.FuzzDecodeNeverPanics— narrower contract for fast CI cycles.core/provider/provider.go(100 lines)Providerinterface:Stream(ctx, Request) (<-chan event.Event, error),Name() string.Request:Model,Messages,Tools, plus three pointer-typed sampling fields (Temperature,TopP,MaxTokens) sonilmeans "provider default" and an explicit0means "deterministic".Message,ToolCall,Tool,Role(system/user/assistant/tool).Tool.ParametersandToolCall.Argumentsarejson.RawMessage— we don't interpret JSON Schema in this package; downstream (core/router,cmd/ask) does.core/provider/openai_compatible.go(458 lines, 93% coverage)OpenAIConfig:BaseURL,APIKey,APIKeyEnv,Headers, optionalHTTPClient(tests inject ahttptestclient).NewOpenAICompatiblevalidates config, resolvesAPIKeyfromAPIKeyEnvif needed, trims a trailing slash fromBaseURL, and builds the request URL once.ErrConfigsentinel forerrors.Is.http.Clienthas 10s dial / 30s response-header / 90s idle timeouts but no overallTimeout— streaming responses can legitimately run for many minutes.Streambuilds the chat-completions payload, setsAuthorization: Bearer ...,Content-Type: application/json,Accept: text/event-stream, and any custom headers, then dispatches to a goroutine.runStream→handleLine→handleChoice/flushPendingso cognitive complexity stays well inside thegocognit=20lint threshold.Indexacross SSE frames into atoolCallAccumulator. A singleevent.ToolCallRequestper call is emitted only when the runtime reportsfinish_reason: tool_calls. Accumulated args are validated withjson.Validbefore emission; invalid JSON produces anevent.Errorrather than a malformed call.flushPendingsorts by index before emitting. Verified by a test that delivers index 1 before index 0.:prefix) and keep-alive frames are silently dropped; missing or late[DONE]is tolerated; if the stream ends without afinish_reason,runStreamsynthesizes aFinish{Reason: stop}so consumers always see a terminal event.core/provider/openai_compatible_test.go(542 lines)17 sub-tests against
httptest.Server:BaseURLrejected, name, custom headers passed through,BaseURLtrailing slash trimmedt.Setenv)APIKeyoverrides env{}, SSE comments ignored,[DONE]withoutfinish_reason, stream ends without[DONE]event.Error, malformed chunk →event.Error, HTTP 500 → init error, dial failuretool_calls+toolmessage withtool_call_id)core/provider/testing.go(102 lines, 99% coverage)FakeProviderplays a sequence ofFakeScriptentries in order. Each script either returns its events on a buffered channel or returnsInitErrorfromStream.StreamreturnsErrFakeExhausted.Calls()returns a copy of the requests received so far for assertion.WithFakeNameoption lets tests override the provider name.Local quality gate (matches CI exactly)
go test -race -shuffle=on -count=1core/...statement coveragegofmt -l .go vet ./...go mod tidy -diffgo tool golangci-lint run ./...go tool govulncheck ./core/...FuzzDecode15sFuzzDecodeNeverPanics10sNotes for the reviewer
testing.gois intentionally exported. It is not gated behind a build tag. Spec §5 directs us to shipprovider/testing.goas a downstream-importable helper; downstream test packages in phases 4-7 will useprovider.NewFake(...)directly. The unexportedFakeProviderfields keep the implementation private; only theNew/Calls/Stream/Namesurface is exposed.json.RawMessageforTool.ParametersandToolCall.Arguments. Lets us validate JSON without interpreting schema. The router (Phase 5: core/loop + core/router + core/approval #6) and any downstream tooling can interpret the schema however it wants without contorting through this package's types.Usageevent surfaces whatever the runtime reports; this package never counts tokens itself.ErrorJSON round-trip is lossy. A wrapped error chain becomes a singleerrors.New(msg)after decode. Documented in the type's comment. Replay consumers needing the original chain should capture before serialization.runStreamwas deliberately broken into helpers. Inlining the loop would have been simpler to read on a screen, butgocognitwould have flagged it. Helper functions return a smallstreamStatusenum (Continue/Done/Abort) so the outer loop's control flow stays explicit.Test plan
lintjob passes:go mod verify,gofmt,go vet,golangci-lint,golangci-lint config verifyall cleantest (ubuntu-latest)andtest (macos-latest)green;coverage.outuploaded with thecoreCodecov flag and thecore/gate at >80%buildmatrix green acrossdarwin/{amd64,arm64},linux/{amd64,arm64},windows/amd64gosecclean;semgrepclean;govulncheckclean;trivy fscleananalyze go(CodeQL) cleandependency-reviewclean (no new module dependencies introduced)conventional commitPR title check passes (feat(core): ...)ci passaggregator greenmaincan have all source-dependent required status checks enabled (perdocs/ci.md)