scriptorium/docs/roadmap/cleanup.md

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:

   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:

   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.