229 lines
13 KiB
Markdown
229 lines
13 KiB
Markdown
# 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–F15, 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? | Callers compared; recommendation pending agreement |
|
||
| 4 | F14–F15 | How should both operations reuse source-prefix processing? | Joint design pending; remediation selected |
|
||
| 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:** recommendation pending agreement; 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.
|
||
|
||
**Preliminary recommendation:** prefer reuse; preserve classification,
|
||
authoritative resolution, deduplication, and ordering. Select the concrete
|
||
traversal and ownership together. No performance test is required to justify
|
||
remediation; correctness validation remains appropriate.
|
||
|
||
**Decision:** joint remediation selected in the catalog; concrete design pending.
|
||
|
||
## 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.
|