# 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.