output: page only long read-only output, not writes or status #236

Merged

stephen merged 1 commit from fix/pager-only-long-reads into main 2026-07-30 18:54:49 +00:00

Conversation 2 Commits 1 Files changed 6 +379 -21

stephen commented 2026-07-30 18:13:11 +00:00

Owner

Copy link

Problem

There was exactly one pager call site, cli::run, and it matched on the top-level Command variant:

Command::Repo(_) | Command::Issue(_) | Command::Pr(_) | Command::Release(_)
| Command::Search(_) | Command::Status(_) | Command::Label(_)
| Command::Milestone(_) | Command::Tag(_) | Command::Run(_)
| Command::Api(_) => crate::output::pager::maybe_start(cli.no_pager),

A Command variant is a command family, not a command. Matching Command::Pr(_) pages pr merge, pr close, pr status, and pr list --json exactly as much as it pages pr list. Across those eleven families that is roughly ninety leaf subcommands routed into less, of which about a dozen produce output long enough to want it. The single carve-out, run watch, is the shape of the bug in miniature: someone hit a concrete symptom (a live stream buffering) and patched that one leaf rather than the granularity.

Most of the rest was invisible, because the default less -FRX quits when output fits one screen, so ✓ Merged pull request #12 looked fine. It stopped looking fine in three places:

• A $PAGER without -F. fj deliberately does not fight the user's $PAGER, which is right, but it meant every write confirmation sat behind a keypress.
• --json. Machine output routed through a program built for humans. The non-TTY check saves fj pr list --json | jq; it does not save reading the JSON on a terminal first, which is how anyone finds the field they are about to pipe.
• Confirmation prompts. This is the real one. The guard is created before dispatch, so by the time repo delete, release delete, or tag delete reaches its Confirm prompt, stdout is already dup2'd into the pager child. The prompt is written into less; the answer is read from the terminal. The user is confirming a destructive action against a prompt they cannot see.

output::pager::maybe_start itself was fine. Non-TTY, FJ_NO_PAGER, --no-pager, and the -F in the default less -FRX all did their jobs. The wrong thing was the call site.

Fix

Decide per subcommand, in a new cli::paging::should_page(&Command) -> bool. It pages only genuinely long, read-only, human-facing output.

Still paged

Command	Why
repo list / view / starred / branches (listing form)	long listings, repo overview
repo collaborator list, repo deploy-key list	read-only listings
issue list / view, pr list / view	long listings, threads with --comments
pr diff / commits / files / checks	read-only, routinely longer than a screen
release list / view, label list, milestone list, tag list	listings
run list / view / log	job logs are the longest output fj produces
search repos / issues / prs / users	listings
fj api GET and HEAD	raw dumps, frequently thousands of lines

No longer paged

• Status and summary blocks. fj status (a capped notification inbox, and --mark-read is a one-line write), fj pr status (a fixed three-section dashboard).
• Every write and its confirmation. repo create/fork/sync/edit/rename/archive/unarchive/delete/mirror/mirror-sync/watch/unwatch/star/unstar, repo branches create|delete, repo collaborator add|remove, repo deploy-key add|delete, issue create/edit/close/reopen/lock/unlock/delete/pin/unpin/comment/edit-comment/delete-comment/develop, pr create/edit/ready/comment/attach/update-branch/review/request-review/unrequest-review/merge/close/reopen/lock/unlock, release create/edit/delete/upload/delete-asset, label create/edit/delete/clone, milestone create/edit/close/reopen/delete/assign, tag create/delete, run rerun/cancel, and fj api with a non-GET method. Each prints a line or two, and the three that prompt were prompting into the pager.
• Commands whose real work is not stdout. repo clone (git), pr checkout (git), release download and run download (write files, report progress).
• Short single-value reads. tag view and milestone view are fixed blocks of four to six lines. repo topics prints one line. repo set-default prints one line or writes config.
• Machine and browser modes on commands that otherwise page. Any --json, any --web (prints nothing, opens a browser), and api --silent (prints nothing).

Two things worth calling out about the shape:

should_page is a pure function of the parsed clap tree, so the entire table above is unit tested with no terminal, no network, and no auth: Cli::try_parse_from(argv) then assert. Seven tests, ~90 assertions, in src/cli/paging.rs. That is the difference between a table that is right today and one that stays right when a subcommand is added, since a new RepoSub variant now fails to compile until someone classifies it. The matches are deliberately exhaustive rather than _ => false for that reason.

