diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md new file mode 100644 index 0000000..b17202c --- /dev/null +++ b/docs/roadmap/cleanup.md @@ -0,0 +1,273 @@ +# Public Package Cleanup And Hardening Plan + +This plan converts the public package audit into staged implementation work. The target audience is an LLM coding agent implementing each stage in order. + +The goals are: + +- make public-facing code concise, idiomatic, and predictable; +- harden all public boundary inputs against accidental secret exposure, mutation side effects, and malformed caller-provided values; +- preserve the current library user story: a downstream Go application can provide prompt/profile sources, choose built-ins or in-memory profiles, pass a request-scoped API key, and receive prepared or generated results; +- avoid broad feature work or unrelated refactors. + +Unless a stage explicitly says otherwise, do not change public method names, request/result shapes, CLI behavior, HTTP behavior, or built-in profile semantics. + +## Stage 1: Make Load Error Classification Deterministic + +Problem: + +`internal/usecase.ErrProfileLoad` currently wraps both prompt-definition load failures and execution-profile load failures. Public mapping then infers `ErrPromptLoad` or `ErrProfileLoad` from wrapped causes. This is brittle because some profile repository failures, such as profile directory read failures, are not wrapped with a profile sentinel and can still map to `scriptorium.ErrPromptLoad`. + +Implementation: + +1. In `internal/usecase`, split the current load sentinel into two explicit sentinels: + - `ErrPromptLoad = errors.New("failed to load prompt definition")` + - `ErrProfileLoad = errors.New("failed to load execution profile")` +2. Update `Runner.Prepare`: + - wrap `promptDefs.GetPromptDefinition` failures with `ErrPromptLoad`; + - wrap prompt-definition hashing failures with `ErrPromptLoad`; + - wrap `profiles.GetProfile` failures with `ErrProfileLoad`. +3. Update internal tests that currently expect prompt load failures to use `ErrProfileLoad`. +4. Simplify public error mapping in `errors.go`: + - map `usecase.ErrPromptLoad` to public `ErrPromptLoad`; + - map `usecase.ErrProfileLoad` to public `ErrProfileLoad`; + - retain direct mappings for `promptdef.ErrPromptDefinitionNotFound`, `profile.ErrProfileNotFound`, profile YAML/profile validation causes, and prompt YAML/prompt validation causes. +5. Keep adapter behavior stable: + - HTTP prompt-load failures should still return the existing prompt-load status/code; + - HTTP profile-load failures should still return the existing profile-load status/code; + - CLI errors should remain concise and must not include raw secrets. + +Tests: + +- Add public package tests proving: + - prompt-definition repository load errors map to `scriptorium.ErrPromptLoad`; + - selected profile repository read/list errors map to `scriptorium.ErrProfileLoad`; + - selected malformed profile YAML and raw `api_key` rejection still map to `ErrProfileLoad`; + - prompt-not-found and profile-not-found sentinels still map to `ErrPromptNotFound` and `ErrProfileNotFound`. +- Add or update internal usecase tests for the new `ErrPromptLoad` and `ErrProfileLoad` split. +- Run `go test ./...`. + +## Stage 2: Stop Mutating Caller-Owned HTTP Clients + +Problem: + +The default OpenAI-compatible client currently stores the caller-provided `*http.Client` and mutates `Timeout` when it is zero. Because the public `scriptorium.Config.HTTPClient` accepts this client, engine construction can unexpectedly modify caller-owned shared clients, including `http.DefaultClient`. + +Implementation: + +1. In `internal/llm.NewOpenAICompatibleClient`, clone any supplied `cfg.HTTPClient` before applying timeout defaults: + - if `cfg.HTTPClient != nil`, copy the struct value with `cloned := *cfg.HTTPClient` and use `&cloned`; + - if `cloned.Timeout == 0`, set `cloned.Timeout` to the resolved default timeout; + - do not mutate `cfg.HTTPClient`. +2. Preserve existing request-level timeout behavior: + - positive `timeout_seconds` still overrides the client timeout for that request; + - explicit `timeout_seconds: 0` with target presence still disables client timeout for that request; + - omitted timeout still uses the client/default timeout. +3. Do not clone or replace shared internals such as `Transport`; shallow `http.Client` copying is the idiomatic choice here. + +Tests: + +- Add LLM client tests proving: + - a supplied zero-timeout `http.Client` remains unchanged after `NewOpenAICompatibleClient`; + - the constructed client still uses the default timeout internally; + - a supplied nonzero-timeout `http.Client` remains unchanged; + - request-level timeout override behavior remains unchanged. +- Run `go test ./internal/llm ./...`. + +## Stage 3: Harden Public JSON-Like Inputs + +Problem: + +Public `ExtraParams` and structured-output schema values are copied with a permissive reflection helper. It deep-copies useful typed JSON-like maps/slices, but it accepts arbitrary values, pointers, functions, channels, and cyclic structures. A cyclic value can recurse indefinitely and crash the process. + +Decision: + +Replace permissive copying at public boundaries with validating JSON-compatible copying. This is more correct and maintainable than trying to copy arbitrary Go object graphs. `ExtraParams` and schema payloads are provider JSON payloads; accepting non-JSON values only delays failure and makes security behavior harder to reason about. + +Implementation: + +1. Introduce an internal root-package helper for public boundary JSON values, for example: + - `copyJSONMap(src map[string]any) (map[string]any, error)` + - `copyJSONValue(value any, path string, seen map[visit]struct{}) (any, error)` +2. Accept only JSON-compatible value kinds: + - `nil` + - `string` + - `bool` + - signed and unsigned integer types that can be represented safely enough for JSON provider payloads; + - floating-point values excluding `NaN` and infinities; + - maps with string keys; + - slices and arrays; + - values behind `interface`; + - optionally pointers only by dereferencing them, with cycle detection. +3. Reject unsupported values with clear public errors: + - `WithProfiles` profile `ExtraParams` errors return `ErrInvalidConfig`; + - `OpenAICompatibleProfile` should not silently accept invalid `ExtraParams`; because it currently returns only `Profile`, keep it a pure field-copy constructor and perform validation in `WithProfiles`; + - request `ExecutionTargetOverride.ExtraParams` errors return `ErrInvalidRequest`. +4. Track visited map/slice/pointer identities to reject cycles instead of recursing indefinitely. +5. Keep typed JSON-compatible values isolated from caller mutation: + - `map[string]string`, `map[string]int`, `[]int`, `[]float64`, nested `map[string]any`, and values behind `any` should still be copied. +6. Preserve internal-only copy helpers where they are operating on trusted domain data, but use validating helpers at public-to-domain boundaries. +7. Ensure any provider `extra_params` validation in the LLM adapter still rejects reserved keys and empty keys as it does today. + +Tests: + +- Add public package tests for `ExecutionTargetOverride.ExtraParams`: + - typed JSON-compatible maps/slices are accepted and isolated from caller mutation; + - function/channel/struct values return `ErrInvalidRequest`; + - maps with non-string keys return `ErrInvalidRequest`; + - cyclic maps/slices return `ErrInvalidRequest` without panic or hang; + - `NaN` and infinities return `ErrInvalidRequest`. +- Add public package tests for `WithProfiles`: + - invalid profile `ExtraParams` return `ErrInvalidConfig`; + - cyclic profile `ExtraParams` return `ErrInvalidConfig`; + - valid typed maps/slices remain deep-copied. +- Run `go test ./...`. + +## Stage 4: Redact Public Secret Fields In String Formatting + +Problem: + +`RunRequest.APIKey` and `GenerateRequest.APIKey` use `json:"-"`, so JSON marshaling does not emit direct API keys. Ordinary Go formatting such as `fmt.Printf("%+v", req)` can still print exported string fields. + +Decision: + +Add redacting formatting methods to the public request types that directly carry raw secrets. This does not protect against reflection-based dumpers, but it materially improves safety for normal `fmt`, structured logs that call `String`, and test diagnostics. Avoid replacing raw strings with a new secret wrapper type in this cleanup pass because that would add API complexity and likely require downstream changes. + +Implementation: + +1. Add `String() string` and `GoString() string` methods for: + - `RunRequest` + - `GenerateRequest` +2. Formatting behavior: + - include useful non-secret context such as prompt/profile IDs and whether an API key is set; + - never include the raw `APIKey` value; + - avoid dumping full rendered prompt or output content unless existing public behavior explicitly needs it. +3. Consider adding redaction helpers for nested public types only if needed by these methods. +4. Do not alter JSON tags or existing JSON output shapes. +5. Update consumer docs to warn that reflection-based debug dumpers can still expose exported fields and that callers should avoid logging raw request structs with reflection dump libraries. + +Tests: + +- Add public package tests proving: + - `fmt.Sprint(req)`, `fmt.Sprintf("%+v", req)`, and `fmt.Sprintf("%#v", req)` do not contain the direct API key for `RunRequest`; + - the same holds for `GenerateRequest`; + - JSON marshaling continues to omit the direct API key. +- Run `go test ./...`. + +## Stage 5: Polish The Public Option Type + +Problem: + +The public API currently exposes `type Option func(*engineOptions) error`. This works, but `go doc` shows an exported type whose signature references an unexported implementation type. That is legal Go, but it is not a polished public API. + +Decision: + +Change `Option` to an exported interface with an unexported method, backed by an unexported function adapter. This is the standard pattern when a package wants an opaque option type while preserving functional option ergonomics for package-provided options. + +Implementation: + +1. Replace the exported function type with: + + ```go + type Option interface { + apply(*engineOptions) error + } + + type optionFunc func(*engineOptions) error + + func (f optionFunc) apply(options *engineOptions) error { + return f(options) + } + ``` + +2. Update all option constructors to return `optionFunc(...)`. +3. Update `NewEngine` to call `opt.apply(&options)`. +4. Preserve nil-option behavior if practical: + - because `Option` is now an interface, a nil interface can still be skipped; + - typed nil option values are not expected from package users and do not need special support. +5. This is a public source compatibility change only for callers that manually construct custom `Option` values. The package does not currently provide a documented way for callers to create custom options, so this is acceptable before API freeze. + +Tests: + +- Existing option tests should continue passing. +- Add a small compile-time or runtime test proving package-provided options still compose as before. +- Run `go test ./...` and `go doc .` to verify the public docs no longer expose `*engineOptions` in the `Option` signature. + +## Stage 6: Review And Tighten Redundant Public Surface + +Problem: + +`OpenAICompatibleProfileConfig` currently duplicates `Profile` almost exactly, and `OpenAICompatibleProfile` is a shallow convenience constructor plus deep copy. This is not harmful, but it may be unnecessary public surface unless it provides provider-specific clarity or future extension space. + +Decision: + +Keep `OpenAICompatibleProfile` and `OpenAICompatibleProfileConfig` for now, but tighten documentation and tests around their purpose. The constructor should remain a convenience for building a normal in-memory `Profile`; it must not register global state, maintain a model catalog, or hide credential resolution. Removing it would reduce surface area, but it would also undo the recently documented library ergonomics without a strong correctness benefit. + +Implementation: + +1. Add or improve doc comments for all exported option functions and source helpers: + - `WithPromptFS` + - `WithPromptFile` + - `WithProfileFS` + - `WithProfileFile` + - `WithSchemaFS` + - `WithSchemaFile` +2. Improve doc comments for `Profile`, `OpenAICompatibleProfileConfig`, and `OpenAICompatibleProfile`: + - explain that the constructor returns an ordinary in-memory profile; + - explain that `APIKeyRequired` is satisfied by `RunRequest.APIKey`; + - state that raw API keys do not belong in profiles. +3. Re-run `go doc .` and confirm the public API reads cleanly. +4. Do not remove or rename existing exported types in this stage. + +Tests: + +- No new behavior tests are required unless comments reveal behavior drift. +- Run `go test ./...` and `go vet ./...`. + +## Stage 7: Final Security And Compatibility Pass + +Implementation: + +1. Search for raw secret emission paths: + - `rg "APIKey|api_key|Authorization|%\\+v|GoString|String"`. +2. Confirm: + - config, profile YAML, CLI flags, and HTTP JSON still reject raw API keys; + - public `RunRequest.APIKey` remains request-scoped and JSON-omitted; + - `PreparedRun`, `RunResult`, and formatter output do not include direct API keys; + - injected `LLMClient` still receives `GenerateRequest.APIKey` when provided. +3. Run: + + ```bash + go test ./... + go vet ./... + go run ./examples/go-library/prepare + go run ./cmd/scriptorium render \ + --config ./examples/config.yml \ + --prompt generic.markdown_summary \ + --input transcript=./examples/fixtures/transcript.md \ + --input glossary=./examples/fixtures/glossary.yml \ + --format json + ``` + +4. Update implemented-behavior docs only after code changes: + - `docs/consumers/pkg-scriptorium.md` for redaction and JSON-compatible `ExtraParams` validation; + - `docs/internal/runner.md` if internal error sentinel names or load classification are documented there; + - `docs/internal/adapters.md` if adapter error mapping documentation references the old shared sentinel. + +## Non-Goals + +- Do not add a credential resolver. +- Do not add raw API-key support to config, profile YAML, CLI flags, or HTTP JSON. +- Do not introduce new third-party dependencies. +- Do not add new model/profile catalog behavior. +- Do not redesign prompt/profile/schema source loading beyond the explicit cleanup items above. +- Do not change CLI or HTTP request/response shapes unless required to preserve current documented behavior. + +## Open Questions + +None. The plan chooses the more idiomatic and maintainable options where the audit identified tradeoffs: + +- split internal load sentinels rather than expanding public inference heuristics; +- clone caller-owned `http.Client` values rather than documenting mutation; +- validate JSON-like inputs rather than copying arbitrary Go object graphs; +- redact public secret-bearing request structs rather than replacing API-key strings with a larger credential abstraction; +- keep `OpenAICompatibleProfile` as documented ergonomic surface for now rather than removing recently added API.