Add an implementation plan and roadmap for Step 4 of the migration plan
This commit is contained in:
554
docs/roadmap/implementation.md
Normal file
554
docs/roadmap/implementation.md
Normal file
@@ -0,0 +1,554 @@
|
||||
# Migration Step 4 Implementation Plan
|
||||
|
||||
## Status
|
||||
|
||||
Proposed. Implement each stage in order and satisfy its gate before continuing.
|
||||
|
||||
## Objective
|
||||
|
||||
Implement the target state in the
|
||||
[Step 4 public-facade adapter roadmap](step4.md): make Scriptorium's CLI, HTTP
|
||||
handler, prepared-run formatter, and HTTP artifact policy consume only the
|
||||
public framework facade, while preserving the current executable 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 current-behavior
|
||||
updates and canonical ownership.
|
||||
|
||||
## Constraints
|
||||
|
||||
- Execute the stages in order and keep the repository buildable and testable at
|
||||
every stage gate.
|
||||
- Do not create the Promptkit repository, change the module path, rename the
|
||||
root package, or move framework packages out of this repository.
|
||||
- Do not export internal repositories, domain values, renderers, validators,
|
||||
the runner, or adapter DTOs.
|
||||
- Add only the public artifact-reader extension and public error identities
|
||||
required by the Step 4 roadmap.
|
||||
- Preserve CLI, application configuration, HTTP, containment, validation,
|
||||
timeout, credential, and redaction behavior.
|
||||
- Keep tests deterministic, offline, independent of real credentials, and
|
||||
focused at the narrowest stable owner.
|
||||
- Prefer rewriting or moving an existing test over retaining parallel
|
||||
assertions for the same behavior.
|
||||
- Preserve unrelated working-tree changes.
|
||||
- Do not begin Migration Step 5 in this change.
|
||||
|
||||
## Stage 1: Complete The Public Framework Boundary
|
||||
|
||||
Add the public artifact-reader extension and error identities before changing
|
||||
any adapter consumer.
|
||||
|
||||
### Public Artifact Reader
|
||||
|
||||
In `types.go`, add:
|
||||
|
||||
```go
|
||||
type ArtifactReader interface {
|
||||
Read(context.Context, ArtifactRef) (*Artifact, error)
|
||||
}
|
||||
```
|
||||
|
||||
In `engine.go`, add `WithArtifactReader(ArtifactReader) Option`. Extend
|
||||
`engineOptions` with an internal artifact-reader slot and source-presence flag,
|
||||
parallel to the existing LLM and source options. A supplied reader replaces
|
||||
`artifact.NewCompositeReader()` when the runner is constructed; when the option
|
||||
is omitted, current default inline and unrestricted file behavior remains
|
||||
unchanged.
|
||||
|
||||
Reject a nil public reader as `ErrInvalidConfig`. Keep option construction
|
||||
errors wrapped by `NewEngine` consistently with the existing option behavior.
|
||||
|
||||
Create `artifact_reader.go` in the root facade with a private adapter that
|
||||
implements `internal/artifact.Reader` by:
|
||||
|
||||
1. converting the internal reference to a public `ArtifactRef`;
|
||||
2. calling the supplied public reader with the original context;
|
||||
3. returning the reader error unchanged so the runner and facade can add their
|
||||
normal artifact-load categories while preserving `errors.Is`;
|
||||
4. rejecting `(nil, nil)` with a descriptive non-sentinel error so the runner
|
||||
maps it to `ErrArtifactLoad`; and
|
||||
5. converting a non-nil public artifact to an internal artifact while copying
|
||||
`Body`.
|
||||
|
||||
Do not teach the public adapter to infer missing names, hashes, sizes, or
|
||||
content types. Those values are the reader's responsibility, except that the
|
||||
existing runner continues assigning the input-map name when `Artifact.Name`
|
||||
is empty.
|
||||
|
||||
### Public Error Detail
|
||||
|
||||
Add public `ErrProfileRequired` and `ErrAPIKeyEnvMissing` sentinels alongside
|
||||
the existing facade errors.
|
||||
|
||||
Update `mapPublicError` so:
|
||||
|
||||
- a missing explicit/default profile matches both `ErrInvalidRequest` and
|
||||
`ErrProfileRequired`;
|
||||
- an unset selected credential environment variable matches both
|
||||
`ErrInvalidRequest` and `ErrAPIKeyEnvMissing`;
|
||||
- the original internal error remains wrapped for context during the
|
||||
single-repository phase; and
|
||||
- all existing broad public mappings retain their current behavior.
|
||||
|
||||
Use error wrapping, not message inspection. Check the specific internal causes
|
||||
before the general internal `ErrInvalidRequest`. Construct the returned chain
|
||||
so `errors.Is` can independently match the broad public sentinel, the specific
|
||||
public sentinel, and the original cause.
|
||||
|
||||
### Public Tests
|
||||
|
||||
Extend `engine_test.go` rather than creating a mock framework. Add a small
|
||||
recording public `ArtifactReader` fake and cover:
|
||||
|
||||
- `WithArtifactReader(nil)` returns `ErrInvalidConfig`;
|
||||
- the reader receives the exact public type, URI, and body from a request;
|
||||
- a returned artifact participates in a successful preparation through the
|
||||
real engine collaborators;
|
||||
- a reader error produces an error matching both the reader's custom sentinel
|
||||
and `ErrArtifactLoad`;
|
||||
- `(nil, nil)` produces `ErrArtifactLoad` without panicking; and
|
||||
- reader cancellation or a canceled caller context remains visible through the
|
||||
artifact-load error chain.
|
||||
|
||||
Add `artifact_reader_internal_test.go` in package `scriptorium` with a focused
|
||||
test of the private adapter's body-copy guarantee.
|
||||
|
||||
Extend the existing missing-credential public test to assert
|
||||
`ErrAPIKeyEnvMissing` and `ErrInvalidRequest`. Add a purpose-built prompt with
|
||||
no default profile and assert that preparation without an explicit profile
|
||||
matches `ErrProfileRequired` and `ErrInvalidRequest`. Do not assert complete
|
||||
error strings.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test .
|
||||
go test -count=20 -run 'Test.*(ArtifactReader|ProfileRequired|MissingCredentials)' .
|
||||
```
|
||||
|
||||
### Stage 1 Gate
|
||||
|
||||
- The public artifact-reader option has the exact contract defined in
|
||||
`step4.md`.
|
||||
- Public error chains retain the two HTTP-required distinctions and their broad
|
||||
invalid-request category.
|
||||
- Default engine artifact behavior is unchanged when the option is absent.
|
||||
- No adapter has been refactored yet.
|
||||
|
||||
## Stage 2: Establish The Scriptorium-Owned HTTP Artifact Reader
|
||||
|
||||
Create `internal/adapter/http/artifact_reader.go`. It must implement the new
|
||||
public `scriptorium.ArtifactReader` interface directly and must not import
|
||||
`internal/domain`, the Promptkit-destined `internal/artifact`, or framework
|
||||
defaults.
|
||||
|
||||
Expose an internal-repository constructor for the CLI adapter:
|
||||
|
||||
```go
|
||||
func NewRestrictedArtifactReader(root string, maxBytes int64) (scriptorium.ArtifactReader, error)
|
||||
```
|
||||
|
||||
Keep these sentinels in the HTTP adapter package:
|
||||
|
||||
- `ErrFileNotAllowed`;
|
||||
- `ErrFileOutsideRoot`; and
|
||||
- `ErrFileTooLarge`.
|
||||
|
||||
The implementation should move the HTTP-owned mechanisms from
|
||||
`internal/artifact` into public types:
|
||||
|
||||
- route public inline and file reference types;
|
||||
- reject unsupported reference types, missing inline bodies, and missing file
|
||||
paths with descriptive local errors;
|
||||
- hash inline and file bodies with SHA-256;
|
||||
- use `text/plain` as the inline and unknown-extension fallback content type;
|
||||
- infer file content type with `mime.TypeByExtension`;
|
||||
- populate file name, URI, size, hash, body, and content type;
|
||||
- deny file references when the trimmed root is empty;
|
||||
- reject a negative byte limit during construction;
|
||||
- treat zero as an unlimited byte limit;
|
||||
- clean and absolutize a configured root;
|
||||
- accept cleaned relative and absolute paths lexically contained by that root;
|
||||
- reject paths lexically outside the root;
|
||||
- retain the documented behavior of following symlinks after lexical checking;
|
||||
- check file size before reading and enforce the limit again with
|
||||
`io.LimitReader(maxBytes+1)`; and
|
||||
- honor caller cancellation before resolving either inline or file input.
|
||||
|
||||
Move the restricted-reader unit cases from
|
||||
`internal/artifact/reader_test.go` into
|
||||
`internal/adapter/http/artifact_reader_test.go`, rewritten with public
|
||||
`ArtifactRef` and `Artifact` values. Preserve coverage for contained relative
|
||||
and absolute paths, traversal, outside absolute paths, symlinks, no-root
|
||||
denial, exact and excessive byte limits, zero limits, metadata, and negative
|
||||
limits. Add focused cancellation and unsupported/malformed-reference cases.
|
||||
|
||||
Keep the old restricted-reader implementation in `internal/artifact` only
|
||||
temporarily because CLI serving and some handler tests still use its internal
|
||||
interface. Remove its now-moved focused unit cases to avoid maintaining two
|
||||
authoritative test sets. Mark no compatibility promise for the old internal
|
||||
constructor; it must be deleted in Stage 4 when the HTTP handler and CLI
|
||||
`serve` path move together.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/artifact ./internal/adapter/http
|
||||
```
|
||||
|
||||
### Stage 2 Gate
|
||||
|
||||
- The new HTTP reader independently satisfies the public reader contract and
|
||||
preserves every documented HTTP containment outcome.
|
||||
- Its focused tests use no framework-internal type.
|
||||
- The old restricted implementation remains only as a temporary build bridge
|
||||
with a mandatory deletion point in Stage 4.
|
||||
|
||||
## Stage 3: Rewrite Formatting And Move CLI Run And Render
|
||||
|
||||
Update `internal/format/prepared_run.go` so every formatter interface and
|
||||
function accepts `*scriptorium.PreparedRun` rather than
|
||||
`*domain.PreparedRun`. Replace internal rendered-message and target references
|
||||
with their corresponding public types. Do not add a translation DTO: the
|
||||
formatter is a Scriptorium consumer of the public value.
|
||||
|
||||
Update `internal/format/prepared_run_test.go` to construct public values. Keep
|
||||
the existing behavioral assertions for:
|
||||
|
||||
- deterministic text formatting;
|
||||
- effective settings and sorted extra parameters;
|
||||
- session IDs and cache-control fields;
|
||||
- message ordering and content;
|
||||
- valid JSON output;
|
||||
- omission of absent optional values; and
|
||||
- absence of resolved or direct secret values.
|
||||
|
||||
Do not change text or JSON output solely because the source Go type changed.
|
||||
Leave the Scriptorium-owned format enum and parser used by the configuration
|
||||
package unchanged.
|
||||
|
||||
Refactor the `run` and `render` paths in `internal/adapter/cli/run.go` to import
|
||||
and consume the root public package. Keep the existing internal construction
|
||||
used by `serve` temporarily because the HTTP handler still accepts internal
|
||||
types until Stage 4. Do not add a conversion shim between a public engine and
|
||||
the old handler.
|
||||
|
||||
### Construction
|
||||
|
||||
Add a public engine-construction helper that:
|
||||
|
||||
- maps prompt, profile, and schema directories into public `Config`;
|
||||
- accepts public engine options so Stage 4 can supply `WithArtifactReader`;
|
||||
- leaves framework timeout and provider defaults unset unless Scriptorium has
|
||||
an actual application-owned override; and
|
||||
- returns the `NewEngine` error to the command.
|
||||
|
||||
`run` and `render` use this helper with the engine's default artifact reader and
|
||||
default OpenAI-compatible client. Stop constructing an internal LLM client or
|
||||
runner on those paths.
|
||||
|
||||
Report engine-construction failures as ordinary command runtime failures
|
||||
without leaking secrets.
|
||||
|
||||
Leave `serve`, its internal LLM construction, its legacy restricted reader, and
|
||||
the internal runner helper buildable but otherwise unchanged in this stage.
|
||||
They are a temporary boundary exception with an explicit removal in Stage 4.
|
||||
|
||||
### Request And Result Mapping
|
||||
|
||||
Change `buildRunRequestFromConfig` to return public `RunRequest` values:
|
||||
|
||||
- use public `File` references for CLI input paths;
|
||||
- copy variables into the public request;
|
||||
- map prompt and profile IDs unchanged;
|
||||
- create a public `ExecutionTargetOverride` only when at least one override flag
|
||||
was explicitly supplied; and
|
||||
- preserve pointer presence and explicit zero for temperature, max tokens,
|
||||
top-p, and timeout seconds.
|
||||
|
||||
Change `determineExitCode`, `printSummary`, output handling, and render
|
||||
formatting to consume public result and prepared values. Preserve current CLI
|
||||
output, validation exit classification, cache-usage summary behavior, and file
|
||||
permissions.
|
||||
|
||||
### CLI Tests
|
||||
|
||||
Rewrite CLI tests and helpers to use public request and result values. Keep
|
||||
application configuration and Scriptorium default imports where they remain
|
||||
owned locally. Preserve the existing parser, precedence, output, built-in
|
||||
profile, selected-profile, explicit-zero, provider-wire, and end-to-end command
|
||||
tests.
|
||||
|
||||
Add or consolidate a focused mapping assertion showing that omitted numeric
|
||||
flags remain nil while explicitly supplied zero values remain non-nil public
|
||||
overrides. Do not duplicate framework precedence tests already owned by the
|
||||
root suite.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/format ./internal/config
|
||||
go test ./internal/adapter/cli
|
||||
go test -count=20 ./internal/adapter/cli
|
||||
bash ./examples/render-markdown-summary.sh
|
||||
```
|
||||
|
||||
### Stage 3 Gate
|
||||
|
||||
- `internal/format` has no `internal/domain` import and formatter output remains
|
||||
unchanged.
|
||||
- Application configuration still parses and applies render-format settings.
|
||||
- CLI `run` and `render` construct and call only the public engine for framework
|
||||
behavior.
|
||||
- CLI behavior and maintained rendering remain unchanged.
|
||||
- The remaining internal framework imports are confined to `serve` construction
|
||||
and its legacy helpers, with deletion required in the next stage.
|
||||
|
||||
## Stage 4: Move The HTTP Handler And CLI Serve Path Together
|
||||
|
||||
Rewrite `internal/adapter/http/handler.go` against the root public facade.
|
||||
|
||||
### Handler Boundary And DTO Conversion
|
||||
|
||||
Keep the consumer interface in the HTTP package and narrow it to:
|
||||
|
||||
```go
|
||||
type Runner interface {
|
||||
Run(context.Context, scriptorium.RunRequest) (*scriptorium.RunResult, error)
|
||||
}
|
||||
```
|
||||
|
||||
The public `*Engine` must satisfy it directly. Do not add this interface to the
|
||||
root facade.
|
||||
|
||||
Map request DTOs directly to public values:
|
||||
|
||||
- convert each input to public `ArtifactRef`;
|
||||
- convert model overrides to public `ExecutionTargetOverride`;
|
||||
- retain pointer-valued numeric fields without dereferencing them;
|
||||
- map variables, prompt version, and profile selection; and
|
||||
- continue omitting direct API keys and framework-only request fields from the
|
||||
HTTP DTO.
|
||||
|
||||
Map public artifacts, validation, execution targets, usage, hashes, timestamps,
|
||||
and duration into the existing response DTO without changing serialized names
|
||||
or inclusion rules.
|
||||
|
||||
### Error Mapping
|
||||
|
||||
Replace every internal prompt, profile, artifact, domain, and use-case error
|
||||
check with public framework or Scriptorium HTTP-reader identities. Preserve
|
||||
the existing mapping order:
|
||||
|
||||
1. public `ErrPromptNotFound` and `ErrProfileNotFound`;
|
||||
2. public `ErrProfileRequired` and `ErrAPIKeyEnvMissing`;
|
||||
3. public prompt/profile load errors and general `ErrInvalidRequest`;
|
||||
4. local `ErrFileNotAllowed` or `ErrFileOutsideRoot`, then local
|
||||
`ErrFileTooLarge`;
|
||||
5. public `ErrArtifactLoad`;
|
||||
6. public render, generation, and validation errors; and
|
||||
7. the sanitized internal fallback.
|
||||
|
||||
Keep current status codes, public error codes, and sanitized messages exactly as
|
||||
defined by `docs/api.md`. Check identity with `errors.Is`, never error text.
|
||||
|
||||
### HTTP Tests
|
||||
|
||||
Rewrite the handler fake to use public request and result values. Convert
|
||||
request/response mapping tests and the error table to public sentinels and
|
||||
local HTTP artifact sentinels.
|
||||
|
||||
For tests that need assembled behavior, construct a real public engine from
|
||||
small temporary prompt/profile fixtures and inject only a public LLM fake. Add
|
||||
the new restricted artifact reader in the containment cases. In particular:
|
||||
|
||||
- retain one public-engine test proving reserved provider parameters map to
|
||||
`invalid_request`;
|
||||
- retain end-to-end handler coverage for inline input, file denial without a
|
||||
root, contained file access, outside-root denial, and artifact-size
|
||||
enforcement; and
|
||||
- keep the maintained `examples/http-run.json` request-contract check.
|
||||
|
||||
Do not recreate internal prompt repositories, profile repositories, renderers,
|
||||
or runners in handler tests. Remove redundant assembled assertions already
|
||||
owned by the public engine or focused artifact-reader tests.
|
||||
|
||||
### Complete The CLI Boundary
|
||||
|
||||
After the handler accepts the public run interface, update `serveCommand` to:
|
||||
|
||||
1. construct `httpadapter.NewRestrictedArtifactReader` from the resolved
|
||||
artifact root and byte limit;
|
||||
2. pass it to the public engine helper through
|
||||
`scriptorium.WithArtifactReader`;
|
||||
3. handle reader and engine construction failures as runtime errors; and
|
||||
4. pass the public engine directly to `NewHandlerWithOptions`.
|
||||
|
||||
Then remove `newRunner`, `newRunnerWithArtifactReader`, `newOpenAIClient`, and
|
||||
all remaining Promptkit-destined internal imports from CLI production. The CLI
|
||||
may continue importing Scriptorium-owned configuration, defaults, formatter,
|
||||
and HTTP adapter packages.
|
||||
|
||||
After all CLI and handler production and test consumers use public types:
|
||||
|
||||
- delete `NewRestrictedCompositeReader`,
|
||||
`NewRestrictedCompositeReaderWithLimit`, the denied and restricted file
|
||||
reader implementations, and `ErrFileNotAllowed`, `ErrFileOutsideRoot`, and
|
||||
`ErrFileTooLarge` from `internal/artifact`;
|
||||
- simplify the general framework file reader to retain unrestricted reading
|
||||
without the HTTP byte-limit path; and
|
||||
- remove any obsolete restricted-reader test code left in
|
||||
`internal/artifact/reader_test.go`.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/artifact ./internal/adapter/http
|
||||
go test ./internal/adapter/cli
|
||||
go test -count=20 ./internal/adapter/http
|
||||
go test ./internal/adapter/http -run TestMaintainedHTTPRunExampleMatchesRequestContract
|
||||
```
|
||||
|
||||
### Stage 4 Gate
|
||||
|
||||
- HTTP production and tests use public framework values and errors only.
|
||||
- CLI `serve` injects the Scriptorium-owned public reader into the public engine
|
||||
and passes that engine directly to the handler.
|
||||
- CLI production has no Promptkit-destined internal import.
|
||||
- All HTTP artifact policy lives with the HTTP adapter.
|
||||
- The legacy restricted reader and its Scriptorium-specific errors no longer
|
||||
exist in the Promptkit-destined general artifact package.
|
||||
- The HTTP contract and maintained request example remain unchanged.
|
||||
|
||||
## Stage 5: Enforce The Boundary, Reconcile Documentation, And Validate
|
||||
|
||||
### Dependency Guard
|
||||
|
||||
Add `internal/adapter/dependency_test.go` with package `adapter_test`. Define
|
||||
`TestScriptoriumAdaptersUseOnlyPublicFrameworkBoundary` to inspect direct
|
||||
imports in non-test Go files for:
|
||||
|
||||
- `internal/adapter/cli`;
|
||||
- `internal/adapter/http`; and
|
||||
- `internal/format`.
|
||||
|
||||
Use the standard library parser with `parser.ImportsOnly`, locating the source
|
||||
directories relative to the test file. Do not invoke network tools or inspect
|
||||
transitive dependencies: importing the current public root necessarily reaches
|
||||
the internal framework until extraction.
|
||||
|
||||
Reject direct imports of:
|
||||
|
||||
- `internal/domain`;
|
||||
- `internal/usecase`;
|
||||
- `internal/promptdef`;
|
||||
- `internal/prompt`;
|
||||
- `internal/profile` and `internal/profile/builtin`;
|
||||
- `internal/validate`;
|
||||
- `internal/llm`; and
|
||||
- `internal/artifact`.
|
||||
|
||||
The failure must identify both the importing file and forbidden path. This test
|
||||
is the durable Step 4 boundary owner; do not add duplicate per-package import
|
||||
tests.
|
||||
|
||||
Run a final search over both production and test files. Scriptorium adapter and
|
||||
formatter tests should also use public framework types, although the durable
|
||||
architecture guard needs to enforce production imports only.
|
||||
|
||||
### Documentation Reconciliation
|
||||
|
||||
Update current-behavior documentation only after the boundary is implemented:
|
||||
|
||||
- `docs/consumers/pkg-scriptorium.md`
|
||||
- add `ArtifactReader` and `WithArtifactReader`;
|
||||
- define replacement of the default reader, nil handling, error wrapping, and
|
||||
custom-error preservation;
|
||||
- add `ErrProfileRequired` and `ErrAPIKeyEnvMissing` and state that both also
|
||||
match `ErrInvalidRequest`.
|
||||
- `docs/policy/architecture.md`
|
||||
- state that executable adapters consume framework behavior through the
|
||||
public facade;
|
||||
- retain the current single-repository system shape and avoid presenting the
|
||||
future repository split as complete.
|
||||
- `docs/internal/adapters.md`
|
||||
- replace direct runner/domain composition with public engine construction,
|
||||
public request/result mapping, and adapter-local consumer interfaces;
|
||||
- describe `serve` reader injection and public error mapping.
|
||||
- `docs/internal/sources.md`
|
||||
- distinguish the framework's ordinary reader from the public extension and
|
||||
Scriptorium's HTTP-owned restricted reader;
|
||||
- remove the stale deleted integration-test reference.
|
||||
- `docs/internal/overview.md`
|
||||
- update the adapter, formatter, and split artifact responsibilities.
|
||||
- `docs/internal/runner.md`
|
||||
- clarify that executable adapters reach the runner through the public engine;
|
||||
- remove the stale deleted integration-test reference.
|
||||
|
||||
Review `README.md`, `docs/cli.md`, `docs/config.md`, `docs/api.md`,
|
||||
`docs/operations.md`, and the integration documents. Do not edit them if their
|
||||
observable contract remains accurate; canonical owners should not receive
|
||||
refactor-only churn.
|
||||
|
||||
### Full Validation
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./...
|
||||
go vet ./...
|
||||
build_output="$(mktemp -d)"
|
||||
go build -o "$build_output/scriptorium" ./cmd/scriptorium
|
||||
go test -race . ./internal/adapter/http
|
||||
go test -count=20 . ./internal/adapter/cli ./internal/adapter/http ./internal/format
|
||||
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
|
||||
```
|
||||
|
||||
Also:
|
||||
|
||||
- run the dependency guard explicitly;
|
||||
- verify both maintained configuration files passed through the real loader;
|
||||
- validate every local Markdown link in changed documentation;
|
||||
- confirm no real credentials or private data were added;
|
||||
- inspect `go.mod` and `go.sum` and confirm no dependency change was needed;
|
||||
- confirm no `go.work`, `go.work.sum`, or local `replace` directive was added;
|
||||
and
|
||||
- inspect the final diff for unrelated or out-of-scope changes.
|
||||
|
||||
### Completion Bookkeeping
|
||||
|
||||
After every check passes:
|
||||
|
||||
1. update `step4.md` to mark the target state complete and summarize the
|
||||
achieved boundary without converting it into an implementation log;
|
||||
2. update the Step 4 gate in `migration.md` with the completion date and a
|
||||
concise validation summary;
|
||||
3. mark this implementation plan complete; and
|
||||
4. identify Step 5 repository creation as the next migration gate.
|
||||
|
||||
Do not begin repository creation or extraction in the same change.
|
||||
|
||||
### Stage 5 Gate
|
||||
|
||||
- Every completion criterion in `step4.md` is satisfied.
|
||||
- The dependency guard proves the intended direct-import boundary.
|
||||
- Current documentation describes the implemented single-repository state.
|
||||
- Full tests, vet, build, race checks, maintained examples, links, and
|
||||
whitespace validation pass.
|
||||
- The main roadmap records Step 4 as complete and Step 5 as next.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None.
|
||||
396
docs/roadmap/step4.md
Normal file
396
docs/roadmap/step4.md
Normal file
@@ -0,0 +1,396 @@
|
||||
# Migration Step 4: Public-Facade Adapter Boundary
|
||||
|
||||
## Status
|
||||
|
||||
Proposed. Migration Steps 1 through 3 are complete; this roadmap defines the
|
||||
required target state for Step 4. The separate
|
||||
[implementation plan](implementation.md) defines the ordered work.
|
||||
|
||||
## 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.
|
||||
|
||||
## Current Gap
|
||||
|
||||
The public `Engine` facade already supports the ordinary `Prepare` and `Run`
|
||||
workflows, directory and alternate framework sources, injected model clients,
|
||||
public result values, and broad public error classification. The executable
|
||||
adapters do not yet use that boundary consistently:
|
||||
|
||||
- the CLI constructs framework repositories, readers, renderer, validator, and
|
||||
OpenAI-compatible client directly, then calls the internal runner;
|
||||
- the HTTP handler accepts internal domain request and result values;
|
||||
- HTTP error mapping inspects framework-internal sentinels;
|
||||
- the HTTP artifact-containment reader implements the internal artifact-reader
|
||||
interface;
|
||||
- prepared-run formatting accepts an internal domain value; and
|
||||
- adapter tests frequently construct internal runners or use internal domain
|
||||
values.
|
||||
|
||||
Those dependencies would prevent Scriptorium from compiling after the
|
||||
framework packages move to Promptkit. They also allow the executable to exercise
|
||||
a different composition path from downstream Go consumers.
|
||||
|
||||
## Target State
|
||||
|
||||
At completion, 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.
|
||||
Reference in New Issue
Block a user