review-go-codex

name: review-go-codex description: > Systematic consistency review of the go-codex library: codecs (Layer 1), REST/event/MCP API contracts (Layer 2), and forge pipelines (Layer 3). Use when asked to "review go-codex", "consistency audit", "check for inconsistencies", "simple workflow review", "audit API surface", or after a significant round of refactoring. Reviews cross-layer naming parity, param types, builder methods, error shapes, structured errors, observer pattern, unit test coverage, and example correctness.

review-go-codex

Systematic consistency audit of the go-codex library across all three layers. Reads key source files, applies the checklist in references/checklist.md, collects findings, categorises by severity, and plans a fix round.

User Experience North Star

Every finding and fix must be evaluated against one question:

Does this make the library more declarative, simple, and consistent for the user?

The three layers form a single workflow. A user should be able to move between them without context switching or learning a new mental model:

The workflow across layers is always: declare → compose → register. If a finding breaks this pattern — forces the user to use imperative calls, repeat themselves, or learn layer-specific vocabulary — it is at minimum a small finding.

Concrete implications for every review:

• Declarative: API objects (RouteHandle, ChannelHandle, ToolHandle, ResourceHandle, PromptHandle, FunctionSpec) are values, not builder side-effects. Users pass and store them. No magic, no global state.
• Simple: one method does one thing. Avoid overloaded methods. Param defaults should be safe.
• Consistent: same concept, same name. Meta structs, Opt interfaces, Builder/Registry fluent methods, error type shapes — all should follow the same pattern across layers. If Layer 2 has WithCodec, Layer 2 events should have it too; if Layer 3 adds governance fields, their names should mirror Layer 2's Meta naming.

When to Use This Skill

• User says "review go-codex", "consistency audit", "check go-codex API", "audit"
• User says "consistent, declarative, simple workflow"
• After a significant feature addition or refactoring round
• When something "feels off" but the user cannot pinpoint it

Step-by-Step Workflow

Phase 1 — Read key files in parallel

Read all of these before opening any finding:

Also scan: api/rest/*_test.go, api/events/*_test.go, api/mcp/*_test.go, adapters/mcpgo/*_test.go, forge/*_test.go, render/pipeline/*_test.go

Then read references/history.md to see what was already fixed. Do not re-report these.

Phase 2 — Apply the checklist

Work through references/checklist.md section by section:

1. Cross-layer naming parity
2. Param type consistency
3. Builder method parity
4. Codec field godoc
5. Format API parity
6. Forge consistency
7. Error sentinel consistency (structured errors)
8. Observer pattern
9. Unit test coverage
10. Example correctness

Phase 3 — Record findings

For every issue found, assign:

Present findings as a table, then group by priority: bugs first, then small, then trivial.

Phase 4 — Produce a plan

Write findings and their priority order to the session plan.md. Do NOT start implementing until the user confirms the plan or says "start" / "get to work".

Phase 5 — Implement (after approval)

For each finding, in priority order:

1. Apply the fix
2. Run go fmt ./... — format immediately after each edit
3. Run go build ./... (must stay clean)
4. Run go test ./... (all packages must pass)
5. Run just check (staticcheck + gosec — no new warnings)
6. Update .github/instructions/go-codex.instructions.md if any exported API changed

After all fixes:

• Run every example: for d in examples/*/; do go run ./$d; done — each must exit 0
• If any example uses a stale pattern, update it to match the current API

Phase 6 — Verify

Run in this order — each must be clean before proceeding to the next:

go fmt ./...          # format all files; no diff should remain
go build ./...        # must compile with zero errors
go test ./...         # all packages must pass
just check            # staticcheck + gosec; no new suppressions allowed

If just check surfaces a warning introduced by your changes, fix it before proceeding. Do not add //nolint or //gosec suppressions to silence new findings.

Phase 7 — Update History

Before producing the commit summary, append a new section to references/history.md for this round. Follow the existing format:

## Round <N> (<short title>)

- **<ID> — <finding title>**: one-sentence description of what was wrong and how it was fixed.
- ...

---

Rules:

• Include only findings that were actually implemented (not deferred/skipped).
• One bullet per finding; keep the description concise and factual.
• Insert the new section above Round (newest round at top of file, below the header).

Phase 8 — Commit Summary

