217 lines
12 KiB
Markdown
217 lines
12 KiB
Markdown
# Complexity And Maintainability Audit
|
|
|
|
Reviewed 2026-11-04 against commit `5adc1c1`.
|
|
|
|
## Method And Architectural Assessment
|
|
|
|
Used the local codebase-memory graph's cyclomatic/cognitive complexity,
|
|
function-length, and call-relationship tools to locate candidates. Reviewed
|
|
functions and methods, including test hotspots, rather than relying on
|
|
function-only rankings. Inspected implementation, callers, owning interfaces,
|
|
and current architecture/internal documentation before selecting findings.
|
|
Metrics are analyzer-specific indicators, not correctness proofs, acceptance
|
|
thresholds, or requirements to split every large switch.
|
|
|
|
PromptKit's overall boundaries remain useful: a public facade adapts values and
|
|
errors; configuration interprets ConfigKit's final handoff and captures runtime
|
|
indexes; the runner coordinates preparation and execution; generation owns
|
|
attempts/retries; capacity owns admission and permits; transport and schema
|
|
adapters own external protocols. No repository-wide architectural rewrite or new
|
|
public abstraction is warranted by this review.
|
|
|
|
Representative hotspot metrics from the current graph:
|
|
|
|
| Function | Cyclomatic | Cognitive |
|
|
| --- | ---: | ---: |
|
|
| `requirementWalker.visit` | 50 | 137 |
|
|
| `validateEffective` | 39 | 132 |
|
|
| `requirementWalker.evaluateCommand` | 26 | 103 |
|
|
| `providerValueForMapping` | 33 | 84 |
|
|
| `jsonvalue.copyValue` | 29 | 66 |
|
|
| `loaded.buildExecutionProfiles` | 27 | 58 |
|
|
| `loaded.materializePrompt` | 30 | 52 |
|
|
| `Coordinator.generate` | 24 | 49 |
|
|
|
|
The findings below are behavior-preserving refactoring opportunities, not
|
|
confirmed bugs or automatic release blockers. They cover both package-local
|
|
complexity and interactions between packages. No production code, tests, or
|
|
dependencies were changed, and no empirical testing or benchmarks were needed.
|
|
|
|
## 1. Consolidate Schema Compilation And Express The Runner's Actual Interface
|
|
|
|
**Code:** [validator interfaces](../../../internal/validate/validator.go:10),
|
|
[schema implementations](../../../internal/validate/standard_validator.go:45),
|
|
[runner preparation](../../../internal/usecase/runner.go:398), and
|
|
[engine assembly](../../../internal/engine/engine.go:133).
|
|
|
|
**Complexity:** The validator maintains direct validation and prepared
|
|
validation, with separate filesystem and `fs.FS` compilation paths. Compiler
|
|
construction, resource registration, error wrapping, and cancellation checks
|
|
are repeated across direct schema-validation methods and `PrepareValidation`.
|
|
The package's constructors return `Validator`, although the runner actually
|
|
requires `ValidationPreparer` and discovers it through a runtime type assertion.
|
|
An implementation satisfying the declared constructor dependency can therefore
|
|
still be unsuitable for the execution path.
|
|
|
|
**Opportunity:** Make the runner's preparation dependency explicit in its
|
|
internal field/constructor types, updating assembly and focused test
|
|
collaborators together. Retain the direct validation interface where callers
|
|
actually need it. Consolidate compilation behind one internal operation, with
|
|
small source-specific loading/URL/loader adapters; direct and prepared callers
|
|
can reuse that operation rather than separately implementing registration and
|
|
compilation. No public validator API or generic source framework is needed.
|
|
|
|
**Preserve:** Direct validation currently rejects malformed JSON before trying
|
|
to compile its schema; simply implementing direct `Validate` as eager
|
|
`PrepareValidation` followed by plan validation would change that error/I/O
|
|
ordering. Keep that short-circuit, operation-local compiled resources, prepared
|
|
freezing, schema dialect and containment rules, synchronous cancellation
|
|
precedence, and the runner's existing nil-validator behavior where used.
|
|
Coordinate schema representation changes with correctness-audit finding 2,
|
|
without treating this refactor alone as its fix.
|
|
|
|
## 2. Decompose Configuration Validation And Resource Materialization
|
|
|
|
**Code:** [validateEffective](../../../internal/configuration/load.go:410),
|
|
[materializePrompt](../../../internal/configuration/prompts.go:58),
|
|
[buildExecutionProfiles](../../../internal/configuration/targets.go:14), and
|
|
[resolution callbacks](../../../internal/configuration/resolve_prompts.go:10).
|
|
|
|
**Complexity:** `validateEffective` combines namespace checks, reference indexes,
|
|
backend settings, capacity arithmetic, smoke metadata, profile bounds, and prompt
|
|
references in one nested traversal. Prompt materialization then combines input
|
|
validation, presence projection, content-file I/O, message validation, template
|
|
compilation, output-contract construction, hashing, and inspection assembly.
|
|
Target assembly is another long presence-state decision chain. Reviewing a
|
|
single field change requires following several distinct responsibilities inside
|
|
large functions.
|
|
|
|
**Opportunity:** Keep the top-level traversal/index preparation visible, but
|
|
delegate coherent record-level work to backend, profile, and prompt helpers
|
|
with explicit record/path/index arguments. Isolate prompt input projection,
|
|
message materialization/compilation, and output-contract construction into
|
|
small functions returning the values the caller needs. Keep target presence
|
|
interpretation in one identifiable helper rather than distributing it between
|
|
new callers. Organize files by those responsibilities only where that helps
|
|
navigation; moving unchanged code between files is not itself the improvement.
|
|
|
|
**Preserve:** Validate only final ConfigKit-provided values. Keep recorded
|
|
normalization, field presence/null distinctions, exact references, error
|
|
identities, source-relative resource contexts, lazy metadata capture, and
|
|
construction-time template/hash capture. Materialization legitimately combines
|
|
validation with resource access; this finding does not require moving every
|
|
check into a new validation pass or aggregating formerly fail-fast errors.
|
|
Avoid a reflection-driven field schema that obscures the domain rules.
|
|
|
|
## 3. Untangle Requirement Traversal, Expression Evaluation, And Evidence
|
|
|
|
**Code:** [requirement walker](../../../internal/prompt/requirements.go:356),
|
|
[syntax dispatch](../../../internal/prompt/requirements.go:451),
|
|
[condition evaluation](../../../internal/prompt/requirements.go:93), and
|
|
[command traversal](../../../internal/prompt/requirements.go:652).
|
|
|
|
**Complexity:** The approximately thousand-line file contains mutually recursive
|
|
syntax walking and partial expression evaluation, branch reachability, caller
|
|
scope tracking, template-call handling, evidence promotion, issue construction,
|
|
and result sorting. `if` and `with` repeat branch orchestration. `input`, `index`,
|
|
root variables, and skipped-path behavior appear in both evaluation and
|
|
conservative traversal. Correctness depends on the interaction of return-value
|
|
flags and mutable walker state such as `rootKnown`, `dependencies`, and the
|
|
unknown-effect flags, not just the local switch case.
|
|
|
|
**Opportunity:** Separate the existing roles into small internal units: syntax
|
|
recognition/evaluation, branch/scope traversal, and evidence/result accumulation.
|
|
Factor shared `if`/`with` orchestration while making their different body-dot
|
|
rules explicit. Reuse recognition of supported command shapes instead of
|
|
duplicating it between evaluative and inventory-only paths. Make existing
|
|
template scope entry/exit state handling explicit. Keep a small enum/value model
|
|
rather than adding a general expression interpreter or pluggable AST framework.
|
|
|
|
**Preserve:** The deliberately conservative recognition scope, skipped-path
|
|
inventory, optional `index` behavior, named-template limitations, incomplete
|
|
results, and cancellation semantics. Traversal and evaluation are not
|
|
interchangeable: inactive paths must retain recognizable names without
|
|
evaluating caller-derived names. Any shared helper must express that distinction.
|
|
The evidence-union efficiency finding in [audit 02](02-efficiency.md) is related
|
|
but separate; a mechanical file split would not fix it.
|
|
|
|
## 4. Clarify Ownership And Policies Across The JSON Value Walkers
|
|
|
|
**Code:** [source mapping conversion](../../../provider_values.go:31),
|
|
[bounded JSON copying](../../../internal/jsonvalue/jsonvalue.go:67), and
|
|
[public-value copying](../../../convert.go:406).
|
|
|
|
**Complexity:** Three recursive reflection-based paths independently handle
|
|
interfaces/pointers, maps, sequences, scalars, and nil values. Each adds its own
|
|
policy: source conversion translates into ConfigKit-compatible containers and
|
|
defers unsupported-value errors; `jsonvalue` validates and bounds owned copies;
|
|
facade copying detaches already-valid values while preserving their types.
|
|
Similar-looking switches conceal different assumptions and failure behavior.
|
|
The numeric conversion defect in correctness-audit finding 1 demonstrates a
|
|
concrete risk at one of these boundaries, but is not a new finding here.
|
|
|
|
**Opportunity:** First make the three responsibilities explicit at their call
|
|
boundaries. Isolate source-number encoding from structural traversal, and
|
|
identify reusable scalar/indirection/container-copy primitives only where their
|
|
semantics truly match. Prefer removing a redundant traversal path or reusing an
|
|
existing trusted-value copy operation over maintaining another policy-rich
|
|
reflection switch. Keep ConfigKit-specific mapping encoding in its adapter;
|
|
do not teach the source-neutral domain about ConfigKit.
|
|
|
|
**Preserve:** Supported concrete types, nil behavior, cycle/depth/work handling,
|
|
caller ownership, and deferred source errors. In particular, passing source
|
|
builders through rejecting `CopyMap` would introduce validation before the
|
|
ConfigKit handoff and is not an acceptable consolidation. Nor should an
|
|
infallible facade projection acquire new rejection behavior merely to reuse a
|
|
validator. A universal callback-configured reflection framework could increase
|
|
complexity; retain separate small adapters where their policies differ.
|
|
|
|
## 5. Extract Attempt Accounting From Generation Control Flow
|
|
|
|
**Code:** [Coordinator.generate](../../../internal/generation/coordinator.go:98),
|
|
[generateInjected](../../../internal/generation/coordinator.go:203),
|
|
[execute](../../../internal/generation/coordinator.go:303), and
|
|
[diagnostic evidence consumers](../../../internal/usecase/smoke_tester.go:246).
|
|
|
|
**Complexity:** The coordinator interleaves permit acquisition, dispatch,
|
|
cancellation, retry decisions, observer events, diagnostic evidence, and
|
|
presence-aware usage accumulation. The built-in and injected paths repeat
|
|
generation-number setup, evidence finishing, attempt projection, and usage
|
|
projection. The injected path also repeats its non-dispatched cancellation
|
|
reporting. `execute` returns five parallel values, increasing the amount of
|
|
state the caller must keep aligned while constructing two reporting forms.
|
|
|
|
**Opportunity:** Return one small internal attempt-result value containing
|
|
outcome and timing/dispatch facts, and extract common projection/accounting
|
|
operations where evidence is genuinely shared. Keep the retry loop primarily
|
|
about eligibility, budget, waits, and terminal outcomes. Keep injected and
|
|
built-in dispatch strategies explicit rather than manufacturing HTTP facts for
|
|
injected clients or introducing an extensible retry framework.
|
|
|
|
**Preserve:** Release active permits before callbacks/backoff, one-shot injected
|
|
dispatch, frozen built-in requests, usage-presence distinctions, private
|
|
diagnostic evidence, ordinary observer boundaries, and retention of decoded
|
|
facts before cancellation discards ordinary results. The HTTP adapter should
|
|
continue owning transport facts; accounting extraction must not push provider
|
|
protocol knowledge into the runner or observation package.
|
|
|
|
## Prioritization And Non-Findings
|
|
|
|
Start with the schema interface/compilation consolidation and configuration
|
|
helpers: their responsibilities provide comparatively clear refactoring seams.
|
|
The walker and generation coordinator need narrower, behavior-preserving units
|
|
because their state transitions are more subtle. Coordinate source conversion
|
|
cleanup with the independently required number-preservation fix.
|
|
|
|
High scores alone did not justify findings for explicit public error-category
|
|
switches, presence-aware source-field encoding, regular-file/schema read loops,
|
|
or table-driven tests. Those branches often directly express selected contracts.
|
|
Large facade files also include substantial GoDoc and straightforward value
|
|
projection, which is not evidence that public/internal separation has failed.
|
|
Keep useful test scenarios; do not rewrite tests solely to reduce their metrics.
|
|
|
|
No additional ordinary correctness defect was established in this pass.
|
|
[Audit 01](01-correctness.md) and [audit 02](02-efficiency.md) retain their own
|
|
findings. No implementation changes or additional consumer guarantees are
|
|
implied by this refactoring catalog.
|