Record complexity and maintainability audit findings

docs/roadmap/audit/03-complexity.md

@@ -0,0 +1,216 @@
+# 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.