Commit Graph

4 Commits

Author SHA1 Message Date
kshitijk4poor c5ff900761 fix: resolve the 33 F821 undefined names outside tui_gateway / feishu / godmode
Sweep of `ruff check . --select F821 --target-version py311`: 2,234 hits. 2,201 are left
alone on purpose: tui_gateway (2,169; bind_module rebinds bodies onto server.py globals,
all names verified to resolve there), the Feishu adapter (27; globals().update() SDK
binding) and the godmode script (5; dead standalone script). The other 33 were all
genuine defects. No lint config change; no TYPE_CHECKING escape hatches — every
annotation names a real, imported type; ty on the touched files: 0 new diagnostics.

- gateway/slash_commands.py: HISTORY_UNREADABLE never imported after #102117
  → NameError on the /btw error branch (same one-liner as #102952).
- gateway/platforms/whatsapp_common.py: `-> Path` return annotation with no Path
  import (the body uses `_Path`). Never raised at runtime thanks to
  `from __future__ import annotations`, but `typing.get_type_hints()` and ty
  both fail on it.
- gateway/run.py: ActivityProvenance imported at module level
  (agent.session_activity has no gateway deps); stringly annotation and the
  lazy in-function import are gone.
- tools/patch_parser.py: PatchResult imported at module level; real return
  annotation. The "avoid circular import" lazy import guarded a cycle that
  does not exist (file_operations_common never imports patch_parser).
- gateway/platforms/helpers.py: base.py imports helpers at module level, so
  MessageEvent cannot be named here; TextBatchAggregator only reads .text and
  .source, so it is typed by a BatchableEvent Protocol that MessageEvent
  satisfies structurally.
- tools/mcp_tool_sampling.py: mcp_tool imports this module, so MCPServerTask
  cannot be named here; ElicitationHandler only reads
  owner._pending_call_context, typed by an ElicitationOwner Protocol.
- plugins/platforms/sms/adapter.py: aiohttp is an optional dep ([messaging] extra) →
  module-level try/except ImportError binding `aiohttp = web = None`, the pattern the
  homeassistant / webhook / whatsapp_cloud adapters already use. Retires three lazy
  in-function imports and the `_aiohttp_available()` wrapper; `_handle_webhook` typed
  `web.Request -> web.Response`.
- plugins/platforms/teams/summary_writer.py: plain module-level `import httpx` — httpx is a
  hard core dependency (pyproject `httpx[socks]==0.28.1`), so the lazy import and the
  "imported on every CLI start" docstring premise were both wrong (plugin discovery never
  imports this module; it is reached only via the Teams adapter / meeting pipeline).

Tests:
- tests/hermes_cli/test_config.py: a test body orphaned by the wave-1 prune
  (6b81590c55) sat inside the class as dead code with self/tmp_path unbound
  — header restored, so the v11→12 custom_providers migration is covered.
- tests/tools/test_mcp_tool.py: @staticmethod recursing on `self` in the
  win32 branch; call portalocker directly.
- tests/test_background_review_list_shapes.py: main() still ran 3 pruned tests.
- tests/agent/test_cursor_optimizations_parity.py: bench() used names only
  imported inside a sibling test.
- GatewayRunner / FeishuAdapter / Dict / Optional: missing imports.
2026-09-07 22:47:33 +05:30
Teknium eba201ff6e refactor(agent/background_review): split _run_review_in_thread into fork/summary phases, extract fork kwargs and action-line helpers 2026-09-02 18:41:26 -07:00
Teknium 6b81590c55 test: prune low-value tests suite-wide (wave 1) — 46,820 → 28,106 test functions
Systematic prune per AGENTS.md test policy, one pass over every major
test tree (gateway, hermes_cli, tools, agent, run_agent, plugins, cli,
cron, tui_gateway, honcho/openviking, root-level):

- DELETE: source-reading tests (read_text/getsource on prod files),
  change-detector tests (exact catalog counts, model-name snapshots,
  config version literals), mock-echo tests (assert a mock returns what
  it was told), assertion-free/trivial tests, near-duplicate
  parametrizations (boundaries + one representative kept), async/sync
  twin duplicates, cosmetic within-file variations.
- KEEP (mandatory): security/redaction/approval guards, message-role
  alternation invariants, prompt-caching/deterministic-call-id
  invariants, issue-number regression tests (deduped), E2E tests.
- 6 test files deleted outright (script-style/no-assert or fully
  redundant); conftest.py, fakes/, fixtures/ untouched.
- tests/acp/conftest.py added: autouse fixture stubs the live
  models.dev/GitHub/Copilot/Anthropic inventory fetches that ACP server
  tests performed on every session create — test_server.py 147s → 3.4s,
  and the tests are now genuinely hermetic.
- Sleep-based slowness shrunk where safe (codex_ttfb_watchdog,
  compression_concurrent_fork, etc.); no wall-clock assertion tightened.

Verification: full hermetic suite via scripts/run_tests.sh —
2439 files, 31,130 tests passed, 0 failed, 0 flaky retries, 315s wall
(baseline: 583s wall, 13,564s subprocess CPU).
2026-07-29 13:10:23 -07:00
Ahmett101 2e828d4b75 fix(background-review): guard summarize against list-shaped tool responses (#59437)
`summarize_background_review_actions` was structured on the assumption
that every parsed tool response is a fully-typed dict-of-fields. In
practice the memory/skill tools — and their wrappers over Mem0 OSS and
the skill_manage MCP server — sometimes serialize `_change` as a list
or scalar, and clamp `operations` to a single string when the field
came in via a partial JSON bridge.

The original code did the equivalent of:
    change = data.get("_change", {})
    change.get("description", "")

so when `_change` was a list the inner .get crashed with
`AttributeError: 'list' object has no attribute 'get'`, every ~10
turns the user saw the entire background review collapse.

Three defensive guards in summarize_background_review_actions:

- `call_details.get(tcid, {})` → `call_details.get(tcid) or {}` plus
  `isinstance(detail, dict)` coercion. Catches stale scalar/None
  values when a fork inherits partial state from a stale tool_call_id.
- `operations = detail.get("operations") or []` → `isinstance(ops_raw, list)`
  coerce, then per-entry isinstance check before `.get()`. Skips
  non-dict items without raising; an entire surrounding review no
  longer goes down because one entry was malformed.
- `change = data.get("_change", {})` → `isinstance(change_raw, dict)`
  coerce. The originally-reported crash class for skill_manage with
  list-shaped _change now falls through to the generic summary path.

And the caller in `_run_review_in_thread` is wrapped in a try/except
that maps any residual summarize exception to `actions = []` and
emits a 'partial results' warning, so even an entirely unanticipated
shape won't take down the outer review — the user only sees
'Background memory/skill review failed' instead of the prior hard
crash that lost every successful action the fork had completed.

Tests: tests/test_background_review_list_shapes.py — standalone
pytest-free runner, 7/7 PASS:
  a_change_as_list_does_not_crash       (originally-reported shape)
  a_change_as_int_does_not_crash        (scalar fallback)
  b_operations_as_string_treated_as_empty
  b_operations_as_none_treated_as_empty
  c_operations_contains_non_dict_entries (verbose-mode per-entry filter)
  d_detail_non_dict_replaced_with_empty
  e_call_defends_via_try_except         (structural anchor)

Refs NousResearch/hermes-agent#59437
2026-07-06 03:28:30 -07:00