feat(cli): add git-style pager to diff and log commands

[davidfirst]

`bit diff` and `bit log` printed everything at once with no pager. This adds git-style paging: interactive terminals get a pager, while AI-agents, CI, and piped output keep getting the full output at once (so agents never receive partial/paged data).

• Report output is piped through a pager (`less` by default) only when `stdout` is an interactive human terminal — not a TTY, or CI / `bit-cli-server` / a known AI-agent env → full output, no pager.
• `less` runs with `LESS=FRX`, so short output prints and returns to the prompt exactly as before; only long output actually pages, with colors preserved.
• Overrides: `--pager` / `--no-pager` flags, plus `BIT_NO_PAGER` and `BIT_PAGER`/`PAGER` env vars.
• If the pager can't launch, it falls back to a direct write so output is never lost.
• Opt-in per command via a new `Command.pager` flag; currently `diff` and `log`. The `--pager`/`--no-pager` flags register only on pageable commands (same pattern as `--token` on `remoteOp` commands).

[qodo-free-for-open-source-projects]

PR Summary by Qodo

feat(cli): git-style pager for bit diff and bit log report output

✨ Enhancement 🕐 20-40 Minutes

AI Description

• Add opt-in paging for command report output in interactive human terminals.
• Introduce --pager/--no-pager flags plus BIT_PAGER/PAGER/BIT_NO_PAGER env overrides.
• Enable paging for bit diff and bit log, while keeping JSON/CI/agents unpaged.

Diagram

