Fix container --tty detection in subprocess mode

[xz-dev]

First, stdin not need --tty option, it only dependent --interactive option. Secendly, when running in subprocess mode, disable --tty option if parent process does not have a tty. This prevents the subprocess incorrectly detecting tty and enabling interactive features like ansible-config init.
For example, if you use ansible-navigator config init -m stdout > config, the subprocess will incorrectly detect tty and waiting 'q' to exit less and cause errors of config. This change avoids that.

More code see at here ansible_navigator. ansible_navigator enable input_fd even if in non-tty mode, It is understandable because they are able to control the input and output based on needs.

[Shrews]

I kinda have a feeling this might break pexpect and passwords (not sure we have testing mechanisms set up for that). Someone will have to dig into that aspect and do some validation there.

[Shrews]

I've done some manual testing in various scenarios (pexpect w/ passwords, subprocess in container connecting stdin to the input, etc) and it seems to work as expected. However, I'm not 100% certain I've covered all of the scenarios. This really needs some testing from the Controller team to validate that nothing on their side is affected negatively by this.

[xz-dev]

Is there anything else that needs testing? I'd be happy to continue improving this PR.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[xz-dev]

Hi @Shrews, picking this up again after a long hiatus. I've rebased and reworked the approach based on your earlier feedback.

The previous version checked sys.stdout.isatty() to decide per runner_mode — which raised the question of why stdout instead of stdin. The new version sidesteps that entirely by checking input_fd.isatty() directly on the actual fd object passed by the caller.

The logic is now in a _should_allocate_tty() method:

• pexpect mode: always returns True (unchanged, passwords still work)
• subprocess with input_fd: returns input_fd.isatty()
• no input_fd at all: returns False (same as pre-b5ead3b behavior)

This should address the Controller team concern — callers that don't pass input_fd (like AWX) see no behavior change at all.

[xz-dev]

How to test

Reproduce the bug from ansible/ansible-navigator#1607 at the runner level:

# test_tty_bug.py — run with: python test_tty_bug.py
from ansible_runner.config.command import CommandConfig
import sys, os

for d in ('/tmp/test_runner/artifacts', '/tmp/test_runner/project'):
    os.makedirs(d, exist_ok=True)

rc = CommandConfig(
    private_data_dir='/tmp/test_runner',
    process_isolation=True,
    container_image='ghcr.io/ansible/community-ansible-dev-tools:latest',
    process_isolation_executable='podman',
    input_fd=sys.stdin,
    output_fd=sys.stdout,
    error_fd=sys.stderr,
)
rc.prepare_run_command('ansible-config', cmdline_args=['init'])

print('runner_mode:', rc.runner_mode)
print('stdin.isatty():', sys.stdin.isatty())
print('Has --tty:', '--tty' in rc.command)

On unpatched runner, Has --tty is True even when stdin.isatty() is False — this is what causes less to hang inside the container when output is redirected.

With this patch, --tty is only added when stdin is actually a terminal. pexpect mode is unaffected (always gets --tty).

[Shrews]

This is going to need tests. The example you provided looks like an excellent start for an integration test, fwiw.

[xz-dev]

Added tests and found the fix was incomplete while writing them.

The original fix only checked input_fd.isatty(), which covers the CI/CD case (stdin is a pipe). But the real ansible-navigator#1607 scenario is: stdin IS a TTY, stdout is redirected (> ansible.cfg). In that case input_fd.isatty() still returns True, so --tty was still added.

Updated _should_allocate_tty() to require both input_fd and output_fd to be real terminals. pexpect mode is unchanged (always gets --tty).

Regarding the earlier concern about breaking pexpect/passwords — this change cannot affect that path. _should_allocate_tty() returns True immediately when runner_mode == 'pexpect', so the output_fd check is never reached for RunnerConfig (AWX/Controller) or any pexpect-mode CommandConfig.

Tests added:

