Record public facade boundary validation
This commit is contained in:
@@ -2,16 +2,14 @@
|
||||
|
||||
## Status
|
||||
|
||||
Follow-up remediation is required. The public-facade adapter boundary is
|
||||
implemented, but the post-implementation review found an error-classification
|
||||
gap, a machine-dependent HTTP artifact test, and an incomplete architecture
|
||||
guard. Migration Step 4 is not fully revalidated until the stages below pass.
|
||||
Complete as of 2026-07-28. The public-facade adapter boundary and its
|
||||
post-implementation remediation are fully revalidated.
|
||||
|
||||
## Objective
|
||||
|
||||
Complete the target state in the
|
||||
[Step 4 public-facade adapter roadmap](step4.md) by correcting the review
|
||||
findings without changing the intended CLI, HTTP, or public Go contracts.
|
||||
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
|
||||
@@ -34,7 +32,7 @@ bookkeeping and canonical ownership.
|
||||
|
||||
## Completed Work
|
||||
|
||||
The original five implementation stages established the Step 4 boundary:
|
||||
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
|
||||
@@ -51,226 +49,14 @@ The original five implementation stages established the Step 4 boundary:
|
||||
repeated tests, maintained examples, configuration loading, links, and
|
||||
whitespace.
|
||||
|
||||
The post-implementation review confirmed the production dependency direction
|
||||
but found that the completion gate needs the corrections below.
|
||||
The post-implementation remediation is also complete:
|
||||
|
||||
## Stage 1: Preserve The Active Public Error Category
|
||||
|
||||
Correct `mapPublicError` so a public error returned by an injected collaborator
|
||||
does not suppress the public category added by the operation currently being
|
||||
performed.
|
||||
|
||||
The current early return from `hasPublicError` is too broad: for example, an
|
||||
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:
|
||||
|
||||
- first classify the internal operation or source category present in the
|
||||
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.
|
||||
|
||||
Remove the blanket public-error short circuit rather than adding an
|
||||
artifact-specific exception. Keep `publicErrorFor` responsible for selecting
|
||||
the outer public category from internal identities, with specific causes
|
||||
checked before broad internal categories. Do not parse error text or change
|
||||
public method signatures.
|
||||
|
||||
### Regression Tests
|
||||
|
||||
Extend the public engine contract tests at the real `Engine.Prepare` and
|
||||
`Engine.Run` boundaries:
|
||||
|
||||
- make an injected artifact reader return `ErrInvalidRequest` and assert that
|
||||
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.
|
||||
|
||||
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
|
||||
and failure category.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test .
|
||||
go test -count=20 -run 'Test.*(ArtifactReader|PublicError|LLM)' .
|
||||
```
|
||||
|
||||
### Stage 1 Gate
|
||||
|
||||
- Every categorized runner failure gains the public category for its active
|
||||
operation even when a nested collaborator error is already public.
|
||||
- Nested custom error identities remain observable through `errors.Is`.
|
||||
- Existing broad and specific public error behavior remains unchanged.
|
||||
|
||||
## Stage 2: Make HTTP Artifact MIME Coverage Deterministic
|
||||
|
||||
Repair `TestRestrictedArtifactReaderReadsContainedFiles` without changing the
|
||||
implemented MIME policy. The contract remains:
|
||||
|
||||
- call `mime.TypeByExtension` for a file extension; and
|
||||
- use `text/plain` only when that lookup returns an empty string.
|
||||
|
||||
Replace the `.md` fixture with an extension in Go's built-in MIME table, such
|
||||
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.
|
||||
|
||||
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
|
||||
accept either value. Change production code only if the deterministic test
|
||||
demonstrates that it does not implement the stated lookup-and-fallback rule.
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
go test ./internal/adapter/http
|
||||
go test -count=20 ./internal/adapter/http
|
||||
```
|
||||
|
||||
### Stage 2 Gate
|
||||
|
||||
- The HTTP package passes repeatedly on a machine whose MIME database has no
|
||||
Markdown registration.
|
||||
- The test independently protects successful MIME inference and the unknown
|
||||
extension fallback.
|
||||
- Artifact metadata, containment, and byte-limit behavior are unchanged.
|
||||
|
||||
## Stage 3: Strengthen The Public-Boundary Dependency Guard
|
||||
|
||||
Update `internal/adapter/dependency_test.go` so the durable guard proves the
|
||||
entire intended production boundary rather than only today's flat directory
|
||||
layout.
|
||||
|
||||
### Recursive Source Inspection
|
||||
|
||||
Walk these roots recursively with `filepath.WalkDir`:
|
||||
|
||||
- `internal/adapter/cli`;
|
||||
- `internal/adapter/http`; and
|
||||
- `internal/format`.
|
||||
|
||||
Inspect every non-test `.go` file under those roots. Skip directories only when
|
||||
they are not Go source owned by the repository; do not silently skip nested
|
||||
adapter or formatter packages. Continue parsing imports with the standard
|
||||
library and `parser.ImportsOnly`.
|
||||
|
||||
### Forbidden Package Families
|
||||
|
||||
Represent each Promptkit-destined internal dependency as a package-family root:
|
||||
|
||||
- `internal/domain`;
|
||||
- `internal/usecase`;
|
||||
- `internal/promptdef`;
|
||||
- `internal/prompt`;
|
||||
- `internal/profile`;
|
||||
- `internal/validate`;
|
||||
- `internal/llm`; and
|
||||
- `internal/artifact`.
|
||||
|
||||
Reject an import when it equals a forbidden root or begins with that root plus
|
||||
`/`. The `internal/profile` rule therefore covers `internal/profile/builtin`
|
||||
and any future descendant. Do not reject the public root package or
|
||||
Scriptorium-owned adapter, configuration, defaults, and formatter packages.
|
||||
|
||||
Diagnostics must identify the importing file and complete forbidden import
|
||||
path. Continue excluding `_test.go` files from the durable production
|
||||
architecture rule; test-boundary review remains a separate source inspection.
|
||||
|
||||
### Guard Regression Test
|
||||
|
||||
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
|
||||
descendant of a forbidden package family and assert that the guard reports it.
|
||||
Also include an allowed public-facade import so the test proves it is not
|
||||
overbroad. Keep this fixture test in the existing dependency-test file rather
|
||||
than creating another architecture-test owner.
|
||||
|
||||
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
|
||||
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 ./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:
|
||||
|
||||
- pass both maintained configuration files through the real configuration
|
||||
loader;
|
||||
- inspect production and test imports under the adapter and formatter roots;
|
||||
- validate every local Markdown link changed by the remediation;
|
||||
- confirm no credential, private data, dependency, module, workspace, or local
|
||||
`replace` change was introduced; and
|
||||
- inspect the final diff for unrelated work.
|
||||
|
||||
Once 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.
|
||||
|
||||
Do not begin extraction or modify Promptkit as part of this stage.
|
||||
|
||||
### Stage 4 Gate
|
||||
|
||||
- Every Step 4 completion criterion is satisfied.
|
||||
- The public error contract is protected against nested public-sentinel
|
||||
collisions.
|
||||
- The default suite is deterministic across machine MIME configurations.
|
||||
- The dependency guard enforces recursive package-family boundaries.
|
||||
- Full tests, vet, build, race checks, repeated tests, maintained examples,
|
||||
configuration loading, links, and whitespace validation pass.
|
||||
- Scriptorium's completion records accurately describe the validated state.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None.
|
||||
- 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.
|
||||
|
||||
@@ -175,8 +175,10 @@ framework behavior, and all tests and documented smoke commands pass.
|
||||
**Gate status:** Complete as of 2026-07-28. CLI `run`, `render`, and `serve`,
|
||||
the HTTP handler, and prepared-run formatting use the public facade; the
|
||||
restricted HTTP reader is injected through the public extension point. The
|
||||
dependency guard, full tests, vet, build, race checks, maintained examples, and
|
||||
configuration smoke checks passed. Step 5 repository creation is next.
|
||||
post-implementation public-error, deterministic MIME, and recursive
|
||||
dependency-guard corrections passed full tests, vet, build, race checks,
|
||||
maintained examples, and configuration smoke checks. Step 5 repository
|
||||
creation is next.
|
||||
|
||||
### Step 5: Create The Promptkit Repository
|
||||
|
||||
|
||||
@@ -2,10 +2,11 @@
|
||||
|
||||
## Status
|
||||
|
||||
Complete as of 2026-07-28. The public-facade adapter boundary is established;
|
||||
the [implementation plan](implementation.md) records the completed work, and
|
||||
the [main migration roadmap](migration.md) identifies repository creation as
|
||||
the next gate.
|
||||
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
|
||||
|
||||
@@ -26,7 +27,8 @@ 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 protects the direct-import boundary.
|
||||
dependency test recursively protects production adapter and formatter packages
|
||||
from framework-owned imports.
|
||||
|
||||
## Target State
|
||||
|
||||
|
||||
Reference in New Issue
Block a user