cli::api::pick_method becomes pub(super) so the api rule asks the same question the handler will, rather than reimplementing "GET unless -X or --input" and drifting from it.

Nothing in src/output/pager.rs changed.

Test

cargo build, cargo fmt --check, and cargo clippy --all-targets --all-features -- -D warnings clean. cargo test --all: 690 passed, 0 failed, 2 ignored (the pre-existing env-mutating pair). cli::tests::cov_ignore_stays_in_sync_with_test_modules passes with paging.rs outside COV_IGNORE, which is correct since it is a unit-test target rather than glue.

Behavior was checked end to end under a real pty (script -qec) with FJ_PAGER pointed at a marker script that announces itself on stderr and then cats, against rasterstate/fj on rasterhub.com. Before this change, all ten of these started the pager:

PAGED   status
PAGED   pr status
PAGED   tag view v0.3.0
PAGED   milestone view 1
PAGED   repo topics
PAGED   pr list --json
PAGED   api /user --silent
PAGED   tag create vfake
PAGED   pr merge 99999
PAGED   repo star ...

After, none of them do, and these still do:

PAGED   pr list          PAGED   tag list
PAGED   pr view 234      PAGED   label list
PAGED   pr diff 234      PAGED   run list
PAGED   issue list       PAGED   search repos fj
PAGED   repo list        PAGED   api /user

The write probes ran against rasterstate/nope-does-not-exist so the API 404s before mutating anything; the pager decision is made ahead of dispatch, so the probe is unaffected by the failure.

No regression in the opt-outs, checked on pr list, a command that does page: --no-pager direct, FJ_NO_PAGER=1 direct, stdout piped to a non-TTY direct. The default less -FRX keeps its -F, so even a paged command that returns two rows does not trap.

Not run locally: make coverage-strict, which needs a nightly toolchain and cargo-llvm-cov, neither installed here. CI's gate is COV_MIN=73 and this adds a fully-covered pure module outside COV_IGNORE, so it should move up rather than down.

Unrelated, noticed while testing

fj label create and fj label edit panic during arg parsing on current main:

thread 'main' panicked at src/cli/mod.rs:105:
Mismatch between definition and access of `color`.
Could not downcast to fj::output::ColorChoice, need to downcast to alloc::string::String

Their --color (a String hex value) collides with the global --color (ColorChoice) on clap's arg id. label create has default_value = "ededed", so the collision fires even without the flag. label delete and label list are fine. Reproduced on stock main at c7d8e0f, so it predates this branch and is out of scope here; those two invocations are omitted from the paging tests with a comment saying why. cli::tests::command_tree_is_internally_consistent does not catch it because Command::debug_assert checks definitions, not downcasts. Worth its own issue.

