Files
commandkit/docs/roadmap/audit/remediation-analysis.md

266 lines
15 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Audit remediation analysis
Status: discussion in progress; no implementation changes made.
This document owns analysis of unresolved remediation choices, in the discussion
order below. The [catalog](catalog.md) owns the findings, accepted remediation
decisions, and their status. Original audits retain detailed evidence. Record
accepted conclusions in the catalog and mark the corresponding discussion here
decided; a decision does not close a finding before implementation.
Existing catalog decisions for F03, F10, F11, F13–F16, and F21 remain selected. In
particular, performance testing is not a prerequisite for F13–F15, and F14 and
F15 are to be designed together. CommandKit is pre-release with no consumers;
consumer compatibility and migration are not constraints on these choices.
Straightforward correctness repairs need not wait for this discussion sequence.
| Order | Findings | Question | Discussion status |
| --- | --- | --- | --- |
| 1 | F10 | Which flag declaration rules should be documented as the intended contract? | Decided: retain and document existing rules; remediation pending |
| 2 | F21 | What narrow diagnostic adaptation API would simplify both integrations? | Decided: shared public ckconfig helper; remediation pending |
| 3 | F16 | Which flag-consumption facts can parsing and completion usefully share? | Decided: shared internal mechanics, separate caller policies; remediation pending |
| 4 | F14–F15 | How should both operations reuse source-prefix processing? | Existing tree inspected; reuse recommendation pending agreement |
| 5 | F17–F20 | Which private extractions materially simplify the existing code? | Individual extraction review pending |
## F10 — Flag declaration naming
**Question:** should remediation document the existing naming rules, or change
them first? This concerns names declared by applications, not the contents of
operator-supplied flag values.
**Evidence:** [catalog F10](catalog.md#f10--p3-public-flag-metadata-omits-enforced-naming-and-reserved-spelling-rules)
and [documentation audit D2](13-documentation.md#d2--p3-public-flag-metadata-omits-enforced-declaration-constraints).
`FlagMeta`, `validateBindings`, and `validFlagName` in
[command.go](../../../command.go) establish the documentation gap and current
validation. Shared reporting spellings are defined in
[commandmeta](../../../internal/commandmeta/metadata.go).
The historical accepted grammar ADR explicitly selected lowercase names with
hyphen-separated words. It is available at commit
`82128463cd9649a426da7c5e3f3ce33bd37cdd9f`, path
`docs/adr/0006-command-grammar.md`; the same commit's
`docs/roadmap/initial.md` carried that direction into implementation. This
supports the broad naming convention, but does not independently establish every
character-level detail of the current validator. Historical records provide
context; they do not replace the current canonical API documentation.
Current long names consist of nonempty groups of Unicode lowercase letters or
digits separated by single hyphens. Names may start with digits or consist
entirely of digits. Uppercase letters, underscores, dots, and leading, trailing,
or repeated hyphens are rejected. Examples:
| Declaration name | Current result |
| --- | --- |
| `max-tokens`, `profile2`, `2fa`, `123`, `équipe` | Accepted, subject to ordinary collision checks |
| `maxTokens`, `max_tokens`, `max.tokens`, `max--tokens`, `-profile`, `profile-` | Rejected |
Core reserves `help`, `quiet`, `verbose`, `debug`, `progress`, and `format` for
its own flags. A short spelling is optional; when present it is one byte,
cannot be `-`, and cannot be the reserved help spelling `h`. The long-name
lowercase rule does not apply to short spellings. Declaration defects are
reported at installation, before command execution.
**Options:**
1. Retain and document the current rules. This makes the shared convention
explicit without introducing new validation or conversion machinery.
2. Relax the stylistic restrictions, allowing application names such as
`maxTokens` or `max_tokens` while preserving core reservations and necessary
parsing constraints. This gives applications more naming freedom but weakens
the common vocabulary; its precise supported grammar would need selection.
3. Tighten the rules, for example to ASCII names beginning with a letter. This
creates a narrower convention but removes currently accepted names without
an identified operator benefit.
**Recommendation:** option 1. Retain lowercase, hyphen-separated long names,
including current Unicode lowercase and digit support. Keep core reservations
and the existing short-spelling rules. Recommend readable names without banning
numeric names or adding an ASCII-only restriction. Do not automatically rewrite
invalid names or accept alternate spellings through normalization.
Document the exact declaration contract once in `FlagMeta` GoDoc and link related
helpers, including `RenameFlag` and flag-alias declarations, as applicable. Keep
operator syntax in the CLI reference. This is a documentation remediation;
it does not require a new ADR, parser, or validation framework.
**Decision:** option 1 accepted and recorded in the catalog. Retain the current
long-name grammar, Unicode lowercase and digit support, reservations, and
short-spelling rules. No runtime validation change or name normalization is
selected. F10 remains open until its public documentation is corrected.
## F21 — Shared ConfigKit diagnostic adaptation
**Question:** what concrete helper, if any, removes duplicated CLI adaptation
without introducing a broader diagnostics framework?
**Evidence:** [catalog F21](catalog.md#f21--p3-both-integrations-maintain-the-same-configkit-diagnostic-adaptation)
links both adapters and the original analysis.
**Options to examine:** a narrow public ckconfig helper returning existing core
diagnostics and an explanation, or retaining private implementations if the
shared API adds more complexity than it removes.
**Analysis:** ckconfig's private `configKitDiagnosticsWithHeading` already
accepts a heading, native sources, and native diagnostic records, and returns
core diagnostics and an explanation. ckprompt duplicates the evidence snapshot,
severity partition, source legend, and delegation to `configkit.WriteDiagnostics`.
Its scope and comparison-side labels can be supplied as the heading; they do not
require another shared options type. See
[ckconfig's adapter](../../../integrations/ckconfig/configkit_adapter.go) and
[ckprompt's adapter](../../../integrations/ckprompt/adapters.go).
**Recommendation:** expose the existing adaptation responsibility through one
narrow ckconfig function and have both integrations use it. An illustrative
signature, not an implemented API, is:
```go
func AdaptDiagnostics(
heading string,
sources []configkit.SourceInfo,
records []configkit.Diagnostic,
) ([]commandkit.Diagnostic, commandkit.Explanation)
```
Retain the existing evidence snapshot and severity treatment, ordering within
each group, source labels, and native record formatting. Return evidence ready
for CLI reporting; do not print immediately, select an invocation outcome, or
load or validate configuration. Callers supply headings and operation-specific
explanations and retain their error causes. Reporter remains responsible for
visibility and delivery.
Keep scheduling with the callers. The smaller scheduling switches do not
independently justify another public helper. ckprompt already depends on
ckconfig; this introduces no new module dependency or ConfigKit dependency in
core. The public function serves CLI presentation, within the integration's
ownership boundary. Compatibility and migration are not obstacles; clarity and
maintenance cost are the decision criteria.
**Decision:** shared public ckconfig adaptation helper accepted and recorded in
the catalog, using the input/output shape above. Both integrations will use it;
the smaller scheduling switches remain with their callers. The function name
above is illustrative; implementation remains pending.
## F16 — Shared flag consumption
**Question:** which consumption facts can be shared while execution and
completion retain their different handling of incomplete input?
**Evidence:** [catalog F16](catalog.md#f16--p3-parsing-and-completion-duplicate-flag-consumption-grammar).
**Options to examine:** a small neutral consumption primitive or deliberate
retention of duplication where a shared helper needs extensive caller modes.
**Analysis:** execution's `interpretFlagTokens` in
[internal/cobra/flags.go](../../../internal/cobra/flags.go) and completion's
`walkCompletionFlags` and `completionOptionTarget` in
[completion_flags.go](../../../completion_flags.go) repeat the decisions about
short-flag clusters, attached values, boolean assignments, and whether a flag
consumes the next argument. They already share classification and lookup through
[commandmeta's flag helpers](../../../internal/commandmeta/flags.go).
Both paths need to recognize that `--profile=team` has an attached value and
`--profile team` takes the following argument. Their responses to unfinished
input differ: executing a bare `--profile` requires a missing-value error, while
completion after `--profile ` should offer values. Likewise, an unfinished
`--pro` can be useful for suggesting flag names without being a valid execution
argument. A single policy-heavy parser would obscure that difference.
**Recommendation:** extend the existing neutral internal helpers with a small
consumption operation. Given flag metadata and the current spelling/position,
it should expose facts such as the identified flag, any attached value, whether
the next argument is required, and where to continue within a short cluster.
Distinguish an explicitly empty attached value from an absent value. The exact
private signature is an implementation choice, not a new public contract.
Execution retains alias normalization, entered-spelling and occurrence tracking,
backend error handling, and typed conversion. Completion retains unfinished-word
handling, suggestion filtering, and insertion text. Existing token classification
continues to identify `--`; callers retain their appropriate end-of-options
handling. Supply each caller's existing lookup rather than extending completion
to new alias-discovery behavior. Do not add execution/completion mode flags,
callback registries, another public parser, or a replacement for Cobra/pflag.
Repair F06's fixture before validating the change. Use focused behavioral
coverage for shared grammar and the deliberately different missing/partial-value
paths, preserving explicit empties, short clusters, and entered spelling.
If a proposed extraction needs extensive caller-specific controls, narrow it
and retain the remaining duplication rather than forcing complete unification.
**Decision:** the recommended internal sharing boundary is accepted and recorded
in the catalog. Execution and completion retain their separate policies; exact
private signatures remain implementation choices. No refactor is implemented.
## F14–F15 — Joint source-prefix design
**Question:** how can discovery preparation and help lookup reuse one source
structure without parallel indexes or changed routing semantics?
**Evidence:** [catalog F14](catalog.md#f14--p3-discovery-context-preparation-repeatedly-rebuilds-shared-prefixes)
and [F15](catalog.md#f15--p3-alias-help-scans-the-entire-source-catalog-for-each-intermediate-route).
**Options to examine:** traversal and descendant lookup through the existing
alias source structure, or one shared private index if the existing structure
cannot serve both operations simply.
**Analysis:** the existing [alias resolver](../../../internal/alias/resolver.go)
already builds a source-path tree. Each node represents one distinct prefix,
has child links, and identifies whether a mapping ends there. Shared source
prefixes therefore already have a single structural representation.
F14's `discoverFlagContexts` in [flag_aliases.go](../../../flag_aliases.go)
instead walks each declaration independently, repeatedly copying and classifying
common prefixes before deduplicating accepted results. F15's `routeHelpAliases`
in [help_view.go](../../../help_view.go) scans the full alias catalog at every
intermediate rewrite path, even when that prefix has no descendants.
For example, sources `legacy team show` and `legacy team check` share `legacy`
and `legacy team`. Setup can examine each shared prefix once. Help at
`legacy team` can find its descendants through that tree branch without scanning
unrelated source paths.
**Recommendation:** add narrow internal read-only operations on the existing
resolver tree for these two needs:
1. Enumerate each distinct nonempty proper source prefix once for F14. Apply
the existing discovery classification once per candidate, retaining exclusions
for canonical paths, exact alias sources, and valid canonical operand space.
2. Find strictly descendant alias sources beneath a supplied prefix for F15.
Continue visiting the existing intermediate resolution paths. Resolve each
composed entered path through the authoritative resolver, then retain current
canonical-target checks, suffix deduplication, and output sorting.
Keep trie nodes private and preserve captured path ownership. Reuse the tree's
existing routing-snapshot lifetime rather than creating a parallel index or a
cross-invocation result cache. Root help can continue using the complete catalog
when it actually needs all entries. No public API or routing grammar change is
needed; internal method names and return shapes remain implementation choices.
This removes the identified repeated scans and preparation, without claiming
that all routing or installation costs become linear. Test the preserved
discovery/help behavior, including shared prefixes, chains, longer overrides,
and updated registration where relevant. As already decided, performance testing
is not a prerequisite for remediation.
**Decision:** joint remediation is selected in the catalog; reuse of the existing
tree through these internal operations is recommended, pending agreement.
## F17–F20 — Proportionate private extractions
**Question:** which proposed extractions make responsibilities clearer without
adding records, phases, or configurable mechanisms merely to shorten functions?
**Evidence:** catalog entries [F17](catalog.md#f17--p3-configuration-scope-capture-combines-several-compilation-phases),
[F18](catalog.md#f18--p3-configuration-preflight-interleaves-shared-and-operation-specific-rules),
[F19](catalog.md#f19--p3-typed-input-preparation-mixes-conversion-provenance-and-replay-assembly),
and [F20](catalog.md#f20--p3-reporter-message-paths-duplicate-detail-rendering-bookkeeping).
**Options to examine:** cohesive functions over existing private types, or
retaining related code together where extraction only adds indirection.
**Preliminary recommendation:** review each extraction separately. F20 has a
concrete shared rendering/error policy; keep its refactor distinct from F03's
correctness repair. For F17–F19, make meaningful responsibilities and required
ordering visible without treating incidental private sequences as contracts.
**Decision:** individual extraction choices pending review.