• Unit: _should_allocate_tty() — all 5 branch combinations (pexpect, no fd, both TTY, stdin pipe, stdout pipe)
• Unit: CommandConfig containerization — both-TTY, stdin-piped, stdout-redirected, no-fd scenarios
• Integration: ansible-config init in a real container with file stdin (CI/CD scenario)
• Integration: ansible-config init in a real container with pty stdin + file stdout (exact #1607 scenario, uses pty.openpty())

[xz-dev]

@Shrews One more note on why _should_allocate_tty() checks both input_fd and output_fd — wanted to explain my thinking and would appreciate your take on whether I'm missing something.

My reasoning is that --tty is only meaningful when both ends are real terminals:

• stdin is a pipe/file, stdout is TTY — the container gets a PTY but stdin has no terminal behind it. podman itself warns "the input device is not a TTY" in this case, and interactive tools like less may hang waiting for keyboard input.
• stdin is TTY, stdout is a file — this is the exact navigator#1607 scenario (> ansible.cfg). ansible-config sees a PTY, invokes less, and it hangs or fills the file with ANSI escapes.

Checking only input_fd.isatty() (my first commit) missed the second case, which is why I added the output_fd check.

That said, I'm not 100% sure I've thought through every scenario — for example, are there any callers (Controller, AWX, or other integrations) that might pass a real-TTY input_fd but intentionally redirect output_fd while still expecting --tty? My assumption is no, but you'd know the downstream usage patterns much better than I do. Would be great if you could help sanity-check that, or point me at any edge cases I should be testing.

[Shrews]

Your logic, at least initially, seems to make sense to me. I do want to come back to this again and do some manual testing on my own (forgot what I did last time), and also want to run this through some controller testing eventually. Both of those may take a little bit, but will try not to let this linger too long for you.

As far as use cases downstream, unfortunately, I do have much input there. Users do crazy things that we aren't always privilege to... until we break something. 😉

Looks like you have some linting errors to fix up, but thanks for adding the new tests.

[xz-dev]

@Shrews Thanks for the review! I've fixed the linting errors in the latest commit, and also local tested the linting.

[xz-dev]

Fixed the CI test ab5e76b

[xz-dev]

https://github.com/ansible/ansible-runner/actions/runs/22068526553/job/63798923163?pr=1306

Seems not due to my code change:

            assert image_name in result_stdout
            error_msg = 'unauthorized'
            auth_file_path = res.config.registry_auth_path
            registry_conf = os.path.join(os.path.dirname(res.config.registry_auth_path), 'registries.conf')
>       assert error_msg in result_stdout
E       assert 'access to the requested resource is not authorized' in 'docker: Error response from daemon: unknown: failed to resolve reference "quay.io/kdelee/does-not-exist:latest": unexpected status from HEAD request to https://quay.io/v2/kdelee/does-not-exist/manifests/latest: 401 UNAUTHORIZED\n\nRun \'docker run --help\' for more information\n'

auth_file_path = '/tmp/ansible_runner_registry_awx_123_oen7a8s1/config.json'
error_msg  = 'access to the requested resource is not authorized'
f          = <_io.TextIOWrapper name='/tmp/pytest-of-runner/pytest-1/popen-gw0/test_invalid_registry_host_doc0/private_data_dir/artifacts/awx_123/stdout' mode='r' encoding='UTF-8'>
image_name = 'quay.io/kdelee/does-not-exist'
pdd_path   = PosixPath('/tmp/pytest-of-runner/pytest-1/popen-gw0/test_invalid_registry_host_doc0/private_data_dir')
private_data_dir = '/tmp/pytest-of-runner/pytest-1/popen-gw0/test_invalid_registry_host_doc0/private_data_dir'
registry_conf = '/tmp/ansible_runner_registry_awx_123_oen7a8s1/registries.conf'
res        = <ansible_runner.runner.Runner object at 0x7fd45ae22ba0>
result_stdout = ('docker: Error response from daemon: unknown: failed to resolve reference '
 '"quay.io/kdelee/does-not-exist:latest": unexpected status from HEAD request '
 'to https://quay.io/v2/kdelee/does-not-exist/manifests/latest: 401 '
 'UNAUTHORIZED\n'
 '\n'
 "Run 'docker run --help' for more information\n")
runtime    = 'docker'
tmp_path   = PosixPath('/tmp/pytest-of-runner/pytest-1/popen-gw0/test_invalid_registry_host_doc0')

test/integration/containerized/test_container_management.py:136: AssertionError
----------------------------- Captured stdout call -----------------------------
docker: Error response from daemon: unknown: failed to resolve reference "quay.io/kdelee/does-not-exist:latest": unexpected status from HEAD request to https://quay.io/v2/kdelee/does-not-exist/manifests/latest: 401 UNAUTHORIZED

Run 'docker run --help' for more information
============================= slowest 10 durations =============================
0.60s call     test/integration/containerized/test_container_management.py::test_invalid_registry_host[docker]
0.00s setup    test/integration/containerized/test_container_management.py::test_invalid_registry_host[docker]
0.00s teardown test/integration/containerized/test_container_management.py::test_invalid_registry_host[docker]
=========================== short test summary info ============================
FAILED test/integration/containerized/test_container_management.py::test_invalid_registry_host[docker] - assert 'access to the requested resource is not authorized' in 'docker: Error response from daemon: unknown: failed to resolve reference "quay.io/kdelee/does-not-exist:latest": unexpected status from HEAD request to https://quay.io/v2/kdelee/does-not-exist/manifests/latest: 401 UNAUTHORIZED\n\nRun \'docker run --help\' for more information\n'
============================== 1 failed in 1.24s ===============================
integration-devel: 2415 C exit 1 (1.53 seconds) /home/runner/work/ansible-runner/ansible-runner> pytest test/integration -vv -n auto --cov --cov-report html --cov-report term --cov-report xml --no-cov -vvvvv --lf pid=18984 [tox/execute/api.py:298]
  integration-devel: FAIL code 1 (1.79=setup[0.26]+cmd[1.53] seconds)
  evaluation failed :( (2.26 seconds)
Error: Process completed with exit code 1.

[Shrews]

Seems not due to my code change:

Yeah, I'll look into it and put up a fix.

[Shrews]

Seems not due to my code change:

Yeah, I'll look into it and put up a fix.

#1485

[xz-dev]

#1485

After it merge to master, will merge to this PR

[Shrews]

@xz-dev Did you, by any chance, test this change with ansible-navigator to verify it solves the issue there?

[xz-dev]

Yes, I've verified it with the latest commit.

Here is the recording: https://asciinema.org/a/QNLnDkgC83MOcLfj

[Shrews]

Thanks for your work on this!

[xz-dev]

Thanks @Shrews for the review and merge! Glad to see this finally land.