# Initial implementation review and remediation

Status: remediation in progress.

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.

The findings below describe the review baseline. Completion is tracked here as
implementation fixes land.

## Remediation progress

- R-01 complete: required selectors are checked after scope selection; unrelated
  scopes remain usable, and help describes conditional requiredness.

## 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.

- R-02 complete: inline document starts are recognized and native content is
  retained, including block scalars.

## 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.