230 lines
13 KiB
Markdown
230 lines
13 KiB
Markdown
# 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.
|