Record behavioral equivalence audit findings
This commit is contained in:
249
docs/roadmap/audit/10-equivalence.md
Normal file
249
docs/roadmap/audit/10-equivalence.md
Normal file
@@ -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.
|
||||
Reference in New Issue
Block a user