Record initial implementation remediation findings

docs/roadmap/remediation.md

@@ -0,0 +1,290 @@
+# Initial implementation review and remediation
+
+Status: findings recorded; implementation fixes pending.
+
+Date: 2026-10-07.
+
+## Review baseline and scope
+
+Reviewed committed implementation at `85f44cd`, including the changes since
+planning baseline `e5c361d`. The review covered production code, public contracts,
+the internal adapter, integration helpers, maintained examples, and existing
+behavioral tests against the accepted ADRs, architecture policy, and selected
+[initial target](initial.md) and [implementation plan](implementation.md).
+
+These are static review findings. No tests were added or executed during this
+review; prior completion notes are historical evidence, not a new validation
+result. The triggering cases below follow from the current code paths. Focused
+regression checks are proposed completion evidence for subsequent fixes.
+
+This document records remediation work only. Implementation remains unchanged.
+
+## Architecture assessment
+
+The main architecture is implemented:
+
+- The root `commandkit` package owns public named types, opaque handles,
+  per-instance registration, typed bindings, outcomes, standard handlers, scope
+  assembly, and ConfigKit/PromptKit integration.
+- `internal/cobra` owns Cobra/pflag operations behind a neutral internal bridge.
+  Public signatures do not expose the backend. Each invocation rebuilds backend
+  state; configuration and engine acquisition remain lazy.
+- Standard families own their namespaces. Applications retain custom commands,
+  loaders, domain behavior, executable entry points, streams, and process exit.
+- Scope capabilities and prompt/profile callbacks can be supplied independently.
+  Native YAML writers and public upstream inspection APIs are used. Profile
+  reports include `ExtraParams` without additional redaction.
+- The module uses published dependencies rather than sibling internal packages
+  or local replacement directives. Simple and composed consumer examples exist.
+
+No package restructuring or additional implementation framework is recommended.
+Diff, catalogs, JSON output, consumer migrations, publication tooling, and
+concurrent-dispatch guarantees are deliberate deferrals, not missing initial
+architecture. The issues below affect behavior within the implemented structure.
+
+## Findings and priority
+
+P1 means a supported invocation is blocked or its report content is corrupted.
+P2 means a selected behavior or public contract is incomplete or unnecessarily
+restricted. All listed items need resolution before calling the initial target
+fully aligned.
+
+| ID | Priority | Finding | Review category |
+| --- | --- | --- | --- |
+| R-01 | P1 | Required selectors block scopes to which they do not apply | Strengthened requirement; behavior defect |
+| R-02 | P1 | Aggregate YAML framing inserts empty documents for inline start markers | Report correctness |
+| R-03 | P2 | Standard inspection handlers omit cancellation boundary checks | Target gap |
+| R-04 | P2 | Help shortcuts bypass diagnostic presentation | Target gap; inconsistent delivery |
+| R-05 | P2 | Validation report failures omit `DeliveryError` | Public outcome contract gap |
+| R-06 | P2 | Zero outcomes and empty delivery state are not normalized | Target gap |
+| R-07 | P2 | Nil diagnostic writers are rejected despite the documented no-output behavior | Strengthened prohibition; contract inconsistency |
+| R-08 | P2 | Unknown/default absent scopes omit available choices | Operator contract gap |
+| R-09 | P2 | PromptKit selectors reserve flag names absent from installed commands | Strengthened prohibition |
+| R-10 | P2 | Parent help omits registered groups containing only help metadata | Discovery gap |
+
+### R-01: enforce selector requiredness only where applicable
+
+**Evidence:** [selectors.go](../../selectors.go), `adaptContextSelector` and
+`configInputFlags`; [binding.go](../../binding.go), `prepareInput`.
+
+All configuration operation commands receive every suite selector. Adaptation
+copies `FlagMeta.Required` unchanged, and generic input preparation checks it
+before scope selection or applicability checks.
+
+With an app-only required `--campaign` selector and app/PromptKit validation
+scopes, `config validate --scope promptkit` fails because campaign is missing.
+Supplying campaign instead fails because it does not apply to PromptKit. There
+is no successful invocation for that independently supported scope.
+
+**Authority:** selected scope-specific routing in INT-01 and the initial
+roadmap's selector section. A required selector for one scope does not authorize
+requiring it for every other scope.
+
+**Remediation:** apply selector requiredness after selecting applicable scopes.
+Keep ordinary custom-command required flags unchanged. Preserve requiredness
+and applicability in help metadata without an unconditional preparation check.
+
+**Completion evidence:** an unrelated scope succeeds without the selector; an
+applicable scope fails before acquisition when it is missing; `all` enforces
+the requirements of its selected scopes; help remains available.
+
+### R-02: recognize inline YAML document-start markers
+
+**Evidence:** [show.go](../../show.go), `frameEffectiveYAML`.
+
+The framer recognizes only a line equal to `---` or starting with `--- #`.
+A native document such as `--- {setting: value}\n` already has a valid document
+start, but receives another opening marker and scope comment. The result starts
+with an empty document followed by the actual value, so document count and scope
+association are wrong even though execution returns success.
+
+**Authority:** [ADR 0007](../adr/0007-native-yaml-reports.md) and CFG-03 require
+native YAML preservation and one framed document per selected scope.
+
+**Remediation:** recognize the document-start token with supported whitespace and
+inline content. Preserve native bytes; a parallel decode/remarshal pipeline or
+a new prohibition on valid native YAML is unnecessary.
+
+**Completion evidence:** aggregate decoding yields exactly one value per scope
+for inline mapping/scalar starts, ordinary marked documents, unmarked documents,
+and directive-bearing documents, while preserving native content.
+
+### R-03: check cancellation at standard inspection boundaries
+
+**Evidence:** `runConfigValidation`, `runConfigShow`, `runConfigSources`, and
+`runConfigExplanation` in [validate.go](../../validate.go),
+[show.go](../../show.go), [sources.go](../../sources.go), and
+[explain.go](../../explain.go); definition handlers and engine loading in
+[promptkit.go](../../promptkit.go).
+
+The handlers pass context to callbacks but do not check `Context.Err()` at their
+own boundaries. With an already canceled context and a successful loader that
+does not itself inspect context, configuration inspection can acquire and emit a
+successful report. Direct PromptKit suite callbacks have the same exposure;
+the engine helper also loads before delegating to context-aware named inspection.
+
+**Authority:** INV-01/OUT-01 and implementation Stage 5 explicitly select standard
+handler checks at operation/acquisition boundaries, with supported cancellation
+identity preserved. ADR 0005 classifies cancellation as operational failure.
+
+**Remediation:** check context at standard acquisition/report boundaries and
+between aggregate acquisitions. Retain `context.Canceled`/deadline identity and
+existing diagnostics/partial-report information. This does not require a
+goroutine cancellation runner, interruption of callbacks, or new restrictions on
+application-owned custom command semantics.
+
+**Completion evidence:** cancellation before acquisition and during acquisition
+returns operational failure with the context cause, and prevents subsequent
+standard work at the next boundary. Include an acquisition callback that ignores
+context so the test protects CommandKit's responsibility.
+
+### R-04: finalize help outcomes through the presentation path
+
+**Evidence:** [execute.go](../../execute.go), `Execute`; [builtins.go](../../builtins.go),
+`renderHelpOutcome`.
+
+Root help, non-runnable group help, and `--help`/`-h` return directly from
+`renderHelpOutcome`. A failed help report write returns an operational outcome
+but bypasses `present`, so it produces no operational diagnostic and cannot
+record a simultaneous diagnostic-stream failure. Explicit `help PATH` goes
+through the normal handler finalization and behaves differently.
+
+**Authority:** ADR 0005's diagnostic channels and INV-01's single presentation path.
+
+**Remediation:** route these outcomes through the same finalization path exactly
+once, preserving existing report failure fields and lazy metadata behavior.
+
+**Completion evidence:** all help entry forms have consistent failure delivery;
+broken report and diagnostic streams retain both errors without double delivery.
+
+### R-05: retain validation report errors in `DeliveryError`
+
+**Evidence:** [validate.go](../../validate.go), `runConfigValidation` and
+`validationFailure`; [outcome.go](../../outcome.go), `Outcome`.
+
+Both validation header and record write failures call `validationFailure`, which
+sets cause and incomplete-report state but never sets `DeliveryError`. With a
+working diagnostic stream, the returned delivery error remains nil despite a
+failed report write. Other standard report paths record this field.
+
+**Remediation:** distinguish report-writing failures from acquisition failures
+and retain the former in `DeliveryError`, alongside the operational cause.
+Do not misclassify every validation acquisition error as a delivery error.
+
+**Completion evidence:** header/record errors and short writes populate both
+relevant error fields and retain incomplete-report state. Combined stdout/stderr
+failures preserve both errors. Existing validation short-write checks inspect
+the report cause but do not check its `DeliveryError`.
+
+### R-06: implement the selected outcome normalization
+
+**Evidence:** [execute.go](../../execute.go), `present`; [outcome.go](../../outcome.go).
+
+A handler returning `Outcome{}` retains an empty kind and delivery state because
+`present` returns it unchanged when there are no diagnostics. An explicit domain
+outcome with an omitted diagnostic state likewise retains an empty state.
+
+**Authority:** the selected outcome model, specified in implementation Stage 1,
+normalizes a zero outcome to success/0 and an empty delivery state to
+`DeliveryNone`. The latter is also stated in public GoDoc.
+
+**Remediation:** normalize these selected defaults during invocation coordination.
+Preserve explicit domain classification, status, cause, and delivery information;
+do not introduce inference of domain meaning from numeric status.
+
+**Completion evidence:** a zero handler outcome returns success/0/none; explicit
+domain outcomes retain their values with an omitted delivery state normalized.
+
+### R-07: honor nil diagnostic writers as no-output diagnostics
+
+**Evidence:** [outcome.go](../../outcome.go), `Diagnostic` GoDoc;
+[execute.go](../../execute.go), `present` and `hasErrorDiagnostic`.
+
+GoDoc says a nil `Write` function has no output. Coordination instead turns it
+into an operational delivery failure with `diagnostic has no writer`. Thus an
+otherwise successful result containing a zero diagnostic becomes a failure.
+An error-severity diagnostic with a nil writer also suppresses automatic cause
+presentation because deduplication checks severity alone.
+
+**Remediation:** treat nil writers as no-output entries, consistent with the
+documented behavior, and exclude them from cause-presentation deduplication and
+delivery-attempt bookkeeping. Requiring every entry to have a writer would be a
+new API restriction requiring a deliberate decision.
+
+**Completion evidence:** nil writers do not change success/domain classifications
+or suppress a real cause; active diagnostic writer failures remain observable.
+
+### R-08: include available scope choices in selection failures
+
+**Evidence:** [selectors.go](../../selectors.go), `selectConfigScopes`.
+
+Unknown selection returns only `unknown configuration scope NAME`. This also
+occurs when only a PromptKit scope exists and the default `app` scope is absent.
+The operator receives no available choices.
+
+**Authority:** CFG-01 expressly requires useful choices for unsupported scopes
+and absent `app`; ADR 0009 requires available choices for unsupported selections.
+
+**Remediation:** include registered scope choices in stable scope order. Retain
+the usage classification, default selection, strict aggregate coverage, and
+pre-acquisition failure; do not silently select an alternative.
+
+**Completion evidence:** explicit unknown scope and omitted selection with absent
+`app` identify available choices without invoking runtime callbacks.
+
+### R-09: reserve selector names for actual command collisions
+
+**Evidence:** [promptkit.go](../../promptkit.go), `normalizePromptKitSuite` and
+`promptKitInputFlags`.
+
+Normalization unconditionally rejects a selector called `version`, even when
+only profile inspection is supplied. `--version` is installed only on prompt
+inspection, so there is no flag collision in a profile-only suite. It also
+reserves `scope` despite not installing a scope-selection flag on definitions.
+
+**Authority:** capability-based family assembly and the agent policy's prohibition
+on inventing restrictions. The selector rules in Stage 7, reused by Stage 12,
+call for detecting collisions with standard local flags, not a reservation of
+absent flags for hypothetical future use.
+
+**Remediation:** detect collisions against flags actually installed on commands
+receiving the selector. Keep genuine collisions rejected. If a broader vocabulary
+reservation is desired, record it as a new decision rather than a parser necessity.
+
+**Completion evidence:** a profile-only selector named `version` can register;
+the same selector is rejected when it collides with installed prompt inspection.
+Review the absent `scope` reservation against the same rule.
+
+### R-10: expose registered metadata groups in parent help
+
+**Evidence:** [command.go](../../command.go), `Group`;
+[internal/cobra/parse.go](../../internal/cobra/parse.go), `buildTree` and
+`usageTemplate`; pinned Cobra v1.10.1 `Command.IsAvailableCommand`.
+
+CommandKit accepts a non-runnable group with no children, and direct invocation
+renders its help. However, parent help filters commands with Cobra's
+`IsAvailableCommand`, which requires a runnable command or an available
+descendant. A metadata-only group, or a chain containing only such groups,
+disappears from the parent's command inventory despite being registered and
+addressable. The custom template provides no additional help-topic inventory.
+
+**Authority:** HELP-01 and the architecture policy select discovery from registered
+metadata; Cobra's availability defaults do not independently select a CommandKit
+visibility restriction.
+
+**Remediation:** derive help inventory from CommandKit's declared commands,
+including groups that expose help. Do not prohibit empty groups or add fake
+application handlers solely to satisfy a backend visibility rule.
+
+**Completion evidence:** parent/root help discovers metadata-only groups and
+their nesting; direct help works and leaves acquisition callbacks uninvoked.
+
+## Follow-up boundaries
+
+Address the behavioral findings with focused fixes and regression evidence at
+the public invocation boundary or the narrow framing boundary as appropriate.
+Update GoDoc/CLI references where needed to match the selected contracts.
+Do not expand this work into upstream redaction changes, general framework
+construction, new consumer mandates, blanket validation gates, or new concurrency
+guarantees. Recheck the completed target after remediation; this review does not
+claim that the existing tests establish these missing cases.