From 3074b3165a55dab1e634f1272410590c650cf372 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 27 Jul 2026 19:05:28 -0500 Subject: [PATCH] Complete step 3 of the migration plan and clean up the implemented roadmap --- docs/roadmap/implementation.md | 340 --------------------------------- docs/roadmap/step3.md | 178 ----------------- engine_test.go | 22 ++- 3 files changed, 19 insertions(+), 521 deletions(-) delete mode 100644 docs/roadmap/implementation.md delete mode 100644 docs/roadmap/step3.md diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md deleted file mode 100644 index 5cb06fa..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -1,340 +0,0 @@ -# Migration Step 3 Implementation Plan - -## Status - -Completed on 2026-07-27. The framework characterization baseline is complete, -and Migration Step 4 is the next planned work. - -## Objective - -Implement the target state in the -[Step 3 framework-characterization roadmap](step3.md): make -Promptkit-destined tests independent of Scriptorium-owned executable examples, -close the identified public contract gaps, and leave every migration-critical -behavior with a clear test owner. - -Follow the accepted ownership boundary in -[ADR 0002](../adr/0002-split-promptkit-from-scriptorium.md) and the test-value -and non-duplication rules in the [testing policy](../policy/testing.md). - -## Constraints - -- Execute the stages in order and satisfy each gate before proceeding. -- Do not change production behavior, public types, method signatures, package - boundaries, or application interfaces. -- Limit implementation changes to testdata, tests, and roadmap status. -- Keep all default-suite tests deterministic, offline, and independent of real - credentials. -- Preserve unrelated working-tree changes. -- Do not copy the complete `examples/` tree into testdata. -- Do not add tests merely to raise statement coverage. -- Prefer extending or consolidating an existing test over adding a parallel - test for the same behavior. - -## Stage 1: Create The Framework Contract Corpus - -Create this Promptkit-destined fixture tree: - -```text -testdata/framework/ -├── fixtures/ -│ ├── glossary.yml -│ └── transcript.md -├── profiles/ -│ ├── contract-fast.yaml -│ └── contract-quality.yaml -├── prompts/ -│ ├── contract.markdown_summary.system.md -│ ├── contract.markdown_summary.user.md -│ ├── contract.markdown_summary.yaml -│ ├── contract.structured_events.system.md -│ ├── contract.structured_events.user.md -│ └── contract.structured_events.yaml -└── schemas/ - └── structured_events.schema.json -``` - -Define the corpus as follows: - -- `contract.markdown_summary` - - version `1.0.0`; - - default profile `contract-fast`; - - required `transcript` input and optional `glossary` input; - - system and user messages loaded through relative `content_file` paths; and - - Markdown output with basic validation and no repair attempts. -- `contract.structured_events` - - version `1.0.0`; - - default profile `contract-quality`; - - the same two inputs; - - system and user messages loaded through relative `content_file` paths; and - - JSON output with JSON Schema validation through - `structured_events.schema.json` and no repair attempts. -- `contract-fast` - - endpoint `http://localhost:8000/v1`; - - model `contract-fast-model`; - - temperature `0.2`, max tokens `500`, top-p `1`, and timeout `90`; and - - no credential requirement. -- `contract-quality` - - endpoint `http://localhost:8000/v1`; - - model `contract-quality-model`; - - temperature `0.1`, max tokens `1000`, top-p `0.9`, and timeout `120`; and - - no credential requirement. -- The schema requires an object containing an `events` array. Keep it small but - valid for the same JSON Schema draft currently used by the maintained - structured-output example. -- The transcript and glossary contain short synthetic values suitable for - rendering and hash assertions. They must contain no private or real-world - data. - -Do not add application configuration, HTTP requests, executable scripts, or -provider credentials to this corpus. - -In `engine_test.go`, add shared constants for the contract root, prompt IDs, -profile IDs, and fixture paths. Replace -`TestPrepareWorksWithExampleDirectoriesAndFileInputs` with -`TestPrepareWorksWithFrameworkContractCorpus`, using table cases for the -ordinary and structured prompts. Construct the public engine from the new -directories, use the new file artifacts, and assert that each prompt renders -with its intended profile. For the structured case, also assert that -`PreparedRun.StructuredOutput` contains the loaded JSON Schema specification. -This test is the real-parser acceptance check for the corpus. - -Run: - -```bash -go test . -go test ./internal/promptdef ./internal/profile ./internal/validate -``` - -### Stage 1 Gate - -- Every corpus file loads through its real owning parser. -- Relative prompt content resolves from the prompt file location. -- The structured schema decodes through the real validator's schema-document - loader and appears in the prepared structured-output specification. -- No production or executable-example file changed. - -## Stage 2: Move Framework Tests Onto Framework-Owned Fixtures - -Update `engine_test.go` to use the new corpus. - -1. Reuse the contract constants introduced in Stage 1. -2. Rename: - - `newExampleEngine` to `newContractEngine`; - - `newExampleEngineWithOptions` to `newContractEngineWithOptions`; and - - `exampleConfig` to `contractConfig`. -3. Make `contractConfig` point at the corpus prompt, profile, and schema - directories. -4. Replace each `./examples/...` dependency in `engine_test.go` with the - matching contract fixture or a purpose-built `t.TempDir`, - `fstest.MapFS`, or in-memory profile. -5. Update assertions that intentionally identify fixture prompt IDs, profile - IDs, models, rendered text, or hashes to the contract values. Do not change - assertions that express independent public behavior. -6. Rename tests whose names say “example” when they now exercise contract - testdata. - -Add `TestEngineRunWithDirectorySourcesAndFileInputs` to `engine_test.go`. It -must assemble the public engine from the contract prompt, profile, schema, and -file-artifact directories; inject a deterministic `LLMClient`; run -`contract.structured_events`; and assert: - -- the prompt-selected `contract-quality` profile; -- a non-empty run ID, prompt hash, rendered-prompt hash, and both input hashes; -- provider-level JSON Schema structured output on the captured generation - request; -- passed JSON Schema validation; -- `application/json` artifact content; -- preserved raw output and injected token usage; and -- non-zero ordered timestamps with non-negative duration. - -Move the unique protection from -`internal/usecase/integration_test.go` into this public test, then delete that -internal integration test. Do not retain both assembled workflows. - -The only test references to `examples/` after this stage should be -Scriptorium-owned adapter or maintained-example checks. In particular, this -command must return no matches: - -```bash -rg -n 'examples/' engine_test.go internal/usecase -``` - -Run: - -```bash -go test . -go test ./internal/usecase -``` - -### Stage 2 Gate - -- Public and framework-internal tests pass without reading Scriptorium-owned - executable examples. -- The new public assembled workflow subsumes the deleted internal integration - test. -- Scriptorium's maintained examples are unchanged. -- No production file changed. - -## Stage 3: Consolidate And Complete Public Characterization - -### Execution-Setting Precedence - -Add a table-driven `TestEngineExecutionSettingPrecedence` in `engine_test.go`. -Run through the public engine with an injected recording `LLMClient`. Cover -these cases: - -1. a profile with zero-valued optional settings receives the documented - framework numeric defaults; -2. non-zero profile settings replace those defaults; -3. request settings replace profile settings; and -4. explicit request numeric zero replaces non-zero profile settings. - -Across the table, verify the effective endpoint, model, temperature, -max-tokens, top-p, timeout, service tier, reasoning effort, API-key environment -name, and `extra_params` where the relevant layer supplies them. Verify -`ExecutionTargetPresence` is false for omitted numeric request settings and -true for every explicitly supplied numeric setting, including zero. - -Use relationally distinct values for each layer. Assert literal framework -defaults only in the framework-default case because those values are part of -the documented public contract. Use `t.Setenv` for every non-empty profile or -request API-key environment name, assert only the environment-variable names, -and never expose the test secret values. - -Consolidate overlapping assertions: - -- remove `TestPreparePreservesExplicitZeroExecutionOverrides` once the new - table protects that behavior; and -- retain `TestRunPassesPreparedRequestToInjectedLLMClient` for rendered prompt, - direct-key, and structured-output handoff, but remove execution-precedence - assertions now owned by the table. - -### Caller Cancellation - -Add `TestEngineRunPropagatesCallerCancellation` using the built-in -OpenAI-compatible client and a custom `RoundTripper`. - -- The transport must signal through a channel when `RoundTrip` begins. -- It must block on `req.Context().Done()` and return the context error. -- Start `Engine.Run` in a goroutine, wait for the transport signal, cancel the - caller context, and collect the result through a buffered channel. -- Assert that the call returns and the error matches `ErrLLMGenerate`. -- Do not use sleeps or elapsed-time assertions. - -### Injected Nil Response - -Add a valid-request case to `TestPublicErrorsSupportErrorsIs` whose injected -`LLMClient` returns `(nil, nil)`. Assert `ErrLLMGenerate`. Extend the existing -fake only as needed to express this case; do not create a mock framework. - -### Reserved Provider Parameters - -Add `TestRunRejectsReservedExtraParamsBeforeProviderCall`. - -- Use the built-in client with a custom immediate `RoundTripper` that records - whether it was invoked. -- Supply a valid contract prompt and profile plus request - `ExtraParams: map[string]any{"model": "collision"}`. -- Assert `ErrInvalidRequest`. -- Assert that the transport was not invoked. - -Run: - -```bash -go test . -go test -count=20 . -``` - -### Stage 3 Gate - -- The four-layer precedence table and presence assertions pass. -- Cancellation is deterministic and contains no wall-clock sleeps. -- Nil injected responses and reserved parameters preserve their public error - categories. -- Superseded assertions or tests have been removed rather than duplicated. -- No production file changed. - -## Stage 4: Audit Ownership And Validate The Baseline - -### Ownership Audit - -Review the behavior list in [step3.md](step3.md) against the final suite. -Confirm: - -- root facade and public contract tests are Promptkit-destined; -- `internal/domain`, `internal/usecase`, `internal/promptdef`, - `internal/prompt`, `internal/profile`, `internal/profile/builtin`, - `internal/filecatalog`, general `internal/artifact`, `internal/validate`, and - `internal/llm` tests move with Promptkit-owned behavior; -- CLI, application configuration, prepared formatting, HTTP DTO, strict JSON, - HTTP limit, and rooted artifact-containment tests remain Scriptorium-owned; -- HTTP and CLI tests that currently construct internal runners or inspect - internal sentinels retain their observable assertions and are explicitly - deferred for boundary rewrites in Migration Step 4; and -- no consequential behavior in the feature roadmap lacks a test owner. - -Do not create a permanent test-inventory document. Record any unexpected -ownership exception in `step3.md`; otherwise the ownership table there is the -complete disposition. - -### Full Validation - -Run: - -```bash -go test ./... -go vet ./... -build_dir="$(mktemp -d)" -go build -o "$build_dir/scriptorium" ./cmd/scriptorium -go test -count=20 . -go test ./internal/adapter/http -run TestMaintainedHTTPRunExampleMatchesRequestContract -bash ./examples/render-markdown-summary.sh -go run ./cmd/scriptorium render \ - --config ./examples/config.full.yml \ - --prompt generic.markdown_summary \ - --input transcript=./examples/fixtures/transcript.md \ - --input glossary=./examples/fixtures/glossary.yml \ - --format text -go run ./examples/go-library/prepare -git diff --check -``` - -Validate every local Markdown link and path in the changed roadmap files. -Confirm both configuration examples were accepted through the real -configuration loader by the two render commands. - -Inspect the final diff and confirm the implementation changed only: - -- `testdata/framework/**`; -- `engine_test.go`; -- `internal/usecase/integration_test.go` by deletion; -- `docs/roadmap/step3.md`; -- `docs/roadmap/implementation.md`; and -- the Step 3 status in `docs/roadmap/migration.md`. - -If a necessary change falls outside that list, stop and revise the plan or -request direction rather than expanding scope implicitly. - -### Completion Bookkeeping - -After every check passes: - -1. update `step3.md` to state that the target state is complete and summarize - the characterized baseline without reintroducing an implementation log; -2. add a Step 3 gate-status entry to `migration.md` with the completion date - and a short validation summary; and -3. mark this implementation plan complete. - -Do not begin Migration Step 4 in the same change. - -### Stage 4 Gate - -- Every completion criterion in `step3.md` is satisfied. -- The full suite and maintained examples pass offline. -- The diff contains no production behavior or API change. -- The main migration roadmap identifies Step 3 as complete and Step 4 as next. - -## Open Questions - -None. diff --git a/docs/roadmap/step3.md b/docs/roadmap/step3.md deleted file mode 100644 index 94948b9..0000000 --- a/docs/roadmap/step3.md +++ /dev/null @@ -1,178 +0,0 @@ -# Migration Step 3: Framework Characterization - -## Status - -Completed on 2026-07-27. The target state below is characterized by the -passing baseline; its ordered implementation record is in -[implementation.md](implementation.md). - -## Purpose - -Characterize the framework and adapter contracts that must survive the -Promptkit extraction. The work should make those contracts portable across the -future repository boundary without duplicating behavior at every test layer or -changing production APIs ahead of Migration Step 4. - -The [testing policy](../policy/testing.md) governs test value, ownership, and -sufficiency. The [accepted split decision](../adr/0002-split-promptkit-from-scriptorium.md) -governs which project will own each behavior. - -## Baseline Findings - -The current suite already provides broad coverage at the public facade, -framework package, CLI, HTTP, source, validation, and model-client boundaries. -Step 3 is therefore a portability and risk-closing exercise rather than a -general coverage expansion. - -The principal extraction risk is fixture ownership: - -- public `Engine` tests rely extensively on executable assets under - `examples/`; -- the assembled runner integration test also reads those assets; and -- the accepted split leaves executable examples in Scriptorium while moving - the public facade and framework tests to Promptkit. - -Those dependencies would either prevent the tests from moving or create -unwanted cross-repository fixture coupling. A smaller set of public-boundary -gaps also remains around complete setting precedence, cancellation, malformed -injected-client behavior, and reserved provider parameters. - -## Target State - -The completed baseline provides: - -- tests destined for Promptkit use only Promptkit-destined testdata or - fixtures generated within the test; -- Scriptorium's maintained executable examples remain independently validated - by Scriptorium-owned checks; -- each behavior named in the main migration roadmap has one clear test owner; -- representative public `Engine` tests protect assembled framework behavior; -- focused package tests continue to own strict parsing, source mechanics, - validation rules, and provider mapping without higher-level duplication; -- Scriptorium adapter tests continue to protect CLI, HTTP, containment, limits, - and transport mappings; and -- no production API or package boundary has changed as part of Step 3. - -## Policy Choices - -### Framework-Owned Testdata - -Promptkit-destined tests will use a compact framework contract corpus under root -`testdata/` or a purpose-built fixture created inside the test. The shared -corpus will contain only the assets needed to express durable framework -behavior: - -- one ordinary prompt definition; -- one JSON Schema structured-output prompt definition; -- representative execution profiles; -- one JSON Schema document; and -- small file-artifact inputs. - -The corpus should exercise directory-backed loading and relative prompt content -where those behaviors matter. Use small inline `fstest.MapFS` or temporary -fixtures for cases that do not benefit from shared files. - -The complete executable example tree will not be copied. Testdata will remain -minimal, synthetic, secret-free, and distinct from user-facing examples. - -### Test Boundaries And Consolidation - -Representative assembled behavior belongs at the public `Engine` boundary. -Focused parsing, source, validation, provider, and adapter mechanics remain -with their package-level owners. Existing tests should be consolidated when a -new public contract test would otherwise duplicate the same risk. - -The assembled runner integration behavior will be protected through the public -facade rather than through a second test tied to internal domain and runner -types. Promptkit-destined tests will not read Scriptorium-owned `examples/` -assets. Scriptorium adapter and maintained-example checks may continue to do so -where the example itself is the contract under test. - -### Required Public Characterization - -The public suite will characterize: - -- an assembled directory-backed `Engine.Run` workflow with file inputs, - structured output, schema validation, hashes, usage, and timing; -- framework-default, profile, and request execution-setting precedence; -- explicit numeric-zero propagation and target-presence metadata; -- caller-context cancellation at the outbound generation boundary and its - public error classification; -- nil responses from injected model clients; and -- reserved provider parameters failing before a provider call. - -These tests will use deterministic synchronization, real local collaborators -where inexpensive, and fakes only at the model-provider boundary. - -## Target Test Ownership - -| Test category | Future disposition | -| --- | --- | -| Public `Engine`, facade, model-client extension, and public error contracts | Move to Promptkit. | -| Framework domain, runner, prompt, profile, built-in registry, general artifact, validation, and LLM package tests | Move with their Promptkit-owned implementation. | -| CLI parsing, application configuration, prepared-run formatting, and process behavior | Remain in Scriptorium. | -| HTTP DTOs, strict JSON, limits, response mapping, and rooted artifact containment | Remain in Scriptorium. | -| Tests that construct internal runners or classify internal framework sentinels from Scriptorium adapters | Preserve their observable assertions, then rewrite against the public Promptkit boundary during Step 4. | -| Maintained Go consumer example | Move to Promptkit. | -| Maintained executable configuration, render, HTTP, and fixture examples | Remain in Scriptorium. | - -Existing focused tests remain the owners of: - -- strict prompt, profile, and application YAML decoding; -- strict HTTP JSON decoding; -- prompt, profile, schema, and artifact source mechanics; -- built-in profile validation and overlay fallback; -- structured-output encoding and schema validation; -- validation content failures versus operational failures; -- credential handling and redaction; -- OpenAI-compatible wire behavior; and -- HTTP artifact restrictions and transport mappings. - -Coverage will be added only if the ownership audit identifies a consequential -behavior with no credible existing owner. - -## Required Validation Outcome - -The characterized baseline must pass the complete Go test and vet suites, a -temporary-output executable build, repeated public contract tests, both -maintained application configurations, maintained render and Go consumer -examples, the maintained HTTP request-example check, documentation-link -validation, and whitespace validation. All checks must remain offline and -independent of real credentials. - -## Out Of Scope - -Step 3 does not: - -- add artifact-reader, repository, validator, or other production extension - APIs; -- refactor CLI or HTTP adapters to consume the public facade; -- move restricted HTTP artifact behavior out of its current package; -- create the Promptkit repository or change the Go module path; -- move implementation packages between repositories; -- create compatibility aliases or forwarding APIs; -- redesign the public facade; or -- add tests solely to increase a coverage percentage. - -Those changes belong to later migration steps. - -## Completion Criteria - -Step 3 is complete when: - -- Promptkit-destined tests have no dependency on Scriptorium-owned executable - examples; -- the targeted public contract gaps are covered with deterministic tests; -- the full roadmap behavior list has a clear, non-duplicative test owner; -- tests that require Step 4 rewrites are explicitly identified; -- no production behavior or public API changed; -- all required validation passes; and -- the main migration roadmap records the Step 3 gate as complete. - -Migration Step 4 must not begin until these criteria are satisfied. - -## Lifecycle - -This completed implementation roadmap remains the concise characterization -record for the migration. Repository history retains the detailed implementation -record. diff --git a/engine_test.go b/engine_test.go index 1d92987..ef47e86 100644 --- a/engine_test.go +++ b/engine_test.go @@ -610,6 +610,8 @@ func TestRunPassesPreparedRequestToInjectedLLMClient(t *testing.T) { } func TestEngineRunPropagatesCallerCancellation(t *testing.T) { + const synchronizationTimeout = 5 * time.Second + started := make(chan struct{}) transport := roundTripFunc(func(req *http.Request) (*http.Response, error) { close(started) @@ -640,10 +642,24 @@ func TestEngineRunPropagatesCallerCancellation(t *testing.T) { result <- err }() - <-started + watchdog := time.NewTimer(synchronizationTimeout) + defer watchdog.Stop() + select { + case <-started: + case err := <-result: + t.Fatalf("Engine.Run returned before the transport started: %v", err) + case <-watchdog.C: + t.Fatal("timed out waiting for the transport to start") + } + cancel() - if err := <-result; !errors.Is(err, scriptorium.ErrLLMGenerate) { - t.Fatalf("expected ErrLLMGenerate after caller cancellation, got %v", err) + select { + case err := <-result: + if !errors.Is(err, scriptorium.ErrLLMGenerate) { + t.Fatalf("expected ErrLLMGenerate after caller cancellation, got %v", err) + } + case <-watchdog.C: + t.Fatal("timed out waiting for Engine.Run to return after cancellation") } }