From 3b760310a68170f46b358a992e7feeeece251d70 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 4 Nov 2026 16:17:03 +0000 Subject: [PATCH] Record package boundary audit findings --- docs/roadmap/audit/11-boundaries.md | 229 ++++++++++++++++++++++++++++ 1 file changed, 229 insertions(+) create mode 100644 docs/roadmap/audit/11-boundaries.md diff --git a/docs/roadmap/audit/11-boundaries.md b/docs/roadmap/audit/11-boundaries.md new file mode 100644 index 0000000..e4541d2 --- /dev/null +++ b/docs/roadmap/audit/11-boundaries.md @@ -0,0 +1,229 @@ +# Package Cohesion And Architectural Boundary Audit + +Reviewed 2026-11-04 against commit `b39460a` and the current worktree. Existing +audit-document moves were preserved. No production code, tests, dependencies, +or functionality were changed. + +## Scope, Method, And Architectural Assessment + +Reviewed the 17 repository-owned internal Go packages, their production import +relationships, engine assembly, relevant callers, and tests. The public facade +was checked where it establishes the boundary around those packages. Third-party +package implementations were outside the audit's scope. + +The [current component inventory](../../internal/overview.md) broadly matches +the implementation. Configuration owns the final ConfigKit handoff and captured +resource indexes; the engine assembles collaborators; use cases coordinate +selection, preparation, execution, and diagnostics. Generation owns dispatch +and retries, capacity owns admission and permits, and outbound transport, +rendering, artifacts, schema validation, observations, and shared values have +identifiable owners. No repository-wide package reorganization is warranted. + +The public facade does not leak internal types or receive reverse production +imports from internal packages in the checks run below. Deliberate boundaries +remain useful: configuration completion differs from request overrides; +diagnostic orchestration differs from ordinary execution; domain invariants +differ from source-specific presence and error handling. A broad package, a +small interface-only package, or an import of another internal component is not +itself a cohesion defect. + +Three focused ownership improvements warrant consideration. They are +maintainability opportunities, not newly demonstrated runtime bugs or automatic +release blockers. + +## 1. Retire The Test-Only Capacity Client As A Production Scheduling Surface + +**Relevant code:** [capacity.NewClient and client.Generate](../../../internal/capacity/client.go:17), +[generation constructors and dispatch](../../../internal/generation/coordinator.go:37), +[engine assembly](../../../internal/engine/engine.go:157), and +[runner scheduling test](../../../internal/usecase/runner_test.go:2391). + +**Basis:** The engine selects `generation.NewBuiltIn` or `NewInjected` for every +production engine. The coordinator owns active-permit acquisition around each +attempt, retry waits, observation, and diagnostic evidence. The earlier +`capacity.NewClient` still provides a separate model-client wrapper that +acquires/releases a permit around an opaque call. Both use the same manager; +there are not two independent FIFO algorithms or pools. + +Caller analysis found only tests invoking the older constructor: capacity tests +and a runner test that assembles initial/repair scheduling with this wrapper. +The [capacity document](../../internal/capacity.md#fifo-generation-permits) +acknowledges its retention for focused callers/tests, but no production caller +was found. Its presence leaves capacity owning a model-client adapter after +production dispatch responsibility moved to generation, and makes one runner +assembly test exercise a different scheduling composition than the engine. + +**Consequences:** The extra adapter and its documented role remain maintenance +surface without a production need. Future scheduling changes can require +reasoning about a legacy assembly alongside the real one. Green tests of the +wrapper alone do not demonstrate that coordinator dispatch applies the same +permit lifetime. Existing coordinator and facade tests do protect that actual +path, so this is not a claim that production scheduling is untested or broken. + +**Potential remediation:** Keep the manager and its admission/FIFO primitives +in `internal/capacity`, and generation dispatch integration in +`internal/generation`. Replace the runner test's wrapper with the production +injected coordinator. For focused capacity scenarios, test `Manager.Acquire` +directly or retain the small one-shot adapter as test-owned scaffolding rather +than a production constructor. Once callers are adjusted, the production +wrapper can be retired and its documentation updated. + +Preserve the useful FIFO, cancellation-race, independent-pool, and panic-release +assertions when adjusting test ownership. This finding does not call for moving +queue internals into generation, deleting those scenarios, or wrapping the +coordinator in another capacity client, which could acquire the same limit +twice. Retaining the wrapper is viable if a real internal production need is +identified; current test reachability alone does not establish one. + +## 2. Put OpenRouter Authentication Protocol Details Behind The Outbound Adapter + +**Relevant code:** [authenticationProbe.runOpenRouterKey](../../../internal/usecase/smoke_authentication.go:66), +[recognizedOpenRouterKeyResponse](../../../internal/usecase/smoke_authentication.go:139), +[diagnostic endpoint derivation](../../../internal/usecase/smoke_selection.go:166), +and [HTTP authentication transport](../../../internal/llm/http_evidence.go:111). + +**Basis:** `internal/usecase` currently interprets OpenRouter-specific HTTP +statuses and the account-response keys `data`, `label`, `is_free_tier`, and +`usage`. It owns JSON envelope parsing, identifying-field type checks, and body +consumption. The same package derives the OpenRouter-relative `/key` endpoint. +Meanwhile `internal/llm` owns authenticated HTTP execution, credential +resolution, evidence collection, and response ownership mechanics. One provider +integration is therefore divided between the application coordinator and its +outbound adapter. + +This division is different from a use case consuming factual HTTP evidence: +the precise external account document and provider endpoint are protocol +knowledge. The [architecture policy](../../policy/architecture.md#architectural-approach) +calls for encapsulating external integration details in internal adapters. +The parser is already functionally isolated, making the ownership seam fairly +small. The authentication tests in +[smoke_authentication_test.go](../../../internal/usecase/smoke_authentication_test.go:64) +also show that this is provider protocol behavior rather than application +selection policy. + +**Consequences:** A change to OpenRouter's account endpoint or identifying +response fields requires changing application-layer code as well as the +transport integration. Adding another authentication adapter would extend +provider-specific branching inside the use-case package. This complicates the +otherwise clear distinction between choosing a diagnostic and implementing +the upstream protocol. + +**Potential remediation:** Move the OpenRouter request/response adapter, +including its default destination and response recognition, into +`internal/llm` alongside the existing diagnostic transport. Have the use case +consume a small factual outcome/evidence value. Keep target selection, +configured applicability, unsupported-operation reporting, caller cancellation, +overall completion, and report assembly in the use case. The existing +consumer-owned diagnostic interface is a useful seam; engine assembly can +supply the concrete outbound implementation without requiring protocol parsing +inside the coordinator. + +An alternative is a focused internal diagnostic-adapter package if separation +from inference transport becomes useful. Another package or a provider plugin +framework is not necessary for the current single protocol adapter. Do not +introduce a new dependency from the outbound adapter back into `usecase`, or +move application report/completion policy into transport merely to relocate +the parser. The diagnostic feature legitimately deals with HTTP facts; a +blanket prohibition on HTTP types or status observations in use cases is not +implied by this finding. + +Preserve anonymous authentication probing, explicit endpoint overrides, +neutral tri-state facts, private body/credential handling, and the absence of +ordinary observer events. The previously recorded unbounded authentication-body +read is a separate resource/security finding; relocation alone does not fix it. + +## 3. Narrow Smoke-Value Copy Helpers To Their Actual Ownership Boundaries + +**Relevant code:** [domain.CloneSmokeTestRequest](../../../internal/domain/smoke_diagnostics.go:157), +[domain.CloneSmokeTestReport](../../../internal/domain/smoke_diagnostics.go:179), +[smokeReportTarget](../../../internal/usecase/smoke_tester.go:432), and +[public smoke-value conversion](../../../smoke_diagnostics.go:279). + +**Basis:** `CloneSmokeTestRequest` is called only by its own domain test; +actual request ownership is established by public-to-domain conversion. +`CloneSmokeTestReport` has one production caller: `smokeReportTarget` creates +an otherwise empty report containing a target, clones the full report, and +extracts just that target. Whole-report facts, timing, and attempt cloning have +no runtime use through that caller. Public report conversion independently +copies the actual facts, timings, attempts, and provider map into public types. + +Thus the domain package maintains a broad request/report-copy API, but the +real internal consumer needs only a detached target/provider map. The +whole-report ownership functionality is exercised by its domain test rather +than by actual whole-report production use. Public DTO conversion is a +legitimate separate responsibility, not something that should disappear into +domain types or public aliases. + +**Consequences:** Smoke-value changes have additional copy lists and tests to +maintain even where those copies are not needed in production. A temporary +report container obscures the narrow target-detachment operation. Keeping a +general cloner solely because its own test calls it reverses the relationship +between production responsibility and supporting tests. + +**Potential remediation:** Express the target-copy operation directly at its +use-case boundary using existing JSON-value ownership, or a small target clone +helper if actual callers justify one. Retain public request/report projection +in the facade and shared JSON-tree copying in its existing owner. Retire the +unused request cloner and the unnecessary whole-report clone surface after +confirming no additional callers need them. Remove only clone-specific helper +code that is then unused. The backend-cloning helpers have real registry +consumers and are outside the proposed retirement; retain that metadata +ownership behavior. + +Preserve provider-map error propagation, caller-owned result values, and +credential/endpoint redaction. Keep the meaningful +[public conversion ownership test](../../../smoke_conversion_internal_test.go:13) +and relevant use-case report tests. If genuine internal whole-report snapshots +are needed, retaining an explicitly used domain cloner is a reasonable +alternative. This finding does not require sharing public/internal DTO types +or routing public conversion through a full internal clone followed by another +copy. Coordinate any JSON traversal consolidation with +[audit 10](10-equivalence.md#2-centralize-structural-copying-of-already-normalized-json-values). + +## Related Existing Findings + +The runner's declared `Validator` dependency and actual `ValidationPreparer` +requirement remain the interface-boundary clarification in +[complexity-audit finding 1](03-complexity.md#1-consolidate-schema-compilation-and-express-the-runners-actual-interface). +The facade's generic JSON reflection copier has an existing internal owner +identified in audit 10. These are relevant ownership concerns, not additional +independent findings to count again. Large orchestration functions and repeated +schema compilation within an already coherent package likewise retain their +existing complexity/equivalence remediation scope. + +## Reachability And Validation Evidence + +Production imports were enumerated with `go list` and compared with the graph +and source callers. Rapid Type Analysis used the locally available pinned +`golang.org/x/tools` v0.52.0 command, with reporting restricted to +repository-owned internal packages: + +```sh +go list -f '{{.ImportPath}}|{{join .Imports " "}}' ./internal/... +go run golang.org/x/tools/cmd/deadcode@v0.52.0 -test -filter='^go.maximumdirect.net/promptkit/internal/' ./... +go run golang.org/x/tools/cmd/deadcode@v0.52.0 -filter='^go.maximumdirect.net/promptkit/internal/' ./... +``` + +With tests included, the tool reported only the unused test helper +`internal/usecase/runner_test.go:stringPtr`. That small test cleanup does not +justify a separate architectural finding. Test-only production helpers remain +reachable to this analysis when their tests call them. + +Without tests, the roots are the repository's example executables, not every +possible downstream library consumer. The output includes valid public +workflows that those examples do not exercise, so it is not a deletion list. +The capacity-wrapper and request-cloner observations above were additionally +checked against actual source references and assembly; graph in-degree or +example reachability alone was not treated as proof of obsolescence. The +analysis also covers only the selected local build configuration. + +The following existing offline checks passed with local Go 1.27.0: + +```sh +go test -race -count=1 -run 'Test(PublicFacadeDoesNotExposeInternalTypes|InternalProductionPackagesDoNotImportFacade|RepositoryDoesNotImportFormerModule|SmokeTestConcludedInferenceMismatchHasNilGoErrorAndDetachedValues)' . +go test -race -count=1 ./internal/capacity ./internal/generation ./internal/domain ./internal/usecase +``` + +No new release gate, consumer contract, provider call, production edit, or test +removal is proposed as part of this audit pass. The tools left module metadata +unchanged.