docs: refresh README, design.md, project-plan.md for the 0.x surface#102
docs: refresh README, design.md, project-plan.md for the 0.x surface#102
Conversation
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>
There was a problem hiding this comment.
Summary
This is a large documentation-and-feature branch that lands multiple 0.x agent-ergonomics improvements: typed NewTool[T] + TextResult/ErrorResult helpers, provider registry + WithFailover, event stream helpers (WithStreamWriter, WithToolLogger), StopReasonMaxTurns, a versioned-prompt catalog (prompts.NewCatalog), standard CLI flags (cli.RegisterStandardFlags), and shared extension packages for filesystem and git tools (tools/fs, tools/git). It also refreshes all project docs (README, design.md, project-plan.md, ADRs) to reflect the shipped surface.
Issues
-
[major] failover.go:94 — Potential goroutine leak: the goroutine forwarding
chtooutinfailoverProvider.Streamwill block forever ifoutis never fully consumed by the consumer (e.g. consumer exits early). The goroutine holds a reference to the source channelchand never has a cancellation signal. Fix: wrap the forwarding goroutine with aselectonctx.Done()and close bothoutand drainchon cancellation. -
[major] event_helpers.go:10 —
WithStreamWriterwraps anio.Writerwith no synchronization. Since events may be delivered concurrently from the loop (or from multiple goroutines downstream), writing to the sameio.Writerfrom multiple goroutines without synchronization can cause interleaved output or data races. Fix: document that the caller must provide a synchronizedio.Writer(e.g.os.Stdoutwhich is already OS-synchronized), or wrap the writes with async.MutexinsideWithStreamWriterandWithToolLogger. -
[major] event_helpers.go:45 —
WithToolLoggerhas the same unsynchronizedio.Writeraccess issue asWithStreamWriter. Fix: apply the same fix as forWithStreamWriter— either document the synchronization requirement or add internal locking. -
[minor] loop/run.go:141 — The
lastAssistantMsgandlastAssistantNewindices are tracked across the loop, but ifmaxTurns == 0(meaning the loop body never executes), these remain-1and the budget-exhaustion tagging is skipped. While this is a degenerate case, it leaves callers withoutStopReasonMaxTurnswhen they might expect it. Fix: add an explicit check before the loop: ifmaxTurns <= 0, return immediately with a clear error, or document thatMaxTurns <= 0is invalid. The current code returnsfail(fmt.Errorf("loop: maximum turns exceeded (%d)", maxTurns))without tagging because the loop never enters.
Suggestions
-
[minor] failover.go:62 —
failoverProvider.Streamcreates a goroutine per successful provider, but there's no way to cancel that goroutine from outside. If a caller callsStreambut never reads from the returned channel, the goroutine leaks indefinitely. Thecontextparameter isn't used for cancellation of the goroutine. Fix: change the forwarding loop toselectonctx.Done()andch, closingoutand drainingchwhen context is cancelled. -
[minor] prompts/catalog.go:90 —
Getperformsstrings.TrimRight(string(data), "\n"), which will trim all trailing newlines. If a prompt file legitimately ends with multiple blank lines for formatting, they are all collapsed. The doc comment says "trimmed of trailing newlines and re-suffixed with a single newline" — consider trimming only a single trailing newline, or documenting that multiple trailing newlines are intentionally collapsed. Fix: in prompts/catalog.go, changestrings.TrimRight(string(data), "\n")tostrings.TrimSuffix(string(data), "\n")to remove only the final newline, preserving intermediate blank lines. -
[minor] tools/fs/read_test.go:44 —
TestReadFileTool_ReadsAndTruncatesusesMaxBytes: 50but then assertslen(got) != 50. This is testing the implementation ofTruncateviaReadFileTool, butTruncatealready has dedicated tests infs_test.go. This makes the read test brittle ifTruncate's marker length changes. Fix: keep the test focused on "tool returns non-error result with truncation marker" rather than asserting exact length; remove thelen(got) != 50assertion or replace it with a looser check. -
[minor] providers/registry.go:19 —
factoriesis a package-levelmapprotected by async.RWMutex, butRegisteris meant to be called frominit()functions. In Go,init()order within a package is deterministic but across packages it is not. If two provider packages are imported in the same binary, theirinit()calls race onRegister. Thesync.RWMutexprotects against concurrent goroutine access but does not prevent issues ifRegisterreads the map while anotherinit()writes — though Go's init ordering makes this unlikely in practice, it's still a latent issue if registration ever moves outsideinit(). Fix: no code change needed, but add a comment in providers/registry.go nearvar factoriesdocumenting thatRegisteris designed forinvoked from init() onlyto set clear expectations. -
[minor] agents/glue-review/tools.go:72 — The agent's
readFileToolno longer uses thetools/fspackage'sReadFileTooldespite it being available in this branch. The commit message for b1e1397 says "Migrating agents/glue-review to use these packages is filed as a follow-up" — this is fine, but the agent'sreadFileToolstill manually implements whatfs.ReadFileTooldoes, including its own blocklist and safe join. Fix: add a TODO comment in agents/glue-review/tools.go above the hand-rolled tool definitions referencing the follow-up migration totools/fsandtools/git.
Looks good
- Nice use of
go test ./...andgo vet ./...verification for every commit — this is a good discipline. - The typed
NewTool[T]helper eliminates exactly the right amount of boilerplate without overreaching into schema generation. WithFailover's commit-after-first-event semantics are well-documented and preserve the "no half-streamed transcripts" invariant correctly.- The
providersregistry design (factory functions, not instantiated providers) keeps registration cheap and side-effect-free. tools/fsandtools/gitbeing split out into extension packages per ADR 0003 is a clean boundary.
Open questions
- Do any of the new event helpers (
WithStreamWriter,WithToolLogger) need to be registered as session-scoped subscribers viaSession.Subscribe, or are they strictly per-prompt? The current implementation adds them topromptConfig.auxEmitswhich is per-prompt only — this seems intentional but worth confirming. - Is there a plan for how the
agents/glue-reviewmigration totools/fsandtools/gitwill be tracked? The commit messages mention follow-ups but there's no linked issue in the branch.
🤖 Posted by glue-review.
| out <- first | ||
| for ev := range ch { | ||
| out <- ev | ||
| } |
There was a problem hiding this comment.
[major] Potential goroutine leak: the goroutine forwarding ch to out in failoverProvider.Stream will block forever if out is never fully consumed by the consumer (e.g. consumer exits early). The goroutine holds a reference to the source channel ch and never has a cancellation signal
💡 AI prompt to fix
wrap the forwarding goroutine with a `select` on `ctx.Done()` and close both `out` and drain `ch` on cancellation.
|
|
||
| // WithStreamWriter mirrors EventTextDelta.Delta to w on every prompt | ||
| // event. Composes additively with WithEvents and other event-related | ||
| // options — installing one does not displace any other. |
There was a problem hiding this comment.
[major] WithStreamWriter wraps an io.Writer with no synchronization. Since events may be delivered concurrently from the loop (or from multiple goroutines downstream), writing to the same io.Writer from multiple goroutines without synchronization can cause interleaved output or data races
💡 AI prompt to fix
document that the caller must provide a synchronized `io.Writer` (e.g. `os.Stdout` which is already OS-synchronized), or wrap the writes with a `sync.Mutex` inside `WithStreamWriter` and `WithToolLogger`.
| if e.Type == EventToolStart && e.ToolName != "" { | ||
| _, _ = fmt.Fprintf(w, "[tool] %s\n", e.ToolName) | ||
| } | ||
| }) |
There was a problem hiding this comment.
[major] WithToolLogger has the same unsynchronized io.Writer access issue as WithStreamWriter
💡 AI prompt to fix
apply the same fix as for `WithStreamWriter` — either document the synchronization requirement or add internal locking.
| } | ||
|
|
||
| func (f failoverProvider) Stream(ctx context.Context, req loop.ProviderRequest) (<-chan loop.ProviderEvent, error) { | ||
| if len(f.provs) == 0 { |
There was a problem hiding this comment.
[minor] failoverProvider.Stream creates a goroutine per successful provider, but there's no way to cancel that goroutine from outside. If a caller calls Stream but never reads from the returned channel, the goroutine leaks indefinitely. The context parameter isn't used for cancellation of the goroutine
💡 AI prompt to fix
change the forwarding loop to `select` on `ctx.Done()` and `ch`, closing `out` and draining `ch` when context is cancelled.
| } | ||
|
|
||
| // Default returns the default version supplied to NewCatalog. | ||
| func (c *Catalog) Default() string { |
There was a problem hiding this comment.
[minor] Get performs strings.TrimRight(string(data), "\n"), which will trim all trailing newlines. If a prompt file legitimately ends with multiple blank lines for formatting, they are all collapsed. The doc comment says "trimmed of trailing newlines and re-suffixed with a single newline" — consider trimming only a single trailing newline, or documenting that multiple trailing newlines are intentionally collapsed
💡 AI prompt to fix
in prompts/catalog.go, change `strings.TrimRight(string(data), "\n")` to `strings.TrimSuffix(string(data), "\n")` to remove only the final newline, preserving intermediate blank lines.
| dir := t.TempDir() | ||
| if err := os.WriteFile(filepath.Join(dir, ".env"), []byte("S=1"), 0o644); err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
[minor] TestReadFileTool_ReadsAndTruncates uses MaxBytes: 50 but then asserts len(got) != 50. This is testing the implementation of Truncate via ReadFileTool, but Truncate already has dedicated tests in fs_test.go. This makes the read test brittle if Truncate's marker length changes
💡 AI prompt to fix
keep the test focused on "tool returns non-error result with truncation marker" rather than asserting exact length; remove the `len(got) != 50` assertion or replace it with a looser check.
| "strings" | ||
| "sync" | ||
|
|
||
| "github.com/erain/glue/loop" |
There was a problem hiding this comment.
[minor] factories is a package-level map protected by a sync.RWMutex, but Register is meant to be called from init() functions. In Go, init() order within a package is deterministic but across packages it is not. If two provider packages are imported in the same binary, their init() calls race on Register. The sync.RWMutex protects against concurrent goroutine access but does not prevent issues if Register reads the map while another init() writes — though Go's init ordering makes this unlikely in practice, it's still a latent issue if registration ever moves outside init()
💡 AI prompt to fix
no code change needed, but add a comment in providers/registry.go near `var factories` documenting that `Register` is designed for `invoked from init() only` to set clear expectations.
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>
Promote agents/glue-review/prompt.go into a small reusable framework helper. Versioning prompts behind an embed.FS is a recurring pattern (A/B test prompts, roll back without rebuild) and the agent's implementation already encodes the right error semantics — unknown version → list available, never silent fallback. New package github.com/erain/glue/prompts: - NewCatalog(fsys fs.FS, dir, defaultVersion) (*Catalog, error) validates that defaultVersion exists at construction time so misconfiguration fails fast. - Get(version) (string, error) — empty version uses the default; unknown returns an error listing every available version. - Versions() (sorted), Default() - Nil-safe getters (return nil/"" / typed error) Returned bodies are right-trimmed of newlines and re-suffixed with a single newline so callers can append further instructions without double-blank-line drift — matches the agent's existing format. Templating / variable substitution remain out of scope: the catalog returns raw bytes; rendering is the caller's job. README: new "Versioned prompts" subsection. Migration of agents/glue-review/prompt.go onto this catalog filed as a follow-up. Verification: go build ./... # ok go vet ./... # ok go test ./prompts -count=1 # ok (8/8) Closes #92 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Today loop.Run errors on max-turns and the partial transcript's last
assistant message carries StopReasonToolUse — indistinguishable from a
turn that legitimately ended on tools. An agent that wants to retry
with a higher budget can't tell "we ran out of turns" apart from
"model errored mid-stream".
Add StopReasonMaxTurns. When the loop exits via the budget guard,
mark the last assistant message in both Messages and NewMessages with
this stop reason before returning the (still-non-nil) error. The
transcript shape is unchanged; only the StopReason field on one
message gains a new value.
- New const loop.StopReasonMaxTurns ("max_turns"), re-exported as
glue.StopReasonMaxTurns
- loop.Run tracks the index of the last appended assistant message
and tags it on max-turns exit
- design.md "Agent Loop" section documents the new outcome
Verification:
go build ./... # ok
go vet ./... # ok
go test ./loop -count=1 -run "MaxTurns|NaturalStop" # ok
go test . ./loop ./prompts ./providers/... ./tools/... # ok
Closes #93
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stop having every multi-provider Glue agent re-register the same six flags. New thin package github.com/erain/glue/cli wires --provider, --model, --id, --store, --work, --max-turns onto a caller-supplied flag.FlagSet and returns a typed getter. - cli.StandardConfig — struct of the six common values - cli.StandardFlagDefaults — exported defaults so callers can override one or two without re-registering everything - cli.RegisterStandardFlags(fs, defaults) func() StandardConfig Help text mentions failover semantics for --provider so agents do not need to redocument them. The package is intentionally thin; cmd/glue remains the canonical CLI runner — this is shared infrastructure for downstream agent binaries. README "CLI" section gains a "Standard flags for downstream agents" subsection with usage example. Verification: go build ./... # ok go vet ./... # ok go test ./cli -count=1 # ok (3/3 — defaults, argv parse, defaults override) Closes #94 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The project-level docs were last truthful when the harness shipped P0. Since then P1 + P2 + a sizeable agent-ergonomics wishlist (#88–#94) all landed, and the docs still claimed "P0 is complete" with a Roadmap that treated P2 features as upcoming work. Refresh in place: README.md: - Tagline now lists all three providers (Gemini + NVIDIA + OpenRouter) - Status section rewritten to enumerate what actually shipped: typed NewTool, WithFailover, WithStreamWriter/WithToolLogger, StopReasonMaxTurns, prompts.NewCatalog, cli.RegisterStandardFlags, tools/fs + tools/git - Subpackage list includes openaicompat / providers registry / tools/ / prompts / cli - File-backed sessions wording updated (no longer "lands in P1") - agents/glue-review reference bumped to v1.1.0 with the AI-fix-prompt detail - Roadmap rewritten — P0–P2 shipped, current focus pointed at flue-gap-analysis.md docs/design.md: - Non-Goals retitled (was "For P0/P1") and rewritten so shipped features (parallel exec, compaction, tools/fs, tools/git) read as shipped-but-opt-in rather than future - Broken `compactor.go` relative link fixed (`../compactor.go`) docs/project-plan.md: - Current Status replaced with an accurate P0/P1/P2 ledger plus a "beyond plan" subsection for the agent-ergonomics work and extra providers. Next-focus paragraph points at the agent migration follow-ups and the Flue gap analysis. Verification: go build ./... # ok go vet ./... # ok (docs-only changes; no runtime behavior touched) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d51a584 to
f8b7d35
Compare
Summary
The project-level docs were last truthful when the harness shipped P0. Since then P1 + P2 + the agent-ergonomics wishlist (#88–#94) all landed, but the docs still claimed "P0 is complete" with a Roadmap that treated P2 features as upcoming work. Refresh in place.
NewTool,WithFailover,WithStreamWriter/WithToolLogger,StopReasonMaxTurns,prompts.NewCatalog,cli.RegisterStandardFlags,tools/fs+tools/git); subpackage list includes the new packages;agents/glue-reviewreference bumped tov1.1.0; Roadmap rewritten — P0–P2 shipped, current focus pointed atdocs/flue-gap-analysis.md.compactor.golink fixed (../compactor.go).Stacked on #95–#101 (the seven issue PRs). Docs-only — no runtime behavior touched.
Test plan
go build ./...go vet ./...🤖 Generated with Claude Code