From 4c7278febc4b0e3489a9aa8821e6632ec30ace54 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 27 Jul 2026 20:40:23 -0500 Subject: [PATCH] Remove the completed step 4 migration roadmap --- docs/roadmap/implementation.md | 62 --- docs/roadmap/step4.md | 386 ------------------ internal/adapter/http/artifact_reader_test.go | 9 +- 3 files changed, 7 insertions(+), 450 deletions(-) delete mode 100644 docs/roadmap/implementation.md delete mode 100644 docs/roadmap/step4.md diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md deleted file mode 100644 index dd37a58..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -1,62 +0,0 @@ -# Migration Step 4 Implementation Plan - -## Status - -Complete as of 2026-07-28. The public-facade adapter boundary and its -post-implementation remediation are fully revalidated. - -## Objective - -This plan completed the target state in the -[Step 4 public-facade adapter roadmap](step4.md) without changing the intended -CLI, HTTP, or public Go contracts. - -The [accepted split decision](../adr/0002-split-promptkit-from-scriptorium.md) -owns the long-term Promptkit/Scriptorium boundary. The -[testing policy](../policy/testing.md) governs test value and ownership. The -[documentation policy](../policy/documentation.md) governs completion -bookkeeping and canonical ownership. - -## Constraints - -- Keep this work in the Scriptorium repository. Do not modify the sibling - Promptkit repository or infer that Migration Step 5 is complete merely from - its local presence. -- Preserve the public `ArtifactReader`, error sentinel, CLI, HTTP, containment, - artifact-size, formatting, timeout, credential, and redaction contracts. -- Do not add compatibility shims, begin framework extraction, change the module - path, or add dependencies. -- Keep tests deterministic, offline, independent of machine MIME databases, - and focused at the narrowest stable owner. -- Preserve unrelated working-tree changes. - -## Completed Work - -The original implementation established the Step 4 boundary: - -- the root facade now exposes `ArtifactReader` and `WithArtifactReader`, copies - mutable artifact bodies, handles nil responses, and exposes the two specific - public request-error identities required by HTTP; -- the restricted artifact reader and its containment and byte-limit policy now - belong to the HTTP adapter and use public artifact types; -- prepared-run formatting and CLI `run` and `render` consume public prepared, - request, and result values; -- the HTTP handler consumes a local interface expressed in public types, and - CLI `serve` injects the restricted reader into the public engine; -- the legacy restricted framework reader was removed, current-behavior - documentation was reconciled, and an initial dependency guard was added; and -- the initial validation pass covered the full suite, vet, build, race and - repeated tests, maintained examples, configuration loading, links, and - whitespace. - -The post-implementation remediation is also complete: - -- public error mapping retains collaborator identities while adding the active - operation's public category; -- HTTP artifact MIME coverage uses a built-in media type while retaining the - unknown-extension fallback check; -- the dependency guard recursively scans production adapter and formatter - sources and rejects descendants of forbidden framework package families; and -- the final validation passed full and repeated tests, vet, a temporary-output - build, race checks, maintained examples, both maintained configuration files, - import and link inspection, and whitespace validation. diff --git a/docs/roadmap/step4.md b/docs/roadmap/step4.md deleted file mode 100644 index 42e774b..0000000 --- a/docs/roadmap/step4.md +++ /dev/null @@ -1,386 +0,0 @@ -# Migration Step 4: Public-Facade Adapter Boundary - -## Status - -Complete as of 2026-07-28. The public-facade adapter boundary and its -post-implementation error-classification, MIME-test, and dependency-guard -corrections are validated. The [implementation plan](implementation.md) -records the completed work, and the [main migration roadmap](migration.md) -identifies repository creation as the next gate. - -## Purpose - -Make Scriptorium's CLI and HTTP adapters genuine consumers of the public -framework facade before that facade moves to Promptkit. This establishes and -tests the dependency boundary inside the current repository, where it can be -changed atomically, before the framework and application are separated across -repositories. - -The [accepted split decision](../adr/0002-split-promptkit-from-scriptorium.md) -owns the long-term project boundary. The -[main migration roadmap](migration.md) owns the overall sequence. This feature -roadmap defines the desired Step 4 state, not an implementation sequence. - -## Achieved Boundary - -The CLI, HTTP handler, and prepared-run formatter now consume public engine -values and errors. `serve` injects Scriptorium's HTTP-owned restricted artifact -reader through the public extension point, while the root facade continues to -compose the framework implementation inside this repository. A repository-level -dependency test recursively protects production adapter and formatter packages -from framework-owned imports. - -## Target State - -Scriptorium's executable path is an ordinary consumer of the same public -framework boundary used by other Go applications: - -```text -cmd/scriptorium - | - v -Scriptorium CLI and HTTP adapters - | - v -public Engine, requests, results, errors, and extension interfaces - | - v -framework implementation packages -``` - -The CLI and HTTP production packages do not import framework-owned internal -packages. They translate application configuration, flags, and HTTP DTOs into -public engine configuration and request values; call `Prepare` or `Run`; and -translate public results and errors back into their owned interfaces. - -The root facade may continue to use the existing internal implementation during -Step 4. That implementation moves in later migration steps. The important -result here is that no Scriptorium-owned adapter or presentation component -depends on it directly. - -## Public Facade Requirements - -### Engine Consumption - -The current public `Engine`, `Config`, `RunRequest`, `PreparedRun`, -`RunResult`, request helpers, result values, and existing broad error sentinels -remain the primary boundary. Step 4 must not introduce a second facade, -adapter-specific engine, exported internal runner, or public repository -constructor. - -Scriptorium-local interfaces may abstract the methods an adapter needs for test -substitution. They must: - -- be declared on the consuming side; -- use only public facade types; -- contain only `Prepare`, `Run`, or the narrower subset required by that - consumer; and -- be satisfied directly by `*Engine`. - -Promptkit must not acquire CLI, HTTP, status-code, or response-format concepts -to satisfy these interfaces. - -### Artifact Reader Extension - -The public facade will expose the demonstrated artifact-loading extension point: - -```go -type ArtifactReader interface { - Read(context.Context, ArtifactRef) (*Artifact, error) -} - -func WithArtifactReader(ArtifactReader) Option -``` - -This option replaces the engine's ordinary composite artifact reader for all -input references. The default remains the framework's ordinary inline and -caller-selected file behavior. - -The boundary must have the same defensive behavior as other public injection -points: - -- a nil reader is rejected as `ErrInvalidConfig`; -- a reader response of `(nil, nil)` becomes `ErrArtifactLoad` rather than - causing a panic; -- reader failures are wrapped as `ErrArtifactLoad` while retaining the - original error identity for `errors.Is`; -- conversion between public and internal values does not expose internal domain - types; and -- mutable artifact content is copied across the boundary where needed to avoid - unintended aliasing. - -This is the only new framework extension interface required by Step 4. No -public prompt repository, profile repository, renderer, validator, or internal -runner interface is needed for the current adapters. - -### Public Error Detail - -The facade's existing broad errors remain authoritative for general consumers. -To preserve Scriptorium's current HTTP contract without inspecting Promptkit -internals, the public error chain must additionally retain stable identities -for: - -- profile selection being required because neither the request nor the prompt - supplies a profile; and -- a selected credential environment variable being unset or empty. - -These identities will be exposed as `ErrProfileRequired` and -`ErrAPIKeyEnvMissing`. Each remains nested under `ErrInvalidRequest`, so callers -that need only the broad category continue to match it. Scriptorium uses the -more specific identities only to preserve its existing HTTP error codes. - -No error-string parsing is permitted. Prompt, profile, artifact, render, -generation, and validation failures continue to use the existing public -sentinels. Scriptorium-owned HTTP artifact-policy errors remain in Scriptorium -and survive wrapping through the public artifact-reader boundary. - -## CLI Boundary - -The `run` and `render` commands will construct the public engine from resolved -application settings: - -- `prompt_dir`, `profile_dir`, and `schema_dir` map to public engine - configuration; -- the public engine supplies the built-in profile registry, renderer, - validator, ordinary artifact reader, and default OpenAI-compatible client; -- CLI input paths map to public file artifact references; -- CLI runtime flags map to the public request override while preserving - numeric presence, including explicit zero; and -- omitted application values remain omitted so framework defaults are not - duplicated in Scriptorium. - -`run` consumes the public result for artifact output, validation exit status, -and the stderr summary. `render` consumes the public prepared value. The -Scriptorium-owned prepared-run formatter will accept public prepared values -instead of internal domain values without changing its text or JSON contract. - -The `serve` command will construct the same public engine with Scriptorium's -restricted artifact reader injected through `WithArtifactReader`. It will pass -that engine to the HTTP handler through a Scriptorium-local run interface. - -CLI construction must handle public engine-construction errors explicitly. -The executable must not construct an internal LLM client, repository, renderer, -validator, artifact reader, or runner. - -## HTTP Boundary - -The HTTP handler's consumer interface will accept public `RunRequest` values -and return public `RunResult` values. DTO mapping will preserve the existing -HTTP contract: - -- strict JSON decoding and request-size enforcement remain in Scriptorium; -- prompt, profile, input, variable, and execution-override fields map to their - corresponding public values; -- pointer-valued numeric overrides retain omitted-versus-explicit-zero - semantics; -- raw API keys remain absent from the HTTP request shape; -- public run results map to the current response DTOs; -- raw model output remains opt-in; and -- response-size enforcement remains an HTTP concern. - -HTTP error mapping will inspect only: - -- public framework errors; -- Scriptorium's HTTP artifact-policy errors; and -- standard-library transport errors owned by the handler. - -It will not inspect internal prompt, profile, artifact, domain, or use-case -errors. Existing HTTP statuses, error codes, and sanitized messages remain -unchanged. - -## HTTP Artifact Policy - -The restricted artifact reader remains Scriptorium-owned and becomes an -implementation of the public `ArtifactReader` interface. It belongs with the -HTTP adapter rather than the Promptkit-destined general artifact package. - -The reader will continue to: - -- resolve inline references without permitting empty inline bodies; -- deny file references when no artifact root is configured; -- enforce the configured maximum artifact size; -- enforce the documented lexical root-containment rule without resolving - symlinks; -- return complete public artifact metadata; and -- preserve distinct Scriptorium errors for a denied or out-of-root file and an - oversized file. - -The public engine treats those errors as artifact-load failures while preserving -their identities. The HTTP mapper checks the Scriptorium-specific identity -before the broad public `ErrArtifactLoad` identity, retaining the current -`artifact_not_allowed`, `artifact_too_large`, and general artifact-read -outcomes. - -General inline and unrestricted file reading remains framework-owned. Step 4 -separates the HTTP policy from that implementation far enough that each side -can later move to its target repository without redesigning the interface. - -## Package And Dependency Boundaries - -The completed dependency state is: - -| Component | Permitted framework dependency | -| --- | --- | -| `internal/adapter/cli` | Public facade types, constructors, options, errors, and methods only | -| `internal/adapter/http` | Public facade types and errors only; Scriptorium-owned HTTP artifact policy remains local | -| `internal/format` | Public prepared-run and rendered-message values only | -| `internal/config` | Scriptorium application settings and Scriptorium-owned defaults; no framework orchestration | -| `cmd/scriptorium` | CLI adapter only | -| Root facade | Existing internal framework implementation until extraction | - -In particular, Scriptorium-owned adapter, formatter, and HTTP artifact-policy -production files must not import: - -- `internal/domain`; -- `internal/usecase`; -- `internal/promptdef` or `internal/prompt`; -- `internal/profile` or `internal/profile/builtin`; -- `internal/validate`; -- `internal/llm`; or -- the Promptkit-destined general artifact implementation. - -Tests for Scriptorium-owned components should follow the same public boundary -except when directly testing a Scriptorium-owned package. - -## Observable Behavior - -Step 4 is an architectural refactor plus the minimum additive public extension -surface required to support it. It is not a redesign of the executable -interfaces. - -The following behavior must remain unchanged: - -- CLI commands, flags, aliases, precedence, output routing, summaries, and exit - codes; -- application configuration discovery, strict decoding, fields, and defaults; -- HTTP routes, strict decoding, DTOs, statuses, codes, messages, and limits; -- HTTP artifact containment and size enforcement; -- prompt/profile selection and override precedence; -- explicit numeric-zero behavior; -- built-in profile fallback and custom-profile overlays; -- structured-output and validation behavior; -- timeout layering; -- secret handling and redaction; and -- maintained executable examples. - -The intended public additions are limited to artifact-reader injection and the -specific error identities required by the HTTP mapper. No compatibility shim is -needed because this repository still owns the facade during Step 4 and the -overall migration is intentionally breaking. - -## Test Ownership And Verification - -Tests will protect the boundary at the layer that owns each risk: - -- public engine tests own artifact-reader option validation, conversion, - invocation, nil-response handling, error wrapping, and error identity; -- CLI tests own flag and configuration mapping into public requests, public - engine wiring, presentation, output, and exit behavior; -- HTTP tests own strict DTO mapping to public requests, public result mapping, - error/status mapping, limits, and raw-output opt-in; -- HTTP artifact-reader tests own denied, contained, escaped, oversized, inline, - cancellation, and metadata behavior; -- formatter tests own stable text and JSON presentation of public prepared - values; and -- existing framework tests continue to own orchestration, source, validation, - provider, and broad public error behavior. - -Adapter tests that currently construct internal runners or assert internal -sentinels will be rewritten against the public engine or small public-typed -fakes. Duplicate framework-semantic assertions should be removed when the -public contract suite already owns the risk. - -The final suite must include an enforceable dependency check showing that -Scriptorium-owned adapter and presentation production packages do not import -Promptkit-destined internal packages. This may be a focused architecture test -or an equivalent deterministic repository check; it must diagnose the -forbidden import clearly. - -## Documentation Outcome - -When the boundary is implemented, current-behavior documentation will be -reconciled in the same change: - -- the public Go package contract will define `ArtifactReader`, - `WithArtifactReader`, and the new error identities; -- adapter internals will describe public-engine composition and public-value - mapping; -- source internals will distinguish the public reader extension, general - framework readers, and Scriptorium's HTTP reader; -- the internal overview and architecture policy will reflect that executable - adapters consume the public facade; and -- CLI, configuration, HTTP, integration, and operations contracts will change - only if verification finds an observable correction is necessary. - -Documents will keep exact external contracts in their existing canonical -owners and link rather than duplicate them. - -## Required Validation Outcome - -The completed boundary must pass: - -- the full Go test suite; -- `go vet` across all packages; -- a temporary-output executable build; -- repeated public artifact-reader and adapter boundary tests; -- both maintained application configurations; -- maintained render, HTTP-request, and Go-package examples; -- CLI and HTTP smoke checks that exercise the public engine path; -- the forbidden-import dependency check; -- local Markdown-link validation; and -- whitespace validation. - -All default validation remains offline, deterministic, and independent of real -credentials. - -## Out Of Scope - -Step 4 does not: - -- create the Promptkit repository or module; -- change the module or root package name; -- move framework implementation packages or built-in profiles out of this - repository; -- remove the current root facade; -- add compatibility aliases or forwarding packages; -- make Scriptorium depend on an external Promptkit revision; -- broadly export framework repositories, domain values, validators, renderers, - or runner constructors; -- redesign prompt, profile, schema, request, response, CLI, or configuration - formats; -- change HTTP containment from lexical path checking to symlink resolution; -- add new execution or repair behavior; or -- perform unrelated facade cleanup. - -Those changes belong to later migration steps or a separately accepted feature. - -## Completion Criteria - -Step 4 is complete when: - -- CLI `run` and `render` execute through the public engine; -- CLI `serve` injects the Scriptorium-owned restricted reader into the public - engine and passes that engine to the HTTP handler; -- HTTP and CLI map only public framework request, result, and error values; -- prepared-run formatting consumes the public prepared value; -- the public artifact-reader extension has the specified validation, - conversion, nil-response, and error-preservation behavior; -- public error identities preserve every distinction required by the current - HTTP contract; -- Scriptorium-owned adapters, formatter, and HTTP artifact reader have no - Promptkit-destined internal imports; -- HTTP containment, limits, error mapping, and all existing executable - behavior remain protected by passing tests; -- current-behavior documentation reflects the implemented boundary; -- every required validation check passes; -- no out-of-scope extraction or compatibility work is included; and -- the main migration roadmap records Step 4 as complete and identifies - repository creation in Step 5 as the next gate. - -Migration Step 5 must not begin until these criteria are satisfied. - -## Lifecycle - -This feature roadmap is a temporary migration artifact. It may be removed after -Step 4 is complete and no longer needs to guide active work; repository history -will retain the decision and completion record. diff --git a/internal/adapter/http/artifact_reader_test.go b/internal/adapter/http/artifact_reader_test.go index 9145a41..26ec808 100644 --- a/internal/adapter/http/artifact_reader_test.go +++ b/internal/adapter/http/artifact_reader_test.go @@ -3,6 +3,7 @@ package httpadapter import ( "context" "errors" + "mime" "os" "path/filepath" "testing" @@ -26,6 +27,10 @@ func TestRestrictedArtifactReaderReadsContainedFiles(t *testing.T) { if err := os.WriteFile(filepath.Join(outside, "secret.txt"), []byte("denied"), 0o644); err != nil { t.Fatal(err) } + expectedContentType := mime.TypeByExtension(filepath.Ext(inputPath)) + if expectedContentType == "" { + t.Fatal("expected built-in HTML content type") + } reader, err := NewRestrictedArtifactReader(root, 0) if err != nil { @@ -43,8 +48,8 @@ func TestRestrictedArtifactReaderReadsContainedFiles(t *testing.T) { if artifact.Name != "input.html" || artifact.URI != inputPath || artifact.Size != int64(len("allowed")) || string(artifact.Body) != "allowed" { t.Fatalf("unexpected artifact metadata: %#v", artifact) } - if artifact.ContentType != "text/html; charset=utf-8" { - t.Fatalf("unexpected artifact content type: %q", artifact.ContentType) + if artifact.ContentType != expectedContentType { + t.Fatalf("unexpected artifact content type: got %q, want %q", artifact.ContentType, expectedContentType) } if artifact.Hash != artifactHash([]byte("allowed")) { t.Fatalf("unexpected artifact hash: %q", artifact.Hash)