Skip to content

fix(cli): stop pytest's -p plugin flag being read as --profile - #346

Merged
OmarB97 merged 1 commit into
mainfrom
fix/pytest-p-flag-profile-collision-fork-20260802
Aug 2, 2026
Merged

fix(cli): stop pytest's -p plugin flag being read as --profile#346
OmarB97 merged 1 commit into
mainfrom
fix/pytest-p-flag-profile-collision-fork-20260802

Conversation

@OmarB97

@OmarB97 OmarB97 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

_apply_profile_override() runs at import of hermes_cli.main and scans raw sys.argv for -p/--profile. pytest's -p selects a plugin, so under pytest that scan reads the plugin name as a profile name.

There is already a guard for this, and that's the interesting part. Step 1b was added for exactly this reason — its comment says "pytest's -p no:xdist would be misread as profile no:xdist". But it is purely syntactic: it rejects values that cannot be profile ids. A plugin name is usually a perfectly valid one. no:logging is rejected for its colon; anyio, cacheprovider, randomly are not. Those sail through to resolve_profile_env(), which exits 1 on a profile the user never asked for:

FileNotFoundError: Profile 'anyio' does not exist.
Create it with: hermes profile create anyio
SystemExit: 1

raised from the import, at setup of every test in any file that imports hermes_cli.main directly or transitively (tui_gateway.server does). Measured with -p poller_probe:

File Before After
tests/hermes_cli/test_config.py 183 errors 183 passed
tests/hermes_cli/test_inventory.py 51 errors
tests/hermes_cli/test_voice_wrapper.py 44 errors
tests/hermes_cli/test_custom_provider_identity.py 6 errors 6 passed
tests/test_tui_gateway_server.py 5 failures

The failure is nastier than the count suggests: the message names a profile nobody typed and points at hermes profile create, and nothing in the traceback mentions pytest's -p. It reads as a broken environment, not a flag collision. It cost me a full audit pass whose "errors" were entirely self-inflicted.

The fix guards the import-time call rather than the function. Under pytest this process's argv is pytest's, never a hermes command line, so there is nothing here to parse. Skipping the sticky active_profile branch there is a bonus: it currently lets a developer's hermes profile use rewrite HERMES_HOME inside the test suite.

Two details worth a reviewer's attention:

  • The guard has to sit at the call site, not inside the function. tests/hermes_cli/test_apply_profile_override.py calls _apply_profile_override() directly with a monkeypatched hermes argv, so an early return inside the function would neuter its own coverage. Only the import-time side effect is skipped; the function is unchanged.
  • _under_pytest() here is deliberately stronger than managed_scope._under_pytest, which checks only PYTEST_CURRENT_TEST. That variable is set per-test, so it is absent during collection — exactly when a test module's top-level import hermes_cli.main runs. sys.modules covers collection, fixture setup, and test bodies alike.

Related Issue

No separate issue. Found while auditing notification-poller leaks after #335/#336.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/main.py — added _under_pytest(), and guarded the module-level _apply_profile_override() call with it. The function body is untouched.
  • tests/hermes_cli/test_apply_profile_override.py — added TestPytestArgvIsNotAHermesCommandLine (4 tests):
    • test_reimport_with_pytest_plugin_flag_does_not_exit — the regression, using cacheprovider: a plugin name that is a valid profile id, so it defeats the old step-1b regex.
    • test_under_pytest_survives_unset_current_test_env — pins the collection-time case that PYTEST_CURRENT_TEST alone would miss.
    • test_under_pytest_is_true_during_a_test.
    • test_explicit_call_still_reads_a_real_hermes_argv — full profile resolution still works when called directly, despite the suite running under pytest.

How to Test

Reproduce on main — any plugin name that is a valid profile id:

python -m pytest tests/hermes_cli/test_custom_provider_identity.py -p cacheprovider -q

6 errors on main, 6 passed on this branch.

Verified so far on this branch:

  • A/B on the same file, same command: 6 errors → 6 passed.
  • tests/hermes_cli/test_config.py -p poller_probe: 183 errors → 183 passed.
  • tests/hermes_cli/test_apply_profile_override.py: 17 passed (13 existing + 4 new).
  • The production CLI path is unaffected — with pytest absent from sys.modules, a real hermes argv still resolves and strips the flag:
    pytest imported?  False
    under_pytest()  -> False
    HERMES_HOME     -> .../.hermes/profiles/coder
    argv after strip-> ['hermes', 'chat']
    
  • Ruff clean.

Targeted baseline-vs-fix A/B — done, no regressions (full detail in this comment). Same 12 files, same runner and env, run twice on the same machine:

Passed Failed
Baseline (unmodified main) 917 4
With fix 921 4

New failures from the fix: none. The failure sets are the same four tests byte-for-byte — all test_gateway_service.py::TestGatewaySystemServiceRouting::test_systemd_restart_*, failing on systemctl --user cannot reach the user D-Bus session (Linux-only tooling on a macOS host). The +4 passed is exactly the four new tests.

Files: test_apply_profile_override.py, test_profiles.py, test_env_loader.py, test_skills_subparser.py, test_container_aware_cli.py, test_web_server_profile_unification.py, test_desktop_spawn.py, test_gateway_service.py, test_setup.py, test_tui_gateway_server.py, test_profile_isolation_runtime.py, test_profile_db_session_sites.py — chosen as the ones that can actually reach the changed code path. test_env_loader.py and test_skills_subparser.py matter most: they are the only two in the repo that del sys.modules['hermes_cli.main'] and re-import, so they are the only ones that re-execute the guarded call. Both green.

