From 32e7a3557cc3934930c1afdecdc23b9bf2d808d4 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 16:31:05 +0000 Subject: [PATCH] Document prepared execution lifecycle audit --- audit.md | 178 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 178 insertions(+) diff --git a/audit.md b/audit.md index 88cef61..1f61042 100644 --- a/audit.md +++ b/audit.md @@ -3135,3 +3135,181 @@ that: 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.