diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md new file mode 100644 index 0000000..551d1a9 --- /dev/null +++ b/docs/roadmap/implementation.md @@ -0,0 +1,262 @@ +# Runtime Parameter Implementation Plan + +This plan implements the target state in `docs/roadmap/params.md`. + +Audience: LLM coding agents implementing the feature in order. Follow `docs/policy/architecture.md`, `docs/policy/development.md`, and `docs/policy/documentation.md` before changing code. + +## Constraints + +- Keep adapters thin. CLI and HTTP should capture caller intent and map it into domain request types; merge decisions belong in `internal/usecase`. +- Keep external decoding strict. Unknown YAML/JSON fields must continue to fail. +- Do not accept or emit raw API key values. +- Do not add dependencies unless there is a clear need. This feature should use the standard library plus existing dependencies. +- Do not expand the HTTP API surface beyond `POST /v1/runs`. +- Do not add provider-specific adapter packages. +- Keep each stage passing `go test ./...` before moving to the next stage. + +## Stage 1: Presence-Aware Request Overrides + +Goal: make per-request numeric execution overrides presence-aware while keeping resolved execution settings concrete. + +### Domain Changes + +1. In `internal/domain/domain.go`, add a request-only type: + +```go +type ExecutionTargetOverride struct { + Endpoint string `json:"endpoint,omitempty"` + Model string `json:"model,omitempty"` + Temperature *float64 `json:"temperature,omitempty"` + MaxTokens *int `json:"max_tokens,omitempty"` + TopP *float64 `json:"top_p,omitempty"` + TimeoutSeconds *int `json:"timeout_seconds,omitempty"` + ServiceTier string `json:"service_tier,omitempty"` + ReasoningEffort string `json:"reasoning_effort,omitempty"` + APIKeyEnv string `json:"api_key_env,omitempty"` + ExtraParams map[string]any `json:"extra_params,omitempty"` +} +``` + +2. Change `domain.RunRequest.Execution` from `*ExecutionTarget` to `*ExecutionTargetOverride`. +3. Change `ExecutionProfile.ExtraParams` and `ExecutionTarget.ExtraParams` from `map[string]string` to `map[string]any`. +4. Keep `ExecutionTarget` concrete. It represents the resolved effective runtime target after defaults, profile, and request overrides are merged. + +### Runner Changes + +1. Update `internal/usecase/runner.go` so profile values still merge over built-in defaults and request overrides merge over that result. +2. Keep the existing concrete profile merge semantics for profile numeric fields. +3. Add a separate request override merge path that uses pointer presence: + - `nil` numeric pointer means omitted; preserve the current value. + - non-nil numeric pointer means explicit override, even when the value is `0`. +4. Validate request override numeric values before or during merge: + - `temperature`: `0 <= value <= 2` + - `max_tokens`: `value >= 0` + - `top_p`: `0 <= value <= 1` + - `timeout_seconds`: `value >= 0` +5. Preserve existing validation after merge: + - effective endpoint required + - effective model required + - `api_key_env`, when set, must name a non-empty environment variable +6. Preserve secret handling. The resolved API key value must never be stored in `PreparedRun`, `RunResult`, logs, or HTTP responses. + +### CLI Changes + +1. Update `internal/adapter/cli/run.go` request construction to build `domain.ExecutionTargetOverride`. +2. Use the existing `flagWasSet` booleans to populate numeric pointers only when the user provided the flag. +3. Required behavior: + - omitted `--temperature` preserves profile/default temperature; + - `--temperature 0` explicitly sets temperature to zero; + - omitted `--top-p` preserves profile/default top-p; + - `--top-p 0` explicitly sets top-p to zero; + - omitted `--max-tokens` preserves profile/default max tokens; + - `--max-tokens 0` explicitly sets max tokens to zero; + - omitted `--timeout` preserves profile/default timeout; + - `--timeout 0s` explicitly sets timeout seconds to zero. +4. Do not add new CLI flags in this stage. + +### HTTP Changes + +1. Update `internal/adapter/http/dto.go` so numeric model override fields are pointers: + - `Temperature *float64` + - `MaxTokens *int` + - `TopP *float64` + - `TimeoutSeconds *int` +2. Update DTO mapping in `internal/adapter/http/handler.go` to build `domain.ExecutionTargetOverride`. +3. Preserve strict JSON decoding and existing error mapping. +4. Required behavior: + - omitted numeric JSON fields preserve profile/default values; + - explicit numeric zero JSON fields override profile/default values. + +### Tests + +Add or update tests in: + +- `internal/usecase/runner_test.go` +- `internal/adapter/cli/run_test.go` +- `internal/adapter/http/handler_test.go` + +Required test coverage: + +- Runner preserves profile value when request numeric override is omitted. +- Runner applies explicit zero request override for `temperature`. +- Runner applies explicit zero request override for `top_p`. +- Runner applies explicit zero request override for `max_tokens`. +- Runner applies explicit zero request override for `timeout_seconds`. +- Invalid request override ranges fail as invalid request errors. +- CLI `--temperature 0` reaches effective settings as zero. +- HTTP `"temperature": 0` reaches effective settings as zero. +- HTTP omitted `temperature` preserves profile/default value. + +### Verification + +Run: + +```bash +go test ./... +``` + +## Stage 2: JSON-Compatible `extra_params` + +Goal: allow provider-specific parameters to carry JSON-compatible values throughout profile, HTTP, prepared output, metadata, and LLM request construction. + +### Domain And Loader Changes + +1. Complete all compile fixes from changing `ExtraParams` to `map[string]any`. +2. Ensure `internal/profile/filesystem_repository.go` continues to decode profiles strictly while allowing nested JSON-compatible values under `extra_params`. +3. Add profile repository tests for `extra_params` containing: + - string + - number + - boolean + - nested object or array +4. Ensure formatter output remains deterministic: + - keep sorting `extra_params` keys in `internal/format/prepared_run.go`; + - render non-string values with stable JSON encoding in text output. +5. Preserve JSON formatter behavior through normal `encoding/json` output. + +### HTTP Changes + +1. Change HTTP model override `ExtraParams` to `map[string]any`. +2. Add handler tests proving HTTP accepts JSON-compatible `extra_params` values. +3. Preserve strict rejection of unknown fields and raw API-key payload fields. + +### Verification + +Run: + +```bash +go test ./... +``` + +## Stage 3: Outbound Serialization + +Goal: serialize `reasoning_effort` and `extra_params` to the OpenAI-compatible chat-completions request. + +### LLM Adapter Changes + +1. In `internal/llm/openai_compatible_client.go`, add first-class outbound support for `reasoning_effort`. +2. Add `extra_params` support by flattening `domain.ExecutionTarget.ExtraParams` into additional top-level JSON request fields. +3. Implement reserved-field collision checks before the HTTP request is made. +4. Reserved keys must include: + - `model` + - `session_id` + - `messages` + - `temperature` + - `max_tokens` + - `top_p` + - `service_tier` + - `reasoning_effort` + - `response_format` +5. Reject empty `extra_params` keys. +6. Ensure each `extra_params` value can be marshaled as JSON. If marshaling fails, return `ErrInvalidRequest` with context. +7. Keep existing request behavior unchanged when `reasoning_effort` and `extra_params` are unset. + +### Recommended Implementation Shape + +Use a custom marshal path for the outbound chat request rather than string manipulation. + +One acceptable shape: + +- Add `ReasoningEffort string` and `ExtraParams map[string]any` to the internal `openAIChatRequest`. +- Add a helper that converts `openAIChatRequest` into `map[string]any`, inserts first-class fields when set, then inserts `ExtraParams` after collision validation. +- Marshal that map with `encoding/json`. + +Do not construct outbound JSON with manual string concatenation. + +### Tests + +Update `internal/llm/openai_compatible_client_test.go`. + +Required test coverage: + +- outbound JSON includes `reasoning_effort` when set; +- outbound JSON omits `reasoning_effort` when unset; +- outbound JSON includes string, number, boolean, object, and array `extra_params`; +- reserved `extra_params` keys fail before provider call; +- empty `extra_params` keys fail before provider call; +- existing message, cache-control, service-tier, response-format, and usage parsing tests continue to pass. + +### Verification + +Run: + +```bash +go test ./... +``` + +## Stage 4: Documentation And Examples + +Goal: move implemented behavior from roadmap to canonical docs after code is complete. + +Update only after Stages 1 through 3 are implemented. + +### Required Docs + +Update: + +- `docs/config.md` +- `docs/cli.md` +- `docs/integrations/http-api.md` +- `docs/integrations/openai-compatible-chat.md` +- `docs/internal/runner.md` +- `docs/internal/adapters.md` + +Required documentation content: + +- `reasoning_effort` is serialized outbound when set. +- `extra_params` serializes as provider-specific top-level outbound JSON fields. +- `extra_params` supports JSON-compatible values. +- reserved `extra_params` fields are rejected. +- per-request numeric overrides distinguish omitted values from explicit zero values. +- CLI explicit zero behavior for existing numeric flags. +- HTTP explicit zero behavior for model override numeric fields. +- no raw API-key values are accepted or emitted. + +### Examples + +Update examples only if needed to keep them accurate and runnable. + +If adding an `extra_params` example, keep it secret-free and simple. Prefer a harmless provider-routing example over a vendor-specific feature that requires special credentials. + +### Verification + +Run: + +```bash +go test ./... +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 +``` + +## Final Checks + +Before considering the feature complete: + +1. Confirm `git diff` contains only intended code, test, doc, and example changes. +2. Confirm all non-roadmap docs describe implemented behavior only. +3. Confirm no output path exposes raw API key values. +4. Confirm `go test ./...` passes. +5. Confirm the render smoke command passes. diff --git a/docs/roadmap/params.md b/docs/roadmap/params.md new file mode 100644 index 0000000..bf7c4e1 --- /dev/null +++ b/docs/roadmap/params.md @@ -0,0 +1,96 @@ +# Runtime Parameter Feature Roadmap + +This roadmap defines the target behavior for runtime model parameters. + +Current behavior has two limitations: + +- `reasoning_effort` and `extra_params` are parsed into effective execution settings but are not serialized into outbound OpenAI-compatible chat-completions requests. +- Per-request numeric execution overrides use zero-value merge semantics, so callers cannot reliably override a profile value with an explicit zero such as `temperature: 0`. + +The implementation plan for this feature lives in `docs/roadmap/implementation.md`. + +## Target State + +Scriptorium should preserve the existing separation between prompt definitions, execution profiles, and per-request execution overrides while making runtime parameter behavior explicit and predictable. + +Expected end state: + +- Effective execution settings remain visible in prepared-run output, run metadata, and HTTP metadata without exposing raw secret values. +- `reasoning_effort` is treated as a first-class effective execution setting and is serialized to the outbound OpenAI-compatible request when set. +- `extra_params` supports provider-specific OpenAI-compatible request fields. +- `extra_params` is serialized as additional top-level outbound JSON fields. +- `extra_params` values support JSON-compatible scalar, object, and array values. +- `extra_params` cannot override first-class outbound request fields. +- Per-request numeric overrides preserve caller intent, including explicit zero values. +- Omitted per-request numeric overrides continue to inherit the selected profile and built-in defaults. +- External decoding remains strict for config, prompt, profile, and HTTP request payloads. + +## Policy Decisions + +### `extra_params` + +`extra_params` should serialize as additional top-level outbound JSON fields in the OpenAI-compatible chat-completions request. + +Reasoning: + +Most OpenAI-compatible providers expose vendor-specific chat-completions parameters as top-level fields. This keeps Scriptorium's adapter compatible with that ecosystem without adding first-class fields for every provider option. + +`extra_params` must not silently override Scriptorium-owned fields. Reserved outbound fields include at least: + +- `model` +- `session_id` +- `messages` +- `temperature` +- `max_tokens` +- `top_p` +- `service_tier` +- `reasoning_effort` +- `response_format` + +If a caller supplies a reserved key through `extra_params`, Scriptorium should fail before making the outbound HTTP request. + +`extra_params` should use JSON-compatible values rather than only strings. + +Reasoning: + +Provider-specific parameters commonly need booleans, numbers, objects, or arrays. String-only values would force awkward encoding and would likely require a later compatibility break. + +### Presence-Aware Overrides + +Per-request execution overrides should use a presence-aware type with pointer fields for optional numeric values. + +Reasoning: + +The resolved execution target should remain a concrete value used by prepared runs, generated requests, and metadata. Optionality matters at the request boundary, not after the runner has resolved the effective target. + +This keeps adapter and merge logic precise while avoiding nil checks in formatter, metadata, and LLM serialization paths. + +## Scope + +In scope: + +- Runtime merge behavior for per-request execution overrides. +- HTTP model override decoding for explicit zero numeric values. +- CLI execution override handling for explicit zero numeric flags. +- Outbound serialization of `reasoning_effort`. +- Outbound serialization of JSON-compatible `extra_params`. +- Tests and documentation for the changed implemented behavior. + +Out of scope: + +- Expanding the HTTP API beyond `POST /v1/runs`. +- Adding built-in HTTP authentication or authorization. +- Adding durable run state, run history, or multi-step orchestration. +- Adding broad provider-specific adapter packages. +- Adding new CLI flags for every provider-specific parameter. + +## Acceptance Criteria + +- A profile containing `reasoning_effort: medium` produces an outbound request with `reasoning_effort`. +- HTTP callers can pass `reasoning_effort` through the existing `model` override object and have it appear outbound. +- A profile or HTTP request containing JSON-compatible `extra_params` produces outbound top-level JSON fields according to the reserved-field policy. +- Reserved `extra_params` collisions fail before the outbound provider call. +- CLI callers can pass `--temperature 0` and observe `temperature: 0` in rendered/effective settings and outbound requests. +- HTTP callers can send `"temperature": 0` and observe the same behavior. +- Omitting `temperature` continues to preserve the selected profile/default value. +- Raw API key values remain unsupported in config, profiles, CLI flags, HTTP payloads, logs, and rendered output.