From 0678d242b9429cd402707c887835ba71e8af24ea Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 14:43:20 +0000 Subject: [PATCH] Record engine operation audit findings --- audit.md | 161 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) diff --git a/audit.md b/audit.md index 92ccc74..4f04cee 100644 --- a/audit.md +++ b/audit.md @@ -583,3 +583,164 @@ S03-F01. The temporary source and generated file were removed. 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.