Record requirements inspection review findings
This commit is contained in:
138
docs/roadmap/remediation.md
Normal file
138
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,138 @@
|
||||
# PromptKit requirements inspection review
|
||||
|
||||
Status: three open findings. This document records required follow-up work;
|
||||
the review does not change implementation or current contracts.
|
||||
|
||||
## Review scope and authority
|
||||
|
||||
Reviewed the requirements-inspection implementation from planning baseline
|
||||
[`3935dcd`](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/3935dcd)
|
||||
through implemented state
|
||||
[`cfc2d1d`](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/cfc2d1dfd0c17c35ffe0d0a1a46f4dc2df8be5bb).
|
||||
The review compared production changes, tests, examples, and documentation with
|
||||
the accepted [feature roadmap](requirements.md), completed
|
||||
[implementation plan](implementation.md), and applicable
|
||||
[architecture](../policy/architecture.md),
|
||||
[documentation](../policy/documentation.md), and
|
||||
[testing](../policy/testing.md) policies.
|
||||
|
||||
The targeted runtime architecture is present: the optional ckprompt suite owns
|
||||
requirements registration, selection, coordination, and YAML presentation;
|
||||
its engine adapter delegates discovery to PromptKit's public API. Core and
|
||||
ckconfig retain their existing responsibilities and dependency boundaries.
|
||||
No production defect or additional domain restriction was identified in this
|
||||
review. The findings concern canonical documentation and one test that constrains
|
||||
presentation beyond the selected contract.
|
||||
|
||||
## R-1: Correct singleton wording for requirements flags
|
||||
|
||||
Status: open. Priority: normal. Category: documentation contract.
|
||||
|
||||
**Evidence:** [CLI reference](../cli.md), lines 418–420, says that the built-in
|
||||
flags are singleton bindings and that the last value is used when repeated.
|
||||
The same section lists `--var` as repeatable. In
|
||||
[command.go](../../integrations/ckprompt/command.go), `newRequirementsCommand`
|
||||
uses `RepeatFlag` for variables and builds the map from every occurrence, with
|
||||
last-value replacement only for repeated keys.
|
||||
|
||||
The unqualified singleton statement contradicts the implemented and accepted
|
||||
behavior. For example, `--var audience=internal --var mode=full` retains both
|
||||
keys; the second occurrence does not replace the entire variable context.
|
||||
|
||||
**Required work:** qualify the singleton description to exclude `--var`, or
|
||||
identify the singleton flags explicitly. Retain the existing explanation that
|
||||
variable occurrences accumulate and only duplicate keys use the last value.
|
||||
Suite selectors continue to follow their declared bindings. No parser or handler
|
||||
change is required.
|
||||
|
||||
**Completion evidence:** the CLI reference gives consistent repetition rules in
|
||||
its flag table and prose. Check the wording against the existing assignment and
|
||||
singleton coverage; new generic parser tests are unnecessary.
|
||||
|
||||
## R-2: Complete the canonical requirements YAML schema
|
||||
|
||||
Status: open. Priority: normal. Category: incomplete documentation delivery.
|
||||
|
||||
**Evidence:** [CLI reference](../cli.md), lines 428–451, describes the report
|
||||
shape but illustrates `variables: []` without defining variable-entry fields
|
||||
anywhere in the requirements section. The implemented
|
||||
`requirementVariableReport` in
|
||||
[command.go](../../integrations/ckprompt/command.go) emits `name`, `kind`,
|
||||
`supplied`, and `explanation`. Those fields were selected by the feature roadmap
|
||||
and are covered by `TestRequirementsReportPreservesNativeFieldsAndMetadata` in
|
||||
[command_test.go](../../integrations/ckprompt/command_test.go).
|
||||
|
||||
Operators and automated consumers cannot obtain the complete CommandKit YAML
|
||||
schema from its canonical owner. PromptKit's Go API defines native meanings,
|
||||
but it does not define CommandKit's YAML field names. The missing fields should
|
||||
remain documented after temporary planning records are retired.
|
||||
|
||||
**Required work:** add a representative variable entry to the CLI example or
|
||||
provide a compact field inventory for the full report. Define the existing
|
||||
`supplied` field as native exact-key presence, including an explicitly empty
|
||||
value, independently of `kind` and `complete`. Keep explanations as upstream
|
||||
human-readable evidence rather than text to parse.
|
||||
|
||||
Use a coherent partial-discovery example if retaining the illustrated
|
||||
`missing_variable` issue for `mode`: include its variable entry as native
|
||||
inspection does. Preserve all already selected fields, false values, empty
|
||||
sequences, and nullable metadata semantics. Do not change the report or add
|
||||
new disclosure, completeness, or readiness guarantees.
|
||||
|
||||
**Completion evidence:** all selected report fields are documented in the CLI
|
||||
owner, and the example matches the existing native/consumer evidence. Review
|
||||
local links and document whitespace. Production serialization and test fixtures
|
||||
need no changes to repair this omission.
|
||||
|
||||
## R-3: Remove an incidental YAML key-order requirement from a test
|
||||
|
||||
Status: open. Priority: low. Category: inadvertently strengthened test contract.
|
||||
|
||||
**Evidence:** `TestRequirementsCommandCapturesContextAndVariableAssignments` in
|
||||
[command_test.go](../../integrations/ckprompt/command_test.go), lines 190–192,
|
||||
uses `strings.HasPrefix(output.String(), "prompt_id:")` to establish that an
|
||||
omitted format defaults to YAML.
|
||||
|
||||
That check also requires `prompt_id` to be the first serialized mapping key and
|
||||
excludes an otherwise valid YAML document-start marker. Neither presentation
|
||||
detail was selected by the user. A semantic-equivalent report would fail this
|
||||
test, contrary to the implementation plan's instruction to avoid incidental
|
||||
key-order assertions and the testing policy's behavior-focused approach.
|
||||
|
||||
**Required work:** replace the prefix assertion with YAML decoding of the second
|
||||
invocation's output and an assertion on the relevant semantic fields, such as
|
||||
the selected prompt identity. Retain the existing success, fresh input/state,
|
||||
and empty discovery-context assertions. Do not add a new key-order guarantee to
|
||||
the CLI documentation to justify the current test.
|
||||
|
||||
**Completion evidence:** the test proves default YAML output without constraining
|
||||
mapping-key order or document-marker style. Run the affected requirements test
|
||||
and owning module suite after changing the assertion.
|
||||
|
||||
## Review validation
|
||||
|
||||
At reviewed commit `cfc2d1d`, on Go 1.26.8:
|
||||
|
||||
- All three workspace module suites passed with `GOTOOLCHAIN=local go test ./...`
|
||||
from their respective module directories.
|
||||
- Fresh root tests passed with `GOWORK=off GOTOOLCHAIN=local`, `-mod=readonly`,
|
||||
and `-count=1`. Root module and production/test import enumeration confirmed
|
||||
no optional integration or owning-library dependency.
|
||||
- Fresh ckconfig and ckprompt suites, module graphs, and production/test import
|
||||
enumeration passed with workspace disabled and readonly disposable copies of
|
||||
committed module/sum files. Only unpublished CommandKit prerequisites were
|
||||
replaced with local sources. Temporary metadata hashes remained unchanged
|
||||
during validation, and distributed manifests were not edited.
|
||||
- The isolated ckprompt graph selects PromptKit v0.14.0 and ConfigKit v0.7.1;
|
||||
ckconfig has no ckprompt or PromptKit dependency. Local prerequisite
|
||||
replacements establish source compatibility, not future tag availability.
|
||||
- Composed requirements invocations produced the expected partial and complete
|
||||
inventories with status `0`. The complete report still contained an unsupplied
|
||||
required variable, consistent with native discovery semantics.
|
||||
- Changed Markdown local links/fragments and commit-range whitespace checks
|
||||
passed. No tests or production code were added or changed for this review.
|
||||
|
||||
PromptKit v0.14.0's known lack of a public fixture path for successful engine
|
||||
configuration warnings remains a validation limitation, not a new implementation
|
||||
gap. Existing custom-callback coverage exercises command diagnostic handling.
|
||||
These passing checks do not resolve the three findings above.
|
||||
Reference in New Issue
Block a user