diff --git a/audit.md b/audit.md index 631c165..ae1b191 100644 --- a/audit.md +++ b/audit.md @@ -2239,3 +2239,370 @@ Temporary package probes, removed before this artifact was edited, confirmed: - Stage 17 owns repository-wide consolidation and cross-cutting efficiency. S09-F04 supplies a measured local allocation candidate without asserting a broader caching policy. + +## Stage 10: Output Validation And Frozen Validation Plans + +### Scope Reviewed + +The review covered every production source and focused test in +`internal/validate`, the maintained framework schema fixture, public schema +source options and validation GoDoc, and the narrow use-case paths that load a +schema document or prepare and retain a frozen validation plan. Root and +use-case tests were consulted only for schema-source composition, structured +output ownership, validation result translation, and prepared-plan lifetime. +Repair decisions and provider request construction were not audited. + +### Accepted Findings + +#### S10-F01: Float64 decoding changes JSON and JSON Schema semantics + +- **Category:** correctness +- **Severity:** high +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/validate/standard_validator.go` (`parseJSON`, + `loadJSONSchemaFile`, `FSValidator.loadSchemaDocument`, both + `LoadSchemaDocument` methods, and the schema resource loaders) and numeric + cases absent from `internal/validate/standard_validator_test.go` +- **Contract at issue:** `json` mode requires syntactically valid JSON, and + `json_schema` mode must evaluate the exact JSON value against the exact + selected schema. JSON numbers are not limited to `float64`, and loading a + schema for structured-output metadata must not silently change its numeric + constraints. +- **Evidence:** Every schema and generated instance is decoded with + `json.Unmarshal` into `any`, which represents numbers as `float64`. A + temporary probe first confirmed that `encoding/json.Valid` accepts `1e400` + while `ValidationJSON` reports it as invalid because unmarshalling into + `float64` overflows. A schema with + `"const": 9007199254740992` then incorrectly accepted generated output + `9007199254740993`; the distinct integers collapse to the same binary float. + The bundled `jsonschema/v6` implementation deliberately uses + `Decoder.UseNumber` and exact rational arithmetic in its own loaders, but + Promptkit's custom loaders discard that precision before the library sees + either value. +- **Failure mode:** Syntactically valid model output can be falsely rejected, + and JSON Schema validation can falsely accept an output that violates + `const`, bounds, or other numeric constraints. Large numeric constraints in + the schema document exposed through `PreparedRun.StructuredOutput` can also + be rounded before a caller receives its copy. +- **Recommended direction:** Decode schema documents and schema-validation + instances with `json.Decoder.UseNumber`, enforcing exactly one complete JSON + value. Implement `json` mode as syntax validation rather than discarded + generic materialization, while preserving the exact documented JSON + acceptance rule. Keep numeric values exact through schema compilation, + structured-output copying, and validation. +- **Required verification:** Cover the integers at and around `2^53`, a large + exponent such as `1e400`, precise decimals, and ordinary finite numbers in + both source types. Exercise `const`, minimum/maximum, and `multipleOf`, and + assert that the caller-owned structured-output schema round-trips the same + numeric spelling and value. Retain malformed syntax and trailing-value + failures. + +#### S10-F02: Valid schema filenames are passed to the compiler as unescaped URLs + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/validate/standard_validator.go` + (`StandardValidator.PrepareValidation`, + `StandardValidator.validateJSONSchema`, `fsSchemaResourceURL`, and both + compiler setup paths) plus + `TestFSValidatorJSONSchemaRegistrationError` +- **Contract at issue:** `schema_path` names a file within the configured + source. Filesystem names are not URI strings, and the format and public + source contracts do not prohibit percent signs or other URI-reserved + characters in otherwise valid filenames. +- **Evidence:** The standard validator gives an absolute filesystem path + directly to `Compiler.Compile`, while `fsSchemaResourceURL` concatenates the + resolved `fs.FS` path directly after `promptkit-schema:///`. Neither path is + URL-escaped. A temporary probe created `%zz.json` in both an OS directory and + an `fstest.MapFS`; direct JSON Schema validation failed in both cases with + an invalid URL escape even though the file was found and decoded. The + existing FS registration-error test uses this legal filename to require the + failure, turning an encoding defect into expected behavior. +- **Failure mode:** Consumers cannot select schemas whose names contain some + legal percent, fragment, query, space, or Unicode characters. A path can + pass containment and file access, then fail only when the implementation + reinterprets it as an unescaped compiler resource identifier. +- **Recommended direction:** Convert OS paths to canonical escaped file URLs + and construct custom `fs.FS` resource URLs by escaping path segments while + preserving separators. Keep schema paths as exact source names and reserve + URL decoding for actual `$ref` resource identifiers. +- **Required verification:** Run the same schema through directory, + `WithSchemaFS`, and `WithSchemaFile` sources with percent, space, `#`, `?`, + and Unicode filename characters. Add relative references from those root + schemas and ensure encoded parent escapes and remote references remain + rejected. Replace the current `%zz.json` expected failure with a real + registration failure only if one remains reachable through a valid source + name. + +#### S10-F03: Ordinary preparation publishes schema graphs it has not compiled + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/usecase/runner.go` + (`resolveStructuredOutput`), `internal/usecase/prepared_execution.go` + (`prepareValidation` and `structuredOutputFromValidationPlan`), the + `SchemaDocumentLoader` and `ValidationPreparer` split in + `internal/validate/validator.go`, and JSON Schema preparation tests in the + root and use-case suites +- **Contract at issue:** An unreadable, invalid, or unresolvable schema is an + operational validation error. Preparation that returns provider-facing JSON + Schema metadata must account for the selected schema graph rather than + publish a root document whose keywords or transitive references are known + only later to be unusable. +- **Evidence:** Ordinary `Prepare` calls `LoadSchemaDocument`, which only + reads, decodes, and checks the root dialect. `PrepareExecution` instead calls + `PrepareValidation`, which compiles the schema and resolves every reference + before returning the root document from the plan. In temporary public + probes, ordinary `Prepare` succeeded and returned structured-output metadata + for both `{"type":42}` and `{"$ref":"missing.json"}`; the same requests + failed `PrepareExecution` with `ErrValidation`. Existing ordinary + preparation tests cover a missing root file but not invalid keywords or + references. A counting-filesystem probe also showed an ordinary JSON Schema + `Run` reads the root document twice: once for metadata and again when live + validation compiles it. +- **Failure mode:** Two preparation APIs disagree about whether the same + schema source is valid. Offline callers can receive a successful + `PreparedRun` containing an unusable schema, and the document-only path + duplicates root I/O and parsing when execution later needs compilation. +- **Recommended direction:** Give JSON Schema preparation one operation-local + compiled-plan path. Derive structured-output metadata from that plan for all + preparation workflows; an ordinary `Prepare` may discard the compiled + validator after returning, while an executable workflow retains it. Keep + source access point-in-time across separate operations rather than adding a + stale engine-wide schema cache. +- **Required verification:** At both public preparation boundaries, reject an + invalid keyword, malformed referenced document, missing direct and + second-level reference, unsupported referenced dialect, and escaping or + remote reference. Confirm a valid multi-document graph returns the same root + metadata from both APIs, and use a counting source to prove each document is + read once within one preparation. + +#### S10-F04: Validation contexts are observed only outside expensive work + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/validate/standard_validator.go` + (`validateArtifact`, both `PrepareValidation` methods, + `LoadSchemaDocument`, schema loaders, JSON parsing, schema compilation, and + schema execution) and cancellation coverage in + `internal/validate/standard_validator_test.go` +- **Contract at issue:** Public execution contexts cover validation, and + preparation contexts govern schema preparation. The validator accepts those + contexts directly, so cancellation must make CPU-heavy parsing and + validation and blocking source work responsive rather than acting only as + admission checks. +- **Evidence:** `validateArtifact` checks the context once before parsing or + validation and never checks it again. The preparation methods check before + work and after compilation, but no context reaches path resolution, file + reads, custom filesystem opens, decoding, reference loading, compilation, + or schema execution. A temporary blocking-`fs.FS` probe remained stuck after + cancellation until the filesystem was externally released, then returned + the delayed context error. A second probe canceled while the schema + validation callback was active; after release, validation returned + `ValidationPassed` rather than the context error. No focused cancellation + test exists. +- **Failure mode:** Canceling `PrepareExecution`, `Run`, or `RunPrepared` can + leave it blocked on schema I/O or consuming CPU and allocations for a large + document. Cancellation during the final schema check can be ignored + completely and return a normal validation result. +- **Recommended direction:** Carry cancellation into bounded schema reads and + JSON parsing, add checkpoints around compilation and execution, and use an + interruptible or explicitly bounded validation mechanism where the + dependency offers no context API. Avoid returning early through abandoned + goroutines that retain source or schema work. +- **Required verification:** Deterministically cancel during a source read, + large JSON parse, transitive-reference compilation, and final prepared-plan + validation. Require prompt return, no partial result, the owning context + identity, and no leaked goroutines; repeat under the race detector. + +#### S10-F05: JSON syntax validation materializes a discarded object graph + +- **Category:** efficiency +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/validate/standard_validator.go` (`parseJSON` + and the `ValidationJSON` branch of `validateArtifact`) and JSON-mode + performance coverage +- **Contract at issue:** `json` mode answers only whether generated bytes are + valid JSON. It does not expose, transform, or schema-check a decoded value, + so allocating a complete generic tree adds no contract value. +- **Evidence:** The JSON branch calls `parseJSON`, which unmarshals the entire + body into maps, slices, strings, and numbers, then discards the value. A + temporary benchmark on an approximately 1 MiB JSON array measured about + 22.7 MiB and 250,031 allocations per validator call, taking 31--38 ms on the + audit host. A syntax-only scan of the same bytes used zero allocations and + about 4.0--4.2 ms. The finding is the avoidable materialization; the exact + benchmark timings are diagnostic rather than a performance contract. +- **Failure mode:** Large JSON output creates substantial transient heap and + garbage-collection pressure on every run, even though successful validation + retains only a small result value. Concurrent engine calls multiply that + cost. +- **Recommended direction:** Use a non-materializing syntax check for + `ValidationJSON`. Keep exact-number decoding only for JSON Schema mode, + where the validator genuinely consumes the instance tree. +- **Required verification:** Retain a benchmark reporting bytes and + allocations for representative scalar, object, and large-array outputs. + Behavioral cases must preserve the decided valid-JSON semantics for + whitespace, trailing data, malformed strings, exact large numbers, and + nested values. + +### Unresolved Observations + +None. The public engine does not expose prepared validation plans for repeated +concurrent use, but the immutable plan and the schema library's call-local +validation state were reviewed and a concurrent race-enabled probe passed. +Cross-operation schema caching was not recommended because configured sources +are intentionally point-in-time. + +### Coverage Ledger + +- **None and basic modes:** None returns a skipped, valid result without + changing the artifact. Basic rejects empty and whitespace-only content and + passes nonblank content; validation checks do not normalize or mutate the + returned output bytes. Nil artifacts and unsupported internal modes return + operational errors, while normalized public definitions and requests keep + those states away from ordinary package callers. +- **JSON mode:** One complete ordinary JSON object is accepted and malformed + syntax becomes a failed validation result rather than an operational error. + The discarded generic tree causes S10-F05, and its `float64` decoding changes + the syntactic acceptance contract as S10-F01 records. The original artifact + and raw model output remain byte-for-byte unchanged. +- **JSON Schema mode:** Malformed generated JSON is a completed failed result; + schema violations are completed failed results; source access, document + decoding, dialect, registration, and compilation failures are operational + errors. Draft 2020-12 is the explicit or default dialect, and incompatible + declared dialects are rejected in roots and loaded references. Exact numeric + behavior is defective as S10-F01 records. +- **Schema path resolution:** OS paths are resolved beneath the symlink-aware + configured root; directory-backed `fs.FS` paths use lexical source + containment; a single-file source accepts only its base name. Both loaders + reject escaping and remote references and resolve contained relative + references from the owning document. S07-F01 already owns whitespace-based + exact-path alteration in the shared file-catalog path helper; S10-F02 owns + the distinct filesystem-path-to-resource-URL encoding defect. +- **Schema graphs and freezing:** `PrepareValidation` loads and compiles the + complete graph before returning. The compiled schema and decoded root are + retained without source handles; maintained OS deletion and mutable + `fstest.MapFS` tests prove later validation uses the captured root and direct + reference. A second-level reference is not currently in that regression + matrix, but compiler traversal and the custom loader path were fully + inspected. Ordinary preparation bypasses this graph guarantee as S10-F03 + records. +- **Ownership:** Schema decoding creates source-independent maps and slices. + The use case takes the plan's internal root document, then its prepared-run + cloning and public conversion create caller-owned structured-output trees; + caller mutation tests protect isolation from the retained execution plan. + Validation errors allocate per result, and neither validation mode mutates + the input artifact. +- **Concurrency:** Standard and FS validators keep only immutable source + references and create compilers per live validation. A prepared plan retains + an immutable compiled schema; the dependency creates all validation walker + state per call. A temporary 32-worker mixed valid/invalid plan probe passed + three times under the race detector. Injected mutable filesystems remain + explicit source references; concurrent mutation during a live lookup is not + a frozen-plan mechanism. +- **Cancellation:** Pre-canceled validation and preparation are rejected by + source inspection, but there are no maintained cancellation tests and active + work is not interruptible. S10-F04 records both the delayed preparation and + ignored validation outcomes. +- **Repeated work:** A frozen plan compiles once and validates repeatedly + without reopening source documents. Live JSON Schema validation deliberately + creates a fresh compiler so separate operations observe point-in-time + sources. The document-loader and plan-preparer split adds the duplicate + within-operation root read recorded in S10-F03; JSON-only tree allocation is + S10-F05. +- **Diagnostics and result metadata:** Diagnostics distinguish malformed + generated JSON, schema rejection, source read/decode, dialect, registration, + and compilation failures with useful paths or schema locations. Public + GoDoc warns that validation errors may contain sensitive output-derived + data. Mode, schema path, status, validity, and attempts fields are copied + into fresh result values, and use-case tests own the actual repair-attempt + override without making repair policy part of this pass. +- **Test ownership:** Validator tests own modes, source mechanics, schema + compilation, references, dialects, result classification, and frozen-plan + source independence. Use-case tests own loader/preparer selection, + structured-output derivation, error categorization, and plan retention. + Root tests own configured source composition, public ownership, and result + translation. The missing numeric, compilation-parity, cancellation, and + allocation cases map directly to S10-F01, S10-F03, S10-F04, and S10-F05; + the `%zz.json` test currently preserves S10-F02 rather than a contract. + +### Verification Performed + +The code knowledge graph was used to inventory the validator package, locate +all mode and source entry points, trace schema document loading and validation +preparation into the use case, and identify the frozen plan's execution and +structured-output consumers. Same-named validator methods that the graph +conflated were read directly, together with every focused test, the framework +schema fixture, public GoDoc, and the canonical format and internal source +contracts. + +The following focused commands passed: + +```sh +validation_audit_cover=$(mktemp) +go test -coverprofile="$validation_audit_cover" ./internal/validate +go tool cover -func="$validation_audit_cover" +rm "$validation_audit_cover" +go test ./internal/usecase -run 'Test(RunnerPrepareJSONSchemaBuildsStructuredOutputSpec|RunnerPrepareJSONSchemaSchemaLoadFailureReturnsValidationError|RunnerRunJSONSchemaSchemaLoadFailureFailsBeforeLLM|RunnerPrepareExecutionCompletesWithoutAdmissionOrGeneration|RunnerRunPreparedUsesFrozenValidationForInitialAndRepairOutputs|RunnerPrepareExecutionRequiresValidationPreparer|RunnerPreparedExecutionWithoutValidatorSkipsValidation)$' +go test . -run 'Test(PrepareWorksWithFrameworkContractCorpus|RunStructuredOutputWorksWithSchemaFS|RunStructuredOutputWorksWithSchemaFile|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|EngineValidationIsSinglePass)$' +go test -race ./internal/validate -count=3 +go test -race . -run 'Test(PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|RunStructuredOutputWorksWithSchemaFS|RunStructuredOutputWorksWithSchemaFile)$' -count=3 +go vet ./internal/validate +``` + +The repository-wide `go test ./...`, `go test -race ./...`, `go vet ./...`, +and `go run ./examples/go-library/prepare` checks also passed. + +The coverage diagnostic reported 79.0% statement coverage for +`internal/validate`; prepared-plan construction reported 68.0% for the OS +source and 70.0% for `fs.FS`. Coverage was used only to locate decisions for +direct inspection. Findings are based on contracts, source and call-path +traces, reproduced behavior, and the allocation benchmark rather than the +percentages. + +Temporary probes, removed before this artifact was edited, confirmed: + +- `json` mode rejected syntactically valid `1e400`, and JSON Schema validation + treated the distinct integers `9007199254740992` and `9007199254740993` as + equal; +- both OS and `fs.FS` validators found and decoded `%zz.json` but failed when + its path was parsed as a compiler URL; +- ordinary `Prepare` published an invalid keyword and a missing reference that + `PrepareExecution` rejected; +- one ordinary JSON Schema run read the root document twice; +- cancellation waited for a blocked schema filesystem, while cancellation + during schema execution was ignored and returned a passed result; +- a 32-worker prepared-plan validation probe passed three race-enabled runs; + and +- JSON-only validation of an approximately 1 MiB value allocated about + 22.7 MiB in 250,031 allocations, versus zero allocations for a syntax-only + scan. + +### Handoff + +- The Stage 0 baseline remains absent and was not backfilled during this + validation review. +- S07-F01 already owns exact-path alteration in the shared file-catalog helper; + its effect on directory-backed schema paths is recorded here without a + duplicate finding. S10-F02 is the separate compiler-URL encoding defect. +- Stage 11 owns effective output-contract selection and preparation fidelity. + It should treat the compiled-plan and root-document invariants recorded here + as established inputs when comparing inspection and preparation. +- Stage 12 owns ordinary execution ordering and error coordination. S10-F03 + supplies confirmed evidence that the live path currently reads the root + twice; this stage did not review generation or repair decisions. +- Stage 14 owns the provider request representation of already prepared + structured-output metadata. It should not re-audit schema loading or numeric + preservation inside the validation package. +- Stage 17 owns cross-cutting efficiency consolidation. S10-F05 provides the + measured JSON-only allocation candidate, while source caching across + operations remains intentionally out of scope.