diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md index b17202c..a5b889f 100644 --- a/docs/roadmap/cleanup.md +++ b/docs/roadmap/cleanup.md @@ -1,239 +1,203 @@ -# Public Package Cleanup And Hardening Plan +# Internal Package Audit And Cleanup 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. +This document records the internal package audit findings and the agreed corrections. It is a planning document only; implementation belongs in a later pass. -The goals are: +The package count is not itself a problem. Most internal packages have useful boundaries: -- 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. +- `internal/domain`: internal contracts and value types. +- `internal/usecase`: prompt preparation and execution orchestration. +- `internal/adapter/cli` and `internal/adapter/http`: transport-specific mapping and IO. +- `internal/config`: app settings and CLI override precedence. +- `internal/promptdef`, `internal/profile`, and `internal/profile/builtin`: prompt/profile repositories and built-in profile source. +- `internal/artifact`: artifact reference resolution. +- `internal/prompt`: prompt rendering. +- `internal/llm`: OpenAI-compatible model adapter. +- `internal/validate`: output validation. +- `internal/format`: prepared-run render formatting. -Unless a stage explicitly says otherwise, do not change public method names, request/result shapes, CLI behavior, HTTP behavior, or built-in profile semantics. +Do not collapse packages merely to reduce the number of directories. Prefer targeted cleanup where a boundary is unclear, duplicated, or security-sensitive. -## Stage 1: Make Load Error Classification Deterministic +## Stage 1: Harden HTTP File Artifact Inputs 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`. +`internal/adapter/http` accepts `file` input references and maps them directly to `domain.ArtifactRef`. `internal/artifact` then reads `ref.URI` with `os.ReadFile`. Because `serve` has no built-in authentication, an HTTP caller can cause the server process to read arbitrary local files accessible to it. + +Decision: + +Keep HTTP file inputs because they are a core use case, but require an explicit allowlisted artifact root for HTTP file reads and enforce path containment before reading. 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. +1. Add an HTTP/server artifact root setting at the app wiring layer. + - Prefer a config field such as `server.artifact_root` or a clearly named top-level field such as `artifact_root`. + - Add the matching CLI `serve` override only if it fits existing config/CLI precedence patterns. + - The setting should apply to HTTP `serve` only; do not change CLI `run`/`render` file input behavior. +2. Introduce an artifact reader mode that restricts file reads to the configured root. + - Resolve the configured root with `filepath.Abs` and `filepath.Clean`. + - Resolve each requested file path against the root. + - Reject paths that escape the root, including `..` traversal and absolute paths outside the root. + - Use `filepath.Rel` or equivalent path-containment checks; do not rely on string-prefix checks alone. + - Consider rejecting symlink escapes if the threat model includes untrusted writable artifact roots. If symlinks are allowed, document that the root must not be writable by untrusted users. +3. Preserve inline input behavior. +4. Decide startup behavior: + - Recommended: if HTTP `serve` accepts `file` refs, `artifact_root` must be configured before file refs are allowed. + - If no root is configured, HTTP `file` refs should return `400 artifact_read_failed` or a more specific `400 artifact_not_allowed`; inline refs should continue to work. +5. Keep public Go API and CLI behavior unchanged unless explicitly using the restricted artifact reader for HTTP wiring. + +Tests: + +- HTTP tests: + - inline refs still work without an artifact root; + - file refs under the artifact root work; + - relative traversal outside the root is rejected; + - absolute paths outside the root are rejected; + - unknown fields and existing HTTP error shapes remain stable. +- Artifact tests: + - restricted reader accepts contained paths; + - restricted reader rejects escaped paths; + - path cleaning and absolute/relative input behavior are covered. +- Config/CLI tests: + - configured artifact root is loaded and overridden according to normal precedence. + +Documentation: + +- Update `docs/config.md`, `docs/cli.md`, `docs/integrations/http-api.md`, `docs/operations.md`, and `docs/internal/adapters.md`. +- Document only implemented behavior outside roadmap docs. + +## Stage 2: Consolidate Prompt/Profile YAML Catalog Helpers + +Problem: + +Prompt and profile repositories duplicate YAML discovery and `fs.FS` path handling. `internal/filecatalog` currently handles only OS directory walking, while `promptdef` and `profile` each carry similar `fs.FS` walking, file extension, path cleaning, display path, and stem logic. + +Decision: + +Expand `internal/filecatalog` into the shared YAML catalog/path helper for both OS directories and `fs.FS` roots. This is preferable to deleting it because both prompt and profile loaders need the same behavior. + +Implementation: + +1. Extend `internal/filecatalog` with helpers for: + - sorted YAML discovery under an `fs.FS` root; + - clean `fs.FS` root handling; + - display paths relative to an `fs.FS` root; + - YAML file stem handling for both OS and `fs.FS` paths. +2. Replace duplicate helpers in `internal/promptdef` and `internal/profile`: + - prompt `findPromptDefinitionYAMLFiles`; + - prompt `cleanFSRoot`; + - prompt `displayPath`; + - prompt `isPromptDefinitionYAMLFile`; + - profile `findProfileYAMLFiles`; + - profile `cleanFSRoot`; + - profile `displayPath`; + - profile `profileFileStem`; + - profile `isProfileYAMLFile`. +3. Preserve behavior: + - recursive nested search; + - `.yaml` and `.yml` support; + - deterministic ordering; + - subdirectories remain organizational only; + - duplicate ID behavior remains unchanged. + +Tests: + +- Expand `internal/filecatalog` tests to cover `fs.FS` roots and display paths. +- Keep prompt/profile repository tests passing unchanged where possible. +- Add regression tests if any ordering or relative-path behavior is easy to break. + +## Stage 3: Deduplicate Validator Control Flow + +Problem: + +`StandardValidator.Validate` and `FSValidator.Validate` duplicate most validation-mode logic. The meaningful difference is how JSON Schema documents are loaded and compiled. + +Decision: + +Extract shared validation-mode control flow and isolate schema loading behind a small internal function/interface. Keep the public package boundary unchanged. + +Implementation: + +1. Introduce an unexported schema validation helper, for example: + - `validateArtifact(ctx, artifact, contract, schemaValidatorFunc)`; + - or a small unexported interface that compiles/validates schema-backed instances. +2. Share handling for: + - context checks; + - nil artifact errors; + - `none`; + - `basic`; + - `json`; + - validation result initialization. +3. Keep source-specific behavior separate: + - directory-backed schema path resolution and `jsonschema.Compile`; + - `fs.FS` schema document loading and `compiler.AddResource`. +4. Preserve current validation semantics: + - content validation failures return `ValidationFailed` results; + - schema path/load/compile/runtime failures return errors; + - JSON parse failures in `json` and `json_schema` modes return failed validation results, not runtime errors. + +Tests: + +- Existing validator tests should remain the main regression suite. +- Add focused tests only if new helper behavior is not covered by current tests. +- Run `go test ./internal/validate ./...`. + +## Stage 4: Remove Stale Domain Surface + +Problem: + +`domain.RunResult.Error` and `domain.RunMetadata` appear unused. They make the central domain package broader than necessary and can confuse future contributors about intended result/error handling. + +Decision: + +Remove unused internal domain fields/types unless implementation-time review finds a live consumer. + +Implementation: + +1. Remove `Error error` from `domain.RunResult`. +2. Remove `domain.RunMetadata`. +3. Update tests, formatters, adapters, and public conversions only if compilation reveals hidden references. +4. Do not remove fields from public root-package types in this stage. 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 +## Stage 5: Reassess Thin Interface Files Without Collapsing Packages 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`. +Several packages have small interface-only files, such as: -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. +- `internal/llm/client.go` +- `internal/prompt/renderer.go` +- `internal/validate/validator.go` +- `internal/profile/repository.go` +- `internal/promptdef/repository.go` 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. +Keep these package boundaries. The interfaces describe real use-case seams and are imported by adapters and `internal/usecase`. Do not merge these packages solely to remove small files. 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. +1. No code change is required for package consolidation. +2. If a package is already being edited, it is acceptable to move an interface into a nearby implementation file only when that improves readability and does not blur ownership. +3. Keep `internal/domain` free of adapter implementation details. +4. Keep `internal/usecase` dependent on interfaces rather than concrete filesystem/model implementations. -Tests: +## Stage 6: Documentation And Final Verification -- 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 ./...`. +After implementation: -## 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. +1. Update internal docs: + - `docs/internal/adapters.md` for HTTP artifact-root behavior and any filecatalog/package-boundary changes; + - `docs/internal/runner.md` only if runner behavior or error classification changed as part of this cleanup. +2. Update user/operator docs for HTTP artifact root: + - `docs/config.md`; + - `docs/cli.md`; + - `docs/integrations/http-api.md`; + - `docs/operations.md`; + - `docs/troubleshooting.md` if new failure modes are introduced. 3. Run: ```bash @@ -248,26 +212,10 @@ Implementation: --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. +- Do not collapse packages merely because they are small. +- Do not remove HTTP file inputs; restrict them with an explicit artifact root. +- Do not change public Go API behavior as part of internal package cleanup. +- Do not introduce a new auth system for HTTP `serve` in this cleanup pass. +- Do not change prompt/profile YAML lookup semantics except by preserving them through shared helpers.