## Problem There was exactly one pager call site, `cli::run`, and it matched on the top-level `Command` variant: ```rust Command::Repo(_) | Command::Issue(_) | Command::Pr(_) | Command::Release(_) | Command::Search(_) | Command::Status(_) | Command::Label(_) | Command::Milestone(_) | Command::Tag(_) | Command::Run(_) | Command::Api(_) => crate::output::pager::maybe_start(cli.no_pager), ``` A `Command` variant is a command *family*, not a command. Matching `Command::Pr(_)` pages `pr merge`, `pr close`, `pr status`, and `pr list --json` exactly as much as it pages `pr list`. Across those eleven families that is roughly ninety leaf subcommands routed into `less`, of which about a dozen produce output long enough to want it. The single carve-out, `run watch`, is the shape of the bug in miniature: someone hit a concrete symptom (a live stream buffering) and patched that one leaf rather than the granularity. Most of the rest was invisible, because the default `less -FRX` quits when output fits one screen, so `✓ Merged pull request #12` looked fine. It stopped looking fine in three places: - **A `$PAGER` without `-F`.** fj deliberately does not fight the user's `$PAGER`, which is right, but it meant every write confirmation sat behind a keypress. - **`--json`.** Machine output routed through a program built for humans. The non-TTY check saves `fj pr list --json | jq`; it does not save reading the JSON on a terminal first, which is how anyone finds the field they are about to pipe. - **Confirmation prompts.** This is the real one. The guard is created *before* `dispatch`, so by the time `repo delete`, `release delete`, or `tag delete` reaches its `Confirm` prompt, stdout is already `dup2`'d into the pager child. The prompt is written into `less`; the answer is read from the terminal. The user is confirming a destructive action against a prompt they cannot see. `output::pager::maybe_start` itself was fine. Non-TTY, `FJ_NO_PAGER`, `--no-pager`, and the `-F` in the default `less -FRX` all did their jobs. The wrong thing was the call site. ## Fix Decide per subcommand, in a new `cli::paging::should_page(&Command) -> bool`. It pages only genuinely long, read-only, human-facing output. **Still paged** | Command | Why | | --- | --- | | `repo list` / `view` / `starred` / `branches` (listing form) | long listings, repo overview | | `repo collaborator list`, `repo deploy-key list` | read-only listings | | `issue list` / `view`, `pr list` / `view` | long listings, threads with `--comments` | | `pr diff` / `commits` / `files` / `checks` | read-only, routinely longer than a screen | | `release list` / `view`, `label list`, `milestone list`, `tag list` | listings | | `run list` / `view` / `log` | job logs are the longest output fj produces | | `search repos` / `issues` / `prs` / `users` | listings | | `fj api` GET and HEAD | raw dumps, frequently thousands of lines | **No longer paged** - **Status and summary blocks.** `fj status` (a capped notification inbox, and `--mark-read` is a one-line write), `fj pr status` (a fixed three-section dashboard). - **Every write and its confirmation.** `repo create/fork/sync/edit/rename/archive/unarchive/delete/mirror/mirror-sync/watch/unwatch/star/unstar`, `repo branches create|delete`, `repo collaborator add|remove`, `repo deploy-key add|delete`, `issue create/edit/close/reopen/lock/unlock/delete/pin/unpin/comment/edit-comment/delete-comment/develop`, `pr create/edit/ready/comment/attach/update-branch/review/request-review/unrequest-review/merge/close/reopen/lock/unlock`, `release create/edit/delete/upload/delete-asset`, `label create/edit/delete/clone`, `milestone create/edit/close/reopen/delete/assign`, `tag create/delete`, `run rerun/cancel`, and `fj api` with a non-GET method. Each prints a line or two, and the three that prompt were prompting into the pager. - **Commands whose real work is not stdout.** `repo clone` (git), `pr checkout` (git), `release download` and `run download` (write files, report progress). - **Short single-value reads.** `tag view` and `milestone view` are fixed blocks of four to six lines. `repo topics` prints one line. `repo set-default` prints one line or writes config. - **Machine and browser modes on commands that otherwise page.** Any `--json`, any `--web` (prints nothing, opens a browser), and `api --silent` (prints nothing). Two things worth calling out about the shape: `should_page` is a pure function of the parsed clap tree, so the entire table above is unit tested with no terminal, no network, and no auth: `Cli::try_parse_from(argv)` then assert. Seven tests, ~90 assertions, in `src/cli/paging.rs`. That is the difference between a table that is right today and one that stays right when a subcommand is added, since a new `RepoSub` variant now fails to compile until someone classifies it. The matches are deliberately exhaustive rather than `_ => false` for that reason. `cli::api::pick_method` becomes `pub(super)` so the api rule asks the same question the handler will, rather than reimplementing "GET unless `-X` or `--input`" and drifting from it. Nothing in `src/output/pager.rs` changed. ## Test `cargo build`, `cargo fmt --check`, and `cargo clippy --all-targets --all-features -- -D warnings` clean. `cargo test --all`: 690 passed, 0 failed, 2 ignored (the pre-existing env-mutating pair). `cli::tests::cov_ignore_stays_in_sync_with_test_modules` passes with `paging.rs` outside `COV_IGNORE`, which is correct since it is a unit-test target rather than glue. Behavior was checked end to end under a real pty (`script -qec`) with `FJ_PAGER` pointed at a marker script that announces itself on stderr and then `cat`s, against `rasterstate/fj` on rasterhub.com. Before this change, all ten of these started the pager: ``` PAGED status PAGED pr status PAGED tag view v0.3.0 PAGED milestone view 1 PAGED repo topics PAGED pr list --json PAGED api /user --silent PAGED tag create vfake PAGED pr merge 99999 PAGED repo star ... ``` After, none of them do, and these still do: ``` PAGED pr list PAGED tag list PAGED pr view 234 PAGED label list PAGED pr diff 234 PAGED run list PAGED issue list PAGED search repos fj PAGED repo list PAGED api /user ``` The write probes ran against `rasterstate/nope-does-not-exist` so the API 404s before mutating anything; the pager decision is made ahead of dispatch, so the probe is unaffected by the failure. No regression in the opt-outs, checked on `pr list`, a command that does page: `--no-pager` direct, `FJ_NO_PAGER=1` direct, stdout piped to a non-TTY direct. The default `less -FRX` keeps its `-F`, so even a paged command that returns two rows does not trap. Not run locally: `make coverage-strict`, which needs a nightly toolchain and `cargo-llvm-cov`, neither installed here. CI's gate is `COV_MIN=73` and this adds a fully-covered pure module outside `COV_IGNORE`, so it should move up rather than down. ## Unrelated, noticed while testing `fj label create` and `fj label edit` panic during arg parsing on current `main`: ``` thread 'main' panicked at src/cli/mod.rs:105: Mismatch between definition and access of `color`. Could not downcast to fj::output::ColorChoice, need to downcast to alloc::string::String ``` Their `--color` (a `String` hex value) collides with the global `--color` (`ColorChoice`) on clap's arg id. `label create` has `default_value = "ededed"`, so the collision fires even without the flag. `label delete` and `label list` are fine. Reproduced on stock `main` at c7d8e0f, so it predates this branch and is out of scope here; those two invocations are omitted from the paging tests with a comment saying why. `cli::tests::command_tree_is_internally_consistent` does not catch it because `Command::debug_assert` checks definitions, not downcasts. Worth its own issue.

