Added a roadmap to address the issues identifed by the audit

2026-05-26 08:01:33 -05:00
parent ef64966897
commit 75fa0a030a
1 changed files with 504 additions and 0 deletions
--- a/docs/roadmap/cleanup.md
+++ 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.