Record prompt source audit findings
This commit is contained in:
351
audit.md
351
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.
|
||||
|
||||
Reference in New Issue
Block a user