From 1d1b04e2e00851549b9ffa7c72f4a8b60bb67ba0 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 16:22:09 +0000 Subject: [PATCH] Record ordinary execution and repair audit findings --- audit.md | 279 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 279 insertions(+) diff --git a/audit.md b/audit.md index ecde8f8..88cef61 100644 --- a/audit.md +++ b/audit.md @@ -2856,3 +2856,282 @@ confirmed that: - 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.