From 68aa9b03896169afbc668c00911405ffbf704a7c Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 10:15:48 -0700 Subject: [PATCH 01/14] feat: add cursor-first pagination (--limit/--continue) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the design in docs/proposals/cursor-first-pagination.md: CommandSpec::with_cursor/CursorConfig register --limit/--continue as a new, parallel pagination mechanism alongside the existing offset-based CommandSpec::with_pagination/PaginationConfig (--limit/--offset), which is untouched and remains fully supported. Unlike offset pagination, the engine never slices or measures a cursor itself — a cursor is backend-opaque, so only the handler that talked to the backend can supply the next resume token. Handlers report what they learned via CommandResult::with_cursor(CursorContinuation), which surfaces as a new envelope.cursor field (CursorMeta) and an automatic next_actions "next page" suggestion, mirroring the existing offset pagination machinery end to end: flag registration, middleware state, envelope construction, human-output rendering (table footer merge and standalone summary), and docs. CursorContinuation::with_limit lets a handler report an effective page size that differs from the parsed --limit (e.g. one it derived from the --continue token itself, so a caller can resume with --continue alone without repeating --limit) — this also signals the engine to omit --limit from the auto-generated next-page command, since the token is then self-sufficient about size. A command opts into at most one of with_pagination/with_cursor; raw_output remains mutually exclusive with both. --- AGENTS.md | 7 +- cli-engine/docs/concepts.md | 24 +- cli-engine/src/cli/flags_apply.rs | 38 +- cli-engine/src/cli/mod.rs | 2 + cli-engine/src/cli/run.rs | 18 +- cli-engine/src/cli/schema_tree.rs | 24 +- cli-engine/src/command/mod.rs | 97 ++- cli-engine/src/command/spec.rs | 79 ++- cli-engine/src/flags/mod.rs | 40 +- cli-engine/src/flags/register.rs | 49 ++ cli-engine/src/lib.rs | 31 +- cli-engine/src/middleware/mod.rs | 20 + cli-engine/src/middleware/run.rs | 76 ++- cli-engine/src/output/envelope.rs | 39 ++ cli-engine/src/output/human/body.rs | 69 ++- cli-engine/src/output/human/footer.rs | 95 ++- cli-engine/src/output/human/mod.rs | 15 +- .../src/output/human/tests/alignment.rs | 10 +- .../src/output/human/tests/width_fitting.rs | 28 +- cli-engine/src/output/mod.rs | 2 +- cli-engine/tests/cursor_pagination.rs | 565 ++++++++++++++++++ cli-engine/tests/foundation.rs | 6 + 22 files changed, 1224 insertions(+), 110 deletions(-) create mode 100644 cli-engine/tests/cursor_pagination.rs diff --git a/AGENTS.md b/AGENTS.md index 80920fa..f1e19ae 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -239,11 +239,8 @@ Command checklist: - Prefer `CommandSpec::from_args::()` + `RuntimeCommandSpec::new_typed` when the command has many flags, needs clap validation attributes, or when porting existing derive-based commands. Use the builder path for simple commands with one or two flags. - Most commands need more than `new_typed`'s `(CredentialResolver, T)` shape — if the handler needs the command path, middleware, or `--dry-run` via `CommandContext`, use `RuntimeCommandSpec::new_typed_with_context` (handler: `Fn(CommandContext, T) -> Fut`) instead of `new_with_context` + `context.typed_args::()`; for streaming commands, use `RuntimeCommandSpec::new_typed_streaming` (handler: `Fn(CommandContext, T, StreamSender) -> Fut`). Both eagerly parse `T` before the handler runs, same guarantee `new_typed` gives the credential-only case. - Use `CommandSpec::with_arg_group(ArgGroup::new(...).args([...]).required(true))` for "at least one of" or mutually-exclusive relationships between args, instead of a `required_unless_present_any`/`conflicts_with` chain. With `from_args::()`, express the same thing declaratively via a struct-level `#[group(required = true, multiple = true)]` (or `multiple = false` for mutually exclusive) on the derive struct — but not on a struct that also has a `#[command(flatten)]` field; `clap_derive` empties that struct's implicit group's members in that case, so the constraint silently does nothing. -- `--limit`/`--offset` are not framework-global: a command only gets them by calling - `.with_pagination(PaginationConfig { default_limit, max_limit, ..Default::default() })`. A - command with no `.with_pagination(...)` call never registers those flags — absent from its - `--help`, rejected as unknown arguments if passed. `default_limit` applies when the user passes - neither flag; `max_limit` (`0` = uncapped) rejects an explicit `--limit` above the cap. +- `--limit`/`--offset` are not framework-global: a command only gets them by calling `.with_pagination(PaginationConfig { default_limit, max_limit, ..Default::default() })`. A command with no `.with_pagination(...)` call never registers those flags. `default_limit` applies when the user passes neither flag; `max_limit` (`0` = uncapped) rejects an explicit `--limit` above the cap. Offset pagination is purely client-side: the engine slices whatever array the handler returns using the parsed `--limit`/`--offset` itself, so it only fits a backend that either hands back its full collection or itself supports arbitrary-offset slicing. +- Use `.with_cursor(CursorConfig { default_limit, max_limit })` instead of `.with_pagination` when the backend is cursor-based. This registers `--limit`/`--continue` instead of `--limit`/`--offset`. A command picks exactly one of `.with_pagination`/`.with_cursor`. Unlike offset pagination, the engine cannot slice or measure a cursor itself — the handler reads the parsed values back off `ctx.middleware.cursor_limit`/`.continue_token` to drive its own backend call, and reports what it learned via `CommandResult::with_cursor(CursorContinuation::more(next_token).with_total(n).with_remaining(n))`, or `CursorContinuation::done()` (or no call at all) once iteration is exhausted. - Use `.raw_output(true)` for a command whose only correct output is verbatim text (e.g. printing a schema/config blob to pipe to a file), not a JSON reconstruction of it. The handler's `CommandResult` data must be a JSON string; this removes the `--output`/`--fields`/`--filter`/`--expr`/pagination flags and is incompatible with streaming commands. ## Output And Schemas diff --git a/cli-engine/docs/concepts.md b/cli-engine/docs/concepts.md index eb968ae..ee9c829 100644 --- a/cli-engine/docs/concepts.md +++ b/cli-engine/docs/concepts.md @@ -282,6 +282,17 @@ CommandSpec::new("list", "List projects").with_pagination(PaginationConfig { }) ``` +`--limit`/`--continue` are the cursor-pagination counterpart, for a command backed by aserver-maintained, forward-only cursor API — see [cursor pagination](#cursor-pagination): + +```rust +CommandSpec::new("list", "List domains").with_cursor(CursorConfig { + default_limit: 25, + max_limit: 500, +}) +``` + +A command opts into exactly one of `with_pagination`/`with_cursor`, never both. + ## Middleware Command execution flows through a consistent middleware chain: @@ -508,6 +519,16 @@ the user ran — including every flag they passed — with `--limit`/`--offset` page, so both agent callers (`next_actions[]`) and human callers (the "Next steps:" footer) get a literal follow-up command instead of having to compute the next offset themselves. +### cursor pagination + +A command that opted into `--limit`/`--continue` cursor pagination via `CommandSpec::with_cursor` gets a top-level `cursor` field on the envelope instead of `pagination` — `limit`, `count`, `total`, `remaining`, `continue_from`, and `has_more` — whenever it returned array data. Unlike `pagination`, the engine cannot compute this itself: a cursor is opaque to everything except the handler that called the backend, so `limit`/`count` are the only pieces the engine derives (the parsed `--limit` and the returned array's length); `total`/`remaining`/`continue_from` come from whatever the handler reported via `CommandResult::with_cursor(CursorContinuation::more(token).with_total(n).with_remaining(n))` — or `CursorContinuation::done()` (or no call at all) to report the end of iteration. `total`/`remaining` are `None` when the backend never reports them, which a pure opaque-cursor API is not obligated to do. + +Human output merges this into the table's row-count footer: `(N of M rows)` when a total is known, `(N rows, M remaining)` when only a remaining count is known, or `(N rows so far; use --continue for more)` when neither is known. A cursor-paginated response that doesn't render as a table gets the standalone counterpart: `Showing N of M`, `Showing N (M remaining)`, or `Showing N items so far; use --continue for more`. + +When `continue_from` is present (`has_more`), the engine appends a `next_actions` entry replaying the command with `--limit`/`--continue ` for the next page. + +A command registers `with_pagination` or `with_cursor`, never both. + ### fix Failed commands can attach recovery guidance as a top-level `fix` on the error envelope @@ -529,8 +550,7 @@ it. The output pipeline runs in this order: 1. **Filtering**: `--filter` evaluates a JMESPath predicate against each item in list data. -2. **Pagination**: `--limit` and `--offset` slice list data and attach the envelope's top-level - `pagination` field (see [pagination](#pagination) above). +2. **Pagination**: `--limit` and `--offset` slice list data and attach the envelope's top-level `pagination` field (see [pagination](#pagination) above). Inert for a `with_cursor` command. 3. **Expression**: `--expr` evaluates a JMESPath query against the whole current result. 4. **Field selection**: `--fields` selects comma-separated fields and nested dot paths. 5. **Formatting**: `--output` renders `json`, `human`, or `toon`. diff --git a/cli-engine/src/cli/flags_apply.rs b/cli-engine/src/cli/flags_apply.rs index 44b9d78..f8e1fa4 100644 --- a/cli-engine/src/cli/flags_apply.rs +++ b/cli-engine/src/cli/flags_apply.rs @@ -48,13 +48,34 @@ pub(super) fn apply_pagination_flags( middleware.offset = leaf.get_one::("offset").copied().unwrap_or(0); } +/// Sets `middleware.cursor_limit`/`middleware.continue_token` from a +/// cursor-paginating command's own `--limit`/`--continue`. Unlike +/// [`apply_pagination_flags`], the framework never slices with these itself +/// — a cursor-aware handler reads them back off +/// [`CommandContext::middleware`](crate::command::CommandContext::middleware) +/// to drive its own backend call. +pub(super) fn apply_cursor_flags( + middleware: &mut Middleware, + spec: &CommandSpec, + leaf: &ArgMatches, +) { + let Some(cursor) = spec.cursor else { + return; + }; + middleware.cursor_limit = leaf + .get_one::("limit") + .copied() + .unwrap_or(cursor.default_limit); + middleware.continue_token = leaf.get_one::("continue").cloned(); +} + /// Replays a paginating command's own explicit args, plus the global /// `--filter`/`--expr`/`--fields` flags, as `--flag value` text, prefixed /// with the CLI's binary name — the base a "view the next page" /// [`crate::NextAction`] is built from once the response's -/// [`crate::PaginationMeta`] is known. Leading with the binary name keeps the -/// suggested command copy-pastable rather than a fragment starting at the -/// noun/verb path. +/// [`crate::PaginationMeta`] or [`crate::CursorMeta`] is known. Leading with +/// the binary name keeps the suggested command copy-pastable rather than a +/// fragment starting at the noun/verb path. /// /// `--filter`/`--expr`/`--fields` sit in the same output pipeline as /// pagination itself (filter -> paginate -> expr -> fields) and change what @@ -71,10 +92,11 @@ pub(super) fn apply_pagination_flags( /// flag occurrence per value (round-trips correctly whether the arg is a /// plain repeatable `ArgAction::Append` or also sets a `value_delimiter`), /// and quotes/escapes values containing whitespace or shell metacharacters -/// (see `quote_pagination_value`). Deliberately omits `--limit`/`--offset` — -/// those are added by the caller once it knows the -/// next page's offset. -pub(super) fn pagination_command_base( +/// (see `quote_pagination_value`). Deliberately omits `--limit`/`--offset` +/// and `--limit`/`--continue` — those are never part of `spec.args` to begin +/// with, and the caller appends the right pair once it knows the next +/// page's offset or continuation token. +pub(super) fn command_replay_base( binary_name: &str, command_path: &str, spec: &CommandSpec, @@ -170,7 +192,7 @@ fn pagination_arg_display(value: &serde_json::Value) -> String { /// re-escape the backslashes it just inserted) so the value can't break out /// of the double quotes or trigger POSIX-shell expansion (`$VAR`, `$(...)`, /// backticks) if the suggestion is copy-pasted into a shell. -fn quote_pagination_value(value: &str) -> String { +pub(crate) fn quote_pagination_value(value: &str) -> String { let safe_unquoted = |c: char| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.' | '/' | ':' | '@'); if value.is_empty() || !value.chars().all(safe_unquoted) { diff --git a/cli-engine/src/cli/mod.rs b/cli-engine/src/cli/mod.rs index b13c8fe..5f6b40c 100644 --- a/cli-engine/src/cli/mod.rs +++ b/cli-engine/src/cli/mod.rs @@ -23,6 +23,8 @@ mod run; mod schema_tree; mod tree_render; +pub(crate) use flags_apply::quote_pagination_value; + use crate::{ AuthProvider, CliCoreError, GuideEntry, Middleware, Module, RuntimeCommandSpec, RuntimeGroupSpec, diff --git a/cli-engine/src/cli/run.rs b/cli-engine/src/cli/run.rs index 76d9617..71048cb 100644 --- a/cli-engine/src/cli/run.rs +++ b/cli-engine/src/cli/run.rs @@ -16,8 +16,8 @@ use super::{ single_leaf_subcommand, }, flags_apply::{ - apply_global_flags, apply_pagination_flags, install_debug_transport_logger, - pagination_command_base, parse_command_timeout, + apply_cursor_flags, apply_global_flags, apply_pagination_flags, command_replay_base, + install_debug_transport_logger, parse_command_timeout, }, lookup::{ find_command_by_colon_path, has_root_version_flag, @@ -462,10 +462,20 @@ where let leaf = leaf_matches(&matches); apply_pagination_flags(&mut middleware, &command.spec, leaf); + apply_cursor_flags(&mut middleware, &command.spec, leaf); let args = command_args_from_matches(leaf, &command.spec, false); let user_args = command_args_from_matches(leaf, &command.spec, true); let pagination_command = command.spec.pagination.is_some().then(|| { - pagination_command_base( + command_replay_base( + &cli.config.name, + &command_path, + &command.spec, + &user_args, + &flags, + ) + }); + let cursor_command = command.spec.cursor.is_some().then(|| { + command_replay_base( &cli.config.name, &command_path, &command.spec, @@ -506,6 +516,7 @@ where auth: command.spec.auth, raw_output: command.spec.raw_output, pagination_command, + cursor_command, }, Arc::new(leaf.clone()), streaming_handler, @@ -542,6 +553,7 @@ where auth: command.spec.auth, raw_output: command.spec.raw_output, pagination_command, + cursor_command, }, async move |credential| { handler(CommandContext { diff --git a/cli-engine/src/cli/schema_tree.rs b/cli-engine/src/cli/schema_tree.rs index 5335c78..86c76db 100644 --- a/cli-engine/src/cli/schema_tree.rs +++ b/cli-engine/src/cli/schema_tree.rs @@ -203,14 +203,21 @@ pub(super) fn command_clap_command_with_schema_help( schemas: &SchemaRegistry, ) -> Command { debug_assert!( - !(spec.raw_output && spec.pagination.is_some()), - "command {:?} sets both raw_output and with_pagination; a single verbatim string \ - has no pages, so the two are mutually exclusive", + !(spec.raw_output && (spec.pagination.is_some() || spec.cursor.is_some())), + "command {:?} sets both raw_output and with_pagination/with_cursor; a single verbatim \ + string has no pages, so they are mutually exclusive", + spec.name + ); + debug_assert!( + !(spec.pagination.is_some() && spec.cursor.is_some()), + "command {:?} sets both with_pagination and with_cursor; a command picks one \ + pagination style, not both", spec.name ); let mut command = spec.clap_command(); command = apply_dry_run_visibility(command, spec); command = apply_pagination_args(command, spec); + command = apply_cursor_args(command, spec); let schema = schemas.get_by_path(command_path); let default_fields = default_field_names(spec); command = apply_fields_arg( @@ -289,6 +296,17 @@ fn apply_pagination_args(command: Command, spec: &CommandSpec) -> Command { crate::flags::apply_pagination_args(command, pagination.default_limit, pagination.max_limit) } +/// Registers `--limit`/`--continue` on this command's own `Command` when its +/// spec opted in via [`CommandSpec::with_cursor`], and leaves the command +/// untouched otherwise so a non-cursor command never sees those flags — in +/// `--help` or on its command line. See [`flags::apply_cursor_args`]. +fn apply_cursor_args(command: Command, spec: &CommandSpec) -> Command { + let Some(cursor) = spec.cursor else { + return command; + }; + crate::flags::apply_cursor_args(command, cursor.default_limit, cursor.max_limit) +} + /// Splits a command's raw `default_fields` string into individual field /// names, dropping the `all`/`*` sentinels that mean "every field" rather /// than naming a real field. diff --git a/cli-engine/src/command/mod.rs b/cli-engine/src/command/mod.rs index 5488d2d..a652b92 100644 --- a/cli-engine/src/command/mod.rs +++ b/cli-engine/src/command/mod.rs @@ -17,7 +17,7 @@ pub use matches::{ command_args_from_matches, command_path_from_matches, command_path_from_parts, leaf_matches, }; pub use runtime::RuntimeCommandSpec; -pub use spec::{CommandSpec, PaginationConfig}; +pub use spec::{CommandSpec, CursorConfig, PaginationConfig}; /// Sender half for streaming command output. /// @@ -92,6 +92,21 @@ impl CommandResult { self.metadata.dry_run = true; self } + + /// Attaches what this handler learned from a cursor-backed backend call. + /// + /// Only meaningful for a command that opted into + /// [`CommandSpec::with_cursor`]. Unlike offset pagination — which the + /// engine slices and measures itself — a cursor is backend-opaque, so + /// only the handler that made the call can supply the next resume token + /// (and, when the backend reports them, a total/remaining count). Omit + /// this call, or pass [`CursorContinuation::done`], to report that + /// iteration has reached its end. + #[must_use] + pub fn with_cursor(mut self, continuation: CursorContinuation) -> Self { + self.metadata.cursor = Some(continuation); + self + } } impl From for CommandResult { @@ -111,6 +126,86 @@ pub struct CommandResultMetadata { /// mutating step. Middleware tags the audit/activity outcome and envelope /// as `dry-run` instead of `ok` when this is `true`. pub dry_run: bool, + /// Set by [`CommandResult::with_cursor`] to report cursor-pagination + /// progress for a [`with_cursor`](CommandSpec::with_cursor) command. + pub cursor: Option, +} + +/// What a cursor-aware handler learned from its own backend call. +/// +/// Construct with [`CursorContinuation::more`] or [`CursorContinuation::done`], +/// then chain [`with_total`](CursorContinuation::with_total)/ +/// [`with_remaining`](CursorContinuation::with_remaining) when the backend +/// reported them. Attach to a [`CommandResult`] with +/// [`CommandResult::with_cursor`]. +#[non_exhaustive] +#[derive(Clone, Debug, Default, Eq, PartialEq)] +pub struct CursorContinuation { + /// Opaque token the engine replays as `--continue ` in the next + /// page's `next_actions` entry. `None` means iteration has reached its + /// end. + pub continue_from: Option, + /// Total item count, when the backend reports one. + pub total: Option, + /// Remaining item count, when the backend reports one. + pub remaining: Option, + /// The page size actually applied, when it differs from the parsed + /// `--limit` the engine would otherwise report. + /// + /// Only needed when a handler's *effective* page size isn't simply + /// `ctx.middleware.cursor_limit` — e.g. it derived the size from the + /// `--continue` token itself (so a caller resuming with `--continue` + /// alone doesn't have to repeat `--limit`) rather than from this + /// invocation's own parsed flag. `None` means the parsed `--limit` is + /// exactly what was applied, the common case. + /// + /// Setting this does double duty: it also tells the engine `continue_from` + /// is self-sufficient about page size, so the auto-generated next-page + /// command omits `--limit` entirely rather than repeating a value the + /// token already carries. Leave it `None` for an opaque token the + /// handler didn't itself encode (e.g. a real server-side cursor) — the + /// engine can't know whether *that* backend tolerates a different size on + /// resume, so it plays it safe and keeps `--limit` in the replay. + pub limit: Option, +} + +impl CursorContinuation { + /// Reports that more data is available, resumed with `token`. + #[must_use] + pub fn more(token: impl Into) -> Self { + Self { + continue_from: Some(token.into()), + ..Self::default() + } + } + + /// Reports that iteration has reached its end. + #[must_use] + pub fn done() -> Self { + Self::default() + } + + /// Records the backend's reported total item count, if any. + #[must_use] + pub fn with_total(mut self, total: i64) -> Self { + self.total = Some(total); + self + } + + /// Records the backend's reported remaining item count, if any. + #[must_use] + pub fn with_remaining(mut self, remaining: i64) -> Self { + self.remaining = Some(remaining); + self + } + + /// Records the page size actually applied, when it differs from this + /// invocation's parsed `--limit`. See [`CursorContinuation::limit`]. + #[must_use] + pub fn with_limit(mut self, limit: i64) -> Self { + self.limit = Some(limit); + self + } } /// Runtime context passed to advanced command handlers. diff --git a/cli-engine/src/command/spec.rs b/cli-engine/src/command/spec.rs index 68829b7..91bcad0 100644 --- a/cli-engine/src/command/spec.rs +++ b/cli-engine/src/command/spec.rs @@ -120,8 +120,17 @@ pub struct CommandSpec { /// `None` (the default) means the command does not paginate: `--limit`/ /// `--offset` are not registered for it, so they neither show up in its /// `--help` nor parse on its command line. Set with - /// [`with_pagination`](CommandSpec::with_pagination). + /// [`with_pagination`](CommandSpec::with_pagination). Mutually exclusive + /// with [`cursor`](CommandSpec::cursor). pub pagination: Option, + /// This command's opt-in cursor-pagination policy, if any. + /// + /// `None` (the default) means the command does not register `--limit`/ + /// `--continue`. Set with [`with_cursor`](CommandSpec::with_cursor) for a + /// command backed by a server-maintained, forward-only cursor API, where + /// client-side offset slicing would cost O(N²) requests to page through. + /// Mutually exclusive with [`pagination`](CommandSpec::pagination). + pub cursor: Option, } /// Opt-in pagination policy for a single command, set with @@ -154,6 +163,37 @@ pub struct PaginationConfig { pub max_limit: i64, } +/// Opt-in cursor-pagination policy for a single command, set with +/// [`CommandSpec::with_cursor`]. +/// +/// Registering this is what makes `--limit`/`--continue` exist for a command +/// at all — without it, the engine does not register those flags, so they are +/// absent from `--help` and rejected as unknown arguments if passed. +/// +/// Unlike [`PaginationConfig`], `default_limit` must be greater than zero: +/// there is no "unlimited" sentinel here, since `--limit` is a per-request +/// page size sent to a backend, not a bound on an already-in-memory +/// collection. +/// +/// ``` +/// use cli_engine::CursorConfig; +/// +/// let cursor = CursorConfig { +/// default_limit: 25, +/// max_limit: 500, +/// }; +/// assert_eq!(cursor.default_limit, 25); +/// ``` +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct CursorConfig { + /// Page size sent to the backend when the user passes no `--limit`. Must + /// be greater than zero. + pub default_limit: i64, + /// Upper bound a user can request with an explicit `--limit`. `0` (the + /// default) means uncapped. Does not affect `default_limit` itself. + pub max_limit: i64, +} + impl CommandSpec { /// Creates a command spec with the required name and one-line help. #[must_use] @@ -348,6 +388,43 @@ impl CommandSpec { self } + /// Opts this command into cursor-paginated list output. + /// + /// Registers `--limit`/`--continue` for this command only — a command + /// that never calls this does not get those flags at all, in `--help` or + /// on the command line. Prefer this over + /// [`with_pagination`](CommandSpec::with_pagination) when the backend API + /// is itself cursor-based (an opaque resume token, or a fixed page size + /// with no arbitrary-offset support): offset-based slicing against such + /// an API costs O(N²) requests to read N items, since every page fetch + /// starts over from the beginning. See [`CursorConfig`]. + /// + /// Unlike offset pagination, the framework cannot compute `--continue`'s + /// next value itself — only the handler, which called the backend, knows + /// it. A cursor-aware handler reads the parsed `--limit`/`--continue` via + /// [`CommandContext::middleware`](crate::CommandContext)'s + /// `cursor_limit`/`continue_token` fields, and reports what it learned + /// back via + /// [`CommandResult::with_cursor`](crate::CommandResult::with_cursor). + #[must_use] + pub fn with_cursor(mut self, config: CursorConfig) -> Self { + debug_assert!( + config.default_limit > 0, + "command {:?} has a cursor default_limit ({}) that is not greater than zero", + self.name, + config.default_limit + ); + debug_assert!( + config.max_limit == 0 || config.default_limit <= config.max_limit, + "command {:?} has a cursor default_limit ({}) greater than its max_limit ({})", + self.name, + config.default_limit, + config.max_limit + ); + self.cursor = Some(config); + self + } + /// Adds provider-specific auth metadata. #[must_use] pub fn with_auth_metadata(mut self, key: impl Into, value: impl Into) -> Self { diff --git a/cli-engine/src/flags/mod.rs b/cli-engine/src/flags/mod.rs index 11203d6..6b5e973 100644 --- a/cli-engine/src/flags/mod.rs +++ b/cli-engine/src/flags/mod.rs @@ -5,7 +5,7 @@ mod register; mod resolve; pub use introspect::{debug_component_enabled, derive_bool_flags, derive_value_flags}; -pub(crate) use register::{apply_pagination_args, compat_bool_value_parser}; +pub(crate) use register::{apply_cursor_args, apply_pagination_args, compat_bool_value_parser}; pub use register::{register_global_flags, register_reason_flag}; pub use resolve::{ app_id_env_prefix, default_output_format, extract_command_path, extract_output_format, @@ -142,13 +142,16 @@ impl Default for GlobalFlags { /// `apply_filter_and_expr_examples`) with contextual help text; they must /// reuse these same values or the override would drift out of position. /// -/// `LIMIT` and `OFFSET` are never registered by [`register_global_flags`] -/// itself — unlike every other value here, `--limit`/`--offset` are not -/// framework-global at all; `cli.rs` registers them directly on a single -/// command's own `Command` (see `apply_pagination_args`), and only for a -/// command that opted in via `CommandSpec::with_pagination`. These two -/// constants exist purely so that per-command registration still parks the -/// flags in the same relative `--help` position other engine flags occupy. +/// `LIMIT`, `OFFSET`, and `CONTINUE` are never registered by +/// [`register_global_flags`] itself — unlike every other value here, +/// `--limit`/`--offset`/`--continue` are not framework-global at all; +/// `cli.rs` registers them directly on a single command's own `Command` (see +/// `apply_pagination_args`/`apply_cursor_args`), and only for a command that +/// opted in via `CommandSpec::with_pagination` or `CommandSpec::with_cursor` +/// respectively — mutually exclusive, so a single command registers `OFFSET` +/// or `CONTINUE`, never both. These constants exist purely so that +/// per-command registration still parks the flags in the same relative +/// `--help` position other engine flags occupy. /// /// `REASON` and `ENV` cover the two global flags `Cli::new` registers /// directly (conditionally, outside `register_global_flags`) rather than @@ -166,16 +169,17 @@ pub(crate) mod global_flag_order { pub(crate) const EXPR: usize = 1006; pub(crate) const LIMIT: usize = 1007; pub(crate) const OFFSET: usize = 1008; - pub(crate) const SCHEMA: usize = 1009; - pub(crate) const TIMEOUT: usize = 1010; - pub(crate) const DEBUG: usize = 1011; - pub(crate) const CREDENTIAL_STORE: usize = 1012; - pub(crate) const JSON: usize = 1013; - pub(crate) const TOON: usize = 1014; - pub(crate) const HUMAN: usize = 1015; - pub(crate) const INTERACTIVE: usize = 1016; - pub(crate) const REASON: usize = 1017; - pub(crate) const ENV: usize = 1018; + pub(crate) const CONTINUE: usize = 1009; + pub(crate) const SCHEMA: usize = 1010; + pub(crate) const TIMEOUT: usize = 1011; + pub(crate) const DEBUG: usize = 1012; + pub(crate) const CREDENTIAL_STORE: usize = 1013; + pub(crate) const JSON: usize = 1014; + pub(crate) const TOON: usize = 1015; + pub(crate) const HUMAN: usize = 1016; + pub(crate) const INTERACTIVE: usize = 1017; + pub(crate) const REASON: usize = 1018; + pub(crate) const ENV: usize = 1019; } #[cfg(test)] diff --git a/cli-engine/src/flags/register.rs b/cli-engine/src/flags/register.rs index 3ca7703..5f71ce6 100644 --- a/cli-engine/src/flags/register.rs +++ b/cli-engine/src/flags/register.rs @@ -286,6 +286,55 @@ fn pagination_offset_value_parser() -> ValueParser { }) } +/// Registers `--limit`/`--continue` directly on one command's own `clap` +/// `Command`, for a command whose [`CommandSpec`](crate::CommandSpec) opted +/// into cursor pagination via `with_cursor`. +pub(crate) fn apply_cursor_args(command: Command, default_limit: i64, max_limit: i64) -> Command { + command + .arg( + Arg::new("limit") + .long("limit") + .value_parser(cursor_limit_value_parser(max_limit)) + .allow_hyphen_values(true) + .default_value(default_limit.to_string()) + .display_order(global_flag_order::LIMIT) + .help(cursor_limit_help(default_limit, max_limit)), + ) + .arg( + Arg::new("continue") + .long("continue") + .value_name("TOKEN") + .allow_hyphen_values(true) + .display_order(global_flag_order::CONTINUE) + .help("Token to fetch the next page of data from a multi-page response (omit to start from the beginning)"), + ) +} + +fn cursor_limit_help(default_limit: i64, max_limit: i64) -> String { + let mut help = format!("Max items to return per page (default {default_limit}"); + if max_limit > 0 { + help.push_str(&format!(", max {max_limit}")); + } + help.push(')'); + help +} + +/// Rejects a non-positive `--limit` at parse time. +fn cursor_limit_value_parser(max_limit: i64) -> ValueParser { + ValueParser::new(move |raw: &str| -> Result { + let value = raw + .parse::() + .map_err(|_| format!("invalid limit value {raw:?}"))?; + if value <= 0 { + return Err(format!("limit {value} must be greater than 0")); + } + if max_limit > 0 && value > max_limit { + return Err(format!("limit {value} exceeds the maximum of {max_limit}")); + } + Ok(value) + }) +} + pub(crate) fn compat_bool_value_parser() -> ValueParser { ValueParser::new(parse_compat_bool) } diff --git a/cli-engine/src/lib.rs b/cli-engine/src/lib.rs index c6bd168..f8f91a0 100644 --- a/cli-engine/src/lib.rs +++ b/cli-engine/src/lib.rs @@ -133,9 +133,9 @@ pub use cli::{ }; pub use command::{ CommandContext, CommandFuture, CommandHandler, CommandResult, CommandResultMetadata, - CommandSpec, GroupSpec, PaginationConfig, RuntimeCommandSpec, RuntimeGroupSpec, StreamSender, - StreamingCommandFuture, StreamingCommandHandler, command_args_from_matches, - command_path_from_matches, command_path_from_parts, leaf_matches, + CommandSpec, CursorConfig, CursorContinuation, GroupSpec, PaginationConfig, RuntimeCommandSpec, + RuntimeGroupSpec, StreamSender, StreamingCommandFuture, StreamingCommandHandler, + command_args_from_matches, command_path_from_matches, command_path_from_parts, leaf_matches, }; pub use config::{ ConfigFile, CredentialStore, CredentialsConfig, EngineConfig, ParseCredentialStoreError, @@ -164,18 +164,19 @@ pub use middleware::{ }; pub use module::{CommandModule, Module, ModuleContext, ModuleRegister, build_module_group}; pub use output::{ - Alignment, Envelope, ErrorEnvelope, FieldInfo, HumanViewDef, HumanViewFn, HumanViewRegistry, - HumanViewRenderer, Metadata, NextAction, NextActionParam, OutputField, OutputFormat, - OutputSchema, PaginationMeta, PipelineOpts, RendererFactory, SchemaInfo, SchemaRegistry, - TableColumn, apply_pipeline, build_detailed_error_envelope, build_error_envelope, fields_for, - fields_from_json_schema, filter_fields, format_help_section, get_global_schema_by_path, - global_human_view_registry_snapshot, global_schema_registry_snapshot, is_valid_output_format, - json_schema_for, json_schema_info, lookup_global_human_view_columns, - lookup_global_human_view_func, parse_fields, preview_human_view, register_global_human_view, - register_global_human_view_func, register_global_json_schema, register_global_schema, - register_global_schema_fields, register_global_schema_info, render, render_data, - render_data_format, render_detailed_error, render_detailed_error_format, render_error, - render_error_format, render_format, render_human, render_json, render_toon, write_render, + Alignment, CursorMeta, Envelope, ErrorEnvelope, FieldInfo, HumanViewDef, HumanViewFn, + HumanViewRegistry, HumanViewRenderer, Metadata, NextAction, NextActionParam, OutputField, + OutputFormat, OutputSchema, PaginationMeta, PipelineOpts, RendererFactory, SchemaInfo, + SchemaRegistry, TableColumn, apply_pipeline, build_detailed_error_envelope, + build_error_envelope, fields_for, fields_from_json_schema, filter_fields, format_help_section, + get_global_schema_by_path, global_human_view_registry_snapshot, + global_schema_registry_snapshot, is_valid_output_format, json_schema_for, json_schema_info, + lookup_global_human_view_columns, lookup_global_human_view_func, parse_fields, + preview_human_view, register_global_human_view, register_global_human_view_func, + register_global_json_schema, register_global_schema, register_global_schema_fields, + register_global_schema_info, render, render_data, render_data_format, render_detailed_error, + render_detailed_error_format, render_error, render_error_format, render_format, render_human, + render_json, render_toon, write_render, }; pub use search::{SearchDocument, SearchResult}; pub use tier::Tier; diff --git a/cli-engine/src/middleware/mod.rs b/cli-engine/src/middleware/mod.rs index 154dd74..786a528 100644 --- a/cli-engine/src/middleware/mod.rs +++ b/cli-engine/src/middleware/mod.rs @@ -504,6 +504,16 @@ pub struct Middleware { pub limit: i64, /// Client-side page offset. pub offset: i64, + /// Parsed `--limit` for a [`with_cursor`](crate::CommandSpec::with_cursor) + /// command. Unlike `limit`/`offset`, the engine never slices with this + /// itself — a cursor-aware handler reads it back via + /// [`CommandContext::middleware`](crate::command::CommandContext::middleware) + /// to drive its own backend call. + pub cursor_limit: i64, + /// Parsed `--continue` for a + /// [`with_cursor`](crate::CommandSpec::with_cursor) command. `None` means + /// the user passed no `--continue`, i.e. start from the beginning. + pub continue_token: Option, /// User reason passed to authorization and audit. pub reason: String, /// Whether schema rendering was requested. @@ -598,6 +608,16 @@ pub struct MiddlewareRequest<'request> { /// directly (e.g. [`Middleware::run_no_auth`]) instead of through /// [`Cli`](crate::Cli), which is what computes this. pub pagination_command: Option, + /// The invoked command replayed as `--flag value` text with + /// `--limit`/`--continue` omitted. + /// + /// `Some` only for a command that opted into `with_cursor`; the engine + /// appends `--limit`/`--continue` for the next page and surfaces it as a + /// `next_actions` entry when the handler reported more data via + /// [`CommandResult::with_cursor`](crate::CommandResult::with_cursor). + /// Mutually exclusive with `pagination_command` — a command registers + /// one pagination style, not both. + pub cursor_command: Option, } /// Convenience helper for building a JSON object map. diff --git a/cli-engine/src/middleware/run.rs b/cli-engine/src/middleware/run.rs index 771ad9f..1d07f0c 100644 --- a/cli-engine/src/middleware/run.rs +++ b/cli-engine/src/middleware/run.rs @@ -7,11 +7,13 @@ use super::{ MiddlewareRequest, ValueMap, effective_request_system, fallback_system, }; use crate::{ - CommandResult, Credential, Result, + CommandResult, Credential, CursorContinuation, Result, + cli::quote_pagination_value, error::{CliCoreError, exit_code_for_error}, output::{ - Envelope, NextAction, OutputFormat, PipelineOpts, apply_pipeline, build_error_envelope, - is_valid_output_format, render_human_with_registry_selected, unknown_fields_message, + CursorMeta, Envelope, NextAction, OutputFormat, PipelineOpts, apply_pipeline, + build_error_envelope, is_valid_output_format, render_human_with_registry_selected, + unknown_fields_message, }, }; @@ -45,6 +47,7 @@ impl Middleware { auth, raw_output, pagination_command, + cursor_command, } = request; let no_auth = auth.is_none(); let command_system = effective_request_system(system, command_path); @@ -167,6 +170,8 @@ impl Middleware { &args, identity, None, + None, + None, false, ); } @@ -245,6 +250,11 @@ impl Middleware { // always runs, dry-run or not) — must not mis-tag that execution as a // dry-run in the audit trail. let is_dry_run = self.dry_run && meta.handles_dry_run && metadata.dry_run; + // Extracted ahead of `metadata.next_actions` below (a partial move): + // only the handler that made the backend cursor call can supply + // this, so it travels alongside the envelope rather than being + // recomputed. + let cursor_continuation = metadata.cursor.clone(); let outcome = if is_dry_run { "dry-run" } else { "ok" }; self.write_audit(command_path, &args, identity, outcome) .await; @@ -274,6 +284,8 @@ impl Middleware { &args, identity, pagination_command.as_deref(), + cursor_command.as_deref(), + cursor_continuation, raw_output && !is_dry_run, ) } @@ -304,6 +316,7 @@ impl Middleware { auth: AuthRequirement::None, raw_output: false, pagination_command: None, + cursor_command: None, }, async move |_resolver| command().await, ) @@ -401,6 +414,8 @@ impl Middleware { effective_args, identity, None, + None, + None, false, ) .map(Some); @@ -420,6 +435,8 @@ impl Middleware { effective_args: &ValueMap, identity: &str, pagination_command: Option<&str>, + cursor_command: Option<&str>, + cursor_continuation: Option, raw_output: bool, ) -> Result { if !is_valid_output_format(&self.output_format) { @@ -552,6 +569,46 @@ impl Middleware { envelope.pagination = Some(pagination); } } + if let Some(base) = cursor_command + && let Some(data) = &envelope.data + { + let count = data.as_array().map_or(0, |items| items.len() as i64); + let continuation = cursor_continuation.unwrap_or_default(); + let has_more = continuation.continue_from.is_some(); + // A handler that reported an effective limit is telling us its + // `--continue` token is self-sufficient about page size — the + // same fact that makes `cursor.limit` need correcting also + // makes repeating `--limit` in the replay redundant (two numbers + // to keep in sync that are really just one). Omit it in that + // case; keep it when the token is opaque to us (a real + // server-side cursor, e.g. `domain list`'s `pageToken`) and the + // engine has no way to know whether the backend even tolerates a + // different page size on resume. + let effective_limit = continuation.limit.unwrap_or(self.cursor_limit); + if has_more { + let token = continuation.continue_from.as_deref().unwrap_or_default(); + let command = if continuation.limit.is_some() { + format!("{base} --continue {}", quote_pagination_value(token)) + } else { + format!( + "{base} --limit {effective_limit} --continue {}", + quote_pagination_value(token) + ) + }; + envelope.next_actions.push(NextAction::new( + command, + next_page_description(continuation.total, continuation.remaining), + )); + } + envelope.cursor = Some(CursorMeta { + limit: effective_limit, + count, + total: continuation.total, + remaining: continuation.remaining, + continue_from: continuation.continue_from, + has_more, + }); + } envelope.with_context( command_path, &self.env, @@ -606,3 +663,16 @@ impl Middleware { }) } } + +/// Builds the human-readable description for a cursor "next page" +/// [`NextAction`], surfacing whatever the backend reported. +fn next_page_description(total: Option, remaining: Option) -> String { + match (remaining, total) { + (Some(remaining), Some(total)) => { + format!("View the next page ({remaining} remaining of {total} total)") + } + (Some(remaining), None) => format!("View the next page ({remaining} remaining)"), + (None, Some(total)) => format!("View the next page (of {total} total)"), + (None, None) => "View the next page".to_owned(), + } +} diff --git a/cli-engine/src/output/envelope.rs b/cli-engine/src/output/envelope.rs index 3bd0d7d..a3defff 100644 --- a/cli-engine/src/output/envelope.rs +++ b/cli-engine/src/output/envelope.rs @@ -22,6 +22,12 @@ pub struct Envelope { /// caller relies on it to know whether more data exists at all. #[serde(skip_serializing_if = "Option::is_none")] pub pagination: Option, + /// Cursor-pagination facts, present whenever a command that opted into + /// [`with_cursor`](crate::CommandSpec::with_cursor) returned array data. + /// Mutually exclusive with [`pagination`](Envelope::pagination): a + /// command registers one style or the other. + #[serde(skip_serializing_if = "Option::is_none")] + pub cursor: Option, /// Optional execution metadata, controlled by `--verbose`. #[serde(skip_serializing_if = "Option::is_none")] pub metadata: Option, @@ -168,6 +174,34 @@ pub struct PaginationMeta { pub has_more: bool, } +/// Cursor-pagination metadata. +/// +/// `limit` and `count` are computed by the engine (the requested page size +/// and the returned array's length); `total`, `remaining`, and +/// `continue_from` come from the handler's +/// [`CursorContinuation`](crate::CursorContinuation), since only it talked to +/// the opaque backend cursor. `total`/`remaining` are `None` when the backend +/// never reports them — a cursor API is not guaranteed to know its own total. +#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] +pub struct CursorMeta { + /// Requested page size. + pub limit: i64, + /// Item count in this response. + pub count: i64, + /// Total item count, when the backend reports one. + #[serde(skip_serializing_if = "Option::is_none")] + pub total: Option, + /// Remaining item count, when the backend reports one. + #[serde(skip_serializing_if = "Option::is_none")] + pub remaining: Option, + /// Opaque token for the next page. `None` means iteration has reached + /// its end. + #[serde(skip_serializing_if = "Option::is_none")] + pub continue_from: Option, + /// Whether more data is available (`continue_from.is_some()`). + pub has_more: bool, +} + /// Structured error payload in an [`Envelope`]. #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] pub struct ErrorEnvelope { @@ -194,6 +228,7 @@ impl Envelope { Self { data, pagination: None, + cursor: None, metadata: Some(Metadata::new(system)), error: None, warnings: Vec::new(), @@ -214,6 +249,7 @@ impl Envelope { Self { data: None, pagination: None, + cursor: None, metadata: Some(Metadata::new(system.clone())), error: Some(ErrorEnvelope { code: code.into(), @@ -241,6 +277,7 @@ impl Envelope { Self { data: None, pagination: None, + cursor: None, metadata: Some(Metadata { request_id: request_id.clone(), ..Metadata::new(system.clone()) @@ -390,6 +427,7 @@ pub fn build_error_envelope(err: &(dyn std::error::Error + 'static), system: &st return Envelope { data: None, pagination: None, + cursor: None, metadata: Some(Metadata { request_id: request_id.clone(), ..Metadata::new(sys.clone()) @@ -521,6 +559,7 @@ pub fn build_detailed_error_envelope(err: &dyn DetailedError, system: &str) -> E Envelope { data: None, pagination: None, + cursor: None, metadata: Some(Metadata { request_id: request_id.clone(), ..Metadata::new(sys.clone()) diff --git a/cli-engine/src/output/human/body.rs b/cli-engine/src/output/human/body.rs index bfcf0fd..57a59fb 100644 --- a/cli-engine/src/output/human/body.rs +++ b/cli-engine/src/output/human/body.rs @@ -3,12 +3,13 @@ use serde_json::Value; use super::columns::{ column_is_all_numeric, columns_fitting_width, dynamic_columns, fit_column_widths, }; +use super::footer::{SummaryStyle, cursor_summary_text, pagination_summary_text}; use super::value_format::{ format_plain_value, format_value, indent_block, is_nestable, resolve_field_parent, resolve_field_path, resolve_nested_pagination, truncate, }; use super::{Alignment, RenderNotes, TableColumn}; -use crate::output::PaginationMeta; +use crate::output::{CursorMeta, PaginationMeta}; /// Upper bound on a `no_truncate` column's width, even though it otherwise /// skips the normal 40-char cap. Prevents a pathologically long field value @@ -37,6 +38,7 @@ pub(super) fn render_data_body( available_width: usize, pagination: Option<&PaginationMeta>, fields_explicit: bool, + cursor: Option<&CursorMeta>, ) -> (String, RenderNotes) { if let Some(columns) = columns { return match data { @@ -46,6 +48,7 @@ pub(super) fn render_data_body( available_width, pagination, fields_explicit, + cursor, ), Value::Object(map) => render_object_with_columns(map, columns, available_width), Value::Null | Value::Bool(_) | Value::Number(_) | Value::String(_) => { @@ -54,9 +57,14 @@ pub(super) fn render_data_body( }; } match data { - Value::Array(items) => { - render_array(items, fields, available_width, pagination, fields_explicit) - } + Value::Array(items) => render_array( + items, + fields, + available_width, + pagination, + fields_explicit, + cursor, + ), Value::Object(map) => { let columns = dynamic_columns(fields, || map.keys().cloned().collect()); render_object_with_columns(map, &columns, available_width) @@ -74,6 +82,7 @@ pub(crate) fn render_array_with_columns( available_width: usize, pagination: Option<&PaginationMeta>, fields_explicit: bool, + cursor: Option<&CursorMeta>, ) -> (String, RenderNotes) { if items.is_empty() || columns.is_empty() { // Empty columns happens when every item is `{}` (the no-view @@ -204,6 +213,7 @@ pub(crate) fn render_array_with_columns( .collect::>(), &rows, pagination, + cursor, ); ( table, @@ -211,7 +221,7 @@ pub(crate) fn render_array_with_columns( truncated, hidden_columns, nested_narrowing: false, - pagination_shown: pagination.is_some(), + pagination_shown: pagination.is_some() || cursor.is_some(), }, ) } @@ -248,6 +258,7 @@ pub(crate) fn render_object_with_columns( nested_columns, child_width, nested_pagination.as_ref(), + None, ); out.push_str(&indent_block(&block, NESTED_INDENT)); if child_notes.truncated @@ -279,6 +290,7 @@ pub(crate) fn render_array( available_width: usize, pagination: Option<&PaginationMeta>, fields_explicit: bool, + cursor: Option<&CursorMeta>, ) -> (String, RenderNotes) { if items.is_empty() { return ("(no results)\n".to_owned(), RenderNotes::default()); @@ -305,6 +317,7 @@ pub(crate) fn render_array( available_width, pagination, fields_explicit, + cursor, ) } @@ -332,6 +345,7 @@ fn render_table( alignments: &[Alignment], rows: &[Vec], pagination: Option<&PaginationMeta>, + cursor: Option<&CursorMeta>, ) -> String { let mut out = String::new(); for (index, header) in headers.iter().enumerate() { @@ -365,22 +379,25 @@ fn render_table( } out.push('\n'); } - // Merge the pagination facts into this footer rather than letting - // `append_pagination_summary` print a second, redundant line right below - // it — both would otherwise state the same shown/total count. The shown - // count comes from `rows.len()`, not `pagination.count`: a later - // pipeline step (`--expr`) can still reshape `envelope.data` after - // pagination ran, so `rows.len()` is what's actually rendered above, - // while `total`/`offset`/`limit` stay pagination's own facts. - match pagination { - Some(pagination) => out.push_str(&format!( - "\n({} of {} rows, offset {}, limit {})\n", - rows.len(), - pagination.total, - pagination.offset, - pagination.limit + // Merge the pagination/cursor facts into this footer rather than letting + // `append_pagination_summary`/`append_cursor_summary` print a second, + // redundant line right below it — both would otherwise state the same + // shown/total count. The shown count comes from `rows.len()`, not + // `pagination.count`: a later pipeline step (`--expr`) can still reshape + // `envelope.data` after pagination ran, so `rows.len()` is what's + // actually rendered above, while `total`/`offset`/`limit` stay + // pagination's own facts. `pagination` and `cursor` are mutually + // exclusive per command, so at most one arm below ever fires. + match (pagination, cursor) { + (Some(pagination), _) => out.push_str(&format!( + "\n({})\n", + pagination_summary_text(SummaryStyle::TableFooter, rows.len(), pagination) + )), + (None, Some(cursor)) => out.push_str(&format!( + "\n({})\n", + cursor_summary_text(SummaryStyle::TableFooter, rows.len(), cursor) )), - None => out.push_str(&format!("\n({} rows)\n", rows.len())), + (None, None) => out.push_str(&format!("\n({} rows)\n", rows.len())), } out } @@ -395,15 +412,21 @@ fn render_nested_value( nested_columns: &[TableColumn], available_width: usize, pagination: Option<&PaginationMeta>, + cursor: Option<&CursorMeta>, ) -> (String, RenderNotes) { match value { // A nested block's columns are authored by the view, never by a // top-level `--fields` selection (which only reaches top-level // declared columns), so width-based dropping here always honors // each column's `essential` flag rather than being disabled wholesale. - Value::Array(items) => { - render_array_with_columns(items, nested_columns, available_width, pagination, false) - } + Value::Array(items) => render_array_with_columns( + items, + nested_columns, + available_width, + pagination, + false, + cursor, + ), Value::Object(map) => render_object_with_columns(map, nested_columns, available_width), other => (format!("{}\n", format_value(other)), RenderNotes::default()), } diff --git a/cli-engine/src/output/human/footer.rs b/cli-engine/src/output/human/footer.rs index 7a5020d..0e5ce17 100644 --- a/cli-engine/src/output/human/footer.rs +++ b/cli-engine/src/output/human/footer.rs @@ -1,7 +1,7 @@ use std::{borrow::Cow, collections::HashMap}; use super::RenderNotes; -use crate::output::{NextAction, NextActionParam, PaginationMeta}; +use crate::output::{CursorMeta, NextAction, NextActionParam, PaginationMeta}; /// Appends footer hints for truncated cells and/or hidden columns to `out` /// (a no-op when neither happened). Mirrors `append_next_actions`: writes @@ -44,6 +44,74 @@ pub(super) fn append_render_notes(out: &mut String, notes: &RenderNotes) { } } +/// Where a pagination/cursor summary clause is being rendered. The two +/// contexts always describe the same underlying facts for the same case — +/// only the wording differs, between a compact parenthetical merged into a +/// table's row-count footer and a full standalone sentence for output that +/// didn't render as a table. [`pagination_summary_text`]/ +/// [`cursor_summary_text`] hold both wordings for every case side by side in +/// one place, so [`super::body::render_table`]'s footer and +/// [`append_pagination_summary`]/[`append_cursor_summary`] can't drift apart +/// from each other the way two independent `format!` call sites could. +#[derive(Clone, Copy)] +pub(super) enum SummaryStyle { + /// Merged into a table's row-count footer, e.g. `N of M rows`. + TableFooter, + /// A standalone sentence, e.g. `Showing N of M`. + Standalone, +} + +/// Builds the offset-pagination summary clause for `count` shown items, +/// worded for `style`. `count` takes anything `Display`s so callers can pass +/// either a `usize` row count (`render_table`) or an `i64` shown count +/// (`append_pagination_summary`) without a cast. +pub(super) fn pagination_summary_text( + style: SummaryStyle, + count: impl std::fmt::Display, + pagination: &PaginationMeta, +) -> String { + match style { + SummaryStyle::TableFooter => format!( + "{count} of {} rows, offset {}, limit {}", + pagination.total, pagination.offset, pagination.limit + ), + SummaryStyle::Standalone => format!( + "Showing {count} of {} (offset {}, limit {})", + pagination.total, pagination.offset, pagination.limit + ), + } +} + +/// Builds the cursor-pagination summary clause for `count` shown items, +/// worded for `style`. Unlike offset pagination, a `total` is not +/// guaranteed — a pure opaque cursor may never report one — so this falls +/// back to a "so far" phrasing naming the resume token, or a bare count when +/// the backend reported nothing at all. +pub(super) fn cursor_summary_text( + style: SummaryStyle, + count: impl std::fmt::Display, + cursor: &CursorMeta, +) -> String { + match (style, cursor.total, cursor.remaining, &cursor.continue_from) { + (SummaryStyle::TableFooter, Some(total), _, _) => format!("{count} of {total} rows"), + (SummaryStyle::TableFooter, None, Some(remaining), _) => { + format!("{count} rows, {remaining} remaining") + } + (SummaryStyle::TableFooter, None, None, Some(token)) => { + format!("{count} rows so far; use --continue {token} for more") + } + (SummaryStyle::TableFooter, None, None, None) => format!("{count} rows"), + (SummaryStyle::Standalone, Some(total), _, _) => format!("Showing {count} of {total}"), + (SummaryStyle::Standalone, None, Some(remaining), _) => { + format!("Showing {count} ({remaining} remaining)") + } + (SummaryStyle::Standalone, None, None, Some(token)) => { + format!("Showing {count} items so far; use --continue {token} for more") + } + (SummaryStyle::Standalone, None, None, None) => format!("Showing {count}"), + } +} + /// Appends a one-line pagination summary to `out` (a no-op when the response /// wasn't paginated). Unlike `next_actions`, this always shows the underlying /// facts even on the last page, where there's no follow-up command to @@ -76,8 +144,8 @@ pub(super) fn append_pagination_summary( }; match shown { Some(count) => out.push_str(&format!( - "\nShowing {count} of {} (offset {}, limit {})\n", - pagination.total, pagination.offset, pagination.limit + "\n{}\n", + pagination_summary_text(SummaryStyle::Standalone, count, pagination) )), None => out.push_str(&format!( "\n(pagination: {} total, offset {}, limit {})\n", @@ -86,6 +154,27 @@ pub(super) fn append_pagination_summary( } } +/// Appends a one-line cursor-pagination summary to `out` (a no-op when the +/// response wasn't cursor-paginated). The cursor counterpart of +/// [`append_pagination_summary`] — same fallback role (only fires when +/// `render_table`'s footer didn't already merge these facts), same `shown` +/// semantics (the actual post-`--expr` rendered count, not the possibly-stale +/// `cursor.count`). +pub(super) fn append_cursor_summary( + out: &mut String, + cursor: Option<&CursorMeta>, + shown: Option, +) { + let Some(cursor) = cursor else { + return; + }; + let count = shown.unwrap_or(cursor.count); + out.push_str(&format!( + "\n{}\n", + cursor_summary_text(SummaryStyle::Standalone, count, cursor) + )); +} + /// Append a "Next steps:" footer listing suggested follow-up commands to `out` /// (a no-op when there are none). Each action shows its command template with /// any known param values substituted into their `` (params diff --git a/cli-engine/src/output/human/mod.rs b/cli-engine/src/output/human/mod.rs index 3a3d854..a843b14 100644 --- a/cli-engine/src/output/human/mod.rs +++ b/cli-engine/src/output/human/mod.rs @@ -16,7 +16,9 @@ mod tests; mod value_format; use body::render_data_body; -use footer::{append_next_actions, append_pagination_summary, append_render_notes}; +use footer::{ + append_cursor_summary, append_next_actions, append_pagination_summary, append_render_notes, +}; pub(crate) use columns::terminal_width; @@ -446,6 +448,7 @@ pub(crate) fn render_human_with_view( available_width, envelope.pagination.as_ref(), fields_explicit, + envelope.cursor.as_ref(), ), }; // Footers are appended in place: the common no-footer path leaves `body` @@ -464,6 +467,7 @@ pub(crate) fn render_human_with_view( .and_then(Value::as_array) .and_then(|items| i64::try_from(items.len()).ok()); append_pagination_summary(&mut body, envelope.pagination.as_ref(), shown); + append_cursor_summary(&mut body, envelope.cursor.as_ref(), shown); } append_next_actions(&mut body, &envelope.next_actions); body @@ -489,9 +493,10 @@ pub(crate) struct RenderNotes { /// as a fix when this is set, even though `hidden_columns`/`truncated` /// are otherwise reported identically either way. pub(crate) nested_narrowing: bool, - /// Whether the table footer already merged in the pagination summary - /// (`render_table`'s `(N of M rows, offset O, limit L)` line) — so - /// [`render_human_with_view`] doesn't also append the standalone - /// `append_pagination_summary` line and duplicate the same facts. + /// Whether the table footer already merged in the pagination or cursor + /// summary (`render_table`'s `(N of M rows, offset O, limit L)` line, or + /// its cursor-flavored counterpart) — so [`render_human_with_view`] + /// doesn't also append the standalone `append_pagination_summary`/ + /// `append_cursor_summary` line and duplicate the same facts. pub(crate) pagination_shown: bool, } diff --git a/cli-engine/src/output/human/tests/alignment.rs b/cli-engine/src/output/human/tests/alignment.rs index 73c04ae..08f3722 100644 --- a/cli-engine/src/output/human/tests/alignment.rs +++ b/cli-engine/src/output/human/tests/alignment.rs @@ -14,7 +14,7 @@ fn right_aligned_column_pads_header_and_cells_on_the_left() { TableColumn::new("price", "Price").align(Alignment::Right), ]; - let (out, _notes) = render_array_with_columns(&items, &columns, 80, None, false); + let (out, _notes) = render_array_with_columns(&items, &columns, 80, None, false, None); let mut lines = out.lines(); let header_line = lines.next().expect("header line"); let row_lines: Vec<&str> = lines.skip(1).take(2).collect(); @@ -33,7 +33,7 @@ fn column_alignment_defaults_to_left() { let items = vec![json!({ "name": "a" }), json!({ "name": "bb" })]; let columns = vec![TableColumn::new("name", "Name")]; - let (out, _notes) = render_array_with_columns(&items, &columns, 80, None, false); + let (out, _notes) = render_array_with_columns(&items, &columns, 80, None, false, None); let mut lines = out.lines(); let header_line = lines.next().expect("header line"); @@ -50,7 +50,7 @@ fn no_view_array_rendering_right_aligns_a_column_that_is_numeric_on_every_row() json!({ "name": "bigger", "count": 42 }), ]; - let (out, _notes) = render_array(&items, "name,count", 80, None, false); + let (out, _notes) = render_array(&items, "name,count", 80, None, false, None); let mut lines = out.lines(); let header_line = lines.next().expect("header line"); let row_lines: Vec<&str> = lines.skip(1).take(2).collect(); @@ -68,7 +68,7 @@ fn no_view_array_rendering_keeps_a_mixed_type_column_left_aligned() { // matching how right-aligning it would look ragged next to text. let items = vec![json!({ "code": 1 }), json!({ "code": "default" })]; - let (out, _notes) = render_array(&items, "", 80, None, false); + let (out, _notes) = render_array(&items, "", 80, None, false, None); let header_line = out.lines().next().expect("header line"); assert!(header_line.starts_with("CODE"), "{header_line}"); @@ -80,7 +80,7 @@ fn no_view_array_rendering_keeps_an_all_null_column_left_aligned() { // signal to right-align on. let items = vec![json!({ "note": null }), json!({ "note": null })]; - let (out, _notes) = render_array(&items, "", 80, None, false); + let (out, _notes) = render_array(&items, "", 80, None, false, None); let header_line = out.lines().next().expect("header line"); assert!(header_line.starts_with("NOTE"), "{header_line}"); diff --git a/cli-engine/src/output/human/tests/width_fitting.rs b/cli-engine/src/output/human/tests/width_fitting.rs index e5ef2aa..a74cef7 100644 --- a/cli-engine/src/output/human/tests/width_fitting.rs +++ b/cli-engine/src/output/human/tests/width_fitting.rs @@ -21,7 +21,7 @@ fn no_truncate_column_keeps_long_values_intact() { TableColumn::new("title", "Title"), ]; - let (out, notes) = render_array_with_columns(&items, &columns, 80, None, false); + let (out, notes) = render_array_with_columns(&items, &columns, 80, None, false, None); assert!( out.contains(long_url), @@ -44,7 +44,7 @@ fn no_truncate_column_still_caps_pathologically_long_values() { let items = vec![json!({ "url": huge_value })]; let columns = vec![TableColumn::new("url", "URL").no_truncate(true)]; - let (out, _notes) = render_array_with_columns(&items, &columns, 80, None, false); + let (out, _notes) = render_array_with_columns(&items, &columns, 80, None, false, None); assert!( out.contains("..."), @@ -64,7 +64,7 @@ fn column_width_never_shrinks_below_a_long_header() { // Deliberately far narrower than the header: the header must still // render in full even though the row ends up wider than the terminal. - let (out, _notes) = render_array_with_columns(&items, &columns, 10, None, false); + let (out, _notes) = render_array_with_columns(&items, &columns, 10, None, false, None); let header_line = out.lines().next().expect("header line"); let separator_line = out.lines().nth(1).expect("separator line"); @@ -89,7 +89,7 @@ fn wide_terminal_shows_full_values_without_truncation() { TableColumn::new("description", "Description"), ]; - let (out, notes) = render_array_with_columns(&items, &columns, 200, None, false); + let (out, notes) = render_array_with_columns(&items, &columns, 200, None, false, None); assert!( !notes.truncated, @@ -110,7 +110,7 @@ fn narrow_terminal_truncates_and_reports_it() { let items = vec![json!({ "description": description })]; let columns = vec![TableColumn::new("description", "Description")]; - let (out, notes) = render_array_with_columns(&items, &columns, 20, None, false); + let (out, notes) = render_array_with_columns(&items, &columns, 20, None, false, None); assert!( notes.truncated, @@ -136,7 +136,7 @@ fn narrow_terminal_hides_columns_before_truncating_any_of_the_survivors() { TableColumn::new("c", "C"), ]; - let (out, notes) = render_array_with_columns(&items, &columns, 10, None, false); + let (out, notes) = render_array_with_columns(&items, &columns, 10, None, false, None); assert!( !notes.truncated, @@ -165,7 +165,7 @@ fn overflow_hides_lowest_priority_columns_first() { TableColumn::new("created_at", "Created At"), ]; - let (out, notes) = render_array_with_columns(&items, &columns, 10, None, false); + let (out, notes) = render_array_with_columns(&items, &columns, 10, None, false, None); assert_eq!( notes.hidden_columns, @@ -199,7 +199,7 @@ fn essential_column_survives_even_when_a_higher_priority_column_is_dropped() { TableColumn::new("data", "Data").essential(true), ]; - let (out, notes) = render_array_with_columns(&items, &columns, 16, None, false); + let (out, notes) = render_array_with_columns(&items, &columns, 16, None, false, None); assert_eq!( notes.hidden_columns, @@ -226,7 +226,7 @@ fn essential_columns_overflow_instead_of_hidden_or_truncated_when_the_terminal_i TableColumn::new("c", "C").essential(true), ]; - let (out, notes) = render_array_with_columns(&items, &columns, 10, None, false); + let (out, notes) = render_array_with_columns(&items, &columns, 10, None, false, None); assert!( notes.hidden_columns.is_empty(), @@ -269,7 +269,7 @@ fn explicit_fields_selection_disables_column_hiding_entirely() { TableColumn::new("created_at", "Created At"), ]; - let (out, notes) = render_array_with_columns(&items, &columns, 10, None, true); + let (out, notes) = render_array_with_columns(&items, &columns, 10, None, true, None); assert!( notes.hidden_columns.is_empty(), @@ -296,7 +296,7 @@ fn explicit_fields_selection_disables_truncation_even_when_a_value_outgrows_its_ let items = vec![json!({ "description": description })]; let columns = vec![TableColumn::new("description", "Description")]; - let (out, notes) = render_array_with_columns(&items, &columns, 20, None, true); + let (out, notes) = render_array_with_columns(&items, &columns, 20, None, true, None); assert!(!notes.truncated, "{out}"); assert!(notes.hidden_columns.is_empty(), "{out}"); @@ -379,7 +379,7 @@ fn overflow_hiding_accounts_for_no_truncate_columns_true_width() { // Exactly enough room for the URL alone (40 chars), not enough for // the URL plus even a 1-char trailing column and its gutter (43). - let (out, notes) = render_array_with_columns(&items, &columns, 42, None, false); + let (out, notes) = render_array_with_columns(&items, &columns, 42, None, false, None); assert_eq!( notes.hidden_columns, @@ -399,7 +399,7 @@ fn render_array_with_columns_handles_no_columns_gracefully() { // build a table from, so this must report "no results" rather than // a blank header/rows table. let items = vec![json!({ "a": "1" })]; - let (out, notes) = render_array_with_columns(&items, &[], 80, None, false); + let (out, notes) = render_array_with_columns(&items, &[], 80, None, false, None); assert_eq!(out, "(no results)\n"); assert!(!notes.truncated, "{out}"); @@ -427,7 +427,7 @@ fn no_view_array_of_empty_objects_reports_no_results() { // keys to derive columns from — same "no columns" case as above, // reached through the no-view path instead. let items = vec![json!({}), json!({})]; - let (out, notes) = render_array(&items, "", 80, None, false); + let (out, notes) = render_array(&items, "", 80, None, false, None); assert_eq!(out, "(no results)\n"); assert!(notes.hidden_columns.is_empty(), "{out}"); diff --git a/cli-engine/src/output/mod.rs b/cli-engine/src/output/mod.rs index 8893628..1d540d2c 100644 --- a/cli-engine/src/output/mod.rs +++ b/cli-engine/src/output/mod.rs @@ -19,7 +19,7 @@ mod toon; pub use crate::error::{DetailedError, ExitCoder, exit_code_for_error, exit_code_for_exit_coder}; pub use envelope::{ - Envelope, ErrorEnvelope, Metadata, NextAction, NextActionParam, PaginationMeta, + CursorMeta, Envelope, ErrorEnvelope, Metadata, NextAction, NextActionParam, PaginationMeta, build_detailed_error_envelope, build_error_envelope, }; pub(crate) use fields::project_fields; diff --git a/cli-engine/tests/cursor_pagination.rs b/cli-engine/tests/cursor_pagination.rs new file mode 100644 index 0000000..de6366b --- /dev/null +++ b/cli-engine/tests/cursor_pagination.rs @@ -0,0 +1,565 @@ +//! End-to-end coverage for opt-in cursor pagination +//! (`CommandSpec::with_cursor`), driven through `Cli::run` the way a real +//! consumer binary would. +//! +//! `--limit`/`--continue` are deliberately not framework-global: a command +//! only gets them — in `--help` and on its command line — by declaring a +//! `CursorConfig`. Unlike offset pagination (`tests/pagination.rs`), the +//! engine never slices or measures a cursor itself: these tests drive a fake +//! in-memory "backend" through the handler, which reads back the parsed +//! `--limit`/`--continue` off `ctx.middleware` and reports what it learned +//! via `CommandResult::with_cursor`. + +use clap::Arg; +use cli_engine::{ + Cli, CliConfig, CommandResult, CommandSpec, CursorConfig, CursorContinuation, + RuntimeCommandSpec, +}; +use serde_json::json; + +fn items() -> Vec { + vec![ + json!({"name": "alpha"}), + json!({"name": "beta"}), + json!({"name": "gamma"}), + json!({"name": "delta"}), + ] +} + +/// A fake cursor-backed handler: `--continue` is the next start index +/// (as a string), `--limit` is the page size. Reports a fresh continuation +/// token whenever more items remain, mirroring how a real handler would +/// resume an opaque backend cursor. +fn cli_with_cursor_list_command(spec: CommandSpec) -> Cli { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new_with_context(spec, async |ctx| { + let all = items(); + let start = ctx + .middleware + .continue_token + .as_deref() + .and_then(|token| token.parse::().ok()) + .unwrap_or(0); + let limit = usize::try_from(ctx.middleware.cursor_limit).unwrap_or(0); + let end = start.saturating_add(limit).min(all.len()); + let page = all.get(start..end).unwrap_or_default().to_vec(); + let mut result = CommandResult::new(json!(page)); + if end < all.len() { + result = result.with_cursor(CursorContinuation::more(end.to_string())); + } + Ok(result) + })); + cli +} + +#[tokio::test] +async fn limit_and_continue_are_unknown_arguments_for_a_command_that_did_not_opt_in() { + let cli = cli_with_cursor_list_command(CommandSpec::new("list", "List things").no_auth(true)); + + let output = cli.run(["my-cli", "list", "--limit", "1"]).await; + assert_eq!( + output.exit_code, 2, + "unopted command should reject --limit as unknown: {}", + output.rendered + ); + + let output = cli.run(["my-cli", "list", "--continue", "1"]).await; + assert_eq!( + output.exit_code, 2, + "unopted command should reject --continue as unknown: {}", + output.rendered + ); + + let help = cli.run(["my-cli", "list", "--help"]).await; + assert!( + !help.rendered.contains("--limit") && !help.rendered.contains("--continue"), + "unopted command's --help should not mention cursor flags: {}", + help.rendered + ); +} + +#[tokio::test] +async fn opted_in_command_documents_limit_and_continue_in_help() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 3, + }), + ); + + let help = cli.run(["my-cli", "list", "--help"]).await; + assert!(help.rendered.contains("--limit"), "{}", help.rendered); + assert!(help.rendered.contains("--continue"), "{}", help.rendered); +} + +#[tokio::test] +async fn default_limit_applies_when_neither_flag_is_passed() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + ); + + let output = cli.run(["my-cli", "list", "--output", "json"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!( + rendered["data"], + json!([{"name": "alpha"}, {"name": "beta"}]) + ); + // total/remaining are absent — this fake backend never reports them. + assert_eq!( + rendered["cursor"], + json!({"limit": 2, "count": 2, "continue_from": "2", "has_more": true}) + ); + assert_eq!( + rendered["next_actions"][0]["command"], + "my-cli list --limit 2 --continue 2" + ); + assert!(rendered.get("metadata").is_none(), "{}", output.rendered); +} + +#[tokio::test] +async fn explicit_limit_and_continue_fetch_the_requested_page() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + ); + + let output = cli + .run([ + "my-cli", + "list", + "--continue", + "1", + "--limit", + "2", + "--output", + "json", + ]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!( + rendered["data"], + json!([{"name": "beta"}, {"name": "gamma"}]) + ); + assert_eq!(rendered["cursor"]["continue_from"], "3"); + assert_eq!( + rendered["next_actions"][0]["command"], + "my-cli list --limit 2 --continue 3" + ); +} + +#[tokio::test] +async fn last_page_has_no_next_action_and_has_more_is_false() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + ); + + let output = cli + .run([ + "my-cli", + "list", + "--continue", + "2", + "--limit", + "2", + "--output", + "json", + ]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!(rendered["cursor"]["has_more"], false); + assert!(rendered["cursor"].get("continue_from").is_none()); + assert!( + rendered.get("next_actions").is_none(), + "no next page exists: {}", + output.rendered + ); +} + +#[tokio::test] +async fn with_total_and_remaining_surface_on_the_envelope() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + async |_credential, _args| { + Ok( + CommandResult::new(json!([{"name": "alpha"}, {"name": "beta"}])).with_cursor( + CursorContinuation::more("tok-2") + .with_total(4) + .with_remaining(2), + ), + ) + }, + )); + + let output = cli.run(["my-cli", "list", "--output", "json"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!( + rendered["cursor"], + json!({ + "limit": 2, + "count": 2, + "total": 4, + "remaining": 2, + "continue_from": "tok-2", + "has_more": true + }) + ); +} + +/// A handler that derives its own effective page size from the `--continue` +/// token (e.g. to let a caller resume with `--continue` alone, without +/// repeating `--limit`) reports that via `CursorContinuation::with_limit`. +/// The envelope's `cursor.limit` reflects that effective size, not the +/// parsed `--limit` this particular invocation happened to carry — and the +/// suggested next-page command omits `--limit` entirely, since `with_limit` +/// also signals that the token is self-sufficient about size. +#[tokio::test] +async fn with_limit_overrides_the_envelope_and_omits_limit_from_the_next_action() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 25, + max_limit: 0, + }), + async |_credential, _args| { + Ok( + CommandResult::new(json!([{"name": "alpha"}, {"name": "beta"}])) + .with_cursor(CursorContinuation::more("tok-2").with_limit(2)), + ) + }, + )); + + // No --limit passed at all — the parsed value defaults to 25, but the + // handler says it actually applied 2 (inherited from a prior token). + let output = cli.run(["my-cli", "list", "--output", "json"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!(rendered["cursor"]["limit"], json!(2)); + let next_actions = rendered["next_actions"].as_array().expect("next_actions"); + // `with_limit` means the token is self-sufficient about page size, so + // the replay omits `--limit` entirely rather than repeating a value + // the token already carries (and that would be wrong here anyway — + // the parsed 25, not the effective 2). + assert_eq!( + next_actions[0]["command"], "my-cli list --continue tok-2", + "next-page command should omit --limit, not repeat the parsed default (25): {}", + output.rendered + ); +} + +#[tokio::test] +async fn max_limit_rejects_an_explicit_limit_above_the_cap_but_allows_the_cap_itself() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 1, + max_limit: 3, + }), + ); + + let output = cli.run(["my-cli", "list", "--limit", "4"]).await; + assert_eq!( + output.exit_code, 2, + "--limit above max_limit should be a usage error: {}", + output.rendered + ); + + let output = cli + .run(["my-cli", "list", "--limit", "3", "--output", "json"]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); +} + +#[tokio::test] +async fn zero_and_negative_limit_are_rejected_at_parse_time() { + // Unlike offset pagination, a cursor's `--limit` has no "0/negative means + // unlimited" reading — it's a per-request page size sent to a backend, + // not a bound on data the framework already holds. + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 1, + max_limit: 0, + }), + ); + + let output = cli.run(["my-cli", "list", "--limit", "0"]).await; + assert_eq!( + output.exit_code, 2, + "--limit 0 should be a usage error: {}", + output.rendered + ); + + let output = cli.run(["my-cli", "list", "--limit", "-1"]).await; + assert_eq!( + output.exit_code, 2, + "negative --limit should be a usage error: {}", + output.rendered + ); +} + +/// A command author setting `default_limit` to `0` (or negative) can never +/// satisfy an unset `--limit` with a valid page size; caught at registration +/// time as a development-time safety net, same idiom as +/// `with_pagination`'s `default_limit > max_limit` debug_assert. +#[test] +#[cfg_attr(debug_assertions, should_panic(expected = "greater than zero"))] +fn with_cursor_panics_when_default_limit_is_not_positive() { + let _unused = CommandSpec::new("list", "List things").with_cursor(CursorConfig { + default_limit: 0, + max_limit: 5, + }); +} + +#[test] +#[cfg_attr( + debug_assertions, + should_panic(expected = "greater than its max_limit") +)] +fn with_cursor_panics_when_default_limit_exceeds_max_limit() { + let _unused = CommandSpec::new("list", "List things").with_cursor(CursorConfig { + default_limit: 10, + max_limit: 5, + }); +} + +/// A command picks one pagination style, not both; caught at registration +/// time (inside `Cli::add_command`'s clap-tree build), not left as a silent +/// "cursor wins" or "offset wins" resolution. +#[test] +#[cfg_attr( + debug_assertions, + should_panic(expected = "picks one pagination style") +)] +fn with_pagination_and_with_cursor_together_panics_on_registration() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("bad", "Bad") + .no_auth(true) + .with_pagination(cli_engine::PaginationConfig::default()) + .with_cursor(CursorConfig { + default_limit: 1, + max_limit: 0, + }), + async |_credential, _args| Ok(CommandResult::new(json!([]))), + )); +} + +/// Same footgun as `raw_output_paired_with_pagination_panics_on_registration` +/// in `tests/foundation.rs`, for the cursor flavor: a single verbatim string +/// has no pages either. +#[test] +#[cfg_attr(debug_assertions, should_panic(expected = "mutually exclusive"))] +fn raw_output_paired_with_cursor_panics_on_registration() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("bad", "Bad") + .no_auth(true) + .raw_output(true) + .with_cursor(CursorConfig { + default_limit: 1, + max_limit: 0, + }), + async |_credential, _args| Ok(CommandResult::new(json!("text"))), + )); +} + +#[tokio::test] +async fn next_page_action_replays_other_flags_the_user_passed() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_arg(Arg::new("status").long("status")) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + ); + + let output = cli + .run(["my-cli", "list", "--status", "active", "--output", "json"]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!( + rendered["next_actions"][0]["command"], + "my-cli list --status active --limit 2 --continue 2" + ); +} + +#[tokio::test] +async fn next_page_action_quotes_a_continuation_token_with_shell_metacharacters() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + async |_credential, _args| { + Ok(CommandResult::new(json!(items())).with_cursor(CursorContinuation::more("a b;c"))) + }, + )); + + let output = cli.run(["my-cli", "list", "--output", "json"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!( + rendered["next_actions"][0]["command"], + "my-cli list --limit 2 --continue \"a b;c\"" + ); +} + +#[tokio::test] +async fn human_output_shows_so_far_summary_when_total_is_unknown() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + ); + + let output = cli.run(["my-cli", "list", "--output", "human"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + assert!( + output + .rendered + .contains("(2 rows so far; use --continue 2 for more)"), + "{}", + output.rendered + ); + assert!( + output.rendered.contains("Next steps:"), + "{}", + output.rendered + ); + assert!( + output + .rendered + .contains("my-cli list --limit 2 --continue 2"), + "{}", + output.rendered + ); +} + +#[tokio::test] +async fn human_output_shows_total_when_known() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + async |_credential, _args| { + Ok( + CommandResult::new(json!([{"name": "alpha"}, {"name": "beta"}])) + .with_cursor(CursorContinuation::more("2").with_total(4)), + ) + }, + )); + + let output = cli.run(["my-cli", "list", "--output", "human"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + assert!( + output.rendered.contains("(2 of 4 rows)"), + "{}", + output.rendered + ); +} + +#[tokio::test] +async fn human_output_on_last_page_shows_summary_but_no_next_steps() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + ); + + let output = cli + .run([ + "my-cli", + "list", + "--continue", + "2", + "--limit", + "2", + "--output", + "human", + ]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + assert!(output.rendered.contains("(2 rows)"), "{}", output.rendered); + assert!( + !output.rendered.contains("Next steps:"), + "no next page exists: {}", + output.rendered + ); +} + +#[tokio::test] +async fn human_standalone_summary_for_a_non_table_cursor_response() { + // Mirrors `tests/pagination.rs`'s `human_standalone_summary_...` for the + // cursor flavor: a bare array of scalars renders via `render_array_lines`, + // not `render_table`, so the standalone `append_cursor_summary` line is + // the one that must fire, not the merged table footer. + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + async |_credential, _args| { + Ok(CommandResult::new(json!(["alpha", "beta"])) + .with_cursor(CursorContinuation::more("2"))) + }, + )); + + let output = cli.run(["my-cli", "list", "--output", "human"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + assert!( + output + .rendered + .contains("Showing 2 items so far; use --continue 2 for more"), + "{}", + output.rendered + ); +} diff --git a/cli-engine/tests/foundation.rs b/cli-engine/tests/foundation.rs index 8b56417..2b66e26 100644 --- a/cli-engine/tests/foundation.rs +++ b/cli-engine/tests/foundation.rs @@ -64,6 +64,7 @@ fn middleware_request<'request>( auth: auth_requirement(no_auth), raw_output: false, pagination_command: None, + cursor_command: None, } } @@ -91,6 +92,7 @@ fn middleware_request_with_view<'request>( auth: auth_requirement(no_auth), raw_output: false, pagination_command: None, + cursor_command: None, } } @@ -125,6 +127,7 @@ fn middleware_request_with_system<'request>( auth: auth_requirement(no_auth), raw_output: false, pagination_command: None, + cursor_command: None, } } @@ -10915,6 +10918,7 @@ async fn optional_skips_auth_when_handler_ignores_credential() { auth: cli_engine::AuthRequirement::Optional, raw_output: false, pagination_command: None, + cursor_command: None, }, async |_resolver| Ok(CommandResult::new(json!({"ok": true}))), ) @@ -10954,6 +10958,7 @@ async fn optional_swallowed_auth_failure_then_command_error_is_not_auth_error() auth: cli_engine::AuthRequirement::Optional, raw_output: false, pagination_command: None, + cursor_command: None, }, async |resolver: CredentialResolver| { // Best-effort identity; the missing provider makes this fail, and @@ -11006,6 +11011,7 @@ async fn optional_handler_propagated_auth_failure_is_classified_auth_error() { auth: cli_engine::AuthRequirement::Optional, raw_output: false, pagination_command: None, + cursor_command: None, }, async |resolver: CredentialResolver| { resolver.resolve().await?; From d0d6bae174270fcfcbe58d9a57704910b4b791a2 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 10:37:03 -0700 Subject: [PATCH 02/14] fix: address Copilot review findings on cursor pagination - CursorConfig no longer derives Default: 0 is not a valid default_limit (unlike PaginationConfig's "0 = unlimited"), so a release build reaching CursorConfig::default() would register an unusable --limit whose own default value the parser then rejects, with only a stripped debug_assert standing between the two. - Cursor metadata (envelope.cursor + the next-page action) is now only ever attached for array data, mirroring apply_pagination's identical guard for offset pagination. Previously any command result -- including one --expr reshaped into a scalar/object -- got a cursor field with a fabricated count and a next_actions entry over data that was never actually paginated. - append_cursor_summary's fallback for non-array shown data now prints a neutral line instead of "Showing ...", matching append_pagination_summary's existing None-branch handling. - The human-readable "so far" summary line now quotes the resume token the same way the auto-generated next_actions command already does; an opaque token with shell metacharacters was otherwise unusable to copy-paste from the sentence context. - Fixed a docs typo ("aserver-maintained" -> "a server-maintained"). Adds 3 regression tests for the array-data guard and human-summary quoting. --- cli-engine/docs/concepts.md | 2 +- cli-engine/src/command/spec.rs | 7 ++- cli-engine/src/middleware/run.rs | 9 ++- cli-engine/src/output/human/footer.rs | 37 ++++++++--- cli-engine/tests/cursor_pagination.rs | 89 +++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 12 deletions(-) diff --git a/cli-engine/docs/concepts.md b/cli-engine/docs/concepts.md index ee9c829..2f629f3 100644 --- a/cli-engine/docs/concepts.md +++ b/cli-engine/docs/concepts.md @@ -282,7 +282,7 @@ CommandSpec::new("list", "List projects").with_pagination(PaginationConfig { }) ``` -`--limit`/`--continue` are the cursor-pagination counterpart, for a command backed by aserver-maintained, forward-only cursor API — see [cursor pagination](#cursor-pagination): +`--limit`/`--continue` are the cursor-pagination counterpart, for a command backed by a server-maintained, forward-only cursor API — see [cursor pagination](#cursor-pagination): ```rust CommandSpec::new("list", "List domains").with_cursor(CursorConfig { diff --git a/cli-engine/src/command/spec.rs b/cli-engine/src/command/spec.rs index 91bcad0..6e4d2bf 100644 --- a/cli-engine/src/command/spec.rs +++ b/cli-engine/src/command/spec.rs @@ -173,7 +173,10 @@ pub struct PaginationConfig { /// Unlike [`PaginationConfig`], `default_limit` must be greater than zero: /// there is no "unlimited" sentinel here, since `--limit` is a per-request /// page size sent to a backend, not a bound on an already-in-memory -/// collection. +/// collection. Deliberately does not derive `Default` — unlike +/// `PaginationConfig`, where `0` is itself a valid ("unlimited") +/// `default_limit`, there is no valid all-zero `CursorConfig`, so both +/// fields must always be given explicitly. /// /// ``` /// use cli_engine::CursorConfig; @@ -184,7 +187,7 @@ pub struct PaginationConfig { /// }; /// assert_eq!(cursor.default_limit, 25); /// ``` -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct CursorConfig { /// Page size sent to the backend when the user passes no `--limit`. Must /// be greater than zero. diff --git a/cli-engine/src/middleware/run.rs b/cli-engine/src/middleware/run.rs index 1d07f0c..0c9434a 100644 --- a/cli-engine/src/middleware/run.rs +++ b/cli-engine/src/middleware/run.rs @@ -571,8 +571,15 @@ impl Middleware { } if let Some(base) = cursor_command && let Some(data) = &envelope.data + && let Some(items) = data.as_array() { - let count = data.as_array().map_or(0, |items| items.len() as i64); + // Cursor metadata is for array data (per `Envelope::cursor`'s own + // contract) — a handler result that isn't an array (or one + // `--expr` reshaped into a scalar/object) gets no cursor field + // at all, mirroring offset pagination's identical guard in + // `apply_pagination`, rather than advertising a bogus page over + // data that was never actually paginated. + let count = items.len() as i64; let continuation = cursor_continuation.unwrap_or_default(); let has_more = continuation.continue_from.is_some(); // A handler that reported an effective limit is telling us its diff --git a/cli-engine/src/output/human/footer.rs b/cli-engine/src/output/human/footer.rs index 0e5ce17..345ddaf 100644 --- a/cli-engine/src/output/human/footer.rs +++ b/cli-engine/src/output/human/footer.rs @@ -1,6 +1,7 @@ use std::{borrow::Cow, collections::HashMap}; use super::RenderNotes; +use crate::cli::quote_pagination_value; use crate::output::{CursorMeta, NextAction, NextActionParam, PaginationMeta}; /// Appends footer hints for truncated cells and/or hidden columns to `out` @@ -98,7 +99,10 @@ pub(super) fn cursor_summary_text( format!("{count} rows, {remaining} remaining") } (SummaryStyle::TableFooter, None, None, Some(token)) => { - format!("{count} rows so far; use --continue {token} for more") + format!( + "{count} rows so far; use --continue {} for more", + quote_pagination_value(token) + ) } (SummaryStyle::TableFooter, None, None, None) => format!("{count} rows"), (SummaryStyle::Standalone, Some(total), _, _) => format!("Showing {count} of {total}"), @@ -106,7 +110,10 @@ pub(super) fn cursor_summary_text( format!("Showing {count} ({remaining} remaining)") } (SummaryStyle::Standalone, None, None, Some(token)) => { - format!("Showing {count} items so far; use --continue {token} for more") + format!( + "Showing {count} items so far; use --continue {} for more", + quote_pagination_value(token) + ) } (SummaryStyle::Standalone, None, None, None) => format!("Showing {count}"), } @@ -159,7 +166,12 @@ pub(super) fn append_pagination_summary( /// [`append_pagination_summary`] — same fallback role (only fires when /// `render_table`'s footer didn't already merge these facts), same `shown` /// semantics (the actual post-`--expr` rendered count, not the possibly-stale -/// `cursor.count`). +/// `cursor.count`) and the same `None` handling: `--expr` reshaping the data +/// into something that's no longer an array (e.g. `length(@)`) must not +/// print a "Showing N ..." claim built from the now-stale pre-`--expr` +/// `cursor.count` — falling back to `cursor.count` here (rather than a +/// neutral line, as `append_pagination_summary` does) would do exactly +/// that. pub(super) fn append_cursor_summary( out: &mut String, cursor: Option<&CursorMeta>, @@ -168,11 +180,20 @@ pub(super) fn append_cursor_summary( let Some(cursor) = cursor else { return; }; - let count = shown.unwrap_or(cursor.count); - out.push_str(&format!( - "\n{}\n", - cursor_summary_text(SummaryStyle::Standalone, count, cursor) - )); + match shown { + Some(count) => out.push_str(&format!( + "\n{}\n", + cursor_summary_text(SummaryStyle::Standalone, count, cursor) + )), + None => out.push_str(&format!( + "\n(cursor: limit {}{})\n", + cursor.limit, + cursor + .total + .map(|total| format!(", {total} total")) + .unwrap_or_default() + )), + } } /// Append a "Next steps:" footer listing suggested follow-up commands to `out` diff --git a/cli-engine/tests/cursor_pagination.rs b/cli-engine/tests/cursor_pagination.rs index de6366b..93c01c7 100644 --- a/cli-engine/tests/cursor_pagination.rs +++ b/cli-engine/tests/cursor_pagination.rs @@ -231,6 +231,64 @@ async fn with_total_and_remaining_surface_on_the_envelope() { ); } +/// Cursor metadata is for array data (mirrors offset pagination's identical +/// `let Value::Array(items) = data else { return Ok(None) }` guard in +/// `apply_pipeline`'s `apply_pagination`): a handler that calls +/// `with_cursor` but returns a non-array result gets no `cursor` field at +/// all, rather than a bogus page (`count: 0`, but still `has_more`/a +/// `next_actions` entry) over data that was never actually paginated. +#[tokio::test] +async fn cursor_metadata_is_absent_when_the_handler_result_is_not_an_array() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + async |_credential, _args| { + Ok(CommandResult::new(json!({"name": "alpha"})) + .with_cursor(CursorContinuation::more("tok-2"))) + }, + )); + + let output = cli.run(["my-cli", "list", "--output", "json"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert!(rendered.get("cursor").is_none(), "{}", output.rendered); + assert!( + rendered.get("next_actions").is_none(), + "{}", + output.rendered + ); +} + +/// Same guard, reached via `--expr` reshaping an originally-array result into +/// a scalar rather than the handler returning a non-array result directly — +/// `apply_pipeline`'s `--expr` step runs after the cursor block would +/// otherwise see the data, so this exercises the same code path a real +/// `length(@)` query would. +#[tokio::test] +async fn cursor_metadata_is_absent_after_expr_reshapes_data_to_a_scalar() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + ); + + let output = cli + .run(["my-cli", "list", "--expr", "length(@)", "--output", "json"]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!(rendered["data"], json!(2)); + assert!(rendered.get("cursor").is_none(), "{}", output.rendered); +} + /// A handler that derives its own effective page size from the `--continue` /// token (e.g. to let a caller resume with `--continue` alone, without /// repeating `--limit`) reports that via `CursorContinuation::with_limit`. @@ -474,6 +532,37 @@ async fn human_output_shows_so_far_summary_when_total_is_unknown() { ); } +/// The table-footer "so far" line interpolates the resume token directly +/// into a sentence, separately from the `next_actions` command (which +/// already quotes it) — a token containing shell metacharacters needs the +/// same quoting here too, or the printed instruction is unusable/unsafe to +/// copy-paste. +#[tokio::test] +async fn human_output_so_far_summary_quotes_a_continuation_token_with_shell_metacharacters() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 2, + max_limit: 0, + }), + async |_credential, _args| { + Ok(CommandResult::new(json!(items())).with_cursor(CursorContinuation::more("a b;c"))) + }, + )); + + let output = cli.run(["my-cli", "list", "--output", "human"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + assert!( + output + .rendered + .contains("so far; use --continue \"a b;c\" for more"), + "{}", + output.rendered + ); +} + #[tokio::test] async fn human_output_shows_total_when_known() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); From 1d540d2f5e1ac1541077a770d45c7d80e0e9713e Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 11:12:39 -0700 Subject: [PATCH 03/14] fix: mark Middleware/MiddlewareRequest non_exhaustive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both structs already grew fields (cursor_limit, continue_token, cursor_command) once without a compatibility escape hatch. Marking them non_exhaustive means the next such addition doesn't break external construction — but non_exhaustive forbids struct-literal syntax entirely for external callers, even with ..Default::default() spread, so this also adds a MiddlewareRequest::new constructor plus with_* builder methods, and updates foundation.rs's integration-test call sites (which compile as an external crate) to use them instead of struct literals. Co-Authored-By: Claude Sonnet 5 --- cli-engine/src/middleware/mod.rs | 73 ++++++++++++++++++++- cli-engine/tests/foundation.rs | 106 +++++++++++-------------------- 2 files changed, 110 insertions(+), 69 deletions(-) diff --git a/cli-engine/src/middleware/mod.rs b/cli-engine/src/middleware/mod.rs index 786a528..a064d70 100644 --- a/cli-engine/src/middleware/mod.rs +++ b/cli-engine/src/middleware/mod.rs @@ -468,7 +468,13 @@ pub struct ActivityEvent { /// Middleware is intentionally a plain, cloneable struct so tests and command /// handlers can inspect what will be used for a run. Application setup usually /// mutates it through `CliConfig` hooks or `ModuleContext`. +/// +/// `#[non_exhaustive]`: construct via [`Middleware::default`]/[`new`](Middleware::new), +/// then mutate individual fields, so the engine can add fields (as it did +/// for cursor pagination) without breaking an exhaustive external struct +/// literal. #[derive(Clone, Debug, Default)] +#[non_exhaustive] pub struct Middleware { /// Optional authorization provider. pub authz: Option>, @@ -570,7 +576,17 @@ pub struct MiddlewareOutput { } /// Inputs for one middleware-managed command execution. -#[derive(Clone, Debug, PartialEq)] +/// +/// `#[non_exhaustive]`: construct via [`MiddlewareRequest::new`], then chain +/// `with_*` methods for anything beyond the commonly-required fields — never +/// as a struct literal (`#[non_exhaustive]` forbids that entirely for a +/// caller outside this crate, even with `..Default::default()`) — so the +/// engine can add fields (as it did for cursor pagination) without breaking +/// external callers. This is the failure mode this exact struct hit once +/// already, when `cursor_command` was added with no such escape hatch +/// available. +#[derive(Clone, Debug, Default, PartialEq)] +#[non_exhaustive] pub struct MiddlewareRequest<'request> { /// Per-command metadata used by authentication, authorization, dry-run, audit, and activity. pub meta: CommandMeta, @@ -620,6 +636,61 @@ pub struct MiddlewareRequest<'request> { pub cursor_command: Option, } +impl<'request> MiddlewareRequest<'request> { + /// Builds a request from the fields every caller needs to set, with + /// everything else defaulted (`auth: AuthRequirement::Required`, + /// `view_id`/`pagination_command`/`cursor_command`: `None`, `raw_output: + /// false`). Chain the `with_*` methods below for anything else. + pub fn new( + meta: CommandMeta, + command_path: &'request str, + system: &'request str, + user_args: ValueMap, + args: ValueMap, + default_fields: &'request str, + ) -> Self { + Self { + meta, + command_path, + system, + user_args, + args, + default_fields, + ..Self::default() + } + } + + /// Sets the authentication requirement enforced for this command. + pub fn with_auth(mut self, auth: AuthRequirement) -> Self { + self.auth = auth; + self + } + + /// Sets the human view id this command declared. + pub fn with_view_id(mut self, view_id: &'request str) -> Self { + self.view_id = Some(view_id); + self + } + + /// Sets whether a successful string result renders verbatim. + pub fn with_raw_output(mut self, raw_output: bool) -> Self { + self.raw_output = raw_output; + self + } + + /// Sets the replayable command text for offset pagination's `next_actions`. + pub fn with_pagination_command(mut self, pagination_command: impl Into) -> Self { + self.pagination_command = Some(pagination_command.into()); + self + } + + /// Sets the replayable command text for cursor pagination's `next_actions`. + pub fn with_cursor_command(mut self, cursor_command: impl Into) -> Self { + self.cursor_command = Some(cursor_command.into()); + self + } +} + /// Convenience helper for building a JSON object map. #[must_use] pub fn value_map(entries: impl IntoIterator, Value)>) -> ValueMap { diff --git a/cli-engine/tests/foundation.rs b/cli-engine/tests/foundation.rs index 2b66e26..0c96224 100644 --- a/cli-engine/tests/foundation.rs +++ b/cli-engine/tests/foundation.rs @@ -51,21 +51,17 @@ fn middleware_request<'request>( default_fields: &'request str, no_auth: bool, ) -> MiddlewareRequest<'request> { - MiddlewareRequest { + MiddlewareRequest::new( meta, command_path, - system: command_path + command_path .split_once(':') .map_or(command_path, |(system, _)| system), user_args, args, default_fields, - view_id: None, - auth: auth_requirement(no_auth), - raw_output: false, - pagination_command: None, - cursor_command: None, - } + ) + .with_auth(auth_requirement(no_auth)) } /// Builds a request that declares a human view id, the way the engine does for a @@ -79,21 +75,18 @@ fn middleware_request_with_view<'request>( default_fields: &'request str, no_auth: bool, ) -> MiddlewareRequest<'request> { - MiddlewareRequest { + MiddlewareRequest::new( meta, command_path, - system: command_path + command_path .split_once(':') .map_or(command_path, |(system, _)| system), user_args, args, default_fields, - view_id: Some(view_id), - auth: auth_requirement(no_auth), - raw_output: false, - pagination_command: None, - cursor_command: None, - } + ) + .with_view_id(view_id) + .with_auth(auth_requirement(no_auth)) } /// Maps the legacy `no_auth` bool used by these helpers to an [`AuthRequirement`]: @@ -116,19 +109,8 @@ fn middleware_request_with_system<'request>( default_fields: &'request str, no_auth: bool, ) -> MiddlewareRequest<'request> { - MiddlewareRequest { - meta, - command_path, - system, - user_args, - args, - default_fields, - view_id: None, - auth: auth_requirement(no_auth), - raw_output: false, - pagination_command: None, - cursor_command: None, - } + MiddlewareRequest::new(meta, command_path, system, user_args, args, default_fields) + .with_auth(auth_requirement(no_auth)) } #[derive(Debug, Default)] @@ -10907,19 +10889,15 @@ async fn optional_skips_auth_when_handler_ignores_credential() { let output = middleware .run( - MiddlewareRequest { - meta: CommandMeta::default(), - command_path: "things:list", - system: "things", - user_args: value_map([]), - args: value_map([]), - default_fields: "", - view_id: None, - auth: cli_engine::AuthRequirement::Optional, - raw_output: false, - pagination_command: None, - cursor_command: None, - }, + MiddlewareRequest::new( + CommandMeta::default(), + "things:list", + "things", + value_map([]), + value_map([]), + "", + ) + .with_auth(cli_engine::AuthRequirement::Optional), async |_resolver| Ok(CommandResult::new(json!({"ok": true}))), ) .await @@ -10947,19 +10925,15 @@ async fn optional_swallowed_auth_failure_then_command_error_is_not_auth_error() let output = middleware .run( - MiddlewareRequest { - meta: CommandMeta::default(), - command_path: "things:list", - system: "things-api", - user_args: value_map([]), - args: value_map([]), - default_fields: "", - view_id: None, - auth: cli_engine::AuthRequirement::Optional, - raw_output: false, - pagination_command: None, - cursor_command: None, - }, + MiddlewareRequest::new( + CommandMeta::default(), + "things:list", + "things-api", + value_map([]), + value_map([]), + "", + ) + .with_auth(cli_engine::AuthRequirement::Optional), async |resolver: CredentialResolver| { // Best-effort identity; the missing provider makes this fail, and // the handler deliberately ignores it. @@ -11000,19 +10974,15 @@ async fn optional_handler_propagated_auth_failure_is_classified_auth_error() { let output = middleware .run( - MiddlewareRequest { - meta: CommandMeta::default(), - command_path: "things:list", - system: "things-api", - user_args: value_map([]), - args: value_map([]), - default_fields: "", - view_id: None, - auth: cli_engine::AuthRequirement::Optional, - raw_output: false, - pagination_command: None, - cursor_command: None, - }, + MiddlewareRequest::new( + CommandMeta::default(), + "things:list", + "things-api", + value_map([]), + value_map([]), + "", + ) + .with_auth(cli_engine::AuthRequirement::Optional), async |resolver: CredentialResolver| { resolver.resolve().await?; Ok(CommandResult::new(json!({}))) From 202f860d862365641255c9781ecbea25fce05f88 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 14:41:25 -0700 Subject: [PATCH 04/14] fix: address second Copilot review pass on cursor pagination Reject with_cursor on handler shapes that can never act on it: non-context RuntimeCommandSpec::new/new_typed (no CommandContext to read back middleware.cursor_limit/continue_token, mirroring the existing handles_dry_run guard) and streaming constructors (a streaming result is always wrapped as CommandResult::new(Value::Null), so there's no array or CursorContinuation to attach cursor metadata to). Also: MiddlewareRequest::with_pagination_command/with_cursor_command now clear each other, so external construction can't set both replayable commands at once; CursorMeta.limit and the concepts.md guide now document that a handler can override the effective page size via CursorContinuation::with_limit; and the human "so far" summary now includes --limit, matching the --limit/--continue pair the engine actually suggests for a token that isn't self-sufficient about page size. Co-Authored-By: Claude Sonnet 5 --- cli-engine/docs/concepts.md | 6 ++-- cli-engine/src/command/runtime.rs | 36 +++++++++++++++++++++++ cli-engine/src/middleware/mod.rs | 8 +++++ cli-engine/src/output/envelope.rs | 12 +++++--- cli-engine/src/output/human/footer.rs | 6 ++-- cli-engine/tests/cursor_pagination.rs | 42 +++++++++++++-------------- 6 files changed, 80 insertions(+), 30 deletions(-) diff --git a/cli-engine/docs/concepts.md b/cli-engine/docs/concepts.md index 2f629f3..9e8ad44 100644 --- a/cli-engine/docs/concepts.md +++ b/cli-engine/docs/concepts.md @@ -521,11 +521,11 @@ literal follow-up command instead of having to compute the next offset themselve ### cursor pagination -A command that opted into `--limit`/`--continue` cursor pagination via `CommandSpec::with_cursor` gets a top-level `cursor` field on the envelope instead of `pagination` — `limit`, `count`, `total`, `remaining`, `continue_from`, and `has_more` — whenever it returned array data. Unlike `pagination`, the engine cannot compute this itself: a cursor is opaque to everything except the handler that called the backend, so `limit`/`count` are the only pieces the engine derives (the parsed `--limit` and the returned array's length); `total`/`remaining`/`continue_from` come from whatever the handler reported via `CommandResult::with_cursor(CursorContinuation::more(token).with_total(n).with_remaining(n))` — or `CursorContinuation::done()` (or no call at all) to report the end of iteration. `total`/`remaining` are `None` when the backend never reports them, which a pure opaque-cursor API is not obligated to do. +A command that opted into `--limit`/`--continue` cursor pagination via `CommandSpec::with_cursor` gets a top-level `cursor` field on the envelope instead of `pagination` — `limit`, `count`, `total`, `remaining`, `continue_from`, and `has_more` — whenever it returned array data. Unlike `pagination`, the engine cannot compute this itself: a cursor is opaque to everything except the handler that called the backend, so `count` is the only piece the engine derives itself (the returned array's length); `limit` defaults to the parsed `--limit`, but a handler can override it via `CursorContinuation::with_limit` to report the effective page size it actually resumed with (e.g. one decoded from `continue_from` itself); `total`/`remaining`/`continue_from` come from whatever the handler reported via `CommandResult::with_cursor(CursorContinuation::more(token).with_total(n).with_remaining(n))` — or `CursorContinuation::done()` (or no call at all) to report the end of iteration. `total`/`remaining` are `None` when the backend never reports them, which a pure opaque-cursor API is not obligated to do. -Human output merges this into the table's row-count footer: `(N of M rows)` when a total is known, `(N rows, M remaining)` when only a remaining count is known, or `(N rows so far; use --continue for more)` when neither is known. A cursor-paginated response that doesn't render as a table gets the standalone counterpart: `Showing N of M`, `Showing N (M remaining)`, or `Showing N items so far; use --continue for more`. +Human output merges this into the table's row-count footer: `(N of M rows)` when a total is known, `(N rows, M remaining)` when only a remaining count is known, or `(N rows so far; use --limit L --continue for more)` when neither is known. A cursor-paginated response that doesn't render as a table gets the standalone counterpart: `Showing N of M`, `Showing N (M remaining)`, or `Showing N items so far; use --limit L --continue for more`. -When `continue_from` is present (`has_more`), the engine appends a `next_actions` entry replaying the command with `--limit`/`--continue ` for the next page. +When `continue_from` is present (`has_more`), the engine appends a `next_actions` entry replaying the command with `--continue ` for the next page, plus `--limit` — unless the handler's `CursorContinuation::with_limit` marked the token itself as already self-sufficient about page size, in which case `--limit` is omitted from the suggested command. A command registers `with_pagination` or `with_cursor`, never both. diff --git a/cli-engine/src/command/runtime.rs b/cli-engine/src/command/runtime.rs index d13a8c3..eeba4d6 100644 --- a/cli-engine/src/command/runtime.rs +++ b/cli-engine/src/command/runtime.rs @@ -71,6 +71,16 @@ impl RuntimeCommandSpec { new_typed_with_context to keep typed args) instead", spec.name ); + debug_assert!( + spec.cursor.is_none(), + "command {:?} sets with_cursor but RuntimeCommandSpec::new's handler \ + (CredentialResolver, args) has no CommandContext and can never read back \ + middleware.cursor_limit/continue_token to drive its own backend call, so \ + --continue would advertise resumption the handler cannot perform; use \ + RuntimeCommandSpec::new_with_context (or new_typed_with_context to keep typed \ + args) instead", + spec.name + ); Self { spec, streaming_handler: None, @@ -117,6 +127,14 @@ impl RuntimeCommandSpec { raw_output is only supported on non-streaming commands", spec.name ); + debug_assert!( + spec.cursor.is_none(), + "command {:?} sets with_cursor but a streaming handler's result is always wrapped \ + as CommandResult::new(Value::Null) — there is no array or CursorContinuation to \ + report, so --continue would advertise resumption that can never happen; \ + with_cursor is only supported on non-streaming commands", + spec.name + ); let streaming: StreamingCommandHandler = Arc::new(move |context, sender| { let future = handler(context, sender); Box::pin(future) @@ -159,6 +177,16 @@ impl RuntimeCommandSpec { new_typed_with_context to keep typed args) instead", spec.name ); + debug_assert!( + spec.cursor.is_none(), + "command {:?} sets with_cursor but RuntimeCommandSpec::new_typed's handler \ + (CredentialResolver, args) has no CommandContext and can never read back \ + middleware.cursor_limit/continue_token to drive its own backend call, so \ + --continue would advertise resumption the handler cannot perform; use \ + RuntimeCommandSpec::new_with_context (or new_typed_with_context to keep typed \ + args) instead", + spec.name + ); let handler = Arc::new(handler); Self { spec, @@ -251,6 +279,14 @@ impl RuntimeCommandSpec { raw_output is only supported on non-streaming commands", spec.name ); + debug_assert!( + spec.cursor.is_none(), + "command {:?} sets with_cursor but a streaming handler's result is always wrapped \ + as CommandResult::new(Value::Null) — there is no array or CursorContinuation to \ + report, so --continue would advertise resumption that can never happen; \ + with_cursor is only supported on non-streaming commands", + spec.name + ); let handler = Arc::new(handler); let streaming: StreamingCommandHandler = Arc::new(move |context, sender| { let parsed = T::from_arg_matches(context.raw_matches.as_ref()); diff --git a/cli-engine/src/middleware/mod.rs b/cli-engine/src/middleware/mod.rs index a064d70..8075379 100644 --- a/cli-engine/src/middleware/mod.rs +++ b/cli-engine/src/middleware/mod.rs @@ -679,14 +679,22 @@ impl<'request> MiddlewareRequest<'request> { } /// Sets the replayable command text for offset pagination's `next_actions`. + /// + /// Clears `cursor_command` — a command replays as one pagination style or + /// the other, never both; setting one via its builder is how a caller + /// signals the other no longer applies. pub fn with_pagination_command(mut self, pagination_command: impl Into) -> Self { self.pagination_command = Some(pagination_command.into()); + self.cursor_command = None; self } /// Sets the replayable command text for cursor pagination's `next_actions`. + /// + /// Clears `pagination_command` — see [`with_pagination_command`](Self::with_pagination_command). pub fn with_cursor_command(mut self, cursor_command: impl Into) -> Self { self.cursor_command = Some(cursor_command.into()); + self.pagination_command = None; self } } diff --git a/cli-engine/src/output/envelope.rs b/cli-engine/src/output/envelope.rs index a3defff..e9b0756 100644 --- a/cli-engine/src/output/envelope.rs +++ b/cli-engine/src/output/envelope.rs @@ -176,15 +176,19 @@ pub struct PaginationMeta { /// Cursor-pagination metadata. /// -/// `limit` and `count` are computed by the engine (the requested page size -/// and the returned array's length); `total`, `remaining`, and -/// `continue_from` come from the handler's +/// `count` is computed by the engine (the returned array's length). `limit` +/// is normally the requested `--limit`, but a handler can override it via +/// [`CursorContinuation::with_limit`](crate::CursorContinuation::with_limit) +/// to report the effective page size it actually resumed with — e.g. one +/// decoded from `continue_from` itself rather than the parsed flag. `total`, +/// `remaining`, and `continue_from` come from the handler's /// [`CursorContinuation`](crate::CursorContinuation), since only it talked to /// the opaque backend cursor. `total`/`remaining` are `None` when the backend /// never reports them — a cursor API is not guaranteed to know its own total. #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] pub struct CursorMeta { - /// Requested page size. + /// Effective page size — the parsed `--limit`, unless overridden by + /// [`CursorContinuation::with_limit`](crate::CursorContinuation::with_limit). pub limit: i64, /// Item count in this response. pub count: i64, diff --git a/cli-engine/src/output/human/footer.rs b/cli-engine/src/output/human/footer.rs index 345ddaf..6face98 100644 --- a/cli-engine/src/output/human/footer.rs +++ b/cli-engine/src/output/human/footer.rs @@ -100,7 +100,8 @@ pub(super) fn cursor_summary_text( } (SummaryStyle::TableFooter, None, None, Some(token)) => { format!( - "{count} rows so far; use --continue {} for more", + "{count} rows so far; use --limit {} --continue {} for more", + cursor.limit, quote_pagination_value(token) ) } @@ -111,7 +112,8 @@ pub(super) fn cursor_summary_text( } (SummaryStyle::Standalone, None, None, Some(token)) => { format!( - "Showing {count} items so far; use --continue {} for more", + "Showing {count} items so far; use --limit {} --continue {} for more", + cursor.limit, quote_pagination_value(token) ) } diff --git a/cli-engine/tests/cursor_pagination.rs b/cli-engine/tests/cursor_pagination.rs index 93c01c7..221f6b3 100644 --- a/cli-engine/tests/cursor_pagination.rs +++ b/cli-engine/tests/cursor_pagination.rs @@ -197,14 +197,14 @@ async fn last_page_has_no_next_action_and_has_more_is_false() { #[tokio::test] async fn with_total_and_remaining_surface_on_the_envelope() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); - cli.add_command(RuntimeCommandSpec::new( + cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) .with_cursor(CursorConfig { default_limit: 2, max_limit: 0, }), - async |_credential, _args| { + async |_ctx| { Ok( CommandResult::new(json!([{"name": "alpha"}, {"name": "beta"}])).with_cursor( CursorContinuation::more("tok-2") @@ -240,14 +240,14 @@ async fn with_total_and_remaining_surface_on_the_envelope() { #[tokio::test] async fn cursor_metadata_is_absent_when_the_handler_result_is_not_an_array() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); - cli.add_command(RuntimeCommandSpec::new( + cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) .with_cursor(CursorConfig { default_limit: 2, max_limit: 0, }), - async |_credential, _args| { + async |_ctx| { Ok(CommandResult::new(json!({"name": "alpha"})) .with_cursor(CursorContinuation::more("tok-2"))) }, @@ -299,14 +299,14 @@ async fn cursor_metadata_is_absent_after_expr_reshapes_data_to_a_scalar() { #[tokio::test] async fn with_limit_overrides_the_envelope_and_omits_limit_from_the_next_action() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); - cli.add_command(RuntimeCommandSpec::new( + cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) .with_cursor(CursorConfig { default_limit: 25, max_limit: 0, }), - async |_credential, _args| { + async |_ctx| { Ok( CommandResult::new(json!([{"name": "alpha"}, {"name": "beta"}])) .with_cursor(CursorContinuation::more("tok-2").with_limit(2)), @@ -420,7 +420,7 @@ fn with_cursor_panics_when_default_limit_exceeds_max_limit() { )] fn with_pagination_and_with_cursor_together_panics_on_registration() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); - cli.add_command(RuntimeCommandSpec::new( + cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("bad", "Bad") .no_auth(true) .with_pagination(cli_engine::PaginationConfig::default()) @@ -428,7 +428,7 @@ fn with_pagination_and_with_cursor_together_panics_on_registration() { default_limit: 1, max_limit: 0, }), - async |_credential, _args| Ok(CommandResult::new(json!([]))), + async |_ctx| Ok(CommandResult::new(json!([]))), )); } @@ -439,7 +439,7 @@ fn with_pagination_and_with_cursor_together_panics_on_registration() { #[cfg_attr(debug_assertions, should_panic(expected = "mutually exclusive"))] fn raw_output_paired_with_cursor_panics_on_registration() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); - cli.add_command(RuntimeCommandSpec::new( + cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("bad", "Bad") .no_auth(true) .raw_output(true) @@ -447,7 +447,7 @@ fn raw_output_paired_with_cursor_panics_on_registration() { default_limit: 1, max_limit: 0, }), - async |_credential, _args| Ok(CommandResult::new(json!("text"))), + async |_ctx| Ok(CommandResult::new(json!("text"))), )); } @@ -477,14 +477,14 @@ async fn next_page_action_replays_other_flags_the_user_passed() { #[tokio::test] async fn next_page_action_quotes_a_continuation_token_with_shell_metacharacters() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); - cli.add_command(RuntimeCommandSpec::new( + cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) .with_cursor(CursorConfig { default_limit: 2, max_limit: 0, }), - async |_credential, _args| { + async |_ctx| { Ok(CommandResult::new(json!(items())).with_cursor(CursorContinuation::more("a b;c"))) }, )); @@ -514,7 +514,7 @@ async fn human_output_shows_so_far_summary_when_total_is_unknown() { assert!( output .rendered - .contains("(2 rows so far; use --continue 2 for more)"), + .contains("(2 rows so far; use --limit 2 --continue 2 for more)"), "{}", output.rendered ); @@ -540,14 +540,14 @@ async fn human_output_shows_so_far_summary_when_total_is_unknown() { #[tokio::test] async fn human_output_so_far_summary_quotes_a_continuation_token_with_shell_metacharacters() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); - cli.add_command(RuntimeCommandSpec::new( + cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) .with_cursor(CursorConfig { default_limit: 2, max_limit: 0, }), - async |_credential, _args| { + async |_ctx| { Ok(CommandResult::new(json!(items())).with_cursor(CursorContinuation::more("a b;c"))) }, )); @@ -557,7 +557,7 @@ async fn human_output_so_far_summary_quotes_a_continuation_token_with_shell_meta assert!( output .rendered - .contains("so far; use --continue \"a b;c\" for more"), + .contains("so far; use --limit 2 --continue \"a b;c\" for more"), "{}", output.rendered ); @@ -566,14 +566,14 @@ async fn human_output_so_far_summary_quotes_a_continuation_token_with_shell_meta #[tokio::test] async fn human_output_shows_total_when_known() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); - cli.add_command(RuntimeCommandSpec::new( + cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) .with_cursor(CursorConfig { default_limit: 2, max_limit: 0, }), - async |_credential, _args| { + async |_ctx| { Ok( CommandResult::new(json!([{"name": "alpha"}, {"name": "beta"}])) .with_cursor(CursorContinuation::more("2").with_total(4)), @@ -629,14 +629,14 @@ async fn human_standalone_summary_for_a_non_table_cursor_response() { // not `render_table`, so the standalone `append_cursor_summary` line is // the one that must fire, not the merged table footer. let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); - cli.add_command(RuntimeCommandSpec::new( + cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) .with_cursor(CursorConfig { default_limit: 2, max_limit: 0, }), - async |_credential, _args| { + async |_ctx| { Ok(CommandResult::new(json!(["alpha", "beta"])) .with_cursor(CursorContinuation::more("2"))) }, @@ -647,7 +647,7 @@ async fn human_standalone_summary_for_a_non_table_cursor_response() { assert!( output .rendered - .contains("Showing 2 items so far; use --continue 2 for more"), + .contains("Showing 2 items so far; use --limit 2 --continue 2 for more"), "{}", output.rendered ); From d6fbdd8fa08da2db2e43e9358a2b3a6040ba0cad Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 15:00:09 -0700 Subject: [PATCH 05/14] fix: clear stale offset-pagination state on cursor commands, fix so-far hint apply_cursor_flags left middleware.limit/offset untouched, so a value set by a prior with_pagination command on the same long-lived Middleware (Cli owns one across repeated run() calls, and it's mutable via Cli::middleware_mut) would survive into a cursor command's run. apply_pipeline slices on limit > 0 || offset > 0 with no idea which pagination style the current command declared, so a stale value would silently client-slice a response the handler already computed the exact requested page for. Cursor commands now zero both fields before running. Also add CursorMeta.self_sufficient_limit, set from whether the handler called CursorContinuation::with_limit, so the human "so far" resume hint can match next_actions exactly: include --limit only when the token isn't self-sufficient about page size, omit it when it is. The previous fix (always show --limit) was itself wrong for the self-sufficient case, where a handler-reported effective limit can exceed the command's own max_limit and get rejected by the parser if replayed literally. Co-Authored-By: Claude Sonnet 5 --- cli-engine/docs/concepts.md | 4 +-- cli-engine/src/cli/flags_apply.rs | 12 +++++++ cli-engine/src/middleware/run.rs | 1 + cli-engine/src/output/envelope.rs | 9 +++++ cli-engine/src/output/human/footer.rs | 30 ++++++++++++---- cli-engine/tests/cursor_pagination.rs | 50 +++++++++++++++++++++++++-- 6 files changed, 96 insertions(+), 10 deletions(-) diff --git a/cli-engine/docs/concepts.md b/cli-engine/docs/concepts.md index 9e8ad44..1473e45 100644 --- a/cli-engine/docs/concepts.md +++ b/cli-engine/docs/concepts.md @@ -521,9 +521,9 @@ literal follow-up command instead of having to compute the next offset themselve ### cursor pagination -A command that opted into `--limit`/`--continue` cursor pagination via `CommandSpec::with_cursor` gets a top-level `cursor` field on the envelope instead of `pagination` — `limit`, `count`, `total`, `remaining`, `continue_from`, and `has_more` — whenever it returned array data. Unlike `pagination`, the engine cannot compute this itself: a cursor is opaque to everything except the handler that called the backend, so `count` is the only piece the engine derives itself (the returned array's length); `limit` defaults to the parsed `--limit`, but a handler can override it via `CursorContinuation::with_limit` to report the effective page size it actually resumed with (e.g. one decoded from `continue_from` itself); `total`/`remaining`/`continue_from` come from whatever the handler reported via `CommandResult::with_cursor(CursorContinuation::more(token).with_total(n).with_remaining(n))` — or `CursorContinuation::done()` (or no call at all) to report the end of iteration. `total`/`remaining` are `None` when the backend never reports them, which a pure opaque-cursor API is not obligated to do. +A command that opted into `--limit`/`--continue` cursor pagination via `CommandSpec::with_cursor` gets a top-level `cursor` field on the envelope instead of `pagination` — `limit`, `count`, `total`, `remaining`, `continue_from`, `has_more`, and `self_sufficient_limit` — whenever it returned array data. Unlike `pagination`, the engine cannot compute this itself: a cursor is opaque to everything except the handler that called the backend, so `count` is the only piece the engine derives itself (the returned array's length); `limit` defaults to the parsed `--limit`, but a handler can override it via `CursorContinuation::with_limit` to report the effective page size it actually resumed with (e.g. one decoded from `continue_from` itself) — `self_sufficient_limit` is `true` exactly when that override happened, meaning `continue_from` alone is enough to resume and a replay command can omit `--limit`; `total`/`remaining`/`continue_from` come from whatever the handler reported via `CommandResult::with_cursor(CursorContinuation::more(token).with_total(n).with_remaining(n))` — or `CursorContinuation::done()` (or no call at all) to report the end of iteration. `total`/`remaining` are `None` when the backend never reports them, which a pure opaque-cursor API is not obligated to do. -Human output merges this into the table's row-count footer: `(N of M rows)` when a total is known, `(N rows, M remaining)` when only a remaining count is known, or `(N rows so far; use --limit L --continue for more)` when neither is known. A cursor-paginated response that doesn't render as a table gets the standalone counterpart: `Showing N of M`, `Showing N (M remaining)`, or `Showing N items so far; use --limit L --continue for more`. +Human output merges this into the table's row-count footer: `(N of M rows)` when a total is known, `(N rows, M remaining)` when only a remaining count is known, or `(N rows so far; use --limit L --continue for more)` when neither is known — `--limit L` is omitted from this hint exactly when `self_sufficient_limit` is `true`, matching the `next_actions` entry below. A cursor-paginated response that doesn't render as a table gets the standalone counterpart: `Showing N of M`, `Showing N (M remaining)`, or `Showing N items so far; use --limit L --continue for more` (same `--limit` omission rule). When `continue_from` is present (`has_more`), the engine appends a `next_actions` entry replaying the command with `--continue ` for the next page, plus `--limit` — unless the handler's `CursorContinuation::with_limit` marked the token itself as already self-sufficient about page size, in which case `--limit` is omitted from the suggested command. diff --git a/cli-engine/src/cli/flags_apply.rs b/cli-engine/src/cli/flags_apply.rs index f8e1fa4..728c5c7 100644 --- a/cli-engine/src/cli/flags_apply.rs +++ b/cli-engine/src/cli/flags_apply.rs @@ -67,6 +67,18 @@ pub(super) fn apply_cursor_flags( .copied() .unwrap_or(cursor.default_limit); middleware.continue_token = leaf.get_one::("continue").cloned(); + // `Middleware` is long-lived across repeated `Cli::run` calls (and + // pre-settable via `Cli::middleware_mut`), but `apply_pagination_flags` + // only touches `limit`/`offset` for a `with_pagination` command — a + // prior command's nonzero values would otherwise survive into this + // cursor command's run. `apply_pipeline`'s offset-slicing triggers on + // `limit > 0 || offset > 0` with no idea which pagination style (if any) + // the current command declared, so a stale value here would client-slice + // a response the handler already computed exactly the requested page + // for. Cursor and offset pagination are mutually exclusive per command, + // so this command never wants pipeline-level slicing at all. + middleware.limit = 0; + middleware.offset = 0; } /// Replays a paginating command's own explicit args, plus the global diff --git a/cli-engine/src/middleware/run.rs b/cli-engine/src/middleware/run.rs index 0c9434a..6dd8f6b 100644 --- a/cli-engine/src/middleware/run.rs +++ b/cli-engine/src/middleware/run.rs @@ -614,6 +614,7 @@ impl Middleware { remaining: continuation.remaining, continue_from: continuation.continue_from, has_more, + self_sufficient_limit: continuation.limit.is_some(), }); } envelope.with_context( diff --git a/cli-engine/src/output/envelope.rs b/cli-engine/src/output/envelope.rs index e9b0756..a15e141 100644 --- a/cli-engine/src/output/envelope.rs +++ b/cli-engine/src/output/envelope.rs @@ -204,6 +204,15 @@ pub struct CursorMeta { pub continue_from: Option, /// Whether more data is available (`continue_from.is_some()`). pub has_more: bool, + /// Whether `continue_from` alone is sufficient to resume at `limit` + /// (the handler called + /// [`CursorContinuation::with_limit`](crate::CursorContinuation::with_limit)), + /// so a replay command can omit `--limit` — the same condition the + /// engine uses to decide whether `next_actions` includes it. `false` for + /// a plain [`CursorContinuation::more`](crate::CursorContinuation::more) + /// token, where `limit` is just the parsed `--limit` and resuming with a + /// different one could change page size. + pub self_sufficient_limit: bool, } /// Structured error payload in an [`Envelope`]. diff --git a/cli-engine/src/output/human/footer.rs b/cli-engine/src/output/human/footer.rs index 6face98..3938393 100644 --- a/cli-engine/src/output/human/footer.rs +++ b/cli-engine/src/output/human/footer.rs @@ -100,9 +100,8 @@ pub(super) fn cursor_summary_text( } (SummaryStyle::TableFooter, None, None, Some(token)) => { format!( - "{count} rows so far; use --limit {} --continue {} for more", - cursor.limit, - quote_pagination_value(token) + "{count} rows so far; use {} for more", + resume_hint(cursor, token) ) } (SummaryStyle::TableFooter, None, None, None) => format!("{count} rows"), @@ -112,15 +111,34 @@ pub(super) fn cursor_summary_text( } (SummaryStyle::Standalone, None, None, Some(token)) => { format!( - "Showing {count} items so far; use --limit {} --continue {} for more", - cursor.limit, - quote_pagination_value(token) + "Showing {count} items so far; use {} for more", + resume_hint(cursor, token) ) } (SummaryStyle::Standalone, None, None, None) => format!("Showing {count}"), } } +/// Builds the `--limit N --continue `/`--continue ` fragment +/// for a cursor "so far" hint, matching exactly what the engine appends to +/// `next_actions` for the same response (`middleware::run::render_envelope`): +/// `--limit` is included unless `cursor.self_sufficient_limit` says the +/// token alone already carries the effective page size. Copy-pasting this +/// hint must produce the same command the machine-readable `next_actions` +/// entry already suggests — including `--limit` when the token doesn't +/// need it would print a fabricated size, but omitting it when the token +/// truly doesn't carry one could resume at a different page size (or, if +/// the handler's effective limit exceeds this command's own `max_limit`, +/// print a `--limit` the parser would reject outright). +fn resume_hint(cursor: &CursorMeta, token: &str) -> String { + let token = quote_pagination_value(token); + if cursor.self_sufficient_limit { + format!("--continue {token}") + } else { + format!("--limit {} --continue {token}", cursor.limit) + } +} + /// Appends a one-line pagination summary to `out` (a no-op when the response /// wasn't paginated). Unlike `next_actions`, this always shows the underlying /// facts even on the last page, where there's no follow-up command to diff --git a/cli-engine/tests/cursor_pagination.rs b/cli-engine/tests/cursor_pagination.rs index 221f6b3..ec6bf07 100644 --- a/cli-engine/tests/cursor_pagination.rs +++ b/cli-engine/tests/cursor_pagination.rs @@ -115,7 +115,13 @@ async fn default_limit_applies_when_neither_flag_is_passed() { // total/remaining are absent — this fake backend never reports them. assert_eq!( rendered["cursor"], - json!({"limit": 2, "count": 2, "continue_from": "2", "has_more": true}) + json!({ + "limit": 2, + "count": 2, + "continue_from": "2", + "has_more": true, + "self_sufficient_limit": false + }) ); assert_eq!( rendered["next_actions"][0]["command"], @@ -226,7 +232,8 @@ async fn with_total_and_remaining_surface_on_the_envelope() { "total": 4, "remaining": 2, "continue_from": "tok-2", - "has_more": true + "has_more": true, + "self_sufficient_limit": false }) ); } @@ -320,6 +327,7 @@ async fn with_limit_overrides_the_envelope_and_omits_limit_from_the_next_action( assert_eq!(output.exit_code, 0, "{}", output.rendered); let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); assert_eq!(rendered["cursor"]["limit"], json!(2)); + assert_eq!(rendered["cursor"]["self_sufficient_limit"], json!(true)); let next_actions = rendered["next_actions"].as_array().expect("next_actions"); // `with_limit` means the token is self-sufficient about page size, so // the replay omits `--limit` entirely rather than repeating a value @@ -563,6 +571,44 @@ async fn human_output_so_far_summary_quotes_a_continuation_token_with_shell_meta ); } +/// The "so far" hint must agree with the generated `next_actions` command: +/// when the handler called `with_limit`, the token alone is self-sufficient +/// about page size, so `next_actions` omits `--limit` — and this hint must +/// omit it too, or copy-pasting it would suggest a `--limit` that could +/// differ from (or exceed the command's own cap for) the effective size the +/// token actually carries. +#[tokio::test] +async fn human_output_so_far_summary_omits_limit_when_the_token_is_self_sufficient() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig { + default_limit: 25, + max_limit: 0, + }), + async |_ctx| { + Ok(CommandResult::new(json!(items())) + .with_cursor(CursorContinuation::more("tok-2").with_limit(2))) + }, + )); + + let output = cli.run(["my-cli", "list", "--output", "human"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + assert!( + output + .rendered + .contains("so far; use --continue tok-2 for more"), + "{}", + output.rendered + ); + assert!( + !output.rendered.contains("--limit"), + "self-sufficient token must not suggest a --limit: {}", + output.rendered + ); +} + #[tokio::test] async fn human_output_shows_total_when_known() { let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); From 1876a2a29880534762eb1de8183b8011a2fe7eb8 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 15:13:40 -0700 Subject: [PATCH 06/14] fix: escape control characters in replay tokens, harden pipeline pagination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit quote_pagination_value only escaped shell metacharacters — a backend- supplied cursor token containing a raw newline or ANSI escape sequence could still make the printed next-page suggestion look like multiple lines or repaint the terminal when displayed. Control characters now render as \n/\r/\t or a \xHH hex placeholder instead of passing through raw. Also make the pipeline itself, not just apply_cursor_flags, the authoritative place a cursor command's response is never client-sliced: a pre_run hook (a legitimate extension point) runs after apply_cursor_flags and could still set middleware.limit/offset, and a caller driving Middleware::run directly bypasses apply_cursor_flags entirely. Forcing limit/offset to zero whenever cursor_command.is_some(), right where PipelineOpts is built, closes that regardless of how the stale state got there. Co-Authored-By: Claude Sonnet 5 --- cli-engine/src/cli/flags_apply.rs | 50 +++++++++++++++++++++++++++---- cli-engine/src/middleware/run.rs | 21 +++++++++++-- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/cli-engine/src/cli/flags_apply.rs b/cli-engine/src/cli/flags_apply.rs index 728c5c7..cb23ef3 100644 --- a/cli-engine/src/cli/flags_apply.rs +++ b/cli-engine/src/cli/flags_apply.rs @@ -203,16 +203,31 @@ fn pagination_arg_display(value: &serde_json::Value) -> String { /// are backslash-escaped (backslash first, so escaping the others doesn't /// re-escape the backslashes it just inserted) so the value can't break out /// of the double quotes or trigger POSIX-shell expansion (`$VAR`, `$(...)`, -/// backticks) if the suggestion is copy-pasted into a shell. +/// backticks) if the suggestion is copy-pasted into a shell. A cursor token +/// is backend-controlled (unlike most other replayed values, which are the +/// user's own prior flags), so a control character — an embedded newline +/// that would make the printed command look like more than one line, or an +/// ANSI escape sequence that could otherwise repaint the terminal when this +/// is printed — is rendered as a literal `\xHH`/`\n`/`\r`/`\t` placeholder +/// rather than passed through raw. pub(crate) fn quote_pagination_value(value: &str) -> String { let safe_unquoted = |c: char| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.' | '/' | ':' | '@'); if value.is_empty() || !value.chars().all(safe_unquoted) { - let escaped = value - .replace('\\', "\\\\") - .replace('"', "\\\"") - .replace('$', "\\$") - .replace('`', "\\`"); + let mut escaped = String::with_capacity(value.len()); + for c in value.chars() { + match c { + '\\' => escaped.push_str("\\\\"), + '"' => escaped.push_str("\\\""), + '$' => escaped.push_str("\\$"), + '`' => escaped.push_str("\\`"), + '\n' => escaped.push_str("\\n"), + '\r' => escaped.push_str("\\r"), + '\t' => escaped.push_str("\\t"), + c if c.is_control() => escaped.push_str(&format!("\\x{:02x}", c as u32)), + c => escaped.push(c), + } + } format!("\"{escaped}\"") } else { value.to_owned() @@ -585,3 +600,26 @@ mod prescan_env_flag_tests { ); } } + +#[cfg(test)] +mod quote_pagination_value_tests { + use super::quote_pagination_value; + + #[test] + fn newline_carriage_return_and_tab_render_as_named_escapes() { + assert_eq!(quote_pagination_value("a\nb\rc\td"), "\"a\\nb\\rc\\td\""); + } + + #[test] + fn other_control_characters_render_as_hex_escapes() { + // ESC (0x1b), the start of most ANSI escape sequences a backend- + // supplied token could otherwise smuggle straight to the terminal. + assert_eq!(quote_pagination_value("a\x1b[31mb"), "\"a\\x1b[31mb\""); + } + + #[test] + fn ordinary_text_is_unaffected() { + assert_eq!(quote_pagination_value("tok-2"), "tok-2"); + assert_eq!(quote_pagination_value("a b;c"), "\"a b;c\""); + } +} diff --git a/cli-engine/src/middleware/run.rs b/cli-engine/src/middleware/run.rs index 6dd8f6b..9525549 100644 --- a/cli-engine/src/middleware/run.rs +++ b/cli-engine/src/middleware/run.rs @@ -542,12 +542,29 @@ impl Middleware { } let projection_fields = if human_view { "" } else { effective_fields }; if let Some(data) = &mut envelope.data { + // A cursor command never wants pipeline-level slicing — the + // handler already returned exactly the page its own backend call + // asked for. `apply_cursor_flags` already zeroes + // `limit`/`offset` for this reason, but that's an earlier step + // in the same call chain, not the only way to reach this point: + // a `run_pre_run` hook (a legitimate, documented extension + // point) runs after it and could still mutate the public + // `Middleware` fields, and a caller driving `Middleware::run` + // directly (bypassing `Cli::run`'s flag application entirely) + // could preset them. Forcing zero here, at the one place that + // actually performs the slicing, is authoritative regardless of + // how `self.limit`/`self.offset` got set. + let (pipeline_limit, pipeline_offset) = if cursor_command.is_some() { + (0, 0) + } else { + (self.limit, self.offset) + }; let pagination = apply_pipeline( data, &PipelineOpts { filter: self.filter.clone(), - limit: self.limit, - offset: self.offset, + limit: pipeline_limit, + offset: pipeline_offset, expr: self.expr.clone(), fields: projection_fields.to_owned(), fields_are_default: !self.fields_explicit, From 2307d690448aeef50671f8d970ec8697f9bdd477 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 15:31:04 -0700 Subject: [PATCH 07/14] fix: mark PaginationConfig/CursorConfig non_exhaustive with constructors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are plain public structs documented and used via literal construction, so a future field addition would break every external caller — the same problem this PR already hardened Middleware/MiddlewareRequest against. CursorConfig was the specific Copilot finding; PaginationConfig is fixed alongside it for API consistency between the two sibling pagination configs, rather than leaving one non_exhaustive and the other not. Add a plain new(default_limit, max_limit) constructor to each (no with_* builders — both fields are always required, nothing to build up incrementally) and convert every literal-construction call site (tests, AGENTS.md, docs) to use it, since non_exhaustive forbids struct-literal syntax entirely for external callers, spread syntax included. Co-Authored-By: Claude Sonnet 5 --- AGENTS.md | 4 +- cli-engine/docs/concepts.md | 11 +- cli-engine/docs/design.md | 2 +- .../docs/proposals/cursor-first-pagination.md | 5 +- cli-engine/src/command/spec.rs | 50 ++++++-- cli-engine/tests/cursor_pagination.rs | 110 ++++-------------- cli-engine/tests/pagination.rs | 101 ++++------------ 7 files changed, 87 insertions(+), 196 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f1e19ae..284781b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -239,8 +239,8 @@ Command checklist: - Prefer `CommandSpec::from_args::()` + `RuntimeCommandSpec::new_typed` when the command has many flags, needs clap validation attributes, or when porting existing derive-based commands. Use the builder path for simple commands with one or two flags. - Most commands need more than `new_typed`'s `(CredentialResolver, T)` shape — if the handler needs the command path, middleware, or `--dry-run` via `CommandContext`, use `RuntimeCommandSpec::new_typed_with_context` (handler: `Fn(CommandContext, T) -> Fut`) instead of `new_with_context` + `context.typed_args::()`; for streaming commands, use `RuntimeCommandSpec::new_typed_streaming` (handler: `Fn(CommandContext, T, StreamSender) -> Fut`). Both eagerly parse `T` before the handler runs, same guarantee `new_typed` gives the credential-only case. - Use `CommandSpec::with_arg_group(ArgGroup::new(...).args([...]).required(true))` for "at least one of" or mutually-exclusive relationships between args, instead of a `required_unless_present_any`/`conflicts_with` chain. With `from_args::()`, express the same thing declaratively via a struct-level `#[group(required = true, multiple = true)]` (or `multiple = false` for mutually exclusive) on the derive struct — but not on a struct that also has a `#[command(flatten)]` field; `clap_derive` empties that struct's implicit group's members in that case, so the constraint silently does nothing. -- `--limit`/`--offset` are not framework-global: a command only gets them by calling `.with_pagination(PaginationConfig { default_limit, max_limit, ..Default::default() })`. A command with no `.with_pagination(...)` call never registers those flags. `default_limit` applies when the user passes neither flag; `max_limit` (`0` = uncapped) rejects an explicit `--limit` above the cap. Offset pagination is purely client-side: the engine slices whatever array the handler returns using the parsed `--limit`/`--offset` itself, so it only fits a backend that either hands back its full collection or itself supports arbitrary-offset slicing. -- Use `.with_cursor(CursorConfig { default_limit, max_limit })` instead of `.with_pagination` when the backend is cursor-based. This registers `--limit`/`--continue` instead of `--limit`/`--offset`. A command picks exactly one of `.with_pagination`/`.with_cursor`. Unlike offset pagination, the engine cannot slice or measure a cursor itself — the handler reads the parsed values back off `ctx.middleware.cursor_limit`/`.continue_token` to drive its own backend call, and reports what it learned via `CommandResult::with_cursor(CursorContinuation::more(next_token).with_total(n).with_remaining(n))`, or `CursorContinuation::done()` (or no call at all) once iteration is exhausted. +- `--limit`/`--offset` are not framework-global: a command only gets them by calling `.with_pagination(PaginationConfig::new(default_limit, max_limit))`. A command with no `.with_pagination(...)` call never registers those flags. `default_limit` applies when the user passes neither flag; `max_limit` (`0` = uncapped) rejects an explicit `--limit` above the cap. Offset pagination is purely client-side: the engine slices whatever array the handler returns using the parsed `--limit`/`--offset` itself, so it only fits a backend that either hands back its full collection or itself supports arbitrary-offset slicing. +- Use `.with_cursor(CursorConfig::new(default_limit, max_limit))` instead of `.with_pagination` when the backend is cursor-based. This registers `--limit`/`--continue` instead of `--limit`/`--offset`. A command picks exactly one of `.with_pagination`/`.with_cursor`. Unlike offset pagination, the engine cannot slice or measure a cursor itself — the handler reads the parsed values back off `ctx.middleware.cursor_limit`/`.continue_token` to drive its own backend call, and reports what it learned via `CommandResult::with_cursor(CursorContinuation::more(next_token).with_total(n).with_remaining(n))`, or `CursorContinuation::done()` (or no call at all) once iteration is exhausted. - Use `.raw_output(true)` for a command whose only correct output is verbatim text (e.g. printing a schema/config blob to pipe to a file), not a JSON reconstruction of it. The handler's `CommandResult` data must be a JSON string; this removes the `--output`/`--fields`/`--filter`/`--expr`/pagination flags and is incompatible with streaming commands. ## Output And Schemas diff --git a/cli-engine/docs/concepts.md b/cli-engine/docs/concepts.md index 1473e45..8284545 100644 --- a/cli-engine/docs/concepts.md +++ b/cli-engine/docs/concepts.md @@ -275,20 +275,13 @@ values into middleware through `CliConfig::apply_flags`. `--limit`/`--offset` are not framework-global; a command only gets them by opting in: ```rust -CommandSpec::new("list", "List projects").with_pagination(PaginationConfig { - default_limit: 20, - max_limit: 100, - ..Default::default() -}) +CommandSpec::new("list", "List projects").with_pagination(PaginationConfig::new(20, 100)) ``` `--limit`/`--continue` are the cursor-pagination counterpart, for a command backed by a server-maintained, forward-only cursor API — see [cursor pagination](#cursor-pagination): ```rust -CommandSpec::new("list", "List domains").with_cursor(CursorConfig { - default_limit: 25, - max_limit: 500, -}) +CommandSpec::new("list", "List domains").with_cursor(CursorConfig::new(25, 500)) ``` A command opts into exactly one of `with_pagination`/`with_cursor`, never both. diff --git a/cli-engine/docs/design.md b/cli-engine/docs/design.md index c6b0208..f201c1a 100644 --- a/cli-engine/docs/design.md +++ b/cli-engine/docs/design.md @@ -293,7 +293,7 @@ Applications can add their own global flags with `CliConfig::with_register_flags values into middleware with `CliConfig::with_apply_flags`. `--limit`/`--offset` are the one exception: they are not global at all. A command registers them -for itself with `CommandSpec::with_pagination(PaginationConfig { default_limit, max_limit, ..Default::default() })`; +for itself with `CommandSpec::with_pagination(PaginationConfig::new(default_limit, max_limit))`; a command that never calls this has neither flag, in `--help` or on its command line. `default_limit` applies when the user passes neither flag; `max_limit` (when non-zero) rejects an explicit `--limit` above the cap. diff --git a/cli-engine/docs/proposals/cursor-first-pagination.md b/cli-engine/docs/proposals/cursor-first-pagination.md index 57de433..2f400fd 100644 --- a/cli-engine/docs/proposals/cursor-first-pagination.md +++ b/cli-engine/docs/proposals/cursor-first-pagination.md @@ -84,10 +84,7 @@ However, there is an important caveat. With Slicing- or Paging-based APIs that w ### `CommandSpec::with_cursor` ```rust -CommandSpec::new("list", "List things").with_cursor(CursorConfig { - default_limit: 25, - max_limit: 500, -}) +CommandSpec::new("list", "List things").with_cursor(CursorConfig::new(25, 500)) ``` Registers `--limit`/`--continue` the same way `with_pagination` registers diff --git a/cli-engine/src/command/spec.rs b/cli-engine/src/command/spec.rs index 6e4d2bf..48f5ab9 100644 --- a/cli-engine/src/command/spec.rs +++ b/cli-engine/src/command/spec.rs @@ -138,21 +138,22 @@ pub struct CommandSpec { /// /// Registering this is what makes `--limit`/`--offset` exist for a command at /// all — without it, the engine does not register those flags, so they are -/// absent from `--help` and rejected as unknown arguments if passed. Construct -/// it with `..Default::default()`, as in the example below, so a future -/// engine release can add fields without breaking existing callers. +/// absent from `--help` and rejected as unknown arguments if passed. +/// +/// `#[non_exhaustive]`: construct via [`new`](PaginationConfig::new) — never as +/// a struct literal, bare or with `..Default::default()` spread, since +/// `#[non_exhaustive]` forbids struct-literal syntax entirely for a caller +/// outside this crate — so a future engine release can add fields without +/// breaking existing callers. /// /// ``` /// use cli_engine::PaginationConfig; /// -/// let pagination = PaginationConfig { -/// default_limit: 20, -/// max_limit: 100, -/// ..Default::default() -/// }; +/// let pagination = PaginationConfig::new(20, 100); /// assert_eq!(pagination.default_limit, 20); /// ``` #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[non_exhaustive] pub struct PaginationConfig { /// Page size applied when the user passes neither `--limit` nor /// `--offset`. `0` (the default) means unlimited — the same "no @@ -163,6 +164,17 @@ pub struct PaginationConfig { pub max_limit: i64, } +impl PaginationConfig { + /// Creates a pagination config with the given `default_limit` and `max_limit`. + #[must_use] + pub fn new(default_limit: i64, max_limit: i64) -> Self { + Self { + default_limit, + max_limit, + } + } +} + /// Opt-in cursor-pagination policy for a single command, set with /// [`CommandSpec::with_cursor`]. /// @@ -178,16 +190,19 @@ pub struct PaginationConfig { /// `default_limit`, there is no valid all-zero `CursorConfig`, so both /// fields must always be given explicitly. /// +/// `#[non_exhaustive]`: construct via [`new`](CursorConfig::new) — never as a +/// struct literal, since `#[non_exhaustive]` forbids struct-literal syntax +/// entirely for a caller outside this crate — so a future engine release can +/// add fields without breaking existing callers. +/// /// ``` /// use cli_engine::CursorConfig; /// -/// let cursor = CursorConfig { -/// default_limit: 25, -/// max_limit: 500, -/// }; +/// let cursor = CursorConfig::new(25, 500); /// assert_eq!(cursor.default_limit, 25); /// ``` #[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[non_exhaustive] pub struct CursorConfig { /// Page size sent to the backend when the user passes no `--limit`. Must /// be greater than zero. @@ -197,6 +212,17 @@ pub struct CursorConfig { pub max_limit: i64, } +impl CursorConfig { + /// Creates a cursor config with the given `default_limit` and `max_limit`. + #[must_use] + pub fn new(default_limit: i64, max_limit: i64) -> Self { + Self { + default_limit, + max_limit, + } + } +} + impl CommandSpec { /// Creates a command spec with the required name and one-line help. #[must_use] diff --git a/cli-engine/tests/cursor_pagination.rs b/cli-engine/tests/cursor_pagination.rs index ec6bf07..1b990bc 100644 --- a/cli-engine/tests/cursor_pagination.rs +++ b/cli-engine/tests/cursor_pagination.rs @@ -83,10 +83,7 @@ async fn opted_in_command_documents_limit_and_continue_in_help() { let cli = cli_with_cursor_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 3, - }), + .with_cursor(CursorConfig::new(2, 3)), ); let help = cli.run(["my-cli", "list", "--help"]).await; @@ -99,10 +96,7 @@ async fn default_limit_applies_when_neither_flag_is_passed() { let cli = cli_with_cursor_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), ); let output = cli.run(["my-cli", "list", "--output", "json"]).await; @@ -135,10 +129,7 @@ async fn explicit_limit_and_continue_fetch_the_requested_page() { let cli = cli_with_cursor_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), ); let output = cli @@ -171,10 +162,7 @@ async fn last_page_has_no_next_action_and_has_more_is_false() { let cli = cli_with_cursor_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), ); let output = cli @@ -206,10 +194,7 @@ async fn with_total_and_remaining_surface_on_the_envelope() { cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), async |_ctx| { Ok( CommandResult::new(json!([{"name": "alpha"}, {"name": "beta"}])).with_cursor( @@ -250,10 +235,7 @@ async fn cursor_metadata_is_absent_when_the_handler_result_is_not_an_array() { cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), async |_ctx| { Ok(CommandResult::new(json!({"name": "alpha"})) .with_cursor(CursorContinuation::more("tok-2"))) @@ -281,10 +263,7 @@ async fn cursor_metadata_is_absent_after_expr_reshapes_data_to_a_scalar() { let cli = cli_with_cursor_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), ); let output = cli @@ -309,10 +288,7 @@ async fn with_limit_overrides_the_envelope_and_omits_limit_from_the_next_action( cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 25, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(25, 0)), async |_ctx| { Ok( CommandResult::new(json!([{"name": "alpha"}, {"name": "beta"}])) @@ -345,10 +321,7 @@ async fn max_limit_rejects_an_explicit_limit_above_the_cap_but_allows_the_cap_it let cli = cli_with_cursor_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 1, - max_limit: 3, - }), + .with_cursor(CursorConfig::new(1, 3)), ); let output = cli.run(["my-cli", "list", "--limit", "4"]).await; @@ -372,10 +345,7 @@ async fn zero_and_negative_limit_are_rejected_at_parse_time() { let cli = cli_with_cursor_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 1, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(1, 0)), ); let output = cli.run(["my-cli", "list", "--limit", "0"]).await; @@ -400,10 +370,7 @@ async fn zero_and_negative_limit_are_rejected_at_parse_time() { #[test] #[cfg_attr(debug_assertions, should_panic(expected = "greater than zero"))] fn with_cursor_panics_when_default_limit_is_not_positive() { - let _unused = CommandSpec::new("list", "List things").with_cursor(CursorConfig { - default_limit: 0, - max_limit: 5, - }); + let _unused = CommandSpec::new("list", "List things").with_cursor(CursorConfig::new(0, 5)); } #[test] @@ -412,10 +379,7 @@ fn with_cursor_panics_when_default_limit_is_not_positive() { should_panic(expected = "greater than its max_limit") )] fn with_cursor_panics_when_default_limit_exceeds_max_limit() { - let _unused = CommandSpec::new("list", "List things").with_cursor(CursorConfig { - default_limit: 10, - max_limit: 5, - }); + let _unused = CommandSpec::new("list", "List things").with_cursor(CursorConfig::new(10, 5)); } /// A command picks one pagination style, not both; caught at registration @@ -432,10 +396,7 @@ fn with_pagination_and_with_cursor_together_panics_on_registration() { CommandSpec::new("bad", "Bad") .no_auth(true) .with_pagination(cli_engine::PaginationConfig::default()) - .with_cursor(CursorConfig { - default_limit: 1, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(1, 0)), async |_ctx| Ok(CommandResult::new(json!([]))), )); } @@ -451,10 +412,7 @@ fn raw_output_paired_with_cursor_panics_on_registration() { CommandSpec::new("bad", "Bad") .no_auth(true) .raw_output(true) - .with_cursor(CursorConfig { - default_limit: 1, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(1, 0)), async |_ctx| Ok(CommandResult::new(json!("text"))), )); } @@ -465,10 +423,7 @@ async fn next_page_action_replays_other_flags_the_user_passed() { CommandSpec::new("list", "List things") .no_auth(true) .with_arg(Arg::new("status").long("status")) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), ); let output = cli @@ -488,10 +443,7 @@ async fn next_page_action_quotes_a_continuation_token_with_shell_metacharacters( cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), async |_ctx| { Ok(CommandResult::new(json!(items())).with_cursor(CursorContinuation::more("a b;c"))) }, @@ -511,10 +463,7 @@ async fn human_output_shows_so_far_summary_when_total_is_unknown() { let cli = cli_with_cursor_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), ); let output = cli.run(["my-cli", "list", "--output", "human"]).await; @@ -551,10 +500,7 @@ async fn human_output_so_far_summary_quotes_a_continuation_token_with_shell_meta cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), async |_ctx| { Ok(CommandResult::new(json!(items())).with_cursor(CursorContinuation::more("a b;c"))) }, @@ -583,10 +529,7 @@ async fn human_output_so_far_summary_omits_limit_when_the_token_is_self_sufficie cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 25, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(25, 0)), async |_ctx| { Ok(CommandResult::new(json!(items())) .with_cursor(CursorContinuation::more("tok-2").with_limit(2))) @@ -615,10 +558,7 @@ async fn human_output_shows_total_when_known() { cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), async |_ctx| { Ok( CommandResult::new(json!([{"name": "alpha"}, {"name": "beta"}])) @@ -641,10 +581,7 @@ async fn human_output_on_last_page_shows_summary_but_no_next_steps() { let cli = cli_with_cursor_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), ); let output = cli @@ -678,10 +615,7 @@ async fn human_standalone_summary_for_a_non_table_cursor_response() { cli.add_command(RuntimeCommandSpec::new_with_context( CommandSpec::new("list", "List things") .no_auth(true) - .with_cursor(CursorConfig { - default_limit: 2, - max_limit: 0, - }), + .with_cursor(CursorConfig::new(2, 0)), async |_ctx| { Ok(CommandResult::new(json!(["alpha", "beta"])) .with_cursor(CursorContinuation::more("2"))) diff --git a/cli-engine/tests/pagination.rs b/cli-engine/tests/pagination.rs index c65ebb1..91cd539 100644 --- a/cli-engine/tests/pagination.rs +++ b/cli-engine/tests/pagination.rs @@ -61,10 +61,7 @@ async fn opted_in_command_documents_limit_and_offset_in_help() { let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - max_limit: 3, - }), + .with_pagination(PaginationConfig::new(2, 3)), ); let help = cli.run(["my-cli", "list", "--help"]).await; @@ -77,10 +74,7 @@ async fn default_limit_applies_when_neither_flag_is_passed() { let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli.run(["my-cli", "list", "--output", "json"]).await; @@ -108,10 +102,7 @@ async fn explicit_limit_and_offset_override_the_default_and_expose_pagination() let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -144,10 +135,7 @@ async fn last_page_has_no_next_action_and_has_more_is_false() { let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -171,10 +159,7 @@ async fn next_page_action_replays_other_flags_the_user_passed() { CommandSpec::new("list", "List things") .no_auth(true) .with_arg(Arg::new("status").long("status")) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -194,10 +179,7 @@ async fn next_page_action_quotes_values_with_whitespace() { CommandSpec::new("list", "List things") .no_auth(true) .with_arg(Arg::new("status").long("status")) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -224,10 +206,7 @@ async fn next_page_action_quotes_a_binary_name_with_whitespace() { cli.add_command(RuntimeCommandSpec::new( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), async |_credential, _args| Ok(CommandResult::new(json!(items()))), )); @@ -256,10 +235,7 @@ async fn next_page_action_uses_the_real_long_flag_not_the_value_map_key() { cli.add_command(RuntimeCommandSpec::new_typed::( CommandSpec::from_args::("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), async |_credential: CredentialResolver, _args: ListArgs| { Ok(CommandResult::new(json!(items()))) }, @@ -281,10 +257,7 @@ async fn max_limit_rejects_an_explicit_limit_above_the_cap_but_allows_the_cap_it let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - max_limit: 3, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(0, 3)), ); let output = cli.run(["my-cli", "list", "--limit", "4"]).await; @@ -308,10 +281,7 @@ async fn max_limit_does_not_constrain_a_negative_limit() { let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - max_limit: 1, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(0, 1)), ); let output = cli @@ -355,10 +325,8 @@ async fn negative_offset_is_rejected_at_parse_time_not_at_runtime() { should_panic(expected = "greater than its max_limit") )] fn with_pagination_panics_when_default_limit_exceeds_max_limit() { - let _unused = CommandSpec::new("list", "List things").with_pagination(PaginationConfig { - default_limit: 10, - max_limit: 5, - }); + let _unused = + CommandSpec::new("list", "List things").with_pagination(PaginationConfig::new(10, 5)); } #[tokio::test] @@ -366,10 +334,7 @@ async fn human_output_shows_pagination_summary_and_next_steps() { let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli.run(["my-cli", "list", "--output", "human"]).await; @@ -398,10 +363,7 @@ async fn human_output_on_last_page_shows_summary_but_no_next_steps() { let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -431,10 +393,7 @@ async fn next_page_action_preserves_filter_expr_and_fields() { let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -472,10 +431,7 @@ async fn next_page_action_replays_a_set_false_flag_as_a_bare_switch() { .long("no-cache") .action(clap::ArgAction::SetFalse), ) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -498,10 +454,7 @@ async fn human_footer_shows_rows_actually_rendered_after_expr_reshapes_data() { let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -538,10 +491,7 @@ async fn next_page_action_replays_a_multi_value_arg_as_repeated_flags() { .long("scope") .action(clap::ArgAction::Append), ) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -568,10 +518,7 @@ async fn human_standalone_summary_shows_rows_actually_rendered_after_expr_reshap cli.add_command(RuntimeCommandSpec::new( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), async |_credential, _args| { Ok(CommandResult::new(json!([ "alpha", "beta", "gamma", "delta" @@ -609,10 +556,7 @@ async fn next_page_action_escapes_shell_metacharacters_and_expansions() { CommandSpec::new("list", "List things") .no_auth(true) .with_arg(Arg::new("status").long("status")) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli @@ -642,10 +586,7 @@ async fn human_output_uses_a_neutral_pagination_line_when_expr_leaves_no_array() let cli = cli_with_list_command( CommandSpec::new("list", "List things") .no_auth(true) - .with_pagination(PaginationConfig { - default_limit: 2, - ..PaginationConfig::default() - }), + .with_pagination(PaginationConfig::new(2, 0)), ); let output = cli From a44848c17305475b91aca9a5f1b8cf87725382a5 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 15:50:59 -0700 Subject: [PATCH 08/14] docs: note quote_pagination_value's display-safe-not-round-trip-safe tradeoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses a Copilot review finding: escaping a control character to a literal \n/\xHH placeholder (added for terminal display safety) means a shell won't decode it back, so a replayed value containing one can't be copy-pasted into an exact resend. Documenting this as a deliberate trade-off rather than fixing it — the alternative (ANSI-C $'...' quoting) isn't POSIX and would make every other, ordinary replayed value non-portable to gain exact reproduction for a case only a malformed or adversarial backend token would hit. Co-Authored-By: Claude Sonnet 5 --- cli-engine/src/cli/flags_apply.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cli-engine/src/cli/flags_apply.rs b/cli-engine/src/cli/flags_apply.rs index cb23ef3..05cd6aa 100644 --- a/cli-engine/src/cli/flags_apply.rs +++ b/cli-engine/src/cli/flags_apply.rs @@ -210,6 +210,15 @@ fn pagination_arg_display(value: &serde_json::Value) -> String { /// ANSI escape sequence that could otherwise repaint the terminal when this /// is printed — is rendered as a literal `\xHH`/`\n`/`\r`/`\t` placeholder /// rather than passed through raw. +/// +/// Display-safe, not round-trip-safe: a plain shell does not decode `\n`/ +/// `\xHH` inside a double-quoted string back into the original byte, so a +/// value containing a control character cannot be copy-pasted back into an +/// exact resend — a deliberate trade-off, since the alternative (an escape +/// a shell *would* decode, e.g. ANSI-C `$'...'` quoting) is not POSIX and +/// would make every other, ordinary replayed value non-portable to gain +/// exact reproduction for a case that, in practice, only a malformed or +/// adversarial backend cursor token would ever hit. pub(crate) fn quote_pagination_value(value: &str) -> String { let safe_unquoted = |c: char| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.' | '/' | ':' | '@'); From 1f2cbe7ea567aa0f7957ac843e2db23b048500f6 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 16:09:26 -0700 Subject: [PATCH 09/14] fix: escape ! against Bash history expansion, fix stale proposal doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit quote_pagination_value's control-character/shell-metacharacter escaping still left a bare ! inside the double-quoted output. Interactive Bash performs history expansion on an unescaped ! even inside double quotes, so a value like "name != 'alpha'" could expand against shell history or fail with "event not found" if copy-pasted. Backslash-escaping ! there doesn't work either — verified against a real Bash that \! leaves the backslash itself in the resulting argument. Instead, splice each ! into its own single-quoted segment: single quotes are immune to history expansion, and adjacent quoted segments with no separator still concatenate into one argument ("a"'!'"b" parses as a!b), so this both round-trips exactly and is safe. Updated the one pre-existing offset-pagination test whose --filter value contained ! to match the new (correct) escaping. Also fixed a stale section of docs/proposals/cursor-first-pagination.md that still described the envelope design as adding an optional offset to PaginationMeta, rather than the shipped CursorMeta shape (no offset at all, plus remaining/self_sufficient_limit). Co-Authored-By: Claude Sonnet 5 --- .../docs/proposals/cursor-first-pagination.md | 4 +- cli-engine/src/cli/flags_apply.rs | 72 ++++++++++++++----- cli-engine/tests/pagination.rs | 5 +- 3 files changed, 59 insertions(+), 22 deletions(-) diff --git a/cli-engine/docs/proposals/cursor-first-pagination.md b/cli-engine/docs/proposals/cursor-first-pagination.md index 2f400fd..e9c4c63 100644 --- a/cli-engine/docs/proposals/cursor-first-pagination.md +++ b/cli-engine/docs/proposals/cursor-first-pagination.md @@ -92,9 +92,9 @@ Registers `--limit`/`--continue` the same way `with_pagination` registers ### Envelope changes -Today's `PaginationMeta { total, offset, limit, count, has_more }` assumes both `total` and `offset` are always known — true for a client-side slice, not guaranteed for a real cursor API that may never report a true count. The `.with_cursor` counterpart needs `total`/`offset` to become optional (present when an adapter can supply them, absent for a pure opaque cursor) and add `continue_from: Option`. +Today's `PaginationMeta { total, offset, limit, count, has_more }` assumes both `total` and `offset` are always known — true for a client-side slice, not guaranteed for a real cursor API that may never report a true count. `offset` itself has no cursor counterpart at all — there is no "skip N" concept for an opaque, forward-only token. The shipped `.with_cursor` counterpart is `CursorMeta { limit, count, total: Option, remaining: Option, continue_from: Option, has_more, self_sufficient_limit }`: `total`/`remaining` are optional (present only when an adapter's backend reports them), and `self_sufficient_limit` records whether the handler's `continue_from` token already carries its own effective page size — see [concepts.md](../concepts.md#cursor-pagination) for the full contract. -Human output changes correspondingly when a total is unknown: "Showing 25 items so far — run with `--continue ` for more" instead of "Showing 25 of 143 rows, offset 0, limit 25". When a total *is* available (some cursor backends do report one, and every client-side-slice command still knows its own total), the existing "N of M" phrasing still applies. +Human output changes correspondingly when a total is unknown: "N rows so far; use --limit L --continue for more" instead of "Showing 25 of 143 rows, offset 0, limit 25" (the `--limit` clause is omitted exactly when `self_sufficient_limit` is set). When a total *is* available (some cursor backends do report one, and every client-side-slice command still knows its own total), the existing "N of M" phrasing still applies. `next_actions` needs no new mechanism — it already replays every flag the user passed and appends an updated pagination flag; for a `.with_cursor` command it appends `--continue `. diff --git a/cli-engine/src/cli/flags_apply.rs b/cli-engine/src/cli/flags_apply.rs index 05cd6aa..2be580c 100644 --- a/cli-engine/src/cli/flags_apply.rs +++ b/cli-engine/src/cli/flags_apply.rs @@ -211,33 +211,53 @@ fn pagination_arg_display(value: &serde_json::Value) -> String { /// is printed — is rendered as a literal `\xHH`/`\n`/`\r`/`\t` placeholder /// rather than passed through raw. /// -/// Display-safe, not round-trip-safe: a plain shell does not decode `\n`/ -/// `\xHH` inside a double-quoted string back into the original byte, so a -/// value containing a control character cannot be copy-pasted back into an -/// exact resend — a deliberate trade-off, since the alternative (an escape -/// a shell *would* decode, e.g. ANSI-C `$'...'` quoting) is not POSIX and -/// would make every other, ordinary replayed value non-portable to gain -/// exact reproduction for a case that, in practice, only a malformed or -/// adversarial backend cursor token would ever hit. +/// Display-safe, not round-trip-safe for a control character: a plain shell +/// does not decode `\n`/`\xHH` inside a double-quoted string back into the +/// original byte, so a value containing one cannot be copy-pasted back into +/// an exact resend — a deliberate trade-off, since the alternative (an +/// escape a shell *would* decode, e.g. ANSI-C `$'...'` quoting) is not +/// POSIX and would make every other, ordinary replayed value non-portable +/// to gain exact reproduction for a case that, in practice, only a +/// malformed or adversarial backend cursor token would ever hit. +/// +/// `!` gets different treatment because a fix that *does* both round-trip +/// and stay safe exists: interactive Bash performs history expansion on an +/// unescaped `!` even inside double quotes (so `"a!b"` can expand against +/// history or fail with "event not found"), and backslash-escaping it +/// there leaves the backslash itself in the resulting argument (`\!`, not +/// `!` — its own round-trip failure, verified against a real Bash). A +/// single-quoted segment is immune to history expansion and, spliced +/// between double-quoted segments with no separator, still concatenates +/// into one argument (`"a"'!'"b"` parses as the single word `a!b`) — this +/// stitches every `!` in as its own single-quoted segment instead. pub(crate) fn quote_pagination_value(value: &str) -> String { let safe_unquoted = |c: char| c.is_ascii_alphanumeric() || matches!(c, '-' | '_' | '.' | '/' | ':' | '@'); if value.is_empty() || !value.chars().all(safe_unquoted) { - let mut escaped = String::with_capacity(value.len()); + let mut out = String::with_capacity(value.len() + 2); + let mut segment = String::new(); + out.push('"'); for c in value.chars() { match c { - '\\' => escaped.push_str("\\\\"), - '"' => escaped.push_str("\\\""), - '$' => escaped.push_str("\\$"), - '`' => escaped.push_str("\\`"), - '\n' => escaped.push_str("\\n"), - '\r' => escaped.push_str("\\r"), - '\t' => escaped.push_str("\\t"), - c if c.is_control() => escaped.push_str(&format!("\\x{:02x}", c as u32)), - c => escaped.push(c), + '!' => { + out.push_str(&segment); + segment.clear(); + out.push_str("\"'!'\""); + } + '\\' => segment.push_str("\\\\"), + '"' => segment.push_str("\\\""), + '$' => segment.push_str("\\$"), + '`' => segment.push_str("\\`"), + '\n' => segment.push_str("\\n"), + '\r' => segment.push_str("\\r"), + '\t' => segment.push_str("\\t"), + c if c.is_control() => segment.push_str(&format!("\\x{:02x}", c as u32)), + c => segment.push(c), } } - format!("\"{escaped}\"") + out.push_str(&segment); + out.push('"'); + out } else { value.to_owned() } @@ -631,4 +651,18 @@ mod quote_pagination_value_tests { assert_eq!(quote_pagination_value("tok-2"), "tok-2"); assert_eq!(quote_pagination_value("a b;c"), "\"a b;c\""); } + + #[test] + fn bang_is_spliced_into_its_own_single_quoted_segment() { + // Verified against a real interactive Bash (with history expansion + // enabled) that this exact splicing both round-trips to the + // original value and never triggers history expansion, unlike a + // backslash-escaped `\!` (which leaves the backslash itself in the + // resulting argument) or a bare `!` inside double quotes (which can + // silently substitute in unrelated history text). + assert_eq!(quote_pagination_value("a!b"), "\"a\"'!'\"b\""); + assert_eq!(quote_pagination_value("!abc"), "\"\"'!'\"abc\""); + assert_eq!(quote_pagination_value("abc!"), "\"abc\"'!'\"\""); + assert_eq!(quote_pagination_value("a!!b"), "\"a\"'!'\"\"'!'\"b\""); + } } diff --git a/cli-engine/tests/pagination.rs b/cli-engine/tests/pagination.rs index 91cd539..23a0be0 100644 --- a/cli-engine/tests/pagination.rs +++ b/cli-engine/tests/pagination.rs @@ -414,7 +414,10 @@ async fn next_page_action_preserves_filter_expr_and_fields() { let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); assert_eq!( rendered["next_actions"][0]["command"], - "my-cli list --filter \"name != 'alpha'\" --expr \"sort_by(@, &name)\" --fields name --limit 2 --offset 2" + // `!` is spliced into its own single-quoted segment (immune to Bash + // history expansion) rather than left bare inside the double + // quotes — see `quote_pagination_value`. + "my-cli list --filter \"name \"'!'\"= 'alpha'\" --expr \"sort_by(@, &name)\" --fields name --limit 2 --offset 2" ); } From ccd5a31853eb615203abbe9119c4ad6c382270c4 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 16:53:03 -0700 Subject: [PATCH 10/14] fix: escape arbitrary control characters in TOON string output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit is_safe_unquoted only forced the quoted path for \n/\r/\t, and escape_string only escaped those three plus \\/" — any other control character (e.g. ESC, the start of most ANSI escape sequences) rendered completely unquoted and unescaped, straight to a terminal running --output toon. Not specific to cursor pagination's continue_from (any string field goes through this same encoder), but this PR's opaque, backend-controlled token is what surfaced it: unlike the shell-quoting fix for the suggested next-page command, nothing sanitized the raw field value itself in TOON's own encoding. Co-Authored-By: Claude Sonnet 5 --- cli-engine/src/output/toon.rs | 30 +++++++++++++++++++++++------- cli-engine/tests/foundation.rs | 15 +++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/cli-engine/src/output/toon.rs b/cli-engine/src/output/toon.rs index aec936d..a7f8b10 100644 --- a/cli-engine/src/output/toon.rs +++ b/cli-engine/src/output/toon.rs @@ -283,7 +283,12 @@ fn is_safe_unquoted(value: &str) -> bool { && !value.contains('\\') && !value.contains(',') && !value.contains(['[', ']', '{', '}']) - && !value.contains(['\n', '\r', '\t']) + // A response field can be backend-controlled (e.g. a cursor + // continuation token) rather than authored by this crate — any + // control character, not just the three with a named escape below, + // forces the quoted path so `escape_string` gets a chance to + // neutralize it instead of it reaching the terminal raw. + && !value.chars().any(|ch| ch.is_control()) && !value.starts_with('-') } @@ -304,12 +309,23 @@ fn is_valid_unquoted_key(key: &str) -> bool { } fn escape_string(value: &str) -> String { - value - .replace('\\', "\\\\") - .replace('"', "\\\"") - .replace('\n', "\\n") - .replace('\r', "\\r") - .replace('\t', "\\t") + let mut escaped = String::with_capacity(value.len()); + for c in value.chars() { + match c { + '\\' => escaped.push_str("\\\\"), + '"' => escaped.push_str("\\\""), + '\n' => escaped.push_str("\\n"), + '\r' => escaped.push_str("\\r"), + '\t' => escaped.push_str("\\t"), + // Any other control character (e.g. ESC, the start of most ANSI + // escape sequences) — not just the three above with a named + // escape — gets a `\xHH` placeholder rather than passing through + // raw to a terminal rendering this output. + c if c.is_control() => escaped.push_str(&format!("\\x{:02x}", c as u32)), + c => escaped.push(c), + } + } + escaped } fn push_line(lines: &mut Vec, depth: usize, line: String) { diff --git a/cli-engine/tests/foundation.rs b/cli-engine/tests/foundation.rs index 0c96224..56ebb65 100644 --- a/cli-engine/tests/foundation.rs +++ b/cli-engine/tests/foundation.rs @@ -9269,6 +9269,21 @@ fn toon_renderer_covers_nested_empty_and_escaped_goldens() { } } +/// A string field can be backend-controlled (e.g. a cursor continuation +/// token), not authored by this crate — a raw control character (ESC, the +/// start of most ANSI escape sequences) must never reach the terminal +/// unescaped just because it isn't one of the three with a named escape +/// (`\n`/`\r`/`\t`). +#[test] +fn toon_renderer_escapes_arbitrary_control_characters() { + let envelope = Envelope::success(json!({"continue_from": "a\x1b[31mb"}), "things-api") + .prepare_for_render(""); + assert_eq!( + render(OutputFormat::Toon, &envelope).expect("toon render should succeed"), + "data:\n continue_from: \"a\\x1b[31mb\"" + ); +} + #[test] fn toon_renderer_covers_nested_array_and_non_tabular_object_paths() { let envelope = Envelope::success( From 9977fcd2bfa0b19991a140d7a12c6d9803300187 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 17:15:18 -0700 Subject: [PATCH 11/14] fix: cursor.count uses raw page size, TOON uses valid \u escape, doc gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cursor.count was measured from envelope.data after apply_pipeline ran, so an --expr that filters but keeps the result an array (e.g. a JMESPath predicate) silently substituted the post-expression display count for the page the handler's backend call actually returned. PaginationMeta.count already avoids this — apply_pipeline captures it internally, before --expr runs — but cursor metadata is built entirely outside apply_pipeline, so it needed its own pre-pipeline snapshot. Added a regression test filtering a 2-item page down to 1 displayed row and asserting count still reads 2. Also fixed \xHH from the previous TOON control-character fix: not a valid JSON/TOON string escape (every other escape there is JSON-style), so it would itself make the rendered output unparseable. Uses \uXXXX now, matching the rest of the encoder. Doc fixes: CommandSpec::pagination/cursor field docs corrected (each being None doesn't mean no --limit at all — the other might still register one), and docs/design.md now describes with_cursor alongside with_pagination instead of omitting it entirely. Co-Authored-By: Claude Sonnet 5 --- cli-engine/docs/design.md | 27 ++++++++++++++-------- cli-engine/src/command/spec.rs | 30 +++++++++++++++--------- cli-engine/src/middleware/run.rs | 17 +++++++++++++- cli-engine/src/output/toon.rs | 9 +++++--- cli-engine/tests/cursor_pagination.rs | 33 +++++++++++++++++++++++++++ cli-engine/tests/foundation.rs | 2 +- 6 files changed, 93 insertions(+), 25 deletions(-) diff --git a/cli-engine/docs/design.md b/cli-engine/docs/design.md index f201c1a..cad986e 100644 --- a/cli-engine/docs/design.md +++ b/cli-engine/docs/design.md @@ -292,11 +292,17 @@ Framework global flags populate middleware and apply consistently to every comma Applications can add their own global flags with `CliConfig::with_register_flags` and copy parsed values into middleware with `CliConfig::with_apply_flags`. -`--limit`/`--offset` are the one exception: they are not global at all. A command registers them -for itself with `CommandSpec::with_pagination(PaginationConfig::new(default_limit, max_limit))`; -a command that never calls this has neither flag, in `--help` or on its command line. -`default_limit` applies when the user passes neither flag; `max_limit` (when non-zero) rejects an -explicit `--limit` above the cap. +Pagination flags are the one exception: they are not global at all, and a command registers at +most one of two mutually exclusive styles for itself. `CommandSpec::with_pagination(PaginationConfig::new(default_limit, max_limit))` +registers `--limit`/`--offset`, purely client-side (the engine slices whatever array the handler +returns). `CommandSpec::with_cursor(CursorConfig::new(default_limit, max_limit))` registers +`--limit`/`--continue` instead, for a backend with its own server-maintained, forward-only cursor — +the engine never slices or measures a cursor itself; the handler reads the parsed values back off +`Middleware::cursor_limit`/`.continue_token` and reports what it learned via +`CommandResult::with_cursor`. A command that calls neither has none of these flags, in `--help` or +on its command line. `default_limit` applies when the user passes neither flag; `max_limit` (when +non-zero) rejects an explicit `--limit` above the cap. See [concepts.md](concepts.md#cursor-pagination) +for the full cursor contract. ## Middleware @@ -365,10 +371,13 @@ Handlers return JSON-serializable data and a system id. Middleware wraps the res - `fix` (optional recovery guidance on failed commands) Metadata is omitted unless `--verbose` is requested. Selective metadata is supported with -comma-separated verbose fields. `pagination`, unlike `metadata`, is never gated by `--verbose` — -a caller relies on it to know whether more data exists at all. It's still conditional on -pagination actually running, though: a paginating command with `default_limit: 0` ("unlimited") -and neither flag passed produces no `pagination` field at all. +comma-separated verbose fields. `pagination`/`cursor`, unlike `metadata`, are never gated by +`--verbose` — a caller relies on them to know whether more data exists at all. `pagination` is +still conditional on pagination actually running, though: a paginating command with +`default_limit: 0` ("unlimited") and neither flag passed produces no `pagination` field at all. +`cursor` is present whenever a `with_cursor` command returned array data, regardless of `--limit`/ +`--continue`, since the engine can't measure a cursor itself the way it slices an offset — see +[concepts.md](concepts.md#cursor-pagination). The output pipeline runs in this order: diff --git a/cli-engine/src/command/spec.rs b/cli-engine/src/command/spec.rs index 48f5ab9..47afe25 100644 --- a/cli-engine/src/command/spec.rs +++ b/cli-engine/src/command/spec.rs @@ -115,21 +115,29 @@ pub struct CommandSpec { /// ancestor chain happens when a [`Cli`](crate::Cli) mounts the enclosing /// module or group. pub feature_flag: Option, - /// This command's opt-in pagination policy, if any. + /// This command's opt-in offset-pagination policy, if any. /// - /// `None` (the default) means the command does not paginate: `--limit`/ - /// `--offset` are not registered for it, so they neither show up in its - /// `--help` nor parse on its command line. Set with - /// [`with_pagination`](CommandSpec::with_pagination). Mutually exclusive - /// with [`cursor`](CommandSpec::cursor). + /// `None` (the default) means this command doesn't register `--offset` + /// (and doesn't register `--limit` for the offset-pagination reading of + /// it) — but that alone doesn't mean the command has no `--limit` at + /// all: [`cursor`](CommandSpec::cursor) registers its own `--limit` + /// alongside `--continue`. A command has at most one of `pagination`/ + /// `cursor` set (mutually exclusive, enforced at registration), so + /// exactly one of the two config docs describes any given `--limit` + /// that shows up in `--help`. Set with + /// [`with_pagination`](CommandSpec::with_pagination). pub pagination: Option, /// This command's opt-in cursor-pagination policy, if any. /// - /// `None` (the default) means the command does not register `--limit`/ - /// `--continue`. Set with [`with_cursor`](CommandSpec::with_cursor) for a - /// command backed by a server-maintained, forward-only cursor API, where - /// client-side offset slicing would cost O(N²) requests to page through. - /// Mutually exclusive with [`pagination`](CommandSpec::pagination). + /// `None` (the default) means this command doesn't register `--continue` + /// (and doesn't register `--limit` for the cursor-pagination reading of + /// it) — but that alone doesn't mean the command has no `--limit` at + /// all: [`pagination`](CommandSpec::pagination) registers its own + /// `--limit` alongside `--offset`. Same mutual-exclusivity note as + /// `pagination`. Set with [`with_cursor`](CommandSpec::with_cursor) for + /// a command backed by a server-maintained, forward-only cursor API, + /// where client-side offset slicing would cost O(N²) requests to page + /// through. pub cursor: Option, } diff --git a/cli-engine/src/middleware/run.rs b/cli-engine/src/middleware/run.rs index 9525549..23f62a6 100644 --- a/cli-engine/src/middleware/run.rs +++ b/cli-engine/src/middleware/run.rs @@ -541,6 +541,21 @@ impl Middleware { } } let projection_fields = if human_view { "" } else { effective_fields }; + // Captured before `apply_pipeline` runs below: cursor metadata + // describes the page the handler's own backend call actually + // returned, not whatever `--expr` reshapes it into for display. An + // `--expr` that keeps the result an array (e.g. a JMESPath filter) + // would otherwise silently substitute the post-expression display + // count for the real page size — `PaginationMeta.count` already + // avoids this because `apply_pipeline` captures it internally, at + // the pagination step, before `--expr` runs; cursor metadata is + // built entirely outside `apply_pipeline`, so it needs its own + // snapshot instead. + let raw_cursor_array_len = envelope + .data + .as_ref() + .and_then(Value::as_array) + .map(|items| items.len() as i64); if let Some(data) = &mut envelope.data { // A cursor command never wants pipeline-level slicing — the // handler already returned exactly the page its own backend call @@ -596,7 +611,7 @@ impl Middleware { // at all, mirroring offset pagination's identical guard in // `apply_pagination`, rather than advertising a bogus page over // data that was never actually paginated. - let count = items.len() as i64; + let count = raw_cursor_array_len.unwrap_or(items.len() as i64); let continuation = cursor_continuation.unwrap_or_default(); let has_more = continuation.continue_from.is_some(); // A handler that reported an effective limit is telling us its diff --git a/cli-engine/src/output/toon.rs b/cli-engine/src/output/toon.rs index a7f8b10..c8a08de 100644 --- a/cli-engine/src/output/toon.rs +++ b/cli-engine/src/output/toon.rs @@ -319,9 +319,12 @@ fn escape_string(value: &str) -> String { '\t' => escaped.push_str("\\t"), // Any other control character (e.g. ESC, the start of most ANSI // escape sequences) — not just the three above with a named - // escape — gets a `\xHH` placeholder rather than passing through - // raw to a terminal rendering this output. - c if c.is_control() => escaped.push_str(&format!("\\x{:02x}", c as u32)), + // escape — gets a `\uXXXX` escape rather than passing through + // raw to a terminal rendering this output. Every other escape + // here is JSON-style, so this must be too (`\xHH` is not valid + // JSON/TOON string syntax and would itself make the output + // unparseable, defeating machine-readable TOON). + c if c.is_control() => escaped.push_str(&format!("\\u{:04x}", c as u32)), c => escaped.push(c), } } diff --git a/cli-engine/tests/cursor_pagination.rs b/cli-engine/tests/cursor_pagination.rs index 1b990bc..e7b742e 100644 --- a/cli-engine/tests/cursor_pagination.rs +++ b/cli-engine/tests/cursor_pagination.rs @@ -275,6 +275,39 @@ async fn cursor_metadata_is_absent_after_expr_reshapes_data_to_a_scalar() { assert!(rendered.get("cursor").is_none(), "{}", output.rendered); } +/// `cursor.count` must describe the page the handler's backend call +/// actually returned, not whatever `--expr` reshapes it into for display — +/// unlike the scalar case above, an `--expr` that filters but keeps the +/// result an array doesn't trip the "not an array" guard, so this exercises +/// the count itself rather than cursor metadata's presence. +#[tokio::test] +async fn cursor_count_reflects_the_raw_page_size_not_the_expr_filtered_display_count() { + let cli = cli_with_cursor_list_command( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig::new(2, 0)), + ); + + let output = cli + .run([ + "my-cli", + "list", + "--expr", + "[?name=='alpha']", + "--output", + "json", + ]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!(rendered["data"], json!([{"name": "alpha"}])); + assert_eq!( + rendered["cursor"]["count"], 2, + "count must reflect the real 2-item page, not the 1 item --expr left displayed: {}", + output.rendered + ); +} + /// A handler that derives its own effective page size from the `--continue` /// token (e.g. to let a caller resume with `--continue` alone, without /// repeating `--limit`) reports that via `CursorContinuation::with_limit`. diff --git a/cli-engine/tests/foundation.rs b/cli-engine/tests/foundation.rs index 56ebb65..f2e2743 100644 --- a/cli-engine/tests/foundation.rs +++ b/cli-engine/tests/foundation.rs @@ -9280,7 +9280,7 @@ fn toon_renderer_escapes_arbitrary_control_characters() { .prepare_for_render(""); assert_eq!( render(OutputFormat::Toon, &envelope).expect("toon render should succeed"), - "data:\n continue_from: \"a\\x1b[31mb\"" + "data:\n continue_from: \"a\\u001b[31mb\"" ); } From 63566dee24cd2de98e2da21bdc59ca781a2ab044 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 17:30:50 -0700 Subject: [PATCH 12/14] fix: require pre-pipeline array snapshot for cursor metadata, add #[must_use] The round-8 fix for cursor.count's --expr timing bug introduced its own gap: the guard still only checked the post-pipeline shape, so a handler that returned a non-array result (never a real cursor page) could still get cursor metadata and a next-page action attached if --expr happened to synthesize an array from it (e.g. [@], wrapping a scalar/object in a single-element list). Both checks are now required: the pre-pipeline snapshot must be Some (the handler's raw result was an array) AND the post-pipeline data must still be an array (catches the opposite direction, --expr reshaping a real page into a scalar). Added a regression test for the newly-caught direction. Also added #[must_use] to MiddlewareRequest::new and its with_* builders, matching the existing convention on this crate's other consuming constructors/builders (Middleware::new, PaginationConfig::new, ...). Co-Authored-By: Claude Sonnet 5 --- cli-engine/src/middleware/mod.rs | 6 +++++ cli-engine/src/middleware/run.rs | 12 +++++++--- cli-engine/tests/cursor_pagination.rs | 33 +++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/cli-engine/src/middleware/mod.rs b/cli-engine/src/middleware/mod.rs index 8075379..64248e1 100644 --- a/cli-engine/src/middleware/mod.rs +++ b/cli-engine/src/middleware/mod.rs @@ -641,6 +641,7 @@ impl<'request> MiddlewareRequest<'request> { /// everything else defaulted (`auth: AuthRequirement::Required`, /// `view_id`/`pagination_command`/`cursor_command`: `None`, `raw_output: /// false`). Chain the `with_*` methods below for anything else. + #[must_use] pub fn new( meta: CommandMeta, command_path: &'request str, @@ -661,18 +662,21 @@ impl<'request> MiddlewareRequest<'request> { } /// Sets the authentication requirement enforced for this command. + #[must_use] pub fn with_auth(mut self, auth: AuthRequirement) -> Self { self.auth = auth; self } /// Sets the human view id this command declared. + #[must_use] pub fn with_view_id(mut self, view_id: &'request str) -> Self { self.view_id = Some(view_id); self } /// Sets whether a successful string result renders verbatim. + #[must_use] pub fn with_raw_output(mut self, raw_output: bool) -> Self { self.raw_output = raw_output; self @@ -683,6 +687,7 @@ impl<'request> MiddlewareRequest<'request> { /// Clears `cursor_command` — a command replays as one pagination style or /// the other, never both; setting one via its builder is how a caller /// signals the other no longer applies. + #[must_use] pub fn with_pagination_command(mut self, pagination_command: impl Into) -> Self { self.pagination_command = Some(pagination_command.into()); self.cursor_command = None; @@ -692,6 +697,7 @@ impl<'request> MiddlewareRequest<'request> { /// Sets the replayable command text for cursor pagination's `next_actions`. /// /// Clears `pagination_command` — see [`with_pagination_command`](Self::with_pagination_command). + #[must_use] pub fn with_cursor_command(mut self, cursor_command: impl Into) -> Self { self.cursor_command = Some(cursor_command.into()); self.pagination_command = None; diff --git a/cli-engine/src/middleware/run.rs b/cli-engine/src/middleware/run.rs index 23f62a6..f0c56ed 100644 --- a/cli-engine/src/middleware/run.rs +++ b/cli-engine/src/middleware/run.rs @@ -602,16 +602,22 @@ impl Middleware { } } if let Some(base) = cursor_command + && let Some(count) = raw_cursor_array_len && let Some(data) = &envelope.data - && let Some(items) = data.as_array() + && data.as_array().is_some() { // Cursor metadata is for array data (per `Envelope::cursor`'s own // contract) — a handler result that isn't an array (or one // `--expr` reshaped into a scalar/object) gets no cursor field // at all, mirroring offset pagination's identical guard in // `apply_pagination`, rather than advertising a bogus page over - // data that was never actually paginated. - let count = raw_cursor_array_len.unwrap_or(items.len() as i64); + // data that was never actually paginated. Both checks are + // required, not either/or: `raw_cursor_array_len` catches a + // handler result that was never an array to begin with (even if + // `--expr` later synthesizes one — that's still not a real + // cursor page); the post-pipeline `is_some()` catches the + // opposite direction, `--expr` reshaping a real page into a + // scalar. let continuation = cursor_continuation.unwrap_or_default(); let has_more = continuation.continue_from.is_some(); // A handler that reported an effective limit is telling us its diff --git a/cli-engine/tests/cursor_pagination.rs b/cli-engine/tests/cursor_pagination.rs index e7b742e..9fd78fc 100644 --- a/cli-engine/tests/cursor_pagination.rs +++ b/cli-engine/tests/cursor_pagination.rs @@ -253,6 +253,39 @@ async fn cursor_metadata_is_absent_when_the_handler_result_is_not_an_array() { ); } +/// The inverse direction from the non-array test above: the handler's raw +/// result was never an array, but `--expr` happens to synthesize one +/// (`[@]`, wrapping the object in a single-element list) — this must still +/// produce no cursor metadata, since there was never a real backend page to +/// describe, regardless of what shape `--expr` leaves the *displayed* data +/// in. +#[tokio::test] +async fn cursor_metadata_is_absent_when_expr_synthesizes_an_array_from_a_non_array_result() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig::new(2, 0)), + async |_ctx| { + Ok(CommandResult::new(json!({"name": "alpha"})) + .with_cursor(CursorContinuation::more("tok-2"))) + }, + )); + + let output = cli + .run(["my-cli", "list", "--expr", "[@]", "--output", "json"]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!(rendered["data"], json!([{"name": "alpha"}])); + assert!(rendered.get("cursor").is_none(), "{}", output.rendered); + assert!( + rendered.get("next_actions").is_none(), + "{}", + output.rendered + ); +} + /// Same guard, reached via `--expr` reshaping an originally-array result into /// a scalar rather than the handler returning a non-array result directly — /// `apply_pipeline`'s `--expr` step runs after the cursor block would From 463b19ee79ad2d5efbd5afd5148f6b46a209a4a7 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 17:43:24 -0700 Subject: [PATCH 13/14] fix: self_sufficient_limit requires a token, not just with_limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CursorContinuation::done().with_limit(n) is a handler misuse — there's no continue_from for n to describe as self-sufficient — but self_sufficient_limit was set purely from continuation.limit.is_some(), so this would claim a nonexistent token is self-sufficient about page size. Gated it on has_more (already computed, and continuation.continue_from itself is unavailable by this point in the struct literal since it's moved into the continue_from field first). Added a regression test. Co-Authored-By: Claude Sonnet 5 --- cli-engine/src/middleware/run.rs | 7 ++++++- cli-engine/tests/cursor_pagination.rs | 30 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/cli-engine/src/middleware/run.rs b/cli-engine/src/middleware/run.rs index f0c56ed..9fbfad1 100644 --- a/cli-engine/src/middleware/run.rs +++ b/cli-engine/src/middleware/run.rs @@ -652,7 +652,12 @@ impl Middleware { remaining: continuation.remaining, continue_from: continuation.continue_from, has_more, - self_sufficient_limit: continuation.limit.is_some(), + // `with_limit` only means anything relative to a token to + // resume with — `CursorContinuation::done().with_limit(n)` + // is a handler misuse (there's no `continue_from` for `n` + // to describe), and must not claim self-sufficiency about a + // token that doesn't exist. + self_sufficient_limit: has_more && continuation.limit.is_some(), }); } envelope.with_context( diff --git a/cli-engine/tests/cursor_pagination.rs b/cli-engine/tests/cursor_pagination.rs index 9fd78fc..fa51275 100644 --- a/cli-engine/tests/cursor_pagination.rs +++ b/cli-engine/tests/cursor_pagination.rs @@ -382,6 +382,36 @@ async fn with_limit_overrides_the_envelope_and_omits_limit_from_the_next_action( ); } +/// `with_limit` only means anything relative to a token to resume with — +/// calling it on `CursorContinuation::done()` (a handler misuse: there's no +/// `continue_from` for the reported limit to describe) must not claim +/// `self_sufficient_limit`, since there's no token for it to be +/// self-sufficient *about*. +#[tokio::test] +async fn self_sufficient_limit_is_false_on_a_completed_page_even_if_with_limit_was_called() { + let mut cli = Cli::new(CliConfig::new("my-cli", "Dev tooling", "my-cli")); + cli.add_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("list", "List things") + .no_auth(true) + .with_cursor(CursorConfig::new(2, 0)), + async |_ctx| { + Ok(CommandResult::new(json!([{"name": "alpha"}])) + .with_cursor(CursorContinuation::done().with_limit(2))) + }, + )); + + let output = cli.run(["my-cli", "list", "--output", "json"]).await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!(rendered["cursor"]["has_more"], false); + assert_eq!(rendered["cursor"]["self_sufficient_limit"], json!(false)); + assert!( + rendered.get("next_actions").is_none(), + "{}", + output.rendered + ); +} + #[tokio::test] async fn max_limit_rejects_an_explicit_limit_above_the_cap_but_allows_the_cap_itself() { let cli = cli_with_cursor_list_command( From 1d936a5dc9cae6fd7821b6fee8fcc6f3dfd884dd Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 16 Sep 2026 17:56:15 -0700 Subject: [PATCH 14/14] docs: self_sufficient_limit requires has_more, not just with_limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fallout of the previous fix (self_sufficient_limit is now has_more && continuation.limit.is_some(), not just the latter) — both the CursorMeta field doc and concepts.md's cursor-pagination section still described the old, incomplete condition. Co-Authored-By: Claude Sonnet 5 --- cli-engine/docs/concepts.md | 2 +- cli-engine/src/output/envelope.rs | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/cli-engine/docs/concepts.md b/cli-engine/docs/concepts.md index 8284545..c8a52ea 100644 --- a/cli-engine/docs/concepts.md +++ b/cli-engine/docs/concepts.md @@ -514,7 +514,7 @@ literal follow-up command instead of having to compute the next offset themselve ### cursor pagination -A command that opted into `--limit`/`--continue` cursor pagination via `CommandSpec::with_cursor` gets a top-level `cursor` field on the envelope instead of `pagination` — `limit`, `count`, `total`, `remaining`, `continue_from`, `has_more`, and `self_sufficient_limit` — whenever it returned array data. Unlike `pagination`, the engine cannot compute this itself: a cursor is opaque to everything except the handler that called the backend, so `count` is the only piece the engine derives itself (the returned array's length); `limit` defaults to the parsed `--limit`, but a handler can override it via `CursorContinuation::with_limit` to report the effective page size it actually resumed with (e.g. one decoded from `continue_from` itself) — `self_sufficient_limit` is `true` exactly when that override happened, meaning `continue_from` alone is enough to resume and a replay command can omit `--limit`; `total`/`remaining`/`continue_from` come from whatever the handler reported via `CommandResult::with_cursor(CursorContinuation::more(token).with_total(n).with_remaining(n))` — or `CursorContinuation::done()` (or no call at all) to report the end of iteration. `total`/`remaining` are `None` when the backend never reports them, which a pure opaque-cursor API is not obligated to do. +A command that opted into `--limit`/`--continue` cursor pagination via `CommandSpec::with_cursor` gets a top-level `cursor` field on the envelope instead of `pagination` — `limit`, `count`, `total`, `remaining`, `continue_from`, `has_more`, and `self_sufficient_limit` — whenever it returned array data. Unlike `pagination`, the engine cannot compute this itself: a cursor is opaque to everything except the handler that called the backend, so `count` is the only piece the engine derives itself (the returned array's length); `limit` defaults to the parsed `--limit`, but a handler can override it via `CursorContinuation::with_limit` to report the effective page size it actually resumed with (e.g. one decoded from `continue_from` itself) — `self_sufficient_limit` is `true` exactly when a next page exists *and* that override happened (`with_limit` alone isn't enough — calling it on a completed `CursorContinuation::done()`, with no `continue_from` at all, must not claim a nonexistent token is self-sufficient), meaning `continue_from` alone is enough to resume and a replay command can omit `--limit`; `total`/`remaining`/`continue_from` come from whatever the handler reported via `CommandResult::with_cursor(CursorContinuation::more(token).with_total(n).with_remaining(n))` — or `CursorContinuation::done()` (or no call at all) to report the end of iteration. `total`/`remaining` are `None` when the backend never reports them, which a pure opaque-cursor API is not obligated to do. Human output merges this into the table's row-count footer: `(N of M rows)` when a total is known, `(N rows, M remaining)` when only a remaining count is known, or `(N rows so far; use --limit L --continue for more)` when neither is known — `--limit L` is omitted from this hint exactly when `self_sufficient_limit` is `true`, matching the `next_actions` entry below. A cursor-paginated response that doesn't render as a table gets the standalone counterpart: `Showing N of M`, `Showing N (M remaining)`, or `Showing N items so far; use --limit L --continue for more` (same `--limit` omission rule). diff --git a/cli-engine/src/output/envelope.rs b/cli-engine/src/output/envelope.rs index a15e141..8beb730 100644 --- a/cli-engine/src/output/envelope.rs +++ b/cli-engine/src/output/envelope.rs @@ -204,14 +204,17 @@ pub struct CursorMeta { pub continue_from: Option, /// Whether more data is available (`continue_from.is_some()`). pub has_more: bool, - /// Whether `continue_from` alone is sufficient to resume at `limit` - /// (the handler called - /// [`CursorContinuation::with_limit`](crate::CursorContinuation::with_limit)), - /// so a replay command can omit `--limit` — the same condition the - /// engine uses to decide whether `next_actions` includes it. `false` for - /// a plain [`CursorContinuation::more`](crate::CursorContinuation::more) - /// token, where `limit` is just the parsed `--limit` and resuming with a - /// different one could change page size. + /// Whether `continue_from` alone is sufficient to resume at `limit` — + /// `true` only when `has_more` *and* the handler called + /// [`CursorContinuation::with_limit`](crate::CursorContinuation::with_limit), + /// so a replay command can omit `--limit`. `with_limit` on its own isn't + /// enough: calling it on a completed + /// [`CursorContinuation::done`](crate::CursorContinuation::done) (no + /// `continue_from` at all) would otherwise claim a nonexistent token is + /// self-sufficient. `false` for a plain + /// [`CursorContinuation::more`](crate::CursorContinuation::more) token + /// with no `with_limit` call, where `limit` is just the parsed `--limit` + /// and resuming with a different one could change page size. pub self_sufficient_limit: bool, }