prompts: PromptCatalog versioned-prompt loader (closes #92) by erain · Pull Request #99 · erain/glue

erain · 2026-05-07T10:55:46Z

Summary

New package github.com/erain/glue/prompts with Catalog (NewCatalog, Get, Versions, Default).
Mirrors the agents/glue-review/prompt.go pattern: A/B-test prompts via embed.FS, roll back without rebuilding history, and reject unknown versions with an error that lists every available version (no silent fallback).
NewCatalog validates defaultVersion exists at construction time so misconfiguration fails fast.
Templating/variable substitution intentionally out of scope; rendering is the caller's job.
README "Versioned prompts" subsection added. Migration of agents/glue-review/prompt.go onto this catalog filed as a follow-up.

Stacked on #95–#98. Closes #92.

Test plan

go build ./...
go vet ./...
go test ./prompts -count=1 (8/8 passing — known/empty/unknown lookups, default-must-exist, missing-dir, Versions/Default getters, nil-catalog safety)

🤖 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 framework-level convenience helpers (typed tools, event writer/log helpers, provider failover registry, versioned prompt catalog, and fs/git tool packages), while converting the internal review agent to consume them.

Issues

[critical] failover.go:60 — The goroutine reading first and forwarding ch does not propagate ctx cancellation; if the consumer stops reading out, the goroutine leaks and blocks on out<-. Fix: In failover.go, change the forwarding goroutine to select on ctx.Done() in addition to range ch, and exit early if the context is cancelled.
[critical] prompts/catalog.go:73 — NewCatalog doesn't defend against paths with interior .. that don't split on /; SafeJoin is used in ReadFileTool but not in the catalog itself, so a caller could request version "../etc/passwd" which would be joined into the fs path traversal. Fix: In prompts/catalog.go, validate version in Get by rejecting any string containing .. or path separators before constructing the file path.
[major] failover.go:55 — failoverProvider stores the passed slice directly without copying; callers who mutate the slice after construction would affect the provider's behavior. Fix: In failover.go, copy the provs slice into a new slice in WithFailover: provs: append([]Provider(nil), provs...).
[major] providers/registry.go:84-91 — KeyAvailable uses os.Getenv which observes the current process environment without any isolation; this makes it impossible to unit test without mutating global state. The t.Setenv in tests works, but the design comment says "no I/O or credential side effects," yet os.Getenv is a read side-effect. Fix: In providers/registry.go, add a Getenv func(string) string field to Factory (defaulting to os.Getenv) so that tests can inject a mock environment without touching process globals.

Suggestions

[minor] session.go:208-210 — auxEmits runs after config.emit, so if an auxEmit panics, the user-provided WithEvents handler hasn't fired yet. This is subtle but documented; consider reversing order or documenting it explicitly. Fix: In session.go, add a comment above the emission loop documenting the exact ordering: config.emit fires before any auxEmits.
[minor] event_helpers.go:23,39 — The helpers silently ignore errors from io.WriteString and fmt.Fprintf. This is documented, but it means a partial write could go unnoticed. For debuggability, consider allowing an optional error logger. Fix: No code change needed; add a design note in the package doc comment explaining why errors are intentionally dropped (convenience over reliability) and pointing to WithEvents for error-handling needs.
[minor] tool_helpers.go:37-43 — NewTool unmarshals into var args T even when call.Arguments is nil or []byte(nil), which works (zero value), but it would be more explicit to skip unmarshal when len(call.Arguments) == 0 and just pass the zero value directly. The current code already does this. Fix: No change needed.
[minor] tools/fs/blocklist.go:128-132 — The Match function lowercases the pattern for every path component check, but filepath.Match is case-sensitive on case-sensitive filesystems. This means a blocklist entry like Id_Rsa would not match id_rsa on Linux, even though the design says "case-insensitive." Fix: In tools/fs/blocklist.go, add strings.ToLower(clean) and strings.ToLower(base) at the top of Match so the whole-path and basename checks are case-insensitive as documented, or clarify in the doc comment that only per-component matching is case-insensitive.

Looks good

Excellent test coverage across all new packages with table-driven tests and edge cases (nil catalog, empty streams, malformed JSON args, blocklist hits/misses, safe join traversals).
The NewTool[T] generic helper cleanly eliminates the json.Unmarshal boilerplate and provides consistent malformed-argument handling.
The provider registry with init() self-registration is idiomatic Go and keeps the core package decoupled from provider-specific imports.
Versioned prompt catalog fails fast on construction with clear error messages listing available versions.
Clear separation between core glue package and POSIX-coupled tools (fs/git) per ADR 0003.

Open questions

Is the failoverProvider goroutine leak a concern in practice, given the typical consumer will read the stream to completion? Should there be an explicit Close() or Stop() method on Provider for lifecycle management?
The KeyAvailable tests use t.Setenv which serializes all tests in the package. Is this a concern for CI performance, or is the registry small enough that it doesn't matter?
Are there plans to migrate agents/glue-review/prompt.go to use prompts.NewCatalog, or is that intentionally left as a follow-up? The branch updates the README but doesn't touch the agent's prompt loading code.

🤖 Posted by glue-review.

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

+	}
+	return "all providers failed: " + strings.Join(parts, "; ")
+}
+


