267 lines
19 KiB
Markdown
267 lines
19 KiB
Markdown
# Architectural responsibility review
|
||
|
||
Status: four open P3 candidates for consideration; no implementation changes.
|
||
Reviewed source: `4a91f29bcd6583fb77d7283f955a0220e7291f8c`.
|
||
|
||
## Scope and conclusion
|
||
|
||
This review examined repository-owned core, internal packages, ckconfig,
|
||
ckprompt, maintained consumers, and release-launcher boundaries. Dependency
|
||
interfaces were consulted to establish what CommandKit can delegate; dependency
|
||
implementations were not audited. The review followed the
|
||
[architecture policy](../../policy/architecture.md),
|
||
[agent policy](../../policy/agents.md), and accepted [ADRs](../../adr/README.md).
|
||
Future proposals and retired plans were distinguished from selected contracts.
|
||
|
||
The broad ownership split is sound. No substantial configuration resolver,
|
||
prompt preparation engine, or application runtime framework was found that
|
||
should be removed wholesale. The actionable opportunities are narrower remnants
|
||
of earlier boundaries and execution paths. Replacing these with another generic
|
||
adapter framework would defeat the purpose of the changes.
|
||
|
||
| Outcome | Assessment |
|
||
| --- | --- |
|
||
| Necessary machinery | Captured declarations, typed preflight, native capability adaptation, safe report conversion, and invocation reporting have concrete selected responsibilities. See the retention assessment below. |
|
||
| Unnecessary implementation complexity | R2–R4 identify redundant parser plumbing, validation of already-valid native paths, and private entry points sustained only by tests. |
|
||
| Complexity caused by an optional contract | R1 identifies an unused method in a public provider interface. Removing it preserves CLI behavior but changes what callers can invoke through that interface. |
|
||
|
||
R1 overlaps M6 in the [complexity audit](03-complexity.md); it is not an additional
|
||
independent issue. This review confirms its historical cause and consumer impact.
|
||
Other findings in that audit concerning function extraction or consolidation
|
||
are not repeated merely because the functions are long.
|
||
|
||
## Current architecture
|
||
|
||
- **Core** captures typed declarations, validates installation batches, resolves
|
||
application aliases, prepares supplied inputs, dispatches callbacks, and owns
|
||
help, completion, reporting, and outcomes. Its shared report API owns text,
|
||
YAML, and JSON encoding. Applications and integrations choose report content.
|
||
- **Private packages** isolate command paths, alias resolution/termination,
|
||
neutral metadata, and the mutable Cobra/pflag tree. The adapter parses and
|
||
renders help; core executes application callbacks. No backend type is required
|
||
in the public API.
|
||
- **ckconfig** composes scope capabilities and independently prepared inputs,
|
||
invokes application loaders, and adapts ConfigKit's effective configuration,
|
||
diagnostics, provenance, metadata, and native differences into CLI reports.
|
||
ConfigKit remains responsible for configuration semantics and redaction.
|
||
- **ckprompt** composes operator-facing resource inspection, requirements,
|
||
request validation, prepared previews, and smoke tests using native PromptKit
|
||
operations. It also supplies reusable CLI bindings and a ckconfig scope.
|
||
PromptKit owns preparation, resolution, and smoke-test semantics; applications
|
||
own engine/client construction and request builders.
|
||
- **Applications** own process exit, contexts, streams, startup, long-lived
|
||
resources, and domain workflows. The release launcher delegates release
|
||
operations to ReleaseKit. Neither integration belongs in core's dependency
|
||
graph; ckprompt uses ckconfig's public API.
|
||
|
||
The [internal overview](../../internal/overview.md) owns the full package
|
||
inventory. Reviewed integration dependencies are ConfigKit v1.1.0 and PromptKit
|
||
v0.20.0, as selected in the integration module files.
|
||
|
||
## R1 — P3: Explanation providers still owe an unused text writer
|
||
|
||
**Outcome:** complexity caused by an optional public contract; recommend narrowing
|
||
that contract deliberately.
|
||
|
||
**Code and evidence:** [contracts.go](../../../integrations/ckconfig/contracts.go),
|
||
`ExplanationReader` at lines 76–80 requires both `Explain` and
|
||
`WriteExplanation`. Production consumers in
|
||
[explain.go](../../../integrations/ckconfig/explain.go) and
|
||
[show.go](../../../integrations/ckconfig/show.go) use only `Explain`.
|
||
The direct interface-writer call remains in
|
||
[contracts_adapter_test.go](../../../integrations/ckconfig/contracts_adapter_test.go),
|
||
line 121; provider doubles also implement the unused method.
|
||
|
||
**Requirement and cause:** ckconfig needs the native explanation value to select
|
||
report content. Core owns formatting that content. Commit `c61e125` replaced the
|
||
text template's call back into `WriteExplanation` with a template over the
|
||
acquired explanation, making text and structured output use the same evidence.
|
||
The provider interface was not narrowed with that change. It still assigns text
|
||
rendering work to a provider even though the CLI no longer uses it.
|
||
|
||
**Removal test:** deleting the method from the interface would leave production
|
||
CLI operations unchanged. Existing native providers still satisfy the smaller
|
||
interface. A consumer calling `view.Explanation.WriteExplanation(...)` through
|
||
the interface would stop compiling; a consumer calling the native ConfigKit
|
||
object directly would be unaffected. That is a real source-level consideration,
|
||
but the repository supplies no remaining CLI requirement for this accommodation.
|
||
|
||
**Potential remediation:** reduce `ExplanationReader` to `Explain`, update its
|
||
GoDoc, and remove writer-only provider stubs and interface assertions. Retain
|
||
coverage of native explanation acquisition and rendering from acquired values.
|
||
Record the public API change through the normal release process. Do not restore
|
||
the obsolete callback, add a second writer interface, or recreate the writer
|
||
through another adapter merely to preserve this unused requirement.
|
||
|
||
## R2 — P3: Parser sessions carry application execution resources they do not use
|
||
|
||
**Outcome:** unnecessary implementation complexity; simplification can preserve
|
||
the public execution contract.
|
||
|
||
**Code and evidence:** [metadata.go](../../../internal/commandmeta/metadata.go),
|
||
`ParseOptions` at lines 184–194;
|
||
[parse.go](../../../internal/cobra/parse.go), `normalizeOptions` and `buildTree`
|
||
at lines 292–358; [execute.go](../../../execute.go), `execution` and
|
||
`newExecution` at lines 80–89 and 118–130.
|
||
|
||
`newExecution` retains an unread `streams` field, gives the streams to the
|
||
reporter, and also passes output/error writers and context into parser options.
|
||
The adapter fills in four resource defaults and installs context/input/output/
|
||
error resources on each Cobra node. Its actual path uses `Find`, `ParseFlags`,
|
||
`Usage`, and `UseLine`; it does not execute Cobra callbacks or read input.
|
||
`RenderHelp` temporarily uses its own buffer and returns the text for core to
|
||
deliver. The existing `TestSessionReusesTreeForParseHelpAndUsageWithFreshState`
|
||
explicitly checks that supplied session streams remain unused.
|
||
|
||
**Requirement and cause:** contexts and destinations belong to the invocation
|
||
and its reporter. Parser options retain an execution-shaped interface even
|
||
though the backend's job is now parsing and help rendering. The extra resources
|
||
have no selected reader or execution behavior to support. `newExecution` also
|
||
normalizes the same streams that `newReporter` normalizes immediately afterward.
|
||
|
||
**Removal test:** dropping the unread execution field and parser context/input
|
||
plumbing does not remove access to `Invocation.Context` or reporter destinations.
|
||
Dropping caller-writer plumbing can likewise preserve behavior if the adapter
|
||
retains explicit discard destinations and local help buffering. Simply removing
|
||
all backend stream setup is not equivalent: Cobra defaults must not introduce
|
||
unmanaged process output. This is a necessary output boundary, not a reason to
|
||
retain caller-owned streams throughout the backend tree.
|
||
|
||
**Potential remediation:** keep resource ownership in execution/reporting,
|
||
remove the unused field and parser resource options, normalize streams once,
|
||
and give the adapter only the alias-selection inputs it consumes. Keep a simple
|
||
internal sink and help buffer where necessary. Preserve the runnable marker
|
||
used by help and other backend metadata; this finding does not justify deleting
|
||
every Cobra setting indiscriminately. Retain the existing tests that show help
|
||
does not escape core delivery and that invocations receive their real context.
|
||
|
||
## R3 — P3: Concise diff rendering revalidates native ConfigKit paths
|
||
|
||
**Outcome:** unnecessary implementation complexity; preserve the concise report.
|
||
|
||
**Code and evidence:** [diff_report.go](../../../integrations/ckconfig/diff_report.go),
|
||
`decodeJSONPointer` at lines 11–44 and its sole production caller,
|
||
`renderDiffReport` at lines 127–155.
|
||
The helper accepts arbitrary strings and validates leading slashes and each
|
||
escape, although its caller supplies only `difference.Path.String()`.
|
||
[diff_report_test.go](../../../integrations/ckconfig/diff_report_test.go) tests
|
||
malformed strings that this production boundary cannot supply.
|
||
|
||
**Requirement and owner:** ConfigKit owns path construction and parsing. Its
|
||
public `Path` is immutable, has unexported representation, has a valid root zero
|
||
value, and documents its string representation as JSON Pointer. Even a custom
|
||
comparison callback returns `[]configkit.Difference`, whose paths have this
|
||
native type. ckconfig needs decoded key tokens to assemble the selected concise
|
||
report's nested additions/changes. ConfigKit exposes no public token accessor,
|
||
so this small adaptation remains necessary; a general pointer validator does not.
|
||
|
||
**Removal test:** eliminating validation of the native representation would not
|
||
reject or accept different supported comparison inputs. Removing token decoding
|
||
altogether would break literal slash/tilde keys. Replacing the entire concise
|
||
report with native difference records would change the deliberately selected
|
||
operator report, including the distinction between ordinary and detailed output.
|
||
|
||
**Potential remediation:** make the private helper explicitly consume a native
|
||
`configkit.Path`, split its canonical pointer text, and decode the two escapes
|
||
with the standard library, without an error-returning validation state machine.
|
||
Use a single-pass replacement so `~01` becomes `~1`, not `/`. Remove only the
|
||
now-unreachable validation branches and malformed-string assertions. Preserve
|
||
root/empty/numeric/dot/percent/Unicode/slash/tilde key behavior and snapshot
|
||
decoding errors. Keep `configkit.ParsePath` for actual operator-supplied paths.
|
||
This does not require an upstream change, a public pointer utility, or relaxed
|
||
validation of untrusted CLI syntax.
|
||
|
||
## R4 — P3: Private production entry points exist only to sustain older tests
|
||
|
||
**Outcome:** unnecessary implementation complexity; lower priority than the
|
||
boundary changes above.
|
||
|
||
**Code and evidence:** [reporter.go](../../../reporter.go), `setVisibility` at
|
||
lines 668–677 and `close` at lines 679–686, have only test callers. Actual
|
||
execution uses `applySelection` and `finalize`. The former test setter repeats
|
||
visibility-state assignment while omitting the current closed-state check and
|
||
format selection; the latter only flips the closed bit, bypassing deferred
|
||
delivery and finalization. `TestReporterClosedHandlesReturnClosedErrorIncludingQuiet`
|
||
therefore exercises a synthetic close path rather than invocation completion.
|
||
|
||
Similarly, `App.parse` in [parser.go](../../../parser.go), lines 13–19, is called
|
||
only by `TestAppParseUsesRegisteredBindingsWithTheAdapter`. Actual dispatch
|
||
uses `execution.parse`, which prepares the route-aware session.
|
||
`prepareInput` and `prepareInputWithSpellings` in
|
||
[binding.go](../../../binding.go), lines 244–250, form a test-only compatibility
|
||
chain into the current `prepareInputWithOccurrences` entry point.
|
||
|
||
**Requirement and cause:** testing internal behavior is legitimate, but these
|
||
superseded entry points are not needed to provide a supported consumer API.
|
||
They retain earlier call shapes after occurrence-aware preparation, routed
|
||
sessions, and reporter finalization became the real paths. Passing their tests
|
||
does not show that the corresponding current lifecycle path was exercised.
|
||
|
||
**Removal test:** production behavior and exported APIs would be unchanged;
|
||
the affected tests would need to use current preparation, selection, and
|
||
finalization entry points. This is test accommodation, not consumer compatibility.
|
||
Other tests already exercise actual finalization and session preparation, so
|
||
the finding is not a claim that these behaviors have no coverage.
|
||
|
||
**Potential remediation:** use the actual private lifecycle/preparation methods
|
||
where the test is asserting their effects, and remove the obsolete production
|
||
wrappers. If concise test setup still benefits from a helper, keep it in a
|
||
`_test.go` file and delegate to the real path. A read-only test observation helper
|
||
such as `reportingState.presentation` can likewise live with the tests; it is
|
||
not itself an alternate lifecycle. Do not remove useful behavioral assertions,
|
||
export new testing hooks, or replace these few helpers with a testing framework.
|
||
|
||
## Machinery worth retaining under the selected contracts
|
||
|
||
These were pressure-tested as potential deletion targets. Their responsibilities
|
||
are more substantial than their surface resemblance to dependency functionality.
|
||
|
||
| Area | Why direct delegation or deletion is insufficient |
|
||
| --- | --- |
|
||
| Core declarations, neutral metadata, and fresh backend sessions | The selected facade isolates backend types, captures declarations, installs atomically, converts supplied inputs before factories, and supports fresh mutable parse state. Calling Cobra execution directly changes these boundaries. There is no need for a public backend/plugin framework. |
|
||
| Alias prefix resolution and termination analysis | Repeated longest-prefix rewrites and residual tokens support selected chained/group aliases. An expanding cycle need not repeat an identical whole command path. A visited-string check or arbitrary hop cap would weaken correctness or introduce an unselected restriction. A single-rewrite product would simplify this, but would change accepted application alias behavior; no such change is recommended here. |
|
||
| Scope plans and independently prepared diff inputs | Scope capabilities, aggregate operations, and independent left/right selections are selected CLI contracts. Loading through application callbacks preserves startup ownership. Sharing one prepared input or inferring the right side from the left would alter those contracts. |
|
||
| Native request conversion and reusable request flags | CLI strings, files, explicit presence, and occurrence order must become native PromptKit request values. The implementation delegates domain preparation and smoke execution upstream. The flag collections serve explicitly selected application and standard-command reuse, rather than a general internal PromptKit convenience layer. |
|
||
| Safe configuration YAML to shared report values | ConfigKit exposes effective output and snapshots through safe YAML interfaces. Returning the original Go settings could bypass native redaction; raw YAML passthrough would bypass selected shared formatting and JSON support. The conversion loses presentation details by design and does not rerun configuration resolution. |
|
||
| Shared JSON/YAML normalization and numeric handling | JSON tags and marshalers define the common report meaning; ordinary YAML marshaling of a `json.Number` is not automatically an equivalent numeric representation. The existing adapter preserves that meaning. CLI JSON's bounded native numeric conversion is a separate selected policy, not grounds for silently rounding or rejecting application report values. No arbitrary-precision public number system is warranted. |
|
||
| PromptKit report projections | Native smoke reports explicitly have no stable JSON contract. The projection supplies the selected field names, duration strings, empty collections, and unknown/false distinctions. Passing the native struct directly would visibly change reports. This is CLI content adaptation; a reflection-based tagging framework would merely move the complexity. |
|
||
| Reporter state and synchronization | Visibility, deferred messages, operation-versus-presentation evidence, borrowed writers, and concurrent whole reporter operations are selected behavior. A bare `fmt.Fprint` cannot preserve them. Existing concurrency/resource findings remain valid and separate; retaining the boundary does not endorse every current implementation detail. |
|
||
|
||
The [complexity audit](03-complexity.md) also identified duplicate ConfigKit
|
||
diagnostic adaptation across the integrations (M5). Both already delegate
|
||
diagnostic text to ConfigKit. Sharing that adapter could reduce maintenance,
|
||
but adding a public helper is an API decision, not evidence that the adaptation
|
||
can simply be deleted. The earlier finding remains open; this review does not
|
||
recommend creating another abstraction to resolve it automatically.
|
||
|
||
## Evidence and limits
|
||
|
||
- Used the repository code graph for symbol/call discovery, inspected source and
|
||
selected history, and checked textual references where graph receiver-name
|
||
collisions or callback references made a call trace insufficient. No-call
|
||
graph results alone were not treated as proof of dead code.
|
||
- Consulted the pinned dependencies' local public GoDoc, including ConfigKit
|
||
`Path` and PromptKit `SmokeTestReport`, to check delegation boundaries.
|
||
- Ran 21 existing focused top-level tests, all passing, in the aligned workspace:
|
||
parser/session isolation, reporter visibility/closure/finalization, native diff
|
||
projection and pointer tokens, smoke forwarding/projection/preparation errors,
|
||
requirements evidence, CLI numeric policy, and shared report number
|
||
preservation. Subtests are additional to that count.
|
||
- Used the cached Go 1.27.0 compiler with `GOTOOLCHAIN=local`, `GOPROXY=off`,
|
||
`-mod=readonly`, and a writable temporary build cache. No dependency files
|
||
changed. The preceding [documentation audit](13-documentation.md) records
|
||
passing full workspace and independent-module suites for the unchanged code;
|
||
those full suites were not repeated for this document-only review.
|
||
- No production code or repository tests were edited, and no replacement
|
||
implementations were tested. Passing current tests substantiates the existing
|
||
boundaries; it does not prove that a proposed deletion is already safe to
|
||
merge. Each eventual change still needs focused validation of its effects.
|
||
- No network service, live model, release publication, or external consumer
|
||
migration was exercised. No additional restrictions on caller input or
|
||
upstream behavior are proposed by this report.
|
||
|
||
Recommended order: resolve the small public-interface decision in R1, remove
|
||
the unused parser resource plumbing in R2, then address the bounded R3/R4
|
||
cleanup when touching those areas. No architectural rewrite is warranted by
|
||
these findings.
|