From fb87fabc2e49aaeabf2498ab8891edd932a42f09 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 4 Nov 2026 13:27:01 +0000 Subject: [PATCH] Record complexity and maintainability audit findings --- docs/roadmap/audit/03-complexity.md | 216 ++++++++++++++++++++++++++++ 1 file changed, 216 insertions(+) create mode 100644 docs/roadmap/audit/03-complexity.md diff --git a/docs/roadmap/audit/03-complexity.md b/docs/roadmap/audit/03-complexity.md new file mode 100644 index 0000000..855d268 --- /dev/null +++ b/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.