16 KiB
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 and implementation plan.
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
commandkitpackage owns public named types, opaque handles, per-instance registration, typed bindings, outcomes, standard handlers, scope assembly, and ConfigKit/PromptKit integration. internal/cobraowns 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
ExtraParamswithout 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, adaptContextSelector and
configInputFlags; 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, 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 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,
show.go, sources.go, and
explain.go; definition handlers and engine loading in
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; 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, runConfigValidation and
validationFailure; 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, present; 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, Diagnostic GoDoc;
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, 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, 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, Group;
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.