274 lines
15 KiB
Markdown
274 lines
15 KiB
Markdown
# 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.
|