diff --git a/audit.md b/audit.md index 12914bd..2b14380 100644 --- a/audit.md +++ b/audit.md @@ -1226,3 +1226,354 @@ declarations would add no behavioral protection. - Stage 15 owns admission, permit scheduling, queue behavior, fairness, and cancellation. It should treat the registry's normalized immutable capacity snapshot as established input. + +## Stage 7: File Discovery And Prompt Definitions + +### Scope Reviewed + +The review covered every source, test, and fixture in `internal/filecatalog` +and `internal/promptdef`. The framework format reference and internal source +document supplied the owning contracts. Narrow traces through root prompt +source options, exact prompt inspection, and preparation were used only to +confirm source selection, error translation, and point-in-time repository +usage. Rendering, artifact loading, profile repositories, and schema loading +or validation were not audited. + +### Accepted Findings + +#### S07-F01: Content-file resolution escapes OS source roots and changes exact paths + +- **Category:** correctness +- **Severity:** high +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/promptdef/filesystem_repository.go` + (`normalizePromptDefinition`, `normalizePromptDefinitionFromFS`, and their + content readers), `internal/filecatalog/catalog.go` (`ResolveFSPath`), and + content-file tests in `internal/promptdef/repository_test.go` +- **Contract at issue:** The framework format reference promises that a + directory or `fs.FS` prompt's `content_file` resolves relative to the prompt + file and remains within the configured source root. A path value also names + an exact filesystem entry; checking whether it is blank must not silently + substitute a different valid name. +- **Evidence:** The OS-directory normalizer receives only the prompt file path, + not the repository root. It accepts an absolute `content_file` unchanged and + cleans a relative path after joining it to the prompt directory, then calls + `os.ReadFile` without a containment check. A temporary probe placed a prompt + under `prompts/`, referenced `../outside.txt`, and observed the repository + return the outside file's sentinel body successfully. Both OS and `fs.FS` + resolution also call `strings.TrimSpace` on the path before opening it. A + second probe created an existing file named `body ` and referenced the + quoted YAML value `"./body "`; lookup instead tried `body` and failed with + `ErrInvalidPromptDefinition`. Existing OS tests cover nested in-root + resolution only, while escape rejection is tested only for directory-backed + `fs.FS`. +- **Failure mode:** A prompt definition writable by a less-trusted source can + read an arbitrary file reachable by the process and incorporate its contents + into a template that may later be sent to a model provider. Independently, a + valid source cannot reference legal filenames whose leading or trailing + whitespace was preserved by YAML. +- **Recommended direction:** Give every directory-backed content resolver the + actual source root and enforce relative, contained resolution before any + read. Use trimming only to decide whether the configured path is blank, then + resolve and open the original parsed value. Keep single-file source behavior + relative to that file's directory and define its absolute-path rule + explicitly rather than converting an absolute `fs.FS` path into a different + relative path. +- **Required verification:** Run the same table against OS-directory, + `WithPromptFS`, and single-file sources. Cover a sibling within the root, a + parent path still within the root, a parent escape, an absolute path, and + exact existing names with leading or trailing whitespace. Confirm rejected + paths perform no outside read and public operations preserve + `ErrPromptLoad`. + +#### S07-F02: Error association runs before canonical ID and version selection + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/promptdef/filesystem_repository.go` + (`filesystemRepository.GetPromptDefinition`, `loadPromptDefinition`, + `promptDefinitionFileHasID`, and `promptDefinitionDataHasID`) and selection + cases in `internal/promptdef/repository_test.go` +- **Contract at issue:** Definitions are selected by normalized YAML `id`, not + filename, and an optional version selects one exact ID/version pair. + Malformed or invalid files outside that selector must not make an otherwise + valid exact definition unavailable. +- **Evidence:** Both repository paths set `fileMatch` from the filename stem + and immediately return a YAML or semantic error for that file even when it + has no matching YAML ID. After a successful strict decode, both normalize + the whole definition and return any semantic or referenced-content error + whenever its ID matches, before checking whether a requested version + matches. Temporary probes demonstrated both effects: malformed + `target.yaml` shadowed a valid differently named definition whose YAML ID + was `target`, and an invalid `target` version `2` blocked a valid exact + lookup for version `1`. Several fixture cases intentionally request + underscore filename stems rather than their hyphenated YAML IDs, so the + current tests encode part of the noncanonical behavior instead of + discriminating it. +- **Failure mode:** Adding or renaming an unrelated malformed file can break a + valid prompt lookup solely because its filename happens to equal the + requested ID. Likewise, a broken historical or future version can take every + other exact version of the same prompt offline. +- **Recommended direction:** Associate strict-decoding and semantic failures + only with selector metadata recovered from the YAML document. Apply the + requested ID and version before content resolution and other semantic work; + do not use filename stems as a second identity system. When malformed YAML + does not provide reliable selector metadata, treat it as unrelated to a + point lookup rather than contradicting the YAML-ID contract. +- **Required verification:** For both source implementations, pair one valid + exact definition with a malformed same-stem/different-ID file and with an + invalid same-ID/different-version file. Assert successful exact lookup, then + retain selected-ID and selected-version malformed cases that return the + contextual YAML or definition sentinel. + +#### S07-F03: Strict decoding silently ignores additional YAML documents + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/promptdef/filesystem_repository.go` + (`loadPromptDefinitionFile` and `decodePromptDefinition`) and strict-decoding + cases in `internal/promptdef/repository_test.go` +- **Contract at issue:** One prompt-definition file contains one strictly + decoded definition. Every supplied YAML document must be accounted for; + trailing documents cannot fall outside unknown-field and semantic + validation. +- **Evidence:** Each decoder enables `KnownFields(true)` but calls `Decode` + exactly once and returns without requiring end of stream. A temporary + `fstest.MapFS` probe appended `---` and a second document containing an + unknown field to a valid selected prompt. Lookup succeeded and returned only + the first document. Existing tests cover unknown fields inside the first + document but no document-stream boundary. +- **Failure mode:** Configuration after a document separator is silently + ignored. A maintainer can believe a field change, replacement definition, or + invalid setting is active while Promptkit hashes and executes only the + earlier document, and strict decoding provides no diagnostic. +- **Recommended direction:** Require exactly one YAML document by decoding the + selected definition and then requiring the next decode to return `io.EOF`. + Preserve comments and ordinary trailing whitespace while rejecting any + additional empty or non-empty document. +- **Required verification:** Add one shared strict-decoder table covering an + ordinary document with comments, a second populated document, a second empty + document, and malformed trailing YAML. Exercise one OS and one `fs.FS` + repository boundary and preserve `ErrInvalidYAML` plus the source path. + +#### S07-F04: Every lookup reads file-backed content for unrelated prompts + +- **Category:** efficiency +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/promptdef/filesystem_repository.go` + (`filesystemRepository.GetPromptDefinition`, `loadPromptDefinition`, + `normalizePromptDefinition`, and `normalizePromptDefinitionFromFS`) +- **Contract at issue:** A point-in-time lookup must scan definition metadata + far enough to select an exact prompt and detect ambiguity, but it need not + load template bodies for definitions whose ID or requested version already + excludes them. +- **Evidence:** Both scan loops strictly decode and then fully normalize every + valid YAML file before comparing `def.ID` and `def.Version`. Normalization + reads every `content_file`. A counting-`fs.FS` probe performed two exact + lookups of an inline `target` prompt in a source containing one unrelated + file-backed prompt; the unrelated template was opened once per lookup. The + behavior follows the same path for OS files. These reads are in addition to + the directory walk and YAML-file reads needed for point-in-time selection. +- **Failure mode:** Lookup work scales with the total bytes of every + file-backed template in the catalog rather than the selected prompt's + content. Exact inspection, preparation, and ordinary execution repeatedly + incur unrelated I/O; a large unused template can dominate lookup latency and + filesystem load. +- **Recommended direction:** After strict decoding, compare normalized ID and + requested version before resolving messages or reading content files. Keep + the deterministic point-in-time directory scan and duplicate detection; no + cache is required to remove the unrelated body reads. +- **Required verification:** Use a counting filesystem to show that selected + content is read exactly once and unrelated content is never opened, while + all YAML metadata needed for duplicate detection is still examined. + Exercise repeated lookups to ensure the point-in-time contract remains + intact. + +#### S07-F05: OS and fs.FS repositories duplicate the same selection policy + +- **Category:** duplication +- **Severity:** medium +- **Confidence:** high +- **Status:** accepted +- **Affected code:** `internal/promptdef/filesystem_repository.go` + (`filesystemRepository.GetPromptDefinition`, `loadPromptDefinition`, + `loadPromptDefinitionFile`, `decodePromptDefinition`, + `promptDefinitionFileHasID`, `promptDefinitionDataHasID`, + `normalizePromptDefinition`, and `normalizePromptDefinitionFromFS`) +- **Contract at issue:** Configured directories and `fs.FS` roots implement + one prompt discovery, strict-decoding, selection, duplicate, and + content-resolution contract. Source mechanics may differ, but the semantic + policy needs one owner so fixes and constraints cannot drift by source type. +- **Evidence:** The two repository paths independently implement nearly the + same ordered scan, filename fallback, strict decode, loose ID recovery, + semantic normalization, ID/version filtering, match collection, duplicate + diagnostics, and not-found result. They also have paired byte-versus-path + decode and ID helpers. The divergence is already observable: + directory-backed `fs.FS` uses `ResolveFSPath` for lexical containment while + the OS-directory path has none, and tests provide a much larger semantic + matrix only for the OS implementation. This is the same semantic rule, not + merely similar filesystem syntax. +- **Failure mode:** A selection, containment, strictness, or contextual-error + fix can land in one path while the other retains old behavior. Consumers + then get different validity or lookup results when replacing `Config.PromptDir` + with `WithPromptFS`, despite the latter's explicit same-rules contract. +- **Recommended direction:** Give discovery and reads a small source adapter, + then run one source-neutral decode, selector, normalization, duplicate, and + error-classification algorithm. Keep OS versus `fs.FS` path display and + content opening in the adapter where their real mechanics differ. +- **Required verification:** Run a shared behavioral suite against both source + adapters for exact selection, ambiguity, malformed selected and unrelated + files, content resolution, cancellation, and contextual errors. Retain only + source-specific tests for genuinely different path representations or + filesystem failures. + +#### S07-F06: Prompt semantic validation has unprotected contract branches + +- **Category:** testing +- **Severity:** medium +- **Confidence:** high +- **Status:** accepted +- **Affected code:** `internal/promptdef/filesystem_repository.go` + (`normalizePromptDefinitionWithContent` and `isValidOutputFormat`), + `internal/promptdef/repository_test.go`, and fixtures under + `internal/promptdef/testdata` +- **Contract at issue:** Prompt package tests own required identity, messages, + input declarations, content selection, cache control, and output-contract + validation. Each independently implemented rejection rule needs compact + protection proportionate to the parser's role as the file-format boundary. +- **Evidence:** The existing fixture matrix protects missing ID, missing + messages, duplicate input names, content/content-file exclusivity, missing + content, validation mode, schema-path dependency, and cache-control rules. + It has no rejection case for a missing version, blank input name, blank + message role, invalid output format, negative repair attempts, or an + explicitly present blank `default_profile`. Statement coverage reflects + some of these omissions—`isValidOutputFormat` reports only 66.7%—but the + finding is the unprotected semantic decisions, not the percentage. +- **Failure mode:** A refactor can remove or invert any of these required-field + or range checks while all focused parser tests remain green, allowing a + malformed definition to reach hashing, rendering, or validation planning. +- **Recommended direction:** Add one compact table at the shared normalization + or repository boundary for the missing semantic categories rather than six + more standalone fixture files. Keep the existing fixtures where source + resolution or strict nested YAML structure is the behavior under test. +- **Required verification:** Confirm each case returns + `ErrInvalidPromptDefinition` through a repository with useful field or + message context, and that a deliberate mutation of each rule fails its table + row. One representative source is sufficient once S07-F05 gives both + adapters a shared semantic implementation. + +### Unresolved Observations + +None. `fs.FS` containment is necessarily expressed in the supplied +filesystem's path namespace; whether a particular implementation follows +symlinks outside an operating-system directory is a property of that injected +filesystem and was not treated as a separate Promptkit security boundary. + +### Coverage Ledger + +- **Discovery:** OS and `fs.FS` discovery recurse, accept only lowercase + `.yaml` and `.yml` suffixes, filter backups and other files, honor + cancellation during walking, and return lexically sorted paths. Sorting + makes duplicate diagnostics and scan order deterministic. No repeated scan + within one lookup was found; each public point-in-time operation initiates + one expected scan. +- **Root and relative paths:** `CleanFSRoot`, `DisplayPath`, `RelativePath`, + and `ResolveFSPath` produce clean source-relative diagnostics and reject + lexical `fs.FS` parent and absolute escapes. The OS content resolver does + not use equivalent containment and all content resolvers alter exact + whitespace-bearing names, as recorded in S07-F01. +- **Selection and duplicates:** Valid normalized ID/version pairs are selected + independent of directory nesting, and sorted match paths make ambiguity + errors stable. Version omission requires exactly one ID match; an explicit + version permits other valid versions and rejects duplicate exact pairs. + Error association before canonical selection is defective as S07-F02 + records. +- **Strict YAML and contextual failures:** Known-field decoding rejects + unknown nested input and cache-control fields, and selected YAML, + definition, content-read, duplicate, directory, not-found, and cancellation + outcomes retain useful sentinels and paths through the public facade. + Additional documents escape decoding as S07-F03 records. Unrelated + malformed definitions are otherwise ignored so a point lookup is not a + whole-catalog validity check. +- **Definition normalization:** IDs, versions, roles, input names, metadata, + session templates, schema paths, default profiles, cache values, and enum + declarations are normalized or validated into domain values. Inputs remain + ordered and unique after trimming; messages require exactly one inline or + file-backed body; cache control accepts only `ephemeral` with empty or `1h` + TTL; output format, validation mode, schema dependency, and nonnegative + repair attempts are enforced. The material test omissions are S07-F06. +- **Inline and file-backed content:** Inline templates preserve their body + while whitespace-only content is rejected. Selected file-backed bodies are + read eagerly so exact inspection and preparation validate the reference at + the same point-in-time boundary. Containment and exact-name defects are + S07-F01; unrelated eager reads are S07-F04. Rendering syntax and input-helper + behavior remain Stage 9 scope. +- **Source parity and ownership:** Both sources share domain normalization but + duplicate discovery-to-selection orchestration. S07-F05 records the + resulting policy ownership and drift risk. File-catalog helpers remain + appropriately shared with profile and validator packages rather than + embedding prompt-specific behavior. +- **Fixtures and test value:** Static fixtures efficiently cover representative + valid definitions, nested strict YAML, cache control, and content references; + dynamic temporary files cover nesting, ambiguity, and contextual selection. + The `fs.FS` cases add distinct source-containment and parity protection + rather than repeating the full OS fixture matrix. No fixture should be + removed solely for sharing a YAML shape; S07-F06 recommends a compact table + only for currently absent scalar validation branches. + +### Verification Performed + +The code knowledge graph was used to inventory both packages, trace discovery +and path helpers into prompt, profile, and validator consumers, trace prompt +repository resolution through exact inspection and preparation, and compare +the two source implementations. Important behavior was confirmed against all +source, tests, fixtures, and canonical format and source documentation. + +The following focused commands passed: + +```sh +go test -cover ./internal/filecatalog ./internal/promptdef +go test ./internal/usecase -run 'Test(RunnerInspectPromptResolvesOneDefinitionWithoutExecutionCollaborators|RunnerInspectPromptClassifiesFailuresWithoutRepositoryWorkAfterCancellation|RunnerPrepareUsesThePromptInspectionSelectionAndHash|RunnerPrepareFileBackedPromptBodiesRenderCorrectly)$' +go test . -run 'Test(InspectPromptReturnsDeclaredMetadataWithoutExecutionWork|InspectPromptPreservesPublicErrorIdentities|InspectPromptReturnsIndependentMetadataMatchingPreparation|PrepareWorksWithPromptFSAndRelativeContentFile|PrepareWithPromptFSRejectsEscapedContentFile|PrepareWorksWithPromptFile|PromptRepositoryReadFailureMapsToPromptLoad)$' +go test -race ./internal/filecatalog ./internal/promptdef -count=3 +go vet ./internal/filecatalog ./internal/promptdef +``` + +The coverage diagnostic reported 92.9% statement coverage for +`internal/filecatalog` and 84.3% for `internal/promptdef`. Coverage output was +used only to locate unexamined decisions; S07-F06 is based on direct comparison +of production validation branches with tests and fixtures. + +Temporary package probes, removed before this artifact was edited, confirmed: + +- an OS-directory `../outside.txt` content reference returned the outside + sentinel body; +- an existing whitespace-suffixed content filename was changed before lookup; +- malformed `target.yaml` shadowed a valid prompt whose YAML ID was `target`; +- an invalid unrequested version blocked a valid requested version; +- a second YAML document with an unknown field was ignored; and +- a counting filesystem observed one unrelated template-body open per exact + lookup. + +### Handoff + +- The Stage 0 baseline remains absent and was not backfilled during this + discovery and prompt-definition review. +- Stage 8 owns profile-specific decoding, selection, source parity, and + repository composition. It should reuse the established file-catalog + behavior but independently assess whether profile selection or YAML stream + handling has analogous defects. +- Stage 9 owns ordinary artifact reads and prompt/session rendering, including + template parsing, variables, input helpers, cache-control propagation, and + rendered-message ownership. This stage established only the definition and + selected template bodies supplied to it. +- Stage 10 owns schema-source containment, schema loading, reference + resolution, validation modes at execution, and frozen validation plans. + Prompt parsing here only verifies the declared output fields.