graph TD
  U(["Interactive user terminal"]) --> Y["YargsAdapter"] --> C["diff/log commands"] --> R["CommandRunner.writeAndExit"]
  R --> D{"shouldUsePager?"} -->|"yes"| W["writeToPager()"] --> L{{"Pager (less)"}}
  D -->|"no"| O["Direct stdout write"]
  W -->|"launch failed"| O

  subgraph Legend
    direction LR
    _t(["Terminal"]) ~~~ _m["Module"] ~~~ _d{"Decision"} ~~~ _e{{"External process"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Stream report output to pager (avoid buffering full string)

• ➕ Handles extremely large diffs/logs without holding the entire output in memory
• ➕ Can start showing output sooner (better perceived performance)
• ➖ Requires changing the report contract to support streams/iterators or adding a parallel streaming API
• ➖ More complex error handling and exit-code coordination with the pager process

2. Adopt a dedicated pager library (instead of custom spawn logic)

• ➕ Potentially better cross-platform behavior and edge-case handling (Windows shells, quoting, etc.)
• ➕ Less bespoke logic to maintain (env parsing, spawn fallbacks)
• ➖ Adds a dependency and any transitive security/maintenance burden
• ➖ May still require custom behavior for agent/CI detection and Bit-specific rules

Recommendation: The PR’s approach (best-effort spawn of a user-configurable pager with a guaranteed direct-write fallback) is a good fit for the current CLI architecture where report returns a full string. If output size becomes a real memory concern, consider a follow-up to add a streaming report pathway and pipe incrementally into the pager.

Files changed (6) +101 / -0

Enhancement (6) +101 / -0

diff-cmd.tsOpt 'bit diff' into pageable report output +1/-0

Opt 'bit diff' into pageable report output

• Marks the diff command as pageable ('pager = true') so its report output can be routed through the pager when appropriate. JSON output remains unpaged via central pager decision logic.

scopes/component/component-compare/diff-cmd.ts

log-cmd.tsOpt 'bit log' into pageable report output +1/-0

Opt 'bit log' into pageable report output

• Marks the log command as pageable ('pager = true') so interactive terminals can page long history output. JSON output remains unpaged via central pager decision logic.

scopes/component/component-log/log-cmd.ts

command-runner.tsRoute report output through pager when enabled +6/-0

Route report output through pager when enabled

• Integrates pager decision + write path into 'writeAndExit()'. Attempts to pipe output to a pager first, and falls back to direct stdout write if pager launch fails so output is never lost.

scopes/harmony/cli/command-runner.ts

command.tsAdd 'Command.pager' opt-in flag to CLI command contract +9/-0

Add 'Command.pager' opt-in flag to CLI command contract

• Extends the 'Command' interface with an optional 'pager?: boolean' flag and documents intended behavior and overrides. This allows paging to be enabled per-command rather than globally.

scopes/harmony/cli/command.ts

pager.tsImplement pager detection and best-effort paging writer +72/-0

Implement pager detection and best-effort paging writer

• Adds logic to detect interactive human terminals and explicitly avoid paging in CI, daemon/server, and known AI-agent environments. Implements 'shouldUsePager()' override rules and 'writeToPager()' spawning ('less' default, 'LESS=FRX'), with safe fallback when paging is unavailable.

scopes/harmony/cli/pager.ts

yargs-adapter.tsRegister '--pager/--no-pager' only for pageable commands +12/-0

Register '--pager/--no-pager' only for pageable commands

• Conditionally adds global '--pager' and '--no-pager' options when a command opts into paging. Keeps the global flag surface minimal and consistent with other per-command global flags (e.g. token on remote ops).

scopes/harmony/cli/yargs-adapter.ts

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (7) 📘 Rule violations (0) 📜 Skill insights (0)

1. Pager exit drops output ✓ Resolved 🐞 Bug ☼ Reliability

Description

writeToPager() resolves success on any pager close (even non-zero exit), so
CommandRunner.writeAndExit() may skip the direct stdout write and terminate with no user-visible
output. This breaks the PR guarantee that output is never lost when the pager can’t be used (e.g.,
pager exits immediately due to invalid args or non-TTY conditions).

Code

scopes/harmony/cli/pager.ts[R59-67]

+    try {
+      const child = spawn(cmd, args, { stdio: ['pipe', 'inherit', 'inherit'], env });
+      // pager missing (ENOENT) or otherwise failed to launch => fall back to a direct write.
+      child.on('error', () => resolve(false));
+      child.on('close', () => resolve(true));
+      // ignore EPIPE that happens when the user quits the pager (e.g. "q") before all data is read.
+      child.stdin?.on('error', () => {});
+      child.stdin?.write(data);
+      child.stdin?.end();

Evidence

The pager helper treats any close as success, and the command runner uses that boolean to decide
whether to skip direct stdout output. This means a pager that starts but exits non-zero (without
emitting error) will cause the command to exit without printing the report.

scopes/harmony/cli/pager.ts[59-67]
scopes/harmony/cli/command-runner.ts[145-153]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`writeToPager()` currently resolves `true` on the pager's `close` event unconditionally. If the pager exits with a non-zero exit code (common for misconfiguration or non-TTY failures), `CommandRunner.writeAndExit()` treats paging as successful and **does not** write the output directly to stdout, losing the command output.
### Issue Context
- `CommandRunner.writeAndExit()` returns early when `writeToPager()` returns `true`, assuming output was successfully paged.
- `writeToPager()` uses `child.on('close', () => resolve(true))` without checking the exit status.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[59-67]
- scopes/harmony/cli/command-runner.ts[146-150]
### Suggested fix
1. In `writeToPager()`, resolve `true` only when the pager exits successfully (typically `code === 0`).
2. Resolve `false` on non-zero exit codes so the caller falls back to `process.stdout.write(data, ...)`.
3. Guard against double-resolve (`error` then `close`) with a small `done()` helper.
Example approach (conceptual):
- On `close`, `resolve(code === 0)`.
- Keep `on('error', ...) => resolve(false)` as-is, but ensure only the first event wins.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. isDaemon blocks paging 🐞 Bug ≡ Correctness

Description

isInteractiveTerminal() returns false when logger.isDaemon is true, but isDaemon is also used
by the interactive bit cli REPL to prevent process.exit(). As a result, pageable commands
(diff/log/etc.) will never page inside bit cli even when stdout is a real TTY.

Code

scopes/harmony/cli/pager.ts[R26-31]

+export function isInteractiveTerminal(): boolean {
+  if (!process.stdout.isTTY) return false;
+  if (logger.isDaemon) return false; // bit-cli-server: output travels over IPC, not a terminal
+  if (isEnvSet('CI')) return false;
+  if (AI_AGENT_ENV_VARS.some(isEnvSet)) return false;
+  return true;

Evidence

The new pager gate treats logger.isDaemon as always non-interactive, but the logger itself
documents isDaemon as being used for bit cli REPL, where stdout is a real terminal. This makes
shouldUsePager() unreachable in that interactive mode.

scopes/harmony/cli/pager.ts[26-31]
scopes/harmony/cli/cli.cmd.ts[74-83]
components/legacy/logger/logger.ts[81-105]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`scopes/harmony/cli/pager.ts:isInteractiveTerminal()` disables paging whenever `logger.isDaemon` is true, assuming this always means "bit-cli-server / IPC". In this repo `logger.isDaemon` is also used for the interactive `bit cli` REPL (and other non-exit contexts), so this check incorrectly disables paging even on a real interactive terminal.
### Issue Context
- `components/legacy/logger/logger.ts` explicitly documents `_isDaemon` as being used for `bit cli` (REPL) to avoid exiting.
- `scopes/harmony/cli/cli.cmd.ts` sets `legacyLogger.isDaemon = true` when entering the REPL.
- `scopes/harmony/cli/pager.ts` treats `logger.isDaemon` as a signal that stdout is not a terminal, which is not always true.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[26-31]
- scopes/harmony/cli/cli.cmd.ts[74-83]
- components/legacy/logger/logger.ts[81-105]
### Suggested approach
- Stop using `logger.isDaemon` as a proxy for "output travels over IPC".
- Gate the CLI-server/IPC case using a more specific signal, e.g. existing env markers such as `BIT_CLI_SERVER`, `BIT_CLI_SERVER_PTY`, `BIT_CLI_SERVER_TTY`, and/or `BIT_CLI_SERVER_NO_TTY` (see how the server is detected elsewhere), while allowing paging in `bit cli` REPL when `process.stdout.isTTY` is true.
- Keep `logger.isDaemon` behavior for exiting semantics, but not for interactivity detection.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. LESS set for non-less 🐞 Bug ≡ Correctness

Description

writeToPager() sets env.LESS='FRX' even when the selected pager command is not less, despite
the comment stating this is a less-specific behavior. This can unintentionally alter behavior of
non-less pagers or wrapper scripts that consult the LESS env var, changing user-configured
paging semantics.

Code

scopes/harmony/cli/pager.ts[R90-113]

+ * the pager binary is taken from BIT_PAGER, then PAGER, defaulting to `less`. an empty or `cat`
+ * pager means the user disabled paging via the env — honored unless `force` is set (the `--pager`
+ * flag), which falls back to a real pager so the flag is a true override. for `less` we set
+ * `LESS=FRX` (unless already set, matching git's default): keep ansi colors (-R), don't clear the
+ * screen on exit (-X), and quit if the output fits one screen (-F).
+ */
+export function writeToPager(data: string, force = false): Promise<boolean> {
+  return new Promise<boolean>((resolve) => {
+    // BIT_PAGER wins over PAGER; default to `less` only when neither is set. use `??` (not `||`)
+    // so an explicit empty string is preserved rather than treated as unset — that lets
+    // BIT_PAGER="" disable paging.
+    const configuredPager = process.env.BIT_PAGER ?? process.env.PAGER ?? 'less';
+    const tokens = parsePagerCommand(configuredPager);
+    // an empty / "cat" pager means the user disabled paging. honor that, unless `--pager` forces
+    // paging on, in which case fall back to a real pager so the flag is a true override.
+    const disabled = !tokens[0] || tokens[0] === 'cat';
+    if (disabled && !force) {
+      resolve(false);
+      return;
+    }
+    const [cmd, ...args] = disabled ? ['less'] : tokens;
+    const env = { ...process.env };
+    if (!env.LESS) env.LESS = 'FRX';
+

Evidence

The function comment explicitly scopes LESS=FRX to the less pager, but the implementation sets
LESS for any configured pager command because the assignment is unconditional after resolving
cmd/args.

scopes/harmony/cli/pager.ts[85-95]
scopes/harmony/cli/pager.ts[110-113]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`writeToPager()` unconditionally sets `env.LESS = 'FRX'` whenever `LESS` is unset, even if the configured pager is not `less`. The inline docs say this behavior is meant for `less` specifically.
### Issue Context
This can leak `LESS` into other pager programs or wrapper scripts and subtly change behavior in user environments where `BIT_PAGER`/`PAGER` points to something other than `less`.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[90-113]
### Suggested fix
Only set `env.LESS = 'FRX'` when the resolved pager command is actually `less` (and still only if `env.LESS` is not already set). For example, after selecting `[cmd, ...args]`, gate the `LESS` assignment on a basename check like `path.basename(cmd) === 'less'` (and consider `less.exe` on Windows).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. CI empty still pages ✓ Resolved 🐞 Bug ≡ Correctness

Description

isInteractiveTerminal() uses a truthy check for process.env.CI, so if CI is present but set to an
empty string, it won’t disable paging. In pseudo-TTY automation runs this can still allow paging
when output overflows the screen, contradicting the intended “CI never pages” behavior.

Code

scopes/harmony/cli/pager.ts[R18-24]

+export function isInteractiveTerminal(): boolean {
+  if (!process.stdout.isTTY) return false;
+  if (logger.isDaemon) return false; // bit-cli-server: output travels over IPC, not a terminal
+  if (process.env.CI) return false;
+  // presence-based, not truthiness: an agent that sets one of these to "" should still disable paging.
+  if (AI_AGENT_ENV_VARS.some((name) => process.env[name] !== undefined)) return false;
+  return true;

Evidence

The implementation disables paging for AI-agent env vars using presence checks (!== undefined) to
handle empty-string values, but uses a truthy check for CI, so CI="" won’t disable paging.

scopes/harmony/cli/pager.ts[18-24]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`isInteractiveTerminal()` currently disables paging only when `process.env.CI` is truthy. If CI is present but `""`, paging is not disabled.
### Issue Context
The same file explicitly uses **presence-based** checks for AI-agent env vars to handle empty-string values correctly.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[18-24]
### Suggested fix
Change the CI check to be presence-based, e.g.:
- `if (process.env.CI !== undefined) return false;`
This makes behavior consistent with the AI-agent env-var handling and with the PR’s “CI never pages” guarantee.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (6)

5. BIT_NO_PAGER empty ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

shouldUsePager() uses a truthy check for process.env.BIT_NO_PAGER, so if BIT_NO_PAGER is present but
set to an empty string, it won’t opt out of paging. This can unexpectedly enable paging in
environments that inject empty env vars.

Code

scopes/harmony/cli/pager.ts[R55-64]

+export function shouldUsePager(command: Command, flags: Flags, output: string): boolean {
+  if (!command.pager) return false; // command didn't opt-in to paging
+  if (flags.json) return false; // json is for machine consumption, never page it
+  // explicit CLI flags win over the BIT_NO_PAGER env var, so `--pager` can force paging for a
+  // single invocation even when the user exports BIT_NO_PAGER globally.
+  if (flags['no-pager']) return false;
+  if (flags.pager) return true; // explicit force-on, even when non-interactive / fits on screen
+  if (process.env.BIT_NO_PAGER) return false; // env opt-out (overridable by --pager above)
+  if (!isInteractiveTerminal()) return false;
+  return !fitsOnScreen(output); // only page when the output is longer than one screen

Evidence

shouldUsePager() uses if (process.env.BIT_NO_PAGER) which does not treat an empty-string value
as set, allowing paging to proceed when other conditions are met (interactive TTY + overflow).

scopes/harmony/cli/pager.ts[55-64]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`shouldUsePager()` currently treats `BIT_NO_PAGER` as enabled only when it’s truthy. If `BIT_NO_PAGER` is set to `""`, paging is not disabled.
### Issue Context
Within the same module, AI-agent env-vars are handled as **presence-based** (not truthy), specifically to avoid the empty-string edge case.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[55-64]
### Suggested fix
Use a presence-based check:
- `if (process.env.BIT_NO_PAGER !== undefined) return false;`
This ensures `BIT_NO_PAGER` consistently disables paging whenever it is set (regardless of value).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Empty agent env still pages ✓ Resolved 🐞 Bug ≡ Correctness

Description

isInteractiveTerminal() is documented to disable paging when any AI-agent env var is present, but
it only disables when the env var value is truthy. If an automation runner sets one of these
variables to an empty string, paging can incorrectly activate and produce interactive/partial output
for agents.

Code

scopes/harmony/cli/pager.ts[R11-23]

+const AI_AGENT_ENV_VARS = ['CLAUDECODE', 'CLAUDE_CODE', 'CURSOR_AGENT'];
+
+/**
+ * detect whether the output is going to an interactive human terminal.
+ * anything non-interactive (piped output, CI, ai-agents, bit-cli-server) gets the full output
+ * with no pager.
+ */
+export function isInteractiveTerminal(): boolean {
+  if (!process.stdout.isTTY) return false;
+  if (logger.isDaemon) return false; // bit-cli-server: output travels over IPC, not a terminal
+  if (process.env.CI) return false;
+  if (AI_AGENT_ENV_VARS.some((name) => process.env[name])) return false;
+  return true;

Evidence

The code comment states paging should be disabled when any AI-agent env var is present, but the
implementation uses a truthiness check (process.env[name]), which does not treat empty-string
values as present.

scopes/harmony/cli/pager.ts[6-23]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`isInteractiveTerminal()` intends to disable paging whenever an AI-agent env var is **present**, but it currently checks for a **truthy value** (`process.env[name]`). In Node, an env var can be present with an empty-string value (`""`), which is falsy, so paging may incorrectly remain enabled.
### Issue Context
The file comment says: “when any is present we never page”, which implies presence-based detection, not value-based detection.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[11-23]
### Suggested fix
Change the AI-agent env check to test presence rather than truthiness, e.g.:
- `if (AI_AGENT_ENV_VARS.some((name) => process.env[name] !== undefined)) return false;`
or
- `if (AI_AGENT_ENV_VARS.some((name) => Object.prototype.hasOwnProperty.call(process.env, name))) return false;`
Keep the rest of the logic unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. --pager not truly forced ✓ Resolved 🐞 Bug ≡ Correctness

Description

--pager is documented/implemented as a force-on override, but if BIT_PAGER is set to an empty
string or cat, writeToPager() returns false and the runner falls back to direct stdout (no
pager). This makes --pager ineffective for users who globally disable paging via BIT_PAGER,
contradicting the flag’s intended semantics.

Code

scopes/harmony/cli/pager.ts[R92-96]

+    const configuredPager = process.env.BIT_PAGER ?? process.env.PAGER ?? 'less';
+    const [cmd, ...args] = parsePagerCommand(configuredPager);
+    if (!cmd || cmd === 'cat') {
+      resolve(false); // paging disabled by the user (empty / "cat" pager)
+      return;

Evidence

The code explicitly treats --pager as a force-on decision, but the pager launcher still
short-circuits when BIT_PAGER is configured to disable paging, so paging does not occur even
though the flag says it should.

scopes/harmony/cli/pager.ts[54-63]
scopes/harmony/cli/pager.ts[87-96]
scopes/harmony/cli/yargs-adapter.ts[95-105]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`--pager` is intended to force paging, but `writeToPager()` treats `BIT_PAGER=""` and `BIT_PAGER=cat` as a hard disable and returns `false`, causing a direct stdout write even when `flags.pager` is set.
## Issue Context
- `shouldUsePager()` returns `true` when `flags.pager` is set (force-on).
- `writeToPager()` currently resolves `false` (disable) when `BIT_PAGER` is empty or `cat`.
- CLI help text describes `--pager` as “force paging the output through a pager”.
## Fix Focus Areas
- scopes/harmony/cli/pager.ts[54-96]
- scopes/harmony/cli/yargs-adapter.ts[95-105]
## Suggested fix
- Thread a `force` boolean into `writeToPager(data, { force })` (or similar), passed as `true` when `flags.pager` is set.
- When `force===true` and `BIT_PAGER` resolves to empty/`cat`, ignore that disable and fall back to `process.env.PAGER ?? 'less'` (or just `'less'`).
- Keep `--no-pager` and `BIT_NO_PAGER` as the authoritative opt-outs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Env blocks --pager ✓ Resolved 🐞 Bug ≡ Correctness

Description

shouldUsePager() checks BIT_NO_PAGER before the explicit --pager flag, so users cannot force
paging when BIT_NO_PAGER is set. This makes the advertised --pager override ineffective for
users who export BIT_NO_PAGER globally.

Code

scopes/harmony/cli/pager.ts[R54-60]

+export function shouldUsePager(command: Command, flags: Flags, output: string): boolean {
+  if (!command.pager) return false; // command didn't opt-in to paging
+  if (flags.json) return false; // json is for machine consumption, never page it
+  if (flags['no-pager'] || process.env.BIT_NO_PAGER) return false;
+  if (flags.pager) return true; // explicit force-on, even when non-interactive / fits on screen
+  if (!isInteractiveTerminal()) return false;
+  return !fitsOnScreen(output); // only page when the output is longer than one screen

Evidence

The current order makes BIT_NO_PAGER a hard-disable even when flags.pager is explicitly set,
contradicting the intention that --pager acts as a user override.

scopes/harmony/cli/pager.ts[48-61]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`shouldUsePager()` currently returns `false` when `BIT_NO_PAGER` is set even if the user explicitly passed `--pager`, because the env-var check occurs before the force-on flag check.
### Issue Context
Users often export `BIT_NO_PAGER=1` to disable paging by default, then expect `bit diff --pager` / `bit log --pager` to page for a single invocation.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[54-60]
### Suggested fix
Change precedence to honor explicit CLI flags first:
1) `--no-pager` should always win.
2) `--pager` should force paging even if `BIT_NO_PAGER` is set.
3) Only then apply env-var automatic disables (`BIT_NO_PAGER`) and interactivity checks.
Concretely, reorder logic to:
- if `!command.pager` return false
- if `flags.json` return false
- if `flags['no-pager']` return false
- if `flags.pager` return true
- if `process.env.BIT_NO_PAGER` return false
- if not interactive return false
- else `!fitsOnScreen(output)`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Empty BIT_PAGER ignored 🐞 Bug ≡ Correctness

Description

writeToPager() uses process.env.BIT_PAGER || process.env.PAGER, so setting BIT_PAGER to an empty
string cannot disable paging and will instead fall back to PAGER/less. This contradicts the
in-file comment that an empty pager disables paging and can unexpectedly launch a pager when the
user tried to turn it off.

Code

scopes/harmony/cli/pager.ts[R84-88]

+    const configuredPager = process.env.BIT_PAGER || process.env.PAGER;
+    const [cmd, ...args] = parsePagerCommand(configuredPager || 'less');
+    if (!cmd || cmd === 'cat') {
+      resolve(false); // paging effectively disabled by the user (empty / "cat" pager)
+      return;

Evidence

The implementation uses ||, which discards empty-string values, while the subsequent !cmd
check/comment explicitly claims empty should disable paging.

scopes/harmony/cli/pager.ts[82-89]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`writeToPager()` currently computes `configuredPager` using `process.env.BIT_PAGER || process.env.PAGER`, which treats an explicitly-set empty string as “unset”. That prevents `BIT_PAGER=""` from disabling paging, even though the code comments say empty should disable paging.
### Issue Context
We want: 
- `BIT_PAGER` to take precedence over `PAGER`
- `BIT_PAGER=""` (or `PAGER=""`) to disable paging
- default to `less` only when neither env var is set
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[84-88]
### Suggested change
Use nullish coalescing (or explicit checks) so empty-string is preserved:
- `const configuredPager = process.env.BIT_PAGER ?? process.env.PAGER;`
- only default to `'less'` when `configuredPager == null`, not when it’s `''`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Pager cmd parsing fails 🐞 Bug ☼ Reliability

Description

writeToPager() splits BIT_PAGER/PAGER on whitespace, so pager executables with spaces in their path
(or quoted commands) get truncated and spawn() fails to launch the pager. This makes paging silently
not work for valid pager configurations (common on Windows/macOS app paths).

Code

scopes/harmony/cli/pager.ts[R76-78]

+    const configuredPager = process.env.BIT_PAGER || process.env.PAGER;
+    const [cmd, ...args] = (configuredPager || 'less').trim().split(/\s+/);
+    if (!cmd || cmd === 'cat') {

Evidence

The pager command is constructed by splitting the environment string on whitespace, which cannot
represent an executable path that itself contains spaces; spawn() requires the full executable
path in the first argument.

scopes/harmony/cli/pager.ts[74-83]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`writeToPager()` parses `BIT_PAGER` / `PAGER` using a whitespace split, which breaks pager executable paths (or commands) that include spaces. This causes `spawn(cmd, args, ...)` to target a truncated/invalid command.
### Issue Context
Users may configure a pager with a full path that includes spaces (or with quoting). The current parsing logic can’t preserve the executable token correctly.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[74-90]
### Implementation notes
- Replace `.trim().split(/\s+/)` with a small quoted-arg tokenizer (supporting at least single/double quotes), or use an existing safe argv parser already available in the repo’s dependency set.
- Keep `shell: false` to avoid introducing shell-injection semantics; just improve tokenization.
- Add a small unit-level test (if this repo has a test harness for cli utils) for `BIT_PAGER` values like:
- `/Applications/My Pager/bin/less -R`
- `C:\\Program Files\\Git\\usr\\bin\\less.exe -R`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. ts-ignore hides type mismatch 🐞 Bug ⚙ Maintainability ⭐ New

Description

IsolatorMain.populateComponentsFilesToWriteForCapsule() adds // @ts-ignore before calling
PackageJsonTransformer.applyTransformers(), disabling compile-time verification for a critical
package.json transformation step. This makes future refactors riskier because TypeScript can no
longer enforce the transformer’s input contract at this call site.

Code

scopes/component/isolator/isolator.main.runtime.ts[R1503-1504]

+    // @ts-ignore capsule build can resolve two copies of @teambit/component.sources (peer variant), yielding a spurious TS2345 on PackageJsonFile; not reproducible in local tsc
    await PackageJsonTransformer.applyTransformers(component, packageJson);

Evidence

The PR introduces a @ts-ignore immediately before invoking
PackageJsonTransformer.applyTransformers(), which disables type checking at that call site.
applyTransformers() is typed to accept a PackageJsonFile from @teambit/component.sources, so
suppressing the error removes the compiler’s ability to validate that contract here.

scopes/component/isolator/isolator.main.runtime.ts[1498-1506]
scopes/workspace/modules/node-modules-linker/package-json-transformer.ts[17-44]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A new `// @ts-ignore` suppresses a type error when calling `PackageJsonTransformer.applyTransformers(component, packageJson)` during capsule builds, which removes type safety from an important packaging path.

### Issue Context
The callee (`applyTransformers`) only relies on `packageJson.packageJsonObject` and `packageJson.mergePackageJsonObject(...)`. The TS error described in the comment is caused by resolving two copies of `@teambit/component.sources`, i.e., a *nominal type identity* mismatch rather than an actual shape mismatch.

### Fix Focus Areas
- scopes/component/isolator/isolator.main.runtime.ts[1503-1504]
- scopes/workspace/modules/node-modules-linker/package-json-transformer.ts[17-44]

### Suggested fix
- Replace the `PackageJsonFile` parameter type in `PackageJsonTransformer.applyTransformers()` with a small structural interface that includes only what the function uses (e.g. `packageJsonObject: Record<string, any>` and `mergePackageJsonObject(obj: Record<string, any>): void`).
- Then remove the `@ts-ignore` in `isolator.main.runtime.ts`.
- If you still want a temporary suppression, prefer `@ts-expect-error` with a tracking ticket/issue so the compiler will fail if the error disappears or changes unexpectedly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

12. Misleading writeToPager docs 🐞 Bug ⚙ Maintainability

Description

writeToPager() is documented as resolving true once data is handed off to the pager, but it
actually resolves only after the pager process exits. This mismatch can cause future callers to
accidentally block until the user quits the pager when they expected a quick “handoff complete”
signal.

Code

scopes/harmony/cli/pager.ts[R88-126]

+/**
+ * pipe the given output through a pager (`less` by default). resolves to `true` once the data
+ * was handed off to the pager, or `false` when no pager could be launched - in which case the
+ * caller must write the data directly so nothing is lost.
+ *
+ * the pager binary is taken from BIT_PAGER, then PAGER, defaulting to `less`. an empty or `cat`
+ * pager means the user disabled paging via the env — honored unless `force` is set (the `--pager`
+ * flag), which falls back to a real pager so the flag is a true override. for `less` we set
+ * `LESS=FRX` (unless already set, matching git's default): keep ansi colors (-R), don't clear the
+ * screen on exit (-X), and quit if the output fits one screen (-F).
+ */
+export function writeToPager(data: string, force = false): Promise<boolean> {
+  return new Promise<boolean>((resolve) => {
+    // BIT_PAGER wins over PAGER; default to `less` only when neither is set. use `??` (not `||`)
+    // so an explicit empty string is preserved rather than treated as unset — that lets
+    // BIT_PAGER="" disable paging.
+    const configuredPager = process.env.BIT_PAGER ?? process.env.PAGER ?? 'less';
+    const tokens = parsePagerCommand(configuredPager);
+    // an empty / "cat" pager means the user disabled paging. honor that, unless `--pager` forces
+    // paging on, in which case fall back to a real pager so the flag is a true override.
+    const disabled = !tokens[0] || tokens[0] === 'cat';
+    if (disabled && !force) {
+      resolve(false);
+      return;
+    }
+    const [cmd, ...args] = disabled ? ['less'] : tokens;
+    const env = { ...process.env };
+    // LESS=FRX only makes sense for `less` itself; don't leak it into other pagers or wrapper scripts.
+    if (/^less(\.exe)?$/i.test(basename(cmd)) && !env.LESS) env.LESS = 'FRX';
+
+    try {
+      const child = spawn(cmd, args, { stdio: ['pipe', 'inherit', 'inherit'], env });
+      // pager missing (ENOENT) or otherwise failed to launch => fall back to a direct write.
+      child.on('error', () => resolve(false));
+      // resolve success only on a clean exit (code 0) or a signal (code null, e.g. the user killed
+      // it). a non-zero exit means the pager failed (bad args, couldn't render) without showing the
+      // output, so fall back to a direct write and never lose it.
+      child.on('close', (code) => resolve(code === 0 || code === null));
+      // ignore EPIPE that happens when the user quits the pager (e.g. "q") before all data is read.

Evidence

The JSDoc claims the promise resolves once output is handed off, but the code resolves only from the
pager child process 'close' event, which occurs when the pager exits.

scopes/harmony/cli/pager.ts[88-126]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`writeToPager()`’s JSDoc states it resolves `true` once the data was “handed off” to the pager, but the implementation resolves on the child process `close` event (i.e., after the pager exits). This is an API contract/documentation mismatch that can mislead future callers.
### Issue Context
Currently `CommandRunner.writeAndExit()` awaits `writeToPager()` and benefits from “wait until pager exits” behavior, so this is not a functional regression today. The risk is future reuse based on the docstring.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[88-126]
### Suggested fix
Either:
1) Update the docstring to explicitly say the promise resolves when the pager exits successfully (or when it fails to launch), **or**
2) If the intended API really is “resolve once handed off”, change the implementation to resolve after piping is established and add a separate awaitable for pager exit (or rename/split the function to make the blocking semantics explicit).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

13. Pager stdin ignores backpressure 🐞 Bug ➹ Performance

Description

writeToPager() writes the entire output string to child.stdin in one call and immediately ends
the stream without handling backpressure. For very large diff/log outputs this can cause avoidable
buffering and higher memory pressure during paging.

Code

scopes/harmony/cli/pager.ts[R98-109]

+    try {
+      const child = spawn(cmd, args, { stdio: ['pipe', 'inherit', 'inherit'], env });
+      // pager missing (ENOENT) or otherwise failed to launch => fall back to a direct write.
+      child.on('error', () => resolve(false));
+      // resolve success only on a clean exit (code 0) or a signal (code null, e.g. the user killed
+      // it). a non-zero exit means the pager failed (bad args, couldn't render) without showing the
+      // output, so fall back to a direct write and never lose it.
+      child.on('close', (code) => resolve(code === 0 || code === null));
+      // ignore EPIPE that happens when the user quits the pager (e.g. "q") before all data is read.
+      child.stdin?.on('error', () => {});
+      child.stdin?.write(data);
+      child.stdin?.end();

Evidence

writeToPager() currently does a single write(data) + end() regardless of size, and
DiffCmd.report() returns formatted diff output which can be large in practice.

scopes/harmony/cli/pager.ts[84-110]
scopes/component/component-compare/diff-cmd.ts[85-92]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`writeToPager()` writes a potentially large string to the pager stdin without respecting backpressure (`write()` return value / `drain`). This can cause unnecessary buffering overhead when paging large outputs.
### Issue Context
Commands like `bit diff` can produce large report strings, which are then passed into `writeToPager()`.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[98-109]
- scopes/component/component-compare/diff-cmd.ts[85-92]
### Suggested fix
Write to the pager in a backpressure-aware way, e.g.:
- Use `Readable.from(data)` (or chunk the string) and `pipe(child.stdin)`.
- Or check `child.stdin.write(...)` return value and await a `drain` event before continuing/ending.
- Keep the existing EPIPE suppression behavior for user-quit scenarios.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

14. fitsOnScreen splits full output ✓ Resolved 🐞 Bug ➹ Performance

Description

fitsOnScreen() splits the entire output string into a full lines[] array before it can early-exit,
adding avoidable allocation/CPU overhead for large diff/log outputs. This runs in the interactive
paging decision path and can inflate memory usage for large reports.

Code

scopes/harmony/cli/pager.ts[R33-42]

+export function fitsOnScreen(output: string): boolean {
+  const { rows, columns } = process.stdout;
+  if (!rows || !columns) return false;
+  const lines = output.replace(/\n$/, '').split('\n');
+  let usedRows = 0;
+  for (const line of lines) {
+    const width = (removeChalkCharacters(line) || '').length;
+    usedRows += width === 0 ? 1 : Math.ceil(width / columns);
+    if (usedRows > rows) return false; // stop early once we know it overflows one screen
+  }

Evidence

fitsOnScreen() currently splits the entire output before iterating, and shouldUsePager() calls
fitsOnScreen() in the automatic interactive-terminal path.

scopes/harmony/cli/pager.ts[33-44]
scopes/harmony/cli/pager.ts[52-59]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`fitsOnScreen()` does `output...split('\n')`, which materializes all lines even though the function only needs to know whether the output exceeds the terminal height.
### Issue Context
This function is called by `shouldUsePager()` in interactive terminals to decide whether to launch a pager. For large diffs/logs, splitting the entire output creates extra allocations.
### Fix Focus Areas
- scopes/harmony/cli/pager.ts[33-43]
- scopes/harmony/cli/pager.ts[52-59]
### Suggested change
Cap the split to at most `rows + 1` lines (sufficient to prove overflow) so the array doesn’t scale with total output size:
- `const lines = output.replace(/\n$/, '').split('\n', rows + 1);`
Keep the existing early-exit on `usedRows > rows`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 6c64e4b

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit c0ad1f7

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 785d8e5

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 163776d

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 6a333aa

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit c52c23b

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit f510bc7

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit dbc3b21

[qodo-free-for-open-source-projects]

Code Review by Qodo

New Review Started

This review has been superseded by a new analysis

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 4e42146

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 887a4f7

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit e3c7a75

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 0430dd1

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 9cfc75a

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 63b5835