Skip to content

fix(cmd): split PAGER into program and arguments - #208

Open
feiiiiii5 wants to merge 1 commit into
openai:mainfrom
feiiiiii5:fix/pager-command-arguments
Open

feiiiiii5 wants to merge 1 commit into
openai:mainfrom
feiiiiii5:fix/pager-command-arguments

Conversation

@feiiiiii5

Copy link
Copy Markdown

Summary

$PAGER is a command line in practice, but both pager implementations in pkg/cmd/cmdutil*.go resolve the whole value as a single executable name. With the very common PAGER="less -R", any ... list command whose output is longer than the terminal fails instead of paging.

Problem

streamToPagerWithPipe and the Unix openSocketPairPager each did:

pagerProgram := os.Getenv("PAGER")
if pagerProgram == "" {
	pagerProgram = "less"
}
pagerPath, err := exec.LookPath(pagerProgram)
...
cmd := exec.Command(pagerProgram)                       // "less -R" as one argv[0]
pid, err := syscall.ForkExec(pagerPath, []string{pagerProgram}, procAttr)

git, gh and man all document PAGER as program [args...], and those tools are what most users set it beside, so PAGER="less -R" is the configuration this CLI meets in the wild. Both call sites then fail with:

exec: "less -R": executable file not found in $PATH

streamOutput has one production caller, ShowJSONIterator (pkg/cmd/cmdutil.go:575), which 68 generated list command sites return through, so the failure is not confined to one command. streamOutputOSSpecific does fall back to streamToPagerWithPipe when the socket pager fails, but the fallback resolves PAGER the same way, so both paths end in the same error.

Fix

One pagerCommand() helper, used by both paths:

  • trims the value, falls back to less when it is empty;
  • splits the trimmed value with strings.Fields and returns argv when the first word resolves via exec.LookPath;
  • if the first word does not resolve, keeps the whole value as one element, so a program path containing spaces (which does resolve today) keeps resolving.

The socket pager now passes that argv to syscall.ForkExec, the pipe pager to exec.Command.

No shell is involved. The value is split into argv words only; nothing is ever passed to sh -c, so docs/architecture/security-model.md:88-91 (a local operator already controls PAGER) is unaffected in either direction: this neither adds an execution capability nor relies on one.

Tests

pkg/cmd/cmdutil_pager_unix_test.go (new, //go:build !windows like the existing cmdutil_output_errors_unix_test.go):

  • TestPagerCommand — 8 cases: unset, blank, bare program, bare program with surrounding whitespace, program plus two arguments, trimmed value with an argument, a path containing spaces, and an unresolvable first word.
  • TestStreamToPagerPassesPagerArguments — drives both pager implementations through the same paths map already used by TestStreamOutputErrorOrigins, with a #!/bin/sh fixture that records $1 and echoes stdin. PAGER="<fixture> --no-init", and the test asserts the fixture received --no-init and the streamed bytes. The socket subtest asserts w.Name() == "parent-socket" so it cannot silently be covered by the pipe path.

Red on the base commit 0169bff (test file copied into a scratch worktree of 0169bff):

--- FAIL: TestStreamToPagerPassesPagerArguments/pipe
    Received unexpected error:
    exec: ".../001/pager --no-init": stat .../001/pager --no-init: no such file or directory
--- FAIL: TestStreamToPagerPassesPagerArguments/socket
    Received unexpected error:
    exec: ".../001/pager --no-init": stat .../001/pager --no-init: no such file or directory

Green on this branch, both subtests.

Validation

cwd /Users/fei/Desktop/开源/.runtime/2026-09-20/oa/src, base 0169bff, go1.25.0 darwin/arm64, network proxies unset, -count=1:

Command Result
go test ./pkg/cmd -run 'TestPagerCommand|TestStreamToPagerPassesPagerArguments' -v PASS (10 subtests)
go test ./pkg/cmd -race -run 'TestPagerCommand|TestStreamToPagerPassesPagerArguments|TestStreamOutput|TestShowJSON' ok
go test ./internal/... ok, 0 failures
go test ./pkg/cmd 284 failures — identical set on base and on this branch (diffed by test name: new failures on branch: [])
./scripts/lint ok
go vet ./... ok
gofmt -l on the three files no output
GOOS=windows GOARCH=amd64 go build ./... and go test -c ./pkg/cmd ok
GOOS=linux GOARCH=amd64 go test -c ./pkg/cmd ok
go mod verify / go mod tidy -diff all modules verified / no diff

Not run, and why:

  • ./scripts/test (full go test ./...) needs the reviewed Steady mock server on 127.0.0.1:4010, which downloads a pinned Deno and fetches the pinned fork. The 284 pkg/cmd failures are all the same precondition error, Mock server is not running on localhost:4010, plus one pre-existing failure I confirmed reproduces unchanged on 0169bff: TestFilesCreateCLICancelClosesStalledFIFOcancellation must close the owned FIFO reader. CI covers the mock-server suite.
  • A real interactive TTY session. streamOutput only reaches a pager when stdout is a terminal, so under go test it short-circuits to streamToStdout; the new tests therefore call streamToPagerWithPipe / streamOutputOSSpecific directly, which is how the existing cmdutil_output_errors_unix_test.go tests reach them too.

Both touched files are wholly handwritten (no Code generated header), so nothing in the generated baseline changes; the Castiron custom-code report will post its own view of the diff.

AI-assisted: an AI coding agent produced this change under the account owner's standing instruction for this repo, ran every command above and read its output. No human reviewed the diff before it was opened.

$PAGER is written as a command line by the tools this CLI is used beside
(git, gh, man), so PAGER="less -R" is a common configuration. Both pager
implementations resolved the whole value as one executable name, so a
command that paged its output failed with
`exec: "less -R": executable file not found in $PATH` instead of
showing the output.

pagerCommand() now splits the value into argv for both the pipe pager and
the Unix socket-pair pager. Splitting is argv-only, never through a shell,
and when the first word does not resolve the whole value is kept so an
executable path containing spaces keeps working.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant