Skip to content

docs(delegate): explain --no-pager in PTY environments - #1062

Open
Qiyuanqiii wants to merge 3 commits into
alibaba:mainfrom
Qiyuanqiii:codex/issue-1059-no-pager
Open

docs(delegate): explain --no-pager in PTY environments#1062
Qiyuanqiii wants to merge 3 commits into
alibaba:mainfrom
Qiyuanqiii:codex/issue-1059-no-pager

Conversation

@Qiyuanqiii

@Qiyuanqiii Qiyuanqiii commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Description

Summary

  • Require git --no-pager in every delegation example that reads a diff or commit through Git.
  • Keep the skill guidance concise and actionable for the host agent.
  • Explain that disabling the pager does not bound captured output and recommend temporary-file reads in bounded chunks for large diffs.
  • Keep the canonical skill and Codex plugin copy synchronized.

Root Cause and Fix

Step 3 of the delegation skill previously instructed host agents to run git diff and git show directly. When an agent or CI runner allocates a pseudo-terminal (PTY), Git can treat stdout as interactive and launch its configured pager, usually less. The pager may then wait for input until the host terminates the process.

The examples now explicitly disable pagination:

Review mode Before Final
Range git diff <merge_base>..<to> -- <path> git --no-pager diff <merge_base>..<to> -- <path>
Commit git show <commit> -- <path> git --no-pager show <commit> -- <path>
Workspace git diff HEAD -- <path> git --no-pager diff HEAD -- <path>

Following review feedback, the skill contains only the actionable one-line reason for the flag. The detailed PTY, timeout, and exit-code explanation remains in this PR description rather than being presented to the agent during every review.

Disabling pagination solves the interactive wait but does not limit the amount of output written to stdout. The skill therefore also recommends inspecting the --stat output first, redirecting a potentially large diff to a temporary file, and reading it in bounded chunks.

Compatibility

  • This is a documentation-only change; OCR runtime behavior is unchanged.
  • No CLI flags, configuration, public APIs, or persisted data formats change.
  • --no-pager is a standard Git global option and is placed before the diff or show subcommand.
  • The canonical and plugin skill copies remain identical except for the plugin's existing mirror notice.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

Reproduction Environment

  • OS: Windows 25H2 (OS build 26200.9168), amd64
  • Git: 2.54.0.windows.1
  • Pager: less 692
  • Host execution: PTY allocated by the command runner
  • Reproduction diff: one tracked file with 226 insertions and 48 deletions
  • PR base: cfbb62e2296d7684dc648e27fe1b906e8c960f9c

Red/Green Reproduction

With TERM=xterm, GIT_PAGER=less, a PTY, and a four-second timeout:

Invocation Result
git diff ... Entered less, remained blocked, and was terminated after 4.083 seconds
git --no-pager diff ... Printed the complete diff and exited 0 in 104 milliseconds

The PowerShell/MSYS process boundary represented the forced termination as exit 2304. Unix shells and CI runners commonly report SIGKILL as status 137 (128 + 9), as reported in #1059. The important red/green result is the pager wait versus immediate completion.

Final Validation

  • mingw32-make check passed after the latest documentation change: license check, English-only check, go mod tidy, gofmt -s -w ., and go vet
  • The full race-enabled make test equivalent passed with GOFLAGS=-p=1 to constrain local package build concurrency
  • git diff --check passed
  • Both modified files use LF line endings after git add --renormalize .
  • The canonical and plugin skill copies differ only by the plugin's pre-existing mirror notice
  • The latest manual staged-diff technical-writing review completed; both wording findings were addressed in ff1e2d6

Checklist

  • My changes follow the project's documentation and command style
  • I have performed a self-review of the final diff
  • Existing tests pass locally with the changes
  • No new unit test is required for this documentation-only change
  • Both copies of the delegation skill are synchronized
  • Modified files use LF line endings

Known Limitations

  • The skill documents the safe invocation but does not programmatically enforce --no-pager for Git commands an agent constructs outside these examples.
  • --no-pager prevents an interactive pager wait but does not limit the amount of output written to stdout; bounded temporary-file reads remain necessary for large diffs.
  • The exact timeout exit code is host-specific even though the pager blocking behavior is the same.

Related Issues

Closes #1059.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review skipped: no items were selected.

Comment thread plugins/open-code-review/skills/open-code-review-delegate/SKILL.md Outdated

@Qiyuanqiii Qiyuanqiii left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Self-review — technical writing pass

The Git commands and pager behavior are correct, but I found two wording issues to address before marking this PR ready:

  1. In both SKILL.md copies, “so a PTY-based agent environment does not start an interactive pager and wait for input” assigns both actions to the environment and uses PTY without defining it. Git launches the pager; the pager waits. Suggested wording:

    Always pass --no-pager when reading diffs or file content through Git. Otherwise, in an agent environment that allocates a pseudo-terminal (PTY), Git may launch an interactive pager that waits for input.

  2. “Inspect the stat” is ambiguous. Git calls the --stat output a diffstat. Suggested wording:

    --no-pager does not bound the amount of output captured by the host. For a potentially large diff, inspect the --stat output first, redirect the full diff to a temporary file, and read it in bounded chunks.

The PR description should likewise replace “does not limit stdout size” with “does not limit the amount of output written to stdout.” It should also attribute status 137 to Unix shells or CI runners (128 + SIGKILL) rather than to Unix hosts generally.

No functional or command-level issues were found. The canonical and plugin skill copies remain synchronized except for the plugin mirror notice.

References: Git global options, Git diffstat terminology, and Bash exit-status semantics.

@Qiyuanqiii

Copy link
Copy Markdown
Contributor Author

Addressed the technical-writing findings in ff1e2d6:

  • expanded pseudo-terminal (PTY) on first use and assigned the actions to Git and the pager
  • replaced “inspect the stat” with “inspect the --stat output”
  • updated the PR description to describe the amount written to stdout and attribute status 137 (128 + 9) to Unix shells and CI runners

Both skill copies remain synchronized, and make check passes.

@Qiyuanqiii
Qiyuanqiii marked this pull request as ready for review August 25, 2026 20:02
@Qiyuanqiii
Qiyuanqiii requested a review from wu21-web August 25, 2026 20:02
@Qiyuanqiii

Copy link
Copy Markdown
Contributor Author

Just like #1061, I’ve also completed my review. You can take a look now. If there are problems, I’ll handle them after I get up around noon.

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.

docs(delegate): explain why git --no-pager is required in PTY-based agent environments

2 participants