After updating history.md, produce a commit-ready summary. Do not commit — present this to the user and let them decide.

Format:

<short imperative title — max 72 chars>

Layer(s) affected: <Layer 1 / Layer 2 REST / Layer 2 events / Layer 2 MCP / Layer 2 MCP adapter / Layer 3 forge>
Round: R<N>

Findings fixed:
- <ID> [<severity>] <one-line description of fix>
- ...

Tests: <N> new / updated test(s) in <package(s)>
Examples: <"no changes needed" | list of updated examples>
Docs: .github/instructions/go-codex.instructions.md updated

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Rules:

• Title must be imperative: "Fix ValidateTopicVars missing key check", not "Fixed…"
• List only findings that were actually implemented, not skipped or deferred
• If zero examples changed, write "Examples: no changes needed"
• Always include the Co-authored-by trailer

Structured Errors Guardrail

go-codex uses typed errors, not bare strings. Check every error return site:

• Server adapters (nethttp/chi) must return typed errors: rest.PathParamError, rest.QueryParamError, rest.HeaderParamError, rest.CookieParamError, rest.MissingPathVarError, rest.SecurityCredentialError, rest.SecurityError.
• Client adapter (nethttp.Call) must return typed errors (all errors.As-navigable):
  • rest.PathParamError, rest.QueryParamError, rest.CookieParamError, rest.HeaderParamError, rest.MissingPathVarError — same as server (from pre-flight codec validation, no HTTP call sent)
  • nethttp.UnexpectedStatusError{Method, Path, StatusCode, Body} — non-2xx response
  • nethttp.RequestBuildError{Err} — http.NewRequestWithContext failure (invalid URL, cancelled ctx)
  • nethttp.RequestError{Method, Path, Err} — http.Client.Do failure (network, DNS, TLS, timeout)
  • nethttp.ResponseBodyError{Err} — io.ReadAll failure after successful connection
  • Bare fmt.Errorf(...) without wrapping a typed sentinel is a finding in client.go.
• events adapter must return: events.TopicParamError, events.MissingTopicVarError.
• forge must return: forge.InputError, forge.OutputError, forge.ApplyError, forge.RefinementError.
• api/mcp typed errors (all errors.As-navigable):
  • mcp.ToolInputError{Name, Err} — from ToolHandle.Decode
  • mcp.ToolOutputError{Name, Err} — from ToolHandle.Encode
  • mcp.ResourceEncodeError{URI, Err} — from ResourceHandle.Encode
  • mcp.ResourceParamError{Name, Value, Err} — from ResourceHandle.BuildURI/ValidateURIVars
  • mcp.MissingResourceVarError{Name} — from ResourceHandle.BuildURI/ValidateURIVars
  • mcp.InvalidResourceParamError{Name, URITemplate} — from Resource.Register
  • mcp.PromptArgError{Name, Err} — from PromptHandle.ValidateArgs
  • mcp.MissingPromptArgError{Name} — from PromptHandle.ValidateArgs
• adapters/mcpgo error behavior — different from REST/events adapters:
  • Input decode/validation failures → mcp.NewToolResultError(err.Error()) returned as *mcp.CallToolResult with IsError: true (not a Go error). LLM sees the error text.
  • Output encode failures → protocol-level Go error (nil, err) (server contract violation).
  • fmt.Errorf("...") wrapping a typed sentinel is fine; bare fmt.Errorf without a typed wrapper is a finding.
• fmt.Errorf("...") wrapping a structured error is fine; a bare fmt.Errorf without wrapping a typed sentinel is a finding.
• Callers must be able to errors.As / type-switch on every error for structured logging.

Observer Pattern Guardrail

stats.Observer has four interfaces; adapters must call them correctly:

Rules:

• SecurityObserver must be guarded: if so, ok := obs.(stats.SecurityObserver); ok { so.RecordSecurityRejection(...) }
• PipelineObserver.RecordApply must be called for every function in a pipeline, including wrapped collection ops (Map, Filter, etc.)
• Adapter Observer must be called on every code path, including early-exit error paths
• adapters/nethttp client (Call) observer rules:
  • RecordRequest(method, path, statusCode, duration) called on every code path — status 0 when no HTTP call was sent (pre-flight validation failure, CredentialFunc error, RequestBuildError)
  • stats.ReportErrors(obs, location, err) called for param validation failures before RecordRequest (location: "path", "query", "cookie", "header", "body")
  • RecordRequest uses the route path template (e.g. "/users/{id}"), not the concrete URL — this allows observers to group metrics by route
  • Status 0 is the sentinel for "call attempted but no HTTP request reached the network" — document this in any Observer godoc for client-side code
