diff --git a/docs/roadmap/audit-remediation.md b/docs/roadmap/audit-remediation.md new file mode 100644 index 0000000..100898b --- /dev/null +++ b/docs/roadmap/audit-remediation.md @@ -0,0 +1,285 @@ +# Audit remediation roadmap + +Status: ready for implementation planning; no remediation implemented by this +document. + +Baseline: `d131999c7c474010a4814bb81601cd32920e6c83`. The +[catalog](audit/catalog.md) contains 27 distinct open findings, five P2 and 22 P3, +from seven audits. Its accepted decisions and the +[remediation analysis](audit/remediation-analysis.md) supply the scope and +rationale for this roadmap. The implementation is unchanged since the audited +state `db732e3a1de55c844f77870e1c6db683c2685d5d`; subsequent commits recorded +the review and decisions. + +This roadmap owns the target, work grouping, dependencies, and completion +criteria. The catalog owns finding status and accepted remediation decisions; +the original audit entries own detailed evidence. This is separate from the +older, completed [shared-reporting remediation](remediation.md), whose findings +are not reopened here. Unselected capabilities in [future work](future.md) +remain outside this effort. + +## Current baseline and targeted end state + +CommandKit has three modules. Core owns declarations, routing, typed inputs, +completion, invocation reporting, and outcomes, with Cobra/pflag behind private +adapters. ckconfig exposes configuration inspection through application-supplied +scopes. ckprompt exposes PromptKit inspection, preparation, requirements, and +smoke testing and composes configuration inspection through ckconfig. +Applications retain loading, runtime construction, resources, and executable +lifecycle; upstream libraries retain domain semantics. + +The current manifests select ConfigKit v1.1.0 and PromptKit v0.20.0. Both +integrations select core v0.4.0, and ckprompt selects ckconfig v0.4.0. The ignored +workspace can align the three local source modules. Core declares Go 1.26.0; +the integrations declare Go 1.27.0. This remediation selects no upstream upgrade +or consumer toolchain change. + +The target keeps that architecture and existing command suite while delivering: + +- Accurate outcomes and retained evidence when cancellation, provider mistakes, + or presentation failures occur, and isolation of CommandKit-parsed JSON + between the identified request destinations. +- An explicit deferred-message acceptance cutoff, release of unnecessary + borrowed-writer references after finalization, and one implementation of + shared detail-rendering bookkeeping. +- Working maintained examples and completion validation, with tests exercising + current production paths instead of obsolete private entry points. +- Narrower provider responsibilities and shared ConfigKit diagnostic adaptation + serving both integrations' CLI reporting. +- Shared internal flag-consumption mechanics, reuse of the existing alias tree, + removal of avoidable repeated work, and bounded private extractions. +- Public documentation that accurately describes implemented provider and flag + contracts, with current guidance separated from historical records. + +CommandKit is pre-release with no consumers. Compatibility scaffolding and +consumer migration work are unnecessary for the selected API changes. This does +not alter the handling of published tags or historical release contents. + +## Scope and implementation boundaries + +Apply the [agent](../policy/agents.md), [architecture](../policy/architecture.md), +[testing](../policy/testing.md), and [documentation](../policy/documentation.md) +policies. The [CLI reference](../cli.md) and exported GoDoc remain the current +behavior owners; update them when their corresponding remedy lands. + +The following limits keep the remedies within their accepted scope: + +- F01 retains cancellation when it stops remaining smoke work; it does not make + every cancellation after completed work a failure. +- F02 copies the known CLI JSON maps and arrays, preserving presence and numeric + values. It does not promise arbitrary deep copies of application objects or + repair application-created sharing. +- F03 closes deferred acceptance atomically when finalization begins, presents + the accepted batch under existing visibility and failure rules, and rejects + later deferred submissions with `ErrReporterClosed`. It does not promise + successful delivery when rendering or writing fails, or require waiting for + application producers or owning their goroutines. +- F04 rejects a missing prepared request in that command. Core can still render + a legitimate null report. F05 keeps operation causes distinct from rendering + and writing failures. +- F07 releases direct writer references after final presentation, without + closing or flushing borrowed writers or promising immediate garbage collection. + Retained errors may legitimately retain their own referenced objects. +- F10 documents the agreed current naming rules, including Unicode lowercase + letters and digits, reserved names, and existing short spellings. It adds no + ASCII-only or leading-letter restriction or automatic name normalization. +- F13–F15 are justified by the repeated-work analysis. Performance testing is + not a prerequisite and no timing, allocation, or whole-installation complexity + guarantee is introduced. +- F16–F20 remain internal simplifications. Keep meaningful caller differences, + existing types, and necessary coordination; do not introduce a public parser, + generic pipeline, replay framework, or new concurrency contract. +- F21 serves CLI presentation. It does not load or validate configuration, + choose operation outcomes, or add general application-internal convenience + interfaces. Core remains independent of ConfigKit and PromptKit. + +Preserve native safe evidence, selected report content, error identity, explicit +presence, and the established independent comparison selections through these +changes. Do not replace upstream domain behavior or expand validation merely to +simplify implementation. Private helper names and exact signatures are ordinary +implementation choices unless explicitly selected as public API. + +## Work required + +The groups below cover every catalog finding. They are reviewable work areas, +not a requirement to combine all changes in a group into one commit. Record +implementation evidence against each finding rather than treating a refactor +or a passing broad suite as proof that every nearby defect is fixed. + +### 1. Correct outcomes, request ownership, and reporting lifecycle + +Prioritize these fixes before maintenance refactors. Move any affected tests +off obsolete closure helpers as needed for F24 so they exercise real finalization. + +| Finding | Required work | Evidence of the intended result | +| --- | --- | --- | +| F01 | Retain cancellation at the aggregate smoke checkpoint that omits remaining kinds, preserving previous errors and completed reports. | Cancellation during progress delivery or between operations produces a failed partial outcome with the cancellation cause; prior errors survive and unattempted kinds receive no invented report. | +| F02 | Copy parsed extra-parameter objects and nested arrays/maps at binding application and each aggregate seed boundary. | Mutating one destination does not alter a later smoke request or an independent captured-binding application; empty/nil and numeric distinctions remain intact. | +| F03 | Make deferred acceptance closure and batch detachment atomic while retaining the finalizer's delivery path. Document the cutoff. | A controlled concurrent submission is either accepted for the final batch or rejected as closed; successful acceptance cannot disappear into an abandoned queue. | +| F04 | Reject nil prepared results returned without an error, after the existing acquisition/cancellation handling. | The prepared-preview command fails operationally and retains supplied diagnostics/explanation; valid previews and core null reports retain their existing treatment. | +| F05 | Carry buffered configuration-show cancellation separately from renderer errors. | Interrupted preparation emits no partial aggregate report, retains cancellation as the operation cause, and does not invent a rendering failure; genuine rendering/writing failures remain observable. | +| F07 | Clear direct destination references under the existing reporting lock after final presentation. | Closed handles still reject late operations, returned presentation evidence survives, and CommandKit no longer retains writers through those direct fields or closes them. | + +F03 and F07 touch the same finalization boundary but fix different problems. +Keep them separately explainable. F20's later bookkeeping extraction must not +be used as a substitute for either correction. + +### 2. Restore examples and completion validation + +| Finding | Required work | Evidence of the intended result | +| --- | --- | --- | +| F06 | Remove the completion fixture's collision with core's reserved `format` flag. Use core's flag when testing report selection, or rename the fixture's custom choice flag when testing application completion. Adjust corresponding expectations. | The fixture constructs and generates scripts, and available opt-in shell checks exercise completion rather than failing during installation. | +| F08 | Assign the example's optional scenario argument only when supplied. | Bare `report` runs its `demo` default; an explicitly supplied value still reaches ordinary validation. | + +Finish F06 before validating F16. Use the existing completion harness; its +prerequisites and commands belong to the [developer guide](../development.md#shell-completion-validation). +Report any unavailable shell checks accurately instead of treating skipped +checks as passed. This does not select a new platform support matrix. + +### 3. Clarify and simplify integration boundaries + +| Finding | Required work | Evidence of the intended result | +| --- | --- | --- | +| F09 | Document `EffectiveWriter` as supplying safe YAML for `DecodeYAMLReport`, link the conversion contract, and correct `View`'s report description. | GoDoc describes the actual single-document conversion boundary and does not imply native formatting passthrough or broader YAML support. | +| F10 | Document the accepted naming/reservation rules once in `FlagMeta`; link relevant rename and alias helpers. | Public guidance matches installation behavior without changing the validator or flag-value semantics. | +| F11 | Remove `WriteExplanation` from `ExplanationReader`; update GoDoc and obsolete stubs/assertions. | Custom explanation providers need only acquisition, and standard handlers continue rendering acquired evidence through core. Native objects may keep their own writer methods. | +| F21 | Expose one narrow ckconfig diagnostic adaptation function and use it in both integrations, replacing the duplicated adaptation. | Both callers retain the selected snapshots, severity treatment, source labels, ordering, and native formatting; command-specific context, causes, outcomes, and scheduling remain with callers. | +| F23 | Make concise-diff token decoding accept native `configkit.Path` and remove unreachable arbitrary-string validation. | Literal key tokens, including `~01`, empty keys, slash, tilde, and root paths, remain correct; snapshot failures and actual operator-input validation remain intact. | + +For F21, the accepted input/output shape is a caller-supplied heading, native +source records, and native diagnostic records returning existing core diagnostics +and an explanation. The illustrative `AdaptDiagnostics` name in the analysis is +not a separate required spelling. Reuse the existing private helper's work; +keep the smaller scheduling switches with their callers. No new module is needed. + +For F23, ConfigKit's opaque path is already valid, but the concise report still +needs key-token decoding. Remove revalidation, not that necessary translation. +Use bounded single-pass escape decoding so `~01` becomes `~1`, not `/`. + +### 4. Remove obsolete plumbing and consolidate private responsibilities + +| Finding | Required work | Evidence of the intended result | +| --- | --- | --- | +| F12 | Remove unused execution-resource fields and parser options and redundant normalization. Keep invocation resources with their real owners. | Parsing and buffered help retain their behavior without application input/context/streams threaded through unused backend fields; internal discard destinations still prevent accidental process output. | +| F24 | Move tests to real selection, finalization, routed parsing, and occurrence-aware preparation paths; remove obsolete production wrappers. | Useful assertions remain, superseded paths have no production test support burden, and any small setup helpers live in test files. | +| F16 | Share flag identity, attached/missing values, next-argument consumption, and short-cluster position through small neutral internal helpers. | Execution and completion agree on shared grammar while preserving entered spellings, explicit empty values, and their distinct treatment of unfinished input. | +| F20 | Extract common detail-rendering/error bookkeeping under the existing reporting lock. | Ordinary and terminal reporting retain distinct headings/visibility/timing while sharing rendering-versus-writing classification, callback order, and error retention. | +| F18 | Separate operation checks, scope/capability selection, and two-sided comparison preparation using private functions. | Help and preflight remain metadata/input-only where required; input checks precede state/acquisition, and comparison sides remain independent. | +| F17 | Separate cohesive scope capture/validation and selector attachment work; make operation bindings and discovery inventory construction explicit. | Captured ownership, retained errors, binding identity, discovery, and construction without provider acquisition remain intact. | +| F19 | Extract conversion-error attribution and operand preparation modestly, keeping flag preparation and occurrence ordering together. | Entered spelling, supplied-state distinctions, error identity, application order, and captured replay remain correct without replacing dense reconstruction or the existing merge algorithm. | + +Apply the relevant pieces of F24 alongside the work they support, then verify +that the entire finding is addressed. Among the extraction work, favor F20, +then F18 and F17, keeping F19 conservative. Keep related loops together and use +existing prepared types. Tests should continue to protect behavior rather than +the number of helpers, incidental private call sequences, or complexity scores. + +For F16, retain execution normalization and failure handling with the parser, +and partial-word handling and suggestions with completion. Keep caller-specific +branches where unification requires extensive modes or special cases. Repair +F06 first; this sharing introduces no new alias-completion behavior. + +### 5. Eliminate repeated alias and diagnostic-path work + +| Finding | Required work | Evidence of the intended result | +| --- | --- | --- | +| F13 | Resolve each context's effective flag-alias graph with visitation states and memoized terminal results, including inactive endpoints. | Each shared suffix is resolved once per context; scoped precedence, canonical endpoints, missing-target handling, and cycle diagnostics retain their meaning. | +| F14 | Enumerate distinct nonempty proper source prefixes through the existing resolver tree and classify each once. | Shared prefixes avoid repeated construction/classification; canonical paths, exact aliases, and valid canonical operand space retain their exclusions. | +| F15 | Query strict descendant sources through that same tree for each existing intermediate resolution path. | Help avoids full-catalog scans for unrelated sources while preserving authoritative resolution, longer overrides, canonical-target checks, suffix deduplication, and output order. | +| F22 | Use a scratch path during declaration validation and format full paths only for emitted diagnostics. | Successful validation avoids eager full-path formatting, while retained issues, error identity, child order, and absent/root-only declaration handling remain correct. | + +Implement F14 and F15 as one coordinated change to internal read-only tree +operations. Keep nodes private, captured paths owned correctly, and the existing +routing-snapshot lifetime. Root help may still use the complete catalog when +all entries are needed. Do not add a second index, a persistent result cache, +new alias limits, or alias-completion features as part of these fixes. + +Source-level review should establish removal of the identified repeated work; +behavioral checks establish preserved results. No performance threshold or +benchmark suite is required to justify these remedies. Other routing, +termination-analysis, formatting, and installation costs remain outside those +specific complexity improvements. + +### 6. Finish documentation consistency and close findings + +| Finding | Required work | Evidence of the intended result | +| --- | --- | --- | +| F25 | Replace stale planning/status directions in reporting ADR 0006 with current GoDoc/CLI references, preserving its accepted decision. | The ADR no longer presents implemented reporting as a pending prerequisite; no replacement ADR is introduced for unchanged architecture. | +| F26 | Keep a brief module/status orientation in README and link the consumer guide for exact compatibility and installation details. | Volatile compatibility guidance has one canonical owner. | +| F27 | Add a useful forward-only correction identifying the correct pinned consumer-guide section for the historical ckprompt note. | The correction points to the actual section without modifying the published tag, historical note, or hosted release contents. | + +Update GoDoc, CLI documentation, examples, and focused internal documentation +with the implementation they describe, rather than postponing those updates +until this last group. Check links and semantic claims. Keep existing audit +evidence available and record each finding's implementation/verification result +in the catalog. A selected remedy alone does not close a finding. + +## Verification and module alignment + +Use the narrowest existing behavioral checks that cover each change; add focused +regressions for consequential defects where existing checks do not expose them. +Reuse successful results for unchanged content. Remove tests that sustain deleted +machinery while retaining useful contract coverage. Assertions should establish +CommandKit's adaptation and coordination, not independently test upstream domain +correctness or turn private mechanics into public guarantees. + +For F03, use controlled synchronization to reproduce the acceptance/finalization +boundary rather than timing sleeps. A focused race-enabled run can supplement +that check, but a race detector alone cannot establish correct queue ordering. +For F07, inspect unnecessary reference retention without creating a garbage +collection timing promise. Keep ordinary suites deterministic, offline, and +independent of credentials or live model services. + +Follow the [developer guide](../development.md#documentation-validation) for +module and workspace test commands. The root suite does not traverse the nested +modules. Validate changed modules during their work, then the combined local +source through the aligned workspace, including representative maintained +consumers and the repaired completion checks where available. Use Go 1.27.0 +for this cross-module work as already selected by the repository. + +The independent-module check has a concrete dependency constraint: ckprompt's +new F21 call will not exist in its currently selected ckconfig v0.4.0 dependency. +Workspace success therefore cannot be reported as proof that unchanged +independent module requirements are ready. Preserve the ignored workspace files; +do not commit sibling replacements or fabricate dependency checksums. + +During source development, record workspace evidence separately. Temporary +external-consumer modules or alternate module files with explicit local +replacements may provide additional boundary evidence, but are not validation +of the published dependency selection. Once the required CommandKit module +versions are available, align requirements/checksums and run each independent +module suite with the workspace disabled. Until then, report the independent +dependency limitation explicitly rather than marking that check complete. + +The [release procedure](../release.md) owns dependency-order publication and +candidate validation. Release version selection, notes, and publication remain +separate work; this roadmap neither authorizes publication nor makes it a +prerequisite for source refactoring. When preparing a release, reuse that +procedure rather than inventing additional gates. The absence of consumers +removes migration work, not the need for coherent module requirements. + +## Completion criteria + +- [ ] Every finding F01–F27 has its selected remediation implemented and recorded + with relevant evidence; related findings remain individually accountable. +- [ ] The five P2 defects have focused behavioral protection, including error + identity, retained evidence, and the deferred acceptance boundary. +- [ ] F06 and F08's maintained consumers work, with actual completion checks + distinguished from unavailable or skipped shell checks. +- [ ] F11 and F21's selected API changes and F09/F10's clarified contracts are + reflected in canonical GoDoc and affected integration guidance. +- [ ] Obsolete production wrappers/plumbing and duplicate adaptation/bookkeeping + are removed at the agreed boundaries, with useful tests retained. +- [ ] F13–F15 and F22 remove the identified repeated work while preserving + behavior, without a new performance guarantee or measurement prerequisite. +- [ ] F17–F20 improve responsibility separation within their bounded scope; + public behavior and necessary ordering remain protected. +- [ ] Documentation corrections F25–F27 are complete and local links resolve. +- [ ] Relevant module and aligned-workspace results are recorded. Independent + results identify the exact dependency selection; any release-alignment work + still pending is explicitly distinguished from completed source work. + +The significant choices have been resolved in the catalog and remediation +analysis. Routine implementation details do not require an Open Decisions +section. Revisit a selected direction only if concrete implementation evidence +reveals a conflict or materially different tradeoff.