145 lines
7.9 KiB
Markdown
145 lines
7.9 KiB
Markdown
# PromptKit requirements inspection review
|
||
|
||
Status: R-1 completed; R-2 and R-3 open. This document records 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: completed. 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.
|
||
|
||
**Completed:** Qualified the CLI singleton statement to exclude `--var`, retaining
|
||
the existing per-key replacement and selector-binding descriptions. Reviewed the
|
||
wording against `newRequirementsCommand` and the existing request/assignment
|
||
coverage. Local links/fragments and `git diff --check` passed. No parser, handler,
|
||
or test changes were needed.
|
||
|
||
## 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.
|