[critical] The goroutine reading first and forwarding ch does not propagate ctx cancellation; if the consumer stops reading out, the goroutine leaks and blocks on out<-

💡 AI prompt to fix

In failover.go, change the forwarding goroutine to select on `ctx.Done()` in addition to `range ch`, and exit early if the context is cancelled.

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

+		version = c.defaultVersion
+	}
+	data, err := fs.ReadFile(c.fsys, path.Join(c.dir, version+".md"))
+	if err != nil {


[critical] NewCatalog doesn't defend against paths with interior .. that don't split on /; SafeJoin is used in ReadFileTool but not in the catalog itself, so a caller could request version "../etc/passwd" which would be joined into the fs path traversal

💡 AI prompt to fix

In prompts/catalog.go, validate `version` in `Get` by rejecting any string containing `..` or path separators before constructing the file path.

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

+
+func (e *FailoverError) Error() string {
+	parts := make([]string, 0, len(e.Attempts))
+	for _, a := range e.Attempts {


[major] failoverProvider stores the passed slice directly without copying; callers who mutate the slice after construction would affect the provider's behavior

💡 AI prompt to fix

In failover.go, copy the `provs` slice into a new slice in `WithFailover`: `provs: append([]Provider(nil), provs...)`.

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

+	sort.Strings(out)
+	return out
+}
+


[major] 91 — KeyAvailable uses os.Getenv which observes the current process environment without any isolation; this makes it impossible to unit test without mutating global state. The t.Setenv in tests works, but the design comment says "no I/O or credential side effects," yet os.Getenv is a read side-effect

💡 AI prompt to fix

In providers/registry.go, add a `Getenv func(string) string` field to `Factory` (defaulting to `os.Getenv`) so that tests can inject a mock environment without touching process globals.

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

 			if config.emit != nil {
 				config.emit(event)
 			}
+			for _, aux := range config.auxEmits {


[minor] 210 — auxEmits runs after config.emit, so if an auxEmit panics, the user-provided WithEvents handler hasn't fired yet. This is subtle but documented; consider reversing order or documenting it explicitly

💡 AI prompt to fix

In session.go, add a comment above the emission loop documenting the exact ordering: `config.emit` fires before any `auxEmits`.

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

+// from T is intentionally out of scope.
+func NewTool[T any](spec ToolSpec, fn func(ctx context.Context, args T) (ToolResult, error)) Tool {
+	return Tool{
+		ToolSpec: spec,


[minor] 43 — NewTool unmarshals into var args T even when call.Arguments is nil or []byte(nil), which works (zero value), but it would be more explicit to skip unmarshal when len(call.Arguments) == 0 and just pass the zero value directly. The current code already does this

💡 AI prompt to fix

No change needed.

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

+		lowPat := strings.ToLower(pat)
+		if ok, _ := filepath.Match(lowPat, strings.ToLower(base)); ok {
+			return true, pat
+		}


[minor] 132 — The Match function lowercases the pattern for every path component check, but filepath.Match is case-sensitive on case-sensitive filesystems. This means a blocklist entry like Id_Rsa would not match id_rsa on Linux, even though the design says "case-insensitive."

💡 AI prompt to fix

In tools/fs/blocklist.go, add `strings.ToLower(clean)` and `strings.ToLower(base)` at the top of `Match` so the whole-path and basename checks are case-insensitive as documented, or clarify in the doc comment that only per-component matching is case-insensitive.

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>

github-actions Bot reviewed May 7, 2026

View reviewed changes

erain mentioned this pull request May 7, 2026

loop: StopReasonMaxTurns to flag budget exhaustion (closes #93) #100

Merged

4 tasks

erain and others added 6 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/92-prompt-catalog branch from 380521f to d328137 Compare May 7, 2026 14:15

erain merged commit c3b7f9b into main May 7, 2026
2 of 4 checks passed

erain deleted the issue/92-prompt-catalog branch May 7, 2026 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

prompts: PromptCatalog versioned-prompt loader (closes #92)#99
erain merged 7 commits into
mainfrom
issue/92-prompt-catalog

erain commented May 7, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot May 7, 2026

Uh oh!

github-actions Bot May 7, 2026

Uh oh!

github-actions Bot May 7, 2026

Uh oh!

github-actions Bot May 7, 2026

Uh oh!

github-actions Bot May 7, 2026

Uh oh!

github-actions Bot May 7, 2026

Uh oh!

github-actions Bot May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant