From cfa3a85308084105c527e22cb83114bb92330bce Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 5 Nov 2026 18:10:01 +0000 Subject: [PATCH] Record the architectural responsibility audit --- docs/roadmap/audit/A1-responsibility.md | 366 ++++++++++++++++++++++++ 1 file changed, 366 insertions(+) create mode 100644 docs/roadmap/audit/A1-responsibility.md diff --git a/docs/roadmap/audit/A1-responsibility.md b/docs/roadmap/audit/A1-responsibility.md new file mode 100644 index 0000000..a70ce29 --- /dev/null +++ b/docs/roadmap/audit/A1-responsibility.md @@ -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.