From bc099a31adac94ebd702a24d0e46e3dc05f52126 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 26 May 2026 11:05:08 -0500 Subject: [PATCH] Finish cleanup roadmap follow-through --- docs/roadmap/audit.md | 519 ---------------------- docs/roadmap/cleanup.md | 504 --------------------- internal/profile/filesystem_repository.go | 49 +- internal/profile/repository_test.go | 14 + 4 files changed, 50 insertions(+), 1036 deletions(-) delete mode 100644 docs/roadmap/audit.md delete mode 100644 docs/roadmap/cleanup.md diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md deleted file mode 100644 index 8d4f7cd..0000000 --- a/docs/roadmap/audit.md +++ /dev/null @@ -1,519 +0,0 @@ -# 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. diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md deleted file mode 100644 index ec2d41b..0000000 --- a/docs/roadmap/cleanup.md +++ /dev/null @@ -1,504 +0,0 @@ -# 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. diff --git a/internal/profile/filesystem_repository.go b/internal/profile/filesystem_repository.go index 5d3f321..5d219ee 100644 --- a/internal/profile/filesystem_repository.go +++ b/internal/profile/filesystem_repository.go @@ -53,18 +53,19 @@ func (r *filesystemRepository) GetProfile(ctx context.Context, id string) (*doma if err != nil { return nil, fmt.Errorf("failed to read profile file %s: %w", relPath, err) } + metadata := readProfileFileMetadata(data) + idMatch := fileMatch || metadata.id == id + if metadata.hasRawAPIKey { + if idMatch { + return nil, fmt.Errorf("%w: %s", ErrRawAPIKeyNotAllowed, relPath) + } + continue + } var prof domain.ExecutionProfile decoder := yaml.NewDecoder(bytes.NewReader(data)) decoder.KnownFields(true) if err := decoder.Decode(&prof); err != nil { - idMatch := fileMatch || profileFileHasID(data, id) - if strings.Contains(err.Error(), "field api_key not found") { - if idMatch { - return nil, fmt.Errorf("%w: %s", ErrRawAPIKeyNotAllowed, relPath) - } - continue - } if idMatch { return nil, fmt.Errorf("%w: %s: %v", ErrInvalidYAML, relPath, err) } @@ -106,14 +107,36 @@ type profileMatch struct { path string } -func profileFileHasID(data []byte, id string) bool { - var raw struct { - ID string `yaml:"id"` +type profileFileMetadata struct { + id string + hasRawAPIKey bool +} + +func readProfileFileMetadata(data []byte) profileFileMetadata { + var node yaml.Node + if err := yaml.NewDecoder(bytes.NewReader(data)).Decode(&node); err != nil { + return profileFileMetadata{} } - if err := yaml.NewDecoder(bytes.NewReader(data)).Decode(&raw); err != nil { - return false + if node.Kind != yaml.DocumentNode || len(node.Content) == 0 { + return profileFileMetadata{} } - return strings.TrimSpace(raw.ID) == id + mapping := node.Content[0] + if mapping.Kind != yaml.MappingNode { + return profileFileMetadata{} + } + + var metadata profileFileMetadata + for i := 0; i+1 < len(mapping.Content); i += 2 { + key := mapping.Content[i] + value := mapping.Content[i+1] + switch key.Value { + case "id": + metadata.id = strings.TrimSpace(value.Value) + case "api_key": + metadata.hasRawAPIKey = true + } + } + return metadata } func validateProfile(p *domain.ExecutionProfile) error { diff --git a/internal/profile/repository_test.go b/internal/profile/repository_test.go index 60b6038..1ecb0ce 100644 --- a/internal/profile/repository_test.go +++ b/internal/profile/repository_test.go @@ -133,6 +133,20 @@ api_key: secret } }) + t.Run("raw api_key in non-target profile is ignored", func(t *testing.T) { + writeProfileTestFile(t, filepath.Join(tmpDir, "raw-api-key-non-target.yaml"), ` +id: raw-api-key-non-target +endpoint: http://localhost:8000/v1 +model: m +api_key: secret +`) + + _, err := repo.GetProfile(ctx, "does-not-exist-with-raw-key-nearby") + if !errors.Is(err, ErrProfileNotFound) { + t.Fatalf("expected ErrProfileNotFound for non-target raw api_key file, got %v", err) + } + }) + t.Run("invalid yaml", func(t *testing.T) { _, err := repo.GetProfile(ctx, "invalid_yaml") if !errors.Is(err, ErrInvalidYAML) {