diff --git a/audit.md b/audit.md index 932758e..631c165 100644 --- a/audit.md +++ b/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.