Audit the internal package API and add a staged improvement roadmap
This commit is contained in:
@@ -1,239 +1,203 @@
|
||||
# Public Package Cleanup And Hardening Plan
|
||||
# Internal Package Audit And Cleanup Plan
|
||||
|
||||
This plan converts the public package audit into staged implementation work. The target audience is an LLM coding agent implementing each stage in order.
|
||||
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 goals are:
|
||||
The package count is not itself a problem. Most internal packages have useful boundaries:
|
||||
|
||||
- make public-facing code concise, idiomatic, and predictable;
|
||||
- harden all public boundary inputs against accidental secret exposure, mutation side effects, and malformed caller-provided values;
|
||||
- preserve the current library user story: a downstream Go application can provide prompt/profile sources, choose built-ins or in-memory profiles, pass a request-scoped API key, and receive prepared or generated results;
|
||||
- avoid broad feature work or unrelated refactors.
|
||||
- `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.
|
||||
|
||||
Unless a stage explicitly says otherwise, do not change public method names, request/result shapes, CLI behavior, HTTP behavior, or built-in profile semantics.
|
||||
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: Make Load Error Classification Deterministic
|
||||
## Stage 1: Harden HTTP File Artifact Inputs
|
||||
|
||||
Problem:
|
||||
|
||||
`internal/usecase.ErrProfileLoad` currently wraps both prompt-definition load failures and execution-profile load failures. Public mapping then infers `ErrPromptLoad` or `ErrProfileLoad` from wrapped causes. This is brittle because some profile repository failures, such as profile directory read failures, are not wrapped with a profile sentinel and can still map to `scriptorium.ErrPromptLoad`.
|
||||
`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. In `internal/usecase`, split the current load sentinel into two explicit sentinels:
|
||||
- `ErrPromptLoad = errors.New("failed to load prompt definition")`
|
||||
- `ErrProfileLoad = errors.New("failed to load execution profile")`
|
||||
2. Update `Runner.Prepare`:
|
||||
- wrap `promptDefs.GetPromptDefinition` failures with `ErrPromptLoad`;
|
||||
- wrap prompt-definition hashing failures with `ErrPromptLoad`;
|
||||
- wrap `profiles.GetProfile` failures with `ErrProfileLoad`.
|
||||
3. Update internal tests that currently expect prompt load failures to use `ErrProfileLoad`.
|
||||
4. Simplify public error mapping in `errors.go`:
|
||||
- map `usecase.ErrPromptLoad` to public `ErrPromptLoad`;
|
||||
- map `usecase.ErrProfileLoad` to public `ErrProfileLoad`;
|
||||
- retain direct mappings for `promptdef.ErrPromptDefinitionNotFound`, `profile.ErrProfileNotFound`, profile YAML/profile validation causes, and prompt YAML/prompt validation causes.
|
||||
5. Keep adapter behavior stable:
|
||||
- HTTP prompt-load failures should still return the existing prompt-load status/code;
|
||||
- HTTP profile-load failures should still return the existing profile-load status/code;
|
||||
- CLI errors should remain concise and must not include raw secrets.
|
||||
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:
|
||||
|
||||
- Add public package tests proving:
|
||||
- prompt-definition repository load errors map to `scriptorium.ErrPromptLoad`;
|
||||
- selected profile repository read/list errors map to `scriptorium.ErrProfileLoad`;
|
||||
- selected malformed profile YAML and raw `api_key` rejection still map to `ErrProfileLoad`;
|
||||
- prompt-not-found and profile-not-found sentinels still map to `ErrPromptNotFound` and `ErrProfileNotFound`.
|
||||
- Add or update internal usecase tests for the new `ErrPromptLoad` and `ErrProfileLoad` split.
|
||||
- Run `go test ./...`.
|
||||
|
||||
## Stage 2: Stop Mutating Caller-Owned HTTP Clients
|
||||
## Stage 5: Reassess Thin Interface Files Without Collapsing Packages
|
||||
|
||||
Problem:
|
||||
|
||||
The default OpenAI-compatible client currently stores the caller-provided `*http.Client` and mutates `Timeout` when it is zero. Because the public `scriptorium.Config.HTTPClient` accepts this client, engine construction can unexpectedly modify caller-owned shared clients, including `http.DefaultClient`.
|
||||
Several packages have small interface-only files, such as:
|
||||
|
||||
Implementation:
|
||||
|
||||
1. In `internal/llm.NewOpenAICompatibleClient`, clone any supplied `cfg.HTTPClient` before applying timeout defaults:
|
||||
- if `cfg.HTTPClient != nil`, copy the struct value with `cloned := *cfg.HTTPClient` and use `&cloned`;
|
||||
- if `cloned.Timeout == 0`, set `cloned.Timeout` to the resolved default timeout;
|
||||
- do not mutate `cfg.HTTPClient`.
|
||||
2. Preserve existing request-level timeout behavior:
|
||||
- positive `timeout_seconds` still overrides the client timeout for that request;
|
||||
- explicit `timeout_seconds: 0` with target presence still disables client timeout for that request;
|
||||
- omitted timeout still uses the client/default timeout.
|
||||
3. Do not clone or replace shared internals such as `Transport`; shallow `http.Client` copying is the idiomatic choice here.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add LLM client tests proving:
|
||||
- a supplied zero-timeout `http.Client` remains unchanged after `NewOpenAICompatibleClient`;
|
||||
- the constructed client still uses the default timeout internally;
|
||||
- a supplied nonzero-timeout `http.Client` remains unchanged;
|
||||
- request-level timeout override behavior remains unchanged.
|
||||
- Run `go test ./internal/llm ./...`.
|
||||
|
||||
## Stage 3: Harden Public JSON-Like Inputs
|
||||
|
||||
Problem:
|
||||
|
||||
Public `ExtraParams` and structured-output schema values are copied with a permissive reflection helper. It deep-copies useful typed JSON-like maps/slices, but it accepts arbitrary values, pointers, functions, channels, and cyclic structures. A cyclic value can recurse indefinitely and crash the process.
|
||||
- `internal/llm/client.go`
|
||||
- `internal/prompt/renderer.go`
|
||||
- `internal/validate/validator.go`
|
||||
- `internal/profile/repository.go`
|
||||
- `internal/promptdef/repository.go`
|
||||
|
||||
Decision:
|
||||
|
||||
Replace permissive copying at public boundaries with validating JSON-compatible copying. This is more correct and maintainable than trying to copy arbitrary Go object graphs. `ExtraParams` and schema payloads are provider JSON payloads; accepting non-JSON values only delays failure and makes security behavior harder to reason about.
|
||||
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. Introduce an internal root-package helper for public boundary JSON values, for example:
|
||||
- `copyJSONMap(src map[string]any) (map[string]any, error)`
|
||||
- `copyJSONValue(value any, path string, seen map[visit]struct{}) (any, error)`
|
||||
2. Accept only JSON-compatible value kinds:
|
||||
- `nil`
|
||||
- `string`
|
||||
- `bool`
|
||||
- signed and unsigned integer types that can be represented safely enough for JSON provider payloads;
|
||||
- floating-point values excluding `NaN` and infinities;
|
||||
- maps with string keys;
|
||||
- slices and arrays;
|
||||
- values behind `interface`;
|
||||
- optionally pointers only by dereferencing them, with cycle detection.
|
||||
3. Reject unsupported values with clear public errors:
|
||||
- `WithProfiles` profile `ExtraParams` errors return `ErrInvalidConfig`;
|
||||
- `OpenAICompatibleProfile` should not silently accept invalid `ExtraParams`; because it currently returns only `Profile`, keep it a pure field-copy constructor and perform validation in `WithProfiles`;
|
||||
- request `ExecutionTargetOverride.ExtraParams` errors return `ErrInvalidRequest`.
|
||||
4. Track visited map/slice/pointer identities to reject cycles instead of recursing indefinitely.
|
||||
5. Keep typed JSON-compatible values isolated from caller mutation:
|
||||
- `map[string]string`, `map[string]int`, `[]int`, `[]float64`, nested `map[string]any`, and values behind `any` should still be copied.
|
||||
6. Preserve internal-only copy helpers where they are operating on trusted domain data, but use validating helpers at public-to-domain boundaries.
|
||||
7. Ensure any provider `extra_params` validation in the LLM adapter still rejects reserved keys and empty keys as it does today.
|
||||
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.
|
||||
|
||||
Tests:
|
||||
## Stage 6: Documentation And Final Verification
|
||||
|
||||
- Add public package tests for `ExecutionTargetOverride.ExtraParams`:
|
||||
- typed JSON-compatible maps/slices are accepted and isolated from caller mutation;
|
||||
- function/channel/struct values return `ErrInvalidRequest`;
|
||||
- maps with non-string keys return `ErrInvalidRequest`;
|
||||
- cyclic maps/slices return `ErrInvalidRequest` without panic or hang;
|
||||
- `NaN` and infinities return `ErrInvalidRequest`.
|
||||
- Add public package tests for `WithProfiles`:
|
||||
- invalid profile `ExtraParams` return `ErrInvalidConfig`;
|
||||
- cyclic profile `ExtraParams` return `ErrInvalidConfig`;
|
||||
- valid typed maps/slices remain deep-copied.
|
||||
- Run `go test ./...`.
|
||||
After implementation:
|
||||
|
||||
## Stage 4: Redact Public Secret Fields In String Formatting
|
||||
|
||||
Problem:
|
||||
|
||||
`RunRequest.APIKey` and `GenerateRequest.APIKey` use `json:"-"`, so JSON marshaling does not emit direct API keys. Ordinary Go formatting such as `fmt.Printf("%+v", req)` can still print exported string fields.
|
||||
|
||||
Decision:
|
||||
|
||||
Add redacting formatting methods to the public request types that directly carry raw secrets. This does not protect against reflection-based dumpers, but it materially improves safety for normal `fmt`, structured logs that call `String`, and test diagnostics. Avoid replacing raw strings with a new secret wrapper type in this cleanup pass because that would add API complexity and likely require downstream changes.
|
||||
|
||||
Implementation:
|
||||
|
||||
1. Add `String() string` and `GoString() string` methods for:
|
||||
- `RunRequest`
|
||||
- `GenerateRequest`
|
||||
2. Formatting behavior:
|
||||
- include useful non-secret context such as prompt/profile IDs and whether an API key is set;
|
||||
- never include the raw `APIKey` value;
|
||||
- avoid dumping full rendered prompt or output content unless existing public behavior explicitly needs it.
|
||||
3. Consider adding redaction helpers for nested public types only if needed by these methods.
|
||||
4. Do not alter JSON tags or existing JSON output shapes.
|
||||
5. Update consumer docs to warn that reflection-based debug dumpers can still expose exported fields and that callers should avoid logging raw request structs with reflection dump libraries.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add public package tests proving:
|
||||
- `fmt.Sprint(req)`, `fmt.Sprintf("%+v", req)`, and `fmt.Sprintf("%#v", req)` do not contain the direct API key for `RunRequest`;
|
||||
- the same holds for `GenerateRequest`;
|
||||
- JSON marshaling continues to omit the direct API key.
|
||||
- Run `go test ./...`.
|
||||
|
||||
## Stage 5: Polish The Public Option Type
|
||||
|
||||
Problem:
|
||||
|
||||
The public API currently exposes `type Option func(*engineOptions) error`. This works, but `go doc` shows an exported type whose signature references an unexported implementation type. That is legal Go, but it is not a polished public API.
|
||||
|
||||
Decision:
|
||||
|
||||
Change `Option` to an exported interface with an unexported method, backed by an unexported function adapter. This is the standard pattern when a package wants an opaque option type while preserving functional option ergonomics for package-provided options.
|
||||
|
||||
Implementation:
|
||||
|
||||
1. Replace the exported function type with:
|
||||
|
||||
```go
|
||||
type Option interface {
|
||||
apply(*engineOptions) error
|
||||
}
|
||||
|
||||
type optionFunc func(*engineOptions) error
|
||||
|
||||
func (f optionFunc) apply(options *engineOptions) error {
|
||||
return f(options)
|
||||
}
|
||||
```
|
||||
|
||||
2. Update all option constructors to return `optionFunc(...)`.
|
||||
3. Update `NewEngine` to call `opt.apply(&options)`.
|
||||
4. Preserve nil-option behavior if practical:
|
||||
- because `Option` is now an interface, a nil interface can still be skipped;
|
||||
- typed nil option values are not expected from package users and do not need special support.
|
||||
5. This is a public source compatibility change only for callers that manually construct custom `Option` values. The package does not currently provide a documented way for callers to create custom options, so this is acceptable before API freeze.
|
||||
|
||||
Tests:
|
||||
|
||||
- Existing option tests should continue passing.
|
||||
- Add a small compile-time or runtime test proving package-provided options still compose as before.
|
||||
- Run `go test ./...` and `go doc .` to verify the public docs no longer expose `*engineOptions` in the `Option` signature.
|
||||
|
||||
## Stage 6: Review And Tighten Redundant Public Surface
|
||||
|
||||
Problem:
|
||||
|
||||
`OpenAICompatibleProfileConfig` currently duplicates `Profile` almost exactly, and `OpenAICompatibleProfile` is a shallow convenience constructor plus deep copy. This is not harmful, but it may be unnecessary public surface unless it provides provider-specific clarity or future extension space.
|
||||
|
||||
Decision:
|
||||
|
||||
Keep `OpenAICompatibleProfile` and `OpenAICompatibleProfileConfig` for now, but tighten documentation and tests around their purpose. The constructor should remain a convenience for building a normal in-memory `Profile`; it must not register global state, maintain a model catalog, or hide credential resolution. Removing it would reduce surface area, but it would also undo the recently documented library ergonomics without a strong correctness benefit.
|
||||
|
||||
Implementation:
|
||||
|
||||
1. Add or improve doc comments for all exported option functions and source helpers:
|
||||
- `WithPromptFS`
|
||||
- `WithPromptFile`
|
||||
- `WithProfileFS`
|
||||
- `WithProfileFile`
|
||||
- `WithSchemaFS`
|
||||
- `WithSchemaFile`
|
||||
2. Improve doc comments for `Profile`, `OpenAICompatibleProfileConfig`, and `OpenAICompatibleProfile`:
|
||||
- explain that the constructor returns an ordinary in-memory profile;
|
||||
- explain that `APIKeyRequired` is satisfied by `RunRequest.APIKey`;
|
||||
- state that raw API keys do not belong in profiles.
|
||||
3. Re-run `go doc .` and confirm the public API reads cleanly.
|
||||
4. Do not remove or rename existing exported types in this stage.
|
||||
|
||||
Tests:
|
||||
|
||||
- No new behavior tests are required unless comments reveal behavior drift.
|
||||
- Run `go test ./...` and `go vet ./...`.
|
||||
|
||||
## Stage 7: Final Security And Compatibility Pass
|
||||
|
||||
Implementation:
|
||||
|
||||
1. Search for raw secret emission paths:
|
||||
- `rg "APIKey|api_key|Authorization|%\\+v|GoString|String"`.
|
||||
2. Confirm:
|
||||
- config, profile YAML, CLI flags, and HTTP JSON still reject raw API keys;
|
||||
- public `RunRequest.APIKey` remains request-scoped and JSON-omitted;
|
||||
- `PreparedRun`, `RunResult`, and formatter output do not include direct API keys;
|
||||
- injected `LLMClient` still receives `GenerateRequest.APIKey` when provided.
|
||||
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
|
||||
@@ -248,26 +212,10 @@ Implementation:
|
||||
--format json
|
||||
```
|
||||
|
||||
4. Update implemented-behavior docs only after code changes:
|
||||
- `docs/consumers/pkg-scriptorium.md` for redaction and JSON-compatible `ExtraParams` validation;
|
||||
- `docs/internal/runner.md` if internal error sentinel names or load classification are documented there;
|
||||
- `docs/internal/adapters.md` if adapter error mapping documentation references the old shared sentinel.
|
||||
|
||||
## Non-Goals
|
||||
|
||||
- Do not add a credential resolver.
|
||||
- Do not add raw API-key support to config, profile YAML, CLI flags, or HTTP JSON.
|
||||
- Do not introduce new third-party dependencies.
|
||||
- Do not add new model/profile catalog behavior.
|
||||
- Do not redesign prompt/profile/schema source loading beyond the explicit cleanup items above.
|
||||
- Do not change CLI or HTTP request/response shapes unless required to preserve current documented behavior.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None. The plan chooses the more idiomatic and maintainable options where the audit identified tradeoffs:
|
||||
|
||||
- split internal load sentinels rather than expanding public inference heuristics;
|
||||
- clone caller-owned `http.Client` values rather than documenting mutation;
|
||||
- validate JSON-like inputs rather than copying arbitrary Go object graphs;
|
||||
- redact public secret-bearing request structs rather than replacing API-key strings with a larger credential abstraction;
|
||||
- keep `OpenAICompatibleProfile` as documented ergonomic surface for now rather than removing recently added API.
|
||||
- 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.
|
||||
|
||||
Reference in New Issue
Block a user