scriptorium/docs/roadmap/cleanup.md

Internal Package Audit And Cleanup Plan

This document records the internal package audit findings and the agreed corrections. It is a planning document only; implementation belongs in a later pass.

The package count is not itself a problem. Most internal packages have useful boundaries:

• internal/domain: internal contracts and value types.
• internal/usecase: prompt preparation and execution orchestration.
• internal/adapter/cli and internal/adapter/http: transport-specific mapping and IO.
• internal/config: app settings and CLI override precedence.
• internal/promptdef, internal/profile, and internal/profile/builtin: prompt/profile repositories and built-in profile source.
• internal/artifact: artifact reference resolution.
• internal/prompt: prompt rendering.
• internal/llm: OpenAI-compatible model adapter.
• internal/validate: output validation.
• internal/format: prepared-run render formatting.

Do not collapse packages merely to reduce the number of directories. Prefer targeted cleanup where a boundary is unclear, duplicated, or security-sensitive.

Stage 1: Harden HTTP File Artifact Inputs

Problem:

internal/adapter/http accepts file input references and maps them directly to domain.ArtifactRef. internal/artifact then reads ref.URI with os.ReadFile. Because serve has no built-in authentication, an HTTP caller can cause the server process to read arbitrary local files accessible to it.

Decision:

Keep HTTP file inputs because they are a core use case, but require an explicit allowlisted artifact root for HTTP file reads and enforce path containment before reading.

Implementation:

1. Add an HTTP/server artifact root setting at the app wiring layer.
   • Prefer a config field such as server.artifact_root or a clearly named top-level field such as artifact_root.
   • Add the matching CLI serve override only if it fits existing config/CLI precedence patterns.
   • The setting should apply to HTTP serve only; do not change CLI run/render file input behavior.
2. Introduce an artifact reader mode that restricts file reads to the configured root.
   • Resolve the configured root with filepath.Abs and filepath.Clean.
   • Resolve each requested file path against the root.
   • Reject paths that escape the root, including .. traversal and absolute paths outside the root.
   • Use filepath.Rel or equivalent path-containment checks; do not rely on string-prefix checks alone.
   • Consider rejecting symlink escapes if the threat model includes untrusted writable artifact roots. If symlinks are allowed, document that the root must not be writable by untrusted users.
3. Preserve inline input behavior.
4. Decide startup behavior:
   • Recommended: if HTTP serve accepts file refs, artifact_root must be configured before file refs are allowed.
   • If no root is configured, HTTP file refs should return 400 artifact_read_failed or a more specific 400 artifact_not_allowed; inline refs should continue to work.
5. Keep public Go API and CLI behavior unchanged unless explicitly using the restricted artifact reader for HTTP wiring.

Tests:

• HTTP tests:
  • inline refs still work without an artifact root;
  • file refs under the artifact root work;
  • relative traversal outside the root is rejected;
  • absolute paths outside the root are rejected;
  • unknown fields and existing HTTP error shapes remain stable.
• Artifact tests:
  • restricted reader accepts contained paths;
  • restricted reader rejects escaped paths;
  • path cleaning and absolute/relative input behavior are covered.
• Config/CLI tests:
  • configured artifact root is loaded and overridden according to normal precedence.

Documentation:

• Update docs/config.md, docs/cli.md, docs/integrations/http-api.md, docs/operations.md, and docs/internal/adapters.md.
• Document only implemented behavior outside roadmap docs.

Stage 2: Consolidate Prompt/Profile YAML Catalog Helpers

Problem:

Prompt and profile repositories duplicate YAML discovery and fs.FS path handling. internal/filecatalog currently handles only OS directory walking, while promptdef and profile each carry similar fs.FS walking, file extension, path cleaning, display path, and stem logic.

Decision:

Expand internal/filecatalog into the shared YAML catalog/path helper for both OS directories and fs.FS roots. This is preferable to deleting it because both prompt and profile loaders need the same behavior.

Implementation:

1. Extend internal/filecatalog with helpers for:
   • sorted YAML discovery under an fs.FS root;
   • clean fs.FS root handling;
   • display paths relative to an fs.FS root;
   • YAML file stem handling for both OS and fs.FS paths.
2. Replace duplicate helpers in internal/promptdef and internal/profile:
   • prompt findPromptDefinitionYAMLFiles;
   • prompt cleanFSRoot;
   • prompt displayPath;
   • prompt isPromptDefinitionYAMLFile;
   • profile findProfileYAMLFiles;
   • profile cleanFSRoot;
   • profile displayPath;
   • profile profileFileStem;
   • profile isProfileYAMLFile.
