Record responsibility audit decisions and ownership guidance
This commit is contained in:
@@ -16,10 +16,16 @@ checked against implementations and assembly; test-only callers were considered
|
||||
separately. Dependency public contracts informed delegation choices; dependency
|
||||
implementations were not audited. No production code or tests were changed.
|
||||
|
||||
These are architectural candidates, not a claim that every candidate is a bug
|
||||
or release blocker. Existing explicit decisions remain authoritative. A test,
|
||||
private interface, or historical implementation alone does not establish an
|
||||
additional consumer contract.
|
||||
These findings are not a claim that every candidate is a bug or release blocker.
|
||||
The evidence describes the audited baseline; subsequent review decisions are
|
||||
recorded below. Findings 1–3 and 5–7 are accepted for remediation, and finding 8
|
||||
is accepted as a decision to preserve current runtime behavior. Implementation
|
||||
has not begun under this document. Finding 4 remains open in its entirety,
|
||||
including both the smoke syntax check and standalone validation retirement.
|
||||
|
||||
Existing explicit decisions remain authoritative except where a later accepted
|
||||
decision deliberately changes them. A test, private interface, or historical
|
||||
implementation alone does not establish an additional consumer contract.
|
||||
|
||||
## Current Architecture
|
||||
|
||||
@@ -44,6 +50,8 @@ and repeated acceptance checks on already captured values.
|
||||
|
||||
### 1. Remove Unused Client-Level Endpoint And Model Defaults
|
||||
|
||||
**Status:** Accepted; implementation pending.
|
||||
|
||||
**Confidence:** High. **Owner:** resolved execution settings belong to
|
||||
configuration/request resolution; HTTP transport consumes the resolved target.
|
||||
|
||||
@@ -71,6 +79,8 @@ with the behavior, rather than sustain a second settings-resolution layer.
|
||||
|
||||
### 2. Build One Root Wire Payload Instead Of Mapping It Twice
|
||||
|
||||
**Status:** Accepted; implementation pending.
|
||||
|
||||
**Confidence:** High. **Owner:** the LLM adapter owns OpenAI-compatible encoding.
|
||||
|
||||
**Evidence:** `PrepareGeneration` calls `openAIChatRequestFromGenerateRequest`,
|
||||
@@ -95,6 +105,8 @@ protect the wire contract rather than the deleted helper arrangement.
|
||||
|
||||
### 3. Retire Alternate Uncaptured Lookup And Rendering Paths
|
||||
|
||||
**Status:** Accepted; implementation pending.
|
||||
|
||||
**Confidence:** High about reachability; exact private interface layout remains
|
||||
an implementation choice. **Owner:** configuration captures definitions,
|
||||
metadata, hashes, and templates; the runner consumes that captured state.
|
||||
@@ -141,6 +153,9 @@ without maintaining a second rendering or repository-resolution workflow.
|
||||
|
||||
### 4. Use The Native JSON Syntax Check In Smoke Diagnostics
|
||||
|
||||
**Status:** Open for discussion. Neither subproposal below is approved; retain
|
||||
the existing workflow pending that discussion.
|
||||
|
||||
**Confidence:** High for the syntax-check simplification. **Owner:** smoke
|
||||
diagnostics report syntax validity separately from schema conformance.
|
||||
|
||||
@@ -157,16 +172,19 @@ result projection. Preserve cancellation checks and the distinction between
|
||||
invalid JSON and schema-not-evaluated. JSON scalars remain valid syntax even
|
||||
when they fail the diagnostic schema.
|
||||
|
||||
**Approach:** Use the standard-library syntax check at that diagnostic operation
|
||||
and retain the prepared schema plan for schema conformance.
|
||||
**Proposed syntax-check approach:** Use the standard-library syntax check at
|
||||
that diagnostic operation and retain the prepared schema plan for schema conformance.
|
||||
|
||||
There is a related larger deletion candidate: concrete host/`fs.FS` validators
|
||||
still expose direct `Validate(artifact, contract)` methods and separate on-demand
|
||||
schema-loading callbacks. The runner now consumes only `ValidationPreparer`.
|
||||
The identified production direct-validation call is the smoke JSON-only call;
|
||||
direct schema workflows are exercised by internal tests, not public injection.
|
||||
After removing the smoke mediation, consider retiring that standalone lifecycle
|
||||
and moving its meaningful source/schema cases to prepared-plan validation.
|
||||
|
||||
**Proposed lifecycle approach:** Retire the standalone validation lifecycle
|
||||
if the smoke syntax simplification is selected, moving meaningful source/schema
|
||||
cases to prepared-plan validation. This is a separate choice from the direct
|
||||
syntax check, even though it depends on removing that production caller.
|
||||
|
||||
The previous roadmap explicitly retained direct validation and its
|
||||
decode-before-schema-I/O behavior. Retiring it is therefore a deliberate change
|
||||
@@ -175,8 +193,24 @@ It need not change the public API or make schema errors lazy again. Keep shared
|
||||
instance validation and source-specific root loading; do not duplicate them in
|
||||
tests to preserve the removed convenience interface.
|
||||
|
||||
**Review recommendation, not yet accepted:** Select both simplifications. The
|
||||
syntax check removes an object/interface used only to report a Boolean fact;
|
||||
prepared validation already serves public execution. Retaining the standalone
|
||||
lifecycle solely for its tests would preserve an alternate private workflow.
|
||||
|
||||
**Viable alternatives:** Keep the existing smoke validator mediation to reuse
|
||||
its cancellation/result handling, at the cost of unnecessary setup and projection
|
||||
for a syntax-only check. Alternatively, use `json.Valid` in smoke diagnostics but
|
||||
retain direct validation as an internal convenience. That avoids migrating its
|
||||
tests and preserves decode-before-schema-I/O behavior for private callers, but
|
||||
retains a second lifecycle without an identified production need. The next
|
||||
discussion should settle these choices explicitly, not infer approval from the
|
||||
other accepted findings.
|
||||
|
||||
### 5. Separate Accepted-Value Detachment From Repeated Input Validation
|
||||
|
||||
**Status:** Accepted; implementation pending.
|
||||
|
||||
**Confidence:** High for repeated work; each replacement needs a clear
|
||||
acceptance/ownership argument. **Owner:** acceptance checks belong where a new
|
||||
value enters; copies protect the boundary at which mutable ownership changes.
|
||||
@@ -215,11 +249,22 @@ configuration, and preserve accepted number/type/nil semantics. Do not move the
|
||||
retired convenience-source compatibility converter back into the shared copier
|
||||
or combine the distinct walkers into a configurable reflection framework.
|
||||
|
||||
Schema-document budgets are a separate contract question below; they should
|
||||
not be silently removed as a side effect of replacing a copy operation.
|
||||
Findings 5–6 should be planned together. For each affected path, record a small
|
||||
ownership inventory: where the final value is accepted, whether it can be mutated
|
||||
afterward, and where it crosses into caller/client-owned mutable state. Check
|
||||
prepared details, execution results, injected-client requests, repair requests,
|
||||
and smoke reports together. Do not blanket-replace `CopyMap` with `CloneTrusted`
|
||||
or remove a copy only to relocate it into another helper. This analysis does not
|
||||
require a new ownership framework.
|
||||
|
||||
Schema-document budgets are addressed by the explicit accepted decision in
|
||||
finding 7, not an accidental side effect of replacing a copy operation. Runtime
|
||||
extra-parameter acceptance and limits remain separate.
|
||||
|
||||
### 6. Retain One Private Prepared Metadata Snapshot And One Outward Copy
|
||||
|
||||
**Status:** Accepted; implementation pending.
|
||||
|
||||
**Confidence:** High. **Owner:** prepared execution owns single claim/discard,
|
||||
private execution settings, and independent public details.
|
||||
|
||||
@@ -245,7 +290,8 @@ Single-use state, owner identity, and synchronization remain necessary.
|
||||
|
||||
**Approach:** Retain one immutable, redacted private metadata snapshot shared
|
||||
by details and the execution payload. Keep execution-only settings separately
|
||||
clearable. Let the facade perform the one necessary public projection/copy
|
||||
clearable. Remove the payload-cleanup mutation of metadata before sharing the
|
||||
snapshot. Let the facade perform the one necessary public projection/copy
|
||||
from that snapshot. Avoid an internal accessor that copies solely for another
|
||||
copying accessor, including its currently hidden copy-failure-to-nil branch.
|
||||
Preserve frozen schema/message/input state and the existing injected-client
|
||||
@@ -256,6 +302,9 @@ lifecycle framework is needed.
|
||||
|
||||
### 7. Decide Whether Prepared Schemas Own A JSON-Tree Resource Budget
|
||||
|
||||
**Status:** Accepted: remove the incidental copier-derived schema budget;
|
||||
implementation pending.
|
||||
|
||||
**Evidence:** `structuredOutputFromValidationPlan` in
|
||||
[the runner](../../../internal/usecase/runner.go) feeds a schema already decoded
|
||||
and compiled by the validator through `jsonvalue.Copy`. Prepared details cloning
|
||||
@@ -271,21 +320,29 @@ review did not establish a separate user decision adopting a schema budget.
|
||||
The deliberately selected runtime-extra-parameter limits are not automatically
|
||||
authority for applying the same limits to loaded schema documents.
|
||||
|
||||
**Recommendation:** Treat copying as copying, and let the existing schema
|
||||
decoder/compiler own schema acceptance unless a separate schema-resource policy
|
||||
is intentionally chosen. Remove the incidental extra rejection if that is the
|
||||
selected decision, retaining detachment and the self-contained-schema rule.
|
||||
**Decision:** Treat copying as copying and let the existing schema
|
||||
decoder/compiler own schema acceptance, retaining detachment and PromptKit's selected
|
||||
self-contained-schema rule. Remove the additional depth/node rejection inherited
|
||||
from `jsonvalue.Copy` rather than adopt it as a schema-resource policy.
|
||||
This could admit schemas previously rejected solely by the copier; it is not
|
||||
a promise of unlimited dependency capacity, stack safety, or provider support.
|
||||
|
||||
**Alternative:** Deliberately adopt a schema budget and enforce it once at the
|
||||
schema acceptance boundary, with appropriate public documentation and focused
|
||||
**Alternative considered, not selected:** Deliberately adopt a schema budget
|
||||
and enforce it once at the schema acceptance boundary, with appropriate public documentation and focused
|
||||
behavioral coverage. This keeps an explicit resource-control policy but adds a
|
||||
consumer restriction and locally owned traversal responsibility. Do not choose
|
||||
it solely to make an existing copier convenient or to keep a fake-plan test.
|
||||
|
||||
**Follow-through:** Update tests and internal prose that pin the copier-derived
|
||||
schema rejection. Preserve schema compilation, completeness checks, and outward
|
||||
mutation isolation. Record the broadened acceptance of schemas previously
|
||||
rejected only by that copier, without adding a new precision, unlimited-resource,
|
||||
or provider-compatibility guarantee. Runtime provider-value budgets are unchanged.
|
||||
|
||||
### 8. Broad Runtime Go-Value Support Is A Real Choice, Not ConfigKit Duplication
|
||||
|
||||
**Status:** Accepted: preserve current runtime behavior in this work.
|
||||
|
||||
**Evidence:** [`ExecutionTargetOverride.ExtraParams`](../../../types.go) accepts
|
||||
JSON-compatible arrays/slices/maps and numbers. The
|
||||
[JSON-value implementation](../../../internal/jsonvalue/jsonvalue.go) also
|
||||
@@ -301,13 +358,13 @@ distinction should not be promoted into an additional promise solely because
|
||||
the implementation/tests currently preserve it. The previous remediation
|
||||
expressly left runtime override behavior unchanged.
|
||||
|
||||
**Recommendation:** Preserve the current runtime behavior while addressing
|
||||
**Decision:** Preserve the current runtime behavior while addressing
|
||||
revalidation and duplicate snapshots in findings 5–6. There is no demonstrated
|
||||
need in this review to narrow the supported input forms. Do not delegate runtime
|
||||
values through ConfigKit merely to reuse its source-input restrictions.
|
||||
|
||||
**Alternative:** If this remaining reflection/type-preservation cost becomes a
|
||||
priority, deliberately standardize runtime inputs on a smaller JSON-native value
|
||||
**Alternative considered, not selected for this work:** If the remaining
|
||||
reflection/type-preservation cost becomes a priority, deliberately standardize runtime inputs on a smaller JSON-native value
|
||||
contract or normalize accepted typed containers to canonical containers. This
|
||||
could reduce copier complexity, but callers using typed values may need explicit
|
||||
conversion, and injected clients may observe changed dynamic types. Number
|
||||
@@ -355,12 +412,23 @@ were made for this review.
|
||||
Begin with the clearly private transport defaults/root-payload simplification.
|
||||
Then align private lookup/rendering collaborators with captured configuration.
|
||||
Address copy validation and prepared snapshot ownership together so that deleting
|
||||
one copy does not relocate it into another helper. Simplify smoke syntax
|
||||
checking and deliberately decide the fate of direct validation. Resolve schema
|
||||
budget authority before changing that rejection behavior; narrowing runtime
|
||||
Go-value support is not a prerequisite for the other improvements.
|
||||
one copy does not relocate it into another helper. Implement schema-budget
|
||||
removal under finding 7's explicit decision, preserving runtime behavior under
|
||||
finding 8. Finding 4's syntax-check and direct-validation proposals remain open;
|
||||
do not include them as selected implementation work before discussion.
|
||||
|
||||
Any implementation plan should include retiring the corresponding private-path
|
||||
tests and internal prose while retaining meaningful public/ownership regressions.
|
||||
This audit authorizes no contract change, new limits, general compatibility
|
||||
framework, performance guarantee, or additional release gate.
|
||||
The accepted decisions above define the selected remediation scope; they do not
|
||||
authorize additional contract changes, new limits, a general compatibility
|
||||
framework, performance guarantees, or additional release gates. The retained
|
||||
machinery table remains applicable, including artifact defensive copying,
|
||||
prepared-handle lifecycle, and separate admission/active-permit responsibilities.
|
||||
|
||||
## Open Decisions
|
||||
|
||||
Only [finding 4](#4-use-the-native-json-syntax-check-in-smoke-diagnostics) remains
|
||||
open: whether to use `json.Valid` directly in smoke diagnostics and whether to
|
||||
retire the standalone direct-validation lifecycle. Its proposed recommendation
|
||||
and viable alternatives are recorded with the finding. All other findings have
|
||||
an accepted disposition; none is marked implemented by this documentation update.
|
||||
|
||||
Reference in New Issue
Block a user