aws history show/list: UnboundLocalError instead of clear error when pager fails to start
- Dominant language
- Python
- Stars
- 17.3k
- Forks
- 4.6k
- Avg merge
- 1d 2h
- Merged PRs (30d)
- 13
Description
## Describe the bug
`OutputStreamFactory.get_pager_stream` (`awscli/utils.py`) is used by `aws history show` / `aws history list` to pipe output through a pager when running in a TTY. The `try`/`except OSError`/`finally` structure is meant to gracefully ignore `OSError`s (e.g. broken pipe when the pager is closed abruptly):
```python
@contextlib.contextmanager
def get_pager_stream(self, preferred_pager=None):
popen_kwargs = self._get_process_pager_kwargs(preferred_pager)
try:
process = self._popen(**popen_kwargs)
yield process.stdin
except OSError:
# Ignore IOError since this can commonly be raised when a pager
# is closed abruptly and causes a broken pipe.
pass
finally:
process.communicate()
```
The `try` block covers both process creation (`self._popen(...)`) and process usage (`yield process.stdin`). If `self._popen(...)` itself raises `OSError` — which happens whenever the configured pager binary doesn't exist (`FileNotFoundError` is an `OSError` subclass), e.g. `AWS_PAGER`/`PAGER` is misconfigured, or a minimal system has no `less` installed — then `process` is never assigned. The `except OSError: pass` swallows that exception, but the `finally: process.communicate()` then references the undefined local variable `process`, raising an unrelated `UnboundLocalError` instead of a clear error about the pager.
## Repro
```python
from awscli.utils import OutputStreamFactory
def bad_popen(*args, **kwargs):
raise OSError('no such file or directory: nonexistentpager')
factory = OutputStreamFactory(bad_popen)
with factory.get_pager_stream('nonexistentpager'):
pass
```
Output:
```
Traceback (most recent call last):
...
File "awscli/utils.py", line 253, in get_pager_stream
process.communicate()
UnboundLocalError: cannot access local variable 'process' where it is not associated with a value
```
## Real-world trigger
Any user of `aws history show`/`aws history list` running in a TTY with `AWS_PAGER`/`PAGER` set to a nonexistent or misspelled command gets this confusing `UnboundLocalError` traceback instead of a clear "pager not found" style error.
## Suggested fix
Only wrap the `yield` (process usage) in the try/except that's meant to swallow broken-pipe errors; let a failure to start the process propagate normally:
```python
process = self._popen(**popen_kwargs)
try:
yield process.stdin
except OSError:
pass
finally:
process.communicate()
```
I have a PR ready with this fix plus a regression test in `tests/unit/test_utils.py::TestOutputStreamFactory` that reproduces the `UnboundLocalError` against the current code and passes with the fix.
## Environment
- `aws-cli` develop branch (current)
- Confirmed no existing test in `tests/unit/test_utils.py::TestOutputStreamFactory` exercises `self._popen(...)` itself raising `OSError` (existing `test_can_silence_io_error_from_pager` only covers `process.stdin` access raising after `Popen` succeeds).
Contributor guide
Research direction
Start in awscli/utils.py at OutputStreamFactory.get_pager_stream, then read tests/unit/test_utils.py::TestOutputStreamFactory and its existing pager error test. Run the focused tests and confirm a pager-start failure reports the original error rather than UnboundLocalError while the broken-pipe case remains covered.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- aws, python
- Domain
- cli
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100