222 lines
9.7 KiB
Markdown
222 lines
9.7 KiB
Markdown
# 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:
|
|
|
|
```bash
|
|
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.
|