From b39460a507f4a0243535232cd73b31fde4f26d3d Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 4 Nov 2026 16:06:17 +0000 Subject: [PATCH] Record behavioral equivalence audit findings --- docs/roadmap/audit/10-equivalence.md | 249 +++++++++++++++++++++++++++ 1 file changed, 249 insertions(+) create mode 100644 docs/roadmap/audit/10-equivalence.md diff --git a/docs/roadmap/audit/10-equivalence.md b/docs/roadmap/audit/10-equivalence.md new file mode 100644 index 0000000..83790bd --- /dev/null +++ b/docs/roadmap/audit/10-equivalence.md @@ -0,0 +1,249 @@ +# Behavioral Equivalence And Shared Abstraction Audit + +Reviewed 2026-11-04 against commit `5177db0` and the current worktree. Existing +audit-document moves were preserved. No production code or repository tests +were changed. + +## Assessment And Method + +The public package adapts consumer values into internal configuration and +execution components. ConfigKit owns source composition; PromptKit interprets +its final handoff and captures indexes and templates. The runner shares +preparation and execution machinery across public workflows, generation owns +attempts and retries, and capacity owns admission and permits. Domain rules, +validation, JSON-value ownership, and observation have existing shared owners. +The architecture does not need another public abstraction or a general-purpose +utility package to address the opportunities below. + +This review started from tests: direct cross-path comparisons, common test +matrices, and separately asserted matching behavior. It then examined the +corresponding implementation and callers to distinguish a shared responsibility +from similar-looking code. Matching syntax, similar coverage, or duplicated +test setup alone did not establish an extraction opportunity. + +Three bounded refactoring opportunities warrant consideration. They provide +test-backed scope for findings 1, 4, and 3 respectively in the +[complexity audit](03-complexity.md), rather than three additional independent +bugs or duplicate remediation projects. None is a newly demonstrated behavioral +failure or an automatic release blocker. Existing behavior, not a stronger +equivalence promise, is the preservation target. + +## 1. Share The Schema Compilation Lifecycle Across Source And Execution Paths + +### Evidence + +- [TestJSONSchemaNumericConstraintsRetainJSONPrecision](../../../internal/validate/standard_validator_test.go:721) + runs the same exact-number constraints through operating-system and `fs.FS` + validators using `jsonSchemaValidatorSources`. Both must distinguish adjacent + large integers, preserve precise decimal boundaries, and accept the same + valid instances. +- [TestJSONSchemaDecodingRequiresOneCompleteDocument](../../../internal/validate/standard_validator_test.go:805) + uses that same source matrix to require equivalent schema/instance framing + and the distinction between operational schema errors and rejected output. +- [TestSchemaReferencesPreserveEscapedFilenames](../../../internal/validate/standard_validator_test.go:492) + applies the same reference cases to both source implementations, with an + explicit platform exception for filenames illegal on Windows. +- [TestStandardValidatorPreparedSchemaSurvivesSourceRemoval](../../../internal/validate/standard_validator_test.go:228) + and [TestFSValidatorPreparedSchemaSurvivesSourceMutation](../../../internal/validate/standard_validator_test.go:416) + separately require the same captured schema to accept an integer-valued + object and reject a string-valued one after its source changes. Their source + operations differ, but the prepared validation responsibility matches. + +These tests support common schema semantics and compilation ownership; they do +not require identical filesystem representations or diagnostic strings. + +### Implementation + +[StandardValidator and FSValidator](../../../internal/validate/standard_validator.go:74) +each implement `PrepareValidation` and direct `validateJSONSchema`. The four +paths repeat compiler setup, root-resource handling, cancellation boundaries, +compilation/error wrapping, and construction or use of executable schema +state. The direct host-file path lets its loader fetch the root, whereas the +other paths explicitly load/register it. Decoding, compiler construction, +schema execution, and content-result classification already have shared helpers; +the remaining duplication is the lifecycle around them. + +### Opportunity And Potential Remediation + +Keep source-specific resolution/loading adapters, but introduce one private +compilation operation in `internal/validate` that produces the root document +and compiled executor for the caller's selected source. Reuse it from prepared +validation and from the direct schema-validation callback. Retain the already +shared artifact-validation/result logic. This makes future dialect, framing, +number-preservation, and cancellation changes less likely to land in only some +of the four paths. + +The existing package is the appropriate owner; neither a public compiler API +nor a generic filesystem framework is needed. The related runner dependency +clarification remains part of complexity-audit finding 1. + +### Tradeoffs And Test Independence + +Direct validation currently rejects malformed output JSON before accessing its +schema. Eagerly implementing it as `PrepareValidation` followed by plan +validation would change I/O and error ordering. Share compilation when that +work is reached, not necessarily the entire entry point. Preserve contained +host paths and symlink handling, logical `fs.FS` paths, escaped resource URLs, +single-file sources, synchronous cancellation precedence, and operation-local +prepared freezing. These source differences belong in their adapters. + +Keep the source matrix and independently specified valid/invalid instances. +Do not compute expected results using the new compilation helper or delete +source-specific containment/cancellation tests merely because compilation is +shared. Provider-facing packaging of external schema references is a separate +correctness finding, not solved by this extraction. + +## 2. Centralize Structural Copying Of Already-Normalized JSON Values + +### Evidence + +- [TestPublicLLMClientAdapterGivesClientOwnedNestedValues](../../../llm_adapter_internal_test.go:14) + requires nested provider maps, schema values, messages, and cache metadata to + remain independent when an injected client mutates and retains its request. +- [TestRunnerPreparedExecutionDetachesTransferredSnapshotSources](../../../internal/usecase/prepared_execution_test.go:634) + separately requires nested provider maps and schema values to survive source + mutation, repeated `Details` calls, and execution without aliasing. +- [TestCopyHandlesIndirectionAndIsolatesNestedMutations](../../../internal/jsonvalue/jsonvalue_test.go:156) + exercises the underlying JSON-value ownership responsibility with typed + collections and repeated acyclic children. The adjacent scalar/collection + tests protect compatible concrete types and exact `json.Number` values. +- [TestCopyAndCopyMapApplyDistinctEmptyKeyRules](../../../internal/jsonvalue/jsonvalue_test.go:204) + explicitly demonstrates a necessary policy distinction: general schema + objects can have empty keys, while provider maps cannot. + +The common requirement is an independently owned copy of accepted nested +values. These tests do not establish universal equivalence of validation, +normalization, and copying on arbitrary Go values. + +### Implementation + +The facade's [copyAny/copyReflectValue](../../../convert.go:406) implements a +separate recursive reflection copier for values projected into client requests, +prepared details, results, and profile inspections. Internal prepared-state +cloning uses [clonePreparedRun](../../../internal/usecase/prepared_execution.go:226) +and [jsonvalue.Copy/CopyMap](../../../internal/jsonvalue/jsonvalue.go:30). +The latter package already owns compatible JSON-tree copying and validation. + +Ordinary runtime provider values reach facade projection after JSON-value +validation/copying. The runner also copies its schema document through +[structuredOutputFromValidationPlan](../../../internal/usecase/runner.go:419) +before projecting it. Thus these normal runtime paths have a common accepted +value domain, even though their entry-point policies are different. + +### Opportunity And Potential Remediation + +Move ownership of trusted JSON-tree cloning into `internal/jsonvalue`, reusing +its compatible structural-copy machinery where semantics match, so the facade +does not maintain another general reflection traversal. Keep public/domain DTO +field projection in the facade. A small internal trusted-value clone operation +is a possible approach; reuse of existing fallible operations is another where +the caller can preserve the existing error boundary. Select the simpler option +after inventorying the concrete callers and values, rather than introducing a +configurable reflection framework. + +The meaningful payoff is removing an independent traversal implementation, +not wrapping the existing facade copier under another name. This does not +require consolidating every pointer or slice helper in the repository. + +### Tradeoffs And Test Independence + +`Copy` validates and bounds values and normalizes null/indirection; the facade +copier assumes prior acceptance and can preserve typed nils and pointers. +Neither is a drop-in replacement for the other on all possible inputs. +Preserve observed types, nil behavior, exact numbers, schema empty keys, and +caller ownership at the relevant boundary. An infallible projection must not +silently turn a new copy error into a zero value, a panic, or an additional +consumer restriction. + +The [configuration-boundary ADR](../../adr/0001-configuration-boundary.md) +also remains authoritative. `providerValueForMapping` serves a different, +pre-handoff source-encoding purpose and defers unsupported-value errors to +ConfigKit. It must not be routed through a rejecting runtime copier as part of +this work. Source number conversion is separately recorded in the correctness +audit; visual similarity between its reflection switch and a copier is not +enough to equate their contracts. + +Keep literal expected values and mutation-based assertions in both facade and +internal tests. A shared production copier must not also manufacture their +expected snapshots. Retain focused tests for policy differences rather than +replacing them with one “all copy functions are equivalent” test. + +## 3. Share If/With Requirement-Branch Orchestration, Not Template Execution + +### Evidence + +- [TestAnalyzeRequirementsClassifiesKnownConditions](../../../internal/prompt/requirements_test.go:268) + verifies required/optional classification for selected and skipped `if` + branches, and directly compares the recognized branch choice with the real + Go renderer. This anchors recognized condition semantics in actual rendering. +- [TestAnalyzeRequirementsHandlesWithDotAndNestedReachability](../../../internal/prompt/requirements_test.go:332) + requires the same selected/skipped classification for `with` and nested + `if`, while separately protecting `with`'s changed dot and preserved root + caller-map access. +- [TestAnalyzeRequirementsReportsUnresolvedDecisionAndContentConditions](../../../internal/prompt/requirements_test.go:369) + protects conditional dependencies and meaningful incompleteness for unknown + decisions. It also establishes that an unresolved condition around plain + text alone does not make discovery incomplete. +- [TestAnalyzeRequirementsDoesNotHideCallerMapScopes](../../../internal/prompt/requirements_test.go:14) + compares several scope forms against actual template execution while allowing + incomplete analysis for constructs outside recognition scope. + +The tests support one reachability/evidence policy for recognizable branches, +with explicit scope differences. They do not require the inspector to execute +or completely interpret arbitrary templates. + +### Implementation + +The `IfNode` and `WithNode` cases in +[requirementWalker.visit](../../../internal/prompt/requirements.go:451) +independently perform inactive-path traversal, condition evaluation, then/else +reachability selection, dependency-counter comparison, and limitation recording. +`with` adds a different body-dot calculation. `branchReach`, condition +evaluation, and `template.IsTrue` already provide shared lower-level behavior, +but their surrounding branch protocol is repeated. + +### Opportunity And Potential Remediation + +Extract a small package-private branch traversal operation that owns the common +reachability and evidence sequence, with explicit body/else scope handling. +Leave node recognition and `withDotIdentity` visible at the node-specific +boundary. Keeping this in `internal/prompt` would allow a change to skipped-path +handling or incompleteness reporting to be reviewed and implemented once. + +This is the narrowly supported part of complexity-audit finding 3. It does not +justify sharing the requirement walker with the real template executor or +building an interpreter abstraction. + +### Tradeoffs And Test Independence + +An abstraction that treats `if` and `with` identically would lose caller-map +scope information. Preserve `with`'s body-dot behavior, else scope, inactive +computed-name handling, and the placement of dependency bookkeeping used to +decide whether an unresolved condition matters. A helper with numerous mode +flags or callbacks would obscure those differences; prefer a small explicit +operation over a generalized AST visitor framework. + +Retain separate node-specific assertions and the independent standard-library +rendering comparisons. Extracting production traversal is not a reason to +calculate expected inventories using the production branch helper or to +strengthen `Complete` into an execution-readiness guarantee. + +## Recommendation And Validation + +Start with schema compilation, which has the clearest shared lifecycle and +cross-source test matrix. The branch helper is a smaller local follow-up. +Treat JSON-copy consolidation as a more careful ownership refactor: remove a +traversal only after its caller/policy differences are accounted for. No new +package boundary is necessary solely to carry out these changes, and retaining +small source/boundary-specific adapters is appropriate. + +Existing offline tests passed with the local Go 1.27.0 toolchain: + +```sh +go test -race -count=1 ./internal/validate ./internal/prompt ./internal/jsonvalue +go test -race -count=1 -run 'Test(PublicLLMClientAdapter|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|RunnerPreparedExecutionDetachesTransferredSnapshotSources|RunnerRunAndPrepareResolveSameProfileAndEffectiveSettings|RunnerPreparationNormalizesOutputContractConsistently|ValidateRequestMatchesPrepareContract)' . ./internal/usecase +``` + +Passing tests substantiate the reviewed current behavior, not equivalence of a +future refactor. No new tests, production changes, live provider requests, +benchmarks, or additional release gates were introduced by this audit.