Skip to content

fix(lifecycle-guard): return a verdict when the home directory is unresolvable - #79830

Closed
ruochu88s wants to merge 1 commit into
NousResearch:mainfrom
ruochu88s:pr/lifecycle-guard-unresolvable-home
Closed

fix(lifecycle-guard): return a verdict when the home directory is unresolvable#79830
ruochu88s wants to merge 1 commit into
NousResearch:mainfrom
ruochu88s:pr/lifecycle-guard-unresolvable-home

Conversation

@ruochu88s

Copy link
Copy Markdown
Contributor

What does this PR do?

The lifecycle guard raises instead of returning a verdict when ~ cannot be
expanded, which aborts the whole terminal tool call:

RuntimeError: Could not determine home directory.
  File "cron/lifecycle_guard.py", line 174, in _resolve_terminal_script_path
    path = Path(candidate).expanduser()

Path.expanduser() raises RuntimeError, not OSError, so the surrounding
except OSError handlers do not catch it. Because this guard runs on every
terminal command, the result is not "command blocked" and not "command failed" —
it is the guard itself crashing for a command that has nothing to do with
gateway lifecycle.

Encountered in real use while running an unrelated git add + python
pipeline.

Reproduction

With HOME / USERPROFILE / HOMEDRIVE / HOMEPATH unset:

'echo hello'         -> verdict=False
'bash ~/script.sh'   -> RAISED RuntimeError: Could not determine home directory.
'sh ~/nested/run.sh' -> RAISED RuntimeError: Could not determine home directory.
'bash ./local.sh'    -> verdict=False

Two conditions must coincide:

  1. the command references a ~/-relative script, and
  2. the environment lacks the home variables.

Commands without ~ never reach expanduser() and are unaffected.

Environments that land in this state: processes started with env -i, service
accounts, stripped containers, and CI runners. This repo's own
scripts/run_tests.sh has needed fixes for the same class of problem, since
env -i drops USERPROFILE on Windows.

The fix

  • Catch RuntimeError around expanduser() and fall back to the literal path.
    An unexpanded ~/x.sh simply will not be readable, and the caller already
    handles unreadable scripts.
  • Also guard Path.cwd(), so a deleted or inaccessible working directory cannot
    break the scan either.

Detection strength is unchanged. With HOME absent, a gateway lifecycle command
is still caught, and a relative script's body is still read and scanned — the
degradation is confined to ~ expansion.

How was it tested?

New: tests/cron/test_lifecycle_guard_unresolvable_home.py9 passing.

The no-HOME fixture asserts that Path('~').expanduser() really does raise
before running the case, so the tests cannot silently pass on a platform where
it resolves.

Cases: three ~-script shapes return a verdict instead of raising; unrelated
commands unaffected; a dangerous command is still detected; a relative script
body is still scanned; baseline behaviour with HOME present is unchanged
(including scanning a tilde script's body); a deleted cwd does not raise.

9 passed, 1 warning in 0.77s

Fault injection. Restoring the bare expanduser() call turns 4 assertions
red; the fix returns 9/9:

--- with the fix
    9 passed, 1 warning in 0.77s
--- fault injection: revert to bare expanduser()
    FAILED test_tilde_script_does_not_raise_without_home[bash ~/a.sh && echo done]
    FAILED test_missing_cwd_does_not_raise
    4 failed, 5 passed, 1 warning in 0.67s

Relationship to existing PRs (not a duplicate)

Two open PRs touch the same class of defect. Both were checked against the
current main:

If safe_expanduser() from #41870/#41881 lands first, this change should be
rewritten to call it instead of the local try/except — the fallback semantics
are identical, and I am happy to rebase onto that helper. The test file stays
valid either way, since it asserts observable behaviour (returns a verdict,
detection unchanged) rather than the mechanism.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor / internal change

Checklist

Platform

  • Windows
  • macOS
  • Linux

Reproduced and verified on Windows 10, but the defect is not
Windows-specific: Path.expanduser() raises the same RuntimeError on POSIX
when HOME is unset and the uid has no passwd entry. On Windows the trigger is
simply easier to hit, because both USERPROFILE and the
HOMEDRIVE+HOMEPATH pair must be present.

`_resolve_terminal_script_path()` called `Path(candidate).expanduser()`, which
raises `RuntimeError` — not `OSError` — when the home directory cannot be
determined. This guard runs on *every* terminal command, so one command
mentioning a `~/`-relative script in an environment without HOME/USERPROFILE
aborted the entire tool call:

    RuntimeError: Could not determine home directory.
      File "cron/lifecycle_guard.py", line 174, in _resolve_terminal_script_path
        path = Path(candidate).expanduser()

The failure is not a guard verdict and not a command error — the guard itself
raises before deciding anything, and the caller sees a crash for a command that
has nothing to do with gateway lifecycle.

Reproduced with HOME/USERPROFILE/HOMEDRIVE/HOMEPATH unset:

    'echo hello'         -> verdict=False
    'bash ~/script.sh'   -> RuntimeError: Could not determine home directory.
    'sh ~/nested/run.sh' -> RuntimeError: Could not determine home directory.
    'bash ./local.sh'    -> verdict=False

Two conditions must coincide: the command references a `~/`-relative script,
and the environment lacks the home variables. Affected environments include
processes started with `env -i`, service accounts, stripped containers, and CI
runners.

- Catch `RuntimeError` around `expanduser()` and fall back to the literal path.
  An unexpanded `~/x.sh` simply will not be readable, which the caller already
  handles.
- Also guard `Path.cwd()`, so a deleted or inaccessible working directory
  cannot break the scan either.

Detection is unchanged: a gateway lifecycle command is still caught with HOME
absent, and a relative script's body is still read and scanned, so the
degradation only affects `~` expansion.

Tests: tests/cron/test_lifecycle_guard_unresolvable_home.py, 9 cases. The
no-HOME fixture asserts `Path('~').expanduser()` really does raise, so the
tests cannot silently pass on a platform where it resolves.

Fault injection: restoring the bare `expanduser()` call turns 4 assertions red;
the fix returns 9/9.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists labels Aug 6, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Closing as superseded by #80258, which fixes this whole bug class architecturally rather than per-callsite: path candidates are sanitized once at the ingestion boundary (NUL/empty/unexpandable tokens rejected before any OS call), text from any read_remote_script callback is sanitized at the recursion boundary (NUL = binary = nothing to scan; >1 MiB = fail closed), the remote fallback read is bounded at the source (head -c, so oversized binaries never cross the wire), and the public guard is total by construction — an unexpected walk failure logs and falls back to the direct-scan verdict instead of breaking every terminal command.

Your report and fix targeted a real member of this class — thank you. The per-callsite patches kept leaving sibling frames exposed (#76762#77703#77780#78256 each crashed one frame away from the previous fix), which is why we went with the boundary fix instead of merging the fragments individually. #80258 carries regression tests for the NUL-path, binary-callback, oversized-read, unset-HOME, and walk-crash cases plus an adversarial never-raises sweep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants