From 75fa0a030af1dc085c2bd43569e931c8578f0454 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 26 May 2026 08:01:33 -0500 Subject: [PATCH] Added a roadmap to address the issues identifed by the audit --- docs/roadmap/cleanup.md | 504 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 504 insertions(+) create mode 100644 docs/roadmap/cleanup.md diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md new file mode 100644 index 0000000..ec2d41b --- /dev/null +++ b/docs/roadmap/cleanup.md @@ -0,0 +1,504 @@ +# Cleanup Implementation Roadmap + +## Purpose + +This roadmap turns the findings in [audit.md](audit.md) into a staged, decision-complete cleanup plan for Scriptorium. + +Audience: LLM coding agents implementing the cleanup in order. + +Controlling policies: + +- [Documentation policy](../policy/documentation.md) +- [Architecture policy](../policy/architecture.md) +- [Development guide](../policy/development.md) + +## Global Implementation Rules + +- Implement stages in order. +- Keep public CLI flags, HTTP request/response shapes, config precedence, prompt/profile ID semantics, and validation behavior stable unless a stage explicitly says otherwise. +- Keep adapters thin and use-case policy in `internal/usecase`. +- Prefer explicit helpers over reflection, generic workflow abstractions, or broad framework-style rewrites. +- Update non-roadmap docs only when implemented behavior changes or when a stale implemented-behavior statement is found during a stage. +- Do not document future behavior outside `docs/roadmap/`. +- Run the stage-specific tests before moving to the next stage. +- Run `go test ./...` after the final stage. + +## Stage 1: Execution Target Mapping Cleanup + +### Goal + +Reduce drift when adding or changing execution/runtime fields such as `service_tier`, `api_key_env`, `reasoning_effort`, or future provider request keys. + +### Scope + +Update only explicit execution-target mapping and serialization paths. Do not add new CLI flags or new provider features. + +### Implementation + +In `internal/usecase`: + +- Keep execution-target merge policy in `internal/usecase`. +- Add focused helper coverage around `resolveExecutionTarget`, `mergeExecutionTarget`, and profile-to-target conversion. +- Keep the existing semantics: + - built-in defaults first; + - profile values override defaults; + - request overrides override profile values; + - zero numeric values do not override; + - empty/whitespace string values do not override; + - non-empty `ExtraParams` replaces the previous map with a copy. + +In `internal/adapter/http`: + +- Add local helper functions: + - `executionTargetFromModelOverrideDTO(*modelOverrideRequestDTO) *domain.ExecutionTarget` + - `modelParamsDTOFromExecutionTarget(domain.ExecutionTarget) modelParamsDTO` +- Use those helpers in `handler.go`. +- Keep DTO types unexported and transport-owned. +- Keep HTTP response field names and omission behavior unchanged. + +In `internal/llm`: + +- Add a local helper such as `openAIChatRequestFromGenerateRequest(req domain.GenerateRequest, defaultModel string) (openAIChatRequest, error)` or an equivalent small function. +- Keep endpoint construction, HTTP client timeout handling, API-key environment lookup, and response parsing in `Generate`. +- Keep outbound serialization behavior unchanged: + - send `model` and `messages`; + - send `temperature`, `max_tokens`, `top_p`, and `service_tier` only when currently sent; + - send `response_format` only when structured output is present; + - do not serialize `reasoning_effort` or `extra_params`. + +Do not: + +- use reflection to copy fields; +- move HTTP DTOs into `internal/domain`; +- add generic mapper packages; +- change prepared-run JSON tags. + +### Tests + +Add or update tests so an all-fields `domain.ExecutionTarget` catches omissions. + +Required tests: + +- Runner merge/profile conversion: + - profile values populate all supported execution fields; + - runtime overrides beat profile values for all overrideable fields; + - empty string overrides do not erase profile values; + - empty `ExtraParams` does not erase profile values. +- HTTP adapter: + - request `model` object maps every supported field into `RunRequest.Execution`; + - response `metadata.model_params` includes every supported field according to current DTO tags. +- LLM client: + - outbound JSON includes every serialized execution field; + - outbound JSON omits `service_tier` when empty; + - outbound JSON still omits `reasoning_effort` and `extra_params`. + +### Validation + +Run: + +```bash +go test ./internal/usecase ./internal/adapter/http ./internal/llm ./internal/format +``` + +### Acceptance Criteria + +- No public behavior changes. +- Adding a new execution target field later has obvious mapping/test locations. +- Existing HTTP and LLM behavior remains stable. +- Stage is small enough for one implementation prompt. + +## Stage 2: Prompt/Profile Filesystem Catalog Helper + +### Goal + +Centralize shared recursive YAML discovery mechanics while preserving prompt/profile-specific validation and error behavior. + +### Scope + +Create a narrow helper package for filesystem catalog mechanics only. + +Recommended package: + +- `internal/filecatalog` + +### Implementation + +Add helper functions with explicit, small responsibilities: + +- recursively find YAML files under a root directory; +- honor context cancellation during walking; +- accept `.yaml` and `.yml`; +- return stable sorted full paths; +- compute clean relative paths from a root; +- return filename stems with `.yaml`/`.yml` stripped. + +Use the helper in: + +- `internal/promptdef/filesystem_repository.go` +- `internal/profile/filesystem_repository.go` + +Preserve existing behavior: + +- prompt/profile lookup uses YAML `id`, not file path; +- subdirectories are organizational only; +- duplicate prompt/profile IDs are invalid; +- malformed likely-target files still surface errors; +- relative nested paths still appear in errors; +- prompt `content_file` resolution remains relative to the prompt YAML file; +- prompt/profile strict YAML and validation stay in their existing packages. + +Do not: + +- create a generic repository framework; +- merge prompt and profile normalization; +- move prompt/profile domain policy into the helper; +- change error messages except for unavoidable wording caused by helper extraction. + +### Tests + +Add tests for `internal/filecatalog`: + +- nested YAML discovery; +- `.yaml` and `.yml` accepted; +- non-YAML files ignored; +- returned paths sorted deterministically; +- relative path formatting works for nested files; +- filename stem stripping handles both extensions. + +Keep existing prompt/profile repository tests passing. + +### Validation + +Run: + +```bash +go test ./internal/filecatalog ./internal/promptdef ./internal/profile +``` + +### Acceptance Criteria + +- Prompt/profile repository tests pass without behavior expectation changes. +- Shared filesystem scanning logic exists in one place. +- Prompt/profile packages still own their own validation and normalization. +- Stage is small enough for one implementation prompt. + +## Stage 3: CLI Wiring And Settings Finalization Cleanup + +### Goal + +Reduce duplicated command setup while preserving each command's public flag surface and behavior. + +### Scope + +Clean up `internal/adapter/cli` only, except for tests. + +### Implementation + +Add CLI-local helpers. Recommended helpers: + +- `commonCommandSettings` or similar struct containing resolved `promptDir`, `profileDir`, `schemaDir`, `serverAddr`, and `defaultRenderFormat` where applicable. +- `resolveCommonSettings(fs *flag.FlagSet, configPath string, overrides appconfig.CLIOverrides) (commonCommandSettings, error)`. +- `validateRequiredLibraryDirs(promptDir, profileDir string) error`. +- `newRunner(promptDir, profileDir, schemaDir string, llmClient llm.Client) *usecase.Runner`. +- Optionally `newOpenAIClient() (*llm.OpenAICompatibleClient, error)` if it removes exact duplication without obscuring command behavior. + +Preserve command differences: + +- `run` exposes runtime model override flags and `--schema-dir`; +- `render` exposes runtime model override flags and `--format`, but not `--schema-dir`; +- `serve` exposes `--addr` and `--schema-dir`, but no runtime model override flags; +- `render` default format comes from `defaults.render_format` unless `--format` is set; +- deprecated `--prompt-id` and `--profile-id` aliases remain accepted. + +Keep existing parse functions: + +- `parseRunArgs` +- `parseRenderArgs` +- `parseServeArgs` + +Do not: + +- replace the standard library `flag` package; +- introduce a command framework; +- make `serve` accept runtime model override flags; +- change error prefixes such as `run parse error`, `render parse error`, or `serve parse error`; +- change CLI output behavior. + +### Tests + +Required regression tests: + +- `run`, `render`, and `serve` still apply config precedence correctly. +- Missing effective `prompt_dir` and `profile_dir` still return the same guidance. +- `render` still uses config default render format and explicit `--format` override. +- `serve` still rejects runtime model override flags. +- `run` and `render` still build equivalent runtime override requests for shared flags. + +### Validation + +Run: + +```bash +go test ./internal/adapter/cli +``` + +### Acceptance Criteria + +- No CLI flag, output, exit-code, or precedence changes. +- Runner dependency construction is centralized inside the CLI adapter. +- Command-specific behavior remains easy to read. +- Stage is small enough for one implementation prompt. + +## Stage 4: Stable Use-Case Error Reasons For HTTP Mapping + +### Goal + +Remove HTTP error mapping's dependency on runner error message substrings. + +### Scope + +Change error identity, not public HTTP error responses. + +### Implementation + +In `internal/usecase`: + +- Add stable sentinel errors for invalid-request reasons that HTTP currently distinguishes by message text. +- Required sentinels: + - missing profile selection, for the case where neither request profile nor prompt `default_profile` is available; + - missing API-key environment value, for the case where `api_key_env` is set but the named environment variable is unset or empty. +- Wrap these sentinels with `ErrInvalidRequest` so existing broad invalid-request checks keep working. +- Preserve clear human-readable runner errors. + +In `internal/adapter/http`: + +- Replace `strings.Contains(err.Error(), ...)` checks for these cases with `errors.Is`. +- Keep current HTTP status codes, error codes, and response messages: + - `400 profile_required`; + - `400 api_key_env_missing`. + +Do not: + +- expose HTTP-specific error codes from `internal/usecase`; +- change the HTTP JSON error body shape; +- remove broad fallback handling for `usecase.ErrInvalidRequest`. + +### Tests + +Required tests: + +- Runner tests assert `errors.Is(err, usecase.ErrProfileRequired)` or the chosen sentinel name for missing profile selection. +- Runner tests assert `errors.Is(err, usecase.ErrAPIKeyEnvMissing)` or the chosen sentinel name for missing API-key environment value. +- HTTP handler tests still assert unchanged status/code/message for both cases. +- HTTP handler tests should not construct errors by relying on exact runner prose for these two cases. + +### Validation + +Run: + +```bash +go test ./internal/usecase ./internal/adapter/http +``` + +### Acceptance Criteria + +- HTTP mapping no longer depends on runner message substrings for the two distinguished invalid-request cases. +- Public HTTP behavior is unchanged. +- Runner errors remain clear in CLI output. +- Stage is small enough for one implementation prompt. + +## Stage 5: Schema Failure Regression Coverage + +### Goal + +Protect the current structured-output invariant before future schema cleanup: `Prepare` must load a `json_schema` document before any LLM call. + +### Scope + +Add regression coverage only. Do not add schema caching or change schema loading architecture in this stage. + +### Implementation + +In `internal/usecase/runner_test.go` or an appropriate package test: + +- Add a test where a prompt uses `validation_mode: json_schema` with a missing or failing schema document. +- Assert `Runner.Prepare` fails with `ErrValidation`. +- Assert no LLM call is made for `Runner.Run` when structured-output schema loading fails. + +If existing tests already cover part of this behavior, consolidate assertions without making the test suite harder to read. + +Do not: + +- cache compiled schemas; +- change `validate.StandardValidator` behavior; +- introduce a schema service abstraction. + +### Validation + +Run: + +```bash +go test ./internal/usecase ./internal/validate +``` + +### Acceptance Criteria + +- Missing structured-output schema fails before generation. +- Existing JSON Schema validation behavior is unchanged. +- Stage is small enough for one implementation prompt. + +## Stage 6: Test Fixture Cleanup + +### Goal + +Reduce repeated test setup after behavior-preserving production refactors are complete. + +### Scope + +Prefer package-local test helpers. Avoid cross-package test utility packages unless a helper is needed by more than two packages and represents a stable public fixture contract. + +### Implementation + +In `internal/adapter/cli/run_test.go`: + +- Consolidate repeated temp prompt/profile/input setup into local helper functions. +- Keep helper names behavior-focused, for example: + - `newCLITestLibrary` + - `writePromptFileWithDefaultProfile` + - `writeProfileFile` + - `runCLICommand` +- Do not hide assertions inside helpers unless the assertion is truly setup validation. + +In `internal/usecase/runner_test.go`: + +- Keep existing fake interfaces package-local. +- Remove only high-volume duplication that obscures test intent. + +Do not: + +- move package-private fake types into production code; +- create a broad `internal/testutil` package unless a later cleanup stage proves it necessary; +- rewrite tests into table-driven form when cases have meaningfully different setup. + +### Validation + +Run: + +```bash +go test ./internal/adapter/cli ./internal/usecase +``` + +### Acceptance Criteria + +- Test intent is at least as clear as before. +- No production behavior changes. +- Test fixture setup has less repeated boilerplate in CLI tests. +- Stage is small enough for one implementation prompt. + +## Stage 7: Unsupported Placeholder Sweep + +### Goal + +Remove code that suggests unimplemented artifact behavior outside roadmap documentation. + +### Scope + +Remove unsupported placeholders only when they are not needed by current tests or public docs. + +### Implementation + +Remove `domain.ArtifactRefS3` from `internal/domain/domain.go` unless new evidence shows it is intentionally needed by implemented code. + +Preserve current behavior: + +- supported artifact reference types remain `inline` and `file`; +- unsupported artifact reference types still return `artifact.ErrUnsupportedRefType`; +- docs continue to describe only `inline` and `file` outside roadmap files. + +Update tests only if they reference the removed constant. Prefer testing unsupported artifact behavior with a literal custom type such as `domain.ArtifactRefType("s3")` or `domain.ArtifactRefType("unsupported")`. + +Do not: + +- add S3 support; +- document S3 as implemented; +- add future-backend placeholders elsewhere. + +### Validation + +Run: + +```bash +go test ./internal/domain ./internal/artifact +``` + +### Acceptance Criteria + +- Unsupported placeholder constant is removed. +- Unsupported artifact-type behavior remains covered. +- No non-roadmap doc claims unimplemented artifact support. +- Stage is small enough for one implementation prompt. + +## Stage 8: Final Verification And Documentation Alignment + +### Goal + +Confirm the cleanup sequence preserved behavior and documentation accuracy. + +### Implementation + +Run the full test suite: + +```bash +go test ./... +``` + +Run the maintained render smoke command: + +```bash +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 +``` + +Search for stale or unsupported terms: + +```bash +rg -n "ArtifactRefS3|s3|strings\\.Contains\\(err\\.Error\\(\\)|TODO|future|planned" internal docs README.md examples +``` + +Review results manually: + +- `s3` should not appear as an implemented artifact type. +- `strings.Contains(err.Error())` should not be used for stable use-case reason mapping. +- Any `future` or `planned` wording outside `docs/roadmap/` must describe current boundaries, not aspirational behavior. + +Update docs only if cleanup changed implemented behavior or if the search reveals stale implemented-behavior docs. + +### Acceptance Criteria + +- Full test suite passes. +- Maintained render smoke command succeeds. +- No stale unsupported artifact placeholder remains. +- Non-roadmap docs describe implemented behavior only. +- Working tree contains only intentional cleanup changes. + +## Deferred Work + +Do not implement these during the staged cleanup unless a later audit makes them high-confidence: + +- schema caching or a combined raw/compiled schema service; +- artifact hash/build helper beyond a small helper introduced opportunistically during touched code; +- generic YAML repository framework; +- generic CLI command framework; +- plugin architecture for future prompt/profile/schema/artifact backends; +- durable state, manifests, checkpoints, or resume behavior. + +## Completion Criteria + +The cleanup roadmap is complete when all stages have been implemented in order, the final verification passes, and the resulting code still satisfies: + +- `Runner.Run` reuses `Runner.Prepare`; +- CLI and HTTP adapters instantiate `Runner` without a repairer; +- unknown config/prompt/profile YAML and HTTP JSON fields are rejected; +- raw API key values are not accepted or emitted; +- prompt/profile subdirectories remain organizational only; +- schema paths remain explicit and relative to `schema_dir` when not absolute; +- public CLI and HTTP behavior remains stable.