Revise the implementation plan to address remaining step 4 gaps
This commit is contained in:
@@ -2,510 +2,228 @@
|
|||||||
|
|
||||||
## Status
|
## Status
|
||||||
|
|
||||||
Complete as of 2026-07-28. All implementation gates, the dependency guard,
|
Follow-up remediation is required. The public-facade adapter boundary is
|
||||||
documentation reconciliation, and required validation passed. Migration Step 5
|
implemented, but the post-implementation review found an error-classification
|
||||||
repository creation is the next gate; it was not started here.
|
gap, a machine-dependent HTTP artifact test, and an incomplete architecture
|
||||||
|
guard. Migration Step 4 is not fully revalidated until the stages below pass.
|
||||||
|
|
||||||
## Objective
|
## Objective
|
||||||
|
|
||||||
Implement the target state in the
|
Complete the target state in the
|
||||||
[Step 4 public-facade adapter roadmap](step4.md): make Scriptorium's CLI, HTTP
|
[Step 4 public-facade adapter roadmap](step4.md) by correcting the review
|
||||||
handler, prepared-run formatter, and HTTP artifact policy consume only the
|
findings without changing the intended CLI, HTTP, or public Go contracts.
|
||||||
public framework facade, while preserving the current executable contracts.
|
|
||||||
|
|
||||||
The [accepted split decision](../adr/0002-split-promptkit-from-scriptorium.md)
|
The [accepted split decision](../adr/0002-split-promptkit-from-scriptorium.md)
|
||||||
owns the long-term Promptkit/Scriptorium boundary. The
|
owns the long-term Promptkit/Scriptorium boundary. The
|
||||||
[testing policy](../policy/testing.md) governs test value and ownership. The
|
[testing policy](../policy/testing.md) governs test value and ownership. The
|
||||||
[documentation policy](../policy/documentation.md) governs current-behavior
|
[documentation policy](../policy/documentation.md) governs completion
|
||||||
updates and canonical ownership.
|
bookkeeping and canonical ownership.
|
||||||
|
|
||||||
## Constraints
|
## Constraints
|
||||||
|
|
||||||
- Execute the stages in order and keep the repository buildable and testable at
|
- Keep this work in the Scriptorium repository. Do not modify the sibling
|
||||||
every stage gate.
|
Promptkit repository or infer that Migration Step 5 is complete merely from
|
||||||
- Do not create the Promptkit repository, change the module path, rename the
|
its local presence.
|
||||||
root package, or move framework packages out of this repository.
|
- Preserve the public `ArtifactReader`, error sentinel, CLI, HTTP, containment,
|
||||||
- Do not export internal repositories, domain values, renderers, validators,
|
artifact-size, formatting, timeout, credential, and redaction contracts.
|
||||||
the runner, or adapter DTOs.
|
- Do not add compatibility shims, begin framework extraction, change the module
|
||||||
- Add only the public artifact-reader extension and public error identities
|
path, or add dependencies.
|
||||||
required by the Step 4 roadmap.
|
- Keep tests deterministic, offline, independent of machine MIME databases,
|
||||||
- Preserve CLI, application configuration, HTTP, containment, validation,
|
and focused at the narrowest stable owner.
|
||||||
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.
|
- Preserve unrelated working-tree changes.
|
||||||
- Do not begin Migration Step 5 in this change.
|
|
||||||
|
|
||||||
## Stage 1: Complete The Public Framework Boundary
|
## Completed Work
|
||||||
|
|
||||||
Add the public artifact-reader extension and error identities before changing
|
The original five implementation stages established the Step 4 boundary:
|
||||||
any adapter consumer.
|
|
||||||
|
|
||||||
### Public Artifact Reader
|
- 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.
|
||||||
|
|
||||||
In `types.go`, add:
|
The post-implementation review confirmed the production dependency direction
|
||||||
|
but found that the completion gate needs the corrections below.
|
||||||
|
|
||||||
```go
|
## Stage 1: Preserve The Active Public Error Category
|
||||||
type ArtifactReader interface {
|
|
||||||
Read(context.Context, ArtifactRef) (*Artifact, error)
|
|
||||||
}
|
|
||||||
```
|
|
||||||
|
|
||||||
In `engine.go`, add `WithArtifactReader(ArtifactReader) Option`. Extend
|
Correct `mapPublicError` so a public error returned by an injected collaborator
|
||||||
`engineOptions` with an internal artifact-reader slot and source-presence flag,
|
does not suppress the public category added by the operation currently being
|
||||||
parallel to the existing LLM and source options. A supplied reader replaces
|
performed.
|
||||||
`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
|
The current early return from `hasPublicError` is too broad: for example, an
|
||||||
errors wrapped by `NewEngine` consistently with the existing option behavior.
|
artifact reader that returns `ErrInvalidRequest` is wrapped by the runner's
|
||||||
|
internal artifact-load category, but the facade returns before adding public
|
||||||
|
`ErrArtifactLoad`. The corrected mapping must:
|
||||||
|
|
||||||
Create `artifact_reader.go` in the root facade with a private adapter that
|
- first classify the internal operation or source category present in the
|
||||||
implements `internal/artifact.Reader` by:
|
complete chain;
|
||||||
|
- add the corresponding public category even when another public sentinel is
|
||||||
|
already nested in the chain;
|
||||||
|
- retain the original collaborator error and every existing internal cause for
|
||||||
|
`errors.Is`;
|
||||||
|
- preserve the dual `ErrInvalidRequest` plus specific-sentinel behavior for
|
||||||
|
`ErrProfileRequired` and `ErrAPIKeyEnvMissing`; and
|
||||||
|
- return an already-public or otherwise unclassified error unchanged when no
|
||||||
|
internal category requires translation.
|
||||||
|
|
||||||
1. converting the internal reference to a public `ArtifactRef`;
|
Remove the blanket public-error short circuit rather than adding an
|
||||||
2. calling the supplied public reader with the original context;
|
artifact-specific exception. Keep `publicErrorFor` responsible for selecting
|
||||||
3. returning the reader error unchanged so the runner and facade can add their
|
the outer public category from internal identities, with specific causes
|
||||||
normal artifact-load categories while preserving `errors.Is`;
|
checked before broad internal categories. Do not parse error text or change
|
||||||
4. rejecting `(nil, nil)` with a descriptive non-sentinel error so the runner
|
public method signatures.
|
||||||
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
|
### Regression Tests
|
||||||
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
|
Extend the public engine contract tests at the real `Engine.Prepare` and
|
||||||
|
`Engine.Run` boundaries:
|
||||||
|
|
||||||
Add public `ErrProfileRequired` and `ErrAPIKeyEnvMissing` sentinels alongside
|
- make an injected artifact reader return `ErrInvalidRequest` and assert that
|
||||||
the existing facade errors.
|
the result matches both `ErrArtifactLoad` and the original
|
||||||
|
`ErrInvalidRequest`;
|
||||||
|
- make an injected LLM client return a different public sentinel, such as
|
||||||
|
`ErrArtifactLoad`, and assert that the result matches both
|
||||||
|
`ErrLLMGenerate` and the original sentinel; and
|
||||||
|
- retain the existing generic reader-error, nil-response, cancellation,
|
||||||
|
profile-required, and missing-credential cases without duplicating them.
|
||||||
|
|
||||||
Update `mapPublicError` so:
|
The second case protects the shared translation rule rather than only the
|
||||||
|
reported artifact example. Do not add a cross-product of every public sentinel
|
||||||
- a missing explicit/default profile matches both `ErrInvalidRequest` and
|
and failure category.
|
||||||
`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:
|
Run:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
go test .
|
go test .
|
||||||
go test -count=20 -run 'Test.*(ArtifactReader|ProfileRequired|MissingCredentials)' .
|
go test -count=20 -run 'Test.*(ArtifactReader|PublicError|LLM)' .
|
||||||
```
|
```
|
||||||
|
|
||||||
### Stage 1 Gate
|
### Stage 1 Gate
|
||||||
|
|
||||||
- The public artifact-reader option has the exact contract defined in
|
- Every categorized runner failure gains the public category for its active
|
||||||
`step4.md`.
|
operation even when a nested collaborator error is already public.
|
||||||
- Public error chains retain the two HTTP-required distinctions and their broad
|
- Nested custom error identities remain observable through `errors.Is`.
|
||||||
invalid-request category.
|
- Existing broad and specific public error behavior remains unchanged.
|
||||||
- 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
|
## Stage 2: Make HTTP Artifact MIME Coverage Deterministic
|
||||||
|
|
||||||
Create `internal/adapter/http/artifact_reader.go`. It must implement the new
|
Repair `TestRestrictedArtifactReaderReadsContainedFiles` without changing the
|
||||||
public `scriptorium.ArtifactReader` interface directly and must not import
|
implemented MIME policy. The contract remains:
|
||||||
`internal/domain`, the Promptkit-destined `internal/artifact`, or framework
|
|
||||||
defaults.
|
|
||||||
|
|
||||||
Expose an internal-repository constructor for the CLI adapter:
|
- call `mime.TypeByExtension` for a file extension; and
|
||||||
|
- use `text/plain` only when that lookup returns an empty string.
|
||||||
|
|
||||||
```go
|
Replace the `.md` fixture with an extension in Go's built-in MIME table, such
|
||||||
func NewRestrictedArtifactReader(root string, maxBytes int64) (scriptorium.ArtifactReader, error)
|
as `.html`, and assert the corresponding non-empty built-in result. Retain the
|
||||||
```
|
separate unknown-extension case that asserts the `text/plain` fallback. Keep
|
||||||
|
the contained relative and absolute path cases and all metadata assertions.
|
||||||
|
|
||||||
Keep these sentinels in the HTTP adapter package:
|
Do not hard-code a platform-specific Markdown media type, consult system MIME
|
||||||
|
files directly, mutate the process-global MIME registry, or weaken the test to
|
||||||
- `ErrFileNotAllowed`;
|
accept either value. Change production code only if the deterministic test
|
||||||
- `ErrFileOutsideRoot`; and
|
demonstrates that it does not implement the stated lookup-and-fallback rule.
|
||||||
- `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:
|
Run:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
go test ./internal/artifact ./internal/adapter/http
|
go test ./internal/adapter/http
|
||||||
|
go test -count=20 ./internal/adapter/http
|
||||||
```
|
```
|
||||||
|
|
||||||
### Stage 2 Gate
|
### Stage 2 Gate
|
||||||
|
|
||||||
- The new HTTP reader independently satisfies the public reader contract and
|
- The HTTP package passes repeatedly on a machine whose MIME database has no
|
||||||
preserves every documented HTTP containment outcome.
|
Markdown registration.
|
||||||
- Its focused tests use no framework-internal type.
|
- The test independently protects successful MIME inference and the unknown
|
||||||
- The old restricted implementation remains only as a temporary build bridge
|
extension fallback.
|
||||||
with a mandatory deletion point in Stage 4.
|
- Artifact metadata, containment, and byte-limit behavior are unchanged.
|
||||||
|
|
||||||
## Stage 3: Rewrite Formatting And Move CLI Run And Render
|
## Stage 3: Strengthen The Public-Boundary Dependency Guard
|
||||||
|
|
||||||
Update `internal/format/prepared_run.go` so every formatter interface and
|
Update `internal/adapter/dependency_test.go` so the durable guard proves the
|
||||||
function accepts `*scriptorium.PreparedRun` rather than
|
entire intended production boundary rather than only today's flat directory
|
||||||
`*domain.PreparedRun`. Replace internal rendered-message and target references
|
layout.
|
||||||
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
|
### Recursive Source Inspection
|
||||||
the existing behavioral assertions for:
|
|
||||||
|
|
||||||
- deterministic text formatting;
|
Walk these roots recursively with `filepath.WalkDir`:
|
||||||
- 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/cli`;
|
||||||
- `internal/adapter/http`; and
|
- `internal/adapter/http`; and
|
||||||
- `internal/format`.
|
- `internal/format`.
|
||||||
|
|
||||||
Use the standard library parser with `parser.ImportsOnly`, locating the source
|
Inspect every non-test `.go` file under those roots. Skip directories only when
|
||||||
directories relative to the test file. Do not invoke network tools or inspect
|
they are not Go source owned by the repository; do not silently skip nested
|
||||||
transitive dependencies: importing the current public root necessarily reaches
|
adapter or formatter packages. Continue parsing imports with the standard
|
||||||
the internal framework until extraction.
|
library and `parser.ImportsOnly`.
|
||||||
|
|
||||||
Reject direct imports of:
|
### Forbidden Package Families
|
||||||
|
|
||||||
|
Represent each Promptkit-destined internal dependency as a package-family root:
|
||||||
|
|
||||||
- `internal/domain`;
|
- `internal/domain`;
|
||||||
- `internal/usecase`;
|
- `internal/usecase`;
|
||||||
- `internal/promptdef`;
|
- `internal/promptdef`;
|
||||||
- `internal/prompt`;
|
- `internal/prompt`;
|
||||||
- `internal/profile` and `internal/profile/builtin`;
|
- `internal/profile`;
|
||||||
- `internal/validate`;
|
- `internal/validate`;
|
||||||
- `internal/llm`; and
|
- `internal/llm`; and
|
||||||
- `internal/artifact`.
|
- `internal/artifact`.
|
||||||
|
|
||||||
The failure must identify both the importing file and forbidden path. This test
|
Reject an import when it equals a forbidden root or begins with that root plus
|
||||||
is the durable Step 4 boundary owner; do not add duplicate per-package import
|
`/`. The `internal/profile` rule therefore covers `internal/profile/builtin`
|
||||||
tests.
|
and any future descendant. Do not reject the public root package or
|
||||||
|
Scriptorium-owned adapter, configuration, defaults, and formatter packages.
|
||||||
|
|
||||||
Run a final search over both production and test files. Scriptorium adapter and
|
Diagnostics must identify the importing file and complete forbidden import
|
||||||
formatter tests should also use public framework types, although the durable
|
path. Continue excluding `_test.go` files from the durable production
|
||||||
architecture guard needs to enforce production imports only.
|
architecture rule; test-boundary review remains a separate source inspection.
|
||||||
|
|
||||||
### Documentation Reconciliation
|
### Guard Regression Test
|
||||||
|
|
||||||
Update current-behavior documentation only after the boundary is implemented:
|
Extract only the small scanning or classification helper needed to exercise
|
||||||
|
the guard. Using `t.TempDir`, create a nested Go source fixture that imports a
|
||||||
- `docs/consumers/pkg-scriptorium.md`
|
descendant of a forbidden package family and assert that the guard reports it.
|
||||||
- add `ArtifactReader` and `WithArtifactReader`;
|
Also include an allowed public-facade import so the test proves it is not
|
||||||
- define replacement of the default reader, nil handling, error wrapping, and
|
overbroad. Keep this fixture test in the existing dependency-test file rather
|
||||||
custom-error preservation;
|
than creating another architecture-test owner.
|
||||||
- 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:
|
Run:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
go test ./internal/adapter -run TestScriptoriumAdaptersUseOnlyPublicFrameworkBoundary
|
||||||
|
go test -count=20 ./internal/adapter
|
||||||
|
```
|
||||||
|
|
||||||
|
### Stage 3 Gate
|
||||||
|
|
||||||
|
- Nested production packages cannot bypass the scan.
|
||||||
|
- Current or future descendants of forbidden framework packages cannot bypass
|
||||||
|
package-family matching.
|
||||||
|
- Allowed public and Scriptorium-owned dependencies remain accepted.
|
||||||
|
- Failures remain precise and actionable.
|
||||||
|
|
||||||
|
## Stage 4: Revalidate And Reconcile Completion Records
|
||||||
|
|
||||||
|
After Stages 1 through 3 pass, run the complete Step 4 validation from the
|
||||||
|
Scriptorium repository:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
go test ./...
|
go test ./...
|
||||||
go vet ./...
|
go vet ./...
|
||||||
build_output="$(mktemp -d)"
|
build_output="$(mktemp -d)"
|
||||||
go build -o "$build_output/scriptorium" ./cmd/scriptorium
|
go build -o "$build_output/scriptorium" ./cmd/scriptorium
|
||||||
go test -race . ./internal/adapter/http
|
go test -race . ./internal/adapter/http
|
||||||
go test -count=20 . ./internal/adapter/cli ./internal/adapter/http ./internal/format
|
go test -count=20 . ./internal/adapter ./internal/adapter/cli ./internal/adapter/http ./internal/format
|
||||||
go test ./internal/adapter/http -run TestMaintainedHTTPRunExampleMatchesRequestContract
|
go test ./internal/adapter/http -run TestMaintainedHTTPRunExampleMatchesRequestContract
|
||||||
bash ./examples/render-markdown-summary.sh
|
bash ./examples/render-markdown-summary.sh
|
||||||
go run ./cmd/scriptorium render \
|
go run ./cmd/scriptorium render \
|
||||||
@@ -520,36 +238,38 @@ git diff --check
|
|||||||
|
|
||||||
Also:
|
Also:
|
||||||
|
|
||||||
- run the dependency guard explicitly;
|
- pass both maintained configuration files through the real configuration
|
||||||
- verify both maintained configuration files passed through the real loader;
|
loader;
|
||||||
- validate every local Markdown link in changed documentation;
|
- inspect production and test imports under the adapter and formatter roots;
|
||||||
- confirm no real credentials or private data were added;
|
- validate every local Markdown link changed by the remediation;
|
||||||
- inspect `go.mod` and `go.sum` and confirm no dependency change was needed;
|
- confirm no credential, private data, dependency, module, workspace, or local
|
||||||
- confirm no `go.work`, `go.work.sum`, or local `replace` directive was added;
|
`replace` change was introduced; and
|
||||||
and
|
- inspect the final diff for unrelated work.
|
||||||
- inspect the final diff for unrelated or out-of-scope changes.
|
|
||||||
|
|
||||||
### Completion Bookkeeping
|
Once every check passes:
|
||||||
|
|
||||||
After every check passes:
|
1. update the Step 4 completion summary in `step4.md` to mention the
|
||||||
|
post-implementation error and guard corrections without turning the feature
|
||||||
|
roadmap into an implementation log;
|
||||||
|
2. update the Step 4 gate in `migration.md` so its validation claim accurately
|
||||||
|
reflects the successful follow-up pass;
|
||||||
|
3. mark this implementation plan complete and move the remediation stages into
|
||||||
|
the completed-work summary; and
|
||||||
|
4. leave the Step 5 gate unchanged unless its out-of-band repository,
|
||||||
|
governance, policy, and CI requirements have been independently confirmed.
|
||||||
|
|
||||||
1. update `step4.md` to mark the target state complete and summarize the
|
Do not begin extraction or modify Promptkit as part of this stage.
|
||||||
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 4 Gate
|
||||||
|
|
||||||
### Stage 5 Gate
|
- Every Step 4 completion criterion is satisfied.
|
||||||
|
- The public error contract is protected against nested public-sentinel
|
||||||
- Every completion criterion in `step4.md` is satisfied.
|
collisions.
|
||||||
- The dependency guard proves the intended direct-import boundary.
|
- The default suite is deterministic across machine MIME configurations.
|
||||||
- Current documentation describes the implemented single-repository state.
|
- The dependency guard enforces recursive package-family boundaries.
|
||||||
- Full tests, vet, build, race checks, maintained examples, links, and
|
- Full tests, vet, build, race checks, repeated tests, maintained examples,
|
||||||
whitespace validation pass.
|
configuration loading, links, and whitespace validation pass.
|
||||||
- The main roadmap records Step 4 as complete and Step 5 as next.
|
- Scriptorium's completion records accurately describe the validated state.
|
||||||
|
|
||||||
## Open Questions
|
## Open Questions
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user