3. Preserve behavior:
   • recursive nested search;
   • .yaml and .yml support;
   • deterministic ordering;
   • subdirectories remain organizational only;
   • duplicate ID behavior remains unchanged.

Tests:

• Expand internal/filecatalog tests to cover fs.FS roots and display paths.
• Keep prompt/profile repository tests passing unchanged where possible.
• Add regression tests if any ordering or relative-path behavior is easy to break.

Stage 3: Deduplicate Validator Control Flow

Problem:

StandardValidator.Validate and FSValidator.Validate duplicate most validation-mode logic. The meaningful difference is how JSON Schema documents are loaded and compiled.

Decision:

Extract shared validation-mode control flow and isolate schema loading behind a small internal function/interface. Keep the public package boundary unchanged.

Implementation:

1. Introduce an unexported schema validation helper, for example:
   • validateArtifact(ctx, artifact, contract, schemaValidatorFunc);
   • or a small unexported interface that compiles/validates schema-backed instances.
2. Share handling for:
   • context checks;
   • nil artifact errors;
   • none;
   • basic;
   • json;
   • validation result initialization.
3. Keep source-specific behavior separate:
   • directory-backed schema path resolution and jsonschema.Compile;
   • fs.FS schema document loading and compiler.AddResource.
4. Preserve current validation semantics:
   • content validation failures return ValidationFailed results;
   • schema path/load/compile/runtime failures return errors;
   • JSON parse failures in json and json_schema modes return failed validation results, not runtime errors.

Tests:

• Existing validator tests should remain the main regression suite.
• Add focused tests only if new helper behavior is not covered by current tests.
• Run go test ./internal/validate ./....

Stage 4: Remove Stale Domain Surface

Problem:

domain.RunResult.Error and domain.RunMetadata appear unused. They make the central domain package broader than necessary and can confuse future contributors about intended result/error handling.

Decision:

Remove unused internal domain fields/types unless implementation-time review finds a live consumer.

Implementation:

1. Remove Error error from domain.RunResult.
2. Remove domain.RunMetadata.
3. Update tests, formatters, adapters, and public conversions only if compilation reveals hidden references.
4. Do not remove fields from public root-package types in this stage.

Tests:

• Run go test ./....

Stage 5: Reassess Thin Interface Files Without Collapsing Packages

Problem:

Several packages have small interface-only files, such as:

• internal/llm/client.go
• internal/prompt/renderer.go
• internal/validate/validator.go
• internal/profile/repository.go
• internal/promptdef/repository.go

Decision:

Keep these package boundaries. The interfaces describe real use-case seams and are imported by adapters and internal/usecase. Do not merge these packages solely to remove small files.

Implementation:

1. No code change is required for package consolidation.
2. If a package is already being edited, it is acceptable to move an interface into a nearby implementation file only when that improves readability and does not blur ownership.
3. Keep internal/domain free of adapter implementation details.
4. Keep internal/usecase dependent on interfaces rather than concrete filesystem/model implementations.

Stage 6: Documentation And Final Verification

After implementation:

1. Update internal docs:

   • docs/internal/adapters.md for HTTP artifact-root behavior and any filecatalog/package-boundary changes;
   • docs/internal/runner.md only if runner behavior or error classification changed as part of this cleanup.

2. Update user/operator docs for HTTP artifact root:

   • docs/config.md;
   • docs/cli.md;
   • docs/integrations/http-api.md;
   • docs/operations.md;
   • docs/troubleshooting.md if new failure modes are introduced.

3. Run:

   go test ./...
   go vet ./...
   go run ./examples/go-library/prepare
   go run ./cmd/scriptorium render \
     --config ./examples/config.yml \
     --prompt generic.markdown_summary \
     --input transcript=./examples/fixtures/transcript.md \
     --input glossary=./examples/fixtures/glossary.yml \
     --format json
   

Non-Goals

• Do not collapse packages merely because they are small.
• Do not remove HTTP file inputs; restrict them with an explicit artifact root.
• Do not change public Go API behavior as part of internal package cleanup.
• Do not introduce a new auth system for HTTP serve in this cleanup pass.
• Do not change prompt/profile YAML lookup semantics except by preserving them through shared helpers.