Audit code quality and deduplication opportunities

docs/roadmap/audit.md

@@ -0,0 +1,519 @@
+# Code Quality And Deduplication Audit
+
+## Executive Summary
+
+Overall code quality is solid for the current project size. The repository follows the documented shape: thin CLI/HTTP adapters, a central `Runner`, strict config/prompt/profile loading, narrow filesystem and LLM adapters, and package tests around the important public behaviors.
+
+The top three refactoring targets are:
+
+1. centralize execution-target field mapping across profiles, CLI overrides, HTTP DTOs, prepared output, and LLM request serialization;
+2. share filesystem repository scanning helpers between prompt and profile repositories;
+3. reduce CLI command wiring duplication for app settings, runner construction, and common preflight behavior.
+
+The codebase appears ready for a limited cleanup pass. No major architectural rewrite is warranted. The main risk before a release is maintenance drift when adding fields or policy to execution settings, repositories, or command setup.
+
+## Repository Map Reviewed
+
+Main areas inspected:
+
+- `cmd/scriptorium`: process entrypoint path, via repository layout and command docs.
+- `internal/adapter/cli`: `run`, `render`, `serve` parsing, config loading, app wiring, stdout/stderr behavior, summary output, exit-code policy.
+- `internal/adapter/http`: request DTOs, strict JSON decoding, domain mapping, response metadata mapping, error mapping.
+- `internal/config`: app config loading, defaults, strict YAML decoding, CLI override precedence.
+- `internal/defaults`: built-in runtime/config/content-type defaults.
+- `internal/domain`: domain request/result/profile/target/artifact/validation types.
+- `internal/usecase`: `Runner.Prepare`, `Runner.Run`, execution target merge, schema structured-output setup, validation and repair policy.
+- `internal/promptdef`: recursive prompt YAML loading, prompt normalization, content-file resolution, prompt output-contract validation.
+- `internal/profile`: recursive profile YAML loading, strict decoding, duplicate ID handling, profile validation, raw API-key rejection.
+- `internal/artifact`: `inline` and `file` artifact resolution.
+- `internal/prompt`: Go template rendering and required input checks.
+- `internal/llm`: OpenAI-compatible request construction, timeout/API-key handling, response parsing.
+- `internal/validate`: basic/JSON/JSON Schema validation and schema document loading.
+- `internal/format`: prepared-run text/JSON output.
+- `examples/`: maintained example config, prompts, profiles, schemas, fixtures, HTTP/render examples.
+- Package tests under `internal/**`.
+
+Major execution paths reviewed:
+
+- CLI `run`: parse flags/config, build `RunRequest`, construct runner, call LLM, write artifact, print summary, choose exit code.
+- CLI `render`: parse flags/config, build `RunRequest`, construct runner without LLM, prepare only, format prepared run.
+- CLI `serve`: parse flags/config, construct runner, expose HTTP `POST /v1/runs`.
+- HTTP `POST /v1/runs`: strict decode DTO, map to `RunRequest`, map `RunResult` to JSON response.
+- Runner `Prepare` and `Run`: prompt/profile/artifact/schema resolution, render, LLM generate, validation, repair hook.
+
+Areas not deeply inspected:
+
+- No `internal/app`, `internal/stage`, `internal/modules`, `internal/storage`, `internal/manifest`, `pkg`, or external test-suite directories exist in the current repository.
+- Full test execution was not run because this is a report-only pass; inspection was static.
+
+## High-Confidence Deduplication Opportunities
+
+### 1. Execution Target Field Mapping Is Repeated Across Boundaries
+
+Affected files/packages:
+
+- `internal/domain/domain.go`
+- `internal/usecase/runner.go`
+- `internal/adapter/cli/run.go`
+- `internal/adapter/http/dto.go`
+- `internal/adapter/http/handler.go`
+- `internal/llm/openai_compatible_client.go`
+- `internal/format/prepared_run.go`
+- tests in `internal/usecase`, `internal/adapter/http`, `internal/adapter/cli`, `internal/llm`, and `internal/format`
+
+Duplicated or near-duplicated behavior:
+
+- Execution settings fields are listed and copied in several places: profile-to-target conversion, target merge, CLI runtime override construction, HTTP request mapping, HTTP metadata mapping, prepared-run text output, and OpenAI-compatible request construction.
+- Adding `service_tier` required coordinated edits across many of these sites, which is a strong signal that field-level mapping is too scattered.
+
+Why it matters:
+
+- A future profile/runtime field can easily be parsed but not sent, displayed but not merged, accepted over HTTP but not included in metadata, or tested in one command but not another.
+- This is public-interface drift risk because CLI, HTTP, render output, and provider requests all expose different slices of the same effective execution target.
+
+Recommended refactor:
+
+- Keep transport DTOs local to adapters, but add small mapping helpers at each boundary:
+  - a use-case/domain helper for profile-to-target copy and execution-target merge;
+  - an HTTP helper such as `executionTargetFromModelOverrideDTO` and `modelParamsDTOFromExecutionTarget`;
+  - an LLM helper such as `openAIChatRequestFromGenerateRequest` for outbound serialization.
+- Add a table-driven test that constructs an `ExecutionTarget` with every supported field and verifies the HTTP metadata DTO and LLM request payload retain the intended fields.
+- Avoid reflection-based generic copying; explicit mapping is still clearer here.
+
+Suggested tests:
+
+- Expand runner merge tests to use an all-fields target.
+- Add HTTP DTO mapping tests that fail when a domain execution field is omitted from request or metadata mapping.
+- Add LLM request serialization tests for all serialized fields and explicit omission of non-serialized fields.
+
+Risk level: high. The refactor itself is low-to-medium implementation risk, but the duplicated behavior has high drift risk.
+
+### 2. Prompt And Profile Filesystem Repository Scanning Is Duplicated
+
+Affected files/packages:
+
+- `internal/promptdef/filesystem_repository.go`
+- `internal/profile/filesystem_repository.go`
+- repository tests in `internal/promptdef` and `internal/profile`
+
+Duplicated or near-duplicated behavior:
+
+- Both repositories recursively walk configured directories, filter `.yaml`/`.yml`, sort paths, compute relative paths, derive likely IDs from filenames, use strict YAML decoding, partially decode IDs to decide whether to surface malformed likely-target files, detect duplicate IDs, and include relative paths in errors.
+
+Why it matters:
+
+- Prompt/profile repository policy should remain aligned: nested scanning, stable ordering, duplicate ID errors, likely-target malformed file behavior, and relative-path diagnostics.
+- Future changes to repository scanning or extension rules would need to be made in two packages.
+
+Recommended refactor:
+
+- Introduce a narrow internal helper for filesystem catalog behavior, for example `internal/filecatalog` or another small package whose scope is only:
+  - recursive YAML file discovery with context cancellation;
+  - stable sorting;
+  - relative path formatting;
+  - YAML extension checks;
+  - filename stem extraction.
+- Keep prompt/profile-specific normalization and validation in their current packages.
+- Do not make a generic repository framework.
+
+Suggested tests:
+
+- Add helper-level tests for recursive YAML discovery, ordering, extension filtering, and relative path output.
+- Keep existing prompt/profile behavior tests unchanged to confirm public error behavior survives.
+
+Risk level: high for drift prevention; low implementation risk if the helper stays small.
+
+### 3. CLI Command Setup And Runner Wiring Are Repeated
+
+Affected files/packages:
+
+- `internal/adapter/cli/run.go`
+- `internal/adapter/cli/run_test.go`
+
+Duplicated or near-duplicated behavior:
+
+- `run`, `render`, and `serve` each construct similar runner dependencies.
+- `run` and `serve` both create an OpenAI-compatible client with default timeout.
+- `run`, `render`, and `serve` all resolve app settings, clean dirs, validate required prompt/profile dirs, and create filesystem repositories/validator/renderer/readers.
+- `run` and `render` share most execution request flags and request construction, with command-specific differences around `--schema-dir`, `--format`, and LLM use.
+
+Why it matters:
+
+- Adding or changing a dependency, default, or preflight check can drift between commands.
+- The current duplication is still readable, but it is large enough that future command additions or option changes will be error-prone.
+
+Recommended refactor:
+
+- Add a small CLI-local wiring helper, such as `newRunnerFromDirs(promptDir, profileDir, schemaDir string, llmClient llm.Client) *usecase.Runner`.
+- Add a small CLI-local resolved settings struct for common `prompt_dir`, `profile_dir`, `schema_dir`, and render-format handling.
+- Preserve the existing command-specific parse functions and flag surfaces; do not hide command behavior behind a generic command framework.
+
+Suggested tests:
+
+- Keep parser tests for each command.
+- Add one test that verifies `run`, `render`, and `serve` use the same configured prompt/profile/schema dirs by exercising config-derived dirs.
+- Keep command-level success tests for `run` and `render`.
+
+Risk level: medium. Behavior is public, but a small CLI-local helper can be behavior-preserving.
+
+### 4. HTTP Error Mapping Depends On Error Message Substrings
+
+Affected files/packages:
+
+- `internal/usecase/runner.go`
+- `internal/adapter/http/handler.go`
+- `internal/adapter/http/handler_test.go`
+
+Duplicated or near-duplicated behavior:
+
+- Runner creates `ErrInvalidRequest` with human-readable details for cases such as missing profile selection and missing API-key env.
+- HTTP maps some specific invalid-request cases by checking `strings.Contains(err.Error(), ...)`.
+
+Why it matters:
+
+- User-facing HTTP error codes can drift if a runner error message is clarified.
+- This crosses package boundaries in a brittle way: HTTP should depend on stable error identity, not exact prose from `internal/usecase`.
+
+Recommended refactor:
+
+- Add narrow sentinel errors or typed invalid-request reasons in `internal/usecase`, for example profile-required and API-key-env-missing.
+- Keep HTTP response messages stable and adapter-owned.
+- Do not expose HTTP-specific error codes from the runner.
+
+Suggested tests:
+
+- HTTP error mapping tests should assert `profile_required` and `api_key_env_missing` via sentinel wrapping, not via message matching.
+- Runner tests should assert `errors.Is` for the new reason errors.
+
+Risk level: high for public API stability; low-to-medium implementation risk.
+
+## Medium-Confidence Opportunities
+
+### 1. Strict YAML Decode Setup Is Repeated
+
+Affected files/packages:
+
+- `internal/config/config.go`
+- `internal/promptdef/filesystem_repository.go`
+- `internal/profile/filesystem_repository.go`
+
+Duplicated or near-duplicated behavior:
+
+- Each package constructs a YAML decoder and enables `KnownFields(true)`.
+
+Semantic differences that may be intentional:
+
+- Prompt/profile loaders need likely-target behavior and raw `api_key` handling.
+- Config loading has its own explicit/implicit file search policy.
+
+Why it matters:
+
+- Strict decoding is an architectural invariant. A future YAML loader could forget to enable it.
+
+Recommended refactor:
+
+- Consider a tiny helper for strict YAML decode from bytes.
+- Keep package-specific error wrapping and partial ID decode logic local.
+
+Suggested tests:
+
+- Existing unknown-field tests in config, promptdef, and profile should remain.
+- Add any new YAML-consuming package with an unknown-field test.
+
+Risk level: medium.
+
+### 2. Schema Path Resolution Is Centralized, But Schema Loading Happens Twice
+
+Affected files/packages:
+
+- `internal/usecase/runner.go`
+- `internal/validate/standard_validator.go`
+
+Duplicated or near-duplicated behavior:
+
+- For `json_schema`, `Runner.Prepare` asks the validator to load the schema document for provider-level structured output.
+- Later validation resolves and compiles the same schema path again.
+
+Semantic differences that may be intentional:
+
+- Provider structured-output payload needs the raw JSON schema document.
+- Runtime validation needs a compiled schema.
+
+Why it matters:
+
+- The current behavior is correct, but schema file errors can surface at different phases and the same file is read more than once during `Run`.
+- Future caching or schema behavior changes should have one clear owner.
+
+Recommended refactor:
+
+- Keep path resolution inside `internal/validate`.
+- Consider a schema service/loader interface that can return both raw document and compiled schema from one path, only if schema-related work grows.
+- Do not add caching unless repeated schema loads become a measured cost.
+
+Suggested tests:
+
+- Keep existing nested schema path tests.
+- Add a test that `Prepare` fails before LLM call when structured-output schema cannot load.
+
+Risk level: medium.
+
+### 3. Artifact Hashing And Output Artifact Construction Are Split
+
+Affected files/packages:
+
+- `internal/artifact/reader.go`
+- `internal/usecase/runner.go`
+
+Duplicated or near-duplicated behavior:
+
+- Input artifacts and generated output artifacts both compute SHA-256 hashes and fill size/body/content-type fields.
+
+Semantic differences that may be intentional:
+
+- Input artifacts derive content type from source extension or inline default.
+- Output artifacts derive content type from prompt output format and always use the default output artifact name.
+
+Why it matters:
+
+- Hash algorithm and artifact metadata policy should remain consistent.
+
+Recommended refactor:
+
+- Consider an `artifact.Build` or `artifact.HashBody` helper only for shared hash/size construction.
+- Keep source-specific content type and naming decisions local.
+
+Suggested tests:
+
+- Existing artifact reader tests and runner output content-type tests should cover behavior.
+- Add a small helper test if a shared hash function is introduced.
+
+Risk level: medium-low.
+
+### 4. Domain Contains An Unsupported `s3` Artifact Reference Constant
+
+Affected files/packages:
+
+- `internal/domain/domain.go`
+- `internal/artifact/reader.go`
+- docs that state only `inline` and `file` are implemented
+
+Duplicated or near-duplicated behavior:
+
+- Not a duplication issue. This is a cleanup issue: `domain.ArtifactRefS3` exists, but no reader supports it and docs do not document it as implemented.
+
+Semantic differences that may be intentional:
+
+- The constant may be a placeholder for a future adapter.
+
+Why it matters:
+
+- Future-facing code outside `docs/roadmap/` can confuse maintainers and tests. It also weakens the otherwise clean "implemented behavior only" policy.
+
+Recommended refactor:
+
+- Remove the constant if no near-term S3 implementation is planned.
+- If kept, add a code comment that it is intentionally unsupported today and ensure docs continue to state only `inline` and `file` are supported.
+
+Suggested tests:
+
+- Existing unsupported artifact type tests should continue to pass.
+
+Risk level: medium-low.
+
+### 5. Test Fixture Setup Is Repeated In CLI And Use-Case Tests
+
+Affected files/packages:
+
+- `internal/adapter/cli/run_test.go`
+- `internal/usecase/runner_test.go`
+- `internal/usecase/integration_test.go`
+
+Duplicated or near-duplicated behavior:
+
+- Tests repeatedly construct temp prompt/profile dirs, write prompt/profile YAML, set up fake LLMs/readers/renderers/validators, and build runners.
+
+Semantic differences that may be intentional:
+
+- Package-local tests avoid exporting test helpers and keep each package independent.
+
+Why it matters:
+
+- Repeated setup makes behavior-preserving refactors noisier and can obscure the exact behavior under test.
+
+Recommended refactor:
+
+- Add package-local helper builders where duplication is highest, especially in CLI command tests.
+- Avoid a cross-package test utility package unless multiple packages need the same public fixture contract.
+
+Suggested tests:
+
+- This is test cleanup only; existing test assertions should remain equivalent.
+
+Risk level: low.
+
+## Boundary And Responsibility Concerns
+
+- HTTP error mapping currently depends on runner error message text. Stable reason identity should live in `internal/usecase`; HTTP status/code/message mapping should remain in `internal/adapter/http`.
+- Execution-target merge policy lives in `internal/usecase`, which fits the architecture. The concern is not placement but incomplete centralization of field mapping around that policy.
+- CLI app wiring currently constructs concrete repositories/readers/renderers/validators inline in each command. That is acceptable adapter responsibility, but repeated wiring should be centralized within the CLI adapter package.
+- Prompt/profile filesystem traversal is duplicated in two repository packages. A narrow filesystem catalog helper would fit the architecture because it would not own prompt/profile policy.
+- LLM provider request shape is properly isolated in `internal/llm`. It should remain there; do not move OpenAI/OpenRouter request-field policy into runner or prompt definitions.
+
+## Path, Key, And Naming Construction Review
+
+Path and naming construction is mostly explicit and low-risk:
+
+- Config paths are cleaned in `internal/config` and again in CLI finalization.
+- Prompt `content_file` paths are resolved relative to the prompt YAML file in `internal/promptdef`.
+- Schema paths are resolved through `internal/validate.StandardValidator`.
+- OpenAI-compatible endpoint paths use `defaults.OpenAIChatCompletionsPath`.
+- Output artifact name comes from `defaults.OutputArtifactName`.
+
+Areas needing cleanup:
+
+- Prompt/profile recursive YAML discovery and relative-path formatting should share one helper.
+- CLI path cleaning after config resolution is repeated and should be consolidated with common command settings finalization.
+- Structured schema names are derived in `internal/usecase`; this is currently one place and should stay there unless structured-output support expands.
+
+No remote keys, cache paths, manifest paths, lock files, or generated report paths exist in the current implementation.
+
+## Resolution And Catalog Review
+
+Named concept resolution is mostly consistent:
+
+- Prompt resolution uses YAML `id` and optional `version`, not file path.
+- Profile resolution uses YAML `id`, not file path.
+- Prompt/profile subdirectories are organizational only.
+- Duplicate prompt/profile IDs fail instead of choosing first match.
+- Schema resolution is explicit path-based relative to `schema_dir`; no basename search.
+- Input artifacts are resolved by `artifact.Reader`, with `inline` and `file` as implemented types.
+- Prompt required-input checks happen in the renderer.
+
+Recommended centralization:
+
+- Share only filesystem catalog mechanics between prompt/profile repositories.
+- Keep prompt ID/version, profile ID, schema path, and input resolution policies in their current owning packages.
+
+## Config And Command-Loading Review
+
+Config precedence is consistent with policy:
+
+1. built-in defaults;
+2. config file values;
+3. CLI overrides.
+
+Intentional differences:
+
+- `render` supports `--format`; `run` does not.
+- `serve` supports `--addr`; `run`/`render` do not.
+- `serve` rejects runtime model override flags.
+- `render` does not expose `--schema-dir`, even though it can prepare `json_schema` prompts through config/default schema settings.
+- CLI runtime model override flags are narrower than HTTP model override fields; this is currently documented by omission in CLI docs.
+
+Likely cleanup areas:
+
+- Common command settings finalization can be made smaller and less repetitive.
+- Runner dependency construction should be a CLI-local helper.
+- The `runConfig` flag-set booleans work, but adding more runtime override fields will continue to require touching several fields and the override condition.
+
+## State, Manifest, Or Progress Handling Review
+
+Scriptorium has no durable state, manifests, checkpoints, progress records, cache state, or resume behavior. This matches `docs/policy/architecture.md` and `docs/operations.md`.
+
+There is no drift affecting resume, retry, force, dry-run, or audit behavior because those features do not exist. Validation repair hooks exist in the runner but are not wired by CLI/HTTP today; that boundary is documented.
+
+## Refactors To Avoid
+
+- Do not introduce a generic workflow/stage engine. Scriptorium intentionally executes one prompt request.
+- Do not build a plugin architecture for prompt/profile/schema/artifact backends before another backend exists.
+- Do not replace explicit CLI parse functions with a broad command framework.
+- Do not merge CLI and HTTP adapters. Their public interfaces and failure semantics differ.
+- Do not create a generic reflection-based mapper for domain/DTO/request structs.
+- Do not centralize prompt/profile normalization into one generic YAML repository; their validation and error semantics are different.
+- Do not add schema caching or a manifest/state store as part of deduplication.
+- Do not document or implement unsupported artifact backends as cleanup.
+
+## Recommended Implementation Sequence
+
+1. Execution-target mapping cleanup
+   - Goal: reduce drift when adding profile/runtime/provider fields.
+   - Files to update: `internal/usecase`, `internal/adapter/http`, `internal/llm`, targeted tests.
+   - Acceptance criteria: all existing field mapping tests pass; a new all-fields test fails if a mapped execution field is omitted.
+   - Suggested validation: `go test ./internal/usecase ./internal/adapter/http ./internal/llm ./internal/format`.
+   - Small enough for one implementation prompt: yes.
+
+2. Prompt/profile filesystem catalog helper
+   - Goal: centralize recursive YAML discovery, path sorting, extension filtering, and relative path formatting.
+   - Files to update: prompt/profile repositories plus new narrow helper package.
+   - Acceptance criteria: existing repository tests pass unchanged; new helper tests cover nested discovery and stable ordering.
+   - Suggested validation: `go test ./internal/promptdef ./internal/profile`.
+   - Small enough for one implementation prompt: yes.
+
+3. CLI wiring and config finalization helper
+   - Goal: centralize common command settings resolution and runner construction.
+   - Files to update: `internal/adapter/cli/run.go` and CLI tests.
+   - Acceptance criteria: no flag behavior changes; run/render/serve config precedence tests pass.
+   - Suggested validation: `go test ./internal/adapter/cli`.
+   - Small enough for one implementation prompt: yes.
+
+4. Stable use-case invalid-request reasons
+   - Goal: remove HTTP dependency on runner error message substrings.
+   - Files to update: `internal/usecase/runner.go`, `internal/adapter/http/handler.go`, tests.
+   - Acceptance criteria: HTTP error codes/messages remain unchanged; runner exposes stable `errors.Is` reason errors.
+   - Suggested validation: `go test ./internal/usecase ./internal/adapter/http`.
+   - Small enough for one implementation prompt: yes.
+
+5. Test helper cleanup
+   - Goal: reduce repeated fixture construction after behavior-preserving refactors are complete.
+   - Files to update: primarily `internal/adapter/cli/run_test.go`, optionally `internal/usecase/runner_test.go`.
+   - Acceptance criteria: test intent remains clear; no cross-package helper package unless strongly justified.
+   - Suggested validation: `go test ./internal/adapter/cli ./internal/usecase`.
+   - Small enough for one implementation prompt: yes.
+
+6. Dead-code/legacy sweep
+   - Goal: remove or clearly mark unsupported placeholders such as `ArtifactRefS3`.
+   - Files to update: domain/artifact tests/docs only if needed.
+   - Acceptance criteria: docs continue to describe only implemented behavior outside roadmap.
+   - Suggested validation: `go test ./internal/domain ./internal/artifact`.
+   - Small enough for one implementation prompt: yes.
+
+## Test Strategy
+
+Tests to add before or during cleanup:
+
+- Execution target all-fields mapping tests:
+  - runner profile/default/request merge;
+  - HTTP request DTO to domain target;
+  - HTTP domain result to metadata DTO;
+  - LLM domain target to outbound JSON payload.
+- Repository catalog helper tests:
+  - recursive scan;
+  - `.yaml` and `.yml` filtering;
+  - stable sorted paths;
+  - relative clean path formatting;
+  - context cancellation if the helper preserves current behavior.
+- HTTP error mapping tests:
+  - profile-required and API-key-env-missing should rely on sentinel errors, not message text.
+- CLI wiring regression tests:
+  - `run`, `render`, and `serve` preserve config precedence and required directory checks.
+- Schema behavior tests:
+  - `Prepare` fails before LLM generation when `json_schema` structured-output schema cannot be loaded.
+
+Validation commands for cleanup work:
+
+- `go test ./internal/usecase ./internal/adapter/http ./internal/llm ./internal/format`
+- `go test ./internal/promptdef ./internal/profile`
+- `go test ./internal/adapter/cli`
+- `go test ./...` before merging broader cleanup.
+
+No automated docs or link checker is currently present.
+
+## Appendix: Findings Not Worth Acting On
+
+- Repeated `select { case <-ctx.Done(): ... }` checks are acceptable. They are local, simple, and appear at IO/loop boundaries where behavior is easy to read.
+- CLI and HTTP request validation should remain separate. Their external contracts differ, and centralizing all request validation would blur adapter responsibilities.
+- Prompt and profile validation should not be merged. They both use YAML and IDs, but their schemas, normalization rules, and error policies differ.
+- Prepared-run text formatting is verbose but intentionally presentation-specific. Avoid abstracting it until another formatter needs the same layout policy.
+- Hashing appears in artifact loading and runner metadata, but not all hashes represent the same thing. A tiny body-hash helper may be useful later; a generic hashing subsystem is not justified.
+- HTTP DTOs duplicate domain field names by design. They should remain transport-owned so JSON compatibility can evolve deliberately.
+- Config `applyConfig` and `ApplyCLIOverrides` look similar, but they apply different source labels and validation contexts. A broad merge abstraction would likely reduce clarity.