stephen added 1 commit 2026-07-30 18:13:11 +00:00

output: page only long read-only output, not writes or status ... 32d71cff53

The pager decision was made on the top-level Command variant, so it was
really a decision about a whole command family. Matching Command::Pr
paged `pr merge`, `pr close`, `pr status`, and `pr list --json` exactly
as much as it paged `pr list`. The same held for repo, issue, release,
label, milestone, tag, run, search, status, and api: 11 families, ~90
leaf subcommands, of which roughly a dozen produce output long enough to
want a pager.

Most of the damage was invisible because the default `less -FRX` quits
when the output fits one screen, so a one-line confirmation looked
normal. It stopped looking normal in three places. A `$PAGER` without
-F, which fj deliberately does not fight, trapped every write behind a
keypress. `--json` piped into a pager is machine output routed through a
program built for humans, and while the non-TTY check saves the common
`fj pr list --json | jq` case, it does not save the one where someone is
reading the JSON on a terminal first. Worst, the guard is created before
dispatch, so the confirm prompts in `repo delete`, `release delete`, and
`tag delete` were writing to a stdout already dup2'd into the pager
child while reading from the terminal, which is a prompt the user
answers without seeing.

Decide per subcommand instead, in cli::paging::should_page. It pages
only genuinely long, read-only, human-facing output:

- list and view: repo/issue/pr/release/label/milestone/tag/run list,
  repo/issue/pr/release view, repo starred, repo branches (listing
  form), repo collaborator/deploy-key list
- pr diff, commits, files, checks
- run log, run view
- search repos/issues/prs/users
- fj api GET and HEAD dumps

and nothing else. Specifically not: status blocks (fj status, pr
status), every write and its confirmation, short single-value reads (tag
view, milestone view, repo topics, repo set-default), run watch, run
download, and any --json, --web, or api --silent invocation of an
otherwise paging command.

The predicate is a pure function of the parsed clap tree, so the whole
table is unit tested without a terminal. `pick_method` in cli::api
becomes pub(super) so the api rule asks the same question the handler
will. Nothing in output::pager changed: the non-TTY check, FJ_NO_PAGER,
--no-pager, and the -F in the default `less -FRX` are all as they were.

Verified under a pty with FJ_PAGER set to a marker script. Before, all
ten of `status`, `pr status`, `tag view`, `milestone view`, `repo
topics`, `pr list --json`, `api --silent`, `tag create`, `pr merge`, and
`repo star` started the pager. After, none do, while `pr list`, `pr
view`, `pr diff`, `issue list`, `tag list`, `label list`, `run list`,
`search repos`, and `api /user` still do. `--no-pager`, FJ_NO_PAGER=1,
and a piped stdout still suppress it on a command that would otherwise
page.

