Retire completed shell completion roadmaps
This commit is contained in:
@@ -1,187 +0,0 @@
|
||||
# Shell completion
|
||||
|
||||
Status: the selected initial scope is implemented and validated. Canonical
|
||||
suggestions and alias-aware interpretation are available for
|
||||
Bash, Zsh, and Fish through explicitly installed completion support.
|
||||
|
||||
## Purpose
|
||||
|
||||
Provide useful shell completion for core-only applications and applications with
|
||||
optional integrations. Reuse installed command declarations and CommandKit's
|
||||
routing rules so consumers need little completion-specific wiring.
|
||||
|
||||
Completion helps an operator construct an invocation. It does not execute the
|
||||
selected application command. Preserve application ownership of runtime behavior
|
||||
and upstream ownership of configuration and prompt semantics.
|
||||
|
||||
## Planning baseline
|
||||
|
||||
Reviewed baseline: `2301872` (`Stop flag interpretation at unknown flags`).
|
||||
|
||||
- Core captures declarations and builds a routing snapshot containing canonical
|
||||
commands and separate application aliases.
|
||||
- [Neutral metadata](../../internal/commandmeta/metadata.go) describes command
|
||||
names, descriptions, flags, boolean/value distinctions, repetition, required
|
||||
flags, and operand cardinality. It has no completion choices or path hints.
|
||||
- The [Cobra adapter](../../internal/cobra/parse.go) constructs a backend tree
|
||||
for parsing and help. CommandKit owns execution and presentation outside that
|
||||
tree; enabling Cobra's default completion command alone is insufficient.
|
||||
- [Alias routing](../../route.go) resolves complete command paths and coordinates
|
||||
canonical parsing with entered spellings. Completion must also handle partial
|
||||
words and an empty current word.
|
||||
- Invocation reporting controls ordinary output and diagnostics. Shell completion
|
||||
queries need machine-readable replies independent of flags in the unfinished
|
||||
command line.
|
||||
- The core module uses Cobra v1.10.1 and pflag v1.0.9. Cobra supplies script
|
||||
generators and completion callbacks for Bash, Zsh, Fish, and PowerShell.
|
||||
See its [versioned completion documentation](https://github.com/spf13/cobra/blob/v1.10.1/site/content/completions/_index.md).
|
||||
|
||||
[ADR 0013](../adr/0013-application-alias-routing.md) owns the separation between
|
||||
alias routes and canonical commands. [ADR 0014](../adr/0014-invocation-reporting.md)
|
||||
owns invocation reporting. This roadmap defines completion within those
|
||||
boundaries; it does not establish a replacement command model.
|
||||
|
||||
## Target experience
|
||||
|
||||
An operator generates and loads a shell integration script, then receives
|
||||
suggestions for installed commands, applicable canonical flags, and declared
|
||||
value choices or paths. The selected script-generation grammar is:
|
||||
|
||||
```text
|
||||
app completion bash
|
||||
app completion zsh
|
||||
app completion fish
|
||||
```
|
||||
|
||||
Consumers explicitly install a core completion declaration through the existing
|
||||
installation API. The completion namespace is reserved only when installed.
|
||||
Keep the machine query hidden from ordinary help, with collision validation for
|
||||
its selected path. Installation must not replace an application-owned command.
|
||||
|
||||
Script generation writes to the caller-selected result stream. Shell setup
|
||||
instructions explain loading the script; automatic modification of shell startup
|
||||
files is outside the initial implementation.
|
||||
|
||||
The initial MVP suggests canonical command names and flag spellings. Completed aliases
|
||||
already entered by the operator are interpreted to identify the canonical target.
|
||||
For example, with `jobs` routing to `workspace jobs`, completing after
|
||||
`app jobs ` can suggest the canonical children of `workspace jobs`.
|
||||
Completion should preserve the text the operator has already entered.
|
||||
|
||||
Matching partial canonical names, resolving completed aliases, and interpreting
|
||||
an alias-only discovery prefix are distinct cases. Initially, an alias-only
|
||||
prefix with no canonical target offers no command continuations. For example,
|
||||
if `legacy inspect` is an alias but `legacy` has no canonical target,
|
||||
`app legacy <TAB>` offers no command continuations. After the complete alias is
|
||||
entered, completion uses its canonical target. Applicable flag suggestions can
|
||||
still follow the existing discovery context.
|
||||
|
||||
Suggested continuations must form a valid next step when inserted into the
|
||||
entered route. Do not append canonical words that would create an invalid route.
|
||||
Alias discovery may be omitted from the initial MVP; this is a scope choice,
|
||||
not a permanent limitation of CommandKit. Suggesting alias names or alias-path
|
||||
continuations remains [potential future work](deferred.md#alias-completion-and-discovery).
|
||||
|
||||
### Flag suggestions
|
||||
|
||||
Initially suppress already supplied singleton flags from flag-name suggestions.
|
||||
Canonical and aliased occurrences count as the same flag. Keep repeatable flags
|
||||
available. Keep all four reporting flags (`--quiet`, `--verbose`, `--debug`, and
|
||||
`--progress`) available because later occurrences can intentionally change or
|
||||
reset their settings.
|
||||
|
||||
Suggestion filtering leaves accepted syntax unchanged. Operators can still type
|
||||
singleton flags again under the existing repetition rules. If an operator has
|
||||
already entered a repeated flag, continue completing its value according to its
|
||||
hint even when its name would have been omitted from suggestions.
|
||||
|
||||
## Selected initial scope
|
||||
|
||||
Provide canonical command and flag suggestions, descriptions where supported by
|
||||
the selected shells, alias-aware interpretation, static value choices, and
|
||||
file/directory hints. Apply the same declaration facilities to application-owned
|
||||
commands and optional integrations.
|
||||
|
||||
Support Bash, Zsh, and Fish using the existing backend generators, with validation
|
||||
in each shell. PowerShell is outside the initial supported shell set because
|
||||
there is no current consumer need; this does not preclude future support.
|
||||
|
||||
Static suggestions should come from captured metadata. Completion should not
|
||||
invoke ordinary handlers, input/state factories, or acquisition callbacks to
|
||||
obtain it. Application startup and declaration-factory effects remain owned by
|
||||
the consumer, just as they do for help.
|
||||
|
||||
Dynamic discovery of configured prompt names, remote resources, or other live
|
||||
values is a possible later extension. Deferring it avoids selecting provider
|
||||
lifetimes and latency behavior before a concrete consumer need; it is not a
|
||||
permanent prohibition on dynamic completion.
|
||||
|
||||
## Ownership and implementation approach
|
||||
|
||||
Core owns completion context, alias interpretation, and neutral suggestions.
|
||||
The internal backend adapter owns Cobra-specific types, directives, script
|
||||
emission, and protocol details. Prefer the existing backend facilities where
|
||||
compatible with CommandKit's grammar. Verify the adapter approach with a small
|
||||
end-to-end prototype before fixing private interfaces.
|
||||
|
||||
A completion request identifies completed words and the current partial word,
|
||||
including an empty word after a space. Interpret only what is needed to locate
|
||||
suggestions. Ordinary required-value validation and input conversion must not
|
||||
reject a partially constructed invocation before completion can help fill it.
|
||||
Reuse token and flag interpretation helpers where practical without changing
|
||||
normal execution grammar or treating incomplete input as an executable command.
|
||||
|
||||
Account for command-local flags, aliases, consumed values, `--`, attached values,
|
||||
boolean assignments, short clusters, repeatable flags, and operand positions.
|
||||
Keep provenance sufficient to return a replacement for the word being edited.
|
||||
Completion candidates are suggestions, not an additional input validator.
|
||||
|
||||
When both an operand and a flag are valid, use the operand's hint for an
|
||||
empty current word after a space. Offer flag names when the operator starts
|
||||
the word with `-` in a context where flags are accepted.
|
||||
|
||||
Value choices and path hints belong in CommandKit-owned public metadata. Carry
|
||||
them through declaration capture and neutral metadata to the adapter. Arbitrary
|
||||
converter functions cannot reliably reveal their accepted values. Adding a list
|
||||
of suggestions must not silently restrict the values a converter accepts.
|
||||
|
||||
Provide declarative static choices with optional descriptions, file/directory
|
||||
hints, and an explicit way to request no value suggestions. Unannotated values
|
||||
default to no value suggestions; applicable commands and flags still complete.
|
||||
Filename completion requires a path hint. This keeps unrelated filenames out of
|
||||
identifier and numeric completions. The implementation plan specifies how hints
|
||||
fit into existing binding declarations.
|
||||
|
||||
Initially use one hint strategy per value position: static choices, files,
|
||||
directories, or no value suggestions. Optional choice descriptions belong to the
|
||||
static-choice strategy. Combined choices and filesystem suggestions can be
|
||||
considered later if consumers need them; the initial API need not define their
|
||||
merging or deduplication behavior.
|
||||
|
||||
Verify the same strategy works for separated and attached flag values, such as
|
||||
`app serve --config ./par<TAB>` and `app serve --config=./par<TAB>`. For the
|
||||
attached form, preserve `--config=` and complete its value correctly. Validate
|
||||
the resulting command line in each supported shell.
|
||||
|
||||
Keep ordinary script generation under the existing reporting policy. A hidden
|
||||
shell query is a protocol exchange: flags in the command being completed are
|
||||
request data, so `--quiet` must not erase its reply and `--debug` must not append
|
||||
ordinary diagnostics to it. When an unrecognized or incomplete command line
|
||||
leaves no inferable completion context, return a valid empty reply that disables filename fallback. Reserve
|
||||
protocol errors for failures in the query itself. Specify their wire behavior
|
||||
and output ownership in the implementation plan. A completion request must not
|
||||
accidentally run the target command.
|
||||
Hidden protocol names are implementation details unless deliberately published.
|
||||
|
||||
## Delivery plan
|
||||
|
||||
The [implementation plan](implementation.md) owns the sprint sequence, API and
|
||||
protocol design, and validation steps. The target includes behavioral coverage
|
||||
and shell-level validation through the public consumer path in Bash, Zsh, and
|
||||
Fish.
|
||||
|
||||
## Deferred scope
|
||||
|
||||
Alias discovery remains a potential extension beyond the MVP. Dynamic discovery
|
||||
remains deferred until a concrete consumer need justifies its callback,
|
||||
cancellation, error-handling, and provider-lifetime design.
|
||||
@@ -27,7 +27,7 @@ relevant ADRs.
|
||||
|
||||
### Alias completion and discovery
|
||||
|
||||
The [shell-completion roadmap](completion.md) selects an initial MVP with
|
||||
The delivered [shell completion feature](../cli.md#shell-completion) provides
|
||||
canonical suggestions and alias-aware interpretation. Potential future work
|
||||
includes suggesting command and flag alias spellings and valid continuations of
|
||||
alias-only prefixes. For example, completion could suggest `inspect` after
|
||||
@@ -45,9 +45,8 @@ These capabilities remain outside the delivered scope and require a concrete
|
||||
operator or consumer need and an appropriate ownership/behavior decision:
|
||||
|
||||
- JSON reports and output-file management or reloadable export.
|
||||
- Shell-completion extensions beyond the initial scope in the
|
||||
[completion roadmap](completion.md), including alias discovery described above
|
||||
and dynamic discovery of configured or remote values.
|
||||
- Shell completion extensions, including alias discovery described above and
|
||||
dynamic discovery of configured or remote values.
|
||||
- Configuration mutation, generic value assignment, and exhaustive field-origin
|
||||
inventories.
|
||||
- Credential diagnostics beyond PromptKit's native smoke checks and additional
|
||||
|
||||
@@ -1,345 +0,0 @@
|
||||
# Shell completion implementation plan
|
||||
|
||||
Status: Stages 1–8 complete.
|
||||
|
||||
Implement the [shell completion roadmap](completion.md) from baseline
|
||||
`2301872`. Follow the stages in order in one sprint. Intermediate stages may
|
||||
leave unfinished wiring or a noncompiling tree; validate the assembled result
|
||||
before calling the sprint complete. Use the existing ownership boundaries in
|
||||
[ADR 0013](../adr/0013-application-alias-routing.md),
|
||||
[ADR 0014](../adr/0014-invocation-reporting.md), and the
|
||||
[architecture policy](../policy/architecture.md). Apply the
|
||||
[testing policy](../policy/testing.md) to integration assertions. Keep protocol
|
||||
names, candidate ordering, and backend mechanics private.
|
||||
|
||||
## Stage 1: Prove the shell transport
|
||||
|
||||
Status: complete.
|
||||
|
||||
1. Use the Cobra version pinned in the root module. In a disposable prototype,
|
||||
generate scripts with `GenBashCompletionV2(w, true)`,
|
||||
`GenZshCompletion(w)`, and `GenFishCompletion(w, true)`. Give the generator
|
||||
a minimal root whose name is the installed executable basename. Do not
|
||||
install Cobra's completion command or execute its completion parser.
|
||||
2. Supply a fixture executable that handles the generated scripts' hidden
|
||||
request (`__complete` with these generator settings). Verify the actual
|
||||
arguments and replies using each script in its shell. The request carries
|
||||
the words through the cursor, including the partial word as its last
|
||||
argument; preserve an explicitly empty last argument. Check what each
|
||||
script does with the prefix of an attached value before deciding how to
|
||||
encode its replacement. Cover `--config=./par`, separated path values,
|
||||
spaces, an empty no-file reply, directory-only completion, and a partial
|
||||
short cluster. Verify the edited line, not only the printed candidate list.
|
||||
3. Use Cobra's line protocol: candidate lines with optional tab-separated
|
||||
descriptions, followed by the colon-prefixed directive integer and newline.
|
||||
Use the adapter's Cobra directive constants, not hardcoded numbers in core.
|
||||
Map command/flag/static choices and no candidates to NoFileComp, file hints
|
||||
to native file completion, and directory hints to FilterDirs. If a generated
|
||||
script cannot express the selected behavior as emitted, make the smallest
|
||||
adapter-owned script adjustment that preserves it and add a shell regression.
|
||||
In particular, verify attached-value prefix reconstruction and Fish's
|
||||
filesystem handling. Do not narrow the supported contexts to fit a script.
|
||||
4. Record the observed argument and replacement conventions in the sprint
|
||||
evidence. Carry forward only the adapter and fixtures needed for the final
|
||||
implementation; do not commit disposable prototype machinery.
|
||||
|
||||
Completion: generated scripts using Cobra v1.10.1 and exercised them through
|
||||
interactive Bash 5.3.9, Zsh 5.9, and Fish 4.2.1 with a fixture that records the
|
||||
query and the executed edited line. In the tested attached-value cases the
|
||||
query includes the entered flag and value; a static candidate contains the value
|
||||
alone, and the shell script preserves the flag prefix. Explicit empty words,
|
||||
quoted prior arguments, short clusters, file and directory hints, and no-file
|
||||
replies were exercised. Zsh's generated word splitting lost the attached flag
|
||||
context when a path contained an escaped space. Fish split quoted prior words,
|
||||
passed escapes in current words, and treated directory-only replies as ordinary
|
||||
file completion. The adapter adjusts those scripts and preserves the first `=`
|
||||
as the flag/value separator. The opt-in `TestCompletionShells` regression checks
|
||||
actual edited lines for these cases. Run it with Bash, Zsh, Fish, Python pexpect,
|
||||
and Bash completion installed using `COMMANDKIT_COMPLETION_SHELLS=1
|
||||
GOWORK=off GOTOOLCHAIN=local go test -count=1 -run '^TestCompletionShells$'
|
||||
./internal/cobra`. Zsh and Fish were extracted into a disposable directory for
|
||||
this run; the focused adapter suite and the full interactive matrix pass.
|
||||
|
||||
## Stage 2: Capture completion hints
|
||||
|
||||
Status: complete.
|
||||
|
||||
1. Add the following public API in the root package. Add
|
||||
`Completion CompletionHint` to both `FlagMeta` and `ArgumentMeta`;
|
||||
existing binding constructors and their signatures stay in place.
|
||||
|
||||
```go
|
||||
type CompletionChoice struct {
|
||||
Value string
|
||||
Description string
|
||||
}
|
||||
|
||||
type CompletionHint struct { /* private representation */ }
|
||||
|
||||
func CompleteChoices(choices ...CompletionChoice) CompletionHint
|
||||
func CompleteFiles() CompletionHint
|
||||
func CompleteDirectories() CompletionHint
|
||||
func NoCompletion() CompletionHint
|
||||
```
|
||||
|
||||
2. Represent the four strategies as a private discriminator and choice list.
|
||||
The zero hint and `NoCompletion()` are equivalent. Copy choice slices
|
||||
when constructing the hint. Capture the hint through ordinary bindings,
|
||||
repeatable bindings, and `CaptureFlags`; bridge it through
|
||||
`bridgeFlags` and `bridgeArguments` to neutral metadata. Use neutral
|
||||
strategy and choice types in `internal/commandmeta`; backend types do not
|
||||
cross the adapter boundary.
|
||||
3. Keep hints independent of binding identity, requiredness, defaults,
|
||||
repetition, and converters. Static choices are literal prefix matches and
|
||||
do not constrain accepted values. Empty choices mean no suggestions, not
|
||||
file fallback. Use ordinary boolean literals `true` and `false` for
|
||||
the built-in boolean assignment case; an application boolean binding
|
||||
without a hint has no value suggestions.
|
||||
4. Verify caller mutation of the original choice slice cannot change captured
|
||||
suggestions, repeated/captured bindings keep hints, and values absent from
|
||||
a suggestion list still reach the existing converter.
|
||||
|
||||
Completion: added the public choice and hint constructors, carried hints through
|
||||
flag and operand bindings into neutral metadata, and supplied explicit boolean
|
||||
choices for built-in help and reporting flags. Focused tests cover copied
|
||||
choices, repeated and captured bindings, the empty-choice strategy, independent
|
||||
bridge copies, and execution of values outside the suggested choices. Root,
|
||||
internal adapter, and neutral metadata package tests pass with `GOWORK=off` and
|
||||
`GOTOOLCHAIN=local`.
|
||||
|
||||
## Stage 3: Install completion as an ordinary declaration
|
||||
|
||||
Status: complete.
|
||||
|
||||
1. Add `func Completion(executable string) Command`. The parameter is the
|
||||
executable basename the operator types, independent of the human-readable
|
||||
`Identity.Name`. Use a root-only `completion` group with three runnable
|
||||
children, `bash`, `zsh`, and `fish`. Use the existing declaration and
|
||||
registration API (`New(..., Completion("app"))` or
|
||||
`app.Register(Completion("app"))`). Keep script-specific name handling
|
||||
inside completion; do not change validation of ordinary application identity.
|
||||
2. Carry a private completion-installation marker with the declaration and
|
||||
derive query availability from the committed routing snapshot. Use the
|
||||
existing registration candidate to validate collisions in either order,
|
||||
including aliases and same-batch declarations. Reserve the query's first
|
||||
token only while installed: a canonical command or alias beginning with
|
||||
that token would be shadowed by raw query dispatch. Reuse existing
|
||||
command/alias collision rules for `completion`. Do not represent the
|
||||
query as a canonical node, alias target, or public help entry.
|
||||
3. Generate scripts in the internal adapter using the transport established
|
||||
in Stage 1. Surface generator errors through ordinary presentation.
|
||||
Use `Reporter.RenderOutput` for script generation and delivery, so
|
||||
`app completion bash --quiet` follows normal reporting. Document the
|
||||
basename expected by the generated scripts; handle unsupported generator
|
||||
names at the completion boundary.
|
||||
4. Verify opt-in behavior, root-only placement, help visibility, batch
|
||||
rollback, late installation, and absence of new reserved names for an app
|
||||
that does not install completion.
|
||||
|
||||
Completion: added the root-only completion declaration and three script
|
||||
commands, with generator output delivered through the invocation reporter.
|
||||
Registration validates hidden-query collisions in either order, and the routing
|
||||
snapshot reflects committed installation. Tests cover help, quiet output,
|
||||
delivery failure, invalid basenames, opt-in behavior, collisions, rollback,
|
||||
nesting, and late installation. Fish variable names are adjusted to support
|
||||
dotted executable basenames; interactive checks of those names pass in Bash,
|
||||
Zsh, and Fish. Root and adapter package tests pass with `GOWORK=off` and
|
||||
`GOTOOLCHAIN=local`.
|
||||
|
||||
## Stage 4: Resolve the entered command context
|
||||
|
||||
Status: complete.
|
||||
|
||||
1. Add a private core completion request that separates completed words from
|
||||
the current word. Call `resolveEnteredRoute` on completed words only;
|
||||
use the snapshot's canonical metadata, resolved flag-alias context, and
|
||||
discovery context. Keep entered words unchanged. For canonical routes
|
||||
with a runnable operand owner, use the existing canonical prefix length
|
||||
to distinguish its trailing operands/options from command words.
|
||||
2. Suggest canonical child names matching the partial word while the input
|
||||
is still in command-path position. At a node with both children and
|
||||
operands, matching children take precedence; if there are no matching
|
||||
children, use the operand context when available. Check insertion against
|
||||
normal routing: resolve completed entered words plus the candidate and
|
||||
retain it only if it reaches the intended canonical child. Do not treat a
|
||||
partial alias as already entered. After a flag or operand has ended the
|
||||
command path, do not suggest children.
|
||||
3. An alias-only discovery prefix provides no command continuations, but can
|
||||
supply applicable canonical flags using discovery metadata and alias
|
||||
mappings. Unknown routes with no valid context yield no candidates.
|
||||
At root, use the installed visible commands, including built-ins and
|
||||
integrations. Exclude the protocol endpoint everywhere.
|
||||
4. Add context tests for root/group/runnable paths, a partial canonical
|
||||
child, a complete alias and alias chain, an alias-only prefix, a canonical
|
||||
child shadowed in the entered route, and flags beginning before the
|
||||
complete command path. Use existing routing rules as the oracle.
|
||||
|
||||
Completion: added private request context resolution using completed words and
|
||||
the committed routing snapshot. Canonical child candidates are checked by
|
||||
resolving the entered route with the candidate inserted; discovery-only and
|
||||
unknown contexts do not invent command continuations. Tests cover canonical
|
||||
and aliased routes, chains, scoped and discovery flag aliases, shadowed
|
||||
children, runnable operand metadata, and paths ended by flags or operands.
|
||||
The root package suite passes with `GOWORK=off` and `GOTOOLCHAIN=local`.
|
||||
|
||||
## Stage 5: Interpret flags and locate value positions
|
||||
|
||||
Status: complete.
|
||||
|
||||
1. Walk the completed tail using the same canonical and alias spellings and
|
||||
flag token rules as normal parsing. Factor shared token classification
|
||||
from `internal/cobra/flags.go` into neutral helpers where needed; keep
|
||||
backend parsing and failure wording at their current owner. The completion
|
||||
walk records supplied canonical flags, consumed operands, whether options
|
||||
ended, and any pending value flag. It does not run Cobra dispatch,
|
||||
converters, required checks, input factories, or providers.
|
||||
2. A pending value takes precedence even if the current word starts with
|
||||
a dash. For long equals forms, explicit boolean assignments, and short
|
||||
value flags in clusters, separate the value prefix from the entered
|
||||
spelling and separator. Preserve that spelling, including an alias.
|
||||
For a partially typed short cluster still accepting flag members, use its
|
||||
canonical short spellings for continuations; do not rewrite earlier members.
|
||||
A standalone `--` ends flags unless consumed as a value. After it,
|
||||
dash-looking words are operands. An unknown completed option that makes
|
||||
further interpretation ambiguous yields no suggestions.
|
||||
3. For flag-name suggestions, match canonical spellings and suppress already
|
||||
supplied singletons by canonical identity, including occurrences through
|
||||
aliases. Keep repeatable flags and the four reporting flags eligible.
|
||||
When a repeated singleton is typed anyway, still complete its value.
|
||||
4. For an operand, use the same cardinality allocation as `allocateOperands`
|
||||
in `binding.go` without required validation: include the current word
|
||||
as a placeholder, determine which binding receives that final position,
|
||||
and use its hint. Extract the cardinality calculation if necessary to avoid
|
||||
a second allocation policy. Excess operands receive no suggestions.
|
||||
When both operand and flag positions are available, an empty word uses
|
||||
the operand hint and an option-looking partial word requests flags.
|
||||
5. Test pending/attached/empty values, short clusters, scoped aliases and
|
||||
unavailable targets, repeats, booleans, option-looking values, interspersed
|
||||
operands, terminators, and optional/repeated operands before a required
|
||||
suffix. Keep existing normal execution tests green at the sprint boundary.
|
||||
|
||||
Completion: shared neutral token classification and flag lookup with normal
|
||||
parsing, and extracted operand count allocation for execution and completion.
|
||||
The completion walk records consumed flags and operands, locates pending or
|
||||
attached values, preserves entered prefixes, and filters canonical flag names
|
||||
by supplied identity. Tests cover aliases, short clusters, repeated flags,
|
||||
boolean assignments, option-looking values, terminators, interspersed operands,
|
||||
and operand cardinality. Root, adapter, and neutral metadata package tests pass
|
||||
with `GOWORK=off` and `GOTOOLCHAIN=local`.
|
||||
|
||||
## Stage 6: Assemble candidates and deliver query replies
|
||||
|
||||
Status: complete.
|
||||
|
||||
1. Produce a neutral result of candidate replacements, optional descriptions,
|
||||
or a filesystem strategy with value context. Keep prefix preservation
|
||||
distinct from matching; the adapter translates the neutral replacement
|
||||
according to the shell conventions verified in Stage 1. Do not interpret
|
||||
choice values as shell source. Normalize description line breaks and omit
|
||||
candidates the line protocol cannot represent rather than changing what
|
||||
an inserted value means. Do not add execution-time validation for these
|
||||
presentation limitations.
|
||||
2. In `Execute`, after normal receiver/context checks and before ordinary
|
||||
routing, recognize the exact raw query token only on an installed snapshot.
|
||||
Treat all following tokens as data. Split off the final word; a missing
|
||||
current-word argument is a malformed transport request, distinct from an
|
||||
empty string. Use the current snapshot on each query, including after
|
||||
later registration. Do not enter normal parsing, reporting selection,
|
||||
input preparation, or handler execution for this path.
|
||||
3. Write the adapter response only to normalized `Streams.Out`, with
|
||||
managed output delivery in ordinary mode. A valid query with no inferable
|
||||
context returns success and a NoFileComp reply. For a malformed request
|
||||
or query construction failure, return an operational outcome and, if the
|
||||
output is still usable, emit an error directive with no candidates.
|
||||
Explanations go to diagnostics, never into the reply. Preserve write
|
||||
failure evidence through normal presentation bookkeeping; do not retry
|
||||
a broken output stream. No generated script should consume ordinary
|
||||
usage guidance or debug output as a candidate.
|
||||
4. Test query dispatch via `App.Execute`, including explicit empty last word,
|
||||
no-context reply, malformed request, writer failure, hidden discovery,
|
||||
later registration, and target words containing `--quiet` and `--debug`.
|
||||
Use counter fixtures to prove target handlers, converters, factories,
|
||||
and providers are untouched. Verify no special dispatch without installation.
|
||||
|
||||
Completion: connected the installed query before normal routing and reporting
|
||||
selection, assembled neutral candidates and filesystem strategies, and routed
|
||||
protocol output through managed reporting. Invalid transport requests return an
|
||||
error directive and a separate diagnostic; write failures remain presentation
|
||||
evidence. The adapter preserves short attached prefixes through a private cue
|
||||
consumed by the generated scripts, while retaining literal choice values and
|
||||
normalizing descriptions. Query tests cover empty and malformed requests,
|
||||
aliases and discovery, later registration, callback isolation, reporting flags
|
||||
as data, and writer failure. Root and adapter package suites and the full
|
||||
interactive Bash, Zsh, and Fish transport matrix pass.
|
||||
|
||||
## Stage 7: Annotate maintained commands and examples
|
||||
|
||||
Status: complete.
|
||||
|
||||
1. Annotate static format/scope flags where their choices already come from
|
||||
fixed command metadata or literals. Use the same values as the existing
|
||||
version, config, and PromptKit command implementations; omit scope hints
|
||||
that would advertise values not supported by that installed declaration.
|
||||
Mark existing config/overlay filename flags as files where the binding
|
||||
takes a filename. Leave prompt IDs, dynamic selectors, and composite
|
||||
assignments unannotated. Do not infer domain semantics or load providers.
|
||||
2. Add explicit installation to the maintained core and composed examples.
|
||||
Demonstrate a static choice, a path hint, and an already-entered alias
|
||||
with no new runtime dependency. Include an integration-level check
|
||||
that installed hints reach completion without acquisition.
|
||||
3. Update GoDoc, the CLI reference, and consumer/example instructions with
|
||||
actual syntax, basename selection, manual script loading for each shell,
|
||||
and examples of hints. Keep the wire protocol private. Once delivered,
|
||||
update the feature status and keep alias discovery and dynamic discovery
|
||||
in deferred work.
|
||||
|
||||
Completion: annotated built-in version, configuration, and PromptKit formats,
|
||||
supported configuration scopes, and filename selections. The core and composed
|
||||
examples install completion and demonstrate choices, paths, and entered
|
||||
aliases. Integration tests verify scope availability and completion without
|
||||
input, state, acquisition, comparison, engine, or client callbacks. GoDoc, CLI,
|
||||
consumer, example, and roadmap documentation now describe the delivered scope;
|
||||
alias and dynamic discovery remain deferred. Root and both integration package
|
||||
tests pass in the local workspace, and changed Markdown links and whitespace
|
||||
checks pass.
|
||||
|
||||
## Stage 8: Validate the assembled sprint
|
||||
|
||||
Status: complete.
|
||||
|
||||
1. Exercise the generated scripts in Bash, Zsh, and Fish using a built fixture
|
||||
executable, a disposable directory with filenames and directories containing
|
||||
spaces, and isolated shell configuration. Verify actual completion output
|
||||
and replacement behavior. Cover canonical commands, an entered alias,
|
||||
static choices and descriptions, no fallback, file/directory hints in
|
||||
separated and attached values, and the empty operand/leading-dash rule.
|
||||
Use a PTY or a shell's native completion test interface where appropriate.
|
||||
Syntax checking alone is insufficient. Record shell versions and the
|
||||
commands or harness used so the evidence is reproducible.
|
||||
2. Run the root and both optional module suites under the local workspace
|
||||
procedure in [development.md](../development.md), plus the root suite
|
||||
with `GOWORK=off`. Keep shell-dependent checks outside the default Go
|
||||
suite unless the existing environment provides those shells; an absent
|
||||
shell is missing acceptance evidence, not a passing check.
|
||||
3. Verify no backend types in public declarations, no reverse optional-module
|
||||
dependency, and no changes to ordinary routing, flag acceptance, or report
|
||||
behavior outside the selected completion scope. Review documentation links,
|
||||
formatting, and the final diff. Record the actual validation results.
|
||||
|
||||
Completion: the opt-in `TestCompletionConsumerInShells` builds a CommandKit
|
||||
fixture, generates its scripts, and verifies edited invocations and displayed
|
||||
descriptions in interactive Bash 5.3.9, Zsh 5.9, and Fish 4.2.1. The fixture
|
||||
uses isolated shell configuration and paths containing spaces. Run it with
|
||||
Python pexpect, Bash completion, and the three shells available using
|
||||
`COMMANDKIT_COMPLETION_SHELLS=1 GOTOOLCHAIN=local go test -count=1 -run
|
||||
'^TestCompletionConsumerInShells$' .`. Zsh and Fish were extracted to a
|
||||
disposable directory for this run; their executable, library, Zsh module, and
|
||||
function paths were supplied through `PATH`, `LD_LIBRARY_PATH`,
|
||||
`COMPLETION_ZSH_MODULE_PATH`, and `COMPLETION_ZSH_FUNCTION_ROOT`, respectively.
|
||||
The adapter's interactive
|
||||
transport matrix and additional short-equals and Unicode-basename checks also
|
||||
pass in all three shells. The root and both optional module suites pass in the
|
||||
local workspace, as does the root suite with `GOWORK=off`. Public GoDoc and
|
||||
dependency checks show no exposed backend types or reverse optional-module
|
||||
dependency. Existing execution suites, changed documentation links, formatting,
|
||||
and diff whitespace checks pass. The local workspace file is ignored.
|
||||
@@ -1,745 +0,0 @@
|
||||
# Shell completion audit remediation
|
||||
|
||||
Status: all eight shell completion findings addressed. The completed
|
||||
invocation reporting audit is retained below as historical evidence.
|
||||
|
||||
## Completion review scope
|
||||
|
||||
Reviewed `10a799a..0450ecf` against the [completion roadmap](completion.md)
|
||||
and [implementation plan](implementation.md). Core captures static hints,
|
||||
resolves entered routes against the installed snapshot, and chooses candidates
|
||||
without dispatching the target. The internal Cobra adapter generates the scripts
|
||||
and translates replies; optional modules contribute hints through core metadata.
|
||||
The opt-in installation, hidden dispatch, and reporting boundary are in place.
|
||||
The principal gaps are in the transport and its public compatibility effects.
|
||||
|
||||
Evidence includes source review, a temporary public consumer using the maintained
|
||||
fixture's shape, interactive shell reproductions, and a compile probe. No
|
||||
implementation changes were made in this review.
|
||||
|
||||
### 1. Keep shell completion data out of Zsh evaluation
|
||||
|
||||
Priority: P1. Status: complete.
|
||||
|
||||
- [x] Completed: Bash and Zsh pass literal query argv without evaluation; Zsh uses direct argument-safe completion calls. Quoted and escaped Tab-only regressions and the existing Bash/Zsh adapter matrices pass.
|
||||
|
||||
[Zsh script adjustment](../../internal/cobra/completion.go#L146) leaves the
|
||||
generator's `eval _describe ... $flagPrefix` in place. The generator builds
|
||||
`flagPrefix` from a regex match on the current word. Thus a quoted literal value
|
||||
can become executable shell source when the operator presses Tab. In an
|
||||
interactive Zsh session with the generated script, typing
|
||||
`probe run --format '-$(touch AUDIT_TAB_ONLY)=x'` and pressing Tab, **without
|
||||
pressing Enter**, created the sentinel in a disposable directory. The value was
|
||||
being entered for a separate flag, so it did not even have an attached prefix.
|
||||
|
||||
Replace evaluation of word-derived text with argument-safe calls. Pass prefix
|
||||
options as separate arguments and use the actual parsed value context. Review
|
||||
other retained script evaluation paths for the same data/code boundary, including
|
||||
query argv construction; preserve ordinary shell completion semantics without
|
||||
executing literal data. Add an interactive regression that presses Tab without
|
||||
Enter and verifies no side effect, with quoted and escaped input.
|
||||
|
||||
### 2. Use parser-derived replacement context in every shell
|
||||
|
||||
Priority: P2. Status: complete.
|
||||
|
||||
- [x] Completed: All three scripts use the reply’s parsed attachment prefix. Interactive checks cover literal option-looking values, operands after the terminator, and existing attached/static/path cases. The root suite and full adapter shell matrix passed.
|
||||
|
||||
[Core interpretation](../../completion_flags.go) correctly prioritizes a
|
||||
pending value and honors `--`. But [reply encoding and script
|
||||
adjustments](../../internal/cobra/completion.go) omit explicit prefix metadata
|
||||
for equals forms and let the scripts infer an attached flag from the word's
|
||||
shape. That inference cannot distinguish an option from a separate value or
|
||||
operand that happens to contain a dash and equals sign.
|
||||
|
||||
Reproduced with a static choice `--tag=chosen` on the value flag `--format`.
|
||||
For `probe run --format --tag=ch<TAB>`, the hidden query returned the correct
|
||||
literal choice. Bash and Zsh left `--tag=ch` unchanged; Fish inserted
|
||||
`--tag=--tag=chosen`. These are edited invocation results, not merely candidate
|
||||
list differences.
|
||||
|
||||
Carry the parser's replacement context to the shell, including an explicit
|
||||
distinction between whole-word replacement and replacement after an attached
|
||||
prefix. Account for Bash's native word-break handling without treating it as
|
||||
CommandKit grammar. Exercise actual edited argv for separate option-looking
|
||||
values, operands after `--`, and real attached flags, including aliases and path
|
||||
hints. Coordinate with finding 1 so the same context drives safe shell calls.
|
||||
|
||||
### 3. Separate transport control records from literal candidates
|
||||
|
||||
Priority: P2. Status: complete.
|
||||
|
||||
- [x] Completed: Successful replies frame replacement metadata in a dedicated first line. Bash and Zsh no longer interpret ActiveHelp-like candidate text. Interactive checks pass for literal control-like choices, attached choices, and empty replies.
|
||||
|
||||
[CompletionReply](../../internal/cobra/completion.go#L19) emits literal choices
|
||||
in a stream where generated scripts also recognize Cobra's `_activeHelp_ `
|
||||
prefix and CommandKit's private short-attachment marker. Control and candidate
|
||||
records are not distinguished unambiguously. For example, a positional choice
|
||||
`_activeHelp_ choice` appears in the query response, but interactive Bash
|
||||
completion of `probe run _act<TAB>` leaves `_act` unchanged because the script
|
||||
interprets the choice as a control record.
|
||||
|
||||
Disable control features that CommandKit does not use and use unambiguous
|
||||
framing or escaping for necessary metadata. Prefer coordinating this work with
|
||||
finding 2 to avoid another content-dependent prefix heuristic. Preserve
|
||||
representable literal choices rather than reserving additional public value
|
||||
prefixes. Test an edited invocation with marker-like values and ensure a
|
||||
control record cannot appear as a candidate.
|
||||
|
||||
### 4. Preserve comparability of public metadata
|
||||
|
||||
Priority: P2. Status: complete.
|
||||
|
||||
- [x] Completed: Choice hints now point to privately owned copied data. Compile checks cover comparison and map keys for both metadata types; the root suite passes.
|
||||
|
||||
[CompletionHint](../../completion.go#L62) embeds a slice by value. Adding it
|
||||
to [FlagMeta and ArgumentMeta](../../command.go#L93) makes both existing exported
|
||||
types noncomparable, even when an application never opts into completion.
|
||||
A public compile probe now rejects both `map[commandkit.FlagMeta]int{}` and
|
||||
`commandkit.ArgumentMeta{} == commandkit.ArgumentMeta{}`. Before the sprint
|
||||
these structs contained only comparable fields. The chosen metadata field
|
||||
addition does not require this extra compatibility break.
|
||||
|
||||
Keep the hint representation comparable, for example with a private pointer to
|
||||
immutable copied choice data. Preserve copying and the zero hint. Verify that
|
||||
ordinary metadata can again be compared and used as map keys. Do not promise
|
||||
semantic equality between separately constructed nonzero choice hints.
|
||||
|
||||
### 5. Document the Bash initialization prerequisite
|
||||
|
||||
Priority: P3. Status: complete.
|
||||
|
||||
- [x] Completed: CLI setup, examples, and validation guidance now explain Bash completion initialization. The harness accepts a package-specific init path; both required helper functions were verified in clean Bash.
|
||||
|
||||
The [CLI instructions](../cli.md#shell-completion) tell operators to source the
|
||||
generated Bash script without mentioning Bash completion helpers. The
|
||||
[interactive fixture](../../internal/cobra/testdata/completion_terminal.py)
|
||||
sources those helpers first. In an otherwise clean interactive Bash, sourcing
|
||||
only the generated script and completing `probe ru` produced
|
||||
`_get_comp_words_by_ref: command not found` and left the word uncompleted.
|
||||
The path branches also use `_filedir`.
|
||||
|
||||
Document how to initialize the Bash completion package before sourcing the
|
||||
script, and align the maintained examples and validation instructions. There
|
||||
is no need to bundle a replacement Bash completion library.
|
||||
|
||||
## Completion interaction review
|
||||
|
||||
The follow-up reviewed the new completion path against registration snapshots,
|
||||
command and scoped flag aliases, ordinary argument allocation, reporting, and
|
||||
the maintained integration declarations. A local build of the composed example
|
||||
returned `text` and `yaml` through the existing `pk` and `--fmt` aliases,
|
||||
even with `--quiet --debug` in the target words. The equivalent ordinary
|
||||
inspection with `--quiet` stayed silent. ConfigKit completion included
|
||||
`app` and `promptkit` for diff, and additionally `all` for validate, matching
|
||||
the installed capabilities. No new discrepancy was found in these paths.
|
||||
The transport and public metadata findings were open at that review.
|
||||
|
||||
### 6. Supply the existing PromptKit smoke kinds as operand hints
|
||||
|
||||
Priority: P3. Status: complete.
|
||||
|
||||
- [x] Completed: The smoke operand suggests the existing native kinds and all. Composed tests verify canonical and alias hints without loading an engine or calling smoke or inference; the ckprompt suite passes.
|
||||
|
||||
The [smoke command](../../integrations/ckprompt/smoke_command.go#L141) still
|
||||
declares `KIND` without a hint. In the maintained composed consumer, queries
|
||||
after `promptkit test ` and the existing `smoke ` alias return only the no-file
|
||||
directive, including for the partial word `re`. The same consumer successfully
|
||||
runs `smoke reachability --campaign spring --prompt briefing --profile spring`
|
||||
using its local fixture. Operators get static format suggestions on the command
|
||||
but none for its required, documented selector.
|
||||
|
||||
Attach a static operand hint using the existing native kind constants and
|
||||
CommandKit's `all` aggregate selector. Preserve upstream ownership of accepted
|
||||
kinds and existing execution behavior; the hint must not become an allowlist.
|
||||
Cover the canonical command and the maintained alias without starting a smoke
|
||||
test or loading an engine for completion.
|
||||
|
||||
## Completion algorithmic review
|
||||
|
||||
Reviewed the installed metadata, route selection, flag and operand walk,
|
||||
candidate construction, and reply encoding by source inspection. The flag walk
|
||||
advances through the input rather than rescanning earlier tokens. Operand
|
||||
allocation is linear in the number of bindings, and choice filtering and reply
|
||||
encoding are linear in the candidates and text they inspect or emit. Stage 4
|
||||
expressly requires checking each suggested child against ordinary alias routing;
|
||||
this review does not propose removing or replacing that check. No additional
|
||||
correctness defect or newly introduced quadratic algorithm was identified within
|
||||
the remaining scope. The six existing findings were open at that review.
|
||||
|
||||
### 7. Look up canonical completion metadata without scanning siblings
|
||||
|
||||
Priority: P3. Status: complete.
|
||||
|
||||
- [x] Completed: Canonical snapshot nodes now hold their neutral metadata. Completion uses one map lookup per path component; root, nested and alias context checks and a late-registration leaf query pass.
|
||||
|
||||
[completionCommand](../../completion_context.go) searches the metadata child
|
||||
slice at every component of an already resolved canonical path. For depth
|
||||
`D` and sibling counts `B_i` at those levels, the lookup can take
|
||||
`O(sum(B_i))` name comparisons. Even querying a single selected leaf in a wide
|
||||
group can scan every sibling, after routing already found the canonical path.
|
||||
This is distinct from enumerating child suggestions, where visiting children
|
||||
can be necessary.
|
||||
|
||||
Consider indexing neutral metadata by canonical path or associating the
|
||||
metadata with the existing canonical nodes when building the snapshot. A
|
||||
name-indexed walk then takes expected `O(D)` map lookups. Building and storing
|
||||
a separate index would cost `O(N)` entries for `N` installed nodes; prefer
|
||||
reusing existing tree storage where practical. Preserve registration snapshot
|
||||
refresh and the selected candidate insertion checks.
|
||||
|
||||
### 8. Avoid sorting resolved flag aliases on every completion query
|
||||
|
||||
Priority: P3. Status: complete.
|
||||
|
||||
- [x] Completed: Completion builds its lookup records directly from unique resolved aliases in linear time. Existing help ordering is unchanged; scoped and discovery completion checks pass.
|
||||
|
||||
[resolveCompletionContext](../../completion_context.go) calls
|
||||
[neutralRecords](../../flag_aliases.go) for the selected canonical or discovery
|
||||
context. For `A` resolved aliases, this builds and sorts a key slice in
|
||||
`O(A log A)` comparisons, then builds a record slice. The completion flag
|
||||
walker immediately indexes those records by spelling. Source spellings are
|
||||
already unique in this resolved context, so sorted input contributes neither
|
||||
precedence nor candidate ordering to the lookup.
|
||||
|
||||
Build completion's lookup from the resolved mappings in expected `O(A)`
|
||||
time, or reuse a snapshot-owned lookup if that avoids duplicate allocation
|
||||
without complicating installation. Preserve existing sorted records where
|
||||
display order matters and preserve scoped precedence and target availability.
|
||||
This is a source-level opportunity; no latency or allocation threshold is
|
||||
proposed.
|
||||
|
||||
## Shell completion remediation validation
|
||||
|
||||
The root and both optional module suites passed in the local workspace, and
|
||||
the root suite passed with `GOWORK=off`. Both opt-in interactive completion
|
||||
suites passed with Bash 5.3.9, Zsh 5.9, and Fish 4.2.1. They cover literal
|
||||
Tab-only input, actual edited arguments, attached and whole-word completion,
|
||||
path hints, descriptions, and control-like literal choices. The ignored
|
||||
workspace and module dependency versions were preserved.
|
||||
|
||||
---
|
||||
|
||||
## Historical invocation reporting audit remediation
|
||||
|
||||
Status: remediation complete; all 7 findings addressed.
|
||||
|
||||
## Review scope
|
||||
|
||||
Reviewed the reporting implementation from `2a1096b` through `7c035cc` against
|
||||
the [feature roadmap](reporting.md), [implementation plan](https://forge.maximumdirect.net/base-layer/commandkit/src/commit/230187296fae5253cb1773f16c2f1d1a39499ed0/docs/roadmap/implementation.md),
|
||||
[ADR 0014](../adr/0014-invocation-reporting.md),
|
||||
[ADR 0005](../adr/0005-invocation-outcomes.md), and the
|
||||
[architecture policy](../policy/architecture.md).
|
||||
|
||||
The intended ownership is substantially implemented: core owns invocation
|
||||
reporting, ordered presentation selection, coordinated delivery, and finalization;
|
||||
the internal Cobra adapter supplies neutral parsing evidence; optional
|
||||
integrations own native adaptation and command presentation. Providers remain
|
||||
passive, and ckprompt depends on ckconfig without a reverse dependency.
|
||||
|
||||
The findings below concern missing presentation and error evidence within that
|
||||
architecture. They do not require a different public architecture or automatic
|
||||
rendering of application causes. No separate unrequested registration restriction,
|
||||
concurrent-dispatch guarantee, or upstream-domain responsibility was identified.
|
||||
|
||||
Evidence includes source and test review, maintained example invocations, and
|
||||
temporary public-API audit consumers outside the repository. The audit did not
|
||||
modify implementation code or add tests, and did not rerun the full test suites.
|
||||
|
||||
The follow-up interaction audit at `1e532cd` reviewed reporting against existing
|
||||
alias contexts, typed bindings, scope/input/state preparation, cancellation,
|
||||
native reports, and maintained consumers. Representative successful invocations
|
||||
covered command and scoped flag aliases with explicit zero and repeated values,
|
||||
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 were still open at that review and were 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: complete.
|
||||
|
||||
- [x] Completed: bundled native operation failures now supply terminal explanations
|
||||
without printing arbitrary application causes. Explicit provider context and
|
||||
independent warnings retain their presentation paths. Native-error forwarding
|
||||
and single-presentation regressions pass in ordinary and quiet modes.
|
||||
Validation: `GOTOOLCHAIN=local go test ./...` in `integrations/ckprompt`.
|
||||
|
||||
### Finding and evidence
|
||||
|
||||
[WithRequestValidation and FromEngine](../../integrations/ckprompt/adapters.go)
|
||||
carry native operation errors back to their callers, but their acquired terminal
|
||||
explanations contain only configuration-loading diagnostic records.
|
||||
[runRequestValidation](../../integrations/ckprompt/validation_command.go) and
|
||||
[definitionFailure](../../integrations/ckprompt/command.go) then retain the native
|
||||
error solely as `Cause` and present a generic operation heading when no acquired
|
||||
explanation exists. Native request-validation and lookup failures consequently
|
||||
lose their useful explanation at the CLI boundary.
|
||||
|
||||
Reproduced in the maintained composed example:
|
||||
|
||||
```sh
|
||||
go run . promptkit requirements validate briefing --campaign spring
|
||||
```
|
||||
|
||||
Its entire diagnostic is:
|
||||
|
||||
```text
|
||||
ERROR: Unable to validate the PromptKit request for prompt "briefing".
|
||||
```
|
||||
|
||||
A separate public-API consumer using the bundled validation adapter confirmed
|
||||
that the native error contains `missing required input artifact: topic`, while
|
||||
the acquisition has neither an explanation heading nor a detail renderer. The
|
||||
cause survives, but the operator cannot see the missing input. The maintained
|
||||
`promptkit show prompt missing --campaign spring --quiet` invocation likewise
|
||||
omits the native lookup explanation.
|
||||
|
||||
This falls short of useful access to native capabilities and the roadmap's
|
||||
requirement to keep failures understandable. Separating causes from presentation
|
||||
does not prohibit an integration from deliberately adapting native failure
|
||||
content into an explanation. Existing native-failure tests primarily check
|
||||
classification and cause identity while discarding the diagnostic destination,
|
||||
so they do not protect this handoff.
|
||||
|
||||
### Required work
|
||||
|
||||
- At the bundled PromptKit adapter/command boundary, deliberately provide the
|
||||
relevant native operation failure explanation, using the owning API's public
|
||||
presentation and disclosure rules. Cover validation and the other native
|
||||
inspection/selection paths affected by the same migration.
|
||||
- Preserve the original cause independently and preserve an explicitly supplied
|
||||
provider explanation. Keep independent warnings separate and avoid presenting
|
||||
the same native failure through two paths.
|
||||
- Keep core's cause-neutral behavior. Do not solve this by printing arbitrary
|
||||
`Outcome.Cause` values or treating every application-owned loader/provider
|
||||
error as safe operator text.
|
||||
|
||||
### Acceptance evidence
|
||||
|
||||
Use controlled upstream failure evidence to verify that CommandKit forwards
|
||||
useful native failure content into the terminal explanation in ordinary and
|
||||
quiet modes, with cause identity retained and unrelated warnings still hidden
|
||||
where selected. Check explicit provider explanations for single presentation.
|
||||
These are adapter/presentation assertions, not tests of whether PromptKit
|
||||
correctly detects missing artifacts or invalid selections.
|
||||
|
||||
## 2. Retain independent rendering errors joined with write errors
|
||||
|
||||
Priority: P2. Status: complete.
|
||||
|
||||
- [x] Completed: joined callback errors retain independent rendering evidence,
|
||||
while plain and wrapped sink errors remain write failures. Output and detail
|
||||
regressions pass, including joined destination errors.
|
||||
Validation: `GOTOOLCHAIN=local go test ./...` in the repository root.
|
||||
|
||||
### Finding and evidence
|
||||
|
||||
In [reporter.go](../../reporter.go), `recordRendererErrorLocked` drops the entire
|
||||
callback error whenever `errors.Is(renderErr, writeErr)` is true. That test also
|
||||
matches `errors.Join(independentRenderErr, writeErr)`: the callback error can
|
||||
contain an independent rendering failure as well as the propagated sink failure.
|
||||
|
||||
The shared helper serves `RenderOutput`, message details, and terminal details.
|
||||
Dropping the whole joined error loses evidence both from the immediate returned
|
||||
error and from `Presentation.*.RenderError`.
|
||||
|
||||
A public-API audit consumer used a failing result writer and this callback:
|
||||
|
||||
```go
|
||||
_, writeErr := io.WriteString(w, "result")
|
||||
return errors.Join(renderErr, writeErr)
|
||||
```
|
||||
|
||||
The invocation failed as expected and retained the sink error, but
|
||||
`errors.Is(returnedError, renderErr)` was false and the final output `RenderError`
|
||||
was nil. This contradicts the selected retention of independently returned
|
||||
rendering failures. The current separation test covers returning a standalone
|
||||
rendering error after a failed write, but not a callback that already joins them.
|
||||
|
||||
### Required work
|
||||
|
||||
- Distinguish a callback merely propagating its managed-writer failure from a
|
||||
callback carrying additional independent error evidence. Do not discard the
|
||||
entire returned error just because its chain also contains the sink error.
|
||||
- Preserve the independent callback error in the immediate error result and
|
||||
bounded rendering observations. Retain the first write error separately and
|
||||
continue avoiding further writes to the failed destination.
|
||||
- Keep ordinary propagation of a write error from being misclassified as an
|
||||
independently discovered rendering failure. No transcript or new public
|
||||
error-classification API is needed.
|
||||
|
||||
### Acceptance evidence
|
||||
|
||||
Add a focused joined-error regression at the shared reporting boundary. Verify
|
||||
`errors.Is` access to both failures in the returned error, rendering evidence in
|
||||
`RenderError`, and sink evidence in `WriteError`. Preserve existing coverage for
|
||||
plain/wrapped write-error propagation and first-error retention. Exercise a
|
||||
detail-rendering path as needed to establish that the shared fix applies there.
|
||||
|
||||
## 3. Explain failures introduced by CommandKit presentation
|
||||
|
||||
Priority: P2. Status: complete.
|
||||
|
||||
- [x] Completed: presentation failures that turn success into failure now retain
|
||||
a core-owned rendering/delivery explanation. Supplied explanations and causes,
|
||||
generic handler fallback, quiet rendering, domain status, and failed-sink
|
||||
behavior remain covered. Validation: `GOTOOLCHAIN=local go test ./...` at root.
|
||||
|
||||
### Finding and evidence
|
||||
|
||||
[Reporter.finalize](../../reporter.go) changes an otherwise successful outcome
|
||||
to failure when presentation failed, then invokes the same
|
||||
`ensureFallbackExplanation` used for an unexplained failed handler outcome.
|
||||
Consequently, a CommandKit-invoked renderer failure or a result-write failure
|
||||
produces only:
|
||||
|
||||
```text
|
||||
ERROR: Command failed.
|
||||
```
|
||||
|
||||
The diagnostic destination may be entirely healthy. The caller can inspect
|
||||
`Presentation`, but the operator is not told that rendering or delivery failed.
|
||||
This also affects successful domain outcomes changed to non-zero status.
|
||||
|
||||
Public-API consumers reproduced the generic explanation after both a callback
|
||||
that failed before writing and a rejected result write. The corresponding
|
||||
reporter and execution tests currently assert this generic text, so they preserve
|
||||
the gap rather than the selected distinction.
|
||||
|
||||
[Stage 4](https://forge.maximumdirect.net/base-layer/commandkit/src/commit/230187296fae5253cb1773f16c2f1d1a39499ed0/docs/roadmap/implementation.md#stage-4-implement-deferred-delivery-and-finalization)
|
||||
requires a useful core-owned explanation when presentation turns success into
|
||||
failure. The roadmap assigns handling of reporting failures to CommandKit and
|
||||
distinguishes healthy diagnostic destinations from failed result destinations.
|
||||
The generic fallback remains appropriate for a failed handler outcome that
|
||||
supplies no explanation; it does not explain the presentation failure core
|
||||
itself discovered.
|
||||
|
||||
### Required work
|
||||
|
||||
- When presentation changes a successful outcome into failure and no explanation
|
||||
was supplied, attach and retain an explanation identifying the failed
|
||||
presentation operation, such as result rendering or result delivery. Present
|
||||
it through a healthy diagnostic destination.
|
||||
- Keep the original optional cause unchanged and preserve supplied explanations.
|
||||
An informative core-owned heading is sufficient; this does not require
|
||||
automatically exposing callback error strings or application causes.
|
||||
- Preserve the selected outcome/status table, exact generic fallback for
|
||||
otherwise unexplained failed outcomes, and explicit native explanations,
|
||||
including a non-nil renderer that emits no bytes.
|
||||
- Do not retry a failed diagnostic destination, redirect into substantive output,
|
||||
replay explanation callbacks, or introduce recursive reporting-failure messages.
|
||||
|
||||
### Acceptance evidence
|
||||
|
||||
Update focused tests so a successful operation with an output-rendering or
|
||||
output-writing failure receives a retained presentation-specific explanation
|
||||
through working diagnostics, including quiet rendering failure and a successful
|
||||
domain outcome. Preserve evidence for supplied explanations/causes, generic
|
||||
fallback on unexplained handler failure, and diagnostic-sink failure without
|
||||
retry. Assert the meaningful failure category rather than inventing a new
|
||||
public exact-wording guarantee.
|
||||
|
||||
## 4. Restore cancellation checks between report preparation and delivery
|
||||
|
||||
Priority: P2. Status: complete.
|
||||
|
||||
- [x] Completed: restored preparation-before-delivery cancellation checks for
|
||||
PromptKit catalogs, definitions, requirements, and validation, and buffered
|
||||
configuration diff/sources/YAML explanation reports. Cancellation retains its
|
||||
operation cause without becoming a rendering failure; native streaming and
|
||||
smoke-report behavior remain unchanged. Controlled preparation/serialization
|
||||
checks cover ordinary and quiet modes. Validation: module suites pass in
|
||||
`integrations/ckconfig` and `integrations/ckprompt`.
|
||||
|
||||
### Finding and evidence
|
||||
|
||||
Before the reporting migration, PromptKit's `writePromptReport` checked the
|
||||
invocation context after a report had been prepared and before writing it.
|
||||
The migrated [renderProfileReport](../../integrations/ckprompt/command.go)
|
||||
formats its text report inside `RenderOutput`, writes it immediately, and
|
||||
returns success without that intervening cancellation check. The surrounding
|
||||
inspection command checks cancellation only before entering report preparation.
|
||||
|
||||
A deterministic public-API consumer supplied a profile inspection with a
|
||||
controlled YAML marshaler in `EffectiveModelParams.ExtraParams`. The marshaler
|
||||
cancelled the invocation context and returned a valid value while the text
|
||||
report was being prepared. This uses the ordinary report serialization boundary;
|
||||
it does not reenter the reporter or require concurrency or an external service.
|
||||
|
||||
Observed results for `promptkit show profile demo --format text`:
|
||||
|
||||
| Mode | Context at completion | Status | Cause matches cancellation | Report bytes |
|
||||
| --- | --- | --- | --- | --- |
|
||||
| Ordinary | `context.Canceled` | `0` | No | `277` |
|
||||
| Quiet | `context.Canceled` | `0` | No | `0` |
|
||||
|
||||
Cancellation occurred before the first destination write. Previously, the shared
|
||||
post-preparation check would have returned operational failure with the
|
||||
cancellation cause and left the report destination untouched. The migration also
|
||||
removed similar post-preparation checks from buffered configuration report paths,
|
||||
including [config diff](../../integrations/ckconfig/diff.go). Its current
|
||||
post-render cancellation branch runs only when rendering already returned an
|
||||
error, so successful preparation does not preserve the old checkpoint.
|
||||
|
||||
The plan requires preserving existing cancellation behavior outside the selected
|
||||
reporting changes. Quiet suppression is not intended to turn this cancellation
|
||||
into success. Existing cancellation tests cover acquisition and write failures,
|
||||
but do not cover cancellation during successful report preparation.
|
||||
|
||||
### Required work
|
||||
|
||||
- Restore the integration-owned cancellation checkpoint after report preparation
|
||||
and before delivery in migrated paths that previously had it. Review the
|
||||
removed `writePromptReport` callers and the buffered configuration paths rather
|
||||
than patching only the demonstrated profile-text case.
|
||||
- Preserve supported cancellation identity in the operation outcome, while
|
||||
retaining reporter ownership of rendering and destination failures. Apply the
|
||||
same operation decision under quiet mode.
|
||||
- Keep this change bounded to the existing preparation/delivery behavior. Do not
|
||||
introduce a core-wide rule that rejects every cancelled application invocation,
|
||||
interrupt caller writers, withdraw delivered bytes, or promise cancellation
|
||||
after delivery has already begun. New whole-run buffering is unnecessary.
|
||||
|
||||
### Acceptance evidence
|
||||
|
||||
Use a controlled serialization/preparation callback to cancel before the first
|
||||
report write. Assert operational failure, `errors.Is` access to
|
||||
`context.Canceled`, and no report delivery, including quiet mode. This verifies
|
||||
CommandKit coordination, not upstream serialization or domain behavior. Preserve
|
||||
existing coverage for cancellation during acquisition, partial output, and
|
||||
caller-owned writes that have already started.
|
||||
|
||||
## 5. Apply reporting-flag aliases in unresolved-route parse contexts
|
||||
|
||||
Priority: P2. Status: complete.
|
||||
|
||||
- [x] Completed: unresolved routes obtain aliases from the context selected by
|
||||
normal parsing, through neutral internal metadata. Root and nested-group
|
||||
canonical/alias comparisons, quiet failure visibility, scoped precedence,
|
||||
unchanged unresolved identity, and factory non-execution pass.
|
||||
Validation: `GOTOOLCHAIN=local go test ./...` at the repository root.
|
||||
|
||||
### Finding and evidence
|
||||
|
||||
[compileFlagAliases](../../flag_aliases.go) now resolves application-wide aliases
|
||||
to the new reporting flags in root and group contexts. However,
|
||||
[execution.prepareSession](../../execute.go) supplies aliases to the parser only
|
||||
for `routeCanonical` and `routeDiscovery`. For an unresolved command path it
|
||||
supplies none, even when the normal parser selects the root or a known group
|
||||
whose reporting flags and alias mappings are available.
|
||||
|
||||
With an installed application-wide `FlagAlias("-d", "--debug")`, a public-API
|
||||
consumer reproduced:
|
||||
|
||||
| Invocation | Observed diagnostics |
|
||||
| --- | --- |
|
||||
| `app missing --debug` | Debug route/selection detail, then the unknown-command usage failure. |
|
||||
| `app missing -d` | `unknown shorthand flag: 'd' in -d`, with no Debug detail. |
|
||||
| `app group missing --debug` | Debug detail, then the group operand/command usage failure. |
|
||||
| `app group missing -d` | `unknown shorthand flag: 'd' in -d`, with no Debug detail. |
|
||||
| `app group run -d` | Successful command execution with Debug detail. |
|
||||
|
||||
The spelling registered for a reporting flag is thus treated as unknown on the
|
||||
failure paths where operators may need debugging. Canonical mode selection on
|
||||
these paths already works. This conflicts with the
|
||||
[CLI alias contract](../cli.md#application-owned-aliases): an application-wide
|
||||
alias is effective wherever its canonical target is available. The new reporting
|
||||
flags make these existing parse-context gaps observable for mode selection.
|
||||
|
||||
Current reporting parse tests cover a canonical reporting flag after a missing
|
||||
command and aliases on valid routes, but not their combination.
|
||||
|
||||
### Required work
|
||||
|
||||
- Supply the alias mapping applicable to the context used by normal parsing
|
||||
when the full entered command path does not resolve. Cover root and known
|
||||
group parse contexts, preserving existing scope/precedence rules where they
|
||||
apply.
|
||||
- Keep ordered canonical/alias occurrence handling in the normal parser. Do not
|
||||
add a separate reporting-flag scanner or enable unrelated aliases whose
|
||||
canonical target is unavailable.
|
||||
- Preserve the original routing failure, handler/factory non-execution, and
|
||||
entered-path usage behavior. Finding a parse context must not manufacture a
|
||||
successfully resolved canonical command identity for the invalid route.
|
||||
|
||||
### Acceptance evidence
|
||||
|
||||
Compare canonical and application-wide aliased reporting flags on unresolved
|
||||
root and nested-group paths. They should select the same mode and retain the
|
||||
same underlying command-path failure rather than invent an unknown-flag error.
|
||||
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: complete.
|
||||
|
||||
- [x] Completed: renderer-free hidden deferred messages record suppression
|
||||
without queue retention. Finalization detaches the queue without an array
|
||||
copy and delivers captured callback lists without recopying them. Hidden rich
|
||||
callbacks, FIFO order, no-content handling, capture, and closure remain covered.
|
||||
Validation: root module suite and focused reporter race checks pass.
|
||||
|
||||
### 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: complete.
|
||||
|
||||
- [x] Completed: interpretation returns the first fatal unknown-flag prefix,
|
||||
retaining earlier canonical/alias occurrences and short-cluster members.
|
||||
Temporary slices grow with that prefix; unreachable values and spellings are
|
||||
no longer normalized or copied. Existing error precedence, values, and
|
||||
terminator coverage passes. Validation: root module suite passes.
|
||||
|
||||
### 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.
|
||||
|
||||
## Final remediation validation
|
||||
|
||||
All seven items have completion markers and focused regression evidence. The
|
||||
following checks pass for the assembled changes:
|
||||
|
||||
- `GOTOOLCHAIN=local go test ./...` at the repository root and in each of
|
||||
`integrations/ckconfig` and `integrations/ckprompt`, including maintained examples.
|
||||
- `GOWORK=off GOTOOLCHAIN=local go test ./...` at the repository root.
|
||||
- `GOTOOLCHAIN=local go test -race -run '^TestReporter|^TestHidden' .` at root.
|
||||
- Go formatting, staged/unstaged whitespace review, and local document-link review.
|
||||
|
||||
The ignored local workspace is preserved. No dependency upgrade, release,
|
||||
publication, or change to upstream domain behavior was part of this work.
|
||||
Reference in New Issue
Block a user