base-layer/promptkit
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 3 Wiki Activity

Record the architectural responsibility audit

Browse Source
This commit is contained in:
Eric Rakestraw
2026-11-05 18:10:01 +00:00
parent 1ff6d778af
commit cfa3a85308
1 changed files with 366 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

366
docs/roadmap/audit/A1-responsibility.md Normal file
View File

@@ -0,0 +1,366 @@
# A1: Responsibility And Removable Machinery
## Scope And Basis
Architectural review of repository-owned code at
`1ff6d778af5209a91d60a9452847c2db20dd7932`, on 2026-11-05. This review follows
the [agent](../../policy/agents.md), [architecture](../../policy/architecture.md),
and [testing](../../policy/testing.md) policies. It is independent of the
completed [earlier remediation](../remediation.md).
The review covered the public facade, internal package boundaries, engine
assembly, configuration capture and inspection, rendering and requirements,
artifact loading, generation/capacity, validation/repair, diagnostics,
observations/errors, examples, and release wiring. Graph caller searches were
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.
## Current Architecture
The root package is a public facade over an internal engine. ConfigKit owns
source composition, inheritance, and provenance; `internal/configuration`
interprets its final handoff and captures domain definitions, effective targets,
templates, inventories, and operator inspection. This follows
[ADR 0001](../../adr/0001-configuration-boundary.md).
Use cases coordinate offline preparation, execution, bounded output repair, and
explicit diagnostics. Generation owns built-in retry orchestration and active
permits; capacity also owns logical-execution admission. The LLM adapter owns
HTTP/wire behavior. The facade owns public value projection, mutable-value
detachment, credential redaction, and public error translation. The
[component overview](../../internal/overview.md) is the maintained inventory.
The overall separation is useful. The strongest simplification opportunities
are inside those boundaries: alternate private workflows, repeated projection,
and repeated acceptance checks on already captured values.
## Findings: Unnecessary Implementation Complexity
### 1. Remove Unused Client-Level Endpoint And Model Defaults
**Confidence:** High. **Owner:** resolved execution settings belong to
configuration/request resolution; HTTP transport consumes the resolved target.
**Evidence:** In the [LLM client](../../../internal/llm/openai_compatible_client.go),
`OpenAICompatibleConfig.BaseURL` and `.Model` become `baseURL` and `defaultModel`.
`PrepareGeneration` falls back to them when request fields are empty. The sole
non-test constructor caller, [engine assembly](../../../internal/engine/engine.go),
supplies only `Timeout` and `HTTPClient`. Normal runs and smoke inference already
provide a resolved target. The root configuration exposes no client-level
endpoint/model default independent of backend/profile/request settings.
**Removal impact:** Internal fixtures that put routing settings on the client
instead of the request would need updating. Public endpoint precedence, default
profiles, and runtime overrides do not need to change. Keeping these fields
does not currently supply an additional reachable consumer capability.
**Approach:** Delete those constructor fields, stored defaults, and fallback
branches; require the selected request target as production already does.
Remove the corresponding default-model argument from the wire builder. Keep
final endpoint/model checks and HTTP-client timeout/ownership behavior.
Update constructor-fallback tests to supply resolved targets, retaining useful
wire/invalid-target tests. The configured-base fallback prose in
[internal model-client documentation](../../internal/llm.md) should retire
with the behavior, rather than sustain a second settings-resolution layer.
### 2. Build One Root Wire Payload Instead Of Mapping It Twice
**Confidence:** High. **Owner:** the LLM adapter owns OpenAI-compatible encoding.
**Evidence:** `PrepareGeneration` calls `openAIChatRequestFromGenerateRequest`,
which creates an `openAIChatRequest`, then `openAIChatRequestPayload`, which
copies its fields into a `map[string]any` and flattens extra parameters. Only
`PrepareGeneration` calls either helper in production. The intermediate root
struct has no separate lifetime or ownership boundary; the map, not that
struct, is actually marshaled. Optional-field selection is expressed in both
representations.
**Removal impact:** The intermediate representation disappears, not provider
features. Explicit-zero presence, omissions, reserved-key rejection, session
IDs, structured output, and extension-field flattening still have to work.
Typed nested message/content-block/schema envelope values need not disappear.
**Approach:** Construct the root payload directly from the resolved request
and marshal it once, using the existing narrow nested wire values where useful.
Compute structured-output-requested evidence from the same selected constraint.
Do not replace two mappings with a configurable serialization framework or
JSON round trip. Existing request-mapping and native-submission tests should
protect the wire contract rather than the deleted helper arrangement.
### 3. Retire Alternate Uncaptured Lookup And Rendering Paths
**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.
**Evidence:** The engine always passes its configuration snapshot to the
runner and constructs `prompt.NewCompiledRenderer(snapshot)`. Nevertheless:
- [Prompt selection](../../../internal/usecase/prompt_inspection.go) optionally
detects `GetPromptSnapshot`, falls back to `GetPromptDefinition`, and can hash
definitions at invocation time. Prompt inspection separately detects metadata
lookup, then falls back to loading/projecting a complete definition.
- [Profile identity lookup](../../../internal/usecase/profile_inspection.go)
detects an identity capability, otherwise loads a complete profile merely to
return its identity.
- [Requirement inspection](../../../internal/usecase/requirements_inspection.go)
already requires a captured snapshot capability; it has no equivalent
uncaptured implementation.
- [The renderer](../../../internal/prompt/go_renderer.go) supports both captured
templates and per-call parsing, with separate session paths and message
branches. `NewGoRenderer` has only test/benchmark callers. Production selects
the captured path.
The captured repository implements the lookup capabilities above. Neither root
construction nor its extension options lets consumers inject alternative prompt
or profile repositories or this uncaptured renderer.
**Removal impact:** Minimal private repositories and renderer fixtures would
need to supply captured data. The test
`TestRunnerInspectPromptUsesCapturedHashWhenMetadataCapabilityIsAbsent`
explicitly exercises an alternate private capability combination. Such tests
show that the accommodation exists, not a public extension requirement.
**Approach:** Declare the captured operations actually consumed on the private
collaborator interfaces, retain the narrow metadata queries, and remove the
capability probes and full-definition/hash fallbacks. Make production rendering
use captured templates exclusively. Tests can compile small fixtures using the
existing template compiler or load real in-memory configuration; they should
exercise the same execution path without a replacement compatibility adapter.
Retain per-call template cloning and function binding for request isolation.
This is not a recommendation to remove every convenient internal constructor
or useful test double. For example, a no-repair runner fixture can remain cheap
without maintaining a second rendering or repository-resolution workflow.
### 4. Use The Native JSON Syntax Check In Smoke Diagnostics
**Confidence:** High for the syntax-check simplification. **Owner:** smoke
diagnostics report syntax validity separately from schema conformance.
**Evidence:** [Smoke inference](../../../internal/usecase/smoke_inference_case.go)
constructs a `StandardValidator` with an empty schema directory solely for
`ValidationJSON`. It retains that object behind `validate.Validator` alongside
a separate prepared schema plan. The relevant branch in
[standard validation](../../../internal/validate/standard_validator.go) performs
`json.Valid` with context checks. It does not use the schema directory.
**Removal impact:** Direct use of `json.Valid` preserves the Boolean fact without
the extra validator object, interface field, output contract, and validation
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.
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.
The previous roadmap explicitly retained direct validation and its
decode-before-schema-I/O behavior. Retiring it is therefore a deliberate change
to a previously selected **private** workflow, not an automatic dead-code fix.
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.
### 5. Separate Accepted-Value Detachment From Repeated Input Validation
**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.
**Evidence:** `jsonvalue.CopyMap` validates supported types, keys, cycles,
depth, and produced-node limits while copying. It is used after acceptance in:
- [Captured profile lookup](../../../internal/configuration/targets.go), then
again during profile inspection.
- [Backend cloning](../../../internal/domain/smoke_diagnostics.go), including
already-validated registry construction and subsequent lookup.
- [Executable preparation and details cloning](../../../internal/usecase/prepared_execution.go).
- [Smoke report target detachment](../../../internal/usecase/smoke_tester.go),
followed by another fallible provider-map copy in
[public report conversion](../../../smoke_diagnostics.go).
The repository already has
[`jsonvalue.CloneTrusted`](../../../internal/jsonvalue/trusted.go), specifically
for detaching previously accepted values without revalidation. Captured data
does not become untrusted again merely because it crosses another internal
lookup or report-projection function.
**Removal impact:** Deleting validation alone must not delete caller/client
isolation. Conversely, every clone is not necessary merely because a helper
currently promises an independently owned value. Infallible projection should
not keep impossible error branches or silently turn copying errors into empty
results. Public smoke conversion currently returns copy errors; it does not
silently swallow them.
**Approach:** Identify the actual acceptance point for each path and keep
validation there. Use existing trusted cloning for already accepted snapshots
where detachment is still needed; transfer/share private immutable values where
ownership permits. Remove redundant error plumbing only after establishing
that invariant. Keep validation of new runtime overrides and final effective
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.
### 6. Retain One Private Prepared Metadata Snapshot And One Outward Copy
**Confidence:** High. **Owner:** prepared execution owns single claim/discard,
private execution settings, and independent public details.
**Evidence:** In [prepared execution](../../../internal/usecase/prepared_execution.go),
`PrepareExecution` receives the prepared operation, recopies its provider map,
and deep-clones its metadata into a separate `details` snapshot. The original
snapshot remains in `payload.prepared`. `Details` then deep-clones `details`
again. The [public Details method](../../../prepared_execution.go) invokes
[`fromDomainPreparedRun`](../../../convert.go), which copies nested values
again while projecting public types.
The prepared metadata is already credential-redacted. The direct key, observer,
retry settings, and validation plan are separate payload fields.
`executePreparedRun` reads prepared metadata and places the direct key on a
local target value. The existing payload `clear` does write an empty key into
the prepared metadata, so sharing that snapshot requires removing that
unnecessary mutation first; simply aliasing the current pointers is not enough.
**Removal impact:** Consumers still need fresh, mutable, redacted details, and
details must remain available after execution or discard. Neither promise
requires two retained private metadata graphs or two deep copies per query.
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
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
copy boundary. No new snapshot manager, public type aliases, or replacement
lifecycle framework is needed.
## Findings: Complexity Caused By Optional Contracts
### 7. Decide Whether Prepared Schemas Own A JSON-Tree Resource Budget
**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
can repeat it. That helper applies the same depth-100/node-100,000 traversal
limits used for runtime JSON-compatible values. The test
`TestRunnerPreparationRejectsExcessivelyDeepPreparedSchema` supplies an oversized
schema through a fake private validation plan and protects that rejection.
**Requirement/authority:** Detaching schema state for clients/public results is
established. The public output/schema contracts do not themselves specify those
numeric schema budgets. Internal comments/tests record the behavior, but this
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.
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
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.
### 8. Broad Runtime Go-Value Support Is A Real Choice, Not ConfigKit Duplication
**Evidence:** [`ExecutionTargetOverride.ExtraParams`](../../../types.go) accepts
JSON-compatible arrays/slices/maps and numbers. The
[JSON-value implementation](../../../internal/jsonvalue/jsonvalue.go) also
handles named/typed containers and scalar types, `json.Number`, and indirection,
preserving compatible concrete representations during detachment. Tests cover
these distinctions. This is more machinery than ConfigKit-native mapping
containers require, but runtime overrides do not pass through ConfigKit.
**Requirement/authority:** Accepted runtime input forms and mutable-value
isolation are consumer-facing concerns. Exact preservation of every concrete
Go representation is broader than the public JSON-compatible wording; that
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
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
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
handling must remain explicit; marshal/unmarshal through `float64` is not a
semantics-preserving shortcut. This is a product/API decision, not an internal
cleanup implicitly authorized by the audit.
## Machinery Worth Retaining
These areas were considered and do not justify blanket deletion:
| Area | Concrete responsibility and removal consequence |
| --- | --- |
| Public/internal value and error projection | Keeps implementation types private, redacts credentials, and gives callers/injected clients independent mutable values. Removing all conversion or aliasing internal types would change the intentionally selected facade. Findings 2 and 6 remove redundant steps, not this boundary. |
| Final ConfigKit handoff interpretation | ConfigKit owns composition; PromptKit owns LLM-specific legality, effective presence/null interpretation, referenced resources, and execution readiness. Runtime overrides are new inputs. No second raw-source validator or execution-time provenance audit is justified. |
| Read-only configuration inspection wrapper | Preserves the captured inspection view without exposing the mutable configuration result. Comparison delegates to ConfigKit; a separate PromptKit diff engine is unnecessary. |
| Directory/source discovery | Discovers YAML inputs and groups their resource roots. ConfigKit's explicit filesystem/file source API does not itself enumerate directories; discovery is distinct from reimplementing source composition. |
| Schema root-source adapters and completeness checks | Host-path containment and generic `fs.FS` loading have different source semantics. The selected self-contained submission rule also includes unused schema declarations and relocation away from the source name; root compilation alone is insufficient. Retain the focused traversal/compiler delegation rather than adding a resolver or bundler. |
| Artifact file checks and defensive copy | File-kind checks prevent reading unsupported resources; opened-file checks and context-checked reads serve actual artifact behavior. The earlier accepted ownership decision expressly retained the adapter copy. Removing it needs its own ownership analysis, not an assumption that all copying is redundant. |
| Admission versus active permits | Admission bounds logical execution occupancy across preparation/repair/backoff; active permits bound dispatched attempts. Replacing both with one semaphore would change the selected scheduling behavior. Cancellation-safe waiters and cleanup remain necessary. |
| Prepared-handle identity and state | Owner tokens, one-claim/discard synchronization, payload clearing, and details-after-use implement the opaque handle contract. Finding 6 does not remove that lifecycle. |
| Captured template clones and requirement traversal | Per-call template clones bind request input functions without mutating captured templates. Requirement analysis supplies selected offline required/conditional/optional facts that executing templates or listing AST fields alone cannot provide. Keep its documented conservative scope. |
| HTTP evidence and timeout/body wrappers | Invocation-local transport wrapping preserves caller client settings while observing redirects/status; timeout cancellation must remain active through body consumption. Removing these wrappers can lose facts or cancel responses at headers. |
| Operation-owner error metadata | Whole-chain sentinel priority cannot distinguish a parent operation from a retained child engine error. The private owner wrapper preserves that distinction through errors and observations without discarding child causes. |
| Release wiring | The local launcher and profile delegate validation/publication to ReleaseKit. The Python test checks repository wiring against disposable repositories rather than supplying a second release implementation. No substantial remaining release orchestration duplication was identified. |
## Evidence And Recommended Disposition
Focused existing tests passed with Go 1.27.3, `GOTOOLCHAIN=local`, `GOWORK=off`,
and `GOPROXY=off`:
```sh
go test -race ./internal/usecase ./internal/llm ./internal/jsonvalue \
-run 'Test(RunnerInspectPromptUsesCapturedHashWhenMetadataCapabilityIsAbsent|RunnerPreparedExecutionWithoutValidatorSkipsValidation|RunnerPreparationRejectsExcessivelyDeepPreparedSchema|OpenAICompatibleClientRequestMapping|CopyPreservesCompatibleCollectionsAndNilEmptyDistinctions)'
go test -race . ./internal/prompt ./internal/configuration ./internal/validate \
-run 'Test(PreparedExecution|SmokeTestConcludedInferenceMismatch|SmokeReportConversion|SchemaFile|GoRenderer|LoadBuildsEffectiveTargetsAfterInheritance|PromptInspectionLookupMatchesSnapshotSelectionAndOwnsInputs|StandardValidator|FSValidator|SchemaPreparationDoesNotDependOnSourceName|SchemaLocations)'
```
These tests substantiate current ownership, fallback, schema, and wire behavior;
they do not prove a proposed deletion correct or quantify its savings. No new
tests, benchmark implementation, live provider requests, or production changes
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.
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.