forgejo-actions commented 2026-07-30 18:14:38 +00:00

Copy link

Forseti review

No blocking findings from the lead reviewer.

No inline findings.

• PR: rasterstate/fj#236
• Head SHA: 32d71cff5337
• Review job: sha256:bef0dcc223c04daa103a4df22f634d9353ffbd160e1fa4d75299157563c3460c
• Provider pair: openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001
• Blocking findings: 0
• Inline findings: 0
• Token source: GITHUB_TOKEN
• Runner: caf2f9a78989
• Run: https://rasterhub.com/rasterstate/fj/actions/runs/411

<!-- forseti:review {"version":2,"repo":"rasterstate/fj","pr":236,"head_sha":"32d71cff533797437823499f18111f349f50c7d7","provider_pair":"openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001","policy_version":"stub-policy-v1","prompt_version":"prompt-v2","context_fingerprint":"fnv64:01836121ae8ccafe","review_job_key":"sha256:bef0dcc223c04daa103a4df22f634d9353ffbd160e1fa4d75299157563c3460c","base_sha":"c7d8e0f3d22bc05592a711c8a42a24acc49c6e04","role":"summary","status":"current"} --> ## Forseti review No blocking findings from the lead reviewer. _No inline findings._ - PR: `rasterstate/fj#236` - Head SHA: `32d71cff5337` - Review job: `sha256:bef0dcc223c04daa103a4df22f634d9353ffbd160e1fa4d75299157563c3460c` - Provider pair: `openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001` - Blocking findings: `0` - Inline findings: `0` - Token source: `GITHUB_TOKEN` - Runner: `caf2f9a78989` - Run: https://rasterhub.com/rasterstate/fj/actions/runs/411

forgejo-actions approved these changes 2026-07-30 18:14:39 +00:00

forgejo-actions left a comment

Copy link

Forseti review

No blocking findings from the lead reviewer.

No inline findings.

• PR: rasterstate/fj#236
• Head SHA: 32d71cff5337
• Review job: sha256:bef0dcc223c04daa103a4df22f634d9353ffbd160e1fa4d75299157563c3460c
• Provider pair: openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001
• Blocking findings: 0
• Inline findings: 0
• Token source: GITHUB_TOKEN
• Runner: caf2f9a78989
• Run: https://rasterhub.com/rasterstate/fj/actions/runs/411

<!-- forseti:review {"version":2,"repo":"rasterstate/fj","pr":236,"head_sha":"32d71cff533797437823499f18111f349f50c7d7","provider_pair":"openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001","policy_version":"stub-policy-v1","prompt_version":"prompt-v2","context_fingerprint":"fnv64:01836121ae8ccafe","review_job_key":"sha256:bef0dcc223c04daa103a4df22f634d9353ffbd160e1fa4d75299157563c3460c","base_sha":"c7d8e0f3d22bc05592a711c8a42a24acc49c6e04","role":"summary","status":"current"} --> ## Forseti review No blocking findings from the lead reviewer. _No inline findings._ - PR: `rasterstate/fj#236` - Head SHA: `32d71cff5337` - Review job: `sha256:bef0dcc223c04daa103a4df22f634d9353ffbd160e1fa4d75299157563c3460c` - Provider pair: `openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001` - Blocking findings: `0` - Inline findings: `0` - Token source: `GITHUB_TOKEN` - Runner: `caf2f9a78989` - Run: https://rasterhub.com/rasterstate/fj/actions/runs/411

stephen merged commit 8363fc58aa into main 2026-07-30 18:54:49 +00:00

stephen deleted branch fix/pager-only-long-reads 2026-07-30 18:54:49 +00:00

stephen referenced this pull request from a commit 2026-07-30 18:54:49 +00:00

Merge pull request 'output: page only long read-only output, not writes or status' (#236) from fix/pager-only-long-reads into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

backlog

Accepted but not scheduled

  

blocked

Blocked or stalled waiting on external progress

  

converted

Converted into tracked work

  

forseti:changes-requested

Forseti review found unaddressed findings; cleared on a clean re-review at the current head.

  

opportunity

Potential product or business opportunity

  

p1

Priority 1

  

p2

Priority 2

  

p3

Priority 3

  

parked

Paused intentionally until later

No labels

backlog

blocked

converted

forseti:changes-requested

opportunity

p1

p2

p3

parked

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rasterstate/fj!236

No description provided.