From 4ca3be2c1479f3585d6eda4653e8a857a06e1393 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 13:01:05 +0000 Subject: [PATCH] Finish audit remediation and prepare v0.6.0 --- README.md | 5 +- docs/releases/v0.6.0.md | 131 + docs/roadmap/audit-sequence.md | 635 --- docs/roadmap/audit.md | 4606 ------------------- docs/roadmap/implementation.md | 840 ---- engine_test.go | 74 + internal/promptdef/filesystem_repository.go | 2 +- internal/promptdef/repository_test.go | 36 + internal/usecase/prepared_execution_test.go | 83 +- internal/usecase/runner.go | 5 + 10 files changed, 308 insertions(+), 6109 deletions(-) create mode 100644 docs/releases/v0.6.0.md delete mode 100644 docs/roadmap/audit-sequence.md delete mode 100644 docs/roadmap/audit.md delete mode 100644 docs/roadmap/implementation.md diff --git a/README.md b/README.md index fc9f92b..8559433 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,10 @@ boundary and constraints that framework work must preserve. ## Release Guidance -Consumers upgrading from `v0.4.0` to `v0.5.0` should read the +Consumers upgrading from `v0.5.0` to `v0.6.0` should read the +[v0.6.0 changelog and migration guide](docs/releases/v0.6.0.md). + +Earlier adopters can consult the [v0.5.0 changelog and migration guide](docs/releases/v0.5.0.md). Consumers upgrading from `v0.3.0` to `v0.4.0` should read the diff --git a/docs/releases/v0.6.0.md b/docs/releases/v0.6.0.md new file mode 100644 index 0000000..aeef44c --- /dev/null +++ b/docs/releases/v0.6.0.md @@ -0,0 +1,131 @@ +# Promptkit v0.6.0 + +This supplemental changelog and migration guide summarizes the consumer-facing +changes from `v0.5.0` to `v0.6.0`. The annotated `v0.6.0` tag is the +authoritative release record. Exact current contracts belong to the linked +GoDoc and durable documentation. + +## Summary + +`v0.6.0` is a broad correctness, safety, efficiency, and maintainability +release. It does not add or remove public declarations. The release: + +- centralizes shared execution-setting, output-contract, endpoint, and + JSON-compatible-value rules; +- unifies prompt repository behavior and avoids unnecessary prompt and profile + decoding; +- bounds consumer-controlled JSON trees and successful provider responses; +- hardens prompt content paths, artifact files, provider URLs, JSON framing, + and error propagation; +- reuses compiled schema plans and rendered artifact text within an operation; + and +- improves cancellation behavior, prepared-value ownership, deterministic + transport testing, and maintainer validation. + +## Compatibility + +No public declaration was added, removed, or changed. Ordinary valid `v0.5.0` +configurations and requests should continue to compile and behave as before. + +The release intentionally rejects or reports several inputs that were +previously accepted, altered, or misclassified: + +- execution settings must be finite, within their documented ranges, and safe + to convert to Go durations; +- output formats, validation modes, repair counts, and JSON Schema dependencies + are validated consistently; +- file-backed prompt and profile identity comes from normalized YAML metadata, + not filenames; +- prompt `content_file` values must be exact relative paths contained by their + configured source root; +- built-in file artifacts must resolve to regular files; +- selected provider endpoints must be absolute HTTP or HTTPS URLs without user + information, query strings, or fragments; +- JSON documents and successful provider responses must contain exactly one + value, and successful provider bodies are limited to 16 MiB; and +- excessively deep or expansive JSON-compatible values fail with ordinary + validation errors. + +These are compatibility corrections and safety boundaries rather than new +consumer configuration requirements. Consumers relying on an invalid or +ambiguous input should correct that input before upgrading. + +## Upgrade + +Update the module dependency with: + +```sh +go get gitea.maximumdirect.net/eric/promptkit@v0.6.0 +go mod tidy +``` + +Run the consuming project's ordinary and race-enabled tests after upgrading. +Applications with custom prompt/profile sources, local provider endpoints, +unusual artifact paths, or assertions over provider error identities should +pay particular attention to the compatibility notes below. + +## Source Loading And Identity + +Prompt definitions now share one source-neutral selection and normalization +flow across operating-system and `fs.FS` sources. YAML `id` and `version` +metadata are authoritative; filenames do not create a second identity system. +Only selected content bodies are loaded, malformed unrelated definitions do +not shadow valid exact matches, and per-file read failures are reported as +prompt-load failures rather than false absence. + +File-backed profiles likewise use normalized YAML IDs, reuse their metadata +read for selected strict decoding, and avoid fully decoding unrelated files. +Selected malformed definitions remain authoritative and do not silently fall +through to a lower-precedence source. + +Prompt `content_file` paths are opened exactly as declared after a separate +blank check. They must remain relative to and contained by the configured +prompt source root, including across operating-system symlinks. + +See the [framework source and identity reference](../formats.md) and +[internal source overview](../internal/sources.md) for the current contracts. + +## Validation, Cancellation, And Efficiency + +JSON Schema documents preserve exact JSON-number representations. Schema +resource URLs safely escape legal filesystem names, and each operation loads +and compiles its schema graph once. `Run` and prepared execution reuse that +operation-local plan; Promptkit does not introduce a cross-operation cache. + +Artifact reading, rendering, schema loading, compilation, and validation now +check cancellation at the synchronous boundaries Promptkit controls. Rendering +memoizes each artifact's text within one render operation, while plain JSON +validation avoids materializing an unnecessary generic tree. + +The shared JSON-compatible-value owner now limits nesting and produced work so +unsafe consumer-controlled structures return errors instead of risking +unbounded recursion or allocation. See the +[architecture policy](../policy/architecture.md) for invariant ownership and +the [format reference](../formats.md) for validation behavior. + +## Provider Transport Hardening + +OpenAI-compatible endpoints are parsed and composed structurally, including +nested base paths. Underlying transport cancellation and deadline errors remain +discoverable with `errors.Is` through Promptkit's generation error category. + +Successful provider bodies are read with a fixed 16 MiB bound and must contain +exactly one JSON response object followed only by whitespace. Oversized, +truncated, malformed, or multiply framed responses fail without returning a +partial result. See the +[OpenAI-compatible integration contract](../integrations/openai-compatible-chat.md) +for the canonical request, endpoint, error, and response behavior. + +## Public API Changes + +None. + +## Consumer Action + +- Correct any configuration or request that depends on the formerly permissive + cases described under Compatibility. +- Confirm custom local provider endpoints are absolute HTTP or HTTPS base URLs + without credentials, queries, or fragments. +- Confirm prompt content paths remain within their configured source root and + file artifacts resolve to regular files. +- Run ordinary and race-enabled consumer tests after updating the dependency. diff --git a/docs/roadmap/audit-sequence.md b/docs/roadmap/audit-sequence.md deleted file mode 100644 index 08cb227..0000000 --- a/docs/roadmap/audit-sequence.md +++ /dev/null @@ -1,635 +0,0 @@ -# Codebase Audit Sequence - -## Purpose - -This document defines the staged sequence for auditing Promptkit before further -feature development. The audit is intended to identify high-confidence -opportunities to improve correctness, efficiency, duplication, implementation -clarity, and test-suite quality without changing production behavior during the -review itself. - -The audit findings belong in `audit.md`. A later, separate planning pass will -translate accepted findings into a staged remediation plan in -`implementation.md`. Neither this sequence nor the findings log owns current -behavior; the canonical sources identified by the -[documentation policy](../policy/documentation.md) remain authoritative. - -Each stage below is deliberately scoped for one LLM coding-agent prompt. Run -the stages in order and do not combine them. A stage may discover a concern -outside its scope, but it should record that concern for the owning later stage -rather than expanding its own review. - -## Governing Policies And Boundaries - -Every stage must follow: - -- the [development guide](../development.md), including its task-specific - reading guide; -- the [architecture policy](../policy/architecture.md), especially the public - facade, internal-package, dependency-direction, and consumer boundaries; -- the [testing policy](../policy/testing.md), including its risk-based, - behavior-oriented standard; and -- the [documentation policy](../policy/documentation.md), including canonical - ownership and the temporary nature of roadmap documents. - -This is an audit, not an implementation pass: - -- Do not change production code, tests, examples, fixtures, public contracts, - or current-state documentation. -- Limit repository edits to the audit artifacts explicitly authorized for the - stage. -- Do not silently repair an issue while investigating it. -- Do not treat coverage, complexity, similarity, lint, or graph output as a - finding without confirming the underlying behavior in source and tests. -- Do not recommend centralization merely because code looks similar. The code - must implement the same semantic rule, and consolidation must improve - ownership or reduce a credible drift risk. -- Do not recommend performance work without identifying a relevant execution - path and establishing a defensible cost model, measurement, or complexity - problem. -- Preserve unrelated working-tree changes. Record the audit baseline rather - than requiring an otherwise unrelated dirty tree to be cleaned. - -## Finding Standard - -Record each actionable finding in `audit.md` with: - -- a stable ID in the form `SNN-FNN`, where the first number is the stage; -- category: correctness, efficiency, duplication, clarity, testing, or - contract-documentation consistency; -- severity: critical, high, medium, or low; -- confidence: confirmed, high, medium, or low; -- affected packages, files, symbols, and tests; -- the contract, invariant, policy, or maintenance concern at issue; -- concrete evidence and a concise explanation of the failure mode or cost; -- the recommended direction, without implementation-level sequencing; -- the verification or regression protection that remediation would require; - and -- status: accepted, deferred, rejected, superseded, or resolved. - -Use **confirmed** confidence when the problem is reproduced or follows -unavoidably from a complete trace. Use **high** confidence when direct source -and test evidence establishes the problem but a safe reproduction is not -practical. Medium- and low-confidence concerns belong in a separate -observations section until a later stage confirms or rejects them; they must -not enter the remediation plan as if they were findings. - -Severity describes impact, not implementation effort: - -- **Critical:** credible data disclosure, data corruption, deadlock, unbounded - resource consumption, or a broadly unusable public contract. -- **High:** violation of an important public contract or invariant, a likely - concurrency or resource-lifecycle defect, or a failure with substantial - downstream impact. -- **Medium:** a real but narrower behavioral defect, meaningful avoidable cost, - duplicated policy with credible drift risk, or a material testing gap. -- **Low:** a bounded clarity, maintainability, or testing-friction problem with - a concrete improvement and little behavioral risk. - -When a reviewed area yields no finding, record the important behavior or risk -that was inspected and found adequately implemented or tested. This coverage -ledger prevents later reviewers from mistaking silence for omission. - -## Per-Stage Procedure - -Unless a stage says otherwise, its single agent prompt should: - -1. Read the required policies, focused internal documentation, production - files, and tests for that stage. -2. Use the code knowledge graph for symbol discovery, callers, callees, and - cross-package traces; confirm important conclusions against source. -3. Trace normal, boundary, and failure paths through the narrowest relevant - public or package contract. -4. Review correctness, meaningful runtime cost, semantic duplication, - responsibility clarity, and the value and ownership of tests in scope. -5. Run the narrowest existing tests needed to validate conclusions. Use - race-enabled or repeated focused tests when concurrency or nondeterminism is - in scope. Do not add permanent tests during the audit. -6. Add the stage result to `audit.md`: accepted findings, unresolved - observations, areas verified, commands run, and any handoff to a later - stage. -7. Recheck the working tree and confirm that only the authorized audit artifact - changed. - -## Stage 0: Initialize The Audit And Establish The Baseline - -Create `audit.md` and establish a reproducible starting point before reviewing -individual components. - -Record: - -- the audited commit, branch, Go version, module identity, and working-tree - state; -- unrelated pre-existing changes that all later stages must preserve; -- the implemented package and public-facade inventory; -- the baseline validation results; and -- the finding template, status vocabulary, and coverage ledger used by later - stages. - -Refresh the code knowledge graph for the recorded commit. Run the repository's -ordinary tests, race tests, vet, build, maintained offline preparation example, -Go formatting check, Markdown link check, and repository-hygiene checks. Run -package coverage once as a diagnostic and record the result without defining a -coverage target or committing generated output. Measure coarse package test -duration only if it can be done without adding tooling or changing tests. - -Compare the validation requirements stated by the testing policy, development -guide, and release procedure. Record a finding if their ownership or command -sets are materially inconsistent; do not edit those documents in this stage. - -**Exit condition:** `audit.md` contains the baseline, ledger structure, and -validation result, and no component-level audit has begun. - -## Stage 1: Public Values, Conversion, Errors, And Formatting - -Review the root facade's public request, result, inspection, prepared-run, and -error values together with public-to-internal and internal-to-public -conversion. Scope the review to `doc.go`, `types.go`, `convert.go`, `errors.go`, -`capacity_error.go`, `formatting.go`, and `prepared_execution.go`, plus the -directly relevant portions of root tests. - -Focus on: - -- zero-value and nil behavior; -- defensive copying, aliasing, and immutable snapshots; -- lossless conversion and field precedence; -- error identity through `errors.Is` and `errors.As`; -- containment of internal representations; -- safe `String`, `GoString`, and diagnostic formatting; -- accidental disclosure of credentials, prompt content, generated content, or - other private state; and -- conversion or copying logic that represents the same rule in multiple - places. - -Review only tests that own these value and boundary contracts. Defer engine -assembly, execution coordination, and transport behavior to their later -stages. - -**Exit condition:** all root value-conversion and error-formatting paths have a -recorded audit result without evaluating engine orchestration. - -## Stage 2: Public Configuration And Extension Adapters - -Review the smaller public construction and extension surfaces in -`backends.go`, `profiles.go`, `artifact_reader.go`, `json.go`, and -`llm_adapter.go`, together with their directly relevant root and internal -adapter tests. - -Focus on: - -- validation performed at the public boundary; -- ownership and copying of caller-supplied maps, slices, filesystems, readers, - and clients; -- adapter error propagation and cancellation; -- consistency between convenience constructors and general configuration; -- whether extension interfaces are as narrow as their consumers require; -- whether public helpers duplicate internal policy or merely translate it; - and -- whether tests protect consumer-visible behavior rather than private adapter - choreography. - -Do not review how `NewEngine` combines these values; that belongs to Stage 3. - -**Exit condition:** every non-engine public configuration helper and adapter -has a recorded result and any assembly questions are handed to Stage 3. - -## Stage 3: Engine Construction, Options, And Source Assembly - -Review the construction and configuration portions of `engine.go` and the -corresponding tests in `engine_test.go`. Limit the scope to `NewEngine`, option -application, dependency defaults, backend registration, profile and prompt -source composition, fallback-profile placement, validator and client -selection, capacity-manager construction, and construction-time validation. - -Focus on: - -- deterministic option precedence; -- required versus optional dependencies; -- isolation between engine instances; -- freezing or copying consumer configuration at the correct boundary; -- correct dependency direction and absence of process-global mutable state; -- failure atomicity and useful public errors; -- consistency between configured backends and capacity policies; and -- assembly logic that is repeated or split across unclear owners. - -Do not audit the runtime behavior of `Run`, `Prepare`, or inspection methods; -that belongs to Stage 4 and the internal use-case stages. - -**Exit condition:** engine construction and source assembly are fully accounted -for, including tests, without expanding into runtime orchestration. - -## Stage 4: Engine Operations And Root Contract Coverage - -Review the remaining public methods in `engine.go` and their directly relevant -root tests, including the external-package contracts in -`public_contract_test.go` and `prepared_execution_contract_test.go` only where -they exercise the engine boundary under review. - -Focus on: - -- request translation and context propagation; -- ordinary run, preparation, inspection, and prepared-execution entry points; -- public error mapping and preservation of injected dependency errors; -- result and prepared-state ownership; -- consistency between method and package-level convenience functions; -- public behavior that is asserted redundantly in root internal tests and - external-package contract tests; and -- important public behavior that is tested only through internal packages. - -Treat internal runner, transport, validation, and capacity mechanics as black -boxes in this stage. Hand questions about their implementation to their owning -later stages. - -**Exit condition:** the public execution boundary and its contract-test -ownership are recorded without duplicating internal component audits. - -## Stage 5: Internal Domain And JSON-Compatible Values - -Review `internal/domain` and `internal/jsonvalue`, including all of their tests. - -Focus on: - -- domain invariants and invalid states; -- session normalization; -- prepared-run and schema immutability; -- deep-copy correctness for every supported JSON-compatible shape; -- numeric-type preservation and rejection policy; -- cycles, excessive nesting, unsupported values, and nil distinctions; -- avoidable repeated copying on execution paths; and -- whether generic value machinery has a single clear owner. - -Trace important callers to confirm that these packages enforce the invariants -their consumers assume, but do not audit the callers' broader behavior. - -**Exit condition:** shared value semantics and their test ownership are fully -recorded. - -## Stage 6: Backend Registry, Defaults, And Built-In Profiles - -Review `internal/backend`, `internal/defaults`, and -`internal/profile/builtin`, including their focused tests and the relevant -backend-policy traces into engine assembly and the LLM reserved-field rule. - -Focus on: - -- immutable registry construction and lookup; -- built-in versus consumer ID collision rules; -- endpoint, credential-environment, header, parameter, and concurrency - validation; -- defensive copies at registry boundaries; -- application-neutral default ownership; -- built-in profile/backend consistency; -- reserved request-field ownership without dependency inversion; and -- duplicated validation or default policy across public and internal layers. - -Defer scheduling mechanics to Stage 15 and actual HTTP request construction to -Stage 14. - -**Exit condition:** registry and default-policy correctness are recorded, with -transport and scheduling questions handed to their owning stages. - -## Stage 7: File Discovery And Prompt Definitions - -Review `internal/filecatalog` and `internal/promptdef`, including their tests -and fixtures. Read the framework format reference and internal source document -before evaluating behavior. - -Focus on: - -- deterministic discovery and duplicate handling; -- filesystem and `fs.FS` parity; -- root and relative-path normalization; -- strict YAML decoding and version selection; -- prompt ID, message, input, cache-control, and validation declarations; -- inline versus file-backed content rules; -- containment of referenced files where promised; -- malformed input and contextual error behavior; -- unnecessary repeated directory scans or file reads; and -- fixture and case duplication that does not protect distinct parser risks. - -Do not audit rendering, artifact loading, profile loading, or schema validation -in this stage. - -**Exit condition:** discovery and prompt-definition parsing have complete -findings and coverage-ledger entries. - -## Stage 8: Profile Sources And Repository Composition - -Review `internal/profile` excluding its built-in subpackage, including all -repository tests and profile fixtures. Read the profile format contract first. - -Focus on: - -- strict decoding and profile validation; -- filesystem and `fs.FS` parity; -- repository overlay and fallback precedence; -- distinction between absence and a malformed authoritative source; -- preservation of useful error identity and context; -- conversion to immutable execution profiles; -- duplicate IDs and deterministic selection; -- repeated parsing, validation, or copying; and -- whether tests at repository, engine, and public-contract layers have clear, - nonduplicative ownership. - -Defer resolution of a profile with runtime overrides and backend definitions to -Stage 11. - -**Exit condition:** profile-source and repository-composition behavior are -fully recorded. - -## Stage 9: Artifact Loading And Prompt Rendering - -Review `internal/artifact` and `internal/prompt`, including all focused tests. -Read the internal source document and format reference first. - -Focus on: - -- inline and file artifact ownership, metadata, hashing, and error behavior; -- copied versus shared byte storage; -- caller-selected path semantics and architecture-policy boundaries; -- template parsing and execution; -- artifact, variable, session, and cache-control rendering; -- missing, extra, nil, and malformed input behavior; -- deterministic output and safe diagnostics; -- unnecessary repeated reads, hashes, parses, or allocations on common paths; - and -- tests coupled to incidental template or struct implementation. - -Do not audit the runner's decision about when rendering occurs. - -**Exit condition:** input materialization and rendering are accounted for -through their package boundaries. - -## Stage 10: Output Validation And Frozen Validation Plans - -Review `internal/validate`, including all tests, schema fixtures used by the -root contract suite, and traces from preparation into frozen validation plans. -Read the format and internal source documents first. - -Focus on: - -- basic, JSON, and JSON Schema mode semantics; -- schema-path resolution and filesystem/`fs.FS` parity; -- schema compilation, transitive references, and source-lifetime independence; -- output normalization and preservation; -- malformed schema and malformed model-output errors; -- thread safety of reusable validators and prepared plans; -- expensive recompilation or copying on repeated execution; and -- whether parser, validator, runner, and public tests each own distinct risks. - -Do not audit repair decisions or provider request construction. - -**Exit condition:** validation behavior, plan lifetime, and focused test value -are fully recorded. - -## Stage 11: Inspection And Execution-Target Resolution - -Review `internal/usecase/profile_inspection.go`, -`internal/usecase/prompt_inspection.go`, and the preparation and target- -resolution portions of `internal/usecase/runner.go`, together with their -focused tests. Use graph traces to define the exact helper and call-path scope -before reviewing. - -Focus on: - -- prompt and profile selection; -- backend lookup and endpoint overrides; -- reasoning, session, and other runtime precedence; -- merge semantics for default, profile, backend, and per-run values; -- inspection fidelity versus actual execution; -- credential-name versus credential-value handling; -- prompt-definition and schema freezing during preparation; -- stable error identity and context; and -- duplicated resolution rules across inspection, preparation, and execution. - -Do not review model invocation, repair execution, or prepared-handle lifecycle; -those belong to Stages 12 and 13. - -**Exit condition:** all selection, merge, inspection, and preparation rules are -traced and recorded once. - -## Stage 12: Ordinary Execution, Validation, And Repair Coordination - -Review `internal/usecase/runner.go`, `internal/usecase/repairer.go`, and -`internal/usecase/capacity_error.go` only for the ordinary execution path after -preparation, together with the corresponding sections of `runner_test.go`. -Use the Stage 11 resolution result as an established input rather than -reauditing it. - -Focus on: - -- rendering, generation, validation, and optional repair transitions; -- context cancellation and dependency-error propagation; -- partial result and usage accounting; -- exact attempt count and repair eligibility; -- avoidance of unintended retries; -- capacity-error translation; -- cleanup and failure behavior on every exit path; -- repeated orchestration or request construction; and -- oversized tests, helpers, or case matrices that obscure distinct behavior. - -Treat LLM transport and capacity scheduling as injected package contracts; -their mechanics belong to Stages 14 and 15. - -**Exit condition:** the ordinary execution state machine and its test ownership -are fully recorded. - -## Stage 13: Prepared Execution Lifecycle - -Review `internal/usecase/prepared_execution.go`, its focused tests, and the -prepared-execution portions of the root facade and external contract tests. -Do not repeat the public value review from Stages 1 and 4 or the resolution -review from Stage 11. - -Focus on: - -- single-attempt or other lifecycle guarantees; -- concurrent use and synchronization; -- discard behavior and resource release; -- frozen source, target, credential, capacity, timing, and schema semantics; -- independence of returned details and results; -- context and error behavior; -- consistency between ordinary and prepared execution where promised; -- private-state containment in formatting; and -- redundant assertions across internal, root, and external-package tests. - -Run focused race tests and repeated tests for lifecycle behavior where useful. - -**Exit condition:** prepared execution has one complete lifecycle analysis and -a clear map of which test layer owns each guarantee. - -## Stage 14: OpenAI-Compatible Transport - -Review `internal/llm`, including all transport tests. Read the -OpenAI-compatible integration contract and internal LLM document first. - -Focus on: - -- request endpoint, headers, authentication, and JSON body construction; -- omission versus explicit zero-value behavior; -- reserved-field enforcement and extra-parameter collision handling; -- session ID and reasoning encoding; -- structured-output and cache-control translation; -- client and per-generation deadlines; -- cancellation, body closure, bounded response reads, and decode failures; -- non-success HTTP response behavior; -- response choices, usage, and malformed-success handling; -- wire-visible compatibility and safe error disclosure; -- unnecessary marshaling, copying, or buffering; and -- whether the large transport test file can be simplified without losing - protocol-risk coverage. - -Use `httptest`-based existing tests; do not contact a live provider. - -**Exit condition:** every outbound and inbound wire path has a recorded result, -including focused test ownership. - -## Stage 15: Capacity, Admission, And Concurrency - -Review `internal/capacity`, its tests, `capacity_contract_test.go`, and the -integration points already identified in engine and use-case stages. Read the -internal capacity document first. - -Focus on: - -- bounded run admission and queue-capacity enforcement; -- per-backend limited and unlimited scheduling; -- FIFO behavior and cancellation-safe waiter removal; -- permit release on success, error, panic-relevant boundaries, and - cancellation; -- goroutine, timer, and waiter lifecycle; -- starvation, deadlock, race, and engine-isolation risks; -- lock scope and meaningful contention or allocation costs; -- preservation of injected-client concurrency where promised; -- relational testing of configured limits rather than duplicated defaults; - and -- duplication between internal concurrency tests and public contract tests. - -Run focused ordinary, race-enabled, and repeated tests. Repetition must remain -bounded and diagnostic; a test that passes many times is not proof of -correctness without a source-level synchronization analysis. - -**Exit condition:** concurrency invariants have both a source trace and a -test-ownership assessment. - -## Stage 16: Repository-Wide Test Strategy And Maintained Examples - -Perform a suite-level review after every component has been audited. Review -the testing policy, test inventory, fixtures, external-package root tests, -`architecture_test.go`, and both maintained examples. Use the component-stage -coverage ledger instead of repeating every individual test assertion. - -Construct a risk-to-owner matrix for: - -- public compatibility and error identity; -- parsing, validation, and serialization; -- immutability and data integrity; -- external wire behavior; -- cancellation, failure propagation, and recovery; -- concurrency and resource lifecycle; and -- representative assembled consumer workflows. - -Identify only evidence-backed cases of: - -- consequential behavior with no credible test owner; -- the same semantic rule asserted redundantly at several layers; -- tests coupled to private helpers, internal constants, exact noncontractual - wording, or collaborator choreography; -- low-value or obsolete cases whose lifetime cost exceeds their protection; -- missing failure, cancellation, race, or boundary coverage; -- nondeterminism, shared state, environment dependence, fixed ports, or test - ordering assumptions; -- helpers and fixtures whose complexity is not justified; and -- maintained examples that duplicate one another without protecting distinct - workflows. - -Use coverage and timing only to direct attention. Do not propose tests solely -to raise percentages or remove tests solely to shorten the suite. - -**Exit condition:** every important risk has a named test owner or an accepted -finding, and every proposed test deletion or consolidation states what -protection remains. - -## Stage 17: Cross-Cutting Duplication, Efficiency, And Architecture Review - -Review the codebase as a whole using the completed component findings, graph -traces, complexity signals, similarity signals, and package dependency map. -Do not reopen settled package behavior without new cross-cutting evidence. - -Focus on: - -- one semantic policy implemented by multiple packages; -- repeated public/internal transformations with credible drift risk; -- interfaces broader than their actual consumers; -- responsibilities split across packages or concentrated in the facade - contrary to the architecture policy; -- repeated parsing, copying, schema compilation, request construction, or - source traversal on important paths; -- avoidable lock contention or serial work supported by the concurrency audit; -- abstractions that add indirection without enforcing a boundary; and -- discrepancies between implemented package responsibilities and their - canonical architecture or internal documentation. - -For each possible consolidation, state why the code represents one rule, which -package should own it, and why the resulting dependency direction remains -valid. For each efficiency finding, state the path frequency, input scale, -complexity or measurement evidence, and the benchmark or invariant needed to -verify a remediation. - -**Exit condition:** all cross-cutting opportunities are either accepted with -high confidence, retained as explicitly lower-confidence observations, or -rejected with a short rationale. - -## Stage 18: Consolidate And Close The Audit - -Perform a findings-only synthesis. Do not change code and do not write the -remediation plan yet. - -- Recheck every accepted finding against the final audited tree. -- Merge duplicates and mark superseded IDs without erasing their history. -- Separate shared root causes from downstream symptoms. -- Confirm that every accepted item is confirmed or high confidence. -- Confirm that severity describes impact rather than effort. -- Reject speculative cleanup, coverage-driven test work, and centralization - without a clear owner or drift risk. -- Record dependencies and a recommended remediation order. -- Distinguish behavioral fixes, safe refactors, performance work, test gaps, - test consolidation, and documentation synchronization. -- Add an audit summary stating what was reviewed, what validation ran, the - accepted finding counts by category and severity, and any residual - uncertainty. -- Re-run baseline validation if audit-only investigation could have affected - repository state, and confirm that only authorized roadmap files differ from - the recorded baseline. - -The recommended ordering should place correctness, data-integrity, -resource-lifecycle, and concurrency defects first; policy duplication and -missing protection for consequential behavior next; then clarity, test -consolidation, and demonstrated efficiency improvements. Actual implementation -stages must be decided in the later `implementation.md` planning pass, where -files, dependencies, acceptance criteria, and validation can be made -decision-complete. - -**Exit condition:** `audit.md` is a complete, internally consistent input to a -separate remediation-planning prompt, with no code or test changes mixed into -the audit. - -## Completion Criteria - -The audit is complete only when: - -- every production component and public boundary appears in the coverage - ledger; -- every test file and maintained example has been reviewed at its owning stage - or in the suite-wide stage; -- important cross-package paths have been traced end to end; -- concurrency-sensitive behavior has received source and race-test review; -- every accepted finding meets the evidence and confidence standard; -- lower-confidence observations are visibly separated from remediation - candidates; -- proposed test additions, deletions, and consolidations are justified against - the testing policy; -- proposed simplifications identify a durable responsibility owner; -- proposed efficiency work has a relevant cost model or measurement plan; and -- the repository remains unchanged except for the authorized audit roadmap - artifacts. diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md deleted file mode 100644 index 1bd4ed1..0000000 --- a/docs/roadmap/audit.md +++ /dev/null @@ -1,4606 +0,0 @@ -# Codebase Audit - -## Sequence Prerequisite - -Stage 0 had not been executed when this artifact was created: the repository -contained no `audit.md` at the start of the Stage 1 review. Consequently, the -reproducible repository baseline, package inventory, validation matrix, graph -refresh, and initial coverage ledger required by Stage 0 remain outstanding. -This review did not backfill that out-of-scope work. - -The Stage 1 review began from commit -`ebf1602635e108e2a7ac1abd3a3ca24a620104ce` on branch `main`, with a clean -working tree and Go `go1.26.5 linux/amd64`. These details identify this review -only; they are not a substitute for the Stage 0 baseline. - -## Stage 1: Public Values, Conversion, Errors, And Formatting - -### Scope Reviewed - -The review covered `doc.go`, `types.go`, `convert.go`, `errors.go`, -`capacity_error.go`, `formatting.go`, and `prepared_execution.go`, plus the -directly relevant root tests. Internal domain declarations and callers were -consulted only to confirm field-complete conversion, ownership, and public -error mapping. Engine assembly, runtime orchestration, adapter implementation, -and JSON codec mechanics were not audited. - -### Accepted Findings - -#### S01-F01: Run-request formatting tests do not protect input and variable redaction - -- **Category:** testing -- **Severity:** medium -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `formatting.go` (`RunRequest.String`, - `RunRequest.GoString`, and `RunRequest.redactedString`) and `engine_test.go` - (`TestRunRequestFormattingRedactsDirectAPIKey`) -- **Contract at issue:** The formatter GoDoc promises that `String` and - `GoString` omit direct credentials and input and variable contents. The - documentation and testing policies treat prompt inputs and other private - content as sensitive and give consequential disclosure behavior a strong - presumption of durable test protection. -- **Evidence:** The implementation currently satisfies the contract by - formatting only request identifiers, collection lengths, presence flags, - and whether an API key is set. The focused test supplies an inline input but - asserts only that the API-key sentinel is absent and `APIKeySet:true` is - present; it supplies no variable values and never checks whether the input - URI, input body, or variable values appear. The test would remain green if a - later formatter change appended input or variable contents while continuing - to omit the API key. -- **Failure mode:** A logging or diagnostic-formatting refactor could disclose - prompt input or template-variable content through ordinary `%v`, `%+v`, or - `%#v` formatting without a contract-test failure. -- **Recommended direction:** Extend the existing formatting test, rather than - adding a parallel test, with distinct input URI, input body, and variable - sentinels and assert that each is absent from all three supported formatting - forms. Retain the positive structural assertions so the test continues to - distinguish a useful summary from an empty formatter. -- **Required verification:** Run the focused request-formatting test and the - root package tests. Confirm that a deliberate formatter mutation exposing - any sentinel makes the focused test fail. - -### Unresolved Observations - -None. Medium- or low-confidence concerns discovered during this review were -not promoted to findings. - -### Coverage Ledger - -- **Public value declarations and zero values:** Reviewed request, prepared, - result, artifact, inspection, target, output, validation, rendered-prompt, - structured-output, generation, and token-usage values. Nil maps, slices, - pointers, optional values, and zero-value public enum strings cross the - facade without panics or invented values. -- **Request conversion:** `toDomainRunRequest` and its helpers preserve every - public field, copy maps and pointer values, validate and deeply copy nested - JSON-compatible overrides, and retain direct credentials only in the - internal request field intended for execution. -- **Prepared and result conversion:** `fromDomainPreparedRun`, - `fromDomainRunResult`, and their helpers preserve all public fields while - copying artifact bytes, validation diagnostics, hashes, rendered messages, - cache-control pointers, effective extra parameters, and structured-output - schemas. Internal credential and target-presence fields do not escape. -- **Inspection and extension conversion:** Profile and prompt inspection - outputs are independent copies. Generation requests receive copied prompt, - target, presence, and structured-output values; generation responses contain - no mutable fields requiring additional copying. -- **Copy-rule ownership:** Caller-supplied JSON-compatible values enter through - `internal/jsonvalue` validation and copying. The outward conversion helpers - copy already-validated domain snapshots without introducing a second - acceptance policy. No consolidation finding was warranted in this scope. -- **Public errors:** Not-found identities remain distinct from load failures; - profile-required and missing-credential errors retain their more specific - identity together with `ErrInvalidRequest`; collaborator and cancellation - identities remain discoverable; and typed capacity errors expose only a - copied backend ID plus `ErrCapacityExceeded` rather than the internal error - type. Nil and zero `CapacityError` values are safe. -- **Diagnostic formatting:** `RunRequest` and `GenerateRequest` currently omit - direct credentials and content from `String`, `GoString`, `%+v`, and `%#v`. - `PreparedExecution` always formats as an opaque constant, including through - a copied handle. Prepared and result values intentionally expose rendered or - generated content as documented in package GoDoc; applications retain - responsibility for logging those content-bearing values. -- **Prepared handle values:** Nil and zero handles return zero details and may - be discarded safely. Details are fresh deep copies and remain stable after - execution or discard. Copying a handle shares its single-use lifecycle - without exposing the internal representation. -- **Test ownership:** Root external-package tests appropriately own public - snapshots, structured error identity, and opaque-handle behavior. Focused - internal error-mapping tests cover internal-type containment. The one - material redaction gap is recorded as S01-F01. - -### Verification Performed - -The code knowledge graph was used to discover the scoped symbols, trace their -callers and callees through the facade and internal domain boundary, and locate -the focused tests. Important conclusions were confirmed against source. - -The following focused commands passed: - -```sh -go test . -run 'Test(PreparedRunJSONDoesNotExposeSecretOrTargetPresence|RunRequestFormattingRedactsDirectAPIKey|GenerateRequestFormattingRedactsDirectAPIKey|MapPublicErrorPreservesGenerationCancellation|MapPublicErrorTranslatesCapacityError|CapacityExceededSentinelContract|InspectProfileReturnsIndependentTargetMatchingPreparation|InspectPromptReturnsIndependentMetadataMatchingPreparation|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|PreparedExecutionLifecycleAndEngineBinding|PreparedExecutionDiscardAndFormattingDoNotExposePrivateState|InMemoryProfileExtraParamsAreCopiedAcrossPublicBoundary|ExtraParamsTypedNestedValuesAreCopiedAcrossPublicBoundary|ArtifactReaderReceivesPublicReferenceAndPreparesArtifact|RunPassesPreparedRequestToInjectedLLMClient|PublicErrorsSupportErrorsIs)$' -go test -race . -run 'Test(PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|PreparedExecutionConcurrentClaimAllowsOneGeneration|PreparedExecutionRunAndDiscardRaceHasOneWinner|PreparedExecutionDiscardAndFormattingDoNotExposePrivateState|MapPublicErrorTranslatesCapacityError|RunRequestFormattingRedactsDirectAPIKey|GenerateRequestFormattingRedactsDirectAPIKey)$' -count=3 -``` - -### Handoff - -- Execute the missing Stage 0 baseline before relying on this file as a - complete audit ledger or beginning the next component review. -- Stage 2 owns public configuration helpers, `json.go`, extension-adapter - implementation, and adapter-specific mutation and cancellation behavior. -- Stages 3 and 4 own engine construction and runtime operations respectively; - this review did not evaluate those paths beyond tracing their use of the - scoped conversion and error boundary. - -## Stage 2: Public Configuration And Extension Adapters - -### Scope Reviewed - -The review covered `backends.go`, `profiles.go`, `artifact_reader.go`, -`json.go`, and `llm_adapter.go`, together with directly relevant root tests, -external-package contract tests, and `internal/profile` validation tests. -Internal backend, profile, artifact, and LLM declarations were consulted only -to compare boundary contracts and policy ownership. `NewEngine` option -assembly, source composition, runtime orchestration, internal profile-source -behavior, and transport mechanics were not audited. - -### Accepted Findings - -#### S02-F01: Out-of-range JSON durations silently overflow during decoding - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `json.go` (`RunResult.UnmarshalJSON` and - `runResultJSON.DurationMS`) and `public_contract_test.go` - (`TestRunResultJSONUsesMillisecondsAndRoundTrips`) -- **Contract at issue:** `RunResult` has a stable JSON representation in which - `duration_ms` is an integer millisecond count. Decoding must not silently - turn an accepted wire value into unrelated duration metadata. -- **Evidence:** The wire field accepts the full `int64` range, then decoding - multiplies that value by `time.Millisecond` without checking whether the - nanosecond-valued `time.Duration` can represent the result. A focused probe - decoded `{"duration_ms":9223372036854775807}` with a nil error and produced - `Duration == -1ms`. The existing round-trip test exercises only `1500ms` and - does not cover either representable boundaries or overflow. -- **Failure mode:** Malformed or untrusted persisted JSON can be accepted while - corrupting a very large positive duration into a negative or otherwise - wrapped value. Downstream timing displays, comparisons, or metrics then - consume false data without a decode error. -- **Recommended direction:** Validate the millisecond value against the range - that can be safely converted to `time.Duration` before multiplication and - return a contextual JSON decoding error for values outside that range. -- **Required verification:** Add boundary cases for the largest safely - representable positive and negative millisecond values and their first - out-of-range neighbors, plus the reproduced maximum-`int64` input. Retain - ordinary and zero-value round-trip coverage. - -#### S02-F02: In-memory and filesystem profiles duplicate semantic validation - -- **Category:** duplication -- **Severity:** medium -- **Confidence:** high -- **Status:** superseded by S17-F01; evidence retained -- **Affected code:** `profiles.go` (`validatePublicProfile`, - `toDomainProfile`, and the memory profile repository) and - `internal/profile/filesystem_repository.go` (`validateProfile` and - `loadProfile`), plus their focused tests -- **Contract at issue:** Profiles supplied in memory and profiles loaded from a - filesystem are two sources for the same execution-profile domain value. - Required identity, backend-or-endpoint selection, model presence, and - numeric bounds are one semantic acceptance policy and need one owner. -- **Evidence:** `validatePublicProfile` and `validateProfile` independently - implement the same seven conditions with the same error text: required ID, - backend or endpoint, required model, temperature in `[0,2]`, non-negative - maximum tokens, top-p in `[0,1]`, and non-negative timeout. The root tests do - not exercise the required-field or scalar-bound cases for in-memory - profiles, and the filesystem tests do not protect all scalar bounds. This is - policy duplication rather than mere translation or error wrapping. -- **Failure mode:** A future constraint or correction can be applied to one - profile source but not the other, making an otherwise identical profile - valid or invalid according to where it was stored. Sparse boundary tests - would not reliably expose the divergence. -- **Recommended direction:** Give the domain-level profile acceptance rule one - internal owner that both in-memory and filesystem repositories invoke, - while leaving source-specific normalization and public error translation at - their existing boundaries. -- **Required verification:** Protect the shared validator with a table covering - every required field and both sides of every numeric bound, then retain a - small integration check for each source and for the public - `ErrInvalidConfig` translation. - -#### S02-F03: The injected LLM client's mutation-ownership contract is untested - -- **Category:** testing -- **Severity:** medium -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `llm_adapter.go` (`publicLLMClientAdapter.Generate`), - `convert.go` (`fromDomainGenerateRequest` and its nested conversions), - `types.go` (`LLMClient`), and injected-client tests in `engine_test.go` -- **Contract at issue:** The public `LLMClient` contract explicitly states that - maps, slices, and pointers in `GenerateRequest` are client-owned copies that - may be mutated or retained. The adapter is the boundary responsible for - satisfying that ownership promise. -- **Evidence:** The adapter currently constructs independent messages, - cache-control pointers, target parameters, and structured-output schema - values before invoking the client. Existing fakes retain requests and tests - inspect field propagation, errors, and cancellation, but no test mutates the - nested request received by the client and proves that the source domain - request remains unchanged. A shallow-copy regression would therefore - preserve all current field-equality assertions. -- **Failure mode:** A conforming injected client could mutate or asynchronously - retain nested request data and thereby alter prepared engine state, affect a - later operation, or introduce a race despite following the documented - interface contract. -- **Recommended direction:** Add a focused adapter-boundary ownership test that - has a client mutate and retain each mutable nested shape, then verifies that - the domain request and its nested values remain unchanged. Keep engine-level - tests focused on observable request propagation and error identity. -- **Required verification:** Exercise prompt messages and cache control, target - extra parameters, and structured-output schema under the focused test; run - it with the race detector as well as normally. - -#### S02-F04: The profile convenience constructor's full mapping is unprotected - -- **Category:** testing -- **Severity:** medium -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `profiles.go` (`OpenAICompatibleProfile` and - `OpenAICompatibleProfileConfig`) and the three - `TestOpenAICompatibleProfile...` tests in `engine_test.go` -- **Contract at issue:** The exported convenience constructor promises a - `Profile` suitable for the general `WithProfiles` path. Its consumer-visible - behavior is the complete, field-for-field mapping of configuration values, - followed by the documented deferred validation and copying rules. -- **Evidence:** The implementation currently maps every configuration field. - The principal integration test asserts backend ID, model, direct API-key - behavior, and extra parameters, while the other tests cover deferred nested - parameter validation and ownership. No test protects endpoint, - temperature, maximum tokens, top-p, timeout, service tier, or reasoning - effort as constructor output. Dropping any of those assignments would leave - the current constructor-specific tests green. -- **Failure mode:** A maintenance edit can silently discard a supported model - setting from the convenience path while the equivalent general `Profile` - configuration continues to work, creating source-dependent behavior for - consumers. -- **Recommended direction:** Add one direct, table-like all-field mapping test - for the constructor and keep only the integration assertions that establish - its passage through ordinary profile validation and ownership boundaries. -- **Required verification:** Populate every scalar and string setting with a - distinct non-zero value, compare the complete returned `Profile`, and retain - the existing nested-extra-parameter and invalid-parameter integration cases. - -#### S02-F05: Stable JSON field mappings have multiple manual owners - -- **Category:** duplication -- **Severity:** medium -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `json.go` (`PreparedRun.MarshalJSON`, `runResultJSON`, - `RunResult.MarshalJSON`, and `RunResult.UnmarshalJSON`) and JSON tests in - `public_contract_test.go` -- **Contract at issue:** The stable public JSON shape should preserve every - public field except for intentional timing representation and omission - rules. The list of ordinary fields is one serialization policy, not a - separate rule for each encoding direction. -- **Evidence:** `PreparedRun.MarshalJSON` redeclares and assigns every field in - an anonymous wire struct. `RunResult` repeats its field list in the public - type, `runResultJSON`, the marshal literal, and the unmarshal literal. The - custom handling is needed only for timing fields, but ordinary fields are - manually synchronized around it. Existing JSON tests cover timing, - artifact content type, session ID, and backend omission but do not round-trip - fully populated values. Adding a public field to either value can therefore - omit it from stable JSON without a compile failure or focused test failure. -- **Failure mode:** Public Go values and their documented stable JSON form can - drift, or marshal and unmarshal can become asymmetric, as fields evolve. - Consumer data may be silently absent after persistence or interchange. -- **Recommended direction:** Structure the wire representation so ordinary - fields derive from a single alias or embedded representation and only the - timing exceptions require explicit mapping. Avoid changing existing JSON - names or omission behavior while consolidating ownership. -- **Required verification:** Add fully populated `PreparedRun` and `RunResult` - JSON contract cases that check required names and omissions and compare all - fields after round trip, alongside the timing-boundary regression from - S02-F01. - -### Unresolved Observations - -None. Questions belonging to engine assembly or internal component behavior -were handed to their owning stages rather than promoted from partial traces. - -### Coverage Ledger - -- **Backend helpers:** `LocalBackend` is a side-effect-free conventional-value - constructor. `WithBackend` copies the queue-capacity pointer when the option - is applied, and existing tests protect normalization, invalid and duplicate - definitions, nested extra-parameter freezing at construction, lookup copy - behavior, and engine isolation. Actual registry composition remains Stage 3 - scope and internal registry policy remains Stage 6 scope. -- **Profile helpers:** `OpenAICompatibleProfile` correctly performs a shallow - top-level extra-parameter copy and defers deep validation and freezing to the - general profile path as documented. The full-mapping test gap is S02-F04; - the duplicated acceptance policy is S02-F02. -- **Memory profile repository:** Repository construction rejects duplicate IDs - and invalid nested JSON-compatible values, stores domain copies, and returns - independent profile copies. Its source-neutral validation rule lacks a - single owner as recorded in S02-F02. -- **Artifact reader adapter:** The public and internal reader interfaces each - contain only `Read`. The adapter passes the caller context and error identity - through, rejects a nil successful artifact, translates references without - policy duplication, and copies returned body bytes. Focused and integrated - tests protect mutation isolation, nil handling, reference translation, - cancellation identity, and collaborator error identity. -- **LLM client adapter:** The public and internal client interfaces each - contain only `Generate`. The adapter forwards the exact context, preserves - client error identity for the use-case boundary, rejects a nil successful - response, and translates the scalar response without extra policy. The - request conversion currently deep-copies mutable data; its missing mutation - regression protection is S02-F03. -- **Adapter cancellation and errors:** Existing public tests establish caller - cancellation identity for generation and artifact loading and preserve - injected sentinel errors through public wrapping. No adapter adds an - independent deadline or cancellation mechanism. -- **Stable JSON:** Intentional timestamp, millisecond-duration, zero-value, - session, artifact, and backend-identity behavior is partly protected. The - confirmed overflow is S02-F01 and manual mapping drift is S02-F05. -- **Filesystem, reader, and client ownership:** Reader and client values are - stored as narrow injected interfaces and mutable values crossing their - adapter calls are copied as described above. Filesystem option validation, - lifetime, and composition reside in `engine.go` and are intentionally handed - to Stage 3 rather than inferred from this stage's helper review. - -### Verification Performed - -The code knowledge graph was used to locate each scoped helper and adapter, -trace its callers and callees, compare public and internal interface widths, -and confirm the duplicated profile rule. Source and focused tests were then -read to verify the graph conclusions. - -The following focused commands passed: - -```sh -go test . -run 'Test(PublicArtifactReaderAdapterCopiesBody|RunSucceedsWithInjectedLLMClient|RunPassesPreparedRequestToInjectedLLMClient|EngineRunPropagatesCallerCancellation|WithArtifactReaderRejectsNilReader|ArtifactReaderReceivesPublicReferenceAndPreparesArtifact|ArtifactReaderFailuresPreserveArtifactLoadErrors|RunAddsLLMGenerateToCollaboratorPublicError|PublicErrorsSupportErrorsIs|OpenAICompatibleProfileRunsThroughNormalProfilePath|OpenAICompatibleProfileDefersExtraParamsValidation|OpenAICompatibleProfileNestedExtraParamsRunThroughWithProfiles|WithProfilesRejectsDuplicateIDs|WithProfilesRejectsInvalidExtraParams|WithProfilesRejectsCyclicExtraParams|LocalBackendConstructsAndRegistersConventionalBackend|WithBackendCopiesQueueCapacity|BackendRegistrationRejectsInvalidAndDuplicateDefinitions|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|PreparedRunJSONOmitsZeroTimingValues|BackendIdentityJSONNamesAndOmission|PreparedRunJSONTimingRoundTrips|RunResultJSONUsesMillisecondsAndRoundTrips)$' -go test ./internal/profile -run 'Test(FilesystemRepository_GetProfile|FSRepository)$' -go test -race . -run 'Test(EngineRunPropagatesCallerCancellation|ArtifactReaderFailuresPreserveArtifactLoadErrors|PublicArtifactReaderAdapterCopiesBody|OpenAICompatibleProfileNestedExtraParamsRunThroughWithProfiles)$' -count=3 -``` - -A temporary program outside the repository decoded -`{"duration_ms":9223372036854775807}` into `RunResult`; `go run` reported -`error= duration=-1ms nanoseconds=-1000000`, confirming S02-F01. The -temporary source was removed and no probe output was added to the repository. - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - component review. -- Stage 3 owns `NewEngine` option application and the actual composition, - validation, and lifetime of configured filesystems, readers, clients, - profiles, and backends. -- Stage 6 owns internal backend registry and built-in profile policy. Stage 8 - owns the broader filesystem profile repository review; it should use - S02-F02 as established evidence rather than repeating the public-side audit. -- Stage 14 owns transport-specific request construction, deadlines, response - decoding, and resource handling. This stage assessed only the public - injection adapter. - -## Stage 3: Engine Construction, Options, And Source Assembly - -### Scope Reviewed - -The review covered the construction and option portions of `engine.go`, the -construction effect of `WithBackend` in `backends.go`, and directly relevant -tests in `engine_test.go`, `public_contract_test.go`, and -`capacity_contract_test.go`. Narrow traces into backend registry snapshots, -capacity-manager construction, built-in client construction, repositories, -validators, and `usecase.NewRunner` were used only to confirm the values and -dependencies assembled by `NewEngine`. Runtime engine methods, repository -parsing mechanics, validation mechanics, transport behavior, and capacity -scheduling were not audited. - -### Accepted Findings - -#### S03-F01: Single-file source options alter valid caller paths - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `engine.go` (`fileSource`, `WithPromptFile`, - `WithProfileFile`, and `WithSchemaFile`) and - `TestSourceOptionsRejectInvalidInputs` plus the three single-file success - tests in `engine_test.go` -- **Contract at issue:** Each single-file option accepts a path naming an - existing non-directory file. Filesystem paths are exact caller values; - leading and trailing whitespace are legal filename characters and the - option GoDoc does not define normalization. -- **Evidence:** `fileSource` assigns `strings.TrimSpace(name)` to `cleanName` - and performs every path operation and `os.Stat` against that altered value. - A focused probe created an existing file named `prompt.yaml `, confirmed - that `os.Stat` on the supplied path succeeded, and passed the same value to - `WithPromptFile`. `NewEngine` returned `ErrInvalidConfig` because it instead - attempted to stat `prompt.yaml` without the trailing space. All three public - file options share this helper. Existing tests cover ordinary paths, blank - paths, one missing path, and one directory path, but no exact-path boundary. -- **Failure mode:** A consumer cannot configure an otherwise valid prompt, - profile, or schema file whose name begins or ends with whitespace. The error - also reports the altered path, obscuring why the supplied existing file was - rejected. -- **Recommended direction:** Use trimming only to enforce the chosen blank- - input rule, then perform path decomposition, validation, error reporting, - and filesystem access with the original caller-supplied path. -- **Required verification:** Add a compact shared regression that constructs - engines through all three single-file options using existing paths with a - leading or trailing whitespace character. Retain the ordinary missing-file - and directory rejection cases. - -#### S03-F02: Construction precedence tests do not isolate documented ordering rules - -- **Category:** testing -- **Severity:** medium -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `engine.go` (`Option`, `NewEngine`, and - `newProfileRepository`) and `TestSourceOptionsRejectInvalidInputs`, - `TestRepeatedOptionsUseLastValueInEachCategory`, - `TestInMemoryProfilesOverrideBuiltInsAndProfileSources`, and - `TestFallbackProfileSourcePrecedence` -- **Contract at issue:** Option order selects the last valid value within a - category, but profile lookup has a fixed cross-category order independent of - argument order: in-memory, ordinary configured, application fallback, then - built-in. A file or FS ordinary-profile option replaces `Config.ProfileDir`, - and any invalid option must fail construction even if a later option would - replace it. -- **Evidence:** The implementation correctly stores categories separately and - assembles the fixed profile overlay after applying options. The main - precedence tests, however, pass fallback, ordinary, and in-memory options in - the same low-to-high order that a generic order-based overlay would use, so - they would remain green if argument order accidentally began controlling - cross-category precedence. No directly relevant test gives - `Config.ProfileDir` and an ordinary-profile option colliding IDs to protect - the documented replacement, and invalid-option tests do not place a valid - replacement after the invalid value. Same-category last-value behavior is - well covered but does not protect these distinct rules. -- **Failure mode:** An assembly refactor could make mixed profile-source order - depend on option order, allow `Config.ProfileDir` to compete with its - replacement option, or silently discard an earlier invalid option. The - current tests could still pass while consumers observe different selected - profiles or construction success. -- **Recommended direction:** Extend the existing precedence coverage with a - small set of discriminating cases rather than a combinatorial matrix: reverse - the cross-category option order, collide `Config.ProfileDir` with its option - replacement, and place a valid same-category option after an invalid one. -- **Required verification:** Assert the selected model for the two profile- - source cases and `errors.Is(err, ErrInvalidConfig)` for the invalid-then- - valid case. Keep the test at the public construction boundary and avoid - assertions about private repository nesting. - -### Unresolved Observations - -None. Lower-confidence concerns about typed-nil interface values and unusual -non-regular files were not promoted because the documented Go interface and -file contracts do not establish stronger behavior. - -### Coverage Ledger - -- **Option application:** `NewEngine` applies non-nil options once in argument - order and stops on the first error. Nil options compose safely in an option - slice. Same-category prompt, ordinary profile, fallback profile, in-memory - profile, schema, client, and reader options use last-valid-value semantics; - backend registrations alone accumulate. The unprotected ordering edges are - recorded as S03-F02. -- **Required and default dependencies:** A nonblank configured prompt - directory or prompt-source option is required. Profiles always end with the - embedded built-in repository; schema validation defaults to the documented - directory; the artifact reader, renderer, and model client receive - application-neutral defaults when not injected. Construction performs no - provider request and requires no credential. -- **Prompt and schema source selection:** Prompt and schema FS or file options - replace their corresponding `Config` directory, retain the injected `fs.FS` - for lazy access, and validate nil filesystems and blank roots. Single-file - exact-path handling is defective as recorded in S03-F01. Source contents - remain lazy and their parsing and containment belong to Stages 7 and 10. -- **Profile composition:** `newProfileRepository` builds one explicit overlay - in the documented order: built-in, application fallback, one ordinary - configured source, then in-memory profiles. Only one ordinary source is - installed, and an ordinary option suppresses `Config.ProfileDir`. Matching - malformed higher-precedence definitions stop lookup rather than becoming - failover. The implementation is clear; S03-F02 concerns discriminating test - coverage, not current behavior. -- **Backend and capacity assembly:** All consumer backend additions enter one - immutable registry with the built-in backend. `NewEngine` takes one capacity- - policy snapshot, constructs a fresh manager, and wraps either the injected - or built-in client with that same manager before passing both to the runner. - Invalid definitions and capacity policies fail as `ErrInvalidConfig`, and - tests protect additive registrations, deep-copy isolation, limited and - unlimited behavior, and independence between engines. Registry rules and - scheduler mechanics remain Stages 6 and 15 scope. -- **Caller-owned values and collaborators:** Queue-capacity pointers are copied - when `WithBackend` is created; backend maps and in-memory profile values are - deeply frozen during construction. Injected filesystems, readers, clients, - and HTTP transports remain explicit collaborator references. The built-in - LLM constructor clones the supplied `http.Client` and focused internal tests - protect non-mutation for positive, zero, and negative timeouts. -- **Client and validator selection:** `WithLLMClient` prevents construction of - the built-in client while retaining engine-local capacity wrapping. - Otherwise `Config.Timeout` and a cloned `Config.HTTPClient` configure the - built-in client. Schema options construct the matching validator, while an - empty `SchemaDir` uses the application-neutral default. Transport and - validation semantics remain Stages 14 and 10 scope. -- **Failure atomicity and global state:** Every error path returns before an - `Engine` is published. Construction state is local, registry and capacity - values are rebuilt for each engine, and there is no process-global mutable - configuration. File-backed prompt, profile, and schema contents are read - lazily; malformed or missing source content is classified only when an - operation selects it. -- **Assembly clarity and cost:** Construction is a single option pass followed - by one repository, registry, manager, client, validator, reader, renderer, - and runner assembly. No relevant repeated I/O, parsing, or copying cost was - found, and the category flags make replacement and default selection - explicit without duplicating internal component policy. -- **Test ownership:** Root external-package tests appropriately protect public - option validity, source selection, profile precedence, copy isolation, - default-client configuration, and engine-local backend and capacity - behavior. Focused internal tests own registry normalization, manager policy, - and HTTP-client cloning. S03-F02 identifies the material missing distinctions - rather than recommending duplicate internal choreography tests. - -### Verification Performed - -The code knowledge graph was used to find `NewEngine`, every option category, -source-assembly helpers, and directly relevant tests; trace construction into -the registry, capacity manager, repositories, validators, model client, and -runner; and confirm that later runtime mechanics were outside the reviewed -path. Important ownership and error conclusions were confirmed against source. - -The following focused commands passed: - -```sh -go test . -run 'Test(NewEngineRejectsMissingPromptDir|NewEngineAcceptsMissingProfileDir|SourceOptionsRejectInvalidInputs|PackageOptionsComposeFromSlice|RepeatedOptionsUseLastValueInEachCategory|FallbackProfileSourcePrecedence|FallbackProfileSourcePreservesLazyLoadingAndErrors|BackendOptionsAccumulateAndRegistrationsAreEngineLocal|BackendRegistrationRejectsInvalidAndDuplicateDefinitions|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|BackendCapacityIsIndependentBetweenEngines|WithLLMClientRejectsNilClient|WithArtifactReaderRejectsNilReader|PromptRepositoryReadFailureMapsToPromptLoad|SelectedProfileRepositoryReadFailureMapsToProfileLoad|PrepareWorksWithPromptFSAndRelativeContentFile|PrepareWorksWithPromptFile|PrepareWorksWithProfileFSOverBuiltIns|PrepareWorksWithProfileFileOverBuiltIns|RunStructuredOutputWorksWithSchemaFS|RunStructuredOutputWorksWithSchemaFile|EngineRunLayersTransportAndGenerationTimeouts)$' -go test ./internal/backend -run 'Test(RegistryIncludesExactOpenRouterDefinition|RegistryNormalizesUniqueAdditionsAndIsolatesMutations|NewRegistryNormalizesCapacityPolicy)$' -go test ./internal/capacity -run 'Test(NewManagerRejectsInvalidPolicies|ManagerAdmissionIsBoundedAndReleaseIsIdempotent|ManagerAdmissionHonorsContextAndUnlimitedBackends)$' -go test ./internal/llm -run 'TestNewOpenAICompatibleClientDoesNotMutateSupplied(Nonzero|Zero)TimeoutClient|TestNewOpenAICompatibleClientTreatsSuppliedNegativeTimeoutAsUnset' -go test -race . -run 'Test(BackendCapacityIsIndependentBetweenEngines|BackendOptionsAccumulateAndRegistrationsAreEngineLocal|EngineSupportsConcurrentPrepareAndRun|RepeatedOptionsUseLastValueInEachCategory)$' -count=3 -``` - -A temporary program outside the repository created an existing -`prompt.yaml ` file and called `NewEngine` with `WithPromptFile` using that -exact path. Direct `os.Stat` returned nil, while construction returned -`ErrInvalidConfig` after reporting the trimmed `prompt.yaml` path, confirming -S03-F01. The temporary source and generated file were removed. - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - component review. -- Stage 4 owns operation entry points and public runtime error/result behavior; - construction tests were read only through the behavior needed to observe - assembled dependencies. -- Stages 6, 7, 8, 10, 14, and 15 own backend policy, prompt sources, profile - repositories, validators, provider transport, and capacity scheduling - respectively. This stage established only that `NewEngine` selects and wires - their boundaries consistently. - -## Stage 4: Engine Operations And Root Contract Coverage - -### Scope Reviewed - -The review covered the public operation portion of `engine.go` -(`InspectPrompt`, `InspectProfile`, `Prepare`, `PrepareExecution`, `Run`, and -`RunPrepared`) and the directly relevant root tests in `engine_test.go`, -`public_contract_test.go`, `prepared_execution_contract_test.go`, and -`errors_internal_test.go`. `prepared_execution.go` and conversion helpers were -consulted only to confirm the operation boundary established in Stage 1. -Internal runner tests were consulted only to identify test ownership; runner, -transport, validation, capacity, and repository mechanics were treated as -black boxes. - -### Accepted Findings - -#### S04-F01: Ordinary-run cancellation identity is protected only below the public boundary - -- **Category:** testing -- **Severity:** medium -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `engine.go` (`Engine.Run`), `errors.go` - (`mapPublicError`), `engine_test.go` - (`TestEngineRunPropagatesCallerCancellation`), - `errors_internal_test.go` - (`TestMapPublicErrorPreservesGenerationCancellation`), and - `internal/usecase/runner_test.go` - (`TestRunnerRunCancellationPreservesGenerationCategory`) -- **Contract at issue:** `Engine.Run` passes the caller's context through the - execution boundary and preserves the active collaborator's cancellation - identity while adding the public operation category. Cancellation and - injected dependency failures are consequential public behaviors that should - be asserted through the consumer-visible boundary. -- **Evidence:** `Engine.Run` currently passes `ctx` unchanged to the runner and - maps its returned error without dropping wrapped identities. The external- - package cancellation test drives a real request context through the built-in - HTTP transport but asserts only `errors.Is(err, ErrLLMGenerate)` after - cancellation. The `context.Canceled` identity is asserted separately only - against the unexported `mapPublicError` helper and internal runner. A search - of the root operation tests found no other ordinary-run assertion for that - identity. Prepared execution does assert both identities at the public - boundary, but it exercises a different entry point. -- **Failure mode:** A facade or ordinary-run composition change could replace - the caller context, stop wrapping the collaborator cancellation, or discard - it during public error mapping. The internal tests and existing public test - could all remain green while consumers lose the ability to distinguish - caller cancellation with `errors.Is(err, context.Canceled)`. -- **Recommended direction:** Extend the existing external-package - `TestEngineRunPropagatesCallerCancellation` assertion to require both - `ErrLLMGenerate` and `context.Canceled`. Retain the focused internal tests - only for the distinct internal translation and runner responsibilities they - protect; do not add a parallel end-to-end cancellation test. -- **Required verification:** Run the focused public cancellation test normally - and with the race detector. Confirm that deliberately removing caller-context - propagation or cancellation wrapping at the facade boundary makes that test - fail. - -### Unresolved Observations - -None. The absence of package-level operation convenience functions was -confirmed and is not a consistency defect; the reviewed public API exposes -these operations only as `Engine` methods. - -### Coverage Ledger - -- **Facade shape and request translation:** All six methods reject a nil or - uninitialized engine before delegation. `Prepare`, `PrepareExecution`, and - `Run` use the same field-complete, defensive request conversion and classify - conversion failures as `ErrInvalidRequest`; inspections pass their scalar - selectors with the documented prompt/profile normalization behavior. - `RunPrepared` unwraps only the opaque handle reference. Each successful - facade method delegates once and converts the returned domain snapshot. -- **Context propagation:** Every method passes the supplied context directly - to its matching runner operation. Public tests protect cancellation before - inspection source work, active ordinary generation, and prepared generation, - and prove that a completed preparation is independent of later cancellation - of its preparation context. The missing consumer-boundary assertion for the - ordinary-run cancellation identity is recorded as S04-F01. -- **Inspection operations:** Prompt inspection loads declared metadata and - referenced content without profile, artifact, schema, capacity, or provider - work. Profile inspection resolves the effective target and credential state - without prompt or generation work. External-package tests protect nil and - blank inputs, not-found versus load identities, cancellation, point-in-time - behavior, agreement with preparation, and deep ownership of returned nested - values. -- **Preparation and ordinary execution:** `Prepare` returns a caller-owned, - credential-redacted prepared snapshot and performs no model generation. - `Run` returns a caller-owned result after one execution path; content - validation failure remains a successful result, while operational failures - return no partial result. Root tests protect representative translation, - prepared/generated metadata agreement, injected artifact and LLM behavior, - validation-result semantics, and the documented public error categories. -- **Prepared execution boundary:** `PrepareExecution` publishes one opaque, - engine-bound handle whose details are independent copies of frozen - preparation state. `RunPrepared` preserves owner binding, atomic single-use - claim behavior, independent execution context, no-result-on-error semantics, - collaborator identities, credential revalidation, capacity rejection, and - execution-only timing. External-package tests also protect concurrent claim - and run/discard behavior. The internal claim, admission, validation, and - release mechanisms remain assigned to later component stages. -- **Public error mapping:** Every runner error is routed through one facade - mapping point. Ordinary public identities and injected collaborator errors - remain discoverable with `errors.Is`; capacity failures become public - `CapacityError` values without leaking the internal type; successful paths - do not invent errors. Internal mapping tests appropriately own internal-type - containment, while public operation tests own consumer-visible categories - and collaborator identities except for S04-F01. -- **Ownership:** Request conversion freezes caller maps, slices, pointers, and - JSON-compatible values before internal use. Inspection, prepared, details, - and result conversions return fresh mutable values. The prepared-execution - contract tests demonstrate that later caller and source mutations do not - change frozen execution and that mutating one returned snapshot does not - change engine-owned state. -- **Convenience-function consistency:** The graph and source search found no - package-level `Prepare`, `PrepareExecution`, `Run`, `RunPrepared`, - `InspectPrompt`, or `InspectProfile` functions. There is therefore no second - operation surface whose translation, errors, or ownership can drift from - the methods. -- **Test ownership and duplication:** Root external-package tests protect the - exported facade and representative assembled workflows. Focused internal - tests own runner coordination and public-error translation mechanics. Some - lifecycle and error categories necessarily appear at both levels, but the - assertions address different stable boundaries; no removable semantic - duplication was found. S04-F01 is the one important identity currently - asserted only below the applicable public operation boundary. -- **Clarity and cost:** The operation facade is a uniform sequence of guard, - translation where needed, one delegation, public error mapping, and outward - conversion. No duplicated orchestration policy, repeated I/O, avoidable - copying, or operation-layer complexity was found. Costs inside preparation, - generation, validation, transport, and capacity remain assigned to their - owning later stages. - -### Verification Performed - -The code knowledge graph was used to find every public engine operation, -confirm their runner and conversion edges, inventory directly relevant root -tests, locate the internal cancellation assertions, and verify that no -package-level operation convenience functions exist. Important context, -error, ownership, and no-partial-result conclusions were confirmed against -source. - -The following focused commands passed: - -```sh -go test . -run 'Test(PrepareWorksWithFrameworkContractCorpus|RunSucceedsWithInjectedLLMClient|RunPassesPreparedRequestToInjectedLLMClient|EngineRunPropagatesCallerCancellation|ArtifactReaderFailuresPreserveArtifactLoadErrors|RunAddsLLMGenerateToCollaboratorPublicError|PrepareWithoutProfileMatchesSpecificPublicError|RunValidationFailureReturnsResult|PublicErrorsSupportErrorsIs|InspectProfileResolvesCredentialStatesWithoutPromptOrGeneration|InspectProfilePreservesPublicErrorIdentities|InspectProfileReturnsIndependentTargetMatchingPreparation|InspectPromptReturnsDeclaredMetadataWithoutExecutionWork|InspectPromptPreservesPublicErrorIdentities|InspectPromptReturnsIndependentMetadataMatchingPreparation|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|PreparedExecutionLifecycleAndEngineBinding|PreparedExecutionConcurrentClaimAllowsOneGeneration|PreparedExecutionRunAndDiscardRaceHasOneWinner|PreparedExecutionDiscardAndFormattingDoNotExposePrivateState|PreparedExecutionCredentialCapacityAndTimingBoundaries)$' -go test -race . -run 'Test(EngineRunPropagatesCallerCancellation|InspectProfilePreservesPublicErrorIdentities|InspectPromptPreservesPublicErrorIdentities|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|PreparedExecutionLifecycleAndEngineBinding|PreparedExecutionConcurrentClaimAllowsOneGeneration|PreparedExecutionRunAndDiscardRaceHasOneWinner)$' -count=3 -go test ./internal/usecase -run 'TestRunnerRunCancellationPreservesGenerationCategory' -go test . -run 'Test(MapPublicErrorPreservesGenerationCancellation|MapPublicErrorTranslatesCapacityError)$' -``` - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - operation-boundary review. -- Stages 7, 8, 10, 11, 13, 14, and 15 own prompt loading, profile loading, - validation, runner preparation/execution, prepared-handle lifecycle, - provider transport, and capacity mechanics respectively. Those stages - should treat the facade behavior recorded here as their outward contract - rather than repeat this public-boundary audit. - -## Stage 5: Internal Domain And JSON-Compatible Values - -### Scope Reviewed - -The review covered every source and test file in `internal/domain` and -`internal/jsonvalue`. Narrow traces into the root conversion boundary, -backend registry, prompt renderer, provider request builder, and prepared-run -cloning were used only to confirm the assumptions those callers make about -session normalization, copied JSON-compatible values, credential redaction, -and frozen schemas. Their broader validation, execution, transport, and -registry behavior was not audited. - -### Accepted Findings - -#### S05-F01: Session normalization accepts values that JSON encoding changes - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/domain/session.go` (`NormalizeSessionID`), - `internal/domain/session_test.go` (`TestNormalizeSessionID`), and callers in - `internal/usecase`, `internal/prompt`, and `internal/llm` -- **Contract at issue:** A normalized session ID is opaque consumer metadata - whose Unicode-code-point length is bounded and whose value is carried into - prepared metadata, rendered-prompt hashing, collaborator requests, and the - provider's JSON `session_id`. Normalization must not approve one byte string - while downstream encoding transmits a different identifier. -- **Evidence:** `NormalizeSessionID` trims the string and counts runes but never - checks `utf8.ValidString`. Go strings may contain invalid UTF-8, and - `utf8.RuneCountInString` counts malformed bytes as error runes rather than - rejecting them. A temporary probe passed `x\xffy`: normalization returned - the original invalid string with no error, while `encoding/json` emitted - `"x\ufffdy"`. Existing tests cover Unicode whitespace and the rune-count - boundary but no malformed encoding. The runner hashes the normalized string - before the provider request is JSON-encoded, so the exposed hash can also - describe a different session value than the provider observes. -- **Failure mode:** A direct or rendered session ID containing malformed UTF-8 - is accepted, retained, and hashed in one form but silently replaced with - Unicode replacement characters on the wire. Provider correlation and local - prepared/result metadata can therefore disagree for an accepted request. -- **Recommended direction:** Make valid UTF-8 part of the shared normalization - rule and reject malformed values before trimming/counting succeeds. Keep the - error contextual but independent of provider-transport implementation. -- **Required verification:** Add malformed UTF-8 cases before, within, and - after otherwise valid content to the domain table, then retain focused - caller checks that direct-request failures map to `ErrInvalidRequest` and - rendered-template failures map to the renderer category without provider - work. - -#### S05-F02: Numeric acceptance depends on the caller's Go representation - -- **Category:** contract-documentation consistency -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/jsonvalue/jsonvalue.go` (`copyValue` and - `maxSafeJSONInteger`), `internal/jsonvalue/jsonvalue_test.go` - (`TestCopyMapRejectsInvalidValues` and - `TestCopyMapValidatesJSONNumberSyntaxAndRange`), and the finite-number - contracts in `types.go` and `backends.go` -- **Contract at issue:** Public extra-parameter contracts accept finite - JSON-compatible numbers, and the shared copier promises to preserve - compatible concrete numeric types. Equivalent numeric values should not - become valid or invalid solely because the caller chose an integer, - floating-point, or `json.Number` representation unless that distinction is - an explicit contract. -- **Evidence:** Signed and unsigned integers outside `[-(2^53-1), 2^53-1]` - are rejected, but finite floats receive no corresponding safe-integer check - and `json.Number` is checked only for JSON syntax and finite `float64` - range. A temporary probe submitted the exact value `9007199254740992` as - `int64`, `float64`, and `json.Number`: only the `int64` was rejected. Go's - JSON encoder can emit the integer spelling without losing it. Existing tests - deliberately require rejection for the integer form while treating no - cross-type boundary as policy, and the public GoDoc says only that numbers - must be finite. -- **Failure mode:** Semantically equivalent backend, profile, or request extra - parameters have different construction or request outcomes based on an - incidental Go type. Consumers decoding into `json.Number` can bypass the - integer limit that consumers using `int64` encounter, so the current limit - neither implements the public finite-number contract nor a uniform safe- - integer policy. -- **Recommended direction:** Establish one numeric acceptance rule in - `internal/jsonvalue` and apply it consistently to every supported concrete - representation. The current public contract points toward accepting all - finite JSON-encodable numeric values; if a narrower interoperability limit - is intentionally retained, make it an explicit public contract and enforce - it for integral floats and `json.Number` as well. -- **Required verification:** Add a representation matrix at the largest - accepted and first rejected positive and negative integer boundaries for - signed integers, unsigned integers, integral floats, and `json.Number`, plus - finite fractional/exponent and non-finite cases. Retain concrete-type - assertions for accepted values and a public-boundary error-mapping check. - -#### S05-F03: JSON-value copying has no nesting or work bound - -- **Category:** correctness -- **Severity:** high -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `internal/jsonvalue/jsonvalue.go` (`Copy`, `CopyMap`, - `copyValue`, `copyMapValue`, and `copySequenceValue`) and - `internal/jsonvalue/jsonvalue_test.go` -- **Contract at issue:** Consumer-controlled extra-parameter and schema values - must fail as ordinary validation errors when their structure is unsafe to - process. Cycle rejection alone does not bound recursive stack use or the - amount of copying performed for an acyclic value. -- **Evidence:** Each pointer, map, slice, or array level recursively calls - `copyValue`, with no depth, visited-node, or copied-node budget. The `seen` - map tracks only the active recursion path and therefore detects cycles but - imposes no size bound. A temporary probe built an acyclic chain 20,000 maps - deep; `CopyMap` accepted and copied it. Sufficiently deeper caller-created - values can continue growing the goroutine stack until Go's fatal stack - limit. Shared acyclic subgraphs are also recopied once for every path rather - than counted or memoized, so a compact caller value can induce much larger - work. Existing tests cover direct map and slice cycles only. -- **Failure mode:** A deeply nested configuration or request can consume - disproportionate CPU, allocations, and stack and can eventually terminate - the process instead of returning `ErrInvalidConfig` or `ErrInvalidRequest`. - The same generic path is used during engine construction, request - conversion, backend lookup, and prepared schema/detail copying. -- **Recommended direction:** Give the shared copier an explicit, defensible - traversal budget that bounds nesting and total copied work, returning a - path-aware validation error when exceeded. Consider preserving already- - copied acyclic aliases or otherwise account for repeated subgraphs so the - bound covers expansion as well as source-node count. Keep the policy in this - package rather than adding different limits at each caller. -- **Required verification:** Add just-below, at-limit, and first-over-limit - cases for alternating map/slice/array nesting and for a shared acyclic - subgraph that expands through multiple paths. Confirm public configuration - and request callers translate the bounded failure without panic or provider - work. Exercise the focused tests under constrained stack/memory settings if - practical without making the default suite environment-dependent. - -#### S05-F04: The generic copier's supported shape contract is only partially tested - -- **Category:** testing -- **Severity:** medium -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `internal/jsonvalue/jsonvalue.go` and all four tests in - `internal/jsonvalue/jsonvalue_test.go` -- **Contract at issue:** `Copy` and `CopyMap` are the single shared validator - and ownership boundary for maps, slices, arrays, scalar and numeric concrete - types, schemas with empty object keys, extra-parameter maps without empty - keys, cycles, unsupported values, and JSON null versus empty containers. The - reflection branches that implement those distinctions need compact tests at - this package boundary. -- **Evidence:** Successful-copy tests cover one `map[string]int`, one - `[]string`, `int64`, `json.Number`, and an empty schema-object key. Rejection - tests cover non-string and empty map keys, one unsupported channel, map and - slice cycles, non-finite floats, two out-of-range integers, and malformed - `json.Number`. No test exercises the distinct array allocation path, named - scalar/map/slice/array types, signed and unsigned width preservation, - ordinary floats, pointer indirection, nil interface/typed map/typed slice - collapse to JSON null, or nil-versus-empty map and slice preservation. The - temporary probe confirmed typed nil maps and slices become nil while empty - containers remain allocated, but this intended JSON distinction has no - durable owner. -- **Failure mode:** A reflection refactor can silently change accepted types, - return a different concrete type, alias an array or nested collection, or - collapse an empty container to null without any focused test failing. Every - engine configuration, request override, and prepared schema/detail path - relies on this machinery. -- **Recommended direction:** Expand the package table by behavior branch, not - by every type permutation: representative named and unnamed scalars, maps, - slices, arrays, pointer/interface indirection, nil and empty containers, and - mixed nested structures. Assert both deep mutation isolation and concrete - type where preservation is promised. Keep higher-level tests to a small - integration sample rather than repeating this matrix. -- **Required verification:** Run the focused package tests and representative - backend, profile, request, and prepared-schema ownership tests. Confirm - deliberate shallow-copy, array-allocation, type-conversion, and nil/empty - regressions each fail at the shared package boundary. - -#### S05-F05: Internal prepared-run JSON tests exercise an unused serialization boundary - -- **Category:** testing -- **Severity:** low -- **Confidence:** high -- **Status:** accepted -- **Affected code:** JSON tags on `internal/domain.PreparedRun` and the three - tests in `internal/domain/prepared_run_test.go` -- **Contract at issue:** The root `promptkit.PreparedRun` owns the stable - consumer JSON representation. Internal domain values should be tested for - invariants their internal consumers use, not as a parallel serialization - contract with no production caller. -- **Evidence:** Production paths construct and clone `domain.PreparedRun`, then - convert it to the root public value before consumer serialization. Source - and graph searches found direct `json.Marshal` calls on the internal type - only in `prepared_run_test.go`. The secret and session assertions overlap - runner/public contract coverage, while the cache-control assertion is made - only against the internal type and would remain green if root conversion - dropped that field. The secret test also constructs a domain prepared value - containing an API key even though the type's invariant says such values must - never contain resolved credentials; it proves only that a dormant JSON tag - hides the deliberately invalid state. -- **Failure mode:** Maintainers pay for and may preserve internal JSON tags and - tests that do not protect the supported facade, while a regression in the - public prepared conversion can escape the only cache-control serialization - assertion. Legitimate internal representation refactors can require test - edits without changing any consumer behavior. -- **Recommended direction:** Test credential absence at the producer and clone - boundaries that own the domain invariant, and keep stable JSON assertions on - the root public type. Move or replace the cache-control case at that public - boundary if the compatibility risk warrants it; remove the parallel - internal serialization expectations and tags unless an actual internal - serialization consumer exists. -- **Required verification:** Confirm prepared construction and cloning never - retain direct credentials, and assert session omission plus message - cache-control behavior through root `PreparedRun` JSON. A source search - should show no remaining production dependency before removing internal - tags or tests. - -### Unresolved Observations - -None. Pointer values are currently dereferenced into JSON tree values and -typed nil pointers, maps, and slices collapse to JSON null. Those behaviors are -consistent with JSON encoding, but S05-F04 records the need to give the -supported nil and indirection distinctions durable package-level coverage. - -### Coverage Ledger - -- **Domain role and invalid states:** `internal/domain` is a dependency-neutral - vocabulary of carrier types, enums, presence bits, and credential-redacted - result shapes, not a collection of independently valid aggregate - constructors. Prompt/profile/output/target validity remains with the - parsers, registries, runner, and validators that have the necessary context. - Zero enum values and partially populated carrier structs are therefore - representable by design; callers validate before effectful use. -- **Session normalization:** One shared function trims Unicode whitespace, - omits blank values, and measures the 256-character limit in Unicode code - points. Direct request, rendered template, and transport callers all use it, - avoiding divergent length rules. Invalid UTF-8 is the uncovered invariant - defect recorded as S05-F01. -- **Credential containment:** Domain request and execution-target values can - temporarily carry a direct API key for execution, with JSON/YAML tags that - omit it. Preparation explicitly clears the credential before constructing a - `PreparedRun`, execution adds it only to the transient generation target, - and results clear it again. Focused use-case tests protect these producer - invariants; dormant internal serialization tests are addressed by S05-F05. -- **Prepared and schema immutability:** Prepared execution takes separate deep - snapshots for executable state and durable details. Cloning copies target - extra parameters through `jsonvalue.CopyMap`, input hashes, rendered - messages and cache-control pointers, structured-output wrappers, and schema - trees through `jsonvalue.Copy`. `Details` produces another fresh snapshot. - Root contract tests demonstrate that source/request/detail mutations do not - alter execution or later details. Lifecycle mechanics remain Stage 13 scope. -- **Generic ownership:** `internal/jsonvalue` is the single owner for recursive - validation and copying of arbitrary JSON-shaped trees. Root profiles and - request overrides, backend construction/lookups, and prepared schema/detail - cloning all call this package rather than maintaining separate recursive - copiers. Shallow map copies in the runner operate only on already validated, - engine-owned snapshots while applying precedence; no competing acceptance - policy was found. -- **Supported values:** The implementation accepts nil, booleans, strings, - finite floating-point values, bounded integer values, valid finite - `json.Number`, string-keyed maps, slices, arrays, interfaces, and pointer - indirection. It preserves assignable concrete scalar and collection types, - otherwise produces canonical `map[string]any` or `[]any` trees. S05-F02 - records the inconsistent integer policy, and S05-F04 records missing branch - protection. -- **Nil and empty distinctions:** A nil interface, pointer, map, or slice - becomes JSON null; non-nil empty maps and slices remain non-nil empty - containers. A nil top-level extra-parameter map remains nil. `Copy` permits - empty object keys for JSON Schemas, while `CopyMap` rejects empty keys at - every depth for provider extra parameters. These are coherent separate entry - contracts rather than duplicated machinery. -- **Cycles and unsupported values:** Active-path identity tracking rejects - pointer, map, and slice cycles, including nested cycles; maps with non-string - keys, channels, functions, structs, complex values, unsafe pointers, - malformed numbers, and non-finite floats are rejected with a structural - path. The lack of an acyclic traversal bound is S05-F03. -- **Copy cost:** Ordinary accepted trees are visited and allocated once per - occurrence, with sorted map keys making the first reported invalid path - deterministic. Boundary copies occur when caller ownership changes, backend - lookups publish snapshots, prepared execution separates payload and details, - and details are returned. No unnecessary duplicate validation owner was - found, but shared acyclic subgraphs can be expanded repeatedly as recorded - in S05-F03. -- **Test ownership:** Domain session tests own the shared normalization rule; - use-case tests own credential-free prepared construction and frozen - execution; root tests own public snapshots and JSON. JSON-value package tests - correctly use the narrow shared boundary but do not yet discriminate all - supported reflection branches (S05-F04). Internal prepared JSON tests are on - a non-production boundary (S05-F05). - -### Verification Performed - -The code knowledge graph was used to inventory every declaration and test in -both packages, trace all callers of `NormalizeSessionID`, `Copy`, and -`CopyMap`, and confirm prepared-run/schema clone and serialization ownership. -Important behavior was confirmed against complete package source and tests. - -The following focused commands passed: - -```sh -go test ./internal/domain ./internal/jsonvalue -go test ./internal/usecase -run 'Test(RunnerDirectSessionResolution|HashRenderedPromptIncludesSessionIDWhenPresent|RunnerPrepareExecutionCompletesWithoutAdmissionOrGeneration|RunnerRunPreparedKeepsDirectCredentialOutOfMetadata|ExecutionProfileToTargetPopulatesAllFieldsAndCopiesExtraParams)$' -go test . -run 'Test(PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|ExtraParamsTypedNestedValuesAreCopiedAcrossPublicBoundary|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|RunRejectsInvalidExtraParams)$' -go test -race ./internal/domain ./internal/jsonvalue -count=3 -go test -race . -run 'Test(PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|ExtraParamsTypedNestedValuesAreCopiedAcrossPublicBoundary|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup)$' -count=3 -``` - -A temporary program under the repository imported the two internal packages -and confirmed S05-F01, S05-F02, and the 20,000-level acceptance evidence for -S05-F03. It also recorded the current nil-versus-empty behavior for the -coverage ledger. The temporary source and directory were removed before the -audit artifact was edited. - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - shared-value review. -- Stage 6 should use the numeric and traversal findings when reviewing backend - extra parameters without repeating the generic copier audit. Stages 10 and - 13 own validation-plan construction and prepared-handle lifecycle; they - should treat the schema-copy and credential-redaction behavior recorded here - as established boundary evidence. -- Stage 11 owns runner precedence and execution coordination. The shallow - runner map copies were consulted only to confirm that validated nested values - already have a single owner; their broader merge behavior remains out of - scope here. - -## Stage 6: Backend Registry, Defaults, And Built-In Profiles - -### Scope Reviewed - -The review covered every source, test, and embedded YAML file in -`internal/backend`, `internal/defaults`, and `internal/profile/builtin`. -Narrow traces into `WithBackend`, `NewEngine`, execution-target resolution, -and the LLM-owned reserved request-field rule were used only to confirm public -translation, registry assembly, default consumption, and rule ownership. -Capacity admission and scheduling mechanics and outbound HTTP request -construction were not audited. - -### Accepted Findings - -#### S06-F01: The fixed model-request timeout is writable process-global state - -- **Category:** clarity -- **Severity:** low -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `internal/defaults/defaults.go` - (`LLMRequestTimeoutDefault`) and its reads in - `internal/llm/openai_compatible_client.go` - (`NewOpenAICompatibleClient` and `OpenAICompatibleClient.Generate`) -- **Contract at issue:** Framework defaults are fixed, application-neutral - policy. The architecture requires explicit dependencies rather than hidden - process-global state, and engine instances must not acquire behavior from a - writable package variable. -- **Evidence:** `LLMRequestTimeoutDefault` is declared as an exported package - `var`, although `10 * time.Minute` is a constant expression and every other - scalar in the defaults package is a constant. The model client reads this - binding when it constructs a default or cloned HTTP client and again when a - zero-valued internal client needs an HTTP client. A complete repository - search found no writer, setter, or documented mutability contract; the - binding is currently writable state solely because of its declaration. -- **Failure mode:** A later internal package or test can reassign the timeout - and silently change clients constructed afterward. A concurrent write can - also race with engine construction or the zero-value fallback, making a - nominally immutable framework default engine-order-dependent. Assigning a - non-positive duration would remove the intended whole-request cap. -- **Recommended direction:** Represent the timeout as a constant, preserving - its value and existing transport semantics. Do not add a setter or a test - that mutates the default; compile-time immutability is the stronger and - cheaper invariant. -- **Required verification:** Run the focused model-client construction and - deadline tests plus the race-enabled package suite. Confirm that all timeout - consumers still compile and that no assignment relied on the former - writable binding. - -### Unresolved Observations - -None. The OpenAI-compatible completion path is stored with other framework -constants but is consumed only by the model client; its placement does not -introduce mutable state or a competing rule. Actual URL and header construction -remains Stage 14 scope. - -### Coverage Ledger - -- **Registry construction and collisions:** `NewRegistry` builds one private - map from the built-in OpenRouter definition followed by consumer additions. - IDs are trimmed once, remain case-sensitive, and are checked after - normalization for both built-in and consumer collisions. Construction is - failure-atomic and publishes no partially populated registry. The public - option merely translates fields and copies the queue-capacity pointer; - validation has one owner in the registry. -- **Lookup and immutable snapshots:** The registry exposes no mutation or - enumeration API. `GetBackend` returns a fresh deep copy of extra parameters, - and `CapacityPolicies` creates a fresh map of scalar policy values. Nil - receivers return a not-found error or an empty policy map rather than - panicking. Focused tests mutate caller inputs, returned nested maps, and - returned policy maps and demonstrate isolation across lookups. -- **Endpoint and credential metadata:** Backend endpoints are trimmed and must - be absolute HTTP or HTTPS URLs with a host and without credentials, query - text, or fragments. Credential environment names are optional, trimmed, and - restricted to the documented portable identifier form. Existing package and - root integration tests cover rejected endpoint classes, invalid environment - names, trimmed values, and ordinary HTTP and HTTPS endpoints. Backend values - expose no arbitrary header map, so there is no registry-owned header state - to validate or copy; authorization and content-type construction belong to - Stage 14. -- **Parameter validation:** Empty and reserved top-level parameter keys are - rejected before registration. The registry consumes - `llm.IsReservedOpenAIChatRequestField`, while the model client owns and tests - the complete reserved-field list against its actual top-level payload. This - preserves dependency direction and gives the registry one representative - integration case rather than duplicating the transport's list. Recursive - validation and copying remain owned by `internal/jsonvalue`; S05-F02 and - S05-F03 already record its numeric inconsistency and missing traversal bound - and were not repeated here. -- **Capacity policy normalization:** Negative limits and queue capacities, - queues on unlimited backends, and overflowing total capacities are rejected. - A positive limit with no explicit queue receives the documented capacity, - while explicit zero is retained and unlimited backends produce no policy. - Registry policy extraction is correct; permit acquisition, fairness, - cancellation, and runtime bounds remain Stage 15 scope. -- **Default ownership:** The default execution target contains only the - documented 600-second framework baseline; all optional provider controls - remain unspecified. Schema, artifact-name, media-type, timeout, and - OpenAI-compatible path constants are application-neutral library values, - while OpenRouter capacity and connection defaults correctly remain with the - backend registry. The only mutable-default concern is S06-F01. -- **Built-in backend and profile consistency:** All 24 embedded profiles load - through the ordinary repository, have unique nonblank IDs, select the exact - `openrouter` registry ID, and omit endpoint, API-key environment, and raw - API-key fields. Their IDs and model values match the canonical catalog in - `docs/formats.md`. Consumer profiles may intentionally override matching - built-in profile IDs through repository precedence, whereas consumer - backends may not replace the reserved built-in backend ID. -- **Test ownership and cost:** Backend package tests own registry validation, - exact operational OpenRouter policy, copies, lookup errors, and capacity - snapshots. Built-in repository tests own embedded-catalog validity and - backend linkage. LLM tests own the reserved request-field list, and root - tests retain only representative public assembly and copy behavior. No - material redundant validation matrix or missing registry boundary test was - found. - -### Verification Performed - -The code knowledge graph was used to inventory the three scoped packages, -trace registry and built-in repository assembly, find all consumers of the -defaults, and confirm that the reserved request-field function has exactly the -registry normalizer and provider payload builder as callers. Important -conclusions were confirmed against complete source, tests, embedded profiles, -and canonical documentation. - -The following focused commands passed: - -```sh -go test -cover ./internal/backend ./internal/defaults ./internal/profile/builtin -go test ./internal/llm -run 'TestOpenAICompatibleClientRejectsInvalidExtraParamsBeforeProviderCall|TestNewOpenAICompatibleClientDoesNotMutateSupplied(Nonzero|Zero)TimeoutClient|TestNewOpenAICompatibleClientTreatsSuppliedNegativeTimeoutAsUnset' -go test ./internal/usecase -run 'Test(ResolveExecutionTargetUsesBackendProfileAndRequestPrecedence|ResolveExecutionTargetDefaultsAndProfileZeros|RunnerInspectProfileResolvesProfileAndBackendOnce)' -go test . -run 'Test(BackendOptionsAccumulateAndRegistrationsAreEngineLocal|BackendRegistrationRejectsInvalidAndDuplicateDefinitions|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|WithBackendCopiesQueueCapacity|PrepareUsesBuiltInProfileWithoutProfileDir|CustomProfileOverridesBuiltInProfile|RunUsesResolvedBackendWithBuiltInLLMClient|EngineExecutionSettingPrecedence)$' -go test -race ./internal/backend ./internal/profile/builtin -count=3 -go vet ./internal/backend ./internal/defaults ./internal/profile/builtin -``` - -The coverage diagnostic reported 94.0% statement coverage for -`internal/backend`, 100.0% for `internal/profile/builtin`, and no direct test -coverage for `internal/defaults`. Coverage alone was not treated as a finding: -the defaults are exercised through the consuming use-case, model-client, -artifact, validator, and root contract tests, and a separate test of constant -declarations would add no behavioral protection. - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - registry and defaults review. -- Stage 8 owns general filesystem profile parsing, validation, discovery, and - overlay mechanics. This stage established only that the embedded catalog - supplies valid ordinary profiles tied to the built-in backend. -- Stage 14 owns completion-URL composition, authentication and content-type - headers, payload merging and serialization, deadline behavior, response - handling, and transport resources. It should treat the shared reserved-field - ownership recorded here as established. -- 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. - -## Stage 8: Profile Sources And Repository Composition - -### Scope Reviewed - -The review covered all production code, tests, and fixtures in -`internal/profile` except the built-in subpackage. The profile format contract, -internal source documentation, root repository composition, profile source -options, and the narrow profile-inspection and preparation call paths were -consulted to establish precedence, public error translation, and selection -normalization. Runtime merging with backend definitions and request overrides -was not audited. - -### Accepted Findings - -#### S08-F01: Non-finite profile settings bypass numeric range validation - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/profile/filesystem_repository.go` - (`validateProfile`), `profiles.go` (`validatePublicProfile`), profile - validation cases in `internal/profile/repository_test.go`, and in-memory - profile validation tests in `engine_test.go` -- **Contract at issue:** Profile `temperature` must be a number from zero - through two and `top_p` must be a number from zero through one. File-backed - and in-memory profiles follow the same ranges; values outside those closed - intervals cannot enter an execution profile. -- **Evidence:** Both validators implement the floating-point bounds solely as - less-than and greater-than comparisons. IEEE NaN makes every such comparison - false. YAML supports `.nan`, and a temporary `fstest.MapFS` profile with - both `temperature: .nan` and `top_p: .nan` loaded successfully with NaN - values in the returned `domain.ExecutionProfile`. Go callers can supply - the same values to the duplicated in-memory validator. No focused profile - test uses a non-finite setting. -- **Failure mode:** Exact inspection and preparation can accept a profile whose - numeric controls are outside the documented domain. The value can then reach - an injected model client or fail much later during default-client JSON - serialization, changing a profile-load error into a generation-time failure. -- **Recommended direction:** Reject NaN and infinity explicitly for every - floating profile setting before applying its closed numeric range. Resolve - this in the shared semantic validator already recommended by S02-F02 so file - and in-memory sources cannot diverge. -- **Required verification:** Run one shared boundary table against both source - categories. Include finite lower and upper bounds, their finite neighbors - outside the range, positive and negative infinity, and NaN for temperature - and top-p. File-source failures must retain `ErrInvalidProfile`; in-memory - failures must retain the public `ErrInvalidConfig` construction boundary. - -#### S08-F02: File-backed extra parameters skip profile-boundary validation - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/profile/filesystem_repository.go` - (`loadProfile` and `validateProfile`), - `internal/domain/domain.go` (`ExecutionProfile.ExtraParams`), and the - extra-parameter case in `internal/profile/repository_test.go` -- **Contract at issue:** Profile `extra_params` accepts only - JSON-compatible values with non-empty string keys. Loading and validating a - file profile must establish that invariant before exact inspection, - preparation, or an injected client consumes the resulting execution - profile. -- **Evidence:** YAML is decoded directly into - `domain.ExecutionProfile.ExtraParams`, and `validateProfile` never invokes - the repository's `internal/jsonvalue` validator. The existing test proves - that one valid nested map can be marshaled but has no rejection cases. A - temporary profile containing an empty key and a `.nan` value loaded - successfully; immediately marshaling the returned map with `encoding/json` - failed with `json: unsupported value: NaN`. The in-memory path, by - contrast, validates and deeply copies the same field through - `jsonvalue.CopyMap`. -- **Failure mode:** `InspectProfile` and preparation can report success for a - profile that violates its file format. Default-client execution fails only - at payload construction, while an injected client receives a value the - public format contract says cannot exist. -- **Recommended direction:** Validate and deeply copy decoded - `extra_params` through the shared JSON-value owner as part of profile - normalization. Keep OpenAI-compatible reserved-field collision policy with - its existing client/registry owners; this finding concerns the universal - JSON-compatible shape only. -- **Required verification:** Add a compact repository table for empty keys, - non-finite numbers, unsupported YAML-decoded values, nested invalid values, - and a representative valid nested tree. Run it through OS-directory and - `fs.FS` sources, confirm `ErrInvalidProfile` and source-path context, and - retain one public preparation or inspection check for `ErrProfileLoad`. - -#### S08-F03: Filename stems can make malformed unrelated profiles authoritative - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/profile/filesystem_repository.go` - (`loadProfile`, `readProfileFileMetadata`, and `profileFileMetadata`) - and selected-error cases in `internal/profile/repository_test.go` -- **Contract at issue:** Definitions are selected by their YAML `id`, not - filename. A higher-precedence source is authoritative only when it contains - the requested ID; an unrelated malformed file must not block a valid - definition in the same source or fallback chain. -- **Evidence:** Before strict decoding, `loadProfile` treats either the - metadata ID or the filename stem as an ID match. A matching stem therefore - turns an unknown-field error or top-level raw `api_key` into a selected - source failure even when parsed metadata identifies a different profile. - A temporary source containing malformed `target.yaml` with YAML ID - `unrelated` and valid `valid.yaml` with YAML ID `target` returned - `ErrInvalidYAML` instead of the valid profile. Existing invalid-YAML and - raw-key fixtures are requested by underscore filename stems, so those tests - encode the second identity system rather than distinguishing it. -- **Failure mode:** Adding or renaming an unrelated malformed file can take a - valid profile offline. In an ordinary or application fallback source, the - same filename can also prevent resolution from reaching a valid - lower-precedence profile or built-in. -- **Recommended direction:** Use recovered YAML metadata as the sole authority - for point selection and raw-key classification. Do not infer identity from a - filename. When malformed YAML provides no reliable ID, treat it as unrelated - to an exact lookup rather than contradicting the YAML-ID contract. -- **Required verification:** Exercise both OS and `fs.FS` repositories with - a malformed same-stem/different-ID file beside a valid exact definition. - Repeat through an overlay with the valid definition in the fallback. Retain - canonical-ID cases showing that a selected unknown field and raw - `api_key` stop fallback with their contextual sentinel. - -#### S08-F04: Strict profile decoding silently ignores additional YAML documents - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/profile/filesystem_repository.go` - (`loadProfile` and `readProfileFileMetadata`) and strict-decoding cases - in `internal/profile/repository_test.go` -- **Contract at issue:** One profile file contains one strictly decoded - definition. Every YAML document supplied in that file must be accounted for; - later documents cannot bypass known-field, raw-key, or semantic validation. -- **Evidence:** Both the metadata decoder and the strict typed decoder call - `Decode` once and return without requiring end of stream. A temporary - selected profile followed by `---` and a second same-ID document containing - an unknown field loaded successfully and returned only the first model. - Existing strict-decoding tests place unknown fields in the first document - and never exercise the stream boundary. -- **Failure mode:** Configuration after a YAML document separator is silently - ignored. A maintainer can believe a replacement model, credential setting, - or provider option is active while inspection and execution use only the - earlier document. -- **Recommended direction:** Require exactly one YAML document by decoding the - definition and then requiring the next decode to return `io.EOF`. Apply - the same stream rule to metadata classification so later raw-key or identity - data cannot escape the selected-source decision. -- **Required verification:** Add a shared strict-decoder table covering one - document with comments, a second populated document, a second empty - document, malformed trailing YAML, and a raw key in a trailing document. - Verify OS and `fs.FS` boundaries preserve `ErrInvalidYAML` and source-path - context. - -#### S08-F05: Whitespace-bearing file profile IDs are valid but publicly unreachable - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/profile/filesystem_repository.go` - (`loadProfile`, `readProfileFileMetadata`, and `validateProfile`), - `internal/usecase/profile_inspection.go` (`Runner.InspectProfile`), and - profile selection in `internal/usecase/runner.go` -- **Contract at issue:** A file profile ID is required to be non-empty, and - public request/default and inspection selection normalize surrounding - whitespace. Every profile accepted under that rule must have one reachable, - normalized identity, consistent with in-memory profiles whose IDs are - explicitly trimmed. -- **Evidence:** Metadata extraction trims the YAML ID, but the strict - `domain.ExecutionProfile.ID` is neither trimmed nor rejected when it has - surrounding whitespace. `loadProfile` compares that raw ID to the requested - ID before validation. A temporary file with `id: " target "` returned - `ErrProfileNotFound` for `target`. Calling the internal repository with - the whitespace-bearing string can select and validate it, but every public - selection path trims the lookup ID first, making the definition unreachable - through the engine. -- **Failure mode:** A syntactically accepted profile silently behaves as - absent and resolution may fall through to a lower-precedence profile with a - different model or endpoint. The file contents and metadata classification - disagree about the source's authoritative ID. -- **Recommended direction:** Normalize the decoded profile ID once before - selection and validation, as the in-memory path does, or explicitly reject - surrounding whitespace in the file-format contract. Use that one normalized - value for metadata association, duplicate detection, returned profiles, and - diagnostics. -- **Required verification:** Cover leading and trailing whitespace, a - whitespace-only ID, duplicate IDs that become equal after normalization, and - an ordinary normalized ID. Verify exact inspection and preparation select - the same normalized profile and report the normalized ID. - -#### S08-F06: Every profile file is decoded twice during each source lookup - -- **Category:** efficiency -- **Severity:** medium -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `internal/profile/filesystem_repository.go` - (`loadProfile` and `readProfileFileMetadata`) and point-in-time profile - lookup through the repository overlay -- **Contract at issue:** Exact point-in-time lookup must read enough metadata - across one source to select an ID and detect duplicates, but it need not - parse every complete YAML tree twice or strictly materialize unrelated - profiles. -- **Evidence:** For every discovered YAML file, `loadProfile` first invokes a - YAML decoder through `readProfileFileMetadata`, then unconditionally - creates a second strict decoder for the same bytes. This happens for valid - unrelated profiles as well as the selected profile. A lookup that falls - through overlays repeats the catalog scan in each source; successful - built-in fallback currently means two decodes for each catalog entry on - every inspection, preparation, or ordinary run lookup. The file bytes are - read once, so the confirmed waste is parsing and allocation rather than - duplicate filesystem reads. -- **Failure mode:** CPU and allocation cost includes an avoidable additional - operation linear in the total YAML bytes of every consulted catalog. Large - consumer profile sources magnify that work on each point-in-time operation, - including repeated inspection and execution. -- **Recommended direction:** Perform one metadata pass across the source, then - strictly decode and normalize only canonical ID matches, or otherwise - arrange one parse to supply both classification and strict selected - decoding. Preserve the uncached point-in-time source behavior and - deterministic duplicate detection. -- **Required verification:** Benchmark representative small and large catalogs - before and after the change, reporting allocations as well as time. Retain - behavior tests for selected malformed files, unrelated malformed files, - duplicates, and repeated lookups so the optimization does not turn source - access into a stale cache. - -### Unresolved Observations - -None. Reserved OpenAI-compatible request-field collisions are deliberately -validated by the model-client and backend-registry owners and remain outside -this profile-source pass. - -### Coverage Ledger - -- **Strict decoding and raw credentials:** The first YAML document is decoded - with known fields enabled. Selected unknown fields preserve - `ErrInvalidYAML`, and a top-level raw `api_key` is detected separately so - it preserves `ErrRawAPIKeyNotAllowed`. Unknown or raw-key definitions with - a reliably different metadata ID do not block a valid lookup. Filename - association and additional document handling are defective as S08-F03 and - S08-F04 record. -- **Profile validation:** Selected profiles require a nonblank ID, one of - backend or endpoint, and a nonblank model. Backend IDs are trimmed, numeric - finite values within the documented ranges are accepted, and ordinary - nested `extra_params` decode into the domain value. Non-finite scalar - values and universal extra-parameter shape are not enforced as S08-F01 and - S08-F02 record. The already accepted S02-F02 owns consolidation of the - duplicated file and in-memory semantic validators. -- **Filesystem parity:** OS-directory repositories adapt their directory with - `os.DirFS`; caller-supplied `fs.FS` repositories then enter the same - `loadProfile` implementation and shared file catalog. Recursive discovery, - extension filtering, deterministic ordering, strict decoding, validation, - duplicate behavior, and error classification therefore have one semantic - implementation. Source-specific tests appropriately confirm a real OS - directory and representative `fs.FS` behavior without duplicating the - complete validation matrix. -- **Identity and duplicates:** Valid matching YAML IDs are independent of - directory nesting, and all matches are collected before duplicate - classification. File-catalog sorting makes duplicate path diagnostics - deterministic. IDs that differ only by accepted surrounding whitespace do - not share the public normalized identity, as S08-F05 records. -- **Overlay and fallback:** The root assembles repositories in the documented - order: in-memory, ordinary configured source, application fallback, then - embedded built-ins. Each overlay consults its fallback only for - `ErrProfileNotFound`; YAML, validation, raw-key, duplicate, directory, - read, and cancellation failures stop resolution. Definitions are complete - values and are never field-merged. Root contract tests add useful - composition protection without repeating the repository's parser matrix. -- **Absence and authoritative failures:** Reliably unrelated malformed and - raw-key files are ignored during a point lookup, while canonical matching - failures are contextual and authoritative. A missing configured directory - remains a profile-load failure instead of silently becoming a built-in - miss. Filename fallback incorrectly broadens authority as S08-F03 records. -- **Errors and cancellation:** Blank lookup IDs preserve - `ErrInvalidProfile`; absent IDs preserve `ErrProfileNotFound`; selected - syntax, semantic, credential, duplicate, discovery, and read failures retain - useful source-relative paths or directory context. File discovery and the - per-file loop honor context cancellation, and overlays do not fall back - after a cancellation error. Public exact absence remains - `ErrProfileNotFound`, while other repository failures map to - `ErrProfileLoad`. -- **Ownership and immutability:** Each file lookup decodes a fresh domain - profile, so caller mutation is not retained by the repository. The in-memory - adapter stores validated copies and returns a new profile with copied - `extra_params`; overlays do not mutate returned definitions. Resolution - and public conversion create later copies, with runtime override and backend - merging deferred to Stage 11. The invalid source values in S08-F01 and - S08-F02 must be rejected before those immutable snapshots are created. -- **Repeated work:** Point-in-time inspection, preparation, and execution - intentionally perform fresh source lookup, and no stale repository cache was - found. Each YAML file is read once per consulted source lookup. The - additional catalog-wide decoder pass is the avoidable work in S08-F06. -- **Test ownership:** Repository tests own YAML selection, source recursion, - raw-key rejection, profile validation, duplicates, and overlay semantics. - Root tests own option composition, complete precedence, lazy source access, - and public error translation. Use-case inspection tests own one exact - repository lookup and collaborator-free inspection behavior. No material - high-level repetition of the full parser matrix was found; the missing - regression cases are tied directly to the accepted findings above and the - established S02-F02 boundary table. - -### Verification Performed - -The code knowledge graph was used to inventory the scoped package, trace both -source constructors and overlay construction into `NewEngine`, trace profile -lookups into exact inspection and preparation, and identify all consumers of -`domain.ExecutionProfile`. Important conclusions were confirmed against the -complete source, repository tests, profile fixtures, public GoDoc, and -canonical profile and source contracts. - -The following focused commands passed: - -```sh -profile_audit_cover=$(mktemp) -go test -coverprofile="$profile_audit_cover" ./internal/profile -go tool cover -func="$profile_audit_cover" -rm "$profile_audit_cover" -go test ./internal/usecase -run 'TestRunnerInspectProfile' -go test . -run 'Test(CustomProfileOverridesBuiltInProfile|MalformedCustomProfileDoesNotFallbackToBuiltIn|PrepareWorksWithProfile(FS|File)OverBuiltIns|FallbackProfileSource(Precedence|PreservesLazyLoadingAndErrors|WorksAcrossWorkflows)|SelectedProfile(RawAPIKey|InvalidYAML|RepositoryReadFailure)MapsToProfileLoad)$' -go test -race ./internal/profile -count=3 -go vet ./internal/profile -``` - -The coverage diagnostic reported 85.6% statement coverage for -`internal/profile`; `validateProfile` reported 66.7%. Coverage was used only -to find decisions for direct inspection. The accepted findings are based on -source traces and reproduced behavior, not the percentages. - -Temporary package probes, removed before this artifact was edited, confirmed: - -- malformed `target.yaml` with a different YAML ID shadowed a valid exact - definition; -- a second YAML document containing an unknown field was ignored; -- NaN temperature and top-p values passed profile validation; -- an empty-key, NaN-bearing `extra_params` map loaded and then failed JSON - serialization; and -- a whitespace-bearing YAML ID was absent under the normalized public lookup - identity. - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - profile-source review. -- S02-F02 already owns consolidation of file and in-memory profile semantic - validation. S08-F01 and S08-F02 supply confirmed correctness requirements - for that shared owner rather than creating a second duplication finding. -- Stage 11 owns backend lookup, default/profile/request merge precedence, - effective-target validation, credential resolution, and runtime override - semantics. It should treat the selected immutable profile and source - precedence recorded here as established inputs. -- 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. - -## 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. - -## Stage 11: Inspection And Execution-Target Resolution - -### Scope Reviewed - -The review covered `internal/usecase/profile_inspection.go`, -`internal/usecase/prompt_inspection.go`, and the selection, target-resolution, -and preparation portions of `internal/usecase/runner.go`, together with their -focused use-case and public contract tests. The preparation entry point in -`internal/usecase/prepared_execution.go` was consulted only through creation -of its frozen execution snapshot. Model invocation, repair coordination, -capacity scheduling, and prepared-handle lifecycle were not audited. - -The code graph was used first to establish the exact shared paths. Prompt -inspection and all preparation workflows converge on -`resolvePromptDefinition`; profile inspection and preparation converge on -`resolveProfileSelection`, `resolveExecutionTarget`, and -`validateResolvedExecutionTarget`; and `Prepare`, `Run`, and -`PrepareExecution` converge on `resolvePreparation`. The review followed those -helpers through default, backend, profile, request, credential, session, -output-contract, schema-metadata, rendering, hashing, and prepared-snapshot -consumers only as far as needed to decide preparation fidelity. - -### Accepted Findings - -#### S11-F01: Runtime NaN overrides bypass the documented numeric ranges - -- **Category:** correctness -- **Severity:** high -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/usecase/runner.go` - (`mergeExecutionTargetOverride` and `resolvePreparation`), - `internal/usecase/runner_test.go` - (`TestRunnerPrepareInvalidRequestNumericOverridesFail` and the target-merge - tables), and root execution-setting tests -- **Contract at issue:** A request temperature must be from `0` through `2` - and top-p must be from `0` through `1`. Preparation is the boundary that - validates explicit numeric overrides and publishes stable, executable target - metadata. -- **Evidence:** `mergeExecutionTargetOverride` rejects each float only with - less-than and greater-than comparisons. IEEE NaN makes all those comparisons - false, so both `Temperature: &nan` and `TopP: &nan` are copied into the - effective target and marked explicitly present. A temporary public probe - passed NaN for both fields to `Engine.Prepare`; it returned a nil error and a - `PreparedRun` containing NaN in both effective settings. The invalid-override - table covers ordinary finite values below and above the ranges but not - non-finite values. S08-F01 records the analogous profile-source defect; this - finding is the distinct per-request path. -- **Failure mode:** A request can produce a supposedly prepared value that - cannot be represented by its stable JSON contract and cannot be serialized - as a valid provider number. Failure is deferred from request validation to a - later serializer or injected client, and the accepted target contradicts - the public numeric range. -- **Recommended direction:** Apply an explicit finite-number check before the - range checks for both pointer fields, using the same domain-level numeric - acceptance rule ultimately used to resolve S08-F01. Return the existing - invalid-request category before artifact, rendering, or model work. -- **Required verification:** Exercise NaN, positive and negative infinity, - both finite out-of-range sides, both exact boundaries, and representative - interior values for temperature and top-p. Cover `Prepare`, - `PrepareExecution`, and ordinary request resolution, require - `ErrInvalidRequest` with no partial prepared value for invalid cases, and - retain explicit-zero presence assertions. Confirm stable JSON never receives - a non-finite effective setting. - -#### S11-F02: Request output-contract replacement accepts unsupported enum values - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/usecase/runner.go` - (`resolveOutputContract`, `resolvePreparation`, and - `resolveStructuredOutput`), `internal/usecase/prepared_execution.go` - (`PrepareExecution` and `prepareValidation`), and output-contract preparation - tests in `internal/usecase/runner_test.go` and the root package -- **Contract at issue:** A request override replaces the complete prompt - output contract, with empty format normalized to `text`; every other - effective format and validation mode must be one of the declared supported - constants. `PrepareExecution` promises a completely prepared execution - snapshot rather than a handle retaining an already-invalid contract. -- **Evidence:** `resolveOutputContract` copies a non-nil request value and - defaults only an empty format. It never checks an unknown nonempty format or - validation mode. `resolveStructuredOutput` and both validation-plan - preparers special-case only `json_schema`; a different unknown mode is - retained without validation. A temporary public probe supplied format - `binary` and validation mode `unknown`. Both `Engine.Prepare` and - `Engine.PrepareExecution` returned nil errors, and their details preserved - the unsupported values. Existing tests cover valid replacement and the - empty-format default, but no unsupported enum. -- **Failure mode:** Offline preparation publishes metadata outside the stable - output vocabulary, while executable preparation returns a handle whose - selected content-check mode was never accepted. The preparation APIs cease - to be reliable preflight boundaries, and different later consumers can - silently interpret the unknown format as text or reject the mode only after - work that preparation should have prevented. -- **Recommended direction:** Normalize and validate the complete effective - output contract once in the shared resolution phase. Preserve the documented - empty-format fallback, require one supported validation mode, and keep - JSON-Schema path checks and schema loading in their existing preparation - owners. Request-invalid enum values should retain the invalid-request - identity rather than masquerading as source or generated-content failures. -- **Required verification:** At both preparation APIs, table the empty and - three supported formats, every supported validation mode, an empty mode, - unknown nonempty format and mode values, and JSON-Schema path requirements. - Invalid request values must fail before artifact reads, rendering, - validation-plan construction, admission, or generation and return no - partial value or handle. Retain a parity assertion that both preparation - workflows publish the same normalized contract for every valid case. - -### Unresolved Observations - -None. Concerns about reserved provider parameters, timeout-to-duration -conversion, model calls, repairs, capacity leases, and handle claiming were -assigned to their owning later stages rather than inferred from preparation -traces. - -### Coverage Ledger - -- **Prompt selection and inspection:** `InspectPrompt`, `Prepare`, `Run`, and - `PrepareExecution` use the same exact ID/version repository lookup and - definition hash. Nonblank identifiers are passed unchanged, ambiguous and - absent selections preserve source identities for public mapping, and - inspection copies declared input metadata without resolving a default - profile, schema, artifact, template, or credential. Focused internal and - root tests protect unchanged lookup values, call counts, hash parity, - ownership, cancellation classification, and public not-found isolation. -- **Profile and backend selection:** Inspection trims its required explicit - profile ID; request preparation selects a trimmed explicit ID before the - prompt's trimmed default. Both paths use the same repository precedence, - make a value copy before normalizing the selected backend ID, resolve that - backend once, and preserve profile and backend error identities. Endpoint- - only profiles retain an empty backend identity, while profile or request - endpoint overrides retain a selected backend's identity and capacity key. -- **Target merging:** `resolveExecutionTarget` applies the framework timeout - baseline, backend defaults, profile values, and request override in order. - Profile numeric zero inherits the lower layer, request pointer zero is - explicit and recorded in `ExecutionTargetPresence`, nonblank strings replace - their lower layer, reasoning implements inherit/set/clear tri-state - semantics, and each nonempty extra-parameter map replaces rather than merges - its predecessor. Target helper and engine integration tables cover every - field and backend/profile/request precedence. S11-F01 is the untested NaN - escape from the otherwise complete float range checks. -- **Credentials:** Backend and profile environment-variable names follow the - documented precedence; a profile requiring a direct key clears an inherited - backend name, and an explicit request environment name or direct key - satisfies preparation. Inspection reports the environment name or separate - direct-key requirement without reading the environment. Preparation checks - availability, retains a direct value only in private execution state, and - clears it from prepared metadata. Focused tests cover absent environments, - direct-key precedence, request-name precedence, mutual exclusivity during - inspection, and redaction. -- **Session precedence:** Direct session IDs are normalized before source - loading. A nonblank direct value clears the session template only on a - definition copy, is installed after message rendering, leaves the original - definition hash unchanged, and participates in the rendered-prompt hash. A - blank direct value retains template behavior. The renderer's source-owned - normalization and limits were established in Stage 9 and were not reopened. -- **Output contracts and schemas:** Prompt inspection reports the normalized - declared contract without loading a schema. A request value replaces the - whole prompt contract and an empty effective format becomes text. Schema - metadata is loaded only for JSON-Schema mode; the compiled-plan versus - document-only distinction remains S10-F03. Unsupported request enums bypass - the shared preparation boundary as S11-F02 records. -- **Preparation and freezing:** `Prepare` and `Run` use one resolution and - completion pipeline; `PrepareExecution` uses the same resolution and render - completion around a retained validation plan. The prompt definition is - consumed into hashes and rendered messages during preparation, target maps - and public results are copied, and the opaque execution snapshot is cloned - before it is exposed. Maintained public tests mutate prompt, profile, schema, - artifact, request, and returned-detail sources after preparation and protect - later execution independence. Prepared-handle synchronization and credential - revalidation remain Stage 13 scope. -- **Errors and cancellation:** Blank required selections fail before source - work; canceled inspection fails before repository calls and preserves both - operation and context identities; absent sources remain distinct public - not-found errors; unknown backends remain profile-load failures; invalid - numeric request values and missing credentials remain invalid requests; and - schema preparation remains a validation operation. No new error-identity - defect was found apart from the invalid values accepted by S11-F01 and - S11-F02. -- **Duplication and test ownership:** Selection, hashing, profile/backend - resolution, target merging, and structural target validation each have one - use-case owner shared by inspection and preparation. Root tests own public - mapping, source assembly, and mutation independence; focused use-case tests - own collaborator call counts, merge rules, and operation categories. No - duplicate finding was warranted. S02-F02 and S08-F01 already establish that - profile acceptance itself needs a shared domain owner; Stage 17 can decide - whether runtime numeric and output-contract validation should join the same - source-neutral validation boundary. - -### Verification Performed - -The code knowledge graph inventoried every function in the three scoped files, -traced the shared helpers inbound from inspection and all preparation entry -points, and traced their default, backend, profile, request, credential, -output-contract, renderer, schema, and snapshot consumers. Source was then read -for every scoped helper and focused test, together with public GoDoc, the -framework format reference, the package consumer guide, internal runner and -architecture documents, and prior audit handoffs. - -The following focused commands passed: - -```sh -go test ./internal/usecase -run 'Test(RunnerInspectPrompt|RunnerInspectProfile|RunnerPrepare|ResolveExecutionTarget|MergeExecutionTarget|ExecutionProfileToTarget)' -count=1 -go test . -run 'Test(InspectPrompt|InspectProfile|EngineExecutionSettingPrecedence|CustomBackendFlowsThroughProfilesOverridesAndInjectedClient|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails)' -count=1 -resolution_audit_cover=$(mktemp) -go test -coverprofile="$resolution_audit_cover" ./internal/usecase -go tool cover -func="$resolution_audit_cover" -rm "$resolution_audit_cover" -go vet ./internal/usecase . -``` - -The repository-wide `go test ./...`, `go test -race ./...`, `go vet ./...`, -and `go run ./examples/go-library/prepare` checks also passed. The coverage -diagnostic reported 90.3% statement coverage for `internal/usecase`; coverage -was used only to locate unexercised resolution branches, not as evidence by -itself. - -A temporary public-package probe, removed before this artifact was edited, -confirmed that: - -- `Prepare` accepted NaN temperature and top-p pointers and returned both NaN - values in `EffectiveModelParams`; and -- `Prepare` and `PrepareExecution` accepted format `binary` and validation mode - `unknown` and preserved both unsupported values in their returned details. - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - inspection and preparation review. -- S08-F01 owns non-finite values entering from profile sources; S11-F01 owns - the distinct request-override path. A later remediation should use one - finite-range rule rather than fixing these paths independently. -- Stage 12 should use the resolved prompt, target, contract, session, and - prepared values recorded here as established inputs. It should not re-audit - merge precedence; S11-F02 supplies the preflight defect when considering - whether invalid modes can reach generation or validation. -- Stage 13 owns lifecycle, concurrent claim/discard, execution-time credential - revalidation, and the retained snapshot after `PrepareExecution` returns. - The source-to-snapshot freezing boundary recorded here is its starting - invariant. -- Stage 14 owns endpoint construction, environment lookup inside the default - client, timeout conversion, reserved extra-parameter enforcement, and wire - serialization of the already resolved target. Those mechanics were not - reviewed here. -- Stage 17 should consolidate the source-neutral acceptance rules already - identified by S02-F02, S08-F01, S11-F01, and S11-F02 without moving use-case - orchestration into a lower-level source package. - -## Stage 12: Ordinary Execution, Validation, And Repair Coordination - -### Scope Reviewed - -The review covered the ordinary post-resolution path in -`internal/usecase/runner.go`, the optional repair path in -`internal/usecase/repairer.go`, `internal/usecase/capacity_error.go`, and the -corresponding ordinary-run sections of `internal/usecase/runner_test.go`. -Public result and error contracts and the internal runner document were -consulted to establish expected outcomes. Provider transport encoding, -capacity-pool scheduling mechanics, and prepared-handle claiming and cleanup -were not audited. - -The code graph was used first to bound the state machine. Ordinary `Run` -resolves its Stage 11 inputs, admits the selected backend once, defers the -lease release, completes artifact loading and rendering, and delegates all -generation, artifact construction, validation, optional repair, and result -construction to `executePreparedRun`. That helper is also called by -`RunPrepared`; the shared transitions were considered here, but the prepared -caller's lifecycle remains Stage 13 scope. - -### Accepted Findings - -#### S12-F01: Repair generation drops explicit numeric override presence - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/usecase/runner.go` - (`executePreparedRun` and its `RepairRequest` construction), - `internal/usecase/repairer.go` (`RepairRequest` and - `defaultOutputRepairer.Repair`), and the repair tests in - `internal/usecase/runner_test.go` -- **Contract at issue:** `ExecutionTargetPresence` distinguishes an explicit - numeric zero from an inherited zero so an LLM client can preserve provider - omission semantics. The runner contract states that the same effective - execution target reaches initial generation and every repair attempt. -- **Evidence:** Initial generation copies `prepared.TargetPresence` into its - `GenerateRequest`. `RepairRequest` has no corresponding field, so - `executePreparedRun` cannot pass it to the repairer and the default repairer - constructs a second `GenerateRequest` with every presence bit false. A - temporary package probe supplied explicit zero temperature, maximum tokens, - and top-p values: the initial request contained all three presence bits, - while the default repair request observed by the same client contained none. - Existing tests assert presence only on initial generation; repair tests - inspect nonzero target values, backend identity, session ID, and structured - output but not presence. -- **Failure mode:** An internally enabled repair can run under provider - defaults even though the original request explicitly selected numeric zero - values. Initial and repaired outputs are then generated under observably - different effective settings, and an injected client receives a request - that contradicts the documented omission semantics. The public `Engine` - currently installs no repairer, which bounds the defect to the documented - internal optional path rather than eliminating it. -- **Recommended direction:** Carry the resolved presence value through - `RepairRequest` and into the default repairer's `GenerateRequest`. Consider - one execution-request constructor for the target, presence, credential, and - structured-output fields so the initial and repair paths cannot drift while - retaining their intentionally different prompts. -- **Required verification:** Drive the ordinary runner through the real - default repairer with a recording client. For each numeric field, prove an - explicit zero has the same presence on initial and repair calls and an - inherited zero remains absent on both. Retain the existing session, backend, - structured-output, and direct-credential assertions, and let Stage 14 own - provider-wire serialization tests. - -#### S12-F02: Repaired results omit usage from earlier model calls - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/usecase/runner.go` - (`executePreparedRun`), `internal/domain/domain.go` (`TokenUsage` and - `RunResult`), and usage and repair tests in - `internal/usecase/runner_test.go` -- **Contract at issue:** A successful run result includes token accounting for - the model work performed by that run. An internal repair makes another model - call and can make several, so retaining only one response does not account - for the completed operation's consumption. -- **Evidence:** After every repair, `executePreparedRun` assigns - `genResp = repairResp`; result construction later copies only - `genResp.Usage`. Usage from initial generation and every earlier repair is - discarded. A temporary package probe gave initial and successful repair - responses total-token counts of 11 and 7; the successful run reported 7, - not 18. Ordinary success tests protect single-call pass-through, but every - repair response in the maintained suite has zero usage and no repair test - asserts accounting. -- **Failure mode:** Consumers of an internally repair-enabled runner - undercount tokens, cost, and quota consumption whenever validation requires - repair. The discrepancy grows with the configured repair budget, while the - final content and validation metadata make the run appear complete. -- **Recommended direction:** Define run-level usage as the field-wise sum of - every completed generation response used by the operation, accumulate it - independently from the response that owns the final output, and clarify the - internal contract accordingly. Preserve exact single-call pass-through for - the public engine, which does not install a repairer. -- **Required verification:** Use distinct values in all five usage fields for - initial generation and each repair. Cover zero, one, and multiple repairs, - early successful repair, and an exhausted budget; require the final artifact - and raw output to come from the last response while usage includes every - completed call exactly once. - -#### S12-F03: Repair tests do not prove multi-attempt progression or early stop - -- **Category:** testing -- **Severity:** medium -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `internal/usecase/runner_test.go` - (`TestRunnerRunStructuredRepairRemainsBoundedAndUsesEffectiveModelSettings`, - `TestRunnerSchedulesInitialAndRepairGenerationThroughOneBackendPool`, and - repair fakes), plus `internal/usecase/runner.go` - (`shouldAttemptRepair` and the repair loop in `executePreparedRun`) -- **Contract at issue:** Repair is eligible only for failed JSON or JSON Schema - validation, must attempt no more than the requested budget, must number and - update attempts exactly, and must stop immediately after successful - validation. Retry behavior can invoke a paid external collaborator, so the - testing policy gives its boundaries and stop conditions priority. -- **Evidence:** Every maintained test that actually executes repair configures - `RepairAttempts` as one. The test named `RemainsBounded` proves that a budget - of one is not exceeded, and the successful repair integration reaches the - same limit at the same moment it becomes valid. No ordinary-run test uses a - budget greater than one, succeeds before a larger budget is exhausted, or - proves that attempt number, previous output, and validation errors advance - together. The focused coverage diagnostic reached only 71.4% of - `shouldAttemptRepair`; the percentage is only a locator, while the absent - behavioral cases establish the finding. -- **Failure mode:** A regression could continue making model calls after a - valid repair, stop too early after an invalid repair, fail to pass the latest - output and diagnostics, or mishandle a mode eligibility guard without a - focused test failure. Such a defect consumes extra tokens or returns invalid - output and is difficult to infer from the final result alone. -- **Recommended direction:** Replace or extend the existing one-attempt repair - case with a compact behavioral table that owns eligibility, progression, - early success, and exhaustion. Keep capacity-pool concurrency in its owning - integration test rather than expanding that already cross-cutting case. -- **Required verification:** Include initial success with zero repairs, - ineligible basic validation with a positive budget, success on an early - attempt below a larger maximum, and failure through the exact maximum. - Assert collaborator call count, `Attempt`, `MaxAttempts`, prior output, - current validation errors, final status, and reported attempts. A deliberate - extra or missing retry must fail the focused table. - -### Unresolved Observations - -None. The optional repair capability has no current production assembler: the -public engine calls `NewRunner`, which deliberately installs a nil repairer. -That bounds S12-F01 and S12-F02 to an internal path, but the path is documented, -implemented, and explicitly part of this audit stage. Initial successful-nil -LLM responses are rejected by the public adapter and are not produced by the -built-in client, so the internal runner's non-nil success assumption was not -promoted to a finding. - -### Coverage Ledger - -- **Transition order:** Ordinary execution creates timing and run identity, - resolves the established preparation inputs, admits once, defers release, - completes artifact loading and rendering, calls the model once initially, - builds one artifact from each candidate output, validates it, and enters - repair only after a completed failed content check. No implicit initial - generation retry exists. -- **Generation and errors:** The initial call receives the rendered messages, - normalized session, effective target, request-presence bits, direct - credential, and structured-output specification. Invalid model requests map - to `ErrInvalidRequest`; other generation failures retain collaborator and - context identity under `ErrLLMGenerate`. All such errors return no partial - result. Transport mechanics remain Stage 14 scope. -- **Validation and partial results:** No validation mode or a nil internal - validator produces a skipped valid result. Completed invalid content returns - a successful result containing the raw output, artifact, and failed - validation diagnostics. An operational validation error returns no result - and retains its cause under `ErrValidation`, as the public contract requires. - S11-F02 already owns unsupported modes reaching this late boundary, and - S10-F03 already owns the ordinary JSON Schema root's duplicate preparation - and validation reads. -- **Repair eligibility and state:** The implementation currently requires an - installed repairer, a positive budget, failed validation, and JSON or JSON - Schema mode. It increments before each call, passes the previous candidate - and current diagnostics, rebuilds and revalidates each repaired artifact, - stops after a pass, and returns the final failed result when the budget is - exhausted. S12-F03 records the missing larger-budget behavioral protection. -- **Repair request fidelity:** Effective target values, direct credential, - selected backend identity, normalized session, structured-output metadata, - attempt numbers, and validation mode reach the default repair path. The - second request intentionally replaces the original messages with a bounded - repair instruction. Target-presence metadata is the one lost execution - setting and is recorded as S12-F01. -- **Result construction and usage:** Final raw output, artifact, validation, - session, hashes, selected identities, effective credential-free settings, - input hashes, run ID, and UTC timing agree with the last accepted candidate. - Single-call usage is preserved exactly. Multi-call usage is replaced rather - than accumulated as S12-F02 records; operational failures intentionally - return no partial accounting result. -- **Admission errors and cleanup:** Capacity exhaustion at admission is - translated to an internal typed error carrying the already resolved backend - ID, while non-capacity and context errors retain their identities. Admission - failure skips completion, generation, validation, and repair. After a - successful admission, the immediate defer releases on completion, - generation, validation, repair, and successful exits. Ordinary-run tests - protect success plus representative completion, generation, and validation - failures; the shared execution helper's repair-failure release is also - exercised by the prepared suite, whose caller lifecycle remains Stage 13. - Permit queues and scheduling policy were not inspected. -- **Duplication and complexity:** Initial and repair generation legitimately - use different rendered prompts, but independently constructing their common - execution fields caused S12-F01. `executePreparedRun` is a linear state - machine whose artifact and validation repetition is localized inside the - bounded loop; splitting it solely by line count would make the transitions - harder to follow. The large runner test file is organized into focused - behavioral cases. The 90-line initial/repair capacity test is cross-cutting - because it protects one lease and one generation pool across both calls; - its scheduling assertions belong to Stage 15 and did not justify a separate - size-only finding here. -- **Test ownership:** Use-case tests own ordering, call counts, effective - request data, error categories, partial results, admission release, repair, - and result metadata. Root tests own public error mapping, one-pass public - validation, and consumer-visible result behavior. Validation packages own - content rules, while transport and capacity packages own their injected - mechanics. S12-F01, S12-F02, and S12-F03 identify the repair-specific - fidelity, accounting, and retry cases missing from that otherwise clear - ownership split. - -### Verification Performed - -The refreshed code knowledge graph inventoried the scoped symbols, traced the -ordinary `Run` path into `executePreparedRun`, confirmed the helper's two -callers, and located the repairer and capacity-error consumers. Every scoped -transition and focused ordinary-run test was then checked in source against -the public GoDoc, internal runner contract, policy documents, and Stage 10 and -11 handoffs. - -The following focused commands passed: - -```sh -go test ./internal/usecase -run 'TestRunner(RunSuccessful|RunPassesExtraParamsToGenerateRequestTarget|AdmissionFailureSkipsCompletionCollaborators|ReleasesAdmissionAcrossRunOutcomes|RunArtifactLoadFailure|RunPromptRenderFailure|RunLLMFailure|RunCancellationPreservesGenerationCategory|RunLLMInvalidRequestMapsToUsecaseInvalidRequest|RunValidationStillWorks|RunStructuredRepairRemainsBoundedAndUsesEffectiveModelSettings|SchedulesInitialAndRepairGenerationThroughOneBackendPool|RunRepairCarriesEffectiveSessionID|RunJSONSchemaRepairCarriesStructuredOutputSpec|RunJSONSchemaSchemaLoadFailureFailsBeforeLLM)$' -count=1 -go test . -run 'Test(RunSucceedsWithInjectedLLMClient|EngineRunWithDirectorySourcesAndFileInputs|RunPassesPreparedRequestToInjectedLLMClient|EngineRunPropagatesCallerCancellation|RunAddsLLMGenerateToCollaboratorPublicError|RunValidationFailureReturnsResult|EngineValidationIsSinglePass|CapacityExceededSentinelContract|MapPublicErrorTranslatesCapacityError)$' -count=1 -runner_audit_cover=/tmp/promptkit-runner-audit.cover -go test -coverprofile="$runner_audit_cover" ./internal/usecase -go tool cover -func="$runner_audit_cover" -rm "$runner_audit_cover" -``` - -The repository-wide `go test ./...`, `go test -race ./...`, `go vet ./...`, -and `go run ./examples/go-library/prepare` checks also passed. The coverage -diagnostic reported 90.3% statement coverage for `internal/usecase`; it was -used only to locate unexercised decisions and not as finding evidence by -itself. - -A temporary package probe, removed before this artifact was edited, confirmed -that: - -- explicit zero temperature, maximum-token, and top-p overrides reached - initial generation with presence bits set but reached the default repair - generation with every presence bit clear; and -- an initial response reporting 11 total tokens followed by a successful - repair reporting 7 produced a run result reporting only 7. - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - ordinary-execution review. -- Stage 13 owns the prepared handle's claim, discard, retained credential, - frozen validation plan, timing start, and concurrent lifecycle. It should - treat the shared generation, repair, validation, and result transitions - recorded here as established rather than reopening them. -- Stage 14 owns transport URL construction, timeout conversion, provider - payload encoding, response decoding, and wire-level use of already prepared - target-presence and structured-output values. S12-F01 establishes the - earlier use-case loss before that transport boundary. -- Stage 15 owns admission and active-generation pool mechanics, fairness, - queue cancellation, and permit accounting. This stage established only the - ordinary runner's translation and defer boundaries. -- Stage 17 can consider the common initial/repair generation-request fields - identified by S12-F01 together with other confirmed consolidation work; it - should retain the intentionally different prompt construction and should not - combine request building merely for visual similarity. - -## Stage 13: Prepared Execution Lifecycle - -### Scope Reviewed - -The review covered `internal/usecase/prepared_execution.go`, its focused -use-case tests, the prepared-handle facade in `prepared_execution.go` and -`engine.go`, and the prepared-execution cases in -`prepared_execution_contract_test.go`. Public GoDoc, the internal runner -contract, the consumer guide, and the Stage 10 through 12 handoffs supplied -the frozen-plan and shared-execution invariants. Source resolution, validation -implementation, ordinary execution internals, provider transport, and -capacity scheduling were treated as established or left to their owning -stages. - -The code graph was refreshed and used first to bound the lifecycle. Preparation -creates two independent snapshots: one private payload for execution and one -credential-free value for `Details`. A single mutex then arbitrates `claim` -and `Discard`; the winning transition detaches the execution payload before -releasing the lock. Every valid owning run defers payload cleanup before later -credential, admission, generation, or validation work. - -### Accepted Findings - -#### S13-F01: Formatting a copied prepared handle reveals its internal representation - -- **Category:** contract-documentation consistency -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `prepared_execution.go` (`PreparedExecution.String` and - `PreparedExecution.GoString`) and - `prepared_execution_contract_test.go` - (`TestPreparedExecutionDiscardAndFormattingDoNotExposePrivateState`) -- **Contract at issue:** `PreparedExecution` is documented as an opaque value - that callers may copy, with every copy sharing one lifecycle. Diagnostic - formatting must not expose the private use-case handle, its address, or the - retained execution state merely because the caller formats a value copy - instead of the pointer originally returned by `PrepareExecution`. -- **Evidence:** Both redaction methods have pointer receivers. Consequently, - `fmt` uses them for `*PreparedExecution` but not for an addressable or - non-addressable `PreparedExecution` value passed through an interface. A - temporary external-package probe copied a live handle and observed `%v` as - `{0x...}`, `%+v` as `{internal:0x...}`, and `%#v` as - `promptkit.PreparedExecution{internal:(*usecase.PreparedExecution)(0x...)}`. - Formatting the original pointer produced only - `promptkit.PreparedExecution{opaque}`. The maintained formatting test checks - `%v`, `%+v`, and `%#v` only on the pointer, despite the same test suite - documenting value-copy behavior. This also disproves the Stage 1 coverage - ledger's statement that copied handles always format opaquely; that ledger - statement was not itself a tested contract. -- **Failure mode:** Logging a copied public handle discloses an internal package - type, field name, and process address. Current formatting did not expose the - retained direct credential or rendered content, but it violates the opaque - boundary and makes ordinary diagnostic output depend on whether a handle - was copied. -- **Recommended direction:** Give both pointer and value formatting paths one - constant opaque representation without dereferencing nil pointers or - exposing fields. Keep formatting behavior independent of lifecycle state; - formatting must not claim, discard, or inspect the private payload. -- **Required verification:** Exercise original pointers, copied values, zero - values, and nil pointers with `%v`, `%+v`, `%#v`, and string-oriented - formatting. Require the same opaque representation where the format permits, - prohibit internal type names, addresses, credentials, and content sentinels, - and prove that formatting leaves the handle usable or discarded exactly as - it was beforehand. - -### Unresolved Observations - -None. No medium- or low-confidence lifecycle concern was promoted to a -finding. In particular, a shallow copy of the public `Engine` retains the same -private runner identity and is therefore the same logical owner for this -purpose; the owner comparison does not make a separately assembled engine -interchangeable. - -### Coverage Ledger - -- **Preparation and freezing:** `PrepareExecution` completes source loading, - rendering, target resolution, output-contract derivation, and validation - preparation without admission or generation. The private execution snapshot - freezes messages, input hashes, target values and presence, session, - structured-output schema, validation plan, selected backend identity, and - the credential source. The public details snapshot is cloned separately and - omits direct credentials. Environment credentials intentionally freeze the - variable name rather than its value and are rechecked at execution. -- **Claim and single attempt:** `claim` first rejects nil and foreign handles - without consuming them, then locks and permits only the ready state with a - live payload. The winning call changes state to claimed and detaches the - payload atomically, so copied handles and concurrent callers share exactly - one attempt. A valid owning attempt is consumed before credential checking, - admission, generation, validation, or cancellation can fail. A nil public - engine is rejected before unwrapping and therefore does not claim the - handle. -- **Discard and private-state release:** `Discard` is nil-safe, uses the same - mutex as claim, and changes only a ready handle to discarded. It detaches - under the lock and clears outside it; losing a race to claim is a no-op and - does not cancel the active execution. The detached payload clears the - retained direct-key fields and drops its execution and validation - references. Every successful claim installs this cleanup before subsequent - exits, including missing credentials, capacity rejection, collaborator - failure, validation failure, and success. -- **Admission and timing:** A claimed run revalidates credential availability, - admits the already frozen backend exactly once, and defers release - immediately after admission. Timing and run identity begin after claim, so - preparation time and time spent holding an unclaimed handle are excluded; - admission and execution time are included. Capacity-policy mechanics remain - Stage 15 scope. -- **Execution, context, and errors:** The execution context is independent of - the preparation context and flows into credential lookup, admission, - generation, and validation. Prepared runs delegate to the same - `executePreparedRun` state machine as ordinary runs, preserving the shared - generation, artifact, validation, repair, and result behavior established in - Stage 12. Invalid handles and wrong owners return the prepared-handle error; - collaborator, capacity, validation, and context identities retain their - public mappings, and failures return no partial result. -- **Returned ownership:** `Details` takes a stable reference under the lifecycle - lock and returns a fresh deep clone on every call, before or after claim or - discard. Execution uses a separate clone, and outward result conversion - creates another caller-owned value. Mutating caller inputs, one details - value, or one result therefore cannot alter the frozen run or another - returned snapshot. -- **Concurrency:** The claim/discard state transition contains no blocking - collaborator work while holding the mutex. Focused race tests prove one - generation across concurrent claims and one winner across repeated - run/discard races. Details reads are synchronized only while acquiring the - immutable details reference, avoiding a race with lifecycle transitions - without serializing deep copying behind the state lock. -- **Private formatting:** Pointer formatting is constant and does not inspect - lifecycle state or retained fields. S13-F01 records the value-formatting - escape caused by pointer-only formatter methods; JSON encoding remains an - empty object because the handle has no exported fields. -- **Complexity and duplication:** The lifecycle implementation is a compact - state machine with one synchronization owner and one cleanup helper; no - duplicate claim or release policy was found. Internal tests own state - transitions, private payload retention, collaborator ordering, and admission - release. External-package tests own public engine binding, copy semantics, - error identity, frozen snapshots, timing, and races. Similar assertions at - both layers terminate at different contracts and are not removable semantic - duplication. Several external cases are long because they exercise a - cohesive cross-boundary lifecycle; apart from the missing value-formatting - matrix in S13-F01, splitting them by line count would not materially improve - behavioral ownership. - -### Verification Performed - -The refreshed code knowledge graph located the prepared facade and internal -state machine, traced both callers of the shared execution helper, and mapped -the focused internal and external tests. Each claim, discard, cleanup, -admission, context, snapshot, and formatting conclusion was then confirmed -against source and the applicable public and internal contracts. - -The following focused race-enabled and repeated commands passed: - -```sh -go test -race ./internal/usecase -run 'PreparedExecution' -count=25 -go test -race . -run '^TestPreparedExecution' -count=25 -``` - -A temporary external-package probe, removed before this artifact was edited, -formatted a live handle both as the returned pointer and as a copied value. It -confirmed the internal-type and address exposure recorded in S13-F01 while -also confirming that the retained credential and generated content were not -printed by the current representation. - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - prepared-lifecycle review. -- Stage 14 owns provider request encoding and response handling. It should - treat the prepared target, presence metadata, credential, messages, session, - and structured-output value reaching the LLM boundary as frozen inputs. -- Stage 15 owns capacity queueing, fairness, cancellation, and permit - accounting. This stage established only that prepared execution admits once - after claim and releases every acquired lease. -- Stage 17 may consider test-file organization only alongside broader - confirmed complexity evidence. The lifecycle and public tests currently - overlap at intentional package boundaries, so this stage found no standalone - consolidation work beyond the formatting regression coverage in S13-F01. - -## Stage 14: OpenAI-Compatible Transport - -### Scope Reviewed - -The review covered `internal/llm/client.go`, -`internal/llm/openai_compatible_client.go`, the complete focused transport -suite in `internal/llm/openai_compatible_client_test.go`, and the root tests -that send resolved settings through the built-in client. The durable -OpenAI-compatible integration contract and internal model-client document -supplied the wire and error expectations. Target resolution, prepared-handle -lifecycle, repair coordination, and capacity scheduling were treated as -established inputs from their owning stages. - -The refreshed code graph bounded the implementation to one constructor, one -outbound `Generate` state machine, and three request-mapping helpers. Source -inspection and temporary local probes then followed every preflight, request, -transport, status, decode, and response-validation exit. No live provider was -contacted. - -### Accepted Findings - -#### S14-F01: The built-in transport discards cancellation and deadline identities - -- **Category:** correctness -- **Severity:** high -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/llm/openai_compatible_client.go` - (`OpenAICompatibleClient.Generate`), - `internal/llm/openai_compatible_client_test.go` - (`TestOpenAICompatibleClientCancellationReturnsRequestFailure` and deadline - tests), and `engine_test.go` - (`TestEngineRunPropagatesCallerCancellation`) -- **Contract at issue:** Caller cancellation is an outer execution boundary. - Transport failures must retain both the model-request category and an - underlying cancellation or deadline identity so callers can use - `errors.Is`, as the public operation boundary and S04-F01 require. -- **Evidence:** When `http.Client.Do` fails, `Generate` constructs - `fmt.Errorf("%w: %v", ErrRequestFailed, err)`. Only `ErrRequestFailed` is - wrapped; the `*url.Error` and its context cause are flattened into text. - Higher use-case and facade layers wrap their input correctly, but they - cannot restore the discarded cause. A temporary package probe canceled the - request context and observed `errors.Is(err, ErrRequestFailed) == true` and - `errors.Is(err, context.Canceled) == false`. The focused cancellation test - checks only the package category and error text, while the root ordinary-run - test checks only `ErrLLMGenerate`. Prepared execution's public cancellation - assertion uses an injected client and therefore does not exercise this - transport. -- **Failure mode:** Consumers using the built-in client cannot reliably - distinguish their own cancellation, caller deadlines, generation - deadlines, or transport timeouts from other provider failures. Retry, - observability, and shutdown logic may misclassify an intentionally aborted - operation as a remote failure. -- **Recommended direction:** Preserve the underlying `http.Client.Do` error - in the chain while retaining `ErrRequestFailed` and the higher public - generation category. Do not expose request headers or provider bodies in - the added context. -- **Required verification:** At the package boundary, require - `ErrRequestFailed` together with `context.Canceled` and - `context.DeadlineExceeded` for caller cancellation, caller deadline, - generation deadline, and whole-request client timeout as applicable. Extend - the existing public ordinary-run cancellation case from S04-F01 to require - both `ErrLLMGenerate` and the context identity; retain the injected prepared - case as a separate adapter contract. - -#### S14-F02: Large positive generation timeouts wrap into expired deadlines - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/llm/openai_compatible_client.go` - (`OpenAICompatibleClient.Generate`), target validation in - `internal/usecase/runner.go`, and timeout-boundary tests in - `internal/llm/openai_compatible_client_test.go` -- **Contract at issue:** `TimeoutSeconds` accepts a non-negative `int`; zero - disables the generation-specific deadline and a positive value adds that - many seconds. Converting an accepted positive value must not silently create - an unrelated duration or a deadline in the past. -- **Evidence:** Generation converts with - `time.Duration(req.Target.TimeoutSeconds) * time.Second` without checking - multiplication overflow. On the current 64-bit build, a temporary probe - supplied `math.MaxInt`; the transport observed a deadline approximately one - second before the call instead of a far-future deadline. Preparation checks - only for negative values, and maintained deadline tests use values from one - through five seconds. This is the outbound counterpart of the duration - conversion class already recorded for JSON in S02-F01, not the writable - default issue in S06-F01. -- **Failure mode:** A syntactically valid large timeout can cancel generation - immediately or wrap to an arbitrary shorter duration. The prepared or - ordinary operation then fails despite requesting a positive deadline, and - its timing behavior depends on integer width and the wrapped value. -- **Recommended direction:** Validate that seconds can be represented as a - `time.Duration` before multiplication and reject out-of-range values as an - invalid generation request. Keep zero and every representable positive - value unchanged. -- **Required verification:** Cover zero, one ordinary positive value, the - largest safely representable second count, its first out-of-range neighbor, - and the platform maximum `int`. Require correct deadline placement for valid - values and `ErrInvalidRequest` before transport invocation for invalid ones. - -#### S14-F03: String-based endpoint composition corrupts query-bearing base URLs - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/llm/openai_compatible_client.go` - (`NewOpenAICompatibleClient` and `OpenAICompatibleClient.Generate`), endpoint - target validation in `internal/usecase/profile_inspection.go`, and endpoint - tests in `internal/llm/openai_compatible_client_test.go` -- **Contract at issue:** The effective endpoint is a provider base URL and the - request must target that base path plus `/chat/completions`. Invalid base - URL shapes must fail before a provider call rather than move the completion - path into a query or fragment. -- **Evidence:** The constructor accepts every value that - `url.ParseRequestURI` parses, while resolved profile and request endpoints - are checked only for nonblank text. Generation then appends the completion - path to the raw string. A temporary `httptest` probe configured - `/v1?route=blue`; construction and generation succeeded, but the - server received path `/v1` and query `route=blue/chat/completions` instead of - path `/v1/chat/completions`. A fragment similarly captures the appended - suffix client-side. The backend registry rejects query, fragment, user-info, - non-HTTP, relative, and hostless endpoints, but endpoint-only profiles and - request overrides bypass that validator. Existing transport tests cover only - syntactically invalid configuration and ordinary absolute endpoints. -- **Failure mode:** A consumer-supplied endpoint can send a valid request to - the wrong provider route, potentially carrying an unintended query value, - and then surface a misleading provider response or status error. Relative - and unsupported-scheme URLs are likewise accepted too early and later - reported as request-execution failures instead of invalid input. -- **Recommended direction:** Give selected transport base URLs one structural - validation and composition rule: require an absolute HTTP or HTTPS URL with - a host and no user information, query, or fragment, then append the - completion path through parsed URL fields. Preserve endpoint override - precedence and trailing-slash normalization. -- **Required verification:** Exercise configured and per-request endpoints - with HTTP and HTTPS, nested paths, repeated trailing slashes, query, - fragment, user information, relative paths, missing hosts, and unsupported - schemes. Require valid paths to reach `/chat/completions` exactly once and - invalid values to fail before the transport is called with the appropriate - configuration or request identity. - -#### S14-F04: Successful provider responses have no byte limit - -- **Category:** correctness -- **Severity:** high -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/llm/openai_compatible_client.go` - (`OpenAICompatibleClient.Generate`) and success/malformed-response tests in - `internal/llm/openai_compatible_client_test.go` -- **Contract at issue:** Provider responses are an external, untrusted byte - stream. Reading and decoding them must have a fixed resource boundary that - is independent of how quickly the remote endpoint can send data; a deadline - alone does not bound memory consumption. -- **Evidence:** Non-success handling explicitly copies at most 4 KiB to a - discard sink, but every 2xx body is passed directly to `json.Decoder`. - Response content is decoded into a string and choices slice with no - `Content-Length` check, limiting reader, or decoder ceiling. The client - timeout bounds elapsed time only. A temporary local provider returned a - valid response containing a 2 MiB content string, which was fully accepted; - source tracing shows no finite size at which reading stops. Maintained tests - contain only small response literals. -- **Failure mode:** A faulty or hostile configured provider can force a run to - allocate and process memory proportional to an arbitrarily large 2xx body, - up to process exhaustion within the transport deadline. Concurrent requests - multiply the exposure. -- **Recommended direction:** Define a documented, application-neutral maximum - provider response size and enforce it while decoding, including responses - without `Content-Length`. Reject a body that crosses the boundary with a - stable malformed-response or request-failure identity, close it on every - exit, and avoid first materializing a second full byte copy. -- **Required verification:** Test just-below, exact-limit, and one-byte-over - bodies through a streaming reader with and without `Content-Length`, plus a - continuing oversized stream. Assert bounded bytes read, no partial result, - the selected error identity, timely return after rejection, and body closure - on success and every failure path. - -#### S14-F05: A valid JSON prefix hides trailing malformed response data - -- **Category:** correctness -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/llm/openai_compatible_client.go` - (`OpenAICompatibleClient.Generate`) and malformed-response tests in - `internal/llm/openai_compatible_client_test.go` -- **Contract at issue:** A successful provider body must be one valid - OpenAI-compatible JSON document. Invalid JSON is a malformed response; a - valid object followed by arbitrary non-whitespace bytes is not a valid - document. -- **Evidence:** `Generate` calls `json.Decoder.Decode` exactly once and never - verifies end of input. A temporary `httptest` provider returned a valid - choice object followed by ` trailing`; generation returned content `ok` and - a nil error. The maintained invalid-JSON test starts with malformed syntax, - so it does not exercise a valid prefix, a second JSON value, or trailing - garbage. -- **Failure mode:** Truncated framing, proxy corruption, accidental - concatenation, or a provider emitting multiple documents is silently - accepted. Consumers receive a successful result even though the wire body - violates the declared response format, and unread trailing bytes can also - prevent efficient connection reuse. -- **Recommended direction:** After decoding the one expected response object, - require that the bounded stream contains only permitted trailing whitespace - and then EOF. Classify any second value or non-whitespace suffix as - `ErrMalformedResponse` without including provider content in the error. -- **Required verification:** Retain ordinary invalid JSON and add valid JSON - followed by whitespace, non-whitespace garbage, and a second JSON value. - Require only the whitespace case to succeed, preserve response-body - redaction, and assert closure for all outcomes. - -#### S14-F06: Repeated HTTP scaffolding obscures the transport behavior matrix - -- **Category:** testing -- **Severity:** low -- **Confidence:** high -- **Status:** accepted -- **Affected code:** `internal/llm/openai_compatible_client_test.go`, especially - `TestOpenAICompatibleClientGenerateSuccess` and the request-serialization, - response, and authentication tests -- **Contract at issue:** Protocol tests should make wire-visible cases and - failure boundaries easy to inventory while keeping realistic `httptest` - coverage. Repeating transport mechanics should not make adding a boundary - case disproportionately expensive or hide which cases are absent. -- **Evidence:** The file is 1,192 lines across 32 tests and creates 17 local - servers. At least ten handlers repeat the same successful choice literal, - and eight independently decode the request into a generic map before making - field assertions. The 133-line baseline success case combines method, path, - content type, authentication, ordinary messages, numeric fields, service - tier, structured output, response content, and usage. Despite that volume, - no maintained case covers the five reproduced boundaries in S14-F01 through - S14-F05, response-body closure, empty first-choice content, or invalid - structured-output defensive branches. The diagnostic coverage run reported - 92.2% statements; the percentage is only a locator, while the repeated - setup and enumerated missing behaviors establish this finding. -- **Failure mode:** Adding or diagnosing a single wire rule requires navigating - substantial repeated setup, while a broad baseline failure gives weak - localization. Mechanical edits to response literals and server construction - can drift, and the suite can appear exhaustive by size while omitting - consequential framing and resource cases. -- **Recommended direction:** Introduce one small recording-provider helper and - organize focused tables around request mapping, authentication, endpoint - construction, timeout/error identity, and response framing. Keep specialized - transports for deadlines, cancellation, byte counts, and body closure, and - retain direct assertions of durable wire fields rather than snapshotting a - whole payload. -- **Required verification:** Demonstrate that every existing semantic - assertion still runs, that each subtest reports its protocol case directly, - and that deliberate mutations to method/path, headers, omission/presence, - reserved fields, response mapping, timeout precedence, and error redaction - each fail an owning case. Run the reorganized suite normally, repeatedly, - and under the race detector. - -### Unresolved Observations - -None. `openAIChatRequestPayload` currently marshals each extra-parameter value -once for invalid-request classification and later marshals the complete body. -That is an additional traversal and transient allocation proportional to the -extra-parameter payload, but provider calls dominate the ordinary path and no -representative cost measurement established material impact, so it was not -promoted to an efficiency finding. The final request marshal remains necessary -regardless. - -### Coverage Ledger - -- **Construction and ownership:** The constructor trims and stores its base, - defaults non-positive configured timeouts, shallow-clones a supplied - `http.Client`, preserves a positive supplied client timeout, and never - mutates the caller's client. Sharing its transport, jar, redirect policy, - and other collaborator pointers is the expected `http.Client` copy - behavior. The writable timeout default remains S06-F01. -- **Endpoint and method:** Generation selects a nonblank target endpoint before - the configured base, removes trailing slashes, appends the completion path, - builds one HTTP `POST`, and sets `Content-Type: application/json`. S14-F03 - records structural validation and query/fragment composition failures; - backend registration's narrower valid endpoint source does not protect - endpoint-only profiles or request overrides. -- **Authentication and private data:** A nonblank direct credential wins over - environment lookup; otherwise a configured environment name is read at - generation and must contain a nonblank value. The selected value appears - only in the Bearer header, while no Authorization header is sent without - either source. Backend identity, environment-variable names, and credential - values are absent from the JSON body. Non-success errors include only the - status code, and focused tests prove provider-body suppression. -- **Request body:** Model and messages are always emitted. Ordinary messages - use string content, while cache-controlled messages use one text block with - the cache-control type and optional TTL. Session ID is normalized and sent - only at the top level. Service tier and reasoning are omitted when empty; - target-presence bits preserve explicit numeric zero versus omission. - JSON-Schema structured output maps its type, name, strict flag, and schema, - and no response format is emitted without a spec. -- **Extra parameters:** The effective map is flattened into the top-level body. - Empty keys, every owned wire-field collision, and non-serializable values - fail as invalid requests before transport; backend registration consumes the - same LLM-owned reserved-field predicate. Public conversion and resolution - own JSON-tree validation and copying, so the transport does not need another - deep copy. The repeated serialization observation is bounded above and was - not accepted as a performance finding. -- **Deadlines and cancellation:** A positive per-generation timeout derives a - child context, zero leaves the caller context unchanged, and a negative - value fails before transport. The cloned `http.Client` supplies the - whole-request cap, and source plus deadline-capturing tests establish that - the earliest caller, generation, or client deadline controls. S14-F02 owns - overflow in seconds conversion; S14-F01 owns loss of context error identity - after an applicable deadline or cancellation fires. -- **HTTP completion and cleanup:** Every successful `Do` immediately installs - a deferred response-body close, which dominates non-2xx, decode, malformed, - and success exits. Non-2xx handling reads at most 4 KiB and never publishes - the bytes. The maintained suite does not directly observe `Close`; the - streaming regression protection required by S14-F04 and S14-F05 should own - that resource assertion rather than adding a parallel close-only workflow. -- **Successful responses:** Any 2xx status enters decoding. The first choice's - nonempty content is returned, later choices are ignored, and prompt, - completion, total, cached, and cache-write token counts map directly; - absent usage fields remain zero. Empty choices and empty first-choice - content are rejected in source, although only the former has a focused test. - S14-F04 and S14-F05 record the missing byte and document boundaries. -- **Errors and retries:** Invalid configuration, invalid requests, request - execution, non-success status, and malformed response retain distinct LLM - sentinels. No transport retry exists, so one generation call makes at most - one HTTP request. S14-F01 records the underlying context identity discarded - inside the request-failure category; provider status bodies remain safely - redacted. -- **Test ownership:** Internal LLM tests own exact HTTP method, URL, headers, - body omission and encoding, timeout layering, response mapping, package - errors, closure, and framing. Root tests appropriately own only resolved - backend fields reaching the built-in transport, reserved-field rejection at - the public operation boundary, and framework/caller timeout composition. - Those cross-layer assertions are not duplicates of transport mechanics. - S14-F06 records the mechanical duplication within the owning transport file. - -### Verification Performed - -The code knowledge graph inventoried every production and test symbol in -`internal/llm`, identified `Generate` and construction as the only transport -hotspots, and traced the generation boundary back to engine assembly and -ordinary/prepared execution. All important graph conclusions were confirmed -against source, GoDoc, the integration contract, the internal model-client -document, and the Stage 12 and 13 handoffs. - -The focused package suite passed with 92.2% statement coverage; coverage was -used only to locate unexercised response and defensive branches: - -```sh -llm_audit_cover=/tmp/promptkit-llm-audit.cover -go test -coverprofile="$llm_audit_cover" ./internal/llm -go tool cover -func="$llm_audit_cover" -``` - -The following focused repeated and race-enabled checks and the repository-wide -ordinary suite also passed: - -```sh -go test -race ./internal/llm -count=10 -go test -race . -run 'Test(EngineRunPropagatesCallerCancellation|RunRejectsReservedExtraParamsBeforeProviderCall|RunUsesResolvedBackendWithBuiltInLLMClient|EngineRunLayersTransportAndGenerationTimeouts)$' -count=3 -go test ./... -``` - -Temporary package probes, removed before this artifact was edited, confirmed -that: - -- a canceled outbound request matched `ErrRequestFailed` but not - `context.Canceled`; -- `math.MaxInt` timeout seconds produced a deadline approximately one second - in the past; -- a query-bearing base sent the completion suffix as query data while - returning success; -- a valid response with a 2 MiB content string was fully accepted; and -- a valid response object followed by non-JSON text returned content and no - error. - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - transport review. -- Stage 15 should treat each call into the client as one non-retrying outbound - operation. Capacity wrapping must preserve the context identities required - by S14-F01 but does not own URL, body, or response mechanics. -- Stage 16 should use this stage's test-ownership map rather than duplicating - the provider-wire matrix in root consumer workflows. S14-F06 is the scoped - candidate for simplifying the transport suite while retaining its - `httptest` boundary. -- Stage 17 may reconsider extra-parameter validation marshaling only if a - representative benchmark or payload model establishes material cost; this - stage found no standalone efficiency defect there. - -## Stage 15: Capacity, Admission, And Generation Scheduling - -### Scope Reviewed - -The review covered all production and test code in `internal/capacity`, the -capacity assembly in `NewEngine`, ordinary and prepared admission in -`internal/usecase`, the root capacity contract suite, and the prepared-capacity -contract case. The internal capacity document supplied the durable ownership, -bounded-admission, FIFO, cancellation, and release expectations. Backend -normalization, ordinary and prepared execution semantics, provider transport, -and public error mapping were treated as established inputs from their owning -stages except where they integrate directly with capacity. - -The refreshed code graph bounded the scheduler to one engine-local `Manager`, -one immutable map of limited-backend pools, one immediate admission operation, -and one model-client wrapper. Source synchronization then followed every -admission and active-permit state transition under its pool mutex, including -the grant/cancel race and panic unwinding, and traced the exact manager instance -through engine construction, both runner entry points, initial generation, and -the internal repair integration. - -### Accepted Findings - -None. The implementation matched the documented capacity invariants, and the -focused ordinary, race-enabled, repeated, and coverage-guided checks did not -establish a correctness, security, maintainability, or material efficiency -defect. - -### Unresolved Observations - -None. The only uncovered statement branch in the focused package coverage run -was `pool.acquire` receiving an already-canceled context before it can reserve -or enqueue. Source proves that branch returns the context error while holding -the same lock used for all state changes, and assembled limited-backend runs -normally reject that state at the preceding admission check. Adding a root -test for the internal defensive branch would duplicate the owning package -boundary without protecting a distinct consumer contract. - -### Coverage Ledger - -- **Construction and ownership:** `NewEngine` obtains a copied policy snapshot - from its immutable backend registry, constructs one new manager, and passes - that same instance to the capacity client and runner. Each manager creates - fresh pools and owns no global state, goroutine, timer, worker, shutdown - hook, or persistence. Concurrent map access is read-only after construction, - registered unlimited backends and endpoint-only profiles have no pool, and - separately constructed engines cannot share counters or waiter lists. -- **Policy arithmetic and admission:** Construction rejects blank policy IDs, - nonpositive concurrency, negative queue capacity, and addition overflow - before storing immutable limits. A limited admission locks its pool, checks - context cancellation, compares `admitted` with - `concurrencyLimit + queueCapacity`, and increments at that linearization - point or returns only `ErrCapacityExceeded`. The returned closure uses - `sync.Once`, so concurrent or repeated release cannot underflow the count. - Missing pools take the unrestricted path without allocating coordination - state. -- **Run boundaries and lease lifetime:** Ordinary execution resolves the prompt, - profile, backend, effective target, credential requirements, and output - contract before admission, then installs the release defer before schema, - artifact, render, generation, validation, or repair work. Preparation alone - does not admit. `RunPrepared` first atomically claims the handle and rechecks - credentials, then admits the frozen backend and installs the same defer - before execution. Thus every successful, failing, canceled, or panicking - exit unwinds one whole-run lease, while an internal repair remains part of - its original admission. -- **Active permits and backend selection:** The wrapper selects exactly one - pool from the request's resolved backend ID. Unlimited requests pass the - original context and request directly to the collaborator. Limited requests - increment `active` only below the immutable limit and defer release before - invoking the collaborator, preserving the exact response and error on - ordinary returns and restoring the permit during panic unwinding. Endpoint - overrides retain their selected backend ID. The owning internal repair - integration gives the runner and default repairer the same wrapper, so its - initial and repair calls use one pool while releasing the active permit - between calls. -- **FIFO and cancellation:** A contended call appends one waiter to a - mutex-protected `container/list`. New arrivals cannot bypass existing - waiters. Release removes the front waiter, marks the grant while still under - the lock, and transfers the existing active count directly before closing - the ready channel outside the lock. Cancellation uses that same lock either - to unlink its still-ungranted element and return the context error or to - observe that the grant won and invoke the collaborator with the original, - now-canceled context. These mutually exclusive transitions prevent lost or - double permits and leave no canceled waiter retained by a pool. -- **Lock scope and resource behavior:** Admission and scheduling critical - sections contain only context inspection, integer comparisons and updates, - and constant-time list operations. Collaborator calls, channel waits, - channel close, payload preparation, validation, and repair occur outside - pool locks. Capacity creates no internal goroutines or timers; one waiter - object and channel are allocated only for an actually contended limited - call. Direct FIFO handoff plus cancellation removal prevents later work from - starving behind abandoned entries, and per-backend locks avoid unrelated - backend contention. -- **Test ownership:** Manager tests own policy validation, immutable snapshot - behavior, admission bounds, backend independence, idempotent release, - context handling, and unrestricted paths. Client tests own exact peak - limits, FIFO order, first and middle waiter cancellation, grant/cancel races, - pool independence, pass-through identity, and panic release. Use-case tests - appropriately own admission ordering and lifetime plus shared - initial/repair scheduling; root tests own assembled queue rejection, - endpoint identity, injected-client limits, unlimited concurrency, public - capacity identity, and engine independence. Prepared tests own deferred - admission and execution-time release. These are relational assertions at - distinct boundaries rather than duplicate scheduler implementations. - -### Verification Performed - -The code knowledge graph was refreshed and used first to inventory every -capacity symbol, identify the manager and wrapper as the only stateful -hotspots, and trace their callers through engine construction and ordinary and -prepared execution. All graph conclusions were checked against source, the -capacity document, architecture and testing policies, and the Stage 12 through -14 handoffs. - -The focused package suite reported 97.3% statement coverage; coverage was used -only to locate the defensive pre-canceled-acquire branch discussed above: - -```sh -capacity_audit_cover=/tmp/promptkit-capacity-audit.cover -go test -coverprofile="$capacity_audit_cover" ./internal/capacity -go tool cover -func="$capacity_audit_cover" -``` - -The following focused ordinary, race-enabled, and repeated checks also passed: - -```sh -go test ./internal/capacity ./internal/usecase . -run 'Capacity|Admission|SchedulesInitialAndRepairGenerationThroughOneBackendPool|ReleasesAdmission|UnlimitedBackends|PreparedExecutionCredentialCapacity' -go test -race ./internal/capacity ./internal/usecase . -run 'Capacity|Admission|SchedulesInitialAndRepairGenerationThroughOneBackendPool|ReleasesAdmission|UnlimitedBackends|PreparedExecutionCredentialCapacity' -go test ./internal/capacity ./internal/usecase . -count=50 -run 'Capacity|Admission|SchedulesInitialAndRepairGenerationThroughOneBackendPool|ReleasesAdmission|UnlimitedBackends|PreparedExecutionCredentialCapacity' -``` - -The repository-wide ordinary and race-enabled suites and the maintained -offline consumer workflow passed as well: - -```sh -go test ./... -go test -race ./... -go run ./examples/go-library/prepare -``` - -### Handoff - -- The Stage 0 baseline remains absent and was not backfilled during this - concurrency review. -- Stage 16 can treat admission, backend selection, and generation scheduling - as one engine-local boundary with no accepted defect. Consumer tests should - assert only visible capacity behavior and should not reproduce mutex or - waiter-list mechanics. -- Stage 17 has no capacity-specific optimization candidate from this review. - Any future change to fairness or allocation strategy should first preserve - the current linearization points and be justified by representative - contention measurements. - -## Stage 16: Repository-Wide Test Strategy And Maintained Examples - -### Scope Reviewed - -The suite-level review covered all 25 `_test.go` files and their 281 top-level -tests, every package-local and repository-level fixture, the external-package -root contracts, `architecture_test.go`, and both maintained programs and prompt -files under `examples/go-library`. The testing policy supplied the -risk-oriented sufficiency standard. The architecture, documentation, consumer, -format, release, and internal-overview documents supplied the repository and -example boundaries. Component behavior was not reopened: the Stage 1 through -15 coverage ledgers were used to assign owners and identify already accepted -gaps. - -The refreshed code graph mapped test symbols and `TESTS` edges across the -public facade and internal packages. Source inspection then checked the suite -for exact error strings, private-helper and constant coupling, fixed ports, -live dependencies, environment and filesystem state, timers, sleeps, fixture -mutation, golden updates, and helper complexity. Coverage and timing directed -attention only; findings below are based on the maintained workflow contract -and one reproduced environment-dependent failure. - -### Accepted Findings - -#### S16-F01: Maintainer validation never executes the maintained Run example - -- **Category:** testing -- **Severity:** low -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `docs/policy/testing.md` and `docs/release.md` (maintainer - validation commands), `docs/development.md` (the assigned contributor-command - owner), and `examples/go-library/run/main.go` plus its `prompt.yaml` -- **Contract at issue:** Both Go-library examples are maintained, deterministic, - offline downstream workflows. The execution example is intentionally - separate because it protects assembled `Run`, injected-client, validation, - usage, and result behavior that the preparation example does not exercise. - Maintainer validation should execute each distinct maintained consumer - artifact whose fixture is otherwise only compiled or ignored. -- **Evidence:** The internal overview and consumer guide name both examples as - maintained offline workflows, but the testing-policy and release command - lists invoke only `go run ./examples/go-library/prepare`. `go test ./...` - reports both example packages as having no test files; diagnostic coverage - reports 0.0% for each, so it compiles `run/main.go` but never opens - `run/prompt.yaml` or invokes the engine. Directly running - `go run ./examples/go-library/run` from the repository root completed in - under a second and returned the documented deterministic result. The - preparation command also passed, confirming that the two programs are - independently runnable rather than alternate entry points to one fixture. -- **Failure mode:** The execution example's relative path, prompt format, - in-memory profile, injected-client adaptation, runtime validation, output - projection, or fixture can drift while every required maintainer command - still passes. A consumer copying the advertised complete example discovers - the breakage after release. Root contracts continue to protect library - behavior, but they do not execute this maintained artifact or its file. -- **Recommended direction:** Add the execution example to the canonical local - maintainer-validation workflow and synchronize documents that currently - present that workflow without creating a second independent command owner. - Keep both examples separate: their preparation and execution outcomes are - distinct and their small duplicated prompt fixtures make each program - independently copyable. -- **Required verification:** Run both example commands from the repository - root without credentials or network access, require successful exit and - stable semantic fields for their respective prepared and executed outputs, - and demonstrate that an invalid or missing execution prompt fixture makes - the maintained validation fail. - -#### S16-F02: A transport test depends on fixed port 9999 being unused - -- **Category:** testing -- **Severity:** low -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/llm/openai_compatible_client_test.go` - (`TestOpenAICompatibleClientAllowsEmptyConfiguredBaseURL`) -- **Contract at issue:** Default tests must be deterministic, offline, - independent of live infrastructure, and free of fixed-port assumptions. A - test of configured-versus-request endpoint selection should control its - transport outcome rather than infer selection from an environmental - connection refusal. -- **Evidence:** The test sends a real HTTP request to - `http://localhost:9999/v1` and requires `ErrRequestFailed`, assuming nothing - is listening. With a temporary standard-library HTTP server bound to - `127.0.0.1:9999`, the focused test failed immediately: the selected endpoint - returned status 501, producing `ErrNonSuccessStatus` instead of the required - connection error. The listener was then stopped and the ordinary suite - passed. Search across all tests found no other contacted fixed port; - `localhost:8000` occurrences are inert fixture values or use injected - clients. -- **Failure mode:** A developer, build host, or shared runner with any service - on port 9999 gets a false test failure whose error category depends on that - unrelated service. The test also performs a real socket operation even - though both constructor acceptance and endpoint-override selection can be - established deterministically. -- **Recommended direction:** Supply a controlled `httptest` endpoint or - injected round tripper that records the selected URL and returns a deliberate - transport result. Retain separate assertions that an empty configured base - is valid and that the request endpoint takes precedence; do not preserve - connection-refusal behavior as a contract. -- **Required verification:** Run the focused case normally, repeatedly, and - while an unrelated listener occupies port 9999. It must make no request to - that listener, must prove that the target endpoint was selected, and must - retain the intended constructor and error identities without relying on the - host network state. - -### Unresolved Observations - -None. The documentation policy assigns local validation commands to the -development guide, while that guide currently has no command list and the -release procedure says its own list comes from the guide. Comparing and -baselining those three command owners is explicitly Stage 0 work, which remains -absent; this stage did not backfill that broader audit or promote a duplicate -finding beyond S16-F01's concrete missing workflow. - -Large files were not treated as defects by size. `engine_test.go`, -`internal/usecase/runner_test.go`, and the external prepared contracts organize -many distinct public or orchestration behaviors, while S14-F06 already records -the one confirmed case where repeated transport scaffolding obscures its -behavior matrix. The 25 ms prepared-timing sleep is unnecessary for ordering -but passed shuffled repetition and race runs, adds negligible suite time, and -does not establish a consequential flaky boundary by itself. - -### Risk-To-Owner Matrix - -| Risk | Credible current owner | Accepted gap or retained protection | -| --- | --- | --- | -| Public compatibility and error identity | External-package root tests in `engine_test.go`, `public_contract_test.go`, `prepared_execution_contract_test.go`, and `capacity_contract_test.go`; focused internal mapping tests own internal-type containment. | S01-F01, S02-F03, S02-F04, S03-F02, S04-F01, and S13-F01 already identify the consequential missing public cases. Other internal/root overlap asserts different stable boundaries and is retained. | -| Parsing, validation, and serialization | `internal/promptdef`, `internal/profile`, `internal/filecatalog`, `internal/jsonvalue`, `internal/validate`, and domain tests own focused rules; root corpus and JSON contracts own assembled public compatibility. | S05-F04 and S07-F06 own material missing rule coverage. The reproduced parser, profile, schema, and numeric defects already specify regression cases in their component findings; no second suite-level test owner is needed. | -| Immutability and data integrity | Domain and JSON-value tests own shared transforms; backend, repository, use-case, prepared, and root tests own snapshots at each ownership transfer. | S02-F03 owns injected-client mutation isolation. S05-F03 owns unbounded tree work. S05-F05 identifies unused internal prepared JSON tests whose stable public serialization remains protected by external root contracts. | -| External wire behavior | `internal/llm` owns realistic `httptest` request/response behavior; root tests own resolved settings reaching the built-in client and public error mapping. | S14-F01 through S14-F05 own missing cancellation, timeout, URL, size, and framing boundaries. S14-F06 retains protocol assertions while consolidating repeated scaffolding. S16-F02 is the only fixed-port/live-environment case. | -| Cancellation, failure propagation, and recovery | Artifact, validator, model-client, capacity, use-case, prepared-lifecycle, and root tests each own failures at their narrow boundary; root tests own public identities and partial-result rules. | S04-F01, S09-F02, S10-F04, and S14-F01 identify missing or defective cancellation paths. Existing failure tests use `errors.Is`/`errors.As`; error-text checks are limited to required path association, redaction, or diagnostic fragments. | -| Concurrency and resource lifecycle | Capacity package and public capacity contracts own relational limits and FIFO release; prepared use-case and public contracts own claim/discard races; race-enabled validation owns frozen-plan safety. | Stage 15 found no scheduling defect or duplicate owner. S05-F03 and S14-F04 own the two unbounded resource paths. Shuffled repeated race runs found no shared-state or ordering failure. | -| Representative assembled consumer workflows | Root external tests own broad assembled behavior; the framework corpus owns a realistic prepare fixture; the two examples own independently copyable offline preparation and execution programs. | S16-F01 records that only the preparation example is executed by maintainer validation. The examples' small parallel fixtures are retained because each program demonstrates a distinct workflow and remains standalone. | - -### Suite And Fixture Ledger - -- **Inventory and boundaries:** The graph and source inventory found 25 test - files with 281 top-level tests. Five root files use package - `promptkit_test` for public contracts, while the two small root internal - files directly own adapter copying and error translation unavailable through - an external surface. Internal packages test their own boundaries. No test - imports a consumer, and the AST-based architecture guard recursively rejects - imports of the former consumer module while a focused self-test proves that - its detector finds nested imports. -- **Fixtures and secrets:** Parser and profile fixtures are minimal malformed - and valid YAML cases; schema and prompt content use synthetic values. The - framework corpus is a shared root integration fixture rather than a second - parser truth table. Tests use `t.TempDir`, temporary directories, `fstest`, - and `httptest` where mutable or external behavior matters. Environment - credentials are synthetic and controlled with `t.Setenv` or deliberate - execution-time removal; no real secret or mutable external service is - required. S16-F02 is the sole contacted fixed port. -- **Assertions and coupling:** Public and collaborator errors are normally - asserted with `errors.Is` or `errors.As`. Remaining text assertions select - structural paths, schema/compiler causes, status codes, redaction sentinels, - or the smallest diagnostic fragment needed to distinguish a case; none - snapshots a complete noncontractual message. Exact collaborator call counts - in runner and prepared tests protect ordering, bounded generation, no partial - work, and one-attempt lifecycle requirements rather than private call graphs. - No golden files or automatic update mode exist. -- **Consolidation:** Component ledgers found intentional relational overlap - between package, use-case, and public boundaries. The accepted deletion or - consolidation candidates already state their remaining protection: - S05-F05 can remove unused internal prepared serialization cases while root - JSON contracts remain; S09-F05 can stop pinning a hash algorithm while - retaining determinism and mutation sensitivity; and S14-F06 can share HTTP - setup while retaining every wire-visible assertion. No additional - evidence-backed deletion emerged at suite level. -- **Determinism and cost:** Twenty shuffled ordinary runs and three shuffled - race-enabled runs passed across every package. No test-order, global-state, - data-race, deadlock, or leaked-waiter symptom appeared. A diagnostic timing - run found no individual test above 50 ms in that sample and no package pass - event above 205 ms. Package coverage ranged from 69.2% to 100% among - packages containing substantive tests; the lower percentages align with - the already recorded JSON-value and validation decisions and were not used - as independent findings. `internal/defaults` - remains untested because its trivial baseline is exercised relationally by - higher owners rather than pinned as private constants. -- **Maintained examples:** Preparation resolves and renders without a model; - execution injects a deterministic client and validates a result. Preparation - never calls its configured endpoint, and execution's injected client bypasses - its endpoint, so both remain offline and credential-free. - Their similar prompt files are intentionally local so either complete - program can be copied without a hidden cross-example dependency. Both ran - successfully during this review; only validation ownership is deficient as - S16-F01 records. - -### Verification Performed - -The code knowledge graph inventoried test files and symbols, associated tests -with production hotspots, and confirmed the external and internal package -boundaries. Source and the prior component ledgers supplied the behavioral -ownership analysis. Diagnostic coverage and timing were used only to direct -the review: - -```sh -go test -cover ./... -count=1 -go test -json ./... -count=1 -``` - -The suite passed shuffled repetition, shuffled race execution, vet, build, Go -formatting, and both offline example runs: - -```sh -go test ./... -shuffle=on -count=20 -go test -race ./... -shuffle=on -count=3 -go vet ./... -go build ./... -go run ./examples/go-library/prepare -go run ./examples/go-library/run -``` - -A temporary Python standard-library server was bound to `127.0.0.1:9999` only -for the focused probe. With it active, this command failed with status 501 -instead of the expected `ErrRequestFailed`, confirming S16-F02; the server was -stopped immediately afterward: - -```sh -go test ./internal/llm -run '^TestOpenAICompatibleClientAllowsEmptyConfiguredBaseURL$' -count=1 -``` - -### Handoff - -- The Stage 0 baseline and its assigned comparison of validation-command - owners remain absent and were not backfilled. -- Stage 17 should use the risk matrix and existing S02-F02, S02-F05, S07-F05, - S12-F01, and S14-F06 duplication evidence without treating file size, helper - counts, or intentional cross-boundary tests as new consolidation findings. -- Stage 18 should retain both S16 findings unless the final audited tree has - gained deterministic execution-example validation or removed the fixed-port - assumption. Its synthesis should count earlier test gaps once at their - owning component rather than cloning them from this matrix. - -## Stage 17: Cross-Cutting Duplication, Efficiency, And Architecture Review - -### Scope Reviewed - -This review synthesized all accepted component findings and coverage ledgers, -then checked the complete production graph for package dependencies, interface -width, structural similarity, complexity, transitive loop depth, and repeated -work. Source inspection covered the graph's production similarity pairs and -hotspots, every production interface, the root facade's assembly and -public/internal adapters, the execution-profile and output-contract ingress -paths, source discovery, JSON-value copying, schema preparation, request -construction, and the capacity boundary. Canonical architecture and internal -component documents were compared with the resulting dependency and ownership -map. - -The graph found 15 production interfaces. Thirteen contain one operation, -`PreparedValidation` contains the cohesive pair needed to validate against and -describe one frozen plan, and `Option` contains only its private application -operation. The import map has the root facade depending inward on the -documented internal components, with no internal dependency back on the root -or a consumer. Similarity and complexity metrics were treated as locators: -each candidate below was accepted or rejected only after its rule, frequency, -and boundary were inspected. - -### Accepted Findings - -#### S17-F01: Execution-setting bounds have three independent acceptance owners - -- **Category:** duplication -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `profiles.go` (`validatePublicProfile`), - `internal/profile/filesystem_repository.go` (`validateProfile`), - `internal/usecase/runner.go` (`mergeExecutionTargetOverride`), and the - execution-setting declarations in `internal/domain/domain.go` -- **Contract at issue:** Temperature, maximum tokens, top-p, and timeout are - the same resolved execution settings whether they enter through an - in-memory profile, a file profile, or a request override. Optional pointer - presence and source error categories differ, but the accepted scalar values - do not; each setting needs one source-neutral invariant owner. -- **Evidence:** The two profile validators repeat the same four checks and - error text, while `mergeExecutionTargetOverride` repeats those checks for - pointer values. The completed component probes found the same IEEE NaN hole - independently in profile inputs (S08-F01) and request overrides (S11-F01), - and S02-F02 had already established drift risk between the two profile - validators. The three paths all consume `internal/domain` values but no - leaf package owns their scalar invariants. The model client separately - rechecks non-negative timeout as a defensive request boundary, reinforcing - that the value rule is broader than any one source. -- **Failure mode:** Corrections and new constraints require coordinated edits - across source and orchestration packages. The existing non-finite defect - already reaches supposedly valid profile and prepared values through more - than one ingress, and a later one-path fix can make identical settings valid - or invalid according to their source. -- **Recommended direction:** Put pure execution-setting scalar validation in - `internal/domain`, beside the shared values whose invariants it defines. - Profile and use-case packages should retain source-specific required fields, - pointer-presence handling, normalization, and error translation while - calling that owner. This dependency remains inward and acyclic because all - three consumers already depend on `internal/domain`, which need not depend - on a source, use case, or provider. The model client may consume the same - predicate while retaining its own defensive error identity. -- **Required verification:** Give the shared owner one table containing NaN, - both infinities, every exact bound, finite neighbors on both sides, and - representative interior values for all four settings. Retain small - integration cases for in-memory profiles, both file-source forms, request - overrides including explicit zero presence, and the model-client timeout - defense. Each boundary must preserve its current public or internal error - category, and no stable JSON or provider request may receive a non-finite - effective setting. - -#### S17-F02: Output-contract legality is split between prompt loading and request resolution - -- **Category:** duplication -- **Severity:** medium -- **Confidence:** confirmed -- **Status:** accepted -- **Affected code:** `internal/promptdef/filesystem_repository.go` - (`normalizePromptDefinitionWithContent`, `isValidOutputFormat`, and - `isValidValidationMode`), `internal/usecase/runner.go` - (`resolveOutputContract` and structured-output resolution), - `internal/domain/domain.go` (`OutputContract` and its enums), and the public - `OutputContract` conversion in `convert.go` -- **Contract at issue:** Supported format and validation-mode membership, - non-negative repair attempts, and the JSON-Schema path relationship describe - one `domain.OutputContract`. Prompt-file requirements and request defaults - can differ, but a request replacement must not create a domain value that - the prompt source would reject under the same stable vocabulary. -- **Evidence:** Prompt normalization owns private enum predicates and rejects - unsupported formats, unsupported modes, a missing JSON-Schema path, and - negative repair attempts. Request resolution copies the same domain value - wholesale and defaults only an empty format. S11-F02 confirmed that both - public preparation APIs accept unknown request enums. Source inspection also - shows that a negative request repair count bypasses the prompt-owned - non-negative repair-count rule, and missing request schema information is - left to later schema work and its different error category. The format reference - declares the same output-contract value set before describing complete - request replacement, so this is a split semantic owner rather than two - provider-specific policies. -- **Failure mode:** File and request forms of the same contract have different - acceptance behavior, preparation can publish unsupported stable metadata, - and adding a format or validation mode requires source and orchestration - edits that have no shared compile-time or test owner. A direct fix to - S11-F02 that copies the prompt package's switches would deepen that drift. -- **Recommended direction:** Put source-neutral output-contract predicates and - cross-field invariants in `internal/domain`. Keep file-required fields and - YAML error context in `internal/promptdef`; keep the request's documented - empty-format default and invalid-request translation in `internal/usecase`; - and keep schema loading and compilation in `internal/validate`. Both current - consumers already depend on the domain leaf, so the shared dependency stays - inward without making prompt loading depend on orchestration or validation. -- **Required verification:** Run one shared table over every supported enum, - empty and unknown values, negative and non-negative repair counts, and - schema-path relationships. Retain source-specific prompt cases and require - `Prepare`, `Run`, and `PrepareExecution` request replacements to fail as - invalid requests before source completion, admission, or generation. Valid - contracts must normalize identically across both public preparation paths. - -### Retained Cross-Cutting Opportunities - -The following accepted component findings already identify the correct owner -or a decision-complete consolidation. They remain accepted without new IDs: - -- S02-F05 owns stable public JSON field mapping. Its repeated declarations are - one serialization policy; a wire alias or embedded representation can make - ordinary fields compile-time shared while keeping timing exceptions local. -- S07-F05 owns prompt selection across OS and `fs.FS` repositories. A source - adapter can leave real opening and display-path mechanics at the edge while - `internal/promptdef` owns one selector and normalizer. -- S08-F02 should make file profiles consume the existing - `internal/jsonvalue` invariant owner. The dependency is already valid for - other profile ingress; creating another JSON-shape validator is not needed. -- S12-F01 owns initial-versus-repair generation request construction. The two - calls follow one effective target policy, and a use-case-local request - constructor can share presence, credentials, backend identity, and - structured output without moving provider mapping out of `internal/llm`. -- S14-F06 owns repeated transport test scaffolding. A recording-provider - fixture belongs in the LLM test package and should consolidate mechanics, - not protocol assertions. - -### Efficiency Disposition - -No new efficiency finding was accepted. The demonstrated opportunities remain -owned by their component findings with the following cross-cutting cost and -verification constraints: - -| Finding | Frequency and scale | Evidence and remediation invariant | -| --- | --- | --- | -| S07-F04 | Every exact prompt lookup used by inspection, preparation, and ordinary execution; cost currently scales with all unrelated file-backed template bytes. | A counting filesystem observed one unrelated body open per lookup. Preserve point-in-time YAML scanning and duplicate detection while proving only selected content is opened once. | -| S08-F06 | Every profile lookup, for every YAML file in every consulted overlay source; cost scales with total catalog YAML bytes. | Source inspection confirms two decoder passes per file. Benchmark small and large catalogs with allocation counts, and preserve strict selected-file and repeated point-in-time behavior. | -| S09-F04 | Every `input` helper reference during rendering; cost scales with artifact bytes times references across the session and messages. | The 1 MiB probe measured approximately one additional body-sized allocation per reference. Benchmark one and repeated references while preserving exact bytes and per-render ownership. | -| S10-F03 | Every ordinary JSON-Schema preparation or run; cost scales with the root and transitive schema graph. | A counting source observed the root read twice in ordinary execution, and compiled-plan parity is also a correctness requirement. Prove each schema document is read once per operation without adding a cross-operation cache. | -| S10-F05 | Every generated artifact validated in JSON mode; cost scales with output bytes and nesting. | The approximately 1 MiB probe measured about 22.7 MiB and 250,031 allocations for a discarded tree versus a zero-allocation syntax scan. Retain scalar, object, large-array, trailing-data, and exact-number benchmarks and semantics. | - -The Stage 15 concurrency review found no avoidable lock contention or serial -work. Admission is a constant-time locked count update, generation locks cover -only pool bookkeeping, provider calls run unlocked, and separate backends use -separate pools. No scheduling change is justified without representative -contention measurements that preserve the documented FIFO and cancellation -linearization points. - -The model-client extra-parameter path validates values before marshaling the -whole request, so it can traverse that subgraph twice. As recorded in Stage -14, no representative measurement established material cost relative to a -provider call; it remains rejected as a standalone efficiency finding pending -a benchmark over realistic parameter sizes. JSON-value defensive copies at -public/internal ownership transfers are likewise contract work, not removable -overhead. S05-F03 already owns their missing depth and work bounds. - -### Rejected Candidates - -- **Facade method similarity:** The graph scored `Prepare` and `Run` as - identical and the two inspection methods nearly so. These are distinct - supported operations whose thin methods consistently perform nil checking, - boundary conversion, error mapping, and result conversion. Their substantive - selection and preparation work is already shared in `internal/usecase`; - hiding the public methods behind another abstraction would enforce no new - rule. -- **Source-option similarity:** File and `fs.FS` option constructors for - prompts, profiles, fallbacks, and schemas have identical small shapes, but - each sets a different typed source slot and precedence category in the - facade that owns assembly. A generic setter would weaken that distinction - without removing semantic policy. The broader prompt repository duplication - remains S07-F05. -- **File-catalog walker similarity:** `FindYAMLFiles` and `FindFSYAMLFiles` - repeat filtering, cancellation, sorting, and collection, but differ at the - standard-library OS-versus-`fs.FS` boundary and return different path forms. - They are short leaf adapters with shared filename helpers and no observed - behavioral drift. Repository-level selection duplication, where drift is - material, is already accepted. -- **Public/internal interface pairs:** `ArtifactReader` and `LLMClient` mirror - one-operation internal consumer interfaces, but their adapters translate - supported public values into private domain values and establish copying and - error boundaries. Merging them would expose internal representations; - S02-F03 owns missing protection for the LLM copy promise. -- **Validator capability interfaces:** `Validator`, `ValidationPreparer`, and - `SchemaDocumentLoader` describe different live, frozen, and document-only - capabilities. They are narrow, consumed conditionally, and preserve test and - source boundaries. S10-F03 may eliminate ordinary document-only schema work, - but metrics alone do not justify broadening or collapsing all capabilities. -- **Constructor width and facade concentration:** `NewRunner` and its internal - repair-capable variant accept eight and nine narrow collaborators, while - `NewEngine` has high graph centrality and transitive loop depth. The runner - is the assigned orchestration owner and the facade is the assigned assembly - root. A parameter object or service-locator wrapper would add indirection - without reducing responsibility or dependencies; `NewEngine`'s propagated - loop metric comes from one-time construction callees, not a nested runtime - hot path. -- **Complex source and copy functions:** Prompt loading and normalization are - the graph's largest source functions, while recursive JSON copying has the - highest cognitive score. S07-F04 and S07-F05 already identify the meaningful - prompt split and repeated work. JSON copying deliberately centralizes type - preservation, deterministic paths, cycle detection, and value validation; - S05-F03 owns its consequential resource risk. Splitting either solely to - lower a metric would obscure its invariant without changing cost. -- **Shallow map-helper similarity:** `copyShallowAnyMap` and `copyStringMap` - received a perfect token-similarity score but copy different value domains - at different boundaries. Each is a few lines, neither owns validation, and - a generic helper would add type indirection without credible drift risk. - -### Documentation And Dependency Check - -The implemented dependency direction and package responsibilities match the -architecture policy and the internal overview, runner, source, LLM, and -capacity documents. The root remains assembly and translation, use-case code -remains orchestration, provider mapping remains in `internal/llm`, and no -internal component imports a consumer. The two accepted consolidations would -expand `internal/domain` from shared values to their source-neutral -invariants; if implemented, the architecture policy and internal overview -must be updated in the same change rather than leaving validation ownership -implicit. - -### Unresolved Observations - -None. Every graph similarity, interface-width, complexity, repeated-work, -contention, transformation, and responsibility candidate reviewed above is -accepted under an existing or new finding or explicitly rejected with its -boundary rationale. - -### Verification Performed - -The refreshed code knowledge graph supplied the complete production interface -inventory, import map, structural similarity pairs, function complexity, -transitive loop depth, and call paths from public entry points to the candidate -owners. Exact graph snippets and source inspection confirmed each candidate; -no temporary benchmark or repository mutation was needed because the completed -confirmed findings already contained the required measurements and probes. -The repository-wide tests and static analysis also passed: - -```sh -go test ./... -go vet ./... -``` - -### Handoff - -- Stage 18 should merge S17-F01 with the shared root cause represented by - S02-F02, S08-F01, and S11-F01 without losing their source-specific error and - regression requirements. -- Stage 18 should merge S17-F02 with S11-F02 and retain the prompt-file cases - as integration protection for one domain-level output-contract rule. -- Previously accepted efficiency and consolidation findings remain separate - where their owners and remediation invariants differ. No lock optimization, - generic facade abstraction, or cross-operation cache should be added to the - final accepted set without new measurement or boundary evidence. -- The Stage 0 baseline remains absent and was not backfilled during this - synthesis-driven review. - -## Stage 18: Final Audit Synthesis - -### Final-Tree Recheck - -All 53 historical finding records were rechecked against the final audited -tree. The Stage 1 review recorded -`ebf1602635e108e2a7ac1abd3a3ca24a620104ce` as its clean code snapshot, and -the complete commit range from that snapshot through Stage 17 changes only -`audit.md`. No production code, test, fixture, example, canonical contract, or -policy changed during the audit sequence. Consequently, none of the confirmed -failure probes or high-confidence test and ownership analyses has been fixed -or invalidated in the final tree. - -The current code graph was rechecked at 1,753 nodes and 7,036 edges, including -489 functions, 100 methods, 15 interfaces, 106 imports, 1,469 calls, 714 test -relationships, and 53 structural-similarity edges. Its public entry points, -package boundaries, dependency direction, and hotspots remain consistent with -the Stage 17 architecture review. Exact source checks reconfirmed the two -cross-cutting merge groups below. Every retained finding has either confirmed -runtime evidence or high-confidence source, contract, and test evidence; no -medium- or low-confidence observation enters the final accepted set. - -### Merge And Supersession Registry - -The audit history is preserved in place. One exact policy-duplication record is -superseded, while source-specific behavioral symptoms remain visible under -their shared root causes: - -| Canonical group | Historical records | Final disposition | -| --- | --- | --- | -| S17-F01, execution-setting acceptance | S02-F02, S08-F01, S11-F01, S17-F01 | S02-F02 is superseded by S17-F01 because both identify the same missing shared profile/execution-setting owner. S08-F01 and S11-F01 remain accepted downstream correctness evidence for profile and request ingress, respectively. The group is counted once as high-severity correctness, with duplicated policy as its root cause. | -| S17-F02, output-contract legality | S11-F02, S17-F02 | S17-F02 owns the split source-neutral policy; S11-F02 remains the confirmed request-boundary symptom and its error-ordering regression requirement. The group is counted once as medium-severity correctness. | - -The other 47 historical IDs remain standalone canonical findings. Similar -effects in separate owners, such as additional YAML documents in prompt and -profile parsers or cancellation in artifact, validation, and transport -boundaries, were not merged: each requires a distinct package fix, preserves a -different error boundary, and has its own regression owner. Keeping those -records separate avoids speculative centralization while still allowing a -later implementation plan to schedule related work together. - -After supersession there are 52 accepted evidence records and one superseded -record. After the two root-cause groups are counted once rather than counting -their downstream evidence again, the final planning input contains 49 -canonical remediation groups. - -### Final Finding Counts - -| Category | Canonical findings | -| --- | ---: | -| Correctness | 27 | -| Testing | 13 | -| Efficiency | 4 | -| Duplication | 2 | -| Contract-documentation consistency | 2 | -| Clarity | 1 | -| **Total** | **49** | - -| Severity | Canonical findings | -| --- | ---: | -| High | 7 | -| Medium | 36 | -| Low | 6 | -| **Total** | **49** | - -The canonical groups comprise 33 confirmed and 16 high-confidence findings. -High severity is reserved for unbounded work or input, source containment, -unresponsive cancellation, data-semantic corruption, and public transport -failure. Medium severity covers bounded but material correctness, -compatibility, policy-drift, test-protection, and measured cost risks. Low -severity is limited to clarity, deterministic workflow protection, and test -maintenance. These ratings describe consumer or maintainer impact, not the -estimated size of a fix. - -### Canonical Finding Registry And Recommended Order - -This ordering is a risk and dependency sequence for later planning, not an -implementation roadmap. The separate implementation pass must decide work -units, exact files, acceptance criteria, and validation. - -| Order | Work class | Canonical IDs | Dependency rationale | -| ---: | --- | --- | --- | -| 1 | High-impact behavioral, data-integrity, resource, and cancellation fixes | S05-F03, S07-F01, S09-F02, S10-F01, S17-F01 (including S08-F01 and S11-F01), S14-F01, S14-F04 | Establish bounds, containment, exact data semantics, shared numeric invariants, and cancellation/resource identities before refactoring or optimizing their paths. | -| 2 | Medium behavioral fixes in public values, normalization, and sources | S02-F01, S03-F01, S05-F01, S07-F02, S07-F03, S08-F02, S08-F03, S08-F04, S08-F05, S09-F01, S09-F03 | Stabilize source selection, parsing, value shape, and error behavior before source consolidation and traversal optimization. S08-F02 should consume bounded shared JSON-value validation after S05-F03. | -| 3 | Medium behavioral fixes in validation, orchestration, and transport | S10-F02, S10-F03, S10-F04, S17-F02 (including S11-F02), S12-F01, S12-F02, S14-F02, S14-F03, S14-F05 | Normalize contracts before schema work, establish one valid compiled schema path, preserve initial/repair semantics, and fix transport framing before reorganizing tests or measuring residual cost. | -| 4 | Contract and documentation consistency decisions | S05-F02, S13-F01 | Decide the public numeric acceptance rule before changing JSON-value boundaries; align copied-handle formatting with the already documented opaque contract. These are confirmed mismatches, not speculative documentation cleanup. | -| 5 | Safe production refactors with clear owners | S02-F05, S07-F05 | Consolidate stable JSON field ownership and prompt source-neutral selection only after their current behavior and defects are fixed and protected. | -| 6 | Consequential medium test gaps | S01-F01, S02-F03, S02-F04, S03-F02, S04-F01, S05-F04, S07-F06, S12-F03 | Add protection at each named owner alongside or immediately after its behavioral work; do not reproduce lower-layer truth tables at the public facade. | -| 7 | Clarity, deterministic workflow, and test consolidation | S06-F01, S05-F05, S09-F05, S14-F06, S16-F01, S16-F02 | Remove writable non-policy state, delete or rewrite low-value tests while retaining stronger contracts, make transport tests environment-independent, and synchronize the maintained offline workflow. | -| 8 | Demonstrated performance work | S07-F04, S08-F06, S09-F04, S10-F05 | Optimize only after correctness and ownership changes settle each path. Preserve the counting, allocation, and behavior invariants specified by the findings and compare representative catalog, artifact, schema, and output sizes. | - -The registry accounts for every canonical group exactly once: 27 behavioral -correctness groups in orders 1 through 3, two contract-consistency decisions, -two safe production refactors, 13 testing findings split by impact and -maintenance type, one clarity finding, and four measured efficiency findings. - -### Dependency And Root-Cause Notes - -- S05-F03 precedes S05-F04 and S08-F02 so expanded JSON-shape protection and - file-profile consumption do not institutionalize an unbounded shared copier. -- S17-F01 is the source-neutral owner for the S08-F01 and S11-F01 regressions; - their profile/config and invalid-request error mappings remain separate test - responsibilities. -- S07-F01, S07-F02, and S07-F03 should establish correct prompt-source - behavior before S07-F05 unifies its OS and `fs.FS` algorithms. S07-F04 then - removes unnecessary content reads from the unified selection path, while - S07-F06 protects the resulting semantic rule set. -- S08-F01 through S08-F05 establish profile validity and selection before - S08-F06 changes decoding work. Point-in-time lookup and overlay fallthrough - remain invariants; no engine-wide cache is implied. -- S10-F01 through S10-F04 establish exact JSON semantics, URL-safe schema - loading, compiled preparation, and cancellation before S10-F05 replaces - JSON-mode materialization. S10-F03 owns one read/compile plan per operation, - not cross-operation schema caching. -- S17-F02 normalizes request contracts before schema loading. S11-F02's public - preparation parity and invalid-request ordering remain the regression - boundary for that shared policy. -- S12-F01 and S12-F02 correct repair request and result semantics before - S12-F03 expands multi-attempt progression coverage. -- S14-F01 through S14-F05 and deterministic replacement of S16-F02 should - settle transport behavior before S14-F06 consolidates its fixture matrix. - The refactor must retain every durable wire assertion and specialized - cancellation, deadline, limit, and body-lifecycle transport. -- S02-F01 should settle duration decoding before S02-F05 consolidates the - stable JSON mapping around it. S05-F02 separately requires an explicit - public numeric policy choice rather than an inferred interoperability limit. -- S16-F01 requires one canonical maintainer-command owner. Update the - development guide and synchronize documents that link to its workflow; do - not create another independent command list. - -### Work-Type Separation - -- **Behavioral fixes:** The 27 correctness groups change accepted inputs, - source containment or selection, cancellation, validation, execution - accounting, response framing, or public error behavior. Each requires a - regression at its narrowest stable owner plus only the public integration - needed to protect mapping or orchestration. -- **Safe refactors:** S02-F05 and S07-F05 have clear semantic owners and drift - evidence. S06-F01 is a small clarity change that removes writable state. - None should alter supported behavior. -- **Performance work:** S07-F04, S08-F06, S09-F04, and S10-F05 have a stated - frequency, scale, measurement or complexity model, and benchmark or - counting invariant. No other performance idea is accepted. -- **Test gaps:** Eight medium findings protect consequential redaction, - ownership, mapping, precedence, cancellation, JSON shape, prompt semantics, - and repair progression. Add them at the owners already named in their - findings rather than chasing coverage percentages. -- **Test consolidation and deterministic maintenance:** S05-F05 can remove an - unused internal serialization boundary, S09-F05 can replace algorithm - literals with hash relationships, and S14-F06 can consolidate HTTP - mechanics. S16-F02 replaces a fixed-port assumption, while S16-F01 adds the - already maintained Run example to canonical validation. Each record states - the protection that must remain. -- **Documentation synchronization:** S05-F02 requires GoDoc and format - documentation only if the chosen numeric contract differs from today's - finite-number promise. S16-F01 synchronizes validation documentation through - its canonical development-guide owner. Implementing either S17 owner in - `internal/domain` requires the architecture policy and internal component - overview to name the new invariant responsibility in the same change. - Other behavior changes should update only their existing canonical GoDoc, - format, integration, or internal owner when observable claims change. - -### Rejected And Residual Work - -The final set excludes every lower-confidence observation and every Stage 17 -candidate rejected for lacking a shared semantic owner, drift risk, runtime -measurement, or durable boundary. In particular, it excludes coverage-driven -tests, generic facade and source-option abstractions, interface collapsing, -constructor parameter objects, cross-operation caches, lock or fairness -changes without contention evidence, complexity-only function splitting, and -an unmeasured extra-parameter marshaling optimization. Stage 15 found no -capacity, race, deadlock, fairness, or resource-release defect, so no -concurrency remediation is invented at closeout. - -Residual uncertainty is limited and explicit: - -- Stage 0 was never executed, so there is no independently recorded initial - package inventory, validation baseline, or initial coverage snapshot. The - unchanged Stage 1 code commit and final full validation establish a - reproducible final reference but cannot recreate that missing historical - baseline. -- Performance figures in S09-F04 and S10-F05 came from temporary diagnostic - benchmarks on the audit host. They confirm allocation and asymptotic waste - but are not release performance promises; implementation must retain - representative benchmarks rather than target the recorded wall-clock - numbers. -- Provider transport review used deterministic local servers and transports, - as required by testing policy. No live or paid provider was contacted, so - upstream behavior outside the documented OpenAI-compatible contract remains - intentionally untested. - -These uncertainties do not lower confidence in an accepted finding. There are -no unresolved observations awaiting promotion or rejection. - -### Audit Coverage Summary - -Stages 1 through 17 reviewed every root public value, formatter, error and JSON -boundary; configuration, extension adapter, engine option and operation; -internal domain, JSON-value, backend, defaults, built-in profile, file catalog, -prompt definition, profile, artifact, renderer, validator, use-case, prepared -execution, model client, and capacity component; all 25 test files and 281 -top-level tests; both maintained offline Go-library examples and their -fixtures; and the cross-package dependency, similarity, interface, complexity, -and responsibility map. Public, package, failure, cancellation, resource, -serialization, source, provider-wire, lifecycle, and concurrency paths all -have a coverage ledger or accepted finding. - -The audit produced 49 canonical remediation groups. It found no production or -test change during the findings-only sequence, no lower-confidence accepted -item, no unsupported consumer dependency, and no code or test modification -mixed into the audit artifact. - -### Final Validation - -After the Stage 18 documentation edit, ordinary and race-enabled package -tests, static analysis, build, and both maintained offline examples passed: - -```sh -go test ./... -go test -race ./... -go vet ./... -go build ./... -go run ./examples/go-library/prepare -go run ./examples/go-library/run -``` - -Every tracked Go file passed `gofmt`. All 180 maintained Markdown links had an -existing repository target or supported external form, all 18 local anchors -resolved, and the one published Markdown target returned HTTP 200. Module -metadata remained: - -```text -gitea.maximumdirect.net/eric/promptkit 1.25.5 -promptkit gitea.maximumdirect.net/eric/promptkit -``` - -No tracked Go workspace, vendor tree, or module replacement exists. -`git diff --check` passed, the diff from the Stage 1 code snapshot through the -final audit contains only `audit.md`, and the working tree immediately before -staging contained only that authorized documentation change. - -### Closeout - -`audit.md` is now the complete findings-only input to a separate remediation- -planning prompt. The later `implementation.md` pass must turn the ordering and -dependencies above into decision-complete work units. This audit does not -authorize or schedule code, test, fixture, contract, or durable-documentation -changes. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md deleted file mode 100644 index bb54583..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -1,840 +0,0 @@ -# Audit Remediation Implementation Plan - -## Purpose - -This document is the decision-complete implementation plan for the accepted -findings in the [codebase audit](audit.md). It is written for a -`gpt-5.6-terra` coding agent that will implement one numbered stage per prompt, -in order. - -The audit remains the evidence and rationale for each finding. This plan owns -implementation order, selected policy decisions, required code and test work, -and stage gates. It does not activate the future public output-repair feature -described in the [future feature catalog](future.md); it only corrects and -protects the retained internal repair machinery on which that later feature -may build. - -## Implementation Policies - -Every stage must follow the [development guide](../development.md), -[architecture policy](../policy/architecture.md), -[testing policy](../policy/testing.md), and -[documentation policy](../policy/documentation.md). Before changing a -subsystem, read the focused current-state documents identified by the -development guide and inspect the exact implementation and tests named by the -stage. - -Apply these rules throughout: - -- Implement exactly one stage per agent prompt. Do not combine stages or begin - a later stage early. -- Inspect the working tree before editing and preserve unrelated changes. -- Use the code knowledge graph to locate symbols, callers, and dependency - paths; confirm important conclusions against source. -- Keep the root package as the public facade and implementation under - `internal/`. Do not expose internal representations or add a public package. -- Put source-neutral invariants in their assigned internal owner while - preserving source-specific normalization, error classification, and public - translation at existing boundaries. -- Add regression protection at the narrowest stable owner in the same stage - as a behavioral fix. Retain only representative integration coverage at - higher layers. -- Do not add tests to raise coverage percentages. Do not preserve tests that - assert an incidental algorithm, private constant, dormant serialization - shape, or duplicated lower-layer truth table. -- Keep all tests deterministic, offline, race-safe, credential-free, and free - of fixed-port or mutable-service assumptions. -- Do not add engine-wide caches, generic facade abstractions, scheduler - changes, provider retry policy, or new consumer configuration unless a stage - explicitly requires it. -- Update canonical GoDoc and current-state documents in the same stage as the - behavior they describe. Do not describe a later stage as already - implemented. -- Format changed Go files. Run the stage's focused commands, then at least - `go test ./...` and `go vet ./...`. Run focused race tests wherever the - stage changes ownership, cancellation, shared state, or lifecycle behavior. -- Do not commit, push, tag, or publish unless separately instructed. - -## Decisions Fixed By This Plan - -The implementing agent must not reopen these choices: - -1. **JSON-compatible numbers:** accept every value Go can faithfully encode as - a JSON number: every signed and unsigned integer width, finite `float32` and - `float64` values, and a `json.Number` whose text is valid JSON-number syntax. - Do not impose the current IEEE-754 safe-integer restriction. Reject NaN, - infinities, and malformed `json.Number` text. Preserve supported concrete - numeric types when copying. -2. **JSON-shaped traversal bounds:** allow at most 100 JSON container levels - and 100,000 produced JSON value nodes per `Copy` or `CopyMap` operation. - Count the root, each map/slice/array container, and every produced child - value; map keys are not separate nodes. Pointer and interface indirection - do not add JSON depth or an extra node. Repeated appearances of an acyclic - shared value count each produced occurrence. Continue rejecting active-path - cycles and return deterministic, path-aware validation errors on either - bound. -3. **Execution timeout bound:** a positive `TimeoutSeconds` must fit in - `time.Duration` after multiplication by `time.Second`. Derive the maximum - from `math.MaxInt64` and `time.Second`; do not duplicate its numeric literal - in tests or documentation. -4. **Output contracts:** the only valid formats are `text`, `markdown`, and - `json`; the only valid validation modes are `none`, `basic`, `json`, and - `json_schema`; repair attempts are non-negative; and `json_schema` requires - a nonblank schema path. A non-nil request replacement defaults an empty - format to `text` before shared validation. It does not default an empty - validation mode. -5. **Prompt content paths:** every `content_file` is an exact, relative path - resolved from its prompt file and contained by the configured prompt source - root. Directory, `fs.FS`, and single-file sources all reject absolute and - escaping paths. A single-file source's root is the containing directory of - that selected prompt file. Trimming determines only whether a value is - blank; it must not change the path opened. OS containment must account for - symlinks; containment inside an injected `fs.FS` remains expressed in that - filesystem's namespace. -6. **Profile IDs:** normalize file-backed IDs with `strings.TrimSpace` once, - just as in-memory IDs are normalized. Use the normalized value for - selection, duplicate detection, results, and diagnostics. A whitespace-only - ID is invalid, and IDs that become equal after normalization are - duplicates. -7. **Ordinary artifact files:** the built-in `File` reader supports regular - files, including symlinks whose targets are regular files. It rejects - directories, FIFOs, devices, sockets, and other non-regular targets before - consuming them. It remains unrestricted by an application root and does - not introduce an application-specific byte limit. -8. **Validation cancellation:** do not return early by abandoning goroutines - around `fs.FS` or the JSON Schema dependency. Promptkit must check - cancellation before, between, and after work it controls; read opened files - in context-checked chunks; and let a canceled context win before publishing - a result after synchronous decode, compile, or validation calls. Go's - `fs.FS` and the current JSON Schema library expose no general mechanism to - preempt a blocked `Open`, `Read`, compile, or validation method, so canonical - documentation must describe this synchronous limitation rather than claim - impossible asynchronous interruption. -9. **Successful provider-response limit:** the built-in OpenAI-compatible - client accepts at most 16 MiB (`16 << 20` bytes) for the complete successful - HTTP response body, including surrounding whitespace. The limit is fixed, - internal, and application-neutral. Exactly the limit is allowed; the first - byte beyond it fails as `internal/llm.ErrMalformedResponse`. Do not add a - public setting. Non-success response parsing remains outside this audit - remediation and belongs to the separate structured-generation-error - roadmap. -10. **Repair machinery:** retain and fix the internal repairer, cumulative - usage, and bounded repair state machine. The public engine must continue to - install no repairer and remain single-pass. Do not activate public repair - in this plan. - -## Stage 1: Centralize Execution-Setting And Session Invariants - -**Findings:** S05-F01, S17-F01, S14-F02. This stage also resolves the -source-specific evidence in S02-F02, S08-F01, and S11-F01. - -Add a source-neutral execution-setting validator to `internal/domain`. It must -validate temperature, maximum tokens, top-p, and timeout on a domain execution -target: temperature and top-p must be finite and within their closed ranges, -maximum tokens must be non-negative, and timeout must be non-negative and no -greater than the derived duration-safe maximum. Keep optional-pointer presence, -profile required fields, normalization, and error wrapping outside this -validator. - -Use that owner from: - -- in-memory profile validation in the root package; -- OS and `fs.FS` profile validation; -- resolved request/target validation in `internal/usecase`; and -- the built-in model client as a defensive final boundary. - -Remove the duplicated scalar comparisons from those callers. Preserve -`ErrInvalidConfig` for in-memory construction, profile-load identities for file -profiles, `ErrInvalidRequest` for runtime overrides, and the LLM package's -defensive invalid-request identity. Explicit numeric zero must retain its -presence semantics. - -Update `internal/domain.NormalizeSessionID` to reject invalid UTF-8 before -trimming or rune counting. Preserve the existing blank and 256-code-point -rules. Direct requests must still map failures to `ErrInvalidRequest`, while -session-template failures remain renderer failures. - -Add one domain-owned table for every exact setting boundary, finite neighbors, -NaN, both infinities, negative values, and the timeout representability edge. -Retain small boundary-integration cases for in-memory profiles, both file -source forms, request overrides through `Prepare` and `PrepareExecution`, and -the model-client defense. Add malformed UTF-8 session cases before, within, -and after otherwise valid content. - -Update the architecture policy and internal component overview so -`internal/domain` explicitly owns source-neutral invariants for its shared -execution values, without claiming ownership of source-specific policy. - -Run focused domain, profile, use-case, root, and LLM tests, including the -affected race-enabled request and profile cases, followed by the repository -test and vet gates. - -## Stage 2: Centralize Output-Contract Legality - -**Finding:** S17-F02, including the request-boundary symptom S11-F02. - -Add one pure `internal/domain` validator for `OutputContract`. It must enforce -the format, validation-mode, non-negative repair-attempt, and JSON-Schema path -rules fixed above. It must not load schemas or apply source/request defaults. - -Make prompt-definition normalization call the shared validator after its file- -specific normalization. Keep prompt-required fields and contextual -`ErrInvalidPromptDefinition` ownership in `internal/promptdef`. Make request -resolution default an empty replacement format to `text`, then call the same -validator and translate failure to `ErrInvalidRequest` before artifact, -rendering, validation, admission, or generation work. Keep schema loading and -compilation in `internal/validate`. - -Add a domain table covering every supported and unsupported enum, empty values, -negative and non-negative repair counts, and schema-path relationships. Retain -small prompt-source and use-case integration tables that prove correct error -categories and parity between `Prepare` and `PrepareExecution`; do not repeat -the entire domain table at those layers. - -Update the architecture and internal overview language added in Stage 1 to -include source-neutral output-contract invariants. Run focused domain, -prompt-definition, use-case, and root tests, then repository test and vet -gates. - -## Stage 3: Make JSON-Compatible Value Handling Coherent And Bounded - -**Findings:** S05-F02, S05-F03, S05-F04. - -Refactor `internal/jsonvalue` around the numeric and traversal decisions fixed -by this plan. Remove the safe-integer restriction and apply one numeric rule to -all supported representations. Preserve concrete named and unnamed scalar, -map, slice, and array types where the existing contract promises preservation; -keep nil versus empty container distinctions and `Copy` versus `CopyMap` empty- -key behavior. - -Extend the traversal state to track JSON container depth and produced-node -work. Enforce the 100-level and 100,000-node limits before allocation or -descent would cross them. Continue using active-path identity for cycle -detection; do not use alias memoization that would make distinct JSON paths -share mutable output. Errors must identify the structural path and whether the -depth or work budget was exceeded. - -Expand the focused package tables by behavior branch: signed and unsigned -integer widths, ordinary and named finite floats, `json.Number`, pointers and -interfaces, named maps/slices/arrays, nil and empty values, mixed nested trees, -arrays, mutation isolation, active cycles, alternating just-below/at/over -depth, and shared acyclic subgraphs just below and over the work budget. Tests -must derive their edges from package constants or relationships instead of -copying unexplained literals. - -Retain only representative public/backend/profile/prepared integration cases -that prove error translation and ownership. Update public GoDoc only if it -currently states the narrower safe-integer behavior; otherwise the existing -finite JSON-compatible-number contract remains canonical. Update the relevant -public value GoDoc and format/internal documentation to state that excessively -deep or large JSON-shaped values are rejected for safety; keep the exact -numeric limits owned by the internal constants rather than duplicating them -throughout consumer documentation. Run focused package and caller tests, -focused race tests, repository tests, and vet. - -## Stage 4: Consolidate Stable Public JSON And Remove Dormant Internal JSON - -**Findings:** S02-F01, S02-F05, S05-F05. - -Refactor `json.go` so each public value has one ordinary field mapping. Use -private aliases or embedded wire representations for ordinary fields and keep -only timestamp, millisecond-duration, and intentional omission exceptions -explicit. Preserve every existing JSON name and omission rule. - -Before converting `duration_ms`, reject values outside the millisecond range -that can be multiplied by `time.Millisecond` without overflow. Derive both -edges from `time.Duration` bounds. Return a contextual decode error and do not -partially update the receiver on failure. - -Add fully populated `PreparedRun` and `RunResult` contract cases. Verify all -ordinary fields, intentional omissions, zero and nonzero timing, complete -round trips, the largest safe positive and negative millisecond values, and -their first unsafe neighbors. - -Remove unused JSON tags and serialization tests from -`internal/domain.PreparedRun` after confirming production never marshals that -type. Keep credential absence protected at preparation/clone producers and -move any useful cache-control JSON assertion to the public `PreparedRun` -contract. Do not retain a parallel internal wire format. - -Run focused domain and root JSON tests, repository tests, and vet. - -## Stage 5: Harden Public Ownership And Diagnostic Contracts - -**Findings:** S01-F01, S02-F03, S02-F04, S13-F01. - -Extend the existing run-request formatting test with distinct input URI, -input-body, variable, and API-key sentinels. Require their absence from -`String`, `GoString`, `%v`, `%+v`, and `%#v` while retaining positive structural -summary assertions. - -Add one focused public-LLM-adapter ownership test. Have the injected client -mutate and retain prompt messages, cache-control pointers, nested target extra -parameters, and structured-output schema values; prove the domain/prepared -source remains unchanged and later details or execution cannot race with those -mutations. - -Add one direct all-field mapping test for `OpenAICompatibleProfile`. Populate -every field distinctly and compare the complete returned `Profile`. Keep only -the existing higher-level cases that prove normal validation and nested-value -ownership. - -Make copied `PreparedExecution` values format opaquely by using value-receiver -formatting behavior shared by non-nil pointers and values. A nil pointer may -use Go's normal `` formatting, but formatting must never panic or expose -internal types, field names, addresses, credentials, or content. Cover original -pointers, copied values, zero values, and nil pointers under string, Go-string, -and ordinary fmt verbs, and prove formatting does not claim or discard a -handle. - -Run focused root tests and the affected prepared/adapter race tests, followed -by repository tests and vet. - -## Stage 6: Correct Engine Construction Edges And Immutable Defaults - -**Findings:** S03-F01, S03-F02, S06-F01. - -Change the shared single-file option helper so trimming is used only for the -blank-input check. Perform `Stat`, path decomposition, storage, diagnostics, -and later access with the exact caller path for prompt, profile, and schema -files. Add one compact table covering existing leading- and trailing-whitespace -names through all three options. - -Strengthen engine construction tests with three discriminating cases: - -- reverse the argument order of in-memory, ordinary, fallback, and built-in - profile categories while retaining fixed category precedence; -- collide `Config.ProfileDir` with an ordinary profile option and prove the - option replaces the configuration source; and -- place a valid same-category replacement after an invalid option and prove - construction still fails at the earlier invalid option. - -Convert `internal/defaults.LLMRequestTimeoutDefault` from a variable to a -constant without changing its value or adding a setter. Do not add a test that -mutates or pins a noncontractual default; existing client deadline behavior is -the verification owner. - -Run focused engine construction, default-client construction, and race tests, -then repository tests and vet. - -## Stage 7: Contain And Preserve Prompt Content Paths - -**Finding:** S07-F01. - -Refactor prompt content resolution so both repository forms receive an -explicit source-root abstraction. Enforce the path decision fixed by this plan -before any content read. Use exact parsed path text after a separate blank -check. For OS sources, canonicalize the root and resolved target sufficiently -to reject symlink escape; for injected `fs.FS`, use its clean relative path -namespace. A parent component that remains inside the root is valid. Absolute, -escaping, and symlink-escaping targets are invalid. - -Apply the same behavioral table to an OS directory, `WithPromptFS`, and a -single-file source: ordinary sibling, nested parent still within root, parent -escape, absolute path, symlink escape where supported, and existing names with -leading or trailing whitespace. Prove rejected targets cause no outside read -and public operations preserve `ErrPromptLoad`. - -Update the framework format reference and internal source document to make the -single-file root and absolute-path rule explicit. Run focused prompt-definition -and public source tests, including race tests, then repository tests and vet. - -## Stage 8: Correct Prompt Selection, Strictness, Coverage, And Lookup Cost - -**Findings:** S07-F02, S07-F03, S07-F04, S07-F06. - -Correct both existing prompt repository paths before consolidating them in the -next stage: - -- Recover selector metadata from YAML `id` and `version`; never use a filename - stem as an identity. -- Apply normalized ID and requested-version selection before semantic - normalization or `content_file` reads. -- Associate strict YAML, semantic, and content errors only with a reliably - matching selected definition. An unidentifiable malformed file is unrelated - to point lookup; a reliably selected malformed file remains authoritative. -- Require exactly one YAML document. Comments and trailing whitespace are - allowed; a second empty or populated document and malformed trailing YAML - are `ErrInvalidYAML`. -- Continue scanning the YAML metadata required for duplicate detection, but - open content only for selected candidates. A selected content file is opened - once; unrelated and different-version bodies are never opened. - -Add paired OS and `fs.FS` regressions for same-stem/different-ID malformed -files, same-ID/different-version invalid files, selected malformed definitions, -additional YAML documents, duplicates, and counting filesystem behavior. -Add a compact normalization table for the previously uncovered missing -version, blank input name, blank message role, invalid output format, negative -repair attempts, and explicit blank default profile. Output-contract rows -should exercise the shared Stage 2 owner rather than recreate its full table. - -Run focused prompt-definition, use-case inspection, and root source tests, -focused race tests, repository tests, and vet. - -## Stage 9: Unify Prompt Repository Semantics - -**Finding:** S07-F05. - -After Stage 8 establishes correct behavior in both paths, replace their -duplicated discovery-to-selection algorithms with one source-neutral prompt -selection and normalization flow. Introduce only the small internal source -adapter needed for YAML discovery, bytes, exact content opening, display paths, -and root containment. Keep genuine OS and `fs.FS` mechanics at the adapter -edge. - -Move exact selection, version filtering, strict one-document decoding, -selected-error classification, normalization, duplicate handling, and -not-found behavior into the shared flow. Preserve point-in-time source access; -do not cache catalogs or definitions across operations. - -Turn the Stage 8 behavior matrix into a shared suite over both adapters and -retain source-specific tests only for distinct path and I/O failures. Delete -superseded duplicate helpers and tests only after the shared suite protects -their meaningful behavior. Use a counting filesystem and a before/after -benchmark over small and large prompt catalogs to confirm unrelated content is -not read and the refactor adds no second scan; do not enforce wall-clock -thresholds. - -Update the internal source document to describe the unified semantic owner. -Run focused package, integration, race, repository test, and vet gates. - -## Stage 10: Correct Profile Source Validation And Identity - -**Findings:** S08-F02, S08-F03, S08-F04, S08-F05. The non-finite scalar -symptom S08-F01 is already resolved by Stage 1. - -Make file profile normalization pass `extra_params` through -`internal/jsonvalue.CopyMap` before publishing a domain profile. Preserve -`ErrInvalidProfile` and source path context for empty keys, non-finite values, -nested invalid data, or traversal-budget failures. Do not move reserved -OpenAI-compatible field policy into the profile package. - -Use YAML metadata ID as the only selector; never infer authority from a -filename. Normalize the decoded ID once according to this plan. Require exactly -one YAML document in both metadata and strict selected decoding, so trailing -raw credentials, unknown fields, empty documents, and malformed YAML cannot be -ignored. Reliably selected malformed definitions must stop overlay fallback; -unrelated malformed files must not. - -Add shared OS and `fs.FS` tables for invalid/valid extra parameters, -same-stem/different-ID malformed files beside a valid profile, fallback -behavior, additional documents, leading/trailing/blank IDs, normalized -duplicates, and exact inspection/preparation of the normalized ID. Retain only -representative public error-translation cases. - -Update the framework format and internal source documents if needed to state -ID normalization and one-document behavior. Run focused profile, use-case, -root, and race tests, followed by repository tests and vet. - -## Stage 11: Eliminate Duplicate Profile Decoding - -**Finding:** S08-F06. - -Refactor point lookup so each file receives one metadata pass and only -canonical ID matches receive strict full decoding and normalization. Reuse -bytes already read for metadata; do not decode every unrelated full profile or -turn the repository into a cache. Preserve deterministic duplicate detection, -strict selected errors, overlay fallthrough only on not-found, and fresh -point-in-time reads on every operation. - -Add counting/parser-observation tests where stable behavior can be observed, -plus benchmarks for small and large catalogs reporting time and allocations. -Exercise valid selection, unrelated malformed files, selected malformed files, -duplicates, overlay fallthrough, and repeated lookup. Do not add brittle exact -allocation thresholds to ordinary tests. - -Run focused profile and root integration tests, benchmarks for diagnostic -comparison, race tests, repository tests, and vet. - -## Stage 12: Correct Artifact Semantics, Cancellation, And Hash Tests - -**Findings:** S09-F01, S09-F02, S09-F05. - -Treat an explicitly typed empty inline reference as a valid zero-byte artifact, -including `InlineWithURI`. Keep absence at the input map/reference boundary and -compute the same metadata and opaque equality value used for other bodies. - -For ordinary file references, inspect the target before opening and again -after opening; reject anything that is not a regular file under the decision -above. Replace unbounded `io.ReadAll` with a normal synchronous chunked read -that checks `ctx.Err()` before open, before and after each read, and before -publishing the artifact. Do not return partial artifacts, add a hidden size -limit, or launch an abandoned reader goroutine. - -Add source-parity cases for empty and nonempty inline, inline-with-URI, and file -content. Add a platform-appropriate FIFO regression proving the known FIFO is -rejected without requiring an external writer, and cancellation cases for a -pre-canceled file and a progressing regular-file read. Run them repeatedly and -under the race detector. - -Replace exact SHA-256 literals with relational assertions: nonempty and stable -for repeat reads, equal for equal inline/file bodies, unequal for changed -bodies, and propagated opaquely through preparation. Do not document or test a -specific algorithm. - -Update public GoDoc and the internal source document to describe regular-file -support and cancellation checkpoints. Run focused package, use-case, root, -race, repository test, and vet gates. - -## Stage 13: Make Rendering Cancellation-Aware And Reuse Artifact Text - -**Findings:** S09-F03, S09-F04. - -Check context before session work, before and after each template parse and -execution, before and after every message, and before returning the completed -prompt. Make the `input` helper return an error when cancellation is observed. -Do not run template execution in a detached goroutine. - -Within one `Render` call, lazily convert each named artifact body to text once -and memoize that string for the session and all messages. Build the cached -string in 64 KiB chunks with one pre-grown `strings.Builder`, checking the -context between chunks. Preserve bytes exactly, including invalid UTF-8; do not -cache across render calls or mutate artifacts. Unknown and nil inputs retain -their current errors, and a canceled conversion must not publish or cache a -partial string. - -Add deterministic tests for pre-cancellation, cancellation during the chunked -input conversion, and cancellation observed after final-message execution. -Require the context identity and no partial prompt while preserving active- -context template errors. Add benchmarks for one and repeated references across -session and messages; report allocations without hard-coded timing limits. - -Run focused renderer/use-case/root tests, benchmarks, repeated race tests, -repository tests, and vet. - -## Stage 14: Preserve Exact JSON Validation Semantics - -**Findings:** S10-F01, S10-F05. - -Create one helper for decoding exactly one JSON value with -`json.Decoder.UseNumber` and required EOF after trailing whitespace. Use it for -schema documents and JSON Schema instance values so large integers, precise -decimals, and exponents retain exact `json.Number` semantics through -compilation, prepared metadata, copying, and validation. - -For plain `ValidationJSON`, use a non-materializing complete-document syntax -check such as `json.Valid`; do not build a generic tree. Preserve the current -result distinction: malformed generated JSON is a completed failed validation, -not an operational error, and original output bytes remain unchanged. - -Add focused OS and `fs.FS` cases around `2^53`, `1e400`, precise decimals, -ordinary numbers, malformed syntax, and trailing values. Exercise schema -`const`, minimum/maximum, and `multipleOf`, and verify the public structured -schema retains exact numeric values. Add benchmarks for scalar, object, and -large-array JSON validation with allocation reporting but no wall-clock -contract. - -Update format/internal validation documentation only where it currently -implies float64-limited semantics. Run focused validator/use-case/root tests, -benchmarks, race tests, repository tests, and vet. - -## Stage 15: Escape Schema Resources And Compile Once Per Operation - -**Findings:** S10-F02, S10-F03. - -Represent schema compiler resources with `url.URL` rather than string -concatenation. Use canonical escaped file URLs for OS paths and a private -scheme URL whose path segments are escaped for `fs.FS`. Preserve separators, -decode resource paths exactly once at the loader boundary, and continue -rejecting remote and escaping references. Legal filenames containing percent, -space, `#`, `?`, or Unicode must compile, including contained relative -references. - -Unify JSON Schema preparation around `validate.PreparedValidation`: - -- the shared preparation pipeline must create one operation-local compiled - plan and derive provider-facing root schema metadata from that plan; -- `Prepare` may discard the plan after returning metadata; -- `Run` must retain and use the plan for its one operation so the schema graph - is not loaded or compiled again during validation; and -- `PrepareExecution` must retain the same plan in its frozen payload. - -Remove the document-only `SchemaDocumentLoader` capability if it has no -remaining production caller. Do not add an engine-wide or cross-operation -schema cache. Keep a clear private preparation carrier in `internal/usecase` -if needed so public `domain.PreparedRun` remains free of validator interfaces. - -Replace the existing legal-filename expected failure with valid behavior and -retain a genuine compiler-registration failure only if reachable through a -valid source. Add public parity tests for invalid keywords, malformed and -missing direct/second-level references, unsupported dialects, escapes, remote -references, and valid multi-document graphs. A counting source must show each -document read once per operation and fresh reads across separate operations. - -Update internal source, validator, and runner documentation for the unified -plan lifetime. Run focused validator/use-case/root tests, race tests, -repository tests, and vet. - -## Stage 16: Make Validation Cancellation Authoritative - -**Finding:** S10-F04. - -Apply the cancellation decision fixed above. Thread context through schema -resource loaders and all Promptkit-controlled read/decode helpers. Read opened -schema files in context-checked chunks. Check the context immediately before -and after JSON decoding, schema compilation, and schema execution; if -cancellation occurred during a synchronous dependency call, return the context -error instead of a schema or successful validation result. Do not publish a -partial plan or validation result. - -Do not place arbitrary `fs.FS` calls or JSON Schema work in goroutines merely -to race them against `ctx.Done()`. Tests must therefore distinguish: - -- prompt cancellation before work; -- cancellation between controlled read chunks; -- cancellation that becomes authoritative immediately after a synchronous - compile or validation call returns; and -- the documented limitation that Promptkit cannot preempt a dependency method - that never returns. - -Use deterministic controlled readers/contexts rather than sleeps. Assert no -goroutine growth or leaked work and preserve operational validation and public -context identities. Update validator GoDoc and internal source/runner documents -to state the synchronous cancellation boundary accurately. - -Run focused cancellation tests normally, repeatedly, and under the race -detector, followed by repository tests and vet. - -## Stage 17: Repair And Protect The Retained Internal Repair Path - -**Findings:** S12-F01, S12-F02, S12-F03. - -Retain the internal repair architecture. Extend `RepairRequest` with -`ExecutionTargetPresence` and carry the resolved presence bits unchanged into -the default repairer's `GenerateRequest`. Factor one use-case-local constructor -for common initial/repair generation fields—effective target, presence, -credential, backend identity, session, and structured output—while keeping the -initial and repair prompts intentionally separate. - -Accumulate every completed generation response's five token-usage fields into -run-level usage. The final content/raw output/artifact continues to come from -the last candidate, while usage includes initial generation and every completed -repair exactly once. A repair call that returns an error still returns no -partial public result under current error semantics. - -Replace the one-attempt-only repair coverage with a compact state-machine -table for: - -- initial success with no repair; -- ineligible basic validation despite a positive budget; -- explicit zero and inherited-zero presence across initial and repair calls; -- success before a larger budget is exhausted; -- exact exhaustion of a larger budget; and -- advancement of attempt number, maximum, prior output, diagnostics, final - status, cumulative usage, and collaborator call count. - -Retain the distinct capacity integration proving initial and repair generation -use the same backend pool and one whole-run admission lease, but simplify it if -the new focused table makes repair-state assertions redundant. - -Update the internal runner and capacity documents for presence fidelity and -cumulative usage. Do not alter `NewRunner` to install a repairer, public GoDoc -that says the engine is single-pass, or the future public repair roadmap. - -Run focused use-case, prepared, capacity, and race tests, followed by repository -tests and vet. - -## Stage 18: Preserve Transport Error Identities And Test Deterministically - -**Findings:** S14-F01, S04-F01, S16-F02. Timeout overflow S14-F02 is already -resolved through Stage 1's shared bound. - -Preserve the underlying `http.Client.Do` error in the chain while retaining -`internal/llm.ErrRequestFailed` and the public generation category. Do not add -headers, request content, endpoints, or provider bodies to error text. -Cancellation and deadline identities must survive caller cancellation, caller -deadline, generation deadline, and whole-request client timeout. - -Replace the port-9999 test with a controlled round tripper or `httptest` -endpoint that records the selected URL and returns a deliberate result. It must -make no host-dependent connection and must separately prove empty configured -base acceptance and request-endpoint precedence. - -Extend the existing external ordinary-run cancellation test to require both -`ErrLLMGenerate` and `context.Canceled`; retain lower-layer tests only for their -distinct error owners. - -Update the OpenAI-compatible integration and internal LLM documents for error -identity behavior. Run focused LLM and root tests with repetition and the race -detector, followed by repository tests and vet. - -## Stage 19: Validate And Compose Effective Provider Endpoints - -**Finding:** S14-F03. - -Add one source-neutral OpenAI-compatible base-endpoint validator in -`internal/domain`, alongside the effective execution target invariant. It must -trim surrounding configuration whitespace, require absolute HTTP or HTTPS with -a host, and reject user information, query, and fragment. Use it from backend -registration, in-memory and file profiles, resolved request overrides, and the -built-in client defense while preserving each boundary's existing config, -profile-load, invalid-request, or LLM error category. - -An empty configured base URL remains valid for a built-in client because a -resolved request endpoint may supply it later. Validate only a nonempty -configured base at construction, and always validate the final selected -endpoint before transport. Backends and endpoint-only profiles retain their -existing nonempty endpoint requirements. - -Compose the completion URL through parsed URL operations (prefer -`url.JoinPath`) so nested paths and trailing slashes reach exactly one -`/chat/completions` suffix. Never append to a raw string. - -Add endpoint tables for HTTP and HTTPS, hosts, nested paths, repeated trailing -slashes, queries, fragments, user information, relative paths, missing hosts, -unsupported schemes, and request/profile/config error mapping. Require every -invalid selected endpoint to fail before transport. - -Update the architecture/internal overview for domain endpoint invariants and -the OpenAI-compatible integration and internal LLM documents for URL behavior. -Run focused domain/backend/profile/use-case/LLM/root tests, repetition and race -tests where ownership crosses packages, followed by repository tests and vet. - -## Stage 20: Bound And Strictly Frame Successful Provider Responses - -**Findings:** S14-F04, S14-F05. - -Enforce the 16 MiB successful-response decision without first copying the -entire body. Reject an over-limit `Content-Length` immediately, but also wrap -the body in a counting/limited reader that reads at most one byte beyond the -limit so chunked or dishonest responses cannot bypass it. Exactly-limit bodies -remain valid. Always close the body; do not drain an unbounded oversized -stream. - -Decode exactly one response object. After the first decode, require only -trailing JSON whitespace and EOF. A second value, non-whitespace suffix, -truncated body, malformed JSON, or size overflow returns -`ErrMalformedResponse` with no partial response and no provider content in the -error. - -Add streaming tests just below, at, and one byte over the limit with and -without `Content-Length`, plus a continuing oversized stream. Assert bounded -bytes read, timely return, no partial result, and closure. Add trailing -whitespace success and trailing garbage/second-value failures. Retain ordinary -response mapping and redaction cases. - -Document the fixed successful-response boundary and strict one-document rule -in the integration and internal LLM documents. Explicitly leave bounded -non-success error-envelope parsing to the structured-generation-error roadmap. -Run focused transport tests normally, repeatedly, and under race, followed by -repository tests and vet. - -## Stage 21: Consolidate Transport Test Scaffolding - -**Finding:** S14-F06. - -After transport behavior is stable, introduce one small recording-provider -fixture for common request capture and successful/error response setup. -Organize focused tables around request mapping, authentication, endpoint -composition, timeout/error identity, and response framing. Keep specialized -round trippers/readers for cancellation, deadlines, byte counts, continuing -streams, and body closure. - -Retain every existing durable assertion for method, path, headers, -authentication, omission and explicit presence, reserved fields, cache -control, structured output, response mapping, usage, error redaction, and -timeout precedence. Retain all Stage 18 through 20 regressions. Delete repeated -servers, generic-map decoding, and response literals only where the fixture -makes the owning behavior clearer; do not replace wire assertions with a broad -snapshot. - -Run the LLM suite normally, with shuffle/repetition, and under the race -detector. Deliberately inspect the resulting test inventory against the audit's -transport matrix before running repository tests and vet. - -## Stage 22: Restore One Canonical Maintainer Validation Workflow - -**Finding:** S16-F01. - -Make `docs/development.md` the canonical owner of the complete local maintainer -workflow, as assigned by the documentation policy. Its validation section must -include, from the repository root: - -```sh -go test ./... -go test -race ./... -go vet ./... -go build ./... -go run ./examples/go-library/prepare -go run ./examples/go-library/run -``` - -It must also own the Go formatting, local Markdown link, `git diff --check`, -workspace/vendor/replacement, generated-output, credential, and working-tree -hygiene checks used before accepting changes. - -Change the testing policy to state the semantic requirements and link to that -canonical workflow instead of maintaining a partial competing command list. -Change the release procedure to invoke the development-guide validation as a -release prerequisite rather than presenting a separately maintained copy; -retain release-specific metadata, candidate, tag, and publication commands in -the release document. - -Run both examples offline and confirm that a missing or invalid Run example -fixture makes its command fail. Validate all changed Markdown links and ensure -current-state documentation describes only the implemented workflow. - -## Stage 23: Complete Traceability And Final Validation - -This final stage introduces no new behavior. Review the final tree against the -finding-to-stage table below and the evidence in `audit.md`. Confirm every -canonical group is implemented and every source-specific symptom retains its -required regression and error boundary. Do not mark a finding resolved merely -because a nearby refactor landed. - -Run the complete development-guide workflow, including both examples, all -formatting and link checks, and repository hygiene. Also run shuffled ordinary -tests and repeated race-enabled tests for the changed concurrency, -cancellation, prepared, validation, repair, and transport packages. Run the -accepted performance benchmarks for prompt lookup, profile lookup, rendering, -and JSON validation and record only qualitative before/after conclusions; do -not establish release timing promises. - -Inspect canonical GoDoc, formats, integration, architecture, and internal -documents against the final implementation. Confirm the public engine still -performs no output repair and the future repair entry remains future work. -Confirm the structured-generation-error feature was not implemented as part of -transport remediation. - -Leave `audit-sequence.md`, `audit.md`, and this plan in place for maintainer -review. Retire them only in a separately authorized roadmap-cleanup pass after -the remediation has been reviewed and accepted. - -## Finding-To-Stage Traceability - -| Stage | Canonical findings | Historical or source-specific records handled with the canonical owner | -| ---: | --- | --- | -| 1 | S05-F01, S17-F01, S14-F02 | S02-F02, S08-F01, S11-F01 | -| 2 | S17-F02 | S11-F02 | -| 3 | S05-F02, S05-F03, S05-F04 | None | -| 4 | S02-F01, S02-F05, S05-F05 | None | -| 5 | S01-F01, S02-F03, S02-F04, S13-F01 | None | -| 6 | S03-F01, S03-F02, S06-F01 | None | -| 7 | S07-F01 | None | -| 8 | S07-F02, S07-F03, S07-F04, S07-F06 | None | -| 9 | S07-F05 | None | -| 10 | S08-F02, S08-F03, S08-F04, S08-F05 | S08-F01 was handled in Stage 1 | -| 11 | S08-F06 | None | -| 12 | S09-F01, S09-F02, S09-F05 | None | -| 13 | S09-F03, S09-F04 | None | -| 14 | S10-F01, S10-F05 | None | -| 15 | S10-F02, S10-F03 | None | -| 16 | S10-F04 | None | -| 17 | S12-F01, S12-F02, S12-F03 | None | -| 18 | S14-F01, S04-F01, S16-F02 | S14-F02 was handled in Stage 1 | -| 19 | S14-F03 | None | -| 20 | S14-F04, S14-F05 | None | -| 21 | S14-F06 | None | -| 22 | S16-F01 | None | - -The table maps all 49 canonical remediation groups exactly once. S02-F02 is -the one superseded historical finding retained as evidence under S17-F01; -S08-F01, S11-F01, and S11-F02 retain their source-specific regression -responsibilities without being double-counted as canonical groups. - -## Open Questions - -None. The numeric contract, resource bounds, path and identity rules, -validation-cancellation limitation, repair retention, transport response -limit, and documentation ownership required to implement these stages are -fixed above. diff --git a/engine_test.go b/engine_test.go index 632ab79..f5ff1c4 100644 --- a/engine_test.go +++ b/engine_test.go @@ -1441,6 +1441,29 @@ func TestPromptRepositoryReadFailureMapsToPromptLoad(t *testing.T) { } } +func TestPromptDefinitionReadFailureMapsToPromptLoad(t *testing.T) { + readErr := errors.New("definition read failed") + source := &publicDefinitionReadFailureFS{ + FS: fstest.MapFS{ + "prompts/target.yaml": &fstest.MapFile{Data: []byte("unread")}, + }, + target: "prompts/target.yaml", + err: readErr, + } + engine, err := promptkit.NewEngine(promptkit.Config{}, promptkit.WithPromptFS(source, "prompts")) + if err != nil { + t.Fatalf("construct engine: %v", err) + } + + inspection, err := engine.InspectPrompt(context.Background(), "target", "1") + if inspection != nil || !errors.Is(err, promptkit.ErrPromptLoad) || !errors.Is(err, readErr) { + t.Fatalf("InspectPrompt() = (%#v, %v), want nil with prompt-load and read-error identities", inspection, err) + } + if errors.Is(err, promptkit.ErrPromptNotFound) { + t.Fatalf("definition read error was classified as absence: %v", err) + } +} + func TestSelectedProfileRepositoryReadFailureMapsToProfileLoad(t *testing.T) { missingProfileDir := filepath.Join(t.TempDir(), "missing-profiles") engine, err := promptkit.NewEngine(promptkit.Config{ @@ -2696,6 +2719,35 @@ func TestPreparePreservesValidationCancellationIdentity(t *testing.T) { } } +func TestPrepareRejectsExcessivelyDeepStructuredOutputSchema(t *testing.T) { + schema, err := json.Marshal(excessivelyDeepStructuredOutputSchema()) + if err != nil { + t.Fatalf("marshal schema: %v", err) + } + engine, err := promptkit.NewEngine(promptkit.Config{}, + promptkit.WithPromptFS(publicStructuredPromptFS("schema.deep.prompt", "schema.json"), "prompts"), + promptkit.WithSchemaFS(fstest.MapFS{ + "schemas/schema.json": &fstest.MapFile{Data: schema}, + }, "schemas"), + promptkit.WithProfiles(promptkit.Profile{ + ID: "contract-fast", Endpoint: "http://example.test/v1", Model: "schema-model", + }), + ) + if err != nil { + t.Fatalf("construct engine: %v", err) + } + + prepared, err := engine.Prepare(context.Background(), promptkit.RunRequest{ + PromptID: "schema.deep.prompt", + Inputs: map[string]promptkit.ArtifactRef{ + "transcript": promptkit.Inline("Rin opens the gate."), + }, + }) + if prepared != nil || !errors.Is(err, promptkit.ErrValidation) { + t.Fatalf("Prepare() = (%#v, %v), want nil with ErrValidation", prepared, err) + } +} + func TestPreparedStructuredOutputRetainsExactSchemaNumbers(t *testing.T) { const schema = `{ "type": "number", @@ -3124,6 +3176,15 @@ func excessivelyDeepJSONValue() any { return value } +func excessivelyDeepStructuredOutputSchema() any { + const clearlyUnsafeSchemaNesting = 128 + schema := any(map[string]any{"type": "object"}) + for level := 0; level < clearlyUnsafeSchemaNesting; level++ { + schema = map[string]any{"allOf": []any{schema}} + } + return schema +} + func excessivelyLargeJSONValue() any { const clearlyUnsafeSharedOccurrences = 60_000 shared := []any{true} @@ -3373,6 +3434,19 @@ type countingSchemaFS struct { reads map[string]int } +type publicDefinitionReadFailureFS struct { + fs.FS + target string + err error +} + +func (f *publicDefinitionReadFailureFS) Open(name string) (fs.File, error) { + if name == f.target { + return nil, f.err + } + return f.FS.Open(name) +} + type cancelingPublicSchemaFS struct { fs.FS mu sync.Mutex diff --git a/internal/promptdef/filesystem_repository.go b/internal/promptdef/filesystem_repository.go index 6e3c80e..8e6a828 100644 --- a/internal/promptdef/filesystem_repository.go +++ b/internal/promptdef/filesystem_repository.go @@ -117,7 +117,7 @@ func (r *sourceRepository) GetPromptDefinition(ctx context.Context, id string, v relPath := r.source.displayPath(fullPath) data, err := r.source.readDefinition(fullPath) if err != nil { - continue + return nil, fmt.Errorf("failed to read prompt definition file %s: %w", relPath, err) } raw, err := decodePromptDefinition(data) if err != nil { diff --git a/internal/promptdef/repository_test.go b/internal/promptdef/repository_test.go index f308f47..4b12132 100644 --- a/internal/promptdef/repository_test.go +++ b/internal/promptdef/repository_test.go @@ -460,6 +460,42 @@ type recordingFS struct { opened []string } +func TestPromptRepositoryReturnsDefinitionReadFailures(t *testing.T) { + readErr := errors.New("definition read failed") + fsys := &definitionReadFailureFS{ + FS: fstest.MapFS{ + "prompts/target.yaml": &fstest.MapFile{Data: []byte("unread")}, + }, + target: "prompts/target.yaml", + err: readErr, + } + repo := NewFSRepository(fsys, "prompts") + + definition, err := repo.GetPromptDefinition(context.Background(), "target", "1") + if definition != nil || !errors.Is(err, readErr) { + t.Fatalf("GetPromptDefinition() = (%#v, %v), want nil and definition read error", definition, err) + } + if errors.Is(err, ErrPromptDefinitionNotFound) { + t.Fatalf("definition read error was classified as absence: %v", err) + } + if !strings.Contains(err.Error(), "target.yaml") { + t.Fatalf("definition read error lacks source context: %v", err) + } +} + +type definitionReadFailureFS struct { + fs.FS + target string + err error +} + +func (f *definitionReadFailureFS) Open(name string) (fs.File, error) { + if name == f.target { + return nil, f.err + } + return f.FS.Open(name) +} + func (f *recordingFS) Open(name string) (fs.File, error) { f.mu.Lock() f.opened = append(f.opened, name) diff --git a/internal/usecase/prepared_execution_test.go b/internal/usecase/prepared_execution_test.go index e00d2f5..d94df64 100644 --- a/internal/usecase/prepared_execution_test.go +++ b/internal/usecase/prepared_execution_test.go @@ -170,34 +170,65 @@ func TestRunnerPrepareExecutionCompletesWithoutAdmissionOrGeneration(t *testing. } } -func TestRunnerPrepareExecutionRejectsExcessivelyDeepPreparedSchema(t *testing.T) { - def := promptDef(domain.FormatJSON, domain.ValidationJSONSchema, 0) - def.Validation.SchemaPath = "schema.json" - llmClient := &fakeLLM{forbid: true} - validator := &recordingValidationPreparer{ - plan: &recordingPreparedValidation{schemaDocument: excessivelyDeepPreparedJSONValue()}, +func TestRunnerPreparationRejectsExcessivelyDeepPreparedSchema(t *testing.T) { + operations := []struct { + name string + run func(*Runner, domain.RunRequest) error + }{ + { + name: "Prepare", + run: func(runner *Runner, request domain.RunRequest) error { + _, err := runner.Prepare(context.Background(), request) + return err + }, + }, + { + name: "Run", + run: func(runner *Runner, request domain.RunRequest) error { + _, err := runner.Run(context.Background(), request) + return err + }, + }, + { + name: "PrepareExecution", + run: func(runner *Runner, request domain.RunRequest) error { + _, err := runner.PrepareExecution(context.Background(), request) + return err + }, + }, } - runner := NewRunner( - &fakePromptRepo{def: def}, - &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{"exec": defaultExecutionProfile()}}, - nil, - defaultArtifactReader(), - defaultRenderer(), - llmClient, - validator, - nil, - ) - _, err := runner.PrepareExecution(context.Background(), domain.RunRequest{ - PromptID: "p", - ProfileID: "exec", - Inputs: singleInputRef(), - }) - if !errors.Is(err, ErrInvalidRequest) { - t.Fatalf("expected ErrInvalidRequest, got %v", err) - } - if llmClient.calls != 0 { - t.Fatalf("invalid prepared schema reached generation: %d calls", llmClient.calls) + for _, operation := range operations { + t.Run(operation.name, func(t *testing.T) { + def := promptDef(domain.FormatJSON, domain.ValidationJSONSchema, 0) + def.Validation.SchemaPath = "schema.json" + llmClient := &fakeLLM{forbid: true} + validator := &recordingValidationPreparer{ + plan: &recordingPreparedValidation{schemaDocument: excessivelyDeepPreparedJSONValue()}, + } + runner := NewRunner( + &fakePromptRepo{def: def}, + &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{"exec": defaultExecutionProfile()}}, + nil, + defaultArtifactReader(), + defaultRenderer(), + llmClient, + validator, + nil, + ) + + err := operation.run(runner, domain.RunRequest{ + PromptID: "p", + ProfileID: "exec", + Inputs: singleInputRef(), + }) + if !errors.Is(err, ErrValidation) { + t.Fatalf("expected ErrValidation, got %v", err) + } + if llmClient.calls != 0 { + t.Fatalf("invalid prepared schema reached generation: %d calls", llmClient.calls) + } + }) } } diff --git a/internal/usecase/runner.go b/internal/usecase/runner.go index ff88113..68e655d 100644 --- a/internal/usecase/runner.go +++ b/internal/usecase/runner.go @@ -17,6 +17,7 @@ import ( "gitea.maximumdirect.net/eric/promptkit/internal/capacity" "gitea.maximumdirect.net/eric/promptkit/internal/defaults" "gitea.maximumdirect.net/eric/promptkit/internal/domain" + "gitea.maximumdirect.net/eric/promptkit/internal/jsonvalue" "gitea.maximumdirect.net/eric/promptkit/internal/llm" "gitea.maximumdirect.net/eric/promptkit/internal/profile" "gitea.maximumdirect.net/eric/promptkit/internal/prompt" @@ -398,6 +399,10 @@ func (r *Runner) structuredOutputFromValidationPlan( } return nil, fmt.Errorf("%w: prepared json_schema validation has no schema document", ErrValidation) } + schemaDocument, err := jsonvalue.Copy(schemaDocument) + if err != nil { + return nil, fmt.Errorf("%w: invalid prepared json_schema schema document: %v", ErrValidation, err) + } return structuredOutputSpec(def, schemaDocument), nil }