Record artifact loading and prompt rendering audit findings
This commit is contained in:
304
audit.md
304
audit.md
@@ -1935,3 +1935,307 @@ Temporary package probes, removed before this artifact was edited, confirmed:
|
||||
- Stage 14 owns default-client reserved-field rejection and outbound JSON
|
||||
payload construction. It should distinguish those transport-specific checks
|
||||
from the universal profile-shape defect in S08-F02.
|
||||
|
||||
## Stage 9: Artifact Loading And Prompt Rendering
|
||||
|
||||
### Scope Reviewed
|
||||
|
||||
The review covered every production source and focused test in
|
||||
`internal/artifact` and `internal/prompt`. The artifact-reference and prompt
|
||||
format contracts, internal source documentation, public artifact constructors
|
||||
and reader contract, and the narrow preparation call path were consulted to
|
||||
establish ownership, error translation, and package use. The runner's decision
|
||||
about when to load or render, output validation, and execution coordination
|
||||
were not audited.
|
||||
|
||||
### Accepted Findings
|
||||
|
||||
#### S09-F01: Empty inline content is rejected while empty file content is valid
|
||||
|
||||
- **Category:** correctness
|
||||
- **Severity:** medium
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/artifact/reader.go` (`inlineReader.Read`), empty
|
||||
body coverage in `internal/artifact/reader_test.go`, and the public `Inline`
|
||||
and `InlineWithURI` constructors in `types.go`
|
||||
- **Contract at issue:** An inline reference's `Body` is its content, and a
|
||||
required prompt input must be present. Neither the public constructors nor
|
||||
the format contract requires that present content be non-empty. Source type
|
||||
should not change whether the same zero-byte artifact can be materialized.
|
||||
- **Evidence:** `inlineReader.Read` returns `ErrMissingInlineBody` whenever
|
||||
`ref.Body == ""`, even though an explicit inline `Type` already distinguishes
|
||||
the reference from an omitted map entry. The file reader accepts a zero-byte
|
||||
file and returns size zero plus the empty-content hash. A temporary probe
|
||||
confirmed those opposite outcomes through one `CompositeReader`; the
|
||||
existing inline test explicitly preserves the rejection while no contract
|
||||
states it.
|
||||
- **Failure mode:** A required input that is present but intentionally empty
|
||||
fails with `ErrArtifactLoad` when constructed with `Inline("")`, while the
|
||||
equivalent `File(pathToEmptyFile)` prepares and renders successfully.
|
||||
Consumers cannot choose the source representation independently of content
|
||||
semantics.
|
||||
- **Recommended direction:** Treat an explicitly typed inline reference with
|
||||
an empty body as a valid zero-byte artifact, computing the same metadata and
|
||||
opaque equality value as any other body. Keep absence at the request input
|
||||
map and unsupported-reference boundaries rather than inferring it from
|
||||
content length.
|
||||
- **Required verification:** Add a source-parity table for empty and non-empty
|
||||
inline, inline-with-URI, and file content. Exercise an empty required input
|
||||
through preparation and through both message and session `input` helpers,
|
||||
retaining the ordinary unsupported-type and missing-file-path failures.
|
||||
|
||||
#### S09-F02: Cancellation cannot stop a blocking default file read
|
||||
|
||||
- **Category:** correctness
|
||||
- **Severity:** high
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/artifact/reader.go` (`fileReader.Read` and
|
||||
`readFileArtifact`) and cancellation coverage in
|
||||
`internal/artifact/reader_test.go`
|
||||
- **Contract at issue:** The public reader contract requires readers to honor
|
||||
context cancellation so preparation remains responsive, and the internal
|
||||
source contract says the ordinary reader does so. The documented default
|
||||
opens unrestricted caller-selected operating-system paths, making blocking
|
||||
path kinds part of the current boundary unless explicitly rejected.
|
||||
- **Evidence:** The file reader checks `ctx.Done()` only before calling
|
||||
`os.Open`; `readFileArtifact` then uses an unbounded `io.ReadAll` without the
|
||||
context. The maintained cancellation test covers only a context canceled
|
||||
before an inline read. In a temporary Linux FIFO probe, cancellation after
|
||||
the reader entered `Read` did not return within 50 milliseconds. Opening and
|
||||
closing the FIFO writer was still required to release the read, which then
|
||||
returned success despite the canceled context. The result repeated three
|
||||
times.
|
||||
- **Failure mode:** `Prepare`, `PrepareExecution`, or `Run` can remain blocked
|
||||
indefinitely after cancellation when a selected path is a FIFO or device,
|
||||
and a producing stream can keep `io.ReadAll` consuming memory and work with
|
||||
no cancellation checkpoint. Caller responsibility for path authorization
|
||||
and size policy does not satisfy the reader's own cancellation contract.
|
||||
- **Recommended direction:** Establish a cancellable ordinary-file operation:
|
||||
reject unsupported non-regular path kinds before consuming them or arrange
|
||||
for cancellation to interrupt the underlying open/read, and check context
|
||||
between bounded read chunks. Preserve the application-owned containment and
|
||||
request-size policies instead of introducing a hidden application limit.
|
||||
- **Required verification:** Add a platform-appropriate blocking-file test
|
||||
that proves cancellation releases the operation without an external writer
|
||||
and never returns a partial artifact. Also cancel a progressing large
|
||||
regular read, retain pre-canceled inline and file cases, and run the package
|
||||
repeatedly under the race detector to catch cleanup leaks.
|
||||
|
||||
#### S09-F03: Session rendering runs before the renderer observes cancellation
|
||||
|
||||
- **Category:** correctness
|
||||
- **Severity:** medium
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/prompt/go_renderer.go` (`goRenderer.Render`,
|
||||
`renderSessionID`, and the `input` template helper) and
|
||||
`internal/prompt/renderer_test.go`
|
||||
- **Contract at issue:** `Renderer.Render` accepts the operation context and
|
||||
already treats cancellation as a rendering boundary. Session and message
|
||||
templates use the same data and helper semantics, so all potentially
|
||||
material rendering work should observe that context coherently.
|
||||
- **Evidence:** `Render` validates inputs and fully parses, executes, and
|
||||
normalizes the session template before its first context check. It checks
|
||||
only before each message parse, not during session or message execution or
|
||||
inside the body-copying `input` helper. A temporary pre-canceled-context
|
||||
probe with a malformed session returned `ErrInvalidTemplate`, not
|
||||
`context.Canceled`, proving session work preceded the first observation.
|
||||
Focused tests contain no cancellation case.
|
||||
- **Failure mode:** A canceled preparation can continue parsing and rendering
|
||||
a session, including copying and writing a large artifact body. Cancellation
|
||||
that arrives during a single large message also has no effect until that
|
||||
template finishes, and has no effect at all when it is the last message.
|
||||
- **Recommended direction:** Check the context before session work, around
|
||||
each parse and execution boundary, and from template helpers before they
|
||||
perform body-sized work. Keep cancellation synchronous and leak-free; do not
|
||||
wrap uninterruptible template execution in an abandoned goroutine merely to
|
||||
return early.
|
||||
- **Required verification:** Cover a context canceled before rendering, during
|
||||
a body-heavy session helper, and during a body-heavy final message. Assert
|
||||
no partial rendered prompt is returned and retain malformed-template and
|
||||
missing-input identities when the context is active.
|
||||
|
||||
#### S09-F04: Every input-helper invocation allocates another full artifact body
|
||||
|
||||
- **Category:** efficiency
|
||||
- **Severity:** medium
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** the `input` closure in
|
||||
`internal/prompt/go_renderer.go` and input-helper cases in
|
||||
`internal/prompt/renderer_test.go`
|
||||
- **Contract at issue:** Materialized artifact bytes must remain isolated, but
|
||||
repeated references during one render do not require repeated immutable
|
||||
byte-to-string copies. The session and all messages share one artifact set
|
||||
and one render lifetime.
|
||||
- **Evidence:** Each `{{input "name"}}` call executes `string(art.Body)` afresh.
|
||||
A temporary benchmark rendering one 1 MiB body through the helper used about
|
||||
4.20 MiB and 67--68 allocations per operation, while rendering the same
|
||||
already-string value through template data used about 3.15 MiB and 63--64
|
||||
allocations. The approximately 1 MiB difference is the avoidable full-body
|
||||
conversion for just one helper call. Referencing the same body in a session
|
||||
and multiple messages repeats it each time in addition to the required
|
||||
rendered output storage.
|
||||
- **Failure mode:** Common prompts that reuse a large transcript or document
|
||||
create one extra body-sized allocation per reference, increasing peak memory
|
||||
and garbage-collection pressure during preparation.
|
||||
- **Recommended direction:** Lazily memoize one string conversion per named
|
||||
non-nil artifact for the duration of `Render`, while retaining the current
|
||||
unknown/nil input errors and the artifact byte ownership boundary. Do not
|
||||
cache across render calls or mutate the artifact.
|
||||
- **Required verification:** Add an allocation benchmark comparing one and
|
||||
repeated references across session and messages before and after the change.
|
||||
Keep behavioral tests for exact rendered bytes, invalid UTF-8 preservation,
|
||||
unknown and nil inputs, and independent artifact ownership.
|
||||
|
||||
#### S09-F05: Artifact tests make an opaque hash algorithm a test contract
|
||||
|
||||
- **Category:** testing
|
||||
- **Severity:** low
|
||||
- **Confidence:** high
|
||||
- **Status:** accepted
|
||||
- **Affected code:** inline and file success cases in
|
||||
`internal/artifact/reader_test.go`
|
||||
- **Contract at issue:** Public artifact hashes are opaque content-equality
|
||||
values whose format and algorithm are explicitly not API contracts. Focused
|
||||
tests should protect stable equality behavior and source parity rather than
|
||||
make replacement of one correct internal algorithm require test rewrites.
|
||||
- **Evidence:** The two primary success cases duplicate exact 64-character
|
||||
SHA-256 literals for their fixture bodies. Higher-level tests appropriately
|
||||
check that hashes are present, forwarded, or relationally unchanged instead
|
||||
of duplicating the encoding. No artifact contract names SHA-256 as required
|
||||
behavior.
|
||||
- **Failure mode:** Replacing the hash with another deterministic opaque
|
||||
equality implementation breaks focused tests despite preserving the public
|
||||
contract, while the current constants do not protect the more important
|
||||
same-content parity and changed-content distinction across source types.
|
||||
- **Recommended direction:** Assert non-empty, deterministic hashes for repeat
|
||||
reads; equal hashes for equal inline and file bodies; and unequal hashes for
|
||||
changed bodies. Retain exact known-vector coverage only if SHA-256 is made a
|
||||
deliberate internal compatibility requirement and documented as such.
|
||||
- **Required verification:** Run the relational matrix for empty, ordinary,
|
||||
and changed bodies through inline and file readers, then retain one
|
||||
preparation assertion that every supplied input hash is propagated without
|
||||
interpreting its representation.
|
||||
|
||||
### Unresolved Observations
|
||||
|
||||
None. MIME lookup may vary with the host database for known extensions, but
|
||||
the reader provides the documented fallback and exposes reader-supplied
|
||||
metadata rather than promising one cross-host MIME registry. The renderer
|
||||
parses each point-in-time definition once per template per operation; no
|
||||
duplicate read, hash, or parse within its package boundary was found.
|
||||
|
||||
### Coverage Ledger
|
||||
|
||||
- **Materialization and dispatch:** The composite reader deterministically
|
||||
routes supported inline and file references, preserves unsupported-type,
|
||||
missing-path, open, and read failures, and does not apply consumer-specific
|
||||
path containment or size policy. Empty-body source parity is defective as
|
||||
S09-F01 records.
|
||||
- **Ownership and metadata:** Inline string conversion and `io.ReadAll` create
|
||||
fresh body storage; repeat-read mutation tests protect inline isolation, and
|
||||
the engine adapter immediately copies injected-reader bodies. The ordinary
|
||||
reader reports name, URI, byte size, content type with fallback, and a
|
||||
content hash without sharing mutable bytes. No package-owned aliasing defect
|
||||
was found.
|
||||
- **Caller-selected paths:** Absolute, relative, symlinked, and otherwise
|
||||
caller-selected OS paths are intentionally unrestricted by an application
|
||||
root. Authorization, containment, application request-size limits, and
|
||||
sensitive logging remain injected-consumer responsibilities. Blocking path
|
||||
cancellation is the package-owned defect in S09-F02.
|
||||
- **Input semantics:** Required declarations reject absent and nil artifacts;
|
||||
optional inputs may be absent; template references reject unknown and nil
|
||||
artifacts; and extra supplied inputs are allowed. Input names appear in
|
||||
diagnostics but artifact bodies and variable values do not. Empty present
|
||||
inline input behavior is accounted for by S09-F01.
|
||||
- **Template behavior:** Session and message templates use Go template parsing
|
||||
with missing-map-key errors, artifact helpers, and string variables. Roles,
|
||||
message order, whitespace, rendered content, and normalized session IDs are
|
||||
carried deterministically. Nil definitions, malformed templates, execution
|
||||
failures, unknown inputs, empty roles, empty sessions, and overlong sessions
|
||||
return no partial prompt. Context responsiveness is incomplete as S09-F03
|
||||
records.
|
||||
- **Cache control and ownership:** Each non-nil cache-control value is copied
|
||||
into its rendered message, and nil remains nil. Rendered message storage and
|
||||
buffers are operation-local; later source mutation cannot change an already
|
||||
prepared prompt.
|
||||
- **Repeated work:** Artifacts are read and hashed once each before the
|
||||
renderer boundary, and each distinct session or message template is parsed
|
||||
once in one render. Point-in-time definitions make cross-operation parse
|
||||
caching a different lifetime decision with no demonstrated need. Repeated
|
||||
input-helper body conversions are the measured waste in S09-F04.
|
||||
- **Diagnostics and determinism:** Error text identifies the input name,
|
||||
message index, or file path needed to diagnose the failure without embedding
|
||||
bodies or variable values. Input map iteration occurs in the runner rather
|
||||
than either scoped package and was not audited here. Rendered order follows
|
||||
definition order and no shared mutable package state was found.
|
||||
- **Test ownership:** Artifact package tests own dispatch, materialization,
|
||||
metadata, ownership, and focused failures; renderer tests own templates,
|
||||
inputs, variables, sessions, roles, and cache control. Use-case tests own
|
||||
error categorization and the real file-content-to-render integration. The
|
||||
renderer subtest labelled file-backed receives already loaded `Content` and
|
||||
cannot exercise `ContentFile`; it is small overlap, while the use-case test
|
||||
provides the actual boundary protection. Exact opaque hash coupling is the
|
||||
actionable test friction in S09-F05.
|
||||
|
||||
### Verification Performed
|
||||
|
||||
The code knowledge graph was used to inventory both scoped packages, trace
|
||||
`Reader.Read` and `Renderer.Render` into preparation, identify their complete
|
||||
focused test surfaces, and confirm that materialization and rendering have one
|
||||
production consumer. The full package source and tests were then checked
|
||||
against public GoDoc, format and source contracts, and the architecture and
|
||||
testing policies.
|
||||
|
||||
The following focused commands passed:
|
||||
|
||||
```sh
|
||||
artifact_audit_cover=$(mktemp)
|
||||
go test -coverprofile="$artifact_audit_cover" ./internal/artifact
|
||||
go tool cover -func="$artifact_audit_cover"
|
||||
rm "$artifact_audit_cover"
|
||||
prompt_audit_cover=$(mktemp)
|
||||
go test -coverprofile="$prompt_audit_cover" ./internal/prompt
|
||||
go tool cover -func="$prompt_audit_cover"
|
||||
rm "$prompt_audit_cover"
|
||||
go test ./internal/usecase -run 'Test(RunnerDirectSessionResolution|RunnerPrepareFileBackedPromptBodiesRenderCorrectly|RunnerPrepareRequiredInputMissingFails|RunnerPrepareUnknownTemplateInputReferenceFails|RunnerRunArtifactLoadFailure|RunnerRunPromptRenderFailure|HashRenderedPromptIncludesCacheControlWhenPresent|HashRenderedPromptIncludesSessionIDWhenPresent)$'
|
||||
go test . -run 'Test(PublicArtifactReaderAdapterCopiesBody|PrepareWorksWithInlineInputs|EngineRunWithDirectorySourcesAndFileInputs|ArtifactReaderReceivesPublicReferenceAndPreparesArtifact|ArtifactReaderFailuresPreserveArtifactLoadErrors)$'
|
||||
go test -race ./internal/artifact ./internal/prompt -count=3
|
||||
go vet ./internal/artifact ./internal/prompt
|
||||
```
|
||||
|
||||
The coverage diagnostics reported 89.7% statement coverage for
|
||||
`internal/artifact` and 93.3% for `internal/prompt`. Coverage was used only to
|
||||
locate unexercised decisions for direct inspection; the accepted findings rest
|
||||
on source traces, contract comparison, reproduced behavior, and a focused
|
||||
allocation measurement.
|
||||
|
||||
Temporary package probes, removed before this artifact was edited, confirmed:
|
||||
|
||||
- empty inline content was rejected while an empty file loaded successfully;
|
||||
- canceling an in-progress FIFO read did not return until a writer externally
|
||||
released it, after which the canceled read returned success;
|
||||
- a pre-canceled render parsed an invalid session and returned the template
|
||||
error before observing cancellation; and
|
||||
- rendering a 1 MiB artifact through the input helper allocated approximately
|
||||
one additional body-sized buffer compared with equivalent string template
|
||||
data.
|
||||
|
||||
### Handoff
|
||||
|
||||
- The Stage 0 baseline remains absent and was not backfilled during this
|
||||
artifact and rendering review.
|
||||
- Stage 10 owns output artifact normalization, schema source loading, and
|
||||
frozen validation plans. It should treat the loaded input ownership and
|
||||
metadata boundaries recorded here as established rather than extending the
|
||||
ordinary artifact reader into schema policy.
|
||||
- Stage 12 owns the runner's ordering decisions, operation-level error
|
||||
categories, and coordination around these collaborators. It should treat
|
||||
the reader and renderer behavior recorded here as established package
|
||||
inputs; this pass did not judge when the runner chooses to render.
|
||||
- Stage 17 owns repository-wide consolidation and cross-cutting efficiency.
|
||||
S09-F04 supplies a measured local allocation candidate without asserting a
|
||||
broader caching policy.
|
||||
|
||||
Reference in New Issue
Block a user