Skip to content

feat(cli)!: refactor fctl for v4 - #195

Draft
flemzord wants to merge 58 commits into
mainfrom
feat/stack-v4-cli
Draft

flemzord wants to merge 58 commits into
mainfrom
feat/stack-v4-cli

Conversation

@flemzord

@flemzord flemzord commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

This draft tracks the ongoing fctl v4 refactor on feat/stack-v4-cli.

The branch separates the CLI host from product-owned plugins through a standalone public SDK, adds external plugin discovery and distribution, retains a legacy compatibility module, and provides streaming execution and progress reporting. The core owns connections, authentication, token renewal, interaction, and output rendering; plugins own service commands and business behavior.

Proposed follow-up: plugin transport v2

Discuss replacing the current JSON-in-BytesValue protocol and separate HTTP callback broker with typed protobuf messages and one bidirectional gRPC execution stream per command. HTTP requests/responses, input/output chunks, progress, cancellation, and the final result would share that stream. Authentication stays in the core; plugins continue using an injected HTTP client. This is a proposal, not an implemented change in this branch.

The design must define bounded multiplexing and flow control, per-request cancellation, explicit v1/v2 compatibility, and migration of independently distributed plugins.

Status

Work in progress; not ready for merge. This PR was opened from the existing published branch without additional code changes or commits. No tests were run as part of opening the PR; validation of the complete branch remains to be established.

BREAKING CHANGE: remove the v3 command tree, shared CLI framework and generated clients before rebuilding fctl v4. This checkpoint intentionally has no runnable CLI.
@NumaryBot NumaryBot added risk: high bot-reviewed draft-reviewed NumaryBot reviewed this draft, but published no readiness signal. labels Oct 12, 2026
@gfyrag

gfyrag commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Could we make the plugin wire protocol explicit with checked-in .proto files and generated Go bindings before deciding whether to replace the separate HTTP callback with a single bidirectional stream?

Today pkg/pluginsdk/transport/wire.go declares both gRPC services by hand and puts JSON-encoded Go structs inside google.protobuf.BytesValue. The same pattern is used for execution frames and the host HTTP callback. That leaves the protocol shared by independently released plugins without a schema that can be reviewed, generated for another language, or checked for compatible evolution.

I would like the current boundary to be described by typed Protobuf messages for both directions: manifest/execution requests and results, host HTTP requests and responses, stream frames, progress, and structured errors. Service payloads can remain bytes where exact JSON preservation matters, especially for large integers; this does not require converting service JSON to google.protobuf.Struct.

Could we also define the protocol versioning and migration path for already distributed plugins, with interoperability tests between host and plugin versions? This can be done while retaining the existing two-channel transport. Whether to multiplex everything onto one stream is a separate design decision with different flow-control and cancellation trade-offs.

@Dav-14 Dav-14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about the product/core presentation contract; this is a design proposal, not a merge-blocking defect.


func orderedKeys(object map[string]any) []string {
keys := slices.Sorted(maps.Keys(object))
preferred := []string{"module", "id", "name", "state", "status", "region", "regionid", "version", "createdat", "asset", "account", "amount", "balance"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we clarify how each command identifies the essential fields for its list and detail views? Today ExecuteResponse only carries JSON data, while this core-level priority list chooses columns globally; listColumns also independently caps them at six even when the terminal is wide enough. My preference would be a small, stable default view defined by the use case, with optional wide output and complete JSON (YAML would be a separate addition). Could the plugin provide optional ordered field hints for list/detail while the core keeps ownership of rendering? The exact contract remains to be agreed; this is a design question rather than a claim that such hints already exist.

@Dav-14 Dav-14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two follow-ups: a P2 help/provider consistency issue identified by the independent review, and a separate product proposal for profile configuration. The latter needs a product decision rather than being treated as an implementation defect.

Comment thread cmd/factory/legacy.go
if err != nil && !errors.Is(err, pluginselection.ErrNotSelected) {
return nil, false, err
}
if err == nil && selectedProvider.Provider == "legacy" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Installing or syncing a modern plugin does not refresh a previously cached legacy provider selection. Help and completion still expose the legacy command tree, while the next business command selects the modern plugin. Could we reconcile this selection using the installed metadata and test the legacy → install → offline-help transition?

This applies to a direct target previously recorded as legacy, with FCTL_PLUGIN_CATALOGUE unset. Help skips installedModern because p.execution is false and reuses the cached legacy selection here; plugins install/sync writes the modern lock without updating that selection. The next business execution updates it through saveModernMetadata. This finding is supported by the source paths; the complete transition has not been reproduced end to end.


func (s *Settings) fields() []field {
return []field{
{"stack-url", &s.Options.StackURL, "Gateway base URL"},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Product proposal: could we move connection and authentication configuration to profile commands?

  • Cloud profiles would discover endpoints through the core.
  • Client-credentials profiles would require a stack URL, with module endpoints derived by the core.
  • None profiles could configure a stack URL or an explicit endpoint for the selected module.

This would remove four global URL flags (stack-url, ledger-url, auth-url, connectivity-url) and five authentication flags (auth-mode, token-url, client-id, scopes, issuer): nine in total. Their required values would remain configurable through profile commands, while profile and Cloud organization/stack selection would remain available. Secrets could continue coming from the environment.

Restricting client credentials to stack gateways changes currently supported and tested behavior, so this needs an explicit product decision. The impact on scripts using global overrides also needs to be considered.

@Dav-14 Dav-14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on the multi-stack cache diagnostic: distinguish ambiguous cached metadata from an absent installation.

continue
}
if selected.Service != "" {
return pluginmanager.Lock{}, pluginmanager.ErrNotInstalled

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple cached targets match here, but returning ErrNotInstalled makes the CLI suggest installation rather than target selection. Could we return a distinct ambiguity error asking the user to specify --organization and/or --stack?

For a Cloud profile without a fully specified target, two prepared matching stacks trigger this branch even though their plugins are installed. This concerns cached/offline command metadata, not choosing the execution target from the cache. TestServiceLoaderCacheIsolation currently expects ErrNotInstalled for this ambiguity; it could instead cover the dedicated diagnostic.

@gfyrag

gfyrag commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

On the proposed single bidirectional execution stream: I think it is worth exploring, but I do not think we have established that it is the best choice yet. My goal here is to compare the viable options before we commit to a public plugin protocol.

The current design already has a bidirectional ExecuteStream for input, output and progress, while HTTP calls back into fctl through a separate gRPC broker connection. Keeping those channels separate lets gRPC handle concurrent HTTP requests independently and gives the two data flows their own backpressure. A single stream could make the connection topology simpler and offer a natural place for future host interactions, such as prompts during execution or other explicitly granted host capabilities. But it would also require us to define request IDs, response correlation, multiplexing, per-request cancellation and flow control so a large transfer cannot block an HTTP response that the plugin needs to continue.

Could we evaluate at least these options explicitly: retain the current separate callback, use one multiplexed bidirectional stream per command, or keep execution streaming while adding narrowly scoped host callback services as new needs arise? Useful scenarios to test against are concurrent HTTP calls, streaming import/export during service requests, cancellation and partial failures, interactive prompts during execution, and future host capabilities. I am not asking to switch to one stream in this PR; I want us to understand what each design makes easier or harder before choosing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-reviewed draft-reviewed NumaryBot reviewed this draft, but published no readiness signal. risk: high

Development

Successfully merging this pull request may close these issues.

4 participants