Document prepared execution lifecycle audit
This commit is contained in:
178
audit.md
178
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.
|
||||
|
||||
Reference in New Issue
Block a user