250 lines
14 KiB
Markdown
250 lines
14 KiB
Markdown
# 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.
|