From 41c6ca807031fc7079f09dddfb1d8c9af77c4342 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 26 Oct 2026 14:14:50 +0000 Subject: [PATCH] Record reporting efficiency opportunities --- docs/roadmap/remediation.md | 148 +++++++++++++++++++++++++++++++++++- 1 file changed, 147 insertions(+), 1 deletion(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 618567f..2e15453 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,8 @@ # Invocation reporting audit remediation -Status: five open findings. Findings 4 and 5 were added by the interaction audit. +Status: five open remediation findings and two efficiency opportunities for +consideration. Findings 4 and 5 came from the interaction audit; findings 6 and 7 +came from the algorithmic audit. ## Review scope @@ -33,6 +35,34 @@ quiet help/version, aggregate configuration YAML, quiet comparison, concrete request validation, and offline aggregate smoke testing. Findings 4 and 5 record the additional gaps; findings 1–3 remain open and are not duplicated below. +The algorithmic audit at `ee76b0f` examined the reporting-specific parser passes, +delivery bookkeeping, deferred storage, native diagnostic partitioning, and +integration report coordination. It used source-level complexity analysis, not +benchmarks or real-world performance measurements. Selected algorithms were +excluded from reconsideration, including synchronous serialized reporting, FIFO +deferred delivery, rendering rich content under quiet mode, fresh backend trees, +and the chosen configuration/smoke report preparation boundaries. Findings 6 and +7 are implementation opportunities within those decisions. + +No newly introduced quadratic-time algorithm was identified in the reviewed +paths. In particular: + +- Reporting selection is linear in the number of flag occurrences. There are + four fixed reporting flags and at most two additional reverse searches for + selection provenance; these do not create a quadratic pass. +- Deferred insertion is amortized constant time plus callback-list copying; + draining is linear in queued messages and details, excluding callback/I/O + costs. The avoidable storage and handoff costs are described in finding 6. +- Native diagnostic snapshotting and partitioning are linear in source records, + diagnostic records, and the total related-location entries. Nested copying of + related locations is not quadratic in the actual input size. +- Explanation combination uses amortized slice appends. Configuration comparison + has two sides, and aggregate smoke execution has four selected operations; + neither introduces an unbounded nested traversal through all prior results. + +The algorithmic audit found no additional ordinary correctness issue beyond +findings 1–5. It made no implementation changes and ran no tests or benchmarks. + ## 1. Preserve native PromptKit operation failure explanations Priority: P2. Status: open. @@ -320,3 +350,119 @@ Include a quiet alias followed by another parse error, without suppressing the terminal usage explanation. Retain scoped-alias precedence, unavailable-target, terminator/value handling, and discovery-context coverage at their existing owners. + +## 6. Avoid retaining deferred messages that need no presentation work + +Priority: P3. Status: for consideration. + +### Finding and complexity + +[DeferredReporter.enqueue](../../reporter.go) unconditionally stores the category, +heading, and copied detail list. Visibility is checked only when `finalize` +drains the queue through `submitMessage`. Consequently, a long-running handler +that submits many simple deferred Debug messages in ordinary mode, or simple +Info/Warning messages in quiet mode, retains all of them despite there being +no output or renderer work to perform. + +Let `M` be the number of these renderer-free hidden messages and `B` the bytes +of otherwise unreferenced distinct heading text. Reporter storage can grow as +`Θ(M + B)` until finalization. Finalization then makes a second `Θ(M)` queue +array copy before visiting and suppressing every entry. This is linear, not +quadratic, but the additional reporter storage for this case can be `O(1)`. +The `M` public submissions still require `Θ(M)` total call work; this is not a +claim that the whole invocation can become sublinear. + +### Refactoring opportunity + +Under the current execution flow, reporting selection is applied before handlers +and providers run, with no public operation to change it mid-handler. For a +hidden deferred submission with no non-nil detail callback, consider recording +the eventual suppression observation without retaining the heading or queue +entry. Preserve the existing no-content treatment of an empty heading with no +renderer. This removes the avoidable `Θ(M + B)` retention and its later sweep +for renderer-free suppressed messages. + +Retain rich hidden submissions: their callbacks must still execute at the +selected deferred time against discard so rendering failures are preserved. +Retain visible messages in FIFO order. Closed-reporter checks must still run +before accepting or discarding any submission. This optimization does not add +a public immutability guarantee for future mode APIs or change the selected +emission-time visibility rules. + +Independently, `finalize` can detach the reporter-owned queue under its existing +mutex instead of copying its array and then clearing the original reference. +That makes queue handoff `O(1)` instead of `Θ(M)` extra copying; actual visible/ +rich message delivery remains linear. An internal delivery path can also avoid +recopying callback lists already captured by `enqueue`, while preserving the +selected defensive copy when first accepting caller-owned lists. + +These opportunities preserve the chosen queue and synchronous delivery model. +They do not justify message caps, dropping visible messages, skipping rich +renderers, or a background delivery subsystem. + +### Review evidence + +The retention and copies follow directly from `enqueue`, the array copy at the +start of `finalize`, and `submitMessage`'s later visibility check. If selected, +use focused behavior checks for suppression observations, FIFO delivery, +callback capture, hidden renderer failures, and late submissions. Source-level +storage analysis is sufficient; no benchmark or allocation threshold is proposed. + +## 7. Stop normalizing an argument tail that parsing cannot reach + +Priority: P3. Status: for consideration. + +### Finding and complexity + +In [interpretFlagTokens](../../internal/cobra/flags.go), the first unknown long +flag or unknown short-cluster member sets `recordingOccurrences = false`, but +interpretation continues through the remaining argv. Later recognized flags +still populate spelling lists and construct canonical `--name=value` strings. +[Session.Parse](../../internal/cobra/parse.go) then calls the backend, which +fails at the earlier unknown flag without consuming that normalized tail. +The adapter does not enable unknown-flag whitelisting. + +For example, after `--unknown`, a later valid `--message=LARGE_VALUE` still +causes the value bytes to be copied into a new canonical argument, even though +no parser, converter, or handler reaches that value. The recent occurrence +tracking correctly excludes the tail from selection evidence but retains this +pre-existing unnecessary normalization work. + +Let `F` and `A` be the flag/alias table sizes, `P` the token/byte processing cost +through the first fatal unknown flag, and `T` the remaining normalization work, +including copied value bytes. With expected constant-time map lookups, the helper +currently takes `O(F + A + P + T)` time on that failure path. It can take +`O(F + A + P)` by stopping at the failing token and growing temporary arrays +with the prefix instead of preallocating for all argv. The existing backend can +still produce the same failure from that prefix. This is an early-failure +improvement, not a quadratic-time finding or a change to the successful parse's +necessary linear work. + +### Refactoring opportunity + +Consider ending interpretation immediately after preserving the first unknown +flag or failing short-cluster suffix. Preserve any earlier valid occurrences and +their spellings, including recognized members preceding a bad short member. +Let the backend retain its existing failure classification, wording, and +precedence; do not replace normal parsing with a reporting-specific scanner. +This can also remove the repeated `recordingOccurrences` guards needed solely +because the current implementation continues past the failure. + +The helper currently preallocates both output and occurrence slices from total +argv length, so an early return alone does not eliminate its `O(argc)` upfront +array allocation and initialization. To obtain the prefix-bounded helper cost, +grow those arrays with the interpreted prefix rather than reserving for the +unreachable tail. This changes allocation strategy, not accepted input sizes. + +Other routing/parser steps still copy argv string headers, so this refactoring +does not make the entire invocation prefix-linear. It specifically removes +avoidable tail interpretation, spelling storage, and large-value byte copies. +There is no need to reconsider the selected backend or fresh-tree strategy. + +### Review evidence + +The unknown-flag branches and continued spelling/string construction establish +the extra work statically. Existing prefix-order, missing-value precedence, +short-cluster, and terminator/value tests provide relevant behavior coverage if +this is selected. Preserve the backend error and consumed occurrence prefix on +early failure; no timing threshold or real-world performance data is needed.