• adapters/mcpgo observer rules:
  • RecordRequest("tool"|"resource"|"prompt", name, statusCode, duration) — called on every code path (200 success, 400 input error, 500 handler/output error)
  • stats.ReportErrors(obs, "input", err) — called before RecordRequest when codec validation fails (fires RecordValidationError per field)
  • No SecurityObserver at the mcpgo adapter level — MCP security is handled outside the adapter (no SecurityFunc field on mcpgo.Options)
  • Observer location values: "input" for tool argument decode/validation; "prompt.args" for prompt arg codec failures

Unit Test Coverage

For each exported type or method added, there must be at least one test covering:

• Happy path (valid input → expected output)
• Error path (invalid input → correct typed error)
• For builder methods: that the method returns the right shape (field set correctly)

Spot-check:

grep -n "func Test" api/rest/builder_test.go api/events/builder_test.go forge/forge_test.go render/pipeline/pipeline_test.go

If a new exported symbol has no corresponding TestX function, file a trivial finding.

Example Correctness

Every examples/*/main.go must:

1. Build cleanly: go build .
2. Demonstrate the feature its directory name implies
3. Use current API (no stale patterns):
   • Must use .WithCodec(codec) not Codec: &codec
   • Must use NewRoute / NewSSERoute / NewChannel declarative constructors
   • Must not call deleted or renamed methods

Run them all:

for d in examples/*/; do
  echo "=== $d ===" && cd $d && timeout 5 go run . &
  cd - >/dev/null
done
wait

If an example panics or uses a stale pattern, file a finding.

Gotchas

• Do not re-report R1-Rxx items. See references/history.md for the full list of what is already fixed.
• FunctionKindScalar is "" (empty string). NewFunction/Compose never write Kind — scalar functions have Kind=="" by design. The render/pipeline renderer omits kind: for scalar. This is correct.
• rest.Builder.AddServer discards name after description fallback. OpenAPI servers are a keyless ordered array. events.Builder.AddServer stores the name (AsyncAPI servers are keyed). Same call site, different semantics.
• PathParam and TopicParam have no Required field. This is by design — OpenAPI mandates path params are always required; topic vars must always be present. Godoc explains this.
• ResourceParam has no Required field. URI vars in a template must always be present (same rationale as PathParam/TopicParam). PromptArg DOES have Required bool — prompt args are optional by default.
• SecurityScheme codec is spec-only. No adapter enforces it unless SecurityFunc does so explicitly.
• SubscribeFormats / PublishFormats take priority over Formats. Adapters must check these before falling back to handle.Formats.
• ToolHandle.Decode takes any, not []byte. MCP protocol already deserializes arguments to map[string]any before the adapter calls BindArguments. Re-encoding to bytes and back would be wasteful. This is correct and intentional — do not flag as inconsistency with REST/events Decode([]byte).
• MCP input errors are IsError: true results, not Go errors. adapters/mcpgo.ToolHandler returns mcp.NewToolResultError(...) with IsError: true for codec validation failures. This is the MCP protocol's way of reporting tool errors to the LLM. Do not flag as missing typed error return.
• mcp.Info uses Name not Title. REST uses Info{Title} (OpenAPI spec), events uses Info{Title} (AsyncAPI spec), MCP uses Info{Name} (MCP protocol). All are protocol-driven — this is correct, not an inconsistency.
• api/mcp has no security methods. No AddSecurityScheme, AddGlobalSecurity, or SecurityFunc — MCP security is handled separately and not part of the core api/mcp builder. This is by design.
• Do not invent new API surface during a review. Findings should fix inconsistencies in existing API, not design new features.
• Update go-codex.instructions.md after every code change. The instructions file is the single source of design truth — it must stay in sync.

References

• references/history.md — findings fixed in Rounds
• references/checklist.md — full cross-layer consistency checklist
• .github/instructions/go-codex.instructions.md — design contract