The residual risk this covers: the only behaviour the change removes under pytest, besides the bug, is the import-time sticky-active_profile redirect. A test depending on that would be host-dependent, which is the class the hermetic runner exists to eliminate — worth proving rather than asserting.

This is a targeted A/B, not the full suite: the full sweep is ~9,500 tests per phase, so I narrowed it to the files that can reach the change rather than report hours late. CI covers the rest.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — not run; a targeted 12-file baseline-vs-fix A/B was run instead (see above), identical failure sets
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.6.0)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

`_apply_profile_override()` runs at import of `hermes_cli.main` and scans raw
`sys.argv` for `-p`/`--profile`. pytest's `-p` selects a plugin, so under pytest
that scan reads the plugin name as a profile.

Step 1b already guards this — it was added for exactly this reason ("pytest's
-p no:xdist would be misread as profile no:xdist") — but the guard is purely
syntactic: it rejects values that cannot be profile ids. A plugin name is
usually a perfectly valid one. `no:logging` is rejected for its colon;
`anyio`, `cacheprovider`, `randomly` are not. Those reach
`resolve_profile_env()`, which exits 1 on a profile the user never asked for:

    FileNotFoundError: Profile 'anyio' does not exist.
    Create it with: hermes profile create anyio
    SystemExit: 1

raised from the import, at setup of every test in any file that imports
`hermes_cli.main` directly or transitively (`tui_gateway.server` does).
Measured with `-p poller_probe`: `tests/hermes_cli/test_config.py` 183 errors,
`test_inventory.py` 51, `test_voice_wrapper.py` 44,
`tests/test_tui_gateway_server.py` 5 failures. The message names a profile
nobody typed and points at `hermes profile create`, and nothing in the
traceback mentions pytest's `-p`, so it reads as a broken environment.

Guard the import-time call rather than the function: under pytest this
process's argv is pytest's, never a hermes command line, so there is nothing
here to parse. The sticky `active_profile` branch is worth skipping there too —
it lets a developer's `hermes profile use` rewrite HERMES_HOME inside the suite.

The guard has to sit at the call site. `tests/hermes_cli/test_apply_profile_override.py`
calls `_apply_profile_override()` directly with a monkeypatched hermes argv, so
an early return inside the function would neuter its own coverage. Only the
import-time side effect is skipped; the function is unchanged.

`_under_pytest()` here is deliberately stronger than `managed_scope._under_pytest`,
which checks only `PYTEST_CURRENT_TEST`. That variable is set per-test, so it is
absent during collection — exactly when a test module's top-level
`import hermes_cli.main` runs. `sys.modules` covers collection, fixture setup,
and test bodies alike.

Four tests pin the regression, including the case the old guard misses: a plugin
name that IS a valid profile id (`cacheprovider`) must not exit on re-import.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@OmarB97
OmarB97 merged commit c371d1a into main Aug 2, 2026
32 checks passed
@OmarB97

OmarB97 commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

Targeted baseline-vs-fix A/B — no regressions

The run promised in the PR description has landed. Same 12 files, same runner, same env, run twice on the same machine: once on unmodified main, once with this branch applied.

Passed Failed
Baseline (unmodified main) 917 4
With fix 921 4

New failures introduced by the fix: none. The failure sets are not merely the same size — they are the same four tests, byte-identical:

tests/hermes_cli/test_gateway_service.py::TestGatewaySystemServiceRouting::test_systemd_restart_gracefully_restarts_running_service_and_waits
tests/hermes_cli/test_gateway_service.py::TestGatewaySystemServiceRouting::test_systemd_restart_recovers_failed_planned_restart
tests/hermes_cli/test_gateway_service.py::TestGatewaySystemServiceRouting::test_systemd_restart_reports_start_limit_hit
tests/hermes_cli/test_gateway_service.py::TestGatewaySystemServiceRouting::test_systemd_restart_uses_systemd_main_pid_when_pid_file_is_missing

These are pre-existing and unrelated: they fail on systemctl --user cannot reach the user D-Bus session, i.e. Linux-only tooling on a macOS host. They fail identically without this branch.

The +4 passed is exactly the four new tests in test_apply_profile_override.py (13 → 17).

Files covered

Chosen as the ones whose behaviour could plausibly depend on the import-time profile override — profile resolution, env loading, module re-import, and the largest transitive importer of hermes_cli.main:

test_apply_profile_override.py · test_profiles.py (155) · test_env_loader.py · test_skills_subparser.py · test_container_aware_cli.py · test_web_server_profile_unification.py · test_desktop_spawn.py · test_gateway_service.py (189) · test_setup.py · test_tui_gateway_server.py (~390) · test_profile_isolation_runtime.py · test_profile_db_session_sites.py

The two that matter most are test_env_loader.py and test_skills_subparser.py — the only two in the repo that del sys.modules['hermes_cli.main'] and re-import, so they are the only ones that re-execute the guarded call. Both green.

Scope, stated honestly

This is a targeted A/B, not the full suite. The full sweep over tests/hermes_cli/ tests/tui_gateway/ tests/test_tui_gateway_server.py is ~9,500 tests per phase and was going to take about three hours on a machine already running other work, so I narrowed it to the files that can actually reach the changed code path rather than let it run unattended and report late. CI still covers the rest.

Reproduced with:

scripts/run_tests.sh <the 12 files> -q --file-retries 0 --file-timeout 1800 -j 6

run against HEAD and against the branch, diffing the FAILED lines.

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.

1 participant