Skip to content

fix(tests): stop tests/ shadowing real packages on sys.path; de-flake LSP wait - #550

Merged
Kyzcreig merged 1 commit into
mainfrom
fix/blackhole-test-syspath-shadow
Aug 10, 2026
Merged

fix(tests): stop tests/ shadowing real packages on sys.path; de-flake LSP wait#550
Kyzcreig merged 1 commit into
mainfrom
fix/blackhole-test-syspath-shadow

Conversation

@Kyzcreig

Copy link
Copy Markdown
Collaborator

Two reds that survived every CI run

Both were invisible because the per-file runner reports them as 1 file where no tests ran and 1 test failed — neither reads as a defect.

1. tests/ was shadowing real packages on sys.path (22 collection errors)

tests/agent/test_endpoint_blackhole.py did:

sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))   # == tests/

tests/ contains packages whose names shadow real top-level ones — measured 10 collisions:

agent  cron  docker  gateway  hermes_cli  plugins  providers  tools  tui_gateway  website

So from hermes_cli import __version__ (reached from agent/auxiliary_client.py:1016) resolved to tests/hermes_cli and raised ImportError. The failure surfaced 22 collection ERRORs pointing deep into agent/auxiliary_client.py — nothing that looks like a path bug.

22 errors → 22 passed. Now resolves the repo ROOT from __file__, so worktrees and second clones behave identically.

The lint

tests/agent/test_no_tests_dir_on_syspath.py AST-walks every literal sys.path insert/append under tests/, statically evaluates the os.path.* expression, and fails if one resolves to tests/ itself.

It asserts the property, not one spelling — a novel phrasing of the same mistake is still caught. Ships with:

  • a positive control — the evaluator must resolve the known-bad idiom to tests/. A lint that silently evaluates nothing would pass forever.
  • a premise guard — fails if tests/ ever stops shadowing a real package, so it can't outlive its reason.

RED-proven: restoring the old one-liner fails the lint by name.

2. LSP wall-clock flake

test_slow_push_is_waited_for asserted a 2.0s ceiling against an 0.8s server-side delay — 1.2s of slack while spawning a real subprocess. That is a timing assertion (2.5× ratio), not a hang guard. Passed 5/5 in isolation, failed under 12-way parallel CI. Raised to 15.0s.

The invariant is preserved, not widened away. The inverse case — "an out-of-budget push must NOT satisfy the wait" — is owned by test_service_reports_no_data_not_stale_errors, which pins its own wait_timeout=1.0 against a server that never re-publishes. This test only ever proved the positive direction.

Load-immunity proven, not just idle re-runs: 3/3 green with 2× ncpu CPU burners at load 11.13.

Scope

Test-only. No production files touched.

… LSP wait

Two unrelated reds that survived every CI run because the per-file runner
reported them as "1 file where no tests ran" and "1 test failed".

1. tests/agent/test_endpoint_blackhole.py inserted os.path.join(dirname(
   __file__), "..") == tests/ onto sys.path. tests/ contains packages whose
   names shadow real top-level ones -- measured: agent, cron, docker, gateway,
   hermes_cli, plugins, providers, tools, tui_gateway, website (10 collisions).
   So `from hermes_cli import __version__`, reached from
   agent/auxiliary_client.py:1016, resolved to tests/hermes_cli and raised
   ImportError. Surfaced as 22 collection ERRORs that look nothing like a
   sys.path bug. 22 errors -> 22 passed. Now resolves the repo ROOT from
   __file__ so worktrees and second clones behave identically.

   Added tests/agent/test_no_tests_dir_on_syspath.py: an AST lint that
   statically evaluates every literal sys.path insert/append in tests/ and
   fails if one resolves to tests/ itself. It asserts the PROPERTY, not one
   spelling, so a novel phrasing of the same mistake is still caught. Ships
   with a positive control (the evaluator must resolve the known-bad idiom --
   a lint that evaluates nothing would pass forever) and a premise guard that
   fails if tests/ ever stops shadowing a real package.

   RED-proven: restoring the old one-liner fails the lint by name.

2. tests/agent/lsp/test_stale_diagnostics.py::test_slow_push_is_waited_for
   asserted a 2.0s ceiling against an 0.8s server-side delay -- 1.2s of slack
   while spawning a real subprocess, i.e. a timing assertion (2.5x ratio), not
   a hang guard. Passed 5/5 in isolation, failed under 12-way parallel CI.
   Raised to 15.0s.

   The invariant is preserved, not widened away: the inverse case ("an
   out-of-budget push must NOT satisfy the wait") is owned by
   test_service_reports_no_data_not_stale_errors, which pins its own
   wait_timeout=1.0 against a server that never re-publishes. This test only
   ever proved the positive direction.

   Load-immunity proven, not just idle re-runs: 3/3 green with 2x ncpu CPU
   burners at load 11.13.

Test-only change; no production files touched.
@Kyzcreig
Kyzcreig added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit 390db4f Aug 10, 2026
42 checks passed
@Kyzcreig
Kyzcreig deleted the fix/blackhole-test-syspath-shadow branch August 10, 2026 06:24
@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

FleetReview

Confidence: 1/5

Findings

  • P1 tools/environments/base.py:482 — Context Leakage
  • P2 tests/acp_adapter/test_acp_mcp_discovery.py:108 — Flaky Timeout
  • P1 tests/agent/test_no_tests_dir_on_syspath.py:77 — Relative paths
  • P1 tests/hermes_cli/test_update_check.py:57 — test_prefetch_non_blocking no longer tests anything: instant mock makes the elapsed assertion vacuous
  • P1 tests/hermes_cli/test_mcp_startup.py:52 — Unbounded stop.wait() turns an inline-discovery regression into an indefinite CI hang
  • P2 tests/hermes_cli/test_mcp_startup.py:91 — Timing Flake
  • P1 tests/hermes_cli/test_api_key_providers.py:821 — Un-joined loser probe threads escape the monkeypatch and issue real HTTPS requests to api.z.ai / open.bigmodel.cn
  • P2 tests/tools/test_mcp_stdio_init_timeout.py:80 — Flaky Timing
  • P1 tools/environments/base.py:531 — State Leakage
  • P1 tests/tools/test_snapshot_execution_identity_leak.py — Deletes the only regression guard for execution-identity markers leaking into the shared shell snapshot — and the source-side exclusion does not cover those vars

FleetReview provenance · models: B=gpt-5.6-sol, C=claude-code-opus-4-8, F=gpt-5.6-sol, G=grok-4.5 · cost: $7.59 · duration: 16m 10s · rounds: 1 · files examined: 3

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