glue: WithStreamWriter / WithToolLogger (closes #91) by erain · Pull Request #98 · erain/glue

erain · 2026-05-07T10:54:32Z

Summary

glue.WithStreamWriter(io.Writer) mirrors EventTextDelta.Delta.
glue.WithToolLogger(io.Writer) writes [tool] <name>\n on EventToolStart.
Both nil-safe (nil writer is a no-op) and silently drop writer errors — documented as convenience options, not delivery-guaranteed pipes.
Compose additively with WithEvents and each other via a new promptConfig.auxEmits slice — installing one does not displace any other handler.
README "Streaming events" subsection rewritten to lead with the helpers.

Stacked on #95, #96, #97. Closes #91.

Test plan

go build ./...
go vet ./...
go test . ./loop ./providers/... ./tools/... — all green
Unit tests cover: text-delta mirror, nil-writer no-op, tool-start mirror, additive composition with WithEvents

🤖 Generated with Claude Code

Every Glue agent re-implemented the same per-tool boilerplate: a json.Unmarshal of call.Arguments into a private struct, plus local textResult / errorResult helpers. Promote the pattern into the framework so tool definitions drop ~15 lines each and gain consistent malformed- argument handling. - glue.TextResult(string) and glue.ErrorResult(error) replace the duplicated helpers (the agent had its own copies; future agents would too). - glue.NewTool[T any](spec, fn) decodes ToolCall.Arguments into T, treats empty arguments as the zero value, and returns an error ToolResult on JSON decode failure so the loop never crashes on a malformed call. Migrate agents/glue-review/tools.go as the first internal consumer: three tools converted, the local textResult/errorResult helpers removed. Schema generation from T is intentionally out of scope; callers continue to pass Parameters explicitly. Verification: go build ./... # ok go vet ./... # ok go test ./... # ok (all packages green) Closes #88 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

github-actions

Summary

This branch introduces several framework-level conveniences: typed tool builders (NewTool[T], TextResult, ErrorResult), event stream helpers (WithStreamWriter, WithToolLogger), a provider failover wrapper (WithFailover), a driver-style provider registry, and shared tools/fs and tools/git packages implementing ADR 0003.

Issues

[major] failover.go:49 — failoverProvider.Stream leaks a goroutine and its output channel when it commits to a provider. The out channel is buffered with 16 slots, but the helper goroutine will block forever if the caller abandons the returned channel without draining it (since nobody closes ch). Additionally, if ch is never closed by the underlying provider, this goroutine leaks indefinitely. Also, the out goroutine receives from ch without a ctx check; if the caller cancels the context the goroutine still blocks on <-ch. Fix: In failover.go, change failoverProvider.Stream to select on an inner ctx.Done() in the forwarding goroutine and, more importantly, propagate cancellation from the caller's context to the goroutine so it exits when the context is cancelled. Consider adding a close(out) via a defer and a select { case <-ctx.Done(): ... } in the forwarding loop.
[major] event_helpers.go:36 — WithToolLogger uses fmt.Fprintf(w, "[tool] %s\n", e.ToolName) without sanitizing e.ToolName; if a malicious provider or tool injects newlines or format specifiers into ToolName, the output could be misleading or contain format-string-like behavior. Fix: In event_helpers.go, replace fmt.Fprintf(w, "[tool] %s\n", e.ToolName) with fmt.Fprint(w, "[tool] ", e.ToolName, "\n") or fmt.Fprintf(w, "[tool] %s\n", strings.ReplaceAll(e.ToolName, "%", "%%")).
[critical] session.go:205 — When config.emit (the WithEvents handler) and config.auxEmits (the helpers) are both present, dispatchEvent is called via s.dispatchEvent(event) after both emit blocks, but config.emit also runs before auxEmits. If config.emit panics, auxEmits and dispatchEvent are skipped. More importantly, WithEvents documented "last-wins" semantics for its own field, but auxEmits appends, so the ordering between a single emit and multiple auxEmits is now mixed without clear precedence. This is a subtle API contract issue. Fix: Document in event_helpers.go that WithStreamWriter/WithToolLogger fire after WithEvents but before dispatchEvent. Better yet, unify the pipelines in session.go so emit, auxEmits, and dispatchEvent are all called in a single deterministic order, with each wrapped in a defer/recover to prevent one panicking handler from killing the rest.
[major] tools/fs/read.go:85 — ReadFileTool uses cap as a variable name, which shadows Go's built-in cap function. While legal, it is poor practice and can confuse readers and linters. Fix: In tools/fs/read.go, rename the local variable cap to max or limit.
[major] tools/git/diff.go:83 — Same cap shadowing issue in DiffBranchTool. Fix: In tools/git/diff.go, rename the local variable cap to max or limit.

Suggestions

[minor] failover.go:16 — The comment says Returns a typed error implementing [FailoverError] when all providers fail, but FailoverError is a concrete struct, not an interface, so "implementing" is slightly misleading. Fix: In failover.go, reword the doc comment to Returns an error of type *FailoverError when all providers fail; use errors.As to inspect per-provider attempts.
[minor] providers/registry.go:38 — Factory.New returns loop.Provider but the name New is ambiguous; the registry is provider-scoped so it's clear, but a more descriptive name would help. This is fine as-is, but consider Create in a future refactor. No fix required.
[minor] tools/fs/blocklist.go:111 — Match does three separate filepath.Match calls per pattern. For large blocklists or deeply nested paths this is O(n * 3) with repeated string allocations for lowPat. In practice blocklists are small, but consider caching lowered patterns. Not actionable here.
[minor] tools/fs/fs.go:39 — SafeJoin calls filepath.Abs(base), which does I/O (Getwd) on every call. For high-volume tool calls this could be a bottleneck. Consider accepting an already-absoluted base path in ReadFileOptions or caching the resolved base. Fix: In tools/fs/fs.go, document that base should ideally be an absolute path to avoid repeated Getwd calls, or add a small cache in ReadFileTool that resolves WorkDir once at construction time.
[minor] event_helpers_test.go:109 — The TestStreamHelpers_ComposeWithWithEvents test asserts evCount == 0 as a failure condition, but it doesn't assert a specific expected count, which could mask under/over-firing. Fix: In event_helpers_test.go, assert evCount equals the expected number of events (e.g., 4) instead of just > 0.
[minor] tool_helpers.go:38 — NewTool hardcodes the error message format decode arguments for tool %q: %w. If the tool name is long or contains special characters, this is fine, but the error is not prefixed with the package name (e.g., glue: ). The rest of the package uses glue: prefixes in some places but not in tool errors. This is inconsistent but not harmful. No fix required.
[minor] providers/registry.go:47 — Register overwrites silently. This is documented as "last importer wins", but in practice it means two providers accidentally registering under the same name will silently hide one. Consider logging or panicking on overwrite during testing. Not actionable in this PR.

Looks good

The typed NewTool[T] helper removes significant boilerplate from agent tool definitions while keeping JSON schema generation out of scope — a clean, pragmatic boundary.
WithFailover correctly preserves the "commit after first non-error event" invariant, which is the right semantic for LLM streaming.
The tools/fs and tools/git packages cleanly separate POSIX concerns from the core glue package, matching ADR 0003 exactly.
Comprehensive test coverage for all new public APIs, including scratch-repo end-to-end tests for the git tools.
The provider registry uses cheap factory registration in init(), avoiding I/O side effects at import time.

Open questions

For WithFailover, is there a plan to expose metrics or hooks (e.g., OnAttempt) so callers can log which provider was selected? It would be useful for debugging but is not blocking.
Should ReadFileTool support binary files with a base64 escape hatch, or is UTF-8-only the intentional permanent boundary?
The agents/glue-review/tools.go still contains the old inline implementations (now wrapped in NewTool). Is the migration to tools/fs and tools/git packages tracked for a follow-up, or should this PR also update agents/glue-review to use the new shared packages? The latter seems like a natural cleanup to include.

🤖 Posted by glue-review.

github-actions · 2026-05-07T10:56:56Z

+
+// FailoverAttempt is one provider's result inside a [FailoverError].
+type FailoverAttempt struct {
+	Index int


[major] failoverProvider.Stream leaks a goroutine and its output channel when it commits to a provider. The out channel is buffered with 16 slots, but the helper goroutine will block forever if the caller abandons the returned channel without draining it (since nobody closes ch). Additionally, if ch is never closed by the underlying provider, this goroutine leaks indefinitely. Also, the out goroutine receives from ch without a ctx check; if the caller cancels the context the goroutine still blocks on <-ch

💡 AI prompt to fix

In `failover.go`, change `failoverProvider.Stream` to select on an inner `ctx.Done()` in the forwarding goroutine and, more importantly, propagate cancellation from the caller's context to the goroutine so it exits when the context is cancelled. Consider adding a `close(out)` via a `defer` and a `select { case <-ctx.Done(): ... }` in the forwarding loop.

github-actions · 2026-05-07T10:56:56Z

+// "[tool] <name>\n". Composes additively with other event-related
+// options. Errors from the writer are silently dropped; nil w is a
+// no-op.
+func WithToolLogger(w io.Writer) PromptOption {


[major] WithToolLogger uses fmt.Fprintf(w, "[tool] %s\n", e.ToolName) without sanitizing e.ToolName; if a malicious provider or tool injects newlines or format specifiers into ToolName, the output could be misleading or contain format-string-like behavior

💡 AI prompt to fix

In `event_helpers.go`, replace `fmt.Fprintf(w, "[tool] %s\n", e.ToolName)` with `fmt.Fprint(w, "[tool] ", e.ToolName, "\n")` or `fmt.Fprintf(w, "[tool] %s\n", strings.ReplaceAll(e.ToolName, "%", "%%"))`.

github-actions · 2026-05-07T10:56:56Z

@@ -204,6 +205,9 @@ func (s *Session) Prompt(ctx context.Context, text string, options ...PromptOpti
 			if config.emit != nil {


[critical] When config.emit (the WithEvents handler) and config.auxEmits (the helpers) are both present, dispatchEvent is called via s.dispatchEvent(event) after both emit blocks, but config.emit also runs before auxEmits. If config.emit panics, auxEmits and dispatchEvent are skipped. More importantly, WithEvents documented "last-wins" semantics for its own field, but auxEmits appends, so the ordering between a single emit and multiple auxEmits is now mixed without clear precedence. This is a subtle API contract issue

💡 AI prompt to fix

Document in `event_helpers.go` that `WithStreamWriter`/`WithToolLogger` fire *after* `WithEvents` but before `dispatchEvent`. Better yet, unify the pipelines in `session.go` so `emit`, `auxEmits`, and `dispatchEvent` are all called in a single deterministic order, with each wrapped in a defer/recover to prevent one panicking handler from killing the rest.

github-actions · 2026-05-07T10:56:56Z

+				return glue.ErrorResult(err), nil
+			}
+			return glue.TextResult(Truncate(string(data), cap)), nil
+		},


[major] ReadFileTool uses cap as a variable name, which shadows Go's built-in cap function. While legal, it is poor practice and can confuse readers and linters

💡 AI prompt to fix

In `tools/fs/read.go`, rename the local variable `cap` to `max` or `limit`.

github-actions · 2026-05-07T10:56:56Z

+			gitArgs := []string{"diff", "--no-color", base + "...HEAD"}
+			if len(opts.Pathspec) > 0 {
+				gitArgs = append(gitArgs, "--")
+				gitArgs = append(gitArgs, opts.Pathspec...)


[major] Same cap shadowing issue in DiffBranchTool

💡 AI prompt to fix

In `tools/git/diff.go`, rename the local variable `cap` to `max` or `limit`.

github-actions · 2026-05-07T10:56:56Z

+	EnvKey string
+}
+
+var (


[minor] Factory.New returns loop.Provider but the name New is ambiguous; the registry is provider-scoped so it's clear, but a more descriptive name would help. This is fine as-is, but consider Create in a future refactor. No fix required.

github-actions · 2026-05-07T10:56:56Z

+func (b Blocklist) Match(rel string) (bool, string) {
+	clean := strings.TrimSpace(rel)
+	if clean == "" {
+		return false, ""


[minor] Match does three separate filepath.Match calls per pattern. For large blocklists or deeply nested paths this is O(n * 3) with repeated string allocations for lowPat. In practice blocklists are small, but consider caching lowered patterns. Not actionable here.

github-actions · 2026-05-07T10:56:56Z

+	}
+	candidate := filepath.Clean(filepath.Join(absBase, rel))
+	rel2, err := filepath.Rel(absBase, candidate)
+	if err != nil {


[minor] SafeJoin calls filepath.Abs(base), which does I/O (Getwd) on every call. For high-volume tool calls this could be a bottleneck. Consider accepting an already-absoluted base path in ReadFileOptions or caching the resolved base

💡 AI prompt to fix

In `tools/fs/fs.go`, document that `base` should ideally be an absolute path to avoid repeated `Getwd` calls, or add a small cache in `ReadFileTool` that resolves `WorkDir` once at construction time.

github-actions · 2026-05-07T10:56:56Z

+func NewTool[T any](spec ToolSpec, fn func(ctx context.Context, args T) (ToolResult, error)) Tool {
+	return Tool{
+		ToolSpec: spec,
+		Execute: func(ctx context.Context, call ToolCall) (ToolResult, error) {


[minor] NewTool hardcodes the error message format decode arguments for tool %q: %w. If the tool name is long or contains special characters, this is fine, but the error is not prefixed with the package name (e.g., glue: ). The rest of the package uses glue: prefixes in some places but not in tool errors. This is inconsistent but not harmful. No fix required.

github-actions · 2026-05-07T10:56:56Z

+// name overwrites — driver-style packages may register from init() and
+// the last importer wins.
+func Register(name string, f Factory) {
+	mu.Lock()


[minor] Register overwrites silently. This is documented as "last importer wins", but in practice it means two providers accidentally registering under the same name will silently hide one. Consider logging or panicking on overwrite during testing. Not actionable in this PR.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b4bd143867

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-05-07T10:57:24Z

+			continue
+		}
+
+		first, ok := <-ch


Honor context cancellation before first provider event

The failover wrapper blocks on first, ok := <-ch without watching ctx.Done(), so if a provider returns a stream channel but never emits its first event (for example, a hung backend connection), the call cannot be canceled and never advances to fallback providers. In this state, Session.Prompt can hang indefinitely even when the caller’s context deadline expires.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-05-07T10:57:24Z

+	candidate := filepath.Clean(filepath.Join(absBase, rel))
+	rel2, err := filepath.Rel(absBase, candidate)
+	if err != nil {
+		return "", err
+	}
+	if strings.HasPrefix(rel2, "..") || rel2 == ".." {
+		return "", fmt.Errorf("path %q escapes work directory", rel)


Resolve symlinks before accepting joined file paths

SafeJoin only performs a lexical filepath.Rel check on the joined path and never resolves symlinks, so a path like repo/linked (where linked points outside base) is accepted and later opened by ReadFileTool. This bypasses the workspace boundary and allows reading files outside the intended directory despite the function contract claiming symlink-safe behavior.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-05-07T10:57:24Z

+		max = DefaultReadMaxBytes
+	}
+	workDir := opts.WorkDir
+	blocklist := opts.Blocklist


Default ReadFileTool to built-in sensitive-file blocklist

The constructor copies opts.Blocklist directly, so the zero-value options path leaves blocklist empty and blocklist.Match never rejects anything. That makes read_file able to return .env, key material, and credential files unless every caller remembers to pass fs.Default(), which contradicts the tool’s own description that it refuses secret-shaped files.

Useful? React with 👍 / 👎.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Promote the path-safety / git-shell-out helpers that agents/glue-review inlined into shared, audited extension packages. ADR 0003 was approved months ago but no implementation landed; the agent's tools.go and blocklist.go are empirical proof of the API shape, so port them. tools/fs: - SafeJoin(base, rel) — traversal + symlink defense - Truncate(s, max) — output cap with marker - Blocklist (Default/Merge/Match) — three-way pattern match (whole-path, basename, components, case-insensitive) - ReadFileTool(opts) — ready-to-register read tool composed of the above tools/git: - RunGit(ctx, opts, args...) — PATH lookup, configurable timeout (default 15s), stderr captured into the error - BuildPathspec(includes, excludes) — Git pathspec construction with the catch-all-include rule - DiffBranchTool(opts) — git_diff_branch tool - LogBranchTool(opts) — git_log_branch tool Both packages live outside the core glue package per ADR 0003 so the harness stays free of POSIX coupling. They depend on glue (for Tool / ToolSpec / ToolResult and the new NewTool[T] helper from #88); the core does not depend on them. ADR 0003 marked Implemented; design.md package boundaries updated. Migrating agents/glue-review to use these packages is filed as a follow-up so this PR stays single-purpose and reviewable. Verification: go build ./... # ok go vet ./... # ok go test ./... # ok go test ./tools/fs ./tools/git # ok (table tests + scratch-repo end-to-end) Closes #89 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Stop forcing agents to hand-code provider failover. Today an agent that wants nvidia → openrouter → gemini failover maintains a private map of provider → env var, hand-codes per-attempt session ids to avoid transcript poisoning, and buffers streamed text per attempt. Move the boring parts into the framework. New driver-style registry at providers/registry.go: - providers.Register(name, Factory) — providers self-register in init() - providers.New(name) → (Provider, defaultModel, envKey, error) - providers.Lookup(name), providers.Known() (sorted), providers.KeyAvailable(name) - Case-insensitive lookup; unknown name returns an error listing the registered names Each shipped provider exposes EnvKey + DefaultModel constants and self-registers from init() (gemini, nvidia, openrouter). Importing `_ "github.com/erain/glue/providers/<name>"` makes the name resolvable. WithFailover wrapper in package glue: - glue.WithFailover(provs ...Provider) Provider - Tries each provider in order; falls through on Stream error, immediate ProviderEventError, or empty stream - Commits to a provider once any non-error event is observed — preserves the loop's "no half-streamed transcripts on retry" invariant, no mid-stream recovery - All-providers-failed surfaces as *FailoverError with per-provider attempts (errors.As compatible) README: new "Provider failover" subsection. design.md: provider registry listed under package boundaries. Migrating agents/glue-review off its hand-coded failover is filed as a follow-up so this PR stays single-purpose. Verification: go build ./... # ok go vet ./... # ok go test . ./loop ./providers/... ./tools/... # ok (all green) go test ./agents/glue-review # one live test # rate-limited # by NVIDIA (HTTP 429), # not a regression Closes #90 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Replace the canonical "switch e.Type { case EventTextDelta: ... }" block that every CLI agent writes with two one-line options. - glue.WithStreamWriter(io.Writer) mirrors EventTextDelta.Delta. - glue.WithToolLogger(io.Writer) writes "[tool] <name>\n" on EventToolStart. - Both nil-safe (nil writer = no-op so callers can pass conditional writers without branching) and silently drop writer errors — documented as convenience options, not delivery-guaranteed pipes. - Compose additively with WithEvents and each other via a new promptConfig.auxEmits slice — installing one does not displace any other handler. WithEvents continues to use the existing single-emit field with last-wins semantics. README "Streaming events" subsection rewritten to lead with the helpers; the WithEvents path is kept as the richer-formatting fallback. Verification: go build ./... # ok go vet ./... # ok go test . ./loop ./providers/... ./tools/... # ok (all green) go test . -run "Stream|ToolLogger|StreamHelpers" # ok Closes #91 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

erain mentioned this pull request May 7, 2026

prompts: PromptCatalog versioned-prompt loader (closes #92) #99

Merged

3 tasks

github-actions Bot reviewed May 7, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed May 7, 2026

View reviewed changes

erain and others added 5 commits May 7, 2026 10:13

test: replace handrolled intStr with strconv.Itoa

84a5af6

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

tools/{fs,git}: rename cap variable to limit (avoid builtin shadowing)

3422565

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

erain force-pushed the issue/91-stream-writer branch from b4bd143 to 5e95615 Compare May 7, 2026 14:15

erain merged commit 7d93aaf into main May 7, 2026
3 of 4 checks passed

erain deleted the issue/91-stream-writer branch May 7, 2026 14:17

		@@ -204,6 +205,9 @@ func (s *Session) Prompt(ctx context.Context, text string, options ...PromptOpti
		if config.emit != nil {

Conversation

erain commented May 7, 2026

Summary

Test plan

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Summary

Issues

Suggestions

Looks good

Open questions

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant