Add an implementation plan to reflect follow-up findings from the audit
This commit is contained in:
@@ -1,221 +1,324 @@
|
|||||||
# Internal Package Audit And Cleanup Plan
|
# Full Codebase Cleanup And Hardening 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.
|
This document is a decision-complete implementation plan for the current full-codebase audit findings. It is a planning document only. Implementation should proceed in stages and should not change unrelated behavior.
|
||||||
|
|
||||||
The package count is not itself a problem. Most internal packages have useful boundaries:
|
The goal is to polish and harden the public and internal code paths without expanding Scriptorium's scope. Keep the existing package boundaries unless a stage explicitly calls for a helper extraction.
|
||||||
|
|
||||||
- `internal/domain`: internal contracts and value types.
|
## Guiding Decisions
|
||||||
- `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.
|
- Treat configured `fs.FS` roots as real containment boundaries for public source options.
|
||||||
|
- Keep local directory-backed CLI behavior compatible unless this plan explicitly names a change.
|
||||||
|
- Keep HTTP `serve` minimal, but safe by default against accidental large request, file, and response bodies.
|
||||||
|
- Do not add HTTP authentication in this cleanup pass.
|
||||||
|
- Do not add new dependencies.
|
||||||
|
- Document only implemented behavior outside `docs/roadmap/`.
|
||||||
|
|
||||||
## Stage 1: Harden HTTP File Artifact Inputs
|
## Stage 1: Enforce Source-Root Containment For `fs.FS` Prompt And Schema Sources
|
||||||
|
|
||||||
Problem:
|
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.
|
Public source options such as `WithPromptFS(fsys, root)` and `WithSchemaFS(fsys, root)` describe `root` as the source boundary, but prompt `content_file` and schema path resolution can clean `..` paths above that root when the supplied `fs.FS` is broader than the configured root.
|
||||||
|
|
||||||
Decision:
|
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.
|
For public `fs.FS` source options, the configured root is a containment boundary. Prompt `content_file` paths and schema paths must resolve inside that root. Absolute paths and relative traversal that escape the root are invalid.
|
||||||
|
|
||||||
Implementation:
|
Implementation:
|
||||||
|
|
||||||
1. Add an HTTP/server artifact root setting at the app wiring layer.
|
1. Add a small shared internal helper for `fs.FS` path resolution.
|
||||||
- Prefer a config field such as `server.artifact_root` or a clearly named top-level field such as `artifact_root`.
|
- Prefer `internal/filecatalog` if the helper naturally belongs with existing clean/display path utilities.
|
||||||
- Add the matching CLI `serve` override only if it fits existing config/CLI precedence patterns.
|
- Inputs should include a source root and a user path.
|
||||||
- The setting should apply to HTTP `serve` only; do not change CLI `run`/`render` file input behavior.
|
- It should trim whitespace, clean slash paths with `path.Clean`, reject empty paths where the caller requires a file, reject absolute paths, and reject any path whose clean form escapes the clean root.
|
||||||
2. Introduce an artifact reader mode that restricts file reads to the configured root.
|
- Use path-component checks, not string-prefix checks alone.
|
||||||
- Resolve the configured root with `filepath.Abs` and `filepath.Clean`.
|
- Return both the resolved `fs.FS` path and a display path when useful for errors.
|
||||||
- Resolve each requested file path against the root.
|
2. Update `internal/promptdef` `fsRepository` content-file loading.
|
||||||
- Reject paths that escape the root, including `..` traversal and absolute paths outside the root.
|
- `content_file: ./local.tmpl` beside the prompt should continue to work.
|
||||||
- Use `filepath.Rel` or equivalent path-containment checks; do not rely on string-prefix checks alone.
|
- Nested prompt files should keep the existing relative-to-prompt-file behavior.
|
||||||
- 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.
|
- `content_file` values that escape the configured `WithPromptFS` root should return a prompt-load error.
|
||||||
3. Preserve inline input behavior.
|
3. Update `internal/validate` `FSValidator` schema resolution.
|
||||||
4. Decide startup behavior:
|
- `WithSchemaFS(fsys, root)` should allow schema paths inside `root`.
|
||||||
- Recommended: if HTTP `serve` accepts `file` refs, `artifact_root` must be configured before file refs are allowed.
|
- `WithSchemaFile(path)` should keep the existing single-file behavior: prompt `schema_path` must match the selected file's base name.
|
||||||
- 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.
|
- Schema paths that escape the configured root should return validation/schema-load errors.
|
||||||
5. Keep public Go API and CLI behavior unchanged unless explicitly using the restricted artifact reader for HTTP wiring.
|
4. Preserve directory-backed compatibility unless a failing test reveals an inconsistency that must be fixed.
|
||||||
|
- `WithPromptFile(path)` should continue resolving `content_file` values relative to the selected prompt file's directory.
|
||||||
|
- `Config.SchemaDir` / CLI `--schema-dir` should keep documented behavior, including absolute `schema_path` support, because these are operator-controlled local filesystem paths.
|
||||||
|
|
||||||
Tests:
|
Tests:
|
||||||
|
|
||||||
- HTTP tests:
|
- Add `internal/promptdef` tests for `fs.FS` `content_file` traversal:
|
||||||
- inline refs still work without an artifact root;
|
- sibling file inside root succeeds;
|
||||||
- file refs under the artifact root work;
|
- nested file inside root succeeds;
|
||||||
- relative traversal outside the root is rejected;
|
- `../outside.tmpl` from a prompt under the root is rejected;
|
||||||
- absolute paths outside the root are rejected;
|
- absolute-style `/outside.tmpl` is rejected.
|
||||||
- unknown fields and existing HTTP error shapes remain stable.
|
- Add public package tests through `WithPromptFS` proving escaped `content_file` returns `ErrPromptLoad`.
|
||||||
- Artifact tests:
|
- Add `internal/validate` tests for `WithSchemaFS` traversal:
|
||||||
- restricted reader accepts contained paths;
|
- schema inside root succeeds;
|
||||||
- restricted reader rejects escaped paths;
|
- `../outside.schema.json` is rejected;
|
||||||
- path cleaning and absolute/relative input behavior are covered.
|
- absolute-style paths are rejected.
|
||||||
- Config/CLI tests:
|
- Keep existing `WithSchemaFile` tests passing.
|
||||||
- configured artifact root is loaded and overridden according to normal precedence.
|
|
||||||
|
|
||||||
Documentation:
|
Documentation:
|
||||||
|
|
||||||
- Update `docs/config.md`, `docs/cli.md`, `docs/integrations/http-api.md`, `docs/operations.md`, and `docs/internal/adapters.md`.
|
- Update `docs/consumers/pkg-scriptorium.md` to state that `WithPromptFS` and `WithSchemaFS` roots are containment boundaries.
|
||||||
- Document only implemented behavior outside roadmap docs.
|
- Update `docs/config.md` only if directory-backed behavior changes. Otherwise leave its local-directory schema-path behavior intact.
|
||||||
|
- Update `docs/internal/adapters.md` if shared source-resolution behavior is documented there.
|
||||||
|
|
||||||
## Stage 2: Consolidate Prompt/Profile YAML Catalog Helpers
|
## Stage 2: Add HTTP Request, Artifact, And Response Size Limits
|
||||||
|
|
||||||
Problem:
|
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.
|
HTTP `serve` decodes request bodies directly from `r.Body`, reads file artifacts fully into memory, and serializes generated artifact bodies fully into the response. This is acceptable for trusted small local use, but it is not hardened against accidental or hostile large inputs.
|
||||||
|
|
||||||
Decision:
|
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.
|
Add configurable HTTP size limits with conservative defaults. Limits apply only to HTTP `serve`; CLI `run` and `render` keep existing direct filesystem behavior.
|
||||||
|
|
||||||
|
Default limits:
|
||||||
|
|
||||||
|
- `server.max_request_bytes`: 16 MiB.
|
||||||
|
- `server.max_artifact_bytes`: 16 MiB.
|
||||||
|
- `server.max_response_bytes`: 16 MiB.
|
||||||
|
|
||||||
|
Use `0` to disable a specific limit only where this is consistent with existing config style. Negative values are invalid config.
|
||||||
|
|
||||||
Implementation:
|
Implementation:
|
||||||
|
|
||||||
1. Extend `internal/filecatalog` with helpers for:
|
1. Add default constants in `internal/defaults`.
|
||||||
- sorted YAML discovery under an `fs.FS` root;
|
2. Extend app config in `internal/config`.
|
||||||
- clean `fs.FS` root handling;
|
- Add `server.max_request_bytes`.
|
||||||
- display paths relative to an `fs.FS` root;
|
- Add `server.max_artifact_bytes`.
|
||||||
- YAML file stem handling for both OS and `fs.FS` paths.
|
- Add `server.max_response_bytes`.
|
||||||
2. Replace duplicate helpers in `internal/promptdef` and `internal/profile`:
|
- Apply built-in defaults, config-file values, and CLI overrides according to existing precedence.
|
||||||
- prompt `findPromptDefinitionYAMLFiles`;
|
- Reject negative values.
|
||||||
- prompt `cleanFSRoot`;
|
3. Add `serve` CLI overrides.
|
||||||
- prompt `displayPath`;
|
- `--max-request-bytes`
|
||||||
- prompt `isPromptDefinitionYAMLFile`;
|
- `--max-artifact-bytes`
|
||||||
- profile `findProfileYAMLFiles`;
|
- `--max-response-bytes`
|
||||||
- profile `cleanFSRoot`;
|
- Keep these flags scoped to `serve`.
|
||||||
- profile `displayPath`;
|
4. Extend HTTP handler construction.
|
||||||
- profile `profileFileStem`;
|
- Add `httpadapter.HandlerOptions` with request and response limit fields.
|
||||||
- profile `isProfileYAMLFile`.
|
- Keep `httpadapter.NewHandler(runner)` as a default constructor for existing tests and callers.
|
||||||
3. Preserve behavior:
|
- Add `httpadapter.NewHandlerWithOptions(runner, options)` for `serve` wiring.
|
||||||
- recursive nested search;
|
- `NewHandler(runner)` should apply built-in defaults.
|
||||||
- `.yaml` and `.yml` support;
|
- `NewHandlerWithOptions(runner, options)` should use the supplied values exactly, so `0` means disabled after config validation.
|
||||||
- deterministic ordering;
|
5. Limit request decoding.
|
||||||
- subdirectories remain organizational only;
|
- Wrap `r.Body` with `http.MaxBytesReader` when `max_request_bytes > 0`.
|
||||||
- duplicate ID behavior remains unchanged.
|
- Return `413 request_too_large` when decoding fails due to size.
|
||||||
|
- Continue returning `400 invalid_json` for malformed JSON.
|
||||||
|
- Ensure the decoder rejects trailing JSON tokens if it does not already.
|
||||||
|
6. Limit HTTP file artifact reads.
|
||||||
|
- Add a max-bytes option to the restricted HTTP artifact reader.
|
||||||
|
- Use `os.Open`, `Stat`, and `io.LimitReader` or equivalent instead of unbounded `os.ReadFile` for restricted HTTP file reads.
|
||||||
|
- If a file exceeds the configured limit, return a specific artifact error that maps to `413 artifact_too_large`.
|
||||||
|
- Keep inline artifact bodies covered by the request-body limit.
|
||||||
|
7. Limit HTTP response artifact bodies.
|
||||||
|
- Build the response DTO, marshal it to JSON bytes, and compare the final encoded response size against `max_response_bytes` when the limit is positive.
|
||||||
|
- Return `413 response_too_large` when the encoded response exceeds the configured limit.
|
||||||
|
- Do not truncate successful artifacts silently.
|
||||||
|
- Apply the same encoded-response check when `include_raw_output` is true.
|
||||||
|
|
||||||
Tests:
|
Tests:
|
||||||
|
|
||||||
- Expand `internal/filecatalog` tests to cover `fs.FS` roots and display paths.
|
- Config tests:
|
||||||
- Keep prompt/profile repository tests passing unchanged where possible.
|
- defaults are applied;
|
||||||
- Add regression tests if any ordering or relative-path behavior is easy to break.
|
- config file values load;
|
||||||
|
- CLI overrides win;
|
||||||
|
- negative values are rejected.
|
||||||
|
- CLI tests:
|
||||||
|
- `serve` parses the three flags;
|
||||||
|
- `run` and `render` do not gain these flags.
|
||||||
|
- HTTP tests:
|
||||||
|
- oversized request body returns `413 request_too_large`;
|
||||||
|
- malformed JSON below the limit still returns `400 invalid_json`;
|
||||||
|
- successful request below the limit still works;
|
||||||
|
- oversized generated artifact returns the configured too-large error;
|
||||||
|
- `include_raw_output` does not bypass response limits.
|
||||||
|
- Artifact tests:
|
||||||
|
- restricted file reader accepts files at or below the limit;
|
||||||
|
- restricted file reader rejects files above the limit;
|
||||||
|
- unlimited mode with `0` keeps existing behavior.
|
||||||
|
|
||||||
## Stage 3: Deduplicate Validator Control Flow
|
Documentation:
|
||||||
|
|
||||||
|
- Update `docs/config.md` with the new server limit fields and defaults.
|
||||||
|
- Update `docs/cli.md` with the new `serve` flags.
|
||||||
|
- Update `docs/integrations/http-api.md` with `413` errors.
|
||||||
|
- Update `docs/operations.md` with sizing guidance.
|
||||||
|
- Update `docs/troubleshooting.md` with common size-limit failures.
|
||||||
|
- Update `docs/internal/adapters.md` with the HTTP limit boundary.
|
||||||
|
|
||||||
|
## Stage 3: Harden `OpenAICompatibleProfile` ExtraParams Copying
|
||||||
|
|
||||||
Problem:
|
Problem:
|
||||||
|
|
||||||
`StandardValidator.Validate` and `FSValidator.Validate` duplicate most validation-mode logic. The meaningful difference is how JSON Schema documents are loaded and compiled.
|
`OpenAICompatibleProfile` currently deep-copies `ExtraParams` through the general internal copy helper. Cyclic caller-provided maps can recurse indefinitely before `WithProfiles` can validate and return `ErrInvalidConfig`.
|
||||||
|
|
||||||
Decision:
|
Decision:
|
||||||
|
|
||||||
Extract shared validation-mode control flow and isolate schema loading behind a small internal function/interface. Keep the public package boundary unchanged.
|
`OpenAICompatibleProfile` is a convenience constructor, not a validator. It must not recursively walk caller-provided `ExtraParams`. Validation and safe deep copying belong in `WithProfiles` through the existing public JSON validation path.
|
||||||
|
|
||||||
Implementation:
|
Implementation:
|
||||||
|
|
||||||
1. Introduce an unexported schema validation helper, for example:
|
1. Change `OpenAICompatibleProfile` to use a shallow map copy for `ExtraParams`.
|
||||||
- `validateArtifact(ctx, artifact, contract, schemaValidatorFunc)`;
|
- Copy only the top-level `map[string]any`.
|
||||||
- or a small unexported interface that compiles/validates schema-backed instances.
|
- Do not recursively copy nested values.
|
||||||
2. Share handling for:
|
- Do not call `copyAnyMap` from this constructor.
|
||||||
- context checks;
|
2. Keep `WithProfiles` validation and deep-copy behavior unchanged.
|
||||||
- nil artifact errors;
|
- Unsupported values, non-finite numbers, non-string map keys, and cycles should still return `ErrInvalidConfig`.
|
||||||
- `none`;
|
3. Review `copyAnyMap` call sites.
|
||||||
- `basic`;
|
- Keep it for trusted internal-to-public conversions where values come from already-decoded JSON-like data.
|
||||||
- `json`;
|
- Do not use it for untrusted public caller input before validation.
|
||||||
- validation result initialization.
|
4. Add a short comment near the constructor if needed to clarify that recursive validation is intentionally deferred.
|
||||||
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:
|
Tests:
|
||||||
|
|
||||||
- Existing validator tests should remain the main regression suite.
|
- Add a public package test where `OpenAICompatibleProfile` receives cyclic `ExtraParams`.
|
||||||
- Add focused tests only if new helper behavior is not covered by current tests.
|
- The constructor must return promptly.
|
||||||
- Run `go test ./internal/validate ./...`.
|
- `NewEngine(..., WithProfiles(profile))` must return `ErrInvalidConfig`.
|
||||||
|
- Add a test proving non-cyclic nested `ExtraParams` still work through `WithProfiles`.
|
||||||
|
- Keep existing mutation-isolation tests passing.
|
||||||
|
|
||||||
## Stage 4: Remove Stale Domain Surface
|
Documentation:
|
||||||
|
|
||||||
|
- No user-facing behavior change is required if existing docs already state that `WithProfiles` validates `ExtraParams`.
|
||||||
|
- Update docs only if constructor behavior is currently described as validating or deep-copying recursively.
|
||||||
|
|
||||||
|
## Stage 4: Redact Provider Non-2xx Response Bodies From Default Errors
|
||||||
|
|
||||||
Problem:
|
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.
|
The OpenAI-compatible client includes a trimmed provider response body snippet in `ErrUnexpectedStatus`. CLI and library callers may log this error. Provider error bodies can include prompt fragments, schema details, request IDs, or other sensitive operational data.
|
||||||
|
|
||||||
Decision:
|
Decision:
|
||||||
|
|
||||||
Remove unused internal domain fields/types unless implementation-time review finds a live consumer.
|
Default errors should include the provider status code but not the response body. Do not add a debug mode in this pass unless an existing debug/logging surface already supports it.
|
||||||
|
|
||||||
Implementation:
|
Implementation:
|
||||||
|
|
||||||
1. Remove `Error error` from `domain.RunResult`.
|
1. Change the non-2xx error returned by `internal/llm.OpenAICompatibleClient`.
|
||||||
2. Remove `domain.RunMetadata`.
|
- Keep wrapping `ErrUnexpectedStatus`.
|
||||||
3. Update tests, formatters, adapters, and public conversions only if compilation reveals hidden references.
|
- Include `status=<code>`.
|
||||||
4. Do not remove fields from public root-package types in this stage.
|
- Do not include response body text.
|
||||||
|
2. Drain and close the response body safely enough for normal HTTP client reuse.
|
||||||
|
- It is acceptable to read and discard a small bounded amount if needed.
|
||||||
|
- Do not store or return the discarded content.
|
||||||
|
3. Review tests that assert the old body-snippet behavior and update them.
|
||||||
|
4. Review CLI, HTTP, and public package error mapping.
|
||||||
|
- HTTP should remain generic and not leak provider details.
|
||||||
|
- CLI/library errors should retain enough status context to diagnose provider failures.
|
||||||
|
|
||||||
Tests:
|
Tests:
|
||||||
|
|
||||||
- Run `go test ./...`.
|
- Update `internal/llm` non-2xx tests:
|
||||||
|
- `errors.Is(err, ErrUnexpectedStatus)` remains true;
|
||||||
|
- the status code appears in the error string;
|
||||||
|
- the provider response body does not appear in the error string.
|
||||||
|
- Add a regression test with a body containing distinctive sensitive-looking text and assert it is absent.
|
||||||
|
|
||||||
## Stage 5: Reassess Thin Interface Files Without Collapsing Packages
|
Documentation:
|
||||||
|
|
||||||
|
- Update `docs/integrations/openai-compatible-chat.md` to remove the claim that `ErrUnexpectedStatus` includes a response body snippet.
|
||||||
|
- Update troubleshooting docs only if they currently instruct users to inspect provider body snippets.
|
||||||
|
|
||||||
|
## Stage 5: Clarify Artifact-Root Symlink Semantics
|
||||||
|
|
||||||
Problem:
|
Problem:
|
||||||
|
|
||||||
Several packages have small interface-only files, such as:
|
The restricted HTTP artifact reader uses lexical path containment before reading the file. Symlinks inside the artifact root are followed by the operating system. This is documented, but the phrase "must stay inside the root" can be overread as a strict realpath guarantee.
|
||||||
|
|
||||||
- `internal/llm/client.go`
|
|
||||||
- `internal/prompt/renderer.go`
|
|
||||||
- `internal/validate/validator.go`
|
|
||||||
- `internal/profile/repository.go`
|
|
||||||
- `internal/promptdef/repository.go`
|
|
||||||
|
|
||||||
Decision:
|
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.
|
Keep symlink-following behavior for this cleanup pass, but make the code and docs explicit that containment is lexical and relies on the artifact root not being writable by untrusted users. This avoids a potentially breaking change for deployments that intentionally use symlinks.
|
||||||
|
|
||||||
Implementation:
|
Implementation:
|
||||||
|
|
||||||
1. No code change is required for package consolidation.
|
1. Rename or comment the restricted reader's path-resolution helper to make the lexical nature clear.
|
||||||
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.
|
2. Add tests documenting current symlink behavior where the platform supports symlinks.
|
||||||
3. Keep `internal/domain` free of adapter implementation details.
|
- A symlink inside the root to a file outside the root is followed.
|
||||||
4. Keep `internal/usecase` dependent on interfaces rather than concrete filesystem/model implementations.
|
- The test should skip cleanly if symlink creation is unavailable.
|
||||||
|
3. Keep traversal rejection tests for `..` and absolute paths outside the root.
|
||||||
|
4. Do not introduce `filepath.EvalSymlinks` in this pass.
|
||||||
|
|
||||||
## Stage 6: Documentation And Final Verification
|
Documentation:
|
||||||
|
|
||||||
After implementation:
|
- Update `docs/config.md`, `docs/operations.md`, `docs/integrations/http-api.md`, and `docs/internal/adapters.md` to say:
|
||||||
|
- lexical traversal outside the root is rejected;
|
||||||
|
- symlinks inside the root are followed;
|
||||||
|
- the artifact root must not be writable by untrusted users.
|
||||||
|
- Avoid wording that implies strict realpath containment unless the implementation changes to enforce it.
|
||||||
|
|
||||||
1. Update internal docs:
|
Future option:
|
||||||
- `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
|
If strict filesystem containment becomes required, add an opt-in or replacement mode that resolves both the configured root and requested file with `filepath.EvalSymlinks` before reading, rejects symlink escapes, and documents any compatibility impact.
|
||||||
go test ./...
|
|
||||||
go vet ./...
|
## Stage 6: Align CLI Help And Documentation
|
||||||
go run ./examples/go-library/prepare
|
|
||||||
go run ./cmd/scriptorium render \
|
Problem:
|
||||||
--config ./examples/config.yml \
|
|
||||||
--prompt generic.markdown_summary \
|
The `serve` usage text omits `--artifact-root`, even though the flag exists. New limit flags from Stage 2 also need to appear consistently in CLI help and docs.
|
||||||
--input transcript=./examples/fixtures/transcript.md \
|
|
||||||
--input glossary=./examples/fixtures/glossary.yml \
|
Decision:
|
||||||
--format json
|
|
||||||
```
|
Keep CLI help concise but complete for supported flags.
|
||||||
|
|
||||||
|
Implementation:
|
||||||
|
|
||||||
|
1. Update `printUsage` in `internal/adapter/cli`.
|
||||||
|
- Include `--artifact-root DIR` in the `serve` usage line.
|
||||||
|
- Include the new size-limit flags from Stage 2.
|
||||||
|
- Keep the line readable; splitting long usage text into multiple lines is acceptable if tests are updated.
|
||||||
|
2. Update CLI tests that assert usage output.
|
||||||
|
3. Confirm `docs/cli.md` matches actual flags.
|
||||||
|
|
||||||
|
Tests:
|
||||||
|
|
||||||
|
- Add or update CLI usage tests to assert that `serve` help mentions:
|
||||||
|
- `--artifact-root`;
|
||||||
|
- `--max-request-bytes`;
|
||||||
|
- `--max-artifact-bytes`;
|
||||||
|
- `--max-response-bytes`.
|
||||||
|
|
||||||
|
Documentation:
|
||||||
|
|
||||||
|
- Update `docs/cli.md` and any command examples affected by line wrapping or flag additions.
|
||||||
|
|
||||||
|
## Stage 7: Final Verification
|
||||||
|
|
||||||
|
Run the full verification set after all stages:
|
||||||
|
|
||||||
|
```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
|
||||||
|
```
|
||||||
|
|
||||||
|
Also run targeted packages while implementing each stage:
|
||||||
|
|
||||||
|
```bash
|
||||||
|
go test ./internal/promptdef ./internal/validate ./internal/filecatalog
|
||||||
|
go test ./internal/artifact ./internal/adapter/http ./internal/adapter/cli ./internal/config
|
||||||
|
go test .
|
||||||
|
```
|
||||||
|
|
||||||
## Non-Goals
|
## Non-Goals
|
||||||
|
|
||||||
- Do not collapse packages merely because they are small.
|
- Do not collapse internal packages merely to reduce package count.
|
||||||
- Do not remove HTTP file inputs; restrict them with an explicit artifact root.
|
- Do not add HTTP authentication or authorization.
|
||||||
- Do not change public Go API behavior as part of internal package cleanup.
|
- Do not remove HTTP file inputs.
|
||||||
- Do not introduce a new auth system for HTTP `serve` in this cleanup pass.
|
- Do not change public request/result type names or method signatures.
|
||||||
- Do not change prompt/profile YAML lookup semantics except by preserving them through shared helpers.
|
- Do not change CLI `run` or `render` local file-input behavior.
|
||||||
|
- Do not silently truncate request, artifact, provider, or response bodies.
|
||||||
|
- Do not accept raw API keys through config files, profile files, CLI flags, or HTTP payloads.
|
||||||
|
|
||||||
|
## Assumptions
|
||||||
|
|
||||||
|
- Public `fs.FS` roots are intended to be narrower than the supplied filesystem and should therefore be enforced.
|
||||||
|
- The initial HTTP size-limit defaults are intentionally conservative and can be tuned by operators.
|
||||||
|
- Provider response-body diagnostics are less important than safe default error handling.
|
||||||
|
- Symlink compatibility is more important than strict realpath containment for the immediate cleanup pass, provided documentation is explicit.
|
||||||
|
|||||||
Reference in New Issue
Block a user