Record dead code audit findings
This commit is contained in:
233
docs/roadmap/audit/12-deadcode.md
Normal file
233
docs/roadmap/audit/12-deadcode.md
Normal file
@@ -0,0 +1,233 @@
|
||||
# Dead Code And Obsolete Functionality Audit
|
||||
|
||||
Reviewed 2026-11-04 against commit `3b76031` and the current worktree. Existing
|
||||
audit-document moves were preserved. No production code, tests, dependencies,
|
||||
or functionality were changed.
|
||||
|
||||
## Scope And Assessment
|
||||
|
||||
Reviewed repository-owned Go packages, public entry points, configuration and
|
||||
runtime values, collaborator interfaces, default/optional paths, embedded
|
||||
resources, build constraints, and the release launchers and their wiring test.
|
||||
The architecture remains a public facade over captured configuration, use-case
|
||||
orchestration, generation/capacity, and focused internal adapters. The former
|
||||
standalone configuration repositories have already been retired; this review
|
||||
found smaller residues rather than another abandoned subsystem.
|
||||
|
||||
The review combined Rapid Type Analysis, Staticcheck unused/data-flow checks,
|
||||
graph caller discovery, source inspection, and the format, internal, policy,
|
||||
and roadmap documents. Tests count as real callers, but a helper's self-test
|
||||
does not by itself establish a production need. Conversely, absence from the
|
||||
examples or default execution path is not evidence that an optional public
|
||||
feature is obsolete.
|
||||
|
||||
Four cleanup areas warrant consideration. These are low-priority reductions in
|
||||
state and maintenance surface, not newly demonstrated runtime bugs. None calls
|
||||
for deleting a supported public feature or imposing a new release gate.
|
||||
|
||||
## 1. Remove Backend Presence Bookkeeping With No Runtime Consumer
|
||||
|
||||
**Relevant code:** [loaded.backends](../../../internal/configuration/load.go:122),
|
||||
[backendFields and presence capture](../../../internal/configuration/load.go:215),
|
||||
[effective backend construction](../../../internal/configuration/targets.go:14),
|
||||
and [Backend.QueueCapacitySet](../../../internal/domain/domain.go:187).
|
||||
|
||||
**Evidence:** Construction allocates a per-backend field-state map and fills it
|
||||
through `captureFields(..., state.backends)`. No production code reads the
|
||||
recorded backend states. The only lookup of the captured queue state is in
|
||||
[TestLoadDefaultsAbsentQueueCapacity](../../../internal/configuration/load_test.go:755).
|
||||
Unlike profile presence, backend presence is not consumed while constructing
|
||||
effective execution targets.
|
||||
|
||||
`QueueCapacitySet` is also assigned during target construction and asserted in
|
||||
tests, but never read to select a production behavior. The current assignment
|
||||
is simply whether concurrency is positive; it no longer distinguishes an
|
||||
explicit queue setting from a derived default. The registry's capacity-policy
|
||||
projection uses `ConcurrencyLimit` and `QueueCapacity`, not this flag.
|
||||
|
||||
**Why obsolete:** Queue defaulting and its provenance are already handled by
|
||||
[resolveBackends](../../../internal/configuration/resolve_targets.go:11) and
|
||||
ConfigKit's derived-default record. Typed final values drive runtime capacity;
|
||||
the retained side map and Boolean do not contribute to that behavior or to the
|
||||
public inspection view. No documented compatibility or planned-feature need for
|
||||
these extra records was found.
|
||||
|
||||
**Potential remediation:** Remove the backend-only presence capture, its field
|
||||
list and retained map, and the unused runtime Boolean/assignment. Adjust tests
|
||||
to assert effective queue behavior and, where relevant, the existing ConfigKit
|
||||
inspection/provenance result. Keep `captureFields`, profile field-state
|
||||
tracking, and prompt-input metadata presence: those have actual consumers and
|
||||
preserve required absent/null/explicit-value distinctions. Retain explicit
|
||||
zero queue capacity, derived defaulting, and operator history unchanged.
|
||||
|
||||
This removes unnecessary construction work and retained per-backend maps; it
|
||||
does not justify dropping configuration provenance or changing its ownership
|
||||
under the [configuration-boundary ADR](../../adr/0001-configuration-boundary.md).
|
||||
|
||||
## 2. Retire Legacy Fields In The Captured Prompt Representation
|
||||
|
||||
**Relevant code:** [PromptDefinition.OutputFormat](../../../internal/domain/domain.go:148),
|
||||
[PromptMessageTemplate.ContentFile](../../../internal/domain/domain.go:173),
|
||||
[prompt materialization](../../../internal/configuration/prompts.go:58),
|
||||
[resolveOutputContract](../../../internal/usecase/runner.go:639), and
|
||||
[definition hashing](../../../internal/promptdef/hash.go:12).
|
||||
|
||||
**Evidence:** Supported engine construction requires and validates a complete
|
||||
output contract. Materialization sets `definition.Validation` to that contract
|
||||
and duplicates its format into `definition.OutputFormat`. The runner reads
|
||||
the duplicate only when `Validation.Format` is empty, a condition that the
|
||||
captured definitions supplied by engine construction cannot have. The standard
|
||||
runner test-definition helper fills both fields with the same value; no
|
||||
supported public repository-injection path supplies an alternative definition.
|
||||
|
||||
For messages, materialization resolves the real source `content_file` into
|
||||
captured `Content` and does not populate the domain `ContentFile` field. The
|
||||
renderer uses `Content`/captured templates, not that path field. Its one
|
||||
nonempty assignment is a renderer fixture demonstrating that already-loaded
|
||||
content is rendered without opening a file. The source-format
|
||||
`promptMessageConfig.ContentFile` remains actively used and is a different
|
||||
field.
|
||||
|
||||
**Why obsolete:** The runtime representation still carries source-era message
|
||||
path data and a second output-format source after configuration loading became
|
||||
the authoritative materialization boundary. The empty-format fallback is
|
||||
disconnected from supported engine definitions. No current documentation or
|
||||
roadmap records a compatibility requirement for these internal duplicate fields.
|
||||
|
||||
**Potential remediation:** Use `PromptDefinition.Validation.Format` as the one
|
||||
captured format and remove the legacy duplicate/fallback. Remove only the
|
||||
runtime message path field, adjusting its fixture to exercise captured text.
|
||||
Preserve the actual YAML `content_file` feature, source-relative resource
|
||||
resolution, ConfigKit origin inspection, and construction-time capture. Also
|
||||
preserve the separate, supported request-override rule that an empty replacement
|
||||
`OutputContract.Format` defaults to text.
|
||||
|
||||
**Serialization caveat:** These fields are included indirectly by
|
||||
`HashDefinition`'s `json.Marshal`, even when a message path is empty. They are
|
||||
therefore not completely inert bytes: removing them changes current prompt
|
||||
hash values. Account for that effect explicitly and protect the documented
|
||||
opaque-equality relationships between inspection and preparation. Do not claim
|
||||
bit-identical hashes after changing the hash input, or invent a permanent hash
|
||||
encoding guarantee merely to perform this cleanup. The fields are internal
|
||||
representation, not part of the public stable DTO shapes.
|
||||
|
||||
## 3. Remove Unused Diagnostic State And A Discarded Helper Result
|
||||
|
||||
**Relevant code:** [reachabilityProbeOutcome.initialEndpoint](../../../internal/usecase/smoke_reachability.go:24),
|
||||
[HTTP report assembly](../../../internal/usecase/smoke_tester.go:376),
|
||||
[fileSource](../../../internal/engine/engine.go:211), and
|
||||
[SetSchemaFile](../../../internal/engine/engine.go:75).
|
||||
|
||||
**Evidence:** The reachability probe assigns its private outcome's
|
||||
`initialEndpoint`, but the smoke tester never reads that member. It records
|
||||
the real public attempt's `InitialEndpoint` from the resolved report target.
|
||||
Only `TestReachabilityProbeRemovesURLCredentials` inspects the private echo.
|
||||
Actual sanitized request/evidence and report behavior are established through
|
||||
other values.
|
||||
|
||||
Separately, `fileSource` returns a filesystem, file name, source-directory
|
||||
string, and error. Its only caller, `SetSchemaFile`, always discards the
|
||||
directory result. The directory is still needed internally to construct
|
||||
`os.DirFS`; exposing it as another return value serves no caller.
|
||||
|
||||
**Potential remediation:** Remove the unused probe-outcome member and its
|
||||
assignment; retain credential-removal assertions against the actual request,
|
||||
evidence, or public diagnostic report. Reduce `fileSource`'s private signature
|
||||
to the consumed results. Keep its directory calculation, file checks, selected
|
||||
filesystem/root, and the public `WithSchemaFile` option intact.
|
||||
|
||||
Neither cleanup removes `SmokeTestAttempt.InitialEndpoint`, destination
|
||||
evidence, or single-schema-file support. Those public features are live even
|
||||
though an extra private field/result is unnecessary.
|
||||
|
||||
## 4. Remove Orphan Helpers Whose Only Callers Are Their Own Tests
|
||||
|
||||
**Relevant code and evidence:**
|
||||
|
||||
- [filecatalog.RelativePath](../../../internal/filecatalog/catalog.go:71) is
|
||||
called only by `TestRelativePathNested`. Current source discovery and
|
||||
source-relative resource resolution use other helpers; no production or
|
||||
public-facade caller uses this wrapper around `filepath.Rel`.
|
||||
- [resourceNamespaceFromID](../../../internal/domain/resource_identity.go:34)
|
||||
is called only by `TestResourceNamespaceFromID`. Actual namespace inventory
|
||||
and filtering paths do not call it. The exported identity-validation and
|
||||
qualification operations remain active and are not candidates for removal.
|
||||
- [runner_test.go:stringPtr](../../../internal/usecase/runner_test.go:2592)
|
||||
has no callers, including in tests. Both reachability analysis and
|
||||
Staticcheck `U1000` report it.
|
||||
|
||||
Caller searches found no reflection, registration, build-constrained caller,
|
||||
or supported external API exposing these helpers. The first two belong to
|
||||
internal packages and have only their direct self-tests; no documented future
|
||||
or compatibility purpose was found. The third is already test-only and unused.
|
||||
|
||||
**Potential remediation:** Remove the orphan helpers and their now-irrelevant
|
||||
self-tests, keeping behavioral coverage of real discovery, namespace
|
||||
inventory/filtering, and identity qualification. Remove the unused pointer
|
||||
helper without replacing it. There is no reason to introduce a new abstraction
|
||||
or move the self-tests to another layer solely to preserve these unused APIs.
|
||||
|
||||
## Intentional Retention And Related Findings
|
||||
|
||||
The following distinctions prevent removal based on misleading reachability:
|
||||
|
||||
- `WithSchemaFile` and `InlineWithURI` are supported public helpers despite
|
||||
being unreachable from the current test executables. The former's missing
|
||||
public wiring coverage is already in [audit 08](08-testing.md); it is not
|
||||
evidence that the option should be deleted.
|
||||
- The older capacity client wrapper and no-repair runner constructor are
|
||||
expressly retained for focused internal callers/tests in the current
|
||||
[capacity](../../internal/capacity.md#fifo-generation-permits) and
|
||||
[runner](../../internal/runner.md#collaborators) documentation. This audit
|
||||
does not classify them as dead functionality or recommend deleting them.
|
||||
[Audit 11](11-boundaries.md) discusses the wrapper's architectural placement;
|
||||
altering that deliberate retention is a separate decision.
|
||||
- Repository capability fallbacks, standalone rendering/validation, injected
|
||||
clients' nil usage-presence compatibility, nil-validator/admitter behavior,
|
||||
and optional provider settings have meaningful callers or documented
|
||||
purposes. Normal engine assembly taking another path does not obsolete them.
|
||||
- ConfigKit YAML fields and stable JSON DTO fields can be used by reflection;
|
||||
formatter methods and interfaces can be invoked indirectly. Embedded
|
||||
OpenRouter and smoke-schema assets, the Linux FIFO test, and release scripts
|
||||
likewise have consumers outside a simple direct-call list. They were not
|
||||
treated as dead. No unused public configuration option or abandoned release
|
||||
launcher was established.
|
||||
|
||||
The domain smoke-request/full-report cloners have a narrow-or-unused consumer
|
||||
surface already described in [audit 11, finding 3](11-boundaries.md#3-narrow-smoke-value-copy-helpers-to-their-actual-ownership-boundaries).
|
||||
That ownership/remediation scope is not counted again here. The schema and
|
||||
JSON-copy consolidations in prior audits likewise remain their existing work,
|
||||
not an excuse to delete intentional alternative implementations wholesale.
|
||||
|
||||
## Tooling And Validation
|
||||
|
||||
The checks used local Go 1.27.0 and pinned audit-tool versions:
|
||||
|
||||
```sh
|
||||
go run golang.org/x/tools/cmd/deadcode@v0.52.0 -test -filter='^go.maximumdirect.net/promptkit(/|$)' ./...
|
||||
go run honnef.co/go/tools/cmd/staticcheck@v0.8.2 -checks 'U1000,SA4*' ./...
|
||||
go list -f '{{.ImportPath}} {{join .GoFiles " "}} {{join .IgnoredGoFiles " "}}' ./...
|
||||
```
|
||||
|
||||
`deadcode -test` reported the two public helpers and unused test pointer helper
|
||||
listed above. Staticcheck reported only `stringPtr` (`U1000`) and exited with
|
||||
status 1 because of that finding; it reported no additional selected `SA4*`
|
||||
diagnostics. These are audit observations, not successful lint sign-offs.
|
||||
Neither tool establishes that all tested or serialized state is necessary,
|
||||
which is why the write-only and redundant fields required manual review.
|
||||
|
||||
Rapid Type Analysis starts from executable roots; tests add roots, and example
|
||||
programs do not exercise every library consumer path. The analysis is local-build
|
||||
evidence, not proof about all platforms or downstream programs. In particular,
|
||||
platform-specific schema handling and optional library operations were retained.
|
||||
|
||||
Existing offline race-enabled tests passed:
|
||||
|
||||
```sh
|
||||
go test -race -count=1 ./internal/configuration ./internal/domain ./internal/filecatalog ./internal/prompt ./internal/engine ./internal/usecase
|
||||
go test -race -count=1 -run 'Test(ConfigurationInspection|PreparedExecution|ValidateRequestMatchesPrepareContract|AppendedMessagesComposeFrozenEffectivePrompt)' .
|
||||
```
|
||||
|
||||
Tool execution left `go.mod` and `go.sum` unchanged. No production changes,
|
||||
test removals, provider calls, or new validation gates were made during this
|
||||
review.
|
||||
Reference in New Issue
Block a user